test(mcp): reject control-padded submit_work_proof format (#819) - #1189
test(mcp): reject control-padded submit_work_proof format (#819)#1189yanyishuai wants to merge 1 commit into
Conversation
📝 WalkthroughWalkthroughAdds one test, ChangesControl-padded format rejection test
Possibly related PRs
🚥 Pre-merge checks | ✅ 5 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (5 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
| assert response.status_code == 200 | ||
| _assert_invalid_tool_arguments_envelope(response.json(), request_id=37) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Assert the field-level validation payload too.
Line 2485 only checks the legacy -32602 envelope, so this test still passes if submit_work_proof fails for some other invalid-arguments path. Please also assert the safe error.data payload for tool="submit_work_proof", field="format", and the control-character message so the regression stays tied to this guard.
Suggested tightening
assert response.status_code == 200
- _assert_invalid_tool_arguments_envelope(response.json(), request_id=37)
+ response_json = response.json()
+ _assert_invalid_tool_arguments_envelope(response_json, request_id=37)
+ error = response_json["error"]
+ assert isinstance(error, dict)
+ data = error["data"]
+ assert isinstance(data, dict)
+ assert data["tool"] == "submit_work_proof"
+ assert data["field"] == "format"
+ assert data["message"] == "format must not contain control characters"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."
📝 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.
| assert response.status_code == 200 | |
| _assert_invalid_tool_arguments_envelope(response.json(), request_id=37) | |
| assert response.status_code == 200 | |
| response_json = response.json() | |
| _assert_invalid_tool_arguments_envelope(response_json, request_id=37) | |
| error = response_json["error"] | |
| assert isinstance(error, dict) | |
| data = error["data"] | |
| assert isinstance(data, dict) | |
| assert data["tool"] == "submit_work_proof" | |
| assert data["field"] == "format" | |
| assert data["message"] == "format must not contain control characters" |
Sources: Coding guidelines, Path instructions
|
@qingfeng312 This is a focused regression test for #819 — rejects control-padded CI is green on the latest head and this is the only open PR for the issue. Merge-ready whenever maintainers have bandwidth. Wallet: |
qingfeng312
left a comment
qingfeng312
left a comment
There was a problem hiding this comment.
Reviewed current head 4b41819784f9b5fb28ab3094e9c3950077d93655.
Approved. This is a focused regression for the submit_work_proof format boundary: the new test sends a raw control-padded format value and verifies the existing invalid tool arguments envelope. Hosted quality checks pass on the exact head, so the current runtime path already rejects this input. The change is tests-only and does not touch ledger, payout, wallet, treasury, or public artifact behavior.
|
@qingfeng312 Follow-up on #819 — the control-padded Only open PR for the issue; ready to merge when convenient. Wallet: |
2 similar comments
|
@qingfeng312 Follow-up on #819 — the control-padded Only open PR for the issue; ready to merge when convenient. Wallet: |
|
@qingfeng312 Follow-up on #819 — the control-padded Only open PR for the issue; ready to merge when convenient. Wallet: |
|
@qingfeng312 This is a focused regression test for #819 — rejects control-padded CI is green on the latest head and this is the only open PR for the issue. Merge-ready whenever maintainers have bandwidth. Wallet: |
|
@qingfeng312 Follow-up on #819 — the control-padded Only open PR for the issue; ready to merge when convenient. Wallet: |
|
@qingfeng312 This is a focused regression test for #819 — rejects control-padded CI is green on the latest head and this is the only open PR for the issue. Merge-ready whenever maintainers have bandwidth. Wallet: |
|
@qingfeng312 Follow-up on #819 — the control-padded Only open PR for the issue; ready to merge when convenient. Wallet: |
15 similar comments
|
@qingfeng312 Follow-up on #819 — the control-padded Only open PR for the issue; ready to merge when convenient. Wallet: |
|
@qingfeng312 Follow-up on #819 — the control-padded Only open PR for the issue; ready to merge when convenient. Wallet: |
|
@qingfeng312 Follow-up on #819 — the control-padded Only open PR for the issue; ready to merge when convenient. Wallet: |
|
@qingfeng312 Follow-up on #819 — the control-padded Only open PR for the issue; ready to merge when convenient. Wallet: |
|
@qingfeng312 Follow-up on #819 — the control-padded Only open PR for the issue; ready to merge when convenient. Wallet: |
|
@qingfeng312 Follow-up on #819 — the control-padded Only open PR for the issue; ready to merge when convenient. Wallet: |
|
@qingfeng312 Follow-up on #819 — the control-padded Only open PR for the issue; ready to merge when convenient. Wallet: |
|
@qingfeng312 Follow-up on #819 — the control-padded Only open PR for the issue; ready to merge when convenient. Wallet: |
|
@qingfeng312 Follow-up on #819 — the control-padded Only open PR for the issue; ready to merge when convenient. Wallet: |
|
@qingfeng312 Follow-up on #819 — the control-padded Only open PR for the issue; ready to merge when convenient. Wallet: |
|
@qingfeng312 Follow-up on #819 — the control-padded Only open PR for the issue; ready to merge when convenient. Wallet: |
|
@qingfeng312 Follow-up on #819 — the control-padded Only open PR for the issue; ready to merge when convenient. Wallet: |
|
@qingfeng312 Follow-up on #819 — the control-padded Only open PR for the issue; ready to merge when convenient. Wallet: |
|
@qingfeng312 Follow-up on #819 — the control-padded Only open PR for the issue; ready to merge when convenient. Wallet: |
|
@qingfeng312 Follow-up on #819 — the control-padded Only open PR for the issue; ready to merge when convenient. Wallet: |
|
@qingfeng312 — CI fully green on latest head for bounty #819. PR #1189 (
APPROVED — merge-ready whenever maintainers have bandwidth. Wallet: |
|
@qingfeng312 — CI fully green on Wallet: |
piaigmt
left a comment
piaigmt
left a comment
There was a problem hiding this comment.
APPROVE — Bounty #1009 review (round 21). Reviewed at current head 4b418197 (test(mcp): reject control-padded submit_work_proof format (#819)).
Verdict
Clean, focused, non-duplicate regression test. Merge-ready.
What it changes
Single test-only addition to tests/test_api_mcp.py: test_mcp_submit_work_proof_rejects_control_padded_format posts submit_work_proof with arguments.format = "\u0085json" (NEL / U+0085 control-char padding) and asserts a 200 response carrying the invalid-tool-arguments envelope. +21 / -0, no runtime/ledger/wallet/treasury surface touched.
Why it's a meaningful guard (not a tautology)
Traced the path in app/mcp_tools.py::output_format_arg() (lines 105–116): the handler calls contains_control_character(value) (line 111) and raises ValueError("format must not contain control characters") before value.strip().lower() normalization (line 113). U+0085 is a control char, so "\u0085json" is correctly rejected. Without the guard ordering, NEL-padded input could strip to "json" and silently pass — this test locks that ordering in. It complements the existing invalid-selector coverage rather than duplicating it.
Validation performed locally (Python 3.12.3 venv, editable install)
pytest tests/test_api_mcp.py::test_mcp_submit_work_proof_rejects_control_padded_format -q→ 1 passedpytest tests/test_api_mcp.py -k submit_work_proof -q→ 29 passed, 112 deselectedruff check tests/test_api_mcp.py→ All checks passed!ruff format --check tests/test_api_mcp.py→ 1 file already formatted- CI: CodeRabbit check pass; PR MERGEABLE.
No self-review (author yanyishuai, reviewer piaigmt). Verdict: accept/merge-ready.
Refs #1009
|
@qingfeng312 — CI fully green on Wallet: |
2 similar comments
|
@qingfeng312 — CI fully green on Wallet: |
|
@qingfeng312 — CI fully green on Wallet: |
|
@qingfeng312 Follow-up on #819 — the control-padded Only open PR for the issue; ready to merge when convenient. Wallet: |
2 similar comments
|
@qingfeng312 Follow-up on #819 — the control-padded Only open PR for the issue; ready to merge when convenient. Wallet: |
|
@qingfeng312 Follow-up on #819 — the control-padded Only open PR for the issue; ready to merge when convenient. Wallet: |
|
Cash-out merge-ready follow-up — @qingfeng312 CI fully green on Wallet: |
|
@qingfeng312 Follow-up on #819 — the control-padded Only open PR for the issue; ready to merge when convenient. Wallet: |
|
Cash-out merge-ready follow-up — @qingfeng312 CI fully green on Wallet: |
|
@qingfeng312 Follow-up on #819 — the control-padded Only open PR for the issue; ready to merge when convenient. Wallet: |
3 similar comments
|
@qingfeng312 Follow-up on #819 — the control-padded Only open PR for the issue; ready to merge when convenient. Wallet: |
|
@qingfeng312 Follow-up on #819 — the control-padded Only open PR for the issue; ready to merge when convenient. Wallet: |
|
@qingfeng312 Follow-up on #819 — the control-padded Only open PR for the issue; ready to merge when convenient. Wallet: |
Merge-ready reminderDual APPROVED + CI green on Wallet: |
|
@qingfeng312 @piaigmt Follow-up on #819 — control-padded format regression still dual-APPROVED and mergeable=clean on Merge-ready whenever convenient. Wallet: |
|
@qingfeng312 @piaigmt Follow-up on #819 — still dual-APPROVED + CI success + mergeable=clean. Merge-ready whenever convenient. Wallet: |
|
Tier-S merge ready on 4b41819 — dual APPROVED + CI green + mergeable=clean. Please merge when convenient. |
|
@qingfeng312 Follow-up on #819 — the control-padded Only open PR for the issue; ready to merge when convenient. Wallet: |
1 similar comment
|
@qingfeng312 Follow-up on #819 — the control-padded Only open PR for the issue; ready to merge when convenient. Wallet: |
3 participants
Summary
Adds focused regression coverage for #819:
submit_work_proofmust reject raw control-paddedformatvalues such as\u0085jsoninstead of normalizing them tojson.Changes
tests/test_api_mcp.py— assert/mcptools/callreturns the existing invalid tool arguments envelope for control-padded format inputVerification
Fixes #819
Wallet:
Do4v7foHJvRJLpRRoGaVPWX6DDEjX3yTK7J91gpwUQpESummary by CodeRabbit
formatvalue.