diff --git a/agents/gsd-code-fixer.md b/agents/gsd-code-fixer.md index 7d1ac154e..f6dfa4075 100644 --- a/agents/gsd-code-fixer.md +++ b/agents/gsd-code-fixer.md @@ -214,32 +214,89 @@ If a finding references multiple files (in Fix section or Issue section): 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. +The cleanup tail (commit fixes -> remove worktree -> drop recovery sentinel) MUST be **transactional**: either all of (worktree, branch advance, sentinel) end in a clean state, or — if the process is interrupted (system restart, OOM kill) between the last commit and `git worktree remove` — a discoverable recovery sentinel is left behind so a future run, `/gsd-resume-work`, or `/gsd-progress` can complete the cleanup. The bug fixed by #2839 was that the cleanup tail was non-transactional and silently left orphan worktrees + unmerged branches with no resume marker. + ```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; } + +# Recovery-sentinel handling (#2839): +# Path is ${phase_dir}/.review-fix-recovery-pending.json. If it already exists, +# a previous run was interrupted between fix commits and `git worktree remove`. +# The pre-existing sentinel records the orphan worktree_path, branch, and +# padded_phase so this run can complete recovery before starting fresh. +sentinel="${phase_dir}/.review-fix-recovery-pending.json" +if [ -f "$sentinel" ]; then + echo "Detected pre-existing recovery sentinel from a prior interrupted run: $sentinel" + prior_wt=$(node -e ' + const fs = require("fs"); + try { + const parsed = JSON.parse(fs.readFileSync(process.argv[1], "utf-8")); + process.stdout.write(parsed.worktree_path || ""); + } catch (err) { + process.stderr.write(`Warning: malformed recovery sentinel ${process.argv[1]}: ${err.message}\n`); + process.stdout.write(""); + } + ' "$sentinel") + if [ -n "$prior_wt" ] && git worktree list --porcelain | grep -q "^worktree $prior_wt$"; then + echo "Removing orphan worktree from prior run: $prior_wt" + git worktree remove "$prior_wt" --force || true + fi + rm -f "$sentinel" +fi + wt=$(mktemp -d "/tmp/sv-${padded_phase}-reviewfix-XXXXXX") git worktree add "$wt" "$branch" + +# Write the recovery sentinel ONLY AFTER `git worktree add` succeeds. +# Writing it before would leave a sentinel pointing at a worktree that does +# not exist if `git worktree add` itself failed. +node -e ' + const fs = require("fs"); + const [sentinelPath, worktree_path, branch, padded_phase] = process.argv.slice(1); + fs.writeFileSync(sentinelPath, JSON.stringify({ + worktree_path, + branch, + padded_phase, + started_at: new Date().toISOString() + }, null, 2)); +' "$sentinel" "$wt" "$branch" "$padded_phase" + cd "$wt" ``` Concrete steps: -1. Parse `padded_phase` from the `` block (needed for the path). +1. Parse `padded_phase` and `phase_dir` from the `` block (needed for the path and for the sentinel location). 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`. +3. **Recovery check (#2839):** If `${phase_dir}/.review-fix-recovery-pending.json` already exists, a prior run was interrupted. Parse the JSON, attempt to remove the orphan worktree it points at (best-effort, with `--force`), then delete the stale sentinel before continuing. This makes a re-run of `/gsd-code-review-fix` self-healing. +4. 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. +5. Run `git worktree add "$wt" "$branch"` — this attaches the worktree to the current branch so commits advance it. +6. **Write the recovery sentinel** at `${phase_dir}/.review-fix-recovery-pending.json` containing `{worktree_path, branch, padded_phase, started_at}`. Doing this AFTER `git worktree add` ensures the sentinel only ever points at a real worktree. +7. 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. +**If `git worktree add` fails**, surface the error and exit — do not force-remove the path, as another concurrent run may be holding it. Do not write the sentinel (the worktree does not exist). + +**Cleanup tail (transactional, ALWAYS — even on failure):** After writing REVIEW-FIX.md and before returning to the orchestrator, run the two-step cleanup in this exact order: -**Cleanup (ALWAYS — even on failure):** After writing REVIEW-FIX.md and before returning to the orchestrator, run: ```bash +# Step 1: drop the worktree FIRST. If this succeeds and the process is then +# killed, the next run finds a sentinel pointing at a worktree that no longer +# exists — the recovery branch handles this gracefully (best-effort remove + +# sentinel delete). If we reversed the order (sentinel removed first, then +# worktree remove), an interruption between the two steps would leave NO +# sentinel and an orphan worktree — exactly the bug from #2839. git worktree remove "$wt" --force + +# Step 2: drop the recovery sentinel ONLY after `git worktree remove` returns +# successfully. This atomic-ish ordering is what makes the cleanup tail +# transactional from the orchestrator's perspective. +rm -f "$sentinel" ``` -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. +This cleanup is unconditional — register it mentally as a finally-block obligation. If the agent exits early (config error, no findings, etc.), still run the two-step cleanup tail (`git worktree remove "$wt" --force` followed by `rm -f "$sentinel"`) before exit. The sentinel must NEVER be removed before `git worktree remove` succeeds. @@ -473,6 +530,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 run the transactional cleanup tail in order** (#2839): `git worktree remove "$wt" --force` MUST happen BEFORE `rm -f "$sentinel"` (the recovery sentinel at `${phase_dir}/.review-fix-recovery-pending.json`). The sentinel is written AFTER `git worktree add` succeeds and removed only AFTER `git worktree remove` returns successfully. This ordering is what makes the cleanup tail transactional — an interruption between commits and `git worktree remove` leaves the sentinel behind so a future run, `/gsd-resume-work`, or `/gsd-progress` can detect and complete the recovery. Reversing the order recreates the orphan-worktree bug. + **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-2839-review-fix-transactional-cleanup.test.cjs b/tests/bug-2839-review-fix-transactional-cleanup.test.cjs new file mode 100644 index 000000000..1a6aa9cd2 --- /dev/null +++ b/tests/bug-2839-review-fix-transactional-cleanup.test.cjs @@ -0,0 +1,181 @@ +/** + * Regression test for bug #2839 + * + * /gsd-code-review-fix cleanup tail is non-transactional. If the agent is + * interrupted (system restart, OOM kill) AFTER the last fix commit but + * BEFORE `git worktree remove`, the worktree is orphaned in + * `git worktree list`, the agent's branch is left with unmerged commits, + * and STATE.md is never advanced. To anyone reading main only, the phase + * looks "ready to plan" while critical fixes sit on a dangling branch. + * + * Fix: introduce a recovery sentinel JSON at + * ${PHASE_DIR}/.review-fix-recovery-pending.json + * The sentinel is written AFTER `git worktree add` succeeds and + * REMOVED only after `git worktree remove` completes, so the cleanup + * tail is transactional from the orchestrator's perspective. If the + * process dies in between, the sentinel is left behind pointing at the + * orphan worktree and branch — a future run, /gsd-resume-work, or + * /gsd-progress can detect and complete the recovery. + */ + +'use strict'; + +// allow-test-rule: source-text-is-the-product +// The gsd-code-fixer agent's working instructions ARE the product — Claude +// follows them at runtime. Structural assertions over the markdown source +// test the deployed contract. See bug-2686 for the same pattern. + +const { describe, test, before } = require('node:test'); +const assert = require('node:assert/strict'); +const fs = require('fs'); +const path = require('path'); + +const SENTINEL_NAME = '.review-fix-recovery-pending.json'; + +function parseFrontmatter(content) { + const match = content.match(/^---\n([\s\S]*?)\n---/); + if (!match) return null; + const body = match[1]; + const out = {}; + for (const line of body.split('\n')) { + const m = line.match(/^([a-zA-Z_]+):\s*(.*)$/); + if (m) out[m[1]] = m[2].trim(); + } + return out; +} + +function extractStep(content, stepName) { + const re = new RegExp(`([\\s\\S]*?)`); + const m = content.match(re); + return m ? m[1] : null; +} + +describe('bug-2839: /gsd-code-review-fix cleanup is transactional', () => { + let agentPath; + let agentContent; + let frontmatter; + + before(() => { + 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'); + frontmatter = parseFrontmatter(agentContent); + assert.ok(frontmatter, 'agent must have YAML frontmatter'); + }); + + test('agent declares a recovery sentinel filename', () => { + assert.ok( + agentContent.includes(SENTINEL_NAME), + `gsd-code-fixer.md must reference the recovery sentinel ${SENTINEL_NAME} so an interrupted cleanup tail is discoverable (#2839)` + ); + }); + + test('sentinel is written inside setup_worktree, after git worktree add', () => { + const setupStep = extractStep(agentContent, 'setup_worktree'); + assert.ok(setupStep, 'setup_worktree step must exist'); + + assert.ok( + setupStep.includes(SENTINEL_NAME), + `setup_worktree must reference ${SENTINEL_NAME} so the sentinel is created at the start of the run (#2839)` + ); + + const addPos = setupStep.indexOf('git worktree add'); + assert.ok(addPos !== -1, 'setup_worktree must contain `git worktree add`'); + + // The sentinel WRITE (not just a reference) must come after `git worktree add`. + // Earlier references are allowed (e.g. recovery check for a stale sentinel + // from a prior interrupted run). Look for an explicit write — either a + // shell `>`/`>>` redirection, a `node -e` invocation that uses + // `fs.writeFileSync(...sentinel...)`, or a `Write` tool reference. + const writeIdx = (() => { + const candidates = [ + /fs\.writeFileSync\([^)]*sentinel/, + />\s*"?\$sentinel/, + />\s*"?\$\{sentinel\}/, + /Write the recovery sentinel/i, + ]; + let earliest = -1; + for (const re of candidates) { + const m = re.exec(setupStep); + if (m && (earliest === -1 || m.index < earliest)) earliest = m.index; + } + return earliest; + })(); + assert.ok( + writeIdx !== -1, + 'setup_worktree must explicitly describe writing the sentinel (#2839)' + ); + assert.ok( + addPos < writeIdx, + 'sentinel must be written AFTER `git worktree add` succeeds (#2839)' + ); + }); + + test('sentinel records worktree path, branch, and padded_phase as JSON fields', () => { + for (const key of ['worktree_path', 'branch', 'padded_phase']) { + assert.ok( + agentContent.includes(key), + `recovery sentinel must record \`${key}\` so a future /gsd-resume-work or /gsd-progress can locate the orphan state (#2839)` + ); + } + }); + + test('sentinel removal happens only AFTER git worktree remove succeeds', () => { + const setupStep = extractStep(agentContent, 'setup_worktree'); + assert.ok(setupStep, 'setup_worktree step must exist'); + + const cleanupAnchor = setupStep.lastIndexOf('Cleanup tail (transactional'); + assert.ok(cleanupAnchor !== -1, 'setup_worktree must document cleanup-tail section'); + const cleanupSection = setupStep.slice(cleanupAnchor); + + const removeIdx = cleanupSection.indexOf('git worktree remove "$wt" --force'); + assert.ok(removeIdx !== -1, 'cleanup-tail must remove worktree'); + + // Within the cleanup-tail section, accept either a literal-filename form + // (`rm -f .../.review-fix-recovery-pending.json`) or a shell-variable form + // referring to the previously-declared `sentinel` variable + // (`rm -f "$sentinel"` / `rm -f "${sentinel}"`). + const escapedName = SENTINEL_NAME.replace(/\./g, '\\.'); + const sentinelRemovalRe = new RegExp( + `(rm\\s+(?:-f\\s+)?[^\\n]*(?:${escapedName}|\\$\\{?sentinel\\}?)|unlink[^\\n]*(?:${escapedName}|\\$\\{?sentinel\\}?))` + ); + const sentinelRemovalMatch = sentinelRemovalRe.exec(cleanupSection); + assert.ok( + sentinelRemovalMatch, + `agent must remove the sentinel file (rm or unlink ${SENTINEL_NAME}) as part of the cleanup tail (#2839)` + ); + const sentinelRemovalIdx = sentinelRemovalMatch.index; + + assert.ok( + removeIdx < sentinelRemovalIdx, + 'cleanup ordering must be: `git worktree remove` BEFORE sentinel removal (#2839)' + ); + }); + + test('agent documents detection of pre-existing sentinel from a prior interrupted run', () => { + const lower = agentContent.toLowerCase(); + const mentionsRecovery = + lower.includes('stale sentinel') || + lower.includes('existing sentinel') || + lower.includes('previous sentinel') || + lower.includes('prior run') || + lower.includes('pre-existing sentinel') || + lower.includes('recovery'); + assert.ok( + mentionsRecovery, + 'agent must describe how it handles a pre-existing sentinel from a previous interrupted run (#2839)' + ); + }); + + test('cleanup-tail obligation is documented as transactional / atomic', () => { + const lower = agentContent.toLowerCase(); + const mentionsTransactional = + lower.includes('transactional') || + lower.includes('atomic cleanup') || + lower.includes('cleanup tail'); + assert.ok( + mentionsTransactional, + 'agent must document the cleanup tail as transactional/atomic (#2839)' + ); + }); +});