test(tui): name the command a dispatch smoke stalls on, and skip /pin on Windows

[Hmbown]

Closes #5919

The offender: /pin

The stderr breadcrumbs in run 34013201624 / job 101432543520 name it. In
every_registered_command_dispatches_to_a_handler the last line is:

dispatch smoke: /pin

with no matching /pin returned in …. In every_command_alias_dispatches_to_a_handler
the last line is dispatch smoke: /mini — and mini is the first alias of
pin (crates/tui/src/commands/groups/core/pin.rs). Two independent tests,
same handler, neither returned.

/pin is not a state toggle. It calls tui::window_control::toggle_pin(),
which resolves the host terminal window (GetConsoleWindow, else the
foreground window, else an EnumWindows walk of the parent process chain)
and then drives it with Win32 SetWindowPos / ShowWindow. That HWND
belongs to a different process: those calls are delivered to that window's
thread and block until it pumps them. On a headless CI window station there
is nothing pumping, so the call never returns and the test runs out the
600 s nextest timeout.

What this PR does

1. Per-command watchdog — each dispatch now builds its app and runs
   execute on its own thread; the test waits 30 s for the result. A
   blocking handler now fails the test in milliseconds, naming the
   invocation, instead of costing a ten-minute CI slot with no attribution.
   Handler panics still surface as that panic (the payload is resumed), which
   is a property the smoke tests exist for.
2. Skip /pin on Windows only, with the reason in the doc comment next
   to the existing /restore skip. On macOS and Linux toggle_pin() is a
   compiled-out no-op, so dispatch coverage for /pin is kept there.
3. Drops the unconditional per-command eprintln pair (≈15k log lines per
   run) whose only job the watchdog now does; a single line remains for any
   dispatch slower than a second.

No production code changed — the diff is entirely inside #[cfg(test)] mod tests.

Verified locally (macOS, aarch64)

cargo fmt --all -- --check                                    clean (exit 0)
cargo test -p codewhale-tui --lib -- commands::tests::every_
  test result: ok. 3 passed; 0 failed; 0 ignored; 0 measured; 11704 filtered out
cargo test -p codewhale-tui --lib -- commands::
  test result: ok. 977 passed; 0 failed; 0 ignored; 0 measured; 10730 filtered out

cargo check -p codewhale-tui does not compile cfg(test) code, so the
cargo test runs above are the compile evidence for this diff.

The watchdog mechanism itself was proved, not assumed:

• DISPATCH_WATCHDOG temporarily set to Duration::ZERO → the test failed
  in 0.05 s with /anchor did not return within 0ns: its handler blocks.
• a panic temporarily injected into the worker thread → propagated to the
  test, with the handler's name on the thread (thread 'dispatch-smoke-anchor').

Both temporary edits were reverted before the runs quoted above.

What only Windows CI can prove

I am on macOS, so I could not reproduce the hang. The /pin attribution is
read off the CI breadcrumbs plus the Win32 code, not from a Windows run;
toggle_pin() compiles to false on this host, so the skip is a no-op
locally and the two tests would pass here either way. The acceptance
criterion "Windows matrix green on three consecutive runs" can only be
checked on windows-latest.

Also deliberately not attempted: hardening window_control::toggle_pin()
itself (e.g. SWP_ASYNCWINDOWPOS / ShowWindowAsync so the TUI thread never
blocks on the terminal host's message pump). That is a real improvement for
Windows users, but it changes the semantics of the existing
apply-then-verify-then-retry logic and I cannot run it on this host. It
belongs in its own issue with a Windows author.

🤖 Generated with Claude Code

https://claude.ai/code/session_0188XYyJaw9Mh9uSrqQBoqhm

---

Note

Low Risk
Test-only changes in the TUI command module; no runtime or dispatch logic in production builds.

Overview
Fixes Windows CI hangs (#5919) where the “every command dispatches” smoke tests could stall for the full nextest timeout when /pin (and alias /mini) blocked on Win32 window calls in headless CI.

/pin is excluded from the smoke loop on Windows only, with the same style of rationale as the existing /restore skip; macOS/Linux still exercise /pin because toggle_pin() is a no-op there.

The smoke tests now run each dispatch on a dedicated thread with a 30s watchdog, so a blocking handler fails fast and names the invocation instead of burning CI time. Handler panics still propagate via resume_unwind. Verbose per-command eprintln breadcrumbs were removed; only dispatches slower than one second log a line.

All changes are under #[cfg(test)] in commands/mod.rs — no production behavior changes.

^{Reviewed by Cursor Bugbot for commit e09fc7d. Bugbot is set up for automated code reviews on this repo. Configure here.}

[cursor]

Bugbot couldn't run - usage limit reached

Bugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit.

A user or team admin can review and increase usage limits in the Cursor dashboard.

(requestId: serverGenReqId_351bd910-d7af-4f1e-8d87-61b1b4911ad4)

[chatgpt-codex-connector]

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review	Status	Commit	Review trigger
📝 Code Review	✅ Completed 2026-09-06T06:19:06.553837Z	e09fc7d	PR opened

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

[claude]

Claude finished @Hmbown's task in 4m 2s —— View job

---

Review: PR #5922

• Gather context (PR body, diff, changed file)
• Read crates/tui/src/commands/mod.rs diff in full
• Review watchdog thread mechanism for correctness (panics, timeouts, thread leaks)
• Review the Windows /pin skip logic
• Check provider/model safety implications (N/A — test-only diff)
• Post final review

Scope confirmed: diff is entirely inside #[cfg(test)] mod tests in crates/tui/src/commands/mod.rs (+73/-23), matching the PR description. No production code touched.

Finding (posted inline at line 1634)

The watchdog's own ConfigPathGuard/lock_test_env() can leak the process-wide test-env mutex on a real timeout.

create_isolated_test_app() acquires lock_test_env() — a process-wide Mutex<()> used by ~30+ other test call sites across runtime_api/tests.rs, model_inventory.rs, config_persistence.rs, etc. — inside ConfigPathGuard. That guard now lives inside dispatch_under_watchdog's spawned worker thread. On RecvTimeoutError::Timeout, the main test thread panic!s and returns immediately (good — that's the intended fast, named failure), but the worker thread genuinely stuck in a blocking handler is never joined or killed, so its ConfigPathGuard never drops and the mutex is never released.

• Under cargo nextest run this is harmless: nextest isolates each test into its own process, so a leaked thread in a doomed process can't touch anything else.
• Under plain cargo test — which this repo also uses, including the verification commands quoted in this very PR (cargo test -p codewhale-tui --lib -- commands::tests::every_) — all tests in the binary share one process. A future hanging handler (exactly the class of bug this watchdog exists to catch) would fail its own test fast and by name, but then leave the mutex permanently locked for the rest of that run, so every later test calling lock_test_env() hangs too — trading one attributable failure for a cascade of unattributed ones.

Suggested direction: build the App/tempdir and acquire the env-lock guard on the main thread before spawning, moving only the already-constructed App into the worker for the execute() call, so the guard drops promptly no matter what the worker does. Would need to confirm App: Send.

This doesn't affect the specific bug this PR fixes (/pin is skipped on Windows, so no current hang triggers it), but it's a real gap in the general-purpose mechanism the PR introduces for catching future hangs.

Good choices

• dispatch_under_watchdog cleanly dedupes the ~20 lines of duplicated per-command dispatch code across both smoke tests.
• Panic message names the exact command/alias that blocked, which is a real improvement over inferring the offender from the last eprintln! breadcrumb in a truncated CI log (the actual mechanism used to attribute /pin in Windows CI: commands::tests::every_registered_command_dispatches_to_a_handler and the alias variant hang to the 600 s nextest timeout #5919).
• The alias-loop skip correctly checks command.name (not the alias string), so /mini — pin's alias — is properly excluded alongside /pin on Windows.
• Handler panics are still forwarded via resume_unwind rather than swallowed, preserving the smoke tests' role as the only panic-in-a-handler net.
• 16 MiB worker stack size is called out as matching the CI RUST_MIN_STACK for the main thread — avoids a stack-overflow false failure from moving the recursive dispatch onto a fresh thread.

No other correctness, provider/model-safety, or security issues found in this test-only diff.

[Copilot]

🟡 Changes recommended

The watchdog timeout path can leave a blocked worker thread holding the global test env mutex and the success path currently risks swallowing worker-thread panics by ignoring join() errors.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

This PR updates the TUI command dispatch smoke tests to fail fast (and attribute the offending command) when a handler blocks on Windows CI, addressing the /pin-related hang described in #5919. All changes are confined to #[cfg(test)] code in the TUI commands module.

Changes:

• Add a per-command watchdog that runs each dispatch in a dedicated thread and times out after 30s, surfacing panics from handlers.
• Skip /pin on Windows in the dispatch smoke loops, with an in-code rationale tied to the Win32 window-control behavior.
• Reduce log noise by removing unconditional per-command breadcrumbs and only logging slow dispatches.

File summaries

File	Description
crates/tui/src/commands/mod.rs	Refactors dispatch smoke tests to use a per-command watchdog thread, skips /pin on Windows, and reduces per-command logging.

Review details

• Files reviewed: 1/1 changed files
• Comments generated: 2
• Review effort level: Lite

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[claude]

create_isolated_test_app() → ConfigPathGuard::new() acquires lock_test_env(), a process-wide Mutex<()> (crates/tui/src/test_env_lock.rs:159) shared by dozens of other tests (runtime_api/tests.rs, model_inventory.rs, config_persistence.rs, …).

That guard now lives inside the spawned worker thread's closure. On the RecvTimeoutError::Timeout path, dispatch_under_watchdog panic!s on the main test thread and returns — but the worker thread that's actually stuck in the blocking handler is never joined or killed, so its ConfigPathGuard is never dropped and the global env mutex is never released.

Under cargo nextest run this is contained (nextest runs each test in its own process, so a leaked thread in a doomed process can't affect anything else). But under plain cargo test — which this repo also uses, including the verification commands quoted in this PR's own description (cargo test -p codewhale-tui --lib -- commands::tests::every_) — all tests in this binary share one process and its threads. If a future handler hangs (the exact scenario this watchdog exists to catch), the offending test now fails fast and by name as intended, but it leaves the shared mutex permanently locked for the rest of that cargo test invocation, so every other test that later calls lock_test_env() (and there are ~30+ call sites) hangs too — trading one attributable timeout for an unattributed pile of them.

Suggest acquiring the env-lock guard (and building App/tempdir) on the main thread before spawning, and only moving the already-built App into the worker for the execute() call — that way the guard drops promptly regardless of whether the worker ever returns. Worth double-checking App: Send if going this route.

[codewhale-agent]

Codewhale review

PR adds a per-command 30s watchdog for the TUI dispatch smoke tests and skips /pin on Windows to avoid the CI hang from Win32 window calls. The change is test-only in commands/mod.rs.

Findings

• [WARNING] Watchdog timeout leaks the worker thread and its isolation guard (crates/tui/src/commands/mod.rs)
  When a handler blocks longer than DISPATCH_WATCHDOG, the test panics but the worker thread remains parked inside execute(). Rust cannot kill threads, so the TempDir and the _guard returned by create_isolated_test_app() are never dropped. If that guard serializes process-wide state (e.g. current directory or environment), later tests may hang or run in the wrong directory, turning a single timeout into a cascade. Consider a design that can clean up on timeout, such as running each command in a separate process or otherwise avoiding leaked guards.
• [INFO] No automated regression test for the watchdog itself (crates/tui/src/commands/mod.rs)
  The watchdog timeout and panic-resume paths are not covered by a committed test; the PR only verified them temporarily with local edits. A future refactor could break the watchdog without any test failure. Consider adding a small test-only command that blocks or panics and asserting that the smoke test fails with the named invocation.
• [INFO] Worker panic after sending a result is swallowed (crates/tui/src/commands/mod.rs:1644)
  In the Ok(message) arm, let _ = handle.join(); discards the join result. If the worker thread panics after sending the result (for example while dropping _guard or tmpdir), the test does not fail even though the stated goal is to surface all handler panics. Consider resuming a join error here as well.

Assessment

The change is reasonable and directly addresses the Windows CI hang, but the leaked worker thread/guard on watchdog timeout is a latent hazard for subsequent tests, and the watchdog lacks committed regression coverage.

---

Advisory review by Codewhale (codewhale review --pr 5922 --post, head e09fc7db1790958bb57b39d7c1c38637eadc0bc8). Line-specific findings are also posted as inline review comments; mechanical fixes arrive as committable suggestions you can apply from the Files tab. CODEOWNERS approval still governs merge.

[codewhale-agent]

[INFO] Worker panic after sending a result is swallowed

In the Ok(message) arm, let _ = handle.join(); discards the join result. If the worker thread panics after sending the result (for example while dropping _guard or tmpdir), the test does not fail even though the stated goal is to surface all handler panics. Consider resuming a join error here as well.