diff --git a/src/plyngent/agent/tools.py b/src/plyngent/agent/tools.py index b82e6be..7a9d10e 100644 --- a/src/plyngent/agent/tools.py +++ b/src/plyngent/agent/tools.py @@ -201,6 +201,30 @@ def tool[**PS, R]( return decorator +def _missing_required_error(definition: ToolDefinition, args: Mapping[str, object]) -> str | None: + """Model-facing error when a call omits a required argument. + + The JSON schema already declares what is required, so the model gets the + exact argument names back instead of the handler's bare ``TypeError`` + (``missing 1 required positional argument``). Unknown *extra* arguments are + left to the handler: a declared schema is not proof that no other key is + accepted (MCP servers may document only part of their input). + """ + schema = definition.parameters + required = schema.get("required") + if not isinstance(required, list): + return None + missing = [name for name in cast("list[object]", required) if isinstance(name, str) and name not in args] + if not missing: + return None + properties = schema.get("properties") + accepted: list[str] = [] + if isinstance(properties, dict): + accepted = sorted(str(key) for key in cast("dict[object, object]", properties)) + hint = f"; it accepts: {', '.join(accepted)}" if accepted else "" + return f"error: `{definition.name}` is missing required argument(s): {', '.join(missing)}{hint}" + + class ToolRegistry: """Name → tool definition map with execution helpers.""" @@ -434,6 +458,11 @@ class ToolRegistry: if not isinstance(raw_args, dict): return "error: tool arguments must be a JSON object" args = {str(key): value for key, value in cast("dict[object, object]", raw_args).items()} + # Argument shape/coverage problems are answered before any confirm prompt: + # the human should never be asked about a call that cannot run. + missing_error = _missing_required_error(definition, args) + if missing_error is not None: + return missing_error async def _run() -> str: missing = self._check_state_tags(definition) diff --git a/tests/test_agent/test_tools.py b/tests/test_agent/test_tools.py index 1414572..2a19cf7 100644 --- a/tests/test_agent/test_tools.py +++ b/tests/test_agent/test_tools.py @@ -70,6 +70,46 @@ async def test_registry_handler_error() -> None: assert "failed" in result +async def test_registry_reports_missing_required_arguments() -> None: + """A missing required arg is explained with the accepted names, not a TypeError.""" + called: list[str] = [] + + @tool(register=False) + def pick(question: str, options: list[str], *, allow_custom: bool = True) -> str: + del allow_custom + called.append(question) + return options[0] + + registry = ToolRegistry([pick]) + result = await registry.execute("pick", '{"question": "Which?"}') + assert result == ( + "error: `pick` is missing required argument(s): options; it accepts: allow_custom, options, question" + ) + assert called == [] # the handler never ran + assert await registry.execute("pick", '{"question": "Which?", "options": ["a"]}') == "a" + assert called == ["Which?"] + + +async def test_registry_missing_arg_skips_confirm_hook() -> None: + """The human is not asked to confirm a call that can never run.""" + confirms: list[str] = [] + + def confirm(name: str, args: object, reason: str) -> bool: + del args, reason + confirms.append(name) + return True + + @tool(register=False) + def write(path: str, content: str) -> str: + del content + return path + + registry = ToolRegistry([write], danger=lambda name, args: "writes a file", on_confirm=confirm) + result = await registry.execute("write", '{"path": "x"}') + assert "missing required argument(s): content" in result + assert confirms == [] + + def test_tool_items() -> None: @tool(register=False) def f(x: int) -> int: diff --git a/tests/test_tools/test_chat_tools.py b/tests/test_tools/test_chat_tools.py index 9fe8ad3..c5ee950 100644 --- a/tests/test_tools/test_chat_tools.py +++ b/tests/test_tools/test_chat_tools.py @@ -333,3 +333,29 @@ def test_wait_prompt_renders_fractional_seconds() -> None: prompt = wait_module._wait_prompt(2.5, reason=None) assert prompt.startswith("Waiting for 2.5s.") assert wait_module._wait_prompt(5, reason=None).startswith("Waiting for 5s.") + + +async def test_choose_user_reports_missing_options() -> None: + """A call without the required ``options`` lists what the tool accepts.""" + backend = ScriptedBackend(["1"]) + with temporary_backend(backend): + registry = ToolRegistry([choose_user]) + out = await registry.execute("ask_user_choice", json.dumps({"question": "Pick"})) + assert out.startswith("error:") + assert "missing required argument(s): options" in out + assert "it accepts: allow_custom, default, options, question" in out + assert backend.lines == ["1"] # the human was never asked + + +async def test_form_user_reports_missing_fields() -> None: + registry = ToolRegistry([form_user]) + out = await registry.execute("ask_user_form", json.dumps({"title": "Setup"})) + assert out.startswith("error:") + assert "missing required argument(s): fields" in out + + +async def test_wait_reports_missing_duration() -> None: + registry = ToolRegistry([wait]) + out = await registry.execute("wait", "{}") + assert out.startswith("error:") + assert "missing required argument(s): duration" in out