Memory review cadence clears the turn counter on spawn, so a failed reviewer run silently drops its transcript window (239 exchanges measured)

[jacobo-ortiz]

Summary

hooks/MemoryReviewFire.hook.ts clears the review cadence counter immediately after spawning MemoryReviewer.ts, before the reviewer's outcome is known. Because the reviewer window is sized to exactly the un-reviewed span (--turns <accumulated>, no overlap by design), a run that fails leaves its span outside every future window. Those exchanges are never curated by anything.

On one install running v7.40.4 (upgraded from v7.28.3), this dropped 239 exchanges across 31 failed runs between 2026-07-21 and 2026-08-19 — 218 of them from inference timeouts.

Mechanism

// hooks/MemoryReviewFire.hook.ts
const { spawned, reason } = spawnReviewer(turnsReviewed, transcriptPath);
logFire({ ... });
session.turn_count_since_last_review = 0;   // ← unconditional
session.last_review_at = now;                // ← unconditional

spawnReviewer is fire-and-forget: it returns whether the spawn succeeded, not whether the review did. The reviewer runs detached for 60–240s — far longer than a Stop hook can wait. When it later fails, nothing reconciles the bookkeeping.

Two properties make the loss total rather than partial:

1. The window has no overlap. spawnReviewer passes --turns turnsReviewed, and extractRecentExchanges does exchanges.slice(-maxExchanges). The window is exactly the span since the last fire.
2. There is no watermark. review-state.json carries turn_count_since_last_review, last_review_at, last_message_at, pending_review — no cursor, no last-reviewed message id. Nothing records which exchanges were actually curated.

Grepping the reviewer for watermark|cursor|lastProcessedTs|sinceTs returns 0.

Measurement

LIFEOS/MEMORY/OBSERVABILITY/reviewer-runs.jsonl already records ok and exchanges per run, so the loss is directly countable:

total runs	188
failed runs	31
exchanges in failed runs (never curated)	239
of those, inference timeouts	27 runs / 218 exchanges
median exchanges per run (ok and failed alike)	8–9

Timeout rate rose with inference latency over the same period: weekly p50 went 25s → 46s → 64s → 82s → 116s with the per-run exchange count flat at 8–9, so this is latency drift rather than heavier sessions. DEFAULT_TIMEOUT_MS was raised 120s → 240s in v7.40.4; the comment says "successful runs measure 50–115s", and on this install the p50 is now 116s with a slowest successful run of 227s. Raising the ceiling again postpones the symptom; the dropped window is the defect.

Suggested fix

The artifact needed already exists — reviewer-runs.jsonl is written when the reviewer finishes, and the next Stop fires minutes later. The hook can read the previous run's outcome and restore its turns:

• Keep a compensated_run_id in the per-session state so a failed run is compensated exactly once.
• Only compensate when the failed run's transcript matches this session's, so one session's loss doesn't inflate another's counter.
• Cap the window that gets sent (min(accumulated, MAX)) and subtract what was sent instead of zeroing, so a backlog drains over several fires rather than being silently truncated. Without a cap, restoring turns enlarges the next window, and a larger window means more latency — the very thing that caused the timeout.

Unrelated but blocking a test

MemoryReviewFire.hook.ts ends in a bare main(); with no import.meta.main guard, so importing it to exercise its logic executes the hook and calls process.exit(0). A test importing it dies before its first assertion and exits 0 — a green that measured nothing. Most hooks in the tree already use the guard (AlgorithmNudge, FormatGate, EgressClassGuard, ComplexityRatchet, LoopDetector, CommunicationSkillGuard, CheckpointPerISC, DeployRegistrationGate); this one and MemoryReviewer.ts's hook path do not.