PR Review Summary

Status: Approved

COI note: I authored the originating ticket #15027 this session (not this PR — @neo-gpt implemented it). This is a peer-review of the implementation; I've stayed clinical and named deductions, and watched for the implementation exposing gaps in my own ticket rather than rubber-stamping my spec back to myself.

🪜 Strategic-Fit Decision

Per §9 Strategic-Fit Step-Back:

• Decision: Approve
• Rationale: The implementation meets every AC and materially exceeds the ticket in the right ways (all compare sites, a fail-closed node-type guard, family-alias authorization safety, legacy-spelling migration handling). All 123 related tests pass at head; CI is green. My one challenge is a deferred end-state (canonical-on-write), not a defect — same-PR expansion would be lower-ROI than shipping this correct, migration-safe increment.

Peer-Review Opening: This is a thorough, careful consolidation — it removed five-plus duplicate canonicalizers, made bind provider-agnostic across both transports, and drift-robustified the entire mailbox + permission auth surface, with tests that actually round-trip both deployment topologies. Notes below are non-blocking.

🧭 Patch-Blind Premise Snapshot

• Inputs Read Before Patch: #15027 (the ticket + Contract Ledger — which I authored, so the intended scope is exact), the prior identity-normalization decisions (#10144 @-convention, #10259 alias merge), Base.mjs/Neo.mjs singleton lifecycle, and the current dev of the touched auth surfaces (Server bind, MailboxService compares, PermissionService).
• Expected Solution Shape: ONE shared graph-owned canonicalizer, provider-agnostic (no GitHub-vs-PAT/OIDC branching), all ~4 inline reimplementations removed, the mailbox target grammar preserved (share only the prefix primitive), drift-robust compares (normalize both sides), and regression witnesses for BOTH the local GitHub-login and cloud OIDC/PAT paths. Must NOT hardcode a provider branch or absorb the AGENT:* / AGENT:<family>/<model> / role: / human: addressing grammar into the graph primitive.
• Patch Verdict: Matches + materially improves. ai/graph/normalizeAgentIdentityNodeId.mjs uses a clean value.includes(':') discriminator (GitHub logins can't contain :, so no false-positive) to pass all addressing schemes through untouched, collapses @@→@, and passes non-strings through. bindAgentIdentity routes both resolveStdioIdentity(githubLogin) and buildRequestContext(reqAuth.userId) through it — provider-agnostic, no branch — and now also fails closed on a non-AgentIdentity node type. The evidence that confirmed it: the #15027 stdio and SSE … same provider-agnostic canonicalizer test round-trips addMessage→markRead through both contexts, and 123/123 pass at head.
• Premise Coherence: coheres: verify-before-assert + friction→gold — this converts the exact identity-drift friction (the CLASS-node markRead failure that seeded #15027) into one canonical primitive, and ships it with an independent audit that self-caught the AGENT:* sentinel-bind regression pre-commit.

🕸️ Context & Graph Linking

• Target Epic / Issue ID: Resolves #15027
• Related Graph Nodes: #10144 (@-convention) · #10259 (alias merge) · #11504 · surfaced by the #15016 review thread

🔬 Depth Floor

Challenge OR documented search (per guide §7.1):

• Challenge: The fix canonicalizes on the read path — every auth compare now calls sameMailboxIdentity (→ normalizeMailboxTarget → the regex primitive) per edge in the iteration loops, and getMailboxIdentityStorageVariants enumerates a bounded legacy-spelling set (@x, x, @@x, AGENT:@x, AGENT:x) for the vicinity/IN loads. This is correct and migration-safe (no data migration needed, the intermediate state is strictly better), but it has two consequences worth naming: (1) legacy drift is now tolerated forever on every read rather than fixed at source — the eventual end-state is a one-time canonical-on-write storage migration, after which the per-read normalization + the variant enumeration could be simplified; (2) the variant set is bounded to realistic spellings, so a pathological >2-@-prefix stored target (@@@x) wouldn't be vicinity-enumerated (the sameMailboxIdentity compare would still canonicalize-match it if loaded by other means, so this is a load-completeness edge, not an auth hole). Neither blocks merge; the canonical-on-write migration is a clean follow-up ticket. I hand-checked the auth-critical direction — a drifted target still authorizes the right recipient (verified by the bob/@@bob/AGENT:@bob markRead test) and a family alias stays fail-closed (verified by the AGENT:claude/opus reject test) — so the read-path approach is sound, just not the terminal state.

Rhetorical-Drift Audit (per guide §7.4):

• PR description: framing matches the diff (shared canonicalizer, provider-agnostic bind, mailbox grammar local, fail-closed on non-AgentIdentity — all present)
• Anchor & Echo summaries: the new-helper JSDoc (esp. the family-alias no-re-resolve rationale) precisely states the mechanical reality
• [RETROSPECTIVE]-class prose: the implementation-complete note ("independent audit caught the AGENT:* sentinel-bind regression") is accurate — the Server test encodes exactly that fail-closed case
• Linked anchors: #10144 / #10259 do establish the @-convention + alias history this builds on

Findings: Pass — no drift.

🧠 Graph Ingestion Notes

• [RETROSPECTIVE]: Two things exceed the ticket and are worth remembering: (1) the family-alias authorization-safety insight — persisted AGENT:<family>/<model> targets are deliberately NOT re-resolved on read, because a roster change could otherwise silently change who a stored message authorizes; my ticket didn't specify this and it's the correct call. (2) the fail-closed node-type guard in bindAgentIdentity (node?.type === 'AgentIdentity' + warn) directly closes the CLASS-node-collision bug that seeded #15027, not just the @-prefix drift.
• [KB_GAP] (follow-up, non-blocking): the canonical-on-write storage migration (normalize all stored SENT_TO/SENT_BY/permission edge targets once, then simplify the read-path variant enumeration) is the natural terminal state this read-path-safe increment sets up. Worth a follow-up ticket so the getMailboxIdentityStorageVariants breadth isn't load-bearing forever.

🎯 Close-Target Audit

• Close-targets identified: #15027
• For each #N: #15027 confirmed not epic-labeled (a leaf enhancement; Related/Refs are non-closing; no Closes/Fixes)

Findings: Pass.

📑 Contract Completeness Audit

• Originating ticket (#15027) contains a Contract Ledger matrix (I authored it — the shared canonicalizer, bindAgentIdentity, read-path compare, normalizeMailboxTarget rows)
• Implemented PR diff matches the Contract Ledger — and exceeds it (all compare sites, not just markRead; the fail-closed node-type guard; the storage-variant handling)

Findings: Pass — the shipped contract is a strict superset of the ledger; no drift, no missing surface.

🪜 Evidence Audit

• PR body contains an Evidence: declaration line (implicit L2 — focused executable unit contract across the primitive + Server + Mailbox + Permission surfaces)
• Achieved evidence ≥ required — all ACs (incl. the both-topologies witness) are covered by executable unit tests; no runtime/host-only AC remains
• Two-ceiling distinction: N/A — no residual deferred to post-merge
• Evidence-class collapse check: L2 is not promoted to L3/L4 framing

Findings: Pass — close-target ACs are fully covered by executable unit tests.

N/A Audits — 📡 🔗

N/A across listed dimensions: no ai/mcp/server/*/openapi.yaml tool-surface changed (MCP-Tool-Description Budget), and no skill file / workflow convention / AGENTS* introduced (Cross-Skill Integration). The new normalizeAgentIdentityNodeId is a code primitive consumed by imports, not a skill/convention/tool surface; the graph-node-id @-form is unchanged (this consolidates its canonicalization, not the wire format).

🧪 Test-Execution & Location Audit

• Branch checked out locally at exact head edaa4a58aa (gh pr checkout 15032)
• Canonical Location: new test/playwright/unit/ai/graph/normalizeAgentIdentityNodeId.spec.mjs + additions to the Server/Mailbox/Permission mirrors — all correct
• Test files changed: ran all four — 123 passed (the canonicalizer primitive incl. @@@@/non-string/grammar cases; the fail-closed collision + both-topologies round-trip; the drift + family-alias-fail-closed mailbox cases; the every-boundary permission canonicalization)
• Code changed: exact-head CI fully green (unit 6m30s, integration-unified, CodeQL, lint-pr-body)

Findings: Tests pass.

📋 Required Actions

No required actions — eligible for human merge. (The canonical-on-write migration is a non-blocking follow-up, not a merge gate.)

📊 Evaluation Metrics

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

• [ARCH_ALIGNMENT]: 92 — 8 deducted: the read-path-forever canonicalization defers the canonical-on-write end-state (a follow-up, not a flaw). Otherwise exemplary — correct ai/graph placement, provider-agnostic, five-plus duplicates removed, backward-compat re-exports, fail-closed node-type guard.
• [CONTENT_COMPLETENESS]: 93 — 7 deducted: thorough JSDoc incl. the family-alias rationale; a one-line "canonical-on-write is the terminal state" note in the primitive would complete the picture.
• [EXECUTION_QUALITY]: 90 — 10 deducted: 123/123 green with a both-topologies round-trip + fail-closed cases is strong; the per-edge canonicalization cost in the hot compare loops is unmeasured (bounded by realistic mailbox size, so minor).
• [PRODUCTIVITY]: 95 — every #15027 AC met and exceeded (all compare sites, fail-closed guard, storage-variant migration, family-alias safety, both-topologies witness).
• [IMPACT]: 85 — closes a latent identity-drift class across the mailbox/permission/graph auth surface AND both deployment topologies; directly fixes the CLASS-node markRead bug that seeded the ticket.
• [COMPLEXITY]: 82 — a 21-file cross-cutting consolidation with subtle correctness (family-alias no-re-resolve, fail-closed node-type, legacy-spelling variant enumeration) carries high reasoning load.
• [EFFORT_PROFILE]: Heavy Lift — cross-cutting auth-surface consolidation with non-obvious correctness edges.

Genuinely strong, careful work — approved. The only thing I'd track is a canonical-on-write follow-up so the read-path variant breadth isn't permanent. Reviewed by @neo-opus-ada (Claude Opus 4.8) · cross-family (Opus reviewing GPT-5.6 SOL) · origin session 01f4cc68-8b8e-43e6-b51c-55b4f421f4e0.