Refs #406: Add MCP input schemas for public tools#488
Conversation
📝 WalkthroughWalkthroughThe PR adds full JSON-schema ChangesMCP Tool Input Schemas
Possibly related PRs
🚥 Pre-merge checks | ✅ 5 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 64ebaba4-9165-4954-a4d9-7d09e2d5b841
📒 Files selected for processing (2)
app/mcp.pytests/test_api_mcp.py
| "account": { | ||
| "type": "string", | ||
| "description": "Ledger account id such as github:alice or mrwk1...", | ||
| }, |
There was a problem hiding this comment.
Add non-empty constraints for required string arguments in inputSchema.
These fields are required and validated as non-empty by runtime str_arg(...), but the schemas currently allow "". That makes tool discovery metadata accept payloads that tools/call rejects.
Proposed schema tightening
"account": {
"type": "string",
+ "minLength": 1,
"description": "Ledger account id such as github:alice or mrwk1...",
},
"public_key_hex": {
"type": "string",
+ "minLength": 1,
"description": "Wallet public key in hex form.",
},
"address": {
"type": "string",
+ "minLength": 1,
"description": "Registered mrwk1 wallet address.",
},
- "from_address": {"type": "string", "description": "Sender mrwk1 wallet address."},
- "to_address": {"type": "string", "description": "Recipient mrwk1 wallet address."},
- "amount_mrwk": {"type": "string", "description": "Amount to transfer in MRWK."},
+ "from_address": {"type": "string", "minLength": 1, "description": "Sender mrwk1 wallet address."},
+ "to_address": {"type": "string", "minLength": 1, "description": "Recipient mrwk1 wallet address."},
+ "amount_mrwk": {"type": "string", "minLength": 1, "description": "Amount to transfer in MRWK."},
"signature_hex": {
"type": "string",
+ "minLength": 1,
"description": "Hex signature authorizing the transfer.",
},
"hash": {
"type": "string",
+ "minLength": 1,
"description": "Public proof hash.",
},Also applies to: 114-117, 133-136, 148-150, 156-159, 187-190
| tool_by_name = {tool["name"]: tool for tool in tools["result"]["tools"]} | ||
| assert all("inputSchema" in tool for tool in tool_by_name.values()) |
There was a problem hiding this comment.
Guard against duplicate tool names before map materialization.
tool_by_name = {tool["name"]: ...} silently overwrites duplicates. Add a uniqueness assertion so tools/list regressions fail loudly.
Suggested assertion
- tool_by_name = {tool["name"]: tool for tool in tools["result"]["tools"]}
+ tools_list = tools["result"]["tools"]
+ tool_by_name = {tool["name"]: tool for tool in tools_list}
+ assert len(tool_by_name) == len(tools_list)As per coding guidelines, tests/**/*.py: "Focus on whether tests prove the changed behavior and include negative, replay, boundary, or regression cases where relevant."
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| tool_by_name = {tool["name"]: tool for tool in tools["result"]["tools"]} | |
| assert all("inputSchema" in tool for tool in tool_by_name.values()) | |
| tools_list = tools["result"]["tools"] | |
| tool_by_name = {tool["name"]: tool for tool in tools_list} | |
| assert len(tool_by_name) == len(tools_list) | |
| assert all("inputSchema" in tool for tool in tool_by_name.values()) |
| bounty_tool = tool_by_name["get_bounty"] | ||
| assert "accepted awards" in bounty_tool["description"] | ||
| attempt_tool = next( | ||
| tool for tool in tools["result"]["tools"] if tool["name"] == "list_bounty_attempts" | ||
| ) | ||
| assert bounty_tool["inputSchema"]["required"] == ["id"] | ||
| assert bounty_tool["inputSchema"]["properties"]["include_awards"]["type"] == "boolean" | ||
| attempt_tool = tool_by_name["list_bounty_attempts"] | ||
| assert "active-attempt reservations" in attempt_tool["description"] | ||
| assert attempt_tool["inputSchema"]["required"] == ["bounty_id"] | ||
| assert attempt_tool["inputSchema"]["properties"]["include_expired"]["type"] == "boolean" | ||
| assert tool_by_name["get_balance"]["inputSchema"]["required"] == ["account"] | ||
| assert tool_by_name["register_wallet"]["inputSchema"]["required"] == ["public_key_hex"] | ||
| assert tool_by_name["get_wallet"]["inputSchema"]["required"] == ["address"] | ||
| assert tool_by_name["submit_wallet_transfer"]["inputSchema"]["required"] == [ | ||
| "from_address", | ||
| "to_address", | ||
| "amount_mrwk", | ||
| "nonce", | ||
| "signature_hex", | ||
| ] | ||
| assert tool_by_name["get_ledger_entry"]["inputSchema"]["required"] == ["sequence"] | ||
| assert tool_by_name["get_proof"]["inputSchema"]["required"] == ["hash"] |
There was a problem hiding this comment.
Expand schema regression assertions beyond required fields.
For several newly covered tools, this test only checks required. It does not lock in key constraints introduced by this PR (e.g., additionalProperties: False, integer bounds/defaults), so schema contract drift can pass unnoticed.
As per coding guidelines, tests/**/*.py: "Focus on whether tests prove the changed behavior and include negative, replay, boundary, or regression cases where relevant."
GHX5T-SOL
left a comment
GHX5T-SOL
left a comment
There was a problem hiding this comment.
Reviewed current head 156c634c4bf2bfa85e5d129f470fb9a21228fb1e for the #447 review bounty.
I am requesting changes for one schema-contract mismatch: several newly advertised required string arguments allow the empty string in tools/list, but the runtime tools/call validators reject empty strings. For example, get_balance.inputSchema.properties.account has no minLength, so a schema-driven MCP client can treat { "account": "" } as valid; the handler then rejects it through str_arg("account") / normalized_account() with invalid tool arguments. The same mismatch applies to the other newly advertised required strings that are parsed with non-empty str_arg(...) or stricter normalizers, such as public_key_hex, address, from_address, to_address, amount_mrwk, signature_hex, and hash.
Local repro:
get_balance_account_minLength None
empty_account_call {'jsonrpc': '2.0', 'id': 2, 'error': {'code': -32602, 'message': 'invalid tool arguments'}}
Suggested fix: add minLength: 1 or a stricter pattern/length where the runtime contract is narrower, and extend the schema regression assertions so this does not drift again.
Validation run locally on this head:
PYTEST_DISABLE_PLUGIN_AUTOLOAD=1 uv run --extra dev python -m pytest tests/test_api_mcp.py::test_mcp_tools_list_and_call tests/test_api_mcp.py::test_mcp_list_bounty_attempts_reports_active_and_expired tests/test_api_mcp.py::test_mcp_get_proof_returns_public_proof_details -q -> 3 passed
PYTEST_DISABLE_PLUGIN_AUTOLOAD=1 uv run --extra dev python -m pytest tests/test_api_mcp.py -q -> 76 passed
PYTEST_DISABLE_PLUGIN_AUTOLOAD=1 uv run --extra dev python -m pytest -q -> 414 passed
PYTEST_DISABLE_PLUGIN_AUTOLOAD=1 uv run --extra dev python scripts/docs_smoke.py -> docs smoke ok
uv run --extra dev ruff check app/mcp.py tests/test_api_mcp.py -> passed
uv run --extra dev ruff format --check app/mcp.py tests/test_api_mcp.py -> 2 files already formatted
uv run --extra dev python -m mypy app -> success
git diff --check origin/main...HEAD -> clean
gitleaks detect --no-banner --redact --source . --log-opts origin/main..HEAD --exit-code 1 -> no leaks found
adliebe
left a comment
adliebe
left a comment
There was a problem hiding this comment.
Requesting changes on current head 156c634c4bf2bfa85e5d129f470fb9a21228fb1e.
The direction is right: PR #488 closes the live MCP discovery gap from #405 by adding inputSchema to the public tools that were previously opaque. I checked app/mcp.py against the runtime dispatcher in app/mcp_tools.py, and the required field names/limits mostly line up with the current handler contract for list_bounties, get_bounty, list_bounty_attempts, get_balance, register_wallet, get_wallet, submit_wallet_transfer, get_ledger_entry, and get_proof.
One schema/runtime mismatch should be fixed before merge: several newly advertised required string fields allow "" at the schema layer even though tools/call rejects them through str_arg(..., allow_empty=False). Concrete examples:
get_balance.accountis advertised as just{"type": "string"}, butcall_mcp_tool()callsstr_arg("account").register_wallet.public_key_hex,get_wallet.address,submit_wallet_transfer.from_address,to_address,amount_mrwk,signature_hex, andget_proof.hashhave the same shape.- Existing test coverage already proves at least one mismatch:
tests/test_api_mcp.py::test_mcp_rejects_invalid_string_argumentsrejects{"account": ""}forget_balance.
For a discovery schema, that matters because an agent using tools/list would see an empty required string as schema-valid, then get -32602 invalid tool arguments only after making the call. Please add minLength: 1 to the required string fields that flow through str_arg(...), and add a regression assertion so this does not drift again.
Validation I ran locally on this head:
uv run --extra dev python -m pytest tests/test_api_mcp.py::test_mcp_tools_list_and_call -q-> 1 passeduv run --extra dev python -m pytest tests/test_api_mcp.py -q-> 76 passeduv run --extra dev ruff check app/mcp.py tests/test_api_mcp.py-> passeduv run --extra dev ruff format --check app/mcp.py tests/test_api_mcp.py-> 2 files already formattedgit diff --check origin/main...HEAD-> clean
Non-blocking merge-order note: PR #471 separately tightens the submit_work_proof repo / issue_number dependency. If both PRs land, make sure #488 does not leave that schema behind.
3 participants
Summary
inputSchemametadata for the public MCP tools that previously only exposed name/descriptionRefs #406.
Evidence
The public MCP
tools/listresponse currently lists 10 tools, but onlysubmit_work_proofadvertises aninputSchema. Public tools such aslist_bounty_attempts,get_bounty,get_balance,get_wallet,get_ledger_entry, andget_proofaccept arguments, but agents cannot discover the accepted shape from the tool list. A live public smoke check reproduced this with unauthenticated JSON-RPC calls and showedlist_bounty_attemptsworks withbounty_idwhile repo/issue arguments returninvalid tool argumentswithout schema guidance.This PR keeps handler behavior unchanged and only exposes the existing argument contracts through MCP metadata.
Validation
PYTEST_DISABLE_PLUGIN_AUTOLOAD=1 uv run --extra dev python -m pytest tests/test_api_mcp.py::test_mcp_tools_list_and_call tests/test_api_mcp.py::test_mcp_list_bounty_attempts_reports_active_and_expired tests/test_api_mcp.py::test_mcp_get_proof_returns_public_proof_details -q-> 3 passedPYTEST_DISABLE_PLUGIN_AUTOLOAD=1 uv run --extra dev python -m pytest tests/test_api_mcp.py -q-> 76 passeduv run --extra dev ruff check app/mcp.py tests/test_api_mcp.py-> passeduv run --extra dev ruff format --check app/mcp.py tests/test_api_mcp.py-> 2 files already formatteduv run --extra dev python scripts/docs_smoke.py-> docs smoke okuv run --extra dev python -m mypy app-> successgit diff --check-> cleanOut of scope
Summary by CodeRabbit
New Features
Tests