From 07603df8f256df03a84d3dfa550fa83858b5ffa1 Mon Sep 17 00:00:00 2001 From: Tom Boucher Date: Fri, 31 Jul 2026 14:45:51 -0400 Subject: [PATCH] fix(#2647): code-fixer worktree under .claude/worktrees/, not a hardcoded /tmp path (#2942) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * test(#2647): failing-first — fixer worktree path must be repo-relative not /tmp * fix(#2647): place code-fixer worktree under .claude/worktrees/, not /tmp The gsd-code-fixer agent hand-rolled its worktree at a hardcoded `/tmp/sv-${padded_phase}-reviewfix-XXXXXX` mktemp path. On Windows/Git Bash that landed OUTSIDE the project tree — outside the agent session's permission allowlist, so every Read inside the worktree prompted (~25/run) — and mktemp's MAX_PATH-avoidance substitute produced an un-removable `C:/mvwtNN` path. Place the worktree repo-relative under `.claude/worktrees/` (the same dir the harness-managed executor worktrees use: gitignored via `.claude/`, inside the session's permission scope), with a $$-PID + epoch suffix for concurrency uniqueness (replacing mktemp's XXXXXX). $main_repo is resolved the same way the cleanup tail already resolves it. Three sites updated: setup_worktree bash, concrete-steps prose, critical_rules. The #2990 `-b "$reviewfix_branch"` invariant is preserved (the folded test asserts it). Failing-first regression added to the #2990 suite in tests/agent-frontmatter.test.cjs. * test(#2647): update #2686 path assertion to expect .claude/worktrees/, not /tmp The #2686 regression test encoded the worktree location as a hardcoded `/tmp/sv-` path (matching sibling GSD agents at the time). #2647 showed that breaks Windows/Git Bash (worktree outside the project tree → permission prompts; mktemp MAX_PATH substitute un-removable). Update the #2686 path assertion to require the repo-relative `.claude/worktrees/` location and forbid `/tmp/sv-`. The #2686 isolation + cleanup assertions are unchanged. * fix(#2647): word-boundary wt= parse + ack the fixer growth vs next Two follow-ups to the #2647 GREEN run: - parseWtAssignments matched `prior_wt=` (no word boundary), polluting the set and tripping the repo-relative + concurrency-unique assertions. Anchor on (?:^|\s)wt= so only the real worktree-path assignment is captured. - emitted-attribution: gsd-code-fixer.md grew 1875 bytes vs origin/next. Update the emitted-drift-ack entry to attribute the #2647 worktree-path change (supersedes the prior #2825 attribution, whose growth is already in next). * fix(#2647): address review — validate padded_phase at the sink + tighten test Code-review + security-review both APPROVED with one actionable minor: padded_phase is interpolated into a worktree PATH and a git BRANCH NAME, but was only validated by the orchestrator (code-review-fix.md), not at the agent sink. The agent prompt is a literal bash contract any caller can spawn, so add a `[[ =~ ^[0-9]+(\.[0-9]+)?$ ]]` self-defense check rejecting traversal/shell metachars (defense-in-depth; not a present vuln — the only caller validates). Also tighten the concurrency-uniqueness test to require BOTH $$ AND $(date +%s) (either-alone was too lax per review). Update the emitted-drift-ack reason to cover the added validation growth. * changeset(#2647): code-fixer worktree under .claude/worktrees not /tmp * changeset(#2647): backfill PR number 2942 * chore(#2938): regenerate stale docs/CONTEXT-INDEX.json on next #2938 (#2928) updated the CONTEXT.md RULESET prose for the new per-PR emitted-drift-ack fragment mechanism (#2914) but shipped a CONTEXT-INDEX.json generated from the OLD prose. lint:generated-sync fails on every PR that rebases onto next after #2938 (the regen produces a 3-line diff bringing three RULESET entries — AGENT_SIZE_BUDGET, EMITTED_ATTRIBUTION, WORKFLOW_SIZE_BUDGET — in sync with the prose already on next). Mechanical regen via `node scripts/gen-context-index.cjs --write`; idempotent; surfaced by the #2647 rebase. No behavioral change. --------- Co-authored-by: sim --- .changeset/proud-jays-munch.md | 5 + agents/gsd-code-fixer.md | 30 ++++- docs/CONTEXT-INDEX.json | 6 +- tests/agent-frontmatter.test.cjs | 104 +++++++++++++++++- .../0000-legacy-migration.json | 2 +- 5 files changed, 134 insertions(+), 13 deletions(-) create mode 100644 .changeset/proud-jays-munch.md diff --git a/.changeset/proud-jays-munch.md b/.changeset/proud-jays-munch.md new file mode 100644 index 000000000..0763b7ff9 --- /dev/null +++ b/.changeset/proud-jays-munch.md @@ -0,0 +1,5 @@ +--- +type: Fixed +pr: 2942 +--- +**gsd-code-fixer no longer creates its review-fix worktree outside the project tree on Windows** — the worktree was hardcoded to a `/tmp/sv-...` mktemp path, which on Git Bash landed outside the repository (every file read inside it prompted for permission) and produced an un-removable short path. The worktree now lives repo-relative under `.claude/worktrees/`, the same location the executor worktrees use. (#2647) diff --git a/agents/gsd-code-fixer.md b/agents/gsd-code-fixer.md index 4126a6157..176078ad7 100644 --- a/agents/gsd-code-fixer.md +++ b/agents/gsd-code-fixer.md @@ -252,6 +252,17 @@ USE_WORKTREES=$(node -e ' branch=$(git branch --show-current) test -n "$branch" || { echo "Detached HEAD is not supported for review-fix (#2686)"; exit 1; } +# #2647 defense-in-depth: padded_phase is interpolated into a worktree PATH +# and a git BRANCH NAME below. The orchestrator (code-review-fix.md) already +# validates it as ^[0-9]+(\.[0-9]+)?$, but this agent prompt is a literal bash +# contract any caller can spawn — validate at the SINK too, so a future caller +# that forgets cannot turn ${padded_phase} into a path-traversal or branch-name +# injection. Reject anything that is not digits + an optional single dotted +# numeric suffix (e.g. '02' or '36.14'); reject '../', spaces, shell metachars. +if ! [[ "$padded_phase" =~ ^[0-9]+(\.[0-9]+)?$ ]]; then + echo "Invalid padded_phase for review-fix: '$padded_phase' (expected e.g. '02' or '36.14')"; exit 1 +fi + # 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`. @@ -303,7 +314,20 @@ if [ "$USE_WORKTREES" = "false" ]; then 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") + # #2647: create the worktree INSIDE the repo under the same `.claude/worktrees/` + # dir the harness-managed executor worktrees already use. An absolute `/tmp` + # path landed outside the project tree (outside the agent session's permission + # allowlist → every Read inside prompted; on Windows/Git Bash mktemp also + # produced an un-removable short `C:/mvwtNN` path to dodge MAX_PATH). A + # repo-relative path inherits the repository's existing permission scope, is + # valid and short on Windows as well as POSIX, and is covered by the single + # `.gitignore` rule for `.claude/` (`.gitignore:12`). Uniqueness across + # concurrent runs for the same phase comes from the PID (`$$`) + epoch suffix + # (replacing mktemp's XXXXXX). `$main_repo` is resolved the same way the + # cleanup tail below resolves it (`git worktree list --porcelain` first line). + main_repo="$(git worktree list --porcelain | awk '/^worktree / { sub(/^worktree /, ""); print; exit }')" + wt="$main_repo/.claude/worktrees/rf-${padded_phase}-$$-$(date +%s)" + mkdir -p "$wt" # 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 @@ -338,7 +362,7 @@ Concrete steps: 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. **Recovery check (#2839, #2990):** 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`), 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. -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. +4. Create a unique worktree path **inside the repo**: `main_repo="$(git worktree list --porcelain | awk '/^worktree / { sub(/^worktree /, ""); print; exit }')"` then `wt="$main_repo/.claude/worktrees/rf-${padded_phase}-$$-$(date +%s)"` + `mkdir -p "$wt"`. The path lives under the same `.claude/worktrees/` dir the harness-managed executor worktrees use (already gitignored via `.claude/`, already in the session's permission scope), and the `$$`-PID + epoch suffix ensures concurrent runs for the same phase do not collide (#2647 — an absolute `/tmp` path landed outside the project tree and prompted on every read). 5. Run `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. 6. **Write the recovery sentinel** at `${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. 7. All subsequent file reads, edits, and commits happen inside `$wt` (which is on `$reviewfix_branch`, not `$branch`). @@ -636,7 +660,7 @@ _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 -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 inside the isolated worktree** — set up via `branch=$(git branch --show-current)` + `main_repo="$(git worktree list --porcelain | awk '/^worktree / { sub(/^worktree /, ""); print; exit }')"` + `wt="$main_repo/.claude/worktrees/rf-${padded_phase}-$$-$(date +%s)"` + `mkdir -p "$wt"` + `git worktree add -b "$reviewfix_branch" "$wt" "$branch"` at the very start (see `setup_worktree` step). The worktree path is repo-relative under `.claude/worktrees/` (the same dir the harness-managed executor worktrees use — gitignored via `.claude/`, inside the session's permission scope); the `$$`-PID + epoch suffix ensures concurrent runs do not collide (#2647 — a hardcoded `/tmp` path landed outside the project tree and prompted on every read). 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). **#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 diff --git a/docs/CONTEXT-INDEX.json b/docs/CONTEXT-INDEX.json index 48303893c..7bcfb64b2 100644 --- a/docs/CONTEXT-INDEX.json +++ b/docs/CONTEXT-INDEX.json @@ -1677,7 +1677,7 @@ { "id": "RULESET.AGENT_SIZE_BUDGET", "klass": "RULESET", - "value": "agent-size-budget (#1074; sibling of WORKFLOW_SIZE_BUDGET; BYTES not lines per #717/#683, rebased from lines in PR 3/3) = differential attribution size ratchet (PRIMARY anti-creep since #2724/ADR-2719 §4, same mechanism and same tests/emitted-drift-ack.json as WORKFLOW_SIZE_BUDGET, scoped to agents/gsd-*.md) + loose tier hard caps (red lines, never raised on approach: XL<=57344 / LARGE<=49152 / DEFAULT<=24576); net-new agents are DEFAULT-tier (no separate new-file cap). Sizes are measured via the shared scripts/workflow-size.cjs measureMdFiles(dir,predicate) counter (tests/helpers/emitted-runtime.cjs's currentSizes() and the guard's own tier-cap checks both import it). A grown agent fails the differential guard — ack + justify, or extract LAZILY to gsd-core/references/. DISTINCT from DEFECT.AGENT-FILE-SIZE-CAP-BREACH (a separate 45K-CHAR extraction-evidence threshold on gsd-planner via planner-decomposition/reachability tests): that guard proves mode-sections were extracted; this one bounds total agent bytes. Two guards, two units (chars vs bytes), two purposes. The prior per-file baseline (tests/agent-size-baseline.json, `npm run size:baseline`) is REMOVED by #2724" + "value": "agent-size-budget (#1074; sibling of WORKFLOW_SIZE_BUDGET; BYTES not lines per #717/#683, rebased from lines in PR 3/3) = differential attribution size ratchet (PRIMARY anti-creep since #2724/ADR-2719 §4, same mechanism and same ack fragments (tests/emitted-drift-acks/, #2914; legacy tests/emitted-drift-ack.json still honored) as WORKFLOW_SIZE_BUDGET, scoped to agents/gsd-*.md) + loose tier hard caps (red lines, never raised on approach: XL<=57344 / LARGE<=49152 / DEFAULT<=24576); net-new agents are DEFAULT-tier (no separate new-file cap). Sizes are measured via the shared scripts/workflow-size.cjs measureMdFiles(dir,predicate) counter (tests/helpers/emitted-runtime.cjs's currentSizes() and the guard's own tier-cap checks both import it). A grown agent fails the differential guard — ack + justify, or extract LAZILY to gsd-core/references/. DISTINCT from DEFECT.AGENT-FILE-SIZE-CAP-BREACH (a separate 45K-CHAR extraction-evidence threshold on gsd-planner via planner-decomposition/reachability tests): that guard proves mode-sections were extracted; this one bounds total agent bytes. Two guards, two units (chars vs bytes), two purposes. The prior per-file baseline (tests/agent-size-baseline.json, `npm run size:baseline`) is REMOVED by #2724" }, { "id": "RULESET.ALLOWED-TOOLS-FRONTMATTER", @@ -1777,7 +1777,7 @@ { "id": "RULESET.EMITTED_ATTRIBUTION", "klass": "RULESET", - "value": "the emitted-artifact family (ADR-2719, epic #2719) — POST-CUTOVER (#2724, Phase 4). Historically tests/fixtures/golden-install-parity/*.json (19 path→hash manifests) + tests/workflow-size-baseline.json + tests/agent-size-baseline.json were all committed, PURE FUNCTIONS of the source tree whose correct merge was ALWAYS \"recompute\" — 140 of 143 conflicted-file instances across the open PR queue were these files. #2724 DELETES all three, the golden test (tests/golden-install-parity.test.cjs), the generator (scripts/gen-golden-install-parity-zcode.cjs), `npm run gen:golden`, `UPDATE_GOLDEN`, the merge-driver bridge (scripts/git-merge-regen-driver.cjs, `npm run setup:merge-driver`, the .gitattributes merge=gsd-regen block), and scripts/update-size-baseline.cjs (`npm run size:baseline`). The differential attribution check (tests/emitted-attribution.test.cjs + tests/emitted-provenance.test.cjs) is now the SOLE gate for emitted-artifact propagation AND size growth — no committed artifact, nothing to hand-merge, nothing to regenerate. `npm run regen:derived` still exists for what remains committed and derived: build, registry, ADR index, capability matrix, inventory manifest, manifest versions, and `tests/fixtures/install-tree/*.json` (now `npm run gen:install-tree`, folded into `regen:derived`). tests/fixtures/install-tree/*.json is DELIBERATELY EXCLUDED from the cutover (ADR-2719 §7): it conflicts on 0 of 7, its diffs are readable, and it preserves \"the installer stopped shipping X\" as a hard absolute failure — capturing it would convert that absolute into an attribution-free auto-resolve. The baseline the differential compares against is now published by `scripts/gen-emitted-baseline.cjs` on every push to `next` (cached, keyed on sha) and restored in PR lanes via `GSD_EMITTED_BASELINE`/`resolveBaseline()` (tests/helpers/emitted-baseline.cjs); a cache miss falls back to an in-job build via a throwaway `git worktree` (tests/helpers/emitted-runtime.cjs's `buildBaselineAtRef`). REMEDIATION IS PART OF THE GATE (#2778): the failure output names its own remedy, because a gate that states a requirement and withholds the means of satisfying it is a maintainer round-trip, not a gate — ADR-2719 §3's \"conspicuous declaration\" only works if the contributor can discover how to make it. Both failing branches name `tests/emitted-drift-ack.json`, say it may not exist yet (absence is the healthy steady state), print a minimal valid document, and repeat \"do NOT regenerate anything\" — post-#2724 there is nothing left to regenerate, and hunting for a deleted baseline is the predictable wrong guess. The two branches key on DIFFERENT spaces and each says which: the hash pass keys on the EMITTED PATH (always contains a `/`), the size ratchet keys on the BARE FILENAME (`currentSizes` writes `sizes[entry.name]` from readdirSync over `gsd-core/workflows/` + `agents/`). A stale-ack failure additionally says to delete the FILE when removing its last entry, since an empty-but-present ack parses fine yet signals nothing; post-#2789 it also offers CORRECTING the entry to name the ripple actually made, which is the other honest resolution and the one a contributor usually wants. NOT ack-able and deliberately given no ack text: the `NEW_FILE_CAP` branch, whose remedy is extraction. Text is sourced from one frozen `REMEDIATION` export in tests/helpers/emitted-diff.cjs whose example document is rendered from `ACK_VERSION` via `JSON.stringify`, so the taught schema cannot drift from the accepted one (a round-trip test feeds the printed document back through `parseAck`); the message teaches ONE canonical shape even though `parseAck` also accepts a bare-string reason and a missing `version` — liberal in what it accepts, conservative in what it sends. Note the ADR's Consequences originally called the #2724 migration \"terminal\"; #2778 corrected that — it is terminal only for a PR that grows no shipped file. cf `RULESET.WORKFLOW_SIZE_BUDGET`, `RULESET.AGENT_SIZE_BUDGET`; see `### Emitted Artifact Provenance`" + "value": "the emitted-artifact family (ADR-2719, epic #2719) — POST-CUTOVER (#2724, Phase 4). Historically tests/fixtures/golden-install-parity/*.json (19 path→hash manifests) + tests/workflow-size-baseline.json + tests/agent-size-baseline.json were all committed, PURE FUNCTIONS of the source tree whose correct merge was ALWAYS \"recompute\" — 140 of 143 conflicted-file instances across the open PR queue were these files. #2724 DELETES all three, the golden test (tests/golden-install-parity.test.cjs), the generator (scripts/gen-golden-install-parity-zcode.cjs), `npm run gen:golden`, `UPDATE_GOLDEN`, the merge-driver bridge (scripts/git-merge-regen-driver.cjs, `npm run setup:merge-driver`, the .gitattributes merge=gsd-regen block), and scripts/update-size-baseline.cjs (`npm run size:baseline`). The differential attribution check (tests/emitted-attribution.test.cjs + tests/emitted-provenance.test.cjs) is now the SOLE gate for emitted-artifact propagation AND size growth — no committed artifact, nothing to hand-merge, nothing to regenerate. `npm run regen:derived` still exists for what remains committed and derived: build, registry, ADR index, capability matrix, inventory manifest, manifest versions, and `tests/fixtures/install-tree/*.json` (now `npm run gen:install-tree`, folded into `regen:derived`). tests/fixtures/install-tree/*.json is DELIBERATELY EXCLUDED from the cutover (ADR-2719 §7): it conflicts on 0 of 7, its diffs are readable, and it preserves \"the installer stopped shipping X\" as a hard absolute failure — capturing it would convert that absolute into an attribution-free auto-resolve. The baseline the differential compares against is now published by `scripts/gen-emitted-baseline.cjs` on every push to `next` (cached, keyed on sha) and restored in PR lanes via `GSD_EMITTED_BASELINE`/`resolveBaseline()` (tests/helpers/emitted-baseline.cjs); a cache miss falls back to an in-job build via a throwaway `git worktree` (tests/helpers/emitted-runtime.cjs's `buildBaselineAtRef`). REMEDIATION IS PART OF THE GATE (#2778): the failure output names its own remedy, because a gate that states a requirement and withholds the means of satisfying it is a maintainer round-trip, not a gate — ADR-2719 §3's \"conspicuous declaration\" only works if the contributor can discover how to make it. Both failing branches name a NEW fragment to create under `tests/emitted-drift-acks/` (#2914; pick a name nobody else is using), say it may not exist yet (absence is the healthy steady state), print a minimal valid document, and repeat \"do NOT regenerate anything\" — post-#2724 there is nothing left to regenerate, and hunting for a deleted baseline is the predictable wrong guess. The two branches key on DIFFERENT spaces and each says which: the hash pass keys on the EMITTED PATH (always contains a `/`), the size ratchet keys on the BARE FILENAME (`currentSizes` writes `sizes[entry.name]` from readdirSync over `gsd-core/workflows/` + `agents/`). A stale-ack failure additionally says to delete the FILE when removing its last entry, since an empty-but-present ack parses fine yet signals nothing; post-#2789 it also offers CORRECTING the entry to name the ripple actually made, which is the other honest resolution and the one a contributor usually wants. NOT ack-able and deliberately given no ack text: the `NEW_FILE_CAP` branch, whose remedy is extraction. Text is sourced from one frozen `REMEDIATION` export in tests/helpers/emitted-diff.cjs whose example document is rendered from `ACK_VERSION` via `JSON.stringify`, so the taught schema cannot drift from the accepted one (a round-trip test feeds the printed document back through `parseAck`); the message teaches ONE canonical shape even though `parseAck` also accepts a bare-string reason and a missing `version` — liberal in what it accepts, conservative in what it sends. Note the ADR's Consequences originally called the #2724 migration \"terminal\"; #2778 corrected that — it is terminal only for a PR that grows no shipped file. #2914 replaced the single shared ack file with per-PR fragments under `tests/emitted-drift-acks/` — exactly the shape `.changeset/` already uses for the identical \"every PR rewrites one shared document\" conflict problem — so two PRs needing an ack can no longer collide with each other, and a fragment left on `next` after merge is inert rather than a shared cell; the legacy file is still read and unioned in for branches that predate the split, and a duplicate path key across two sources is a hard, loudly-reported error, never silent last-wins. `tests/emitted-drift-ack.json` (the LEGACY file specifically, NOT the fragment directory) must NEVER persist on `next` (#2914): every entry is scoped to the diff that introduced it, so once merged it is by definition already at the base — spent and inert regardless of shape — and a persistent copy makes that ONE file a shared merge-conflict cell across every open PR that also carries an ack, exactly the \"140 of 143\" cost this whole cutover exists to remove; a persisting FRAGMENT is harmless by construction and is deliberately not what this guard checks. This is enforced on `next` itself only, never as a PR-lane check: the `guard-no-ack-on-next` job in `.github/workflows/test.yml` (push-to-`next` trigger) runs `scripts/lint-emitted-drift-ack.cjs --guard-next` (`assertAbsentOnNext`), which fails on the LEGACY file's PRESENCE alone, valid or not — a PR-lane \"base ack must be absent\" check would red every open PR the instant a spent ack merged, which is the #2768 shape #2789 already ended. cf `RULESET.WORKFLOW_SIZE_BUDGET`, `RULESET.AGENT_SIZE_BUDGET`; see `### Emitted Artifact Provenance`" }, { "id": "RULESET.GH.AUTH.DEFAULT", @@ -1942,7 +1942,7 @@ { "id": "RULESET.WORKFLOW_SIZE_BUDGET", "klass": "RULESET", - "value": "workflow size enforcement (#1074; BYTES not lines per #717; LF-normalized per #683) = differential attribution size ratchet (PRIMARY anti-creep since #2724/ADR-2719 §4: tests/emitted-attribution.test.cjs's real-tree test reports growth in any gsd-core/workflows/*.md with its exact byte delta vs `next`, no committed snapshot, requires a tests/emitted-drift-ack.json entry) + loose tier hard caps (outer red lines, NEVER raised on approach: XL<=98304 / LARGE<=61440 / DEFAULT<=40960) + discuss-phase<32000; a file that grew fails the differential guard — add an ack entry naming the file and reason, justify the growth in the PR (or extract LAZILY-loaded content; eager @-imports don't reduce loaded context); crossing a hard cap means EXTRACT, not bump. The prior per-file baseline (tests/workflow-size-baseline.json, `npm run size:baseline`) is REMOVED by #2724. Its new-file cap (ADR-1610 Decision point 3, un-baselined files <=32768, the Codex anchor) is REVIVED inside the differential's size ratchet itself (`NEW_FILE_CAP` in tests/helpers/emitted-diff.cjs) rather than lost: \"not yet baselined\" is exactly \"present in sizeCurrent, absent from sizeBaseline\", a signal the ratchet already computes for its own reasons. NOT ack-able — same as the tier hard caps, the fix is extraction. Narrower than the original: this check cannot see XL/LARGE tiering (tests/workflow-size-budget.test.cjs's classification, invisible to the pure differential module), so a legitimately large NEW file must extract rather than tier in, one release earlier than an existing file would need to — a disclosed, deliberate simplification" + "value": "workflow size enforcement (#1074; BYTES not lines per #717; LF-normalized per #683) = differential attribution size ratchet (PRIMARY anti-creep since #2724/ADR-2719 §4: tests/emitted-attribution.test.cjs's real-tree test reports growth in any gsd-core/workflows/*.md with its exact byte delta vs `next`, no committed snapshot, requires an ack entry — a fragment under tests/emitted-drift-acks/, #2914; the legacy tests/emitted-drift-ack.json is still honored and unioned in) + loose tier hard caps (outer red lines, NEVER raised on approach: XL<=98304 / LARGE<=61440 / DEFAULT<=40960) + discuss-phase<32000; a file that grew fails the differential guard — add an ack entry naming the file and reason, justify the growth in the PR (or extract LAZILY-loaded content; eager @-imports don't reduce loaded context); crossing a hard cap means EXTRACT, not bump. The prior per-file baseline (tests/workflow-size-baseline.json, `npm run size:baseline`) is REMOVED by #2724. Its new-file cap (ADR-1610 Decision point 3, un-baselined files <=32768, the Codex anchor) is REVIVED inside the differential's size ratchet itself (`NEW_FILE_CAP` in tests/helpers/emitted-diff.cjs) rather than lost: \"not yet baselined\" is exactly \"present in sizeCurrent, absent from sizeBaseline\", a signal the ratchet already computes for its own reasons. NOT ack-able — same as the tier hard caps, the fix is extraction. Narrower than the original: this check cannot see XL/LARGE tiering (tests/workflow-size-budget.test.cjs's classification, invisible to the pure differential module), so a legitimately large NEW file must extract rather than tier in, one release earlier than an existing file would need to — a disclosed, deliberate simplification" }, { "id": "SESSION.2026-05-05", diff --git a/tests/agent-frontmatter.test.cjs b/tests/agent-frontmatter.test.cjs index a5a0e283b..cd0f3dc40 100644 --- a/tests/agent-frontmatter.test.cjs +++ b/tests/agent-frontmatter.test.cjs @@ -1288,6 +1288,89 @@ describe('Bug #2990: gsd-code-fixer worktree attaches to a NEW branch, not the u }); }); +/** + * Bug #2647: gsd-code-fixer hand-rolled its worktree at a hardcoded `/tmp/sv-...` + * mktemp path, which on Windows/Git Bash landed OUTSIDE the project tree — + * outside the agent session's permission allowlist, so every Read inside the + * worktree prompted — and mktemp's MAX_PATH-avoidance substitute produced an + * un-removable `C:/mvwtNN` path. The fix places the worktree repo-relative + * under `.claude/worktrees/` (the same dir the harness-managed executor + * worktrees use: gitignored via `.claude/`, inside the permission scope), + * with a PID+epoch suffix for concurrency uniqueness. + * + * These assertions parse the `wt=...` assignment(s) out of the agent markdown + * (source-text-is-the-product, same allow-test-rule as the #2990 block above) + * and assert the path is repo-relative under `.claude/worktrees/`, never an + * absolute `/tmp` path, and concurrency-unique. + */ +describe('Bug #2647: gsd-code-fixer worktree path is repo-relative, not a hardcoded /tmp path', () => { + const md = fs.readFileSync(AGENT_PATH, 'utf-8'); + + // Extract `wt=...` shell assignments from fenced bash blocks (skip inline + // backtick spans and bash comments, mirroring parseWorktreeAddInvocations). + function parseWtAssignments(markdown) { + const assigns = []; + const lines = markdown.split('\n'); + for (const line of lines) { + // Match `wt=` only as a whole token (preceded by start-of-line or + // whitespace) so `prior_wt=` and `${reviewfix_wt}`-style names are not + // captured. `\b` would also match the `wt` in `prior_wt`, so anchor on + // `(?:^|\s)wt=`. + const m = /(?:^|\s)wt=/.exec(line); + if (!m) continue; + const idx = m.index + m[0].length - 'wt='.length; + // Skip inline-code spans (odd backtick count before the match). + const before = line.slice(0, idx); + if ((before.match(/`/g) || []).length % 2 === 1) continue; + // Skip bash comments. + if (line.trimStart().startsWith('#')) continue; + // Skip `wt="."` (the use_worktrees=false opt-out — not a worktree path). + const value = line.slice(idx + 'wt='.length).trim(); + if (value === '"."' || value === "'.'") continue; + assigns.push(value); + } + return assigns; + } + + test('the worktree path is NOT an absolute /tmp path', () => { + const wtAssigns = parseWtAssignments(md); + assert.ok(wtAssigns.length > 0, 'expected at least one wt= assignment in gsd-code-fixer.md'); + const tmpViolations = wtAssigns.filter(v => /\/tmp\/sv-|mktemp -d "\/tmp\//.test(v)); + assert.deepEqual( + tmpViolations, + [], + `worktree paths still hardcoded to /tmp (#2647): ${JSON.stringify(tmpViolations, null, 2)}`, + ); + }); + + test('the worktree path is repo-relative under .claude/worktrees/', () => { + const wtAssigns = parseWtAssignments(md); + assert.ok(wtAssigns.length > 0, 'expected at least one wt= assignment in gsd-code-fixer.md'); + const nonRelative = wtAssigns.filter(v => !v.includes('.claude/worktrees/rf-')); + assert.deepEqual( + nonRelative, + [], + `worktree paths not under .claude/worktrees/ (#2647): ${JSON.stringify(nonRelative, null, 2)}`, + ); + }); + + test('the worktree path is concurrency-unique (PID + epoch suffix)', () => { + const wtAssigns = parseWtAssignments(md); + assert.ok(wtAssigns.length > 0, 'expected at least one wt= assignment in gsd-code-fixer.md'); + // The path must carry BOTH a PID (`$$`) AND a time component (`$(date +%s)`) + // so concurrent runs for the same phase do not collide — the property + // mktemp's XXXXXX provided before #2647. Requiring both (not either) is + // faithful to the fix: PID alone could recycle after wrap; epoch alone could + // collide for two same-second runs. + const nonUnique = wtAssigns.filter(v => !v.includes('$$') || !v.includes('$(date +%s)')); + assert.deepEqual( + nonUnique, + [], + `worktree paths lack the PID+epoch concurrency-uniqueness suffix (#2647): ${JSON.stringify(nonUnique, null, 2)}`, + ); + }); +}); + /** * Extract the cleanup-tail bash block from the agent .md, then parse it into * an ordered array of `git ...` invocation records. Per-record assertions go @@ -1496,16 +1579,25 @@ describe('bug-2686: review-fix agent worktree isolation', () => { ); }); - 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. + test('agent instructions use a repo-relative worktree path under .claude/worktrees/ (not a hardcoded /tmp path)', () => { + // #2647: the original #2686 fix placed the worktree at a hardcoded `/tmp/sv-` + // mktemp path to match sibling GSD agents. On Windows/Git Bash that landed + // OUTSIDE the project tree — outside the agent session's permission allowlist, + // so every Read inside the worktree prompted — and mktemp's MAX_PATH-avoidance + // substitute produced an un-removable `C:/mvwtNN` path. The worktree must now + // be repo-relative under `.claude/worktrees/` (the same dir the harness-managed + // executor worktrees use: gitignored via `.claude/`, inside the permission + // scope), and the agent must NOT define a `/tmp/sv-` worktree path. 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)' + !hasTmpWorktreePath, + 'gsd-code-fixer.md must NOT define a worktree variable at a /tmp/sv-... path (#2647 — use a repo-relative .claude/worktrees/ path instead)' + ); + assert.ok( + /\.claude\/worktrees\//.test(agentContent), + 'gsd-code-fixer.md must define the worktree under .claude/worktrees/ (repo-relative, gitignored, inside the permission scope) (#2647)' ); }); }); diff --git a/tests/emitted-drift-acks/0000-legacy-migration.json b/tests/emitted-drift-acks/0000-legacy-migration.json index a05e13c8b..a26296f09 100644 --- a/tests/emitted-drift-acks/0000-legacy-migration.json +++ b/tests/emitted-drift-acks/0000-legacy-migration.json @@ -34,7 +34,7 @@ "agents/gsd-ui-researcher.toml": "#2834: Codex agent TOML now carries model-routing fields on first install.", "agents/gsd-user-profiler.toml": "#2834: Codex agent TOML now carries model-routing fields on first install.", "agents/gsd-verifier.toml": "#2834: Codex agent TOML now carries model-routing fields on first install.", - "gsd-code-fixer.md": "#2825: setup_worktree now reads workflow.use_worktrees and gates git worktree add on it (skipping worktree creation when the user opted out), the cleanup tail is gated to a no-op in that mode, and the spec adds three safety guardrails — honor the opt-out, never rm -rf a possible Windows reparse point/junction (the delete-the-target path that wiped real node_modules), and record where verification ran. Growth is the gated bash branch + the three guardrail paragraphs.", + "gsd-code-fixer.md": "#2647: the three worktree-path sites (setup_worktree bash, concrete-steps prose, critical_rules) replaced the hardcoded /tmp/sv- mktemp path with a repo-relative .claude/worktrees/rf--- path, and added a defense-in-depth padded_phase validation at the sink (the agent prompt is a literal bash contract any caller can spawn; the orchestrator validates upstream but the sink now self-defends against path-traversal/branch-name injection). On Windows/Git Bash the /tmp path landed outside the project tree (outside the session permission allowlist, prompting on every read) and mktemp's MAX_PATH substitute was un-removable; .claude/worktrees/ is the same dir the harness-managed executor worktrees use (gitignored via .claude/, inside the permission scope). Growth is the path-resolution bash (main_repo via `git worktree list --porcelain | awk`) + the $$-PID/epoch uniqueness replacing mktemp's XXXXXX + the padded_phase guard + the #2647 rationale comments at each site. Supersedes the prior #2825 attribution, whose gated-bash + guardrail growth is already in next.", "spec-phase.md": { "reason": "#2733: five transitions in gsd-core/workflows/spec-phase.md were re-pointed so control reaches the mandatory Step 5.5 edge-completeness and Step 5.6 prohibition-completeness probes, which no path could reach before. Four upstream gate-passed jumps went from 'Jump to Step 6' to 'Jump to Step 5.5', and Step 5.5's own terminal soft gate at :305 went from 'proceed to Step 6' to 'proceed to Step 5.6' so the common all-edges-resolved path stops skipping the prohibition probe. The +10 bytes is exactly those five targets growing by 2 bytes each ('Step 6' -> 'Step 5.5' / 'Step 5.6'); it is the literal fix, not incidental prose growth, and cannot be avoided without leaving a probe unreachable. Verified: 31987 -> 31997 bytes, DEFAULT tier, cap 40960." }