Repository navigation
fix(cli): honour --api-key and stop the empty model catalog in setup-* commands - #18581
Closed
magnum8237 wants to merge 2 commits into
Closed
magnum8237 wants to merge 2 commits into
magnum8237 wants to merge 2 commits into
Conversation
The program-level `--api-key` option (bin/cli/program.mjs, bound to OMNIROUTE_API_KEY) shares its long name with the one each `setup-*` command declares. Commander 15 binds the value to the program, so `opts.apiKey` inside the subcommand action is undefined in EVERY arrangement — including `omniroute setup-claude --api-key X`, with the flag after the subcommand name. The commands read `opts.apiKey ?? opts["api-key"] ?? process.env.OMNIROUTE_API_KEY`, so they only worked by accident when OMNIROUTE_API_KEY happened to be exported. Without it they silently wrote a profile carrying no credential. Resolve the key via `cmd.optsWithGlobals().apiKey` while leaving the rest of the local opts untouched — the same narrow fix setup-opencode already carries (cheaper-inference#13246). Spreading the whole `optsWithGlobals()` bag instead would also start populating `opts.context` (read by 10 of these commands, none of which declare a local `--context`), changing context resolution well beyond this bug. Covers the 11 commands that declare their own `--api-key`: cursor, aider, goose, crush, qwen, cline, continue, roo, kilo, claude, codex.
`categoriseModel()` is a hardcoded regex table over legacy prefixes (kmc/, glm/,
opencode-go/) and returns null for everything else. Both commands did
`const cfg = categoriseModel(id); if (!cfg) continue;`, so a catalog dominated by
newer prefixes (aug/, dva/, tllm/, oc/ ...) produced ZERO models and the commands
printed "No matching curated models" with hundreds of models available.
Fall back to the derived profile the sibling commands already use — setup-codex
(`?? fallbackCodexProfile`) and setup-claude (`?? fallbackClaudeProfile`) have
carried this guard for a while; continue and crush were the two stragglers.
`fallbackCodexProfile` is called with `{ id }` rather than the bare id string:
the object form keeps its non-text modality filter active (image/audio/embedding
ids stay out of a chat-tool config), and `ctx` still resolves to a usable
128000 instead of undefined, which matters because crush writes `cfg.ctx`
straight into `context_window`.
Two existing assertions encoded the old skip-everything behavior and are updated
to the fallback contract, keeping their curated-value and non-text coverage.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Two independent bugs in the CLI
setup-*commands, one commit each.Bug 1 —
--api-keyis silently discarded (11 files)Each
setup-*command declares its own--api-key, whose long name collides with theprogram-level option in
bin/cli/program.mjs:With this project's Commander 15.0.0, the value binds to the program, so
opts.apiKeyinside the subcommand action is
undefinedin every arrangement — includingomniroute setup-claude --api-key X, with the flag after the subcommand name. Verifieddirectly against the installed Commander:
opts.apiKeycmd.optsWithGlobals().apiKey--api-keyafter subcommandundefinedCLI--api-keybefore subcommandundefinedCLIOMNIROUTE_API_KEYsetundefinedFROM-ENVundefinedundefinedThe commands read
opts.apiKey ?? opts["api-key"] ?? process.env.OMNIROUTE_API_KEY ?? "",so they only worked by accident when
OMNIROUTE_API_KEYhappened to be exported.Without it they wrote a profile carrying no credential, with no warning.
Precedent:
setup-opencodealready carries this exact fix via #13246("setup-opencode respects --api-key/OMNIROUTE_API_KEY over context token (#12783)").
This change applies the same pattern to the remaining 11 commands: cursor, aider, goose,
crush, qwen, cline, continue, roo, kilo, claude, codex.
Two deliberate deviations from a naive fix
1.
setup-codexis included. It was initially scoped out on the belief that itdeclares no
--api-keyof its own. It does —setup-codex.mjs:433— and uses.action(async (opts))→runSetupCodexCommand(opts)with the sameopts.apiKeyread atline 338. The collision and the failure mode are identical, so leaving it out would have
shipped a known-broken command.
2. Only
apiKeyis overridden, not the whole opts bag. Following the #13246 pattern:Spreading all of
optsWithGlobals()instead would also start populatingopts.context,which 10 of these 11 commands already read (
resolveActiveContext(opts.context ?? process.env.OMNIROUTE_CONTEXT))while declaring no local
--context. That would silently change context resolution acrossthe whole family — well beyond this bug.
--api-keyis the only option name that collideswith a program-level global in any of the 11 files (checked against all of
--api-key,--base-url,--context,--lang,--no-color,--output,--timeout).Bug 2 — empty model catalog in
setup-continue/setup-crush(2 files)categoriseModel()is a hardcoded regex table over legacy prefixes (kmc/,glm/,opencode-go/) and returnsnullfor everything else. Both commands did:so a catalog dominated by newer prefixes (
aug/,dva/,tllm/,oc/…) produced zeromodels and printed "No matching curated models" with hundreds of models available.
The fix reuses the fallback the sibling commands already have —
setup-codex.mjs:297(
?? fallbackCodexProfile) andsetup-claude.mjs:212(?? fallbackClaudeProfile).continueandcrushwere the two stragglers.fallbackCodexProfileis called asfallbackCodexProfile(id, { id }), not with the bareid string, for two reasons:
ctxmust be valid.crushwritescfg.ctxstraight intocontext_window; theobject form resolves it to a usable
128000rather thanundefined. (continueonlyreads
cfg.effort, which the fallback legitimately omits — it just means noautocompleterole.)isCodexCompatibleTextModelmatching its modality patterns against the id, so
image/audio/embedding/rerankids stay out of a chat-tool config. Passing the bare string would short-circuit that
gate (
if (typeof model === "string") return true) and let image models into the config.Note that
fallbackCodexProfile(id, undefined)would throw onmodel.context_length.Two pre-existing assertions encoded the old skip-everything behavior
(
"buildContinueModels skips uncategorised models", and crush'smodels.length === 1 // unknown skipped) and are updated to the fallback contract, keeping their curated-valueand non-text coverage.
Validation (Hard Rule #18 — TDD)
TDD was possible, so no VPS route was used. Both regression tests failed before the fix and
pass after.
tests/unit/cli/setup-cli-api-key-globals.test.ts— drives the realregisterSetupClaudeaction against a stub catalog server and asserts the key arrives asan
Authorization: Bearerheader, withOMNIROUTE_API_KEYdeleted from the env. Coversthe flag after and before the subcommand name, plus the no-key case.
Red before:
actual: undefined, expected: 'Bearer sk-cli-only'.tests/unit/cli/setup-cli-catalog-fallback.test.ts— unknown-prefix ids survive with apositive
context_window, non-text ids are still dropped, curated values preserved.Red before:
actual: 0, expected: 4.Verified locally:
tests/unit/cli/setup-{continue,crush,codex,claude,opencode}+ local-gatenpm run typecheck:coreeslinton changed filescheck-docs-sync,any-budget:t11,tracked-artifacts,gate-manifest, git-identity)release/v3.8.53)7 unrelated tests in
tests/unit/cli/fail identically on the pristine base tip(
origin/release/v3.8.53) and on this branch — POSIX mode-bit assertions(
actual: 438=0o666vsexpected: 384=0o600) that cannot hold on Windows. Confirmedpre-existing by re-running the directory against unmodified base copies of every file this
branch touches; this change introduces no new failures.