Skip to content

fix: replace dirs::home_dir() with effective_home_dir() in crates/tui - #4760

Closed
EvanProgramming wants to merge 3 commits into
Hmbown:mainfrom
EvanProgramming:fix/issue-4757-dirs-home-dir-audit
Closed

fix: replace dirs::home_dir() with effective_home_dir() in crates/tui#4760
EvanProgramming wants to merge 3 commits into
Hmbown:mainfrom
EvanProgramming:fix/issue-4757-dirs-home-dir-audit

Conversation

@EvanProgramming

@EvanProgramming EvanProgramming commented Jul 24, 2026

Copy link
Copy Markdown

Summary

Audit and replace all remaining raw dirs::home_dir() call sites in crates/tui/src with crate::config::effective_home_dir().

Context: Fixed two Windows-only CI test failures in bc461c60c caused by raw dirs::home_dir() calls in tools/workflow.rs and tui/views/skills_manager.rs. Root cause: dirs::home_dir() on Windows resolves via a native OS API call and does not respect HOME/USERPROFILE env var overrides, so tests that fake $HOME via the established EnvVarGuard pattern silently disagreed with production code that used the real Windows user profile instead. crate::config::effective_home_dir() already exists for exactly this (checks HOME, then USERPROFILE, then HOMEDRIVE+HOMEPATH) and is used correctly elsewhere in the crate.

This PR sweeps all remaining call sites (45 replacements across 29 files) so the crate consistently uses effective_home_dir(). The fallback inside effective_home_dir() itself (config/paths.rs:55) is intentionally left as dirs::home_dir().

Note: network_policy.rs and skills/install.rs are #[path]-included from integration tests (tests/skill_cli.rs), so crate::config:: does not exist in the test binary crate context. These two files keep dirs::home_dir() with explanatory comments — they are not test-fakability-critical.

Testing

  • cargo fmt --all -- --check
  • cargo clippy --workspace --all-targets --all-features --locked — warning-free
  • cargo test --workspace --all-features --locked — 8155 passed, 0 failed (only 1 unrelated PTY flake)

Checklist

  • Updated docs or comments as needed
  • Added or updated tests where relevant (N/A — pure refactor)
  • Verified TUI behavior manually if UI changes (N/A)
  • Harvested/co-authored credit uses a GitHub numeric noreply address (N/A)

Closes #4757

Audit and replace all remaining raw dirs::home_dir() call sites in
crates/tui/src with crate::config::effective_home_dir(). This ensures
the crate consistently respects HOME/USERPROFILE env var overrides,
making tests that fake $HOME portable across Windows CI runners.

Previously fixed in bc461c6 for tools/workflow.rs and
tui/views/skills_manager.rs. This commit sweeps the remaining ~70
call sites across 29 files.

The fallback inside effective_home_dir() itself
(config/paths.rs:55) is intentionally left as dirs::home_dir()
since that is its last-resort path.

Closes Hmbown#4757
@EvanProgramming
EvanProgramming requested a review from Hmbown as a code owner July 24, 2026 10:15
@github-actions

Copy link
Copy Markdown
Contributor

Thanks @EvanProgramming for taking the time to contribute.

This repository is observing a maintainer-managed PR intake gate in dry-run mode, so this pull request is staying open. This note helps maintainers prepare the allowlist before any enforcement is considered.

Please read CONTRIBUTING.md for the expected contribution shape. A maintainer can grant recurring PR access by commenting /lgtm on a pull request.

…ibility

network_policy.rs and skills/install.rs are #[path]-included from
tests/skill_cli.rs, so crate::config::effective_home_dir() cannot
resolve (the 'config' module does not exist in the test binary crate).

These call sites are safe: the integration tests supply an explicit
temp directory for the install target, and the audit-log default path
is not test-fakability-critical.
@Hmbown

Hmbown commented Jul 26, 2026

Copy link
Copy Markdown
Owner

Thank you for this, @EvanProgramming — the report and the sweep were right, and #4757 is closed.

Closing as already landed: 0d7312736 landed the same change on main. rg -n 'dirs::home_dir' crates/tui/src/ now returns only 3 hits, all inside crates/tui/src/config/home.rs itself (two doc comments and the OS fallback in the definition), against 78 effective_home_dir call sites.

Credit note: that commit went in without a Co-authored-by trailer for you, which was our miss — your PR predated it by ~11 hours. I'm adding you to .github/AUTHOR_MAP so future harvests in this area attribute correctly.

One thing worth calling out because you got it right and flagged it honestly: you deliberately left network_policy.rs alone with a note that crate::config wasn't reachable from the #[path]-included skill_cli test binary. That was a real constraint, not an oversight. It was later solved by splitting effective_home_dir into a dependency-free leaf (189c93965), so network_policy.rs:324 now uses it too. Your note is what made that the obvious next step.

@Hmbown Hmbown closed this Jul 26, 2026
Hmbown added a commit that referenced this pull request Jul 26, 2026
Each of these people had a PR triaged during the v0.9.2 pass, and three of
them had work reach main without a Co-authored-by trailer because they were
not in this file. Adding them so future harvests attribute correctly and the
check-coauthor-trailers gate can resolve them.

- EvanProgramming — #4760; equivalent change landed as 0d73127 with no
  trailer.
- adity982 — #4756; the MCP call-once fix landed as cccaa4f with no trailer.
- vibecoding-skills — #4743; the SSE-timeout split landed as a374d19.
- XhesicaFrost — #4610, pending harvest for #4520.
- ffaacceelee — #4686, closed without harvest, mapped pre-emptively.

Numeric noreply ids verified against the GitHub users API rather than
inferred, since a wrong id silently drops credit out of the contributor graph
instead of failing loudly.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Audit remaining raw dirs::home_dir() call sites for the same Windows test-fakeability bug

2 participants