Skip to content

Reject unexpected MCP wallet arguments#927

Merged
ramimbo merged 1 commit into
ramimbo:mainfrom
alan747271363-art:codex/mcp-wallet-argument-guards-844
Jun 5, 2026
Merged

Reject unexpected MCP wallet arguments#927
ramimbo merged 1 commit into
ramimbo:mainfrom
alan747271363-art:codex/mcp-wallet-argument-guards-844

Conversation

@alan747271363-art
Copy link
Copy Markdown
Contributor

@alan747271363-art alan747271363-art commented Jun 5, 2026

Summary

  • Reject undeclared MCP arguments for wallet write tools: register_wallet and submit_wallet_transfer.
  • Prevent typoed fields such as public_key or signature from being silently ignored while the tool call succeeds or continues to deeper wallet validation.
  • Keep valid wallet registration and transfer argument names, response shapes, signature verification, ledger mutation behavior, and wallet semantics unchanged.

Bounty #844

Why this fits #844

This is a focused MCP safe argument-error and conformance improvement. Existing read-only MCP tools already reject unexpected arguments in open PR #899; this PR applies the same agent-facing safety expectation to wallet write tools without changing valid write behavior.

Duplicate / scope check

Validation

  • .\.venv\Scripts\python.exe -m pytest tests\test_api_mcp.py::test_mcp_can_register_and_fetch_wallet tests\test_api_mcp.py::test_mcp_wallet_write_tools_reject_unexpected_arguments -q -> 2 passed, 1 existing Starlette/httpx warning.
  • .\.venv\Scripts\python.exe -m pytest tests\test_api_mcp.py tests\test_mcp_tools.py -q -> 113 passed, 1 existing Starlette/httpx warning.
  • .\.venv\Scripts\python.exe -m ruff check app\mcp_tools.py tests\test_api_mcp.py -> passed.
  • .\.venv\Scripts\python.exe -m ruff format --check app\mcp_tools.py tests\test_api_mcp.py -> 2 files already formatted.
  • .\.venv\Scripts\python.exe -m mypy app\mcp_tools.py app\mcp.py -> success.
  • .\.venv\Scripts\python.exe scripts\docs_smoke.py -> docs smoke ok.
  • git diff --check -> clean.
  • git merge-tree --write-tree origin/main HEAD -> clean tree abda6f4c7b8cc6d19c1ef4bc0fd2711b121cfce1.

Scope

MCP argument validation only. No wallet registration semantics, transfer signing, nonce validation, amount handling, ledger writes, treasury behavior, payout execution, proposal execution, admin-token behavior, private data, credentials, secrets, exchange, bridge, cash-out, or MRWK price behavior changed.

@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented Jun 5, 2026

Review Change Stack

📝 Walkthrough

Walkthrough

This PR introduces strict argument validation for two MCP wallet tools by adding a reject_unexpected_args helper function that compares provided arguments against an explicit allowlist. The helper is integrated into register_wallet and submit_wallet_transfer handlers to reject extraneous parameters with a formatted error, and is covered by a new test that verifies the MCP error response format.

Changes

Argument Validation for Wallet Tools

Layer / File(s) Summary
Validation helper and tool integration
app/mcp_tools.py
New reject_unexpected_args function enforces allowlist validation and is called in register_wallet and submit_wallet_transfer handlers. Unexpected arguments trigger ValueError with sorted offending key names.
Test coverage for unexpected argument rejection
tests/test_api_mcp.py
Test verifies both wallet write tools return MCP error code -32602 when invoked with unexpected fields.

Possibly related PRs

  • ramimbo/mergework#398: Introduces the initial call_mcp_tool dispatcher with argument handling; this PR extends it with reject_unexpected_args validation.
🚥 Pre-merge checks | ✅ 6
✅ Passed checks (6 passed)
Check name Status Explanation
Title check ✅ Passed The title 'Reject unexpected MCP wallet arguments' accurately and concretely names the changed surface—validation of MCP tool arguments for wallet write tools.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Mergework Public Artifact Hygiene ✅ Passed No README, docs, or public artifact changes; code contains only technical MCP validation with no investment, price, cash-out, payout, or security claims.
Bounty Pr Focus ✅ Passed PR diff matches stated files only: app/mcp_tools.py (+18 lines), tests/test_api_mcp.py (+55 lines). Changes limited to wallet write tool argument validation with test evidence. No unrelated scope.
Description check ✅ Passed The PR description is comprehensive and complete, covering all required template sections with specific details about the changes, validation results, and scope.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.


Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Copy Markdown

@catcherintheroad-hub catcherintheroad-hub left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed current head 1eb0470988cb6fb70995ed64360eaca016e7486d.

Evidence checked:

  • inspected app/mcp_tools.py and tests/test_api_mcp.py;
  • confirmed the new reject_unexpected_args() helper is only wired into the wallet write tools register_wallet and submit_wallet_transfer;
  • confirmed the allowed argument sets preserve the existing accepted inputs for those tools while rejecting alias/extra keys before calling wallet registration or transfer submission;
  • confirmed MCP error handling still maps the raised ValueError to JSON-RPC -32602 / invalid tool arguments through handle_mcp_request();
  • confirmed get_wallet, read-only bounty/proof/ledger tools, wallet address normalization, transfer signing semantics, ledger mutation code, treasury code, and payout code are not changed.

Validation run locally on this exact head in an isolated worktree:

  • uv run --python 3.12 --extra dev python -m pytest tests/test_api_mcp.py::test_mcp_wallet_write_tools_reject_unexpected_arguments tests/test_api_mcp.py::test_mcp_can_register_and_fetch_wallet -q -> 2 passed, 1 existing Starlette/httpx warning.
  • uv run --python 3.12 --extra dev ruff check app/mcp_tools.py tests/test_api_mcp.py -> passed.
  • uv run --python 3.12 --extra dev ruff format --check app/mcp_tools.py tests/test_api_mcp.py -> 2 files already formatted.
  • uv run --python 3.12 --extra dev mypy app/mcp_tools.py -> success.
  • uv run --python 3.12 --extra dev python scripts/docs_smoke.py -> docs smoke ok.
  • git diff --check origin/main...HEAD -> clean.
  • git merge-tree --write-tree origin/main HEAD -> clean tree c10344676ece200675253a0b75d4564cc38f45f5.

GitHub state checked before review: PR open, mergeStateStatus=CLEAN, hosted Quality/readiness/docs/image check successful, CodeRabbit successful/no actionable comments, and no human reviews visible on the current head.

No blocker found in this focused MCP wallet argument guard follow-up.

Copy link
Copy Markdown
Contributor

@pqmfei pqmfei left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed PR #927 at current head 1eb0470988cb6fb70995ed64360eaca016e7486d.

Evidence included in this review:

  • inspected app/mcp_tools.py, app/mcp.py, and tests/test_api_mcp.py for the wallet MCP write-tool argument path;
  • confirmed the only MCP wallet write tools currently exposed are register_wallet and submit_wallet_transfer, and both now reject undeclared arguments before deeper wallet validation;
  • confirmed valid wallet registration/lookup and transfer validation paths keep their existing argument names and JSON-RPC error envelope behavior;
  • checked hosted state: Quality/readiness/docs/image check passed, CodeRabbit passed/skipped, and merge state is clean.

Validation run locally:

  • .\.venv\Scripts\python.exe -m pytest tests\test_api_mcp.py::test_mcp_can_register_and_fetch_wallet tests\test_api_mcp.py::test_mcp_wallet_write_tools_reject_unexpected_arguments -q -> 2 passed, 1 existing Starlette/httpx warning
  • .\.venv\Scripts\python.exe -m pytest tests\test_api_mcp.py tests\test_mcp_tools.py -q -> 113 passed, 1 existing Starlette/httpx warning
  • git diff --check origin/main...HEAD -> clean
  • git merge-tree --write-tree origin/main HEAD -> clean tree c10344676ece200675253a0b75d4564cc38f45f5

Verdict: approved; no blocking issues found. No wallet registration semantics, transfer signing, nonce/amount handling, ledger mutation, treasury behavior, payout execution, private data, exchange, bridge, cash-out, or MRWK price behavior changed.

@ramimbo ramimbo merged commit 22815dd into ramimbo:main Jun 5, 2026
2 checks passed
@ramimbo ramimbo added mrwk:accepted Maintainer accepted for payout mrwk:paid Ledger payment recorded labels Jun 5, 2026 — with ChatGPT Codex Connector
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

mrwk:accepted Maintainer accepted for payout mrwk:paid Ledger payment recorded

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants