feat(session): write a crash log when a run ends on a hard error#166
feat(session): write a crash log when a run ends on a hard error#166PierrunoYT wants to merge 2 commits into
Conversation
A failed run printed its error and lost the conversation. The transcript is already checkpointed to sessions/, but nothing recorded what ended the run, so a crash left an ordinary-looking session and no explanation. Write both to a separate crash-logs/ directory: the transcript as a normal session file, and the error in a sidecar. The transcript stays an ordinary session so /resume restores it with no new code — losing the conversation is the part that costs the user. The error goes in a sidecar rather than the session JSON so the transcript stays loadable by the existing reader, and `.` is not a legal session-id character, so `list` skips sidecars while returning the transcripts beside them. Crash logs use the session's own id (crash-<id>) rather than a fresh one, so a crash is traceable to the conversation already being checkpointed. AgentEvent::Crashed is distinct from Error: Error reports a recoverable per-turn failure and is shown inline, Crashed says the run is over and the transcript should be preserved. It is emitted after sync_live_mirror so the handler sees this run's messages rather than the previous state. The TUI handler reuses session_snapshot rather than reading the live mirror directly, so a run that crashed before the mirror was populated still records what is on screen. Print and exec modes had no live mirror at all; both now attach one. A crash there is exactly when the transcript matters, since there is no scrollback to recover it from. Writing the log is best-effort in both: a failure to record the crash must not mask the error that caused it. crash_logs_dir resolves HOME then USERPROFILE, matching sessions_dir. Resolving only HOME would leave Windows on a relative path, dropping transcripts of full prompts into whatever directory the agent was launched from. Adds /crashes, which lists crash transcripts with the error beside each — a crash transcript is far less useful without knowing what ended it. Session now derives Clone so the crash transcript can be built with struct update syntax without hand-listing fields a future one could be dropped from. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Warning Review limit reached
Next review available in: 35 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (5)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
euxaristia
left a comment
There was a problem hiding this comment.
Summary
Crash-log write path is thoughtful (atomic save, 0600, sidecar, Crashed after mirror sync, print/exec mirrors). The advertised recovery path is broken: crash transcripts land in crash-logs/ while /resume and resume_session only load sessions_dir(), so /resume cannot restore them. Also apply redact_secrets to sidecar error text for defense in depth.
Verdict
REQUEST_CHANGES
Issues
-
[bug] src/tui.rs:3154 — resume_session / /resume only call session::load(&self.sessions_dir(), id). write_crash_log saves under config::crash_logs_dir() with id crash-<session_id>. After a crash the UI says "/resume restores this conversation" and list_crashes says the same, but /resume will not find those files. Fix: when id starts with "crash-" (or load from sessions fails), try crash_logs_dir(); teach resolve_id/list picker to include crash logs when resuming from /crashes; add a regression test that write_crash_log + load from crash dir (or the TUI resume path) round-trips.
-
[bug] src/tui.rs:2935 — User-facing copy claims resume works today ("Crash log saved to {path} — /resume restores this conversation"). Until resume can load crash-logs/, either implement the load path above or stop promising /resume and document the real recovery steps (path + how to open it).
-
[suggestion] src/session.rs:220 — Secret leakage: transcripts are full session JSON (same trust model as /save, 0600) which is acceptable if documented. Sidecar
errorand optionalbacktraceshould still pass through crate::redact::redact_secrets before write (and ideally again on read for display in list_crashes). LLM errors are usually pre-redacted via format_llm_err, but other Err strings and RUST_BACKTRACE frames can still carry token-shaped text or sensitive paths. Do not rely on hand-built JSON alone for scrubbing. -
[nit] src/tui.rs:2254 — /crashes is in SLASH_COMMANDS but not HELP_ROWS / the session-management help cluster. Add it next to /sessions for discoverability.
The crash log promised '/resume restores this conversation', but the transcript is written to crash_logs_dir while resume_session, /resume and resolve_id all read sessions_dir. Nothing the crash message or /crashes printed could actually be restored. Route on the id instead of assuming a directory: session::dir_for_id sends crash- ids to the crash directory and everything else to the session directory, and resume, resolve, and the picker go through it. /resume now accepts an explicit id — it previously ignored arguments and fell through to the picker, which is what made the id printed by /crashes useless — and the picker lists crash transcripts after the saved sessions. Resuming adopts the original session id rather than the crash- one, so autosaves after the resume land in sessions_dir where dir_for_id will find them again. Carrying the crash id forward would have routed those saves back to the crash directory and lost everything typed after recovery. The sidecar's error and backtrace now pass through redact_secrets on write and again on read: LLM errors arrive pre-redacted via format_llm_err, but other Err strings and backtrace frames can still carry token-shaped text, and escaping only makes the JSON well-formed, not safe to show. /crashes is listed in /help. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
All four addressed in 6e1bc7b. [bug] /resume could not reach crash transcripts. Routing now happens on the id rather than by assuming a directory: pub fn dir_for_id<'a>(sessions_dir: &'a str, crash_dir: &'a str, id: &str) -> &'a str {
if is_crash_id(id) { crash_dir } else { sessions_dir }
}
Something the routing exposed: resuming a crash log used to adopt the [bug] copy. The message now names the id it needs: [suggestion] redaction. [nit] discoverability. Tests (527 lib tests, +11):
One thing I did not change: Verified: |
Requested changes and bugs fixedThis PR implements the recovery design from #165:
The owner’s changes-requested review requested resumable transcripts, an accurate recovery message, secret redaction, and Review follow-upsOne end-to-end recovery defect remains actionable:
Consequently, a crash-only session can leave agent state unrestored, while an existing ordinary checkpoint can load an older transcript than the final crash copy. The UI can look restored even though the next model turn lacks recovered context. The selected crash transcript should be transferred directly to the agent, or the load command should preserve both the crash source and ordinary continuation ID. A regression test should resume a crash-only transcript and verify agent state before the next turn. Current status
The explicit review bullets are substantially addressed, but |
Fixes #165.
What lands on disk
Verified end to end — the sidecar is correctly excluded from
list(), and the error reads back:Shape
/resumerestores it with no new machinery. Written throughsession::save, which brings the atomic rename,0600file mode, and0700directory mode — a transcript holds whatever the user typed and whatever the tools read..is not a legal session-id character, solistskips sidecars while returning the transcripts beside them.AgentEvent::Crashedis distinct fromError—Erroris a recoverable per-turn failure shown inline;Crashedsays the run is over and the transcript should be preserved./crasheslists crash transcripts with the error beside each.Details worth a reviewer's eye
Event ordering.
Crashedis emitted aftersync_live_mirror(), not alongsideError. The handler reads the transcript from that mirror, so emitting earlier would capture a stale one missing the final turn.Snapshot source. The TUI handler uses the existing
session_snapshot()rather than readinglive_mirrordirectly, so a run that crashed before the mirror was populated still records what's on screen.Id reuse. Crash logs use the session's own id (
crash-<id>) rather than a fresh one, so a crash is traceable to the conversation already being checkpointed instead of appearing as an unrelated transcript.Windows path.
crash_logs_dir()resolvesHOMEthenUSERPROFILE, matchingsessions_dir(). Resolving onlyHOMEwould leave Windows on a relative path — dropping transcripts of full prompts into whatever directory the agent was launched from, quite possibly a git repo.Print and exec modes had no live mirror attached at all, so there was no transcript to preserve. Both now attach one. Writing the log is best-effort in both: a failure to record the crash must not mask the error that caused it, so problems go to stderr and nothing else changes.
Sessionnow derivesCloneso the crash transcript can be built with struct-update syntax rather than hand-listing fields a future one could be dropped from.Scope
An empty transcript writes nothing — a crash before any exchange leaves nothing worth preserving, and an empty file is more confusing than none. There is no retention policy; crash logs accumulate until deleted. Worth adding if you'd like, but it seemed better decided than assumed.
Verification
cargo test --lib— 520 pass (4 new: resumability round-trip, sidecar exclusion fromlist, escaping of quotes/newlines/control characters in the sidecar with optional backtrace, and missing-sidecar handling).cargo test— all integration suites pass.cargo clippy --all-targets— 91 warnings, identical to baseline.cargo fmt --checkclean.🤖 Generated with Claude Code