Skip to content

fix(agent): honor auxiliary.fallback_chain in review forks - #93664

Closed
Halldrix wants to merge 1 commit into
NousResearch:mainfrom
Halldrix:fix/bg-review-aux-fallback-chain
Closed

Halldrix wants to merge 1 commit into
NousResearch:mainfrom
Halldrix:fix/bg-review-aux-fallback-chain

Conversation

@Halldrix

Copy link
Copy Markdown
Contributor

What does this PR do?

When auxiliary.background_review.provider fails, the per-task
fallback_chain configured under auxiliary.background_review.fallback_chain
is never consulted: the review retries its primary 3 times and then dies
silently with calls=0, even though working fallback entries are configured.

Root cause: _run_review_in_thread() spawns the review fork without a
fallback_model argument, so agent._fallback_chain starts empty and
_try_activate_fallback() gives up immediately. Every other auxiliary
sub-module goes through call_llm() (_try_configured_fallback_chain)
and honors its chain — the two run_conversation()-based forks were the
only ones that ignored it.

This wires both forks through the same layering the rest of the aux tasks
get: per-task auxiliary.<task>.fallback_chain entries first, then the
top-level chain as the last-resort safety net, deduplicated by backend
identity. The user-facing documentation already promises this behavior
("Each task can also declare its own fallback_chain"), and #78371
describes exactly this wiring for the curator.

Related Issue

Fixes #93592
Fixes #78371

Type of Change

  • 🐛 Bug fix (non-breaking change that fixes an issue)

Changes Made

  • hermes_cli/fallback_config.py: add
    resolve_aux_task_fallback_chain(task, config=None) — builds the
    effective chain for an AIAgent-fork review task: normalized
    auxiliary.<task>.fallback_chain entries first, top-level chain
    (get_fallback_chain) appended as safety net, deduplicated by backend
    identity (provider+model+base_url). Entry normalization preserves
    base_url/key_env/api_key and aliases transport -> api_mode
    for parity with the call_llm fallback path, which accepts either
    spelling while _try_activate_fallback() only reads api_mode.
  • agent/background_review.py: pass the resolved chain to the review
    fork's AIAgent(...) constructor as fallback_model.
  • agent/curator.py: same wiring for the curation-pass fork.
  • tests/agent/test_aux_review_fallback_chain.py: behavior-contract
    tests — task-first ordering, cross-source dedupe (case-insensitive),
    invalid-entry tolerance, fresh-dict guarantees, entry-field
    preservation, live-config resolution, and construction-time wiring of
    both forks.

Users with no auxiliary.<task>.fallback_chain and no top-level chain get
byte-identical behavior (empty chain -> None -> empty _fallback_chain,
as before).

How to Test

  1. Targeted suites (CI-parity wrapper):

    HERMES_PYTHON=$(which python3) scripts/run_tests.sh \
      tests/agent/test_aux_review_fallback_chain.py \
      tests/hermes_cli/test_fallback_config.py
    

    -> 15 passed.

  2. Regression proof (fails on code before this PR): revert the two
    constructor hunks in agent/background_review.py / agent/curator.py
    — TestBackgroundReviewWiring::* and TestCuratorWiring::* fail;
    restore them and all pass.

  3. Manual: configure auxiliary.background_review.{provider,model} pointing
    at a failing provider plus a working fallback_chain entry; trigger a
    background review. Before: Background review complete: calls=0 ... result=none. After: the loop walks the configured entries (and then the
    top-level chain) exactly like the main agent does.

Note on the full suite: on this host, scripts/run_tests.sh shows
environment-dependent failures unrelated to this change (terminal-width and
env-gate tests such as tests/cli/test_prompt_stash_cli.py and
tests/hermes_cli/test_ignore_user_config_flags.py). Both sets were
verified to fail identically on pristine origin/main without this branch
(via stash / scratch-worktree runs); every suite touching the modified
subsystems passes.

Checklist

Code

  • I've read the Contributing Guide
  • My commit messages follow Conventional Commits (fix(scope):, feat(scope):, etc.)
  • I searched for existing PRs to make sure this isn't a duplicate
  • My PR contains only changes related to this fix/feature (no unrelated commits)
  • I've run pytest tests/ -q and all tests pass
  • I've added tests for my changes (required for bug fixes, strongly encouraged for features)
  • I've tested on my platform: Debian GNU/Linux 13 (trixie), x86_64

Documentation & Housekeeping

  • I've updated relevant documentation (README, docs/, docstrings) — or N/A
  • I've updated cli-config.yaml.example if I added/changed config keys — or N/A
  • I've updated CONTRIBUTING.md or AGENTS.md if I changed architecture or workflows — or N/A
  • I've considered cross-platform impact (Windows, macOS) per the compatibility guide — or N/A
  • I've updated tool descriptions/schemas if I changed tool behavior — or N/A

(No new config keys and no behavior beyond what the existing fallback-providers
documentation already describes, so the documentation boxes are N/A.)


🛠️ Dev: Halldrix
🤖 Sidekick: Hermes Agent v0.20.5
🐞 Reproduction: ✅ Confirmed

The background-review and curator forks spawn an AIAgent that fails over
through run_conversation()'s _try_activate_fallback(), which consults only
the constructor-provided chain. Neither fork passed fallback_model, so a
configured auxiliary.<task>.fallback_chain was silently ignored and the
review died with its primary provider (calls=0, no fallback attempts).

Add resolve_aux_task_fallback_chain() to hermes_cli/fallback_config.py:
per-task entries first, then the top-level chain as the last-resort safety
net, deduplicated by backend identity. Entries are normalized to the exact
shape agent_init expects, aliasing transport -> api_mode for parity with
the call_llm fallback path. Wire it into both fork constructors.

Fixes NousResearch#93592
Fixes NousResearch#78371
@alt-glitch alt-glitch added type/bug Something isn't working P3 Low — cosmetic, nice to have comp/agent Core agent runtime: loop, agent_init, prompt builder, context-compression, responses endpoint comp/cli CLI entry point, hermes_cli/, setup wizard area/config Config system, migrations, profiles duplicate This issue or pull request already exists labels Aug 24, 2026
@alt-glitch

Copy link
Copy Markdown
Contributor

This was generated by AI during triage.

Duplicate of #79750: both wire configured per-task and global fallback chains into the curator and background-review forks.

@Halldrix

Copy link
Copy Markdown
Contributor Author

Closing in favor of #79750, which implements the same fix and predates this PR by three weeks. Review notes posted there cover two things worth folding in: citing Fixes #93592 so the newer report auto-closes on merge, and transport -> api_mode normalization for chain entries.

@Halldrix Halldrix closed this Aug 24, 2026
@Halldrix
Halldrix deleted the fix/bg-review-aux-fallback-chain branch August 24, 2026 08:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/config Config system, migrations, profiles comp/agent Core agent runtime: loop, agent_init, prompt builder, context-compression, responses endpoint comp/cli CLI entry point, hermes_cli/, setup wizard duplicate This issue or pull request already exists P3 Low — cosmetic, nice to have type/bug Something isn't working

Projects

None yet

2 participants