Skip to content

Stop account switches from duplicating managed homes - #563

Open
skyeyesec333 wants to merge 1 commit into
nesszer:mainfrom
skyeyesec333:fix/managed-home-reuse
Open

skyeyesec333 wants to merge 1 commit into
nesszer:mainfrom
skyeyesec333:fix/managed-home-reuse

Conversation

@skyeyesec333

@skyeyesec333 skyeyesec333 commented Sep 19, 2026

Copy link
Copy Markdown

Summary

materialize_as_managed minted a fresh UUID home on every call. switch_account calls 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::copy from the ambient auth.json. Once those credentials went stale, every home created afterwards carried the same expired token, the provider failed Authentication required on every poll, backoff eventually silenced it, and recovery meant deleting the account store by hand.

This makes materialize_as_managed reuse a managed home that already holds the account credentials, creating a fresh one only when none matches. The existing-home scan uses the same discovered_managed_account + matches pattern that managed_home_paths_matching already uses for removal.

When reusing a home, the stored auth.json is 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_refresh gate. This PR addresses the path where credentials genuinely expire because a frozen auth.json is cloned into each new managed home and never refreshed.

Affected areas

  • Config file / settings persistence
  • Provider-specific behavior
  • Tray panel
  • Settings UI
  • CLI
  • Installer / release packaging
  • Startup / background behavior
  • Documentation

Account store only (codex-accounts/managed-homes, accounts.json). No UI, tray or frontend changes.

Validation

Developed on Linux against the rust crate. The Windows slice has not been run, see Notes.

cargo test --manifest-path rust/Cargo.toml --lib codex_accounts::account_manager
    8 passed (3 new)

cargo test --manifest-path rust/Cargo.toml --lib
    1881 passed, 4 failed (pre-existing, see below)

cargo clippy --manifest-path rust/Cargo.toml --all-targets
    no findings in the changed file

cargo fmt -- --check rust/src/codex_accounts/account_manager.rs
    clean

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:

agent_sessions::pi_family::pi_family_tests::config_dir_must_stay_within_home
agent_sessions::pi_family::pi_family_tests::windows_shims_and_paths_are_normalized_for_dialect
codex_sessions::tests::normalizes_codex_root_to_sessions_dir
core::hooks::tests::config_matching_respects_enabled_flag

Clippy reported 14 pre-existing findings across providers/ and agent_sessions/ on a newer toolchain than CI pins. None are in codex_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 1

  • materialize_creates_a_home_when_no_managed_copy_matches: an unrelated account home is not reused and a new home is created

  • materialize_does_not_overwrite_newer_managed_credentials: a stale ambient auth.json leaves newer managed credentials byte-identical

  • powershell.exe -ExecutionPolicy Bypass -NoProfile -File scripts\local-check.ps1

  • Thermo-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

  • Not applicable. No UI, tray, settings chrome or visual change. The diff touches only account-store file handling in rust/src/codex_accounts/.

Notes for reviewers

  • Not validated on Windows. This was written and tested on Linux against the rust crate, so scripts\local-check.ps1 and 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.
  • Reuse picks the lowest-sorted matching home so repeated calls are stable. Installs that already accumulated duplicates will settle on one of them. This PR does not prune the rest, and pruning homes not referenced by accounts.json at startup would be a reasonable follow-up.
  • Backoff is not addressed here and is the more damaging half of the original failure. After a sustained auth failure the Codex provider stops retrying and does not resume when credentials change, so only an app restart recovers it. Re-arming the poller when auth.json changes, and capping the backoff, would close that.
  • Authentication required is only visible in the log file. A sign-in affordance on the affected account row would make this self-service.

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.
@coderabbitai

coderabbitai Bot commented Sep 19, 2026

Copy link
Copy Markdown

Review Change StackReview Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Advanced

Run ID: 5c5c60f3-a8df-45fa-93fc-bbe87cef6bc3

📥 Commits

Reviewing files that changed from the base of the PR and between 10e3b09 and 1f3e153.

📒 Files selected for processing (1)
  • rust/src/codex_accounts/account_manager.rs

Included review availability: Your plan provides up to 4 included reviews per hour; 3 remain after this review.


📝 Walkthrough

Walkthrough

Changes

Managed home reuse

Layer / File(s) Summary
Home selection and materialization
rust/src/codex_accounts/account_manager.rs
materialize_as_managed reuses the lowest-named managed home for the account. It creates a new home when no matching home exists.
Credential freshness and validation
rust/src/codex_accounts/account_manager.rs
Credential freshness uses last_refresh. Tests cover home reuse, non-matching accounts, and preservation of newer managed credentials.

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
Loading

Suggested reviewers: finesssee, xuelongmu

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 66.67% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 15 functions across 1 files. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and concisely describes the main change: preventing duplicate managed homes during account switches.
  • Fix all pre-merge checks with AI
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create a new PR

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug]: Account switching creates a new managed home each time, seeded with stale credentials

1 participant