Repository navigation
feat(llm): add openai-chatgpt provider with ChatGPT subscription OAuth - #1639
trongtrandp wants to merge 2 commits into
Conversation
Sign in with 'ocr llm login openai-chatgpt'; requests go through the Responses API using the account's OAuth token instead of an API key.
|
|
|
🔍 OpenCodeReview found 5 issue(s) in this PR.
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| profile, err := c.Login(ctx, accountID, func(authURL string) error { | ||
| fmt.Printf("Continue with ChatGPT:\n%s\n", authURL) | ||
| if err := openChatGPTBrowser(authURL); err != nil { | ||
| fmt.Println("Could not open a browser automatically. Open the sign-in URL above.") | ||
| } | ||
| return nil | ||
| }) |
There was a problem hiding this comment.
runChatGPTLogin uses fmt.Printf / fmt.Println directly (lines 141, 143, 176) while every cobra subcommand handler in init() consistently writes to cmd.OutOrStdout(). This breaks output redirection and testability for the login flow. Consider passing io.Writer (or *cobra.Command) into runChatGPTLogin so all user-facing output goes through the same writer.
| if ep.ChatGPT { | ||
| if len(tools) > 0 && !toolCalled { | ||
| return fmt.Errorf("ChatGPT did not call the required self-test tool; tool-call round trip could not be verified") | ||
| } | ||
| if strings.TrimSpace(resp.VisibleContent()) == "" { | ||
| return fmt.Errorf("LLM connectivity test returned no visible answer; connection verification failed") | ||
| } | ||
| } |
There was a problem hiding this comment.
When ep.ChatGPT is true but len(tools) == 0, firstChoice remains "" and the test sends the request without forcing a tool call. The subsequent ChatGPT-specific validation block (lines 158-164) then checks len(tools) > 0 && !toolCalled, which correctly skips the tool-call assertion. However, it still enforces strings.TrimSpace(resp.VisibleContent()) == "" even when no tools were offered. If the ChatGPT plan route legitimately returns an empty visible response for a connectivity probe without tools (e.g., a reasoning-only reply), this would produce a false failure.
Consider whether the empty-visible-content check should also be gated on len(tools) > 0, or whether the intent is that ChatGPT must always produce visible text regardless of tool presence. If the latter, adding a brief comment explaining why would help future maintainers.
|
|
||
| {!ambientAuth && ( | ||
| {preset.oauth && ( | ||
| <p class="form-hint">{t('view.config.chatgptLoginHint')} <code>ocr llm login openai-chatgpt</code></p> |
There was a problem hiding this comment.
The provider name openai-chatgpt is hardcoded in the command string. If additional OAuth providers are added in the future, this hint won't dynamically reflect the correct provider name. Consider using preset.name to make it generic:
<code>ocr llm login {preset.name}</code>This way the hint remains accurate for any OAuth provider.
| if err := f.Close(); err != nil { | ||
| return err | ||
| } | ||
| return os.Rename(f.Name(), c.path) |
There was a problem hiding this comment.
The credential file is written via os.CreateTemp + os.Rename, which creates the temp file with mode 0600. However, there is no explicit os.Chmod on the final file after rename. While os.CreateTemp uses 0600 on Unix, this is an implementation detail rather than a documented guarantee, and behavior may differ on non-Unix platforms. Since this file contains OAuth tokens and refresh tokens, explicitly setting restrictive permissions would be more robust.
Consider adding os.Chmod(c.path, 0o600) after the rename (or as part of a follow-up hardening step).
| if !c.now().Add(time.Minute).Before(a.ExpiresAt) && !c.now().Before(a.EarliestRefreshAt) { | ||
| if a.RefreshToken == "" { | ||
| return errors.New("ChatGPT session is not renewable; sign in again") | ||
| } | ||
| p, _, err := c.discovery(ctx) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| var t tokenResponse | ||
| err = c.request(ctx, http.MethodPost, p.Endpoint().TokenURL, url.Values{"grant_type": {"refresh_token"}, "client_id": {a.ClientID}, "refresh_token": {a.RefreshToken}, "resource": {c.resource}}, "", &t) |
There was a problem hiding this comment.
Holding a file lock across network I/O can block other processes (or concurrent goroutines in the same process via TokenSource) for up to the HTTP client timeout (30s per request). During token refresh, two HTTP calls are made (discovery + token endpoint) while the lock is held. Similarly, Logout makes up to 3 retry attempts against the revocation endpoint under the same lock.
Consider restructuring to read/update the store under the lock, release it for the HTTP call, then re-acquire to persist the result. Alternatively, use a shorter per-request timeout for the refresh path or document that callers should expect contention during refresh.
Also clarify why the ChatGPT self-test requires a visible answer, and derive the extension login hint from the preset name.
This branch has not been deployed
2 participants
Description
Adds a built-in
openai-chatgptprovider so OCR can run reviews on a ChatGPTsubscription through OAuth instead of a platform API key.
ocr llm login openai-chatgptruns an OAuth authorization-codeflow with PKCE and OIDC ID-token validation (nonce, subject, audience), using
a loopback callback on
127.0.0.1. Available models are discovered peraccount and written to the provider entry.
~/.opencodereview/chatgpt/credentials.json(0600), written atomically under a file lock. Access tokens are refreshed on
demand; a refresh response without
refresh_tokenkeeps the current one(RFC 6749 §6), and a token response without
scopemeans the requestedscopes were granted (RFC 6749 §5.1).
logout,status,modelsandaccount <client-id>.Logout revokes the refresh token, or the access token when there is no
refresh token, and keeps the registration.
namespaced under
ocr. Streamerror/response.failedevents keep theserver's code and message, and subscription quota/eligibility errors map to
actionable messages.
the OAuth token (
api_key,api_key_cmd,url,auth_header,extra_headers,protocol).extra_bodyis limited toreasoning,textand
prompt_cache_key.OpenAI-Organization/OpenAI-Projectheaders fromthe environment are stripped on this route.
ocr config providerlaunches the login when no model is known yet. The VS Code config view shows
a localized login hint. Generated provider catalogs are regenerated via
go generate ./internal/llm.internal/viewernow exportsOpenBrowserso the loginreuses the existing browser opener (
$BROWSER, exit-status check) instead ofa second copy.
ocr llm test: the stricter "tool call plus visible answer" check appliesonly to this provider. Other providers keep the previous behavior.
Open design points for reviewers
These are deliberate choices I would like maintainer input on:
WithMaxRetries(0)on the ChatGPT route: no SDK retries at all, includingtransient 5xx. This avoids hammering quota errors; a narrower policy (retry
5xx and connection resets, skip quota codes) is possible.
expires_in> 86400 are rejected.retry_codesandOCR_LLM_EXTRA_HEADERShave no effect on this route, whileextra_headersin the config is rejected. Silently ignoring them isinconsistent; I can reject or honor them instead.
caching the token in memory, so logout and account switches take effect
immediately. This serializes concurrent requests.
Models()decodes a{"models":[{"slug","visibility"}]}response. The unittests use a mock, so this schema relies on manual verification.
Type of Change
How Has This Been Tested?
make check,make test(with-race) andmake coverage(92.3%, threshold90%) pass locally; frontend jest/tsc pass.
New unit tests cover:
scope;make testpasses locallyManual testing (describe below)
Checklist
go fmt,go vet)AI disclosure:
review and for the fixes found in review.
ocr review(provideropenai-chatgpt, modelgpt-6.1-sol) was used forpre-commit review.
I have reviewed all generated code and can explain every change.
https://claude.ai/code/session_01DnWv6iXVSLhVRX343cw36k
Related Issues
None.