Skip to content

fix(claude-cli lane): don't crash a settled agent on its own drain-interrupt (#32) - #33

Merged
jarvisallmen merged 1 commit into
mainfrom
fix/32-settle-interrupt-aborted-benign
Sep 14, 2026
Merged

jarvisallmen merged 1 commit into
mainfrom
fix/32-settle-interrupt-aborted-benign

Conversation

@Alex-vonAllmen

Copy link
Copy Markdown
Owner

What

Regression from #28, found on a deep verify run against stealth-mode@2ea29ee: every agent that cleanly finished was marked crashed.

17:03:19 [#24-diag] lane settled status=completed agent=56c4594a; interrupting, draining to ResultMessage
17:03:20 ERROR agent run failed for 56c4594a; marking crashed  ← ClaudeCliLaneError

Why

When a terminal lifecycle tool settles an agent, the #28 drain calls client.interrupt() to capture the final usage. The SDK then emits the drained ResultMessage with is_error=True / terminal_reason="aborted_streaming" — our own doing — but the fatal-error guard in _translate only whitelisted error_max_turns/max_turns, so it raised ClaudeCliLaneError. deepsec never hits this because it never interrupts.

It shipped because #28's drain test used an is_error=False stand-in — unrealistic, so the error-guard path was never exercised for the drained result.

Change

Verified

The same deep run confirmed #28 works — llm_usage recorded 123 requests / 21.8M tokens (was 0). This PR removes the crash so a scan can actually reach a clean finish.

Full lane suite green (79 passed); ruff + mypy clean.

Closes #32.

🤖 Generated with Claude Code

…terrupt (#32)

Regression from #28. When a terminal lifecycle tool settles an agent, the drain
calls client.interrupt() to capture the final usage. The SDK then emits the drained
ResultMessage with is_error=True and terminal_reason="aborted_streaming" — our own
doing — but the fatal-error guard in _translate only whitelisted error_max_turns /
max_turns, so it raised ClaudeCliLaneError and the runner marked every cleanly
finished agent "crashed". A whole deep scan looked like a pile of crashes even
though the agents did their work.

Fix: treat the aborted-streaming result produced by our settle-interrupt as benign
— bypass the raise when _settle_interrupted and terminal_reason=="aborted_streaming"
— and still record its usage. Genuine fatal errors, and aborted results we did not
trigger, still fail fast (#5 behaviour unchanged).

Why it shipped: #28's drain test used an is_error=False stand-in, which never
exercised the error-guard path. Made the stand-in realistic (is_error=True,
aborted_streaming) and added two scoped tests in test_claude_cli_model_error.py.

Closes #32.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@jarvisallmen

Copy link
Copy Markdown
Collaborator

oren-agent · qa · 2026-09-14T15:27:10.985Z · initial · corr=20301:33:qa

@jarvisallmen-qa jarvisallmen-qa left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

QA Verdict: APPROVE

Milestone: fix/32-settle-interrupt-aborted-benign PR: #33 (fix/32-settle-interrupt-aborted-benign)

CI

  • Lint & Format: pass
  • Tests & Coverage: pass (1842 passed / 3 skipped, overall coverage 73.49%, strix/llm/claude_cli/dispatch.py 85.15%)

Acceptance criteria

Criterion (from FRD / Issue #32) Implemented Test verifying it Status
AC-1: A settled agent that the lane interrupts to drain does not raise and completes cleanly yes (strix/llm/claude_cli/dispatch.py:463,507) tests.test_claude_cli_model_error::test_settle_interrupt_aborted_result_does_not_raise, tests.test_claude_cli_completion::test_stream_settles_then_drains_to_result_message met
AC-2: Genuine fatal CLI error still raises ClaudeCliLaneError yes (strix/llm/claude_cli/dispatch.py:504) tests.test_claude_cli_model_error::test_fatal_error_still_raises_after_narrowing, tests.test_claude_cli_model_error::test_errored_result_raises_lane_error met
AC-3: Aborted ResultMessage not triggered by settle-interrupt still raises yes (strix/llm/claude_cli/dispatch.py:463,507) tests.test_claude_cli_model_error::test_aborted_result_without_settle_interrupt_still_raises met
AC-4: Regression tests cover all three paths with realistic is_error=True drained results yes (tests/test_claude_cli_completion.py:78, tests/test_claude_cli_model_error.py:157-183) test_settle_interrupt_aborted_result_does_not_raise, test_aborted_result_without_settle_interrupt_still_raises, test_stream_settles_then_drains_to_result_message met

Non-blocking notes

  • Fix is surgical, minimal, and cleanly scoped to the settle-interrupt path via self._settle_interrupted flag and terminal_reason == "aborted_streaming" check.
  • Traceability and test coverage completely address the root cause and regression mechanism.

@jarvisallmen-qa jarvisallmen-qa added the qa-approved QA approved this PR label Sep 14, 2026
@jarvisallmen

Copy link
Copy Markdown
Collaborator

vera-agent · security · 2026-09-14T15:28:05.441Z · initial · corr=20303:33:security

@jarvisallmen jarvisallmen added the stage:security PR under security review (last gate) label Sep 14, 2026
@jarvisallmen

jarvisallmen commented Sep 14, 2026 •

Copy link
Copy Markdown
Collaborator
{
  "status": "done",
  "stage": "security",
  "agent": "vera-agent",
  "attempt": 1,
  "heartbeat_at": "2026-09-14T15:36:00Z",
  "correlation_id": "e3906b33-abe0-49ea-acac-4fbaf932b820",
  "result": "APPROVE submitted as native PR review on head 147708203a9cf2bb60a283352188daefdbaa0bb8; labels sec-approved + stage:security applied. All 4 security.yml jobs passed (semgrep 6 SARIF results all inSource-suppressed and pre-existing; osv 29 policy-filtered, none introduced). No undesigned surface: guard narrowing is authorised by strix-9 FRD-M2 and issue #32 AC-3. Note: no strix-32 vault folder exists — gated on issue #32 work order + live FRD-M2."
}

@jarvisallmen jarvisallmen left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Security Verdict: APPROVE

Milestone: #32 (settle-interrupt crash regression) PR: #33 (fix/32-settle-interrupt-aborted-benign) QA: approved by @jarvisallmen-qa at 2026-09-14T15:28:02Z (head 14770820 — unchanged since)

Static analysis (security.yml — 4-job mirror on this repo)

This repo's security.yml defines four jobs; the Tier-2 language-boundary jobs in the reference pack are not present here. Recorded honestly, not read as green.

Check Result Findings Disposition
actionlint (workflows) pass — —
semgrep (SAST) pass — 636 rules / 399 files, 0 blocking 6 results in SARIF, all inSource-suppressed 0 confirmed; 6 out of scope (below)
gitleaks (secrets) pass, full history — —
osv-scanner (dependencies) pass 29 filtered by the workflow's own policy → 0 counted out of scope (below)
detect languages / repo hygiene / client / python boundary not defined in this repo's workflow — n/a

Semgrep SARIF triage (all 6 pre-existing, none in this diff)

Finding Where Disposition
jinja2 direct use strix/agents/prompt.py:89 false positive — # nosemgrep annotation verified in source; file untouched by #33
tainted os.execve env strix/interface/update_check.py:295 false positive — annotated (noqa S606 / nosec B606 / nosemgrep); self-update path, untouched
non-literal import strix/llm/warmup.py:38 false positive — annotated; untouched
dynamic urllib strix/tools/proxy/caido_api.py:79 false positive — annotated (nosec B310 / nosemgrep); untouched
non-literal regexp (TS) viewer/.../TerminalRenderer.tsx:74 out of scope — inSource-suppressed, untouched
dangerouslySetInnerHTML (TSX) viewer/.../PocBlock.tsx:72 out of scope — inSource-suppressed, untouched; worth a tracked look next time the viewer is touched

OSV filter note (out of scope, pre-existing)

The workflow's filter policy excluded 29 advisories with stated reasons (dev-only frontend deps browserslist/nanoid/postcss, aiohttp 3.14.1 pin, cryptography <49 pin). This diff touches no manifest, so none of that state is introduced here — file a follow-up issue for the pinned advisory groups.

Undesigned surface

None. The diff (strix/llm/claude_cli/dispatch.py + two test files) adds one boolean and narrows the fatal-error guard. No new auth/session path, listener, egress, subprocess, file write, deserialization, or dependency. The benign classification of our own settle-interrupt result is explicitly authorised by the live FRD — strix-9 01-FRD-M2 §104: "interrupt() yields an aborted result → classified benign (M1 set + OQ-3 subtype); not fatal" — and issue #32's AC-3 scopes it: an aborted result without _settle_interrupted still raises, with a test proving it. Usage and final_output recording is preserved (AC-1).

Open questions a pentest would settle

None. The suppression predicate requires the lane's own flag, set only after the lifecycle settle, and only suppresses terminal_reason="aborted_streaming" on the drained result — there is no new reachable input path to attack.

Coverage limits

  • The dispatch pointed at a vault FRD in strix-32/, which does not exist. I gated on the issue #32 work order (ACs + scope) and the live strix-9/01-FRD-M2, which covers this exact area of the lane; no vault doc was contradicted.
  • No pentest has run against this head (normal — on-demand only, not requested).
  • The two TSX suppressions were verified via SARIF suppression flags only, not read line-by-line (files outside this diff).

Approved. This review is the security gate on the merge.

@jarvisallmen jarvisallmen added the sec-approved Security approved; this review merges the PR label Sep 14, 2026
@jarvisallmen
jarvisallmen merged commit afe0b4d into main Sep 14, 2026
6 checks passed
@jarvisallmen

Copy link
Copy Markdown
Collaborator
@Alex-vonAllmen **Merged — the next milestone was not started.**

This PR's branch points at issue #32 is not admitted to the dev-team pipeline (no dev-team label).

QA, security and the merge still ran on this PR and will on the next one — only the automatic hand-off to the developer agent is withheld.

If that was the final milestone, close issue #32 and add stage:done yourself. Closing the issue is part of the advance stage, so it is withheld along with the build — the issue would otherwise sit open at stage:building and eventually be flagged as stalled. The tidiest way to avoid this step next time is Closes #32 in the last milestone's PR body, which lets GitHub close the issue on merge with no agent involved.

To bring that issue into the pipeline, add the dev-team label to it. Re-applying issue-approved will not do it if that label is already present — GitHub emits no event for a label already there.

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

Labels

qa-approved QA approved this PR sec-approved Security approved; this review merges the PR stage:security PR under security review (last gate)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

claude-cli lane: settle-interrupt crashes every cleanly-finished agent (regression from #28)

3 participants