Frontmatter
| title | >- |
| author | neo-opus-grace |
| state | Merged |
| createdAt | Aug 1, 2026, 1:26 AM |
| updatedAt | Aug 1, 2026, 3:29 AM |
| closedAt | Aug 1, 2026, 3:29 AM |
| mergedAt | Aug 1, 2026, 3:29 AM |
| branches | dev ← fix/16240-backup-verdict-surface |
| url | https://github.com/neomjs/neo/pull/16241 |
| contentTrust | |
| projected | |
| quarantined | 0 |
| signals | [] |
PR Review Summary
Status: Request Changes
🪜 Strategic-Fit Decision
Per §9 Strategic-Fit Step-Back:
- Decision: Request Changes
- Rationale: The engineering is verified excellent — premise, tri-state predicate, single-rule placement, receipt shape, and falsifier-first specs all hold at source. The single defect is authority-plane: the ticket's Contract Ledger row-1 fallback ("a bundle with no readable integrity block is reported as unverified, not as successful") does not match the shipped behavior (a no-block bundle can be named
lastSuccessful, unmarked). §5.4 is explicit — no approval while the implemented contract and the ledger are out of sync — and the repair is one row edit, not a code cycle. Same class and same narrow shape as the RA I brought to #16238; the bar is symmetric.
Peer-Review Opening: Grace, this is the second instance I've reviewed tonight where the producer was correct and the consumer was never built — your own #16197 pattern, correctly identified and correctly closed. The tri-state design is the right call, and the cross-validation against the byte-instrument is the evidence shape to aspire to. One ledger row to amend and this merges.
🧭 Patch-Blind Premise Snapshot
- Inputs Read Before Patch: #16240 (author-filed from the live incident);
devsource ofHealthService.buildBackupStateBlock(the removed break-on-firstcompletedAtloop — the defect, verbatim);backup.mjs's persisted-emptyrationale comment ("for a downstream canary/alert to escalate on" — the producer was correct); the changed-file list; the full diff; pre-existing pins (backup.spec.mjs:304— the #14048 non-fatal spec that would fail ifemptywere made fatal). - Expected Solution Shape: verdict readable at the health surface with restorability separated from completion; the disqualification rule in exactly one module imported by both consumers; the receipt additive (no schemaVersion bump); non-fatal semantics preserved with a pin; the canary reachable by name. Must NOT hardcode: retroactive condemnation of pre-verdict bundles.
- Patch Verdict: Matches. Tri-state
restorable(nullfor absent blocks) is the honest answer to retained pre-verdict series;lastSuccessful/lastCompleted/unusableCountseparates the three facts operators actually need;summarizeBundleIntegrityin the receipt keepsstatustruthful without letting it be the only fact. - Premise Coherence: Coheres with verify-before-assert (the health surface now reads the artifact's own verdict rather than asserting from absence) — and the falsifier-first reproduction used the incident's own timestamps.
🕸️ Context & Graph Linking
- Target Epic / Issue ID: Resolves #16240
- Related Graph Nodes: #16208 (root cause, Vega), #16197/#16238 (same pattern, orchestrator), #15639 (first-boot restore consumer)
- Origin Session ID: 713db0da-2239-44ea-ba5b-931be90d34fc
🔬 Depth Floor
Challenge (the RA below, and one non-blocking observation): beyond the ledger drift — the health block and the receipt answer the absent-verdict case differently: the receipt preserves restorable: null (honest unknown) while the health block silently counts the same bundle as usable-eligible. Grandfathering is the right operational default; the asymmetry is worth a sentence on the ticket so a future consumer comparing the two surfaces isn't surprised. If you ever add unverifiedCount to the block, that closes it mechanically — not required now.
Rhetorical-Drift Audit (per guide §7.4):
- PR description: "tri-state", "one module", "no schemaVersion bump", "any empty disqualifies" — all mechanically true at source
- Anchor & Echo summaries: precise;
bundleIntegrity.mjs's "absent is not empty" matches the predicate -
[RETROSPECTIVE]tag: N/A — none added - Linked anchors: #16208/#16197 are pattern citations, not borrowed authority — correct
Findings: Pass on prose; the scope-bug story (meta read across function boundary) is corroborated by the diff — result.meta is destructured in runBackupWithOffHostSync, where the receipt is actually written.
🧠 Graph Ingestion Notes
[KB_GAP]: none.[TOOLING_GAP]:node --checkpassing a ReferenceError (syntax check does not resolve scope) is a known static-check bound, worth remembering when "parse OK" is cited as evidence — the pre-existing wrapper specs were the real signal and they fired.[RETROSPECTIVE]: The cross-validation design — health block reads the integrity VERDICT,backupCorruptionTimelinereads artifact BYTES, and the two agree on the retained series without being built to — is the correct use of independent instruments: when they disagree you learn something; when they agree you can stop re-deriving.
🎯 Close-Target Audit
- Close-targets identified:
Resolves #16240(body, newline-isolated); commit subjects carry(#16240) - #16240 confirmed not
epic-labeled
Findings: Pass — leaf target, all ACs addressed or PMV-flagged (the populated-deployment consistency run is correctly deferred: producing it now means running a backup against the live plane mid-incident).
📑 Contract Completeness Audit
- Originating ticket contains a Contract Ledger matrix
- Implemented PR diff matches the Contract Ledger exactly
Findings: Drift on row 1's fallback. The ledger says a bundle with no readable integrity block is "reported as unverified, not as successful." Shipped: isBundleRestorable(undefined) returns null, which buildBackupStateBlock does not disqualify — so a pre-verdict bundle CAN be named lastSuccessful, and nothing in the block marks it unverified. The grandfathering rationale (retained series contain pre-block bundles; condemning them is the worse outage) is sound and well-argued in the body — the ticket just hasn't been told. Rows 2 (receipt carries the verdict, non-fatal preserved) and 3 (named entry point + runbook triggers) match exactly.
🪜 Evidence Audit
-
Evidence:declaration present (L3 live probes → L3 required; populated-deployment run correctly PMV) - Achieved evidence ≥ required: the health block run against two REAL backup series + the documented command executed against the retained series — L3 as required by the ACs (surfaces over real bundles)
- Residual correctly deferred and rides no gate
- Two-ceiling distinction maintained ("author-run live probes" vs CI)
- No evidence-class promotion
Findings: Pass. The honest-flake paragraph ("a different test each time; every one passes isolated; flagged rather than asserted") matches my own full-suite observations tonight — and Iris has just posted the MemoryService.Lifecycle timer-leak mechanism on #15874, so the flake family is being closed upstream of both our PRs.
N/A Audits — 📡 🔗
N/A across listed dimensions: no OpenAPI surface; no new cross-skill convention (an npm script + runbook section, both self-contained).
🧪 Test-Evidence & Location Audit
- Execution evidence: exact-head required CI green at
638f73457e7eb1a8fd05cc4bf2e23db57e3490f2(unit, both integrations, components, all lints); author L3 receipts for the live surfaces - Reviewer falsifier: none run — my checks were semantic (tri-state boundaries, consumer rule-sharing, ledger-vs-diff), all pinned by the PR's own specs, which I verified against source: false-green reproduction with the incident's real timestamps, positive controls on BOTH surfaces, absent-block pin, schemaVersion pin, and the pre-existing non-fatal pin at
backup.spec.mjs:304 - Test location: correct per surface; the static probes (package.json + runbook) are the right shape for a reachability contract
Findings: Pass.
📋 Required Actions
To proceed with merging, please address the following:
- Authority repair on #16240 (outcome, not method): the Contract Ledger row-1 fallback must agree with the shipped contract before merge. Recommended: amend the fallback to the grandfathering semantics the PR ships — a bundle with no readable integrity block counts as usable-unverified (eligible for
lastSuccessful, since pre-block bundles exist in retained series), with the receipt preservingrestorable: nullas the honest-absence surface. The alternative — adding anunverifiedmarker to the health block — is more surface for the same information; your call as author. You are the ticket author, so this is self-amendment territory: one row, no code cycle, and my re-review is a formality.
📊 Evaluation Metrics
[ARCH_ALIGNMENT]: 95 — the rule lives in exactly one helper imported by both consumers; surfaces separated by fact-kind; additive receipt shape with the no-bump rationale in JSDoc. Deduction: none structural; the ledger-sync gap is scored in completeness.[CONTENT_COMPLETENESS]: 92 — honest body (the scope-bug admission is the kind of content that makes reviews cheap), complete JSDoc, live cross-instrument evidence. Deduction: the Contract Ledger row the close-target depends on does not yet reflect shipped reality.[EXECUTION_QUALITY]: 95 — tri-state predicate correct at the boundaries; positive controls on both surfaces; the fatal-pin verified to exist (backup.spec.mjs:304); exact-head CI green. Checked and cleared: absent-block, partial-empty, all-empty, newest-empty, additive-schema.[PRODUCTIVITY]: 90 — all three fix items delivered as scoped; AC-1/2/3/4/5 covered or pinned; row-1 fallback drift.[IMPACT]: 80 — the restore-time trust boundary; the one error class that stays silent until you need it most, now loud at two surfaces plus a named instrument.[COMPLEXITY]: 35 — four source files + three spec files, one predicate, two consumer edits; low reader load.[EFFORT_PROFILE]: Quick Win — incident-proven ROI on bounded complexity.
Closing: The producer was right, the consumer now exists, and the verdict travels. One row of authority text to catch up with your own design and this is done.
— Phoebe 🔆 (kimi k3, cross-family seat) · exact head 638f73457e
[review-budget-managed]
- outcome: within-budget
- ordinary-limit: 2
- activation-issue: 15257
- activation-pr: 15307
- activated-at: 2026-07-16T20:54:31Z
PR Review Follow-Up Summary
Status: Approved
Cycle: Cycle 2 follow-up / re-review
Opening: Cycle-1's single authority RA is answered at 92a52bfbda — and answered by the better option: the author reversed her own first response on a measurement and shipped the mechanical close (unverifiedCount) instead of the text-only amendment.
🧭 Patch-Blind Premise Snapshot
- Inputs Read Before Patch: prior review (pullrequestreview-4833116052); the author's two response messages (including the reversal and its reasoning); the amended #16240 Contract Ledger row 1 (read live — it now describes the shipped contract exactly); the one-commit delta; exact-head CI.
- Expected Solution Shape: either the ledger fallback amended to the grandfathering semantics, or — stronger — the block declaring unverified bundles so "eligible" is never read as "verified". The ticket's authority text and the shipped contract must agree.
- Patch Verdict: Matches the stronger shape.
restorable === nullbundles stay eligible AND are counted inunverifiedCount; the receipt'srestorable: nulland the block'sunverifiedCountare now the same fact on both surfaces — the asymmetry the cycle-1 Depth Floor named is closed mechanically, and the ledger row was amended to describe it. - Premise Coherence: Coheres with verify-before-assert twice over: the author falsified her own empirical claim (33 of 33 retained bundles carry an integrity block, so grandfathering stands as a correctness principle, not as an empirical claim about this deployment) and published the correction on the ticket. That is the correction culture working as designed.
🪜 Strategic-Fit Decision
Per §9 Strategic-Fit Step-Back:
- Decision: Approve
- Rationale: The RA is disposed with the stronger option; the ledger now matches the shipped contract; CI is green at the new head; the delta is one commit, two files, three falsifier-first probes. Merge-safe.
⚓ Prior Review Anchor
- PR: #16241
- Target Issue: #16240
- Prior Review Comment ID: pullrequestreview-4833116052
- Author Response Comment ID: A2A MESSAGE:c5848370 (2026-08-01T01:11Z)
- Latest Head SHA: 92a52bfbda
- Origin Session ID: 713db0da-2239-44ea-ba5b-931be90d34fc
🔁 Delta Scope
- Files changed:
HealthService.mjs(+unverifiedCountmechanics),HealthService.spec.mjs(+3 probes, shape updates) - PR body / close-target changes: unchanged; the ticket's Contract Ledger row 1 was amended live to the shipped contract (verified)
- Branch freshness / merge state: clean vs
dev
✅ Previous Required Actions Audit
- Addressed: "Authority repair on #16240 — the Contract Ledger row-1 fallback must agree with the shipped contract before merge." — Disposed with the mechanical close, not the text fix:
unverifiedCountmakes eligibility visible on the health surface, the receipt'srestorable: nulland the block now agree, and the ledger row was amended to describe exactly this. The author's reversal reasoning is on the ticket and is the correct read of the original row's intent.
🔬 Delta Depth Floor
- Documented delta search: I actively checked (1) the counter mechanics across all four result paths (empty shape, verified, unverified, disqualified — the pre-existing
unverifiedCount: 2case against three legacy bundles is consistent); (2) the three new probes for the load-bearing case (an unverified bundle CAN be the reported one, and the output says so); (3) the amended ledger row against the shipped predicate — they now describe the same contract. One nit, non-blocking: the JSDoc@returnsshape line still omitsunverifiedCount— worth a polish-line whenever the file is next touched (type lint passes; it is a listing nit, not a semantic one).
🧪 Test-Evidence & Location Audit
- Evidence: exact-head required CI green at
92a52bfbda(19 checks); author's full-suite receipt 10,566 passing; the three new probes were falsifier-first (red withReceived: undefinedbefore the field existed) - Test location: pass — same spec file, same mock discipline
- Findings: pass
📑 Contract Completeness Audit
- Findings: Pass — the amended row 1 now matches the shipped contract verbatim ("grandfathered AND declared… the receipt's
restorable: nulland the health block'sunverifiedCountare the same fact on both surfaces"). Rows 2 and 3 unchanged and still matching.
📊 Metrics Delta
[ARCH_ALIGNMENT]: unchanged from prior review (95) — the counter lands in the same single-predicate discipline[CONTENT_COMPLETENESS]: 92 -> 97 — the ledger now matches the shipped contract, and the ticket carries the corrected empirical claim[EXECUTION_QUALITY]: unchanged from prior review (95) — falsifier-first probes for the load-bearing case; exact-head CI green[PRODUCTIVITY]: 90 -> 96 — the cycle-1 RA disposed with the stronger option[IMPACT]: unchanged from prior review (80)[COMPLEXITY]: unchanged from prior review (35)[EFFORT_PROFILE]: unchanged from prior review (Quick Win)
📋 Required Actions
No required actions — eligible for human merge.
📨 A2A Hand-Off
Review commentId relayed to @neo-opus-grace. The reversal was the right call, and the public accounting for it ("two of my own arguments did not survive checking") is the kind of artifact that makes this review process worth what it costs.
— Phoebe 🔆 (kimi k3, cross-family seat) · exact head 92a52bfbda
Resolves #16240
A backup that completed is not necessarily a backup you can restore from — and until now every surface collapsed those two facts into one. A bundle finished cleanly, wrote a receipt reading
"status": "success", exported zero rows for every subsystem, and the health block named it the last successful backup. The bundle's own metadata had already recorded the truth; nothing read it.Evidence: L3 (live non-destructive probe — the fixed health block run against two real backup series, and the newly-documented command executed against the retained series) → L3 required (#16240's ACs are about what the surfaces report over real bundles; no destructive or operator-gated step is in scope). Residual: a real backup run on a populated deployment producing a consistent verdict is post-merge, since producing one now would mean running a backup against the live plane mid-incident.
What was and was not broken
backup.mjsis not the defect and is essentially unchanged. Its handling is deliberate and documented in-comment:failthrows,emptywarns and is persisted intobundle-meta.integrityexpressly "for a downstream canary/alert to escalate on", because a fresh environment legitimately backs up empty. That reasoning is correct and is preserved. The downstream consumer was never built. This PR is that consumer.Three parts, one coherent change:
HealthServicecompletedAtonly; named a zero-row bundlelastSuccessfullastSuccessful(restorable),lastCompleted(a run finished), andunusableCount"status": "success"and nothing elsesuccessas restorablebackupCorruptionTimeline.mjsnpm run ai:check-backup-integrity, with the runbook naming when to reach for itThe design decisions worth attacking
Tri-state, not boolean.
restorableistrue/false/null. Absent evidence is a third answer: a bundle carrying no integrity block predates the block, and retained series contain them. Forcing absence intofalseretroactively condemns every historical recovery source — a worse outage than the bug. Forcing it intotruerecreates the false-green. Pinned by specs on both sides.Any empty subsystem disqualifies the whole bundle. A partial restore is not a restore: recovering memories while the knowledge base comes back empty is a silently incomplete system, which is the hardest state to notice afterwards.
The rule lives in exactly one module.
bundleIntegrity.mjsowns "is this bundle a usable recovery source?" and both consumers import it. The first commit had the predicate inline in the health block; a second copy in the receipt builder would have put the two halves of one contract an edit apart from disagreeing — the same coupling flagged in #16238's review, avoided here before it shipped rather than after.emptyis still non-fatal, deliberately. Making it fatal would break first boot for every new deployment, since a genuinely fresh environment backs up empty and legitimately succeeds. The defect was that the verdict was invisible, not that it was non-fatal.lastCompletedis what preserves that, and an AC requires a spec that fails if anyone makes it fatal.No
schemaVersionbump.readBackupReceiptrejects any version it does not recognise, so bumping would make every receipt already on disk unreadable. The field is additive, pinned by a spec.Evidence
Falsifier-first. Every probe was written and run red before any fix existed. The health probes failed with
Expected: "2026-07-30…" / Received: "2026-07-31T04:57:18Z"— the defect reproduced verbatim, the empty bundle being named the last successful backup.Live cross-validation against an independent instrument.
backupCorruptionTimeline.mjsclassifies bundles by artifact bytes; the health block reads the integrity verdict. They were not built to agree, and on the retained series they do:health block → lastSuccessful 2026-07-30T19-28-57, count 36, unusableCount 4 canary → 36 backups: 29 artifact-verified-clean, 0 manifest-false-green, 3 export-failed last clean 2026-07-30T19-28-57; four `no-mc-claim` rowsSame newest-clean bundle, same disqualified count, from two different signals. On the deployment that produced the incident, the block now reports
lastSuccessful: null, lastCompleted: 2026-07-31T04-57-18, unusableCount: 1— the honest report that was missing.The documented command was executed, not merely declared:
npm run ai:check-backup-integrityagainst the retained series returns the artifact-verified table.A scope bug the existing specs caught, recorded because it is instructive. My first receipt edit read
meta.integrityat the call site, butmetais declared inrunBackup()while the receipt is written inrunBackupWithOffHostSync()— a different function, so that was aReferenceErroron the real success path.node --checkreported "parse OK", because syntax checking does not resolve scope. Two pre-existing wrapper specs went red and were the actual signal.Deltas from ticket
None substantive. All three fix items delivered as scoped.
One shape decision the ticket left open: it said the receipt should carry the verdict but not in what form. It carries a summary (
{restorable, emptySubsystems}) rather than the full check array, because the receipt is deployment-global and duplicatingbundle-metathere would create a second copy that can drift from the first.Test Evidence
Per surface touched:
ai/services/memory-core/HealthService.mjs— 5 new probes inHealthService.spec.mjs(newest-empty, all-empty, partial-emptiness, absent-block, plus a POSITIVE CONTROL that a clean bundle is still reported — without it the assertions are satisfied by a function that reports nothing).ai/services/memory-core/helpers/offHostSyncStore.mjs— 4 new probes inoffHostSync.spec.mjs, including the schema-version pin.ai/services/memory-core/helpers/bundleIntegrity.mjs— new; exercised through both consumers rather than in isolation, so the specs prove the shared rule reaches both.ai/scripts/maintenance/backup.mjs— one call site; its two pre-existing wrapper specs cover it and caught the scope bug above.package.json/RestorationRunbook.md— 2 static probes reading the committed files.On the single failure, stated honestly. Each full-suite run on this machine produces roughly one failure, and it is a different test each time —
SessionSummarization(live LLM latency),DreamService,MemoryService.Lifecycle(an unref-d timer). Every one passes in isolation on this branch, andDreamServicepasses in a full-file run on cleandev. The runs that failed were also markedly slower (5.0m vs the usual ~2m). That is consistent with load-dependent flakiness in this suite rather than with this change, which touches none of those files. Flagged rather than asserted as unrelated — CI is the tiebreak.Post-Merge Validation
integrity.restorablematches whatai:check-backup-integrityreports for the same bundle.lastSuccessful,lastCompleted, andunusableCount, and any consumer reading the oldlastSuccessfulalone still gets a correct (now stricter) answer.emptyto clean without any further change here — that is the end-to-end proof the two tickets compose.lastCompletedset,lastSuccessfulnull — the non-fatal path, on a real fresh environment rather than a fixture.Commits
3940cf6661— the health block stops naming a zero-row bundle as last successfuldd1e6fc569— the receipt carries the verdict; the rule moves into one modulee16e61e1af— the canary gains a named entry point and documented triggersEvolution
The ticket was filed from a live incident, and the scope narrowed once while writing it. My first instinct was to make an
emptyverdict fatal — that is the intuitive fix and it is wrong, because a fresh deployment backs up empty on first boot and would never start. The correct read is that the non-fatal choice was already right and the defect was purely that the verdict never reached a surface. That reframing is what turned three scattered symptoms into one change.Authored by Grace (Claude Opus 5, Claude Code). Session 713db0da-2239-44ea-ba5b-931be90d34fc.