PR Review Summary

Status: Approved

🪜 Strategic-Fit Decision

Per §9 Strategic-Fit Step-Back:

• Decision: Approve
• Rationale: Correct, well-placed Brain-substrate operational-safety fix that reuses proven infra, is ADR-0019-clean, meets all four ACs, and is proven at head at both L1 (unit) and L2 (integration). No delivered-scope defect and no deferred correctness — the observations below are cleanliness/accuracy notes, not scope transfer, so neither Request Changes nor A+FU applies.

Peer-Review Opening: Solid fix, Phoebe. Turning a silent mid-restore stall (the worst failure shape on a destructive op — fresh host, cold embedder) into a fail-fast refusal that names the provider + remediation is exactly right, and reusing the proven #14182 write-canary rather than rolling a new probe is the correct instinct (no duplication). Placement, ADR-0019 compliance, and coverage all check out — a couple of non-blocking notes below, one of which is that your evidence is actually stronger than you claimed.

🧭 Patch-Blind Premise Snapshot

• Inputs Read Before Patch: ADR-0019 (rule-10 read-gate for any ai/ config touch); ticket #15640 (crash = stall deep in first embed batch; ACs; the "reuse the canary" architectural note); base runRestore flow order (restore.mjs — mode/bundle validation vs services boot vs writes); buildEmbeddingWriteCanaryBlock (HealthService); BackupRestoreWipe.integration.spec.mjs + test.yml suite mapping.
• Expected Solution Shape: An embedding-provider preflight in restore.mjs, after bundle validation + service boot but before any write, reusing the existing write-canary (not a new probe), refusing with provider + remediation, plus a documented --skip escape and unit coverage via a stubbed provider. Must NOT re-implement/thread/mutate AiConfig (read the canary, which owns the config read).
• Patch Verdict: Matches / improves. Preflight is inserted right after Promise.all([…ready()]) and before the occupancy check + emptyDir writes — and since base-flow validation (Unknown mode at :142, validateBundle at :158) precedes services, a torn bundle still errors as a torn bundle, not as a cold embedder (correct fail-fast ordering). Improves on the ticket in two ways it names honestly: 30s budget (the proven cold-embedder canary budget, not the ticket's sketched 10s) and an embedText test seam mirroring the canary's own.
• Premise Coherence: Coheres — verify-before-assert: the fix converts an unobservable stall into a named, evidenced refusal (the whole point is making the failure legible); friction→gold: reuses the #14182 canary substrate rather than duplicating a probe (ADR-0019 C2 avoided).

🕸️ Context & Graph Linking

• Target Epic / Issue ID: Resolves #15640
• Related Graph Nodes: #15639 / #15641 (backup-parity trio siblings) · #14182 (cold-embedder canary 5s→30s — the reused budget) · #10871 (backup/restore parity) · ADR-0019 (AiConfig SSOT read-gate) · ai/services/memory-core/HealthService.mjs (buildEmbeddingWriteCanaryBlock) · BackupRestoreWipe.integration.spec.mjs

📜 ADR-0019 / AiConfig SSOT Audit

(§critical_gates rule 10 — mandatory for any ai/ config touch. ADR-0019 read before this verdict.)

V-B-A'd the diff against the §3 antipattern catalog and §5 sanctioned forms:

• No config re-derivation (A1) / re-implementation (A2–A9): the diff reads no config directly — it calls buildEmbeddingWriteCanaryBlock, which owns the cfg.vectorDimension / provider read at its use site. No process.env reads, no hasEnvValue, no formulas.
• No indirection around the SSOT (B1/B2/B5): embedText is a test seam (function), skipEmbedPreflight/timeoutMs are an operator flag + literal — none are AiConfig values threaded into a consumer's config.
• No runtime writes to AiConfig (B4, safety-critical): zero AiConfig.<path> = … assignments; the preflight is read-only. Tests inject a fake embedText, never mutate the shared singleton.
• ?. is on the injected logger (logger.warn?.), not on an AiConfig read — not a B3 violation.
• C1 (Neo imports only in entrypoints): restore.mjs IS a thread-entrypoint (import.meta.url CLI guard); importing the canary-builder from HealthService is service reuse, not a non-entrypoint Neo import.

Findings: Clean — no antipattern. The fix consumes the embedding config the sanctioned way (through the service that owns the read).

🔬 Depth Floor

Challenge OR documented search (per guide §7.1):

• Favorable finding — evidence is stronger than claimed: the PR body frames the integration path as "L2 required … Residual: integration run on this exact head." But BackupRestoreWipe.integration.spec.mjs:105 calls runRestore({bundleRoot, mode:'merge', logger}) with no embedText/skip seam, so it exercises the real preflight against the deployed openAiCompatible fixture — and integration-unified (which runs test/playwright/integration/ per test.yml) is green at facf802e8d (4m5s). So L2 is effectively already achieved at head, not a pending residual. Non-blocking, but you could upgrade the evidence line to L2 (integration-unified green at head) and tick the Post-Merge box.
• Scope note (non-blocking): restoreFlatDir/restoreFlatFile carry a cosmetic colon re-alignment (mode/target/source) unrelated to the preflight — behavior-preserving, but it's diff noise in two destructive-guard call sites; ideally its own change (or pre-aligned by hand) to keep the diff scoped to the fix.
• Verified, no concern: the refuse path is robust to a throwing probe (spec 3's embedText throws → the canary catches and returns an error block → the preflight throws the nice wrapped message), and healthyEmbedText threaded into the flow specs that actually reach the preflight is correct (the early-throw specs reach validateBundle first, so their seam is belt-and-suspenders — harmless).

Rhetorical-Drift Audit (per guide §7.4):

• "reuses the healthcheck write-canary probe" — true (buildEmbeddingWriteCanaryBlock, same dimension validation).
• "30s budget matches the proven cold-embedder canary" — true (#14182), and the Deltas section flags the deviation from the ticket's 10s honestly.
• "refuses BEFORE any write" — true; preflight precedes the occupancy check and all emptyDir/write paths. The only drift is the under-claim above (safe direction).

Findings: Pass — no over-claim; one favorable under-claim on the L2 evidence.

🧠 Graph Ingestion Notes

• [KB_GAP]: None.
• [TOOLING_GAP]: None — the reuse of buildEmbeddingWriteCanaryBlock is the tooling working as intended (one probe, two consumers: healthcheck + restore).
• [RETROSPECTIVE]: The pattern worth remembering: a restore that re-embeds through a live provider has a hidden environmental prerequisite that only surfaces mid-write. Moving that check to a fail-fast preflight — reusing the existing canary rather than duplicating it — is the right shape, and it generalizes to any re-ingest-through-a-provider operation (the sibling backup-parity trio should mirror it).

🎯 Close-Target Audit

• Close-targets identified: Resolves #15640 (newline-isolated in PR body; both commits reference (#15640)).
• #15640 is labeled enhancement / ai — not epic. Valid delivered leaf; two clean commits (fix + test); no stacked-guard comment.

Findings: Pass.

📑 Contract Completeness Audit

New operator-facing surface: the --skip-embed-preflight CLI flag + runRestore's skipEmbedPreflight/embedText params.

• The flag/params are documented at every layer: module JSDoc bullet, runRestore @params, preflightEmbeddingProvider JSDoc, the CLI usage string, and parseArgs coverage (threading + --skip-embed-preflight=yes rejection).
• Contract matches the ticket's Fix section (escape hatch, documented + logged). No formal Contract Ledger is warranted for a single maintenance-script escape-hatch flag.

Findings: Pass — surface fully documented; no drift.

🪜 Evidence Audit

Close-target ACs span refuse-before-write (unit) + the real-provider integration path (in-container).

• Evidence: line present (L1 → L2 required).
• L1 achieved: 6 targeted preflight specs (empty-vector, dimension-divergence, provider+remediation naming, healthy pass-through, escape-hatch, parseArgs threading) + flow specs threaded through a healthy seam.
• L2 achieved at head (stronger than the body's "residual" framing): BackupRestoreWipe drives runRestore seam-free → real preflight against the deployed openAiCompatible fixture; integration-unified green at facf802e8d. Deployment causality holds — the receipt is reachable from this exact head via the CI integration suite.
• No evidence-class inflation (if anything, deflation).

Findings: Pass — both levels proven at head; recommend upgrading the body's L2 framing from residual to achieved.

🧪 Test-Evidence & Location Audit

• Execution evidence: exact-head required CI green at facf802e8d (all checks — unit 9m13s, integration-unified 4m5s, components, 4× lint, lint-pr-body, CodeQL; verified via gh pr checks). Author receipt: unit-brain "maintenance/restore|maintenance/backup" → 76 passed.
• Reviewer falsifier: N/A — correctness (flow ordering, ADR-0019, the L2 path) resolved by reading base flow order + BackupRestoreWipe + test.yml, not a runtime falsifier.
• Test location: pass — new spec at canonical test/playwright/unit/ai/scripts/maintenance/restore-embed-preflight.spec.mjs, mirroring the script; flow-spec edits in the existing sibling.

Findings: Pass.

N/A Audits — 📡 🔗

N/A across listed dimensions: no ai/mcp/**/openapi.yaml touch (📡); no skill/convention/primitive introduced — the --skip-embed-preflight flag is self-documented in the CLI usage string, no cross-skill reference gap (🔗). ai:structure-map N/A: no new source-file placement — the preflight lives in its ticket-prescribed home (restore.mjs) and the new test is in the canonical mirror dir.

📋 Required Actions

No required actions — eligible for human merge.

(Optional, author's discretion — not return-cycle items: upgrade the PR-body L2 evidence line from "residual" to "achieved at head" (integration-unified green); split or hand-pre-align the cosmetic colon re-alignment in restoreFlatDir/restoreFlatFile to keep the diff scoped.)

📊 Evaluation Metrics

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

• [ARCH_ALIGNMENT]: 96 — Correct fail-fast placement (after bundle validation + service boot, before writes), reuses the proven canary (no duplication, ADR-0019 C2 avoided), ADR-0019-clean, clean test-seam DI. −4: unrelated cosmetic alignment churn in two functions.
• [CONTENT_COMPLETENESS]: 95 — Thorough JSDoc (module bullet + function @params + why) and a complete Fat Ticket with honest Deltas. −5: the L2 evidence line under-claims what CI already proves at head.
• [EXECUTION_QUALITY]: 96 — Correct, robust (canary catches a throwing probe → wrapped actionable error), all ACs met, well-covered; sound 30s-budget judgment. −4: minor test over-threading + the raw-canary-throw edge (both harmless).
• [PRODUCTIVITY]: 100 — All four ACs met (refuse-before-write with provider+remediation; healthy pass-through unchanged; escape hatch honored+logged; unit specs cover refuse + pass-through), and it reused proven substrate rather than duplicating.
• [IMPACT]: 62 — Brain-substrate operational safety: converts a silent, unactionable mid-restore stall on a destructive disaster-recovery path into a fail-fast named refusal. Operationally important; not core-architecture-changing.
• [COMPLEXITY]: 40 — +160/−17 across three files, but the logic is a compact preflight + wiring + flag; the bulk is test scaffold. One flow-ordering subtlety and the canary-reuse seam.
• [EFFORT_PROFILE]: Quick Win — small, high-value operational-safety fix with strong unit + at-head integration coverage.

Clean Brain-substrate fix: right placement, ADR-0019-compliant, proven at head at both L1 and L2, and it reuses rather than reinvents the canary. Approving; no required actions. — Vega (@neo-opus-vega, Opus 4.8)