diff --git a/docs/MCP_AND_TOOLS.md b/docs/MCP_AND_TOOLS.md index 5b616f75..737289cf 100644 --- a/docs/MCP_AND_TOOLS.md +++ b/docs/MCP_AND_TOOLS.md @@ -132,6 +132,24 @@ def book_flight() -> str: raise NotImplementedError ``` +Describe each parameter with `Annotated` so the LLM knows what value to pass. +The description is included in the tool's input schema: + +```python +from typing import Annotated + + +def book_flight( + origin: Annotated[str, "IATA code of the departure airport, e.g. TLV"], + destination: Annotated[str, "IATA code of the arrival airport"], + date: Annotated[str, "Travel date as YYYY-MM-DD"], +) -> str: + ... +``` + +`agentctl validate` warns about any tool parameter without a description (or +with a blank one). It does not fail validation yet. + Run `agentctl generate` to create tool stubs. The engine loads each tool once at graph-build time and wraps it as a LangChain `StructuredTool`. At runtime, only the agent's declared tools are bound to its LLM, and a tool-call loop runs until diff --git a/docs/cli.mdx b/docs/cli.mdx index 12b6d596..50abaf14 100644 --- a/docs/cli.mdx +++ b/docs/cli.mdx @@ -30,6 +30,8 @@ agentctl validate agents.yml Checks schema, all references (tools, resolvers, MCPs), prompt file paths, graph structure, and imports any declared hooks to verify they load. Exits non-zero on any error. +It also warns (without failing) about tool parameters that have no description for the LLM. Describe a parameter with `Annotated`; see [Implementing a tool](/docs/mcp-and-tools#implementing-a-tool). + --- ### `inspect` diff --git a/docs/mcp-and-tools.mdx b/docs/mcp-and-tools.mdx index 7c1f799d..69465c25 100644 --- a/docs/mcp-and-tools.mdx +++ b/docs/mcp-and-tools.mdx @@ -149,7 +149,21 @@ def get_order_status(order_id: str) -> dict: } ``` -The docstring becomes the tool description the LLM sees. The engine wraps the function automatically. +The engine wraps the function automatically. The tool description the LLM sees comes from the YAML `description`; the docstring is for developers and is not sent to the model. + +Describe each parameter with `Annotated` so the LLM knows what value to pass. The description is included in the tool's input schema: + +```python +from typing import Annotated + + +def get_order_status( + order_id: Annotated[str, "The order ID from the confirmation email, e.g. ORD-1042"], +) -> dict: + ... +``` + +`agentctl validate` warns about any tool parameter without a description, or with a blank one. It does not fail validation yet. Write clear descriptions and typed parameters. The LLM reads both when deciding how to call a tool. Vague descriptions lead to wrong or missed tool calls. diff --git a/src/agent_engine/core/plugin_stubs.py b/src/agent_engine/core/plugin_stubs.py index 8ca7d7ba..19b48c78 100644 --- a/src/agent_engine/core/plugin_stubs.py +++ b/src/agent_engine/core/plugin_stubs.py @@ -194,15 +194,16 @@ def _stub_verdict(path: Path, function_name: str, class_name: str | None = None) tree = ast.parse(path.read_text(encoding="utf-8")) except (SyntaxError, UnicodeDecodeError, OSError): return _Verdict.IMPLEMENTED - fn = _find_function(tree, function_name, class_name) + fn = find_function(tree, function_name, class_name) if fn is None: return _Verdict.NO_FUNCTION return _Verdict.STUB if _is_stub_body(fn) else _Verdict.IMPLEMENTED -def _find_function( +def find_function( tree: ast.Module, name: str, class_name: str | None ) -> ast.FunctionDef | ast.AsyncFunctionDef | None: + """Find a top-level function, or a method of ``class_name``, by name.""" body: list[ast.stmt] = tree.body if class_name is not None: cls = next( diff --git a/src/agent_engine/core/tool_metadata.py b/src/agent_engine/core/tool_metadata.py new file mode 100644 index 00000000..d5415e47 --- /dev/null +++ b/src/agent_engine/core/tool_metadata.py @@ -0,0 +1,90 @@ +"""Offline check that every tool parameter tells the LLM what it means. + +A tool's parameters reach the model as its input schema. Parameter types come +from the signature; descriptions come from ``Annotated`` metadata, which the +engine's ``StructuredTool.from_function`` already copies into that schema: + + def get_customer(customer_code: Annotated[str, "The unique customer code."]) -> str: + +A parameter without one leaves the model to guess what value to pass. This +module reports such parameters so ``agentctl validate`` can surface them. + +This is the first step of issue #114. Missing descriptions are reported as +warnings, not errors: existing tools (including every shipped example) have +none yet, and failing validation would break them all at once. + +Detection is purely static, like ``plugin_stubs``: tool files are parsed +(``ast``), never executed. Anything the scan cannot positively identify — a +missing or unparseable file, a function it cannot find — is skipped and left +to the existing plugin checks and runtime loaders to report. +""" + +from __future__ import annotations + +import ast +from pathlib import Path + +from agent_engine.core.plugin_stubs import find_function +from agent_engine.core.spec import AgentSpec, GraphNode, SystemSpec + + +def scan_tool_param_descriptions(spec: SystemSpec, base_dir: Path) -> list[str]: + """Return one warning per tool parameter with a missing or blank description.""" + warnings: list[str] = [] + seen: set[str] = set() + for node in _walk(spec.graph): + if not isinstance(node.node, AgentSpec): + continue + for tool in node.node.tools: + if tool.id in seen: + continue + seen.add(tool.id) + path = base_dir / "plugins" / "tools" / f"{tool.id}.py" + warnings.extend(_check_tool(path, tool.id)) + return warnings + + +def _walk(node: GraphNode) -> list[GraphNode]: + nodes = [node] + for child in node.children: + nodes.extend(_walk(child)) + return nodes + + +def _check_tool(path: Path, tool_id: str) -> list[str]: + if not path.is_file(): + return [] + try: + tree = ast.parse(path.read_text(encoding="utf-8")) + except (SyntaxError, UnicodeDecodeError, OSError): + return [] + fn = find_function(tree, tool_id, None) + if fn is None: + return [] + warnings: list[str] = [] + for arg in (*fn.args.posonlyargs, *fn.args.args, *fn.args.kwonlyargs): + description = _annotated_description(arg.annotation) + if description is None: + warnings.append(f'tool "{tool_id}": parameter "{arg.arg}" is missing a description') + elif not description.strip(): + warnings.append(f'tool "{tool_id}": parameter "{arg.arg}" has an empty description') + return warnings + + +def _annotated_description(annotation: ast.expr | None) -> str | None: + """The first string in ``Annotated[T, ...]`` metadata, or None if there is none.""" + if not isinstance(annotation, ast.Subscript) or not _is_annotated(annotation.value): + return None + if not isinstance(annotation.slice, ast.Tuple): + return None + for item in annotation.slice.elts[1:]: + if isinstance(item, ast.Constant) and isinstance(item.value, str): + return item.value + return None + + +def _is_annotated(node: ast.expr) -> bool: + """``Annotated`` or ``.Annotated`` (e.g. ``typing.Annotated``).""" + if isinstance(node, ast.Name): + return node.id == "Annotated" + return isinstance(node, ast.Attribute) and node.attr == "Annotated" diff --git a/src/agentctl/diagnostics.py b/src/agentctl/diagnostics.py index 52f73caa..217869c5 100644 --- a/src/agentctl/diagnostics.py +++ b/src/agentctl/diagnostics.py @@ -21,6 +21,7 @@ from pathlib import Path from agent_engine.core.spec import AgentSpec, MCPSpec, OrchestratorSpec, SystemSpec +from agent_engine.core.tool_metadata import scan_tool_param_descriptions from agent_engine.core.validator import SystemSpecValidator from agent_engine.engine.langgraph.graph.traversal import collect_mcp_specs, render_graph, walk from agent_engine.generate.manifest import hook_plugin_refs @@ -53,6 +54,8 @@ def load_spec(path: str) -> tuple[SystemSpec, Path]: class ValidationResult: ok: bool errors: list[str] = field(default_factory=list) + # Reported but never fail validation (``ok`` depends on ``errors`` only). + warnings: list[str] = field(default_factory=list) import_roots: list[Path] = field(default_factory=list) agents: int = 0 mcp_servers: int = 0 @@ -93,6 +96,9 @@ def validate_spec(path: str, *, register_roots: bool = True) -> ValidationResult # Local tool / resolver plugin files exist (file check only — no import). result.errors.extend(_check_plugin_files(spec, base_dir)) + # Tool parameters the LLM gets no description for (static parse — no import). + result.warnings.extend(scan_tool_param_descriptions(spec, base_dir)) + # MCP url sanity (the parser already validates tool_tags/transport). mcps = collect_mcp_specs(spec.graph) for server_id, mcp in mcps.items(): @@ -132,6 +138,7 @@ def format_validation_report(result: ValidationResult) -> str: if not result.ok: lines = ["Validation failed:"] lines.extend(f" ✗ {e}" for e in result.errors) + lines.extend(_format_warnings(result.warnings)) return "\n".join(lines) lines = [ @@ -143,10 +150,17 @@ def format_validation_report(result: ValidationResult) -> str: ] for server_id, tags in result.tool_tags.items(): lines.append(f"✓ tool_tags: {server_id} -> {','.join(tags)}") + lines.extend(_format_warnings(result.warnings)) lines.append("✓ validation passed") return "\n".join(lines) +def _format_warnings(warnings: list[str]) -> list[str]: + if not warnings: + return [] + return ["warnings:", *(f" ! {w}" for w in warnings)] + + # --------------------------------------------------------------------------- # inspect # --------------------------------------------------------------------------- diff --git a/tests/cli/test_validate_command.py b/tests/cli/test_validate_command.py index 64f912a8..9f393b1a 100644 --- a/tests/cli/test_validate_command.py +++ b/tests/cli/test_validate_command.py @@ -281,6 +281,59 @@ def test_cli_validate_exit_zero() -> None: assert "validation passed" in res.output +def _spec_with_tool(tmp_path: Path, tool_source: str) -> str: + tools = tmp_path / "plugins" / "tools" + tools.mkdir(parents=True) + (tools / "lookup.py").write_text(tool_source, encoding="utf-8") + return _write( + tmp_path, + "system: {name: t}\ntools: {lookup: {description: Look things up.}}\n" + "agents: {a: {description: d, tools: [lookup]}}\ngraph: {a: }\n", + ) + + +def test_undescribed_tool_parameter_warns_but_passes(tmp_path: Path) -> None: + spec = _spec_with_tool(tmp_path, "def lookup(query: str) -> str:\n return query\n") + + result = validate_spec(spec) + res = CliRunner().invoke(cli, ["--log-level", "WARNING", "validate", spec]) + + assert result.ok, result.errors + assert result.warnings == ['tool "lookup": parameter "query" is missing a description'] + assert res.exit_code == 0 + assert 'warnings:\n ! tool "lookup": parameter "query"' in res.output + assert "validation passed" in res.output + + +def test_described_tool_parameter_has_no_warnings(tmp_path: Path) -> None: + spec = _spec_with_tool( + tmp_path, + "from typing import Annotated\n\n" + "def lookup(query: Annotated[str, 'What to look up.']) -> str:\n return query\n", + ) + + result = validate_spec(spec) + + assert result.ok, result.errors + assert result.warnings == [] + assert "warnings:" not in CliRunner().invoke(cli, ["validate", spec]).output + + +def test_warnings_are_shown_alongside_errors(tmp_path: Path) -> None: + spec = _spec_with_tool(tmp_path, "def lookup(query: str) -> str:\n return query\n") + # Break the spec so validation fails for an unrelated reason. + Path(spec).write_text( + Path(spec).read_text(encoding="utf-8") + "plugins: {import_roots: [missing_dir]}\n", + encoding="utf-8", + ) + + res = CliRunner().invoke(cli, ["--log-level", "WARNING", "validate", spec]) + + assert res.exit_code != 0 + assert "Validation failed" in res.output + assert 'parameter "query" is missing a description' in res.output + + def test_cli_validate_exit_nonzero_on_failure(tmp_path: Path) -> None: spec = _write(tmp_path, "system: {name: t}\n") # missing graph -> invalid runner = CliRunner() diff --git a/tests/core/test_tool_metadata.py b/tests/core/test_tool_metadata.py new file mode 100644 index 00000000..48339803 --- /dev/null +++ b/tests/core/test_tool_metadata.py @@ -0,0 +1,198 @@ +"""Tests for the offline check that tool parameters carry descriptions (#114). + +The scan is static (AST) — tool files are parsed, never executed — so these +tests write real tool files into tmp_path and assert on the warnings returned. +""" + +from __future__ import annotations + +from pathlib import Path +from textwrap import dedent + +from agent_engine.core.spec import ( + AgentSpec, + BasePromptSet, + GraphNode, + ModelConfig, + OrchestratorPromptSet, + OrchestratorSpec, + SystemMeta, + SystemSpec, + ToolSpec, +) +from agent_engine.core.tool_metadata import scan_tool_param_descriptions + +_MODEL = ModelConfig(provider="fake", name="fake", temperature=None) + + +def agent(node_id: str, *tool_ids: str) -> GraphNode: + spec = AgentSpec( + id=node_id, + name=node_id, + description=f"{node_id} agent", + model=_MODEL, + prompts=BasePromptSet(), + tools=tuple(ToolSpec(t, t) for t in tool_ids), + ) + return GraphNode(node=spec) + + +def orchestrator(node_id: str, children: list[GraphNode]) -> GraphNode: + spec = OrchestratorSpec( + id=node_id, + name=node_id, + description=f"{node_id} orchestrator", + model=_MODEL, + prompts=OrchestratorPromptSet(), + ) + return GraphNode(node=spec, children=tuple(children)) + + +def system(graph: GraphNode) -> SystemSpec: + return SystemSpec(meta=SystemMeta(name="test-system"), defaults=None, graph=graph) + + +def write_tool(base: Path, tool_id: str, source: str) -> None: + path = base / "plugins" / "tools" / f"{tool_id}.py" + path.parent.mkdir(parents=True, exist_ok=True) + path.write_text(dedent(source), encoding="utf-8") + + +def scan_one(tmp_path: Path, source: str) -> list[str]: + write_tool(tmp_path, "get_customer", source) + return scan_tool_param_descriptions(system(agent("a", "get_customer")), tmp_path) + + +def test_fully_described_tool_has_no_warnings(tmp_path: Path) -> None: + warnings = scan_one( + tmp_path, + """\ + from typing import Annotated + + def get_customer( + customer_code: Annotated[str, "The unique customer code."], + include_inactive: Annotated[bool, "Whether to include inactive customers."] = False, + ) -> str: + return customer_code + """, + ) + + assert warnings == [] + + +def test_plain_parameter_is_reported_missing(tmp_path: Path) -> None: + warnings = scan_one( + tmp_path, + "def get_customer(customer_code: str) -> str:\n return customer_code\n", + ) + + assert warnings == ['tool "get_customer": parameter "customer_code" is missing a description'] + + +def test_unannotated_parameter_is_reported_missing(tmp_path: Path) -> None: + warnings = scan_one(tmp_path, "def get_customer(customer_code):\n return customer_code\n") + + assert warnings == ['tool "get_customer": parameter "customer_code" is missing a description'] + + +def test_blank_and_whitespace_descriptions_are_reported_empty(tmp_path: Path) -> None: + warnings = scan_one( + tmp_path, + """\ + from typing import Annotated + + def get_customer(code: Annotated[str, ""], region: Annotated[str, " "]) -> str: + return code + """, + ) + + assert warnings == [ + 'tool "get_customer": parameter "code" has an empty description', + 'tool "get_customer": parameter "region" has an empty description', + ] + + +def test_typing_annotated_attribute_form_is_accepted(tmp_path: Path) -> None: + warnings = scan_one( + tmp_path, + """\ + import typing + + def get_customer(customer_code: typing.Annotated[str, "The customer code."]) -> str: + return customer_code + """, + ) + + assert warnings == [] + + +def test_annotated_without_string_metadata_is_reported_missing(tmp_path: Path) -> None: + """Only string metadata counts as a description for now; ``Field(...)`` does not.""" + warnings = scan_one( + tmp_path, + """\ + from typing import Annotated + from pydantic import Field + + def get_customer(customer_code: Annotated[str, Field(min_length=1)]) -> str: + return customer_code + """, + ) + + assert warnings == ['tool "get_customer": parameter "customer_code" is missing a description'] + + +def test_every_bad_parameter_is_reported_including_keyword_only(tmp_path: Path) -> None: + warnings = scan_one( + tmp_path, + """\ + from typing import Annotated + + def get_customer( + code: str, /, region: Annotated[str, "Sales region."], limit: int, *, verbose: bool + ) -> str: + return code + """, + ) + + assert warnings == [ + 'tool "get_customer": parameter "code" is missing a description', + 'tool "get_customer": parameter "limit" is missing a description', + 'tool "get_customer": parameter "verbose" is missing a description', + ] + + +def test_star_args_and_kwargs_are_ignored(tmp_path: Path) -> None: + warnings = scan_one( + tmp_path, + "def get_customer(*args, **kwargs) -> str:\n return ''\n", + ) + + assert warnings == [] + + +def test_async_tool_is_scanned(tmp_path: Path) -> None: + warnings = scan_one( + tmp_path, + "async def get_customer(customer_code: str) -> str:\n return customer_code\n", + ) + + assert warnings == ['tool "get_customer": parameter "customer_code" is missing a description'] + + +def test_tool_shared_by_two_agents_is_reported_once(tmp_path: Path) -> None: + write_tool(tmp_path, "lookup", "def lookup(query: str) -> str:\n return query\n") + spec = system(orchestrator("root", [agent("a", "lookup"), agent("b", "lookup")])) + + warnings = scan_tool_param_descriptions(spec, tmp_path) + + assert warnings == ['tool "lookup": parameter "query" is missing a description'] + + +def test_unscannable_tools_are_skipped_without_crashing(tmp_path: Path) -> None: + """Missing files, syntax errors and missing functions are reported elsewhere.""" + write_tool(tmp_path, "broken", "def broken(:\n") + write_tool(tmp_path, "renamed", "def something_else(query: str) -> str:\n return query\n") + spec = system(orchestrator("root", [agent("a", "absent", "broken", "renamed")])) + + assert scan_tool_param_descriptions(spec, tmp_path) == []