Stop account switches from duplicating managed homes - #563
Open
skyeyesec333 wants to merge 1 commit into
Open
skyeyesec333 wants to merge 1 commit into
skyeyesec333 wants to merge 1 commit into
Conversation
materialize_as_managed minted a fresh UUID home on every call. Because switch_account calls it whenever the outgoing ambient account differs from the target, every switch away from the ambient identity left behind another managed home. One install reached 24 homes for 2 accounts. Each duplicate was seeded with fs::copy from the ambient auth.json, so once those credentials went stale every new home carried the same expired token. Reuse a managed home that already holds credentials for this account, creating a fresh one only when none matches. When reusing, refresh the stored auth.json only if the ambient copy is at least as recently refreshed.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (1)
Included review availability: Your plan provides up to 4 included reviews per hour; 3 remain after this review. 📝 WalkthroughWalkthroughChangesManaged home reuse
Priority: ⬇️ Low Estimated code review effort: 3 (Moderate) | ~20 minutes Change: Bug fix Sequence Diagram(s)sequenceDiagram
participant materialize_as_managed
participant existing_managed_home_matching
participant managed_home_directory
materialize_as_managed->>existing_managed_home_matching: Find matching managed home
existing_managed_home_matching->>managed_home_directory: Discover homes and accounts
managed_home_directory-->>existing_managed_home_matching: Return matching path or None
existing_managed_home_matching-->>materialize_as_managed: Return destination
materialize_as_managed->>managed_home_directory: Copy auth.json or create fresh home
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
materialize_as_managedminted a fresh UUID home on every call.switch_accountcalls it whenever the outgoing ambient account differs from the target (account_manager.rs:228-233), so every switch away from the ambient identity left another managed home behind. Nothing deduped or pruned them. My install reached 24 managed homes for 2 accounts over about 10 days of normal switching.Each duplicate was seeded with
fs::copyfrom the ambientauth.json. Once those credentials went stale, every home created afterwards carried the same expired token, the provider failedAuthentication requiredon every poll, backoff eventually silenced it, and recovery meant deleting the account store by hand.This makes
materialize_as_managedreuse a managed home that already holds the account credentials, creating a fresh one only when none matches. The existing-home scan uses the samediscovered_managed_account+matchespattern thatmanaged_home_paths_matchingalready uses for removal.When reusing a home, the stored
auth.jsonis refreshed only if the ambient copy is at least as recently refreshed (last_refresh). Without that guard, reuse would let a stale ambient file overwrite live managed credentials, which is the step that turned duplicate directories into an unusable app. Unreadable or undated credentials fall back to the previous behaviour.Related issue
Fixes #561
Related, both closed, same warning from a different cause: #468 and #533. Those concern credentials that are still valid being rejected by the 8-day
last_refreshgate. This PR addresses the path where credentials genuinely expire because a frozenauth.jsonis cloned into each new managed home and never refreshed.Affected areas
Account store only (
codex-accounts/managed-homes,accounts.json). No UI, tray or frontend changes.Validation
Developed on Linux against the
rustcrate. The Windows slice has not been run, see Notes.The 4 full-suite failures reproduce identically on a clean tree with this patch stashed, so they are not from this change. They look like Windows path assertions running on a Linux host and may pass on Windows:
Clippy reported 14 pre-existing findings across
providers/andagent_sessions/on a newer toolchain than CI pins. None are incodex_accounts/, and this PR adds none.Tests added in
rust/src/codex_accounts/account_manager.rs:materialize_reuses_the_existing_home_for_the_same_account: two successive calls return the same home and the directory count stays at 1materialize_creates_a_home_when_no_managed_copy_matches: an unrelated account home is not reused and a new home is createdmaterialize_does_not_overwrite_newer_managed_credentials: a stale ambientauth.jsonleaves newer managed credentials byte-identicalpowershell.exe -ExecutionPolicy Bypass -NoProfile -File scripts\local-check.ps1Thermo-nuclear code quality review completed before submitting
Both are left unchecked because I could not run them. Please treat the hosted Windows gate as the real check.
UI / tray proof
rust/src/codex_accounts/.Notes for reviewers
rustcrate, soscripts\local-check.ps1and the Tauri shell clippy pass have not run, and the change has not been exercised inside the running app. Happy to add results if you would rather I do that first.accounts.jsonat startup would be a reasonable follow-up.auth.jsonchanges, and capping the backoff, would close that.Authentication requiredis only visible in the log file. A sign-in affordance on the affected account row would make this self-service.