fix: sweep remaining env-mutating unit tests for hermeticity#445
Open
BunsDev wants to merge 1 commit into
Open
fix: sweep remaining env-mutating unit tests for hermeticity#445BunsDev wants to merge 1 commit into
BunsDev wants to merge 1 commit into
Conversation
Follow-up to #440: remove every env_lock/EnvVarGuard mutex-and-mutate pattern left in coven-cli by injecting dependencies instead of writing to process env. Public signatures and production behavior unchanged. - harness.rs: snapshot adapter env once via AdapterEnv (COVEN_HOME, manifest/dirs vars) and thread it through spec resolution; split command construction into env-free command_parts_with_specs taking codex_json/claude_bypass/resolve_program; add harness_available_with. Tests inject specs, AdapterEnv, and an identity program resolver — the ~45 formerly lock-guarded tests now run lock-free and parallel, and program asserts no longer depend on host PATH. - privacy.rs: snapshot env once via PrivacyEnv + apply_env_overrides (preserving file → env → settings → env precedence); tests inject PrivacyEnv; add retention-days override coverage. - main.rs → paths.rs: move the USERPROFILE fallback test onto the already-injectable coven_home_from_env seam; delete the last env_lock/EnvVarGuard helpers. - api.rs: launch validation reads configured harnesses from process env, so the unknown-harness test used a real installable adapter id (hermes) and failed 201!=400 on any machine with local adapter recipes; use a synthetic id so the test is hermetic everywhere. Supersedes the crate-wide test_env lock #444 landed as a stopgap for the same class of flake: with every test converted to injection there are no env writers left to serialize, so test_env.rs is deleted and the #444 guards in api.rs give way to the synthetic harness id. Ports #443's hermes model-forwarding test onto the injected AdapterEnv + command_parts_in_specs seams, preserving its coverage lock-free. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Co-authored-by: Timothy Wayne Gregg <5861166+CompleteDotTech@users.noreply.github.com>
BunsDev
force-pushed
the
fix/test-hermeticity-sweep
branch
from
July 24, 2026 09:18
eb28e15 to
8896799
Compare
Contributor
There was a problem hiding this comment.
Pull request overview
This PR completes the coven-cli test hermeticity sweep by removing the remaining process-env mutating unit test patterns (locks/guards/set_var/remove_var) and replacing them with dependency-injected “env snapshot” seams, while keeping public signatures and production behavior intact.
Changes:
- Deleted the crate-wide
test_envlock/guard module and refactored remaining env-dependent unit tests to use injected env snapshots instead of mutating process environment. - Introduced
AdapterEnv(harness adapter resolution) andPrivacyEnv(privacy config overrides) snapshots + helpers to keep resolution/override logic env-free and testable. - Fixed an env-sensitive API test by switching from a real adapter id (
hermes) to a synthetic unknown harness id.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| crates/coven-cli/src/test_env.rs | Removed shared env-mutation coordination utilities now made unnecessary by DI-based tests. |
| crates/coven-cli/src/privacy.rs | Added PrivacyEnv snapshot + apply_env_overrides; refactored config loading/tests to avoid process env mutation and added retention override coverage. |
| crates/coven-cli/src/paths.rs | Moved the USERPROFILE fallback coverage onto the injectable coven_home_from_env seam. |
| crates/coven-cli/src/main.rs | Dropped the last env-mutating coven_home fallback test and removed the test_env module reference. |
| crates/coven-cli/src/harness.rs | Added AdapterEnv snapshot + env-free command construction seam; refactored tests to inject specs/env/program resolution and removed env locks/guards. |
| crates/coven-cli/src/api.rs | Made the unknown-harness launch test hermetic by using a synthetic harness id rather than a real installable adapter id. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
BunsDev
marked this pull request as ready for review
July 24, 2026 10:13
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Follow-up to #440, completing the sweep: zero
env_lock/EnvVarGuard/set_var/remove_varremain incoven-cli(grep -rnclean). All conversions use dependency injection; public signatures and production behavior are unchanged.harness.rs (~45 lock-guarded tests → lock-free)
AdapterEnvsnapshot (COVEN_HOME,COVEN_HARNESS_ADAPTER_MANIFEST,COVEN_HARNESS_ADAPTER_DIRS) taken once per resolution and threaded throughexternal_adapter_manifest_sources/external_harness_specs/ configured-spec helpers; pub fns are thinfrom_process()wrappers.command_parts_with_specs(specs, …, codex_json, claude_bypass_enabled, resolve_program)holds the logic; the inner wrapper reads process env. Dropped the now-redundantwith_claude_permission_flagsvariant.harness_available_with(executable, engine_resolves)seam; engine test injectsengine::resolve_from.AdapterEnv, and an identity program resolver — side benefit:program ==asserts no longer depend on hostPATH(notably flaky-prone on Windows).privacy.rs
PrivacyEnvsnapshot + singleapply_env_overrides, deduplicating the copy-pasted override block while preserving the original file → env → settings → env precedence. Added retention-days override coverage (positive, zero, non-numeric).main.rs → paths.rs
USERPROFILEfallback test onto the already-injectablecoven_home_from_envseam; deleted the lastenv_lock/EnvVarGuardhelpers frommain.rs.api.rs (latent env-dependent failure found by this sweep)
launch_request_with_unknown_harness_returns_400_upfront_no_session_rowusedhermes— a real installable adapter — so on any machine with local adapter recipes (~/.coven/adapters) the launch succeeded (201 ≠ 400). Reproduced on currentmain; CI only passes because runners are bare. Switched to a synthetic id so the test is hermetic everywhere.Rebase over #443/#444 (post-fork main)
test_env.rslock is deleted and fix: make unknown-harness tests hermetic against ambient adapter env #444's guards in the api.rs unknown-harness test give way to this branch's synthetic harness id.hermes_recipe_forwards_selected_models_before_the_shim_promptis converted to the injectedAdapterEnv+command_parts_in_specsseams (lock-free); fix(hermes): forward models through adapter recipe #443'shermes-covenrecipe/manifest changes are preserved as-is.Verification
cargo fmt --check✅cargo clippy --workspace --all-targets --locked -- -D warnings✅ (0 warnings)cargo test --workspace --locked✅ 1269 passed / 0 failedpython scripts/check-secrets.py✅Readiness