fix(#2686): review-fix agent uses isolated git worktree, prevents main-tree race (#2705)

* fix(#2686): review-fix agent now uses git worktree for isolation

The gsd-code-fixer agent operated directly against the main working tree,
racing any concurrent foreground session for HEAD, the index, and on-disk
files. Added a setup_worktree step (git worktree add /tmp/sv-N-reviewfix
HEAD) as the first action before any file operations, with unconditional
git worktree remove cleanup on exit. Mirrors the pattern used by all other
GSD per-issue agents.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

* fix(#2686): address CodeRabbit review — mktemp unique path, branch-aware worktree, tighten test assertions

- Use mktemp -d for unique worktree path (prevents concurrent-run collision)
- Resolve branch via git branch --show-current before worktree add (prevents detached HEAD)
- Error-and-exit on worktree add failure instead of force-removing shared path
- Test: use .exec().index for checkout position (not indexOf on match string)
- Test: match gsd-sdk query commit as well as git commit for ordering assertion
- Test: tighten /tmp path assertion to require actual /tmp/sv- assignment

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

---------

Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com>

agents/gsd-code-fixer.md

@@ -209,6 +209,39 @@ If a finding references multiple files (in Fix section or Issue section):
 
 <execution_flow>
 
+<step name="setup_worktree">
+**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 `<config>` 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.
+</step>
+
 <step name="load_context">
 **1. Read mandatory files:** Load all files from `<required_reading>` block if present.
 
@@ -437,6 +470,8 @@ _Iteration: {N}_
 
 <critical_rules>
 
+**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.

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-<n>.
+ */
+
+'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 <branch>` (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)'
+    );
+  });
+});