* fix(#2839): make /gsd-code-review-fix cleanup transactional Cleanup tail in agents/gsd-code-fixer.md previously did 'git worktree remove' without any recovery marker. If the process was killed between fix commits and worktree removal, the orphan worktree + branch survived with no resume path — the next run had no way to discover or finish the cleanup. Introduce a recovery sentinel at ${phase_dir}/.review-fix-recovery-pending.json with strict ordering: - Sentinel written AFTER 'git worktree add' succeeds (never points at a worktree that does not exist). - Sentinel removed ONLY AFTER 'git worktree remove' returns successfully (interruption between commits and removal leaves a sentinel behind). - New runs detect a pre-existing sentinel, force-remove the recorded orphan worktree, then drop the stale sentinel before continuing — making the agent self-healing after a crash. Closes #2839 * fix(#2839): harden sentinel JSON parse and scope ordering assertion Address CodeRabbit review feedback on PR #2846: - agents/gsd-code-fixer.md: Guard the recovery-sentinel JSON parse with try/catch so a corrupted/truncated sentinel (a realistic crash artifact) emits a warning and yields an empty prior_wt instead of aborting setup. This preserves the self-healing recovery path even when the sentinel itself is the casualty of the original crash. - tests/bug-2839-review-fix-transactional-cleanup.test.cjs: Scope the cleanup-ordering assertion to the cleanup-tail section of the setup_worktree step rather than first global occurrences. Previously the assertion could pass on pre-recovery references even if cleanup-tail ordering regressed. The regex also now accepts the shell-variable form (\`rm -f \"\$sentinel\"\`) used in the cleanup tail. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.7 <noreply@anthropic.com>
This commit is contained in:
Tom Boucher
@@ -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 `<config>` block (needed for the path).
|
||||
1. Parse `padded_phase` and `phase_dir` from the `<config>` 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.
|
||||
</step>
|
||||
|
||||
<step name="load_context">
|
||||
@@ -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.
|
||||
|
||||
181
tests/bug-2839-review-fix-transactional-cleanup.test.cjs
Normal file
181
tests/bug-2839-review-fix-transactional-cleanup.test.cjs
Normal file
@@ -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(`<step\\s+name="${stepName}">([\\s\\S]*?)</step>`);
|
||||
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)'
|
||||
);
|
||||
});
|
||||
});
|
||||
Reference in New Issue
Block a user