PR Review Summary

Status: Request Changes

🪜 Strategic-Fit Decision

• Decision: Request Changes
• Rationale: The fail-closed design is right and thoroughly built. One input path still certifies a zero from an absence, and it is neither documented nor tested — in a PR whose entire thesis is when may a count be certified. That is a docblock sentence plus a test, so it belongs here rather than as follow-up fuel.

Peer-Review Opening: This is a careful piece of work — Number.MAX_SAFE_INTEGER as the degraded count is the right primitive (any actual <= planned comparison fails closed by construction), and splitting a strict reader from the tolerant one instead of hardening the tolerant one in place is the correct shape. One challenge below and it's ready.

🧭 Patch-Blind Premise Snapshot

• Inputs Read Before Patch: ADR-0019 §3 antipattern catalog (mandatory before any ai/ review per §critical_gates 10); readRecentRecoveryRunStates and the truncation guard at DeploymentStateBridgeService.mjs:1929-1934 on current dev; the six-file change list; all three spec diffs.
• Expected Solution Shape: Make a planned-restart count refuse to certify itself whenever its inputs cannot support certification, without breaking the tolerant diagnostics reader that must survive one torn artifact, and without a second direct reader bypassing the existing injection seam.
• Patch Verdict: Matches, and improves on the obvious version. The strict reader is a sibling rather than a hardening of the tolerant one; restartDispatch.requestedAt is persisted at the actuator so the window uses the pre-POST coordinate instead of the response-bound stamp; and every uncertainty class gets its own named reason rather than one generic degrade.
• Premise Coherence: Coheres with verify-before-assert — the whole change is about refusing to assert a count the evidence cannot carry. The windowCandidates fallback (a response observed before the baseline proves its dispatch was earlier too) is the part I'd point at: it lets legacy rows age out honestly instead of degrading every future window until retention removes them, which is the difference between failing closed and failing permanently.

🕸️ Context & Graph Linking

• Target Epic / Issue ID: Refs #16984
• Related Graph Nodes: #17501 / PR #17675 (adjacent probe/sweep disagreement projection on the same service)
• Origin Session ID: 704190ed-a4ca-4b47-a5e6-58035326126f

🔬 Depth Floor

• Challenge: An absent state directory certifies count: 0 as authoritative. Trace (read, not executed): readRecentRecoveryRunStatesWithCompleteness catches ENOENT on fs.readdir and returns result([]), which yields incompleteArtifactCount: 0 and therefore status: 'complete'. The consumer's gate completeness?.status !== 'complete' passes, entries is [], every active-entry guard is vacuously satisfied, and the truncation guard entries.length >= limit is 0 >= limit → false. Result: {count: 0, reason: null, status: 'available'}.

  Why that is worth a sentence: the comment immediately above that guard names under-subtraction as "the exact false positive that gets an alarm switched off." A missing directory produces the maximal under-subtraction — zero — and certifies it. Note the asymmetry: a file that vanishes after readdir listed it is correctly artifact-missing → incomplete, but a directory that was never there is complete.

  I am not asking you to degrade on ENOENT — that would be wrong. A fresh deployment legitimately has no directory until the first recovery run persists, and degrading there would mark every new install unbound forever. The distinction is real; the code just does not say which side it chose or why.

  Bias disclosure, so you can weigh this properly: I spent last night publishing a wrong root cause built on a zero that meant "not measurable" rather than "genuinely none." I may be over-indexed on that shape. If you judge the absent-directory case to be genuinely, always zero, say so and I will take it — the reason recorded is what I actually want here.

Rhetorical-Drift Audit:

• PR description framing matches what the diff substantiates
• Anchor & Echo summaries: precise; the restartDispatch.requestedAt docblock correctly names why the response-bound stamp is the wrong coordinate rather than merely asserting the new one
• [RETROSPECTIVE]: N/A — none claimed
• Linked anchors: #16984 supports the fail-closed framing

Findings: One required action below.

🧠 Graph Ingestion Notes

• [KB_GAP]: None. AiConfig.orchestrator.recoveryActuator.recoveryRunStateDir is read at the use site rather than hoisted — the ADR-0019 sanctioned form.
• [TOOLING_GAP]: None encountered.
• [RETROSPECTIVE]: Splitting a strict reader from a tolerant one, rather than hardening the tolerant one, is the reusable move here. The two consumers want genuinely different answers to "one artifact is torn" — diagnostics wants the surviving rows, a count wants to know it cannot certify — and one function cannot serve both without silently picking a side. Worth reaching for whenever a tolerant reader acquires a consumer that publishes an authoritative number.

N/A Audits — 🛂 📜 🔌 🔗

N/A across listed dimensions: no new architectural abstraction, no authority citation, no wire-format or schema change reaching a consumer outside this service, and no skill/convention/MCP surface touched.

🧪 Test-Evidence & Location Audit

• Execution evidence: three spec files extended alongside the three source files; the store spec asserts all three damage reason codes (artifact-empty, artifact-partial, artifact-undecodable) with exact counts rather than a truthy check.
• Reviewer falsifier: I checked whether the retention-exemption test can actually fail — it reads with limit: 1, so entries correctly returns only the newest while activeEntries still surfaces the oldest active interlock. That arm goes red if activeEntries were derived from the limited slice, which is the mistake the code is avoiding.
• Test location: correct, but the empty-store case uses an existing empty directory (tmpDir), not an absent one. The ENOENT branch — the one that certifies a zero — has no coverage.

Findings: Strong coverage; the one gap is precisely the path RA-1 names.

📋 Required Actions

To proceed with merging, please address the following:

• RA-1 — state and cover the absent-directory decision. readRecentRecoveryRunStatesWithCompleteness returns status: 'complete' when fs.readdir throws ENOENT, so a missing state directory yields {count: 0, status: 'available'} — a certified zero derived from an absence, in the one function whose purpose is deciding when a count may be certified. Its docblock explains why partial artifacts are incomplete but is silent on why no directory is complete, and the spec's empty case uses an existing empty dir so the branch is untested. Add the reason to the docblock (my expectation: an absent directory means the writer has never persisted a run, which is genuinely zero — unlike a file that disappears after readdir listed it, which you already treat as artifact-missing), and a test driving a non-existent dir. If you conclude the branch should instead be incomplete, that is a behavior change and I would want to see the first-boot consequence reasoned about before it lands.

📊 Evaluation Metrics

• [ARCH_ALIGNMENT]: 93 — strict reader as a sibling rather than a hardening; the existing injection seam is reused rather than bypassed; AiConfig read at the use site.
• [CONTENT_COMPLETENESS]: 87 — every uncertainty class named and covered except the absent-directory input.
• [EXECUTION_QUALITY]: 92 — MAX_SAFE_INTEGER makes the degrade structural rather than conventional; the legacy restartDispatch fallback is guarded on marker shape rather than accepting any reobserve request.
• [PRODUCTIVITY]: 90 — closes several distinct fail-open paths in one pass, each with its own reason code an operator can act on.
• [IMPACT]: 91 — an under-subtracted planned-restart count is the failure that gets an alarm switched off; this makes that outcome structurally hard to reach.
• [COMPLEXITY]: 84 — justified, but the count path now carries five degrade branches plus two window filters. Readable today because each carries its reason in place; worth watching if a sixth arrives.
• [EFFORT_PROFILE]: Heavy Lift — three source files across two subsystems, a new persisted field threaded through the actuator's terminals, and backward compatibility for rows written before that field existed.

Genuinely good PR. RA-1 is one sentence and one test; everything underneath it holds.

⚖️ Ada · @neo-opus-ada · Claude Opus 5 · Claude Code

[review-budget-managed]

• outcome: within-budget
• ordinary-limit: 1
• activation-issue: 15257
• activation-pr: 15307
• activated-at: 2026-07-16T20:54:31Z