Retire EnvLock and the env-mutation guards from test_support (#494)

[leynos]

Summary

Closes #494

Retire the environment-mutation machinery in test_support and add a hard gate so the pattern cannot be reintroduced. All production seams now accept injected base-directory/environment data instead of reading ambient process state; the real CWD is read only at the command-line composition boundary.

Changes

Phase 1 — explicit base-directory seams

• src/manifest/workspace.rs: resolve_absolute_workspace_root now takes an explicit base directory; open_manifest_workspace and wrappers thread it through. No more internal std::env::current_dir().
• src/manifest/glob/{mod.rs,walk.rs}: expand_glob, glob_paths, open_root_dir, open_literal_prefix accept an explicit base for relative literal prefixes. The Dir::open_ambient_dir(".", ...) call is removed.
• src/manifest/mod.rs / query.rs: captures the already-resolved ManifestWorkspace.root in the glob() Jinja closure and threads it into expand_glob.
• Follows ADR-008: capture CWD as data at one composition boundary.

Phase 2 — test migration

All manifest, glob, and BDD tests now pass explicit base directories instead of mutating CWD. GlobalStateGuard/ensure_global_state_lock and their EnvLock/CwdGuard usage are gone; project_scope_file(directory: Option<&Path>) is used for configuration discovery.

Phase 3 — deletion + audit

• Deleted: test_support/src/env_lock.rs, test_support/src/cwd_guard.rs (previously env_guard.rs, env_var_guard.rs, path_guard.rs were already removed).
• test_support/src/env.rs now holds only the pure helpers prepend_path_value and write_manifest.
• Audit: test_support/src/http/mod.rs duration_from_env/from_env_provider read through the mockable::Env seam (env.raw(...)), not std::env::var — confirmed, no change needed.

Phase 4 — enforcement gate (demonstrated to fail)

make lint now runs lint-env-mutation first. The grep gate (scripts/check-env-mutation.sh) rejects std::env::set_var, std::env::remove_var, and std::env::set_current_dir under src/, tests/, and test_support/, matching only the full std::env:: path so Command::env/env_clear/current_dir stay allowed. Both clippy.toml and test_support/clippy.toml gain the set_current_dir disallowed-method entry in lockstep.

Deliberate-violation proof — a temporary tests/env_mutation_gate_proof.rs containing let _ = std::env::set_current_dir("/tmp"); produced:

<local>/tests/env_mutation_gate_proof.rs:3:    let _ = std::env::set_current_dir("/tmp");
error: in-process environment mutation is forbidden (see AGENTS.md testing mandate)
make: *** [Makefile:101: lint-env-mutation] Error 1

and independently via clippy disallowed-methods:

error: use of a disallowed method `std::env::set_current_dir`
 --> tests/env_mutation_gate_proof.rs:3:13
  = note: inject a base-directory seam; confine CWD changes to Command::current_dir

The temporary file was removed and the tree left clean.

Validation

• make check-fmt ✓ (exit 0)
• make lint ✓ (exit 0) — includes lint-env-mutation, clippy -D warnings, and Whitaker
• make test ✓ (exit 0) — suite + doctests green (30 passed, 6 ignored in test_support)
• CodeRabbit --agent review: 0 findings across 28 reviewed files

References

• Issue: Retire EnvLock and the env mutation guards from test_support #494
• Lody session: https://lody.ai/leynos/sessions/c6635e5f-d3f4-4f7c-839d-7e1f69d172b7

Summary by Sourcery

Eliminate ambient process-state mutation from manifest and test infrastructure by injecting base-directory data and enforcing the policy with lint and test gates.

Bug Fixes:

• Anchor manifest glob expansion to the manifest workspace root so relative patterns resolve consistently without depending on the process working directory.
• Align explicit configuration selectors with the -C/--directory contract while preserving absolute-path behavior and unanchored resolution when no directory is supplied.

Enhancements:

• Replace ambient working-directory access in manifest and glob resolution with explicit base-directory seams.
• Retire EnvLock, CwdGuard, and related environment-mutation test utilities in favor of injected environment and directory data.

Build:

• Add a source scan that rejects in-process environment and working-directory mutation under production and test trees.
• Extend lint and test targets to run the environment-mutation gate and its disposable fixture tests.

Documentation:

• Document the base-directory seam, glob anchoring, explicit-selector behavior, and prohibition on process-global environment mutation.

Tests:

• Migrate manifest, glob, BDD, and configuration-discovery tests away from process working-directory mutation.
• Add unit, property-based, end-to-end, and Clippy UI coverage for base-directory behavior and the environment-mutation policy.

Chores:

• Remove obsolete environment-locking and working-directory guard modules from test_support.

References

• Issue: Retire EnvLock and the env mutation guards from test_support #494

• Lody session: https://lody.ai/leynos/sessions/25818303-60fe-4eee-b1ac-cffbd2f3a196

[coderabbitai]

Warning

Your free Security trial is over. An organization admin can activate billing to continue.

[coderabbitai]

Warning

Your free Security trial is over. An organization admin can activate billing to continue.

[sourcery-ai]

Reviewer's Guide

Retires test-support environment and CWD mutation utilities by introducing explicit base-directory seams in manifest/glob code, updating tests to use injected bases, and enforcing a new lint/grep gate that forbids in-process environment mutation.

Sequence diagram for manifest glob expansion with an injected workspace root

sequenceDiagram
    participant CLI as CLI composition boundary
    participant Query as Manifest query
    participant Manifest as Manifest renderer
    participant Glob as Glob expansion
    participant FS as Filesystem

    CLI->>Query: open_manifest_workspace(path, base)
    Query->>Manifest: from_str_named(manifest_root)
    Manifest->>Glob: expand_glob(pattern, manifest_root)
    Glob->>FS: glob_with(base.join(pattern))
    FS-->>Glob: matched paths
    Glob-->>Manifest: pattern-relative paths
    Manifest-->>CLI: rendered manifest result

Loading

File-Level Changes

Change	Details	Files
Introduce explicit base-directory seams for manifests and glob expansion so callers inject roots instead of relying on process CWD.	Resolve manifest workspace roots using an optional injected base path, keeping ambient current_dir only as a fallback. Anchor relative glob patterns to an optional injected base directory and strip that base from returned matches to preserve pattern-relative spellings. Adapt glob capability root opening to take normalized pattern strings and an injected base instead of reading the current directory. Thread an optional manifest workspace root into the Jinja glob helper so manifest glob patterns resolve against the workspace root. Update CLI discovery to resolve explicit relative config paths against the CLI-provided working directory flag.	src/manifest/workspace.rs src/manifest/glob/mod.rs src/manifest/glob/walk.rs src/manifest/mod.rs src/manifest/parse_with_config.rs src/manifest/query.rs src/cli/discovery.rs
Refactor tests to use explicit base directories and project-scoped file helpers instead of mutating process CWD or environment state.	Update manifest and glob unit tests to pass explicit base directories into the new seams and stop using CwdGuard/EnvLock. Simplify manifest workspace tests by passing optional base paths rather than changing the process working directory. Adjust BDD steps for configuration discovery and manifest compilation to rely on absolute paths and CLI directory configuration instead of CWD mutation. Change glob-related test data manifests so glob patterns are workspace-relative instead of referencing tests/data prefixes.	src/manifest/glob/tests/capability.rs src/manifest/glob/tests/diagnostics.rs src/manifest/glob/tests/expansion.rs src/manifest/tests/workspace.rs tests/bdd/fixtures/mod.rs tests/bdd/steps/configuration_discovery.rs tests/bdd/steps/ir.rs tests/bdd/steps/manifest/mod.rs tests/manifest_glob_tests/capability_scope.rs tests/data/glob.yml tests/data/glob_windows.yml
Delete the environment-locking and CWD-guard infrastructure from test_support and confine env helpers to pure utilities.	Remove env_lock and cwd_guard modules and their re-exports from the test_support crate. Trim test_support::env down to pure helpers without any environment mutation machinery. Clean up localizer tests to no longer reference env_lock recovery semantics. Update env-related test documentation to reflect the absence of process-global env and CWD coordination.	test_support/src/env_lock.rs test_support/src/cwd_guard.rs test_support/src/lib.rs test_support/src/env.rs test_support/src/localizer.rs tests/env_path_tests.rs
Add a hard enforcement gate that forbids in-process environment mutation across src, tests, and test_support.	Introduce a lint-env-mutation Makefile target that runs first in the lint pipeline. Add a shell script that greps for std::env::set_var, std::env::remove_var, and std::env::set_current_dir in Rust sources and fails on matches. Disallow std::env::set_current_dir via Clippy disallowed-methods in both the main crate and test_support configuration. Document glob behaviour to be manifest-root-relative to align with the new seams.	Makefile scripts/check-env-mutation.sh clippy.toml test_support/clippy.toml docs/users-guide.md

Assessment against linked issues

Issue	Objective	Addressed	Explanation
#494	Remove the remaining environment-mutation machinery from test_support, including EnvLock, CwdGuard, and the mutating helpers in env.rs, while migrating callers to explicit seams or pure data composition.	✅
#494	Audit environment access in test_support and production code so environment and working-directory behavior use injected seams or command-builder configuration rather than in-process global mutation.	✅
#494	Add and wire an enforcement gate into make lint that rejects std::env::set_var, std::env::remove_var, and std::env::set_current_dir under src/, tests/, and test_support/, with the gate's failure behavior demonstrated.	✅

Possibly linked issues

• Retire EnvLock and the env mutation guards from test_support #494: PR Retire EnvLock and the env mutation guards from test_support #494 directly retires the specified machinery and adds the requested lint gate and validation.
• Retire EnvLock and the env mutation guards from test_support #494: Directly implements Retire EnvLock and the env mutation guards from test_support #494: removes EnvLock and mutation guards, migrates tests to injected paths, and adds lint enforcement.
• Retire EnvLock and the env mutation guards from test_support #494: The PR directly satisfies issue Retire EnvLock and the env mutation guards from test_support #494 by removing EnvLock from workspace tests and replacing global-state mutation with injected seams.

---

Tips and commands

Interacting with Sourcery

• Trigger a new review: Comment @sourcery-ai review on the pull request.
• Continue discussions: Reply directly to Sourcery's review comments.
• Generate a GitHub issue from a review comment: Ask Sourcery to create an
  issue from a review comment by replying to it. You can also reply to a
  review comment with @sourcery-ai issue to create an issue from it.
• Generate a pull request title: Write @sourcery-ai anywhere in the pull
  request title to generate a title at any time. You can also comment
  @sourcery-ai title on the pull request to (re-)generate the title at any time.
• Generate a pull request summary: Write @sourcery-ai summary anywhere in
  the pull request body to generate a PR summary at any time exactly where you
  want it. You can also comment @sourcery-ai summary on the pull request to
  (re-)generate the summary at any time.
• Generate reviewer's guide: Comment @sourcery-ai guide on the pull
  request to (re-)generate the reviewer's guide at any time.
• Resolve all Sourcery comments: Comment @sourcery-ai resolve on the
  pull request to resolve all Sourcery comments. Useful if you've already
  addressed all the comments and don't want to see them anymore.
• Dismiss all Sourcery reviews: Comment @sourcery-ai dismiss on the pull
  request to dismiss all existing Sourcery reviews. Especially useful if you
  want to start fresh with a new review - don't forget to comment
  @sourcery-ai review to trigger a new review!

Customizing Your Experience

Access your dashboard to:

• Enable or disable review features such as the Sourcery-generated pull request
  summary, the reviewer's guide, and others.
• Change the review language.
• Add, remove or edit custom review instructions.
• Adjust other review settings.

Getting Help

• Contact our support team for questions or feedback.
• Visit our documentation for detailed guides and information.
• Keep in touch with the Sourcery team by following us on X/Twitter, LinkedIn or GitHub.

[coderabbitai]

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

• @coderabbitai resume to resume automatic reviews.
• @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

• ▶️ Resume reviews
• 🔍 Trigger review

Important

Approval pending

CodeRabbit has no unresolved comments, but it has not reviewed the latest commit.

Use the checkbox below to review the latest commit. CodeRabbit will approve the changes if it finds no blocking issues.

• 🔍 Trigger review

Summary

• Remove EnvLock and CwdGuard from test_support.
• Resolve manifest globs through explicit base-directory seams instead of process-global state.
• Anchor manifest-relative glob paths to the manifest workspace.
• Update configuration discovery and documentation for --directory and relative --config paths.
• Add lint-env-mutation to make lint to reject direct environment and working-directory mutation.
• Restrict std::env::set_current_dir through Clippy configuration.
• Migrate affected tests and update glob fixtures.

Documentation

• Update the developer guide with environment-isolation guidance.
• Align the user guide and design document with the configuration and glob-resolution behaviour.

Tests

• Add discovery tests for explicit configuration selectors.
• Add base-directory and symlink coverage for glob expansion.
• Preserve capability, diagnostic, syntax, and path-handling coverage after the API changes.

Walkthrough

Changes

Path resolution and mutation control

Layer / File(s)	Summary
Environment mutation enforcement Makefile, clippy.toml, test_support/clippy.toml, scripts/check-env-mutation.sh	The lint target scans Rust sources for forbidden in-process environment and working-directory mutations before running existing checks.
CLI configuration path resolution src/cli/discovery.rs, src/cli/discovery_layer_selector_tests.rs, docs/netsuke-design.md, docs/users-guide.md	Configuration selector documentation and tests cover explicit selector resolution and directory discovery behaviour.
Manifest roots and glob bases src/manifest/mod.rs, src/manifest/query.rs, src/manifest/parse_with_config.rs, src/manifest/glob/*, docs/users-guide.md	Manifest parsing passes an optional root to glob expansion. Glob preparation and traversal use injected bases and restore relative result paths.
Test migration to explicit bases src/manifest/glob/tests/*, tests/manifest_glob_tests/*, tests/data/*, test_support/src/*, docs/developers-guide.md, tests/bdd/steps/*, tests/env_path_tests.rs	Tests and guidance remove process-global directory and environment guards and use explicit directory seams instead.

Suggested labels: Issue

Poem

Anchor each path where patterns grow
Scan mutations before checks flow
Let manifests carry their base
Keep test state in its proper place
Build clean seams for every trace

Merge Risk: 🟡 Moderate · up to 37d9e

The PR replaces process-wide directory and environment mutation with explicit path inputs and adds enforcement, but the current head still has a likely Windows build failure in the new tests and a way to bypass the mutation lint gate by creating a matching filesystem entry. These issues should be fixed before merging.

---

Caution

Pre-merge checks failed

Please resolve all errors before merging. Addressing warnings is optional.

• Ignore

❌ Failed checks (4 errors, 7 warnings)

Check name	Status	Explanation	Resolution
Testing (Overall)	❌ Error	FAIL — The new environment-mutation gate has no durable behavioural test. scripts/check-env-mutation.sh rejects three forbidden calls, and Makefile adds it to lint, but repository tests contain …	Add committed tests for the new enforcement behaviour. Exercise scripts/check-env-mutation.sh with isolated fixtures containing each of std::env::set_var, std::env::remove_var, and std::env::set_current_dir, and assert that each fai…
Unit Architecture	❌ Error	Fail: PreparedGlob::new hides fallible filesystem work in a query path. The changed code calls dir.canonicalize_utf8() at src/manifest/glob/mod.rs:320-323, then discards every error with `unwrap…	Separate text preparation from base resolution. Resolve the base only for a relative pattern in a small, explicitly fallible helper. Return and propagate canonicalisation errors with the existing glob error context, or define and return a t…
Security And Privacy	❌ Error	Fix the injected-base glob construction before merging. src/manifest/glob/mod.rs:320-330 now builds dir.join(normalized) and passes that string to glob_with without escaping metacharacters in `d…	Escape every metacharacter in every injected base component before concatenating it with the normalized glob pattern, using the glob crate's literal-escaping rules for the target platform. Preserve separators and roots, and keep the escap…
Rust Compiler Lint Integrity	❌ Error	The PR introduces cross-platform compiler-lint failures in test imports. In src/manifest/glob/tests/capability.rs, it adds #[cfg(unix)] to the anyhow import although unconditionally compiled tes…	Restore the unconditional anyhow::{Context, Result, anyhow, ensure} import in src/manifest/glob/tests/capability.rs. Restore #[cfg(unix)] on the literal_dir_prefix and minijinja::ErrorKind imports. Add #[cfg(unix)] to `mod base;…
User-Facing Documentation	⚠️ Warning	The PR changes glob() so relative patterns use the manifest workspace root: the final code passes manifest_root to expand_glob, joins relative patterns to that base, and strips the base from res…	Replace the stale working-directory wording in docs/users-guide.md with an unambiguous rule: relative patterns, including parent-relative patterns, resolve from the manifest directory; absolute patterns remain absolute. Add the manifest-r…
Developer Documentation	⚠️ Warning	Fail this check. The PR introduces a new glob_paths(pattern, base: Option<&Utf8Path>) API and threads ManifestWorkspace.root into expand_glob, but docs/developers-guide.md does not document th…	Update docs/developers-guide.md to document the new base-directory contract and ownership: glob_paths and expand_glob accept Option<&Utf8Path>, relative patterns use the injected base and strip it from results, absolute patterns are…
Testing (Unit And Behavioural)	⚠️ Warning	Fail the testing check because the new enforcement behaviour has no durable test coverage. The PR adds Makefile:118-121, scripts/check-env-mutation.sh, and set_current_dir Clippy entries, but re…	Add a committed test harness for scripts/check-env-mutation.sh. Run it against temporary src/, tests/, and test_support/ fixtures. Verify that std::env::set_var, remove_var, and set_current_dir each fail, that Command::env, …
Testing (Property / Proof)	⚠️ Warning	The PR introduces range-based invariants in PreparedGlob::new and strip_base: relative and absolute patterns, optional bases, canonicalisation fallback, rebasing, and separator normalisation must …	Add substantive proptest coverage through glob_paths or a pure extracted preparation helper. Generate relative and absolute patterns, optional relative and absolute bases, parent-relative patterns, rebasing, and platform separator forms…
Testing (Compile-Time / Ui)	⚠️ Warning	The pull request introduces compile-time Clippy behaviour: both clippy.toml and test_support/clippy.toml add std::env::set_current_dir to disallowed-methods, and Cargo.toml denies that lint.…	Add a committed Rust compile-time/UI test for the new Clippy restriction. Compile a fixture containing std::env::set_current_dir and assert that Clippy rejects it with clippy::disallowed_methods and the configured remediation reason. Co…
Performance And Resource Use	⚠️ Warning	The base-anchored glob path adds an avoidable allocation for every matched file. names_a_file already creates a String with path.as_str().replace(...) at src/manifest/glob/walk.rs:392. When a …	Refactor match handling so the owned path is stripped and separator-normalized in one final conversion, with no intermediate String from names_a_file. Resolve the manifest base once per manifest parse, reuse the resolved value for all `…
Concurrency And State	⚠️ Warning	The changed VerboseTimingReporter::report_complete no longer enforces exactly-once completion forwarding. In src/status_timing.rs, the mutex-protected state returns Vec::new() when `state.comple…	Restore an exactly-once gate around the whole completion effect: return before calling the inner reporter when the completion transition was already taken, or compute and check a should_forward flag while holding the mutex. Add a determin…

✅ Passed checks (9 passed)

Check name	Status	Explanation
Linked Issues check	✅ Passed	Accept the implementation for issue #494: it reworks or deletes the remaining mutation support, adds the required gate for forbidden std::env calls, wires the gate into make lint, demonstrates failure…
Out of Scope Changes check	✅ Passed	Keep the changes in scope: the configuration, glob, documentation, and test updates support removal of process-global environment and working-directory mutation and its regression controls.
Docstring Coverage	✅ Passed	Docstring coverage is 94.59% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 37 functions across 16 files. (4 skipped: 4…
Module-Level Documentation	✅ Passed	PASS — All Rust modules introduced or modified by the PR have module-level //! documentation. The new selector-test and glob-base modules explain their purpose, use, and relationship to discovery or…
Domain Architecture	✅ Passed	Pass the Domain Architecture check. The pull request does not change src/ast or src/ir, and the domain model continues to represent path-like manifest fields as strings. The new Utf8Path base is…
Observability	✅ Passed	PASS — the changed production path is manifest glob resolution, and the existing Jinja glob adapter still records every completed expansion at the composition boundary. `src/manifest/glob/diagnostics.…
Architectural Complexity And Maintainability	✅ Passed	Accept the architectural change. Use the explicit base-directory seam because it removes process-global CWD mutation from manifest and glob paths. Keep PreparedGlob as a private boundary because it …
Title check	✅ Passed	The title accurately describes the retirement of EnvLock and environment-mutation guards, and it references the linked issue with (#494).
Description check	✅ Passed	The description directly explains the changes, enforcement gate, migration work, validation results, and linked issue. It is clearly related to the changeset.

Full details: Linked Issues check

Explanation

Accept the implementation for issue #494: it reworks or deletes the remaining mutation support, adds the required gate for forbidden std::env calls, wires the gate into make lint, demonstrates failure on a deliberate violation, and reports passing format, lint, and test checks.

Full details: Docstring Coverage

Explanation

Docstring coverage is 94.59% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 37 functions across 16 files. (4 skipped: 4 unsupported.)

Full details: Testing (Overall)

Explanation

FAIL — The new environment-mutation gate has no durable behavioural test. scripts/check-env-mutation.sh rejects three forbidden calls, and Makefile adds it to lint, but repository tests contain no reference to the script, lint-env-mutation, or these rejection cases. The contributor's temporary manual probe does not protect against later regressions. The new selector tests also do not exercise their stated relative-path behaviour: both selectors are constructed with temp.path().join(...), so both are absolute. The existing end-to-end relative-selector tests predate this pull request. The new glob tests provide useful coverage for relative bases, parent-relative paths, and symlinked bases, but they do not compensate for the untested enforcement feature.

Resolution

Add committed tests for the new enforcement behaviour. Exercise scripts/check-env-mutation.sh with isolated fixtures containing each of std::env::set_var, std::env::remove_var, and std::env::set_current_dir, and assert that each fails. Exercise fixtures containing Command::env, Command::env_clear, and Command::current_dir, and assert that they pass. Assert that make lint depends on lint-env-mutation. Add an equivalent test for both Clippy configurations that rejects std::env::set_current_dir and accepts Command::current_dir. Replace the new selector cases with genuinely relative selectors, or remove them and rely on a clearly scoped end-to-end test that runs from a controlled original working directory. Extend glob coverage with at least an absolute-pattern-with-base case and a nested rebasing case.

Full details: User-Facing Documentation

Explanation

The PR changes glob() so relative patterns use the manifest workspace root: the final code passes manifest_root to expand_glob, joins relative patterns to that base, and strips the base from results. The user's guide documents this at lines 536–539, but the following paragraph still says that patterns can be relative to the working directory at lines 542–543. These instructions conflict, so the behaviour is not clearly documented. The existing docs/v0-1-0-migration-guide.md is unchanged and does not signpost this changed path anchor.

Resolution

Replace the stale working-directory wording in docs/users-guide.md with an unambiguous rule: relative patterns, including parent-relative patterns, resolve from the manifest directory; absolute patterns remain absolute. Add the manifest-root anchoring change to docs/v0-1-0-migration-guide.md and link to the detailed user's-guide section.

Full details: Developer Documentation

Explanation

Fail this check. The PR introduces a new glob_paths(pattern, base: Option<&Utf8Path>) API and threads ManifestWorkspace.root into expand_glob, but docs/developers-guide.md does not document this contract. Its retained glob guidance still says that open_literal_prefix opens the current directory ambiently, and the PR removes the detailed manifest workspace base-seam section. The PR also changes docs/netsuke-design.md to state that relative --config paths use -C/--directory, while the changed implementation comments, tests, and user's guide state that explicit selectors use the shell's original working directory. Finally, the unchanged ADR-008 records environment-reader seams but not the new filesystem base-directory seam or its architectural decision.

Resolution

Update docs/developers-guide.md to document the new base-directory contract and ownership: glob_paths and expand_glob accept Option<&Utf8Path>, relative patterns use the injected base and strip it from results, absolute patterns are not rebased, ManifestParse.manifest_root receives ManifestWorkspace.root, and open_root_dir receives the prepared search path without applying the base twice. Restore or replace the removed workspace-seam guidance with the current resolve_absolute_workspace_root and open_manifest_workspace rules. Correct docs/netsuke-design.md §8.4 to match the implemented and tested explicit-selector behaviour, or change the implementation and all tests and user documentation consistently. Record the filesystem base-directory and no-process-CWD-mutation decision in the relevant design document or an addendum to ADR-008, then link the records from the developer guide and remove any contradictory living documentation.

Full details: Module-Level Documentation

Explanation

PASS — All Rust modules introduced or modified by the PR have module-level //! documentation. The new selector-test and glob-base modules explain their purpose, use, and relationship to discovery or glob_paths. The modified manifest, glob, CLI, test-support, and test modules also retain clear module documentation. Deleted modules do not create a documentation failure.

Full details: Testing (Unit And Behavioural)

Explanation

Fail the testing check because the new enforcement behaviour has no durable test coverage. The PR adds Makefile:118-121, scripts/check-env-mutation.sh, and set_current_dir Clippy entries, but repository searches found no test that invokes the script or verifies its reject, allow, and error-status paths. The reported temporary proof is not a committed test. Retain the existing CLI and manifest-glob boundary tests: tests/config_discovery_e2e_tests.rs covers relative --config and NETSUKE_CONFIG with -C, and tests/manifest_glob_tests/capability_scope.rs exercises manifest-level parent-relative expansion.

Resolution

Add a committed test harness for scripts/check-env-mutation.sh. Run it against temporary src/, tests/, and test_support/ fixtures. Verify that std::env::set_var, remove_var, and set_current_dir each fail, that Command::env, env_clear, and current_dir pass, and that scan errors propagate. Add a Makefile contract assertion that lint depends on lint-env-mutation and that the target invokes the script. Add durable Clippy restriction coverage with a generated or otherwise unscanned UI fixture that rejects std::env::set_current_dir and accepts Command::current_dir.

Full details: Testing (Property / Proof)

Explanation

The PR introduces range-based invariants in PreparedGlob::new and strip_base: relative and absolute patterns, optional bases, canonicalisation fallback, rebasing, and separator normalisation must remain consistent. The PR adds only two fixed base-directory tests. The existing src/manifest/glob/tests/property.rs is unchanged from origin/main and covers literal-prefix extraction and GlobRoot::relativise, not the new base-aware glob_paths behaviour. The diff adds no proptest, bounded-model, or exhaustive proof coverage for these cases.

Resolution

Add substantive proptest coverage through glob_paths or a pure extracted preparation helper. Generate relative and absolute patterns, optional relative and absolute bases, parent-relative patterns, rebasing, and platform separator forms. Assert that matches are equivalent to matching the joined search path, that injected bases are stripped exactly once, and that absolute patterns ignore the base. Add cases for canonicalisation and fallback behaviour where filesystem setup is required.

Full details: Testing (Compile-Time / Ui)

Explanation

The pull request introduces compile-time Clippy behaviour: both clippy.toml and test_support/clippy.toml add std::env::set_current_dir to disallowed-methods, and Cargo.toml denies that lint. The diff adds no trybuild test or equivalent Clippy UI test. The checked-in UI fixtures cover unrelated API, cfg, and StubEnv contracts. The temporary violation used for the contributor's manual proof is not a committed test. Runtime glob and discovery tests do not validate this compile-time diagnostic or its allowed control case.

Resolution

Add a committed Rust compile-time/UI test for the new Clippy restriction. Compile a fixture containing std::env::set_current_dir and assert that Clippy rejects it with clippy::disallowed_methods and the configured remediation reason. Compile a control fixture using Command::current_dir and assert success. Cover both crate configurations, or invoke the same Clippy configuration through a shared harness. If trybuild cannot preserve the repository's Rust flags, use the repository's direct compiler/command harness instead. Use focused semantic assertions or a small redacted snapshot for diagnostic text, and register the test in the normal test or lint path.

Full details: Unit Architecture

Explanation

Fail: PreparedGlob::new hides fallible filesystem work in a query path. The changed code calls dir.canonicalize_utf8() at src/manifest/glob/mod.rs:320-323, then discards every error with unwrap_or_else and silently uses the original path. canonicalize_utf8 performs environmental filesystem I/O, but PreparedGlob::new documents only brace-validation errors and does not expose canonicalisation failures. glob_paths and the manifest glob() helper therefore cannot distinguish an unavailable, inaccessible, or otherwise unresolvable base from the fallback path. The new preparation unit also claims to perform pure text work while performing this I/O. The injected base and the outer Result do not correct the hidden error handling.

Resolution

Separate text preparation from base resolution. Resolve the base only for a relative pattern in a small, explicitly fallible helper. Return and propagate canonicalisation errors with the existing glob error context, or define and return a typed expected outcome for a missing base instead of catching every error. Do not canonicalize an unused base for absolute patterns. Update the API documentation to list base-resolution and filesystem failures, and add tests for missing, inaccessible, and unresolvable bases plus the absolute-pattern case.

Full details: Domain Architecture

Explanation

Pass the Domain Architecture check. The pull request does not change src/ast or src/ir, and the domain model continues to represent path-like manifest fields as strings. The new Utf8Path base is confined to the manifest glob and workspace-loading boundary, where filesystem matching already belongs; ManifestParse passes it to the Jinja glob adapter rather than storing it in the AST or IR. The change also replaces process-global CWD mutation with explicit injected data and keeps child-process directory changes on Command builders. The repository documentation and ADRs identify glob/walk as the filesystem boundary and the manifest-to-IR conversion as the path interpretation boundary.

Full details: Observability

Explanation

PASS — the changed production path is manifest glob resolution, and the existing Jinja glob adapter still records every completed expansion at the composition boundary. src/manifest/glob/diagnostics.rs emits bounded counters for matched and unopenable_prefix, a counter for skipped-entry reasons, and debug events with match counts or failure context. Paths and patterns are redacted, and labels use fixed vocabularies. Manifest and CLI failure paths retain user-facing error diagnostics, while the existing manifest template and configuration boundaries provide duration and error telemetry. The new lint gate reports a clear stderr failure but is development tooling, not a production operation. The selector changes in this PR add documentation and tests; they do not add a new runtime failure path. No new service, process, queue, retry, or network boundary requires tracing or alerting.

Full details: Security And Privacy

Explanation

Fix the injected-base glob construction before merging. src/manifest/glob/mod.rs:320-330 now builds dir.join(normalized) and passes that string to glob_with without escaping metacharacters in dir. A valid Unix workspace path such as /tmp/workspace* therefore becomes /tmp/workspace*/*.txt, and the glob matcher can select sibling directories such as /tmp/workspace-secret. literal_dir_prefix then stops at the base's *, so open_root_dir scopes metadata checks to /tmp, not the workspace. strip_base cannot strip a sibling match and returns its absolute path. This introduces a file-path injection and over-broad filesystem access path caused by the pull request. A temporary standard-library glob reproduction confirmed that the wildcard in the injected base selects the sibling directory. The new base tests cover relative and symlinked bases, but do not cover metacharacters in base components.

Resolution

Escape every metacharacter in every injected base component before concatenating it with the normalized glob pattern, using the glob crate's literal-escaping rules for the target platform. Preserve separators and roots, and keep the escaped search string separate from the canonical base used by strip_base. Add regression tests with base directory names containing *, ?, [, and {/}, plus sibling decoy directories, and assert that results contain only files below the injected base and remain relative. Verify that the capability root remains the injected base rather than its parent.

Full details: Performance And Resource Use

Explanation

The base-anchored glob path adds an avoidable allocation for every matched file. names_a_file already creates a String with path.as_str().replace(...) at src/manifest/glob/walk.rs:392. When a manifest supplies Some(workspace.root) (src/manifest/query.rs:67), expand_glob then calls strip_base, which creates another String with replace(...) at src/manifest/glob/mod.rs:355. The PR therefore adds a second full path copy per match. The comment claiming in-place replacement is not accurate because the replacement operates on a borrowed stripped view and assigns a new String. PreparedGlob::new also canonicalizes the injected base on every glob expansion, including absolute patterns that do not use the base, and does not cache this repeated filesystem work.

Resolution

Refactor match handling so the owned path is stripped and separator-normalized in one final conversion, with no intermediate String from names_a_file. Resolve the manifest base once per manifest parse, reuse the resolved value for all glob() calls, and skip base canonicalisation when the normalized pattern is absolute. Add a realistic large-directory benchmark or allocation/syscall regression test for repeated base-anchored glob expansion, then verify that the result and resource use do not regress.

Full details: Concurrency And State

Explanation

The changed VerboseTimingReporter::report_complete no longer enforces exactly-once completion forwarding. In src/status_timing.rs, the mutex-protected state returns Vec::new() when state.completed is already true, but the method still calls self.inner.report_complete(tool_key) after the lock. The base implementation returned immediately for that state. The deleted src/status_timing_lifecycle_tests.rs test explicitly called completion twice and required one delegated completion; the current test set has no equivalent duplicate-completion or interleaving test. This violates the required atomic state transition and duplicate-message behaviour. The change also removes the dedicated blocking and re-entrant timing-sink tests while retaining state shared through a mutex and changing summary output to separate io::stderr() writes.

Resolution

Restore an exactly-once gate around the whole completion effect: return before calling the inner reporter when the completion transition was already taken, or compute and check a should_forward flag while holding the mutex. Add a deterministic concurrent duplicate-completion test that asserts one inner completion and one summary. Retain equivalent tests for blocking and re-entrant output paths, and keep callbacks and blocking I/O outside the state lock.

Full details: Architectural Complexity And Maintainability

Explanation

Accept the architectural change. Use the explicit base-directory seam because it removes process-global CWD mutation from manifest and glob paths. Keep PreparedGlob as a private boundary because it separates validation/base resolution from matching and result collection, and it enforces the no-double-base invariant. Keep manifest_root in the existing ManifestParse input bundle because the manifest loader already owns that dependency. The change adds no dependency edges or third-party packages, deletes EnvLock and CwdGuard, and leaves no references to those concepts. The new lint script is a direct enforcement mechanism, not an indirection layer. The remaining additions are focused tests and module wiring.

Full details: Rust Compiler Lint Integrity

Explanation

The PR introduces cross-platform compiler-lint failures in test imports. In src/manifest/glob/tests/capability.rs, it adds #[cfg(unix)] to the anyhow import although unconditionally compiled tests use Result, Context, and ensure; it also removes #[cfg(unix)] from literal_dir_prefix and minijinja::ErrorKind, which are used only by Unix-gated tests. The new src/manifest/glob/tests/base.rs is included unconditionally by tests/mod.rs, while all its uses and tests are Unix-gated, so its imports are unused on Windows. CI compiles the Windows test tree with -D warnings, making this PR-caused import state a compiler failure. No broad unused-code suppression or artificial usage anchor was added.

Resolution

Restore the unconditional anyhow::{Context, Result, anyhow, ensure} import in src/manifest/glob/tests/capability.rs. Restore #[cfg(unix)] on the literal_dir_prefix and minijinja::ErrorKind imports. Add #[cfg(unix)] to mod base; in src/manifest/glob/tests/mod.rs so the Unix-only module and its imports are not compiled on Windows, or apply equivalent #[cfg(unix)] gates to every Unix-only import. Re-run the Windows all-targets lint with -D warnings.

✨ Finishing Touches 💡 1

🛠️ Fix failing CI checks 💡

• Create stacked PR
• Commit on current branch

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch issue-494-retire-envlock-and-the-env-mutation-guards-from-test-support

---

_{Comment @coderabbitai help to get the list of available commands.}

[sourcery-ai]

Sorry @leynos, you have reached your weekly rate limit of 500000 diff characters.

Please try again later or upgrade to continue using Sourcery

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 89ce5d9341

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[leynos]

@coderabbitai Please suggest a fix for this issue and supply a prompt for an AI coding agent to enable it to apply the fix. Include the file and symbol names indicated in the issue at the head of your response. Ensure that this is validated against the current version of the codegraph.

If further refinement to address this finding would be deleterious, please supply a clear explanatory one to two paragraph markdown message in a code block that I can paste into the CodeScene web ui's diagnostic suppression function so this diagnostic can be silenced.

src/manifest/glob/mod.rs

Comment on lines +204 to +207

pub(super) fn expand_glob(
    pattern: &str,
    base: Option<&Path>,
) -> std::result::Result<GlobExpansion, Error> {

❌ New issue: Large Method
expand_glob has 71 lines, threshold = 70

[pandalump]

@coderabbitai review

[wafflecat-df12]

@coderabbitai review

[coderabbitai]

✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

[leynos]

@coderabbitai Please suggest a fix for this issue and supply a prompt for an AI coding agent to enable it to apply the fix. Include the file and symbol names indicated in the issue at the head of your response. Ensure that this is validated against the current version of the codegraph.

If further refinement to address this finding would be deleterious, please supply a clear explanatory one to two paragraph markdown message in a code block that I can paste into the CodeScene web ui's diagnostic suppression function so this diagnostic can be silenced.

Bumpy Road Ahead

scripts/check_env_mutation.py: scan_root

What lead to degradation?

scan_root has 2 blocks with nested conditional logic. Any nesting of 2 or deeper is considered. Threshold is 2 blocks per function

Why does this problem occur?

A Bumpy Road is a function that contains multiple chunks of nested conditional logic inside the same function. The deeper the nesting and the more bumps, the lower the code health.
A bumpy code road represents a lack of encapsulation which becomes an obstacle to comprehension. In imperative languages there’s also an increased risk for feature entanglement, which leads to complex state management. CodeScene considers the following rules for the code health impact: 1) The deeper the nested conditional logic of each bump, the higher the tax on our working memory. 2) The more bumps inside a function, the more expensive it is to refactor as each bump represents a missing abstraction. 3) The larger each bump – that is, the more lines of code it spans – the harder it is to build up a mental model of the function. The nesting depth for what is considered a bump is levels of conditionals.

How to fix it?

Bumpy Road implementations indicate a lack of encapsulation. Check out the detailed description of the Bumpy Road code health issue.
A Bumpy Road often suggests that the function/method does too many things. The first refactoring step is to identify the different possible responsibilities of the function. Consider extracting those responsibilities into smaller, cohesive, and well-named functions. The EXTRACT FUNCTION refactoring is the primary response.

[leynos]

@coderabbitai Please suggest a fix for this issue and supply a prompt for an AI coding agent to enable it to apply the fix. Include the file and symbol names indicated in the issue at the head of your response. Ensure that this is validated against the current version of the codegraph.

If further refinement to address this finding would be deleterious, please supply a clear explanatory one to two paragraph markdown message in a code block that I can paste into the CodeScene web ui's diagnostic suppression function so this diagnostic can be silenced.

Deep, Nested Complexity

scripts/check_env_mutation.py: scan_root

What lead to degradation?

scan_root has a nested complexity depth of 4, threshold = 4

Why does this problem occur?

Deep nested logic means that you have control structures like if-statements or loops inside other control structures. Deep nested logic increases the cognitive load on the programmer reading the code. The human working memory has a maximum capacity of 3-4 items; beyond that threshold, we struggle with keeping things in our head. Consequently, deep nested logic has a strong correlation to defects and accounts for roughly 20% of all programming mistakes.
CodeScene measures the maximum nesting depth inside each function. The deeper the nesting, the lower the code health. The threshold for the Python language is 4 levels of nesting.

How to fix it?

Occassionally, it's possible to get rid of the nested logic by Replacing Conditionals with Guard Clauses.
Another viable strategy is to identify smaller building blocks inside the nested chunks of logic and extract those responsibilities into smaller, cohesive, and well-named functions. The EXTRACT FUNCTION refactoring explains the steps.

[leynos]

@coderabbitai Please investigate the cause of the following issue using codegraph exploration and research, identify a fix and provide an AI coding agent prompt for the fix:

error: cannot find macro `ensure` in this scope
Error:    --> src\manifest\glob\tests\capability.rs:375:5
    |
375 |     ensure!(
    |     ^^^^^^
    |
    = note: macro `crate::stdlib::time::tests::timedelta_iso8601_property::ensure` exists but is inaccessible
help: consider importing one of these macros
    |
  2 + use anyhow::ensure;
    |
  2 + use miette::ensure;
    |

error: cannot find macro `anyhow` in this scope
Error:    --> src\manifest\glob\tests\capability.rs:374:24
    |
374 |         .ok_or_else(|| anyhow!("the working directory always exists"))?;
    |                        ^^^^^^
    |
    = note: macro `crate::stdlib::time::tests::timedelta_iso8601_property::anyhow` exists but is inaccessible
    = note: `anyhow` is in scope, but it is a crate, not a macro
help: consider importing this macro
    |
  2 + use anyhow::anyhow;
    |

error: cannot find macro `ensure` in this scope
Error:    --> src\manifest\glob\tests\capability.rs:271:5
    |
271 |     ensure!(
    |     ^^^^^^
    |
    = note: macro `crate::stdlib::time::tests::timedelta_iso8601_property::ensure` exists but is inaccessible
help: consider importing one of these macros
    |
  2 + use anyhow::ensure;
    |
  2 + use miette::ensure;
    |

error: cannot find macro `ensure` in this scope
Error:    --> src\manifest\glob\tests\capability.rs:251:5
    |
251 |     ensure!(results.len() == 1, "expected one match: {results:?}");
    |     ^^^^^^
    |
    = note: macro `crate::stdlib::time::tests::timedelta_iso8601_property::ensure` exists but is inaccessible
help: consider importing one of these macros
    |
627 |       fn with_context<C, F>(self, f: F) -> Result<T, Error>
    |          ------------ the method is available for `std::result::Result<std::option::Option<manifest::glob::walk::GlobRoot>, std::io::Error>` here
    |
    = help: items from traits can only be used if the trait is in scope
help: trait `Context` which provides `with_context` is implemented but not in scope; perhaps you want to import it
    |
  2 + use anyhow::Context;
    |

error[E0599]: no method named `context` found for enum `std::result::Result<T, E>` in the current scope
Error:    --> src\manifest\glob\tests\capability.rs:191:10
    |
190 |       let root = open_root_dir(pattern.normalized(), None)
    |  ________________-
191 | |         .context("open capability root")?
    | |_________-^^^^^^^
    |
    = help: items from traits can only be used if the trait is in scope
help: the following traits which provide `context` are implemented but not in scope; perhaps you want to import one of them
    |
  2 + use anyhow::Context;
    |
  2 + use quick_error::ResultExt;
    |
help: there is a method `with_context` with a similar name
    |
191 |         .with_context("open capability root")?
    |          +++++

error[E0599]: no method named `context` found for enum `std::result::Result<T, E>` in the current scope
Error:    --> src\manifest\glob\tests\capability.rs:217:58
    |
217 |         Utf8PathBuf::try_from(temp.path().to_path_buf()).context("temp dir path is not UTF-8")?;
    |                                                          ^^^^^^^
    |
    = help: items from traits can only be used if the trait is in scope
help: the following traits which provide `context` are implemented but not in scope; perhaps you want to import one of them
    |
  2 + use anyhow::Context;
    |
  2 + use quick_error::ResultExt;
    |
help: there is a method `with_context` with a similar name
    |
217 |         Utf8PathBuf::try_from(temp.path().to_path_buf()).with_context("temp dir path is not UTF-8")?;
    |                                                          +++++

error[E0599]: no method named `context` found for enum `std::result::Result<T, E>` in the current scope
Error:    --> src\manifest\glob\tests\capability.rs:220:10
    |
219 |       let root = open_root_dir(pattern.normalized(), None)
    |  ________________-
220 | |         .context("open capability root")?
    | |_________-^^^^^^^
    |
    = help: items from traits can only be used if the trait is in scope
help: the following traits which provide `context` are implemented but not in scope; perhaps you want to import one of them
    |
  2 + use anyhow::Context;
    |
  2 + use quick_error::ResultExt;
    |
help: there is a method `with_context` with a similar name
    |
220 |         .with_context("open capability root")?
    |          +++++

error[E0599]: no method named `context` found for enum `std::result::Result<T, E>` in the current scope
Error:    --> src\manifest\glob\tests\capability.rs:373:10
    |
372 |       let root = open_root_dir(pattern.normalized(), None)
    |  ________________-
373 | |         .context("open capability root")?
    | |_________-^^^^^^^
    |
    = help: items from traits can only be used if the trait is in scope
help: the following traits which provide `context` are implemented but not in scope; perhaps you want to import one of them
    |
  2 + use anyhow::Context;
    |
  2 + use quick_error::ResultExt;
    |
help: there is a method `with_context` with a similar name
    |
373 |         .with_context("open capability root")?
    |          +++++

Some errors have detailed explanations: E0107, E0599.
For more information about an error, try `rustc --explain E0107`.
error: could not compile `netsuke-build` (lib test) due to 37 previous errors
warning: build failed, waiting for other jobs to finish...
make: *** [Makefile:127: lint-clippy] Error 101

https://github.com/leynos/netsuke/actions/runs/33194269118/job/98927282835?pr=583

[codescene-access]

No quality gates enabled for this code.