* chore(#3677): checkpoint design artifacts (gitignored, dev-only) * test(#3677): add failing regression test for the crash-window duplicate-dispatch gap (RED) Independently re-traces resume-mode.md/planner-wave.md/worktree-dispatch.md/ merge-wave.md and src/quick-batch.cts's resumeBatch (lines 894-899) and confirms the prior research pass's Open Question 1: a coordinator crash between Step 6 (executor commits, SUMMARY.md written) and Step 7 (merge) leaves BATCH.json at "pending" with no STATE.md row yet (only written in Step 9), so --resume's eligibility re-derivation would dispatch a second executor into a new worktree for the same item, orphaning the first. This test asserts worktree-dispatch.md's Step 6 excludes an item whose SUMMARY.md already exists from the spawn set, mirroring planner-wave.md's existing PLAN.md-existence check one layer earlier. Fails against the current worktree-dispatch.md, which has no such guard. See .gsd/phase/feat-3677-quick-batch-hardening-acceptance/40-design.md §1 for the full trace and fix-location rationale. * fix(#3677): guard worktree-dispatch.md against re-dispatching an already-executed item (GREEN) worktree-dispatch.md's Step 6 re-derives eligibility every dispatch round via the same quick-batch resume call resume-mode.md uses, but had no check for "did this item already finish executing" the way planner-wave.md already checks "did this item already get planned" (PLAN.md existence) before re-planning. A coordinator crash between Step 6 (executor commits, SUMMARY.md written) and Step 7 (merge) left the item eligible for a second dispatch on --resume, orphaning the first worktree's real, already- committed work and silently losing it once the second executor's SUMMARY.md write clobbered the first at the same item_dir path. Adds a SUMMARY.md-existence exclusion before spawn-plan is computed, symmetric to planner-wave.md's PLAN.md check. The excluded item is not lost: merge-wave.md's own mergeable-wave criterion (status=pending, SUMMARY.md on disk, not yet merged) already picks it up independently of this eligible/spawn list. Workflow-prose-only fix — touches no already-merged/reviewed .cts module. See .gsd/phase/feat-3677-quick-batch-hardening-acceptance/40-design.md §1 for the fix-location rationale (why not resumeBatch itself). * test(#3677): add real-git coverage for worktree-ownership tampering, scope drift, and submodules Closes the three coverage gaps identified in 40-design.md §2/§3 (#3677, epic #3344 Phase 5's own AC bullets: "arbitrary-worktree ownership attempts", "scope drift", "submodules"): - Arbitrary-worktree ownership tampering: a manifest entry naming a non-agent branch is silently dropped at normalization before any git subprocess runs; a manifest entry naming a plausible agent-branch that was never actually created by this repo's own worktree.create (a genuinely foreign repo/branch) is blocked via base_mismatch. Both leave the foreign location and repoRoot's HEAD provably untouched. - Advisory scope drift: a committed path outside declared files_modified still merges successfully (advisory, never blocking) while surfacing a scope_out_of_declared warning naming the drifted path; an exact declared-scope match produces zero warnings (boundary case). - Real .gitmodules submodule integration: a repo containing a real local git submodule merges cleanly through executeWorktreeWaveCleanupPlan for an unrelated plan; a real gitlink pointer bump (declared) merges cleanly with the superproject tree reflecting the new pinned commit; an undeclared bump is advisory-only and surfaces a scope warning naming vendor/sub, same as any other undeclared modification. No src/*.cts changes — all three gaps were coverage-only; the underlying primitives already behaved correctly (independently verified against real git subprocess output before writing each assertion). * docs(#3677): document how to diagnose a preserved quick-batch worktree Extends the one-sentence "worktree is preserved (never deleted)" mention into a concrete diagnosis procedure: where the preserved directory is, how to read the executor's real commits/diff against the plan's declared files_modified, how to read the item's own SUMMARY.md independent of merge outcome, how to manually merge-and-clean-up or discard, and how to re-run --resume afterward. Also documents that a SUMMARY.md-written-but-still- pending item (the crash-window case fixed in this same PR) needs no manual intervention — --resume routes it straight to the merge step. * chore(#3677): checkpoint final acceptance-evidence mapping (gitignored, dev-only) * fix(#3677): make crash-window duplicate-dispatch guard behaviorally provable and durably recoverable Orthogonal review (Spec finding): the crash-window regression test added earlier this phase only asserted readStep('worktree-dispatch.md') + regex matches against the markdown prose — 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" for exactly this reason. Extracts the filtering decision into 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) — the same pure-decision- then-CLI-wired pattern computeSpawnPlan/computeMergeOrder already establish. worktree-dispatch.md now calls this verb explicitly instead of only describing the decision in prose. A genuine fixture-based test in tests/quick-batch.test.cjs constructs a REAL BATCH.json (createBatch), writes a REAL SUMMARY.md on disk at the item's real item_dir, calls the REAL resumeBatch, and proves both that resumeBatch alone still reports the item eligible AND that filterAlreadyExecuted (fed a real filesystem check) correctly excludes it. The prior prose-assertion tests are kept — they now prove the workflow markdown is correctly WIRED to the verb — but are no longer the only proof. Self-discovered defect while building that fixture (fixed inline, not deferred): tracing merge-wave.md against /gsd:quick's own prior art (QUICK_WORKTREE_MANIFEST=$(mktemp ...), quick.md:415) showed $QUICK_BATCH_WORKTREE_MANIFEST is a fresh PER-PROCESS temp file. A resumed coordinator correctly does not re-dispatch an already-executed item (this fix), but nothing durably recorded that item's worktree_path/branch/base either — Step 7 in the resumed process would have had no data to build its cleanup-wave entry from. Adds dispatched_worktree/dispatched_branch/ dispatched_base to QuickBatchItem (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). worktree-dispatch.md persists the triple once a worktree is created; merge-wave.md falls back to it when the ephemeral manifest lacks an entry, clears it after a successful merge, and fails closed rather than guessing if no record exists anywhere. See .gsd/phase/feat-3677-quick-batch-hardening-acceptance/40-design.md §9.1 and §9.3 for the full trace, empirical verification notes, and rejected alternatives (reusing `worktree` directly). * test(#3677): prove the arbitrary-worktree-ownership boundary against two real sibling worktrees Orthogonal review (Security finding): the two existing ownership-tampering tests didn't test ownership — one was trivially rejected by WORKTREE_AGENT_BRANCH_RE's shape check before any git call (proves branch- NAME filtering, not ownership), the other pointed at a wholly separate, never-linked foreign repo, so merge-base failed immediately because the branch didn't exist as a ref at all. Neither exercised the real scenario: a manifest entry whose worktree_path/branch are swapped to point at a DIFFERENT, GENUINELY-REGISTERED sibling worktree of the SAME repoRoot, with a branch name passing the shape check and a base in allowed_bases. Investigated executeWorktreeWaveCleanupPlan (src/worktree-safety.cts) directly: this is NOT a reachable gap. Git enforces branch-per-worktree uniqueness, so a swapped-in entry.branch can only match worktree_path's ACTUAL checked-out branch if it names that 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-<quick_id>[-<timestamp>]-shaped, and quick_id allocation is collision-checked GLOBALLY across every existing quick task and batch, not merely within one batch). Adds a stronger test that empirically proves this: two REAL, concurrently- alive sibling worktrees of the same repo (both via real `git worktree add`, both WORKTREE_AGENT_BRANCH_RE-passing, both sharing one merge-base), with worktree_path/branch swapped between them in both directions. Both attempts are blocked via branch_mismatch; both real worktrees, their branches, and one sibling's real uncommitted-to-main commit survive completely untouched. Supplements (does not replace) the original two tests, which still prove distinct, real boundaries. See .gsd/phase/feat-3677-quick-batch-hardening-acceptance/40-design.md §9.2 for the full trace, including the one explicitly-documented (not fixed) trust boundary this investigation surfaced: the primitive defends against fabricated data, not a caller bug that misattributes a real-but-wrong item's own triple to a different item. * chore(#3677): checkpoint design-doc addendum for review pass 2 findings (gitignored, dev-only) * docs(#3677): add changeset for PR 4240 Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> --------- Co-authored-by: sim <sim@local> Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
This commit is contained in:
Tom Boucher
@@ -60,6 +60,7 @@ interface QuickBatchDispatchModule {
|
||||
routeVerificationOutcome(status: string): unknown;
|
||||
routeMergeOutcome(outcome: Record<string, unknown>): unknown;
|
||||
buildCleanupManifestEntry(input: Record<string, unknown>): 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 <json-array> --executed <json-array>`
|
||||
// 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<string[]>(argValue(args, '--eligible'), '--eligible');
|
||||
if (!eligibleResult.ok) return makeInvalidArgs('--eligible', eligibleResult.reason, ERROR_REASON.USAGE);
|
||||
const executedResult = parseJsonArg<string[]>(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 <passed|gaps_found|human_needed>`
|
||||
'verification-routing': () => {
|
||||
const status = argValue(args, '--status');
|
||||
|
||||
@@ -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<string>): 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,
|
||||
};
|
||||
}
|
||||
|
||||
|
||||
@@ -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<QuickBatc
|
||||
if (typeof it.worktree === 'string' && it.worktree !== '' && !fs.existsSync(it.worktree)) {
|
||||
return { ok: false, reason: `item ${it.quick_id} references a worktree that does not exist on disk: ${it.worktree}` };
|
||||
}
|
||||
// #3677: dispatched_worktree/branch/base deliberately carry NO
|
||||
// existence check (unlike `worktree` above) — they must stay readable
|
||||
// after a legitimate post-merge worktree removal.
|
||||
if (it.dispatched_worktree !== null && it.dispatched_worktree !== undefined && typeof it.dispatched_worktree !== 'string') {
|
||||
return { ok: false, reason: `item ${it.quick_id} has an invalid dispatched_worktree field` };
|
||||
}
|
||||
if (it.dispatched_branch !== null && it.dispatched_branch !== undefined && typeof it.dispatched_branch !== 'string') {
|
||||
return { ok: false, reason: `item ${it.quick_id} has an invalid dispatched_branch field` };
|
||||
}
|
||||
if (it.dispatched_base !== null && it.dispatched_base !== undefined && typeof it.dispatched_base !== 'string') {
|
||||
return { ok: false, reason: `item ${it.quick_id} has an invalid dispatched_base field` };
|
||||
}
|
||||
if (it.wave !== undefined && (typeof it.wave !== 'number' || !Number.isInteger(it.wave) || it.wave < 0)) {
|
||||
return { ok: false, reason: `item ${it.quick_id} has an invalid wave field` };
|
||||
}
|
||||
@@ -605,6 +649,9 @@ function validateBatchSchema(parsed: unknown, batchId: string): Result<QuickBatc
|
||||
planned_files: it.planned_files,
|
||||
directory: typeof it.directory === 'string' ? it.directory : null,
|
||||
worktree: typeof it.worktree === 'string' ? it.worktree : null,
|
||||
dispatched_worktree: typeof it.dispatched_worktree === 'string' ? it.dispatched_worktree : null,
|
||||
dispatched_branch: typeof it.dispatched_branch === 'string' ? it.dispatched_branch : null,
|
||||
dispatched_base: typeof it.dispatched_base === 'string' ? it.dispatched_base : null,
|
||||
wave: typeof it.wave === 'number' ? it.wave : -1,
|
||||
commit: typeof it.commit === 'string' ? it.commit : null,
|
||||
failure_reason: typeof it.failure_reason === 'string' ? it.failure_reason : null,
|
||||
@@ -755,15 +802,25 @@ function completeQuickItem(
|
||||
// ─── Post-planning update (Phase 4, #3676) ──────────────────────────────────────
|
||||
|
||||
/**
|
||||
* One item's post-planning update: `dependsOn` (already-canonical `quick_id`
|
||||
* strings — planning happens after allocation, so these are never indices or
|
||||
* `clientId`s) and/or `plannedFiles` (normalized here, same as `createBatch`).
|
||||
* Either field may be omitted to leave that item's existing value untouched.
|
||||
* One item's post-planning/post-dispatch update: `dependsOn` (already-
|
||||
* canonical `quick_id` strings — planning happens after allocation, so
|
||||
* these are never indices or `clientId`s), `plannedFiles` (normalized here,
|
||||
* same as `createBatch`), and/or the #3677 durable worktree-recovery triple
|
||||
* `dispatchedWorktree`/`dispatchedBranch`/`dispatchedBase` (persisted once
|
||||
* `worktree.create` succeeds in Step 6, and explicitly cleared back to
|
||||
* `null` once Step 7 removes the worktree on a successful merge — the
|
||||
* crash-window duplicate-dispatch guard's fallback for a coordinator
|
||||
* process that resumes without the prior process's ephemeral
|
||||
* `$QUICK_BATCH_WORKTREE_MANIFEST`). Any field may be omitted to leave that
|
||||
* item's existing value untouched; pass `null` explicitly to clear one.
|
||||
*/
|
||||
interface QuickBatchItemUpdate {
|
||||
quickId: string;
|
||||
dependsOn?: string[];
|
||||
plannedFiles?: string[];
|
||||
dispatchedWorktree?: string | null;
|
||||
dispatchedBranch?: string | null;
|
||||
dispatchedBase?: string | null;
|
||||
}
|
||||
|
||||
/**
|
||||
@@ -813,6 +870,20 @@ function updateBatchItems(
|
||||
if (update.plannedFiles !== undefined) {
|
||||
item.planned_files = update.plannedFiles.map(posixNormalize);
|
||||
}
|
||||
// #3677: durable worktree-recovery fields — persisted verbatim, no
|
||||
// existence/shape validation (opaque identifiers this module never
|
||||
// interprets, same as `commit`). Explicit `null` clears a field
|
||||
// (post-merge); `undefined` leaves it untouched, same convention
|
||||
// `dependsOn`/`plannedFiles` above already use.
|
||||
if (update.dispatchedWorktree !== undefined) {
|
||||
item.dispatched_worktree = update.dispatchedWorktree;
|
||||
}
|
||||
if (update.dispatchedBranch !== undefined) {
|
||||
item.dispatched_branch = update.dispatchedBranch;
|
||||
}
|
||||
if (update.dispatchedBase !== undefined) {
|
||||
item.dispatched_base = update.dispatchedBase;
|
||||
}
|
||||
}
|
||||
|
||||
const wavesResult = computeWaves(toWaveInput(manifest.items));
|
||||
|
||||
Reference in New Issue
Block a user