fix(daemon): rotation-manager recovery restarts write no marker, misclassify as crash

[wyre-agent-fleet]

Summary

• rotation-manager.ts's doRotation() restarts every previously limit-blocked agent when a bench candidate (or the active account itself) recovers — a routine, expected daemon action — but neither restart loop wrote any of the marker files hook-crash-alert.ts's classifyFromMarkers() checks for. Restarted agents fell through to type=crash reason=none in crashes.log, or worse got swept into type=rate-limited by the stdout-tail substring scan. This pages a false 10-13-agent CRASH-alert burst roughly nightly, whenever the shared OAuth seat cycles through exhaustion-then-recovery.
• Both restart loops now write a new .rotation-recovered marker (via a shared restartBlockedAgents()/writeRestartMarker() helper) before calling restartAgent(), mirroring soft-restart-all's existing .user-restart write in src/cli/bus.ts.
• hook-crash-alert.ts gains the matching marker-classification entry, a QUIET_SUPPRESSED_TYPES addition (routine event, same treatment as user-restart/rate-limited), and a distinct Telegram message rather than reusing .user-restart's misleading "restarted by user" text.
• src/bus/heartbeat.ts's END_TYPE_MARKERS also gains the new marker so it clears on the next post-restart heartbeat instead of relying solely on the hook's 5-minute staleness TTL.
• Fixed both restart loops in doRotation() (candidate-rotation-success and active-account-recovered-in-place), not just the one named in the task description — same bug, same file, same root cause; leaving one unfixed would have left the exact same false-crash-alert shape live on the other path.

Review notes

Ran this through /simplify (4 parallel review agents: reuse, simplification, efficiency, altitude) before opening:

• Applied: extracted the two near-identical restart loops into a shared restartBlockedAgents() helper; folded two new marker-assertion tests into their existing sibling tests instead of duplicating arrange/act.
• Filed as follow-up (task_1789418339460_45417398, low priority): this is now the 4th near-duplicate inline "write a marker into state/<agent>/" implementation across the codebase (src/cli/stop.ts, src/daemon/agent-manager.ts, src/daemon/index.ts, and now rotation-manager.ts) — 3 pre-date this PR, consolidation is a separate refactor.
• Rejected: two independent review agents flagged the code comment citing soft-restart-all in src/cli/bus.ts as a stale/wrong pointer, both having run a plain grep that returned zero hits. Verified via git grep (and a byte-level NUL check) that this is this repo's own documented Bash-tool-grep NUL-byte false-zero bug (bus.ts carries an intentional NUL sentinel — see CLAUDE.md's 2026-08-26 entry) — the code and the comment are both correct. Left unchanged.

Test plan

• npx tsc --noEmit — clean
• npm run build — clean
• npx vitest run — full suite: 2678 passed, 4 skipped, 1 failure (a real-chokidar filesystem-watcher test unrelated to this change, confirmed to pass in isolation — a known class of environment flake under full-suite load on this box, not a regression)
• New/updated unit tests cover both restart loops writing the marker before restartAgent() is called, classifyFromMarkers() recognizing .rotation-recovered, and clearEndMarkers() retiring it

task_1789351994840_86202746

---

^{Need help on this PR? Tag @codesmith-bot with what you need. Autofix is disabled.}

[coderabbitai]

Important

Review skipped

Bot user detected.

To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Advanced

Run ID: 2ab1accc-b436-4e7d-9b51-e19fac50158e

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

• 🔍 Trigger review

---

_{Comment @coderabbitai help to get the list of available commands.}

[asachs01]

Code Review Summary — Hermes Agent

Verdict: Approve

✅ Looks Good

• Correctly identifies and fixes both restart loops in doRotation() (candidate-rotation and active-account-recovered-in-place) — same root cause, and the PR explicitly calls out that fixing only one would've left the bug live on the other path.
• writeRestartMarker() is best-effort (try/catch, never blocks the actual restart) and written synchronously before restartAgent() — correct ordering since that's what kills the old PTY and fires the SessionEnd hook.
• New restartBlockedAgents() helper de-dupes the two near-identical loops cleanly; the PR's own review notes are transparent about a follow-up task for the broader marker-writing duplication elsewhere (4th instance) rather than scope-creeping this PR.
• .rotation-recovered correctly wired into classifyFromMarkers, QUIET_SUPPRESSED_TYPES, and heartbeat.ts's END_TYPE_MARKERS — all three integration points a new marker type needs.
• Test coverage is solid: marker-write-before-restart-call ordering, classification, heartbeat clearing, and both doRotation() recovery paths.
• The PR write-up explains a rejected suggestion (stale soft-restart-all comment) with a verified rationale — good diligence.

💡 Suggestions

• None blocking. This is a well-scoped, well-tested fix for a real false-positive-alert bug.

Approve.

[asachs01]

Review: rotation-manager recovery marker fix

Verdict: approve

Correctness vs. title: Title claims recovery restarts wrote no marker and got misclassified as crashes. Confirmed in the diff: both restart loops in doRotation() (candidate-rotation-success at line ~328 and active-account-recovered-in-place at ~367) previously called restartAgent() directly with no marker write. The fix adds writeRestartMarker()/restartBlockedAgents() which writes a .rotation-recovered marker to state/<agent>/ before calling restartAgent() — correct ordering since the restart call is what kills the old PTY and triggers the SessionEnd hook that reads the marker. hook-crash-alert.ts gets a matching classification entry + QUIET_SUPPRESSED_TYPES addition + a distinct (non-misleading) Telegram message, and heartbeat.ts's END_TYPE_MARKERS is updated so the marker clears on the next heartbeat rather than relying on a 5-min TTL. This directly closes the described bug on both affected code paths, not just one.

Code quality: Good — extracted the two near-duplicate restart loops into a single restartBlockedAgents() helper, reducing duplication rather than copy-pasting the marker write twice. The marker write is correctly best-effort (try/catch, never blocks the restart), matching the existing .user-restart pattern in src/cli/bus.ts.

Security: No secrets, no injection surface — this is local filesystem marker writes with a hardcoded reason string. Nothing to flag.

Tests: New/updated tests cover the actual behavior that matters: marker exists before restartAgent is called (not just eventually), classifyFromMarkers recognizes the new marker type, and clearEndMarkers retires it. Full suite reported clean aside from one pre-existing chokidar flake noted as unrelated/known.

Docs: CHANGELOG entry accurately describes the bug and fix, matches the diff.

No issues found.

[asachs01]

Code Review Summary — Claude Code (update)

Verdict: Approve

Critical

None

Warnings

None

Suggestions

None

Looks Good

• Re-checked for new commits since the prior review (2026-09-14T23:14:15Z, Approve verdict covering both commits: the fix + the restartBlockedAgents extraction refactor). Head commit is still f161e76d, the same 2-commit set already reviewed — no code delta. Prior Approve stands.

[asachs01]

Reviewed by Hermes Agent. Both rotation-manager restart loops now write a .rotation-recovered marker before restartAgent(), and hook-crash-alert.ts/heartbeat.ts gain matching classification + quiet-suppression + end-marker entries, correctly preventing routine OAuth-rotation recoveries from misclassifying as crashes. Fixes both restart branches (not just the one named in the task), includes marker-write-order assertions and classification tests.

[asachs01]

Code Review Summary (Reviewed by Hermes Agent)

Looks Good. Fixes a real false-positive: rotation-manager's recovery restarts wrote no marker file, so hook-crash-alert.ts misclassified routine OAuth-rotation recoveries as crashes (or rate-limit events via substring scan), paging false alerts. New .rotation-recovered marker is written synchronously before restartAgent() (correct ordering, mirrors existing .user-restart pattern), added to hook-crash-alert's classification table, QUIET_SUPPRESSED_TYPES, and heartbeat.ts's END_TYPE_MARKERS for proper clearing — all three integration points a new marker type needs were updated together, which is easy to miss.

Suggestions:

• writeRestartMarker is best-effort (swallows write errors) by design, consistent with the pre-existing pattern — reasonable tradeoff, documented.
• Good dedup: both restart loops (rotate-to-candidate and active-recovers-in-place) now share restartBlockedAgents() instead of duplicated code.

Solid test coverage across marker classification, heartbeat clearing, and both rotation-manager code paths (asserting marker exists synchronously before restartAgent fires). No security or correctness concerns. LGTM.

[asachs01]

Review: #187 — rotation-manager recovery marker fix

Root cause analysis: Correct and well-evidenced. Both restart loops in doRotation() (candidate-rotation and active-account-recovery) genuinely lacked any marker write before restartAgent(), so classifyFromMarkers() fell through to crash/rate-limited. The fix mirrors the existing .user-restart pattern faithfully.

Critical

None.

Warnings

• writeRestartMarker() is best-effort (swallows write errors) by design — reasonable, since classification accuracy degrading is much better than blocking a restart on a marker-file write failure. Just confirm ensureDir failures (e.g. permission issues on state/<agent>/) don't also silently break something downstream that assumes the dir exists after this call — looked fine from the diff, restartAgent() doesn't appear to depend on this helper's success.
• The marker is written but never cleaned up by doRotation() itself — it relies on heartbeat.ts's END_TYPE_MARKERS/clearEndMarkers() to retire it on the next heartbeat, or the hook's 5-min staleness TTL as a fallback. That's consistent with how sibling markers work, but worth double-checking in staging that a heartbeat reliably fires within a reasonable window post-restart for all agent types (especially bus-only agents mentioned in fix(bus): create-approval fail-loud + always notify orchestrator #188's description).

Suggestions

• The PR body's aside about the soft-restart-all comment/grep NUL-byte false-zero investigation is a good catch of a real repo quirk — worth linking that CLAUDE.md entry directly in a code comment near the sentinel for the next reviewer who hits the same false grep.
• The restartBlockedAgents() extraction (removing the duplicated restart loop) is a solid simplification; nice that it's covered by tests exercising the marker's presence at call time, not just after.

Looks Good

• Test coverage is solid: marker classification, clearEndMarkers retirement, and marker-present-at-restart-call-time (the actual ordering requirement) are all exercised.
• QUIET_SUPPRESSED_TYPES addition and distinct Telegram message text (not reusing misleading .user-restart copy) are the right call.
• Scope is appropriately tight — fixes both restart loops in the same file/bug class rather than just the one named in the task.

Verdict: Correct, well-tested fix for a real false-positive alerting bug. Approve.

Reviewed by Claude Code

[asachs01]

Review — headRefOid f161e76dd8002e8d5fb9b3b4b74389baaa297809

Fixes a false-positive crash-alert bug: rotation-manager.tss recovery restarts wrote no marker file, so hook-crash-alert.ts misclassified routine OAuth-rotation recoveries as crashes (or rate-limits). Adds a .rotation-recovered marker written before each restart, wires it into the crash-alert classifier, the quiet-suppressed-types list, and the heartbeat end-marker list, and extracts the duplicated restart loop into restartBlockedAgents().

Critical

None found.

Warnings

• writeRestartMarker is best-effort (try { ... } catch { /* ignore */ }) with no logging on failure — if the write consistently fails (e.g. permissions issue on state/<agent>/), this regresses silently back to the original false-crash-alert bug with no trace in logs. A this.deps.log(...) in the catch block would preserve at least a signal without violating the "must never block the restart" requirement.
• The marker is written per-agent inside the for loop in restartBlockedAgents, and the same markerReason string is reused for every agent in that batch. If a mix of agents were blocked for different underlying reasons within the same rotation tick (unlikely given the current call sites, but not structurally prevented), the reason text would not distinguish them. Low risk given current usage.

Suggestions

• The comment noting the marker "fires twice per restart" and is not consumed by classifyFromMarkers is a good call-out — confirms this is deliberate and not a leftover-state bug, but might be worth an explicit test asserting the marker is NOT cleaned up until the heartbeat/TTL path fires, to guard against a future "helpful" cleanup that breaks the described flow.

Looks Good

• Correct root-cause fix: marker written synchronously before restartAgent() is called, matching the existing .user-restart pattern.
• Good extraction of the duplicated restart-loop code into restartBlockedAgents() — both doRotation() call sites now share one implementation.
• Comprehensive test coverage: marker presence/content at restart-call-time, classifier behavior, heartbeat end-marker clearing, and both rotation-recovery branches (candidate rotation + active-account-recovered-in-place).
• QUIET_SUPPRESSED_TYPES and distinct Telegram message text avoid conflating this with user-restart, which was flagged as misleading.

Verdict

Approve

[asachs01]

Review

Summary: Fixes a real false-positive-crash-alert bug: rotation-manager.ts's two restart loops (candidate-rotation-success, active-account-recovered-in-place) restarted previously limit-blocked agents without writing any marker hook-crash-alert.ts recognizes, so routine OAuth-rotation recoveries were misclassified as crash or rate-limited. The fix adds a .rotation-recovered marker written synchronously before restartAgent(), wires it into classifyFromMarkers, QUIET_SUPPRESSED_TYPES, and heartbeat.ts's END_TYPE_MARKERS.

Correctness

• Marker write happens before the restart call in both loops (verified by the diff), matching the doc comment's stated ordering requirement — important since the restart is what kills the old PTY / triggers the SessionEnd hook that reads the marker.
• writeRestartMarker is correctly best-effort (swallows errors) so a marker-write failure can't block a restart — reasonable given the fallback is only "worse classification," not a functional break.
• Extracting the duplicated loop into restartBlockedAgents() is a good simplification; both call sites now share one implementation, reducing future drift risk (this bug was literally two copies of the same logic diverging in fix coverage).
• Marker is written once per restart and not consumed/cleared during classification — confirmed as intentional via the new test comment ("fires twice per restart"), since heartbeat.ts's clearEndMarkers handles removal on the next heartbeat. Worth double-checking there's no race where a second rotation event writes a fresh marker for an already-recovering agent, overwriting reason text before the SessionEnd hook reads the first — but this predates the PR's scope (limitBlocked bookkeeping already deletes the agent from that map on success).

Tests

• New unit test for classifyFromMarkers handling .rotation-recovered.
• rotation-manager.test.ts updated to assert the marker exists at restartAgent call time (not just eventually) — good, catches ordering regressions directly rather than just end-state.
• Both restart-loop call sites get coverage (candidate rotation + active-account-recovered-in-place).

Quality/Docs

• Clear, well-commented code; CHANGELOG entry explains root cause and blast radius (10-13 false alerts/night) concretely.
• No security or performance concerns — this is process-internal daemon logic.

Verdict: Approve. Solid root-cause fix with good test coverage for both affected code paths and a reasonable refactor to prevent recurrence.

Reviewed SHA: f161e76

[asachs01]

Claude Code Review

Verdict: Approve

Critical

None.

Warnings

None.

Suggestions

• writeRestartMarker() writes the free-text reason string directly into the marker file with no size/format cap — low risk since it is only ever daemon-generated text, but consider trimming/truncating for defense-in-depth given the marker file also feeds a Telegram message.
• The shared restartBlockedAgents() helper is a nice dedup of two previously-copied loops; worth considering whether the other 3 pre-existing near-duplicate marker-write call sites noted in the PR description (already filed as a follow-up task) get consolidated soon so this doesn't become a 5th divergent copy.

Looks Good

• Correctly identifies and fixes the root cause: recovery restarts fell through to the crash default (or worse, false rate-limited classification) because no marker was written before restartAgent().
• Fix applied consistently to both restart loops in doRotation() (candidate rotation and active-account-recovery-in-place) — the PR explicitly calls out not leaving the second path unfixed, which is correct given both hit the exact same bug.
• Marker write happens synchronously before the restart call, matching the existing .user-restart pattern in src/cli/bus.ts — ordering is correct since restarting is what actually kills the PTY and triggers the hook.
• Write is best-effort (try/catch, never blocks the restart) — correct fail-safe behavior.
• QUIET_SUPPRESSED_TYPES and END_TYPE_MARKERS both updated so the new marker is suppressed from noisy alerting and cleared on the next heartbeat rather than relying solely on TTL.
• Good test coverage: marker-at-call-time assertions in both rotation-manager tests, a new classifyFromMarkers unit test, and updated clearEndMarkers marker list.

[asachs01]

Code Review Summary (Reviewed by Hermes Agent, updated for new commits — sha 4761d1f)

Fix verified: rotation-manager.ts's doRotation() restart loops (both the "rotate onto new candidate" and "active account recovered in place" branches) now write a .rotation-recovered marker via a new writeRestartMarker() helper, synchronously before restartAgent() is called — correctly mirroring the existing .user-restart write pattern in src/cli/bus.ts. hook-crash-alert.ts gained a matching marker classification, QUIET_SUPPRESSED_TYPES entry, and a distinct Telegram message rather than reusing the misleading "restarted by user" text. heartbeat.ts's END_TYPE_MARKERS also picked up the new marker so it clears on the next heartbeat instead of waiting on the 5-minute staleness TTL.

Critical: None.

Warnings: None. Marker write is wrapped in try/catch and explicitly documented as best-effort — a failed write only costs classification accuracy, never blocks the restart.

Code quality: Good — the two previously-duplicated restart loops in doRotation() are consolidated into restartBlockedAgents(), removing copy-paste drift risk.

Testing: hook-crash-alert.test.ts adds a direct regression test asserting the marker classifies as rotation-recovered (not crash) and preserves the reason string.

Suggestions: None blocking.

Looks good. Approving from a review-comment perspective (no formal PR review submitted per bot policy).

Reviewed by Hermes Agent

[asachs01]

Review update (Hermes sweep, delta since f161e76d): approved - extraction verified line by line

Delta on this PR's own branch since last review: 4761d1fb "refactor(daemon): extract shared restartBlockedAgents loop in rotation-manager". Verified the patch:

• The new restartBlockedAgents(state, markerReason) private method is a faithful extraction of the two previously-duplicated loops in doRotation() (rotation-onto-new-account branch and active-account-recovery branch): same marker-write -> restart -> delete state.limitBlocked[agent] -> catch-and-log-stays-blocked sequence, same log message text, marker reason passed as parameter preserving each branch's distinct message.
• Both call sites reload state via loadState() before calling (as before), and both still set lastRotationAt/retryAt/alertedHalt after. No ordering change.
• Tests updated in the same commit; CI green on head (Build/Type Check, Unit Tests, rotation-manager suite included).

Pure dedupe, no behavior change. See the same note I left on #188 about the shared base commits between these two branches - whichever merges second rebases.

[asachs01]

Hermes Agent Review

Verdict: Approve

Correctly fixes the rotation-recovery false-crash misclassification: both restart loops in doRotation() now write a .rotation-recovered marker before calling restartAgent(), mirroring the existing .user-restart convention, and hook-crash-alert.ts/heartbeat.ts are updated in lockstep (QUIET_SUPPRESSED_TYPES, END_TYPE_MARKERS) so the marker both suppresses the false alert and clears promptly. Good refactor pulling the duplicated restart loop into restartBlockedAgents() while fixing the bug in one place. Marker write is correctly best-effort (try/catch, never blocks the restart). Tests included for both the hook classification and rotation-manager.

---

Reviewed by Hermes Agent