feat: add MCP input schemas#738
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds shared numeric JSON-Schema constants, expands MCP_TOOLS entries with per-tool ChangesMCP Tool Input Schema Contract
Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 6✅ Passed checks (6 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: 2
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 2adb2242-5567-4f86-bed6-60e1a5e2f1cd
📒 Files selected for processing (3)
app/mcp.pydocs/api-examples.mdtests/test_api_mcp.py
Gwani-28
left a comment
Gwani-28
left a comment
There was a problem hiding this comment.
Requesting changes for one schema/runtime mismatch in the signed transfer tool.\n\nThe new docs say tools/list inputSchema is the preflight argument contract while server validation remains authoritative, and #710 asks that schemas mirror current handler validation. For submit_wallet_transfer, the schema currently allows two values the runtime can never accept:\n\n- app/mcp.py:112-115 uses pattern ^\d+(?:.\d{1,6})?$ for amount_mrwk, which admits "0" and "0.000000". The authoritative parser parse_mrwk_amount rejects non-positive amounts with "amount must be positive".\n- app/mcp.py:116 declares nonce minimum 0. Wallets start at nonce 0 and _verify_wallet_payload requires nonce == wallet.nonce + 1, so nonce 0 is not a valid transfer nonce for any normal registered wallet state.\n\nBecause these are wallet-transfer fields, agents using the new schema for preflight would still generate calls that the server rejects immediately. Please tighten the schema/test coverage so transfer preflight rejects zero MRWK amounts and nonce 0, or otherwise document why the schema is intentionally wider than the runtime validator.\n\nLocal validation on current head 7d8fdf5:\n- .venv/bin/python -m pytest tests/test_api_mcp.py tests/test_docs_public_urls.py -q -> 114 passed, 1 existing Starlette/httpx warning\n- .venv/bin/ruff check app/mcp.py tests/test_api_mcp.py -> passed\n- .venv/bin/ruff format --check app/mcp.py tests/test_api_mcp.py -> 2 files already formatted\n- .venv/bin/python scripts/docs_smoke.py -> docs smoke ok\n- git diff --check origin/main...HEAD -> clean\n- git merge-tree --write-tree origin/main HEAD -> clean tree 91f8e8e98ea8d18e1be9b2bac3775a97b7aa6880\n- gh pr checks 738 --repo ramimbo/mergework -> CodeRabbit passed/skipped and Quality/readiness/docs/image checks passed\n\nNo private data, credentials, wallet material, production mutation, price/exchange/bridge/off-ramp behavior, or fabricated payout claims were used.
Gwani-28
left a comment
Gwani-28
left a comment
There was a problem hiding this comment.
Rechecked current head 6f403d2 after the schema-alignment follow-up.
The new commit improves the MCP contract by allowing integer strings for ids/limits and by documenting the MCP 100-row limit separately from the REST 200-row limit. That addresses the numeric-string/doc mismatch from the earlier review path.
The signed transfer schema mismatch is still not fully resolved, though. submit_wallet_transfer.amount_mrwk still uses ^\d+(?:\.\d{1,6})?$, so schema preflight accepts 0 and 0.000000; parse_mrwk_amount() rejects those as non-positive. nonce is now widened to NONNEGATIVE_INTEGER_SCHEMA, but that still allows integer/string zero even though wallet transfers require the next nonce and registered wallets start at nonce 0, so nonce 0 remains an immediately rejected preflight value for normal transfer calls.
Since the docs now explicitly position inputSchema as the agent-side preflight contract while server validators stay authoritative, these wallet-transfer fields should be tightened or the intentional looseness should be documented with tests.
Local validation on current head 6f403d2:
.venv/bin/python -m pytest tests/test_api_mcp.py tests/test_docs_public_urls.py -q-> 114 passed, 1 existing Starlette/httpx warning.venv/bin/ruff check app/mcp.py tests/test_api_mcp.py-> passed.venv/bin/ruff format --check app/mcp.py tests/test_api_mcp.py-> 2 files already formatted.venv/bin/python scripts/docs_smoke.py-> docs smoke okgit diff --check origin/main...HEAD-> cleangit merge-tree --write-tree origin/main HEAD-> clean tree46f4e12398e1966e7a97a9b6a2a2bd9d13b3413fgh pr checks 738 --repo ramimbo/mergework-> Quality/readiness/docs/image checks passed; CodeRabbit was still pending at review time
No private data, credentials, wallet material, production mutation, price/exchange/bridge/off-ramp behavior, or fabricated payout claims were used.
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 79cfce9f-6432-4838-99ab-64575e727c80
📒 Files selected for processing (4)
app/mcp.pydocs/api-examples.mdtests/test_api_mcp.pytests/test_docs_public_urls.py
|
Addressed the current-head review feedback in
Verification: |
xyjk0511
left a comment
xyjk0511
left a comment
There was a problem hiding this comment.
Reviewed current head af049303081e71282744217c1a04a22d7a613a17 as a current-head follow-up.
What I checked:
- inspected the latest MCP schema updates in
app/mcp.py, plus docs/test updates indocs/api-examples.md,tests/test_api_mcp.py, andtests/test_docs_public_urls.py; - verified the latest head tightens numeric preflight semantics with integer-or-string schemas that include control-character exclusion and bounded limits for list endpoints;
- verified the previously reported transfer preflight mismatch is now narrowed:
submit_wallet_transfer.amount_mrwkrejects zero-valued amounts by schema pattern, and transfernoncenow requires positive integer-or-string (minimum=1equivalent); - verified docs now explicitly distinguish MCP list limit cap (
100) from REST list cap (200) and describeinputSchemaas preflight contract while server validators remain authoritative.
Validation run locally:
$env:PYTEST_DISABLE_PLUGIN_AUTOLOAD='1'; python -m pytest tests/test_api_mcp.py tests/test_docs_public_urls.py -q-> 114 passedpython -m ruff check app/mcp.py tests/test_api_mcp.py tests/test_docs_public_urls.py-> passedpython -m ruff format --check app/mcp.py tests/test_api_mcp.py tests/test_docs_public_urls.py-> 3 files already formattedpython scripts/docs_smoke.py-> docs smoke okgit diff --check origin/main...HEAD-> cleangit merge-tree --write-tree origin/main HEAD-> clean tree2af78b676a09c435545e535f7775d80e43d9f91f- hosted Quality/readiness/docs/image checks are green; CodeRabbit was pending at review time.
I did not find a blocker on this current head.
No private data, credentials, wallet material, production mutation, payout/proof/ledger execution, exchange/bridge/cash-out, or price behavior was used or changed.
Difficult-Burger
left a comment
Difficult-Burger
left a comment
There was a problem hiding this comment.
Reviewed current head af049303081e71282744217c1a04a22d7a613a17 as a current-head follow-up after the earlier stale requested-changes review.
No blocker found for the MCP input schema change.
Evidence checked:
- applied the PR patch cleanly onto base
origin/main0eb12a384da80cacf669b0fb98dc06afefbbd2e3in an isolatedmergework-review-738-localworktree; - inspected
app/mcp.py,app/mcp_tools.py,app/ledger/service.py,docs/api-examples.md,tests/test_api_mcp.py, andtests/test_docs_public_urls.py; - confirmed
tools/listnow exposes schemas for the argument-taking MCP tools without changing handler dispatch or runtime validators; - confirmed numeric id and limit schemas accept integer values and integer strings while preserving the runtime positive-int/list-limit behavior, including the MCP
limit <= 100cap; - rechecked the previously reported wallet-transfer mismatch and confirmed the current head tightens
amount_mrwkto reject zero-valued amounts and requires positive nonce semantics, matching the normalwallet.nonce + 1transfer path; - confirmed the docs now distinguish MCP list limits from REST bounty list limits.
Validation run in the isolated dollar-hunt-mergework conda env:
PYTHONPATH=$PWD conda run -n dollar-hunt-mergework pytest tests/test_api_mcp.py tests/test_docs_public_urls.py -q-> 114 passed, 1 existing Starlette/httpx warningPYTHONPATH=$PWD conda run -n dollar-hunt-mergework python scripts/docs_smoke.py-> docs smoke okPYTHONPATH=$PWD conda run -n dollar-hunt-mergework ruff check app/mcp.py tests/test_api_mcp.py tests/test_docs_public_urls.py-> passedPYTHONPATH=$PWD conda run -n dollar-hunt-mergework ruff format --check app/mcp.py tests/test_api_mcp.py tests/test_docs_public_urls.py-> 3 files already formattedPYTHONPATH=$PWD conda run -n dollar-hunt-mergework python -m py_compile app/mcp.py-> passedgit diff --check-> clean
Hosted state checked before review: Quality/readiness/docs/image check passed, CodeRabbit passed, and GitHub reports the PR as mergeable.
No private data, credentials, wallet material, production mutation, payout/proof/ledger execution, exchange/bridge/cash-out, price behavior, or fabricated payout claims were used or changed.
|
Maintainer triage 2026-06-01T05:10Z: holding for bounty-path cleanup. The implementation has useful current-head reviews, but the PR body does not include a live |
Jorel97
left a comment
Jorel97
left a comment
There was a problem hiding this comment.
Reviewed current head af049303081e71282744217c1a04a22d7a613a17 after the PR became dirty against the current base.
Requesting changes because the branch no longer merges cleanly into current origin/main:
git merge-tree --write-tree origin/main HEADreports content conflicts inapp/mcp.pyandtests/test_api_mcp.py.- GitHub also reports
mergeStateStatus: DIRTYfor the PR. - The conflict is on the same MCP surface this PR changes, so it should be resolved before maintainers can safely merge/review the final combined schema behavior.
What I checked on the PR head itself:
- inspected the MCP schema changes in
app/mcp.py, plus docs/tests indocs/api-examples.md,tests/test_api_mcp.py, andtests/test_docs_public_urls.py; - verified the latest branch-local test target still passes:
python -m pytest tests/test_api_mcp.py -q-> 82 passed, 1 existing Starlette/httpx warning; python scripts/docs_smoke.py-> docs smoke ok;python -m ruff check app/mcp.py tests/test_api_mcp.py docs/api-examples.md tests/test_docs_public_urls.py-> passed;python -m ruff format --check app/mcp.py tests/test_api_mcp.py-> 2 files already formatted;git diff --check-> clean;- hosted checks are green (
Quality, readiness, docs, and image checks, CodeRabbit).
So the branch-local implementation looks healthy, but the current-base merge conflict is a blocker that needs a rebase/update. No private data, credentials, wallet material, production mutation, payout/proof/ledger execution, exchange/bridge/cash-out, price behavior, or fabricated payout claims were used.
|
Updated the PR body to include the exact live bounty reference |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
app/mcp.py (2)
14-38:⚠️ Potential issue | 🟠 Major | ⚡ Quick winTighten the integer-string schema to canonical forms only.
These regexes still preflight-accept values like
"+3","03", and" 3", but the MCP runtime now rejects those forms. That meanstools/listcan tell clients an argument is valid andtools/callimmediately rejects the same payload.Proposed fix
-INTEGER_STRING_PADDING = r"(?!.*[\u0000-\u001f\u007f-\u009f])\s*" +INTEGER_STRING_PADDING = r"(?!.*[\u0000-\u001f\u007f-\u009f])" POSITIVE_INTEGER_SCHEMA: dict[str, Any] = { "anyOf": [ {"type": "integer", "minimum": 1}, { "type": "string", - "pattern": rf"^{INTEGER_STRING_PADDING}\+?(?:0*[1-9][0-9]*)\s*$", + "pattern": rf"^{INTEGER_STRING_PADDING}[1-9][0-9]*$", }, ] } NONNEGATIVE_INTEGER_SCHEMA: dict[str, Any] = { "anyOf": [ {"type": "integer", "minimum": 0}, - {"type": "string", "pattern": rf"^{INTEGER_STRING_PADDING}\+?[0-9]+\s*$"}, + {"type": "string", "pattern": rf"^{INTEGER_STRING_PADDING}(?:0|[1-9][0-9]*)$"}, ] } LIST_LIMIT_SCHEMA: dict[str, Any] = { "anyOf": [ {"type": "integer", "minimum": 1, "maximum": 100}, { "type": "string", - "pattern": rf"^{INTEGER_STRING_PADDING}\+?0*(?:[1-9]|[1-9][0-9]|100)\s*$", + "pattern": rf"^{INTEGER_STRING_PADDING}(?:[1-9][0-9]?|100)$", }, ] }
186-214:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
submit_work_proofstill permits invalid selector shapes.This schema only blocks
bounty_idandissue_numbertogether. It still allows{"repo": "owner/name"}and{"bounty_id": 1, "repo": "owner/name"}, even though the handler rejects both. Since{}is also valid for generic guidance, the schema needs to model the three allowed shapes explicitly: generic/no selector,bounty_idselector, orissue_numberselector with optionalrepo.tests/test_api_mcp.py (1)
18-29:⚠️ Potential issue | 🟠 Major | ⚡ Quick winThe schema helper does not verify the canonical string contract.
This only checks that the regex contains the control-character guard, so it would still pass patterns that allow
"+3","03", or"3 ". Add a fewre.fullmatch(...)allow/deny cases here so the schema tests actually lock down the runtime-compatible forms. As per coding guidelines,tests/**/*.py: Focus on whether tests prove the changed behavior and include negative, replay, boundary, or regression cases where relevant.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 2d16adba-6d52-40a8-9a8f-b282414fa114
📒 Files selected for processing (4)
app/mcp.pydocs/api-examples.mdtests/test_api_mcp.pytests/test_docs_public_urls.py
|
Addressed the current-base merge blocker on PR #738 in head
Local validation:
|
weilixiong
left a comment
weilixiong
left a comment
There was a problem hiding this comment.
Requesting changes on current head 5ac3cff218140589ba908ac1adf88a731e00b661 after a fresh current-main review.
Blocker: the numeric inputSchema string patterns do not match the actual MCP runtime parser. POSITIVE_INTEGER_SCHEMA, NONNEGATIVE_INTEGER_SCHEMA, and LIST_LIMIT_SCHEMA permit padded / signed forms such as "+1", "001", and " 1 ". But app/mcp_tools.py:int_arg() currently applies MCP_INTEGER_RE.fullmatch(value) directly with MCP_INTEGER_RE = re.compile(r"^(?:0|-?[1-9][0-9]*)$"); it does not strip padding and does not accept a leading + or leading zeroes. A strict MCP client can therefore pass preflight and still fail tools/call.
This also conflicts with the new docs statement that numeric ids and limits accept integer strings that match the runtime parsers. Please either tighten the schema patterns to the runtime regex semantics or deliberately widen int_arg() and add execution-level tests for the accepted padded forms.
Verification evidence:
- reviewed
app/mcp.py,app/mcp_tools.py, docs, and the MCP tests on current head; git merge-tree --write-tree upstream/main pr-738succeeds (62397c93309dc9b7c3062990f5b3dfb6d096514d);python3 -m pytest tests/test_api_mcp.py tests/test_docs_public_urls.py -q->114 passed;python3 scripts/docs_smoke.py, Ruff check, Ruff format check, andgit diff --checkpass.
The existing tests assert schema shape but do not call the runtime parser with the newly schema-accepted string variants, which is why this mismatch passes locally.
|
Addressed the requested schema/runtime mismatch in head Changes made:
Validation:
|
keilogic
left a comment
keilogic
left a comment
There was a problem hiding this comment.
Reviewed current head 186096a7f9d169d4aa355d3ed9a205d26a0e7b6a after the follow-up updates for the MCP schema/runtime mismatch.
I did not find a remaining blocker. I checked app/mcp.py, app/mcp_tools.py, docs/api-examples.md, tests/test_api_mcp.py, and tests/test_docs_public_urls.py. The current schema patterns now match the MCP integer parser's canonical string behavior for ids and limits: no leading +, no whitespace padding, and no leading zeroes except the single "0" case where zero is valid. The wallet-transfer preflight schema rejects zero-valued amounts and requires a positive nonce, and submit_work_proof now models its valid selector shapes with oneOf so repo alone and bounty_id plus repo are not advertised as valid.
Validation:
PYTEST_DISABLE_PLUGIN_AUTOLOAD=1 .venv\Scripts\python.exe -m pytest tests/test_api_mcp.py tests/test_docs_public_urls.py -q-> 134 passed, 1 existing Starlette/httpx warning..venv\Scripts\python.exe scripts/docs_smoke.py-> docs smoke ok..venv\Scripts\python.exe -m ruff check app/mcp.py tests/test_api_mcp.py tests/test_docs_public_urls.py-> passed..venv\Scripts\python.exe -m ruff format --check app/mcp.py tests/test_api_mcp.py tests/test_docs_public_urls.py-> 3 files already formatted..venv\Scripts\python.exe -m mypy app/mcp.py-> success.git merge-tree --write-tree origin/main HEAD-> clean tree3d4bb42ea13cfdd7abe2db7f97c656e78b8d1fe0.git diff --check origin/main...HEAD-> clean.
GitHub reports mergeStateStatus=CLEAN; hosted Quality/readiness/docs/image and CodeRabbit are green.
|
Maintainer queue pass 2026-06-02: holding this as a possible #776 candidate after that create_bounty proposal executes and GitHub finalization completes. This is not payable under the old/non-live refs now, and #776 is not live yet. If #776 opens, retarget the PR body to |
xiefuzheng713-alt
left a comment
xiefuzheng713-alt
left a comment
There was a problem hiding this comment.
Thanks for the implementation. I verified the branch-local behavior and the focused checks pass for this head:
uv run --extra dev pytest tests/test_api_mcp.py tests/test_docs_public_urls.py -q-> 134 passed, 1 warninguv run --extra dev ruff check app/mcp.py tests/test_api_mcp.py tests/test_docs_public_urls.pyuv run --extra dev ruff format --check app/mcp.py tests/test_api_mcp.py tests/test_docs_public_urls.pyuv run --extra dev mypy app/mcp.pyuv run --extra dev python scripts/docs_smoke.pygit diff --check origin/main...HEAD
Blocking issue: the current head no longer merges cleanly with origin/main. git merge-tree --write-tree origin/main HEAD exits 1 with a content conflict in app/mcp.py:
Auto-merging app/mcp.py
CONFLICT (content): Merge conflict in app/mcp.py
Auto-merging docs/api-examples.md
Auto-merging tests/test_api_mcp.py
Auto-merging tests/test_docs_public_urls.py
GitHub also reports this PR as mergeStateStatus: DIRTY / mergeable: CONFLICTING now. Please rebase or merge current main, resolve the app/mcp.py conflict, and rerun the focused MCP/API/docs checks before merge.
9 participants
Summary
inputSchemametadata for every MCP tool that accepts arguments.tools/listschemas as the preflight argument contract while keeping server validators authoritative.Testing
.venv/bin/python -m pytest tests/test_api_mcp.py tests/test_docs_public_urls.py -q(114 passed, 1 warning).venv/bin/python scripts/docs_smoke.py.venv/bin/python -m ruff check app/mcp.py tests/test_api_mcp.py.venv/bin/python -m ruff format --check app/mcp.py tests/test_api_mcp.pygit diff --checkSource issue: #710
Summary by CodeRabbit
New Features
Documentation
Tests