golem15/msd-core
4
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

fix(#2825): gsd-code-fixer honors workflow.use_worktrees; never rm -rf a reparse point (#2905)

Browse Source 
* fix(#2825): gsd-code-fixer honors workflow.use_worktrees; never rm -rf a reparse point

gsd-code-fixer was the only writer that hand-rolled a git worktree inside the
agent prompt and the only one that never read workflow.use_worktrees. With the
setting explicitly false, --fix still created worktrees; the fresh worktree had
no node_modules, so runs improvised a teardown whose `rm -rf` followed a Windows
junction into the REAL node_modules (silent data loss, 3x observed).

Defect 1: gate setup_worktree + its cleanup tail on workflow.use_worktrees
(same gsd_run query config-get read the four sibling workflows use). When false:
edit/commit in the main checkout (wt='.', no temp branch, no sentinel, no cleanup).

Defect 2: forbid rm -rf on a possible reparse point in the spec — never fall
through to a destructive remove; on failure, stop and surface the error.

Defect 3: REVIEW-FIX records where verification ran (main checkout vs worktree).

The transactional worktree path (#2839/#2990/#2686) is unchanged when worktrees
are enabled. Docs-parity guards in tests/code-review.test.cjs bind the spec to
the fix.

* chore(#2825): backfill changeset PR 2905

* fix(#2825): drop stale agents/gsd-code-fixer.toml ack + spent entries (emitted-attribution)

gsd-test failed: agents/gsd-code-fixer.toml is a STALE ack here — that TOML was
changed by #2834 (now in next), not this PR. The 4 base-carried acks
(autonomous/discuss-phase-assumptions/next/plan-phase) are spent/inert.

---------

Co-authored-by: Test <test@example.com>
(cherry picked from commit 5d0fd4dc53)
This commit is contained in:
Tom Boucher
2026-07-31 01:57:50 -04:00
committed by sim
parent dc73680532
commit 3aabb0f441
4 changed files with 175 additions and 144 deletions
Download Patch File Download Diff File Expand all files Collapse all files

131
agents/gsd-code-fixer.md
View File

@@ -216,9 +216,36 @@ 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.
**#2825: honor `workflow.use_worktrees`.** This is the ONLY writer that hand-rolls a git worktree
inside the agent prompt; every other writer path (`/gsd:execute-phase`, `/gsd:execute-plan`,
`/gsd:quick`, `/gsd:diagnose-issues`) reads `workflow.use_worktrees` and skips isolation when it is
`false`. Read the same flag here and, when it is `false`, edit and commit in the main checkout
directly (set `wt="."`, no `reviewfix_branch`, no recovery sentinel, no `git worktree add`, and skip
the cleanup tail — there is no worktree to remove). When the flag is not `false`, the transactional
worktree path below runs unchanged. A user who explicitly opted out of worktrees must never have a
worktree created; the hand-rolled worktree also cannot run the project's gates safely (no
`node_modules`), so the opt-out is also the safe path.
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
# #2825: honor workflow.use_worktrees — the documented opt-out. When false,
# edit/commit in the main checkout (wt=".", no temp branch, no sentinel, no
# cleanup tail). Read the flag the same way the four sibling writer workflows
# do. NOTE: this read parses .planning/config.json directly via `node` rather
# than the gsd-tools CLI, because setup_worktree runs BEFORE the canonical
# launcher preamble is sourced — invoking the CLI here would be undefined at
# runtime and violates the runtime-launcher-parity preamble-ordering rule.
# Once the preamble is sourced (later steps), the CLI is available.
USE_WORKTREES=$(node -e '
try {
const fs = require("fs");
const p = (process.env.GSD_PROJECT_DIR || process.cwd()) + "/.planning/config.json";
const cfg = JSON.parse(fs.readFileSync(p, "utf8"));
process.stdout.write(String((cfg.workflow && cfg.workflow.use_worktrees) ?? true));
} catch { process.stdout.write("true"); }
')
# 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:
@@ -264,34 +291,47 @@ if [ -f "$sentinel" ]; then
rm -f "$sentinel"
fi
wt=$(mktemp -d "/tmp/sv-${padded_phase}-reviewfix-XXXXXX")
# #2825: when the user opted out of worktrees, edit/commit in the main
# checkout directly — no temp branch, no sentinel, no cleanup tail. This is
# the safe path: the hand-rolled worktree has no node_modules, so it cannot
# run the project's gates, and an improvised teardown can destroy the real
# node_modules on Windows (a junction followed by rm -rf). wt="." means every
# downstream read/edit/commit lands in the main working tree, and the cleanup
# tail below is a no-op (nothing to fast-forward, no worktree to remove).
if [ "$USE_WORKTREES" = "false" ]; then
wt="."
reviewfix_branch="$branch"
echo "workflow.use_worktrees=false — editing/committing in the main checkout (no worktree)."
else
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"
# 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"
# 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"
cd "$wt"
fi
```
Concrete steps:
@@ -305,9 +345,18 @@ Concrete steps:
**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:
**Cleanup tail (transactional, ALWAYS — even on failure — when a worktree was created):** After writing REVIEW-FIX.md and before returning to the orchestrator, run the cleanup in this exact order. (When `workflow.use_worktrees` is `false`, no worktree was created — the cleanup is a no-op and the bash below early-exits.)
```bash
# #2825: when worktrees were disabled, there is nothing to clean up — the
# agent edited/committed on $branch directly in the main checkout (wt=".",
# reviewfix_branch==$branch, no sentinel, no temp worktree). Skip the whole
# tail; the four steps below are all no-ops or harmful (e.g. `git worktree
# remove "."` ) in that mode.
if [ "$USE_WORKTREES" = "false" ]; then
exit 0
fi
# 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
@@ -354,7 +403,7 @@ fi
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.
This cleanup is unconditional when a worktree was created — 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. (When `workflow.use_worktrees` is `false`, no worktree exists and the bash above early-exits before these steps.) 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>
<step name="load_context">
@@ -587,9 +636,33 @@ _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 -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 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 when done (treat it as a finally block) — but only when a worktree was actually created; when `workflow.use_worktrees` is `false` the cleanup early-exits (no worktree to remove). 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.
**#2825 — honor `workflow.use_worktrees`.** Before creating a worktree, read the
`workflow.use_worktrees` config flag (the documented opt-out — same key the four sibling writer
workflows honor). `setup_worktree` reads it via `node` directly from `.planning/config.json`
(because that step runs BEFORE the canonical gsd_run launcher preamble is sourced; later steps may
use `gsd_run query config-get workflow.use_worktrees`). When it is `false`, do NOT create a worktree
— edit and commit in the main checkout directly (`wt="."`, no temp branch, no sentinel, no cleanup
tail). A user who opted out of worktrees must
never have one created. See the `setup_worktree` step for the gated bash.
**NEVER `rm -rf` a possible reparse point** (#2825). On Windows, `node_modules` inside the worktree
may be a junction/reparse point whose target is the REAL `node_modules` in the main checkout — and
`rm -rf` follows the link and deletes the target's contents (silent, misdiagnosable data loss). Do
NOT improvise a `node_modules` teardown. The worktree has no `node_modules` by design; if you need
the project's gates, run them in the main checkout after the fast-forward, OR leave the worktree's
dependency handling to `git worktree remove` (which does not recurse into a separately-managed
link). Never use `rm -rf` (or `2>/dev/null || rm -rf || true`) as a fallback for removing a path
that might be a reparse point — on failure, STOP and surface the error rather than falling through
to a destructive remove.
**Record where verification ran** (#2825). The REVIEW-FIX.md verification section must state whether
the gates ran in the main checkout or the isolated worktree, so a reader can tell whether the numbers
are reproducible from the tree they are looking at (a worktree-env run is not reproducible from the
main checkout after teardown).
**ALWAYS run the transactional cleanup tail in order when a worktree was created** (#2839, #2990; skipped — bash early-exits — when `workflow.use_worktrees` is `false`): 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.