fix: reliability fixes for handoff crash detection, output capture, and status detection

[chrisdfir]

Summary

A claude_code worker segfaulted mid-handoff during a read-only recon run. Debugging that one incident surfaced a chain of independent reliability bugs in the handoff/teardown/status path — each of which can turn a worker problem into a silent bad result (a crash reported as success, a report truncated, a 10s "completion" that did no work). This PR fixes all of them.

Each fix is its own commit with a self-contained message, so the PR can be reviewed commit-by-commit. All changes are unit-tested.

The fixes

1. status_monitor: drop stray output for deleted terminals instead of raising.
   When a worker dies, its DB row is deleted but the pane keeps emitting (shell prompt, the "Resume this session" banner, tmux redraws). _process_chunk called provider_manager.get_provider() unguarded, which raises ValueError for an unknown terminal, and run() logged a full traceback per stray chunk — flooding the log until the pane went quiet. Guarded like get_status() already is.

2. agent_step: detect a crashed worker instead of reporting false success.
   A segfaulted CLI freezes its pane. A frozen screen with a leftover response marker satisfies the COMPLETED heuristic trivially — death is permanent stability, so the confirm-stable window passes because the process is dead — and the handoff returns success with a truncated message. (A frozen screen matching no marker instead pins PROCESSING and the wait burns the full timeout, mislabeling a 5s crash as a 600s timeout.) Adds a fail-safe pane-liveness probe (worker_returned_to_shell): a live agent keeps its CLI as the pane foreground; only a crashed/exited one reverts to the shell baseline captured at launch. Probed at both decision points so a crash becomes kind="error" → 502 → handoff success=False. Generalizes the shell-baseline capture (previously kiro-only) to every provider. The probe returns False on any uncertainty, so healthy/indeterminate workers are never aborted.

3. claude_code: stop clipping reports at in-report markdown rules / tables.
   extract_last_message_from_script stopped at the first full-width ─ run, assuming it was the TUI turn-separator. But the renderer draws a markdown thematic break (---) and plain table dividers with the same pure-dash run, so any report containing one was clipped mid-message. Now a dash run terminates extraction only when it frames the ❯ input box (next non-blank line is the prompt, or the capture ends); a rule followed by more report text is content.

4. terminal_service: serve persisted scrollback when re-reading a torn-down terminal.
   handoff tears the worker down when it finishes (DB row deleted), so re-reading it via get_terminal_output returned 404 — the in-band handoff result was the only copy. delete_terminal already persists {id}.scrollback + {id}.snapshot.json; get_output now falls back to them (FULL → raw scrollback; LAST → re-extract with the recorded provider). A genuinely-unknown terminal still 404s.

5. providers: declarative DEFAULT_LAUNCH_ENV for per-provider launch env.
   Generalizes per-provider launch-environment injection instead of hand-rolling inline shell prefixes. Providers declare a class-level DEFAULT_LAUNCH_ENV; the service layer resolves it by provider type (before the instance exists, since the pane env is assembled at session/window creation) and merges it into extra_env for both supervisor and workers, under any operator --env. Unlike the process-local --env store, it is re-derived from the provider class on every spawn, so it survives a cao-server restart. First user: claude_code sets BUN_JSC_useDFGJIT=0 to disable the Bun DFG-JIT tier whose background compile thread can segfault mid-task.

6. claude_code: don't read the start-of-line effort footer as a response.
   The effort indicator renders as ● high · /effort (also medium/low/xhigh/…). EXTRACTION_RESPONSE_PATTERN's line-start anchor only excluded it when rendered inline (… esc to interrupt ● high · /effort), but it also renders on its own line, where the leading ● was read as a response marker — flipping get_status to a false COMPLETED on the idle launch screen (the ❯ box + this footer satisfy the completion heuristic), so a handoff "completed" in ~10s before the worker did any work and extraction returned high · /effort as the message. Adds a negative lookahead rejecting a start-of-line marker whose line ends in /effort.

7. claude_code: treat an in-progress tool-use marker as PROCESSING, not done.
   A response-marker line ending in the … ellipsis (⏺ Reading 1 file…, ● Running command…) is an active tool call, not a finished answer. When the worker emits a preamble and then thinks quietly for >10s, the live spinner scrolls out of the rolling buffer, the PROCESSING checks miss it, and get_status falls through to COMPLETED on the marker + idle prompt — false-completing a handoff in ~10s and returning the preamble instead of the report. Now the freshest response-marker line ending in … reads as PROCESSING. Gated on last_completion is None, so a genuinely finished turn (which shows the ✻ Worked for Ns summary) still completes; only the single-char … (U+2026) the TUI emits is matched, so prose ending in ASCII ... cannot false-trigger.

8. handoff: inherit the conductor's working directory.
   handoff() placed the worker in the cao-server process CWD instead of the supervisor's project directory. assign()/_create_terminal resolve the conductor's working directory when none is pinned, but _handoff_impl never did — so a supervisor launched in ~/dev/personal/myrepo handed off to a developer that ran in whatever directory the daemon was started from, operating on the wrong tree (recon only worked because it used absolute paths). _handoff_impl now resolves the conductor's working directory when the caller didn't pin one, mirroring _create_terminal.

Testing

• Full non-integration suite green: uv run pytest test/ --ignore=test/e2e -m "not integration" → 3226 passed.
• New regression tests for each fix (status_monitor deleted-terminal, crash detection + liveness probe, extraction of in-report rules, persisted-output fallback, DEFAULT_LAUNCH_ENV resolution + injection, effort-footer exclusion, in-progress tool-use detection, handoff working-directory inheritance).
• black, isort, mypy src/ clean (no new findings; pre-existing pyte typing notes in status_monitor unchanged).
• Verified live end-to-end against a real recon: pane-env delivery confirmed; a previously-crashing/false-completing handoff now runs a real ~40s handoff (worker actually executes npm run typecheck + test:unit) and returns an un-truncated report.

🤖 Generated with Claude Code

[Copilot]

Pull request overview

This PR hardens the worker lifecycle/handoff path so worker crashes, torn-down terminals, and Claude Code TUI quirks don’t produce silent false-successes, truncated outputs, or noisy status-monitor failures.

Changes:

• Add crash/liveness detection using a shell-baseline + foreground-process probe to avoid false COMPLETED/timeout outcomes.
• Improve output handling by re-serving persisted scrollback for deleted terminals and making Claude Code extraction resilient to in-report “rule/table” separators and effort footers.
• Introduce provider-scoped DEFAULT_LAUNCH_ENV defaults resolved pre-instantiation and injected at tmux pane creation.

Reviewed changes

Copilot reviewed 11 out of 11 changed files in this pull request and generated 2 comments.

Show a summary per file

File	Description
test/services/test_terminal_service_full.py	Adds regression tests for provider launch-env injection, persisted output fallback, and liveness probe behavior.
test/services/test_status_monitor.py	Adds tests ensuring stray output for deleted terminals is tolerated without exceptions and clears residual state.
test/services/test_agent_step.py	Adds tests asserting crashed-worker detection prevents false success/timeout reporting.
test/providers/test_provider_manager_unit.py	Adds tests for resolving DEFAULT_LAUNCH_ENV per provider type (including exhaustiveness).
test/providers/test_claude_code_unit.py	Adds tests preventing Claude Code report clipping on rules/tables and excluding the effort footer from extraction/completion.
src/cli_agent_orchestrator/services/terminal_service.py	Injects provider launch-env defaults into tmux pane env, captures shell baseline, adds persisted-output fallback, and introduces worker_returned_to_shell().
src/cli_agent_orchestrator/services/status_monitor.py	Makes _process_chunk tolerant of missing terminals by dropping stray output and clearing related state.
src/cli_agent_orchestrator/services/agent_step.py	Uses liveness probe to classify crashed workers as kind="error" instead of success/timeout.
src/cli_agent_orchestrator/providers/manager.py	Adds provider-type → class mapping plus helpers to resolve class-level defaults and stateless extractors.
src/cli_agent_orchestrator/providers/claude_code.py	Adds DEFAULT_LAUNCH_ENV, refines extraction response pattern to ignore effort footer, and improves separator handling to avoid truncation.
src/cli_agent_orchestrator/providers/base.py	Introduces DEFAULT_LAUNCH_ENV as a supported provider-level declarative default.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[codecov-commenter]

Codecov Report

❌ Patch coverage is 85.10638% with 14 lines in your changes missing coverage. Please review.
⚠️ Please upload report for BASE (main@0214f23). Learn more about missing BASE report.

Files with missing lines	Patch %	Lines
...li_agent_orchestrator/services/terminal_service.py	81.25%	9 Missing ⚠️
.../cli_agent_orchestrator/services/status_monitor.py	70.00%	3 Missing ⚠️
src/cli_agent_orchestrator/mcp_server/server.py	81.81%	2 Missing ⚠️

Additional details and impacted files

@@           Coverage Diff           @@
##             main     #352   +/-   ##
=======================================
  Coverage        ?   87.47%           
=======================================
  Files           ?      115           
  Lines           ?    13624           
  Branches        ?        0           
=======================================
  Hits            ?    11917           
  Misses          ?     1707           
  Partials        ?        0           

Flag	Coverage Δ
unittests	87.47% <85.10%> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[Copilot]

Pull request overview

Copilot reviewed 13 out of 13 changed files in this pull request and generated 1 comment.

[haofeif]

@chrisdfir thanks for your contribution!

I rechecked #352 carefully on the latest head (117d5f3) against current
origin/main (5dcf319).

I found one confirmed P2 in the new persisted-output path:

_persisted_output() rebuilds the provider extractor as:

extractor = provider_cls(terminal_id, "", "")

But the delete-time snapshot already stores agent_profile, and both
QCliProvider and KiroCliProvider require it positionally in __init__.
So for torn-down q_cli / kiro_cli terminals, get_output(..., LAST) now
throws during reconstruction, swallows that exception, and silently falls back
to raw scrollback instead of returning the provider-extracted final answer.

Relevant spots:

• src/cli_agent_orchestrator/services/terminal_service.py:687-694
• src/cli_agent_orchestrator/services/terminal_service.py:902-907
• src/cli_agent_orchestrator/providers/q_cli.py:30-36
• src/cli_agent_orchestrator/providers/kiro_cli.py:132-138

The new tests only cover the Claude path:

• test/services/test_terminal_service_full.py:1270-1283

So this provider gap is currently untested.

Suggested fix: parse the full snapshot in _persisted_output() and pass
agent_profile when rebuilding the extractor, or add a small provider-manager
helper for snapshot-based extractor creation. I’d also add persisted LAST
coverage for at least q_cli and kiro_cli.

Focused checks I ran locally:

uv run pytest test/services/test_agent_step.py test/services/test_status_monitor.py test/services/test_terminal_service_full.py test/providers/test_claude_code_unit.py test/providers/test_provider_manager_unit.py test/mcp_server/test_handoff_equivalence.py -q
uv run black --check src/ test/
uv run isort --check-only src/ test/
git diff --check origin/main...HEAD

Everything above passed, but I would still fix the persisted-output gap before
merge.

Findings

P2 - torn-down LAST output still breaks for q_cli and kiro_cli

_persisted_output() rebuilds a provider extractor as:

• src/cli_agent_orchestrator/services/terminal_service.py:687-694

It always does:

extractor = provider_cls(terminal_id, "", "")

That only works for providers whose constructor arguments after window_name are optional.

But the delete-time snapshot already persists agent_profile:

• src/cli_agent_orchestrator/services/terminal_service.py:902-907

And both of these providers require it positionally:

• src/cli_agent_orchestrator/providers/q_cli.py:30-36
• src/cli_agent_orchestrator/providers/kiro_cli.py:132-138

So for torn-down Q/Kiro terminals, the new LAST recovery path throws during provider reconstruction, swallows the exception, and silently falls back to raw scrollback instead of the provider-extracted final answer.

That means the new "recover output after worker teardown" behavior is only fully working for providers like claude_code; it is still broken for two major providers that the product already supports.

The current tests only cover the Claude path:

• test/services/test_terminal_service_full.py:1270-1283

There is no regression coverage for persisted LAST output on q_cli or kiro_cli, which is why this slips through.

Why I still classify this as P2, not P1

I rechecked the actual user-facing blast radius before settling on severity.

I still think this is P2 because:

• the main handoff success path still returns the worker result inline from run_agent_step; this bug does not break the primary completion path
• the torn-down reread path no longer 404s; it degrades to raw scrollback
• the regression is limited to providers whose extractor reconstruction needs snapshot metadata (q_cli / kiro_cli), not all providers
• this is a correctness/contract gap in a fallback read path, not data loss or a broken core execution flow

What would make it P1 for me:

• handoff/assign returning the wrong result on the primary success path
• rereads of torn-down workers failing entirely again (404 / 500)
• terminal teardown losing the only copy of the worker output
• the bug affecting the common live get_output(..., LAST) path rather than only the persisted fallback

So I would still ask for a fix before merge, but I do not think it rises to a P1 blocker.

Repro I confirmed locally

With a persisted snapshot like:

{"terminal_id":"dead","provider":"q_cli","agent_profile":"developer"}

and scrollback:

[developer] >
> final answer
[developer] >

the current code returns the full raw scrollback from get_output("dead", OutputMode.LAST).

But the actual provider extractor works when instantiated with the recorded profile:

QCliProvider("t", "", "", "developer").extract_last_message_from_script(raw)
# => "final answer"

Suggested fix

Parse the full snapshot in _persisted_output() and pass agent_profile when rebuilding the extractor, or add a small provider-manager helper that constructs a stateless extractor from snapshot metadata instead of guessing constructor shape in terminal_service.

I would also add direct regression tests for persisted LAST output on at least:

• q_cli
• kiro_cli

What I rechecked besides the finding

• crash-vs-timeout/error classification in run_agent_step
• handoff working-directory inheritance
• deleted-terminal chunk handling in StatusMonitor
• provider shell-baseline capture / restore
• Claude Code launch-env, status, and extraction changes

I did not confirm another blocker from those areas.

Checks run

In /private/tmp/cao-pr352:

uv run pytest test/services/test_agent_step.py test/services/test_status_monitor.py test/services/test_terminal_service_full.py test/providers/test_claude_code_unit.py test/providers/test_provider_manager_unit.py test/mcp_server/test_handoff_equivalence.py -q
# 246 passed

uv run black --check src/ test/
# passed

uv run isort --check-only src/ test/
# passed

git diff --check origin/main...HEAD
# passed

Review conclusion

I would hold merge on this until the persisted LAST extraction path uses the snapshot metadata it already saves and has coverage beyond claude_code.

[gutosantos82]

PR Review: #352 — fix: reliability fixes for handoff crash detection, output capture, and status detection

Summary

This PR bundles 8 independent reliability fixes in the handoff/teardown/status path (crash-vs-completion detection, torn-down output recovery, status-detection heuristics, per-provider launch env, conductor CWD inheritance), each with its own commit and regression tests (~573 test lines; PR reports 3226 passing, black/isort/mypy clean). The description matches the implementation faithfully and the structural claims (provider-type exhaustiveness, shell_baseline generalization, endpoint existence) all check out. Overall a high-quality, tightly-scoped bundle — but two findings in the exact core paths it hardens should be addressed before merge: a status-detection heuristic that can regress completed turns into a hang, and a security-relevant broadening of the persisted-scrollback read surface. Recommendation: Request changes, then this is a strong merge.

Blocking (must fix before merge)

• [correctness+tests] providers/claude_code.py:673-684 — The new "in-progress tool-use" ellipsis check can permanently mislabel a genuinely COMPLETED turn as PROCESSING, hanging the handoff to full timeout. It fires whenever the freshest response-marker line ends in U+2026 … and last_completion is None. That gate is not the claimed safety net: (a) the legacy TUI never emits a ✻ Worked for Ns summary — completion there is signalled by the ⏺ marker + idle prompt alone — so any legacy final answer whose marker line ends in … (e.g. ⏺ Here is the summary…) now latches PROCESSING forever; (b) even on the new TUI the summary can evict from the rolling buffer exactly as the PR's own rationale says the spinner does. It also inspects only the marker line (first line of the response), so a multi-line report whose opening line ends in … false-triggers. Two reviewers converge here (correctness flags it as the block; tests independently flagged the same uncovered false-negative). This is a behavioral regression in the core provider path this PR is meant to harden. Fix: require live-spinner/PROCESSING corroboration before treating a trailing … on an idle screen as active work (or key off the marker being the freshest line with no idle prompt yet), and add a false-negative test for a completed response ending in ….

Important (should fix)

• [security] services/terminal_service.py:734-743 (+ _persisted_output) — The new persisted-scrollback fallback broadens an info-leak surface. Reading a torn-down terminal via GET /terminals/{id}/output previously 404'd; it now serves the worker's full plaintext scrollback from disk, which routinely contains echoed env dumps, tokens, and AWS keys. The API has no per-caller authz, terminal IDs are 8 hex chars (enumerable), and the file persists until cleanup. Any actor reaching the localhost API can read another (already-deleted) terminal's history for an extended window. Fix: gate the fallback (e.g. only serve to the recorded caller_id when resolvable) and/or scrub secrets before persisting; at minimum document that scrollback outlives the terminal. (introduced)

• [security] services/terminal_service.py scrollback/snapshot writes (~:898, :915) — {id}.scrollback / {id}.snapshot.json are written under the process umask (typically world-readable 0644) in the same tree where the repo's own audit_log.py deliberately uses 0600 for sensitive files. This PR turns these from debug artifacts into API-served content, so their permissions now matter. Fix: create with 0600 (via os.open O_CREAT mode or chmod-after-write), matching the audit_log convention. (writes pre-existing; risk elevated by this PR's read path)

• [correctness] services/terminal_service.py:376-405 (probed at services/agent_step.py:206,234) — worker_returned_to_shell returns True whenever the pane's foreground command string equals shell_baseline, a false-positive whenever a live worker legitimately foregrounds a process matching the captured shell name (zsh/bash, zsh -c …, a tool call spawning the login shell). At the success probe (agent_step.py:234) this is the dangerous direction: a step that already reached COMPLETED would extract the real message, but a spurious True raises "exited before completing" and discards a genuine successful result. Mitigated (not eliminated) by the COMPLETED gate — at a true idle prompt the foreground is claude/node — so the window is narrow, but the comparison is bare-command-name with no PID/dead-pane check. Fix: compare pane PID or dead-pane state rather than just the command string. (introduced)

• [conventions] CHANGELOG.md — No CHANGELOG entry for a user-facing fix PR. [Unreleased] has only an "Added" block; this PR changes observable behavior (conductor CWD inheritance, crash detection, torn-down output recovery, status-detection fixes) and should add a ### Fixed entry. (introduced)

Nits (optional)

• [correctness] providers/claude_code.py:859-866 — The markdown-rule extraction fix terminates on a following ❯/> prompt or EOF; on the persisted-scrollback/LAST recovery path there may be no ❯ after the final rule, so trailing footer chrome can be appended to the extracted message. Degraded-fallback path only.
• [correctness] providers/claude_code.py:674-678 — response_marker = max(last_sol_response, last_response, ...); last_response uses the un-anchored ⏺-only pattern, so a literal ⏺ appearing mid-line in prose after the real start-of-line ● marker can win the max() and route the ellipsis check to the wrong line. Edge case.
• [security] services/terminal_service.py:669,687 — _persisted_output builds the path directly from terminal_id with no local guard. Not reachable today (HTTP boundary enforces ^[a-f0-9]{8}$), but a future non-HTTP caller passing "../.." would read an arbitrary file. Defense-in-depth: assert the 8-hex pattern inside the function.
• [security] mcp_server/server.py:689-701 — Handoff inherits working_directory from a localhost API response and launches a process there; mitigated by _resolve_and_validate_working_directory and mirrors the existing assign() path. Low, no new trust beyond the local API.
• [conventions] providers/manager.py:29,56 — Typing style mix: new annotations use PEP 585 type[BaseProvider] while the module uses Dict/Optional from typing. Valid/mypy-clean but inconsistent with the module's legacy-typing convention.
• [conventions] services/terminal_service.py:683 — Inline import json as _json (mirrors pre-existing inline use at ~:900); json is stdlib and the _json alias is redundant. Could be top-level.
• [conventions] services/terminal_service.py:~451 — Unnecessary adjacent string-literal split in a logger.debug message ("...(%s); " "returning raw scrollback"); renders correctly, purely cosmetic.
• [consistency] test/services/test_terminal_service_full.py:844 — Test docstring claims the BUN_JSC guard "used to be a hand-rolled inline command prefix in the provider," which could not be confirmed in this diff/branch (no such prefix present or removed). Mildly misleading test prose; author should confirm/soften.

Prior feedback (already raised — not restating)

• ↩︎ _persisted_output()'s provider_cls(terminal_id, "", "") reconstruction breaks the torn-down LAST recovery path for q_cli/kiro_cli (they require agent_profile positionally), silently degrading to raw scrollback; untested for those providers — already raised by @haofeif; we concur.

Tests

Coverage is adequate and above the merge bar: one regression class per fix, correct markers (@pytest.mark.asyncio, no integration/e2e needed), backend/tmux mocked throughout. Every fix has at least one test that fails without it; both agent_step crash-guard call sites are distinguished (timeout path and post-complete path, with get_output asserted not-called on the dead-pane path); default_launch_env has a provider-type exhaustiveness guard; the two new regexes have solid false-positive coverage.

Highest-value gaps (recommended, mostly non-blocking — the first overlaps the blocking finding):

• False-negative regex cases untested — a completed response ending in real U+2026 … (the blocking regression above), and a real response line legitimately ending in /effort (would be wrongly rejected by the lookahead). These are the highest-risk untested regressions.
• worker_returned_to_shell exact-match edge (empty current, whitespace like " zsh " vs "zsh" — code does raw ==, no strip) is uncovered.
• The shell_baseline capture at terminal_service.py:327-333 (now runs for every provider) has no direct assertion — the autospec'd mock silently returns a Mock.
• Over-mocking caveat: all three agent_step crash tests patch worker_returned_to_shell entirely rather than exercising the real probe; acceptable since the probe is unit-tested separately, but no end-to-end test proves the two connect.
• Uncovered error/fallback branches (nits): handoff non-200/exception/unset-CAO_TERMINAL_ID branches; _persisted_output missing/malformed snapshot, OSError read, and extraction-fail fallbacks; _process_chunk generic except Exception branch.

Verdict

Request changes — one behavioral regression in a core status path (ellipsis→false PROCESSING, corroborated by two reviewers) plus a security-relevant broadening of the persisted-scrollback read surface warrant a revision; the remaining items are important-but-mitigated or nits. Once the blocking heuristic and the scrollback exposure are addressed, this is a strong, well-tested merge.

Pre-existing (out of diff, FYI for CI): pyproject.toml:110 python_version = "2.1.1" under [tool.mypy] is a bogus value (should be "3.10"); not introduced by this PR.