From 1f3e8f57a31f0a848b289f08654de179ad4cee76 Mon Sep 17 00:00:00 2001 From: ADITYA <59918965+adity982@users.noreply.github.com> Date: Tue, 11 Aug 2026 18:43:04 +0530 Subject: [PATCH] fix: resolve subagent tools at execution decision (#381) * fix: defer subagent tool resolution Signed-off-by: Aditya Datta * fix: preserve inherited subagent tools * test: cover injected subagent tools * style: format subagent regression --------- Signed-off-by: Aditya Datta --- EvoScientist/EvoScientist.py | 50 +++++++----- EvoScientist/subagents/_factory.py | 4 +- EvoScientist/utils.py | 84 ++++++++----------- tests/test_async_subagent_swap.py | 34 +++++++- tests/test_load_subagents.py | 127 ++++++++++++++++++----------- 5 files changed, 183 insertions(+), 116 deletions(-) diff --git a/EvoScientist/EvoScientist.py b/EvoScientist/EvoScientist.py index dba2fea..212d4ba 100644 --- a/EvoScientist/EvoScientist.py +++ b/EvoScientist/EvoScientist.py @@ -442,7 +442,11 @@ def _fold_expert_subagents(subs: list[dict], tool_registry: dict) -> None: def _maybe_swap_async_subagents( - subs: list, middleware: list | None = None, *, cfg=None + subs: list, + middleware: list | None = None, + *, + tool_registry: dict | None = None, + cfg=None, ) -> list: """Replace ``_async``-flagged sub-agents with ``AsyncSubAgent`` specs when enabled. @@ -458,17 +462,24 @@ def _maybe_swap_async_subagents( Adding a new async sub-agent requires no change here — flip ``async: true`` in its yaml and create the matching deployment graph. - All return paths strip the internal ``_async`` field from sub-agent dicts - before handoff, since deepagents may schema-validate the kwarg. + YAML tool names stay in the internal ``_tool_names`` field until this + decision point. In-process specs resolve them against ``tool_registry``; + swapped remote specs discard them because their graph factory resolves + tools in its own process. All return paths strip internal fields before + handoff, since deepagents may schema-validate the kwargs. When async subagents are actually swapped in and ``middleware`` is provided, appends ``AsyncWatcherMiddleware`` so launches spawn an ``async_notifier`` watcher. """ + from .utils import resolve_subagent_tools + cfg = cfg if cfg is not None else _ensure_config() + tool_registry = tool_registry or {} if not getattr(cfg, "enable_async_subagents", False): - # Async fully disabled — strip the internal flag before handoff. + # Async fully disabled: every spec will run in-process. for s in subs: + resolve_subagent_tools(s, tool_registry) s.pop("_async", None) return subs @@ -482,9 +493,9 @@ def _maybe_swap_async_subagents( "enable_async_subagents=true but langgraph dev is not reachable; " "falling back to in-process sync delegation for all sub-agents." ) - # Strip the internal ``_async`` flag (carried from ``load_subagents``) - # before sub-agents reach deepagents — it's never a deepagents key. + # Every spec falls back to in-process execution. for s in subs: + resolve_subagent_tools(s, tool_registry) s.pop("_async", None) return subs @@ -496,6 +507,7 @@ def _maybe_swap_async_subagents( if not async_specs: for s in subs: + resolve_subagent_tools(s, tool_registry) s.pop("_async", None) return subs @@ -526,7 +538,7 @@ def _maybe_swap_async_subagents( agent_specs[name] = spec out.append(spec) else: - # Strip the internal flag before handoff to deepagents. + resolve_subagent_tools(s, tool_registry) s.pop("_async", None) out.append(s) @@ -662,21 +674,20 @@ def _build_base_kwargs( tool_registry["tavily_search"] = tavily_search base_tools = [think_tool, skill_manager] - # ``async_swap_pending=True`` because ``_maybe_swap_async_subagents`` - # below re-resolves tools for async subagents against the deployed - # graph's own registry (via ``subagents/_factory.py``). A tool missing - # from ``tool_registry`` for an async spec logs at DEBUG, not WARNING. subs = load_subagents( SUBAGENTS_CONFIG, - tool_registry=tool_registry, - async_swap_pending=True, ) _fold_expert_subagents(subs, tool_registry) _ensure_general_purpose_subagent(subs) _inject_subagent_middleware( subs, workspace_dir=workspace_dir, cfg=cfg, chat_model=chat_model ) - subs = _maybe_swap_async_subagents(subs, base_middleware, cfg=cfg) + subs = _maybe_swap_async_subagents( + subs, + base_middleware, + tool_registry=tool_registry, + cfg=cfg, + ) # Route AsyncSubAgent specs (both standard and expert) through # EvoAsyncSubAgentMiddleware so the payload-aware start_async_task tool # replaces upstream's non-parameterisable one. @@ -746,12 +757,8 @@ def load_mcp_and_build_kwargs( mcp_main = mcp_by_agent.pop("main", []) - # Same rationale as ``_build_base_kwargs``: async subagents get - # re-resolved downstream by the deployed graph's factory registry. subs = load_subagents( SUBAGENTS_CONFIG, - tool_registry=registry, - async_swap_pending=True, ) _fold_expert_subagents(subs, registry) @@ -767,7 +774,12 @@ def load_mcp_and_build_kwargs( # Swap selected sub-agents to AsyncSubAgent (must happen AFTER MCP injection # since async sub-agents are remote graphs that load their own tools). - subs = _maybe_swap_async_subagents(subs, base_middleware, cfg=cfg) + subs = _maybe_swap_async_subagents( + subs, + base_middleware, + tool_registry=registry, + cfg=cfg, + ) # Mirror the base path: route AsyncSubAgent specs through # EvoAsyncSubAgentMiddleware so the payload-aware start_async_task tool # is the one composed into the main agent. diff --git a/EvoScientist/subagents/_factory.py b/EvoScientist/subagents/_factory.py index ef26f73..ef9f888 100644 --- a/EvoScientist/subagents/_factory.py +++ b/EvoScientist/subagents/_factory.py @@ -52,7 +52,7 @@ def build_async_subagent_graph(name: str) -> Any: _inject_subagent_middleware, ) from EvoScientist.tools import skill_manager, tavily_search, think_tool - from EvoScientist.utils import load_subagents + from EvoScientist.utils import load_subagents, resolve_subagent_tools # Surface API keys as env vars so downstream SDKs (openai, anthropic, …) # find them on subprocess invocations from langgraph dev. @@ -68,7 +68,6 @@ def build_async_subagent_graph(name: str) -> Any: # all wired the same way as the in-process sync version. specs = load_subagents( SUBAGENTS_CONFIG, - tool_registry=tool_registry, ) spec = next((s for s in specs if s.get("name") == name), None) if spec is None: @@ -76,6 +75,7 @@ def build_async_subagent_graph(name: str) -> Any: f"Sub-agent {name!r} not found in {SUBAGENTS_CONFIG}. " f"Available: {[s.get('name') for s in specs]}" ) + resolve_subagent_tools(spec, tool_registry) # Load MCP tools routed to THIS agent via ``expose_to: `` in # ``mcp.yaml``. Use the cached helper so multiple ``build_async_subagent_graph`` diff --git a/EvoScientist/utils.py b/EvoScientist/utils.py index 89993ea..9ad09d3 100644 --- a/EvoScientist/utils.py +++ b/EvoScientist/utils.py @@ -18,6 +18,28 @@ logger = logging.getLogger(__name__) console = Console() +def resolve_subagent_tools( + subagent: dict[str, Any], tool_registry: dict[str, Any] +) -> dict[str, Any]: + """Resolve deferred YAML tool names while preserving injected tool objects.""" + tool_names = subagent.pop("_tool_names", None) + if tool_names is None: + return subagent + + resolved = list(subagent.get("tools", [])) + for tool_name in tool_names: + if tool_name in tool_registry: + resolved.append(tool_registry[tool_name]) + else: + logger.warning( + "Subagent %r: tool %r not in registry, skipping", + subagent.get("name"), + tool_name, + ) + subagent["tools"] = resolved + return subagent + + def format_message_content(message): """Convert message content to displayable string. @@ -112,11 +134,9 @@ def show_prompt(prompt_text: str, title: str = "Prompt", border_style: str = "bl def load_subagents( config_path: Path, *, - tool_registry: dict[str, Any], prompt_refs: dict[str, str] | None = None, - async_swap_pending: bool = False, ) -> list[dict[str, Any]]: - """Load subagent definitions from a directory of YAML files and wire up tools. + """Load subagent definitions from a directory of YAML files. NOTE: This is a custom utility. deepagents does not natively load subagents from files - they're normally defined inline in the create_deep_agent() call. @@ -141,16 +161,9 @@ def load_subagents( system_prompt: | ... - ``async_swap_pending`` tells the loader that an async-subagent swap will - run downstream against a different tool registry (typically the deployed - graph's own registry in ``subagents/_factory.py``). When True, a tool - missing from ``tool_registry`` for an ``async: true`` spec is logged at - DEBUG (it will be re-resolved by the async graph). When False, the - caller IS the terminal registry — any missing tool is a genuine typo - and logs at WARNING. Sync in-process callers - (``EvoScientist._build_base_kwargs``, ``load_mcp_and_build_kwargs``) - should pass True; the factory - (``subagents/_factory.build_async_subagent_graph``) leaves it False. + Tool names are retained until the caller selects an execution mode. This + avoids resolving async specs against the in-process registry; the selected + in-process or remote graph resolves names against its terminal registry. """ prompt_refs = prompt_refs or {} @@ -224,9 +237,14 @@ def load_subagents( if "skills" in spec: subagent["skills"] = spec["skills"] - # Compute the async flag up-front so the tools-resolution block below - # can pick the right log level for missing tools. Internal field: - # carries the ``async:`` yaml flag through to + # Defer YAML-name resolution until the caller decides whether this + # spec will run in-process. Async specs are replaced by remote graph + # references and must not warn against the caller's unrelated registry. + if "tools" in spec: + subagent["_tool_names"] = list(spec["tools"]) + subagent["tools"] = [] + + # Internal field: carries the ``async:`` yaml flag through to # ``_maybe_swap_async_subagents`` so the swap doesn't need a second # yaml pass to discover async-flagged agents. Underscore prefix marks # it as internal — must be popped before passing to deepagents. @@ -240,30 +258,6 @@ def load_subagents( f"got {type(async_val).__name__}: {async_val!r}" ) - if "tools" in spec: - resolved = [] - for t in spec["tools"]: - if t in tool_registry: - resolved.append(tool_registry[t]) - elif async_val and async_swap_pending: - # Caller expects an async-subagent swap downstream against - # a different registry. The in-process registry is - # intentionally minimal for async agents — a miss here is - # not degraded state. - logger.debug( - "Subagent %r: tool %r not in the in-process registry; " - "resolved by the async graph", - name, - t, - ) - else: - # Terminal registry (factory) OR sync subagent — a missing - # tool is a genuine typo and won't be resolved anywhere. - logger.warning( - "Subagent %r: tool %r not in registry, skipping", name, t - ) - subagent["tools"] = resolved - subagent["_async"] = async_val return subagent @@ -280,18 +274,12 @@ def load_subagent( *, tool_registry: dict[str, Any], prompt_refs: dict[str, str] | None = None, - async_swap_pending: bool = False, ) -> dict[str, Any]: - """Load a single sub-agent by name from YAML. - - See :func:`load_subagents` for ``async_swap_pending`` semantics. - """ + """Load and resolve a single sub-agent by name from YAML.""" for agent in load_subagents( config_path, - tool_registry=tool_registry, prompt_refs=prompt_refs, - async_swap_pending=async_swap_pending, ): if agent.get("name") == name: - return agent + return resolve_subagent_tools(agent, tool_registry) raise KeyError(f"Sub-agent not found: {name}") diff --git a/tests/test_async_subagent_swap.py b/tests/test_async_subagent_swap.py index faa2439..a861690 100644 --- a/tests/test_async_subagent_swap.py +++ b/tests/test_async_subagent_swap.py @@ -13,13 +13,20 @@ from unittest.mock import patch from EvoScientist.EvoScientist import _maybe_swap_async_subagents -def _sub(name: str, *, async_flag: bool, description: str = "desc") -> dict: +def _sub( + name: str, + *, + async_flag: bool, + description: str = "desc", + tool_names: list[str] | None = None, +) -> dict: """Build a sub-agent dict shaped like ``utils.load_subagents`` output.""" return { "name": name, "description": description, "system_prompt": "x", "tools": [], + "_tool_names": tool_names or [], "_async": async_flag, } @@ -45,6 +52,7 @@ def test_returns_unchanged_when_async_disabled_and_strips_flag(): assert out is subs for s in out: assert "_async" not in s, f"_async leaked into {s['name']}" + assert "_tool_names" not in s # ============================================================================= @@ -158,6 +166,30 @@ def test_swaps_async_flagged_subs(): assert data["url"] == "http://127.0.0.1:6174" +def test_only_in_process_specs_resolve_against_the_caller_registry(): + cfg = SimpleNamespace(enable_async_subagents=True, langgraph_dev_port=6174) + sync_tool = object() + subs = [ + _sub("planner-agent", async_flag=False, tool_names=["think_tool"]), + _sub("writing-agent", async_flag=True, tool_names=["remote_only"]), + ] + with patch( + "EvoScientist.langgraph_dev.manager.is_async_subagents_available", + return_value=True, + ): + out = _maybe_swap_async_subagents( + subs, + tool_registry={"think_tool": sync_tool}, + cfg=cfg, + ) + + planner = next(s for s in out if s["name"] == "planner-agent") + assert planner["tools"] == [sync_tool] + assert "_tool_names" not in planner + writing = next(s for s in out if s["name"] == "writing-agent") + assert writing["graph_id"] == "writing-agent" + + def test_swap_uses_configured_port(): """AsyncSubAgent.url should reflect cfg.langgraph_dev_port, not hardcoded.""" cfg = SimpleNamespace(enable_async_subagents=True, langgraph_dev_port=9999) diff --git a/tests/test_load_subagents.py b/tests/test_load_subagents.py index 951cf7c..44364f5 100644 --- a/tests/test_load_subagents.py +++ b/tests/test_load_subagents.py @@ -11,7 +11,7 @@ import textwrap import pytest -from EvoScientist.utils import load_subagents +from EvoScientist.utils import load_subagents, resolve_subagent_tools def _write_yaml(tmp_path, name: str, body: str): @@ -33,7 +33,7 @@ def test_async_flag_accepts_real_bool(tmp_path): async: true """, ) - subs = load_subagents(config_path, tool_registry={}) + subs = load_subagents(config_path) assert len(subs) == 1 assert subs[0]["name"] == "writing-agent" assert subs[0]["_async"] is True @@ -51,10 +51,67 @@ def test_async_flag_defaults_to_false_when_omitted(tmp_path): tools: [] """, ) - subs = load_subagents(config_path, tool_registry={}) + subs = load_subagents(config_path) assert subs[0]["_async"] is False +def test_tool_names_are_deferred_until_the_spec_is_selected(tmp_path, caplog): + selected_tool = object() + config_path = _write_yaml( + tmp_path, + "agents.yaml", + """ + selected-agent: + tools: [available] + remote-agent: + tools: [remote_only] + async: true + """, + ) + + subs = load_subagents(config_path) + + assert not caplog.records + assert subs[0]["tools"] == [] + assert subs[0]["_tool_names"] == ["available"] + resolve_subagent_tools(subs[0], {"available": selected_tool}) + assert subs[0]["tools"] == [selected_tool] + assert "_tool_names" not in subs[0] + + +def test_resolve_subagent_tools_preserves_injected_tools(): + injected = object() + selected_tool = object() + subagent = { + "name": "selected-agent", + "tools": [injected], + "_tool_names": ["available"], + } + + resolve_subagent_tools(subagent, {"available": selected_tool}) + + assert subagent["tools"] == [injected, selected_tool] + assert "_tool_names" not in subagent + + +def test_agent_without_tools_preserves_parent_tool_inheritance(tmp_path): + config_path = _write_yaml( + tmp_path, + "general.yaml", + """ + general-purpose: + description: Handles general tasks + system_prompt: "" + """, + ) + + subagent = load_subagents(config_path)[0] + resolve_subagent_tools(subagent, {"available": object()}) + + assert "tools" not in subagent + assert "_tool_names" not in subagent + + def test_async_flag_rejects_quoted_string(tmp_path): """``async: "false"`` (quoted) is a real user trap — bool("false") is True. @@ -73,7 +130,7 @@ def test_async_flag_rejects_quoted_string(tmp_path): """, ) with pytest.raises(ValueError, match=r"'async' must be a boolean"): - load_subagents(config_path, tool_registry={}) + load_subagents(config_path) def test_async_flag_rejects_integer(tmp_path): @@ -90,7 +147,7 @@ def test_async_flag_rejects_integer(tmp_path): """, ) with pytest.raises(ValueError, match=r"'async' must be a boolean"): - load_subagents(config_path, tool_registry={}) + load_subagents(config_path) def test_async_flag_error_includes_agent_name(tmp_path): @@ -107,7 +164,7 @@ def test_async_flag_error_includes_agent_name(tmp_path): """, ) with pytest.raises(ValueError, match=r"my-bad-agent"): - load_subagents(config_path, tool_registry={}) + load_subagents(config_path) def test_non_dict_spec_raises(tmp_path): @@ -125,7 +182,7 @@ def test_non_dict_spec_raises(tmp_path): """, ) with pytest.raises(ValueError, match=r"must map to a spec dict"): - load_subagents(config_path, tool_registry={}) + load_subagents(config_path) def test_non_dict_spec_error_includes_filename_and_name(tmp_path): @@ -138,16 +195,11 @@ def test_non_dict_spec_error_includes_filename_and_name(tmp_path): """, ) with pytest.raises(ValueError, match=r"weird\.yaml.*weird-agent"): - load_subagents(config_path, tool_registry={}) + load_subagents(config_path) -def test_missing_tool_on_sync_subagent_logs_warning(tmp_path, caplog): - """Sync sub-agents with a tool missing from the registry log at WARNING. - - Sync sub-agents run in-process under the main agent and rely on the - in-process registry to wire every tool they declare. A missing tool - IS a genuine degradation — surfaces it as a warning. - """ +def test_missing_tool_on_sync_subagent_warns_at_resolution(tmp_path, caplog): + """A selected sync spec warns when its terminal registry lacks a tool.""" config_path = _write_yaml( tmp_path, "planner.yaml", @@ -159,23 +211,19 @@ def test_missing_tool_on_sync_subagent_logs_warning(tmp_path, caplog): """, ) with caplog.at_level("DEBUG", logger="EvoScientist.utils"): - subs = load_subagents(config_path, tool_registry={}) + subs = load_subagents(config_path) assert subs[0]["_async"] is False assert subs[0]["tools"] == [] warnings = [r for r in caplog.records if r.levelname == "WARNING"] + assert not warnings + + resolve_subagent_tools(subs[0], {}) + warnings = [r for r in caplog.records if r.levelname == "WARNING"] assert any("nonexistent_tool" in r.getMessage() for r in warnings) -def test_missing_tool_on_async_subagent_logs_debug_when_swap_pending(tmp_path, caplog): - """Sync in-process callers pass ``async_swap_pending=True`` — a tool - missing for an ``async: true`` spec logs at DEBUG because the async - graph's own registry will re-resolve it downstream - (``subagents/_factory.py``). - - Regression guard for the spurious startup WARNING that pre-fix logs - fired on every ``EvoSci`` startup even though the tool was wired at - runtime by the deployed graph. - """ +def test_missing_tool_on_async_subagent_is_deferred_without_logging(tmp_path, caplog): + """Async tool names do not emit warnings against the caller registry.""" config_path = _write_yaml( tmp_path, "scheduler.yaml", @@ -188,31 +236,18 @@ def test_missing_tool_on_async_subagent_logs_debug_when_swap_pending(tmp_path, c """, ) with caplog.at_level("DEBUG", logger="EvoScientist.utils"): - subs = load_subagents(config_path, tool_registry={}, async_swap_pending=True) + subs = load_subagents(config_path) assert subs[0]["_async"] is True assert subs[0]["tools"] == [] warnings = [r for r in caplog.records if r.levelname == "WARNING"] assert not any("nonexistent_tool" in r.getMessage() for r in warnings) debugs = [r for r in caplog.records if r.levelname == "DEBUG"] - assert any( - "nonexistent_tool" in r.getMessage() and "async graph" in r.getMessage() - for r in debugs - ) + assert not any("nonexistent_tool" in r.getMessage() for r in debugs) + assert subs[0]["_tool_names"] == ["nonexistent_tool"] -def test_missing_tool_on_async_subagent_logs_warning_at_terminal_registry( - tmp_path, caplog -): - """When ``async_swap_pending`` is False (the default), the caller IS the - terminal registry — the factory boundary - (``subagents/_factory.build_async_subagent_graph``). A tool missing for - an ``async: true`` spec is a genuine typo that won't be resolved anywhere - downstream, so log at WARNING. - - Without this guard, an earlier version of the fix (unconditional DEBUG for - every async spec) silently hid factory-boundary typos. Reviewer flagged - this as the last remaining gap. - """ +def test_missing_async_tool_warns_at_terminal_resolution(tmp_path, caplog): + """The selected async graph warns against its terminal registry.""" config_path = _write_yaml( tmp_path, "scheduler.yaml", @@ -225,8 +260,8 @@ def test_missing_tool_on_async_subagent_logs_warning_at_terminal_registry( """, ) with caplog.at_level("DEBUG", logger="EvoScientist.utils"): - # Default: async_swap_pending=False → factory-boundary semantics. - subs = load_subagents(config_path, tool_registry={}) + subs = load_subagents(config_path) + resolve_subagent_tools(subs[0], {}) assert subs[0]["_async"] is True assert subs[0]["tools"] == [] warnings = [r for r in caplog.records if r.levelname == "WARNING"]