fix openrouter aliases and tui key handling#57198
Conversation
Greptile SummaryThis PR fixes two independent regressions: Changes:
Minor concern: When Confidence Score: 5/5Safe to merge — both fixes are correct, regression tests cover the happy paths, and the only open concerns are P2 style/edge-case quality issues. All identified issues are P2: one is dead code (redundant String()/?? ""), and the other is a missing user-facing error message for a corner case (openrouter:free with no configured free models). Neither blocks correct behaviour on the documented paths. The core logic is sound, tests are well-placed, and the TUI migration is a straightforward API swap. src/agents/model-selection.ts — minor cleanup in resolveOpenRouterCompatAlias() and consideration of the openrouter:free-with-no-models error path.
|
| Filename | Overview |
|---|---|
| src/agents/model-selection.ts | Adds resolveOpenRouterCompatAlias() to handle openrouter:auto and openrouter:free shorthands before the legacy anthropic fallback. Logic is correct for the happy path; the openrouter:free no-configured-models error path silently falls through to a misleading anthropic fallback warning. |
| src/agents/model-selection.test.ts | Adds two regression tests covering openrouter:auto and openrouter:free happy paths. No test for the openrouter:free-with-no-configured-models edge case. |
| src/tui/components/filterable-select-list.ts | Replaces removed getKeybindings() / kb.matches() API with the current public Key.escape / Key.ctrl('c') + matchesKey() pattern. Straightforward and correct. |
| src/tui/components/searchable-select-list.ts | Same getKeybindings() removal as filterable-select-list.ts — clean API migration, no behavioural change to cancel handling. |
Prompt To Fix All With AI
This is a comment left during a code review.
Path: src/agents/model-selection.ts
Line: 288-302
Comment:
**Misleading fallback when `openrouter:free` is used but no free models are configured**
When a user sets their primary model to `openrouter:free` but has not configured any concrete `openrouter/...:free` entries under `agents.defaults.models`, `resolveOpenRouterCompatAlias` returns `null`. Control then falls through to the legacy anthropic fallback, which emits a misleading warning (`"openrouter:free" specified without provider. Falling back to "anthropic/openrouter:free"`) and returns an invalid model ref that will fail at runtime.
Consider emitting a dedicated warning inside `resolveOpenRouterCompatAlias` before returning `null`, telling the user they need at least one `openrouter/...:free` entry configured under `agents.defaults.models`.
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: src/agents/model-selection.ts
Line: 293-294
Comment:
**Redundant defensive wrapping on `Object.keys()` result**
`Object.keys(configuredModels)` always yields plain strings, so `keyRaw` is never null/undefined. Both the `?? ""` fallback and the `String()` cast are unreachable dead code that can be dropped.
How can I resolve this? If you propose a fix, please make it concise.Reviews (1): Last reviewed commit: "fix: restore openrouter aliases and tui ..." | Re-trigger Greptile
| if (normalized !== "openrouter:free") { | ||
| return null; | ||
| } | ||
|
|
||
| const configuredModels = params.cfg.agents?.defaults?.models ?? {}; | ||
| for (const keyRaw of Object.keys(configuredModels)) { | ||
| const key = String(keyRaw ?? "").trim(); | ||
| if (!key.startsWith("openrouter/")) { | ||
| continue; | ||
| } | ||
| const parsed = parseModelRef(key, DEFAULT_PROVIDER); | ||
| if (parsed?.provider === "openrouter" && parsed.model.endsWith(":free")) { | ||
| return parsed; | ||
| } | ||
| } |
There was a problem hiding this comment.
openrouter:free is used but no free models are configured
When a user sets their primary model to openrouter:free but has not configured any concrete openrouter/...:free entries under agents.defaults.models, resolveOpenRouterCompatAlias returns null. Control then falls through to the legacy anthropic fallback, which emits a misleading warning ("openrouter:free" specified without provider. Falling back to "anthropic/openrouter:free") and returns an invalid model ref that will fail at runtime.
Consider emitting a dedicated warning inside resolveOpenRouterCompatAlias before returning null, telling the user they need at least one openrouter/...:free entry configured under agents.defaults.models.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/agents/model-selection.ts
Line: 288-302
Comment:
**Misleading fallback when `openrouter:free` is used but no free models are configured**
When a user sets their primary model to `openrouter:free` but has not configured any concrete `openrouter/...:free` entries under `agents.defaults.models`, `resolveOpenRouterCompatAlias` returns `null`. Control then falls through to the legacy anthropic fallback, which emits a misleading warning (`"openrouter:free" specified without provider. Falling back to "anthropic/openrouter:free"`) and returns an invalid model ref that will fail at runtime.
Consider emitting a dedicated warning inside `resolveOpenRouterCompatAlias` before returning `null`, telling the user they need at least one `openrouter/...:free` entry configured under `agents.defaults.models`.
How can I resolve this? If you propose a fix, please make it concise.| for (const keyRaw of Object.keys(configuredModels)) { | ||
| const key = String(keyRaw ?? "").trim(); |
There was a problem hiding this comment.
Object.keys() result
Object.keys(configuredModels) always yields plain strings, so keyRaw is never null/undefined. Both the ?? "" fallback and the String() cast are unreachable dead code that can be dropped.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/agents/model-selection.ts
Line: 293-294
Comment:
**Redundant defensive wrapping on `Object.keys()` result**
`Object.keys(configuredModels)` always yields plain strings, so `keyRaw` is never null/undefined. Both the `?? ""` fallback and the `String()` cast are unreachable dead code that can be dropped.
How can I resolve this? If you propose a fix, please make it concise.There was a problem hiding this comment.
Pull request overview
Fixes two regressions impacting model resolution and local TUI type-checking: OpenRouter shorthand refs (openrouter:auto, openrouter:free) no longer incorrectly fall through to the legacy Anthropic providerless fallback, and TUI selector components are updated to use the current @mariozechner/pi-tui key API.
Changes:
- Add explicit OpenRouter compatibility handling for
openrouter:autoandopenrouter:freebefore the providerless Anthropic fallback inresolveConfiguredModelRef(). - Add unit tests covering both OpenRouter shorthands to prevent regression.
- Replace removed
getKeybindingsusage in TUI selectors withKey+matchesKeycancel handling.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| src/agents/model-selection.ts | Adds OpenRouter shorthand resolution logic prior to legacy providerless fallback. |
| src/agents/model-selection.test.ts | Adds regression tests for openrouter:auto and openrouter:free. |
| src/tui/components/filterable-select-list.ts | Updates cancel key handling to Key.escape / Key.ctrl("c") using matchesKey. |
| src/tui/components/searchable-select-list.ts | Updates cancel key handling to Key.escape / Key.ctrl("c") using matchesKey. |
| const configuredModels = params.cfg.agents?.defaults?.models ?? {}; | ||
| for (const keyRaw of Object.keys(configuredModels)) { | ||
| const key = String(keyRaw ?? "").trim(); | ||
| if (!key.startsWith("openrouter/")) { |
There was a problem hiding this comment.
resolveOpenRouterCompatAlias() filters configured model keys with key.startsWith("openrouter/"), which is case-sensitive. Elsewhere provider IDs are normalized via normalizeProviderId() (lowercased), so configs like OpenRouter/... would be ignored and openrouter:free would incorrectly fall back to the anthropic providerless path. Consider removing the startsWith prefilter (parse every key and check parsed.provider === "openrouter") or at least using a case-insensitive check like key.toLowerCase().startsWith("openrouter/").
| if (!key.startsWith("openrouter/")) { | |
| if (!key.toLowerCase().startsWith("openrouter/")) { |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bbd98e25a6
ℹ️ 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".
| const configuredModels = params.cfg.agents?.defaults?.models ?? {}; | ||
| for (const keyRaw of Object.keys(configuredModels)) { | ||
| const key = String(keyRaw ?? "").trim(); | ||
| if (!key.startsWith("openrouter/")) { |
There was a problem hiding this comment.
Normalize OpenRouter keys when resolving
openrouter:free
The new compatibility path only considers configured model keys that literally start with "openrouter/", which is case-sensitive. Because provider ids are normalized elsewhere, configs like "OpenRouter/meta-llama/...:free" are valid in other flows but get skipped here, causing openrouter:free to fall through to the anthropic fallback again. Lowercasing before the prefix check (or parsing first and checking parsed.provider) would avoid this regression for mixed-case configs.
Useful? React with 👍 / 👎.
f3dda91 to
842797e
Compare
8685791 to
d0621fb
Compare
Summary
openrouter:autoandopenrouter:freecurrently fall through the providerless-model path and get rewritten to invalidanthropic/openrouter:*refs; in parallel,pnpm tsgois broken because two TUI selector components still import removedgetKeybindingsAPI from@mariozechner/pi-tui.resolveConfiguredModelRef(), added regression tests for both shorthands, and updated the two TUI selector components to use the current publicKey/matchesKeyAPI for cancel handling.Change Type (select all)
Scope (select all touched areas)
Linked Issue/PR
openrouter:freeresolves toanthropic/openrouter:free;openrouter:autoshould also be handled explicitly #57066Root Cause / Regression History (if applicable)
resolveConfiguredModelRef()handled configured aliases, then immediately applied the legacy anthropic fallback to any remaining providerless ref, so OpenRouter compatibility shorthands never had a chance to resolve; separately, the TUI components still referencedgetKeybindingsafter@mariozechner/pi-tuimoved consumers toKey+matchesKey.openrouter:autooropenrouter:free, and no type-check guard had been exercised recently against the currentpi-tuipublic exports before local commit validation.git blame, prior PR, issue, or refactor if known): closed PR fix: handle openrouter compatibility aliases #57078 proposed the OpenRouter compatibility behavior; issue tui: getKeybindings import no longer exported by @mariozechner/pi-tui, breaking pnpm tsgo #57196 records the TUI type-check break discovered while running the repo's required check chain.pi-tuikey API surface.mainlocally before patching; no evidence of config-specific or environment-specific causation was needed for either failure.Regression Test Plan (if applicable)
src/agents/model-selection.test.tsopenrouter:autoresolves toopenrouter/auto;openrouter:freeresolves to the first configured concreteopenrouter/...:freemodel.pnpm tsgoonce the imports use the current public API.pnpm tsgo.User-visible / Behavior Changes
openrouter:autonow resolves to canonicalopenrouter/autoinstead ofanthropic/openrouter:auto.openrouter:freenow resolves to the first configured concrete free OpenRouter model instead ofanthropic/openrouter:free.pi-tuikey API.Security Impact (required)
Yes/No) NoYes/No) NoYes/No) NoYes/No) NoYes/No) NoYes, explain risk + mitigation:Repro + Verification
Environment
openrouter:auto,openrouter:free)agents.defaults.model.primaryset to the shorthand refs;agents.defaults.modelscontaining concreteopenrouter/...:freeentries for the free-model case.Steps
main, evaluateresolveConfiguredModelRef()withagents.defaults.model.primaryset toopenrouter:auto.{"provider":"anthropic","model":"openrouter:auto"}.openrouter:freeand configured concrete free OpenRouter models.{"provider":"anthropic","model":"openrouter:free"}.pnpm tsgoon currentmainand observegetKeybindingsimport failures in the two TUI selector components.Expected
pnpm tsgopasses.Actual
pnpm tsgofailed on removedgetKeybindingsimports.Evidence
Attach at least one:
Human Verification (required)
What you personally verified (not just CI), and how:
pnpm test -- src/agents/model-selection.test.ts -t 'resolveConfiguredModelRef';pnpm tsgo;pnpm build; full repo check chain viascripts/committer.openrouter:freeonly resolves from configured concreteopenrouter/...:freeentries; non-cancel TUI key handling was left untouched.Review Conversations
Compatibility / Migration
Yes/No) YesYes/No) NoYes/No) NoFailure Recovery (if this breaks)
bbd98e25a67b1789a6ba3e76cc3418c3203142ff.src/agents/model-selection.ts,src/agents/model-selection.test.ts,src/tui/components/filterable-select-list.ts,src/tui/components/searchable-select-list.tsRisks and Mitigations
openrouter:freeselection depends on configured model order.openrouter:freeresolves toanthropic/openrouter:free;openrouter:autoshould also be handled explicitly #57066 / prior PR fix: handle openrouter compatibility aliases #57078.