refactor(model-routing): centralize explicit foreground fallback policy - #6020
Conversation
Make foreground fallback an explicit per-user, availability-only policy shared by streaming Chat, non-stream Chat, and Agent runs. Preserve strict defaults, owner/model and credential boundaries, pinned Agent routes, and truthful per-round provenance/accounting. Carry provider-reported model identifiers through native streaming adapters, non-stream responses, and caches, and keep legacy default_model_fallbacks as tombstoned raw storage that generic settings APIs and agent tools cannot expose or mutate.
StressTestor
left a comment
There was a problem hiding this comment.
ran two independent review passes on this, one diffing every finding against base b522964 in a throwaway worktree, one probing the server paths at runtime. the fallback-policy design itself looks solid: owner-scoped consent, prefs migration, and credential identity all verify clean. but the rebase dropped real code on the floor, and two of those drops are ship blockers. requesting changes.
rebase casualties (blockers)
-
chat send is broken in the browser. the rebase deleted the definitions of
_touchStreamActivity,_syncForegroundStreamGlobals, and_getForegroundStreamState(they exist at baseb522964, chat.js:585-609) but left the call sites, 16+ of them. every send hits_touchStreamActivity(streamSessionId)at chat.js:1375, outside any try/catch, and throws ReferenceError before the request dispatches. also breaks_releaseSendFlag(composer stays disabled), the stop button (chat.js:4455), and the slash-command streaming check (chat.js:716). quick repro:grep -n "function _touchStreamActivity" static/js/chat.jsfinds nothing at head, finds it at base.node --checkpasses because the failure only happens at runtime. -
notes mode disables every tool, including manage_notes. the general no-tool branch got fused into the notes branch during the rebase: agent_loop.py:3960 now runs
disabled_tools.update(known_tool_names())insideelif _ody_notes_finetune_mode, so "add a note to buy milk" on odysseus-qwen3 gets the minimal notes prompt with zero callable tools. the fusion is visible in the logs, "notes finetune tool clamp" and "general no-tool clamp active" fire back to back. the inverse also regressed: general qwen turns (zero tools at base) now keep full schemas. -
calendar routing is dead code. notes_mode at agent_loop.py:3926 collapsed to
(A or B) and B, which is just B, so pure calendar phrasing ("what's on my calendar today") never routes. the calendar-followup branch from base (3527-3540,_looks_like_notes_calendar_followup+ recent-tool-context) is gone too. -
the notes clamp lost manage_calendar and manage_tasks. agent_loop.py:3944 clamps to
{manage_notes, ask_user, update_plan}; base had all three domain tools plus thedifference_updatethat re-enabled them when tool-RAG had disabled them. -
workspace rules never reach the system prompt. the sole
_build_system_promptcall at agent_loop.py:4095 dropped theworkspace=workspacekwarg from base, so_workspace_coding_rules(confinement, get_workspace orientation) is never injected. tool selection still reads workspace, so the agent gets file tools without the guardrails. -
the qwen3 temperature clamp is gone. base force-capped odysseus-qwen3 to 0.2 (base 3148-3152); that was deleted and never re-homed. the only remaining 0.2 clamp (llm_core.py:1079) is scoped to local MiniMax MLX.
bugs in the new code
-
IPv6 endpoints are never cost-tracked.
endpoint_cost_tracked(endpoint_resolver.py:58) short-circuits on"." not in hostbefore parsing, sohttp://[2606:4700::1]/v1classifies as not-cost-tracked. the private-range list also omits link-local 169.254/16, which classifies as cost-tracked.chat_helpers._is_local_hostalready handles both families correctly; reusing it fixes both cases and removes a third copy of this classification. -
a None endpoint_url turns the clean 400 into a 500. chat_routes.py:711 and 1063 use
getattr(sess, "endpoint_url", "").strip(). when the attribute exists and is None, getattr returns None and.strip()raises AttributeError. lines 403 and 461 in the same file already use(getattr(...) or "").strip(). -
PoolTimeout blocks availability fallback. llm_core.py:2493 groups
httpx.PoolTimeoutwithWriteTimeoutas fallback-ineligible, but PoolTimeout fires while waiting for a connection, before anything is sent. splitting it out keeps the fail-closed intent for ambiguous writes while letting the unambiguous case fall back. -
terminal-then-EOF clobbers the saved result. when the canonical terminal event arrives and the connection then dies, chat.js:4297 checks
isRecoverableStreamError(err)before_canonicalTerminalSaved(only consulted at 4299, inside the terminal branch), so auto-recover kicks in, resume 404s for the completed run, and "connection lost" replaces the persisted result. checking the flag first fixes it. -
rapid resend can strand the old run's SSE forever. agent_runs.py:170 cancels the previous task; when the cancel lands before
_drainfirst runs, the finally that flips status and arms eviction never executes, and a subscriber bound to the old run heartbeats forever. flipping the old run's status synchronously instart()closes it. -
stop has two dead zones. (a) the run id is only captured when response headers arrive (chat.js:1926), but the UI is stoppable before that, so a fast stop is a silent no-op (
_stopExactRunat chat.js:614 returns false without making a request). (b) only the detached-run and resume responses setX-Odysseus-Run-Id(chat_routes.py:2380/2396), so header-less paths and non-browser clients ofPOST /api/chat/stoplost the ability to cancel. probably one design pass: queue the stop until the id arrives, and decide what header-less callers should get. -
the fallback allowlist filters after slicing. foreground_model_routing.py:80 applies
entries[:MAX_FOREGROUND_FALLBACKS]before theallowed_modelsfilter, so allowed entries past position 10 get dropped and the policy can silently resolve to strict. filter first, then slice.
lower priority
- chatRenderer.js:997: the cost ledger does an unlocked whole-map localStorage read-modify-write. the comment claims two-tab safety via per-runId idempotence, but two tabs writing different runIds are last-writer-wins. worth at least fixing the comment.
- agent_loop.py:5726: persisted tool_events dropped
descand_resolved_tool_event_name(), so saved history permanently shows generic names for MCP tools. - agent_loop.py:4366 / endpoint_resolver.py:568: route descriptors get recovered and re-aligned by equality checks and index scans with silent fallbacks (stale headers degrade to a generic descriptor, identical endpoints attribute by row order). threading the known endpoint id through with the candidate removes the whole desync class. bigger refactor, fine as a follow-up.
- foreground_model_routing.py:106: the resolver-seam compat branch pairs descriptors positionally against a deduped list, so indices desync. only tests reassign the seam today, but it's a trap on a documented seam.
- llm_core.py:3441: a symbolic-only status like
{"status": "RATE_LIMITED"}returns the 400 default before the marker heuristics would map it to 429, so fallback stalls on that shape. the docstring reads like fail-closed is intentional here, so flagging rather than requesting. your call.
the meta issue
everything in the blockers section is invisible to node --check and the focused pytest additions, which is all this PR ran (the e2e checklist item is unchecked, which tracks). worth launching the app and sending one message before merge (that alone catches finding 1), or a browser smoke test in CI that does the same, or the next rebase does this again.
the two CodeQL prototype-pollution alerts look refutable btw: the dynamic keys trace to server-generated uuids (agent_runs.py:37) and never reach shared or server state.
happy to push the restoration commits for 1-6 if useful, they're mostly copy-back-from-base.
…t, and temperature clamp
…ification and fallback eligibility
| // refresh produces a fresh metrics object. The Web Lock around this | ||
| // read/modify/write also keeps distinct runs from two tabs from | ||
| // overwriting one another's stale snapshot. | ||
| sessionRuns[runId] = cost; |
| (total, entry) => total + (Number(entry[1]) || 0), | ||
| 0, | ||
| ); | ||
| overflow.forEach(([oldRunId]) => delete sessionRuns[oldRunId]); |
|
pushed the fixes to this branch, 10 commits on top of eb3a094, head cdeb4fe. everything from the review is addressed:
test story: ~30 new regression tests, including browser tests that execute the real chat.js code under node (extracted and run, which catches what deliberate scope calls, flagging them so you can veto:
the request-changes review above predates these commits. ping me if any of this needs more detail. |
Summary
Centralize explicit foreground fallback policy for Chat and Agent routing.
Fallback remains disabled by default, advances only on eligible availability
failures, preserves owner/model/credential boundaries, pins the answering Agent
route after substantive output or a completed tool call, and records truthful
per-round model/endpoint provenance and usage attribution.
This is the clean rebased replacement for #5811 on top of merged #5801. The
implementation was reconciled against current
devand the rebase regressionsfound during final audit were fixed.
Target branch
dev, notmain.Linked Issue
Fixes #5626
Supersedes #5811.
Part of #5625.
Type of Change
Checklist
devdocker compose uporuvicorn app:app) and verified the change works end-to-end.Behavior contract
default_model_fallbacksremains stored but is not used as foreground fallback policy.Validation
Final rebase audit was performed against current
dev._ody_general_no_tool_modereference removed.How to Test
requirements.txtin a clean virtual environment.prefs, and LLM fallback tests.
python -m pytest -q.python -m compileall -q core routes src.node --checkagainst the modified/new Chat JavaScript modules.may advance to an explicitly configured route.
and saved metrics identify the actual answering route.
Visual / UI changes — REQUIRED if you touched anything that renders
This changes existing Chat provenance and terminal-stream behavior rather than
introducing a new visual component or layout. Existing visual styling is
preserved. Browser-side JavaScript regression tests cover provenance, terminal
errors, background terminal state, and interrupted streams.
No new visual surface is introduced.