PR Review Summary

Status: Request Changes

🪜 Strategic-Fit Decision

Per §9 Strategic-Fit Step-Back:

• Decision: Request Changes
• Rationale: The premise, placement, and shape are all right — this is the split the ticket prescribed, built on the existing bundleIntegrity authority rather than a second predicate. One delivered-scope correctness defect blocks it: the walk returns the newest bundle's priorStateEvidence, so a populated-but-incomplete bundle sitting below an empty/torn one becomes invisible to the --initialize interlock. That is the data-destroying direction #16567 exists to forbid, so it is not Approve+Follow-Up (unresolved correctness) and not Drop+Supersede (the premise is sound and the repair is contained).

Peer-Review Opening: This is a careful piece of work, and the part I want to name first is that you resisted the obvious fix. The ticket hands you a trap — tighten the boolean — and you instead consumed the existing all-substrate bundleIntegrity rule, moved RECOVERY_SUBSTRATES so producer and reader cannot drift onto different populations, and fail-closed the partial-census third state to null. The BUNDLE_INCOMPLETE fall-through to older complete history is better than what the ticket asked for. One defect below, in the layer the two-question model did not reach.

🧭 Patch-Blind Premise Snapshot

• Inputs Read Before Patch: #16567 body + its "Note on this ticket's history" correction; the changed-file list; restore.mjs / redeployPreflight.mjs / bundleIntegrity.mjs at exact head 2de59e97c2; the #16348 fall-back-past-unusable-newest suite as sibling precedent; ADR-0019 (§critical_gates #10, mandatory before any ai/ touch); #16563 as the producer-side context for how kb:0 bundles arise.
• Expected Solution Shape: Two independently computed facts — priorStateEvidence (ANY populated collection, must not tighten) and recoverySourceAuthorized (per-substrate completeness) — with the incomplete-bundle refusal carrying no --initialize instruction. Must NOT hardcode the substrate list where a fourth collection could silently pass, and a skipped/unknown census must fail closed rather than resolve to permission. Test isolation: real bundle trees or the pure validateFn seam, template-not-overlay imports (ADR-0019 C3).
• Patch Verdict: Improves the expected shape on declaration, contradicts it on aggregation. Improves: authorization delegates to isBundleRestorable instead of a new count-based predicate; RECOVERY_SUBSTRATES moves into bundleIntegrity.mjs and is re-exported from backup.mjs, so the census population has one owner; the partial-census null is genuinely fail-closed. Contradicts: priorStateEvidence is computed per bundle in probeBundle but consumed per root by redeployPreflight, and nothing reconciles the two — restore.mjs:1018 newest ??= verdict latches the newest candidate and :1027 returns {...newest}, so the walk discards the prior-state fact of every older bundle it passed over.
• Premise Coherence: Coheres with verify-before-assert — the ## Evolution section records that the first sketch derived authorization from embedding counts and that the engine-first scan replaced it with the shipped bundleIntegrity authority. That is the ask-whether-the-engine-already-solved-it discipline applied without being told, and it is why this PR consumes a primitive instead of growing a rival one.

🕸️ Context & Graph Linking

• Target Epic / Issue ID: Resolves #16567
• Related Graph Nodes: #16563 (producer side — kb:0 bundles, 5 consecutive observed), #16510 / #16520 (per-collection facts), #16348 (the fall-back-past-unusable-newest walk this builds on), #16568 / #16569 (same well-formed-pass-about-the-wrong-subject family)
• Origin Session ID: be6b6eb4-dabe-4deb-9924-7c92335c69ff

🔬 Depth Floor

Challenge: The two-question model is applied at probeBundle (per candidate) but consumed at evaluateRedeployPreconditions (per backup root), and the walk between them only ever forwards one candidate's answer.

restore.mjs:963-1027:

let examined = 0,
    newest   = null;
// ...
if (verdict.recoverySourceAuthorized) { return {...verdict, skipped, examined} }   // :982
// ...
newest ??= verdict;                                                                // :1018
skipped.push({bundleName, code: verdict.code, reason: verdict.reason});            // :1019
// ...
return {...newest, skipped, examined}                                              // :1027

newest ??= latches the first candidate examined. skipped rows carry only {bundleName, code, reason} — not priorStateEvidence. So on exhaustion the root-level verdict reports the newest bundle's prior-state fact and silently drops every older one's.

Failure ordering (each element observed in production independently):

Both codes are in CONTINUE_ELIGIBLE_BUNDLE_VERDICTS, so the walk passes the empty newest, reaches the populated older bundle, declines to return it (not authorized), exhausts, and returns the newest's priorStateEvidence: false. redeployPreflight then computes bundleProvesPriorState === false, omits 'a bundle containing prior-state rows' from priorEvidence, and with no marker and no primary volume, --initialize proceeds against a plane that had a real backup.

This is a regression, not a pre-existing gap. Before this PR the walk returned early on verdict.restorable, and restorable was rowTotal > 0 — so that same older bundle satisfied it, early-returned true, and the interlock held. Narrowing the early-return to recoverySourceAuthorized is correct for authorization and is exactly what removed prior-state's only path out of the walk.

Two reasons I do not think this is a corner case. The #16348 suite is titled "falls back past an unusable newest bundle" and its lead test is "the live incident" — an unreadable newest bundle above a good one is an observed shape on this fleet. And #16563 records kb:0 bundles as the observed norm ("5 consecutive observed"), which is precisely the class that now returns BUNDLE_INCOMPLETE. The defect is the intersection of two independently-observed production states, not a constructed one.

The same path also fires when maxBundlesExamined caps the walk before it reaches a populated candidate.

Suggested shape (yours to choose): carry prior-state as a root-level disjunction over everything examined — e.g. accumulate sawPriorState ||= verdict.priorStateEvidence in the loop and spread it over the exhaustion return — rather than inheriting it from newest. The skipped rows are the natural place to carry the per-bundle fact if you want it observable in the deploy log.

Empirical isolation test (§5.1). I could not execute this on my seat: ai/** unit specs run under the unit-brain* projects, and this checkout has no Brain tier (chromadb absent), so playwright.config.unit.mjs skips them and a direct import fails at ChromaManager. My producer-side claim is therefore an exact-head source read, not an execution — named as such. The consumer-side arithmetic in evaluateRedeployPreconditions is pure and reads unambiguously. A spec that decides it, with a positive control so a false cannot be mistaken for a driver that never reached the branch:

const incompleteMeta = {
    embeddingAdvisories: [],
    embedding          : {counts: {kb: 0, memories: 32462, summaries: 1777}},
    integrity          : [
        {bundleCount: 0,     sourceCount: 0,     status: 'empty', subsystem: 'kb'},
        {bundleCount: 34239, sourceCount: 34239, status: 'pass',  subsystem: 'mc'},
        {bundleCount: 12,    sourceCount: 12,    status: 'pass',  subsystem: 'graph'}
    ],
    streamedCounts: {memories: 32462, summaries: 1777}
};
const emptyMeta = {
    embeddingAdvisories: [],
    embedding          : {counts: {kb: 0, memories: 0, summaries: 0}},
    integrity          : ['kb', 'mc', 'graph'].map(subsystem => ({bundleCount: 0, sourceCount: 0, status: 'empty', subsystem})),
    streamedCounts: {}
};
// Drive verifyLatestBackupRestorable with validateFn: async bundleRoot => bundles[path.basename(bundleRoot)],
// then feed the verdict into evaluateRedeployPreconditions with
// {markerPresent: false, initializeRequested: true, primaryVolumeState: null}.
// POSITIVE CONTROL — incomplete bundle alone: expect proceed === false (passes today).
{'backup-2026-08-06T01-00-00': incompleteMeta}
// PROBE — empty newest above populated older: expect proceed === false (I read this as PROCEED_INITIALIZING).
{'backup-2026-08-06T02-00-00': emptyMeta, 'backup-2026-08-05T01-00-00': incompleteMeta}

If the probe refuses, my read is wrong and I withdraw the action — say so and I will yield on it.

Rhetorical-Drift Audit (per guide §7.4):

• PR description: framing matches the diff, with one exception carried into Required Actions — AC-1 claims the interlock is "proven unchanged", and the proof covers the single-bundle case only.
• Anchor & Echo summaries: drift flagged. restore.mjs:881 documents the function's @returns as "priorStateEvidence remains independently true for an incomplete non-empty bundle, preserving the initialization interlock without authorizing recovery." That is true of probeBundle's per-candidate verdict and not of verifyLatestBackupRestorable's root-level return, which is the contract that JSDoc block governs. The prose asserts the property at the layer where it does not hold, which is why the gap reads as covered.
• [RETROSPECTIVE] tag: N/A — none claimed.
• Linked anchors: #16510 / #16520 / #16348 each establish what they are cited for.

Findings: One drift flagged (the @returns contract above); it is the same defect stated in prose, so fixing the aggregation and the sentence together closes both.

🧠 Graph Ingestion Notes

• [KB_GAP]: None. The primitive was understood — authorization delegates to isBundleRestorable rather than re-deriving completeness, which is the engine-first move.
• [TOOLING_GAP]: ai/** unit specs are unreachable on a base install: playwright.config.unit.mjs skips every unit-brain* project when the Brain tier is absent, and it prints an advisory rather than failing. A reviewer without npm run install-brain cannot run a falsifier against any ai/ change and can only read source — worth knowing when routing ai/ reviews to a seat.
• [RETROSPECTIVE]: The durable lesson is a layer mismatch, not an arithmetic slip. A per-item fact and a per-collection consumer look identical at both endpoints; the walk between them is where the fact silently narrows. When a verdict function grows a loop, every field in its return type needs asking "is this the collection's answer, or the first element's?" — restorable survived that transition because rowTotal > 0 happened to also be the early-return predicate, and priorStateEvidence did not.

🎯 Close-Target Audit

• Close-targets identified: Resolves #16567 (newline-isolated in the PR body, single leaf)
• For each #N: #16567 carries bug + ai, no epic label

Findings: Pass.

📑 Contract Completeness Audit

• Originating ticket contains the consumed-surface prescription (#16567 §"The real repair", item 2 — restorable splits into two named fields or gains a role parameter)
• Implemented diff matches, with the deltas declared in the PR body's ## Deltas from ticket

Findings: Pass. The BUNDLE_INCOMPLETE addition and the bundleIntegrity.mjs census move are both declared rather than silent, and both are improvements on the prescription. restorable surviving as a compatibility alias of recoverySourceAuthorized is the right call given the audited single caller, and the JSDoc warning against reading it as prior-state evidence is the correct guard.

N/A Audits — 🪜 📡 🔗

N/A across listed dimensions: close-target ACs are fully unit-observable (no sandbox-unreachable runtime surface), no openapi.yaml touched, and no skill / convention / MCP-tool surface introduced.

🧪 Test-Evidence & Location Audit

• Execution evidence: exact-head required CI green at 2de59e97c2 — 27/27 SUCCESS, verified live at review time.
• Reviewer falsifier: named concern, not executed on this seat. Concern: priorStateEvidence narrows to the newest candidate across the walk. Blocked by the absent Brain tier (above); spec supplied for the author to run.
• Test location: added coverage sits beside its subject in test/playwright/unit/ai/scripts/maintenance/ and .../ai/services/memory-core/; the validateFn seam is used for pure-shape arms and real temp trees for structural ones, which is the right split.

Findings: Author evidence is strong for the single-bundle dimension and absent for the multi-bundle one — the diagonal controls listed in ## Test Evidence all vary the bundle's content, none vary position in the walk. That is the axis the defect lives on.

Verified and cleared while looking for related failures, so it is on record that these were checked rather than assumed:

• AC-4's caller census holds. verifyLatestBackupRestorable has exactly one production caller, redeployPreflight.mjs:14/395; the remaining non-test hits are JSDoc references (activationReceipt.mjs:170, configBase.mjs:2433) and its own definition. Positive control: summarizeBundleIntegrity returned 10 hits on the same probe, so the search finds callers when they exist.
• isBundleRestorable's widened null domain is safely consumed. It has a second production consumer AC-4 did not name — HealthService.mjs:735 — and that consumer is explicitly tri-state (false → unusableCount, null → unverifiedCount), so the additional null cases land in "unverified" rather than being coerced to "unusable". Correct, and the spec was updated to match.
• The census population cannot drift. RECOVERY_SUBSTRATES is ['kb','mc','graph'] and runBackup writes integrity rows for exactly those three (backup.mjs:601/607/614). A fourth bundle subdirectory (concepts, trajectories) carries no integrity row, so the !RECOVERY_SUBSTRATES.includes(...) → null arm cannot be tripped by ordinary bundles.
• ADR-0019 scan: clean. No A-group re-derivation, no hasEnvValue, no config export or alias (B1/B2), no defensive ?. on an AiConfig read, no runtime write to the singleton (B4), no C1 competing resolver. maxBundlesExamined is read as a default parameter off AiConfig, evaluated per call — the sanctioned read-at-use-site form.

📋 Required Actions

To proceed with merging, please address the following:

• Preserve prior-state evidence across the whole walk. verifyLatestBackupRestorable must report prior state as a disjunction over every candidate it examined, not inherit it from newest (restore.mjs:1018/1027). Today an empty or torn newest bundle above a populated-but-incomplete older one returns priorStateEvidence: false, and --initialize proceeds against a plane that has a real backup. Include an arm that varies position in the walk rather than bundle content, and extend AC-1's interlock proof to the multi-bundle case.
• Correct the @returns contract at restore.mjs:881 so it states the prior-state property at the layer that actually holds it (per-candidate), or — preferably — make the root-level return honour the sentence as written.

📊 Evaluation Metrics

• [ARCH_ALIGNMENT]: 88 — the split lands where the ticket put it, authorization delegates to the existing bundleIntegrity SSOT instead of a rival predicate, and RECOVERY_SUBSTRATES moves to one owner consumed by both producer and reader. 12 deducted because the two-question model stops at probeBundle: no layer owns reconciling a per-candidate fact with a per-root consumer.
• [CONTENT_COMPLETENESS]: 82 — JSDoc is genuinely above bar; the preserved interlock rationale and the CONTINUE_ELIGIBLE_BUNDLE_VERDICTS fail-closed note both explain why, not just what. 18 deducted for the @returns sentence asserting a per-candidate property as a function-level guarantee — documentation that describes the intended behavior rather than the shipped one.
• [EXECUTION_QUALITY]: 62 — the per-bundle logic, the tri-state null, and the BUNDLE_INCOMPLETE fall-through are all correct and carefully bounded. Capped by one defect on the interlock the ticket names as the data-destroying direction, reachable from an observed production ordering.
• [PRODUCTIVITY]: 78 — AC-2 through AC-6 are met and independently checked. AC-1 is met for the single-bundle case and unmet for the multi-bundle one, which is the case its own "interlock proven unchanged" language claims.
• [IMPACT]: 90 — governs whether a container-affecting redeploy may proceed and whether --initialize may destroy a plane. The failure mode is irreversible data loss.
• [COMPLEXITY]: 80 — nine files across producer, reader, and two consumer surfaces, with a three-valued authorization domain and a backwards walk whose continue-eligibility is itself a safety contract.
• [EFFORT_PROFILE]: Heavy Lift — a consumed-surface change on a safety interlock, with per-branch diagonal controls rather than a happy-path test.

The thing I would most like on the record: you were handed a ticket whose obvious fix is a trap, and you not only avoided the trap, you found that the engine already owned the completeness question and consumed it. The defect I am blocking on is one layer above where you were working, and it is the layer the ticket's own framing does not mention — it says "two questions", and the walk is where a per-bundle answer becomes a per-root one.

⚖️ 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