Repository navigation
fix(core): isolate ChatGPT account registrations - #56302
KomeijiReimu wants to merge 3 commits into
Conversation
Use independent registrations for new accounts and pass an explicit credential target when reauthorizing an existing connection. Closes anomalyco#56298
|
The following comment was made by an LLM, it may be inaccurate: |
There was a problem hiding this comment.
The core fix looks right, but one TUI path now fails where it used to work. I couldn't confirm the fix end to end: OpenAI only returns the workspace denial on its live sign-in page, and testing that needs two real ChatGPT accounts. I read the code and ran the closest tests: core provider-chatgpt and integration (50 passed), the TUI integration dialog (15 passed) and the server fetch suite (13 passed). typecheck passes in core, plugin, tui, server, protocol and client.
A new connection now sends dynamic_agent_client, and a reauthorization uses that credential's own saved client ID. That matches the cause found in #56298. The new "no registered client ID" error shouldn't trap older accounts, because every ChatGPT credential is saved with a clientID.
Public API: the changes are additive and backwards compatible.
POST /api/integration/{integrationID}/connect/oauthtakes a new optionalcredentialIDin its body. The OpenAPI files and generated clients are updated to match.- Plugin API (effect and promise):
IntegrationOAuthMethodRegistration.authorize(answer)becomesauthorize(answer, context?: { credential?: Credential.OAuth }). Existing plugins that take one argument keep working. - The plugin host's
integration.oauth.connecttakes a new optionalcredentialID.
Concerns:
- Picking a different method now fails. In "Sign in required", the TUI sends the account's ID with whichever OAuth method the user picks. OpenAI has three OAuth methods: "Sign in with ChatGPT" and the two Codex legacy ones. Picking a method other than the one the account was made with now fails with "OAuth credential belongs to another method". Before this PR, that path signed in and created a new credential. Details inline.
- Tests are large. About 240 test lines go into
provider-chatgpt.test.ts, mostly generic plugin-adapter tests (details inline).
| location, | ||
| dialog, | ||
| onConnected, | ||
| connection.method === "oauth" ? connection.id : undefined, |
There was a problem hiding this comment.
This credentialID goes to selectMethod and is used with whichever OAuth method the user picks from the full list. OpenAI has three OAuth methods (Sign in with ChatGPT, Codex browser (legacy), Codex device code (legacy)). Connections only say method: "oauth", so the TUI can't tell which method the account was made with. If someone picks a Codex method to sign in again to an expired ChatGPT account (or the reverse), core now rejects it with "OAuth credential belongs to another method". Before this PR, that path worked and created a new credential.
Two options: show the credential's method ID on the connection and only send credentialID (or only show that method) when the methods match. Or have core and the plugin ignore the target when the method differs, instead of failing.
There was a problem hiding this comment.
Fixed in 9b73e65. After validating that the target exists, belongs to the integration and is OAuth, core treats a different OAuth method as a new connection without reusing its credential context. Same-method reauthorization still uses the selected registration. Added core completion, legacy-to-token-sharing URL and TUI method-selection regressions; the two cross-method tests fail before the fix and pass afterward.
| } | ||
| }) | ||
|
|
||
| describe("OAuth plugin authorization context", () => { |
There was a problem hiding this comment.
This block tests generic plugin-host and promise-adapter plumbing (passing context, single-argument compatibility) inside the ChatGPT provider tests, and it's large for what it checks. Please cut it down to the core cases: the ChatGPT authorize URL uses dynamic_agent_client when there's no target and the target's client ID when there is one. The credentialID checks can go in one small case in integration.test.ts.
There was a problem hiding this comment.
Removed the 130-line generic adapter block and its import in 9b73e65. Provider authorization coverage is now ChatGPT-specific; target validation and the same/cross-method context contract live in compact integration-service tests. The existing single-argument OAuth case remains.
Start independent authorization when a saved credential uses another OAuth method, while retaining validation of invalid and foreign targets.
|
@opencode-agent Both review comments are addressed in 9b73e65. Please re-review the latest commit. Switching OAuth methods now starts an independent connection, and the generic adapter test block has been removed. Local |
There was a problem hiding this comment.
Both concerns from my last review are fixed, and the new commit adds no problems I could find. I still couldn't confirm the actual fix, because OpenAI only returns the workspace denial on its live sign-in page and testing that needs two real ChatGPT accounts. On 9b73e65 I ran core provider-chatgpt and integration (49 passed) and the TUI integration dialog test (16 passed); typecheck passes in core and tui.
Fixed since last time:
- Picking a different method: in "Sign in required", choosing a method other than the account's own now starts a fresh connection instead of failing with "belongs to another method". When that fresh connection is "Sign in with ChatGPT", it uses
dynamic_agent_client, which the new ChatGPT test checks. - Large tests: the generic adapter block is gone from
provider-chatgpt.test.ts. What's left is small and specific to the fix.
Public API: no change since my last review, all additions and backwards compatible. The connect/oauth endpoint, OpenAPI files and generated clients take a new optional credentialID. Plugin OAuth authorize(answer) becomes authorize(answer, context?: { credential?: Credential.OAuth }), and existing plugins that take one argument keep working.
No remaining concerns.
|
Thanks. I already re-reviewed
On that commit, the core |
Regenerate clients with the current generator and keep the retired website OpenAPI artifacts removed.
|
Merged current dev (fa1260d) in 582746c and resolved the generated-client conflicts using the current generator. Kept the retired website OpenAPI files deleted. All six affected package suites pass (8,874 passed, 36 skipped), as do |
Issue for this PR
Closes #56298
Type of change
What does this PR do?
“Add account” reuses the active ChatGPT registration, so another account can be denied by OpenAI. New connections now use
dynamic_agent_client; reauthorization preserves the selected account's client ID through an optionalcredentialID. Selecting a different OAuth method starts an independent connection.The target is passed through the protocol, generated clients and plugin API. Existing callback checks and credential-persistence behavior are preserved. Synced with
devatfa1260d6ca; clients are regenerated with the current generator, and obsolete website OpenAPI files stay deleted.How did you verify your code works?
bun run checkandbun run typecheck:tscpass. Client generation is repeatable and the protocol OpenAPI freshness check passes.Visual
Screenshots / recordings
Authentication routing only; the TUI layout is unchanged.
Checklist