Skip to content

feat(llm): add reasoningEffort so a caller can turn hidden reasoning off - #308

Open
InfiniLakeSoftware wants to merge 2 commits into
trailhq:mainfrom
InfiniLakeSoftware:feat/reasoning-effort-option
Open

feat(llm): add reasoningEffort so a caller can turn hidden reasoning off#308
InfiniLakeSoftware wants to merge 2 commits into
trailhq:mainfrom
InfiniLakeSoftware:feat/reasoning-effort-option

Conversation

@InfiniLakeSoftware

Copy link
Copy Markdown
Contributor

Problem

A reasoning-capable model can spend its entire max_tokens budget on hidden reasoning and return 200 with empty content. ChatSummarizer caps maxTokens at 2048 and context/build.ts caches whatever comes back, so the result is a blank summary written by a run that reported success the whole way through.

Measured directly against a local LM Studio server with qwen/qwen3.5-9b, using graft's own summarizer prompt:

finish_reason: "length"
content length: 0
usage: { completion_tokens: 2048, completion_tokens_details: { reasoning_tokens: 2048 } }

Every token went to reasoning; the answer never started. On a real repo this left 1,638 of 3,502 cached summaries empty.

This is not the case #27 fixed, and not the one isRejectedToolsWithReasoning covers. Every fallback in createChatCompletion lives in the catch and is keyed on a 400 — they handle providers that reject the request. Here nothing is rejected and nothing throws, so no catch-based recovery can reach it. The only fix is for the request to say up front how much reasoning to spend.

Fix

Add reasoningEffort to the OpenAI-compatible transport and thread it through the existing config chain:

  • ReasoningEffort in src/ai/llm/types.ts (none | minimal | low | medium | high), so the public config surface doesn't leak the OpenAI SDK's type.
  • OpenAIChatModelOptions.reasoningEffort → sent as reasoning_effort on the request. LiteLLMChatModel and OrcaRouterChatModel extend OpenAIChatModel, so they inherit it; ChatModelConfig passes it to all three. The anthropic adapter ignores it.
  • EngineConfig.reasoningEffort, resolved in resolveConfig from GRAFT_REASONING_EFFORT, matching how GRAFT_PROVIDER / GRAFT_MODEL / GRAFT_BASE_URL already work.
  • --reasoning-effort <level> alongside the existing --provider / --model / --api-key / --base-url flags.

Unset by default, so the model's own default stays in force and nothing changes for existing users or providers.

Why an explicit knob rather than detection

Detecting "empty content plus a large reasoning_tokens" and retrying would be guessing at intent — a legitimately empty completion is possible, and retrying doubles cost on a path that is already the expensive one. Whether reasoning is wanted is the caller's decision, and every other provider dimension in graft is already expressed the same way: config field, env var, CLI flag.

Worth noting for anyone hitting this against LM Studio specifically: it ignores both chat_template_kwargs: {enable_thinking: false} and Qwen's /no_think prompt switch. I verified both are no-ops there — identical reasoning-token counts and byte-identical output with each set. reasoning_effort is the only switch it honours.

Testing

Two adapter tests in test/llm-adapters.test.ts, network-free, using the existing stub-client pattern:

  • omitted from the request unless set; forwarded verbatim when set (none, high)
  • present on the first request alongside a forced object tool_choice — the exact combination the existing 400-driven fallback cannot produce, since it only ever appears on a retry

Full suite green (1,217 passing, 5 skipped). README.md updated alongside the other GRAFT_* variables.

Measured effect

On the same 4-file sample at temperature 0, none vs the model's default: summaries of equivalent quality, but 8.7× faster (15s vs 132s) with 7,276 reasoning tokens no longer generated and discarded. Turning reasoning off also stopped the blank-summary failure entirely — peak output was 262 tokens, comfortably inside the existing 2048 budget.

A reasoning-capable model can spend the entire max_tokens budget on hidden
reasoning and return HTTP 200 with empty content and finish_reason "length".
graft caches whatever comes back, so this shows up as blank summaries from a
run that reported success throughout. Measured against a local LM Studio
server with qwen3.5-9b: 2048 max_tokens, 2048 reasoning tokens, zero content.

createChatCompletion already recovers from providers that REJECT tools while
reasoning is active, but that path is driven by a 400 - it cannot reach this
case, because nothing throws. The request has to say up front how much
reasoning to spend.

Adds reasoningEffort to the OpenAI-compatible transport, threaded through
ChatModelConfig and EngineConfig, resolved from GRAFT_REASONING_EFFORT, and
exposed as --reasoning-effort. Unset by default, so the model's own default
stays in force and no existing behaviour changes. LiteLLM and OrcaRouter
inherit it via OpenAIChatModel.

Two adapter tests cover it: omitted unless set, forwarded when set, and
present on the first request alongside a forced object tool_choice - the
combination the existing 400-driven fallback cannot produce.

Files changed:
- src/ai/llm/types.ts
- src/ai/llm/openai.ts
- src/ai/llm/factory.ts
- src/ai/providers.ts
- src/cli.ts
- src/engine.ts
- test/llm-adapters.test.ts
- README.md
@trailhq-graft

trailhq-graft Bot commented Sep 8, 2026

Copy link
Copy Markdown

🌱 graft blast radius

4 areas changed → 3 areas can be affected. 8 dependent symbols, depth 2.
Tests: no test reaches Report Naming, CLI Configuration; 2 areas updated their tests.
Tag: @anirudhkumar-nanonets — 6 of 7 areas · @shhdwi — CLI Configuration · lovejones2914-spec — Chat Model Factory, LLM Routing

flowchart TB
  A0(("Pull Request Review<br/>4 symbols"))
  A1(("LLM Routing<br/>2 symbols"))
  A2(("Cluster Naming<br/>2 symbols"))
  classDef reached fill:#D9EDF3,stroke:#3AA7C9,stroke-width:1.5px,color:#0E313C;
  class A0,A1,A2 reached;
Loading
Can be affected Symbols Nearest hop Reached from
Pull Request Review 4 src/app/review.ts:L45-L99 reviewPullRequest — calls, depth 1 Chat Model Factory, Provider Configuration +1
LLM Routing 2 src/ai/llm/litellm.ts:L25-L33 LiteLLMChatModel — extends, depth 1 Chat Model Factory
Cluster Naming 2 src/blast/blast-cli.ts:L132-L136 nameClusters — calls, depth 1 Chat Model Factory, Provider Configuration +1
Who knows this code — 4 people across 7 areas
Area Who knows it
Chat Model Factory · changed @anirudhkumar-nanonets — 3 commits, last 20d ago · lovejones2914-spec — 1 commit, last 9d ago
Report Naming · changed @anirudhkumar-nanonets — 2 commits, last 13d ago
Provider Configuration · changed @anirudhkumar-nanonets — 7 commits, last 2mo ago
CLI Configuration · changed @anirudhkumar-nanonets — 38 commits, last 7d ago · @shhdwi — 24 commits, last 28d ago
Pull Request Review · affected @anirudhkumar-nanonets — 5 commits, last 8d ago
LLM Routing · affected lovejones2914-spec — 1 commit, last 9d ago · Prodman Devokadev — 1 commit, last 13d ago
Cluster Naming · affected @anirudhkumar-nanonets — 7 commits, last 13d ago

Ownership is git history over each area's own files, weighted towards recent work (120-day half-life). Merge commits and bots are dropped, and you are dropped from your own PR. A name with no @ has no GitHub handle in its commit email — tag them by hand, or add a .mailmap entry. A suggestion from history, not a CODEOWNERS rule.

All 8 dependent symbols, grouped by area

Pull Request Review — 4 symbols in 4 files

  • src/app/review.ts:L45-L99 — reviewPullRequest (calls, depth 1)
    51: const checkout = checkoutPullRequest({ owner: job.owner, repo: job.repo, number: job.number, baseRef: job.baseRef, token, log });
  • src/app/review-process.ts:L179-L183 — childReviewer (references, depth 2)
  • src/app/review-worker.ts:L67-L87 — run (calls, depth 2)
  • src/app/server.ts:L31-L37 — AppSeams (references, depth 2)

LLM Routing — 2 symbols in 2 files

  • src/ai/llm/litellm.ts:L25-L33 — LiteLLMChatModel (extends, depth 1)
    25: export class LiteLLMChatModel extends OpenAIChatModel {
  • src/ai/llm/orcarouter.ts:L25-L33 — OrcaRouterChatModel (extends, depth 1)
    25: export class OrcaRouterChatModel extends OpenAIChatModel {

Cluster Naming — 2 symbols in 1 file

  • src/blast/blast-cli.ts:L132-L136 — nameClusters (calls, depth 1)
    133: const { nameReport } = await import("./name.js");
  • src/blast/blast-cli.ts:L63-L124 — runBlastCommand (calls, depth 2)
Test signal per changed area — 2 ✓ · 2 ✗

Reached = a node under a test path has a resolved edge into the changed symbol. It undercounts anything called indirectly — through a CLI, a spawned process or a dynamic import — so read a low ratio as “look here”, never as a coverage gate.

  • Chat Model Factory — 3 of 5 reached · 2 test files changed here: test/llm-adapters.test.ts, test/provider-extra-body.test.ts
    • not reached: sanitizeExtraBody, constructor
  • Report Naming — 0 of 1 reached · no test file reaches it
    • not reached: nameReport
  • Provider Configuration — 2 of 2 reached · 1 test file changed here: test/provider-extra-body.test.ts
  • CLI Configuration — 0 of 2 reached · no test file reaches it
    • not reached: cliConfig, chatModel
2 test suites also reference this code

2 symbols, kept out of the diagram and the table so they cannot crowd out the areas a reviewer has to look at.

  • test/litellm-adapter.test.ts
  • test/orcarouter-adapter.test.ts

⚠️ 2 changed files not in the graph (.env.example, README.md) — no parser claims the extension, or the index predates the file.

graft blast · origin/main...HEAD · depth 2 · 12 changed files

Open the interactive graph → — click an area to see its dependent symbols at file:line.

github-actions Bot added a commit that referenced this pull request Sep 8, 2026
@jgarciao

jgarciao commented Sep 9, 2026

Copy link
Copy Markdown

Ran into exactly the case this PR targets (a reasoning model burning the whole --deep budget on hidden reasoning), but on a different backend — GRAFT_PROVIDER=litellm → a LiteLLM gateway fronting vLLM (a Qwen3 reasoning variant). Wanted to flag a wire-level gotcha before this lands, because on this stack the PR as written wouldn't turn reasoning off.

I reproduced graft's concept-step request (forced emit_json tool, tool_choice forced) against the gateway and varied only how the reasoning switch is delivered:

Delivery reasoning tokens latency result
reasoning_effort: "none" top-level (this PR) 177 38.8s still reasons
extra_body: { reasoning_effort: "none" } 0 7.9s clean tool_calls
chat_template_kwargs: { enable_thinking: false } (top-level) 0 12.2s clean tool_calls

So on LiteLLM+vLLM the top-level reasoning_effort is silently dropped, while the same value delivered under extra_body works — as does vLLM's own chat_template_kwargs.enable_thinking. (The PR notes enable_thinking/no_think were no-ops on LM Studio; that doesn't generalize — on vLLM they're the switch that actually fires.) Without this, the request just runs long enough to hit the gateway's ~60s timeout and graft fails with Connection error.

Would you consider either (a) sending reasoning_effort via extra_body in the OpenAI-compatible adapter, or (b) exposing a small general extra_body passthrough (e.g. GRAFT_LLM_EXTRA_BODY as JSON, merged into the request body)? Option (b) covers reasoning_effort, chat_template_kwargs, and future provider-specific params in one switch and would make this work across gateways, not just LM Studio. Happy to test any change against this gateway.

…body

PR trailhq#308 added `reasoning_effort` as a top-level field, which is what LM Studio
honours. On a LiteLLM proxy fronting vLLM it is silently dropped during the
gateway's own param mapping: the request then runs long enough to hit the
gateway timeout, and graft fails with `Connection error.` The same value under
`extra_body` is forwarded to the server untouched, and a vLLM server reached
directly wants `chat_template_kwargs: { enable_thinking: false }` instead —
reported on trailhq#308 with measurements (177 reasoning tokens / 38.8s top-level,
0 / 7.9s under `extra_body`, 0 / 12.2s via `chat_template_kwargs`).

So the switch is real but its spelling is not portable, and a typed field per
gateway quirk is a losing game. Add `extraBody`: a JSON object merged into the
request body, threaded through the same chain as every other provider
dimension — `ChatModelConfig`, `EngineConfig`, `GRAFT_LLM_EXTRA_BODY`, and
`--extra-body`. It covers `reasoning_effort`, `chat_template_kwargs`, and
whatever the next gateway names differently, in one switch.

Merged last, so a caller who names a key graft also sets gets their value on
the wire: the stack in front of the model, not this adapter, decides which
spelling works. Five keys are reserved — model, messages, tools, tool_choice,
stream — because those carry the structured-output coercion the caller asked
for, the manifest label, and the response shape. They are dropped with one
warning at construction rather than per request.

Malformed input throws instead of being ignored. A passthrough exists precisely
because the request fails without it, so silently dropping a bad one would send
the very request the user was avoiding, and they would debug their gateway
rather than their JSON.

Also threads both `reasoningEffort` and `extraBody` into the naming pass in
`nameReport`, which built its own chat model and so was the one LLM call in the
product that ignored the user's reasoning settings entirely — the same omission
this change would otherwise repeat.

Testing: the adapter merge, override and reserved-key cases are unit tests, but
the path that actually broke is the whole chain, so `cli-extra-body.test.ts`
runs a real `graft build --deep` against a recording stand-in gateway and
asserts the JSON on the socket — both the flag and the env var, including that
a reserved key is refused out loud. `provider-extra-body.test.ts` covers config
resolution and that the value reaches all three OpenAI-compatible adapters.
Full suite green (1,232 passing, 5 skipped).

Files changed:
- src/ai/llm/types.ts
- src/ai/llm/openai.ts
- src/ai/llm/factory.ts
- src/ai/providers.ts
- src/blast/name.ts
- src/cli.ts
- src/engine.ts
- test/llm-adapters.test.ts
- test/provider-extra-body.test.ts
- test/cli-extra-body.test.ts
- README.md
- .env.example

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
github-actions Bot added a commit that referenced this pull request Sep 9, 2026
@InfiniLakeSoftware

Copy link
Copy Markdown
Contributor Author

Thanks — the measurements are what makes this actionable, especially the three-way split. I've gone with (b), your general passthrough, and pushed it to this branch.

GRAFT_LLM_EXTRA_BODY (or --extra-body) takes a JSON object merged into the request body, threaded through the same chain as every other provider dimension — ChatModelConfig, EngineConfig, env var, CLI flag. Your two working spellings then both express themselves without graft having to know which gateway is in the way:

GRAFT_LLM_EXTRA_BODY={"extra_body":{"reasoning_effort":"none"}}          # LiteLLM → vLLM
GRAFT_LLM_EXTRA_BODY={"chat_template_kwargs":{"enable_thinking":false}}  # vLLM direct

I didn't take (a) — moving reasoning_effort into extra_body in the adapter — for two reasons. On a plain OpenAI-compatible endpoint, a top-level extra_body key is an unknown field, and OpenAI's own API rejects those, so it would trade your breakage for someone else's. And your own table shows the two working deliveries are different shapes, not one relocation: no single hardcoded position covers both. Making it explicit means graft never invents a body its user didn't ask for.

Details worth knowing before you test it:

  • The passthrough is merged last, so if you set reasoning_effort there it wins over --reasoning-effort. The stack in front of the model decides which spelling fires, not the adapter.
  • Five keys are reservedmodel, messages, tools, tool_choice, stream — since they carry the structured-output coercion, the manifest label, and the response shape. They're dropped with one warning at construction, not silently.
  • Malformed JSON throws, naming the source. Ignoring it would send exactly the request the passthrough was added to avoid, and you'd end up debugging the gateway instead of the JSON.
  • I also threaded both reasoningEffort and the new field into nameReport, which builds its own chat model — graft blast --name was the one LLM call in the product ignoring reasoning settings entirely.

On testing: I can only prove graft's side. test/cli-extra-body.test.ts runs a real graft build --deep against a recording stand-in gateway and asserts the JSON that reaches the socket, because the thing I couldn't take on faith was whether the openai Node SDK forwards unknown params at all — it does. What I can't reproduce is your LiteLLM→vLLM hop actually forwarding extra_body to the server. If you still have that gateway up, that's the half I'd value a check on.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants