Validate deploy numeric options before actions run#744
Conversation
Greptile SummaryReplaces the bare
Confidence Score: 4/5Safe to merge; the validation logic is correct for all specified cases and the stub actions are unaffected. Both parsers correctly guard against NaN, Infinity, zero, negatives, fractions, and unsafe integers. The one open question is whether hex (0x10) and scientific-notation (1e2) inputs should also be rejected — they currently pass through to the action handler with unexpected numeric values. The commands are stubs today, so there is no immediate data-loss risk, but the gap is worth addressing before these commands start making real API calls. The two new parser functions in packages/cli/src/commands/deploy.ts around lines 4–18 deserve a second look for the hex/scientific-notation edge case. Important Files Changed
Sequence DiagramsequenceDiagram
participant User as CLI User
participant Commander as Commander.js
participant Parser as parsePositive*
participant Action as .action() handler
User->>Commander: deploy quote --cpu 4 --memory 8.5
Commander->>Parser: parsePositiveSafeInteger("4")
Parser-->>Commander: 4
Commander->>Parser: parsePositiveFiniteNumber("8.5")
Parser-->>Commander: 8.5
Commander->>Action: "opts = { cpu: 4, memory: 8.5, ... }"
Action-->>User: [stub] deploy quote ...
User->>Commander: deploy provision --cpu abc
Commander->>Parser: parsePositiveSafeInteger("abc")
Parser-->>Commander: throws InvalidArgumentError
Commander-->>User: "error: option '--cpu <n>' argument 'abc' is invalid. must be a positive safe integer"
Reviews (1): Last reviewed commit: "Validate deploy numeric options" | Re-trigger Greptile |
| const parsed = Number(value); | ||
| if (!Number.isSafeInteger(parsed) || parsed < 1) { |
There was a problem hiding this comment.
Hex and scientific-notation strings silently accepted
Number('0x10') → 16 and Number('1e2') → 100 both satisfy the safe-integer and finite-positive checks, so --cpu 0x10 or --gpu-count 1e2 pass validation and are forwarded to the action. A user who fat-fingers 0x or uses scientific notation by accident would get an unexpected count rather than a validation error. Adding an explicit check (/^\d+$/ for the integer parser, /^\d+(\.\d+)?$/ for the float parser) before calling Number() would close this gap without affecting normal decimal input.
|
CI is green for PR #744. Verification:
uGig invoice evidence will be sent after the duplicate-invoice retry window expires. |
|
🤖 Auto-rebase: The branch was rebased successfully locally but could not be pushed to the fork. Please enable 'Allow edits from maintainers' in the PR settings, or rebase manually: |
2 similar comments
|
🤖 Auto-rebase: The branch was rebased successfully locally but could not be pushed to the fork. Please enable 'Allow edits from maintainers' in the PR settings, or rebase manually: |
|
🤖 Auto-rebase: The branch was rebased successfully locally but could not be pushed to the fork. Please enable 'Allow edits from maintainers' in the PR settings, or rebase manually: |
Fixes #743.
Changes
Verification
corepack pnpm vitest run packages/cli/src/commands/deploy.test.ts(14 passed)corepack pnpm --filter @profullstack/sh1pt typecheckattempted; it still fails on existing workspace/module-resolution errors unrelated to this PR