Skip to content

fix(config): harden the Claude Code subscription backend - #2

Open
alexzerabr wants to merge 14 commits into
m-de-graaff:feat/claude-code-subscriptionfrom
alexzerabr:feat/claude-code-subscription-hardening
Open

fix(config): harden the Claude Code subscription backend#2
alexzerabr wants to merge 14 commits into
m-de-graaff:feat/claude-code-subscriptionfrom
alexzerabr:feat/claude-code-subscription-hardening

Conversation

@alexzerabr

Copy link
Copy Markdown

Hardening for usestrix#1114 (Claude Pro/Max subscription backend). Targets
m-de-graaff:feat/claude-code-subscription, not main. Refs usestrix#66.

Everything here came out of validating usestrix#1114 end to end against a real Claude
Max 20x subscription with Claude Code CLI 2.1.239. The design holds up: the ten
flags the transport uses all exist, claude auth {login,logout,status} all
exist, auth status --json matches the parser field for field, and
--json-schema returns exactly the {text, tool_calls} contract the bridge
expects. What follows are the defects that validation surfaced, in the areas the
PR description already flagged as unexercised.

1. A 529 overload killed the scan instead of retrying it

_error_status tags an "Overloaded" result event as 529, but
DEFAULT_MODEL_RETRY listed (429, 500, 502, 503, 504) and the statusless
fallback requires status_code is None. So a tagged 529 matched no policy:

case                          status   retried?
bridge 429 (rate limit)       429      RetryDecision(retry=True)
bridge 529 (overloaded)       529      RetryDecision(retry=False)   <- scan dies
bridge untagged (None)        None     RetryDecision(retry=True)    <- would have retried

Tagging the status is what disabled the retry. LiteLLM-backed routes never see
a bare 529 (its exception mapper folds 529 into a 500 InternalServerError), so
the tuple never needed it before; this backend bypasses LiteLLM and is the first
route that can produce one.

The existing unit test asserted status_code == 529 and passed the whole time.
The new test asserts the retry outcome, through the same normalizer the runner
uses for a custom Model.

2. Token counts were ~50x low, and the authoritative cost was discarded

Anthropic reports input_tokens EXCLUDING cache_read_input_tokens and
cache_creation_input_tokens. Every other Strix route normalizes through
LiteLLM, whose Anthropic transformation does
prompt_tokens += cache_creation + cache_read. The bridge left both counters in
the details only. On a real turn:

raw claude -p usage : input=3  cache_creation=7253  cache_read=0  output=140
bridge reported     : total_tokens = 143
LiteLLM would report: total_tokens = 7396          -> 51.7x undercount

That breaks the documented promise of "real token counts at $0" on a
subscription, and understates the estimate backing the budget guard on a metered
session by the same factor. Extended-thinking tokens were hardcoded to 0 while
the CLI reports them.

Separately, this backend never reaches LiteLLM, so litellm_cost_callback --
the hook every metered route relies on -- never fires for it. A claude-code/
run on an API key was classified api_key correctly (since b81ac5b) but still
accrued no observed cost, leaving the budget guard nothing to stop. The terminal
result event already carries total_cost_usd, priced by the CLI per model the
turn actually touched; it is now forwarded to the ledger, which drops it on a
subscription run and uses it in place of a local estimate on a metered one.

Live end-to-end after the fix, same subscription:

usage    : input=7409 output=128 total=7537  (cache_write=7406, reasoning=27)
cost hook: $0.047467
(pre-fix this same turn reported total=131)

3. The image-block detector deleted real text

The fix for the earlier review comment matched any block carrying an image_url
or source key, regardless of type. The marker replaces the block it fires
on, so a plain text block that happened to carry either key lost its text:

>>> _stringify([{"type": "output_text", "text": "see line 5", "source": "app.py"}])
'[image returned by tool, not visible to this backend]'

The rest of the codebase settles this by type -- llm/compaction.py and
interface/tui/live_view.py match {input_image, output_image, image, image_url}, core/sessions.py matches input_image. Now matching the union of
those, which covers every image block Strix actually produces (including
Anthropic-style {"type": "image", "source": {...}}) without claiming text.

4. An abandoned turn leaked the claude process past its semaphore slot

asyncio.to_thread cannot be cancelled -- the worker thread runs to completion
regardless. The turn held no handle on the child, so an abandoned turn (an outer
asyncio.wait_for, a budget stop, a wind-down) unwound and released its
semaphore slot at once while claude kept running for the rest of the turn
timeout: still spending subscription quota, still holding an executor thread,
and letting real concurrency exceed STRIX_CLAUDE_CODE_MAX_PROCS exactly when
turns are being abandoned. Preflight makes this reachable by default, wrapping
the turn in LLM_TIMEOUT (300s) against the transport's own 900s.

Now spawned via Popen so run_turn owns the handle and kills it on every exit
path. Killing also unblocks the stranded worker, since communicate() returns
as soon as the child dies. The new test drives a real cancellation: without the
kill it fails and hangs to its own deadline, which is the leak it guards.

5. Deduplication was broken on this backend, contrary to the docs

Only a real scan surfaces this one. report/dedupe.py picks its model with
(dedupe.model or "").strip() or settings.llm.model, so with STRIX_DEDUPE_MODEL
unset -- the default -- it runs on the main model. On a claude-code/ scan that
is this backend, which the docs explicitly say deduplication does not use.

It ran, and it failed every time. build_prompt gave every turn the agent
framing ("put your narration in text, list tools in tool_calls") and forced
--json-schema, so a one-shot completion came back narrating a "step" instead
of the JSON object dedupe asked for:

ERROR strix.report.dedupe: Error during vulnerability deduplication check
ValueError: No JSON object found in dedupe response: Comparing the candidate
against both existing reports: the candidate targets `/admin` via a hardcoded
secret token, while vuln-0001 is pickle deserialization RCE at `/session`...

A quick scan of a small Flask app produced six findings, four such tracebacks,
and 38k subscription tokens spent on calls that could never succeed. Duplicate
findings went unmerged.

Strix calls this backend two ways, and they need different contracts: agent turns
carry tools and must reply in the {text, tool_calls} envelope; one-shot
completions (dedupe, preflight) pass tools=[] and their caller parses the reply.
A tool-less turn now asks for the answer itself and leaves the schema off the
argv. _structured_payload also stops mistaking the caller's own JSON answer for
the envelope -- reading it that way found no "text" key and reported an empty
response, discarding the answer.

Re-running the same scan after the fix: 0 dedupe errors.

6. A malformed usage or cost field crashed the turn

The usage block and total_cost_usd are decoded from CLI stdout -- untrusted
input -- but were read with int(data.get(key) or 0). int("abc") raises
ValueError and int({...}) raises TypeError, straight through the run loop,
failing a turn that had already produced a good result. And json.loads accepts
the non-standard Infinity / NaN literals, so an infinite total_cost_usd
would reach the ledger and trip the budget guard permanently.

Every numeric read now goes through one _finite_number / _token_count pair
that degrades to zero (None for cost) and logs a present-but-unreadable field at
debug, so a wire-format change surfaces as something other than silence.

7. The version floor let through CLIs that cannot run a turn

MIN_CLAUDE_VERSION = (2, 0, 0), on the comment that the result schema "has been
stable since Claude Code 2.0". Checking the published npm bundles, 2.0 carries
almost none of what the transport drives:

2.0.0    no --json-schema, no --effort, no --no-session-persistence,
         no --disable-slash-commands, no api_error_status
2.0.45   --json-schema appears
2.0.60   --disable-slash-commands appears
2.0.77   --no-session-persistence appears
2.1.100  api_error_status still absent (last release shipping a readable cli.js;
         later ones ship a downloaded binary)
2.1.220  verified end to end on Windows, 2.1.239 on Linux

So preflight waved through CLIs that cannot run a single turn, trading an
actionable "update your CLI" for a cryptic runtime failure. api_error_status
matters most: it is what the retry policy classifies a 429/529 on.

The floor is now the lowest release verified to carry the whole contract. It is
conservative by construction -- the release that added api_error_status is not
visible in the published artifacts -- and erring high costs a user one update,
while erring low costs them a broken scan.

Also

run_turn had grown to three jobs -- build the argv, run a bounded subprocess
with its cleanup, translate the transcript -- so the subprocess lifecycle moved
to _execute and the translation to _decode_transcript, leaving run_turn as
the three-line pipeline it describes. No behaviour change: same semaphore scope,
same cleanup, same errors.

Removed three dangling .artifacts/DESIGN.md / .artifacts/SPIKE-DECISION.md
references (and a "future 4a path" comment) that reviewers cannot follow, and
moved the user-facing examples to claude-code/claude-opus-5, which
RECOMMENDED_MODEL_NAMES already lists ahead of claude-opus-4-8. Verified
accepted by the CLI on a live Max plan.

Risk

Only #1 leaves the claude-code/ blast radius: DEFAULT_MODEL_RETRY is shared
by every backend. Nothing else currently produces a bare 529 (LiteLLM maps it to
500 first), so in practice the change is confined to this backend, and retrying
an overload is correct for any route that ever does surface one.

Everything else is keyed on the claude-code/ prefix or lives inside the Claude
Code transport. The ChatGPT backend and the metered API-key path are untouched;
subscription.auth_mode() still short-circuits on codex.subscription_model()
before it ever probes the Claude session.

Not addressed

  • The result stream carries a rate_limit_event with structured
    utilization / resetsAt / status that parse_transcript discards (it
    keeps only the result event). Surfacing it would let Strix warn before a 429
    rather than react after one, but it changes parse_transcript's return shape,
    so it belongs in its own change.
  • A 429/529 under a real rate-limit burst. The retry classification is asserted
    through the runner's own normalizer, but neither test account throttled.

Verified on native Windows

Run under Windows Python 3.14.4 with Claude Code 2.1.220, driving the same argv
the transport builds, on a Max plan:

sys.platform                                   = win32
policy forced by main.py:447                   = _WindowsSelectorEventLoop
asyncio.create_subprocess_exec under it        raises NotImplementedError   <- the reason for to_thread
shutil.which("claude")                         = C:\...\claude.EXE
Popen(argv, shell=False)                       launches it
to_thread + Popen.communicate                  full turn, exit=0, 8.2s
  structured_output                            {"text": "Ready", "tool_calls": []}
  usage input/cache/output                     3 / 7011 / 75
  total_cost_usd                               0.043784
kill on cancellation                           child dies, worker thread released

Also checked with Claude Code installed via npm rather than the native installer,
which is the case that produces a .cmd shim:

shutil.which("claude")            = ...\node_modules\.bin\claude.CMD
Popen([shim], shell=False)        exit=0, "2.1.241 (Claude Code)"
full turn through the shim        exit=0, {"text": "Ready", "tool_calls": []}

Python launches a .cmd without a shell, so that variant needs no special
handling.

The NotImplementedError line is the one that matters: the PR justifies
asyncio.to_thread on the claim that a Windows SelectorEventLoop cannot spawn
subprocesses, and that is now measured rather than argued.

Feeding that same result event through the bridge gives input_tokens=7,014,
total_tokens=7,089, cost=$0.043784. Before the usage fix the same turn read
as 78 tokens: a 90.9x undercount, the cache-heavy pattern being if anything more
pronounced on a cold Windows session than on Linux.

Verified on a real scan

Two strix -n -m quick runs against a deliberately vulnerable Flask app, on a
live Claude Max plan, STRIX_CLAUDE_CODE_MAX_PROCS=4:

run.json  status=completed  auth_mode=subscription  cost=0.0
          total_tokens=2,189,566   (cache write 1,679,619 + read 471,977)
          6 sub-agents in parallel + dedupe, 0 dedupe errors

Before the usage fix that same scan would have reported 37,970 tokens -- a
57.7x undercount. Observed claude -p concurrency never exceeded the configured
bound.

One side effect worth knowing (pre-existing Strix behaviour, not this PR):
persist_current writes STRIX_LLM into ~/.strix/cli-config.json, so trying this
backend once silently makes it the default for every later run, and those runs
then report $0.

Verification

uv run pytest tests/test_claude_code_provider.py tests/test_claude_code_bridge.py \
  tests/test_claude_code_model.py tests/test_claude_code_auth_cli.py \
  tests/test_claude_code_preflight.py tests/test_model_retry.py
uv run pre-commit run --all-files

Note make check-all resolves whatever ruff uv sync picks (0.15.20 here),
while pre-commit.ci pins ruff v0.11.13; the two disagree on this tree. The
pinned one is green.

Environment: Linux (WSL2), Python 3.14.7, strix-agent 1.5.3, Claude Code 2.1.239,
Claude Max plan.

`claude_bridge._error_status` tags an "Overloaded" result event as 529, but
DEFAULT_MODEL_RETRY only listed (429, 500, 502, 503, 504). The statusless
fallback needs `status_code is None`, so a tagged 529 matched nothing and the
scan died on the first overload -- leaving the status untagged would have
retried it, which made the classification actively harmful.

LiteLLM-backed routes never surface a bare 529 (its exception mapper folds it
into a 500 InternalServerError), so the tuple never needed it. The Claude Code
backend bypasses LiteLLM and reports the status the CLI gives it, making it the
first route that can produce one.

The new test asserts the retry *outcome* through the same normalizer the runner
uses for a custom Model, not just the tagging: the previous unit test asserted
`status_code == 529` and passed while the scan still died.
Anthropic reports input_tokens EXCLUDING cache_read_input_tokens and
cache_creation_input_tokens. Every other Strix route normalizes through LiteLLM,
whose Anthropic transformation does `prompt_tokens += cache_creation + cache_read`,
so the bridge left both counters in the details and undercounted the turn.

On a real `claude -p` turn the gap is not marginal: 3 raw input tokens against
7253 cache-creation tokens, reported as 143 total where the metered route would
report 7396 -- a 51.7x undercount. That breaks the documented promise of "real
token counts at $0" on a subscription, and understates the cost estimate that
backs the budget guard on a metered session by the same factor.

Also stops discarding extended-thinking tokens, which Anthropic nests under
output_tokens_details and the bridge hardcoded to 0, and derives total_tokens
instead of reading it back: a CLI-supplied total would carry the same
cache-excluding semantics and reopen the undercount.

The existing fixture assertion (`total_tokens == 1245` over 53528 tokens of
cache) encoded the bug, so it moves with the fix.
The Claude Code backend never reaches LiteLLM, so `litellm_cost_callback` -- the
hook every metered route relies on to accrue observed cost -- never fires for it.
A `claude-code/` run signed in on an API key was therefore classified `api_key`
(correctly, since b81ac5b) but still accrued no observed cost, leaving the budget
guard with nothing to stop.

The terminal `result` event already carries `total_cost_usd`, priced by the CLI
per model the turn actually touched, which is strictly better than guessing one
rate from a `claude-code/<slug>` name LiteLLM has no first-party price entry for.
Forward it to the ledger, which drops it on a subscription run (`zero_cost`) and
uses it in place of a local estimate on a metered one.
`_is_image_block` treated any block carrying an `image_url` or `source` key as an
image, regardless of its type. The marker *replaces* the block it fires on, so a
plain text block that happened to carry either key had its text deleted and
replaced with "[image returned by tool, not visible to this backend]" -- losing
real tool output while telling the model an image existed.

The rest of Strix already settles this by type: llm/compaction.py and
interface/tui/live_view.py match {input_image, output_image, image, image_url},
core/sessions.py matches the input_image the SDK's sandbox tools emit. Match the
union of those, which covers every image block the codebase actually produces
(including the Anthropic-style {"type": "image", "source": {...}}) without
claiming text blocks.
`asyncio.to_thread` is not cancellable -- the worker thread runs to completion
whatever the awaiting coroutine does. The turn held no handle on the child, so an
abandoned turn (an outer `asyncio.wait_for`, a budget stop, a wind-down) unwound
and released its semaphore slot at once while the `claude` process kept running
for the remainder of the turn timeout: still spending subscription quota, still
holding an executor thread, and letting real concurrency exceed
STRIX_CLAUDE_CODE_MAX_PROCS exactly when turns are being abandoned. Preflight
makes this reachable by default, wrapping the turn in LLM_TIMEOUT (300s) against
the transport's own 900s.

Spawn via Popen so run_turn owns the handle, and kill it on every exit path.
Killing also unblocks the stranded worker, since communicate() returns as soon as
the child dies. Popen.communicate() additionally leaves the child alive on
timeout, unlike subprocess.run(), so the timeout path reaps it explicitly.

The new test drives a real cancellation: without the kill it fails and hangs to
its own deadline, which is the leak it guards.
Three source files pointed readers at `.artifacts/DESIGN.md` and
`.artifacts/SPIKE-DECISION.md`, and a comment referred to "a future 4a path".
None of those exist in the repo or in this PR, so a reviewer cannot follow them;
inline the substance instead.

The user-facing examples suggested `claude-code/claude-opus-4-8` while
RECOMMENDED_MODEL_NAMES already lists `anthropic/claude-opus-5` ahead of it.
Verified `claude-opus-5` is accepted by the CLI on a live Max plan.
The usage block and `total_cost_usd` are decoded from CLI stdout, so they are
untrusted input, but they were read with `int(data.get(key) or 0)` and a bare
isinstance check. Two ways that bites:

- A non-numeric field raises straight through the run loop --
  `int("abc")` -> ValueError, `int({...})` -> TypeError -- failing a turn that
  had already produced a perfectly good result.
- `json.loads` accepts the non-standard `Infinity` and `NaN` literals, so an
  infinite `total_cost_usd` would reach the ledger and trip the budget guard
  permanently.

Route every numeric read through one `_finite_number` / `_token_count` pair that
degrades to zero (or None, for cost) and logs a present-but-unreadable field at
debug, so a wire-format change surfaces as something other than silence. That
also removes the four-way duplication of the coercion expression.
run_turn had grown to three jobs -- build the argv, run a bounded subprocess with
its cleanup, and translate the transcript into a result event or an error -- with
the second job's rationale comment pushing it past 35 lines. Move the subprocess
lifecycle into `_execute` and the transcript translation into `_decode_transcript`,
leaving run_turn as the three-line pipeline it describes.

No behaviour change: same semaphore scope, same cleanup, same errors. `_kill` is
renamed `_kill_if_running` so the call sites read as what they do, and the two
cost tests share the helper they had been duplicating.
The pre-commit mypy hook type-checks tests/ (unlike `make type-check`, which
runs `mypy strix/` only), and a `function_call_output` literal whose `output` is
a list of content blocks does not match the SDK's FunctionCallOutput TypedDict,
which declares `output` as a plain string.

Route both such literals through one `_tool_output` helper that casts once and
says why the cast is honest: the runtime item really does carry a block list
when a tool returns mixed text and images. Takes the hook from 128 errors on the
base branch to 126 -- the new test stops adding two, and the pre-existing image
test stops reporting two.
… a step

Strix calls this backend two ways: agent turns, which carry tools and must reply
in the `{text, tool_calls}` envelope, and one-shot completions with `tools=[]`
whose callers parse the reply themselves. Both got the agent framing and the
forced `--json-schema`, so a completion came back as narration about a "step"
rather than the answer that was asked for.

That broke deduplication outright. `report/dedupe.py` falls back to the main model
when STRIX_DEDUPE_MODEL is unset (`(dedupe.model or "").strip() or settings.llm.model`),
so it does run on a claude-code/ scan -- contrary to the docs, which claim it does
not -- and every check died on `No JSON object found in dedupe response`. A real
quick scan produced six findings, four dedupe tracebacks, and 38k subscription
tokens spent on calls that could never succeed. Duplicate findings went unmerged.

Two changes make a tool-less turn a plain completion: the closing instruction
asks for the answer itself, and the schema is left off the argv. `_structured_payload`
also stops mistaking the caller's own JSON answer for the envelope -- dedupe asks
for a JSON object, and reading it as the envelope found no "text" key and reported
an empty response, discarding the answer.

Verified against the live CLI with the real DEDUPE_SYSTEM_PROMPT: the reply now
parses through `_parse_dedupe_response`.
`response.output[0].content[0].text` walks a union of thirty-odd Response item
types, so the pre-commit mypy hook emitted one error per member -- 28 per
assertion. Two typed helpers (`_assistant_text`, `_tool_calls`) narrow once with
an isinstance assert, and `_decode` now returns ModelResponse instead of object.

Takes the hook from 128 errors on the base branch to 67, with this file going
from 65 to 4.
The page claimed deduplication does not run on the Claude subscription. It was
already false when written -- report/dedupe.py falls back to the scan's own model
when STRIX_DEDUPE_MODEL is unset, so it ran and failed every time -- and after
the tool-less completion fix it is false in the other direction: it runs, and it
works. Say what actually happens and how to route it elsewhere.
Reproduced on a real Max account whose organization has Claude Code subscription
access turned off:

  Your organization has disabled Claude subscription access for Claude Code
  - Use an Anthropic API key instead, or ask your admin to enable access

It arrives with `is_error: true`, `subtype: "success"`, and no api_error_status,
so `_error_status` returned None and the statusless fallback retried it: five
attempts with 2s..90s backoff, per turn, per agent, for a policy change no second
attempt can clear. No error hint matched either, so the user never saw the CLI's
own actionable message.

Tag it 403 so the run stops, and add the hint pointing at the metered path. 429,
529, and genuinely transient statusless errors are unaffected.

This is the failure mode the PR's risk section describes -- Anthropic tightening
subscription access -- so it is worth handling as more than a generic error.
…the contract

MIN_CLAUDE_VERSION was (2, 0, 0), on the comment "the stream-json result schema
this backend relies on (structured_output, api_error_status) has been stable
since Claude Code 2.0". Checking the published npm bundles, 2.0 carries almost
none of it:

  2.0.0    no --json-schema, no --effort, no --no-session-persistence,
           no --disable-slash-commands, no api_error_status
  2.0.45   --json-schema appears
  2.0.60   --disable-slash-commands appears
  2.0.77   --no-session-persistence appears
  2.1.100  api_error_status still absent (last release shipping a readable
           cli.js; later ones ship a downloaded binary)
  2.1.220  verified end to end on Windows, 2.1.239 on Linux

So preflight waved through CLIs that cannot run a single turn, trading an
actionable "update your CLI" for a cryptic runtime failure. api_error_status
matters most: it is what the retry policy classifies a 429/529 on, so a CLI
without it degrades every rate limit into an unclassified error.

The floor is the lowest release actually verified to carry the whole contract.
It is conservative by construction -- the release that added api_error_status is
not visible in the published artifacts, since versions past 2.1.100 ship a binary
rather than a bundle -- and erring high costs a user one update, while erring low
costs them a broken scan.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant