fix(mcp): enforce exact submit_work_proof format enum#793
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 tightens validation of the ChangesMCP format argument validation
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 |
yui-stingray
left a comment
There was a problem hiding this comment.
Reviewed current head ac185b9e21564f19cc40708a732e9dd35ba2c7bb for the #654 review bounty.
No blocker found in this focused fix.
What I checked:
submit_work_proofstill advertises theformatenum astextorjson, withtextas the default.output_format_arg()now uses that default only whenformatis omitted; explicit values are no longer trimmed or lowercased before validation.- Non-string,
null, uppercase, padded, and unsupportedformatvalues now all reject through the existing invalid-tool-arguments path. - The new regression cases for
null,JSON, andjsonfit the existing invalid selector coverage, while the existing JSON guidance path remains covered.
Verification:
uv run --extra dev pytest tests/test_api_mcp.py -q --capture=no-> 104 passed, 1 warning.uv run --extra dev pytest -q --capture=no-> 677 passed, 1 warning.uv run --extra dev ruff check app/mcp_tools.py tests/test_api_mcp.py-> passed.uv run --extra dev ruff format --check app/mcp_tools.py tests/test_api_mcp.py-> formatted.git diff --check-> clean.- GitHub shows the hosted quality check and CodeRabbit check successful for this head.
|
Maintainer queue hold: #656 currently has 0 effective awards remaining because the final slot is covered by pending proposal #118. This PR is not accepted or payable under #656 in this state. The successor issues #798/#799 are still pending |
xiefuzheng713-alt
left a comment
There was a problem hiding this comment.
Reviewed current head ac185b9e21564f19cc40708a732e9dd35ba2c7bb as a non-author current-head review.
I did not find a blocker in the focused submit_work_proof.format enum validation fix. The advertised enum now matches runtime behavior: omitted format uses the text default, while provided values must be exactly text or json without case folding or trimming.
Evidence checked:
- inspected
app/mcp_tools.pyand confirmedoutput_format_arg()only defaults when theformatfield is absent; - confirmed explicit
Noneand non-string values reject before guidance generation; - confirmed uppercase and padded strings are no longer normalized into valid enum values;
- inspected
tests/test_api_mcp.pyand confirmed regression coverage forformat: None,format: "JSON", andformat: " json "alongside existing invalid selector cases; - direct
call_mcp_tool()smoke: omitted/default andtextreturned text guidance,jsonreturned structured guidance, andJSON, paddedjson, andNoneall raised boundedValueErrors.
Validation I ran:
uv run --extra dev pytest tests/test_api_mcp.py -q-> 104 passed, 1 warninguv run --extra dev ruff check app/mcp_tools.py tests/test_api_mcp.py-> passeduv run --extra dev ruff format --check app/mcp_tools.py tests/test_api_mcp.py-> 2 files already formatteduv run --extra dev mypy app/mcp_tools.py-> successuv run --extra dev python scripts/docs_smoke.py-> docs smoke okgit diff --check origin/main...HEAD-> cleangit merge-tree --write-tree origin/main HEAD-> clean tree06ac7e1336862a048d0155ae73f9cfe23d147602
GitHub state checked: mergeStateStatus=CLEAN; hosted Quality/readiness/docs/image and CodeRabbit checks are successful on this head. No private data, wallet material, payout execution, treasury mutation, exchange, bridge, cash-out, MRWK price behavior, or issue mutation was used.
alan747271363-art
left a comment
There was a problem hiding this comment.
Reviewed current head ac185b9e21564f19cc40708a732e9dd35ba2c7bb after earlier clean approvals; GitHub now reports mergeStateStatus=DIRTY / mergeable=CONFLICTING against current origin/main.
Requesting changes because the branch-local exact submit_work_proof.format enum validation still passes, but the PR is now blocked by current-main content conflicts in app/mcp_tools.py and tests/test_api_mcp.py. The hosted checks are green on the PR head, but they predate the current-main conflict and do not make the branch mergeable now.
Validation on this exact head:
.\.venv\Scripts\python.exe -m pytest tests/test_api_mcp.py -q-> 104 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-> success..\.venv\Scripts\python.exe scripts/docs_smoke.py-> docs smoke ok.git diff --check origin/main...HEAD-> clean.git merge-tree --write-tree $(git rev-parse origin/main) HEAD-> failed with content conflicts inapp/mcp_tools.pyandtests/test_api_mcp.py.
Suggested next step: rebase or merge current main, resolve the MCP tool/test conflicts while preserving the exact enum validation behavior, then rerun MCP API tests plus ruff/mypy/docs smoke checks before merge.
Scope reviewed: MCP submit_work_proof.format argument validation and regression tests only. No wallet material, ledger mutation, treasury/proposal execution, payout execution, admin-token behavior, private data, credentials, bridge/exchange/cash-out behavior, or MRWK price behavior was used.
yanyishuai
left a comment
There was a problem hiding this comment.
Review packet (Bounty #933)
- Reviewed PR: #793
- Head commit: ac185b9
- Files inspected:
app/mcp_tools.py(output_format_arg),tests/test_api_mcp.py(invalid selector cases) - Verdict: Useful narrow MCP conformance fix, but not merge-ready on current head — PR reports merge conflicts with
mainand needs rebase before maintainer merge. - Validation: Compared advertised
submit_work_proofinputSchema (formatenumtext|json, defaulttext) with runtime behavior change: explicitformat: nullnow rejected instead of silently defaulting totext. That aligns schema/runtime for agents passing null. Added pytest case{"format": None}is appropriate. Recommend rebase onto currentmain, rerun MCP pytest slice, and confirm no overlap with newer open MCP schema work (#946 / PR #1180).
Bounty #933
Summary
submit_work_proofuse the defaultformatonly when the field is omittedformatvalues to match the advertised MCP enum exactly:textorjsonformatvaluesRefs #656
Report: #656 (comment)
Validation
python -m pytest tests/test_api_mcp.py -q-> 104 passed, 1 warningpython -m pytest -q-> 677 passed, 1 warningpython -m mypy app-> successruff check app/mcp_tools.py tests/test_api_mcp.pyruff format --check app/mcp_tools.py tests/test_api_mcp.pygit diff --checkSummary by CodeRabbit