From c47bf78d68b0b1a2e58c924acd963931242dfa18 Mon Sep 17 00:00:00 2001 From: Ayush Nangia Date: Mon, 7 Sep 2026 23:30:46 +0530 Subject: [PATCH] fix(delegation): keep child routes and fallback policy together --- cli-config.yaml.example | 4 +- tests/tools/test_delegate_fallback_matrix.py | 150 ++++++++++++++++++ tools/delegate_tool.py | 23 ++- tools/delegate_tool_config.py | 79 ++++++++- .../user-guide/features/fallback-providers.md | 4 +- 5 files changed, 248 insertions(+), 12 deletions(-) diff --git a/cli-config.yaml.example b/cli-config.yaml.example index 6785e52c97..b63a32cdae 100644 --- a/cli-config.yaml.example +++ b/cli-config.yaml.example @@ -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) diff --git a/tests/tools/test_delegate_fallback_matrix.py b/tests/tools/test_delegate_fallback_matrix.py index 24f560c41d..6a18ae94b1 100644 --- a/tests/tools/test_delegate_fallback_matrix.py +++ b/tests/tools/test_delegate_fallback_matrix.py @@ -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() diff --git a/tools/delegate_tool.py b/tools/delegate_tool.py index f4d04c9c42..5a927bf08a 100644 --- a/tools/delegate_tool.py +++ b/tools/delegate_tool.py @@ -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) diff --git a/tools/delegate_tool_config.py b/tools/delegate_tool_config.py index 98603368d0..eb10aeb840 100644 --- a/tools/delegate_tool_config.py +++ b/tools/delegate_tool_config.py @@ -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}, diff --git a/website/docs/user-guide/features/fallback-providers.md b/website/docs/user-guide/features/fallback-providers.md index 3a6b6c8797..82dc096324 100644 --- a/website/docs/user-guide/features/fallback-providers.md +++ b/website/docs/user-guide/features/fallback-providers.md @@ -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` |