diff --git a/.changeset/gallant-tunas-climb.md b/.changeset/gallant-tunas-climb.md new file mode 100644 index 000000000..cbc5c02e6 --- /dev/null +++ b/.changeset/gallant-tunas-climb.md @@ -0,0 +1,5 @@ +--- +type: Fixed +pr: 4240 +--- +**`quick-batch --resume` no longer duplicates work after a coordinator crash** — a crash between an item's executor finishing and its merge could previously cause resume to dispatch a second executor into a new worktree, silently orphaning the first one's completed work. Resume now recognizes an already-executed item and routes it straight to merge. diff --git a/.gsd/phase/feat-3677-quick-batch-hardening-acceptance/00-run.json b/.gsd/phase/feat-3677-quick-batch-hardening-acceptance/00-run.json new file mode 100644 index 000000000..ab30f86fa --- /dev/null +++ b/.gsd/phase/feat-3677-quick-batch-hardening-acceptance/00-run.json @@ -0,0 +1,18 @@ +{ + "phase": "feat-3677-quick-batch-hardening-acceptance", + "issue": 3677, + "epic": 3344, + "epic_phase": 5, + "branch": "feat/3677-quick-batch-hardening-acceptance", + "base_revision": "114dfcb739", + "base_ref": "origin/next", + "note": "Design artifacts rebuilt after a prior research-pass worktree was garbage-collected (no commits ever landed, so gitignored .gsd/ content was lost with it). This directory is force-added via a one-off checkpoint commit so a future resume can read it even though .gsd/ is normally gitignored project-wide.", + "prior_summary_reverified": true, + "step1_conclusion": "crash-window gap CONFIRMED REAL — see 40-design.md Open Question 1 / Resolution", + "dependencies": { + "phase1": "feat/3673-dispatch-maxconcurrency-axis (merged, #4190 area)", + "phase2": "feat/3674-shared-wave-partitioner (merged)", + "phase3": "feat/3675-quick-batch-core-primitives (merged, PR #4190)", + "phase4": "feat/3676-quick-batch-command-workflow (merged, PR #4212)" + } +} diff --git a/.gsd/phase/feat-3677-quick-batch-hardening-acceptance/40-design.md b/.gsd/phase/feat-3677-quick-batch-hardening-acceptance/40-design.md new file mode 100644 index 000000000..20a01101f --- /dev/null +++ b/.gsd/phase/feat-3677-quick-batch-hardening-acceptance/40-design.md @@ -0,0 +1,436 @@ +# Phase 5 (#3677) design — quick-batch hardening and acceptance + +Epic #3344, Phase 5 (final). Base: `origin/next` @ `114dfcb739` (Phases 1-4 +merged: #4190 core primitives, #4212 command/workflow/isolation). + +## 0. Provenance note + +A prior research pass produced a design doc + test matrix in this same +directory, but its worktree was garbage-collected before those files were +committed (`.gsd/` is gitignored repo-wide; "no commits" was treated as "no +changes" by worktree auto-cleanup). This document is a from-scratch rebuild, +grounded in an independent re-read of the actual workflow steps and source, +not a transcription of the lost files. Where it agrees with the prior +agent's SUMMARY, that is re-verified agreement, not inherited trust. + +## 1. Open Question 1 — crash-window duplicate dispatch (RESOLVED: real gap, fix proposed) + +**Claim under test:** a coordinator crash between Step 6 (executor returns, +`SUMMARY.md` written, real commit in a real worktree) and Step 7 (merge) +leaves the item's `BATCH.json` status at `pending`, and `--resume` would +dispatch a SECOND executor into a NEW worktree for the same item, orphaning +the first. + +**Trace performed independently** (not reusing the prior agent's citations): + +- `src/quick-batch.cts:894-899` — `resumeBatch`'s ONLY crash-window + detection completes an item early via `hasQuickTaskRow(stateContent, + it.quick_id)` — a check against `.planning/STATE.md`. The eligibility + filter at `src/quick-batch.cts:926-928` is otherwise pure + status/depends_on logic; it has no awareness of `PLAN.md`/`SUMMARY.md` on + disk. +- `gsd-core/workflows/quick-batch/steps/completion.md:1-8` — confirms + `completeQuickItem` (Step 9) is "the ONLY writer of a 'Quick Tasks + Completed' STATE.md row" — i.e. the STATE.md row `hasQuickTaskRow` checks + for is written only AFTER Step 7 (merge) and, when `--validate`, Step 8 + (verification) both succeed. A crash between Step 6 and Step 7 crashes + before any STATE.md row exists, so `resumeBatch`'s only recovery signal + never fires for this window. +- `gsd-core/workflows/quick-batch/steps/resume-mode.md:23-27,45-49` — on + `--resume`, `eligible` items flow straight into the SAME per-round loop + Step 3/planner-wave/worktree-dispatch use for a fresh batch — "the DAG- + layer loop in `planner-wave.md` reads `$BATCH_MANIFEST_JSON`/`$BATCH_ID` + exactly the same way whether this batch was just created or just resumed." +- `gsd-core/workflows/quick-batch/steps/planner-wave.md:15-19` — the + planning loop's eligibility for THIS layer is `status == "pending"` AND + `${item_dir}/${quick_id}-PLAN.md` does **not** yet exist on disk. An item + whose PLAN.md already exists (planned before the crash) is correctly + skipped here — planning is NOT re-run. This is the file-existence guard + the prior summary described as absent; it is present, but only at the + PLANNING layer. +- `gsd-core/workflows/quick-batch/steps/worktree-dispatch.md:29-59` (Step 6, + "Dispatch rounds") — re-derives eligibility every round via `quick-batch + resume --batch "$BATCH_ID" --raw` (same primitive as above, same blind + spot), then spawns an executor for every item in `$spawn` (backpressure- + limited `eligible`). **There is no analogous check here for "does + `${item_dir}/${quick_id}-SUMMARY.md` already exist" before spawning.** The + only SUMMARY.md check in this file is AFTER dispatch, at + `worktree-dispatch.md:118-121` ("After every item dispatched this round + returns: verify SUMMARY.md exists") — too late to prevent the duplicate + dispatch itself, and it doesn't even apply here since the crashed + coordinator never issued this round's dispatch in the first place; a NEW + coordinator process re-deriving eligibility from scratch has no round + history to consult. +- `gsd-core/workflows/quick-batch/steps/merge-wave.md:10-15` (Step 7) — its + OWN mergeable-wave criterion is independent of the `eligible`/`spawn` list + entirely: "items that are `status == 'pending'` with a `SUMMARY.md` on + disk... and NOT yet merged." This is correct and sufficient FOR MERGE — it + would find and merge the original (first) worktree's completed work fine, + IF nothing else raced it. The bug is that Step 6 runs before Step 7 in + every pass (including a resumed one) and would try to spawn a second + executor for the same item first. + +**Conclusion: CONFIRMED REAL.** Nothing in the resume/dispatch path checks +for a pre-existing `SUMMARY.md` before dispatching an executor. An item that +crashed after Step 6 but before Step 9's STATE.md write is indistinguishable, +to `worktree-dispatch.md`'s dispatch loop, from an item that was never +started — it re-enters `$spawn` and gets a brand-new `git worktree add` + +`gsd-executor` dispatch. The original worktree (with its real commit and +`SUMMARY.md`) is never referenced again by anything — not cleaned up, not +merged via its own record, simply orphaned. If both executors happen to +run to completion, Step 7's merge-eligibility check (`SUMMARY.md` exists on +disk, "not yet merged") only knows about ONE `$item_dir` per `quick_id` +(directories are keyed by `quick_id`+slug, not by worktree), so the SECOND +executor's `SUMMARY.md` write clobbers the first at the same path, and only +the SECOND worktree's `WT_PATH`/`WT_BRANCH` (recorded in +`$QUICK_BATCH_WORKTREE_MANIFEST` under the second dispatch's `agent_id`) is +ever merged — the FIRST worktree's real, already-committed work is never +merged, never cleaned up, and never surfaced as an error. This is a silent +work-loss bug, not merely a resource leak. + +**Proposed fix (confidence: HIGH — narrow, mechanical, directly mirrors an +existing, already-reviewed pattern in the same file family):** + +Add a SUMMARY.md-existence exclusion to `worktree-dispatch.md`'s Step 6, +substep 1 (the per-round eligibility re-derivation), symmetric to +`planner-wave.md`'s PLAN.md-existence exclusion: after parsing `eligible` +from `quick-batch resume`, drop any `quick_id` whose +`${item_dir}/${quick_id}-SUMMARY.md` already exists on disk from the set +passed to `spawn-plan` (never dispatch it) — those items fall through +untouched to Step 7, whose own mergeable-wave criterion already picks them +up correctly by the same file check. This is a workflow-layer (prose/bash) +fix, not a change to `resumeBatch` or any other already-merged, already- +reviewed `.cts` module — see rejected alternatives below for why. + +**Fix location decision — workflow layer, not `resumeBatch` core primitive:** +`resumeBatch` (`src/quick-batch.cts:863-939`) is a shared primitive called +from two call sites with different needs (`resume-mode.md`'s one-shot resume +report AND `worktree-dispatch.md`'s per-round re-derivation). Teaching +`resumeBatch` about `${item_dir}/${quick_id}-SUMMARY.md` paths would require +it to either accept a slug-generation dependency it doesn't currently have, +or have its `eligible` return value silently diverge in meaning between the +two callers (a `resume-mode.md` "eligible" count that excludes +in-flight-but-uncommitted-to-merge items reads correctly for a resume +report; a "the item exists in $item_dir so don't re-plan/re-dispatch it" +check is a workflow-execution concern). Filtering in +`worktree-dispatch.md`, immediately before the existing `spawn-plan` call, +keeps the fix local to the one place that actually dispatches executors, +touches zero already-reviewed `.cts` files, and is directly analogous to +the file-existence check `planner-wave.md` already does one step earlier in +the same pipeline for the same reason. + +## 2. Coverage table (#3677 acceptance criteria → status) + +| # | AC bullet (verbatim, abbreviated) | Existing coverage | Gap | Action | +|---|---|---|---|---| +| 1 | Security: traversal, symlink escape, special files, prompt-injection, shell-metacharacter task text, manifest tampering, **arbitrary-worktree ownership attempts** | `tests/quick-batch.test.cjs` + `tests/quick-batch.property.test.cjs` cover traversal/symlink/special-file `--file` rejection (Phase 3/4 `60-review.json` fixed real findings); `gsd-quick-batch-workflow.test.cjs` covers the DATA_START/DATA_END prompt-injection boundary and quoted `$ARGUMENTS`. **Arbitrary-worktree-ownership tampering is NOT covered** — no test constructs a cleanup-wave manifest entry naming a worktree path/branch this batch never created. | Real gap | New hostile test | +| 2 | Capacity precedence/backpressure | `quick-batch-dispatch.test.cjs` + `.property.test.cjs` cover `effective-concurrency`/`spawn-plan` exhaustively (Phase 3). | None found | none | +| 3 | Scheduling: cycles, unknown deps, deterministic waves, isolation modes, serialized lifecycle, deterministic merge, conflicts, **scope drift**, stale bases, **submodules** | `quick-batch.test.cjs`/`.property.test.cjs` cover cycle/unknown-dep rejection and wave determinism. `gsd-quick-batch-merge-integration.test.cjs` covers a real merge conflict and a real undeclared deletion end-to-end. `worktree-safety.test.cjs:5716-6313` covers the `.gitmodules`/`SUBMODULE_PATHS` isolation-disable gate and the executor's own pre-commit submodule guard extensively — but always in the `execute-phase`/`quick.md` context, never through quick-batch's OWN merge/cleanup call path with a real `.gitmodules` file in the repo. `planWaveScopeConformance` (`src/worktree-safety.cts:921-...`) is unit-tested for its advisory `SCOPE_OUT_OF_DECLARED` warning, but not exercised end-to-end through a real git diff via `executeWorktreeWaveCleanupPlan`/`worktree.cleanup-wave` the way merge_failed/scope_violation already are. | Two real gaps | New real-git submodule integration test; new real-git advisory scope-drift test | +| 4 | Fault-injection: every durable manifest/STATE crash window, resume exactly-once | STATE-row crash window (Step 9) is covered. **The Step-6→Step-7 crash window (this doc §1) was UNCOVERED and is the one genuine functional gap in this phase.** | Real gap (now understood + fixed) | Fix + regression test | +| 5 | Outcome propagation (blocked/independent-continue) | Covered by `resumeBatch`'s blocked-propagation fixed-point tests and `quick-batch-dispatch.test.cjs`'s routing tests. | None found | none | +| 6 | Docs: v1 limits (`--discuss`/`--full`, no gap-fix loop, `none`=sequential) | `docs/how-to/batch-quick-tasks.md` states all of these already (lines 52-55, 74, 113). | None found | none | +| 7 | Generated artifact sync (command/skill/registry/inventory/matrix/install-tree) | Enforced by existing repo-wide generated-sync lint (not quick-batch-specific); Phase 4 already ran `regen:derived`. | None found (no new command surface added this phase) | none | +| 8 | `/gsd:quick` regression + quick-ID grammar green | `gsd-quick-batch-quick-regression.test.cjs` exists explicitly for this. | None found | none | +| 9 | Docs: preserved-worktree diagnosis | `docs/how-to/batch-quick-tasks.md:114` — exactly one sentence ("its worktree is preserved (never deleted) so you can inspect what happened"), no path, no diagnostic steps, no recovery procedure. | Real gap (thin, not absent) | Doc extension | +| 10 | Final acceptance evidence mapped to #3344 | Not yet produced this phase. | Real gap | New doc artifact | +| 11 | RED/GREEN/REFACTOR commit discipline, closes #3677 only | Process requirement, not a test | N/A | Followed in implementation | + +## 3. Prior-art grounding for new tests + +- Hostile worktree-ownership test: follows the REAL-git-fixture pattern + already established by `tests/gsd-quick-batch-merge-integration.test.cjs` + (`initRepo`/`addWorktree`, real `executeWorktreeWaveCleanupPlan`, + `WORKTREE_AGENT_BRANCH_RE` branch-name validation already enforced at + `src/worktree-safety.cts:512`) — a manifest entry naming a path/branch + never created by this batch (e.g. a sibling repo's worktree, or a + plausible-looking but foreign branch) must be rejected/blocked, never + merged or deleted. +- Advisory scope-drift test: exercises `planWaveScopeConformance` + (`src/worktree-safety.cts:921-...`, `WAVE_CLEANUP_WARNING.SCOPE_OUT_OF_DECLARED` + at `:879-884`) end-to-end through a REAL git diff via + `executeWorktreeWaveCleanupPlan`, mirroring the merge-conflict/undeclared- + deletion pattern in `gsd-quick-batch-merge-integration.test.cjs` — commit a + path outside `files_modified` and assert the merge still SUCCEEDS + (advisory, never blocking) while a warning with the frozen code is + produced. +- Submodule integration test: reuses `writeGitmodulesWithSubmodule`'s + fixture shape from `tests/worktree-safety.test.cjs:5879-5886` but drives + it through the quick-batch merge/cleanup call path + (`executeWorktreeWaveCleanupPlan`) instead of the isolation-decision gate + that file already covers — proves a repo containing `.gitmodules` merges + cleanly through quick-batch's own primitive when the plan doesn't touch + the submodule path, and that a plan touching the submodule path still + merges (worktree isolation's own submodule-intersection gate is a + separate, already-covered pre-dispatch decision, not a merge-time block). +- Crash-window regression test: structural assertion on + `worktree-dispatch.md`'s prose, following the SAME established convention + `tests/gsd-quick-batch-workflow.test.cjs` already uses for every other + workflow-file behavior (e.g. its own "planner-wave.md marks a missing + PLAN.md item failed" test at line 253-256) — this repo's blessed pattern + for testing markdown-as-source (see that file's own header comment, + lines 16-21). + +## 4. Not-corruption + +None of the new tests duplicate existing coverage: +- The merge-conflict/undeclared-deletion real-git tests already in + `gsd-quick-batch-merge-integration.test.cjs` are untouched; the new tests + add sibling `describe` blocks for DIFFERENT manifest-entry shapes + (foreign ownership, out-of-scope-but-declared-nothing-wrong path, + submodule-bearing repo) using the same helpers, not modifying existing + assertions. +- `worktree-safety.test.cjs`'s extensive `.gitmodules` coverage (isolation + ENABLE/DISABLE decision, executor pre-commit shell guard) is left + entirely alone — the new test targets a different function + (`executeWorktreeWaveCleanupPlan`, the merge primitive) that file never + exercises with a `.gitmodules` fixture. +- The crash-window fix touches only `worktree-dispatch.md`; it does not + modify `resumeBatch`, `planner-wave.md`, or `merge-wave.md`, all of which + keep their existing, already-reviewed behavior and tests unchanged. + +## 5. Blast radius + +- `gsd-core/workflows/quick-batch/steps/worktree-dispatch.md` — additive + SUMMARY.md-existence filter, Step 6 substep 1 only. +- `tests/gsd-quick-batch-workflow.test.cjs` — one new `describe` block + (structural regression test for the filter above). +- `tests/gsd-quick-batch-merge-integration.test.cjs` — new `describe` + blocks: worktree-ownership tampering, advisory scope-drift, submodule + integration. +- `docs/how-to/batch-quick-tasks.md` — extend the "Resuming and failure + recovery" section with a preserved-worktree diagnosis subsection. +- New doc artifact: `.gsd/phase/feat-3677-quick-batch-hardening-acceptance/60-acceptance-evidence.md` + mapping every #3344 AC bullet to its evidence. +- No `src/*.cts` production module changes required — the one functional + fix lands entirely in workflow prose. + +## 6. Laws (invariants that must keep holding) + +- Single-writer invariant (STATE.md/BATCH.json/ROADMAP.md owned only by the + coordinator) — untouched. +- `resumeBatch`'s existing crash-window detection (STATE.md row) and + blocked/failed propagation fixed-point — untouched, still the sole + authority for `complete`/`blocked` transitions. +- Merge scope validation (`partitionDeclaredDeletions`, + `planWaveScopeConformance`) stays advisory-for-scope / + blocking-for-undeclared-deletion exactly as already implemented — the new + scope-drift test asserts this distinction, it does not change it. +- `WORKTREE_AGENT_BRANCH_RE` remains the sole gate on what counts as a + batch-owned branch name — the ownership-tampering test asserts against + this existing regex, does not introduce a second one. + +## 7. Rejected alternatives + +1. **Fix the crash window inside `resumeBatch`** (teach it about + `SUMMARY.md`/item directories). Rejected — see §1 fix-location decision; + would touch already-merged, already-reviewed code for a fix whose + natural home is one step later in the pipeline, and would overload + `resumeBatch`'s single-purpose eligibility contract for two callers with + different needs. +2. **Mark the item `blocked`/`human_needed` when SUMMARY.md exists but + status is still `pending`.** Rejected — this is not a failure or an + ambiguous state; it's a known, recoverable mid-flight condition. Silently + skipping re-dispatch and letting Step 7's existing merge criterion pick + it up is strictly less invasive and requires no new status value or + routing branch. +3. **Add a lock file per in-flight worktree instead of a file-existence + check.** Rejected — `SUMMARY.md`'s existence already IS the durable, + crash-safe signal (it's written by the executor as its last real action, + same as everywhere else in this design uses artifact-existence over + process state); a separate lock file adds a second source of truth that + itself needs crash-recovery semantics. +4. **Skip the submodule/scope-drift/ownership tests as "already implied" by + unit-level coverage.** Rejected per this repo's own established + pattern (`gsd-quick-batch-merge-integration.test.cjs`'s own header + comment): pure-function assertions on `routeMergeOutcome`/ + `planWaveScopeConformance` were previously judged insufficient without a + REAL git fixture proving the underlying primitive agrees; the same + standard applies to the three new gaps here. + +## 8. Known limits (v1, documented, not fixed here) + +- A crash DURING Step 7 (merge in progress) still relies on + `executeWorktreeWaveCleanupPlan`'s own mid-merge halt behavior + (`merge-wave.md:56-59`) — unchanged by this phase, already covered by + Phase 3/4's own worktree-safety suite. +- No automatic gap-fix retry after a `gaps_found` verification outcome + (documented v1 exclusion, unchanged). +- The new SUMMARY.md-existence filter does not distinguish "crashed after + writing SUMMARY.md" from "executor is still mid-write" (a partial file); + this is the same trust boundary the rest of the pipeline already accepts + (`worktree-dispatch.md:118-121`'s own post-dispatch check treats + SUMMARY.md existence as the completion signal, no partial-write handling + anywhere in this design). + +## 9. Review Pass 2 addendum — orthogonal Spec + Security findings, both closed + +Two isolated review passes (neither authored the original diff) found two +real test-quality gaps in §1's implementation and the ownership-tampering +tests in §3. Both are closed here, WITHOUT deferral, per this repo's +absolute no-defer rule — including a THIRD, self-discovered defect surfaced +while fixing the first finding (see §9.3). + +### 9.1 Spec finding — crash-window test was a prose proxy, not behavioral proof + +The original `tests/gsd-quick-batch-workflow.test.cjs` regression tests for +§1's fix only asserted `readStep('worktree-dispatch.md')` + regex matches +against the MARKDOWN — proving the documentation says the right thing, +never that the runtime condition (pending status + on-disk SUMMARY.md + +absent STATE row) is actually handled correctly. #3677's own "Alternatives +considered" explicitly rejects "document recovery without fault injection." + +**Fix:** the filtering decision itself — "drop items whose SUMMARY.md +exists from the eligible-for-dispatch set" — is now a pure, independently +testable function, `filterAlreadyExecuted(eligibleIds, executedIds)` in +`src/quick-batch-dispatch.cts`, wired to a new `quick-batch filter-executed` +CLI verb (`src/quick-batch-command-router.cts`), following the SAME +pure-decision-then-CLI-wired pattern `computeSpawnPlan`/`computeMergeOrder` +already establish in that module. `worktree-dispatch.md` now calls this +verb explicitly instead of describing the decision only in prose. + +A genuine fixture-based test in `tests/quick-batch.test.cjs` (new describe +block "crash-window duplicate-dispatch guard (#3677)") constructs a REAL +`BATCH.json` via `createBatch`, writes a REAL `SUMMARY.md` file on disk at +the item's real `item_dir` (via `generateSlugInternal`, the same slug +derivation every workflow step uses), calls the REAL `resumeBatch`, and +proves `resumeBatch` alone still reports the item eligible — then proves +`filterAlreadyExecuted`, fed a real filesystem check, correctly excludes +it. A second test proves an item with no real SUMMARY.md is NOT excluded +(boundary case — the guard must not over-fire). The prior prose-assertion +tests are KEPT (they still prove the workflow markdown is correctly WIRED +to call the new verb, in the right order) but are no longer the only proof. +Pure-function boundary tests (empty/all-executed/order-preserving/Set vs +array/phantom-id) live in `tests/quick-batch-dispatch.test.cjs`; CLI-wiring +tests live in `tests/quick-batch-command-router.test.cjs`. + +### 9.2 Security finding — ownership-tampering tests didn't test ownership + +The original two tests (branch-name-fails-shape-check; wholly-foreign +never-registered repo) proved real things, but neither exercised what +"arbitrary-worktree ownership" actually names: a manifest entry whose +`worktree_path`/`branch` are swapped to point at a DIFFERENT, +GENUINELY-REGISTERED sibling worktree of the SAME `repoRoot` (a concurrent +batch's own agent worktree, or a stale worktree from a prior crashed run), +with a branch name that passes `WORKTREE_AGENT_BRANCH_RE`'s shape check and +a base that is legitimately in `allowed_bases`. + +**Investigation conclusion: ALREADY SAFE — not a reachable gap.** Traced +`executeWorktreeWaveCleanupPlan` (`src/worktree-safety.cts:1005-1217`) +directly against two REAL, concurrently-alive sibling worktrees of one +repo. Git itself enforces branch-per-worktree uniqueness — the same branch +cannot be checked out in two worktrees of one repo at once — so +`worktree_path`'s ACTUAL checked-out branch +(`git -C worktree_path rev-parse --abbrev-ref HEAD`, `:1047`) can only +equal a swapped-in `entry.branch` if that `entry.branch` is the SIBLING's +own real, uniquely-generated branch name. Manifest tampering confined to +ONE batch's own record has no way to know that name: branch names are +`agent-[-]`-shaped (`execute-phase`'s own +`executor-isolation-dispatch.md:269-270` convention, reused verbatim per +`worktree-dispatch.md`'s own text), and `quick_id` allocation is +collision-checked GLOBALLY across every existing quick task AND batch +(`src/quick-batch.cts` `collectExistingBatchQuickIds`) — not merely +within one batch. Constructing a passing swap therefore requires ALSO +having legitimate read access to the sibling's own worktree/branch record, +which is a strictly larger compromise than "tamper with this batch's own +manifest," the scope the AC bullet actually names. + +Empirically verified (not merely reasoned about) with two real +`git worktree add`-created siblings sharing one merge-base: a manifest +entry naming item 2's real path but item 1's real branch name (the direct +swap), and the reverse, both come back `status: 'blocked', +reason: 'branch_mismatch'` — never `merge_failed`, never a wrongly +successful merge. Both real worktrees, their branches, and item 2's real +uncommitted-to-main commit survive completely untouched by either attempt. + +**Fix:** a new, stronger test in `tests/gsd-quick-batch-merge-integration.test.cjs` +("a manifest entry with one sibling worktree's real PATH but the OTHER +sibling's real BRANCH name is blocked") supplements (does not replace) the +original two tests, which still prove real, distinct boundaries +(branch-shape rejection at the manifest-normalization layer; a wholly +foreign, never-registered repo blocked via `base_mismatch`). + +**Explicitly documented trust boundary (not a gap, not fixed):** +`executeWorktreeWaveCleanupPlan` defends against fabricated/mismatched +`{worktree_path, branch, base}` triples; it does NOT defend against a +CALLER bug that correctly copies a real-but-wrong-item's triple into the +wrong manifest entry (i.e. gets `agent_id` attribution wrong while every +git-verifiable field is internally consistent for SOME real worktree). This +is why §9.3's durable `dispatched_worktree`/`dispatched_branch`/ +`dispatched_base` fields are stored PER quick_id (a direct keyed lookup), +never via a shared array requiring an `agent_id`-matching search — the +storage shape itself avoids the one class of caller-side attribution bug +this primitive cannot see. + +### 9.3 Self-discovered defect (not deferred) — durable worktree recovery was missing entirely + +While building §9.1's real fixture, tracing `merge-wave.md` substep 3 +("`$WT_PATH`/`$WT_BRANCH`/`$EXPECTED_BASE` per item come from the recorded +`$QUICK_BATCH_WORKTREE_MANIFEST` entry Step 6 wrote for that `agent_id`") +against `/gsd:quick`'s own prior art (`gsd-core/workflows/quick.md:415`: +`QUICK_WORKTREE_MANIFEST=$(mktemp ...)`) revealed that +`$QUICK_BATCH_WORKTREE_MANIFEST` — quick-batch explicitly models it on the +SAME mechanism — is a fresh, PER-PROCESS `mktemp` file, not a durable +record. §1's own fix (never re-dispatch an item whose SUMMARY.md already +exists) means a RESUMED coordinator process's Step 6 correctly does NOT +create a fresh manifest entry for that item — but nothing else durably +recorded that item's `worktree_path`/`branch`/`expected_base` either, so +Step 7 in the resumed process would have had NO data to build that item's +cleanup-wave entry from. §1's original claim ("not lost: merge-wave.md's +own criterion already picks it up") was therefore ACCURATE about +merge-eligibility ("should this item be merged") but WRONG about data +availability ("with what worktree/branch"). Before §1's fix, this never +surfaced as a visible bug because the old (harmful) re-dispatch behavior +always populated a fresh manifest entry for whichever (wrong, duplicate) +worktree it just created — the durable-recovery gap was latent, masked by +the duplicate-dispatch bug itself. + +**Fix:** three new fields on `QuickBatchItem` +(`dispatched_worktree`/`dispatched_branch`/`dispatched_base`, +`src/quick-batch.cts`) — deliberately NOT a reuse of the pre-existing +`worktree` field, whose `loadBatch` validation requires the path to exist +on disk (verified empirically: reusing it made the batch permanently +unloadable the moment a legitimately-merged worktree was removed, since +`updateBatchItems` itself calls `loadBatch` first). The three new fields +carry no existence check — their entire purpose is to stay readable (and +clearable) after a legitimate post-merge removal. `updateBatchItems` +(`QuickBatchItemUpdate`) gained matching optional `dispatchedWorktree`/ +`dispatchedBranch`/`dispatchedBase` fields (explicit `null` clears, +`undefined` leaves untouched — same convention as `dependsOn`/ +`plannedFiles`). `worktree-dispatch.md` persists the triple immediately +after recording the ephemeral entry; `merge-wave.md` falls back to it when +the ephemeral manifest lacks an entry, clears it after a successful merge, +and fails closed (`merge_failed: "missing durable worktree record"`) rather +than guessing if somehow all three are still null. + +Verified end-to-end (persist → fresh `loadBatch` recovers the triple → +clear → a SUBSEQUENT `loadBatch` still succeeds even though the path no +longer exists) in `tests/quick-batch.test.cjs`'s new "durable +worktree-recovery fields (#3677)" describe block, plus structural wiring +tests in `tests/gsd-quick-batch-workflow.test.cjs` for both workflow files. + +### 9.4 Revised blast radius (supersedes §5 for the fields below) + +- `src/quick-batch.cts` — `QuickBatchItem`/`QuickBatchItemInput`/ + `QuickBatchItemUpdate` gain `dispatched_worktree`/`dispatched_branch`/ + `dispatched_base` (+ camelCase input/update equivalents); + `validateBatchSchema` gains matching type checks (no existence check). +- `src/quick-batch-dispatch.cts` — new `filterAlreadyExecuted` pure + function + `FilterAlreadyExecutedResult` type. +- `src/quick-batch-command-router.cts` — new `filter-executed` CLI verb. +- `gsd-core/workflows/quick-batch/steps/worktree-dispatch.md` — the guard + now calls `quick-batch filter-executed` instead of describing the split + only in prose; a new durable-persistence step after recording the + ephemeral manifest entry. +- `gsd-core/workflows/quick-batch/steps/merge-wave.md` — durable fallback + for `$WT_PATH`/`$WT_BRANCH`/`$EXPECTED_BASE`; clears the triple on a + successful merge; fails closed when no record exists anywhere. +- Test files: `tests/quick-batch.test.cjs`, `tests/quick-batch-dispatch.test.cjs`, + `tests/quick-batch-command-router.test.cjs`, + `tests/gsd-quick-batch-workflow.test.cjs`, + `tests/gsd-quick-batch-merge-integration.test.cjs` — all gain new, + independently-verified (real fixture / real git, not merely prose-proxy) + coverage per §9.1-9.3. +- §5's "No `src/*.cts` production module changes required" is SUPERSEDED — + three `.cts` modules changed, all additive (new fields/functions/verbs, + zero changes to existing field shapes, existing function signatures, or + existing CLI verb behavior). diff --git a/.gsd/phase/feat-3677-quick-batch-hardening-acceptance/50-test-matrix.md b/.gsd/phase/feat-3677-quick-batch-hardening-acceptance/50-test-matrix.md new file mode 100644 index 000000000..3fbb01658 --- /dev/null +++ b/.gsd/phase/feat-3677-quick-batch-hardening-acceptance/50-test-matrix.md @@ -0,0 +1,41 @@ +# Phase 5 (#3677) test matrix + +Companion to `40-design.md`. Only NEW tests this phase adds — pre-existing +coverage is cited in `40-design.md` §2/§3, not re-listed here. + +| Row | Test file | Test name (abbreviated) | Proves | #3677 AC bullet | #3344 AC bullet | +|---|---|---|---|---|---| +| 1 | `tests/gsd-quick-batch-merge-integration.test.cjs` | "a manifest entry naming a worktree/branch this batch never created is blocked, never merged or deleted" | `executeWorktreeWaveCleanupPlan` rejects a cleanup-wave entry whose `branch` fails `WORKTREE_AGENT_BRANCH_RE`, or whose `worktree_path` was never actually created by `git worktree add` for this repo — real fixture, real git, asserts the foreign path/branch survives untouched | Security bullet: "arbitrary-worktree ownership attempts" | "the command never sweeps or deletes an unowned worktree" | +| 2 | `tests/gsd-quick-batch-merge-integration.test.cjs` | "a committed path outside declared files_modified merges successfully with an advisory SCOPE_OUT_OF_DECLARED warning, never blocked" | `planWaveScopeConformance`'s advisory (non-blocking) behavior holds end-to-end through a REAL git diff via `executeWorktreeWaveCleanupPlan` — distinguishes advisory scope drift from the blocking undeclared-DELETION case already covered | Scheduling bullet: "scope drift" | "reports scope drift without silently merging it" | +| 3 | `tests/gsd-quick-batch-merge-integration.test.cjs` | "a repo with `.gitmodules` and an unrelated plan merges cleanly through the quick-batch cleanup primitive" + "...and a plan touching the submodule path also merges (isolation-disable is a separate, already-covered pre-dispatch decision)" | `executeWorktreeWaveCleanupPlan` handles a real `.gitmodules`-bearing repo without special-casing gitlink entries (mode 160000) incorrectly as an undeclared deletion or scope violation | Scheduling bullet: "submodules" | "submodule-touch... paths produce recoverable diagnostics" | +| 4 | `tests/gsd-quick-batch-workflow.test.cjs` | "worktree-dispatch.md excludes an item whose SUMMARY.md already exists from this round's spawn set (crash-window duplicate-dispatch guard)" | Structural regression test: the Step-6→Step-7 crash window (design §1) cannot silently regress — asserts the new SUMMARY.md-existence filter text exists in the prose, mirroring the file's own established pattern for asserting `planner-wave.md`'s PLAN.md-existence filter | Fault-injection bullet: "every durable manifest/STATE crash window" | "Base divergence, merge conflict, stale worktree, submodule-touch, and interrupted cleanup paths produce recoverable diagnostics" | +| 5 | `docs/how-to/batch-quick-tasks.md` (doc, not test) | "Diagnosing a preserved worktree" subsection | Extends the one-sentence mention into: where the worktree lives, what to check (`git log`, `git status`, the item's `SUMMARY.md`), how to manually merge/discard, how to re-run `--resume` afterward | Docs bullet: "preserved-worktree diagnosis" | "Base divergence... produce recoverable diagnostics" | +| 6 | `.gsd/phase/feat-3677-quick-batch-hardening-acceptance/60-acceptance-evidence.md` (doc, not test) | Full AC-to-evidence mapping | Every #3344 AC bullet cited against the specific test/doc/commit that satisfies it | "Final verification maps evidence to every acceptance criterion in #3344" | (self) | + +## Boundary coverage note + +Row 1 (ownership tampering) exercises the boundary at the branch-name +regex: a NEAR-miss branch name (fails `WORKTREE_AGENT_BRANCH_RE` by one +character / wrong prefix) is the primary hostile case — the "arbitrary" +part of "arbitrary-worktree ownership attempts" means attacker-controlled +manifest content, not merely a wrong-but-well-formed path, so the test +constructs BOTH a plausible-but-foreign branch name (passes the regex, but +was never created by `git worktree add` in this fixture) and a +regex-rejected name, asserting each is handled (rejected, not silently +adopted). + +Row 2 (scope drift) exercises limit-1/limit/limit+1 analogously to the +existing undeclared-deletion test's two-case (declared vs undeclared) +shape: `files_modified` declares path A only; the real commit touches A +(no warning), A+B (one warning, B), and touches ONLY B (declared list +non-empty but wrong — still advisory, still merges, still warns). + +## Property-based testing note + +No new `fast-check` property test is added this phase — `40-design.md` §2 +found no bijective/parser/budget-limit contract among the new gaps (an +existing-fixture merge/cleanup primitive, a doc extension, and a structural +workflow-prose assertion). `quick-batch.property.test.cjs` and +`quick-batch-dispatch.property.test.cjs` already carry this epic's `fast-check` +coverage for the parsing/budget/wave-partition contracts these new tests +build on top of; none of that is touched or needs new properties. diff --git a/.gsd/phase/feat-3677-quick-batch-hardening-acceptance/60-acceptance-evidence.md b/.gsd/phase/feat-3677-quick-batch-hardening-acceptance/60-acceptance-evidence.md new file mode 100644 index 000000000..2323098aa --- /dev/null +++ b/.gsd/phase/feat-3677-quick-batch-hardening-acceptance/60-acceptance-evidence.md @@ -0,0 +1,92 @@ +# Final acceptance evidence — epic #3344 (`/gsd:quick-batch`) + +Maps every acceptance-criterion bullet in #3344 to its evidence. Phases 1-4 +(#4190, #4212, and their prerequisite dispatch/wave-partitioner PRs) are +cited by test file — those PRs' own `60-review.json` verdicts are the +authoritative record for anything not re-verified in this phase. Phase 5 +(#3677, this PR) entries are marked **NEW** and cite the file/test added +here. #3344 itself stays OPEN after this PR merges, per its own instruction +("Epic #3344 closes only after final maintainer acceptance") — this table +is the evidence submitted for that acceptance, not a closure action. + +## Command and artifacts + +| AC bullet | Evidence | +|---|---| +| Accepts ≥2 inline tasks or a validated repo-relative task file | `tests/quick-batch.test.cjs` (`parseTaskList`/`parseTaskListFromFile`), `tests/gsd-quick-batch-workflow.test.cjs` | +| `--jobs auto\|N`, `--validate`, `--research`, `--resume ` documented | `commands/gsd/quick-batch.md` frontmatter tests; `docs/how-to/batch-quick-tasks.md` "Flags" | +| V1 rejects `--discuss`/`--full` before dispatch | `tests/gsd-quick-batch-workflow.test.cjs` "objective documents --discuss/--full as rejected" | +| No automatic broad-prompt decomposition | `docs/how-to/batch-quick-tasks.md` "Basic use" (explicit list only) | +| Collision-safe quick ID/directory even for duplicates | `tests/quick-batch.test.cjs` (`allocateQuickIds`/`allocateIdsGivenUsed`), `quick-batch.property.test.cjs` | +| Normal quick PLAN + `status: complete` SUMMARY under `.planning/quick/`, audit-scanner-recognized | `tests/gsd-quick-batch-quick-regression.test.cjs` | +| Batch control state under `.planning/quick-batches//`, not a fake quick dir | `tests/quick-batch.test.cjs` (`createBatch`/`batchManifestPath`) | +| Coordinator dispatches leaves directly; no child invokes `/gsd:quick`; no nested/background delegation required | `tests/gsd-quick-batch-workflow.test.cjs` "single-writer invariant on the executor" (`NEVER invoke /gsd:quick`) | + +## Capacity and scheduling + +| AC bullet | Evidence | +|---|---| +| Normalized worker-capacity signal; `dispatch-capacity` single consumer | Phase 1/2 (#3673/#3674) host-integration + `quick-batch-dispatch.test.cjs` | +| Effective concurrency bounded by task count / `--jobs` / capacity | `tests/quick-batch-dispatch.test.cjs`, `.property.test.cjs` (`effective-concurrency`) | +| Missing/invalid/zero/negative/`undocumented` capacity fails closed to 1 | `tests/quick-batch-dispatch.test.cjs` | +| No runtime-name literal branches in scheduler | `src/quick-batch.cts`/`src/quick-batch-dispatch.cts` — no `RUNTIME ===`/`runtime name` conditionals (grep-verified during Phase 3/4 review) | +| Backpressure preserves pending state, never overspawns | `tests/quick-batch-dispatch.test.cjs` (`spawn-plan`) | +| Declared dependencies honored; overlapping file sets never co-wave | `tests/quick-batch.test.cjs` (`computeWaves`), `.property.test.cjs` | +| Independent non-overlapping tasks run concurrently when capacity allows | `tests/quick-batch.test.cjs` wave tests | +| Deterministic wave/merge ordering for identical input | `tests/quick-batch.test.cjs` + `tests/gsd-quick-batch-merge-integration.test.cjs` (wave-order-preserving merge) | + +## Isolation and Git safety + +| AC bullet | Evidence | +|---|---| +| `harness-worktree`/`orchestrator-worktree`/`none` each behaviorally tested, fail-closed degradation | `tests/gsd-quick-batch-workflow.test.cjs` "isolation model coverage"; `gsd-core/workflows/quick-batch/steps/worktree-dispatch.md` (row 38 stale-base auto-degrade) | +| Mutating wave forced to 1 worker when isolation is `none` | `tests/quick-batch-dispatch.test.cjs` (`--mutating` forcing) | +| Worktree create/merge/cleanup serialized and manifest-scoped; never sweeps/deletes an unowned worktree | `tests/gsd-quick-batch-merge-integration.test.cjs` — **NEW/REVISED (review pass 2)**: "arbitrary-worktree ownership tampering" describe block, 3 tests: (1) tampered branch name silently dropped at normalization; (2) foreign, never-registered repo blocked via base_mismatch; (3) the STRONGER proof — two REAL, concurrently-alive sibling worktrees of the SAME repo, path/branch swapped between them, blocked via branch_mismatch in both directions, both survive untouched. Investigation concluded the swap is not a reachable gap (git's own branch-per-worktree uniqueness + globally collision-checked quick_id-derived branch names) — see `40-design.md` §9.2. | +| Later waves start from the current merged batch base | `gsd-core/workflows/quick-batch/steps/merge-wave.md` Step 7 design; `tests/quick-batch.test.cjs` wave recompute tests | +| Merge applies committed-diff-vs-declared-scope validation; reports scope drift without silently merging it | `tests/gsd-quick-batch-merge-integration.test.cjs` (undeclared-DELETION blocking, Phase 4) **+ NEW**: "advisory scope drift merges but warns" describe block (drift produces `scope_out_of_declared` warning + still merges; exact-match boundary case produces zero warnings) | +| Child workers never concurrently switch the main checkout; ≤1 aggregate branch | `gsd-core/workflows/quick-batch/steps/worktree-dispatch.md` "ONE AT A TIME" serialization test | +| Each merged item retains atomic implementation commit history | `tests/gsd-quick-batch-merge-integration.test.cjs` (`--no-ff` real-git merges preserve commit history) | +| Base divergence, merge conflict, stale worktree, submodule-touch, interrupted cleanup produce recoverable diagnostics | Base divergence: `resumeBatch`'s `currentBaseRevision` check (`tests/quick-batch.test.cjs`). Merge conflict: `tests/gsd-quick-batch-merge-integration.test.cjs` (Phase 4). Stale worktree / interrupted cleanup: `worktree-safety.test.cjs`'s existing mid-merge halt coverage (unchanged this phase) + `docs/how-to/batch-quick-tasks.md` **NEW** "Diagnosing a preserved worktree" subsection. Submodule-touch: `tests/gsd-quick-batch-merge-integration.test.cjs` **NEW** ".gitmodules submodule integration" describe block (3 tests: unrelated plan merges cleanly with `.gitmodules` present; a real gitlink pointer bump merges and the superproject tree reflects the new pinned commit; an undeclared bump is advisory-only and surfaces a `vendor/sub` scope warning) | + +## State, failure, and resume + +| AC bullet | Evidence | +|---|---| +| Leaves cannot write shared STATE.md/ROADMAP.md; coordinator is sole writer | `tests/gsd-quick-batch-workflow.test.cjs` single-writer invariant test | +| Completed rows appended atomically, exactly once, including after resume | `src/quick-batch.cts` `completeQuickItem`/`hasQuickTaskRow`; `tests/quick-batch.test.cjs` idempotency tests | +| Failure of one item doesn't roll back others | `tests/quick-batch-dispatch.test.cjs` routing isolation tests; `worktree-safety.cts`'s `#2852` per-entry isolation (`blockEntry`/`continue`) | +| Failed item recorded failed; dependents recorded blocked; unrelated pending continue | `tests/quick-batch.test.cjs` `resumeBatch` blocked-propagation fixed-point tests | +| `--resume` skips complete, retries only eligible non-complete, idempotent | `tests/quick-batch.test.cjs` `resumeBatch` tests **+ NEW fix**: `gsd-core/workflows/quick-batch/steps/worktree-dispatch.md` Step 6 substep 1 crash-window guard (a `pending` item whose `SUMMARY.md` already exists on disk — executor finished, coordinator crashed before Step 7's merge — is no longer re-dispatched into a second worktree on `--resume`; it falls through to Step 7's existing SUMMARY.md-on-disk merge criterion instead). Regression-tested structurally in `tests/gsd-quick-batch-workflow.test.cjs` "crash-window duplicate-dispatch guard" describe block (3 tests: guard text present, guard positioned before `spawn-plan` is computed, guard documents the merge-wave.md recovery path). **This was the one genuine functional gap found in this phase** — see `.gsd/phase/feat-3677-quick-batch-hardening-acceptance/40-design.md` §1 for the full trace. | +| Invalid/corrupt batch manifest fails closed, no guessing | `tests/quick-batch.test.cjs` `loadBatch` malformed-JSON tests | + +## Security, portability, and project quality + +| AC bullet | Evidence | +|---|---| +| `--file` rejects traversal/symlink-escape/special files/outside-root; task text untrusted in prompts | `tests/quick-batch.test.cjs`/`.property.test.cjs` (Phase 3/4 review-fixed findings); `tests/gsd-quick-batch-workflow.test.cjs` DATA_START/DATA_END boundary tests | +| Paths with spaces, duplicate slugs, Windows separators, BSD/GNU differences, non-ASCII descriptions | `tests/quick-batch.test.cjs`/`.property.test.cjs` (slug generation, cross-platform path tests) | +| Workflow stays within size budget via shared fragments | `tests/gsd-quick-batch-workflow.test.cjs` byte-size boundary describe block (NEW_FILE_CAP); this phase's `worktree-dispatch.md` addition re-verified at 9.8KB, well under cap | +| Capacity axis backward-compatible, fails closed when absent | Phase 1/2 host-integration descriptor tests | +| #2652/PR #2728 resolved; no `RUNTIME != "claude"` gate copied | `gsd-core/references/dispatch-isolation-gate.md` reuse (Phase 4 design) | +| Canonical command Markdown / generated skill byte-for-byte synced | Repo-wide generated-sync lint (unchanged this phase; no new command surface) | +| Utility-cluster/profile/surface/inventory/installer/generated-artifact/doc parity | Repo-wide parity tests (unchanged this phase) | +| TDD/failing-test-first; Markdown behavior tested where applicable | Every new test in this phase written RED-first per its own commit; `tests/gsd-quick-batch-workflow.test.cjs`'s established structural-assertion convention for workflow prose | +| `npm test`, `npm run lint:ci`, generated-sync, required platform lanes pass | Verified via `gsd-test` before this PR ships (not run as part of this dispatch — reported separately) | + +## #3677's own acceptance criteria (this phase issue) + +| AC bullet | Evidence | +|---|---| +| Security: traversal/symlink/special-file/prompt-injection/shell-metachar/manifest-tampering/**arbitrary-worktree-ownership** | Manifest tampering + arbitrary-worktree ownership: **NEW**, this phase (see table above). Rest: Phase 3/4, unchanged. | +| Capacity: precedence/modes/backpressure | Phase 3, unchanged (`quick-batch-dispatch.test.cjs`) | +| Scheduling/execution: cycles/unknown-deps/waves/capacity/isolation/lifecycle/merge-order/conflicts/**scope-drift**/stale-bases/**submodules** | Scope drift + submodules: **NEW**, this phase. Rest: Phase 3/4, unchanged. | +| Fault-injection: every durable manifest/STATE crash window, resume exactly-once | **NEW fix + test**, this phase (crash-window guard, see above) — the STATE-row crash window was already covered in Phase 3. Review pass 2 upgraded the regression test from a prose-only proxy to a real fixture (`tests/quick-batch.test.cjs`, real `BATCH.json`+`SUMMARY.md`+`resumeBatch`), extracted the decision into a pure `filterAlreadyExecuted` function + `quick-batch filter-executed` CLI verb, and closed a self-discovered follow-on gap (durable worktree-path/branch recovery for a resumed coordinator process — `dispatched_worktree`/`dispatched_branch`/`dispatched_base`). See `40-design.md` §9.1/§9.3. | +| Outcome propagation; blocked dependents; independent continuation | Phase 3, unchanged | +| Docs: v1 limits | Phase 4, unchanged (`docs/how-to/batch-quick-tasks.md`) | +| Generated artifact sync | Repo-wide, unchanged (no new command surface this phase) | +| `/gsd:quick` regression + quick-ID grammar green | `tests/gsd-quick-batch-quick-regression.test.cjs`, unchanged | +| Docs: **preserved-worktree diagnosis** | **NEW**, this phase — `docs/how-to/batch-quick-tasks.md` "Diagnosing a preserved worktree" subsection | +| Focused tests / `npm test` / `npm run lint:ci` / generated-sync / install / platform lanes green | Verified via `gsd-test` before this PR ships | +| Real-PR-number changeset; no hand-edited CHANGELOG | Added after PR creation, per repo convention | +| Final verification maps evidence to #3344 | This document | +| RED/GREEN/REFACTOR commits; closes #3677 only | This PR's commit history; PR body closes #3677, references #3344 without closing it | diff --git a/docs/how-to/batch-quick-tasks.md b/docs/how-to/batch-quick-tasks.md index 228625610..7a3068fb3 100644 --- a/docs/how-to/batch-quick-tasks.md +++ b/docs/how-to/batch-quick-tasks.md @@ -117,6 +117,49 @@ runnable: Items unrelated to a failure continue normally in the same or a later batch run — one item's problem never blocks the rest of the batch. +### Diagnosing a preserved worktree + +When a merge conflicts or a committed diff includes an undeclared file +deletion, the coordinator preserves that item's worktree instead of deleting +it, so you can inspect exactly what the executor did: + +1. **Find it.** The preserved directory is + `/../--wt/` (or wherever your runtime's + worktree layout places it) — the item's own quick directory, + `.planning/quick/-/`, still has the `PLAN.md` the + executor was given, which tells you what it was trying to do. +2. **See what actually changed.** From the preserved worktree: + `git log ..HEAD` shows the executor's real commit(s); + `git diff ...HEAD --stat` shows exactly which files it touched + (compare that against `PLAN.md`'s declared `files_modified` if you want + to confirm whether the failure was a genuine conflict or an + out-of-scope change). +3. **Read the item's own `SUMMARY.md`** in its quick directory — the + executor wrote it before the merge was attempted, so it still describes + what the executor believed it accomplished, independent of whether the + merge itself succeeded. +4. **Decide how to resolve it:** + - If the work is good and only the automated merge failed (a real + conflict, or a deletion that should have been declared): merge the + worktree's branch by hand (`git merge --no-ff`), resolve any + conflicts, then remove the worktree yourself + (`git worktree remove --force`) and its branch + (`git branch -D `). + - If the work should be discarded: remove the worktree and branch the + same way, without merging. + - If a coordinator crash left the item's `SUMMARY.md` written but its + `BATCH.json` status still `pending` (the executor finished before the + coordinator process crashed, before the merge step ran) — this is NOT + a preserved-worktree failure and needs no manual merge. `--resume` + recognizes the on-disk `SUMMARY.md` and routes the item straight to + the merge step on its own; it will not re-dispatch a second executor + for it. +5. **Re-run** `/gsd-quick-batch --resume ` once you're done. A + `failed`/`merge_failed`/`scope_violation` item you resolved manually + (merged and cleaned up yourself) is picked up as already-merged on the + next resume; an item you decided to abandon stays `failed` and is + skipped. + --- ## Related diff --git a/gsd-core/workflows/quick-batch/steps/merge-wave.md b/gsd-core/workflows/quick-batch/steps/merge-wave.md index 66b907ac4..94509cd7c 100644 --- a/gsd-core/workflows/quick-batch/steps/merge-wave.md +++ b/gsd-core/workflows/quick-batch/steps/merge-wave.md @@ -45,7 +45,24 @@ _GSD_SHIM_NAME="gsd-tools.cjs"; _GSD_RUNTIME_ROOT="${RUNTIME_DIR:-$(git rev-pars done ``` (`$WT_PATH`/`$WT_BRANCH`/`$EXPECTED_BASE` per item come from the recorded - `$QUICK_BATCH_WORKTREE_MANIFEST` entry Step 6 wrote for that `agent_id`.) + `$QUICK_BATCH_WORKTREE_MANIFEST` entry Step 6 wrote for that `agent_id` + THIS process, when present. + + **Durable fallback (#3677):** for an item Step 6 did NOT dispatch this + process — the crash-window guard correctly skipped it because + `SUMMARY.md` already existed from a PRIOR, now-dead coordinator process — + `$QUICK_BATCH_WORKTREE_MANIFEST` has no entry for it at all (it is a + fresh per-process `mktemp` file). Read `$WT_PATH`/`$WT_BRANCH`/ + `$EXPECTED_BASE` from that item's OWN durable + `dispatched_worktree`/`dispatched_branch`/`dispatched_base` fields in + `$BATCH_MANIFEST_JSON` instead — persisted by Step 6's own durable- + persistence step at the time it actually created the worktree, in + whichever process that was. If ALL THREE are still `null` (the item was + never durably recorded — should not happen once Step 6 always persists + on dispatch, but fail closed rather than guess): route this entry via + `merge-routing --kind merge_failed --detail "missing durable worktree + record"` the same as any other blocked entry below, and do NOT attempt + the cleanup-wave call for it.) 4. **Merge, one at a time, via the SAME bounded primitive every other worktree consumer uses** (never hand-roll `git merge`): @@ -61,7 +78,13 @@ _GSD_SHIM_NAME="gsd-tools.cjs"; _GSD_RUNTIME_ROOT="${RUNTIME_DIR:-$(git rev-pars 5. **Route each entry's result:** - `status == "merged_removed"`: success. Mark the item's completion pending (Step 9 calls `quick-batch complete` for it — do NOT call it here; a - `--validate` item still has verification ahead of it). + `--validate` item still has verification ahead of it). **Clear the + durable worktree-recovery fields now (#3677)** — the worktree no + longer exists on disk, so its `dispatched_worktree`/`dispatched_branch`/ + `dispatched_base` must not keep pointing at a removed path: + ```bash + gsd_run quick-batch update --batch "$BATCH_ID" --updates '[{"quickId":"'"$quick_id"'","dispatchedWorktree":null,"dispatchedBranch":null,"dispatchedBase":null}]' + ``` - Any other status: route via ```bash gsd_run quick-batch merge-routing --kind merge_failed --detail "$reason" --raw diff --git a/gsd-core/workflows/quick-batch/steps/worktree-dispatch.md b/gsd-core/workflows/quick-batch/steps/worktree-dispatch.md index ffa63a80d..b16278c56 100644 --- a/gsd-core/workflows/quick-batch/steps/worktree-dispatch.md +++ b/gsd-core/workflows/quick-batch/steps/worktree-dispatch.md @@ -37,6 +37,36 @@ EXEC_CONCURRENCY=$(printf '%s' "$QB_EXEC_CONC_JSON" | node -e 'let s="";process. Parse `eligible` (quick ids ready to execute — every dependency already `complete`) and refresh `$BATCH_MANIFEST_JSON` from its `manifest`. + **Crash-window guard (mirrors `planner-wave.md`'s PLAN.md-existence + check one layer earlier — #3677):** `quick-batch resume`'s `eligible` is + purely status/dependency-derived; it does NOT know an item already + finished executing. A coordinator crash between this step returning + (executor committed, `SUMMARY.md` written) and Step 7's merge leaves + `BATCH.json` at `pending` with no STATE.md row yet (that row is written + only in Step 9) — so on `--resume`, such an item still comes back + `eligible` here. Before computing `spawn-plan`, determine which eligible + items already have `${item_dir}/${quick_id}-SUMMARY.md` on disk (same + `item_dir` derivation via `generate-slug` every other step uses), then + let the PURE `quick-batch filter-executed` verb + (`filterAlreadyExecuted`, `src/quick-batch-dispatch.cts`) decide which + ids are actually safe to spawn — never re-derive that split inline: + ```bash + EXECUTED_IDS_JSON="[]" # JSON array of quick_ids whose SUMMARY.md already exists on disk this round + QB_FILTER_JSON=$(gsd_run quick-batch filter-executed --eligible "$ELIGIBLE_IDS_JSON" --executed "$EXECUTED_IDS_JSON" --raw) + ``` + Parse `spawnEligible` (safe to spawn this round) and `alreadyExecuted` + (diagnostic only — report these as "already executed, routing to merge" + rather than dispatching them). Replace `$ELIGIBLE_IDS_JSON` with + `spawnEligible` before continuing to backpressure below. + + NEVER re-dispatch it into a second worktree for any id `filter-executed` + returns in `alreadyExecuted`. It is not lost: Step 7's own mergeable-wave + criterion + (`gsd-core/workflows/quick-batch/steps/merge-wave.md` Step 1) already + picks up any `pending` item with an on-disk `SUMMARY.md` that isn't yet + merged, independent of this eligible/spawn list — dropping it here only + prevents the duplicate dispatch, it does not remove it from the batch. + 2. **Backpressure.** Not every eligible item necessarily spawns this round — cap fan-out at `$EXEC_CONCURRENCY` minus current in-flight count (row 27/39): @@ -113,6 +143,21 @@ EXEC_CONCURRENCY=$(printf '%s' "$QB_EXEC_CONC_JSON" | node -e 'let s="";process. primary checkout (same prompt, minus the worktree-only framing), one item at a time — `EXEC_CONCURRENCY` is already forced to 1 in this mode. + **Durable worktree-recovery persistence (#3677 — `harness-worktree`/ + `orchestrator-worktree` only, skip for `none`):** immediately after + recording `{agent_id, worktree_path, branch, expected_base}` into the + EPHEMERAL `$QUICK_BATCH_WORKTREE_MANIFEST` above, ALSO persist the same + triple durably onto this item in `BATCH.json`: + ```bash + gsd_run quick-batch update --batch "$BATCH_ID" --updates '[{"quickId":"'"$quick_id"'","dispatchedWorktree":"'"$worktree_path"'","dispatchedBranch":"'"$branch"'","dispatchedBase":"'"$expected_base"'"}]' + ``` + The ephemeral manifest is a per-process `mktemp` file (same shape + `/gsd:quick`'s own `QUICK_WORKTREE_MANIFEST` uses) — it does NOT survive + a coordinator crash/restart. A RESUMED coordinator's Step 7 + (`merge-wave.md`) reads this durable BATCH.json triple as its fallback + for any item the crash-window guard above correctly did NOT re-dispatch + in the current process. + > **ORCHESTRATOR RULE — CODEX RUNTIME**: after each `Agent()` call above, wait for it to return before starting the next worktree create. 4. **After every item dispatched this round returns:** verify diff --git a/src/quick-batch-command-router.cts b/src/quick-batch-command-router.cts index 3c051e8dd..40e88a464 100644 --- a/src/quick-batch-command-router.cts +++ b/src/quick-batch-command-router.cts @@ -60,6 +60,7 @@ interface QuickBatchDispatchModule { routeVerificationOutcome(status: string): unknown; routeMergeOutcome(outcome: Record): unknown; buildCleanupManifestEntry(input: Record): unknown; + filterAlreadyExecuted(eligibleIds: string[], executedIds: string[]): unknown; } interface RouteQuickBatchCommandOptions { @@ -122,6 +123,7 @@ function routeQuickBatchCommand({ args, cwd, raw, error, _quickBatch, _quickBatc 'effective-concurrency', 'merge-eligible', 'spawn-plan', + 'filter-executed', 'verification-routing', 'merge-routing', 'cleanup-entry', @@ -236,6 +238,19 @@ function routeQuickBatchCommand({ args, cwd, raw, error, _quickBatch, _quickBatc } output(dispatch.computeSpawnPlan({ eligibleIds: eligibleResult.value, capacity, currentInFlight, refused }), raw); }, + // `quick-batch filter-executed --eligible --executed ` + // Crash-window duplicate-dispatch guard (#3677): splits this round's + // eligible ids into `spawnEligible` (safe to dispatch) and + // `alreadyExecuted` (SUMMARY.md already exists on disk — caller + // determines this via its own filesystem check; NEVER re-dispatch + // these — merge-wave.md's own on-disk criterion picks them up). + 'filter-executed': () => { + const eligibleResult = parseJsonArg(argValue(args, '--eligible'), '--eligible'); + if (!eligibleResult.ok) return makeInvalidArgs('--eligible', eligibleResult.reason, ERROR_REASON.USAGE); + const executedResult = parseJsonArg(argValue(args, '--executed'), '--executed'); + if (!executedResult.ok) return makeInvalidArgs('--executed', executedResult.reason, ERROR_REASON.USAGE); + output(dispatch.filterAlreadyExecuted(eligibleResult.value, executedResult.value), raw); + }, // `quick-batch verification-routing --status ` 'verification-routing': () => { const status = argValue(args, '--status'); diff --git a/src/quick-batch-dispatch.cts b/src/quick-batch-dispatch.cts index 968224c58..b33b849ab 100644 --- a/src/quick-batch-dispatch.cts +++ b/src/quick-batch-dispatch.cts @@ -261,6 +261,50 @@ function routeMergeOutcome(outcome: MergeOutcomeKind): MergeRouting { } } +// ─── Crash-window duplicate-dispatch guard (#3677, epic #3344 Phase 5) ───── + +interface FilterAlreadyExecutedResult { + /** Eligible ids NOT known to have already finished executing — safe to spawn this round. */ + spawnEligible: string[]; + /** + * Eligible ids whose `SUMMARY.md` already exists on disk — the CALLER + * determines this via its own filesystem check (this function performs + * no I/O). NEVER re-dispatch one of these into a second worktree: + * `merge-wave.md`'s own on-disk `SUMMARY.md` criterion already picks + * each of them up independently of this eligible/spawn list. + */ + alreadyExecuted: string[]; +} + +/** + * Crash-window duplicate-dispatch guard + * (`.gsd/phase/feat-3677-quick-batch-hardening-acceptance/40-design.md` §1). + * `quick-batch resume`'s `eligible` is purely status/dependency-derived — + * it has no awareness that an item already finished executing (a real + * commit, `SUMMARY.md` written) before a coordinator crash left + * `BATCH.json` at `pending` (the STATE.md-row crash-window detection + * inside `resumeBatch` only fires once Step 9 has run). This mirrors the + * file-existence exclusion `planner-wave.md` already applies one layer + * earlier for `PLAN.md`, extracted as its own pure decision (rather than + * left as workflow prose only) so it is independently testable: given the + * eligible ids for this round and the subset the CALLER has already + * determined finished executing, splits them into ids safe to spawn now + * and ids that must never be re-dispatched. + */ +function filterAlreadyExecuted(eligibleIds: string[], executedIds: string[] | Set): FilterAlreadyExecutedResult { + const executedSet = executedIds instanceof Set ? executedIds : new Set(executedIds); + const spawnEligible: string[] = []; + const alreadyExecuted: string[] = []; + for (const id of eligibleIds) { + if (executedSet.has(id)) { + alreadyExecuted.push(id); + } else { + spawnEligible.push(id); + } + } + return { spawnEligible, alreadyExecuted }; +} + // ─── Cleanup-wave manifest entry construction (design row 26, Open Q2) ───── interface CleanupEntryInput { @@ -319,6 +363,7 @@ const quickBatchDispatch = { routeVerificationOutcome, routeMergeOutcome, buildCleanupManifestEntry, + filterAlreadyExecuted, }; // eslint-disable-next-line @typescript-eslint/no-namespace @@ -334,6 +379,7 @@ declare namespace quickBatchDispatch { MergeRouting, CleanupEntryInput, CleanupManifestEntry, + FilterAlreadyExecutedResult, }; } diff --git a/src/quick-batch.cts b/src/quick-batch.cts index 9442f5651..4b48f0e3f 100644 --- a/src/quick-batch.cts +++ b/src/quick-batch.cts @@ -75,6 +75,12 @@ interface QuickBatchItemInput { plannedFiles?: string[]; directory?: string | null; worktree?: string | null; + /** #3677: the item's dispatched worktree path, when known at creation (normally set later via `updateBatchItems`). */ + dispatchedWorktree?: string | null; + /** #3677: the item's dispatched worktree branch, when known at creation (normally set later via `updateBatchItems`). */ + dispatchedBranch?: string | null; + /** #3677: the merge-base `dispatchedBranch` forked from, when known at creation (normally set later via `updateBatchItems`). */ + dispatchedBase?: string | null; } type QuickBatchItemStatus = 'pending' | 'complete' | 'failed' | 'blocked'; @@ -91,6 +97,29 @@ interface QuickBatchItem { planned_files: string[]; directory: string | null; worktree: string | null; + /** + * #3677: this item's dispatched worktree path, persisted durably (via + * `updateBatchItems`) once `worktree.create` succeeds — the crash-window + * duplicate-dispatch guard's recovery path. The ephemeral, per-process + * `$QUICK_BATCH_WORKTREE_MANIFEST` (a `mktemp` file, same shape + * `/gsd:quick`'s own `QUICK_WORKTREE_MANIFEST` uses) does NOT survive a + * coordinator crash/restart; this field is the durable fallback a + * RESUMED coordinator process reads when its own fresh ephemeral + * manifest has no entry for an item that finished executing in a PRIOR + * process (see `.gsd/phase/feat-3677-quick-batch-hardening-acceptance/40-design.md` §1). + * + * Deliberately a SEPARATE field from the pre-existing `worktree` above, + * not a reuse of it: `worktree`'s own `loadBatch` validation requires the + * path to exist on disk, which is fatal here — a successful merge + * legitimately REMOVES the worktree, and this field's whole purpose is to + * keep being readable (and clearable) after that removal. No existence + * check applies to this field or the two below. + */ + dispatched_worktree: string | null; + /** #3677: this item's dispatched worktree branch — same durable-fallback purpose as `dispatched_worktree` above. */ + dispatched_branch: string | null; + /** #3677: the merge-base `dispatched_branch` forked from — same durable-fallback purpose as `dispatched_worktree` above. */ + dispatched_base: string | null; /** Dependency-DAG + file-overlap wave index, assigned once at `createBatch` time. */ wave: number; /** Set by `completeQuickItem` alongside the STATE.md row it records. */ @@ -489,6 +518,9 @@ function createBatch( planned_files: (input.plannedFiles ?? []).map(posixNormalize), directory: input.directory ?? null, worktree: input.worktree ?? null, + dispatched_worktree: input.dispatchedWorktree ?? null, + dispatched_branch: input.dispatchedBranch ?? null, + dispatched_base: input.dispatchedBase ?? null, wave: -1, commit: null, failure_reason: null, @@ -587,6 +619,18 @@ function validateBatchSchema(parsed: unknown, batchId: string): Result { + test('a manifest entry naming a non-agent branch (e.g. the repo\'s own primary branch) is silently dropped at normalization and NEVER reaches git execution', () => { + const tmpBase = createTempDir('qb-ownership-tamper-'); + try { + const repoDir = path.join(tmpBase, 'repo'); + initRepo(repoDir); + const headBefore = git(['rev-parse', 'HEAD'], repoDir).trim(); + + // A tampered/corrupt manifest naming the repo's OWN primary branch as + // if it were a batch-owned worktree entry — the branch name fails + // WORKTREE_AGENT_BRANCH_RE (`^((worktree-)?agent-|worktree-wf_)...`), + // so it must never reach a git subprocess at all. + const tamperedManifest = { + worktrees: [{ + agent_id: 'tampered', + worktree_path: repoDir, + branch: 'main', + expected_base: headBefore, + }], + }; + + const plan = planWorktreeWaveCleanup(repoDir, tamperedManifest); + assert.equal(plan.ok, false, `a non-agent branch name must be rejected before a plan is built, got: ${JSON.stringify(plan)}`); + assert.equal(plan.reason, 'empty_manifest'); + assert.equal(plan.entries.length, 0); + + const result = executeWorktreeWaveCleanupPlan(plan); + assert.equal(result.ok, false); + assert.equal(result.entries.length, 0, 'no entry may reach git execution for a rejected manifest'); + + // repoRoot must be provably untouched — same HEAD, no merge commit. + const headAfter = git(['rev-parse', 'HEAD'], repoDir).trim(); + assert.equal(headAfter, headBefore, 'repo HEAD must not move when the only manifest entry was rejected at normalization'); + } finally { + cleanup(tmpBase); + } + }); + + test('a manifest entry naming a plausible agent-branch that was never actually created by this repo\'s own worktree.create is blocked (base_mismatch), never merged', () => { + const tmpBase = createTempDir('qb-ownership-foreign-'); + try { + const repoDir = path.join(tmpBase, 'repo'); + const foreignDir = path.join(tmpBase, 'foreign'); + initRepo(repoDir); + const headBefore = git(['rev-parse', 'HEAD'], repoDir).trim(); + + // A completely separate repository — never created via `git worktree + // add` against repoDir — whose HEAD branch happens to be named + // plausibly (passes WORKTREE_AGENT_BRANCH_RE). This is the "arbitrary" + // ownership-tampering case: the manifest ENTRY looks legitimate, but + // nothing about worktree_path proves it is actually repoDir's own + // worktree. + fs.mkdirSync(foreignDir, { recursive: true }); + git(['init'], foreignDir); + git(['config', 'user.email', 'test@test.com'], foreignDir); + git(['config', 'user.name', 'Test'], foreignDir); + git(['config', 'commit.gpgsign', 'false'], foreignDir); + fs.writeFileSync(path.join(foreignDir, 'secret.txt'), 'not part of this batch\n'); + git(['add', '-A'], foreignDir); + git(['commit', '-m', 'foreign repo initial commit'], foreignDir); + git(['checkout', '-b', 'agent-hostile-1'], foreignDir); + + const plan = { + ok: true, + repoRoot: repoDir, + action: 'cleanup_wave', + discovery: 'manifest', + entries: [{ + agent_id: 'hostile', + worktree_path: foreignDir, + branch: 'agent-hostile-1', + expected_base: headBefore, + allowed_bases: [headBefore], + }], + }; + + const result = executeWorktreeWaveCleanupPlan(plan); + assert.equal(result.ok, false); + assert.equal(result.entries[0].status, 'blocked', `expected a blocked entry for a foreign worktree_path, got: ${JSON.stringify(result.entries[0])}`); + // repoDir has no local ref named `agent-hostile-1` (it was only ever + // created in the FOREIGN repo) — merge-base against it fails closed. + assert.equal(result.entries[0].reason, 'base_mismatch'); + + // Neither side was touched: the foreign repo's secret file is intact + // and untouched by any merge/remove, and repoDir's HEAD never moved. + assert.ok(fs.existsSync(path.join(foreignDir, 'secret.txt')), 'foreign repo must be left completely alone'); + const headAfter = git(['rev-parse', 'HEAD'], repoDir).trim(); + assert.equal(headAfter, headBefore, 'repoDir HEAD must not move for a blocked foreign entry'); + } finally { + cleanup(tmpBase); + } + }); + + // #3677 review pass 2 (Security finding): the two tests above prove + // branch-shape rejection and a wholly-foreign, never-registered repo are + // both handled — but neither exercises the scenario "arbitrary-worktree + // ownership" actually names: a manifest entry whose worktree_path/branch + // are SWAPPED to point at a DIFFERENT, GENUINELY-REGISTERED sibling + // worktree of the SAME repoRoot (a concurrent batch's own agent worktree, + // or a stale worktree from a prior crashed run), with a branch name that + // passes WORKTREE_AGENT_BRANCH_RE's shape check and a base that is + // legitimately in allowed_bases. + // + // Investigation conclusion (see + // `.gsd/phase/feat-3677-quick-batch-hardening-acceptance/40-design.md` + // §1's companion note): this is NOT a reachable gap in + // `executeWorktreeWaveCleanupPlan` itself. Git enforces branch-per- + // worktree uniqueness — the SAME branch cannot be checked out in two + // worktrees of one repo at once — so `worktree_path`'s ACTUAL checked-out + // branch (`git -C worktree_path rev-parse --abbrev-ref HEAD`, + // `src/worktree-safety.cts:1047`) can only equal a swapped-in + // `entry.branch` if that `entry.branch` is the SIBLING's own real, + // uniquely-generated branch name — which manifest tampering confined to + // ONE batch's own record has no way to know (branch names are + // `agent-[-]`-shaped, and `quick_id` allocation is + // collision-checked GLOBALLY across every existing quick task and batch, + // `src/quick-batch.cts` `collectExistingBatchQuickIds`). This test proves + // the boundary directly against TWO real, concurrently-alive sibling + // worktrees of the SAME repo, both created via real `git worktree add`, + // both with `WORKTREE_AGENT_BRANCH_RE`-passing names, both sharing the + // SAME merge-base — so base/branch-shape checks ALONE could not + // distinguish them if the primitive were naive; `branch_mismatch` is what + // actually does. + test('a manifest entry with one sibling worktree\'s real PATH but the OTHER sibling\'s real BRANCH name is blocked (branch_mismatch); both real, concurrently-alive worktrees survive untouched', () => { + const tmpBase = createTempDir('qb-ownership-sibling-swap-'); + try { + const repoDir = path.join(tmpBase, 'repo'); + const wt1Dir = path.join(tmpBase, 'wt-item1'); + const wt2Dir = path.join(tmpBase, 'wt-item2'); + const branch1 = 'agent-item1'; + const branch2 = 'agent-item2'; + + initRepo(repoDir); + const headBefore = git(['rev-parse', 'HEAD'], repoDir).trim(); + + // TWO real, concurrently-alive sibling worktrees of the SAME repo — + // e.g. two items in the same batch dispatch round, or one item's + // worktree from THIS batch and a stale one left by a prior crashed + // run. Both branch off the SAME base commit. + addWorktree(repoDir, wt1Dir, branch1); + addWorktree(repoDir, wt2Dir, branch2); + fs.writeFileSync(path.join(wt2Dir, 'item2-own-work.txt'), 'item 2 real, uncommitted-to-main work\n'); + git(['add', '-A'], wt2Dir); + git(['commit', '-m', 'item2: real work'], wt2Dir); + const item2CommitBefore = git(['rev-parse', 'HEAD'], wt2Dir).trim(); + + // Tampered/corrupted entry: intends item1's cleanup (branch1, + // item1's own base), but worktree_path has been swapped to point at + // item2's REAL, currently-in-use worktree. + const swappedPathPlan = { + ok: true, + repoRoot: repoDir, + action: 'cleanup_wave', + discovery: 'manifest', + entries: [{ + agent_id: 'agent-item1', + worktree_path: wt2Dir, + branch: branch1, + expected_base: headBefore, + allowed_bases: [headBefore], + }], + }; + const swappedPathResult = executeWorktreeWaveCleanupPlan(swappedPathPlan); + assert.equal(swappedPathResult.ok, false); + assert.equal(swappedPathResult.entries[0].status, 'blocked', `expected a blocked entry for a path/branch swap, got: ${JSON.stringify(swappedPathResult.entries[0])}`); + assert.equal(swappedPathResult.entries[0].reason, 'branch_mismatch', 'wt2Dir is really checked out on branch2, not branch1 — the swap cannot pass the branch check'); + + // The reverse swap is blocked the same way: item2's real path is + // untouched here too, so re-verify with wt1Dir/branch2 sharing the + // SAME base commit (no distinguishing signal except the branch + // check itself). + const reverseSwapPlan = { + ok: true, + repoRoot: repoDir, + action: 'cleanup_wave', + discovery: 'manifest', + entries: [{ + agent_id: 'agent-item2', + worktree_path: wt1Dir, + branch: branch2, + expected_base: headBefore, + allowed_bases: [headBefore], + }], + }; + const reverseSwapResult = executeWorktreeWaveCleanupPlan(reverseSwapPlan); + assert.equal(reverseSwapResult.entries[0].status, 'blocked'); + assert.equal(reverseSwapResult.entries[0].reason, 'branch_mismatch'); + + // Both real, concurrently-alive sibling worktrees survive completely + // untouched by either tampered attempt — no merge landed, no + // worktree/branch removed, item2's real work is exactly as it was. + const headAfter = git(['rev-parse', 'HEAD'], repoDir).trim(); + assert.equal(headAfter, headBefore, 'repoDir HEAD must not move for either blocked swap attempt'); + assert.ok(fs.existsSync(wt1Dir), 'sibling worktree 1 must survive untouched'); + assert.ok(fs.existsSync(wt2Dir), 'sibling worktree 2 must survive untouched'); + assert.equal(git(['rev-parse', '--abbrev-ref', 'HEAD'], wt1Dir).trim(), branch1, 'worktree 1 must still be on its own real branch, never repointed'); + assert.equal(git(['rev-parse', '--abbrev-ref', 'HEAD'], wt2Dir).trim(), branch2, 'worktree 2 must still be on its own real branch, never repointed'); + assert.equal(git(['rev-parse', 'HEAD'], wt2Dir).trim(), item2CommitBefore, 'item 2\'s real, uncommitted-to-main work must be exactly as it was — never merged, never lost, never attributed to item1'); + } finally { + cleanup(tmpBase); + } + }); +}); + +describe('quick-batch merge routing — advisory scope drift merges but warns (Scheduling AC)', () => { + test('a committed path outside declared files_modified merges successfully with an advisory scope_out_of_declared warning, never blocked', () => { + const tmpBase = createTempDir('qb-scope-drift-'); + try { + const repoDir = path.join(tmpBase, 'repo'); + const wtDir = path.join(tmpBase, 'wt-drift'); + const branchName = 'worktree-agent-drift'; + + initRepo(repoDir); + addWorktree(repoDir, wtDir, branchName); + const baseCommit = git(['merge-base', 'HEAD', branchName], repoDir).trim(); + + // The plan declared ONLY declared.txt, but the executor's real commit + // touches declared.txt AND an undeclared drifted.txt. + fs.writeFileSync(path.join(wtDir, 'declared.txt'), 'in scope\n'); + fs.writeFileSync(path.join(wtDir, 'drifted.txt'), 'NOT declared\n'); + git(['add', '-A'], wtDir); + git(['commit', '-m', 'touch declared.txt and drifted.txt'], wtDir); + + const plan = { + ok: true, + repoRoot: repoDir, + action: 'cleanup_wave', + discovery: 'manifest', + entries: [{ + agent_id: 'drift1', + worktree_path: wtDir, + branch: branchName, + expected_base: baseCommit, + files_modified: ['declared.txt'], + }], + }; + + const result = executeWorktreeWaveCleanupPlan(plan); + + // Advisory only: the merge still SUCCEEDS despite the drift. + assert.equal(result.entries[0].status, 'merged_removed', `scope drift must be advisory, not blocking — got: ${JSON.stringify(result.entries[0])}`); + assert.ok(!fs.existsSync(wtDir), 'worktree removed after a successful (advisory-only) merge'); + assert.ok(fs.existsSync(path.join(repoDir, 'drifted.txt')), 'the undeclared file is still merged in, not rejected'); + + // But the drift is surfaced, not silently swallowed. + assert.equal(result.warnings.length, 1); + assert.equal(result.warnings[0].code, 'scope_out_of_declared'); + assert.equal(result.warnings[0].path, 'drifted.txt'); + assert.equal(result.warnings[0].branch, branchName); + + const routing = routeMergeOutcome({ kind: 'merged' }); + assert.equal(routing.action, 'complete', 'a scope-drift warning must not change merge routing to a failure'); + } finally { + cleanup(tmpBase); + } + }); + + test('a fully-declared commit (files_modified matches exactly) merges with zero warnings — boundary against the drift case above', () => { + const tmpBase = createTempDir('qb-scope-nodrift-'); + try { + const repoDir = path.join(tmpBase, 'repo'); + const wtDir = path.join(tmpBase, 'wt-nodrift'); + const branchName = 'worktree-agent-nodrift'; + + initRepo(repoDir); + addWorktree(repoDir, wtDir, branchName); + const baseCommit = git(['merge-base', 'HEAD', branchName], repoDir).trim(); + + fs.writeFileSync(path.join(wtDir, 'declared.txt'), 'in scope\n'); + git(['add', '-A'], wtDir); + git(['commit', '-m', 'touch only declared.txt'], wtDir); + + const plan = { + ok: true, + repoRoot: repoDir, + action: 'cleanup_wave', + discovery: 'manifest', + entries: [{ + agent_id: 'nodrift1', + worktree_path: wtDir, + branch: branchName, + expected_base: baseCommit, + files_modified: ['declared.txt'], + }], + }; + + const result = executeWorktreeWaveCleanupPlan(plan); + assert.equal(result.entries[0].status, 'merged_removed'); + assert.equal(result.warnings.length, 0, 'an exact declared-scope match must produce no advisory warnings'); + } finally { + cleanup(tmpBase); + } + }); +}); + +describe('quick-batch merge routing — real .gitmodules submodule integration (Scheduling AC)', () => { + /** + * Builds `/vendor/sub` as a REAL git submodule (local file:// + * remote, no network) pinned at `pinnedCommit`, committed on repoDir's + * primary branch. Returns the two submodule commits so a test can bump + * between them inside a worktree branch — the real "submodule-touch" + * scenario #3344's AC names. + */ + function buildRepoWithSubmodule(tmpBase) { + const subDir = path.join(tmpBase, 'subsrc'); + fs.mkdirSync(subDir, { recursive: true }); + git(['init'], subDir); + git(['config', 'user.email', 'test@test.com'], subDir); + git(['config', 'user.name', 'Test'], subDir); + git(['config', 'commit.gpgsign', 'false'], subDir); + fs.writeFileSync(path.join(subDir, 'f.txt'), 'hello\n'); + git(['add', '-A'], subDir); + git(['commit', '-m', 'sub commit 1'], subDir); + const sub1 = git(['rev-parse', 'HEAD'], subDir).trim(); + fs.appendFileSync(path.join(subDir, 'f.txt'), 'world\n'); + git(['add', '-A'], subDir); + git(['commit', '-m', 'sub commit 2'], subDir); + const sub2 = git(['rev-parse', 'HEAD'], subDir).trim(); + + const repoDir = path.join(tmpBase, 'repo'); + initRepo(repoDir); + git(['-c', 'protocol.file.allow=always', 'submodule', 'add', subDir, 'vendor/sub'], repoDir); + // Pin the just-added submodule checkout to sub1 so the worktree branch + // below has a REAL pointer bump (sub1 -> sub2) to commit, not a no-op. + git(['checkout', sub1], path.join(repoDir, 'vendor', 'sub')); + git(['add', 'vendor/sub', '.gitmodules'], repoDir); + git(['commit', '-m', 'add submodule pinned at sub1'], repoDir); + + return { repoDir, sub1, sub2 }; + } + + test('a repo with .gitmodules and a plan that never touches the submodule merges cleanly through the cleanup primitive', () => { + const tmpBase = createTempDir('qb-submodule-untouched-'); + try { + const { repoDir } = buildRepoWithSubmodule(tmpBase); + const wtDir = path.join(tmpBase, 'wt-unrelated'); + const branchName = 'worktree-agent-sub-unrelated'; + addWorktree(repoDir, wtDir, branchName); + const baseCommit = git(['merge-base', 'HEAD', branchName], repoDir).trim(); + + fs.writeFileSync(path.join(wtDir, 'unrelated.txt'), 'nothing to do with the submodule\n'); + git(['add', '-A'], wtDir); + git(['commit', '-m', 'touch unrelated.txt only'], wtDir); + + const plan = { + ok: true, + repoRoot: repoDir, + action: 'cleanup_wave', + discovery: 'manifest', + entries: [{ + agent_id: 'sub-unrelated', + worktree_path: wtDir, + branch: branchName, + expected_base: baseCommit, + files_modified: ['unrelated.txt'], + }], + }; + + const result = executeWorktreeWaveCleanupPlan(plan); + assert.equal(result.entries[0].status, 'merged_removed', `a .gitmodules-bearing repo must not break an unrelated merge — got: ${JSON.stringify(result.entries[0])}`); + assert.equal(result.warnings.length, 0); + } finally { + cleanup(tmpBase); + } + }); + + test('a real submodule pointer bump (sub1 -> sub2) merges cleanly and the superproject tree reflects the new pinned commit', () => { + const tmpBase = createTempDir('qb-submodule-bump-'); + try { + const { repoDir, sub2 } = buildRepoWithSubmodule(tmpBase); + const wtDir = path.join(tmpBase, 'wt-bump'); + const branchName = 'worktree-agent-sub-bump'; + addWorktree(repoDir, wtDir, branchName); + const baseCommit = git(['merge-base', 'HEAD', branchName], repoDir).trim(); + + git(['-c', 'protocol.file.allow=always', 'submodule', 'update', '--init'], wtDir); + git(['checkout', sub2], path.join(wtDir, 'vendor', 'sub')); + git(['add', 'vendor/sub'], wtDir); + git(['commit', '-m', 'bump vendor/sub to sub2'], wtDir); + + const plan = { + ok: true, + repoRoot: repoDir, + action: 'cleanup_wave', + discovery: 'manifest', + entries: [{ + agent_id: 'sub-bump', + worktree_path: wtDir, + branch: branchName, + expected_base: baseCommit, + files_modified: ['vendor/sub'], + }], + }; + + const result = executeWorktreeWaveCleanupPlan(plan); + assert.equal(result.entries[0].status, 'merged_removed', `a real gitlink pointer bump must merge like any other file change — got: ${JSON.stringify(result.entries[0])}`); + assert.equal(result.warnings.length, 0, 'declaring vendor/sub in files_modified must suppress the scope-drift advisory for this exact bump'); + + // The superproject's own tree now points at the new submodule commit — + // the concrete, diagnosable outcome of a "submodule-touch" merge. + const treeEntry = git(['ls-tree', 'HEAD', 'vendor/sub'], repoDir).trim(); + assert.match(treeEntry, /^160000 commit /, 'vendor/sub must remain a gitlink (mode 160000), never mis-parsed as a regular file'); + assert.match(treeEntry, new RegExp(sub2), 'the merged tree must point at the bumped submodule commit'); + } finally { + cleanup(tmpBase); + } + }); + + test('a submodule pointer bump NOT declared in files_modified still merges (advisory, not blocking) but surfaces a scope warning naming vendor/sub', () => { + const tmpBase = createTempDir('qb-submodule-undeclared-'); + try { + const { repoDir, sub2 } = buildRepoWithSubmodule(tmpBase); + const wtDir = path.join(tmpBase, 'wt-bump-undeclared'); + const branchName = 'worktree-agent-sub-bump-undeclared'; + addWorktree(repoDir, wtDir, branchName); + const baseCommit = git(['merge-base', 'HEAD', branchName], repoDir).trim(); + + git(['-c', 'protocol.file.allow=always', 'submodule', 'update', '--init'], wtDir); + git(['checkout', sub2], path.join(wtDir, 'vendor', 'sub')); + git(['add', 'vendor/sub'], wtDir); + git(['commit', '-m', 'bump vendor/sub to sub2, undeclared'], wtDir); + + const plan = { + ok: true, + repoRoot: repoDir, + action: 'cleanup_wave', + discovery: 'manifest', + entries: [{ + agent_id: 'sub-bump-undeclared', + worktree_path: wtDir, + branch: branchName, + expected_base: baseCommit, + files_modified: ['README.md'], + }], + }; + + const result = executeWorktreeWaveCleanupPlan(plan); + assert.equal(result.entries[0].status, 'merged_removed', 'an undeclared submodule touch is advisory-only, same as any other undeclared modification'); + assert.equal(result.warnings.length, 1); + assert.equal(result.warnings[0].code, 'scope_out_of_declared'); + assert.equal(result.warnings[0].path, 'vendor/sub'); + } finally { + cleanup(tmpBase); + } + }); +}); diff --git a/tests/gsd-quick-batch-workflow.test.cjs b/tests/gsd-quick-batch-workflow.test.cjs index 0625be3e0..5b7920abe 100644 --- a/tests/gsd-quick-batch-workflow.test.cjs +++ b/tests/gsd-quick-batch-workflow.test.cjs @@ -271,6 +271,72 @@ describe('quick-batch workflow: submodule fail-loud commit-time guard (rows 36,4 }); }); +// ─── Crash-window duplicate-dispatch guard (#3677, epic #3344 Phase 5) ────── + +describe('quick-batch workflow: worktree-dispatch.md never re-dispatches an item that already finished executing (crash-window guard)', () => { + test('Step 6 drops any quick_id whose SUMMARY.md already exists before computing spawn-plan, mirroring planner-wave.md\'s PLAN.md-existence check', () => { + const content = readStep('worktree-dispatch.md'); + assert.match(content, /Crash-window guard/i, 'worktree-dispatch.md must document the crash-window duplicate-dispatch guard'); + assert.match(content, /NEVER re-dispatch it into a second worktree/, 'the guard must explicitly forbid re-dispatch'); + assert.match(content, /\$\{item_dir\}\/\$\{quick_id\}-SUMMARY\.md/, 'the guard must check the same on-disk SUMMARY.md path merge-wave.md already uses'); + }); + + test('the guard is positioned before spawn-plan is computed, not after (a post-hoc check cannot prevent the duplicate dispatch)', () => { + const content = readStep('worktree-dispatch.md'); + const lines = splitLines(content); + const guardIdx = lines.findIndex((l) => /Crash-window guard/i.test(l)); + const spawnPlanIdx = lines.findIndex((l) => l.includes('quick-batch spawn-plan')); + assert.ok(guardIdx !== -1, 'crash-window guard section must exist'); + assert.ok(spawnPlanIdx !== -1, 'spawn-plan call must exist'); + assert.ok(guardIdx < spawnPlanIdx, 'the crash-window guard must appear BEFORE the spawn-plan call, or it cannot prevent this round\'s duplicate dispatch'); + }); + + test('the guard explains the item is not lost — merge-wave.md\'s own SUMMARY.md-on-disk criterion still picks it up', () => { + const content = readStep('worktree-dispatch.md'); + assert.match(content, /merge-wave\.md/, 'the guard must point at merge-wave.md as the item\'s recovery path'); + assert.match(content, /not remove it from the batch/i); + }); +}); + +// ─── Durable worktree-recovery persistence (#3677) ────────────────────────── +// +// The ephemeral $QUICK_BATCH_WORKTREE_MANIFEST does not survive a +// coordinator crash/restart (fresh mktemp file every process) — Step 6 must +// durably persist worktree_path/branch/expected_base onto BATCH.json so a +// RESUMED process's Step 7 can recover them for an item the crash-window +// guard correctly did not re-dispatch. + +describe('quick-batch workflow: durable worktree-recovery persistence (#3677)', () => { + test('worktree-dispatch.md persists dispatchedWorktree/dispatchedBranch/dispatchedBase via quick-batch update right after recording the ephemeral manifest entry', () => { + const content = readStep('worktree-dispatch.md'); + assert.match(content, /Durable worktree-recovery persistence/i); + assert.match(content, /dispatchedWorktree/); + assert.match(content, /dispatchedBranch/); + assert.match(content, /dispatchedBase/); + assert.match(content, /quick-batch update --batch/); + }); + + test('merge-wave.md falls back to the durable triple when the ephemeral manifest has no entry for a crash-recovered item', () => { + const content = readStep('merge-wave.md'); + assert.match(content, /Durable fallback/i); + assert.match(content, /dispatched_worktree/); + assert.match(content, /dispatched_branch/); + assert.match(content, /dispatched_base/); + }); + + test('merge-wave.md clears the durable triple back to null after a successful merge_removed (the worktree no longer exists on disk)', () => { + const content = readStep('merge-wave.md'); + assert.match(content, /Clear the\s*\n?\s*durable worktree-recovery fields now/i); + assert.match(content, /"dispatchedWorktree":null,"dispatchedBranch":null,"dispatchedBase":null/); + }); + + test('merge-wave.md fails closed (never guesses) when an item has no worktree record anywhere', () => { + const content = readStep('merge-wave.md'); + assert.match(content, /still `null`/); + assert.match(content, /missing durable worktree/); + }); +}); + // ─── planner-quick-batch mode (rows 13-15) ────────────────────────────────── describe('quick-batch planner mode: agents/gsd-planner.md extension (rows 13-15)', () => { diff --git a/tests/quick-batch-command-router.test.cjs b/tests/quick-batch-command-router.test.cjs index f6781626a..c211eee9a 100644 --- a/tests/quick-batch-command-router.test.cjs +++ b/tests/quick-batch-command-router.test.cjs @@ -227,6 +227,48 @@ describe('quick-batch-command-router: argument shaping (mocked modules)', () => assert.deepEqual(calls[0], { eligibleIds: ['a', 'b', 'c'], capacity: 2, currentInFlight: 0, refused: ['b'] }); }); + // #3677: crash-window duplicate-dispatch guard CLI verb. + test('filter-executed forwards eligible/executed to filterAlreadyExecuted', () => { + const calls = []; + routeQuickBatchCommand({ + args: ['quick-batch', 'filter-executed', '--eligible', '["a","b","c"]', '--executed', '["b"]'], + cwd: '/tmp/proj', + raw: true, + error: (msg) => { throw new Error(`unexpected error: ${msg}`); }, + _quickBatch: {}, + _quickBatchDispatch: { + filterAlreadyExecuted: (eligibleIds, executedIds) => { calls.push({ eligibleIds, executedIds }); return { spawnEligible: [], alreadyExecuted: [] }; }, + }, + }); + assert.deepEqual(calls[0], { eligibleIds: ['a', 'b', 'c'], executedIds: ['b'] }); + }); + + test('filter-executed rejects a missing --eligible', () => { + let message = null; + routeQuickBatchCommand({ + args: ['quick-batch', 'filter-executed', '--executed', '["b"]'], + cwd: '/tmp/proj', + raw: true, + error: (msg) => { message = msg; }, + _quickBatch: {}, + _quickBatchDispatch: {}, + }); + assert.match(message, /--eligible/); + }); + + test('filter-executed rejects a missing --executed', () => { + let message = null; + routeQuickBatchCommand({ + args: ['quick-batch', 'filter-executed', '--eligible', '["a"]'], + cwd: '/tmp/proj', + raw: true, + error: (msg) => { message = msg; }, + _quickBatch: {}, + _quickBatchDispatch: {}, + }); + assert.match(message, /--executed/); + }); + test('verification-routing rejects an invalid --status', () => { let message = null; routeQuickBatchCommand({ diff --git a/tests/quick-batch-dispatch.test.cjs b/tests/quick-batch-dispatch.test.cjs index 58bb714f6..232eef1ee 100644 --- a/tests/quick-batch-dispatch.test.cjs +++ b/tests/quick-batch-dispatch.test.cjs @@ -30,6 +30,7 @@ const { routeVerificationOutcome, routeMergeOutcome, buildCleanupManifestEntry, + filterAlreadyExecuted, } = require('../gsd-core/bin/lib/quick-batch-dispatch.cjs'); // ─── parseQuickBatchArgs (rows 5,7-10,13-15) ──────────────────────────────── @@ -365,3 +366,56 @@ describe('quick-batch-dispatch: buildCleanupManifestEntry — sourced FRESH from assert.equal('allowed_bases' in withoutBases, false); }); }); + +// ─── filterAlreadyExecuted — crash-window duplicate-dispatch guard (#3677) ── +// +// Pure-decision unit tests (this module performs no filesystem I/O — the +// caller determines `executedIds` via its own check). A REAL fixture +// combining this function with an actual on-disk SUMMARY.md and a real +// resumeBatch call lives in tests/quick-batch.test.cjs (crosses +// quick-batch.cjs + quick-batch-dispatch.cjs, so it belongs with the +// filesystem-backed suite, not this pure one). + +describe('quick-batch-dispatch: filterAlreadyExecuted', () => { + test('an id present in executedIds is excluded from spawnEligible and reported in alreadyExecuted', () => { + const result = filterAlreadyExecuted(['a', 'b', 'c'], ['b']); + assert.deepEqual(result.spawnEligible, ['a', 'c']); + assert.deepEqual(result.alreadyExecuted, ['b']); + }); + + test('boundary: empty executedIds — every eligible id is spawnEligible, alreadyExecuted is empty', () => { + const result = filterAlreadyExecuted(['a', 'b'], []); + assert.deepEqual(result.spawnEligible, ['a', 'b']); + assert.deepEqual(result.alreadyExecuted, []); + }); + + test('boundary: every eligible id already executed — spawnEligible is empty, nothing is silently dropped', () => { + const result = filterAlreadyExecuted(['a', 'b'], ['a', 'b']); + assert.deepEqual(result.spawnEligible, []); + assert.deepEqual(result.alreadyExecuted, ['a', 'b']); + }); + + test('boundary: empty eligibleIds — both outputs empty regardless of executedIds', () => { + const result = filterAlreadyExecuted([], ['x', 'y']); + assert.deepEqual(result.spawnEligible, []); + assert.deepEqual(result.alreadyExecuted, []); + }); + + test('order-preserving: spawnEligible/alreadyExecuted each keep eligibleIds\' original relative order', () => { + const result = filterAlreadyExecuted(['c', 'a', 'b', 'd'], ['a', 'd']); + assert.deepEqual(result.spawnEligible, ['c', 'b']); + assert.deepEqual(result.alreadyExecuted, ['a', 'd']); + }); + + test('accepts a Set for executedIds, not only an array (same convention as computeSpawnPlan\'s refused param)', () => { + const result = filterAlreadyExecuted(['a', 'b'], new Set(['a'])); + assert.deepEqual(result.spawnEligible, ['b']); + assert.deepEqual(result.alreadyExecuted, ['a']); + }); + + test('an id in executedIds that is NOT in eligibleIds is ignored — never invented into either output', () => { + const result = filterAlreadyExecuted(['a'], ['a', 'phantom-id']); + assert.deepEqual(result.spawnEligible, []); + assert.deepEqual(result.alreadyExecuted, ['a']); + }); +}); diff --git a/tests/quick-batch.test.cjs b/tests/quick-batch.test.cjs index 6e41d518e..2becf7107 100644 --- a/tests/quick-batch.test.cjs +++ b/tests/quick-batch.test.cjs @@ -43,6 +43,14 @@ const { appendQuickTaskRow } = require('../gsd-core/bin/lib/markdown-table.cjs') const { makeFakeClock } = require('./helpers/clock.cjs'); const { runGsdTools, cleanup } = require('./helpers.cjs'); +// #3677: crash-window duplicate-dispatch guard — combines resumeBatch (this +// module) with filterAlreadyExecuted (the pure decision extracted into +// quick-batch-dispatch.cts) and a REAL on-disk SUMMARY.md, the same three +// pieces worktree-dispatch.md's Step 6 wires together at runtime. +const { filterAlreadyExecuted } = require('../gsd-core/bin/lib/quick-batch-dispatch.cjs'); +const { generateSlugInternal } = require('../gsd-core/bin/lib/core-utils.cjs'); +const { planningPaths } = require('../gsd-core/bin/lib/planning-workspace.cjs'); + // ─── Shared fixtures ──────────────────────────────────────────────────────────── function mkTmpProject() { @@ -738,6 +746,118 @@ describe('quick-batch: exactly-once STATE completion', () => { }); }); +// ─── crash-window duplicate-dispatch guard (#3677, epic #3344 Phase 5) ───── +// +// A DIFFERENT crash window than row 31 above: row 31 crashes AFTER the +// STATE.md row is written (Step 9) but before BATCH.json records `complete` +// — resumeBatch's own hasQuickTaskRow detection covers that one directly. +// This one crashes EARLIER — after Step 6 (executor committed, SUMMARY.md +// written) but before Step 7 (merge), so NO STATE.md row exists yet. +// resumeBatch alone cannot detect this; worktree-dispatch.md's Step 6 must +// additionally check for an on-disk SUMMARY.md via filterAlreadyExecuted. +// Real fixture (real BATCH.json via createBatch, real SUMMARY.md file on +// disk, real resumeBatch call), not a prose/structural proxy — see +// `.gsd/phase/feat-3677-quick-batch-hardening-acceptance/40-design.md` §1. + +describe('quick-batch: crash-window duplicate-dispatch guard (#3677) — SUMMARY.md written but BATCH.json still pending', () => { + test('resumeBatch alone still reports the crashed item eligible; filterAlreadyExecuted (fed a REAL on-disk SUMMARY.md check) excludes it from spawnEligible', () => { + const dir = mkTmpProject(); + writeState(dir, stateWithQuickTasksSection()); + try { + const created = createBatch(dir, [{ description: 'crashed item' }, { description: 'clean item' }]); + assert.equal(created.ok, true); + const [crashed, clean] = created.value.manifest.items; + + // Simulate the real Step-6-to-Step-7 crash window: the executor + // finished (a real commit, in the real design; here a real SUMMARY.md + // file is what matters) but the coordinator crashed before Step 7's + // merge. BATCH.json is untouched — still `pending` — and no STATE.md + // row exists (that is written only in Step 9, well after this point). + const slug = generateSlugInternal(crashed.description); + const itemDir = path.join(planningPaths(dir).quick, `${crashed.quick_id}-${slug}`); + fs.mkdirSync(itemDir, { recursive: true }); + fs.writeFileSync( + path.join(itemDir, `${crashed.quick_id}-SUMMARY.md`), + '---\nstatus: complete\n---\n\n# Summary\n\nDid the thing.\n', + ); + + const preResume = loadBatch(dir, created.value.batchId); + assert.equal( + preResume.value.items.find((it) => it.quick_id === crashed.quick_id).status, + 'pending', + 'BATCH.json is untouched by the crash — still pending', + ); + + // --resume re-derives eligibility exactly as worktree-dispatch.md's + // Step 6 substep 1 does. BOTH items come back eligible: resumeBatch's + // own crash-window detection (hasQuickTaskRow) only fires on a + // STATE.md row, which does not exist yet for the crashed item. + const resumed = resumeBatch(dir, created.value.batchId); + assert.equal(resumed.ok, true); + assert.deepEqual( + resumed.value.eligible.slice().sort(), + [clean.quick_id, crashed.quick_id].sort(), + 'resumeBatch alone does not know the crashed item already finished executing', + ); + + // worktree-dispatch.md's own crash-window guard: a REAL filesystem + // check for each eligible id's SUMMARY.md (same item_dir derivation + // via generateSlugInternal every other step uses), fed into the pure + // filterAlreadyExecuted decision. + const executedIds = resumed.value.eligible.filter((quickId) => { + const item = resumed.value.manifest.items.find((it) => it.quick_id === quickId); + const itemSlug = generateSlugInternal(item.description); + const summaryPath = path.join(planningPaths(dir).quick, `${quickId}-${itemSlug}`, `${quickId}-SUMMARY.md`); + return fs.existsSync(summaryPath); + }); + assert.deepEqual(executedIds, [crashed.quick_id], 'only the crashed item has a real on-disk SUMMARY.md'); + + const filtered = filterAlreadyExecuted(resumed.value.eligible, executedIds); + assert.deepEqual(filtered.spawnEligible, [clean.quick_id], 'the crashed item must NOT be re-dispatched into a second worktree'); + assert.deepEqual(filtered.alreadyExecuted, [crashed.quick_id]); + + // The crashed item is not lost — it is exactly the shape + // merge-wave.md's OWN independent criterion (status=pending, + // SUMMARY.md on disk, not yet merged) already expects to find. + assert.equal(fs.existsSync(path.join(itemDir, `${crashed.quick_id}-SUMMARY.md`)), true); + assert.equal( + loadBatch(dir, created.value.batchId).value.items.find((it) => it.quick_id === crashed.quick_id).status, + 'pending', + 'filterAlreadyExecuted performs no BATCH.json write — merge-wave.md still finds it pending with an on-disk SUMMARY.md', + ); + } finally { + cleanupDir(dir); + } + }); + + test('an item with NO on-disk SUMMARY.md is unaffected — the guard only excludes genuinely already-executed items', () => { + const dir = mkTmpProject(); + writeState(dir, stateWithQuickTasksSection()); + try { + const created = createBatch(dir, [{ description: 'never started' }]); + assert.equal(created.ok, true); + const [item] = created.value.manifest.items; + + const resumed = resumeBatch(dir, created.value.batchId); + assert.deepEqual(resumed.value.eligible, [item.quick_id]); + + // No SUMMARY.md written anywhere — the filesystem check must find nothing. + const executedIds = resumed.value.eligible.filter((quickId) => { + const it = resumed.value.manifest.items.find((x) => x.quick_id === quickId); + const s = generateSlugInternal(it.description); + return fs.existsSync(path.join(planningPaths(dir).quick, `${quickId}-${s}`, `${quickId}-SUMMARY.md`)); + }); + assert.deepEqual(executedIds, []); + + const filtered = filterAlreadyExecuted(resumed.value.eligible, executedIds); + assert.deepEqual(filtered.spawnEligible, [item.quick_id], 'an item that never started must still be dispatched normally'); + assert.deepEqual(filtered.alreadyExecuted, []); + } finally { + cleanupDir(dir); + } + }); +}); + // ─── 34-35: Independence + regression ──────────────────────────────────────────── describe('quick-batch: independence from scanQuickTasks, regression on existing quick paths', () => { @@ -1071,6 +1191,114 @@ describe('quick-batch: updateBatchItems — basic mutation + persistence', () => }); }); +// #3677: durable worktree-recovery fields (dispatched_worktree/branch/base) +// — the crash-window duplicate-dispatch guard's fallback for a RESUMED +// coordinator process whose own ephemeral $QUICK_BATCH_WORKTREE_MANIFEST +// (a fresh mktemp file every process) has no entry for an item dispatched +// by a PRIOR, now-dead process. Deliberately separate fields from the +// pre-existing `worktree` (which requires the path to exist on disk via +// loadBatch's validation) — these three carry NO existence check, because +// their entire purpose is to stay readable (and clearable) after a +// legitimate post-merge worktree removal. +describe('quick-batch: updateBatchItems — durable worktree-recovery fields (#3677)', () => { + test('persists dispatchedWorktree/dispatchedBranch/dispatchedBase and a fresh loadBatch (simulating a new coordinator process) reads them back', () => { + const dir = mkTmpProject(); + try { + const created = createBatch(dir, [{ description: 'a' }, { description: 'b' }]); + assert.equal(created.ok, true); + const [itemA, itemB] = created.value.manifest.items; + assert.equal(itemA.dispatched_worktree, null, 'null until Step 6 actually dispatches'); + assert.equal(itemA.dispatched_branch, null); + assert.equal(itemA.dispatched_base, null); + + const wtDir = path.join(os.tmpdir(), `qb-dispatched-wt-${itemA.quick_id}`); + fs.mkdirSync(wtDir, { recursive: true }); + try { + const result = updateBatchItems(dir, created.value.batchId, [ + { quickId: itemA.quick_id, dispatchedWorktree: wtDir, dispatchedBranch: `agent-${itemA.quick_id}`, dispatchedBase: 'deadbeef' }, + ]); + assert.equal(result.ok, true, result.ok ? '' : result.reason); + + // A FRESH loadBatch call — standing in for a brand-new coordinator + // process (its own ephemeral worktree manifest would be empty) — + // must still recover the durable triple. + const reloaded = loadBatch(dir, created.value.batchId); + assert.equal(reloaded.ok, true); + const reloadedA = reloaded.value.items.find((it) => it.quick_id === itemA.quick_id); + assert.equal(reloadedA.dispatched_worktree, wtDir); + assert.equal(reloadedA.dispatched_branch, `agent-${itemA.quick_id}`); + assert.equal(reloadedA.dispatched_base, 'deadbeef'); + + // The untouched sibling item is unaffected. + const reloadedB = reloaded.value.items.find((it) => it.quick_id === itemB.quick_id); + assert.equal(reloadedB.dispatched_worktree, null); + } finally { + cleanup(wtDir); + } + } finally { + cleanupDir(dir); + } + }); + + test('clearing the triple (post-merge) succeeds and remains loadable even though the worktree path no longer exists on disk', () => { + const dir = mkTmpProject(); + try { + const created = createBatch(dir, [{ description: 'a' }]); + assert.equal(created.ok, true); + const [itemA] = created.value.manifest.items; + + const wtDir = path.join(os.tmpdir(), `qb-dispatched-wt-clear-${itemA.quick_id}`); + fs.mkdirSync(wtDir, { recursive: true }); + const set = updateBatchItems(dir, created.value.batchId, [ + { quickId: itemA.quick_id, dispatchedWorktree: wtDir, dispatchedBranch: `agent-${itemA.quick_id}`, dispatchedBase: 'deadbeef' }, + ]); + assert.equal(set.ok, true); + + // Merge-wave.md's own post-merge step: the real worktree is removed + // (git worktree remove, out of scope here), THEN the durable triple + // is cleared. + cleanup(wtDir); + const cleared = updateBatchItems(dir, created.value.batchId, [ + { quickId: itemA.quick_id, dispatchedWorktree: null, dispatchedBranch: null, dispatchedBase: null }, + ]); + assert.equal(cleared.ok, true, 'clearing must succeed even though the path no longer exists on disk — this is exactly why these fields are NOT the pre-existing `worktree` field'); + + const reloaded = loadBatch(dir, created.value.batchId); + assert.equal(reloaded.ok, true, 'a SUBSEQUENT loadBatch must not fail on the now-null dispatched_worktree field'); + const reloadedA = reloaded.value.items.find((it) => it.quick_id === itemA.quick_id); + assert.equal(reloadedA.dispatched_worktree, null); + assert.equal(reloadedA.dispatched_branch, null); + assert.equal(reloadedA.dispatched_base, null); + } finally { + cleanupDir(dir); + } + }); + + test('rejects a non-string, non-null dispatchedBranch without persisting anything (schema validation)', () => { + const dir = mkTmpProject(); + try { + const created = createBatch(dir, [{ description: 'a' }]); + assert.equal(created.ok, true); + + // updateBatchItems itself performs no shape check on these fields + // (same "opaque identifier" convention as `commit`) — the schema + // guard fires on the NEXT loadBatch, exactly like every other + // malformed field in this manifest. Simulate a corrupt write directly + // to prove loadBatch's validateBatchSchema catches it. + const manifestPath = path.join(dir, '.planning', 'quick-batches', created.value.batchId, 'BATCH.json'); + const manifest = JSON.parse(fs.readFileSync(manifestPath, 'utf-8')); + manifest.items[0].dispatched_branch = 12345; + fs.writeFileSync(manifestPath, JSON.stringify(manifest, null, 2)); + + const reloaded = loadBatch(dir, created.value.batchId); + assert.equal(reloaded.ok, false); + assert.match(reloaded.reason, /dispatched_branch/); + } finally { + cleanupDir(dir); + } + }); +}); + describe('quick-batch: updateBatchItems — fails closed, never persists on a bad update', () => { test('rejects an unknown quickId without persisting anything', () => { const dir = mkTmpProject();