MCP output schemas and schema/runtime conformance tests (Bounty #946)#1180
MCP output schemas and schema/runtime conformance tests (Bounty #946)#1180yanyishuai wants to merge 1 commit into
Conversation
|
Warning Review limit reached
More reviews will be available in 48 minutes and 22 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based credits. 🚦 How do rate limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (5)
📝 WalkthroughWalkthroughCentralizes MCP tool output schemas, tightens unknown-argument handling, and adds schema/runtime conformance tests and documentation for the affected tools. ChangesMCP Output Schema and Argument Validation
Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 6✅ Passed checks (6 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
yanyishuai
force-pushed
the
fix/issue-946-mcp-output-schemas
branch
from
07c238d to
2675473
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: 498579dd-d155-4ddb-8941-bd11b9a9cd4b
📒 Files selected for processing (5)
app/mcp.pyapp/mcp_tools.pydocs/agent-guide.mdtests/test_api_mcp.pytests/test_mcp_schema_conformance.py
qingfeng312
left a comment
qingfeng312
left a comment
There was a problem hiding this comment.
CI is failing in the MCP schema/runtime conformance test.
tests/test_api_mcp.py::test_mcp_tools_list_and_call now reaches the balance tool assertions and raises KeyError: 'outputSchema', which means at least one exposed MCP tool is missing the advertised output schema expected by the API tests.
Please add the missing output schema for the balance tool, or adjust the schema generation consistently so all MCP tools expose the contract expected by the tests.
Evidence: GitHub Actions run 28346068593, job 83969516521, 1 failed, 905 passed.
yanyishuai
force-pushed
the
fix/issue-946-mcp-output-schemas
branch
3 times, most recently
from
66f6797 to
b379e6a
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/test_api_mcp.py (1)
2443-2471: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winAssert the classified
formaterror payload for the new null case.The added
{"format": null}case only proves the legacy envelope. It would still pass if the classifier stopped returning the field-leveldatapayload forformat, which is the behavior this change is meant to cover. As per coding guidelines, "Add or update tests for changed behavior"; as per path instructions, "Focus on whether tests prove the changed behavior and include negative, replay, boundary, or regression cases where relevant."Suggested fix
def test_mcp_submit_work_proof_rejects_invalid_bounty_selectors( sqlite_url: str, arguments: dict[str, object], request_id: int ) -> None: @@ - _assert_invalid_tool_arguments_envelope(response.json(), request_id=request_id) + expected_data = None + if arguments == {"format": None}: + expected_data = { + "code": "invalid_argument", + "tool": "submit_work_proof", + "field": "format", + "message": "must be a string", + } + _assert_invalid_tool_arguments_envelope( + response.json(), request_id=request_id, expected_data=expected_data + )Sources: Coding guidelines, Path instructions
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 389b931a-66d9-4b15-81a0-8909069e01e5
📒 Files selected for processing (5)
app/mcp.pyapp/mcp_tools.pydocs/agent-guide.mdtests/test_api_mcp.pytests/test_mcp_schema_conformance.py
| def output_format_arg() -> str: | ||
| value = args.get("format", "text") | ||
| if value is None: | ||
| if "format" not in args: | ||
| return "text" | ||
| value = args["format"] |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Reject explicit null consistently across optional typed arguments.
This fixes {"format": null}, but the shared optional helpers still accept explicit null as “argument omitted” for other fields. For example, repo, q, limit, include_awards, and include_expired still pass with null even though their schemas only allow string/integer/boolean, so tools/list and tools/call remain out of sync for those parameters.
Suggested direction
def optional_clean_str_arg(field: str) -> str | None:
- value = args.get(field)
- if value is None:
+ if field not in args:
return None
+ value = args[field]
+ if value is None:
+ raise ValueError(f"{field} must be a string")
if not isinstance(value, str):
raise ValueError(f"{field} must be a string")
@@
def list_limit_arg(default: int = 25) -> int:
- if "limit" not in args or args.get("limit") is None:
+ if "limit" not in args:
return default
value = positive_int_arg("limit")
@@
def optional_bool_arg(field: str, default: bool = False) -> bool:
- value = args.get(field, default)
- if value is None:
+ if field not in args:
return default
+ value = args[field]
if not isinstance(value, bool):
raise ValueError(f"{field} must be a boolean")| MCP_WALLET_OUTPUT_SCHEMA: dict[str, Any] = { | ||
| "type": "object", | ||
| "description": "Registered MRWK wallet payload returned in structuredContent.", | ||
| "properties": { | ||
| "address": { | ||
| "type": "string", | ||
| "pattern": "^[mM][rR][wW][kK]1[0-9a-fA-F]{40}$", | ||
| }, | ||
| "public_key_hex": {"type": "string", "pattern": "^[0-9a-fA-F]{64}$"}, | ||
| "label": {"type": ["string", "null"]}, | ||
| "github_login": {"type": ["string", "null"]}, | ||
| "balance_mrwk": {"type": "string"}, | ||
| "nonce": {"type": "integer", "minimum": 0}, | ||
| "next_nonce": {"type": "integer", "minimum": 1}, | ||
| "created_at": {"type": "string"}, | ||
| }, | ||
| "required": [ | ||
| "address", | ||
| "public_key_hex", | ||
| "label", | ||
| "github_login", | ||
| "balance_mrwk", | ||
| "nonce", | ||
| "next_nonce", | ||
| "created_at", | ||
| ], | ||
| "additionalProperties": True, |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Close the wallet output schema.
register_wallet/get_wallet advertise a fixed field set here, and tests/test_api_mcp.py:2941-2960 already asserts those schema keys exactly match the serialized wallet payload. Leaving additionalProperties open means undeclared wallet fields can slip into structuredContent without breaking the published contract.
Suggested fix
MCP_WALLET_OUTPUT_SCHEMA: dict[str, Any] = {
@@
- "additionalProperties": True,
+ "additionalProperties": False,
}📝 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.
| MCP_WALLET_OUTPUT_SCHEMA: dict[str, Any] = { | |
| "type": "object", | |
| "description": "Registered MRWK wallet payload returned in structuredContent.", | |
| "properties": { | |
| "address": { | |
| "type": "string", | |
| "pattern": "^[mM][rR][wW][kK]1[0-9a-fA-F]{40}$", | |
| }, | |
| "public_key_hex": {"type": "string", "pattern": "^[0-9a-fA-F]{64}$"}, | |
| "label": {"type": ["string", "null"]}, | |
| "github_login": {"type": ["string", "null"]}, | |
| "balance_mrwk": {"type": "string"}, | |
| "nonce": {"type": "integer", "minimum": 0}, | |
| "next_nonce": {"type": "integer", "minimum": 1}, | |
| "created_at": {"type": "string"}, | |
| }, | |
| "required": [ | |
| "address", | |
| "public_key_hex", | |
| "label", | |
| "github_login", | |
| "balance_mrwk", | |
| "nonce", | |
| "next_nonce", | |
| "created_at", | |
| ], | |
| "additionalProperties": True, | |
| MCP_WALLET_OUTPUT_SCHEMA: dict[str, Any] = { | |
| "type": "object", | |
| "description": "Registered MRWK wallet payload returned in structuredContent.", | |
| "properties": { | |
| "address": { | |
| "type": "string", | |
| "pattern": "^[mM][rR][wW][kK]1[0-9a-fA-F]{40}$", | |
| }, | |
| "public_key_hex": {"type": "string", "pattern": "^[0-9a-fA-F]{64}$"}, | |
| "label": {"type": ["string", "null"]}, | |
| "github_login": {"type": ["string", "null"]}, | |
| "balance_mrwk": {"type": "string"}, | |
| "nonce": {"type": "integer", "minimum": 0}, | |
| "next_nonce": {"type": "integer", "minimum": 1}, | |
| "created_at": {"type": "string"}, | |
| }, | |
| "required": [ | |
| "address", | |
| "public_key_hex", | |
| "label", | |
| "github_login", | |
| "balance_mrwk", | |
| "nonce", | |
| "next_nonce", | |
| "created_at", | |
| ], | |
| "additionalProperties": False, | |
| } |
| def _assert_structured_matches_required( | ||
| payload: dict[str, Any] | list[Any], schema: dict[str, Any] | ||
| ) -> None: | ||
| if schema.get("type") == "array": | ||
| assert isinstance(payload, list) | ||
| item_schema = schema["items"] | ||
| for item in payload: | ||
| assert isinstance(item, dict) | ||
| for key in item_schema.get("required", []): | ||
| assert key in item | ||
| return | ||
| assert isinstance(payload, dict) | ||
| for key in schema.get("required", []): | ||
| assert key in payload | ||
|
|
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Validate the full advertised schema, not just required keys.
_assert_structured_matches_required() only checks that required keys exist. It never checks declared types, patterns, minima, or additionalProperties, so this suite would still pass if balance_microunits became a string or a closed schema started emitting undeclared fields. As per coding guidelines, "Add or update tests for changed behavior"; as per path instructions, "Focus on whether tests prove the changed behavior and include negative, replay, boundary, or regression cases where relevant."
Sources: Coding guidelines, Path instructions
yanyishuai
force-pushed
the
fix/issue-946-mcp-output-schemas
branch
from
b379e6a to
f2a3257
Compare
|
@qingfeng312 Thanks for the review — CI is green on the latest head. Changes since the failing run:
Latest head: |
|
@qingfeng312 CI is green on the latest head ( |
1 similar comment
|
@qingfeng312 CI is green on the latest head ( |
qingfeng312
left a comment
qingfeng312
left a comment
There was a problem hiding this comment.
Follow-up on current head f2a32572ac62d04e98fd6528de72379122adb9a3.
The earlier MCP output-schema test blocker has been resolved. I rechecked app/mcp.py, app/mcp_tools.py, the agent-guide update, and the new schema/runtime conformance tests.
The affected tools now advertise the expected outputSchema entries, the structuredContent tests cover the runtime shape, and the unknown-argument behavior remains bounded without echoing unexpected caller input.
Validation checked: GitHub CI Quality, readiness, docs, and image checks passed on run 28349428755; CodeRabbit status is success on this head. I did not find a remaining blocker for the scoped #946/#934 change.
2 participants
Summary
Adds focused MCP structured-output and schema/runtime conformance improvements for #946 (also aligns with #934).
outputSchemaforget_balance,get_wallet,get_ledger_entry, andget_proofMCP_WALLET_OUTPUT_SCHEMAfor register/get wallet toolsadditionalProperties: falseat runtime via safeunknown argumenterrors (no caller echo)tests/test_mcp_schema_conformance.pycomparingtools/listschemas withtools/callstructuredContentVerification
Wallet
Do4v7foHJvRJLpRRoGaVPWX6DDEjX3yTK7J91gpwUQpEBounty #946
Refs #934
Summary by CodeRabbit
format: Noneas invalid.tools/listinput/output schema relationships.