Skip to content

fix(agent): forward fallback_providers[].extra_body into the active request path (#26460) - #9

Open
hashbender wants to merge 1 commit into
mainfrom
mirror/pr-55892
Open

fix(agent): forward fallback_providers[].extra_body into the active request path (#26460)#9
hashbender wants to merge 1 commit into
mainfrom
mirror/pr-55892

Conversation

@hashbender

Copy link
Copy Markdown
Owner

Problem

fallback_providers entries can include OpenRouter-specific routing metadata under extra_body.provider:

fallback_providers:
- provider: openrouter
  model: z-ai/glm-5.1
  extra_body:
    provider:
      order: [baidu/fp8, gmicloud/fp8]
      allow_fallbacks: false

When the fallback was activated, _try_activate_fallback swapped the client, model, provider, api_mode, and credential pool — but never read the entry's extra_body. The request-build path assembles _prefs (which becomes extra_body.provider in the OpenRouter request) from agent-level attributes (providers_order, providers_allowed, …) that are only set from the primary config. The fallback-local routing directives were silently dropped on every fallback request.

This fixes NousResearch#26460. Previous attempts (NousResearch#26483, NousResearch#26492, NousResearch#26517) were all closed due to merge conflicts with a rapidly moving main. This PR is rebased on current main (30 Jun 2026).

Shoutout to @fleps for flagging that all three PRs ended up closed and the bug was still live — that's what prompted this fresh attempt.

Fix

Three small, targeted changes:

1. try_activate_fallback (chat_completion_helpers.py) — store fb.get("extra_body") on agent._fallback_extra_body when activating:

agent._fallback_activated = True
agent._fallback_extra_body = fb.get("extra_body") or None

2. _prefs assembly (chat_completion_helpers.py) — merge _fallback_extra_body["provider"] into _prefs when a fallback is active. Fallback-local directives take precedence over global ones because they are explicitly scoped to that fallback target:

_fb_extra_body = getattr(agent, "_fallback_extra_body", None) or {}
_fb_provider_prefs = _fb_extra_body.get("provider") if isinstance(_fb_extra_body, dict) else None
if _fb_provider_prefs and isinstance(_fb_provider_prefs, dict):
    _prefs.update(_fb_provider_prefs)

3. Clear _fallback_extra_body in both deactivation paths (restore_primary_runtime and the 429-recovery reset in conversation_loop.py) so the previous fallback's routing doesn't leak into the primary's next request.

Verification

# Run the new regression tests
python -m pytest -o addopts= tests/run_agent/test_26460_fallback_extra_body.py -v

# Run the existing fallback suite to confirm no regressions
python -m pytest -o addopts= tests/run_agent/test_provider_fallback.py -q
python -m pytest -o addopts= tests/run_agent/test_provider_parity.py::TestProviderRouting -q

# Static checks
python -m py_compile agent/chat_completion_helpers.py agent/agent_runtime_helpers.py agent/conversation_loop.py tests/run_agent/test_26460_fallback_extra_body.py
python -m ruff check agent/chat_completion_helpers.py agent/agent_runtime_helpers.py agent/conversation_loop.py tests/run_agent/test_26460_fallback_extra_body.py
git diff --check

Files changed

agent/chat_completion_helpers.py                      — store + merge fallback extra_body (27 lines)
agent/agent_runtime_helpers.py                        — clear on restore (1 line)
agent/conversation_loop.py                            — clear on 429-recovery reset (1 line)
tests/run_agent/test_26460_fallback_extra_body.py     — 9 new regression tests

Mirror-of: NousResearch#55892
NousResearch#55892

@tenki-reviewer

tenki-reviewer Bot commented Jun 30, 2026

Copy link
Copy Markdown

Review Complete

Files Reviewed: 4
Findings: 2

By Severity:

  • 🟠 High: 1
  • 🟢 Low: 1

This PR adds _fallback_extra_body to carry provider routing directives through fallback chains, but leaves two critical gaps: switch_model() fails to clear the stale fallback body (leaking routing into new model calls), and primary request_overrides silently overwrite fallback routing in the transport layer.

Files Reviewed (4 files)
agent/agent_runtime_helpers.py
agent/chat_completion_helpers.py
agent/conversation_loop.py
tests/run_agent/test_26460_fallback_extra_body.py

@tenki-reviewer tenki-reviewer Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Risk: 🟠 High (75/100) — 1 high finding, 1 low · 256 LOC across 4 files


Summary

This PR introduces _fallback_extra_body in agent/agent_runtime_helpers.py to propagate provider-specific routing directives (e.g. OpenRouter order, allow_fallbacks) through fallback chains. Three findings were identified:

High Severity

  • finding-002: switch_model() in agent/agent_runtime_helpers.py:1886 resets _fallback_activated and _fallback_index but omits _fallback_extra_body. Since build_api_kwargs() reads _fallback_extra_body unconditionally, stale fallback routing leaks into the new primary model's API calls after /model switch. Reachable via CLI, gateway, and TUI.

  • finding-003: When primary uses custom_providers with extra_body.provider routing, try_activate_fallback() in agent/chat_completion_helpers.py:1291 never clears agent.request_overrides. The transport layer (agent/transports/chat_completions.py:543-570) applies request_overrides after provider_preferences, so the primary's routing silently overwrites the fallback's.

Low Severity

  • finding-001: Dead import of _build_api_kwargs_for_openai at tests/run_agent/test_26460_fallback_extra_body.py:125 — imported but never called in that test.


def test_provider_order_forwarded_to_prefs(self):
"""provider.order from extra_body must appear in _prefs after activation."""
from agent.chat_completion_helpers import _build_api_kwargs_for_openai

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟢 Unused import in fallback extra_body test (suggestion)

In tests/run_agent/test_26460_fallback_extra_body.py at line 125, the function _build_api_kwargs_for_openai is imported from agent.chat_completion_helpers inside test_provider_order_forwarded_to_prefs() but is never used anywhere in the test. The test at lines 136-148 manually reconstructs the _prefs dict instead of calling this function. The import is dead code.

💡 Suggestion: Remove the unused import: delete line 125 from agent.chat_completion_helpers import _build_api_kwargs_for_openai.

📋 Prompt for AI Agents

In tests/run_agent/test_26460_fallback_extra_body.py on line 125, delete the line from agent.chat_completion_helpers import _build_api_kwargs_for_openai. This import is unused — the test manually assembles _prefs rather than calling this function. Remove it to clean up dead code.

# are silently dropped because _prefs is assembled from agent-level
# attributes (providers_order, providers_allowed, …) that are never
# updated when the fallback is activated. See issue #26460.
agent._fallback_extra_body = fb.get("extra_body") or None

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟠 Primary request_overrides.extra_body.provider silently overwrites fallback routing in transport layer (bug)

When a primary provider is configured via custom_providers with extra_body.provider routing (e.g. OpenRouter provider.order/allow_fallbacks), agent.request_overrides["extra_body"] carries that routing into the fallback. The transport layer's profile path (agent/transports/chat_completions.py:543-570) assembles extra_body from the fallback's provider_preferences (which correctly includes _fallback_extra_body.provider via the PR's new merge at chat_completion_helpers.py:749-752) — but then applies request_overrides afterwards at line 564-570, doing a shallow extra_body.update(v) that overwrites the fallback's routing with the primary's. Since try_activate_fallback (chat_completion_helpers.py:1277-1291) never clears or scopes down agent.request_overrides, the primary's routing silently persists and defeats the fallback's carefully configured provider directives.

💡 Suggestion: During fallback activation in try_activate_fallback() (chat_completion_helpers.py:~1291), clear or scope down agent.request_overrides to remove extra_body — or at minimum drop the provider key from request_overrides['extra_body'] — so the primary's provider routing does not overwrite the fallback's. Alternatively, apply request_overrides before provider_preferences in the transport so fallback-specific routing always takes precedence.

📋 Prompt for AI Agents

In agent/chat_completion_helpers.py, after line 1291 where _fallback_extra_body is set, add code to neutralize the primary's request_overrides.extra_body so it doesn't overwrite fallback routing. The cleanest approach: if agent.request_overrides contains extra_body, either clear request_overrides['extra_body'] entirely or pop the provider key from it. This ensures the transport layer in agent/transports/chat_completions.py:568 doesn't silently overwrite the fallback's provider_preferences routing with the primary's stale routing directives.

hashbender pushed a commit that referenced this pull request Jul 27, 2026
…e_check_xsrf pitfalls

Add two pitfalls discovered when running the skill against a fresh
Jupyter server:

- Pitfall #9: When the websocket reply channel hangs on every execute
  even though the kernel actually ran (REST shows execution_state=idle
  and execution_count increments), force zmq transport with
  --transport zmq. The zmq transport uses jupyter_client directly and
  sidesteps the broken websocket layer.

- Pitfall #10: A fresh ServerApp rejects POST /api/sessions with
  "_xsrf argument missing from POST" unless you start it with
  --ServerApp.disable_check_xsrf=True. Needed for REST-only flows
  where no browser/cookie is establishing the XSRF token.
hashbender pushed a commit that referenced this pull request Aug 4, 2026
- tests/agent/test_session_activity.py asserts against
  ACTIVITY_DESCRIPTION_MAX instead of the literal 120.
- The session-stall WARNING log line names its config knob
  (agent.session_stall_timeout) so operators can find the setting.
- hermes_state.py: collapse the triple blank line near line 191.
- hermes_cli/status.py no longer imports the private
  hermes_cli.main._relative_time: the helper moved to a public home
  (hermes_cli.timefmt.relative_time); main._relative_time stays as a
  thin back-compat wrapper (sessions_cmd and external patchers keep
  working).
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.

[Bug]: OpenRouter fallback entry ignores fallback-local provider routing metadata

1 participant