refactor: Add EnvGuard RAII wrapper for safe env var handling in tests (#179)

[inureyes]

Closes #179

Summary

Introduce EnvGuard, an RAII wrapper that encapsulates the unsafe
std::env::set_var / std::env::remove_var calls and restores the
prior value on drop, then convert 16 test files to use it together
with #[serial] from the already-present serial_test dev-dependency.

This finishes the Rust 2024 edition migration that commit 73b78571
started but left in a broken state: the test build previously failed
with 120 E0133 errors on cargo check --lib --tests. It now
builds cleanly with zero such errors.

Phases

Phase 1 — Foundation

New files:

• src/test_helpers/env_guard.rs — EnvGuard struct with set /
  remove constructors and a Drop impl that restores the
  OsString-preserving original. Gated with #![cfg(test)] at the
  module declaration in src/lib.rs. All unsafe {} blocks live
  here under documented SAFETY: comments.
• src/test_helpers/mod.rs — module exports.
• tests/common/mod.rs — re-exports EnvGuard to integration tests
  via a #[path = "../../src/test_helpers/env_guard.rs"] include.
  This keeps one source of truth without making EnvGuard public
  API of the bssh crate.

Changes to existing files:

• src/lib.rs — adds #[cfg(test)] pub(crate) mod test_helpers;.

Phase 2 — src/ unit tests (11 files)

• src/cli/pdsh.rs — doctest marked ignore
• src/ssh/ssh_config/integration_tests/env_cache_integration_test.rs
• src/security/sudo.rs
• src/ssh/auth.rs
• src/cli/mode_detection_tests.rs
• src/jump/parser/tests.rs
• src/config/tests.rs
• src/server/config/loader.rs
• src/ssh/client/connection.rs
• src/executor/rank_detector.rs
• src/jump/chain/auth.rs

Every env::set_var / env::remove_var call in these files is now
either removed (when the adjacent save/restore boilerplate is no
longer needed) or wrapped in EnvGuard::set / EnvGuard::remove.
Every test that touches environment variables has #[serial]
applied.

Phase 3 — tests/ integration tests (5 files)

• tests/interactive_test.rs
• tests/pdsh_compat_test.rs
• tests/jump_host_config_test.rs
• tests/exit_code_integration_test.rs
• tests/backendai_env_test.rs

Each file gains mod common; + use common::EnvGuard;. The
backendai_env_test.rs conversion also deletes the hand-rolled
ENV_MUTEX / once_cell::sync::Lazy<Mutex<()>> pattern at the top
of the file and every let _guard = ENV_MUTEX.lock().await; call
site — #[serial] on #[tokio::test] replaces it. serial_test's
#[serial] attribute works correctly on async Tokio tests, verified
by running the converted tests.

Phase 4 — Finalization (PR review MEDIUM finding)

Both pr-reviewer and pr-security-checker flagged the same MEDIUM
issue: the original SAFETY: comments on the three unsafe blocks
overstated what serial_test::serial guarantees — it only serializes
against other #[serial]/#[parallel] tests, not against unannotated
tests that may still run concurrently and race on environment reads.

Addressed in two commits on top of the original PR work:

• docs: tighten EnvGuard SAFETY comments and add soundness contract

  • Expanded the set SAFETY comment to state the full caller contract
  • Updated remove and Drop::drop comments to reference the full
    comment and module-level soundness section
  • Added a ## Soundness contract section to the module-level rustdoc
    explicitly naming the UB risk on glibc/musl/macOS
  • Added a "Test Environment-Variable Mutation Pattern" subsection to
    ARCHITECTURE.md covering where EnvGuard lives, the soundness
    contract, how integration tests access it, and #[serial] vs
    #[serial(key)] guidance

• test: add unit tests for EnvGuard RAII behaviour

  • Five focused unit tests directly verify the Drop-based restore
    semantics: set-from-absent, remove-with-restore, chained LIFO
    guards, remove-on-already-unset (no-op), and set-over-existing
  • All tests carry #[serial] and use unique BSSH_ENVGUARD_TEST_VAR_*
    names to avoid clashing with other tests
  • Raises lib test count from 1189 to 1194

Validation

• cargo build --all-targets — clean
• cargo clippy --all-targets --all-features -- -D warnings — clean
• cargo fmt --all -- --check — clean
• cargo test --lib -- --skip keychain_macos — 1194 passed, 0 failed, 9 ignored
• All 5 modified integration-test binaries pass independently (87 tests total)
• Zero remaining unsafe { env::set_var ... } outside src/test_helpers/env_guard.rs
• Zero remaining ENV_MUTEX references in tests/

[inureyes]

Implementation Review Summary

Intent

Wrap unsafe env::set_var/env::remove_var calls in an EnvGuard RAII helper plus #[serial], finish the Rust 2024 edition migration, and fix the broken test build (120 → 0 E0133 errors).

Findings

#	Severity	File	Issue
1	MEDIUM	src/test_helpers/env_guard.rs:78-80	SAFETY: justification is technically incomplete: #[serial] only serializes against other #[serial] tests, not against non-serial tests that read env vars (e.g. commands::interactive::utils::tests::test_expand_path_with_tilde calls dirs::home_dir() which reads HOME while serial tests mutate HOME). Pre-existing race hazard, not a regression — and the practical impact is bounded because env operations are atomic on modern libc and tests use ASCII values. Worth tightening the SAFETY comment in a follow-up.

No CRITICAL or HIGH findings. Codex review (codex-cli 0.120.0) was consulted and surfaced the same SAFETY-comment observation.

Findings Addressed

None — sole MEDIUM finding is documentation-only and non-blocking; deferred to a follow-up SAFETY-comment tightening.

Remaining Items

• (MEDIUM) src/test_helpers/env_guard.rs:78-80 — SAFETY comment should acknowledge that serialization is only against other #[serial] tests; the practical safety relies on atomic libc operations and ASCII test values. Suitable for a follow-up doc PR.

Verification

• All stated requirements implemented
  • src/test_helpers/env_guard.rs matches the design (OsString-based, #[must_use], #![cfg(test)], Drop restores None → remove_var, all unsafe {} documented with // SAFETY:)
  • src/test_helpers/mod.rs exports EnvGuard
  • src/lib.rs adds #[cfg(test)] pub(crate) mod test_helpers;
  • tests/common/mod.rs uses #[path = "../../src/test_helpers/env_guard.rs"] correctly — same source file shared with unit tests, no duplication
• No placeholder/mock code remaining
• Integrated into project code flow (test infrastructure refactor; integration here means the test build compiles and tests pass)
• Project conventions followed
  • Commit messages use refactor: prefix, no Co-Authored-By, no Generated by Claude attribution
  • PR title and body reference issue (Closes #179)
  • Code is idiomatic Rust 2024
  • EnvGuard is pub(crate) via test_helpers module, gated by #[cfg(test)] — no public API leak
• Existing modules reused where applicable (uses already-present serial_test dev-dependency)
• No unintended structural changes (only test files plus the new test_helpers module)
• Tests pass

Build/Test Validation (commit fb206899)

• cargo check --lib --tests: 0 errors (was 120 E0133 on main)
• cargo build --all-targets: clean
• cargo build --release --lib: clean (verifies test_helpers is gated by #[cfg(test)] and not linked in release)
• cargo clippy --all-targets --all-features -- -D warnings: clean
• cargo test --lib -- --skip keychain_macos: 1189 passed, 0 failed, 9 ignored (7 filtered)
• All 5 modified integration test binaries pass independently:
  • backendai_env_test: 3 passed
  • pdsh_compat_test: 30 passed
  • jump_host_config_test: 33 passed
  • exit_code_integration_test: 12 passed
  • interactive_test: 9 passed
• Two clippy-fix files still pass: glob_pattern_test (8) and pty_stress_test (10)
• unsafe { env::set_var ... } / unsafe { env::remove_var ... } outside src/test_helpers/env_guard.rs: 0 in compiled code (only orphaned src/commands/interactive_unit_test.rs retains them — file is not declared in src/commands/mod.rs, has stale struct fields, never compiles, and the PR's decision to skip is justified)
• ENV_MUTEX references in tests/: 0

interactive_unit_test.rs Skip Verified

The file is a true orphan: not declared in src/commands/mod.rs, references stale InteractiveCommand struct fields (missing key_path, use_agent, use_password, strict_mode, pty_config, jump_hosts, ssh_connection_config, use_pty, use_keychain), and contributes 0 E0133 errors to the build. Touching it would have drifted into out-of-scope territory.

Verdict: APPROVE

The PR delivers the full scope of issue #179 and substantially improves the test infrastructure. No CRITICAL or HIGH findings; the single MEDIUM finding is a documentation refinement that doesn't block the merge.

[inureyes]

PR Finalization Complete

Summary

MEDIUM finding addressed: The SAFETY comments in src/test_helpers/env_guard.rs overstated what serial_test::serial guarantees. The fix accurately documents that #[serial] only serializes against other #[serial]/#[parallel] tests — unannotated tests can still race on env reads while serial tests mutate the env. Both reviewers raised the same finding; it is now resolved.

Changes by file:

• src/test_helpers/env_guard.rs:

  • Added ## Soundness contract section to module-level rustdoc naming the libc-level UB risk and the full caller contract
  • Expanded SAFETY comment on set with accurate, complete wording; updated remove and Drop::drop to reference it
  • Added 5 unit tests in a mod tests block verifying Drop-based restore semantics (set-from-absent, remove-with-restore, chained LIFO guards, remove-on-unset no-op, set-over-existing)

• ARCHITECTURE.md:

  • Added "Test Environment-Variable Mutation Pattern (EnvGuard)" subsection documenting where EnvGuard lives, the soundness contract, the integration-test access pattern via tests/common/mod.rs, and guidance on #[serial] vs #[serial(key)]

Commits pushed:

• 1964d34f — docs: tighten EnvGuard SAFETY comments and add soundness contract
• b8b749bb — test: add unit tests for EnvGuard RAII behaviour

Final verification:

• cargo build --all-targets — clean
• cargo clippy --all-targets --all-features -- -D warnings — clean
• cargo fmt --all -- --check — clean
• cargo test --lib -- --skip keychain_macos — 1194 passed, 0 failed (up from 1189; +5 new EnvGuard unit tests)

Ready for merge.