* fix(#3057): refuse the write when the duplicate scan cannot complete writeManifest documents itself as a fail-closed duplicate guard: if any existing manifest shares plan_id with a different, non-terminal job_id it must refuse, because dispatching again would duplicate the external job. It could not honour that. The scan reads every sibling manifest looking for the duplicate, and an unreadable or unparseable sibling was `continue`d past. If the corrupt file was the one holding the live duplicate, the scan found nothing and a duplicate external job dispatched. The asymmetry is what gives it away: a malformed TARGET refused with malformed_existing because clobbering is unacceptable, while a malformed SIBLING was skipped — yet siblings are the only thing the duplicate check reads. Adds a scan_incomplete verdict that refuses and names the offending file, so an operator can quarantine or repair it. Fail-closed alone would let one stale corrupt manifest wedge every dispatch for that planning dir permanently; naming the file is what makes refusing survivable. malformed_existing is untouched, so the target/sibling distinction stays visible. The docstring is updated — it previously stated a rule the function did not keep. memFs() gains an optional failReads map so these branches are reachable at all; they had zero coverage because the fake could not express a per-file read fault. The signature is additive and every existing caller is unchanged. The regression is proved by a pair, not a single test. A control writes a readable sibling holding a genuine non-terminal duplicate and asserts duplicate_plan_id, establishing the scenario is real; the regression then makes that same path unreadable and asserts scan_incomplete. A first draft of this test used a corrupt-JSON fixture containing no plan_id at all while its comment claimed otherwise — it duplicated the unparseable-sibling case and proved nothing, which is the defect class this phase exists to remove. Refs #3051 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(#3057): make a guard's failure distinguishable from its benign result Wave 1 of the negative-space backfill: the branches where a guard that could not verify something reported the same value it reports when everything is fine. That indistinguishability is the defect; every fix here makes the two states tellable apart, and every test proves it with a pair — one for the failure, one for the benign case. A single test cannot establish that two states are distinguishable, which is the whole property being fixed. state.cts phaseInventoryProvider returned null for both a real disk-scan failure and a genuinely empty phases dir, so `state rebuild` could report success while phase-table reconciliation never ran. It now returns a discriminated result and the CLI surfaces phase_inventory_scan_failed plus a reason. The reason field turned out never to have been wired into the emitted JSON at all — it existed only as an internal variable — so a test could only assert on the operator-facing note. It is a real field now. state.cts treated an unreadable lock body the same as an empty one, applying the 1-second stealable floor. A lock we cannot read is not a lock we know is stale; an unreadable body is now held to the deadman ceiling like a live holder. verification.cts findStaleVerificationSummary returned null on any fs, scan or clock failure — meaning "not stale". It now returns a discriminated StaleCheckResult and the caller records that the check was indeterminate. git-base-branch resolveBaseBranch returned 'main' both when no candidate branch existed and when every git tier timed out. A diagnostics variant now reports whether the answer was verified, and the CLI writes an unverified-fallback note to stderr. The stdout contract five workflows parse is untouched. worktree-safety snapshotWorktreeInventory left exists:true when statSync threw, so a guard that could not check reported the worktree present; exists is now tri-state and a stat failure surfaces as an 'unverified' finding. planWorktreePrune reported 'no_worktrees' for a parse failure, which is not the same as an empty list — and it drives a prune. It now reports 'parse_failed'. Fixing the inventory change exposed a second fail-open in verify.cts: the validate-health consumer silently dropped findings whose kind it did not recognise, so the new kind would have vanished. That is closed too — worth noting that the survey enumerated producers of degraded verdicts, not consumers that discard them. worktree-base-ref and state-transition gain the distinguishing signal without changing what they do: headAbsenceVerified, and a phase-inventory scan meta. Whether those guards should ACT differently is a product question this change does not answer, and both are flagged rather than quietly settled. rescueSummaryArtifacts is left alone: rescuing on an uncertain cat-file is deliberate per #2556. It now has tests proving it, and a recorded negative finding — git cat-file -e returns 128 for both "absent from HEAD" and a fatal error, so "uncertain" and "certain-and-fine" are not separable at the git level. Refs #3051 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * test(#3057): assert typed values, not rendered text Ten assertions in the rebuild CLI suite matched substrings of produced output — STATE.md body fields, a markdown table row, an audit-log heading, and JSON keys read as text. CONTRIBUTING prohibits that: if the code under test produces text, the test asserts on its structured surface instead. No production surface had to be built. Every one already existed and was already compiled into bin/lib: stateExtractField for body fields, parseMarkdownTable for the phase table, collectSection for the audit-log section, and result.data.log — already a typed RebuildLogEntry[]. The tests were matching rendered text sitting next to the structured data. One of those assertions was passing for the wrong reason. `stdout.includes ('rebuilt')` matched the JSON KEY name, not a value: the dry-run path emits `mutated` and the real path emits `rebuilt`, so it would have passed whether the value was true or false. It now asserts the value. external-job's refusal already had to name the offending file — that naming is why the fail-closed variant is survivable rather than a permanent wedge — but the tests proved it by substring of a prose message. The failure result now carries offendingPath as its own field and the tests assert it by value. The human message is unchanged; operators read it. Array membership is left alone. `phaseIds.includes('99')` and `result.updated.includes('Completed Phases')` are membership checks on real arrays, not text matching, and converting them would weaken nothing and clarify nothing. Refs #3051 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * test(#3057): execute acquireStateLock instead of grepping its source The non-EEXIST lock test asserted on the TEXT of the built .cjs and never called acquireStateLock. It carried an allow-test-rule: architectural-invariant exemption to permit that. A source grep proves a literal is present in a file, not that the behaviour works — it is weaker than a liveness test, which at least runs the code, and it was the only coverage the fatal-errno path had. Replaced with tests that inject the errno through fs and assert what actually happens: a fatal EACCES propagates out of acquireStateLock with zero backoff sleeps, while EAGAIN/EINTR/EINVAL/EIO/ENOENT/ESTALE/EPERM/EBUSY retry once and succeed. The exemption is removed and its allowlist entry with it. One old assertion is deliberately not carried over: it checked the retryable errnos were expressed as a Set rather than an inline literal. That is a shape check with no runtime signature; the behavioural tests fail if the code reverts to the old inline check, which is the regression it was really guarding. The #3057 lock-body tests move into that same file rather than a new one, which is what lint-test-file-count asks for and puts every acquireStateLock test in one place. Refs #3051 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(#3057): surface an indeterminate staleness check to its callers An isolated review caught an inconsistency inside this wave. Two of the three "add the distinguishing signal" fixes wire through to something a user sees: git base-branch writes an unverified-fallback diagnostic to stderr, and an unverifiable worktree surfaces as a W020 finding. The third set staleCheckIndeterminate on readVerificationStatus's result and nothing read it. A signal nobody consumes leaves the fail-open exactly as silent as before: the staleness check could fail and the operator saw precisely what they would see if the answer were genuinely "not stale". That is the defect this issue exists to remove, so it is not defensible as scaffolding when its two siblings in the same change already wire through. All five callers now surface it, each through the channel it already had rather than a mechanism imposed uniformly: phase complete adds it to its existing warnings array and, on the blocked path, as an additive note on the error text; init and roadmap carry it as a field on output they already emit; the UAT report carries it without ever gating passed/blockers; workstream inventory takes an injectable writeDiagnostic mirroring the git base-branch idiom, because its return shape had nowhere to hang a per-phase field without rippling the builder's types. The routing decision is unchanged everywhere. What changes is only that a caller and an operator can now tell a failed check from a completed one. That diagnostic carries structured meta rather than being asserted by regex — the default still writes only the human message to stderr, but tests assert phaseDir and reason by value. Two earlier assertions in this branch were converted the same way; this was the last raw-text assertion left. Also records a scope correction: the completePhaseCore guards now compare stateReplaceField's result to the body instead of testing truthiness, so a field whose substitution produced identical text no longer reports as updated. That is a real behaviour fix, not the signal-only change this file was described as carrying, and its tests cover both the changed and unchanged cases. Refs #3051 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * test(#3057): bound two heavy subprocesses for a loaded bench, not an idle one The remote matrix surfaced three failures unrelated to this branch's changes. All were bad tests, and a re-run would have hidden every one of them. The reviewer-flags parse block bounded bash -> node -> a full gsd-tools cold start at 5 seconds. On a bench running thirty thousand tests in parallel that is not a hang, it is a busy machine. Raised to 30s, matching the convention sibling suites already use for script invocations, with a comment saying what the budget covers so nobody tightens it back. Two further copies of the same 5-second spawn in the same file had the identical defect and are raised too — they were not in the failure report, but they will be next time. The fragment-propagation test bounded npm run regen:derived — a full build plus eight generators, the heaviest subprocess in the suite — at five minutes, and node22 was killed near the end. The captured output proves it: every generator had written its files and gen:install-tree had emitted all fifteen runtimes before the kill. Raised to fifteen minutes. That failure read as `null !== 0`, which says nothing. status null means killed, not a non-zero exit, and the two want different responses: one is a timeout to size correctly, the other is a real build break. The assertion now distinguishes them and names the signal. Neither test's assertions were weakened and no retry was added. A retry here would suppress exactly the signal the timeout exists to produce. Refs #3051 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * test(#3057): capture fd 1 through the mock tracker, not a raw reassignment The phase suite reported zero test results on both lanes while running for five and a half minutes and exiting 1. No assertion text, no stderr, four events for the whole file: enqueue, start, dequeue, complete. That shape is not a failing assertion — it is the runner being unable to read the child at all, because it parses its event stream from the child's stdout. The cause was the capture helper reassigning fs.writeSync directly. Proven rather than assumed: a standalone probe patched fs.writeSync and called process.stdout.write, and the interception fired only when fd 1 resolved to a FILE, not when it was a pipe. The remote runner captures the event stream to a file, so a helper that was invisible against a pipe swallowed the reporter's own output on the bench. That is also why the two sibling suites wired the same way in this change pass cleanly — they use the mock tracker, the seam io.test.cjs established for this exact function. The helper now uses t.mock.method with an explicit restore after each call, so teardown belongs to node:test rather than a second hand-rolled implementation, and the interception cannot outlive the one synchronous call it wraps even if that call throws. Ten call sites thread the test context through; three test callbacks gained the parameter they lacked. The three B3 tests are untouched — same assertions, same fault injection. Only how the context reaches the helper changed. Refs #3051 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * test(#3057): capture phase-complete output from a subprocess, not fd 1 Two attempts to make in-process fd-1 interception safe both failed on the bench. The suite reported zero test results on either lane while exiting 1 — four events for the whole file — because the runner parses its event stream from the child's stdout, and process.stdout.write routes through fs.writeSync whenever fd 1 resolves to a file, which is how the runner captures. Patching that seam anywhere in a file can therefore destroy the file's own reporting, and tightening the window only moved the runtime from 326s to 125s without recovering a single event. So the interception is gone rather than tuned. The helper now spawns gsd-tools as a real subprocess and reads stdout the way the OS already gives it to us, which is what the rest of the suite does. It asserts the command succeeded before parsing, so a genuine failure can no longer present as a JSON parse error. The two fault-injecting tests could not survive that move as written: a subprocess cannot see a mock installed in the parent. Instead of reinstating the interception they now produce the fault on disk — the summary artifact is created as a dangling symlink, so the staleness check's real statSync throws inside the child. That is a more honest fixture than a mock in any case, since it is a condition a user's tree can actually be in. Skipped on Windows, matching the existing symlink precedent in the write-guard suite. Three further call sites turned out to depend on parent-process writeFileSync mocks the subprocess could not see. Those call the CJS function directly, which is what they always wanted — they never needed stdout at all. Refs #3051 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(#3057): one name for one signal, one encoding for one distinction Standards review found four things this branch introduced, all of them inconsistencies with itself rather than with the repo. One upstream bit reached its consumers under three names — verification_stale_check_indeterminate in two modules, the same value with "stale" dropped in a third, and stderr only in the fourth. Standardised on the long name wherever it is a field. The workstream inventory keeps its stderr channel, since its return shape has nowhere to hang a per-phase field without rippling the builder's types, but it now says the same word for the same thing. worktree-safety encoded one three-way distinction two ways in a single file: a named union for a finding's kind, and boolean|null for an inventory entry's existence. The second is now a named union too. Two assertions matched human prose because the blocked and non-blocked completion paths carried no typed field for the signal. Both now assert typed values. The first round of this fix added the field but left the regex beside it, which is the banned pattern sitting next to its own replacement; the second removed it and added an assertion on the reason enum so nothing was lost. The remaining two were reasoned away before being fixed, and both reasons were bad. "No typed surface exists" is the condition CONTRIBUTING says to fix by adding one — it took three lines. "The file already does this dozens of times" is not licence to add instance number thirty-one; a convention that violates a documented rule is debt, not precedent. Vocabulary differing across DIFFERENT modules is left alone: CONTEXT.md rejects a single shared result envelope, so per-module shapes are precedented, and a baseline smell does not outrank a documented standard. A census of every line this branch adds to a test file now finds no regex or substring assertion on produced prose: 87 strictEqual, 25 ok (all non-empty or shape guards), 12 equal, 3 throws (all typed err.code predicates), 3 deepStrictEqual, 2 notStrictEqual. Refs #3051 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * chore(#3057): backfill changeset pr number to 3088 --------- Co-authored-by: sim <sim@local> Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
This commit is contained in:
Tom Boucher
@@ -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/<job_id>.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<string, unknown>;
|
||||
try {
|
||||
existing = JSON.parse(raw) as Record<string, unknown>;
|
||||
} 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;
|
||||
|
||||
@@ -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;
|
||||
|
||||
16
src/init.cts
16
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,
|
||||
};
|
||||
}
|
||||
|
||||
|
||||
11
src/io.cts
11
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<string, unknown>): 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');
|
||||
|
||||
@@ -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);
|
||||
|
||||
@@ -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}`);
|
||||
}
|
||||
|
||||
|
||||
@@ -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 |
|
||||
|
||||
124
src/state.cts
124
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');
|
||||
}
|
||||
|
||||
|
||||
@@ -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,
|
||||
};
|
||||
}
|
||||
|
||||
|
||||
@@ -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 } : {}),
|
||||
};
|
||||
}
|
||||
|
||||
|
||||
@@ -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',
|
||||
);
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
@@ -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,
|
||||
};
|
||||
});
|
||||
|
||||
|
||||
@@ -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 };
|
||||
}
|
||||
|
||||
@@ -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,
|
||||
|
||||
Reference in New Issue
Block a user