fix(agent): routed background reviews honor auxiliary.background_review.reasoning_effort
The review fork is a full AIAgent, not an auxiliary_client call, and its routed branch deliberately skips the parent's reasoning_config (the parent's effort vocabulary may be invalid for the routed provider). It also never read the per-task key, so an explicit `auxiliary.background_review.reasoning_effort` was silently ignored and the routed fork ran at the provider default (#94825). Routed forks now parse the task key through the shared parse_reasoning_effort (same levels and `none` alias as every other aux task); unset keeps the provider default, an unknown level warns and falls through. The same-model path is untouched: it still inherits the parent's reasoning_config verbatim for prompt-cache parity. Salvaged from #94832 (liuhao1024), re-applied on the decomposed _fork_init_kwargs seam. Co-authored-by: liuhao1024 <sunsky.lau@gmail.com>
This commit is contained in:
@@ -844,7 +844,27 @@ def _detach_fork_compression(review_agent: Any) -> None:
|
||||
review_agent._review_defer_compaction_before_first_response = True
|
||||
|
||||
|
||||
def _fork_init_kwargs(agent: Any, rt: Dict[str, Any], routed: bool, max_iterations: int) -> Dict[str, Any]:
|
||||
def _routed_reasoning_config(task_cfg: Optional[Dict[str, Any]]) -> Optional[Dict[str, Any]]:
|
||||
"""``reasoning_config`` for a ROUTED fork from ``auxiliary.background_review.reasoning_effort``
|
||||
(#94825). The routed branch never inherits the parent's effort (its vocabulary may be invalid for
|
||||
the routed provider), but an explicit per-task pin is the user's choice for THAT model and must
|
||||
win over provider defaults, as every other aux task already does via ``_get_task_extra_body``.
|
||||
None = unset (provider default); an unknown level warns and falls through to the default."""
|
||||
effort = _background_review_task_config(task_cfg).get("reasoning_effort")
|
||||
if effort is None or effort == "":
|
||||
return None
|
||||
from hermes_constants import VALID_REASONING_EFFORTS, parse_reasoning_effort
|
||||
parsed = parse_reasoning_effort(effort)
|
||||
if parsed is None:
|
||||
logger.warning(
|
||||
"auxiliary.background_review.reasoning_effort %r is not a valid level (none, %s) — using "
|
||||
"the routed provider's default", effort, ", ".join(VALID_REASONING_EFFORTS),
|
||||
)
|
||||
return parsed
|
||||
|
||||
|
||||
def _fork_init_kwargs(agent: Any, rt: Dict[str, Any], routed: bool, max_iterations: int,
|
||||
task_cfg: Optional[Dict[str, Any]] = None) -> Dict[str, Any]:
|
||||
"""AIAgent constructor kwargs for the review fork. skip_memory=True: an external memory plugin
|
||||
scoped to the parent's session_id would leak the harness prompt into the user's real memory
|
||||
namespace; built-in MEMORY.md/USER.md state is re-bound by the caller. Toolsets match the
|
||||
@@ -865,6 +885,8 @@ def _fork_init_kwargs(agent: Any, rt: Dict[str, Any], routed: bool, max_iteratio
|
||||
kwargs.update(acp_command=rt["command"], acp_args=rt.get("args") or [])
|
||||
if not routed:
|
||||
kwargs.update(_same_model_parity_kwargs(agent))
|
||||
elif (routed_cfg := _routed_reasoning_config(task_cfg)) is not None:
|
||||
kwargs["reasoning_config"] = routed_cfg
|
||||
return kwargs
|
||||
|
||||
|
||||
@@ -906,7 +928,7 @@ def build_cache_parity_fork(
|
||||
# separate, tracked issue (#94825) and are left alone.
|
||||
if not _routed and write_origin == "background_review":
|
||||
_warn_ignored_reasoning_effort(agent, task_cfg)
|
||||
review_agent = AIAgent(**_fork_init_kwargs(agent, _rt, _routed, max_iterations))
|
||||
review_agent = AIAgent(**_fork_init_kwargs(agent, _rt, _routed, max_iterations, task_cfg))
|
||||
review_agent._memory_write_origin = review_agent._memory_write_context = write_origin
|
||||
review_agent._memory_store = agent._memory_store
|
||||
review_agent._memory_enabled = agent._memory_enabled
|
||||
|
||||
50
tests/agent/test_background_review_routed_effort.py
Normal file
50
tests/agent/test_background_review_routed_effort.py
Normal file
@@ -0,0 +1,50 @@
|
||||
"""Routed background reviews honor ``auxiliary.background_review.reasoning_effort`` (#94825).
|
||||
|
||||
The review fork is a full AIAgent, not an auxiliary_client call. The routed branch deliberately
|
||||
skips the PARENT's reasoning_config (its effort vocabulary may be invalid for the routed model),
|
||||
but an explicitly configured per-task effort must win over provider defaults — mirroring how every
|
||||
other auxiliary task folds the same key into ``extra_body.reasoning``.
|
||||
"""
|
||||
from __future__ import annotations
|
||||
|
||||
import logging
|
||||
from unittest.mock import patch
|
||||
|
||||
import run_agent
|
||||
import agent.background_review as bg_review
|
||||
from agent.background_review import build_cache_parity_fork
|
||||
|
||||
from tests.agent.test_background_review_cache_parity import _make_agent_stub, _make_recorder_class
|
||||
|
||||
ROUTED_RUNTIME = {
|
||||
"provider": "openrouter", "model": "aux-cheap-model", "api_key": "test-key",
|
||||
"base_url": None, "api_mode": None, "credential_pool": None, "request_overrides": {},
|
||||
"max_tokens": None, "command": None, "args": [], "routed": True,
|
||||
}
|
||||
|
||||
|
||||
def _routed_fork_kwargs(task_cfg):
|
||||
captured = {}
|
||||
agent = _make_agent_stub(run_agent.AIAgent)
|
||||
agent.reasoning_config = {"enabled": True, "effort": "high"}
|
||||
with patch.object(run_agent, "AIAgent", _make_recorder_class(captured)), \
|
||||
patch.object(bg_review, "_resolve_review_runtime", return_value=ROUTED_RUNTIME):
|
||||
_fork, _rt, routed = build_cache_parity_fork(agent, task_cfg, max_iterations=5)
|
||||
assert routed
|
||||
return captured["init_kwargs"]
|
||||
|
||||
|
||||
def test_routed_review_applies_configured_effort_not_parents():
|
||||
kwargs = _routed_fork_kwargs({"reasoning_effort": "xhigh"})
|
||||
assert kwargs["reasoning_config"] == {"enabled": True, "effort": "xhigh"}
|
||||
# ``none`` disables thinking on the routed fork, same vocabulary as every other aux task.
|
||||
assert _routed_fork_kwargs({"reasoning_effort": "none"})["reasoning_config"] == {"enabled": False}
|
||||
|
||||
|
||||
def test_routed_review_falls_back_to_provider_default(caplog):
|
||||
# Unset: provider default, and the parent's ``high`` is NOT smuggled across the route.
|
||||
assert "reasoning_config" not in _routed_fork_kwargs({"reasoning_effort": ""})
|
||||
assert "reasoning_config" not in _routed_fork_kwargs({})
|
||||
with caplog.at_level(logging.WARNING):
|
||||
assert "reasoning_config" not in _routed_fork_kwargs({"reasoning_effort": "ludicrous"})
|
||||
assert "ludicrous" in caplog.text
|
||||
Reference in New Issue
Block a user