diff --git a/CLAUDE.md b/CLAUDE.md index 1481bca..6720422 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -55,7 +55,8 @@ Ordered as registered; the earlier one wraps the later: |---|---| | `PlaneLoggingMiddleware` | structured logging, plus the tool name | | `CoerceArguments` | repairs arguments a client encoded as strings, before validation (`coercion.py`) | -| `ValidateActionArguments` | refuses arguments the chosen action does not accept, from the `ACTIONS` declaration | +| `ValidateActionArguments` | refuses a call with no `action`, and arguments the chosen action does not accept, from the `ACTIONS` declaration | + A dispatch tool's name is not its operation, so `PlaneLoggingMiddleware` adds `resource` and `action` to every `tools/call` record — start, success and error alike, where previously only success and error carried anything. For a retired name both are resolved through the alias table, since it carries no `action` of its own. @@ -67,7 +68,7 @@ A dispatch tool's name is not its operation, so `PlaneLoggingMiddleware` adds `r `resource` + `action` names one operation however it was reached, and `tool != resource` is exactly the set of calls still arriving on a retired name. `legacy.py` also keeps its per-resolution log line, which predates these fields. -Coercion runs before validation so an argument is judged by the value it repairs to. `ValidateActionArguments` closes a gap a per-tool schema cannot: every action's parameters share one schema, so an argument meant for another action validated cleanly and was then dropped, and the call answered a different question than the one asked. Only arguments carrying a value are judged, and retired names are exempt — they arrive with no `action` and under their own parameter spelling. +Coercion runs before validation so an argument is judged by the value it repairs to. `ValidateActionArguments` closes a gap a per-tool schema cannot: every action's parameters share one schema, so an argument meant for another action validated cleanly and was then dropped, and the call answered a different question than the one asked. Only arguments carrying a value are judged. It also answers a call that names no `action` at all, because the schema error for that case reports the missing parameter without reporting one permitted value — and an agent probing a tool with empty arguments to learn its interface is asking exactly the question the action list answers. Retired names are exempt from both checks: they are keyed by their own retired name, which the `ACTIONS` table does not carry, so nothing there claims them. ### Client Context (`client.py`) diff --git a/plane_mcp/middleware.py b/plane_mcp/middleware.py index 35605a3..da1a849 100644 --- a/plane_mcp/middleware.py +++ b/plane_mcp/middleware.py @@ -16,6 +16,11 @@ logger = get_logger(__name__) +def missing_action_error(tool: str, actions: Collection[str]) -> str: + """Error naming the actions `tool` offers, for a call that chose none.""" + return f"Error: {tool} requires an action. It takes: {', '.join(sorted(actions))}." + + def stray_argument_error(action: str, arguments: dict, accepted: Collection[str]) -> str | None: """Error naming the arguments `action` does not take, or None when all are valid.""" stray = sorted(n for n, value in arguments.items() if n != "action" and value and n not in accepted) @@ -26,7 +31,7 @@ def stray_argument_error(action: str, arguments: dict, accepted: Collection[str] class ValidateActionArguments(Middleware): - """Refuse arguments the chosen action has no use for, before they are dropped.""" + """Refuse a call whose action is absent, or whose arguments that action has no use for.""" def __init__(self) -> None: self._accepted = action_arguments() @@ -41,8 +46,17 @@ async def on_call_tool(self, context: MiddlewareContext, call_next): def rejection(self, tool: str, arguments: dict) -> str | None: """The message refusing this call, or None to let it through.""" by_action = self._accepted.get(tool) - action = arguments.get("action") - if by_action is None or action not in by_action: + if by_action is None: + # A retired name, or not ours at all. Either way not our business. + return None + if "action" not in arguments: + # Pydantic names the parameter but not one permitted value, so a caller + # that omitted the choice learns nothing it did not already know. + return missing_action_error(tool, by_action) + action = arguments["action"] + if action not in by_action: + # A present-but-wrong action is left alone: the Literal already reports + # the permitted set, and a second opinion here would only muddle it. return None return stray_argument_error(action, arguments, by_action[action]) diff --git a/plane_mcp/tools/project_estimate.py b/plane_mcp/tools/project_estimate.py index fb47086..a2b5ca9 100644 --- a/plane_mcp/tools/project_estimate.py +++ b/plane_mcp/tools/project_estimate.py @@ -31,7 +31,12 @@ ACTIONS = ( Action("retrieve", ("project_id",), note="a project has at most one estimate", read=True), - Action("create", ("project_id", "name"), ("type", "description", "last_used", "external_source", "external_id")), + Action( + "create", + ("project_id", "name"), + ("type", "description", "last_used", "external_source", "external_id"), + note="creates the estimate only; add its values afterwards with create_points", + ), Action("update", ("project_id",), ("name", "description", "external_source", "external_id")), Action("delete", ("project_id",), destructive=True), Action("link", ("project_id", "estimate_id"), note="makes that estimate the project's active one"), diff --git a/tests/test_argument_validation.py b/tests/test_argument_validation.py index c082a5f..1eff792 100644 --- a/tests/test_argument_validation.py +++ b/tests/test_argument_validation.py @@ -71,6 +71,30 @@ def test_action_itself_is_never_stray(rejection): assert rejection("workitem", {"action": "count"}) is None +def test_a_call_that_chose_no_action_is_told_which_actions_exist(rejection): + """The observed failure: three of one weak model's six errored calls omitted + `action`, and Pydantic's missing_argument answer names the parameter without + naming a single permitted value -- so the turn buys nothing.""" + message = rejection("project", {"project_id": "p"}) + assert message and "requires an action" in message + for action in action_arguments()["project"]: + assert action in message, f"{action} missing from the refusal" + + +def test_every_resource_names_its_actions_when_none_is_chosen(rejection): + """A resource left out would answer the one question the caller has with silence.""" + for tool, actions in action_arguments().items(): + message = rejection(tool, {}) + assert message, f"{tool} refused a call with no action without saying why" + for action in actions: + assert action in message, f"{tool} omitted {action}" + + +def test_a_call_with_no_action_on_an_unknown_tool_is_left_to_the_server(rejection): + """The missing-action check must not claim tools this server does not own.""" + assert rejection("not_a_tool", {}) is None + + def test_an_unknown_action_is_left_to_the_schema(rejection): """The Literal reports the permitted set; a second opinion here would only muddle it.""" assert rejection("workitem", {"action": "cout", "query": "x"}) is None @@ -120,6 +144,13 @@ def test_a_stray_argument_never_reaches_plane(): assert "403" not in answer +def test_a_call_with_no_action_never_reaches_plane(): + """The refusal has to replace the schema error, not arrive after a wasted call.""" + answer = _call("workitem", {"project_id": "p"}) + assert "requires an action" in answer + assert "403" not in answer + + def test_a_clean_call_is_not_blocked(): answer = _call("workitem", {"action": "count", "project_id": "p", "pql": 'priority = "urgent"'}) assert "does not take" not in answer