fix(runner): reject a spawn model missing from the machine's catalog - #140
fix(runner): reject a spawn model missing from the machine's catalog#140heavygee wants to merge 4 commits into
Conversation
Spawning with a model the agent does not have (`--agent cursor --model gpt-5`) booted a child that died at the agent handshake, leaving an archived session with zero turns and no explanation anywhere in the UI. Add a runner preflight next to the agent-availability check: when the machine already has a cached model catalog for the flavor, an id that is not in it fails the spawn with `model_unavailable` and a message naming the accepted ids (near-misses first). Cursor is the only flavor with a catalog readable without a probe today; every other flavor reports an empty catalog, which the validator treats as "unknown" and never rejects — a spawn must not block on `agent --list-models`, which can take 30s and contends with the ACP spawn lease. Fixes tiann#1752 via [HAPI](https://hapi.run) Co-Authored-By: HAPI <noreply@hapi.run>
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9dc0d41826
ℹ️ 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 sharedAgeMs = getSharedCursorModelsCacheAgeMs(); | ||
| if (sharedAgeMs !== null && sharedAgeMs <= CACHED_CATALOG_MAX_AGE_MS) { |
There was a problem hiding this comment.
In the runner startup path, scheduleCursorModelsPrewarm() calls listCursorModels(), which reads any existing shared cache through applyInMemoryCache() and writes the same catalog back, refreshing its file mtime without obtaining a new ACP catalog. Consequently this age check treats a stale catalog as fresh after every runner restart or model-list request, so a catalog predating a Cursor upgrade can indefinitely reject newly valid models and continue accepting removed ones. Store an observation timestamp that is preserved when reusing cached data, or refresh the ACP catalog before renewing it.
Useful? React with 👍 / 👎.
| // Renamed bases (grok-4.5 → cursor-grok-4.5) still reach spawn from stale hub | ||
| // rows and mobile drafts, where cursorStaleModelRemap resolves them; the | ||
| // preflight must not reject what that remap would have fixed. | ||
| const bases = [trimmed, cursorModelBaseId(trimmed), cursorCliSkuBaseId(trimmed)] |
There was a problem hiding this comment.
When the cache contains only one SKU variant, such as gpt-5.5-high, a request for the absent gpt-5.5-low is accepted because both candidates include the same stripped base. Checked against the Cursor spawn path: remapStaleCursorModelId() deliberately leaves non-legacy, non-exact SKU requests unchanged, so Cursor still receives the unavailable SKU and can fail during initialization, leaving precisely the empty archived session this preflight is intended to prevent. Bare base requests may use base matching, but explicitly suffixed SKUs should require an exact catalog entry.
Useful? React with 👍 / 👎.
| const trimmed = modelId.trim().toLowerCase() | ||
| const base = agent === 'cursor' ? cursorModelBaseId(trimmed) : trimmed | ||
| return WILDCARD_MODEL_IDS.has(trimmed) || WILDCARD_MODEL_IDS.has(base) |
There was a problem hiding this comment.
For Cursor, reducing the request to its bracketless base makes values such as default[fast=true] and auto[garbage] bypass catalog validation as wildcards. The actual spawn helpers in cursorAcpBackend.ts and cursorAcpRemoteLauncher.ts recognize only the exact values auto, default, and default[]; other bracketed forms are forwarded as explicit models. An absent malformed or stale model can therefore still reach Cursor and fail initialization, so wildcard matching should use only those exact supported spellings.
Useful? React with 👍 / 👎.
The spawn preflight dates the Cursor catalog by the shared cache file's mtime, but `listCursorModels` fed the file's own contents back through `applyInMemoryCache`, which writes it out again. Runner startup prewarm therefore re-stamped an arbitrarily old snapshot as current on every boot, so the 24h fail-open never triggered and a catalog from before a Cursor upgrade kept rejecting models that had become valid since. Only a live source (ACP snapshot or probe) may stamp the file now; the two shared-cache read branches update the in-process cache without persisting. Reported by the upstream PR review bot on tiann#1753. via [HAPI](https://hapi.run) Co-Authored-By: HAPI <noreply@hapi.run>
…variants Base collapsing let an explicit CLI sku through whenever any variant of its base existed, so `gpt-5.5-high-fast` passed a catalog offering only `gpt-5.5-medium`. remapStaleCursorModelId deliberately refuses to downgrade that pair and resolveCursorSpawnModel forwards the original, so the handshake failure this guard exists to prevent landed anyway. When the catalog enumerates CLI sku variants of the requested base, an explicit variant of that base must now match one exactly, and the error names the siblings that do exist. Scoped to CLI skus deliberately. Rejecting any variant the remap declines to rewrite - as first suggested - would reject every suffixed sku on this machine: its cached catalog is 74 ACP wire ids and zero CLI skus, and the remap returns null for `cursor-grok-4.5-fast` and `gpt-5.5-medium` alike there. A wire-only catalog is a different naming of the same models and says nothing about which skus exist, so those keep matching on base. Reported by the upstream PR review bot on tiann#1753. via [HAPI](https://hapi.run) Co-Authored-By: HAPI <noreply@hapi.run>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 410be9818a
ℹ️ 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 shared = readSharedCursorModelsCache(); | ||
| if (shared) { | ||
| return applyInMemoryCache(shared); | ||
| return applyInMemoryCache(shared, false); |
There was a problem hiding this comment.
When the runner starts with a shared catalog older than 24 hours, startup prewarm enters this branch and returns the cached response without reaching the ACP probe below, while getCachedCursorModelIds() subsequently discards that same response as expired. The follow-up now preserves the original mtime, but the fresh evidence is that validation therefore remains disabled until a successful Cursor session rewrites the cache, so the first unavailable-model spawn after a day of inactivity can again reach the handshake and leave an empty archived session. Detect the expired cache here and refresh it rather than returning it.
Useful? React with 👍 / 👎.
| const bases = [trimmed, cursorModelBaseId(trimmed), cursorCliSkuBaseId(trimmed)] | ||
| return [...new Set([...bases, ...bases.map(resolveCursorLegacyModelBase)])].filter(Boolean) |
There was a problem hiding this comment.
When the cache contains only ACP wires, an explicit request such as gpt-5.5[reasoning=high,fast=false] is accepted against a catalog containing only gpt-5.5[reasoning=medium,fast=false] because both entries contribute the same base candidate. The downstream remapStaleCursorModelId() requires every requested wire parameter to match and returns null in this case, after which resolveCursorSpawnModel() forwards the unavailable original ID to Cursor. Preserve base matching for bare model requests, but require a compatible parameter set for explicit wire requests.
Useful? React with 👍 / 👎.
…nstead 410be98 assumed a catalog listing CLI sku variants of a base was authoritative about that base's variants. It is not. Shared cliModelSkus rows are explicitly partial and unioned with a later probe, and that probe is skipped entirely while an ACP session holds the CLI lock - so a cache holding only `gpt-5.5-medium` would have rejected a perfectly valid `gpt-5.5-high` for as long as it stayed partial. That is a worse failure than the handshake error it pre-empted. Reverts to base-level matching and states the contract where it belongs: this guard rejects a model only when its base appears nowhere in the catalog. Variant-level availability stays the handshake's job. Tests pin both an unlisted sku and a contradictory bracket wire as allowed, so the scope is not re-narrowed by accident. Reported by the upstream PR review bot on tiann#1753. via [HAPI](https://hapi.run) Co-Authored-By: HAPI <noreply@hapi.run>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d5fa8963fa
ℹ️ 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".
| sources.push(cache.response.availableModels, cache.response.cliModelSkus); | ||
| const shared = readSharedCursorModelsCache(); | ||
| sources.push(shared?.availableModels, shared?.cliModelSkus); |
There was a problem hiding this comment.
When runner prewarm has populated cache.response and a later Cursor child overwrites the shared file with a changed catalog, these lines union the runner's old IDs with the new on-disk IDs while using the new file's mtime to declare both fresh. A model removed from Cursor therefore remains accepted until the runner restarts, reaches the handshake, and can still produce the empty archived session this preflight targets. Fresh evidence after the prior mtime fix is that a child-process write refreshes the file without replacing the runner process's cache.response; prefer the newest snapshot rather than combining catalog generations.
Useful? React with 👍 / 👎.
| const bases = [trimmed, cursorModelBaseId(trimmed), cursorCliSkuBaseId(trimmed)] | ||
| return [...new Set([...bases, ...bases.map(resolveCursorLegacyModelBase)])].filter(Boolean) |
There was a problem hiding this comment.
On a machine running a pre-rename Cursor build whose fresh catalog lists grok-4.5, a request for cursor-grok-4.5 is accepted because alias resolution is added to catalog candidates as well as request candidates. The downstream remapStaleCursorModelId() only translates the stale grok-4.5 spelling to cursor-grok-4.5; it returns null in the reverse direction and resolveCursorSpawnModel() forwards the unavailable original ID, so the handshake can still fail. Apply legacy expansion only to requested stale IDs rather than making the alias symmetric.
Useful? React with 👍 / 👎.
| function catalogCandidates(agent: AgentFlavor, modelId: string): string[] { | ||
| const trimmed = modelId.trim().toLowerCase() |
There was a problem hiding this comment.
When an API client or persisted draft supplies different casing, such as Composer-2.5 against catalog entry composer-2.5, lowercasing here makes the preflight accept it but does not change the model passed to the child. resolveCursorSpawnModel() and remapStaleCursorModelId() perform case-sensitive catalog matching and therefore return the original spelling, which is forwarded unchanged to agent --model; the preflight can consequently allow the same handshake failure it is intended to prevent. Either require exact casing or return the canonical catalog ID for spawning.
Useful? React with 👍 / 👎.
| const requested = model?.trim() ?? '' | ||
| if (!requested || isWildcardModelId(agent, requested)) return { ok: true } |
There was a problem hiding this comment.
For a request whose model is only whitespace, requested becomes empty and this branch treats it like no model, but the runner later tests the original options.model for truthiness and appends it to --model. In the Cursor path, resolveCursorSpawnModel() also returns the original value when its trimmed form is empty, and buildCursorAcpArgs() consequently emits --model with an empty argument, allowing another handshake failure instead of omitting the model. Normalize the value passed onward or reject this input rather than returning success here.
Useful? React with 👍 / 👎.
1 participant
Fixes tiann#1752
Problem
Spawning with a model the agent does not have —
--agent cursor --model gpt-5— booted a child that died at the agent handshake. The hub row was already created by then, so the result was an archived session with zero turns and no explanation anywhere: no error on the spawn call, nothing in the session, nothing in the UI.gpt-5is not incursor-agent --list-models(the catalog hasgpt-5.1…gpt-5.6-sol), but nothing on the spawn path checked.SpawnSessionRequestSchema.modelis an unconstrainedz.string().optional()and the runner forwards it straight to--model.Fix
A runner preflight beside the existing agent-availability check: when the machine already has a cached model catalog for the resolved flavor, an id that is not in it fails the spawn with
code: 'model_unavailable'and a message naming the accepted ids. Nothing is spawned, so no zombie row is created.Near-misses are listed first (a rejected
gpt-5is most usefully answered with thegpt-5.xids), and Cursor entries collapse to the base slug so the list is not spent ongpt-5-mini[fast=false]-style variants.Deliberately conservative
The preflight rejects only what it can prove wrong:
agent --list-models, which can take 30s and contends with the ACP spawn lease.[]and is never rejected. Adding a flavor is one line ingetCachedModelCatalog.grok-4.5→cursor-grok-4.5) resolve throughresolveCursorLegacyModelBasethe same waycursorStaleModelRemapdoes, comparison is case-insensitive, andauto/default/default[]are always allowed.Tests
shared/src/spawnModelCatalog.test.ts— 11 cases over the matching rules: rejection message and its ordering, exact/base/sku/legacy-alias/case-insensitive acceptance, wildcards, unknown catalog, and that Cursor sku-suffix stripping is not applied to other flavors.cli/src/runner/spawnModelPreflight.test.ts— 7 cases over the cached-catalog reader: the#1752repro, ACP snapshot and on-disk sources, the 24h freshness bound, flavors with no catalog, and no model at all.Red-green verified: stubbing
validateSpawnModelAgainstCatalogto always returnokfails 4 shared + 2 CLI tests.bun typecheckclean.test:cli2473 pass,test:shared309 pass,test:web2859 pass,test:relay80 pass.test:hubhas 2 failures (TitleSuggestionService,resolveFcmConfig) that reproduce identically with this change reverted and pass when those files run in isolation — pre-existing cross-file pollution, untouched here.Also verified against this machine's real
~/.hapi/cache/cursor-models.json:gpt-5rejected;gpt-5.5,GPT-5.5,grok-4.5,composer-2.5,default[]and no-model all allowed;claude --model gpt-5unaffected.Note
Agent CLI
--helpexamples can drift from--list-modelsand from the hub catalog — this is what sentgpt-5down the spawn path in the first place. This change makes that drift fail loudly at spawn instead of silently at handshake.🤖 Generated with Claude Code