[validation,mcp] fix: reject oversized numeric session inputs - #105
Conversation
|
Warning You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again! |
|
Warning Review limit reached
More reviews will be available in 51 minutes and 11 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based credits. 🚦 How do rate limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughAdds ChangesFinite Numeric Validation for Interval and VRAM
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (3)
docs/plans/mcp-finite-numeric-validation.md (1)
109-119: 📐 Maintainability & Code Quality | 🔵 TrivialTask 4 file list omits
docs/guides/python.md.The plan's Task 4 lists documentation files to modify but does not include
docs/guides/python.md, which was updated with the newintervalandvram_to_keepbounds documentation. Add it to the file list so the plan accurately reflects all touched docs.- Modify: `docs/guides/cli.md` - Modify: `docs/guides/mcp.md` + - Modify: `docs/guides/python.md` - Modify: `docs/reference/cli.md`🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/plans/mcp-finite-numeric-validation.md` around lines 109 - 119, The Task 4 documentation file list in the plan is missing docs/guides/python.md, which was also updated with the new interval and vram_to_keep bounds details. Update the file list in the plan’s Task 4 section to include docs/guides/python.md alongside the other documentation files, keeping the list aligned with the touched docs.tests/mcp/test_server.py (1)
528-528: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMake the schema type assertion order-insensitive.
This locks the test to one JSON Schema serialization order even though
["string", "integer"]and["integer", "string"]are equivalent. Prefer asserting the set so harmless schema construction changes do not fail the test.♻️ Proposed fix
- assert start_schema["properties"]["vram"]["type"] == ["string", "integer"] + assert set(start_schema["properties"]["vram"]["type"]) == {"string", "integer"}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/mcp/test_server.py` at line 528, The schema assertion in the test for start_schema["properties"]["vram"]["type"] is too order-dependent; update the check to be order-insensitive so equivalent JSON Schema arrays in any serialization order pass. Locate the assertion in the test around the start_schema validation and compare the type values as an unordered collection rather than a fixed list.tests/test_cli_service_commands.py (1)
550-550: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDrop the
result.exceptionassertion from this test.The observable CLI contract here is the exit code and rendered message. Asserting on
CliRunner.invoke()'s captured exception couples the test to Click/Typer internals and can fail on harmless error-handling refactors.♻️ Proposed fix
assert result.exit_code == 1 - assert result.exception is not None assert message in result.output🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_cli_service_commands.py` at line 550, The test is too tightly coupled to Click/Typer internals by asserting on result.exception; remove that assertion and keep the test focused on the observable CLI contract. Update the relevant CLI test in the test case around CliRunner.invoke() so it only checks the exit code and the rendered output/message, without relying on exception capture.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@docs/plans/mcp-finite-numeric-validation.md`:
- Around line 109-119: The Task 4 documentation file list in the plan is missing
docs/guides/python.md, which was also updated with the new interval and
vram_to_keep bounds details. Update the file list in the plan’s Task 4 section
to include docs/guides/python.md alongside the other documentation files,
keeping the list aligned with the touched docs.
In `@tests/mcp/test_server.py`:
- Line 528: The schema assertion in the test for
start_schema["properties"]["vram"]["type"] is too order-dependent; update the
check to be order-insensitive so equivalent JSON Schema arrays in any
serialization order pass. Locate the assertion in the test around the
start_schema validation and compare the type values as an unordered collection
rather than a fixed list.
In `@tests/test_cli_service_commands.py`:
- Line 550: The test is too tightly coupled to Click/Typer internals by
asserting on result.exception; remove that assertion and keep the test focused
on the observable CLI contract. Update the relevant CLI test in the test case
around CliRunner.invoke() so it only checks the exit code and the rendered
output/message, without relying on exception capture.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 835af7a1-831b-467c-a345-c0bd9777602e
📒 Files selected for processing (20)
AGENTS.mdREADME.mddocs/concepts/architecture.mddocs/getting-started.mddocs/guides/cli.mddocs/guides/mcp.mddocs/guides/python.mddocs/plans/mcp-finite-numeric-validation.mddocs/reference/api.mddocs/reference/cli.mdsrc/keep_gpu/cli.pysrc/keep_gpu/mcp/server.pysrc/keep_gpu/utilities/humanized_input.pysrc/keep_gpu/utilities/session_config.pytests/global_controller/test_contract.pytests/mcp/test_http_api.pytests/mcp/test_server.pytests/test_cli_service_commands.pytests/utilities/test_humanized_input.pytests/utilities/test_session_config.py
|
Resolved CodeRabbit nitpicks in
Post-fix verification:
|
1 participant
Summary
Verification
PYTHONPATH=$PWD/src pytest tests/utilities/test_session_config.py tests/utilities/test_humanized_input.py tests/mcp/test_server.py tests/mcp/test_http_api.py tests/test_cli_service_commands.py tests/global_controller/test_contract.py -q-> 259 passedPYTHONPATH=$PWD/src pytest tests -q-> 360 passed, 11 skippedPYTHONPATH=$PWD/src mkdocs build-> passed with known Material/MkDocs and unlisted-plan warningspre-commit run --all-files-> passedgit diff --check-> passedLocal Review
Summary by CodeRabbit
New Features
--intervalvalues now have an explicit maximum, and VRAM values are capped at 1 PiB equivalent.Bug Fixes
Documentation