Skip to content

fix(memory): use primary model for background memory/skill review - #8351

Closed
BukeLy wants to merge 2 commits into
NousResearch:mainfrom
BukeLy:fix/background-review-use-primary-model
Closed

fix(memory): use primary model for background memory/skill review#8351
BukeLy wants to merge 2 commits into
NousResearch:mainfrom
BukeLy:fix/background-review-use-primary-model

Conversation

@BukeLy

@BukeLy BukeLy commented Apr 12, 2026

Copy link
Copy Markdown

What does this PR do?

_spawn_background_review creates a review AIAgent with self.model and self.provider, inheriting the current turn's model. When smart model routing sends the turn to a cheap/weak model (e.g. a local 9B), the background review also runs on that weak model — which typically lacks the tool-calling ability needed to actually save memories or create skills.

Fix: read the primary model config (model.default, model.provider, model.base_url, model.api_key) from config.yaml for the review agent, with fallback to the current model if config is unavailable.

See also: #7491 (complementary — passes base_url/api_key from parent agent; this PR goes further by reading from config since the parent may itself be a cheap routing model)

Related Issue

Fixes #8517

Type of Change

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

Changes Made

  • run_agent.py: Modified _spawn_background_review_run_review to read primary model from hermes_cli.config.load_config() instead of inheriting self.model
  • tests/run_agent/test_background_review_model.py: New test file with 2 tests covering config-based model selection and fallback on config error

How to Test

  1. Enable smart model routing with a cheap model that cannot do tool calls
  2. Send enough messages to trigger memory nudge (nudge_interval turns)
  3. Verify background review runs on the primary (strong) model, not the cheap model
  4. Verify memories are actually saved (check ~/.hermes/memories/USER.md)

Checklist

Code

  • I've read the Contributing Guide
  • My commit messages follow Conventional Commits
  • I searched for existing PRs to make sure this isn't a duplicate
  • My PR contains only changes related to this fix
  • I've run pytest tests/ -q and all tests pass
  • I've added tests for my changes
  • I've tested on my platform: macOS 15.4 (Sequoia), Apple Silicon

Documentation & Housekeeping

  • N/A - no config keys or docs changed
  • Cross-platform: all platforms affected equally, fix is platform-independent

BukeLy added 2 commits April 12, 2026 20:13
_spawn_background_review creates a review AIAgent with self.model
and self.provider, inheriting the current turn's model. When smart
model routing sends the turn to a cheap/weak model, the background
review also runs on that weak model — which typically lacks the
tool-calling ability needed to actually save memories or create skills.

Now read the primary model config (model.default) from config.yaml
for the review agent, falling back to the current model if config
is unavailable. This ensures memory and skill reviews always run on
the strong model that can reliably call the memory/skill_manage tools.
Verify that _spawn_background_review reads the primary model from
config.yaml instead of inheriting the current turn's model, and
falls back to self.model when config loading fails.
@BukeLy

BukeLy commented Apr 13, 2026

Copy link
Copy Markdown
Author

Hi @jbarket @chalkers @leon @intertwine,
could you please take a look at this PR and help enable the CI run? Thanks!

@teknium1

Copy link
Copy Markdown
Contributor

Thanks for the thoughtful PR, @BukeLy! This is a real concern, but the problem it solves no longer exists on main.

Automated hermes-sweeper review.

  • The smart_model_routing feature — which caused the background review to inherit a cheap/weak turn model — was removed wholesale in commit 424e9f36b (PR refactor: remove smart_model_routing feature #12732, April 19 2026). The routing module, its config keys, docs, and tests are all gone.
  • The related credential-inheritance issue (review fork not getting api_key/base_url/api_mode) was independently fixed in commit e3901d5b2 (PR fix(run_agent): background review fork inherits parent's live runtime #16099, April 26 2026), which already passes the full parent runtime into the review fork via _current_main_runtime().
  • Current _spawn_background_review (run_agent.py line 3257) uses self.model — the parent's actual configured model — with no per-turn model-switching mechanism that could downgrade it.

Closing as already addressed on main. If a new per-turn routing feature lands in the future, the fix pattern here would be a good reference.

@teknium1 teknium1 closed this Apr 28, 2026
@alt-glitch alt-glitch added type/bug Something isn't working P2 Medium — degraded but workaround exists comp/agent Core agent runtime: loop, agent_init, prompt builder, context-compression, responses endpoint tool/memory Memory tool and memory providers tool/skills Skills system (list, view, manage) labels Apr 28, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

comp/agent Core agent runtime: loop, agent_init, prompt builder, context-compression, responses endpoint P2 Medium — degraded but workaround exists tool/memory Memory tool and memory providers tool/skills Skills system (list, view, manage) type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug]: Background memory review uses cheap model from smart routing — cannot call tools

3 participants