From 76c2a5f860591ecb2d5c6567d94c13cb89ac4b72 Mon Sep 17 00:00:00 2001 From: CK Date: Thu, 9 Jul 2026 13:15:10 +0800 Subject: [PATCH 1/5] feat(delegation): add delegate_tool_reply explicit delivery channel for subagents MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Subagents that emit their deliverable on a turn that also calls a cleanup tool (e.g. terminal rm) lose it: the loop treats that content as mid-task narration, and a later short closing comment overwrites final_response. Heuristics (length, tool category) can't reliably distinguish deliverable from narration — the 0/1-judgment + long-cleanup counterexample proves it. Add an explicit delivery channel: delegate_tool_reply, a leaf-subagent-only tool that hands back the result via a structured tool call. The parent's extraction reads the tool args (authoritative) instead of guessing from final_response. Falls back to final_response when the child never calls it (strictly not-worse-than-status-quo). Visibility (zero core footprint, kanban toolset precedent): - delegation_reply toolset not in _HERMES_CORE_TOOLS / CONFIGURABLE_TOOLSETS - _build_child_agent appends it to every spawned child's toolset - ordinary conversations never see the schema; hermes tools / /tools hide it Extraction priority in _extract_reply_deliverable: 1. spill file (handler writes cache/delegation/delegate_reply_*.txt) — complete, immune to context-compression args truncation 2. tool-call args content (last call in protected tail = intact) 3. multi-call concat in order; truncated-only calls get a marker 4. no call -> final_response (unchanged) Compression resilience: handler spills every call to disk and returns the path; extraction prefers the file over possibly-truncated args. Handler runs in the agent process (like todo/memory), not the terminal sandbox, so the spill lands on the agent host reachable by the parent's read_file regardless of remote (docker/ssh/modal) terminal backends. --- tests/tools/test_delegate.py | 3 +- tests/tools/test_delegate_tool_reply.py | 218 ++++++++++++++++++++++++ tools/delegate_tool_child_run.py | 49 +++++- tools/delegate_tool_progress.py | 13 ++ tools/delegate_tool_reply.py | 159 +++++++++++++++++ tools/delegate_tool_toolsets.py | 3 + toolsets.py | 2 + 7 files changed, 444 insertions(+), 3 deletions(-) create mode 100644 tests/tools/test_delegate_tool_reply.py create mode 100644 tools/delegate_tool_reply.py diff --git a/tests/tools/test_delegate.py b/tests/tools/test_delegate.py index dd3fee084132..c26df41b30d7 100644 --- a/tests/tools/test_delegate.py +++ b/tests/tools/test_delegate.py @@ -1283,7 +1283,6 @@ def test_same_provider_shares_parent_pool(self): # --- Custom-endpoint identity resolution (issue #7833) --- - @patch( "tools.delegate_tool._load_config", return_value={"inherit_mcp_toolsets": False}, @@ -1309,7 +1308,7 @@ def test_build_child_agent_strict_intersection_when_opted_out(self, mock_cfg): self.assertEqual( MockAgent.call_args[1]["enabled_toolsets"], - ["web", "browser"], + ["web", "browser", "delegation_reply"], ) diff --git a/tests/tools/test_delegate_tool_reply.py b/tests/tools/test_delegate_tool_reply.py new file mode 100644 index 000000000000..048d3d7c1377 --- /dev/null +++ b/tests/tools/test_delegate_tool_reply.py @@ -0,0 +1,218 @@ +"""Tests for the ``delegate_tool_reply`` explicit delivery channel. + +Covers: +- handler: spill file + ack result +- extraction: no-call fallback, single call, multi-call concat, truncation, + spill-file path override +- visibility: delegation_reply not in CONFIGURABLE_TOOLSETS, not in default + tool definitions, but present in child toolsets built by _build_child_agent + (validated via toolset resolution) +""" + +import json +import os +import tempfile + +import pytest + +import tools.delegate_tool as dt +from tools.delegate_tool_child_run import _extract_reply_deliverable +import tools.delegate_tool_reply as dtr +from tools.registry import registry + + +# --------------------------------------------------------------------------- +# Handler +# --------------------------------------------------------------------------- + +def test_handler_returns_ack_and_writes_spill(monkeypatch): + with tempfile.TemporaryDirectory() as td: + monkeypatch.setenv("HERMES_HOME", os.path.join(td, ".hermes")) + result = dtr.delegate_tool_reply(content="my deliverable", parent_agent=None) + data = json.loads(result) + assert data["acknowledged"] is True + assert isinstance(data["path"], str) or data["path"] is None + if data["path"]: + with open(data["path"], encoding="utf-8") as f: + assert f.read() == "my deliverable" + + +def test_handler_idempotent_distinct_paths(monkeypatch): + with tempfile.TemporaryDirectory() as td: + monkeypatch.setenv("HERMES_HOME", os.path.join(td, ".hermes")) + r1 = json.loads(dtr.delegate_tool_reply(content="a", parent_agent=None)) + r2 = json.loads(dtr.delegate_tool_reply(content="b", parent_agent=None)) + # Timestamps include microseconds so paths differ. + assert r1["path"] != r2["path"] + + +# --------------------------------------------------------------------------- +# Extraction: _extract_reply_deliverable +# --------------------------------------------------------------------------- + +def _assistant_with_reply(content, tc_id="call_1"): + return { + "role": "assistant", + "content": "", + "tool_calls": [ + { + "id": tc_id, + "type": "function", + "function": { + "name": "delegate_tool_reply", + "arguments": json.dumps({"content": content}), + }, + } + ], + } + + +def _tool_result(tc_id, payload): + return {"role": "tool", "tool_call_id": tc_id, "content": json.dumps(payload)} + + +def test_extraction_no_calls_returns_none(): + msgs = [{"role": "assistant", "content": "just prose", "tool_calls": []}] + assert _extract_reply_deliverable(msgs) is None + + +def test_extraction_single_call_uses_content(): + msgs = [_assistant_with_reply("THE REPORT", "c1")] + assert _extract_reply_deliverable(msgs) == "THE REPORT" + + +def test_extraction_multi_call_concatenates_in_order(): + msgs = [ + _assistant_with_reply("part1", "c1"), + _assistant_with_reply("part2", "c2"), + ] + assert _extract_reply_deliverable(msgs) == "part1\n\npart2" + + +def test_extraction_prefers_spill_file(monkeypatch): + with tempfile.TemporaryDirectory() as td: + monkeypatch.setenv("HERMES_HOME", os.path.join(td, ".hermes")) + # Handler writes the spill file; simulate the tool result pointing to it. + handler_result = json.loads( + dtr.delegate_tool_reply(content="FULL DELIVERABLE", parent_agent=None) + ) + spill_path = handler_result["path"] + # Args truncated by compression (only 200-char head), but spill is complete. + truncated_content = "X" * 200 + dt._REPLY_TRUNCATED_MARKER + msgs = [ + { + "role": "assistant", + "content": "", + "tool_calls": [ + { + "id": "c1", + "type": "function", + "function": { + "name": "delegate_tool_reply", + "arguments": json.dumps({"content": truncated_content}), + }, + } + ], + }, + _tool_result("c1", {"acknowledged": True, "path": spill_path}), + ] + result = _extract_reply_deliverable(msgs) + assert result == "FULL DELIVERABLE" + + +def test_extraction_truncated_without_spill_includes_marker(): + truncated_content = "X" * 200 + dt._REPLY_TRUNCATED_MARKER + msgs = [_assistant_with_reply(truncated_content, "c1")] + result = _extract_reply_deliverable(msgs) + assert result is not None + assert dt._REPLY_TRUNCATED_MARKER in result + assert "truncated by context compression" in result + + +def test_extraction_empty_content_call_returns_empty_string_not_none(): + msgs = [_assistant_with_reply("", "c1")] + # Found the call (returns "" not None), so it signals "child used the tool" + assert _extract_reply_deliverable(msgs) == "" + + +def test_extraction_ignores_other_tools(): + msgs = [ + { + "role": "assistant", + "content": "", + "tool_calls": [ + { + "id": "c1", + "type": "function", + "function": {"name": "terminal", "arguments": "{}"}, + } + ], + }, + {"role": "tool", "tool_call_id": "c1", "content": "{}"}, + ] + assert _extract_reply_deliverable(msgs) is None + + +def test_extraction_empty_messages(): + assert _extract_reply_deliverable([]) is None + assert _extract_reply_deliverable(None) is None + + +# --------------------------------------------------------------------------- +# Visibility / toolset membership +# --------------------------------------------------------------------------- + +def test_delegation_reply_not_in_configurable_toolsets(): + from hermes_cli.tools_config import CONFIGURABLE_TOOLSETS + keys = {ts_key for ts_key, _, _ in CONFIGURABLE_TOOLSETS} + assert "delegation_reply" not in keys + + +def test_delegation_reply_not_in_core_tools(): + from toolsets import _HERMES_CORE_TOOLS + assert "delegate_tool_reply" not in _HERMES_CORE_TOOLS + + +def test_delegation_reply_toolset_resolves(): + from toolsets import resolve_toolset, get_toolset + ts = get_toolset("delegation_reply") + assert ts is not None + assert "delegate_tool_reply" in ts["tools"] + resolved = resolve_toolset("delegation_reply") + assert "delegate_tool_reply" in resolved + + +def test_tool_registered_under_delegation_reply_toolset(): + entry = registry.get_entry("delegate_tool_reply") + assert entry is not None + assert entry.toolset == "delegation_reply" + + +def test_child_toolsets_include_delegation_reply_after_build(monkeypatch): + # Exercise the toolset-assembly branch of _build_child_agent without + # constructing a full AIAgent. We replicate the final assembly steps + # (the lines after _strip_blocked_tools) to assert delegation_reply is + # appended unconditionally. + child_toolsets = ["terminal", "file"] + if "delegation_reply" not in child_toolsets: + child_toolsets.append("delegation_reply") + assert "delegation_reply" in child_toolsets + + +# --------------------------------------------------------------------------- +# System prompt discipline injection +# --------------------------------------------------------------------------- + +def test_system_prompt_includes_delivery_discipline(): + prompt = dt._build_child_system_prompt( + "do the task", role="leaf", max_spawn_depth=2, child_depth=1, + ) + assert "delegate_tool_reply" in prompt + assert "Delivery Discipline" in prompt + + +def test_system_prompt_discipline_present_for_orchestrator(): + prompt = dt._build_child_system_prompt( + "do the task", role="orchestrator", max_spawn_depth=2, child_depth=1, + ) + assert "delegate_tool_reply" in prompt diff --git a/tools/delegate_tool_child_run.py b/tools/delegate_tool_child_run.py index 036e83f7bb2b..092ca50db943 100644 --- a/tools/delegate_tool_child_run.py +++ b/tools/delegate_tool_child_run.py @@ -475,13 +475,60 @@ def _build_tool_trace(messages: Any) -> list[Dict[str, Any]]: tool_trace[-1].update(result_meta) # no tool_call_id: pair with the latest call return tool_trace +def _extract_reply_deliverable(messages: list) -> Optional[str]: + """Join explicit deliveries in transcript order, preferring complete spill files.""" + if not isinstance(messages, list) or not messages: + return None + result_by_id = { + msg["tool_call_id"]: _stringify_tool_content(msg.get("content", "")) + for msg in messages if isinstance(msg, dict) and msg.get("role") == "tool" and msg.get("tool_call_id") + } + chunks = [] + found = False + for msg in messages: + if not isinstance(msg, dict) or msg.get("role") != "assistant": + continue + for tc in msg.get("tool_calls") or []: + if not isinstance(tc, dict): + continue + fn = tc.get("function", {}) + if fn.get("name") != "delegate_tool_reply": + continue + found = True + content_arg = "" + try: + parsed = json.loads(fn.get("arguments") or "{}") + if isinstance(parsed, dict): + content_arg = parsed.get("content", "") + if not isinstance(content_arg, str): + content_arg = str(content_arg) if content_arg is not None else "" + except (ValueError, TypeError): + pass + spill_content = None + try: + res_obj = json.loads(result_by_id.get(tc.get("id"), "{}")) + if isinstance(res_obj, dict) and isinstance(res_obj.get("path"), str) and res_obj["path"]: + from pathlib import Path + spill_content = Path(res_obj["path"]).read_text(encoding="utf-8") + except (ValueError, TypeError, OSError): + pass + if spill_content is not None: + chunks.append(spill_content) + elif content_arg: + if content_arg.endswith("...[truncated]"): + content_arg += "\n[NOTE: this chunk was truncated by context compression; spill file unreadable]" + chunks.append(content_arg) + return "\n\n".join(chunks) if found else None + + def _build_result_entry( child: Any, result: Dict[str, Any], task_index: int, duration: float, schema: _SchemaOutcome, ) -> Dict[str, Any]: """Parent-visible result entry (status, exit_reason, tool trace, tokens, cost). ``status``/``exit_reason``/``truncated`` follow the ``_run_single_child`` contract; a structured failure always wins over the summary-presence heuristic (a fallback for legacy/mock results only).""" - summary = result.get("final_response") or "" + delivery = _extract_reply_deliverable(result.get("messages") or []) + summary = delivery if delivery is not None else result.get("final_response") or "" # "(empty)" is run_agent's give-up sentinel after repeated empty LLM # responses (usually a transport bug) — a failure, not a success. usable_summary = bool(summary) and summary.strip() != "(empty)" diff --git a/tools/delegate_tool_progress.py b/tools/delegate_tool_progress.py index 46e913f88e6f..f5ce2752461a 100644 --- a/tools/delegate_tool_progress.py +++ b/tools/delegate_tool_progress.py @@ -175,6 +175,19 @@ def _build_child_system_prompt( if _ctx_files.strip(): parts.append(_CONTEXT_FILES_INTRO + _ctx_files.strip()) parts.append(_COMPLETION_INSTRUCTIONS) + parts.append( + "\n## Delivery Discipline\n" + "Your real result must go through the `delegate_tool_reply` tool. " + "When your deliverable is ready, call `delegate_tool_reply` with the " + "full text as `content`. Do NOT rely on a trailing prose message as " + "your result: a short closing comment about cleanup can be mistaken " + "for your deliverable and the real content lost. You may call " + "`delegate_tool_reply` more than once: chunks are concatenated in " + "order, so a large deliverable can be split across calls. After " + "calling it you may still run cleanup tools (it does not stop you). " + "Your final plain-text reply is only used as a fallback if you " + "never call `delegate_tool_reply`." + ) if role == "orchestrator": child_note = _LEAF_CHILDREN_NOTE if child_depth + 1 >= max_spawn_depth else _NESTED_CHILDREN_NOTE parts.append( diff --git a/tools/delegate_tool_reply.py b/tools/delegate_tool_reply.py new file mode 100644 index 000000000000..a4c4ab1bb139 --- /dev/null +++ b/tools/delegate_tool_reply.py @@ -0,0 +1,159 @@ +"""``delegate_tool_reply`` — explicit delivery channel for subagent results. + +A leaf-subagent-only tool that hands the deliverable back to the parent agent +through a structured tool call instead of relying on the trailing +``final_response`` prose. This closes the information-loss bug where a +subagent's real result (emitted on a turn that also called a housekeeping / +cleanup tool like ``terminal``) gets overwritten by a short closing comment +on a later turn. + +Why a tool and not just ``final_response``? + +The agent loop treats content emitted on a tool-calling turn as mid-task +narration (``_last_content_with_tools`` in ``conversation_loop.py``), so it +is **not** promoted to ``final_response`` when a later turn produces text. +For a subagent whose job is to *produce a deliverable*, the trailing prose is +a fragile proxy for the real result — a one-line "done, cleaned up" closer +clobbers the six-block report. ``delegate_tool_reply`` gives the subagent an +explicit, in-band channel: the deliverable is the tool's ``content`` arg, +and the parent's extraction layer reads that arg directly. + +Visibility (zero core footprint): + +This tool lives in the ``delegation_reply`` toolset, which is **not** in +``_HERMES_CORE_TOOLS`` (so no platform bundle auto-includes it) and **not** in +``CONFIGURABLE_TOOLSETS`` (so it never appears in ``hermes tools`` / ``/tools``). +Only ``_build_child_agent`` in ``delegate_tool.py`` adds ``delegation_reply`` +to a child's toolset, so the tool is only ever visible to subagents spawned by +``delegate_task``. Ordinary conversations never see the schema. + +Execution model: + +The handler runs in the agent's Python process (like ``todo`` / ``memory``), +not in the terminal sandbox — so the spill file lands on the agent host and is +reachable by the parent's ``read_file`` regardless of whether the child's +terminal points at a remote Docker / SSH / Modal backend. + +Compression resilience: + +If a long-running subagent triggers context compression, the compressor +(``context_compressor.py``) truncates ``tool_call`` args > 500 chars for +assistant messages outside the protected tail. To survive that, the handler +spills the full ``content`` to ``cache/delegation/delegate_reply_*.txt`` and +returns the absolute path in its result; the extraction layer prefers the +spill file over the (possibly truncated) args. +""" +from __future__ import annotations + +import json +import logging +from typing import Optional + +from tools.registry import registry + +logger = logging.getLogger(__name__) + +_TOOL_NAME = "delegate_tool_reply" +_TRUNCATED_MARKER = "...[truncated]" + +DELEGATE_TOOL_REPLY_SCHEMA = { + "name": _TOOL_NAME, + "description": ( + "Hand back your final result to the parent agent. Call this with the " + "complete deliverable text as `content`. You may call it multiple " + "times to deliver in chunks (they are concatenated in order), or " + "update it by calling again — the last value per chunk is used. This " + "does NOT stop you; finish any cleanup afterward. Always deliver your " + "real result through this tool, not as a trailing prose comment." + ), + "parameters": { + "type": "object", + "properties": { + "content": { + "type": "string", + "description": "The complete deliverable text for the parent agent.", + }, + }, + "required": ["content"], + }, +} + + +def _spill_reply_to_file(content: str, subagent_id: Optional[str]) -> Optional[str]: + """Write the deliverable to the delegation cache and return the abs path. + + Mirrors ``_spill_summary_to_file`` in ``delegate_tool.py``: the file lands + in ``cache/delegation`` which is mounted read-only into remote backends + (Docker/Modal/SSH) via ``credential_files._CACHE_DIRS``, so the parent's + ``terminal`` / ``read_file`` tools can page through the complete text on any + backend. Returns the absolute path, or ``None`` on failure (best-effort: + extraction then falls back to the in-memory args). + """ + try: + from hermes_constants import get_hermes_dir + import datetime as _dt + + cache_dir = get_hermes_dir("cache/delegation", "delegation_cache") + cache_dir.mkdir(parents=True, exist_ok=True) + ts = _dt.datetime.now().strftime("%Y%m%d_%H%M%S_%f") + sid = subagent_id or "unknown" + # Sanitize the subagent id for use in a filename. + safe_sid = "".join(c if c.isalnum() or c in "-_" else "_" for c in sid)[:64] + path = cache_dir / f"delegate_reply-{safe_sid}-{ts}.txt" + path.write_text(content, encoding="utf-8") + return str(path) + except Exception as exc: + logger.debug("Failed to spill delegate_tool_reply content to file: %s", exc) + return None + + +def delegate_tool_reply(content: str, parent_agent=None, **kw) -> str: + """Acknowledge a subagent deliverable and spill it to disk. + + No side effects beyond writing the spill file. Does **not** terminate the + subagent loop — the child keeps running (e.g. cleanup) to natural end. + The parent's extraction layer (``delegate_tool.py``) reads this call's + args/result after the child completes. + + Args: + content: the full deliverable text. + parent_agent: the child AIAgent instance (threaded in by the registry + via ``kw["parent_agent"]``); used only for the subagent id when + naming the spill file. + + Returns: + JSON string ``{"acknowledged": true, "path": }``. + """ + subagent_id = getattr(parent_agent, "_subagent_id", None) if parent_agent is not None else None + spill_path = _spill_reply_to_file(content, subagent_id) + return json.dumps( + {"acknowledged": True, "path": spill_path}, + ensure_ascii=False, + ) + + +def _handle_delegate_tool_reply(args, **kw): + content = args.get("content", "") + if not isinstance(content, str): + content = str(content) if content is not None else "" + return delegate_tool_reply(content=content, parent_agent=kw.get("parent_agent")) + + +def check_delegate_reply_requirements() -> bool: + """No external requirements — always available when the toolset is enabled. + + Mirrors ``check_delegate_requirements`` in ``delegate_tool.py``: visibility + is governed purely by toolset membership (the ``delegation_reply`` + toolset is only added to child agents by ``_build_child_agent``). + """ + return True + + +registry.register( + name=_TOOL_NAME, + toolset="delegation_reply", + schema=DELEGATE_TOOL_REPLY_SCHEMA, + handler=_handle_delegate_tool_reply, + check_fn=check_delegate_reply_requirements, + emoji="📨", +) \ No newline at end of file diff --git a/tools/delegate_tool_toolsets.py b/tools/delegate_tool_toolsets.py index be75cf95a5fb..cd0afedda160 100644 --- a/tools/delegate_tool_toolsets.py +++ b/tools/delegate_tool_toolsets.py @@ -111,4 +111,7 @@ def _resolve_child_toolsets( child_disabled_toolsets = list( dict.fromkeys(inherited_disabled + _blocked_toolsets_for_role(effective_role) + ["kanban"]) ) + # Both leaf and orchestrator children produce deliverables for their parent. + if "delegation_reply" not in child_toolsets: + child_toolsets.append("delegation_reply") return child_toolsets, child_disabled_toolsets diff --git a/toolsets.py b/toolsets.py index 24c58b967719..4cec870dd60e 100644 --- a/toolsets.py +++ b/toolsets.py @@ -144,6 +144,8 @@ def _core_without(*excluded, kanban=True): "clarify": _ts("Ask the user clarifying questions (multiple-choice or open-ended)", ["clarify"]), "code_execution": _ts("Run Python scripts that call tools programmatically (reduces LLM round trips)", ["execute_code"]), "delegation": _ts("Spawn subagents with isolated context for complex subtasks", ["delegate_task"]), + # Granted only during child construction, not in core bundles or configurable menus. + "delegation_reply": _ts("Subagent-only explicit result delivery", ["delegate_tool_reply"]), "homeassistant": _ts("Home Assistant smart home control and monitoring", _HA_TOOLS), "kanban": _ts( "Kanban multi-agent coordination — only active when the agent is spawned by " From b3672c64fe0d3d0dbf79a99cabcbfdd1245ab45d Mon Sep 17 00:00:00 2001 From: CK Date: Sun, 12 Jul 2026 13:41:28 +0800 Subject: [PATCH 2/5] fix(delegation): compression-safe delivery via agent-instance state Address teknium1 review on PR #61332: 1. Compression resilience (the core issue): the previous extractor scanned result["messages"] for delegate_tool_reply tool calls + spill paths. But context_compressor.py Phase 4 replaces the entire middle transcript with a summary - a delivery call that falls into the compacted window loses both its args AND its tool-result spill path. Fix: the handler now appends content to child._delegate_reply_chunks (agent-instance state, outside messages[]) at execution time. _extract_reply_deliverable reads from the agent instance, not the transcript. Compression cannot touch it. 2. Schema semantics: removed contradictory update/replace wording. The schema now states append-only: every call content is concatenated in order. The extractor matches - pure append, no replace. 3. Tests: replaced the hand-written child_toolsets.append() test with the constructor-capture pattern from test_delegate.py:1837 (patch run_agent.AIAgent, call _build_child_agent, assert enabled_toolsets contains delegation_reply). Added compression regression test proving the deliverable survives when messages is replaced by a synthetic summary. --- tests/tools/test_delegate_tool_reply.py | 250 ++++++++++++------------ tools/delegate_tool_child_run.py | 50 +---- tools/delegate_tool_reply.py | 66 ++++--- 3 files changed, 179 insertions(+), 187 deletions(-) diff --git a/tests/tools/test_delegate_tool_reply.py b/tests/tools/test_delegate_tool_reply.py index 048d3d7c1377..997e85aaaae2 100644 --- a/tests/tools/test_delegate_tool_reply.py +++ b/tests/tools/test_delegate_tool_reply.py @@ -1,17 +1,19 @@ """Tests for the ``delegate_tool_reply`` explicit delivery channel. Covers: -- handler: spill file + ack result -- extraction: no-call fallback, single call, multi-call concat, truncation, - spill-file path override +- handler: records to agent-instance state + spill file + ack result +- extraction: no-call fallback, single call, multi-call append, compression + safety (agent-instance state survives when messages[] is replaced) - visibility: delegation_reply not in CONFIGURABLE_TOOLSETS, not in default tool definitions, but present in child toolsets built by _build_child_agent - (validated via toolset resolution) + (validated via constructor-capture pattern from test_delegate.py) +- system prompt discipline injection """ import json import os import tempfile +from unittest.mock import MagicMock, patch import pytest @@ -25,143 +27,162 @@ # Handler # --------------------------------------------------------------------------- -def test_handler_returns_ack_and_writes_spill(monkeypatch): +def test_handler_records_to_agent_instance_and_spills(monkeypatch): with tempfile.TemporaryDirectory() as td: monkeypatch.setenv("HERMES_HOME", os.path.join(td, ".hermes")) - result = dtr.delegate_tool_reply(content="my deliverable", parent_agent=None) + agent = MagicMock() + agent._subagent_id = "child-123" + result = dtr.delegate_tool_reply(content="my deliverable", parent_agent=agent) data = json.loads(result) assert data["acknowledged"] is True - assert isinstance(data["path"], str) or data["path"] is None + # Agent-instance state recorded + assert hasattr(agent, "_delegate_reply_chunks") + assert agent._delegate_reply_chunks == ["my deliverable"] + # Spill file written if data["path"]: with open(data["path"], encoding="utf-8") as f: assert f.read() == "my deliverable" -def test_handler_idempotent_distinct_paths(monkeypatch): +def test_handler_multi_call_appends_to_instance_list(monkeypatch): with tempfile.TemporaryDirectory() as td: monkeypatch.setenv("HERMES_HOME", os.path.join(td, ".hermes")) - r1 = json.loads(dtr.delegate_tool_reply(content="a", parent_agent=None)) - r2 = json.loads(dtr.delegate_tool_reply(content="b", parent_agent=None)) - # Timestamps include microseconds so paths differ. - assert r1["path"] != r2["path"] + agent = MagicMock() + agent._subagent_id = "child-456" + dtr.delegate_tool_reply(content="chunk1", parent_agent=agent) + dtr.delegate_tool_reply(content="chunk2", parent_agent=agent) + assert agent._delegate_reply_chunks == ["chunk1", "chunk2"] + + +def test_handler_no_agent_still_spills(monkeypatch): + with tempfile.TemporaryDirectory() as td: + monkeypatch.setenv("HERMES_HOME", os.path.join(td, ".hermes")) + result = dtr.delegate_tool_reply(content="orphan deliverable", parent_agent=None) + data = json.loads(result) + assert data["acknowledged"] is True # --------------------------------------------------------------------------- -# Extraction: _extract_reply_deliverable +# Extraction: _extract_reply_deliverable (reads from agent instance) # --------------------------------------------------------------------------- -def _assistant_with_reply(content, tc_id="call_1"): - return { - "role": "assistant", - "content": "", - "tool_calls": [ - { - "id": tc_id, - "type": "function", - "function": { - "name": "delegate_tool_reply", - "arguments": json.dumps({"content": content}), - }, - } - ], - } +def test_extraction_no_chunks_returns_none(): + child = MagicMock() + # No _delegate_reply_chunks attribute set + del child._delegate_reply_chunks + assert _extract_reply_deliverable(child) is None -def _tool_result(tc_id, payload): - return {"role": "tool", "tool_call_id": tc_id, "content": json.dumps(payload)} +def test_extraction_single_chunk(): + child = MagicMock() + child._delegate_reply_chunks = ["THE REPORT"] + assert _extract_reply_deliverable(child) == "THE REPORT" -def test_extraction_no_calls_returns_none(): - msgs = [{"role": "assistant", "content": "just prose", "tool_calls": []}] - assert _extract_reply_deliverable(msgs) is None +def test_extraction_multi_chunk_appends_in_order(): + child = MagicMock() + child._delegate_reply_chunks = ["part1", "part2"] + assert _extract_reply_deliverable(child) == "part1\n\npart2" -def test_extraction_single_call_uses_content(): - msgs = [_assistant_with_reply("THE REPORT", "c1")] - assert _extract_reply_deliverable(msgs) == "THE REPORT" +def test_extraction_empty_list_returns_empty_string_not_none(): + child = MagicMock() + child._delegate_reply_chunks = [] + assert _extract_reply_deliverable(child) == "" -def test_extraction_multi_call_concatenates_in_order(): - msgs = [ - _assistant_with_reply("part1", "c1"), - _assistant_with_reply("part2", "c2"), - ] - assert _extract_reply_deliverable(msgs) == "part1\n\npart2" +def test_extraction_non_list_returns_none(): + child = MagicMock() + child._delegate_reply_chunks = "not a list" + assert _extract_reply_deliverable(child) is None -def test_extraction_prefers_spill_file(monkeypatch): - with tempfile.TemporaryDirectory() as td: - monkeypatch.setenv("HERMES_HOME", os.path.join(td, ".hermes")) - # Handler writes the spill file; simulate the tool result pointing to it. - handler_result = json.loads( - dtr.delegate_tool_reply(content="FULL DELIVERABLE", parent_agent=None) - ) - spill_path = handler_result["path"] - # Args truncated by compression (only 200-char head), but spill is complete. - truncated_content = "X" * 200 + dt._REPLY_TRUNCATED_MARKER - msgs = [ - { - "role": "assistant", - "content": "", - "tool_calls": [ - { - "id": "c1", - "type": "function", - "function": { - "name": "delegate_tool_reply", - "arguments": json.dumps({"content": truncated_content}), - }, - } - ], - }, - _tool_result("c1", {"acknowledged": True, "path": spill_path}), - ] - result = _extract_reply_deliverable(msgs) - assert result == "FULL DELIVERABLE" - - -def test_extraction_truncated_without_spill_includes_marker(): - truncated_content = "X" * 200 + dt._REPLY_TRUNCATED_MARKER - msgs = [_assistant_with_reply(truncated_content, "c1")] - result = _extract_reply_deliverable(msgs) - assert result is not None - assert dt._REPLY_TRUNCATED_MARKER in result - assert "truncated by context compression" in result - - -def test_extraction_empty_content_call_returns_empty_string_not_none(): - msgs = [_assistant_with_reply("", "c1")] - # Found the call (returns "" not None), so it signals "child used the tool" - assert _extract_reply_deliverable(msgs) == "" - - -def test_extraction_ignores_other_tools(): - msgs = [ - { - "role": "assistant", - "content": "", - "tool_calls": [ - { - "id": "c1", - "type": "function", - "function": {"name": "terminal", "arguments": "{}"}, - } - ], - }, - {"role": "tool", "tool_call_id": "c1", "content": "{}"}, +# --------------------------------------------------------------------------- +# Compression safety regression (the core teknium1 review point) +# --------------------------------------------------------------------------- + +def test_extraction_survives_context_compression(): + """The deliverable is read from agent-instance state, not messages[]. + + Context compression replaces the middle of messages[] with a summary + (context_compressor.py Phase 4). This test proves that even if messages[] + is completely replaced by a synthetic summary, the deliverable recorded on + the agent instance is intact — because the handler wrote it at execution + time, outside the transcript. + """ + child = MagicMock() + child._delegate_reply_chunks = ["FULL AUDIT REPORT"] + # Simulate compression: messages[] is now a synthetic summary, no + # delegate_tool_reply tool calls remain in it. + compressed_messages = [ + {"role": "user", "content": "do the audit"}, + {"role": "assistant", "content": "[Summary of earlier turns: subagent ran audit and delivered results.]"}, + {"role": "assistant", "content": "done"}, ] - assert _extract_reply_deliverable(msgs) is None + # Extraction does NOT read messages — it reads the agent instance. + assert _extract_reply_deliverable(child) == "FULL AUDIT REPORT" + # Even if someone passed messages, it wouldn't matter — the function + # signature takes `child`, not `messages`. -def test_extraction_empty_messages(): - assert _extract_reply_deliverable([]) is None - assert _extract_reply_deliverable(None) is None +def test_extraction_multi_chunk_survives_compression(): + child = MagicMock() + child._delegate_reply_chunks = ["chunk-A", "chunk-B", "chunk-C"] + assert _extract_reply_deliverable(child) == "chunk-A\n\nchunk-B\n\nchunk-C" # --------------------------------------------------------------------------- -# Visibility / toolset membership +# Visibility / toolset membership (constructor-capture pattern) # --------------------------------------------------------------------------- +def _make_mock_parent(): + """Create a mock parent matching test_delegate.py's _make_mock_parent.""" + parent = MagicMock() + parent.base_url = "https://openrouter.ai/api/v1" + parent.api_key = "***" + parent.provider = "openrouter" + parent.api_mode = "chat_completions" + parent.model = "anthropic/claude-sonnet-4" + parent.platform = "cli" + parent.providers_allowed = None + parent.providers_ignored = None + parent.providers_order = None + parent.provider_sort = None + parent._session_db = None + parent._delegate_depth = 0 + parent._active_children = [] + parent._active_children_lock = MagicMock() + parent._print_fn = None + parent.tool_progress_callback = None + parent.thinking_callback = None + return parent + + +def test_build_child_agent_includes_delegation_reply(): + """Exercise the real _build_child_agent, not a hand-written append.""" + parent = _make_mock_parent() + parent.enabled_toolsets = ["terminal", "file"] + + with patch("tools.delegate_tool._load_config", return_value={}): + with patch("run_agent.AIAgent") as MockAgent: + mock_child = MagicMock() + MockAgent.return_value = mock_child + + dt._build_child_agent( + task_index=0, + goal="Test delivery channel", + context=None, + toolsets=["terminal", "file"], + model=None, + max_iterations=10, + parent_agent=parent, + task_count=1, + ) + + enabled = MockAgent.call_args[1]["enabled_toolsets"] + assert "delegation_reply" in enabled + + def test_delegation_reply_not_in_configurable_toolsets(): from hermes_cli.tools_config import CONFIGURABLE_TOOLSETS keys = {ts_key for ts_key, _, _ in CONFIGURABLE_TOOLSETS} @@ -174,12 +195,10 @@ def test_delegation_reply_not_in_core_tools(): def test_delegation_reply_toolset_resolves(): - from toolsets import resolve_toolset, get_toolset + from toolsets import get_toolset ts = get_toolset("delegation_reply") assert ts is not None assert "delegate_tool_reply" in ts["tools"] - resolved = resolve_toolset("delegation_reply") - assert "delegate_tool_reply" in resolved def test_tool_registered_under_delegation_reply_toolset(): @@ -188,17 +207,6 @@ def test_tool_registered_under_delegation_reply_toolset(): assert entry.toolset == "delegation_reply" -def test_child_toolsets_include_delegation_reply_after_build(monkeypatch): - # Exercise the toolset-assembly branch of _build_child_agent without - # constructing a full AIAgent. We replicate the final assembly steps - # (the lines after _strip_blocked_tools) to assert delegation_reply is - # appended unconditionally. - child_toolsets = ["terminal", "file"] - if "delegation_reply" not in child_toolsets: - child_toolsets.append("delegation_reply") - assert "delegation_reply" in child_toolsets - - # --------------------------------------------------------------------------- # System prompt discipline injection # --------------------------------------------------------------------------- diff --git a/tools/delegate_tool_child_run.py b/tools/delegate_tool_child_run.py index 092ca50db943..0b19ac7ef1bb 100644 --- a/tools/delegate_tool_child_run.py +++ b/tools/delegate_tool_child_run.py @@ -475,50 +475,12 @@ def _build_tool_trace(messages: Any) -> list[Dict[str, Any]]: tool_trace[-1].update(result_meta) # no tool_call_id: pair with the latest call return tool_trace -def _extract_reply_deliverable(messages: list) -> Optional[str]: - """Join explicit deliveries in transcript order, preferring complete spill files.""" - if not isinstance(messages, list) or not messages: +def _extract_reply_deliverable(child) -> Optional[str]: + """Join append-only deliveries stored on the child, outside its compressible transcript.""" + chunks = getattr(child, "_delegate_reply_chunks", None) + if not isinstance(chunks, list): return None - result_by_id = { - msg["tool_call_id"]: _stringify_tool_content(msg.get("content", "")) - for msg in messages if isinstance(msg, dict) and msg.get("role") == "tool" and msg.get("tool_call_id") - } - chunks = [] - found = False - for msg in messages: - if not isinstance(msg, dict) or msg.get("role") != "assistant": - continue - for tc in msg.get("tool_calls") or []: - if not isinstance(tc, dict): - continue - fn = tc.get("function", {}) - if fn.get("name") != "delegate_tool_reply": - continue - found = True - content_arg = "" - try: - parsed = json.loads(fn.get("arguments") or "{}") - if isinstance(parsed, dict): - content_arg = parsed.get("content", "") - if not isinstance(content_arg, str): - content_arg = str(content_arg) if content_arg is not None else "" - except (ValueError, TypeError): - pass - spill_content = None - try: - res_obj = json.loads(result_by_id.get(tc.get("id"), "{}")) - if isinstance(res_obj, dict) and isinstance(res_obj.get("path"), str) and res_obj["path"]: - from pathlib import Path - spill_content = Path(res_obj["path"]).read_text(encoding="utf-8") - except (ValueError, TypeError, OSError): - pass - if spill_content is not None: - chunks.append(spill_content) - elif content_arg: - if content_arg.endswith("...[truncated]"): - content_arg += "\n[NOTE: this chunk was truncated by context compression; spill file unreadable]" - chunks.append(content_arg) - return "\n\n".join(chunks) if found else None + return "\n\n".join(chunks) def _build_result_entry( @@ -527,7 +489,7 @@ def _build_result_entry( """Parent-visible result entry (status, exit_reason, tool trace, tokens, cost). ``status``/``exit_reason``/``truncated`` follow the ``_run_single_child`` contract; a structured failure always wins over the summary-presence heuristic (a fallback for legacy/mock results only).""" - delivery = _extract_reply_deliverable(result.get("messages") or []) + delivery = _extract_reply_deliverable(child) summary = delivery if delivery is not None else result.get("final_response") or "" # "(empty)" is run_agent's give-up sentinel after repeated empty LLM # responses (usually a transport bug) — a failure, not a success. diff --git a/tools/delegate_tool_reply.py b/tools/delegate_tool_reply.py index a4c4ab1bb139..6b78aa612ff7 100644 --- a/tools/delegate_tool_reply.py +++ b/tools/delegate_tool_reply.py @@ -36,12 +36,24 @@ Compression resilience: -If a long-running subagent triggers context compression, the compressor -(``context_compressor.py``) truncates ``tool_call`` args > 500 chars for -assistant messages outside the protected tail. To survive that, the handler -spills the full ``content`` to ``cache/delegation/delegate_reply_*.txt`` and -returns the absolute path in its result; the extraction layer prefers the -spill file over the (possibly truncated) args. +The deliverable is recorded in **two** places at call time: + +1. **Agent-instance state** (``child._delegate_reply_chunks``) — a list the + handler appends to at execution time. ``_run_single_child`` reads this list + after the child finishes; it is never touched by context compression, + which only mutates the ``messages`` transcript. This is the authoritative + source. +2. **Spill file** (``cache/delegation/delegate_reply_*.txt``) — a backup on + disk, mirroring ``_spill_summary_to_file``. Useful if the agent instance + is somehow lost (e.g. timeout where the future result is unavailable but + the child object still exists). + +Because the agent-instance state lives outside the ``messages`` list, context +compression — which replaces the middle transcript with a summary +(``context_compressor.py`` Phase 4) — cannot destroy the recorded delivery. +The earlier approach of scanning ``result["messages"]`` for tool-call args + +spill paths was vulnerable: a delivery call that fell into the compacted +window lost both its args *and* its tool-result spill path. """ from __future__ import annotations @@ -54,17 +66,16 @@ logger = logging.getLogger(__name__) _TOOL_NAME = "delegate_tool_reply" -_TRUNCATED_MARKER = "...[truncated]" DELEGATE_TOOL_REPLY_SCHEMA = { "name": _TOOL_NAME, "description": ( "Hand back your final result to the parent agent. Call this with the " "complete deliverable text as `content`. You may call it multiple " - "times to deliver in chunks (they are concatenated in order), or " - "update it by calling again — the last value per chunk is used. This " - "does NOT stop you; finish any cleanup afterward. Always deliver your " - "real result through this tool, not as a trailing prose comment." + "times to deliver in chunks — every call's content is appended in " + "order to form the final result. This does NOT stop you; finish any " + "cleanup afterward. Always deliver your real result through this " + "tool, not as a trailing prose comment." ), "parameters": { "type": "object", @@ -86,8 +97,7 @@ def _spill_reply_to_file(content: str, subagent_id: Optional[str]) -> Optional[s in ``cache/delegation`` which is mounted read-only into remote backends (Docker/Modal/SSH) via ``credential_files._CACHE_DIRS``, so the parent's ``terminal`` / ``read_file`` tools can page through the complete text on any - backend. Returns the absolute path, or ``None`` on failure (best-effort: - extraction then falls back to the in-memory args). + backend. Returns the absolute path, or ``None`` on failure (best-effort). """ try: from hermes_constants import get_hermes_dir @@ -108,22 +118,34 @@ def _spill_reply_to_file(content: str, subagent_id: Optional[str]) -> Optional[s def delegate_tool_reply(content: str, parent_agent=None, **kw) -> str: - """Acknowledge a subagent deliverable and spill it to disk. + """Record a subagent deliverable and spill it to disk. - No side effects beyond writing the spill file. Does **not** terminate the - subagent loop — the child keeps running (e.g. cleanup) to natural end. - The parent's extraction layer (``delegate_tool.py``) reads this call's - args/result after the child completes. + Appends ``content`` to ``parent_agent._delegate_reply_chunks`` (the + authoritative store) and writes a spill-file backup. Does **not** terminate + the subagent loop — the child keeps running (e.g. cleanup) to natural end. + ``_run_single_child`` in ``delegate_tool.py`` reads + ``child._delegate_reply_chunks`` after the child completes. Args: - content: the full deliverable text. + content: the full deliverable text (one chunk). parent_agent: the child AIAgent instance (threaded in by the registry - via ``kw["parent_agent"]``); used only for the subagent id when - naming the spill file. + via ``kw["parent_agent"]``). Returns: JSON string ``{"acknowledged": true, "path": }``. """ + if not isinstance(content, str): + content = str(content) if content is not None else "" + + # Record in agent-instance state — compression-safe (not in messages[]). + if parent_agent is not None: + chunks = getattr(parent_agent, "_delegate_reply_chunks", None) + if chunks is None: + chunks = [] + setattr(parent_agent, "_delegate_reply_chunks", chunks) + chunks.append(content) + + # Spill to disk as a backup (best-effort). subagent_id = getattr(parent_agent, "_subagent_id", None) if parent_agent is not None else None spill_path = _spill_reply_to_file(content, subagent_id) return json.dumps( @@ -156,4 +178,4 @@ def check_delegate_reply_requirements() -> bool: handler=_handle_delegate_tool_reply, check_fn=check_delegate_reply_requirements, emoji="📨", -) \ No newline at end of file +) From 0c6d37e5e3d0387cdb95f82aec44006ed74617e4 Mon Sep 17 00:00:00 2001 From: CK Date: Thu, 16 Jul 2026 18:10:43 +0800 Subject: [PATCH 3/5] fix(delegation): intercept delegate_tool_reply as agent-level tool MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit delegate_tool_reply was routed through registry.dispatch which never forwards parent_agent — the handler received None and only produced spill files with 'unknown' subagent ids, never recording chunks on the agent instance. _run_single_child then found empty _delegate_reply_chunks and fell back to trailing final_response prose. Fix: intercept delegate_tool_reply inline in both dispatch paths (tool_executor.py sequential + agent_runtime_helpers.invoke_tool concurrent), passing the agent instance directly — same pattern as todo/memory/clarify/ delegate_task. Add to AGENT_RUNTIME_POST_HOOK_TOOL_NAMES for post-hook ownership tracking. Tests: fix 2 existing MagicMock tests (SimpleNamespace for real attribute semantics), add 2 dispatch-path tests proving registry loses the agent reference while agent-level interception preserves it. --- agent/agent_runtime_helpers.py | 1 + agent/inline_tool_executors.py | 5 ++ tests/tools/test_delegate_tool_reply.py | 69 +++++++++++++++++++++++-- 3 files changed, 71 insertions(+), 4 deletions(-) diff --git a/agent/agent_runtime_helpers.py b/agent/agent_runtime_helpers.py index 4c49896d5c94..1ca37d79d136 100644 --- a/agent/agent_runtime_helpers.py +++ b/agent/agent_runtime_helpers.py @@ -80,6 +80,7 @@ def _ra(): AGENT_RUNTIME_POST_HOOK_TOOL_NAMES = frozenset({ "todo_list", "session_search", "memory", "clarify", "read_terminal", "desktop_preview", "drive_preview", "annotate_preview", "read_window_below", "setup_mcp", "gui_tour", "delegate_task", + "delegate_tool_reply", }) _TRAJECTORY_SYSTEM_PROMPT = ( diff --git a/agent/inline_tool_executors.py b/agent/inline_tool_executors.py index 98b9ef85ea02..50a379c059ed 100644 --- a/agent/inline_tool_executors.py +++ b/agent/inline_tool_executors.py @@ -197,6 +197,11 @@ def _desktop_preview(agent, args: dict, ctx: InlineToolContext) -> Any: ("server", "server", ""), ("action", "action", "install"), ("reason", "reason", ""), ), "delegate_task": lambda agent, args, ctx: agent._dispatch_delegate_task(args), + # Registry dispatch has no executing-agent reference; both tool paths use this table. + "delegate_tool_reply": _tool( + "tools.delegate_tool_reply", "delegate_tool_reply", ("content", "content", ""), + parent_agent=lambda agent, ctx: agent, + ), } # ``invoke_tool`` (concurrent path) consults the memory manager right after these three diff --git a/tests/tools/test_delegate_tool_reply.py b/tests/tools/test_delegate_tool_reply.py index 997e85aaaae2..aae1bd1f4094 100644 --- a/tests/tools/test_delegate_tool_reply.py +++ b/tests/tools/test_delegate_tool_reply.py @@ -13,6 +13,7 @@ import json import os import tempfile +from types import SimpleNamespace from unittest.mock import MagicMock, patch import pytest @@ -27,11 +28,24 @@ # Handler # --------------------------------------------------------------------------- +def _make_agent(subagent_id="child-123"): + """A lightweight agent stand-in with real attribute semantics. + + ``MagicMock()`` returns a mock for ANY ``getattr`` (including + ``_delegate_reply_chunks``), so the handler's ``is None`` check never + fires and the list never gets created. ``SimpleNamespace`` has real + attribute access with ``AttributeError`` on missing attrs, which mirrors + how the handler interacts with a real AIAgent instance. + """ + agent = SimpleNamespace() + agent._subagent_id = subagent_id + return agent + + def test_handler_records_to_agent_instance_and_spills(monkeypatch): with tempfile.TemporaryDirectory() as td: monkeypatch.setenv("HERMES_HOME", os.path.join(td, ".hermes")) - agent = MagicMock() - agent._subagent_id = "child-123" + agent = _make_agent("child-123") result = dtr.delegate_tool_reply(content="my deliverable", parent_agent=agent) data = json.loads(result) assert data["acknowledged"] is True @@ -47,8 +61,7 @@ def test_handler_records_to_agent_instance_and_spills(monkeypatch): def test_handler_multi_call_appends_to_instance_list(monkeypatch): with tempfile.TemporaryDirectory() as td: monkeypatch.setenv("HERMES_HOME", os.path.join(td, ".hermes")) - agent = MagicMock() - agent._subagent_id = "child-456" + agent = _make_agent("child-456") dtr.delegate_tool_reply(content="chunk1", parent_agent=agent) dtr.delegate_tool_reply(content="chunk2", parent_agent=agent) assert agent._delegate_reply_chunks == ["chunk1", "chunk2"] @@ -224,3 +237,51 @@ def test_system_prompt_discipline_present_for_orchestrator(): "do the task", role="orchestrator", max_spawn_depth=2, child_depth=1, ) assert "delegate_tool_reply" in prompt + + +# --------------------------------------------------------------------------- +# Dispatch path — agent-level interception +# --------------------------------------------------------------------------- + +def test_dispatch_via_registry_loses_agent_reference(): + """Registry dispatch does NOT forward ``parent_agent`` — only task_id, + session_id, user_task. This is the root cause of the anchorless delivery + bug: when ``delegate_tool_reply`` was routed through registry.dispatch, + the handler received ``parent_agent=None`` and only produced a spill file + with an "unknown" subagent id, never recording the chunk on the agent + instance. The fix intercepts the tool at the agent level (like todo / + memory / delegate_task) so the agent instance is passed directly. + """ + # Simulate what registry.dispatch forwards to the handler. + result = dtr._handle_delegate_tool_reply( + {"content": "delivered via registry"}, + task_id="t1", + session_id="s1", + ) + data = json.loads(result) + assert data["acknowledged"] is True + # No agent instance → the spill filename uses "unknown" as the subagent id. + # This proves the registry path cannot record on the agent instance. + assert data.get("path") is None or "unknown" in data["path"] + + +def test_agent_level_intercept_passes_agent_to_handler(monkeypatch): + """When the tool_executor intercepts ``delegate_tool_reply`` as an + agent-level tool, it passes the agent instance directly to the handler — + NOT through registry.dispatch. This is the fix for the anchorless delivery + bug where registry.dispatch lost the agent reference.""" + with tempfile.TemporaryDirectory() as td: + monkeypatch.setenv("HERMES_HOME", os.path.join(td, ".hermes")) + agent = _make_agent("child-intercept-test") + # Simulate the inline interception in tool_executor.py / agent_runtime_helpers.py + result = dtr.delegate_tool_reply( + content="delivered via agent-level intercept", + parent_agent=agent, + ) + data = json.loads(result) + assert data["acknowledged"] is True + # Agent-instance state IS recorded when the agent is passed directly. + assert agent._delegate_reply_chunks == ["delivered via agent-level intercept"] + # Spill file uses the real subagent id, not "unknown". + if data.get("path"): + assert "child-intercept-test" in data["path"] From a8dc217bffa4dc5a959304bf0351f1b86f5126d8 Mon Sep 17 00:00:00 2001 From: "hono_hermes(Agent)" Date: Thu, 20 Aug 2026 16:16:12 +0800 Subject: [PATCH 4/5] fix(delegation): adapt explicit replies to current runtime Validate delegate_tool_reply content before output-schema retries and isolate reply chunks between attempts. Keep the hidden delivery tool enabled for children, serialize its calls, preserve current middleware dispatch semantics, and fail closed when agent context is missing. Tests: scripts/run_tests.sh tests/tools/test_delegate*.py tests/run_agent/test_tool_batch_segmentation.py tests/run_agent/test_tool_call_incremental_persistence.py tests/run_agent/test_sequential_tool_timeout.py -q --- agent/tool_dispatch_helpers.py | 2 +- tests/tools/test_delegate_output_schema.py | 36 +++- tests/tools/test_delegate_tool_reply.py | 182 ++++++++++++++------- tools/delegate_tool.py | 2 + tools/delegate_tool_child_run.py | 14 +- tools/delegate_tool_reply.py | 102 ++++-------- tools/delegate_tool_toolsets.py | 2 + 7 files changed, 206 insertions(+), 134 deletions(-) diff --git a/agent/tool_dispatch_helpers.py b/agent/tool_dispatch_helpers.py index 2ec46728d288..ff5c070fa5cd 100644 --- a/agent/tool_dispatch_helpers.py +++ b/agent/tool_dispatch_helpers.py @@ -25,7 +25,7 @@ logger = logging.getLogger(__name__) # Interactive / user-facing tools never run concurrently: any of these in a batch is a barrier. -_NEVER_PARALLEL_TOOLS = frozenset({"clarify", "manage_connections"}) +_NEVER_PARALLEL_TOOLS = frozenset({"clarify", "manage_connections", "delegate_tool_reply"}) # Read-only tools with no shared mutable session state. _PARALLEL_SAFE_TOOLS = frozenset({ diff --git a/tests/tools/test_delegate_output_schema.py b/tests/tools/test_delegate_output_schema.py index 56ea19d3a25f..31ae0ba48785 100644 --- a/tests/tools/test_delegate_output_schema.py +++ b/tests/tools/test_delegate_output_schema.py @@ -167,13 +167,19 @@ class _StubChild: def __init__(self, responses): self.responses = list(responses) self.calls: list = [] + self._delegate_reply_chunks: list[str] = [] def get_activity_summary(self): return {"api_call_count": 1, "max_iterations": 5, "current_tool": None} def run_conversation(self, user_message, task_id=None, **_kwargs): self.calls.append(user_message) - text = self.responses.pop(0) + response = self.responses.pop(0) + if isinstance(response, tuple): + text, reply_chunks = response + self._delegate_reply_chunks.extend(reply_chunks) + else: + text = response return { "final_response": text, "completed": True, @@ -198,6 +204,34 @@ def _run(child): class TestRunSingleChildSchemaValidation: + def test_explicit_delivery_is_validated_before_trailing_prose(self): + child = _StubChild( + [("cleanup complete", ['{"city": "Rome"}'])] + ) + child._delegate_output_schema = ADDRESS_SCHEMA + + entry = _run(child) + + assert entry["schema_valid"] is True + assert entry["summary"] == '{"city": "Rome"}' + assert len(child.calls) == 1 + + def test_schema_retry_replaces_rejected_delivery_attempt(self): + child = _StubChild( + [ + ("first cleanup", ["not json"]), + ("retry cleanup", ['{"city": "Oslo"}']), + ] + ) + child._delegate_output_schema = ADDRESS_SCHEMA + + entry = _run(child) + + assert entry["schema_valid"] is True + assert entry["schema_retries"] == 1 + assert entry["summary"] == '{"city": "Oslo"}' + assert "not json" not in entry["summary"] + def test_valid_first_try_no_retry(self): child = _StubChild(['{"city": "Berlin"}']) child._delegate_output_schema = ADDRESS_SCHEMA diff --git a/tests/tools/test_delegate_tool_reply.py b/tests/tools/test_delegate_tool_reply.py index aae1bd1f4094..8ddcc611d748 100644 --- a/tests/tools/test_delegate_tool_reply.py +++ b/tests/tools/test_delegate_tool_reply.py @@ -1,7 +1,7 @@ """Tests for the ``delegate_tool_reply`` explicit delivery channel. Covers: -- handler: records to agent-instance state + spill file + ack result +- handler: records to agent-instance state and fails closed without it - extraction: no-call fallback, single call, multi-call append, compression safety (agent-instance state survives when messages[] is replaced) - visibility: delegation_reply not in CONFIGURABLE_TOOLSETS, not in default @@ -11,8 +11,6 @@ """ import json -import os -import tempfile from types import SimpleNamespace from unittest.mock import MagicMock, patch @@ -42,37 +40,26 @@ def _make_agent(subagent_id="child-123"): return agent -def test_handler_records_to_agent_instance_and_spills(monkeypatch): - with tempfile.TemporaryDirectory() as td: - monkeypatch.setenv("HERMES_HOME", os.path.join(td, ".hermes")) - agent = _make_agent("child-123") - result = dtr.delegate_tool_reply(content="my deliverable", parent_agent=agent) - data = json.loads(result) - assert data["acknowledged"] is True - # Agent-instance state recorded - assert hasattr(agent, "_delegate_reply_chunks") - assert agent._delegate_reply_chunks == ["my deliverable"] - # Spill file written - if data["path"]: - with open(data["path"], encoding="utf-8") as f: - assert f.read() == "my deliverable" +def test_handler_records_to_agent_instance(): + agent = _make_agent("child-123") + result = dtr.delegate_tool_reply(content="my deliverable", parent_agent=agent) + data = json.loads(result) + assert data == {"acknowledged": True} + assert agent._delegate_reply_chunks == ["my deliverable"] -def test_handler_multi_call_appends_to_instance_list(monkeypatch): - with tempfile.TemporaryDirectory() as td: - monkeypatch.setenv("HERMES_HOME", os.path.join(td, ".hermes")) - agent = _make_agent("child-456") - dtr.delegate_tool_reply(content="chunk1", parent_agent=agent) - dtr.delegate_tool_reply(content="chunk2", parent_agent=agent) - assert agent._delegate_reply_chunks == ["chunk1", "chunk2"] +def test_handler_multi_call_appends_to_instance_list(): + agent = _make_agent("child-456") + dtr.delegate_tool_reply(content="chunk1", parent_agent=agent) + dtr.delegate_tool_reply(content="chunk2", parent_agent=agent) + assert agent._delegate_reply_chunks == ["chunk1", "chunk2"] -def test_handler_no_agent_still_spills(monkeypatch): - with tempfile.TemporaryDirectory() as td: - monkeypatch.setenv("HERMES_HOME", os.path.join(td, ".hermes")) - result = dtr.delegate_tool_reply(content="orphan deliverable", parent_agent=None) - data = json.loads(result) - assert data["acknowledged"] is True +def test_handler_no_agent_fails_closed(): + result = dtr.delegate_tool_reply(content="orphan deliverable", parent_agent=None) + data = json.loads(result) + assert data["acknowledged"] is False + assert "subagent context" in data["error"] # --------------------------------------------------------------------------- @@ -98,10 +85,10 @@ def test_extraction_multi_chunk_appends_in_order(): assert _extract_reply_deliverable(child) == "part1\n\npart2" -def test_extraction_empty_list_returns_empty_string_not_none(): +def test_extraction_empty_list_means_no_delivery_call(): child = MagicMock() child._delegate_reply_chunks = [] - assert _extract_reply_deliverable(child) == "" + assert _extract_reply_deliverable(child) is None def test_extraction_non_list_returns_none(): @@ -196,6 +183,31 @@ def test_build_child_agent_includes_delegation_reply(): assert "delegation_reply" in enabled +def test_parent_disabled_toolsets_cannot_remove_delivery_channel(): + parent = _make_mock_parent() + parent.enabled_toolsets = ["terminal", "file"] + parent.disabled_toolsets = ["browser", "delegation_reply"] + + with patch("tools.delegate_tool._load_config", return_value={}): + with patch("run_agent.AIAgent") as MockAgent: + MockAgent.return_value = MagicMock() + dt._build_child_agent( + task_index=0, + goal="Test delivery channel", + context=None, + toolsets=["terminal", "file"], + model=None, + max_iterations=10, + parent_agent=parent, + task_count=1, + ) + + kwargs = MockAgent.call_args[1] + assert "delegation_reply" in kwargs["enabled_toolsets"] + assert "delegation_reply" not in kwargs["disabled_toolsets"] + assert "browser" in kwargs["disabled_toolsets"] + + def test_delegation_reply_not_in_configurable_toolsets(): from hermes_cli.tools_config import CONFIGURABLE_TOOLSETS keys = {ts_key for ts_key, _, _ in CONFIGURABLE_TOOLSETS} @@ -243,14 +255,13 @@ def test_system_prompt_discipline_present_for_orchestrator(): # Dispatch path — agent-level interception # --------------------------------------------------------------------------- -def test_dispatch_via_registry_loses_agent_reference(): +def test_dispatch_via_registry_fails_without_agent_reference(): """Registry dispatch does NOT forward ``parent_agent`` — only task_id, session_id, user_task. This is the root cause of the anchorless delivery bug: when ``delegate_tool_reply`` was routed through registry.dispatch, - the handler received ``parent_agent=None`` and only produced a spill file - with an "unknown" subagent id, never recording the chunk on the agent - instance. The fix intercepts the tool at the agent level (like todo / - memory / delegate_task) so the agent instance is passed directly. + the handler received ``parent_agent=None`` and could not record the chunk + on the agent instance. The handler must fail closed rather than acknowledge + a result that the parent can never recover. """ # Simulate what registry.dispatch forwards to the handler. result = dtr._handle_delegate_tool_reply( @@ -259,29 +270,88 @@ def test_dispatch_via_registry_loses_agent_reference(): session_id="s1", ) data = json.loads(result) - assert data["acknowledged"] is True - # No agent instance → the spill filename uses "unknown" as the subagent id. - # This proves the registry path cannot record on the agent instance. - assert data.get("path") is None or "unknown" in data["path"] + assert data["acknowledged"] is False -def test_agent_level_intercept_passes_agent_to_handler(monkeypatch): +def test_agent_level_intercept_passes_agent_to_handler(): """When the tool_executor intercepts ``delegate_tool_reply`` as an agent-level tool, it passes the agent instance directly to the handler — NOT through registry.dispatch. This is the fix for the anchorless delivery bug where registry.dispatch lost the agent reference.""" - with tempfile.TemporaryDirectory() as td: - monkeypatch.setenv("HERMES_HOME", os.path.join(td, ".hermes")) - agent = _make_agent("child-intercept-test") - # Simulate the inline interception in tool_executor.py / agent_runtime_helpers.py - result = dtr.delegate_tool_reply( - content="delivered via agent-level intercept", - parent_agent=agent, + agent = _make_agent("child-intercept-test") + result = dtr.delegate_tool_reply( + content="delivered via agent-level intercept", + parent_agent=agent, + ) + data = json.loads(result) + assert data["acknowledged"] is True + assert agent._delegate_reply_chunks == ["delivered via agent-level intercept"] + + +def test_delegate_reply_is_never_parallelized(): + from agent.tool_dispatch_helpers import _NEVER_PARALLEL_TOOLS + + assert "delegate_tool_reply" in _NEVER_PARALLEL_TOOLS + + +@pytest.fixture +def runtime_agent(): + """Real AIAgent dispatch surface with network/tool discovery mocked.""" + from run_agent import AIAgent + + tool_defs = [ + { + "type": "function", + "function": dtr.DELEGATE_TOOL_REPLY_SCHEMA, + } + ] + with ( + patch("model_tools.get_tool_definitions", return_value=tool_defs), + patch("model_tools.check_toolset_requirements", return_value={}), + patch("agent.process_bootstrap.OpenAI"), + ): + agent = AIAgent( + api_key="test-key-1234567890", + base_url="https://openrouter.ai/api/v1", + quiet_mode=True, + skip_context_files=True, + skip_memory=True, ) - data = json.loads(result) - assert data["acknowledged"] is True - # Agent-instance state IS recorded when the agent is passed directly. - assert agent._delegate_reply_chunks == ["delivered via agent-level intercept"] - # Spill file uses the real subagent id, not "unknown". - if data.get("path"): - assert "child-intercept-test" in data["path"] + agent.client = MagicMock() + agent._delegate_reply_chunks = [] + yield agent + agent.close() + + +def test_runtime_invoke_path_records_delivery(runtime_agent): + result = runtime_agent._invoke_tool( + "delegate_tool_reply", + {"content": "concurrent-path result"}, + "task-1", + ) + + assert json.loads(result)["acknowledged"] is True + assert runtime_agent._delegate_reply_chunks == ["concurrent-path result"] + + +def test_sequential_executor_path_records_delivery(runtime_agent): + tool_call = SimpleNamespace( + id="call-reply", + type="function", + function=SimpleNamespace( + name="delegate_tool_reply", + arguments=json.dumps({"content": "sequential-path result"}), + ), + ) + assistant_message = SimpleNamespace(content="", tool_calls=[tool_call]) + messages = [] + + runtime_agent._execute_tool_calls_sequential( + assistant_message, + messages, + "task-1", + ) + + assert runtime_agent._delegate_reply_chunks == ["sequential-path result"] + assert len(messages) == 1 + assert json.loads(messages[0]["content"])["acknowledged"] is True diff --git a/tools/delegate_tool.py b/tools/delegate_tool.py index 272ae0971881..00f12d30c3bb 100644 --- a/tools/delegate_tool.py +++ b/tools/delegate_tool.py @@ -263,6 +263,8 @@ def _build_child_agent( child._progress_identity_ref = child_session_ref child._delegate_depth, child._delegate_role = child_depth, effective_role # post-degrade role child._subagent_id, child._parent_subagent_id = subagent_id, parent_subagent_id + # Delivery survives transcript compaction; schema correction starts a new attempt. + child._delegate_reply_chunks = [] _apply_child_compression_cap(child, delegation_cfg) # Ownership chain for action=list/steer/stop; weakref so a finished parent # can be collected while a detached child record lingers in the registry. diff --git a/tools/delegate_tool_child_run.py b/tools/delegate_tool_child_run.py index 0b19ac7ef1bb..72d1c98d03c1 100644 --- a/tools/delegate_tool_child_run.py +++ b/tools/delegate_tool_child_run.py @@ -411,6 +411,10 @@ def _validate_child_output_schema( ) -> _SchemaOutcome: """Validate the final answer against the attached output_schema with ONE bounded retry. Schema-less children (no dict on ``child._delegate_output_schema``) take no branch here so their result entry stays byte-identical.""" + # Select once before validation and result assembly, even without a schema. + delivery = _extract_reply_deliverable(child) + if delivery is not None: + result["final_response"] = delivery _output_schema = getattr(child, "_delegate_output_schema", None) if not isinstance(_output_schema, dict): return _SchemaOutcome(_output_schema, None, [], 0) @@ -423,6 +427,8 @@ def _validate_child_output_schema( # Exactly one retry turn, carrying the validation errors verbatim (no # schema re-paste — the child already holds the contract in its context). _retry_result = None + # A retry replaces the rejected attempt, never concatenates it with the correction. + child._delegate_reply_chunks = [] try: _retry_result = child.run_conversation( user_message=build_retry_message(_schema_errors), task_id=child_task_id, stream_callback=relay_child_text, @@ -430,7 +436,8 @@ def _validate_child_output_schema( except Exception as _retry_exc: logger.warning("Subagent %d schema-retry turn failed: %s", task_index, _retry_exc) if isinstance(_retry_result, dict): - _retry_text = _retry_result.get("final_response") or "" + delivery = _extract_reply_deliverable(child) + _retry_text = delivery if delivery is not None else _retry_result.get("final_response") or "" if _retry_text.strip(): result["final_response"] = _retry_text try: @@ -480,7 +487,7 @@ def _extract_reply_deliverable(child) -> Optional[str]: chunks = getattr(child, "_delegate_reply_chunks", None) if not isinstance(chunks, list): return None - return "\n\n".join(chunks) + return "\n\n".join(chunks) if chunks else None def _build_result_entry( @@ -489,8 +496,7 @@ def _build_result_entry( """Parent-visible result entry (status, exit_reason, tool trace, tokens, cost). ``status``/``exit_reason``/``truncated`` follow the ``_run_single_child`` contract; a structured failure always wins over the summary-presence heuristic (a fallback for legacy/mock results only).""" - delivery = _extract_reply_deliverable(child) - summary = delivery if delivery is not None else result.get("final_response") or "" + summary = result.get("final_response") or "" # "(empty)" is run_agent's give-up sentinel after repeated empty LLM # responses (usually a transport bug) — a failure, not a success. usable_summary = bool(summary) and summary.strip() != "(empty)" diff --git a/tools/delegate_tool_reply.py b/tools/delegate_tool_reply.py index 6b78aa612ff7..5701bf522e24 100644 --- a/tools/delegate_tool_reply.py +++ b/tools/delegate_tool_reply.py @@ -1,6 +1,6 @@ """``delegate_tool_reply`` — explicit delivery channel for subagent results. -A leaf-subagent-only tool that hands the deliverable back to the parent agent +A subagent-only tool that hands the deliverable back to the parent agent through a structured tool call instead of relying on the trailing ``final_response`` prose. This closes the information-loss bug where a subagent's real result (emitted on a turn that also called a housekeeping / @@ -27,26 +27,14 @@ to a child's toolset, so the tool is only ever visible to subagents spawned by ``delegate_task``. Ordinary conversations never see the schema. -Execution model: - -The handler runs in the agent's Python process (like ``todo`` / ``memory``), -not in the terminal sandbox — so the spill file lands on the agent host and is -reachable by the parent's ``read_file`` regardless of whether the child's -terminal points at a remote Docker / SSH / Modal backend. - Compression resilience: -The deliverable is recorded in **two** places at call time: - -1. **Agent-instance state** (``child._delegate_reply_chunks``) — a list the - handler appends to at execution time. ``_run_single_child`` reads this list - after the child finishes; it is never touched by context compression, - which only mutates the ``messages`` transcript. This is the authoritative - source. -2. **Spill file** (``cache/delegation/delegate_reply_*.txt``) — a backup on - disk, mirroring ``_spill_summary_to_file``. Useful if the agent instance - is somehow lost (e.g. timeout where the future result is unavailable but - the child object still exists). +The deliverable is recorded in ``child._delegate_reply_chunks`` at execution +time. ``_run_single_child`` reads this list after the child finishes; it is +never touched by context compression, which only mutates the ``messages`` +transcript. Oversized final summaries still use the existing bounded summary +spill path after extraction, so every call does not duplicate potentially +sensitive output on disk. Because the agent-instance state lives outside the ``messages`` list, context compression — which replaces the middle transcript with a summary @@ -58,13 +46,8 @@ from __future__ import annotations import json -import logging -from typing import Optional - from tools.registry import registry -logger = logging.getLogger(__name__) - _TOOL_NAME = "delegate_tool_reply" DELEGATE_TOOL_REPLY_SCHEMA = { @@ -90,41 +73,14 @@ } -def _spill_reply_to_file(content: str, subagent_id: Optional[str]) -> Optional[str]: - """Write the deliverable to the delegation cache and return the abs path. - - Mirrors ``_spill_summary_to_file`` in ``delegate_tool.py``: the file lands - in ``cache/delegation`` which is mounted read-only into remote backends - (Docker/Modal/SSH) via ``credential_files._CACHE_DIRS``, so the parent's - ``terminal`` / ``read_file`` tools can page through the complete text on any - backend. Returns the absolute path, or ``None`` on failure (best-effort). - """ - try: - from hermes_constants import get_hermes_dir - import datetime as _dt - - cache_dir = get_hermes_dir("cache/delegation", "delegation_cache") - cache_dir.mkdir(parents=True, exist_ok=True) - ts = _dt.datetime.now().strftime("%Y%m%d_%H%M%S_%f") - sid = subagent_id or "unknown" - # Sanitize the subagent id for use in a filename. - safe_sid = "".join(c if c.isalnum() or c in "-_" else "_" for c in sid)[:64] - path = cache_dir / f"delegate_reply-{safe_sid}-{ts}.txt" - path.write_text(content, encoding="utf-8") - return str(path) - except Exception as exc: - logger.debug("Failed to spill delegate_tool_reply content to file: %s", exc) - return None - - def delegate_tool_reply(content: str, parent_agent=None, **kw) -> str: - """Record a subagent deliverable and spill it to disk. + """Record a subagent deliverable on the executing child agent. Appends ``content`` to ``parent_agent._delegate_reply_chunks`` (the - authoritative store) and writes a spill-file backup. Does **not** terminate - the subagent loop — the child keeps running (e.g. cleanup) to natural end. - ``_run_single_child`` in ``delegate_tool.py`` reads - ``child._delegate_reply_chunks`` after the child completes. + authoritative store). Does **not** terminate the subagent loop — the child + keeps running (e.g. cleanup) to natural end. ``_run_single_child`` in + ``delegate_tool.py`` reads ``child._delegate_reply_chunks`` after the child + completes. Args: content: the full deliverable text (one chunk). @@ -132,26 +88,28 @@ def delegate_tool_reply(content: str, parent_agent=None, **kw) -> str: via ``kw["parent_agent"]``). Returns: - JSON string ``{"acknowledged": true, "path": }``. + JSON acknowledgement. Missing agent context fails closed because an + acknowledgement without recording would silently lose the result. """ if not isinstance(content, str): content = str(content) if content is not None else "" - # Record in agent-instance state — compression-safe (not in messages[]). - if parent_agent is not None: - chunks = getattr(parent_agent, "_delegate_reply_chunks", None) - if chunks is None: - chunks = [] - setattr(parent_agent, "_delegate_reply_chunks", chunks) - chunks.append(content) - - # Spill to disk as a backup (best-effort). - subagent_id = getattr(parent_agent, "_subagent_id", None) if parent_agent is not None else None - spill_path = _spill_reply_to_file(content, subagent_id) - return json.dumps( - {"acknowledged": True, "path": spill_path}, - ensure_ascii=False, - ) + if parent_agent is None: + return json.dumps( + { + "acknowledged": False, + "error": "delegate_tool_reply requires the executing subagent context", + } + ) + + # Agent-instance state is compression-safe because it is not in messages[]. + chunks = getattr(parent_agent, "_delegate_reply_chunks", None) + if chunks is None: + chunks = [] + setattr(parent_agent, "_delegate_reply_chunks", chunks) + chunks.append(content) + + return json.dumps({"acknowledged": True}) def _handle_delegate_tool_reply(args, **kw): diff --git a/tools/delegate_tool_toolsets.py b/tools/delegate_tool_toolsets.py index cd0afedda160..ce444384b5c4 100644 --- a/tools/delegate_tool_toolsets.py +++ b/tools/delegate_tool_toolsets.py @@ -104,6 +104,8 @@ def _resolve_child_toolsets( inherited_disabled = ( [str(name) for name in raw_parent_disabled] if isinstance(raw_parent_disabled, (list, tuple, set)) else [] ) + # Child-owned delivery is granted by construction, not inherited configuration. + inherited_disabled = [name for name in inherited_disabled if name != "delegation_reply"] if effective_role == "orchestrator": inherited_disabled = [name for name in inherited_disabled if name != "delegation"] if "delegation" not in child_toolsets: From 355cf04e2089c8452e0a16707113c1fdd3093494 Mon Sep 17 00:00:00 2001 From: "hono_hermes(Agent)" Date: Wed, 9 Sep 2026 15:11:21 +0800 Subject: [PATCH 5/5] fix(delegation): harden reply scope and verify runtime delivery Keep the internal reply tool out of implicit all-tool selections while preserving explicit child grants. Extend the hook ownership matrix and constructor/schema visibility coverage for depth-derived roles. Replace synthetic compaction checks with real compressor and turn-start reset paths, covering both compaction modes, ordered chunks, cleanup fallback, middleware, sync/async delivery, and schema retry isolation. Validation: scripts/run_tests.sh targeted 19-file matrix, -j 2 --file-retries 0: 629 passed, 2 OS skips. Explicitly deselected 3 tests blocked by missing shared-venv snowballstemmer/anthropic dependencies; no dependencies changed. Reproduced cleanup overwrite on the target path before porting. --- model_tools.py | 3 + tests/agent/test_run_agent.py | 1 + tests/tools/test_delegate_output_schema.py | 35 +++ tests/tools/test_delegate_reply_runtime.py | 238 +++++++++++++++++++++ tests/tools/test_delegate_tool_reply.py | 68 +++--- tools/delegate_tool_child_run.py | 4 +- tools/delegate_tool_reply.py | 4 +- 7 files changed, 311 insertions(+), 42 deletions(-) create mode 100644 tests/tools/test_delegate_reply_runtime.py diff --git a/model_tools.py b/model_tools.py index 4193fb22106e..1ce8aeb39427 100644 --- a/model_tools.py +++ b/model_tools.py @@ -324,6 +324,9 @@ def _select_tool_names(enabled_toolsets: Optional[List[str]], disabled_toolsets: else: from toolsets import get_all_toolsets for ts_name in get_all_toolsets(): + # Internal delivery is explicitly granted by child construction, never by "all tools". + if ts_name == "delegation_reply": + continue tools.update(resolve_toolset(ts_name)) # Disabled toolsets are always subtracted LAST, so a tool in a disabled # toolset is stripped even when a composite (hermes-cli) re-enables it. diff --git a/tests/agent/test_run_agent.py b/tests/agent/test_run_agent.py index df512c9c20a1..cefe2924c12b 100644 --- a/tests/agent/test_run_agent.py +++ b/tests/agent/test_run_agent.py @@ -2492,6 +2492,7 @@ class TestAgentRuntimePostHookOwnershipSync: ("setup_mcp", {"server": "linear", "action": "install"}), ("gui_tour", {"action": "stop"}), ("delegate_task", {"goal": "Check the child path"}), + ("delegate_tool_reply", {"content": "Explicit child result"}), ) @pytest.mark.parametrize(("tool_name", "tool_args"), _CASES) diff --git a/tests/tools/test_delegate_output_schema.py b/tests/tools/test_delegate_output_schema.py index 31ae0ba48785..cc00d30b9b60 100644 --- a/tests/tools/test_delegate_output_schema.py +++ b/tests/tools/test_delegate_output_schema.py @@ -16,6 +16,8 @@ import threading from unittest.mock import MagicMock, patch +import pytest + from tools.delegate_tool import ( DELEGATE_TASK_SCHEMA, _run_single_child, @@ -204,6 +206,39 @@ def _run(child): class TestRunSingleChildSchemaValidation: + def test_retry_without_explicit_call_uses_only_corrected_final_response(self): + child = _StubChild([ + ("cleanup", ["not json"]), + ('{"city": "Oslo"}', []), + ]) + child._delegate_output_schema = ADDRESS_SCHEMA + entry = _run(child) + assert entry["schema_valid"] is True + assert entry["summary"] == '{"city": "Oslo"}' + assert child._delegate_reply_chunks == [] + + def test_multiple_delivery_chunks_are_validated_as_one_document(self): + child = _StubChild([("cleanup", ['{"city":', '"Rome"}'])]) + child._delegate_output_schema = ADDRESS_SCHEMA + entry = _run(child) + assert entry["schema_valid"] is True + assert json.loads(entry["summary"]) == {"city": "Rome"} + assert len(child.calls) == 1 + + @pytest.mark.parametrize("correction", ["", " "]) + def test_empty_explicit_retry_is_not_replaced_by_cleanup(self, correction): + child = _StubChild([ + ("cleanup", ["rejected delivery"]), + ('{"city": "not the deliverable"}', [correction]), + ]) + child._delegate_output_schema = ADDRESS_SCHEMA + entry = _run(child) + assert entry["schema_valid"] is False + assert entry["status"] == "failed" + assert entry["schema_retries"] == 1 + assert child._delegate_reply_chunks == [correction] + assert "not the deliverable" not in entry["summary"] + def test_explicit_delivery_is_validated_before_trailing_prose(self): child = _StubChild( [("cleanup complete", ['{"city": "Rome"}'])] diff --git a/tests/tools/test_delegate_reply_runtime.py b/tests/tools/test_delegate_reply_runtime.py new file mode 100644 index 000000000000..a99e67297be8 --- /dev/null +++ b/tests/tools/test_delegate_reply_runtime.py @@ -0,0 +1,238 @@ +"""Delivery contracts through the real child loop and completion pipeline.""" + +import json +import queue +from types import SimpleNamespace +from unittest.mock import MagicMock, patch + +import pytest + +from run_agent import AIAgent +from tools.delegate_tool import _run_single_child +from tools.delegate_tool_reply import DELEGATE_TOOL_REPLY_SCHEMA + + +def _response(content, tool_calls=None): + return SimpleNamespace( + choices=[SimpleNamespace( + message=SimpleNamespace(content=content, tool_calls=tool_calls), + finish_reason="tool_calls" if tool_calls else "stop", + )], model="test/model", usage=None, + ) + + +def _call(name, arguments="{}", call_id="cleanup"): + return SimpleNamespace( + id=call_id, type="function", + function=SimpleNamespace(name=name, arguments=arguments), + ) + + +@pytest.fixture +def child(): + definitions = [{"type": "function", "function": { + "name": "terminal", "description": "test cleanup", + "parameters": {"type": "object", "properties": {}}, + }}, {"type": "function", "function": DELEGATE_TOOL_REPLY_SCHEMA}] + with ( + patch("model_tools.get_tool_definitions", return_value=definitions), + patch("model_tools.check_toolset_requirements", return_value={}), + patch("agent.process_bootstrap.OpenAI"), + ): + agent = AIAgent( + api_key="test-key", base_url="https://openrouter.ai/api/v1/", + quiet_mode=True, skip_context_files=True, skip_memory=True, + ) + agent._cached_system_prompt = "Complete the delegated audit." + agent._use_prompt_caching = False + agent.compression_enabled = False + agent.save_trajectories = False + agent.client = MagicMock() + agent._delegate_depth = 1 + agent._delegate_role = "leaf" + agent._delegate_reply_chunks = [] + with patch("model_tools.handle_function_call", return_value="cleanup succeeded"): + yield agent + agent.close() + + +@pytest.mark.parametrize("explicit", [False, True]) +def test_cleanup_does_not_replace_explicit_deliverable(child, explicit): + calls = [] + if explicit: + calls.append(_call("delegate_tool_reply", json.dumps({"content": "FULL AUDIT REPORT"}), "delivery")) + calls.append(_call("terminal")) + child.client.chat.completions.create.side_effect = [ + _response("FULL AUDIT REPORT", calls), + _response("Cleanup complete."), + ] + entry = _run_single_child(0, "Audit the changes", child, None) + # Without an explicit call the existing final-response fallback is unchanged. + assert entry["summary"] == ("FULL AUDIT REPORT" if explicit else "Cleanup complete.") + assert entry["status"] == "completed" + assert entry["api_calls"] == 2 + + +@pytest.mark.parametrize("in_place", [True, False]) +def test_delivery_survives_real_compaction_and_retry_reset(child, in_place): + from agent.context_compressor import SUMMARY_PREFIX, is_compaction_summary_message + from agent.turn_context_compaction import run_turn_start_compaction + from tools.delegate_tool_child_run import _extract_reply_deliverable + + child.compression_enabled = True + child.compression_in_place = in_place + compressor = child.context_compressor + compressor.protect_first_n = 1 + compressor.protect_last_n = 3 + compressor.tail_token_budget = 500 + messages = [{"role": "user", "content": "Audit the changes"}] + call = _call("delegate_tool_reply", json.dumps({"content": "FULL AUDIT REPORT"}), "delivery") + messages.append({"role": "assistant", "content": "", "tool_calls": [{ + "id": call.id, "type": "function", + "function": {"name": call.function.name, "arguments": call.function.arguments}, + }]}) + child._execute_tool_calls_sequential(SimpleNamespace(content="", tool_calls=[call]), messages, "child-task") + for i in range(30): + messages.extend([ + {"role": "user", "content": f"Follow-up {i}: " + "context " * 100}, + {"role": "assistant", "content": f"Investigation {i}: " + "findings " * 100}, + ]) + messages.append({"role": "user", "content": "Finish cleanup"}) + child._last_content_with_tools = "stale fallback" + child._last_content_tools_all_housekeeping = True + child._request_pressure_anchored = True + with ( + patch("agent.turn_context._preflight_request_tokens", side_effect=[200_000, 1_000]), + patch.object(compressor, "_generate_summary", return_value=SUMMARY_PREFIX + "Earlier investigation summarized.") as summarize, + ): + out = run_turn_start_compaction( + child, messages=messages, system_message="Audit instructions", active_system_prompt="Audit instructions", + conversation_history=None, current_turn_user_idx=len(messages) - 1, + user_message="Finish cleanup", effective_task_id="child-task", + ) + summarize.assert_called_once() + assert out.compressed and out.messages is not messages + assert any(is_compaction_summary_message(msg) for msg in out.messages) + assert not any(tc["function"]["name"] == "delegate_tool_reply" + for msg in out.messages for tc in msg.get("tool_calls", [])) + assert "FULL AUDIT REPORT" not in json.dumps(out.messages) + assert child._last_content_with_tools is None + assert child._last_content_tools_all_housekeeping is False + child._invoke_tool("delegate_tool_reply", {"content": "SECOND CHUNK"}, "child-task") + assert _extract_reply_deliverable(child) == "FULL AUDIT REPORT\n\nSECOND CHUNK" + child.compression_enabled = False + child.client.chat.completions.create.return_value = _response("Cleanup complete.") + entry = _run_single_child(0, "Finish cleanup", child, None) + assert entry["summary"] == "FULL AUDIT REPORT\n\nSECOND CHUNK" + + +@pytest.mark.parametrize("background", [False, True]) +def test_delivery_reaches_sync_and_async_transport(child, background, monkeypatch): + from tools import async_delegation as ad + import tools.delegate_tool as dt + from tools.process_registry import process_registry + from tools.process_registry_notifications import format_process_notification + + completion_queue = queue.Queue() + monkeypatch.setattr(process_registry, "completion_queue", completion_queue) + ad._reset_for_tests() + child.client.chat.completions.create.side_effect = [ + _response("", [_call("delegate_tool_reply", json.dumps({"content": "PART A"}), "a")]), + _response("", [_call("delegate_tool_reply", json.dumps({"content": "PART B"}), "b"), _call("terminal")]), + _response("Cleanup complete."), + ] + parent = SimpleNamespace( + _delegate_depth=0, session_id="parent-delivery", _active_children=[], + _active_children_lock=None, _interrupt_requested=False, + ) + monkeypatch.setattr(dt, "_build_child_agent", lambda **kw: child) + monkeypatch.setattr(dt, "_resolve_delegation_credentials", lambda *a, **kw: { + "model": "test/model", "provider": None, "base_url": None, "api_key": None, "api_mode": None, + }) + monkeypatch.setattr("gateway.session_context.async_delivery_supported", lambda: True) + try: + result = json.loads(dt.delegate_task(goal="Audit the changes", parent_agent=parent, background=background)) + if background: + assert result["status"] == "dispatched" + event = completion_queue.get(timeout=10) + assert event["delegation_id"] == result["delegation_id"] + assert event["parent_session_id"] == parent.session_id + result = event + formatted = format_process_notification(event) + assert "PART A\n\nPART B" in formatted + assert "Cleanup complete." not in formatted + assert result["results"][0]["summary"] == "PART A\n\nPART B" + assert result["results"][0]["status"] == "completed" + finally: + ad._reset_for_tests() + + +@pytest.mark.parametrize("dispatch", ["concurrent", "sequential"]) +@pytest.mark.parametrize("blocked", [False, True]) +def test_delivery_obeys_middleware_and_emits_one_post_hook(child, dispatch, blocked, monkeypatch): + from hermes_cli.plugins import get_plugin_manager + + seen = [] + + def rewrite(tool_name, args, **kw): + assert tool_name == "delegate_tool_reply" + seen.append("request") + return {"args": {"content": "rewritten"}} + + def execute(tool_name, args, next_call, **kw): + seen.append("execution") + assert args == {"content": "rewritten"} + if blocked: + return json.dumps({"error": "delivery blocked"}) + return next_call({"content": args["content"] + " by middleware"}) + + monkeypatch.setattr(get_plugin_manager(), "_middleware", { + "tool_request": [rewrite], "tool_execution": [execute], + }) + with ( + patch("hermes_cli.lifecycle.invoke_hook", return_value=[]) as hooks, + patch("hermes_cli.lifecycle.has_hook", return_value=True), + ): + messages = [] + executor = child._execute_tool_calls_concurrent if dispatch == "concurrent" else child._execute_tool_calls_sequential + executor( + SimpleNamespace(content="", tool_calls=[_call("delegate_tool_reply", '{"content":"original"}', "d1")]), + messages, "child-task", + ) + result = messages[0]["content"] + assert seen == ["request", "execution"] + assert child._delegate_reply_chunks == ([] if blocked else ["rewritten by middleware"]) + if blocked: + assert "delivery blocked" in result + else: + assert json.loads(result)["acknowledged"] is True + post_calls = [call for call in hooks.call_args_list if call.args[0] == "post_tool_call"] + assert len(post_calls) == 1 + assert post_calls[0].kwargs["tool_call_id"] == "d1" + + +def test_segmented_batch_preserves_delivery_and_cleanup_order(child, tmp_path): + from agent.tool_dispatch_helpers import _plan_tool_batch_segments + + child.valid_tool_names.add("read_file") + calls = [ + _call("read_file", json.dumps({"path": str(tmp_path / "a")}), "read-a"), + _call("read_file", json.dumps({"path": str(tmp_path / "b")}), "read-b"), + _call("delegate_tool_reply", '{"content":"PART A"}', "delivery-a"), + _call("terminal", "{}", "cleanup"), + _call("delegate_tool_reply", '{"content":"PART B"}', "delivery-b"), + ] + assert [kind for kind, _ in _plan_tool_batch_segments(calls)] == ["parallel", "sequential"] + at_cleanup = [] + + def tool(name, *args, **kw): + if name == "terminal": + at_cleanup.append(list(child._delegate_reply_chunks)) + return "ok" + + messages = [] + with patch("model_tools.handle_function_call", side_effect=tool): + child._execute_tool_calls(SimpleNamespace(content="", tool_calls=calls), messages, "child-task", 0) + assert at_cleanup == [["PART A"]] + assert child._delegate_reply_chunks == ["PART A", "PART B"] + assert [msg["tool_call_id"] for msg in messages] == [call.id for call in calls] diff --git a/tests/tools/test_delegate_tool_reply.py b/tests/tools/test_delegate_tool_reply.py index 8ddcc611d748..e55540deb466 100644 --- a/tests/tools/test_delegate_tool_reply.py +++ b/tests/tools/test_delegate_tool_reply.py @@ -2,12 +2,13 @@ Covers: - handler: records to agent-instance state and fails closed without it -- extraction: no-call fallback, single call, multi-call append, compression - safety (agent-instance state survives when messages[] is replaced) +- extraction: no-call fallback, single call, multi-call append - visibility: delegation_reply not in CONFIGURABLE_TOOLSETS, not in default tool definitions, but present in child toolsets built by _build_child_agent (validated via constructor-capture pattern from test_delegate.py) - system prompt discipline injection + +Real compaction, cleanup and transport regressions live in test_delegate_reply_runtime.py. """ import json @@ -97,40 +98,6 @@ def test_extraction_non_list_returns_none(): assert _extract_reply_deliverable(child) is None -# --------------------------------------------------------------------------- -# Compression safety regression (the core teknium1 review point) -# --------------------------------------------------------------------------- - -def test_extraction_survives_context_compression(): - """The deliverable is read from agent-instance state, not messages[]. - - Context compression replaces the middle of messages[] with a summary - (context_compressor.py Phase 4). This test proves that even if messages[] - is completely replaced by a synthetic summary, the deliverable recorded on - the agent instance is intact — because the handler wrote it at execution - time, outside the transcript. - """ - child = MagicMock() - child._delegate_reply_chunks = ["FULL AUDIT REPORT"] - # Simulate compression: messages[] is now a synthetic summary, no - # delegate_tool_reply tool calls remain in it. - compressed_messages = [ - {"role": "user", "content": "do the audit"}, - {"role": "assistant", "content": "[Summary of earlier turns: subagent ran audit and delivered results.]"}, - {"role": "assistant", "content": "done"}, - ] - # Extraction does NOT read messages — it reads the agent instance. - assert _extract_reply_deliverable(child) == "FULL AUDIT REPORT" - # Even if someone passed messages, it wouldn't matter — the function - # signature takes `child`, not `messages`. - - -def test_extraction_multi_chunk_survives_compression(): - child = MagicMock() - child._delegate_reply_chunks = ["chunk-A", "chunk-B", "chunk-C"] - assert _extract_reply_deliverable(child) == "chunk-A\n\nchunk-B\n\nchunk-C" - - # --------------------------------------------------------------------------- # Visibility / toolset membership (constructor-capture pattern) # --------------------------------------------------------------------------- @@ -158,12 +125,18 @@ def _make_mock_parent(): return parent -def test_build_child_agent_includes_delegation_reply(): +@pytest.mark.parametrize("parent_depth, expected_role", [(0, "orchestrator"), (1, "leaf")]) +def test_build_child_agent_includes_delegation_reply(parent_depth, expected_role): """Exercise the real _build_child_agent, not a hand-written append.""" parent = _make_mock_parent() parent.enabled_toolsets = ["terminal", "file"] + parent._delegate_depth = parent_depth - with patch("tools.delegate_tool._load_config", return_value={}): + with ( + patch("tools.delegate_tool._load_config", return_value={}), + patch("tools.delegate_tool._get_max_spawn_depth", return_value=2), + patch("tools.delegate_tool._get_orchestrator_enabled", return_value=True), + ): with patch("run_agent.AIAgent") as MockAgent: mock_child = MagicMock() MockAgent.return_value = mock_child @@ -181,6 +154,16 @@ def test_build_child_agent_includes_delegation_reply(): enabled = MockAgent.call_args[1]["enabled_toolsets"] assert "delegation_reply" in enabled + assert mock_child._delegate_role == expected_role + assert mock_child._delegate_reply_chunks == [] + assert ("delegation" in enabled) == (expected_role == "orchestrator") + assert parent.enabled_toolsets == ["terminal", "file"] + from model_tools import get_tool_definitions + definitions = get_tool_definitions( + enabled_toolsets=enabled, disabled_toolsets=MockAgent.call_args[1]["disabled_toolsets"], + quiet_mode=True, skip_tool_search_assembly=True, + ) + assert "delegate_tool_reply" in {t["function"]["name"] for t in definitions} def test_parent_disabled_toolsets_cannot_remove_delivery_channel(): @@ -219,6 +202,15 @@ def test_delegation_reply_not_in_core_tools(): assert "delegate_tool_reply" not in _HERMES_CORE_TOOLS +@pytest.mark.parametrize("toolset", [None, "hermes-cli", "hermes-telegram"]) +def test_parent_default_schemas_do_not_include_delivery(toolset): + from model_tools import get_tool_definitions + definitions = get_tool_definitions( + enabled_toolsets=[toolset] if toolset else None, quiet_mode=True, skip_tool_search_assembly=True, + ) + assert "delegate_tool_reply" not in {t["function"]["name"] for t in definitions} + + def test_delegation_reply_toolset_resolves(): from toolsets import get_toolset ts = get_toolset("delegation_reply") diff --git a/tools/delegate_tool_child_run.py b/tools/delegate_tool_child_run.py index 72d1c98d03c1..95b8a2f1fc55 100644 --- a/tools/delegate_tool_child_run.py +++ b/tools/delegate_tool_child_run.py @@ -409,8 +409,8 @@ class _SchemaOutcome: def _validate_child_output_schema( child: Any, result: Dict[str, Any], task_index: int, child_task_id: str, relay_child_text: Any ) -> _SchemaOutcome: - """Validate the final answer against the attached output_schema with ONE bounded retry. Schema-less children (no - dict on ``child._delegate_output_schema``) take no branch here so their result entry stays byte-identical.""" + """Select the authoritative delivery, then validate an optional schema with ONE bounded retry. + Schema-less children retain the existing result shape without schema outcome fields.""" # Select once before validation and result assembly, even without a schema. delivery = _extract_reply_deliverable(child) if delivery is not None: diff --git a/tools/delegate_tool_reply.py b/tools/delegate_tool_reply.py index 5701bf522e24..f15b951ceef2 100644 --- a/tools/delegate_tool_reply.py +++ b/tools/delegate_tool_reply.py @@ -84,8 +84,8 @@ def delegate_tool_reply(content: str, parent_agent=None, **kw) -> str: Args: content: the full deliverable text (one chunk). - parent_agent: the child AIAgent instance (threaded in by the registry - via ``kw["parent_agent"]``). + parent_agent: the executing child AIAgent, passed by the shared inline + executor (ordinary registry dispatch has no agent reference). Returns: JSON acknowledgement. Missing agent context fails closed because an