diff --git a/docs/agent.md b/docs/agent.md index 053929c..fd1f76c 100644 --- a/docs/agent.md +++ b/docs/agent.md @@ -10,10 +10,10 @@ ### `run(session_id, user_message)` → `str` Non-streaming. Delegates to `run_stream()` and returns the final text. Planning and the full tool loop run; events are consumed internally, not yielded. -### `run_ephemeral(user_message, profile_id, *, max_iterations=40, exclude_tools=(), briefing=None, custom_system_prompt=None, inherit_system_prompt=False, context_transfer=None, parent_session_id=None, timeout_seconds=300.0)` → `tuple[str, bool]` +### `run_ephemeral(user_message, profile_id, *, max_iterations=40, exclude_tools=(), briefing=None, custom_system_prompt=None, inherit_system_prompt=False, context_transfer=None, parent_session_id=None, timeout_seconds=300.0)` → `SubAgentOutcome` Non-persistent subagent. Temporary in-memory context. Called by `SpawnAgentTool`. -Returns `(result_text, completed_normally)`. `completed_normally` is `False` if the subagent hit the iteration limit or timed out. +Returns a `SubAgentOutcome`: `text` (the progress report, always populated) and `status` — one of `ok`, `timeout`, `max_iterations`, `thinking_stall`, `user_stop`, `context_overflow`. `completed` is `True` only for `ok`. `SpawnAgentTool` renders its result header from `status`, so a timeout is not reported to the parent as an iteration limit. `spawn_agent.profile_id` is optional. If omitted, `SpawnAgentTool` resolves the parent session's current profile. If provided, the subagent uses the selected profile's model, `subagent_system_prompt`, planning flags, and tool set. Its tools come from that profile's `tools.subagent`, falling back to `tools.agent` when `tools.subagent` is empty. `exclude_tools` removes specific tools from that set; `briefing`/`custom_system_prompt`/`inherit_system_prompt` shape the system prompt; `context_transfer` injects a synthetic user/assistant exchange before the task message. diff --git a/docs/mechanics.md b/docs/mechanics.md index 47000a0..40ba010 100644 --- a/docs/mechanics.md +++ b/docs/mechanics.md @@ -53,7 +53,7 @@ | Mechanic | Description | Config / Flags | Files | Docs | |---|---|---|---|---| -| **Ephemeral execution** | Runs tool loop without persistent session, temporary in-memory context. Returns `(result_text, completed_normally)`. | `max_iterations` param, `timeout_seconds` param | `agent.py` | ✅ | +| **Ephemeral execution** | Runs tool loop without persistent session, temporary in-memory context. Returns a `SubAgentOutcome` (`text` + `status`: `ok`/`timeout`/`max_iterations`/`thinking_stall`/`user_stop`/`context_overflow`). | `max_iterations` param, `timeout_seconds` param | `agent.py` | ✅ | | **Inherit system prompt** | When `inherit_system_prompt=True`, prepends parent's `profile.system_prompt` as base layer, then subagent specialization on top. | `inherit_system_prompt` param | `agent.py` | ✅ | | **Context transfer priming** | If `context_transfer` provided, injects it as synthetic user/assistant exchange before task message. | `context_transfer` param | `agent.py` | ❌ | | **Wall-clock timeout** | Monitors elapsed time; aborts and returns `[Sub-agent timed out]` if exceeded. | `timeout_seconds` param (default 300.0) | `agent.py` | ✅ | diff --git a/manuals/create_mcp_server.md b/manuals/create_mcp_server.md index a16b03f..6f873d3 100644 --- a/manuals/create_mcp_server.md +++ b/manuals/create_mcp_server.md @@ -142,11 +142,13 @@ | Exit code | Meaning | |---|---| | `124` | `timeout` killed a server that was still running. **Success.** | - | `0` | The server exited on its own before 5 seconds. **Failure** — usually `main()` is missing its parentheses at the bottom of the file. | + | `0` | The server exited on its own before 5 seconds. **Failure** — usually `main()` is missing its parentheses at the bottom of the file, but see the note below before you go looking for one. | | anything else | Traceback or crash. Read it and fix. | Never run the server without `timeout`: an MCP server blocks forever and the terminal hangs. Repeat until you get `124`, then move on to registration. + A `0` with no output at all often means stdin was already closed rather than a crash: an stdio server exits cleanly the moment it reads EOF, and a command run through `terminal` or `code_exec` can hand it a closed pipe. Hold stdin open to tell the two apart — `sleep 30 | timeout 5 .venv/bin/python -m app.mcp_server` gives `124` for a healthy server — and only then go hunting for the missing `main()`. + ## 5. Registering the server in Navi Create a file `mcp_servers.d/.json` in the project root. The filename (without `.json`) becomes the server name. Example for a server named `my_server`: @@ -154,9 +156,9 @@ ```json { "transport": "stdio", - "command": "/absolute/path/to/mcp-servers/my_server/.venv/bin/python", + "command": "./mcp-servers/my_server/.venv/bin/python", "args": ["-m", "app.mcp_server"], - "cwd": "/absolute/path/to/mcp-servers/my_server", + "cwd": "./mcp-servers/my_server", "env": { "MCP_TRANSPORT": "stdio" }, @@ -168,12 +170,14 @@ ``` **Critical fields:** -- `command` — absolute path to the venv's Python binary. -- `cwd` — absolute path to the server directory. +- `command` — path to the venv's Python binary. +- `cwd` — path to the server directory. - The filename must be `.json` (e.g. `my_server.json`). - `args` — usually `["-m", "app.mcp_server"]`. - `groups` — organize tools into named groups so profiles can reference them cleanly. +**Write `command` and `cwd` relative to the project root, as `./mcp-servers//...`.** This file is tracked in git: an absolute path would name the machine you happened to write it on, and the server would then fail to start anywhere else with `[Errno 2] No such file or directory`. Relative paths are resolved against the project root — the directory holding `mcp_servers.d/` — when the server is connected. Absolute paths still work, so an existing config is not broken; new ones should be relative. In `args` and `env`, a value is resolved the same way only when it starts with `./` or `../` (those fields also carry flags and URLs, which must be left alone). + After editing `mcp_servers.d/.json`, call `reload_tools` to connect the server and register its tools. ## 6. Testing @@ -199,7 +203,7 @@ If `test_mcp_tool` answers "MCP server '' is not connected", work the list in order rather than retrying blindly: 1. `mcp_status` — is the server listed, and as connected or disconnected? -2. Read `mcp_servers.d/.json` and check that `command` and `cwd` are absolute paths that exist. +2. Read `mcp_servers.d/.json` and check that `command` and `cwd` point at paths that exist — relative paths resolve against the project root, so a wrong prefix lands elsewhere. 3. `reload_tools`. 4. `test_mcp_tool` again. 5. Still failing — the code is at fault: back to syntax check and the `timeout` smoke test, then repeat from here. @@ -253,7 +257,8 @@ | Symptom | Cause | Fix | |---|---|---| -| `mcp_status` shows `disconnected` | Wrong `command` or `cwd` path | Double-check absolute paths | +| `mcp_status` shows `disconnected` | Wrong `command` or `cwd` path | Check the path; if it is relative it resolves against the project root | +| Server works on one machine, `[Errno 2]` on another | Absolute path in `command`/`cwd`/`env` | Rewrite it as `./mcp-servers//...` | | Traceback on startup | Syntax error or missing import | Run `python -m py_compile app/mcp_server.py` | | `test_mcp_tool` returns `is_error=True` | Tool raised an exception | Fix the tool logic; check parameter validation | | Tool schema missing descriptions | Used plain types instead of `Annotated[..., Field(...)]` | Add `Field(description=...)` to every parameter | diff --git a/manuals/spawn_agent.md b/manuals/spawn_agent.md index 5f7c412..efe419f 100644 --- a/manuals/spawn_agent.md +++ b/manuals/spawn_agent.md @@ -71,7 +71,9 @@ The result always starts with a header visible only to you (never repeat it to the user): - `[Sub-agent completed ...]` — finished normally; synthesise the findings. -- `[Sub-agent hit iteration limit ...]` — may be incomplete; note what's missing. +- `[Sub-agent timed out ...]` / `[Sub-agent hit the iteration limit ...]` / `[Sub-agent stalled while thinking ...]` / `[Sub-agent was stopped by the user ...]` / `[Sub-agent ran out of context ...]` — may be incomplete; note what's missing. + +Whichever it is, the body carries a progress report of every tool call the sub-agent made, so you always have something to work with — a short run is never a bare failure. The result is capped at ~8000 characters (head and tail kept, middle elided): a long run cannot flood your context. ## Multi-agent execution pattern @@ -98,7 +100,7 @@ The user cannot see sub-agent output — present findings yourself. -1. If "hit iteration limit" — note what is missing in your response. +1. If the header is anything other than `completed` — note what is missing in your response. 2. Synthesise key findings in your own words. 3. If result is insufficient, spawn again with a more focused task. diff --git a/mcp_servers.d/navi-3d.json b/mcp_servers.d/navi-3d.json index a441536..3f2e49a 100644 --- a/mcp_servers.d/navi-3d.json +++ b/mcp_servers.d/navi-3d.json @@ -1,13 +1,13 @@ { "transport": "stdio", - "command": "/home/ubuntu/navi-1/mcp-servers/navi-3d/.venv/bin/python", + "command": "./mcp-servers/navi-3d/.venv/bin/python", "args": [ "-m", "app.mcp_server" ], - "cwd": "/home/ubuntu/navi-1/mcp-servers/navi-3d", + "cwd": "./mcp-servers/navi-3d", "env": { - "SESSION_FILES_DIR": "/home/ubuntu/navi-1/session_files", + "SESSION_FILES_DIR": "./session_files", "NAVI_3D_MCP_TRANSPORT": "stdio" }, "groups": { diff --git a/mcp_servers.d/navi-web.json b/mcp_servers.d/navi-web.json index 6ff1592..1d166ca 100644 --- a/mcp_servers.d/navi-web.json +++ b/mcp_servers.d/navi-web.json @@ -1,11 +1,11 @@ { "transport": "stdio", - "command": "/home/ubuntu/navi-1/mcp-servers/navi-web/.venv/bin/python", + "command": "./mcp-servers/navi-web/.venv/bin/python", "args": [ "-m", "app.mcp_server" ], - "cwd": "/home/ubuntu/navi-1/mcp-servers/navi-web", + "cwd": "./mcp-servers/navi-web", "env": { "NAVI_WEB_MCP_TRANSPORT": "stdio" }, diff --git a/navi/api/routes/peer.py b/navi/api/routes/peer.py index 58e5611..ed83752 100644 --- a/navi/api/routes/peer.py +++ b/navi/api/routes/peer.py @@ -150,7 +150,7 @@ from navi.api.deps import get_agent agent = get_agent() - answer, completed = await asyncio.wait_for( + outcome = await asyncio.wait_for( agent.run_ephemeral( user_message=payload.question, profile_id=settings.peer_ask_profile, @@ -160,6 +160,7 @@ ), timeout=settings.peer_ask_timeout_sec + 30, # LLM ceiling + slack ) + answer, completed = outcome.text, outcome.completed except asyncio.TimeoutError: log.warning("peer.ask_timeout", request_id=request_id, peer=payload.from_name, timeout=settings.peer_ask_timeout_sec) diff --git a/navi/core/agent.py b/navi/core/agent.py index 954a9c3..9709bb4 100644 --- a/navi/core/agent.py +++ b/navi/core/agent.py @@ -53,7 +53,7 @@ from .planning import PlanningEngine from navi.tools.plan import PlanRunner from .stream_guard import _iter_stream_guarded -from .subagent_runner import SubAgentRunner +from .subagent_runner import SubAgentOutcome, SubAgentRunner from .tool_utils import build_tool_list from .events import ( AgentEvent, @@ -240,7 +240,7 @@ context_transfer: str | None = None, parent_session_id: str | None = None, timeout_seconds: float = 300.0, - ) -> tuple[str, bool]: + ) -> SubAgentOutcome: """Delegate to SubAgentRunner.""" return await self._subagent.run( user_message=user_message, diff --git a/navi/core/subagent_runner.py b/navi/core/subagent_runner.py index 65953a2..10b2f30 100644 --- a/navi/core/subagent_runner.py +++ b/navi/core/subagent_runner.py @@ -4,14 +4,17 @@ import time import uuid +from dataclasses import dataclass from datetime import datetime, timezone from typing import TYPE_CHECKING import structlog from navi.config import settings +from navi.exceptions import ContextTooLargeError from navi.llm.base import LLMBackend, Message, ToolCallRequest from navi.tools._internal.base import ToolContext, current_event_sink, current_stop_event +from navi.tools._internal.redact import redact_args from .events import AIHelperTokensUsed, SubagentComplete, ToolEvent, ToolStarted, TurnThinking from .stream_guard import _iter_stream_guarded @@ -34,6 +37,27 @@ _SUBAGENT_THINKING_STALL_CHARS = 12_000 +@dataclass(frozen=True) +class SubAgentOutcome: + """What one sub-agent run produced, and how it ended. + + ``status`` is the reason the loop stopped — ``ok``, ``timeout``, + ``max_iterations``, ``thinking_stall``, ``user_stop`` or + ``context_overflow``. The caller renders its result header from it, so a + timeout no longer reaches the parent labelled as an iteration limit. + ``text`` always carries the progress report, so a run that stopped early + still hands back what it managed to do. + """ + + text: str + status: str + + @property + def completed(self) -> bool: + """True only when the sub-agent produced a final answer of its own.""" + return self.status == "ok" + + class SubAgentRunner: """Runs a tool-calling sub-agent loop with timeout and thinking-stall guards.""" @@ -75,12 +99,13 @@ context_transfer: str | None = None, parent_session_id: str | None = None, timeout_seconds: float = 300.0, - ) -> tuple[str, bool]: + ) -> SubAgentOutcome: """ Run a sub-agent loop without a persistent session. - Returns (result_text, completed_normally). - completed_normally is False if the sub-agent hit the iteration limit or timed out. + Returns a :class:`SubAgentOutcome`: the progress report plus the final + answer, wrapped with the ``status`` the loop ended on. Callers decide + what to do with a partial run — the text is always populated. """ from navi.tools._internal.base import ( current_session_id as _sid_var, @@ -148,15 +173,16 @@ if briefing: sys_parts.append(f"## Task context\n\n{briefing}") if parent_session_id: + # Generic session plumbing only. Server-specific calling conventions + # (path shapes, session_id handling) belong to the MCP server that + # owns them — in its tool schemas and INSTRUCTIONS, which reach + # every agent, not just sub-agents. sys_parts.append( "[Parent session context]\n" f"Parent Session ID: {parent_session_id}\n" f"Session files directory: {settings.session_files_dir}/{parent_session_id}/\n" "For files the user should see, write to this exact session directory. " - "Do not use or invent a subagent_* directory.\n" - "When calling MCP navi-3d tools (compile_scad, render_stl, lint_scad), pass ONLY the filename " - "(e.g. 'falcon9_rocket.scad') for source_path and output_path. Do NOT include the session_id or " - "the session_files directory in those paths — the MCP server resolves them automatically." + "Do not use or invent a subagent_* directory." ) if not sys_parts: sys_parts.append(profile.system_prompt) @@ -221,22 +247,33 @@ for iteration in range(max_iterations): if stop_event and stop_event.is_set(): - report = self._build_progress_report(context, "user_stop", iteration) - return report + "\n\n" + (accumulated_text or ""), False + return await self._finish( + sink=sink, + context=context, + reason="user_stop", + status="user_stop", + iterations=iteration, + text=accumulated_text, + token_count=_turn_tokens, + tool_call_count=_sub_tool_count, + ) elapsed = time.monotonic() - _start_time if elapsed >= timeout_seconds: log.warning( "agent.subagent.timeout", elapsed=elapsed, timeout=timeout_seconds ) - if sink is not None: - await sink.put( - SubagentComplete( - token_count=_turn_tokens, tool_call_count=_sub_tool_count - ) - ) - report = self._build_progress_report(context, "timeout", iteration) - return report + "\n\n" + (accumulated_text or "[Sub-agent timed out]"), False + return await self._finish( + sink=sink, + context=context, + reason="timeout", + status="timeout", + iterations=iteration, + text=accumulated_text, + fallback="[Sub-agent timed out]", + token_count=_turn_tokens, + tool_call_count=_sub_tool_count, + ) log.debug("agent.subagent.iteration", iteration=iteration) @@ -253,7 +290,30 @@ if mcp_msg: built_ctx.append(mcp_msg) built_ctx.extend(m for m in context if m.role != "system") - self._compressor.check_context_size(built_ctx) + try: + self._compressor.check_context_size(built_ctx) + except ContextTooLargeError: + # The parent compresses its own context; a sub-agent has no + # session to compress into, so it stops here instead of + # dying — and hands back everything it had done by then. A + # raised error used to reach the parent as a bare + # "Sub-agent failed: …", discarding the whole run. + log.warning( + "agent.subagent.context_overflow", + iteration=iteration, + messages=len(built_ctx), + ) + return await self._finish( + sink=sink, + context=context, + reason="context_overflow", + status="context_overflow", + iterations=iteration, + text=accumulated_text, + fallback="[Sub-agent ran out of context before finishing]", + token_count=_turn_tokens, + tool_call_count=_sub_tool_count, + ) async for chunk in _iter_stream_guarded( llm.stream_complete( @@ -299,18 +359,28 @@ turn_tool_calls = chunk.tool_calls if stop_event and stop_event.is_set(): - return accumulated_text, False + return await self._finish( + sink=sink, + context=context, + reason="user_stop", + status="user_stop", + iterations=iteration + 1, + text=accumulated_text, + token_count=_turn_tokens, + tool_call_count=_sub_tool_count, + ) if thinking_stalled_reason: - if sink is not None: - await sink.put( - SubagentComplete( - token_count=_turn_tokens, - tool_call_count=_sub_tool_count, - ) - ) - report = self._build_progress_report(context, "thinking_stall", iteration + 1) - return report + "\n\n" + f"[{thinking_stalled_reason}]", False + return await self._finish( + sink=sink, + context=context, + reason="thinking_stall", + status="thinking_stall", + iterations=iteration + 1, + text=f"[{thinking_stalled_reason}]", + token_count=_turn_tokens, + tool_call_count=_sub_tool_count, + ) if not turn_tool_calls: log.info( @@ -318,20 +388,16 @@ iterations=iteration + 1, result_len=len(accumulated_text), ) - if sink is not None: - await sink.put( - SubagentComplete( - token_count=_turn_tokens, - tool_call_count=_sub_tool_count, - ) - ) - report = self._build_progress_report(context, "completed", iteration + 1) - result = accumulated_text or "" - if result: - result = report + "\n\n" + result - else: - result = report - return result, True + return await self._finish( + sink=sink, + context=context, + reason="completed", + status="ok", + iterations=iteration + 1, + text=accumulated_text, + token_count=_turn_tokens, + tool_call_count=_sub_tool_count, + ) if accumulated_thinking and sink is not None: log.debug( @@ -365,8 +431,15 @@ # skip remaining tools in this batch. if stop_event and stop_event.is_set(): log.info("agent.subagent.tool_batch_stopped", tool=tc.name) - report = self._build_progress_report(context, "user_stop", iteration + 1) - return report + "\n\n", False + return await self._finish( + sink=sink, + context=context, + reason="user_stop", + status="user_stop", + iterations=iteration + 1, + token_count=_turn_tokens, + tool_call_count=_sub_tool_count, + ) _sub_tool_count += 1 if sink is not None: @@ -390,7 +463,7 @@ success = False else: log.info( - "tool.execute.subagent", tool=tc.name, args=tc.arguments + "tool.execute.subagent", tool=tc.name, args=redact_args(tc.arguments) ) try: # Same background interception as the main loop: @@ -454,15 +527,17 @@ log.warning( "agent.subagent.max_iterations", max_iterations=max_iterations ) - if sink is not None: - await sink.put( - SubagentComplete( - token_count=_turn_tokens, tool_call_count=_sub_tool_count - ) - ) - report = self._build_progress_report(context, "max_iterations", max_iterations) - result = accumulated_text or "[Sub-agent reached iteration limit without a final answer]" - return report + "\n\n" + result, False + return await self._finish( + sink=sink, + context=context, + reason="max_iterations", + status="max_iterations", + iterations=max_iterations, + text=accumulated_text, + fallback="[Sub-agent reached iteration limit without a final answer]", + token_count=_turn_tokens, + tool_call_count=_sub_tool_count, + ) finally: _sid_var.set(_prev_sid) _model_var.set(_prev_model) @@ -473,6 +548,34 @@ _uinfo_var.set(_prev_uinfo) @staticmethod + async def _finish( + *, + sink, + context: list[Message], + reason: str, + status: str, + iterations: int, + text: str = "", + fallback: str = "", + token_count: int = 0, + tool_call_count: int = 0, + ) -> SubAgentOutcome: + """Emit completion metrics and build the outcome for one way out of the loop. + + Every exit routes through here so token accounting is never dropped + (the parent's context budget reads it) and so no exit returns a bare + string: the progress report always leads, and ``fallback`` supplies a + body when the sub-agent produced no text of its own. + """ + if sink is not None: + await sink.put( + SubagentComplete(token_count=token_count, tool_call_count=tool_call_count) + ) + report = SubAgentRunner._build_progress_report(context, reason, iterations) + body = text.strip() or fallback + return SubAgentOutcome(text=f"{report}\n\n{body}" if body else report, status=status) + + @staticmethod def _build_progress_report( context: list[Message], reason: str, @@ -512,7 +615,7 @@ snippet = result_msg.content.strip().replace("\n", " ")[:120] turn_lines.append(f" - {tc.name} → {status}: {snippet}{'...' if len(result_msg.content) > 120 else ''}") else: - args = str(tc.arguments).replace("\n", " ")[:80] + args = str(redact_args(tc.arguments)).replace("\n", " ")[:80] turn_lines.append(f" - {tc.name}({args}) → (no result yet)") lines.extend(turn_lines) lines.append("") diff --git a/navi/core/tool_executor.py b/navi/core/tool_executor.py index 1dc0999..47744e7 100644 --- a/navi/core/tool_executor.py +++ b/navi/core/tool_executor.py @@ -8,6 +8,7 @@ from navi.llm.base import Message, ToolCallRequest from navi.tools._internal.base import Tool, ToolResult, current_session_id +from navi.tools._internal.redact import redact_args from .tool_utils import resolve_tool as _resolve_tool @@ -149,13 +150,16 @@ if background_result is not None: log.info( "tool.backgrounded", tool=resolved_name, - args=tc.arguments, task_id=background_result.metadata.get("task_id"), + args=redact_args(tc.arguments), task_id=background_result.metadata.get("task_id"), ) result = background_result for mw in middlewares: await mw.after_execute(resolved_name, tc.arguments, result) else: - log.info("tool.execute", tool=resolved_name, requested_tool=tc.name, args=tc.arguments) + log.info( + "tool.execute", tool=resolved_name, requested_tool=tc.name, + args=redact_args(tc.arguments), + ) for mw in middlewares: await mw.before_execute(resolved_name, tc.arguments) result = await tool.execute(tc.arguments, ctx=ctx) diff --git a/navi/mcp/client.py b/navi/mcp/client.py index 718175f..ee237f2 100644 --- a/navi/mcp/client.py +++ b/navi/mcp/client.py @@ -15,7 +15,7 @@ from mcp.client.streamable_http import streamable_http_client from mcp.types import Tool -from .config import McpServerConfig +from .config import McpServerConfig, resolve_paths logger = logging.getLogger(__name__) @@ -184,7 +184,9 @@ async def open_transport() -> None: nonlocal session, instructions - cfg = self.config + # Project-relative paths from the config file become absolute here, + # so a config committed from another machine still runs on this one. + cfg = resolve_paths(self.config) try: if cfg.is_stdio: if not cfg.command: diff --git a/navi/mcp/config.py b/navi/mcp/config.py index 031711f..26785ed 100644 --- a/navi/mcp/config.py +++ b/navi/mcp/config.py @@ -120,6 +120,52 @@ return Path("mcp_servers.d") +def project_root() -> Path: + """Return the directory that relative paths in a config are written against. + + That is the project root holding ``mcp_servers.d/`` — the same anchor + ``_default_dir()`` is found from, so the config directory and the paths + inside it always move together. ``cwd`` at process start, in practice. + """ + return _default_dir().resolve().parent + + +def resolve_paths(cfg: McpServerConfig, root: Path | None = None) -> McpServerConfig: + """Return a copy of *cfg* with project-relative paths made absolute. + + ``mcp_servers.d/*.json`` is tracked in git, and ``create_mcp_server`` used to + write the scaffolding machine's absolute paths into it — so the server ran + only on the machine that created it. A config now spells its paths relative + to the project root (``./mcp-servers//...``) and they are resolved here, + at connect time, which keeps ``load_mcp_servers`` / ``save_mcp_servers`` a + faithful round-trip of the file. + + Absolute paths pass through untouched, so existing configs keep working. + ``command`` and ``cwd`` are always paths and are resolved whenever they are + relative; a value in ``args`` or ``env`` is resolved only when it starts with + ``./`` or ``../``, since those fields also carry flags and URLs that merely + look path-like. + """ + root = project_root() if root is None else Path(root) + + def _absolute(value: str) -> str: + path = Path(value).expanduser() + return str(path if path.is_absolute() else root / path) + + def _marked(value: str) -> str: + return _absolute(value) if value.startswith(("./", "../")) else value + + resolved = cfg.model_copy(deep=True) + if resolved.command: + resolved.command = _absolute(resolved.command) + if resolved.cwd: + resolved.cwd = _absolute(resolved.cwd) + resolved.args = [_marked(arg) for arg in resolved.args] + if resolved.env: + resolved.env = {key: _marked(value) for key, value in resolved.env.items()} + return resolved + + def _default_legacy_file() -> Path: """Return the legacy monolithic config file path.""" return Path("mcp_servers.json") diff --git a/navi/profiles/developer/system_prompt.txt b/navi/profiles/developer/system_prompt.txt index 458a7ee..c6bf388 100644 --- a/navi/profiles/developer/system_prompt.txt +++ b/navi/profiles/developer/system_prompt.txt @@ -55,8 +55,8 @@ What the manual does not spell out, and what each costs an iteration when missed: - **`reload_tools` before the first `test_mcp_tool`.** A freshly registered server is not connected yet, so the test fails with "not connected" and the iteration is wasted. `reload_tools` is also the step that ends an edit to server code or to its config — nothing else picks those up. -- **Smoke-test startup under `timeout`, and read the exit code.** `124` means `timeout` killed a server that was still running — success. `0` means it exited on its own — a real failure, usually `main()` not called. -- **Absolute paths in `mcp_servers.d/.json`** for both `command` and `cwd`; they are passed to the process launcher as-is. +- **Smoke-test startup under `timeout`, and read the exit code.** `124` means `timeout` killed a server that was still running — success. `0` means it exited on its own — a real failure, usually `main()` not called, though a closed stdin looks the same and the manual says how to tell them apart. +- **Project-relative paths in `mcp_servers.d/.json`** for both `command` and `cwd` (`./mcp-servers//...`). The file is tracked in git, so an absolute path binds the server to the machine that wrote it; relative paths are resolved against the project root at connect time. Absolute paths still work, but write relative ones. - **`mcp_status` is discovery only** — it tells you the server is connected and how many tools it exposes. It never proves a tool works; only `test_mcp_tool` does. - **Test every tool you add, one call each**, and never report success without the output in hand. diff --git a/navi/tools/_internal/logging_middleware.py b/navi/tools/_internal/logging_middleware.py index 5b2cce5..2a132f5 100644 --- a/navi/tools/_internal/logging_middleware.py +++ b/navi/tools/_internal/logging_middleware.py @@ -4,6 +4,7 @@ from navi.tools._internal.middleware import ToolMiddleware from navi.tools._internal.base import ToolResult +from navi.tools._internal.redact import redact_args log = structlog.get_logger() @@ -12,7 +13,7 @@ """Logs every tool execution with duration and result summary.""" async def before_execute(self, tool_name: str, params: dict) -> None: - log.debug("middleware.tool.before", tool=tool_name, args=params) + log.debug("middleware.tool.before", tool=tool_name, args=redact_args(params)) async def after_execute(self, tool_name: str, params: dict, result: ToolResult) -> None: log.info( diff --git a/navi/tools/_internal/redact.py b/navi/tools/_internal/redact.py new file mode 100644 index 0000000..0c6284c --- /dev/null +++ b/navi/tools/_internal/redact.py @@ -0,0 +1,95 @@ +"""Redaction of secret tool arguments before they reach the logs. + +An ``ssh_exec`` password and a third-party MCP tool's ``api_key`` are the same +problem, so the rule is keyed on the *parameter name* and applied wherever +arguments are logged — the main loop, its background path, the sub-agent runner +and the logging middleware. Keying on names rather than on a per-tool +declaration means a new tool is covered the day it is written, and a tool that +forgot to declare nothing stays exposed. + +A copy is returned, so the arguments handed to the tool and shipped to the +client are untouched: this is about what the log keeps, not about what the tool +needs. + +Known gap: a secret embedded in a free-form string — ``terminal``'s ``command``, +``code_exec``'s ``code``, ``test_mcp_tool``'s ``arguments`` — is invisible to any +name-based rule and is logged as written. Nothing here can see inside a command +line. The only cure for that is not putting the secret in one. +""" + +from __future__ import annotations + +from typing import Any + +# Matched against the whole key, case-insensitively, with '-' read as '_'. +# Masking a harmless field costs a little debugging detail; missing a real +# credential costs the credential, so the balance leans towards masking. +SENSITIVE_KEYS = frozenset( + { + "password", + "passwd", + "pwd", + "secret", + "client_secret", + "secret_key", + "token", + "access_token", + "refresh_token", + "id_token", + "auth_token", + "api_key", + "apikey", + "private_key", + "passphrase", + "authorization", + "credential", + "credentials", + } +) + +# Qualified names (`db_password`, `user_token`) are as common as bare ones. Only +# unambiguous tails belong here: `max_tokens` and `token_count` must stay +# readable, so `token` alone is not a suffix. +SENSITIVE_SUFFIXES = ( + "_password", + "_passwd", + "_secret", + "_token", + "_api_key", + "_apikey", + "_private_key", + "_passphrase", +) + +REDACTED = "***" + +# Arguments nest shallowly in practice (a dict, a list of dicts). The cap is a +# guard against a pathological self-referencing structure, not a real limit. +_MAX_DEPTH = 6 + + +def is_sensitive_key(key: object) -> bool: + """Whether a parameter name holds a credential.""" + if not isinstance(key, str): + return False + normalized = key.strip().lower().replace("-", "_") + return normalized in SENSITIVE_KEYS or normalized.endswith(SENSITIVE_SUFFIXES) + + +def redact_args(args: Any, _depth: int = 0) -> Any: + """Return a copy of *args* with every sensitive value replaced by ``***``. + + Recurses through dicts and lists so a password nested in a tool's + ``arguments`` payload is masked too. Keys are kept: the name of a secret is + not itself secret, and losing it makes the log unreadable. + """ + if _depth >= _MAX_DEPTH: + return args + if isinstance(args, dict): + return { + key: REDACTED if is_sensitive_key(key) else redact_args(value, _depth + 1) + for key, value in args.items() + } + if isinstance(args, (list, tuple)): + return [redact_args(item, _depth + 1) for item in args] + return args diff --git a/navi/tools/create_mcp_server.py b/navi/tools/create_mcp_server.py index a151c74..66997e5 100644 --- a/navi/tools/create_mcp_server.py +++ b/navi/tools/create_mcp_server.py @@ -54,10 +54,13 @@ Create a file `mcp_servers.d/{name}.json` with the server config: - transport: stdio -- command: absolute path to `.venv/bin/python` +- command: `./mcp-servers/{name}/.venv/bin/python` (relative to the project root) - args: `["-m", "app.mcp_server"]` -- cwd: absolute path to this directory +- cwd: `./mcp-servers/{name}` (relative to the project root) - groups: map tool names to logical groups + +Paths are relative to the project root on purpose: this file is tracked in git, +and an absolute path would only work on the machine that wrote it. """ @@ -147,7 +150,6 @@ # event-loop/subprocess interaction issues under uvicorn/anyio). abs_dir = base_dir.resolve() venv_dir = abs_dir / ".venv" - python_bin = venv_dir / "bin" / "python" pip_bin = venv_dir / "bin" / "pip" def _setup() -> tuple[bool, str]: @@ -225,9 +227,12 @@ configs = load_mcp_servers() configs[name] = McpServerConfig( transport="stdio", - command=str(python_bin), + # Relative to the project root, never absolute: this config is + # tracked in git, and an absolute path here would bind the + # server to the machine that scaffolded it. + command=f"./{base_dir}/.venv/bin/python", args=["-m", "app.mcp_server"], - cwd=str(abs_dir), + cwd=f"./{base_dir}", env={"MCP_TRANSPORT": "stdio"}, groups={"default": []}, ) @@ -253,9 +258,9 @@ f"2. Create mcp_servers.d/{name}.json with:\n" f'{{\n' f' "transport": "stdio",\n' - f' "command": "{python_bin}",\n' + f' "command": "./{base_dir}/.venv/bin/python",\n' f' "args": ["-m", "app.mcp_server"],\n' - f' "cwd": "{abs_dir}",\n' + f' "cwd": "./{base_dir}",\n' f' "env": {{"MCP_TRANSPORT": "stdio"}},\n' f' "groups": {{\n' f' "default": []\n' diff --git a/navi/tools/spawn_agent.py b/navi/tools/spawn_agent.py index 1acbe7c..5f3dd36 100644 --- a/navi/tools/spawn_agent.py +++ b/navi/tools/spawn_agent.py @@ -14,6 +14,53 @@ log = structlog.get_logger() +# The parent synthesises the sub-agent's result into its own answer, so a long +# run must not flood the parent's context: Anthropic's multi-agent write-up puts +# a useful sub-agent digest at ~1–2k tokens, and this is the boundary that +# enforces it. Head *and* tail are kept — the head carries the status line and +# the early turns, the tail carries the sub-agent's final answer. +_MAX_RESULT_CHARS = 8_000 + +# Before this, every non-ok run reached the parent as "hit iteration limit". +# A timeout reported as an iteration limit is a wrong diagnosis the parent then +# repeats to the user, since it synthesises from this header. +_STATUS_LABELS = { + "timeout": "Sub-agent timed out", + "max_iterations": "Sub-agent hit the iteration limit", + "thinking_stall": "Sub-agent stalled while thinking", + "user_stop": "Sub-agent was stopped by the user", + "context_overflow": "Sub-agent ran out of context", +} + + +def _truncate_result(text: str) -> str: + """Bound the result at ``_MAX_RESULT_CHARS``, keeping both ends.""" + if len(text) <= _MAX_RESULT_CHARS: + return text + half = _MAX_RESULT_CHARS // 2 + dropped = len(text) - 2 * half + return ( + f"{text[:half]}\n\n" + f"[... {dropped} characters truncated from the middle ...]\n\n" + f"{text[-half:]}" + ) + + +def _render_result(text: str, status: str) -> str: + """Wrap the sub-agent's report in a header naming how the run actually ended.""" + if status == "ok": + header = ( + "[Sub-agent completed — USER CANNOT SEE THIS. " + "Synthesise the findings into your own response.]" + ) + else: + label = _STATUS_LABELS.get(status, "Sub-agent stopped") + header = ( + f"[{label} — result may be incomplete. USER CANNOT SEE THIS. " + "Synthesise what was found and note what is missing.]" + ) + return f"{header}\n\n{_truncate_result(text)}" + class SpawnAgentTool(Tool): name = "spawn_agent" @@ -107,59 +154,78 @@ from navi.core.agent import Agent from navi.tools.scratchpad import get_section - task = params["task"].strip() - briefing = (params.get("briefing") or "").strip() or None - custom_system_prompt = (params.get("system_prompt") or "").strip() or None - max_iterations = int(params.get("max_iterations") or 40) - inherit_system_prompt = bool(params.get("inherit_system_prompt", False)) - - # task → user message (what to do) - # briefing → system prompt (context, credentials, instructions) - user_message = task - - # Resolve profile: explicit override → parent session's profile → first available - # Support both 'profile_id' and 'profile' as parameter names since models sometimes - # use 'profile' when the description says "Profile to use". - profile_id = (params.get("profile_id") or params.get("profile") or "").strip() - if not profile_id: - profile_id = await self._resolve_parent_profile(ctx) + # Everything that can fail lives inside the try: a bad-but-schema-plausible + # argument used to raise out of the tool and reach the parent as a generic + # tool_exception warning instead of a clean error result the model can read. try: - selected_profile = self._profile_registry.get(profile_id) - except ProfileNotFound: - available = ", ".join(p.id for p in self._profile_registry.all()) - return ToolResult( - success=False, - output=( - f"Unknown sub-agent profile_id: {profile_id!r}. " - f"Available profiles: {available or '(none)'}." - ), - error=f"unknown_profile:{profile_id}", + task = (params.get("task") or "").strip() + if not task: + return ToolResult( + success=False, + output="spawn_agent requires a non-empty 'task'.", + error="missing_task", + ) + briefing = (params.get("briefing") or "").strip() or None + custom_system_prompt = (params.get("system_prompt") or "").strip() or None + try: + max_iterations = int(params.get("max_iterations") or 40) + except (TypeError, ValueError): + return ToolResult( + success=False, + output=( + "max_iterations must be an integer, got " + f"{params.get('max_iterations')!r}." + ), + error="invalid_max_iterations", + ) + inherit_system_prompt = bool(params.get("inherit_system_prompt", False)) + + # task → user message (what to do) + # briefing → system prompt (context, credentials, instructions) + user_message = task + + # Resolve profile: explicit override → parent session's profile → first available + # Support both 'profile_id' and 'profile' as parameter names since models sometimes + # use 'profile' when the description says "Profile to use". + profile_id = (params.get("profile_id") or params.get("profile") or "").strip() + if not profile_id: + profile_id = await self._resolve_parent_profile(ctx) + try: + selected_profile = self._profile_registry.get(profile_id) + except ProfileNotFound: + available = ", ".join(p.id for p in self._profile_registry.all()) + return ToolResult( + success=False, + output=( + f"Unknown sub-agent profile_id: {profile_id!r}. " + f"Available profiles: {available or '(none)'}." + ), + error=f"unknown_profile:{profile_id}", + ) + + # Read parent scratchpad context_transfer section and pass it to the sub-agent. + parent_sid = ctx.session_id if ctx else current_session_id.get() + context_transfer = (await get_section(parent_sid, "context_transfer")) if parent_sid else "" + + scope = selected_profile.get_subagent_tools() + + log.info("spawn_agent.start", profile_id=profile_id, max_iterations=max_iterations, + task_preview=task[:80], has_briefing=bool(briefing), + has_context_transfer=bool(context_transfer), + has_system_prompt=bool(custom_system_prompt), + subagent_tools=len(scope.native) + sum(len(v) for v in scope.mcp.values())) + + agent = Agent( + session_store=None, # ephemeral — no DB access + profile_registry=self._profile_registry, + tool_registry=self._tool_registry, + backend_registry=self._backend_registry, + workers=[], # no post-response workers for sub-agents + memory_store=self._memory_store, + mcp_manager=self._mcp_manager, ) - # Read parent scratchpad context_transfer section and pass it to the sub-agent. - parent_sid = ctx.session_id if ctx else current_session_id.get() - context_transfer = (await get_section(parent_sid, "context_transfer")) if parent_sid else "" - - scope = selected_profile.get_subagent_tools() - - log.info("spawn_agent.start", profile_id=profile_id, max_iterations=max_iterations, - task_preview=task[:80], has_briefing=bool(briefing), - has_context_transfer=bool(context_transfer), - has_system_prompt=bool(custom_system_prompt), - subagent_tools=len(scope.native) + sum(len(v) for v in scope.mcp.values())) - - agent = Agent( - session_store=None, # ephemeral — no DB access - profile_registry=self._profile_registry, - tool_registry=self._tool_registry, - backend_registry=self._backend_registry, - workers=[], # no post-response workers for sub-agents - memory_store=self._memory_store, - mcp_manager=self._mcp_manager, - ) - - try: - result_text, completed = await agent.run_ephemeral( + outcome = await agent.run_ephemeral( user_message=user_message, profile_id=profile_id, max_iterations=max_iterations, @@ -171,27 +237,17 @@ parent_session_id=parent_sid, timeout_seconds=300.0, ) - log.info("spawn_agent.done", profile_id=profile_id, completed=completed, - result_len=len(result_text)) - - if completed: - output = ( - "[Sub-agent completed — USER CANNOT SEE THIS. " - "Synthesise the findings into your own response.]\n\n" - + result_text - ) - else: - output = ( - "[Sub-agent hit iteration limit — result may be incomplete. " - "USER CANNOT SEE THIS. " - "Synthesise what was found and note what is missing.]\n\n" - + result_text - ) + log.info("spawn_agent.done", profile_id=profile_id, status=outcome.status, + result_len=len(outcome.text)) return ToolResult( - success=completed, - output=output, - metadata={"completed": completed, "result_len": len(result_text)}, + success=outcome.completed, + output=_render_result(outcome.text, outcome.status), + metadata={ + "completed": outcome.completed, + "status": outcome.status, + "result_len": len(outcome.text), + }, ) except Exception as e: log.error("spawn_agent.error", error=str(e), exc_info=True) diff --git a/navi/tools/todo.py b/navi/tools/todo.py index 4e02d30..09c3a8d 100644 --- a/navi/tools/todo.py +++ b/navi/tools/todo.py @@ -15,6 +15,7 @@ current_todo_session_id, current_user_id, ) +from navi.tools._internal.redact import redact_args _STATUS_ICON: dict[str, str] = { "pending": "○", @@ -176,7 +177,7 @@ if op is None: op = _infer_op(params) if op is not None: - log.info("todo.op_inferred", op=op, args=params) + log.info("todo.op_inferred", op=op, args=redact_args(params)) if op == "set": raw = params.get("tasks") or [] diff --git a/tests/unit/core/test_agent.py b/tests/unit/core/test_agent.py index 9491248..693f05a 100644 --- a/tests/unit/core/test_agent.py +++ b/tests/unit/core/test_agent.py @@ -771,10 +771,11 @@ backend = FakeLLMBackend(responses=["subagent result"]) agent._backends.register("ollama", backend) - result, ok = await agent.run_ephemeral("task", profile_id="test") - assert "subagent result" in result - assert "[Sub-agent stopped: completed]" in result - assert ok is True + outcome = await agent.run_ephemeral("task", profile_id="test") + assert "subagent result" in outcome.text + assert "[Sub-agent stopped: completed]" in outcome.text + assert outcome.completed is True + assert outcome.status == "ok" @pytest.mark.asyncio async def test_run_ephemeral_max_iterations(self, agent): @@ -786,11 +787,12 @@ ) agent._backends.register("ollama", backend) - result, ok = await agent.run_ephemeral( + outcome = await agent.run_ephemeral( "task", profile_id="test", max_iterations=1 ) - assert ok is False - assert "iteration limit" in result.lower() + assert outcome.completed is False + assert outcome.status == "max_iterations" + assert "iteration limit" in outcome.text.lower() @pytest.mark.skip(reason="run_ephemeral uses 'import time as _time' inside the function; CPython LOAD_GLOBAL caching makes module-level mock replacement unreliable in pytest-asyncio.") @pytest.mark.asyncio @@ -822,8 +824,8 @@ agent._planning.run = _mock_planning - result, ok = await agent.run_ephemeral("task", profile_id="test") - assert ok is True + outcome = await agent.run_ephemeral("task", profile_id="test") + assert outcome.completed is True # Drain sink for SubagentComplete subagent_complete = None @@ -852,9 +854,33 @@ backend.stream_complete = _thinking_only agent._backends.register("ollama", backend) - result, ok = await agent.run_ephemeral("task", profile_id="test") - assert ok is False - assert "thinking" in result.lower() or "stall" in result.lower() + outcome = await agent.run_ephemeral("task", profile_id="test") + assert outcome.completed is False + assert outcome.status == "thinking_stall" + assert "thinking" in outcome.text.lower() or "stall" in outcome.text.lower() + + @pytest.mark.asyncio + async def test_run_ephemeral_context_overflow_returns_partial(self, agent): + """An overflow stops the sub-agent with a report instead of killing it. + + ``check_context_size`` raises; used to escape as ``Sub-agent failed: …`` + and discard the whole run. + """ + from navi.exceptions import ContextTooLargeError + + backend = FakeLLMBackend(responses=["subagent result"]) + agent._backends.register("ollama", backend) + + def _raise(_ctx): + raise ContextTooLargeError("too big") + + agent._subagent._compressor.check_context_size = _raise + + outcome = await agent.run_ephemeral("task", profile_id="test") + assert outcome.completed is False + assert outcome.status == "context_overflow" + assert "[Sub-agent stopped: context_overflow]" in outcome.text + assert "ran out of context" in outcome.text class _SnapshotSessionStore(InMemorySessionStore): diff --git a/tests/unit/core/test_tool_executor.py b/tests/unit/core/test_tool_executor.py index 080ac6e..823f768 100644 --- a/tests/unit/core/test_tool_executor.py +++ b/tests/unit/core/test_tool_executor.py @@ -308,3 +308,38 @@ assert "tool.not_found" in logged assert "ssh_exec" in logged assert "developer" in logged + + +class TestSecretRedactionInLogs: + """A credential must not reach the journal through the tool.args log line. + + ``ssh_exec`` takes a ``password`` parameter, and both the main loop and the + sub-agent loop logged the whole argument dict — the prod journal held SSH + passwords in plaintext. Masking happens at the log call, so what the tool + receives and what the client is shown stay intact. + """ + + async def test_password_is_masked_in_the_log_but_not_for_the_tool(self, capfd): + tool = RecordingTool("ssh_exec", output="ok") + executor = ToolExecutor(ToolRegistry()) + + event, _msg, _image = await executor._execute_one( + ToolCallRequest( + id="1", + name="ssh_exec", + arguments={"host": "10.0.0.1", "username": "ubuntu", "password": "hunter2"}, + ), + {"ssh_exec": tool}, + ctx=_Ctx(), + ) + + logged = capfd.readouterr().out + assert "hunter2" not in logged + assert "'password': '***'" in logged + # The neighbours are still there — the log has to stay worth reading. + assert "10.0.0.1" in logged and "ubuntu" in logged + + assert tool.calls == [ + {"host": "10.0.0.1", "username": "ubuntu", "password": "hunter2"} + ] + assert event.arguments["password"] == "hunter2" diff --git a/tests/unit/mcp/test_config_paths.py b/tests/unit/mcp/test_config_paths.py new file mode 100644 index 0000000..dda94a7 --- /dev/null +++ b/tests/unit/mcp/test_config_paths.py @@ -0,0 +1,79 @@ +"""Unit tests for project-relative path resolution in MCP server configs.""" + +from pathlib import Path + +from navi.mcp.config import McpServerConfig, project_root, resolve_paths + + +class TestProjectRoot: + def test_is_the_directory_holding_the_config_dir(self, monkeypatch, tmp_path: Path): + monkeypatch.chdir(tmp_path) + assert project_root() == tmp_path + + +class TestResolvePaths: + def test_relative_command_and_cwd_become_absolute(self, tmp_path: Path): + cfg = McpServerConfig( + command="./mcp-servers/my_server/.venv/bin/python", + cwd="./mcp-servers/my_server", + ) + resolved = resolve_paths(cfg, tmp_path) + assert resolved.command == str(tmp_path / "mcp-servers/my_server/.venv/bin/python") + assert resolved.cwd == str(tmp_path / "mcp-servers/my_server") + + def test_a_relative_path_without_the_dot_slash_still_resolves(self, tmp_path: Path): + """`command`/`cwd` are always paths — no marker needed there.""" + cfg = McpServerConfig(command="mcp-servers/x/.venv/bin/python") + assert resolve_paths(cfg, tmp_path).command == str( + tmp_path / "mcp-servers/x/.venv/bin/python" + ) + + def test_absolute_paths_are_left_alone(self, tmp_path: Path): + cfg = McpServerConfig( + command="/usr/bin/python3", + cwd="/srv/somewhere", + env={"TOKEN_PATH": "/etc/navi/token"}, + ) + assert resolve_paths(cfg, tmp_path) == cfg + + def test_relative_env_and_args_resolve_on_the_marker(self, tmp_path: Path): + cfg = McpServerConfig( + command="./bin/python", + args=["-m", "app.mcp_server", "--data", "./data"], + env={"SESSION_FILES_DIR": "./session_files", "MCP_TRANSPORT": "stdio"}, + ) + resolved = resolve_paths(cfg, tmp_path) + assert resolved.args == ["-m", "app.mcp_server", "--data", str(tmp_path / "data")] + assert resolved.env == { + "SESSION_FILES_DIR": str(tmp_path / "session_files"), + "MCP_TRANSPORT": "stdio", + } + + def test_home_relative_paths_expand(self, tmp_path: Path): + cfg = McpServerConfig(command="~/bin/python") + assert resolve_paths(cfg, tmp_path).command == str(Path("~/bin/python").expanduser()) + + def test_env_values_that_are_not_paths_are_untouched(self, tmp_path: Path): + """`env` carries URLs and flags too — the `./` marker is what opts in.""" + cfg = McpServerConfig( + command="./bin/python", + env={"BASE_URL": "http://localhost:8000/mcp", "FLAG": "a/b"}, + ) + resolved = resolve_paths(cfg, tmp_path) + assert resolved.env == {"BASE_URL": "http://localhost:8000/mcp", "FLAG": "a/b"} + + def test_http_transport_is_untouched(self, tmp_path: Path): + cfg = McpServerConfig(transport="sse", url="http://127.0.0.1:8098/mcp") + assert resolve_paths(cfg, tmp_path) == cfg + + def test_does_not_mutate_the_original(self, tmp_path: Path): + """`save_mcp_servers` writes configs back to the file: a resolved copy + leaking into it would bake this machine's paths into a tracked file.""" + cfg = McpServerConfig(command="./mcp-servers/x/.venv/bin/python") + resolve_paths(cfg, tmp_path) + assert cfg.command == "./mcp-servers/x/.venv/bin/python" + + def test_default_root_is_the_project_root(self, monkeypatch, tmp_path: Path): + monkeypatch.chdir(tmp_path) + cfg = McpServerConfig(cwd="./mcp-servers/x") + assert resolve_paths(cfg).cwd == str(tmp_path / "mcp-servers/x") diff --git a/tests/unit/test_peer_routes.py b/tests/unit/test_peer_routes.py index 08d75ea..b1a5c8c 100644 --- a/tests/unit/test_peer_routes.py +++ b/tests/unit/test_peer_routes.py @@ -8,6 +8,7 @@ from navi import swarm from navi.api.routes import peer as peer_routes +from navi.core.subagent_runner import SubAgentOutcome from navi.identity import ensure_identity KEY = "secret-key-123" @@ -89,7 +90,7 @@ message=user_message, profile=profile_id, exclude=exclude_tools, briefing=briefing, timeout=timeout_seconds, ) - return "the answer from the other side", True + return SubAgentOutcome(text="the answer from the other side", status="ok") monkeypatch.setattr("navi.api.deps.get_agent", lambda: FakeAgent()) payload = {"from_name": "amber-falcon", "from_instance_id": "y" * 32, "question": "what is your load?"} diff --git a/tests/unit/tools/test_redact_args.py b/tests/unit/tools/test_redact_args.py new file mode 100644 index 0000000..70362d5 --- /dev/null +++ b/tests/unit/tools/test_redact_args.py @@ -0,0 +1,120 @@ +"""Unit tests for secret masking in tool-argument logs.""" + +import pytest + +from navi.tools._internal.redact import REDACTED, is_sensitive_key, redact_args + + +class TestIsSensitiveKey: + @pytest.mark.parametrize( + "key", + [ + "password", + "Password", + "PASSWORD", + "passwd", + "pwd", + "secret", + "client_secret", + "token", + "access_token", + "refresh_token", + "api_key", + "api-key", + "apikey", + "private_key", + "passphrase", + "authorization", + ], + ) + def test_credentials_are_sensitive(self, key: str): + assert is_sensitive_key(key) is True + + @pytest.mark.parametrize( + "key", + [ + "db_password", + "user_token", + "openai_api_key", + "ssh_private_key", + "my-passphrase", + ], + ) + def test_qualified_names_are_sensitive(self, key: str): + assert is_sensitive_key(key) is True + + @pytest.mark.parametrize( + "key", + [ + # Real ssh_exec parameter: a path, not a credential. + "key_path", + "command", + "host", + "username", + # Would be caught by a careless suffix rule, and must not be. + "max_tokens", + "token_count", + "token_limit", + "tokens", + ], + ) + def test_ordinary_parameters_stay_readable(self, key: str): + assert is_sensitive_key(key) is False + + def test_non_string_key_is_not_sensitive(self): + assert is_sensitive_key(7) is False + + +class TestRedactArgs: + def test_masks_a_password_and_keeps_everything_else(self): + args = {"command": "uptime", "host": "10.0.0.1", "password": "hunter2"} + assert redact_args(args) == { + "command": "uptime", + "host": "10.0.0.1", + "password": REDACTED, + } + + def test_masks_at_depth(self): + """A credential nested in a tool's own arguments payload (test_mcp_tool).""" + args = {"server_name": "x", "arguments": {"user": "a", "api_key": "sk-1"}} + assert redact_args(args)["arguments"] == {"user": "a", "api_key": REDACTED} + + def test_masks_inside_lists(self): + args = {"items": [{"password": "a"}, {"note": "b"}]} + assert redact_args(args) == {"items": [{"password": REDACTED}, {"note": "b"}]} + + def test_masks_each_item_of_a_list_of_dicts(self): + args = [{"token": "a"}, {"token": "b"}] + assert redact_args(args) == [{"token": REDACTED}, {"token": REDACTED}] + + def test_the_key_is_spelled_out_so_the_log_stays_useful(self): + assert redact_args({"authorization": "Bearer x"}) == {"authorization": REDACTED} + + def test_a_secret_in_a_value_under_an_ordinary_key_is_not_caught(self): + """The documented gap: only the *key* is inspected, so this stays visible. + + Same shape as a password inside ``terminal``'s ``command`` or a token in + an unlabelled header value. Nothing name-based can see it. + """ + args = {"headers": [{"name": "Authorization", "value": "Bearer sekrit"}]} + assert redact_args(args) == args + + def test_does_not_mutate_the_original(self): + """The tool and the client still get the real value — only the log is masked.""" + args = {"password": "hunter2", "nested": {"token": "t"}} + original = {"password": "hunter2", "nested": {"token": "t"}} + redact_args(args) + assert args == original + assert args["nested"]["token"] == "t" + + def test_scalars_and_none_pass_through(self): + assert redact_args(None) is None + assert redact_args("plain") == "plain" + assert redact_args(3) == 3 + + def test_a_deeply_nested_structure_terminates(self): + """The depth cap is a guard, not a feature — it just must not recurse forever.""" + node: dict = {"password": "hunter2"} + for _ in range(30): + node = {"nested": node} + redact_args(node) # must return, not blow the stack diff --git a/tests/unit/tools/test_spawn_agent.py b/tests/unit/tools/test_spawn_agent.py index b6f38d4..6157458 100644 --- a/tests/unit/tools/test_spawn_agent.py +++ b/tests/unit/tools/test_spawn_agent.py @@ -1,6 +1,7 @@ import pytest from navi.core.session import InMemorySessionStore +from navi.core.subagent_runner import SubAgentOutcome from navi.tools._internal.base import ToolContext from navi.tools.spawn_agent import SpawnAgentTool from tests.conftest_factory import ( @@ -29,7 +30,7 @@ async def fake_run_ephemeral(self, **kwargs): captured.update(kwargs) - return "developer result", True + return SubAgentOutcome(text="developer result", status="ok") monkeypatch.setattr("navi.core.agent.Agent.run_ephemeral", fake_run_ephemeral) @@ -51,7 +52,7 @@ async def fake_run_ephemeral(self, **kwargs): captured.update(kwargs) - return "secretary result", True + return SubAgentOutcome(text="secretary result", status="ok") monkeypatch.setattr("navi.core.agent.Agent.run_ephemeral", fake_run_ephemeral) @@ -84,3 +85,68 @@ assert "task_id" in background["description"] # the default remains synchronous assert "synchronous" in tool.description.lower() + + +class TestResultRendering: + """How the run ended must reach the parent, the result must stay bounded, + and a bad argument must not become an exception.""" + + @pytest.mark.anyio + @pytest.mark.parametrize( + ("status", "label"), + [ + ("timeout", "timed out"), + ("max_iterations", "iteration limit"), + ("thinking_stall", "stalled"), + ("user_stop", "stopped by the user"), + ("context_overflow", "ran out of context"), + ], + ) + async def test_partial_status_gets_its_own_header( + self, monkeypatch, spawn_tool, status, label + ): + tool, _, _ = spawn_tool + + async def fake_run_ephemeral(self, **kwargs): + return SubAgentOutcome(text=f"[Sub-agent stopped: {status}]\n\npartial", status=status) + + monkeypatch.setattr("navi.core.agent.Agent.run_ephemeral", fake_run_ephemeral) + result = await tool.execute({"task": "x"}, ctx=ToolContext()) + + assert result.success is False + assert result.metadata["status"] == status + assert label in result.output + # The bug this replaces: every short run was reported as an iteration limit. + if status != "max_iterations": + assert "iteration limit" not in result.output.lower() + + @pytest.mark.anyio + async def test_long_result_is_truncated_keeping_head_and_tail(self, monkeypatch, spawn_tool): + tool, _, _ = spawn_tool + + async def fake_run_ephemeral(self, **kwargs): + return SubAgentOutcome(text="HEAD" + "x" * 20_000 + "TAIL", status="ok") + + monkeypatch.setattr("navi.core.agent.Agent.run_ephemeral", fake_run_ephemeral) + result = await tool.execute({"task": "x"}, ctx=ToolContext()) + + assert "HEAD" in result.output + assert "TAIL" in result.output + assert "truncated from the middle" in result.output + assert len(result.output) < 20_000 + + @pytest.mark.anyio + async def test_empty_task_is_a_clean_failure(self, spawn_tool): + tool, _, _ = spawn_tool + result = await tool.execute({"task": " "}, ctx=ToolContext()) + assert result.success is False + assert result.error == "missing_task" + + @pytest.mark.anyio + async def test_bad_max_iterations_is_a_clean_failure(self, spawn_tool): + tool, _, _ = spawn_tool + result = await tool.execute( + {"task": "x", "max_iterations": "many"}, ctx=ToolContext() + ) + assert result.success is False + assert result.error == "invalid_max_iterations"