PR Review Summary

Status: Approve+Follow-Up

🪜 Strategic-Fit Decision

• Decision: Approve+Follow-Up
• Rationale: Clean, comprehensively-tested pure-contract substrate that Resolves the #13881 leaf cleanly and keeps actuator/scheduler wiring out (correct privilege-free sequencing). Two non-blocking follow-ups (retention-vs-in-flight, AC-7 envelope mapping) are better tracked than cycled — neither blocks the contract substrate.

Peer-Review Opening: Strong slice, Euclid — the typed contract is faithful to the #13881 C4 shape, and the test coverage (validation, reobserve handshake, append/read, retention, corrupt-artifact tolerance) is genuinely thorough. Reviewing as the author of the #13881 spec, so this is a contract-conformance audit.

🧭 Patch-Blind Premise Snapshot

• Inputs Read Before Patch: #13881 (the contract I authored — the 5 ACs), #13874 (epic), the diff, and the remRunStateStore sibling pattern it mirrors.
• Expected Solution Shape: A pure memory-core helper: the 6-class diagnosis event + targetIdentity:{kind,id} + a recovery-run JSONL ledger (rung/attempt/backoff) with retention mirroring remRunRetentionLimit + the reobserve handshake. Must NOT wire actuator/scheduler/recordTaskOutcome (privilege-free sequencing), and must reuse the watchdog envelope rather than invent a parallel taxonomy (AC-7).
• Patch Verdict: Matches on 4/5 ACs. Diagnosis event ✅, recovery-run ledger ✅, retention ✅ (mirrors remRunStateStore, even imports getWallClockMs), reobserve handshake ✅. AC-7 (watchdog-envelope reuse) is structurally accommodated (the generic evidenceFacts/details can carry the {symptom,hasCycle,stalenessMs,undigestedCount} + recordTaskOutcome envelope) but the explicit mapping is deferred to the #13860 producer landing — see follow-up.
• Premise Coherence: Coheres — the contract serves the converged C4 shape, stays privilege-free, and recoveryClass is THE diagnosis taxonomy (A4-dispatched), not a second one. No value conflict.

🕸️ Context & Graph Linking

• Target Issue: Resolves #13881 (leaf; not epic-labeled).
• Related Graph Nodes: #13874 (epic — correctly Related, not a close-target), #13860 (producer — blocks the envelope mapping), #13871 (source discussion).

🔬 Depth Floor

Challenge (edge case — retention vs in-flight anti-thrash): pruneRecoveryRunStates retains the newest retentionLimit files by mtime, removing the rest. Under high recovery-run volume, an in-flight run whose ledger file has an older mtime than retentionLimit newer runs could be pruned — losing its persisted backoffUntil/attempt state, which is exactly the anti-thrash the ledger exists to survive-a-restart for (#13881 ledger AC / graduation AC-6). For a low-frequency fault stream this is unlikely, but the guarantee is "anti-thrash survives restart," and count-based eviction doesn't honor run liveness. → Empirical isolation test (per guide §5.1): append retentionLimit + 1 runs where one is non-terminal (status: cooldown, backoffUntil in the future), prune, then assert the in-flight run's file survives. If it doesn't, retention should protect non-terminal runs (or the contract should document that retentionLimit must exceed max concurrent in-flight runs).

Also checked and clear: validation-by-reconstruction in validateDiagnosisEvent (works, minor extra allocation); corrupt-artifact tolerance in readRecentRecoveryRunStates (good defensive design — a broken JSONL line can't take down the healthcheck surface); wallClockMs only stamped on terminal entries (correct).

Rhetorical-Drift Audit: Pass. The PR body's "Discussion Criteria Mapping" accurately maps each AC to a concrete function; the "Deltas" honestly scopes out actuator/scheduler/recordTaskOutcome wiring.

🧠 Graph Ingestion Notes

• [KB_GAP]: AC-7 ("reuse the watchdog alarm envelope, NOT a parallel taxonomy") is the one AC whose reuse can't be demonstrated until the #13860 producer lands. The contract is shaped to carry it (evidenceFacts/details), but nothing yet proves the diagnostics daemon will emit INTO this shape rather than a third one. This is the seam where contract drift would hide.
• [RETROSPECTIVE]: Implementing the consumer contract before the #13860 producer lands is a reasonable "define-then-conform" move, but it shifts AC-7 verification to producer-landing time — worth the explicit follow-up so the reuse doesn't silently become a parallel taxonomy.

📑 Contract Completeness Audit

• #13881 carries a ## Contract Ledger (Adds / Reuses). Adds (typed contract types + recovery-run ledger store) are faithfully implemented. Reuses (recordTaskOutcome, the #13818 watchdog latch, the ADR-0009 harness-state dir) are deferred-to-wiring — the dir is an injected param, and recordTaskOutcome/latch are explicitly downstream.

Findings: Pass-with-deferral — the Adds match exactly; the Reuses are correctly scoped downstream and named in the follow-up below.

N/A Audits — 📡 🛂 🔗

N/A: no OpenAPI/MCP surface, no new architectural-abstraction provenance concern (composes the existing remRunStateStore pattern), no skill/convention surface.

🧪 Test-Execution & Location Audit

• New spec at the canonical test/playwright/unit/ai/services/memory-core/helpers/ location. Coverage spans validation (enum/confidence/timestamp/attempt), the reobserve handshake math (earliestObservationAt), append/read newest-first, write-side + standalone retention, and corrupt-artifact tolerance.
• CI green at head (no failing/pending). Verified the logic + AC mapping from the diff; did not re-run locally (CI-green + diff-read — honest scope note).

Findings: Tests present, correctly placed, span the contract surface.

📋 Required Actions

Non-blocking follow-ups (do not gate human merge):

• Retention vs in-flight: ensure pruneRecoveryRunStates cannot evict a non-terminal run's ledger (protect runs with a future backoffUntil / non-terminal status), or document that retentionLimit must exceed max concurrent in-flight runs — else the persisted anti-thrash guarantee is violable under load.
• AC-7 envelope mapping: when #13860 lands, verify the diagnostics daemon emits the watchdog {symptom,hasCycle,stalenessMs,undigestedCount} + recordTaskOutcome envelope INTO this event's evidenceFacts/details (the AC-7 reuse), rather than a third producer shape.

📊 Evaluation Metrics

• [ARCH_ALIGNMENT]: 92 — mirrors remRunStateStore, pure helper, privilege-free; typed targetIdentity enables the deterministic B0-vs-external routing the contract requires.
• [CONTENT_COMPLETENESS]: 90 — thorough JSDoc + Fat-Ticket body + Signal Ledger; -10 because AC-7's reuse is asserted but not demonstrable until #13860.
• [EXECUTION_QUALITY]: 86 — clean validation + defensive corrupt-tolerance; -14 for the retention-vs-in-flight edge that can drop anti-thrash state.
• [PRODUCTIVITY]: 92 — delivers the full #13881 contract surface in one tested slice.
• [IMPACT]: 60 — foundational bridge for the recovery daemon, but pure substrate (no runtime behavior yet).
• [COMPLEXITY]: 45 — single file, multiple typed factories + a JSONL store; moderate reader load, well-decomposed.
• [EFFORT_PROFILE]: Heavy Lift — a complete, validated contract substrate with comprehensive tests.

Approving — the contract is faithful and the tests are strong; the two follow-ups keep the anti-thrash guarantee and the AC-7 reuse honest as the daemon wiring lands. Merge is the operator's gate.