Skip to content

fix(combo): trust finish_reason stop over the reasoning-ratio heuristic - #14980

Closed
HouMinXi wants to merge 37 commits into
cheaper-inference:release/v3.8.51from
HouMinXi:fix/quality-gate-stop-trust
Closed

HouMinXi wants to merge 37 commits into
cheaper-inference:release/v3.8.51from
HouMinXi:fix/quality-gate-stop-trust

Conversation

@HouMinXi

Copy link
Copy Markdown
Contributor

Summary

The combo quality gate treats a response as failed when reasoning tokens
consume 90% or more of completion tokens and the visible content is empty.
This was added for models that optionally think and run out of budget
mid-reasoning (#3587, #12262). It misfires on models that cannot disable
thinking: Claude Opus 4.7+ and GLM-5.3 both spend most of their output
budget on reasoning even when they finish normally.

The gate already trusts finish_reason: "length" and "max_tokens" as a
direct truncation signal (#12262). This change extends that trust to
finish_reason: "stop": a clean stop means the model ended on its own, so
the 90% ratio is not evidence of exhaustion. The ratio check stays as the
fallback when the provider reports no finish_reason at all.

What we checked

Three vendor APIs document the same contract:

  • OpenAI Chat Completions: finish_reason: "stop" is a natural end of turn;
    "length" is the token cap. A reasoning model that spends the whole budget
    on thinking returns "length" with empty content, not "stop".
  • Anthropic Messages: stop_reason: "end_turn" is a natural end;
    "max_tokens" is the cap. The translator maps end_turn to stop.
  • Z.ai GLM-5.3: thinking is forced. thinking.type: "disabled" returns an
    error. The model still reports finish_reason: "stop" when it finishes
    normally (docs.z.ai, rechecked 2026-09-05).

models.dev does not record whether a model's stop is trustworthy. Its
schema has reasoning: boolean and reasoning_options, but nothing about
finish_reason semantics. The one counterexample we found (LM Studio issue
#1829, a local runtime that reports stop on an internal cap) is a bug in
that runtime, not a cloud API contract. Patching OmniRoute per model would
not fix it.

What stays the same

  • finish_reason: "length" or "max_tokens" with empty content is still
    rejected, regardless of the reasoning ratio.
  • No finish_reason plus reasoning at 90% or more is still rejected. The
    ratio is the fallback for providers that omit the field.
  • Streaming responses are unaffected. The streaming peek already treats a
    non-empty thinking_delta as real content.

Test plan

  • tests/unit/combo-quality-validator-reasoning.test.ts (17 tests):
    finish_reason: "stop" plus reasoning over 90% is now valid; no
    finish_reason plus reasoning over 90% stays invalid; length and
    max_tokens stay invalid.
  • Defect injection: removing the finishReason !== "stop" guard turns
    the new test red; restoring it turns the suite green.

HouMinXi and others added 30 commits September 26, 2026 13:31
…-inference#14888)

ensureThinkingBudget lifted every positive max_tokens below 4096 up to that
floor whenever the model's output cap was larger. On Kimi K3 with effort max
the whole 4096 window went to reasoning and the reply came back empty with
finish_reason length. A budget the client chose is now left as-is; the floor
only fills in when the field is missing or non-positive.

The existing tests asserted the raise, so they now assert the value survives.
A new suite covers both token fields and the omitted-budget case.

Signed-off-by: Minxi Hou <houminxi@gmail.com>
…per-inference#14869)

supportsMaxTokens() decides whether chatCore rewrites a client's max_tokens.
Its blocklist named gpt-5.4 and gpt-5.5 but not gpt-6, so a user-added
gpt-6-luna or gpt-6-sol (no registry targetFormat, so it stays on Chat
Completions) forwarded max_tokens and OpenAI rejected it with a 400 asking
for max_completion_tokens. Adding gpt-6 uses the same substring match the
list already uses for o1 and o3.

Signed-off-by: Minxi Hou <houminxi@gmail.com>
…heaper-inference#14715)

The flush-empty retry assigned the retry connection onto the shared
credentials object before it knew the retry response was usable. A rejected
retry kept the original response but left later usage and rate-limit writes
on the wrong account. The loop now snapshots the credentials and restores
them unless the retry is adopted.

Signed-off-by: Minxi Hou <houminxi@gmail.com>
…overy

Both override execute() without calling super.execute(), so the reactive
clamp-and-retry that cheaper-inference#14774 extracted never ran for them. An upstream 400
naming the accepted reasoning_effort enum now clamps and retries once.

glm only on the openai transport; the anthropic transport does not send the
field. cliproxyapi reuses its wire serializer so the in-memory tool maps stay
off the wire.

ninerouter, gitlab, and nlpcloud are left unwired. ninerouter returns before
any fetch when its local supervisor is down, and the other two never send
reasoning_effort.

Related to cheaper-inference#14629.

Signed-off-by: Minxi Hou <houminxi@gmail.com>
server.cjs writes SSE chunks with a bare res.write(text). A slow client
grows the socket write queue without bound. The write now waits for drain,
and a close or error during the wait releases it so a dead client cannot
park the loop.

cheaper-inference#14594 already covers pipeSSE and the inspector slice. This is the third
site Diego named on cheaper-inference#14528, which that PR does not touch.

Signed-off-by: Minxi Hou <houminxi@gmail.com>
A request refused because every connection is parked by the quota cache
returns a 429 the router built itself. Nothing upstream happened, so the
normal failure path never wrote a call_logs row and the refusal only
existed in the client terminal. The skip now records the row before the
response goes back.

Signed-off-by: Minxi Hou <houminxi@gmail.com>
chat.ts opens the provider-level breaker by name, but the combo success
path only recorded the success against the connection-scoped breaker. A
probe that returned 200 left the provider breaker HALF_OPEN, so the next
request was rejected before dispatch.

Signed-off-by: Minxi Hou <houminxi@gmail.com>
(cherry picked from commit 93f4947)
Add a Grok Bot executor with per-request tool discovery, one tunnel retry, and explicit bridge success, rejection, and missing-call outcomes. Include token refresh and executor registration so daily builds can exercise the provider path.

Signed-off-by: Minxi Hou <houminxi@gmail.com>
(cherry picked from commit 54edf1641c83631b80f50a257c64666caf2f6d5e)
The Claude message shape already treats stop_reason max_tokens with no
visible text as a valid truncated completion (cheaper-inference#12968): a thinking model
can burn a one-token probe budget before emitting anything. Translating
that body to chat completions maps the stop reason to finish_reason
"length", and both the malformed-body check and the combo quality gate
then rejected it as empty, so the same request 502'd on /v1/chat and
/v1/responses while /v1/messages returned 200. A length finish with no
text now passes; a plain stop with no text stays rejected.

Signed-off-by: Minxi Hou <houminxi@gmail.com>
(cherry picked from commit ff4ad1f)
A non-streaming /v1/responses call with a small max_output_tokens on a
reasoning model returns status:"incomplete" with usable partial output.
detectMalformedNonStream and the combo quality gate only accepted
completed/done, so the body became a 502 and every combo target failed
over. incomplete, cancelled, and the SSE parser's canceled spelling now
pass through, and the sanitizer keeps incomplete_details so the client
still sees why the response stopped. failed and in-flight statuses stay
rejected.

Signed-off-by: Minxi Hou <houminxi@gmail.com>
(cherry picked from commit 821fb14)
…e#14573)

A rate limit hits ONE key, not the credential. Previously a 429 on a
provider with extra API keys still drove the connection-level
QUOTA_EXHAUSTED path, disabling the whole connection even though
sibling keys could keep serving.

- apiKeyRotator: KeyHealth gains cooldownUntil. recordKeyCooldown
  cools a single key without touching status/failures; getValidApiKey
  skips cooling keys and lands on siblings; isCooling treats an
  in-memory record as fully authoritative (a fresh success-clear must
  not be overridden by a stale DB-provided snapshot); hasEligibleKey
  is a pure predicate that never advances the round-robin index.
- keyHealth: 429 branch records the per-key cooldown (Retry-After
  honored via parseRetryAfterMs, env-tunable default otherwise);
  per-model-quota gateways mirror the 402 exemption.
- chatCore: streaming and non-streaming 429 responses forward the
  Retry-After window to the health recorder; the QUOTA_EXHAUSTED
  connection guard consults the new rateLimitConnectionGuard and
  keeps the connection active while any sibling key stays eligible.
- Tests: 24 cases in api-key-429-cooldown.test.ts, each behavioral
  change proven by defect injection (red on revert, green on fix).

Signed-off-by: Minxi Hou <houminxi@gmail.com>
(cherry picked from commit 64d7d25e1115f1ccfc903d37330779f86f4a13c2)
…-inference#14744)

In new-code mode (`--base-ref <sha>`), `withBaseWorktree` creates a temporary
worktree under `os.tmpdir()` to run ESLint on the merge-base. On macOS and other
systems where `os.tmpdir()` is a symlink (such as `/var/folders/...` pointing to
`/private/var/folders/...`), ESLint reports canonical realpaths in `entry.filePath`
while `cwd` remains the un-dereferenced symlink path.

As a result, `path.relative(cwd, entry.filePath)` in `perFileRuleCounts` produced
a traversal string (e.g. `../../private/...`) instead of a repository-relative
path, causing `baseCounts.get(file)` to always resolve to 0. Consequently, PRs
touching pre-existing files were falsely flagged as introducing all historical
violations on those files.

Resolve `cwd` and absolute `entry.filePath` via `fs.realpathSync` (falling back
gracefully to the un-resolved path if unavailable) so relative path keys match
consistently across HEAD and base reports.

Signed-off-by: Minxi Hou <houminxi@gmail.com>
(cherry picked from commit 19d69b8)
…heaper-inference#14722)

`resolvePortPid` in `src/lib/services/portProbe.ts` previously ran `lsof -ti :PORT`.
A bare `-ti :PORT` returns PIDs for both listening sockets and connected clients
(such as dashboard browser tabs, SSH tunnels, or VM/container port forwards).
On systems with lsof installed, the first PID returned could belong to an
unrelated client, leading ServiceSupervisor to adopt or signal the wrong process.

Align `lsof` with the existing `-sTCP:LISTEN` filtering used by `killByPortPosix`
in `bin/cli/commands/stop.mjs` (cheaper-inference#14605) and the ss/netstat fallback probes:
run `lsof -nP -t -iTCP:${port} -sTCP:LISTEN` to select only the listening socket.

Signed-off-by: Minxi Hou <houminxi@gmail.com>
(cherry picked from commit 4b957e6)
stream.ts and codex.ts already exceed their recorded ceilings on the
upstream tip, and the Responses terminal-state test (cheaper-inference#14894) pushes
response-sanitizer.test.ts past the new-test cap. Gate numbers only.

Signed-off-by: Minxi Hou <houminxi@gmail.com>
Carries cheaper-inference#14548. The tip added claude-opus-5-5 with its tier list but left
the plain claude-opus-5 entry without supportsReasoning or
supportedThinkingEfforts, so the static catalog still lists it with no
effort tiers. The branch commit no longer applies cleanly, so the two
lines and its test are replayed by hand.

Signed-off-by: Minxi Hou <houminxi@gmail.com>
…quest proxy

The Next.js App Router hands route handlers a Proxy around the inbound
request for dynamic "auto" routes. Passing that proxy as the Request
constructor input makes undici read #state on the proxy receiver;
ECMAScript gives a Proxy no [[PrivateFieldValues]], so every request on
/v1/chat/completions, /v1/messages and /v1/responses answered HTTP 500
with "TypeError: Cannot read private member #state ..." after cheaper-inference#14808
wired withDeadlineSignal into those routes.

Rebuild the wrapped request field-by-field (url/method/headers/body/
signal/duplex) — property reads go through the proxy get trap safely.

Adds a regression test that wraps the input in a Next-style trap and
asserts the wrap survives, with bug-injection verified (reverting to
the constructor-input form fails the test).

Signed-off-by: Minxi Hou <houminxi@gmail.com>
(cherry picked from commit 86b75f7)
Register claude-opus-5-5 on the Anthropic API, Claude OAuth, Bedrock
(anthropic.claude-opus-5-5) and Vertex catalogs with native 1M context,
128K output and the five effort tiers. The model spec marks it
adaptive-only, rejecting disabled thinking and forced tool_choice, so the
existing normalizers strip thinking.type:"disabled", collapse manual
budgets and relax any/tool choices before dispatch.

Adds $4/$20 pricing (cache read $0.20, write $5), Opus 5 family
fallback, Fast Mode defaults, static discovery, and native-1M
recognition so the context-1m beta is never forwarded for it.

(cherry picked from commit f990c02)
… claude-opus-5.5

Bedrock anthropic.claude-opus-5-5 now declares temperature/top_p/top_k as
unsupported, so the Converse translator no longer forwards them into a 400.
The Opus 5.5 spec gains a claude-opus-5.5 alias so dot-notation ids stop
prefix-matching the Opus 5 spec (which skipped the disabled-thinking and
forced tool_choice normalizers), and modelHasNativeContext1m folds dotted
versions before matching. The fast-mode test now asserts behavior instead
of list membership.

(cherry picked from commit dba8f24)
Anthropic's OAuth endpoint rejects claude-opus-5-5 with
"Claude Code 2.1.258 does not support this model; version 2.1.280 or
newer is required". Values were captured from the real 2.1.280 CLI via a
loopback listener: the version and billing revision (d7b) change; the
Stainless package (0.112.1) and runtime (v26.3.0) are unchanged. The Devin
bridge image pin stays at 2.1.258 until it is live-validated.

(cherry picked from commit eb0f8ff)
Connections whose authoritative live catalog was synced before the launch
reject claude-opus-5-5 with model_not_found until refreshed, so listing
Opus 5.5 first made ANTHROPIC_DEFAULT_OPUS_MODEL point at a model those
catalogs refuse. List it after Opus 5 until catalogs catch up.

Refs cheaper-inference#14612

(cherry picked from commit acb0e69)
Ported from cheaper-inference#14557: guards against a future pin regressing below the
2.1.280 floor Anthropic enforces for claude-opus-5-5.

Co-authored-by: PixmaNTS <maxime@maxime-pointet.fr>
(cherry picked from commit 71dec04)
Ported from cheaper-inference#14548: requests without an explicit effort get
reasoning_effort=medium; a client-chosen effort still wins.

Co-authored-by: Minxi Hou <houminxi@gmail.com>
(cherry picked from commit 592ab2a)
Co-authored-by: Minxi Hou <houminxi@gmail.com>
(cherry picked from commit 6ee27c8)
Add the Grok Bot executor, refresh path, provider registration, and weekly quota display.

Signed-off-by: Minxi Hou <houminxi@gmail.com>
(cherry picked from commit 994c22b755030dc3154adafedae99337f2f4a22d)
Add the Grok Bot executor, refresh path, provider registration, and weekly quota display.

Signed-off-by: Minxi Hou <houminxi@gmail.com>
(cherry picked from commit ad10bc0171ee131b5943c96a7249086659b5b138)
The grok-bot executor test landed at 1219 lines, nineteen over the
1200 cap for new test files. Freeze it at its landed size so the gate
holds it there instead of failing the build.

Signed-off-by: Minxi Hou <houminxi@gmail.com>
Add the Grok Bot executor, refresh path, provider registration, and weekly quota display.

Signed-off-by: Minxi Hou <houminxi@gmail.com>
(cherry picked from commit 45aaeb95d83c92d4f6b2a4d5772ba6a8995e8dcf)
Add the Grok Bot executor, refresh path, provider registration, and weekly quota display.

Signed-off-by: Minxi Hou <houminxi@gmail.com>
(cherry picked from commit 4d872a1cc39a20cafac857de0fd2a9dc68f1bea4)
Add the Grok Bot executor, refresh path, provider registration, and weekly quota display.

Signed-off-by: Minxi Hou <houminxi@gmail.com>
(cherry picked from commit beede52)
The 90% reasoning-ratio check rejects responses where reasoning fills
the budget and visible content is empty. That is correct for models
that optionally think and run out of budget mid-reasoning, but it
misfires on models that cannot disable thinking: Claude Opus 4.7+ and
GLM-5.3 both spend most of their output on reasoning and still finish
normally with finish_reason "stop".

OpenAI, Anthropic, and Z.ai all document "stop" as a natural end of
turn and "length"/"max_tokens" as truncation. Trust the stop signal
and keep the ratio check only as the fallback when the provider
reports no finish_reason.

Signed-off-by: Minxi Hou <houminxi@gmail.com>
diegosouzapw added a commit that referenced this pull request Sep 28, 2026
…ic (#15020)

Carries @HouMinXi's fix from #14980 (commit 4a03936, authorship preserved); policy change approved by the maintainer. Tests 33/33; validated in the 2026-09-28 merge-batch board.

Co-authored-by: Minxi Hou <houminxi@gmail.com>
@diegosouzapw

Copy link
Copy Markdown
Collaborator

Thank you @HouMinXi! Your change (commit 4a03936, trust finish_reason stop over the reasoning-ratio heuristic) landed on release/v3.8.51 via #15020, cherry-picked unchanged with your authorship. This PR also carried ~20 unrelated commits from your deploy branch (grok-bot tool bridge executor, Opus 5.5 defaults, rebaselines), so it couldn't be merged as a whole — please open any of those that are meant for upstream as separate PRs. Closing as carried.

@diegosouzapw diegosouzapw mentioned this pull request Sep 29, 2026
@HouMinXi
HouMinXi deleted the fix/quality-gate-stop-trust branch October 30, 2026 16:06
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.

3 participants