Skip to content
Merged
Show file tree
Hide file tree
Changes from 1 commit
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
4 changes: 2 additions & 2 deletions CLAUDE.md
Original file line number Diff line number Diff line change
Expand Up @@ -55,9 +55,9 @@ 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 |

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`)

Expand Down
20 changes: 17 additions & 3 deletions plane_mcp/middleware.py
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,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)
Expand All @@ -25,7 +30,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()
Expand All @@ -40,8 +45,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.
Comment thread
akhil-vamshi-konam marked this conversation as resolved.
return None
return stray_argument_error(action, arguments, by_action[action])

Expand Down
7 changes: 6 additions & 1 deletion plane_mcp/tools/project_estimate.py
Original file line number Diff line number Diff line change
Expand Up @@ -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"),
Expand Down
31 changes: 31 additions & 0 deletions tests/test_argument_validation.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down