Skip to content

fix(providers): make the model picker reflect what the CLI actually accepts - #222

Merged
j35dev merged 7 commits into
mainfrom
fix/model-catalog-integrity
Sep 14, 2026
Merged

j35dev merged 7 commits into
mainfrom
fix/model-catalog-integrity

Conversation

@j35dev

@j35dev j35dev commented Sep 13, 2026 •

Copy link
Copy Markdown
Owner

The bug

The model picker could offer a model the harness would not accept, and the drop happened at log.debug. The turn ran on the agent's default while the composer kept showing the user's pick — silently, every time.

Four defects, all shipping in v0.3.1 (74e85e2):

Defect Fix
D1 applyModel swallowed a refusal; the picker offered models the agent never advertises Outcome reported + a new non-fatal notice event; the session's own agent now supplies the live catalog
D2 #writeDiskCache persisted the post-probe merged overlay under the models.dev provider key Writes only what the registry round actually carried
D3 CLAUDE_ALIASES prepended at two sites onto lists already containing those families Version-less pointers ride as aliases; duplicate names collapse; superseded members become isLegacy
D4 findBinary was a bare existsSync, so a directory named codex was a provider row Regular-file check, plus a readiness gate on the rail

The guarantee

Ari automatically reflects every model exposed by your configured CLI — with a clearly marked cached fallback.

The picker records each catalog's provenance and names a non-live list for what it is ("… has not reported its own models — this is a bundled list. It may not accept every entry.") with a refresh control, rather than presenting a guess as fact.

Why discovery moved to the session

The probe runs --no-install from homedir(). When the npx package isn't cached the probe fails while a real turn still works — and the snapshot fallback then names models the agent refuses outright. On this machine the bundled Claude ids and what the local claude agent actually advertises share zero entries.

I pre-scoped a fix changing the probe's cwd to the session workspace and measured it instead: cwd=C:\Users\user and cwd=D:/Projects/Ari both return default, opus[1m], sonnet, sonnet[1m], haiku, deepseek-v4-flash. No difference, so no change. The real fix is the better direction anyway — an ACP session installs the model list its own agent advertises as the kind's live catalog, so the picker offers what the runtime that will actually run accepts.

Also fixed along the way

  • Ari's placeholder id 'default' was conflated with an agent's genuinely advertised default option, making "switch back to the CLI default" a silent no-op.
  • A kind already serving live data is no longer re-probed on every refresh (LIVE_RECHECK_MS, 6h). Re-asking spawned an agent process per kind per minute to hear the same list. A kind still on a fallback keeps retrying on the existing 60s throttle.

Contract changes

notice is a new AgentEvent variant (packages/contracts/src/agent-event.ts). It is deliberately not error: error sets firstErrorMessage in engine.ts and settles the turn as failed, and a turn that ran on the wrong model still ran. Anything switching exhaustively over AgentEvent needs a case. catalogModelSchema gains optional aliases and isLegacy.

Known, not fixed — worth a look

  • Codex shows 11 registry rows. models.dev's family values are per-variant (gpt-sol, gpt-pro), not per-generation, so the collapse doesn't fire. Applying CURRENT_MODEL_IDS as isLegacy reclassification gets it to ~5 but hides newly-released models until the static allowlist is refreshed — against the guarantee above. Left as-is; the row count is a product call.
  • opencode's live probe returns 392 models, uncapped. MAX_MODELS_PER_PROVIDER = 60 exists only in scripts/update-model-snapshot.ts.
  • scripts/update-model-snapshot.ts now emits family, but catalog-snapshot.json has not been regenerated, so the fallback path uses id-prefix matching (covered by tests).

Testing

TDD throughout — every production change was preceded by a test watched to fail for the right reason. Mutation-checked the drift guard: forcing ModelDriftWatch.observe to drop its current === this.#agentModel false-positive guard fails exactly one test ("stays quiet when the agent just re-advertises the model already in use"), confirming that test is load-bearing rather than vacuous.

pnpm verify green, exit 0: typecheck 7/7 projects, eslint clean, tests — shared 15, contracts 21, ui 159, providers 440, engine 164, ari-core 260, desktop 1365 passed / 3 skipped.

Stacking note

Based on main by request. The branch also carries 7987186 fix(desktop): bind control socket outside macOS userData, which is fix/macos-control-socket-einval's commit (that branch is pushed but has no PR of its own) — plus a separate commit, test(desktop): derive the music runtime key from the runtime itself, an unrelated test fix that was sitting uncommitted on the branch.

🤖 Generated with Claude Code


Devin Review

petros-double-test1 and others added 3 commits September 13, 2026 14:14
`sockaddr_un.sun_path` is 104 bytes on macOS (108 on Linux), NUL terminator
included, and libuv returns EINVAL rather than truncating a path that does not
fit. macOS userData spends 70 of those bytes before the UUID and `.sock` are
added — `/Users/<name>/Library/Application Support/@ari/desktop/agent-control/`
— so `server.listen()` always rejected and every dispatch failed with
`listen EINVAL: invalid argument`, leaving macOS users unable to send prompts.

Bind POSIX sockets in a short private directory under /tmp instead, keeping the
whole path near 60 bytes, and remove it on close. Windows keeps its named pipe.

Covers the fix with tests that reconstruct the overflowing path and pin the
shipped socket root, and adds macOS to the verify matrix so platform-specific
transport failures surface on PRs rather than at release.

Co-Authored-By: Claude <noreply@anthropic.com>
…ccepts

The picker could offer a model the harness would not accept, and the drop
happened at `log.debug`: the turn ran on the agent default while the
composer kept showing the user's pick. Four defects, all shipping in
v0.3.1:

- `applyModel` now reports its outcome. When the agent does not offer the
  requested model the turn runs anyway and says so in the transcript via a
  new `notice` agent event. `error` was wrong for this: it settles the turn
  as failed, and a turn that ran on the wrong model still ran.
- Closed from the better side too: an ACP session installs the model list
  its own agent advertises as the kind's live catalog, so the picker offers
  what the runtime that will actually run accepts. This matters most when
  the throwaway probe fails (uncached npx package under `--no-install`)
  while a real turn still works; the snapshot fallback then names ids the
  agent refuses outright.
- `#writeDiskCache` persisted the post-probe merged overlay under the
  models.dev provider key, so one session's probe output outlived it as
  vendor registry data. It now writes only what the registry round carried.
- Version-less family pointers (`opus`, `sonnet`) ride as aliases on the
  newest row instead of appearing as rows of their own; duplicate display
  names collapse; superseded family members become `isLegacy` rather than
  disappearing. Collapsing happens at read time so every source gets the
  same treatment and stored catalogs stay as-reported.
- `findBinary` used a bare `existsSync`, so a *directory* named `codex` was
  a provider row. It is now a regular-file check, and the rail gates on
  installation and auth rather than on a non-null path alone.

The picker records each catalog provenance and marks a non-live list as
such, with a refresh control: the guarantee is that Ari reflects what the
configured CLI exposes, with a clearly marked cached fallback rather than
a silent one.

Also fixed: Ari's placeholder id `default` was conflated with an agent's
genuinely advertised `default` option, making a switch back to the CLI
default a silent no-op. And a kind already serving live data is no longer
re-probed on every refresh (6h window); a kind still on a fallback keeps
retrying on the existing 60s throttle.

Co-Authored-By: Claude <noreply@anthropic.com>
The self-heal suite seeded a `win32-x64` / `linux-x64` key from a
win32-vs-not branch, so on macOS it wrote a manifest under a key
`MusicRuntime` never looks up. The runtime then found no binary and every
test in the block returned RUNTIME_MISSING instead of the failure it meant
to exercise. Derive the key from `musicRuntimeTargetKey()` and fail loudly
on a platform the runtime does not support, so the fixture cannot drift
from the lookup again.

Unrelated to the model-catalog work in the previous commit; it was sitting
uncommitted on this branch.

Co-Authored-By: Claude <noreply@anthropic.com>
devin-ai-integration[bot]

This comment was marked as resolved.

`controlEndpoint` creates the private /tmp/ari-* directory before the
launcher write, the workspace setup and `server.listen()`, but the only
removal of it lives in the `close` method of the object a successful
start returns. Any rejection in between therefore left the directory
behind — and because the same startup failure repeats on every launch,
they accumulate one per attempt.

Split the builder out and remove the directory when it rejects. `close`
still takes ownership once the builder resolves, so the happy path is
unchanged.

Co-Authored-By: Claude <noreply@anthropic.com>
devin-ai-integration[bot]

This comment was marked as resolved.

petros-double-test1 and others added 3 commits September 13, 2026 15:46
`statSync` with `throwIfNoEntry: false` suppresses a missing entry, but
an unusable one still throws — a path through a file raises ENOTDIR, a
directory that denies traversal raises EACCES. That escaped `findBinary`
and aborted the whole scan, so one bad PATH entry hid every provider
installed in the directories after it.

Treat an unreadable candidate as a miss, which is what the scan already
does for a missing one.

Co-Authored-By: Claude <noreply@anthropic.com>
`AgentControlServer.listen` binds before it applies the socket's 0600
mode, so a rejection at that second step leaves the server listening —
and because `startControlTransport` never returns, nothing can call the
`close` that would release the handle. The listening handle outlives the
startup failure for the life of the process.

Close it on the way out, without masking the error that caused it. The
caller still removes the socket directory.

Co-Authored-By: Claude <noreply@anthropic.com>
Adding the macOS leg moved this job's check from `verify` to
`verify (ubuntu-latest)` / `verify (macos-latest)`. The `protect main`
ruleset requires a check named exactly `verify`, and a matrix job cannot
produce that name — so every PR would have been permanently blocked the
moment this landed on main.

Run the matrix under its own name and carry the required one on an
aggregator that runs regardless and fails when any leg does.

Co-Authored-By: Claude <noreply@anthropic.com>
@j35dev
j35dev merged commit 01ecf04 into main Sep 14, 2026
7 checks passed
@j35dev
j35dev deleted the fix/model-catalog-integrity branch September 14, 2026 14:14
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.

2 participants