Skip to content

Refs #576: Reject control chars in MCP clean filters#612

Open
Squirbie wants to merge 1 commit into
ramimbo:mainfrom
Squirbie:codex/mcp-clean-str-control-576
Open

Refs #576: Reject control chars in MCP clean filters#612
Squirbie wants to merge 1 commit into
ramimbo:mainfrom
Squirbie:codex/mcp-clean-str-control-576

Conversation

@Squirbie
Copy link
Copy Markdown
Contributor

@Squirbie Squirbie commented May 29, 2026

Summary

  • reject raw control characters in MCP optional clean-string arguments before trimming
  • cover list_bounties status/sort/q filters and submit_work_proof repo selector regressions

Distinctness

Validation

  • PYTEST_DISABLE_PLUGIN_AUTOLOAD=1 uv run --extra dev python -m pytest tests/test_mcp_tools.py -q -> 9 passed
  • PYTEST_DISABLE_PLUGIN_AUTOLOAD=1 uv run --extra dev python -m pytest tests/test_mcp_tools.py tests/test_api_mcp.py -q -> 91 passed, 1 warning
  • PYTEST_DISABLE_PLUGIN_AUTOLOAD=1 uv run --extra dev python -m pytest -q -> 486 passed, 1 warning
  • uv run --extra dev ruff check app/mcp_tools.py tests/test_mcp_tools.py -> passed
  • uv run --extra dev ruff format --check app/mcp_tools.py tests/test_mcp_tools.py -> 2 files already formatted
  • uv run --extra dev mypy app/mcp_tools.py -> success
  • PYTEST_DISABLE_PLUGIN_AUTOLOAD=1 uv run --extra dev python scripts/docs_smoke.py -> docs smoke ok
  • git diff --check -> clean

No private data, wallet material, tokens, signatures, live mutations, admin access, price/liquidity/exchange/bridge/off-ramp claims, or fabricated payout claims were used.

Summary by CodeRabbit

  • Bug Fixes

    • Enhanced input validation for bounty list filtering (status, sort, query fields) and work proof submission to reject requests containing invalid control characters.
  • Tests

    • Added comprehensive test coverage validating control character rejection across tool inputs.

Review Change Stack

@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented May 29, 2026

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: 907eef11-3198-42ef-8f8f-e2efa1ef508a

📥 Commits

Reviewing files that changed from the base of the PR and between e00dc75 and 0991b44.

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

📝 Walkthrough

Walkthrough

MCP tool input validation now rejects control characters. The optional_clean_str_arg validator uses CONTROL_CHAR_RE to detect control characters in string arguments and raises ValueError when found. Tests cover list_bounties filters and submit_work_proof repo selection.

Changes

Control character validation in MCP tools

Layer / File(s) Summary
Control character validation and tests
app/mcp_tools.py, tests/test_mcp_tools.py
CONTROL_CHAR_RE is imported and applied in optional_clean_str_arg to reject control characters in MCP string arguments. Parametrized tests validate that list_bounties filter fields and submit_work_proof repo selector reject control-character input with expected error messages.

Possibly related PRs

  • ramimbo/mergework#398: Extracted and refactored MCP dispatcher logic that this PR's validation updates extend.
  • ramimbo/mergework#286: Introduced list_bounties filter inputs (status, q) that this PR adds control-character validation coverage for.
🚥 Pre-merge checks | ✅ 6
✅ Passed checks (6 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly names the changed surface (control character rejection in MCP clean filters) and directly relates to the main changeset modifications.
Description check ✅ Passed The description covers the summary, distinctness, and validation evidence comprehensively, though it lacks the formal template sections (Evidence, Test Evidence checkboxes, MRWK reference).
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 contains no investment, price, cash-out, fabricated payout, or security detail claims. README/AGENTS.md correctly describe MRWK as native coin with proper future disclaimers.
Bounty Pr Focus ✅ Passed Diff matches stated files and line counts. Tests cover list_bounties filters (status/sort/q) and submit_work_proof repo rejecting control chars. Scope limited to MCP tools only.

✏️ 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

@barnacleagent-svg barnacleagent-svg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Verdict: APPROVED

Scope: Rejects control characters in MCP clean filters. Uses existing CONTROL_CHAR_RE pattern. Consistent with prior validation PRs.

Checklist:

  • Focused validation addition
  • Uses established patterns
  • CI passing

Conclusion: Clean input validation. Ready to merge.

Copy link
Copy Markdown

@eliasx45 eliasx45 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 0991b4483df4b35875b4c813c5369f59bd62ac08 for Refs #576.

Verdict: approve.

The PR is focused on MCP tool argument handling and uses the same established CONTROL_CHAR_RE validation before trimming optional string fields. That closes the path where raw tab/newline/carriage-return input could normalize into an accepted status, sort, q, or repo selector. The tests cover both list_bounties filters and submit_work_proof repo selection, and the change does not overlap the separate public/admin route validation PRs.

Validation on this checkout:

git diff --check origin/main...HEAD
# clean

git merge-tree --write-tree origin/main HEAD
# 9524e49edac32e65ef555b8af40942f8d5268b34

PYTEST_DISABLE_PLUGIN_AUTOLOAD=1 ..\mergework\.venv\Scripts\python.exe -m pytest tests\test_mcp_tools.py -q
# 9 passed

PYTEST_DISABLE_PLUGIN_AUTOLOAD=1 ..\mergework\.venv\Scripts\python.exe -m pytest tests\test_mcp_tools.py tests\test_api_mcp.py -q
# 91 passed

..\mergework\.venv\Scripts\python.exe -m ruff check app\mcp_tools.py tests\test_mcp_tools.py
# passed

..\mergework\.venv\Scripts\python.exe -m ruff format --check app\mcp_tools.py tests\test_mcp_tools.py
# 2 files already formatted

PYTEST_DISABLE_PLUGIN_AUTOLOAD=1 ..\mergework\.venv\Scripts\python.exe -m mypy app\mcp_tools.py
# success

PYTEST_DISABLE_PLUGIN_AUTOLOAD=1 ..\mergework\.venv\Scripts\python.exe scripts\docs_smoke.py
# docs smoke ok

GitHub readback: PR is open/non-draft, merge state is clean, project CI is green, and CodeRabbit has no actionable comments.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants