fix(effort): send reasoning effort in the provider's own vocabulary - #791
Merged
Conversation
Two bugs found checking how `--effort` reaches deepseek-v4-flash-luna against DeepSeek's thinking-mode docs. **DeepSeek does not share the clawcodex effort ladder.** We expose `low | medium | high | xhigh | max` (Anthropic's), and DeepSeek's OpenAI-format thinking mode accepts `low | high | max` only. Critically it does NOT validate the field — probed 2026-08-03 against deepseek-v4-flash, every one of low/medium/high/xhigh/max/minimal returned 200. So an unsupported level is not an error, it is silently discarded and DeepSeek's default (`high`) applies: the request looks fine and the user gets a level they never chose. The damaging direction is downward. `xhigh` means "more than high", and dropping it delivered `high` when `max` was available — on the setting people reach for precisely when a task is hard. Adds a per-provider `normalize_reasoning_effort` hook, identity by default so OpenAI and OpenRouter (which take all five) are untouched, with DeepSeek declaring its own vocabulary: medium→high (already the de-facto behaviour, now explicit) and xhigh→max (the real fix). **`is_anthropic_wire` was blind to delegating wrappers.** It is an `isinstance` test, and `FusionProvider`'s MRO is `(FusionProvider, object)`, so EVERY fusion model reported OpenAI-compatible — including one whose base is Anthropic. Both things that predicate decides are hard failures when decided wrongly: effort would go out as a top-level `reasoning_effort`, which Anthropic rejects with `400 ... Extra inputs are not permitted`, and the system prompt would be prepended as a message instead of passed as the `system` kwarg. `fusion=anthropic:claude-opus-5+openai:gpt-5.6-luna` is expressible today and would fail on its first request. Adds `unwrap_provider`, bounded against cycles so a self-referential wrapper cannot hang the wire check. The normalize hook's result is VALIDATED, not trusted. It is a duck-typed `getattr` and not every provider-shaped object is a real BaseProvider — a MagicMock answers every attribute with a callable returning another Mock, which unguarded writes a `<MagicMock ...>` repr into the request body as the effort level. Caught by 8 existing tests when I first wired it without the guard. Anything not a plain string on the known ladder is discarded, and a raising hook falls back rather than failing the turn. No explicit unwrap at the normalize call site, deliberately: `FusionProvider.__getattr__` already delegates, so the lookup lands on the base. Mutation-testing proved an unwrap there was dead code, so the delegation is pinned by a test instead of guarded by a redundant line. Suite: 9666 passed, 9 skipped, 0 failures. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Test Results 1 files 1 suites 8m 23s ⏱️ Results for commit 35a9feb. |
ericleepi314
added a commit
that referenced
this pull request
Aug 3, 2026
…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>
ericleepi314
added a commit
that referenced
this pull request
Aug 3, 2026
…give it reasoning effort (#793) * fix(advisor): make the reviewer usable on a Claude subscription, and 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> * fix(eval): forward the ADVISOR provider's key into the container 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> * fix(advisor): resolve the advisor provider's key from the environment 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> * docs(eval): advisor pairing run guide; harden the advisor backoff sleep 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> * fix(advisor): address critic review — bound the OpenAI-wire budget, cover 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> * test(advisor): close the three follow-ups from the re-review 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> * fix(advisor): translate effort into the provider's vocabulary; shut the 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> * test(advisor): pin the OpenAI-wire ceiling to the Anthropic family maximum _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> * ci(harbor): actually run the adapter tests, and make one of them hermetic 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> --------- 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.
Checking how
--effortreachesdeepseek-v4-flash-lunaagainstDeepSeek's thinking-mode docs
turned up two bugs.
1. DeepSeek does not share our effort ladder
We expose
low | medium | high | xhigh | max— Anthropic's ladder. DeepSeek'sOpenAI-format thinking mode accepts
low | high | maxonly, and defaults tohigh.Critically, it does not validate the field. Probed 2026-08-03 against
deepseek-v4-flash, every one oflow / medium / high / xhigh / max / minimalreturned 200. So an unsupported level is not an error — it is silently
discarded and the default applies. The request looks fine and the user gets a
level they never chose.
The damaging direction is downward:
xhighmeans "more than high", anddropping it delivered
highwhenmaxwas available — on the setting peoplereach for precisely when a task is hard.
Adds a per-provider
normalize_reasoning_efforthook, identity by defaultso OpenAI and OpenRouter (which take all five) are untouched. DeepSeek declares
its own vocabulary:
low/high/maxmediumhighxhighmaxhigh2.
is_anthropic_wirewas blind to delegating wrappersIt is an
isinstancetest, andFusionProvider's MRO is(FusionProvider, object)— so every fusion model reportedOpenAI-compatible, including one whose base is Anthropic.
Both things that predicate decides are hard failures when decided wrongly:
effort would ship as a top-level
reasoning_effort, which Anthropic rejectswith
400 ... Extra inputs are not permitted, and the system prompt would beprepended as a message instead of passed as the
systemkwarg.fusion=anthropic:claude-opus-5+openai:gpt-5.6-lunais expressible today andwould fail on its first request.
Adds
unwrap_provider, bounded against cycles so a self-referential wrappercannot hang the wire check.
The hook's result is validated, not trusted
It is a duck-typed
getattr, and not every provider-shaped object is a realBaseProvider. AMagicMockanswers every attribute with a callable returninganother Mock — unguarded, that writes a
<MagicMock ...>repr into the requestbody as the effort level. Eight existing tests caught it when I first wired the
hook without the guard. Anything that is not a plain string on the known ladder
is discarded, and a raising hook falls back rather than failing the turn.
Note on what is not here
There is no explicit unwrap at the normalize call site, deliberately.
FusionProvider.__getattr__already delegates, so the lookup lands on the base.Mutation-testing showed an unwrap there was dead code — no test could
distinguish it — so the delegation is pinned by a test instead of guarded by a
redundant line.
Verification
25 new tests, including end-to-end assertions on the actual
_call_model_syncwire kwargs for both a bare DeepSeek provider and the fusion model. Five
mutants, all caught: reverting the isinstance test, removing DeepSeek's
vocabulary, skipping the hook, breaking the fusion delegation, and dropping the
validation guard.
Suite: 9666 passed, 9 skipped, 0 failures.
🤖 Generated with Claude Code