diff --git a/BUILDLOG.md b/BUILDLOG.md new file mode 100644 index 00000000000..a3ad0dfcfc6 --- /dev/null +++ b/BUILDLOG.md @@ -0,0 +1,33 @@ +# BUILDLOG + +Source of truth for what exists in this fork (`albatrossflyon-coder/nanobot`, tracking upstream `HKUDS/nanobot`) beyond upstream's own docs. + +## Tech Stack + +- **Languages:** Python, TypeScript +- **Frameworks/Libraries:** FastAPI-style channel manager, React (webui), LangGraph-adjacent agent loop +- **Dev Tools:** pytest, ruff, basedpyright + +--- + +## 2026-08-11 — Tool-call-markup-leak fix: 4 original gaps closed, 3 new gaps found by code-review + +**Context:** A model finalizing with no tools offered could still emit literal `` text instead of a real answer, and that raw markup could reach a real user-facing channel. Confirmed live twice today via real email to Chris (`~/.nanobot/logs/gateway.log`, 14:02 and 14:27 CDT — `Response to email:...: ` at INFO level, meaning it was never blocked; the filter that should have caught it was built but never actually committed/active in the running gateway). + +**4 gaps from the prior session's `/code-review` pass — all fixed and tested this session:** +1. Dominant per-turn finalize path (`runner.py` `_run_core`, where most turns actually end) had no leak guard — only the max-iterations retry path did. Fixed: added `contains_leaked_tool_call_markup(clean)` check alongside the existing blank-content check, same pattern (fallback message, `stop_reason="leaked_tool_call_markup"`, drain injections, break). +2. Only the email channel had the egress filter; 15 other channels didn't. Fixed by centralizing instead of propagating: `ChannelManager._send_once` (the single funnel all non-streaming channel sends pass through, confirmed via `find_references`/`search_text`) now runs the check once for all 17 channels. Removed the now-redundant duplicate check from `EmailChannel.send()`. +3. Regex `` wrapper tag. Verified live that **both** `` and `` failed to match (the prior session only caught the opening-tag case) — fixed to ``. +4. The blocked-leak warning log wrote the raw leaked content (potentially shell commands/session IDs) unredacted. Fixed at the source: the new centralized check in `_send_once` never logs the raw content at all (length only). + +**Verification:** Full test suite 5914 passed / 44 skipped / 0 failed. `vuln-hunter scan_diff` clean except one unrelated pre-existing item (see below). New/moved tests: `tests/agent/test_runner_safety.py` (2 new — dominant-path leak rejection + clean-response negative case), `tests/channels/test_channel_manager_leak_filter.py` (3 new — centralized filter, plural-tag regression, normal-content passthrough), `nanobot/channels/email/tests/test_email_channel.py` (obsolete email-specific leak test removed, now covered at the manager level instead). + +**3 new findings from a fresh `/code-review high` pass after the fix — triaged with Chris, not silently shipped-and-disclosed:** + +1. **Streaming bypass (pre-existing, NOT introduced tonight, NOT fixed tonight).** The filter only runs in `_send_once`'s non-streaming branch. A leaked `` in a streaming response (webui/websocket) would already display live, token-by-token, before any finalize-time check runs — this gap existed before tonight's fix too (streaming had zero leak protection either way) and closing it properly needs mid-stream detection or buffering, a real design change, not a quick patch. **Status: open, tracked as a follow-up, not fixed.** +2. **MessageTool-suppression behavior for the new `leaked_tool_call_markup` stop_reason was a genuine design question, not a clear bug** (`loop.py` `_assemble_outbound`, line ~1594). `empty_final_response` always suppresses the fallback notice when `MessageTool` already sent real content this turn. `leaked_tool_call_markup` was following the general rule instead (suppress only if no new injections occurred) — a code-review report initially described this backwards (claimed the leak notice gets silently dropped when the empty one doesn't; direct code tracing showed the opposite: in the `had_injections=True` case, the leak notice was the one that got delivered, `empty_final_response` was the one still suppressed). Chris's call: always suppress, matching `empty_final_response` — real content already went out via MessageTool, so a leak on the wrap-up has nothing useful to add. **Status: fixed 2026-08-11** — `stop_reason in ("empty_final_response", "leaked_tool_call_markup")` now both suppress unconditionally. New test: `tests/tools/test_message_tool_suppress.py::test_injected_followup_with_message_tool_suppresses_leaked_markup_notice`. +3. **Regex ``/`` singular/plural portion. Checked `~/.nanobot/logs/gateway.log` for evidence this ever fired as a false positive in production — found none; the filter was never actually active before tonight (see Context above), so this risk hasn't manifested yet, but is real now that the filter is about to go live for real. **Status: open, not fixed tonight, worth a follow-up if it's ever observed firing on legitimate content.** + +**Also caught tonight:** checked GitHub notifications before pushing and found CI already failing on this same branch from an earlier commit (`6e8e2755`) — a `basedpyright --strict` error in `runner.py` (`append`/`sorted` on a partially-unknown type). A follow-up commit (`7cd2b29f`) already fixed that one but was stuck on GitHub's `action_required` approval gate, unverified. Ran `basedpyright` locally against all files touched tonight and found a **new** instance of the same error class in my own new code (`runner.py:817`, `len(clean)` where `clean: str | None` — the `is_blank_text` guard proves it's non-empty at runtime but basedpyright doesn't narrow through that call). Fixed (`len(clean or "")`). Re-ran clean: 0 errors across all 9 touched files. + +**Branch:** `fix/tool-call-loop-detection` in `C:\Repos\nanobot`, not yet committed as of this entry — pending Chris's sign-off per the `start-to-finish` skill. diff --git a/nanobot/agent/loop.py b/nanobot/agent/loop.py index 23c555cf4bb..2755f62e40e 100644 --- a/nanobot/agent/loop.py +++ b/nanobot/agent/loop.py @@ -1591,7 +1591,10 @@ def _assemble_outbound( """Assemble the final outbound message from turn results.""" # MessageTool suppression if (mt := self.tools.get("message")) and isinstance(mt, MessageTool) and mt._sent_in_turn: - if not had_injections or stop_reason == "empty_final_response": + if not had_injections or stop_reason in ( + "empty_final_response", + "leaked_tool_call_markup", + ): return None if log_content: diff --git a/nanobot/agent/runner.py b/nanobot/agent/runner.py index 7d03674b350..9a163c2741d 100644 --- a/nanobot/agent/runner.py +++ b/nanobot/agent/runner.py @@ -4,6 +4,7 @@ import asyncio import inspect +import json import os from collections.abc import Awaitable, Callable, Iterable from copy import deepcopy @@ -39,6 +40,7 @@ from nanobot.utils.helpers import ( IncrementalThinkExtractor, build_assistant_message, + contains_leaked_tool_call_markup, estimate_message_tokens, estimate_prompt_tokens_chain, extract_reasoning, @@ -49,6 +51,7 @@ from nanobot.utils.prompt_templates import render_template from nanobot.utils.runtime import ( EMPTY_FINAL_RESPONSE_MESSAGE, + LEAKED_TOOL_CALL_FINAL_RESPONSE_MESSAGE, build_budget_exhausted_finalization_message, build_finalization_retry_message, build_goal_continue_message, @@ -431,6 +434,10 @@ async def _run_core( external_lookup_counts: dict[str, int] = {} # Per-turn throttle for repeated attempts against the same outside target. workspace_violation_counts: dict[str, int] = {} + # Per-turn loop guard: the last few tool-call signatures (name + args), + # most recent last. Used to warn the model when it repeats the exact + # same call several times in a row instead of making progress. + recent_tool_signatures: list[str] = [] empty_content_retries = 0 # Segments from one uninterrupted length-recovery chain. Tool work or # injected user input starts a new logical answer and clears the chain. @@ -523,6 +530,13 @@ async def _run_core( response, ) messages.append(assistant_message) + + loop_warning = self._detect_tool_call_loop( + response.tool_calls, recent_tool_signatures + ) or self._detect_intra_round_duplicate_calls(response.tool_calls) + if loop_warning: + messages.append({"role": "system", "content": loop_warning}) + await self._emit_checkpoint( spec, { @@ -790,6 +804,35 @@ async def _run_core( length_recovery_parts.clear() continue break + if contains_leaked_tool_call_markup(clean): + # Same last-mile safety net as the max-iterations finalize path + # (above): a model asked to finalize can still write literal + # ... text instead of a real answer. This is the + # dominant path where most turns actually end, so it needs its + # own guard rather than relying on the retry-path check alone. + logger.warning( + "Leaked tool-call markup in final response for {}; " + "substituting fallback ({} chars)", + spec.session_key or "default", + len(clean or ""), + ) + final_content = LEAKED_TOOL_CALL_FINAL_RESPONSE_MESSAGE + stop_reason = "leaked_tool_call_markup" + error = final_content + self._append_final_message(messages, final_content) + context.final_content = final_content + context.error = error + context.stop_reason = stop_reason + await hook.after_iteration(context) + should_continue, injection_cycles = await self._try_drain_injections( + spec, messages, None, injection_cycles, + phase="after leaked tool-call markup", + ) + if should_continue: + had_injections = True + length_recovery_parts.clear() + continue + break messages.append( assistant_message @@ -1218,12 +1261,14 @@ async def _try_finalize_after_max_iterations( raw_usage = self._usage_or_estimate(spec, retry_messages, response) self._accumulate_usage(usage, raw_usage) - if response.finish_reason == "error" or response.has_tool_calls: + leaked_tool_call = contains_leaked_tool_call_markup(response.content) + if response.finish_reason == "error" or response.has_tool_calls or leaked_tool_call: logger.warning( "Budget-exhausted finalization returned finish_reason='{}' " - "with {} tool call(s) for {}; using fallback", + "with {} tool call(s){} for {}; using fallback", response.finish_reason, len(response.tool_calls), + " (leaked tool-call markup in content)" if leaked_tool_call else "", spec.session_key or "default", ) return None @@ -1277,6 +1322,97 @@ def _max_iterations_fallback(spec: AgentRunSpec) -> str: max_iterations=spec.max_iterations, ) + @staticmethod + def _single_call_signature(tc: ToolCallRequest) -> str: + """Build a stable signature for one tool call (name + arguments).""" + if isinstance(tc.arguments, str): + args_repr = tc.arguments + else: + try: + args_repr = json.dumps(tc.arguments, sort_keys=True, default=str) + except (TypeError, ValueError): + args_repr = str(tc.arguments) + return f"{tc.name}:{args_repr}" + + @staticmethod + def _tool_call_signature(tool_calls: Iterable[ToolCallRequest]) -> str: + """Build a stable signature for one round of tool calls (name + args). + + Order-independent (sorted) since concurrent_tools can execute several + calls in one round and their relative order isn't semantically + meaningful for loop detection. + """ + parts = [AgentRunner._single_call_signature(tc) for tc in tool_calls] + return "|".join(sorted(parts)) + + @staticmethod + def _detect_intra_round_duplicate_calls( + tool_calls: Iterable[ToolCallRequest], + *, + threshold: int = 3, + ) -> str | None: + """Detect several identical calls batched into a single round. + + Complements ``_detect_tool_call_loop``, which only tracks one + signature per whole round and so only catches a loop that repeats + across separate rounds -- it cannot see ``threshold`` identical + calls issued together in one round (e.g. three parallel read_file + calls with the same path), since that round produces its own + distinct joined signature just once. This checks within a single + round instead, so it fires the first time such a round occurs + rather than requiring it to repeat. + """ + counts: dict[str, int] = {} + for tc in tool_calls: + signature = AgentRunner._single_call_signature(tc) + counts[signature] = counts.get(signature, 0) + 1 + if not any(count >= threshold for count in counts.values()): + return None + return ( + f"You have just made the exact same tool call {threshold}+ times " + "in a single turn (identical tool name and arguments, issued " + "together). Repeating an identical call will not produce a " + "different result. Stop repeating this action -- either try a " + "genuinely different approach, or report what you have found so " + "far and ask how to proceed." + ) + + @staticmethod + def _detect_tool_call_loop( + tool_calls: Iterable[ToolCallRequest], + recent_tool_signatures: list[str], + *, + threshold: int = 3, + ) -> str | None: + """Track recent tool-call signatures and return a warning once the + exact same round of calls repeats ``threshold`` times in a row. + + Mutates ``recent_tool_signatures`` in place (bounded to ``threshold`` + entries) and clears it after a warning fires, so the same loop won't + re-trigger the warning every single iteration once it's already been + flagged once. + """ + signature = AgentRunner._tool_call_signature(tool_calls) + if not signature: + return None + recent_tool_signatures.append(signature) + if len(recent_tool_signatures) > threshold: + recent_tool_signatures.pop(0) + if ( + len(recent_tool_signatures) == threshold + and len(set(recent_tool_signatures)) == 1 + ): + recent_tool_signatures.clear() + return ( + f"You have just made the exact same tool call(s) {threshold} times " + "in a row (identical tool name and arguments). Repeating an " + "identical call will not produce a different result. Stop " + "repeating this action -- either try a genuinely different " + "approach, or report what you have found so far and ask how " + "to proceed." + ) + return None + def _usage_or_estimate( self, spec: AgentRunSpec, diff --git a/nanobot/channels/email/runtime.py b/nanobot/channels/email/runtime.py index f3aaefa5f9d..a9aeddf91fe 100644 --- a/nanobot/channels/email/runtime.py +++ b/nanobot/channels/email/runtime.py @@ -283,6 +283,9 @@ async def send(self, msg: OutboundMessage) -> None: failed_attachments.append(f"[attachment: {filename} - send failed]") self.logger.exception("Failed to attach file {}", media_path) + # Leaked tool-call markup is filtered centrally in ChannelManager._send_once + # before any channel's send() is invoked -- see contains_leaked_tool_call_markup + # usage there. No per-channel check needed here. content = msg.content or "" if failed_attachments: fallback = "\n".join(failed_attachments) diff --git a/nanobot/channels/email/tests/test_email_channel.py b/nanobot/channels/email/tests/test_email_channel.py index 7e4a30a9db2..aa0293a8aad 100644 --- a/nanobot/channels/email/tests/test_email_channel.py +++ b/nanobot/channels/email/tests/test_email_channel.py @@ -1867,3 +1867,41 @@ def send_message(self, msg: EmailMessage): attachment_parts.append(part) assert len(attachment_parts) == 1 assert attachment_parts[0].get_filename() == "summary.pdf" + + +@pytest.mark.asyncio +async def test_send_passes_through_normal_content_unchanged(monkeypatch) -> None: + """Negative case for the leak filter -- ordinary replies, including ones + that mention code or angle brackets in prose, must not be mangled.""" + sent_messages: list[EmailMessage] = [] + + class FakeSMTP: + def __init__(self, _host: str, _port: int, timeout: int = 30) -> None: + self.timeout = timeout + + def __enter__(self): + return self + + def __exit__(self, exc_type, exc, tb): + return False + + def starttls(self, context=None): + return None + + def login(self, _user: str, _pw: str): + return None + + def send_message(self, msg: EmailMessage): + sent_messages.append(msg) + + monkeypatch.setattr("nanobot.channels.email.runtime.smtplib.SMTP", lambda h, p, timeout=30: FakeSMTP(h, p, timeout=timeout)) + + normal = "Scan complete: 3 findings, all low severity. See in the dashboard." + channel = EmailChannel(_make_config(), MessageBus()) + await channel.send( + OutboundMessage(channel="email", chat_id="alice@example.com", content=normal) + ) + + assert len(sent_messages) == 1 + body = sent_messages[0].get_body(preferencelist=("plain",)).get_content() + assert body.strip() == normal diff --git a/nanobot/channels/manager.py b/nanobot/channels/manager.py index 15fd5276878..8a592caa78b 100644 --- a/nanobot/channels/manager.py +++ b/nanobot/channels/manager.py @@ -34,11 +34,13 @@ ) from nanobot.channels.registry import channel_default_enabled from nanobot.config.schema import Config +from nanobot.utils.helpers import contains_leaked_tool_call_markup from nanobot.utils.restart import ( RestartNotice, consume_restart_notice_from_env, format_restart_completed_message, ) +from nanobot.utils.runtime import LEAKED_TOOL_CALL_FINAL_RESPONSE_MESSAGE if TYPE_CHECKING: from nanobot.cron.service import CronService @@ -835,6 +837,22 @@ async def _send_once(channel: BaseChannel, msg: OutboundMessage) -> None: elif isinstance(event, StreamEndEvent): await ChannelManager._send_stream_event(channel, msg, event) elif not isinstance(event, StreamedResponseEvent): + if contains_leaked_tool_call_markup(msg.content): + # Last-mile safety net for every channel: whatever produced + # this (a model finalizing with no tools offered and + # emitting tool-call-shaped text anyway, or any other path) + # should never reach a user-facing channel as raw unexecuted + # tool syntax. Content is deliberately not logged -- it may + # contain leaked tool arguments (shell commands, session + # IDs, etc). + logger.warning( + "Blocked outbound message to {}:{} containing leaked " + "tool-call markup ({} chars)", + msg.channel, + msg.chat_id, + len(msg.content or ""), + ) + msg.content = LEAKED_TOOL_CALL_FINAL_RESPONSE_MESSAGE await channel.send(msg) def _coalesce_stream_deltas( diff --git a/nanobot/utils/helpers.py b/nanobot/utils/helpers.py index 1f1720dd343..556dfbcf3f4 100644 --- a/nanobot/utils/helpers.py +++ b/nanobot/utils/helpers.py @@ -227,6 +227,21 @@ def strip_reasoning_tags(text: object) -> str: return text.strip() +_LEAKED_TOOL_CALL_RE = re.compile( + r"|", + re.IGNORECASE, +) + + +def contains_leaked_tool_call_markup(text: object) -> bool: + """True if text contains tool-call syntax that was never actually + executed -- e.g. a model asked to finalize with no tools offered still + writing literal `` instead of a real answer. + Used as a content-level safety net anywhere a response might reach a + user-facing channel without having gone through normal tool dispatch.""" + return isinstance(text, str) and bool(_LEAKED_TOOL_CALL_RE.search(text)) + + def extract_think(text: str) -> tuple[str | None, str]: """Extract thinking content from inline thinking tags. diff --git a/nanobot/utils/runtime.py b/nanobot/utils/runtime.py index fc850648cba..b8943ef62e8 100644 --- a/nanobot/utils/runtime.py +++ b/nanobot/utils/runtime.py @@ -21,6 +21,11 @@ "Please try again or narrow the task." ) +LEAKED_TOOL_CALL_FINAL_RESPONSE_MESSAGE = ( + "I ran into an internal formatting issue producing this response. " + "No action was taken from this reply -- please try again." +) + FINALIZATION_RETRY_PROMPT = ( "Please provide your response to the user based on the conversation above." ) diff --git a/tests/agent/test_runner_safety.py b/tests/agent/test_runner_safety.py index 3819a3e5a84..93d03adae83 100644 --- a/tests/agent/test_runner_safety.py +++ b/tests/agent/test_runner_safety.py @@ -239,3 +239,310 @@ async def test_runner_throttles_repeated_workspace_bypass_attempts(): "expected at least one escalated workspace_violation event, got: " f"{result.tool_events}" ) + + +@pytest.mark.asyncio +async def test_runner_warns_on_repeated_identical_tool_call(): + """Loop guard: 3 identical (name + args) tool-call rounds in a row inject + a system warning into the conversation. The warning informs, it does not + block -- all four iterations must still run, and the warning must fire + exactly once, not once per repeat. + """ + repeated_call = ToolCallRequest( + id="call_x", name="read_file", arguments={"path": "/workspace/a.md"}, + ) + provider = MagicMock() + provider.chat_with_retry = AsyncMock(side_effect=[ + LLMResponse(content="", tool_calls=[repeated_call]), + LLMResponse(content="", tool_calls=[repeated_call]), + LLMResponse(content="", tool_calls=[repeated_call]), + LLMResponse(content="giving up on that path", tool_calls=[]), + ]) + tools = MagicMock() + tools.get_definitions.return_value = [] + tools.execute = AsyncMock(return_value=ToolResult("not found", is_error=False)) + + runner = AgentRunner() + result = await runner.run(make_run_spec( + provider, + initial_messages=[], + tools=tools, + model="test-model", + max_iterations=6, + max_tool_result_chars=_MAX_TOOL_RESULT_CHARS, + )) + + assert provider.chat_with_retry.await_count == 4, ( + "the loop guard must not short-circuit execution -- all four " + "iterations (three repeats plus the final differing response) " + "should still run" + ) + warnings = [ + m for m in result.messages + if m.get("role") == "system" and "same tool call" in m.get("content", "") + ] + assert len(warnings) == 1, ( + f"expected exactly one loop warning, got {len(warnings)}: {warnings}" + ) + + +@pytest.mark.asyncio +async def test_runner_does_not_warn_on_two_repeats_or_varied_calls(): + """Two identical calls in a row is not (yet) a loop; varying the + arguments between calls must never trigger a false-positive warning. + """ + provider = MagicMock() + provider.chat_with_retry = AsyncMock(side_effect=[ + LLMResponse(content="", tool_calls=[ToolCallRequest( + id="c1", name="read_file", arguments={"path": "/workspace/a.md"}, + )]), + LLMResponse(content="", tool_calls=[ToolCallRequest( + id="c2", name="read_file", arguments={"path": "/workspace/a.md"}, + )]), + LLMResponse(content="", tool_calls=[ToolCallRequest( + id="c3", name="read_file", arguments={"path": "/workspace/b.md"}, + )]), + LLMResponse(content="done", tool_calls=[]), + ]) + tools = MagicMock() + tools.get_definitions.return_value = [] + tools.execute = AsyncMock(return_value=ToolResult("ok", is_error=False)) + + runner = AgentRunner() + result = await runner.run(make_run_spec( + provider, + initial_messages=[], + tools=tools, + model="test-model", + max_iterations=6, + max_tool_result_chars=_MAX_TOOL_RESULT_CHARS, + )) + + warnings = [ + m for m in result.messages + if m.get("role") == "system" and "same tool call" in m.get("content", "") + ] + assert warnings == [], f"expected no loop warning, got: {warnings}" + + +@pytest.mark.asyncio +async def test_runner_warns_on_batched_identical_tool_calls_in_one_round(): + """Loop guard, batched variant: 3 identical (name + args) tool calls + issued together in a single round must warn immediately, on that first + round -- not just when the same round repeats across iterations. This is + the gap _detect_tool_call_loop's one-signature-per-round approach can't + see (three parallel calls in one round produce a distinct joined + signature exactly once, so it would never look like a repeat). + """ + batched_calls = [ + ToolCallRequest(id="c1", name="read_file", arguments={"path": "/workspace/a.md"}), + ToolCallRequest(id="c2", name="read_file", arguments={"path": "/workspace/a.md"}), + ToolCallRequest(id="c3", name="read_file", arguments={"path": "/workspace/a.md"}), + ] + provider = MagicMock() + provider.chat_with_retry = AsyncMock(side_effect=[ + LLMResponse(content="", tool_calls=batched_calls), + LLMResponse(content="done", tool_calls=[]), + ]) + tools = MagicMock() + tools.get_definitions.return_value = [] + tools.execute = AsyncMock(return_value=ToolResult("not found", is_error=False)) + + runner = AgentRunner() + result = await runner.run(make_run_spec( + provider, + initial_messages=[], + tools=tools, + model="test-model", + max_iterations=6, + max_tool_result_chars=_MAX_TOOL_RESULT_CHARS, + )) + + warnings = [ + m for m in result.messages + if m.get("role") == "system" and "same tool call" in m.get("content", "") + ] + assert len(warnings) == 1, ( + f"expected exactly one loop warning on the first (batched) round, got {len(warnings)}: {warnings}" + ) + + +@pytest.mark.asyncio +async def test_runner_does_not_warn_on_two_batched_identical_or_varied_calls(): + """Two identical calls batched in one round is not (yet) a loop; a + third, differently-argued call in the same round must not tip it over + into a false-positive warning. + """ + provider = MagicMock() + provider.chat_with_retry = AsyncMock(side_effect=[ + LLMResponse(content="", tool_calls=[ + ToolCallRequest(id="c1", name="read_file", arguments={"path": "/workspace/a.md"}), + ToolCallRequest(id="c2", name="read_file", arguments={"path": "/workspace/a.md"}), + ToolCallRequest(id="c3", name="read_file", arguments={"path": "/workspace/b.md"}), + ]), + LLMResponse(content="done", tool_calls=[]), + ]) + tools = MagicMock() + tools.get_definitions.return_value = [] + tools.execute = AsyncMock(return_value=ToolResult("ok", is_error=False)) + + runner = AgentRunner() + result = await runner.run(make_run_spec( + provider, + initial_messages=[], + tools=tools, + model="test-model", + max_iterations=6, + max_tool_result_chars=_MAX_TOOL_RESULT_CHARS, + )) + + warnings = [ + m for m in result.messages + if m.get("role") == "system" and "same tool call" in m.get("content", "") + ] + assert warnings == [], f"expected no loop warning, got: {warnings}" + + +@pytest.mark.asyncio +async def test_runner_rejects_leaked_tool_call_markup_after_max_iterations(): + """Reporter scenario 2026-08-11: a model asked to finalize with no tools + offered (has_tool_calls is therefore always False) can still emit + tool-call-shaped text instead of a real answer. That text must never + become the final response the user sees -- it should fall back to the + safe max-iterations template instead. + """ + tool_call = ToolCallRequest( + id="c1", name="read_file", arguments={"path": "/workspace/a.md"}, + ) + provider = MagicMock() + provider.chat_with_retry = AsyncMock(side_effect=[ + # Both real iterations keep calling tools, so max_iterations is hit. + LLMResponse(content="", tool_calls=[tool_call]), + LLMResponse(content="", tool_calls=[tool_call]), + # The finalize-with-no-tools retry: no structured tool_calls (none + # were offered), but the content is leaked tool-call markup anyway. + LLMResponse( + content=( + "\n\n\n" + 'Remove-Item "C:\\temp\\file.vtt" -Force\n\n' + "\n" + ), + tool_calls=[], + ), + ]) + tools = MagicMock() + tools.get_definitions.return_value = [] + tools.execute = AsyncMock(return_value=ToolResult("ok", is_error=False)) + + runner = AgentRunner() + result = await runner.run(make_run_spec( + provider, + initial_messages=[], + tools=tools, + model="test-model", + max_iterations=2, + max_tool_result_chars=_MAX_TOOL_RESULT_CHARS, + )) + + assert result.stop_reason == "max_iterations" + assert result.final_content is not None + assert "\n\n\n" + 'Remove-Item "C:\\temp\\file.vtt" -Force\n\n' + "\n" + ), + tool_calls=[], + ), + ]) + tools = MagicMock() + tools.get_definitions.return_value = [] + + runner = AgentRunner() + result = await runner.run(make_run_spec( + provider, + initial_messages=[], + tools=tools, + model="test-model", + max_iterations=5, + max_tool_result_chars=_MAX_TOOL_RESULT_CHARS, + )) + + assert result.stop_reason == "leaked_tool_call_markup" + assert result.final_content is not None + assert " None: + leaked = ( + "\n\n\n" + "51c8aae161d1\n\n\n" + ) + msg = OutboundMessage(channel="mock", chat_id="alice", content=leaked) + + await ChannelManager._send_once(channel, msg) + + channel._send_mock.assert_awaited_once() + sent_msg = channel._send_mock.await_args.args[0] + assert " None: + """Regression check for the plural wrapper tag the original + regex missed (verified live: ).""" + leaked = "" + msg = OutboundMessage(channel="mock", chat_id="alice", content=leaked) + + await ChannelManager._send_once(channel, msg) + + sent_msg = channel._send_mock.await_args.args[0] + assert " None: + """Negative case for the leak filter -- ordinary replies, including ones + that mention code or angle brackets in prose, must not be mangled.""" + normal = "Scan complete: 3 findings, all low severity. See in the dashboard." + msg = OutboundMessage(channel="mock", chat_id="alice", content=normal) + + await ChannelManager._send_once(channel, msg) + + sent_msg = channel._send_mock.await_args.args[0] + assert sent_msg.content == normal diff --git a/tests/tools/test_message_tool_suppress.py b/tests/tools/test_message_tool_suppress.py index 4e1542ccdb6..df056fc9067 100644 --- a/tests/tools/test_message_tool_suppress.py +++ b/tests/tools/test_message_tool_suppress.py @@ -123,6 +123,51 @@ async def test_injected_followup_with_message_tool_does_not_emit_empty_fallback( assert sent[0].content == "Tool reply" assert result is None + @pytest.mark.asyncio + async def test_injected_followup_with_message_tool_suppresses_leaked_markup_notice( + self, tmp_path: Path + ) -> None: + """Companion to the empty-fallback case above: once MessageTool has + already sent real content this turn, a leaked-tool-call-markup + fallback notice is suppressed the same way an empty response is, + even when a mid-turn injection occurred.""" + loop = _make_loop(tmp_path) + tool_call = ToolCallRequest( + id="call1", name="message", + arguments={"content": "Tool reply", "channel": "feishu", "chat_id": "chat123"}, + ) + calls = iter([ + LLMResponse(content="First answer", tool_calls=[]), + LLMResponse(content="", tool_calls=[tool_call]), + LLMResponse( + content=( + "\n\n\n" + 'Remove-Item "C:\\temp\\file.vtt" -Force\n\n' + "\n" + ), + tool_calls=[], + ), + ]) + loop.provider.chat_with_retry = AsyncMock(side_effect=lambda *a, **kw: next(calls)) + loop.tools.get_definitions = MagicMock(return_value=[]) + + sent: list[OutboundMessage] = [] + mt = loop.tools.get("message") + if isinstance(mt, MessageTool): + mt.set_send_callback(AsyncMock(side_effect=lambda m: sent.append(m))) + + pending_queue = asyncio.Queue() + await pending_queue.put( + InboundMessage(channel="feishu", sender_id="user1", chat_id="chat123", content="follow-up") + ) + + msg = InboundMessage(channel="feishu", sender_id="user1", chat_id="chat123", content="Start") + result = await loop._process_message(msg, pending_queue=pending_queue) + + assert len(sent) == 1 + assert sent[0].content == "Tool reply" + assert result is None + async def test_progress_hides_internal_reasoning(self, tmp_path: Path) -> None: loop = _make_loop(tmp_path) tool_call = ToolCallRequest(id="call1", name="read_file", arguments={"path": "foo.txt"})