feat: jig Milestone 1 — control plane, worker, phase engine, and direct run harness - #1
Conversation
Idempotent claim transaction with retained-cap skip-over and env-name eligibility, sleep-benign lease renewal of expired-unswept leases, uptime-gated sweep, cold retry at pinned SHA, loopback-only server with Origin fencing. Race suite covers claim/heartbeat/sweep interleavings.
Single implicit worker: probed registration with env-name advertisement, wake-safe claim/heartbeat loop, bounded managed-identity repo cache with origin revalidation, worktree-per-attempt at the pinned SHA, fail-closed manifests and bidirectional reconcile, and the control-plane worktree ledger with confirmed release.
…dapter (U4) Chain executor with if: guards and repair edges, per-emission parse budgets nested in gate corrections, five built-in claim-verifier gates, write-boundary fingerprint enforcement, ephemeral agent HOME, acceptance predicate with evidence, JSONL event sink with per-attempt seq. Real-CLI live smoke passes (env-guarded).
Serverless direct-run path through the same store and fenced transitions, validation-before-spawn, darwin keychain HomeSeeder, smoke and two-phase example definitions. Milestone 1 exit gate: three consecutive accepted real-CLI runs including forced parse and gate corrections.
… review
Seven-persona review of the M1 branch surfaced two reproduced failures
and a confirmed containment bypass. Fixes, each with a regression test
verified to fail beforehand:
- tests_pass gate ran with jig's full environment and the operator's
real HOME, so gate commands executing agent-authored code reached
every credential. Gates now share the composed role env; a nil env
is an error rather than silent inheritance.
- Renames fingerprinted as '{a => b}/f' tokens, so rollback failed and
a rename could cross the write allowlist undetected. Enumeration is
now literal NUL-separated paths with --no-renames.
- Fingerprints were name/count based, blind to content edits, .git
metadata, and gitignored paths; a planted hook fired during jig's
own rollback. Content hashing plus .git/hook fingerprinting, and
jig-side git runs with hooks and fsmonitor disabled.
- Interrupting jig run wedged the data dir permanently and orphaned a
bypassPermissions agent. Signal handling, terminal-state recording,
scratch destruction, liveness-marker reclaim, and a dead-man's
switch on the process-group anchor.
- Host authority is validated before the Origin check, closing DNS
rebinding against the unauthenticated loopback API.
- Stale process groups are reaped on worker start, identity-gated so a
recycled pid is never signalled.
- Send ladder is bounded by an enforced per-attempt counter; result
JSON degrades validly instead of byte-truncating into invalid JSON;
runtime is_error ends a phase instead of burning the retry ladder.
- jig run gains --json and distinct exit codes; race suite runs in CI;
gate registry gains an AST lockstep test.
📝 WalkthroughWalkthroughThe change adds a direct ChangesExecution platform
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant Jig
participant ControlPlane
participant Worker
participant Engine
participant Runtime
User->>Jig: invoke jig run
Jig->>ControlPlane: create and claim pinned job
ControlPlane->>Worker: assign attempt lease
Worker->>Engine: execute prepared attempt
Engine->>Runtime: send phase prompts
Runtime-->>Engine: stream events and terminal result
Engine->>ControlPlane: persist events and outcome
ControlPlane-->>Jig: return terminal run result
Jig-->>User: print report and trace path
🚥 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: 13
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
🟡 Minor comments (13)
internal/runtime/claudecode/adapter_test.go-183-190 (1)
183-190: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winThe
--resumeadjacency assertion is dead.The condition is
!Contains("--resume\n"+id) && !Contains("--resume"). If the first operand is true because the pairing is wrong, the second operand still evaluates. Any occurrence of--resumemakes the second operand false, so the whole condition is false. The stricter pairing check never fires, and only the presence of the flag is verified.The stub writes one argument per line, so the paired form is the correct assertion.
🐛 Proposed fix
arguments, _ = os.ReadFile(filepath.Join(stubDir, "args")) - if !strings.Contains(string(arguments), "--resume\n"+session.NativeID) && - !strings.Contains(string(arguments), "--resume") { - t.Errorf("second-send args missing --resume:\n%s", arguments) + if !strings.Contains(string(arguments), "--resume\n"+session.NativeID+"\n") { + t.Errorf("second-send args missing --resume paired with %q:\n%s", + session.NativeID, arguments) }🤖 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_test.go` around lines 183 - 190, Fix the `--resume` assertion in the second-send verification so it requires the exact adjacent argument pair `--resume` followed by `session.NativeID`, rather than allowing any standalone `--resume` occurrence. Keep the existing `--session-id` rejection unchanged.internal/worker/worker_integration_test.go-215-229 (1)
215-229: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
gitRuncallst.Fatalffrom a non-test goroutine.The runner at Line 215 executes inside the goroutine started at Line 238.
gitRunat Line 224 callst.Fatalfon failure.t.Fatalfoutside the test goroutine callsruntime.Goexiton that goroutine only. The test goroutine keeps waiting ingroup.Wait(), and the failure is not reported at the point of occurrence. The surrounding code already usest.Errorat Line 222 for that reason.Return the git error to the test goroutine instead, or add a
gitOutputhelper that reports the error rather than callingt.Fatalf.🤖 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/worker_integration_test.go` around lines 215 - 229, Update the RunnerFunc callback and its git HEAD lookup so failures from the goroutine do not call gitRun’s t.Fatalf. Return or propagate the git command error through the runner’s Outcome, then have the test goroutine report it via t.Error or the existing failure path while preserving the observation logic for successful commands.internal/controlplane/sweep.go-95-126 (1)
95-126: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winCheck
rows.Err()after the scan loop.
rows.Next()also returns false when the driver stops iteration because of an error. The current code cannot distinguish that case from a normal end of results. A truncated result set produces a partialleasedmap, so the loop at Line 122 deletes the miss counters of attempts that were never read, andoverduesilently omits expired attempts.rows.Close()does not report iteration errors.🐛 Proposed fix
} + if err := rows.Err(); err != nil { + rows.Close() + return nil, unavailable(err) + } if err := rows.Close(); err != nil { return nil, 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/sweep.go` around lines 95 - 126, After the rows.Next scan loop in the sweep query, check rows.Err() before processing sw.misses or returning results; on iteration error, return unavailable(err) and ensure rows is closed consistently. Keep the existing scan-error and normal-close handling unchanged, while preventing partial leased and overdue data from being used.Source: Linters/SAST tools
cmd/jig/run.go-343-362 (1)
343-362: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winBind
repoHeadto the command context.
repoHeadcallsexec.Command, so the git invocation has no deadline and no cancellation. Ifgit rev-parseblocks — for example on a network-backed filesystem or an interactive credential prompt —jig runhangs before the signal-aware path starts.golangci-lintnoctxalso flags Line 346.Pass the context that
runCommandalready holds and reuseprotocol.GitCommandTimeout, which the engine'srunGituses for the same purpose.🐛 Proposed fix
-func repoHead(repoPath string) (string, error) { - command := exec.Command("git", "rev-parse", "HEAD") +func repoHead(ctx context.Context, repoPath string) (string, error) { + ctx, cancel := context.WithTimeout(ctx, protocol.GitCommandTimeout) + defer cancel() + command := exec.CommandContext(ctx, "git", "rev-parse", "HEAD") command.Dir = repoPathThe caller at Line 111 becomes
headSHA, err := repoHead(ctx, repoPath). Move theif ctx == nilguard at Lines 117-119 above that call.🤖 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 343 - 362, Update repoHead to accept the existing context and create the git command with exec.CommandContext, applying protocol.GitCommandTimeout as the command deadline. Move the ctx nil guard in the caller before repoHead is invoked, then pass ctx into repoHead so cancellation and timeout apply during git rev-parse.Source: Linters/SAST tools
cmd/jig/run_test.go-436-447 (1)
436-447: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
waitForScratchcan report after the test completes and panic the test binary.
waitForScratchruns inside the goroutines at Lines 366-370 and Lines 406-409. It polls for up to 30 seconds. If the run terminates before the scratch directory appears — a definition change, an early infrastructure failure, or a fast rejection —runJigContextreturns and the test function finishes while the goroutine is still polling. The goroutine then callst.Erroron Line 446. The testing package panics with "Log in goroutine after Test... has completed", which fails the whole package, not just this test.Bound the poll with a channel the test closes on return.
♻️ Proposed fix
-func waitForScratch(t *testing.T, scratchRoot string) { +func waitForScratch(t *testing.T, done <-chan struct{}, scratchRoot string) { t.Helper() deadline := time.Now().Add(30 * time.Second) for time.Now().Before(deadline) { + select { + case <-done: + return + default: + } entries, err := os.ReadDir(scratchRoot) if err == nil && len(entries) > 0 { return } time.Sleep(10 * time.Millisecond) } t.Error("the attempt never reached execution") }Each caller creates
done := make(chan struct{}), addsdefer close(done)in the test body, and passes it in.🤖 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_test.go` around lines 436 - 447, Update waitForScratch to accept a done channel and exit immediately when it is closed, including while sleeping or before reporting failure; ensure both goroutine call sites pass the channel, and create it in the test body with a deferred close before runJigContext can return.cmd/jig/main.go-26-34 (1)
26-34: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winThe second-signal escape hatch described in the comment does not exist.
signal.NotifyContextcancels the context on the first signal. It keeps the signal handler installed untilstopis called.stopruns only through thedefer, andos.Exitskips deferred functions. A second SIGINT is therefore still trapped and discarded, so an operator who presses Ctrl-C again cannot terminatejig. If the phase engine hangs during unwinding, the process cannot be stopped without SIGKILL.Install an explicit escalation, or correct the comment.
🐛 Proposed fix: restore the default disposition after the first signal
- ctx, stop := signal.NotifyContext(context.Background(), os.Interrupt, syscall.SIGTERM) - defer stop() - os.Exit(run(ctx, os.Args[1:])) + ctx, stop := signal.NotifyContext(context.Background(), os.Interrupt, syscall.SIGTERM) + // After the first signal the default disposition is restored, so a second + // Ctrl-C always kills jig outright. + go func() { + <-ctx.Done() + stop() + }() + code := run(ctx, os.Args[1:]) + stop() + os.Exit(code)🤖 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 26 - 34, Update the signal handling around signal.NotifyContext in main so the first SIGINT/SIGTERM cancels the context and a subsequent signal restores the default disposition, allowing immediate process termination. Ensure the escalation cleanup is registered independently of the deferred stop, which os.Exit skips, and keep the documented graceful first-signal behavior intact.cmd/jig/testdata/two-phase-forced.yaml-8-13 (1)
8-13: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winThe parse trap competes with the contract text the engine appends.
composePromptininternal/engine/phase.goLines 1320-1322 appends this instruction to every user prompt: respond with a JSON object whosestatusis"success"or"fail".The model therefore receives the
"status": "done"template on Line 41 and the correct contract in the same prompt. Whether the first emission is invalid depends on which instruction the model follows.The header states the gate trap is "deterministic by construction" on Lines 21-22 and makes no equivalent claim for the parse trap, so the distinction is already understood. Record it in the header comment so a later reader does not treat a passing first emission as a regression.
Also applies to: 38-41
🤖 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/testdata/two-phase-forced.yaml` around lines 8 - 13, Update the header comments in the two-phase forced test fixture to note that the parse-correction trap is not deterministic because composePrompt appends the valid status contract alongside the invalid template, so a passing first emission is not a regression; preserve the existing explanation of the required parse-correction behavior.internal/engine/phase.go-842-849 (1)
842-849: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winThe breach diagnostic is unbounded.
Lines 842-845 join every breach path into one string. Line 846 embeds that string in
phaseRun.failure, which becomeschainEnd.diagnosticat Line 599 and thenworker.Outcome.Errorat Line 292.The engine bounds every other variable-length payload:
boundedEnvelopeon Line 365,truncateTexton Line 359, andprotocol.MaxCommandOutputTailByteson Line 1185.Outcome.Errorhas no such bound here. The breach count is whatever the agent wrote outside its allowlist, so a run that creates many stray files produces a very large error field for the control plane to persist.Cap the joined list and report the total count.
🐛 Proposed fix to bound the diagnostic
var paths []string for _, item := range breaches { + if len(paths) == 20 { + paths = append(paths, fmt.Sprintf("… and %d more", len(breaches)-20)) + break + } paths = append(paths, item.Path+" — "+item.Outcome) }🤖 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/engine/phase.go` around lines 842 - 849, Bound the breach-path portion of the diagnostic in the phase-abort flow that builds phaseRun.failure, while preserving the total breach count in the message. Reuse the existing truncation helper or established diagnostic-size limit (such as truncateText) when formatting strings.Join(paths, "; "), and ensure the resulting failure remains safe for propagation through chainEnd.diagnostic and worker.Outcome.Error.internal/engine/phase.go-273-276 (1)
273-276: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winA cancelled attempt returns no
Resultpayload.Every other terminal branch attaches
e.summaryJSON(...): Line 282 for the ceiling, Line 289 for the send budget, Line 292 for abort and failure, and Lines 302 and 306 for the acceptance verdicts. The cancellation branch on Line 275 returnsOutcomewith onlyStateandError.A cancelled attempt has usually written to the worktree, and the worktree is retained.
summaryJSONcarrieschanged_pathsand the per-phase results. Dropping it discards that record for the one case where an operator most wants to know what the agent had already done.🐛 Proposed fix to keep the record on cancellation
case endCancelled: e.emit.emit(protocol.EventLog, "", "attempt_cancelled", nil) - return worker.Outcome{State: protocol.AttemptCancelled, Error: end.diagnostic} + return worker.Outcome{State: protocol.AttemptCancelled, + Error: end.diagnostic, Result: e.summaryJSON(nil)}🤖 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/engine/phase.go` around lines 273 - 276, Update the endCancelled branch in the phase terminal-outcome handling to include the same e.summaryJSON(...) Result payload used by the other terminal branches, while preserving its AttemptCancelled state, diagnostic error, and cancellation event.internal/engine/phase.go-1299-1305 (1)
1299-1305: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winParameter substitution is order-dependent.
Lines 1303-1305 iterate
parameters, which is a map. Go randomizes map iteration order.strings.ReplaceAllis applied once per name against the text produced by the previous names.If one parameter value contains another parameter's
{{name}}placeholder, the output depends on iteration order. With{"a": "{{b}}", "b": "X"}, substitutingafirst yieldsX, and substitutingbfirst yields the literal{{b}}. The same definition and the same parameters then render two different prompts across runs.The PR describes repeatable phased workflows. Prompt rendering must be deterministic. Build the output in one pass so a substituted value is never rescanned.
🐛 Proposed fix for a single-pass, order-independent substitution
- text := template - for name, value := range parameters { - text = strings.ReplaceAll(text, "{{"+name+"}}", value) - } + pairs := make([]string, 0, len(parameters)*2) + for name, value := range parameters { + pairs = append(pairs, "{{"+name+"}}", value) + } + // One pass: a substituted value is never rescanned, so the result does + // not depend on map iteration order. + text := strings.NewReplacer(pairs...).Replace(template)
strings.NewReplacermatches at each position once and does not revisit inserted text, so the result is deterministic.🤖 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/engine/phase.go` around lines 1299 - 1305, Update composePrompt to perform parameter substitution in a single pass using a deterministic replacer such as strings.NewReplacer, ensuring inserted parameter values are never rescanned and output does not depend on map iteration order. Preserve existing placeholder syntax and replacement behavior.internal/engine/phase.go-762-766 (1)
762-766: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winCancellation does not reach the gate commands of a code phase.
Line 765 passes
ctxtorunGates. Lines 743-750 wiree.attempt.CancelledintocommandCtxfor the phase command only.
internal/engine/gates.goLine 198 runstests_passthroughrunShellCommandwithgc.ctx. When cancellation arrives through the heartbeat (R5), it closese.attempt.Cancelledbut does not cancelctx. A gate command such as thegrepincmd/jig/testdata/two-phase-forced.yamlLine 51 returns quickly, but ago testgate keeps running untile.timeouts.phaseexpires.The same gap applies to the
runGatescall on Line 973 inrunAgentPhaseAttempt.Derive one cancellable context for the whole phase and pass it to both the command and the gates.
🐛 Proposed fix to cover the gates with the same cancellation
env := e.phaseEnv(phase) // Cancellation during a code phase kills the command's process group via // context cancellation, then reports cancelled rather than a phase fail. commandCtx, stopCommand := context.WithCancel(ctx) + defer stopCommand() go func() { select { case <-e.attempt.Cancelled: stopCommand() case <-commandCtx.Done(): } }() result := runShellCommand(commandCtx, e.attempt.WorktreePath, phase.Command, env, e.timeouts.phase) - stopCommand() if e.cancelled() { return phaseRun{outcome: phaseCancelled, failure: "cancelled during phase " + phase.Name} }Then pass
commandCtxto the gate run:- report := e.runGates(ctx, phase, envelope, entry) + report := e.runGates(commandCtx, phase, envelope, entry)🤖 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/engine/phase.go` around lines 762 - 766, Derive a single cancellable phase context that is canceled when e.attempt.Cancelled closes, and use it for both phase command execution and gate evaluation. Update the runGates calls in the code-phase flow near result.Passed() and in runAgentPhaseAttempt to receive commandCtx rather than the uncanceled ctx, preserving the existing phase timeout and cancellation behavior.internal/engine/boundary_test.go-17-38 (1)
17-38: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winIsolate the test helper from the developer's git configuration.
gitInon Line 19 does not setcommand.Env, sogitinherits the test process environment and reads the user and system gitconfig.initBoundaryReposets only the localuser.emailanduser.name.A machine with
commit.gpgsign = true,core.hooksPath, orGIT_CONFIG_GLOBALset makesgit commiton Line 36 fail or behave differently. The result is a test that passes in CI and fails locally, or the reverse.This file already asserts that the production path does not execute repository-supplied code — see
TestJigSideGitCommandsDoNotRunRepositoryHookson Line 436. Apply the same isolation to the fixture that sets the repository up.♻️ Proposed change to pin the helper's git environment
func gitIn(t *testing.T, dir string, args ...string) { t.Helper() command := exec.Command("git", args...) command.Dir = dir + // Pin the environment so a developer's global or system gitconfig + // cannot change what these tests exercise. + command.Env = append(os.Environ(), + "GIT_CONFIG_GLOBAL=/dev/null", + "GIT_CONFIG_SYSTEM=/dev/null", + "GIT_CONFIG_NOSYSTEM=1", + "GIT_TERMINAL_PROMPT=0", + ) if output, err := command.CombinedOutput(); err != nil { t.Fatalf("git %v: %v: %s", args, err, output) } }🤖 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/engine/boundary_test.go` around lines 17 - 38, Update the gitIn test helper to provide an isolated command environment instead of inheriting the test process’s git configuration. Preserve the existing environment variables needed to run Git, while disabling global/system configuration and repository hooks so init, config, add, and commit in initBoundaryRepo behave deterministically.internal/engine/repair.go-34-35 (1)
34-35: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winReject unrecognized boolean literals
ParsePredicatevalidates only the predicate shape and operator, soapproved == falsreaches this code. Any literal other than"true"maps tofalse; therefore, the invalid literal matches afalsefield. Returnfalseunless the literal is exactly"true"or"false". Do not usestrconv.ParseBool, because it also accepts values such as"1"and"t".🤖 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/engine/repair.go` around lines 34 - 35, Update the bool case in the predicate evaluation logic to accept only literals exactly equal to "true" or "false"; return false for any other literal before comparing with typed, while preserving the existing matching behavior for valid boolean values.
🤖 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/claim.go`:
- Around line 139-161: Update the candidate-loading loop in the claim flow to
check rows.Err() immediately after rows.Next() finishes and before closing or
returning candidates. If iteration failed, return unavailable(err) instead of
allowing a truncated candidates list to reach chosenJob and commitEmptyClaim;
preserve the existing scan and close error handling.
In `@internal/controlplane/embedded.go`:
- Around line 601-625: In the candidate-loading flow around rows.Next() in the
enclosing method, check rows.Err() after the iteration completes and before
proceeding with candidates, returning unavailable(err) on failure. Keep the
existing scan-error handling and rows.Close() handling intact, and only act on
the candidate list when iteration ended without error.
- Around line 310-320: Update the Persist closure in the config.Execute call to
append events using the existing completionCtx rather than the
caller-cancellable ctx. Keep FreshenLease on its current context and preserve
the existing store.AppendEvents arguments so final unwinding events continue to
be persisted after cancellation.
In `@internal/controlplane/server.go`:
- Around line 90-93: Make server startup context-aware by adding context.Context
parameters to NewServer and Start, then replace uncancellable address resolution
and net.Listen usage with Resolver.LookupPort, Resolver.LookupIPAddr, and
ListenConfig.Listen using that context. Propagate the context through the
startup flow while preserving existing error handling, and update all test call
sites to pass the required context.
In `@internal/engine/gates.go`:
- Around line 120-123: The gateDiffMatchesClaims function must explicitly record
a failed check when changed_files is missing or cannot be asserted as []any,
before iterating over claimed. Preserve the existing iteration and comparison
behavior for valid arrays, ensuring invalid or absent input cannot leave
GateReport.Checks empty and pass implicitly.
In `@internal/engine/phase.go`:
- Around line 945-949: Update the terminal-send handling around terminalSend so
phaseCancelled, phaseCeiling, and phaseSendBudget exits follow the same rollback
path as death, invoking restoreSnapshot and enforceWriteBoundary before
returning. Apply this consistently to the initial send, parse loop, correction
send, and the shown default branch, while preserving each terminal result and
false status.
- Around line 1276-1293: Update execution.rolePrompt to open the worktree with
os.OpenRoot(e.attempt.WorktreePath) and read the validated relative path via the
returned root’s Open method, rather than filepath.Join followed by os.ReadFile.
Preserve the existing content/path checks and error context, and close the root
and opened file handles appropriately.
In `@internal/runtime/claudecode/adapter.go`:
- Around line 448-455: Update claudeHandle.Result so it waits for the consume
goroutine via <-h.done before calling h.command.Wait(). Preserve the subsequent
cleanup sequence, including stopEverything and h.anchor.Wait(), while ensuring
the terminal result is processed before the command closes its stdout pipe.
In `@internal/worker/claiming.go`:
- Around line 231-245: Update the error paths around the manifest updates in the
attempt execution flow so a write failure retains the worktree via
retainAfterAttempt and reports the runner result via completeAttempt, matching
the start-failure handling. After the runner has finished, preserve and
propagate outcome even when the completed-manifest update fails; ensure both
lifecycle-update failure paths fail closed rather than returning before
retention and outcome reporting.
In `@internal/worker/manifest.go`:
- Around line 409-454: Update manifestStore.validate to reject
manifest.ProcessGroupID values less than or equal to 1, preventing invalid and
globally dangerous signaling targets. Additionally, require manifests in the
active lifecycle state to use a process-group ID greater than 1, while
preserving validation for other lifecycle states.
In `@internal/worker/reconcile.go`:
- Around line 288-303: Update stopProcessGroup to return immediately when
groupID is less than or equal to 1, before negating it or calling syscall.Kill.
Keep the existing termination and grace-period behavior unchanged for valid
process group IDs.
In `@internal/worker/repocache.go`:
- Around line 133-154: Update repoCache.enforceLimit so cleanup of “.clone-”
directories only removes stale temporary directories from previous processes,
never clone directories owned by the current process. Track current-process
temporary directory ownership and make enforceLimit skip those entries while
preserving cleanup of unowned leftovers and the existing cache-limit behavior.
In `@internal/worker/worktree.go`:
- Around line 219-235: Update worktreeRegistered to resolve symlinks for both
Git’s reported path and the expected path before comparing them. Resolve the
parent directory of the expected path, since the worktree itself may already be
deleted, then compare the resulting cleaned paths while preserving the existing
error and match behavior.
---
Minor comments:
In `@cmd/jig/main.go`:
- Around line 26-34: Update the signal handling around signal.NotifyContext in
main so the first SIGINT/SIGTERM cancels the context and a subsequent signal
restores the default disposition, allowing immediate process termination. Ensure
the escalation cleanup is registered independently of the deferred stop, which
os.Exit skips, and keep the documented graceful first-signal behavior intact.
In `@cmd/jig/run_test.go`:
- Around line 436-447: Update waitForScratch to accept a done channel and exit
immediately when it is closed, including while sleeping or before reporting
failure; ensure both goroutine call sites pass the channel, and create it in the
test body with a deferred close before runJigContext can return.
In `@cmd/jig/run.go`:
- Around line 343-362: Update repoHead to accept the existing context and create
the git command with exec.CommandContext, applying protocol.GitCommandTimeout as
the command deadline. Move the ctx nil guard in the caller before repoHead is
invoked, then pass ctx into repoHead so cancellation and timeout apply during
git rev-parse.
In `@cmd/jig/testdata/two-phase-forced.yaml`:
- Around line 8-13: Update the header comments in the two-phase forced test
fixture to note that the parse-correction trap is not deterministic because
composePrompt appends the valid status contract alongside the invalid template,
so a passing first emission is not a regression; preserve the existing
explanation of the required parse-correction behavior.
In `@internal/controlplane/sweep.go`:
- Around line 95-126: After the rows.Next scan loop in the sweep query, check
rows.Err() before processing sw.misses or returning results; on iteration error,
return unavailable(err) and ensure rows is closed consistently. Keep the
existing scan-error and normal-close handling unchanged, while preventing
partial leased and overdue data from being used.
In `@internal/engine/boundary_test.go`:
- Around line 17-38: Update the gitIn test helper to provide an isolated command
environment instead of inheriting the test process’s git configuration. Preserve
the existing environment variables needed to run Git, while disabling
global/system configuration and repository hooks so init, config, add, and
commit in initBoundaryRepo behave deterministically.
In `@internal/engine/phase.go`:
- Around line 842-849: Bound the breach-path portion of the diagnostic in the
phase-abort flow that builds phaseRun.failure, while preserving the total breach
count in the message. Reuse the existing truncation helper or established
diagnostic-size limit (such as truncateText) when formatting strings.Join(paths,
"; "), and ensure the resulting failure remains safe for propagation through
chainEnd.diagnostic and worker.Outcome.Error.
- Around line 273-276: Update the endCancelled branch in the phase
terminal-outcome handling to include the same e.summaryJSON(...) Result payload
used by the other terminal branches, while preserving its AttemptCancelled
state, diagnostic error, and cancellation event.
- Around line 1299-1305: Update composePrompt to perform parameter substitution
in a single pass using a deterministic replacer such as strings.NewReplacer,
ensuring inserted parameter values are never rescanned and output does not
depend on map iteration order. Preserve existing placeholder syntax and
replacement behavior.
- Around line 762-766: Derive a single cancellable phase context that is
canceled when e.attempt.Cancelled closes, and use it for both phase command
execution and gate evaluation. Update the runGates calls in the code-phase flow
near result.Passed() and in runAgentPhaseAttempt to receive commandCtx rather
than the uncanceled ctx, preserving the existing phase timeout and cancellation
behavior.
In `@internal/engine/repair.go`:
- Around line 34-35: Update the bool case in the predicate evaluation logic to
accept only literals exactly equal to "true" or "false"; return false for any
other literal before comparing with typed, while preserving the existing
matching behavior for valid boolean values.
In `@internal/runtime/claudecode/adapter_test.go`:
- Around line 183-190: Fix the `--resume` assertion in the second-send
verification so it requires the exact adjacent argument pair `--resume` followed
by `session.NativeID`, rather than allowing any standalone `--resume`
occurrence. Keep the existing `--session-id` rejection unchanged.
In `@internal/worker/worker_integration_test.go`:
- Around line 215-229: Update the RunnerFunc callback and its git HEAD lookup so
failures from the goroutine do not call gitRun’s t.Fatalf. Return or propagate
the git command error through the runner’s Outcome, then have the test goroutine
report it via t.Error or the existing failure path while preserving the
observation logic for successful commands.
---
Nitpick comments:
In `@cmd/jig/run.go`:
- Around line 429-447: Update seedClaudeAuth to rename the UserHomeDir result
from real to operatorHome and adjust all references, including the credentials
path. Replace the context-free security exec.Command call with a bounded-context
invocation using an appropriate timeout, while preserving the existing keychain
lookup arguments and error handling.
In `@internal/controlplane/claim.go`:
- Line 50: Update the deferred rollback and error-path close call sites in the
claim transaction flow to explicitly discard their returned errors, satisfying
errcheck while preserving the existing cleanup behavior.
- Around line 139-177: Bound candidate selection in the claim flow around the
`tx.QueryContext` and `protocol.ParseDefinition` loop so each round trip fetches
only a fixed-size batch instead of every queued job. Add keyset pagination using
`created_at` and `id`, continue fetching batches until an eligible job is found
or candidates are exhausted, and keep all eligibility checks unchanged.
In `@internal/controlplane/embedded.go`:
- Around line 291-307: Ensure the heartbeat goroutine is joined before teardown
by adding a wait group around the goroutine in the surrounding function,
registering the goroutine with it, and deferring its wait immediately after
defer stopBeats(). Preserve LIFO ordering so cancellation occurs first, the
heartbeat loop exits, and only then the existing store.Close() defer runs.
In `@internal/controlplane/http.go`:
- Around line 358-362: Update writeJSON to capture the error returned by
json.NewEncoder(w).Encode(value) and log it when encoding fails, preserving the
existing response-writing behavior.
In `@internal/controlplane/server.go`:
- Around line 114-120: The Server.Shutdown method must wait for the sweeper
goroutine to finish after invoking s.stop. Add a done channel owned by the
sweeper lifecycle, ensure sweeper.Run closes it when returning, and have
Shutdown wait on that channel before returning from the shutdown sequence while
preserving HTTP server draining.
In `@internal/controlplane/worktree_ledger.go`:
- Around line 201-206: Update boundedReason to truncate at a UTF-8 rune boundary
rather than slicing raw bytes, reusing the worker package’s existing boundedText
helper or a shared equivalent. Preserve the MaxRetentionReasonBytes limit and
return unchanged values that already fit.
- Around line 19-25: Surface the skipped attempt IDs produced by
applyRetainedWorktrees instead of silently discarding them. Update both
RegisterWorker and ReconcileWorktrees to log or otherwise report the returned
skipped IDs to the operator, preserving the documented behavior that foreign or
unknown attempts are named; alternatively remove the skipped return value and
related documentation if no reporting is desired.
In `@internal/engine/boundary_test.go`:
- Around line 68-75: Remove the local contains helper in boundary_test.go and
import the standard-library slices package. Replace each contains call in the
boundary tests with slices.Contains, keeping the existing arguments and behavior
unchanged.
In `@internal/engine/boundary.go`:
- Around line 273-292: Add a reasoned //nolint:nilerr directive to the statErr
return in fingerprintTree, matching the existing directive on the walkErr
branch, while preserving the behavior of recording the stat failure and
continuing traversal.
In `@internal/engine/gates.go`:
- Around line 196-208: Update gateTestsPass to include result.StartError in the
recorded note when the command fails to start, preserving the existing timeout
and output-tail handling for other failures. Ensure the note surfaces the actual
start failure cause instead of only reporting exit -1.
In `@internal/engine/homedir.go`:
- Around line 100-119: Update subprocessEnv to derive homeFamily from the
variable names emitted by homeEnv(home) instead of maintaining a separate
hardcoded map. Use that derived set when filtering composeEnv(base, allow),
preserving the existing removal of HOME-family entries before appending
homeEnv(home).
In `@internal/engine/phase_test.go`:
- Line 920: Update the deferred cleanup around sink.Close to explicitly discard
its returned error by wrapping the call in a deferred function, preserving the
existing cleanup timing while satisfying errcheck.
- Around line 1116-1121: Rename the test constant cap to sendCap in this setup,
and update all references in the loop, make capacity, and config.MaxAttemptSends
assignment to use sendCap.
In `@internal/engine/phase.go`:
- Around line 513-516: Update the PhaseKindAgent handling in the edge evaluation
flow to distinguish predicate parse failures from predicates that evaluate
false. When protocol.ParsePredicate returns an error, emit the same error event
used for frozen-snapshot integrity failures in gates.go, then keep the edge
untriggered; only call predicateHolds when parsing succeeds.
- Around line 41-54: Track the contract mismatch by opening an issue to correct
protocol.WorstCaseSendCount in internal/protocol, documenting that its current
value of 6 understates the engine’s actual send ladder (roughly 252 at budget
3). Leave defaultMaxAttemptSends and the existing TODO unchanged.
In `@internal/protocol/gate_registry_lockstep_test.go`:
- Around line 102-119: Update findModuleRoot to fall back to os.Getwd when
runtime.Caller(0) yields a non-existent source path, such as with -trimpath.
Start the upward go.mod search from the working directory in that case, while
preserving the existing caller-path behavior and root failure handling.
In `@internal/runtime/claudecode/adapter_test.go`:
- Around line 205-206: Replace the fixed 200 ms sleep in the test with the
existing waitForStub helper so startup is deterministic, then call
waitForGroupExit after the result assertion to verify the process group was torn
down as promised by the test.
- Around line 231-241: Update waitForGroupExit to return successfully only when
syscall.Kill(-int(groupID), 0) returns syscall.ESRCH; continue polling for other
errors, including EPERM, and retain the existing timeout failure.
- Around line 148-154: Update the test around StartOrContinue to set a known
environment variable with t.Setenv at the beginning of the test, before the
subprocess starts, then assert that variable is absent from the stub environment
instead of checking GOPATH. Keep the existing MARKER=present assertion and do
not run this test in parallel.
- Around line 296-315: Register t.Cleanup immediately after groupID is assigned
in TestTheProcessGroupAnchorDiesWithItsParentAndTakesTheGroupWithIt, ensuring it
signals the process group with the known group ID. Also register cleanup for the
anchor and member process handles so failures during member.Start or later
t.Fatal paths cannot leave the anchor, shell, or sleep process running.
In `@internal/runtime/claudecode/adapter.go`:
- Around line 196-199: Update the cleanup path in the anchor.Start error branch
to explicitly discard the watchdog.Close error using the file’s existing
underscore-assignment pattern, keeping the existing return behavior unchanged.
In `@internal/worker/manifest.go`:
- Around line 340-377: Update writeProtectedJSON to open and fsync the parent
directory after os.Rename succeeds, then close the directory and return any sync
or close errors with contextual messages. Keep the existing temporary-file
cleanup and atomic rename behavior unchanged.
In `@internal/worker/reconcile_test.go`:
- Around line 1-17: Add a Unix build constraint at the top of the
reconcile_test.go file so the process-group tests using
syscall.SysProcAttr.Setpgid, syscall.Kill, and negative-pid signaling are
excluded from Windows builds while remaining enabled on Unix targets.
In `@internal/worker/reconcile.go`:
- Around line 354-370: Update scanOrphanWorktrees to skip entries that are not
directories by checking entry.IsDir() before constructing and appending their
paths. Preserve the existing manifest-path filtering and error handling for
directory entries.
- Around line 260-286: Update inspectProcessGroupLeader so the ps subprocess
runs with LC_ALL=C before requesting lstart, ensuring its output matches
processStartLayout regardless of the worker’s locale. Preserve the existing
command arguments and parsing behavior.
- Around line 397-404: Add an explicit `//nolint:nilerr` directive with a brief
reason to the intentional `return nil` branch after `removeWorktree` fails in
the reconciliation flow, preserving the existing retention-as-success behavior
and error reporting.
In `@internal/worker/registration.go`:
- Around line 244-265: Update loadOrCreateWorkerID to create the missing worker
identity file exclusively with O_CREATE|O_EXCL instead of os.WriteFile. When
exclusive creation reports that the file already exists, re-read and validate
the existing identity, preserving the current corrupt-file and other I/O error
handling; return the newly generated ID only after successful creation.
In `@internal/worker/repocache.go`:
- Around line 239-242: Update the repository identity parsing return path to
pass parsed.Hostname() rather than parsed.Host into normalizeHostPath, keeping
host validation and normalization consistent while preserving the existing
parsed.Path handling.
In `@internal/worker/worker_integration_test.go`:
- Around line 68-70: The integration test’s SQLite DSN duplicates the production
pragma configuration. Reuse the production `sqlitePragmas` from `controlplane`
by exporting it, or add a small `controlplane` helper for opening the seeding
connection, and update the test setup around `sql.Open` to use that shared
configuration.
In `@Justfile`:
- Around line 15-20: Update the comment above the test-race target to explicitly
name the runtime suite covered by ./internal/runtime/... so its documented scope
matches the command; leave the test command unchanged.
🪄 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: d76ced3a-008b-47c5-9535-02cbd414a353
📒 Files selected for processing (49)
.github/workflows/ci.yml.gitignoreJustfilecmd/jig/main.gocmd/jig/run.gocmd/jig/run_test.gocmd/jig/testdata/two-phase-forced.yamlexamples/definitions/smoke.yamlexamples/definitions/two-phase.yamlinternal/controlplane/claim.gointernal/controlplane/claim_test.gointernal/controlplane/embedded.gointernal/controlplane/embedded_test.gointernal/controlplane/http.gointernal/controlplane/server.gointernal/controlplane/store.gointernal/controlplane/store_test.gointernal/controlplane/sweep.gointernal/controlplane/worktree_ledger.gointernal/engine/accept.gointernal/engine/boundary.gointernal/engine/boundary_test.gointernal/engine/code_phase.gointernal/engine/enginetest/runtime.gointernal/engine/envelope.gointernal/engine/events.gointernal/engine/gates.gointernal/engine/homedir.gointernal/engine/phase.gointernal/engine/phase_test.gointernal/engine/repair.gointernal/protocol/definition.gointernal/protocol/gate_registry_lockstep_test.gointernal/protocol/limits.gointernal/protocol/types.gointernal/runtime/claudecode/adapter.gointernal/runtime/claudecode/adapter_test.gointernal/runtime/runtime.gointernal/worker/claiming.gointernal/worker/client.gointernal/worker/doc.gointernal/worker/manifest.gointernal/worker/reconcile.gointernal/worker/reconcile_test.gointernal/worker/registration.gointernal/worker/repocache.gointernal/worker/worker_integration_test.gointernal/worker/worktree.gomigrations/002_job_cancellation.sql
| rows, err := tx.QueryContext(ctx, ` | ||
| SELECT j.id, r.snapshot | ||
| FROM jobs j JOIN runs r ON r.id = j.run_id | ||
| WHERE j.state = 'queued' | ||
| AND (SELECT COUNT(*) FROM retained_worktrees rw | ||
| WHERE rw.repository = j.repository AND rw.state = 'retained') < ? | ||
| ORDER BY j.created_at, j.id | ||
| `, protocol.MaxRetainedWorktreesPerRepo) | ||
| if err != nil { | ||
| return nil, unavailable(err) | ||
| } | ||
| var candidates []candidate | ||
| for rows.Next() { | ||
| var value candidate | ||
| if err := rows.Scan(&value.jobID, &value.snapshot); err != nil { | ||
| rows.Close() | ||
| return nil, unavailable(err) | ||
| } | ||
| candidates = append(candidates, value) | ||
| } | ||
| if err := rows.Close(); err != nil { | ||
| return nil, unavailable(err) | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Check rows.Err() after the candidate loop.
rows.Next() returns false both at normal end-of-result and on an iteration error. rows.Close() does not report that iteration error, so a mid-scan failure silently truncates candidates.
The consequence is not a dropped log line. A truncated list makes chosenJob empty, and commitEmptyClaim then persists an empty answer under this request_id. The worker replays that emptiness for EmptyClaimTTL even though eligible work was queued.
🐛 Proposed fix
if err := rows.Close(); err != nil {
return nil, unavailable(err)
}
+ if err := rows.Err(); err != nil {
+ return nil, 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.
| rows, err := tx.QueryContext(ctx, ` | |
| SELECT j.id, r.snapshot | |
| FROM jobs j JOIN runs r ON r.id = j.run_id | |
| WHERE j.state = 'queued' | |
| AND (SELECT COUNT(*) FROM retained_worktrees rw | |
| WHERE rw.repository = j.repository AND rw.state = 'retained') < ? | |
| ORDER BY j.created_at, j.id | |
| `, protocol.MaxRetainedWorktreesPerRepo) | |
| if err != nil { | |
| return nil, unavailable(err) | |
| } | |
| var candidates []candidate | |
| for rows.Next() { | |
| var value candidate | |
| if err := rows.Scan(&value.jobID, &value.snapshot); err != nil { | |
| rows.Close() | |
| return nil, unavailable(err) | |
| } | |
| candidates = append(candidates, value) | |
| } | |
| if err := rows.Close(); err != nil { | |
| return nil, unavailable(err) | |
| } | |
| rows, err := tx.QueryContext(ctx, ` | |
| SELECT j.id, r.snapshot | |
| FROM jobs j JOIN runs r ON r.id = j.run_id | |
| WHERE j.state = 'queued' | |
| AND (SELECT COUNT(*) FROM retained_worktrees rw | |
| WHERE rw.repository = j.repository AND rw.state = 'retained') < ? | |
| ORDER BY j.created_at, j.id | |
| `, protocol.MaxRetainedWorktreesPerRepo) | |
| if err != nil { | |
| return nil, unavailable(err) | |
| } | |
| var candidates []candidate | |
| for rows.Next() { | |
| var value candidate | |
| if err := rows.Scan(&value.jobID, &value.snapshot); err != nil { | |
| rows.Close() | |
| return nil, unavailable(err) | |
| } | |
| candidates = append(candidates, value) | |
| } | |
| if err := rows.Close(); err != nil { | |
| return nil, unavailable(err) | |
| } | |
| if err := rows.Err(); err != nil { | |
| return nil, unavailable(err) | |
| } |
🧰 Tools
🪛 golangci-lint (2.12.2)
[error] 154-154: Error return value of rows.Close is not checked
(errcheck)
[error] 139-139: rows.Err must be checked
(rowserrcheck)
🤖 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/claim.go` around lines 139 - 161, Update the
candidate-loading loop in the claim flow to check rows.Err() immediately after
rows.Next() finishes and before closing or returning candidates. If iteration
failed, return unavailable(err) instead of allowing a truncated candidates list
to reach chosenJob and commitEmptyClaim; preserve the existing scan and close
error handling.
Source: Linters/SAST tools
| outcome := config.Execute(ctx, DirectExecution{ | ||
| Claim: *claim, | ||
| TracePath: tracePath, | ||
| Persist: func(event protocol.Event) error { | ||
| return store.AppendEvents(ctx, attemptID, []protocol.Event{event}) | ||
| }, | ||
| FreshenLease: func(ctx context.Context) error { | ||
| _, err := store.Heartbeat(ctx, attemptID, protocol.HeartbeatRequest{LeaseToken: token}) | ||
| return err | ||
| }, | ||
| }) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Persist rides the cancellable context, so an interrupted run loses its final events from the store.
completionCtx exists because "completion must survive cancellation" (Lines 178-182), and CompleteAttempt at Line 332 uses it. The Persist closure at Lines 313-315 still captures the caller's cancellable ctx. After Ctrl-C, ctx.Err() is non-nil, so every AppendEvents call fails for the remainder of the attempt.
The engine keeps emitting during unwinding: rollback, boundary enforcement, and the terminal phase events. Those events reach the JSONL trace but never reach the events table. The package doc at Lines 8-11 states that a direct run's record is indistinguishable in shape from a server-path run, and cmd/jig/run_test.go Lines 274-278 assert that the store row count equals the JSONL line count. That invariant holds only on the non-cancelled path.
Persist on the detached context.
🐛 Proposed fix
Persist: func(event protocol.Event) error {
- return store.AppendEvents(ctx, attemptID, []protocol.Event{event})
+ // Trace persistence outlives cancellation for the same reason
+ // completion does: an interrupted attempt still owes its record.
+ return store.AppendEvents(completionCtx, attemptID, []protocol.Event{event})
},📝 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.
| outcome := config.Execute(ctx, DirectExecution{ | |
| Claim: *claim, | |
| TracePath: tracePath, | |
| Persist: func(event protocol.Event) error { | |
| return store.AppendEvents(ctx, attemptID, []protocol.Event{event}) | |
| }, | |
| FreshenLease: func(ctx context.Context) error { | |
| _, err := store.Heartbeat(ctx, attemptID, protocol.HeartbeatRequest{LeaseToken: token}) | |
| return err | |
| }, | |
| }) | |
| outcome := config.Execute(ctx, DirectExecution{ | |
| Claim: *claim, | |
| TracePath: tracePath, | |
| Persist: func(event protocol.Event) error { | |
| // Trace persistence outlives cancellation for the same reason | |
| // completion does: an interrupted attempt still owes its record. | |
| return store.AppendEvents(completionCtx, attemptID, []protocol.Event{event}) | |
| }, | |
| FreshenLease: func(ctx context.Context) error { | |
| _, err := store.Heartbeat(ctx, attemptID, protocol.HeartbeatRequest{LeaseToken: token}) | |
| return 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/embedded.go` around lines 310 - 320, Update the Persist
closure in the config.Execute call to append events using the existing
completionCtx rather than the caller-cancellable ctx. Keep FreshenLease on its
current context and preserve the existing store.AppendEvents arguments so final
unwinding events continue to be persisted after cancellation.
| rows, err := s.db.QueryContext(ctx, ` | ||
| SELECT a.id, a.job_id, a.worker_id, a.lease_expires_at | ||
| FROM attempts a JOIN jobs j ON j.id = a.job_id | ||
| WHERE a.state IN ('queued', 'preparing', 'running') AND j.state IN ('queued', 'active') | ||
| `) | ||
| if err != nil { | ||
| return nil, unavailable(err) | ||
| } | ||
| type candidate struct { | ||
| attemptID, jobID string | ||
| workerID sql.NullString | ||
| expiry sql.NullInt64 | ||
| } | ||
| var candidates []candidate | ||
| for rows.Next() { | ||
| var value candidate | ||
| if err := rows.Scan(&value.attemptID, &value.jobID, &value.workerID, &value.expiry); err != nil { | ||
| rows.Close() | ||
| return nil, unavailable(err) | ||
| } | ||
| candidates = append(candidates, value) | ||
| } | ||
| if err := rows.Close(); err != nil { | ||
| return nil, unavailable(err) | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Check rows.Err() before you act on the candidate list.
The loop stops on the first rows.Next() that returns false. That happens both at the natural end of the result set and on an iteration error. The code does not distinguish the two, so a truncated candidate list is treated as complete. Reclamation then silently skips abandoned attempts, which is the wedge this function exists to prevent. golangci-lint rowserrcheck flags Line 601.
🐛 Proposed fix
}
+ if err := rows.Err(); err != nil {
+ rows.Close()
+ return nil, unavailable(err)
+ }
if err := rows.Close(); err != nil {
return nil, 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.
| rows, err := s.db.QueryContext(ctx, ` | |
| SELECT a.id, a.job_id, a.worker_id, a.lease_expires_at | |
| FROM attempts a JOIN jobs j ON j.id = a.job_id | |
| WHERE a.state IN ('queued', 'preparing', 'running') AND j.state IN ('queued', 'active') | |
| `) | |
| if err != nil { | |
| return nil, unavailable(err) | |
| } | |
| type candidate struct { | |
| attemptID, jobID string | |
| workerID sql.NullString | |
| expiry sql.NullInt64 | |
| } | |
| var candidates []candidate | |
| for rows.Next() { | |
| var value candidate | |
| if err := rows.Scan(&value.attemptID, &value.jobID, &value.workerID, &value.expiry); err != nil { | |
| rows.Close() | |
| return nil, unavailable(err) | |
| } | |
| candidates = append(candidates, value) | |
| } | |
| if err := rows.Close(); err != nil { | |
| return nil, unavailable(err) | |
| } | |
| rows, err := s.db.QueryContext(ctx, ` | |
| SELECT a.id, a.job_id, a.worker_id, a.lease_expires_at | |
| FROM attempts a JOIN jobs j ON j.id = a.job_id | |
| WHERE a.state IN ('queued', 'preparing', 'running') AND j.state IN ('queued', 'active') | |
| `) | |
| if err != nil { | |
| return nil, unavailable(err) | |
| } | |
| type candidate struct { | |
| attemptID, jobID string | |
| workerID sql.NullString | |
| expiry sql.NullInt64 | |
| } | |
| var candidates []candidate | |
| for rows.Next() { | |
| var value candidate | |
| if err := rows.Scan(&value.attemptID, &value.jobID, &value.workerID, &value.expiry); err != nil { | |
| rows.Close() | |
| return nil, unavailable(err) | |
| } | |
| candidates = append(candidates, value) | |
| } | |
| if err := rows.Err(); err != nil { | |
| rows.Close() | |
| return nil, unavailable(err) | |
| } | |
| if err := rows.Close(); err != nil { | |
| return nil, unavailable(err) | |
| } |
🧰 Tools
🪛 golangci-lint (2.12.2)
[error] 601-601: rows.Err must be checked
(rowserrcheck)
🤖 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/embedded.go` around lines 601 - 625, In the
candidate-loading flow around rows.Next() in the enclosing method, check
rows.Err() after the iteration completes and before proceeding with candidates,
returning unavailable(err) on failure. Keep the existing scan-error handling and
rows.Close() handling intact, and only act on the candidate list when iteration
ended without error.
Source: Linters/SAST tools
| listener, err := net.Listen("tcp", s.http.Addr) | ||
| if err != nil { | ||
| return fmt.Errorf("listen on %s: %w", s.http.Addr, err) | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Confirm noctx is enabled in the lint configuration and find every caller of NewServer/Start.
fd -H -t f '^\.golangci' --exec cat {}
rg -n -C3 'controlplane\.NewServer|\.Start\(\)' --type=goRepository: StructuPath/jig
Length of output: 153
🏁 Script executed:
#!/bin/bash
set -u
printf '%s\n' '--- tracked lint/config files ---'
git ls-files | grep -E '(^|/)(\.golangci[^/]*|Makefile|go\.mod|go\.sum)$' || true
printf '%s\n' '--- controlplane server symbols ---'
rg -n -C5 'func (NewServer|Start)|net\.(Listen|LookupPort|LookupIP)|LookupIPAddr|ListenConfig|Resolver' internal/controlplane --type go || true
printf '%s\n' '--- NewServer and Start callers ---'
rg -n -C4 '(controlplane\.)?NewServer|\.Start\(' --type go . || true
printf '%s\n' '--- lint configuration references ---'
rg -n -C3 'noctx|golangci' -g '*.yml' -g '*.yaml' -g '*.toml' -g '*.json' -g 'Makefile' -g '*.md' . || trueRepository: StructuPath/jig
Length of output: 12723
🏁 Script executed:
#!/bin/bash
set -u
printf '%s\n' '--- module version ---'
cat go.mod
printf '%s\n' '--- server implementation ---'
cat -n internal/controlplane/server.go | sed -n '1,180p'
printf '%s\n' '--- all NewServer references ---'
rg -n -C5 '\bNewServer\s*\(' --type go .
printf '%s\n' '--- all server.Start references ---'
rg -n -C5 '\bserver\.Start\s*\(' --type go .
printf '%s\n' '--- controlplane tests around construction/start ---'
cat -n internal/controlplane/store_test.go | sed -n '560,615p'Repository: StructuPath/jig
Length of output: 14543
🏁 Script executed:
#!/bin/bash
set -u
printf '%s\n' '--- all top-level and hidden files ---'
find . -maxdepth 2 -type f -print | sort
printf '%s\n' '--- noctx references in all files ---'
rg -n -uuu 'noctx|golangci' . || true
printf '%s\n' '--- Go toolchain and Resolver APIs ---'
if command -v go >/dev/null 2>&1; then
go version
go env GOROOT
go doc net.ListenConfig.Listen
go doc net.Resolver.LookupPort
go doc net.Resolver.LookupIPAddr
else
printf '%s\n' 'go command unavailable'
fi
printf '%s\n' '--- standard-library resolver/listener context implementations ---'
if command -v go >/dev/null 2>&1; then
goroot="$(go env GOROOT)"
rg -n -C4 'func \(.*Resolver\).*Lookup(Port|IPAddr)|func \(.*ListenConfig\).*Listen' "$goroot/src/net" || true
fiRepository: StructuPath/jig
Length of output: 4367
🏁 Script executed:
#!/bin/bash
set -u
python3 - <<'PY'
from pathlib import Path
source = Path("internal/controlplane/server.go").read_text()
checks = {
"NewServer calls validation": "validateLoopbackAddress(address)" in source,
"validation calls net.LookupPort": "net.LookupPort(\"tcp\", port)" in source,
"validation calls net.LookupIP": "net.LookupIP(host)" in source,
"Start calls net.Listen": "net.Listen(\"tcp\", s.http.Addr)" in source,
}
for name, result in checks.items():
print(f"{name}: {result}")
assert all(checks.values())
PY
cat >/tmp/context_probe.go <<'GO'
package main
import (
"context"
"errors"
"fmt"
"net"
"time"
)
func main() {
resolver := &net.Resolver{
PreferGo: true,
Dial: func(ctx context.Context, _, _ string) (net.Conn, error) {
<-ctx.Done()
return nil, ctx.Err()
},
}
ctx, cancel := context.WithTimeout(context.Background(), 20*time.Millisecond)
defer cancel()
_, err := resolver.LookupIPAddr(ctx, "example.invalid")
if !errors.Is(err, context.DeadlineExceeded) {
panic(fmt.Sprintf("LookupIPAddr error = %v, want context deadline", err))
}
ctx, cancel = context.WithCancel(context.Background())
cancel()
_, err = (&net.ListenConfig{}).Listen(ctx, "tcp", "localhost:0")
if !errors.Is(err, context.Canceled) {
panic(fmt.Sprintf("Listen error = %v, want context canceled", err))
}
fmt.Println("context cancellation reached Resolver.LookupIPAddr and ListenConfig.Listen")
}
GO
go run /tmp/context_probe.go
rm -f /tmp/context_probe.goRepository: StructuPath/jig
Length of output: 401
Make server startup context-aware.
NewServer performs uncancellable localhost DNS lookups, and Start uses net.Listen with a background context. A slow resolver can block startup for a resolver-controlled duration. Pass context.Context to both methods and use Resolver.LookupPort, Resolver.LookupIPAddr, and ListenConfig.Listen. Update the test call sites.
🧰 Tools
🪛 golangci-lint (2.12.2)
[error] 90-90: net.Listen must not be called. use (*net.ListenConfig).Listen
(noctx)
🤖 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/server.go` around lines 90 - 93, Make server startup
context-aware by adding context.Context parameters to NewServer and Start, then
replace uncancellable address resolution and net.Listen usage with
Resolver.LookupPort, Resolver.LookupIPAddr, and ListenConfig.Listen using that
context. Propagate the context through the startup flow while preserving
existing error handling, and update all test call sites to pass the required
context.
Source: Linters/SAST tools
| func gateDiffMatchesClaims(gc gateContext) protocol.GateReport { | ||
| var report protocol.GateReport | ||
| claimed, _ := gc.envelope.Fields["changed_files"].([]any) | ||
| for _, item := range claimed { |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Resolve GateReport.Passed()/Violations() semantics and find gate configurations.
set -euo pipefail
# Test: Passed() must be shown to handle an empty Checks slice. Expect: an explicit len() guard, or proof it returns true.
ast-grep run --pattern $'func ($_ $_) Passed() bool {
$$$
}' --lang go internal/protocol
ast-grep run --pattern $'func ($_ $_) Violations() []string {
$$$
}' --lang go internal/protocol
# Test: Find every definition that configures diff_matches_claims. Expect: they rely on the agent emitting changed_files.
rg -n -C 4 'diff_matches_claims' --glob '*.yaml' --glob '*.yml' --glob '*.go'Repository: StructuPath/jig
Length of output: 1030
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- gateDiffMatchesClaims ---'
sed -n '108,155p' internal/engine/gates.go
printf '%s\n' '--- GateReport methods ---'
sed -n '195,235p' internal/protocol/types.go
printf '%s\n' '--- diff_matches_claims configurations and related tests ---'
rg -n -C 5 'diff_matches_claims|changed_files' \
--glob '*.yaml' --glob '*.yml' --glob '*.go' .Repository: StructuPath/jig
Length of output: 9891
Fail when changed_files is missing or invalid.
GateReport.Passed() returns true when Checks is empty. The type assertion at internal/engine/gates.go:122 therefore lets an agent bypass diff_matches_claims by omitting changed_files or providing a non-array value. Record an explicit failed check for both cases before iterating.
🤖 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/engine/gates.go` around lines 120 - 123, The gateDiffMatchesClaims
function must explicitly record a failed check when changed_files is missing or
cannot be asserted as []any, before iterating over claimed. Preserve the
existing iteration and comparison behavior for valid arrays, ensuring invalid or
absent input cannot leave GateReport.Checks empty and pass implicitly.
| if _, err := w.manifests.update(claim.Attempt.ID, func(manifest *attemptManifest) error { | ||
| manifest.Lifecycle = manifestRunning | ||
| return nil | ||
| }); err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| outcome := w.config.Runner.Run(ctx, prepared) | ||
| if _, err := w.manifests.update(claim.Attempt.ID, func(manifest *attemptManifest) error { | ||
| manifest.Lifecycle = manifestCompleted | ||
| manifest.TerminalState = outcome.State | ||
| return nil | ||
| }); err != nil { | ||
| return nil, err | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
A manifest write failure drops the attempt outcome and leaves the worktree unretained.
Both manifest updates return early on error. Neither path calls completeAttempt nor retainAfterAttempt.
At Line 239 the runner has already finished. Its outcome is discarded, so the control plane never learns the terminal state. The attempt stays running until the sweeper marks it lost, and the job then reports failed even when the runner accepted the work. The worktree is also left with no ledger retention, so the operator has no record of the surviving work.
This contradicts the file's own contract at Line 200: "disposal afterward fails closed regardless of how the attempt ended". The start-failure branch at Line 227 already applies that rule.
Treat a manifest write failure the same way: retain the worktree and report the outcome.
🐛 Proposed fix
if _, err := w.manifests.update(claim.Attempt.ID, func(manifest *attemptManifest) error {
manifest.Lifecycle = manifestRunning
return nil
}); err != nil {
+ w.retainAfterAttempt(ctx, claim.Attempt.ID,
+ "attempt manifest could not record the running lifecycle: "+err.Error())
return nil, err
}
outcome := w.config.Runner.Run(ctx, prepared)
if _, err := w.manifests.update(claim.Attempt.ID, func(manifest *attemptManifest) error {
manifest.Lifecycle = manifestCompleted
manifest.TerminalState = outcome.State
return nil
}); err != nil {
- return nil, err
+ // The outcome exists; record it, then retain on the manifest doubt.
+ w.logger.Warn("attempt_manifest_completion_failed",
+ "attempt_id", claim.Attempt.ID, "error", err)
+ if _, completeErr := w.completeAttempt(ctx, lease, outcome); completeErr != nil {
+ err = errors.Join(err, completeErr)
+ }
+ w.retainAfterAttempt(ctx, claim.Attempt.ID,
+ "attempt manifest could not record completion: "+err.Error())
+ return nil, 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 := w.manifests.update(claim.Attempt.ID, func(manifest *attemptManifest) error { | |
| manifest.Lifecycle = manifestRunning | |
| return nil | |
| }); err != nil { | |
| return nil, err | |
| } | |
| outcome := w.config.Runner.Run(ctx, prepared) | |
| if _, err := w.manifests.update(claim.Attempt.ID, func(manifest *attemptManifest) error { | |
| manifest.Lifecycle = manifestCompleted | |
| manifest.TerminalState = outcome.State | |
| return nil | |
| }); err != nil { | |
| return nil, err | |
| } | |
| if _, err := w.manifests.update(claim.Attempt.ID, func(manifest *attemptManifest) error { | |
| manifest.Lifecycle = manifestRunning | |
| return nil | |
| }); err != nil { | |
| w.retainAfterAttempt(ctx, claim.Attempt.ID, | |
| "attempt manifest could not record the running lifecycle: "+err.Error()) | |
| return nil, err | |
| } | |
| outcome := w.config.Runner.Run(ctx, prepared) | |
| if _, err := w.manifests.update(claim.Attempt.ID, func(manifest *attemptManifest) error { | |
| manifest.Lifecycle = manifestCompleted | |
| manifest.TerminalState = outcome.State | |
| return nil | |
| }); err != nil { | |
| // The outcome exists; record it, then retain on the manifest doubt. | |
| w.logger.Warn("attempt_manifest_completion_failed", | |
| "attempt_id", claim.Attempt.ID, "error", err) | |
| if _, completeErr := w.completeAttempt(ctx, lease, outcome); completeErr != nil { | |
| err = errors.Join(err, completeErr) | |
| } | |
| w.retainAfterAttempt(ctx, claim.Attempt.ID, | |
| "attempt manifest could not record completion: "+err.Error()) | |
| return nil, 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/worker/claiming.go` around lines 231 - 245, Update the error paths
around the manifest updates in the attempt execution flow so a write failure
retains the worktree via retainAfterAttempt and reports the runner result via
completeAttempt, matching the start-failure handling. After the runner has
finished, preserve and propagate outcome even when the completed-manifest update
fails; ensure both lifecycle-update failure paths fail closed rather than
returning before retention and outcome reporting.
| func (store *manifestStore) validate(manifest attemptManifest) error { | ||
| if manifest.SchemaVersion != manifestSchemaVersion { | ||
| return fmt.Errorf("unsupported attempt manifest schema version %d", manifest.SchemaVersion) | ||
| } | ||
| for name, value := range map[string]string{ | ||
| "worker_id": manifest.WorkerID, "job_id": manifest.JobID, "attempt_id": manifest.AttemptID, | ||
| } { | ||
| if !uuidPattern.MatchString(value) { | ||
| return fmt.Errorf("attempt manifest %s is not a valid UUID", name) | ||
| } | ||
| } | ||
| if manifest.WorkerID != store.workerID { | ||
| return errors.New("attempt manifest belongs to a different worker") | ||
| } | ||
| if manifest.AttemptNumber < 1 { | ||
| return errors.New("attempt manifest attempt number must be positive") | ||
| } | ||
| if strings.TrimSpace(manifest.Repository) == "" { | ||
| return errors.New("attempt manifest repository identity is required") | ||
| } | ||
| if !filepath.IsAbs(manifest.RepositoryDir) || filepath.Clean(manifest.RepositoryDir) != manifest.RepositoryDir { | ||
| return errors.New("attempt manifest repository directory is not canonical") | ||
| } | ||
| if !commitPattern.MatchString(manifest.BaseSHA) { | ||
| return errors.New("attempt manifest base SHA is invalid") | ||
| } | ||
| expectedPath := filepath.Join(store.dataDirectory, "worktrees", manifest.AttemptID) | ||
| if manifest.WorktreePath != expectedPath { | ||
| return errors.New("attempt manifest worktree path is not the owned jig path") | ||
| } | ||
| if manifest.Branch != attemptBranch(manifest.JobID, manifest.AttemptNumber) { | ||
| return errors.New("attempt manifest branch does not match its job and attempt") | ||
| } | ||
| if !manifestLifecycles[manifest.Lifecycle] { | ||
| return fmt.Errorf("attempt manifest lifecycle %q is invalid", manifest.Lifecycle) | ||
| } | ||
| if manifest.CleanupIntent != "" && | ||
| manifest.CleanupIntent != cleanupIntentAutomatic && | ||
| manifest.CleanupIntent != cleanupIntentOperator { | ||
| return fmt.Errorf("attempt manifest cleanup intent %q is invalid", manifest.CleanupIntent) | ||
| } | ||
| if manifest.CreatedAt.IsZero() || manifest.UpdatedAt.IsZero() { | ||
| return errors.New("attempt manifest timestamps are incomplete") | ||
| } | ||
| return nil | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Check how reconcile validates ProcessGroupID before signalling.
set -euo pipefail
fd -t f 'reconcile.go' internal/worker --exec cat -n {}
echo '--- every signal send in the worker package ---'
rg -nP --type=go -C6 'syscall\.Kill|ProcessGroupID' internal/workerRepository: StructuPath/jig
Length of output: 30427
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- manifest loading and validation ---'
rg -nP --type=go -C8 'func \(store \*manifestStore\) (load|loadAll|update|validate)|\.validate\(' internal/worker/manifest.go internal/worker
echo '--- process-group recording and manifest fixtures ---'
rg -nP --type=go -C8 'RecordProcessGroup|ProcessGroupID|ProcessActive|CreatedAt|UpdatedAt' internal/worker --glob '*.go'Repository: StructuPath/jig
Length of output: 30997
Denial of Service (CWE-20): Improper Input Validation
Reachability: Internal
Reject ProcessGroupID == 1 before reconciliation signals it.
Reconciliation already skips ProcessGroupID <= 0, so kill(-0, ...) is unreachable. It does not skip ProcessGroupID == 1; if identity checks accept PID 1, syscall.Kill(-1, ...) can signal every process available to the worker. Reject negative IDs and 1 in validate, and require an active manifest to use an ID greater than 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 `@internal/worker/manifest.go` around lines 409 - 454, Update
manifestStore.validate to reject manifest.ProcessGroupID values less than or
equal to 1, preventing invalid and globally dangerous signaling targets.
Additionally, require manifests in the active lifecycle state to use a
process-group ID greater than 1, while preserving validation for other lifecycle
states.
| // stopProcessGroup TERMs the whole group, waits out the grace window, then | ||
| // KILLs whatever remains. ESRCH means already gone, which is success. | ||
| func stopProcessGroup(groupID int64) { | ||
| pgid := int(groupID) | ||
| if err := syscall.Kill(-pgid, syscall.SIGTERM); err != nil { | ||
| return | ||
| } | ||
| deadline := time.Now().Add(processTerminationGrace) | ||
| for time.Now().Before(deadline) { | ||
| time.Sleep(processTerminationPoll) | ||
| if err := syscall.Kill(-pgid, 0); err != nil { | ||
| return | ||
| } | ||
| } | ||
| _ = syscall.Kill(-pgid, syscall.SIGKILL) | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Reject a process group id of 1 before signalling.
stopProcessGroup negates groupID and passes it to syscall.Kill. A groupID of 1 produces syscall.Kill(-1, SIGTERM), which signals every process the calling user may signal, then SIGKILL after the grace window. The caller at Line 207 only rejects ProcessGroupID <= 0.
Today the start-time window in processGroupIsOurs blocks this path, because pid 1 starts at boot and falls outside manifest.CreatedAt-30s .. manifest.UpdatedAt+30s. That protection is incidental. The sink itself accepts the value, so a corrupt manifest, a clock jump, or a future change to the identity check turns a single bad integer into a machine-wide kill.
Add an explicit guard in stopProcessGroup.
🛡️ Proposed fix
func stopProcessGroup(groupID int64) {
+ // -1 signals every process the user may signal; 0 signals our own group.
+ // Neither can ever be an attempt's agent group.
+ if groupID <= 1 {
+ return
+ }
pgid := int(groupID)📝 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.
| // stopProcessGroup TERMs the whole group, waits out the grace window, then | |
| // KILLs whatever remains. ESRCH means already gone, which is success. | |
| func stopProcessGroup(groupID int64) { | |
| pgid := int(groupID) | |
| if err := syscall.Kill(-pgid, syscall.SIGTERM); err != nil { | |
| return | |
| } | |
| deadline := time.Now().Add(processTerminationGrace) | |
| for time.Now().Before(deadline) { | |
| time.Sleep(processTerminationPoll) | |
| if err := syscall.Kill(-pgid, 0); err != nil { | |
| return | |
| } | |
| } | |
| _ = syscall.Kill(-pgid, syscall.SIGKILL) | |
| } | |
| // stopProcessGroup TERMs the whole group, waits out the grace window, then | |
| // KILLs whatever remains. ESRCH means already gone, which is success. | |
| func stopProcessGroup(groupID int64) { | |
| // -1 signals every process the user may signal; 0 signals our own group. | |
| // Neither can ever be an attempt's agent group. | |
| if groupID <= 1 { | |
| return | |
| } | |
| pgid := int(groupID) | |
| if err := syscall.Kill(-pgid, syscall.SIGTERM); err != nil { | |
| return | |
| } | |
| deadline := time.Now().Add(processTerminationGrace) | |
| for time.Now().Before(deadline) { | |
| time.Sleep(processTerminationPoll) | |
| if err := syscall.Kill(-pgid, 0); err != nil { | |
| return | |
| } | |
| } | |
| _ = syscall.Kill(-pgid, syscall.SIGKILL) | |
| } |
🤖 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/reconcile.go` around lines 288 - 303, Update stopProcessGroup
to return immediately when groupID is less than or equal to 1, before negating
it or calling syscall.Kill. Keep the existing termination and grace-period
behavior unchanged for valid process group IDs.
| func (c *repoCache) enforceLimit() error { | ||
| entries, err := os.ReadDir(c.root) | ||
| if err != nil { | ||
| return fmt.Errorf("list repository cache: %w", err) | ||
| } | ||
| installed := 0 | ||
| for _, entry := range entries { | ||
| if strings.HasPrefix(entry.Name(), ".clone-") { | ||
| if err := os.RemoveAll(filepath.Join(c.root, entry.Name())); err != nil { | ||
| return fmt.Errorf("remove interrupted clone: %w", err) | ||
| } | ||
| continue | ||
| } | ||
| if entry.IsDir() { | ||
| installed++ | ||
| } | ||
| } | ||
| if installed >= protocol.MaxCachedRepositories { | ||
| return fmt.Errorf("repository cache limit of %d entries reached", protocol.MaxCachedRepositories) | ||
| } | ||
| return nil | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
enforceLimit can delete another goroutine's in-flight clone.
Entry mutexes are per-identity, so two different uncached repositories can materialize at the same time when worker capacity is above one. Each of them calls enforceLimit, and enforceLimit removes every .clone-* directory in the shared cache root — including the temporary directory the other goroutine is currently cloning into.
The concurrent clone then fails at git init, at fetch, or at the os.Rename of a directory that no longer exists. The attempt fails preparation for a reason unrelated to the repository.
The cleanup itself is worth keeping. Scope it so it only reclaims leftovers from a previous process. One option is to track the temporary directories this process owns and skip them.
🐛 Proposed fix: skip temporary directories owned by this process
type repoCache struct {
root string
mutex sync.Mutex
entries map[string]*repoEntry
+ // inFlight holds the .clone-* directory names this process is
+ // currently building into, so enforceLimit never reclaims them.
+ inFlight map[string]bool
} func newRepoCache(root string) *repoCache {
- return &repoCache{root: root, entries: make(map[string]*repoEntry)}
+ return &repoCache{
+ root: root,
+ entries: make(map[string]*repoEntry),
+ inFlight: make(map[string]bool),
+ }
} temporary, err := os.MkdirTemp(c.root, ".clone-")
if err != nil {
return fmt.Errorf("create temporary clone directory: %w", err)
}
- defer os.RemoveAll(temporary)
+ c.mutex.Lock()
+ c.inFlight[filepath.Base(temporary)] = true
+ c.mutex.Unlock()
+ defer func() {
+ c.mutex.Lock()
+ delete(c.inFlight, filepath.Base(temporary))
+ c.mutex.Unlock()
+ _ = os.RemoveAll(temporary)
+ }() for _, entry := range entries {
if strings.HasPrefix(entry.Name(), ".clone-") {
+ c.mutex.Lock()
+ owned := c.inFlight[entry.Name()]
+ c.mutex.Unlock()
+ if owned {
+ continue
+ }
if err := os.RemoveAll(filepath.Join(c.root, entry.Name())); err != nil {
return fmt.Errorf("remove interrupted clone: %w", err)
}
continue
}🤖 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/repocache.go` around lines 133 - 154, Update
repoCache.enforceLimit so cleanup of “.clone-” directories only removes stale
temporary directories from previous processes, never clone directories owned by
the current process. Track current-process temporary directory ownership and
make enforceLimit skip those entries while preserving cleanup of unowned
leftovers and the existing cache-limit behavior.
| func worktreeRegistered(ctx context.Context, repositoryDir, path string) (bool, error) { | ||
| stdout, err := runGit(ctx, repositoryDir, "worktree", "list", "--porcelain") | ||
| if err != nil { | ||
| return false, err | ||
| } | ||
| for _, line := range strings.Split(stdout, "\n") { | ||
| value, found := strings.CutPrefix(line, "worktree ") | ||
| if !found { | ||
| continue | ||
| } | ||
| absolute, absErr := filepath.Abs(strings.TrimSpace(value)) | ||
| if absErr == nil && filepath.Clean(absolute) == path { | ||
| return true, nil | ||
| } | ||
| } | ||
| return false, nil | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Compare resolved paths in worktreeRegistered.
The function compares git's reported worktree path against manifest.WorktreePath after filepath.Abs and filepath.Clean. Neither call resolves symlinks. Git reports the worktree path in its own resolved form.
On macOS this diverges in the ordinary case. t.TempDir() and the default temporary root return paths under /var/folders/..., and /var is a symlink to /private/var. Git reports /private/var/... while the manifest holds /var/..., so the strings never match.
The failure mode is silent. worktreeRegistered returns false, removeWorktree concludes the registration is gone, and a stale registration survives in the cache entry. The check that exists specifically because "git reporting success is not the same as the worktree being gone" then proves nothing on that platform.
Resolve both sides before comparing. Resolve the parent directory, because the worktree path itself is already deleted by the time this runs.
🐛 Proposed fix
// worktreeRegistered reports whether git still lists the path as a worktree
-// of the cache entry.
+// of the cache entry. Both sides are resolved through symlinks first: git
+// reports resolved paths, and the manifest holds the unresolved owned path.
func worktreeRegistered(ctx context.Context, repositoryDir, path string) (bool, error) {
stdout, err := runGit(ctx, repositoryDir, "worktree", "list", "--porcelain")
if err != nil {
return false, err
}
+ want := resolvedWorktreePath(path)
for _, line := range strings.Split(stdout, "\n") {
value, found := strings.CutPrefix(line, "worktree ")
if !found {
continue
}
- absolute, absErr := filepath.Abs(strings.TrimSpace(value))
- if absErr == nil && filepath.Clean(absolute) == path {
+ absolute, absErr := filepath.Abs(strings.TrimSpace(value))
+ if absErr != nil {
+ continue
+ }
+ if resolvedWorktreePath(absolute) == want {
return true, nil
}
}
return false, nil
}
+
+// resolvedWorktreePath canonicalizes a worktree path through its parent, so
+// it still resolves after the worktree directory itself is deleted.
+func resolvedWorktreePath(path string) string {
+ path = filepath.Clean(path)
+ parent, err := filepath.EvalSymlinks(filepath.Dir(path))
+ if err != nil {
+ return path
+ }
+ return filepath.Join(parent, filepath.Base(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/worktree.go` around lines 219 - 235, Update
worktreeRegistered to resolve symlinks for both Git’s reported path and the
expected path before comparing them. Resolve the parent directory of the
expected path, since the worktree itself may already be deleted, then compare
the resulting cleaned paths while preserving the existing error and match
behavior.
CodeRabbit review of #1 surfaced 13 findings; two were regressions from the prior review-fix commit. - stopProcessGroup passed an unvalidated manifest value to syscall.Kill(-pgid). Group id 1 means kill(-1, ...) — every process the operator can signal — and the identity check does not block it: pid 1 leads group 1, so only a start-time window stood in the way, and a boot-started worker satisfies it. Guarded in the predicate, the identity gate, the signal path, and manifest validation. - The direct run's Persist closure rode the cancellable context while completion used the detached one, so an interrupt dropped the unwinding events from the store. - Terminal phase exits (cancel, ceiling, send budget) skipped write boundary enforcement; two are agent-steerable, and a hook planted in the shared git dir outlives the attempt. - Prompt paths were read with IsLocal plus ReadFile, which follows symlinks the boundary permits; now opened under an OpenRoot. - diff_matches_claims passed vacuously when changed_files was absent. - rows.Err() unchecked in two claim loops; a truncated candidate list persists an empty claim the worker then replays. - enforceLimit could delete a concurrent in-flight clone; Result() raced the stream consumer; a manifest write failure dropped the attempt outcome; worktreeRegistered compared unresolved paths, so on macOS it always returned false.
fix: address PR #1 review findings (includes a kill(-1) guard now missing from main)
1 participant
Milestone 1 — a real accepted run on one machine
First working slice of jig, a local-first software factory: a single Go binary that runs repeatable, phased coding-agent workflows against Git repositories. It merges the durable coordination model of an existing Go control plane (definitions → runs → jobs → attempts, leases, fail-closed worktree hygiene) with the phased execution model of disler's super-simple-software-factory (agent and code phases, claim-verifying gates, typed envelopes, live-session repair loops).
Plan:
docs/plans/2026-08-05-001-feat-jig-software-factory-plan.mdWhat landed
if:guards, limits table, Justfile with a mechanical package-boundary check, SHA-pinned CIjig runserverless direct harness with--json, smoke and two-phase example definitionsMilestone 1 exit gate — passed
Three consecutive accepted runs against the real Claude Code CLI (2.1.223, haiku):
Run 2's corrections were induced honestly at the engine level (a reporting template carrying a status literal the envelope contract rejects; a gate requirement stated only in the definition and never in any prompt) and both recovered inside the same live session. Two earlier forcing designs were defeated by the model spontaneously normalizing its claims — each defeat restarted the three-run count rather than being papered over.
Review and fixes
A seven-persona review (correctness, security, adversarial, testing, maintainability, reliability, agent-native) ran against the branch. It reproduced two failures and confirmed a containment bypass; all are fixed in
fix(review):with regression tests verified to fail beforehand:tests_passgate ran with jig's full environment and the operator's realHOME. Gate commands execute agent-authored code in the worktree, so ~60 inherited variables includingOPENAI_API_KEY,GITHUB_TOKEN, andSSH_AUTH_SOCKwere reachable. Gates now share the composed role env; a nil env is an error rather than silent inheritance.git mv docs/a.md secrets/a.mdfingerprinted as the literal token{docs => secrets}/a.md, so rollback failed with "pathspec did not match" and, under a**/*.mdallowlist, the file landed in a forbidden directory with zero breaches reported. Enumeration is now literal NUL-separated paths with--no-renames.runningwith no sweeper on the direct path, so every laterjig runfailed "nothing eligible" until~/.jigwas deleted. Fixed with signal handling, terminal-state recording, scratch destruction, and liveness-marker reclaim..gitand gitignored paths — a plantedpost-checkouthook actually fired during jig's own rollback. Now content-hashed fingerprints plus.git/hook coverage, and jig-side git runs with hooks and fsmonitor disabled.is_errorends a phase instead of burning the retry ladder.One reviewer finding was dropped as a false positive after direct verification: the adapter's
--toolsflag is real (claude --help).Testing
just checkandjust test-racegreen; the race suite now runs in CI on Linux and macOS. Live smoke against the real CLI re-run after the fixes: stillaccepted_unpublishedin 16.6s. Worker and control-plane tests run against real SQLite, real git repositories, and a real HTTP server rather than mocks.Known residuals
phase.gois 1115 lines and wants a three-way split; UUID minting, lease-token minting, andboundedTextare duplicated across packages, and two env-name extractors for the same wire field have behavioral drift (one dedupes and sorts). Both are churn-heavy refactors deliberately deferred rather than raced against three concurrent fix batches.snapshotTreecallsgit diff HEAD, which fails on a repository with no commits. Pre-existing; will bite the first definition run against an empty repo.node_modules. Documented tradeoff — content-hashing a build cache every phase costs more than the residual.jig runagainst a not-yet-created database can hitSQLITE_BUSYinOpen; wants a retry-on-busy.protocol.WorstCaseSendCountstill reads 6 and is referenced by no code; the engine enforces its own bound and carries a TODO naming the real figure.Post-Deploy Monitoring & Validation
No production or runtime impact — jig is a local-first developer tool with no deployed surface, and this branch has no consumers beyond its author. Validation is the exit gate above plus CI on both platforms. When dogfooding starts, the signals to watch are repair-budget burn per run (rising burn means adapter tuning, not engine bugs), retained-worktree ledger growth (fail-closed retention wedging a repo at its cap), and
jig run --jsonoutcome states.🤖 Generated with Claude Code
Summary by CodeRabbit
jig runfor direct local repository execution with human-readable or JSON results.