fix(providers): refresh inference OAuth tokens - #1465
Conversation
Aaronontheweb
left a comment
There was a problem hiding this comment.
Self-review notes on the main OAuth refresh design choices and boundary decisions.
| /// Implementations may use <paramref name="providerName"/> to update persisted | ||
| /// runtime metadata, such as refreshed OAuth credentials. | ||
| /// </summary> | ||
| public interface IConfiguredProviderProbe |
There was a problem hiding this comment.
Self-review: IConfiguredProviderProbe is intentionally separate from the generic probe API. Refresh persistence needs the config key (providerName), and temporary add/fix probes do not have a committed key yet, so they should not be able to write refreshed credentials.
| if (!NeedsRefresh(entry.OAuthTokenExpiry)) | ||
| return accessToken; | ||
|
|
||
| var refreshGate = _refreshLocks.GetOrAdd(providerName, _ => new SemaphoreSlim(1, 1)); |
There was a problem hiding this comment.
Self-review: This per-provider semaphore avoids a refresh stampede when several chat or probe calls find the same expired OAuth credential. The second expiry check inside the lock lets callers that waited on another refresh return the newly valid token without making a duplicate token request.
| } | ||
|
|
||
| ApplyRefreshResult(entry, result); | ||
| OAuthTokenPersistence.PersistTokens(paths, providerName, result); |
There was a problem hiding this comment.
Self-review: Persistence is centralized here instead of spread across provider-specific code. That keeps OpenAI, Copilot, and configured-provider probes writing the same config/secrets representation, and prevents a successful in-memory refresh from being lost after restart.
| $"Unknown provider type: {entry.Type}", []); | ||
| } | ||
|
|
||
| if (entry.AuthMethod is AuthMethod.OAuthDevice or AuthMethod.OAuthPkce |
There was a problem hiding this comment.
Self-review: Only persisted OAuth entries with a known expiry enter the refresh path. The plain IProviderProbe overloads still delegate unchanged so provider add/fix validation remains no-clobber until validation succeeds.
| ProviderDisplayItem item, | ||
| CancellationToken ct) | ||
| { | ||
| if (item is { Entry: not null, ConfiguredName: not null } |
There was a problem hiding this comment.
Self-review: This is the TUI boundary between existing configured providers and transient add/fix entries. Existing providers supply ConfiguredName and can refresh/persist; pending entries fall back to the plain probe so a failed validation cannot overwrite stored credentials.
| if (tokenRefreshService is null) | ||
| return await GetTokenAsync(entry, ct); | ||
|
|
||
| var oauthToken = await tokenRefreshService.GetValidAccessTokenAsync( |
There was a problem hiding this comment.
Self-review: Copilot has two token layers. This refreshes the persisted GitHub OAuth token before exchanging it for the short-lived Copilot API token; otherwise /copilot_internal/v2/token receives an expired GitHub bearer and runtime chat fails even though a refresh token is available.
| PipelineMessage message, IReadOnlyList<PipelinePolicy> pipeline, int currentIndex) | ||
| { | ||
| var token = await exchanger.GetTokenAsync(entry, message.CancellationToken); | ||
| var token = providerName is not null && oauth is not null |
There was a problem hiding this comment.
Self-review: Passing providerName and descriptor OAuth config through the request policy is what lets the daemon chat path use the refresh-aware exchanger. The fallback keeps descriptor/probe paths working when they only have a raw entry and must not persist.
| Endpoint = new Uri("https://chatgpt.com/backend-api/codex") | ||
| }; | ||
| options.AddPolicy(new OpenAiCodexRequestPolicy(accountId), PipelinePosition.PerCall); | ||
| var credential = new ApiKeyCredential(token.Value); |
There was a problem hiding this comment.
Self-review: The credential object is shared with OpenAiCodexRequestPolicy so the policy can update it before the SDK auth policy runs. We only require the existing account id immediately when refresh is unavailable; with refresh enabled, the policy can recover it from refreshed OpenAI token metadata.
| Modify(message); | ||
| if (_tokenRefreshService is not null) | ||
| { | ||
| var token = await _tokenRefreshService.GetValidAccessTokenAsync( |
There was a problem hiding this comment.
Self-review: Refresh is async because it may issue an OAuth network call. The sync Process path fails loudly instead of using a stale token; in the async path we update the SDK credential before calling the next policy so authorization uses the refreshed token.
| /// Call after <see cref="ProviderDescriptorServiceExtensions.AddProviderDescriptors"/> | ||
| /// when the host needs OAuth provider flows or configured-provider refresh. | ||
| /// </summary> | ||
| public static IServiceCollection AddProviderOAuthServices(this IServiceCollection services) |
There was a problem hiding this comment.
Self-review: Centralizing this registration keeps the daemon, init wizard, provider TUI, and config editor on the same OAuth stack. This avoids hosts drifting into slightly different device-flow or refresh-probe wiring.
Aaronontheweb
left a comment
There was a problem hiding this comment.
Generally looks good, had one question that I need to answer...
| CancellationToken ct = default) | ||
| { | ||
| if (tokenRefreshService is null) | ||
| return await GetTokenAsync(entry, ct); |
There was a problem hiding this comment.
get the initial token
| if (tokenRefreshService is null) | ||
| return await GetTokenAsync(entry, ct); | ||
|
|
||
| var oauthToken = await tokenRefreshService.GetValidAccessTokenAsync( |
There was a problem hiding this comment.
get the refresh token
|
|
||
| // Coalesce concurrent refreshes for the same configured provider so a | ||
| // burst of requests does not spend the same refresh token multiple times. | ||
| var refreshGate = _refreshLocks.GetOrAdd(providerName, _ => new SemaphoreSlim(1, 1)); |
There was a problem hiding this comment.
not sure how necessary this really is but we'll see in practice
| new OAuthDeviceFlowService( | ||
| sp.GetRequiredService<IHttpClientFactory>().CreateClient("OAuthDeviceFlow"), | ||
| sp.GetService<TimeProvider>())); | ||
| services.AddSingleton(sp => |
There was a problem hiding this comment.
why are we adding this every time?
There was a problem hiding this comment.
NM, only used when OpenAI oauth is actually needed.
Summary
Fixes #1463
Validation