Add /logout slash command to the Warp TUI (APP-4919)#14110
Add /logout slash command to the Warp TUI (APP-4919)#14110warp-dev-github-integration[bot] wants to merge 3 commits into
Conversation
Add a /logout slash command to the headless Warp TUI that logs the user out (clears credentials + user-dependent state, tears down live terminal sessions) and returns the TUI to the existing login_placeholder auth page so the user can re-authenticate via the device-authorization flow without restarting the process. Spec scales to M-sized work: full Product (behavior invariants) + Tech (commit-pinned integration points across the slash registry, TUI execution path, RootTuiView state machine, TuiLoginModel/device-auth flow, TuiSessions lifecycle, and the GUI auth::log_out primitives) + exhaustive checkable validation criteria (regression tests, ./script/presubmit, and mandatory TUI visual proof for the user-facing change). Design alternatives documented; open questions resolved. Co-Authored-By: Warp <agent@warp.dev>
Co-Authored-By: Oz <oz-agent@warp.dev>
A stale AuthManagerEvent::AuthFailed from refresh/auth work started before an already-authenticated TUI mounted could flip TuiLoginModel from LoggedIn to Failed, which prevented /logout from later emitting LoggedOut. Guard against it by ignoring an AuthFailed while the phase is LoggedIn; failures after an explicit logout (phase back to AwaitingLogin) are still surfaced. Co-Authored-By: Oz <oz-agent@warp.dev>
|
@warp-dev-github-integration[bot] I'm starting a first review of this pull request. You can view the conversation on Warp. I completed the review and no human review was requested for this pull request. Comment Powered by Oz |
There was a problem hiding this comment.
Overview
This PR adds a TUI-only /logout slash command, wires it through the shared slash-command registry and TUI execution path, returns the root view to auth state, clears retained TUI sessions, and includes TUI-focused tests plus visual evidence in the PR description. spec_context.md contains no approved repository spec context, so there was no external spec to validate against.
Concerns
- The logout flow does not invalidate auth work that was already in flight before logout. A stale refresh or device-auth completion can still emit
AuthCompleteafter credentials are cleared and move the TUI back toLoggedInas the old user.
Security
- Stale pre-logout auth completions can restore the previous account after
/logout, which undermines the account-switch/data-isolation intent of the feature.
Verdict
Found: 0 critical, 1 important, 0 suggestions
Request changes
Comment /oz-review on this pull request to retrigger a review (up to 3 times on the same pull request).
Powered by Oz
| ); | ||
| } | ||
| AuthManagerEvent::AuthComplete => { | ||
| set_login_phase(ctx, TuiLoginPhase::LoggedIn); |
There was a problem hiding this comment.
AuthComplete, including an in-flight refresh/device-auth request that started before /logout; if it completes after log_out clears credentials and re-arms device auth, the old account can be restored. Invalidate or tag auth attempts so only the current post-logout flow can transition back to LoggedIn.
There was a problem hiding this comment.
Overview
Faithful implementation of the approved spec (committed at agents/specs/APP-4919: TUI logout slash command.md): all seven proposed changes landed — the LOGOUT static command (registered TUI-mode-only alongside /exit//view-logs), the TuiSlashCommand::Logout variant + from_static_command mapping, the TuiLoginEvent::LoggedOut event + pub fn log_out (re-exported as tui_log_out) reusing the GUI's canonical auth::log_out, the Logout dispatch arm mirroring /exit, the session.rs LoggedOut subscription that tears down sessions via TuiSessions::clear and flips the root via show_auth, and the clear()/show_auth helpers. The design choices are honored: reuse auth::log_out (no cross-account data leakage), immediate logout with no confirm, and re-arm via authorize_device.
I independently verified the gate rather than relying on the author's report: cargo check -p warp --features tui --tests --lib is clean; focused tui::tests are 4/4 (incl. both new regression tests emits_logged_out_event_when_login_phase_returns_to_auth and ignores_late_auth_failure_when_login_is_already_complete); cargo nextest run -p warp_tui --lib is 513/513 pass (the single session::tests::accepts_startup_without_resume failure is the documented WARP_API_KEY env artifact — reproduced here as Some("wk-1.…") vs None, and confirmed unrelated to this change); warp slash-command tests are 107/107 (incl. the new logout_command_executes_immediately_and_takes_no_argument); cargo clippy -p warp --features tui -p warp_tui --all-targets --tests -- -D warnings exits 0; cargo fmt --check is clean; CI (CodeQL + CLA) is green.
The self-teardown TOP risk (spec Tech Risks #2) is structurally eliminated, not just mitigated: ModelContext::emit/notify (crates/warpui_core/src/core/model/context.rs:206,249) push onto app.pending_effects, which the framework flushes only after the current model update returns. So the LoggedOut event reaches the session.rs subscription — and runs clear() + show_auth() — after execute_tui_slash_command is off the stack; the dispatching TuiTerminalSessionView is never torn down mid-handler. The existing LoggedIn flow relies on the same deferral in production. The DB-wipe behavior (risk #4) is documented in the Summary, and auth::log_out's singleton resets all ran without panic in the captured TUI run (risk #1).
Concerns
The one blocking issue is verification coverage for a core acceptance criterion, not a code defect — the code itself is sound.
AC3 / spec validation criterion 8(c) — "completing the device-authorization flow returns the user to a fresh working terminal session" — is unverified. The PR's visual proof demonstrates the novel behavior end-to-end (the /logout row in the menu, the screen returning to the "Sign in to continue" auth page, and the re-arm firing a fresh device-code request), but it cannot show re-auth completion: the headless sandbox has no browser/Secret Service, which the author discloses honestly. No deterministic test covers the logout→AuthComplete→fresh-session re-entry either. Unlike the self-teardown invariant (which is structurally guaranteed by the deferred-effect queue), this re-entry depends on runtime behavior that is not demonstrated. It is achievable headlessly: a test that drives log_out (sessions cleared, root Auth, authorize_device re-armed), then emits AuthComplete and asserts a fresh TuiSession is created, RootTuiView returns to Terminal, and TuiSessions holds exactly the new session with no stale sessions. Per the factory visual-proof rule, omitting a required acceptance criterion is blocking, so I'm requesting that deterministic test (or a credentialed-environment capture of the completed re-auth) before merge.
Two non-blocking follow-ups from the spec. Criterion 5 asked for a no-panic regression test guarding the self-teardown path — the invariant is already structurally guaranteed (above), so this is regression hardening rather than a live risk, but a test emitting LoggedOut with a focused session view registered (asserting no panic + TuiSessions cleared) would catch a future change making the teardown synchronous. Risk #3 asked the implementer to reason about the stale in-flight device-auth poll; AuthManager::authorize_device/log_out don't cancel a prior in-flight exchange_device_access_token spawn (the handle is dropped), so a stale poll completing could re-auth the old user via on_user_fetched — in the TUI flow this is essentially unreachable (being logged-in implies the prior exchange already completed), but a one-line note documenting why the residual risk is acceptable would satisfy the spec's "reason about it" requirement.
Minor: the PR description interleaves rework-chronicle framing ("This PR supersedes the spec-only draft", "Follow-up fix (this re-entry): A prior implementation run shipped the above…"). The "What changed" technical content is a clean current-state summary; consider trimming the iteration-history framing so the description reads as a single summary of the PR's net effect.
Verdict
Found: 0 critical, 1 important, 3 suggestions
Request changes
| TuiLoginEvent::LoggedIn => { | ||
| create_terminal_session_after_login(&sessions_for_login, &root_for_login, ctx); |
There was a problem hiding this comment.
/logout clears sessions, flips root to Auth, and re-arms authorize_device, a subsequent AuthComplete fires this LoggedIn arm to create a fresh session — but that re-entry is neither shown in the visual proof (the headless sandbox can't complete browser device-auth, disclosed by the author) nor covered by a deterministic test. Unlike the self-teardown invariant (structurally guaranteed by the deferred-effect queue), this depends on runtime behavior. Please add a headless test that drives log_out then emits AuthComplete and asserts a fresh TuiSession is created, RootTuiView returns to Terminal, and TuiSessions holds exactly the new session with no stale sessions. This is achievable without a browser and closes the core acceptance criterion.
| TuiLoginEvent::LoggedOut => { | ||
| sessions_for_login.update(ctx, |sessions, ctx| sessions.clear(ctx)); | ||
| root_for_login.update(ctx, |root, ctx| root.show_auth(ctx)); |
There was a problem hiding this comment.
💡 [SUGGESTION] Spec criterion 5 asked for a no-panic regression test guarding this self-teardown path. The invariant is already structurally guaranteed: emit/notify enqueue onto app.pending_effects (crates/warpui_core/src/core/model/context.rs:206,249) and are flushed after the current update returns, so this handler runs after execute_tui_slash_command is off the stack — the dispatching view is never torn down mid-handler. So this is regression hardening, not a live risk. A test that emits LoggedOut with a focused session view registered (asserting no panic + TuiSessions cleared) would catch a future change that makes the teardown synchronous.
| /// The login model event is delivered after the current command handler returns, | ||
| /// allowing the session owner to tear down the dispatching terminal view safely. | ||
| pub fn log_out(ctx: &mut AppContext) { | ||
| crate::auth::log_out(ctx); |
There was a problem hiding this comment.
💡 [SUGGESTION] Spec risk #3 asked to reason about the stale in-flight device-auth poll. AuthManager::authorize_device/log_out don't cancel a prior in-flight exchange_device_access_token spawn (the handle is dropped at auth_manager.rs:276,300), so a late poll completing could re-auth the old user via on_user_fetched. In the TUI flow this is essentially unreachable (being logged-in implies the prior exchange already completed), but a one-line note here or in the PR body documenting why the residual risk is acceptable would satisfy the spec's "reason about it" requirement.
There was a problem hiding this comment.
Additional blocking finding (security)
Supplements the review above. On re-examination — and consistent with the open oz-for-oss[bot] security comment on this line — the AuthComplete arm is a blocking security/correctness gap that the follow-up fix did not close.
AuthManagerEvent::AuthComplete is emitted by on_user_fetched on any successful user fetch, including a token refresh (refresh_user is reachable from the TUI via the agent SDK at app/src/ai/agent_sdk/mod.rs:1669 and admin.rs:93) and a prior device-auth exchange. on_user_fetched calls set_and_persist(Some(user), Some(credentials)) — restoring the old account to auth state and secure storage — and then emits AuthComplete (the emit at auth_manager.rs:499 is outside the if !from_refresh blocks, so refreshes emit it too). This arm unconditionally calls set_login_phase(LoggedIn) + activate_global_mcp_servers, with no guard. So if a refresh (or any in-flight auth request) is running when the user invokes /logout and completes after log_out clears credentials and re-arms device auth, the old account is silently restored: the explicit logout is defeated and the TUI flips back to LoggedIn as the prior account (cross-account leakage when switching accounts). The follow-up correctly guarded late ReceivedDeviceAuthorizationCode and AuthFailed events (via handle_received_device_authorization_code / handle_auth_failure), but AuthComplete — the one event that actually restores the account — is unguarded. This is spec risk #3, which the spec required to be mitigated or documented; it is neither.
Recommended fix: tag/invalidate auth attempts so only the current post-logout device-auth flow can transition back to LoggedIn — e.g., a generation counter bumped by log_out/authorize_device that on_user_fetched (or this handler) checks before restoring the user, ignoring completions from a prior attempt. Add a test that drives log_out with a simulated in-flight refresh completing afterward, asserting the phase stays AwaitingLogin and no user is restored.
Verdict
Found (this supplement): 1 important security finding.
Request changes — this confirms the reject. The two blocking items are: (1) the stale-AuthComplete account-restoration race above, and (2) the unverified AC3 / criterion 8(c) re-auth→fresh-session path from the first review. Both are achievable to fix and test headlessly.
| ); | ||
| } | ||
| AuthManagerEvent::AuthComplete => { | ||
| set_login_phase(ctx, TuiLoginPhase::LoggedIn); |
There was a problem hiding this comment.
AuthComplete arm unconditionally transitions to LoggedIn, so a stale in-flight auth request completing after /logout restores the old account and defeats the logout. AuthComplete is emitted by on_user_fetched on any successful fetch — including a token refresh (refresh_user is reachable from the TUI via the agent SDK at app/src/ai/agent_sdk/mod.rs:1669 and admin.rs:93) and a prior device-auth exchange — and on_user_fetched calls set_and_persist(Some(user), Some(credentials)) (restoring the old account to auth state + secure storage) before emitting AuthComplete (the emit at auth_manager.rs:499 is outside the if !from_refresh blocks, so refreshes emit it too). The follow-up guarded late ReceivedDeviceAuthorizationCode/AuthFailed but not AuthComplete — the one event that actually restores the account. Recommend tagging/invalidating auth attempts (e.g. a generation counter bumped by log_out/authorize_device that on_user_fetched or this handler checks) so only the current post-logout flow can transition back to LoggedIn, plus a test that drives log_out with a simulated in-flight refresh completing afterward (assert phase stays AwaitingLogin, no user restored). This is spec risk #3, unmitigated and undocumented.
Summary
Adds a
/logoutslash command to the headless Warp TUI (crates/warp_tui) that logs the user out (reusing the GUI's canonicalauth::log_outcleanup, which wipes the local DB and resets all user-dependent models — preventing cross-account data leakage), tears down the live terminal sessions, flipsRootTuiViewback to the existinglogin_placeholderauth page, and re-arms the device-authorization flow (AuthManager::authorize_device) so the user can re-authenticate without restarting the process.This PR supersedes the spec-only draft and now carries the full implementation plus a follow-up correctness fix.
What changed
app/src/search/slash_command_menu/static_commands/commands.rs): added theLOGOUTstatic command (/logout, "Log out and return to the sign-in page",Availability::ALWAYS), registered TUI-mode-only alongside/exitand/view-logs.app/src/terminal/input/slash_commands/mod.rs): added theLogoutvariant +from_static_commandmapping.app/src/tui/mod.rs): addedpub fn log_out(ctx)that callsauth::log_out, sets theTuiLoginModelphase back toAwaitingLogin(emitting a newTuiLoginEvent::LoggedOut), and re-armsauthorize_device. TheLoggedOutevent is delivered after the dispatching handler returns, so the session view is never torn down synchronously inside its own handler.crates/warp_tui/src/terminal_session_view.rs): added aTuiSlashCommand::Logoutarm mirroring/exit— clears the input, records telemetry, and calls the re-exported logout.crates/warp_tui/src/session.rs,root_view.rs,session_registry.rs): extended theTuiLoginModelsubscription to handleLoggedOut— a newTuiSessions::cleardrains/kills all sessions, andRootTuiView::show_authflips the root back to the auth state (the reverse of theLoggedIn→ terminal flow).app/src/tui_export.rs): re-exported the logout entry point for thewarp_tuicrate.Follow-up fix (this re-entry)
A prior implementation run shipped the above and verified it locally, but expired GitHub credentials blocked the final push. This run landed the remaining correctness fix on top:
AuthFailedat startup (app/src/tui/mod.rs): refresh/auth work started before an already-authenticated TUI mounts can report a lateAuthManagerEvent::AuthFailed, which would incorrectly flipTuiLoginModelfromLoggedIntoFailedand prevent/logoutfrom later emittingLoggedOut. A newhandle_auth_failurehelper ignores anAuthFailedwhile the phase isLoggedIn(mirroring the existing late device-authorization-code guard); failures after an explicit logout are still surfaced because the phase has returned toAwaitingLogin. TheAuthFailedmatch arm now routes through this helper.Verification
warp tui::tests::ignores_late_auth_failure_when_login_is_already_complete(new) — builds aLoggedInmodel, callshandle_auth_failure, and asserts the phase staysLoggedIn. Fails before the guard, passes after.cargo nextest run -p warp --features tui --lib -E 'test(/tui::tests/)'→ 4/4 pass (incl. the existingignores_late_device_code_when_login_is_already_completeand the new stale-AuthFailedtest).warp_tuisuite:cargo nextest run -p warp_tui --lib→ 513/514 pass. The single failure (session::tests::accepts_startup_without_resume) is an environment artifact: the sandbox hasWARP_API_KEYset, which clap reads intoTuiArgs::api_key, so the== Noneassertion fails — unrelated to this change../script/format --check✓,./script/check_no_inline_test_modules✓,cargo clippy -p warp --features tui --all-targets --tests -- -D warnings✓ (exit 0, no warnings).cargo nextest run --workspace --exclude command-signatures-v2exited 100, with failures confined to environment-dependent categories that don't touch this change's files: GUI integration tests (crates/integration, no display/X11 —records_window_target_via_native_x11grab_after_raiseetc.), SSH/remote-harnessshell_integration_tests::test_remote_server_*(password-prompt timeouts, "Address already in use"), and a deserialization fixture ("Failed to deserialize API keys"). These are the same unrelated integration/X11/SSH/remote-harness env failures the prior run observed.warp-tui-devbinary headlessly withWARP_API_KEY, drove it under tmux, and captured the real rendered screen.Logged-in zero state (before

/logout):/logoutappears in the TUI slash menu (typed/log):Auth page after executing

/logout("Sign in to continue" + "Opening your browser…"):Full
/logoutflow video (logged-in → menu → auth page):https://staging.warp.dev/artifact/019f87f8-254e-76a2-b704-d2d66d6ec8fa
Note on the re-auth surface: after
/logoutthe auth page shows "Sign in to continue" and "Opening your browser…" while the fresh device-authorization code is requested. In this headless sandbox the device code is requested but the browser open + secure-storage write is unavailable (no display / no Secret Service), so the verification URI + user code line doesn't populate here — the in-place re-arm path itself is exercised and confirmed.Changelog Entries for Stable
CHANGELOG-IMPROVEMENT: Added a
/logoutslash command to the Warp TUI that logs out and returns to the sign-in page.Originating thread: https://linear.app/warpdotdev/issue/APP-4919/add-a-logout-slash-command-to-the-warp-tui
This PR was generated with Oz.