-
Notifications
You must be signed in to change notification settings - Fork 7.6k
feat(app-server): support external auth mode #10012
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
@codex review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1d8e3f6349
ℹ️ 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".
92fc6e1 to
066d53b
Compare
|
@codex review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 066d53b68e
ℹ️ 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".
|
@codex review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1437b502f5
ℹ️ 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".
|
@codex review |
|
Codex Review: Didn't find any major issues. Nice work! ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
|
@codex review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ef1bc6bb68
ℹ️ 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".
|
@codex review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 89629234c1
ℹ️ 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".
8fc3b23 to
083c2f1
Compare
|
@codex review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 083c2f1d87
ℹ️ 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".
|
@codex review |
|
Codex Review: Didn't find any major issues. What shall we delve into next? ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
|
chatted offline, we'll hide the ephemeral logic in |
|
@codex review |
|
Codex Review: Didn't find any major issues. Keep it up! ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
0129f58 to
6a0c20f
Compare
|
@codex review |
885db36 to
d307824
Compare
|
the code lgtm, can we also test out that there's no regression & test out the new endpoints (log in, refresh) in a local build end-to-end? |
|
@celia-oai yep, tested this build locally as well |
|
I think we need some documentation that declares the intent behind this
codex/codex-rs/core/src/model_provider_info.rs Lines 140 to 144 in b79bf69
To me, it feels like |
|
This is maybe OK for now, but ultimately, I think we should be focused on pulling the auth tokens out of the app server rather than pushing them in. That is, I would rather see a new |
This enables a new use case where
codex app-serveris embedded into a parent application that will directly own the user's ChatGPT auth lifecycle, which means it owns the user’s auth tokens and refreshes it when necessary. The parent application would just want a way to pass in the auth tokens for codex to use directly.The idea is that we are introducing a new "auth mode" currently only exposed via app server:
chatgptAuthTokenswhich consist of theid_token(stores account metadata) andaccess_token(the bearer token used directly for backend API calls). These auth tokens are only stored in-memory. This new mode is in addition to the existingapiKeyandchatgptauth modes.This PR reuses the shape of our existing app-server account APIs as much as possible:
account/login/startwith a newchatgptAuthTokensvariant, which will allow the client to pass in the tokens and have codex app-server use them directly. Upon success, the server emitsaccount/login/completedandaccount/updatednotifications.account/chatgptAuthTokens/refreshwhich the server can use whenever the access token previously passed in has expired and it needs a new one from the parent application.I leveraged the core 401 retry loop which typically triggers auth token refreshes automatically, but made it pluggable:
account/chatgptAuthTokens/refresh, the client responds with updated tokens, codex updates its in-memory auth, then retries. This RPC has a 10s timeout and handles JSON-RPC errors from the client.Also some additional things:
account/logoutclears external auth in memoryforced_chatgpt_workspace_idis set via the user's config, we respect it in both:account/login/startwithchatgptAuthTokens(returns a JSON-RPC error back to the client)account/chatgptAuthTokens/refresh(fails the turn, and on next request app-server will send anotheraccount/chatgptAuthTokens/refreshrequest to the client).