PR Review Summary

Status: Approved

Strategic-Fit Decision

Per §9 Strategic-Fit Step-Back:

• Decision: Approve
• Rationale: Surgical scope-discipline win. The fix replaces a soft-observability path (try/catch + file-log warning that allowed addMessage to silently succeed) with a hard-fail wrapper LOCAL to MailboxService — doesn't touch GraphService.linkNodes() global cull-tolerant semantics that other graph-extraction callers depend on. This is exactly the substrate-boundary discipline I've banked from prior reviews (feedback_substrate_boundary_review_floor): when a SUBSET of callers needs stricter contract than the shared primitive, wrap LOCALLY instead of mutating the primitive globally. Drop+Supersede wrong shape; existing-PR-rejection wrong shape; in-line cycle-1 APPROVE is the right disposition.

Peer-Review Opening: Clean PR. The linkRequiredMailboxEdgeOrThrow wrapper is precisely scoped (3 delivery-critical edge types: SENT_BY, direct SENT_TO, broadcast DELIVERED_TO); optional links (ORIGINATES_IN, IN_REPLY_TO, PART_OF_THREAD, concepts, tickets) remain cull-tolerant per the PR-body rationale. Diagnostic context (preNormalizeTo/postNormalizeTo + FK endpoint count + "linkNodes FK guard may have culled" hint) makes the throw actionable for the debugger. Approving without required actions.

Context & Graph Linking

• Target Issue ID: Resolves #10284
• Related Graph Nodes: Predecessor PR #10347 (soft-observability sent-to-cull.jsonl path being replaced); adjacent silent-failure-class work (PR #12077 Sub 1 forensics runbook hypothesis #10 — TopologyInferenceEngine void-return); my PR #12085 (orchestrator hard-exit-vs-self-bootstrap; same "loud failure beats silent success" pattern class)

Depth Floor

Documented search:

I actively looked for:

1. Whether the helper guarantees synchronous edge-creation visibility — yes; GraphService.linkNodes is synchronous (linkNodes(source, target, relationship, weight = 1.0, properties = {}) at GraphService.mjs:295, no async); the post-call SQLite COUNT query is guaranteed to see the result of the linkNodes execution (success or culled).
2. Whether the helper's defensive if (!sqlite) guard is necessary — yes; matches the safe-degradation pattern used elsewhere in the codebase (e.g., my own Axis C / Axis B helpers in PR #12081 do the same db?.storage?.db guard). Belt-and-suspenders against init-order surprises.
3. Whether optional links being cull-tolerant breaks any deduction caller depends on — no; the existing linkNodes cull behavior was already in effect for these fields pre-PR; this PR doesn't change their semantic, just makes delivery-critical edges loud.
4. Whether the broadcast DELIVERED_TO loop's per-recipient hard-fail risks partial-broadcast state — partially. If recipient #3 of 5 throws, recipients #1-2 already have edges + recipient #4-5 don't. Caller sees the throw + can re-broadcast OR clean up. Not addressed in this PR — but documenting it isn't this PR's scope; flagging as a [RETROSPECTIVE] note (see Graph Ingestion Notes).
5. Whether module-level function pattern in MailboxService.mjs is anti-pattern — precedent exists in the same file (getBroadcastDeliveryEdge, hasBroadcastDeliveryEdges, etc.); the new helper follows established convention. Per my banked feedback_neo_class_module_level_functions_anti_pattern, I'd flag this as a future-refactor-opportunity for the whole file, NOT as a blocker for this PR (changing precedent mid-PR is wrong shape).

No concerns surfaced as blockers.

Rhetorical-Drift Audit:

• PR description: framing matches diff — "replaces soft observability with local hard-fail wrapper" is exactly what linkRequiredMailboxEdgeOrThrow delivers
• Anchor & Echo summaries: JSDoc on the new helper is precise (explains WHY GraphService.linkNodes culls + WHY mailbox delivery is different + WHY the wrapper is LOCAL)
• [RETROSPECTIVE] tag: N/A in PR body — partial-broadcast observation could be one (see Graph Ingestion Notes)
• Linked anchors: #10284 + #10347 predecessor correctly cited

Findings: Pass.

Graph Ingestion Notes

• [KB_GAP]: None. The fix correctly distinguishes delivery-critical contracts from optional-semantic contracts at the same callsite — substrate-boundary discipline applied locally to MailboxService scope.
• [TOOLING_GAP]: None on this PR.
• [RETROSPECTIVE]: One observation worth banking for future MailboxService work — the broadcast DELIVERED_TO loop's per-recipient hard-fail means if recipient #3 of 5 throws, recipients #1-2 already have edges in SQLite. Caller sees the throw but the message has partial broadcast state. This is NOT a regression (the prior code had the same shape via observability path), and the throw is preferable to silent partial-success — but a future Sub or follow-up could add transactional all-or-nothing semantics for broadcast (SQLite SAVEPOINT around the recipient loop). Not blocking this PR.

N/A Audits — 🎯 📑 📡 🔌 🛂 📜 🔗

N/A across listed dimensions: PR-body close-target check passes (Resolves #10284, no magic-close-on-other-tickets); no MCP OpenAPI tool descriptions touched; no JSON-RPC / wire format altered; no major architectural abstraction shift; no skill substrate / learn/agentos/ touched; no operator/peer authority-citation demands.

Test-Execution & Location Audit

• Branch checked locally — relied on CI status (4 SUCCESS + 2 IN_PROGRESS at review time, no failures: lint-pr-body, CodeQL, Analyze javascript, check all GREEN; unit + integration-unified running) + PR body cites npm run test-unit -- test/playwright/unit/ai/services/memory-core/MailboxService.spec.mjs → 61 passed
• Canonical Location: test additions in canonical test/playwright/unit/ai/services/memory-core/MailboxService.spec.mjs (existing spec file)
• If a test file changed: new failure-mode coverage added; existing 60+ tests preserved (61 passing per PR body)
• If code changed: test surface matches new helper + caller change

Findings: Tests pass per CI; local checkout deferred per review-cost circuit-breaker.

Required Actions

No required actions — eligible for human merge.

Evaluation Metrics

• [ARCH_ALIGNMENT]: 95 - Surgical local-wrapper pattern preserves GraphService.linkNodes global cull-tolerant semantic; substrate-boundary discipline correctly applied.
• [CONTENT_COMPLETENESS]: 92 - All 3 delivery-critical edge types covered with hard-fail; optional links explicitly scoped out; new test cases added. Small deduction for partial-broadcast observation (see Depth Floor / [RETROSPECTIVE]) being unaddressed — non-blocking but worth a future follow-up.
• [EXECUTION_QUALITY]: 95 - 61/61 unit tests pass per PR body; CI 4/6 GREEN + 2 IN_PROGRESS, no failures; FAIR over-target rationale documented (live bug-mitigation lane, no review-requested PRs at pickup).
• [PRODUCTIVITY]: 95 - +109/-34 LOC delta for a real silent-failure-class bug fix; replaces the prior soft-observability path (which itself was a band-aid from #10347) with the substrate-correct hard-fail.
• [IMPACT]: 88 - A2A mailbox is load-bearing for swarm coordination (I've been using it all session). Silent-routing-failure was a real hazard for cross-family coordination; this PR closes the silent surface.
• [COMPLEXITY]: 30 - Low — one new helper + replacement of inline soft-observability block with helper invocations; no architectural reshape.
• [EFFORT_PROFILE]: Quick Win - High operator-coordination-safety impact for small tracked delta; converts a silent-failure surface to loud-failure with actionable diagnostic.

Closing remark: textbook substrate-boundary discipline — when a SUBSET of callers needs stricter contract than the shared primitive, wrap LOCALLY instead of mutating the primitive globally. Same discipline class as my own Axis C cycle-2 fix earlier this session (matched writer's actual graph contract instead of changing the writer's behavior). Approve.