From 62bd54ac4a2433ecfaf10c478980d22c25a65cf0 Mon Sep 17 00:00:00 2001 From: chengyongru Date: Mon, 13 Apr 2026 11:27:16 +0800 Subject: [PATCH 01/12] fix(agent): skip auto-compact for sessions with active agent tasks Prevent proactive compaction from archiving sessions that have an in-flight agent task, avoiding mid-turn context truncation when a task runs longer than the idle TTL. --- nanobot/agent/autocompact.py | 17 ++++-- nanobot/agent/loop.py | 5 +- tests/agent/test_auto_compact.py | 100 ++++++++++++++++++++++++++++++- 3 files changed, 115 insertions(+), 7 deletions(-) diff --git a/nanobot/agent/autocompact.py b/nanobot/agent/autocompact.py index 47c7b5a3..ce70337c 100644 --- a/nanobot/agent/autocompact.py +++ b/nanobot/agent/autocompact.py @@ -2,6 +2,7 @@ from __future__ import annotations +from collections.abc import Collection from datetime import datetime from typing import TYPE_CHECKING, Any, Callable, Coroutine @@ -23,12 +24,13 @@ class AutoCompact: self._archiving: set[str] = set() self._summaries: dict[str, tuple[str, datetime]] = {} - def _is_expired(self, ts: datetime | str | None) -> bool: + def _is_expired(self, ts: datetime | str | None, + now: datetime | None = None) -> bool: if self._ttl <= 0 or not ts: return False if isinstance(ts, str): ts = datetime.fromisoformat(ts) - return (datetime.now() - ts).total_seconds() >= self._ttl * 60 + return ((now or datetime.now()) - ts).total_seconds() >= self._ttl * 60 @staticmethod def _format_summary(text: str, last_active: datetime) -> str: @@ -56,10 +58,17 @@ class AutoCompact: cut = len(tail) - len(kept) return tail[:cut], kept - def check_expired(self, schedule_background: Callable[[Coroutine], None]) -> None: + def check_expired(self, schedule_background: Callable[[Coroutine], None], + active_session_keys: Collection[str] = ()) -> None: + """Schedule archival for idle sessions, skipping those with in-flight agent tasks.""" + now = datetime.now() for info in self.sessions.list_sessions(): key = info.get("key", "") - if key and key not in self._archiving and self._is_expired(info.get("updated_at")): + if not key or key in self._archiving: + continue + if key in active_session_keys: + continue + if self._is_expired(info.get("updated_at"), now): self._archiving.add(key) logger.debug("Auto-compact: scheduling archival for {} (idle > {} min)", key, self._ttl) schedule_background(self._archive(key)) diff --git a/nanobot/agent/loop.py b/nanobot/agent/loop.py index 5631e12a..5c4fbfc4 100644 --- a/nanobot/agent/loop.py +++ b/nanobot/agent/loop.py @@ -433,7 +433,10 @@ class AgentLoop: try: msg = await asyncio.wait_for(self.bus.consume_inbound(), timeout=1.0) except asyncio.TimeoutError: - self.auto_compact.check_expired(self._schedule_background) + self.auto_compact.check_expired( + self._schedule_background, + active_session_keys=self._pending_queues.keys(), + ) continue except asyncio.CancelledError: # Preserve real task cancellation so shutdown can complete cleanly. diff --git a/tests/agent/test_auto_compact.py b/tests/agent/test_auto_compact.py index b3462820..1f6886ed 100644 --- a/tests/agent/test_auto_compact.py +++ b/tests/agent/test_auto_compact.py @@ -560,9 +560,12 @@ class TestProactiveAutoCompact: """Test proactive auto-new on idle ticks (TimeoutError path in run loop).""" @staticmethod - async def _run_check_expired(loop): + async def _run_check_expired(loop, active_session_keys=()): """Helper: run check_expired via callback and wait for background tasks.""" - loop.auto_compact.check_expired(loop._schedule_background) + loop.auto_compact.check_expired( + loop._schedule_background, + active_session_keys=active_session_keys, + ) await asyncio.sleep(0.1) @pytest.mark.asyncio @@ -701,6 +704,99 @@ class TestProactiveAutoCompact: assert not archive_called await loop.close_mcp() + @pytest.mark.asyncio + async def test_skip_expired_session_with_active_agent_task(self, tmp_path): + """Expired session with an active agent task should NOT be archived.""" + loop = _make_loop(tmp_path, session_ttl_minutes=15) + session = loop.sessions.get_or_create("cli:test") + _add_turns(session, 6, prefix="old") + session.updated_at = datetime.now() - timedelta(minutes=20) + loop.sessions.save(session) + + archive_count = 0 + + async def _fake_archive(messages): + nonlocal archive_count + archive_count += 1 + return "Summary." + + loop.consolidator.archive = _fake_archive + + # Simulate an active agent task for this session + await self._run_check_expired(loop, active_session_keys={"cli:test"}) + assert archive_count == 0 + + session_after = loop.sessions.get_or_create("cli:test") + assert len(session_after.messages) == 12 # All messages preserved + + await loop.close_mcp() + + @pytest.mark.asyncio + async def test_archive_after_active_task_completes(self, tmp_path): + """Session should be archived on next tick after active task completes.""" + loop = _make_loop(tmp_path, session_ttl_minutes=15) + session = loop.sessions.get_or_create("cli:test") + _add_turns(session, 6, prefix="old") + session.updated_at = datetime.now() - timedelta(minutes=20) + loop.sessions.save(session) + + archive_count = 0 + + async def _fake_archive(messages): + nonlocal archive_count + archive_count += 1 + return "Summary." + + loop.consolidator.archive = _fake_archive + + # First tick: active task, skip + await self._run_check_expired(loop, active_session_keys={"cli:test"}) + assert archive_count == 0 + + # Second tick: task completed, should archive + await self._run_check_expired(loop) + assert archive_count == 1 + await loop.close_mcp() + + @pytest.mark.asyncio + async def test_partial_active_set_only_archives_inactive_expired(self, tmp_path): + """With multiple sessions, only the expired+inactive one should be archived.""" + loop = _make_loop(tmp_path, session_ttl_minutes=15) + # Session A: expired, no active task -> should be archived + s1 = loop.sessions.get_or_create("cli:expired_idle") + _add_turns(s1, 6, prefix="old_a") + s1.updated_at = datetime.now() - timedelta(minutes=20) + loop.sessions.save(s1) + # Session B: expired, has active task -> should be skipped + s2 = loop.sessions.get_or_create("cli:expired_active") + _add_turns(s2, 6, prefix="old_b") + s2.updated_at = datetime.now() - timedelta(minutes=20) + loop.sessions.save(s2) + # Session C: recent, no active task -> should be skipped + s3 = loop.sessions.get_or_create("cli:recent") + s3.add_message("user", "recent") + loop.sessions.save(s3) + + archive_count = 0 + + async def _fake_archive(messages): + nonlocal archive_count + archive_count += 1 + return "Summary." + + loop.consolidator.archive = _fake_archive + + await self._run_check_expired(loop, active_session_keys={"cli:expired_active"}) + + assert archive_count == 1 + s1_after = loop.sessions.get_or_create("cli:expired_idle") + assert len(s1_after.messages) == loop.auto_compact._RECENT_SUFFIX_MESSAGES + s2_after = loop.sessions.get_or_create("cli:expired_active") + assert len(s2_after.messages) == 12 # Preserved + s3_after = loop.sessions.get_or_create("cli:recent") + assert len(s3_after.messages) == 1 # Preserved + await loop.close_mcp() + @pytest.mark.asyncio async def test_no_reschedule_after_successful_archive(self, tmp_path): """Already-archived session should NOT be re-scheduled on subsequent ticks.""" From 89ea2375fdb7cffc98aab5b32e315ccc84fb5550 Mon Sep 17 00:00:00 2001 From: chengyongru Date: Mon, 13 Apr 2026 11:30:54 +0800 Subject: [PATCH 02/12] fix(provider): recover trailing assistant message as user to prevent empty request When a subagent result is injected with current_role="assistant", _enforce_role_alternation drops the trailing assistant message, leaving only the system prompt. Providers like Zhipu/GLM reject such requests with error 1214 ("messages parameter invalid"). Now the last popped assistant message is recovered as a user message when no user/tool messages remain. --- nanobot/providers/base.py | 16 +++++++- .../test_enforce_role_alternation.py | 41 +++++++++++++++++++ 2 files changed, 56 insertions(+), 1 deletion(-) diff --git a/nanobot/providers/base.py b/nanobot/providers/base.py index 8ce2b9a7..759d880a 100644 --- a/nanobot/providers/base.py +++ b/nanobot/providers/base.py @@ -392,8 +392,22 @@ class LLMProvider(ABC): else: merged.append(dict(msg)) + last_popped = None while merged and merged[-1].get("role") == "assistant": - merged.pop() + last_popped = merged.pop() + + # If removing trailing assistant messages left only system messages, + # the request would be invalid for most providers (e.g. Zhipu/GLM + # error 1214). Recover by converting the last popped assistant + # message to a user message so the LLM can still see the content. + if ( + merged + and last_popped is not None + and not any(m.get("role") in ("user", "tool") for m in merged) + ): + recovered = dict(last_popped) + recovered["role"] = "user" + merged.append(recovered) return merged diff --git a/tests/providers/test_enforce_role_alternation.py b/tests/providers/test_enforce_role_alternation.py index aef57f47..333c5d04 100644 --- a/tests/providers/test_enforce_role_alternation.py +++ b/tests/providers/test_enforce_role_alternation.py @@ -131,6 +131,47 @@ class TestEnforceRoleAlternation: assert msgs[0] == original_first assert len(msgs) == 2 + def test_trailing_assistant_recovered_as_user_when_only_system_remains(self): + """Subagent result injected as assistant message must not be silently dropped. + + When build_messages(current_role="assistant") produces [system, assistant], + _enforce_role_alternation would drop the assistant, leaving only [system]. + Most providers (e.g. Zhipu/GLM error 1214) reject such requests. + The trailing assistant should be recovered as a user message instead. + """ + msgs = [ + {"role": "system", "content": "You are helpful."}, + {"role": "assistant", "content": "Subagent completed successfully."}, + ] + result = LLMProvider._enforce_role_alternation(msgs) + assert len(result) == 2 + assert result[0]["role"] == "system" + assert result[1]["role"] == "user" + assert "Subagent completed successfully." in result[1]["content"] + + def test_trailing_assistant_not_recovered_when_user_message_present(self): + """Recovery should NOT happen when a user message already exists.""" + msgs = [ + {"role": "system", "content": "You are helpful."}, + {"role": "user", "content": "Hi"}, + {"role": "assistant", "content": "Hello!"}, + ] + result = LLMProvider._enforce_role_alternation(msgs) + assert len(result) == 2 + assert result[-1]["role"] == "user" + + def test_trailing_assistant_recovered_with_tool_result_preceding(self): + """When only [system, tool, assistant] remains, recovery is not needed + because tool messages are valid non-system content.""" + msgs = [ + {"role": "system", "content": "You are helpful."}, + {"role": "tool", "content": "result", "tool_call_id": "1"}, + {"role": "assistant", "content": "Done."}, + ] + result = LLMProvider._enforce_role_alternation(msgs) + assert len(result) == 2 + assert result[-1]["role"] == "tool" + def test_only_assistant_messages(self): msgs = [ {"role": "assistant", "content": "A"}, From b311759e87fc56e8ff12f1bc2dcbdf648ee52723 Mon Sep 17 00:00:00 2001 From: chengyongru Date: Mon, 13 Apr 2026 16:03:15 +0800 Subject: [PATCH 03/12] fix(log): remove noisy no-op logs from auto-compact Remove two debug log lines that fire on every idle channel check: - "scheduling archival" (logged before knowing if there's work) - "skipping, no un-consolidated messages" (the common no-op path) The meaningful "archived" info log (only on real work) is preserved. --- nanobot/agent/autocompact.py | 2 -- 1 file changed, 2 deletions(-) diff --git a/nanobot/agent/autocompact.py b/nanobot/agent/autocompact.py index ce70337c..9d9c1e29 100644 --- a/nanobot/agent/autocompact.py +++ b/nanobot/agent/autocompact.py @@ -70,7 +70,6 @@ class AutoCompact: continue if self._is_expired(info.get("updated_at"), now): self._archiving.add(key) - logger.debug("Auto-compact: scheduling archival for {} (idle > {} min)", key, self._ttl) schedule_background(self._archive(key)) async def _archive(self, key: str) -> None: @@ -79,7 +78,6 @@ class AutoCompact: session = self.sessions.get_or_create(key) archive_msgs, kept_msgs = self._split_unconsolidated(session) if not archive_msgs and not kept_msgs: - logger.debug("Auto-compact: skipping {}, no un-consolidated messages", key) session.updated_at = datetime.now() self.sessions.save(session) return From b3288fbc87e31a992d92f92e71bf6b1cd58fd24d Mon Sep 17 00:00:00 2001 From: chengyongru Date: Mon, 13 Apr 2026 16:49:01 +0800 Subject: [PATCH 04/12] fix(log): only log auto-compact when messages are actually archived --- nanobot/agent/autocompact.py | 15 ++++++++------- 1 file changed, 8 insertions(+), 7 deletions(-) diff --git a/nanobot/agent/autocompact.py b/nanobot/agent/autocompact.py index 9d9c1e29..eabd8615 100644 --- a/nanobot/agent/autocompact.py +++ b/nanobot/agent/autocompact.py @@ -93,13 +93,14 @@ class AutoCompact: session.last_consolidated = 0 session.updated_at = datetime.now() self.sessions.save(session) - logger.info( - "Auto-compact: archived {} (archived={}, kept={}, summary={})", - key, - len(archive_msgs), - len(kept_msgs), - bool(summary), - ) + if archive_msgs: + logger.info( + "Auto-compact: archived {} (archived={}, kept={}, summary={})", + key, + len(archive_msgs), + len(kept_msgs), + bool(summary), + ) except Exception: logger.exception("Auto-compact: failed for {}", key) finally: From 0750d1f182a0a751f08b7d5c95cbd378da0d071c Mon Sep 17 00:00:00 2001 From: moranfong <274257964+zijiefang@users.noreply.github.com> Date: Mon, 13 Apr 2026 23:42:58 +0800 Subject: [PATCH 05/12] fix(config): return provider default api base in config resolution --- nanobot/config/schema.py | 6 ++---- tests/cli/test_commands.py | 33 +++++++++++++++++++++++++++++++++ 2 files changed, 35 insertions(+), 4 deletions(-) diff --git a/nanobot/config/schema.py b/nanobot/config/schema.py index aa5ab993..fb891515 100644 --- a/nanobot/config/schema.py +++ b/nanobot/config/schema.py @@ -304,17 +304,15 @@ class Config(BaseSettings): return p.api_key if p else None def get_api_base(self, model: str | None = None) -> str | None: - """Get API base URL for the given model. Applies default URLs for gateway/local providers.""" + """Get API base URL for the given model, falling back to the provider default when present.""" from nanobot.providers.registry import find_by_name p, name = self._match_provider(model) if p and p.api_base: return p.api_base - # Only gateways get a default api_base here. Standard providers - # resolve their base URL from the registry in the provider constructor. if name: spec = find_by_name(name) - if spec and (spec.is_gateway or spec.is_local) and spec.default_api_base: + if spec and spec.default_api_base: return spec.default_api_base return None diff --git a/tests/cli/test_commands.py b/tests/cli/test_commands.py index 3a1e7145..fd5429d8 100644 --- a/tests/cli/test_commands.py +++ b/tests/cli/test_commands.py @@ -264,6 +264,39 @@ def test_find_by_name_accepts_camel_case_and_hyphen_aliases(): assert find_by_name("github-copilot").name == "github_copilot" +def test_config_explicit_xiaomi_mimo_provider_uses_default_api_base(): + config = Config.model_validate( + { + "agents": { + "defaults": { + "provider": "xiaomi_mimo", + "model": "MiniMax-M1-80k", + } + }, + "providers": { + "xiaomiMimo": { + "apiKey": "test-key", + } + }, + } + ) + + assert config.get_provider_name() == "xiaomi_mimo" + assert config.get_api_base() == "https://api.xiaomimimo.com/v1" + + +def test_config_auto_detects_xiaomi_mimo_from_model_keyword(): + config = Config.model_validate( + { + "agents": {"defaults": {"provider": "auto", "model": "mimo/MiniMax-M1-80k"}}, + "providers": {"xiaomiMimo": {"apiKey": "test-key"}}, + } + ) + + assert config.get_provider_name() == "xiaomi_mimo" + assert config.get_api_base() == "https://api.xiaomimimo.com/v1" + + def test_config_auto_detects_ollama_from_local_api_base(): config = Config.model_validate( { From 655f3d2cc53c66c46ef650ed50b31501df48e628 Mon Sep 17 00:00:00 2001 From: yeyitech Date: Tue, 14 Apr 2026 12:40:23 +0800 Subject: [PATCH 06/12] fix: harden cron tool contract and repeat guard --- nanobot/agent/runner.py | 42 ++++++++---- nanobot/agent/tools/cron.py | 103 +++++++++++++++++++++--------- nanobot/utils/runtime.py | 34 ++++++++++ tests/agent/test_runner.py | 42 ++++++++++++ tests/cron/test_cron_tool_list.py | 42 +++++++++++- tests/utils/test_runtime.py | 20 ++++++ 6 files changed, 242 insertions(+), 41 deletions(-) create mode 100644 tests/utils/test_runtime.py diff --git a/nanobot/agent/runner.py b/nanobot/agent/runner.py index 592af9de..c7006caa 100644 --- a/nanobot/agent/runner.py +++ b/nanobot/agent/runner.py @@ -3,15 +3,14 @@ from __future__ import annotations import asyncio -from dataclasses import dataclass, field import inspect +from dataclasses import dataclass, field from pathlib import Path from typing import Any from loguru import logger from nanobot.agent.hook import AgentHook, AgentHookContext -from nanobot.utils.prompt_templates import render_template from nanobot.agent.tools.registry import ToolRegistry from nanobot.providers.base import LLMProvider, ToolCallRequest from nanobot.utils.helpers import ( @@ -22,6 +21,7 @@ from nanobot.utils.helpers import ( maybe_persist_tool_result, truncate_text, ) +from nanobot.utils.prompt_templates import render_template from nanobot.utils.runtime import ( EMPTY_FINAL_RESPONSE_MESSAGE, build_finalization_retry_message, @@ -29,6 +29,7 @@ from nanobot.utils.runtime import ( ensure_nonempty_tool_result, is_blank_text, repeated_external_lookup_error, + repeated_tool_call_error, ) _DEFAULT_ERROR_MESSAGE = "Sorry, I encountered an error calling the AI model." @@ -233,6 +234,7 @@ class AgentRunner: stop_reason = "completed" tool_events: list[dict[str, str]] = [] external_lookup_counts: dict[str, int] = {} + tool_call_counts: dict[str, int] = {} empty_content_retries = 0 length_recovery_count = 0 had_injections = False @@ -303,6 +305,7 @@ class AgentRunner: spec, response.tool_calls, external_lookup_counts, + tool_call_counts, ) tool_events.extend(new_events) context.tool_results = list(results) @@ -616,18 +619,21 @@ class AgentRunner: spec: AgentRunSpec, tool_calls: list[ToolCallRequest], external_lookup_counts: dict[str, int], + tool_call_counts: dict[str, int], ) -> tuple[list[Any], list[dict[str, str]], BaseException | None]: batches = self._partition_tool_batches(spec, tool_calls) tool_results: list[tuple[Any, dict[str, str], BaseException | None]] = [] for batch in batches: if spec.concurrent_tools and len(batch) > 1: tool_results.extend(await asyncio.gather(*( - self._run_tool(spec, tool_call, external_lookup_counts) + self._run_tool(spec, tool_call, external_lookup_counts, tool_call_counts) for tool_call in batch ))) else: for tool_call in batch: - tool_results.append(await self._run_tool(spec, tool_call, external_lookup_counts)) + tool_results.append( + await self._run_tool(spec, tool_call, external_lookup_counts, tool_call_counts) + ) results: list[Any] = [] events: list[dict[str, str]] = [] @@ -644,8 +650,9 @@ class AgentRunner: spec: AgentRunSpec, tool_call: ToolCallRequest, external_lookup_counts: dict[str, int], + tool_call_counts: dict[str, int], ) -> tuple[Any, dict[str, str], BaseException | None]: - _HINT = "\n\n[Analyze the error above and try a different approach.]" + _hint = "\n\n[Analyze the error above and try a different approach.]" lookup_error = repeated_external_lookup_error( tool_call.name, tool_call.arguments, @@ -658,8 +665,22 @@ class AgentRunner: "detail": "repeated external lookup blocked", } if spec.fail_on_tool_error: - return lookup_error + _HINT, event, RuntimeError(lookup_error) - return lookup_error + _HINT, event, None + return lookup_error + _hint, event, RuntimeError(lookup_error) + return lookup_error + _hint, event, None + repeat_error = repeated_tool_call_error( + tool_call.name, + tool_call.arguments, + tool_call_counts, + ) + if repeat_error: + event = { + "name": tool_call.name, + "status": "error", + "detail": "repeated identical tool call blocked", + } + if spec.fail_on_tool_error: + return repeat_error + _hint, event, RuntimeError(repeat_error) + return repeat_error + _hint, event, None prepare_call = getattr(spec.tools, "prepare_call", None) tool, params, prep_error = None, tool_call.arguments, None if callable(prepare_call): @@ -675,7 +696,7 @@ class AgentRunner: "status": "error", "detail": prep_error.split(": ", 1)[-1][:120], } - return prep_error + _HINT, event, RuntimeError(prep_error) if spec.fail_on_tool_error else None + return prep_error + _hint, event, RuntimeError(prep_error) if spec.fail_on_tool_error else None try: if tool is not None: result = await tool.execute(**params) @@ -700,8 +721,8 @@ class AgentRunner: "detail": result.replace("\n", " ").strip()[:120], } if spec.fail_on_tool_error: - return result + _HINT, event, RuntimeError(result) - return result + _HINT, event, None + return result + _hint, event, RuntimeError(result) + return result + _hint, event, None detail = "" if result is None else str(result) detail = detail.replace("\n", " ").strip() @@ -966,4 +987,3 @@ class AgentRunner: if current: batches.append(current) return batches - diff --git a/nanobot/agent/tools/cron.py b/nanobot/agent/tools/cron.py index f0d3ddab..dd2d9d43 100644 --- a/nanobot/agent/tools/cron.py +++ b/nanobot/agent/tools/cron.py @@ -5,39 +5,72 @@ from datetime import datetime from typing import Any from nanobot.agent.tools.base import Tool, tool_parameters -from nanobot.agent.tools.schema import BooleanSchema, IntegerSchema, StringSchema, tool_parameters_schema +from nanobot.agent.tools.schema import ( + BooleanSchema, + IntegerSchema, + StringSchema, + tool_parameters_schema, +) from nanobot.cron.service import CronService from nanobot.cron.types import CronJob, CronJobState, CronSchedule +_CRON_PARAMETERS = tool_parameters_schema( + action=StringSchema("Action to perform", enum=["add", "list", "remove"]), + name=StringSchema( + "Optional short human-readable label for the job " + "(e.g., 'weather-monitor', 'daily-standup'). Defaults to first 30 chars of message." + ), + message=StringSchema( + "Instruction for the agent to execute when the job triggers. " + "Required when action='add' " + "(e.g., 'Send a reminder to WeChat: xxx' or 'Check system status and report')" + ), + every_seconds=IntegerSchema(0, description="Interval in seconds (for recurring tasks)"), + cron_expr=StringSchema("Cron expression like '0 9 * * *' (for scheduled tasks)"), + tz=StringSchema( + "Optional IANA timezone for cron expressions (e.g. 'America/Vancouver'). " + "When omitted with cron_expr, the tool's default timezone applies." + ), + at=StringSchema( + "ISO datetime for one-time execution (e.g. '2026-02-12T10:30:00'). " + "Naive values use the tool's default timezone." + ), + deliver=BooleanSchema( + description="Whether to deliver the execution result to the user channel (default true)", + default=True, + ), + job_id=StringSchema("Job ID (for remove)"), + required=["action"], + description=( + "Action-specific parameters: add requires a non-empty message plus one schedule " + "(every_seconds, cron_expr, or at); remove requires job_id; list only needs action." + ), +) +_CRON_PARAMETERS["oneOf"] = [ + { + "properties": { + "action": {"enum": ["add"]}, + "message": {"type": "string", "minLength": 1}, + }, + "required": ["action", "message"], + }, + { + "properties": { + "action": {"enum": ["list"]}, + }, + "required": ["action"], + }, + { + "properties": { + "action": {"enum": ["remove"]}, + }, + "required": ["action", "job_id"], + }, +] + @tool_parameters( - tool_parameters_schema( - action=StringSchema("Action to perform", enum=["add", "list", "remove"]), - name=StringSchema( - "Optional short human-readable label for the job " - "(e.g., 'weather-monitor', 'daily-standup'). Defaults to first 30 chars of message." - ), - message=StringSchema( - "Instruction for the agent to execute when the job triggers " - "(e.g., 'Send a reminder to WeChat: xxx' or 'Check system status and report')" - ), - every_seconds=IntegerSchema(0, description="Interval in seconds (for recurring tasks)"), - cron_expr=StringSchema("Cron expression like '0 9 * * *' (for scheduled tasks)"), - tz=StringSchema( - "Optional IANA timezone for cron expressions (e.g. 'America/Vancouver'). " - "When omitted with cron_expr, the tool's default timezone applies." - ), - at=StringSchema( - "ISO datetime for one-time execution (e.g. '2026-02-12T10:30:00'). " - "Naive values use the tool's default timezone." - ), - deliver=BooleanSchema( - description="Whether to deliver the execution result to the user channel (default true)", - default=True, - ), - job_id=StringSchema("Job ID (for remove)"), - required=["action"], - ) + _CRON_PARAMETERS ) class CronTool(Tool): """Tool to schedule reminders and recurring tasks.""" @@ -94,6 +127,15 @@ class CronTool(Tool): f"If tz is omitted, cron expressions and naive ISO times default to {self._default_timezone}." ) + def validate_params(self, params: dict[str, Any]) -> list[str]: + errors = super().validate_params(params) + action = params.get("action") + if action == "add" and not str(params.get("message") or "").strip(): + errors.append("message is required when action='add'") + if action == "remove" and not str(params.get("job_id") or "").strip(): + errors.append("job_id is required when action='remove'") + return errors + async def execute( self, action: str, @@ -128,7 +170,10 @@ class CronTool(Tool): deliver: bool = True, ) -> str: if not message: - return "Error: message is required for add" + return ( + "Error: cron action='add' requires a non-empty 'message' parameter " + "describing what to do when the job triggers. Retry including message=\"...\"." + ) if not self._channel or not self._chat_id: return "Error: no session context (channel/chat_id)" if tz and not cron_expr: diff --git a/nanobot/utils/runtime.py b/nanobot/utils/runtime.py index 39822fd4..0101443e 100644 --- a/nanobot/utils/runtime.py +++ b/nanobot/utils/runtime.py @@ -2,6 +2,7 @@ from __future__ import annotations +import json from typing import Any from loguru import logger @@ -9,6 +10,7 @@ from loguru import logger from nanobot.utils.helpers import stringify_text_blocks _MAX_REPEAT_EXTERNAL_LOOKUPS = 2 +_MAX_REPEAT_TOOL_CALLS = 2 EMPTY_FINAL_RESPONSE_MESSAGE = ( "I completed the tool steps but couldn't produce a final answer. " @@ -73,6 +75,15 @@ def external_lookup_signature(tool_name: str, arguments: dict[str, Any]) -> str return None +def tool_call_signature(tool_name: str, arguments: dict[str, Any]) -> str: + """Stable signature for repeated tool calls across retries.""" + try: + args_json = json.dumps(arguments, sort_keys=True, default=str, ensure_ascii=True) + except Exception: + args_json = repr(sorted(arguments.items())) + return f"{tool_name}:{args_json}" + + def repeated_external_lookup_error( tool_name: str, arguments: dict[str, Any], @@ -95,3 +106,26 @@ def repeated_external_lookup_error( "Error: repeated external lookup blocked. " "Use the results you already have to answer, or try a meaningfully different source." ) + + +def repeated_tool_call_error( + tool_name: str, + arguments: dict[str, Any], + seen_counts: dict[str, int], +) -> str | None: + """Block repeated identical tool calls after a small retry budget.""" + signature = tool_call_signature(tool_name, arguments) + count = seen_counts.get(signature, 0) + 1 + seen_counts[signature] = count + if count <= _MAX_REPEAT_TOOL_CALLS: + return None + logger.warning( + "Blocking repeated tool call {} on attempt {}", + signature[:160], + count, + ) + return ( + f"Error: repeated identical call to '{tool_name}' blocked after {count - 1} attempts. " + "The previous attempts used the same arguments. Change the arguments or try a different " + "approach." + ) diff --git a/tests/agent/test_runner.py b/tests/agent/test_runner.py index 74025d77..69a5f51e 100644 --- a/tests/agent/test_runner.py +++ b/tests/agent/test_runner.py @@ -798,6 +798,48 @@ async def test_runner_blocks_repeated_external_fetches(): assert "repeated external lookup blocked" in blocked_tool_message["content"] +@pytest.mark.asyncio +async def test_runner_blocks_repeated_identical_tool_calls(): + from nanobot.agent.runner import AgentRunSpec, AgentRunner + + provider = MagicMock() + captured_final_call: list[dict] = [] + call_count = {"n": 0} + + async def chat_with_retry(*, messages, **kwargs): + call_count["n"] += 1 + if call_count["n"] <= 3: + return LLMResponse( + content="working", + tool_calls=[ToolCallRequest(id=f"call_{call_count['n']}", name="read_file", arguments={"path": "memory/history.jsonl", "limit": 50, "offset": 1})], + usage={}, + ) + captured_final_call[:] = messages + return LLMResponse(content="done", tool_calls=[], usage={}) + + provider.chat_with_retry = chat_with_retry + tools = MagicMock() + tools.get_definitions.return_value = [] + tools.execute = AsyncMock(return_value="file content") + + runner = AgentRunner(provider) + result = await runner.run(AgentRunSpec( + initial_messages=[{"role": "user", "content": "what happened recently?"}], + tools=tools, + model="test-model", + max_iterations=4, + max_tool_result_chars=_MAX_TOOL_RESULT_CHARS, + )) + + assert result.final_content == "done" + assert tools.execute.await_count == 2 + blocked_tool_message = [ + msg for msg in captured_final_call + if msg.get("role") == "tool" and msg.get("tool_call_id") == "call_3" + ][0] + assert "repeated identical call to 'read_file' blocked" in blocked_tool_message["content"] + + @pytest.mark.asyncio async def test_loop_max_iterations_message_stays_stable(tmp_path): loop = _make_loop(tmp_path) diff --git a/tests/cron/test_cron_tool_list.py b/tests/cron/test_cron_tool_list.py index 86f3055c..a3ee9b1a 100644 --- a/tests/cron/test_cron_tool_list.py +++ b/tests/cron/test_cron_tool_list.py @@ -7,7 +7,6 @@ import pytest from nanobot.agent.tools.cron import CronTool from nanobot.cron.service import CronService from nanobot.cron.types import CronJob, CronJobState, CronPayload, CronSchedule -from tests.test_openai_api import pytest_plugins def _make_tool(tmp_path) -> CronTool: @@ -346,6 +345,47 @@ def test_add_job_can_disable_delivery(tmp_path) -> None: assert job.payload.deliver is False +def test_cron_schema_advertises_action_specific_requirements(tmp_path) -> None: + tool = _make_tool(tmp_path) + + assert tool.parameters["required"] == ["action"] + assert tool.parameters["oneOf"] == [ + { + "properties": { + "action": {"enum": ["add"]}, + "message": {"type": "string", "minLength": 1}, + }, + "required": ["action", "message"], + }, + { + "properties": {"action": {"enum": ["list"]}}, + "required": ["action"], + }, + { + "properties": {"action": {"enum": ["remove"]}}, + "required": ["action", "job_id"], + }, + ] + + +def test_validate_params_requires_message_only_for_add(tmp_path) -> None: + tool = _make_tool(tmp_path) + + assert "message is required when action='add'" in tool.validate_params({"action": "add"}) + assert tool.validate_params({"action": "list"}) == [] + assert "job_id is required when action='remove'" in tool.validate_params({"action": "remove"}) + + +def test_add_job_empty_message_returns_actionable_error(tmp_path) -> None: + tool = _make_tool(tmp_path) + tool.set_context("telegram", "chat-1") + + result = tool._add_job(None, "", 60, None, None, None) + + assert "action='add' requires a non-empty 'message'" in result + assert "Retry including message=" in result + + def test_list_excludes_disabled_jobs(tmp_path) -> None: tool = _make_tool(tmp_path) job = tool._cron.add_job( diff --git a/tests/utils/test_runtime.py b/tests/utils/test_runtime.py new file mode 100644 index 00000000..aa25e155 --- /dev/null +++ b/tests/utils/test_runtime.py @@ -0,0 +1,20 @@ +from nanobot.utils.runtime import repeated_tool_call_error, tool_call_signature + + +def test_tool_call_signature_sorts_arguments_stably() -> None: + first = tool_call_signature("read_file", {"offset": 1, "path": "memory/history.jsonl"}) + second = tool_call_signature("read_file", {"path": "memory/history.jsonl", "offset": 1}) + + assert first == second + + +def test_repeated_tool_call_error_blocks_after_two_attempts() -> None: + seen: dict[str, int] = {} + + assert repeated_tool_call_error("read_file", {"path": "a.txt"}, seen) is None + assert repeated_tool_call_error("read_file", {"path": "a.txt"}, seen) is None + + error = repeated_tool_call_error("read_file", {"path": "a.txt"}, seen) + + assert error is not None + assert "repeated identical call to 'read_file' blocked after 2 attempts" in error From fb28678b641726ec8a2e35e73672d62d6623a45e Mon Sep 17 00:00:00 2001 From: longle325 Date: Sat, 18 Apr 2026 23:16:04 +0700 Subject: [PATCH 07/12] fix: prevent GitStore from creating nested repos and overwriting .gitignore (#2980) GitStore.init() now checks if the workspace is already inside a git repository before calling porcelain.init(). If so, it refuses to create a nested repo. Additionally, existing .gitignore files are preserved by appending only missing Dream-specific entries rather than overwriting. Closes #2980 --- .gitignore | 1 + nanobot/utils/gitstore.py | 38 +++++++++++++++- tests/utils/test_gitstore.py | 85 +++++++++++++++++++++++++++++++++++- 3 files changed, 121 insertions(+), 3 deletions(-) diff --git a/.gitignore b/.gitignore index 054e5ce7..18d4df7e 100644 --- a/.gitignore +++ b/.gitignore @@ -92,3 +92,4 @@ logs/ tmp/ temp/ *.tmp +.oss/ diff --git a/nanobot/utils/gitstore.py b/nanobot/utils/gitstore.py index e51a63cc..ffa241e7 100644 --- a/nanobot/utils/gitstore.py +++ b/nanobot/utils/gitstore.py @@ -64,14 +64,35 @@ class GitStore: if self.is_initialized(): return False + if self._is_inside_git_repo(): + logger.warning( + "Workspace {} is already inside a git repo; " + "skipping nested repo initialization", + self._workspace, + ) + return False + try: from dulwich import porcelain porcelain.init(str(self._workspace)) - # Write .gitignore + # Write .gitignore (merge with existing if present) gitignore = self._workspace / ".gitignore" - gitignore.write_text(self._build_gitignore(), encoding="utf-8") + dream_entries = self._build_gitignore() + if gitignore.exists(): + existing = gitignore.read_text(encoding="utf-8") + existing_lines = set(existing.splitlines()) + new_lines = [ + line + for line in dream_entries.splitlines() + if line not in existing_lines + ] + if new_lines: + merged = existing.rstrip("\n") + "\n" + "\n".join(new_lines) + "\n" + gitignore.write_text(merged, encoding="utf-8") + else: + gitignore.write_text(dream_entries, encoding="utf-8") # Ensure tracked files exist (touch them if missing) so the initial # commit has something to track. @@ -155,6 +176,19 @@ class GitStore: except Exception: return None + def _is_inside_git_repo(self) -> bool: + """Check if self._workspace is already inside a git repository. + + Walks up from self._workspace to the filesystem root, returning True + if any parent directory contains a .git directory. + """ + current = self._workspace.resolve() + while current != current.parent: + if (current / ".git").is_dir(): + return True + current = current.parent + return False + def _build_gitignore(self) -> str: """Generate .gitignore content from tracked files.""" dirs: set[str] = set() diff --git a/tests/utils/test_gitstore.py b/tests/utils/test_gitstore.py index 8c401e38..b7ee0ef2 100644 --- a/tests/utils/test_gitstore.py +++ b/tests/utils/test_gitstore.py @@ -1,7 +1,7 @@ """Tests for GitStore — line_ages() and core git operations.""" import time -from datetime import datetime, timezone, timedelta +from datetime import datetime, timedelta, timezone from unittest.mock import patch import pytest @@ -89,3 +89,86 @@ class TestLineAges: # "- new" line and "- keep" line both age=0 (same day), but # the key point is we get per-line results assert len(ages) == 7 + + +class TestNestedRepoProtection: + """Regression tests for GitHub issue #2980: nested repo protection.""" + + def test_init_refuses_inside_git_repo(self, tmp_path): + """init() should detect it's inside an existing git repo and refuse.""" + project = tmp_path / "project" + project.mkdir() + (project / ".git").mkdir() + + workspace = project / "workspace" + workspace.mkdir() + + g = GitStore(workspace, tracked_files=["MEMORY.md"]) + result = g.init() + + assert result is False + assert not (workspace / ".git").is_dir() + + def test_init_preserves_existing_gitignore(self, tmp_path): + """init() should preserve existing .gitignore entries and append new ones.""" + workspace = tmp_path / "workspace" + workspace.mkdir() + + existing = "*.pyc\n__pycache__/\n" + (workspace / ".gitignore").write_text(existing, encoding="utf-8") + + g = GitStore(workspace, tracked_files=["MEMORY.md"]) + result = g.init() + + assert result is True + gitignore = (workspace / ".gitignore").read_text(encoding="utf-8") + assert "*.pyc" in gitignore + assert "__pycache__/" in gitignore + assert "!MEMORY.md" in gitignore + assert "!.gitignore" in gitignore + + def test_init_no_gitignore_creates_new(self, tmp_path): + """init() should create .gitignore with Dream content when none exists.""" + workspace = tmp_path / "workspace" + workspace.mkdir() + + g = GitStore(workspace, tracked_files=["MEMORY.md"]) + result = g.init() + + assert result is True + gitignore = (workspace / ".gitignore").read_text(encoding="utf-8") + expected = g._build_gitignore() + assert gitignore == expected + + def test_init_gitignore_merge_idempotent(self, tmp_path): + """init() should not duplicate Dream entries already in .gitignore.""" + workspace = tmp_path / "workspace" + workspace.mkdir() + + # Pre-existing .gitignore that already has some Dream entries + existing = "*.pyc\n/*\n!MEMORY.md\n" + (workspace / ".gitignore").write_text(existing, encoding="utf-8") + + g = GitStore(workspace, tracked_files=["MEMORY.md"]) + result = g.init() + + assert result is True + gitignore = (workspace / ".gitignore").read_text(encoding="utf-8") + # No duplicate lines + lines = gitignore.splitlines() + assert lines.count("/*") == 1 + assert lines.count("!MEMORY.md") == 1 + # Existing entry preserved, new Dream entries appended + assert "*.pyc" in gitignore + assert "!.gitignore" in gitignore + + def test_init_outside_git_repo_works_normally(self, tmp_path): + """init() should succeed and create .git when not inside a git repo.""" + workspace = tmp_path / "workspace" + workspace.mkdir() + + g = GitStore(workspace, tracked_files=["MEMORY.md"]) + result = g.init() + + assert result is True + assert (workspace / ".git").is_dir() From ff5b97dc3493fb859f76022fa3cf6a527073a40a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?L=C3=AA=20B=E1=BA=A3o=20Long?= <140832783+longle325@users.noreply.github.com> Date: Sat, 18 Apr 2026 23:24:25 +0700 Subject: [PATCH 08/12] Remove .oss from .gitignore --- .gitignore | 1 - 1 file changed, 1 deletion(-) diff --git a/.gitignore b/.gitignore index 18d4df7e..054e5ce7 100644 --- a/.gitignore +++ b/.gitignore @@ -92,4 +92,3 @@ logs/ tmp/ temp/ *.tmp -.oss/ From e08507f3ce4c8462224bd1b63b93da3b0b201a7c Mon Sep 17 00:00:00 2001 From: Xubin Ren Date: Sat, 18 Apr 2026 19:35:06 +0000 Subject: [PATCH 09/12] fix: handle git worktrees in GitStore nested repo protection Treat `.git` files the same as `.git` directories so GitStore refuses to initialize inside git worktrees, and add a focused regression test for that checkout shape. Made-with: Cursor --- nanobot/utils/gitstore.py | 7 ++++-- tests/utils/test_gitstore.py | 42 ++++++++++++++++++++++++++++++++++++ 2 files changed, 47 insertions(+), 2 deletions(-) diff --git a/nanobot/utils/gitstore.py b/nanobot/utils/gitstore.py index ffa241e7..d9b528c9 100644 --- a/nanobot/utils/gitstore.py +++ b/nanobot/utils/gitstore.py @@ -180,11 +180,14 @@ class GitStore: """Check if self._workspace is already inside a git repository. Walks up from self._workspace to the filesystem root, returning True - if any parent directory contains a .git directory. + if any parent directory contains a .git entry. + + Git worktrees and submodules can use a ``.git`` file instead of a + directory, so we must treat either form as "already inside a repo". """ current = self._workspace.resolve() while current != current.parent: - if (current / ".git").is_dir(): + if (current / ".git").exists(): return True current = current.parent return False diff --git a/tests/utils/test_gitstore.py b/tests/utils/test_gitstore.py index b7ee0ef2..b431bf71 100644 --- a/tests/utils/test_gitstore.py +++ b/tests/utils/test_gitstore.py @@ -1,5 +1,6 @@ """Tests for GitStore — line_ages() and core git operations.""" +import subprocess import time from datetime import datetime, timedelta, timezone from unittest.mock import patch @@ -172,3 +173,44 @@ class TestNestedRepoProtection: assert result is True assert (workspace / ".git").is_dir() + + def test_init_refuses_inside_git_worktree(self, tmp_path): + """init() should refuse when the parent checkout is a git worktree.""" + repo = tmp_path / "repo" + repo.mkdir() + subprocess.run(["git", "init", "-q", str(repo)], check=True) + (repo / "README.md").write_text("x\n", encoding="utf-8") + subprocess.run(["git", "-C", str(repo), "add", "README.md"], check=True) + subprocess.run( + [ + "git", + "-C", + str(repo), + "-c", + "user.name=test", + "-c", + "user.email=test@example.com", + "commit", + "-q", + "-m", + "init", + ], + check=True, + ) + subprocess.run(["git", "-C", str(repo), "branch", "wt-branch"], check=True) + + worktree = tmp_path / "worktree" + subprocess.run( + ["git", "-C", str(repo), "worktree", "add", "-q", str(worktree), "wt-branch"], + check=True, + ) + assert (worktree / ".git").is_file() + + workspace = worktree / "workspace" + workspace.mkdir() + + g = GitStore(workspace, tracked_files=["MEMORY.md"]) + result = g.init() + + assert result is False + assert not (workspace / ".git").exists() From 9c0dc8b2761a94c70bf20e4fb6593d43ef902f7e Mon Sep 17 00:00:00 2001 From: Xubin Ren Date: Sat, 18 Apr 2026 19:59:58 +0000 Subject: [PATCH 10/12] fix: drop generic repeated tool-call guard The global guard changed baseline agent and subagent behavior without proving a real no-progress loop. Keep this PR focused on the cron contract hardening and validation fixes. Made-with: Cursor --- nanobot/agent/runner.py | 42 ++++++++++--------------------------- nanobot/utils/runtime.py | 34 ------------------------------ tests/agent/test_runner.py | 42 ------------------------------------- tests/utils/test_runtime.py | 20 ------------------ 4 files changed, 11 insertions(+), 127 deletions(-) delete mode 100644 tests/utils/test_runtime.py diff --git a/nanobot/agent/runner.py b/nanobot/agent/runner.py index 8221076c..d90c79fe 100644 --- a/nanobot/agent/runner.py +++ b/nanobot/agent/runner.py @@ -3,14 +3,15 @@ from __future__ import annotations import asyncio -import inspect from dataclasses import dataclass, field +import inspect from pathlib import Path from typing import Any from loguru import logger from nanobot.agent.hook import AgentHook, AgentHookContext +from nanobot.utils.prompt_templates import render_template from nanobot.agent.tools.registry import ToolRegistry from nanobot.providers.base import LLMProvider, ToolCallRequest from nanobot.utils.helpers import ( @@ -21,7 +22,6 @@ from nanobot.utils.helpers import ( maybe_persist_tool_result, truncate_text, ) -from nanobot.utils.prompt_templates import render_template from nanobot.utils.runtime import ( EMPTY_FINAL_RESPONSE_MESSAGE, build_finalization_retry_message, @@ -29,7 +29,6 @@ from nanobot.utils.runtime import ( ensure_nonempty_tool_result, is_blank_text, repeated_external_lookup_error, - repeated_tool_call_error, ) _DEFAULT_ERROR_MESSAGE = "Sorry, I encountered an error calling the AI model." @@ -235,7 +234,6 @@ class AgentRunner: stop_reason = "completed" tool_events: list[dict[str, str]] = [] external_lookup_counts: dict[str, int] = {} - tool_call_counts: dict[str, int] = {} empty_content_retries = 0 length_recovery_count = 0 had_injections = False @@ -306,7 +304,6 @@ class AgentRunner: spec, response.tool_calls, external_lookup_counts, - tool_call_counts, ) tool_events.extend(new_events) context.tool_results = list(results) @@ -627,21 +624,18 @@ class AgentRunner: spec: AgentRunSpec, tool_calls: list[ToolCallRequest], external_lookup_counts: dict[str, int], - tool_call_counts: dict[str, int], ) -> tuple[list[Any], list[dict[str, str]], BaseException | None]: batches = self._partition_tool_batches(spec, tool_calls) tool_results: list[tuple[Any, dict[str, str], BaseException | None]] = [] for batch in batches: if spec.concurrent_tools and len(batch) > 1: tool_results.extend(await asyncio.gather(*( - self._run_tool(spec, tool_call, external_lookup_counts, tool_call_counts) + self._run_tool(spec, tool_call, external_lookup_counts) for tool_call in batch ))) else: for tool_call in batch: - tool_results.append( - await self._run_tool(spec, tool_call, external_lookup_counts, tool_call_counts) - ) + tool_results.append(await self._run_tool(spec, tool_call, external_lookup_counts)) results: list[Any] = [] events: list[dict[str, str]] = [] @@ -658,9 +652,8 @@ class AgentRunner: spec: AgentRunSpec, tool_call: ToolCallRequest, external_lookup_counts: dict[str, int], - tool_call_counts: dict[str, int], ) -> tuple[Any, dict[str, str], BaseException | None]: - _hint = "\n\n[Analyze the error above and try a different approach.]" + _HINT = "\n\n[Analyze the error above and try a different approach.]" lookup_error = repeated_external_lookup_error( tool_call.name, tool_call.arguments, @@ -673,22 +666,8 @@ class AgentRunner: "detail": "repeated external lookup blocked", } if spec.fail_on_tool_error: - return lookup_error + _hint, event, RuntimeError(lookup_error) - return lookup_error + _hint, event, None - repeat_error = repeated_tool_call_error( - tool_call.name, - tool_call.arguments, - tool_call_counts, - ) - if repeat_error: - event = { - "name": tool_call.name, - "status": "error", - "detail": "repeated identical tool call blocked", - } - if spec.fail_on_tool_error: - return repeat_error + _hint, event, RuntimeError(repeat_error) - return repeat_error + _hint, event, None + return lookup_error + _HINT, event, RuntimeError(lookup_error) + return lookup_error + _HINT, event, None prepare_call = getattr(spec.tools, "prepare_call", None) tool, params, prep_error = None, tool_call.arguments, None if callable(prepare_call): @@ -704,7 +683,7 @@ class AgentRunner: "status": "error", "detail": prep_error.split(": ", 1)[-1][:120], } - return prep_error + _hint, event, RuntimeError(prep_error) if spec.fail_on_tool_error else None + return prep_error + _HINT, event, RuntimeError(prep_error) if spec.fail_on_tool_error else None try: if tool is not None: result = await tool.execute(**params) @@ -729,8 +708,8 @@ class AgentRunner: "detail": result.replace("\n", " ").strip()[:120], } if spec.fail_on_tool_error: - return result + _hint, event, RuntimeError(result) - return result + _hint, event, None + return result + _HINT, event, RuntimeError(result) + return result + _HINT, event, None detail = "" if result is None else str(result) detail = detail.replace("\n", " ").strip() @@ -1005,3 +984,4 @@ class AgentRunner: if current: batches.append(current) return batches + diff --git a/nanobot/utils/runtime.py b/nanobot/utils/runtime.py index 0101443e..39822fd4 100644 --- a/nanobot/utils/runtime.py +++ b/nanobot/utils/runtime.py @@ -2,7 +2,6 @@ from __future__ import annotations -import json from typing import Any from loguru import logger @@ -10,7 +9,6 @@ from loguru import logger from nanobot.utils.helpers import stringify_text_blocks _MAX_REPEAT_EXTERNAL_LOOKUPS = 2 -_MAX_REPEAT_TOOL_CALLS = 2 EMPTY_FINAL_RESPONSE_MESSAGE = ( "I completed the tool steps but couldn't produce a final answer. " @@ -75,15 +73,6 @@ def external_lookup_signature(tool_name: str, arguments: dict[str, Any]) -> str return None -def tool_call_signature(tool_name: str, arguments: dict[str, Any]) -> str: - """Stable signature for repeated tool calls across retries.""" - try: - args_json = json.dumps(arguments, sort_keys=True, default=str, ensure_ascii=True) - except Exception: - args_json = repr(sorted(arguments.items())) - return f"{tool_name}:{args_json}" - - def repeated_external_lookup_error( tool_name: str, arguments: dict[str, Any], @@ -106,26 +95,3 @@ def repeated_external_lookup_error( "Error: repeated external lookup blocked. " "Use the results you already have to answer, or try a meaningfully different source." ) - - -def repeated_tool_call_error( - tool_name: str, - arguments: dict[str, Any], - seen_counts: dict[str, int], -) -> str | None: - """Block repeated identical tool calls after a small retry budget.""" - signature = tool_call_signature(tool_name, arguments) - count = seen_counts.get(signature, 0) + 1 - seen_counts[signature] = count - if count <= _MAX_REPEAT_TOOL_CALLS: - return None - logger.warning( - "Blocking repeated tool call {} on attempt {}", - signature[:160], - count, - ) - return ( - f"Error: repeated identical call to '{tool_name}' blocked after {count - 1} attempts. " - "The previous attempts used the same arguments. Change the arguments or try a different " - "approach." - ) diff --git a/tests/agent/test_runner.py b/tests/agent/test_runner.py index 0350d234..b47db948 100644 --- a/tests/agent/test_runner.py +++ b/tests/agent/test_runner.py @@ -854,48 +854,6 @@ async def test_runner_blocks_repeated_external_fetches(): assert "repeated external lookup blocked" in blocked_tool_message["content"] -@pytest.mark.asyncio -async def test_runner_blocks_repeated_identical_tool_calls(): - from nanobot.agent.runner import AgentRunSpec, AgentRunner - - provider = MagicMock() - captured_final_call: list[dict] = [] - call_count = {"n": 0} - - async def chat_with_retry(*, messages, **kwargs): - call_count["n"] += 1 - if call_count["n"] <= 3: - return LLMResponse( - content="working", - tool_calls=[ToolCallRequest(id=f"call_{call_count['n']}", name="read_file", arguments={"path": "memory/history.jsonl", "limit": 50, "offset": 1})], - usage={}, - ) - captured_final_call[:] = messages - return LLMResponse(content="done", tool_calls=[], usage={}) - - provider.chat_with_retry = chat_with_retry - tools = MagicMock() - tools.get_definitions.return_value = [] - tools.execute = AsyncMock(return_value="file content") - - runner = AgentRunner(provider) - result = await runner.run(AgentRunSpec( - initial_messages=[{"role": "user", "content": "what happened recently?"}], - tools=tools, - model="test-model", - max_iterations=4, - max_tool_result_chars=_MAX_TOOL_RESULT_CHARS, - )) - - assert result.final_content == "done" - assert tools.execute.await_count == 2 - blocked_tool_message = [ - msg for msg in captured_final_call - if msg.get("role") == "tool" and msg.get("tool_call_id") == "call_3" - ][0] - assert "repeated identical call to 'read_file' blocked" in blocked_tool_message["content"] - - @pytest.mark.asyncio async def test_loop_max_iterations_message_stays_stable(tmp_path): loop = _make_loop(tmp_path) diff --git a/tests/utils/test_runtime.py b/tests/utils/test_runtime.py deleted file mode 100644 index aa25e155..00000000 --- a/tests/utils/test_runtime.py +++ /dev/null @@ -1,20 +0,0 @@ -from nanobot.utils.runtime import repeated_tool_call_error, tool_call_signature - - -def test_tool_call_signature_sorts_arguments_stably() -> None: - first = tool_call_signature("read_file", {"offset": 1, "path": "memory/history.jsonl"}) - second = tool_call_signature("read_file", {"path": "memory/history.jsonl", "offset": 1}) - - assert first == second - - -def test_repeated_tool_call_error_blocks_after_two_attempts() -> None: - seen: dict[str, int] = {} - - assert repeated_tool_call_error("read_file", {"path": "a.txt"}, seen) is None - assert repeated_tool_call_error("read_file", {"path": "a.txt"}, seen) is None - - error = repeated_tool_call_error("read_file", {"path": "a.txt"}, seen) - - assert error is not None - assert "repeated identical call to 'read_file' blocked after 2 attempts" in error From 261b843839806849d496c3640b8f19d41fc3283d Mon Sep 17 00:00:00 2001 From: Alfredo Arenas Date: Sat, 18 Apr 2026 00:09:20 -0600 Subject: [PATCH 11/12] fix(cli): respect sys.stdout.isatty() in stream renderer (#3265) --- nanobot/cli/stream.py | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/nanobot/cli/stream.py b/nanobot/cli/stream.py index 9454edac..addf4fe7 100644 --- a/nanobot/cli/stream.py +++ b/nanobot/cli/stream.py @@ -18,7 +18,17 @@ from nanobot import __logo__ def _make_console() -> Console: - return Console(file=sys.stdout, force_terminal=True) + """Create a Console that emits plain text when stdout is not a TTY. + + Rich's spinner, Live render, and cursor-visibility escape codes all + key off ``Console.is_terminal``. Forcing ``force_terminal=True`` overrode + the ``isatty()`` check and caused control sequences (``\\x1b[?25l``, + braille spinner frames) to pollute programmatic consumers such as + ``docker exec -i`` or pipes, even with ``NO_COLOR`` or ``TERM=dumb``. + Deferring to ``isatty()`` keeps Rich output in interactive terminals + and plain text everywhere else (#3265). + """ + return Console(file=sys.stdout, force_terminal=sys.stdout.isatty()) class ThinkingSpinner: From 2d0442976e4b58b3a147ce713aa553d6df1d629b Mon Sep 17 00:00:00 2001 From: Alfredo Arenas Date: Sat, 18 Apr 2026 08:23:20 -0600 Subject: [PATCH 12/12] test(cli): update _make_console tests for isatty-based fix (#3265) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The old test `test_make_console_uses_force_terminal` hardcoded `force_terminal is True`, which contradicts the fix: we now defer to sys.stdout.isatty() so piped / non-TTY output gets plain text instead of ANSI escape codes. Split into two tests covering both branches: - test_make_console_force_terminal_when_stdout_is_tty: TTY path (force_terminal=True, rich output) - test_make_console_force_terminal_false_when_stdout_is_not_tty: non-TTY path (force_terminal=False, plain text) — regression guard for the bug reported in #3265 Co-authored with Claude Opus 4.7 --- tests/cli/test_cli_input.py | 20 ++++++++++++++++---- 1 file changed, 16 insertions(+), 4 deletions(-) diff --git a/tests/cli/test_cli_input.py b/tests/cli/test_cli_input.py index b772293b..0e1235b8 100644 --- a/tests/cli/test_cli_input.py +++ b/tests/cli/test_cli_input.py @@ -167,7 +167,19 @@ def test_stream_renderer_stop_for_input_stops_spinner(): spinner.stop.assert_called_once() -def test_make_console_uses_force_terminal(): - """Console should be created with force_terminal=True for proper ANSI handling.""" - console = stream_mod._make_console() - assert console._force_terminal is True +def test_make_console_force_terminal_when_stdout_is_tty(): + """Console should set force_terminal=True when stdout is a TTY (rich output).""" + import sys + with patch.object(sys.stdout, "isatty", return_value=True): + console = stream_mod._make_console() + assert console._force_terminal is True + + +def test_make_console_force_terminal_false_when_stdout_is_not_tty(): + """Console should set force_terminal=False when stdout is not a TTY so that + ANSI escape codes (cursor visibility, braille spinner frames) don't pollute + piped output such as `docker exec -i` (#3265).""" + import sys + with patch.object(sys.stdout, "isatty", return_value=False): + console = stream_mod._make_console() + assert console._force_terminal is False