Skip to content

fix(claude-cli lane): drain to the ResultMessage on settle so token usage is recorded (#28) - #30

Merged
jarvisallmen merged 1 commit into
mainfrom
fix/28-claude-cli-usage-drain
Sep 14, 2026
Merged

jarvisallmen merged 1 commit into
mainfrom
fix/28-claude-cli-usage-drain

Conversation

@Alex-vonAllmen

Copy link
Copy Markdown
Owner

What

On the claude-cli/ lane, llm_usage came back all-zero (0 requests, 0 tokens, $0) on successful scans — the ones that end via finish_scan/agent_finish. Runs that happened to end on max_turns kept their usage; the normal success path lost it, leaving blank cost telemetry.

Why

Usage (and final_output) are recorded only in the ResultMessage branch of _translate. But the lane did interrupt(); break the instant a lifecycle tool settled the agent — before the SDK yielded its ResultMessage. So on every settle-path exit the usage was never seen.

This is exactly what Vercel's deepsec avoids: it reads usage from the ResultMessage and never interrupts the query. The SDK does emit a final ResultMessage (terminal_reason=aborted_streaming) after interrupt(), carrying the cumulative usage — we just weren't draining to it.

Change

stream_events now interrupts once on settle, then keeps draining receive_response() until that ResultMessage, and breaks there. The ResultMessage is the single exit point, so usage + final_output are recorded on the settle, max_turns, and natural-end paths alike.

Test

test_stream_settles_then_drains_to_result_message: an agent settled from the start interrupts once, drains to the ResultMessage (does not stop before it), records its usage (total_tokens == 120), sets final_output, and does not consume the trailing message.

Full lane suite green; ruff + mypy clean.

Closes #28. Builds on #27 (the #24 terminal-only fix, already merged).

🤖 Generated with Claude Code

…s recorded (#28)

llm_usage came back all-zero on successful runs because usage (and final_output)
are recorded only in the ResultMessage branch, but the lane did `interrupt();
break` the instant a lifecycle tool settled the agent — before the ResultMessage
was yielded. Runs that ended on max_turns kept their usage; runs that ended via
finish_scan/agent_finish (the normal success) lost it.

The SDK emits a final ResultMessage (terminal_reason=aborted_streaming) AFTER
interrupt(), carrying the cumulative usage — the same source deepsec reads.
stream_events now: interrupts once on settle, then keeps draining until that
ResultMessage (recorded in _translate) and breaks there. The ResultMessage is
now the sole exit, so usage + final_output are never dropped — on the settle,
max_turns, or natural-end paths alike.

Test: a settled run interrupts once, drains to the ResultMessage (not before),
records its usage (total_tokens 120), sets final_output, and does not consume the
trailing message.

Closes #28. Stacks on #27 (#24 fix) — merge that first.

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

Copy link
Copy Markdown
Collaborator

oren-agent · qa · 2026-09-14T13:25:53.107Z · initial · corr=19830:30: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: bugfix/28-claude-cli-usage-drain PR: #30

CI

  • Lint & Format: pass
  • Tests & Coverage: pass (1835 passed, 3 skipped, line coverage 73.48%)

Acceptance criteria

Criterion (from Brief) Implemented Test verifying it Status
C1 — Cycle ending via lifecycle tool records usage: on_llm_end called with token counts, llm_usage reflects them yes (strix/llm/claude_cli/dispatch.py:425-442) test_stream_settles_then_drains_to_result_message met
C2 — test_stream_interrupts_and_stops_when_settled reworked: interrupts once, drains receive_response() until ResultMessage, stops yes (strix/llm/claude_cli/dispatch.py:433-442) test_stream_settles_then_drains_to_result_message (tests/test_claude_cli_completion.py:94-142) met
C3 — No regression: runs ending via max_turns or naturally still record usage yes (strix/llm/claude_cli/dispatch.py:425-426) test_result_message_records_usage_and_final_output met

Non-blocking notes

  • The diff cleanly addresses the issue by interrupting once on settle and making isinstance(message, sdk.ResultMessage) the single exit point for stream_events. All 1835 tests in the test suite pass.

@jarvisallmen

Copy link
Copy Markdown
Collaborator

vera-agent · security · 2026-09-14T13:26:56.587Z · initial · corr=19837:30:security

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

jarvisallmen commented Sep 14, 2026 •

Copy link
Copy Markdown
Collaborator
{
  "schema": 1,
  "stage": "security",
  "agent": "vera-agent",
  "status": "done",
  "started_at": "2026-09-14T13:27:30Z",
  "heartbeat_at": "2026-09-14T13:31:50Z",
  "checkpoints": ["security.yml triaged (4-job variant, all green; 6 semgrep SARIF hits all out of scope)", "diff vs issue-28 brief read — no undesigned surface", "APPROVE review submitted on head 5b1a704; sec-approved applied"],
  "note": "verdict APPROVE — my approval triggers the merge"
}

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

@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: bugfix/28 — claude-cli usage drain PR: #30 (fix/28-claude-cli-usage-drain, head 5b1a704) QA: approved by @jarvisallmen-qa at 2026-09-14T13:26:53Z (head matches — verdict not stale)

Authorization for this change is the issue #28 brief (self-build bugfix; no strix-28 vault folder exists). Scope per the brief: the settle/interrupt path in strix/llm/claude_cli/dispatch.py and its tests. The diff touches exactly those two files.

Static analysis (security.yml)

Check Result Findings Disposition
actionlint (workflows) pass 0 —
semgrep (SAST) pass 6 in SARIF (all info-level, all pre-existing) 6 out of scope — see below
gitleaks (secrets) pass (full history) 0 —
osv-scanner (dependencies) pass 0 —
detect languages / repo hygiene / client boundary / python boundary not present — this repo's security.yml is the 4-job variant — n/a — see coverage limits

Semgrep triage — all 6 out of scope

None of the six SARIF results falls in this diff; five of six carry explicit inline # nosemgrep audit suppressions (developer-acknowledged):

Finding Where Why it does not apply
direct-use-of-jinja2 strix/agents/prompt.py:89 Pre-existing, untouched by this PR; inline # nosemgrep with explicit autoescape config decision
dangerous-os-exec-tainted-env-args strix/interface/update_check.py:295 Pre-existing, untouched; inline # nosemgrep/# nosec B606 (self-restart exec of own sys.argv)
detect-non-literal-regexp .../TerminalRenderer.tsx:74 Pre-existing, untouched; input passed through escapeRegex() before new RegExp()
react-dangerouslysetinnerhtml .../PocBlock.tsx:72 Pre-existing, untouched (renderer for pentest PoC HTML — by-design HTML)
non-literal-import strix/llm/warmup.py:38 Pre-existing, untouched; inline # nosemgrep
dynamic-urllib-use-detected strix/tools/proxy/caido_api.py:79 Pre-existing, untouched; inline # nosemgrep/# nosec B310 (configured proxy API base URL)

Follow-ups belong on separate issues, not on this gate.

Undesigned surface

  • None. The change replaces interrupt(); break with interrupt-once → drain-to-ResultMessage → break. No new auth/session path, listener, egress destination, dependency, subprocess, file write, or deserialization. client.interrupt() and the finally teardown pre-existed.
  • The drain reads only messages from the claude-CLI subprocess — the same trust boundary already consumed before this change. Usage recording is the point of the fix, not a new reporting surface (the brief explicitly permits it).
  • Drain termination is settled by reading: ResultMessage is the SDK's last message of a turn, so the break fires; in the edge case where it never arrives, receive_response() exhausts on subprocess exit and the finally teardown still kills the orphan. No unbounded loop introduced.

Open questions a pentest would settle

  • None. No attacker-controlled input reaches a new sink in this diff.

Strix

No pentest has run against this head — normal for the merge gate; not a blocker.

Coverage limits

  • This repo's security.yml runs four jobs (actionlint, semgrep, gitleaks, osv-scanner); the detect languages, repo hygiene, and Tier 2 boundary jobs of the reference workflow are absent here. All four that exist ran green on this head. The tracked-.env hygiene check therefore did not run in CI — gitleaks full-history is the compensating control.
  • The semgrep job exits 0 while the SARIF artifact still carries the six suppressed/pre-existing results above — triaged individually rather than accepted as green.
  • No pentest; verdict rests on security.yml plus my read of the diff.

Non-blocking notes

  • The detect languages/repo hygiene/boundary jobs missing from this repo's security.yml is a repo-level gap worth filing separately (this PR didn't touch the workflow).

@jarvisallmen jarvisallmen added the sec-approved Security approved; this review merges the PR label Sep 14, 2026
@jarvisallmen
jarvisallmen merged commit 81de6ac 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 #28 is marked self-build, so the developer role is owned by a human (or Claude Code) rather than the developer agent.

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 #28 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 #28 in the last milestone's PR body, which lets GitHub close the issue on merge with no agent involved.

To hand the developer role back to the dev team, remove self-build from issue #28.

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: token usage not recorded (llm_usage=0) — the settle interrupt skips the ResultMessage that carries usage

3 participants