diff --git a/.changeset/gallant-eagles-hum.md b/.changeset/gallant-eagles-hum.md new file mode 100644 index 000000000..ae889a1ab --- /dev/null +++ b/.changeset/gallant-eagles-hum.md @@ -0,0 +1,5 @@ +--- +type: Fixed +pr: 3088 +--- +Several guards that could not verify something previously reported the same result as everything is fine: a duplicate external job could dispatch past a corrupt sibling manifest, `state rebuild` could report success while phase-table reconciliation never ran, an unreadable lock body was treated as freely stealable at the same short window as a genuinely empty one, a staleness check that itself failed reported not stale, and `git base-branch` returned `main` whether it verified that or every git query timed out. These now fail closed instead of silently succeeding. (#3057) diff --git a/scripts/lint-allow-test-rule-refs.allowlist.json b/scripts/lint-allow-test-rule-refs.allowlist.json index 5cf81b351..4fff797c9 100644 --- a/scripts/lint-allow-test-rule-refs.allowlist.json +++ b/scripts/lint-allow-test-rule-refs.allowlist.json @@ -153,7 +153,6 @@ "tests/settings-jsonc.test.cjs :: structural-regression-guard", "tests/skill-frontmatter-contract.test.cjs :: source-text-is-the-product", "tests/spawn-liveness-banner.test.cjs :: source-text-is-the-product", - "tests/state-acquirestatelock-non-eexist.test.cjs :: architectural-invariant", "tests/state.test.cjs :: source-text-is-the-product", "tests/subagent-timeout.test.cjs :: source-text-is-the-product", "tests/template.test.cjs :: source-text-is-the-product", diff --git a/src/external-job.cts b/src/external-job.cts index 17fe420ea..64266d675 100644 --- a/src/external-job.cts +++ b/src/external-job.cts @@ -264,7 +264,8 @@ interface FsLike { type WriteResult = | { ok: true; path: string } - | { ok: false; kind: 'malformed_existing' | 'duplicate_plan_id' | 'io_error'; message: string }; + | { ok: false; kind: 'malformed_existing' | 'duplicate_plan_id' | 'io_error'; message: string } + | { ok: false; kind: 'scan_incomplete'; message: string; offendingPath: string }; /** * Pure path projection: `.planning/async-jobs/.json`. @@ -289,6 +290,11 @@ function _isNonTerminal(status: unknown): boolean { * - Same `job_id` for the same `plan_id` -> allowed (status progression). * - A prior job for the same `plan_id` that is already terminal -> allowed * (the duplicate guard only protects against re-dispatching live work). + * - If a SIBLING manifest (not the target) cannot be read or parsed, the + * duplicate scan cannot be completed and may be hiding a live duplicate + * -> refuse (`scan_incomplete`), naming the offending file's path so an + * operator can quarantine or repair it. Never silently skip a sibling the + * scan could not inspect. */ function writeManifest( manifest: Manifest, @@ -313,17 +319,27 @@ function writeManifest( let raw: string; try { raw = String(fs.readFileSync(p)); - } catch { - continue; + } catch (e) { + return { + ok: false, + kind: 'scan_incomplete', + message: `duplicate scan incomplete: failed to READ sibling manifest ${p} (${(e as Error).message}); quarantine or repair this file before retrying`, + offendingPath: p, + }; } let existing: Record; try { existing = JSON.parse(raw) as Record; - } catch { + } catch (e) { if (p === target) { return { ok: false, kind: 'malformed_existing', message: `target manifest ${p} is not valid JSON` }; } - continue; + return { + ok: false, + kind: 'scan_incomplete', + message: `duplicate scan incomplete: failed to PARSE sibling manifest ${p} (${(e as Error).message}); quarantine or repair this file before retrying`, + offendingPath: p, + }; } const samePlan = existing.plan_id === manifest.plan_id; const sameJob = existing.job_id === manifest.job_id; diff --git a/src/git-base-branch.cts b/src/git-base-branch.cts index 1a0c36cb8..8bcd1529f 100644 --- a/src/git-base-branch.cts +++ b/src/git-base-branch.cts @@ -17,7 +17,14 @@ * 5. "main" (last-resort default) * * Every git subprocess is bounded with a timeout (≤ 30 s); on timeout/error - * the resolver degrades gracefully to the next tier — it never throws. + * the resolver degrades gracefully to the next tier — it never throws. Tier 5 + * is reachable two ways that `resolveBaseBranch()` alone cannot tell apart: a + * repository that genuinely has no candidate branch (every git query on tiers + * 2-4 completed and cleanly answered "nothing"), or a total resolution + * failure (some query timed out / could not run). `resolveBaseBranchDiagnostics()` + * distinguishes the two via `verified`; `cmdGitBaseBranch` surfaces the + * unverified case as a stderr diagnostic without changing its stdout contract + * (#3057 B4). * * Pure/testable: all I/O is injectable via the `deps` argument so unit * tests can run without touching the real filesystem or spawning real git. @@ -38,6 +45,13 @@ export interface BaseBranchDeps { readFile?: (p: string) => string | null; /** Inject the write function used by cmdGitBaseBranch (default: process.stdout.write) */ write?: (s: string) => void; + /** + * Inject the diagnostic-write function used by cmdGitBaseBranch when the + * last-resort default is reached WITHOUT verification (default: + * process.stderr.write). Never used for the resolved branch itself — only + * for the "this is an unverified guess" warning (#3057 B4). + */ + writeDiagnostic?: (s: string) => void; } // ─── Helpers ────────────────────────────────────────────────────────────────── @@ -162,38 +176,88 @@ export function tryLocalBranch( } /** - * Resolve the default/base branch for the repository at `cwd`. + * Result of {@link resolveBaseBranchDiagnostics}: the resolved branch plus + * whether it was actually verified against the repository. + */ +export interface ResolvedBaseBranch { + branch: string; + /** + * True when `branch` came from a tier that ran to completion and produced + * a real answer: a config override, a resolved git query (tiers 2-4), or + * the tier-5 default reached because every git query on tiers 2-4 + * completed and cleanly reported no candidate. False when the tier-5 + * default was reached because at least one of those git queries timed out + * or failed to run (e.g. git missing) — collapsing that case into the same + * `'main'` as a verified "no candidate" answer is the fail-open branch + * fixed by #3057 B4: the two are no longer indistinguishable. + */ + verified: boolean; +} + +/** + * Resolve the default/base branch for the repository at `cwd`, along with + * whether the tier-5 last-resort default (if reached) was verified. * * Consults the full precedence ladder and always returns a non-empty string. * Never throws. */ -export function resolveBaseBranch( +export function resolveBaseBranchDiagnostics( cwd: string, deps?: BaseBranchDeps -): string { - const execGit: ExecGitFn = deps?.execGit ?? execGitSeam; +): ResolvedBaseBranch { + const rawExecGit: ExecGitFn = deps?.execGit ?? execGitSeam; + // A genuine execGit failure (timeout, or the call could not even spawn — + // e.g. git missing, surfaced as exitCode 127 with `error` set) is distinct + // from git completing and cleanly reporting a negative answer (non-zero + // exit with no useful output, or exit 0 with empty stdout). Only the former + // means a tier's answer was never actually obtained. Wrapping execGit here + // observes every tier's calls uniformly without changing trySymbolicRef / + // tryRemoteShow / tryLocalBranch's own return contracts. + let anyGitFailure = false; + const execGit: ExecGitFn = (args, opts) => { + const r = rawExecGit(args, opts); + if (r.timedOut || r.error) anyGitFailure = true; + return r; + }; // Derive .planning dir relative to cwd (mirrors planningDir() in planning-workspace.cjs) const planningDir = path.join(cwd, '.planning'); // 1. Config override const configured = readConfigBaseBranch(planningDir, deps); - if (configured) return configured; + if (configured) return { branch: configured, verified: true }; // 2. symbolic-ref (fast, no network) const symref = trySymbolicRef(cwd, execGit); - if (symref) return symref; + if (symref) return { branch: symref, verified: true }; // 3. git remote show origin (authoritative when origin/HEAD unset) const remoteShow = tryRemoteShow(cwd, execGit); - if (remoteShow) return remoteShow; + if (remoteShow) return { branch: remoteShow, verified: true }; // 4. Local branch existence const local = tryLocalBranch(cwd, execGit); - if (local) return local; + if (local) return { branch: local, verified: true }; - // 5. Last-resort default - return 'main'; + // 5. Last-resort default. `verified:false` when at least one tier-2/3/4 + // execGit call timed out or failed to run — the default was never actually + // checked against this repository, it is just what's left after git could + // not answer (#3057 B4). + return { branch: 'main', verified: !anyGitFailure }; +} + +/** + * Resolve the default/base branch for the repository at `cwd`. + * + * Consults the full precedence ladder and always returns a non-empty string. + * Never throws. See {@link resolveBaseBranchDiagnostics} for a caller that + * needs to distinguish a verified answer from an unverified fallback. + */ +export function resolveBaseBranch( + cwd: string, + deps?: BaseBranchDeps +): string { + return resolveBaseBranchDiagnostics(cwd, deps).branch; } // ─── gitWorktreeInfoInternal (moved from core.cjs, ADR-857 T0 #1268) ───────── @@ -240,7 +304,14 @@ export function cmdGitBaseBranch( _args: string[], deps?: BaseBranchDeps ): string { - const branch = resolveBaseBranch(cwd, deps); + const { branch, verified } = resolveBaseBranchDiagnostics(cwd, deps); + if (!verified) { + const writeDiagnostic = deps?.writeDiagnostic ?? ((s: string) => process.stderr.write(s)); + writeDiagnostic( + `⚠ git-base-branch: defaulted to 'main' WITHOUT verifying against this repository — ` + + `a git query timed out or could not run. See #3057.\n` + ); + } const write = deps?.write ?? ((s: string) => process.stdout.write(s)); write(branch + '\n'); return branch; diff --git a/src/init.cts b/src/init.cts index 8eb16387f..0daaeb112 100644 --- a/src/init.cts +++ b/src/init.cts @@ -227,6 +227,18 @@ interface PhaseCompletionProjection { completion_status: string; verification_next_action: string; verification_next_command: string; + /** + * #3057 B3: true when readVerificationStatus's internal staleness check could + * NOT run to completion (an fs / scanPhasePlans / clock failure) — routing + * above is unaffected (the pre-existing fail-open contract), but this lets a + * workflow step distinguish "checked; nothing is stale" from "could not + * check" instead of silently treating both as the same "not stale" answer. + * Always present (unlike verification.cts's own optional field) so this + * projection's shape stays uniform with its sibling boolean fields; false + * when the staleness check was never reached (e.g. implementation not yet + * complete) or ran to completion. + */ + verification_stale_check_indeterminate: boolean; } @@ -271,6 +283,10 @@ function buildPhaseCompletionProjection( completion_status: projectCompletionStatus(implementationComplete, verificationPassed), verification_next_action: projectedVerificationAction, verification_next_command: verificationStatus.next_command, + // #3057 B3: only readVerificationStatus's result ever carries this flag — + // the `not_required` synthetic object above never does. + verification_stale_check_indeterminate: 'staleCheckIndeterminate' in verificationStatus + && verificationStatus.staleCheckIndeterminate === true, }; } diff --git a/src/io.cts b/src/io.cts index fa58f2fcf..f52aaed7c 100644 --- a/src/io.cts +++ b/src/io.cts @@ -225,10 +225,17 @@ function getJsonErrorMode(): boolean { return _jsonErrorMode; } * process is in JSON-error mode, stderr receives `{ ok: false, reason, * message }` so callers can parse it; otherwise stderr keeps the plain * text form for human operators. + * + * `extra` (optional) lets a caller attach additional structured fields + * (e.g. `{ verification_stale_check_indeterminate: true }`) onto the + * JSON-error-mode payload, spread alongside `ok`/`reason`/`message`, so a + * test can assert on the value directly instead of regexing `message`'s + * human-readable text. Ignored entirely in plain-text mode — the human + * message is the only thing an operator sees there. */ -function error(message: string, reason: ErrorReasonValue = ERROR_REASON.UNKNOWN): never { +function error(message: string, reason: ErrorReasonValue = ERROR_REASON.UNKNOWN, extra?: Record): never { if (_jsonErrorMode) { - const payload = JSON.stringify({ ok: false, reason, message }) + '\n'; + const payload = JSON.stringify({ ok: false, reason, message, ...(extra || {}) }) + '\n'; writeAllSync(2, payload); } else { writeAllSync(2, 'Error: ' + message + '\n'); diff --git a/src/phase.cts b/src/phase.cts index fc45d3ce7..e49d11bc5 100644 --- a/src/phase.cts +++ b/src/phase.cts @@ -1834,6 +1834,11 @@ function cmdPhaseComplete(cwd: string, phaseNum: string, raw: boolean): void { let requirementsUpdated = false; const warnings: string[] = []; + // #3057 B3: mirrors `verification_stale_check_indeterminate` on init.cts / + // roadmap.cts / uat-predicate.cts's outputs — set on the non-blocking path + // below (inside withPlanningLock) alongside the warnings[] entry, so a + // caller can assert on the typed field instead of the warning's prose. + let staleCheckIndeterminate = false; const phaseFullDir = path.join(cwd, phaseInfo['directory'] as string); // #2648: fail-closed plan-coverage gate. phase.complete used to gate ONLY on a @@ -2000,6 +2005,21 @@ function cmdPhaseComplete(cwd: string, phaseNum: string, raw: boolean): void { // suggests the command surface this runtime actually installs // ($gsd-… on Codex) rather than a hard-coded Claude-style string. const verificationStatus = readVerificationStatus(phaseFullDir, { runtime: resolveRuntime(cwd) }); + // #3057 B3: the staleness check inside readVerificationStatus can itself + // fail (fs / scanPhasePlans / clock error), in which case `status` above + // was routed as if nothing were stale (unchanged fail-open routing) — but + // that must not be silently identical to a check that actually ran and + // found nothing stale. Join the SAME advisory channel the UAT/VERIFICATION + // pre-scan above already uses (`warnings[]`, rendered by execute-phase.md's + // "If has_warnings is true" step) rather than inventing a new one. This + // only fires on the non-blocking path (status resolves to 'passed' despite + // the indeterminate check) — the blocked path below carries its own note. + if (verificationStatus.staleCheckIndeterminate) { + staleCheckIndeterminate = true; + warnings.push( + `verification staleness check could not complete for phase ${phaseNum} — routed as not-stale, but this was not actually verified (#3057)`, + ); + } if (verificationStatus.status !== 'passed') { return verificationStatus; } @@ -2703,9 +2723,21 @@ function cmdPhaseComplete(cwd: string, phaseNum: string, raw: boolean): void { const nextStep = verificationBlocked.next_command ? ` Next: ${verificationBlocked.next_command}` : ''; + // #3057 B3: purely additive to the message text — does not change WHETHER + // this blocks (verificationBlocked was already truthy) or the + // ERROR_REASON, only whether the operator can see the staleness check + // itself did not complete. The same fact is also attached as a typed + // field (`verification_stale_check_indeterminate`) on the JSON-error-mode + // payload so a test can assert on it by value instead of regexing this + // human-readable note. + const staleCheckIndeterminate = verificationBlocked.staleCheckIndeterminate === true; + const indeterminateNote = staleCheckIndeterminate + ? ' (staleness check could not complete — see #3057)' + : ''; error( - `Phase ${phaseNum} verification is incomplete: ${verificationBlocked.next_action}${nextStep}`, + `Phase ${phaseNum} verification is incomplete: ${verificationBlocked.next_action}${nextStep}${indeterminateNote}`, ERROR_REASON.PHASE_VERIFICATION_INCOMPLETE, + { verification_stale_check_indeterminate: staleCheckIndeterminate }, ); } @@ -2741,6 +2773,7 @@ function cmdPhaseComplete(cwd: string, phaseNum: string, raw: boolean): void { auto_pruned: autoPruned, warnings, has_warnings: warnings.length > 0, + verification_stale_check_indeterminate: staleCheckIndeterminate, }; output(result, raw); diff --git a/src/roadmap.cts b/src/roadmap.cts index 1c8d4f5f8..60f0b0ccf 100644 --- a/src/roadmap.cts +++ b/src/roadmap.cts @@ -551,8 +551,13 @@ function cmdRoadmapUpdatePlanProgress(cwd: string, phaseNum: string | null | und // cmdPhaseComplete's gate (phase.cts:1436). Previously the checkbox fired the // moment the last plan summary landed — before gsd-verifier had verified. const phaseDir = path.join(cwd, phaseInfo!.directory); - const verificationPassed = readVerificationStatus(phaseDir).status === 'passed'; + const verificationResult = readVerificationStatus(phaseDir); + const verificationPassed = verificationResult.status === 'passed'; const isComplete = summaryCount >= planCount && verificationPassed; + // #3057 B3: routing above is unchanged (an indeterminate staleness check + // still routes as if nothing were stale) — this only makes the fact visible + // to whatever reads this command's JSON output. + const verificationStaleCheckIndeterminate = verificationResult.staleCheckIndeterminate === true; const status = isComplete ? 'Complete' : summaryCount > 0 ? 'In Progress' : 'Planned'; const today = realClock.localToday(); @@ -757,6 +762,7 @@ function cmdRoadmapUpdatePlanProgress(cwd: string, phaseNum: string | null | und summary_count: summaryCount, status, complete: isComplete, + verification_stale_check_indeterminate: verificationStaleCheckIndeterminate, }, raw, `${summaryCount}/${planCount} ${status}`); } diff --git a/src/state-transition.cts b/src/state-transition.cts index 9312f4909..9db1f96f3 100644 --- a/src/state-transition.cts +++ b/src/state-transition.cts @@ -294,12 +294,20 @@ export type StateTransitionDeps = { roadmapProvider?: () => string | null; /** * Phase-inventory provider for `rebuild` (ADR-1817 §2): re-derives the - * `## By-Phase Progress` table from canonical disk sources. Returns one - * record per on-disk phase directory under `.planning/phases/`. Optional: - * when absent, `rebuild` skips table reconciliation (Leaky-Abstractions + * `## By-Phase Progress` table from canonical disk sources. Optional: when + * absent, `rebuild` skips table reconciliation entirely (Leaky-Abstractions * guard — the core stays pure and testable without disk I/O). + * + * When present, MUST return a discriminated result, never a bare + * array-or-null: `{ ok: true, phases }` for a (possibly empty) successful + * scan vs `{ ok: false, reason }` for a scan that could not complete. A + * disk-scan failure is NOT the same as "no phases on disk" — collapsing + * both to the same value made a real failure indistinguishable from + * "nothing to reconcile" (issue #3057 B1); `reconcileByPhaseTable` treats + * the two differently and surfaces `ok:false` to the caller instead of + * silently no-op'ing. */ - phaseInventoryProvider?: () => PhaseInventoryRecord[] | null; + phaseInventoryProvider?: () => PhaseInventoryResult; /** * Resolved path of the STATE.md the content was read from. Injected (not * derived) so the core stays pure; used only to name the file in an @@ -321,6 +329,19 @@ export type PhaseInventoryRecord = { summaryCount: number; }; +/** + * Discriminated result of a `phaseInventoryProvider` disk scan (issue #3057 + * B1). `ok:true` covers a completed scan, including the genuinely-empty case + * (`phases: []` — no `.planning/phases/` directory exists yet). `ok:false` + * covers a scan that could not complete (e.g. the phases directory could not + * be read). The two are NOT interchangeable: only `ok:true` with an empty + * array means "nothing to reconcile"; `ok:false` means "unknown — do not + * treat as reconciled". + */ +export type PhaseInventoryResult = + | { ok: true; phases: PhaseInventoryRecord[] } + | { ok: false; reason: string }; + export type StateTransitionIntent = | { kind: 'beginPhase'; phaseNumber: string | number; phaseName: string | null; planCount: number | null } | { kind: 'advancePlan' } @@ -1009,8 +1030,17 @@ function completePhaseCore( if (derived.completedPhases !== null) newCompleted = derived.completedPhases; if (derived.totalPhases !== null) derivedTotalPhases = derived.totalPhases; } + // #3057 B9: only mark 'Completed Phases' updated when the text actually + // changed. `stateReplaceField` returns the full (re-)substituted content + // whenever the field pattern matches, REGARDLESS of whether newCompleted + // differs from the value already in `body` — so a truthy-only check here + // marked the field 'updated' even when the roadmap was unavailable and + // newCompleted is just completedRaw parsed back to itself. That collapsed + // "recomputed from roadmap" and "left as-is" into the same `updated` + // signal. Comparing to `body` (the idiom every other field in this + // function already uses) restores the distinction. const completedAfter = stateReplaceField(body, 'Completed Phases', String(newCompleted)); - if (completedAfter) { + if (completedAfter !== null && completedAfter !== body) { body = completedAfter; updated.push('Completed Phases'); } @@ -1019,8 +1049,9 @@ function completePhaseCore( const totalPhases = derivedTotalPhases || (totalRaw ? parseInt(totalRaw, 10) : null); if (totalPhases && totalPhases > 0) { const newPercent = clampPercent(newCompleted, totalPhases); + // Same guard as 'Completed Phases' above, and for the same reason. const progAfter = stateReplaceField(body, 'Progress', `${newPercent}%`); - if (progAfter) { + if (progAfter !== null && progAfter !== body) { body = progAfter; updated.push('Progress'); } @@ -1715,6 +1746,20 @@ interface RebuildLogEntry { reason: string; } +/** + * Out-of-band signal from `reconcileByPhaseTable` back to `rebuildCore` + * (issue #3057 B1): a mutable sibling to `log`, carrying whether the + * phase-inventory disk scan failed. Kept separate from `log`/`RebuildLogEntry` + * because a scan failure does not mutate content (there is nothing to diff), + * so it does not fit the before/after log-entry shape — but it still must + * reach the caller so "scan failed" and "nothing to reconcile" stay + * distinguishable in `rebuildCore`'s returned `data`. + */ +interface PhaseInventoryScanMeta { + failed: boolean; + reason: string | null; +} + /** * Truncate a string for inclusion in a rebuild log entry. Per ADR-1817 §3 the * `before` / `after` fields are bounded to REBUILD_LOG_TRUNCATION_LIMIT chars @@ -1737,6 +1782,7 @@ function rebuildCore( ): StateTransitionResult { const timestamp = deps.clock.nowIso(); const log: RebuildLogEntry[] = []; + const phaseInventoryScan: PhaseInventoryScanMeta = { failed: false, reason: null }; let modified = content; // §2 Decision: re-derive derived sections, preserve others. Order is @@ -1744,7 +1790,7 @@ function rebuildCore( // sourcePath threaded so `state rebuild --dry-run` names the file: that branch reads STATE.md // directly rather than through readModifyWriteStateMd, so nothing upstream has named it yet. modified = reconcileCurrentPosition(modified, timestamp, log, deps.sourcePath); - modified = reconcileByPhaseTable(modified, deps, timestamp, log); + modified = reconcileByPhaseTable(modified, deps, timestamp, log, phaseInventoryScan); modified = stripTemplatePlaceholders(modified, timestamp, log); modified = deduplicateSessionArchive(modified, timestamp, log); @@ -1763,6 +1809,11 @@ function rebuildCore( mutated: log.length > 0, mutations: log.length, log, + // #3057 B1: distinguishable from a clean "nothing to reconcile" — a + // failed phase-inventory scan means the by-phase table was NOT + // verified against disk, even though `mutated` may still be false. + phase_inventory_scan_failed: phaseInventoryScan.failed, + ...(phaseInventoryScan.reason !== null ? { phase_inventory_scan_reason: phaseInventoryScan.reason } : {}), }, }; } @@ -1849,16 +1900,29 @@ function reconcileCurrentPosition( * Leaky-Abstractions guard (ADR-1817 §1): when `phaseInventoryProvider` is * absent (no disk scan wired), this step is a no-op. The core stays pure and * testable without disk I/O. + * + * A DIFFERENT case is a scan that ran but failed (`ok:false`): that is NOT a + * no-op-equivalent "nothing to reconcile" — the table is left untouched (we + * have no trustworthy inventory to reconcile against) but the failure is + * recorded into `meta` so the caller (`rebuildCore`) can surface it instead + * of reporting a clean, fully-reconciled rebuild (#3057 B1). */ function reconcileByPhaseTable( content: string, deps: StateTransitionDeps, timestamp: string, log: RebuildLogEntry[], + meta: PhaseInventoryScanMeta, ): string { if (!deps.phaseInventoryProvider) return content; - const inventory = deps.phaseInventoryProvider(); - if (!inventory || inventory.length === 0) return content; + const result = deps.phaseInventoryProvider(); + if (!result.ok) { + meta.failed = true; + meta.reason = result.reason; + return content; // cannot reconcile without a trustworthy inventory — leave the table as-is + } + const inventory = result.phases; + if (inventory.length === 0) return content; // The canonical table shape (from gsd-core/templates/state.md): // | Phase | Plans | Total | Avg/Plan | diff --git a/src/state.cts b/src/state.cts index 67fa6769d..867b03f58 100644 --- a/src/state.cts +++ b/src/state.cts @@ -36,6 +36,7 @@ const { transitionCore, applyStatePreservation, sliceCurrentPositionSection } = type StateTransitionIntent = stateTransitionMod.StateTransitionIntent; type StateTransitionDeps = stateTransitionMod.StateTransitionDeps; type PhaseInventoryRecord = stateTransitionMod.PhaseInventoryRecord; +type PhaseInventoryResult = stateTransitionMod.PhaseInventoryResult; import { computeProgressPercent, normalizeProgressNumbers, @@ -263,24 +264,54 @@ function _stateHolderVerifiedLive(lockPath: string): boolean { return pid !== null && _stateLockIsPidAlive(pid); } +/** + * Three-way classification of a lock body read (issue #3057 B2): a pid that + * parses cleanly, a body that reads but is empty/garbage/non-numeric, or a + * body that could not be READ at all (I/O fault — permission error, transient + * NFS/overlay-fs hiccup, mid-rename, etc.). The third case is NOT the same as + * the second: an unreadable body tells us nothing about whether the lock is + * fresh, stale, or actively held mid-write by a live process whose file the + * fault merely prevented us from reading. Collapsing it into "empty" would + * make it eligible for the short fresh-create-floor steal window, which can + * rob an active holder purely because of a transient read fault. + */ +type LockBodyStatus = + | { kind: 'pid'; pid: number } + | { kind: 'empty' } + | { kind: 'unreadable' }; + +/** + * Read + classify the lock body at `lockPath`. See `LockBodyStatus` for the + * three-way distinction the steal decision in `acquireStateLock` relies on. + */ +function _stateLockBodyStatus(lockPath: string): LockBodyStatus { + let body: string; + try { + body = fs.readFileSync(lockPath, 'utf-8'); + } catch { + return { kind: 'unreadable' }; + } + const trimmed = body.trim(); + const pid = parseInt(trimmed, 10); + if (!Number.isInteger(pid) || pid <= 0 || String(pid) !== trimmed) return { kind: 'empty' }; + return { kind: 'pid', pid }; +} + /** * Parse the lock body to its recorded pid, or null when the body is empty / non-numeric * / unreadable (legacy or mid-creation). Distinguishing a COMPLETE dead-pid body (steal * promptly) from an EMPTY/unparseable one (the create→write window — do not steal while * fresh) is what `_stateHolderVerifiedLive` alone cannot express, so the steal decision * in acquireStateLock reads the pid directly (PR #1532 review, window a). + * + * NOTE: this collapses "genuinely empty" and "unreadable" to the same `null` — + * that is fine for `_stateHolderVerifiedLive` (both mean "not verified-live" + * either way), but the STEAL-TIMING decision must not make that same + * collapse (#3057 B2) and reads `_stateLockBodyStatus` directly instead. */ function _stateLockBodyPid(lockPath: string): number | null { - let body: string; - try { - body = fs.readFileSync(lockPath, 'utf-8'); - } catch { - return null; // unreadable body → cannot verify - } - const trimmed = body.trim(); - const pid = parseInt(trimmed, 10); - if (!Number.isInteger(pid) || pid <= 0 || String(pid) !== trimmed) return null; - return pid; + const status = _stateLockBodyStatus(lockPath); + return status.kind === 'pid' ? status.pid : null; } // Monotonic sequence for unique stale-steal rename targets (no crypto dependency). @@ -2043,17 +2074,22 @@ function acquireStateLock(statePath: string, clock?: StateLockClock): string { } if ((err as NodeJS.ErrnoException).code !== 'EEXIST') throw err; // propagate — silent bypass causes lost updates // Liveness-gated steal (audit M1) + steal-safety (PR #1532 review). The steal - // decision is three-way on the lock body: + // decision is four-way on the lock body (#3057 B2 added the fourth): // - VERIFIED-LIVE holder (parseable pid that signals alive): NEVER stolen until // its age crosses the absolute deadman ceiling (the pid-reuse backstop) — // nuking a slow-but-live writer's lock causes lost updates (#3711 / #500/#905/ // #1230 family). // - COMPLETE DEAD pid (parseable pid, not alive): stolen PROMPTLY regardless of // age — a crashed holder left a full body. - // - EMPTY / unparseable body: liveness is unknowable. While FRESH (age <= - // freshCreateFloorMs) it is a lock still mid-creation (O_EXCL done, pid not yet - // written) and is NOT stolen (window a); only once aged past the floor is it a - // genuine orphan and stealable. + // - UNREADABLE body (I/O fault reading the file): NOT the same as empty — we + // have no evidence this is a fresh create window, only that we could not read + // it. Held to the SAME conservative ceiling as a verified-live holder rather + // than the short fresh-create floor, so a transient read fault can never rob + // an active holder the way stealing at 1s would. + // - EMPTY / unparseable body (body WAS read, and holds no valid pid): liveness is + // unknowable. While FRESH (age <= freshCreateFloorMs) it is a lock still + // mid-creation (O_EXCL done, pid not yet written) and is NOT stolen (window a); + // only once aged past the floor is it a genuine orphan and stealable. // The steal itself is an ATOMIC rename-then-recreate (only one racer can rename the // inode) guarded by an identity re-confirm, so a racer that recreates a fresh lock // in the decision→steal gap never has its replacement deleted (window b). Mirrors @@ -2061,13 +2097,16 @@ function acquireStateLock(statePath: string, clock?: StateLockClock): string { try { const stat = fs.statSync(lockPath); const ageMs = clock.now() - stat.mtimeMs; - const bodyPid = _stateLockBodyPid(lockPath); + const bodyStatus = _stateLockBodyStatus(lockPath); + const bodyPid = bodyStatus.kind === 'pid' ? bodyStatus.pid : null; const holderLive = bodyPid !== null && _stateLockIsPidAlive(bodyPid); let steal: boolean; if (holderLive) { steal = ageMs > deadmanCeilingMs; // pid-reuse backstop only } else if (bodyPid !== null) { steal = true; // complete dead pid → prompt steal + } else if (bodyStatus.kind === 'unreadable') { + steal = ageMs > deadmanCeilingMs; // I/O fault ≠ known-fresh — do not grant the short floor } else { steal = ageMs > freshCreateFloorMs; // empty/garbage → protect the create window } @@ -3103,10 +3142,19 @@ function cmdStateRebuild(cwd: string, options: StateRebuildOptions, raw: boolean // is the same canonical source `buildStateFrontmatter` consults; the Leaky- // Abstractions guard in `rebuildCore` (ADR-1817 §1) keeps the pure core // testable without this dep — here we provide it. - const phaseInventoryProvider = (): PhaseInventoryRecord[] | null => { + // + // #3057 B1: a missing `.planning/phases/` directory is genuinely "nothing + // to reconcile" (`ok:true, phases: []`) — but a `readdirSync`/`statSync` + // THROW on a directory that DOES exist (permission fault, corrupted + // mount, etc.) is a real scan failure (`ok:false`). The old implementation + // returned `null` for both, so `state rebuild` could report success while + // by-phase-table reconciliation silently never ran. Per-entry stat + // failures (an individual phase dir vanishing mid-scan) still `continue` + // past that one entry — that is not a whole-scan failure. + const phaseInventoryProvider = (): PhaseInventoryResult => { try { const phasesDir = path.join(planningPaths(cwd).planning, 'phases'); - if (!fs.existsSync(phasesDir) || !fs.statSync(phasesDir).isDirectory()) return null; + if (!fs.existsSync(phasesDir) || !fs.statSync(phasesDir).isDirectory()) return { ok: true, phases: [] }; const entries = fs.readdirSync(phasesDir); const records: PhaseInventoryRecord[] = []; for (const entry of entries) { @@ -3122,9 +3170,9 @@ function cmdStateRebuild(cwd: string, options: StateRebuildOptions, raw: boolean const summaryCount = files.filter(f => /-SUMMARY\.md$/i.test(f)).length; records.push({ number: m[1], name: m[2], planCount, summaryCount }); } - return records; - } catch { - return null; + return { ok: true, phases: records }; + } catch (err) { + return { ok: false, reason: err instanceof Error ? err.message : String(err) }; } }; @@ -3150,18 +3198,36 @@ function cmdStateRebuild(cwd: string, options: StateRebuildOptions, raw: boolean } }; + // #3057 B1: distinguish "nothing to rebuild" from "the phase-inventory + // disk scan failed, so by-phase-table reconciliation could not run" — both + // used to collapse to the same `mutated:false` / "Nothing to rebuild" note. + type RebuildData = { + log?: unknown[]; + mutated?: boolean; + phase_inventory_scan_failed?: boolean; + phase_inventory_scan_reason?: string; + }; + const scanFailureNote = (reason: string | undefined): string => + 'Nothing rebuilt: the phase-inventory disk scan failed, so by-phase-table reconciliation did not run' + + (reason ? ` (${reason})` : ''); + if (dryRun) { const content = fs.readFileSync(statePath, 'utf-8'); const result = runRebuild(content); - const data = (result.data ?? {}) as { log?: unknown[]; mutated?: boolean }; + const data = (result.data ?? {}) as RebuildData; emitVerboseLog(data.log); const mutated = data.mutated === true; + const scanFailed = data.phase_inventory_scan_failed === true; emit({ rebuilt: false, dry_run: true, mutations: Array.isArray(data.log) ? data.log.length : 0, mutated, - note: mutated ? 'Run without --dry-run to apply changes' : 'Nothing to rebuild', + phase_inventory_scan_failed: scanFailed, + phase_inventory_scan_reason: scanFailed ? data.phase_inventory_scan_reason : undefined, + note: mutated + ? 'Run without --dry-run to apply changes' + : scanFailed ? scanFailureNote(data.phase_inventory_scan_reason) : 'Nothing to rebuild', }, raw, mutated ? 'true' : 'false'); return; } @@ -3171,11 +3237,15 @@ function cmdStateRebuild(cwd: string, options: StateRebuildOptions, raw: boolean // to STATE.md by rebuildCore itself, per ADR-1817 §3). let capturedLog: unknown[] = []; let capturedMutated = false; + let capturedScanFailed = false; + let capturedScanReason: string | undefined; readModifyWriteStateMd(statePath, (content: string) => { const result = runRebuild(content); - const data = (result.data ?? {}) as { log?: unknown[]; mutated?: boolean }; + const data = (result.data ?? {}) as RebuildData; capturedLog = Array.isArray(data.log) ? data.log : []; capturedMutated = data.mutated === true; + capturedScanFailed = data.phase_inventory_scan_failed === true; + capturedScanReason = data.phase_inventory_scan_reason; return result.content; }, cwd); @@ -3184,7 +3254,11 @@ function cmdStateRebuild(cwd: string, options: StateRebuildOptions, raw: boolean emit({ rebuilt: capturedMutated, mutations: capturedLog.length, - note: capturedMutated ? 'STATE.md rebuilt; see ## Rebuild Log section for the audit trail' : 'Nothing to rebuild', + phase_inventory_scan_failed: capturedScanFailed, + phase_inventory_scan_reason: capturedScanFailed ? capturedScanReason : undefined, + note: capturedMutated + ? 'STATE.md rebuilt; see ## Rebuild Log section for the audit trail' + : capturedScanFailed ? scanFailureNote(capturedScanReason) : 'Nothing to rebuild', }, raw, capturedMutated ? 'true' : 'false'); } diff --git a/src/uat-predicate.cts b/src/uat-predicate.cts index 780fe8bf1..186874c3b 100644 --- a/src/uat-predicate.cts +++ b/src/uat-predicate.cts @@ -43,6 +43,17 @@ interface UatPassedReport { policy: { require_verification: boolean; }; + /** + * #3057 B3: true when the `requireVerification` policy check's own + * readVerificationStatus call could not complete its internal staleness + * check (fs / scanPhasePlans / clock failure). `passed`/`blockers` above are + * UNCHANGED by this — the pre-existing fail-open routing (indeterminate + * treated as not-stale) is preserved — this only makes the fact visible to + * cmdPhaseUatPassed's JSON output (which spreads this whole report), rather + * than silently indistinguishable from "checked; nothing is stale". Always + * false when `requireVerification` is not set (the check is never reached). + */ + verification_stale_check_indeterminate: boolean; } // ─── Blocking state sets (documented for maintainability) ───────────────────── @@ -227,6 +238,8 @@ function evaluateUatPassed( blockers, no_uat_artifacts, policy: { require_verification: requireVerification }, + // readVerificationStatus was never reached on this early-return path. + verification_stale_check_indeterminate: false, }; } @@ -312,8 +325,15 @@ function evaluateUatPassed( } // ── Policy: requireVerification ─────────────────────────────────────────── + // #3057 B3: routing here is UNCHANGED — an indeterminate staleness check + // still falls through to the same `verificationStatus !== 'passed'` branch + // it always did (the pre-existing fail-open contract). `verificationStaleCheckIndeterminate` + // only records the fact for the report below; it never itself gates `blockers`. + let verificationStaleCheckIndeterminate = false; if (requireVerification) { - const verificationStatus = readVerificationStatus(phaseFullDir).status; + const verificationResult = readVerificationStatus(phaseFullDir); + const verificationStatus = verificationResult.status; + verificationStaleCheckIndeterminate = verificationResult.staleCheckIndeterminate === true; if (verificationStatus === 'stale') { blockers.push('policy: verification status=stale'); } else if (verificationStatus !== 'passed' || !hasPassingVerification) { @@ -339,6 +359,7 @@ function evaluateUatPassed( policy: { require_verification: requireVerification, }, + verification_stale_check_indeterminate: verificationStaleCheckIndeterminate, }; } diff --git a/src/verification.cts b/src/verification.cts index a8699a27a..80db8a39a 100644 --- a/src/verification.cts +++ b/src/verification.cts @@ -143,10 +143,18 @@ interface FsLike { statSync(filePath: string): { mtimeMs: number }; } -interface StaleVerificationInfo { - verificationFile: string; - summaryFile: string; -} +/** + * Outcome of a staleness check. `determined:false` means the check could NOT + * run to completion (an fs / scanPhasePlans / injected-clock failure) — this + * is distinct from `determined:true, stale:false`, which means the check ran + * to completion and genuinely found nothing stale. Collapsing the two (the + * pre-#3057 behavior: both returned `null`) let a disk-scan failure silently + * report "not stale" — the same fail-open shape as #3050. (#3057 B3) + */ +type StaleCheckResult = + | { determined: true; stale: true; verificationFile: string; summaryFile: string } + | { determined: true; stale: false } + | { determined: false }; /** * Resolve the git commit time (epoch-ms) for each of `files` (paths relative to @@ -296,30 +304,44 @@ interface VerificationStatusResult { status: string; next_action: string; next_command: string; + /** + * True when the internal staleness check (findStaleVerificationSummary) + * could not run to completion (an fs / scanPhasePlans / clock failure) — + * `status` above was routed as if the phase were not stale (the pre-existing + * no-throw fail-open contract, preserved unchanged), but this flag lets a + * caller distinguish "checked; nothing is stale" from "could not check" so + * the two are no longer silently identical (#3057 B3). Omitted (not present) + * when the staleness check ran to completion, or was never reached (e.g. the + * `gaps_found` short-circuit above it, or no verification file at all). + */ + staleCheckIndeterminate?: boolean; } function findStaleVerificationSummary( phaseDir: string, fsImpl: FsLike = fs, phaseCleanCommitTimesMs: PhaseCleanCommitTimesFn = defaultPhaseCleanCommitTimesMs, -): StaleVerificationInfo | null { +): StaleCheckResult { // FS errors (TOCTOU: a SUMMARY listed by scanPhasePlans then removed before statSync; - // unreadable dir; broken symlink; file->dir swap) must degrade to "not stale" rather - // than throw uncaught into callers that are NOT under the planning lock - // (init.manager / init.progress / uat-predicate). Mirrors readVerificationStatus's - // no-throw contract; `fsImpl` threads the same injectable-fs seam for parity/testing. - // (Review B1 on #1548.) + // unreadable dir; broken symlink; file->dir swap) must degrade rather than throw + // uncaught into callers that are NOT under the planning lock (init.manager / + // init.progress / uat-predicate). Mirrors readVerificationStatus's no-throw + // contract; `fsImpl` threads the same injectable-fs seam for parity/testing. + // (Review B1 on #1548.) The degraded result is `{determined:false}`, NOT the + // same value as a completed "nothing is stale" check — see StaleCheckResult + // doc and #3057 B3. The caller decides how to route an indeterminate result; + // this function only reports what it actually knows. try { const phaseFiles = fsImpl.readdirSync(phaseDir); const verificationFile = phaseFiles.filter((f) => f.endsWith('-VERIFICATION.md')).sort()[0]; - if (!verificationFile) return null; + if (!verificationFile) return { determined: true, stale: false }; const summaryFiles = (scanPhasePlans(phaseDir) as { summaryFiles: string[] }).summaryFiles .slice() .sort(); // No summary can be newer than the verification → never stale. Return before // touching git so a phase with no summaries costs zero subprocesses. (#2348) - if (summaryFiles.length === 0) return null; + if (summaryFiles.length === 0) return { determined: true, stale: false }; // Each file's effective "last changed" time = its commit time when committed // AND clean (content-tied and clone-stable), else its filesystem mtime (the @@ -337,13 +359,13 @@ function findStaleVerificationSummary( // The caller only needs whether the phase is stale, not which summary — // the first stale summary (in sorted order) is enough. Short-circuit. if (effectiveTimeMs(summaryFile) > verificationTimeMs) { - return { verificationFile, summaryFile }; + return { determined: true, stale: true, verificationFile, summaryFile }; } } - return null; + return { determined: true, stale: false }; } catch { - return null; + return { determined: false }; } } @@ -359,6 +381,12 @@ function findStaleVerificationSummary( * If no frontmatter block or no `status` key → status 'missing'. * 3. Map to routing table. Unknown non-empty value → status 'unknown'. * + * The internal staleness check can itself fail (fs / scanPhasePlans / clock + * error); when it does, `status` is routed as if nothing were stale (the + * pre-existing no-throw fail-open contract — unchanged), but the returned + * result carries `staleCheckIndeterminate: true` so a caller can distinguish + * "checked; nothing is stale" from "could not check" (#3057 B3). + * * @param phaseDir - Absolute path to the phase directory. * @param opts - Options. `opts.fs` allows test injection (defaults to node:fs). * `opts.runtime` selects the command surface `next_command` is @@ -434,8 +462,8 @@ function readVerificationStatus( }; } - const staleVerification = findStaleVerificationSummary(phaseDir, fsImpl, phaseCleanCommitTimesMs); - if (staleVerification) { + const staleCheck = findStaleVerificationSummary(phaseDir, fsImpl, phaseCleanCommitTimesMs); + if (staleCheck.determined && staleCheck.stale) { const entry = VERIFICATION_ROUTING_TABLE['stale']; return { status: entry.status, @@ -443,6 +471,12 @@ function readVerificationStatus( next_command: projectNextCommand('verify-work', runtime, phaseArg), }; } + // staleCheck is either {determined:true, stale:false} (checked; nothing + // stale) or {determined:false} (could not check — fs/scan/clock failure). + // Both fall through to normal routing below (the pre-existing no-throw + // fail-open contract is unchanged), but the indeterminate case is flagged + // on the returned result so a caller can tell the two apart (#3057 B3). + const staleCheckIndeterminate = !staleCheck.determined; // 3. Route — exclude internal sentinels from raw-file lookup (they are // constructed internally above, never written by the verifier). @@ -458,6 +492,7 @@ function readVerificationStatus( status: entry.status, next_action: entry.next_action, next_command: projectNextCommand(entry.next_command, runtime, phaseArg), + ...(staleCheckIndeterminate ? { staleCheckIndeterminate: true } : {}), }; } @@ -467,6 +502,7 @@ function readVerificationStatus( status: unknownRoute.status, next_action: `Unexpected verification status '${rawStatus}'. Re-run execute-phase verification.`, next_command: projectNextCommand(unknownRoute.next_command, runtime, phaseArg), + ...(staleCheckIndeterminate ? { staleCheckIndeterminate: true } : {}), }; } diff --git a/src/verify.cts b/src/verify.cts index 1a1a6ade9..f1e1d81db 100644 --- a/src/verify.cts +++ b/src/verify.cts @@ -2079,6 +2079,20 @@ function cmdValidateHealth( `Stale git worktree: ${worktreePath} (last modified ${finding['ageMinutes'] as number} minutes ago)`, `Run: git worktree remove ${worktreePath} --force`, ); + continue; + } + + // #3050/#3057 (B5): a 'unverified' finding means existsSync confirmed + // the worktree is present but statSync threw, so orphan/stale status + // could not be determined for THIS entry — it must not be silently + // dropped (that would be the exact fail-open the row exists to close). + if (finding['kind'] === 'unverified') { + addIssue( + 'warning', + 'W020', + `Worktree health check degraded: could not stat ${finding['path'] as string} — presence/staleness could not be verified`, + 'Check filesystem permissions on the worktree path, or investigate why statSync failed for it', + ); } } } diff --git a/src/workstream-inventory.cts b/src/workstream-inventory.cts index 9de9cfd25..6612fe7dd 100644 --- a/src/workstream-inventory.cts +++ b/src/workstream-inventory.cts @@ -46,6 +46,22 @@ interface PhaseFileCounts { interface InspectWorkstreamOptions { active?: string | null; + /** + * #3057 B3: injectable diagnostic-write seam, mirrored from + * cmdGitBaseBranch's `writeDiagnostic` (git-base-branch.cts). Called with a + * stderr-style line when a phase's readVerificationStatus staleness check + * could not run to completion — WorkstreamInventory's own return shape + * (`phases: PhaseStatus[]`) has no per-phase verification detail today, so + * this side channel surfaces the fact without widening that aggregate type. + * The second argument carries the same facts as structured, typed data + * (CONTRIBUTING: no raw-text matching on produced diagnostics) so a caller + * — tests included — can assert on `phaseDir`/`reason` directly instead of + * pattern-matching the operator-facing `message`. The default + * implementation writes only `message` to stderr; operator output is + * unchanged. Never affects routing: the ledger / rollup computation below + * is unchanged either way (the pre-existing fail-open contract). + */ + writeDiagnostic?: (message: string, meta: { phaseDir: string; reason: string }) => void; } interface WorkstreamInventoryList { @@ -470,6 +486,7 @@ function inspectWorkstream(cwd: string, name: string, options: InspectWorkstream if (!fs.existsSync(wsDir)) return null; const activeWorkstreamName = options.active === undefined ? getActiveWorkstream(cwd) : options.active; + const writeDiagnostic = options.writeDiagnostic ?? ((message: string) => process.stderr.write(message)); const p = planningPaths(cwd, name); const phaseDirNames = readSubdirectories(p.phases); @@ -582,6 +599,20 @@ function inspectWorkstream(cwd: string, name: string, options: InspectWorkstream const rawPhaseEntries = [...phaseDirNames].sort().map(dir => { const phaseDir = path.join(p.phases, dir); const counts = countPhaseFiles(phaseDir); + const verificationResult = readVerificationStatus(phaseDir); + // #3057 B3: routing is UNCHANGED — `liveVerificationStatus` below is still + // `.status`, exactly as before, so the ledger/rollup logic that consumes + // it is unaffected. This only makes an indeterminate staleness check + // visible (stderr), matching cmdGitBaseBranch's own non-blocking + // unverified-fallback diagnostic (#3057 B4) — the closest existing idiom, + // since `WorkstreamInventory`'s aggregate return shape carries no + // per-phase verification detail for this to attach to. + if (verificationResult.staleCheckIndeterminate) { + writeDiagnostic( + `⚠ workstream-inventory: verification staleness check could not complete for phase directory '${dir}' in workstream '${name}' — routed as not-stale, but this was not actually verified. See #3057.\n`, + { phaseDir: dir, reason: 'staleCheckIndeterminate' }, + ); + } return { directory: dir, phaseKey: phaseKeyFromDir(dir), @@ -589,7 +620,7 @@ function inspectWorkstream(cwd: string, name: string, options: InspectWorkstream planCount: counts.planCount, summaryCount: counts.summaryCount, inMilestone: isDirInCurrentMilestone(dir), - liveVerificationStatus: readVerificationStatus(phaseDir).status, + liveVerificationStatus: verificationResult.status, }; }); diff --git a/src/worktree-base-ref.cts b/src/worktree-base-ref.cts index f2350677f..371654082 100644 --- a/src/worktree-base-ref.cts +++ b/src/worktree-base-ref.cts @@ -347,6 +347,18 @@ export function evaluateWorktreeBaseDegrade(deps?: { headSha: string | null; forkRef: string | null; forkSha: string | null; + /** + * Only meaningful when `reason === 'no-head'` (both non-degrade outcomes); + * `null` for every other reason. `true` for exit 128 — git's definitive + * "not a git repository" answer. `false` for exit 0 with empty stdout: git + * completed but did NOT give a confirmed "no HEAD" answer, unlike exit 128 + * — this outcome is left `shouldDegrade:false` unchanged (pinned by an + * existing regression guard; the underlying product question of whether it + * SHOULD degrade is still open, see #3050 review), but a caller can now + * tell the two `'no-head'` causes apart instead of treating them as the + * same verified answer. (#3057 B8) + */ + headAbsenceVerified: boolean | null; } { const execGit: ExecGitFn = deps?.execGit ?? execGitSeam; const cwd = deps?.cwd; @@ -358,7 +370,7 @@ export function evaluateWorktreeBaseDegrade(deps?: { // complete: any non-"head" value (including "fresh" and absent/null) has fresh/origin-HEAD // semantics and must be evaluated against origin/HEAD. (Reference: Claude Code worktrees docs, #683.) if (deps?.effectiveBaseRef === 'head') { - return { shouldDegrade: false, reason: 'baseref-head', message: null, headSha: null, forkRef: null, forkSha: null }; + return { shouldDegrade: false, reason: 'baseref-head', message: null, headSha: null, forkRef: null, forkSha: null, headAbsenceVerified: null }; } // b. Resolve HEAD sha. @@ -367,7 +379,7 @@ export function evaluateWorktreeBaseDegrade(deps?: { // git repository" and must fail closed (distinct from the clean-exit-128 // "no-head" case below, which genuinely completed and reported no HEAD). if (isExecGitTimeout(headResult)) { - return { shouldDegrade: true, reason: 'head-unresolvable', message: MSG_HEAD_UNRESOLVABLE, headSha: null, forkRef: null, forkSha: null }; + return { shouldDegrade: true, reason: 'head-unresolvable', message: MSG_HEAD_UNRESOLVABLE, headSha: null, forkRef: null, forkSha: null, headAbsenceVerified: null }; } const headStdout = headResult.stdout ? headResult.stdout.trim() : ''; // exit 128 is git's definitive "not a git repository" answer — it completed @@ -375,14 +387,19 @@ export function evaluateWorktreeBaseDegrade(deps?: { // stays a benign non-degrade; every other non-success outcome below is // NOT a definitive answer from git and must fail closed (#3050). if (headResult.exitCode === 128) { - return { shouldDegrade: false, reason: 'no-head', message: null, headSha: null, forkRef: null, forkSha: null }; + return { shouldDegrade: false, reason: 'no-head', message: null, headSha: null, forkRef: null, forkSha: null, headAbsenceVerified: true }; } // Exit 0 with empty stdout is pinned as benign no-degrade by an existing // regression guard (tests/worktree-base-ref.test.cjs — "git rev-parse HEAD // returns empty stdout"). Left unchanged deliberately; flagged in the // #3050 review for a product-intent call rather than silently flipped. + // Unlike the exit-128 case above, git did NOT give a definitive "no HEAD" + // answer here — `headAbsenceVerified:false` names that gap explicitly + // instead of leaving it folded into an identical-looking 'no-head' reason + // (#3057 B8; the product question of whether this SHOULD degrade is + // unchanged and still open). if (headResult.exitCode === 0 && !headStdout) { - return { shouldDegrade: false, reason: 'no-head', message: null, headSha: null, forkRef: null, forkSha: null }; + return { shouldDegrade: false, reason: 'no-head', message: null, headSha: null, forkRef: null, forkSha: null, headAbsenceVerified: false }; } if (headResult.exitCode !== 0) { // Any other non-success outcome (e.g. exit 127 — git missing — or any @@ -391,7 +408,7 @@ export function evaluateWorktreeBaseDegrade(deps?: { // (`!headStdout` was previously OR'd in here but is unreachable: the // exitCode===0 && !headStdout case is already handled above, and every // other branch here has exitCode!==0 already true — #3050 review.) - return { shouldDegrade: true, reason: 'head-unresolvable', message: MSG_HEAD_UNRESOLVABLE, headSha: null, forkRef: null, forkSha: null }; + return { shouldDegrade: true, reason: 'head-unresolvable', message: MSG_HEAD_UNRESOLVABLE, headSha: null, forkRef: null, forkSha: null, headAbsenceVerified: null }; } const headSha = headStdout; @@ -423,11 +440,11 @@ export function evaluateWorktreeBaseDegrade(deps?: { // d. Evaluate. if (forkSha === null) { - return { shouldDegrade: true, reason: 'fork-ref-unknown', message: MSG_UNKNOWN, headSha, forkRef: null, forkSha: null }; + return { shouldDegrade: true, reason: 'fork-ref-unknown', message: MSG_UNKNOWN, headSha, forkRef: null, forkSha: null, headAbsenceVerified: null }; } if (forkSha === headSha) { - return { shouldDegrade: false, reason: 'head-matches-fork', message: null, headSha, forkRef, forkSha }; + return { shouldDegrade: false, reason: 'head-matches-fork', message: null, headSha, forkRef, forkSha, headAbsenceVerified: null }; } const message = buildMsgDiverged(headSha, forkRef, forkSha); - return { shouldDegrade: true, reason: 'head-diverged-from-fork', message, headSha, forkRef, forkSha }; + return { shouldDegrade: true, reason: 'head-diverged-from-fork', message, headSha, forkRef, forkSha, headAbsenceVerified: null }; } diff --git a/src/worktree-safety.cts b/src/worktree-safety.cts index 26433a046..857439fa8 100644 --- a/src/worktree-safety.cts +++ b/src/worktree-safety.cts @@ -236,17 +236,26 @@ function planWorktreePrune(repoRoot: string, options: { allowDestructive?: boole } let worktrees: WorktreeBranchEntry[] = []; + let parseFailed = false; try { worktrees = parsePorcelain(listed.porcelain); } catch { // Keep historical behavior: still run metadata prune when parsing fails. + // #3050/#3057 (B6): but the reason must NOT collide with the + // genuinely-empty-list case below — a parser that could not read the + // porcelain output is not the same fact as "there are no worktrees", and + // this plan drives a PRUNE, so conflating them means a prune decision made + // on unread data would be indistinguishable from one made on real data. worktrees = []; + parseFailed = true; } return { repoRoot, action: 'metadata_prune_only', - reason: worktrees.length === 0 ? 'no_worktrees' : 'worktrees_present', + reason: parseFailed + ? 'parse_failed' + : (worktrees.length === 0 ? 'no_worktrees' : 'worktrees_present'), destructiveModeRequested, }; } @@ -326,7 +335,7 @@ function listLinkedWorktreePaths(repoRoot: string, deps: WorktreeDeps = {}): Lin } interface WorktreeFinding { - kind: 'orphan' | 'stale'; + kind: 'orphan' | 'stale' | 'unverified'; path: string; ageMinutes?: number; } @@ -349,13 +358,25 @@ function inspectWorktreeHealth(repoRoot: string, options: { staleAfterMs?: numbe const findings: WorktreeFinding[] = []; for (const entry of inventory.entries) { - if (!entry.exists) { + if (entry.exists === 'absent') { findings.push({ kind: 'orphan', path: entry.path, }); continue; } + if (entry.exists === 'unverified') { + // #3050/#3057 (B5): existsSync confirmed the path is present but statSync + // threw, so age/staleness could not be determined. This is neither + // "orphan" (existsSync says it IS there) nor "healthy" (we never verified + // it) — surface it as its own finding so a caller can't silently treat an + // unverifiable worktree as confirmed present-and-not-stale. + findings.push({ + kind: 'unverified', + path: entry.path, + }); + continue; + } if (entry.isStale) { findings.push({ kind: 'stale', @@ -374,7 +395,15 @@ function inspectWorktreeHealth(repoRoot: string, options: { staleAfterMs?: numbe interface InventoryEntry { path: string; - exists: boolean; + /** + * Tri-state (#3050/#3057, B5): `'present'` = existsSync AND statSync both + * succeeded (confirmed present, age known); `'absent'` = existsSync confirmed + * the path is genuinely absent; `'unverified'` = existsSync confirmed presence + * but statSync threw — presence could not be fully verified. `'unverified'` + * MUST NOT be treated as `'present'`: a caller that could not check must not + * report the worktree as confirmed present. + */ + exists: 'present' | 'absent' | 'unverified'; isStale: boolean; ageMinutes: number | null; } @@ -401,7 +430,7 @@ function snapshotWorktreeInventory(repoRoot: string, options: { staleAfterMs?: n const entries: InventoryEntry[] = []; for (const worktreePath of listed.paths) { - let exists = false; + let exists: 'present' | 'absent' | 'unverified' = 'absent'; let isStale = false; let ageMinutes: number | null = null; @@ -415,16 +444,21 @@ function snapshotWorktreeInventory(repoRoot: string, options: { staleAfterMs?: n continue; } - exists = true; try { const stat = statSync(worktreePath); + exists = 'present'; const ageMs = nowMs - stat.mtimeMs; ageMinutes = Math.round(ageMs / 60000); if (ageMs > staleAfterMs) { isStale = true; } } catch { - // Keep historical behavior: stat failures are ignored. + // #3050/#3057 (B5): a statSync throw means presence could not be + // verified — do NOT report exists:'present' (a guard that could not + // check must not claim the worktree is confirmed present). Distinguish + // from the genuinely-absent case above with a third state ('unverified') + // rather than silently falling through to the pre-existing 'present' default. + exists = 'unverified'; } entries.push({ path: worktreePath, diff --git a/tests/external-job.test.cjs b/tests/external-job.test.cjs index f9fd5681f..3ffd24fa2 100644 --- a/tests/external-job.test.cjs +++ b/tests/external-job.test.cjs @@ -185,8 +185,9 @@ test('parseSacctRow parses [jobid, state] columns', () => { // ─── writeManifest (fail-closed duplicate guard + fs injection) ──────────────── -function memFs(files = {}) { +function memFs(files = {}, opts = {}) { const store = new Map(Object.entries(files)); + const failReads = new Map(Object.entries(opts.failReads || {})); return { mkdirSync: () => undefined, readdirSync: (d) => { @@ -194,6 +195,9 @@ function memFs(files = {}) { return Array.isArray(set) ? set : []; }, readFileSync: (p) => { + if (failReads.has(p)) { + throw new Error(failReads.get(p)); + } if (!store.has(p)) { const e = new Error('enoent'); e.code = 'ENOENT'; throw e; } return store.get(p); }, @@ -268,6 +272,129 @@ test('writeManifest fails closed on a malformed existing manifest', () => { assert.strictEqual(res.kind, 'malformed_existing'); }); +// ─── writeManifest — negative-space: an incomplete duplicate scan (#3057) ───── +// A corrupt/unreadable SIBLING manifest must not be silently skipped: the +// duplicate scan cannot be trusted to have inspected it, so the write must +// refuse rather than risk dispatching a duplicate external job. + +test('an unreadable sibling manifest refuses the write', () => { + // A1 + const dir = path.join('.planning', 'async-jobs'); + const siblingPath = path.join(dir, '99999.json'); + const clock = { nowIso: () => '2020-06-15T12:00:00.000Z' }; + const fs = memFs( + { [dir]: ['99999.json'] }, + { failReads: { [siblingPath]: 'eio: input/output error' } }, + ); + const manifest = buildManifest(baseInput(), { clock }); + const res = writeManifest(manifest, '.planning', { fs, clock }); + assert.strictEqual(res.ok, false); + assert.strictEqual(res.kind, 'scan_incomplete'); + assert.strictEqual(res.offendingPath, siblingPath, 'offendingPath must name the unreadable file'); +}); + +test('an unparseable sibling manifest refuses the write', () => { + // A2 + const dir = path.join('.planning', 'async-jobs'); + const siblingPath = path.join(dir, '99999.json'); + const clock = { nowIso: () => '2020-06-15T12:00:00.000Z' }; + const fs = memFs({ [dir]: ['99999.json'], [siblingPath]: '{not json' }); + const manifest = buildManifest(baseInput(), { clock }); + const res = writeManifest(manifest, '.planning', { fs, clock }); + assert.strictEqual(res.ok, false); + assert.strictEqual(res.kind, 'scan_incomplete'); + assert.strictEqual(res.offendingPath, siblingPath, 'offendingPath must name the unparseable file'); +}); + +test('target and sibling failures stay distinct kinds', () => { + // A3 + const dir = path.join('.planning', 'async-jobs'); + const targetPath = path.join(dir, '12345.json'); + const clock = { nowIso: () => '2020-06-15T12:00:00.000Z' }; + const fs = memFs({ [dir]: ['12345.json'], [targetPath]: '{not json' }); + const manifest = buildManifest(baseInput(), { clock }); + const res = writeManifest(manifest, '.planning', { fs, clock }); + assert.strictEqual(res.ok, false); + assert.strictEqual(res.kind, 'malformed_existing', 'the TARGET failure must stay malformed_existing, not scan_incomplete'); +}); + +// A4 (paired): prove the sibling really holds a duplicate (control), then +// prove that making that SAME content unreadable still refuses (regression). + +test('control: a readable sibling with a non-terminal duplicate plan_id is duplicate_plan_id', () => { + // A4a — control. Same construction as the "FAILS CLOSED on duplicate + // plan_id" test above: valid JSON, same plan_id, different job_id, + // non-terminal status. This establishes the sibling content genuinely + // triggers the duplicate guard when it CAN be read. + const dir = path.join('.planning', 'async-jobs'); + const siblingPath = path.join(dir, '99999.json'); + const clock = { nowIso: () => '2020-06-15T12:00:00.000Z' }; + const duplicate = buildManifest({ ...baseInput(), job_id: '99999' }, { clock }); + const fs = memFs({ [dir]: ['99999.json'], [siblingPath]: JSON.stringify(duplicate) }); + const manifest = buildManifest(baseInput(), { clock }); + const res = writeManifest(manifest, '.planning', { fs, clock }); + assert.strictEqual(res.ok, false); + assert.strictEqual(res.kind, 'duplicate_plan_id', 'the sibling content must be a real duplicate trigger when readable'); +}); + +test('a corrupt sibling can no longer hide a duplicate dispatch', () => { + // A4b — the bug, as a test. Same sibling PATH as the control above, which + // that test proves can hold a genuine non-terminal duplicate for this + // plan_id. Here the read itself faults (opts.failReads), so no content is + // reachable at all — and that is the point: the guard now refuses on ANY + // unreadable sibling precisely because it cannot know whether that file + // was the duplicate. The control supplies the "it could have been"; this + // supplies the "and we no longer gamble on it". + // + // Before the fix, an unreadable sibling was `continue`d past, the + // duplicate was never seen, and this returned ok:true — dispatching a + // duplicate external job. + const dir = path.join('.planning', 'async-jobs'); + const siblingPath = path.join(dir, '99999.json'); + const clock = { nowIso: () => '2020-06-15T12:00:00.000Z' }; + const fs = memFs( + { [dir]: ['99999.json'] }, + { failReads: { [siblingPath]: 'eio: input/output error' } }, + ); + const manifest = buildManifest(baseInput(), { clock }); + const res = writeManifest(manifest, '.planning', { fs, clock }); + assert.strictEqual(res.ok, false, 'a corrupt sibling must refuse, not silently permit a possible duplicate dispatch'); + assert.strictEqual(res.kind, 'scan_incomplete'); +}); + +test('happy path unaffected', () => { + // A5 + const dir = path.join('.planning', 'async-jobs'); + const clock = { nowIso: () => '2020-06-15T12:00:00.000Z' }; + const fs = memFs({ [dir]: [] }); + const manifest = buildManifest(baseInput(), { clock }); + const res = writeManifest(manifest, '.planning', { fs, clock }); + assert.strictEqual(res.ok, true); +}); + +test('a terminal prior job is still allowed', () => { + // A6 + const dir = path.join('.planning', 'async-jobs'); + const otherPath = path.join(dir, '99999.json'); + const clock = { nowIso: () => '2020-06-15T12:00:00.000Z' }; + const dead = buildManifest({ ...baseInput(), job_id: '99999', status: 'failed', terminal_details: { code: 1 } }, { clock }); + const fs = memFs({ [dir]: ['99999.json'], [otherPath]: JSON.stringify(dead) }); + const manifest = buildManifest(baseInput(), { clock }); + const res = writeManifest(manifest, '.planning', { fs, clock }); + assert.strictEqual(res.ok, true, 'the duplicate guard only protects against re-dispatching live work'); +}); + +test('a directory read failure is io_error, not scan_incomplete', () => { + // A7 + const clock = { nowIso: () => '2020-06-15T12:00:00.000Z' }; + const fs = memFs({}, { failReads: {} }); + fs.readdirSync = () => { throw new Error('eio: input/output error'); }; + const manifest = buildManifest(baseInput(), { clock }); + const res = writeManifest(manifest, '.planning', { fs, clock }); + assert.strictEqual(res.ok, false); + assert.strictEqual(res.kind, 'io_error'); +}); + // ─── Property-based (CLAUDE.md: parsers/contracts need a fast-check test) ───── test('property: mapSlurmState is total and idempotent over the known alphabet', () => { diff --git a/tests/fragment-single-edit-propagation.install.test.cjs b/tests/fragment-single-edit-propagation.install.test.cjs index d27ef3911..fa3a03398 100644 --- a/tests/fragment-single-edit-propagation.install.test.cjs +++ b/tests/fragment-single-edit-propagation.install.test.cjs @@ -1181,14 +1181,24 @@ test('regenDerivedPropagatesSingleFragmentEditWithNoSecondSourceSurface', (t) => cwd: overlay, encoding: 'utf8', env: installerEnv(), - timeout: 300000, + // `regen:derived` chains a full `npm run build` plus eight generators — + // the single heaviest subprocess in this suite. 300_000 (5min) was + // observed to be killed (status: null) near the very end of a genuinely + // completed run on a loaded bench (linux-node22), not from a real hang. + // 900_000 (15min) is deliberately generous so this can never again flake + // on load while still catching a true hang. + timeout: 900000, maxBuffer: 64 * 1024 * 1024, }); assert.equal( regen.status, 0, - `npm run regen:derived must succeed inside the copy-mode overlay\n` + - `stdout: ${regen.stdout}\nstderr: ${regen.stderr}`, + regen.status === null + ? `npm run regen:derived was KILLED (status: null, signal: ${regen.signal}) inside the copy-mode ` + + `overlay — likely a timeout, not a build failure; the captured output below may show the build ` + + `actually completed\nstdout: ${regen.stdout}\nstderr: ${regen.stderr}` + : `npm run regen:derived must succeed inside the copy-mode overlay (exit ${regen.status})\n` + + `stdout: ${regen.stdout}\nstderr: ${regen.stderr}`, ); // 3. P2, now non-tautological because real writers ran: compare the diff --git a/tests/git-base-branch.test.cjs b/tests/git-base-branch.test.cjs index 17a17f552..dcbde474f 100644 --- a/tests/git-base-branch.test.cjs +++ b/tests/git-base-branch.test.cjs @@ -30,6 +30,7 @@ const path = require('node:path'); const { execSync } = require('node:child_process'); const { runGsdTools, cleanup, readFileNormalized } = require('./helpers.cjs'); +const { makeFaultyGit } = require('./helpers/faulty-deps.cjs'); // ─── helpers ────────────────────────────────────────────────────────────────── @@ -301,6 +302,82 @@ describe('#1268 gitWorktreeInfoInternal: relocation to git-base-branch', () => { }); }); +// ─── #3057 B4: last-resort "main" — verified vs unverified ─────────────────── +// +// `resolveBaseBranch` alone collapses two very different situations into the +// same `'main'` string: a repository that genuinely has no candidate branch +// (every git query on tiers 2-4 completed and cleanly answered "nothing"), +// and a total resolution failure (every query timed out). `resolveBaseBranchDiagnostics` +// exposes `verified` so a caller can tell them apart; `cmdGitBaseBranch` +// surfaces the unverified case as a stderr diagnostic without touching its +// stdout contract (five workflows parse that stdout literally). + +describe('#3057 B4: resolveBaseBranchDiagnostics — verified vs unverified last-resort default', () => { + test('every tier-2/3/4 git query TIMES OUT → last-resort "main" is UNVERIFIED', (t) => { + const dir = fs.mkdtempSync(path.join(os.tmpdir(), 'gsd-3057-b4-fault-')); + t.after(() => cleanup(dir)); + // No .planning/config.json in this dir → the config-override tier is + // skipped naturally (readConfigBaseBranch's real-fs read misses cleanly). + const faultyGit = makeFaultyGit({ faults: [{ kind: 'timeout' }] }); + + const result = gitBaseBranch.resolveBaseBranchDiagnostics(dir, { execGit: faultyGit }); + + assert.strictEqual(result.branch, 'main'); + assert.strictEqual(result.verified, false); + }); + + test('every tier-2/3/4 git query cleanly reports no candidate → last-resort "main" is VERIFIED', (t) => { + const dir = fs.mkdtempSync(path.join(os.tmpdir(), 'gsd-3057-b4-clean-')); + t.after(() => cleanup(dir)); + // Default passthrough: exitCode 0, empty stdout for every call — a real, + // completed "no answer" from git, not a failure (timedOut:false, error:null). + const faultyGit = makeFaultyGit(); + + const result = gitBaseBranch.resolveBaseBranchDiagnostics(dir, { execGit: faultyGit }); + + assert.strictEqual(result.branch, 'main'); + assert.strictEqual(result.verified, true); + }); + + test('resolveBaseBranch (string-returning) is unaffected — both cases still return "main"', (t) => { + const dir = fs.mkdtempSync(path.join(os.tmpdir(), 'gsd-3057-b4-compat-')); + t.after(() => cleanup(dir)); + assert.strictEqual( + gitBaseBranch.resolveBaseBranch(dir, { execGit: makeFaultyGit({ faults: [{ kind: 'timeout' }] }) }), + 'main', + ); + assert.strictEqual( + gitBaseBranch.resolveBaseBranch(dir, { execGit: makeFaultyGit() }), + 'main', + ); + }); + + test('cmdGitBaseBranch writes an unverified-fallback diagnostic to stderr ONLY when unverified', (t) => { + const dir = fs.mkdtempSync(path.join(os.tmpdir(), 'gsd-3057-b4-cmd-')); + t.after(() => cleanup(dir)); + + let stdoutText = ''; + let stderrText = ''; + gitBaseBranch.cmdGitBaseBranch(dir, [], { + execGit: makeFaultyGit({ faults: [{ kind: 'timeout' }] }), + write: (s) => { stdoutText += s; }, + writeDiagnostic: (s) => { stderrText += s; }, + }); + assert.strictEqual(stdoutText, 'main\n'); + assert.ok(stderrText.length > 0, 'the unverified fallback must write a stderr diagnostic'); + + stdoutText = ''; + stderrText = ''; + gitBaseBranch.cmdGitBaseBranch(dir, [], { + execGit: makeFaultyGit(), + write: (s) => { stdoutText += s; }, + writeDiagnostic: (s) => { stderrText += s; }, + }); + assert.strictEqual(stdoutText, 'main\n'); + assert.strictEqual(stderrText, '', 'a verified fallback must not write any diagnostic'); + }); +}); + // ─── setGsdConfig prototype-pollution guard (#1406) ─────────────────────────── describe('#1406: setGsdConfig prototype-pollution guard', () => { diff --git a/tests/init.test.cjs b/tests/init.test.cjs index 88e0b2ee6..7236a2ad3 100644 --- a/tests/init.test.cjs +++ b/tests/init.test.cjs @@ -2775,6 +2775,112 @@ describe('#2376 — init.* path fields resolve when process cwd differs from --c }); }); +// ───────────────────────────────────────────────────────────────────────────── +// #3057 B3: cmdInitVerifyWork surfaces an indeterminate staleness check +// +// buildPhaseCompletionProjection (init.cts) projects readVerificationStatus's +// result into phase_completion — a workflow step reads phase_completion.* +// fields directly. Pre-#3057 B3 wiring, `staleCheckIndeterminate` was +// computed by readVerificationStatus but dropped here, so a workflow could +// never distinguish "checked; nothing is stale" from "could not check". +// ───────────────────────────────────────────────────────────────────────────── + +describe('#3057 B3: cmdInitVerifyWork — verification staleness-check indeterminate is surfaced', () => { + const initMod = require(path.join(__dirname, '..', 'gsd-core', 'bin', 'lib', 'init.cjs')); + let projectDir; + + beforeEach(() => { + projectDir = createFixture(); + }); + + afterEach(() => { + cleanup(projectDir); + }); + + /** + * In-process capture of cmdInitVerifyWork's stdout JSON, stderr discarded. + * + * io.cts's `output()` writes via `writeAllSync` → `fs.writeSync(1, ...)` + * directly (bug #1008's non-blocking-pipe fix), NOT `process.stdout.write` + * — so mocking `process.stdout.write` here silently captures nothing and + * every assertion below saw `JSON.parse('')` ("Unexpected end of JSON + * input") regardless of what cmdInitVerifyWork actually produced. The fix + * is the fd-level seam tests/io.test.cjs already established for exactly + * this function (bug #1008's `t.mock.method(fs, 'writeSync', ...)` + * pattern): intercept fd 1, discard fd 2, and pass every OTHER fd through + * to the real writeSync — any code path that opens its own fd (e.g. a + * lock file) must still actually write, not be silently swallowed as if + * it were stdout. + */ + function captureInitVerifyWork(t, cwd, phase) { + const chunks = []; + const origWriteSync = fs.writeSync.bind(fs); + t.mock.method(fs, 'writeSync', (fd, data, offset, length) => { + if (fd === 2) return Buffer.isBuffer(data) ? data.length : String(data).length; + if (fd !== 1) return origWriteSync(fd, data, offset, length); + const chunk = Buffer.isBuffer(data) + ? data.subarray(offset ?? 0, length === undefined ? data.length : (offset ?? 0) + length).toString('utf8') + : String(data); + chunks.push(chunk); + return Buffer.byteLength(chunk, 'utf8'); + }); + initMod.cmdInitVerifyWork(cwd, phase, false); + const captured = chunks.join(''); + assert.ok(captured.length > 0, 'cmdInitVerifyWork produced no stdout output'); + return captured; + } + + function seedVerifiedPhase() { + seedPhase(projectDir, '03-api', { + '03-01-PLAN.md': '# Plan', + '03-01-SUMMARY.md': '# Summary', + '03-VERIFICATION.md': '---\nstatus: passed\n---\n\n# Verification\n', + }); + fs.writeFileSync(path.join(projectDir, '.planning', 'STATE.md'), '# State\n'); + fs.writeFileSync(path.join(projectDir, '.planning', 'ROADMAP.md'), '# Roadmap\n'); + const phaseDir = path.join(projectDir, '.planning', 'phases', '03-api'); + const summaryPath = path.join(phaseDir, '03-01-SUMMARY.md'); + const verificationPath = path.join(phaseDir, '03-VERIFICATION.md'); + // Deterministic mtime ordering (never rely on write-order clock ties): + // verification strictly newer than the summary → a completed check finds + // nothing stale. + const older = new Date('2026-01-01T00:00:00.000Z'); + const newer = new Date('2026-01-01T00:01:00.000Z'); + fs.utimesSync(summaryPath, older, older); + fs.utimesSync(verificationPath, newer, newer); + return { summaryPath, verificationPath }; + } + + test('an fs failure inside the staleness check sets phase_completion.verification_stale_check_indeterminate:true', (t) => { + const { summaryPath, verificationPath } = seedVerifiedPhase(); + const origStatSync = fs.statSync; + + t.mock.method(fs, 'statSync', function injectedStaleCheckFault(target, ...args) { + const targetPath = String(target); + if (targetPath === verificationPath || targetPath === summaryPath) { + throw new Error('injected stat failure (#3057 B3)'); + } + return origStatSync.call(fs, target, ...args); + }); + + const output = JSON.parse(captureInitVerifyWork(t, projectDir, '03')); + + // Pre-existing no-throw fail-open routing is UNCHANGED: status still + // resolves to 'passed' exactly as it would without the injected fault. + assert.strictEqual(output.phase_completion.verification_status, 'passed'); + assert.strictEqual(output.phase_completion.verification_stale_check_indeterminate, true); + }); + + test('a completed staleness check that finds nothing stale reports verification_stale_check_indeterminate:false', (t) => { + seedVerifiedPhase(); + + const output = JSON.parse(captureInitVerifyWork(t, projectDir, '03')); + + assert.strictEqual(output.phase_completion.verification_status, 'passed'); + assert.strictEqual(output.phase_completion.verification_stale_check_indeterminate, false); + }); +}); + // ───────────────────────────────────────────────────────────────────────────── // roadmap analyze command // ───────────────────────────────────────────────────────────────────────────── diff --git a/tests/phase.test.cjs b/tests/phase.test.cjs index 0a2a67f8a..b50cde6ad 100644 --- a/tests/phase.test.cjs +++ b/tests/phase.test.cjs @@ -8374,23 +8374,46 @@ function extractFrontmatterField(stateContent, fieldName) { return fieldMatch ? fieldMatch[1].trim() : null; } -// Capture stdout from cmdPhaseComplete (it calls output() which writes to stdout) -function capturePhaseComplete(cwd, phaseNum) { - // We invoke gsd-tools directly for the full CJS path, but with GSD_DISABLE_SDK_BRIDGE=1 - // to force the CJS implementation. Since no env var disables bridge, we call cmdPhaseComplete - // directly and redirect output capture. - const chunks = []; - const origWrite = process.stdout.write.bind(process.stdout); - const origErrWrite = process.stderr.write.bind(process.stderr); - process.stdout.write = (chunk) => { chunks.push(chunk); return true; }; - process.stderr.write = () => true; - try { - cmdPhaseComplete(cwd, phaseNum, false); - } finally { - process.stdout.write = origWrite; - process.stderr.write = origErrWrite; +// Capture stdout from `gsd-tools phase complete ` via a REAL subprocess. +// +// #3057: this used to call cmdPhaseComplete(...) IN-PROCESS and capture its +// stdout by monkeypatching fs.writeSync (io.cts's output() writes via +// writeAllSync -> fs.writeSync(1, ...), not process.stdout.write — bug #1008's +// non-blocking-pipe fix). That interception shares the exact seam the remote +// matrix's own event-stream capture depends on: when the runner's stdout is +// redirected to a file (the common case for a captured CI child), +// `process.stdout.write` itself resolves through that SAME public +// `fs.writeSync`, so any write racing the patched window — including the +// runner's own test:pass/test:fail events — could be silently swallowed into +// `chunks` or dropped instead of reaching the real fd. Two attempts to make +// that interception safe (manual save/restore, then `t.mock.method`) both +// still left this file reporting zero test:pass/test:fail events on the +// remote matrix. The fix is to stop intercepting fd 1 altogether: run the +// real CLI in a real subprocess, exactly like every other test in this file +// already does via `runGsdTools`, so the OS owns stdout capture and the +// runner's own event stream is never at risk. +// +// The phase family router (phase-command-router.cjs) calls +// `phase.cmdPhaseComplete(cwd, phaseNum, raw)` directly for `phase complete` +// — no SDK delegation on this path — so this reaches the exact same CJS +// function the old in-process call did. +// +// A handful of call sites depend on state a subprocess cannot see (a mock +// installed in THIS process, or the parent's own fs.writeFileSync mock for +// the rollback-failure tests below); those call sites do not use this +// helper — see the inline notes at each one. +function capturePhaseComplete(t, cwd, phaseNum) { + const result = runGsdTools(['phase', 'complete', String(phaseNum)], cwd); + if (!result.success) { + // Surface exitCode/error verbatim so a real failure never presents as a + // downstream `JSON.parse('')` error, and so assert.throws() callers keep + // matching against the real stderr text (e.g. "verification is + // incomplete..."). + throw new Error( + result.error || `cmdPhaseComplete failed (exitCode=${result.exitCode})`, + ); } - return chunks.join(''); + return result.output; } // ── T1: Double invocation must NOT double-increment Completed Phases ───────── @@ -8406,9 +8429,9 @@ describe('issue #4 (CJS): cmdPhaseComplete — idempotency (blind-increment bug) cleanup(tmpDir); }); - test('T1: double invocation does NOT double-increment Completed Phases in STATE.md body', () => { + test('T1: double invocation does NOT double-increment Completed Phases in STATE.md body', (t) => { // First call — legitimate completion - capturePhaseComplete(tmpDir, '1'); + capturePhaseComplete(t, tmpDir, '1'); const stateAfter1 = readStateMd(tmpDir); const completedAfter1Body = extractField(stateAfter1, 'Completed Phases'); @@ -8426,7 +8449,7 @@ describe('issue #4 (CJS): cmdPhaseComplete — idempotency (blind-increment bug) ); // Second call on the same phase — must be idempotent - capturePhaseComplete(tmpDir, '1'); + capturePhaseComplete(t, tmpDir, '1'); const stateAfter2 = readStateMd(tmpDir); const completedAfter2Body = extractField(stateAfter2, 'Completed Phases'); @@ -8465,8 +8488,15 @@ describe('issue #4 (CJS): cmdPhaseComplete — idempotency (blind-increment bug) return originalWriteFileSync.call(this, target, ...args); }); + // Calls cmdPhaseComplete directly (bypassing capturePhaseComplete's + // subprocess helper): this test's fault is a `t.mock.method(fs, + // 'writeFileSync', ...)` installed in THIS process, which a subprocess + // cannot see. No stdout capture is needed here — only the thrown + // exception and the resulting on-disk file state — so calling the CJS + // function directly does not touch fd 1 and does not reinstate the + // fs.writeSync interception this file removed. assert.throws( - () => capturePhaseComplete(tmpDir, '1'), + () => cmdPhaseComplete(tmpDir, '1', false), /injected STATE\.md write failure/, ); @@ -8500,8 +8530,11 @@ describe('issue #4 (CJS): cmdPhaseComplete — idempotency (blind-increment bug) return originalWriteFileSync.call(this, target, ...args); }); + // See the parent-process-mock note in the STATE.md-write-fails test + // above: this must call cmdPhaseComplete directly, not via + // capturePhaseComplete's subprocess helper. assert.throws( - () => capturePhaseComplete(tmpDir, '1'), + () => cmdPhaseComplete(tmpDir, '1', false), /injected REQUIREMENTS\.md write failure/, ); @@ -8535,13 +8568,123 @@ describe('issue #4 (CJS): cmdPhaseComplete — idempotency (blind-increment bug) return originalWriteFileSync.call(this, target, ...args); }); + // See the parent-process-mock note in the STATE.md-write-fails test + // above: this must call cmdPhaseComplete directly, not via + // capturePhaseComplete's subprocess helper. assert.throws( - () => capturePhaseComplete(tmpDir, '1'), + () => cmdPhaseComplete(tmpDir, '1', false), /injected REQUIREMENTS\.md write failure[\s\S]*WARNING: rollback failed while restoring[\s\S]*injected ROADMAP\.md rollback failure/, ); }); }); +// ───────────────────────────────────────────────────────────────────────────── +// #3057 B3: cmdPhaseComplete surfaces an indeterminate staleness check +// +// readVerificationStatus's internal staleness check can itself fail (fs / +// scanPhasePlans / clock error). Pre-#3057 B3, that failure was silently +// identical to a completed check that genuinely found nothing stale — the +// SAME fail-open shape as #3050. B3 flags this on the result +// (`staleCheckIndeterminate`); this test proves phase.cts actually SURFACES +// that flag (into `warnings[]`, the same advisory channel the UAT/VERIFICATION +// pre-scan above already uses) rather than dropping it on the floor. +// ───────────────────────────────────────────────────────────────────────────── + +describe('#3057 B3: cmdPhaseComplete — verification staleness-check indeterminate is surfaced', () => { + let tmpDir; + + beforeEach(() => { + tmpDir = createFixture('gsd-3057-b3-phase-'); + }); + + afterEach(() => { + cleanup(tmpDir); + }); + + test( + 'an fs failure inside the staleness check adds a warning; completion routing is unchanged', + { skip: process.platform === 'win32' ? 'symlink creation needs privilege on Windows' : false }, + (t) => { + const phase01Dir = path.join(tmpDir, '.planning', 'phases', '01-foundation'); + const summaryPath = path.join(phase01Dir, '01-01-SUMMARY.md'); + + // Real, on-disk fault instead of an in-process fs.statSync mock: this now + // runs cmdPhaseComplete in a subprocess (via capturePhaseComplete), which + // cannot see a mock installed in this process. findStaleVerificationSummary + // (verification.cjs) calls fs.statSync on each summary file to compare + // mtimes, and statSync follows symlinks — so pointing the summary at a + // target that does not exist reproduces a genuine ENOENT there, degrading + // the staleness check to {determined:false} exactly like the removed + // injected statSync throw did. scanPhasePlans only matches summary + // *filenames* (never stats them), so the plan-coverage gate still sees + // the summary as present. + fs.unlinkSync(summaryPath); + fs.symlinkSync(path.join(phase01Dir, '.does-not-exist'), summaryPath); + + const output = JSON.parse(capturePhaseComplete(t, tmpDir, '1')); + + // Pre-existing no-throw fail-open routing is UNCHANGED: the phase still + // completes exactly as it would have before #3057 B3. + assert.strictEqual(output.completed_phase, '1'); + assert.ok(Array.isArray(output.warnings), 'result must carry a warnings array'); + assert.strictEqual( + output.verification_stale_check_indeterminate, + true, + `result must surface the indeterminate staleness check as a typed field; got ${JSON.stringify(output.warnings)}`, + ); + assert.strictEqual(output.has_warnings, true); + }, + ); + + test('a completed staleness check that finds nothing stale does NOT add an indeterminate warning', (t) => { + const output = JSON.parse(capturePhaseComplete(t, tmpDir, '1')); + + assert.strictEqual(output.completed_phase, '1'); + assert.strictEqual( + output.verification_stale_check_indeterminate, + false, + `must not report an indeterminate check when the staleness check ran to completion; got ${JSON.stringify(output.warnings)}`, + ); + }); + + test( + 'a BLOCKED completion (status=human_needed) with an indeterminate staleness check still blocks, but the error note says so', + { skip: process.platform === 'win32' ? 'symlink creation needs privilege on Windows' : false }, + () => { + const phase02Dir = path.join(tmpDir, '.planning', 'phases', '02-api'); + fs.writeFileSync(path.join(phase02Dir, '02-01-PLAN.md'), '# Plan\nDo the work.\n'); + fs.writeFileSync(path.join(phase02Dir, '02-VERIFICATION.md'), [ + '---', + 'status: human_needed', + '---', + '', + '# Verification', + '', + ].join('\n')); + + // Real, on-disk fault — see the note in the sibling test above. The + // summary is a dangling symlink so fs.statSync (inside + // findStaleVerificationSummary, running in the subprocess) throws ENOENT. + const summaryPath = path.join(phase02Dir, '02-01-SUMMARY.md'); + fs.symlinkSync(path.join(phase02Dir, '.does-not-exist'), summaryPath); + + // Routing is UNCHANGED — status !== 'passed' already blocked before #3057 + // B3; the note is purely additive to the message text. Assert the fact + // structurally (via --json-errors) rather than regexing the human- + // readable note — CONTRIBUTING requires a typed surface alongside any + // text a caller might otherwise only match on, and once that typed + // surface exists the test must assert on IT, not also on the rendered + // prose (src/phase.cts's human message wording is out of scope for this + // test — operators read it, but the test must not lock its exact text). + const result = runGsdTools(['--json-errors', 'phase', 'complete', '2'], tmpDir); + assert.equal(result.success, false, 'phase complete must fail when verification is blocked'); + const errorPayload = JSON.parse(result.error); + assert.equal(errorPayload.reason, 'phase_verification_incomplete'); + assert.equal(errorPayload.verification_stale_check_indeterminate, true); + }, + ); +}); + // ───────────────────────────────────────────────────────────────────────────── // Regressions: phase complete preserves completion date (#1161) // Tests drive the REAL handler (cmdPhaseComplete) via the CLI entry point @@ -8842,7 +8985,7 @@ describe('issue #4 (CJS): cmdPhaseComplete — progress percent clamp', () => { cleanup(tmpDir); }); - test('T2: Progress percent never exceeds 100 after double invocation', () => { + test('T2: Progress percent never exceeds 100 after double invocation', (t) => { tmpDir = createFixture(); // Pre-load STATE.md with Completed Phases: 1, Total Phases: 1 (already 100%) @@ -8873,9 +9016,9 @@ describe('issue #4 (CJS): cmdPhaseComplete — progress percent clamp', () => { fs.writeFileSync(path.join(tmpDir, '.planning', 'ROADMAP.md'), roadmap); // First call - capturePhaseComplete(tmpDir, '1'); + capturePhaseComplete(t, tmpDir, '1'); // Second call — this is the problematic one - capturePhaseComplete(tmpDir, '1'); + capturePhaseComplete(t, tmpDir, '1'); const stateAfterBoth = readStateMd(tmpDir); diff --git a/tests/plan-review-convergence.test.cjs b/tests/plan-review-convergence.test.cjs index 99cdd8e01..8e62e2829 100644 --- a/tests/plan-review-convergence.test.cjs +++ b/tests/plan-review-convergence.test.cjs @@ -73,7 +73,13 @@ function runReviewerFlagsParseBlock(block, args) { return execFileSync('bash', ['-c', script], { env: { ...process.env, ARGUMENTS: args, GSD_TOOLS_PATH }, encoding: 'utf8', - timeout: 5000, + // 30s covers the nested bash → node → gsd-tools.cjs cold-start chain this + // helper spawns. 5s was too tight: on a loaded bench (30k tests running + // in parallel) that budget was consumed by process-spawn scheduling + // latency alone, producing a spurious ETIMEDOUT with no genuine hang. + // 30_000 matches the convention other script-invocation tests in this + // repo already use (see e.g. adr-index-gate.test.cjs, check-env.test.cjs). + timeout: 30_000, }).trim(); } @@ -393,7 +399,10 @@ describe('plan-review-convergence: #2315 respects review.default_reviewers (no-f return execFileSync('bash', ['-c', script], { env: { ...process.env, ARGUMENTS: args, GSD_TEST_DEFAULT_REVIEWERS: defaultReviewers ?? '', GSD_TOOLS_PATH }, encoding: 'utf8', - timeout: 5000, + // 30s covers the same nested bash → node → gsd-tools.cjs cold-start + // chain as runReviewerFlagsParseBlock above; see that helper's + // comment for why 5s flaked under bench load. + timeout: 30_000, }); }; @@ -447,7 +456,10 @@ describe('plan-review-convergence: #2315 respects review.default_reviewers (no-f return execFileSync('bash', ['-c', script], { env: { ...process.env, ARGUMENTS: args, GSD_TEST_DEFAULT_REVIEWERS: defaultReviewers ?? '', GSD_TOOLS_PATH }, encoding: 'utf8', - timeout: 5000, + // 30s covers the same nested bash → node → gsd-tools.cjs cold-start + // chain as runReviewerFlagsParseBlock above; see that helper's + // comment for why 5s flaked under bench load. + timeout: 30_000, }); }; diff --git a/tests/roadmap.test.cjs b/tests/roadmap.test.cjs index 5af46a12a..8b9e60d6f 100644 --- a/tests/roadmap.test.cjs +++ b/tests/roadmap.test.cjs @@ -1117,6 +1117,128 @@ describe('roadmap update-plan-progress command', () => { }); }); +// ───────────────────────────────────────────────────────────────────────────── +// #3057 B3: cmdRoadmapUpdatePlanProgress surfaces an indeterminate staleness check +// +// readVerificationStatus's internal staleness check can fail (fs / +// scanPhasePlans / clock error). Pre-#3057 B3 wiring, that failure was +// dropped here — `verificationPassed` used only `.status`, never +// `.staleCheckIndeterminate` — so this command's JSON output could never +// distinguish "checked; nothing is stale" from "could not check". These +// tests require roadmap.cjs directly (in-process) so an `fs.statSync` fault +// can be injected via the same deterministic path-scoped seam #3057 B4 uses +// for git-base-branch.cts (see tests/git-base-branch.test.cjs). +// ───────────────────────────────────────────────────────────────────────────── + +describe('#3057 B3: roadmap update-plan-progress — verification staleness-check indeterminate is surfaced', () => { + const roadmapMod = require(path.join(__dirname, '..', 'gsd-core', 'bin', 'lib', 'roadmap.cjs')); + let tmpDir; + + beforeEach(() => { + tmpDir = createTempProject('gsd-3057-b3-roadmap-'); + }); + + afterEach(() => { + cleanup(tmpDir); + }); + + /** + * In-process capture of cmdRoadmapUpdatePlanProgress's stdout JSON, stderr + * discarded. + * + * io.cts's `output()` writes via `writeAllSync` → `fs.writeSync(1, ...)` + * directly (bug #1008's non-blocking-pipe fix), NOT `process.stdout.write` + * — so mocking `process.stdout.write` here silently captured nothing and + * every assertion below saw `JSON.parse('')` ("Unexpected end of JSON + * input") regardless of what the command actually produced. The fix is the + * fd-level seam tests/io.test.cjs already established for exactly this + * function (bug #1008's `t.mock.method(fs, 'writeSync', ...)` pattern): + * intercept fd 1, discard fd 2, and pass every OTHER fd through to the + * real writeSync — this command takes the planning lock, which writes its + * own JSON via a separate fd that must not be swallowed as if it were + * stdout (confirmed: without the pass-through, its lock-acquire JSON + * corrupts the captured payload into two concatenated JSON objects). + */ + function captureUpdatePlanProgress(t, cwd, phaseNum) { + const chunks = []; + const origWriteSync = fs.writeSync.bind(fs); + t.mock.method(fs, 'writeSync', (fd, data, offset, length) => { + if (fd === 2) return Buffer.isBuffer(data) ? data.length : String(data).length; + if (fd !== 1) return origWriteSync(fd, data, offset, length); + const chunk = Buffer.isBuffer(data) + ? data.subarray(offset ?? 0, length === undefined ? data.length : (offset ?? 0) + length).toString('utf8') + : String(data); + chunks.push(chunk); + return Buffer.byteLength(chunk, 'utf8'); + }); + roadmapMod.cmdRoadmapUpdatePlanProgress(cwd, phaseNum, false); + const captured = chunks.join(''); + assert.ok(captured.length > 0, 'cmdRoadmapUpdatePlanProgress produced no stdout output'); + return captured; + } + + function seedVerifiedPhase() { + fs.writeFileSync( + path.join(tmpDir, '.planning', 'ROADMAP.md'), + `# Roadmap + +### Phase 1: Test +**Goal:** Test goal +**Plans:** TBD + +## Progress + +| Phase | Milestone | Plans Complete | Status | Completed | +|-------|-----------|----------------|--------|-----------| +| 1. Test | v1.0 | 0/1 | Planned | - | +` + ); + const p1 = path.join(tmpDir, '.planning', 'phases', '01-test'); + fs.mkdirSync(p1, { recursive: true }); + fs.writeFileSync(path.join(p1, '01-01-PLAN.md'), '# Plan 1'); + fs.writeFileSync(path.join(p1, '01-01-SUMMARY.md'), '# Summary 1'); + fs.writeFileSync(path.join(p1, '01-VERIFICATION.md'), '---\nstatus: passed\n---\n\n# Verification\n'); + + const summaryPath = path.join(p1, '01-01-SUMMARY.md'); + const verificationPath = path.join(p1, '01-VERIFICATION.md'); + // Deterministic mtime ordering — never rely on write-order clock ties. + const older = new Date('2026-01-01T00:00:00.000Z'); + const newer = new Date('2026-01-01T00:01:00.000Z'); + fs.utimesSync(summaryPath, older, older); + fs.utimesSync(verificationPath, newer, newer); + return { summaryPath, verificationPath }; + } + + test('an fs failure inside the staleness check sets verification_stale_check_indeterminate:true; routing unchanged', (t) => { + const { summaryPath, verificationPath } = seedVerifiedPhase(); + const origStatSync = fs.statSync; + + t.mock.method(fs, 'statSync', function injectedStaleCheckFault(target, ...args) { + const targetPath = String(target); + if (targetPath === verificationPath || targetPath === summaryPath) { + throw new Error('injected stat failure (#3057 B3)'); + } + return origStatSync.call(fs, target, ...args); + }); + + const output = JSON.parse(captureUpdatePlanProgress(t, tmpDir, '1')); + + // Pre-existing no-throw fail-open routing is UNCHANGED: the phase still + // resolves complete, exactly as it would without the injected fault. + assert.strictEqual(output.complete, true, 'routing unchanged — phase still marked complete'); + assert.strictEqual(output.verification_stale_check_indeterminate, true); + }); + + test('a completed staleness check that finds nothing stale reports verification_stale_check_indeterminate:false', (t) => { + seedVerifiedPhase(); + + const output = JSON.parse(captureUpdatePlanProgress(t, tmpDir, '1')); + + assert.strictEqual(output.complete, true); + assert.strictEqual(output.verification_stale_check_indeterminate, false); + }); +}); + // ───────────────────────────────────────────────────────────────────────────── // phase add command // ───────────────────────────────────────────────────────────────────────────── diff --git a/tests/state-acquirestatelock-non-eexist.test.cjs b/tests/state-acquirestatelock-non-eexist.test.cjs index 545f70d81..51428487f 100644 --- a/tests/state-acquirestatelock-non-eexist.test.cjs +++ b/tests/state-acquirestatelock-non-eexist.test.cjs @@ -1,236 +1,285 @@ -// allow-test-rule: architectural-invariant -// acquireStateLock is a private function (not exported). The behavioral contract — -// throw on non-EEXIST errors rather than returning a false-success lockPath — is -// an implementation invariant that cannot be verified through the public CLI API -// without introducing timing-sensitive mocks. Source inspection is the correct -// and authoritative level for this contract. +'use strict'; /** - * Regression tests for #3772 — acquireStateLock silently returns false-success + * Regression tests for #3772 — acquireStateLock silently returned false-success * on non-EEXIST openSync errors (EMFILE / EINTR / ENOSPC under load). * - * Extended in #3776 to cover Docker overlay-fs and NFS transient errno codes. + * Extended in #3776 to cover Docker overlay-fs and NFS transient errno codes, + * and in #3057 (B2) to cover the steal-decision fault path for an unreadable + * lock body (merged in from tests/state-lock-body-unreadable.test.cjs, which + * this file absorbed — same acquireStateLock surface, see lint-test-file-count). + * + * Every test in this file actually CALLS acquireStateLock (never regexes the + * built .cjs source) and injects errno faults via `withFaultyFs` on the exact + * fs.openSync call the lock-create path makes (src/state.cts, the + * `fs.openSync(lockPath, O_CREAT|O_EXCL|O_WRONLY)` line inside + * acquireStateLock's retry loop) — never chmod/subprocess tricks. * * Contract under test: - * C1. Non-EEXIST error from fs.openSync → must throw, not return lockPath - * C2. Success path (openSync succeeds) → must return lockPath - * C3. EEXIST error → retry / wait semantics unchanged (not impacted by this fix) - * C4. RETRY_ERRNOS set: EAGAIN/EINTR/EINVAL/EIO/ENOENT/ESTALE must be in the retry allowlist - * C5. Fatal codes: EMFILE/ENOSPC/EROFS/EACCES must NOT be in the retry allowlist - * C6. Unknown errno codes must NOT be in the retry allowlist (conservative default) - * C7. EPERM/EBUSY must remain in the retry allowlist (from #3773) + * C1. A fatal non-EEXIST error (EACCES) propagates/throws — not swallowed + * as EEXIST contention and not retried. + * C2. Success path (openSync succeeds) → returns lockPath and writes this + * process's pid into the lock body. + * C4/C7. ACQUIRE_LOCK_RETRY_ERRNOS codes (EAGAIN/EINTR/EINVAL/EIO/ENOENT/ + * ESTALE/EPERM/EBUSY) are retried — the open eventually succeeds and + * exactly one contention-style backoff (clock.sleep) occurs first. + * C5/C6. Fatal codes (EMFILE/ENOSPC/EROFS) and unknown codes propagate + * immediately — zero backoff, the error is thrown on the first attempt. + * (C8 — "uses a Set, not an inline literal" — is an implementation-shape + * assertion with no independent runtime signature; it is subsumed by C4c-f + * above, since a regression to the old inline EPERM||EBUSY check would fail + * those newer-errno retry assertions.) */ -const { test, describe } = require('node:test'); +const { describe, test } = require('node:test'); const assert = require('node:assert/strict'); -const fs = require('fs'); -const path = require('path'); +const fs = require('node:fs'); +const path = require('node:path'); +const os = require('node:os'); -const STATE_CJS_PATH = path.join( - __dirname, '..', 'gsd-core', 'bin', 'lib', 'state.cjs' -); +const { makeFakeClock } = require('./helpers/clock.cjs'); +const { withFaultyFs } = require('./helpers/faulty-deps.cjs'); +const { cleanup } = require('./helpers.cjs'); +const { acquireStateLock, releaseStateLock } = require('../gsd-core/bin/lib/state.cjs'); -// ───────────────────────────────────────────────────────────────────────────── -// Helpers -// ───────────────────────────────────────────────────────────────────────────── +const originalOpenSync = fs.openSync; +const originalReadFileSync = fs.readFileSync; -/** Extract the text of acquireStateLock from the source file. */ -function extractAcquireStateLockSource(src) { - const fnStart = src.indexOf('function acquireStateLock('); - assert.ok(fnStart !== -1, 'acquireStateLock function must exist in state.cjs'); - // Find the closing brace by counting open/close braces from the function start - let depth = 0; - let i = fnStart; - let foundOpen = false; - while (i < src.length) { - if (src[i] === '{') { depth++; foundOpen = true; } - if (src[i] === '}') { depth--; } - if (foundOpen && depth === 0) { return src.slice(fnStart, i + 1); } - i++; - } - throw new Error('Could not find closing brace of acquireStateLock'); +/** Fresh temp project dir with a STATE.md, for a single test. */ +function makeTempState() { + const tmpDir = fs.mkdtempSync(path.join(os.tmpdir(), 'gsd-lock-non-eexist-')); + fs.mkdirSync(path.join(tmpDir, '.planning'), { recursive: true }); + const statePath = path.join(tmpDir, '.planning', 'STATE.md'); + fs.writeFileSync(statePath, '# State\n'); + return { tmpDir, statePath }; +} + +/** Back-date `lockPath`'s mtime by `ageMs` (real fs time, not fake-clock). */ +function backdateMtime(lockPath, ageMs) { + const staled = new Date(Date.now() - ageMs); + fs.utimesSync(lockPath, staled, staled); +} + +/** + * Build a `t`-taking test body that faults fs.openSync for `lockPath` ONCE + * with `code`, then lets the retried open succeed for real — proving the + * errno is retried (not thrown) and exactly one backoff occurs. + */ +function assertOpenSyncErrorIsRetried(code) { + return (t) => { + const { tmpDir, statePath } = makeTempState(); + t.after(() => cleanup(tmpDir)); + const lockPath = statePath + '.lock'; + const clock = makeFakeClock(0); + let calls = 0; + const acquired = withFaultyFs( + { + openSync: (p, ...rest) => { + if (String(p) === lockPath) { + calls++; + if (calls === 1) { + throw Object.assign(new Error(code + ': injected transient error'), { code }); + } + } + return originalOpenSync(p, ...rest); + }, + }, + () => acquireStateLock(statePath, clock), + ); + t.after(() => releaseStateLock(acquired)); + assert.equal( + acquired, lockPath, + code + ' must be retried and the retried open must succeed, not be thrown immediately', + ); + assert.equal( + clock.sleepCalls.length, 1, + code + ' must trigger exactly one contention-style backoff before the retried open succeeds', + ); + }; +} + +/** + * Build a `t`-taking test body that faults fs.openSync for `lockPath` on + * EVERY call with `code` — proving the errno propagates on the first attempt + * with zero backoff (fatal, not retried). + */ +function assertOpenSyncErrorIsFatal(code) { + return (t) => { + const { tmpDir, statePath } = makeTempState(); + t.after(() => cleanup(tmpDir)); + const lockPath = statePath + '.lock'; + const clock = makeFakeClock(0); + assert.throws( + () => withFaultyFs( + { + openSync: (p, ...rest) => { + if (String(p) === lockPath) { + throw Object.assign(new Error(code + ': injected fatal error'), { code }); + } + return originalOpenSync(p, ...rest); + }, + }, + () => acquireStateLock(statePath, clock), + ), + (err) => err.code === code, + code + ' must propagate to the caller rather than being retried or swallowed', + ); + assert.equal( + clock.sleepCalls.length, 0, + code + ' must not trigger a contention/backoff retry before throwing', + ); + }; } // ───────────────────────────────────────────────────────────────────────────── -// C1. Non-EEXIST error → must throw, not return lockPath +// C1. Non-EEXIST fatal error → must throw, not be swallowed as contention // ───────────────────────────────────────────────────────────────────────────── describe('acquireStateLock: non-EEXIST openSync errors (#3772)', () => { - test('C1: source contains throw-not-return for non-EEXIST errors', () => { - const src = fs.readFileSync(STATE_CJS_PATH, 'utf-8'); - const fnSrc = extractAcquireStateLockSource(src); - - // The bug pattern: silently returning the lockPath on non-EEXIST error. - // This branch must NOT appear in the fixed code. - const bugPattern = /if\s*\(\s*err\.code\s*!==\s*['"]EEXIST['"]\s*\)\s*return\s+lockPath/; - assert.ok( - !bugPattern.test(fnSrc), - 'acquireStateLock must NOT return lockPath on non-EEXIST errors (silent false-success — #3772)' - ); - - // The fix: throw the error so callers get the real OS-level failure. - const fixPattern = /if\s*\(\s*err\.code\s*!==\s*['"]EEXIST['"]\s*\)\s*throw\s+err/; - assert.ok( - fixPattern.test(fnSrc), - 'acquireStateLock must throw err on non-EEXIST openSync errors (EMFILE/EINTR/ENOSPC — #3772)' - ); - }); + test( + 'C1: a fatal non-EEXIST error (EACCES) propagates — not swallowed as EEXIST contention', + assertOpenSyncErrorIsFatal('EACCES'), + ); }); // ───────────────────────────────────────────────────────────────────────────── -// C2. Success path → returns lockPath (regression guard — fix must not break success) +// C2. Success path → returns lockPath and writes this process's pid // ───────────────────────────────────────────────────────────────────────────── describe('acquireStateLock: success path still returns lockPath', () => { - test('C2: source contains return lockPath in the success (try) branch', () => { - const src = fs.readFileSync(STATE_CJS_PATH, 'utf-8'); - const fnSrc = extractAcquireStateLockSource(src); + test('C2: openSync succeeding returns the lock path and writes this process pid', (t) => { + const { tmpDir, statePath } = makeTempState(); + t.after(() => cleanup(tmpDir)); - // The success path: openSync succeeds → write PID → close → add to held set → return lockPath. - // Verify the return is still present inside the try block (before the catch). - const tryBlock = fnSrc.slice(fnSrc.indexOf('try {'), fnSrc.indexOf('} catch (')); - assert.ok( - tryBlock.includes('return lockPath'), - 'acquireStateLock must still return lockPath when fs.openSync succeeds (success path intact)' + const acquired = acquireStateLock(statePath); + t.after(() => releaseStateLock(acquired)); + + assert.equal(acquired, statePath + '.lock', 'acquireStateLock must return the lock path on success'); + assert.ok(fs.existsSync(acquired), 'the lock file must exist on disk after a successful acquire'); + assert.equal( + fs.readFileSync(acquired, 'utf8'), String(process.pid), + 'the lock body must contain this process pid on the success path', ); }); }); // ───────────────────────────────────────────────────────────────────────────── -// C4. RETRY_ERRNOS set: new Docker/NFS transient codes must be present (#3776) +// C4 / C7. Transient errno codes are retried (#3776 / #3773 regression guard) // ───────────────────────────────────────────────────────────────────────────── -/** Extract the ACQUIRE_LOCK_RETRY_ERRNOS Set literal from the source file. */ -function extractRetryErrnosSource(src) { - const constStart = src.indexOf('const ACQUIRE_LOCK_RETRY_ERRNOS'); - assert.ok(constStart !== -1, 'ACQUIRE_LOCK_RETRY_ERRNOS constant must exist in state.cjs'); - // Extract to the end of the Set(...) constructor — find the closing ]); - const setEnd = src.indexOf(']);', constStart); - assert.ok(setEnd !== -1, 'ACQUIRE_LOCK_RETRY_ERRNOS Set must have closing ]);'); - return src.slice(constStart, setEnd + 2); -} +describe('acquireStateLock: transient errno codes are retried, not thrown (#3776)', () => { + test('C4a: EAGAIN is retried (resource temporarily unavailable)', assertOpenSyncErrorIsRetried('EAGAIN')); + test('C4b: EINTR is retried (syscall interrupted)', assertOpenSyncErrorIsRetried('EINTR')); + test('C4c: EINVAL is retried (Docker overlay-fs transient)', assertOpenSyncErrorIsRetried('EINVAL')); + test('C4d: EIO is retried (Docker overlay-fs / NFS transient)', assertOpenSyncErrorIsRetried('EIO')); + test('C4e: ENOENT is retried (Docker overlay-fs parent dir transient)', assertOpenSyncErrorIsRetried('ENOENT')); + test('C4f: ESTALE is retried (NFS stale file handle)', assertOpenSyncErrorIsRetried('ESTALE')); +}); -describe('acquireStateLock: RETRY_ERRNOS Set contains expected transient codes (#3776)', () => { - test('C4a: EAGAIN is in ACQUIRE_LOCK_RETRY_ERRNOS', () => { - const src = fs.readFileSync(STATE_CJS_PATH, 'utf-8'); - const setBlock = extractRetryErrnosSource(src); - assert.ok(setBlock.includes("'EAGAIN'"), 'ACQUIRE_LOCK_RETRY_ERRNOS must include EAGAIN (resource temporarily unavailable)'); - }); - - test('C4b: EINTR is in ACQUIRE_LOCK_RETRY_ERRNOS', () => { - const src = fs.readFileSync(STATE_CJS_PATH, 'utf-8'); - const setBlock = extractRetryErrnosSource(src); - assert.ok(setBlock.includes("'EINTR'"), 'ACQUIRE_LOCK_RETRY_ERRNOS must include EINTR (syscall interrupted)'); - }); - - test('C4c: EINVAL is in ACQUIRE_LOCK_RETRY_ERRNOS', () => { - const src = fs.readFileSync(STATE_CJS_PATH, 'utf-8'); - const setBlock = extractRetryErrnosSource(src); - assert.ok(setBlock.includes("'EINVAL'"), 'ACQUIRE_LOCK_RETRY_ERRNOS must include EINVAL (Docker overlay-fs transient)'); - }); - - test('C4d: EIO is in ACQUIRE_LOCK_RETRY_ERRNOS', () => { - const src = fs.readFileSync(STATE_CJS_PATH, 'utf-8'); - const setBlock = extractRetryErrnosSource(src); - assert.ok(setBlock.includes("'EIO'"), 'ACQUIRE_LOCK_RETRY_ERRNOS must include EIO (Docker overlay-fs / NFS transient)'); - }); - - test('C4e: ENOENT is in ACQUIRE_LOCK_RETRY_ERRNOS', () => { - const src = fs.readFileSync(STATE_CJS_PATH, 'utf-8'); - const setBlock = extractRetryErrnosSource(src); - assert.ok(setBlock.includes("'ENOENT'"), 'ACQUIRE_LOCK_RETRY_ERRNOS must include ENOENT (Docker overlay-fs parent dir transient)'); - }); - - test('C4f: ESTALE is in ACQUIRE_LOCK_RETRY_ERRNOS', () => { - const src = fs.readFileSync(STATE_CJS_PATH, 'utf-8'); - const setBlock = extractRetryErrnosSource(src); - assert.ok(setBlock.includes("'ESTALE'"), 'ACQUIRE_LOCK_RETRY_ERRNOS must include ESTALE (NFS stale file handle)'); - }); +describe('acquireStateLock: EPERM/EBUSY still retried (regression guard, #3773)', () => { + test('C7a: EPERM is retried (Windows / macOS AV scanner)', assertOpenSyncErrorIsRetried('EPERM')); + test('C7b: EBUSY is retried (Windows file in use)', assertOpenSyncErrorIsRetried('EBUSY')); }); // ───────────────────────────────────────────────────────────────────────────── -// C5. Fatal codes: EMFILE/ENOSPC/EROFS/EACCES must NOT be in the retry set +// C5 / C6. Fatal and unknown errno codes propagate immediately, never retried // ───────────────────────────────────────────────────────────────────────────── -describe('acquireStateLock: fatal errno codes NOT in RETRY_ERRNOS (#3776)', () => { - test('C5a: EMFILE is NOT in ACQUIRE_LOCK_RETRY_ERRNOS (fd limit exhausted — fatal)', () => { - const src = fs.readFileSync(STATE_CJS_PATH, 'utf-8'); - const setBlock = extractRetryErrnosSource(src); - assert.ok(!setBlock.includes("'EMFILE'"), 'ACQUIRE_LOCK_RETRY_ERRNOS must NOT include EMFILE (fatal: fd limit)'); - }); - - test('C5b: ENOSPC is NOT in ACQUIRE_LOCK_RETRY_ERRNOS (disk full — fatal)', () => { - const src = fs.readFileSync(STATE_CJS_PATH, 'utf-8'); - const setBlock = extractRetryErrnosSource(src); - assert.ok(!setBlock.includes("'ENOSPC'"), 'ACQUIRE_LOCK_RETRY_ERRNOS must NOT include ENOSPC (fatal: disk full)'); - }); - - test('C5c: EROFS is NOT in ACQUIRE_LOCK_RETRY_ERRNOS (read-only fs — fatal)', () => { - const src = fs.readFileSync(STATE_CJS_PATH, 'utf-8'); - const setBlock = extractRetryErrnosSource(src); - assert.ok(!setBlock.includes("'EROFS'"), 'ACQUIRE_LOCK_RETRY_ERRNOS must NOT include EROFS (fatal: read-only fs)'); - }); - - test('C5d: EACCES is NOT in ACQUIRE_LOCK_RETRY_ERRNOS (permission denied — fatal)', () => { - const src = fs.readFileSync(STATE_CJS_PATH, 'utf-8'); - const setBlock = extractRetryErrnosSource(src); - assert.ok(!setBlock.includes("'EACCES'"), 'ACQUIRE_LOCK_RETRY_ERRNOS must NOT include EACCES (fatal: no permission)'); - }); +describe('acquireStateLock: fatal errno codes propagate without retry (#3776)', () => { + test('C5a: EMFILE propagates immediately (fd limit exhausted — fatal)', assertOpenSyncErrorIsFatal('EMFILE')); + test('C5b: ENOSPC propagates immediately (disk full — fatal)', assertOpenSyncErrorIsFatal('ENOSPC')); + test('C5c: EROFS propagates immediately (read-only fs — fatal)', assertOpenSyncErrorIsFatal('EROFS')); + // EACCES is covered by C1 above (the canonical non-EEXIST-fatal case). }); -// ───────────────────────────────────────────────────────────────────────────── -// C6. Unknown errno codes are not in the retry set (conservative default) -// ───────────────────────────────────────────────────────────────────────────── - describe('acquireStateLock: unknown errno codes not retried (conservative default, #3776)', () => { - test('C6: ACQUIRE_LOCK_RETRY_ERRNOS does not include ESOMETHING (unknown code)', () => { - const src = fs.readFileSync(STATE_CJS_PATH, 'utf-8'); - const setBlock = extractRetryErrnosSource(src); - assert.ok( - !setBlock.includes("'ESOMETHING'"), - 'ACQUIRE_LOCK_RETRY_ERRNOS must not include unknown errno ESOMETHING (conservative: surface unknowns)' - ); - }); + test( + 'C6: an unrecognized errno (ESOMETHING) propagates rather than being retried', + assertOpenSyncErrorIsFatal('ESOMETHING'), + ); }); // ───────────────────────────────────────────────────────────────────────────── -// C7. EPERM/EBUSY remain in the retry set (regression guard from #3773) +// #3057 B2 — an unreadable STATE.md lock body must not get the same +// fresh-create-floor stealable treatment as a genuinely empty one. +// +// `_stateLockBodyPid` used to collapse two different situations to the same +// `null`: a lock body that reads back empty/garbage (the create→write +// window — expected, benign) and a lock body that could not be READ at all +// (an I/O fault — permission error, transient NFS/overlay-fs hiccup, etc.). +// Both got the SAME 1-second (`freshCreateFloorMs`) steal-eligibility +// window, so a transient read fault could rob an actively-held lock exactly +// as fast as a lock that is merely mid-creation. +// +// The fix (`_stateLockBodyStatus`, state.cts) makes the steal decision +// four-way: an unreadable body is now held to the SAME conservative +// `deadmanCeilingMs` ceiling as a verified-live holder, not the short +// fresh-create floor. +// +// These two tests are a pair by construction: identical lock age (past the +// fresh-create floor, nowhere near the deadman ceiling), identical clock +// rig — the ONLY variable is whether the body read throws (fault-injected +// via `withFaultyFs`, never chmod/subprocess) or genuinely reads back empty. // ───────────────────────────────────────────────────────────────────────────── -describe('acquireStateLock: EPERM/EBUSY still in RETRY_ERRNOS (regression guard, #3773)', () => { - test('C7a: EPERM is in ACQUIRE_LOCK_RETRY_ERRNOS (Windows / macOS AV scanner)', () => { - const src = fs.readFileSync(STATE_CJS_PATH, 'utf-8'); - const setBlock = extractRetryErrnosSource(src); - assert.ok(setBlock.includes("'EPERM'"), 'ACQUIRE_LOCK_RETRY_ERRNOS must still include EPERM (#3773 regression guard)'); - }); +describe('#3057 B2: acquireStateLock steal decision — unreadable lock body vs. genuinely empty', () => { + test('FAILURE path: an unreadable lock body is NOT stolen at the fresh-create-floor age — the acquire budget is exhausted instead', (t) => { + const { tmpDir, statePath } = makeTempState(); + t.after(() => cleanup(tmpDir)); - test('C7b: EBUSY is in ACQUIRE_LOCK_RETRY_ERRNOS (Windows file in use)', () => { - const src = fs.readFileSync(STATE_CJS_PATH, 'utf-8'); - const setBlock = extractRetryErrnosSource(src); - assert.ok(setBlock.includes("'EBUSY'"), 'ACQUIRE_LOCK_RETRY_ERRNOS must still include EBUSY (#3773 regression guard)'); - }); -}); + const lockPath = statePath + '.lock'; + // Content is irrelevant — the fault-injected read throws before it is ever parsed. + fs.writeFileSync(lockPath, '12345'); + t.after(() => { try { fs.unlinkSync(lockPath); } catch { /* already gone */ } }); -// ───────────────────────────────────────────────────────────────────────────── -// C8. acquireStateLock uses Set-based retry check (not inline literal) -// ───────────────────────────────────────────────────────────────────────────── + // Age the lock past freshCreateFloorMs (1000ms) but nowhere near + // deadmanCeilingMs (60000ms) — this is EXACTLY the age at which a + // genuinely-empty body would already be stolen (see the paired test below). + backdateMtime(lockPath, 5000); -describe('acquireStateLock: uses Set-based retry check (grep-able, not inline literal)', () => { - test('C8: retry check uses ACQUIRE_LOCK_RETRY_ERRNOS.has() not hardcoded comparisons', () => { - const src = fs.readFileSync(STATE_CJS_PATH, 'utf-8'); - const fnSrc = extractAcquireStateLockSource(src); + const baseClock = makeFakeClock(Date.now() + 100); // ageMs ≈ 5100ms at start + // Jump the virtual clock past the 30 000ms acquire budget on the very + // first retry sleep, so the test proves "never stolen within budget" + // deterministically without hundreds of synchronous retry iterations. + const fastClock = { + now: baseClock.now.bind(baseClock), + sleep(ms) { + baseClock.sleep(ms); + baseClock.advance(31000); + }, + }; - // Must use the named Set for the check inside the function - assert.ok( - fnSrc.includes('ACQUIRE_LOCK_RETRY_ERRNOS.has('), - 'acquireStateLock catch block must use ACQUIRE_LOCK_RETRY_ERRNOS.has() for retry decision' - ); - - // Must NOT have the old inline EPERM/EBUSY literal check - const oldPattern = /err\.code\s*===\s*['"]EPERM['"]\s*\|\|\s*err\.code\s*===\s*['"]EBUSY['"]/; - assert.ok( - !oldPattern.test(fnSrc), - 'acquireStateLock must not use old inline EPERM||EBUSY check (should use ACQUIRE_LOCK_RETRY_ERRNOS.has())' + assert.throws( + () => withFaultyFs( + { + readFileSync: (p, ...rest) => { + if (String(p) === lockPath) { + throw Object.assign(new Error('EIO: i/o error, read'), { code: 'EIO' }); + } + return originalReadFileSync(p, ...rest); + }, + }, + () => acquireStateLock(statePath, fastClock), + ), + /acquireStateLock.*exceeded.*30000ms budget/, + 'an unreadable lock body past the fresh-create-floor age must NOT be stolen — it must hit the acquire-budget timeout', ); }); + + test('BENIGN path: a genuinely empty lock body at the SAME age IS stolen (fresh-create-floor path unaffected by the fix)', (t) => { + const { tmpDir, statePath } = makeTempState(); + t.after(() => cleanup(tmpDir)); + + const lockPath = statePath + '.lock'; + fs.writeFileSync(lockPath, ''); // genuinely empty — mid-creation window, not an I/O fault + + backdateMtime(lockPath, 5000); // identical age to the FAILURE test above + + const clock = makeFakeClock(Date.now() + 100); // ageMs ≈ 5100ms, identical rig to the FAILURE test above + + const acquired = acquireStateLock(statePath, clock); + t.after(() => releaseStateLock(acquired)); + assert.ok(fs.existsSync(acquired), + 'a genuinely empty lock body past the fresh-create-floor age must still be stolen and re-acquired'); + }); }); diff --git a/tests/state-rebuild-cli.test.cjs b/tests/state-rebuild-cli.test.cjs index c741b0062..4d72f4a15 100644 --- a/tests/state-rebuild-cli.test.cjs +++ b/tests/state-rebuild-cli.test.cjs @@ -17,6 +17,11 @@ const { cleanup, runGsdTools, } = require('./helpers.cjs'); +const { withFaultyFs } = require('./helpers/faulty-deps.cjs'); +const stateMod = require('../gsd-core/bin/lib/state.cjs'); +const { stateExtractField } = require('../gsd-core/bin/lib/state-document.cjs'); +const { parseMarkdownTable } = require('../gsd-core/bin/lib/markdown-table.cjs'); +const { collectSection } = require('../gsd-core/bin/lib/markdown-sectionizer.cjs'); const TOOLS_PATH = path.join(__dirname, '..', 'gsd-core', 'bin', 'gsd-tools.cjs'); @@ -113,6 +118,124 @@ function readLiveState(cwd) { return content.replace(/^## Rebuild Log[\s\S]*$/m, ''); } +/** + * A project whose ONLY drift is the `**By Phase:**` table (an orphan row for + * phase 99, which isn't on disk). Every other body/frontmatter field is + * already canonical, so `rebuildCore`'s other reconciliation steps are all + * no-ops — the ONLY thing that can add a `## Rebuild Log` entry / flip + * `mutated` is whether the phase-inventory disk scan actually reconciles the + * table. This isolates the phase-inventory-scan outcome for #3057 B1. + */ +function projectWithOnlyPhaseTableDrift() { + const cwd = createTempProject('state-rebuild-cli-scan'); + const planningPath = path.join(cwd, '.planning'); + fs.mkdirSync(path.join(planningPath, 'phases', '01-phase-one'), { recursive: true }); + fs.writeFileSync(path.join(planningPath, 'phases', '01-phase-one', '01-PLAN.md'), '# Plan'); + + const stateContent = [ + '---', + 'gsd_state_version: \'1.0\'', + 'status: executing', + 'milestone: 1.0.0', + 'milestone_name: Test', + 'current_phase: 1', + 'current_phase_name: Phase One', + 'current_plan: 1', + 'progress:', + ' total_phases: 1', + ' completed_phases: 0', + ' total_plans: 1', + ' completed_plans: 0', + ' percent: 0', + '---', + '', + '# Project State', + '', + '## Project Reference', + '', + '**Core value:** A test project', + '**Current focus:** Phase One', + '', + '## Current Position', + '', + '**Current Phase:** 1', + '**Current Phase Name:** Phase One', + '**Current Plan:** 1', + '**Total Plans in Phase:** 1', + '**Status:** executing', + '**Last Activity:** 2026-06-29', + '**Last Activity Description:** mid-flight', + '', + 'Phase: 1 of 1 (Phase One)', + 'Plan: 1 of 1', + 'Status: Executing Phase 1', + 'Last activity: 2026-06-29 — mid-flight', + '', + '**Progress:** [░░░░░░░░░░] 0%', + '', + '## Performance Metrics', + '', + '**By Phase:**', + '', + '| Phase | Plans | Total | Avg/Plan |', + '|-------|-------|-------|----------|', + '| 1 | 1 | - | - |', + '| 99 | 1 | - | - |', + '', + '## Accumulated Context', + '', + '### Decisions', + '', + 'None yet.', + '', + '## Session Continuity', + '', + 'Last session: 2026-06-29 12:00', + 'Stopped at: mid-flight', + 'Resume file: None', + '', + ].join('\n'); + fs.writeFileSync(path.join(planningPath, 'STATE.md'), stateContent); + return cwd; +} + +/** + * Typed presence check for the `## Rebuild Log` audit-log section, shared by + * both call sites that need it (CONTRIBUTING: no raw-text `.includes()` on + * produced STATE.md text). Built on the existing `collectSection` seam + * (markdown-sectionizer.cjs) — a `Section | null`, not string matching. + */ +function hasRebuildLogSection(content) { + return collectSection(content, (h) => h.text.trim() === 'Rebuild Log') !== null; +} + +/** + * Run `fn` while capturing every fd-1 write `cmdStateRebuild`'s `output()` + * performs (it writes via a raw `fs.writeSync(1, ...)`, never + * `console.log`/`process.stdout.write` — same seam `tests/io.test.cjs` + * exercises for bug #1008). Standalone helper with no test context, so the + * try/finally restore is CONTRIBUTING-compliant (same shape as + * `withFaultyFs`). + */ +function captureStdout(fn) { + const chunks = []; + const original = fs.writeSync; + fs.writeSync = (fd, data, offset, length) => { + if (fd !== 1) return original(fd, data, offset, length); + const chunk = Buffer.isBuffer(data) + ? data.subarray(offset ?? 0, length === undefined ? data.length : (offset ?? 0) + length).toString('utf8') + : String(data); + chunks.push(chunk); + return Buffer.byteLength(chunk, 'utf8'); + }; + try { + fn(); + } finally { + fs.writeSync = original; + } + return chunks.join(''); +} + // --------------------------------------------------------------------------- // Tests // --------------------------------------------------------------------------- @@ -127,18 +250,21 @@ describe('ADR-1817 Phase 2: `state rebuild` CLI subcommand dispatch (criterion # // Body fields reconciled with frontmatter. const live = readLiveState(cwd); - assert.ok(live.includes('**Current Phase:** 2'), + assert.strictEqual(stateExtractField(live, 'Current Phase'), '2', 'body Current Phase must be reconciled to frontmatter value 2'); - assert.ok(live.includes('**Current Phase Name:** Phase Two'), + assert.strictEqual(stateExtractField(live, 'Current Phase Name'), 'Phase Two', 'body Current Phase Name must be reconciled to frontmatter value'); // Orphan table row dropped (phase 99 is not on disk). - assert.ok(!live.includes('| 99 |'), - 'orphan row for phase 99 must be dropped (phaseInventoryProvider wired to disk scan)'); + const byPhaseTable = parseMarkdownTable(live); + assert.ok(byPhaseTable.ok, `By Phase table must parse; reason: ${byPhaseTable.ok ? '' : byPhaseTable.reason}`); + const phaseIds = byPhaseTable.value.rows.map((r) => r.Phase); + assert.ok(!phaseIds.includes('99'), + `orphan row for phase 99 must be dropped (phaseInventoryProvider wired to disk scan); rows: ${JSON.stringify(phaseIds)}`); // Audit log appended. const fullState = fs.readFileSync(path.join(cwd, '.planning', 'STATE.md'), 'utf8'); - assert.ok(fullState.includes('## Rebuild Log'), + assert.ok(hasRebuildLogSection(fullState), 'audit log section must be appended'); }); @@ -155,9 +281,12 @@ describe('ADR-1817 Phase 2: `state rebuild` CLI subcommand dispatch (criterion # '--dry-run must NOT modify STATE.md on disk'); // The structured output should signal mutations would occur (the fixture - // has drift, so mutated=true in dry-run preview). - assert.ok(result.output.includes('mutated'), - `dry-run output should report mutated flag; output: ${result.output}`); + // has drift, so mutated=true in dry-run preview). `state rebuild --dry-run` + // emits pure JSON to stdout (src/state.cts:3221 `cmdStateRebuild`'s dry-run + // branch: `emit({ ..., mutated, ... })`). + const parsed = JSON.parse(result.output); + assert.strictEqual(parsed.mutated, true, + `dry-run output should report mutated:true; output: ${result.output}`); }); test('`state rebuild --verbose` emits audit-log entries to stderr', (t) => { @@ -177,10 +306,14 @@ describe('ADR-1817 Phase 2: `state rebuild` CLI subcommand dispatch (criterion # // tees the same entries to stderr. The functional guarantee (audit log // written) is what matters; the stderr tee is a convenience. const after = fs.readFileSync(path.join(cwd, '.planning', 'STATE.md'), 'utf8'); - assert.ok(after.includes('## Rebuild Log'), + assert.ok(hasRebuildLogSection(after), '--verbose must still produce the audit log section in STATE.md'); - assert.ok(stdout.includes('rebuilt'), - `--verbose stdout must include the rebuild result; got: ${stdout.slice(0, 200)}`); + // The real (non-dry-run) path emits `{ rebuilt: capturedMutated, ... }` + // to stdout as pure JSON (src/state.cts:3254 `cmdStateRebuild`). The + // fixture has drift, so `rebuilt` must be true. + const parsedStdout = JSON.parse(stdout); + assert.strictEqual(parsedStdout.rebuilt, true, + `--verbose stdout must report rebuilt:true; got: ${stdout.slice(0, 200)}`); }); test('`state rebuild` on a clean STATE.md is a no-op (idempotency, end-to-end)', (t) => { @@ -207,11 +340,81 @@ describe('ADR-1817 Phase 2: `state rebuild` CLI subcommand dispatch (criterion # // No STATE.md written. const result = runGsdTools('state rebuild', cwd); - // The command emits an error result but does not crash the process. - const combined = `${result.output}\n${result.error || ''}`; - assert.ok(combined.includes('STATE.md not found'), - 'missing STATE.md should produce a clean "STATE.md not found" message'); - assert.ok(!combined.includes('at Object.'), - 'no raw stack trace should leak into the output (CONTRIBUTING QA Matrix)'); + // The command emits a typed JSON error result and exits 0 — it does not + // crash the process (src/state.cts:3136 `cmdStateRebuild`'s missing-file + // guard: `emit({ error: 'STATE.md not found' }, raw); return;`, no + // `process.exit`). `result.success` proves the clean exit (a crash would + // flip it to false and populate `result.error` with a raw stack trace + // instead); `JSON.parse` proves stdout is exactly the structured payload + // with nothing else — including no leaked stack-trace text — mixed in. + assert.ok(result.success, + `state rebuild on a missing STATE.md should exit cleanly (no crash); stderr: ${result.error || ''}`); + const parsed = JSON.parse(result.output); + assert.strictEqual(parsed.error, 'STATE.md not found', + `missing STATE.md should produce a typed error field; output: ${result.output}`); + }); +}); + +// --------------------------------------------------------------------------- +// #3057 B1: the PRODUCTION `phaseInventoryProvider` closure (state.cts:3106), +// not just the pure `reconcileByPhaseTable` core, must distinguish a real +// disk-scan failure from a genuinely-empty/reconciled scan. In-process fault +// injection via `withFaultyFs` on the real `fs.readdirSync` the closure +// calls — never chmod, never the subprocess seam (both banned per +// tests/helpers/faulty-deps.cjs's module doc). +// --------------------------------------------------------------------------- + +describe('#3057 B1: cmdStateRebuild (production adapter) surfaces a real phase-inventory scan failure', () => { + test('FAILURE path: readdirSync(.planning/phases) faulted → phase_inventory_scan_failed:true, table left untouched, mutated:false', (t) => { + const cwd = projectWithOnlyPhaseTableDrift(); + t.after(() => cleanup(cwd)); + const phasesDir = path.join(cwd, '.planning', 'phases'); + const originalReaddirSync = fs.readdirSync; + + const stdout = withFaultyFs({ + readdirSync: (p, ...rest) => { + if (String(p) === phasesDir) { + throw Object.assign(new Error('EACCES: permission denied, scandir ' + phasesDir), { code: 'EACCES' }); + } + return originalReaddirSync(p, ...rest); + }, + }, () => captureStdout(() => { + stateMod.cmdStateRebuild(cwd, { dryRun: true }, false); + })); + + const parsed = JSON.parse(stdout); + assert.strictEqual(parsed.phase_inventory_scan_failed, true, + `a faulted disk scan must report phase_inventory_scan_failed:true; got: ${stdout}`); + assert.strictEqual(parsed.mutated, false, + 'nothing else drifted in this fixture, so mutated must stay false — the failure is carried ONLY by the dedicated field'); + assert.strictEqual(parsed.phase_inventory_scan_reason, + 'EACCES: permission denied, scandir ' + phasesDir, + `phase_inventory_scan_reason must carry the exact fault message from the injected readdirSync throw; got: ${JSON.stringify(parsed.phase_inventory_scan_reason)}`); + + // The orphan row must survive untouched — a failed scan is not a + // trustworthy inventory to reconcile the table against. + const live = readLiveState(cwd); + const byPhaseTable = parseMarkdownTable(live); + assert.ok(byPhaseTable.ok, `By Phase table must parse; reason: ${byPhaseTable.ok ? '' : byPhaseTable.reason}`); + const phaseIds = byPhaseTable.value.rows.map((r) => r.Phase); + assert.ok(phaseIds.includes('99'), + `orphan row for phase 99 must be preserved when the scan failed; rows: ${JSON.stringify(phaseIds)}`); + }); + + test('BENIGN path: the same fixture with an unfaulted disk scan reconciles the table and reports no failure', (t) => { + const cwd = projectWithOnlyPhaseTableDrift(); + t.after(() => cleanup(cwd)); + + const stdout = captureStdout(() => { + stateMod.cmdStateRebuild(cwd, { dryRun: true }, false); + }); + + const parsed = JSON.parse(stdout); + assert.strictEqual(parsed.phase_inventory_scan_failed, false, + `an unfaulted disk scan must report phase_inventory_scan_failed:false; got: ${stdout}`); + assert.strictEqual(parsed.mutated, true, + 'the real disk scan finds phase 1 only, so the orphan row 99 is real drift the (dry-run) rebuild would fix'); + assert.strictEqual(parsed.phase_inventory_scan_reason, undefined, + 'a clean scan must leave phase_inventory_scan_reason absent, distinguishing it from a faulted scan'); }); }); diff --git a/tests/state-rebuild.test.cjs b/tests/state-rebuild.test.cjs index d9ebb7b68..03a5e6c1f 100644 --- a/tests/state-rebuild.test.cjs +++ b/tests/state-rebuild.test.cjs @@ -22,6 +22,7 @@ const { transitionCore, } = require('../gsd-core/bin/lib/state-transition.cjs'); const { stateExtractField } = require('../gsd-core/bin/lib/state-document.cjs'); +const { parseMarkdownTable } = require('../gsd-core/bin/lib/markdown-table.cjs'); const fixedClock = Object.freeze({ today: () => '2026-06-29', @@ -30,7 +31,12 @@ const fixedClock = Object.freeze({ }); const noProgress = () => null; -const noPhases = () => null; +// #3057 B1: `phaseInventoryProvider` returns a discriminated result, never a +// bare array-or-null — `{ ok: true, phases: [] }` is the genuinely-empty +// benign case ("nothing to reconcile"), distinct from `{ ok: false, reason }` +// (a scan that could not complete). See tests/helpers/faulty-deps.cjs and the +// dedicated describe block below for the failure-path coverage. +const noPhases = () => ({ ok: true, phases: [] }); const baseDeps = Object.freeze({ progressProvider: noProgress, @@ -362,11 +368,14 @@ describe('ADR-1817 §2: rebuild reconciles **By Phase:** table via phaseInventor ); const deps = { ...baseDeps, - phaseInventoryProvider: () => [ - { number: '1', name: 'Phase 1', planCount: 2, summaryCount: 2 }, - { number: '2', name: 'Phase 2', planCount: 3, summaryCount: 3 }, - { number: '3', name: 'Test Phase', planCount: 5, summaryCount: 4 }, - ], + phaseInventoryProvider: () => ({ + ok: true, + phases: [ + { number: '1', name: 'Phase 1', planCount: 2, summaryCount: 2 }, + { number: '2', name: 'Phase 2', planCount: 3, summaryCount: 3 }, + { number: '3', name: 'Test Phase', planCount: 5, summaryCount: 4 }, + ], + }), }; // First call with no phaseInventoryProvider → no-op (covered by its own test below). // Re-run with the provider-wired deps: @@ -389,11 +398,14 @@ describe('ADR-1817 §2: rebuild reconciles **By Phase:** table via phaseInventor ); const deps = { ...baseDeps, - phaseInventoryProvider: () => [ - { number: '1', name: 'Phase 1', planCount: 2, summaryCount: 2 }, - { number: '2', name: 'Phase 2', planCount: 3, summaryCount: 3 }, - { number: '3', name: 'Test Phase', planCount: 5, summaryCount: 4 }, - ], + phaseInventoryProvider: () => ({ + ok: true, + phases: [ + { number: '1', name: 'Phase 1', planCount: 2, summaryCount: 2 }, + { number: '2', name: 'Phase 2', planCount: 3, summaryCount: 3 }, + { number: '3', name: 'Test Phase', planCount: 5, summaryCount: 4 }, + ], + }), }; const result = transitionCore(drifted, { kind: 'rebuild' }, deps); assert.ok(result.content.includes('kind: by-phase-table-reconciled'), @@ -405,7 +417,7 @@ describe('ADR-1817 §2: rebuild reconciles **By Phase:** table via phaseInventor /\| 3 \| 5 \| - \| - \|\n/, '| 3 | 5 | - | - |\n| 99 | 1 | - | - |\n', ); - // baseDeps.phaseInventoryProvider = noPhases (returns null) → step is no-op. + // baseDeps.phaseInventoryProvider = noPhases ({ ok: true, phases: [] }) → step is no-op. const result = transitionCore(drifted, { kind: 'rebuild' }, baseDeps); assert.ok(result.content.includes('| 99 |'), 'orphan row must be preserved when no canonical source is wired'); @@ -414,6 +426,60 @@ describe('ADR-1817 §2: rebuild reconciles **By Phase:** table via phaseInventor }); }); +// --------------------------------------------------------------------------- +// Tests — #3057 B1: a phase-inventory scan FAILURE must be distinguishable +// from a genuinely-empty scan. Before the fix both were `null`, so a real +// disk-scan failure and "no phases on disk" produced the identical result: +// `state rebuild` could report success while by-phase-table reconciliation +// silently never ran. +// --------------------------------------------------------------------------- + +describe('#3057 B1: phaseInventoryProvider scan-failure is distinguishable from genuinely-empty', () => { + const drifted = () => cleanState().replace( + /\| 3 \| 5 \| - \| - \|\n/, + '| 3 | 5 | - | - |\n| 99 | 1 | - | - |\n', + ); + + test('FAILURE path: ok:false surfaces phase_inventory_scan_failed and leaves the table untouched', () => { + const deps = { + ...baseDeps, + phaseInventoryProvider: () => ({ ok: false, reason: 'EACCES: permission denied, readdir .planning/phases' }), + }; + const result = transitionCore(drifted(), { kind: 'rebuild' }, deps); + assert.strictEqual(result.data.phase_inventory_scan_failed, true, + 'a scan failure must set phase_inventory_scan_failed:true on the result data'); + assert.strictEqual(result.data.phase_inventory_scan_reason, + 'EACCES: permission denied, readdir .planning/phases', + 'the failure reason must be threaded through to the caller'); + const table = parseMarkdownTable(result.content); + assert.ok(table.ok, `By Phase table must parse; reason: ${table.ok ? '' : table.reason}`); + const phaseIds = table.value.rows.map((r) => r.Phase); + assert.ok(phaseIds.includes('99'), + 'orphan row must be preserved — a failed scan is not a trustworthy inventory to reconcile against'); + assert.ok(!result.data.log.some((e) => e.kind === 'by-phase-table-reconciled'), + 'a failed scan must not log a by-phase-table-reconciled entry (nothing was actually reconciled)'); + }); + + test('BENIGN path: ok:true with an empty phases array reports no failure and no reconciliation', () => { + const deps = { + ...baseDeps, + phaseInventoryProvider: () => ({ ok: true, phases: [] }), + }; + const result = transitionCore(drifted(), { kind: 'rebuild' }, deps); + assert.strictEqual(result.data.phase_inventory_scan_failed, false, + 'a genuinely-empty successful scan must report phase_inventory_scan_failed:false'); + assert.strictEqual(result.data.phase_inventory_scan_reason, undefined, + 'no reason field when the scan did not fail'); + const table = parseMarkdownTable(result.content); + assert.ok(table.ok, `By Phase table must parse; reason: ${table.ok ? '' : table.reason}`); + const phaseIds = table.value.rows.map((r) => r.Phase); + assert.ok(phaseIds.includes('99'), + 'orphan row is preserved (same visible outcome as the failure case — the DATA field is what distinguishes them)'); + assert.ok(!result.data.log.some((e) => e.kind === 'by-phase-table-reconciled'), + 'an empty inventory logs no reconciliation entry, same as the failure case'); + }); +}); + // --------------------------------------------------------------------------- // Tests — §5 + §6: regression guard (sync/prune unchanged by rebuild presence) // --------------------------------------------------------------------------- diff --git a/tests/state-transition.test.cjs b/tests/state-transition.test.cjs index f9ac59369..6b7409a2d 100644 --- a/tests/state-transition.test.cjs +++ b/tests/state-transition.test.cjs @@ -676,6 +676,43 @@ describe('ADR-1769 Phase 3: completePhase progress derivation (roadmap)', () => assert.strictEqual(stateExtractField(result.content, 'Completed Phases'), '2'); assert.strictEqual(stateExtractField(result.content, 'Progress'), '40%'); }); + + // #3057 B9: `result.updated` must let a caller tell "recomputed from the + // roadmap" apart from "roadmap unavailable, left as-is". Before the fix, + // `stateReplaceField`'s return was truthy whenever the field pattern + // matched — regardless of whether the substituted text actually differed + // from `body` — so 'Completed Phases' (and 'Progress') were marked + // 'updated' even when nothing changed. + + test('FAILURE path (roadmap unavailable): Completed Phases / Progress are NOT marked updated — left-as-is is distinguishable from recomputed', () => { + const nullDeps = { clock: fixedClock, progressProvider: noProgress, roadmapProvider: () => null }; + const result = transitionCore( + completePhaseBody(), + { kind: 'completePhase', phaseNum: '3', nextPhaseNum: '4', nextPhaseName: null, isLastPhase: false, planCount: 3, summaryCount: 3 }, + nullDeps, + ); + assert.ok(!result.updated.includes('Completed Phases'), + `left-as-is 'Completed Phases' must NOT appear in updated; got ${JSON.stringify(result.updated)}`); + assert.ok(!result.updated.includes('Progress'), + `left-as-is 'Progress' must NOT appear in updated; got ${JSON.stringify(result.updated)}`); + // Values are unchanged (the benign-preservation contract from the test above). + assert.strictEqual(stateExtractField(result.content, 'Completed Phases'), '2'); + assert.strictEqual(stateExtractField(result.content, 'Progress'), '40%'); + }); + + test('BENIGN path (roadmap recomputes a different value): Completed Phases / Progress ARE marked updated — recomputed is distinguishable from left-as-is', () => { + const result = transitionCore( + completePhaseBody(), + { kind: 'completePhase', phaseNum: '3', nextPhaseNum: '4', nextPhaseName: null, isLastPhase: false, planCount: 3, summaryCount: 3 }, + deps, // roadmapProvider => ROADMAP_3_OF_5, which recomputes Completed Phases 2 → 3 + ); + assert.ok(result.updated.includes('Completed Phases'), + `recomputed 'Completed Phases' must appear in updated; got ${JSON.stringify(result.updated)}`); + assert.ok(result.updated.includes('Progress'), + `recomputed 'Progress' must appear in updated; got ${JSON.stringify(result.updated)}`); + assert.strictEqual(stateExtractField(result.content, 'Completed Phases'), '3'); + assert.strictEqual(stateExtractField(result.content, 'Progress'), '60%'); + }); }); describe('ADR-1769 Phase 3: completePhase edge cases', () => { diff --git a/tests/uat-predicate.test.cjs b/tests/uat-predicate.test.cjs index 6a96edf89..799840953 100644 --- a/tests/uat-predicate.test.cjs +++ b/tests/uat-predicate.test.cjs @@ -598,6 +598,80 @@ describe('evaluateUatPassed — policy.requireVerification', () => { }); }); +// ─── evaluateUatPassed — #3057 B3: staleness-check indeterminate is surfaced ── +// +// readVerificationStatus's internal staleness check can fail (fs / +// scanPhasePlans / clock error). Pre-#3057 B3 wiring, the requireVerification +// policy check used only `.status`, dropping `.staleCheckIndeterminate` on +// the floor — so cmdPhaseUatPassed's JSON output (which spreads this whole +// report) could never distinguish "checked; nothing is stale" from "could +// not check". `verification_stale_check_indeterminate` must never itself gate +// `passed`/`blockers` — only readVerificationStatus's `.status` may. + +describe('#3057 B3: evaluateUatPassed — verification staleness-check indeterminate is surfaced', () => { + let tmpDir; + + beforeEach(() => { + tmpDir = makeTmpDir(); + }); + + afterEach(() => { + rmDir(tmpDir); + }); + + function seedVerifiedPhase() { + writeFile(tmpDir, 'phase-UAT.md', makePassingUat(1)); + writeFile(tmpDir, 'phase-VERIFICATION.md', '---\nstatus: passed\n---\n\nOK.'); + writeFile(tmpDir, 'phase-SUMMARY.md', '# Summary'); + const verificationPath = path.join(tmpDir, 'phase-VERIFICATION.md'); + const summaryPath = path.join(tmpDir, 'phase-SUMMARY.md'); + // Deterministic mtime ordering — never rely on write-order clock ties. + const older = new Date('2026-01-01T00:00:00.000Z'); + const newer = new Date('2026-01-01T00:01:00.000Z'); + fs.utimesSync(summaryPath, older, older); + fs.utimesSync(verificationPath, newer, newer); + return { summaryPath, verificationPath }; + } + + test('an fs failure inside the staleness check sets verification_stale_check_indeterminate:true; passed/blockers unchanged', (t) => { + const { summaryPath, verificationPath } = seedVerifiedPhase(); + const origStatSync = fs.statSync; + + t.mock.method(fs, 'statSync', function injectedStaleCheckFault(target, ...args) { + const targetPath = String(target); + if (targetPath === verificationPath || targetPath === summaryPath) { + throw new Error('injected stat failure (#3057 B3)'); + } + return origStatSync.call(fs, target, ...args); + }); + + const report = evaluateUatPassed(tmpDir, { policy: { requireVerification: true } }); + + // Pre-existing no-throw fail-open routing is UNCHANGED — `passed`/ + // `blockers` are exactly what they would be without the injected fault. + assert.strictEqual(report.passed, true); + assert.deepStrictEqual(report.blockers, []); + assert.strictEqual(report.verification_stale_check_indeterminate, true); + }); + + test('a completed staleness check that finds nothing stale reports verification_stale_check_indeterminate:false', () => { + seedVerifiedPhase(); + + const report = evaluateUatPassed(tmpDir, { policy: { requireVerification: true } }); + + assert.strictEqual(report.passed, true); + assert.strictEqual(report.verification_stale_check_indeterminate, false); + }); + + test('requireVerification not set → verification_stale_check_indeterminate is always false (readVerificationStatus never reached)', () => { + seedVerifiedPhase(); + + const report = evaluateUatPassed(tmpDir, { policy: { requireVerification: false } }); + + assert.strictEqual(report.verification_stale_check_indeterminate, false); + }); +}); + // ─── evaluateUatPassed — malformed markdown guard ───────────────────────────── describe('evaluateUatPassed — malformed markdown blocker', () => { diff --git a/tests/verification-status.test.cjs b/tests/verification-status.test.cjs index f74de9521..aa16604cb 100644 --- a/tests/verification-status.test.cjs +++ b/tests/verification-status.test.cjs @@ -802,6 +802,83 @@ describe('verification-status', () => { }); +// ─── #3057 B3: findStaleVerificationSummary — indeterminate vs not-stale ───── +// +// The pre-fix catch-all returned `null` on ANY fs / scanPhasePlans / clock +// failure — identical to a completed check that genuinely found nothing +// stale. `opts.fs` had never been exercised by any test. These two tests +// confirm (a) the `opts.fs` injection seam actually works, and (b) the two +// outcomes are now distinguishable via `staleCheckIndeterminate` on the +// `readVerificationStatus` result. + +describe('#3057 B3: staleness check — indeterminate is distinguishable from not-stale', () => { + test('an fs failure inside the staleness check yields staleCheckIndeterminate:true, not a silent "not stale"', (t) => { + const baseDir = fs.mkdtempSync(path.join(os.tmpdir(), 'gsd-3057-b3-fault-')); + t.after(() => cleanup(baseDir)); + const dir = path.join(baseDir, '01-stale-check-fault'); + fs.mkdirSync(dir); + + const verificationPath = path.join(dir, '01-VERIFICATION.md'); + const summaryPath = path.join(dir, '01-01-SUMMARY.md'); + writeVerificationMd(dir, '01-VERIFICATION.md', 'passed'); + fs.writeFileSync(summaryPath, '# Summary'); + // The summary IS newer — if the check ran to completion it would find + // 'stale'. The point of this test is that it never gets to find out. + setMtime(verificationPath, '2026-01-01T00:00:00.000Z'); + setMtime(summaryPath, '2026-01-01T00:01:00.000Z'); + + // Confirms opts.fs is actually threaded through: readdirSync/readFileSync + // delegate to the real fs (so "find the VERIFICATION.md" / "read its + // frontmatter" upstream of the staleness check still succeed normally), + // and ONLY statSync is faulted — driving findStaleVerificationSummary's + // catch branch specifically, via the injected seam, not a global monkeypatch. + const fsLike = { + readdirSync: (d) => fs.readdirSync(d), + readFileSync: (p, enc) => fs.readFileSync(p, enc), + statSync: () => { throw new Error('injected stat failure (#3057 B3)'); }, + }; + + const result = readVerificationStatus(dir, { + fs: fsLike, + phaseCleanCommitTimesMs: () => new Map(), + }); + + // Pre-existing no-throw fail-open contract is UNCHANGED: routing still + // proceeds as if nothing were stale (status stays 'passed', not 'stale' — + // a genuinely-stale summary sits right there and would have tripped the + // 'stale' route had the check run to completion). + assert.equal(result.status, 'passed'); + // But the cause is no longer silently identical to a completed "nothing + // is stale" check — this MUST be flagged as indeterminate. + assert.strictEqual(result.staleCheckIndeterminate, true); + }); + + test('a completed staleness check that finds nothing stale never reports indeterminate', (t) => { + const baseDir = fs.mkdtempSync(path.join(os.tmpdir(), 'gsd-3057-b3-ok-')); + t.after(() => cleanup(baseDir)); + const dir = path.join(baseDir, '01-stale-check-ok'); + fs.mkdirSync(dir); + + const verificationPath = path.join(dir, '01-VERIFICATION.md'); + const summaryPath = path.join(dir, '01-01-SUMMARY.md'); + writeVerificationMd(dir, '01-VERIFICATION.md', 'passed'); + fs.writeFileSync(summaryPath, '# Summary'); + // Verification NEWER than the summary → the check runs to completion + // (no fault injected) and genuinely finds nothing stale. + setMtime(summaryPath, '2026-01-01T00:00:00.000Z'); + setMtime(verificationPath, '2026-01-01T00:01:00.000Z'); + + const result = readVerificationStatus(dir, { phaseCleanCommitTimesMs: () => new Map() }); + + assert.equal(result.status, 'passed'); + assert.strictEqual( + result.staleCheckIndeterminate, + undefined, + 'a completed check that found nothing stale must not be flagged indeterminate', + ); + }); +}); + // ─── #2617: next_command runtime projection ────────────────────────────────── // // Regression tests for #2617 — verification-status `next_command` bypassed the diff --git a/tests/workstream-inventory.test.cjs b/tests/workstream-inventory.test.cjs index 82f810260..f538cf2e7 100644 --- a/tests/workstream-inventory.test.cjs +++ b/tests/workstream-inventory.test.cjs @@ -1762,3 +1762,103 @@ describe('#2645 — deleting a verification report must not raise completeness', 'the in-milestone phase (2-new) must be recorded normally'); }); }); + +// ───────────────────────────────────────────────────────────────────────────── +// #3057 B3: inspectWorkstream surfaces an indeterminate staleness check +// +// readVerificationStatus's internal staleness check can fail (fs / +// scanPhasePlans / clock error). Pre-#3057 B3 wiring, that failure was +// dropped here — `liveVerificationStatus` used only `.status` — so nothing +// could ever distinguish "checked; nothing is stale" from "could not check". +// `WorkstreamInventory`'s own return shape has no per-phase verification +// detail to carry this on, so it is surfaced via the SAME injectable +// stderr-diagnostic seam #3057 B4 added to cmdGitBaseBranch (see +// tests/git-base-branch.test.cjs), never via a change to `phases[]`/ +// `completed_phases` — the rollup routing must stay byte-identical. +// ───────────────────────────────────────────────────────────────────────────── + +describe('#3057 B3: inspectWorkstream — verification staleness-check indeterminate is surfaced', () => { + let tmpDir; + before(() => { tmpDir = createFixture(); }); + after(() => cleanup(tmpDir)); + + const V2_STATE = 'milestone: v2.0\nstatus: executing\n'; + + function roadmapWithRows(rows) { + return [ + '# Roadmap', '', '## Progress', '', + '| Phase | Milestone | Plans Complete | Status | Completed |', + '| --- | --- | --- | --- | --- |', + ...rows, + '', + ].join('\n'); + } + + /** Writes a verified phase directory with a deterministic (never-stale) mtime ordering. */ + function writeVerifiedPhase(wsDir, slug) { + const dir = path.join(wsDir, 'phases', slug); + fs.mkdirSync(dir, { recursive: true }); + const summaryPath = path.join(dir, '01-SUMMARY.md'); + const verificationPath = path.join(dir, '01-VERIFICATION.md'); + fs.writeFileSync(path.join(dir, '01-PLAN.md'), '# plan\n'); + fs.writeFileSync(summaryPath, '# summary\n'); + fs.writeFileSync(verificationPath, '---\nstatus: passed\n---\n'); + // Deterministic mtime ordering — never rely on write-order clock ties. + const older = new Date('2026-01-01T00:00:00.000Z'); + const newer = new Date('2026-01-01T00:01:00.000Z'); + fs.utimesSync(summaryPath, older, older); + fs.utimesSync(verificationPath, newer, newer); + return { summaryPath, verificationPath }; + } + + test('an fs failure inside the staleness check writes a stderr diagnostic; the rollup is UNCHANGED', (t) => { + const wsDir = seedWorkstream(tmpDir, { name: 'ws-3057-b3-fault' }); + fs.writeFileSync(path.join(wsDir, 'STATE.md'), V2_STATE); + fs.writeFileSync(path.join(wsDir, 'ROADMAP.md'), roadmapWithRows([ + '| 1. Alpha | v2.0 | 1/1 | Complete | - |', + ])); + const { summaryPath, verificationPath } = writeVerifiedPhase(wsDir, '1-alpha'); + const origStatSync = fs.statSync; + + t.mock.method(fs, 'statSync', function injectedStaleCheckFault(target, ...args) { + const targetPath = String(target); + if (targetPath === verificationPath || targetPath === summaryPath) { + throw new Error('injected stat failure (#3057 B3)'); + } + return origStatSync.call(fs, target, ...args); + }); + + const calls = []; + const inv = inspectWorkstream(tmpDir, 'ws-3057-b3-fault', { + active: null, + writeDiagnostic: (message, meta) => { calls.push({ message, meta }); }, + }); + + assert.ok(inv); + // Pre-existing no-throw fail-open routing is UNCHANGED: the rollup counts + // exactly what it would without the injected fault. + assert.equal(inv.completed_phases, 1, 'rollup routing unchanged'); + assert.strictEqual(calls.length, 1, 'an indeterminate staleness check must write exactly one diagnostic'); + assert.strictEqual(calls[0].meta.phaseDir, '1-alpha', 'diagnostic meta should name the affected phase directory'); + assert.strictEqual(calls[0].meta.reason, 'staleCheckIndeterminate', 'diagnostic meta should carry a stable reason'); + }); + + test('a completed staleness check that finds nothing stale writes NO diagnostic', () => { + const wsDir = seedWorkstream(tmpDir, { name: 'ws-3057-b3-ok' }); + fs.writeFileSync(path.join(wsDir, 'STATE.md'), V2_STATE); + fs.writeFileSync(path.join(wsDir, 'ROADMAP.md'), roadmapWithRows([ + '| 1. Alpha | v2.0 | 1/1 | Complete | - |', + ])); + writeVerifiedPhase(wsDir, '1-alpha'); + + const calls = []; + const inv = inspectWorkstream(tmpDir, 'ws-3057-b3-ok', { + active: null, + writeDiagnostic: (message, meta) => { calls.push({ message, meta }); }, + }); + + assert.ok(inv); + assert.equal(inv.completed_phases, 1); + assert.strictEqual(calls.length, 0, 'a completed, non-stale check must not write any diagnostic'); + }); +}); diff --git a/tests/worktree-base-ref.test.cjs b/tests/worktree-base-ref.test.cjs index 4407385c1..9044767b4 100644 --- a/tests/worktree-base-ref.test.cjs +++ b/tests/worktree-base-ref.test.cjs @@ -16,6 +16,8 @@ const { describe, test } = require('node:test'); const assert = require('node:assert/strict'); const path = require('node:path'); +const { makeFaultyGit } = require('./helpers/faulty-deps.cjs'); + const MODULE_PATH = path.join( __dirname, '..', 'gsd-core', 'bin', 'lib', 'worktree-base-ref.cjs' ); @@ -380,6 +382,42 @@ describe('evaluateWorktreeBaseDegrade', () => { assert.strictEqual(result.reason, 'no-head'); }); + // ─── #3057 B8: headAbsenceVerified distinguishes the two "no-head" causes ── + // + // Both outcomes below keep `shouldDegrade:false, reason:'no-head'` — that + // product decision is deliberately UNCHANGED (pinned by the regression + // guards above and flagged in the #3050 review as still an open question). + // What changes is that a caller can now tell git's DEFINITIVE "not a git + // repository" answer (exit 128) apart from git completing but returning + // nothing useful (exit 0, empty stdout) — the module's own #380-383 comment + // named this gap; these two paired tests prove it is closed. + + test('exit 128 — git\'s definitive "not a git repository" answer → headAbsenceVerified:true', () => { + const faultyGit = makeFaultyGit({ + faults: [{ kind: 'exit', exitCode: 128, stderr: 'fatal: not a git repository' }], + }); + const result = evaluateWorktreeBaseDegrade({ execGit: faultyGit }); + assert.strictEqual(result.shouldDegrade, false); + assert.strictEqual(result.reason, 'no-head'); + assert.strictEqual(result.headAbsenceVerified, true); + }); + + test('exit 0 with empty stdout — git completed but gave no useful answer → headAbsenceVerified:false', () => { + // makeFaultyGit()'s default passthrough IS exit 0 / empty stdout / no + // error / not timed out — a real, completed, but non-substantive answer. + const faultyGit = makeFaultyGit(); + const result = evaluateWorktreeBaseDegrade({ execGit: faultyGit }); + assert.strictEqual(result.shouldDegrade, false); + assert.strictEqual(result.reason, 'no-head'); + assert.strictEqual(result.headAbsenceVerified, false); + }); + + test('headAbsenceVerified is null (not applicable) for a reason other than no-head', () => { + const result = evaluateWorktreeBaseDegrade({ effectiveBaseRef: 'head' }); + assert.strictEqual(result.reason, 'baseref-head'); + assert.strictEqual(result.headAbsenceVerified, null); + }); + test('HEAD == origin/HEAD → no degrade, reason head-matches-fork', () => { const HEAD_SHA = 'aabbccdd11223344aabbccdd11223344aabbccdd'; const result = evaluateWorktreeBaseDegrade({ diff --git a/tests/worktree-safety.test.cjs b/tests/worktree-safety.test.cjs index c8930e6c1..f19b1dd0b 100644 --- a/tests/worktree-safety.test.cjs +++ b/tests/worktree-safety.test.cjs @@ -21,6 +21,7 @@ const path = require('node:path'); const childProcess = require('node:child_process'); const fc = require('fast-check'); const { createTempGitProject, createTempDir, cleanup } = require('./helpers.cjs'); +const { makeFaultyGit } = require('./helpers/faulty-deps.cjs'); const WORKTREE_SAFETY_PATH = path.join( __dirname, '..', 'gsd-core', 'bin', 'lib', 'worktree-safety.cjs' @@ -405,7 +406,36 @@ describe('planWorktreePrune', () => { }, }); assert.strictEqual(plan.action, 'metadata_prune_only'); - assert.strictEqual(plan.reason, 'no_worktrees'); + // #3050/#3057 (B6): a parse failure must NOT collide with the + // genuinely-empty-list verdict below ('no_worktrees') — see the paired + // test 'reason distinguishes a parse failure from a genuinely empty list'. + assert.strictEqual(plan.reason, 'parse_failed'); + }); + + // Counter-test pair (B5/B6 negative-space, #3057): the same 0-worktrees + // shape must yield a DIFFERENT reason depending on WHY the list came back + // empty — a parser that threw vs a porcelain output that genuinely listed + // nothing. Asserting only one of these could not prove they are + // distinguishable; asserting both, side by side, proves it. + test('reason distinguishes a parse failure from a genuinely empty list', () => { + const failurePlan = planWorktreePrune('/repo/main', {}, { + execGit: () => ({ exitCode: 0, stdout: 'not-porcelain', stderr: '' }), + parseWorktreePorcelain: () => { + throw new Error('parse failed'); + }, + }); + assert.strictEqual(failurePlan.action, 'metadata_prune_only'); + assert.strictEqual(failurePlan.reason, 'parse_failed'); + + const benignPlan = planWorktreePrune('/repo/main', {}, { + execGit: () => ({ exitCode: 0, stdout: '', stderr: '' }), + parseWorktreePorcelain: () => [], + }); + assert.strictEqual(benignPlan.action, 'metadata_prune_only'); + assert.strictEqual(benignPlan.reason, 'no_worktrees'); + + assert.notStrictEqual(failurePlan.reason, benignPlan.reason, + 'a parse failure must not be reported as the same reason as a genuinely empty worktree list'); }); // Counter-test: timeout path (Contract 6) @@ -629,6 +659,41 @@ describe('inspectWorktreeHealth', () => { const result = inspectWorktreeHealth('/tmp', {}, { execGit: makeTimeoutStub() }); assert.strictEqual(Array.isArray(result.findings), true, 'findings must be an array even when ok:false'); }); + + // Counter-test (B5, #3057): a statSync throw must surface as its own + // 'unverified' finding, distinct from 'orphan' (the case above, where + // existsSync itself says the path is absent). Silently producing NO finding + // for an unverifiable worktree would be the fail-open this row exists to close. + test('reports an unverified finding (not silently healthy, not orphan) when statSync throws', () => { + const health = inspectWorktreeHealth( + '/repo/main', + { staleAfterMs: 60 * 60 * 1000, nowMs: 2 * 60 * 60 * 1000 }, + { + execGit: () => ({ + exitCode: 0, + stdout: [ + 'worktree /repo/main', + 'HEAD aaa', + 'branch refs/heads/main', + '', + 'worktree /repo/wt-unverified', + 'HEAD bbb', + 'branch refs/heads/feat-a', + '', + ].join('\n'), + stderr: '', + }), + existsSync: () => true, + statSync: () => { + throw Object.assign(new Error('EACCES: permission denied'), { code: 'EACCES' }); + }, + } + ); + assert.strictEqual(health.ok, true); + assert.deepStrictEqual(health.findings, [ + { kind: 'unverified', path: '/repo/wt-unverified' }, + ]); + }); }); // ─── snapshotWorktreeInventory ──────────────────────────────────────────────── @@ -663,8 +728,8 @@ describe('snapshotWorktreeInventory', () => { ); assert.strictEqual(inventory.ok, true); assert.deepStrictEqual(inventory.entries, [ - { path: '/repo/wt-a', exists: true, isStale: true, ageMinutes: 120 }, - { path: '/repo/wt-b', exists: false, isStale: false, ageMinutes: null }, + { path: '/repo/wt-a', exists: 'present', isStale: true, ageMinutes: 120 }, + { path: '/repo/wt-b', exists: 'absent', isStale: false, ageMinutes: null }, ]); }); @@ -694,6 +759,52 @@ describe('snapshotWorktreeInventory', () => { 'must use reason=git_timed_out when execGit returns timedOut:true' ); }); + + // Counter-test pair (B5, #3057): a statSync throw must NOT be reported as + // exists:'present' (unverified presence masquerading as confirmed presence), + // and must be distinguishable from a genuinely-absent worktree (existsSync + // returns false for a different entry in the SAME call). Asserting only one + // side could not prove the two are distinguishable. + test("exists is 'unverified' (not 'present') when statSync throws — distinct from a genuinely absent worktree", () => { + const porcelain = [ + 'worktree /repo/main', + 'HEAD aaa', + 'branch refs/heads/main', + '', + 'worktree /repo/wt-unverified', + 'HEAD bbb', + 'branch refs/heads/feat-a', + '', + 'worktree /repo/wt-absent', + 'HEAD ccc', + 'branch refs/heads/feat-b', + '', + ].join('\n'); + const inventory = snapshotWorktreeInventory( + '/repo/main', + { staleAfterMs: 60 * 60 * 1000, nowMs: 2 * 60 * 60 * 1000 }, + { + execGit: () => ({ exitCode: 0, stdout: porcelain, stderr: '' }), + existsSync: (p) => p !== '/repo/wt-absent', + statSync: (p) => { + if (p === '/repo/wt-unverified') { + throw Object.assign(new Error('EACCES: permission denied'), { code: 'EACCES' }); + } + return { mtimeMs: 0 }; + }, + } + ); + assert.strictEqual(inventory.ok, true); + assert.deepStrictEqual(inventory.entries, [ + { path: '/repo/wt-unverified', exists: 'unverified', isStale: false, ageMinutes: null }, + { path: '/repo/wt-absent', exists: 'absent', isStale: false, ageMinutes: null }, + ]); + assert.notStrictEqual( + inventory.entries[0].exists, + inventory.entries[1].exists, + 'an unverifiable statSync failure must not collapse to the same exists value as a genuinely absent worktree' + ); + }); }); // ─── Degraded-git prune flow (AC3) ─────────────────────────────────────────── @@ -2922,6 +3033,111 @@ describe('executeWorktreeWaveCleanupPlan', () => { assert.equal(result.ok, true, 'cleanup can still succeed after rescuing on cat-file timeout'); }); + // ─── B7 (#3050/#3057): rescue-anyway on an uncertain cat-file, via the + // Phase 2 fault-injection adapter (makeFaultyGit) rather than a hand-rolled + // key-matching stub. The two tests above already prove the verdict with + // hand-written mocks; these prove it again through the in-process fault + // seam the epic standardized on, for both fault shapes the design calls + // "uncertain": a fatal exit (128) and a timeout. + // + // #3057 finding: `git cat-file -e` returns exit 128 BOTH for the normal + // "absent from HEAD" case (#2556's own comment above, line ~2833) and for a + // genuine fatal git error — there is no third exit code that means + // "confirmed absent, no ambiguity" as opposed to "uncertain". The + // production code's own non-zero-exit check (`catFileResult.exitCode === 0 + // ? skip : rescue`) therefore CANNOT distinguish "uncertain" from + // "certain-and-fine" — every non-zero exit, whatever its cause, is uncertain + // by construction, and #2556 rescues all of them uniformly. That is not a + // gap the tests below can paper over: it is the reason "rescue whenever not + // confirmed committed" is the whole rule, rather than a narrower + // uncertain-only carve-out. + function makeWaveCleanupPassthrough() { + return (args) => { + const key = args.join(' '); + if (key === '-C /repo/.claude/worktrees/agent-a1 rev-parse --abbrev-ref HEAD') { + return { exitCode: 0, stdout: 'worktree-agent-a1', stderr: '', signal: null, error: null, timedOut: false }; + } + if (key === 'merge-base HEAD worktree-agent-a1') { + return { exitCode: 0, stdout: 'abc123', stderr: '', signal: null, error: null, timedOut: false }; + } + if (key === 'diff --diff-filter=D --name-only HEAD...worktree-agent-a1') { + return { exitCode: 0, stdout: '', stderr: '', signal: null, error: null, timedOut: false }; + } + if (key === '-C /repo/.claude/worktrees/agent-a1 status --porcelain --untracked-files=all') { + return { exitCode: 0, stdout: '', stderr: '', signal: null, error: null, timedOut: false }; + } + if (key.startsWith('merge worktree-agent-a1')) { + return { exitCode: 0, stdout: '', stderr: '', signal: null, error: null, timedOut: false }; + } + if (key === 'worktree remove /repo/.claude/worktrees/agent-a1 --force') { + return { exitCode: 0, stdout: '', stderr: '', signal: null, error: null, timedOut: false }; + } + if (key === 'branch -D worktree-agent-a1') { + return { exitCode: 0, stdout: '', stderr: '', signal: null, error: null, timedOut: false }; + } + return { exitCode: 0, stdout: '', stderr: '', signal: null, error: null, timedOut: false }; + }; + } + + const waveCleanupPlanFixture = { + ok: true, + repoRoot: '/repo/main', + action: 'cleanup_wave', + discovery: 'manifest', + entries: [{ + agent_id: 'a1', + worktree_path: '/repo/.claude/worktrees/agent-a1', + branch: 'worktree-agent-a1', + expected_base: 'abc123', + }], + }; + + test('#3057/B7 (makeFaultyGit): cat-file exit 128 still rescues anyway (deliberate, #2556)', () => { + const rescued = []; + const execGit = makeFaultyGit({ + faults: [{ kind: 'exit', exitCode: 128, when: (args) => args.includes('cat-file') }], + passthrough: makeWaveCleanupPassthrough(), + }); + const result = executeWorktreeWaveCleanupPlan(waveCleanupPlanFixture, { + execGit, + findSummaryFiles: (worktreePath) => ( + worktreePath === '/repo/.claude/worktrees/agent-a1' + ? ['/repo/.claude/worktrees/agent-a1/.planning/q1-SUMMARY.md'] + : [] + ), + readFileSync: () => 'summary content', + existsSync: () => false, + mkdirSync: () => {}, + copyFileSync: (src, dest) => { rescued.push({ src, dest }); }, + }); + assert.strictEqual(rescued.length, 1, + 'an uncertain cat-file (exit 128) must still rescue — the deliberate #2556 verdict'); + assert.strictEqual(result.ok, true); + }); + + test('#3057/B7 (makeFaultyGit): cat-file timeout still rescues anyway (deliberate, #2556)', () => { + const rescued = []; + const execGit = makeFaultyGit({ + faults: [{ kind: 'timeout', when: (args) => args.includes('cat-file') }], + passthrough: makeWaveCleanupPassthrough(), + }); + const result = executeWorktreeWaveCleanupPlan(waveCleanupPlanFixture, { + execGit, + findSummaryFiles: (worktreePath) => ( + worktreePath === '/repo/.claude/worktrees/agent-a1' + ? ['/repo/.claude/worktrees/agent-a1/.planning/q1-SUMMARY.md'] + : [] + ), + readFileSync: () => 'summary content', + existsSync: () => false, + mkdirSync: () => {}, + copyFileSync: (src, dest) => { rescued.push({ src, dest }); }, + }); + assert.strictEqual(rescued.length, 1, + 'an uncertain cat-file (timeout) must still rescue — the deliberate #2556 verdict, same as exit 128'); + assert.strictEqual(result.ok, true); + }); + test('blocks dirty worktrees before merge/remove/delete', () => { const calls = []; const plan = {