[Mikola Lysenko (mikolalysenko)]

LLM Description written by Claude Code:claude-opus-5-5

Fixes #1889

Root cause

Every core test that changes process env wrote its own restore-on-drop guard: 11 struct EnvGuard copies, the with_env / with_home_env closure helpers and api::client's OrgResolutionEnvGuard. The copies had drifted, and two of the drifts were real defects:

• Most copies saved the previous value as a String (env::var(..).ok()). A non-UTF-8 value was therefore removed instead of restored.
• with_env (utils/socket_cli_config.rs) and with_home_env (utils/fs.rs) restored env only after the closure returned. A failing assertion inside them leaked HOME, USERPROFILE, XDG_DATA_HOME or SOCKET_NO_CONFIG into the next #[serial] test.

Change

• New #[cfg(test)] crate::test_env, next to test_rng:
  • EnvGuard::set(key, impl AsRef<OsStr>), EnvGuard::unset(key) and EnvGuard::many(pairs), where a None value removes the var.
  • with_env(pairs, f).
  • Saves previous values as OsString and restores newest-first, so a key listed twice ends at its original value.
  • Panic-safe, including a panic partway through many.
• Every struct EnvGuard copy in core src/ now uses it. with_env and with_home_env are thin wrappers or re-exports. OrgResolutionEnvGuard becomes isolate_org_resolution_env(), which returns an EnvGuard. The proxy test's hand-written SOCKET_PROXY_URL save/restore moved under the guard.
• The three integration tests that had their own copy (upstream_restore_golden, crawler_deno_e2e, crawler_npm_e2e) compile the same file in with #[path = "../src/test_env.rs"]. grep -rn "struct EnvGuard" crates/socket-patch-core now finds exactly one definition.
• The guard's self-tests live in src/test_env_tests.rs, so they run once (lib) and not again in each binary that includes it.
• No production code changes. Every touched test keeps its #[serial].

Net diff: about -270 lines.

Review findings I didn't fix (from /code-review high)

• Other hand-written set_var / remove_var save-restore in core tests (update/{state,swap,release,mod}.rs, policy/tests.rs, telemetry.rs, vendor/registry_fetch.rs, the token-veto tests in api/client.rs and others, about 150 call sites in 20+ files): out of scope. Share one scoped env-var test guard: EnvGuard is copy-pasted 11 times across core unit and integration tests #1889 is about the guard copies. Moving every raw set_var onto the guard is a larger follow-up, which is now easy because the guard exists. Some of those tests also lack #[serial] (api/client.rs test_get_api_client_from_env_no_token), and that belongs in the same follow-up.
• The #[path] include stanza appears 3 times instead of living in tests/common/mod.rs: two of the three binaries don't include common, and including it would pull its helpers into binaries that don't use them. One 4-line stanza that points at the single definition replaces three 25-line copies.

CI cost

No workflow change. A lean PR run is unchanged: ~28 Linux jobs, about 8 min. Because one edited test is cfg(target_os = "macos") (npm_crawler volta/fnm), I dispatched one macOS-only run on this branch (os=macos, hosted_e2e=skip): https://github.com/SocketDev/socket-patch/actions/runs/41020866014

Test evidence

• cargo fmt --all -- --check: clean.
• cargo clippy --workspace --all-features -- -D warnings: clean. With --all-targets, the lint set is the same as on origin/main; the review commit fixed the new needless_borrows / unnecessary_to_owned hits.
• cargo test -p socket-patch-core --all-features --lib: 6997 passed, 4 failed. The same 4 fail on unmodified origin/main in this sandbox, because it runs as root and root ignores the read-only dirs they rely on (same as Build: share one test RNG across randomized tests (#1868) #1882): copy_tree::relax_loop_must_not_traverse_symlinked_root, vlt_heal::an_unremovable_hidden_lock_keeps_every_store_entry, pypi_poetry::wire_write_failure_maps_error_and_leaves_lock_untouched, pypi_requirements::wire_failure_rolls_back_already_written_files.
• cargo test -p socket-patch-core --all-features --test upstream_restore_golden --test crawler_deno_e2e --test crawler_npm_e2e: 53 + 123 + 13 passed.
• New self-tests (test_env_tests): restore after set, unset and a repeated key; with_env restores after a panic (the old with_env / with_home_env failed this); a non-UTF-8 previous value survives (the old String copies lost it).
• I couldn't cross-check x86_64-apple-darwin / windows locally because ring needs a C cross toolchain. That's why the macOS dispatch above exists.

https://claude.ai/code/session_011f9aePwCD7qBYF59E3rCfT

---

Generated by Claude Code

[Mikola Lysenko (mikolalysenko)]

BugBot review

---

Generated by Claude Code

[cursor]

✅ Bugbot reviewed your changes and found no new issues!

Comment @cursor review or bugbot run to trigger another review on this PR

^{Reviewed by Cursor Bugbot for commit c34e51c. Configure here.}

[Mikola Lysenko (mikolalysenko)]

[agent] e2e-macos (macos-latest, e2e_redirect_npm_build, 1) failed on the macOS-only dispatch run 41020866014. The failing test is npm_agent_keeps_crlf_shebang_of_bin_npm_did_not_link (e2e_redirect_npm_build.rs:1388): npm rewrote the CRLF shebang of a bin it wasn't expected to link.

This failure is not from this PR:

• The same leg fails on main's 10-30 nightly (41009903891). It passed on the 10-29 nightly (40824880386).
• This PR is #[cfg(test)]-only in socket-patch-core and touches no CLI or npm code.
• The leg is in the full tier only, so it doesn't gate this PR's lean checks.

No fix exists yet. It looks like the macOS runner's npm changed which bins get their shebang rewritten (#1804 territory). I'm leaving it to the nightly/janitor lane rather than widening this PR. No other macOS leg in the dispatch run has failed so far.

Update: test (macos-latest, 2) also failed, on -p socket-patch-cli --test e2e_golang_workspace_build. That is the same single target that fails test (macos-latest, 2) on main's 10-30 nightly (job 123913620124). It's a CLI Go e2e suite, which this PR doesn't touch, and every other target in the shard passed.

---

Generated by Claude Code

[Mikola Lysenko (mikolalysenko)]

[burn-down agent] Labeled Ready for review at c34e51c.

• CI: all lean PR checks green on c34e51c; mergeable.
• Main-wide failures (not from this PR, also failing on main's 10-30 nightly run 41009903891): e2e-macos (macos-latest, e2e_redirect_npm_build, 1) (fix in Fix CRLF bin e2e failing under npm 11 on macOS #1896), test (macos-latest, 2) / e2e_golang_workspace_build (fix in Fix Go GOWORK test failing on macOS nightly #1894), and the ci-ok (macos only) rollup they feed.
• Bugbot: reviewed c34e51c, no findings.
• Reviewer note: #[cfg(test)]-only; replaces 11 hand-rolled EnvGuard copies in core with one shared guard.

---

Generated by Claude Code

[Mikola Lysenko (mikolalysenko)]

Final review brief

What it does: It replaces 11 hand-rolled test EnvGuard copies and the with_env/with_home_env helpers with one #[cfg(test)] module, crate::test_env. That module saves previous values as OsString and restores them newest-first on drop, so values are restored even when a test panics. The three integration-test binaries share the same file through #[path].

Risk: low. The change is test-only: both new modules are behind #[cfg(test)], so nothing reaches release builds. Ordering still comes from #[serial], which every converted call site already had. The guard takes no lock, so nested use can't deadlock.

Look here

• test_env.rs:26: many saves each value right before setting it, so a panic partway through is safe
• test_env.rs:48: the drop restores newest-first, so a key listed twice ends at its original value
• fs.rs:1489: with_home_env is now a one-line wrapper
• upstream_restore_golden.rs:189:`` the #[path] include used by the integration tests

Verified: I read the full diff, and git grep "struct EnvGuard" finds a single definition. There is no debug code, and CHANGELOG.md and the workflows are untouched. The required checks ci-ok and clippy are green, and Bugbot passed. The macOS-only legs fail on two tests that were already failing on main and are fixed by #1894 and #1896, not by anything in this PR. A macOS run is worth repeating after those land.

Changes I made: none.

Open questions (non-blocking): the guard can't force a test to use #[serial]. The roughly 150 raw set_var calls left in the tree are deferred to a follow-up, as the description says.

Auto-merge is armed, so approving sends this straight to the merge queue.

---

Generated by Claude Code