Iaw/feature/account switcher - #1388
Conversation
ilysenko
left a comment
There was a problem hiding this comment.
Thanks for the substantial work on this feature. I found several blockers:
- Preserve the real upstream default Electron user-data profile instead of substituting a new
codex-desktop/electronpath. - Redesign shared-local mode so it never shares credential-bearing shell snapshots or account-scoped state, and migrate SQLite/session data only while offline with rollback and crash recovery.
- Perform account handoff through the supported launcher/AppRun lifecycle, wait for replacement readiness, and do not force-kill arbitrary active descendants.
- Apply persisted shared/isolated state using the actual production hook order.
- Use one strict, path-contained profile-ID contract in both JS and shell.
- Prevent background refresh from overwriting newer registry mutations.
- Report missing or duplicate preload anchors as enabled-feature drift and add report-level regression tests.
- Remove the unrelated
.gitignorecleanup and update the documentation for stored metadata and authenticated network requests.
Please add executable lifecycle, cold-restart, concurrent-write, failed-handoff, active-SQLite/WAL, and AppImage composition tests for these paths.
Finally, please run the complete updated base-to-head diff through your own code-review model, fix every blocker it finds, rerun the review, and repeat until that model reports no remaining blockers before requesting maintainer re-review.
|
Let me know if this works. I had good luck with it, burned many tokens on the automated tests just to be sure. |
|
This has been working for me in everyday use testing as well. |
ilysenko
left a comment
There was a problem hiding this comment.
Thanks for the updates; the unrelated installer changes are now removed. I still found blockers on 4051376:
- Keep each SQLite database and its WAL/SHM together. A source database without WAL currently receives the target profile's WAL; the reproduction loses source rows and leaves an incomplete target backup (
shared-state.sh:543-582,899-928). - Preserve existing shared/target data before committing shared-local migration. Target-only projects and divergent rollout continuations are overwritten, and their journal backups are deleted. Also, the shell wrapper removes the previous shared JSON before the helper reads it, so a missing source global-state file wipes existing shared metadata (
shared-state.sh:500-523,954,977;shared-state-json.js:66-100). Removing shared-local from this PR is also an acceptable way to narrow the change. - Make handoff routing/prepared flags single-use and validate their live handoff ownership. Inherited PROFILE overrides the persisted selection, while inherited MIGRATION_PREPARED bypasses recovery and prelaunch safety checks (
launcher-hook.sh:12-14,69,177;prelaunch-hook.sh:46-47). - Check for a live default Electron process before interrupted-handoff rollback. Recovery currently relies on a --user-data-dir argument that the default profile intentionally does not pass; the stronger prelaunch check runs too late (
launcher-hook.sh:117-118;shared-state.sh:190-198). - Select AppImage/AppRun only when it belongs to this installation; an executable inherited APPIMAGE/APPDIR currently relaunches an unrelated application (
after-exit-hook.sh:155-160). - Resolve the launcher and Nix conflicts with current main without reverting its fixes/pins, then validate account-switcher alone against the latest signed Linux package. Existing CI covers 26.820.80927; the newer locally installed preload no longer contains the literal usesOwlAppShell anchor, so verify/update that contract on a clean current package.
Please add focused regression coverage for these cases. Run the complete updated base-to-head diff through your own code-review model, fix every blocker it finds, rerun the review, and repeat until that model reports no remaining blockers before requesting maintainer re-review.
…-switcher # Conflicts: # launcher/start.sh.template # nix/upstream-linux-packages.json
ilysenko
left a comment
There was a problem hiding this comment.
Thanks for the substantial fixes. I re-reviewed the complete current diff and found four remaining blockers:
- Make SQLite rollback generation-safe. If the merge helper is killed after committing but before closing, it can create a new WAL/SHM after the pre-merge snapshot; rollback restores the old main database but leaves that new sidecar, so reopening the restored database replays the wrong generation and loses/corrupts shared rows (
shared-state.sh:543-552). - When routing to the default profile, explicitly restore
CODEX_HOMEto the base/default home. An inherited named-profileCODEX_HOMEcurrently survives while the hook emitsPROFILE=default, so Electron opens the default profile but the app server can use another account's auth/state (launcher-hook.sh:212-221). - Link an existing shared SQLite catalog into a new/empty target profile. The target loop currently skips a catalog when the target main file does not exist, so the first launch of a new shared-local profile silently creates an isolated database instead (
shared-state.sh:1008-1018). - Make shared JSON reconciliation honor authoritative deletions and ordering. The recursive set/union merge resurrects deleted projects/thread assignments and preserves stale array order, so users cannot persist legitimate removals or reorders (
shared-state-json.js:57-66,118-121).
Please add focused regression tests for these cases. Please run the complete updated base-to-head diff through your own code-review model, fix every blocker it finds, rerun the review, and repeat until that model reports no remaining blockers before requesting maintainer re-review.
|
Not sure I agree with the closing of this. This feature is working and in my daily testing I haven't experienced any breaking bugs. |
|
Thank you for all the work you put into this feature. After considering its scope and complexity, I’ve decided not to merge it into the repository. It touches several sensitive areas and carries too much risk of breaking existing behavior. You’re welcome to keep using it as a local feature if it works well for your setup. We may revisit the idea in the future, especially if a simpler and safer approach becomes possible. |
Summary
Adds an opt-in Linux account switcher below Log out in the bottom-left profile menu.
The feature supports:
.env,.env.*, and.envscredential files.Validation
node --test scripts/patch-linux-window-ui.test.js scripts/lib/linux-features.test.js linux-features/account-switcher/test.jsnode --check linux-features/account-switcher/patch.jsbash -n linux-features/account-switcher/launcher-hook.shgit diff --checkaccount-switcherenabled.Checklist