feat(cli): fast and no more JIT openapi spec#13
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ac7e6a7566
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| ], | ||
| "scripts": { | ||
| "test": "bun test", | ||
| "typecheck": "tsc --noEmit", |
There was a problem hiding this comment.
In the CI workflow I checked, .github/workflows/ci.yml now runs bun run typecheck, but this script expands to tsc --noEmit while the repo has no tsconfig.json and passes no source files. Running that command in the repo causes TypeScript to print help and exit non-zero, so every CI run and release will stop at the new Typecheck step before tests or conformance can run.
Useful? React with 👍 / 👎.
| if (["schema", "__schema", "__spec"].includes(args[0])) { | ||
| const schema = schemaOutput(contract); | ||
| if (config.json) process.stdout.write(`${JSON.stringify(schema)}\n`); |
There was a problem hiding this comment.
When callers use the documented/backward-compatible api __schema --json path, this now serializes the new schemaOutput() object directly instead of the legacy specli envelope; the previous CLI code itself parsed output.data.resources from __schema --json, so existing tooling following that contract will see no resources after upgrading. Please keep the legacy shape for the __schema alias, or reserve the new shape for schema --json.
Useful? React with 👍 / 👎.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| for (let index = 0; index < tokens.length; index++) { | ||
| const token = tokens[index]; | ||
| if (!token.startsWith("--")) { | ||
| positionals.push(token); | ||
| continue; | ||
| } | ||
| const option = splitOption(token); | ||
| let raw = option.inline; | ||
| if ( | ||
| raw === undefined && | ||
| tokens[index + 1] !== undefined && | ||
| !tokens[index + 1].startsWith("--") | ||
| ) { | ||
| raw = tokens[++index]; | ||
| } | ||
| if (option.name === "body-json") { |
There was a problem hiding this comment.
🔴 Boolean flags in the new hand-rolled parser (parseOperationInput) greedily consume the next bare token as their value before checking whether the flag is boolean-kind, so a bare boolean flag followed by a positional path argument breaks. E.g. langfuse api prompts get --resolve my-prompt-name throws Expected boolean, got my-prompt-name, and --no-resolve my-prompt-name silently swallows the positional, failing with prompts_get expects 1 path argument(s), got 0. This affects any operation with a boolean query/header parameter followed by a positional (e.g. prompts_get's resolve + promptName); fix by checking the target flag's kind (boolean vs. not, and negated vs. not) before consuming tokens[index+1].
Extended reasoning...
The bug: In parseOperationInput (src/cli.ts:446-461), the token-parsing loop decides whether to consume the next token as a flag's raw value purely by checking that the next token does not start with --:
let raw = option.inline;
if (
raw === undefined &&
tokens[index + 1] !== undefined &&
!tokens[index + 1].startsWith("--")
) {
raw = tokens[++index];
}This runs before the parser knows anything about the flag's kind — including whether it's a boolean-kind query/header parameter. Boolean flags are explicitly designed to be usable bare: addParameterValue (src/cli.ts:361) falls back to raw ?? "true" when no raw value is supplied, and there's a dedicated --no-<flag> negation path (src/cli.ts:471-476) that forces the value to "false". Both mechanisms only make sense if a bare boolean flag can appear without consuming a following token.
Concrete trigger: prompts_get (GET /api/public/v2/prompts/{promptName}) has a boolean query parameter resolve plus a required positional path argument promptName. Running the natural invocation:
langfuse api prompts get --resolve my-prompt-name
walks the loop like this: at --resolve, option.inline is undefined, the next token my-prompt-name doesn't start with --, so it's greedily assigned to raw. addParameterValue then calls parseJsonValue("my-prompt-name", "boolean"), which throws Expected boolean, got my-prompt-name. The promptName positional is never pushed onto positionals because it was consumed here instead.
Worse, with negation:
langfuse api prompts get --no-resolve my-prompt-name
the same greedy consumption swallows my-prompt-name into raw, but then addParameterValue(input, parameter, option.negated ? "false" : raw) discards raw entirely because option.negated is true. The positional is lost with no error at the point of loss, and the command instead fails downstream with the confusing prompts_get expects 1 path argument(s), got 0.
Why nothing else prevents this: The loop has no way to look up the flag's kind before deciding whether to consume the next token — parameterByFlag.get(option.name) isn't invoked until after raw has already been computed. Since path arguments are positional and can be any bare string, there's no syntactic way to distinguish "next token is this boolean flag's value" from "next token is an unrelated positional" without first resolving the flag.
Step-by-step proof (matches independent verifier runs against the built CLI):
- Build:
bun run build(compiles 678 operations, includingprompts_getwith booleanresolve). - Run
bun bin/langfuse.mjs api prompts get --resolve my-prompt-name→ the loop sees--resolve, thenmy-prompt-name(no--prefix) →raw = "my-prompt-name";indexadvances past it, somy-prompt-namenever becomes a positional.addParameterValuecallsparseJsonValue("my-prompt-name", "boolean")→ throwsExpected boolean, got my-prompt-name. - Run
bun bin/langfuse.mjs api prompts get --no-resolve my-prompt-name→ same greedy consumption ofmy-prompt-nameintoraw, butoption.negatedis true soaddParameterValueis called with"false"instead ofraw— the positional is discarded silently.positionalsends up empty, and the command fails at path-binding time withprompts_get expects 1 path argument(s), got 0. - Control:
bun bin/langfuse.mjs api prompts get my-prompt-name --resolve(positional before flag) works correctly and emitsresolve=truein the query, confirming the defect is purely about ordering/greedy-consumption, not aboutresolveitself.
Fix direction: Before consuming tokens[index+1] as raw, resolve parameterByFlag.get(option.name) (or the request-body field lookup) first, and skip the greedy consumption when the resolved target is boolean-kind and either not negated-incompatible (i.e., always skip for boolean kind, since raw ?? "true" already covers the bare case) — requiring --flag=value or a following --flag value structure only for non-boolean flags. This affects every operation that pairs a boolean query/header parameter with a positional path argument (or simply a later bare token), which is a systemic issue in the new parser introduced by this PR (replacing the removed specli dependency), not just prompts_get.
| "bin", | ||
| "dist", | ||
| "openapi.yml", | ||
| "README.md" |
There was a problem hiding this comment.
🔴 This PR adds a "typecheck": "tsc --noEmit" script (package.json) and wires it into a required CI step (.github/workflows/ci.yml) and into scripts/release.ts before publishing, but no tsconfig.json exists anywhere in the repository. Running tsc --noEmit with no tsconfig.json in the working directory does not type-check anything — it prints tsc's help/usage banner and exits with code 1 — so the new "Typecheck" CI step fails on every PR/push and the release script is blocked before build/publish.
Extended reasoning...
The bug: package.json adds a "typecheck": "tsc --noEmit" script, and this PR wires it into two new call sites: .github/workflows/ci.yml (a required Typecheck step running bun run typecheck) and scripts/release.ts:387 (invoked before building/publishing during an interactive release). The repository has no tsconfig.json anywhere — confirmed via git ls-files | grep tsconfig and a directory search, both empty.
TypeScript's tsc requires either a tsconfig.json in the current working directory or explicit file arguments to know what to compile. With neither present, tsc --noEmit does not type-check any source files at all — instead it falls back to printing its CLI help/usage banner and exits with status code 1, treating the invocation as a usage error rather than a compilation run.
Reproduction: Running bun run typecheck directly in the repo (verified independently by four reviewers, using the installed typescript@7.0.2) prints only the tsc help text and exits with 1. No files under src/, conformance/, or scripts/ are actually type-checked — the command never gets that far.
Why nothing catches this today: The script and its two call sites (CI step, release script) are all newly added by this PR; there is no existing tsconfig for tsc to pick up, and nothing in the PR adds one. Since bun run typecheck was never previously invoked anywhere in the repo, this failure mode was not previously exercised.
Impact: The new "Typecheck" step in ci.yml will fail deterministically on every PR, every merge-queue run, and every push to main, turning CI permanently red for a step that is presumably meant to be a real gate. Separately, scripts/release.ts runs the same command before building and publishing, so the interactive release flow will also abort at that step, blocking releases entirely until this is fixed.
Proof walkthrough:
git ls-files | grep -i tsconfig→ no output (no tsconfig.json tracked in the repo).package.json(this PR) adds:"typecheck": "tsc --noEmit".- Run
bun run typecheckin the repo root → tsc (v7.0.2, present innode_modules/bun.lock) finds no tsconfig.json and no explicit input files. - tsc's behavior in that situation is to print its help/usage text (not an error about missing config) and exit with code
1. .github/workflows/ci.yml(this PR) adds a step- name: Typecheck / run: bun run typecheck— this step will exit non-zero and fail the job on every run.scripts/release.ts:387(this PR) calls the same command before build/publish, so the release script will also fail at that point.
Fix: Add a tsconfig.json that includes src/, conformance/, and scripts/ (matching what the PR's conformance README implies should be checked), so that tsc --noEmit actually type-checks the intended files instead of falling back to the help banner.
| } | ||
| const name = path.at(-1)!; | ||
| const kind = path.length > 1 || field?.kind === "array" ? undefined : field?.kind; | ||
| const parsed = parseJsonValue(raw ?? "true", kind); | ||
| const existing = target[name]; | ||
| if (field?.kind === "array") { | ||
| if (Array.isArray(parsed)) target[name] = parsed; | ||
| else if (Array.isArray(existing)) existing.push(parsed); | ||
| else target[name] = [parsed]; | ||
| } else if (existing !== undefined) { | ||
| target[name] = Array.isArray(existing) ? [...existing, parsed] : [existing, parsed]; | ||
| } else { | ||
| target[name] = parsed; | ||
| } | ||
| } |
There was a problem hiding this comment.
🟡 Array-typed request body fields (e.g. customModels on llm-connections put) lose their item type when compiled: ApiBodyField has no itemKind (unlike ApiParameter, which does), so setBodyValue() forces kind = undefined for array fields and falls back to a bare JSON.parse(value). As a result, a legacy field-flag value that happens to look like a JSON literal (e.g. --customModels 123) is inserted into the body as a number/boolean instead of the intended string, sending a wrong-typed JSON body to the API.
Extended reasoning...
The bug. ApiBodyField (src/contracts/types.ts:43-48) only carries kind: ValueKind, with no itemKind field. Compare this to ApiParameter (types.ts:32-41), which does carry itemKind?: ValueKind. In collectBodyFields() (src/contracts/compiler.ts:153-192), body fields are built from the schema's kind/required/description only — array-kind properties never get an item type computed, whereas mergeParameters() (compiler.ts:140-142) explicitly sets itemKind = schemaKind(items) for array-kind query/path parameters.
How it manifests. In src/cli.ts, setBodyValue() (lines ~384-398) works around the missing itemKind by forcibly setting kind = undefined whenever field.kind === "array". This routes the value into parseJsonValue(value, undefined), which (per parseJsonValue, cli.ts:309-343) has no type to coerce against and falls through to the generic try { JSON.parse(value) } catch { return value } branch. That means any array body field — regardless of whether its declared item type is string, number, or boolean — ends up coerced by whatever JSON.parse can successfully parse, rather than by the schema's actual item type. This is the exact inconsistency addParameterValue() avoids for query/path arrays by using parameter.itemKind ?? parameter.kind.
Why nothing currently prevents it. The legacy field-flag adapter is the only code path affected (--body-json bypasses this entirely by passing raw JSON straight through), and it silently "works" for the common case because most realistic string values (model names, IDs, tags) fail to parse as JSON and fall back to being kept as raw strings. The bug only surfaces when a string-array value happens to also be valid JSON syntax — a number, true/false, null, or a JSON array/object literal.
Concrete reproduction (step-by-step). llmConnections_upsert (the llm-connections put CLI command) is not in LEGACY_FIELD_FLAGS_UNSUPPORTED, so it accepts legacy field flags, and its request body schema (UpsertLlmConnectionRequest.customModels) is array of string.
- Run
langfuse api llm-connections put --provider openai --adapter openai --secretKey sk --customModels 123 --curl. setBodyValue()seesfield.kind === "array"forcustomModelsand forceskind = undefined.parseJsonValue("123", undefined)callsJSON.parse("123"), which succeeds and returns the number123.- The emitted request body contains
"customModels":[123]— a JSON number — even though the schema requires an array of strings. - Compare with
--customModels gpt-4:JSON.parse("gpt-4")throws, so the raw string is kept and["gpt-4"]is emitted correctly — this is why the bug is invisible for ordinary values and only appears for JSON-literal-looking ones (e.g.--customModels truealso inserts the booleantrueinstead of the string"true").
Impact and fix. The practical trigger is narrow: it only affects legacy-field-flag invocations of array-of-string body fields whose value coincidentally parses as JSON. When it does trigger, the CLI silently sends a wrong-typed body element to the API, which will likely be rejected or misinterpreted server-side, and there is a lossless workaround available today via --body-json. The fix is straightforward and mirrors the existing parameter-side handling: add itemKind?: ValueKind to ApiBodyField, compute it in collectBodyFields() for array-kind body properties the same way mergeParameters() does, and change setBodyValue() to use field.itemKind ?? field.kind instead of unconditionally clearing kind for arrays.
| "files": [ | ||
| "bin", | ||
| "dist", | ||
| "openapi.yml", | ||
| "README.md" | ||
| ], | ||
| "scripts": { | ||
| "test": "bun test", | ||
| "typecheck": "tsc --noEmit", | ||
| "conformance:sync": "bun conformance/src/cli.ts sync", | ||
| "conformance:run": "bun conformance/src/cli.ts run", | ||
| "patch-openapi": "bun scripts/patch-openapi.ts", | ||
| "refetch-openapi": "bun scripts/patch-openapi.ts --refetch", | ||
| "build": "bun run refetch-openapi && bun build src/cli.ts --outdir dist --target node --format esm", | ||
| "conformance:all": "bun conformance/src/all.ts", | ||
| "build": "bun scripts/build.ts", | ||
| "release": "bun scripts/release.ts", | ||
| "prepublishOnly": "rm -rf dist && bun run build" | ||
| }, | ||
| "dependencies": { | ||
| "specli": "^0.0.39" | ||
| "prepublishOnly": "bun run build" | ||
| }, | ||
| "dependencies": {}, | ||
| "devDependencies": { | ||
| "@apidevtools/swagger-parser": "^12.1.0", | ||
| "@types/bun": "^1.3.14", | ||
| "ajv": "^8.17.1", | ||
| "ajv-formats": "^3.0.1", | ||
| "typescript": "^7.0.2", | ||
| "yaml": "^2.8.2" | ||
| }, | ||
| "engines": { | ||
| "node": ">=20" | ||
| "bun": ">=1.3.0" | ||
| } | ||
| } |
There was a problem hiding this comment.
🟡 This PR removes scripts/patch-openapi.ts, the patch-openapi/refetch-openapi npm scripts, the specli dependency, and openapi.yml, but README.md still documents them as current — including a whole 'OpenAPI Patch Script' section with commands that no longer exist. It also still warns that dry-run may update 'openapi.yml', which is gone, and never mentions the new --api-version/--body-json/--body-file/--timeout flags or the versions subcommand.
Extended reasoning...
This PR replaces the old specli/field-flag CLI generation pipeline with a new contract-based system, and as part of that it deletes scripts/patch-openapi.ts, removes the patch-openapi/refetch-openapi npm scripts and the specli dependency from package.json, and deletes openapi.yml outright. README.md is not among the changed files in this PR, so it still describes the old subsystem as if it were current.
Concretely, README.md's "OpenAPI Patch Script" section (lines 94-109) documents scripts/patch-openapi.ts flattening oneOf/allOf for specli, and instructs readers to run bun run refetch-openapi, bun run patch-openapi -- --refetch ..., and bun run patch-openapi. None of these scripts exist after this PR — running any of them will fail immediately since the referenced files and package.json script entries are gone. Separately, the Release section still warns that a dry-run release "may update generated release artifacts like dist/ and openapi.yml," but openapi.yml no longer exists in the repository at all.
The reason this isn't caught by any existing check is that README.md is plain prose, not code exercised by tests, typecheck, or CI — nothing fails when it goes stale. The PR's own diff is the only signal: it deletes the very files/scripts the README describes as the primary way to regenerate the OpenAPI-derived CLI surface.
Proof of the mismatch, step by step:
- Before this PR,
package.jsonhad"patch-openapi"and"refetch-openapi"scripts and aspeclidependency;scripts/patch-openapi.tsandopenapi.ymlexisted at the repo root. - This PR's diff (see
package.jsonlines 23-50 and thedeleted file modeentries forscripts/patch-openapi.tsandopenapi.yml) removes all of them, replacing them with the newsrc/contracts/*compiler/loader and CLI flags (--body-json,--body-file,--api-version,--timeout,--curl, and aversionssubcommand). - README.md is untouched by the diff, so it still contains the "OpenAPI Patch Script" section and the openapi.yml reference from before the PR.
- A contributor following the README today would run
bun run patch-openapi(orrefetch-openapi) and get an npm "missing script" error, since those scripts no longer exist in package.json. - None of the new global flags or the
versionssubcommand introduced by this PR are mentioned anywhere in the README, so users have no discoverable documentation for the CLI's actual current surface.
Impact-wise, this is a documentation-only defect: nothing in the shipped CLI binary breaks, no runtime behavior changes, and no user data is at risk. It is, however, actively misleading — a contributor or user reading the README would be pointed at tooling that has been deleted in this very PR, and would have no way to discover the new --api-version/--body-json/versions surface from the docs. The fix is straightforward: remove the "OpenAPI Patch Script" section (or replace it with an explanation of the new contract compiler/loader workflow in src/contracts), drop the stale openapi.yml reference from the Release section, and add a short blurb documenting the new global flags and versions subcommand.
I considered whether this should be normal given the size of the stale section, but nothing breaks at runtime for existing users of the built CLI — the failure mode is limited to a contributor trying to run an removed dev script, which is quickly self-diagnosing (npm reports the script doesn't exist). That keeps this at the documentation-only, non-blocking end of the spectrum, so I'm marking it nit rather than normal.
| async function writeResult( | ||
| result: ApiResult, | ||
| config: RuntimeConfig, | ||
| ): Promise<void> { | ||
| if (config.output) { | ||
| const content = | ||
| typeof result.body === "string" | ||
| ? result.body | ||
| : JSON.stringify(result.body, null, 2); | ||
| await Bun.write(config.output, content ?? ""); | ||
| } else if (config.json) { | ||
| process.stdout.write( | ||
| `${JSON.stringify({ status: result.status, headers: result.headers, body: result.body })}\n`, | ||
| ); | ||
| } else if (typeof result.body === "string") { | ||
| process.stdout.write(result.body.endsWith("\n") ? result.body : `${result.body}\n`); | ||
| } else if (result.body !== null) { | ||
| process.stdout.write(`${JSON.stringify(result.body, null, 2)}\n`); | ||
| } | ||
| if (!result.ok) process.exitCode = 1; | ||
| } |
There was a problem hiding this comment.
🟡 In writeResult() (src/cli.ts:588-593), the --output branch writes the literal string "null" to the output file for empty/204 responses (e.g. models delete, prompts delete, media patch), instead of writing nothing. This is inconsistent with the stdout branches just below, which explicitly special-case a null body and print nothing.
Extended reasoning...
writeResult() in src/cli.ts computes the file content for --output as:
const content =
typeof result.body === "string"
? result.body
: JSON.stringify(result.body, null, 2);
await Bun.write(config.output, content ?? "");For any operation whose response has no body — HTTP 204/205, or an empty response text — client.ts's responseBody() returns null (see lines 131-134). Since typeof null === "object", the ternary falls into the JSON.stringify(null, null, 2) branch, which evaluates to the 4-character string "null", not the value null. The content ?? "" fallback only guards against undefined (which JSON.stringify never returns for a null input), so it never fires here. The net effect: the literal text null gets written to the user's --output file.
This is inconsistent with the CLI's own stdout behavior a few lines below in the same function:
} else if (result.body !== null) {
process.stdout.write(`${JSON.stringify(result.body, null, 2)}\n`);
}Here the author explicitly checks result.body !== null and intentionally writes nothing to stdout for an empty-body response. The --output branch has no equivalent check, so it diverges from the documented/intended default behavior purely because of which sink is used.
Trigger path: run any operation whose backend returns 204 with --output, e.g. langfuse api models delete <id> --output out.json, langfuse api prompts delete <name> --output out.json, or langfuse api media patch <id> --body-json '{}' --output out.json. responseBody() returns null for these, writeResult takes the --output path, and out.json ends up containing the 4-byte string null instead of being empty.
Step-by-step proof:
models deletehitsDELETE /api/public/models/{id}, which the spec defines with a204response (seeopenapi.ymlschema for that operation).client.ts's response handling sees status 204 and setsresult.body = nullperresponseBody().writeResult(result, config)is called withconfig.outputset (user passed--output out.json).typeof null === "object"socontent = JSON.stringify(null, null, 2)→ the string"null".content ?? ""→"null"(non-nullish, so the fallback doesn't apply).Bun.write("out.json", "null")writes 4 bytes:null.- Meanwhile, running the same command without
--outputprints nothing to stdout, because theresult.body !== nullguard suppresses output — proving the intended behavior for an empty body is "no output", which--outputfails to honor.
Why this isn't more severe: the string "null" is itself syntactically valid JSON (it parses to the JSON value null), so scripts that JSON.parse() the output file won't crash — they'll just get null instead of an empty/absent file. This is a minor behavioral inconsistency between the --output and stdout code paths for empty-body responses, not a crash or data-loss bug, so it doesn't need to block merge, but it's worth aligning with the stdout behavior (e.g. skip the write, or write an empty string, when result.body === null).
1 participant
No description provided.