PR Review Summary

Status: Approved

🪜 Strategic-Fit Decision

Per §9 Strategic-Fit Step-Back:

• Decision: Approve
• Rationale: The change consumes an index the ticket already established as existing, rather than inventing one — the cheapest correct shape. Correctness is preserved at the branch level (broadcast, damaged-projection fallback, totalCount semantics), and the spec carries a real mutation witness rather than a timing assertion. My two findings are a changed failure mode and an unconverted sibling call site; neither is delivered-scope incorrectness, so Request Changes would be scope transfer dressed as rigor.

Peer-Review Opening: Nice piece of work. The part I want to single out is the falsifier: poisoning _items through a Proxy so any full-array walk throws is a mutation witness, not a timing proxy — it fails for the exact reason the ticket exists, and it will keep failing if someone reintroduces a scan later. That is a materially better test than a benchmark would have been.

🧭 Patch-Blind Premise Snapshot

• Inputs Read Before Patch: #16960 body (including its Architectural Reality section), the changed-file list, ai/graph/Store.mjs getByIndex() at dev, MailboxService.mjs helper surface at the PR head, and #16960's labels. Prior-art sweep run against Memory Core — see [TOOLING_GAP] below.
• Expected Solution Shape: Replace the repeated O(E) full-edge walks with lookups against the source/target secondary indexes the ticket states Store.mjs already maintains, and replace the O(M²) messages.find() dedup with a keyed structure. Must not hardcode index internals or duplicate an index inside MailboxService; must preserve totalCount = all filter matches with pagination applied last. Test isolation should prove no full-store walk occurs, not merely that results are unchanged.
• Patch Verdict: Matches. db.edges.getByIndex('target'|'source', …) is consumed as a public Store API, no index is duplicated locally, candidateMessageIds is a Set (killing the O(M²) dedup), and pagination/totalCount are untouched below the hunk. The evidence that confirmed rather than assumed this: Store.getByIndex() (ai/graph/Store.mjs:113) resolves through indexMaps and returns [] — a genuine keyed lookup, not a filtered scan behind an index-shaped name.
• Premise Coherence: Coheres — verify-before-assert. The PR does not assert the projection got cheaper; it makes a full-store walk throw, so the claim is mechanically enforced rather than measured once and trusted.

🕸️ Context & Graph Linking

• Target Epic / Issue ID: Resolves #16960
• Related Graph Nodes: parent #16677; adjacent-not-duplicate #16767; consumer fleetA2AActivityAdapter.mjs
• Origin Session ID: 77a6d06c-ef28-41d9-9a9d-6ebc78814611

🔬 Depth Floor

Challenge (two, both non-blocking):

1. The failure mode changed from slow to silently empty, and that is the class this repo has been bleeding on all week. Store.getByIndex() returns [] when indexMaps — or the property map, or the value set — is absent (ai/graph/Store.mjs:113-126). It does not fall back to a scan. The superseded code walked db.edges.items, which is always correct-if-slow. So an edge store constructed without both indexes now yields an empty mailbox rather than a slow one, and an empty mailbox is indistinguishable from "you have no messages." Production is safe today — Database.mjs constructs the edge store with both indexes, and your poisoned-iterator test proves the index path is live — but the safety is positional, not asserted. A one-line precondition (assert the source/target index exists before routing discovery) converts a silent wrong answer into a loud one. Non-blocking, and I would take a ticket over an in-place change here.

2. getBroadcastDeliveryEdges() still performs the exact scan this ticket names. MailboxService.mjs:1702 filters GraphService.db.edges.items, and it is still reached from five call sites (:3190, :3480, :3505, :3516, :3690 — authorization, mark-read, archive). Correctly out of this PR's scope: those are single-message paths, so O(E) once rather than O(E×M), and the ticket is explicitly the list projection. Flagging it because this PR makes getByIndex('source', messageId) the established idiom in this same file, which turns those five into near-mechanical conversions for whoever picks up the parent.

Rhetorical-Drift Audit (per guide §7.4):

• PR description: framing matches what the diff substantiates (no overshoot)
• Anchor & Echo summaries: the new comments describe the mechanism (index-vs-scan, the bounded variant union) without overshooting durable intent
• [RETROSPECTIVE] tag: n/a — none claimed by the author
• Linked anchors: #16767 is cited as adjacent, not duplicate, which the diff supports

One item worth naming as precision rather than drift: the in-code comment "limit bounds the response page, not the amount of unrelated graph work we may perform" is an accurate statement of the defect and the best single sentence in the diff.

Findings: Pass.

🧠 Graph Ingestion Notes

• [KB_GAP]: none.
• [TOOLING_GAP]: the mandated prior-art sweep (query_raw_memories) returned only unrelated mailbox-ping memories for a targeted MailboxService listMessages edge scan projection index query. This plane's semantic gate is currently degraded (embed-deferred, canary loop stale), so I record the sweep as run but unreliable, not as a clean negative. Reviewers on this plane today should not treat semantic recall as an absence proof.
• [RETROSPECTIVE]: The generalisable move is making the anti-pattern throw instead of measuring that it got faster. A benchmark asserts a number that drifts with hardware and dataset; the Proxy on _items asserts a property — "this code path does not walk the store" — which is what the ticket actually wants and survives every future refactor. This is the shape performance regressions should be pinned with.

N/A Audits — 📑 🪜 📡 🔗

N/A across listed dimensions: no public/consumed contract shape changed (the list_messages request/response surface in openapi.yaml is untouched), no OpenAPI description modified, no skill/convention/MCP-tool surface introduced, and the close-target ACs are fully CI-reachable so no evidence-ladder residual arises.

🎯 Close-Target Audit

• Close-targets identified: Resolves #16960 (newline-isolated, single leaf)
• For each #N: #16960 labels are bug, ai, performance, agent-os — confirmed not epic-labeled

Findings: Pass.

🧪 Test-Evidence & Location Audit

• Execution evidence: exact-head required CI green at e25f972812 — 21 checks reported, 0 failing, 0 running. Author receipt present with an Evidence: L2 → L2 required. No residuals. declaration.
• Reviewer falsifier: N/A as a rerun — I did not duplicate green CI. My named concern (does getByIndex silently degrade?) was resolved by source read at ai/graph/Store.mjs:113-126, the correct instrument for it; a test run could not have answered it.
• Test location: added coverage sits in the existing MailboxService.spec.mjs beside its subject — correct owner, no new file, idioms consistent with the surrounding suite.

Findings: Pass. The added test earns particular credit for asserting a property rather than a duration.

📋 Required Actions

No required actions — eligible for human merge.

(Both Depth-Floor challenges are explicitly non-blocking and neither is a delivered-scope defect. If either is worth carrying, it belongs on parent #16677 as a leaf rather than as a return cycle here — I did not mint one, since ticket budget is the author's call in their own lane.)

📊 Evaluation Metrics

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

• [ARCH_ALIGNMENT]: 96 - consumes Store's public index API rather than duplicating an index in the service or reaching into indexMaps; projection stays in the file that owns projection. 4 deducted because getRelatedTicketsForMessage() now performs a DB read in a default parameter (sourceEdges = db.edges.getByIndex(…)) — functionally fine here with no totality contract, but a default-parameter side effect is a seam that reads as free and is not.
• [CONTENT_COMPLETENESS]: 98 - the new comments explain why the index is mandatory and why the variant union preserves legacy identity spellings, which is the non-obvious half. 2 deducted for the absent Origin Session ID in the PR body.
• [EXECUTION_QUALITY]: 95 - branch-level semantics preserved under inspection: the damaged-projection fallback is retained and commented, and isInboxMatch = isDirectRecipient || Boolean(deliveryEdge) || (isBroadcastRecipient && !hasDeliveryEdges) reproduces the superseded deliveryEdge || !hasBroadcastDeliveryEdges rule while folding in the direct-recipient case the outer loop used to carry. Scored from exact-head CI plus source read, not author prose. 5 deducted for the silent-empty degradation in challenge 1.
• [PRODUCTIVITY]: 100 - the ticket's five enumerated cost sources are each addressed: candidate scan, O(M²) dedup, per-message metadata scan, per-message related-ticket scan, and the repeated broadcast-delivery filter.
• [IMPACT]: 74 - removes a quadratic factor from the hot mailbox path on a plane that measured 93,765 edges and list calls reaching 21,606 ms; bounded to one service's read path, so not core-architectural.
• [COMPLEXITY]: 62 - one method restructured from edge-driven to message-driven iteration; the reader must hold the candidate-set/projection split and the broadcast branch simultaneously, though both are locally commented.
• [EFFORT_PROFILE]: Quick Win - high ROI against a measured hot path, contained to one service plus its spec, with no contract change.

Approving. Cross-family seat (GPT→Claude); CI green at exact head; the two findings above are carried as observations rather than gates.

⚖️ Ada