Repository navigation
fix(models): stop discovery from deleting bundled catalog entries - #6502
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
A live discovery catalog reflects what a provider -- or the signed-in ChatGPT plan -- currently lists, not what exists. Treating it as authoritative deleted bundled entries the provider omitted, so a ChatGPT Plus account lost selectable ids such as openai-codex/gpt-6.1-sol from /model, profile activation, and preset availability even though the model was bundled. Discovery now only enriches the bundled catalog: it adds unknown ids and refreshes metadata, and never removes a bundled model. Access decisions defer to the provider, which returns a typed entitlement error. Lore-id: available-catalog-keeps-bundled-models Constraint: the bundled catalog is authoritative for catalog visibility Rejected: keep hiding for plan-scoped providers only | no reliable signal separates plan entitlement from a deprecation, and codex was the only observed case Directive: do not reintroduce discovery-driven deletion of bundled models Confidence: high Scope-risk: wide Reversibility: easy Tested: model-registry, model-profile-activation, codex-profile-pinned-discovery, cli-args-mpreset, model-selector-profiles, sdk-q27-model-profiles, sdk-model-selection, model-registry-*, discovery and autorouting suites (bun test) Not-tested: spawn-based broker/e2e suites (EBADF posix_spawn in this environment)
e86fcfe to
1a76ad5
Compare
![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: e86fcfe3f3
ℹ️ 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".
| } | ||
| return !bundledIds.has(model.id) || liveIds.has(model.id); | ||
| }); | ||
| this.#availableModelsCache = this.#models.filter(model => this.#isModelAvailable(model, disabledProviders)); |
There was a problem hiding this comment.
When a descriptor-backed provider is refreshed under a different account or after its live catalog changes, #mergeDiscoveredModels() only merges the new snapshot into #models; it does not remove previously discovered IDs. The removed authoritative filter used profileModelIds to hide such non-bundled IDs once the current catalog omitted them, but this unconditional filter now exposes them indefinitely. For example, a paid Codex-only model remains selectable after switching to a free account, and a model unloaded from vLLM remains in /model and can be auto-selected. Preserve bundled/static entries as intended, but continue filtering or evicting dynamically learned IDs that are absent from current discovery evidence.
Useful? React with 👍 / 👎.
probepark
left a comment
probepark
left a comment
There was a problem hiding this comment.
Review (head 1a76ad5, gajae-reviewer on behalf of probepark)
Blocking findings:
- P1 —
packages/coding-agent/src/config/model-registry.ts:5674: Preserve bundled models without retaining obsolete discovery-only models.#mergeDiscoveredModels()merges into the previous catalog (lines 3491–3506).#mergeResolvedModels()never removes absent IDs (lines 2423–2449). The replacement filter checks only provider authentication and disabled status (lines 5221–5227). After discovery returns a previously unknown ID, a subsequent successful response omitting that ID leaves it selectable indefinitely. This also affects profile activation and startup fallback. Retain bundled and explicit custom entries, but filter or evict discovery-only IDs absent from current evidence. Add a two-refresh regression: discover an unknown ID, omit it on refresh, then assert it disappears while an omitted bundled ID survives. This independently confirms the earlier Codex finding on the current head. - CI blocker, cause unclassified —
packages/coding-agent/src/sdk/host/session-runtime.test.ts:5392,8610: The exact-head CI job fails two accepted-control regressions. One never reaches terminal reconciliation; the other returns a result that does not match the expected settlement state. Job: https://github.com/Yeachan-Heo/gajae-code/actions/runs/37738534564/job/113188668410 (225 pass, 2 fail, exit 1). The revieweddevfailure run, https://github.com/Yeachan-Heo/gajae-code/actions/runs/37733646186, contains other failures; it does not establish these two as base failures. Fix the failures, or provide matching base-run evidence before approval.
CI: Coding-agent check, registry tests, profile activation tests, and selector integration tests passed. Other affected checks were still running when inspected. No local builds or tests were run in this pod.
Scope: +70 / -316, 14 files — model registry, profile activation, selector, SDK startup/query surface, eight test files, and a release-note fragment. All five OCR-selected source diffs were reviewed.
Conventions: Release-note fragment present, as required by AGENTS.md:202; no generated files changed; no labels.
ocr: blocking 1 / nit 0
Spec axis: The PR's bundled-catalog visibility change reaches all five source call sites. The discovery-only retention regression above is outside the stated bundled-preservation goal.
Checked and clean: Provider-authentication/disabled-provider filtering remains; profile materialization and selector/query call sites use the same available catalog. Test expectation changes have an explicit rationale in the PR description.
Not assessed: Live provider entitlement errors and spawn-based end-to-end behavior were not exercised locally. CI failure provenance remains unclassified.
Blocking: 2 total (1 source defect, 1 unclassified CI failure).
Verdict: gajae.pr-review-verdict.v1 needs-human sha256:b8c1e2b9ed42034eb47b48aa39cc30075afc312bacf388e61669fc4baff09bec reviewer:critic reviewer-id:gajae-reviewer evidence:stale-discovery-only-ids-remain-selectable;session-runtime-ci-failure-unclassified
PR body verdict line count=0, not updated. Body verdict line is owned by absent; not edited. Suggested verdict line: gajae.pr-review-verdict.v1 needs-human sha256:b8c1e2b9ed42034eb47b48aa39cc30075afc312bacf388e61669fc4baff09bec reviewer:critic reviewer-id:gajae-reviewer evidence:stale-discovery-only-ids-remain-selectable;session-runtime-ci-failure-unclassified
2 participants
Why
The ChatGPT $20 (Plus) plan was blocked from
gpt-6.1-solon gjc only. The cause was client-side:openai-codexis the one provider whose live catalog is an account entitlement list (GET chatgpt.com/backend-api/codex/modelswith the OAuth bearer +chatgpt-account-id). The registry treated that list as authoritative and deleted bundled entries absent from it in two places:getAvailable()via#getAuthoritativeDiscoveredModelIds()getAvailableForProfileActivation()(its sole purpose)So when a Plus account's plan-scoped response omitted
gpt-6.1-sol, gjc silently hid the bundledopenai-codex/gpt-6.1-solfrom/model, profile activation (codex-medium/codex-pro/astra-*are built on it), and preset availability — while other clients still offered it. The earlier client-side entitlement preflight was already removed (#5412); this discovery-driven hide was the leftover.What changed
A discovered catalog now only enriches the bundled catalog (adds unknown ids, refreshes metadata) and never deletes a bundled model. Access decisions defer to the provider, which returns a typed error (
openai-codex-responses.tsmapsnot supported when using codex with a chatgpt account).getAvailable()keeps every bundled entry; discovery no longer removes.ModelRegistry.getAvailableForProfileActivation()and theprofileModelIds/profileFresh/profileEndpointevidence fields behind it.model-profile-activation.ts,model-selector.ts,sdk/session.ts,sdk/host/session-runtime.ts) now readgetAvailable(); the now-redundant startup fallback filter insdk/session.tswas dropped.Tests
model-registry,model-profile-activation,codex-profile-pinned-discovery,cli-args-mpreset,model-selector-profiles,model-preset-landing-redteam-qa,sdk-q27-model-profiles,sdk-model-selection) to assert bundled entries survive; deleted one test whose only premise was the two-catalog distinction.bun --cwd=packages/coding-agent run check:typesclean; biome clean.bun testacross the model/profile/discovery/autorouting selector suites: pass.EBADF posix_spawnin this environment (also fails on a pristine tree).