fix(auth): preserve Auto file authority - #27
Conversation
cba33d5 to
4815c1d
Compare
46aa99f to
36b2b46
Compare
|
@codex review |
![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: 36b2b46ff3
ℹ️ 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".
| @@ -0,0 +1,105 @@ | |||
| use std::fs::OpenOptions; | |||
There was a problem hiding this comment.
This non-mechanical auth/concurrency change is 952 changed lines, exceeding the repo's 800-line guidance and 500-line target for complex logic. Please split it into reviewable stages, such as landing the file-authority storage marker and tests first, then guarded manager/imported-account refresh and logout behavior, then the external bearer refresh change.
Useful? React with 👍 / 👎.
| marker.clear()?; | ||
| if let Err(err) = delete_file_if_exists(&self.codex_home) { | ||
| warn!("failed to remove CLI auth fallback file: {err}"); | ||
| } | ||
| Ok(()) |
There was a problem hiding this comment.
When Auto has a marked file fallback and this keyring save succeeds but auth.json cannot be removed (for example due to a lock or permissions), clearing the marker before only warning about the delete failure leaves a fallback file that future keyring saves stop updating. If Auto later falls back because keyring is unavailable, it can read stale credentials and reuse an old refresh token; keep the marker active or return the delete error until the fallback is gone.
Useful? React with 👍 / 👎.
36b2b46 to
22e5905
Compare
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 22e5905cd8
ℹ️ 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".
| if let Some(auth) = self.file_authority.load_authoritative(&self.file_storage)? { | ||
| return Ok(Some(auth)); |
There was a problem hiding this comment.
This is now the first of several Codex++ file-authority decisions owned directly by the upstream storage.rs flow. /workspace/codex/AGENTS.md says fork-specific behavior should live in focused codex_plus_plus/ files and that upstream files should keep only minimal declarations, fields, and delegation calls; because this method and save_guarded now own the authority branching and fallback persistence path, future upstream rebases must reason through fork behavior here. Please delegate this Auto storage file-authority flow into the codex_plus_plus module and leave this file as the integration seam.
Useful? React with 👍 / 👎.
22e5905 to
30889d2
Compare
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 30889d2482
ℹ️ 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".
| "failed to delete auth from encrypted auth storage: {err}" | ||
| )) | ||
| })?; | ||
| let direct_removed = self.direct_storage.delete_with_guard(guard)?; |
There was a problem hiding this comment.
When the Secrets backend logout path reaches the legacy Direct keyring cleanup and that delete returns an error, this ? exits before auth.json or the file-authority marker are removed because DirectKeyringAuthStorage::delete_guarded deletes the file only after its own keyring delete succeeds. Before this change, the Secrets path removed the fallback file before attempting the legacy Direct cleanup, so in a Secrets/Auto install with a fallback file present, a legacy keyring delete error can leave usable file-backed credentials behind after logout.
Useful? React with 👍 / 👎.
30889d2 to
1159c6e
Compare
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1159c6e9e7
ℹ️ 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 file_removed = delete_file_if_exists(&self.codex_home)?; | ||
| let keyring_removed = self.delete_keyring()?; |
There was a problem hiding this comment.
When the stale fallback auth.json cannot be removed (for example it is a directory or has restrictive permissions), this ? returns before deleting the keyring entry, so logout in Keyring/Auto mode reports an error but leaves the actual credentials in the keyring and the user remains authenticated. The Secrets backend has the same ordering below; try deleting the keyring credentials regardless of fallback-file cleanup, then report any cleanup error.
Useful? React with 👍 / 👎.
1159c6e to
5126d58
Compare
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5126d589fd
ℹ️ 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 marker = FileAuthorityMarker::new(&self.codex_home); | ||
| marker.prepare_keyring_save(auth)?; | ||
| self.save_preserving_file(auth)?; | ||
| delete_file_if_exists(&self.codex_home)?; |
There was a problem hiding this comment.
When a file-authoritative Auto fallback is later migrated through keyring storage, this deletes auth.json while the authority marker is still active. If the process crashes in this window, or if marker.clear() fails after the delete, the next Auto load will see an active marker, require the now-missing file, and ignore the valid keyring copy, leaving the user unable to load auth. Clear the marker before removing the fallback file (and apply the same ordering in the Secrets helper below) so the failure mode falls back to keyring instead of a missing authoritative file.
Useful? React with 👍 / 👎.
…authority fix(auth): preserve Auto file authority
…authority fix(auth): preserve Auto file authority
1 participant
Why
When
Autoauth storage could not save to the keyring, it fell back toauth.jsonwithout recording that the file had become authoritative. A later process could then reload stale keyring credentials and reuse an already-rotated refresh token. Guarded refreshes also need storage reads and writes to honor the same cross-process lock. Separately, public refresh returned early for external bearer auth because its cached token is represented as API-key auth.What changed
Autostorage falls back toauth.json.Autoauth durably file-backed across loads, refreshes, and saves; only explicit Keyring save or logout clears authority.Validation
Autostorage.CARGO_BUILD_JOBS=2and--test-threads 1.git diff --check.The broad
codex-coresuite, broad Clippy, and Bazel validation were intentionally skipped for workstation resource safety after a prior broad nextest run nearly exhausted the machine. Deep Review Suite is run separately.