Convert the init-step and verify tests, and remove the last environment mutation

[sehkone]

Convert the init-step and verify tests, and remove the last environment mutation

Context

This is the last of the environment mutation in this crate's tests. In Rust 2024 std::env::set_var and std::env::remove_var are unsafe: outside Windows, another thread reading the environment concurrently is undefined behaviour, and the shared Mutex used as a workaround does not discharge that contract — it serialises only the code that takes it, while a tokio runtime, an HTTP client, an env-filtered tracing subscriber and the libc resolver read the environment from threads that hold nothing.

Two groups remain by the time this issue runs.

The init step tree. Five files write a shell script named docker into a tempdir, prepend that directory to PATH, and let production pick the fake up. They do it with machinery that does not live in their own test_support: all of it — ENV_LOCK, env_lock(), ScopedEnvVar, path_with_prepend, write_fake_docker_script, TEST_DOCKER_ARGS_ENV — is pub(in crate::commands) in src/commands/rotate.rs:266, and these files import it across the tree. src/commands/init/steps.rs:861's own test_support holds only default_init_args and re-exported test_messages / test_cert_pem; its ENV_LOCK and env_lock() are already gone, deleted along with their sole caller in database.rs.

src/commands/verify.rs. It has its own ENV_LOCK at line 395 and does the same for a different executable. resolve_agent_binary (line 158) falls back to find_on_path(AGENT_BINARY_NAME) at line 179 — find_on_path is defined at line 193 and reads PATH directly — so its tests overwrite PATH to point at a fake bootroot-agent. One of them additionally calls std::env::set_current_dir, which is process-global in exactly the same way and is currently covered by the same lock.

Two facts about the repository this issue can rely on. Production no longer hard-codes the docker executable on any path a PATH-faking test exercises: infra::run_docker, the compose path through ComposeInvocation::command, and the two test-reached direct spawns in init/steps/stepca_setup.rs:480 and rotate/infra_cert.rs:209 all take it from a caller-supplied value defaulting to docker, and the production functions between them and the tests carry it through — infra::run_compose, issue_openbao_tls_cert, reissue_http01_admin_tls_cert and restart_stepca_openbao_agent for this tree. infra::docker_output, the two clean.rs spawn helpers and rotate::helpers::try_restart_container were deliberately left hard-coded, because no test in either conversion tree reaches them; if a conversion here turns out to need one, extend the seam to it — with the test that forced it named in the pull request — rather than reaching for PATH. And nothing under src/commands/rotate/ uses rotate::test_support's lock or ScopedEnvVar any more; the files converted here are its last consumers, which is why this issue is the one that can delete it.

Note that src/commands/ lives in the binary crate (src/main.rs:6), so cargo test --lib runs none of these tests. Use cargo test --bin bootroot.

Scope

The init step tree

Convert every remaining PATH-faking test to supply the fake executable through the injection seam:

File	env_lock() call sites	references to the fake-docker helpers
src/commands/init/steps/openbao_tls.rs	4	10
src/commands/init/steps/openbao_transition.rs	2	7
src/commands/init/steps.rs	1	5
src/commands/init/steps/stepca_setup.rs	1	5
src/commands/init/steps/http01_admin_tls.rs	1	5

These reach docker three different ways, and all three are already covered by the seam. openbao_tls.rs's four tests drive issue_openbao_tls_cert into run_docker at :97 and :203, and http01_admin_tls.rs's one drives reissue_http01_admin_tls_cert into run_docker at :84. stepca_setup.rs:596 calls restart_stepca_openbao_agent (:479), which spawns directly at :480 — note that this file's other run_docker call, at :550, is not what its fake-docker test exercises. And steps.rs:145, :168, :215 and openbao_transition.rs:249 go through run_compose, which builds its command in ComposeInvocation::command.

Three of these tests are negative: steps.rs's rollback_skips_openbao_recreate_when_this_run_did_not_recreate asserts the argv log file was never created, and openbao_transition.rs's no_available_source_fails_before_any_docker_call and tls_probe_failure_stops_before_the_url_is_recorded are the same shape. Converting one of these carelessly makes it vacuous — if the test stops supplying a fake at all, the absence of a log proves nothing. Keep supplying a fake that would log, through the seam, so the empty log stays evidence.

Two of these files write their own fake rather than using the shared helper. openbao_tls.rs has a local script that appends one line per invocation instead of truncating, because the ordering of the two containers it drives is exactly what its wiring test pins. Preserve that distinction — a converted test that loses the ordering assertion is a regression.

How a converted test gets its argv log out of the fake is settled the same way in every case. The seam carries a program path and nothing else — it does not carry child-environment entries — so there is no way to put BOOTROOT_TEST_DOCKER_ARGS in front of a child that a production function spawns except by setting it on this process, which is the mutation being removed. Every converted test therefore writes its own self-contained fake, with its argv-log path, and its exit code where one is needed, interpolated into the script text at the moment the test writes it into a tempdir. The child reads nothing from any environment, and the test passes whatever the ambient environment holds. That covers the three negative tests too: a self-contained fake still writes its log whenever it runs, so an absent log still proves it did not. If a conversion genuinely cannot work this way, extend the seam to carry child-environment entries and say in the pull request which test forced it. Do not set a variable on this process, and do not build a second injection mechanism alongside the seam.

verify.rs

src/commands/verify.rs holds 14 mutation sites, 10 unsafe blocks and 6 direct ENV_LOCK.lock() acquisitions at lines 638, 650, 677, 720, 759 and 794. All of it is scaffolding for two things:

• PATH. run_verify resolves the agent binary at line 38 and spawns whatever it gets at line 39, and resolve_agent_binary already accepts an explicit path through args.agent_binary. Most of these tests should supply that path instead of overwriting PATH. Where a test's whole point is the search behaviour of find_on_path, give that function the search path as a parameter and leave one caller reading PATH once. Preserve its semantics exactly, including the POSIX rule that an empty entry means the current directory — resolve_agent_binary_error_lists_empty_path_segment_as_cwd (line 719) and resolve_agent_binary_uses_cwd_for_empty_path_segment (line 758) are what pin it.
• The working directory. Lines 770-777 call std::env::current_dir and set_current_dir around the assertion in the second of those tests. The working directory is process-global exactly as the environment is, and deleting the lock without addressing it re-introduces a race the lock was covering. Pass the directory in and drop the process-wide move.

Delete ENV_LOCK at line 395 once nothing acquires it.

Emptying rotate::test_support

Once the five init files above are converted, src/commands/rotate.rs:266's test_support has no caller left for its global-state machinery. Delete ENV_LOCK (275), env_lock() (308), ScopedEnvVar (279) — which is where that file's three set_var/remove_var calls live — and path_with_prepend (335).

Keep what only touches a tempdir and still has a caller: test_messages, which src/commands/rotate/ca.rs:1101 imports, and write_fake_docker_script (314) if anything still calls it. Expect write_fake_docker_script and both variable names to go rather than stay: its script body is what reads BOOTROOT_TEST_DOCKER_ARGS and BOOTROOT_TEST_DOCKER_EXIT, a self-contained fake carries its log path in its own text instead, and once the last converted test stops calling it nothing names TEST_DOCKER_ARGS_ENV (276) or TEST_DOCKER_EXIT_ENV (277) either. Let the compiler decide: delete what has no caller, keep what has one. Leave nothing dead: cargo clippy -- -D warnings fails on an unused constant or function.

Likewise keep src/commands/init/steps.rs:861's test_support — default_init_args, test_messages, test_cert_pem are imported by orchestrator.rs, secrets.rs, responder_setup.rs, ca_certs.rs, openbao_setup.rs and stepca_setup.rs, none of which this issue otherwise touches.

Closing the effort out

This is the final issue in the effort, so it carries the crate-wide assertion: after it merges, the only set_var call left anywhere in src/ is the single production one at src/commands/dotenv.rs:96. grep -rn "set_var\|remove_var" src returns two lines, that call and the word set_var in the Settings::from_file rustdoc at src/config.rs:280, which is prose rather than code. No serialisation lock for environment mutation exists at all.

Acceptance criteria

• No env::set_var or env::remove_var under #[cfg(test)] remains anywhere in src/; the only call left is the production one at src/commands/dotenv.rs:96, unchanged and with its existing // SAFETY: comment intact. The rustdoc mention at src/config.rs:280 is prose and stays
• No ENV_LOCK or env_lock() definition or call site remains anywhere in src/, and neither does ScopedEnvVar or path_with_prepend
• No test anywhere in src/ calls std::env::set_current_dir
• src/commands/init/steps.rs's test_support still exports default_init_args, test_messages and test_cert_pem, and rotate::test_support still exports test_messages; every module outside this issue's scope that imports them still compiles
• find_on_path's search semantics are unchanged, empty-entry handling included
• Every converted test asserts what it asserted before, and still fails when production does the wrong thing
• No production behaviour changes: init and verify issue the same commands with the same arguments as before
• The diff adds no new unsafe block, and every unsafe block that existed to support environment mutation is gone

Constraints

• Do not replace the lock with another lock, and do not reach for --test-threads=1: it serialises test functions and says nothing about runtime threads.
• Do not add a second injection mechanism. Use the seam that already exists on the production spawn helpers; if it does not fit a call site here, extend it rather than building a parallel one.
• Never set a variable on this process. Note that the tests converted here do not construct the Command that runs the fake — production does — so Command::env on that child is not theirs to call unless the seam is extended to carry the entry; a self-contained fake that needs no variable at runtime is the expected shape. Where a test spawns a child itself, setting that child's environment is of course fine.
• Do not weaken an assertion to make a conversion easy. A test that stops checking what it used to check is a regression even when it passes.
• Follow the repository's Rust standards in AGENTS.md: narrowest visibility that compiles, no unwrap() outside tests, &Path/&str parameters over owned values.

Out of scope

• The production set_var in src/commands/dotenv.rs:96. Its // SAFETY: comment is correct: it runs once during single-threaded init setup before any worker thread exists, which does discharge the contract. Leave the call and the comment alone. Worth knowing, though not acted on here: that justification is about when it is called, so it would quietly stop holding if that function were ever invoked after startup.
• unsafe that has nothing to do with environment mutation. src/commands/init/steps/orchestrator.rs:1875 and :1879 wrap libc::umask in a test asserting the root-token file is created 0600 under a permissive umask; that is a thread-local syscall with its own correct // SAFETY: comment. Leave it, and leave any other unrelated unsafe in the crate alone.
• Changing what init or verify actually do. This is a test-side change; the sequence of commands each flow issues must be identical before and after.
• Extending or redesigning the injection seam beyond what these conversions need.
• src/commands/monitoring.rs:254, src/commands/rotate/ca.rs:824 and src/commands/init/steps/orchestrator.rs:1111, which read the environment in production but which no test mutates.

Test plan

• cargo test --bin bootroot and cargo test --lib both pass
• grep -rn "set_var\|remove_var" src returns only src/commands/dotenv.rs:96 and the doc comment in src/config.rs:280
• grep -rn "ENV_LOCK\|env_lock\|ScopedEnvVar\|ScopedEnv\|path_with_prepend\|set_current_dir" src returns nothing
• grep -rn '"PATH"' src/commands/init returns nothing, and the same grep over src/commands/verify.rs returns exactly one line: the single std::env::var_os("PATH") that the thin caller above find_on_path keeps. That surviving read is the point of the design, not a leftover — a check that demanded zero would be demanding the search behaviour be deleted
• grep -rn "unsafe {" src/commands/verify.rs src/commands/init/steps.rs src/commands/init/steps/openbao_tls.rs src/commands/init/steps/openbao_transition.rs src/commands/init/steps/stepca_setup.rs src/commands/init/steps/http01_admin_tls.rs src/commands/init/steps/database.rs src/commands/rotate.rs src/commands/compose_project.rs src/commands/openbao_auth.rs src/commands/openbao_url.rs src/commands/reinit.rs src/host_port.rs src/db.rs returns nothing, and the same grep over src/commands/dotenv.rs returns only the production call's block. Match unsafe {, not the bare word: src/commands/reinit.rs contains three unrelated matches for unsafe — the i18n method names error_reinit_root_token_output_unsafe (:574) and error_reinit_summary_json_unsafe (:666), and a doc sentence at :956 — none of which is an unsafe block and none of which this effort may touch. unsafe { is a complete pattern for this crate: grep -rn "unsafe fn\|unsafe impl\|unsafe trait" src returns nothing, so there is no unsafe construct here that the block form would miss
• Read the diff and confirm it introduces no unsafe block; this is a review criterion, not a grep, because unrelated unsafe remains elsewhere in the crate by design
• For at least one test per converted file, temporarily corrupt the expected argv or point the fake executable somewhere wrong, and confirm the assertion fails
• cargo clippy --all-targets -- -D warnings is clean, matching CI. A dead-code warning on a test_support item that lost its last caller is the expected way this goes wrong
• cargo fmt -- --check --config group_imports=StdExternalCrate is clean, matching CI
• Full CI green, including the Docker E2E matrix and the extended E2E workflow — init and verify run there against the real docker, so a conversion that changed real behaviour shows up

Dependencies

Three, all load-bearing, so this issue runs last. Part of #801.

• It depends on the issue that makes the docker executable a caller-supplied value. Without that seam these tests cannot redirect the child, because production constructs the Command.
• It depends on the issue that parameterises the environment-steered resolvers, because the crate-wide criteria above cannot hold while the tests in database.rs, dotenv.rs, host_port.rs, db.rs and the rest still mutate. What that prerequisite must remove is every remaining test-side mutation — dotenv.rs's six included, at lines 212, 219, 237, 244, 318 and 326 — while leaving the production set_var at dotenv.rs:96 and its // SAFETY: comment untouched. That call is the documented exception both issues preserve, not something the prerequisite has to eliminate; dotenv.rs appears in this list for its tests alone. That issue is also what removes init::steps::test_support's lock.
• It depends on the issue that converts the rotate test tree, because rotate::test_support's ENV_LOCK, ScopedEnvVar and path_with_prepend cannot be deleted here while approle.rs, infra_cert.rs, stepca_password.rs and helpers.rs still use them.

Pointers

• src/commands/rotate.rs:266-345 — the test_support module this issue empties, and the source of every fake-docker helper the init tests import
• src/commands/init/steps.rs:780 and :861 — a representative import of that machinery, and the local test_support that stays
• src/commands/init/steps/stepca_setup.rs:480 and :550 — a direct spawn and a run_docker call in the same file
• src/commands/init/steps/openbao_transition.rs:249 — the compose path this tree's tests fake
• src/commands/init/steps/openbao_tls.rs:471 — its imports, and the local append-mode fake below them
• src/commands/verify.rs:38, :158, :179 and :193 — the spawn, resolve_agent_binary, the find_on_path fallback and find_on_path itself
• src/commands/verify.rs:395 and :637-805 — the lock and the six tests that hold it, including the working-directory one at :758
• AGENTS.md, testing section — the rule this issue enforces