diff --git a/nanobot/agent/tools/cron.py b/nanobot/agent/tools/cron.py index f1af59d7..111bb1c8 100644 --- a/nanobot/agent/tools/cron.py +++ b/nanobot/agent/tools/cron.py @@ -5,41 +5,71 @@ 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 - -@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( - "REQUIRED when action='add'. Instruction for the agent to execute when the job triggers " - "(e.g., 'Send a reminder to WeChat: xxx' or 'Check system status and report'). " - "Not used for action='list' or action='remove'." - ), - 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("REQUIRED when action='remove'. Job ID to remove (obtain via action='list')."), - required=["action"], - ) +_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( + "REQUIRED when action='add'. Instruction for the agent to execute when the job triggers " + "(e.g., 'Send a reminder to WeChat: xxx' or 'Check system status and report'). " + "Not used for action='list' or action='remove'." + ), + 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("REQUIRED when action='remove'. Job ID to remove (obtain via action='list')."), + 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(_CRON_PARAMETERS) class CronTool(Tool): """Tool to schedule reminders and recurring tasks.""" @@ -95,6 +125,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, @@ -130,8 +169,8 @@ class CronTool(Tool): ) -> str: if not message: return ( - "Error: cron action='add' requires a non-empty 'message' " - "parameter describing what to do when the job triggers " + "Error: cron action='add' requires a non-empty 'message' parameter " + "describing what to do when the job triggers " "(e.g. the reminder text). Retry including message=\"...\"." ) if not self._channel or not self._chat_id: 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: diff --git a/nanobot/config/schema.py b/nanobot/config/schema.py index 66759cb3..ac48619f 100644 --- a/nanobot/config/schema.py +++ b/nanobot/config/schema.py @@ -319,17 +319,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/nanobot/utils/gitstore.py b/nanobot/utils/gitstore.py index e51a63cc..d9b528c9 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,22 @@ 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 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").exists(): + 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/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 diff --git a/tests/cli/test_commands.py b/tests/cli/test_commands.py index a2199195..5966d520 100644 --- a/tests/cli/test_commands.py +++ b/tests/cli/test_commands.py @@ -286,6 +286,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( { 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_gitstore.py b/tests/utils/test_gitstore.py index 8c401e38..b431bf71 100644 --- a/tests/utils/test_gitstore.py +++ b/tests/utils/test_gitstore.py @@ -1,7 +1,8 @@ """Tests for GitStore — line_ages() and core git operations.""" +import subprocess import time -from datetime import datetime, timezone, timedelta +from datetime import datetime, timedelta, timezone from unittest.mock import patch import pytest @@ -89,3 +90,127 @@ 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() + + 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()