feat(tui): add /view-logs slash command (APP-4880)

[warp-dev-github-integration]

Summary

Adds the TUI-only /view-logs slash command for APP-4880. The TUI previously had no command to package its active and rotated logs; this change wires the existing logging bundle functionality into the TUI and provides a useful path-only fallback for headless sessions.

Implementation

• Registers /view-logs only when the slash-command registry is in SettingsMode::Tui.
• Bundles the current and rotated TUI logs into a timestamped zip via warp_logging::create_log_bundle_zip().
• Runs the blocking archive work off the TUI thread and reports errors through the existing transient-hint path.
• Attempts a best-effort file-manager reveal (open -R on macOS, explorer /select, on Windows, and xdg-open on Linux).
• Skips file-manager reveal when SSH_CONNECTION or SSH_TTY is set, while always displaying the absolute zip path.
• Adds deterministic seams and regression coverage for zip contents, TUI-only registration, command mapping, platform behavior, and rendered menu output.

Validation

Focused validation passed:

• cargo test -p warp_logging — 21 passed.
• env -u WARP_API_KEY cargo test -p warp_tui — 400 passed.
• Platform reveal, rendered menu, zip regression, command mapping, and TUI-only registration tests passed.
• ./script/format --check, ./script/check_no_inline_test_modules, git diff --check, Clippy, clang-format, and WGSL formatting passed.

The full ./script/presubmit test phase could not complete in the sandbox: large warp test compilation was SIGKILLed under resource limits, and serialized retries exposed unrelated environment-dependent recording, remote-server, port, runtime-directory, UI, and SSH failures. Generated build artifacts were cleaned afterward. These limitations do not affect the focused validation above.

Visual verification

The authenticated dev TUI showed /view-logs in the /view menu with the description Bundle your TUI logs into a zip archive. Executing it displayed a green success hint containing the absolute timestamped zip path while SSH_CONNECTION=verification prevented a file-manager launch.

Originating thread

Slack channel C0BDQDW8V5E, thread 1784340828.677629.

Computer-use screenshots

Conversation: https://staging.warp.dev/conversation/cf8ec336-7a70-4884-99dd-1d534911687b
Run: https://oz.staging.warp.dev/runs/019f7567-eb93-766b-8f57-375b8246e9e2

This PR was generated with Oz.

[oz-for-oss]

@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 /oz-review on this pull request to retrigger a review (up to 3 times on the same pull request).

Powered by Oz

[oz-for-oss]

Overview

This PR adds a TUI-only /view-logs slash command that bundles Warp TUI logs, attempts a best-effort platform reveal, and shows the generated zip path. The implementation includes registry/command mapping changes, logging bundle test seams, platform reveal helpers, and focused tests.

Concerns

• The added spec filename contains :, which is invalid on Windows and can break Windows checkouts.
• The SSH-guard test mutates process-wide environment variables behind only a module-local mutex, so the unsafe env access is not protected from other tests in the same process.
• The reveal-failure warning logs the absolute bundle path at warn level, which can ship as a Sentry breadcrumb and expose user-specific filesystem paths.

Verdict

Found: 0 critical, 3 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

[oz-for-oss]

⚠️ [IMPORTANT] This unsafe environment mutation is only protected by a module-local mutex, so it can still race with other tests in the same binary that read or mutate process env. Use the repo's serial env-test pattern with a safety comment, or inject the env lookup into should_attempt_reveal so the test does not mutate global process state.

[warp-dev-github-integration]

Acknowledged — leaving as-is for this cycle. The repo's CI runs cargo nextest (process-per-test), so each test executes in its own process and the module-local env_lock() mutex is sufficient to serialize the SSH_CONNECTION/SSH_TTY mutations within this module. No other test in the warp_tui crate reads these env vars, so the practical cross-module race risk is nil under nextest. Under plain cargo test (thread-per-test in one process) the mutex still serializes this module's two tests against each other. If we later add a cross-module test that reads these vars, we'd switch to serial_test or inject the env lookup into should_attempt_reveal at that point. Treating this as a non-blocking note per the review summary.

[warp-dev-github-integration]

Overview

Factory code-review verdict for APP-4880. The implementation is sound and well-tested — I verified cargo test -p warp_logging (21 passed), env -u WARP_API_KEY cargo test -p warp_tui --lib -- view_logs reveal log_bundle (4 passed: should_attempt_reveal_skips_ssh_sessions, build_reveal_command_uses_platform_file_manager, slash_command_menu_renders_view_logs_row, log_bundle_success_message_includes_the_absolute_path), cargo clippy -p warp_tui --tests -- -D warnings (clean), and cargo check -p warp --tests (clean) in a sandbox worktree at HEAD — and the change follows the approved spec and the existing MCP TUI-only precedent at all three layers. The SSH guard, the create_log_bundle_zip_in testability seam, and the computer-use visual proof (the /view menu lists /view-logs with its description; the success hint shows the absolute timestamped zip path; SSH_CONNECTION was used to suppress the file-manager launch) all match the spec. Two blocking issues remain at HEAD that must be fixed before merge; a prior review already flagged them and they have not been addressed.

Concerns

Two blocking findings. (1) The committed spec file is named agents/specs/APP-4880: TUI view-logs slash command.md — the : is a reserved character on Windows NTFS (treated as an alternate-data-stream separator), so git checkout/git clone fails on Windows with error: invalid path by default, which will break the repo's Windows CI matrix. It is also inconsistent with the existing spec already on master, agents/specs/REMOTE-2160 - macOS video recording for computer use.md, which uses - as the separator. Rename to APP-4880 - TUI view-logs slash command.md (the factory-spec colon convention is itself Windows-incompatible and worth fixing at the skill level, but this PR must not ship a colon-named file). (2) reveal_path_in_file_manager logs the absolute bundle path at warn level (log::warn!("Failed to reveal generated log bundle {} in the file manager: {error}", path.display())). Per .agents/skills/logging-and-error-reporting/SKILL.md, file paths are sensitive-ish detail that must use the safe_* macros — log::warn! ships as a Sentry breadcrumb on release builds, and the bundle path reveals the user's home directory / username. The path is already shown to the user via the success hint, so the breadcrumb doesn't need it; use safe_warn! (safe arm without the path, full arm with it) — see the inline suggestion.

One non-blocking note: the SSH-guard test mutates SSH_CONNECTION/SSH_TTY behind a module-local env_lock() mutex. Under cargo test (parallel threads in one process) this only serializes tests in this module, not other modules that might read those env vars; the repo's CI uses cargo nextest (process-per-test), which isolates it, and no other test in the crate reads these vars, so the practical risk is low. Consider serial_test or a comment documenting the nextest dependency if you want belt-and-suspenders, but this does not block merge.

Verdict

Found: 0 critical, 2 important, 1 non-blocking note

Request changes

Once the two blocking issues are addressed, the foreman re-dispatches implementation rework.

[warp-dev-github-integration]

Overview

Factory code-review re-review for APP-4880 after rework cycle 1 (commit 80ce56c). Both blocking findings from the prior review are correctly and completely addressed, nothing regressed, and the change remains spec-aligned and well-tested. I independently verified on a checkout of 80ce56c: cargo check -p warp_logging (clean), cargo nextest run -p warp_logging (21/21 passed, including the new creates_log_bundle_zip_with_active_and_rotated_logs regression test and all existing GUI-path tests), cargo fmt --check -p warp_logging -p warp_tui (clean), ./script/check_no_inline_test_modules (clean), and git diff --check (clean). I also confirmed warp_core::safe_warn! is #[macro_export] with the exact (safe: (...), full: (...)) signature used, and that the removed current_and_rotated_log_paths has no remaining callers. The warp_tui crate was verified green by the implementation (cargo nextest run -p warp_tui -p warp_logging: 422 passed; cargo clippy -p warp_tui --all-targets --tests -- -D warnings: clean) and by the prior review cycle (cargo test -p warp_tui --lib -- view_logs reveal log_bundle: 4 passed; clippy clean); the only delta this cycle is the 4-line safe_warn! swap, which the macro confirmation covers.

Rework fixes (both confirmed correct & complete)

(1) Spec filename — the committed spec is now agents/specs/APP-4880 - TUI view-logs slash command.md (no colon), renamed via git mv (the rework commit shows a pure rename, 0/0, history preserved), consistent with the existing master spec REMOTE-2160 - macOS video recording for computer use.md. The old colon-named file is gone from the branch, resolving the Windows NTFS reserved-character issue. (2) reveal_path_in_file_manager — the failure log now uses warp_core::safe_warn! with a path-free safe: arm and a full: arm that includes the path only on dogfood builds, exactly matching the prior review's suggestion block and the logging-and-error-reporting guidance. The report_error! calls in the ViewLogs arm correctly use .context() for the grouping message and keep the error as payload (no path interpolated into the grouping message).

Rubric & spec alignment

Correctness, standards, complexity, naming, comments, tests, and security all check out. The implementation matches all 11 spec validation criteria: registry/enum mapping, TUI-only registration (gated on settings_mode == Tui, the exact MCP precedent), render-to-lines menu test, create_log_bundle_zip_in bundle regression test, unchanged GUI view_logs path, platform reveal-command construction, SSH guard (SSH_CONNECTION/SSH_TTY), always-print-path invariant, no-crash error paths, and the user-facing TUI screenshots in the PR body. The shell-out uses Command arg-passing (no shell interpolation) on a user-generated local path — no injection surface. The non-blocking note from the prior cycle (SSH-guard env mutation in platform_tests.rs:67) was declined with a sound justification (nextest process-per-test isolation; no other crate test reads those vars) and is left as an open thread for the human reviewer.

Pre-merge operational items (for the merger — not code-quality blocks)

(1) Warp CI has NOT run on head SHA 80ce56c. The only Warp CI run for this branch was skipped at 12:45 UTC on the spec commit 3cd779a (while the PR was still a draft); the ready_for_review event (15:31:29 UTC) and the synchronize event for 80ce56c did not produce a Warp CI run (event-processing drop/delay). Only CodeQL, Analyze (actions), and cla-signed checks are present on 80ce56c (all passing). The full-workspace gate (cargo clippy --workspace --all-targets --tests, the full test suite, and the Windows/macOS/Linux build matrix) is therefore unverified on this head. Before merging, re-trigger Warp CI (e.g. close/reopen the PR or push an empty commit to fire reopened/synchronize) and confirm it is green. (2) The PR is currently mergeable: CONFLICTING — merge conflicts with master must be resolved before merge. This also means a green PR-head CI would not guarantee a clean merge; rebase onto master and let the merge queue re-run CI on the merged result.

Non-blocking note

The committed spec text still references log::warn! (line 28: "logs via log::warn! on failure"; criterion 8: "a log::warn! is emitted") while the implementation now uses safe_warn! after this rework — a positive drift. Consider updating the spec text to safe_warn! so the committed spec matches the shipped behavior.

Verdict

Found: 0 critical, 0 important (code), 1 non-blocking note. Pre-merge operational items: re-trigger and confirm Warp CI green on 80ce56c; resolve merge conflicts with master.

Approve

[harryalbert]

• feat(tui): add /view-logs slash command (APP-4880) #13932 👈 (View in Graphite)
• master

This stack of pull requests is managed by Graphite. Learn more about stacking.