diff --git a/agents/gsd-code-fixer.md b/agents/gsd-code-fixer.md index 523f7f025..d856748e9 100644 --- a/agents/gsd-code-fixer.md +++ b/agents/gsd-code-fixer.md @@ -209,6 +209,39 @@ If a finding references multiple files (in Fix section or Issue section): + +**Isolation: create a dedicated git worktree BEFORE touching any files.** + +This agent runs as a background process that makes commits. Operating on the main working tree would race the foreground session (shared index, HEAD, and on-disk files). Instead, every instance runs in its own isolated worktree. + +```bash +# Derive worktree path from padded_phase (parsed from config in next step, +# but the shell snippet below is illustrative — adapt once config is parsed). +# In practice: parse padded_phase from config first, then run: +branch=$(git branch --show-current) +test -n "$branch" || { echo "Detached HEAD is not supported for review-fix (#2686)"; exit 1; } +wt=$(mktemp -d "/tmp/sv-${padded_phase}-reviewfix-XXXXXX") +git worktree add "$wt" "$branch" +cd "$wt" +``` + +Concrete steps: +1. Parse `padded_phase` from the `` block (needed for the path). +2. Resolve the current branch: `branch=$(git branch --show-current)`. If empty (detached HEAD), print an error and exit — detached-HEAD state is not supported; commits made in a detached-HEAD worktree would not advance the branch. +3. Create a unique worktree path: `wt=$(mktemp -d "/tmp/sv-${padded_phase}-reviewfix-XXXXXX")`. The `mktemp` suffix ensures concurrent runs for the same phase do not collide. +4. Run `git worktree add "$wt" "$branch"` — this attaches the worktree to the current branch so commits advance it. +5. All subsequent file reads, edits, and commits happen inside `$wt`. + +**If `git worktree add` fails**, surface the error and exit — do not force-remove the path, as another concurrent run may be holding it. + +**Cleanup (ALWAYS — even on failure):** After writing REVIEW-FIX.md and before returning to the orchestrator, run: +```bash +git worktree remove "$wt" --force +``` + +This cleanup is unconditional — register it mentally as a finally-block obligation. If the agent exits early (config error, no findings, etc.), still run `git worktree remove "$wt" --force` before exit. + + **1. Read mandatory files:** Load all files from `` block if present. @@ -437,6 +470,8 @@ _Iteration: {N}_ +**ALWAYS run inside the isolated worktree** — set up via `branch=$(git branch --show-current)` + `wt=$(mktemp -d "/tmp/sv-${padded_phase}-reviewfix-XXXXXX")` + `git worktree add "$wt" "$branch"` at the very start (see `setup_worktree` step). Using `mktemp` ensures concurrent runs do not collide. Attaching to `$branch` (not `HEAD`) ensures commits advance the branch. Every file read, edit, and commit must happen inside `$wt`. Run `git worktree remove "$wt" --force` unconditionally when done (treat it as a finally block). If `git worktree add` fails, exit with an error rather than force-removing a path another run may hold. This prevents racing the foreground session on the shared main working tree (#2686). + **ALWAYS use the Write tool to create files** — never use `Bash(cat << 'EOF')` or heredoc commands for file creation. **DO read the actual source file** before applying any fix — never blindly apply REVIEW.md suggestions without understanding current code state. diff --git a/tests/bug-2686-review-fix-worktree.test.cjs b/tests/bug-2686-review-fix-worktree.test.cjs new file mode 100644 index 000000000..0542e36dc --- /dev/null +++ b/tests/bug-2686-review-fix-worktree.test.cjs @@ -0,0 +1,91 @@ +/** + * Regression test for bug #2686 + * + * The gsd-code-fixer agent (spawned by /gsd-code-review-fix) operated directly + * against the main working tree. When it ran concurrently with a foreground + * session both processes raced for HEAD, the index, and on-disk files. The + * foreground session's next commit could land on the wrong branch (whichever + * branch the agent last checked out). + * + * Fix: the agent's working instructions must include `git worktree add` as the + * FIRST git operation, run ALL subsequent git operations inside that worktree + * path, and call `git worktree remove` for cleanup when done. + * + * This mirrors the pattern already used by every other per-issue GSD agent at + * /private/tmp/sv-. + */ + +'use strict'; + +// allow-test-rule: source-text-is-the-product +// The gsd-code-fixer agent's working instructions ARE the product — Claude +// executes them literally at runtime. Testing the text content tests the +// deployed contract: if the instruction is absent, the isolation guarantee +// is absent. + +const { describe, test, before } = require('node:test'); +const assert = require('node:assert/strict'); +const fs = require('fs'); +const path = require('path'); + +describe('bug-2686: review-fix agent worktree isolation', () => { + let agentContent; + + before(() => { + const agentPath = path.join(__dirname, '..', 'agents', 'gsd-code-fixer.md'); + assert.ok(fs.existsSync(agentPath), 'agents/gsd-code-fixer.md must exist'); + agentContent = fs.readFileSync(agentPath, 'utf-8'); + }); + + test('agent instructions include git worktree add before any branch-switching checkout or commit', () => { + const worktreePos = agentContent.indexOf('git worktree add'); + + assert.ok( + worktreePos !== -1, + 'gsd-code-fixer.md must include a "git worktree add" instruction to isolate operations from the main working tree (#2686)' + ); + + // `git checkout -- {file}` is a file-restore within the worktree — safe, not a branch switch. + // The dangerous operation is `git checkout ` (no leading --). + // Find the first branch-switching checkout (pattern: "git checkout " NOT followed by "--"). + const branchCheckoutMatch = /git checkout (?!--)/.exec(agentContent); + if (branchCheckoutMatch) { + const branchCheckoutPos = branchCheckoutMatch.index; + assert.ok( + worktreePos < branchCheckoutPos, + 'git worktree add must appear before any branch-switching git checkout in the agent instructions' + ); + } + + // commit command must come after worktree setup — the fixer may use + // either `git commit` directly or `gsd-sdk query commit` + const commitMatch = /(?:git commit|gsd-sdk query commit)/.exec(agentContent); + if (commitMatch) { + const commitPos = commitMatch.index; + assert.ok( + worktreePos < commitPos, + 'git worktree add must appear before any commit command in the agent instructions' + ); + } + }); + + test('agent instructions include worktree cleanup after completion', () => { + assert.ok( + agentContent.includes('git worktree remove') || agentContent.includes('worktree remove'), + 'gsd-code-fixer.md must include worktree cleanup (git worktree remove) to avoid leaking tmp directories (#2686)' + ); + }); + + test('agent instructions use a /tmp path for the worktree', () => { + // Require either a literal /tmp/sv- path or a variable assignment to /tmp/sv- + // (e.g. `wt=$(mktemp -d "/tmp/sv-..."`). Bare `$wt` or `wt=` references + // without a /tmp/sv- assignment are not sufficient. + const hasTmpWorktreePath = + /\/tmp\/sv-/.test(agentContent) || + /\bwt\s*=\s*["']?\/tmp\/sv-/.test(agentContent); + assert.ok( + hasTmpWorktreePath, + 'gsd-code-fixer.md must define a worktree variable at a /tmp/sv-... path, consistent with other GSD agents (#2686)' + ); + }); +});