Skip to content

fix(runner): reject a spawn model missing from the machine's catalog - #1753

Open
heavygee wants to merge 4 commits into
tiann:mainfrom
heavygee:fix/spawn-model-validate-1752
Open

fix(runner): reject a spawn model missing from the machine's catalog#1753
heavygee wants to merge 4 commits into
tiann:mainfrom
heavygee:fix/spawn-model-validate-1752

Conversation

@heavygee

@heavygee heavygee commented Sep 2, 2026

Copy link
Copy Markdown
Collaborator

Fixes #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-5 is not in cursor-agent --list-models (the catalog has gpt-5.1gpt-5.6-sol), but nothing on the spawn path checked.

SpawnSessionRequestSchema.model is an unconstrained z.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.

Model 'gpt-5' is not in the cursor model catalog on this machine.
Accepted: gpt-5-mini, gpt-5.1, gpt-5.2, gpt-5.3-codex, gpt-5.4, gpt-5.4-mini,
gpt-5.4-nano, gpt-5.5, gpt-5.6-luna, gpt-5.6-sol, gpt-5.6-terra, claude-fable-5, … (36 total)

Near-misses are listed first (a rejected gpt-5 is most usefully answered with the gpt-5.x ids), and Cursor entries collapse to the base slug so the list is not spent on gpt-5-mini[fast=false]-style variants.

Deliberately conservative

The preflight rejects only what it can prove wrong:

  • Never probes. It reads the ACP snapshot, the in-process cache and the on-disk shared cache — nothing else. A spawn must not block on agent --list-models, which can take 30s and contends with the ACP spawn lease.
  • Empty catalog means "unknown", not "no models". Cursor is the only flavor whose catalog is readable without a probe today; every other flavor reports [] and is never rejected. Adding a flavor is one line in getCachedModelCatalog.
  • Stale catalogs go dormant. A cached catalog older than 24h is dropped rather than trusted: a runner can stay up for weeks, and a catalog predating a Cursor upgrade would otherwise reject an id that is now valid.
  • Matching is as permissive as the spawn path. ACP wire ids and CLI skus collapse to their base slug, renamed bases (grok-4.5cursor-grok-4.5) resolve through resolveCursorLegacyModelBase the same way cursorStaleModelRemap does, comparison is case-insensitive, and auto / 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 #1752 repro, ACP snapshot and on-disk sources, the 24h freshness bound, flavors with no catalog, and no model at all.

Red-green verified: stubbing validateSpawnModelAgainstCatalog to always return ok fails 4 shared + 2 CLI tests.

bun typecheck clean. test:cli 2473 pass, test:shared 309 pass, test:web 2859 pass, test:relay 80 pass. test:hub has 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-5 rejected; gpt-5.5, GPT-5.5, grok-4.5, composer-2.5, default[] and no-model all allowed; claude --model gpt-5 unaffected.

Note

Agent CLI --help examples can drift from --list-models and from the hub catalog — this is what sent gpt-5 down 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

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>
@heavygee heavygee added the low-impact Focused / low-blast-radius change; estate lane B promote candidate (PR merge policy) label Sep 2, 2026

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Findings

  • [Major] Cache reads can make an arbitrarily stale catalog look fresh - the new preflight trusts the shared file's mtime at cli/src/modules/common/cursorModels.ts:399, but runner startup calls scheduleCursorModelsPrewarm() (cli/src/runner/run.ts:1202). When a shared cache exists, listCursorModels() loads it at cli/src/modules/common/cursorModels.ts:335 and applyInMemoryCache() writes the same catalog back at cli/src/modules/common/cursorModels.ts:294. That renews the mtime without obtaining a fresh ACP catalog, so even a cache older than 24 hours becomes trusted again and can reject a valid model added since the original snapshot.
    Suggested fix:
    async function applyInMemoryCache(
        response: ListCursorModelsResponse,
        persist = true
    ): Promise<ListCursorModelsResponse> {
        const enriched = await enrichCursorModelsWithCliSkus(response)
        if ((enriched.availableModels?.length ?? 0) > 0) {
            cache.expiresAt = Date.now() + CACHE_TTL_MS
            cache.response = enriched
            if (persist) writeSharedCursorModelsCache(enriched)
        }
        return enriched
    }
    
    // Use persist=false in both branches that loaded response from the shared file.
    if (shared) return applyInMemoryCache(shared, false)

Summary
Review mode: initial

  • One Major issue: startup prewarm can revalidate an arbitrarily stale Cursor catalog, causing the new guard to block valid session spawns.
  • Residual testing gap: no test loads/prewarms an expired shared cache before calling checkSpawnModel.

Testing

  • Not run (automation/security constraint). GitHub integration and fixture checks passed; the main test job was still in progress when reviewed.

HAPI Bot

// for weeks, and a catalog that predates a Cursor upgrade would reject an id
// that is now valid.
const sharedAgeMs = getSharedCursorModelsCacheAgeMs();
if (sharedAgeMs !== null && sharedAgeMs <= CACHED_CATALOG_MAX_AGE_MS) {

@github-actions github-actions Bot Sep 2, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

[MAJOR] A cache read can renew stale catalog freshness

This age check is based on file mtime, but runner startup prewarm calls listCursorModels(); its shared-cache branches pass the old response through applyInMemoryCache(), which writes it back and updates that mtime. No fresh ACP catalog was obtained, so an arbitrarily old snapshot becomes trusted for another 24 hours and can reject newly available models.

Suggested fix:

async function applyInMemoryCache(
    response: ListCursorModelsResponse,
    persist = true
): Promise<ListCursorModelsResponse> {
    const enriched = await enrichCursorModelsWithCliSkus(response)
    if ((enriched.availableModels?.length ?? 0) > 0) {
        cache.expiresAt = Date.now() + CACHE_TTL_MS
        cache.response = enriched
        if (persist) writeSharedCursorModelsCache(enriched)
    }
    return enriched
}

// In both shared-cache read branches:
if (shared) return applyInMemoryCache(shared, false)

Please also add a regression test that ages the shared file, runs the prewarm/list path, then verifies an unknown model remains allowed.

heavygee added a commit to heavygee/hapi that referenced this pull request Sep 2, 2026
…a turn

The final check was messages>=1, but the wrapper's own remit ping is that
message. A peer whose agent died at startup — a rejected --model spawns a
child that never survives the agent handshake — archived with our ping as
its only turn and the wrapper still printed OK. That is exactly the empty
shell this wrapper exists to catch, one turn later.

`active` alone would not have fixed it: the hub marks a session active
when the CLI socket connects, and sessionFactory connects that socket
before launching the agent, so every spawn reads active the moment we
have its id. What separates a live peer from a corpse is what happens
next — a dead child sends session-end or stops heartbeating, and the hub
drops it inactive within ~30s.

So: poll for an agent turn (a message beyond our ping, or thinking) and
leave early on it; otherwise wait that window out and require the session
to be still active at the end. Undelivered remit still exits 4;
delivered-onto-a-corpse now exits 5 with a diagnosis pointing at the
machine's model catalog rather than a CLI --help example. The OK line
carries which proof it got. A non-numeric
HAPI_SPAWN_PEER_VERIFY_TIMEOUT_S is now a usage error up front instead of
a set -e crash after the remit has already gone out.

Adds hapi-spawn-peer.test.sh, which drives the wrapper against a stub hub
modelled on the real one (active from t=0 in every scenario) across live,
dead, quiet, silent and bad-timeout. Red-green verified: on the pre-fix
wrapper the dead-agent case exits 0.

Context: upstream tiann#1752, tiann#1753

via [HAPI](https://hapi.run)

Co-Authored-By: HAPI <noreply@hapi.run>
@heavygee heavygee added bug Something isn't working area:cli CLI, runner, agent wrappers labels Sep 4, 2026
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>
@heavygee

heavygee commented Sep 4, 2026

Copy link
Copy Markdown
Collaborator Author

Good catch — confirmed and fixed in d6cd148.

listCursorModels was feeding the shared cache file's own contents back through applyInMemoryCache, which writes it out again, so startup prewarm re-stamped an arbitrarily old snapshot as current on every boot. The 24h fail-open could never fire.

Applied the suggested shape: applyInMemoryCache(response, persist = true), with persist: false in both branches that loaded response off the shared file (listCursorModels and listCursorModelsWhileAcpActive). Only a live source — ACP snapshot or probe — may stamp the file now. Added a comment at the function stating that invariant so a future edit doesn't quietly reintroduce it.

Regression test added as requested (cli/src/runner/spawnModelPreflight.test.ts), modelling a fresh runner: cold in-process cache, leftover shared file, aged 2 days, then the prewarm path.

writeSharedCursorModelsCache({ success: true, availableModels: [{ modelId: 'composer-2.5' }], currentModelId: 'composer-2.5' })
ageSharedCache(2 * 24 * 60 * 60 * 1000)
await listCursorModels()
expect(checkSpawnModel('cursor', 'gpt-5')).toEqual({ ok: true })

Red-green verified: reverting just the two persist: false arguments fails exactly this test and nothing else.

bun typecheck clean. cli 2474 pass, shared 309, web 2859, relay 80. The 2 hub failures (TitleSuggestionService, resolveFcmConfig) are pre-existing cross-file pollution — they reproduce identically with this whole PR reverted and pass when those files run alone.

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Findings

  • [Major] Explicit Cursor variants are accepted when only a different variant exists — catalogCandidates() reduces both the request and every catalog entry to the same base, so gpt-5.5-high-fast passes against a catalog containing only gpt-5.5-medium. Existing spawn resolution deliberately returns no remap for that pair (shared/src/cursorCliSku.test.ts:182), and resolveCursorSpawnModel() then forwards the original unavailable SKU (cli/src/cursor/utils/cursorStaleModelRemap.ts:49), recreating the handshake failure this PR is meant to prevent. Evidence: shared/src/spawnModelCatalog.ts:30.
    Suggested fix:
    const normalizedCatalog = catalog.map((modelId) => ({ modelId: modelId.trim().toLowerCase() }))
    const explicitCursorVariant = agent === 'cursor'
        && (isCursorAcpWireModelId(requested) || isCursorCliSkuVariantId(requested))
    const matches = explicitCursorVariant
        ? remapStaleCursorModelId(requested.toLowerCase(), normalizedCatalog) !== null
        : catalogCandidates(agent, requested).some((candidate) => index.has(candidate))
    if (matches) return { ok: true }

Questions

  • None.

Summary
Review mode: follow-up after new commits

  • The prior stale-mtime finding is fixed and covered by the new prewarm regression test.
  • One Major issue remains: unavailable explicit Cursor variants can still bypass the preflight because matching collapses them to a base model.

Testing

  • Not run (automation/security constraint). Add a regression proving gpt-5.5-high-fast is rejected when the cached CLI catalog only contains gpt-5.5-medium, while an exact variant and a bare base remain accepted. At review time, fixture drift and integration checks passed; the main test job was still in progress.

HAPI Bot

// 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)]

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

[MAJOR] Preserve explicit Cursor variant identity

This collapses both requested and catalog SKU variants to their base. For example, gpt-5.5-high-fast therefore matches a catalog containing only gpt-5.5-medium. That conflicts with the existing spawn resolver, whose regression test explicitly requires this pair to remain unresolved (shared/src/cursorCliSku.test.ts:182); resolveCursorSpawnModel() then falls back to the original SKU at cli/src/cursor/utils/cursorStaleModelRemap.ts:49, so the child can still die during handshake.

Suggested fix:

const normalizedCatalog = catalog.map((modelId) => ({ modelId: modelId.trim().toLowerCase() }))
const explicitCursorVariant = agent === 'cursor'
    && (isCursorAcpWireModelId(requested) || isCursorCliSkuVariantId(requested))
const matches = explicitCursorVariant
    ? remapStaleCursorModelId(requested.toLowerCase(), normalizedCatalog) !== null
    : catalogCandidates(agent, requested).some((candidate) => index.has(candidate))
if (matches) return { ok: true }

Please replace the current cross-variant acceptance test with a regression for gpt-5.5-high-fast versus gpt-5.5-medium, plus exact-variant and bare-base cases.

…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>
@heavygee

heavygee commented Sep 4, 2026

Copy link
Copy Markdown
Collaborator Author

Fixed in 410be98 — but not with the suggested form, which I have to push back on.

The finding is real. Base collapsing let an explicit 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 refuses to downgrade that pair by design and resolveCursorSpawnModel forwards the original, so the handshake failure landed anyway. Exactly as described.

The suggested fix would have been much worse than the bug. Treating remapStaleCursorModelId(...) === null as a rejection signal conflates "I will not rewrite this id" with "this id is invalid" — and null is the common case. Measured against this machine's real ~/.hapi/cache/cursor-models.json (74 entries, all ACP wire ids, zero CLI sku variants):

gpt-5.5-high-fast     variant=true  remap=null   → would reject
gpt-5.5-medium        variant=true  remap=null   → would reject
cursor-grok-4.5-fast  variant=true  remap=null   → would reject
cursor-grok-4.5-high  variant=true  remap=null   → would reject
claude-opus-5-high    variant=true  remap=null   → would reject

Every one of those is a valid Cursor sku. A wire-only cache is a different naming of the same models — it carries no effort= params to match a sku against — so the remap declines, and the guard would have blocked essentially every suffixed spawn on this machine. That is a far bigger regression than the one it closes.

What landed instead: the exact-variant requirement applies only when the catalog actually enumerates CLI sku variants of the requested base. ['gpt-5.5-medium'] is authoritative about gpt-5.5's skus, so gpt-5.5-high-fast is rejected and the message names the siblings. A wire-only catalog is not authoritative about skus, so those keep matching on base.

Tests added as requested, including the false-rejection guard that pins why the suggested form was rejected:

// authoritative sku catalog → exact variant required
validateSpawnModelAgainstCatalog('cursor', 'gpt-5.5-high-fast', ['gpt-5.5-medium'])
// → not an available cursor variant of 'gpt-5.5'. Accepted: gpt-5.5-medium
validateSpawnModelAgainstCatalog('cursor', 'gpt-5.5-high', ['gpt-5.5-medium','gpt-5.5-high']) // ok
validateSpawnModelAgainstCatalog('cursor', 'gpt-5.5',      ['gpt-5.5-medium','gpt-5.5-high']) // ok

// wire-only catalog says nothing about skus → base matching stands
validateSpawnModelAgainstCatalog('cursor', 'cursor-grok-4.5-high', ['cursor-grok-4.5[fast=true]']) // ok

Red-green verified: stubbing the new rule to a no-op fails exactly the new rejection test.

Re-verified end to end against the real cached catalog — gpt-5 still rejected; gpt-5.5, gpt-5.5-high-fast, composer-2.5-high, cursor-grok-4.5-high, claude-opus-5-high, default[] and no-model all allowed.

bun typecheck clean. cli 2474 pass, shared 311, web 2859, relay 80. The 2 hub failures are the pre-existing cross-file pollution noted earlier.

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Findings

  • [Major] ACP wire variants still collapse to their base — a request such as claude-opus-4-8[thinking=false,effort=high] passes when the catalog only contains thinking=true, because the final match includes the shared base. The existing resolver intentionally returns null for contradictory explicit parameters (shared/src/cursorCliSku.test.ts:167), so the original unavailable wire is forwarded and the handshake failure remains. Evidence: shared/src/spawnModelCatalog.ts:133.
    Suggested fix:

    if (agent === 'cursor' && isCursorAcpWireModelId(requested)) {
        const available = catalog.map((modelId) => ({
            modelId: modelId.trim().toLowerCase()
        }))
        if (remapStaleCursorModelId(requested.toLowerCase(), available) === null) {
            return {
                ok: false,
                message: `Model '${requested}' is not in the cursor model catalog on this machine. Accepted: ${formatAcceptedModels(agent, requested, catalog)}`
            }
        }
        return { ok: true }
    }
  • [Major] Any cached CLI SKU sibling is treated as a complete variant catalog — the repository explicitly supports partial shared SKU caches (cli/src/modules/common/cursorModels.test.ts:305), and prewarm is asynchronous. During that window, or whenever the live probe fails/is locked, a cache containing only gpt-5.5-medium rejects a valid gpt-5.5-high even though absence from a partial cache proves nothing. Evidence: shared/src/spawnModelCatalog.ts:97.
    Suggested fix:

    type CachedModelCatalog = {
        ids: readonly string[]
        authoritativeCursorSkus: readonly string[]
    }
    
    const siblings = unavailableCursorSkuVariants(
        requested,
        catalog.authoritativeCursorSkus
    )

    Populate authoritativeCursorSkus only after a successful full agent --list-models probe (and persist a completeness bit); inherited/partial shared rows must remain non-authoritative.

Questions

  • None.

Summary
Review mode: follow-up after new commits

The prior explicit CLI SKU case was changed, but two Major paths remain: contradictory ACP wire parameters can still reach the handshake, and partial caches can now block valid spawns.

Testing

  • Not run (automation/security constraint).
  • Add regressions for a contradictory bracket-wire request and for a partial shared SKU cache before/without successful prewarm.

HAPI Bot

}

const index = buildSpawnModelCatalogIndex(agent, catalog)
if (catalogCandidates(agent, requested).some((candidate) => index.has(candidate))) return { ok: true }

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

[MAJOR] Validate explicit ACP wire parameters, not only the base

The final candidate match collapses bracket wires to their base. For example, claude-opus-4-8[thinking=false,effort=high] therefore passes against a catalog containing only claude-opus-4-8[thinking=true,context=300k,effort=high,fast=false]. The existing resolver explicitly rejects that contradictory pair at shared/src/cursorCliSku.test.ts:167; resolveCursorSpawnModel() then forwards the original unavailable wire, recreating the handshake failure.

Suggested fix:

if (agent === 'cursor' && isCursorAcpWireModelId(requested)) {
    const available = catalog.map((modelId) => ({
        modelId: modelId.trim().toLowerCase()
    }))
    if (remapStaleCursorModelId(requested.toLowerCase(), available) === null) {
        return {
            ok: false,
            message: `Model '${requested}' is not in the cursor model catalog on this machine. Accepted: ${formatAcceptedModels(agent, requested, catalog)}`
        }
    }
    return { ok: true }
}

Comment thread shared/src/spawnModelCatalog.ts Outdated
const siblings = [...new Set(
catalog
.map((id) => id.trim().toLowerCase())
.filter((id) => isCursorCliSkuVariantId(id) && cursorCliSkuBaseId(id) === base)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

[MAJOR] Do not infer completeness from one cached SKU sibling

This treats any same-base CLI SKU as an authoritative enumeration. However, cli/src/modules/common/cursorModels.test.ts:305 explicitly supports a shared cache containing only gpt-5.5-medium before a probe adds gpt-5.5-high. Because runner prewarm is asynchronous—and probing can fail or be blocked by an ACP lease—a valid gpt-5.5-high spawn can be rejected from that partial cache. That conflicts with this preflight's “reject only what can be proven wrong” contract.

Suggested fix:

type CachedModelCatalog = {
    ids: readonly string[]
    authoritativeCursorSkus: readonly string[]
}

const siblings = unavailableCursorSkuVariants(
    requested,
    catalog.authoritativeCursorSkus
)

Populate authoritativeCursorSkus only after a successful full agent --list-models probe (persisting a completeness bit); do not use inherited/partial shared rows for exact-variant rejection.

…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>
@heavygee

heavygee commented Sep 4, 2026

Copy link
Copy Markdown
Collaborator Author

Round 3: the second finding is right and I have reverted my own previous commit for it. The first one I am declining, with measurements.

Partial SKU caches — correct, reverted in d5fa896

You're right, and cursorModels.test.ts says so in a test name: "unions shared partial cliModelSkus with probe results when lock is inactive". A shared row holding only gpt-5.5-medium is a supported intermediate state, not a complete catalog — and enrichCursorModelsWithCliSkus skips the probe entirely while an ACP session holds the CLI lock, so that state can persist indefinitely. My 410be98 would have rejected a valid gpt-5.5-high throughout.

That is a worse failure than the handshake error it pre-empted, so 410be98 is reverted rather than patched. I did not take the authoritativeCursorSkus + persisted-completeness-bit route: it is a new on-disk schema field and a structured catalog type threaded through three call sites, to gate a case the handshake already reports. Not proportionate for a low-impact fix.

ACP wire params — declining, same trap as round 2

remapStaleCursorModelId(...) === null still is not an availability signal. Measured against this machine's real cache (74 wire ids):

catalog wires = 74,  remap→null = 0          ← nothing already cached would break
composer-2.5[thinking]        remap = null   ← would be REJECTED
gpt-5.5[reasoning=high]       remap = null   ← would be REJECTED
composer-2.5[fast=true]       remap = composer-2.5[fast=true]
claude-opus-5[fast=false]     remap = claude-opus-5[fast=false]

composer-2.5[thinking] is a partial param set for a model that plainly exists here — it is the id used throughout this PR's own fixtures. The null means "I will not rewrite this", not "this does not exist"; resolveCursorSpawnModel forwards it deliberately. Rejecting on null would block valid spawns whenever a user supplies fewer params than the cache happens to store.

The contract, now stated in the code

Both findings are the same shape, so I have pinned the scope rather than keep re-narrowing it:

This guard rejects a model only when its base appears nowhere in the catalog. Variant-level availability is left to the agent handshake, because a cached catalog is not authoritative about variants — sku rows are partial and mid-union, wire-only caches carry no sku naming, sku-only caches carry no param sets.

Two tests pin it so the scope is not re-narrowed by accident:

validateSpawnModelAgainstCatalog('cursor', 'gpt-5.5-high-fast', ['gpt-5.5-medium'])            // ok
validateSpawnModelAgainstCatalog('cursor', 'claude-opus-4-8[thinking=false,effort=high]',
    ['claude-opus-4-8[thinking=true,context=300k,effort=high,fast=false]'])                    // ok

This is the scope #1752 actually reported: gpt-5, a base present in the catalog in no form at all. That still fails closed, with the gpt-5.x ids named.

bun typecheck clean. cli 2474 pass, shared 310, web 2859, relay 80. Real-catalog check: gpt-5 rejected; gpt-5.5, gpt-5.5-high-fast, composer-2.5, composer-2.5[thinking], cursor-grok-4.5-high, grok-4.5, default[] and no-model all allowed.

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Findings

  • [Major] Cache age is not invalidated when Cursor changes — the preflight trusts any catalog file younger than 24 hours, even if the resolved Cursor executable was upgraded after that file was written. A newly added valid model is then rejected locally until the TTL expires, which is the same user-visible failure this guard is meant to avoid. Evidence: cli/src/modules/common/cursorModels.ts:411.
    Suggested fix:
    const cacheMtimeMs = getSharedCursorModelsCacheMtimeMs()
    const cursorMtimeMs = getResolvedCursorExecutableMtimeMs()
    if (cacheMtimeMs === null || cursorMtimeMs === null || cursorMtimeMs > cacheMtimeMs) {
        return []
    }
    Keep the 24-hour bound as a secondary check, but fail open when the executable identity is newer or cannot be verified.

Questions

  • None.

Summary
Review mode: follow-up after new commits

The follow-up removes the partial-variant rejection paths from the prior review. One Major issue remains: elapsed cache age alone cannot establish that the installed Cursor version still has the same catalog.

Testing

  • Not run (automation/security constraint).
  • Add a regression that writes a fresh catalog, makes the resolved Cursor executable newer than the cache, and verifies an otherwise missing model is allowed; retain coverage that an unchanged executable plus a fresh catalog rejects a missing base.

HAPI Bot

// for weeks, and a catalog that predates a Cursor upgrade would reject an id
// that is now valid.
const sharedAgeMs = getSharedCursorModelsCacheAgeMs();
if (sharedAgeMs !== null && sharedAgeMs <= CACHED_CATALOG_MAX_AGE_MS) {

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

[MAJOR] Invalidate the catalog when the Cursor executable changes

This condition only measures time since the cache file was written. If Cursor is upgraded one minute later and the new version adds a model, the old catalog is still trusted for almost 24 hours and the runner rejects that valid model before the real handshake. The code already treats pre-upgrade catalogs as unsafe, but the age threshold does not detect an upgrade that happens inside the threshold.

Suggested fix:

const cacheMtimeMs = getSharedCursorModelsCacheMtimeMs()
const cursorMtimeMs = getResolvedCursorExecutableMtimeMs()
if (cacheMtimeMs === null || cursorMtimeMs === null || cursorMtimeMs > cacheMtimeMs) {
    return []
}

Resolve the actual Cursor executable (including PATH overrides), and keep the 24-hour TTL only as a secondary bound. Add a regression with an executable mtime newer than a still-fresh catalog.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:cli CLI, runner, agent wrappers bug Something isn't working low-impact Focused / low-blast-radius change; estate lane B promote candidate (PR merge policy)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

bug(cli): bad --model on spawn (e.g. cursor gpt-5) archives peer silently; wrapper OK is remit-only

1 participant