Prevent Windows test PATH leaks into User PATH

[tonythethompson]

Summary

Yes: those C:\Users\...\Temp\.tmp*\off and ...\existing-nu entries are Numan leftovers, but from Windows acceptance tests (cargo test -- --ignored covering numan setup nu use), not from a normal managed numan setup nu install.

setup nu use calls persist_path_dir, which writes the Windows User PATH via PowerShell. Tests previously left tempfile fixture dirs permanently when that path ran without a durable-write block. On Unix, persist_user_path could still create ~/.local/bin/nu and update a shell profile while PathRestoreGuard was held.

Fix

• PathRestoreGuard sets NUMAN_TEST_NO_PERSIST_USER_PATH while held and restores any pre-existing value of that flag on drop (or removes it only when it was originally absent)
• Does not rewrite the Windows User PATH registry on drop (avoids erasing concurrent external PATH changes; Greptile review)
• persist_path_dir and Unix persist_user_path honor the test flag and refuse paths under the system temp folder (fail closed when temp canonicalize fails)

Cleanup

PowerShell cleanup is documented in CHANGELOG.md under Unreleased.

Test plan

• cargo test --lib util::test_paths
• cargo test --lib nu::bootstrap
• cargo clippy -- -D warnings
• cargo fmt --all --check

  

[coderabbitai]

Warning

Review limit reached

@tonythethompson, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 47 minutes

You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository.

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews.

How do review limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please refer docs for additional details.

Review details

⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: 280f44af-97bc-4b06-87df-a5ce399783fb

📥 Commits

Reviewing files that changed from the base of the PR and between e029de7 and 55163ce.

📒 Files selected for processing (3)

• CHANGELOG.md
• src/nu/bootstrap.rs
• src/util/test_paths.rs

📝 Walkthrough

Walkthrough

The change isolates persistent PATH updates during tests, rejects system-temporary directories from PATH persistence on Windows and Unix, and documents cleanup guidance for existing PATH entries.

Changes

PATH persistence isolation

Layer / File(s)	Summary
Persistent PATH guard src/util/test_paths.rs	PathRestoreGuard sets a test-only flag, restores the process PATH, and removes the flag when dropped. Documentation and examples describe this behavior.
Temporary-path persistence rejection src/nu/bootstrap.rs, CHANGELOG.md	Windows and Unix PATH persistence skip writes when the test flag is set and reject directories under the system temporary directory. A test covers nested temporary paths. The changelog documents the fix and cleanup guidance.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Possibly related PRs

• tonythethompson/numan#66: Modifies the same setup nu PATH persistence behavior.
• tonythethompson/numan#69: Extends related PathRestoreGuard and PATH persistence logic.
• tonythethompson/numan#83: Modifies related PATH persistence behavior and tests.

Suggested reviewers: greptile-apps

🚥 Pre-merge checks | ✅ 8

✅ Passed checks (8 passed)

Check name	Status	Explanation
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Pipeline Stage Enum Ordering	✅ Passed	PASS: The PR changes only CHANGELOG.md, src/nu/bootstrap.rs, and src/util/test_paths.rs; no SessionWorkflowStage enum, member reference, or stage comparison exists in the patch or tracked solution.
Gpu/Cpu Runtime Boundary	✅ Passed	The PR changes only src/util/test_paths.rs; no inference/ files or managed CPU/GPU requirements files are modified, so the runtime-boundary checks do not apply.
Managed Host Restart Safety	✅ Passed	The PR modifies only CHANGELOG.md, src/nu/bootstrap.rs, and src/util/test_paths.rs; none of the four managed-host entities or related restart/readiness identifiers exist in the repository.
Description check	✅ Passed	The description clearly explains the Windows PATH leak fix, test safeguards, cleanup guidance, and validation steps.
Title check	✅ Passed	The title clearly and concisely identifies the main change: preventing Windows test PATH leaks into the User PATH.

✨ Finishing Touches 💡 1

🛠️ Fix failing CI checks 💡

• Create stacked PR
• Commit on current branch

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch cursor/fix-windows-path-test-leak-7e44

✨ Simplify code

• Create PR with simplified code
• Commit simplified code in branch cursor/fix-windows-path-test-leak-7e44

Warning

Review ran into problems

🔥 Problems

Linked repositories: Public OSS repositories can only analyze public repositories installed in this organization. Analyzed tonythethompson/QuickShell, tonythethompson/numan, tonythethompson/dependency-chain-substrate, skipped Trackdubllc/Trackdub.

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[coderabbitai]

Actionable comments posted: 3

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@src/nu/bootstrap.rs`:
- Around line 603-610: Update path_is_under_temp_dir so failure to canonicalize
the system temporary directory does not return false; apply the same
lexical-prefix fallback used in the dir.canonicalize failure branch, or
otherwise fail closed. Ensure persist_path_dir still refuses paths under the
temporary root, and add a regression test covering an uncanonicalizable
temporary root.

In `@src/util/test_paths.rs`:
- Around line 114-130: Update read_windows_user_path to distinguish command
failure, an absent User PATH, and an explicitly empty value by using an
unambiguous PowerShell output marker or snapshot representation. Update the
restoration logic in Drop to pass $null to SetEnvironmentVariable for an absent
original value, while preserving empty-string restoration for an explicitly
empty value. Add Windows tests covering both absent and empty User PATH
snapshots.
- Around line 56-68: Update PathRestoreGuard::new to capture the pre-existing
NUMAN_TEST_NO_PERSIST_USER_PATH value before setting it, then update its Drop
implementation to restore that value or remove the variable only when it was
originally absent. Preserve the current test-only flag behavior while the guard
is active.

🪄 Autofix

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: 77e92cbf-1caa-4338-8801-a21c06537b14

📥 Commits

Reviewing files that changed from the base of the PR and between 894c508 and 3ed1106.

📒 Files selected for processing (3)

• CHANGELOG.md
• src/nu/bootstrap.rs
• src/util/test_paths.rs

🔗 Linked repositories identified

CodeRabbit considers these linked repositories for cross-repo context during reviews:

• tonythethompson/QuickShell (manual)
• tonythethompson/numan (manual)
• tonythethompson/dependency-chain-substrate (manual)

📜 Review details

⏰ Context from checks skipped due to timeout. (3)

• GitHub Check: Test (windows-latest)
• GitHub Check: Real-Nu acceptance (windows-latest)
• GitHub Check: Analyze (rust)

🧰 Additional context used

📓 Path-based instructions (11)

**/*

📄 CodeRabbit inference engine (CLAUDE.md)

**/*: Use Serena's semantic, symbol-aware tools as the primary tools for reading and editing code; use built-in Read, Glob, Grep, and Edit only under the stated exceptions.
Before editing a code file, inspect its symbol overview, read the specific symbols being changed, and edit them with Serena's symbol-aware tools.
Understand the existing code before changing it and make the smallest change that satisfies the request; avoid unrelated cleanup, premature abstractions, unnecessary error handling, feature flags, and compatibility shims.
Prefer editing existing files over creating new ones, and never create Markdown or README files unless explicitly requested.
For exploratory questions, provide a 2–3 sentence recommendation with the main tradeoff and do not implement until the user agrees.
For UI or frontend changes that cannot be tested in a browser, explicitly state that browser testing was not performed rather than claiming success.
Address security issues when discovered, including injection, XSS, SQL injection, path traversal, and secret leaks.
Pause and obtain confirmation before destructive, hard-to-reverse, externally visible, or third-party-upload actions, including deleting files or branches, dropping tables, force-pushing, modifying CI/CD, posting externally, or uploading content.
When blocked, investigate the root cause instead of bypassing it with --no-verify, --force, or deletion; investigate unfamiliar files, branches, and configuration before deleting them.
Only commit when explicitly asked; do not proactively update git configuration or push changes.
Do not skip Git hooks unless explicitly asked; if a pre-commit hook fails, fix the issue, re-stage, and create a new commit rather than amending.
Stage files by name rather than using git add -A or git add ., to avoid accidentally including secrets or large binaries.
Use a HEREDOC for commit messages, and add co-author attribution only when the user explicitly requests the exact trailer.
Do not force-pus...

Files:

• CHANGELOG.md
• src/util/test_paths.rs
• src/nu/bootstrap.rs

!**/.env,!**/credentials.json,!**/*.pem

📄 CodeRabbit inference engine (CLAUDE.md)

Do not commit files that appear to contain secrets, including .env, credentials.json, and PEM files; warn before doing so even if explicitly requested.

Files:

• CHANGELOG.md
• src/util/test_paths.rs
• src/nu/bootstrap.rs

**/*.{rs,md}

📄 CodeRabbit inference engine (CONTRIBUTING.md)

Match existing naming, module layout, and documentation level in the file being edited; update AGENTS.md, docs/, or command help when structure, conventions, or user-visible behavior changes.

Tests must cover failure modes, not only successful execution.

Files:

• CHANGELOG.md
• src/util/test_paths.rs
• src/nu/bootstrap.rs

**/*.{rs,md,toml}

📄 CodeRabbit inference engine (CONTRIBUTING.md)

Use the repository's established serialization and module conventions rather than introducing unrelated refactors.

Files:

• CHANGELOG.md
• src/util/test_paths.rs
• src/nu/bootstrap.rs

**/*.md

📄 CodeRabbit inference engine (REVIEW.md)

Update documentation and AGENTS.md when project structure or conventions change.

Files:

• CHANGELOG.md

**/*.{js,jsx,ts,tsx,py,java,go,rs,rb,php,c,cpp,h,hpp,cs,swift,kt,kts}

📄 CodeRabbit inference engine (CLAUDE.md)

Add comments only when the WHY is non-obvious; do not narrate what the code does, reference the current task, or reference the PR in comments.

Files:

• src/util/test_paths.rs
• src/nu/bootstrap.rs

**/*.rs

📄 CodeRabbit inference engine (CONTRIBUTING.md)

**/*.rs: Use the Rust 2021 edition.
Use anyhow::Result with .context(...) in application code; use thiserror for library error types that callers match on.
Use clap derive macros for CLI definitions.
Use serde with serde_json or toml for serialization.
Function parameters must use &Path, not &PathBuf.
Library code must not panic; error paths should return anyhow::Result with context where appropriate.
Add or update tests for behavior changes, including relevant failure paths.
New mutating code paths must acquire the mutation lock via acquire_mutation_lock(root) and snapshot the lockfile before writes.
Lockfile, journal, and state-file JSON writes must use write_json_atomic.
numan install must write only to $NUMAN_ROOT; it must not invoke Nu or register plugins/autoloads.
Only activate and deactivate may modify Nu integration state.
Treat the lockfile as the authoritative source of truth; derived projections such as autoload state must not be authoritative.
Install payloads under versioned, content-addressed paths and never overwrite them in place.
Never overwrite foreign autoload files; respect OWNERSHIP_MARKER.
Pass plugin paths through environment variables only; do not use runtime interpolation in Nu program strings.

**/*.rs: All CI gates must pass: cargo test, cargo clippy -- -D warnings, and cargo fmt --check.
Every mutating command—including install, remove, update, gc, and future nupm import—must call acquire_mutation_lock(root).
Lockfiles, journals, and state files must use write_json_atomic; partial writes are not allowed.
Pending activation, autoload, and lifecycle journals must be stored under $NUMAN_ROOT/state/.
Module autoload identity must match all four fields: Nu executable hash, Nu version, vendor autoload directory, and managed file path; the lockfile module_activation value is authoritative.
Never overwrite foreign autoload files; respect OWNERSHIP_MARKER.
Pass paths to Nu only throu...

Files:

• src/util/test_paths.rs
• src/nu/bootstrap.rs

**/*.{rs,toml}

📄 CodeRabbit inference engine (CONTRIBUTING.md)

Run and keep cargo fmt/rustfmt clean, and ensure cargo clippy -- -D warnings passes.

Files:

• src/util/test_paths.rs
• src/nu/bootstrap.rs

**/*.{rs,nu}

📄 CodeRabbit inference engine (CONTRIBUTING.md)

**/*.{rs,nu}: Real-Nu acceptance tests must be marked #[ignore] and should be run when changes affect activation or nupm import; unit tests must not spawn real nu and should use injectable seams such as FakeCandidateRunner or registrars.
The nupm integration must be read-only toward NUPM_HOME, must not execute build.nu, and must not perform bidirectional synchronization.

Unit tests must use FakeCandidateRunner or injectable registrars and must not spawn a real nu process.

Files:

• src/util/test_paths.rs
• src/nu/bootstrap.rs

src/**/*.rs

📄 CodeRabbit inference engine (AGENTS.md)

Use anyhow::Result for application code, thiserror for library error types, add context with .context(...) or ?, and never panic in library code.

Files:

• src/util/test_paths.rs
• src/nu/bootstrap.rs

src/nu/**/*.rs

📄 CodeRabbit inference engine (AGENTS.md)

Managed Nushell files must contain OWNERSHIP_MARKER; use assert_managed_file_owned to prevent overwriting foreign files.

Files:

• src/nu/bootstrap.rs

🔍 Remote MCP GitHub Copilot

Additional review context

• The PR is #103, an open draft with one commit, three changed files, and +153/-1. No review threads were returned.
• persist_user_path (Windows) and register_existing_nu both ultimately call persist_path_dir, so the new guard covers installed and off-path registration flows.
• Ignored real-Nu acceptance tests use PathRestoreGuard; the documented command is cargo test -- --ignored.
• PathRestoreGuard sets NUMAN_TEST_NO_PERSIST_USER_PATH, snapshots User PATH through PowerShell, and restores it on drop. However:
  • It unconditionally removes the environment variable instead of restoring any pre-existing value.
  • If the User PATH snapshot fails, restoration is skipped after only a warning.
  • path_is_under_temp_dir fails open when the system temp directory itself cannot be canonicalized.
• CI results currently show Linux/macOS tests, formatting, Clippy, packaging, MSRV, and CodeFactor passed; Windows test and Windows Real-Nu acceptance jobs were still in progress, while CodeQL was neutral.

🔇 Additional comments (2)

CHANGELOG.md (1)

12-18: LGTM!

src/nu/bootstrap.rs (1)

1138-1154: 🩺 Stability & Availability

Wait for all required Rust gates before merge.

Merge block is still in place while Windows test and Windows Real-Nu acceptance jobs are not showing completed success. Ship only after cargo test, cargo clippy -- -D warnings, and cargo fmt --check pass for this PR.

[sourcery-ai]

Sorry @tonythethompson, you have reached your weekly rate limit of 500000 diff characters.

Please try again later or upgrade to continue using Sourcery

[chatgpt-codex-connector]

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.

[qodo-code-review]

PR Summary by Qodo

Prevent Windows acceptance tests from persisting temp dirs into User PATH

🐞 Bug fix 🧪 Tests 📝 Documentation 🕐 40+ Minutes

AI Description

• Prevent ignored acceptance tests from writing to durable User PATH while PATH guard is held.
• Refuse persisting directories under the system temp folder to avoid fixture leaks.
• Add regression test and document cleanup guidance in the changelog.

Diagram

graph TD
  T["Ignored acceptance tests"] --> G["PathRestoreGuard"] --> F["NUMAN_TEST_NO_PERSIST_USER_PATH"] -.-> P["persist_path_dir"] --> D{"Temp dir?"}
  D -->|"yes"| X["Refuse / bail"]
  D -->|"no"| U[("User PATH store")]

  subgraph Legend
    direction LR
    _t[/"Test"/] ~~~ _u["Utility"] ~~~ _d{"Decision"} ~~~ _db[("Durable store")]
  end

Loading

High-Level Assessment

The following are alternative approaches to this PR:

1. Inject a persistence backend (trait) and use a no-op backend in tests

• ➕ Avoids global env flags and reduces reliance on process-global state
• ➕ Enables deterministic unit tests without PowerShell invocation
• ➖ Larger refactor: thread backend through more call sites
• ➖ More boilerplate/indirection for a narrow problem

2. Use Windows registry APIs (e.g., winreg) instead of PowerShell for snapshot/restore

• ➕ Removes dependency on external PowerShell process
• ➕ Potentially faster and more robust error handling
• ➖ Adds a new dependency and requires careful handling of expand-string semantics
• ➖ More Windows-specific code surface to maintain

3. Change acceptance tests to never call durable persistence (explicit test-only option)

• ➕ Keeps production code simpler (no temp-dir refusal logic needed for tests)
• ➕ Avoids touching durable PATH mechanisms altogether during tests
• ➖ Risk of tests diverging from production behavior they intend to validate
• ➖ Still needs guardrails to prevent accidental future persistence paths

Recommendation: Current approach is a good balance for a targeted safety fix: blocking persistence via a guard-held flag prevents test pollution, and refusing temp-rooted entries adds production hardening against accidental persistence of unstable paths. Consider switching to direct registry APIs later if PowerShell fragility becomes an issue, but it’s not required to address the leak.

Files changed (3) +153 / -1

Bug fix (2) +146 / -1

bootstrap.rsBlock durable PATH writes during tests and refuse temp-rooted PATH persistence +54/-0

Block durable PATH writes during tests and refuse temp-rooted PATH persistence

• Adds a test-harness escape hatch (NUMAN_TEST_NO_PERSIST_USER_PATH) to short-circuit durable PATH persistence. Introduces a temp-directory guard (path_is_under_temp_dir) and uses it to reject attempts to persist system temp directories on both Windows and Unix, plus a regression test asserting refusal for temp paths.

src/nu/bootstrap.rs

test_paths.rsExtend PathRestoreGuard to snapshot/restore Windows User PATH and block persistence +92/-1

Extend PathRestoreGuard to snapshot/restore Windows User PATH and block persistence

• Enhances PathRestoreGuard to set NUMAN_TEST_NO_PERSIST_USER_PATH while held and to restore it on drop. On Windows, additionally snapshots the User PATH via PowerShell at construction and restores it in Drop, emitting warnings/errors without panicking if snapshot/restore fails.

src/util/test_paths.rs

Documentation (1) +7 / -0

CHANGELOG.mdDocument fix for Windows User PATH pollution from acceptance tests +7/-0

Document fix for Windows User PATH pollution from acceptance tests

• Adds a changelog entry describing the PATH leak source (ignored acceptance tests) and the new protections: blocking durable PATH writes during tests and refusing temp-rooted PATH entries. Includes user guidance for cleaning existing leaked entries.

CHANGELOG.md

[greptile-apps]

Greptile Summary

The PR prevents acceptance tests from making durable User PATH or shell-profile changes while PathRestoreGuard is held and rejects persistence of temporary paths.

• Adds and restores a test-only persistence-suppression environment flag.
• Extends suppression to Unix symlink and shell-profile persistence.
• Rejects paths beneath the system temporary directory.
• Avoids restoring a stale Windows User PATH registry snapshot.
• Documents cleanup of previously leaked PATH entries.

Confidence Score: 5/5

The PR appears safe to merge.

No blocking failure remains; the prior stale Windows PATH restoration, Unix persistence bypass, and non-ASCII snapshot corruption paths have been removed or guarded.

Important Files Changed

Filename	Overview
src/util/test_paths.rs	Replaces Windows User PATH snapshot restoration with a scoped persistence-suppression flag while preserving process PATH restoration and prior flag values.
src/nu/bootstrap.rs	Honors the test suppression flag across persistence paths and rejects temporary-directory binaries before durable PATH, profile, or symlink writes.
CHANGELOG.md	Documents the acceptance-test PATH leak fix and provides PowerShell cleanup instructions for existing entries.

_{Reviews (7): Last reviewed commit: "Merge branch 'master' into cursor/fix-wi..." | Re-trigger Greptile}

[qodo-code-review]

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (0) 📜 Skill insights (0)

1. User PATH overwritten in tests ✓ Resolved 🐞 Bug ☼ Reliability

Description

On Windows, PathRestoreGuard::drop always writes the snapshotted User PATH registry value back via
PowerShell, which can overwrite legitimate User PATH updates made by other processes during the test
run. Since persist_path_dir already short-circuits when NUMAN_TEST_NO_PERSIST_USER_PATH is set,
this unconditional restore is an unnecessary and risky durable mutation.

Code

src/util/test_paths.rs[R93-96]

+            if let Some(user_path) = self.original_user_path.as_ref() {
+                if let Err(err) = write_windows_user_path(user_path) {
+                    // Never panic in Drop; surface the leak clearly on stderr.
+                    eprintln!(

Relevance

●● Moderate

Change reduces risky durable writes in Drop, but may conflict with current “always restore”
leak-prevention strategy.

PR-#35
PR-#83

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The drop handler performs a durable write to the Windows User PATH whenever a snapshot exists,
regardless of whether this process actually changed it; and persistence through persist_path_dir
is already blocked via the env flag while the guard is held, making the restore both risky and
largely redundant for the stated leak scenario.

src/util/test_paths.rs[84-104]
src/nu/bootstrap.rs[570-586]
src/nu/bootstrap.rs[630-641]
REVIEW.md[13-20]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`PathRestoreGuard` now restores the Windows **User** PATH registry value unconditionally on `Drop` (when snapshot succeeded). This can overwrite unrelated concurrent User PATH changes (e.g., installers or manual edits during a long test run), causing durable state loss.

### Issue Context
- The guard already sets `NUMAN_TEST_NO_PERSIST_USER_PATH`, and both Windows/Unix persistence functions return early when this flag is present.
- Therefore, in the intended usage (tests holding the guard), the guard itself should not need to rewrite the User PATH at all unless a leak actually occurred.

### Fix Focus Areas
- src/util/test_paths.rs[84-104]
- src/nu/bootstrap.rs[570-600]

### What to change
- Re-read the current Windows User PATH on drop and **skip writing** if it differs from the snapshot (treat differences as external changes and do not overwrite them).
 - Optionally: only write back when the current User PATH contains known test-leak patterns (e.g., temp fixture suffixes) and you can safely remove them.
- Alternatively (simplest/safer): remove the registry restore entirely and rely on the env-flag blocking to prevent persistent writes.
- If you keep restoration, also avoid depending on PATH lookup for `powershell` (use a stable resolution method) to reduce restore failures.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

2. Env flag clobbered on drop ✓ Resolved 🐞 Bug ≡ Correctness

Description

PathRestoreGuard sets NUMAN_TEST_NO_PERSIST_USER_PATH but on drop unconditionally removes it
instead of restoring any pre-existing value, mutating unrelated process-global state beyond PATH.
This can cause later code in the same process to unexpectedly start persisting durable PATH updates.

Code

src/util/test_paths.rs[90]

+        std::env::remove_var("NUMAN_TEST_NO_PERSIST_USER_PATH");

Relevance

●●● Strong

Deterministic guard bug: should restore prior env value to avoid clobbering process-global state.

PR-#84
PR-#71

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The guard sets the env var on construction but does not snapshot the prior value and always removes
it on drop, which can clobber any caller-established setting and change subsequent persistence
behavior in-process.

src/util/test_paths.rs[51-92]
src/nu/bootstrap.rs[449-464]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`PathRestoreGuard` sets `NUMAN_TEST_NO_PERSIST_USER_PATH` to "1" but always removes it on drop, losing any prior value/state. This is a global env var used to control durable PATH persistence.

### Issue Context
- Both Windows and Unix `persist_path_dir_*` paths check only for presence of `NUMAN_TEST_NO_PERSIST_USER_PATH`.
- The guard should behave like other RAII restorers: restore the previous env var value (or absence), not force-remove.

### Fix Focus Areas
- src/util/test_paths.rs[44-105]

### What to change
- Add a field on `PathRestoreGuard` like `original_no_persist_flag: Option<OsString>`.
- In `new()`: capture `std::env::var_os("NUMAN_TEST_NO_PERSIST_USER_PATH")` before setting it.
- In `Drop`: restore the captured state:
 - `Some(v)` => `set_var(..., v)`
 - `None` => `remove_var(...)`
- Keep the existing PATH restore semantics unchanged.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

3. Temp-dir check fails open ✓ Resolved 🐞 Bug ☼ Reliability

Description

path_is_under_temp_dir returns false when std::env::temp_dir().canonicalize() fails, disabling
the new safety barrier and potentially allowing temporary directories onto the durable PATH in those
environments. This contradicts the surrounding intent to refuse temp-root persistence.

Code

src/nu/bootstrap.rs[R603-606]

+fn path_is_under_temp_dir(dir: &Path) -> bool {
+    let Ok(temp) = std::env::temp_dir().canonicalize() else {
+        return false;
+    };

Relevance

●●● Strong

Team often accepts safety hardening; fail-open contradicts stated “refuse temp dirs” intent.

PR-#83
PR-#84

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The helper immediately returns false if canonicalizing the temp root fails, but it is used as a
safety gate to refuse persisting temp directories in both Windows and Unix persistence functions.

src/nu/bootstrap.rs[570-586]
src/nu/bootstrap.rs[603-613]
src/nu/bootstrap.rs[630-641]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
The new `path_is_under_temp_dir` helper fails open when it cannot canonicalize the system temp dir (`return false`). That means the caller will proceed with durable PATH persistence even though the safety check couldn't be evaluated.

### Issue Context
- Callers use this helper to decide whether to `bail!("Refusing to add temporary directory ...")`.
- Returning `false` on inability to canonicalize temp_dir defeats the protection.

### Fix Focus Areas
- src/nu/bootstrap.rs[570-613]
- src/nu/bootstrap.rs[630-650]

### What to change
- Prefer fail-closed behavior:
 - Either return `anyhow::Result<bool>` and `bail!/context` when temp_dir canonicalization fails, OR
 - Fall back to a raw-prefix check against `std::env::temp_dir()` (similar to the existing fallback used when `dir.canonicalize()` fails) instead of immediately returning `false`.
- Ensure behavior is consistent across Windows and Unix callers.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

Context used

✅ Compliance rules (platform): 26 rules

✅ REVIEW.md

Review mode: ⚖️ Balanced: This changes cross-platform test isolation and durable Windows User PATH persistence across multiple code paths, creating meaningful environment and rollback risks that warrant a complete single-pass review.

_{To customize comments, go to the Qodo configuration screen, or learn more in the docs.}

[qodo-code-review]

Qodo Fixer

✅ Committed (1) · ☑ Fixed (1)

Commits pushed directly to this PR — no separate fix PR opened.

Process — 1 fixed

• ☑ Fixed: User PATH overwritten in tests

[coderabbitai]

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (2)

src/util/test_paths.rs (2)

85-85: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win

Make the formatting gate pass before merge.

Current CI reports Format failed. Run cargo fmt --all, commit the formatter changes, and rerun cargo fmt --check. Do not treat this change as ready while the required Rust formatting gate is red.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/util/test_paths.rs` at line 85, Run cargo fmt --all to apply the
repository’s Rust formatting, commit all resulting formatter changes, and rerun
cargo fmt --check to confirm the formatting gate passes before merge.

Sources: Coding guidelines, MCP tools

---

76-76: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift

Implement the promised User PATH restoration.

PathRestoreGuard restores only the process-local PATH and removes the test flag. It does not snapshot or restore the Windows User PATH registry value. The supplied persist_path_dir_refuses_temp_directories test removes the flag while the guard is held, so a future non-temporary write could survive teardown.

Snapshot the User PATH at construction and restore it in Drop. Preserve the difference between an absent value and an empty value. Add Windows tests for both states. If prevention without restoration is the intended contract, update the objective and verify every User PATH write path remains blocked while the guard is held.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/util/test_paths.rs` at line 76, Update PathRestoreGuard to snapshot the
Windows User PATH registry value during construction and restore it in Drop,
preserving the distinction between an absent value and an explicitly empty
value; add Windows tests covering both states and retain cleanup of
NUMAN_TEST_NO_PERSIST_USER_PATH.

Source: MCP tools

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Outside diff comments:
In `@src/util/test_paths.rs`:
- Line 85: Run cargo fmt --all to apply the repository’s Rust formatting, commit
all resulting formatter changes, and rerun cargo fmt --check to confirm the
formatting gate passes before merge.
- Line 76: Update PathRestoreGuard to snapshot the Windows User PATH registry
value during construction and restore it in Drop, preserving the distinction
between an absent value and an explicitly empty value; add Windows tests
covering both states and retain cleanup of NUMAN_TEST_NO_PERSIST_USER_PATH.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: d117bc1c-26c1-4226-9655-30a4122ac2e4

📥 Commits

Reviewing files that changed from the base of the PR and between 3ed1106 and e029de7.

📒 Files selected for processing (1)

• src/util/test_paths.rs

🔗 Linked repositories identified

CodeRabbit considers these linked repositories for cross-repo context during reviews:

• tonythethompson/QuickShell (manual)
• tonythethompson/numan (manual)
• tonythethompson/dependency-chain-substrate (manual)

📜 Review details

⏰ Context from checks skipped due to timeout. (4)

• GitHub Check: Greptile Review
• GitHub Check: Real-Nu acceptance (windows-latest)
• GitHub Check: Test (windows-latest)
• GitHub Check: Analyze (rust)

🧰 Additional context used

📓 Path-based instructions (9)

**/*

📄 CodeRabbit inference engine (CLAUDE.md)

**/*: Use Serena's semantic, symbol-aware tools as the primary tools for reading and editing code; use built-in Read, Glob, Grep, and Edit only under the stated exceptions.
Before editing a code file, inspect its symbol overview, read the specific symbols being changed, and edit them with Serena's symbol-aware tools.
Understand the existing code before changing it and make the smallest change that satisfies the request; avoid unrelated cleanup, premature abstractions, unnecessary error handling, feature flags, and compatibility shims.
Prefer editing existing files over creating new ones, and never create Markdown or README files unless explicitly requested.
For exploratory questions, provide a 2–3 sentence recommendation with the main tradeoff and do not implement until the user agrees.
For UI or frontend changes that cannot be tested in a browser, explicitly state that browser testing was not performed rather than claiming success.
Address security issues when discovered, including injection, XSS, SQL injection, path traversal, and secret leaks.
Pause and obtain confirmation before destructive, hard-to-reverse, externally visible, or third-party-upload actions, including deleting files or branches, dropping tables, force-pushing, modifying CI/CD, posting externally, or uploading content.
When blocked, investigate the root cause instead of bypassing it with --no-verify, --force, or deletion; investigate unfamiliar files, branches, and configuration before deleting them.
Only commit when explicitly asked; do not proactively update git configuration or push changes.
Do not skip Git hooks unless explicitly asked; if a pre-commit hook fails, fix the issue, re-stage, and create a new commit rather than amending.
Stage files by name rather than using git add -A or git add ., to avoid accidentally including secrets or large binaries.
Use a HEREDOC for commit messages, and add co-author attribution only when the user explicitly requests the exact trailer.
Do not force-pus...

Files:

• src/util/test_paths.rs

**/*.{js,jsx,ts,tsx,py,java,go,rs,rb,php,c,cpp,h,hpp,cs,swift,kt,kts}

📄 CodeRabbit inference engine (CLAUDE.md)

Add comments only when the WHY is non-obvious; do not narrate what the code does, reference the current task, or reference the PR in comments.

Files:

• src/util/test_paths.rs

!**/.env,!**/credentials.json,!**/*.pem

📄 CodeRabbit inference engine (CLAUDE.md)

Do not commit files that appear to contain secrets, including .env, credentials.json, and PEM files; warn before doing so even if explicitly requested.

Files:

• src/util/test_paths.rs

**/*.rs

📄 CodeRabbit inference engine (CONTRIBUTING.md)

**/*.rs: Use the Rust 2021 edition.
Use anyhow::Result with .context(...) in application code; use thiserror for library error types that callers match on.
Use clap derive macros for CLI definitions.
Use serde with serde_json or toml for serialization.
Function parameters must use &Path, not &PathBuf.
Library code must not panic; error paths should return anyhow::Result with context where appropriate.
Add or update tests for behavior changes, including relevant failure paths.
New mutating code paths must acquire the mutation lock via acquire_mutation_lock(root) and snapshot the lockfile before writes.
Lockfile, journal, and state-file JSON writes must use write_json_atomic.
numan install must write only to $NUMAN_ROOT; it must not invoke Nu or register plugins/autoloads.
Only activate and deactivate may modify Nu integration state.
Treat the lockfile as the authoritative source of truth; derived projections such as autoload state must not be authoritative.
Install payloads under versioned, content-addressed paths and never overwrite them in place.
Never overwrite foreign autoload files; respect OWNERSHIP_MARKER.
Pass plugin paths through environment variables only; do not use runtime interpolation in Nu program strings.

**/*.rs: All CI gates must pass: cargo test, cargo clippy -- -D warnings, and cargo fmt --check.
Every mutating command—including install, remove, update, gc, and future nupm import—must call acquire_mutation_lock(root).
Lockfiles, journals, and state files must use write_json_atomic; partial writes are not allowed.
Pending activation, autoload, and lifecycle journals must be stored under $NUMAN_ROOT/state/.
Module autoload identity must match all four fields: Nu executable hash, Nu version, vendor autoload directory, and managed file path; the lockfile module_activation value is authoritative.
Never overwrite foreign autoload files; respect OWNERSHIP_MARKER.
Pass paths to Nu only throu...

Files:

• src/util/test_paths.rs

**/*.{rs,toml}

📄 CodeRabbit inference engine (CONTRIBUTING.md)

Run and keep cargo fmt/rustfmt clean, and ensure cargo clippy -- -D warnings passes.

Files:

• src/util/test_paths.rs

**/*.{rs,nu}

📄 CodeRabbit inference engine (CONTRIBUTING.md)

**/*.{rs,nu}: Real-Nu acceptance tests must be marked #[ignore] and should be run when changes affect activation or nupm import; unit tests must not spawn real nu and should use injectable seams such as FakeCandidateRunner or registrars.
The nupm integration must be read-only toward NUPM_HOME, must not execute build.nu, and must not perform bidirectional synchronization.

Unit tests must use FakeCandidateRunner or injectable registrars and must not spawn a real nu process.

Files:

• src/util/test_paths.rs

**/*.{rs,md}

📄 CodeRabbit inference engine (CONTRIBUTING.md)

Match existing naming, module layout, and documentation level in the file being edited; update AGENTS.md, docs/, or command help when structure, conventions, or user-visible behavior changes.

Tests must cover failure modes, not only successful execution.

Files:

• src/util/test_paths.rs

**/*.{rs,md,toml}

📄 CodeRabbit inference engine (CONTRIBUTING.md)

Use the repository's established serialization and module conventions rather than introducing unrelated refactors.

Files:

• src/util/test_paths.rs

src/**/*.rs

📄 CodeRabbit inference engine (AGENTS.md)

Use anyhow::Result for application code, thiserror for library error types, add context with .context(...) or ?, and never panic in library code.

Files:

• src/util/test_paths.rs

🔍 Remote MCP DeepWiki, GitHub Copilot

Additional review context

• The PR diff does not implement User PATH snapshot/restoration, despite the PR description claiming it does. PathRestoreGuard only sets/removes NUMAN_TEST_NO_PERSIST_USER_PATH and restores the process PATH; no registry snapshot or restoration is present.
• PathRestoreGuard is used by tests in src/nu/paths.rs, src/cmd/setup.rs, src/cmd/doctor.rs, and tests/setup_nu_test.rs, so the environment flag affects multiple test paths.
• persist_path_dir is the shared persistence entry point for PATH updates, including Windows User PATH handling.
• Current CI reports Format failed, while Clippy, MSRV, Linux tests, packaging, and CodeFactor passed. Windows and macOS test jobs were still running at retrieval time.
• DeepWiki could not locate the repository files or symbols needed for architectural analysis, so it provided no additional repository-grounded context.

🔇 Additional comments (3)

src/util/test_paths.rs (3)

50-62: Restore the prior test-flag value.

Drop still removes NUMAN_TEST_NO_PERSIST_USER_PATH unconditionally. Save its value before PathRestoreGuard::new changes it, then restore that value or remove the variable only when it was initially absent.

Also applies to: 76-76

---

8-12: LGTM!

---

38-38: LGTM!

[linear-code]

NUM-81