Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 20 additions & 0 deletions libs/code/deepagents_code/_tool_errors.py
Original file line number Diff line number Diff line change
@@ -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.
"""
62 changes: 61 additions & 1 deletion libs/code/deepagents_code/agent.py
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand All @@ -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,
Expand Down Expand Up @@ -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],
*,
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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,
Expand Down
22 changes: 12 additions & 10 deletions libs/code/deepagents_code/ask_user.py
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@
format_ask_user_error_answer,
format_ask_user_transcript,
)
from deepagents_code._tool_errors import ToolArgumentError

logger = logging.getLogger(__name__)

Expand Down Expand Up @@ -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
Expand All @@ -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:

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

shouldn't the burden be on pydantic to validate the Question type, we shouldn't need a custom error?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Expand All @@ -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
Expand All @@ -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")
Expand All @@ -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,
Expand All @@ -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:
Expand Down
43 changes: 42 additions & 1 deletion libs/code/tests/unit_tests/test_agent.py
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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(
Expand Down
Loading