Summary
A test-quality sweep found that several test helpers still mutate the process environment in-process (std::env::set_var / std::env::remove_var), relying on a shared mutex or OnceLock for safety. Project policy (AGENTS.md) prefers dependency injection via the mockable crate for environment-dependent behaviour; mutex-guarded in-process mutation remains a process-wide side effect, is unsafe in Rust 2024, and can still leak between tests when a guard is dropped early, a test panics, or a third-party library reads the environment on another thread. Sub-process isolated environments (e.g. Command::env) are fine and are not in scope.
Findings
tests/test_utils.rs:
clear_podbot_env() — std::env::remove_var("PODBOT_CONFIG_PATH") and a loop removing every variable in env_var_names() (lines ~89 and ~96), guarded by EnvGuard.set_env_var(&EnvGuard, key, value) — std::env::set_var (line ~125).clean_env rstest fixture — wraps clear_podbot_env() (line ~197).TestStdinForwardingGuard::disable() / Drop — sets and removes PODBOT_DISABLE_STDIN_FORWARDING_FOR_TESTS (lines ~161 and ~174).
Consumers of TestStdinForwardingGuard (in-process mutation at one remove):
tests/bdd_interactive_exec_helpers/steps.rs line ~108.tests/bdd_orchestration_helpers/steps.rs line ~109.
tests/bdd_repository_cloning_e2e_helpers/container.rs:
ensure_docker_host() — std::env::set_var("DOCKER_HOST", &endpoint) inside a OnceLock::get_or_init (lines ~114-116). This one exists because testcontainers reads DOCKER_HOST during client construction and exposes no in-process alternative; it may need an upstream fix or a documented, narrowly scoped exception.
For contrast, the config-loader, hosting-config-loader, and engine-connection BDD suites already use mockable::MockEnv with per-scenario state maps and need no changes; they are the model to converge on.
Proposed enforcement
Adopt the clippy disallowed-methods prohibition used in leynos/netsuke, which denies the environment-mutation and environment-read APIs with actionable reasons:
# clippy.toml
disallowed-methods = [
{ path = "std::env::var", reason = "inject an environment reader" },
{ path = "std::env::var_os", reason = "inject an environment reader" },
{ path = "std::env::vars", reason = "inject an environment reader" },
{ path = "std::env::vars_os", reason = "inject an environment reader" },
{ path = "std::env::set_var", reason = "use a stub environment in tests" },
{ path = "std::env::remove_var", reason = "use a stub environment in tests" },
]with disallowed_methods = "deny" in the workspace lint table. As in netsuke, any site that cannot yet migrate (e.g. the testcontainers DOCKER_HOST case) carries #[expect(clippy::disallowed_methods, reason = "..")] rather than allow, so the expectation warns once the site is migrated and the backlog removes itself.
Suggested migration
- Rework
load_config-style entry points and the stdin-forwarding knob to accept an injected mockable::Env (most call paths already support this).
- Replace
clean_env/set_env_var/TestStdinForwardingGuard usage with MockEnv-backed state, following the config-loader BDD suites.
- Decide the
DOCKER_HOST/testcontainers exception: documented #[expect] at a single choke point, or move that suite to a sub-process harness.
- Add the clippy prohibitions above so regressions fail the lint gate.
Found during the test-quality sweep on branch test-quality-sweep (flagged only; deliberately not fixed there).
Summary
A test-quality sweep found that several test helpers still mutate the process environment in-process (
std::env::set_var/std::env::remove_var), relying on a shared mutex orOnceLockfor safety. Project policy (AGENTS.md) prefers dependency injection via themockablecrate for environment-dependent behaviour; mutex-guarded in-process mutation remains a process-wide side effect, is unsafe in Rust 2024, and can still leak between tests when a guard is dropped early, a test panics, or a third-party library reads the environment on another thread. Sub-process isolated environments (e.g.Command::env) are fine and are not in scope.Findings
tests/test_utils.rs:clear_podbot_env()—std::env::remove_var("PODBOT_CONFIG_PATH")and a loop removing every variable inenv_var_names()(lines ~89 and ~96), guarded byEnvGuard.set_env_var(&EnvGuard, key, value)—std::env::set_var(line ~125).clean_envrstest fixture — wrapsclear_podbot_env()(line ~197).TestStdinForwardingGuard::disable()/Drop— sets and removesPODBOT_DISABLE_STDIN_FORWARDING_FOR_TESTS(lines ~161 and ~174).Consumers of
TestStdinForwardingGuard(in-process mutation at one remove):tests/bdd_interactive_exec_helpers/steps.rsline ~108.tests/bdd_orchestration_helpers/steps.rsline ~109.tests/bdd_repository_cloning_e2e_helpers/container.rs:ensure_docker_host()—std::env::set_var("DOCKER_HOST", &endpoint)inside aOnceLock::get_or_init(lines ~114-116). This one exists becausetestcontainersreadsDOCKER_HOSTduring client construction and exposes no in-process alternative; it may need an upstream fix or a documented, narrowly scoped exception.For contrast, the config-loader, hosting-config-loader, and engine-connection BDD suites already use
mockable::MockEnvwith per-scenario state maps and need no changes; they are the model to converge on.Proposed enforcement
Adopt the clippy
disallowed-methodsprohibition used in leynos/netsuke, which denies the environment-mutation and environment-read APIs with actionable reasons:with
disallowed_methods = "deny"in the workspace lint table. As in netsuke, any site that cannot yet migrate (e.g. thetestcontainersDOCKER_HOSTcase) carries#[expect(clippy::disallowed_methods, reason = "..")]rather thanallow, so the expectation warns once the site is migrated and the backlog removes itself.Suggested migration
load_config-style entry points and the stdin-forwarding knob to accept an injectedmockable::Env(most call paths already support this).clean_env/set_env_var/TestStdinForwardingGuardusage withMockEnv-backed state, following the config-loader BDD suites.DOCKER_HOST/testcontainersexception: documented#[expect]at a single choke point, or move that suite to a sub-process harness.Found during the test-quality sweep on branch
test-quality-sweep(flagged only; deliberately not fixed there).