Files
msd-core/src/quick-batch-dispatch.cts
Tom Boucher 515191f07d feat(#3677): quick-batch hardening and acceptance (#4240)
* 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>
2026-09-03 09:47:22 -04:00

387 lines
15 KiB
TypeScript

/**
* Quick-Batch Dispatch Core (#3676, Phase 4 of epic #3344 / ADR-1239
* "Quick-batch binding").
*
* PURE decision logic for `/gsd:quick-batch`: argument validation, effective
* concurrency, deterministic merge ordering, spawn backpressure, and
* failure/verification routing. This module answers "what should happen
* next" — it NEVER performs `Agent()` dispatch, `git worktree` I/O, or writes
* `BATCH.json`/STATE.md itself. Those live in the workflow markdown (a
* separate follow-up pass) and in `src/quick-batch.cts` (durable manifest
* mutation, reused as-is).
*
* Capacity and isolation are CALLER-SUPPLIED inputs here — the CLI layer
* resolves those via the existing `dispatch-capacity`/`dispatch-isolation`
* queries (`gsd-core/bin/gsd-tools.cjs`'s `routeDispatchCapacity`/
* `routeDispatchIsolation`); this module never re-derives that negotiation.
*
* The one exception to "never performs I/O" is `buildCleanupManifestEntry`,
* which accepts a plan document's raw text (already read by the caller) and
* parses it via the existing `parsePlanDocument` (`src/plan-document.cts`) —
* no filesystem access happens inside this module.
*
* Design lock: `.gsd/phase/feat-3676-quick-batch-command-workflow/40-design.md`.
* Test matrix: `.gsd/phase/feat-3676-quick-batch-command-workflow/50-test-matrix.md`.
*
* ADR-457 build-at-publish: compiled by tsc to gsd-core/bin/lib/quick-batch-dispatch.cjs.
*/
import type { Result } from './write-set.cjs';
// eslint-disable-next-line @typescript-eslint/no-require-imports
import planDocumentMod = require('./plan-document.cjs');
const { parsePlanDocument } = planDocumentMod;
// ─── Argument validation (design rows 5,7-10,13-15) ────────────────────────
/** Parsed, validated `/gsd:quick-batch` arguments. */
interface QuickBatchArgs {
jobs: 'auto' | number;
validate: boolean;
research: boolean;
/** `--resume <batch-id>`, or null when omitted. */
resume: string | null;
}
/**
* Validate `/gsd:quick-batch` CLI args BEFORE any dispatch. Rejects
* `--discuss`/`--full` outright (v1 exclusion, design rows 7-9,13-15 — a
* mixed valid+rejected-flag invocation is still rejected: presence alone is
* sufficient, regardless of other flags), and rejects a malformed `--jobs`
* value (non-numeric or <= 0, row 5/9) before any dispatch. `--jobs` omitted
* defaults to `'auto'` (row 10).
*/
function parseQuickBatchArgs(args: string[]): Result<QuickBatchArgs> {
if (!Array.isArray(args)) {
return { ok: false, reason: 'quick-batch args must be an array' };
}
if (args.includes('--discuss')) {
return { ok: false, reason: 'quick-batch does not support --discuss in v1 — use /gsd:quick --discuss per item instead' };
}
if (args.includes('--full')) {
return { ok: false, reason: 'quick-batch does not support --full in v1' };
}
let jobs: 'auto' | number = 'auto';
const jobsIdx = args.indexOf('--jobs');
if (jobsIdx !== -1) {
const raw = args[jobsIdx + 1];
if (raw === undefined || raw.startsWith('--')) {
return { ok: false, reason: 'quick-batch --jobs requires a value ("auto" or a positive integer)' };
}
if (raw === 'auto') {
jobs = 'auto';
} else {
const n = Number(raw);
if (!Number.isFinite(n) || !Number.isInteger(n) || n <= 0) {
return { ok: false, reason: `quick-batch --jobs must be "auto" or a positive integer, got ${JSON.stringify(raw)}` };
}
jobs = n;
}
}
const validate = args.includes('--validate');
const research = args.includes('--research');
let resume: string | null = null;
const resumeIdx = args.indexOf('--resume');
if (resumeIdx !== -1) {
const raw = args[resumeIdx + 1];
if (raw === undefined || raw.startsWith('--')) {
return { ok: false, reason: 'quick-batch --resume requires a batch id' };
}
resume = raw;
}
return { ok: true, value: { jobs, validate, research, resume } };
}
// ─── Effective concurrency (design rows 3-4,6,12) ───────────────────────────
interface EffectiveConcurrencyInput {
/** `'auto'` or the validated positive integer from `--jobs N`. */
jobs: 'auto' | number;
/** Number of tasks/items the concurrency is being computed for. */
taskCount: number;
/** Resolved capacity ceiling (from `dispatch-capacity`). */
capacity: number;
/** Resolved isolation mode (from `dispatch-isolation`). Only `'none'` is special-cased here. */
isolation: string;
/**
* Whether the wave being scheduled performs a mutation (worktree
* create/executor/merge). `isolation === 'none'` forces concurrency to 1
* ONLY for a mutating wave (row 6) — a non-mutating (research-only) stage
* is NOT subject to that cap (row 12).
*/
mutating: boolean;
}
/**
* `--jobs auto` (or omitted) → capacity alone (rows 3,10). `--jobs N` →
* `min(taskCount, N, capacity)` (row 4). When `isolation === 'none'` and the
* wave is `mutating`, concurrency is forced to 1 regardless of `--jobs`/
* capacity (row 6) — a non-mutating wave is unaffected (row 12).
*/
function computeEffectiveConcurrency(input: EffectiveConcurrencyInput): number {
let concurrency = input.jobs === 'auto'
? input.capacity
: Math.min(input.taskCount, input.jobs, input.capacity);
if (input.isolation === 'none' && input.mutating) {
concurrency = Math.min(concurrency, 1);
}
return Math.max(concurrency, 0);
}
// ─── Deterministic merge ordering (design row 24; test-matrix rows 32-33) ───
/**
* Given a wave's ORIGINAL dispatch order (`waveOrder`, as `computeWaves`
* assigned it) and the set of ids whose leaf has finished ("ready"),
* returns the prefix of `waveOrder` that is currently mergeable — merges
* happen strictly in wave order, so an item is only mergeable once every
* item before it in `waveOrder` is ALSO ready. An out-of-order completion
* (e.g. item 2 finishes before item 1) waits: `computeMergeOrder` returns
* only `[]` until item 1 is also ready.
*/
function computeMergeOrder(waveOrder: string[], readyIds: Set<string>): string[] {
const mergeable: string[] = [];
for (const id of waveOrder) {
if (!readyIds.has(id)) break;
mergeable.push(id);
}
return mergeable;
}
// ─── Spawn backpressure (design row 27; test-matrix row 39) ────────────────
interface SpawnPlanInput {
/** Eligible item ids, in dispatch-priority order. */
eligibleIds: string[];
/** Total in-flight ceiling (effective concurrency). */
capacity: number;
/** How many leaves are already in flight right now. */
currentInFlight: number;
/** Ids the host explicitly refused to spawn this round (never counted against capacity). */
refused?: Set<string> | string[];
}
interface SpawnPlanResult {
/** Ids to spawn now — never more than `capacity - currentInFlight`. */
spawn: string[];
/** Ids that stay/return to `pending` — refused OR backpressured. Never `failed`, never increases fan-out. */
pending: string[];
}
/**
* Pure backpressure model: given eligible items, a capacity ceiling, and a
* set of host-refused ids, decide which ids spawn now and which stay
* `pending`. Total in-flight (`currentInFlight + spawn.length`) never
* exceeds `capacity` at any point (row 27, property row 53). A refused id
* is never counted against capacity and never marked `failed` — it simply
* returns to `pending` for a later round.
*/
function computeSpawnPlan(input: SpawnPlanInput): SpawnPlanResult {
const refusedSet = input.refused instanceof Set ? input.refused : new Set(input.refused ?? []);
let available = Math.max(input.capacity - input.currentInFlight, 0);
const spawn: string[] = [];
const pending: string[] = [];
for (const id of input.eligibleIds) {
if (refusedSet.has(id)) {
pending.push(id);
continue;
}
if (available > 0) {
spawn.push(id);
available -= 1;
} else {
pending.push(id);
}
}
return { spawn, pending };
}
// ─── Failure/verification routing (design rows 28,30,31,34-35) ─────────────
type VerifierStatus = 'passed' | 'gaps_found' | 'human_needed';
type VerificationRouting =
| { action: 'complete' }
| { action: 'human_needed' }
| { action: 'fail'; failureReason: string };
/**
* Route a verifier's status to an item action. `human_needed` is terminal
* for the item — the caller must NOT call `completeQuickItem` (row 30, no
* STATE row appended). `gaps_found` fails the item WITHOUT rollback and
* WITHOUT an automatic gap-fix retry (row 31,34). `passed` completes it.
*/
function routeVerificationOutcome(status: VerifierStatus): VerificationRouting {
switch (status) {
case 'passed':
return { action: 'complete' };
case 'human_needed':
return { action: 'human_needed' };
case 'gaps_found':
return { action: 'fail', failureReason: 'verification reported gaps_found (no automatic gap-fix retry in v1)' };
}
}
type MergeOutcomeKind =
| { kind: 'merged' }
| { kind: 'merge_failed'; detail?: string }
| { kind: 'scope_violation'; detail?: string };
type MergeRouting =
| { action: 'complete' }
| { action: 'fail'; failureReason: string; preserveWorktree: true };
/**
* Route a merge attempt's outcome to an item action. Both `merge_failed`
* (real conflict) and `scope_violation` (undeclared deletion / advisory
* scope drift escalated to a gate — see `executeWorktreeWaveCleanupPlan`)
* mark the item `failed` with a `failure_reason` and PRESERVE the worktree
* for diagnosis — this function never signals worktree removal (row 28,
* 34-35). `merged` completes the item.
*/
function routeMergeOutcome(outcome: MergeOutcomeKind): MergeRouting {
switch (outcome.kind) {
case 'merged':
return { action: 'complete' };
case 'merge_failed':
return {
action: 'fail',
failureReason: outcome.detail ? `merge_failed: ${outcome.detail}` : 'merge_failed',
preserveWorktree: true,
};
case 'scope_violation':
return {
action: 'fail',
failureReason: outcome.detail ? `scope_violation: ${outcome.detail}` : 'scope_violation',
preserveWorktree: true,
};
}
}
// ─── 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 {
agentId: string | null;
worktreePath: string;
branch: string;
expectedBase: string;
allowedBases?: string[];
/** Raw `*-PLAN.md` text, read fresh by the caller — NEVER sourced from `BATCH.json`. */
planContent: string;
}
interface CleanupManifestEntry {
agent_id: string | null;
worktree_path: string;
branch: string;
expected_base: string;
allowed_bases?: string[];
files_modified: string[];
declared_deletions: string[];
}
/**
* Build one `worktree.cleanup-wave` manifest entry for an item, deriving
* `files_modified`/`declared_deletions` FRESH from the item's own PLAN.md via
* the existing `parsePlanDocument` (never from `BATCH.json`'s `planned_files`
* alone — Open Question 2's accepted resolution). An empty `declared_deletions`
* for a plan that genuinely deletes nothing is indistinguishable from a plan
* that forgot to declare one — this is `partitionDeclaredDeletions`'s own
* pre-existing "absent/empty = declares nothing" convention, inherited here,
* not resolved.
*/
function buildCleanupManifestEntry(input: CleanupEntryInput): CleanupManifestEntry {
const parsed = parsePlanDocument(input.planContent);
const entry: CleanupManifestEntry = {
agent_id: input.agentId,
worktree_path: input.worktreePath,
branch: input.branch,
expected_base: input.expectedBase,
files_modified: parsed.filesModified,
declared_deletions: parsed.filesDeleted,
};
if (input.allowedBases !== undefined) {
entry.allowed_bases = input.allowedBases;
}
return entry;
}
// ─── Exports ────────────────────────────────────────────────────────────────
const quickBatchDispatch = {
parseQuickBatchArgs,
computeEffectiveConcurrency,
computeMergeOrder,
computeSpawnPlan,
routeVerificationOutcome,
routeMergeOutcome,
buildCleanupManifestEntry,
filterAlreadyExecuted,
};
// eslint-disable-next-line @typescript-eslint/no-namespace
declare namespace quickBatchDispatch {
export {
QuickBatchArgs,
EffectiveConcurrencyInput,
SpawnPlanInput,
SpawnPlanResult,
VerifierStatus,
VerificationRouting,
MergeOutcomeKind,
MergeRouting,
CleanupEntryInput,
CleanupManifestEntry,
FilterAlreadyExecutedResult,
};
}
export = quickBatchDispatch;