Skip to content

feat(session): write a crash log when a run ends on a hard error#166

Open
PierrunoYT wants to merge 2 commits into
euxaristia:mainfrom
PierrunoYT:feat/crash-session-logs
Open

feat(session): write a crash log when a run ends on a hard error#166
PierrunoYT wants to merge 2 commits into
euxaristia:mainfrom
PierrunoYT:feat/crash-session-logs

Conversation

@PierrunoYT

Copy link
Copy Markdown
Contributor

Fixes #165.

What lands on disk

~/.config/cairn-code/crash-logs/
  crash-0199aabbccddeeff0001        ← transcript, resumable via /resume
  crash-0199aabbccddeeff0001.info   ← {"session_id","error","timestamp"[,"backtrace"]}

Verified end to end — the sidecar is correctly excluded from list(), and the error reads back:

listed = ["crash-0199aabbccddeeff0001"]
error  = Some("HTTP 500")

Shape

  • Transcript is an ordinary session file, so /resume restores it with no new machinery. Written through session::save, which brings the atomic rename, 0600 file mode, and 0700 directory mode — a transcript holds whatever the user typed and whatever the tools read.
  • Error in a sidecar, so the transcript stays loadable by the existing reader. . is not a legal session-id character, so list skips sidecars while returning the transcripts beside them.
  • AgentEvent::Crashed is distinct from ErrorError is a recoverable per-turn failure shown inline; Crashed says the run is over and the transcript should be preserved.
  • /crashes lists crash transcripts with the error beside each.

Details worth a reviewer's eye

Event ordering. Crashed is emitted after sync_live_mirror(), not alongside Error. 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 reading live_mirror directly, 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() 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, 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.

Session now derives Clone so 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 from list, 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 --check clean.

🤖 Generated with Claude Code

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>
@PierrunoYT
PierrunoYT requested a review from euxaristia as a code owner July 24, 2026 21:29
@coderabbitai

coderabbitai Bot commented Jul 24, 2026

Copy link
Copy Markdown

Warning

Review limit reached

@PierrunoYT, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 35 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

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 configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: 326779d1-b99d-4e38-832a-ac1540fd9ce2

📥 Commits

Reviewing files that changed from the base of the PR and between f87d710 and 6e1bc7b.

📒 Files selected for processing (5)
  • src/agent.rs
  • src/config.rs
  • src/main.rs
  • src/session.rs
  • src/tui.rs
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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.

❤️ Share

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

@euxaristia euxaristia left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 error and optional backtrace should 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>
@PierrunoYT

Copy link
Copy Markdown
Contributor Author

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 }
}

resume_session, resolve_id and the picker all go through it. Two things had to change for the advertised path to actually work:

  • /resume <id> was ignored entirely — any argument fell through to the picker, which is what made the id printed by /crashes useless. It now resolves (prefixes included) against the directory the id routes to.
  • The /resume picker lists crash transcripts after the saved sessions, so recovery does not depend on remembering an id. They stay out of /sessions, which was the point of the separate directory.

Something the routing exposed: resuming a crash log used to adopt the crash- id as current_session_id. With dir_for_id in place that would have been a data-loss bug — autosaves write to sessions_dir, but the id would route reads back to the crash directory, so everything typed after recovery would be invisible to the next /resume. Resuming now continues under the original session id (session_id_from_crash_id), which is also the more sensible semantic: a crash log restores the conversation, and work after that is an ordinary session.

[bug] copy. The message now names the id it needs: Crash log saved to {path} — /resume {crash-id} restores this conversation.

[suggestion] redaction. error and backtrace pass through redact_secrets on write, and read_crash_error redacts again on the way out — logs already on disk predate the write-side scrub, and that string goes straight to the screen.

[nit] discoverability. /crashes added to HELP_ROWS, with a test asserting a command users are told to run appears in /help.

Tests (527 lib tests, +11):

  • crash_log_round_trips_through_the_directory_its_id_routes_to — write_crash_log, then both halves of what /resume does (resolve, then load) for a crash id and a plain id in one temp tree, plus prefix resolution.
  • a_resumed_crash_id_continues_the_original_session — pins the id-carry-forward invariant above.
  • crash_sidecar_redacts_secrets_in_error_and_backtrace — asserts the token is absent from the bytes on disk, not just from the parsed value.
  • crash_error_read_redacts_text_written_before_redaction_existed — a hand-written pre-redaction sidecar.
  • TUI-side: dir_for_id routing, and /resume <unknown-id> reporting a miss instead of silently opening the picker.

One thing I did not change: /resume and /delete share a completion list built from sessions_dir, so crash ids do not autocomplete. Adding them there would have offered /delete ids it cannot delete. /crashes prints full ids for copying. Happy to extend both commands to crash logs if you would rather have that.

Verified: cargo fmt --check, clippy (no new warnings — the hits under session.rs/tui.rs are pre-existing), cargo doc with -Dwarnings, and cargo test --locked across lib and integration suites.

@PierrunoYT
PierrunoYT requested a review from euxaristia July 25, 2026 09:14
@PierrunoYT

Copy link
Copy Markdown
Contributor Author

Requested changes and bugs fixed

This PR implements the recovery design from #165:

  • hard-error runs produce a normal session-format transcript and error sidecar under crash-logs/;
  • AgentEvent::Crashed is emitted after synchronizing the live mirror;
  • TUI, --print, and exec failures preserve available transcript state;
  • writes reuse atomic session persistence and restrictive permissions; and
  • /crashes lists retained transcripts with their failure context.

The owner’s changes-requested review requested resumable transcripts, an accurate recovery message, secret redaction, and /crashes help. PierrunoYT addressed those in 6e1bc7b: crash IDs route to the crash directory, /resume <id> honors its argument, the picker includes crashes, errors/backtraces are redacted, help includes /crashes, and subsequent saves continue under the original session ID.

Review follow-ups

One end-to-end recovery defect remains actionable:

Tui::resume_session loads the selected crash transcript and rebuilds the visible TUI, but then sends __load_session__:<original-id>. The handler in src/main.rs loads that ID only from the ordinary sessions directory.

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

  • Open and technically mergeable, but blocked by the active CHANGES_REQUESTED review.
  • All current CI checks pass across formatting, policy, MSRV, Ubuntu, Windows, and macOS.
  • PierrunoYT re-requested review after the follow-up commit; no new human review has been submitted.

The explicit review bullets are substantially addressed, but /resume <crash-id> is not yet a reliable end-to-end restoration path.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Preserve the transcript and error when a run ends on a hard error

2 participants