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

[leynos]

Summary

env_var — backing the env() Jinja helper — read the process environment directly:

fn env_var(name: &str) -> std::result::Result<String, Error> {
    match std::env::var(name) { ... }
}

Its three outcomes could only be reached by mutating global state, which the AGENTS.md testing mandate no longer permits. One was unreachable in practice regardless: fabricating a non-UTF-8 value in the live environment needs platform-specific OsString surgery, so the NotUnicode branch and its distinct Jinja error kind had no coverage at all.

The function's doc comment also advertised a test_support::env::VarGuard example that mutates global state; that goes with it.

Approach

Split resolution into env_var_with, taking a read_env closure — consistent with the existing seam in runner::process::ninja_program and the two added in #486 and #487. A closure rather than a trait object: keyed lookup, one caller, and the mandate permits a narrow closure where a trait object would be disproportionate.

Coverage

Six cases, none mutating anything:

• a present variable resolves to its value
• an empty value is returned rather than treated as missing — an empty value is a value, not an absence
• both failures map to their documented Jinja error kind
• the two failure kinds stay distinct — a missing variable is a template authoring error, whereas a non-UTF-8 value is an environment problem the author cannot fix in the template. Collapsing them onto one kind would misdirect whoever reads the failure, and nothing previously stopped that
• the requested name reaches the seam unaltered and appears in the message

Checked against upstream RFCs

ortho-config RFC 0001 governs field-level environment aliases for configuration structs. It has no bearing on the manifest env() helper, which is Netsuke's own template surface, so the closure seam here is not in tension with the EnvSource abstraction being added upstream.

Verification

All gates pass: check-fmt, lint, typecheck, test (1194 nextest), markdownlint, nixie. CodeScene delta: no issues.

Closes #484.
Refs #496.

🤖 Generated with Claude Code

Summary by Sourcery

Introduce an injectable environment lookup seam for the manifest env() helper to enable full testing of all resolution outcomes without mutating process state.

Enhancements:

• Add env_var_with helper that accepts a closure for environment variable resolution while preserving existing env_var behavior and error mapping semantics.

Tests:

• Add dedicated env_function tests that cover successful resolution, empty values, distinct failure kinds, and propagation of variable names through error messages without touching the real environment.

[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

Summary

• Inject environment lookups into manifest env() evaluation through EnvReader.
• Add manifest::from_str_with_env, process_env_reader, and env_var_with.
• Retain process-environment defaults at public boundaries.
• Preserve distinct error mappings for missing and non-UTF-8 values.
• Add platform-independent tests without mutating process-global state.
• Remove manifest_env_tests from the serial environment test group.
• Update user, developer, and design documentation.
• Replace the VarGuard example with a MockEnv example.
• Add documentation coverage for from_str_with_env, EnvReader, and env('PROFILE').

Closes #484. References #496.

Verification

• check-fmt
• lint
• typecheck
• test — 1194 nextest tests
• markdownlint
• nixie
• No CodeScene delta issues

Walkthrough

Use an injectable EnvReader for manifest Jinja lookups. Keep process-environment defaults for standard and file-based loading. Update tests, documentation, and serial-test configuration.

Changes

Manifest environment injection

Layer / File(s)	Summary
Injectable environment helper and parser integration src/manifest/env_reader.rs, src/manifest/mod.rs	Add EnvReader, process_env_reader, and from_str_with_env. Route Jinja env() lookups through the configured reader. Preserve Jinja error mappings.
Injected lookup tests src/manifest/tests/*, tests/manifest_env_tests.rs	Test present, empty, missing, invalid UTF-8, special-character, and requested-name cases without changing process-global environment state.
Parallel environment test configuration and documentation .config/nextest.toml, tests/makefile_test_target.rs, docs/developers-guide.md, docs/users-guide.md, docs/netsuke-design.md, tests/documentation_examples_tests.rs	Remove manifest_env_tests from serial-env. Document injected-reader ownership, execution rules, and the public API. Validate the documented example.

Sequence Diagram(s)

sequenceDiagram
  participant Caller
  participant ManifestParser
  participant Jinja
  participant EnvReader
  Caller->>ManifestParser: Call from_str_with_env(yaml, reader)
  ManifestParser->>Jinja: Register env() with reader
  Jinja->>EnvReader: Request variable name
  EnvReader-->>Jinja: Return value or mapped error
  Jinja-->>Caller: Return parsed manifest or diagnostic

Loading

Possibly related issues

• leynos/netsuke#496 — Covers the same manifest env() injection work.
• leynos/netsuke#522 — Relates to env_var_with documentation in the new environment-reader module.

Possibly related PRs

• leynos/netsuke#330 — Uses a related injected environment-reader pattern.
• leynos/netsuke#473 — Updates the same serial environment test configuration and validation.
• leynos/netsuke#515 — Directly overlaps the manifest reader, parser, and environment test changes.

Suggested labels: Issue

Suggested reviewers: codescene-access

Poem

Inject the reader; keep globals still.
Render manifests with deterministic skill.
Map missing names and invalid bytes.
Run manifest tests through parallel gates.
Keep process access at the boundary.

🚥 Pre-merge checks | ✅ 20

✅ Passed checks (20 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title accurately describes the injectable manifest env() seam and links issue #484 as required.
Description check	✅ Passed	The description directly explains the injectable environment seam, test coverage, documentation, and verification results.
Linked Issues check	✅ Passed	The changes satisfy issue #484 by injecting EnvReader, preserving process defaults, removing environment mutation, and covering non-UTF-8 errors.
Out of Scope Changes check	✅ Passed	The code, tests, configuration, and documentation changes support the linked issue objectives and contain no unrelated scope.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Testing (Overall)	✅ Passed	Unit tests cover success, empty, missing, non-UTF-8, error kinds, and variable names; integration tests exercise injected readers through real manifest Jinja registration.
User-Facing Documentation	✅ Passed	The users' guide documents the new EnvReader API, live-environment default, deterministic injection, error behaviour, and a concrete from_str_with_env example.
Developer Documentation	✅ Passed	Accept the change: the developer's guide documents EnvReader ownership, call sites, composition, and test isolation; the design document records the injected env() boundary and defaults.
Module-Level Documentation	✅ Passed	All touched Rust modules have //! documentation; env_reader explains its purpose, process-reader relationship, and composition rules, and test modules state their scope.
Testing (Unit And Behavioural)	✅ Passed	Accept the check: unit tests cover present, empty, missing, non-UTF-8, error kinds, and names; behavioural tests exercise public from_str_with_env and rendered commands.
Testing (Property / Proof)	✅ Passed	The change has a finite outcome mapping, not a range-based state or ordering invariant; tests cover Ok, empty, NotPresent, NotUnicode, error kinds, names, and integration wiring. No proof obligatio...
Testing (Compile-Time / Ui)	✅ Passed	The change has no compile-time behaviour; its doctest and focused assertions cover runtime values, ErrorKind, names, and message fragments, so trybuild or snapshots are not required.
Unit Architecture	✅ Passed	EnvReader is an explicit Result-returning seam; only process_env_reader calls std::env::var, and public parse boundaries inject it. Unit and integration tests avoid environment mutation.
Domain Architecture	✅ Passed	from_str_with_env injects EnvReader; the sole std::env::var call is confined to process_env_reader, and tests exercise parsing without process mutation.
Observability	✅ Passed	Retain the existing diagnostic path: missing and invalid-UTF-8 lookups expose distinct error kinds and the requested variable name; no service boundary or metric-worthy runtime behaviour was added.
Security And Privacy	✅ Passed	The PR adds no credentials or secret literals; EnvReader returns values without logging them, and errors include only the requested name while discarding non-UTF-8 contents.
Performance And Resource Use	✅ Passed	Accept the change: production paths create one EnvReader and clone one Arc per parse; lookups remain bounded by template use, with no new unbounded collection, loop, or repeated I/O.
Concurrency And State	✅ Passed	EnvReader is caller-owned and Send + Sync; parsing clones it into a local Jinja environment, while migrated tests inject readers without global mutation or serialisation.
Architectural Complexity And Maintainability	✅ Passed	Keep the change: EnvReader isolates one global lookup, supports both parse entry points and real registration tests, and has documented ownership without new dependencies or cycles.
Rust Compiler Lint Integrity	✅ Passed	The PR adds no broad unused-code suppressions or artificial anchors; all new helpers and re-exports have real uses, and Arc/String clones serve callback ownership or repeated test results.

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch issue-484-inject-env-seam-into-manifest-env-function

---

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

[sourcery-ai]

Reviewer's Guide

Injects an environment-reading seam into the manifest env() helper by introducing env_var_with, updating env_var to delegate through it, and adding focused tests that exercise all Jinja error paths without mutating the process environment.

Sequence diagram for manifest env() helper with injectable env seam

sequenceDiagram
    title Manifest env helper resolution via env_var_with

    actor TemplateAuthor
    participant Template as template
    participant EnvHelper as env
    participant EnvVar as env_var
    participant EnvVarWith as env_var_with
    participant ReadEnv as read_env_closure

    TemplateAuthor->>Template: render_manifest
    Template->>EnvHelper: env("FOO")
    EnvHelper->>EnvVar: env_var("FOO")
    EnvVar->>EnvVarWith: env_var_with("FOO", read_env_closure)
    EnvVarWith->>ReadEnv: read_env_closure("FOO")

    alt [value present]
        ReadEnv-->>EnvVarWith: Ok(String)
        EnvVarWith-->>EnvVar: Ok(String)
        EnvVar-->>EnvHelper: Ok(String)
        EnvHelper-->>Template: "FOO" value
    else [variable missing]
        ReadEnv-->>EnvVarWith: Err(VarError::NotPresent)
        EnvVarWith-->>EnvVar: Err(ErrorKind::UndefinedError)
        EnvVar-->>EnvHelper: Err(ErrorKind::UndefinedError)
    else [value not utf8]
        ReadEnv-->>EnvVarWith: Err(VarError::NotUnicode)
        EnvVarWith-->>EnvVar: Err(ErrorKind::TemplateRuntimeError)
        EnvVar-->>EnvHelper: Err(ErrorKind::TemplateRuntimeError)
    end

Loading

File-Level Changes

Change	Details	Files
Introduce an injectable environment seam for the manifest env() helper and adjust its documentation.	Add env_var helper that delegates to env_var_with using std::env::var Extract env_var_with that takes a FnOnce(&str) closure returning Result<String, VarError> and maps outcomes to existing Jinja ErrorKind values Update the doc comment example to use env_var_with instead of VarGuard and remove global environment mutation from documentation	src/manifest/mod.rs
Add targeted tests for env_var_with to cover success, empty, missing, and non-UTF-8 cases and ensure error kind distinctions and name propagation.	Create env_function test module for env() resolution behavior Add tests verifying present and empty variable values are returned correctly Add rstest-based parameterized tests that map VarError variants to the documented Jinja ErrorKind values Add tests confirming the two failure kinds remain distinct and that the requested variable name is passed through to the seam and included in error messages Wire the new env_function module into the manifest tests suite	src/manifest/tests/mod.rs src/manifest/tests/env_function.rs

Assessment against linked issues

Issue	Objective	Addressed	Explanation
#484	Introduce an injectable environment seam for the manifest env() helper, including removing direct std::env::var usage from src/manifest/mod.rs and supplying a mockable DefaultEnv at the manifest public boundary.	❌	The PR introduces env_var_with(name, read_env) and has env_var delegate to it via a closure, which does provide an injectable seam. However, env_var still calls std::env::var inside src/manifest/mod.rs via env_var_with(name,
#484	Rewrite the env_var documentation example to use a non-mutating mock/injected environment instead of test_support::env::VarGuard, eliminating in-process mutation.	✅
#484	Migrate tests for the env() helper to the injected environment seam so they no longer mutate the process environment, and ensure the NotUnicode branch is covered by tests.	✅

Possibly linked issues

• Inject an Env seam into the manifest env() Jinja function #484: PR implements the injectable env seam, removes direct std::env::var usage, and updates tests including NotUnicode coverage.

---

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.

[sourcery-ai]

Hey - I've reviewed your changes and they look great!

---

Sourcery is free for open source - if you like our reviews please consider sharing them ✨

• X
• Mastodon
• LinkedIn
• Facebook

_{Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.}

[coderabbitai]

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@src/manifest/mod.rs`:
- Around line 73-74: Update the manifest parsing flow around from_str and
env_var to accept a mockable::Env dependency, register env() with it, and
replace direct std::env::var access with the injected reader. Use
mockable::DefaultEnv in production and mockable::MockEnv in tests, preserving
existing environment lookup behavior.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: b0c043b2-8934-4085-9bd9-462f2dfcfd04

📥 Commits

Reviewing files that changed from the base of the PR and between 3f84545 and 48c1b8f.

📒 Files selected for processing (3)

• src/manifest/mod.rs
• src/manifest/tests/env_function.rs
• src/manifest/tests/mod.rs

🔗 Linked repositories identified

CodeRabbit considers these linked repositories for cross-repo context during reviews:

• leynos/rstest-bdd (auto-detected)
• leynos/ortho-config (auto-detected)
• leynos/shared-actions (auto-detected)

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 48c1b8f7e7

ℹ️ 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".

[buzzybee-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.

[coderabbitai]

Actionable comments posted: 3

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@docs/developers-guide.md`:
- Around line 541-546: The developer guide paragraph incorrectly retains
isolation requirements for manifest environment tests. Update the discussion
around the “Test isolation utilities” section to exclude
tests/manifest_env_tests.rs from EnvLock, EnvVarGuard, CwdGuard, and #[serial]
guidance, limiting those requirements to binaries that still mutate
process-global state or documenting a separate valid coverage exception.

In `@src/manifest/env_reader.rs`:
- Around line 27-33: Mark the Rust doctest fence in the process_env_reader
documentation as no_run, changing the existing rust fence to rust,no_run while
preserving the example content.
- Around line 46-50: Replace the stale [`env_var`] intra-doc link in the
surrounding documentation with plain text or a valid symbol reference, ensuring
rustdoc no longer attempts to resolve the removed helper while preserving the
description of the three outcomes tested by the helper.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: 70a6ec0c-c5bc-46a7-9db9-b0e694c737da

📥 Commits

Reviewing files that changed from the base of the PR and between 3f84545 and d10fa0c.

📒 Files selected for processing (11)

• docs/adr-006-adopt-polonius-nightly-toolchain.md
• docs/developers-guide.md
• docs/netsuke-design.md
• docs/polonius.md
• docs/snapshot-testing-in-netsuke-using-insta.md
• docs/users-guide.md
• src/manifest/env_reader.rs
• src/manifest/mod.rs
• src/manifest/tests/env_function.rs
• src/manifest/tests/mod.rs
• tests/manifest_env_tests.rs

🔗 Linked repositories identified

CodeRabbit considers these linked repositories for cross-repo context during reviews:

• leynos/rstest-bdd (auto-detected)
• leynos/ortho-config (auto-detected)
• leynos/shared-actions (auto-detected)

[buzzybee-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 Have the following now been resolved?

docs/developers-guide.md (1)

837-839: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Restrict the ownership claim to production code.
tests/manifest_env_tests.rs constructs EnvReader directly with Arc::new. Therefore, “Nothing else constructs one except the public entry points” is too broad. State that production callers use the public entry points, while tests may construct injected readers directly.
Triage: [type:docstyle]

🤖 Detailed instructions

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In @docs/developers-guide.md around lines 837 - 839, Update the ownership
statement near manifest::from_str_named to scope it to production callers: say
production code uses the public entry points, while tests such as
manifest_env_tests.rs may construct injected EnvReader instances directly with
Arc::new.

[coderabbitai]

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@docs/users-guide.md`:
- Around line 470-472: Remove the contributor-focused paragraph from the user
guide, including the reference to the developers' guide and the Manifest env()
reader section. Keep the surrounding user and embedding workflow documentation
unchanged.

🪄 Autofix

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: 10708772-63f0-4bd5-a2eb-6fb3aa945aa1

📥 Commits

Reviewing files that changed from the base of the PR and between 30cf9c9 and b2262b5.

📒 Files selected for processing (1)

• docs/users-guide.md

🔗 Linked repositories identified

CodeRabbit considers these linked repositories for cross-repo context during reviews:

• leynos/rstest-bdd (auto-detected)
• leynos/ortho-config (auto-detected)
• leynos/whitaker (auto-detected)
• leynos/shared-actions (auto-detected)

[leynos]

@coderabbitai Have the following now been resolved?

tests/makefile_test_target.rs (1)

169-498: 📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy lift
Split the Rustflags contract tests into a sibling module.
Move the Rustflags helpers and tests into a dedicated module. Keep both Rust files

below 400 lines. Add a //! module comment to the new file.
As per coding guidelines, “No single Rust source file may exceed 400 lines.”

As per path instructions, “Files must not exceed 400 lines.”

Proposed module boundary

+#[path = "makefile_test_target/rustflags_contract.rs"]
+mod rustflags_contract;
+
-const RUSTFLAGS_PREFIX: &str = "RUSTFLAGS=\"";
-// Move the RustflagsCase helpers and Rustflags contract tests to the new module.

🤖 Detailed instructions

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In @tests/makefile_test_target.rs around lines 169 - 498, Move the
Rustflags-specific constants, RustflagsCase, helper functions, and related tests
from the current test module into a sibling Rust module, preserving their
behavior and test coverage. Add a module-level //! documentation comment to the
new file, and ensure both the original file and the new module remain below 400
lines.

Sources: Coding guidelines, Path instructions, Learnings

[leynos]

@coderabbitai Have the following failed checks now been resolved?

If further work is required, please provide an AI agent prompt for the remaining work to be done to address these failures.

Do not treat warnings as optional or aspirational. Where a change is out of scope for this PR, propose a GitHub issue unless one exists already. (Treat o11y, code safety, documentation and validation coverage as in scope).

Assessment against linked issues

Issue	Objective	Addressed	Explanation
#484	Introduce an injectable environment seam for the manifest env() helper, including removing direct std::env::var usage from src/manifest/mod.rs and supplying a mockable DefaultEnv at the manifest public boundary.	❌	The PR introduces env_var_with(name, read_env) and has env_var delegate to it via a closure, which does provide an injectable seam. However, env_var still calls std::env::var inside src/manifest/mod.rs via env_var_with(name,

[leynos]

On the pre-merge table's Developer Documentation warning: that row predates dce4728, which already updated netsuke-design.md §4.4 (it now describes env() reading through the injected EnvReader, names process_env_reader() and from_str_with_env(), and states the caller-owns-the-reader model) and corrected the developers' guide's from_str_named ownership claim in the same commit. The refreshed table on the next round should reflect it.

Separately, the makefile_test_target decomposition requested against this PR now lives in #526, stacked on this branch, closing #524.

[pandalump]

@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.

[codescene-access]

No quality gates enabled for this code.