feat(crashlog): preserve the claude stream when a tick dies mid-call (observe #279) #280

Merged

joshtronic merged 1 commit from feat/preserve-claude-stream-on-crash into master

2026-06-28 15:52:21 +00:00

igor commented

2026-06-28 15:29:21 +00:00

Collaborator

Observability for #279, not a fix — so the next occurrence tells us the actual cause instead of us guessing.

Why: #279 (and #272) die with status=1 and no "claude exited" line, right after the security-review Skill's [tool_result]. The model invocation doesn't return, and the worktree is then removed (issue work) or overwritten next tick (PR rework) — so claude's actual death reason (its merged stderr lives in the raw stream) is lost. I exhausted static analysis: the post-pipe bookkeeping is provably abort-proof (cost/health are || true'd, classify and record_failure return 0), so the death is in the claude/Skill process, which can't be diagnosed without seeing what it printed.

What this does:

claude_run_with_cost drops a .agent/claude-in-flight marker for the duration of every call and clears it on a clean return.
cleanup(), on an abnormal exit, copies any still-marked worktree's raw stream + display log + call-site into ~/.local/state/agent/crash-logs/<stamp>-rc<N>-<wt>/ before the worktree is gone.
Capped at the 20 newest dirs; best-effort throughout — a capture failure can never change the tick's exit.

New lib/crashlog.sh + bin/test-crashlog.sh (8 checks: preserve-with-marker, skip-without-marker, empty-arg no-op, rc in dir name, 20-cap, safe-on-missing-dir). bash -n + check-sync green.

So: next time a tick dies mid-Skill, we get claude's actual stderr/exit in crash-logs/ and can finally close out #279. Refs #279 (left open — this captures the cause, doesn't fix it). No reviewer.

**Observability for #279, not a fix** — so the next occurrence tells us the actual cause instead of us guessing. **Why:** #279 (and #272) die with `status=1` and **no "claude exited" line**, right after the security-review Skill's `[tool_result]`. The model invocation doesn't return, and the worktree is then removed (issue work) or overwritten next tick (PR rework) — so claude's actual death reason (its merged stderr lives in the raw stream) is **lost**. I exhausted static analysis: the post-pipe bookkeeping is provably abort-proof (cost/health are `|| true`'d, `classify` and `record_failure` return 0), so the death is in the **claude/Skill process**, which can't be diagnosed without seeing what it printed. **What this does:** - `claude_run_with_cost` drops a `.agent/claude-in-flight` marker for the duration of every call and clears it on a clean return. - `cleanup()`, on an abnormal exit, copies any **still-marked** worktree's raw stream + display log + call-site into `~/.local/state/agent/crash-logs/<stamp>-rc<N>-<wt>/` **before** the worktree is gone. - Capped at the 20 newest dirs; best-effort throughout — a capture failure can never change the tick's exit. New `lib/crashlog.sh` + `bin/test-crashlog.sh` (8 checks: preserve-with-marker, skip-without-marker, empty-arg no-op, rc in dir name, 20-cap, safe-on-missing-dir). `bash -n` + check-sync green. So: next time a tick dies mid-Skill, we get claude's actual stderr/exit in `crash-logs/` and can finally close out #279. **Refs #279** (left open — this captures the cause, doesn't fix it). No reviewer.

igor added 1 commit

2026-06-28 15:29:21 +00:00

feat(crashlog): preserve the claude stream when a tick dies mid-call (observe #279 )

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

Details

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

Details

c1b09c1bbe

#279 (and #272 before it) die with status=1 and NO 'claude exited' line, right
after the security-review Skill's tool_result: the model invocation doesn't return
and the worktree is then removed (issue work) or overwritten next tick (PR rework),
so claude's actual death reason -- its merged stderr lives in the raw stream -- is
lost. Static analysis is exhausted: the post-pipe bookkeeping is provably
abort-proof (cost/health are guarded; classify and record_failure return 0), so the
death is in the claude/Skill PROCESS, which can't be diagnosed without seeing what
it printed.

So capture it. claude_run_with_cost drops a .agent/claude-in-flight marker for the
duration of every call and clears it on a clean return. cleanup(), on an abnormal
exit, copies any still-marked worktree's raw stream + display log + call-site into
$AGENT_STATE_DIR/crash-logs/<stamp>-rc<N>-<wt>/ before the worktree is gone. Capped
at the 20 newest; best-effort throughout (a capture failure can never change the
tick's exit).

This is OBSERVABILITY, not a fix: the next #279 occurrence will leave claude's
actual stderr/exit behind so we can finally see the cause. Refs #279.

New lib/crashlog.sh + bin/test-crashlog.sh (8 checks). bash -n + check-sync green.

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

igor referenced this pull request

2026-06-28 15:29:41 +00:00

agent.service exits 1 at security-review Skill, abandoning PR-review rework mid-tick #279

igor commented

2026-06-28 15:31:57 +00:00

Author

Collaborator

🤖 Review — `COMMENT` (automated)

CI for c1b09c1b: success

Adds best-effort post-mortem capture (lib/crashlog.sh + tests, wired into cleanup()/claude_run_with_cost) that copies a worktree's raw claude stream when a tick dies mid-model-call. Scope-tight (~110 lines), CI green, tests included, no CI-config changes, no security concerns. The one thing I can't verify from the diff is the integration assumption that makes the feature actually fire — hence COMMENT, not APPROVE.

Checklist vs. diff — all honest:

Marker drop in claude_run_with_cost, clear on clean return, cleanup() capture, 20-dir cap, new lib + test file — all present.
"8 checks": I count exactly 8 eq assertions in test-crashlog.sh. Matches.

The unverifiable bit (please confirm before merge): The whole feature hinges on $scratch in lib/claude.sh resolving to $WORKTREE/.agent (or $PR_WORKTREE/.agent), because crashlog_preserve looks for the marker at $wt/.agent/claude-in-flight while the marker is dropped at $scratch/claude-in-flight. The diff doesn't show where $scratch is defined.

The stream filename is confirmed (local stream_log="$scratch/claude-stream.jsonl"), and the PR's own premise (stream lost when the worktree is removed) only holds if $scratch is inside the worktree — so this is very likely correct. But if $scratch lives outside the worktree, the marker is never found and the capture silently no-ops (no breakage, just no diagnosis of #279).
Same for the display log: crashlog_preserve copies $scratch/claude-output.log, but I can't see what $display_log is actually named in claude.sh. If it isn't claude-output.log, that copy silently fails (|| true) and you lose the display log — the stream still survives, so it's a partial miss, not a break. The test fabricates its own files so it doesn't catch a filename mismatch against the real code.

Correctness notes (no blockers):

[ -n "$wt" ] && [ -f ... ] || continue precedence is fine — both false-A and true-A/false-B correctly continue.
crashlog_prune keep-20 logic (ls -1dt | tail -n +21 | rm -rf) is sound; empty/missing dirs return 0; glob-no-match is harmless.
Stale-marker false positives are possible (if an early return path in claude_run_with_cost skips the rm -f, a later unrelated abnormal exit would preserve a stale stream and log "claude call did not return"). Harmless for observability, worth a mental note.
Everything is || true-guarded, so a capture failure genuinely cannot change the tick's exit, as claimed.

Ask: confirm $scratch == $WORKTREE/.agent and that the display log is claude-output.log in lib/claude.sh. If both hold, this is good to merge; if not, the feature won't capture what #279 needs.

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

### 🤖 Review — `COMMENT` _(automated)_ CI for `c1b09c1b`: **success** Adds best-effort post-mortem capture (`lib/crashlog.sh` + tests, wired into `cleanup()`/`claude_run_with_cost`) that copies a worktree's raw claude stream when a tick dies mid-model-call. Scope-tight (~110 lines), CI green, tests included, no CI-config changes, no security concerns. The one thing I can't verify from the diff is the integration assumption that makes the feature actually fire — hence COMMENT, not APPROVE. **Checklist vs. diff — all honest:** - Marker drop in `claude_run_with_cost`, clear on clean return, `cleanup()` capture, 20-dir cap, new lib + test file — all present. - "8 checks": I count exactly 8 `eq` assertions in `test-crashlog.sh`. Matches. **The unverifiable bit (please confirm before merge):** The whole feature hinges on `$scratch` in `lib/claude.sh` resolving to `$WORKTREE/.agent` (or `$PR_WORKTREE/.agent`), because `crashlog_preserve` looks for the marker at `$wt/.agent/claude-in-flight` while the marker is dropped at `$scratch/claude-in-flight`. The diff doesn't show where `$scratch` is defined. - The stream filename *is* confirmed (`local stream_log="$scratch/claude-stream.jsonl"`), and the PR's own premise (stream lost when the worktree is removed) only holds if `$scratch` is inside the worktree — so this is very likely correct. But if `$scratch` lives outside the worktree, the marker is never found and the capture silently no-ops (no breakage, just no diagnosis of #279). - Same for the display log: `crashlog_preserve` copies `$scratch/claude-output.log`, but I can't see what `$display_log` is actually named in `claude.sh`. If it isn't `claude-output.log`, that copy silently fails (`|| true`) and you lose the display log — the stream still survives, so it's a partial miss, not a break. The test fabricates its own files so it doesn't catch a filename mismatch against the real code. **Correctness notes (no blockers):** - `[ -n "$wt" ] && [ -f ... ] || continue` precedence is fine — both false-A and true-A/false-B correctly `continue`. - `crashlog_prune` keep-20 logic (`ls -1dt | tail -n +21 | rm -rf`) is sound; empty/missing dirs return 0; glob-no-match is harmless. - Stale-marker false positives are possible (if an early `return` path in `claude_run_with_cost` skips the `rm -f`, a later unrelated abnormal exit would preserve a stale stream and log "claude call did not return"). Harmless for observability, worth a mental note. - Everything is `|| true`-guarded, so a capture failure genuinely cannot change the tick's exit, as claimed. **Ask:** confirm `$scratch == $WORKTREE/.agent` and that the display log is `claude-output.log` in `lib/claude.sh`. If both hold, this is good to merge; if not, the feature won't capture what #279 needs. --- <sub>Independent review by the harness on `claude-opus-4-8`. The human reviewer is requested once Igor has reviewed; a human still merges.</sub>

igor added spent time

2026-06-28 15:31:57 +00:00

1 minute 49 seconds

igor requested review from joshtronic

2026-06-28 15:31:58 +00:00