Immutable CodexAuth #8857
Immutable CodexAuth #8857
Conversation
…cess sync - Add AuthManager::auth_cached and move stale-token refresh logic into AuthManager - Make CodexAuth::get_token/get_token_data synchronous and remove CodexAuth::refresh_token - Change auth_provider_from_auth and BackendClient::from_auth to non-async - Replace direct CodexAuth usage with AuthManager where appropriate and use cached auth for UI/status
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d8143b7a97
ℹ️ 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".
| let token_data = match auth.get_token_data() { | ||
| Ok(data) => data, |
There was a problem hiding this comment.
This enforcement now reads auth.get_token_data() without any refresh path, but get_token_data no longer refreshes stale ChatGPT tokens. If last_refresh is older than the refresh interval (e.g., user changed workspaces or lost access since the last refresh), this check will validate against an outdated ID token and allow login; later calls to AuthManager::auth() will refresh, but there is no re-check, so the workspace restriction can be bypassed until manual logout. Consider refreshing here (e.g., via AuthManager::auth() or an explicit refresh-if-stale) before validating the workspace ID.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
I think this is a legit bug, but it's a pretty extreme edge case. Could add a TODO and address it in a follow-on PR.
There was a problem hiding this comment.
My understanding is that refreshing the token doesn't refresh the workspace (same way it doesn't pick up the plan_type).
There was a problem hiding this comment.
OK, then it should be fine.
etraut-openai
left a comment
etraut-openai
left a comment
There was a problem hiding this comment.
The new separation of concerns is a big improvement. I should have thought of this when I added the AuthManager in the first place. Thanks!
Codex found one bug related to workspace enforcement, but it's a pretty small hole. Could leave it for a later PR IMO, but it would be good to at least add a TODO.
| let token_data = match auth.get_token_data() { | ||
| Ok(data) => data, |
There was a problem hiding this comment.
I think this is a legit bug, but it's a pretty extreme edge case. Could add a TODO and address it in a follow-on PR.
Historically we started with a CodexAuth that knew how to refresh it's own tokens and then added AuthManager that did a different kind of refresh (re-reading from disk).
I don't think it makes sense for both
CodexAuthandAuthManagerto be mutable and contain behaviors.Move all refresh logic into
AuthManagerand keepCodexAuthas a data object.