Skip to content

fix(cli): keep TOML digit separators in --codex-config values - #34

Open
bluzername wants to merge 1 commit into
0xnyn:mainfrom
bluzername:fix/codex-config-underscore-numbers
Open

bluzername wants to merge 1 commit into
0xnyn:mainfrom
bluzername:fix/codex-config-underscore-numbers

Conversation

@bluzername

Copy link
Copy Markdown

The bug

TOML let a number use _ between digits as a separator, so
model_max_output_tokens=100_000 is valid TOML and much nicer to read
than 100000. coerce() in apps/cli/src/lib/backends.ts decide if a
--codex-config k=v value is a bool, a number or a string by trying
Number(raw) on the text, but Number("100_000") is NaN in
JavaScript, it does not know this TOML syntax at all. So the value
fall through to the string branch, and Codex would get the literal
text "100_000" in its config where it expect the integer 100000.

It is the exact same class of bug the docstring on coerce() already
call out (a JS string reaching Codex where a scalar was wanted), just
a different input shape that was not covered.

coerce("100_000")
// before: "100_000"  (string - wrong)
// after:  100000      (number - correct)

The fix

Strip an underscore only when it sit between two digits, with a small
regex lookaround, before calling Number(). A wrongly placed one
(leading _100, trailing 100_, doubled 1__000) is left alone, so
it still fail the numeric check and fall back to a string, same as
real TOML would also treat it as invalid.

Test plan

  • Added cases to apps/cli/src/lib/backends.test.ts: an integer with
    separators, a float with separators, a negative number, and the
    three invalid-placement cases that must stay strings.
  • Confirmed RED first: ran the new tests against the unmodified
    coerce(), 3 of them failed exactly as expected.
  • Confirmed GREEN after the fix: pnpm --filter @airshiplabs/cli exec vitest run src/lib/backends.test.ts -> all 22 tests in that file
    pass.
  • Full suite after: pnpm typecheck and pnpm turbo run test --filter=@airshiplabs/cli both green, 182 tests passed (176 before
    this change, +6 new).

Not a collaborator, opening this for review whenever you get to it.

TOML let a number use `_` between digits, like
`model_max_output_tokens=100_000`, so it is nicer to read than
`100000`. But coerce() in backends.ts just call Number() on the raw
text, and Number("100_000") is NaN in JS - it not understand that
syntax at all. So the value fall through to the string branch and
Codex get the literal text "100_000" where its config want a number.
This is exactly the same class of bug the function already exist for
(a string reaching Codex where a scalar was expect), just a different
syntax that trip it up.

Fix strip an underscore only when it sit between two digits before
calling Number() on it, using a regex lookaround. A wrongly placed
underscore (leading, trailing, or doubled, like "_100", "100_",
"1__000") is left alone and still fail the numeric check same as
before, so it keep falling back to a string like real TOML would treat
it as invalid too.

Test: added cases in backends.test.ts for an integer, a float, a
negative number, all with separators, plus the three invalid-placement
cases that must stay strings. Ran the new tests against the old
coerce() first to confirm they fail (they did, 3 failing), then again
with the fix to confirm all pass. Full suite: pnpm typecheck and pnpm
turbo run test --filter=@airshiplabs/cli both green, 182 tests passed
(176 before, +6 new).

This branch has not been deployed

No deployments
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