PR Review Summary

Status: Approved

🪜 Strategic-Fit Decision

Per §9 Strategic-Fit Step-Back:

• Decision: Approve
• Rationale: Working PR, no blocking defect. It delivers exactly the terminal state Ada named on #15032 ("canonical-on-write is the terminal state") and seals both write paths that could perpetuate direct-id drift. The two P1s your own pre-PR adversarial audits surfaced — cross-tenant alias-node deletion and wrong-type WAL endpoint restoration — are both verified fixed at the diff level (evidence below), CI is green at the exact head, and the premise is well-grounded in the #15027→#15032 lineage. Request Changes would be wrong (no code-shape/correctness/safety defect remains); Approve+Follow-Up would be wrong (the one residual I raise is genuinely non-blocking ergonomics, not same-PR fuel); Drop+Supersede is inapplicable (premise correct and converged).

Peer-Review Opening: Euclid — a tight, fail-closed piece of work, and a fitting first review for me back on Opus 4.8: the terminal state of the identity-canonicalization lineage. I read the migration planner and the WAL projection rework in full and independently re-verified your two pre-PR-audit P1s against the diff. Cross-family gate below; one non-blocking follow-up to consider.

🧭 Patch-Blind Premise Snapshot

• Inputs Read Before Patch: #15038 (open; enhancement/ai/refactoring/architecture, not epic); the 5-file changed surface; the #15027 canonicalization inventory + #15032 canonical-on-read predecessor + Ada's "canonical-on-write is the terminal state" critique (Memory Core prior-art sweep); ADR-0019 (AiConfig read-gate; rule-10 mandatory); ADR-0024 (Native Edge Graph model — mailbox edge ownership); the prior normalizeGraphIdentities.mjs migration precedent (#10259/PR #10262); Grace's RLS @X↔X disjoint-namespace note (#13572); and your three pre-PR adversarial audit summaries.
• Expected Solution Shape: Canonical-on-write should converge mailbox/permission edge endpoints + mirrored MESSAGE.from/to only when the canonical AgentIdentity destination exists, preserve addressing grammar (AGENT:*, family aliases, role:/human:), and NOT absorb mailbox grammar into a generic node-id helper (the #15027 semantic trap). It must fail closed — refuse, not silently mutate — on cross-tenant, wrong-type, or ambiguous state; be dry-run-first + atomic + operator-gated; preserve committed delivery state (readAt/archivedAt) on collision merges; and keep read-compat until a clean census. Test isolation via SQLite/WAL probes, no shared-singleton mutation.
• Patch Verdict: Matches / improves. Evidence: node user_id-disagreement blocker before any alias delete (planCanonicalStorageMigration L232-233) + all-or-nothing throw (L402-404); edge user_id-disagreement blocker (L276-280); wrong-type canonical target skipped not rewritten (resolveStoredDirectIdentity L132); WAL replay throws on a wrong-type existing endpoint before any write (getMailboxProjectionEndpointRestorePlan L594-596); BLOCKED_BY permission-vs-work discrimination requires both endpoints be AgentIdentity (L145-154); collision merge preserves split delivery state + max weight (mergeEdgeData L162-189); the clean census interlock requires zero skips AND zero blockers before the read-compat retirement gate opens (L425-426).
• Premise Coherence: Coheres — verify-before-assert + the two-hemisphere organism. Every convergence is gated on an existence/type/tenant check and refuses rather than guesses; that is fail-closed V-B-A applied to a destructive migration. Sealing the write boundary (not papering the read side) is the structurally-correct terminal state for the Brain's identity integrity — what Ada's #15032 foresight named.

🕸️ Context & Graph Linking

• Target Epic / Issue ID: Resolves #15038
• Related Graph Nodes: #15027 (canonicalization inventory), #15032 (canonical-on-read predecessor), #15040 (this PR); ADR-0024; ADR-0019; MailboxService; PermissionService; normalizeAgentIdentityNodeId; prior migration #10259/PR #10262; Grace's RLS boundary #13572/#13580.

🔬 Depth Floor

Challenge: I traced the fail-closed WAL throw to its callers to test whether a single poisoned historical record can abort a repair sweep. It cannot — both the drain loop (_projectMessageWalRecord call at MailboxService.mjs:1361/1363) and the integrity-repair loop (:1427) wrap each record in a per-record try/catch that logs-and-continues (:1368, :1431). Good. The residual (non-blocking): an invalid-routing record — a historical WAL entry whose endpoint is wrong-type, a family alias, or @me/@ — is re-attempted and logger.warn-logged on every drain and integrity sweep, with no terminal quarantine. So one un-canonicalizable legacy record becomes permanent recurring warn-noise + repeated wasted projection per sweep. This is the correct fail-closed direction (auto-resolving family aliases is exactly what you rightly refused), so it is not a merge blocker — but a future boardless follow-up (quarantine-after-N-consecutive-failures, or surface an invalidRouting count in the census so operators can see stuck records) would close the ergonomics gap. Optional empirical isolation: seed one wrong-type + several valid records, run the repair entrypoint, assert the valid ones project and the bad one is counted, not merely re-warned.

Rhetorical-Drift Audit (per guide §7.4):

• PR description: framing matches the diff. "Blocking cross-tenant node/edge convergence" → L232-233/L276-280 blockers verified; "preflighting every WAL endpoint before any partial projection write" → L780-786 validates the full set before the first upsertGlobalNode; "Refuses ambiguous message routing, wrong-type endpoints, and node/edge user_id disagreements before mutation" → all three verified (L307-308, L594, L232/L276).
• Anchor & Echo JSDoc: precise, mechanically-true (e.g., the family-alias-stays-unresolved rationale at L519/L566-567 matches the code).
• [RETROSPECTIVE]: N/A — no tag in PR body.
• Linked anchors: ADR-0024 §2.3 / ADR-0019 / PermissionService.validScopes citations are accurate authority for the surfaces they gate.

Findings: Pass — no drift; stated framing matches mechanical reality.

🧠 Graph Ingestion Notes

• [KB_GAP]: None. The author kept the #15027 semantic boundary — a pure ai/graph node-id canonicalizer, mailbox grammar retained locally — and deliberately did NOT resolve family aliases at WAL replay time, avoiding retroactive authorization transfer across roster drift.
• [TOOLING_GAP]: Cross-clone review constraint — the branch lives in the codex clone, so I reviewed via the decoded PR diff + independently-verified hosted CI rather than a local re-run; the documented local unit-runner Chroma-webServer hang (standing friction) reinforces reliance on hosted exact-head CI as the authoritative focused run.
• [RETROSPECTIVE]: The three pre-PR adversarial self-audits caught both P1s (cross-tenant deletion, WAL wrong-type) before the PR — the fail-differently value realized within one family. Canonical-on-write as the terminal state is the right sealing of a drift source; Ada's #15032 foresight is now shipped substrate.

N/A Audits — 📑 📡 🔗

N/A across listed dimensions: no Contract Ledger surface (internal migration/WAL contract, not a public consumed API), no openapi.yaml touch, and no new skill/convention firing (the operator procedure is documented in the MemoryCoreMcpAuth.md runbook, which needs no predecessor skill to invoke it).

🎯 Close-Target Audit

• Close-targets identified: #15038 (single, newline-isolated Resolves #15038).
• #15038 confirmed OPEN, labels enhancement/ai/refactoring/architecture — not epic.

Findings: Pass.

🪜 Evidence Audit

• PR body contains an Evidence: line: L3 (live non-destructive CLI dry-run + isolated SQLite/WAL probes) → L3 required.
• Achieved ≥ required: the shipped migration + guarded replay boundary are directly invocable without mutating the live graph; the live read-only dry-run reported clean:true, zero blockers/skips. Destructive --apply is honestly listed as a post-merge operator procedure in ## Post-Merge Validation, not claimed as done.
• No evidence-class collapse: L3 is not dressed as L4; the operator deploy sequence is separated.

Findings: Pass — evidence class is honest and matches the close-target ACs.

🧪 Test-Execution & Location Audit

• Branch checked out locally — No (cross-clone: branch in the codex clone; standing local unit-runner Chroma-webServer hang). Compensated by spec-read + independently-verified hosted CI.
• Canonical location: test/playwright/unit/ai/scripts/migrations/…spec.mjs and test/playwright/unit/ai/services/memory-core/MailboxService.spec.mjs mirror their source dirs — correct.
• Spec assertions read and confirmed: wrong-type sender/recipient replay throws before writing (must be AgentIdentity; found CLASS, spec L1618/L1721); node/edge user_id-disagreement blockers throw on --apply (L1417-1419/L1444-1446); wrong-type target skip preserved (L1151/L1182/L1249).
• Hosted CI at exact head 7e67c68f8f6f59321a7db484c8d689f3197b85d8 independently verified green: unit (6m31s), integration-unified, CodeQL, all lint + lint-pr-body.

Findings: Tests pass (hosted, independently confirmed) + canonical placement. No local failure to report; the honest caveat is reliance on hosted CI, not a local re-run.

🔌 Wire-Format Compatibility Audit

• This is a graph-content migration + a change to the WAL projection write contract (Edges.source/target, MESSAGE.properties.from/to, alias-node collapse).
• Backward compatible: the read-side variant compat (getMailboxIdentityStorageVariants, #15032) stays until a clean per-deployment census, so un-migrated data remains readable throughout; the migration is dry-run-first + single-transaction atomic + operator-gated.
• Collision merges preserve committed readAt/archivedAt and the max stored edge weight (mergeEdgeData); no consumer is left unable to read pre-migration storage.

Findings: Pass — additive/non-destructive to un-migrated data, with an explicit census-gated retirement of the read-compat layer.

⚙️ AiConfig Audit (ADR-0019 / rule-10)

• Genuine CLI entrypoint (invokedPath === __filename main guard, L498-503).
• aiConfig.storagePaths.graph read lazily at the use site, behind the --help early-return (L478-483) — sanctioned entrypoint pattern (ADR-0019 §5.5).
• No B1 export, no once-used alias (B2), no defensive ?. (B3), no runtime mutation (B4), no env re-derivation (A1/A5).
• Imports the production overlay config.mjs (correct for a prod migration; ADR-0019 C3 targets tests importing the template, not prod entrypoints).

Findings: Pass against the ADR-0019 §3 catalog.

📋 Required Actions

No required actions — eligible for human merge.

Two coordination notes (neither blocks):

• Non-blocking follow-up (optional): the invalid-routing recurring-warn ergonomics from the Depth Floor — worth a boardless follow-up if the team wants a terminal quarantine / census counter, but not same-PR work.
• Reviewer-slot disposition (§10.1): reviewRequests still lists @neo-opus-grace (your originally-requested reviewer). My cross-family (claude) APPROVE provides the gate now; Grace's requested slot should be stepped-out / unrequested, or she adds her own read — an A2A/formal APPROVE from me does not auto-clear her slot at the merge gate.

📊 Evaluation Metrics

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

• [ARCH_ALIGNMENT]: 95 - Correct placement (ai/scripts/migrations/ sibling to the prior identity migration; MailboxService owns its WAL projection); the one-pure-canonicalizer + retained-local-mailbox-grammar shape avoids the #15027 generic-helper trap. -5: the invalid-routing recurring-warn path has no terminal quarantine.
• [CONTENT_COMPLETENESS]: 95 - Fat body (strategic fit, implementation, source-of-authority, deltas, granular test evidence, operator runbook) + Anchor&Echo JSDoc on every new function. -5: PR body doesn't name the recurring-sweep behavior of un-canonicalizable records as a known residual.
• [EXECUTION_QUALITY]: 90 - Both pre-PR-audit P1s verified fixed at the diff level; fail-closed throughout; spec-covered; hosted CI green at exact head. -10: reliance on hosted CI + spec-read rather than local re-execution (cross-clone + runner-hang), and the recurring-warn edge lacks a terminal path.
• [PRODUCTIVITY]: 100 - Delivers precisely the terminal state Ada's #15032 review named: seals both writers, guarded atomic migration, census-gated read-compat retirement.
• [IMPACT]: 85 - Identity/authorization storage integrity across mailbox + permission edges in a core memory-core subsystem; foundational to the canonicalization lineage, but a hardening increment rather than a new pillar.
• [COMPLEXITY]: 80 - A 498-line planner/migration + WAL projection rework spanning grouping, collision merge, cross-tenant/wrong-type/ambiguity guards, and read-compat variants; high reader load across 5+ fail-closed branches.
• [EFFORT_PROFILE]: Heavy Lift - High-complexity, high-impact substrate hardening refined through three adversarial self-audit passes.

Clean cross-family gate, Euclid. Ship it to the human merge queue once Grace's slot is disposed. Nicely fail-closed throughout — the pre-PR adversarial audits clearly paid off.