diff --git a/libs/code/deepagents_code/_tool_errors.py b/libs/code/deepagents_code/_tool_errors.py new file mode 100644 index 00000000000..6c4166627d6 --- /dev/null +++ b/libs/code/deepagents_code/_tool_errors.py @@ -0,0 +1,20 @@ +"""Exception type for tool arguments the model authored incorrectly. + +Lives in its own module so both the tools that raise it and the agent wiring +that recovers from it can import it without a circular import. +""" + +from __future__ import annotations + + +class ToolArgumentError(ValueError): + """A tool rejected arguments the model authored. + + Raise this only when the model can fix the call by rewriting its arguments. + `create_cli_agent` wires a `ToolErrorMiddleware` that turns this into an + error `ToolMessage` the model can retry from. Every other exception, plain + `ValueError` included, stays fatal. + + Subclasses `ValueError` so existing callers that catch `ValueError` around + tool argument validation keep working. + """ diff --git a/libs/code/deepagents_code/agent.py b/libs/code/deepagents_code/agent.py index f03209d49eb..84b2791783a 100644 --- a/libs/code/deepagents_code/agent.py +++ b/libs/code/deepagents_code/agent.py @@ -49,8 +49,9 @@ from langchain.agents.middleware import ( HumanInTheLoopMiddleware, InterruptOnConfig, + ToolErrorMiddleware, ) -from langchain.agents.middleware.types import AgentMiddleware +from langchain.agents.middleware.types import AgentMiddleware, ToolCallRequest from langchain.tools import ( BaseTool, ToolRuntime, # LangChain inspects this annotation for runtime injection. @@ -70,6 +71,7 @@ REPOSITORY_TOOL_NAMES, RepositoryBounds, ) +from deepagents_code._tool_errors import ToolArgumentError from deepagents_code.approval_mode import ( ApprovalMode, aread_approval_mode_from_store, @@ -177,6 +179,47 @@ def _get_harness_tool_descriptions( return dict(_harness_profile_for_model(model, None).tool_description_overrides) +# Tools that raise `ToolArgumentError` on model-authored arguments. The name +# filter is a second gate only: recovery keys off the exception type, so a +# different error from one of these tools still halts the run. +# +# `read_file` is deliberately absent. It catches its own argument errors and +# returns an error `ToolMessage` already. The only `ValueError`s that escape it +# come from backend invariants the model cannot fix, and the SDK raises those +# on purpose to stop a backend from silently skipping unshown source lines. +_TOOL_ARG_VALIDATION_TOOLS: tuple[str, ...] = ("ask_user",) + + +def _tool_arg_validation_on_error( + exc: Exception, request: ToolCallRequest +) -> str | None: + """Convert a `ToolArgumentError` into a model-correctable message. + + `ToolErrorMiddleware` re-raises `GraphBubbleUp` before it calls this, so + interrupts and parent commands never arrive here. + + Args: + exc: Exception raised during tool execution. + request: The tool call request that failed. + + Returns: + A message naming the tool and the validation detail, so the model can + fix its input and retry. `None` for any other exception, so + unexpected errors propagate and halt the run. + """ + if isinstance(exc, ToolArgumentError): + # The run no longer fails, so this is the only record that the model + # sent bad arguments. `exc_info` keeps the raise site for debugging. + logger.warning( + "Recovered tool argument error from %s: %s", + request.tool_call["name"], + exc, + exc_info=exc, + ) + return f"`{request.tool_call['name']}` failed: {exc}. Fix the input and retry." + return None + + def _inject_fs_tools_into_subagents( custom_subagents: list[SubAgent | CompiledSubAgent], *, @@ -2497,6 +2540,9 @@ def _subagent_cli_middleware( from deepagents_code.hooks.server_middleware import ServerHooksMiddleware hooks_cwd = Path(effective_cwd) if effective_cwd is not None else Path.cwd() + # No `ToolErrorMiddleware` here. `_TOOL_ARG_VALIDATION_TOOLS` covers + # `ask_user` only, and subagents never get `AskUserMiddleware`, so an + # instance on this stack could never fire. middleware.append( ServerHooksMiddleware( cwd=hooks_cwd, @@ -3070,6 +3116,20 @@ def _subagent_cli_middleware( rubric_kwargs["max_iterations"] = rubric_max_iterations agent_middleware.append(ReliableRubricMiddleware(**rubric_kwargs)) + # Turn a `ToolArgumentError` into a recoverable error `ToolMessage` instead + # of a fatal run error. Any other exception propagates and halts the run. + # + # This must stay last. `_chain_tool_call_wrappers` composes the list first + # to outermost, so anything appended after this would run inside the + # handler. `ServerHooksMiddleware` keeps a plain `ValueError` fatal on + # purpose: it means the client answered a different request. Catching that + # here would report a hook fault to the model as its own bad tool input. + agent_middleware.append( + ToolErrorMiddleware( + _tool_arg_validation_on_error, tools=list(_TOOL_ARG_VALIDATION_TOOLS) + ) + ) + # Create the agent all_subagents: list[SubAgent | CompiledSubAgent | AsyncSubAgent] = [ *custom_subagents, diff --git a/libs/code/deepagents_code/ask_user.py b/libs/code/deepagents_code/ask_user.py index 7b9f9dfc441..4f1367f3a81 100644 --- a/libs/code/deepagents_code/ask_user.py +++ b/libs/code/deepagents_code/ask_user.py @@ -37,6 +37,7 @@ format_ask_user_error_answer, format_ask_user_transcript, ) +from deepagents_code._tool_errors import ToolArgumentError logger = logging.getLogger(__name__) @@ -97,7 +98,7 @@ def _validate_choices( forbidden-substring check, and names the type in error messages. Raises: - ValueError: If any choice is malformed, blank, or ambiguous. + ToolArgumentError: If any choice is malformed, blank, or ambiguous. """ # On the tool path pydantic has already parsed `choices` into `list[Choice]`, # so the shape checks below are redundant there. They are kept — and the @@ -110,14 +111,14 @@ def _validate_choices( f"{question_type} question {question_text!r} has a choice with a " f"missing or blank 'value': {choice!r}" ) - raise ValueError(msg) + raise ToolArgumentError(msg) if question_type == "multi_select" and MULTI_SELECT_FORBIDDEN_IN_VALUE in value: msg = ( f"multi_select question {question_text!r} has a choice value " f"containing {MULTI_SELECT_FORBIDDEN_IN_VALUE!r}, which would " f"make the joined answer ambiguous: {value!r}" ) - raise ValueError(msg) + raise ToolArgumentError(msg) def _validate_questions(questions: list[Question]) -> None: @@ -127,22 +128,23 @@ def _validate_questions(questions: list[Question]) -> None: questions: Question definitions provided to the `ask_user` tool. Raises: - ValueError: If the questions list or an individual question is invalid. + ToolArgumentError: If the questions list or an individual question is + invalid. """ if not questions: msg = "ask_user requires at least one question" - raise ValueError(msg) + raise ToolArgumentError(msg) for q in questions: question_text = q.get("question") if not isinstance(question_text, str) or not question_text.strip(): msg = "ask_user questions must have non-empty 'question' text" - raise ValueError(msg) + raise ToolArgumentError(msg) question_type = q.get("type") if question_type not in QUESTION_TYPES: msg = f"unsupported ask_user question type: {question_type!r}" - raise ValueError(msg) + raise ToolArgumentError(msg) # Belt-and-braces: on the tool path `Question.required` is `strict=True`, # so pydantic has already rejected a non-boolean before this runs. This @@ -157,7 +159,7 @@ def _validate_questions(questions: list[Question]) -> None: f"ask_user question {question_text!r} has a non-boolean " f"'required': {required!r}" ) - raise ValueError(msg) + raise ToolArgumentError(msg) if question_type in CHOICE_QUESTION_TYPES: choices = q.get("choices") @@ -167,7 +169,7 @@ def _validate_questions(questions: list[Question]) -> None: f"{q.get('question')!r} requires a " f"non-empty 'choices' list" ) - raise ValueError(msg) + raise ToolArgumentError(msg) _validate_choices( choices, question_text=question_text, @@ -181,7 +183,7 @@ def _validate_questions(questions: list[Question]) -> None: msg = ( f"{question_type} question {question_text!r} must not define 'choices'" ) - raise ValueError(msg) + raise ToolArgumentError(msg) def _context_string(context: object, name: str) -> str | None: diff --git a/libs/code/tests/unit_tests/test_agent.py b/libs/code/tests/unit_tests/test_agent.py index db2ecd57fbe..0134e274cc1 100644 --- a/libs/code/tests/unit_tests/test_agent.py +++ b/libs/code/tests/unit_tests/test_agent.py @@ -2978,6 +2978,33 @@ def test_ask_user_excluded_when_disabled(self, tmp_path: Path) -> None: middleware = self._capture_middleware(tmp_path, enable_ask_user=False) assert not any(isinstance(mw, AskUserMiddleware) for mw in middleware) + def test_tool_error_middleware_is_wired_and_scoped(self, tmp_path: Path) -> None: + """The agent stack installs `ToolErrorMiddleware` scoped to `ask_user`.""" + from langchain.agents.middleware import ToolErrorMiddleware + + from deepagents_code.agent import _TOOL_ARG_VALIDATION_TOOLS + + middleware = self._capture_middleware(tmp_path, enable_ask_user=True) + error_middleware = next( + (mw for mw in middleware if isinstance(mw, ToolErrorMiddleware)), None + ) + assert error_middleware is not None + assert error_middleware._tool_filter == list(_TOOL_ARG_VALIDATION_TOOLS) + + def test_tool_error_middleware_is_last(self, tmp_path: Path) -> None: + """It must wrap only tool execution, not other middleware. + + `_chain_tool_call_wrappers` composes the list first to outermost. Any + middleware appended after this one runs inside the handler, so a + `ToolArgumentError` it raised would be misreported to the model as the + model's own bad tool input. + """ + from langchain.agents.middleware import ToolErrorMiddleware + + middleware = self._capture_middleware(tmp_path, enable_ask_user=True) + assert len(middleware) > 1 + assert isinstance(middleware[-1], ToolErrorMiddleware) + class TestLoadAsyncSubagents: def test_returns_empty_when_no_file(self, tmp_path: Path) -> None: @@ -3578,6 +3605,8 @@ def test_subagent_middleware_combines_shell_configurable_model_and_cost( must not gain `ConfigurableModelMiddleware`, which would let a runtime `/model` switch clobber the pinned model. """ + from langchain.agents.middleware import ToolErrorMiddleware + from deepagents_code.agent import ShellAllowListMiddleware from deepagents_code.configurable_model import ConfigurableModelMiddleware from deepagents_code.cost_tracking import CostTrackingMiddleware @@ -3644,7 +3673,19 @@ def test_subagent_middleware_combines_shell_configurable_model_and_cost( ShellAllowListMiddleware, ServerHooksMiddleware, ], f"Unexpected middleware on subagent {name!r}: {middleware_types}" - assert subagents_by_name[name]["middleware"][-1]._emit_stop is False + # Subagents never get `AskUserMiddleware`, and `ask_user` is the + # only tool in `_TOOL_ARG_VALIDATION_TOOLS`, so a + # `ToolErrorMiddleware` here could never fire. + assert not any( + isinstance(mw, ToolErrorMiddleware) + for mw in subagents_by_name[name]["middleware"] + ) + hooks = next( + mw + for mw in subagents_by_name[name]["middleware"] + if isinstance(mw, ServerHooksMiddleware) + ) + assert hooks._emit_stop is False # Nested spend is priced once by the main agent, so a subagent's # instance must not also write the shared cost channel. assert all( diff --git a/libs/code/tests/unit_tests/test_ask_user_middleware.py b/libs/code/tests/unit_tests/test_ask_user_middleware.py index d96c3dec4e5..939094de303 100644 --- a/libs/code/tests/unit_tests/test_ask_user_middleware.py +++ b/libs/code/tests/unit_tests/test_ask_user_middleware.py @@ -8,7 +8,11 @@ from unittest.mock import AsyncMock, Mock, patch import pytest +from langchain.agents.middleware import ToolErrorMiddleware +from langchain.agents.middleware.types import ToolCallRequest from langchain_core.messages import HumanMessage, SystemMessage, ToolMessage +from langgraph.errors import GraphInterrupt +from langgraph.types import Interrupt from pydantic import TypeAdapter, ValidationError from deepagents_code._ask_user_types import ( @@ -19,6 +23,11 @@ Question, _requires_choices, ) +from deepagents_code._tool_errors import ToolArgumentError +from deepagents_code.agent import ( + _TOOL_ARG_VALIDATION_TOOLS, + _tool_arg_validation_on_error, +) from deepagents_code.ask_user import ( AskUserMiddleware, _parse_answers, @@ -720,3 +729,138 @@ async def test_awrap_model_call_appends_system_prompt(self) -> None: assert system_message.content_blocks[-1]["text"] == "\n\nASK_USER_PROMPT" handler.assert_awaited_once_with(overridden_request) assert result == "ok" + + +def _make_request(tool_name: str, args: dict[str, Any]) -> ToolCallRequest: + """Build a `ToolCallRequest` carrying `tool_name` and `args`.""" + return ToolCallRequest( + tool_call={"name": tool_name, "args": args, "id": f"{tool_name}-1"}, + tool=None, + state={}, + runtime=cast("Any", None), + ) + + +class TestToolArgValidationRecovery: + """`ToolErrorMiddleware` converts `ToolArgumentError` to error messages.""" + + def _middleware(self) -> ToolErrorMiddleware: + return ToolErrorMiddleware( + _tool_arg_validation_on_error, + tools=list(_TOOL_ARG_VALIDATION_TOOLS), + ) + + def test_value_error_becomes_error_tool_message(self) -> None: + """A model-authored `ToolArgumentError` is recoverable, not fatal.""" + middleware = self._middleware() + request = _make_request("ask_user", {"questions": []}) + + def handler(_: ToolCallRequest) -> ToolMessage: + _validate_questions([]) + msg = "unreachable" + raise AssertionError(msg) + + result = middleware.wrap_tool_call(request, handler) + + assert isinstance(result, ToolMessage) + assert result.status == "error" + assert result.tool_call_id == "ask_user-1" + assert "`ask_user` failed" in str(result.content) + assert "at least one question" in str(result.content) + + def test_blank_choice_value_becomes_error_tool_message(self) -> None: + """A blank choice value names the offending field.""" + middleware = self._middleware() + questions = [ + { + "question": "Pick some", + "type": "multi_select", + "choices": [{"value": "logs"}, {"value": " "}], + } + ] + request = _make_request("ask_user", {"questions": questions}) + + def handler(_: ToolCallRequest) -> ToolMessage: + _validate_questions(cast("list[Question]", questions)) + msg = "unreachable" + raise AssertionError(msg) + + result = middleware.wrap_tool_call(request, handler) + + assert isinstance(result, ToolMessage) + assert result.status == "error" + assert "missing or blank 'value'" in str(result.content) + + def test_plain_value_error_propagates(self) -> None: + """Recovery keys off the exception type, not the tool name. + + A bare `ValueError` raised while a scoped tool runs is an internal + fault, not model-authored input. It must stay fatal. Middleware inside + this one raises that way on purpose. + """ + middleware = self._middleware() + request = _make_request("ask_user", {"questions": []}) + + def handler(_: ToolCallRequest) -> ToolMessage: + msg = "client answered a different request" + raise ValueError(msg) + + with pytest.raises(ValueError, match="client answered a different request"): + middleware.wrap_tool_call(request, handler) + + def test_non_value_error_propagates(self) -> None: + """Unexpected errors still halt the run rather than reaching the model.""" + middleware = self._middleware() + request = _make_request("ask_user", {"questions": []}) + + def handler(_: ToolCallRequest) -> ToolMessage: + msg = "unexpected internal failure" + raise RuntimeError(msg) + + with pytest.raises(RuntimeError, match="unexpected internal failure"): + middleware.wrap_tool_call(request, handler) + + def test_interrupt_propagates_unchanged(self) -> None: + """`ask_user`'s `interrupt()` control-flow signal must not be converted.""" + middleware = self._middleware() + request = _make_request("ask_user", {"questions": []}) + + def handler(_: ToolCallRequest) -> ToolMessage: + raise GraphInterrupt((Interrupt(value={"kind": "ask_user"}, id="i-1"),)) + + with pytest.raises(GraphInterrupt): + middleware.wrap_tool_call(request, handler) + + def test_out_of_scope_tool_is_not_converted(self) -> None: + """A `ToolArgumentError` outside the scope list still propagates.""" + middleware = self._middleware() + request = _make_request("some_other_tool", {}) + + def handler(_: ToolCallRequest) -> ToolMessage: + msg = "validation detail" + raise ToolArgumentError(msg) + + with pytest.raises(ToolArgumentError, match="validation detail"): + middleware.wrap_tool_call(request, handler) + + async def test_async_value_error_becomes_error_tool_message(self) -> None: + """Production runs the async path, so it needs the same recovery. + + `read_file` and friends register coroutines, so `awrap_tool_call` is + the wrapper that actually runs. It has no `aon_error`, so it falls back + to the sync handler; this pins that fallback. + """ + middleware = self._middleware() + request = _make_request("ask_user", {"questions": []}) + + # Must be async: `awrap_tool_call` awaits the handler it is given. + async def handler(_: ToolCallRequest) -> ToolMessage: # noqa: RUF029 + _validate_questions([]) + msg = "unreachable" + raise AssertionError(msg) + + result = await middleware.awrap_tool_call(request, handler) + + assert isinstance(result, ToolMessage) + assert result.status == "error" + assert "at least one question" in str(result.content) diff --git a/libs/code/tests/unit_tests/test_end_to_end.py b/libs/code/tests/unit_tests/test_end_to_end.py index 681e47d658d..798ed1864c2 100644 --- a/libs/code/tests/unit_tests/test_end_to_end.py +++ b/libs/code/tests/unit_tests/test_end_to_end.py @@ -462,3 +462,55 @@ def test_cli_agent_backend_setup(self, tmp_path: Path) -> None: assert isinstance(backend, CompositeBackend) assert isinstance(backend.default, FilesystemBackend) + + async def test_ask_user_argument_error_is_recoverable(self, tmp_path: Path) -> None: + """A malformed `ask_user` call must not abort the run. + + This is the only test that drives the composed graph. The unit tests + build `ToolErrorMiddleware` directly, so they cannot catch a regression + in how `ToolNode` surfaces the exception or where the middleware sits + in the wrapper chain. + """ + with mock_settings(tmp_path): + model = FixedGenericFakeChatModel( + messages=iter( + [ + AIMessage( + content="Let me ask.", + tool_calls=[ + { + "name": "ask_user", + # No questions: `_validate_questions` raises + # `ToolArgumentError` before `interrupt()`. + "args": {"questions": []}, + "id": "call_1", + "type": "tool_call", + } + ], + ), + AIMessage(content="Recovered."), + ] + ) + ) + + agent, _ = create_cli_agent( + model=model, + assistant_id="test-agent", + tools=[], + checkpointer=InMemorySaver(), + enable_ask_user=True, + ) + + result = await agent.ainvoke( + {"messages": [HumanMessage(content="Ask me something")]}, + {"configurable": {"thread_id": str(uuid.uuid4())}}, + ) + + tool_messages = [m for m in result["messages"] if m.type == "tool"] + assert len(tool_messages) == 1 + assert tool_messages[0].status == "error" + assert "`ask_user` failed" in str(tool_messages[0].content) + assert "at least one question" in str(tool_messages[0].content) + + # The run continued instead of halting. + assert result["messages"][-1].content == "Recovered."