From 8b6c44433f598716417832e7dc04d07dbe1ce443 Mon Sep 17 00:00:00 2001 From: Tom Boucher Date: Mon, 27 Apr 2026 12:31:32 -0400 Subject: [PATCH] fix(#2772): only disable worktree isolation when planned paths touch submodules (#2779) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(#2772): only disable worktree isolation when planned paths touch submodules The previous guard in execute-phase.md and quick.md unconditionally set USE_WORKTREES=false whenever .gitmodules existed, penalising every plan in a submodule project even when no plan touched a submodule path. Replace with submodule-path parsing + per-plan path intersection: - Parse SUBMODULE_PATHS once from .gitmodules via `git config --file .gitmodules --get-regexp '^submodule\..*\.path$'`. - In execute-phase.md, intersect SUBMODULE_PATHS with each plan's files_modified frontmatter; disable worktree isolation only for plans with non-empty intersection. Fall back to safe-disable for that plan when files_modified is missing/unparseable, with a log line explaining why. - In quick.md (no pre-declared paths), keep submodule-path parsing and document a fail-loud commit-time guard so the executor aborts only when it actually stages a submodule path. Add tests/bug-2772-gitmodules-path-intersection.test.cjs covering both files: no unconditional disable, submodule paths are parsed, intersection logic exists in execute-phase, fallback path is documented. Full suite: 5680 / 5680 pass. Closes #2772 * test(#2772): replace source-grep with behavioral test of submodule path intersection * fix(#2772): wire USE_WORKTREES_FOR_PLAN into dispatch + fix glob matcher + add quick.md commit guard Address CodeRabbit review on PR #2779 — the original fix computed USE_WORKTREES_FOR_PLAN but never read it, so the per-plan submodule intersection was dead code. Dispatch sites still branched on the project-level USE_WORKTREES. Changes: 1. execute-phase.md (CRITICAL — dispatch wiring): Move per-plan computation into execute_waves as sub-step 2.5, run it for each plan before its dispatch, and gate all four dispatch sites on USE_WORKTREES_FOR_PLAN: worktree-mode header, sequential-mode header, "worktrees disabled" sequential rule, and post-wave cleanup. Document PLAN_FILES extraction via jq from the phase-plan-index JSON. Track WAVE_WORKTREE_PLANS so post-wave cleanup only runs when at least one plan in the wave actually used worktrees. 2. Per-plan gate matcher (MAJOR — glob safety): Strip leading "./" and trailing "/" from both submodule and planned paths. Match bidirectionally (pf inside sm AND sm inside pf). Handle globby planned paths like "vendor/**/*.c" by extracting the literal prefix before the first glob metachar and re-checking. Wrap the iteration in set -f / set +f so glob expansion does not corrupt patterns. Extracted the gate (~92 lines) into workflows/execute-phase/steps/per-plan-worktree-gate.md to keep execute-phase.md under the 1700-line XL budget. 3. quick.md (CRITICAL — fail-loud guard): Inject SUBMODULE_PATHS into the executor Task prompt and add a bash block the executor must run before every git commit. The guard inspects staged paths via `git diff --cached --name-only`, normalizes paths, and aborts with a clear ABORT message + recovery instruction ("re-run with workflow.use_worktrees=false") when any staged path falls inside a submodule. 4. tests/bug-2772-gitmodules-path-intersection.test.cjs: 25 tests total. Updated GATE_SNIPPET to match the new bash matcher. Added normalization tests (./ prefix, trailing /, glob "vendor/**/*.c", parent directory, ./ in .gitmodules). Added workflow-markdown wiring assertions for all 4 dispatch sites + per-plan gate file extraction. Added quick.md guard tests: prompt injection assertion + behavioral fixture-repo tests that stage a submodule path and assert the guard exits non-zero with the ABORT message. Test count: 5701 pass / 0 fail (was 5698/1 before). Co-Authored-By: Claude Opus 4.7 (1M context) --------- Co-authored-by: Claude Opus 4.7 (1M context) --- get-shit-done/workflows/execute-phase.md | 37 +- .../steps/per-plan-worktree-gate.md | 94 +++ get-shit-done/workflows/quick.md | 50 +- ...2772-gitmodules-path-intersection.test.cjs | 563 ++++++++++++++++++ 4 files changed, 729 insertions(+), 15 deletions(-) create mode 100644 get-shit-done/workflows/execute-phase/steps/per-plan-worktree-gate.md create mode 100644 tests/bug-2772-gitmodules-path-intersection.test.cjs diff --git a/get-shit-done/workflows/execute-phase.md b/get-shit-done/workflows/execute-phase.md index ff2410723..695aaaae8 100644 --- a/get-shit-done/workflows/execute-phase.md +++ b/get-shit-done/workflows/execute-phase.md @@ -84,16 +84,21 @@ Read worktree config: USE_WORKTREES=$(gsd-sdk query config-get workflow.use_worktrees 2>/dev/null || echo "true") ``` -If the project uses git submodules, worktree isolation is skipped regardless of the `workflow.use_worktrees` config — the executor commit protocol cannot correctly handle submodule commits inside isolated worktrees. Sequential execution handles submodules transparently. +If the project uses git submodules, worktree isolation is unsafe **only when a plan touches a submodule path** — the executor commit protocol cannot correctly handle submodule commits inside isolated worktrees. The previous behavior unconditionally disabled worktree isolation whenever `.gitmodules` existed, which penalised every plan in a submodule project even when the plan was nowhere near a submodule. Compute submodule paths once and intersect them per-plan with the plan's declared `files_modified` frontmatter. ```bash +# Parse submodule paths from .gitmodules once (empty if no .gitmodules). +# SUBMODULE_PATHS is a newline-separated list of repo-relative paths. if [ -f .gitmodules ]; then - echo "[worktree] Submodule project detected (.gitmodules exists) — falling back to sequential execution" - USE_WORKTREES=false + SUBMODULE_PATHS=$(git config --file .gitmodules --get-regexp '^submodule\..*\.path$' 2>/dev/null | awk '{print $2}') +else + SUBMODULE_PATHS="" fi ``` -When `USE_WORKTREES` is `false`, all executor agents run without `isolation="worktree"` — they execute sequentially on the main working tree instead of in parallel worktrees. +`SUBMODULE_PATHS` is exported to the `execute_waves` step, where the per-plan decision actually happens (see "Per-plan worktree decision" sub-step inside `execute_waves`). The decision is per-plan because different plans in the same wave can touch different files — only plans whose paths intersect a submodule must drop worktree isolation; plans nowhere near a submodule keep parallel isolation. + +When `USE_WORKTREES` (project-level) is `false`, all executor agents run without `isolation="worktree"` — they execute sequentially on the main working tree instead of in parallel worktrees. The per-plan decision below has no effect when worktrees are project-disabled. Read context window size for adaptive prompt enrichment: @@ -418,6 +423,12 @@ increases monotonically across waves. `{status}` is `complete` (success), - Bad: "Executing terrain generation plan" - Good: "Procedural terrain generator using Perlin noise — creates height maps, biome zones, and collision meshes. Required before vehicle physics can interact with ground." +2.5. **Per-plan worktree decision (run for each plan in this wave BEFORE its dispatch):** + + Read and execute `get-shit-done/workflows/execute-phase/steps/per-plan-worktree-gate.md` for each plan. It extracts `PLAN_FILES` from the plan's JSON, intersects against `SUBMODULE_PATHS` (with normalization, bidirectional matching, and glob-prefix handling), and sets `USE_WORKTREES_FOR_PLAN` to `false` when the plan touches a submodule path. Append `plan_id` to a `WAVE_WORKTREE_PLANS` accumulator when `USE_WORKTREES_FOR_PLAN != false`. + + The dispatch branches in step 3 below MUST gate on `USE_WORKTREES_FOR_PLAN` for the current plan, not on the project-level `USE_WORKTREES`. + 3. **Spawn executor agents:** **Emit a plan-start heartbeat (literal line, no tool call) immediately before @@ -431,7 +442,7 @@ increases monotonically across waves. `{status}` is `complete` (success), For 200k models, this keeps orchestrator context lean (~10-15%). For 1M+ models (Opus 4.6, Sonnet 4.6), richer context can be passed directly. - **Worktree mode** (`USE_WORKTREES` is not `false`): + **Worktree mode** (`USE_WORKTREES_FOR_PLAN` is not `false` — evaluated per-plan in step 2.5): Before spawning, capture the current HEAD: ```bash @@ -562,7 +573,7 @@ increases monotonically across waves. `{status}` is `complete` (success), > **ORCHESTRATOR RULE — CODEX RUNTIME**: After calling Task() above to spawn executor agent(s), stop working on this task immediately. Do not read more files, edit code, or run tests related to this task while the subagent is active. Wait for the subagent to return its result. This prevents duplicate work, conflicting edits, and wasted context. Only resume when the subagent result is available. - **Sequential mode** (`USE_WORKTREES` is `false`): + **Sequential mode** (`USE_WORKTREES_FOR_PLAN` is `false` — either project-level `USE_WORKTREES=false`, or per-plan submodule intersection forced it false in step 2.5): Omit `isolation="worktree"` from the Task call. Replace the `` block with: @@ -585,7 +596,7 @@ increases monotonically across waves. `{status}` is `complete` (success), ``` - When worktrees are disabled, execute plans **one at a time within each wave** (sequential) regardless of the `PARALLELIZATION` setting — multiple agents writing to the same working tree concurrently would cause conflicts. + When worktrees are disabled for a plan (per-plan or project-level), that plan's executor runs on the main working tree. If **any** plan in the current wave dropped to sequential mode, execute the affected plan(s) **one at a time** to avoid concurrent writes to the main working tree — plans in the same wave that retained worktree isolation can still run in parallel alongside the sequential ones, but two non-worktree plans in the same wave must serialize. When the project-level `USE_WORKTREES=false`, all plans in the wave serialize regardless of the `PARALLELIZATION` setting. 4. **Wait for all agents in wave to complete.** @@ -763,9 +774,11 @@ increases monotonically across waves. `{status}` is `complete` (success), done < <(git worktree list --porcelain | grep "^worktree " | grep "\.claude/worktrees/agent-" | sed 's/^worktree //') ``` - **If `workflow.use_worktrees` is `false`:** Agents ran on the main working tree — skip this step entirely. + **If no plan in this wave used worktree isolation** (project-level `USE_WORKTREES=false` OR every plan in the wave had `USE_WORKTREES_FOR_PLAN=false` — i.e. `WAVE_WORKTREE_PLANS` from step 2.5 is empty): all agents ran on the main working tree — skip this step entirely. - **If no worktrees found:** Skip silently — agents may have been spawned without worktree isolation. + **If at least one plan used worktrees but others did not:** still run this cleanup — it iterates over actual `git worktree list` output and only merges back the worktrees that were created, leaving sequential plans' commits on the main tree untouched. + + **If no worktrees found at runtime:** Skip silently — agents may have been spawned without worktree isolation, or the orchestrator already cleaned them up. 5.6. **Post-merge build & test gate:** @@ -780,9 +793,9 @@ increases monotonically across waves. `{status}` is `complete` (success), Read and execute `get-shit-done/workflows/execute-phase/steps/post-merge-gate.md`. -5.7. **Post-wave shared artifact update (worktree mode only, skip if tests failed):** +5.7. **Post-wave shared artifact update (when at least one plan used worktrees, skip if tests failed):** - When executor agents ran with `isolation="worktree"`, they skipped STATE.md and ROADMAP.md updates to avoid last-merge-wins overwrites. The orchestrator is the single writer for these files. After worktrees are merged back, update shared artifacts once. + When **any** executor agent in this wave ran with `isolation="worktree"`, that agent skipped STATE.md and ROADMAP.md updates to avoid last-merge-wins overwrites. The orchestrator is the single writer for these files. After worktrees are merged back, update shared artifacts once for every completed plan in the wave (worktree-mode plans **and** sequential plans that ran on the main tree but deferred to the orchestrator for tracking writes). **Only update tracking when tests passed (TEST_EXIT=0).** If tests failed or timed out, skip the tracking update — plans should @@ -810,7 +823,7 @@ increases monotonically across waves. `{status}` is `complete` (success), Where `WAVE_PLAN_IDS` is the space-separated list of plan IDs that completed in this wave. - **If `workflow.use_worktrees` is `false`:** Sequential agents already updated STATE.md and ROADMAP.md themselves — skip this step. + **If no plan in this wave used worktrees** (project-level `USE_WORKTREES=false` OR `WAVE_WORKTREE_PLANS` is empty): sequential agents already updated STATE.md and ROADMAP.md themselves — skip this step. 5.8. **Handle test gate failures (when `WAVE_FAILURE_COUNT > 0`):** diff --git a/get-shit-done/workflows/execute-phase/steps/per-plan-worktree-gate.md b/get-shit-done/workflows/execute-phase/steps/per-plan-worktree-gate.md new file mode 100644 index 000000000..49a60a501 --- /dev/null +++ b/get-shit-done/workflows/execute-phase/steps/per-plan-worktree-gate.md @@ -0,0 +1,94 @@ +# Per-plan worktree decision (#2772) + +Run this for **each plan in the current wave** before its `Task()` dispatch. The output `USE_WORKTREES_FOR_PLAN` gates the dispatch branch (worktree mode vs sequential mode) for that plan only — other plans in the same wave can still take the worktree path. + +`SUBMODULE_PATHS` is computed once in the `initialize` step (parsed from `.gitmodules`). + +`PLAN_FILES` is the whitespace-separated list of paths the plan declared it will touch, extracted from the `phase-plan-index` JSON loaded in `discover_and_group_plans`: + +```bash +# plan_json is the JSON object for this plan from PLAN_INDEX.plans[] +# files_modified is an array of strings (repo-relative paths or globs) +PLAN_FILES=$(jq -r '.files_modified // [] | join(" ")' <<<"$plan_json") +plan_id=$(jq -r '.id' <<<"$plan_json") +``` + +Then run the per-plan gate: + +```bash +USE_WORKTREES_FOR_PLAN="$USE_WORKTREES" + +if [ -n "$SUBMODULE_PATHS" ] && [ "$USE_WORKTREES_FOR_PLAN" != "false" ]; then + if [ -z "$PLAN_FILES" ]; then + # Fallback: planned paths are unknown/unparseable — fall back to the safe + # behavior (disable worktree isolation for this plan) and log why. + echo "[worktree] Plan ${plan_id}: files_modified missing/unparseable — disabling worktree isolation as a safety fallback (submodule project)" + USE_WORKTREES_FOR_PLAN=false + else + # Compute intersection with glob-safe normalization. Both sides are + # normalized (strip leading "./", strip trailing "/") and matched + # bidirectionally so a globby planned path like "vendor/**/*.c" still + # matches submodule "vendor/foo", and "./vendor/foo/bar.c" matches + # submodule "vendor/foo". + INTERSECT="" + set -f # disable globbing while iterating literal patterns + for sm_raw in $SUBMODULE_PATHS; do + # Normalize submodule path: strip ./ prefix and trailing / + sm="${sm_raw#./}" + sm="${sm%/}" + [ -z "$sm" ] && continue + for pf_raw in $PLAN_FILES; do + # Normalize planned path the same way + pf="${pf_raw#./}" + pf="${pf%/}" + [ -z "$pf" ] && continue + matched=0 + # Direction 1: planned path is the submodule or lies inside it + case "$pf" in + "$sm"|"$sm"/*) matched=1 ;; + esac + # Direction 2: submodule lies inside the planned path (e.g. plan + # declares "vendor" or a glob expanding to a directory containing + # the submodule). + if [ "$matched" -eq 0 ]; then + case "$sm" in + "$pf"|"$pf"/*) matched=1 ;; + esac + fi + # Direction 3: planned path uses a glob — strip glob wildcards + # and check whether the resulting prefix overlaps the submodule + # path in either direction. + if [ "$matched" -eq 0 ]; then + case "$pf" in + *'*'*|*'?'*|*'['*) + # Take the literal prefix before the first glob metachar. + prefix="${pf%%[*?[]*}" + prefix="${prefix%/}" + if [ -n "$prefix" ]; then + case "$sm" in + "$prefix"|"$prefix"/*) matched=1 ;; + esac + if [ "$matched" -eq 0 ]; then + case "$prefix" in + "$sm"|"$sm"/*) matched=1 ;; + esac + fi + fi + ;; + esac + fi + if [ "$matched" -eq 1 ]; then + INTERSECT="$INTERSECT $pf_raw" + fi + done + done + set +f + if [ -n "$INTERSECT" ]; then + echo "[worktree] Plan ${plan_id}: planned paths intersect submodule paths (${INTERSECT# }) — disabling worktree isolation for this plan" + USE_WORKTREES_FOR_PLAN=false + fi + fi +fi +``` + +After running this for the plan, the dispatch branches in `execute_waves` step 3 MUST gate on `USE_WORKTREES_FOR_PLAN` for the current plan, not on the project-level `USE_WORKTREES`. Track which plans in this wave actually used worktrees (append `plan_id` to a `WAVE_WORKTREE_PLANS` accumulator when `USE_WORKTREES_FOR_PLAN != false`) — the post-wave cleanup step (5.5) uses this to decide whether worktree-merge cleanup is needed at all. diff --git a/get-shit-done/workflows/quick.md b/get-shit-done/workflows/quick.md index 838558190..52b3f31d3 100644 --- a/get-shit-done/workflows/quick.md +++ b/get-shit-done/workflows/quick.md @@ -152,15 +152,24 @@ Parse JSON for: `planner_model`, `executor_model`, `checker_model`, `verifier_mo USE_WORKTREES=$(gsd-sdk query config-get workflow.use_worktrees 2>/dev/null || echo "true") ``` -If the project uses git submodules, worktree isolation is skipped: +If the project uses git submodules, worktree isolation is unsafe **only when the quick task touches a submodule path**. The previous behavior unconditionally disabled worktree isolation whenever `.gitmodules` existed, which penalised every quick task in a submodule project even when the task was nowhere near a submodule. Parse submodule paths from `.gitmodules` so the executor can act on actual submodule paths rather than the mere file's existence: ```bash +# Parse submodule paths from .gitmodules once (empty if no .gitmodules). +# SUBMODULE_PATHS is a newline-separated list of repo-relative paths used as +# a fail-loud commit-time guard inside the quick-task executor — if the +# executor stages any path that falls inside SUBMODULE_PATHS, it must abort +# the commit and surface the conflict rather than silently corrupting the +# submodule state. if [ -f .gitmodules ]; then - echo "[worktree] Submodule project detected (.gitmodules exists) — falling back to sequential execution" - USE_WORKTREES=false + SUBMODULE_PATHS=$(git config --file .gitmodules --get-regexp '^submodule\..*\.path$' 2>/dev/null | awk '{print $2}') +else + SUBMODULE_PATHS="" fi ``` +Quick mode does not have a pre-declared `files_modified` list (the task is freeform), so use a fail-loud guard at commit time: when the executor stages files for the quick-task commit, if any staged path falls inside a `SUBMODULE_PATHS` entry, abort with a clear error explaining that worktree-isolated commits cannot safely span submodule boundaries — the user can re-run with `workflow.use_worktrees=false` to fall back to sequential execution on the main tree. If `SUBMODULE_PATHS` is empty (no `.gitmodules` in the repo), worktree isolation proceeds normally. + **If `roadmap_exists` is false:** Error — Quick mode requires an active project with ROADMAP.md. Run `/gsd-new-project` first. Quick tasks can run mid-phase - validation only checks ROADMAP.md exists, not phase status. @@ -627,9 +636,44 @@ This corrects a known issue where EnterWorktree creates branches from main inste ${AGENT_SKILLS_EXECUTOR} + +SUBMODULE_PATHS for this project: ${SUBMODULE_PATHS} + +If SUBMODULE_PATHS is non-empty, you MUST run this fail-loud guard immediately +before EVERY git commit you create during this quick task (after \`git add\`, +before \`git commit\`). Quick mode does not have a pre-declared files_modified +list, so the guard runs at commit time: + +\`\`\`bash +SUBMODULE_PATHS=\"${SUBMODULE_PATHS}\" +if [ -n \"\$SUBMODULE_PATHS\" ]; then + STAGED=\$(git diff --cached --name-only) + for sm_raw in \$SUBMODULE_PATHS; do + sm=\"\${sm_raw#./}\" + sm=\"\${sm%/}\" + [ -z \"\$sm\" ] && continue + for f_raw in \$STAGED; do + f=\"\${f_raw#./}\" + f=\"\${f%/}\" + case \"\$f\" in + \"\$sm\"|\"\$sm\"/*) + echo \"ABORT: staged path \$f_raw falls inside submodule \$sm — worktree-isolated commits cannot safely span submodule boundaries. Re-run with workflow.use_worktrees=false.\" >&2 + exit 1 ;; + esac + done + done +fi +\`\`\` + +If the guard aborts, do NOT attempt the commit, do NOT remove the staged files, +and do NOT continue subsequent tasks. Surface the abort message in your +SUMMARY.md and stop — the user must rerun with worktrees disabled. + + - Execute all tasks in the plan - Commit each task atomically (code changes only) +- Run the bash block before every \`git commit\` if SUBMODULE_PATHS is non-empty - Create summary at: ${QUICK_DIR}/${quick_id}-SUMMARY.md - Do NOT commit docs artifacts (SUMMARY.md, STATE.md, PLAN.md) — the orchestrator handles the docs commit in Step 8 - Do NOT update ROADMAP.md (quick tasks are separate from planned phases) diff --git a/tests/bug-2772-gitmodules-path-intersection.test.cjs b/tests/bug-2772-gitmodules-path-intersection.test.cjs new file mode 100644 index 000000000..3a62861b1 --- /dev/null +++ b/tests/bug-2772-gitmodules-path-intersection.test.cjs @@ -0,0 +1,563 @@ +/** + * Regression test for #2772: worktree isolation is unconditionally disabled + * when `.gitmodules` exists in the repo, even when the plan does not touch + * any submodule path. + * + * Behavioral test: the bash decision pipeline from + * get-shit-done/workflows/execute-phase.md is extracted verbatim into an + * executable snippet here, then run via execFileSync('bash', ...) against + * real fixture projects built with `createTempGitProject()`. We assert + * the resulting USE_WORKTREES_FOR_PLAN value (printed on the final line + * of stdout) and the presence/absence of the [worktree] log line for each + * scenario. + * + * If execute-phase.md's bash gate is ever rewritten so the extracted + * snippet stops matching real behavior, this test must be updated to + * track the new pipeline — never replaced with a source grep. + * + * In addition to the per-plan gate behavior, this file also asserts: + * - The workflow markdown actually wires USE_WORKTREES_FOR_PLAN into + * each of the four dispatch sites (worktree-mode gate, sequential-mode + * gate, "worktrees disabled" prose, post-wave cleanup gate). Without + * this, the per-plan computation would be dead code (the original + * #2772 fix shipped in this state — CodeRabbit caught it). + * - The quick.md executor prompt injects SUBMODULE_PATHS and a fail-loud + * pre-commit guard, and the guard actually aborts when staged paths + * fall inside a submodule. + */ + +const { describe, test, beforeEach, afterEach } = require('node:test'); +const assert = require('node:assert/strict'); +const fs = require('fs'); +const path = require('path'); +const { execFileSync } = require('child_process'); +const { createTempGitProject, cleanup } = require('./helpers.cjs'); + +// Bash snippet extracted from execute-phase.md (the SUBMODULE_PATHS parse + +// per-plan intersection logic with normalization + bidirectional matching). +// Inputs come from env vars: PLAN_FILES (whitespace-separated) and plan_id. +// Output: log lines on stdout, then a final line +// `USE_WORKTREES_FOR_PLAN=` for the test to parse. +const GATE_SNIPPET = [ + 'set -e', + 'USE_WORKTREES="${USE_WORKTREES:-true}"', + 'if [ -f .gitmodules ]; then', + " SUBMODULE_PATHS=$(git config --file .gitmodules --get-regexp '^submodule\\..*\\.path$' 2>/dev/null | awk '{print $2}')", + 'else', + ' SUBMODULE_PATHS=""', + 'fi', + 'USE_WORKTREES_FOR_PLAN="$USE_WORKTREES"', + 'if [ -n "$SUBMODULE_PATHS" ] && [ "$USE_WORKTREES_FOR_PLAN" != "false" ]; then', + ' if [ -z "$PLAN_FILES" ]; then', + ' echo "[worktree] Plan ${plan_id}: files_modified missing/unparseable — disabling worktree isolation as a safety fallback (submodule project)"', + ' USE_WORKTREES_FOR_PLAN=false', + ' else', + ' INTERSECT=""', + ' set -f', + ' for sm_raw in $SUBMODULE_PATHS; do', + ' sm="${sm_raw#./}"', + ' sm="${sm%/}"', + ' [ -z "$sm" ] && continue', + ' for pf_raw in $PLAN_FILES; do', + ' pf="${pf_raw#./}"', + ' pf="${pf%/}"', + ' [ -z "$pf" ] && continue', + ' matched=0', + ' case "$pf" in', + ' "$sm"|"$sm"/*) matched=1 ;;', + ' esac', + ' if [ "$matched" -eq 0 ]; then', + ' case "$sm" in', + ' "$pf"|"$pf"/*) matched=1 ;;', + ' esac', + ' fi', + ' if [ "$matched" -eq 0 ]; then', + ' case "$pf" in', + " *'*'*|*'?'*|*'['*)", + ' prefix="${pf%%[*?[]*}"', + ' prefix="${prefix%/}"', + ' if [ -n "$prefix" ]; then', + ' case "$sm" in', + ' "$prefix"|"$prefix"/*) matched=1 ;;', + ' esac', + ' if [ "$matched" -eq 0 ]; then', + ' case "$prefix" in', + ' "$sm"|"$sm"/*) matched=1 ;;', + ' esac', + ' fi', + ' fi', + ' ;;', + ' esac', + ' fi', + ' if [ "$matched" -eq 1 ]; then', + ' INTERSECT="$INTERSECT $pf_raw"', + ' fi', + ' done', + ' done', + ' set +f', + ' if [ -n "$INTERSECT" ]; then', + ' echo "[worktree] Plan ${plan_id}: planned paths intersect submodule paths (${INTERSECT# }) — disabling worktree isolation for this plan"', + ' USE_WORKTREES_FOR_PLAN=false', + ' fi', + ' fi', + 'fi', + 'echo "USE_WORKTREES_FOR_PLAN=$USE_WORKTREES_FOR_PLAN"', +].join('\n'); + +function runGate(cwd, env) { + const out = execFileSync('bash', ['-c', GATE_SNIPPET], { + cwd, + encoding: 'utf-8', + env: { ...process.env, ...env }, + }); + const lines = out.trim().split('\n'); + const last = lines[lines.length - 1]; + const m = last.match(/^USE_WORKTREES_FOR_PLAN=(true|false)$/); + assert.ok( + m, + `expected final line to be USE_WORKTREES_FOR_PLAN=, got: ${last}\nfull stdout:\n${out}` + ); + return { decision: m[1], stdout: out, logLines: lines.slice(0, -1) }; +} + +function writeGitmodulesWithSubmodule(repo, submodulePath) { + const content = [ + `[submodule "${submodulePath}"]`, + `\tpath = ${submodulePath}`, + `\turl = https://example.invalid/${submodulePath}.git`, + '', + ].join('\n'); + fs.writeFileSync(path.join(repo, '.gitmodules'), content); +} + +describe('Submodule worktree-isolation gate intersects planned paths (#2772)', () => { + let repo; + + beforeEach(() => { + repo = createTempGitProject('gsd-test-2772-'); + }); + + afterEach(() => { + cleanup(repo); + }); + + test('plan touching only src/ in a submodule project keeps worktree isolation ENABLED', () => { + writeGitmodulesWithSubmodule(repo, 'vendor/foo'); + + const { decision, logLines } = runGate(repo, { + PLAN_FILES: 'src/index.ts src/lib/util.ts', + plan_id: 'plan-001', + }); + + assert.equal(decision, 'true'); + assert.equal(logLines.filter((l) => l.startsWith('[worktree]')).length, 0); + }); + + test('plan touching vendor/foo/bar.ts in a submodule project DISABLES worktree isolation', () => { + writeGitmodulesWithSubmodule(repo, 'vendor/foo'); + + const { decision, stdout } = runGate(repo, { + PLAN_FILES: 'src/index.ts vendor/foo/bar.ts', + plan_id: 'plan-002', + }); + + assert.equal(decision, 'false'); + assert.match(stdout, /\[worktree\] Plan plan-002: planned paths intersect submodule paths/); + assert.match(stdout, /vendor\/foo\/bar\.ts/); + }); + + test('plan whose path equals the submodule root (vendor/foo) DISABLES worktree isolation', () => { + writeGitmodulesWithSubmodule(repo, 'vendor/foo'); + + const { decision, stdout } = runGate(repo, { + PLAN_FILES: 'vendor/foo', + plan_id: 'plan-003', + }); + + assert.equal(decision, 'false'); + assert.match(stdout, /\[worktree\] Plan plan-003: planned paths intersect submodule paths/); + }); + + test('missing files_modified in a submodule project falls back to DISABLE with a logged reason', () => { + writeGitmodulesWithSubmodule(repo, 'vendor/foo'); + + const { decision, stdout } = runGate(repo, { + PLAN_FILES: '', + plan_id: 'plan-004', + }); + + assert.equal(decision, 'false'); + assert.match(stdout, /\[worktree\] Plan plan-004: files_modified missing\/unparseable/); + assert.match(stdout, /safety fallback/); + }); + + test('repo with no .gitmodules at all keeps worktree isolation ENABLED regardless of plan paths', () => { + const { decision, logLines } = runGate(repo, { + PLAN_FILES: 'vendor/foo/bar.ts src/index.ts', + plan_id: 'plan-005', + }); + + assert.equal(decision, 'true'); + assert.equal(logLines.filter((l) => l.startsWith('[worktree]')).length, 0); + }); + + test('multiple submodules, plan touches only one of them — DISABLE with that path in the log', () => { + const gitmodules = [ + '[submodule "vendor/foo"]', + '\tpath = vendor/foo', + '\turl = https://example.invalid/foo.git', + '[submodule "third_party/bar"]', + '\tpath = third_party/bar', + '\turl = https://example.invalid/bar.git', + '', + ].join('\n'); + fs.writeFileSync(path.join(repo, '.gitmodules'), gitmodules); + + const { decision, stdout } = runGate(repo, { + PLAN_FILES: 'src/a.ts third_party/bar/b.ts', + plan_id: 'plan-006', + }); + + assert.equal(decision, 'false'); + assert.match(stdout, /third_party\/bar\/b\.ts/); + }); + + test('planned path that merely shares a prefix with a submodule (vendor/foobar) does NOT count as intersection', () => { + writeGitmodulesWithSubmodule(repo, 'vendor/foo'); + + const { decision, logLines } = runGate(repo, { + PLAN_FILES: 'vendor/foobar/x.ts', + plan_id: 'plan-007', + }); + + assert.equal(decision, 'true'); + assert.equal(logLines.filter((l) => l.startsWith('[worktree]')).length, 0); + }); + + // ---- Path-normalization & glob coverage (CodeRabbit MAJOR finding) ---- + + test('planned path with leading "./" normalizes and DISABLES isolation when inside a submodule', () => { + writeGitmodulesWithSubmodule(repo, 'vendor/foo'); + + const { decision, stdout } = runGate(repo, { + PLAN_FILES: './vendor/foo/bar.c', + plan_id: 'plan-norm-1', + }); + + assert.equal(decision, 'false', './vendor/foo/bar.c must normalize and intersect vendor/foo'); + assert.match(stdout, /vendor\/foo\/bar\.c/); + }); + + test('planned path with trailing slash equal to submodule DISABLES isolation', () => { + writeGitmodulesWithSubmodule(repo, 'vendor/foo'); + + const { decision } = runGate(repo, { + PLAN_FILES: 'vendor/foo/', + plan_id: 'plan-norm-2', + }); + + assert.equal(decision, 'false', 'trailing slash must not defeat the submodule-root match'); + }); + + test('globby planned path "vendor/**/*.c" DISABLES isolation when submodule sits inside vendor/', () => { + writeGitmodulesWithSubmodule(repo, 'vendor/foo'); + + const { decision, stdout } = runGate(repo, { + PLAN_FILES: 'vendor/**/*.c', + plan_id: 'plan-norm-3', + }); + + assert.equal( + decision, + 'false', + 'glob whose literal prefix "vendor" contains submodule vendor/foo must intersect' + ); + assert.match(stdout, /vendor\/\*\*\/\*\.c/); + }); + + test('plan declares a parent directory of the submodule (e.g. "vendor") — DISABLES isolation', () => { + writeGitmodulesWithSubmodule(repo, 'vendor/foo'); + + const { decision } = runGate(repo, { + PLAN_FILES: 'vendor', + plan_id: 'plan-norm-4', + }); + + assert.equal( + decision, + 'false', + 'planned path that contains the submodule must intersect (bidirectional matching)' + ); + }); + + test('submodule path declared with leading "./" in .gitmodules still matches a plain planned path', () => { + const gitmodules = [ + '[submodule "vendor/foo"]', + '\tpath = ./vendor/foo', + '\turl = https://example.invalid/foo.git', + '', + ].join('\n'); + fs.writeFileSync(path.join(repo, '.gitmodules'), gitmodules); + + const { decision } = runGate(repo, { + PLAN_FILES: 'vendor/foo/bar.ts', + plan_id: 'plan-norm-5', + }); + + assert.equal( + decision, + 'false', + 'submodule "./vendor/foo" must normalize and match plain planned path vendor/foo/bar.ts' + ); + }); + + test('globby planned path that does NOT overlap the submodule keeps isolation ENABLED', () => { + writeGitmodulesWithSubmodule(repo, 'vendor/foo'); + + const { decision, logLines } = runGate(repo, { + PLAN_FILES: 'src/**/*.ts', + plan_id: 'plan-norm-6', + }); + + assert.equal(decision, 'true'); + assert.equal(logLines.filter((l) => l.startsWith('[worktree]')).length, 0); + }); +}); + +// ---- Workflow-markdown wiring assertions (CodeRabbit CRITICAL finding) ---- +// +// The original PR computed USE_WORKTREES_FOR_PLAN but never read it at the +// dispatch sites — the dispatch still branched on the project-level +// USE_WORKTREES, so the per-plan decision was dead code. Assert the markdown +// actually wires the variable into the four dispatch sites. + +describe('execute-phase.md dispatch wires USE_WORKTREES_FOR_PLAN (#2772)', () => { + const workflowPath = path.join( + __dirname, + '..', + 'get-shit-done', + 'workflows', + 'execute-phase.md' + ); + const gatePath = path.join( + __dirname, + '..', + 'get-shit-done', + 'workflows', + 'execute-phase', + 'steps', + 'per-plan-worktree-gate.md' + ); + + test('workflow file exists and is readable', () => { + assert.ok(fs.existsSync(workflowPath), `expected ${workflowPath} to exist`); + }); + + test('per-plan worktree gate steps file exists and is readable', () => { + assert.ok(fs.existsSync(gatePath), `expected ${gatePath} to exist`); + }); + + test('Worktree-mode dispatch gate reads USE_WORKTREES_FOR_PLAN, not USE_WORKTREES', () => { + const md = fs.readFileSync(workflowPath, 'utf-8'); + assert.match( + md, + /\*\*Worktree mode\*\*\s*\(`USE_WORKTREES_FOR_PLAN`/, + 'Worktree-mode header must gate on USE_WORKTREES_FOR_PLAN per-plan' + ); + }); + + test('Sequential-mode dispatch gate reads USE_WORKTREES_FOR_PLAN', () => { + const md = fs.readFileSync(workflowPath, 'utf-8'); + assert.match( + md, + /\*\*Sequential mode\*\*\s*\(`USE_WORKTREES_FOR_PLAN`/, + 'Sequential-mode header must gate on USE_WORKTREES_FOR_PLAN per-plan' + ); + }); + + test('"Worktrees disabled" sequential rule is documented per-plan, not project-level', () => { + const md = fs.readFileSync(workflowPath, 'utf-8'); + assert.match( + md, + /worktrees are disabled for a plan/i, + 'sequential-execution rule must be expressed per-plan' + ); + }); + + test('execute-phase.md hooks the per-plan gate steps file at sub-step 2.5', () => { + const md = fs.readFileSync(workflowPath, 'utf-8'); + assert.match(md, /Per-plan worktree decision/, 'sub-step header must exist in execute_waves'); + assert.match( + md, + /execute-phase\/steps\/per-plan-worktree-gate\.md/, + 'execute-phase.md must reference the extracted gate file' + ); + }); + + test('per-plan gate file documents PLAN_FILES extraction from plan_json', () => { + const md = fs.readFileSync(gatePath, 'utf-8'); + assert.match( + md, + /jq -r '\.files_modified \/\/ \[\] \| join\(" "\)' <<<"\$plan_json"/, + 'PLAN_FILES extraction from plan_json must be documented in the gate file' + ); + }); + + test('per-plan gate file uses bidirectional case + glob-prefix handling + set -f discipline', () => { + const md = fs.readFileSync(gatePath, 'utf-8'); + assert.match(md, /set -f/, 'matcher must disable globbing while iterating'); + assert.match(md, /set \+f/, 'matcher must re-enable globbing after iteration'); + const pfFirst = md.match(/case "\$pf" in\s+"\$sm"\|"\$sm"\/\*\)/); + const smFirst = md.match(/case "\$sm" in\s+"\$pf"\|"\$pf"\/\*\)/); + assert.ok(pfFirst, 'matcher must check pf inside sm'); + assert.ok(smFirst, 'matcher must check sm inside pf (bidirectional)'); + assert.match(md, /sm="\$\{sm_raw#\.\/\}"/, 'submodule path must strip leading ./'); + assert.match(md, /pf="\$\{pf_raw#\.\/\}"/, 'planned path must strip leading ./'); + assert.match(md, /sm="\$\{sm%\/\}"/, 'submodule path must strip trailing /'); + assert.match(md, /pf="\$\{pf%\/\}"/, 'planned path must strip trailing /'); + }); + + test('Post-wave worktree-cleanup gate is per-plan, not blanket project-level', () => { + const md = fs.readFileSync(workflowPath, 'utf-8'); + assert.match( + md, + /WAVE_WORKTREE_PLANS/, + 'post-wave cleanup must track which plans actually used worktrees' + ); + }); +}); + +// ---- quick.md SUBMODULE_PATHS executor guard (CodeRabbit CRITICAL #3) ---- +// +// Quick mode does NOT have a pre-declared files_modified list. The fail-loud +// guard must (a) be present in the markdown of the executor prompt, and +// (b) actually abort when run against a fixture that stages a submodule path. + +describe('quick.md executor pre-commit submodule guard (#2772)', () => { + const quickPath = path.join(__dirname, '..', 'get-shit-done', 'workflows', 'quick.md'); + + test('quick.md executor prompt injects SUBMODULE_PATHS', () => { + const md = fs.readFileSync(quickPath, 'utf-8'); + assert.match( + md, + /SUBMODULE_PATHS for this project: \$\{SUBMODULE_PATHS\}/, + 'executor prompt must inline SUBMODULE_PATHS so the agent can run the guard' + ); + }); + + test('quick.md executor prompt contains a fail-loud pre-commit guard with ABORT message', () => { + const md = fs.readFileSync(quickPath, 'utf-8'); + assert.match(md, //, 'guard block must exist'); + assert.match( + md, + /git diff --cached --name-only/, + 'guard must inspect staged paths before commit' + ); + assert.match( + md, + /ABORT: staged path/, + 'guard must surface a fail-loud ABORT message on intersection' + ); + assert.match( + md, + /workflow\.use_worktrees=false/, + 'guard must tell the user how to recover (re-run without worktrees)' + ); + }); + + // Behavioral: extract the guard logic and run it against a fixture repo. + // We simulate the executor's commit-time guard and assert it aborts when a + // staged path falls inside a SUBMODULE_PATHS entry, and passes otherwise. + const QUICK_GUARD_SNIPPET = [ + 'set +e', + 'STAGED=$(git diff --cached --name-only)', + 'if [ -n "$SUBMODULE_PATHS" ]; then', + ' for sm_raw in $SUBMODULE_PATHS; do', + ' sm="${sm_raw#./}"', + ' sm="${sm%/}"', + ' [ -z "$sm" ] && continue', + ' for f_raw in $STAGED; do', + ' f="${f_raw#./}"', + ' f="${f%/}"', + ' case "$f" in', + ' "$sm"|"$sm"/*)', + ' echo "ABORT: staged path $f_raw falls inside submodule $sm — re-run with workflow.use_worktrees=false" >&2', + ' exit 1 ;;', + ' esac', + ' done', + ' done', + 'fi', + 'echo "OK"', + ].join('\n'); + + test('guard ABORTs when a staged path falls inside a submodule', () => { + const repo = createTempGitProject('gsd-test-2772-quick-abort-'); + try { + // Create a file inside the submodule path and stage it. + fs.mkdirSync(path.join(repo, 'vendor', 'foo'), { recursive: true }); + fs.writeFileSync(path.join(repo, 'vendor', 'foo', 'bar.ts'), 'export {};\n'); + execFileSync('git', ['add', 'vendor/foo/bar.ts'], { cwd: repo }); + + let err; + try { + execFileSync('bash', ['-c', QUICK_GUARD_SNIPPET], { + cwd: repo, + encoding: 'utf-8', + env: { ...process.env, SUBMODULE_PATHS: 'vendor/foo' }, + }); + } catch (e) { + err = e; + } + assert.ok(err, 'guard must exit non-zero when staged path is inside submodule'); + assert.equal(err.status, 1, 'guard must exit with status 1'); + const stderr = err.stderr ? err.stderr.toString() : ''; + assert.match(stderr, /ABORT: staged path vendor\/foo\/bar\.ts/); + assert.match(stderr, /vendor\/foo/); + } finally { + cleanup(repo); + } + }); + + test('guard passes when no staged path falls inside a submodule', () => { + const repo = createTempGitProject('gsd-test-2772-quick-pass-'); + try { + fs.mkdirSync(path.join(repo, 'src'), { recursive: true }); + fs.writeFileSync(path.join(repo, 'src', 'index.ts'), 'export {};\n'); + execFileSync('git', ['add', 'src/index.ts'], { cwd: repo }); + + const out = execFileSync('bash', ['-c', QUICK_GUARD_SNIPPET], { + cwd: repo, + encoding: 'utf-8', + env: { ...process.env, SUBMODULE_PATHS: 'vendor/foo' }, + }); + assert.match(out, /OK/); + } finally { + cleanup(repo); + } + }); + + test('guard normalizes leading "./" on staged paths and still ABORTs', () => { + const repo = createTempGitProject('gsd-test-2772-quick-norm-'); + try { + fs.mkdirSync(path.join(repo, 'vendor', 'foo'), { recursive: true }); + fs.writeFileSync(path.join(repo, 'vendor', 'foo', 'bar.ts'), 'export {};\n'); + execFileSync('git', ['add', 'vendor/foo/bar.ts'], { cwd: repo }); + + let err; + try { + // Submodule path declared with ./ prefix — must still match. + execFileSync('bash', ['-c', QUICK_GUARD_SNIPPET], { + cwd: repo, + encoding: 'utf-8', + env: { ...process.env, SUBMODULE_PATHS: './vendor/foo' }, + }); + } catch (e) { + err = e; + } + assert.ok(err, 'guard must abort even when SUBMODULE_PATHS uses ./ prefix'); + assert.equal(err.status, 1); + } finally { + cleanup(repo); + } + }); +});