Give the manifest env() helper an injectable env seam (#484)

.config/nextest.toml

@@ -32,5 +32,5 @@ slow-timeout = { period = "60s", terminate-after = 5 }
 serial-env = { max-threads = 1 }
 
 [[profile.default.overrides]]
-filter = 'binary(manifest_env_tests) | binary(ninja_env_tests) | binary(env_path_tests)'
+filter = 'binary(ninja_env_tests) | binary(env_path_tests)'
 test-group = 'serial-env'

docs/developers-guide.md

@@ -1137,10 +1137,11 @@ variables rather than reinterpreting one as the other.
 The runner is configured by `.config/nextest.toml` at the workspace root. It
 governs the non-doctest pass only, and deliberately stays small:
 
-- **`serial-env` test group** (`max-threads = 1`) covering exactly three
-  binaries: `manifest_env_tests`, `ninja_env_tests`, and `env_path_tests`.
-  These mutate process-global environment state — `PATH`, `NINJA_ENV`, and ad
-  hoc `NETSUKE_*` variables. Every other test remains fully parallel.
+- **`serial-env` test group** (`max-threads = 1`) covering exactly two
+  binaries: `ninja_env_tests` and `env_path_tests`. These mutate process-global
+  environment state — `PATH` and `NINJA_ENV`. Every other test remains fully
+  parallel. `manifest_env_tests` was a member until it moved to an injected
+  reader; it mutates nothing now, so it runs parallel with the rest.
 - **No blanket retries.** A test that fails intermittently is a defect to
   diagnose. Add a targeted override with a written rationale only when a
   genuine external-resource constraint requires one.
@@ -1154,9 +1155,14 @@ nextest runs each test in its own process, so environment and working-directory
 mutations cannot leak between tests the way they can under the threaded
 in-process harness. The `EnvLock`, `EnvVarGuard`, and `CwdGuard` utilities
 described in [Test isolation utilities](#test-isolation-utilities), and the
-`#[serial]` markers on the tests in the three binaries above, remain necessary
+`#[serial]` markers on the tests in the two binaries above, remain necessary
 because the coverage workflow still drives an in-process runner.
 
+They are needed only for binaries that still mutate process-global state. A
+binary migrated to an injected seam should leave the group and drop its
+`#[serial]` markers in the same change, so the configuration does not outlive
+the constraint it describes.
+
 The `serial-env` group is therefore not load-bearing for the tests that exist
 today; it states the serialization contract once so both runners agree, and so
 it is not silently lost if a future test in those binaries reaches for
@@ -1638,6 +1644,47 @@ lock is still held; `try_lock` from the owning thread returns `WouldBlock`, so
 "blocked" means the bundle still holds it. Reverting the field order turns that
 assertion red deterministically.
 
+### Manifest `env()` reader
+
+The `env()` Jinja helper reads through an injected [`EnvReader`], a shared
+`Fn(&str) -> Result<String, VarError>`. `minijinja` requires registered
+functions to be `Send + Sync`, so the reader is an `Arc` captured by the
+registered closure rather than a borrowed parameter.
+
+`manifest::from_str` supplies `process_env_reader()`; `from_str_with_env` takes
+one explicitly, so a test can drive the **real registration path** — the same
+`Environment`, the same `add_function("env", ..)` call — without touching the
+process.
+
+#### Ownership and permitted call sites
+
+- The caller owns the reader. `from_str` constructs `process_env_reader()`
+  and `from_str_with_env` borrows the caller's reader; both pass it to
+  `from_str_named`, which receives it as `&EnvReader` and `Arc::clone`s it
+  into the registered closure, so the closure co-owns the `Arc` alongside the
+  caller. `from_str_named` remains the only place the `env()` function is
+  registered. In production nothing else constructs a reader; tests build
+  their own with `Arc::new`, which is the point of the seam.
+- `process_env_reader()` is the sole production supplier and the only place
+  `std::env::var` appears in the module.
+- The two test layers cover different things, and both are needed:
+  - **Integration tests use `from_str_with_env`.** Only they exercise
+    registration — that the reader actually reaches the `env()` function
+    Jinja calls. Covering the leaf mapper alone would leave that untested,
+    which is the gap the earlier process-mutating tests existed to fill.
+  - **Unit tests may call `env_var_with` directly** to cover error mapping.
+    `src/manifest/tests/env_function.rs` does so deliberately: the
+    present, absent, and non-UTF-8 branches are cheaper to drive at the leaf,
+    and the non-UTF-8 case is unreachable through a real environment without
+    platform-specific `OsString` surgery.
+
+#### Reader composition rules
+
+- One reader serves an entire parse. A manifest reading several variables
+  passes one reader consulted repeatedly, never a per-variable registry.
+- The reader answers by name only. It must not enumerate, and it must not
+  mutate.
+
 ### `EnvLock`
 
 `test_support::env_lock::EnvLock` is a global mutex that serializes all

docs/netsuke-design.md

@@ -1028,14 +1028,18 @@ the template environment. These functions will be implemented in safe Rust,
 providing a secure bridge to the underlying system.
 
 - `env(var_name: &str, default: Option<String>) -> Result<String, Error>`: A
-  function that reads an environment variable from the system. This allows
-  build configurations to be influenced by the external environment (e.g.,
-  `PATH`, `CC`). It returns an error if the variable is undefined and no
-  `default` is provided, or if the variable contains invalid UTF-8. The
-  `default` argument is planned; the current implementation only accepts the
-  variable name. This helper only reads the process environment during manifest
-  evaluation; it does not set the action execution environment. The planned
-  manifest-level `env` block in
+  function that reads an environment variable through an injected
+  `EnvReader` — an `Arc`'d `Fn(&str) -> Result<String, VarError>` — rather
+  than the process environment directly. `manifest::from_str` supplies the
+  process-backed `process_env_reader()`, and `manifest::from_str_with_env`
+  lets a caller supply its own, so evaluation need not touch the process
+  environment; ownership sits with the caller, and `from_str_named` clones
+  the reader into the single `env()` registration site. This allows build
+  configurations to be influenced by the external environment (e.g., `PATH`,
+  `CC`). It returns an error if the variable is undefined and no `default` is
+  provided, or if the variable contains invalid UTF-8. The `default` argument
+  is planned; the current implementation only accepts the variable name. The
+  planned manifest-level `env` block in
   [§2.6](#26-planned-recipe-ergonomics-and-execution-feedback) controls the
   environment Netsuke applies when actions run.
 

docs/users-guide.md

@@ -432,6 +432,44 @@ Both helpers accept:
 The `env(name)` function reads one required environment variable. v0.1.0 does
 not accept a default argument; an absent or non-Unicode value is an error.
 
+### Inject the environment reader for tests and embedding
+
+`env()` does not read `std::env::var` directly. Manifest parsing goes through
+an injectable `EnvReader` seam, so callers that need deterministic `env()`
+results — test suites and programs embedding Netsuke as a library — can supply
+their own reader instead of mutating the process environment.
+
+- `netsuke::manifest::from_str` parses a manifest using the live process
+  environment.
+- `netsuke::manifest::from_str_with_env` takes an explicit `EnvReader`,
+  letting the caller control every value `env()` returns.
+- `netsuke::manifest::process_env_reader` builds the process-backed reader
+  that `from_str` uses by default.
+
+A missing variable still fails the parse with a Jinja "undefined" error, and a
+non-Unicode value still fails with an "invalid operation" error; only the
+source of the values changes.
+
+<!-- tested-example: guide-env-reader-snippet -->
+
+```rust
+use netsuke::manifest::{EnvReader, from_str_with_env};
+use std::sync::Arc;
+
+let reader: EnvReader = Arc::new(|_| Ok(String::from("release")));
+let yaml = concat!(
+    "netsuke_version: \"1.0.0\"\n",
+    "targets:\n",
+    "  - name: \"{{ env('PROFILE') }}\"\n",
+    "    command: echo hi\n",
+);
+let manifest = from_str_with_env(yaml, &reader).expect("parse");
+assert!(format!("{:?}", manifest.targets[0].name).contains("release"));
+```
+
+This snippet mirrors the executable doctest on `from_str_with_env` in the API
+documentation, rather than the YAML-only examples elsewhere in this guide.
+
 ## Use the template standard library
 
 Netsuke registers focused path, collection, command, network, and time helpers

src/manifest/env_reader.rs

@@ -0,0 +1,83 @@
+//! Environment access for the manifest `env()` Jinja helper.
+//!
+//! Isolated from `manifest::mod` so the reader type, its process-backed
+//! default, and the failure mapping sit together, and so neither module
+//! exceeds the repository's 400-line file limit.
+//!
+//! See the "Manifest `env()` reader" section of the developers' guide for this
+//! seam's ownership boundary and composition rules.
+
+use std::sync::Arc;
+
+use minijinja::{Error, ErrorKind};
+
+use crate::localization::{self, keys};
+
+/// Environment reader supplied to the `env()` Jinja helper.
+///
+/// Shared and `Send + Sync` because `minijinja` requires registered functions
+/// to be both, and the reader is captured by the registered closure.
+pub type EnvReader =
+    Arc<dyn Fn(&str) -> std::result::Result<String, std::env::VarError> + Send + Sync>;
+
+/// Reader backed by the live process environment.
+///
+/// # Examples
+///
+/// ```rust,no_run
+/// use netsuke::manifest::process_env_reader;
+///
+/// // Reads whatever the process has. Not executed: the result depends on the
+/// // caller's environment, so running it would make the doctest a hostage to
+/// // whatever CI happens to export.
+/// let reader = process_env_reader();
+/// drop(reader("PATH"));
+/// ```
+#[must_use]
+#[expect(
+    clippy::disallowed_methods,
+    reason = "composition root: supplies the process environment to the env() Jinja helper"
+)]
+pub fn process_env_reader() -> EnvReader {
+    Arc::new(|key: &str| std::env::var(key))
+}
+
+/// Resolve the value of an environment variable for the `env()` Jinja helper.
+///
+/// Returns the variable's value or a structured error that mirrors Jinja's
+/// failure modes, ensuring templates halt when a variable is missing or not
+/// valid UTF-8.
+/// Resolve `name` through `read_env`, mapping failures to Jinja errors.
+///
+/// Kept separate from the Jinja registration so all three outcomes — present, absent, and
+/// non-UTF-8 — can be exercised without mutating the process environment. The
+/// non-UTF-8 branch is otherwise unreachable from a test: fabricating such a
+/// value in the live environment needs platform-specific `OsString` surgery,
+/// and the AGENTS.md testing mandate forbids in-process mutation regardless.
+///
+/// # Examples
+///
+/// ```rust,ignore
+/// let value = env_var_with("FOO", |_| Ok(String::from("bar")));
+/// assert_eq!(value.expect("FOO"), "bar");
+/// ```
+pub(super) fn env_var_with<F>(name: &str, read_env: F) -> std::result::Result<String, Error>
+where
+    F: FnOnce(&str) -> std::result::Result<String, std::env::VarError>,
+{
+    match read_env(name) {
+        Ok(val) => Ok(val),
+        Err(std::env::VarError::NotPresent) => Err(Error::new(
+            ErrorKind::UndefinedError,
+            localization::message(keys::MANIFEST_ENV_MISSING)
+                .with_arg("name", name)
+                .to_string(),
+        )),
+        Err(std::env::VarError::NotUnicode(_)) => Err(Error::new(
+            ErrorKind::InvalidOperation,
+            localization::message(keys::MANIFEST_ENV_INVALID_UTF8)
+                .with_arg("name", name)
+                .to_string(),
+        )),
+    }
+}