feat: jig Milestone 2 — definitions, runs, admission triggers, publish, and Codex - #4
Conversation
Second CodeRabbit pass on the review fixes. - The process-group guard narrowed int64 to int without checking the conversion, so a value above MaxInt32 could truncate onto 0 or 1 on a 32-bit build — back onto the syscall the guard exists to prevent. The round-trip check now lives in the predicate, so the signal path, the identity gate, and manifest validation all inherit it. - Result() waited for stdout EOF before reaping, so a descendant holding the stream blocked it indefinitely on the normal-exit path; stderr was a buffer rather than a file, so Cmd.Wait blocked for the same reason and waiting on exit was not an available fallback. jig now owns both pipes, waits on process exit, releases the group, then drains under a bound. - worktreeRegistered swallowed resolution failures into false, making 'could not tell' indistinguishable from 'not registered' in a fail-closed path. - A reconcile assertion read a missing manifest field as an empty one.
… U10) U5: definitions edit in place with generation counters; runs freeze source, parameters, and composed prompt by value with a base SHA pinned per target at admission; fan-out to independent jobs with recomputed accepted/failed/mixed aggregation; re-admit-at-head as the explicit escape hatch. Prompt composition fences untrusted context and neutralizes forged markers. Repository identity normalization hoisted to protocol so the cap skip-over, ledger, and clone source share one implementation. U7: publish is a fenced critical section — authorize, perform, record per step, each carrying the lease token. Attempt-scoped branch, find-or-create PR by head ref, remote-ref proof, publish-only retry that skips proven steps without re-running phases. Staging uses the engine's declared changed paths and aborts if anything else reached the index. Stray branches a zombie pushed before expiry are reported, not deleted — the fence cannot fence GitHub. U10: Codex adapter with capabilities probed rather than declared. turn.completed.usage is cumulative per thread, so the adapter reports per-send deltas; context occupancy is absent from the exec stream and stays zero rather than misreporting the cumulative figure. A declared tool allowlist fails the send, since codex exec cannot honor one and silently dropping it would widen the agent's reach.
…(U6) Schedule and GitHub-polling triggers, created disabled. Hand-rolled five-field cron with IANA zones whose DST semantics fall out of a UTC-minute walk: a fall-back overlap hour fires twice, a spring-forward local minute never fires. Wake after downtime admits the single stored overdue instant and jumps to the next future match rather than catching up. Exactly-once admission: the occurrence is reserved, resolution happens outside any transaction, then the run and the dispatched mark commit together — so a crash rolls back the run with its mark and startup recovery re-drives without risking a second run. GitHub polling uses fixed argument vectors, strict JSON decoding, a match limit, and repository-scoped URL checks, with a distinct diagnostic per failure mode. Issue and PR text enters as untrusted context sections; PR head SHAs pin without touching the network. Also closes R12's third conjunct server-side: CompleteAttempt now refuses 'accepted' without a recorded proof of publish, so the guarantee lives in the transaction rather than worker-side only.
📝 WalkthroughWalkthroughThis PR adds durable definitions, runs, schedules, GitHub admission, publish proof, runtime adapters, worker trace ingestion, CLI commands, an embedded UI, migrations, release automation, and integration tests. ChangesJig platform integration
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 16
🧹 Nitpick comments (8)
internal/controlplane/store_test.go (1)
308-308: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove the unused
branchvariable and its placeholder assertion.
branchis computed at Line 308 but is only read by thebranch == ""check at Lines 333-335.protocol.PublishBranchis already covered by the publish-ledger tests, so this assertion adds no coverage and only exists to keep the variable used.♻️ Proposed fix
- branch := protocol.PublishBranch(claim.Job.ID, claim.Attempt.AttemptNumber) if _, err := recordStep(store, claim, tokenA, protocol.PublishStepPush, shaA, ""); err != nil {if unpublished.State != protocol.AttemptAcceptedUnpublished { t.Fatalf("attempt state = %q, want accepted_unpublished", unpublished.State) } - if branch == "" { - t.Fatal("the attempt-scoped branch is empty") - } }Also applies to: 333-335
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/controlplane/store_test.go` at line 308, Remove the unused branch variable declaration near the claim setup and delete the corresponding branch == "" placeholder assertion, leaving the surrounding test behavior unchanged.internal/worker/publish.go (2)
1074-1079: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuePopulate
Numberon a created pull request, or drop the field from the create path contract.
FindOrCreatePullRequestreturnsPullRequest{URL: url, State: "open"}after a create, soNumberstays zero, whilefindPullRequestreturns the real number. The current callers only readURL, so nothing breaks today. A later consumer that readsNumberwould see zero only for freshly created pull requests, which is a hard defect to trace.Parse the number from the URL, or document that
Numberis only set on the find path.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/worker/publish.go` around lines 1074 - 1079, Update FindOrCreatePullRequest’s create path to populate PullRequest.Number by parsing the pull request number from the URL returned by firstPullRequestURL, matching the value provided by findPullRequest; handle an unparseable URL through the existing publish failure path rather than returning Number as zero.
791-794: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winLoad
manifest.WorktreePathinstead of reconstructing the worktree path.
RetryPublishrebuilds the worktree path fromw.worktreeRoot()and the attempt ID. The manifest already records the path thatprepareAttemptcreated (line 32), anddisposeAttemptWorktree(called at line 810 in the same function) loads and usesmanifest.WorktreePath. If the worktreeRoot directory layout ever changes, the retry will silently use a stale reconstructed path while initial publish and cleanup use the manifest, causing them to diverge. Readingmanifest.WorktreePathdirectly viaw.manifests.load(retry.Attempt.ID)ensures consistency and makes the manifest the single source of truth for the worktree location.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/worker/publish.go` around lines 791 - 794, Update RetryPublish to load the attempt manifest through w.manifests.load(retry.Attempt.ID) and use its WorktreePath for target.worktreePath, removing the reconstructed filepath.Join(w.worktreeRoot(), retry.Attempt.ID) path and related stat check.internal/worker/publish_test.go (1)
702-742: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDocument that the exit gate leaves a branch and a pull request behind.
Each run of
TestMilestone2ExitGatepushes a new attempt-scoped branch and opens a new pull request on the scratch repository, and nothing removes them. Repeated runs accumulate open pull requests. Add a line to the doc comment that states the operator must close and delete them, or close the pull request in at.Cleanupafter the assertions pass.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/worker/publish_test.go` around lines 702 - 742, Update TestMilestone2ExitGate to document that each run leaves an attempt-scoped branch and pull request in the scratch repository, requiring the operator to close the pull request and delete the branch; alternatively, register t.Cleanup after the assertions to perform that cleanup.internal/controlplane/github_poll.go (1)
395-406: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate handling differs between the issue path and the pull-request path.
ListIssuesrejects a repeated number withgh_conflicting_duplicateon line 364. Here a repeated number is skipped silently. A pull request has exactly one base branch, so two--basequeries cannot both return it. A duplicate therefore means oneghresponse contained the same pull request twice, which is the same contract violation the issue path refuses.Align the two paths so the same
ghanomaly produces the same diagnostic.♻️ Proposed alignment
for _, match := range values { if seen[match.Number] { - continue + return nil, checkFailure("gh_conflicting_duplicate", + "gh pr list returned pull request #%d twice", match.Number) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/controlplane/github_poll.go` around lines 395 - 406, Update the duplicate handling in the pull-request aggregation loop around the seen map so a repeated match.Number returns checkFailure("gh_conflicting_duplicate") instead of being silently skipped. Align its diagnostic and failure behavior with the existing ListIssues duplicate handling, while preserving normal unique-match collection and the MaxTriggerMatches limit.internal/controlplane/schedule.go (2)
416-441: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winBound the number of schedule targets.
validateTriggerConfigrejects an empty target list and duplicate repositories, but it does not cap the count. The only limit isMaxTriggerConfigBytes, which still admits several hundred repositories. Every firing then fans out one job per target, with no named diagnostic. The GitHub path hasMaxTriggerMatchesfor exactly this reason.Add an explicit cap so a mis-authored schedule fails at save time with an actionable code.
♻️ Proposed target-count bound
if len(config.Targets) == 0 { return zero, invalid("no_targets", "a schedule trigger requires at least one target repository") } + if len(config.Targets) > protocol.MaxTriggerMatches { + return zero, invalid("too_many_targets", fmt.Sprintf( + "a schedule trigger may fan out to at most %d repositories", protocol.MaxTriggerMatches)) + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/controlplane/schedule.go` around lines 416 - 441, Update validateTriggerConfig’s schedule-target validation to reject configurations whose targets exceed an explicit maximum, using a named diagnostic code and actionable message that includes the allowed limit. Apply the check before building the normalized targets list, while preserving the existing empty-list and duplicate-repository validation.
619-625: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueName the lateness threshold as a constant.
2 * time.Minutedecides whether a firing is reported as "admitted after downtime". Every other bound in this path comes frominternal/protocol/limits.go, includingTriggerTickInterval. A literal here can drift from the tick interval, and the diagnostic then appears for firings that were merely one tick late.Define the threshold beside the other trigger limits and derive it from
TriggerTickInterval.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/controlplane/schedule.go` around lines 619 - 625, Define a named lateness-threshold constant alongside the trigger limits in internal/protocol/limits.go, deriving it from TriggerTickInterval. Update the schedule logic around the due.Add comparison in the firing admission path to use this constant instead of the literal 2 * time.Minute.internal/controlplane/http.go (1)
174-181: 🚀 Performance & Scalability | 🔵 TrivialConsider a projection or pagination for the run list.
Runsselectssnapshotandparametersfor every run, and this route returns all of them in one response. The frozen snapshot is a full definition YAML, and the composed prompt can reach tens of kilobytes. The response therefore grows without bound as runs accumulate. Add a limit plus a summary projection for the list route, and keep the full snapshot on the detail route.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/controlplane/http.go` around lines 174 - 181, Update API.listRuns and its store query to return a bounded, paginated run list using a summary projection that excludes the full snapshot and parameters; preserve the complete snapshot and parameters on the run detail route.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/controlplane/cron.go`:
- Around line 142-143: Update parseCronField so wildcard is true only for a bare
"*" value, not stepped fields such as "*/2" or "*/3"; preserve wildcard behavior
for unstepped fields and ensure day-of-month/day-of-week matching uses the
existing OR rule for restricted stepped fields.
- Around line 224-234: Optimize the minute-by-minute search in schedule.Next by
skipping ahead when coarse fields cannot match: advance to the first minute of
the next local month when the candidate’s local month is not admitted, and
advance to the next local midnight when its local day matches neither day rule.
Preserve the existing candidate matching and DST behavior, while retaining the
cronSearchYears limit and no-match error.
In `@internal/controlplane/github_poll.go`:
- Around line 265-270: Update boundedBody to truncate by UTF-8 rune boundaries,
matching the rune-safe approach used by boundedDiagnostic, while preserving the
existing maximum byte limit and unchanged behavior for shorter bodies.
- Around line 107-118: Check rows.Err() after the rows.Next() loop and before
relying on ids, returning unavailable(err) when iteration failed. Keep the
existing rows.Scan handling and rows.Close error handling unchanged, and ensure
the id list is used only after confirming iteration completed without error.
- Around line 162-179: Re-read the trigger’s enabled state inside the
transaction before the occurrence loop, and only execute the inserts in the poll
flow when the trigger remains enabled. Preserve the existing trigger update
guard and ensure a trigger disabled after observe commits no new pending
occurrences.
In `@internal/controlplane/publish_ledger.go`:
- Line 262: Update AttemptPublishRecords and its final sort key to use an
explicit pipeline step rank rather than alphabetically sorting p.step. Ensure
the rank orders proof, pull_request, and push according to the documented
pipeline order, while preserving the existing timestamp and other sort keys.
In `@internal/controlplane/runs_test.go`:
- Around line 240-247: Remove the initial head return value assignment from
newFixtureRepository in the fixture setup loop, since it is immediately
overwritten by commitTo. Preserve the subsequent commitTo result assignment and
fixture construction using the updated head.
In `@internal/controlplane/runs.go`:
- Around line 127-151: Update ReadmitRunAtHead and the underlying
protocol.RunTarget persistence flow to retain each target’s Ref alongside
Repository and BaseSHA, then replay that Ref when constructing InvocationTarget
values so re-admission resolves the original branch rather than another branch’s
HEAD. Add a regression test covering a run admitted with a non-empty Ref and
verifying the same Ref is used during re-admission.
In `@internal/controlplane/schedule.go`:
- Around line 543-554: Check rows.Err() after each id-collecting loop in the
schedule admission and pending-occurrence dispatch paths, including
admitDueSchedules and dispatchPendingOccurrences. If iteration ended because of
a read error, return unavailable(err) before treating the collected IDs as
complete; retain the existing scan and close error handling.
- Around line 969-975: Update boundedDiagnostic to truncate value on a valid
UTF-8 rune boundary rather than slicing at an arbitrary byte offset; use
unicode/utf8 to ensure the returned diagnostic remains valid UTF-8 while
respecting protocol.MaxTriggerDiagnosticBytes.
- Around line 826-829: In the occurrenceWithTrigger error path within the
dispatch flow, call a best-effort releaseOccurrenceReservation helper with
occurrenceID before returning the read error. Add the helper beside
failOccurrence to update only dispatching rows back to pending, refresh
updated_at, and warn if the release update fails, allowing the next tick to
retry the occurrence.
In `@internal/runtime/claudecode/adapter.go`:
- Around line 552-563: Update claudeHandle.drainStream and consume so the drain
timeout is based on read progress rather than wall time since process exit: have
consume increment an atomic progress counter after each successful ReadLine, and
have drainStream monitor that counter during the grace window, closing
stdoutReader and stderrReader only when no progress occurs. Preserve waiting for
h.done and h.stderrDone after readers are closed, while allowing ongoing
backpressure-driven draining to continue.
- Around line 504-510: Synchronize the final process-group signal and anchor
reap around the Result() cleanup flow: ensure every stopEverything/stopGroup
call is excluded once h.anchor.Wait() begins, including repeated Result() calls
and concurrent Kill(). Replace any standalone groupReaped check with the same
lock or synchronization mechanism guarding the signal/reap transition, and add
regression coverage for repeated Result() plus concurrent Kill().
In `@internal/runtime/codex/adapter.go`:
- Around line 236-239: Update the error path after anchor.Start in the
surrounding adapter flow to explicitly discard the watchdog.Close error using
the existing _ = convention, matching the abandon closure below; preserve the
current returned start-process error.
In `@internal/worker/publish.go`:
- Around line 896-904: Update the branch-processing logic around
pushedBranchesForJob so a nil result, indicating no control-plane answer, skips
accusation for that job’s branches. Distinguish nil from an empty non-nil map:
continue treating explicitly absent branch records as stray, while avoiding
pushed[branch.Branch] lookup fallthrough when the result is nil.
- Around line 774-798: Derive a publishCtx using context.WithoutCancel(ctx)
before creating the heartbeat context, then derive heartbeatCtx from publishCtx
so lease.keepAlive remains active through the uncancellable critical section.
Pass the same publishCtx to w.publish instead of creating a separate
context.WithoutCancel(ctx) inline, preserving cancellation behavior after the
publish completes.
---
Nitpick comments:
In `@internal/controlplane/github_poll.go`:
- Around line 395-406: Update the duplicate handling in the pull-request
aggregation loop around the seen map so a repeated match.Number returns
checkFailure("gh_conflicting_duplicate") instead of being silently skipped.
Align its diagnostic and failure behavior with the existing ListIssues duplicate
handling, while preserving normal unique-match collection and the
MaxTriggerMatches limit.
In `@internal/controlplane/http.go`:
- Around line 174-181: Update API.listRuns and its store query to return a
bounded, paginated run list using a summary projection that excludes the full
snapshot and parameters; preserve the complete snapshot and parameters on the
run detail route.
In `@internal/controlplane/schedule.go`:
- Around line 416-441: Update validateTriggerConfig’s schedule-target validation
to reject configurations whose targets exceed an explicit maximum, using a named
diagnostic code and actionable message that includes the allowed limit. Apply
the check before building the normalized targets list, while preserving the
existing empty-list and duplicate-repository validation.
- Around line 619-625: Define a named lateness-threshold constant alongside the
trigger limits in internal/protocol/limits.go, deriving it from
TriggerTickInterval. Update the schedule logic around the due.Add comparison in
the firing admission path to use this constant instead of the literal 2 *
time.Minute.
In `@internal/controlplane/store_test.go`:
- Line 308: Remove the unused branch variable declaration near the claim setup
and delete the corresponding branch == "" placeholder assertion, leaving the
surrounding test behavior unchanged.
In `@internal/worker/publish_test.go`:
- Around line 702-742: Update TestMilestone2ExitGate to document that each run
leaves an attempt-scoped branch and pull request in the scratch repository,
requiring the operator to close the pull request and delete the branch;
alternatively, register t.Cleanup after the assertions to perform that cleanup.
In `@internal/worker/publish.go`:
- Around line 1074-1079: Update FindOrCreatePullRequest’s create path to
populate PullRequest.Number by parsing the pull request number from the URL
returned by firstPullRequestURL, matching the value provided by findPullRequest;
handle an unparseable URL through the existing publish failure path rather than
returning Number as zero.
- Around line 791-794: Update RetryPublish to load the attempt manifest through
w.manifests.load(retry.Attempt.ID) and use its WorktreePath for
target.worktreePath, removing the reconstructed filepath.Join(w.worktreeRoot(),
retry.Attempt.ID) path and related stat check.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 568ef957-6d7c-434d-944c-d9bfa33f5cbc
📒 Files selected for processing (37)
internal/controlplane/admission_test.gointernal/controlplane/claim_test.gointernal/controlplane/cron.gointernal/controlplane/cron_test.gointernal/controlplane/definitions.gointernal/controlplane/definitions_test.gointernal/controlplane/embedded.gointernal/controlplane/embedded_test.gointernal/controlplane/github_poll.gointernal/controlplane/http.gointernal/controlplane/publish_ledger.gointernal/controlplane/publish_ledger_test.gointernal/controlplane/runs.gointernal/controlplane/runs_test.gointernal/controlplane/schedule.gointernal/controlplane/store.gointernal/controlplane/store_test.gointernal/protocol/limits.gointernal/protocol/prompt.gointernal/protocol/prompt_test.gointernal/protocol/publish.gointernal/protocol/repository.gointernal/protocol/repository_test.gointernal/runtime/claudecode/adapter.gointernal/runtime/claudecode/adapter_test.gointernal/runtime/codex/adapter.gointernal/runtime/codex/adapter_test.gointernal/runtime/codex/live_test.gointernal/worker/claiming_test.gointernal/worker/publish.gointernal/worker/publish_test.gointernal/worker/reconcile.gointernal/worker/reconcile_test.gointernal/worker/repocache.gointernal/worker/worktree.gointernal/worker/worktree_test.gomigrations/003_admission_triggers.sql
| func parseCronField(value string, minimum, maximum int, names map[string]int, sundaySeven bool) (cronField, error) { | ||
| field := cronField{allowed: make([]bool, maximum+1), wildcard: strings.HasPrefix(value, "*")} |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
A stepped day-of-week field is treated as a wildcard.
parseCronField sets wildcard from strings.HasPrefix(value, "*"), so */2 and */3 are marked as wildcards even though they restrict the field. matches then applies the wrong branch of cron's day rule.
Example: 0 0 1 * */2. The day-of-week field admits only even weekdays, but wildcard is true, so matches takes case schedule.dayOfWeek.wildcard: return dayOfMonth. The schedule fires on the 1st only. Vixie cron fires on the 1st OR on every even weekday. The same inversion applies when day-of-month carries a step, for example */2 * */2 * MON.
Set the flag only for a bare * field with no step.
🐛 Proposed fix
func parseCronField(value string, minimum, maximum int, names map[string]int, sundaySeven bool) (cronField, error) {
- field := cronField{allowed: make([]bool, maximum+1), wildcard: strings.HasPrefix(value, "*")}
+ // Only a bare `*` is unrestricted: `*/2` selects a subset, and cron's
+ // day-of-month/day-of-week OR rule must see it as restricted.
+ field := cronField{allowed: make([]bool, maximum+1), wildcard: value == "*"}
if value == "" {
return field, fmt.Errorf("field is empty")
}Add the case to cron_test.go:
// A stepped day-of-week field is restricted, so cron's OR rule applies.
{"0 0 1 * */2",
time.Date(2026, 8, 6, 0, 0, 0, 0, time.UTC), // a Thursday
time.Date(2026, 8, 6, 0, 0, 0, 0, time.UTC).AddDate(0, 0, 2)}, // the next even weekdayAlso applies to: 245-256
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/controlplane/cron.go` around lines 142 - 143, Update parseCronField
so wildcard is true only for a bare "*" value, not stepped fields such as "*/2"
or "*/3"; preserve wildcard behavior for unstepped fields and ensure
day-of-month/day-of-week matching uses the existing OR rule for restricted
stepped fields.
| func (schedule cronSchedule) Next(after time.Time) (time.Time, error) { | ||
| candidate := after.UTC().Truncate(time.Minute).Add(time.Minute) | ||
| limit := candidate.AddDate(cronSearchYears, 0, 0) | ||
| for !candidate.After(limit) { | ||
| if schedule.matches(candidate) { | ||
| return candidate, nil | ||
| } | ||
| candidate = candidate.Add(time.Minute) | ||
| } | ||
| return time.Time{}, fmt.Errorf("cron has no matching instant within %d years", cronSearchYears) | ||
| } |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟠 Major | 🏗️ Heavy lift
Bound the cost of the minute-by-minute walk.
Next advances one minute at a time for up to cronSearchYears. A never-matching expression therefore performs about 5.3 million iterations, each with a timezone conversion, before it returns the error. The comment on line 39 states that such an expression is "a save-time error, not a scheduler that spins", but validateTriggerConfig reaches that error through this same walk, on the request thread that serves POST /api/triggers. TestAnExpressionThatCanNeverMatchFailsRatherThanSpinning exercises exactly that worst case.
Skip ahead in larger units when the coarse fields cannot match. If the local month is not admitted, advance to the first minute of the next month. If the local day matches neither day rule, advance to the next local midnight. Both reduce the worst case from millions of iterations to thousands, and neither changes the DST semantics, because the walk still tests every candidate minute it visits.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/controlplane/cron.go` around lines 224 - 234, Optimize the
minute-by-minute search in schedule.Next by skipping ahead when coarse fields
cannot match: advance to the first minute of the next local month when the
candidate’s local month is not admitted, and advance to the next local midnight
when its local day matches neither day rule. Preserve the existing candidate
matching and DST behavior, while retaining the cronSearchYears limit and
no-match error.
| var ids []string | ||
| for rows.Next() { | ||
| var id string | ||
| if err := rows.Scan(&id); err != nil { | ||
| rows.Close() | ||
| return unavailable(err) | ||
| } | ||
| ids = append(ids, id) | ||
| } | ||
| if err := rows.Close(); err != nil { | ||
| return unavailable(err) | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Check rows.Err() before using the id list.
rows.Next() returns false both at the end of the result set and on a read error, and rows.Close() does not report the iteration error. A truncated list then looks complete, so due GitHub triggers are silently skipped for that tick. golangci-lint flags this line.
🐛 Proposed fix
ids = append(ids, id)
}
+ if err := rows.Err(); err != nil {
+ rows.Close()
+ return unavailable(err)
+ }
if err := rows.Close(); err != nil {
return unavailable(err)
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| var ids []string | |
| for rows.Next() { | |
| var id string | |
| if err := rows.Scan(&id); err != nil { | |
| rows.Close() | |
| return unavailable(err) | |
| } | |
| ids = append(ids, id) | |
| } | |
| if err := rows.Close(); err != nil { | |
| return unavailable(err) | |
| } | |
| var ids []string | |
| for rows.Next() { | |
| var id string | |
| if err := rows.Scan(&id); err != nil { | |
| rows.Close() | |
| return unavailable(err) | |
| } | |
| ids = append(ids, id) | |
| } | |
| if err := rows.Err(); err != nil { | |
| rows.Close() | |
| return unavailable(err) | |
| } | |
| if err := rows.Close(); err != nil { | |
| return unavailable(err) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/controlplane/github_poll.go` around lines 107 - 118, Check
rows.Err() after the rows.Next() loop and before relying on ids, returning
unavailable(err) when iteration failed. Keep the existing rows.Scan handling and
rows.Close error handling unchanged, and ensure the id list is used only after
confirming iteration completed without error.
Source: Linters/SAST tools
| for _, source := range sources { | ||
| // A key collision means an earlier poll already committed this exact | ||
| // event. That is the dedup working, not an anomaly, so it is silent — | ||
| // counting it would turn "this issue is still open" into a rising | ||
| // skip counter. | ||
| if _, err := insertOccurrenceTx(ctx, tx, | ||
| trigger.ID, githubRequestKey(trigger, source), OccurrencePending, | ||
| source, nil, "", now); err != nil { | ||
| return err | ||
| } | ||
| } | ||
| if _, err := tx.ExecContext(ctx, ` | ||
| UPDATE triggers SET last_checked_at = ?, next_poll_at = ?, | ||
| diagnostic_code = '', diagnostic = '', updated_at = ? | ||
| WHERE id = ? AND enabled = 1 | ||
| `, now.UnixMilli(), s.nextPollAt(trigger, now), now.UnixMilli(), trigger.ID); err != nil { | ||
| return unavailable(err) | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
A poll in flight can still admit work for a trigger the operator just disabled.
The occurrence inserts on lines 167-172 carry no enabled guard, but the trigger update on line 176 does. If the operator disables the trigger after observe returns and before this transaction commits, the occurrences are committed anyway. SetTriggerEnabled in internal/controlplane/schedule.go clears the cursors only; it does not cancel pending occurrences. The next dispatchPendingOccurrences therefore turns them into runs for a disabled trigger.
Re-read enabled inside the transaction and skip the inserts when it is 0.
🐛 Proposed fix
defer tx.Rollback()
+ // Disabling must stop admission, including a poll that was already in
+ // flight when the operator turned the trigger off.
+ var stillEnabled int
+ if err := tx.QueryRowContext(ctx,
+ `SELECT enabled FROM triggers WHERE id = ?`, trigger.ID).Scan(&stillEnabled); err != nil {
+ return unavailable(err)
+ }
+ if stillEnabled == 0 {
+ return nil
+ }
for _, source := range sources {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/controlplane/github_poll.go` around lines 162 - 179, Re-read the
trigger’s enabled state inside the transaction before the occurrence loop, and
only execute the inserts in the poll flow when the trigger remains enabled.
Preserve the existing trigger update guard and ensure a trigger disabled after
observe commits no new pending occurrences.
| func boundedBody(body string) string { | ||
| if len(body) <= protocol.MaxUntrustedSectionBytes { | ||
| return body | ||
| } | ||
| return body[:protocol.MaxUntrustedSectionBytes] | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Truncate the observed body on a rune boundary.
body[:protocol.MaxUntrustedSectionBytes] can split a multi-byte UTF-8 rune. Issue and pull-request bodies are arbitrary user text, so non-ASCII content is common. The invalid tail is then frozen into the occurrence source and into the composed prompt. Apply the same rune-safe truncation suggested for boundedDiagnostic in internal/controlplane/schedule.go.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/controlplane/github_poll.go` around lines 265 - 270, Update
boundedBody to truncate by UTF-8 rune boundaries, matching the rune-safe
approach used by boundedDiagnostic, while preserving the existing maximum byte
limit and unchanged behavior for shorter bodies.
| waitErr := h.command.Wait() | ||
| // The CLI is gone; release the group. The anchor ignores TERM by design, | ||
| // so this is the one place it dies. | ||
| // so this is the one place it dies. This is also what frees the inherited | ||
| // descriptor: any descendant still holding stdout is in this group. | ||
| h.stopEverything(0) | ||
| h.drainStream() | ||
| _ = h.anchor.Wait() |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: find Result/Kill call sites on runtime handles.
rg -nP --type=go -C4 '\.Result\s*\(\s*\)|\.Kill\s*\(\s*\)' -g '!internal/runtime/**'
rg -nP --type=go -C6 'handle\.Result|handle\.Kill' internal/runtimeRepository: StructuPath/jig
Length of output: 153
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- target symbols and surrounding implementation ---'
rg -n -C8 'type claudeHandle|func \(h \*claudeHandle\) (Result|Kill|stopEverything)|groupReaped|reapOnce|stopOnce|anchor|stopGroup' internal/runtime/claudecode/adapter.go
printf '%s\n' '--- all Result and Kill call sites ---'
rg -n -P --type=go '\.(Result|Kill)\s*\(\s*\)' .
printf '%s\n' '--- interface and handle construction references ---'
rg -n -C5 --type=go 'claudeHandle|Result\(\)|Kill\(\)|New.*Claude|Claude' internal/runtimeRepository: StructuPath/jig
Length of output: 50371
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- claude handle state and teardown ---'
sed -n '297,355p' internal/runtime/claudecode/adapter.go
sed -n '487,530p' internal/runtime/claudecode/adapter.go
sed -n '568,595p' internal/runtime/claudecode/adapter.go
printf '%s\n' '--- engine lifecycle around Result and Kill ---'
sed -n '1165,1270p' internal/engine/phase.go
printf '%s\n' '--- Claude-specific lifecycle references ---'
rg -n -C6 --type=go 'claude|handle\.Result|handle\.Kill|context\.Canceled|ErrKilled' internal/engine internal/runtime/claudecodeRepository: StructuPath/jig
Length of output: 50371
🏁 Script executed:
#!/bin/bash
set -eu
python3 - <<'PY'
# Read-only model of the proposed reap flag.
# It checks whether a signal can pass the flag check before the reaper marks
# the anchor as reaped, then execute after the anchor is reaped.
states = []
def explore(trace, reaped, signal_checked):
if len(trace) >= 4:
if signal_checked and reaped:
states.append(trace)
return
# Result reaps the anchor, then sets groupReaped.
if not reaped:
explore(trace + ["Result: anchor.Wait()"], True, signal_checked)
explore(trace + ["Kill: read groupReaped=false"], reaped, True)
else:
explore(trace + ["Result: set groupReaped=true"], True, signal_checked)
if signal_checked:
explore(trace + ["Kill: stopGroup(groupID)"], True, signal_checked)
explore([], False, False)
for trace in states:
if "Kill: stopGroup(groupID)" in trace:
print("unsafe interleaving:")
print(" -> ".join(trace))
break
else:
print("no unsafe interleaving found")
PY
printf '%s\n' '--- current stopEverything idempotence check ---'
python3 - <<'PY'
from pathlib import Path
s = Path("internal/runtime/claudecode/adapter.go").read_text()
start = s.index("func (h *claudeHandle) stopEverything")
end = s.index("\n}", start) + 2
body = s[start:end]
print(body)
print("stopGroup calls:", body.count("stopGroup("))
print("guarded by stopOnce:", "stopOnce.Do" in body and "stopGroup(" in body)
PYRepository: StructuPath/jig
Length of output: 570
Serialize the final anchor reap with all group signals.
stopOnce only closes h.stopped; every stopEverything call still invokes stopGroup. A repeated Result() call or a context-cancellation Kill() can therefore signal -h.groupID after h.anchor.Wait() reaps the anchor and its PID is reused. A standalone groupReaped check is also racy. Serialize the signal/reap transition so no stopGroup call can occur after the reap, and add a regression test for repeated Result() and concurrent Kill().
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/runtime/claudecode/adapter.go` around lines 504 - 510, Synchronize
the final process-group signal and anchor reap around the Result() cleanup flow:
ensure every stopEverything/stopGroup call is excluded once h.anchor.Wait()
begins, including repeated Result() calls and concurrent Kill(). Replace any
standalone groupReaped check with the same lock or synchronization mechanism
guarding the signal/reap transition, and add regression coverage for repeated
Result() plus concurrent Kill().
| func (h *claudeHandle) drainStream() { | ||
| timer := time.NewTimer(streamDrainGrace) | ||
| defer timer.Stop() | ||
| select { | ||
| case <-h.done: | ||
| case <-timer.C: | ||
| } | ||
| _ = h.stdoutReader.Close() | ||
| _ = h.stderrReader.Close() | ||
| <-h.done | ||
| <-h.stderrDone | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
The drain grace also fires on normal backpressure and then discards stream data.
h.done closes only when consume returns. consume blocks in emit whenever the event channel (depth 256) is full, which is the documented backpressure path, not a stuck descendant. If the engine needs more than streamDrainGrace to drain the queued events, Line 559 closes stdoutReader under the reader. consume then returns on ErrClosed, and every byte still in the pipe is lost — including a terminal result line, which surfaces as "claude returned no terminal result event".
Bound the wait on absence of read progress, not on wall time since exit. For example, record a progress counter in consume and only close the readers if the counter did not advance during the window.
🐛 Sketch: only close when the capture made no progress
func (h *claudeHandle) drainStream() {
- timer := time.NewTimer(streamDrainGrace)
- defer timer.Stop()
- select {
- case <-h.done:
- case <-timer.C:
- }
+ for {
+ before := h.progress.Load()
+ timer := time.NewTimer(streamDrainGrace)
+ select {
+ case <-h.done:
+ timer.Stop()
+ return
+ case <-timer.C:
+ }
+ // Progress means the stream is alive and the consumer is slow, not
+ // that a descendant is holding the write end.
+ if h.progress.Load() == before {
+ break
+ }
+ }
_ = h.stdoutReader.Close()
_ = h.stderrReader.Close()
<-h.done
<-h.stderrDone
}consume increments h.progress (an atomic.Uint64) after each successful ReadLine.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/runtime/claudecode/adapter.go` around lines 552 - 563, Update
claudeHandle.drainStream and consume so the drain timeout is based on read
progress rather than wall time since process exit: have consume increment an
atomic progress counter after each successful ReadLine, and have drainStream
monitor that counter during the grace window, closing stdoutReader and
stderrReader only when no progress occurs. Preserve waiting for h.done and
h.stderrDone after readers are closed, while allowing ongoing
backpressure-driven draining to continue.
| if err := anchor.Start(); err != nil { | ||
| watchdog.Close() | ||
| return nil, fmt.Errorf("start process-group anchor: %w", err) | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Assign the ignored Close error to satisfy errcheck.
The lint gate reports Line 237. The abandon closure below already uses the _ = form.
🔧 Proposed fix
if err := anchor.Start(); err != nil {
- watchdog.Close()
+ _ = watchdog.Close()
return nil, fmt.Errorf("start process-group anchor: %w", err)
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if err := anchor.Start(); err != nil { | |
| watchdog.Close() | |
| return nil, fmt.Errorf("start process-group anchor: %w", err) | |
| } | |
| if err := anchor.Start(); err != nil { | |
| _ = watchdog.Close() | |
| return nil, fmt.Errorf("start process-group anchor: %w", err) | |
| } |
🧰 Tools
🪛 golangci-lint (2.12.2)
[error] 237-237: Error return value of watchdog.Close is not checked
(errcheck)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/runtime/codex/adapter.go` around lines 236 - 239, Update the error
path after anchor.Start in the surrounding adapter flow to explicitly discard
the watchdog.Close error using the existing _ = convention, matching the abandon
closure below; preserve the current returned start-process error.
Source: Linters/SAST tools
| lease := newAttemptLease(w.client, retry.Attempt.ID, token) | ||
| heartbeatCtx, stopHeartbeat := context.WithCancel(ctx) | ||
| defer stopHeartbeat() | ||
| go lease.keepAlive(heartbeatCtx, w.logger) | ||
|
|
||
| w.trackActive(retry.Attempt.ID) | ||
| defer w.untrackActive(retry.Attempt.ID) | ||
|
|
||
| target := publishTarget{ | ||
| attemptID: retry.Attempt.ID, | ||
| jobID: retry.Job.ID, | ||
| attemptNumber: retry.Attempt.AttemptNumber, | ||
| repository: retry.Job.Repository, | ||
| branch: protocol.PublishBranch(retry.Job.ID, retry.Attempt.AttemptNumber), | ||
| baseSHA: retry.Job.BaseSHA, | ||
| lease: lease, | ||
| } | ||
| worktreePath := filepath.Join(w.worktreeRoot(), retry.Attempt.ID) | ||
| if _, err := os.Stat(worktreePath); err == nil { | ||
| target.worktreePath = worktreePath | ||
| } | ||
| // The critical section again: a retry that has just pushed must not be | ||
| // abandoned before its pull request exists. | ||
| summary := w.publish(context.WithoutCancel(ctx), gateway, options, target, | ||
| changedPathsFromResult(retry.Attempt.Result)) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Derive the heartbeat context from the uncancellable context.
publish runs on context.WithoutCancel(ctx) at Line 797, but heartbeatCtx at Line 775 derives from ctx. If ctx is cancelled during a retry — worker shutdown, for example — lease.keepAlive stops while the publish pipeline keeps running. The lease then expires mid-pipeline and the next AuthorizePublishStep or RecordPublishStep fails with lease_not_owner. A push that already reached the remote cannot be recorded, which produces exactly the stray branch this design tries to avoid.
Keep the lease alive for as long as the critical section runs.
🔒 Proposed fix
lease := newAttemptLease(w.client, retry.Attempt.ID, token)
- heartbeatCtx, stopHeartbeat := context.WithCancel(ctx)
+ // Publish is uncancellable, so the lease that fences it must be too:
+ // a heartbeat that stops mid-pipeline fences the publisher out of its
+ // own push.
+ publishCtx := context.WithoutCancel(ctx)
+ heartbeatCtx, stopHeartbeat := context.WithCancel(publishCtx)
defer stopHeartbeat()
go lease.keepAlive(heartbeatCtx, w.logger)Then pass publishCtx at Line 797 in place of context.WithoutCancel(ctx).
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| lease := newAttemptLease(w.client, retry.Attempt.ID, token) | |
| heartbeatCtx, stopHeartbeat := context.WithCancel(ctx) | |
| defer stopHeartbeat() | |
| go lease.keepAlive(heartbeatCtx, w.logger) | |
| w.trackActive(retry.Attempt.ID) | |
| defer w.untrackActive(retry.Attempt.ID) | |
| target := publishTarget{ | |
| attemptID: retry.Attempt.ID, | |
| jobID: retry.Job.ID, | |
| attemptNumber: retry.Attempt.AttemptNumber, | |
| repository: retry.Job.Repository, | |
| branch: protocol.PublishBranch(retry.Job.ID, retry.Attempt.AttemptNumber), | |
| baseSHA: retry.Job.BaseSHA, | |
| lease: lease, | |
| } | |
| worktreePath := filepath.Join(w.worktreeRoot(), retry.Attempt.ID) | |
| if _, err := os.Stat(worktreePath); err == nil { | |
| target.worktreePath = worktreePath | |
| } | |
| // The critical section again: a retry that has just pushed must not be | |
| // abandoned before its pull request exists. | |
| summary := w.publish(context.WithoutCancel(ctx), gateway, options, target, | |
| changedPathsFromResult(retry.Attempt.Result)) | |
| lease := newAttemptLease(w.client, retry.Attempt.ID, token) | |
| // Publish is uncancellable, so the lease that fences it must be too: | |
| // a heartbeat that stops mid-pipeline fences the publisher out of its | |
| // own push. | |
| publishCtx := context.WithoutCancel(ctx) | |
| heartbeatCtx, stopHeartbeat := context.WithCancel(publishCtx) | |
| defer stopHeartbeat() | |
| go lease.keepAlive(heartbeatCtx, w.logger) | |
| w.trackActive(retry.Attempt.ID) | |
| defer w.untrackActive(retry.Attempt.ID) | |
| target := publishTarget{ | |
| attemptID: retry.Attempt.ID, | |
| jobID: retry.Job.ID, | |
| attemptNumber: retry.Attempt.AttemptNumber, | |
| repository: retry.Job.Repository, | |
| branch: protocol.PublishBranch(retry.Job.ID, retry.Attempt.AttemptNumber), | |
| baseSHA: retry.Job.BaseSHA, | |
| lease: lease, | |
| } | |
| worktreePath := filepath.Join(w.worktreeRoot(), retry.Attempt.ID) | |
| if _, err := os.Stat(worktreePath); err == nil { | |
| target.worktreePath = worktreePath | |
| } | |
| // The critical section again: a retry that has just pushed must not be | |
| // abandoned before its pull request exists. | |
| summary := w.publish(publishCtx, gateway, options, target, | |
| changedPathsFromResult(retry.Attempt.Result)) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/worker/publish.go` around lines 774 - 798, Derive a publishCtx using
context.WithoutCancel(ctx) before creating the heartbeat context, then derive
heartbeatCtx from publishCtx so lease.keepAlive remains active through the
uncancellable critical section. Pass the same publishCtx to w.publish instead of
creating a separate context.WithoutCancel(ctx) inline, preserving cancellation
behavior after the publish completes.
| for _, branch := range branches { | ||
| pushed, exists := pushedByJob[branch.JobID] | ||
| if !exists { | ||
| pushed = w.pushedBranchesForJob(ctx, branch.JobID, &scanErrors) | ||
| pushedByJob[branch.JobID] = pushed | ||
| } | ||
| if pushed[branch.Branch] { | ||
| continue | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Fix the "cannot tell" case: a nil map makes every branch of that job stray.
pushedBranchesForJob returns nil when the control plane could not answer, and the comment states that nothing is accused in that case. The caller does not honor that. pushed[branch.Branch] on a nil map returns false, so the branch falls through and is appended to stray. A transient control-plane failure therefore reports legitimate branches as stray, which is the opposite of the documented behavior.
Distinguish "no records" from "no answer" and skip the branch when the answer is missing.
🐛 Proposed fix
for _, branch := range branches {
pushed, exists := pushedByJob[branch.JobID]
if !exists {
pushed = w.pushedBranchesForJob(ctx, branch.JobID, &scanErrors)
pushedByJob[branch.JobID] = pushed
}
+ if pushed == nil {
+ // The control plane could not answer for this job. Without an
+ // answer, nothing is accused.
+ continue
+ }
if pushed[branch.Branch] {
continue
}Also applies to: 931-948
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/worker/publish.go` around lines 896 - 904, Update the
branch-processing logic around pushedBranchesForJob so a nil result, indicating
no control-plane answer, skips accusation for that job’s branches. Distinguish
nil from an empty non-nil map: continue treating explicitly absent branch
records as stray, while avoiding pushed[branch.Branch] lookup fallthrough when
the result is nil.
… (U8, U9) U8: the worker streams bounded seq-ordered event batches to the control plane and dual-writes attempt-local JSONL, one batch in flight so a UI cursor can never advance past an undelivered seq. Ingestion is insert-or-ignore on (attempt_id, seq) and projects gate and envelope evidence in the same transaction, so replay adds no second copy. Buffer overflow emits a declared trace_gap range rather than losing events silently. R15 redaction replaces sensitive env values before the event leaves the worker, in both the stream and the raw record. React UI with per-attempt swim lanes, envelope and gate inspection, queue depth, and confirm-gated worktree release; no metrics dashboard. U9: jig serve recovers interrupted occurrences before it serves, jig worker wraps the engine in the publishing runner and reconciles including stray publish branches, jig def and jig trigger give the API a CLI, and both runtimes get their auth seeded into the ephemeral HOME. Stock definitions cover scout, plan-build-test, and simple-sdlc including its conditional retest. Also: direct runs now project the same gate and envelope evidence the worker path does, and the scripted runtime can emit tool-call events and error verdicts rather than dropping them.
There was a problem hiding this comment.
Actionable comments posted: 7
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
🟡 Minor comments (20)
internal/worker/events_test.go-239-242 (1)
239-242: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winThe exact
pendingassertion races with the background sender.
sendOnceremoves up toprotocol.MaxEventsPerBatchevents frompendingbefore it calls the offline ingester, and it restores them only after the call fails.Undeliveredtherefore can report fewer than 11 during that window, so this assertion is flaky. Assert the invariant that matters: nothing beyond seq 1 was delivered, and no event was dropped.🧪 Proposed fix
pending, dropped := stream.Undelivered() - if pending != 11 || dropped != 0 { - t.Fatalf("expected 11 buffered and 0 dropped during the outage, got %d/%d", pending, dropped) + if pending == 0 || pending > 11 || dropped != 0 { + t.Fatalf("expected up to 11 buffered and 0 dropped during the outage, got %d/%d", pending, dropped) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/worker/events_test.go` around lines 239 - 242, Replace the exact pending-count assertion in the outage test with assertions that no event beyond sequence 1 was delivered and dropped remains zero. Avoid relying on the transient value returned by stream.Undelivered while sendOnce is processing a failed batch.internal/worker/events_test.go-76-78 (1)
76-78: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
os.NewFile(0, os.DevNull)wraps standard input, not/dev/null.The first argument is a file descriptor.
0is standard input;os.DevNullis only the name label. Log writes therefore target fd 0, andos.NewFileattaches a finalizer that can close it. Write toio.Discardinstead.🧪 Proposed fix
func discardLogger() *slog.Logger { - return slog.New(slog.NewTextHandler(os.NewFile(0, os.DevNull), nil)) + return slog.New(slog.NewTextHandler(io.Discard, nil)) }Add the import:
"errors" + "io" "log/slog"🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/worker/events_test.go` around lines 76 - 78, Update discardLogger to use an io.Discard-backed slog handler instead of os.NewFile(0, os.DevNull), and add the required io import while removing any now-unused os import.internal/worker/events.go-569-573 (1)
569-573: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Undeliveredundercounts events while a batch is in flight.
sendOnceremoves the batch frompendingbefore the call and restores it only on failure. Between those pointsUndeliveredreports fewer events than are actually undelivered, so a caller that checks completeness mid-attempt can read a low count.Closeis unaffected because it waits for the sender to stop. Count the in-flight batch.🔢 Proposed fix to count the in-flight batch
sending bool + inFlight int dropped int64batch := append([]protocol.Event(nil), s.pending[:count]...) s.pending = append([]protocol.Event(nil), s.pending[count:]...) s.sending = true + s.inFlight = count s.mutex.Unlock()s.mutex.Lock() defer s.mutex.Unlock() s.sending = false + s.inFlight = 0func (s *TraceStream) Undelivered() (pending int, dropped int64) { s.mutex.Lock() defer s.mutex.Unlock() - return len(s.pending), s.dropped + return len(s.pending) + s.inFlight, s.dropped }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/worker/events.go` around lines 569 - 573, Update TraceStream.Undelivered to include the batch currently being processed by sendOnce in its pending count, using the existing in-flight batch state and protecting the read with the mutex. Preserve the current pending and dropped counts when no batch is in flight.cmd/jig/serve_test.go-90-90 (1)
90-90: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winResolve the
noctxlint findings.golangci-lint reports errors for
http.Geton lines 90, 102, and 181, and fordb.Execon line 219. If the linter covers test files in CI, this file failsjust check. Usehttp.NewRequestWithContextwithhttp.DefaultClient.Do, anddb.ExecContext.🐛 Example fix for one call site
- health, err := http.Get(base + "/api/health") + request, err := http.NewRequestWithContext( + context.Background(), http.MethodGet, base+"/api/health", nil) + if err != nil { + t.Fatal(err) + } + health, err := http.DefaultClient.Do(request)Also applies to: 102-102, 181-181, 219-219
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cmd/jig/serve_test.go` at line 90, Resolve the noctx lint findings in the test calls: replace each http.Get invocation with an http.NewRequestWithContext request executed via http.DefaultClient.Do, and replace db.Exec with db.ExecContext. Update the affected call sites around the existing health checks and database operation, supplying the appropriate test context while preserving current request and query behavior.Source: Linters/SAST tools
internal/controlplane/ingest.go-270-275 (1)
270-275: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winInconsistent not-found behavior on the read surface.
AttemptEventPagecallss.Attemptfirst, so an unknown attempt returns 404.AttemptGateEvidenceandAttemptEnvelopesdo not, so an unknown attempt returns200with[]. A scripted caller cannot tell "this attempt has no gate evidence" from "this attempt does not exist". Add the same existence check to both.🐛 Add the existence check
func (s *Store) AttemptGateEvidence(ctx context.Context, attemptID string) ([]GateEvidence, error) { + if _, err := s.Attempt(ctx, attemptID); err != nil { + return nil, err + } rows, err := s.db.QueryContext(ctx, `Apply the same guard in
AttemptEnvelopes.Also applies to: 304-308
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/controlplane/ingest.go` around lines 270 - 275, Update AttemptGateEvidence and AttemptEnvelopes to match AttemptEventPage by calling s.Attempt(ctx, attemptID) before querying their tables, so an unknown attempt returns the same not-found error instead of an empty 200 response. Keep the existing query and result handling unchanged for valid attempts, and apply the same existence guard in both methods using the visible Attempt, AttemptGateEvidence, and AttemptEnvelopes symbols.cmd/jig/main.go-4-7 (1)
4-7: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winThe comment count no longer matches the usage text.
The comment says "Five subcommands" and names
run,serve,worker,def, andtrigger. The usage text on lines 27-32 lists six commands, because the same change addedversion. Update the comment so the two agree.🐛 Name the version command
-// Five subcommands, one binary, no Node (R18): run is the serverless direct +// Six subcommands, one binary, no Node (R18): run is the serverless direct // harness (U11), serve is the control plane with its embedded UI (U2/U6/U8), // worker is the execution host (U3/U4/U7), and def and trigger are the -// operator's surface over definitions and admission (U5/U6). +// operator's surface over definitions and admission (U5/U6). version prints +// the build-stamped release identity.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cmd/jig/main.go` around lines 4 - 7, Update the introductory command-count comment in main to include the version subcommand and accurately state six subcommands, naming version alongside run, serve, worker, def, and trigger so it matches the usage text.cmd/jig/runtime.go-117-127 (1)
117-127: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winBound the keychain lookup and gate it on darwin.
Two problems in this block:
- Line 119 uses
exec.Command, so thesecuritylookup has no deadline. golangci-lint reports it as anoctxerror. The seeder runs on the attempt path, so a stalled keychain prompt stalls the attempt.- The comment calls the seeder "darwin-aware", but there is no
runtime.GOOScheck. On Linux with no credentials file, the operator sees an error that names a macOS keychain item that cannot exist on that platform.
HomeSeederdoes not appear to receive a context, so useexec.CommandContextwith a locally bounded context.🐛 Bound and gate the lookup
credentials, readErr := os.ReadFile(filepath.Join(real, ".claude", ".credentials.json")) if readErr != nil { - extracted, keychainErr := exec.Command("security", - "find-generic-password", "-s", "Claude Code-credentials", "-w").Output() + if goruntime.GOOS != "darwin" { + return fmt.Errorf( + "seed claude auth: %s is unreadable (%v); run `claude login` first, "+ + "or use --no-seed-auth if the CLI authenticates through its environment", + filepath.Join(real, ".claude", ".credentials.json"), readErr) + } + lookupCtx, cancel := context.WithTimeout(context.Background(), 10*time.Second) + defer cancel() + extracted, keychainErr := exec.CommandContext(lookupCtx, "security", + "find-generic-password", "-s", "Claude Code-credentials", "-w").Output() if keychainErr != nil {Import
"time"andgoruntime "runtime". The alias avoids a collision with the existingruntimeimport.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cmd/jig/runtime.go` around lines 117 - 127, Update the credential fallback in HomeSeeder to attempt the `security` keychain lookup only when `goruntime.GOOS` is `darwin`; otherwise return the existing missing-credentials error without invoking macOS tooling. Create a locally bounded context using a short timeout from `time` and pass it to `exec.CommandContext` for the keychain command, preserving the existing extracted-credentials and error handling behavior.Source: Linters/SAST tools
cmd/jig/def.go-418-434 (1)
418-434: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winAn unknown
--kindproduces a misleading error.The
elsebranch treats every value that is notTriggerScheduleas a GitHub kind. A typo such as--kind shedulewith two--repoflags reports "a shedule trigger takes exactly one --repo" instead of naming the unknown kind. Validate*kindagainst the accepted vocabulary first.🐛 Validate the kind
+ switch *kind { + case controlplane.TriggerSchedule, + controlplane.TriggerGitHubIssue, + controlplane.TriggerGitHubPullRequest: + default: + fmt.Fprintf(stderr, + "jig trigger create: unknown --kind %q — expected schedule, github_issue, or github_pull_request\n", + *kind) + return exitUsage + } if *kind == controlplane.TriggerSchedule {Confirm the exported constant name for the pull-request kind.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cmd/jig/def.go` around lines 418 - 434, Validate *kind against the accepted trigger vocabulary before entering the schedule/GitHub repository handling in the trigger creation flow. Include the exported constant for the pull-request trigger alongside controlplane.TriggerSchedule and the valid GitHub kind, and return the existing usage error naming the unknown kind before repository-count validation; preserve the current schedule and single-repository behavior for valid kinds.cmd/jig/serve.go-145-153 (1)
145-153: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winThe background loops are not awaited before the store closes.
stopBackgroundcancels the sweeper and the admission runner, but nothing waits for them to return. The deferredstore.Closeon line 100 then runs immediately after cancellation, so a sweep or dispatch still in flight can observe a closed database and log a spurious failure on every shutdown. Add async.WaitGrouparound both goroutines and wait afterstopBackground.🐛 Wait for the background loops
background, stopBackground := context.WithCancel(context.WithoutCancel(ctx)) defer stopBackground() + var loops sync.WaitGroup + defer loops.Wait() sweeper := controlplane.NewSweeper(store) - go sweeper.Run(background, logger) - go admission.Run(background) + loops.Add(2) + go func() { defer loops.Done(); sweeper.Run(background, logger) }() + go func() { defer loops.Done(); admission.Run(background) }()
defer loops.Wait()must be registered afterdefer stopBackground()so it runs first.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cmd/jig/serve.go` around lines 145 - 153, Use a sync.WaitGroup to track both background goroutines started for sweeper.Run and admission.Run, calling Add before launch and Done on each goroutine’s return. Register defer loops.Wait() after defer stopBackground() so waiting occurs first during shutdown, ensuring both loops exit before the deferred store.Close runs.cmd/jig/run.go-126-130 (1)
126-130: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winAn unknown
--runtimevalue exits 1 instead of the documented 2.
selectRuntimereturnsunknown runtime %q — expected one of ...for a flag typo (cmd/jig/runtime.go lines 86-89). This block maps every selection error toexitInfraFailed, but the usage text on lines 50-54 reserves exit 2 for usage errors. A typo in--runtimeis a usage error, not an infrastructure failure.cmd/jig/worker.golines 112-116 have the same mapping.Validate the name against
runtimeNamesbefore callingselectRuntime, or return a sentinel error the callers can distinguish.🐛 Sentinel error the callers can classify
In
cmd/jig/runtime.go:+// errUnknownRuntime marks a bad --runtime value: a usage error, not an +// infrastructure failure. +var errUnknownRuntime = errors.New("unknown runtime") + default: - return selectedRuntime{}, fmt.Errorf("unknown runtime %q — expected one of %s", - name, strings.Join(runtimeNames, ", ")) + return selectedRuntime{}, fmt.Errorf("%w %q — expected one of %s", + errUnknownRuntime, name, strings.Join(runtimeNames, ", "))At each call site:
selected, err := selectRuntime(ctx, *runtimeName, !*noSeedAuth) if err != nil { fmt.Fprintf(stderr, "jig run: %v\n", err) + if errors.Is(err, errUnknownRuntime) { + return exitUsage + } return exitInfraFailed }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cmd/jig/run.go` around lines 126 - 130, Map unknown --runtime values to the documented usage exit code instead of exitInfraFailed. Update the runtime selection flow around selectRuntime and its corresponding call site in worker.go to distinguish the unknown-runtime error, using runtimeNames validation or a shared sentinel from runtime.go; retain infrastructure-failure handling for other selection errors.internal/controlplane/ingest_test.go-263-281 (1)
263-281: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winThe test passes only because
insertEventdoes not modify the original event when capping the payload.
insertEventreceives a struct copy and modifiesevent.Payloadin that copy before storing. The original event from the loop retains the full uncapped payload.projectEvidencereceives this original event, unmarshals the fullpayload.Raw(69632 bytes), and then caps it toMaxInvalidEnvelopeBytesbefore storing in theenvelopestable.If
insertEventwere refactored to cap the payload before the call toprojectEvidence(for example, by using a pointer parameter or a separate output),projectEvidencewould receive the truncated{"truncated": true, ...}object instead of the original payload. Theinvalid_envelopecase would fail to unmarshal the expected shape and skip the envelope row entirely, causing the test assertion to fail.Clarify whether both constants must remain equal or whether
oversizeshould be sized to exercise both caps deliberately. IfMaxEventPayloadBytescan differ fromMaxInvalidEnvelopeBytesin the future, add an assertion about their relationship to document this implicit dependency.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/controlplane/ingest_test.go` around lines 263 - 281, Update the oversize payload test around insertEvent and projectEvidence to explicitly document the dependency between MaxEventPayloadBytes and MaxInvalidEnvelopeBytes. Size oversize to exercise both limits deliberately, or assert their required relationship before constructing the batch, so the test remains valid if the constants diverge; preserve the invalid_envelope envelope-row assertion.web/src/SwimLane.tsx-89-99 (1)
89-99: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winFix the ARIA roles on the phase block and the mark pin.
role="listitem"on the<button>replaces the implicitbuttonrole. Assistive technology then announces a list item and not an activatable control, even though the element still receives focus and responds to Enter. Move the role to a wrapper element.
aria-labelon a plain<span>with no role is not exposed by most screen readers. GiveMarkPina role so its label reaches the accessibility tree.♿ Proposed change
- <button - type="button" - role="listitem" - className={`phase-block phase-${status}`} - style={{ - left: `${offsetPercent(span, startMs, endMs)}%`, - width: `${widthPercent(span, startMs, endMs, nowMs)}%`, - }} - aria-label={`phase ${span.label}, ${status}, ${duration}`} - onClick={() => onSelect?.(span.phase)} - > - <span className="phase-name">{span.label}</span> - <span className="phase-duration">{duration}</span> - </button> + <div + role="listitem" + className={`phase-block phase-${status}`} + style={{ + left: `${offsetPercent(span, startMs, endMs)}%`, + width: `${widthPercent(span, startMs, endMs, nowMs)}%`, + }} + > + <button + type="button" + className="phase-block-button" + aria-label={`phase ${span.label}, ${status}, ${duration}`} + onClick={() => onSelect?.(span.phase)} + > + <span className="phase-name">{span.label}</span> + <span className="phase-duration">{duration}</span> + </button> + </div><span + role="img" className={`mark mark-${mark.kind}`} style={{ left: `${left}%` }}The wrapper needs the absolute positioning rules in
styles.cssif you moveleft/widthto it.Also applies to: 110-117
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@web/src/SwimLane.tsx` around lines 89 - 99, Update the phase block markup in SwimLane so role="listitem" is applied to an absolutely positioned wrapper rather than the interactive button, preserving the wrapper’s left and width positioning through the existing styles.css rules while keeping the button’s implicit button role and behavior. Update MarkPin to assign an appropriate role to the labeled span so its aria-label is exposed to assistive technology.examples/definitions/simple-sdlc.yaml-133-149 (1)
133-149: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winDelete
$HOME/commit-messageon the nothing-to-commit path.The commit phase returns early when the index is clean. It leaves an authored message file behind.
commit-revisionlater reads the same path, so a stale message from the build phase can become the subject of the revision commit. Remove the file before the early exit.♻️ Proposed change
git add -A if git diff --cached --quiet; then echo "commit-build: nothing to commit" + rm -f "$message" exit 0 fi🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@examples/definitions/simple-sdlc.yaml` around lines 133 - 149, Update the clean-index early-exit path in the commit command to remove $HOME/commit-message before returning. Preserve the existing “nothing to commit” output and exit behavior, while ensuring stale build-phase messages cannot be consumed by commit-revision.cmd/jig/definitions_test.go-28-50 (1)
28-50: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winFilter the directory entries before you parse them.
The loop treats every entry as a definition file. A subdirectory makes
os.ReadFilefail and stops the test witht.Fatal. AREADME.mdor a.ymlfile failsParseDefinition, and thestrings.TrimSuffix(entry.Name(), ".yaml")comparison is wrong for any other extension. Thelen(entries) < 5check counts those entries too.💚 Proposed fix
- if len(entries) < 5 { - t.Fatalf("the stock library has %d definitions; the plan ships five", len(entries)) - } + var definitions []string for _, entry := range entries { - source, err := os.ReadFile(stockDefinition(entry.Name())) + if entry.IsDir() || filepath.Ext(entry.Name()) != ".yaml" { + continue + } + definitions = append(definitions, entry.Name()) + } + if len(definitions) < 5 { + t.Fatalf("the stock library has %d definitions; the plan ships five", len(definitions)) + } + for _, name := range definitions { + source, err := os.ReadFile(stockDefinition(name)) if err != nil { t.Fatal(err) } spec, err := protocol.ParseDefinition(source) if err != nil { - t.Errorf("%s does not validate: %v", entry.Name(), err) + t.Errorf("%s does not validate: %v", name, err) continue } // The file name is how an operator refers to it; a definition whose // declared name disagrees is a definition that lists confusingly. - if want := strings.TrimSuffix(entry.Name(), ".yaml"); spec.Name != want { - t.Errorf("%s declares name %q", entry.Name(), spec.Name) + if want := strings.TrimSuffix(name, ".yaml"); spec.Name != want { + t.Errorf("%s declares name %q", name, spec.Name) } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cmd/jig/definitions_test.go` around lines 28 - 50, Filter the entries returned by os.ReadDir before the count and parsing loop, retaining only regular files with the .yaml extension. Apply the filtered collection to the len check, os.ReadFile, and name validation so non-definition files and directories are excluded.web/src/lanes.ts-204-210 (1)
204-210: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winReplace the argument spreads in
laneEnd.
Math.maxreceives one argument per phase, tool, and mark. Engines cap the argument count, so a long attempt with a large event count raisesRangeError: too many function argumentsand the detail view stops rendering. Fold the maximum instead.🐛 Proposed fix
if (lane.endMs !== null) return Math.max(lane.endMs, lane.startMs + 1); - const latest = Math.max( - lane.startMs, - ...lane.phases.map((span) => span.endMs ?? span.startMs), - ...lane.tools.map((span) => span.endMs ?? span.startMs), - ...lane.marks.map((mark) => mark.atMs), - ); + let latest = lane.startMs; + for (const span of lane.phases) latest = Math.max(latest, span.endMs ?? span.startMs); + for (const span of lane.tools) latest = Math.max(latest, span.endMs ?? span.startMs); + for (const mark of lane.marks) latest = Math.max(latest, mark.atMs); return Math.max(nowMs, latest, lane.startMs + 1);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@web/src/lanes.ts` around lines 204 - 210, Update laneEnd to avoid spreading phase, tool, and mark arrays into Math.max; compute their maximum values incrementally or via reductions, then combine those maxima with lane.startMs while preserving the existing nowMs and minimum-duration behavior.web/src/format.ts-16-22 (1)
16-22: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winCarry the rounded seconds into the minute.
Math.roundcan return 60, soformatDuration(119_700)renders1m 60s. The same rounding makesformatDuration(59_600)render60s. Round the total seconds once before you split the value.🐛 Proposed fix
- const seconds = milliseconds / 1000; - if (seconds < 60) return `${seconds.toFixed(seconds < 10 ? 1 : 0)}s`; - const minutes = Math.floor(seconds / 60); - const rest = Math.round(seconds - minutes * 60); - if (minutes < 60) return `${minutes}m ${rest}s`; - const hours = Math.floor(minutes / 60); - return `${hours}h ${minutes - hours * 60}m`; + const exact = milliseconds / 1000; + if (exact < 9.95) return `${exact.toFixed(1)}s`; + const seconds = Math.round(exact); + if (seconds < 60) return `${seconds}s`; + const minutes = Math.floor(seconds / 60); + if (minutes < 60) return `${minutes}m ${seconds - minutes * 60}s`; + const hours = Math.floor(minutes / 60); + return `${hours}h ${minutes - hours * 60}m`;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@web/src/format.ts` around lines 16 - 22, Update formatDuration’s duration decomposition to round the total seconds once before calculating minutes and remaining seconds. Use that rounded total for all subsequent unit calculations so values such as 119_700 milliseconds normalize to 2m 0s and 59_600 milliseconds normalize to 1m rather than emitting 60 seconds.docs/quickstart.md-36-45 (1)
36-45: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winSpecify the output fence language.
Add
textto each output fence. These fences trigger markdownlint rule MD040.Proposed documentation fix
-``` +```text run: ee31d5e7-baae-415d-b421-11d6b135419e ...</details> Also applies to: 127-129, 164-169 <details> <summary>🤖 Prompt for AI Agents</summary>Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.In
@docs/quickstart.mdaround lines 36 - 45, Update every output code fence in
the quickstart documentation, including the sections around the shown output and
the referenced additional locations, to specify the text language as ```text
while preserving all output content unchanged.</details> <!-- cr-comment:v1:86fcf6f0fc2f9ee9f5d937f7 --> _Source: Linters/SAST tools_ </blockquote></details> <details> <summary>README.md-18-22 (1)</summary><blockquote> `18-22`: _📐 Maintainability & Code Quality_ | _🟡 Minor_ | _⚡ Quick win_ **Specify the diagram fence language.** Add `text` to this fence. The current fence triggers markdownlint rule MD040. <details> <summary>Proposed documentation fix</summary> ```diff -``` +```text definition (YAML) → run (frozen by value) → job per repository → attempt │ phase → phase → phase, each gated ────────┘ ``` ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@README.md` around lines 18 - 22, Update the fenced diagram in the README to specify the text language, changing the opening fence from an untyped fence to a text fence while preserving the diagram content unchanged. ``` </details> <!-- cr-comment:v1:9d63441550162a8b10c2e2e9 --> _Source: Linters/SAST tools_ </blockquote></details> <details> <summary>web/src/styles.css-17-17 (1)</summary><blockquote> `17-17`: _📐 Maintainability & Code Quality_ | _🟡 Minor_ | _⚡ Quick win_ **Fix the Stylelint declaration-spacing error.** Stylelint requires an empty line before `font-family` at line 17. <details> <summary>Proposed fix</summary> ```diff --bad: `#f87171`; --tool: `#7c8cf8`; + font-family: ui-sans-serif, system-ui, -apple-system, "Segoe UI", Roboto, sans-serif; ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@web/src/styles.css` at line 17, Insert an empty line immediately before the font-family declaration in the stylesheet to satisfy Stylelint’s declaration-spacing rule, without changing the declaration itself. ``` </details> <!-- cr-comment:v1:a8a870488cc162fb7cf4de3f --> _Source: Linters/SAST tools_ </blockquote></details> <details> <summary>web/src/router.ts-19-20 (1)</summary><blockquote> `19-20`: _🩺 Stability & Availability_ | _🟡 Minor_ | _⚡ Quick win_ **Handle malformed encoded route IDs.** If `decodeURIComponent` fails for `/runs/%` or `/jobs/%`, return `{ name: "unknown", path }` so invalid URLs do not break route parsing. <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@web/src/router.ts` around lines 19 - 20, Handle decodeURIComponent failures in the route parsing branches for “runs” and “jobs” by catching malformed encoded IDs and returning { name: "unknown", path }. Preserve the existing decoded run/job results for valid route IDs and use the current path value in the fallback. ``` </details> <!-- cr-comment:v1:cca28a0fd05d5233e8d088c8 --> </blockquote></details> </blockquote></details> <details> <summary>🧹 Nitpick comments (9)</summary><blockquote> <details> <summary>cmd/jig/def.go (1)</summary><blockquote> `649-656`: _🎯 Functional Correctness_ | _🔵 Trivial_ | _⚡ Quick win_ **Use `errors.New` for the message-only rejection.** Line 653 passes a runtime string as the format argument to `fmt.Errorf` with no arguments. Any `%` in the control plane's message is then interpreted as a verb and renders as `%!s(MISSING)`. `errors.New` avoids that. <details> <summary>♻️ Replace with errors.New</summary> ```diff - return fmt.Errorf("%s", document.Error.Message) + return errors.New(document.Error.Message) ``` Add `"errors"` to the import block. </details> <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cmd/jig/def.go` around lines 649 - 656, Update the message-only rejection in the error-handling branch of the control-plane response to use errors.New with document.Error.Message, and add the errors import. Leave the formatted error path for document.Error.Code unchanged. ``` </details> <!-- cr-comment:v1:d607421344e62a63c33862b7 --> </blockquote></details> <details> <summary>cmd/jig/serve_test.go (1)</summary><blockquote> `53-67`: _📐 Maintainability & Code Quality_ | _🔵 Trivial_ | _💤 Low value_ **Simplify the listen-address parse.** Lines 56-58 call `strings.CutPrefix`, discard the remainder, then recompute the same value with `Split` and `TrimPrefix`. Use the remainder that `CutPrefix` already returns. <details> <summary>♻️ Use the CutPrefix remainder</summary> ```diff - if _, found := strings.CutPrefix(stdout.String(), "jig serve: listening on "); found { - base = strings.TrimSpace(strings.TrimPrefix( - strings.Split(stdout.String(), "\n")[0], "jig serve: listening on ")) - break + if rest, found := strings.CutPrefix(stdout.String(), "jig serve: listening on "); found { + line, _, complete := strings.Cut(rest, "\n") + if !complete { + continue + } + base = strings.TrimSpace(line) + break } ``` The `complete` check avoids reading a partially written line. </details> <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cmd/jig/serve_test.go` around lines 53 - 67, Update the listen-address parsing loop in the serve test to capture and reuse the remainder returned by strings.CutPrefix instead of recomputing it with strings.Split and strings.TrimPrefix. Retain the complete-prefix check so partially written output is ignored, then trim the captured remainder before assigning it to base. ``` </details> <!-- cr-comment:v1:5359fbc6d35beb126bb5084b --> </blockquote></details> <details> <summary>cmd/jig/run.go (1)</summary><blockquote> `381-400`: _📐 Maintainability & Code Quality_ | _🔵 Trivial_ | _💤 Low value_ **The scripted event cannot carry `Text` or per-step `Usage`.** `runtime.Event` has a `Text` field, and `enginetest.Step` has a `Usage` field. Neither is reachable from the script schema, so a scripted run cannot exercise text-bearing events or cost accounting. Add the keys when a scenario needs them. <details> <summary>♻️ Add the text key</summary> ```diff Events []struct { Type string `json:"type"` Name string `json:"name"` + Text string `json:"text"` Payload json.RawMessage `json:"payload"` } `json:"events"` ``` ```diff events = append(events, runtime.Event{ Kind: event.Type, Name: event.Name, + Text: event.Text, Payload: event.Payload, }) ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cmd/jig/run.go` around lines 381 - 400, Extend the scripted runtime schema and conversion in the script-loading flow around json.Unmarshal and the event loop to expose runtime.Event.Text and enginetest.Step.Usage. Add the corresponding JSON keys to the parsed event and step structures, then copy them into the constructed runtime.Event and enginetest.Step so scripted scenarios support text-bearing events and per-step usage. ``` </details> <!-- cr-comment:v1:08917111b2ac6a0e100de539 --> </blockquote></details> <details> <summary>cmd/jig/worker_test.go (1)</summary><blockquote> `302-315`: _📐 Maintainability & Code Quality_ | _🔵 Trivial_ | _⚡ Quick win_ **Parse the JSON instead of scanning for the field marker.** `jsonField` finds the first occurrence of `"<field>": ` anywhere in the payload, including inside nested objects, and it does not handle escaped quotes. Decode the payload instead. The result is shorter and cannot match the wrong field. <details> <summary>♻️ Decode the payload</summary> ```diff -// jsonField pulls one top-level string field out of a --json payload. +// jsonField pulls one top-level string field out of a --json payload. func jsonField(t *testing.T, body, field string) string { t.Helper() - marker := fmt.Sprintf("%q: ", field) - index := strings.Index(body, marker) - if index < 0 { - t.Fatalf("payload has no %q field:\n%s", field, body) - } - rest := body[index+len(marker):] - value, _, _ := strings.Cut(strings.TrimPrefix(strings.TrimSpace(rest), `"`), `"`) - if value == "" { - t.Fatalf("field %q is empty:\n%s", field, body) - } - return value + var document map[string]any + if err := json.Unmarshal([]byte(body), &document); err != nil { + t.Fatalf("payload is not a JSON object: %v\n%s", err, body) + } + value, ok := document[field].(string) + if !ok || value == "" { + t.Fatalf("field %q is missing or empty:\n%s", field, body) + } + return value } ``` This removes the `fmt` import requirement if no other use remains. </details> <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cmd/jig/worker_test.go` around lines 302 - 315, Update jsonField to unmarshal body as JSON into a map and retrieve the requested top-level field, preserving fatal errors for invalid JSON, missing fields, or empty values. Remove the now-unused fmt and strings imports if no other code uses them. ``` </details> <!-- cr-comment:v1:dc8f2739f5562a7d2eeb5dc0 --> </blockquote></details> <details> <summary>internal/controlplane/ingest.go (1)</summary><blockquote> `87-100`: _🗄️ Data Integrity & Integration_ | _🔵 Trivial_ | _💤 Low value_ **Consider projecting evidence from the capped event.** `insertEvent` caps `event.Payload` on its own copy, so line 97 passes the uncaught, uncapped payload to `projectEvidence`. The `invalid_envelope` branch caps `body` separately, but the gate branch inserts `check.Item` and `check.Note` verbatim. An oversized gate payload therefore produces `gate_results` rows larger than the stored `events` row. Cap once before both calls. <details> <summary>♻️ Cap the payload once</summary> ```diff for _, event := range batch.Events { + event.Payload = capIngestedPayload(event.Payload) inserted, err := insertEvent(ctx, tx, attemptID, event, now) ``` Then remove the capping line from `insertEvent`. </details> <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/controlplane/ingest.go` around lines 87 - 100, Cap the event payload once in the batch loop before calling insertEvent, then pass that capped event to both insertEvent and projectEvidence so stored events and projected evidence match. Remove the independent payload-capping logic from insertEvent and preserve the existing replay and error-handling flow. ``` </details> <!-- cr-comment:v1:c47ed183ac882a37a65013c0 --> </blockquote></details> <details> <summary>web/src/lanes.ts (1)</summary><blockquote> `93-95`: _🎯 Functional Correctness_ | _🔵 Trivial_ | _💤 Low value_ **Close the open tool span before you replace it.** The `tool_call` branch assigns `openTool` without closing the previous span when the previous span belongs to another phase. That span then keeps `endMs === null` and renders as running until a `phase_end` arrives. A `phase_end` for the earlier phase does close it, so the gap only appears when events from two phases interleave. Close unconditionally on a new `tool_call` to remove the dependency on event interleaving. <details> <summary>♻️ Proposed change</summary> ```diff case "tool_call": { + closeTool(at); const span: Span = { ``` </details> Also applies to: 132-143 <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@web/src/lanes.ts` around lines 93 - 95, Update the tool_call handling around openTool so any existing open tool span is closed before assigning the new tool span, regardless of phase. Preserve the existing phase_end behavior while ensuring interleaved phases cannot leave the previous span with endMs === null. ``` </details> <!-- cr-comment:v1:04611bed415d0c87bffc4dea --> </blockquote></details> <details> <summary>web/embed.go (1)</summary><blockquote> `84-90`: _📐 Maintainability & Code Quality_ | _🔵 Trivial_ | _💤 Low value_ **Consider `http.ServeContent` for the SPA shell.** The current code writes the index body directly. A HEAD response therefore carries no `Content-Length`, and no `ETag` or `Last-Modified` is offered. `http.ServeContent` handles the HEAD case, sets the length, and keeps the `Cache-Control: no-cache` header you already set. <details> <summary>♻️ Proposed change</summary> ```diff func (h *handler) serveIndex(w http.ResponseWriter, r *http.Request) { w.Header().Set("Content-Type", "text/html; charset=utf-8") w.Header().Set("Cache-Control", "no-cache") - if r.Method == http.MethodGet { - _, _ = w.Write(h.index) - } + http.ServeContent(w, r, "index.html", h.modTime, bytes.NewReader(h.index)) } ``` </details> Store a fixed `modTime` (for example the build time or the zero time) on `handler` if you adopt this. <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@web/embed.go` around lines 84 - 90, Update handler.serveIndex to serve the SPA index through http.ServeContent instead of writing h.index directly, preserving the existing Content-Type and Cache-Control headers. Add and use a fixed handler modTime value, such as build time or zero time, so ServeContent can provide correct HEAD handling, Content-Length, and modification metadata. ``` </details> <!-- cr-comment:v1:da0e0ed91b583a1f72978270 --> </blockquote></details> <details> <summary>web/embed_test.go (1)</summary><blockquote> `70-76`: _📐 Maintainability & Code Quality_ | _🔵 Trivial_ | _⚡ Quick win_ **Add HEAD coverage.** The handler allows GET and HEAD, and `serveIndex` branches on the method. No test exercises HEAD, so a regression that returns a body or a wrong status for HEAD would pass. <details> <summary>💚 Proposed test</summary> ```diff func TestNonReadMethodsAreRefused(t *testing.T) { recorder := httptest.NewRecorder() Handler().ServeHTTP(recorder, httptest.NewRequest(http.MethodPost, "/", nil)) if recorder.Code != http.StatusMethodNotAllowed { t.Fatalf("expected 405 for POST, got %d", recorder.Code) } } + +func TestHeadReturnsHeadersWithoutABody(t *testing.T) { + recorder := httptest.NewRecorder() + Handler().ServeHTTP(recorder, httptest.NewRequest(http.MethodHead, "/runs", nil)) + if recorder.Code != http.StatusOK { + t.Fatalf("expected 200 for HEAD, got %d", recorder.Code) + } + if recorder.Body.Len() != 0 { + t.Fatalf("HEAD must not carry a body, got %d bytes", recorder.Body.Len()) + } +} ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@web/embed_test.go` around lines 70 - 76, Add HEAD-method coverage to TestNonReadMethodsAreRefused or a nearby handler test, invoking Handler().ServeHTTP with http.MethodHead and asserting the expected successful status and HEAD semantics, including no response body. Keep the existing POST rejection assertion unchanged. ``` </details> <!-- cr-comment:v1:36da24219cd2999501ad77b5 --> </blockquote></details> <details> <summary>examples/definitions/simple-sdlc.yaml (1)</summary><blockquote> `150-158`: _📐 Maintainability & Code Quality_ | _🔵 Trivial_ | _💤 Low value_ **Use YAML anchors to eliminate duplication across test, retest, commit-build, and commit-revision phases.** The `test` and `retest` phases (lines 150–158, 186–195) contain identical test-detection scripts. The `commit-build` and `commit-revision` phases (lines 130–149, 165–185) contain identical commit commands; only log messages differ. YAML anchors and aliases will keep these blocks synchronized when the commands are edited. The definition parser's `KnownFields(true)` setting does not block YAML anchors and aliases, since the YAML parser resolves them before struct field validation occurs. <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@examples/definitions/simple-sdlc.yaml` around lines 150 - 158, Use YAML anchors to centralize the shared test-detection script between the test and retest phases, and the shared commit command between commit-build and commit-revision. Define each command once on the appropriate phase and reference it with aliases in the duplicate phases, preserving the existing phase-specific log messages. ``` </details> <!-- cr-comment:v1:3273e55758c1d33197657c4c --> </blockquote></details> </blockquote></details> <details> <summary>🤖 Prompt for all review comments with AI agents</summary>Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.Inline comments:
In @.github/workflows/ci.yml:
- Around line 57-64: Harden the ui job by adding job-level permissions limited
to contents: read, and update its actions/checkout step to set
persist-credentials: false. Keep the existing checkout revision and
setup-node/install flow unchanged.In @.github/workflows/release.yml:
- Around line 22-24: Separate the build and publication workflows so the build
job uses read-only contents permissions, while a dedicated publish job runs only
when the ref matches refs/tags/v*. Have the publish job depend on the build,
download its artifact, and assign contents: write only within that job.In
@cmd/jig/worker.go:
- Around line 319-338: Update anyClosed to return both the merged cancellation
channel and a release function that closes merged through the existing
sync.Once, allowing waiting goroutines to exit when the attempt completes. At
the call site that invokes anyClosed for Execute, capture the release function
and register defer releaseCancellation() before returning, ensuring it runs
after Execute returns.In
@internal/worker/events.go:
- Around line 480-506: Update TraceStream.sendLoop to track a retry deadline
whenever sendOnce fails, and ensure wake events do not trigger another send
until that deadline has passed. When nudge wakes the loop during backoff,
reschedule or wait on the timer rather than calling sendOnce; retain immediate
wake-driven sends when no retry backoff is active and preserve the existing
exponential backoff limits.- Around line 544-563: Update sendOnce’s ingest-error handling to classify
invalid_event_seq, invalid_event_type, and lease_not_owner for the current token
as permanent rejections: remove the rejected batch, record a valid gap marker
with the appropriate reason, and do not restore it to s.pending. Continue
prepending the batch and retrying for transport and storage failures, while
preserving the existing warning and return behavior.In
@web/src/polling.ts:
- Around line 141-148: The polling stop condition in the arrived-empty branch
must not terminate after two empty pages based only on terminal status. Update
the polling flow around arrived, terminal, and idleRef to continue until the
event API exposes an explicit finalized/completion signal, or until the control
plane guarantees ingestion has ended; use that signal as the sole condition for
clearing timer.In
@web/tsconfig.json:
- Around line 3-6: Update the referenced projects in tsconfig.app.json and
tsconfig.node.json to use build-compatible configurations with composite enabled
and emit output enabled, then retain separate no-emit configurations for regular
type checking. Ensure the references in tsconfig.json target the build configs
so tsc -b no longer reports TS6306 or TS6310.
Minor comments:
In@cmd/jig/def.go:
- Around line 418-434: Validate *kind against the accepted trigger vocabulary
before entering the schedule/GitHub repository handling in the trigger creation
flow. Include the exported constant for the pull-request trigger alongside
controlplane.TriggerSchedule and the valid GitHub kind, and return the existing
usage error naming the unknown kind before repository-count validation; preserve
the current schedule and single-repository behavior for valid kinds.In
@cmd/jig/definitions_test.go:
- Around line 28-50: Filter the entries returned by os.ReadDir before the count
and parsing loop, retaining only regular files with the .yaml extension. Apply
the filtered collection to the len check, os.ReadFile, and name validation so
non-definition files and directories are excluded.In
@cmd/jig/main.go:
- Around line 4-7: Update the introductory command-count comment in main to
include the version subcommand and accurately state six subcommands, naming
version alongside run, serve, worker, def, and trigger so it matches the usage
text.In
@cmd/jig/run.go:
- Around line 126-130: Map unknown --runtime values to the documented usage exit
code instead of exitInfraFailed. Update the runtime selection flow around
selectRuntime and its corresponding call site in worker.go to distinguish the
unknown-runtime error, using runtimeNames validation or a shared sentinel from
runtime.go; retain infrastructure-failure handling for other selection errors.In
@cmd/jig/runtime.go:
- Around line 117-127: Update the credential fallback in HomeSeeder to attempt
thesecuritykeychain lookup only whengoruntime.GOOSisdarwin; otherwise
return the existing missing-credentials error without invoking macOS tooling.
Create a locally bounded context using a short timeout fromtimeand pass it
toexec.CommandContextfor the keychain command, preserving the existing
extracted-credentials and error handling behavior.In
@cmd/jig/serve_test.go:
- Line 90: Resolve the noctx lint findings in the test calls: replace each
http.Get invocation with an http.NewRequestWithContext request executed via
http.DefaultClient.Do, and replace db.Exec with db.ExecContext. Update the
affected call sites around the existing health checks and database operation,
supplying the appropriate test context while preserving current request and
query behavior.In
@cmd/jig/serve.go:
- Around line 145-153: Use a sync.WaitGroup to track both background goroutines
started for sweeper.Run and admission.Run, calling Add before launch and Done on
each goroutine’s return. Register defer loops.Wait() after defer
stopBackground() so waiting occurs first during shutdown, ensuring both loops
exit before the deferred store.Close runs.In
@docs/quickstart.md:
- Around line 36-45: Update every output code fence in the quickstart
documentation, including the sections around the shown output and the referenced
additional locations, to specify the text language as ```text while preserving
all output content unchanged.In
@examples/definitions/simple-sdlc.yaml:
- Around line 133-149: Update the clean-index early-exit path in the commit
command to remove $HOME/commit-message before returning. Preserve the existing
“nothing to commit” output and exit behavior, while ensuring stale build-phase
messages cannot be consumed by commit-revision.In
@internal/controlplane/ingest_test.go:
- Around line 263-281: Update the oversize payload test around insertEvent and
projectEvidence to explicitly document the dependency between
MaxEventPayloadBytes and MaxInvalidEnvelopeBytes. Size oversize to exercise both
limits deliberately, or assert their required relationship before constructing
the batch, so the test remains valid if the constants diverge; preserve the
invalid_envelope envelope-row assertion.In
@internal/controlplane/ingest.go:
- Around line 270-275: Update AttemptGateEvidence and AttemptEnvelopes to match
AttemptEventPage by calling s.Attempt(ctx, attemptID) before querying their
tables, so an unknown attempt returns the same not-found error instead of an
empty 200 response. Keep the existing query and result handling unchanged for
valid attempts, and apply the same existence guard in both methods using the
visible Attempt, AttemptGateEvidence, and AttemptEnvelopes symbols.In
@internal/worker/events_test.go:
- Around line 239-242: Replace the exact pending-count assertion in the outage
test with assertions that no event beyond sequence 1 was delivered and dropped
remains zero. Avoid relying on the transient value returned by
stream.Undelivered while sendOnce is processing a failed batch.- Around line 76-78: Update discardLogger to use an io.Discard-backed slog
handler instead of os.NewFile(0, os.DevNull), and add the required io import
while removing any now-unused os import.In
@internal/worker/events.go:
- Around line 569-573: Update TraceStream.Undelivered to include the batch
currently being processed by sendOnce in its pending count, using the existing
in-flight batch state and protecting the read with the mutex. Preserve the
current pending and dropped counts when no batch is in flight.In
@README.md:
- Around line 18-22: Update the fenced diagram in the README to specify the text
language, changing the opening fence from an untyped fence to a text fence while
preserving the diagram content unchanged.In
@web/src/format.ts:
- Around line 16-22: Update formatDuration’s duration decomposition to round the
total seconds once before calculating minutes and remaining seconds. Use that
rounded total for all subsequent unit calculations so values such as 119_700
milliseconds normalize to 2m 0s and 59_600 milliseconds normalize to 1m rather
than emitting 60 seconds.In
@web/src/lanes.ts:
- Around line 204-210: Update laneEnd to avoid spreading phase, tool, and mark
arrays into Math.max; compute their maximum values incrementally or via
reductions, then combine those maxima with lane.startMs while preserving the
existing nowMs and minimum-duration behavior.In
@web/src/router.ts:
- Around line 19-20: Handle decodeURIComponent failures in the route parsing
branches for “runs” and “jobs” by catching malformed encoded IDs and returning {
name: "unknown", path }. Preserve the existing decoded run/job results for valid
route IDs and use the current path value in the fallback.In
@web/src/styles.css:
- Line 17: Insert an empty line immediately before the font-family declaration
in the stylesheet to satisfy Stylelint’s declaration-spacing rule, without
changing the declaration itself.In
@web/src/SwimLane.tsx:
- Around line 89-99: Update the phase block markup in SwimLane so
role="listitem" is applied to an absolutely positioned wrapper rather than the
interactive button, preserving the wrapper’s left and width positioning through
the existing styles.css rules while keeping the button’s implicit button role
and behavior. Update MarkPin to assign an appropriate role to the labeled span
so its aria-label is exposed to assistive technology.
Nitpick comments:
In@cmd/jig/def.go:
- Around line 649-656: Update the message-only rejection in the error-handling
branch of the control-plane response to use errors.New with
document.Error.Message, and add the errors import. Leave the formatted error
path for document.Error.Code unchanged.In
@cmd/jig/run.go:
- Around line 381-400: Extend the scripted runtime schema and conversion in the
script-loading flow around json.Unmarshal and the event loop to expose
runtime.Event.Text and enginetest.Step.Usage. Add the corresponding JSON keys to
the parsed event and step structures, then copy them into the constructed
runtime.Event and enginetest.Step so scripted scenarios support text-bearing
events and per-step usage.In
@cmd/jig/serve_test.go:
- Around line 53-67: Update the listen-address parsing loop in the serve test to
capture and reuse the remainder returned by strings.CutPrefix instead of
recomputing it with strings.Split and strings.TrimPrefix. Retain the
complete-prefix check so partially written output is ignored, then trim the
captured remainder before assigning it to base.In
@cmd/jig/worker_test.go:
- Around line 302-315: Update jsonField to unmarshal body as JSON into a map and
retrieve the requested top-level field, preserving fatal errors for invalid
JSON, missing fields, or empty values. Remove the now-unused fmt and strings
imports if no other code uses them.In
@examples/definitions/simple-sdlc.yaml:
- Around line 150-158: Use YAML anchors to centralize the shared test-detection
script between the test and retest phases, and the shared commit command between
commit-build and commit-revision. Define each command once on the appropriate
phase and reference it with aliases in the duplicate phases, preserving the
existing phase-specific log messages.In
@internal/controlplane/ingest.go:
- Around line 87-100: Cap the event payload once in the batch loop before
calling insertEvent, then pass that capped event to both insertEvent and
projectEvidence so stored events and projected evidence match. Remove the
independent payload-capping logic from insertEvent and preserve the existing
replay and error-handling flow.In
@web/embed_test.go:
- Around line 70-76: Add HEAD-method coverage to TestNonReadMethodsAreRefused or
a nearby handler test, invoking Handler().ServeHTTP with http.MethodHead and
asserting the expected successful status and HEAD semantics, including no
response body. Keep the existing POST rejection assertion unchanged.In
@web/embed.go:
- Around line 84-90: Update handler.serveIndex to serve the SPA index through
http.ServeContent instead of writing h.index directly, preserving the existing
Content-Type and Cache-Control headers. Add and use a fixed handler modTime
value, such as build time or zero time, so ServeContent can provide correct HEAD
handling, Content-Length, and modification metadata.In
@web/src/lanes.ts:
- Around line 93-95: Update the tool_call handling around openTool so any
existing open tool span is closed before assigning the new tool span, regardless
of phase. Preserve the existing phase_end behavior while ensuring interleaved
phases cannot leave the previous span with endMs === null.</details> <details> <summary>🪄 Autofix</summary> Fix all unresolved CodeRabbit comments on this PR: - [ ] <!-- {"checkboxId": "4b0d0e0a-96d7-4f10-b296-3a18ea78f0b9"} --> Push a commit to this branch (recommended) - [ ] <!-- {"checkboxId": "ff5b1114-7d8c-49e6-8ac1-43f82af23a33"} --> Create a new PR with the fixes </details> --- <details> <summary>ℹ️ Review info</summary> <details> <summary>⚙️ Run configuration</summary> **Configuration used**: defaults **Review profile**: CHILL **Plan**: Pro Plus **Run ID**: `52b30671-5658-4e9d-b27a-2ca08b212ef9` </details> <details> <summary>📥 Commits</summary> Reviewing files that changed from the base of the PR and between d55c05a27e149ba7327729f83fd32f18d00948ef and 635f88b19d3c84b4e9c6adbb1e79f52c128e9042. </details> <details> <summary>⛔ Files ignored due to path filters (4)</summary> * `web/dist/assets/index-CMBiLIMc.js` is excluded by `!**/dist/**` * `web/dist/assets/index-vg-WGSXA.css` is excluded by `!**/dist/**` * `web/dist/index.html` is excluded by `!**/dist/**` * `web/package-lock.json` is excluded by `!**/package-lock.json` </details> <details> <summary>📒 Files selected for processing (52)</summary> * `.github/workflows/ci.yml` * `.github/workflows/release.yml` * `.gitignore` * `Justfile` * `README.md` * `cmd/jig/def.go` * `cmd/jig/definitions_test.go` * `cmd/jig/main.go` * `cmd/jig/run.go` * `cmd/jig/runtime.go` * `cmd/jig/serve.go` * `cmd/jig/serve_test.go` * `cmd/jig/worker.go` * `cmd/jig/worker_test.go` * `docs/quickstart.md` * `examples/definitions/plan-build-test.yaml` * `examples/definitions/scout.yaml` * `examples/definitions/simple-sdlc.yaml` * `internal/controlplane/embedded.go` * `internal/controlplane/http.go` * `internal/controlplane/ingest.go` * `internal/controlplane/ingest_test.go` * `internal/protocol/types.go` * `internal/worker/events.go` * `internal/worker/events_test.go` * `scripts/release.sh` * `web/.gitignore` * `web/embed.go` * `web/embed_test.go` * `web/index.html` * `web/package.json` * `web/src/App.tsx` * `web/src/Queue.tsx` * `web/src/RunDetail.test.tsx` * `web/src/RunDetail.tsx` * `web/src/SwimLane.tsx` * `web/src/Worktrees.tsx` * `web/src/api.ts` * `web/src/format.ts` * `web/src/lanes.test.ts` * `web/src/lanes.ts` * `web/src/main.tsx` * `web/src/polling.ts` * `web/src/router.ts` * `web/src/styles.css` * `web/src/test/setup.ts` * `web/src/types.ts` * `web/src/ui.tsx` * `web/tsconfig.app.json` * `web/tsconfig.json` * `web/tsconfig.node.json` * `web/vite.config.ts` </details> <details> <summary>🚧 Files skipped from review as they are similar to previous changes (1)</summary> * internal/controlplane/http.go </details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
| - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 | ||
| - uses: actions/setup-node@2028fbc5c25fe9cf00d9f06a71cc4710d4507903 # v6.0.0 | ||
| with: | ||
| node-version: 24.18.0 | ||
| cache: npm | ||
| cache-dependency-path: web/package-lock.json | ||
| - name: Install | ||
| run: npm ci |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
# First, check the actual workflow file to see the full context and current permissions
cat -n .github/workflows/ci.yml | head -100Repository: StructuPath/jig
Length of output: 3498
🌐 Web query:
actions/checkout persist-credentials default behavior GitHub Actions
💡 Result:
The persist-credentials input for the actions/checkout GitHub Action defaults to true [1][2]. When set to true, the action configures the authentication token (or SSH key) used to fetch the repository into the local git configuration [3][4]. This allows subsequent steps in your workflow to execute authenticated git commands (e.g., git fetch, git push) without needing to manually re-authenticate [3][5][4]. To maintain security, the action performs a post-job cleanup to remove these credentials [3][4]. In recent versions of the action, credentials are stored in a separate file under $RUNNER_TEMP rather than being written directly into the.git/config file, which improves security [3][6][7]. You can set persist-credentials to false if you wish to opt out of this behavior and prevent the action from configuring these credentials [3][5]. Although there have been community discussions and pull requests proposing a change to the default value to improve security, it remains true by default as of August 2026 [8][2].
Citations:
- 1: https://github.com/actions/checkout/blob/master/action.yml
- 2: https://github.com/actions/checkout/blob/main/action.yml
- 3: https://github.com/actions/checkout/?tab=readme-ov-file
- 4: https://github.com/actions/checkout/blob/master/README.md
- 5: https://github.com/actions/checkout/blob/master/adrs/0153-checkout-v2.md
- 6: https://github.com/actions/checkout/tree/v6.0.2?tab=readme-ov-file
- 7: https://github.com/actions/checkout
- 8: Change the default value of persist-credentials to false actions/checkout#1687
🌐 Web query:
GITHUB_TOKEN security risk npm ci GitHub Actions persist-credentials
💡 Result:
The primary security risk associated with GITHUB_TOKEN and actions/checkout is that by default, the checkout action persists credentials (the GITHUB_TOKEN) into the runner's local.git/config file [1][2][3]. When this occurs, any subsequent step in the workflow—including third-party actions, build scripts, or dependency installation commands like npm ci—can potentially access and exfiltrate these credentials [4][2][3]. If a compromised or malicious dependency executes code during the installation process (e.g., via npm scripts like preinstall or postinstall), it can read the persisted token from the.git/config file and use it to perform unauthorized actions, such as pushing code, modifying releases, or dispatching workflows, depending on the token's granted permissions [4][5][2]. To mitigate this risk, you should follow these security best practices: 1. Explicitly disable credential persistence: Set persist-credentials to false in your actions/checkout step unless your workflow specifically requires authenticated git operations (like pushing commits back to the repository) [1][4][2][3]. - uses: actions/checkout@v4 with: persist-credentials: false 2. Implement least-privilege permissions: Always define an explicit permissions block at the job or workflow level [3][6][7]. Grant only the minimum necessary permissions required for the task rather than relying on default settings, which are often too broad [5][6][8]. permissions: contents: read # Default to read-only where possible 3. Harden dependency installation: Since commands like npm ci can execute arbitrary scripts from dependencies, consider using flags like --ignore-scripts to prevent them from running during installation, especially if you have not fully audited the dependency tree [7]. By setting persist-credentials to false, you ensure that the GITHUB_TOKEN is not stored in the git configuration, effectively preventing malicious scripts from easily harvesting the token during the CI process [4][2].
Citations:
- 1: https://github.com/actions/checkout
- 2: https://actsense.dev/vulnerabilities/unsafe_checkout/
- 3: https://blog.stephane-robert.info/en/post/github-actions-15-security-pitfalls/
- 4: harden: scope release token out of npm test askalf/hands#129
- 5: https://cheatsheetseries.owasp.org/cheatsheets/GitHub_Actions_Security_Cheat_Sheet.html
- 6: https://github.com/github/docs/blob/main/content/actions/reference/security/secure-use.md
- 7: https://github.com/jordanconway/package-manager-hardening/blob/main/docs/github-actions.md
- 8: https://docs.github.com/actions/reference/authentication-in-a-workflow
Set persist-credentials: false and add explicit job permissions.
The ui job executes npm ci, which runs dependency scripts (preinstall, postinstall) that can access persisted Git credentials. The default actions/checkout behavior stores the GITHUB_TOKEN in the runner environment. A compromised or malicious dependency can read this token and perform unauthorized actions (code push, release modification, workflow dispatch) within the scope of the job's permissions. Disable credential persistence and restrict the job to the minimum necessary permissions.
Add permissions: contents: read to the ui job and set persist-credentials: false on the checkout step.
Proposed workflow hardening
ui:
name: UI tests and committed bundle
runs-on: ubuntu-latest
timeout-minutes: 20
+ permissions:
+ contents: read
defaults:
run:
working-directory: web
steps:
- uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1
+ with:
+ persist-credentials: false🧰 Tools
🪛 zizmor (1.29.0)
[warning] 57-57: credential persistence through GitHub Actions artifacts (artipacked): does not set persist-credentials: false
(artipacked)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/workflows/ci.yml around lines 57 - 64, Harden the ui job by adding
job-level permissions limited to contents: read, and update its actions/checkout
step to set persist-credentials: false. Keep the existing checkout revision and
setup-node/install flow unchanged.
Source: Linters/SAST tools
| permissions: | ||
| # Only the release step needs write, and only on a tag. | ||
| contents: write |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- workflow ---'
cat -n .github/workflows/release.yml
printf '%s\n' '--- related workflow references ---'
rg -n -C 3 'release\.yml|workflow_dispatch|contents:\s*(read|write)|gh release|upload-artifact|download-artifact|if:' .github/workflows .github 2>/dev/null || trueRepository: StructuPath/jig
Length of output: 8065
Limit contents: write to a tag-only publish job.
workflow_dispatch runs the build job with contents: write, even though the publish step does not run. Keep the build job read-only. Move publication to a separate job that runs only for refs/tags/v*, downloads the artifact, and receives contents: write.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/workflows/release.yml around lines 22 - 24, Separate the build and
publication workflows so the build job uses read-only contents permissions,
while a dedicated publish job runs only when the ref matches refs/tags/v*. Have
the publish job depend on the build, download its artifact, and assign contents:
write only within that job.
Source: Linters/SAST tools
| // anyClosed merges cancellation channels: the result closes as soon as any | ||
| // input does. It never blocks the caller and never leaks past the first | ||
| // close, which is all the engine's one-shot cancellation signal needs. | ||
| func anyClosed(channels ...<-chan struct{}) <-chan struct{} { | ||
| merged := make(chan struct{}) | ||
| var once sync.Once | ||
| for _, channel := range channels { | ||
| if channel == nil { | ||
| continue | ||
| } | ||
| go func(c <-chan struct{}) { | ||
| select { | ||
| case <-c: | ||
| once.Do(func() { close(merged) }) | ||
| case <-merged: | ||
| } | ||
| }(channel) | ||
| } | ||
| return merged | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
anyClosed leaks two goroutines per attempt on the normal path.
Each spawned goroutine blocks until its input channel closes or merged closes. On a successful attempt neither event occurs: the operator does not interrupt, and the control plane does not cancel. merged is therefore never closed, and both goroutines stay parked forever. A long-lived worker accumulates two leaked goroutines per completed attempt, each retaining the merged channel and the prepared cancellation channel.
Give the caller a way to release the merge when the attempt ends.
🐛 Add a release function and call it when the attempt returns
-func anyClosed(channels ...<-chan struct{}) <-chan struct{} {
+// anyClosed merges cancellation channels. The returned stop function releases
+// the watcher goroutines when the attempt ends without cancellation.
+func anyClosed(channels ...<-chan struct{}) (<-chan struct{}, func()) {
merged := make(chan struct{})
var once sync.Once
+ closeMerged := func() { once.Do(func() { close(merged) }) }
for _, channel := range channels {
if channel == nil {
continue
}
go func(c <-chan struct{}) {
select {
case <-c:
- once.Do(func() { close(merged) })
+ closeMerged()
case <-merged:
}
}(channel)
}
- return merged
+ return merged, closeMerged
}At the call site:
+ cancelled, releaseCancellation := anyClosed(signalCtx.Done(), prepared.Cancelled())
+ defer releaseCancellation()
return runner.Execute(context.WithoutCancel(ctx), engine.Attempt{
Claim: prepared.Claim,
WorktreePath: prepared.WorktreePath,
Branch: prepared.Branch,
BaseSHA: prepared.BaseSHA,
- Cancelled: anyClosed(signalCtx.Done(), prepared.Cancelled()),
+ Cancelled: cancelled,
FreshenLease: prepared.FreshenLease,
})Register defer releaseCancellation() before the return statement so it runs after Execute returns.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@cmd/jig/worker.go` around lines 319 - 338, Update anyClosed to return both
the merged cancellation channel and a release function that closes merged
through the existing sync.Once, allowing waiting goroutines to exit when the
attempt completes. At the call site that invokes anyClosed for Execute, capture
the release function and register defer releaseCancellation() before returning,
ensuring it runs after Execute returns.
| func (s *TraceStream) sendLoop(ctx context.Context) { | ||
| defer close(s.done) | ||
| backoff := traceRetryFloor | ||
| timer := time.NewTimer(s.flushEvery) | ||
| defer timer.Stop() | ||
| for { | ||
| select { | ||
| case <-ctx.Done(): | ||
| return | ||
| case <-s.wake: | ||
| case <-timer.C: | ||
| } | ||
| delivered, err := s.sendOnce(ctx) | ||
| switch { | ||
| case err != nil: | ||
| resetTraceTimer(timer, backoff) | ||
| backoff = min(backoff*traceRetryBackoff, traceRetryCeiling) | ||
| case delivered > 0: | ||
| // More may already be waiting: come straight back. | ||
| backoff = traceRetryFloor | ||
| resetTraceTimer(timer, 0) | ||
| default: | ||
| backoff = traceRetryFloor | ||
| resetTraceTimer(timer, s.flushEvery) | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Backoff is bypassed by nudge, so a failing ingester is retried per event.
Emit calls nudge for every event. The loop then wakes on s.wake and calls sendOnce immediately, so backoff never delays the next attempt while events keep arriving. During an ingest outage of a chatty phase the stream retries at event rate against a control plane that is already failing. Track a retry deadline and reschedule the timer instead of sending before it passes.
🔁 Proposed fix to honour the backoff deadline
func (s *TraceStream) sendLoop(ctx context.Context) {
defer close(s.done)
backoff := traceRetryFloor
+ var retryAt time.Time
timer := time.NewTimer(s.flushEvery)
defer timer.Stop()
for {
select {
case <-ctx.Done():
return
case <-s.wake:
case <-timer.C:
}
+ // A wake during backoff must not shorten it: reschedule instead.
+ if wait := time.Until(retryAt); wait > 0 {
+ resetTraceTimer(timer, wait)
+ continue
+ }
delivered, err := s.sendOnce(ctx)
switch {
case err != nil:
+ retryAt = time.Now().Add(backoff)
resetTraceTimer(timer, backoff)
backoff = min(backoff*traceRetryBackoff, traceRetryCeiling)
case delivered > 0:
// More may already be waiting: come straight back.
backoff = traceRetryFloor
+ retryAt = time.Time{}
resetTraceTimer(timer, 0)
default:
backoff = traceRetryFloor
+ retryAt = time.Time{}
resetTraceTimer(timer, s.flushEvery)
}
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func (s *TraceStream) sendLoop(ctx context.Context) { | |
| defer close(s.done) | |
| backoff := traceRetryFloor | |
| timer := time.NewTimer(s.flushEvery) | |
| defer timer.Stop() | |
| for { | |
| select { | |
| case <-ctx.Done(): | |
| return | |
| case <-s.wake: | |
| case <-timer.C: | |
| } | |
| delivered, err := s.sendOnce(ctx) | |
| switch { | |
| case err != nil: | |
| resetTraceTimer(timer, backoff) | |
| backoff = min(backoff*traceRetryBackoff, traceRetryCeiling) | |
| case delivered > 0: | |
| // More may already be waiting: come straight back. | |
| backoff = traceRetryFloor | |
| resetTraceTimer(timer, 0) | |
| default: | |
| backoff = traceRetryFloor | |
| resetTraceTimer(timer, s.flushEvery) | |
| } | |
| } | |
| } | |
| func (s *TraceStream) sendLoop(ctx context.Context) { | |
| defer close(s.done) | |
| backoff := traceRetryFloor | |
| var retryAt time.Time | |
| timer := time.NewTimer(s.flushEvery) | |
| defer timer.Stop() | |
| for { | |
| select { | |
| case <-ctx.Done(): | |
| return | |
| case <-s.wake: | |
| case <-timer.C: | |
| } | |
| // A wake during backoff must not shorten it: reschedule instead. | |
| if wait := time.Until(retryAt); wait > 0 { | |
| resetTraceTimer(timer, wait) | |
| continue | |
| } | |
| delivered, err := s.sendOnce(ctx) | |
| switch { | |
| case err != nil: | |
| retryAt = time.Now().Add(backoff) | |
| resetTraceTimer(timer, backoff) | |
| backoff = min(backoff*traceRetryBackoff, traceRetryCeiling) | |
| case delivered > 0: | |
| // More may already be waiting: come straight back. | |
| backoff = traceRetryFloor | |
| retryAt = time.Time{} | |
| resetTraceTimer(timer, 0) | |
| default: | |
| backoff = traceRetryFloor | |
| retryAt = time.Time{} | |
| resetTraceTimer(timer, s.flushEvery) | |
| } | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/worker/events.go` around lines 480 - 506, Update
TraceStream.sendLoop to track a retry deadline whenever sendOnce fails, and
ensure wake events do not trigger another send until that deadline has passed.
When nudge wakes the loop during backoff, reschedule or wait on the timer rather
than calling sendOnce; retain immediate wake-driven sends when no retry backoff
is active and preserve the existing exponential backoff limits.
| _, err := s.ingest.IngestEvents(ctx, s.attemptID, protocol.EventBatch{ | ||
| LeaseToken: s.leaseToken, | ||
| Events: batch, | ||
| }) | ||
|
|
||
| s.mutex.Lock() | ||
| defer s.mutex.Unlock() | ||
| s.sending = false | ||
| if err != nil { | ||
| if ctx.Err() == nil { | ||
| s.logger.Warn("trace_ingest_failed", "attempt_id", s.attemptID, | ||
| "events", count, "error", err) | ||
| } | ||
| // Back at the head, where they were: the batch is the oldest run, so | ||
| // restoring it in front of whatever arrived meanwhile keeps the buffer | ||
| // in seq order. | ||
| s.pending = append(batch, s.pending...) | ||
| return 0, err | ||
| } | ||
| return count, nil |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Inspect how the worker HTTP client surfaces control-plane service errors, to see whether permanent rejections are distinguishable.
set -euo pipefail
fd -t f -e go . internal/worker | xargs -r rg -n -C 6 'func \(c \*client\) call|ServiceError|StatusCode|http\.Status'
rg -n -C 4 'func invalid\(|func conflict\(|func unavailable\(' internal/controlplaneRepository: StructuPath/jig
Length of output: 7736
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- client error decoding and IngestEvents call ---'
ast-grep outline internal/worker/client.go
rg -n -C 12 'decodeAPIError|IngestEvents|APIError|sendOnce|dropOldest|gap' internal/worker internal/controlplane
printf '%s\n' '--- relevant source sections ---'
sed -n '1,90p' internal/worker/client.go
sed -n '124,180p' internal/worker/client.go
sed -n '500,575p' internal/worker/events.go
sed -n '1,110p' internal/controlplane/ingest.goRepository: StructuPath/jig
Length of output: 50371
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- ingest validation and lease ownership ---'
sed -n '53,125p' internal/controlplane/ingest.go
rg -n -C 10 'lease_not_owner|lease_superseded|validateLeaseToken|Owner|token|LeaseToken' internal/controlplane internal/worker/claiming.go
printf '%s\n' '--- sendOnce, Flush, and close behavior ---'
sed -n '521,610p' internal/worker/events.go
sed -n '610,655p' internal/worker/events.go
printf '%s\n' '--- error status mapping ---'
rg -n -C 8 'func writeError|ServiceError|Status' internal/controlplaneRepository: StructuPath/jig
Length of output: 50372
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '53,115p' internal/controlplane/ingest.go
sed -n '521,605p' internal/worker/events.go
printf '%s\n' '--- lease checks used by ingestion ---'
rg -n -C 5 'func \(s \*Store\) IngestEvents|verify.*Lease|lease_not_owner|isLeasedState' internal/controlplane/ingest.go internal/controlplane/store.go
printf '%s\n' '--- error response writer ---'
rg -n -C 12 'func writeError|func \(a \*API\).*write|ServiceError' internal/controlplane --glob '*.go' | head -n 180Repository: StructuPath/jig
Length of output: 25701
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- lease loss mutations ---'
sed -n '680,730p' internal/controlplane/embedded.go
rg -n -C 12 'UPDATE attempts SET state = .lost|AttemptLost|lose.*Attempt|lease_digest = NULL' internal/controlplane --glob '*.go' | head -n 220
printf '%s\n' '--- event creation and validation boundaries ---'
rg -n -C 8 'protocol.Event\{|EventError|invalid_event_type|Seq:' internal/worker/events.go internal/worker --glob '*.go' | head -n 260Repository: StructuPath/jig
Length of output: 23906
🏁 Script executed:
#!/bin/bash
set -euo pipefail
ast-grep outline internal/worker/events.go
sed -n '250,395p' internal/worker/events.go
rg -n -C 5 'TraceStream|NewTrace|Emit|eventType|EventType|NextSeq|seq' internal/worker --glob '*.go' | head -n 260Repository: StructuPath/jig
Length of output: 25565
Stop replaying permanent ingest rejections.
TraceStream.Emit accepts events without validating Seq or Type. sendOnce restores every rejected batch at the head. A batch with invalid_event_seq or invalid_event_type can therefore block Flush and later events until Close times out. Classify these errors, and lease_not_owner for the current token, as permanent. Remove the rejected batch and record a valid gap marker with the reason. Retry transport and storage failures only. batch_too_large cannot occur here because sendOnce caps each batch, and lease loss alone does not produce lease_not_owner because ingestion accepts the stale digest for a terminal lost attempt.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/worker/events.go` around lines 544 - 563, Update sendOnce’s
ingest-error handling to classify invalid_event_seq, invalid_event_type, and
lease_not_owner for the current token as permanent rejections: remove the
rejected batch, record a valid gap marker with the appropriate reason, and do
not restore it to s.pending. Continue prepending the batch and retrying for
transport and storage failures, while preserving the existing warning and return
behavior.
| if (arrived.length === 0) { | ||
| idleRef.current += 1; | ||
| // A finished attempt whose tail has stopped arriving needs no more | ||
| // polling; a running one keeps its interval. | ||
| if (terminal && idleRef.current >= 2 && timer) { | ||
| clearInterval(timer); | ||
| timer = null; | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
# Inspect whether attempt completion is ordered after trace ingestion and
# whether the HTTP event response exposes a completion marker.
ast-grep outline internal/controlplane/ingest.go --items all
ast-grep outline internal/controlplane/http.go --items all
rg -n -C 5 'CompleteAttempt|AttemptEventPage|next_cursor|trace.*complete|finalized|ingest' \
internal/controlplane internal/workerRepository: StructuPath/jig
Length of output: 50371
🏁 Script executed:
# Check protocol definitions for event/attempt structures
fd -e go -path '*/protocol/*' | head -20
# Search for completion-related timing or windows
rg -n 'CompleteAttempt|LeaseDuration|eventWindow|flushInterval|inFlight' \
--type go internal/worker internal/protocol -A 3 -B 1 | head -100
# Check worker event buffer and flush behavior on completion
rg -n -A 10 'func.*complete|events.*flush|drainTrace' internal/worker/Repository: StructuPath/jig
Length of output: 5616
🏁 Script executed:
# Check protocol definitions for EventPage and Attempt structures
cat internal/protocol/types.go | grep -A 20 'type EventPage\|type Attempt struct'
# Search for event buffer draining on completion
rg -n 'func.*drain|stopDrain|close.*trace|eventBuffer|flushEvents' \
internal/worker/ -A 5
# Check if there's a window/timeout for events after completion
rg -n 'after.*complet|terminal.*event|drainTrace|sendBatch|flush' \
internal/worker/events.go -A 3 -B 3 | head -80Repository: StructuPath/jig
Length of output: 2755
🏁 Script executed:
# Check the web polling logic more carefully
cat web/src/polling.ts | head -160 | tail -50
# Also check if there's any mechanism that stops accepting events after completion
rg -n 'AttemptTerminal|AttemptFinal|State.*terminal|completeAttempt' \
web/src/ -A 2 -B 2
# Check attempt states to see if terminal prevents event ingestion
rg -n 'AttemptAccepted|AttemptFailed|terminal' internal/protocol/ -B 2 -A 2 | head -60Repository: StructuPath/jig
Length of output: 4284
🏁 Script executed:
# Verify the IngestEvents route accepts events even after terminal state
rg -n -B 5 -A 15 'func.*IngestEvents' internal/controlplane/ingest.go | head -40
# Check if there's any lease validation that would prevent events after completion
rg -n 'token.*valid|lease.*check|terminal.*event' internal/controlplane/ -A 3 | head -50
# Verify event flush behavior on worker completion (grace period)
rg -n -B 3 -A 10 'Close|Flush.*context|grace|drainCtx' internal/worker/events.goRepository: StructuPath/jig
Length of output: 6102
Keep polling until the trace is known complete, or expose an explicit completion signal.
Line 145 stops polling after two empty pages. The control plane accepts events indefinitely after terminal completion (see ingest.go lines 48–52); a worker's 5-second grace window (traceCloseGrace) for event flush can deliver events after the UI stops polling. The EventPage protocol has no completion marker. Delayed events then arrive after the UI stops polling and are never rendered, silently omitting trace tail, gates, or errors.
Either expose an explicit finalized signal from the control plane in the event API response, or extend polling until the control plane guarantees no further ingestion will occur.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@web/src/polling.ts` around lines 141 - 148, The polling stop condition in the
arrived-empty branch must not terminate after two empty pages based only on
terminal status. Update the polling flow around arrived, terminal, and idleRef
to continue until the event API exposes an explicit finalized/completion signal,
or until the control plane guarantees ingestion has ended; use that signal as
the sole condition for clearing timer.
| "references": [ | ||
| { "path": "./tsconfig.app.json" }, | ||
| { "path": "./tsconfig.node.json" } | ||
| ] |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Confirm every project reference targets a composite project.
jq '.references' web/tsconfig.json
jq '.compilerOptions.composite // false' web/tsconfig.app.json
jq '.compilerOptions.composite // false' web/tsconfig.node.jsonRepository: StructuPath/jig
Length of output: 255
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- web TypeScript configuration files ---'
for f in web/tsconfig.json web/tsconfig.app.json web/tsconfig.node.json; do
if [ -f "$f" ]; then
echo "--- $f ---"
cat -n "$f"
fi
done
printf '%s\n' '--- TypeScript project-reference and build commands ---'
rg -n --glob '!*node_modules*' --glob '!dist/**' \
'"references"|"composite"|tsc(\\.exe)?[[:space:]]+-b|vue-tsc|typescript' \
web package.json 2>/dev/null || true
printf '%s\n' '--- Resolved composite values for referenced projects ---'
python3 - <<'PY'
import json
from pathlib import Path
def load(path):
return json.loads(Path(path).read_text())
def resolve_compiler_options(path, seen=None):
path = Path(path)
seen = set() if seen is None else seen
if path in seen:
raise RuntimeError(f"extends cycle at {path}")
seen.add(path)
data = load(path)
opts = {}
ext = data.get("extends")
if ext:
base = Path(ext)
if not base.is_absolute():
base = path.parent / base
if base.suffix == "":
base = base.with_suffix(".json")
opts.update(resolve_compiler_options(base, seen))
opts.update(data.get("compilerOptions", {}))
return opts
root = load("web/tsconfig.json")
for ref in root.get("references", []):
path = Path("web") / ref["path"]
if path.suffix == "":
path = path.with_suffix(".json")
opts = resolve_compiler_options(path)
print(f"{path}: composite={opts.get('composite', False)!r}")
PYRepository: StructuPath/jig
Length of output: 2466
🌐 Web query:
TypeScript project references composite noEmit referenced project tsc -b TS6310
💡 Result:
The TypeScript error TS6310, "Referenced project may not disable emit," occurs when a project references another project that has "noEmit": true enabled in its configuration while also being marked as "composite": true [1][2][3]. TypeScript project references require referenced projects to be "composite" to ensure that the compiler can reliably determine where the outputs (such as.d.ts declaration files) of those dependencies are located [4][5]. Because composite projects are designed to be built incrementally, they must be capable of producing output files [4][5]. Consequently, TypeScript explicitly forbids the combination of "composite": true and "noEmit": true [6][7]. To resolve this error, you generally have a few options depending on your specific requirements: 1. Disable "noEmit" (Recommended for Composite Projects): If the referenced project must be composite, you should set "noEmit": false in its tsconfig.json [8][2]. If you only want to generate declaration files without emitting full JavaScript code, use "emitDeclarationOnly": true instead of "noEmit": true [1][7][9]. 2. Remove "composite" (If the Project is Not Used as a Dependency): If the project does not actually need to be referenced by other projects as a composite module, you can remove "composite": true [10][11]. However, if other projects still need to reference it, this will likely cause further errors, as they require the composite flag to manage build outputs correctly [4][11]. 3. Adjust Configuration for Type Checking Only: If you are trying to run a type-check command on a composite project and encounter this error, ensure that your build configuration does not propagate "noEmit": true to your library packages [12]. Some developers use separate configuration files (e.g., tsconfig.json for building and a separate file for type-checking) to manage these conflicting requirements [8][2]. In summary, the TS6310 error is a consistency check to ensure that referenced composite projects are actually capable of emitting the files that parent projects rely on for correct compilation [4][10]. Enabling "noEmit" breaks this contract, triggering the error [1][12].
Citations:
- 1: https://stackoverflow.com/questions/71704754/typescript-yarn-workspaces-referenced-project-may-not-disable-emit
- 2: Typescript compiler reports errors in react-native project nrwl/nx#12417
- 3: Repair TS6310 workspace project-reference failures so package typecheck gates are usable again oscharko-dev/Keiko#935
- 4: https://www.typescriptlang.org/docs/handbook/project-references.html
- 5: https://www.typescriptlang.org/tsconfig/composite.html
- 6: tsc: TS5053: Option 'noEmit' cannot be specified with option 'composite' microsoft/TypeScript#36917
- 7: Allow noEmit and composite together in 3.7 microsoft/TypeScript#33809
- 8: js emit is not supported vuejs/language-tools#3889
- 9: https://tessl.io/registry/skills/github/nrwl/nx/nx-import
- 10: Missing error when referenced project does not have required
compositeflag microsoft/TypeScript#40766 - 11: "Referenced projects must have the new composite setting enabled" does not hold true microsoft/TypeScript#60465
- 12: Codeneos/vlocode@34431fb
Configure referenced projects for tsc -b.
Neither referenced project is composite, so tsc -b reports TS6306. Adding composite: true alone is insufficient because both projects also set noEmit: true, which causes TS6310. Use build configs that enable composite output, and retain separate no-emit configs for type checking.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@web/tsconfig.json` around lines 3 - 6, Update the referenced projects in
tsconfig.app.json and tsconfig.node.json to use build-compatible configurations
with composite enabled and emit output enabled, then retain separate no-emit
configurations for regular type checking. Ensure the references in tsconfig.json
target the build configs so tsc -b no longer reports TS6306 or TS6310.
ListenConfig.Listen consults the context only where it resolves a name, so an address needing no resolution binds happily on a cancelled context — and which addresses need resolution differs by platform and resolver. Check the context explicitly before the bind. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The scan's repository set comes from attempt manifests, which outlive the repository they name. A remote that is deleted or renamed therefore stays in the set forever, and every worker start reported the same permanently incomplete scan — noise that buries the strays an operator can still act on. Classify a gone remote and skip it, logged at info rather than counted as a scan failure. The classifier stays narrow on purpose: timeouts, DNS failures, and refused credential prompts mean "ask again later" and must keep their place in the scan errors, because silently skipping a live repository is how a real stray branch goes unreported. A vanished file:// remote arrives typed as ENOENT and is matched that way; a missing git binary is not, since exec reports it as ErrNotFound. The one ambiguity left is a private repository whose credentials no longer reach it — GitHub answers not-found rather than forbidden so as not to leak existence — which is why the skip is logged, not swallowed. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
drainStream waited only on the stdout capture before closing both read ends, but closing a read end is what ends its capture. When stdout finished first the stderr copy was cut off with the CLI's diagnostic still sitting unread in the pipe, and a nonzero exit reached the trace as "codex exited: exit status 3:" with no cause after the colon — the one thing that path exists to carry. It is timing-dependent, which is why it passed locally and failed on loaded CI runners: the capture goroutine is normally already blocked in read() and wakes with the data long before drainStream runs. This is what has been failing CI on both platforms since 635f88b. Wait on both captures under one shared deadline, so the grace still bounds the whole drain rather than each stream separately. Both adapters carried the identical bug; both are fixed, and both get a regression test that gates the capture's first read to make the losing order deterministic (each fails on the old code with an empty tail). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/runtime/codex/adapter_test.go`:
- Around line 799-802: Replace the fixed 50 ms sleep in the test with an
observable synchronization hook or close observer tied to drainStream’s
stderrReader closure. Wait for that signal before calling close(gate), ensuring
the test deterministically verifies the closure ordering and cannot pass due to
timing variance.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: b5467fe6-983b-40a8-b1bf-f55705915626
📒 Files selected for processing (7)
internal/controlplane/server.gointernal/runtime/claudecode/adapter.gointernal/runtime/claudecode/adapter_test.gointernal/runtime/codex/adapter.gointernal/runtime/codex/adapter_test.gointernal/worker/publish.gointernal/worker/publish_test.go
🚧 Files skipped from review as they are similar to previous changes (5)
- internal/runtime/claudecode/adapter.go
- internal/runtime/claudecode/adapter_test.go
- internal/worker/publish.go
- internal/worker/publish_test.go
- internal/runtime/codex/adapter.go
| // Long enough that a drainStream which closes on stdout alone has already | ||
| // done so; the release then finds a dead descriptor instead of the tail. | ||
| time.Sleep(50 * time.Millisecond) | ||
| close(gate) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Replace the fixed delay with an observable synchronization point.
The 50 ms delay does not prove that drainStream closed stderrReader before the test releases gate. On a slow runner, the old faulty implementation can start later, capture "codex fell over", and pass this regression test.
Add a test hook or close observer. Wait for that observer before releasing gate.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/runtime/codex/adapter_test.go` around lines 799 - 802, Replace the
fixed 50 ms sleep in the test with an observable synchronization hook or close
observer tied to drainStream’s stderrReader closure. Wait for that signal before
calling close(gate), ensuring the test deterministically verifies the closure
ordering and cannot pass due to timing variance.
1 participant
Milestone 2 — definitions in, changes out
Builds on Milestone 1's core loop. A definition can now be authored through the API, invoked manually or by a trigger, fanned out across repositories, and the accepted work published as a real branch and pull request.
Plan:
docs/plans/2026-08-05-001-feat-jig-software-factory-plan.mdIncludes the three commits from PR #3 (truncation guard, adapter drain bound, fail-closed path resolution) since that PR is still open and this branch descends from it. Merging this supersedes #3.
Milestone 2 exit gate — passed on real GitHub
An accepted run through the worker path published a real pull request on a scratch repo: attempt-scoped branch
jig/<job>/1, PR open againstmain, containing exactly one file — the changed-paths staging discipline holding against a real remote rather than only a local bare repo. The worktree was deleted only after remote-ref proof.The gate also found a bug the faked gateway could not:
gh repo viewtakes its project positionally and has no--repoflag.What landed
re-admit at headas the explicit escape hatchDecisions worth reviewing
Untrusted context is fenced, not just bounded. The M1 security review flagged "bounded untrusted context" as underspecified. Composition now states the untrusted rule before any untrusted byte, fences each section with explicit markers, and neutralizes forged markers inside the body while preserving content so an agent can report an injection attempt. Oversized sections truncate; dropped sections are declared in the prompt. Tests cover a section trying to close its own fence and a label trying to smuggle markup.
Codex reports cumulative usage, so the adapter reports deltas. Measured across four real turns (20,180 → 40,575 → 61k → 81,603 input tokens),
turn.completed.usageis cumulative per thread. Reporting it raw would re-count earlier turns on every correction — inflating spend accounting on exactly the repair loops that matter. Context occupancy is absent from the exec stream entirely and stays zero rather than borrowing the cumulative figure.A Codex-rostered role declaring a tool allowlist fails the send.
codex exechas no tool-allowlist flag and approvals are bypassed, so silently dropping the allowlist would hand the agent wider reach than the definition asked for. Failing loudly is the honest option; definition validation should warn at save time.Run aggregation has one rule now. The direct-run path previously mapped
accepted_unpublished → acceptedfor single-job runs while the plan's R12 said otherwise. Reconciled to R12 read literally: anaccepted_unpublishedjob aggregates tomixed.jig run's exit code is unaffected — it reports the attempt state.R12's third conjunct is enforced server-side.
CompleteAttemptpreviously acceptedstate: acceptedwithout checking for a publish-proof record, so "published" was a worker-side convention. It is now checked inside the completion transaction, after the lease fence.The fence cannot fence GitHub. A zombie whose lease expires mid-publish can still complete a push. Damage is confined by attempt-scoped branch naming and surfaced by a stray-branch report at reconcile time — reported, never deleted. This is stated rather than papered over.
Testing
just checkandjust test-racegreen. Real SQLite, real git fixture repos, real HTTP servers;ghis faked behind an interface in unit tests and exercised for real only in the exit gate. Codex's live smoke ran against the real CLI (codex-cli 0.146.0, authenticated) and confirmed live-session repair: a forced parse-correction resumed the same thread and still knew the filename.Known gaps
jig serveandjig workerdo not exist yet (U9), so triggers and the publish pipeline have no runtime host. Two one-line wirings are documented in the code: wrap the engine runner withNewPublishingRunner, and callRecoverbefore serving so interrupted occurrences do not stick indispatching.jig runverification is outstanding until a roster can namecodex.CODEX_HOMEauth must be seeded into the ephemeral HOME or Codex cannot authenticate under KTD11.phase.gosplit, cross-package helper dedup,git diff HEADon an empty repo, stat-based fingerprints for wholly-ignored directories, trace redaction (U8).Post-Deploy Monitoring & Validation
No production or runtime impact — local-first developer tool, no deployed surface. Validation is CI on both platforms, the exit gate above, and the Codex live smoke.
🤖 Generated with Claude Code
Summary by CodeRabbit