diff --git a/coding-agent/src/coding_agent/server/session_manager.py b/coding-agent/src/coding_agent/server/session_manager.py index 7e78dbc9d..2658478d1 100644 --- a/coding-agent/src/coding_agent/server/session_manager.py +++ b/coding-agent/src/coding_agent/server/session_manager.py @@ -2456,6 +2456,7 @@ async def run_agent( outcome = await adapter.run_turn(prompt) if self._runtime_store is not None: turn_outcome = self._require_turn_outcome(outcome) + turn_status = self._status_from_turn_outcome(turn_outcome) session.tape_id = ctx.tape.tape_id await self._save_runtime_message_snapshot( session, @@ -2465,13 +2466,30 @@ async def run_agent( await self._finish_runtime_agent_run( session, run_id=run_id, - status=self._status_from_turn_outcome(turn_outcome), + status=turn_status, result=self._result_from_turn_outcome(turn_outcome), error=turn_outcome.error, ) + if turn_status == "failed": + session.turn_status = "failed" + reason = turn_outcome.error or turn_outcome.stop_reason.value + session.last_failure_details = f"Agent turn failed: {reason}" + else: + session.last_failure_details = None else: session.tape_id = ctx.tape.tape_id - session.last_failure_details = None + if isinstance(outcome, TurnOutcome): + turn_status = self._status_from_turn_outcome(outcome) + if turn_status == "failed": + session.turn_status = "failed" + reason = outcome.error or outcome.stop_reason.value + session.last_failure_details = ( + f"Agent turn failed: {reason}" + ) + else: + session.last_failure_details = None + else: + session.last_failure_details = None await self._persist_session_async(session) except FatalToolExecutionError as exc: if agent_run_created: diff --git a/coding-agent/src/coding_agent/tools/file_ops.py b/coding-agent/src/coding_agent/tools/file_ops.py index 3bc28d351..5a28a37bb 100644 --- a/coding-agent/src/coding_agent/tools/file_ops.py +++ b/coding-agent/src/coding_agent/tools/file_ops.py @@ -77,7 +77,10 @@ def _resolve_workspace_path(path: str) -> Path: try: resolved.relative_to(_workspace_root) except ValueError as exc: - raise ValueError(f"Path is outside workspace root: {path}") from exc + raise ValueError( + f"Path is outside workspace root: {path}. " + f"Use a path under workspace root: {_workspace_root}" + ) from exc return resolved @@ -111,13 +114,19 @@ def resolve_path(path: str) -> Path: try: resolved.relative_to(root) except ValueError as exc: - raise ValueError(f"Path is outside workspace root: {path}") from exc + raise ValueError( + f"Path is outside workspace root: {path}. " + f"Use a path under workspace root: {root}" + ) from exc return resolved @tool( name="file_read", - description="Read file contents. Returns file text or error message.", + description=( + "Read file contents under the workspace root. Returns file text or " + "error message." + ), ) def bound_file_read(path: str) -> str | dict[str, Any]: try: @@ -132,7 +141,10 @@ def bound_file_read(path: str) -> str | dict[str, Any]: @tool( name="file_write", - description="Write content to a file. Creates parent directories if needed.", + description=( + "Write content to a file under the workspace root. Creates parent " + "directories if needed." + ), ) def bound_file_write(path: str, content: str) -> str: try: @@ -143,7 +155,10 @@ def bound_file_write(path: str, content: str) -> str: except Exception as e: return f"Error writing {path}: {e}" - @tool(name="file_replace", description="Replace exact string in a file.") + @tool( + name="file_replace", + description="Replace exact string in a file under the workspace root.", + ) def bound_file_replace(path: str, old: str, new: str) -> str: try: p = resolve_path(path) @@ -158,7 +173,10 @@ def bound_file_replace(path: str, old: str, new: str) -> str: except Exception as e: return f"Error: {e}" - @tool(name="glob_files", description="Search for files matching a glob pattern.") + @tool( + name="glob_files", + description="Search under the workspace root for files matching a glob pattern.", + ) def bound_glob_files(pattern: str, directory: str = ".") -> str: try: base = resolve_path(directory) @@ -169,7 +187,10 @@ def bound_glob_files(pattern: str, directory: str = ".") -> str: except Exception as e: return f"Error: {e}" - @tool(name="grep_search", description="Search file contents for a regex pattern.") + @tool( + name="grep_search", + description="Search file contents under the workspace root for a regex pattern.", + ) def bound_grep_search( pattern: str, directory: str = ".", include: str = "" ) -> str | dict[str, Any]: @@ -204,27 +225,37 @@ def bound_grep_search( return tools -@tool(description="Read file contents. Returns file text or error message.") +@tool( + description=( + "Read file contents under the workspace root. Returns file text or error " + "message." + ) +) def file_read(path: str) -> str | dict[str, Any]: return build_file_tools(_workspace_root)[0](path) -@tool(description="Write content to a file. Creates parent directories if needed.") +@tool( + description=( + "Write content to a file under the workspace root. Creates parent " + "directories if needed." + ) +) def file_write(path: str, content: str) -> str: return build_file_tools(_workspace_root)[1](path, content) -@tool(description="Replace exact string in a file.") +@tool(description="Replace exact string in a file under the workspace root.") def file_replace(path: str, old: str, new: str) -> str: return build_file_tools(_workspace_root)[2](path, old, new) -@tool(description="Search for files matching a glob pattern.") +@tool(description="Search under the workspace root for files matching a glob pattern.") def glob_files(pattern: str, directory: str = ".") -> str: return build_file_tools(_workspace_root)[3](pattern, directory) -@tool(description="Search file contents for a regex pattern.") +@tool(description="Search file contents under the workspace root for a regex pattern.") def grep_search( pattern: str, directory: str = ".", include: str = "" ) -> str | dict[str, Any]: diff --git a/coding-agent/src/coding_agent/tools/shell.py b/coding-agent/src/coding_agent/tools/shell.py index a88cf2a3a..5a2505a4c 100644 --- a/coding-agent/src/coding_agent/tools/shell.py +++ b/coding-agent/src/coding_agent/tools/shell.py @@ -182,6 +182,26 @@ def _optional_int(value: object | None) -> int | None: return int(value) +def _positive_int(value: object, *, name: str) -> int: + if isinstance(value, bool): + raise ValueError(f"{name} must be a positive integer") + if isinstance(value, int): + parsed = value + elif isinstance(value, str): + stripped = value.strip() + if not stripped: + raise ValueError(f"{name} must be a positive integer") + try: + parsed = int(stripped) + except ValueError as exc: + raise ValueError(f"{name} must be a positive integer") from exc + else: + raise ValueError(f"{name} must be a positive integer") + if parsed <= 0: + raise ValueError(f"{name} must be a positive integer") + return parsed + + def _sandbox_request( *, args: list[str], cwd: str | None, env: dict[str, str] | None, timeout: int ) -> object: @@ -205,7 +225,8 @@ def _validated_execution_cwd( resolved_cwd.relative_to(workspace_root) except ValueError as exc: raise ValueError( - f"Working directory is outside sandbox workspace: {resolved_cwd}" + "Working directory is outside sandbox workspace: " + f"{resolved_cwd}. Use a path under workspace root: {workspace_root}" ) from exc return str(resolved_cwd) @@ -218,11 +239,18 @@ def _validate_no_path_escape(args: list[str], workspace_root: Path) -> None: candidate.relative_to(workspace_root) except ValueError as exc: raise ValueError( - f"Path is outside sandbox workspace: {candidate}" + "Path is outside sandbox workspace: " + f"{candidate}. Use a path under workspace root: {workspace_root}" ) from exc -@tool(description="Run a shell command and return stdout/stderr.") +@tool( + description=( + "Run one simple command and return stdout/stderr. Use separate bash_run " + "calls instead of &&, ||, pipes, redirects, heredocs, semicolons, or " + "backgrounding. Paths must stay under the workspace root." + ) +) def bash_run( command: str, timeout: int = 120, @@ -231,6 +259,7 @@ def bash_run( __pipeline_ctx__: object | None = None, ) -> str | dict[str, str | int]: try: + timeout_seconds = _positive_int(timeout, name="timeout") shell_config = _pipeline_shell_config(__pipeline_ctx__) mode_value = _sandbox_mode(shell_config) execution_cwd = _validated_execution_cwd( @@ -251,7 +280,9 @@ def bash_run( Path(changed_dir).relative_to(workspace_root) except ValueError as exc: raise ValueError( - f"Directory is outside sandbox workspace: {changed_dir}" + "Directory is outside sandbox workspace: " + f"{changed_dir}. Use a path under workspace root: " + f"{workspace_root}" ) from exc return f"Changed directory to {changed_dir}" @@ -273,7 +304,7 @@ def bash_run( shell=False, capture_output=True, text=True, - timeout=timeout, + timeout=timeout_seconds, cwd=execution_cwd, env=_build_env(env), ) @@ -287,7 +318,7 @@ def bash_run( args=args, cwd=execution_cwd, env=env, - timeout=timeout, + timeout=timeout_seconds, ) ) if _structured_results_enabled(): @@ -301,7 +332,7 @@ def bash_run( output += f"\nExit code: {result.returncode}" return output.strip() or "(no output)" except subprocess.TimeoutExpired: - return f"Error: command timed out after {timeout}s" + return f"Error: command timed out after {timeout_seconds}s" except Exception as e: return f"Error: {e}" diff --git a/coding-agent/tests/coding_agent/test_cli_pipeline.py b/coding-agent/tests/coding_agent/test_cli_pipeline.py index 0dec68914..7f738578e 100644 --- a/coding-agent/tests/coding_agent/test_cli_pipeline.py +++ b/coding-agent/tests/coding_agent/test_cli_pipeline.py @@ -79,9 +79,7 @@ async def test_tui_run_uses_pipeline_adapter(self): "coding_agent.__main__.create_agent", return_value=(mock_pipeline, mock_ctx), ) as p_create, - patch( - "coding_agent.__main__.PipelineAdapter", mock_adapter_cls - ) as p_adapter, + patch("coding_agent.__main__.PipelineAdapter", mock_adapter_cls), patch("coding_agent.__main__.CodingAgentTUI", mock_tui_cls), ): from coding_agent.__main__ import _run_with_tui @@ -606,16 +604,9 @@ async def fake_run_turn(msg: str) -> TurnOutcome: ) return _ok_outcome() - with patch("coding_agent.cli.repl.CodingAgentTUI") as mock_tui_cls: - mock_tui = MagicMock() - mock_tui.__enter__ = MagicMock(return_value=mock_tui) - mock_tui.__exit__ = MagicMock(return_value=False) - mock_tui.consumer = MagicMock() - mock_tui_cls.return_value = mock_tui - - with patch.object(adapter, "run_turn", side_effect=fake_run_turn): - await session._process_message("first question") - await session._process_message("second question") + with patch.object(adapter, "run_turn", side_effect=fake_run_turn): + await session._process_message("first question") + await session._process_message("second question") assert call_messages == ["first question", "second question"] assert session._pipeline_adapter is adapter @@ -697,18 +688,9 @@ async def side_effect_run_turn(msg: str) -> TurnOutcome: patch("coding_agent.cli.commands.print_pt"), patch("coding_agent.cli.commands.print_html"), ): - with patch("coding_agent.cli.repl.CodingAgentTUI") as mock_tui_cls: - mock_tui = MagicMock() - mock_tui.__enter__ = MagicMock(return_value=mock_tui) - mock_tui.__exit__ = MagicMock(return_value=False) - mock_tui.consumer = MagicMock() - mock_tui_cls.return_value = mock_tui - - with patch.object( - adapter, "run_turn", side_effect=side_effect_run_turn - ): - await session._process_message("bad input") - await session._process_message("good input") + with patch.object(adapter, "run_turn", side_effect=side_effect_run_turn): + await session._process_message("bad input") + await session._process_message("good input") assert len(turn_results) == 2 assert turn_results[0].stop_reason == StopReason.ERROR @@ -809,8 +791,8 @@ async def test_headless_pipeline_no_final_message_omits_echo(self, capsys): # Result summary line present assert "--- Result" in captured.out # No additional content line after the result summary - lines = [l for l in captured.out.strip().split("\n") if l.strip()] - result_lines = [l for l in lines if "--- Result" in l] + lines = [line for line in captured.out.strip().split("\n") if line.strip()] + result_lines = [line for line in lines if "--- Result" in line] assert len(result_lines) == 1 @@ -979,7 +961,7 @@ def test_repl_sets_ask_user_handler_on_directive_executor(self, mock_create_agen from coding_agent.cli.repl import InteractiveSession config = _make_repl_config() - session = InteractiveSession(config) + _ = InteractiveSession(config) assert mock_pipeline._directive_executor._ask_user is not None assert callable(mock_pipeline._directive_executor._ask_user) @@ -1000,7 +982,7 @@ async def test_repl_approval_prompt_approved(self, mock_create_agent, monkeypatc from coding_agent.cli.repl import InteractiveSession config = _make_repl_config() - session = InteractiveSession(config) + _ = InteractiveSession(config) assert executor._ask_user is not None @@ -1026,7 +1008,7 @@ async def test_repl_approval_prompt_denied(self, mock_create_agent, monkeypatch) from coding_agent.cli.repl import InteractiveSession config = _make_repl_config() - session = InteractiveSession(config) + _ = InteractiveSession(config) monkeypatch.setattr("builtins.input", lambda prompt="": "n") @@ -1051,7 +1033,7 @@ async def test_repl_approval_prompt_empty_defaults_no( from coding_agent.cli.repl import InteractiveSession config = _make_repl_config() - session = InteractiveSession(config) + _ = InteractiveSession(config) monkeypatch.setattr("builtins.input", lambda prompt="": "") @@ -1075,7 +1057,7 @@ async def test_repl_approval_yes_variants(self, mock_create_agent, monkeypatch): from coding_agent.cli.repl import InteractiveSession config = _make_repl_config() - session = InteractiveSession(config) + _ = InteractiveSession(config) for variant in ("y", "Y", "yes", "YES", "Yes"): monkeypatch.setattr("builtins.input", lambda prompt="", v=variant: v) diff --git a/coding-agent/tests/coding_agent/tools/test_file_ops.py b/coding-agent/tests/coding_agent/tools/test_file_ops.py index ec43c7cd0..246a5c997 100644 --- a/coding-agent/tests/coding_agent/tools/test_file_ops.py +++ b/coding-agent/tests/coding_agent/tools/test_file_ops.py @@ -1,5 +1,3 @@ -import pytest - from coding_agent.tools.file_ops import ( configure_workspace, file_read, @@ -29,14 +27,14 @@ def test_file_read_missing(self, tmp_path): def test_file_write(self, tmp_path): configure_workspace(tmp_path) f = tmp_path / "out.txt" - result = file_write(path="out.txt", content="written") + file_write(path="out.txt", content="written") assert f.read_text() == "written" def test_file_replace(self, tmp_path): configure_workspace(tmp_path) f = tmp_path / "repl.txt" f.write_text("old text here") - result = file_replace(path="repl.txt", old="old", new="new") + file_replace(path="repl.txt", old="old", new="new") assert f.read_text() == "new text here" def test_file_read_rejects_escape_outside_workspace(self, tmp_path): @@ -48,6 +46,7 @@ def test_file_read_rejects_escape_outside_workspace(self, tmp_path): assert isinstance(result, str) assert "outside workspace" in result.lower() + assert str(tmp_path) in result def test_glob_files_rejects_escape_directory(self, tmp_path): configure_workspace(tmp_path) @@ -56,3 +55,12 @@ def test_glob_files_rejects_escape_directory(self, tmp_path): assert isinstance(result, str) assert "outside workspace" in result.lower() + assert str(tmp_path) in result + + def test_file_tool_descriptions_mention_workspace_root(self, tmp_path): + configure_workspace(tmp_path) + + assert "workspace root" in file_read._tool_schema.description + assert "workspace root" in file_write._tool_schema.description + assert "workspace root" in file_replace._tool_schema.description + assert "workspace root" in glob_files._tool_schema.description diff --git a/coding-agent/tests/tools/test_shell.py b/coding-agent/tests/tools/test_shell.py index 3b1f7b04b..7f803c182 100644 --- a/coding-agent/tests/tools/test_shell.py +++ b/coding-agent/tests/tools/test_shell.py @@ -47,6 +47,17 @@ def test_timeout(self): result = bash_run(command="sleep 10", timeout=1) assert "timed out" in _as_text(result).lower() + def test_timeout_numeric_string_is_normalized(self): + result = bash_run(command="echo hello", timeout="1") # type: ignore[arg-type] + assert _as_text(result) == "hello" + + def test_invalid_timeout_string_returns_clear_error(self): + result = bash_run(command="echo hello", timeout="soon") # type: ignore[arg-type] + result_text = _as_text(result).lower() + assert "error" in result_text + assert "timeout" in result_text + assert "positive integer" in result_text + def test_empty_command(self): result = bash_run(command="") result_text = _as_text(result).lower() @@ -131,7 +142,9 @@ def test_none_sandbox_rejects_cwd_outside_workspace(self, tmp_path: Path): result = bash_run( command="pwd", cwd=str(tmp_path), __pipeline_ctx__=pipeline_ctx ) - assert "outside sandbox workspace" in _as_text(result).lower() + result_text = _as_text(result).lower() + assert "outside sandbox workspace" in result_text + assert str(workspace).lower() in result_text def test_none_sandbox_blocks_absolute_path_escape(self, tmp_path: Path): workspace = tmp_path / "workspace" @@ -158,6 +171,7 @@ def test_none_sandbox_blocks_absolute_path_escape(self, tmp_path: Path): blocked_text = _as_text(blocked).lower() assert "error" in blocked_text assert "workspace" in blocked_text + assert str(workspace).lower() in blocked_text assert "secret" not in blocked_text def test_cd_outside_workspace_is_rejected(self, tmp_path: Path): @@ -169,7 +183,15 @@ def test_cd_outside_workspace_is_rejected(self, tmp_path: Path): result = bash_run( command="cd /tmp", cwd=str(workspace), __pipeline_ctx__=pipeline_ctx ) - assert "outside sandbox workspace" in _as_text(result).lower() + result_text = _as_text(result).lower() + assert "outside sandbox workspace" in result_text + assert str(workspace).lower() in result_text + + def test_tool_description_guides_simple_workspace_safe_commands(self): + description = bash_run._tool_schema.description + assert "separate bash_run calls" in description + assert "workspace root" in description + assert "&&" in description def test_cd_within_workspace_succeeds(self, tmp_path: Path): workspace = tmp_path / "workspace" diff --git a/coding-agent/tests/ui/test_session_manager_runtime.py b/coding-agent/tests/ui/test_session_manager_runtime.py index 92a38b558..14340a742 100644 --- a/coding-agent/tests/ui/test_session_manager_runtime.py +++ b/coding-agent/tests/ui/test_session_manager_runtime.py @@ -415,6 +415,55 @@ def fake_create_agent(**kwargs): "steps_taken": 0, } assert runtime_store.updated[-1]["error"] == "model failed" + session = await manager.get_session_async(session_id) + assert session.turn_status == "failed" + assert session.last_failure_details == "Agent turn failed: model failed" + + +@pytest.mark.asyncio +async def test_run_agent_records_error_outcome_without_runtime_store() -> None: + manager = SessionManager() + session_id = await manager.create_session() + + class FakeAdapter: + def __init__(self, pipeline, ctx, consumer) -> None: + del pipeline, ctx, consumer + + async def run_turn(self, prompt: str) -> TurnOutcome: + del prompt + return TurnOutcome(stop_reason=StopReason.ERROR, error="provider failed") + + fake_pipeline = types.SimpleNamespace( + _registry=types.SimpleNamespace( + get=lambda _: types.SimpleNamespace(_instance=None) + ) + ) + + def fake_create_agent(**kwargs): + environment = kwargs["environment"] + if not isinstance(environment, LocalEnvironment): + raise TypeError("expected local environment") + return fake_pipeline, types.SimpleNamespace( + session_id=kwargs["session_id_override"], + config={}, + tape=kwargs.get("tape") or Tape(tape_id="stable-tape"), + run_context=AgentRunContext( + session_id=kwargs["session_id_override"], + run_id=kwargs["run_id_override"], + agent_id=None, + environment=environment, + ), + ) + + with pytest.MonkeyPatch.context() as mp: + mp.setattr("coding_agent.__main__.create_agent", fake_create_agent) + mp.setattr("coding_agent.ui.session_manager.PipelineAdapter", FakeAdapter) + + await manager.run_agent(session_id, "hello") + + session = await manager.get_session_async(session_id) + assert session.turn_status == "failed" + assert session.last_failure_details == "Agent turn failed: provider failed" @pytest.mark.asyncio