Fix ChatGPT OAuth model discovery and ACP model selection - #724
Conversation
Co-authored-by: Pierre Bruno <pierrebruno@hotmail.ch>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughACP sessions now advertise selectable model and mode options, persist model selection across load, and validate advertised models. ChatGPT model discovery now authenticates with OAuth, resolves Codex account headers, and retries unauthorized requests. ChangesACP model configuration and discovery
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant CLI
participant ACPAgent
participant Discover
participant AuthRetry
participant OAuthResolver
participant CodexAccountResolver
CLI->>ACPAgent: create or load session
ACPAgent->>Discover: resolve provider models
Discover->>AuthRetry: send models request
AuthRetry->>OAuthResolver: resolve OAuth credentials
Discover->>CodexAccountResolver: resolve account ID
AuthRetry-->>Discover: retry and return model list
Discover-->>ACPAgent: return model choices
ACPAgent-->>CLI: return ConfigOptions
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Pull request overview
This PR improves ChatGPT/Codex model discovery by authenticating /models probes with stored OAuth credentials (including the required Codex headers) and adds ACP v1 session config options so editor clients can safely select a model and a permission mode.
Changes:
- Add shared Codex header injection (
ApplyCodexHeaders) and reuse it across runtime and model discovery paths. - Update provider model discovery to use OAuth bearer resolution with a single 401 refresh retry.
- Extend ACP session/new + session/load responses with
configOptions(model + mode) and add validation for standard ACP model/mode selections.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| internal/providers/openai/codex.go | Introduces shared Codex header helper and refactors Codex header injection to use it. |
| internal/providers/factory.go | Exposes a Codex account-id resolver tied to a specific OAuth login key for auxiliary requests. |
| internal/providermodeldiscovery/discovery.go | Adds OAuth + Codex header support for live model discovery via SendWithAuthRetry. |
| internal/providermodeldiscovery/discovery_test.go | Adds coverage for ChatGPT/Codex discovery using OAuth + Codex headers and 401 refresh retry. |
| internal/cli/provider_models.go | Wires provider model discovery to use OAuth resolver + Codex account-id resolver. |
| internal/acp/types.go | Updates ACP config-option wire schema to support select options with currentValue + options. |
| internal/acp/agent.go | Adds ACP config options (model/mode), selection validation, and session lifecycle wiring. |
| internal/acp/agent_test.go | Adds tests for ACP config options, schema fields, validation, and persistence across load. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| resolver, loginKey := oauthLoginForProfile(profile) | ||
| return providermodeldiscovery.Discover(ctx, profile, providermodeldiscovery.Options{ | ||
| OAuthResolver: resolver, | ||
| CodexAccountResolver: providers.CodexAccountResolverForLogin(loginKey), | ||
| UserAgent: "zero", | ||
| }) |
| func (s *acpSession) setModel(model string) { | ||
| s.mu.Lock() | ||
| found := false | ||
| for _, option := range s.models { | ||
| if option.Value == model { | ||
| found = true | ||
| break | ||
| } | ||
| } | ||
| if !found && strings.TrimSpace(model) != "" { | ||
| s.models = append(s.models, SessionConfigOptionValue{Value: model, Name: model}) | ||
| } | ||
| s.model = model | ||
| s.mu.Unlock() | ||
| } |
| func (s *acpSession) hasModel(model string) bool { | ||
| s.mu.Lock() | ||
| defer s.mu.Unlock() | ||
| for _, option := range s.models { | ||
| if option.Value == model { | ||
| return true | ||
| } | ||
| } | ||
| return false | ||
| } |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
internal/providers/factory.go (1)
364-372: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse this helper in
newCodexProvidertoo.Lines 313-319 retain an equivalent resolver closure. Replace it with
CodexAccountResolverForLogin(accountKey)so runtime and discovery cannot drift.As per coding guidelines, “Unify functions and methods where possible to prevent codebase inflation.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/providers/factory.go` around lines 364 - 372, Update newCodexProvider to use the existing CodexAccountResolverForLogin(accountKey) helper instead of defining its equivalent inline resolver closure, keeping runtime and discovery account resolution unified.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/acp/agent.go`:
- Around line 370-375: Update the ConfigID model-selection branch around
sess.hasModel and sess.setModel to track whether the current snapshot uses a
standard model catalog, enforcing model membership only for standard catalogs
while allowing vendor-specific model IDs for providers without one.
- Around line 172-176: Update the session initialization flow around
resolveModelChoices and registerSession to read the persisted model selection
from session metadata and use it as the initial model after restart, rather than
recomputing the selection solely from current workspace configuration. Ensure
setModel writes the selected model into the same persisted metadata so
subsequent loads restore it, while preserving model-choice resolution for
sessions without a stored selection.
- Around line 430-454: The resolveModelChoices method currently enumerates only
the static providermodelcatalog. Update it to snapshot the effective provider’s
authenticated/live discovered model catalog, using the configured selected model
as the fallback when discovery or enumeration fails, while preserving
deduplication and option construction for session/new and session/load.
---
Nitpick comments:
In `@internal/providers/factory.go`:
- Around line 364-372: Update newCodexProvider to use the existing
CodexAccountResolverForLogin(accountKey) helper instead of defining its
equivalent inline resolver closure, keeping runtime and discovery account
resolution unified.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 7b337a89-9d40-4cbf-a60a-07f7b733f2f5
📒 Files selected for processing (8)
internal/acp/agent.gointernal/acp/agent_test.gointernal/acp/types.gointernal/cli/provider_models.gointernal/providermodeldiscovery/discovery.gointernal/providermodeldiscovery/discovery_test.gointernal/providers/factory.gointernal/providers/openai/codex.go
gnanam1990
left a comment
gnanam1990
left a comment
There was a problem hiding this comment.
Requesting changes — the ChatGPT OAuth discovery half is correct and well tested, but the ACP half derives restrictModels from catalog membership rather than from whether discovery actually produced a list, which is a straight capability regression for providers that have no discovery endpoint. It's a small fix.
Scope note first: the branch is based on 18cce35, not fbf8598. git diff fbf8598..HEAD reports 97 files / ~7300 deletions because it also reverses everything main landed after 18cce35 (profile_controller, task_state, turn_session, execprofile, …). The real PR is git diff 18cce35..HEAD — 11 files, +592/-71, matching pr724.diff exactly. All 11 files are germane; I found no unrelated hunks. This will need a merge/rebase rather than a squash of the two-dot diff.
[Major] Catalog providers without live discovery can no longer change model over ACP
internal/acp/agent.go:476
restrictModels := knownProvider && !descriptor.CustomThis is computed before discovery runs and is never revised based on the outcome. providermodeldiscovery.Discover only handles OpenAI/OpenAI-compatible and Anthropic/Anthropic-compat kinds (internal/providermodeldiscovery/discovery.go:79-88); for every other catalog transport — google, bedrock, vertex — it unconditionally returns provider X does not expose model discovery. For those profiles restrictModels == true but options == [configured model], so the guard at internal/acp/agent.go:422 rejects every other value.
Confirmed with a probe test: a google profile advertises options = [gemini-2.5-pro], and session/set_config_option{configID:"model", value:"gemini-2.5-flash"} returns -32602: unknown model: gemini-2.5-flash. Before this PR the handler was a bare sess.setModel(p.Value) with no validation (pr724.diff:144), so any model was accepted. Same failure mode hits every catalog provider for the duration of any discovery outage.
The discovery error is also discarded at internal/acp/agent.go:478 with no log or notification, so the user gets a one-entry picker and no explanation.
Fix — only enforce the allow-list when it is actually authoritative:
discoverySucceeded := false
if a.deps.DiscoverModels != nil {
discovered, discoverErr := a.deps.DiscoverModels(ctx, resolved.Provider)
if ctx.Err() != nil { return "", nil, false, ctx.Err() }
if discoverErr == nil && len(discovered) > 0 {
discoverySucceeded = true
// …existing add() loop…
}
// consider: else log discoverErr so a collapsed picker is diagnosable
}
restrictModels := knownProvider && !descriptor.Custom && discoverySucceededi.e. false when DiscoverModels is nil, errored, or returned zero usable models. Worth a test with a discovery-less catalog provider asserting set_config_option still accepts an arbitrary model.
[Minor] session/new and session/load block on an unbounded, uncached live network probe
internal/acp/agent.go:139
resolveModelChoices passes the connection-level ctx (internal/acp/jsonrpc.go:121 — not a per-request ctx, so it's only cancelled at shutdown) straight into DiscoverModels. The production path is defaultDiscoverProviderModels → providermodeldiscovery.Discover, whose default client timeout is 10s (internal/providermodeldiscovery/discovery.go:238) and which can issue two attempts via the 401 force-refresh loop (internal/providers/providerio/auth.go:70-73); each refresh runs through an OAuth manager with its own 30s client timeout (internal/cli/oauth_provider.go:47). Worst case a single session/new blocks ~80s. Nothing caches the result, so every new thread re-probes. Requests are dispatched on goroutines (jsonrpc.go:167) so the connection isn't stalled, but an editor like Zed shows a hung thread where previously session/new was purely local.
Fix: wrap the discovery call in a short context.WithTimeout (a few seconds) inside resolveModelChoices — the failure path already degrades cleanly to the configured model — and consider caching the per-provider result for the connection's lifetime.
[Nit] Breaking ACP wire-format change — confirm it's coordinated
internal/acp/types.go renames SessionConfigOptionValue.id → value, SessionConfigOption.Value → currentValue, Values → options, and adds required type/category fields. TestACPConfigOptionWireSchema (internal/acp/agent_test.go:332) explicitly asserts the old value/values keys are gone, so this is deliberate — just make sure editor-side consumers are lined up.
Checked and cleared (not findings)
session/load"silently discarding" the persisted model on a discovery outage. The fallback is real (internal/acp/agent.go:180) but it is not silent:configOptions(agent.go:496-517) populates the samesession/loadresponse withCurrentValueand the degraded one-entryOptions, which is ACP's canonical model-display surface — verified by probe. It's also non-destructive and self-healing: the store keeps the user's choice and load restores it once discovery recovers. Writing the fallback back to the store would be strictly worse. (ThewarnPersistenceanalogy doesn't hold —LoadSessionResultattypes.go:142-145has no history field, so that notification is the only channel for history loss, whereas model state is fully represented in the payload.)_zero/set_modelbypassing the allow-list. It does passrestrictModels=false(agent.go:413), but at the merge base both paths were entirely unvalidated, so this PR strictly tightens the surface; the bypass is circular (the caller has already set the bogus model directly); and it is not permanent —agent.go:180discards an unadvertised persisted model on load for restricted sessions. Contamination is confined to the in-memory session. A one-line comment atagent.go:413explaining the intentional asymmetry would help future readers.sess.restrictModelsread withoutsess.mu. Not a race by construction: the field (agent.go:74) has exactly one write, the composite-literal init inregisterSessionundera.mu(agent.go:654-661), and is never reassigned. Every reader goes througha.session()(agent.go:667), which crosses the same mutex.go test -race -count=3 ./internal/acp/is clean. The asymmetry withmodel/models/modeis justified — those are mutated post-publication.
What's genuinely well done
The OAuth/credential half is solid and I found no leak on that surface. SendWithAuthRetry resolves auth before dispatch, so a resolver error can never let an unauthenticated request go out first (internal/providers/providerio/auth.go:42-59), and withBearer clears APIKey so both auth methods can never be sent together (auth.go:86). Discovery errors pass through redactDiscoveryError (discovery.go:452) plus redaction.RedactString. Extracting CodexAccountResolverForLogin (factory.go:863) is the right call — it binds chatgpt-account-id to the exact login key that issued the bearer instead of doing a second independent lookup — and the shared ApplyCodexHeaders (codex.go:29) stops runtime completions and discovery from drifting apart. I verified the injectCodexHeaders refactor is behavior-preserving (p.originator is normalized non-empty at codex.go:111-113, so the != codexDefaultOriginator guard correctly restores a custom value). TestDiscoverChatGPTModelsUsesOAuthAndCodexHeaders is a real test, not a smoke test — it asserts the 401 path re-sends both a refreshed bearer and a re-derived account id. Store.UpdateModel correctly mirrors UpdateTitle's locking and its deliberate non-touching of UpdatedAt/EventCount.
Build / test
gofmt clean, go build ./... clean, go vet clean on all touched packages. go test passes for internal/acp, internal/providermodeldiscovery, internal/providers/..., internal/sessions. go test -race -count=3 on those packages: clean, no data races.
internal/cli has 5 failures — all confirmed pre-existing, re-run with the identical -run selectors against the true merge base 18cce35 where they fail identically: TestRunAddDirDispatchForwardsGrantIntoExecScope, TestExecScopeReRegistrationSwapsCoreToolsByName, TestRunDoctorFormatsRedactedProviderDiagnostics, TestRunDoctorConnectivityProbesProvider, TestRunSandboxCheckJSONDeniesOutOfWorkspaceWrite. The sandbox_check JSON reports "platform": "windows" / "backend.available": false while running on darwin/arm64, which is the root cause of three of them. None of these touch the two internal/cli files this PR changes (acp.go, provider_models.go). Nothing is attributable to this PR.
Housekeeping: internal/acp/zzrefute_test.go is an untracked file in the shared worktree that is not part of this PR — it ends in an unconditional t.Fatalf, so it makes go test ./internal/acp/ fail by construction. Delete it before reusing the worktree.
Merge is kevin's call per the program gate.
gnanam1990
left a comment
gnanam1990
left a comment
There was a problem hiding this comment.
Verdict: APPROVE
The core bug is genuinely fixed. Previously ACP/CLI called Discover with empty Options{}, so an OAuth-only ChatGPT login sent an unauthenticated request to /backend-api/codex/models → 401 → no models. This PR wires oauthLoginForProfile's resolver and the same login key into providers.CodexAccountResolverForLogin, so the bearer and chatgpt-account-id now come from one login and discovery routes through providerio.SendWithAuthRetry (1 retry on 401 with force-refresh). The new TestDiscoverChatGPTModelsUsesOAuthAndCodexHeaders exercises exactly this path and would fail without the fix. Lock ordering (modelMu→mu) is sequential with no nested hold, restrictModels is set-once before publish, and -race is clean. Nice work.
Build/test on the PR HEAD (go1.26.5): gofmt -l clean, go build ./... and go vet pass, and go test -race ./internal/acp/... ./internal/providermodeldiscovery/... ./internal/providers/... ./internal/sessions/... all pass.
The internal/cli sandbox tests (TestRunAddDirDispatchForwardsGrantIntoExecScope, TestExecScopeReRegistrationSwapsCoreToolsByName, TestRunSandboxCheckJSONDeniesOutOfWorkspaceWrite) FAIL — but they fail identically in the clean base worktree (they report an unavailable/"windows" sandbox backend in this macOS environment) and don't touch the PR's changed files. Pre-existing / environmental, not PR-attributable.
[Minor] Transient discovery failure on session/load silently reverts a restricted-provider user's persisted model to the configured default — PR-introduced
internal/acp/agent.go:180 (with resolveModelChoices at agent.go:458)
resolveModelChoices only returns early on ctx.Err(); a plain discovery error (network blip, 401, API down) falls through to return selected, options, restrictModels, nil with options containing just the configured default and restrictModels still true.
Concrete failure: over a ChatGPT profile (restrictModels=true, config default gpt-5.5), the user selects gpt-5.4-mini via set_config_option, which Store.UpdateModel persists as meta.ModelID. On a later session/load while discovery is transiently unavailable, the merge at line 180:
persistedModel != "" && (!restrictModels || modelChoiceExists(models, persistedModel))evaluates to true && (false || false) = false, so the persisted gpt-5.4-mini is not applied. registerSession stores gpt-5.5, the advertised CurrentValue and runTurn both use gpt-5.5 — a silently substituted model the user did not choose. The stored ModelID is untouched, so it self-heals on the next successful load (transient, not permanent data loss).
Fix: distinguish "discovery returned a curated/empty set" from "discovery failed." Have resolveModelChoices report whether live discovery actually succeeded, and on the load path honor the persisted selection when discovery was unavailable (it was already validated when the user picked it) — e.g. append and select meta.ModelID whenever discovery did not succeed, rather than dropping it because the degraded option set no longer contains it.
Merge is kevin's call per the program gate.
Summary
Validation
go test ./...go vet ./...govulncheck ./...Repository-wide golangci-lint still reports pre-existing findings outside the changed packages.
Fixes #722
Summary by CodeRabbit
New Features
Bug Fixes
Tests