fix(advisor): make the reviewer usable on a Claude subscription, and give it reasoning effort - #793
Merged
Merged
Conversation
…give it effort
The client-side advisor had four defects that compounded: it was
outright broken for premium Anthropic models on a subscription login,
and where it did run it ran weakly.
1. `system` went as a plain STRING. On the subscription (OAuth) path
`_prepare_subscription_request` prepends the required "You are Claude
Code…" preamble; with a string it CONCATENATES into one blob, with a
list it INSERTS the preamble as its own block. The endpoint accepts
only the latter for premium models. Wire-probed against claude-opus-5
over subscription, 3/3 per cell:
system=None (bare preamble string) -> 200
system=<string> (preamble + advisor text) -> 429
system=[<block>] (preamble block + text) -> 200
The rejection arrives MISLABELLED as
{"type": "rate_limit_error", "message": "Error"}, so it reads as
capacity and invites a backoff hunt rather than a shape fix. Haiku
accepts the string form, which is why a cheap smoke test misses it.
2. No thinking config and no reasoning effort on either wire — a model
chosen precisely because it reasons harder ran with thinking off at
the API default. Extracted `build_anthropic_thinking_kwargs` from
`_call_model_sync` so both callers share ONE copy of the model gates
(adaptive-vs-budget, the effort allowlist, the xhigh clamp) and they
cannot drift; OpenAI-compat wires get `extra_body.reasoning_effort`
with clamp_xhigh=False, since that allowlist holds Anthropic model
names and matched nothing here. New `advisor_effort` setting and
`/advisor <provider>:<model> --effort <level>`; unset inherits the
session effort, then omits the parameter entirely.
3. `max_tokens` was a flat 4096. Thinking is drawn from the SAME budget,
so a high-effort reviewer could spend it all reasoning and return
stop_reason=max_tokens with no text — surfacing as the useless
"Advisor returned no text content". Now per-model, floored at 4096.
4. No retry: one transient 429/5xx ended the consultation. Bounded to 3
attempts, abort-aware, honouring Retry-After via the main loop's own
classifier so both lanes agree on "transient".
Also removes a dead `call_kwargs.get("model")` lookup (call_kwargs never
carries one) and a function-local `import logging` that shadowed the
module import for the whole scope.
Harbor adapter: new `advisor` / `advisor_effort` agent kwargs, and
`subscription=true` now covers an anthropic ADVISOR rather than only an
anthropic main model — the pairing that motivated this (an API-key
worker consulting a subscription reviewer) was previously inexpressible.
The worker's own provider key is still forwarded in that configuration;
only ANTHROPIC_API_KEY stays withheld so OAuth remains the sole route.
Verified live: gpt-5.6-luna worker (API, effort=xhigh) consulting
claude-opus-5 (subscription, effort=xhigh) end to end on the headless
path — advisor called twice, real advice both times, worker acted on it.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The adapter forwarded only the MAIN model provider's env vars, so a run whose reviewer sits at a different vendor got an advisor with no credentials. Caught on the first container smoke: the advisor fired twice and both calls died on "Missing credentials". That failure mode is quiet by design — a failed consultation leaves the worker to carry on, and it still solved the task, so the job reported reward 1.0 with an advisor that never answered once. Anything reading the score alone would have concluded the advisor worked. Union the advisor provider's keys into the forwarded set, deduped and order-preserving. ANTHROPIC_API_KEY stays excluded under subscription in BOTH roles, so OAuth remains the only route to the subscription. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
… too execute_client_advisor read providers.<name>.api_key straight out of config.json, making it the one call site in the codebase that ignored the environment. An advisor pointed at a provider whose key lives in an env var — how eval containers and most shells supply credentials — was constructed with api_key="" and died on "Missing credentials", while the exact same provider worked fine as the main loop. Use resolve_api_key(), the shared resolver: configured value first, then the provider's known env vars via the secret store. Empty stays a legitimate, non-fatal outcome — the Anthropic subscription path REQUIRES an empty key so the provider falls through to OAuth (a key would silently outrank it and bill the API). Pinned by a test. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
RUN_ADVISOR_TB21.md covers the luna-worker/opus-5-reviewer pairing: the two prerequisites (a build with these fixes, and a real `clawcodex login` — an imported keychain token cannot carry a long run past the refresh threshold), smoke and full-run commands, the no-advisor control run needed to read the delta, and the shared-subset comparison rule. It also spells out how to VERIFY the advisor answered. A failed consultation degrades quietly, so a job can report reward 1.0 with a reviewer that never once replied — the reward alone cannot tell you. `_advisor_sleep` read the clock twice (loop condition, then remainder), so the remainder could go negative in between and `time.sleep` raises ValueError on a negative argument. Clamped at zero. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…over the gaps Five findings from an adversarial review of the branch. MAJOR — the per-model max_tokens from fix #3 reached the OpenAI-compatible wire, where the main loop deliberately sends none and the table value is an auto-compact reservation rather than a legal request cap (deepseek 384000, luna 128000; openai_compatible does forward max_tokens). Clamped to 32768 there; the Anthropic branch still takes the table value whole, because on that wire it IS the request's max_tokens by design. Honest scope: I probed DeepSeek at 384000 and it returns 200, so this was never a live outage — it is an untested number per provider across ~30 of them, where a rejection is a non-retryable 400 that dies on attempt 1 and degrades silently. Real, but MINOR in practice, not MAJOR as first called. MAJOR — ~280 lines of new command and adapter logic had no tests. Added 12 for the /advisor --effort parser (both flag forms, missing and invalid values, retune-without-model, auto-clears, unset-clears, status render) and 19 for the adapter (advisor key forwarding, ANTHROPIC_API_KEY withheld under subscription in both roles, settings seeding, the relaxed subscription gate, kwarg validation). The adapter file is importorskip'd — it runs for people with harbor installed and is invisible to CI, which is worth knowing. MINOR — changing the reviewer model no longer carries a stale advisor_effort across. That is not merely untidy: the xhigh clamp keys on Anthropic model NAMES, so an xhigh set for an Opus reviewer went out UNCLAMPED to a newly-selected OpenAI-compatible one. Cleared, and the confirmation says so. NIT — resolve_max_output_tokens now receives base_url, so per-endpoint overrides apply as they do in the main loop. NIT — a consultation that exhausts its retries logs at INFO. It used to be invisible: the worker carries on and the task can still score, so a run that quietly lost its advisor looked identical to a healthy one. Also: /advisor unset --effort <level> now errors instead of silently discarding the flag. Verified correct by the same review: the block-list system change is safe on Minimax (already receives block lists from the main loop) and on the plain API-key path; the build_anthropic_thinking_kwargs extraction was proven behavior-preserving differentially across 2688 combinations with 0 divergences; and the test suite survived 8 mutants. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
All found by mutation testing, all previously invisible. 1. `test_anthropic_takes_the_model_table_value_whole` was VACUOUS. 32768 is itself the largest max_output_tokens in the Anthropic family, so no real model could distinguish clamped from unclamped and the wire asymmetry the whole fix rests on was unpinned — applying the clamp to BOTH wires passed green. Now patches the ceiling down to 8192 so the assertion has to mean something. Mutant (clamp both wires) → red. 2. The `base_url` passthrough was untested; deleting the kwarg was invisible. Mutant → red. 3. The ANTHROPIC_API_KEY strip on the MAIN-model branch was untested, and it guards the expensive failure: for a mapped provider it is a no-op, but an unmapped one falls back to the all-providers set, and without the strip the key rides into the container where it silently outranks OAuth and bills the API instead of the subscription. Mutant → red. Writing (3) turned up a real misreading on my part, now pinned separately: `_ALL_PROVIDER_ENV_VARS` is the union of the seven MAPPED vendors, so an unmapped provider's OWN key is never forwarded at all. That is pre-existing and unrelated to the advisor, but it looks exactly like advisor breakage from a container log, so it gets its own test saying so. Also documents why the ceiling is 32768 (a rule — the Anthropic family maximum — not a taste) and that it also bounds a CLAUDE_CODE_MAX_OUTPUT_TOKENS override on this wire while the Anthropic wire honours one whole. Moves the `__main__` block below the new classes in test_advisor_command.py: it was stranded mid-file, so a direct `python tests/...` run executed 15 of 28 tests and silently skipped the rest — the exact class of quiet no-op this branch keeps finding. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…he second credential door A second review pass caught three issues the first missed, one of them a regression against the very commit this branch is built on. M1 — the advisor's OpenAI-compat branch skipped `normalize_reasoning_effort`, silently reintroducing what #791 (ac741d6) had just fixed for the main loop. A DeepSeek advisor received `xhigh` — a level DeepSeek does not know — so it dropped the field and applied its own default. No error; just a level the user did not ask for, biased DOWNWARD on the setting people reach for when a task is hard. My comment claiming it "mirrors query.py" was false. Extracted `normalize_effort_for_provider` so the two call sites share one implementation, same reasoning as build_anthropic_thinking_kwargs: this hook has now been forgotten once, and a copy would let it happen again. The validate-don't-trust guard moves with it (a duck-typed getattr on a MagicMock answers with a callable, which would otherwise write a repr into the body). M3 — `_host_env_keys()` defeated the subscription ANTHROPIC_API_KEY exclusion. We withheld the key from the container's process env in both roles, then forwarded the host's whole global-config `env` block into the container's config.json — and `get_secret` reads process env THEN that block, so a stored key there is found by `resolve_api_key`, takes the API-key path, and OAuth never engages: silently billing the API on a run that asked for the subscription. Not currently triggered (this host's block holds only TAVILY_API_KEY), but the previous commit message asserted "OAuth remains the only route" as though it held. Both doors are now shut. M2 — the /advisor status line credited "(inherited from /effort)", asserting a link that exists only on the registry path and under an eval adapter: the TUI's /effort writes a session-only field and headless --effort is per-turn, so on the surface most users are looking at, that inheritance is dead. Names the setting instead of the command. Minors: `advisor=` now validates BOTH halves (a bare colon test let "anthropic:" and ":claude-opus-5" seed a silently inert advisor, where `fusion=` validates both); `_VALID_ADVISOR_EFFORTS` derives from VALID_EFFORT_VALUES rather than being a third hand-maintained ladder; the retry classifier's comment no longer claims a 529 arm that is in fact unreachable; /advisor's TUI menu hint documents --effort. Both new fixes mutation-tested (skip the normalize → red; drop the env-block strip → red). Full suite 9719 passed, 10 skipped, exit 0. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…ximum _ADVISOR_MAX_OPENAI_WIRE_TOKENS is documented as a RULE — "the OpenAI wire never gets a larger budget than the most generous Anthropic model" — so pin it rather than leaving the claim to a comment, the way VALID_THINKING_EFFORT_ LEVELS pins its ladder. Load-bearing because the anchor is a SINGLE LEGACY ROW: claude-opus-4- 20250514 is 32768 while opus-5, opus-4-8 and fable-5 are all 32000. Pruning old model rows would drop the family maximum to 32000 and silently turn the constant's stated rationale into a false statement, with nothing noticing. Verified against the table rather than taken on trust, and mutation-tested (32_768 → 32_000 turns it red). Full suite 9720 passed, 10 skipped, exit 0. One earlier run showed test_sigterm_triggers_drain failing; it passes 3/3 in isolation, 13/13 on main, and green on a clean re-run — a load-dependent timing flake in the same family as the known test_sigint_during_prefetch one, and nothing here touches signal handling. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…etic
The "Harbor adapter (3.13)" job installs harbor but ran only
test_headless_usage_events.py, so both adapter test files were invisible to
CI by OMISSION, not by necessity. They open with importorskip("harbor"), so
under the main test (3.11) job they skip silently — this job is the only
place they can run, and a file left out of its list never runs anywhere.
Added both, with a note to add future tests/test_harbor_* files too. The
"SKIPPED IN CI" docstrings were wrong and are corrected.
That change immediately earned its keep: running the real job command turned
up test_subscription_accepted_for_an_anthropic_advisor asserting a
RuntimeError that only occurs when the host has NO Anthropic login. It
passed on a machine without credentials and broke the moment a real
`clawcodex login` landed. Now stubs fresh_subscription_credentials to a
sentinel and asserts identity, so it tests the role gate — which is what it
was always meant to test — rather than the developer's login state.
Verified both ways: green with the oauth file present AND absent.
Note for anyone reproducing the job locally: `uv run --isolated` is safe,
but a bare `uv run` in this repo DELETES and recreates .venv.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
/advisorwas unusable for premium Anthropic models on a subscription login, and where it did run, it ran weakly. Seven defects, found by driving the real path rather than reading it.The headline bug —
systemas a string vs a block listAnthropicProvider._prepare_subscription_requestprepends the required "You are Claude Code…" preamble. Given a string it CONCATENATES into one blob; given a list it INSERTS the preamble as its own block at index 0. The subscription endpoint accepts only the latter for premium models.Wire-probed against
claude-opus-5over subscription OAuth, 3/3 per cell:systemshapeNone→ bare preamble string<string>→ preamble + advisor text fused[<block>]→ preamble block + advisor blockThe rejection arrives mislabelled by the API as
{"type":"rate_limit_error","message":"Error"}— so it reads as capacity and invites a pointless backoff hunt rather than a shape fix. Haiku accepts the string form, which is why a cheap smoke test misses it entirely. The main loop has always sent blocks (that is what carriescache_control), which is why only this path was broken.The rest
build_anthropic_thinking_kwargsfrom_call_model_syncso both callers share ONE copy of the model gates (adaptive-vs-budget, effort allowlist, xhigh clamp). Newadvisor_effortsetting +/advisor <provider>:<model> --effort <level>.max_tokensflat 4096. Thinking draws from the SAME budget, so a high-effort reviewer could spend it all reasoning and returnstop_reason=max_tokenswith no text. Now per-model, floored at 4096 and ceiled at 32768 on the OpenAI-compat wire — the table value there is an auto-compact reservation, not a legal request cap (deepseek 384000, luna 128000). Honest scope: DeepSeek accepts 384000 (probed, 200 OK), so this guards an untested number per provider, not a known outage.Retry-After.config.jsononly — the one call site ignoring the environment. Now uses the sharedresolve_api_key(). Empty stays legitimate: the subscription path requires an empty key to fall through to OAuth.xhigh, a level it does not know, so it dropped the field and applied its default: a silent downgrade. Extractednormalize_effort_for_providerso the hook cannot be forgotten a third time.call_kwargs.get("model"); a function-localimport loggingshadowing the module import for the whole scope; a negativetime.sleepin the backoff.Harbor adapter
New
--ak advisor=<provider>:<model>and--ak advisor_effort=.subscription=truenow accepts anthropic in either role — it previously required an anthropic main model, making "cheap API-key worker + premium subscription reviewer" inexpressible. The advisor provider's key is forwarded, andANTHROPIC_API_KEYis withheld via both routes (process env and the seeded configenvblock, whichget_secretalso reads) so OAuth stays the only path to the subscription.Verification
gpt-5.6-lunaworker (API, xhigh) consultingclaude-opus-5(subscription, xhigh) — advisor called twice, real advice both times, worker acted on it_call_model_syncextraction differentially over 17,280 combinations (0 divergences) and killed 11/11 mutants; the other over 2,688 (0 divergences) and 8/8.A recurring theme worth calling out for reviewers: every failure mode here is quiet. A failed consultation lets the worker carry on, so a run can score 1.0 with a reviewer that never once answered — that is exactly how the missing key forwarding was found.
RUN_ADVISOR_TB21.mdleads with how to verify the advisor actually replied.🤖 Generated with Claude Code