Refs #406: Add MCP input schemas for public tools#488
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThe PR replaces minimal MCP tool metadata with 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
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.
rebel117
left a comment
rebel117
left a comment
There was a problem hiding this comment.
Reviewed PR #488 at current head for bounty #447.
Evidence checked:
- Cross-checked every inputSchema entry against the corresponding argument parsing in app/mcp_tools.py call_mcp_tool().
- list_bounties: status default "open" matches optional_clean_str_arg fallback, limit 1–100 matches list_limit_arg bounds.
- get_bounty: id required/positive matches positive_int_arg, include_awards boolean default False matches optional_bool_arg.
- list_bounty_attempts: bounty_id required, include_expired boolean False, limit 25/100 — all match mcp_tools.py.
- get_balance: account required matches str_arg("account").
- register_wallet: public_key_hex required, label optional — matches conditional optional_str_arg pattern.
- get_wallet: address required matches str_arg.
- submit_wallet_transfer: from_address, to_address, amount_mrwk, nonce, signature_hex required; memo optional — matches str_arg/int_arg calls.
- get_ledger_entry: sequence required/positive matches positive_int_arg.
- get_proof: hash required matches str_arg.
Test coverage: the added assertions verify every tool has inputSchema, spot-check required fields, enum values, and bounds. The refactored tool_by_name lookup is cleaner than the previous next() calls.
No blockers from this pass.
Baijack-star
left a comment
Baijack-star
left a comment
There was a problem hiding this comment.
Reviewed current head 5b0a131fbe0aa81871623d64af6e283f35493300 as a follow-up to the earlier schema/runtime mismatch reviews.
No blocker found in this current head. The previous required-string mismatch is fixed: the required string fields that flow through non-empty runtime validators now advertise minLength: 1 in tools/list, including get_balance.account, register_wallet.public_key_hex, get_wallet.address, submit_wallet_transfer.from_address, to_address, amount_mrwk, signature_hex, and get_proof.hash. The regression assertions cover those fields, the required arrays, integer lower bounds, limit bounds, enum/default metadata, and additionalProperties: false on the advertised schemas.
I also cross-checked the schema shapes against app/mcp_tools.py: list_bounties keeps optional status/q/limit; get_bounty, list_bounty_attempts, and get_ledger_entry use positive integer selectors; optional booleans/defaults match the handler defaults; and submit_work_proof keeps the existing bounty-id vs issue-number mutual exclusion.
Validation run locally on this head:
PYTEST_DISABLE_PLUGIN_AUTOLOAD=1 ./.venv/bin/python -m pytest tests/test_api_mcp.py::test_mcp_tools_list_and_call tests/test_api_mcp.py::test_mcp_rejects_invalid_string_arguments -q-> 7 passedPYTEST_DISABLE_PLUGIN_AUTOLOAD=1 ./.venv/bin/python -m pytest tests/test_api_mcp.py -q-> 76 passedPYTEST_DISABLE_PLUGIN_AUTOLOAD=1 ./.venv/bin/python -m pytest -q-> 414 passed./.venv/bin/python -m ruff check app/mcp.py tests/test_api_mcp.py-> passed./.venv/bin/python -m ruff format --check app/mcp.py tests/test_api_mcp.py-> 2 files already formattedPYTEST_DISABLE_PLUGIN_AUTOLOAD=1 ./.venv/bin/python -m mypy app/mcp.py app/mcp_tools.py-> successPYTEST_DISABLE_PLUGIN_AUTOLOAD=1 ./.venv/bin/python scripts/docs_smoke.py-> docs smoke okgit diff --check origin/main...HEAD-> clean
|
Reviewed current PR #488 head No blocker found on this head. Evidence checked:
This looks mergeable from my reviewed slice. |
eliasx45
left a comment
eliasx45
left a comment
There was a problem hiding this comment.
Reviewed current head 5b0a131 against refreshed origin/main.
Verdict: request changes.
The schema/runtime contract fix looks good on its own: the required MCP string inputs now advertise minLength: 1, the schema assertions cover the required fields and additionalProperties behavior, and focused MCP tests pass. However, the branch is stale against current main and merge-tree reports content conflicts in both touched files:
- app/mcp.py
- tests/test_api_mcp.py
Validation on this head:
- PYTEST_DISABLE_PLUGIN_AUTOLOAD=1 ..venv\Scripts\python.exe -m pytest tests\test_api_mcp.py::test_mcp_tools_list_and_call tests\test_api_mcp.py::test_mcp_rejects_invalid_string_arguments 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 -> 9 passed
- ..venv\Scripts\python.exe -m ruff check app\mcp.py tests\test_api_mcp.py -> passed
- ..venv\Scripts\python.exe -m ruff format --check app\mcp.py tests\test_api_mcp.py -> 2 files already formatted
- PYTEST_DISABLE_PLUGIN_AUTOLOAD=1 ..venv\Scripts\python.exe scripts\docs_smoke.py -> docs smoke ok
- git diff --check origin/main...HEAD -> clean
- git merge-tree --write-tree origin/main HEAD -> conflicts in app/mcp.py and tests/test_api_mcp.py
Required follow-up: rebase/merge current main, resolve those two conflicts, and rerun the focused MCP schema tests. I do not see a remaining schema-level blocker beyond the stale/conflicting base.
MINBBBIGcode
force-pushed
the
codex/mcp-input-schemas
branch
from
5b0a131 to
14caf94
Compare
|
Resolved the stale-base blocker by rebasing this PR onto current main and resolving the conflicts in Current head: Validation run after the rebase:
|
eliasx45
left a comment
eliasx45
left a comment
There was a problem hiding this comment.
Re-reviewed refreshed head 14caf94a0a44ba6080f51a34804e70b1aa7c6ba2 after the rebase/conflict resolution. Verdict: approve.
The stale-base blocker from my previous review is resolved: GitHub reports the branch cleanly mergeable, and a local synthetic merge against current origin/main succeeded. I also rechecked the schema/runtime contract that this PR is meant to fix: every public MCP tool now advertises an inputSchema, required primitive string args use minLength: 1 where the runtime rejects empty strings, enum/default/range metadata matches the existing argument parsing, and submit_work_proof still keeps the bounty selector constraint plus additionalProperties: false.
Validation on this checkout:
git diff --check origin/main...HEAD
# clean
git merge-tree --write-tree origin/main HEAD
# 2437d55eb40aae31eb4dc4142e19658a00ef5021
python -m py_compile app/mcp.py
# passed
.\.venv\Scripts\python.exe -m pytest -q tests/test_api_mcp.py
# 81 passed, 1 warning in 36.36s
Note: this Windows host did not have uv on PATH, so I created an isolated local venv in the temporary review worktree and installed the repo with the dev extra before running pytest.
7 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