Skip to content

fix(combo): stop spurious universal-handoff on same-request fallback cascades - #12227

Closed
hartmark wants to merge 3 commits into
diegosouzapw:release/v3.8.51from
hartmark:fix/handoff-note-english
Closed

hartmark wants to merge 3 commits into
diegosouzapw:release/v3.8.51from
hartmark:fix/handoff-note-english

Conversation

@hartmark

Copy link
Copy Markdown
Contributor

Summary

Fixes a real production incident: a Swedish-language conversation got a Spanish reply from the model for no apparent reason. Root-caused via correlation_id in call_logs down to two compounding bugs in the universal-handoff mechanism, both in the same code family.

Related Issues

  • Not filed as an issue first; found and root-caused live while debugging a reported wrong-language reply.

What was actually happening

One client request, traced via a shared correlation_id:

  1. deepseek-v4-flash-free (a combo step) failed with [400] Model is unavailable — never reaches the client, purely internal.
  2. The combo's usual next step (big-pickle) wasn't retried this time; it fell through to an openrouter free-tier model instead.
  3. Because that landed model differed from what succeeded on the session's previous turn, the universal-handoff mechanism treated this as a genuine cross-turn handoff: it injected a role: system "you are being handed off" note into the wire request, and separately kicked off a background LLM call to generate/persist a conversation summary for future handoffs.
  4. The no-summary fallback note (buildUniversalHandoffSystemMessage's bare branch) was hardcoded in Spanish while every other string in the file is English — a plain copy/paste or authoring mistake, unnoticed since this specific fallback branch apparently isn't exercised by existing tests in a way that would surface it as wrong.
  5. The model, given a system-role instruction with Spanish text mixed into an otherwise-Swedish conversation, replied in Spanish.

Root cause and fix

Bug 1 (language): buildUniversalHandoffSystemMessage in open-sse/services/contextHandoff.ts — the no-summary-payload branch's <note> was hardcoded in Spanish ("A continuación se resume toda la conversacion para continuar sin perder el hilo."). Every sibling function/branch in this file uses English. Fixed to English, matching the rest of the file.

Bug 2 (the actual trigger, in combo.ts): the universal-handoff injection (and the paired maybeGenerateUniversalHandoff background-summary call) compares modelStr against getLastSessionModel() — the model that succeeded on the previous turn — on every combo target attempt, including fallback steps (i > 0) within the same client request. A same-request fallback landing on a different model than last turn's isn't a genuine handoff — the client's own request never exposed the earlier step's failure, so there's no discontinuity to explain to the model. The code already has the right signal to distinguish this: i (targetIndex) is used a few lines above to gate fallback-compression (i > 0), but the handoff blocks never checked it.

Fixed both handoff call sites (injection + background-summary generation) to only fire when i === 0 (the combo's primary target). recordSessionModelUsage bookkeeping stays unconditional — it needs to reflect whichever model actually served the response, since the next request's i === 0 comparison depends on that being accurate.

User Impact

  • Any combo whose primary target has an unreliable step ahead of a stable fallback (this combo's deepseek-v4-flash-free has failed 100% of ~1200+ attempts over the past week) was previously prone to spurious handoff noise on ordinary requests: an extra, unnecessary background LLM call per landed-on-a-different-fallback request, plus a system-role message injected into the wire request that could steer the model's behavior/language away from the actual conversation — exactly what happened here.
  • Genuine cross-turn account/session handoffs (the feature's actual purpose) are unaffected: a single-target combo's only attempt is always i === 0, and a multi-target combo's primary-target attempt on a fresh request is also i === 0.
  • Existing Spanish text, if it had ever reached a payload-bearing branch, is now English regardless.

Validation

  • Change type: provider / routing
  • Focused tests: tests/unit/universal-handoff.test.ts (23/23), tests/unit/context-handoff.test.ts, tests/integration/combo-matrix/context-relay-handoff.test.ts — the one test exercising a genuine cross-turn handoff uses a single-target combo (always i === 0), confirmed unaffected by this change (passes identically with and without the fix).
  • npm run lint (scoped: eslint on the three touched files) — clean; no new findings on touched lines.
  • Reconciled with upstream/release/v3.8.51 tip at branch time.
  • Production-code changes include an updated automated test in this PR (universal-handoff.test.ts's assertion, which previously locked in the Spanish text as expected output).

One pre-existing, unrelated integration-test flake (context-relay-handoff.test.ts's "does NOT fire when x-omniroute-session-id header is absent" case, a 503 from the combo's own retry-exhaustion path) reproduces identically on unmodified upstream/release/v3.8.51 — confirmed via git stash before/after. Not caused by this change.

Tests Added Or Updated

  • tests/unit/universal-handoff.test.ts: updated the assertion in "buildUniversalHandoffSystemMessage basic when payload summary empty" to check for the corrected English text instead of the Spanish string it previously locked in as expected behavior.

Coverage Notes

  • combo.ts's i === 0 gating isn't independently unit-tested here — the existing context-relay-handoff.test.ts coverage only exercises the single-target (i === 0 always) path. Adding a genuine multi-target fallback-cascade test for this exact scenario would need a fuller combo-matrix harness setup (scripted step failure + fallback success + differing prior-session model); flagging as a good follow-up rather than expanding this PR's scope, since the fix itself is a minimal, precisely-targeted one-line-condition change at both call sites, mirroring an already-tested pattern (i > 0 fallback-compression gating a few lines above).

Reviewer Notes

  • Verified live on a real deployment: identical Spanish-language fallback message reproduced from real call_logs/transcript data (session/correlation IDs available on request, redacted from this description).
  • The i === 0 gate is the minimal, correctly-scoped fix; alternative designs (e.g., tracking a per-request "already attempted a different model this request" flag) would be more invasive for the same outcome.

…cascades

Two bugs, found together while diagnosing a real-world Spanish-language
reply from a Swedish conversation.

1. buildUniversalHandoffSystemMessage's no-summary fallback note was
   hardcoded in Spanish while every other string in this file is English.
   This is a role:system message sent verbatim to whatever model the
   handoff lands on -- a non-English instruction here silently steers the
   model's reply language regardless of what the user was actually
   speaking.

2. The deeper cause of why a handoff fired at all for an ordinary combo
   fallback: the universal-handoff injection (and the background
   maybeGenerateUniversalHandoff summary call) compared modelStr against
   getLastSessionModel() -- the previous TURN's model -- on every combo
   target attempt, not just the primary one. A same-request fallback step
   (i > 0) landing on a different model than the last turn's is completely
   internal routing the client's own request never exposed as a failure;
   it isn't a genuine cross-turn account/session handoff. Traced via
   correlation_id in call_logs: one client request, deepseek-v4-flash-free
   failed (400, never reached the client), the combo skipped straight to
   an openrouter free-tier fallback without retrying the usual step, and
   because that landed model differed from last turn's, the handoff fired
   -- generating a real background LLM summary call and injecting the
   (Spanish) transfer note into the wire request, right before the model
   replied in Spanish.

Gated both call sites to i === 0 (the combo's primary target), matching
the i > 0 signal already used a few lines above for fallback-compression
gating. recordSessionModelUsage bookkeeping stays unconditional -- it
needs to reflect whichever model actually served the response so the
next request's i === 0 comparison has accurate ground truth.

Verified: the one single-target combo test that exercises a genuine
cross-turn handoff (context-relay-handoff.test.ts) is unaffected since a
single-target combo's only attempt is always i === 0.
@hartmark
hartmark marked this pull request as ready for review August 31, 2026 20:57
@hartmark
hartmark requested a review from diegosouzapw as a code owner August 31, 2026 20:57
…English handoff note

The i === 0 gate suppressed universal handoff on same-request fallback
cascades, contradicting the contract pinned by
tests/unit/combo-context-relay.test.ts (a fallback landing on a different
model than the session's last turn injects exactly one retargeted handoff).
The cost concern behind the gate is tracked in a follow-up issue instead.
hartmark added a commit to hartmark/OmniRoute that referenced this pull request Sep 1, 2026
…off on same-request fallback cascades) into dev/omniroute-dev-combined
hartmark added a commit to hartmark/OmniRoute that referenced this pull request Sep 1, 2026
…off on same-request fallback cascades) into dev/omniroute-dev-combined
Co-authored-by: diegosouzapw <8016841+diegosouzapw@users.noreply.github.com>
hartmark added a commit to hartmark/OmniRoute that referenced this pull request Sep 1, 2026
…off on same-request fallback cascades) into dev/omniroute-dev-combined
hartmark added a commit to hartmark/OmniRoute that referenced this pull request Sep 1, 2026
…off on same-request fallback cascades) into dev/omniroute-dev-combined
hartmark added a commit to hartmark/OmniRoute that referenced this pull request Sep 1, 2026
… targets

A same-request fallback target (i > 0 within the same client request)
serves the SAME request the failed primary target would have served, with
the original messages already intact -- the client never saw the earlier
target fail, so there is no genuine handoff to explain. The universal-
handoff mechanism previously compared every attempt (including same-
request fallbacks) against the session's PREVIOUS TURN model, injecting a
context-free note whenever they differed. That replaces real conversation
content with a note claiming continuity the request doesn't provide.

Observed live: a same-request fallback landing on a weak free-tier model,
handed this note plus only trimmed tool-output input (no real history),
fabricated an entirely invented response about an unrelated topic instead
of just answering the actual request.

Gates both handoff call sites (injection + background-summary trigger) on
i === 0. recordSessionModelUsage bookkeeping stays unconditional -- it
must reflect whichever model actually served the response, since the
next request's i === 0 comparison depends on that being accurate.

This restores the intent of an earlier attempt at this same fix (ffbd069,
part of diegosouzapw#12227) that was reverted because it broke a test asserting the
OLD behavior was correct; that test's premise conflated same-request
fallback with genuine cross-turn provider switches. Updated it to assert
the corrected contract: a same-request fallback target receives the
original request unmodified, with zero handoff messages injected.
hartmark added a commit to hartmark/OmniRoute that referenced this pull request Sep 1, 2026
…off on same-request fallback cascades) into dev/omniroute-dev-combined
@hartmark

hartmark commented Sep 1, 2026

Copy link
Copy Markdown
Contributor Author

Closing as superseded by #12338.

This PR's actual shipped diff is only the Spanish→English note-text fix (open-sse/services/contextHandoff.ts + tests/unit/universal-handoff.test.ts) — the title's claimed behavior (stopping the spurious same-request-fallback trigger, commit ffbd0699) was reverted by this PR's own second commit (058c430a) because it broke combo-context-relay.test.ts's then-correct contract.

#12338 fully subsumes this PR:

  • The same note is fixed there too (rewritten further, to also stop claiming continuity the request doesn't provide — not just translated).
  • The actual same-request-fallback trigger fix this PR's title describes is implemented there properly, with the blocking test rewritten to assert the corrected contract instead of the old one.
  • Plus the silent-failure diagnostic logging that traced this whole bug family live.

No content from this PR is lost — closing without merging.

@hartmark hartmark closed this Sep 1, 2026
@hartmark
hartmark deleted the fix/handoff-note-english branch September 1, 2026 18:58
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