Skip to content

fix(cli): reject non-finite float flag values - #200

Open
feiiiiii5 wants to merge 2 commits into
openai:mainfrom
feiiiiii5:fix/reject-non-finite-float-flags
Open

feiiiiii5 wants to merge 2 commits into
openai:mainfrom
feiiiiii5:fix/reject-non-finite-float-flags

Conversation

@feiiiiii5

Copy link
Copy Markdown

What happens

--temperature inf (also -inf, nan, infinity, in any case) is accepted by flag parsing, and the multipart encoder then writes the literal bytes +Inf or NaN into the part body. The request is sent, the API rejects it, and the CLI's own failure is only the server's confusing message. Piped YAML reaches the same encoder without ever going through a typed flag, and JSON-body endpoints fail late during marshaling with json: unsupported value: +Inf after flag parsing had already accepted the value.

Measured on 4b75e9b against a local capture listener, both with exit code 0:

$ openai audio:transcriptions create --model whisper-1 --file dummy.mp3 \
    --temperature inf --base-url http://127.0.0.1:41789/v1
Content-Disposition: form-data; name="temperature"

+Inf

$ printf 'temperature: .inf\n' | openai audio:transcriptions create \
    --model whisper-1 --file dummy.mp3 --base-url http://127.0.0.1:41789/v1
Content-Disposition: form-data; name="temperature"

+Inf

What changes

Both boundaries now reject non-finite floats:

  • parseCLIArg routes float64 and *float64 through a parseFiniteFloat helper that returns the same *strconv.NumError it returns for malformed numeric input, so the existing flag-error wrapper reports invalid value "inf" for flag -temperature: strconv.ParseFloat: parsing "inf": invalid syntax.
  • apiform routes every float it formats through a formatFloat helper that returns apiform: unsupported value: +Inf, which also covers the piped-YAML route and the comma-array, repeat and indexed array encoders.

No CLI surface is added or changed: same flags, same accepted values for every finite number, same defaults. Only values that previously produced an unwireable request now fail during parsing.

Encoding is unchanged for finite values, including the cases covered by #195 — exponent form still writes 1000 for 1e3, and negative zero still writes -0. Both are now pinned by the existing TestEncode table so the rejection cannot be widened into them by accident.

Scope and limits

internal/apiquery/encoder.go:164-168 formats float32/float64 query parameters the same way, and strconv.FormatFloat would render NaN/+Inf there too. I left it alone rather than guarding it: there is currently no float32/float64 value anywhere under pkg/cmd/, so no query parameter can reach that arm. If a future generated command adds one, it will need the same check. Say so if you would rather have it handled now for symmetry.

Testing

go1.25.0 darwin/arm64. The new tests were also run against unmodified sources in a separate worktree at 4b75e9b with only the two test files copied in, so the failure below is the defect and not the environment:

  • base tree + new tests: 65 --- FAIL lines (49 in internal/requestflag, 16 in internal/apiform), with the wire bytes in the message, e.g. expected an error encoding map[temperature:+Inf], got body "...name=\"foo\"\r\n\r\n+Inf".
  • fixed tree: go test -count=1 ./internal/requestflag/ ./internal/apiform/ → both ok, rc=0.
  • go test -count=1 ./internal/... → rc=0, 9 packages ok, 0 failures.
  • go test ./... -run '^$' → rc=0 (compiles every package, including pkg/cmd).
  • ./scripts/lint → rc=0. go vet ./internal/apiform/ ./internal/requestflag/ → rc=0. gofmt -l on the four changed files → empty. go mod verify → all modules verified.
  • Each half is pinned independently: with only requestflag.go reverted, internal/apiform stays ok while internal/requestflag fails; with only encoder.go reverted, internal/requestflag stays ok while internal/apiform fails, and the piped-YAML command sends +Inf again with exit 0.

Not run: ./scripts/test, which starts the source-based local mock server and installs the pinned Deno runtime. The pkg/cmd mock-server integration tests are therefore compile-checked here, not executed.

Both files are handwritten and outside pkg/cmd/, so they are not Castiron-generated and .castiron-ratchet.json is untouched.

@feiiiiii5
feiiiiii5 requested a review from a team as a code owner September 18, 2026 13:06
`openai audio:transcriptions create --temperature inf` (also -inf, nan,
infinity, in any case) exited 0 and put the literal bytes "+Inf"/"NaN"
in the multipart temperature part, sending a value no API accepts and
reporting success. Piped YAML bodies (`temperature: .inf` on stdin)
reached the same multipart encoder without going through typed flags,
so they sent "+Inf" too. JSON-body endpoints only failed late during
marshaling with "json: unsupported value: +Inf" after flag parsing had
already accepted the value.
@feiiiiii5
feiiiiii5 force-pushed the fix/reject-non-finite-float-flags branch from b7bd869 to 7d6be10 Compare September 19, 2026 06:26
@feiiiiii5

Copy link
Copy Markdown
Author

Rebased onto current main to clear the behind state — no content change.

git diff b7bd869 7d6be10 -- internal/apiform/encoder.go internal/apiform/form_test.go internal/requestflag/requestflag.go internal/requestflag/requestflag_test.go is empty, and everything else in that range is upstream's own 1d4e76c / dea465a / 3f2c883. mergeable_state moved from behind to blocked on the pending checks, so a one-click merge is available once CI is approved.

One claim in the description could have been invalidated by the new base, so I re-checked it: pkg/cmd/webhook.go and pkg/cmd/eventtype.go introduce no float32/float64 flags, so the internal/apiquery float arm described under Scope and limits is still unreachable.

Checks re-run on 7d6be10 (go1.25.0 darwin/arm64), each rc=0:

  • go test -count=1 ./internal/... → 9 packages ok, 0 failures
  • go test ./... -run '^$' → compiles every package including pkg/cmd
  • go mod verify → all modules verified
  • ./scripts/lint → rc=0
  • go vet ./internal/apiform/ ./internal/requestflag/ → rc=0; gofmt -l on the four changed files → empty

Still not run: ./scripts/test, which starts the source-based mock server and installs the pinned Deno runtime, so the pkg/cmd mock-server integration tests remain compile-checked here.

Routing note: AGENTS.md lists request parsing among the areas that need SDK CODEOWNER review, and this touches flag parsing plus multipart encoding — worth tagging whoever owns that. The three workflow runs on this head are still queued for a maintainer to approve.

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