Skip to content

fix(mcp): suppress JSON-RPC notification responses#795

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

fix(mcp): suppress JSON-RPC notification responses#795
keilogic wants to merge 1 commit into
ramimbo:mainfrom
keilogic:codex/mcp-notifications-656

Conversation

@keilogic
Copy link
Copy Markdown
Contributor

@keilogic keilogic commented Jun 2, 2026

Summary

  • distinguish JSON-RPC requests from notifications by checking whether the id member is present
  • process MCP notifications without returning JSON-RPC result/error bodies
  • add regression coverage for successful, unknown-method, invalid-param, and invalid-tool-argument notifications

Refs #656
Report: #656 (comment)

Validation

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

Summary by CodeRabbit

  • Bug Fixes

    • MCP notification-style requests (missing ID) now return HTTP 202 with an empty body for applicable operations; JSON-RPC responses remain for requests that include an ID.
  • Tests

    • Added parametrized tests to verify notification requests receive HTTP 202 empty responses instead of JSON-RPC error objects.

@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: 8c9ab3e1-f261-46d3-9669-99974f818ef4

📥 Commits

Reviewing files that changed from the base of the PR and between 135cb58 and a73cd0e.

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

📝 Walkthrough

Walkthrough

The MCP request handler now treats JSON-RPC requests missing an "id" as notifications: it returns an empty HTTP 202 Response for those notification-style requests (including tools/list, tools/call, and several validation/exception paths). When an "id" is present, the handler still returns JSON-RPC error or result objects. Tests added validate the 202 notification behavior.

Changes

MCP notification handling

Layer / File(s) Summary
Response import and signature
app/mcp.py
Import Response from FastAPI and expand handle_mcp_request return type to include Response.
Notification branching and response behavior
app/mcp.py
Handler checks for JSON-RPC "id"; if missing, returns empty Response(status_code=202) for tools/list, tools/call, and several invalid-param/exception paths; if present, returns JSON-RPC error objects or JSON-RPC tool result as before.
Notification-style tests
tests/test_api_mcp.py
Add parametrized test that POSTs multiple MCP payload shapes and asserts the /mcp endpoint responds with HTTP 202 and no response content.

Possibly related PRs

  • ramimbo/mergework#329: Related changes that previously centralized MCP transport logic and adjusted notification handling.
🚥 Pre-merge checks | ✅ 5 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Bounty Pr Focus ⚠️ Warning PR modifies 58 files beyond stated scope: accounts, activity, bounty, treasury, scripts, templates. Only app/mcp.py and tests/test_api_mcp.py should be modified for MCP notifications fix. Isolate changes to app/mcp.py (+19 lines) and tests/test_api_mcp.py (+159 lines) only; exclude unrelated module modifications.
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title accurately names the changed surface and summarizes the main change: suppressing JSON-RPC notification responses in MCP request handling.
Description check ✅ Passed The description includes a clear summary, evidence of the issue being addressed, validation results, and a related issue reference. All critical sections are completed.
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 description and changed code contain no investment, price, cash-out, payout, or security claims. Documentation guidelines already prohibit such language.

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

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1


ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: 6d9d4333-a996-4619-b0f0-1cb507f9055c

📥 Commits

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

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

Comment thread app/mcp.py
@keilogic keilogic force-pushed the codex/mcp-notifications-656 branch from 135cb58 to a73cd0e Compare June 2, 2026 11:01
Copy link
Copy Markdown
Contributor

@jakerated-r jakerated-r 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 a73cd0e06fbbc0c2dd6898944a6f3a9d6704a29c as a non-author current-head review.

No blocker found in the MCP JSON-RPC notification handling.

Evidence checked:

  • inspected app/mcp.py and confirmed notification detection is based on presence of the id member rather than truthiness, so requests with explicit "id": null still receive normal JSON-RPC responses while true notifications do not;
  • confirmed notification paths now return an empty HTTP 202 response for tools/list, successful tools/call, unknown methods, invalid params, invalid tool names, invalid arguments, and tool errors;
  • inspected tests/test_api_mcp.py and confirmed coverage for notification payloads across success and error paths;
  • verified the earlier CodeRabbit pre-merge warning about 204 is addressed on this head: the code and test now use 202.

Validation run locally:

  • .venv/bin/python -m pytest tests/test_api_mcp.py -q -> 106 passed, 1 warning;
  • .venv/bin/python -m pytest -q -> 679 passed, 1 warning;
  • .venv/bin/python -m ruff check app/mcp.py tests/test_api_mcp.py -> passed;
  • .venv/bin/python -m ruff format --check app/mcp.py tests/test_api_mcp.py -> passed;
  • .venv/bin/python -m mypy app -> passed;
  • git diff --check origin/main...HEAD -> passed;
  • git merge-tree --write-tree origin/main HEAD -> clean tree d2846ec462e8e7e1c0c184becb45604c9274390a.

Hosted quality/readiness/docs/image check is green. GitHub still shows CodeRabbit pending on the latest head at review time, so I am treating that as the only remaining external status caveat rather than a code blocker.

@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.

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.

3 participants