mirror of
https://github.com/usestrix/strix.git
synced 2026-10-21 05:13:42 +00:00
fix(bedrock): give Claude an explicit, clamped output ceiling
make_model_settings never set max_tokens. On Bedrock Converse an
adaptive-thinking Claude model (opus-4-6/4-8, sonnet-4-6, and future tiers)
maps reasoning_effort to {'type': 'adaptive'} with no fixed budget_tokens, so
litellm's update_optional_params_with_thinking_tokens fallback (maxTokens =
budget_tokens + DEFAULT_MAX_TOKENS) can't fire and NO maxTokens reaches
Bedrock. Converse then applies a low internal default that "high" thinking
exhausts, truncating a long tool call mid-JSON (arguments cut one brace short).
That malformed assistant message poisons the next turn: litellm's
openai->bedrock converter does json.loads(arguments) and raises
APIConnectionError ("Unable to convert openai tool calls ... Expecting ','
delimiter"), killing the scan.
Fix: resolve_max_tokens(model_name, configured), threaded via STRIX_MAX_TOKENS
(default None) -> make_model_settings -> ModelSettings.max_tokens -> RunConfig.
Scoped deliberately:
- Gated on the CLAUDE FAMILY, not a version/tier list. The adaptive-thinking
bug spans opus-4-6/4-8 + sonnet-4-6 today and will grow, while opus-4-1 is
NOT adaptive -- tier is the wrong axis, "claude in name" is stable.
- Non-Claude models (gpt/ollama/gemini/...) are left untouched: configured
value passes through, None stays None (provider default). A local user on a
small-context model is never forced past its limit.
- The chosen ceiling is CLAMPED to the model's known max_output_tokens, so it
can never exceed what the provider accepts (older Claude tops out at
4096-8192). If a Claude model's ceiling is unknown (absent from litellm's
cost map) and the user set nothing, fall back to None rather than guess.
Verified end-to-end on Bedrock opus-4-8 (thinking high): a SQLi/cmd-inj/
weak-hash fixture that previously crashed on the 3rd/longest finding now
reports all 3.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This commit is contained in:
parent
f967e6017b
commit
d235aecc49
4 changed files with 179 additions and 1 deletions
|
|
@ -40,6 +40,15 @@ class LlmSettings(BaseSettings):
|
|||
default=False,
|
||||
alias="STRIX_FORCE_REQUIRED_TOOL_CHOICE",
|
||||
)
|
||||
# Explicit output-token ceiling. Leave unset (None) to let make_model_settings
|
||||
# decide: Anthropic/Claude models get a sensible default (they need one on
|
||||
# Bedrock Converse — adaptive-thinking models otherwise send no maxTokens and
|
||||
# truncate long tool calls), every other provider keeps its own default so
|
||||
# local users on smaller-ceiling models (gpt/ollama/gemini) aren't forced past
|
||||
# their limit. Any value set here is clamped to the model's known ceiling.
|
||||
# Must be positive: 0 or negative would yield empty output or a provider 400,
|
||||
# so reject it at load time rather than pass it through to a request.
|
||||
max_tokens: int | None = Field(default=None, gt=0, alias="STRIX_MAX_TOKENS")
|
||||
timeout: int = Field(default=300, alias="LLM_TIMEOUT")
|
||||
|
||||
|
||||
|
|
|
|||
|
|
@ -122,18 +122,103 @@ def build_scope_context(scan_config: dict[str, Any]) -> dict[str, Any]:
|
|||
}
|
||||
|
||||
|
||||
# Output ceiling for Claude models when the user hasn't set one. Adaptive-thinking
|
||||
# Claude on Bedrock Converse otherwise sends no maxTokens and truncates long tool
|
||||
# calls mid-JSON (the arguments blob is cut a brace short → next-turn history replay
|
||||
# 400s in litellm's openai→bedrock converter). Every current Claude tier accepts at
|
||||
# least this much output; the value is clamped to the model's own ceiling below, so
|
||||
# it is safe even on older Claude with a smaller limit.
|
||||
_CLAUDE_DEFAULT_MAX_TOKENS = 32000
|
||||
|
||||
|
||||
def resolve_max_tokens(model_name: str, configured: int | None) -> int | None:
|
||||
"""Decide the output ceiling for a model.
|
||||
|
||||
- Non-Claude models: return ``configured`` untouched (``None`` → provider
|
||||
default). We do not impose a ceiling on gpt/ollama/gemini/etc. — a local
|
||||
user on a small-context model must not be forced past its limit.
|
||||
- Claude models: use ``configured`` or a family default, then clamp to the
|
||||
model's known output ceiling so the value can never exceed what the
|
||||
provider accepts (older Claude tops out at 4096-8192).
|
||||
|
||||
Gated on the Claude family rather than a version/tier list: the bug this
|
||||
guards (adaptive-thinking truncation) spans opus-4-6/4-8 and sonnet-4-6 today
|
||||
and will grow, while opus-4-1 is not adaptive — so tier is the wrong axis.
|
||||
"""
|
||||
if not _is_claude_model(model_name):
|
||||
return configured
|
||||
|
||||
ceiling = _model_output_ceiling(model_name)
|
||||
if configured is not None:
|
||||
# Respect an explicit user value; clamp only if we know the ceiling.
|
||||
return min(configured, ceiling) if ceiling else configured
|
||||
# No explicit value: only inject the family default when we know it fits the
|
||||
# model. If the ceiling is unknown (model missing from litellm's cost map),
|
||||
# fall back to None (provider default) rather than guess a value that could
|
||||
# exceed the true limit — the adaptive models that need the fix all have a
|
||||
# known ceiling, so nothing is lost.
|
||||
if ceiling is None:
|
||||
return None
|
||||
return min(_CLAUDE_DEFAULT_MAX_TOKENS, ceiling)
|
||||
|
||||
|
||||
def _is_claude_model(model_name: str) -> bool:
|
||||
return "claude" in model_name.strip().lower()
|
||||
|
||||
|
||||
def _model_output_ceiling(model_name: str) -> int | None:
|
||||
"""Look up a model's max output tokens in litellm's cost map.
|
||||
|
||||
litellm keys the same model under several names (``global.anthropic.claude-
|
||||
opus-4-8``, ``anthropic.claude-opus-4-8``, ``claude-opus-4-8``), and not every
|
||||
region/provider-prefixed variant is present for every model. Try the name as
|
||||
given, then progressively strip the SDK route prefix, an ``owner/`` segment,
|
||||
and leading dotted segments (region like ``global.``/``us.``, then provider
|
||||
like ``anthropic.``) so a prefixed name still resolves to the bare key.
|
||||
"""
|
||||
import litellm
|
||||
|
||||
name = model_name.strip().lower()
|
||||
for prefix in ("litellm/", "any-llm/", "openai/", "bedrock/"):
|
||||
if name.startswith(prefix):
|
||||
name = name[len(prefix) :]
|
||||
break
|
||||
|
||||
candidates = [name]
|
||||
if "/" in name:
|
||||
candidates.append(name.rsplit("/", 1)[1])
|
||||
# Strip leading dotted segments one at a time: global.anthropic.claude-x
|
||||
# -> anthropic.claude-x -> claude-x. Each intermediate form is a real key
|
||||
# for some models, so try them all.
|
||||
for cand in list(candidates):
|
||||
rest = cand
|
||||
while "." in rest:
|
||||
rest = rest.split(".", 1)[1]
|
||||
candidates.append(rest)
|
||||
|
||||
for cand in candidates:
|
||||
entry = litellm.model_cost.get(cand)
|
||||
if entry:
|
||||
ceiling = entry.get("max_output_tokens") or entry.get("max_tokens")
|
||||
if isinstance(ceiling, int):
|
||||
return ceiling
|
||||
return None
|
||||
|
||||
|
||||
def make_model_settings(
|
||||
reasoning_effort: ReasoningEffort | None,
|
||||
*,
|
||||
model_name: str,
|
||||
force_required_tool_choice: bool = False,
|
||||
request_timeout: float | None = None,
|
||||
max_tokens: int | None = None,
|
||||
) -> ModelSettings:
|
||||
model_settings = ModelSettings(
|
||||
parallel_tool_calls=False,
|
||||
retry=DEFAULT_MODEL_RETRY,
|
||||
include_usage=True,
|
||||
extra_args=request_timeout_extra_args(request_timeout),
|
||||
max_tokens=resolve_max_tokens(model_name, max_tokens),
|
||||
)
|
||||
if (
|
||||
reasoning_effort is not None
|
||||
|
|
|
|||
|
|
@ -216,6 +216,7 @@ async def run_strix_scan(
|
|||
model_name=resolved_model,
|
||||
force_required_tool_choice=settings.llm.force_required_tool_choice,
|
||||
request_timeout=settings.llm.timeout,
|
||||
max_tokens=settings.llm.max_tokens,
|
||||
)
|
||||
run_config = RunConfig(
|
||||
model=resolved_model,
|
||||
|
|
|
|||
|
|
@ -7,7 +7,12 @@ from typing import Any
|
|||
|
||||
import pytest
|
||||
|
||||
from strix.core.inputs import build_root_task, child_initial_input, make_model_settings
|
||||
from strix.core.inputs import (
|
||||
build_root_task,
|
||||
child_initial_input,
|
||||
make_model_settings,
|
||||
resolve_max_tokens,
|
||||
)
|
||||
|
||||
|
||||
def _child_kwargs(parent_history: list[Any]) -> dict[str, Any]:
|
||||
|
|
@ -185,3 +190,81 @@ def test_make_model_settings_timeout_survives_reasoning_resolve() -> None:
|
|||
|
||||
assert settings.extra_args is not None
|
||||
assert settings.extra_args["timeout"] == 120.0
|
||||
|
||||
|
||||
@pytest.mark.parametrize(
|
||||
"model_name",
|
||||
[
|
||||
"openai/gpt-5.4",
|
||||
"gpt-4o",
|
||||
"gemini/gemini-1.5-flash",
|
||||
"ollama/llama3",
|
||||
"bedrock/us.amazon.nova-lite-v1:0",
|
||||
],
|
||||
)
|
||||
def test_resolve_max_tokens_non_claude_is_untouched(model_name: str) -> None:
|
||||
# No ceiling is imposed on non-Claude models: None stays None (provider
|
||||
# default) and an explicit value passes through verbatim.
|
||||
assert resolve_max_tokens(model_name, None) is None
|
||||
assert resolve_max_tokens(model_name, 16000) == 16000
|
||||
|
||||
|
||||
@pytest.mark.parametrize(
|
||||
"model_name",
|
||||
[
|
||||
"bedrock/global.anthropic.claude-opus-4-8",
|
||||
"bedrock/us.anthropic.claude-opus-4-8",
|
||||
"bedrock/global.anthropic.claude-sonnet-4-6",
|
||||
],
|
||||
)
|
||||
def test_resolve_max_tokens_adaptive_claude_gets_default(model_name: str) -> None:
|
||||
# The models the fix targets: unset -> family default, well under their
|
||||
# (>=64k) ceiling so the value is the default itself, regardless of the
|
||||
# region/provider prefix on the model id.
|
||||
assert resolve_max_tokens(model_name, None) == 32000
|
||||
|
||||
|
||||
def test_resolve_max_tokens_clamps_default_to_small_ceiling() -> None:
|
||||
# Older Claude tops out below the family default; the returned value must be
|
||||
# the model's own ceiling, never the larger default (which would 400).
|
||||
result = resolve_max_tokens("bedrock/anthropic.claude-3-5-sonnet-20240620-v1:0", None)
|
||||
assert result is not None
|
||||
assert result <= 8192
|
||||
|
||||
|
||||
def test_resolve_max_tokens_clamps_explicit_override_to_ceiling() -> None:
|
||||
# An over-large explicit value is clamped down to what the model accepts.
|
||||
assert resolve_max_tokens("bedrock/global.anthropic.claude-opus-4-8", 500_000) == 128_000
|
||||
|
||||
|
||||
def test_resolve_max_tokens_unknown_claude_without_config_returns_none() -> None:
|
||||
# A Claude model absent from litellm's cost map has no known ceiling; rather
|
||||
# than guess a value that might exceed the real limit, fall back to None.
|
||||
assert resolve_max_tokens("claude-does-not-exist-9", None) is None
|
||||
|
||||
|
||||
def test_resolve_max_tokens_unknown_claude_respects_explicit_config() -> None:
|
||||
# If the ceiling is unknown but the user set a value, honor it as-is.
|
||||
assert resolve_max_tokens("claude-does-not-exist-9", 12345) == 12345
|
||||
|
||||
|
||||
@pytest.mark.parametrize("bad_value", ["0", "-1", "-4096"])
|
||||
def test_llm_settings_rejects_nonpositive_max_tokens(
|
||||
monkeypatch: pytest.MonkeyPatch, bad_value: str
|
||||
) -> None:
|
||||
# A nonpositive STRIX_MAX_TOKENS would yield empty output or a provider 400;
|
||||
# it must fail at settings-load rather than reach a request.
|
||||
from pydantic import ValidationError
|
||||
|
||||
from strix.config.settings import LlmSettings
|
||||
|
||||
monkeypatch.setenv("STRIX_MAX_TOKENS", bad_value)
|
||||
with pytest.raises(ValidationError):
|
||||
LlmSettings()
|
||||
|
||||
|
||||
def test_llm_settings_accepts_positive_max_tokens(monkeypatch: pytest.MonkeyPatch) -> None:
|
||||
from strix.config.settings import LlmSettings
|
||||
|
||||
monkeypatch.setenv("STRIX_MAX_TOKENS", "4096")
|
||||
assert LlmSettings().max_tokens == 4096
|
||||
|
|
|
|||
Loading…
Add table
Reference in a new issue