Skip to content

fix(sse): exempt tiny-budget reasoning probes and trace persisted-cooldown skips (#12659) - #13269

Merged
diegosouzapw merged 1 commit into
release/v3.8.51from
fix/12659-combo-skip-reasons
Sep 12, 2026
Merged

diegosouzapw merged 1 commit into
release/v3.8.51from
fix/12659-combo-skip-reasons

Conversation

@diegosouzapw

Copy link
Copy Markdown
Owner

Closes #12659

Root cause

Two independent gaps in open-sse/services/combo/:

  1. Tiny-budget reasoning probes poisoned the model lockout. validateResponseQuality (the combo quality gate) had no way to distinguish a deliberate tiny-budget capability probe (e.g. max_tokens: 1) from a genuine large-budget reasoning exhaustion, so both hit the same reasoning truncated at token limit / reasoning consumed N/M tokens invalid-quality branches — producing a 502 and a recordModelLockoutFailure("quality_failure", 502, ...) call for what was really a capability ping.
  2. Persisted-cooldown skips were untraced. executeTargetGates.ts's persisted-connection-cooldown skip branch never called recordComboDecision, persisted_cooldown was not even an allowlisted ComboSkipReason, and buildComboDiag()'s excluded[] only ever sourced from exhaustedProviders/exhaustedConnections — so an ALL_TARGETS_SKIPPED caused purely by persisted cooldowns surfaced as an opaque attempted=0, excluded=[].

Fix

  1. validateResponseQuality now exempts a reasoning-truncated response whose actual completion_tokens sits below the fix(api): answer tiny-budget reasoning probes (max_tokens:1) with truncated 200 — no 500 empty response, no cooldown poisoning #10281 tiny-budget threshold (REASONING_BUFFER_MIN_TRIGGER = 256, reused from reasoningTokenBuffer.ts rather than duplicating the constant) — since completion_tokens can never exceed the caller's max_tokens, a tiny count proves a tiny budget was requested without needing to thread the request body through every combo dispatch call site. The exemption returns valid: true, which passes the original upstream 200 straight through (already the correct fix(api): answer tiny-budget reasoning probes (max_tokens:1) with truncated 200 — no 500 empty response, no cooldown poisoning #10281 truncated-200 shape: 200, empty content, finish_reason: length) — no synthetic response needed, and the caller's existing "only lock out on !quality.valid" branch means no model-lockout call happens for a probe.
  2. decisionTrace.ts adds persisted_cooldown to the COMBO_SKIP_REASONS allowlist, an optional non-secret detail field on ComboTraceEntry, and a new pure summarizeSkippedTargets() helper that groups skip decisions by reason.
  3. executeTargetGates.ts's persisted-cooldown skip branch now calls recordComboDecision(..., reason: "persisted_cooldown").
  4. comboAttemptLoop.ts's buildComboDiag surfaces a new skippedTargets[] field (via summarizeSkippedTargets) on the all_targets_skipped diagnostics body. ComboDiagnostics/sanitizeComboDiagnostics in open-sse/utils/error.ts gained the matching optional field, sanitized (length + count capped) exactly like every other diagnostics field — the whole body still goes through buildErrorBody(), never raw err.stack/err.message.

Scope note

The upstream issue's checklist also asks for (a) wiring the same persisted-cooldown trace into roundRobinCombo.ts's dispatch path and (b) threading max_tokens/model context into validateResponseQuality's call sites. I verified by reading source that roundRobinCombo.ts never calls resolvePersistedConnectionCooldownSkipReason at all (round-robin has no persisted-cooldown gate to wire), so there is no equivalent gap there today. For (b), the self-contained completion_tokens-based heuristic reuses the existing regression-test suite's own resolution scale (all #3587 exhaustion cases sit at 512+ tokens, the reported probe at 10) without touching executeTargetAttempt.ts (frozen at its exact LOC baseline) or roundRobinCombo.ts (1 line of file-size headroom) — both zero free bytes I did not want to spend given they're unrelated to the confirmed root cause.

Regression test — RED → GREEN

tests/unit/combo-quality-tiny-budget-probe.test.ts (mirrors the plan's proven repro, the issue's exact reported shape: reasoning consumed 10/10 tokens, finish_reason: length):

  • RED (before fix): AssertionError: reproduces #12659: combo validator rejected the tiny-budget probe as a genuine quality failure instead of exempting it like #10281 does on the direct path (reason: reasoning truncated at token limit (finish_reason: length) — no content output)
  • GREEN (after fix): valid: true.

tests/unit/combo/combo-skipped-targets-summary.test.ts (new): persisted_cooldown allowlisting, summarizeSkippedTargets grouping/edge-cases, and an explicit assertion that the diagnostics body carries no stack-trace frame and no connection/account-id fragment.

Gates run

  • node scripts/check/check-file-size.mjs → OK (138 frozen files, cap 1200 — no growth on touched files)
  • node scripts/check/check-complexity.mjs → OK (2798 violations vs baseline 3218)
  • node scripts/check/check-cognitive-complexity.mjs → OK (1265 violations vs baseline 1437)
  • npm run typecheck:core → exit 0
  • npx eslint --suppressions-location config/quality/eslint-suppressions.json <changed files> → clean
  • npx prettier --check <changed files> → clean
  • New tests: node --import tsx/esm --test tests/unit/combo-quality-tiny-budget-probe.test.ts tests/unit/combo/combo-skipped-targets-summary.test.ts → 6/6 pass
  • Existing area regression suites (all pass, 129 tests total): tests/unit/combo-quality-validator-reasoning.test.ts (16), tests/unit/combo/combo-decision-trace.test.ts + tests/unit/combo/execute-target-gates.test.ts (15), tests/unit/combo-diagnostics-trace.test.ts (6), tests/unit/combo-stream-readiness-fallback.test.ts + tests/unit/combo-responses-sse-failure-fallback.test.ts + tests/unit/masked-200-exhaustion-fallback-6427.test.ts + tests/unit/combo-terminal-status-policy-10501.test.ts + tests/unit/9303-recovery-hint-all-targets-skipped.test.ts + tests/unit/combo-cooldown-retry.test.ts (55), tests/unit/9303-recovery-hint-all-targets-skipped.test.ts + tests/unit/combo-routing-engine.test.ts (92, overlaps counted once)
  • node scripts/check/check-changelog-integrity.mjs → OK

⚠️ base-red inherited: #12732 — unit #12058, integration codex-cache, package-artifact, tarball-smoke, agent-skills-sync

Note

The two PRs the original issue linked as carrying the fix (#12531 and its combo-skip-diagnostics branch, #12661) were both closed without merging — this PR is a fresh implementation pass against the current release/v3.8.51 tip.

…ldown skips (#12659)

- validateResponseQuality (combo quality gate) now exempts a reasoning
  truncation whose completion_tokens is below the #10281 tiny-budget
  threshold (256), passing the original 200 through instead of failing
  quality and poisoning the model lockout for a deliberate capability
  probe.
- executeTargetGates.ts records a persisted_cooldown decision on the
  decision trace (new allowlisted ComboSkipReason); comboAttemptLoop's
  buildComboDiag surfaces it via a new skippedTargets[] field on the
  ALL_TARGETS_SKIPPED diagnostics body, sanitized through
  sanitizeComboDiagnostics like every other diagnostics field.
@diegosouzapw
diegosouzapw merged commit 2cb02d2 into release/v3.8.51 Sep 12, 2026
15 of 21 checks passed
Githab-capibara added a commit to Githab-capibara/OmniRoute that referenced this pull request Sep 17, 2026
…ldown skips (diegosouzapw#12659) (diegosouzapw#13269)

Merged as part of the 39-PR owner batch of 2026-09-11, validated as a unit.

Boarded into one consolidated worktree cut from `release/v3.8.51` with the other 38 — zero conflicts between them.

- ESLint over every changed file: no errors (the only finding was one suppression entry the batch emptied, pruned on diegosouzapw#13243)
- `typecheck:core` clean; `check:dashboard-typecheck` OK (206 pre-existing, within baseline); `check:changelog-integrity` OK
- complexity 2821 / baseline 3218 and cognitive-complexity 1272 / baseline 1437 — both under baseline
- 256 assertions green: 246 under node:test and 10 under vitest, which is where `tests/unit/**/*.test.tsx` actually runs
- `check-file-size`: `chatCore.ts` rebaselined 6144 → 6146 for diegosouzapw#13278 and diegosouzapw#13276, annotated and landed on diegosouzapw#13243

⚠️ base-red inherited: diegosouzapw#12732 — the provider count (356 in the docs vs the 358 the modules define) and `open-sse/utils/stream.ts` at 3115 > frozen 3098 both reproduce on the pure tip with zero contribution from this batch.
muhamadgalihsaputra pushed a commit to niyatna/NiyatnaRoute that referenced this pull request Sep 27, 2026
…ldown skips (diegosouzapw#12659) (diegosouzapw#13269)

Merged as part of the 39-PR owner batch of 2026-09-11, validated as a unit.

Boarded into one consolidated worktree cut from `release/v3.8.51` with the other 38 — zero conflicts between them.

- ESLint over every changed file: no errors (the only finding was one suppression entry the batch emptied, pruned on diegosouzapw#13243)
- `typecheck:core` clean; `check:dashboard-typecheck` OK (206 pre-existing, within baseline); `check:changelog-integrity` OK
- complexity 2821 / baseline 3218 and cognitive-complexity 1272 / baseline 1437 — both under baseline
- 256 assertions green: 246 under node:test and 10 under vitest, which is where `tests/unit/**/*.test.tsx` actually runs
- `check-file-size`: `chatCore.ts` rebaselined 6144 → 6146 for diegosouzapw#13278 and diegosouzapw#13276, annotated and landed on diegosouzapw#13243

⚠️ base-red inherited: diegosouzapw#12732 — the provider count (356 in the docs vs the 358 the modules define) and `open-sse/utils/stream.ts` at 3115 > frozen 3098 both reproduce on the pure tip with zero contribution from this batch.
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.

fix(combo): ALL_TARGETS_SKIPPED must carry per-target skip reasons; tiny-budget reasoning probes should return truncated 200s

1 participant