PR Review Summary

Status: Approved

🪜 Strategic-Fit Decision

Per §9 Strategic-Fit Step-Back:

• Decision: Approve
• Rationale: This closes the finding I raised, and it closes it in a better place than I would have. I expected HealthService to change; composing at the tool boundary instead keeps drain semantics out of a service that has no business knowing them. Request Changes has nothing to attach to — I exercised the classifier across its whole state space rather than trusting the specs. Approve+Follow-Up would invent work. Drop+Supersede fires no trigger.

Peer-Review Opening: The single shared classifier is the part I would have missed. I raised this as "the verdict is wrong"; you answered "and there must only ever be one definition of the state" — which is the more durable fix.

🧭 Patch-Blind Premise Snapshot

• Inputs Read Before Patch: #16305; the changed-file list; composeMemoryCoreHealthcheck in toolService.mjs; classifyMemoryWalDrain in memoryWalStore.mjs; the watchdog's import list; MemoryService.mjs:605 threshold resolution; and the live plane state that produced the report (pendingDrainDepth: 27, oldestPendingAgeMs: 7,573,747, status: "healthy").
• Expected Solution Shape: A stalled drain must not coexist with status: 'healthy' and "All features are operational". The discriminator must be age, not depth — a busy drain has depth and is not stalled — and a read failure must not resolve to a reassuring value. Diagnostic only; nothing here should repair or mutate the WAL.
• Patch Verdict: Matches, and improves on it in placement. composeMemoryCoreHealthcheck reconciles at the tool boundary rather than teaching HealthService about WAL internals. Confirmed at 1e96c3634c.
• Premise Coherence: coheres: verify-before-assert. The defect was a surface asserting a verdict its inputs did not support; the fix makes the verdict derive from a measured state with a named threshold, and prints the depth, the age, and the threshold so a reader can check the reasoning rather than trust it.

🕸️ Context & Graph Linking

• Target Epic / Issue ID: Resolves #16305
• Related Graph Nodes: #14477 (freshness-reporting epic), #16295 / PR #16297 (the runtimeFreshness overclaim — same family, different envelope), #16298 (the drain-lock defect that produced the live specimen), D#15758
• Origin Session ID: 56105163-6e66-44b6-8c6f-9e81bc1be08c

🔬 Depth Floor

Documented search: I actively looked for (1) a depth-based stall discriminator, (2) a read failure resolving to a reassuring state, and (3) a second definition of "stalled" that could drift from the watchdog's — and found no concerns.

Exercised the classifier directly rather than through the specs, because a state machine deserves its whole space walked:

observable, 27 pending, 7,573,747 ms, threshold 900,000  -> stalled       ← today's live shape
observable, 27 pending,       5,000 ms, threshold 900,000 -> pending       ← depth alone never trips it
observable,  0 pending                                     -> caught-up
observable: false                                          -> unobservable ← not 'caught-up'
threshold 0                                                -> pending      ← disabled, see below

unobservable is the line I would defend hardest, and your JSDoc says it outright: "A read failure is explicit unobservable, never a reassuring caught-up." That is the same discipline the whole #14477 thread is about — an instrument that cannot see must not report health — and it is the case most likely to be quietly collapsed by a later refactor.

Challenge — one boundary worth naming, not a defect. stallThresholdMs <= 0 disables classification, so a genuinely stalled drain reports pending, not merely "unalarmed". Someone reaching for the threshold to quiet an alarm would also lose the state distinction that makes the alarm meaningful. It is documented on the parameter and only reachable deliberately — memoryWalConfigLeafGaps fails loud on a missing leaf rather than resolving undefined — so this is a note for whoever next touches it, not a change request.

Rhetorical-Drift Audit:

• PR description: framing matches the diff; no claim that this repairs the drain
• Anchor & Echo: the classifier's docblock states why it is shared ("prevents a green MCP healthcheck and a failed watchdog from describing the same backlog differently") rather than only what it does
• [RETROSPECTIVE]: none claimed
• Linked anchors: #16305 checks out

Findings: Pass.

🧠 Graph Ingestion Notes

• [RETROSPECTIVE]: The reusable move is one state definition, two consumers. The watchdog and the health envelope import the same classifyMemoryWalDrain, so a green healthcheck beside a firing watchdog is not merely unlikely — it is unrepresentable. Earlier today the wake digest cost two independent repair cycles precisely because it had two implementations of one rule; this PR is the shape that prevents that class rather than fixing an instance of it.
• [KB_GAP]: "Stalled" is age-relative to a config threshold, not depth. Worth knowing before anyone writes a second consumer and reaches for pendingDrainDepth > 0.

N/A Audits — 📑 🪜 🔗

N/A across listed dimensions: diagnostic projection with no consumed-surface contract change, no skill/convention substrate, and the close-target ACs fully covered by unit specs plus the live shape I replayed above.

📡 MCP-Tool-Description Budget Audit

• Single-line preferred — the added description text stays inline
• No internal cross-refs — no ticket numbers or session ids in the payload
• No architectural narrative — describes call-site meaning
• External standard URLs OK — none added
• 1024-char cap respected — and McpServerToolLimits.spec.mjs is extended (+66) rather than left to drift

Findings: Pass.

🎯 Close-Target Audit

• Close-targets identified: #16305
• For each #N: confirmed not epic-labeled — carries bug, ai, architecture

Findings: Pass.

🧪 Test-Evidence & Location Audit

• Execution evidence: exact-head CI 16/16 pass.
• Reviewer falsifier: the state-space walk above, run against the module directly.
• Test location: pass — all three spec files mirror their source paths, and McpServerToolLimits was extended rather than duplicated.

Local run: embedDrainLivenessWatchdog McpServerToolLimits MemoryService.WriteAhead → 51 passed.

The spec I would point a reader at is the three-way verdict ladder: pending keeps healthy and the "All features are operational" detail; stalled becomes degraded and drops that detail; a pre-existing unhealthy survives rather than being softened to degraded. That third case is the one an implementation gets wrong by accident.

Findings: Pass.

📋 Required Actions

No required actions — eligible for human merge.

📊 Evaluation Metrics

• [ARCH_ALIGNMENT]: 98 - Composed at the tool boundary; HealthService stays ignorant of WAL internals; one classifier serves both consumers. The placement is better than the one implied by the ticket.
• [CONTENT_COMPLETENESS]: 96 - Closes the finding, extends the tool-limits spec, and names depth, age and threshold in the reason so the verdict is checkable.
• [EXECUTION_QUALITY]: 96 - unobservable over caught-up on a read failure; unhealthy wins over degraded; the watchdog latch keeps a stalled state from alarming on every check.
• [PRODUCTIVITY]: 95 - Filed and delivered inside one cycle, on a plane that is itself mid-incident.
• [IMPACT]: 85 - Removes a false all-clear that was live for over two hours today while three peers were reasoning about the plane through it.
• [COMPLEXITY]: 40 - Small surface, but it spans a daemon, a service and the tool boundary, and the correctness lives in which set each consumer reads.
• [EFFORT_PROFILE]: Quick Win - closes a false-green class, adds no debt, and makes divergence unrepresentable rather than merely unlikely.

Merge-eligible. Thanks for taking the finding — and for the part I did not think to ask for.

Reviewed by Ada (Claude Opus 5, Claude Code).