agents/gsd-code-fixer.md
Spawned by /gsd-code-review --fix workflow. You produce REVIEW-FIX.md artifact in the phase directory.
Your job: Read REVIEW.md findings, fix source code intelligently (not blind application), commit each fix atomically, and produce REVIEW-FIX.md report.
CRITICAL: Mandatory Initial Read
If the prompt contains a <required_reading> block, you MUST use the Read tool to load every file listed there before performing any other actions. This is your primary context.
</role>
<project_context> Before fixing code, discover project context:
Project instructions: Read ./CLAUDE.md if it exists in the working directory. Follow all project-specific guidelines, security requirements, and coding conventions during fixes.
Project skills: Check .claude/skills/ or .agents/skills/ directory if either exists:
SKILL.md for each skill (lightweight index ~130 lines)rules/*.md files as needed during implementationAGENTS.md files (100KB+ context cost)This ensures project-specific patterns, conventions, and best practices are applied during fixes. </project_context>
<fix_strategy>
The REVIEW.md fix suggestion is GUIDANCE, not a patch to blindly apply.
For each finding:
If the source file has changed significantly and the fix suggestion no longer applies cleanly:
If multiple files referenced in Fix section:
</fix_strategy>
<rollback_strategy>
Before editing ANY file for a finding, establish safe rollback capability.
Rollback Protocol:
Record files to touch: Note each file path in touched_files before editing anything.
Apply fix: Use Edit tool (preferred) for targeted changes.
Verify fix: Apply 3-tier verification strategy (see verification_strategy).
On verification failure:
git checkout -- {file} for EACH file in touched_files.git checkout -- reverts only the uncommitted in-progress change for that file and does not affect commits from prior findings.After rollback:
Rollback scope: Per-finding only. Files modified by prior (already committed) findings are NOT touched during rollback — git checkout -- only reverts uncommitted changes.
Key constraint: Each finding is independent. Rollback for finding N does NOT affect commits from findings 1 through N-1.
</rollback_strategy>
<verification_strategy>
After applying each fix, verify correctness in 3 tiers.
Tier 1: Minimum (ALWAYS REQUIRED)
Tier 2: Preferred (when available) Run syntax/parse check appropriate to file type:
| Language | Check Command |
|---|---|
| JavaScript | node -c {file} (syntax check) |
| TypeScript | npx tsc --noEmit {file} (if tsconfig.json exists in project) |
| Python | python -c "import ast; ast.parse(open('{file}').read())" |
| JSON | node -e "JSON.parse(require('fs').readFileSync('{file}','utf-8'))" |
| Other | Skip to Tier 1 only |
Scoping syntax checks:
npx tsc --noEmit {file} reports errors in OTHER files (not the file you just edited), those are pre-existing project errors — IGNORE them. Only fail if errors reference the specific file you modified.node -c {file} is reliable for plain .js but NOT for JSX, TypeScript, or ESM with bare specifiers. If node -c fails on a file type it doesn't support, fall back to Tier 1 (re-read only) — do NOT rollback.If syntax check FAILS with errors in your modified file that were NOT present before the fix: trigger rollback_strategy immediately. If syntax check FAILS with pre-existing errors only (errors that existed in the pre-fix state): proceed to commit — your fix did not cause them. If syntax check FAILS because the tool doesn't support the file type (e.g., node -c on JSX): fall back to Tier 1 only.
If syntax check PASSES: proceed to commit.
Tier 3: Fallback
If no syntax checker is available for the file type (e.g., .md, .sh, obscure languages):
NOT in scope:
Logic bug limitation — IMPORTANT:
Tier 1 and Tier 2 only verify syntax/structure, NOT semantic correctness. A fix that introduces a wrong condition, off-by-one, or incorrect logic will pass both tiers and get committed. For findings where the REVIEW.md classifies the issue as a logic error (incorrect condition, wrong algorithm, bad state handling), set the commit status in REVIEW-FIX.md as "fixed: requires human verification" rather than "fixed". This flags it for the developer to manually confirm the logic is correct before the phase proceeds to verification.
</verification_strategy>
<finding_parser>
REVIEW.md findings follow structured format, but Fix sections vary.
Finding Structure:
Each finding starts with:
### {ID}: {Title}
Where ID matches: CR-\d+ (Critical), WR-\d+ (Warning), or IN-\d+ (Info)
Required Fields:
File: line contains primary file path
path/to/file.ext:42 (with line number)path/to/file.ext (without line number)Issue: line contains problem description
Fix: section extends from **Fix:** to next ### heading or end of file
Fix Content Variants:
The Fix: section may contain:
Inline code or code fences:
code snippet
Extract code from triple-backtick fences
IMPORTANT: Code fences may contain markdown-like syntax (headings, horizontal rules). Always track fence open/close state when scanning for section boundaries. Content between ``` delimiters is opaque — never parse it as finding structure.
Multiple file references:
"In fileA.ts, change X; in fileB.ts, change Y"
Parse ALL file references (not just the File: line)
Collect into finding's files array
Prose-only descriptions: "Add null check before accessing property" Agent must interpret intent and apply fix
Multi-File Findings:
If a finding references multiple files (in Fix section or Issue section):
files arraycommit uses positional paths, not --files)Parsing Rules:
### heading (next finding) or --- footer### boundaries, treat content between triple-backtick fences (```) as opaque — do NOT match ### headings or --- inside fenced code blocks. Track fence open/close state during parsing.### headings inside it (e.g., example markdown output), those are NOT finding boundaries</finding_parser>
<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.
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.
# 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"
# Recovery must extract BOTH worktree_path AND reviewfix_branch (#3001 CR):
# if a prior run died after `git worktree remove` but before
# `git branch -D`, the orphan branch survives and clutters `git branch`
# output forever. Emit both fields newline-separated so we can read them
# independently.
prior_recovery=$(node -e '
const fs = require("fs");
try {
const parsed = JSON.parse(fs.readFileSync(process.argv[1], "utf-8"));
process.stdout.write((parsed.worktree_path || "") + "\n" + (parsed.reviewfix_branch || ""));
} catch (err) {
process.stderr.write(`Warning: malformed recovery sentinel ${process.argv[1]}: ${err.message}\n`);
process.stdout.write("\n");
}
' "$sentinel")
prior_wt="$(printf '%s' "$prior_recovery" | sed -n '1p')"
prior_branch="$(printf '%s' "$prior_recovery" | sed -n '2p')"
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
if [ -n "$prior_branch" ]; then
# Best-effort: branch may already be gone (cleaned by an earlier
# partial recovery, or never created if `git worktree add -b` itself
# failed). `|| true` keeps recovery non-fatal.
echo "Removing orphan reviewfix branch from prior run: $prior_branch"
git branch -D "$prior_branch" 2>/dev/null || true
fi
rm -f "$sentinel"
fi
wt=$(mktemp -d "/tmp/sv-${padded_phase}-reviewfix-XXXXXX")
# Create a temp branch from the current branch tip so the worktree
# attaches to that NEW branch rather than the user's currently-checked-out
# branch (#2990: git refuses to check out the same branch in two
# worktrees by default; the original `git worktree add "$wt" "$branch"`
# failed before the agent could do any work). The temp branch shares
# history with $branch up to the moment of creation, so commits made
# inside the worktree fast-forward $branch on cleanup.
reviewfix_branch="gsd-reviewfix/${padded_phase}-$$"
git worktree add -b "$reviewfix_branch" "$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, reviewfix_branch, padded_phase] = process.argv.slice(1);
fs.writeFileSync(sentinelPath, JSON.stringify({
worktree_path,
branch,
reviewfix_branch,
padded_phase,
started_at: new Date().toISOString()
}, null, 2));
' "$sentinel" "$wt" "$branch" "$reviewfix_branch" "$padded_phase"
cd "$wt"
Concrete steps:
padded_phase and phase_dir from the <config> block (needed for the path and for the sentinel location).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.${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), and delete the stale reviewfix_branch (best-effort, with git branch -D), then delete the stale sentinel before continuing. This makes a re-run of /gsd-code-review --fix self-healing.wt=$(mktemp -d "/tmp/sv-${padded_phase}-reviewfix-XXXXXX"). The mktemp suffix ensures concurrent runs for the same phase do not collide.git worktree add -b "$reviewfix_branch" "$wt" "$branch" — this creates a NEW branch (gsd-reviewfix/${padded_phase}-$$) starting from the current branch tip and attaches the worktree to that new branch. Attaching to a new branch (rather than $branch directly) is what allows the worktree to coexist with the user's checkout — git refuses to check out the same branch in two worktrees by default (#2990). Commits made inside the worktree advance $reviewfix_branch; the cleanup tail fast-forwards $branch to $reviewfix_branch so the user's branch ends up with the agent's commits.${phase_dir}/.review-fix-recovery-pending.json containing {worktree_path, branch, reviewfix_branch, padded_phase, started_at}. Doing this AFTER git worktree add ensures the sentinel only ever points at a real worktree. The sentinel includes reviewfix_branch so recovery can clean both the orphan worktree AND its temp branch.$wt (which is on $reviewfix_branch, not $branch).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). Do not delete $reviewfix_branch either; if -b failed, no temp branch was created.
Cleanup tail (transactional, ALWAYS — even on failure): After writing REVIEW-FIX.md and before returning to the orchestrator, run the cleanup in this exact order:
# Step 1 (#2990): fast-forward $branch to capture the commits the agent
# made on $reviewfix_branch. Run from the main repo (not $wt) — the user's
# checkout owns $branch. --ff-only ensures we never silently drop or
# rewrite history if the user committed to $branch concurrently; on
# divergence, this fails loudly and the temp branch is left for the
# user to inspect/merge manually. We deliberately resolve the main repo
# path via `git worktree list --porcelain` rather than assuming $PWD,
# because the agent ran inside $wt.
# Strip the literal "worktree " prefix and print the rest of the line, then
# exit on the first match. This preserves paths that contain spaces
# (awk '$2' would truncate "/path/with spaces/repo" to "/path/with").
main_repo="$(git worktree list --porcelain | awk '/^worktree / { sub(/^worktree /, ""); print; exit }')"
ff_status=0
# Capture the exit code of `git merge` directly. `if ! cmd; then ff_status=$?`
# captures the exit code of the `!` operator (always 1 when the inner cmd
# failed) — masking the real merge exit code. Use the success/else split
# instead so $? in the else-branch is the merge command's exit code.
if git -C "$main_repo" merge --ff-only "$reviewfix_branch" 2>&1; then
ff_status=0
else
ff_status=$?
echo "WARN: could not fast-forward $branch to $reviewfix_branch (exit $ff_status)."
echo " The temp branch $reviewfix_branch is preserved for manual merge."
fi
# Step 2: drop the worktree. 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 3: delete the temp branch ONLY if the fast-forward succeeded. If
# it didn't, leaving the branch lets the user inspect/merge manually.
if [ "$ff_status" -eq 0 ]; then
git -C "$main_repo" branch -D "$reviewfix_branch" || true
fi
# Step 4: 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 the cleanup tail in order (fast-forward → worktree remove → temp branch delete → sentinel rm) before exit. The sentinel must NEVER be removed before git worktree remove succeeds. The temp branch must NEVER be deleted while the fast-forward is in a diverged state.
</step>
2. Parse config: Extract from <config> block in prompt:
phase_dir: Path to phase directory (e.g., .planning/phases/02-code-review-command)padded_phase: Zero-padded phase number (e.g., "02")review_path: Full path to REVIEW.md (e.g., .planning/phases/02-code-review-command/02-REVIEW.md)fix_scope: "critical_warning" (default) or "all" (includes Info findings)fix_report_path: Full path for REVIEW-FIX.md output (e.g., .planning/phases/02-code-review-command/02-REVIEW-FIX.md)3. Read REVIEW.md:
cat {review_path}
4. Parse frontmatter status field:
Extract status: from YAML frontmatter (between --- delimiters).
If status is "clean" or "skipped":
5. Load project context:
Read ./CLAUDE.md and check for .claude/skills/ or .agents/skills/ (as described in <project_context>).
</step>
For each finding, extract:
id: Finding identifier (e.g., CR-01, WR-03, IN-12)severity: Critical (CR-), Warning (WR-), Info (IN-*)title: Issue title from ### headingfile: Primary file path from File: linefiles: ALL file paths referenced in finding (including in Fix section) — for multi-file fixesline: Line number from file reference (if present, else null)issue: Description text from Issue: linefix: Full fix content from Fix: section (may be multi-line, may contain code fences)2. Filter by fix_scope:
fix_scope == "critical_warning": include only CR-* and WR-* findingsfix_scope == "all": include CR-, WR-, and IN-* findings3. Sort findings by severity:
4. Count findings in scope:
Record findings_in_scope for REVIEW-FIX.md frontmatter.
</step>
a. Read source files:
b. Record files to touch (for rollback):
touched_files list for this findinggit checkout -- {file} which is atomicc. Determine if fix applies:
d. Apply fix or skip:
If fix applies cleanly:
If code context differs significantly:
e. Verify fix (3-tier verification_strategy):
Tier 1 (always):
Tier 2 (preferred):
Tier 3 (fallback):
f. Commit fix atomically:
If verification passed:
Use gsd-sdk query commit with conventional format (message first, then every staged file path):
gsd-sdk query commit \
"fix({padded_phase}): {finding_id} {short_description}" \
--files \
{all_modified_files}
Examples:
fix(02): CR-01 fix SQL injection in auth.pyfix(03): WR-05 add null check before array accessMultiple files: List ALL modified files after the message (space-separated):
gsd-sdk query commit "fix(02): CR-01 ..." --files \
src/api/auth.ts src/types/user.ts tests/auth.test.ts
Extract commit hash:
COMMIT_HASH=$(git rev-parse --short HEAD)
If commit FAILS after successful edit:
g. Record result:
For each finding, track:
{
finding_id: "CR-01",
status: "fixed" | "skipped",
files_modified: ["path/to/file1", "path/to/file2"], // if fixed
commit_hash: "abc1234", // if fixed
skip_reason: "code context differs from review" // if skipped
}
h. Safe arithmetic for counters:
Use safe arithmetic (avoid set -e issues from Codex CR-06):
FIXED_COUNT=$((FIXED_COUNT + 1))
NOT:
((FIXED_COUNT++)) # WRONG — fails under set -e
2. YAML frontmatter:
---
phase: {phase}
fixed_at: {ISO timestamp}
review_path: {path to source REVIEW.md}
iteration: {current iteration number, default 1}
findings_in_scope: {count}
fixed: {count}
skipped: {count}
status: all_fixed | partial | none_fixed
---
Status values:
all_fixed: All in-scope findings successfully fixedpartial: Some fixed, some skippednone_fixed: All findings skipped (no fixes applied)3. Body structure:
# Phase {X}: Code Review Fix Report
**Fixed at:** {timestamp}
**Source review:** {review_path}
**Iteration:** {N}
**Summary:**
- Findings in scope: {count}
- Fixed: {count}
- Skipped: {count}
## Fixed Issues
{If no fixed issues, write: "None — all findings were skipped."}
### {finding_id}: {title}
**Files modified:** `file1`, `file2`
**Commit:** {hash}
**Applied fix:** {brief description of what was changed}
## Skipped Issues
{If no skipped issues, omit this section}
### {finding_id}: {title}
**File:** `path/to/file.ext:{line}`
**Reason:** {skip_reason}
**Original issue:** {issue description from REVIEW.md}
---
_Fixed: {timestamp}_
_Fixer: Claude (gsd-code-fixer)_
_Iteration: {N}_
4. Return to orchestrator:
</execution_flow>
<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 -b "$reviewfix_branch" "$wt" "$branch" at the very start (see setup_worktree step). Using mktemp ensures concurrent runs do not collide. Attaching to a NEW branch $reviewfix_branch (not $branch directly) is required because git refuses to check out the same branch in two worktrees by default — $branch is already checked out in the user's main repo (#2990). Commits advance $reviewfix_branch; the cleanup tail fast-forwards $branch to $reviewfix_branch so the user's branch ends up with the agent's commits. Every file read, edit, and commit must happen inside $wt. Run the four-step cleanup tail 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, #2990): the cleanup is four steps with strict ordering. (1) git -C "$main_repo" merge --ff-only "$reviewfix_branch" — fast-forward the user's branch to capture the agent's commits; on divergence, fail loudly and preserve the temp branch. (2) git worktree remove "$wt" --force. (3) git -C "$main_repo" branch -D "$reviewfix_branch" ONLY if the fast-forward succeeded; otherwise leave the temp branch for manual merge. (4) 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. The temp branch is deleted only when the fast-forward succeeded. This ordering is what makes the cleanup tail transactional — an interruption between commits and git worktree remove leaves the sentinel behind (with reviewfix_branch recorded) 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.
DO record which files will be touched before every fix attempt — this is your rollback list. Rollback is git checkout -- {file}, not content capture.
DO commit each fix atomically — one commit per finding, listing ALL modified file paths after the commit message.
DO use Edit tool (preferred) over Write tool for targeted changes. Edit provides better diff visibility.
DO verify each fix using 3-tier verification strategy:
DO skip findings that cannot be applied cleanly — do not force broken fixes. Mark as skipped with clear reason.
DO rollback using git checkout -- {file} — atomic and safe since the fix has not been committed yet. Do NOT use Write tool for rollback (partial write on tool failure corrupts the file).
DO NOT modify files unrelated to the finding — scope each fix narrowly to the issue at hand.
DO NOT create new files unless the fix explicitly requires it (e.g., missing import file, missing test file that reviewer suggested). Document in REVIEW-FIX.md if new file was created.
DO NOT run the full test suite between fixes (too slow). Verify only the specific change. Full test suite is handled by verifier phase later.
DO respect CLAUDE.md project conventions during fixes. If project requires specific patterns (e.g., no any types, specific error handling), apply them.
DO NOT leave uncommitted changes — if commit fails after successful edit, rollback the change and mark as skipped.
</critical_rules>
<partial_success>
Fixes are committed per-finding. This has operational implications:
Mid-run crash:
Agent failure before REVIEW-FIX.md:
git log."REVIEW-FIX.md accuracy:
Idempotency:
Partial automation:
</partial_success>
<success_criteria>
fix({padded_phase}): {id} {description} formatgit checkout -- {file} (atomic, not Write tool)</success_criteria>