fix(delegation): keep child routes and fallback policy together
This commit is contained in:
@@ -1663,8 +1663,8 @@ delegation:
|
||||
# # vast majority of tokens, so this is where spend is cut while
|
||||
# # planning quality stays with the frontier parent.
|
||||
# fallback_providers: # Fallback chain for delegated children (same entry format as the
|
||||
# - provider: "openrouter" # top-level fallback_providers list). Unset = inherit the parent
|
||||
# model: "deepseek/deepseek-chat" # agent's chain; [] = disable child fallback entirely (#65038).
|
||||
# - provider: "openrouter" # top-level list). Pinned children use only a declared child chain;
|
||||
# model: "deepseek/deepseek-chat" # unpinned children inherit when unset. [] disables fallback (#65038).
|
||||
|
||||
# =============================================================================
|
||||
# Honcho Integration (Cross-Session User Modeling)
|
||||
|
||||
@@ -9,6 +9,8 @@ pin+declared-chain composition cell raised in the #80450 cross-PR map.
|
||||
import unittest
|
||||
from unittest.mock import MagicMock, patch
|
||||
|
||||
import pytest
|
||||
|
||||
from tools.delegate_tool import _build_child_agent, _resolve_child_fallback_chain
|
||||
from tests.tools.test_delegate import _make_mock_parent
|
||||
|
||||
@@ -95,6 +97,24 @@ class TestResolveChildFallbackChainMatrix(unittest.TestCase):
|
||||
routes = [(e.get("provider"), e.get("model")) for e in (chain or [])]
|
||||
self.assertEqual(len(routes), len(set(routes)))
|
||||
|
||||
def test_valid_route_survives_invalid_neighbor(self):
|
||||
"""One malformed entry must not discard a usable declared route."""
|
||||
declared = [
|
||||
{"provider": "deepseek", "model": "worker-fallback"},
|
||||
{"provider": "deepseek"},
|
||||
]
|
||||
expected = [{"provider": "deepseek", "model": "worker-fallback"}]
|
||||
for pinned in (False, True):
|
||||
with self.subTest(pinned=pinned):
|
||||
self.assertEqual(
|
||||
_resolve_child_fallback_chain(
|
||||
_parent(list(PARENT_CHAIN)),
|
||||
{"fallback_providers": declared},
|
||||
pinned=pinned,
|
||||
),
|
||||
expected,
|
||||
)
|
||||
|
||||
def test_malformed_declared_value_pin_aware_fallback(self):
|
||||
"""Malformed config logs and falls back pin-aware: None when pinned
|
||||
(never reintroduce the silent drag through the error path), parent
|
||||
@@ -263,5 +283,135 @@ def test_declared_chain_flows_through_real_profile_config_loader(
|
||||
assert child_kwargs["fallback_model"] == DECLARED_CHAIN
|
||||
|
||||
|
||||
def test_explicit_empty_chain_survives_real_profile_config_loader(tmp_path, monkeypatch):
|
||||
"""An explicit [] remains an authoritative disable after config loading."""
|
||||
import yaml
|
||||
|
||||
from hermes_constants import reset_hermes_home_override, set_hermes_home_override
|
||||
|
||||
monkeypatch.delenv("HERMES_IGNORE_USER_CONFIG", raising=False)
|
||||
token = set_hermes_home_override(tmp_path)
|
||||
try:
|
||||
(tmp_path / "config.yaml").write_text(
|
||||
yaml.safe_dump({"delegation": {"fallback_providers": []}}),
|
||||
encoding="utf-8",
|
||||
)
|
||||
with patch("run_agent.AIAgent") as mock_agent:
|
||||
mock_agent.return_value = MagicMock()
|
||||
_build_child_agent(
|
||||
task_index=0,
|
||||
goal="explicit disable",
|
||||
context=None,
|
||||
toolsets=None,
|
||||
model=None,
|
||||
max_iterations=10,
|
||||
parent_agent=_parent(list(PARENT_CHAIN)),
|
||||
task_count=1,
|
||||
)
|
||||
finally:
|
||||
reset_hermes_home_override(token)
|
||||
|
||||
assert mock_agent.call_args.kwargs["fallback_model"] is None
|
||||
|
||||
|
||||
def test_pinned_review_does_not_borrow_general_worker_chain(tmp_path, monkeypatch):
|
||||
"""The public /review route owns its fallback policy as well as its model."""
|
||||
import yaml
|
||||
|
||||
from agent.review_engine import start_review
|
||||
from hermes_constants import reset_hermes_home_override, set_hermes_home_override
|
||||
|
||||
monkeypatch.delenv("HERMES_IGNORE_USER_CONFIG", raising=False)
|
||||
(tmp_path / "config.yaml").write_text(
|
||||
yaml.safe_dump(
|
||||
{
|
||||
"delegation": {
|
||||
"fallback_providers": [
|
||||
{"provider": "deepseek", "model": "worker-fallback"}
|
||||
]
|
||||
},
|
||||
"auxiliary": {
|
||||
"review": {
|
||||
"provider": "custom",
|
||||
"model": "review-model",
|
||||
"base_url": "http://127.0.0.1:18479/v1",
|
||||
"api_key": "test-only",
|
||||
}
|
||||
},
|
||||
}
|
||||
),
|
||||
encoding="utf-8",
|
||||
)
|
||||
parent = _parent(list(PARENT_CHAIN))
|
||||
parent.session_id = "review-80479-parent"
|
||||
captured = {}
|
||||
|
||||
class ReachedConstructor(RuntimeError):
|
||||
pass
|
||||
|
||||
def capture(**kwargs):
|
||||
captured.update(kwargs)
|
||||
raise ReachedConstructor()
|
||||
|
||||
token = set_hermes_home_override(tmp_path)
|
||||
try:
|
||||
with patch("run_agent.AIAgent", side_effect=capture):
|
||||
with pytest.raises(ReachedConstructor):
|
||||
start_review(
|
||||
parent,
|
||||
[{"role": "user", "content": "Check the last result"}],
|
||||
)
|
||||
finally:
|
||||
reset_hermes_home_override(token)
|
||||
|
||||
assert captured["model"] == "review-model"
|
||||
assert captured["base_url"] == "http://127.0.0.1:18479/v1"
|
||||
assert captured["fallback_model"] is None
|
||||
|
||||
|
||||
def test_declared_child_chain_activates_on_primary_failure():
|
||||
"""The selected chain is accepted by the real fallback activation rail."""
|
||||
from agent.error_classifier import FailoverReason
|
||||
from run_agent import AIAgent
|
||||
|
||||
chain = _resolve_child_fallback_chain(
|
||||
_parent(list(PARENT_CHAIN)),
|
||||
{"fallback_providers": list(DECLARED_CHAIN)},
|
||||
pinned=True,
|
||||
)
|
||||
with (
|
||||
patch("model_tools.get_tool_definitions", return_value=[]),
|
||||
patch("model_tools.check_toolset_requirements", return_value={}),
|
||||
patch("agent.process_bootstrap.OpenAI"),
|
||||
):
|
||||
child = AIAgent(
|
||||
api_key="primary-test-key",
|
||||
base_url="https://primary.example/v1",
|
||||
model="primary-model",
|
||||
provider="custom",
|
||||
quiet_mode=True,
|
||||
skip_context_files=True,
|
||||
skip_memory=True,
|
||||
fallback_model=chain,
|
||||
)
|
||||
fallback_client = MagicMock()
|
||||
fallback_client.base_url = "https://fallback.example/v1"
|
||||
fallback_client.api_key = "fallback-test-key"
|
||||
with (
|
||||
patch(
|
||||
"agent.auxiliary_client.resolve_provider_client",
|
||||
return_value=(fallback_client, "deepseek-chat"),
|
||||
),
|
||||
patch(
|
||||
"hermes_cli.model_normalize.normalize_model_for_provider",
|
||||
side_effect=lambda model, _provider: model,
|
||||
),
|
||||
):
|
||||
assert child._try_activate_fallback(FailoverReason.rate_limit) is True
|
||||
|
||||
assert child.model == "deepseek-chat"
|
||||
assert child.provider == "deepseek"
|
||||
|
||||
|
||||
if __name__ == "__main__":
|
||||
unittest.main()
|
||||
|
||||
+18
-5
@@ -31,7 +31,8 @@ from tools.delegate_tool_config import ( # noqa: F401
|
||||
_DEFAULT_MAX_CONCURRENT_CHILDREN, _get_child_timeout, _get_max_async_children, _get_max_concurrent_children,
|
||||
_get_max_spawn_depth, _get_orchestrator_enabled, _get_subagent_approval_callback, _get_worktree_isolation,
|
||||
_inherit_parent_capabilities, _load_config, _merge_request_overrides, _resolve_child_credential_pool,
|
||||
_resolve_child_runtime, _resolve_delegation_credentials, _subagent_auto_approve, _subagent_auto_deny,
|
||||
_resolve_child_fallback_chain, _resolve_child_runtime, _resolve_delegation_credentials,
|
||||
_subagent_auto_approve, _subagent_auto_deny,
|
||||
)
|
||||
from tools.delegate_tool_dispatch import _Batch, _announce_batch, _capture_origin, _run_batch
|
||||
from tools.delegate_tool_progress import ( # noqa: F401
|
||||
@@ -169,6 +170,10 @@ def _build_child_agent(
|
||||
# ACP transport overrides from trusted delegation config.
|
||||
override_acp_command: Optional[str] = None,
|
||||
override_acp_args: Optional[List[str]] = None,
|
||||
# Configuration block that owns the selected provider/model route. Internal
|
||||
# callers such as /review pass auxiliary.review here so fallback policy is
|
||||
# not accidentally read from the general delegation block.
|
||||
routing_cfg: Optional[Dict[str, Any]] = None,
|
||||
# Legacy; accepted for wire compat but ignored (capability is depth-derived).
|
||||
role: str = "leaf",
|
||||
):
|
||||
@@ -188,6 +193,9 @@ def _build_child_agent(
|
||||
subagent_id = f"sa-{task_index}-{_uuid.uuid4().hex[:8]}"
|
||||
parent_subagent_id = getattr(parent_agent, "_subagent_id", None)
|
||||
|
||||
# General delegation behavior (reasoning, compression, capabilities) stays
|
||||
# global. Only fallback policy follows the owner of a per-call route such
|
||||
# as auxiliary.review.
|
||||
delegation_cfg = _load_config()
|
||||
child_toolsets, child_disabled_toolsets = _resolve_child_toolsets(parent_agent, toolsets, effective_role)
|
||||
child_prompt = _build_child_system_prompt(
|
||||
@@ -211,6 +219,7 @@ def _build_child_agent(
|
||||
override_base_url=override_base_url, override_api_key=override_api_key, override_api_mode=override_api_mode,
|
||||
override_acp_command=override_acp_command,
|
||||
override_acp_args=override_acp_args,
|
||||
fallback_cfg=routing_cfg,
|
||||
)
|
||||
if override_request_overrides is not None:
|
||||
# honored whenever set, incl. the inherit branch where
|
||||
@@ -350,7 +359,8 @@ def _run_single_child(
|
||||
|
||||
def _build_children(
|
||||
task_list: List[Dict[str, Any]], task_schemas: List[Optional[Dict[str, Any]]], creds: Dict[str, Any], *,
|
||||
top_role: str, max_iterations: int, parent_agent, live_deleg_id: Optional[str], live_writers: list,
|
||||
top_role: str, max_iterations: int, parent_agent, routing_cfg: Dict[str, Any],
|
||||
live_deleg_id: Optional[str], live_writers: list,
|
||||
) -> tuple[List[tuple], Optional[str]]:
|
||||
"""Build every child on the main thread (construction is not thread-safe);
|
||||
``(children, None)`` or ``([], error)`` on an explicit-pin preflight failure."""
|
||||
@@ -362,6 +372,7 @@ def _build_children(
|
||||
"override_request_overrides": creds.get("request_overrides"),
|
||||
"override_acp_command": creds.get("command"),
|
||||
"override_acp_args": creds.get("args"),
|
||||
"routing_cfg": routing_cfg,
|
||||
}
|
||||
children = []
|
||||
for i, t in enumerate(task_list):
|
||||
@@ -446,9 +457,11 @@ def delegate_task(
|
||||
max_iterations, default_max_iter,
|
||||
)
|
||||
# credentials_cfg (internal callers only, e.g. /review → auxiliary.review) is
|
||||
# a per-call override shaped like the delegation config section.
|
||||
# a per-call routing owner shaped like the delegation config section. Keep
|
||||
# the route and its fallback policy together through child construction.
|
||||
routing_cfg = credentials_cfg if credentials_cfg is not None else cfg
|
||||
try:
|
||||
creds = _resolve_delegation_credentials(credentials_cfg if credentials_cfg else cfg, parent_agent)
|
||||
creds = _resolve_delegation_credentials(routing_cfg, parent_agent)
|
||||
except ValueError as exc:
|
||||
# Explicit-pin preflight failures (e.g. pinned delegation.command missing from PATH) refuse the
|
||||
# spawn loudly (#80450).
|
||||
@@ -472,7 +485,7 @@ def delegate_task(
|
||||
|
||||
children, err = _build_children(
|
||||
task_list, task_schemas, creds, top_role=top_role, max_iterations=default_max_iter, parent_agent=parent_agent,
|
||||
live_deleg_id=live_deleg_id, live_writers=live_writers,
|
||||
routing_cfg=routing_cfg, live_deleg_id=live_deleg_id, live_writers=live_writers,
|
||||
)
|
||||
if err:
|
||||
return tool_error(err)
|
||||
|
||||
@@ -412,10 +412,79 @@ _ROUTING_FILTER_DEFAULTS = (
|
||||
|
||||
_NOUS_PROVIDERS = frozenset({"nous", "nous-portal", "nousresearch"})
|
||||
|
||||
|
||||
def _resolve_child_fallback_chain(parent_agent, routing_cfg: Any, pinned: bool) -> Optional[List[Dict[str, Any]]]:
|
||||
"""Resolve the fallback chain owned by the same config block as the child route.
|
||||
|
||||
A pinned child (provider, endpoint, or model) never borrows the parent's
|
||||
chain. It may use only a chain explicitly declared by its routing owner.
|
||||
An unpinned child inherits the parent chain when the setting is absent or
|
||||
null. An explicit empty list disables fallback in either case.
|
||||
|
||||
Valid entries in a partially malformed declaration are retained through
|
||||
the canonical fallback normalizer. If no usable entry remains, the
|
||||
pin-aware default applies.
|
||||
"""
|
||||
parent_chain = getattr(parent_agent, "_fallback_chain", None) or None
|
||||
default = None if pinned else parent_chain
|
||||
if not isinstance(routing_cfg, dict) or "fallback_providers" not in routing_cfg:
|
||||
return default
|
||||
|
||||
declared = routing_cfg.get("fallback_providers")
|
||||
if declared is None:
|
||||
return default
|
||||
if declared == []:
|
||||
return None
|
||||
if not isinstance(declared, list):
|
||||
logger.warning(
|
||||
"Ignoring delegation fallback_providers: expected a list, got %s; using the %s default",
|
||||
type(declared).__name__,
|
||||
"pinned" if pinned else "inherited",
|
||||
)
|
||||
return default
|
||||
|
||||
invalid_positions = [
|
||||
index
|
||||
for index, entry in enumerate(declared)
|
||||
if not isinstance(entry, dict)
|
||||
or not str(entry.get("provider") or "").strip()
|
||||
or not str(entry.get("model") or "").strip()
|
||||
]
|
||||
if invalid_positions:
|
||||
logger.warning(
|
||||
"Ignoring invalid delegation fallback_providers entr%s at index%s %s",
|
||||
"y" if len(invalid_positions) == 1 else "ies",
|
||||
"" if len(invalid_positions) == 1 else "es",
|
||||
", ".join(str(index) for index in invalid_positions),
|
||||
)
|
||||
|
||||
try:
|
||||
from hermes_cli.fallback_config import get_fallback_chain
|
||||
|
||||
normalized = get_fallback_chain({"fallback_providers": declared})
|
||||
except Exception as exc:
|
||||
logger.warning(
|
||||
"Could not normalize delegation fallback_providers (%s); using the %s default",
|
||||
exc,
|
||||
"pinned" if pinned else "inherited",
|
||||
)
|
||||
return default
|
||||
|
||||
if normalized:
|
||||
return normalized
|
||||
if declared:
|
||||
logger.warning(
|
||||
"delegation fallback_providers contains no usable routes; using the %s default",
|
||||
"pinned" if pinned else "inherited",
|
||||
)
|
||||
return default
|
||||
|
||||
|
||||
def _resolve_child_runtime(
|
||||
parent_agent, delegation_cfg: dict, parent_api_key: Any, *, model: Optional[str], override_provider: Optional[str],
|
||||
override_base_url: Optional[str], override_api_key: Optional[str], override_api_mode: Optional[str],
|
||||
override_acp_command: Optional[str], override_acp_args: Optional[List[str]],
|
||||
fallback_cfg: Optional[Dict[str, Any]] = None,
|
||||
) -> Dict[str, Any]:
|
||||
"""Child credentials, transport and routing (config override > parent inherit) as ``AIAgent`` kwargs. Rules that
|
||||
are easy to break: api_mode is re-derived (not inherited) when the child's provider differs from the parent's
|
||||
@@ -489,9 +558,13 @@ def _resolve_child_runtime(
|
||||
"capabilities": _inherit_parent_capabilities(parent_agent, override_provider, override_base_url),
|
||||
"api_mode": effective_api_mode, "acp_command": effective_acp_command, "acp_args": effective_acp_args,
|
||||
"reasoning_config": child_reasoning,
|
||||
# Inherit the parent's fallback chain EXCEPT under a pinned provider: a mid-run 429/auth failure must not
|
||||
# silently reroute the quiet child onto the parent's fallbacks. Predictability > liveness for explicit pins.
|
||||
"fallback_model": None if override_provider else (getattr(parent_agent, "_fallback_chain", None) or None),
|
||||
# Resolve routing and recovery policy from the same configuration owner. A pinned provider, endpoint, or
|
||||
# model never borrows the parent's chain; an explicitly declared child chain still remains available.
|
||||
"fallback_model": _resolve_child_fallback_chain(
|
||||
parent_agent,
|
||||
fallback_cfg if isinstance(fallback_cfg, dict) else delegation_cfg,
|
||||
pinned=bool(override_provider or override_base_url or model),
|
||||
),
|
||||
"openrouter_min_coding_score": getattr(parent_agent, "openrouter_min_coding_score", None),
|
||||
# Routing filters reset to their defaults under a pinned provider (see _ROUTING_FILTER_DEFAULTS).
|
||||
**{a: d if override_provider else getattr(parent_agent, a, d) for a, d in _ROUTING_FILTER_DEFAULTS},
|
||||
|
||||
@@ -185,7 +185,7 @@ fallback_providers:
|
||||
|---------|-------------------|
|
||||
| CLI sessions | ✔ |
|
||||
| Messaging gateway (Telegram, Discord, etc.) | ✔ |
|
||||
| Subagent delegation | ✔ (`delegation.fallback_providers` when set; otherwise inherit parent chain; `[]` disables) |
|
||||
| Subagent delegation | ✔ (`delegation.fallback_providers` when set; otherwise only unpinned children inherit the parent chain; `[]` disables) |
|
||||
| Cron jobs | ✔ (cron agents inherit configured fallback providers) |
|
||||
| Auxiliary tasks on `provider: auto` | ✔ (try per-task fallback, then the main fallback chain before built-in aux discovery) |
|
||||
|
||||
@@ -440,5 +440,5 @@ See [Scheduled Tasks (Cron)](/user-guide/features/cron) for full configuration d
|
||||
| Approval classification | Layered (see above) | `auxiliary.approval` |
|
||||
| Title generation | Layered (see above) | `auxiliary.title_generation` |
|
||||
| Triage specifier | Layered (see above) | `auxiliary.triage_specifier` |
|
||||
| Delegation | Inherits the parent's `fallback_providers` chain; optional provider/model override | `delegation.provider` / `delegation.model` |
|
||||
| Delegation | Uses `delegation.fallback_providers` when declared; otherwise only unpinned children inherit the parent chain | `delegation.provider` / `delegation.model` / `delegation.fallback_providers` |
|
||||
| Cron jobs | Inherit the configured `fallback_providers` chain; optional per-job provider override | Per-job `provider` / `model` |
|
||||
|
||||
Reference in New Issue
Block a user