Repository navigation
fix(cli): validate --max-turns as a positive integer #1993
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Closed
Closed
Changes from 6 commits
Commits
Show all changes
7 commits
Select commit
Hold shift + click to select a range
db01673
fix(cli): validate --max-turns as a positive integer
0xfandom 63706a8
fix(cli): bound numeric args to the safe-integer range
0xfandom f76a245
fix(cli): report --max-budget-usd failures as InvalidArgumentError
0xfandom 553fa19
test(cli): cover the --max-budget-usd validation path
0xfandom 4fb9448
fix(cli): bound --max-budget-usd to an enforceable range
0xfandom fcd07e5
fix(cli): reject a decimal budget cap that loses precision
0xfandom 9c03577
fix(cli): apply the budget precision guard to every spelling
0xfandom File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,127 @@ | ||
| import { expect, test } from 'bun:test' | ||
| import { InvalidArgumentError } from '@commander-js/extra-typings' | ||
|
|
||
| import { | ||
| parsePositiveAmountArg, | ||
| parsePositiveIntArg, | ||
| } from './numericArgs.js' | ||
|
|
||
| test('accepts a positive integer', () => { | ||
| expect(parsePositiveIntArg('--max-turns', '3')).toBe(3) | ||
| expect(parsePositiveIntArg('--max-turns', '100')).toBe(100) | ||
| }) | ||
|
|
||
| test('rejects non-numeric input that Number() coerces to NaN', () => { | ||
| // The bug: `.argParser(Number)` turned "abc"/"10x" into NaN, and the cap | ||
| // check `maxTurns && turnCount > maxTurns` is falsy for NaN, so the agent | ||
| // ran with no turn limit at all. | ||
| for (const bad of ['abc', '10x', '', ' ']) { | ||
| expect(() => parsePositiveIntArg('--max-turns', bad)).toThrow( | ||
| InvalidArgumentError, | ||
| ) | ||
| } | ||
| }) | ||
|
|
||
| test('rejects zero and negatives', () => { | ||
| // 0 is falsy (unbounded); a negative cap fires on turn 1 (immediate exit). | ||
| expect(() => parsePositiveIntArg('--max-turns', '0')).toThrow( | ||
| InvalidArgumentError, | ||
| ) | ||
| expect(() => parsePositiveIntArg('--max-turns', '-5')).toThrow( | ||
| InvalidArgumentError, | ||
| ) | ||
| }) | ||
|
|
||
| test('rejects non-integers and Infinity', () => { | ||
| expect(() => parsePositiveIntArg('--max-turns', '2.5')).toThrow( | ||
| InvalidArgumentError, | ||
| ) | ||
| expect(() => parsePositiveIntArg('--max-turns', '1e999')).toThrow( | ||
| InvalidArgumentError, | ||
| ) | ||
| }) | ||
|
|
||
| test('rejects integers beyond the safe range', () => { | ||
| // Number.isInteger accepts these, but they no longer round-trip: | ||
| // 9007199254740993 silently parses as 9007199254740992, and 1e308 is a cap | ||
| // the turn loop can never reach. | ||
| expect(() => parsePositiveIntArg('--max-turns', '9007199254740993')).toThrow( | ||
| InvalidArgumentError, | ||
| ) | ||
| expect(() => parsePositiveIntArg('--max-turns', '1e308')).toThrow( | ||
| InvalidArgumentError, | ||
| ) | ||
| // The boundary itself is still accepted. | ||
| expect(parsePositiveIntArg('--max-turns', '9007199254740991')).toBe( | ||
| Number.MAX_SAFE_INTEGER, | ||
| ) | ||
| }) | ||
|
|
||
| test('names the option in the error message', () => { | ||
| expect(() => parsePositiveIntArg('--max-turns', 'abc')).toThrow( | ||
| '--max-turns must be a positive integer', | ||
| ) | ||
| }) | ||
|
|
||
| test('parsePositiveAmountArg accepts positive amounts including decimals', () => { | ||
| expect(parsePositiveAmountArg('--max-budget-usd', '5')).toBe(5) | ||
| expect(parsePositiveAmountArg('--max-budget-usd', '0.25')).toBe(0.25) | ||
| }) | ||
|
|
||
| test('parsePositiveAmountArg rejects non-numeric, zero and negative values', () => { | ||
| for (const bad of ['abc', '', ' ', '0', '-1']) { | ||
| expect(() => parsePositiveAmountArg('--max-budget-usd', bad)).toThrow( | ||
| InvalidArgumentError, | ||
| ) | ||
| } | ||
| }) | ||
|
|
||
| test('parsePositiveAmountArg rejects an unbounded budget', () => { | ||
| // Number('Infinity') is not NaN and is greater than zero, so a bare isNaN | ||
| // check would accept it as a spending cap that can never be reached. | ||
| expect(() => parsePositiveAmountArg('--max-budget-usd', 'Infinity')).toThrow( | ||
| InvalidArgumentError, | ||
| ) | ||
| expect(() => parsePositiveAmountArg('--max-budget-usd', '1e999')).toThrow( | ||
| InvalidArgumentError, | ||
| ) | ||
| }) | ||
|
|
||
| test('parsePositiveAmountArg rejects finite budgets the guard can never reach', () => { | ||
| // `getTotalCost() >= maxBudgetUsd` never fires for these, so they are the | ||
| // unbounded-budget hole again -- and past MAX_SAFE_INTEGER the enforced cap | ||
| // is not even the number the user supplied. | ||
| expect(() => parsePositiveAmountArg('--max-budget-usd', '1e308')).toThrow( | ||
| InvalidArgumentError, | ||
| ) | ||
| expect(() => | ||
| parsePositiveAmountArg('--max-budget-usd', '9007199254740993'), | ||
| ).toThrow(InvalidArgumentError) | ||
| // Realistic budgets, and the boundary itself, still parse. | ||
| expect(parsePositiveAmountArg('--max-budget-usd', '250.75')).toBe(250.75) | ||
| expect( | ||
| parsePositiveAmountArg('--max-budget-usd', '9007199254740991'), | ||
| ).toBe(Number.MAX_SAFE_INTEGER) | ||
| }) | ||
|
|
||
| test('parsePositiveAmountArg rejects a decimal cap that loses precision', () => { | ||
| // Number('9007199254740990.5') rounds to 9007199254740990, which is under | ||
| // MAX_SAFE_INTEGER, so the earlier bound accepts it -- but the enforced cap | ||
| // is no longer the value the user typed. A double cannot hold this decimal. | ||
| expect(() => | ||
| parsePositiveAmountArg('--max-budget-usd', '9007199254740990.5'), | ||
| ).toThrow(InvalidArgumentError) | ||
| expect(() => | ||
| parsePositiveAmountArg('--max-budget-usd', '1234567890.1234567'), | ||
| ).toThrow(InvalidArgumentError) | ||
| // A decimal within a double's ~15 significant digits is fine, including one | ||
| // whose trailing/leading zeros are not significant. | ||
| expect(parsePositiveAmountArg('--max-budget-usd', '1000.50')).toBe(1000.5) | ||
| expect(parsePositiveAmountArg('--max-budget-usd', '0.000001')).toBe(0.000001) | ||
| }) | ||
|
|
||
| test('parsePositiveAmountArg names the option in the error message', () => { | ||
| expect(() => parsePositiveAmountArg('--max-budget-usd', 'abc')).toThrow( | ||
| '--max-budget-usd must be a positive number greater than 0', | ||
| ) | ||
| }) |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,84 @@ | ||
| import { InvalidArgumentError } from '@commander-js/extra-typings' | ||
|
|
||
| /** | ||
| * Commander argParser for options that must be a positive integer. | ||
| * | ||
| * A bare `.argParser(Number)` accepts anything Number() coerces without | ||
| * complaint: "abc"/"10x" become NaN, "0" stays 0, "1e999" becomes Infinity, | ||
| * "2.5" stays a float, "-5" stays negative. For a cap like --max-turns those | ||
| * all defeat the limit — the enforcement check `maxTurns && turnCount > maxTurns` | ||
| * is falsy for NaN/0 (so the agent runs unbounded) and negatives exit | ||
| * immediately. | ||
| * | ||
| * The bound is Number.isSafeInteger, not Number.isInteger: values past 2^53-1 | ||
| * are integers but no longer round-trip, so `--max-turns 9007199254740993` | ||
| * would silently become 9007199254740992, and 1e308 would pass as a cap that | ||
| * can never be reached. Matches the maxSteps check in runAgent.ts. | ||
| */ | ||
| export function parsePositiveIntArg(name: string, value: string): number { | ||
| const parsed = Number(value) | ||
| if (!Number.isSafeInteger(parsed) || parsed <= 0) { | ||
| throw new InvalidArgumentError(`${name} must be a positive integer`) | ||
| } | ||
| return parsed | ||
| } | ||
|
|
||
| /** | ||
| * Commander argParser for options that must be a positive amount, decimals | ||
| * allowed (e.g. a dollar budget). | ||
| * | ||
| * Number.isFinite rather than a bare !isNaN check: `Number('Infinity')` is not | ||
| * NaN and is greater than zero, so an unbounded budget would otherwise pass | ||
| * validation for a spending cap. | ||
| * | ||
| * Finite is not sufficient on its own though. The cap is enforced as | ||
| * `getTotalCost() >= maxBudgetUsd`, so it has to be a dollar figure real | ||
| * spending can actually reach: 1e308 is finite and positive but the guard can | ||
| * never fire, which is the unbounded-budget hole again with a different | ||
| * literal. Anything past Number.MAX_SAFE_INTEGER also stops round-tripping, | ||
| * so the enforced cap would differ from the one the user typed. Bound at the | ||
| * safe-integer range -- far above any real budget, below where either problem | ||
| * starts. | ||
| * | ||
| * The safe-integer bound alone misses one case: a fractional value just under | ||
| * it can still lose precision. `Number('9007199254740990.5')` rounds to | ||
| * 9007199254740990, which is <= MAX_SAFE_INTEGER yet no longer the value that | ||
| * was typed, so the enforced cap silently shifts. A double round-trips only | ||
| * ~15 significant decimal digits, so a fractional input carrying more than that | ||
| * is rejected. (Whole-number precision is already bounded by MAX_SAFE_INTEGER, | ||
| * which legitimately carries 16 digits, so the digit check is gated on a | ||
| * decimal point to avoid rejecting the boundary integer itself.) | ||
| */ | ||
| export function parsePositiveAmountArg(name: string, value: string): number { | ||
| const parsed = Number(value) | ||
| if ( | ||
| !Number.isFinite(parsed) || | ||
| parsed <= 0 || | ||
| parsed > Number.MAX_SAFE_INTEGER | ||
| ) { | ||
| throw new InvalidArgumentError( | ||
| `${name} must be a positive number greater than 0`, | ||
| ) | ||
| } | ||
| if (value.includes('.') && significantDigitCount(value) > 15) { | ||
| throw new InvalidArgumentError( | ||
| `${name} has more precision than can be represented exactly`, | ||
| ) | ||
| } | ||
| return parsed | ||
| } | ||
|
|
||
| /** | ||
| * Count the significant decimal digits in a numeric string: drop the sign, any | ||
| * exponent suffix, the decimal point, then leading and trailing zeros (neither | ||
| * of which carries precision). "1000.50" -> "10005" -> 5; "9007199254740990.5" | ||
| * -> 17. Used to reject caps a double cannot hold exactly. | ||
| */ | ||
| function significantDigitCount(value: string): number { | ||
| const mantissa = value | ||
| .trim() | ||
| .replace(/^[+-]/, '') | ||
| .replace(/[eE][+-]?\d+$/, '') | ||
| const digitsOnly = mantissa.replace('.', '') | ||
| return digitsOnly.replace(/^0+/, '').replace(/0+$/, '').length | ||
| } | ||
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.