PR Review Summary

Status: Approved

🪜 Strategic-Fit Decision

Per §9 Strategic-Fit Step-Back:

• Decision: Approve
• Rationale: The contract change is the right shape — an unavailable index now fails where it used to answer [], which is the whole point of the ticket. I attacked three things and two of them refuted my own draft findings. The one that survived is an observability gap at a caller you do not touch, and it is strictly better than the pre-PR behaviour, so it is an observation rather than a blocker.

Peer-Review Opening: Emmy — I went after the blast radius of making getByIndex throw, the node-side write path, and the caller set. The first two came back clean in ways that corrected me. The third turned up one call site where the new explicit failure is caught and discarded, which is worth a line even though the degradation itself is correct.

🧭 Patch-Blind Premise Snapshot

• Inputs Read Before Patch: ticket #16965 in full; ai/graph/Store.mjs and Database.mjs on dev; a content census of every getByIndex caller in ai/ and src/; the node-side vs edge-side receipt writers at your head; issueFocusSections.hasOpenIssueBlocker. ADR-0019 not required — no ai/ config leaf is touched.
• Expected Solution Shape: the lookup surface must separate index unavailable from value absent, and the consumer must not reach into indexMaps to do it. A throw is only safe if every production construction path supplies the indexes and no caller silently swallows the new error — otherwise the failure just moves.
• Patch Verdict: Matches. assertIndices puts the check on the Store where it belongs, getByIndex returns [] only for a genuine no-match, and MailboxService consumes the contract rather than the index maps.
• Premise Coherence: Coheres with verify-before-assert: it converts a state that answered plausibly into one that cannot answer at all. That is the correct direction for a lookup whose false answer is "your inbox is empty".

🕸️ Context & Graph Linking

• Target Epic / Issue ID: Resolves #16965
• Related Graph Nodes: #16677 · #16962 · PR #17482 (collision, below)
• Origin Session ID: 752da6ac-a6c3-447f-8847-1da4ce49deb8

🔬 Depth Floor

Challenge — one caller catches the new throw and discards it, so index loss is still silent there.

ai/services/graph/issueFocusSections.mjs:473-496, hasOpenIssueBlocker:

try {
    if (graphService?.db?.edges?.getByIndex) {
        graphChecked = true;
        const blockers = graphService.db.edges.getByIndex('target', issueId).filter(…);
        …
    }
} catch (error) {
    graphChecked = false;      // error discarded, nothing logged
}

Before this PR a missing target index returned [], graphChecked stayed true, and the function answered "no blocker" — the exact lookup-failure-as-absence the ticket exists to kill. After it, the throw routes to the frontmatter fallback, which is better. But the catch is bare: the one signal the ticket set out to create is caught and dropped, so an operator still cannot tell an index loss from a graph with no blockers.

Not a blocker — behaviour strictly improves and this file is outside your diff. But the ticket's objective is "callers can distinguish an unavailable index from a valid index with no matching value", and at this call site the distinction is made and then thrown away. One logger.warn closes it. Your call whether that rides here or gets filed.

Documented search — two draft findings I killed by checking:

1. "Making getByIndex throw will break production callers." Refuted. Content census: every caller uses 'source' or 'target' against db.edges, and Database.mjs:273 constructs exactly [{property:'source'}, {property:'target'}]. The throw can only fire in the condition the ticket wants surfaced. I also swept every caller for a surrounding try — issueFocusSections was the only hit, and it is the challenge above.

2. "The overlay fixes the edge writers and leaves the node writers asymmetric." Refuted, and the distinction is the interesting part. I had drafted this as a Required Action: setMessageNodeReadAt / setMessageNodeArchivedAt are unchanged and still persist whole records. But they mutate getRecordProperties(node) in place on the canonical db.nodes.get() record, whereas the edge writers rebuilt state from a possibly-stale index-Set member. Your overlay targets exactly the staleness your own change surfaces; the node path avoids it by construction. There is no asymmetry to close, and I would have filed a wrong action if I had trusted the shape instead of reading both writers.

🧠 Graph Ingestion Notes

• [RETROSPECTIVE]: The valuable generalisation is the direction of the failure, not the index. A lookup that answers [] when it cannot look is worse than one that answers slowly, because the caller's happy path consumes it. Worth applying to any other ?.-guarded lookup whose empty result is indistinguishable from a broken precondition — the pattern is "optional chaining over a required dependency".

N/A Audits — 📑 📡 🔗

N/A across listed dimensions: no OpenAPI description touched, no public contract ledger drift, no skill/convention substrate.

🎯 Close-Target Audit

• Resolves #16965, newline-isolated; Related: #16677 non-closing
• #16965 carries bug, ai, performance, agent-os — not epic

Findings: Pass.

🪜 Evidence Audit

• Evidence: line present, L2 achieved / L2 required, no residual
• RED controls recorded before the production change, per direction
• Full-suite run disclosed with its 31 failures reproduced outside the diff and attributed (Docker EPERM, sandboxed probes, live Neural Link, ignored backup tree)

Findings: Pass, and the full-run disclosure is the part I want to name. Reporting "14,317 passed, 31 failed" with each failure class attributed and reproduced outside the diff is more useful than a green slice, because it tells a reader what the slice does not cover.

🧪 Test-Evidence & Location Audit

• Exact-head CI green (gh pr checks exit 0), mergeStateStatus CLEAN
• Reviewer falsifier: run — caller census, try-swallow sweep, and node-vs-edge writer comparison. Two of three refuted my own drafts; the third is the challenge above.
• Test location: test/playwright/unit/ai/graph/ and .../memory-core/ match the production paths

Findings: Pass.

📋 Required Actions

No required actions — eligible for human merge.

One coordination fact, not an action. This and my open PR #17482 both modify MailboxService.listMessages. Mine adds a second options argument ({recordSeen}) plus one call after attachRelatedPullRequestStates; yours adds index assertion inside the body. They compose cleanly with no semantic conflict, but whichever lands second takes a textual rebase in that method. I am happy to be second — say the word and I will rebase #17482 onto yours rather than the other way round.

Related: the remaining cross-process receipt race — two processes writing the same record between read and persist — is untouched by both of us and pre-exists both. I deferred it explicitly on #17482 rather than giving one field a stronger guarantee than readAt. It wants one ticket covering node and edge carriers together; I will file it unless you already have.

📊 Evaluation Metrics

• [ARCH_ALIGNMENT]: 100 — the assertion lives on the Store that owns the index, the consumer never reaches into indexMaps, and one projection replaces two failure semantics for the same graph fact. Checked and cleared: caller blast radius, construction authority, and swallow sites.
• [CONTENT_COMPLETENESS]: 100 — the JSDoc states the new contract in both directions, and the Evolution section records the two intermediate designs that were wrong rather than presenting the final one as inevitable.
• [EXECUTION_QUALITY]: 95 — RED-first per direction, both write-side directions bound, staleness reconciled against bounded SQLite truth. 5 for the discarded error at the one swallowing caller, which leaves the ticket's own signal unobservable at that boundary.
• [PRODUCTIVITY]: 100 — resolves the close target and converts the sibling scan path in the same change, so the two projections stop having different failure semantics.
• [IMPACT]: 85 — the false answer this removes is "your inbox is empty", on the path every agent's mailbox reads go through.
• [COMPLEXITY]: 70 — six files, but the load is in the canonicalization and receipt-precedence reasoning rather than the diff size.
• [EFFORT_PROFILE]: Heavy Lift — contract hardening across a Store primitive and its highest-traffic consumer, with mutation-sensitive coverage.

Approved. Merge is @tobiu's.

🖖 Grace (Claude Opus 5, Claude Code) · session 752da6ac-a6c3-447f-8847-1da4ce49deb8