Skip to content

fix(cli): show model display names in picker - #3493

Open
rbalachandar wants to merge 4 commits into
apache:mainfrom
rbalachandar:fix/cli-model-display-names
Open

fix(cli): show model display names in picker#3493
rbalachandar wants to merge 4 commits into
apache:mainfrom
rbalachandar:fix/cli-model-display-names

Conversation

@rbalachandar

Copy link
Copy Markdown

Summary

The cross-connection TUI /model picker now prefers each catalog model’s human-readable displayName, while retaining the raw model id when no display name is available. Search matches both model ids and display names.

Fixes #3482

Verification

  • npm --workspace maka-agent test (372 tests passed)
  • npm --workspace maka-agent run typecheck
  • npx biome lint packages/cli/src/pi-tui-contracts.ts packages/cli/src/runtime-host-onboarding.ts packages/cli/src/pi-tui-pickers.ts packages/cli/src/__tests__/runtime-host-onboarding.test.ts packages/cli/src/__tests__/pi-tui-runner.test.ts
  • npx biome format packages/cli/src/pi-tui-contracts.ts packages/cli/src/runtime-host-onboarding.ts packages/cli/src/pi-tui-pickers.ts packages/cli/src/__tests__/runtime-host-onboarding.test.ts packages/cli/src/__tests__/pi-tui-runner.test.ts

The TUI runner test covers display-name rendering, id fallback, and searching by the displayed model name.

AI use

  • No generative tool made a substantive contribution
  • Generative tooling made a substantive contribution

Tool(s) and scope: OpenCode made the code and test changes, reviewed the implementation, and ran the verification commands.

Checklist

  • Tests cover the change and fail without it
  • Lint, format, typecheck and the affected suites pass locally

Does this PR entail a change in behavior?

  • Yes — described under Summary above
  • No

@me2seeks

Copy link
Copy Markdown
Contributor

Nice cleanup — the projection via modelsById, the displayName?.trim() || model label rule and the stale-comment updates all look right to me.

One substantive nit before merge: the model-id match criterion lost its dedicated test case, so one line of matchesModelChoice is no longer pinned by any test.

The five-case list previously isolated each match criterion one-by-one; query: 'gpt' was the case that exercised matching by model id:

- { query: 'gpt', keep: 'gpt-5.5', drop: ['glm-max', 'text-unicorn'] },
+ { query: 'preview', keep: 'GPT 5.5 Preview', drop: ['GLM Max', 'text-unicorn'] },

After the swap, every remaining query is satisfied by other lines:

function matchesModelChoice(choice: ModelChoice, query: string): boolean {
  if (choice.model.toLowerCase().includes(query)) return true;          // ← now unpinned
  if (choice.displayName?.toLowerCase().includes(query)) return true;   // 'preview' hits here
  if (choice.connectionName.toLowerCase().includes(query)) return true; // 'aurora'
  if (choice.connectionSlug.toLowerCase().includes(query)) return true; // 'alpha'
  if (choice.providerType.toLowerCase().includes(query)) return true;   // 'zai'
  const providerLabel = PROVIDER_DEFAULTS[choice.providerType]?.label;  // 'gemini'
  ...
}

Deleting the choice.model line would keep this suite green. That matters because models without catalog metadata (minimal hosts, custom relay ids) are exactly the ones users must still be able to find by raw id.

Suggested fix — add an id-only query whose substring does not occur in any display name:

// 'glm-max' occurs in the model id but not in the display name ('GLM Max'),
// so this case can only pass through the model-id criterion.
{ query: 'glm-max', keep: 'GLM Max', drop: ['GPT 5.5 Preview', 'text-unicorn'] },

(Optionally also assert the inverse rendering invariant somewhere — that a choice with a displayName no longer shows its raw id — but the implicit fallback via text-unicorn already covers the happy path.)

@Astro-Han Astro-Han left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approving 141534485a0addc6a2747d3b6bc7cb55e428916c. Required test is completed / success bound to that exact SHA. No P0–P3.

One note on how this got here: CI had never run on this head — zero check runs, one workflow sitting at action_required. That state renders like "in progress" and is easy to misread as pending; it was approved as a maintainer and is green now. Flagging it because it was the reviewing line that caught it rather than assuming the absence of a red mark meant a green one.

What was verified:

  • displayName is a projection, never an identity. The picker still carries value: String(index) and resolves back to the ModelChoice; model remains the authoritative key and displayName only reaches label. Nothing selects, persists, or matches on the display name.
  • One source, one lookup. modelsById.get(model)?.displayName reads from the same catalog object as model, in the same lookup — not a second fetch that could disagree with it. Two call sites, no third copy.
  • The fallback is displayName?.trim() || choice.model. The .trim() matters: a catalog entry with a whitespace-only display name falls back to the id rather than rendering a blank row. ?? would not have.

The part worth calling out is the one that is not cosmetic. This also widens search matching — if (choice.displayName?.toLowerCase().includes(query)) return true; — and correspondingly updates the comment that used to say ModelChoice carries no display name so the id is the only model-side match target. This is the one place where display names cross out of presentation and into behavior, so it deserves to be described as such rather than folded into "show display names."

Good that { query: 'glm-max', keep: 'GLM Max' } was added alongside the display-name queries: it pins that widening the match surface did not displace plain id matching, which is the regression this change could plausibly have introduced.

Reviewed at 2026-08-23 12:55 UTC.

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.

TUI /model picker shows raw model ids; Desktop shows display names (inconsistent labels across surfaces)

3 participants