Skip to content
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
66 changes: 64 additions & 2 deletions agent/background_review.py
Original file line number Diff line number Diff line change
Expand Up @@ -201,6 +201,43 @@ def cancel_background_review_for_live_turn(agent: Any) -> None:
_REVIEW_MAX_ITERATIONS = 16


def _review_provider_matches_parent(
agent: Any,
parent_runtime: Dict[str, Any],
task_provider: str,
) -> bool:
"""Return whether an aux provider name identifies the live parent route.

Named custom providers resolve to the generic runtime provider ``custom``
while the agent retains the user-facing alias in ``requested_provider``.
Compare both identities, but do not treat bare ``custom`` as equivalent to
every named custom endpoint: that would collapse a legitimate separate
route onto the parent credential.
"""
task_identity = str(task_provider or "").strip().lower().replace(" ", "-")
resolved_identity = str(
parent_runtime.get("provider") or getattr(agent, "provider", "") or ""
).strip().lower().replace(" ", "-")
requested_identity = str(
parent_runtime.get("requested_provider")
or getattr(agent, "requested_provider", "")
or ""
).strip().lower().replace(" ", "-")

if requested_identity and task_identity == requested_identity:
return True
if resolved_identity and resolved_identity != "custom":
return task_identity == resolved_identity
if resolved_identity != "custom":
return False

if requested_identity and requested_identity not in {"auto", "custom"}:
from hermes_cli.providers import custom_provider_aliases

return task_identity in custom_provider_aliases(requested_identity)
return task_identity == "custom"


def _background_review_task_config(
task_cfg: Optional[Dict[str, Any]] = None,
) -> Dict[str, Any]:
Expand Down Expand Up @@ -295,6 +332,11 @@ def _resolve_review_runtime(
parent_api_mode = "codex_responses"
parent = {
"provider": agent.provider,
"requested_provider": (
parent_runtime.get("requested_provider")
or getattr(agent, "requested_provider", "")
or agent.provider
),
"model": agent.model,
"api_key": parent_runtime.get("api_key") or None,
"base_url": parent_runtime.get("base_url") or None,
Expand All @@ -310,11 +352,26 @@ def _resolve_review_runtime(
task_provider = (str(task.get("provider", "")).strip() or None)
task_model = (str(task.get("model", "")).strip() or None)
task_base_url = (str(task.get("base_url", "")).strip() or None)
task_api_key = (str(task.get("api_key", "")).strip() or None)
if not (task_provider and task_provider != "auto" and task_model):
return parent
if task_provider == (agent.provider or "") and task_model == (agent.model or ""):
if (
task_model == (agent.model or "")
and _review_provider_matches_parent(agent, parent_runtime, task_provider)
):
return parent # same model/provider as parent -> not routed
task_key_env = str(
task.get("key_env") or task.get("api_key_env") or ""
).strip()
if task_key_env:
from agent.secret_scope import get_secret

# ``key_env`` is the durable credential reference and is authoritative
# over any already-expanded ``api_key`` copy. In a multiplex process,
# that copy may have been interpolated from the default profile's
# process environment before this profile's scope was installed.
task_api_key = (get_secret(task_key_env, "") or "").strip() or None
else:
task_api_key = (str(task.get("api_key", "")).strip() or None)
try:
from hermes_cli.runtime_provider import resolve_runtime_provider
rp = resolve_runtime_provider(
Expand All @@ -325,6 +382,7 @@ def _resolve_review_runtime(
)
return {
"provider": rp.get("provider") or task_provider,
"requested_provider": rp.get("requested_provider") or task_provider,
"model": rp.get("model") or task_model,
"api_key": rp.get("api_key"),
"base_url": rp.get("base_url"),
Expand Down Expand Up @@ -1190,6 +1248,10 @@ def _finish_request_phase(agent_ref) -> None:
quiet_mode=True,
platform=agent.platform,
provider=_rt.get("provider") or agent.provider,
requested_provider=(
_rt.get("requested_provider")
or getattr(agent, "requested_provider", None)
),
api_mode=_rt.get("api_mode"),
base_url=_rt.get("base_url") or None,
api_key=_rt.get("api_key") or None,
Expand Down
2 changes: 2 additions & 0 deletions tests/run_agent/test_background_review_cache_parity.py
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ def _make_agent_stub(agent_cls):
agent.model = "test-model"
agent.platform = "test"
agent.provider = "openai"
agent.requested_provider = "openai-direct"
agent.session_id = "sess-123"
agent.quiet_mode = True
agent._memory_store = None
Expand Down Expand Up @@ -234,6 +235,7 @@ def test_review_fork_inherits_prefill_and_provider_routing():
), "fork prefill aliases the parent's dicts (needs deepcopy)"
assert init_kwargs.get("providers_allowed") == agent.providers_allowed
assert init_kwargs.get("provider_sort") == agent.provider_sort
assert init_kwargs.get("requested_provider") == agent.requested_provider


def test_review_fork_pins_session_start_and_session_id():
Expand Down
157 changes: 156 additions & 1 deletion tests/run_agent/test_background_review_cost_controls.py
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,10 @@
from typing import Any
from unittest.mock import patch

import pytest

from agent import background_review as br
from hermes_cli import runtime_provider as _runtime_provider # noqa: F401


def _msg(role, content, tool_calls=None):
Expand All @@ -26,8 +29,14 @@ def _msg(role, content, tool_calls=None):
# ---------------------------------------------------------------------------

class _FakeAgent:
def __init__(self, provider="openai-codex", model="gpt-5.5"):
def __init__(
self,
provider="openai-codex",
model="gpt-5.5",
requested_provider=None,
):
self.provider = provider
self.requested_provider = requested_provider or provider
self.model = model
self._credential_pool: Any = None
self.request_overrides = {}
Expand Down Expand Up @@ -98,6 +107,152 @@ def test_routing_same_model_as_parent_is_not_routed():
assert rt["routed"] is False # same model/provider → keep full-replay path


@pytest.mark.parametrize(
"task_provider",
("litellm-local", "custom:litellm-local"),
)
def test_routing_same_custom_alias_inherits_live_parent_credential(task_provider):
"""A named custom alias resolving to ``custom`` is still the parent route.

In a multiplex process, config interpolation may have captured the default
profile's process-global key before this profile's scope was installed.
The review must recognize the requested alias and inherit the live parent
credential instead of explicitly forwarding that stale expanded value.
"""
from agent import secret_scope

agent = _FakeAgent(
provider="custom",
requested_provider="litellm-local",
model="MiniMax-M3",
)
task = {
"provider": task_provider,
"model": "MiniMax-M3",
"api_key": "wrong-default-profile-key",
"key_env": "LITELLM_MASTER_KEY",
}
secret_scope.set_multiplex_active(True)
token = secret_scope.set_secret_scope(
{"LITELLM_MASTER_KEY": "scoped-profile-key"}
)
try:
with patch(
"hermes_cli.runtime_provider.resolve_runtime_provider"
) as resolve_runtime, patch(
"agent.secret_scope.get_secret"
) as get_secret:
rt = br._resolve_review_runtime(agent, task)
finally:
secret_scope.reset_secret_scope(token)
secret_scope.set_multiplex_active(False)

assert rt["routed"] is False
assert rt["requested_provider"] == "litellm-local"
assert rt["api_key"] == "parent-key"
resolve_runtime.assert_not_called()
get_secret.assert_not_called()


def test_distinct_review_provider_uses_scoped_key_env_credential():
"""A different provider remains routed even when its model name matches."""
from agent import secret_scope

agent = _FakeAgent(
provider="custom",
requested_provider="litellm-local",
model="MiniMax-M3",
)
task = {
"provider": "other-proxy",
"model": "MiniMax-M3",
"api_key": "wrong-default-profile-key",
"key_env": "LITELLM_MASTER_KEY",
}
routed = {
"provider": "custom",
"requested_provider": "other-proxy",
"model": "MiniMax-M3",
"api_key": "scoped-profile-key",
"base_url": "http://127.0.0.1:4001/v1",
"api_mode": "chat_completions",
}
secret_scope.set_multiplex_active(True)
token = secret_scope.set_secret_scope(
{"LITELLM_MASTER_KEY": "scoped-profile-key"}
)
try:
with patch(
"hermes_cli.runtime_provider.resolve_runtime_provider",
return_value=routed,
) as resolve_runtime:
rt = br._resolve_review_runtime(agent, task)
finally:
secret_scope.reset_secret_scope(token)
secret_scope.set_multiplex_active(False)

assert rt["routed"] is True
assert rt["requested_provider"] == "other-proxy"
assert rt["model"] == "MiniMax-M3"
resolve_runtime.assert_called_once_with(
requested="other-proxy",
target_model="MiniMax-M3",
explicit_api_key="scoped-profile-key",
explicit_base_url=None,
)


def test_distinct_review_provider_real_resolution_uses_profile_scope(
tmp_path,
monkeypatch,
):
"""Exercise config expansion through the real custom-provider resolver."""
from agent import secret_scope

hermes_home = tmp_path / "profile"
hermes_home.mkdir()
(hermes_home / "config.yaml").write_text(
"""
providers:
other-proxy:
api: http://127.0.0.1:4001/v1
key_env: LITELLM_MASTER_KEY
default_model: MiniMax-M3
auxiliary:
background_review:
provider: other-proxy
model: MiniMax-M3
api_key: ${LITELLM_MASTER_KEY}
key_env: LITELLM_MASTER_KEY
""".lstrip(),
encoding="utf-8",
)
monkeypatch.setenv("HERMES_HOME", str(hermes_home))
monkeypatch.setenv("LITELLM_MASTER_KEY", "wrong-default-profile-key")
agent = _FakeAgent(
provider="custom",
requested_provider="litellm-local",
model="MiniMax-M3",
)

secret_scope.set_multiplex_active(True)
token = secret_scope.set_secret_scope(
{"LITELLM_MASTER_KEY": "scoped-profile-key"}
)
try:
rt = br._resolve_review_runtime(agent)
finally:
secret_scope.reset_secret_scope(token)
secret_scope.set_multiplex_active(False)

assert rt["routed"] is True
assert rt["provider"] == "custom"
assert rt["requested_provider"] == "other-proxy"
assert rt["model"] == "MiniMax-M3"
assert rt["api_key"] == "scoped-profile-key"
assert rt["base_url"] == "http://127.0.0.1:4001/v1"


def test_routing_resolution_failure_falls_back_to_parent():
agent = _FakeAgent()
cfg = {"auxiliary": {"background_review": {
Expand Down