test: isolate CODEWHALE_HOME in the provider onboarding tests

[Hmbown]

Refs #5932 (targeted guard; the process-wide guard and a hermetic CI home both stay open there — each broke tests that rely on an implicit home, receipts on the issue)

The onboarding tests in tui::ui::provider_key_validation_tests guarded CODEWHALE_CONFIG_PATH but not CODEWHALE_HOME, so completing onboarding wrote setup_state.json into the developer's real ~/.codewhale (tonight's full-suite runs left the founder's provider step recorded as fixture-local). ConfigPathEnvGuard now sets both to one temp dir.

Verified: RUST_MIN_STACK=16777216 cargo test -p codewhale-tui --lib -- tui::ui::provider_key_validation_tests → 15 passed, 0 failed, with the real ~/.codewhale/setup_state.json mtime unchanged across the run.

---

Note

Low Risk
Test-only environment isolation; no production runtime or persistence behavior changes.

Overview
Fixes #5932 by extending ConfigPathEnvGuard in provider_key_validation_tests so onboarding runs against a temp home, not the developer's real ~/.codewhale.

The guard already pointed CODEWHALE_CONFIG_PATH (and DEEPSEEK_CONFIG_PATH) at a temp config.toml, but onboarding completion still persisted setup_state.json and settings.toml via CODEWHALE_HOME. Full-suite runs could therefore record fixture provider progress in the user's actual setup state.

ConfigPathEnvGuard::new() now builds a single temp .codewhale directory, sets CODEWHALE_HOME to that path alongside the existing config env guards, and documents why both are required.

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

[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:59:18.779721Z	4025ede	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.

[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_cf49479e-0a29-482d-9975-78de7837befb)

[claude]

Claude finished @Hmbown's task in 59s —— View job

---

Review: test(tui) isolate CODEWHALE_HOME in provider onboarding tests

• Gather context (diff, related test file)
• Understand request (code review of test isolation fix)
• Review correctness / provider-model-route safety / reuse / tests / security
• Post final review

Summary: Small, well-targeted fix. ConfigPathEnvGuard::new() in crates/tui/src/tui/ui.rs (provider_key_validation_tests) now also sets CODEWHALE_HOME via EnvVarGuard, pointed at the same tempdir used for CODEWHALE_CONFIG_PATH/DEEPSEEK_CONFIG_PATH. This closes the real leak described in #5932: onboarding completion writes setup_state.json/settings.toml against CODEWHALE_HOME, which was previously unguarded, so the fixture-local provider was landing in the developer's real ~/.codewhale.

Correctness

• Confirmed CODEWHALE_HOME is an established, hard-override env var throughout the workspace (crates/paths/src/lib.rs, automation_manager.rs, runtime_log.rs, etc.) — when set, it is the home dir directly (no .codewhale appended). Here home = tmp.path().join(".codewhale") is just a path value; the literal name doesn't matter functionally, and config_path = home.join("config.toml") is unchanged from before, so no regression to the existing config-path assertions.
• EnvVarGuard::set (crates/tui/src/test_support.rs:193) asserts the calling thread holds lock_test_env(), which ConfigPathEnvGuard::new already acquires before constructing the guards — safe.
• Field/drop order: _codewhale_home is declared before _lock, so it (along with the other env guards) restores its value before the test-env lock is released — correct ordering, and since the three guarded vars are independent keys, relative drop order among them doesn't matter.
• This is consistent with the same isolation pattern used elsewhere (e.g. provider_picker.rs, settings.rs, config.rs) that pairs a sealed CODEWHALE_HOME with lock_test_env().

Provider/model/route safety: N/A — no provider-prefix inference logic touched.

Reuse/simplification: Reuses the existing EnvVarGuard/TestEnvLock primitives; no new abstraction introduced. Good instinct to make this a minimal, targeted fix per the PR description rather than trying to fix the "systemic guard" in this PR.

Tests: This is the test fix; the PR body reports 15 passed, 0 failed for the affected module and verifies the real ~/.codewhale/setup_state.json mtime is unchanged, which is exactly the right evidence for this kind of regression (proving isolation held, not just that the suite passed).

Security: No secret handling changes; test-only.

No issues found — this is a correct, minimal, well-evidenced fix.

[Copilot]

🔵 Needs a closer look

ConfigPathEnvGuard declares the env lock field last, causing it to drop first and potentially restore env vars without holding the required mutex (violating EnvVarGuard’s safety contract).

Pull request overview

This PR tightens test isolation for tui::ui::provider_key_validation_tests by ensuring onboarding-related persistence targets a temporary $CODEWHALE_HOME rather than the developer’s real ~/.codewhale, preventing setup_state.json/settings pollution as described in #5932.

Changes:

• Extend the tests’ ConfigPathEnvGuard to also set CODEWHALE_HOME to the same temp .codewhale directory as the config path.
• Adjust the constructed config_path to live under that isolated temp home (<tmp>/.codewhale/config.toml).
• Add an explanatory comment documenting why CODEWHALE_HOME must be guarded for these tests.

File summaries

File	Description
crates/tui/src/tui/ui.rs	Updates the provider onboarding test harness to isolate CODEWHALE_HOME alongside CODEWHALE_CONFIG_PATH to prevent real home-directory writes.

Review details

Suppressed comments (1)

crates/tui/src/tui/ui.rs:1285

• EnvVarGuard requires the test-env lock to be held until after the guard is dropped (its Drop uses unsafe { set_var/remove_var } under that assumption). With _lock declared last, Rust will drop _lock first, so the env-var guards may restore variables without holding the mutex, reintroducing cross-test races.

        _codewhale_home: crate::test_support::EnvVarGuard,
        _codewhale_config_path: crate::test_support::EnvVarGuard,
        _deepseek_config_path: crate::test_support::EnvVarGuard,
        _lock: crate::test_support::TestEnvLock,
    }

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

---

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

[codewhale-agent]

Codewhale review

Test-only isolation fix: ConfigPathEnvGuard now sets CODEWHALE_HOME to the same temp .codewhale directory as CODEWHALE_CONFIG_PATH, preventing onboarding setup files from being written to the real user home.

Findings

• [INFO] No automated regression assertion for home isolation (crates/tui/src/tui/ui.rs:1297)
  The PR verifies via a manual mtime check that the real ~/.codewhale/setup_state.json is not modified, but the test suite itself does not assert that onboarding writes setup_state.json under the guarded temp CODEWHALE_HOME or that the previous home is untouched. If the guard is later removed or bypassed, the existing tests can still pass while polluting the developer's home.

Suggestions

• crates/tui/src/tui/ui.rs:1297 — Add an automated regression check in the provider onboarding test, or a helper assertion, that setup_state.json is created under the temporary CODEWHALE_HOME and not under the original CODEWHALE_HOME. This makes the fix self-verifying instead of relying on manual mtime validation.

Assessment

The change is correct and low-risk: it follows the existing env guard pattern and redirects both CODEWHALE_HOME and CODEWHALE_CONFIG_PATH consistently. The only gap is lack of automated coverage for the regression, which would be nice to add but does not block merging.

---

Advisory review by Codewhale (codewhale review --pr 5933 --post, head 4025edec248dd735f1dbaace01ab4d81ee6fb832). 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] No automated regression assertion for home isolation

The PR verifies via a manual mtime check that the real ~/.codewhale/setup_state.json is not modified, but the test suite itself does not assert that onboarding writes setup_state.json under the guarded temp CODEWHALE_HOME or that the previous home is untouched. If the guard is later removed or bypassed, the existing tests can still pass while polluting the developer's home.

[codewhale-agent]

Add an automated regression check in the provider onboarding test, or a helper assertion, that setup_state.json is created under the temporary CODEWHALE_HOME and not under the original CODEWHALE_HOME. This makes the fix self-verifying instead of relying on manual mtime validation.

[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_c86f542f-191e-40f6-a1d7-e15b55bf5ee1)

[claude]

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

---

Review: test(tui) isolate ~/.codewhale from unit tests (#5933)

• Gather context (diff, related files)
• Understand request (code review of the test-isolation fix, including the field-drop-order concern raised in the Copilot review)
• Review correctness / provider-model-route safety / reuse / tests / security
• Post final review

Summary: Solid, well-evidenced fix for #5932 — both the targeted ConfigPathEnvGuard fix and the systemic install_test_home_override mechanism are sound. One dead-code nit found; the field-order concern raised by the Copilot review is a false positive.

Correctness

• The Copilot review comment claims declaring _lock: TestEnvLock last in ConfigPathEnvGuard (crates/tui/src/tui/ui.rs:1284) makes it drop first, releasing the mutex before the env guards restore their values. This is backwards: Rust struct fields drop in declaration order (first-declared field drops first), the opposite convention from local let bindings (which drop LIFO). So _lock, being declared last, is dropped last — after _codewhale_home, _codewhale_config_path, and _deepseek_config_path have already restored their env vars. That's the order EnvVarGuard's safety contract requires, not a violation of it. This is also the established convention throughout the codebase — every other TestEnvLock-holding test fixture in the tui crate (tui/ui/tests.rs, views/skills_manager.rs, views/mod.rs, commands/mod.rs, doctor_fix.rs, config_persistence.rs, etc.) places _lock last for the same reason. No change needed here.
• crates/paths/src/lib.rs: codewhale_home() correctly checks the explicit codewhale_home_override() (env var) before falling back to TEST_HOME_OVERRIDE, so an explicit CODEWHALE_HOME still wins as documented. OnceLock::set is inherently race-safe, so installing the override outside lock_test_env()'s mutex (crates/tui/src/test_env_lock.rs:160) is fine — no TOCTOU issue.
• crates/tui/src/test_support.rs:44-46: isolated_test_state_root() now does a root.join("implicit-home") allocation and an install_test_home_override call (an Err-returning no-op after the first time) on every call, not just on first init. Harmless — test-only, cheap — but could be moved inside the get_or_init closure to avoid the redundant work on the hot path.

Provider/model/route safety: N/A — no provider-prefix inference logic touched.

Reuse/simplification

• crates/paths/src/lib.rs:134-137: test_home_override() (the getter) is added but never called anywhere in this PR or the existing codebase — the new regression test in test_support.rs reads codewhale_home() and compares it to isolated_test_state_root() directly instead of using this getter. Per the ponytail rule ("does this need to exist?"), this is dead code and should either be used by the test it was written for or dropped.
• install_test_home_override/test_home_override are #[doc(hidden)] but not #[cfg(test)]-gated in codewhale-paths, so they're part of the crate's real (if hidden) public API in production builds, even though only #[cfg(test)] code in the tui crate calls them today. Minor — this is an internal workspace crate, not published externally — but worth a passing thought if this pattern spreads.
• Otherwise this reuses existing primitives (EnvVarGuard, TestEnvLock, OnceLock) with no new abstraction layer — consistent with rung 2/6 of the ponytail method.

Tests: The new an_unsealed_test_never_resolves_the_developers_real_home test (crates/tui/src/test_support.rs) directly closes the gap the codewhale-agent review flagged on the ui.rs diff — it proves, for the whole test process rather than one test module, that an unsealed test's codewhale_home() and SetupState::path() resolve under the isolated root and never the real home. Good, targeted regression coverage; no further tests needed.

Security: No secret handling changes; test/CI-only. .github/workflows/ci.yml correctly pre-creates CODEWHALE_HOME before nextest run so the hermetic home exists before any test writes to it.

Verdict: No blocking issues. The one real (if minor) actionable item is the unused test_home_override() getter — either wire it into the new regression test or remove it.

Fix this →

[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_931dfb4a-34b3-4ea6-9b2b-44a336e03379)

[claude]

Claude finished @Hmbown's task in 3m 49s —— View job

---

Review: test(tui) isolate CODEWHALE_HOME (#5933)

• Gather context (diff, related files)
• Review correctness / provider-model-route safety / reuse / tests / security
• Post final review

Summary: The core mechanism (comparing the current user_home() against the real_user_home captured at install time, so a test that legitimately redirects HOME still falls through correctly) is a sound design, and the explicit-CODEWHALE_HOME-wins ordering, CI hermetic home, and new regression test are all good. However, the wiring reintroduces an unsynchronized env-var read into the exact code path this file's own safety comments say must be mutex-protected.

Findings

1. Correctness/soundness — unsynchronized HOME read races with mutex-protected HOME mutation (crates/tui/src/test_env_lock.rs:160, crates/paths/src/lib.rs:142-152)

lock_test_env() calls crate::test_support::ensure_test_home_override() before env_lock().lock() (test_env_lock.rs:159-161). That call reaches install_test_home_override, which unconditionally calls user_home() — an unsynchronized std::env::var_os("HOME") — on every invocation, not just the first (the OnceLock::set no-ops after the first success, but the user_home() read before it still runs every time).

Meanwhile, dozens of existing tests do EnvVarGuard::set("HOME", tmp.path()) while holding that same mutex (e.g. crates/tui/src/tui/ui/tests.rs:1930, crates/tui/src/tui/provider_picker.rs:5445, crates/tui/src/tui/app/tests.rs:3976) — and EnvVarGuard::set's own safety comment (test_support.rs:212) says // SAFETY: callers hold the process-wide test env mutex, precisely because std::env::set_var/var are not safe to call concurrently from different threads.

ensure_test_home_override() is also called directly from test_tui_options() (crates/tui/src/test_support.rs:307) with no lock at all — e.g. crates/tui/src/tui/context_inspector.rs:1035 calls test_support::test_tui_options(...) without ever acquiring lock_test_env(). So under plain multi-threaded cargo test (which .config/nextest.toml's own header comment calls "the authoritative release gate," distinct from nextest's one-process-per-test model that happens to sidestep this), a thread building TuiOptions via test_tui_options() can read HOME at the same moment another thread is mid-set_var/remove_var on it under the lock — the same class of race this whole file exists to prevent.

Fix: move the override installation so it only ever reads user_home() either (a) once, inside the OnceLock::get_or_init closure in isolated_test_state_root() (already the pattern used for ROOT), and (b) only from call sites that already hold lock_test_env() — or acquire the lock internally before calling install_test_home_override.

Fix this →

2. Reuse — test_home_override() getter is still dead code (crates/paths/src/lib.rs:158)

Already flagged in an earlier review pass on this PR; still unused anywhere in the diff or repo. The new regression test in test_support.rs re-derives the same fact by comparing codewhale_home() against isolated_test_state_root() directly instead of calling this getter. Per the ponytail rule, either wire it into that test or drop it.

Good choices worth noting

• codewhale_home()'s comparison against the captured real_user_home (paths/lib.rs:114-123) correctly lets a test that redirects HOME itself (legacy-migration/product-dir tests) fall through to the real logic instead of being hijacked by the override — this was the tricky part to get right.
• Explicit CODEWHALE_HOME still short-circuits before the override is consulted (paths/lib.rs:111-113), matching the PR's stated invariant.
• CI change correctly pre-creates the hermetic CODEWHALE_HOME directory before nextest run (ci.yml).
• New an_unsealed_test_never_resolves_the_developers_real_home test is a well-targeted regression check for the actual Test isolation leak: tui::ui::tests onboarding tests persist a fixture provider into the real ~/.codewhale/setup_state.json #5932 failure mode.
• The Copilot review's field-drop-order concern on ConfigPathEnvGuard (_lock declared last) is a false positive: Rust struct fields drop in declaration order (first-declared drops first), so _lock — declared last — drops last, after the env guards restore their values. No change needed there.

[codewhale-agent]

Codewhale review

PR attempts to isolate all unit tests from the developer's real ~/.codewhale via a process-wide test home override, targeted CODEWHALE_HOME guards, and hermetic CI env. The direction is sound, but the override installation is vulnerable to a parallel-test race, and the CI step has a Windows shell portability problem.

Findings

• [ERROR] Test home override may be installed against a redirected HOME before the env lock is held (crates/tui/src/test_env_lock.rs:160)
  In lock_test_env, ensure_test_home_override() is called before env_lock().lock(). With parallel tests, a waiting test can execute this while another test holds the lock and has temporarily changed HOME to a temp directory. install_test_home_override then persists that temp path as real_user_home; once HOME is restored, later implicit codewhale_home() lookups compare the real HOME against the saved temp path and skip the override, so they fall back to the developer's real ~/.codewhale. test_tui_options has the same lazy-install risk. The override should be installed once before any env-mutating test runs, or ensure_test_home_override should be moved inside the env lock after acquisition.
• [WARNING] CI step uses bash-only mkdir on Windows runners (.github/workflows/ci.yml:599)
  The updated step runs mkdir -p "${CODEWHALE_HOME}" under the repository's default shell. On Windows the default Actions shell is pwsh, where ${CODEWHALE_HOME} does not expand to the environment variable and mkdir -p is not bash syntax. This can fail to create the hermetic home directory or fail the workflow on the matrix's Windows jobs. Add shell: bash to this step or use a cross-platform creation command.
• [INFO] Test-only override is exposed in production codewhale-paths (crates/paths/src/lib.rs:141)
  install_test_home_override and test_home_override are #[doc(hidden)] but public and unconditionally compiled into production. Production code can accidentally mutate global path resolution for all later codewhale_home() calls. Consider gating these hooks behind a test-utils feature or cfg(test) equivalent.
• [INFO] Local non-TUI test binaries are not protected by the process-wide override
  The override is installed only from crates/tui/src/test_support.rs. Running e.g. cargo test -p codewhale-config locally does not install it, so an unsealed test in those crates can still hit ~/.codewhale. CI hides this because CODEWHALE_HOME is set globally, but the local cargo test -p path is not isolated for every crate.

Suggestions

• .github/workflows/ci.yml:598 — Force bash for this step so the mkdir -p command and environment variable expansion behave consistently on Windows runners.

          run: |
            mkdir -p "${CODEWHALE_HOME}"
            cargo nextest run --workspace --all-features --locked --profile ci
          shell: bash
  

Assessment

The isolation strategy is reasonable but not yet safe under the parallel Rust test harness. Fix the lazy override ordering before merge, and make the CI directory creation cross-platform.

---

Advisory review by Codewhale (codewhale review --pr 5933 --post, head 97288e0c14755bdfb0435ca12f627e2042677c41). 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]

[ERROR] Test home override may be installed against a redirected HOME before the env lock is held

In lock_test_env, ensure_test_home_override() is called before env_lock().lock(). With parallel tests, a waiting test can execute this while another test holds the lock and has temporarily changed HOME to a temp directory. install_test_home_override then persists that temp path as real_user_home; once HOME is restored, later implicit codewhale_home() lookups compare the real HOME against the saved temp path and skip the override, so they fall back to the developer's real ~/.codewhale. test_tui_options has the same lazy-install risk. The override should be installed once before any env-mutating test runs, or ensure_test_home_override should be moved inside the env lock after acquisition.

[codewhale-agent]

[WARNING] CI step uses bash-only mkdir on Windows runners

The updated step runs mkdir -p "${CODEWHALE_HOME}" under the repository's default shell. On Windows the default Actions shell is pwsh, where ${CODEWHALE_HOME} does not expand to the environment variable and mkdir -p is not bash syntax. This can fail to create the hermetic home directory or fail the workflow on the matrix's Windows jobs. Add shell: bash to this step or use a cross-platform creation command.

[codewhale-agent]

[INFO] Test-only override is exposed in production codewhale-paths

install_test_home_override and test_home_override are #[doc(hidden)] but public and unconditionally compiled into production. Production code can accidentally mutate global path resolution for all later codewhale_home() calls. Consider gating these hooks behind a test-utils feature or cfg(test) equivalent.

[codewhale-agent]

Force bash for this step so the mkdir -p command and environment variable expansion behave consistently on Windows runners.

Suggested change

	run: |
	mkdir -p "${CODEWHALE_HOME}"
	cargo nextest run --workspace --all-features --locked --profile ci
	run: |
	mkdir -p "${CODEWHALE_HOME}"
	cargo nextest run --workspace --all-features --locked --profile ci
	shell: bash

[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_654f7dca-e5a8-44ba-86be-4529127b5bc2)

[claude]

Claude finished @Hmbown's task in 3m 20s —— View job

---

Review: test(tui) isolate CODEWHALE_HOME (#5933)

• Gather context (diff vs branch point 464ced2, confirm current PR content matches metadata)
• Review correctness / provider-model-route safety / reuse / tests / security
• Reconcile prior bot findings against the current diff
• Post final review

Note on scope: the current diff is exactly .github/workflows/ci.yml (+6/-1) and crates/tui/src/tui/ui.rs (+7/-1) — confirmed via git diff 464ced2 HEAD. Several earlier review comments on this PR (Devin's race-condition finding, codewhale-agent's install_test_home_override finding) discuss crates/paths/src/lib.rs, test_support.rs, and test_env_lock.rs changes that are not present in this diff — that was an earlier, since-withdrawn "systemic guard" revision (the PR body confirms it: "the process-wide guard stays open there with findings", i.e. deferred back to #5932). Those findings no longer apply to what's actually here.

Findings on the current diff

1. [Correctness, confirmed] Run tests step breaks on windows-latest (.github/workflows/ci.yml:598-604)
The new run: | mkdir -p "${CODEWHALE_HOME}" ... block has no shell: key. This step's if: (matrix.os != 'ubuntu-latest' || github.event_name == 'pull_request') runs it on all three matrix legs for a PR, including windows-latest, whose default shell is pwsh (verified: no top-level defaults.run.shell in this file, and every other step needing bash elsewhere in this same job explicitly sets shell: bash). In pwsh, ${CODEWHALE_HOME} is PowerShell's brace-variable syntax for a PowerShell variable, not $env:CODEWHALE_HOME — there is no such variable, so it evaluates empty, and mkdir -p "" fails the step before cargo nextest even runs. Test (windows-latest) is a required check, so this would redden every PR.
Fix: add shell: bash to this step.

2. [Correctness, confirmed] Globally-set CODEWHALE_HOME breaks log_directory_prefers_home (.github/workflows/ci.yml:604, crates/tui/src/runtime_log.rs:211-227,402-415)
log_directory() checks codewhale_home_override() (raw CODEWHALE_HOME env var) before falling back to user_home() — by design, per its own comment, so an explicit override is never silently bypassed. The CI change now sets CODEWHALE_HOME for the entire Run tests step, i.e. for every test process nextest spawns. log_directory_prefers_home (runtime_log.rs:402) only redirects HOME/USERPROFILE, never touches CODEWHALE_HOME, and asserts log_directory() == tmp.path().join(".codewhale").join("logs"). With the CI env var present, this resolves to ${{runner.temp}}/cw-test-home/.codewhale/logs instead, and the assertion fails — on Linux and macOS too, not just Windows. This is a direct, deterministic consequence of the exact codewhale_home_override()-first logic the PR's own thread praised as correct; it isn't hypothetical.
There may be other tests with the same shape (redirect HOME/USERPROFILE, assert against an implicit .codewhale-under-home path, never touch CODEWHALE_HOME) that this CI change puts at equal risk — worth a quick grep -rn 'set_var("HOME"' crates/tui/src sweep for any assertion comparing against a .codewhale path before merging.
Fix options: have such tests explicitly clear/override CODEWHALE_HOME alongside HOME (matching the pattern the PR just added to ConfigPathEnvGuard), or scope the hermetic-home env var more narrowly than the whole job step.

Good choices

• ui.rs: reuses the existing EnvVarGuard/TestEnvLock primitives, no new abstraction — a correctly minimal, targeted fix for the actual leak in Test isolation leak: tui::ui::tests onboarding tests persist a fixture provider into the real ~/.codewhale/setup_state.json #5932 (onboarding writes setup_state.json/settings.toml under CODEWHALE_HOME, which was previously unguarded).
• The renamed home value (tmp.path().join(".codewhale")) matches production semantics: an explicit CODEWHALE_HOME is the home dir directly (no extra .codewhale join), so config_path = home.join("config.toml") stays correct.
• Copilot's field-order concern on ConfigPathEnvGuard (_lock declared last) is a false positive, already addressed in this PR's thread: Rust struct fields drop in declaration order (first-declared drops first), so _lock drops last, after the env guards restore their values — matching the safety contract, not violating it.

Provider/model/route safety: N/A — no provider-prefix inference logic touched.
Reuse/tests/security: No new abstractions; test-only + CI-only change; no secret handling.
· branch fix/test-home-isolation-5932

[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_75d7bce6-366e-4342-a0da-524cc402c84b)

[claude]

Claude finished @Hmbown's task in 3m 16s —— View job

---

Review: test(tui) isolate CODEWHALE_HOME (#5933)

• Gather context (current diff vs branch point 464ced2, confirm what's actually in HEAD)
• Review correctness / provider-model-route safety / reuse / tests / security
• Verify prior bot findings against the current diff state
• Post final review

Scope confirmed: current diff is exactly .github/workflows/ci.yml (+8/-2) and crates/tui/src/tui/ui.rs (+8/-1), via git diff 464ced2..HEAD. The earlier "systemic guard" (TEST_HOME_OVERRIDE/install_test_home_override in crates/paths/src/lib.rs) is confirmed gone from the tree (grep -r TEST_HOME_OVERRIDE crates → no matches) — this PR is now the narrow fix only.

Good news first: the Windows-shell finding from earlier passes (Devin, codewhale-agent) is fixed in 4b08a25 — the Run tests step now has shell: bash (.github/workflows/ci.yml:598), so mkdir -p "${CODEWHALE_HOME}" expands correctly on windows-latest too.

Findings

1. [Confirmed, blocking] Job-wide CODEWHALE_HOME breaks two existing runtime_log tests that assert the implicit home-fallback path (.github/workflows/ci.yml:605, crates/tui/src/runtime_log.rs:401-427 and :429-456)

log_directory() checks the raw CODEWHALE_HOME env var first and short-circuits (crates/tui/src/runtime_log.rs:211-219, codewhale_paths::codewhale_home_override()), by design — that's the "hard override" contract. The new CI step sets CODEWHALE_HOME for the entire Run tests step (ci.yml:605), which every cargo nextest-spawned test process inherits.

Two tests never guard CODEWHALE_HOME at all — they only redirect HOME/USERPROFILE via raw std::env::set_var, specifically to exercise the fallback path:

• log_directory_prefers_home (runtime_log.rs:400-427): asserts log_directory() == tmp.path().join(".codewhale").join("logs").
• log_directory_uses_existing_legacy_deepseek_logs (runtime_log.rs:429-456): asserts log_directory() == legacy (a .deepseek/logs dir under the same temp HOME).

With CODEWHALE_HOME present in the environment, both will instead resolve to ${{runner.temp}}/cw-test-home/.codewhale/logs, and both assertions will fail. This isn't hypothetical for this PR specifically: crates/tui/src/tui/ui.rs is a Rust source change, so needs.changes.outputs.heavy classifies true (ci.yml:136-149), and the Test job's matrix runs ubuntu-latest/macos-latest/windows-latest for pull_request events (ci.yml:519,662) — so this PR's own required Test job should hit this on all three OSes once nextest reaches these two tests. (Note the sibling test whitespace_home_override_is_consistent_across_tui_state_entry_points a few lines above at runtime_log.rs:363-390 gets this right — it explicitly seals CODEWHALE_HOME to " \t " before asserting the HOME-fallback path — these two didn't follow that pattern.)

Fix: either have these two tests explicitly clear CODEWHALE_HOME (EnvVarGuard::remove("CODEWHALE_HOME"), matching config_persistence.rs's EnvGuard) alongside their HOME/USERPROFILE redirection, or scope the CI hermetic-home env var so it isn't inherited by every spawned test process (e.g. export it only for the mkdir -p line, or set a default only where a test hasn't already sealed it).

Fix this →

2. [Info] Same shape may exist elsewhere, but a spot check of the largest HOME-touching files came back clean. I checked settings.rs, config/tests.rs, provider_picker.rs, and config_persistence.rs — every HOME-redirecting test in those files already pairs it with an explicit CODEWHALE_HOME set/remove. The two tests above in runtime_log.rs appear to be the outliers, but I did not exhaustively check all 31 files that touch HOME via EnvVarGuard.

Good choices

• ui.rs: minimal, reuses EnvVarGuard/TestEnvLock, no new abstraction — correctly targets the actual Test isolation leak: tui::ui::tests onboarding tests persist a fixture provider into the real ~/.codewhale/setup_state.json #5932 leak (onboarding writes setup_state.json/settings.toml against CODEWHALE_HOME, previously unguarded).
• home = tmp.path().join(".codewhale") / config_path = home.join("config.toml") matches production semantics (an explicit CODEWHALE_HOME is the home dir, no extra .codewhale join) — no regression to the existing config-path assertions.
• Copilot's field-order concern on ConfigPathEnvGuard (_lock declared last) is a false positive: Rust struct fields drop in declaration order (first-declared drops first), so _lock — declared last — drops last, after the env guards restore their values, matching EnvVarGuard's safety contract.
• The Windows shell: bash fix from the prior review round is in and correct.

Provider/model/route safety: N/A — no provider-prefix inference logic touched.
Security: No secret handling changes; test/CI-only.

Note: I could not execute cargo test/cargo nextest in this sandbox (Bash tool calls require interactive approval not available here), so finding #1 is based on static code-path analysis of log_directory()'s override-precedence and the two tests' bodies, not an observed local failure — the reasoning chain is laid out above so it can be checked without rerunning it.
· branch fix/test-home-isolation-5932

[codewhale-agent]

Codewhale review

The PR makes provider onboarding tests hermetic by setting CODEWHALE_HOME alongside CODEWHALE_CONFIG_PATH, and makes the main CI cargo nextest step run with an isolated CODEWHALE_HOME under runner.temp.

Assessment

Low-risk test and CI isolation improvement. The guard directs both home and config path into the same temporary .codewhale tree, and the CI step creates the isolated home before running tests. No correctness issues found.

---

Advisory review by Codewhale (codewhale review --pr 5933 --post, head 4b08a2534470c3a3c67e58b2ca1d9dc3e719a351). 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.

[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_0fad2c48-0c0b-4e6d-8e70-ddd555688e63)

[claude]

Claude finished @Hmbown's task in 1m 37s —— View job

---

Review: test(tui) isolate CODEWHALE_HOME in provider onboarding tests (#5933)

• Gather context (diff, related files, prior bot findings)
• Confirm current diff scope vs earlier review rounds
• Review correctness / provider-model-route safety / reuse / tests / security
• Post final review

Scope check: the current diff is exactly crates/tui/src/tui/ui.rs (+7/-1), confirmed via git show c7fc000. The earlier, larger "systemic guard" revision (TEST_HOME_OVERRIDE/install_test_home_override in crates/paths/src/lib.rs, plus the .github/workflows/ci.yml hermetic-home step) that drew most of the findings in this thread (race condition, Windows pwsh shell, log_directory_prefers_home breakage, dead-code getter) is no longer in the tree — grep -rn "TEST_HOME_OVERRIDE|install_test_home_override" crates/ returns nothing, and crates/paths/src/lib.rs:110 is back to its original one-line codewhale_home(). This PR is now the narrow, targeted fix only, matching the PR body's description. Those prior findings are moot against the current HEAD.

Correctness

• Confirmed SetupState::path() (crates/config/src/setup_state.rs:626) resolves via crate::codewhale_home() → codewhale_paths::codewhale_home(), which returns the raw CODEWHALE_HOME value when set (crates/paths/src/lib.rs:110-111). Onboarding completion writes setup_state.json through this path, so it was genuinely unguarded before this fix — the bug description in Test isolation leak: tui::ui::tests onboarding tests persist a fixture provider into the real ~/.codewhale/setup_state.json #5932 is accurate and this closes it.
• home = tmp.path().join(".codewhale") and config_path = home.join("config.toml") correctly matches production semantics: an explicit CODEWHALE_HOME is the home dir directly (no extra .codewhale join), so the existing config-path assertions are unaffected.
• EnvVarGuard::set (crates/tui/src/test_support.rs:193) asserts the calling thread holds lock_test_env() via debug_assert!, which ConfigPathEnvGuard::new already acquires first — safe.
• Field-drop order: _lock is declared last in the struct, and Rust drops struct fields in declaration order (first-declared drops first) — the opposite of local-binding LIFO order. So _lock drops last, after _codewhale_home/_codewhale_config_path/_deepseek_config_path have already restored their env vars via EnvVarGuard::drop (test_support.rs:222-235), matching that Drop impl's stated safety requirement ("callers hold the process-wide test env mutex until after this guard is dropped"). Copilot's inline comment flagging this as backwards is incorrect; every other TestEnvLock-holding fixture in this crate uses the same last-position convention.

Provider/model/route safety: N/A — no provider-prefix inference logic touched.

Reuse/simplification: Reuses the existing EnvVarGuard/TestEnvLock primitives, no new abstraction. Good instinct keeping this PR narrowed to the actual leak rather than re-attempting the systemic guard here (per the PR body, that stays open on #5932).

Tests: This is the test-isolation fix itself. The PR body's evidence (15 passed / 0 failed, real ~/.codewhale/setup_state.json mtime unchanged across the run) is the right kind of proof for this class of bug — it demonstrates isolation held, not just that the suite passed.

Security: No secret handling; test-only change.

No blocking issues on the current diff. Nice, well-scoped fix.
· branch fix/test-home-isolation-5932

[codewhale-agent]

Codewhale review

Extends ConfigPathEnvGuard to set CODEWHALE_HOME to the isolated temp directory used for config paths, so provider onboarding tests no longer write setup_state.json into the developer's real ~/.codewhale.

Findings

• [INFO] Relies on manual mtime verification instead of an automated assertion (crates/tui/src/tui/ui.rs:1280)
  The fix is verified manually by checking that ~/.codewhale/setup_state.json is unchanged during the test run. Without an automated guard/assertion, a future regression could reintroduce writes to the real home without failing any test.
• [INFO] TempDir field is dropped before environment guards (crates/tui/src/tui/ui.rs:1276)
  Rust drops struct fields in declaration order, so _tmp is removed before _codewhale_home and the other EnvVarGuards restore their variables. This is not currently harmful because the guards do not touch the filesystem, but declaring _tmp after the env guards would be more robust.

Suggestions

• crates/tui/src/tui/ui.rs:1276 — Reorder the fields in ConfigPathEnvGuard so _tmp is declared after the EnvVarGuard fields. That keeps the isolated temp directory alive until the environment variables are restored when the guard drops.

Assessment

The test-only isolation change is correct and low risk. It should resolve the #5932 contamination. The remaining notes are hardening suggestions rather than blocking issues.

---

Advisory review by Codewhale (codewhale review --pr 5933 --post, head c7fc0007b378543e83f8ec235307b7b76c2768bd). 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] Relies on manual mtime verification instead of an automated assertion

The fix is verified manually by checking that ~/.codewhale/setup_state.json is unchanged during the test run. Without an automated guard/assertion, a future regression could reintroduce writes to the real home without failing any test.

[codewhale-agent]

[INFO] TempDir field is dropped before environment guards

Rust drops struct fields in declaration order, so _tmp is removed before _codewhale_home and the other EnvVarGuards restore their variables. This is not currently harmful because the guards do not touch the filesystem, but declaring _tmp after the env guards would be more robust.

[codewhale-agent]

Reorder the fields in ConfigPathEnvGuard so _tmp is declared after the EnvVarGuard fields. That keeps the isolated temp directory alive until the environment variables are restored when the guard drops.