Skip to content

fix(providers): send the request shape reasoning models require - #184

Open
LorenzoGentile wants to merge 2 commits into
atomicstrata:mainfrom
LorenzoGentile:fix/openai-reasoning-model-request-shape
Open

fix(providers): send the request shape reasoning models require#184
LorenzoGentile wants to merge 2 commits into
atomicstrata:mainfrom
LorenzoGentile:fix/openai-reasoning-model-request-shape

Conversation

@LorenzoGentile

Copy link
Copy Markdown

What

OpenAIProvider picks the token-limit field from the model id instead of always sending max_tokens, and can attach reasoning_effort. Two env overrides cover ids that prefix detection cannot classify.

Why

complete, stream, and toolCall each hard-coded max_tokens. The o-series and GPT-5 families reject it:

Unsupported parameter: 'max_tokens' is not supported with this model.
Use 'max_completion_tokens' instead.

That 400 comes back on the first extraction request of a compile, before any page is written — so these models are not degraded under llmwiki, they are unusable. The SDK agrees on the direction: openai@6.42.0 documents max_tokens as "now deprecated in favor of max_completion_tokens".

Separately, some of these models reject a request that carries function tools without reasoning_effort — which is the shape of every compile extraction call, since it goes through toolCall with tool_choice: "required".

I ran into both while evaluating a GPT-5-class model for a compile workload, and had to put a local proxy in front of llmwiki that rewrote the request body. That proxy is what this PR makes unnecessary.

Design

  • src/providers/openai-request.ts owns the two decisions. It is a separate module mostly because openai.ts was at 299 lines against the 400-line limit in CLAUDE.md.
  • The three completion methods now share a private requestBase(...). That is what stops one of them from drifting back to a hard-coded field — the duplication was the reason all three had to be fixed at once.
  • Detection is by model-id prefix (o1, o3, o4, gpt-5, case-insensitive). I deliberately did not try to infer anything from arbitrary vendor ids: behind an OpenAI-compatible gateway a reasoning model can be called anything, and a guess would misfire in both directions. LLMWIKI_OPENAI_TOKEN_PARAM is the escape hatch for exactly that case, and it is how I tested this.
  • LLMWIKI_OPENAI_REASONING_EFFORT is opt-in and unset by default. llmwiki does not guess an effort level — the right value is a property of the model and the workload, and silently choosing one would change cost and latency on models that were working fine.
  • Both variables are validated against the accepted values on read, so a typo fails immediately with them named rather than as an opaque 400 part-way through a compile. That follows the direction Allow a separate embedding endpoint for Claude Agent #154 took with the embedding-provider guard.

Defaults are byte-identical for every model that does not match a reasoning prefix.

What this PR does not do

It does not touch tool_choice: "required". That hard-coding causes a separate incompatibility with thinking-capable models on some non-OpenAI gateways, but I do not have a verified fix for it, so I have written it up as an issue rather than guessing here.

How to test

npx tsc --noEmit
npm run build
npm test
npm run fallow:ci

test/openai-request-shape.test.ts covers prefix detection across both families, both override directions, rejection of unknown values, the full reasoning_effort value set, and — most importantly — the actual bodies the provider hands the SDK on the complete and toolCall paths, asserting that the reasoning path sends max_completion_tokens and not max_tokens, and that tool_choice is untouched. No network access: the SDK's create is swapped for a capture.

Full suite on this branch: 4881 passed, 3 skipped, no regressions. npm run fallow:ci: 0 above threshold, maintainability 91.3.

Against a live gateway:

LLMWIKI_PROVIDER=openai LLMWIKI_MODEL=<gpt-5-class-id> \
LLMWIKI_OPENAI_TOKEN_PARAM=max_completion_tokens \
LLMWIKI_OPENAI_REASONING_EFFORT=none \
llmwiki compile

Docs updated in docs/configuration/environment-variables.mdx, plus a CHANGELOG entry under Unreleased.

`OpenAIProvider` hard-coded `max_tokens` in all three completion methods.
The o-series and GPT-5 families reject it outright:

  Unsupported parameter: 'max_tokens' is not supported with this model.
  Use 'max_completion_tokens' instead.

That 400 lands on the first extraction request, so those models were not
degraded but unusable. The SDK's own types have carried `max_tokens` as
deprecated in favour of `max_completion_tokens` since 6.x.

The field is now chosen from the model id, with LLMWIKI_OPENAI_TOKEN_PARAM
to force it — a gateway can serve a reasoning model under an id no prefix
list can anticipate, which is how we hit this.

LLMWIKI_OPENAI_REASONING_EFFORT adds `reasoning_effort`, which some of
these models require whenever the request carries function tools — the
shape every compile extraction call has. Unset sends nothing.

An unrecognised value for either variable fails with the accepted values
named, instead of as an opaque 400 part-way through a compile.

The three methods now share one request builder, so they cannot drift back
apart. Defaults are byte-identical for non-reasoning models.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Selecting `max_completion_tokens` from the model id while leaving
`reasoning_effort` env-only left the feature half-done: a GPT-5 model got
past the token-parameter error and then failed on the next line, because
it rejects a request carrying function tools unless the field is present —
and llmwiki's extraction pass always sends tools. Working out of the box
should not require reading the changelog.

`none` because extraction and page generation are structured tool calls,
where reasoning tokens buy latency rather than accuracy. The env override
still wins, for anyone who wants to trade that back.

The default is deliberately narrower than the token-parameter prefixes.
The o-series accepts a tool-carrying request with the field absent and
rejects some of the accepted values, so applying it there would turn a
working request into a 400; it stays opt-in through the variable.

@ethanj ethanj left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good fix, and the restraint is the best part: defaulting GPT-5 to none but leaving the o-series alone, with the docstring explaining why the two prefix lists differ. I checked the diagnosis too, and extraction-phase.ts does rethrow, so "unusable rather than degraded" is accurate. After this there's no max_tokens left anywhere in openai.ts, which is the drift guard you promised.

One thing to fix.

Invalid override values get retried as transient API failures. callClaude decides retriability by matching the error message against /^4(?!29)\d\d\b/. OpenAIRequestConfigError starts with the variable name, not a status code, so a local typo enters the four-attempt backoff ladder. With LLMWIKI_OPENAI_REASONING_EFFORT=bogus: attempts 1/4 through 3/4, failure after the fourth, 15.2 seconds and no network call. At the default concurrency of 5 that runs on five calls at once.

Worth noting #154 went the other way on this. From ensureEmbeddingProviderAvailable: "Runs HERE, at the door [...] One misspelled variable is not worth three behaviours."

Validate both variables in ensureProviderAvailable() and a typo fails instantly with the variable named, which is what your description claims. Failing that, classify the error non-retriable in llm.ts. Either way, a test asserting one attempt, no backoff.

Three smaller ones:

  • Copilot, MiniMax and Ollama all extend OpenAIProvider, so a gpt-5 id on any of them now gets reasoning_effort: none with nothing configured. The default is right, but the docs note shouldn't sit under the OpenAI heading.
  • REASONING_EFFORTS matches openai@6.42.0 exactly, but package.json uses ^6.42.0. A conditional-type assertion against Exclude<OpenAI.ReasoningEffort, null> would catch drift at tsc time.
  • No stream() case in the tests. complete and toolCall are pinned, stream isn't, and that's the one path spreading requestBase into a differently-typed request.

ethanj pushed a commit that referenced this pull request Aug 21, 2026
…hich ran

Closes the first two items of #186.

A prompt modifier is a setting that changes what the page prompt ASKS FOR
without changing the committed prompt wording — today only the output language,
set by --lang or LLMWIKI_OUTPUT_LANG. `detectChanges` classifies a source purely
by the SHA-256 of its bytes, so `llmwiki compile --lang Japanese` over a settled
project reported "Nothing to compile" and left every page in the previous
language. The selection now travels in state.json and a flipped modifier
invalidates the pages it would have changed.

Pages also carry a `promptModifiers` frontmatter entry, surfaced per page in the
JSON export. PROMPT_VERSION names the prompt IMPLEMENTATION and is identical
whether or not a modifier was active, so it could not separate two such pages.

The digest is one GLOBAL fact, so anything that narrows what a run recompiles
can otherwise advance it past work that never happened, leaving a page current
and permanently stale. Two such paths exist and both are handled:

  - A SCOPED run (`refresh --stale`, which supplies a changeFilter) recompiles a
    subset by design and must not record the selection as true of the project.
    It flushes source state but leaves the digest, so the sources it filtered out
    keep the only signal that says they are stale. The refreshed pages pay a
    second regeneration on the next full compile — the safe direction.

  - `markUnchangedPendingSources` demotes a promoted source back to `unchanged`
    when a pending review candidate carries the same source hash. It ran on the
    line after the promotion, so with any candidate present the feature was
    undone entirely and a modifier flip made zero model calls. A candidate
    produced under a different selection is not a duplicate of this run's work,
    so deduplication is skipped when the selection changed.

An ABSENT digest reads as "none selected" rather than as its own third state.
Reading absence as "no difference" was tried first and is wrong: the no-op
compile path never flushes state, so a project with nothing to compile would
never record a first digest, and flipping a modifier on exactly the settled
project this exists for would stay silent forever. Reading it as "none" costs an
untouched project nothing and costs one recompile to a project already running
under --lang when it upgrades — the same trade the embedding store makes for an
index that predates fingerprints.

The digest is recorded at the single durable flush rather than at load, so a
compile that dies mid-run leaves the previous digest on disk and the re-run
still sees the difference.

`promptModifierPairs` is the one canonical rendering that both the digest and
the frontmatter stamp consume, so a page's recorded modifiers and the state
digest cannot disagree about what the same selection means.

Items 3 and 4 of #186 are not included: they need files from #182 and #184,
which are unmerged. When #183 lands, --no-sources-section becomes one more
entry in activePromptModifiers and inherits the invalidation.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01MUdoRq1DJq23aJuhK9QK7X
ethanj pushed a commit that referenced this pull request Aug 21, 2026
…hich ran

Closes the first two items of #186.

A prompt modifier is a setting that changes what the page prompt ASKS FOR
without changing the committed prompt wording — today only the output language,
set by --lang or LLMWIKI_OUTPUT_LANG. `detectChanges` classifies a source purely
by the SHA-256 of its bytes, so `llmwiki compile --lang Japanese` over a settled
project reported "Nothing to compile" and left every page in the previous
language. The selection now travels in state.json and a flipped modifier
invalidates the pages it would have changed.

Pages also carry a `promptModifiers` frontmatter entry, surfaced per page in the
JSON export. PROMPT_VERSION names the prompt IMPLEMENTATION and is identical
whether or not a modifier was active, so it could not separate two such pages.

The digest is one GLOBAL fact, so anything that narrows what a run recompiles
can otherwise advance it past work that never happened, leaving a page current
and permanently stale. Two such paths exist and both are handled:

  - A SCOPED run (`refresh --stale`, which supplies a changeFilter) recompiles a
    subset by design and must not record the selection as true of the project.
    It flushes source state but leaves the digest, so the sources it filtered out
    keep the only signal that says they are stale. The refreshed pages pay a
    second regeneration on the next full compile — the safe direction.

  - `markUnchangedPendingSources` demotes a promoted source back to `unchanged`
    when a pending review candidate carries the same source hash. It ran on the
    line after the promotion, so with any candidate present the feature was
    undone entirely and a modifier flip made zero model calls. A candidate
    produced under a different selection is not a duplicate of this run's work,
    so deduplication is skipped when the selection changed.

An ABSENT digest reads as "none selected" rather than as its own third state.
Reading absence as "no difference" was tried first and is wrong: the no-op
compile path never flushes state, so a project with nothing to compile would
never record a first digest, and flipping a modifier on exactly the settled
project this exists for would stay silent forever. Reading it as "none" costs an
untouched project nothing and costs one recompile to a project already running
under --lang when it upgrades — the same trade the embedding store makes for an
index that predates fingerprints.

The digest is recorded at the single durable flush rather than at load, so a
compile that dies mid-run leaves the previous digest on disk and the re-run
still sees the difference.

`promptModifierPairs` is the one canonical rendering that both the digest and
the frontmatter stamp consume, so a page's recorded modifiers and the state
digest cannot disagree about what the same selection means.

Items 3 and 4 of #186 are not included: they need files from #182 and #184,
which are unmerged. When #183 lands, --no-sources-section becomes one more
entry in activePromptModifiers and inherits the invalidation.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01MUdoRq1DJq23aJuhK9QK7X
ethanj pushed a commit that referenced this pull request Aug 21, 2026
…hich ran

Closes the first two items of #186.

A prompt modifier is a setting that changes what the page prompt ASKS FOR
without changing the committed prompt wording — today only the output language,
set by --lang or LLMWIKI_OUTPUT_LANG. `detectChanges` classifies a source purely
by the SHA-256 of its bytes, so `llmwiki compile --lang Japanese` over a settled
project reported "Nothing to compile" and left every page in the previous
language. The selection now travels in state.json and a flipped modifier
invalidates the pages it would have changed.

Pages also carry a `promptModifiers` frontmatter entry, surfaced per page in the
JSON export. PROMPT_VERSION names the prompt IMPLEMENTATION and is identical
whether or not a modifier was active, so it could not separate two such pages.

The digest is one GLOBAL fact, so anything that narrows what a run recompiles
can otherwise advance it past work that never happened, leaving a page current
and permanently stale. Two such paths exist and both are handled:

  - A SCOPED run (`refresh --stale`, which supplies a changeFilter) recompiles a
    subset by design and must not record the selection as true of the project.
    It flushes source state but leaves the digest, so the sources it filtered out
    keep the only signal that says they are stale. The refreshed pages pay a
    second regeneration on the next full compile — the safe direction.

  - `markUnchangedPendingSources` demotes a promoted source back to `unchanged`
    when a pending review candidate carries the same source hash, so with any
    candidate present a modifier flip made zero model calls. Each candidate now
    records the digest it was GENERATED under and dedup demotes only when the
    hash and the selection both match.

    Comparing against the PROJECT's digest instead is not sufficient, and the
    difference is only visible in one direction: review mode never flushes
    state, so a project whose only compiles were `--review` has no recorded
    digest, and an absent digest means "none selected" — exactly what clearing a
    modifier requests. Japanese to Spanish passes either way; Japanese to
    default does not.

An ABSENT digest reads as "none selected" rather than as its own third state.
Reading absence as "no difference" was tried first and is wrong: the no-op
compile path never flushes state, so a project with nothing to compile would
never record a first digest, and flipping a modifier on exactly the settled
project this exists for would stay silent forever. Reading it as "none" costs an
untouched project nothing and costs one recompile to a project already running
under --lang when it upgrades — the same trade the embedding store makes for an
index that predates fingerprints.

The digest is recorded at the single durable flush rather than at load, so a
compile that dies mid-run leaves the previous digest on disk and the re-run
still sees the difference.

`promptModifierPairs` is the one canonical rendering that both the digest and
the frontmatter stamp consume, so a page's recorded modifiers and the state
digest cannot disagree about what the same selection means.

Items 3 and 4 of #186 are not included: they need files from #182 and #184,
which are unmerged. When #183 lands, --no-sources-section becomes one more
entry in activePromptModifiers and inherits the invalidation.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01MUdoRq1DJq23aJuhK9QK7X
ethanj added a commit that referenced this pull request Aug 21, 2026
…hich ran (#188)

Closes the first two items of #186.

A prompt modifier is a setting that changes what the page prompt ASKS FOR
without changing the committed prompt wording — today only the output language,
set by --lang or LLMWIKI_OUTPUT_LANG. `detectChanges` classifies a source purely
by the SHA-256 of its bytes, so `llmwiki compile --lang Japanese` over a settled
project reported "Nothing to compile" and left every page in the previous
language. The selection now travels in state.json and a flipped modifier
invalidates the pages it would have changed.

Pages also carry a `promptModifiers` frontmatter entry, surfaced per page in the
JSON export. PROMPT_VERSION names the prompt IMPLEMENTATION and is identical
whether or not a modifier was active, so it could not separate two such pages.

The digest is one GLOBAL fact, so anything that narrows what a run recompiles
can otherwise advance it past work that never happened, leaving a page current
and permanently stale. Two such paths exist and both are handled:

  - A SCOPED run (`refresh --stale`, which supplies a changeFilter) recompiles a
    subset by design and must not record the selection as true of the project.
    It flushes source state but leaves the digest, so the sources it filtered out
    keep the only signal that says they are stale. The refreshed pages pay a
    second regeneration on the next full compile — the safe direction.

  - `markUnchangedPendingSources` demotes a promoted source back to `unchanged`
    when a pending review candidate carries the same source hash, so with any
    candidate present a modifier flip made zero model calls. Each candidate now
    records the digest it was GENERATED under and dedup demotes only when the
    hash and the selection both match.

    Comparing against the PROJECT's digest instead is not sufficient, and the
    difference is only visible in one direction: review mode never flushes
    state, so a project whose only compiles were `--review` has no recorded
    digest, and an absent digest means "none selected" — exactly what clearing a
    modifier requests. Japanese to Spanish passes either way; Japanese to
    default does not.

An ABSENT digest reads as "none selected" rather than as its own third state.
Reading absence as "no difference" was tried first and is wrong: the no-op
compile path never flushes state, so a project with nothing to compile would
never record a first digest, and flipping a modifier on exactly the settled
project this exists for would stay silent forever. Reading it as "none" costs an
untouched project nothing and costs one recompile to a project already running
under --lang when it upgrades — the same trade the embedding store makes for an
index that predates fingerprints.

The digest is recorded at the single durable flush rather than at load, so a
compile that dies mid-run leaves the previous digest on disk and the re-run
still sees the difference.

`promptModifierPairs` is the one canonical rendering that both the digest and
the frontmatter stamp consume, so a page's recorded modifiers and the state
digest cannot disagree about what the same selection means.

Items 3 and 4 of #186 are not included: they need files from #182 and #184,
which are unmerged. When #183 lands, --no-sources-section becomes one more
entry in activePromptModifiers and inherits the invalidation.
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