Skip to content

fix(mcp): enforce exact submit_work_proof format enum#793

Open
keilogic wants to merge 1 commit into
ramimbo:mainfrom
keilogic:codex/mcp-format-enum-656
Open

fix(mcp): enforce exact submit_work_proof format enum#793
keilogic wants to merge 1 commit into
ramimbo:mainfrom
keilogic:codex/mcp-format-enum-656

Conversation

@keilogic
Copy link
Copy Markdown
Contributor

@keilogic keilogic commented Jun 2, 2026

Summary

  • make submit_work_proof use the default format only when the field is omitted
  • require provided format values to match the advertised MCP enum exactly: text or json
  • add regression coverage for uppercase, padded, and explicit-null format values

Refs #656
Report: #656 (comment)

Validation

  • python -m pytest tests/test_api_mcp.py -q -> 104 passed, 1 warning
  • python -m pytest -q -> 677 passed, 1 warning
  • python -m mypy app -> success
  • ruff check app/mcp_tools.py tests/test_api_mcp.py
  • ruff format --check app/mcp_tools.py tests/test_api_mcp.py
  • git diff --check

Summary by CodeRabbit

  • Bug Fixes
    • Enhanced format argument validation for MCP tools. The format parameter now requires exact case-sensitive values ("text" or "json") only, with null values explicitly rejected and whitespace trimming removed for stricter input handling.

@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented Jun 2, 2026

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: ceadfc9a-b66f-4646-8b8e-1f429ef34e32

📥 Commits

Reviewing files that changed from the base of the PR and between 68845a6 and ac185b9.

📒 Files selected for processing (2)
  • app/mcp_tools.py
  • tests/test_api_mcp.py

📝 Walkthrough

Walkthrough

The PR tightens validation of the format argument in MCP tool calls. The output_format_arg() validator now accepts only exact "text" or "json" strings, defaults to "text" only when the key is absent, and rejects None and case/whitespace variants. Test cases are expanded to verify these stricter rules.

Changes

MCP format argument validation

Layer / File(s) Summary
Format argument validation tightening
app/mcp_tools.py
output_format_arg() now returns "text" only when the "format" key is absent, requires the value to be a string (rejecting None), and accepts only exact "text" or "json" without trimming or lowercasing.
Format validation test coverage
tests/test_api_mcp.py
Test cases expanded to cover format=None, uppercase "JSON", and whitespace-padded " json " as separate invalid selector arguments, each expecting rejection as invalid tool arguments.

Possibly Related PRs

  • ramimbo/mergework#317: Tightens and structures format argument validation for the MCP submit_work_proof tool, overlapping with main PR's stricter "text"/"json" rules and expanded test coverage.
  • ramimbo/mergework#398: Modifies call_mcp_tool argument validation for the format selector, directly overlapping with the main PR's validation changes.
  • ramimbo/mergework#287: Updates MCP submit_work_proof tooling and tests around invalid request arguments, complementary to the main PR's format validation tightening.
🚥 Pre-merge checks | ✅ 6
✅ Passed checks (6 passed)
Check name Status Explanation
Title check ✅ Passed The title accurately names the changed surface and describes the core fix: enforcing exact format enum matching for submit_work_proof.
Description check ✅ Passed The description covers the changes clearly, includes validation evidence, and references the related issue, though it omits some optional template sections like Evidence and MRWK bounty link.
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 PR only modifies code in app/mcp_tools.py and tests/test_api_mcp.py; no README/docs changes. Existing documentation properly describes MRWK as a native coin with conditional future features.
Bounty Pr Focus ✅ Passed PR diff matches stated files (app/mcp_tools.py +4/-5, tests/test_api_mcp.py +3/-0), includes test evidence for three format validation cases (None, uppercase, whitespace), and avoids unrelated 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
Contributor

@yui-stingray yui-stingray 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 ac185b9e21564f19cc40708a732e9dd35ba2c7bb for the #654 review bounty.

No blocker found in this focused fix.

What I checked:

  • submit_work_proof still advertises the format enum as text or json, with text as the default.
  • output_format_arg() now uses that default only when format is omitted; explicit values are no longer trimmed or lowercased before validation.
  • Non-string, null, uppercase, padded, and unsupported format values now all reject through the existing invalid-tool-arguments path.
  • The new regression cases for null, JSON, and json fit 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.

@ramimbo
Copy link
Copy Markdown
Owner

ramimbo commented Jun 2, 2026

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 create_bounty proposals and are not claimable until execution/finalization. Leaving this open with mrwk:needs-info; after a live successor exists, maintainers can decide whether to reroute, merge without a bounty claim, or close as duplicate/superseded.

Copy link
Copy Markdown

@xiefuzheng713-alt xiefuzheng713-alt 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 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.py and confirmed output_format_arg() only defaults when the format field is absent;
  • confirmed explicit None and 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.py and confirmed regression coverage for format: None, format: "JSON", and format: " json " alongside existing invalid selector cases;
  • direct call_mcp_tool() smoke: omitted/default and text returned text guidance, json returned structured guidance, and JSON, padded json, and None all raised bounded ValueErrors.

Validation I ran:

  • uv run --extra dev pytest tests/test_api_mcp.py -q -> 104 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 -> 2 files already formatted
  • uv run --extra dev mypy app/mcp_tools.py -> success
  • uv run --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 06ac7e1336862a048d0155ae73f9cfe23d147602

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

mrwk:needs-info More information needed

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants