Skip to content

refactor(server): keep ResourceNotFoundError out of top-level mcpserv…

0172696
Select commit
Loading
Failed to load commit list.
Open

fix(server): return -32602 for resource not found (SEP-2164) #2344

refactor(server): keep ResourceNotFoundError out of top-level mcpserv…
0172696
Select commit
Loading
Failed to load commit list.
Claude / Claude Code Review completed Apr 30, 2026 in 26m 25s

Code review found 3 potential issues

Found 5 candidates, confirmed 3. See review comments for details.

Details

Severity Count
🔴 Important 0
🟡 Nit 3
🟣 Pre-existing 0
Severity File:Line Issue
🟡 Nit src/mcp/server/mcpserver/resources/resource_manager.py:94-96 get_resource() docstring missing Raises: section
🟡 Nit src/mcp/server/mcpserver/server.py:350-351 Template-creation INTERNAL_ERROR path never logged server-side
🟡 Nit tests/server/mcpserver/test_server.py:754-782 New tests added inside legacy Test* class (AGENTS.md)

Annotations

Check warning on line 96 in src/mcp/server/mcpserver/resources/resource_manager.py

See this annotation in the file changed.

@claude claude / Claude Code Review

get_resource() docstring missing Raises: section

Nit: per AGENTS.md ("When a public API raises exceptions a caller would reasonably catch, document them in a `Raises:` section"), `get_resource()`'s docstring should mention `ResourceNotFoundError` (and that `ResourceError` may propagate from template creation). The PR updated the sibling `create_resource()` `Raises:` section and added a migration.md entry telling callers to catch these, but left this docstring as a one-liner.

Check warning on line 351 in src/mcp/server/mcpserver/server.py

See this annotation in the file changed.

@claude claude / Claude Code Review

Template-creation INTERNAL_ERROR path never logged server-side

Nit: this branch maps template-handler crashes to `INTERNAL_ERROR` without calling `logger.exception()`, and the lowlevel server's `except MCPError` handler doesn't log either — so the operator gets no server-side traceback for the failure. The parallel `resource.read()` path a few lines below in `read_resource()` does `logger.exception(...)` before wrapping, and AGENTS.md asks for `logger.exception()` when catching exceptions; consider adding the same in templates.py:135 before the `ResourceErr

Check warning on line 782 in tests/server/mcpserver/test_server.py

See this annotation in the file changed.

@claude claude / Claude Code Review

New tests added inside legacy Test* class (AGENTS.md)

Nit: per AGENTS.md ("Do not use `Test` prefixed classes — write plain top-level `test_*` functions… do NOT follow that pattern for new tests even when adding to such a file"), the two new tests `test_read_resource_template_error` and `test_read_resource_template_not_found` should be top-level `async def test_…` functions rather than methods on the legacy `TestServerResources` class. They don't use `self` or any class fixtures, so they can be moved out as-is (just add the `@pytest.mark.anyio` dec