Skip to content

fix(sse): trust finish_reason over reasoning-ratio heuristic in response quality validation - #12262

Merged
diegosouzapw merged 2 commits into
diegosouzapw:release/v3.8.51from
brick30llc-ctrl:fix/reasoning-truncation-guard
Sep 1, 2026
Merged

diegosouzapw merged 2 commits into
diegosouzapw:release/v3.8.51from
brick30llc-ctrl:fix/reasoning-truncation-guard

Conversation

@brick30llc-ctrl

Copy link
Copy Markdown
Contributor

Summary

validateResponseQuality()'s reasoning-truncation guard (added for #3587) only rejects a
response with empty content + present reasoning_content when reasoning consumed 90%+
of completion_tokens. A response truncated at a lower ratio slips through as "valid" even
though the caller gets nothing usable — and the response already carries an unambiguous
signal that was going unread: finish_reason: "length" (or the "max_tokens" naming some
providers use).

Reproduced live against nvidia/nemotron-3-super-120b-a12b on a self-hosted deployment:
content: null, finish_reason: "length", reasoning_tokens: 645 / completion_tokens: 1024
(63% — below the 90% threshold). The combo loop treated it as a successful completion and
never retried, so the end client received an empty answer for what looked like a normal,
non-degraded 200 response.

Fix

When content is empty and reasoning_content is present (the existing precondition for
this whole branch), check finish_reason first: "length" or "max_tokens" is trusted
directly as truncation, independent of the token ratio. The existing ratio heuristic remains
as a fallback for providers that don't report finish_reason reliably. Does not touch or
weaken the deliberate-tiny-probe case (e.g. max_tokens: 1 connectivity pings, see
errorClassifier.ts's LEGIT_EMPTY_OPENAI_FINISH) — those never produce reasoning_content
at all, so this branch's precondition already excludes them.

Test plan

  • 4 new unit tests in tests/unit/combo-quality-validator-reasoning.test.ts: the exact
    63%-ratio repro (now invalid), the max_tokens naming variant, a regression guard that
    no-finish_reason + <90% ratio stays valid (unchanged [BUG] Reasoning models in combos consume all tokens for reasoning_content, leaving content empty #3587 behavior), and a check that
    finish_reason: "stop" doesn't short-circuit the ratio heuristic.
  • node --import tsx/esm --test tests/unit/combo-quality-validator-reasoning.test.ts —
    16/16 pass (12 pre-existing + 4 new).
  • Adjacent quality-validation suites unaffected: validate-response-quality.test.ts (17),
    quality-validation-benign-error.test.ts, routing-quality.test.ts,
    routing-scoring-quality.test.ts, quality-rail-gate-membership.test.ts (23) — all pass.
  • npm run typecheck:core — only pre-existing, unrelated omniglyph errors (confirmed
    present on the base branch too, nothing from the changed files).
  • eslint on both changed files — clean.

…tio heuristic in response quality validation

A truncated response with empty content and reasoning_content present was
only rejected by validateResponseQuality() when reasoning consumed >=90%
of completion_tokens. A response truncated at a lower ratio (e.g. 63%)
passed through as "valid" even though the caller received no usable
content and finish_reason was explicitly "length" (or the alternate
"max_tokens" naming some providers use) -- an unambiguous truncation
signal the validator wasn't reading. Reproduced live against
nvidia/nemotron-3-super-120b-a12b: content:null, finish_reason:length,
reasoning_tokens 645/1024 (63%).

Trust finish_reason directly when it's reported, falling back to the
existing token-ratio heuristic only when it isn't. Does not affect the
deliberate-tiny-probe case (e.g. max_tokens:1 connectivity pings) --
those never produce reasoning_content, so the branch this change is in
doesn't run for them.
@diegosouzapw

Copy link
Copy Markdown
Owner

Validated in local merge-train on 192.168.0.113 — train of #12258 #12262 #12166 #12281 #11259 #11950 merged clean onto origin/release/v3.8.51 (f5e7095):

  • Run 1 (/opt/actions-runner-omniroute-5, train tip 85cd9119): typecheck:core, file-size, complexity ×2, changelog-integrity — all green; the test:unit step was killed by a CI job landing on that runner mid-train (workspace clobbered — infra, documented risk).
  • Run 2 (/srv/omniroute-train/.claude/worktrees/mt-green1, same 6 PRs re-boarded, fresh npm ci): unit 35768/35805 pass (/tmp/mt-unit2.log), vitest 464/465 (/tmp/mt-vitest2.log).
  • Every failure is a latency/timing assert (bounded-time, event-loop lag, cooldown windows) on a box that was never idle (3 active CI runner workers throughout). Discriminated per merge-gates §3: the 3 persistent titles reproduce identically on the pure base tip on the same box (/tmp/mt-base-isolated.log, BASE_ISOLATED_EXIT=1 — same tests, same asserts), 5 more titles reproduce on the pure base on the devbox, and the single vitest failure (provider-family-combos) also fails on main's nightly without any of these PRs. Inherited/infra — not introduced by this train.

@diegosouzapw
diegosouzapw merged commit 0ff1647 into diegosouzapw:release/v3.8.51 Sep 1, 2026
13 of 16 checks passed
muhamadgalihsaputra pushed a commit to niyatna/NiyatnaRoute that referenced this pull request Sep 27, 2026
…nse quality validation (diegosouzapw#12262)

* fix(sse): trust finish_reason:length/max_tokens over the reasoning-ratio heuristic in response quality validation

A truncated response with empty content and reasoning_content present was
only rejected by validateResponseQuality() when reasoning consumed >=90%
of completion_tokens. A response truncated at a lower ratio (e.g. 63%)
passed through as "valid" even though the caller received no usable
content and finish_reason was explicitly "length" (or the alternate
"max_tokens" naming some providers use) -- an unambiguous truncation
signal the validator wasn't reading. Reproduced live against
nvidia/nemotron-3-super-120b-a12b: content:null, finish_reason:length,
reasoning_tokens 645/1024 (63%).

Trust finish_reason directly when it's reported, falling back to the
existing token-ratio heuristic only when it isn't. Does not affect the
deliberate-tiny-probe case (e.g. max_tokens:1 connectivity pings) --
those never produce reasoning_content, so the branch this change is in
doesn't run for them.

* docs(changelog): add fragment for diegosouzapw#12262

---------

Co-authored-by: brick30llc-ctrl <admin@brick30.com>
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