Conversation
The model pickers were fed entirely by curated lists — the catalog baked into the build, overlaid by docs/model-catalog.json. Neither knows what this machine can reach, which depends on the user's account, plan and keys: `opencode models` reports ~380 slugs against the user's own providers, and `agy models` reports whatever the logged-in Antigravity account is entitled to. A model you hold a key for was simply absent until someone hand-added it upstream. So ask the CLIs, and append their answer to the curated list. The curated list stays the floor: it carries the readable labels, the editorial ordering and any version bounds, and it wins every collision. A CLI that is missing, logged out or slow contributes nothing, which degrades to exactly the behaviour that shipped before. Three things this had to get right, each of which is a test: - agy's `--model` takes the DISPLAY NAME, not the slug — which is why every curated antigravity entry carries an id like `Gemini 3.1 Pro (High)`. Reading the slug column yields a value agy rejects AND a dedupe that never matches its curated twin, so the picker fills with duplicates that cannot be selected. - A packaged app does not inherit the login shell's PATH. Launched from the desktop, an .AppImage or .deb never found `agy` or `opencode` and the probe failed silently, so the user's real PATH is injected. - `agy models` round-trips to Google and takes ~6s, so the 5s default timeout returned empty on a healthy machine. Raised to 15s, and the answer is cached for 10 minutes — only when non-empty, so one transient failure does not pin the pickers short for the whole window. The picker itself could not take the result. It rendered one chip per model into an unbounded flex-wrap row with no scroll cap, so 391 entries pushed the rest of the form off the modal. The chip grid moves to a shared ModelChipPicker that grows a filter box and a scroll cap past 24 options and is byte-identical below it, so every curated provider looks untouched. The remaining pickers are native selects, which handle long lists on their own; two were keyed on label, which is safe only until a live list merges in, and are now keyed on id. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Contributor
|
✅ Evidence received. Before and after are both attached. Thanks — this is what makes a PR reviewable in one pass. |
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.
What & why
The model pickers are fed entirely by curated lists —
src/shared/modelCatalog.jsonbaked into the build, overlaid bydocs/model-catalog.json. Neither knows what the machine in front of the user can actually reach, which depends on their account, plan and keys. On this machineopencode modelsreports 384 slugs andagy modelsreports 14; the pickers offered 8 and 12. A model you hold a key for was simply absent until someone hand-added it upstream.So ask the CLIs, and append their answer to the curated list. The curated list stays the floor and wins every collision — it carries the readable labels, the editorial ordering (recommended picks first) and any version bounds, and the CLI only knows the raw id. Only the appended tail is sorted, because a CLI emits its list in no order a human scrolling 380 rows can predict. A CLI that is missing, logged out or slow contributes nothing, degrading to exactly the behaviour that ships today.
Three things this had to get right, each of which is a test in
test/model-catalog-cli.test.cjs:agy --modeltakes the DISPLAY NAME, not the slug — which is why every curated antigravity entry already carries an id likeGemini 3.1 Pro (High). Reading the slug column yields both a value agy rejects and a dedupe that never matches its curated twin, so the picker fills with duplicates that cannot be selected. A row with no display name is dropped rather than backfilled from its slug, since the slug is precisely the value agy refuses..AppImageor.debnever foundagyoropencodeand the probe failed silently, so the user's real PATH is injected via the existinguserShellPath().agy modelsround-trips to Google and takes ~6s, so the 5s default timeout returned empty on a healthy machine. Raised to 15s, and the answer is cached for 10 minutes — cached only when non-empty, so one transient failure doesn't pin the pickers short for the whole window.The CLI probing, parsing and merge live in
src/main/modelCatalog.tsbeside the remote-catalog loader as pure exported functions, with the IPC handler reduced to wiring — that's what makes them testable without spawning anything.On "one change per PR"
This touches main and renderer, so it's worth saying why it isn't splittable. The picker could not physically take the result:
AddAgentModal/EditAgentModalrendered one chip per model into an unboundedflex-wraprow with no scroll cap, so 391 entries push the rest of the form off the modal. Landing the fetch alone would ship a broken Add Agent dialog. The chip grid therefore moves to a sharedModelChipPickerthat grows a filter box and a scroll cap past 24 options and is byte-identical below that threshold, so every curated provider looks exactly as it does today.Happy to split it if you'd rather review them separately — say the word and the picker change becomes the prerequisite PR.
Type of change
Evidence
Both shots are the Add Agent → Engine step: same window size, same theme, same harness config, taken from
npm run devon this branch's parent and on the branch.Before
After
How this was tested
OS: Zorin OS 18.1, kernel 7.0.0-31-generic, X11. Electron 32.3.3, Node 22.22.3. Linux only — I have no macOS or Windows machine to verify on, and the Windows path is reasoned about below rather than run.
npm run typecheck— clean (both node and web TS projects).npm run test:focused— 849 pass, 0 fail (834 existing + 15 new).npm run build— clean.opencode8 curated + 383 appended = 391;antigravity12 curated + 6 appended = 18; zero duplicate ids in either.~/.local/binto confirm the packaged-PATH fix works end to end — this is the path that was silently failing, so it needed a packaged run, not a dev run.New i18n keys are added to all three locales (
en,ar,zh-CN).Agent review of this PR
Run as asked, against the brief in CONTRIBUTING (cross-platform, paths with spaces, provider neutrality, hot paths, security). What it flagged, including what I chose not to change:
Fixed as a result
parseAgyModelshad a|| slugfallback for rows with a blank display name that the line-level.trim()made unreachable — and had it fired, it would have put a slug on the command line, which is the exact value agy rejects. The bug this PR exists to fix, rebuilt in the error path. Rows with no display name are now dropped explicitly.<select>s (CommandCenterPanel,OnboardingWizard) were keyed onm.label. Safe only until a live list merges in and two entries share a label; keyed onm.idnow.Flagged, not changed — with reasons
Gemini 3.1 Pro (High)). That's pre-existing and correct —buildSpawnCommandquotes them and the shared tokenizer keeps them whole. This PR adds more such ids but no new handling, so it rides on the existing path rather than a new one.probeClipassesshell: process.platform === 'win32'and resolves through the existingresolveCommand, matching how every other CLI spawn in main already works. Unverified on Windows — I can't run it. A reviewer with a Windows box should sanity-check the probe before this is relied on there.opencodeandantigravityare probed, because those are the two CLIs whose live list I can actually verify. The probe table inloadCliModelsis one line per provider, socursor-agent models/grok modelsare a cheap follow-up — deliberately not in this PR.models:catalogis not hot, but it was spawning two processes per call and blocking ~7s on the network. Now cached for 10 minutes, and only non-empty answers are cached so a transient failure doesn't pin the pickers short.modelsargument throughresolveCommand, and CLI stdout becomes picker labels and--modelvalues that already flow through the existing quoting. Worth a reviewer's eye regardless, since this is the first place third-party CLI stdout becomes a spawn argument.ModelChipPickeruses--cth-*colour and font variables throughout, but its 168px scroll cap and4px 8pxinput padding are literals copied from the chips it replaces rather than thespacescale exported fromtokens.ts. Consistent with the surrounding code, still a deviation from the DESIGN.md rule. Happy to convert if you'd prefer the scale used.No new runtime dependencies. No reformatting — the diff is only lines this change needed. Debug logging added during development was removed before the commit.