Skip to content

Validate scale numeric options before actions run#742

Closed
rissrice2105-agent wants to merge 1 commit into
profullstack:masterfrom
rissrice2105-agent:codex/sh1pt-next-work
Closed

Validate scale numeric options before actions run#742
rissrice2105-agent wants to merge 1 commit into
profullstack:masterfrom
rissrice2105-agent:codex/sh1pt-next-work

Conversation

@rissrice2105-agent

Copy link
Copy Markdown
Contributor

Fixes #729.

Changes

  • require safe integer values for scale counts, percentages, cooldown, and TTL
  • require a positive finite number for --max-hourly-price
  • reject NaN, Infinity, fractions, non-positive counts, and unsafe integers during Commander parsing
  • add parser regression tests

Verification

  • corepack pnpm vitest run packages/cli/src/commands/scale.test.ts (20 passed)
  • corepack pnpm --filter @profullstack/sh1pt typecheck attempted; it still fails on existing workspace/module-resolution errors unrelated to this PR

@rissrice2105-agent

Copy link
Copy Markdown
Contributor Author

Closing this PR because #730 predates it and already implements the same fix for #729. This duplicate should not be reviewed or paid.

@greptile-apps

greptile-apps Bot commented Jun 14, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

This PR adds parse-time validation for numeric CLI options in the scale command by introducing three exported parser functions (parsePositiveSafeInteger, parseNonNegativeSafeInteger, parsePositiveFiniteNumber) and wiring them into Commander's option definitions, replacing the bare Number coercer.

  • Three new parsers cover the full range of numeric option types (positive integers, non-negative integers, positive finite floats) and reject NaN, Infinity, fractions, non-positive values, and unsafe integers via InvalidArgumentError, giving users immediate feedback from Commander.
  • --instances, --max, --cooldown, --ttl, and --percent now reject bad values at parse time; --max-hourly-price gets a proper finite-number check; action-level guards for --min < 0 and --max < 1 are now unreachable dead code.
  • The --target-cpu upper bound (> 100) is still enforced only in the action handler via process.exit(1), inconsistent with the parse-time style adopted everywhere else in this PR.

Confidence Score: 4/5

Safe to merge; the parsers correctly block all targeted bad inputs before actions run, and no existing valid inputs are broken.

The core logic is correct and the test coverage is solid. Two action-level guards (min < 0, max < 1) are now dead code that could be cleaned up, and --target-cpu still relies on the action handler for its upper-bound check rather than the new parse-time style — both are minor quality gaps rather than functional defects.

The auto-scale action handler in scale.ts (lines 466–481) has checks that are now either unreachable or inconsistent with the parse-time validation pattern introduced here.

Important Files Changed

Filename Overview
packages/cli/src/commands/scale.ts Adds three exported parser functions (parsePositiveSafeInteger, parseNonNegativeSafeInteger, parsePositiveFiniteNumber) and wires them into Commander option definitions; existing action-level guards for min < 0 and max < 1 are now dead code, and --target-cpu upper-bound is still only enforced at action level.
packages/cli/src/commands/scale.test.ts Adds a describe block with three parser regression tests; covers NaN, fractions, negatives, Infinity, and unsafe integers for parsePositiveSafeInteger, but missing the unsafe-integer boundary test for parseNonNegativeSafeInteger.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[CLI argument string] --> B{Which option?}
    B --> |--instances / --max / --cooldown / --ttl / --percent| C[parsePositiveSafeInteger]
    B --> |--min| D[parseNonNegativeSafeInteger]
    B --> |--max-hourly-price| E[parsePositiveFiniteNumber]
    B --> |--target-cpu| F[parsePositiveSafeInteger only lower bound ge 1]
    C --> G{Number.isSafeInteger AND value ge 1?}
    D --> H{Number.isSafeInteger AND value ge 0?}
    E --> I{Number.isFinite AND value gt 0?}
    F --> J{Number.isSafeInteger AND value ge 1?}
    G -- No --> K[InvalidArgumentError - Commander exits immediately]
    H -- No --> K
    I -- No --> K
    J -- No --> K
    G -- Yes --> L[Action handler runs]
    H -- Yes --> L
    I -- Yes --> L
    J -- Yes --> M{targetCpu gt 100?}
    M -- Yes --> N[process.exit 1 - action-level check]
    M -- No --> L
    L --> O[Command executes]
Loading

Comments Outside Diff (1)

  1. packages/cli/src/commands/scale.ts, line 466-473 (link)

    P2 Dead action-level guards after parser enforcement

    The min < 0 and max < 1 checks at lines 466–473 are now unreachable because parseNonNegativeSafeInteger and parsePositiveSafeInteger throw InvalidArgumentError before the action runs. Commander exits immediately on a parse-time error, so these process.exit(1) paths can never fire.

    Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

Reviews (1): Last reviewed commit: "Validate scale numeric options" | Re-trigger Greptile

.option('--cooldown <seconds>', 'minimum time between scale events', Number, 300)
.option('--min <n>', 'minimum instances', parseNonNegativeSafeInteger, 1)
.option('--max <n>', 'maximum instances', parsePositiveSafeInteger, 10)
.option('--target-cpu <percent>', 'target CPU utilization to maintain', parsePositiveSafeInteger, 70)

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.

P2 --target-cpu upper-bound check inconsistency

parsePositiveSafeInteger accepts any integer >= 1, so values like --target-cpu 200 pass parse-time validation and are only caught in the action handler (line 478) with a process.exit(1) message. All other options in this PR now surface validation errors early via Commander's InvalidArgumentError. Consider using a dedicated parser for percent values (e.g. one that rejects anything outside 1–100) for consistent user experience.

Comment on lines +39 to +44
}
});

it('allows zero only for non-negative safe integer options', () => {
expect(parseNonNegativeSafeInteger('0')).toBe(0);
expect(parseNonNegativeSafeInteger('4')).toBe(4);

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.

P2 Missing unsafe-integer test for parseNonNegativeSafeInteger

The parsePositiveSafeInteger suite explicitly tests '9007199254740992' (2^53, the first unsafe integer), but the parseNonNegativeSafeInteger suite omits that boundary case. Adding it would ensure the upper bound of Number.isSafeInteger is uniformly exercised across all three parsers.

Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

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.

scale numeric options accept NaN, Infinity, and fractional counts

1 participant