Surface safe field-level MCP argument errors via additive error.data#1045
Surface safe field-level MCP argument errors via additive error.data#1045jdjioe5-cpu wants to merge 2 commits into
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 (3)
📝 WalkthroughWalkthroughWhen MCP tool calls raise whitelisted ChangesMCP error.data 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 |
|
Submitted for SummaryAdditive Why this laneEight other open PRs against #946 cover Touched files (4)
Validation (all green)
Bounty eligibility (re-checked at submission)
Distinct from my own open PR #1020PR #1020 (still open, mergeable) is the focused Distinct from the other eight #946 PRsPRs #1008, #1013, #1017, #1019, #1029, #1031, #1035, #1038 all add Out of scope (per bounty body)No new write-capable MCP tools, no admin-token APIs, no payouts, no treasury mutation, no ledger mutation, no wallet custody, no bridge, exchange, off-ramp, cash-out, or price behavior. No MRWK investment or price claims. No private security details, secrets, or fabricated payout claims. |
Bounty ramimbo#946 When call_mcp_tool raises a ValueError whose message matches a closed whitelist of safe phrases, attach an additive error.data payload to the existing -32602 invalid tool arguments envelope: { "code": -32602, "message": "invalid tool arguments", "data": { "code": "invalid_argument", "tool": <requested_tool>, "field": <known_field_or_null>, "message": <static_whitelisted_phrase> } } Caller input is never echoed: every component of the data payload is checked against _KNOWN_TOOL_FIELDS / _KNOWN_FIELD_MESSAGES / _KNOWN_FIELDLESS_MESSAGES before being placed in the response. When the ValueError message does not match the whitelist, error.data is omitted and the response is byte-for-byte identical to the previous envelope. KeyError, TypeError, LedgerError, and HTTPException paths still go through the unchanged _jsonrpc_error. Updated 18+1 existing exact-dict error-envelope assertions in tests/test_api_mcp.py and tests/test_security.py to use a small helper that asserts code + message + (optional) data shape, instead of brittle full-dict equality. Added 9 focused new tests covering: known field-level phrases (limit, account, include_expired, status), known field-less phrases (unknown tool, repo requires issue_number), the unmatched-phrase fallthrough, and PII safety (caller-supplied int values and oversized integer values never appear in the response). docs/agent-guide.md documents the additive error.data shape with an example and a note on the legacy-fallback behavior.
jdjioe5-cpu
force-pushed
the
hermes/mergework-946-mcp-field-error-data
branch
from
dbe90dd to
304fbee
Compare
![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: 9a88e78f-2a04-4496-a0c3-4844945de6d4
📒 Files selected for processing (4)
app/mcp.pydocs/agent-guide.mdtests/test_api_mcp.pytests/test_security.py
modelsbridgeaicom-ship-it
left a comment
modelsbridgeaicom-ship-it
left a comment
There was a problem hiding this comment.
Reviewed current head 304fbeecbab70c9cf9b942b8a2a5e61675ad1905 from the MCP argument-error safety/contract angle. I am requesting changes because the current implementation violates its own no-caller-input-echo contract for unknown tools and leaves one advertised whitelist path unreachable.
Findings:
error.data.toolechoes arbitrary unknown tool names.
handle_mcp_request() passes the request-supplied name directly into _invalid_tool_arguments_response(...), and _invalid_tool_arguments_response() writes it to error.data.tool whenever _classify_value_error() recognizes the ValueError. For the unknown tool path, the tool name is not a known tool; it is caller-controlled input. This contradicts the new comments/docs that say untrusted caller input is never echoed and that the payload is safe for LLM prompts/logs.
Concrete reproduction on this head:
{"error":{"code":-32602,"data":{"code":"invalid_argument","field":null,"message":"unknown tool","tool":"unknown_tool_SECRET_SENTINEL_1234567890"},"message":"invalid tool arguments"},"id":77,"jsonrpc":"2.0"}That response came from a tools/call request with params.name = "unknown_tool_SECRET_SENTINEL_1234567890". The sentinel should not be reflected in the structured error payload. A safe shape would omit error.data for unknown tools, use tool: null, or only attach tool after checking the requested name against the advertised tool-name set.
The new test test_mcp_field_error_data_attaches_known_fieldless_unknown_tool currently locks in the unsafe behavior by expecting "tool": "definitely_unknown", so the test needs to change with the implementation.
- The
matches multiple bountieswhitelist entry is unreachable for the actual exception message.
The runtime raises ValueError("issue_number matches multiple bounties"). _classify_value_error() splits that into head="issue_number" and tail="matches multiple bounties", then only checks the tail against _KNOWN_FIELD_MESSAGES, not _KNOWN_FIELDLESS_MESSAGES. The fieldless whitelist contains "matches multiple bounties", but the actual full message is never exactly that string, so no error.data is attached for this intended case.
Concrete classifier check on this head:
issue_number matches multiple bounties => None
matches multiple bounties => {'code': 'invalid_argument', 'field': None, 'message': 'matches multiple bounties'}
repo can only be used with issue_number => {'code': 'invalid_argument', 'field': None, 'message': 'repo can only be used with issue_number'}
If this error is intended to be structured, it should probably be a field-level mapping for issue_number + matches multiple bounties, with coverage that creates/queries duplicate issue numbers. If not intended, remove the dead whitelist entry so the documented contract stays finite and accurate.
Validation I ran on the reviewed head:
.\.venv\Scripts\python.exe -m pytest tests\test_api_mcp.py -k "field_error_data or mcp_tools_list_and_call" -q-> 10 passed, 121 deselected, 1 existing Starlette/httpx warning..\.venv\Scripts\python.exe -m pytest tests\test_security.py::test_mcp_malformed_tool_call_returns_jsonrpc_error -q-> 1 passed, 1 existing Starlette/httpx warning..\.venv\Scripts\ruff.exe check app\mcp.py tests\test_api_mcp.py tests\test_security.py docs\agent-guide.md-> all checks passed..\.venv\Scripts\ruff.exe format --check app\mcp.py tests\test_api_mcp.py tests\test_security.py-> 3 files already formatted..\.venv\Scripts\python.exe -m mypy app\mcp.py-> success..\.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-> clean treeb9675254ab056ababd64a3b9181e109708ce75e0.
GitHub state before review: open, non-draft, mergeable/CLEAN, hosted CI success, CodeRabbit success with actionable comments, no human reviews, and no visible #933 review-bounty claim for PR #1045.
Scope boundaries: this is a read-only review of MCP dispatcher error handling, tests, and docs. I did not use private data, credentials, production mutation, payout execution, treasury/proposal execution, ledger mutation, wallet behavior, exchange/bridge/cash-out behavior, MRWK price behavior, or fabricated payout claims.
Address both unresolved CodeRabbit review threads on PR ramimbo#1045 (Bounty ramimbo#946): 1. error.data.tool no longer reflects arbitrary caller input. Define _KNOWN_TOOL_NAMES from MCP_TOOLS at import time. The dispatcher now sets data.tool to the registered name when tool_name is a known tool, and to None for the unknown-tool path where the rejected name is, by definition, not a member of the static MCP_TOOLS list. Caller input is never echoed. 2. _classify_value_error reaches the field-less whitelist when the message carries a known field prefix. The phrase 'issue_number matches multiple bounties' was previously unreachable because the field-prefixed branch only matched _KNOWN_FIELD_MESSAGES and the field-less branch only matched the bare message. Add a third branch that drops the leading field token when the tail is in _KNOWN_FIELDLESS_MESSAGES. Tests: - test_mcp_field_error_data_attaches_known_fieldless_unknown_tool now asserts data.tool is None for the rejected unknown tool. - test_mcp_field_error_data_binds_known_tool_name is new: positive half of the data.tool bounding guarantee. - test_mcp_field_error_data_reaches_fieldless_matches_multiple_bounties is new: exercises the new branch directly because the Bounty UniqueConstraint('repo', 'issue_number') blocks the integration path. The test docstring documents the production constraint. Docs: - agent-guide.md updates the bullet list of additive data fields: data.tool is now 'registered name or null', and the field-less list now includes 'matches multiple bounties' with the prefix caveat. Validation: - pytest tests/test_api_mcp.py -q 133 passed - pytest tests/ -q 895 passed - ruff format --check clean - ruff check clean - mypy app/mcp.py clean - scripts/docs_smoke.py ok Bounty ramimbo#946 Refs ramimbo#946
|
Reviewer-blocker fixes are now in commit What changed
Re-validation
CI on commit Refs #946 |
modelsbridgeaicom-ship-it
left a comment
modelsbridgeaicom-ship-it
left a comment
There was a problem hiding this comment.
Follow-up review on current head 40cd98dec241e6236cad6f5c384d7c3d68ba5e3f.
Approved. This head resolves both blockers from my previous review on 304fbeecbab70c9cf9b942b8a2a5e61675ad1905:
- Unknown tool names are no longer echoed into
error.data.tool;_invalid_tool_arguments_response()now bounds the tool slot to_KNOWN_TOOL_NAMESand returnsNonefor the unknown-tool path. - The
issue_number matches multiple bountiespath now reaches the field-less whitelist branch, with focused direct classifier coverage documenting why the integration path cannot create duplicate(repo, issue_number)bounty rows.
Evidence checked:
- Inspected the follow-up diff in
app/mcp.py,tests/test_api_mcp.py, anddocs/agent-guide.md. - Confirmed the new tests cover both sides of the safe tool binding guarantee: known tools keep their registered name, unknown tools get
None. - Confirmed docs now describe
data.toolas registered name-or-null and explain the field-lessmatches multiple bountiesprefix behavior. - Checked current GitHub state: PR open, non-draft, mergeable/CLEAN, hosted quality check successful, CodeRabbit successful, current head matches
40cd98dec241e6236cad6f5c384d7c3d68ba5e3f.
Validation on this exact head:
.\.venv\Scripts\python.exe -m pytest tests\test_api_mcp.py -k "field_error_data or mcp_tools_list_and_call" -q-> 12 passed, 121 deselected, 1 existing Starlette/httpx warning..\.venv\Scripts\python.exe -m pytest tests\test_security.py::test_mcp_malformed_tool_call_returns_jsonrpc_error -q-> 1 passed, 1 existing Starlette/httpx warning..\.venv\Scripts\python.exe -m pytest tests\test_api_mcp.py tests\test_security.py -q-> 191 passed, 1 existing Starlette/httpx warning..\.venv\Scripts\ruff.exe check app\mcp.py tests\test_api_mcp.py tests\test_security.py docs\agent-guide.md-> All checks passed..\.venv\Scripts\ruff.exe format --check app\mcp.py tests\test_api_mcp.py tests\test_security.py-> 3 files already formatted..\.venv\Scripts\python.exe -m mypy app\mcp.py-> Success: no issues found..\.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-> clean tree334dc2e79c1e10103bdb6bba92b6c4d50cc9497f.
Scope: read-only review of MCP dispatcher error handling, tests, and docs. No ledger mutation, wallet signing/custody, treasury/proposal execution, payout execution, admin-token behavior, private data, secrets, exchange, bridge, cash-out, MRWK price behavior, or fabricated payout claim was used.
|
Re-claiming for Live eligibility (re-checked 2026-06-07T02:58Z)
Live PR state
Why this PR is the right claim for the error-usability lane
What the diff does (2 commits on
|
2 participants
Bounty #946
Summary
error.datapayload to the MCPtools/callerror envelope so clients can act on argument-validation failures without parsing natural language.error.messagestays exactly"invalid tool arguments"for every argument-validation failure, so existing clients that only read the message keep working.fieldandmessageare built from static whitelists inapp/mcp.py.tests/test_api_mcp.pyand the one intests/test_security.pyto use a small helper that assertscode+message+ (optional)datashape, instead of brittle full-dict equality.data), and PII safety (caller-supplied values never appear in the response).docs/agent-guide.mdwith a short paragraph and example for the additiveerror.datashape, including the legacy-fallback note for unmatched phrases.Why
The bounty body explicitly calls out "improving safe field-level MCP argument errors for invalid selectors or fields" as good scope. Eight other open PRs against this bounty add
outputSchemablocks for specific tools; zero of them touch the dispatcher error path. This PR targets that open lane with a minimal, additive, backward-compatible change.What changed
app/mcp.py_KNOWN_TOOL_FIELDS,_KNOWN_FIELD_MESSAGES,_KNOWN_FIELDLESS_MESSAGES— static whitelists of closed-set strings._classify_value_error(exc)— returns a{"code", "field", "message"}dict only when theValueErrormessage matches the whitelist, elseNone._invalid_tool_arguments_response(response_id, tool_name, exc)— builds the legacy envelope, attacheserror.dataonly when the classifier matches.ValueErrorflows through the new helper;(KeyError, TypeError, LedgerError, HTTPException)still goes through the unchanged_jsonrpc_error(nodata).tests/test_api_mcp.py_assert_invalid_tool_arguments_envelope(response_json, request_id, expected_data=_UNSET)— replaces 18 exact-dict assertions in this file with calls to the helper.error.datacontract.tests/test_security.pydocs/agent-guide.mdBackward compatibility
error.codeis unchanged:-32602for every argument-validation failure.error.messageis unchanged: literal"invalid tool arguments"for every argument-validation failure.error.datais additive. When the underlyingValueErrordoes not match the whitelist,error.datais omitted and the response is byte-for-byte identical to the previous envelope.KeyError,TypeError,LedgerError, andHTTPExceptionpaths still go through the legacy_jsonrpc_errorwith noerror.data.Security
error.messageorerror.data. The two new teststest_mcp_field_error_data_does_not_echo_caller_inputandtest_mcp_field_error_data_does_not_echo_oversized_limit_valuepin this invariant.Touched surfaces
app/mcp.pytests/test_api_mcp.pytests/test_security.pydocs/agent-guide.md4 files changed, 572 insertions, 100 deletions.
Validation
python3 -m pytest tests/ -q-> 869 passed, 1 existing Starlette/httpx warning.python3 -m pytest tests/test_api_mcp.py -k field_error -v-> 9 passed.python3 -m ruff check app/mcp.py tests/test_api_mcp.py tests/test_security.py docs/agent-guide.md-> All checks passed.python3 -m ruff format --check app/mcp.py tests/test_api_mcp.py tests/test_security.py-> 3 files already formatted.python3 -m mypy app/mcp.py-> Success: no issues found.python3 scripts/docs_smoke.py-> docs smoke ok.git diff --check-> clean.Out of scope
Summary by CodeRabbit
New Features
Documentation
Tests