Retire EnvLock rather than restore its synchronisation unit tests

[coderabbitai]

Rescoped 2026-08-01

This issue originally asked to restore unit tests covering ensure_env_lock() idempotency and Drop-based CWD restoration, removed in #273. That work is no longer wanted: AGENTS.md now forbids in-process environment mutation in tests and explicitly disqualifies process-wide locks as a fallback. Adding coverage for EnvLock would entrench the mechanism being retired.

The original text is preserved below for the record.

Required work

Retire EnvLock rather than test it.

• test_support/src/env_lock.rs is a static ENV_LOCK: Mutex<()> plus a thread-local re-entrancy depth counter. Its purpose is to serialize std::env::set_var and std::env::set_current_dir calls across the suite.
• The correct replacement is an injected mockable::Env in production signatures, so no test needs to mutate global state and no lock is required.
• Removal is tracked in Retire EnvLock and the env mutation guards from test_support #494; the callers that must migrate first are tracked in Migrate manifest workspace unit tests off EnvLock and EnvVarGuard #491, Migrate rstest-bdd scenarios off in-process environment and CWD mutation #492, and Migrate the last EnvLock users onto injected seams (env_path_tests + manifest workspace tests) #493.

This issue now stands for the decision — that EnvLock is retired rather than hardened — and closes when #494 closes.

Why the synchronisation properties no longer need coverage

The two properties named in the original issue are properties of the lock, not of Netsuke. Once the lock has no callers, ensure_env_lock idempotency is vacuous and CWD restoration is handled by the scenarios that no longer change the working directory at all (#492).

---

Original issue text

Summary

PR #273 removed the #[cfg(test)] mod tests block from tests/bdd/fixtures/mod.rs that previously validated ensure_env_lock() and Drop CWD restoration behaviour. This leaves the EnvLock guard untested at the unit level, which is a risk given that BDD tests rely on it for process-wide environment isolation.

Context

The removed block validated:

• ensure_env_lock() is idempotent when the lock is already held
• The Drop implementation restores the original working directory

These are synchronisation correctness properties and should be covered by dedicated unit tests.

Proposed work

• Restore the ensure_env_lock idempotency test
• Restore the Drop-based CWD restoration test
• Adjust both tests to match the updated ensure_env_lock and restore_environment_locked semantics introduced in PR Complete CLI config struct implementation with OrthoConfig (0.8.0) (3.11.1) #273

Backlinks

• PR: Complete CLI config struct implementation with OrthoConfig (0.8.0) (3.11.1) #273
• Requested by @leynos

[coderabbitai]

🔗 Related PRs

leynos/whitaker#235 - Implement rstest helper call-site collection (8.2.2) [merged]
#327 - Verify config precedence invariant (#291) [merged]
leynos/pg-embed-setup-unpriv#157 - Add verification tests for lifecycle invariants (#152) [merged]
leynos/mdtablefix#393 - Document LazyLock race-safety coverage (#342) [merged]
leynos/pg-embed-setup-unpriv#186 - Restore dev-worker coverage ratchet with unprivileged unit tests [merged]

---

📝 Issue Planner

_{Check the box below or use the @coderabbitai plan command to generate an implementation plan and prompts that you can use with your favorite coding assistant.}

---

🧪 Issue enrichment is currently in open beta.

You can configure auto-planning by selecting labels in the issue_enrichment configuration.

To disable automatic issue enrichment, add the following to your .coderabbit.yaml:

issue_enrichment:
  auto_enrich:
    enabled: false

💬 Have feedback or questions? Drop into our discord!

[leynos]

Tracked under #496.

[leynos]

@CodeRabbit plan

[coderabbitai]

Coding Plan

Summary

Treat this ticket as a decision record. Record the retirement in ADR-008 by in-place amendment, per the house style.

Propagate the decision to AGENTS.md, the module doc in env_lock.rs, the developers' guide, and the crate doc, so no new callers or tests are added.

Do not restore the deleted test block, add EnvLock tests, or remove EnvLock. Delegate removal to #494 and migrations to #491-#493, and give the removal a ready follow-up prompt.

Design Choices

Design Choice 1: Record the decision in ADR-008 or create a new ADR?

Options Considered:

1. Amend ADR-008 in place with a dated retirement note.
2. Create a new dedicated retirement ADR (for example ADR-014).

Chosen Option: 1

Rationale: ADR-008 already discusses EnvLock's narrowed role, and the house style is in-place amendment, so extending ADR-008 keeps one authoritative record.

Design Choice 2: What is the concrete deliverable, given the ticket closes when `#494` closes?

Options Considered:

1. Treat this ticket as a documentation-and-decision deliverable only.
2. Perform the EnvLock removal and caller migration here.

Chosen Option: 1

Rationale: The ticket explicitly delegates removal to #494 and migrations to #491-#493, so the scope here is the decision record and its propagation.

💡 User Tips

Regenerate the plan with different choices with @coderabbitai <feedback>.

Implementation Steps

Phase 1: Record the retirement decision

Record the architectural decision that EnvLock is retired rather than hardened. Establish one authoritative source, then align the governing policy document with it.

Task 1: Amend ADR-008 to record the retirement decision

Extend the existing environment-seam ADR to state the decision and its rationale.

• Update docs/adr-008-environment-seam-taxonomy.md.
• Amend ## Status with a dated sentence in the existing house style, for example "Updated on <date> to record the retirement of EnvLock". Follow the precedent in docs/adr-002-* and docs/adr-004-*.
• Bump ## Date to the amendment date, per the ADR-004 precedent.
• Extend the decision and consequences narrative to state four points: EnvLock is retired rather than hardened; the replacement is an injected mockable::Env seam in production signatures; removal is tracked in #494; the callers that must migrate first are tracked in #491, #492, and #493.
• State explicitly that synchronisation unit tests for EnvLock will not be added, and give the reason: the two named properties are properties of the lock, not of Netsuke, and become vacuous once the lock has no callers.
• Cross-reference AGENTS.md and docs/reliable-testing-in-rust-via-dependency-injection.md as the target pattern.

Task 2: Align AGENTS.md with the recorded decision

Resolve the tension between the absolute policy wording and the surviving guard.

• Update AGENTS.md.
• Add a short cross-reference in the testing section that names EnvLock as a retiring exception, tracked for removal in #494, and points to ADR-008.
• Do not weaken the existing rules. The rules that forbid in-process environment mutation and disqualify process-wide locks stay unchanged.

🤖 Prompt for AI agents

Implement Phase 1: record the architectural decision that EnvLock is retired
rather than hardened, and align the governing policy document with that
decision.

Task 1.1 - Amend ADR-008:
- Update `docs/adr-008-environment-seam-taxonomy.md`.
- Amend the `## Status` section with a dated sentence in the existing house
style, for example "Updated on `<date>` to record the retirement of `EnvLock`".
Follow the precedent set in `docs/adr-002-*` and `docs/adr-004-*`.
- Bump the `## Date` field to the amendment date, per the ADR-004 precedent.
- Extend the decision and consequences narrative to state four points: `EnvLock`
is retired rather than hardened; the replacement is an injected `mockable::Env`
seam in production signatures; removal is tracked in issue `#494`; the callers
that must migrate first are tracked in issues `#491`, `#492`, and `#493`.
- State explicitly that synchronisation unit tests for `EnvLock` will not be
added. Give the reason: the two named properties are properties of the lock, not
of Netsuke, and become vacuous once the lock has no callers.
- Cross-reference `AGENTS.md` and
`docs/reliable-testing-in-rust-via-dependency-injection.md` as the target
pattern.

Task 1.2 - Align AGENTS.md:
- Update `AGENTS.md`.
- Add a short cross-reference in the testing section that names `EnvLock` as a
retiring exception, tracked for removal in issue `#494`, and points to ADR-008.
- Keep every existing rule intact. Do not weaken the rules that forbid
in-process environment mutation and that disqualify process-wide locks.

Phase 2: Propagate the decision to code-adjacent documentation

Make the decision visible where developers first meet the guard, so no new callers or tests are added before removal.

Task 1: Mark the retirement in the module documentation

State the retirement at the point of definition.

• Update the module-level //! doc comment in test_support/src/env_lock.rs.
• State that EnvLock is retired, is retained only until its remaining callers migrate, and is scheduled for removal under #494.
• Instruct readers to add no new callers and no new tests, and point to ADR-008.
• Do not restore or add the removed #[cfg(test)] mod tests block. Leave the existing implementation and its current test module unchanged.

Task 2: Note the retirement in the developers' guide

Keep the narrative guide consistent with the decision.

• Update the ### EnvLock, ### CwdGuard, and ### Ordering rules sections in docs/developers-guide.md.
• Add a note that EnvLock is retired and that no new callers or tests should be added, with a pointer to ADR-008.
• Keep the existing usage and ordering guidance for the current CWD-only callers until they migrate.

Task 3: Fill the crate-level documentation gap

Name the module in the crate summary and mark it as retiring.

• Update the crate-level doc comment in test_support/src/lib.rs.
• Add env_lock/EnvLock to the module bullet summary, and mark it as a retiring legacy seam that points to ADR-008.

🤖 Prompt for AI agents

Implement Phase 2: propagate the retirement decision to code-adjacent
documentation, so developers see the decision at the point where they first meet
the guard, and no new callers or tests get added before removal.

Task 2.1 - Module documentation:
- Update the module-level `//!` doc comment in `test_support/src/env_lock.rs`.
- State that `EnvLock` is retired, is retained only until its remaining callers
migrate, and is scheduled for removal under issue `#494`.
- Instruct readers to add no new callers and no new tests. Point readers to
ADR-008.
- Do not restore or add the removed `#[cfg(test)] mod tests` block. Leave the
existing implementation and its current test module unchanged.

Task 2.2 - Developers' guide:
- Update the `### EnvLock`, `### CwdGuard`, and `### Ordering rules` sections in
`docs/developers-guide.md`.
- Add a note that `EnvLock` is retired and that no new callers or tests should
be added. Point to ADR-008.
- Keep the existing usage and ordering guidance for the current CWD-only callers
until they migrate.

Task 2.3 - Crate-level documentation:
- Update the crate-level doc comment in `test_support/src/lib.rs`.
- Add `env_lock`/`EnvLock` to the module bullet summary.
- Mark it as a retiring legacy seam that points to ADR-008.

Phase 3: Track the decision in planning documents

Give the retirement a tracked home so the dependent work is discoverable.

Add a roadmap entry for the retirement

Record the decision and its dependency chain.

• Update docs/roadmap.md.
• Add an entry that records the EnvLock retirement decision, names the replacement (injected mockable::Env), and lists the dependency chain: migrations #491, #492, #493, then removal #494.

🤖 Prompt for AI agents

Implement Phase 3: give the retirement decision a tracked home in planning
documents, so the dependent work is discoverable.

Task 3.1 - Roadmap entry:
- Update `docs/roadmap.md`.
- Add an entry that records the `EnvLock` retirement decision.
- Name the replacement: the injected `mockable::Env` seam.
- List the dependency chain in order: migrations tracked in issues `#491`, `#492`,
and `#493`, followed by removal tracked in issue `#494`.

Research

EnvLock lives in test_support/src/env_lock.rs. It is a static ENV_LOCK: Mutex<()> plus a thread-local re-entrancy depth counter, wrapped in a RAII guard. It serialises real process state mutation (std::env::set_current_dir) across the test suite. It is paired with CwdGuard in test_support/src/cwd_guard.rs.

Live callers remain in src/manifest/tests/workspace.rs, src/manifest/glob/tests/capability.rs, tests/manifest_glob_tests/capability_scope.rs, and tests/bdd/fixtures/mod.rs (via GlobalStateGuard/ensure_global_state_lock).

The sanctioned replacement, an injected mockable::Env seam (DefaultEnv/MockEnv), is already adopted across src/ and test_support/.

AGENTS.md forbids in-process environment mutation and disqualifies process-wide locks. ADR-008 (docs/adr-008-environment-seam-taxonomy.md) documents the environment-seam taxonomy but does not yet record an EnvLock retirement decision.

No planning document references #491-#494. The removed #[cfg(test)] mod tests block in tests/bdd/fixtures/mod.rs is absent, so "do not restore tests" is already satisfied.

---

🚀 Next Steps

🤖 All AI agent prompts combined

Task: 1

Implement Phase 1: record the architectural decision that EnvLock is retired
rather than hardened, and align the governing policy document with that
decision.

Task 1.1 - Amend ADR-008:
- Update `docs/adr-008-environment-seam-taxonomy.md`.
- Amend the `## Status` section with a dated sentence in the existing house
style, for example "Updated on `<date>` to record the retirement of `EnvLock`".
Follow the precedent set in `docs/adr-002-*` and `docs/adr-004-*`.
- Bump the `## Date` field to the amendment date, per the ADR-004 precedent.
- Extend the decision and consequences narrative to state four points: `EnvLock`
is retired rather than hardened; the replacement is an injected `mockable::Env`
seam in production signatures; removal is tracked in issue `#494`; the callers
that must migrate first are tracked in issues `#491`, `#492`, and `#493`.
- State explicitly that synchronisation unit tests for `EnvLock` will not be
added. Give the reason: the two named properties are properties of the lock, not
of Netsuke, and become vacuous once the lock has no callers.
- Cross-reference `AGENTS.md` and
`docs/reliable-testing-in-rust-via-dependency-injection.md` as the target
pattern.

Task 1.2 - Align AGENTS.md:
- Update `AGENTS.md`.
- Add a short cross-reference in the testing section that names `EnvLock` as a
retiring exception, tracked for removal in issue `#494`, and points to ADR-008.
- Keep every existing rule intact. Do not weaken the rules that forbid
in-process environment mutation and that disqualify process-wide locks.
===============================================================================

Task: 2

Implement Phase 2: propagate the retirement decision to code-adjacent
documentation, so developers see the decision at the point where they first meet
the guard, and no new callers or tests get added before removal.

Task 2.1 - Module documentation:
- Update the module-level `//!` doc comment in `test_support/src/env_lock.rs`.
- State that `EnvLock` is retired, is retained only until its remaining callers
migrate, and is scheduled for removal under issue `#494`.
- Instruct readers to add no new callers and no new tests. Point readers to
ADR-008.
- Do not restore or add the removed `#[cfg(test)] mod tests` block. Leave the
existing implementation and its current test module unchanged.

Task 2.2 - Developers' guide:
- Update the `### EnvLock`, `### CwdGuard`, and `### Ordering rules` sections in
`docs/developers-guide.md`.
- Add a note that `EnvLock` is retired and that no new callers or tests should
be added. Point to ADR-008.
- Keep the existing usage and ordering guidance for the current CWD-only callers
until they migrate.

Task 2.3 - Crate-level documentation:
- Update the crate-level doc comment in `test_support/src/lib.rs`.
- Add `env_lock`/`EnvLock` to the module bullet summary.
- Mark it as a retiring legacy seam that points to ADR-008.
===============================================================================

Task: 3

Implement Phase 3: give the retirement decision a tracked home in planning
documents, so the dependent work is discoverable.

Task 3.1 - Roadmap entry:
- Update `docs/roadmap.md`.
- Add an entry that records the `EnvLock` retirement decision.
- Name the replacement: the injected `mockable::Env` seam.
- List the dependency chain in order: migrations tracked in issues `#491`, `#492`,
and `#493`, followed by removal tracked in issue `#494`.

💡 Iterate on the plan with: `@coderabbitai` <feedback>

Example Feedback
- `@coderabbitai` You can skip phase 3. Add a simple unit test case for phase 2.
- `@coderabbitai` For design choice 1 go ahead with option 3 and replan.

---