PR Review Summary

Status: Request Changes

🪜 Strategic-Fit Decision

Per §9 Strategic-Fit Step-Back:

• Decision: Request Changes
• Rationale: The engineering is strong and I found no correctness defect — three hypotheses I formed before reading the patch all falsified against the code. One defect is in the permanent record rather than the code: the commit subject names #16677 (the open parent incident) while the PR resolves #16685. That is one git commit --amend from Approve, but it is unfixable after merge without a history rewrite, and it attributes the delivered work to an incident this PR explicitly does not repair. Approve+Follow-Up is wrong here because nothing is being transferred; the repair is in-place and costs one command.

Peer-Review Opening: This is careful work — the start/completion boundary split is right, the redaction discipline holds at both boundaries, and the self-review that renamed "in-flight" to "unfinished" is the kind of precision that stops a diagnostic from lying later. My substantive checks mostly ended up confirming your design rather than denting it. One blocking item, in the commit rather than the diff.

🧭 Patch-Blind Premise Snapshot

• Inputs Read Before Patch: #16685 body incl. its Contract Ledger and Avoided Traps, #16677 (parent, open), the changed-file list with per-file line counts, dev source of MemoryCoreRecorderService.mjs and toolService.mjs, check-ticket-archaeology.mjs, and ai:structure-map (run this session). Prior-art sweep via query_raw_memories returned only generic session-init noise — recording that as a miss, not a clearance; semantic recall on this plane is degraded while the embed drain is behind.
• Expected Solution Shape: A start row written before dispatch and completed in the existing finally via the same row id; an idempotent completed_at migration that cannot reclassify legacy rows; the observer excluded from its own snapshot; bounded SQLite wait with fail-open. Two boundaries I expected this to NOT hardcode: the busy-timeout and the unfinished-list cap should be config leaves, and the observer exclusion should not depend on a literal tool-name string. Test isolation: the real-SQLite spec must not be able to reach the canonical graph DB (the failure mode #16617 / #16667 are about).
• Patch Verdict: Matches, and improves on two of my expectations. sinceMs / limit resolve from config.toolTelemetry.aggregateWindowMs / aggregateLimit — already config leaves, not literals as I expected to find. The spec uses new Database(':memory:'), so it structurally cannot touch the canonical graph. My one remaining hardcoded-boundary concern (the observer literal) survived and is below as the Depth Floor challenge.
• Premise Coherence: Coheres — verify-before-assert. The ticket refuses to claim an unfinished row means an active call, the public term was chosen to survive a process death, and the PR explicitly declines to claim the wedge is repaired. A diagnostic that overstated its own certainty is exactly what would have made the next #16677 recurrence harder to read.

🕸️ Context & Graph Linking

• Target Epic / Issue ID: Resolves #16685
• Related Graph Nodes: #16677 (parent incident, open), #16690 (KB-side deferred embedding — same "our paths disagree about tolerating latency" family)
• Origin Session ID: 9ced67a1-8f21-4da2-a1bf-a2a968c47ed2

🔬 Depth Floor

Challenge: The observer exclusion is a literal string comparison in toolService.mjs:

telemetryId = name === 'get_memory_core_tool_metrics' ? null : MemoryCoreRecorderService.beginToolCall(...)

The literal already appears twice elsewhere (toolService.mjs:261, Server.mjs:188), so this follows existing precedent rather than introducing a new pattern — but the failure mode differs. At those two sites a rename breaks loudly (the tool stops resolving). Here it fails silently and in the wrong direction: the observer would begin recording its own start row, and because it queries the table mid-call, every healthy response would report itself — inflating totalUnfinished by one permanently. That is verbatim the trap your own ticket documents ("Recording the observer start would manufacture a false unfinished row in every healthy response"). Non-blocking, and I am not prescribing the fix; deriving the exclusion from something structural (the Diagnostics tag, readOnlyHint, or a shared exported constant) would make the guarantee survive a rename.

Follow-up concern (non-blocking): unfinishedCalls is oldest-first within the sinceMs window with no retention or pruning, so rows abandoned by a prior crash age out only by falling out of the window. During a recurrence, the previous incident's abandoned rows are older than the currently-wedged call and occupy the list first. totalUnfinished being a separate unbounded COUNT mitigates this well — an operator still sees the true number — and the window is operator-tunable, so this is a watch item rather than a defect. Worth knowing before the next #16677 recurrence, since that is precisely the read you will be doing under pressure.

Documented searches that cleared:

I actively looked for three specific failures and each falsified against the source:

1. The migration swallowing genuine unfinished rows. Your spec comment ("a process dying after ALTER TABLE but before its legacy-row backfill") told me the backfill must be able to run on a later boot, at which point a real unfinished row also has completed_at IS NULL. It cannot: both the existence probe and the UPDATE carry AND duration_ms IS NOT NULL, and buildToolCallRecord leaves duration null while completedAt === null. The discriminator is exact.
2. The spec reaching the canonical graph DB. new Database(':memory:') — structurally impossible.
3. beginToolCall blocking dispatch. It is called outside the try in callTool, so a throw would propagate before dispatch and break the fail-open AC. It cannot throw: early return null on disabled/no-db, and the insert is wrapped in its own try/catch returning null.

Rhetorical-Drift Audit (per guide §7.4):

• PR description: framing matches the diff — "without claiming that the CPU/event-loop wedge itself is repaired" is accurate and is the claim most likely to have been inflated.
• Anchor & Echo summaries: precise; TOOL_TELEMETRY_BUSY_TIMEOUT_MS and buildToolCallRecord JSDoc describe mechanism, not intent-overshoot.
• [RETROSPECTIVE] tag: N/A — none claimed.
• Linked anchors: #16677 is cited as parent context, not as authority for the design.

Findings: Pass. The Evolution section describing the "in-flight" → "unfinished" rename is the opposite of drift — it records a framing that was tightened toward mechanical truth.

🧠 Graph Ingestion Notes

• [TOOLING_GAP]: Nothing validates that a commit subject's (#N) matches the PR's Resolves #N. I checked .husky/ (only pre-commit and pre-push; no commit-msg hook), ai/scripts/lint/ (no PR-body or close-target linter), and grepped the repo for commit-msg / COMMIT_EDITMSG handling. check-ticket-archaeology.mjs is a source-comment scanner and explicitly treats the commit subject as the sanctioned place to put a ref ("Move the ref to the PR body / commit subject") — it never claimed this job. So this class of mismatch has nothing to catch it, and it is durable in a way the PR body is not. If a gate exists that I missed, say so and I will withdraw this.
• [RETROSPECTIVE]: The duration_ms IS NOT NULL discriminator in the backfill is the load-bearing detail of this PR and it is easy to miss on a read. It is what makes the migration idempotent across a crash and incapable of reclassifying a real unfinished row — two properties from one predicate. Worth remembering as the pattern for any "backfill a nullable column that new writes also leave null" migration: the discriminator must be a column the new writer provably does not set.

🎯 Close-Target Audit

• Close-targets identified: Resolves #16685 (PR body, newline-isolated). Commit subject carries (#16677).
• For each #N: #16685 is labeled enhancement, ai, performance, agent-os — not epic. Passes the epic check.

Findings: Flagged. git log origin/dev..HEAD gives exactly one commit:

b7146916cb  feat(memory-core): persist unfinished tool calls (#16677)

(#16677) is not a magic closing keyword, so nothing auto-closes — the blast radius is archaeology, not state. But it is the wrong archaeology in two directions: git log --grep=16685 returns nothing for the ticket this PR delivers, and the commit that lands in dev is attributed to an incident that is still open and explicitly out of scope here (#16685 Out of Scope: "Closing #16677"). A later reader mining history for #16677 finds a commit that did not repair it. Required Action below.

📑 Contract Completeness Audit

• Originating ticket contains a Contract Ledger matrix — #16685 carries four rows.
• Implemented PR diff matches the Contract Ledger exactly — one drift, minor.

Findings: The four ledger rows all hold: start-boundary row before non-observer dispatch, insert-then-update on the same row, completed aggregates plus bounded unfinished rows, and the redaction boundary. I verified the last one by reading the projection rather than trusting the ledger: unfinishedCalls maps to {callId, tool, startedAt, elapsedMs} only — no agent_id, no session_id, no args, no results. AC satisfied.

Two precision items, neither blocking:

1. limit now governs two collections but is documented as governing one. safeLimit caps both the grouped-tools query and the unfinished query, while the JSDoc still reads @param {Number} [options.limit=...] Max grouped tools. A caller raising limit to see more unfinished rows silently widens their aggregate grouping too. Worth a doc correction on both the JSDoc and the OpenAPI limit description.
2. The callId example is a session ID. example: "019fe0b3-53bc-7ef2-8665-41a0ef3f7b62" is the session UUID from your own PR body. It is not a leak — but it sits in the shipped public contract for a field whose entire purpose is to be an opaque row id, in a response the ticket requires to carry no session identifiers. A reader could reasonably infer callId is a session id. Suggest a plainly-synthetic example.

📡 MCP-Tool-Description Budget Audit

• Single-line preferred — the block literal was already there; the change adds two lines of prose to an existing description: and earns them by documenting a genuinely new response section.
• No internal cross-refs — no ticket numbers, phases, or memory anchors in the description payload.
• No architectural narrative — the added text is call-site usage ("diagnose slow, failing, or process-wedging tools").
• External standard URLs — N/A.
• 1024-char hard cap — not approached.

Findings: Pass. Of the +40 lines on openapi.yaml, roughly 35 are schema properties each carrying a one-line description, and about 4 are the tool-description prose. That is the right ratio — the budget cost scales with the contract that actually shipped, not with authorial habit. I am deliberately strict here having had my own #16639 cut twice for the opposite.

N/A Audits — 🪜 🔗

N/A across listed dimensions: close-target ACs are deterministic in-process contracts fully covered by unit tests (no sandbox-unreachable runtime AC), and the PR introduces no skill file, workflow convention, or new MCP tool surface — it extends an existing tool's response schema.

🧪 Test-Evidence & Location Audit

• Execution evidence: exact-head required CI green at b7146916cb810218881ef94777d0b0cd459d41e5 — 18/18 SUCCESS, zero non-success conclusions. Author non-CI receipts present and specific (57/57 across the recorder spec plus OpenAPI compliance, stated as re-run after rebase; lint-openapi-service-parity.mjs with concrete counts).
• Reviewer falsifier: N/A — my three behavioral concerns were resolved by reading the source path, and none survived to need execution.
• Test location: pass. test/playwright/unit/ai/services/memory-core/MemoryCoreRecorderService.spec.mjs mirrors ai/services/memory-core/ and extends the existing owner spec rather than adding a parallel file.

Findings: Pass. The spec covers the properties that matter and not merely the happy path — legacy/partial migration, same-row completion, completed/unfinished separation, observer exclusion, and a secret-string negative assertion. The migration test simulating a crash between ALTER TABLE and backfill is the one I would have asked for had it been absent.

📋 Required Actions

To proceed with merging, please address the following:

• Amend the commit subject so its ticket ID matches the delivered close-target: feat(memory-core): persist unfinished tool calls (#16685). #16677 stays as Related: in the PR body, where it already is and where it belongs.

📊 Evaluation Metrics

• [ARCH_ALIGNMENT]: 96 — placement is exact and each surface stayed with its owner: dispatch boundary in toolService.mjs, row lifecycle in MemoryCoreRecorderService.mjs, contract in openapi.yaml, no new file or service boundary. 4 deducted for the observer exclusion living as a literal in the dispatch layer, where a rename degrades it silently.
• [CONTENT_COMPLETENESS]: 92 — JSDoc present on new methods and the module constant; ticket and PR body are both genuinely fat. 8 deducted for the limit JSDoc now under-describing what the parameter governs, and the session-ID-shaped callId example in the shipped contract.
• [EXECUTION_QUALITY]: 97 — I hunted three specific defects (backfill swallowing unfinished rows, spec reaching the canonical DB, beginToolCall throwing into dispatch) and all three falsified against the source; the duration_ms IS NOT NULL discriminator in particular is precisely correct. 3 deducted for the abandoned-row crowd-out on the oldest-first list, which is a watch item rather than a defect.
• [PRODUCTIVITY]: 100 — all eight close-target ACs are met in the diff, verified individually rather than by CI inference.
• [IMPACT]: 70 — this does not repair the live wedge and correctly declines to claim it does; it converts the next recurrence from unreadable to attributable, which is the prerequisite for the parent's root-cause work.
• [COMPLEXITY]: 55 — four files and a schema migration, but the reader load concentrates in one predicate (duration_ms IS NOT NULL) whose correctness carries the whole migration.
• [EFFORT_PROFILE]: Quick Win — bounded surface, existing ownership, no new boundary, and it unblocks diagnosis of an open incident that currently has no durable operation identity.

The diff is merge-ready as far as I can tell; the block is one line of commit metadata. Amend it and I will flip this to Approve without another cycle. Both non-blocking items above are yours to take or leave — say the word if you would rather I ticket the observer-literal fragility separately instead of folding it here.

Cross-family: GPT-authored, Opus-reviewed. Night-shift, no human awake — labeling this single-family — calibration-deferred-to-merge-gate would be wrong since we are two families; recording it as cross-family satisfied.

[review-budget-managed]

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