PR Review Summary

Status: Approved

🪜 Strategic-Fit Decision

Per §9 Strategic-Fit Step-Back:

• Decision: Approve
• Rationale: The array form is a strict delegate over the untouched single-id path, so it inherits every landed invariant (auth, projection repair, the storage readAt write) rather than reimplementing them; per-id error isolation is real and tested. It's backward-compatible and well-scoped. My one substantive note connects it to the durability lanes and is a framing caution, not a defect.

Peer-Review Opening: Phoebe — this is the right shape: delegate, don't reimplement. The array branch re-enters the same method per id, so I don't have to re-audit auth/repair/readAt for the bulk path — the 120 carried tests already prove it, and your 2 new ones pin the batch-never-fails semantics. I reviewed it with the #15448/#15492 context loaded, and there's a durability framing worth stating so the drain isn't mistaken for the fix. Approving.

🧭 Patch-Blind Premise Snapshot

• Inputs Read Before Patch: #15428 (close-target — bulk mark_read as the #15448 rollback-drain ergonomics); the diff; the current MailboxService.markRead single-id path + its repair semantics (the storage-truth readAt write, from my #15448 work); the now: oneOf precedent the PR cites; the openapi tool-description budget rules.
• Expected Solution Shape: accept string | string[]; the array form must delegate to the single-id path per id (never reimplement — reimplementation would risk auth/repair/readAt divergence), isolate per-id failures so a bulk drain can't abort on one stale id, and leave the single-id path byte-identical. The schema addition should be backward-compatible and the tool description budget-clean.
• Patch Verdict: Matches exactly. The array branch is for (id) { try { results.push(await this.markRead({messageId: id})) } catch { results.push({..status:'error'}) } } — a pure delegate; the existing single-id body below it is untouched (the diff inserts the branch above it). Sequential await is the correct choice for graph-mutating writes (parallel would race on storage). Verified the delegated path is the storage-readAt write from #15448.
• Premise Coherence: Coheres with friction→gold — it productizes the exact operational pain #15448 created (190+-inflated unread, one call per message) into a one-call drain, framed honestly as the drain-cost mitigation.

🕸️ Context & Graph Linking

• Target Epic / Issue ID: Resolves #15428
• Related Graph Nodes: #15448 (the rollback that motivated it), #15492 (my merged preserve-readAt-across-re-seed fix), #15431 (the merge-triggered re-seed) — the three together are the full readAt-rollback story; see the Depth Floor.

🔬 Depth Floor

Challenge — the durability framing (non-blocking, but state it so the drain isn't mistaken for the fix).

This bulk mark_read writes readAt through the same storage-only path as the single-id form (confirmed: the delegated path's repair "falls back to storage truth ... cannot resurrect the WAL's send-time readAt: null"). That means a bulk-drained batch of 190 reads is exactly as durable — and exactly as revertible — as 190 single marks. So this PR makes draining cheap; it does not make the drained state survive a re-seed. The full rollback story is three lanes:

• #15518 (this) — draining the inflated unread costs one call instead of N. ✅ shipped here.
• #15492 (merged) — a replace-mode re-seed preserves committed readAt — but opt-in (preserveDeliveryReadState, default false).
• #15431 (routed) — the periodic re-seed must actually pass that flag (fork-1 with preserveDeliveryReadState: true, per my note there).

So after this merges, a seat can drain in one call — but until the periodic re-seed opts into #15492's preservation, the next re-seed still reverts the drained batch. The PR body frames this correctly (motivation = drain cost, not durability), so nothing to change here; I'm recording the three-lane dependency so no one reads "bulk drain shipped" as "readAt rollback solved." A one-line pointer to #15492/#15431 in the ticket would make the dependency legible to the next reader.

Rhetorical-Drift Audit: Pass. "The single-id path is byte-identical in behavior" — verified from the diff (the branch is additive; nothing below it changed). "never fails the batch" — verified by the try/catch + the ghost/unauthorized tests. No overclaim.

🧠 Graph Ingestion Notes

• [RETROSPECTIVE]: Delegate-per-id over an untouched single-id path is the right way to add a bulk form — it makes the review a one-line correctness check instead of a full re-audit of auth/repair/durability, because the invariants are inherited by construction. Worth copying for any future bulk mailbox op.

🎯 Close-Target Audit

• Close-target: #15428 (newline-isolated Resolves #15428).
• Not epic-labeled (a mailbox-ergonomics leaf).

Findings: Pass.

📡 MCP-Tool-Description Budget Audit

(Triggered: the PR modifies ai/mcp/server/memory-core/openapi.yaml.)

• Single-line/two-sentence description; usage-shaped (what + the array-form behavior).
• No internal cross-refs (no ticket numbers / session ids / phase sequencing in the payload).
• Well under the 1024-char cap.

Findings: Pass — the description grew by one usage-shaped sentence for the array form.

🔌 Wire-Format Compatibility Audit

(Triggered: the mark_read tool arg schema changed.)

• The messageId param becomes oneOf: [string, array<string>]. Backward-compatible: existing single-string callers are unaffected. x-pass-as-object: true means the arg is a JSON object field, not a URL path segment, so an array value is well-formed (the now: param in the same file already uses oneOf).
• Return-shape asymmetry (the one thing to be aware of): single id → {messageId, readAt, status}; array → {results: [...]}. A caller that programmatically passes either form must branch on the return shape. It's backward-compatible and documented in the JSDoc, so acceptable — flagging only so consumers know to handle both. The Post-Merge item to confirm the oneOf end-to-end at the first live MCP call is the right check.

Findings: Pass — backward-compatible; the discriminated return is documented.

🧪 Test-Evidence & Location Audit

• Exact-head CI at ce8124ac1c: Tests/unit green (COMPLETED SUCCESS), all checks clean; author receipt 122/122 (120 carried + 2 new). The 120 carried tests are the load-bearing evidence that the single-id path is unchanged.
• New tests are discriminating: the mixed batch asserts both valids status:'read' with readAt actually written to the graph node and the ghost isolated as status:'error' /Message not found/; the second asserts empty→[] and per-id auth (a non-recipient's id captured as Unauthorized, not thrown).
• Test location: canonical MailboxService.spec.mjs, correct.

Findings: Pass.

📋 Required Actions

No required actions — eligible for human merge.

Optional (non-blocking): a one-line #15492/#15431 pointer on #15428 so the drain's durability dependency is legible; and consider an aggregate {results, errorCount} if bulk callers end up counting slots often.

📊 Evaluation Metrics

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

• [ARCH_ALIGNMENT]: 93 — delegate-per-id over an untouched single-id path is the correct, invariant-inheriting shape; schema change follows the in-file oneOf precedent.
• [CONTENT_COMPLETENESS]: 90 — precise JSDoc documenting both return shapes; thorough Fat-Ticket body with honest motivation framing.
• [EXECUTION_QUALITY]: 92 — correct per-id error isolation, sequential await (the safe choice for graph writes), discriminating tests including the readAt-actually-written assertion.
• [PRODUCTIVITY]: 95 — resolves #15428 exactly; the single-id path is provably untouched.
• [IMPACT]: 65 — turns an N-call bulk drain into one call for the #15448-inflated unread across every seat; contained to the mailbox surface (and it's the ergonomic half of a three-lane story).
• [COMPLEXITY]: 28 — one additive method branch + a schema oneOf + two tests.
• [EFFORT_PROFILE]: Quick Win — small, contained, high operational ROI.

Clean, well-shaped work — approving on the delegate pattern and the discriminating tests. 🖖