Skip to content

[mcp] fix: classify invalid visible gpu ids - #146

Merged
Wangmerlyn merged 1 commit into
mainfrom
codex/jsonrpc-gpu-ids-invalid-params
Jun 29, 2026
Merged

[mcp] fix: classify invalid visible gpu ids#146
Wangmerlyn merged 1 commit into
mainfrom
codex/jsonrpc-gpu-ids-invalid-params

Conversation

@Wangmerlyn

@Wangmerlyn Wangmerlyn commented Jun 29, 2026

Copy link
Copy Markdown
Owner

Summary

  • Add a specific visible GPU selection exception for public gpu_ids mistakes.
  • Map out-of-range visible gpu_ids in service startup to invalid params while preserving internal errors for arbitrary controller failures.
  • Document the direct JSON-RPC and MCP tool error contracts.

Test Plan

  • RED focused tests first failed with direct JSON-RPC returning -32603 and MCP tools/call returning an internal-error envelope.
  • PYTHONPATH=$PWD/src pytest tests/mcp/test_server.py::test_jsonrpc_start_keep_out_of_range_gpu_ids_returns_invalid_params tests/mcp/test_server.py::test_mcp_tools_call_out_of_range_gpu_ids_returns_tool_error tests/mcp/test_server.py::test_jsonrpc_start_keep_runtime_value_error_remains_internal_error -q
  • PYTHONPATH=$PWD/src pytest tests/mcp -q
  • PYTHONPATH=$PWD/src pytest tests -q
  • PYTHONPATH=$PWD/src mkdocs build
  • pre-commit run --all-files
  • git diff --check

Local Review

  • Local subagent code review found no critical or important issues; the minor plan-command mismatch was fixed before this PR.

Summary by CodeRabbit

  • New Features

    • Improved gpu_ids validation so out-of-range visible GPU selections are reported as input errors instead of generic server failures.
    • JSON-RPC requests now return Invalid params, and MCP tool calls now return isError=true for these cases.
  • Bug Fixes

    • Added clearer error handling for invalid GPU selections during startup.
    • Covered the new behavior with tests for both JSON-RPC and MCP flows.

@gemini-code-assist

Copy link
Copy Markdown
Contributor

Warning

You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again!

@coderabbitai

coderabbitai Bot commented Jun 29, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: a9db890c-7725-4e3d-8180-707625ec2a96

📥 Commits

Reviewing files that changed from the base of the PR and between 13f07f9 and 7351a50.

📒 Files selected for processing (6)
  • AGENTS.md
  • docs/guides/mcp.md
  • docs/plans/jsonrpc-gpu-ids-invalid-params.md
  • src/keep_gpu/global_gpu_controller/global_gpu_controller.py
  • src/keep_gpu/mcp/server.py
  • tests/mcp/test_server.py

📝 Walkthrough

Walkthrough

Adds InvalidVisibleGPUSelectionError (a ValueError subclass) to the GPU controller, raised when explicit gpu_ids reference ordinals outside the visible device count. The MCP server catches this and maps it to SessionInputError, producing JSON-RPC -32602 or MCP isError=true. Two tests and documentation updates accompany the change.

Changes

GPU IDs Invalid Params Reclassification

Layer / File(s) Summary
InvalidVisibleGPUSelectionError and controller validation
src/keep_gpu/global_gpu_controller/global_gpu_controller.py
Defines InvalidVisibleGPUSelectionError(ValueError) and updates _resolve_visible_gpu_ids and GlobalGPUController.__init__ to raise it instead of generic ValueError for out-of-range visible gpu_ids.
MCP server error mapping
src/keep_gpu/mcp/server.py
Imports InvalidVisibleGPUSelectionError and adds an explicit catch in start_keep that re-raises it as SessionInputError, routing to JSON-RPC -32602 or MCP isError=true.
Tests and docs
tests/mcp/test_server.py, AGENTS.md, docs/guides/mcp.md, docs/plans/jsonrpc-gpu-ids-invalid-params.md
Adds JSON-RPC and MCP tools/call tests asserting the new error codes for out-of-range gpu_ids; updates agent guidelines, MCP guide, and adds the implementation plan doc.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Possibly related PRs

  • Wangmerlyn/KeepGPU#59: Initial implementation of KeepGPUServer.start_keep in src/keep_gpu/mcp/server.py, directly modified by this PR to add the new error-mapping branch.
  • Wangmerlyn/KeepGPU#137: Also modifies the start_keep error-handling path to distinguish internal vs. client-facing errors, the same pattern extended here for InvalidVisibleGPUSelectionError.

Poem

🐇 When GPU ordinals stray too far,
No longer blamed on an internal scar.
-32602 says "your params are wrong,"
isError=true carries the song.
The rabbit validates, clear and precise —
Bad input gets its own device! 🎯

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 10.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly summarizes the main change: classifying invalid visible GPU IDs as a distinct error case in MCP.
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.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch codex/jsonrpc-gpu-ids-invalid-params

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@Wangmerlyn
Wangmerlyn merged commit e1edcf8 into main Jun 29, 2026
5 checks passed
@Wangmerlyn
Wangmerlyn deleted the codex/jsonrpc-gpu-ids-invalid-params branch June 29, 2026 02:47
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.

1 participant