diff --git a/app/mcp.py b/app/mcp.py index c8f1b63b..5108e8a7 100644 --- a/app/mcp.py +++ b/app/mcp.py @@ -119,6 +119,101 @@ "additionalProperties": False, } +MCP_GET_BALANCE_OUTPUT_SCHEMA: dict[str, Any] = { + "type": "object", + "description": "Account balance payload returned in structuredContent.", + "properties": { + "account": {"type": "string"}, + "balance_mrwk": {"type": "string"}, + "balance_microunits": {"type": "integer"}, + }, + "required": ["account", "balance_mrwk", "balance_microunits"], + "additionalProperties": False, +} + +MCP_WALLET_OUTPUT_SCHEMA: dict[str, Any] = { + "type": "object", + "description": "Registered MRWK wallet payload returned in structuredContent.", + "properties": { + "address": { + "type": "string", + "pattern": "^[mM][rR][wW][kK]1[0-9a-fA-F]{40}$", + }, + "public_key_hex": {"type": "string", "pattern": "^[0-9a-fA-F]{64}$"}, + "label": {"type": ["string", "null"]}, + "github_login": {"type": ["string", "null"]}, + "balance_mrwk": {"type": "string"}, + "nonce": {"type": "integer", "minimum": 0}, + "next_nonce": {"type": "integer", "minimum": 1}, + "created_at": {"type": "string"}, + }, + "required": [ + "address", + "public_key_hex", + "label", + "github_login", + "balance_mrwk", + "nonce", + "next_nonce", + "created_at", + ], + "additionalProperties": True, +} + +MCP_GET_LEDGER_ENTRY_OUTPUT_SCHEMA: dict[str, Any] = { + "type": "object", + "description": "Public ledger entry payload returned in structuredContent.", + "properties": { + "sequence": {"type": "integer", "minimum": 1}, + "type": {"type": "string"}, + "from": {"type": "string"}, + "to": {"type": "string"}, + "amount_mrwk": {"type": "string"}, + "reference": {"type": ["string", "null"]}, + "previous_hash": {"type": "string"}, + "entry_hash": {"type": "string"}, + "proof_hash": {"type": ["string", "null"]}, + "created_at": {"type": "string"}, + }, + "required": [ + "sequence", + "type", + "from", + "to", + "amount_mrwk", + "reference", + "previous_hash", + "entry_hash", + "proof_hash", + "created_at", + ], + "additionalProperties": False, +} + +MCP_GET_PROOF_OUTPUT_SCHEMA: dict[str, Any] = { + "type": "object", + "description": "Public proof payload returned in structuredContent.", + "properties": { + "hash": {"type": "string", "pattern": "^[0-9a-fA-F]{64}$"}, + "kind": {"type": "string"}, + "ledger_sequence": {"type": ["integer", "null"], "minimum": 1}, + "bounty_id": {"type": ["integer", "null"], "minimum": 1}, + "submission_id": {"type": ["integer", "null"], "minimum": 1}, + "created_at": {"type": "string"}, + "proof": {"type": "object", "additionalProperties": True}, + }, + "required": [ + "hash", + "kind", + "ledger_sequence", + "bounty_id", + "submission_id", + "created_at", + "proof", + ], + "additionalProperties": False, +} + MCP_TOOLS: list[dict[str, Any]] = [ { "name": "list_bounties", @@ -292,6 +387,7 @@ "required": ["account"], "additionalProperties": False, }, + "outputSchema": MCP_GET_BALANCE_OUTPUT_SCHEMA, }, { "name": "register_wallet", @@ -312,33 +408,7 @@ "required": ["public_key_hex"], "additionalProperties": False, }, - "outputSchema": { - "type": "object", - "properties": { - "address": { - "type": "string", - "pattern": "^[mM][rR][wW][kK]1[0-9a-fA-F]{40}$", - }, - "public_key_hex": {"type": "string", "pattern": "^[0-9a-fA-F]{64}$"}, - "label": {"type": ["string", "null"]}, - "github_login": {"type": ["string", "null"]}, - "balance_mrwk": {"type": "string"}, - "nonce": {"type": "integer", "minimum": 0}, - "next_nonce": {"type": "integer", "minimum": 1}, - "created_at": {"type": "string"}, - }, - "required": [ - "address", - "public_key_hex", - "label", - "github_login", - "balance_mrwk", - "nonce", - "next_nonce", - "created_at", - ], - "additionalProperties": True, - }, + "outputSchema": MCP_WALLET_OUTPUT_SCHEMA, }, { "name": "get_wallet", @@ -355,6 +425,7 @@ "required": ["address"], "additionalProperties": False, }, + "outputSchema": MCP_WALLET_OUTPUT_SCHEMA, }, { "name": "submit_wallet_transfer", @@ -375,6 +446,7 @@ "required": ["sequence"], "additionalProperties": False, }, + "outputSchema": MCP_GET_LEDGER_ENTRY_OUTPUT_SCHEMA, }, { "name": "get_proof", @@ -393,6 +465,7 @@ "required": ["hash"], "additionalProperties": False, }, + "outputSchema": MCP_GET_PROOF_OUTPUT_SCHEMA, }, { "name": "submit_work_proof", @@ -514,6 +587,7 @@ def _jsonrpc_error(response_id: Any, code: int, message: str) -> dict[str, Any]: # field-prefixed (e.g. `unknown tool`). _KNOWN_FIELDLESS_MESSAGES: dict[str, str] = { "unknown tool": "unknown tool", + "unknown argument": "unknown argument", "matches multiple bounties": "matches multiple bounties", "repo can only be used with issue_number": ("repo can only be used with issue_number"), } diff --git a/app/mcp_tools.py b/app/mcp_tools.py index 8ce6ca2c..1e2619f1 100644 --- a/app/mcp_tools.py +++ b/app/mcp_tools.py @@ -103,9 +103,9 @@ def optional_bounty_search_query_arg() -> str | None: return query def output_format_arg() -> str: - value = args.get("format", "text") - if value is None: + if "format" not in args: return "text" + value = args["format"] if not isinstance(value, str): raise ValueError("format must be a string") if contains_control_character(value): @@ -142,7 +142,7 @@ def optional_bool_arg(field: str, default: bool = False) -> bool: def require_known_fields(*allowed_fields: str) -> None: unknown_fields = set(args) - set(allowed_fields) if unknown_fields: - raise ValueError(f"unknown argument: {sorted(unknown_fields)[0]}") + raise ValueError("unknown argument") def bounty_by_issue_number(repo_selector: str | None) -> Bounty | None: issue_query = select(Bounty).where(Bounty.issue_number == positive_int_arg("issue_number")) @@ -245,6 +245,7 @@ def reject_unexpected_args(tool_name: str, allowed: set[str]) -> None: ) return json.dumps(sorted_bounties[:limit]) if name == "get_bounty": + require_known_fields("id", "bounty_id", "issue_number", "repo", "include_awards") bounty = selected_bounty("id", internal_id_aliases=("bounty_id",)) if bounty is None: return "bounty not found" @@ -253,6 +254,14 @@ def reject_unexpected_args(tool_name: str, allowed: set[str]) -> None: bounty_data["awards"] = bounty_awards_to_dict(session, bounty.id) return json.dumps(bounty_data) if name == "list_bounty_attempts": + require_known_fields( + "id", + "bounty_id", + "issue_number", + "repo", + "include_expired", + "limit", + ) bounty = selected_bounty("bounty_id", internal_id_aliases=("id",)) if bounty is None: return "bounty not found" @@ -270,6 +279,7 @@ def reject_unexpected_args(tool_name: str, allowed: set[str]) -> None: "attempts": attempt_listing["attempts"], } if name == "get_balance": + require_known_fields("account") account = normalized_account(str_arg("account")) balance_microunits = get_balance(session, account) balance_mrwk = format_mrwk(balance_microunits) @@ -290,6 +300,7 @@ def reject_unexpected_args(tool_name: str, allowed: set[str]) -> None: ) return json.dumps(wallet_to_dict(session, wallet)) if name == "get_wallet": + require_known_fields("address") wallet_row = session.get(Wallet, normalized_wallet_address(str_arg("address"))) if wallet_row is None: return "wallet not found" @@ -317,11 +328,13 @@ def reject_unexpected_args(tool_name: str, allowed: set[str]) -> None: ) return json.dumps(wallet_transfer_to_dict(transfer)) if name == "get_ledger_entry": + require_known_fields("sequence") entry = ledger_entry_to_dict(session, positive_int_arg("sequence")) if entry is None: return "ledger entry not found" return json.dumps(entry) if name == "get_proof": + require_known_fields("hash") proof = session.get(Proof, proof_hash_from_path(str_arg("hash"))) if proof is None: return "proof not found" diff --git a/docs/agent-guide.md b/docs/agent-guide.md index c98166ee..383ac5df 100644 --- a/docs/agent-guide.md +++ b/docs/agent-guide.md @@ -309,13 +309,16 @@ Tools: - `list_bounties` - `get_bounty` - `list_bounty_attempts` -- `get_balance` +- `get_balance` (`tools/list` advertises the account input schema and balance + output schema) - `register_wallet` (`tools/list` advertises the public-key input schema and the registered wallet output schema) -- `get_wallet` +- `get_wallet` (same wallet output schema as `register_wallet`) - `submit_wallet_transfer` -- `get_ledger_entry` -- `get_proof` +- `get_ledger_entry` (`tools/list` advertises the sequence input schema and + ledger-entry output schema) +- `get_proof` (`tools/list` advertises the hash input schema and proof output + schema) - `submit_work_proof` (`format: "json"` returns structuredContent; `tools/list` advertises the selector and format schema) @@ -330,7 +333,14 @@ back to text for human-readable not-found messages. `get_balance` keeps the legacy balance text and also includes `result.structuredContent.account`, `balance_mrwk`, and `balance_microunits` so -callers do not need to parse the text response. +callers do not need to parse the text response. `tools/list` advertises the +same fields in `get_balance.outputSchema.required`. + +Reusable schema/runtime conformance coverage lives in +`tests/test_mcp_schema_conformance.py`. It compares advertised `tools/list` +output schemas with representative `tools/call` structuredContent payloads and +checks that tools with `additionalProperties: false` reject unknown arguments +with the safe `error.data.message: "unknown argument"` envelope. ### Argument-validation errors diff --git a/tests/test_api_mcp.py b/tests/test_api_mcp.py index 7f0a27f0..c7fbc101 100644 --- a/tests/test_api_mcp.py +++ b/tests/test_api_mcp.py @@ -468,6 +468,8 @@ def test_mcp_tools_list_and_call(sqlite_url: str) -> None: assert balance_schema["additionalProperties"] is False assert balance_schema["properties"]["account"]["minLength"] == 1 assert "github:" in balance_schema["properties"]["account"]["description"] + balance_output_schema = balance_tool["outputSchema"] + assert balance_output_schema["required"] == ["account", "balance_mrwk", "balance_microunits"] ledger_tool = next( tool for tool in tools["result"]["tools"] if tool["name"] == "get_ledger_entry" ) @@ -475,11 +477,15 @@ def test_mcp_tools_list_and_call(sqlite_url: str) -> None: assert ledger_schema["required"] == ["sequence"] assert ledger_schema["additionalProperties"] is False assert ledger_schema["properties"]["sequence"]["minimum"] == 1 + ledger_output_schema = ledger_tool["outputSchema"] + assert "sequence" in ledger_output_schema["required"] proof_tool = next(tool for tool in tools["result"]["tools"] if tool["name"] == "get_proof") proof_schema = proof_tool["inputSchema"] assert proof_schema["required"] == ["hash"] assert proof_schema["additionalProperties"] is False assert proof_schema["properties"]["hash"]["pattern"] == "^[0-9a-fA-F]{64}$" + assert "proof" in proof_tool["outputSchema"]["required"] + assert "outputSchema" in wallet_tool balance = client.post( "/mcp", @@ -2434,6 +2440,7 @@ def test_mcp_submit_work_proof_scopes_issue_number_by_repo(sqlite_url: str) -> N ({"bounty_id": 1, "issue_number": 1}, 25), ({"format": "xml"}, 26), ({"format": 1}, 27), + ({"format": None}, 33), ({"format": "\x85json"}, 28), ({"repo": "ramimbo/mergework"}, 29), ({"bounty_id": 1, "repo": "ramimbo/mergework"}, 30), diff --git a/tests/test_mcp_schema_conformance.py b/tests/test_mcp_schema_conformance.py new file mode 100644 index 00000000..a22b8740 --- /dev/null +++ b/tests/test_mcp_schema_conformance.py @@ -0,0 +1,244 @@ +from __future__ import annotations + +import json +from typing import Any + +import pytest +from fastapi.testclient import TestClient + +from app.db import create_schema, session_scope +from app.ledger.service import ( + GENESIS_SUPPLY_MICRO, + create_bounty, + ensure_genesis, + pay_bounty, +) +from app.main import create_app +from app.mcp import MCP_TOOLS + + +def _assert_invalid_tool_arguments_envelope( + response_json: dict[str, object], + *, + request_id: object, + expected_data: object = None, +) -> None: + assert response_json["jsonrpc"] == "2.0" + assert response_json["id"] == request_id + error = response_json["error"] + assert isinstance(error, dict) + assert error["code"] == -32602 + assert error["message"] == "invalid tool arguments" + if expected_data is None: + return + if expected_data is False: + assert "data" not in error + return + assert error["data"] == expected_data + + +def _tool_by_name(name: str) -> dict[str, Any]: + return next(tool for tool in MCP_TOOLS if tool["name"] == name) + + +def _tools_with_output_schema() -> list[dict[str, Any]]: + return [tool for tool in MCP_TOOLS if "outputSchema" in tool] + + +def _assert_structured_matches_required( + payload: dict[str, Any] | list[Any], schema: dict[str, Any] +) -> None: + if schema.get("type") == "array": + assert isinstance(payload, list) + item_schema = schema["items"] + for item in payload: + assert isinstance(item, dict) + for key in item_schema.get("required", []): + assert key in item + return + assert isinstance(payload, dict) + for key in schema.get("required", []): + assert key in payload + + +@pytest.mark.parametrize("tool", _tools_with_output_schema(), ids=lambda tool: tool["name"]) +def test_mcp_tools_list_output_schema_required_fields_are_documented( + tool: dict[str, Any], +) -> None: + schema = tool["outputSchema"] + assert schema.get("type") in {"object", "array"} + if schema.get("type") == "object": + assert isinstance(schema.get("required"), list) + assert schema["required"] + + +def test_mcp_schema_conformance_balance_wallet_ledger_and_proof( + sqlite_url: str, +) -> None: + create_schema(sqlite_url) + with session_scope(sqlite_url) as session: + ensure_genesis(session) + bounty = create_bounty( + session, + repo="ramimbo/mergework", + issue_number=946, + issue_url="https://github.com/ramimbo/mergework/issues/946", + title="MCP schema conformance", + reward_mrwk="150", + acceptance="Schema conformance tests.", + ) + proof = pay_bounty( + session, + bounty_id=bounty.id, + to_account="github:alice", + submission_url="https://github.com/ramimbo/mergework/pull/946", + accepted_by="maintainer", + verifier_result={"label": "mrwk:accepted"}, + ) + ledger_sequence = proof.ledger_sequence + + client = TestClient(create_app(database_url=sqlite_url, webhook_secret="secret")) + tools = client.post("/mcp", json={"jsonrpc": "2.0", "id": 1, "method": "tools/list"}).json() + tools_by_name = {tool["name"]: tool for tool in tools["result"]["tools"]} + + balance = client.post( + "/mcp", + json={ + "jsonrpc": "2.0", + "id": 2, + "method": "tools/call", + "params": {"name": "get_balance", "arguments": {"account": "treasury:mrwk"}}, + }, + ).json()["result"] + balance_payload = balance["structuredContent"] + _assert_structured_matches_required( + balance_payload, tools_by_name["get_balance"]["outputSchema"] + ) + assert balance_payload["balance_microunits"] == GENESIS_SUPPLY_MICRO - 150_000_000 + + public_key_hex = "33" * 32 + registered = client.post( + "/mcp", + json={ + "jsonrpc": "2.0", + "id": 3, + "method": "tools/call", + "params": { + "name": "register_wallet", + "arguments": {"public_key_hex": public_key_hex, "label": "schema wallet"}, + }, + }, + ).json()["result"] + wallet_payload = registered["structuredContent"] + _assert_structured_matches_required( + wallet_payload, tools_by_name["register_wallet"]["outputSchema"] + ) + + fetched = client.post( + "/mcp", + json={ + "jsonrpc": "2.0", + "id": 4, + "method": "tools/call", + "params": {"name": "get_wallet", "arguments": {"address": wallet_payload["address"]}}, + }, + ).json()["result"] + _assert_structured_matches_required( + fetched["structuredContent"], tools_by_name["get_wallet"]["outputSchema"] + ) + + ledger = client.post( + "/mcp", + json={ + "jsonrpc": "2.0", + "id": 5, + "method": "tools/call", + "params": { + "name": "get_ledger_entry", + "arguments": {"sequence": ledger_sequence}, + }, + }, + ).json()["result"] + ledger_payload = ledger["structuredContent"] + _assert_structured_matches_required( + ledger_payload, tools_by_name["get_ledger_entry"]["outputSchema"] + ) + + proof_result = client.post( + "/mcp", + json={ + "jsonrpc": "2.0", + "id": 6, + "method": "tools/call", + "params": {"name": "get_proof", "arguments": {"hash": proof.hash}}, + }, + ).json()["result"] + proof_payload = proof_result["structuredContent"] + _assert_structured_matches_required(proof_payload, tools_by_name["get_proof"]["outputSchema"]) + + +def test_mcp_list_bounties_rejects_unknown_argument_with_safe_error_data( + sqlite_url: str, +) -> None: + create_schema(sqlite_url) + with session_scope(sqlite_url) as session: + ensure_genesis(session) + + client = TestClient(create_app(database_url=sqlite_url, webhook_secret="secret")) + response = client.post( + "/mcp", + json={ + "jsonrpc": "2.0", + "id": 7, + "method": "tools/call", + "params": { + "name": "list_bounties", + "arguments": {"status": "open", "unexpected_field": "drop-me"}, + }, + }, + ) + + payload = response.json() + _assert_invalid_tool_arguments_envelope( + payload, + request_id=7, + expected_data={ + "code": "invalid_argument", + "tool": "list_bounties", + "field": None, + "message": "unknown argument", + }, + ) + assert "drop-me" not in json.dumps(payload) + + +@pytest.mark.parametrize( + ("tool_name", "arguments"), + [ + ("get_balance", {"account": "treasury:mrwk", "extra": 1}), + ("get_wallet", {"address": "mrwk1" + ("a" * 40), "label": "ignored"}), + ("get_ledger_entry", {"sequence": 1, "offset": 0}), + ("get_proof", {"hash": "0" * 64, "kind": "ignored"}), + ], +) +def test_mcp_read_tools_reject_unknown_arguments( + sqlite_url: str, tool_name: str, arguments: dict[str, object] +) -> None: + create_schema(sqlite_url) + with session_scope(sqlite_url) as session: + ensure_genesis(session) + + client = TestClient(create_app(database_url=sqlite_url, webhook_secret="secret")) + response = client.post( + "/mcp", + json={ + "jsonrpc": "2.0", + "id": 8, + "method": "tools/call", + "params": {"name": tool_name, "arguments": arguments}, + }, + ) + + payload = response.json() + _assert_invalid_tool_arguments_envelope(payload, request_id=8) + assert payload["error"].get("data", {}).get("message") == "unknown argument"