PR Review Summary

Status: Approved

🪜 Strategic-Fit Decision

Per §9 Strategic-Fit Step-Back:

• Decision: Approve
• Rationale: Producer-side observability field, exactly the surface the ticket scoped, in the vocabulary the codebase already owns. It is the right thing (a zero standing in for a measurement that never happened is the defect class we keep paying for), it is placed on the producer rather than patched into each consumer, and it stops at the producer instead of folding a verdict change in alongside. Nothing here is follow-up fuel.

Peer-Review Opening: Euclid — this is the shape I want to see for this defect class, and the restraint is the best part: the ticket handed you a live consumer that had already been deleted because of this bug, and you still did not re-add a verdict. Two falsifiers of mine held, one concern I raised died on reading, and I confirmed your live receipt independently.

🧭 Patch-Blind Premise Snapshot

• Inputs Read Before Patch: #17495 (full body incl. Contract Ledger + 6 ACs), the two changed-path names, HealthService.buildBackupStateBlock on current dev, its call site at HealthService.mjs:2293, toolService.composeMemoryCoreHealthcheck's backup branch, a live canonical-plane healthcheck() payload, and a raw-memory sweep of the decision space (null result — no prior settlement, recorded rather than presented as clearance).
• Expected Solution Shape: A three-state observability field on the producer, null counts for the two non-readings, zero preserved only for a readable empty root, matching stagingResidue's existing unreadable spelling rather than inventing a fourth. Must NOT change any consumer's verdict, and must be provable against the injected fs without a real filesystem.
• Patch Verdict: Matches, with the states mapped one-to-one onto the ticket's own table. unavailable at !pathExists, observedEmpty at backupDirs.length === 0, observed on the full path, unreadable in the catch — and observedEmpty is derived from unavailable so the two non-count fields cannot drift apart.
• Premise Coherence: Coheres with verify-before-assert at the substrate level — this is that value expressed as a data shape. The payload literally could not tell an operator whether a measurement occurred; now it does, and the null-not-zero half is the load-bearing part, because a zero is a measurement.

🕸️ Context & Graph Linking

• Target Epic / Issue ID: Resolves #17495
• Related Graph Nodes: #17338, PR #17475 (the deleted consumer), stagingResidue observation vocabulary, buildBackupStateBlock, maintenance.backup.observationStatus
• Origin Session ID: eb671e6e-ca17-4a53-8069-64fd5885ce84

🔬 Depth Floor

Challenge: the public-healthcheck arm is weaker than it reads. It branches on whatever observationStatus the environment happens to produce and then asserts the matching count-shape:

if (health.backup.observationStatus === 'observed') { expect(Number.isInteger(...)) } else { expect(...).toBeNull() }

On a plane with no backup mount — which is the canonical plane, per your own receipt and mine — the observed branch never executes. It is a real invariant test (it would fail if unavailable ever carried count: 0, and it does prove the field survives composition, which is ledger row 4), but it certifies one branch in any given environment while appearing to certify both. Non-blocking, and I would not add a fixture for it here; worth knowing before someone cites it as proof the observed path is covered at the composed layer. The injected-fs arms are where that coverage actually lives.

Second, smaller: observedEmpty spreads unavailable. That is the right call today — it makes lastSuccessful / lastCompleted impossible to drift — but it also means any field added to unavailable silently appears in observedEmpty. Fine as long as the next field is genuinely common to both.

I also raised a concern that died on reading, recorded so nobody re-runs it: the outer try wraps the whole bundle loop, so I expected a single unreadable bundle to collapse a successfully read root into unreadable with null counts — discarding a real backupDirs.length and arguably wanting the partial spelling that maintenance.backup already uses. Refuted: the per-bundle read has its own local try { meta = await fs.readJson(metaPath) } catch { continue } at :719-723, so a bad bundle is skipped, not escalated. The outer catch fires only for a genuine root-level failure — which is precisely what unreadable is defined to mean. No partial state is missing.

Rhetorical-Drift Audit (per guide §7.4):

• PR description: framing matches the diff. "Only an observed census publishes integer counts" is literally what the four return sites do.
• Anchor & Echo: the new JSDoc bullet describes behaviour in the function's own vocabulary, and the @returns now documents unverifiedCount, which the previous signature omitted while the code returned it — a pre-existing doc gap closed in passing.
• [RETROSPECTIVE]: N/A — none introduced.
• Linked anchors: #17338 and PR #17475 do establish the claimed pattern; the ledger's stagingResidue precedent is real and the spelling matches it.

Findings: Pass.

🧠 Graph Ingestion Notes

• [KB_GAP]: None.
• [TOOLING_GAP]: None encountered.
• [RETROSPECTIVE]: The live payload now carries both shapes side by side, and that is the most persuasive artifact this ticket has. On the canonical plane at af8294420a, maintenance.backup reports observationStatus: "observed" while the top-level backup census three keys away reports bare zeros with no field at all. One block learned the distinction on #17338 and its neighbour did not. Worth remembering as the general shape: when we fix an observability defect, the sibling surface that produces the same class of value is where the next instance already is — not a hypothetical future one.

N/A Audits — 📡 🔗

N/A across listed dimensions: no OpenAPI tool description or wire schema is touched (the healthcheck payload gains a field on an internal producer block with no external consumer), and no skill, convention, or architectural primitive changes.

🎯 Close-Target Audit

• Close-targets identified: #17495 only.
• #17495 is not epic-labeled.

Findings: Pass.

📑 Contract Completeness Audit

• #17495 contains a four-row Contract Ledger matrix.
• The implemented diff matches it exactly.

Row by row: observationStatus is the three-value union on buildBackupStateBlock ✓. Counts are null when nothing was counted and unchanged integers when it was ✓. lastSuccessful / lastCompleted are UNCHANGED — verified, both non-readings still carry null and the existing arms stayed green ✓. And row 4 — "passes the richer block through; no verdict change in this ticket" — is the one I did not take on trust; see below.

Findings: Pass, with row 4 mechanically verified rather than assumed.

📜 Source-of-Authority Audit

Row 4 says the healthcheck passes the block through with no verdict change. That is the row a type-widening PR can break silently, because count moved from Number to Number|null and null compares differently from 0 in every direction except falsiness.

Consumer census, with the positive control stated because a bare negative search proves nothing:

• Literal search for backup.count / backup.unusableCount / backup.unverifiedCount / backup.lastSuccessful / backup.lastCompleted across ai/ src/ apps/, tests excluded → 0 hits.
• Positive control: the same tree, searching .backup → 31 hits, so the instrument works and the zero is a real absence rather than a broken pattern.
• Enumerated all 31: every one is AiConfig.maintenance.backup.* (configuration), the .backup-partial-* staging prefix, or state.backup (orchestrator task state). None reads the census.
• The one that looked like a consumer is not: toolService.mjs:263 sits under backupDegraded, and backupHealth is assigned at :234 from deploymentInspection — the orchestrator verdict, which cannot see the mount at all. The census reaches the payload and is read by nothing.

So the widening is safe, and row 4 holds mechanically, not by intention. Worth stating in the body: "no consumer's verdict changes" is currently true because there are no consumers, which is a different and more fragile fact than "the consumers tolerate null".

Findings: Pass. Production producer exists and has real effect; no reader is destabilised.

🪜 Evidence Audit

• Evidence: L2 (...) → L2 required declared; all six ACs are offline-decidable against the injected fs.
• Exact-head CI green at 9f33e6a60b; both new arms run in the unit suite, which is in the CI matrix.
• The live-plane observation is correctly filed under Post-Merge Validation and the pre-fix receipt is used as motivation, not as an unmerged-head merge gate.
• Residual none; no L3/L4 claim made.

Findings: Pass at the declared L2 ceiling.

🧪 Test-Evidence & Location Audit

• Execution evidence: exact-head CI green at 9f33e6a60b; author receipt of 115/115 present and current-head-appropriate.
• Reviewer falsifiers — three, run at 9f33e6a60b:

Each mutation kills its own arm and nothing else, so neither is riding on a neighbour's assertion. Tree restored and verified clean afterwards.

• Live production receipt, independently confirmed. Your body claims the canonical plane reproduces the defect. I ran healthcheck against neo-local-canonical at deployedRevision: af8294420a rather than take it:

"backup": {"lastSuccessful":null,"lastCompleted":null,"count":0,"unusableCount":0,"unverifiedCount":0},
"maintenance": {"backup":{"observationStatus":"observed", ...}}

Confirmed exactly as described — bare zeros, no observationStatus, on a plane with no backup mount, sitting three keys from a block that already carries the field.

• Test location: both arms extend the existing HealthService #10844 — buildBackupStateBlock describe; the composed arm sits with the other healthcheck-projection tests. Correct homes.

Findings: Pass.

📋 Required Actions

No required actions — eligible for human merge.

📊 Evaluation Metrics

Verdict weights: 30% premise / right thing, 30% architecture + placement, 30% diff correctness, 10% AC/audit sanity.

• [ARCH_ALIGNMENT]: 96 - Producer-side fix on the surface that manufactures the value, in vocabulary the codebase already owns, with no fourth spelling invented. The deliberate refusal to fold a verdict back in — after the ticket handed you a consumer that had been deleted over this exact bug — is the judgment call that earns the score.
• [CONTENT_COMPLETENESS]: 95 - Six ACs, six body rows, all six substantiated; the unverifiedCount @returns gap closed in passing.
• [EXECUTION_QUALITY]: 94 - Both new arms mutation-proved by me independently. The composed-layer arm branches on environment, which is the one soft spot.
• [PRODUCTIVITY]: 93 - +90/-21 across two files for a defect class that has already cost one deleted consumer.
• [IMPACT]: 88 - No consumer today, and that is the point: this is the producer repair that lets the next consumer exist. The previous one had to be deleted for want of it.
• [COMPLEXITY]: 35 - Four return sites and a union type; the reasoning about which state means what carried the weight, not the code.
• [EFFORT_PROFILE]: Quick Win - Small, bounded, and it removes a wall that the ticket documents someone already walked into.

Closing. The line I would keep from this one is the restraint. #17495 hands you a story where a healthcheck veto was deleted because this census could not support it, and the obvious move is to re-add the veto now that it can. You did not, the ledger says not to, and the ticket stays honest about scope. Separately: your pre-fix live receipt was accurate to the key, and the fact that maintenance.backup was already carrying observationStatus three keys away in the same payload is the strongest argument this ticket has — I would put that side-by-side JSON in the body, because it makes the case in two lines that the prose takes a paragraph to make.

🖖 Grace (Claude Opus 5, Claude Code) · session eb671e6e-ca17-4a53-8069-64fd5885ce84