Give the workspace fallback switch an injectable env seam (#487)

docs/adr-008-environment-seam-taxonomy.md

@@ -0,0 +1,199 @@
+# Architecture decision record (ADR): Environment seam taxonomy
+
+## Status
+
+Accepted.
+
+## Date
+
+2026-08-06
+
+## Context and problem statement
+
+`clippy.toml` disallows `std::env::var`, `var_os`, `set_var`, `remove_var`,
+`vars`, and `vars_os` across the workspace, per the testing mandate in
+`AGENTS.md`: behaviour that depends on an environment variable should accept
+that value as an argument rather than read the process directly. Several
+callers have satisfied that mandate with different shapes — a bare closure
+parameter, a mockable trait object, a shared `Arc`-wrapped reader — chosen
+independently as each site migrated off the disallowed calls under issues 484,
+488, and 493. Without a stated taxonomy, a future migration has no way to pick
+the right shape for a new boundary, and a reviewer lacks a yardstick for
+whether a proposed seam is over- or under-engineered for its call-site count.
+
+Netsuke's design record (`docs/netsuke-design.md`) does not describe these
+seams. This ADR fills that gap and gives `docs/developers-guide.md`'s
+"Environment and template ports", "Environment lookup seams", and "Manifest
+`env()` reader" sections a single decision record to point back to.
+
+## Decision
+
+Adopt three seam shapes, selected by how many call sites a boundary has and
+whether it is expected to grow:
+
+- **Narrow closure seams**, for a single variable read by a single caller.
+   The module owns a private function that takes an
+   `FnOnce(&str) -> Result<String, env::VarError>` (or the equivalent
+   `OsString`-typed form) instead of calling `std::env::var` itself. Examples:
+   the `resolve_with` variants in `output_mode.rs` and `output_prefs.rs`
+   described earlier in the developer guide. A related but distinct pattern
+   injects a resolved *value* rather than a closure: the `stdlib::path`
+   home-directory resolver's `HomeDirectory` enum (`Ambient`/`Missing`/
+   `Explicit`) lets a caller supply the home directory directly, so the
+   process-reading `home_from_env` ladder in `src/stdlib/path/path_utils.rs`
+   remains a directly annotated composition root rather than gaining its own
+   `_with` closure parameter.
+- **The `mockable::Env` trait**, for a boundary mocked across many tests or
+   expected to grow further inputs. `resolve_ninja_program_utf8_with` in
+   `src/runner/process/ninja_program.rs` takes `&impl Env`; production supplies
+   `mockable::DefaultEnv`, and tests supply `mockable::MockEnv` for every
+   resolution branch without mutating the process (#488).
+   `stdlib::which::env::EnvSnapshot::capture_with_env` takes the same
+   `&impl Env` and reads `PATH`, `PATHEXT`, and `NETSUKE_WHICH_WORKSPACE`
+   through it, so one provider covers every ambient input the resolver has
+   (#487).
+- **`EnvReader` `Arc` closures**, for a boundary whose registration point
+   requires `Send + Sync`. The manifest `env()` Jinja helper
+   (`src/manifest/env_reader.rs`) reads through an injected `EnvReader`, a
+   shared `Fn(&str) -> Result<String, EnvReadError>` (a manifest-owned error
+   type distinguishing an absent variable from a non-UTF-8 one, so the helper
+   does not expose the process adapter's `VarError`); `minijinja` requires
+   registered functions to be `Send + Sync`, so the reader is captured as an
+   `Arc` by the registered closure rather than borrowed (#484).
+
+None of these shapes is a general-purpose environment service. Each is owned by
+the module that reads its variable, stays private to it, and covers one
+variable or one precedence ladder; see "Environment and template ports" in
+`docs/developers-guide.md` for the composition rules that apply to all three,
+and "Ownership and permitted call sites" under that guide's "Manifest `env()`
+reader" section for the `EnvReader` shape specifically.
+
+### `EnvSnapshot` ownership
+
+`stdlib::which::env::EnvSnapshot::capture` is the resolver's single ambient
+boundary: it captures `PATH`, `PATHEXT`, and the `NETSUKE_WHICH_WORKSPACE`
+switch as *data*, in one place, rather than letting each downstream decision
+read the process independently. Absence and malformed-UTF-8 outcomes are
+stored, not resolved, at capture time.
+
+Capture is also the only place the platform's `std::env::VarError` is spoken.
+It translates the reading into the `WorkspaceSwitch` domain state (`Value`,
+`Absent`, `NotUnicode`) and emits the non-UTF-8 warning there, so the policy
+behind the boundary carries neither the platform error type nor a logging
+dependency. `workspace_switch.rs` holds only the variable name and that state,
+making it a leaf module: it is used by `env` and by `lookup::workspace`, and it
+calls back into neither, so there is no environment-to-lookup cycle.
+
+The which-resolver `CacheKey` incorporates every input the snapshot captured,
+though not uniformly: `cwd` is stored directly as its own field, while
+`stdlib::which::cache::env_fingerprint` hashes `raw_path`, `raw_pathext`, and
+the `WorkspaceSwitch` state, which derives `Hash` precisely so it can be
+hashed directly. Either way, two resolutions that differ only in one captured
+environment input cannot share a cache entry.
+
+### Explicit child-environment composition
+
+Tests that need a controlled child-process environment configure the child
+explicitly; nothing sanctioned mutates the parent test process's environment to
+influence a spawned `netsuke` binary. `test_support::netsuke`'s
+`run_netsuke_in_with_env` calls `env_clear()` on the constructed
+`assert_cmd::Command`, forwards the host `PATH`, and then applies the caller's
+`extra_env` pairs through `Command::env`. The BDD helper
+`build_netsuke_command` follows the same pattern: it clears the inherited
+environment and forwards only `PATH` and the scenario's tracked
+`env_vars_forward` map, one `cmd.env(key, value)` call per entry. Since #493,
+nothing reads `NETSUKE_NINJA` (or any other override) from the parent process
+to populate a child; the value always travels explicitly through `extra_env` or
+`env_vars_forward`.
+
+Subprocess isolation constructed this way — explicit `Command::env` calls
+against a cleared child environment — is the only sanctioned route for getting
+an ambient-looking variable such as `NETSUKE_NINJA` or `PATH` to a spawned
+process under test. Mutating the test process's own environment to achieve the
+same effect is not an accepted alternative to any of the three seam shapes
+above.
+
+Composition does not stop at the child-process boundary: in-process callers
+that need a deterministic Ninja executable use `runner::run_with_ninja_program`
+to supply the already-resolved program path directly, bypassing `NETSUKE_NINJA`
+resolution entirely rather than setting the variable for a child to read.
+
+## Rationale
+
+- **Proportionate abstraction.** A trait object for a single-variable,
+  single-caller boundary would recreate the ambient coupling the seam exists to
+  remove, just one layer down; a bare closure is cheaper to read and to test.
+  `mockable::Env` earns its weight only when a boundary is exercised by many
+  tests or is expected to grow (#488).
+- **`Send + Sync` is a real constraint, not a preference.** `EnvReader`'s
+  `Arc` wrapping is not a stylistic choice; `minijinja`'s function-registration
+  API requires it, and a plain closure parameter cannot satisfy that bound when
+  the closure must be captured by a registered, potentially cloned function
+  (#484).
+- **Data over decisions at the boundary.** Capturing `EnvSnapshot` once and
+  deriving decisions downstream keeps the resolver testable without process
+  mutation, and keeps `workspace_switch` a leaf module rather than a second
+  place that reads the process.
+- **Cache correctness follows from capture completeness.** Hashing every
+  captured input, including the workspace switch, is what prevents a resolution
+  made with the fallback enabled from answering a lookup made with it disabled.
+- **No back door around subprocess isolation.** Explicit `Command::env`
+  composition is auditable per test and cannot race with parallel test
+  execution the way a shared-process mutation could.
+
+## Consequences
+
+- A contributor introducing a new environment-dependent boundary chooses
+  among the three seam shapes by call-site count and `Send + Sync`
+  requirements, rather than inventing a fourth shape or reaching for the
+  heaviest option by default.
+- `docs/developers-guide.md`'s "Environment and template ports", "Environment
+  lookup seams", and "Manifest `env()` reader" sections, and this ADR must stay
+  consistent; widen one only alongside the others when a boundary's shape
+  changes.
+- Reviewers can reject a new `mockable::Env`-shaped boundary for a
+  single-variable, single-caller site, and a new closure-shaped boundary for a
+  site that clearly needs `Send + Sync` registration or broad mocking.
+- Any future ambient input added to the which resolver's boundary (a new
+  environment variable, for example) must be folded into `EnvSnapshot` and into
+  `env_fingerprint`, not read independently downstream, to preserve the
+  no-cycle and cache-correctness properties this ADR records.
+
+## Alternatives considered
+
+- **A single shared `Env` trait for every boundary.** Rejected: forcing
+  `mockable::Env` (or an equivalent trait object) on single-variable,
+  single-caller sites such as `output_mode.rs`'s `resolve_with` would add
+  indirection with no matching test-surface benefit, and would blur the "one
+  variable or one precedence ladder" ownership rule this ADR reaffirms.
+- **Reading the parent process's environment for child-process tests.**
+  Rejected: mutating the test process to influence a spawned `netsuke` binary
+  reintroduces the shared-mutable-state races that injected readers and
+  child-process configuration exist to avoid, and it is exactly the pattern
+  #493 removed from the BDD and integration test helpers.
+  `.config/nextest.toml` runs no serialized environment group precisely because
+  no sanctioned test still mutates the harness environment; `EnvLock` and
+  `CwdGuard` remain only for the few tests that exercise process
+  working-directory behaviour.
+
+## Implementation references
+
+- Workspace switch state:
+  [`src/stdlib/which/workspace_switch.rs`](../src/stdlib/which/workspace_switch.rs)
+- `EnvSnapshot`: [`src/stdlib/which/env.rs`](../src/stdlib/which/env.rs)
+- Cache fingerprint: [`src/stdlib/which/cache.rs`](../src/stdlib/which/cache.rs)
+- `mockable::Env` seam:
+  [`src/runner/process/ninja_program.rs`](../src/runner/process/ninja_program.rs);
+  `runner::run_with_ninja_program` in
+  [`src/runner/mod.rs`](../src/runner/mod.rs) is the companion injected seam
+  that lets callers select the resolved Ninja executable directly, without
+  going through `NETSUKE_NINJA` resolution at all
+- `EnvReader`: [`src/manifest/env_reader.rs`](../src/manifest/env_reader.rs)
+  (manifest `env()` Jinja helper)
+- Child-environment composition:
+  [`test_support/src/netsuke.rs`](../test_support/src/netsuke.rs)
+  (`run_netsuke_in_with_env`) and `tests/bdd/steps/manifest_command_helpers.rs`
+  (`build_netsuke_command`)
+- Policy narrative: "Environment and template ports" and "Injected and
+  child-process environments" in
+  [`docs/developers-guide.md`](developers-guide.md)

docs/contents.md

@@ -46,6 +46,9 @@ operator, user, and contributor references are easier to find.
 - [adr-007-publish-as-netsuke-build.md](adr-007-publish-as-netsuke-build.md):
   crates.io package rename decision record, and the package-versus-target
   naming rule it establishes.
+- [adr-008-environment-seam-taxonomy.md](adr-008-environment-seam-taxonomy.md):
+  Environment seam taxonomy decision record: three sanctioned shapes for
+  injecting environment-dependent input instead of reading the process directly.
 
 ## User and operator guides
 

docs/developers-guide.md

@@ -1569,9 +1569,9 @@ is not obvious from the name:
   present.
 - `try_is_file(path) -> io::Result<bool>` is the fallible counterpart to the
   boolean predicates: `Ok(true)` when the path is a regular file, `Ok(false)`
-  when it is absent (`NotFound` is folded into the boolean result), and
-  `Err` for any other metadata failure, so callers can distinguish absence
-  from inaccessibility. The binary locator in `test_support/src/netsuke.rs`
+  when it is absent (`NotFound` is folded into the boolean result), and `Err`
+  for any other metadata failure, so callers can distinguish absence from
+  inaccessibility. The binary locator in `test_support/src/netsuke.rs`
   (`netsuke_executable_from`, see
   [Locating the netsuke binary](#locating-the-netsuke-binary)) relies on it to
   surface unexpected filesystem errors while probing candidate paths.
@@ -1757,6 +1757,13 @@ boundary policy.
 
 ### Environment and template ports
 
+The seams described in this section follow one of three sanctioned shapes —
+narrow closures, `mockable::Env`, or `EnvReader` — chosen by call-site count,
+expected growth, and `Send + Sync` registration requirements: use
+`mockable::Env` when a boundary is expected to acquire more inputs, even
+before its call-site count grows. See
+[ADR-008](adr-008-environment-seam-taxonomy.md) for the taxonomy.
+
 `manifest::EnvReader` owns environment lookup for the manifest `env()` helper.
 Production constructs the process-backed adapter at the manifest loading
 boundary; tests pass an `Arc`-backed reader directly. The port is only for
@@ -2261,6 +2268,40 @@ Selected file-load errors and malformed `NETSUKE_JSON` values are returned to
 the caller. Accepted environment values are `true`, `false`, `1`, and `0`. An
 explicit root `--json` flag bypasses environment parsing.
 
+#### Workspace fallback switch seam
+
+`src/stdlib/which/workspace_switch.rs` is a leaf module holding the
+`NETSUKE_WHICH_WORKSPACE` name and the domain state `WorkspaceSwitch`
+(`Value`, `Absent`, `NotUnicode`) with its `enabled()` decision. The variable
+is read by `EnvSnapshot::capture` through the injected `mockable::Env`
+provider and stored as snapshot data; the enable/disable decision is derived
+from that snapshot on demand. The cache fingerprint hashes the state —
+`WorkspaceSwitch` derives `Hash` for exactly that purpose — so two resolutions
+differing only in this switch never share a cache entry.
+
+The adapter owns everything platform-specific. `env.rs` holds the
+`From<Result<String, std::env::VarError>>` conversion, the single point at
+which the platform error becomes a domain state, and emits the non-UTF-8
+warning once per capture immediately after the read. Only the variable's name
+is logged, never its value. The leaf module therefore names neither `VarError`
+nor `tracing`, and consulting the switch afterwards is silent. See
+[ADR-008](adr-008-environment-seam-taxonomy.md) for the seam taxonomy.
+
+#### Ninja program resolver seam
+
+`resolve_ninja_program_utf8_with` in `src/runner/process/ninja_program.rs`
+takes `&impl mockable::Env`, with `mockable::DefaultEnv` as the production
+adapter supplied by the ambient `resolve_ninja_program_utf8` wrapper. The unit
+tests inject a `MockEnv` that pins the `NETSUKE_NINJA` key, so every override
+branch runs without process mutation.
+
+`resolve_ninja_program_with`, in the same module, takes the identical `&impl
+mockable::Env` seam and converts the UTF-8 result into a general platform
+`PathBuf`. It is compiled only under `#[cfg(test)]`: production reaches the
+platform-path form through `resolve_ninja_program`, which itself calls the
+UTF-8 resolver and converts its result, so no production path constructs a
+platform `PathBuf` independently of `resolve_ninja_program_utf8_with`.
+
 ### Configuration discovery module layout
 
 `src/cli/discovery.rs` attaches several small `#[path = "..."]` modules that
@@ -2419,8 +2460,8 @@ any scenario that requires a hermetic child environment.
 
 #### Locating the netsuke binary
 
-Both `run_netsuke_in` and `run_netsuke_in_with_env` depend on a private
-locator, `netsuke_executable()`, to find the built `netsuke` binary.
+Both `run_netsuke_in` and `run_netsuke_in_with_env` depend on a private locator,
+`netsuke_executable()`, to find the built `netsuke` binary.
 `netsuke_executable()` converts `std::env::current_exe()` to a
 `camino::Utf8PathBuf` and delegates to `netsuke_executable_from`, which takes
 an injected `mockable::Env` — the same injectable-environment pattern used by
@@ -2438,8 +2479,8 @@ The locator checks candidate paths in order:
 3. `CARGO_TARGET_DIR/<triple>/<profile>/`, for `--target` builds where the
    profile directory nests under the target triple.
 
-Filesystem errors other than "not found" are surfaced rather than treated as
-a missing candidate, via the [`test_support::fs`](#test_supportfs) wrapper
+Filesystem errors other than "not found" are surfaced rather than treated as a
+missing candidate, via the [`test_support::fs`](#test_supportfs) wrapper
 `try_is_file`. When every candidate misses, the resulting error lists all
 attempted paths.
 

docs/users-guide.md

@@ -716,6 +716,7 @@ Common environment equivalents include:
 - `NETSUKE_LOCALE=en-US`
 - `NETSUKE_DEFAULT_TARGETS__0=hello.txt`
 - `NETSUKE_NINJA=/opt/ninja/bin/ninja`
+- `NETSUKE_WHICH_WORKSPACE=0`
 
 `NETSUKE_LOCALE` selects the interface language; see
 [Choose a language with `--locale`](#choose-a-language-with---locale) for how
@@ -725,6 +726,13 @@ it combines with the flag and the system default.
 Leave it unset to use `ninja` from `PATH`, or set another executable name or an
 absolute path. Empty and non-UTF-8 values fall back to the default.
 
+`NETSUKE_WHICH_WORKSPACE` switches off the `which()` workspace-tree fallback
+search that runs when a command is not found on `PATH`. Set it to `0`,
+`false`, or `off` (case-insensitively) to disable the fallback; any other
+value, or leaving it unset, keeps the fallback enabled. A non-Unicode value
+also disables the fallback and is treated as an explicit opt-out, emitting a
+warning.
+
 The CLI and configuration use the same policy values. `auto` follows terminal
 and environment detection. `always` or `never` makes colour, emoji, or progress
 behaviour explicit. Accessibility uses `on` and `off` for its explicit values.

src/runner/process/mod.rs

@@ -31,7 +31,7 @@ pub use ninja_program::resolve_ninja_program;
 #[cfg(doctest)]
 pub use ninja_program::resolve_ninja_program_utf8;
 #[cfg(test)]
-use ninja_program::resolve_ninja_program_utf8_with;
+use ninja_program::{resolve_ninja_program_utf8_with, resolve_ninja_program_with};
 pub use paths::*;
 use streaming::{ForwardStats, forward_child_output, forward_child_output_with_ninja_status};