Skip to content

[cli] fix: reject non-canonical numeric inputs - #282

Merged
Wangmerlyn merged 2 commits into
mainfrom
codex/cli-strict-numeric-parsing
Jul 2, 2026
Merged

[cli] fix: reject non-canonical numeric inputs#282
Wangmerlyn merged 2 commits into
mainfrom
codex/cli-strict-numeric-parsing

Conversation

@Wangmerlyn

@Wangmerlyn Wangmerlyn commented Jul 2, 2026

Copy link
Copy Markdown
Owner

Summary

  • reject non-canonical CLI numeric spellings before daemon auto-start, RPC, or blocking runner side effects
  • tighten --gpu-ids, --interval, --busy-threshold, and legacy --threshold parsing while preserving valid legacy behavior
  • document the plain-ASCII CLI numeric-token contract in AGENTS and CLI docs

Verification

  • Red tests failed before fixes for permissive int()/float() parsing, signed-zero GPU IDs, and legacy --threshold numeric-token bypasses
  • PYTHONPATH=src pytest tests/test_cli_thresholds.py tests/test_cli_service_commands.py tests/utilities/test_endpoint_validation.py -q -> 357 passed
  • pre-commit run --all-files --show-diff-on-failure -> passed
  • mkdocs build --strict -> exit 0, known Material/MkDocs 2.0 warning only
  • PYTHONPATH=src pytest tests -q -> 1104 passed, 11 skipped

Local review

  • Local reviewer found signed-zero GPU ID normalization; fixed and re-reviewed.
  • Local reviewer found hidden --threshold bypass; fixed and re-reviewed.
  • Final local subagent review: no Critical, Important, or Minor issues; ready to merge.

Summary by CodeRabbit

  • Bug Fixes

    • Tightened CLI numeric input handling so only plain ASCII digit formats are accepted.
    • Rejected underscore-separated, full-width, and other non-canonical number spellings in startup and blocking commands.
    • Improved error messages for invalid numeric options, including GPU IDs, intervals, busy thresholds, and legacy threshold input.
  • Documentation

    • Clarified CLI guidance to specify ASCII-only numeric formatting for supported options and thresholds.

@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 Jul 2, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

Warning

Review limit reached

@Wangmerlyn, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 42 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

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

How do review 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 refer docs for additional details.

Review details
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: cc75d0fe-7864-4862-a5c6-b757a581fcac

📥 Commits

Reviewing files that changed from the base of the PR and between 7c4abd5 and 3461fb9.

📒 Files selected for processing (6)
  • AGENTS.md
  • docs/guides/cli.md
  • docs/reference/cli.md
  • src/keep_gpu/cli.py
  • tests/test_cli_service_commands.py
  • tests/test_cli_thresholds.py
📝 Walkthrough

Walkthrough

This PR tightens CLI numeric input validation in KeepGPU to require plain ASCII digit tokens, rejecting non-canonical spellings like underscore-separated numbers (1_000) and full-width digits. Changes span cli.py validation logic, documentation, and tests.

Changes

ASCII-only numeric CLI validation

Layer / File(s) Summary
Regex token validation helpers
src/keep_gpu/cli.py
Adds import re, compiled regex patterns _CLI_INTEGER_TOKEN_RE/_CLI_NUMBER_TOKEN_RE, and helper functions to classify numeric-like tokens and detect invalid gpu-id tokens (including rejecting "-0").
Apply validation to gpu-ids, interval, busy-threshold, legacy threshold
src/keep_gpu/cli.py
Reworks legacy --threshold parsing, tightens _parse_gpu_ids, _validate_cli_interval, _validate_cli_busy_threshold to require regex matches, and wires legacy threshold validation into the blocking-mode main callback.
Tests for non-canonical numeric token rejection
tests/test_cli_service_commands.py, tests/test_cli_thresholds.py
Extends test matrices with parametrized cases for underscore-separated and fullwidth digit tokens across gpu-ids, interval, busy-threshold, and legacy threshold; adds a new blocking-mode rejection test.
Documentation of ASCII numeric requirement
AGENTS.md, docs/guides/cli.md, docs/reference/cli.md
Updates guidance and option tables to document that numeric CLI tokens must be plain ASCII digit spellings.

Estimated code review effort: 2 (Simple) | ~15 minutes

Sequence Diagram(s)

sequenceDiagram
  participant User
  participant CLI as cli.main
  participant Legacy as _apply_legacy_threshold
  participant Validators as Validators (interval/busy-threshold/vram)

  User->>CLI: keep-gpu --threshold value
  CLI->>Legacy: _apply_legacy_threshold(value)
  Legacy->>Legacy: classify token via regex
  Legacy-->>CLI: legacy_vram / legacy_busy_threshold
  CLI->>Validators: validate derived values
  Validators-->>CLI: BadParameter or validated value
  CLI-->>User: proceed to _run_blocking or error
Loading

Possibly related PRs

  • Wangmerlyn/KeepGPU#79: Both PRs modify busy-threshold validation logic in the CLI, one tightening numeric token formats and the other constraining the valid value range.
🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly matches the main change: rejecting non-canonical numeric CLI inputs.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
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 unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch codex/cli-strict-numeric-parsing

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.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Actionable comments posted: 1

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

Inline comments:
In `@src/keep_gpu/cli.py`:
- Around line 58-61: The CLI numeric token rules are inconsistent because
_CLI_INTEGER_TOKEN_RE and _CLI_NUMBER_TOKEN_RE allow a leading + for flags like
--interval, --busy-threshold, and legacy --threshold, but the --gpu-ids parsing
path rejects it. Update the gpu-id parsing logic in keep_gpu CLI helpers to use
the same canonical token rules as the other numeric flags, or explicitly
document and add tests for the intentional exception so the behavior is
consistent and discoverable.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 5be7110e-3024-4335-be56-e17b4ddfa028

📥 Commits

Reviewing files that changed from the base of the PR and between dbc1459 and 7c4abd5.

📒 Files selected for processing (6)
  • AGENTS.md
  • docs/guides/cli.md
  • docs/reference/cli.md
  • src/keep_gpu/cli.py
  • tests/test_cli_service_commands.py
  • tests/test_cli_thresholds.py

Comment thread src/keep_gpu/cli.py Outdated
@Wangmerlyn
Wangmerlyn merged commit e1c2556 into main Jul 2, 2026
6 checks passed
@Wangmerlyn
Wangmerlyn deleted the codex/cli-strict-numeric-parsing branch July 2, 2026 04:12
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