Skip to content

fix(agent): forward reasoning_config to background review AIAgent fork - #18973

Closed
nftpoetrist wants to merge 1 commit into
NousResearch:mainfrom
nftpoetrist:fix/18871-background-review-reasoning-config
Closed

fix(agent): forward reasoning_config to background review AIAgent fork#18973
nftpoetrist wants to merge 1 commit into
NousResearch:mainfrom
nftpoetrist:fix/18871-background-review-reasoning-config

Conversation

@nftpoetrist

@nftpoetrist nftpoetrist commented May 2, 2026

Copy link
Copy Markdown
Contributor

What does this PR do?

`_spawn_background_review()` in `run_agent.py` (line ~3611) inherits the parent session's `provider`, `model`, `base_url`, `api_key`, and `api_mode` when forking a review agent. `reasoning_config` was not forwarded.

On Codex Responses routes the missing field causes the forked review agent to fall back to the transport default (`medium` effort), so a session configured for `agent.reasoning_effort: xhigh` still generates medium-effort background review requests — wasting budget and ignoring the user's explicit preference.

One-line fix in `_spawn_background_review()` — passes `reasoning_config=self.reasoning_config`. When `reasoning_config` is `None` the behaviour is identical to before: harmless no-op for sessions without a reasoning override. Symmetric with the `api_mode`, `base_url`, and `api_key` fields that #16006 and #15884 already propagate on the same fork path.

Related Issue

Fixes #18871

Type of Change

  • 🐛 Bug fix
  • ✨ New feature
  • 🔒 Security fix
  • 📝 Documentation update
  • ✅ Tests
  • ♻️ Refactor
  • 🎯 New skill

Changes Made

  • `run_agent.py`: add `reasoning_config=self.reasoning_config` to `AIAgent(...)` in `_spawn_background_review()` (+1 line)
  • `tests/run_agent/test_background_review.py`: add `reasoning_config = None` to `_bare_agent()` helper; add `test_background_review_inherits_reasoning_config` parametrized over `None`, `{"effort": "xhigh"}`, and `{"enabled": False}` (+49 lines)

How to Test

```bash
python3.11 -m pytest tests/run_agent/test_background_review.py -v --override-ini="addopts="
```

Checklist

Code

  • Contributing Guide read
  • Conventional Commits
  • No duplicate PR
  • Single logical change only
  • pytest passing
  • Tests added
  • Platform: macOS

Documentation & Housekeeping

  • Docs updated — N/A
  • cli-config.yaml.example — N/A
  • CONTRIBUTING.md/AGENTS.md — N/A
  • Cross-platform impact — N/A
  • Tool descriptions — N/A

_spawn_background_review() (run_agent.py line ~3611) inherits the parent
session's provider, model, base_url, api_key, and api_mode — but not
reasoning_config. On Codex Responses routes the forked review agent
falls back to the transport default (medium effort), so a session
configured for agent.reasoning_effort: xhigh still generates medium-
effort background review requests.

Fix: pass reasoning_config=self.reasoning_config to the AIAgent
constructor in _spawn_background_review(). When reasoning_config is
None the behaviour is identical to before — harmless no-op for sessions
without a reasoning override. Symmetric with the api_mode/base_url/api_key
fields that NousResearch#16006 and NousResearch#15884 already propagate on the same fork path.

Fixes NousResearch#18871
@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 labels May 2, 2026

@teknium1 teknium1 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks for identifying the missing forked-agent runtime field. The defect remains on current main, but the implementation location moved after this PR was opened.

Problems

  • run_agent.py:1598-1625 is now only a wrapper; the live review-fork construction is agent/background_review.py:683-697, where reasoning_config is still absent. The PR's added line therefore needs to be relocated to affect current behavior.
  • This matters on the reported route: agent/transports/codex.py:155-163 defaults missing reasoning_config to medium and only applies a supplied effort value.

Suggested changes

  • Salvage the constructor argument into agent/background_review.py:683-697 and port the regression test to capture that current constructor call for None, enabled xhigh, and disabled configurations.

Automated hermes-sweeper review.

Comment thread run_agent.py
credential_pool=getattr(self, "_credential_pool", None),
parent_session_id=self.session_id,
enabled_toolsets=["memory", "skills"],
reasoning_config=self.reasoning_config,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Current main moved the review-fork constructor to agent/background_review.py:683-697 in 1f6eb1738c206e95c3e0641c3d8000a4d0be841b; run_agent.py:1598-1625 is now a wrapper. Relocate this argument to the extracted constructor so the fix reaches the live path.

@teknium1 teknium1 added sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform labels Jul 12, 2026
@kshitijk4poor

Copy link
Copy Markdown
Collaborator

Thanks for the fix — this was the right change, and you had it early. The same fix has now landed on main via #64379, a salvage of #30532 (the competing PR that paired the identical one-line change with the Anthropic cache-parity framing, wire-level measurements, and a regression test in the existing cache-parity suite). Your PR was submitted first among the competing fixes for #18871; #30532's version was selected on test coverage and framing, not timing.

Current main also gates the inheritance on the non-routed path (if not _routed: — when auxiliary.background_review routes the review to a different model, the fork uses provider defaults instead of the parent's effort vocabulary).

Closing as superseded by the merged fix. Thanks again for the contribution.

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 sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Background review agents ignore reasoning_config and fall back to medium

4 participants