PR Review Summary

Status: Request Changes

🪜 Strategic-Fit Decision

Per §9 Strategic-Fit Step-Back:

• Decision: Request Changes
• Rationale: WAL-first persistence ordering is correct, and bounding the derived response stages is the right repair seam. The synchronous mailbox query is still executed on the response path before any bound can act, so the patch does not yet establish the end-to-end response contract it claims.

Peer-Review Opening: The WAL anchor and the asynchronous presence/visibility bounds are useful. One response-path invariant remains open, and the exact-head helper disables its own cooldown in the worst case.

🧭 Patch-Blind Premise Snapshot

• Inputs Read Before Patch: #16808 and its live evidence/comments; current MemoryService.addMemory; MemoryResponse OpenAPI; the new helper and both changed specs.
• Expected Solution Shape: Once the WAL append returns, every remaining response dependency is mechanically bounded or removed from the critical path; any degraded envelope is public-contract documented and proven through real addMemory execution.
• Patch Verdict: Persistence order passes: appendWalMemory completes before projection, mailbox, presence, and visibility work. Presence and visibility have real asynchronous bounds. buildMailboxDelta remains a synchronous better-sqlite3 query on the response path, and runSelfDegradingSyncCall can only measure after it returns. The first slow request therefore remains unbounded.
• Premise Coherence: A retrospective observation can protect later calls, but it cannot be described as a deadline for the request that paid the blocking work.

🕸️ Context & Graph Linking

• Target Epic / Issue ID: #16808
• Related Graph Nodes: #16706, #16780, Memory Core WAL and mailbox-delta path
• Origin Session ID: 7b51208b-bfd4-4372-94c6-49f6242e709d

🔬 Depth Floor

Challenge OR documented search (per guide §7.1):

• Challenge: Executing the exact-head helper with a 61,000 ms synchronous call and a 60,000 ms cooldown produced durationMs 61000 and skipUntil 60000. The immediate call at 61001 executed again, because skipUntil is computed from call start rather than completion. More importantly, MemoryService lines 572-577 still call buildMailboxDelta synchronously before any response can return, so the first slow call can reproduce the timeout-but-STORED symptom regardless of the later cooldown.

Rhetorical-Drift Audit (per guide §7.4):

• “The RESPONSE must never be held hostage” is not mechanically true while the synchronous query remains on that path.
• MemoryResponse OpenAPI does not describe stageTimings or the new cooldown meaning of mailbox null.
• addMemory JSDoc does not declare the new response envelope.
• No retrospective tag was added.

🧠 Graph Ingestion Notes

• [KB_GAP]: A retrospective sync guard is load shedding for later calls, not a deadline for the current call.
• [TOOLING_GAP]: The real addMemory spec checks only field types; it never forces mailbox overrun, presence timeout, visibility timeout, or an end-to-end return budget.
• [RETROSPECTIVE]: Response contracts must be proved at the consumed method boundary, not inferred from isolated helper branches.

🎯 Close-Target Audit

• Resolves #16808 overclaims this slice: the PR and ticket comments explicitly retain timeout-but-LOST and structural mailbox-query work on the same still-open ticket.

Findings: Align the close target with the actual delivered slice in the same repair; this is collateral to the behavioral action, not a separate return-cycle reason.

📑 Contract Completeness Audit

• #16808 has no Contract Ledger or explicit AC matrix for the consumed response change.
• stageTimings and the new mailbox-null reason are absent from OpenAPI/JSDoc.

Findings: Backfill the contract while repairing the behavior so the next review has one executable authority.

🧪 Test-Evidence & Location Audit

• Exact-head hosted CI is fully green at 49712f7c8d2311f987301f4821520719a6aacd91.
• Exact-head falsifier failed: a call longer than the cooldown immediately re-enters the producer.
• Production reach is unproved: no real addMemory test forces the three degraded branches or asserts return within one declared budget.

📋 Required Actions

• RA-1 — Make the post-WAL response contract genuinely end-to-end bounded. Remove, precompute, or cache the synchronous mailbox CTE off addMemory’s critical response path; a guard that learns only after the blocking call returns is not a deadline. If the retrospective guard remains anywhere, arm its cooldown from completion so a call longer than cooldown cannot disable its own protection. Add a real addMemory test that forces mailbox slow/skip behavior plus presence and visibility overruns and proves the method returns the exact degraded envelope within one declared consumer-owned budget after WAL acceptance. In the same repair, give #16808 an executable Contract Ledger, align MemoryResponse OpenAPI and addMemory JSDoc with stageTimings/mailbox-null semantics, and change the close target so this bounded slice does not claim the ticket’s explicitly retained lost-write residuals.

📊 Evaluation Metrics

• [ARCH_ALIGNMENT]: 61 - Correct WAL anchor and async-bound direction; the synchronous response dependency remains structurally unbounded.
• [CONTENT_COMPLETENESS]: 48 - Public response contract and ticket ledger are absent.
• [EXECUTION_QUALITY]: 55 - Green CI, but the exact helper and real response path falsify the central bound.
• [PRODUCTIVITY]: 68 - The patch contains useful salvage and is repairable in place.
• [IMPACT]: 94 - A client timeout after durable acceptance is interpreted as write loss and drives duplicate/recovery behavior.
• [COMPLEXITY]: 70 - Removing the synchronous query from the critical path needs one clear ownership choice and end-to-end evidence.
• [EFFORT_PROFILE]: Maintenance - Tighten an existing write response path and its contract.

Keep the WAL-first and asynchronous-budget work. Close the one synchronous escape hatch, and the instrumentation will describe a response contract that the implementation can actually uphold.

[review-budget-managed]

• outcome: within-budget
• ordinary-limit: 2
• activation-issue: 15257
• activation-pr: 15307
• activated-at: 2026-07-16T20:54:31Z