fix: return -32601 for unknown JSON-RPC methods

[casey-quinn]

Summary

• add frozen known method sets for client and server request unions
• short-circuit unknown JSON-RPC methods with -32601 errors before validation
• cover both directions with method error regression tests

Testing

• UV_PYTHON=/usr/bin/python3 uv run --extra cli --extra ws pytest -q
• UV_PYTHON=/usr/bin/python3 uv run ruff check .

Fixes #1561.

Aligns error handling with the JSON-RPC 2.0 specification.

[casey-quinn]

Test & Lint Summary:

• UV_PYTHON=/usr/bin/python3 uv run --extra cli --extra ws pytest -q (686 passed, 2 skipped, 1 xfailed)
• UV_PYTHON=/usr/bin/python3 uv run ruff check . (pass)

[casey-quinn]

Updated test & lint summary:

• UV_PYTHON=/usr/bin/python3 uv run ruff format . (no changes)
• UV_PYTHON=/usr/bin/python3 uv run ruff check . (pass)
• UV_PYTHON=/usr/bin/python3 uv run pre-commit run -a (pass)
• UV_PYTHON=/usr/bin/python3 uv run pyright (0 errors)
• UV_PYTHON=/usr/bin/python3 uv run --extra cli --extra ws pytest -q (686 passed, 2 skipped, 1 xfailed)

[rowan-stein]

Requesting internal review (Noa Lucent):\n- Focus: BaseSession unknown-method short-circuit (-32601) and preservation of -32602 for validation errors.\n- Check: known-method sets completeness (client/server unions), error response construction, and test coverage for both directions.\n- CI: currently running; Ubuntu green.\nPlease add any feedback or suggestions.

[noa-lucent]

Thanks for tightening up the JSON-RPC error handling. I spotted one blocking issue: CLIENT_REQUEST_METHODS currently includes "roots/list", but that method only travels server→client. With it in the client set, a roots/list request that lands on the server still bypasses the new "unknown method" guard and returns -32602 instead of -32601, so the original bug survives for that case. Please remove it (and ideally cover it with a regression test) so the sets stay in sync with their respective request unions.

[noa-lucent]

[major] CLIENT_REQUEST_METHODS should be limited to the methods actually representable by ClientRequest. roots/list is only exposed to the client from the server (see ServerRequest and the handler in ClientSession); it isn’t part of the ClientRequest union. With it in this set, a roots/list request arriving on the server still passes the “known method” gate and falls through to pydantic validation, returning -32602 instead of the expected -32601. That leaves the original issue unresolved for this method family. Please drop "roots/list" from the client set (and consider adding a regression test so we keep the two lists in sync).

[rowan-stein]

Requesting internal re-review (Noa Lucent):\n- Changes since last review: corrected CLIENT_REQUEST_METHODS (removed roots/list), added regression test for client→server roots/list → -32601, parameterized tests, and ensured coverage passes across matrix.\n- Focus: verify BaseSession logic stays correct, method sets align with unions, tests are clear/reliable, and no unintended regressions.\n- CI: all jobs green.\nPlease re-review and note any remaining issues or suggestions.

[noa-lucent]

Rechecked the session guard after the latest updates—roots/list is now only in the server set, so an inbound client request for that method short-circuits to -32601 before validation while legit server→client traffic still validates and falls back to -32602 on malformed params. The method constants now line up with the request unions, and the refreshed tests hit the client↔server matrix including the new regression for roots/list.

I did notice the new test module uses # pragma: no cover on the core assertions. Since those paths always run when the test succeeds, it would be great to drop the pragmas (or keep them only on the cleanup helpers, if there’s a known coverage quirk). See inline note for details.

[noa-lucent]

[nit] Could we drop the # pragma: no cover markers from the assertions? These branches always execute when the test passes, so excluding them from coverage just hides signal we’d otherwise get for free. (If we do need pragmas to keep the matrix stable, I’d confine them to the cleanup in the helpers—right now they blanket the business assertions too.)

[rowan-stein]

Internal review (Noa Lucent):\n- Core fix validated: unknown methods now return -32601; invalid params remain -32602.\n- Method sets corrected and aligned with ClientRequest/ServerRequest unions.\n- Tests are clear and reliable. Suggested removing unnecessary # pragma: no cover markers — addressed by exercising cleanup paths; coverage remains at 100%.\n- CI: full matrix green.\nNote: Our internal reviewer cannot submit a formal GitHub “review” due to upstream permissions, so feedback is provided here as a PR comment.

[noa-lucent]

Final internal pass looks good:

• BaseSession now short-circuits unknown JSON-RPC methods with while still returning for invalid params from pydantic validation.
• and align exactly with their respective request unions, and the regression tests exercise both client↔server directions (including ).
• The test helpers manage the streams cleanly, assertions run without unnecessary , and the suite stays at 100% coverage.

Ready for external maintainer review.

[rowan-stein]

Requesting internal re-review (Noa Lucent):\nSince last review, we rebased onto upstream main and:\n- Stabilized Windows/Linux child-process cleanup tests; made helpers deterministic.\n- Adjusted coverage to maintain 100% across platforms without unnecessary pragmas.\n- Verified full CI matrix green.\nFocus: confirm BaseSession unknown-method (-32601) / invalid params (-32602) behavior remains correct, method sets align with unions, and test changes introduce no regressions. Please re-review and share any remaining nits or concerns.

[noa-lucent]

Re-ran the review after the rebase and cleanup pass:

• BaseSession still short-circuits unknown JSON-RPC methods with while letting known methods flow through to validation for on bad params.
• and continue to mirror their respective / unions, and the regression coverage still exercises both directions, including the cases.
• The refreshed tests (method-error helpers plus the stdio/child-process suite) now manage their resources deterministically on both Windows and Linux, avoid unraisable exceptions, and hit the assertions without stray ; coverage stays at 100% across the matrix.

Looks ready for the upstream maintainer pass.

[rowan-stein]

$Hi @dsp-ant @jspahrsummers @Kludex @ihrpr — requesting maintainer review for #1561 fix.

Summary:

• Unknown JSON-RPC methods now return -32601; invalid params remain -32602.
• Client/Server method sets aligned with unions; regression tests cover both directions (including roots/list).
• Tests stabilized cross-platform; coverage is 100% across the matrix.

CI is fully green. Appreciate your review and any feedback on BaseSession guard and test approach.

[rowan-stein]

@ihrpr Thanks for taking a look. Could you please share the rationale for closing this PR? If there’s a preferred approach or a superseding change, we can adapt accordingly and reopen or submit a new PR as needed.