Skip to content

validate --max-turns and --temperature flags to prevent silent NaN#130

Open
zjshen14 wants to merge 1 commit into
mainfrom
fix/issue-116
Open

validate --max-turns and --temperature flags to prevent silent NaN#130
zjshen14 wants to merge 1 commit into
mainfrom
fix/issue-116

Conversation

@zjshen14
Copy link
Copy Markdown
Owner

Summary

  • Replaces bare parseInt/parseFloat coercers on --max-turns and --temperature with validator functions that throw InvalidArgumentError on bad input
  • --max-turns abc or --max-turns 0 now exits 1 with "--max-turns must be a positive integer" instead of silently passing NaN (which disabled the safety guard in agent.ts)
  • --temperature notanumber or out-of-range values now exits 1 with "--temperature must be between 0 and 2" instead of forwarding NaN to the provider
  • Pattern matches existing --sandbox / --provider validation already in the codebase

Closes #116

Test plan

  • npm run typecheck && npm run lint && npm run format:check && npm test — all pass
  • New smoke tests cover: non-numeric --max-turns, zero --max-turns, non-numeric --temperature, out-of-range --temperature (all run against the built dist via npm run build)
  • Valid values (--max-turns 5, --temperature 0.7) still pass through unchanged

Generated by Claude Code

…o agent/provider

Bare parseInt/parseFloat coercers silently produce NaN for non-numeric
inputs. NaN breaks the turns > maxTurns safety guard (always false) and
is passed raw to the LLM provider for temperature. Added parseTurns and
parseTemperature validators that throw commander's InvalidArgumentError
with a clear message, matching the existing --sandbox / --provider
validation pattern.

Closes #116
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@zjshen14
Copy link
Copy Markdown
Owner Author

Good fix — InvalidArgumentError gives commander the clean exit-1 formatting that parseInt/parseFloat can't, and the rejection of 0 for --max-turns is the right call.

Two things to check before landing:

Coverage gap: chat command --temperature
The diff updates --temperature on the run subcommand but I don't see a corresponding change on the chat command definition. If chat also accepts --temperature (and the current code uses bare parseFloat there), the fix is incomplete — a user could still pass --temperature abc to chat and get a silent NaN. Worth confirming whether chat has that option and, if so, applying parseTemperature there too.

Stray comma in src/cli/run.smoke.test.ts
After the third added test case ("run --temperature with non-numeric value exits 1..."), the closing line reads:

  }),

with a trailing comma instead of a semicolon. TypeScript parses this as a comma expression (the return value of it() is discarded, so Vitest still registers the test), but it reads like a mistake and a linter configured with no-comma-dangle in statement position would flag it.

Recommendation: confirm chat --temperature coverage, fix the comma, then merge.


Generated by Claude Code

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.

Numeric CLI flags accept garbage values without validation

2 participants