fix(observability): preserve claude's stream on a nonzero exit (closes #326) #327

Merged

joshtronic merged 1 commit from fix/preserve-claude-stream-on-nonzero into master

2026-07-03 02:06:10 +00:00

igor commented

2026-07-03 01:21:09 +00:00

Collaborator

Closes #326.

When claude exits nonzero on its own (rc=${PIPESTATUS[0]} in claude_run_with_cost), the function returns cleanly — so the tick-death crashlog path (crashlog_preserve, which scans worktrees for a lingering in-flight marker) never fires. The worktree, with the raw stream, is torn down, and there's no post-mortem for why claude exited nonzero.

That's the exact blind spot behind vps-showdown#41 and #46: both bombed with claude exited 1, and we could only infer the cause — OOM for #41 (confirmed by the 4.7 GB peak); unknown for #46, which peaked at a normal 236 MB after the re-scope but still exited 1. The re-scope fixed the memory blowup; it exposed a second, invisible exit-1.

Fix

lib/crashlog.sh — new crashlog_preserve_scratch <rc> <state_dir> <call_site> <scratch>: copies the known .agent scratch's claude-stream.jsonl + claude-output.log to crash-logs/<stamp>-rc<rc>-<call_site>/, pruned to CRASHLOG_KEEP (20). Unlike crashlog_preserve, it needs no in-flight marker or worktree scan — it's handed the scratch directly.
lib/claude.sh — claude_run_with_cost calls it on rc != 0, before the in-flight marker is cleared and before the caller tears down the worktree. Guarded (declare -F + || true) so a missing helper or any error can never break the tick.
bin/test-crashlog.sh — 5 new assertions on the helper.

Every claude exited N now leaves a readable stream under ~/.local/state/agent/crash-logs/. Next time #46 (or anything) bombs, we read the cause instead of guessing.

Testing

bin/check-sync.sh green; test-crashlog.sh extended.

🤖 Generated with Claude Code

Closes #326. When claude exits nonzero on its own (`rc=${PIPESTATUS[0]}` in `claude_run_with_cost`), the function returns **cleanly** — so the tick-death crashlog path (`crashlog_preserve`, which scans worktrees for a lingering in-flight marker) never fires. The worktree, with the raw stream, is torn down, and there's no post-mortem for *why* claude exited nonzero. That's the exact blind spot behind **vps-showdown#41 and #46**: both bombed with `claude exited 1`, and we could only *infer* the cause — OOM for #41 (confirmed by the 4.7 GB peak); **unknown for #46**, which peaked at a normal **236 MB** after the re-scope but *still* exited 1. The re-scope fixed the memory blowup; it exposed a second, invisible exit-1. ## Fix - **`lib/crashlog.sh`** — new `crashlog_preserve_scratch <rc> <state_dir> <call_site> <scratch>`: copies the known `.agent` scratch's `claude-stream.jsonl` + `claude-output.log` to `crash-logs/<stamp>-rc<rc>-<call_site>/`, pruned to `CRASHLOG_KEEP` (20). Unlike `crashlog_preserve`, it needs no in-flight marker or worktree scan — it's handed the scratch directly. - **`lib/claude.sh`** — `claude_run_with_cost` calls it on `rc != 0`, **before** the in-flight marker is cleared and **before** the caller tears down the worktree. Guarded (`declare -F` + `|| true`) so a missing helper or any error can never break the tick. - **`bin/test-crashlog.sh`** — 5 new assertions on the helper. Every `claude exited N` now leaves a readable stream under `~/.local/state/agent/crash-logs/`. Next time #46 (or anything) bombs, we read the cause instead of guessing. ## Testing `bin/check-sync.sh` green; `test-crashlog.sh` extended. 🤖 Generated with [Claude Code](https://claude.com/claude-code)

igor added 1 commit

2026-07-03 01:21:09 +00:00

fix(observability): preserve claude's stream on a nonzero exit (igor#326)

Lint / check-sync (push) Successful in 4s

Details

Lint / check-sync (pull_request) Successful in 4s

Details

b5348e4b8a

When claude exits nonzero on its own, claude_run_with_cost returns CLEANLY, so
the tick-death crashlog path (crashlog_preserve, which scans worktrees for a
lingering in-flight marker) never fires -- the worktree, with the raw stream,
is torn down and there's no post-mortem for WHY claude exited nonzero. That's
the vps-showdown#41/#46 blind spot: both bombed with `claude exited 1` and we
could only INFER the cause.

- lib/crashlog.sh: crashlog_preserve_scratch <rc> <state_dir> <call_site>
  <scratch> -- copies the known .agent scratch's stream + display log to
  crash-logs/<stamp>-rc<rc>-<call_site>/, pruned to CRASHLOG_KEEP. No in-flight
  marker or worktree scan needed.
- lib/claude.sh: call it from claude_run_with_cost on rc != 0, before the
  in-flight marker is cleared and before the caller tears down the worktree.
  Guarded (declare -F + || true) so it can never break the tick.
- bin/test-crashlog.sh: cover the new helper.

Closes #326.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_011KXPafSYJPY6XkWHHk1Lfs

igor commented

2026-07-03 01:23:39 +00:00

Author

Collaborator

🤖 Review — `COMMENT` (automated)

CI for b5348e4b: success

Adds crashlog_preserve_scratch to capture claude's stream/output log on a nonzero exit (before worktree teardown), wired into claude_run_with_cost, plus 5 unit assertions. Small, in-scope, well-commented, CI green. My only reservation is that the actual integration point isn't verifiable from this diff.

Findings

Unverifiable integration variable (lib/claude.sh): The new call passes "$scratch", but the surrounding claude_run_with_cost body isn't in the diff, so I can't confirm $scratch is defined at that point and points at the .agent scratch containing claude-stream.jsonl / claude-output.log. If it's the wrong/undefined var, the whole feature silently no-ops (everything is || true-guarded, so nothing breaks — but it also wouldn't preserve anything, defeating the fix). This is exactly the behavior the PR exists to deliver, and the new tests only exercise the helper in isolation, never the call site. Please confirm $scratch is in scope and correct there (or point me at the surrounding lines).
Same-second collision (lib/crashlog.sh): stamp is second-precision (%Y%m%dT%H%M%S). Two nonzero exits in the same second with the same call_site/rc produce the same dest and the second cp overwrites the first. Low-likelihood in practice, but worth a nanosecond/PID suffix if double-preserve is plausible.

Positives

Checklist is honest: all 3 files (crashlog.sh, claude.sh, test-crashlog.sh) match the diff; the "5 new assertions" claim is accurate (stream, display log, call-site, rc-in-dirname, missing-dir no-op).
call_site is sanitized (tr -c 'A-Za-z0-9._-' '_') before use in the path — no path-injection concern.
Best-effort/guarded throughout (declare -F, 2>/dev/null, || true); can't break the tick.
No secrets, no CI-config changes, no unrelated churn.

Not blocking, but I'm withholding APPROVE because the one thing that actually matters — that $scratch resolves to the real stream dir in claude_run_with_cost — can't be confirmed from the diff and has no test around it. A quick confirmation (or an integration-level assertion) would move this to approve.

_{Independent review by the harness on claude-opus-4-8 (effort: high). The human reviewer is requested once Igor has reviewed; a human still merges.}

### 🤖 Review — `COMMENT` _(automated)_ CI for `b5348e4b`: **success** Adds `crashlog_preserve_scratch` to capture claude's stream/output log on a nonzero exit (before worktree teardown), wired into `claude_run_with_cost`, plus 5 unit assertions. Small, in-scope, well-commented, CI green. My only reservation is that the actual integration point isn't verifiable from this diff. **Findings** - **Unverifiable integration variable (`lib/claude.sh`):** The new call passes `"$scratch"`, but the surrounding `claude_run_with_cost` body isn't in the diff, so I can't confirm `$scratch` is defined at that point and points at the `.agent` scratch containing `claude-stream.jsonl` / `claude-output.log`. If it's the wrong/undefined var, the whole feature silently no-ops (everything is `|| true`-guarded, so nothing breaks — but it also wouldn't preserve anything, defeating the fix). This is exactly the behavior the PR exists to deliver, and the new tests only exercise the helper in isolation, never the call site. Please confirm `$scratch` is in scope and correct there (or point me at the surrounding lines). - **Same-second collision (`lib/crashlog.sh`):** `stamp` is second-precision (`%Y%m%dT%H%M%S`). Two nonzero exits in the same second with the same `call_site`/`rc` produce the same `dest` and the second `cp` overwrites the first. Low-likelihood in practice, but worth a nanosecond/PID suffix if double-preserve is plausible. **Positives** - Checklist is honest: all 3 files (`crashlog.sh`, `claude.sh`, `test-crashlog.sh`) match the diff; the "5 new assertions" claim is accurate (stream, display log, call-site, rc-in-dirname, missing-dir no-op). - `call_site` is sanitized (`tr -c 'A-Za-z0-9._-' '_'`) before use in the path — no path-injection concern. - Best-effort/guarded throughout (`declare -F`, `2>/dev/null`, `|| true`); can't break the tick. - No secrets, no CI-config changes, no unrelated churn. Not blocking, but I'm withholding APPROVE because the one thing that actually matters — that `$scratch` resolves to the real stream dir in `claude_run_with_cost` — can't be confirmed from the diff and has no test around it. A quick confirmation (or an integration-level assertion) would move this to approve. --- <sub>Independent review by the harness on `claude-opus-4-8` (effort: high). The human reviewer is requested once Igor has reviewed; a human still merges.</sub>

igor added spent time

2026-07-03 01:23:40 +00:00

37 seconds

igor requested review from joshtronic

2026-07-03 01:23:40 +00:00