diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 78e9e7f4..ba253d9e 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -71,9 +71,19 @@ jobs: uses: astral-sh/setup-uv@v5 - name: Run adapter tests + # The harbor-dependent test files ALL belong here. They open with + # `pytest.importorskip("harbor")`, so in the main `test (3.11)` job — + # where harbor is not installed — they skip silently and a green tick + # says nothing about them. This job is the only place they can + # actually run, so a file omitted here is a file that never runs at + # all. Add new tests/test_harbor_*.py files to this list. run: | uv run --isolated --python 3.13 --with harbor --with pytest \ - python -m pytest tests/test_headless_usage_events.py -v + python -m pytest \ + tests/test_headless_usage_events.py \ + tests/test_harbor_adapter_fusion.py \ + tests/test_harbor_adapter_advisor.py \ + -v event_file: name: "Event File" diff --git a/eval/harbor/RUN_ADVISOR_TB21.md b/eval/harbor/RUN_ADVISOR_TB21.md new file mode 100644 index 00000000..84587056 --- /dev/null +++ b/eval/harbor/RUN_ADVISOR_TB21.md @@ -0,0 +1,169 @@ +# Running an ADVISOR pairing on terminal-bench 2.1 + +A cheap API-key worker consulting a premium subscription reviewer: +`openai/gpt-5.6-luna` at effort `xhigh` as the main loop, with +`anthropic:claude-opus-5` at effort `xhigh` as the advisor. + +The advisor tool forwards the whole conversation so far to a stronger +model and feeds its critique back as a tool result. In client-side mode +that is a **separate API call per consultation**, so the reviewer's +provider is independent of the worker's. + +## Prerequisites + +### 1. A build containing the advisor fixes + +Containers install clawcodex from PyPI or from a git ref. The published +package predates this work, so the run **must** pin a source: + +```bash +export CX_SOURCE=git+https://github.com/agentforce314/clawcodex@feat/advisor-subscription-and-effort +``` + +Before this branch the advisor was unusable in exactly this configuration: + +| Defect | Symptom | +|---|---| +| `system` sent as a string | Premium Anthropic models over subscription rejected every consultation with a **mislabelled** `429 rate_limit_error` — it reads as capacity, not shape | +| No thinking / effort on either wire | The reviewer ran with thinking off at the API default | +| Flat `max_tokens=4096` | Thinking draws from the same budget → `stop_reason=max_tokens`, "Advisor returned no text content" | +| No retry | One transient 429/5xx ended the consultation | +| Key read only from `config.json` | An advisor provider whose key lives in an env var got `api_key=""` → "Missing credentials" | +| Adapter forwarded only the main provider's key | Advisor at a different vendor had no credentials at all | + +### 2. Host subscription credentials + +`subscription=true` reads `~/.clawcodex/anthropic-oauth.json` on the host +and refreshes it when under 2h of runway, injecting a refresh-token-free +copy per container. **That file must exist**: + +```bash +clawcodex login # → anthropic → subscription +``` + +A token imported from the Claude Code keychain is *not* sufficient for a +long run: the adapter refreshes below the runway threshold, which needs a +real `refresh_token`. + +> Subscription rate limits are **shared with interactive Claude usage**. +> A wide `--n-concurrent` competes with your own session; the advisor now +> retries transient 429s three times, but sustained saturation still ends +> individual consultations (the worker continues without advice). + +## Smoke first + +```bash +cd /Users/ericlee2/workspace/clawcodex +export OPENAI_API_KEY=$(python3 -c \ + "import json,os;print(json.load(open(os.path.expanduser('~/.clawcodex/config.json')))['providers']['openai']['api_key'])") + +PYTHONPATH=$PWD/eval/harbor harbor run \ + --dataset terminal-bench/terminal-bench-2-1 \ + --agent clawcodex_agent:Clawcodex \ + --model openai/gpt-5.6-luna \ + --ak source=$CX_SOURCE \ + --ak effort=xhigh \ + --ak subscription=true \ + --ak advisor=anthropic:claude-opus-5 \ + --ak advisor_effort=xhigh \ + --jobs-dir eval/harbor/jobs --job-name smoke-advisor-opus5 \ + -i 'terminal-bench/fix-git' --n-concurrent 1 +``` + +**Always verify the advisor actually answered.** A failed consultation +degrades quietly — the worker carries on and can still score 1.0, so the +reward alone will not tell you: + +```bash +python3 - <<'PY' +import json, pathlib +d = sorted(pathlib.Path("eval/harbor/jobs/smoke-advisor-opus5").glob("*/agent/clawcodex.txt")) +for p in d: + calls = answered = 0 + for ln in p.read_text().splitlines(): + try: ev = json.loads(ln) + except Exception: continue + if ev.get("type") == "tool_use" and ev.get("name") == "advisor": + calls += 1 + if ev.get("type") == "tool_result" and "Advisor unavailable" not in (ev.get("output") or ""): + answered += ("Gaps" in (ev.get("output") or "")) + print(f"{p.parts[-3]}: advisor calls={calls} answered={answered}") +PY +``` + +## Full run (89 tasks) + +```bash +cd /Users/ericlee2/workspace/clawcodex +export OPENAI_API_KEY=$(python3 -c \ + "import json,os;print(json.load(open(os.path.expanduser('~/.clawcodex/config.json')))['providers']['openai']['api_key'])") +export CX_SOURCE=git+https://github.com/agentforce314/clawcodex@feat/advisor-subscription-and-effort + +PYTHONPATH=$PWD/eval/harbor harbor run \ + --dataset terminal-bench/terminal-bench-2-1 \ + --agent clawcodex_agent:Clawcodex \ + --model openai/gpt-5.6-luna \ + --ak source=$CX_SOURCE \ + --ak effort=xhigh \ + --ak subscription=true \ + --ak advisor=anthropic:claude-opus-5 \ + --ak advisor_effort=xhigh \ + --jobs-dir eval/harbor/jobs \ + --job-name tb21-luna-xhigh-advisor-opus5 \ + --n-concurrent 4 +``` + +### The control run you need for a comparison + +The advisor's whole point is the delta it produces, and that is only +readable against the same worker with no reviewer: + +```bash +PYTHONPATH=$PWD/eval/harbor harbor run \ + --dataset terminal-bench/terminal-bench-2-1 \ + --agent clawcodex_agent:Clawcodex \ + --model openai/gpt-5.6-luna \ + --ak source=$CX_SOURCE --ak effort=xhigh \ + --jobs-dir eval/harbor/jobs \ + --job-name tb21-luna-xhigh-noadvisor \ + --n-concurrent 4 +``` + +Compare on the **shared scored subset**, never whole-run means — task +difficulty varies enormously and an unequal denominator has inverted +conclusions here before: + +```bash +python3 eval/harbor/compare_trajectories.py \ + eval/harbor/jobs/tb21-luna-xhigh-advisor-opus5 \ + eval/harbor/jobs/tb21-luna-xhigh-noadvisor +``` + +## Cost note + +The advisor roughly **doubles API calls on turns where it fires**, and +each consultation forwards the entire conversation so far — cost grows +with conversation length, not with the size of the advice. The worker +side is cheap ($0.10/$0.60 per Mtok, doubling above 272K prompt tokens); +the reviewer side bills against the subscription (reported as $0 with an +`estimated_cost_usd` computed from list price for observability). + +## Tuning + +* `--ak advisor_effort=` — the reviewer's own level, independent of the + worker's `--ak effort=`. Omit to inherit `effort`; omit both and the + API applies its model default. `xhigh` is accepted on Opus 5 and + clamped to `high` on models that reject it (e.g. Sonnet 4.6). +* `--ak advisor=:` — any configured provider. An + API-key reviewer (`--ak advisor=zai:glm-5.2`) needs no subscription and + no `clawcodex login`; its provider key is forwarded automatically. +* Drop `--ak subscription=true` if you point the advisor at an + API-key Anthropic account instead. + +## Verified + +| | | +|---|---| +| Wire shape | `claude-opus-5` + `thinking={"type":"adaptive"}` + `output_config={"effort":"xhigh"}` + block-list `system` over subscription OAuth — 200, `billing_mode: subscription` | +| Local e2e | luna worker (API, xhigh) → opus-5 advisor (subscription, xhigh), advisor called twice, real advice both times, worker acted on it | +| Container e2e | tb2.1 `fix-git`, advisor calls=3 answered=3, reward 1.0, 0 exceptions (reviewer `zai:glm-5.2`, since host OAuth was not provisioned on this machine) | diff --git a/eval/harbor/clawcodex_agent.py b/eval/harbor/clawcodex_agent.py index 7e5a7bbb..4dbf34db 100644 --- a/eval/harbor/clawcodex_agent.py +++ b/eval/harbor/clawcodex_agent.py @@ -49,6 +49,32 @@ Agent kwargs (``--ak key=value``): +* ``advisor`` — run with an ADVISOR (reviewer) model the worker consults + through the advisor tool, as ``:`` (e.g. + ``advisor=anthropic:claude-opus-5``). Seeded into the container's global + config rather than passed as a flag: clawcodex reads the advisor from + settings, and ``/advisor`` is not reachable on the headless ``-p`` path. + Always seeded as CLIENT-side dispatch (a separate API call per + consultation), so the advisor behaves identically regardless of which + worker model a run is comparing — the server-side path is an + Anthropic-only beta that also requires a 1P Anthropic main loop. + + Combines with ``subscription=true`` even when the MAIN model is another + vendor: ``--model openai/gpt-5.6-luna --ak subscription=true + --ak advisor=anthropic:claude-opus-5`` runs an API-key worker against a + Claude-subscription reviewer. The worker's own provider key is still + forwarded in that configuration; only ``ANTHROPIC_API_KEY`` is withheld, + so OAuth stays the single route to the subscription. + + NOTE the advisor doubles the API calls on turns where it fires, and each + consultation forwards the whole conversation so far — budget for it. +* ``advisor_effort`` — reasoning effort for the ADVISOR's own call + (low|medium|high|xhigh|max), independent of the worker's ``effort``. + Requires ``advisor``. Unset means the advisor inherits ``effort``; if + that is unset too the parameter is omitted and the API applies its model + default. Subject to the same per-wire gates as ``effort`` below — on the + Anthropic wire ``xhigh`` is clamped to ``high`` for models that reject it + (Opus 5 accepts it; Sonnet 4.6 does not). * ``fusion`` — run a FUSION model: a text-only base plus a borrowed vision model, as ``+`` with each side ``provider:model`` (e.g. ``fusion=deepseek:deepseek-v4-flash+openai:gpt-5.6-luna``). Pair @@ -334,6 +360,8 @@ def __init__( source: str | None = None, fusion: str | None = None, forward_keys: bool | str = True, + advisor: str | None = None, + advisor_effort: str | None = None, *args, **kwargs, ): @@ -341,6 +369,37 @@ def __init__( self._subscription = parse_bool_env_value(subscription, name="subscription") self._source = source + # ``advisor`` is ``:`` — the reviewer model the + # worker consults through the advisor tool. Same rationale as + # ``fusion`` for living here rather than in CLI_FLAGS: it is config, + # not a clawcodex flag (there is no ``--advisor``), and it reaches the + # container through the seeded global config instead. + self._advisor = advisor + self._advisor_effort = advisor_effort + if self._advisor: + # Validate BOTH halves, as ``fusion=`` does. A bare colon test + # lets "anthropic:" and ":claude-opus-5" through, and each seeds + # a half-configured advisor that is silently inert at run time — + # the failure mode this adapter keeps producing. + provider_half, _, model_half = self._advisor.partition(":") + if not provider_half.strip() or not model_half.strip(): + raise ValueError( + "Agent kwarg 'advisor' must be ':' with " + f"both halves non-empty (got {self._advisor!r}) — " + "e.g. anthropic:claude-opus-5" + ) + if self._advisor_effort and self._advisor_effort not in ( + "low", "medium", "high", "xhigh", "max", + ): + raise ValueError( + "Agent kwarg 'advisor_effort' must be one of low|medium|" + f"high|xhigh|max (got {self._advisor_effort!r})" + ) + if self._advisor_effort and not self._advisor: + raise ValueError( + "Agent kwarg 'advisor_effort' requires 'advisor' — an effort " + "level with no reviewer configured is silently inert." + ) # Explicit constructor kwargs, NOT CLI_FLAGS entries: both are config # inputs with no clawcodex flag behind them, and every CLI_FLAGS entry # is emitted into the command by ``build_cli_flags()``. Declaring them @@ -446,17 +505,64 @@ async def install(self, environment: BaseEnvironment) -> None: ), ) + def _advisor_provider(self) -> str: + """Provider half of the ``advisor`` kwarg ("" when unset).""" + if not self._advisor: + return "" + return self._advisor.split(":", 1)[0].strip().lower() + + def _advisor_env_vars(self) -> tuple[str, ...]: + """Env keys the ADVISOR provider needs, beyond the main loop's. + + The advisor makes its own API call to its own provider, so a run + whose reviewer sits at a different vendor needs that vendor's key + too. Without this the advisor fires and dies on "Missing + credentials" — and because a failed consultation degrades quietly + (the worker carries on and can still solve the task), the run + LOOKS clean: reward 1.0 with an advisor that never once answered. + Observed exactly that on the first container smoke. + + Anthropic is excluded under ``subscription``: OAuth must stay the + only route there, matching the main-loop rule. + """ + advisor_provider = self._advisor_provider() + if not advisor_provider: + return () + keys = _PROVIDER_ENV_VARS.get(advisor_provider, _ALL_PROVIDER_ENV_VARS) + if self._subscription: + keys = tuple(k for k in keys if k != "ANTHROPIC_API_KEY") + return keys + def _build_env(self) -> dict[str, str]: env: dict[str, str] = {} if self._subscription: # Subscription mode authenticates via the injected OAuth file; # forwarding ANTHROPIC_API_KEY would silently win over it inside # clawcodex (API key takes precedence), billing the API instead. - forwarded: tuple[str, ...] = () + # + # That reasoning is specific to ANTHROPIC. When the subscription + # backs only the advisor, the MAIN loop is a different provider + # and still needs its own key — suppressing everything left the + # worker with no credentials at all. + model_provider = (self._parsed_model_provider or "").lower() + if model_provider and model_provider != "anthropic": + forwarded = _PROVIDER_ENV_VARS.get( + model_provider, _ALL_PROVIDER_ENV_VARS + ) + # Never the Anthropic key: OAuth must stay the only route to + # the subscription, whichever role it is filling. + forwarded = tuple(k for k in forwarded if k != "ANTHROPIC_API_KEY") + else: + forwarded = () else: forwarded = _PROVIDER_ENV_VARS.get( (self._parsed_model_provider or "").lower(), _ALL_PROVIDER_ENV_VARS ) + # The advisor calls its own provider, which may be a different + # vendor than the worker's. Union, deduped, order-preserving. + forwarded = tuple( + dict.fromkeys(forwarded + self._advisor_env_vars()) + ) for key in forwarded: value = self._get_env(key) if value: @@ -499,10 +605,17 @@ async def _inject_subscription_credentials( import asyncio provider = (self._parsed_model_provider or "anthropic").lower() - if provider != "anthropic": + # Subscription auth is legitimate for EITHER role. The original gate + # assumed the subscription always backed the main loop, which made + # the interesting pairing — a cheap API-key worker consulting a + # premium subscription reviewer, e.g. openai/gpt-5.6-luna with + # ``--ak advisor=anthropic:claude-opus-5`` — impossible to express. + if provider != "anthropic" and self._advisor_provider() != "anthropic": raise RuntimeError( - "subscription=true only applies to anthropic/... models " - f"(got provider {provider!r})" + "subscription=true requires anthropic in some role: either an " + "anthropic/... model or --ak advisor=anthropic: " + f"(got model provider {provider!r}, advisor " + f"{self._advisor or '(none)'!r})" ) credentials = dict(await asyncio.to_thread(fresh_subscription_credentials)) credentials["refresh_token"] = "" @@ -533,11 +646,21 @@ def _host_env_keys(self) -> dict[str, str]: return {} if not isinstance(block, dict): return {} - return { + keys = { str(k): str(v) for k, v in block.items() if isinstance(v, (str, int, float)) and str(v).strip() } + if self._subscription: + # This block is a SECOND route to a credential and it defeated the + # process-env exclusion. ``get_secret`` reads the process env and + # THEN this config ``env`` block, so a stored ANTHROPIC_API_KEY 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. Withholding it from the exec env alone was not + # enough; both doors have to be shut. + keys.pop("ANTHROPIC_API_KEY", None) + return keys def _fusion_record(self) -> dict[str, str] | None: """The ``fusionModels`` entry for a ``fusion=`` run, or None. @@ -609,6 +732,22 @@ async def _seed_container_settings( if effort: settings["effort"] = effort + # Advisor (reviewer model). Settings-only by design: clawcodex reads + # the advisor config from settings, there is no CLI flag, and the + # /advisor slash command is not reachable on the headless -p path. + if self._advisor: + advisor_provider, advisor_model = self._advisor.split(":", 1) + settings["advisor_enabled"] = True # master switch, default off + settings["advisor_provider"] = advisor_provider.strip() + settings["advisor_model"] = advisor_model.strip() + # Force client-side dispatch. Server-side is an Anthropic-only + # beta that additionally requires the MAIN loop to be 1P + # Anthropic; pinning client mode keeps the advisor behaving + # identically no matter which worker model a run is comparing. + settings["advisor_client_mode"] = True + if self._advisor_effort: + settings["advisor_effort"] = self._advisor_effort + config: dict[str, Any] = {} if settings: config["settings"] = settings diff --git a/src/command_system/builtins.py b/src/command_system/builtins.py index 2a1082f4..b7931e83 100644 --- a/src/command_system/builtins.py +++ b/src/command_system/builtins.py @@ -680,6 +680,52 @@ def _write_advisor_enabled(context: CommandContext, value: bool) -> None: invalidate_settings_cache() +# Accepted values for ``/advisor --effort``: DERIVED from the canonical +# ladder rather than retyped, so a new level cannot land in one list and +# not the other (``effort_command.py`` derives from the same constant for +# exactly this reason). ``VALID_EFFORT_VALUES`` carries a leading "" as its +# auto sentinel; drop it and expose ``auto`` instead, which CLEARS +# advisor_effort so the advisor inherits the session-wide ``effort`` +# (there's no way to type an empty string as a CLI argument). +def _valid_advisor_efforts() -> tuple[str, ...]: + from ..settings.constants import VALID_EFFORT_VALUES + return ("auto",) + tuple(v for v in VALID_EFFORT_VALUES if v) + + +_VALID_ADVISOR_EFFORTS = _valid_advisor_efforts() + + +def _read_current_advisor_effort(context: CommandContext) -> str: + """Resolve the configured advisor_effort ("" = inherit ``effort``). + + Settings-only, deliberately: unlike model/provider/client_mode there is + no app-state field for this, and inventing a store-preferred read here + would reintroduce the restart-blind bug that made ``/advisor`` report a + stale config while a different one was actually firing. + """ + try: + from ..settings.settings import get_settings + return (getattr(get_settings(), "advisor_effort", "") or "").strip() + except Exception: + return "" + + +def _write_advisor_effort(context: CommandContext, value: str | None) -> None: + """Persist advisor_effort. ``None``/empty clears it (inherit ``effort``).""" + from .. import config as cfg_mod + from ..settings.settings import invalidate_settings_cache + normalized = (value or "").strip().lower() + mgr = cfg_mod._get_default_manager() + cfg = mgr.load_global() + settings_section = cfg.get("settings") + if not isinstance(settings_section, dict): + settings_section = {} + settings_section["advisor_effort"] = normalized + cfg["settings"] = settings_section + mgr.save_global(cfg) + invalidate_settings_cache() + + def advisor_command_call(args: str, context: CommandContext) -> LocalCommandResult: """Handle /advisor — configure the reviewer model. @@ -692,18 +738,26 @@ def advisor_command_call(args: str, context: CommandContext) -> LocalCommandResu openai (litellm), openrouter, bedrock, etc. Name-based inference was ambiguous and silently routed to the wrong endpoint. - Branches (after parsing optional ``--client`` / ``--no-client``): - * no args, no flags → status report (provider/model + mode). + Branches (after parsing optional ``--client`` / ``--no-client`` / + ``--effort ``): + * no args, no flags → status report (provider/model + mode + effort). * ``unset`` | ``off`` → clear advisor_model, advisor_provider, - and advisor_client_mode. + advisor_client_mode, and advisor_effort. * ``--no-client`` alone → keep model+provider, clear client-mode. * ``--client`` alone → keep model+provider, set client-mode. + * ``--effort `` alone → retune the configured advisor; + ``--effort auto`` clears it so the session ``effort`` is inherited. * ``:`` → validate provider exists in config, - persist both fields together. ``--client`` flag (if present) - also persists advisor_client_mode. + persist both fields together. ``--client`` / ``--effort`` flags + (if present) persist alongside. + + ``--effort`` sets the reviewer's OWN reasoning level, which is + independent of the worker's ``/effort`` — the advisor is usually the + stronger model and is often worth running harder than the main loop. Examples: * ``/advisor anthropic:claude-opus-4-7`` (direct Anthropic API) + * ``/advisor anthropic:claude-opus-5 --effort xhigh`` * ``/advisor openai:claude-opus-4-7`` (litellm/proxy via openai provider) * ``/advisor openrouter:anthropic/claude-opus-4.1`` * ``/advisor gemini:gemini-2.5-pro`` @@ -736,14 +790,40 @@ def advisor_command_call(args: str, context: CommandContext) -> LocalCommandResu # off cleanly without breaking the model identifier. raw_tokens = (args or "").strip().split() force_client_flag: bool | None = None # None = no flag passed + effort_flag: str | None = None # None = no flag passed rest_tokens: list[str] = [] + _expect_effort = False for tok in raw_tokens: - if tok == "--client": + if _expect_effort: + effort_flag = tok.strip().lower() + _expect_effort = False + elif tok == "--client": force_client_flag = True elif tok == "--no-client": force_client_flag = False + elif tok == "--effort": + # Value arrives as the NEXT token. + _expect_effort = True + elif tok.startswith("--effort="): + effort_flag = tok.split("=", 1)[1].strip().lower() else: rest_tokens.append(tok) + if _expect_effort: + return LocalCommandResult( + type="text", + value=( + "--effort needs a level. Expected one of: " + f"{', '.join(_VALID_ADVISOR_EFFORTS)}." + ), + ) + if effort_flag is not None and effort_flag not in _VALID_ADVISOR_EFFORTS: + return LocalCommandResult( + type="text", + value=( + f"Invalid effort {effort_flag!r}. Expected one of: " + f"{', '.join(_VALID_ADVISOR_EFFORTS)}." + ), + ) arg = " ".join(rest_tokens).strip() arg_lower = arg.lower() @@ -751,6 +831,7 @@ def advisor_command_call(args: str, context: CommandContext) -> LocalCommandResu current_provider = _read_current_advisor_provider(context) current_client_mode = _read_current_advisor_client_mode(context) current_enabled = _read_current_advisor_enabled(context) + current_effort = _read_current_advisor_effort(context) main_loop_model = "" if provider is not None: @@ -822,16 +903,72 @@ def _render_status() -> str: suffix = " [--client forced]" if not current_enabled: suffix += " [disabled: advisor_enabled is off]" + # Show where the advisor's effort comes from — an inherited level + # looks identical on the wire to an explicitly set one, and the + # difference matters the moment the session ``effort`` changes. + if current_effort: + effort_line = f"Effort: {current_effort}\n" + else: + try: + from ..settings.settings import get_settings + inherited = (getattr(get_settings(), "effort", "") or "").strip() + except Exception: + inherited = "" + # Name the SETTING, not the command. ``/effort`` only reaches + # ``settings.effort`` on the registry path (REPL/SDK) and when an + # eval adapter seeds it; the TUI's ``/effort`` writes a + # session-only field and headless ``--effort`` is per-turn, so + # crediting "/effort" would assert a link that does not exist on + # the surface most users are looking at. + effort_line = ( + f"Effort: {inherited} (inherited from settings.effort)\n" + if inherited + else "Effort: model default (set one with --effort)\n" + ) return ( f"Advisor: {current_provider}:{current_advisor} — {mode_label}{suffix}\n" + f"{effort_line}" 'Use "/advisor unset" to disable or ' '"/advisor :" to change.' ) # No model arg, no flags → status only. - if not arg and force_client_flag is None: + if not arg and force_client_flag is None and effort_flag is None: return LocalCommandResult(type="text", value=_render_status()) + # ``--effort `` with no model → retune the existing advisor. + # Handled before the client-mode branches so ``--effort`` can be + # combined with them or stand alone. + if not arg and effort_flag is not None: + if not current_advisor or not current_provider: + return LocalCommandResult( + type="text", + value=( + "Cannot set advisor effort: advisor is not configured. " + 'Use "/advisor : --effort ".' + ), + ) + _write_advisor_effort( + context, None if effort_flag == "auto" else effort_flag + ) + if force_client_flag is not None: + _write_advisor_client_mode(context, force_client_flag) + if effort_flag == "auto": + return LocalCommandResult( + type="text", + value=( + "Advisor effort cleared — it now inherits the session " + "effort (/effort)." + ), + ) + return LocalCommandResult( + type="text", + value=( + f"Advisor effort set to {effort_flag} for " + f"{current_provider}:{current_advisor}." + ), + ) + # --no-client alone (no model) → just clear the forced-client flag. if not arg and force_client_flag is False: if not current_client_mode: @@ -874,6 +1011,17 @@ def _render_status() -> str: ) if arg_lower in ("unset", "off"): + # ``unset`` clears advisor_effort anyway, so a level passed alongside + # it can only be a mistake. Erroring beats silently discarding it. + if effort_flag is not None: + return LocalCommandResult( + type="text", + value=( + f"'{arg_lower}' clears the advisor entirely — drop the " + "--effort flag, or use \"/advisor --effort \" to " + "retune the current one." + ), + ) previous_model = current_advisor previous_provider = current_provider if previous_model: @@ -884,6 +1032,10 @@ def _render_status() -> str: _write_advisor_client_mode(context, False) if current_enabled: _write_advisor_enabled(context, False) # master switch off + if current_effort: + # Clear the effort too, so a later /advisor doesn't silently + # inherit a level the user set for a different reviewer model. + _write_advisor_effort(context, None) if previous_model or previous_provider: prior = ( f"{previous_provider}:{previous_model}" @@ -960,6 +1112,25 @@ def _render_status() -> str: _write_advisor_client_mode(context, True) elif force_client_flag is False: _write_advisor_client_mode(context, False) + carried_over = "" + if effort_flag is not None: + _write_advisor_effort( + context, None if effort_flag == "auto" else effort_flag + ) + current_effort = "" if effort_flag == "auto" else effort_flag + elif current_effort and normalized != current_advisor: + # Switching REVIEWER MODEL with no explicit level: a leftover effort + # belongs to the old model. Same rationale the ``unset`` branch + # states — and it can be actively wrong rather than merely stale, + # because the xhigh clamp is keyed on Anthropic model NAMES, so an + # xhigh left over from an Opus advisor goes out UNCLAMPED to a + # newly-selected OpenAI-compatible one. + _write_advisor_effort(context, None) + carried_over = ( + f" Cleared effort {current_effort!r} (it was set for " + f"{current_advisor})." + ) + current_effort = "" # Report what mode the chosen pair lands in, so the user can spot # mismatches immediately (e.g., they expected server-side but the @@ -986,9 +1157,13 @@ def _render_status() -> str: "Note: advisor is currently inactive (no path applies for " f"main loop {main_loop_model!r} + advisor {normalized!r})." ) + effort_msg = f" Effort: {current_effort}." if current_effort else "" return LocalCommandResult( type="text", - value=f"Advisor set to {provider_part}:{normalized}. {mode_msg}", + value=( + f"Advisor set to {provider_part}:{normalized}." + f"{effort_msg}{carried_over} {mode_msg}" + ), ) diff --git a/src/query/query.py b/src/query/query.py index 39f0f443..a26c118a 100644 --- a/src/query/query.py +++ b/src/query/query.py @@ -607,6 +607,112 @@ def resolve_thinking_effort( return value +def normalize_effort_for_provider( + provider: Any, resolved_effort: str | None +) -> str | None: + """Translate an effort level into the provider's own vocabulary. + + THE single source of truth for this step, shared by the main loop + (:func:`_call_model_sync`) and the client-side advisor. Both send + ``extra_body.reasoning_effort`` on non-Anthropic wires and both must + translate first — the advisor originally skipped it and so silently + reintroduced the bug the hook exists to prevent: a DeepSeek advisor got + ``xhigh`` where the main loop sends ``max``, and DeepSeek — which does + not know ``xhigh`` — drops the field and applies 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. + + The result is VALIDATED, not trusted. This is a duck-typed ``getattr`` + on whatever the caller passed, and providers are not all real + ``BaseProvider`` subclasses — mocks, gateway shims and third-party + wrappers all reach here. A ``MagicMock`` in particular answers every + attribute with a callable returning another Mock, so an unguarded + assignment writes a ```` repr into the request body. + Anything that is not a plain string on the known ladder is discarded in + favour of the level already resolved. + """ + if resolved_effort is None: + return None + _normalize = getattr(provider, "normalize_reasoning_effort", None) + if not callable(_normalize): + return resolved_effort + try: + _mapped = _normalize(resolved_effort) + except Exception: # noqa: BLE001 — never fail a request on this + return resolved_effort + if isinstance(_mapped, str) and _mapped in VALID_THINKING_EFFORT_LEVELS: + return _mapped + return resolved_effort + + +def build_anthropic_thinking_kwargs( + model: str | None, + *, + explicit_effort: str | None = None, + max_tokens: int = 0, + force_thinking: bool = False, +) -> dict[str, Any]: + """Build the Anthropic-wire ``thinking`` / ``output_config`` kwargs. + + THE single source of truth for the adaptive-vs-budget selection and + the effort gate. Two callers share it and must never drift: + + * the main loop (:func:`_call_model_sync`), and + * the client-side advisor (``src/utils/advisor.py``), whose separate + API call previously sent neither parameter — so a reviewer model + configured precisely because it reasons harder ran with thinking + off and the API's default effort. + + The adaptive-vs-budget split mirrors TS claude.ts:1612-1640: models + that support the adaptive type get ``{type: "adaptive"}``; models that + support thinking but not adaptive get an explicit ``budget_tokens`` + instead, because adaptive is a hard 400 on them. ``output_config.effort`` + is gated separately and more narrowly (:func:`_model_supports_effort`). + + ``max_tokens`` is only consulted on the budget branch, where the budget + must land in ``[1024, max_tokens)``; pass the value already resolved for + the request. ``force_thinking`` mirrors the caller's explicit + ``extended_thinking=True`` override, which enables thinking even for a + model the eligibility regex doesn't recognise. + + Returns a dict to merge into the request kwargs — empty when the model + supports neither, so merging is always safe. + """ + out: dict[str, Any] = {} + if not (force_thinking or _model_supports_extended_thinking(model)): + return out + if _model_supports_adaptive_thinking(model): + out["thinking"] = {"type": "adaptive"} + else: + # Budget thinking is the safe direction — it works on any + # thinking-capable model, whereas adaptive 400s where unsupported. + # TS defaults an unknown first-party alias to adaptive + # (thinking.ts:178-183); we deliberately prefer budget. TS clamps to + # maxOutputTokens-1 (claude.ts:1628-1635); we clamp identically. + _max_tok = int(max_tokens or 0) + if _max_tok > 1024: + out["thinking"] = { + "type": "enabled", + "budget_tokens": max(1024, _max_tok - 1), + } + else: + # No budget in [1024, max_tokens) fits — omit thinking rather + # than send an invalid request (max_tokens this small only + # happens via an explicit tiny override). + logger.debug( + "thinking omitted: max_tokens=%s too small for a " + "valid budget on non-adaptive model %s", + _max_tok, model, + ) + if _model_supports_effort(model): + resolved_effort = resolve_thinking_effort(explicit_effort, model) + # None = nothing requested anywhere — omit the parameter so the API + # applies its own model default (TS parity; see resolve_thinking_effort). + if resolved_effort is not None: + out["output_config"] = {"effort": resolved_effort} + return out + + def _is_overloaded_error(e: Exception) -> bool: """Anthropic 529 / overloaded_error classification (duck-typed so test fakes and other providers' shapes participate).""" @@ -1134,43 +1240,14 @@ async def _call_model_sync( # subscription/OAuth path, which is how this surfaced. if extended_thinking is not False and is_anthropic: provider_model = getattr(provider, "model", None) or call_kwargs.get("model") - if extended_thinking is True or _model_supports_extended_thinking(provider_model): - if _model_supports_adaptive_thinking(provider_model): - call_kwargs["thinking"] = {"type": "adaptive"} - else: - # Non-adaptive (incl. an explicit ``extended_thinking=True`` - # on an unknown alias): budget thinking. This is the safe - # direction — budget works on any thinking-capable model, - # whereas adaptive 400s where unsupported; TS defaults an - # unknown first-party alias to adaptive (thinking.ts:178-183), - # we deliberately prefer budget. budget_tokens must be ≥ 1024 - # (API minimum) and strictly less than max_tokens, which is - # already resolved above for every Anthropic request. TS uses - # maxOutputTokens-1 (claude.ts:1628-1635); we clamp identically. - _max_tok = int(call_kwargs.get("max_tokens") or 0) - if _max_tok > 1024: - call_kwargs["thinking"] = { - "type": "enabled", - "budget_tokens": max(1024, _max_tok - 1), - } - else: - # No budget in [1024, max_tokens) fits — omit thinking - # rather than send an invalid request (max_tokens this - # small only happens via an explicit tiny override). - logger.debug( - "thinking omitted: max_tokens=%s too small for a " - "valid budget on non-adaptive model %s", - _max_tok, provider_model, - ) - if _model_supports_effort(provider_model): - resolved_effort = resolve_thinking_effort( - thinking_effort, provider_model - ) - # None = nothing requested anywhere — omit the parameter so - # the API applies its own model default (TS parity; see - # resolve_thinking_effort). - if resolved_effort is not None: - call_kwargs["output_config"] = {"effort": resolved_effort} + call_kwargs.update( + build_anthropic_thinking_kwargs( + provider_model, + explicit_effort=thinking_effort, + max_tokens=int(call_kwargs.get("max_tokens") or 0), + force_thinking=extended_thinking is True, + ) + ) elif not is_anthropic: # NON-Anthropic wire: reasoning effort is a top-level # ``reasoning_effort`` body field, NOT ``output_config``. This is the @@ -1239,15 +1316,7 @@ async def _call_model_sync( # unguarded assignment writes a ```` repr into the # request body. Anything that is not a plain string on the known ladder # is discarded in favour of the level we already resolved. - if resolved_effort is not None: - _normalize = getattr(provider, "normalize_reasoning_effort", None) - if callable(_normalize): - try: - _mapped = _normalize(resolved_effort) - except Exception: # noqa: BLE001 — never fail a request on this - _mapped = None - if isinstance(_mapped, str) and _mapped in VALID_THINKING_EFFORT_LEVELS: - resolved_effort = _mapped + resolved_effort = normalize_effort_for_provider(provider, resolved_effort) if resolved_effort is not None: extra_body = dict(call_kwargs.get("extra_body") or {}) extra_body.setdefault("reasoning_effort", resolved_effort) diff --git a/src/settings/types.py b/src/settings/types.py index 9440a0b7..ec5fabc2 100644 --- a/src/settings/types.py +++ b/src/settings/types.py @@ -161,6 +161,13 @@ class SettingsSchema: # block, or by running ``/advisor :`` (which flips it on). # ``decide_advisor_mode`` returns INACTIVE whenever this is False. advisor_enabled: bool = False + # Reasoning-effort level for the advisor's OWN API call, independent of + # the worker's ``effort`` above — the reviewer is usually a stronger model + # and is often worth running harder than the main loop. One of + # VALID_EFFORT_VALUES. Empty = inherit ``effort``; if that is empty too, + # the parameter is omitted and the API applies its model default. + # Set via ``/advisor : --effort ``. + advisor_effort: str = "" # Auto-mode transcript classifier (ch06 round-4 PR-B). The # ``feature('TRANSCRIPT_CLASSIFIER')`` analog: default OFF, so `auto` diff --git a/src/settings/validation.py b/src/settings/validation.py index 230cc3fc..83fbd14f 100644 --- a/src/settings/validation.py +++ b/src/settings/validation.py @@ -33,6 +33,18 @@ def validate_settings(settings: SettingsSchema) -> list[ValidationError]: value=settings.effort, )) + # Advisor effort — same ladder as ``effort``; empty inherits it. + advisor_effort = getattr(settings, "advisor_effort", "") + if advisor_effort and advisor_effort not in VALID_EFFORT_VALUES: + errors.append(ValidationError( + field="advisor_effort", + message=( + f"Invalid advisor_effort value: {advisor_effort!r}. " + f"Must be one of {VALID_EFFORT_VALUES}" + ), + value=advisor_effort, + )) + # Permission mode if settings.permission_mode not in VALID_PERMISSION_MODES: errors.append(ValidationError( diff --git a/src/utils/advisor.py b/src/utils/advisor.py index a1c0e7c8..a8ccc2be 100644 --- a/src/utils/advisor.py +++ b/src/utils/advisor.py @@ -30,6 +30,7 @@ from __future__ import annotations +import logging import os import time from typing import Any, Mapping, TYPE_CHECKING @@ -663,6 +664,106 @@ def build_advisor_forwarded_messages( return flattened +# Historical flat output budget for an advisor call, now a FLOOR rather +# than the value (see the budget note in ``execute_client_advisor``). +_ADVISOR_MIN_MAX_TOKENS = 4096 + +# Ceiling for the OpenAI-compatible wire, where the per-model table is an +# advisory compaction figure rather than a legal request cap. See the budget +# note in ``execute_client_advisor`` for why the two wires differ. +# +# The number is a RULE, not a taste: 32768 is the largest +# ``max_output_tokens`` in the entire Anthropic family, so the ceiling says +# "the OpenAI wire never gets a larger budget than the most generous +# Anthropic model gets". Update it if that family maximum moves, not +# otherwise. It sits far above any advisor critique (~1-2K plus reasoning) +# and far below the outliers it exists to stop (deepseek 384000). +# +# Note it also caps a CLAUDE_CODE_MAX_OUTPUT_TOKENS override on this wire +# (resolve_max_output_tokens consults the env before the table), while the +# Anthropic wire still honours such an override whole. That asymmetry is +# intended: the override is a main-loop budget knob, and the reason to bound +# this wire — untested numbers reaching completions.create() — applies to an +# env-supplied value just as much as to a table one. +_ADVISOR_MAX_OPENAI_WIRE_TOKENS = 32_768 + +# Transient-failure budget for one consultation. Deliberately far below the +# main loop's DEFAULT_MAX_RETRIES (10): the advisor is an auxiliary call the +# worker is blocked on, so a rate-limited reviewer must degrade to "no advice" +# in seconds rather than stall the turn for minutes. Before this existed a +# single 429 — routine on a subscription bucket shared with an interactive +# session — ended the consultation outright. +_ADVISOR_MAX_ATTEMPTS = 3 +_ADVISOR_RETRY_BASE_DELAY = 2.0 +_ADVISOR_RETRY_MAX_DELAY = 30.0 + + +def _resolve_advisor_effort() -> str | None: + """Reasoning-effort level for the advisor's own API call. + + ``advisor_effort`` when the user set one (so the reviewer can be dialled + independently of the worker), else the session-wide ``effort``. Returns + ``None`` when neither is set, which leaves the parameter off the wire and + lets the API apply its own default — same omit-don't-guess contract as + :func:`~src.query.query.resolve_thinking_effort`. + """ + try: + from src.settings.settings import get_settings + + settings = get_settings() + except Exception: # noqa: BLE001 — settings must never break the advisor + return None + for attr in ("advisor_effort", "effort"): + value = (getattr(settings, attr, "") or "").strip().lower() + if value: + return value + return None + + +def _advisor_error_is_retryable(exc: Exception) -> bool: + """Whether one failed advisor attempt is worth re-issuing. + + Reuses the main loop's classifier so the two agree on what "transient" + means. The trailing checks are a BACKSTOP, not the primary path: + ``categorize_retryable_api_error`` already has an explicit overloaded + lane, so the ``status_code == 529`` arm is unreachable in practice. The + prose arm still earns its place — it catches an overloaded error raised + as a bare exception carrying no ``status_code`` at all, which the + classifier cannot categorise. + """ + try: + from src.services.api.errors import ( + categorize_retryable_api_error, + is_quota_exhausted, + ) + + if is_quota_exhausted(exc): + return False + if categorize_retryable_api_error(exc).retryable: + return True + except Exception: # noqa: BLE001 — classifier unavailable: fall through + pass + status = getattr(exc, "status_code", None) + if status == 529: + return True + text = str(exc).lower() + return "overloaded" in text + + +def _advisor_sleep(delay: float, abort_signal: Any) -> bool: + """Abort-aware backoff. Returns False if the wait was cut short by an + abort, so the caller stops retrying instead of sleeping through an ESC.""" + deadline = time.monotonic() + delay + while time.monotonic() < deadline: + if abort_signal is not None and getattr(abort_signal, "aborted", False): + return False + # ``max(0.0, …)``: the loop condition and this expression read the + # clock separately, so the remainder can go negative in between — + # and ``time.sleep`` raises ValueError on a negative argument. + time.sleep(max(0.0, min(0.25, deadline - time.monotonic()))) + return True + + def execute_client_advisor( advisor_model: str, forwarded_messages: list[dict[str, Any]], @@ -738,8 +839,22 @@ def execute_client_advisor( # Translate explicitly so unknown keys (default_model, plus any # future config fields like extra_headers) don't get forwarded # as kwargs and crash the constructor. + # Resolve the key the way every other call site does: configured + # ``providers..api_key`` first, then the provider's known env + # vars via the secret store. Reading cfg_raw["api_key"] directly + # meant the advisor was the ONE path that ignored the environment, + # so an advisor provider whose key lives in ``ZAI_API_KEY`` (how + # eval containers and plenty of shells supply credentials) got + # ``api_key=""`` and died on "Missing credentials" — while the same + # provider worked fine as the main loop. + # + # Empty is still a legitimate outcome and must stay non-fatal here: + # the Anthropic subscription path REQUIRES an empty key to fall + # through to OAuth (a key would silently outrank it). + from src.providers import resolve_api_key + provider = provider_cls( - api_key=cfg_raw.get("api_key", ""), + api_key=resolve_api_key(advisor_provider, cfg_raw), base_url=cfg_raw.get("base_url"), model=advisor_model, ) @@ -760,13 +875,108 @@ def execute_client_advisor( is_anthropic_shape = is_anthropic_wire(provider) + # Output budget. The advisor used to send a flat 4096, which was fine + # while it sent no thinking — but thinking tokens are drawn from the + # SAME max_tokens budget as the reply, so a high-effort reviewer can + # spend the entire allowance reasoning and come back with + # ``stop_reason=max_tokens`` and no text at all (surfacing here as the + # useless "Advisor returned no text content"). Floored at the historical + # 4096 so this can only ever widen the budget. It's a cap, not a target: + # an advisor that answers in 300 tokens still costs 300 tokens. + # + # The ceiling is WIRE-DEPENDENT, and the two families are not symmetric: + # + # * Anthropic — ``max_output_tokens`` IS the request's max_tokens by + # design (resolve_max_output_tokens is exactly what the main loop + # sends), so take the table value whole. + # * OpenAI-compatible — the main loop deliberately sends NO max_tokens + # here, and the table value is ADVISORY on this wire: it is tuned as + # an auto-compact reservation, not as a legal request cap (deepseek's + # row is 384000, luna's 128000). ``openai_compatible`` DOES forward + # max_tokens to completions.create(), so the raw table value really + # does reach the wire. + # + # Honest scope: DeepSeek accepts 384000 (probed 2026-08-02, 200 OK), + # so this is not a live outage — it is an untested number per + # provider across a registry of ~30, where a rejection would be a + # 400: non-retryable, so the consultation dies on attempt 1 and + # degrades SILENTLY (the worker continues and the task can still + # score). Clamp to a value large enough that reasoning tokens can't + # starve the reply, small enough to stay unremarkable on any wire. + max_tokens = _ADVISOR_MIN_MAX_TOKENS + try: + from src.models.context import resolve_max_output_tokens + + table = int( + resolve_max_output_tokens( + None, advisor_model, base_url=cfg_raw.get("base_url") + ) + or 0 + ) + if not is_anthropic_shape: + table = min(table, _ADVISOR_MAX_OPENAI_WIRE_TOKENS) + max_tokens = max(max_tokens, table) + except Exception: # noqa: BLE001 — unknown model / bad env falls back + pass + call_kwargs: dict[str, Any] = { "tools": [], - "max_tokens": 4096, + "max_tokens": max_tokens, } + + # Reasoning effort for the advisor's own call. Without this the + # reviewer — chosen precisely because it reasons harder than the + # worker — ran with thinking off and the API's default effort, on + # both wires. ``advisor_effort`` wins when set so the advisor can be + # dialled independently of the main loop; otherwise the session-wide + # ``effort`` applies, matching what the worker is running at. + effort = _resolve_advisor_effort() + if is_anthropic_shape: - call_kwargs["system"] = CLIENT_ADVISOR_SYSTEM_PROMPT + # System goes as a BLOCK LIST, not a bare string. This is not a + # style choice — it is load-bearing on the Claude subscription + # (OAuth) path, and getting it wrong broke the advisor outright for + # premium models. + # + # ``_prepare_subscription_request`` prepends the "You are Claude + # Code…" preamble that the subscription endpoint requires. With a + # STRING it concatenates, producing one blob of + # ``preamble + "\n\n" + advisor prompt``; with a LIST it inserts the + # preamble as its own block at index 0. The endpoint only accepts + # the latter for premium models: wire-probed 2026-08-02 against + # claude-opus-5 over subscription OAuth, 3/3 per cell — + # + # system=None (bare preamble string) -> 200 + # system= (preamble + text) -> 429 + # system=[] (preamble block + text block) -> 200 + # + # and the rejection arrives MISLABELLED as + # ``{"type": "rate_limit_error", "message": "Error"}``, so it reads + # as capacity and invites a pointless backoff hunt. Haiku accepts + # the string form, which is why a cheap smoke test misses this. + # The main loop has always sent blocks (that is what carries the + # cache_control markers), which is why it works on subscription + # while this path did not. + call_kwargs["system"] = [ + {"type": "text", "text": CLIENT_ADVISOR_SYSTEM_PROMPT} + ] request_messages = list(forwarded_messages) + # Shared with the main loop so the model gates (adaptive-vs-budget + # thinking, the effort allowlist, the xhigh clamp) can't drift. + try: + from src.query.query import build_anthropic_thinking_kwargs + + call_kwargs.update( + build_anthropic_thinking_kwargs( + advisor_model, + explicit_effort=effort, + max_tokens=max_tokens, + ) + ) + except Exception: # noqa: BLE001 — never let this break the call + logging.getLogger(__name__).debug( + "advisor thinking kwargs failed", exc_info=True + ) else: # Prepend the system message; OpenAI-compat will honor it # naturally as the first message in the conversation. @@ -774,6 +984,37 @@ def execute_client_advisor( {"role": "system", "content": CLIENT_ADVISOR_SYSTEM_PROMPT}, *forwarded_messages, ] + # NON-Anthropic wire: effort is a top-level ``reasoning_effort`` + # body field, not ``output_config``. ``clamp_xhigh=False`` because + # the xhigh allowlist is a list of Anthropic model NAMES and matches + # nothing here — clamping on it silently downgraded every xhigh. + # Mirrors the equivalent branch in query.py::_call_model_sync, + # INCLUDING the provider-vocabulary translation: without it a + # DeepSeek advisor received ``xhigh`` where the main loop sends + # ``max``, and DeepSeek drops a level it doesn't know and applies + # its default — silently downgraded, which is the exact bug the + # normalize hook exists to prevent. + if effort: + try: + from src.query.query import ( + normalize_effort_for_provider, + resolve_thinking_effort, + ) + + resolved = normalize_effort_for_provider( + provider, + resolve_thinking_effort( + effort, advisor_model, clamp_xhigh=False + ), + ) + if resolved is not None: + extra_body = dict(call_kwargs.get("extra_body") or {}) + extra_body["reasoning_effort"] = resolved + call_kwargs["extra_body"] = extra_body + except Exception: # noqa: BLE001 — never let this break the call + logging.getLogger(__name__).debug( + "advisor reasoning_effort failed", exc_info=True + ) # ``chat_stream_response`` is the cross-provider call that accepts # ``abort_signal`` uniformly (per BaseProvider) and returns a fully @@ -783,22 +1024,76 @@ def execute_client_advisor( # Anthropic (line 239 of anthropic_provider.py forwards unknown # kwargs straight to ``messages.create``). Streaming under the hood # but no ``on_text_chunk`` callback — we only need the final text. + # + # Bounded retry on transient failures. A consultation used to die on the + # first 429/5xx/connection blip; on a subscription bucket shared with an + # interactive session those are routine, so the reviewer was effectively + # unavailable under exactly the load an eval produces. Non-retryable + # errors (bad key, quota exhausted, 400) still fail on attempt 1. _t0 = time.monotonic() - try: + response = None + for attempt in range(1, _ADVISOR_MAX_ATTEMPTS + 1): try: - response = provider.chat_stream_response( - request_messages, - on_text_chunk=None, - abort_signal=abort_signal, - **call_kwargs, + try: + response = provider.chat_stream_response( + request_messages, + on_text_chunk=None, + abort_signal=abort_signal, + **call_kwargs, + ) + except (NotImplementedError, AttributeError): + # Older or stub providers may not implement streaming. + # Fall back to plain chat() — drop abort_signal there since + # we can't pass it portably. + response = provider.chat(request_messages, **call_kwargs) + break + except Exception as e: # noqa: BLE001 — surface as advisor failure + if attempt >= _ADVISOR_MAX_ATTEMPTS or not _advisor_error_is_retryable(e): + # INFO, not DEBUG: a consultation that gives up is invisible + # otherwise. The worker carries on and the task can still + # score, so a run that quietly lost its advisor looks + # identical to one where the advisor worked — the failure + # shows up only as a token-count difference. + logging.getLogger(__name__).info( + "advisor consultation failed after %d attempt(s) " + "(%s: %s); continuing without advice", + attempt, type(e).__name__, str(e)[:200], + ) + return ( + False, + f"Advisor unavailable: {type(e).__name__}: {e}", + _zero_usage, + ) + delay = min( + _ADVISOR_RETRY_BASE_DELAY * (2 ** (attempt - 1)), + _ADVISOR_RETRY_MAX_DELAY, ) - except (NotImplementedError, AttributeError): - # Older or stub providers may not implement streaming. - # Fall back to plain chat() — drop abort_signal there since - # we can't pass it portably. - response = provider.chat(request_messages, **call_kwargs) - except Exception as e: # noqa: BLE001 — surface as advisor failure - return (False, f"Advisor unavailable: {type(e).__name__}: {e}", _zero_usage) + # A rate limiter that tells us when to come back beats guessing: + # exponential backoff from a 2s base clears a burst but not a + # per-minute subscription window. Reuses the main loop's header + # reader so both lanes honour Retry-After identically; the + # helper's own clamp keeps a hostile header from parking the + # worker indefinitely. + try: + from src.query.query import _retry_after_seconds + + delay = min( + _retry_after_seconds(e, delay), _ADVISOR_RETRY_MAX_DELAY + ) + except Exception: # noqa: BLE001 — header unavailable: keep backoff + pass + logging.getLogger(__name__).debug( + "advisor attempt %d/%d failed (%s); retrying in %.1fs", + attempt, _ADVISOR_MAX_ATTEMPTS, type(e).__name__, delay, + ) + if not _advisor_sleep(delay, abort_signal): + return ( + False, + f"Advisor unavailable: {type(e).__name__}: {e}", + _zero_usage, + ) + if response is None: # pragma: no cover — loop returns or breaks + return (False, "Advisor unavailable: no response", _zero_usage) # Pull token counts off the ChatResponse for the session # accumulator. Defaults to zero when the provider didn't return @@ -817,17 +1112,24 @@ def execute_client_advisor( from src.bootstrap.state import add_to_total_duration_state from src.cost_tracker import record_api_usage + # ``call_kwargs`` never carries a ``model`` key — the model rides on + # the provider instance, which is constructed per consultation with + # ``model=advisor_model``. Reading it from call_kwargs was dead and + # made the attribution look configurable when it wasn't. record_api_usage( - call_kwargs.get("model") - or getattr(response, "model", None) - or getattr(provider, "model", "unknown"), + getattr(response, "model", None) + or getattr(provider, "model", None) + or advisor_model + or "unknown", raw_usage, ) _api_ms = int((time.monotonic() - _t0) * 1000) add_to_total_duration_state(_api_ms, _api_ms) except Exception: - import logging - + # NOTE: no function-local ``import logging`` here — a local import + # binds the name for the WHOLE function scope, which shadowed the + # module-level import and made every earlier logging call in this + # function an UnboundLocalError. logging.getLogger(__name__).debug( "advisor cost recording failed", exc_info=True ) diff --git a/tests/test_advisor_client_side.py b/tests/test_advisor_client_side.py index a3422e58..f50a101c 100644 --- a/tests/test_advisor_client_side.py +++ b/tests/test_advisor_client_side.py @@ -341,7 +341,14 @@ def test_returns_text_on_success_anthropic(self) -> None: call = fake_provider.chat_stream_response.call_args self.assertEqual(call.kwargs.get("tools"), []) self.assertIn("system", call.kwargs) - self.assertIn("reviewer", call.kwargs["system"].lower()) + # Sent as a BLOCK LIST, not a string. This assertion used to read + # ``call.kwargs["system"].lower()`` and so pinned the string shape — + # which is precisely what the Claude subscription endpoint rejects + # for premium models (mislabelled as a 429 rate_limit_error). See + # tests/test_advisor_effort_wiring.py::TestSubscriptionSystemShape. + system = call.kwargs["system"] + self.assertIsInstance(system, list) + self.assertIn("reviewer", system[0]["text"].lower()) # Messages array unchanged (no system message prepended). forwarded_messages = call.args[0] self.assertEqual(forwarded_messages[0].get("role"), "user") diff --git a/tests/test_advisor_command.py b/tests/test_advisor_command.py index 3902b27d..01d3abd5 100644 --- a/tests/test_advisor_command.py +++ b/tests/test_advisor_command.py @@ -313,5 +313,121 @@ def test_store_setstate_invalidates_settings_cache(self) -> None: self.assertEqual(store.get_state().advisor_model, "claude-opus-4-6") +class TestAdvisorEffortFlag(unittest.TestCase): + """``/advisor --effort `` — the reviewer's own reasoning level, + independent of the worker's ``/effort``.""" + + def _settings(self): + from src.settings.settings import get_settings, invalidate_settings_cache + invalidate_settings_cache() + return get_settings() + + def test_set_with_model_persists_effort(self) -> None: + with _IsolatedEnv(): + ctx = _make_context(provider=_fake_first_party_provider()) + res = advisor_command_call( + "anthropic:claude-opus-4-6 --effort xhigh", ctx + ) + self.assertIn("Effort: xhigh", res.value) + self.assertEqual(self._settings().advisor_effort, "xhigh") + + def test_flag_order_is_insensitive(self) -> None: + with _IsolatedEnv(): + ctx = _make_context(provider=_fake_first_party_provider()) + advisor_command_call( + "--effort max anthropic:claude-opus-4-6 --client", ctx + ) + s = self._settings() + self.assertEqual(s.advisor_effort, "max") + self.assertEqual(s.advisor_model, "claude-opus-4-6") + self.assertTrue(s.advisor_client_mode) + + def test_equals_form_is_accepted(self) -> None: + with _IsolatedEnv(): + ctx = _make_context(provider=_fake_first_party_provider()) + advisor_command_call("anthropic:claude-opus-4-6 --effort=high", ctx) + self.assertEqual(self._settings().advisor_effort, "high") + + def test_retune_without_model(self) -> None: + with _IsolatedEnv(): + ctx = _make_context(provider=_fake_first_party_provider()) + advisor_command_call("anthropic:claude-opus-4-6 --effort low", ctx) + res = advisor_command_call("--effort max", ctx) + self.assertIn("max", res.value) + self.assertEqual(self._settings().advisor_effort, "max") + + def test_retune_requires_a_configured_advisor(self) -> None: + with _IsolatedEnv(): + ctx = _make_context(provider=_fake_first_party_provider()) + res = advisor_command_call("--effort max", ctx) + self.assertIn("not configured", res.value) + self.assertEqual(self._settings().advisor_effort, "") + + def test_auto_clears_to_inherit_session_effort(self) -> None: + with _IsolatedEnv(): + ctx = _make_context(provider=_fake_first_party_provider()) + advisor_command_call("anthropic:claude-opus-4-6 --effort xhigh", ctx) + res = advisor_command_call("--effort auto", ctx) + self.assertIn("inherits", res.value) + self.assertEqual(self._settings().advisor_effort, "") + + def test_invalid_level_rejected_without_writing(self) -> None: + with _IsolatedEnv(): + ctx = _make_context(provider=_fake_first_party_provider()) + advisor_command_call("anthropic:claude-opus-4-6 --effort high", ctx) + res = advisor_command_call("--effort bogus", ctx) + self.assertIn("Invalid effort", res.value) + self.assertEqual(self._settings().advisor_effort, "high") + + def test_missing_value_rejected(self) -> None: + with _IsolatedEnv(): + ctx = _make_context(provider=_fake_first_party_provider()) + res = advisor_command_call("--effort", ctx) + self.assertIn("needs a level", res.value) + + def test_unset_clears_effort(self) -> None: + with _IsolatedEnv(): + ctx = _make_context(provider=_fake_first_party_provider()) + advisor_command_call("anthropic:claude-opus-4-6 --effort xhigh", ctx) + advisor_command_call("unset", ctx) + self.assertEqual(self._settings().advisor_effort, "") + + def test_unset_with_effort_flag_is_an_error_not_a_silent_drop(self) -> None: + with _IsolatedEnv(): + ctx = _make_context(provider=_fake_first_party_provider()) + advisor_command_call("anthropic:claude-opus-4-6 --effort xhigh", ctx) + res = advisor_command_call("unset --effort high", ctx) + self.assertIn("--effort", res.value) + # Still configured — the command refused rather than half-acting. + self.assertEqual(self._settings().advisor_model, "claude-opus-4-6") + + def test_changing_model_clears_a_stale_effort(self) -> None: + """An xhigh set for an Opus reviewer must not ride along to an + OpenAI-compatible one, where the xhigh clamp does not apply.""" + with _IsolatedEnv(): + ctx = _make_context(provider=_fake_first_party_provider()) + advisor_command_call("anthropic:claude-opus-4-6 --effort xhigh", ctx) + res = advisor_command_call("zai:glm-5.2", ctx) + self.assertIn("Cleared effort", res.value) + self.assertEqual(self._settings().advisor_effort, "") + + def test_reselecting_the_same_model_keeps_its_effort(self) -> None: + with _IsolatedEnv(): + ctx = _make_context(provider=_fake_first_party_provider()) + advisor_command_call("anthropic:claude-opus-4-6 --effort xhigh", ctx) + advisor_command_call("anthropic:claude-opus-4-6", ctx) + self.assertEqual(self._settings().advisor_effort, "xhigh") + + def test_status_distinguishes_explicit_from_inherited(self) -> None: + with _IsolatedEnv(): + ctx = _make_context(provider=_fake_first_party_provider()) + advisor_command_call("anthropic:claude-opus-4-6 --effort xhigh", ctx) + self.assertIn("Effort: xhigh", advisor_command_call("", ctx).value) + advisor_command_call("--effort auto", ctx) + status = advisor_command_call("", ctx).value + self.assertIn("Effort:", status) + self.assertNotIn("Effort: xhigh", status) + + if __name__ == "__main__": unittest.main() diff --git a/tests/test_advisor_effort_wiring.py b/tests/test_advisor_effort_wiring.py new file mode 100644 index 00000000..328c5d40 --- /dev/null +++ b/tests/test_advisor_effort_wiring.py @@ -0,0 +1,600 @@ +"""Advisor reasoning-effort / thinking wiring and transient-failure retry. + +The client-side advisor used to send neither a thinking config nor an +effort level on either wire, so a reviewer model chosen precisely because +it reasons harder than the worker ran with thinking off at the API's +default effort. It also had no output budget headroom for thinking, and +no retry: a single 429 — routine on a subscription bucket shared with an +interactive session — ended the consultation. + +Test surface: + * effort resolution + precedence (``advisor_effort`` > ``effort`` > omit) + * Anthropic wire: adaptive-vs-budget thinking, ``output_config.effort``, + the xhigh clamp + * OpenAI-compatible wire: ``extra_body.reasoning_effort``, deliberately + NOT clamped (the xhigh allowlist holds Anthropic model names) + * ``max_tokens`` headroom so thinking can't starve the reply + * bounded, abort-aware retry on transient errors only + * an AUTHORITY test that the advisor delegates to the main loop's model + gates rather than carrying its own copy +""" + +from __future__ import annotations + +import types +import unittest +from typing import Any +from unittest.mock import patch + +import src.config as cfg_mod +import src.providers as providers_mod +import src.settings.settings as settings_mod +import src.utils.advisor as advisor_mod +from src.query.query import build_anthropic_thinking_kwargs +from src.utils.advisor import execute_client_advisor + + +class _FakeResponse: + content = "**Gaps:** none\n**Risks:** none\n**Do next:** ship it" + usage = {"input_tokens": 11, "output_tokens": 7} + model = "fake-model" + + +class _Recorder: + """Provider double that records the kwargs it was called with.""" + + def __init__(self, *, fail_with: list[Exception] | None = None) -> None: + self.kwargs: dict[str, Any] = {} + self.messages: list[Any] = [] + self.calls = 0 + self._fail_with = list(fail_with or []) + + def __call__(self, api_key: str = "", base_url: Any = None, model: Any = None): + self.model = model + self.seen_api_key = api_key + return self + + def chat_stream_response(self, messages, **kwargs): + self.calls += 1 + self.messages = messages + self.kwargs = kwargs + if self._fail_with: + raise self._fail_with.pop(0) + return _FakeResponse() + + +def _run_advisor( + model: str, + *, + anthropic_wire: bool, + effort: str = "", + advisor_effort: str = "", + recorder: _Recorder | None = None, + abort_signal: Any = None, +) -> tuple[tuple[bool, str, dict[str, int]], _Recorder]: + rec = recorder or _Recorder() + fake_settings = types.SimpleNamespace(effort=effort, advisor_effort=advisor_effort) + with ( + patch.object(settings_mod, "get_settings", lambda: fake_settings), + patch.object(providers_mod, "get_provider_class", lambda key: rec), + patch.object(providers_mod, "is_anthropic_wire", lambda p: anthropic_wire), + patch.object( + cfg_mod, "get_provider_config", + lambda key: {"api_key": "k", "base_url": "https://example.test"}, + ), + ): + result = execute_client_advisor( + model, + [{"role": "user", "content": "hi"}], + advisor_provider="p", + abort_signal=abort_signal, + ) + return result, rec + + +class TestAdvisorEffortResolution(unittest.TestCase): + """Where the advisor's effort level comes from.""" + + def test_advisor_effort_overrides_session_effort(self) -> None: + (ok, _, _), rec = _run_advisor( + "claude-opus-5", anthropic_wire=True, effort="low", advisor_effort="max", + ) + self.assertTrue(ok) + self.assertEqual(rec.kwargs["output_config"], {"effort": "max"}) + + def test_session_effort_inherited_when_advisor_effort_unset(self) -> None: + (ok, _, _), rec = _run_advisor( + "claude-opus-5", anthropic_wire=True, effort="xhigh", + ) + self.assertTrue(ok) + self.assertEqual(rec.kwargs["output_config"], {"effort": "xhigh"}) + + def test_no_effort_anywhere_omits_the_parameter(self) -> None: + """Omit-don't-guess: the API applies its own model default.""" + (ok, _, _), rec = _run_advisor("claude-opus-5", anthropic_wire=True) + self.assertTrue(ok) + self.assertNotIn("output_config", rec.kwargs) + + def test_settings_failure_does_not_break_the_consultation(self) -> None: + def _boom(): + raise RuntimeError("settings exploded") + + rec = _Recorder() + with ( + patch.object(settings_mod, "get_settings", _boom), + patch.object(providers_mod, "get_provider_class", lambda key: rec), + patch.object(providers_mod, "is_anthropic_wire", lambda p: True), + patch.object( + cfg_mod, "get_provider_config", + lambda key: {"api_key": "k", "base_url": None}, + ), + ): + ok, _text, _usage = execute_client_advisor( + "claude-opus-5", [{"role": "user", "content": "hi"}], + advisor_provider="p", + ) + self.assertTrue(ok) + self.assertNotIn("output_config", rec.kwargs) + + +class TestAdvisorAnthropicWire(unittest.TestCase): + """Thinking config + effort on the Anthropic wire.""" + + def test_opus5_gets_adaptive_thinking_and_effort(self) -> None: + (ok, _, _), rec = _run_advisor( + "claude-opus-5", anthropic_wire=True, advisor_effort="xhigh", + ) + self.assertTrue(ok) + self.assertEqual(rec.kwargs["thinking"], {"type": "adaptive"}) + self.assertEqual(rec.kwargs["output_config"], {"effort": "xhigh"}) + + def test_non_adaptive_model_gets_budget_thinking(self) -> None: + """Adaptive is a hard 400 on these; budget is the safe direction.""" + (ok, _, _), rec = _run_advisor( + "claude-haiku-4-5-20251001", anthropic_wire=True, advisor_effort="high", + ) + self.assertTrue(ok) + thinking = rec.kwargs["thinking"] + self.assertEqual(thinking["type"], "enabled") + self.assertGreaterEqual(thinking["budget_tokens"], 1024) + self.assertLess(thinking["budget_tokens"], rec.kwargs["max_tokens"]) + # Not on the effort allowlist -> no output_config, or the API 400s. + self.assertNotIn("output_config", rec.kwargs) + + def test_xhigh_clamped_on_models_that_reject_it(self) -> None: + """sonnet-4-6 400s on xhigh; the shared resolver downgrades to high.""" + (ok, _, _), rec = _run_advisor( + "claude-sonnet-4-6", anthropic_wire=True, advisor_effort="xhigh", + ) + self.assertTrue(ok) + self.assertEqual(rec.kwargs["output_config"], {"effort": "high"}) + + def test_system_is_sent_as_blocks_not_a_string(self) -> None: + """Load-bearing on the Claude subscription (OAuth) path. + + ``_prepare_subscription_request`` prepends 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. The + endpoint accepts only the latter for premium models — wire-probed + against claude-opus-5 over subscription, 3/3 per cell — and the + rejection arrives mislabelled as ``rate_limit_error``, so a + regression here looks like a capacity problem, not a bug. + """ + (_ok, _, _), rec = _run_advisor("claude-opus-5", anthropic_wire=True) + system = rec.kwargs["system"] + self.assertIsInstance( + system, list, + msg="advisor system reverted to a string — this 429s on " + "subscription for premium models", + ) + self.assertEqual(system[0]["type"], "text") + self.assertIn("senior reviewer", system[0]["text"]) + + def test_no_reasoning_effort_body_field_on_anthropic(self) -> None: + """``extra_body.reasoning_effort`` is the OpenAI shape; on the + Anthropic wire it is a 400 (extra inputs are not permitted).""" + (_ok, _, _), rec = _run_advisor( + "claude-opus-5", anthropic_wire=True, advisor_effort="xhigh", + ) + self.assertNotIn("extra_body", rec.kwargs) + + +class TestAdvisorOpenAIWire(unittest.TestCase): + """Effort on OpenAI-compatible wires.""" + + def test_reasoning_effort_goes_in_extra_body(self) -> None: + (ok, _, _), rec = _run_advisor( + "glm-5.2", anthropic_wire=False, advisor_effort="high", + ) + self.assertTrue(ok) + self.assertEqual(rec.kwargs["extra_body"], {"reasoning_effort": "high"}) + + def test_xhigh_is_not_clamped_on_openai_wire(self) -> None: + """The xhigh allowlist holds Anthropic model NAMES and matches + nothing here — clamping on it silently downgraded every xhigh.""" + (_ok, _, _), rec = _run_advisor( + "glm-5.2", anthropic_wire=False, advisor_effort="xhigh", + ) + self.assertEqual(rec.kwargs["extra_body"], {"reasoning_effort": "xhigh"}) + + def test_no_thinking_or_output_config_on_openai_wire(self) -> None: + (_ok, _, _), rec = _run_advisor( + "glm-5.2", anthropic_wire=False, advisor_effort="xhigh", + ) + self.assertNotIn("thinking", rec.kwargs) + self.assertNotIn("output_config", rec.kwargs) + + def test_effort_omitted_when_unset(self) -> None: + (_ok, _, _), rec = _run_advisor("glm-5.2", anthropic_wire=False) + self.assertNotIn("extra_body", rec.kwargs) + + def test_effort_is_translated_into_the_providers_vocabulary(self) -> None: + """The advisor must apply ``normalize_reasoning_effort`` like the main + loop. Skipping it sent DeepSeek ``xhigh`` — a level it does not know — + so it dropped the field and applied its own default: a SILENT + downgrade on the setting people pick when a task is hard.""" + rec = _Recorder() + rec.normalize_reasoning_effort = lambda e: "max" if e == "xhigh" else e + (_ok, _, _), rec = _run_advisor( + "deepseek-v4-pro", anthropic_wire=False, + advisor_effort="xhigh", recorder=rec, + ) + self.assertEqual(rec.kwargs["extra_body"], {"reasoning_effort": "max"}) + + def test_a_bogus_normalization_is_discarded_not_trusted(self) -> None: + """Duck-typed getattr: a MagicMock-ish provider answers every + attribute with a callable, so an unguarded assignment would write a + repr into the request body.""" + rec = _Recorder() + rec.normalize_reasoning_effort = lambda e: object() + (_ok, _, _), rec = _run_advisor( + "glm-5.2", anthropic_wire=False, advisor_effort="high", recorder=rec, + ) + self.assertEqual(rec.kwargs["extra_body"], {"reasoning_effort": "high"}) + + def test_a_raising_normalizer_never_breaks_the_call(self) -> None: + def _boom(_e): + raise RuntimeError("provider exploded") + + rec = _Recorder() + rec.normalize_reasoning_effort = _boom + (ok, _, _), rec = _run_advisor( + "glm-5.2", anthropic_wire=False, advisor_effort="high", recorder=rec, + ) + self.assertTrue(ok) + self.assertEqual(rec.kwargs["extra_body"], {"reasoning_effort": "high"}) + + +class TestAdvisorOutputBudget(unittest.TestCase): + """Thinking tokens come out of the same max_tokens budget as the reply.""" + + def test_budget_never_below_the_historical_floor(self) -> None: + for model, wire in ( + ("claude-opus-5", True), + ("glm-5.2", False), + ("some-unknown-model", False), + ): + with self.subTest(model=model): + (_ok, _, _), rec = _run_advisor(model, anthropic_wire=wire) + self.assertGreaterEqual(rec.kwargs["max_tokens"], 4096) + + def test_thinking_capable_model_gets_headroom_beyond_the_floor(self) -> None: + """At a flat 4096 a high-effort reviewer can spend the whole budget + thinking and return stop_reason=max_tokens with no text at all.""" + (_ok, _, _), rec = _run_advisor( + "claude-opus-5", anthropic_wire=True, advisor_effort="xhigh", + ) + self.assertGreater(rec.kwargs["max_tokens"], 4096) + + def test_anthropic_takes_the_model_table_value_whole(self) -> None: + """On this wire max_output_tokens IS the request's max_tokens by + design — same value the main loop sends. + + The ceiling is PATCHED DOWN for this assertion. At its real value no + Anthropic model can distinguish clamped from unclamped — 32768 is + itself the largest max_output_tokens in the whole Anthropic family — + so without this patch the test passes even if the clamp is applied + to BOTH wires, leaving the asymmetry the fix rests on unpinned. + """ + from src.models.context import resolve_max_output_tokens + + with patch.object(advisor_mod, "_ADVISOR_MAX_OPENAI_WIRE_TOKENS", 8192): + (_ok, _, _), rec = _run_advisor("claude-opus-5", anthropic_wire=True) + self.assertEqual( + rec.kwargs["max_tokens"], + resolve_max_output_tokens(None, "claude-opus-5"), + msg="the OpenAI-wire ceiling leaked onto the Anthropic wire", + ) + + def test_base_url_reaches_the_budget_resolver(self) -> None: + """Per-endpoint model-limit overrides are keyed on base_url; the main + loop passes it, so the advisor must too.""" + seen: dict[str, Any] = {} + real = None + import src.models.context as ctx_mod + + real = ctx_mod.resolve_max_output_tokens + + def spy(override, model_id, *, base_url=None): + seen["base_url"] = base_url + return real(override, model_id, base_url=base_url) + + with patch.object(ctx_mod, "resolve_max_output_tokens", spy): + _run_advisor("claude-opus-5", anthropic_wire=True) + self.assertEqual(seen.get("base_url"), "https://example.test") + + def test_openai_wire_is_clamped_below_the_advisory_table_value(self) -> None: + """The table value is an auto-compact reservation on this wire, not a + legal request cap — deepseek's row is 384000, luna's 128000, and + openai_compatible DOES forward max_tokens to completions.create(). + + DeepSeek accepts 384000 (probed), so this guards an untested number + per provider across ~30 of them rather than a known outage — but a + rejection would be a non-retryable 400 that degrades silently.""" + for model in ("deepseek-v4-pro", "gpt-5.6-luna"): + with self.subTest(model=model): + (_ok, _, _), rec = _run_advisor(model, anthropic_wire=False) + self.assertEqual( + rec.kwargs["max_tokens"], + advisor_mod._ADVISOR_MAX_OPENAI_WIRE_TOKENS, + ) + + def test_the_ceiling_still_equals_the_anthropic_family_maximum(self) -> None: + """The ceiling is documented as a RULE — "the OpenAI wire never gets a + larger budget than the most generous Anthropic model" — so pin it, + the way VALID_THINKING_EFFORT_LEVELS pins its ladder. + + This is load-bearing because the anchor is a SINGLE legacy row + (claude-opus-4-20250514 at 32768; 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 else noticing. + """ + from src.models.configs import MODEL_CONFIGS + + family_max = max( + cfg.max_output_tokens + for name, cfg in MODEL_CONFIGS.items() + if "claude" in name.lower() + ) + self.assertEqual( + advisor_mod._ADVISOR_MAX_OPENAI_WIRE_TOKENS, family_max, + msg=( + "the Anthropic family maximum moved — either update " + "_ADVISOR_MAX_OPENAI_WIRE_TOKENS to match, or rewrite the " + "constant's comment to stop claiming it equals that maximum" + ), + ) + + def test_clamp_never_raises_a_smaller_model_budget(self) -> None: + """The clamp is a ceiling, not a floor — a model whose table value is + already modest keeps it.""" + (_ok, _, _), rec = _run_advisor("glm-5.2", anthropic_wire=False) + self.assertLess( + rec.kwargs["max_tokens"], + advisor_mod._ADVISOR_MAX_OPENAI_WIRE_TOKENS, + ) + self.assertGreaterEqual(rec.kwargs["max_tokens"], 4096) + + +class TestAdvisorRetry(unittest.TestCase): + """Bounded retry on transient failures only.""" + + def _rate_limit(self) -> Exception: + exc = Exception("Error code: 429 - rate_limit_error") + setattr(exc, "status_code", 429) + return exc + + def test_transient_failure_is_retried_and_can_succeed(self) -> None: + rec = _Recorder(fail_with=[self._rate_limit()]) + with patch.object(advisor_mod, "_advisor_sleep", lambda d, s: True): + (ok, text, usage), rec = _run_advisor( + "claude-opus-5", anthropic_wire=True, recorder=rec, + ) + self.assertTrue(ok, msg=text) + self.assertEqual(rec.calls, 2) + self.assertEqual(usage["input_tokens"], 11) + + def test_retries_are_bounded(self) -> None: + rec = _Recorder(fail_with=[self._rate_limit() for _ in range(10)]) + with patch.object(advisor_mod, "_advisor_sleep", lambda d, s: True): + (ok, text, usage), rec = _run_advisor( + "claude-opus-5", anthropic_wire=True, recorder=rec, + ) + self.assertFalse(ok) + self.assertEqual(rec.calls, advisor_mod._ADVISOR_MAX_ATTEMPTS) + self.assertEqual(usage, {"input_tokens": 0, "output_tokens": 0}) + + def test_non_retryable_error_fails_on_first_attempt(self) -> None: + """A bad request must not be re-issued three times.""" + exc = Exception("Error code: 400 - invalid_request_error: bad model") + setattr(exc, "status_code", 400) + rec = _Recorder(fail_with=[exc, exc, exc]) + with patch.object(advisor_mod, "_advisor_sleep", lambda d, s: True): + (ok, _text, _usage), rec = _run_advisor( + "claude-opus-5", anthropic_wire=True, recorder=rec, + ) + self.assertFalse(ok) + self.assertEqual(rec.calls, 1) + + def test_abort_during_backoff_stops_retrying(self) -> None: + """ESC must not be stuck behind the advisor's backoff.""" + rec = _Recorder(fail_with=[self._rate_limit() for _ in range(5)]) + with patch.object(advisor_mod, "_advisor_sleep", lambda d, s: False): + (ok, _text, _usage), rec = _run_advisor( + "claude-opus-5", anthropic_wire=True, recorder=rec, + ) + self.assertFalse(ok) + self.assertEqual(rec.calls, 1) + + def test_advisor_sleep_returns_false_once_aborted(self) -> None: + signal = types.SimpleNamespace(aborted=True) + self.assertFalse(advisor_mod._advisor_sleep(5.0, signal)) + + def test_advisor_sleep_completes_without_abort(self) -> None: + self.assertTrue(advisor_mod._advisor_sleep(0.01, None)) + + +class TestSharedGateAuthority(unittest.TestCase): + """The advisor must DELEGATE to the main loop's gates, not copy them. + + Both call sites read the same model allowlists; a future edit that + inlines a private copy into advisor.py would keep every assertion above + green while silently reintroducing drift. Patch the gate at its single + source and require the advisor's wire output to follow. + """ + + def test_effort_gate_is_the_main_loops(self) -> None: + with patch("src.query.query._model_supports_effort", lambda m: False): + (_ok, _, _), rec = _run_advisor( + "claude-opus-5", anthropic_wire=True, advisor_effort="xhigh", + ) + self.assertNotIn( + "output_config", rec.kwargs, + msg="advisor ignored the main loop's effort gate — it has its " + "own copy of the allowlist and will drift", + ) + + def test_adaptive_thinking_gate_is_the_main_loops(self) -> None: + with patch("src.query.query._model_supports_adaptive_thinking", lambda m: False): + (_ok, _, _), rec = _run_advisor( + "claude-opus-5", anthropic_wire=True, advisor_effort="xhigh", + ) + self.assertEqual( + rec.kwargs["thinking"]["type"], "enabled", + msg="advisor ignored the main loop's adaptive-thinking gate", + ) + + +class TestAdvisorCredentialResolution(unittest.TestCase): + """The advisor must resolve its provider key the way every other call + site does — configured value first, then the environment.""" + + def _run_with_config(self, provider_cfg: dict, env: dict) -> _Recorder: + rec = _Recorder() + fake_settings = types.SimpleNamespace(effort="", advisor_effort="") + with ( + patch.object(settings_mod, "get_settings", lambda: fake_settings), + patch.object(providers_mod, "get_provider_class", lambda key: rec), + patch.object(providers_mod, "is_anthropic_wire", lambda p: False), + patch.object(cfg_mod, "get_provider_config", lambda key: provider_cfg), + patch.dict("os.environ", env, clear=False), + ): + execute_client_advisor( + "glm-5.2", [{"role": "user", "content": "hi"}], + advisor_provider="zai", + ) + return rec + + def test_key_falls_back_to_the_environment(self) -> None: + """An advisor provider whose key lives in an env var — how eval + containers and most shells supply credentials — must authenticate. + Reading providers..api_key directly returned "" and the call + died on 'Missing credentials'.""" + rec = self._run_with_config( + {"api_key": "", "base_url": None}, {"ZAI_API_KEY": "from-env"}, + ) + self.assertEqual(rec.model, "glm-5.2") + # No getattr default — a missing attribute must fail loudly rather + # than pass by falling back to the value under test. + self.assertEqual( + rec.seen_api_key, "from-env", + msg="advisor ignored the environment for its provider key", + ) + + def test_configured_key_outranks_the_environment(self) -> None: + """Precedence must match resolve_api_key: config wins. This is what + keeps an exported ANTHROPIC_API_KEY from silently outranking a + subscription OAuth login in the opposite direction.""" + from src.providers import resolve_api_key + + self.assertEqual( + resolve_api_key("zai", {"api_key": "from-config"}), "from-config", + ) + + def test_empty_key_survives_for_the_oauth_fallback(self) -> None: + """The Anthropic subscription path REQUIRES an empty api_key so the + provider falls through to OAuth — resolution must not invent one.""" + from src.providers import resolve_api_key + + with patch.dict("os.environ", {}, clear=True): + self.assertEqual( + resolve_api_key("anthropic", {"api_key": "", "base_url": None}), "", + ) + + +class TestSubscriptionSystemShape(unittest.TestCase): + """The mechanism behind the block-list requirement, pinned at the + provider seam rather than asserted only at the advisor's call site.""" + + def _provider(self): + from src.providers.anthropic_provider import AnthropicProvider + + p = AnthropicProvider(api_key="k", model="claude-opus-5") + p._subscription_token = "fake-oauth-token" # engage the OAuth path + return p + + def test_block_list_keeps_the_preamble_as_its_own_block(self) -> None: + p = self._provider() + _msgs, _tools, system = p._prepare_subscription_request( + [{"role": "user", "content": "hi"}], + None, + [{"type": "text", "text": "You are a senior reviewer."}], + ) + self.assertIsInstance(system, list) + self.assertIn("Claude Code", system[0]["text"]) + # The preamble must stand alone — a premium model rejects the + # request when the advisor text is fused into that same block. + self.assertNotIn("senior reviewer", system[0]["text"]) + self.assertIn("senior reviewer", system[1]["text"]) + + def test_string_system_fuses_the_preamble_with_the_prompt(self) -> None: + """Documents the shape that gets rejected, so the reason the + advisor sends blocks stays visible to the next reader.""" + p = self._provider() + _msgs, _tools, system = p._prepare_subscription_request( + [{"role": "user", "content": "hi"}], None, "You are a senior reviewer.", + ) + self.assertIsInstance(system, str) + self.assertIn("Claude Code", system) + self.assertIn("senior reviewer", system) + + def test_advisor_shape_survives_the_subscription_rewrite(self) -> None: + """End-to-end on the shape: what execute_client_advisor builds must + still be a preamble-first block list after the provider rewrite.""" + (_ok, _, _), rec = _run_advisor("claude-opus-5", anthropic_wire=True) + p = self._provider() + _m, _t, system = p._prepare_subscription_request( + [{"role": "user", "content": "hi"}], None, rec.kwargs["system"], + ) + self.assertIsInstance(system, list) + self.assertIn("Claude Code", system[0]["text"]) + self.assertNotIn("senior reviewer", system[0]["text"]) + + +class TestBuildAnthropicThinkingKwargs(unittest.TestCase): + """The extracted builder itself (shared by both call sites).""" + + def test_returns_empty_for_non_thinking_model(self) -> None: + self.assertEqual(build_anthropic_thinking_kwargs("gpt-4o"), {}) + + def test_force_thinking_enables_an_unrecognised_alias(self) -> None: + out = build_anthropic_thinking_kwargs( + "some-unknown-alias", max_tokens=8192, force_thinking=True, + ) + self.assertEqual(out["thinking"]["type"], "enabled") + + def test_budget_omitted_when_max_tokens_too_small(self) -> None: + """No budget in [1024, max_tokens) fits — omit rather than 400.""" + out = build_anthropic_thinking_kwargs( + "claude-haiku-4-5-20251001", max_tokens=512, + ) + self.assertNotIn("thinking", out) + + def test_adaptive_model_ignores_max_tokens(self) -> None: + out = build_anthropic_thinking_kwargs("claude-opus-5", max_tokens=0) + self.assertEqual(out["thinking"], {"type": "adaptive"}) + + +if __name__ == "__main__": + unittest.main() diff --git a/tests/test_harbor_adapter_advisor.py b/tests/test_harbor_adapter_advisor.py new file mode 100644 index 00000000..dba36586 --- /dev/null +++ b/tests/test_harbor_adapter_advisor.py @@ -0,0 +1,334 @@ +"""Advisor wiring in the harbor eval adapter. + +Runs ONLY in the dedicated "Harbor adapter (3.13)" CI job, which installs +harbor explicitly. ``eval/harbor/clawcodex_agent.py`` imports ``harbor`` at +module scope, so under the main ``test (3.11)`` job the ``importorskip`` +below fires and every assertion here skips silently. A file left out of that +job's file list therefore never runs at all — add new ``tests/test_harbor_*`` +files to it. + +The failure mode being pinned here is a QUIET one. A consultation that +cannot authenticate degrades gracefully: the worker carries on and the task +can still score 1.0, so a run whose advisor never once answered looks +identical to a healthy one in the results table. It was found by grepping a +container trajectory, not by a red test — hence these. +""" + +from __future__ import annotations + +import asyncio +import json +import sys +from pathlib import Path + +import pytest + +pytest.importorskip("harbor", reason="harbor is an eval-only tool dependency") + +_ADAPTER_DIR = Path(__file__).resolve().parents[1] / "eval" / "harbor" +if str(_ADAPTER_DIR) not in sys.path: + sys.path.insert(0, str(_ADAPTER_DIR)) + +from clawcodex_agent import Clawcodex # noqa: E402 + + +def _agent( + *, + model_provider: str = "openai", + advisor: str | None = None, + advisor_effort: str | None = None, + subscription: bool = False, + effort: str | None = "xhigh", +) -> Clawcodex: + """An adapter instance carrying only the fields these paths read.""" + agent = Clawcodex.__new__(Clawcodex) + agent._subscription = subscription + agent._advisor = advisor + agent._advisor_effort = advisor_effort + agent._parsed_model_provider = model_provider + agent._forward_keys = False + agent._fusion = None + agent._resolved_flags = {"effort": effort} if effort else {} + agent._extra_env = {} + agent._get_env = lambda key: f"{key}-VALUE" + return agent + + +def _seeded_config(agent: Clawcodex) -> dict: + captured: dict = {} + + async def fake_exec(environment, command=None, env=None): + captured.update(json.loads(env["CLAWCODEX_SEED_CONFIG"])) + + agent.exec_as_agent = fake_exec + agent._host_env_keys = lambda: {} + agent._fusion_record = lambda: None + asyncio.run(agent._seed_container_settings(None)) + return captured + + +# -------------------------------------------------------------------------- +# Credential forwarding +# -------------------------------------------------------------------------- + +def test_advisor_provider_key_is_forwarded() -> None: + """The advisor calls its OWN provider. Forwarding only the main model's + key left it with no credentials — observed live as two consultations + dying on "Missing credentials" while the task still scored 1.0.""" + env = _agent(model_provider="openai", advisor="zai:glm-5.2")._build_env() + assert "OPENAI_API_KEY" in env + assert "ZAI_API_KEY" in env + + +def test_anthropic_key_withheld_when_advisor_uses_the_subscription() -> None: + """OAuth must stay the only route to the subscription: inside clawcodex + an API key outranks it and would silently bill the API instead.""" + env = _agent( + model_provider="openai", + advisor="anthropic:claude-opus-5", + subscription=True, + )._build_env() + assert "ANTHROPIC_API_KEY" not in env + assert "OPENAI_API_KEY" in env, "the worker still needs its own key" + + +def test_main_provider_key_survives_subscription_mode() -> None: + """Subscription used to suppress ALL provider keys, which is right only + when the subscription backs the MAIN loop. Backing just the advisor left + the worker with no credentials at all.""" + env = _agent( + model_provider="openai", + advisor="anthropic:claude-opus-5", + subscription=True, + )._build_env() + assert env.get("OPENAI_API_KEY") + + +def test_unmapped_main_provider_still_withholds_the_anthropic_key() -> None: + """The expensive branch. + + For a MAPPED provider the strip is a no-op (``openai`` maps to only + OPENAI_API_KEY), so it looks redundant. It bites when the main provider + is unmapped and falls back to _ALL_PROVIDER_ENV_VARS — which is most of + the registry (groq, fireworks, cerebras, together, …). Without the + strip, ANTHROPIC_API_KEY rides into the container, where an API key + silently OUTRANKS OAuth inside clawcodex and bills the API instead of + the subscription. That is the exact outcome subscription mode exists to + prevent, and no mapped-provider test can catch it. + """ + env = _agent( + model_provider="groq", + advisor="anthropic:claude-opus-5", + subscription=True, + )._build_env() + assert "ANTHROPIC_API_KEY" not in env + # The fallback DID fire — other mapped keys came through, so the absence + # above is the strip doing its job, not an empty forward set. + assert env.get("OPENAI_API_KEY"), "expected the all-providers fallback" + + +def test_unmapped_provider_key_is_not_forwarded_at_all() -> None: + """PRE-EXISTING limitation, pinned so it is not mistaken for advisor + breakage. `_ALL_PROVIDER_ENV_VARS` is the union of the seven MAPPED + vendors, so an unmapped provider's own key (GROQ_API_KEY, TOGETHER_API_KEY, + …) is never forwarded — the fallback is 'every key we know about', not + 'every key that exists'. A run using one of those as the worker needs its + key passed explicitly via --ae, whether or not an advisor is configured. + """ + env = _agent(model_provider="groq", advisor=None)._build_env() + assert "GROQ_API_KEY" not in env + + +def test_anthropic_main_loop_subscription_forwards_nothing() -> None: + """The original behaviour, unchanged.""" + env = _agent(model_provider="anthropic", subscription=True)._build_env() + assert not [k for k in env if k.endswith("_API_KEY")] + + +def test_no_advisor_leaves_forwarding_untouched() -> None: + env = _agent(model_provider="openai", advisor=None)._build_env() + assert sorted(k for k in env if k.endswith("_API_KEY")) == ["OPENAI_API_KEY"] + + +# -------------------------------------------------------------------------- +# Settings seeding +# -------------------------------------------------------------------------- + +def test_advisor_settings_are_seeded() -> None: + config = _seeded_config( + _agent(advisor="anthropic:claude-opus-5", advisor_effort="xhigh") + ) + settings = config["settings"] + assert settings["advisor_enabled"] is True, "master switch defaults off" + assert settings["advisor_provider"] == "anthropic" + assert settings["advisor_model"] == "claude-opus-5" + assert settings["advisor_effort"] == "xhigh" + # Client-side pinned so the advisor behaves identically regardless of + # which worker model a run is comparing. + assert settings["advisor_client_mode"] is True + + +def test_worker_and_advisor_efforts_are_independent() -> None: + settings = _seeded_config( + _agent(effort="low", advisor="anthropic:claude-opus-5", advisor_effort="max") + )["settings"] + assert settings["effort"] == "low" + assert settings["advisor_effort"] == "max" + + +def test_advisor_effort_omitted_means_inherit() -> None: + settings = _seeded_config( + _agent(effort="xhigh", advisor="anthropic:claude-opus-5") + )["settings"] + assert "advisor_effort" not in settings + + +def test_no_advisor_seeds_no_advisor_keys() -> None: + settings = _seeded_config(_agent(advisor=None))["settings"] + assert not [k for k in settings if k.startswith("advisor")] + + +# -------------------------------------------------------------------------- +# Validation +# -------------------------------------------------------------------------- + +def _construct(**kwargs): + """Run the adapter's own __init__ validation without harbor's base + machinery — the base __init__ needs a real environment we don't have.""" + from unittest.mock import patch + + base = Clawcodex.__mro__[1] + with patch.object(base, "__init__", lambda self, *a, **k: None): + agent = Clawcodex(Path("/tmp"), **kwargs) + return agent + + +@pytest.mark.parametrize("bad", ["anthropic", "claude-opus-5"]) +def test_advisor_without_a_colon_is_rejected(bad: str) -> None: + with pytest.raises(ValueError, match=":"): + _construct(advisor=bad) + + +def test_valid_advisor_is_accepted_and_parsed() -> None: + agent = _construct(advisor="anthropic:claude-opus-5") + assert agent._advisor_provider() == "anthropic" + + +def test_model_may_contain_further_colons_and_slashes() -> None: + """Only the FIRST colon separates; model ids keep their own punctuation.""" + agent = _construct(advisor="openrouter:anthropic/claude-opus-4.1") + assert agent._advisor_provider() == "openrouter" + assert agent._advisor.split(":", 1)[1] == "anthropic/claude-opus-4.1" + + +@pytest.mark.parametrize("bad", ["bogus", "XHIGH ", "10"]) +def test_invalid_advisor_effort_is_rejected(bad: str) -> None: + with pytest.raises(ValueError, match="advisor_effort"): + _construct(advisor="anthropic:claude-opus-5", advisor_effort=bad) + + +def test_advisor_effort_without_an_advisor_is_rejected() -> None: + """An effort level with no reviewer configured is silently inert — + exactly the class of quiet no-op this adapter has been bitten by.""" + with pytest.raises(ValueError, match="requires 'advisor'"): + _construct(advisor_effort="xhigh") + + +def test_subscription_requires_anthropic_in_some_role() -> None: + """Rejected when nothing anthropic is present...""" + agent = _agent(model_provider="openai", advisor=None, subscription=True) + with pytest.raises(RuntimeError, match="requires anthropic in some role"): + asyncio.run(agent._inject_subscription_credentials(None)) + + +def test_subscription_accepted_for_an_anthropic_advisor() -> None: + """...and accepted when anthropic is the ADVISOR rather than the main + model — the pairing that was previously inexpressible. + + Getting PAST the role gate is the whole assertion, so the credential + fetch is stubbed to a sentinel. An earlier version of this test relied + on the host having no Anthropic login and asserting the resulting + RuntimeError: that made it pass or fail depending on whether whoever ran + it happened to be logged in, and it broke the moment a real + `clawcodex login` landed on this machine. + """ + from unittest.mock import patch + + import clawcodex_agent as _mod + + sentinel = RuntimeError("reached the credential fetch") + + def _boom(): + raise sentinel + + agent = _agent( + model_provider="openai", + advisor="anthropic:claude-opus-5", + subscription=True, + ) + with patch.object(_mod, "fresh_subscription_credentials", _boom): + with pytest.raises(RuntimeError) as excinfo: + asyncio.run(agent._inject_subscription_credentials(None)) + assert excinfo.value is sentinel, ( + "the role gate rejected an anthropic ADVISOR: " + f"{excinfo.value}" + ) + + +def test_subscription_strips_anthropic_key_from_the_seeded_env_block() -> None: + """The config ``env`` block is a SECOND route to a credential. + + ``get_secret`` reads the process env and THEN this block, so a stored + ANTHROPIC_API_KEY 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. Withholding it from the exec env is not + enough on its own. + """ + import json as _json + import tempfile + from unittest.mock import patch + + with tempfile.TemporaryDirectory() as home: + cfg = Path(home) / ".clawcodex" + cfg.mkdir() + (cfg / "config.json").write_text(_json.dumps({ + "env": {"ANTHROPIC_API_KEY": "sk-leak", "TAVILY_API_KEY": "tav"} + })) + with patch.object(Path, "home", staticmethod(lambda: Path(home))): + agent = _agent( + model_provider="openai", + advisor="anthropic:claude-opus-5", + subscription=True, + ) + agent._forward_keys = True + keys = agent._host_env_keys() + assert "ANTHROPIC_API_KEY" not in keys + assert keys.get("TAVILY_API_KEY") == "tav", "unrelated keys must survive" + + +def test_non_subscription_keeps_the_whole_env_block() -> None: + """The strip is scoped to subscription runs — an API-key run legitimately + wants its stored Anthropic key.""" + import json as _json + import tempfile + from unittest.mock import patch + + with tempfile.TemporaryDirectory() as home: + cfg = Path(home) / ".clawcodex" + cfg.mkdir() + (cfg / "config.json").write_text( + _json.dumps({"env": {"ANTHROPIC_API_KEY": "sk-real"}}) + ) + with patch.object(Path, "home", staticmethod(lambda: Path(home))): + agent = _agent(model_provider="anthropic", subscription=False) + agent._forward_keys = True + keys = agent._host_env_keys() + assert keys.get("ANTHROPIC_API_KEY") == "sk-real" + + +@pytest.mark.parametrize("bad", ["anthropic:", ":claude-opus-5", ":", "anthropic: "]) +def test_half_empty_advisor_is_rejected(bad: str) -> None: + """A bare colon check let these through, each seeding a half-configured + advisor that is silently inert at run time.""" + with pytest.raises(ValueError, match="both halves non-empty"): + _construct(advisor=bad) diff --git a/tests/test_harbor_adapter_fusion.py b/tests/test_harbor_adapter_fusion.py index 8e5c4ded..3fed0d4a 100644 --- a/tests/test_harbor_adapter_fusion.py +++ b/tests/test_harbor_adapter_fusion.py @@ -1,10 +1,10 @@ """Fusion-model wiring in the harbor eval adapter. -SKIPPED IN CI. ``eval/harbor/clawcodex_agent.py`` imports ``harbor`` at -module scope, and harbor is not a dev dependency — it is installed as a -separate uv tool by the people who run evals. So these assertions run for -them and nowhere else, which is worth knowing before treating a green CI as -coverage of this file. The adapter had no tests at all before this. +Runs ONLY in the dedicated "Harbor adapter (3.13)" CI job, which installs +harbor explicitly. ``eval/harbor/clawcodex_agent.py`` imports ``harbor`` at +module scope, so under the main ``test (3.11)`` job the ``importorskip`` +below fires and every assertion here skips silently. A file left out of that +job's file list therefore never runs at all. """ from __future__ import annotations diff --git a/ui-tui/src/gatewayClient.ts b/ui-tui/src/gatewayClient.ts index 153ef6ef..425e33f0 100644 --- a/ui-tui/src/gatewayClient.ts +++ b/ui-tui/src/gatewayClient.ts @@ -540,7 +540,7 @@ const SLASHES: ReadonlyArray<{ desc: string; hint?: string; name: string }> = [ { desc: 'Switch the provider', hint: '[]', name: '/provider' }, { desc: 'Configure the advisor reviewer model (consulted mid-task by the worker)', - hint: '[: [--client] | --no-client | off|unset]', + hint: '[: [--client] [--effort ] | --effort | --no-client | off|unset]', name: '/advisor' }, {