fix(llm): make reasoning_effort model-aware (denylist + retry-on-400)#47
fix(llm): make reasoning_effort model-aware (denylist + retry-on-400)#47msyed-01 wants to merge 8 commits into
Conversation
Adds _REASONING_EFFORT_UNSUPPORTED_PATTERNS and the AsyncLLMClient._auto_resolve_reasoning_effort static method. No call sites use it yet -- wired up in the next commit.
New sentinel default 'auto' dispatches to _auto_resolve_reasoning_effort. Explicit values (including None and 'medium') still pass through untouched -- fully backward-compatible for any caller that was setting the param explicitly.
Classifies RuntimeError messages from genai-pyo3 so we only retry on the specific 'reasoning_effort rejected' 400, not on rate-limits or unrelated 400s.
Reconstructs a frozen ChatOptions with reasoning_effort=None, preserving all other fields. Needed by the retry-on-400 path in achat/achat_stream.
Defense-in-depth for models the denylist misses. After one retry the instance state is mutated to None, so subsequent calls skip the param without another wasted round-trip.
Symmetric to the non-streaming path. Refactors the consume loop into an inner coroutine so the retry can re-run it cleanly with rebuilt options.
Whitespace-only changes. No behavior change. `ruff check` and `ruff format --check` now pass clean for the files this branch touches (the two pre-existing format issues in clearwing/llm/native.py at lines 190 and 615 also exist on main, so this PR leaves them alone).
The original docstring pointed at a design doc that lives in our internal tree, not in this repo. Replaced with a self-contained description of what the file covers.
|
master is the real main branch. Please rebase if you still want to merge it. |
|
Confirmed this addresses a real gap that's still present on One thing before review: this PR targets Thanks for the detailed problem writeup and the two-layer fix — keeping it open pending the rebase. |
|
Closing this out. To be clear for the record: this is a valid fix for a real gap — We're not merging it right now because:
Not a reflection on the work — the diagnosis and two-layer approach (denylist + retry-on-400) were solid. If/when we prioritize multi-provider support, this is the reference. Happy to reopen or take a fresh rebase against |
AsyncLLMClient defaulted reasoning_effort to "medium" and sent it on every OpenAI-compat request unconditionally. Non-reasoning models served over OpenAI-compat endpoints (Llama 3.x/4, Mistral, Mixtral, Qwen2/2.5, Gemma on Groq/Together/Fireworks/etc.) reject the param with HTTP 400, which killed the sourcehunt hunter loop and ranker for those providers. Two layers, both in clearwing/llm/native.py: 1. Model-name denylist with a sentinel default. The constructor default becomes "auto": _auto_resolve_reasoning_effort() maps known non-reasoning model families to None and everything else (incl. Anthropic/Claude) to "medium" — so the default Anthropic path is unchanged. Any explicit value (including None) bypasses auto-resolution. 2. Retry-on-400 safety net. If a provider still rejects reasoning_effort at call time, achat/achat_stream detect that specific 400, drop the param, latch reasoning_effort=None for the session, and retry once. Unrelated 400s/429s/500s propagate untouched. Re-port of #47 onto the refactored native.py (provider routing + codex/ aiohttp streaming fallback landed since the original branch). Tests from the original PR carried over verbatim. Co-authored-by: msyed-01 <288113432+msyed-01@users.noreply.github.com>
2 participants
Problem
AsyncLLMClientdefaultsreasoning_effortto"medium"and passes it through to every OpenAI-compat endpoint unconditionally. This breaks against any provider whose configured model isn't reasoning-capable. The most common case is Groq direct + Llama 3.x:Reproducible against the documented
clearwing config --set-providerflow followed byclearwing sourcehunt <target> --depth quick— the static-analyzer pre-pass still runs (so the user sees some output), but the LLM hunter loop and Ranker never execute.Files Hunted: 0. Same problem against Mistral, Mixtral, Qwen2/2.5 base, Gemma on Groq, Together, Fireworks, and other OpenAI-compat providers that serve mainstream non-reasoning models.The constructor accepts
reasoning_effort=Noneto opt out, but no caller in the codebase (across all 7AsyncLLMClient(...)construction sites inproviders/manager.pyandllm/chat.py) passes anything other than the default. There's no CLI surface to override it either, so end users on documented providers have no workaround short of patching the source.Fix — two layers, both inside
clearwing/llm/native.pyLayer 1: model-name denylist with sentinel default
A module-level pattern list (
_REASONING_EFFORT_UNSUPPORTED_PATTERNS) lists case-insensitive substrings of model names known to rejectreasoning_effort:llama-3,llama-4,mistral,mixtral,qwen2,gemma. An override allowlist (_REASONING_EFFORT_OVERRIDE_ALLOW, empty today) gives a clean escape hatch for reasoning-tuned variants of denylisted families as they ship.The constructor signature changes from
reasoning_effort: str | None = "medium"toreasoning_effort: str | None | Literal["auto"] = "auto". The new"auto"sentinel dispatches to a static helper that resolves toNoneon a denylist match,"medium"otherwise (preserving current behavior for everything not denylisted).Explicit values pass through untouched, including
None,"medium","high", etc. Any caller that was settingreasoning_effortexplicitly continues to work identically. Layer 1 only affects code paths that relied on the default.Layer 2: retry-on-400 fallback (defense in depth)
The denylist will never be complete. To handle the long tail — model renames, new providers tightening validation, patterns we missed — both
achatandachat_streamnow catchRuntimeError, classify it via_is_unsupported_reasoning_effort_error(requires both"reasoning_effort"AND either"400"or"unsupported"in the message to match), and on a match: rebuildChatOptionswithreasoning_effort=None, mutateself.reasoning_effort = Nonefor the instance lifetime, retry exactly once. UnrelatedRuntimeErrors propagate untouched.The instance-state mutation means subsequent calls on the same client skip the param without another wasted round-trip — one 400 per (model, provider) pair per task, not per call.
AsyncLLMClientinstances are per-task (sourcehunt/runner.py:1783 _get_llm(task, override)), so the mutation is bounded and safe.Validation
ruff check/ruff format --checkon touched filesnative.py:190and:615are pre-existing format issues onmain, left untouched — not in this PR's scope)mypy --follow-imports=silent clearwing/llm/native.pycall-argwarnings, all on the newChatOptions(...)reconstruction. Identical pattern to the 18 existing warnings on the two pre-existingChatOptions(...)constructions at lines 259 and 341 —genai_pyo3's stub doesn't expose its kwargs to mypy. No new classes of error.clearwing/llm/is outsideMYPY_SCOPEper the Makefile anyway.llama-3.3-70b-versatile,clearwing sourcehunt --depth standardagainst DVMCP)Files Hunted: 0, Ranker LLM call dies on first 400. After this PR:Files Hunted: 20/20, 10 findings, 5 verified, 4 CRITICAL + 1 HIGH, $0 actual spend (Groq free tier). Correctly identified DVMCP's intentionaleval()criticals. Runtime ~16min (Groq free-tier RPM throttling; Clearwing's existing rate-limit retry handled it cleanly).meta-llama/llama-3.3-70b-instruct)What this PR does NOT touch
clearwing/providers/manager.py,clearwing/llm/chat.py,clearwing/sourcehunt/runner.py,clearwing/ui/commands/sourcehunt.py— no plumbing changes--reasoning-efforttosourcehunt, but kept this PR scoped to the bug; happy to follow up in a separate PR if you want the explicit surface)FakeResponse.reasoning_contentmissing intest_deep_agent_loop.py/test_band_promotion.py;AgentTool.fnremoved but still called intest_cve_tools.py;test_webcrypto_hooks::test_succeeds_with_init_scriptassertingFalse is True) — separate concerns, can file as a separate issue if usefulFiles changed
clearwing/llm/native.py(+151 / -13)tests/test_native_reasoning_effort.py(+319, new file, 26 tests across 6 test classes)Commits
Seven commits, each a coherent TDD step (red → green → commit), so the history is reviewable commit-by-commit. Squash on merge is fine if you prefer single-commit PRs.
Backward compatibility
reasoning_effort=Xcalls pass through untouched (includingreasoning_effort="medium"against a denylisted model — Layer 1 only fires when the caller didn't specify)Happy to iterate on the denylist patterns, the sentinel naming, or the retry semantics. Thanks for considering!