Repository navigation
fix(claude-cli lane): drain to the ResultMessage on settle so token usage is recorded (#28) - #30
Conversation
…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-qa
left a comment
There was a problem hiding this comment.
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 forstream_events. All 1835 tests in the test suite pass.
|
|
{
"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
left a comment
There was a problem hiding this comment.
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(); breakwith interrupt-once → drain-to-ResultMessage→ break. No new auth/session path, listener, egress destination, dependency, subprocess, file write, or deserialization.client.interrupt()and thefinallyteardown 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:
ResultMessageis the SDK's last message of a turn, so thebreakfires; in the edge case where it never arrives,receive_response()exhausts on subprocess exit and thefinallyteardown 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.ymlruns four jobs (actionlint, semgrep, gitleaks, osv-scanner); thedetect 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-.envhygiene 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.ymlplus my read of the diff.
Non-blocking notes
- The
detect languages/repo hygiene/boundary jobs missing from this repo'ssecurity.ymlis a repo-level gap worth filing separately (this PR didn't touch the workflow).
|
@Alex-vonAllmen **Merged — the next milestone was not started.**
This PR's branch points at issue #28 is marked 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 To hand the developer role back to the dev team, remove |
What
On the
claude-cli/lane,llm_usagecame back all-zero (0 requests, 0 tokens, $0) on successful scans — the ones that end viafinish_scan/agent_finish. Runs that happened to end onmax_turnskept their usage; the normal success path lost it, leaving blank cost telemetry.Why
Usage (and
final_output) are recorded only in theResultMessagebranch of_translate. But the lane didinterrupt(); breakthe instant a lifecycle tool settled the agent — before the SDK yielded itsResultMessage. So on every settle-path exit the usage was never seen.This is exactly what Vercel's deepsec avoids: it reads usage from the
ResultMessageand never interrupts the query. The SDK does emit a finalResultMessage(terminal_reason=aborted_streaming) afterinterrupt(), carrying the cumulative usage — we just weren't draining to it.Change
stream_eventsnow interrupts once on settle, then keeps drainingreceive_response()until thatResultMessage, and breaks there. TheResultMessageis the single exit point, so usage +final_outputare 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 theResultMessage(does not stop before it), records its usage (total_tokens == 120), setsfinal_output, and does not consume the trailing message.Full lane suite green;
ruff+mypyclean.Closes #28. Builds on #27 (the #24 terminal-only fix, already merged).
🤖 Generated with Claude Code