diff --git a/apps/cli/src/lib/backends.test.ts b/apps/cli/src/lib/backends.test.ts index cdffa21..9ba1273 100644 --- a/apps/cli/src/lib/backends.test.ts +++ b/apps/cli/src/lib/backends.test.ts @@ -26,6 +26,31 @@ describe("coerce", () => { expect(coerce('"true"')).toBe("true"); expect(coerce('"42"')).toBe("42"); }); + + // The bug this is for: TOML let a number use `_` between digits, so + // `model_max_output_tokens=100_000` is valid TOML and much easier to read + // than `100000`. Before this, coerce() did not know that syntax and the + // value reach Codex as the literal string "100_000" instead of the number + // 100000. + it("reads a TOML digit separator in an integer", () => { + expect(coerce("100_000")).toBe(100_000); + expect(coerce("1_000_000")).toBe(1_000_000); + }); + + it("reads a TOML digit separator in a float", () => { + expect(coerce("1_234.5")).toBe(1234.5); + }); + + it("keeps a negative number with a digit separator", () => { + expect(coerce("-1_000")).toBe(-1000); + }); + + it.each(["_100", "100_", "1__000"])( + "does not touch an underscore that is not a valid separator: %s", + (bad) => { + expect(coerce(bad)).toBe(bad); + } + ); }); describe("parseCodexConfig", () => { diff --git a/apps/cli/src/lib/backends.ts b/apps/cli/src/lib/backends.ts index 5865801..590b79e 100644 --- a/apps/cli/src/lib/backends.ts +++ b/apps/cli/src/lib/backends.ts @@ -11,6 +11,16 @@ import { readFileSync } from "node:fs"; import type { CodexConfigValue } from "@airship/server"; import { CliError, EXIT } from "./errors"; +// TOML lets a number use `_` between digits as a separator, so a config value +// like `model_max_output_tokens=100_000` is valid TOML and reads a lot better +// than `100000`. `Number()` does not know this syntax, so left alone it falls +// through to the string branch below and Codex would get the literal text +// "100_000" where it wants an integer. The lookaround only strips an +// underscore that sits between two digits, so a wrongly placed one (`_100`, +// `100_`, `1__000`) is left in place and still fails the numeric check, same +// as it would in real TOML. +const DIGIT_SEPARATOR = /(?<=[0-9])_(?=[0-9])/g; + /** * Interpret a flag value as the TOML scalar it looks like. * @@ -27,8 +37,9 @@ export function coerce(raw: string): CodexConfigValue { if (raw === "false") { return false; } - if (raw !== "" && Number.isFinite(Number(raw))) { - return Number(raw); + const withoutSeparators = raw.replace(DIGIT_SEPARATOR, ""); + if (raw !== "" && Number.isFinite(Number(withoutSeparators))) { + return Number(withoutSeparators); } // An explicitly quoted value keeps its quotes stripped but stays a string. if (raw.length > 1 && raw.startsWith('"') && raw.endsWith('"')) {