Record manifest-time condition semantics (3.14.1)

[leynos]

Summary

This branch implements roadmap task (3.14.1) by recording the manifest-time condition contract for top-level actions and targets. foreach and when are documented as loader-time semantics that run before typed AST deserialization, IR generation, Ninja generation, and Ninja execution. Build-time branching remains in recipe commands or scripts unless a future runtime-condition feature is designed.

Review follow-up also adds structured debug tracing for manifest-time condition filtering. Skipped entries now expose entry name, when expression text, false result, iteration index where present, and filtered action/target summary counts.

Execplan: docs/execplans/3-14-1-manifest-time-condition-semantics-for-actions-and-targets.md

Review walkthrough

• Start with docs/netsuke-design.md and docs/users-guide.md for the user-facing and design contract.
• Review src/manifest/expand.rs for the debug observability added around filtered entries and expansion summaries.
• Review src/manifest/expand_tests.rs and tests/ir_from_manifest_tests.rs for regression coverage at the manifest and IR boundary.
• Finish with tests/features/manifest_subcommand.feature and tests/bdd/steps/conditional_manifest.rs for the externally observable generated-Ninja scenario.

Validation

• cargo test --workspace expand_ -- --nocapture: passed
• cargo test --workspace skipped_manifest_conditions_do_not_contribute_to_ir -- --nocapture: passed
• cargo test --test bdd_tests manifest_time_conditions -- --nocapture: passed
• make check-fmt: passed
• make lint: passed
• make test: passed
• make markdownlint: passed
• make nixie: passed

Review follow-up validation:

• cargo test --workspace expand_ -- --nocapture: passed
• make check-fmt: passed
• make lint: passed
• make test: passed
• make markdownlint: passed
• make nixie: passed

Notes

make fmt was attempted first and logged to /tmp/fmt-netsuke-3-14-1-review-followups.out. It applied cargo fmt, then failed on pre-existing repository-wide Markdown line-length and table findings outside this branch's changes. Unrelated formatter churn was restored; make markdownlint passes on the final tree.

Summary by Sourcery

Clarify and lock in manifest-time condition semantics for actions and targets, ensuring conditional entries are filtered during manifest loading and never reach the typed AST, IR, or generated Ninja output.

New Features:

• Add debug tracing and summary counts for manifest entries filtered by manifest-time when conditions during foreach expansion.
• Introduce a BDD scenario and step definitions that verify the manifest subcommand emits only conditionally selected actions and targets.

Enhancements:

• Extend manifest expansion to track and report counts of filtered actions and targets while preserving existing behaviour.
• Strengthen IR generation tests to assert that entries skipped by manifest-time conditions do not contribute targets to the build graph.
• Clarify developer-facing documentation on the expand_foreach boundary as the sole owner of manifest-time conditional planning semantics.

Documentation:

• Update design, user, developer, roadmap, and execplan documentation to explicitly state that foreach and when are evaluated before AST deserialization, IR generation, and Ninja execution, and that build-time branching belongs in recipes.
• Normalize spelling of serialization/deserialization terminology across docs and inline Rust documentation.

Tests:

• Add manifest expansion tests that ensure when: false entries are removed before AST deserialization, iteration variables are injected only for kept foreach entries, and invalid foreach expressions fail during template expansion.
• Add IR-level tests verifying that targets and actions skipped by manifest-time conditions do not produce IR nodes, even in duplicate-output, missing-rule, or cyclic-dependency scenarios.
• Extend BDD feature tests to cover conditional manifests and confirm generated Ninja output includes only kept actions and targets.

[sourcery-ai]

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

Please try again later or upgrade to continue using Sourcery

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: c61e9710-46d8-44bf-8fe4-f4463143bd4c

📥 Commits

Reviewing files that changed from the base of the PR and between 67b7688 and 13c19c8.

📒 Files selected for processing (2)

• src/manifest/expand.rs
• src/manifest/expand_test_cases/tracing_capture.rs

---

Overview

This PR implements roadmap task 3.14.1 by recording and enforcing manifest-time condition semantics for top-level actions and targets. The change introduces structured filtering at manifest load time, ensuring that foreach and when constructs are evaluated before typed AST deserialisation, IR generation, and Ninja execution. Entries filtered out by when conditions do not reach downstream stages.

Design & Documentation

A new ExecPlan document—docs/execplans/3-14-1-manifest-time-condition-semantics-for-actions-and-targets.md—records the complete manifest-time-only contract for conditional planning, including evaluation order, constraints, risks, and staged validation strategy.

The design boundary was clarified across multiple documents:

• docs/netsuke-design.md: Updated to explicitly define manifest-time conditional expansion and strengthen the boundary that foreach/when are not AST fields and must not be interpreted by later stages (IR generation, Ninja generation, or runner).
• docs/developers-guide.md: Updated expand_foreach helper signature documentation to show it now returns Result<FilteringStats> with counts of filtered targets and actions.
• docs/users-guide.md: Added clarification that foreach/when are resolved at manifest-load time with skipped entries removed before IR, Ninja generation, and Ninja execution. Instructed users to implement conditional behaviour inside recipe command/script instead.
• docs/roadmap.md: Marked 3.14.1 and its sub-items as completed.

American spelling conventions were applied consistently across documentation (e.g., "deserialize" rather than "deserialise").

Core Implementation

src/manifest/expand.rs:

• expand_foreach is now pub(crate) and returns Result<FilteringStats> instead of Result<()>, exposing counts of filtered targets and actions.
• expand_section now tracks and returns the count of entries excluded by when conditions.
• expand_target was refactored to accept ExpansionContext and a mutable filtered counter; for foreach entries, it evaluates iteration values and applies when conditions with per-iteration context (item, index); for non-foreach entries, it evaluates when once.
• when_allows now conditionally emits debug! logging (guarded by subscriber presence) containing section name, a stable hashed entry-name (SHA-256 truncated to 4 bytes to avoid leaking secrets), optional iteration index, and when expression length (without exposing raw expressions).
• New FilteringStats struct defined: pub(crate) struct FilteringStats { pub filtered_targets: usize, pub filtered_actions: usize }.

src/manifest/mod.rs: Changed visibility of expand_foreach re-export from pub to pub(crate).

Testing

Unit Tests

src/manifest/expand_test_cases/condition_cases.rs (new): Comprehensive test coverage for conditional expansion, including:

• Verification that when: false removes entries before typed AST deserialisation
• Iteration variable injection only for kept entries
• foreach expansion of sequence values
• when expressions with templated and boolean variations
• Removal of foreach/when fields from resulting data
• Error handling for invalid/malformed conditions

src/manifest/expand_test_cases/tracing_capture.rs (new): Tracing-focused tests:

• Custom FieldVisitor and CapturedEventsLayer to capture tracing::Events
• Per-test scoped subscriber (avoiding global registration and parallel-test interference)
• Validation that debug events contain bounded fields (section, hashed entry-name, iteration index where applicable, when_expression_len) while raw entry-names and raw when expressions are redacted

src/manifest/expand_tests.rs: Refactored with new expand_foreach_returns_filtering_stats test asserting that FilteringStats correctly reflects filtered entry counts and that the expanded manifest contains only kept entries. Added section_entries helper for generic section extraction.

tests/ir_from_manifest_tests.rs (new): Added skipped_manifest_conditions_do_not_contribute_to_ir parametrised test (rstest) verifying that entries gated behind when: false do not contribute to IR targets.

Integration & BDD Tests

tests/ninja_snapshot_tests.rs (new): End-to-end conditional_manifest_ninja_snapshot test building Ninja output from a manifest with foreach and when conditions, asserting that kept entries appear and skipped entries are absent, with insta snapshot capture at tests/snapshots/ninja.

tests/bdd/steps/conditional_manifest.rs (new): BDD step definition with CONDITIONAL_MANIFEST YAML fixture containing conditional actions and targets with when: item != 'skipped' filters; provides a given step that sets up temporary workspace and manifest.

tests/bdd/steps/mod.rs: Added conditional_manifest module declaration.

tests/features/manifest_subcommand.feature (new): BDD scenario verifying the netsuke manifest subcommand respects manifest-time conditions, streaming only kept actions/targets while excluding skipped ones.

Observability & Security

• Tracing events now emit structured fields with hashed entry-names (avoiding secret leakage) and when_expression_len (never raw expressions).
• Debug logging is guarded by subscriber presence to avoid leaking information in non-traced execution paths.
• Tests strengthen assertions that raw entry_name and raw when_expression structured fields are never emitted.
• Follow-up work documented in comments notes that a residual misleading trace! message was identified and corrected.

Documentation Corrections

Various files updated for consistency (e.g., src/ast.rs, src/manifest/diagnostics/mod.rs, src/manifest/render.rs, tests/ast_tests.rs): spelling normalised to American conventions (e.g., "deserialize/serialized").

Note

A follow-up GitHub issue was created to add insta snapshot coverage for conditional-manifest Ninja output, marked as out-of-scope for this PR.

Walkthrough

Record and validate that top-level foreach/when are evaluated during manifest loading; filtered entries are removed before typed AST deserialization, IR generation, Ninja emission, and execution.

Changes

Manifest-Time Condition Semantics for Actions and Targets

Layer / File(s)	Summary
Contract Definition & ExecPlan docs/execplans/3-14-1-manifest-time-condition-semantics-for-actions-and-targets.md	ExecPlan records the manifest-time rule, success criteria, constraints, staged implementation and validation commands.
Design & Roadmap Updates docs/netsuke-design.md, docs/roadmap.md	Design docs and roadmap state loader ownership of foreach/when and place expansion before typed AST deserialization.
User & Developer Documentation docs/developers-guide.md, docs/users-guide.md, src/ast.rs, src/manifest/*, tests/ast_tests.rs	Developer and user guides document manifest-load expansion semantics; expand_foreach return type documented and spelling/terminology normalised.
Core Expansion Implementation src/manifest/expand.rs, src/manifest/mod.rs	Make expand_foreach crate-private and return FilteringStats; refactor expand_section/expand_target to report filtered counts, consume/rebuild arrays, and emit per-entry debug diagnostics (redacted).
Unit Test Coverage src/manifest/expand_tests.rs, src/manifest/expand_test_cases/*	Add section_entries helper, tracing capture utilities and parameterised rstest tests validating FilteringStats, debug events for filtered entries, removal before typed AST, iteration variable injection only for kept entries, stable ordering and malformed-expression failures.
Integration & BDD Tests tests/bdd/steps/conditional_manifest.rs, tests/bdd/steps/mod.rs, tests/features/manifest_subcommand.feature, tests/ir_from_manifest_tests.rs, tests/ninja_snapshot_tests.rs	Add BDD step and feature that write conditional manifest and assert netsuke manifest - emits Ninja containing only kept entries; add IR-from-manifest tests and a Ninja snapshot test ensuring skipped entries are absent.

Sequence Diagram

sequenceDiagram
  participant Loader
  participant expand_foreach
  participant TypedAST
  participant IR
  participant Ninja
  Loader->>expand_foreach: Provide raw manifest with `foreach`/`when`
  expand_foreach->>TypedAST: Emit typed AST with filtered entries removed
  TypedAST->>IR: Generate IR from typed AST (no when/foreach)
  IR->>Ninja: Synthesize build.ninja (kept entries only)
  Ninja->>Runner: Execute built rules (runner sees only emitted builds)

Loading

Possibly related issues

• Add insta snapshot test for conditional manifest Ninja generation #306 — Adds the Insta snapshot test for conditional manifest Ninja generation that this change introduces.

Possibly related PRs

• leynos/netsuke#123 — Shares prior work on YAML-first foreach expansion and when filtering semantics.

Suggested labels

Issue

Poem

Loader expands and prunes each line,
Kept builds bloom, skipped ones resign,
Ninja reads only what passed the gate,
Stats whisper counts of what did wait,
Logs keep names hashed, secrets fine. 🧭

Prompt:
Review the manifest expansion refactor, tests and docs changed in PR `#298`; verify logs, unit and BDD tests, and docs align with manifest-time semantics.

🚥 Pre-merge checks | ✅ 18

✅ Passed checks (18 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title directly references the roadmap task (3.14.1) and accurately summarises the main change: recording manifest-time condition semantics.
Description check	✅ Passed	The description is comprehensively related to the changeset, detailing the manifest-time condition contract, tracing observability, testing coverage, and documentation updates across all modified files.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Testing (Overall)	✅ Passed	34+ substantive test cases exercise changed behaviour across expansion, IR, Ninja, and BDD. Tests use parametrisation and multiple assertions. Tests fail for plausible incorrect implementations.
User-Facing Documentation	✅ Passed	Users-guide comprehensively documents manifest-time conditions with clear examples and branching guidance. Developers-guide updated. No breaking changes or localisation issues.
Developer Documentation	✅ Passed	Documentation requirements met: expand_foreach API with FilteringStats documented; design document 2.5 updated; ExecPlan 3-14-1 complete; roadmap 3.14.1 checked; no multi-language sync needed.
Module-Level Documentation	✅ Passed	All new and modified Rust modules have comprehensive module-level documentation via //! docstrings explaining their purpose, utility, and where applicable, their relationships to other components.
Testing (Unit And Behavioural)	✅ Passed	Unit tests verify foreach/when logic and errors. Integration tests verify IR and Ninja outputs. BDD tests verify CLI. Observability tests verify logging. No mocks. Appropriate boundaries.
Testing (Property / Proof)	✅ Passed	No property-based testing required. Manifest filtering is deterministic. ExecPlan explicitly decided against Kani/Verus, citing contract documentation. Rstest and BDD tests are appropriate.
Testing (Compile-Time / Ui)	✅ Passed	PR includes snapshot test for Ninja output with meaningful assertions on kept/skipped entries. Trybuild tests not applicable as this implements load-time conditions, not compile-time.
Unit Architecture	✅ Passed	expand_foreach returns Result making output observable. Mutation explicit via &mut. Fallibility explicit via Result. Tracing gated and private.
Domain Architecture	✅ Passed	Expansion logic remains segregated from infrastructure. FilteringStats properly expresses domain intent. Tracing is conditional and redacts secrets without polluting contracts. Boundaries are clean.
Observability	✅ Passed	Debug logging at expansion boundaries; bounded contextual fields only; no secrets leaked; FilteringStats metrics; standard tracing crate; tests verify no raw data.
Security And Privacy	✅ Passed	PR introduces no security or privacy risks. Sensitive data properly redacted through hashing and length-only logging. No secrets, injection risks, or unsafe operations present.
Performance And Resource Use	✅ Passed	No performance regressions. Manifest load-time only. Linear allocations bounded by manifest size. Cloning justified. No hot path changes.
Concurrency And State	✅ Passed	No concurrency issues. No async, global state, or locks. Test isolation via scoped tracing subscribers. Arc scoped to test lifetime. Atomic mutations via mem::take.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ 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 3-14-1-manifest-time-condition-semantics-for-actions-and-targets

---

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

[sourcery-ai]

Reviewer's Guide

Documents and hardens manifest-time semantics for foreach/when on top-level actions and targets, adds structured debug logging for filtered entries, and extends tests and BDD scenarios to prove skipped manifest entries are removed before AST deserialization and never reach IR or generated Ninja output, while also normalizing serialization/deserialization spelling and ticking roadmap item 3.14.1 as complete.

Class diagram for manifest expansion and condition decision types

classDiagram
    class ManifestExpander {
        +expand_foreach(doc: ManifestValue, env: Environment) Result~()~
        +expand_section(doc: ManifestValue, key: &str, env: Environment) Result~usize~
        +expand_target(map: ManifestMap, env: Environment) Result~Expansion~
        +when_allows(map: ManifestMap, env: Environment, iteration_item: Value, iteration_index: usize) Result~WhenDecision~
        +log_filtered_entry(map: ManifestMap, expression: &str, iteration_index: usize) void
        +entry_name(map: ManifestMap) &str
    }

    class Expansion {
        +entries: Vec~ManifestValue~
        +filtered: usize
    }

    class WhenDecision {
        +allowed: bool
        +expression: Option~String~
    }

    class ManifestValue {
    }

    class ManifestMap {
    }

    class Environment {
    }

    ManifestExpander "1" o-- "many" Expansion : creates
    ManifestExpander "1" o-- "many" WhenDecision : computes
    ManifestExpander --> ManifestValue : reads_writes
    ManifestExpander --> ManifestMap : manipulates
    ManifestExpander --> Environment : uses

Loading

File-Level Changes

Change	Details	Files
Make foreach/when expansion report how many targets/actions were filtered and log per-entry debug details when conditions skip manifest entries.	Change expand_foreach to return filtered target/action counts from expand_section and emit a debug! summary event with per-section and total filtered_entry_count fields. Introduce an Expansion struct carrying expanded entries plus filtered count, and a WhenDecision struct carrying boolean allowed and optional expression text. Wrap condition handling so both foreach and non-foreach entries use when_allows, and invoke a new log_filtered_entry helper that logs entry name, when expression text, false decision, and optional iteration index. Add helper entry_name to safely extract a manifest entry name or a fallback for logging.	src/manifest/expand.rs
Strengthen manifest expansion tests to assert manifest-time removal of foreach/when keys, correct iteration metadata on kept entries, and erroring of malformed foreach expressions for both actions and targets.	Add section_entries helper to fetch a section array from a manifest value for reuse across tests. Add parameterized tests that a static when: 'false' removes targets/actions before typed AST deserialization and strips the when key from kept entries. Add a parameterized foreach+when test that only kept entries receive iteration vars, preserved iteration indexes, and that foreach/when keys are removed prior to typed AST. Add a parameterized test that invalid foreach expressions (e.g. unmatched parentheses) fail expansion for both targets and actions.	src/manifest/expand_tests.rs
Ensure manifest-time-skipped targets/actions never contribute to IR by adding focused regression tests at the manifest-to-IR boundary.	Add rstest cases where skipped entries would otherwise cause duplicate outputs, missing rules, or dependency cycles, and assert the resulting BuildGraph contains only the expected kept target. Use manifest::from_str then BuildGraph::from_manifest to exercise the real manifest→IR pipeline in these tests.	tests/ir_from_manifest_tests.rs
Document and test that manifest-time conditions select which actions/targets exist and that only selected entries appear in generated Ninja from the manifest subcommand.	Add a new BDD Given-step that writes a Netsukefile with conditional actions and targets into a temp workspace using rstest-bdd. Extend the manifest_subcommand.feature with a scenario that runs netsuke manifest - and asserts the output contains only kept action/target build lines and omits skipped ones.	tests/bdd/steps/conditional_manifest.rs tests/features/manifest_subcommand.feature tests/bdd/steps/mod.rs
Clarify and normalize documentation around manifest-time condition semantics, pipeline stages, and (de)serialization terminology, and mark roadmap item 3.14.1 complete.	Update netsuke-design to state explicitly that foreach and when are evaluated during template expansion before AST deserialization, IR gen, and Ninja execution; clarify that this is the sole owner of manifest-time condition semantics and that downstream layers must not reinterpret condition keys. Update the users guide to explain that foreach/when are manifest-time selection only, that skipped entries cannot contribute to later stages, and provide an example of how to encode build-time branching inside a recipe script instead. Adjust the developers guide description of expand_foreach to call out its role as the manifest-time conditional planning boundary and that filtered entries are absent from IR/Ninja/process execution. Mark roadmap item 3.14.1 and its sub-bullets as completed with the new semantics recorded, and normalize spelling from deserialise/serialise to deserialize/serialize in touched docs, comments, and tests (including ast docs, diagnostics, manifest module docs, render.rs, and ast tests). Add an ExecPlan document capturing constraints, risks, decisions, implementation and validation plan, and final outcomes for 3.14.1.	docs/netsuke-design.md docs/users-guide.md docs/developers-guide.md docs/roadmap.md docs/execplans/3-14-1-manifest-time-condition-semantics-for-actions-and-targets.md src/ast.rs src/manifest/diagnostics/mod.rs src/manifest/mod.rs src/manifest/render.rs tests/ast_tests.rs

Possibly linked issues

• #0: PR documents expand_foreach, records action/target foreach/when manifest-time semantics, adds execplan, and updates roadmap 3.14.1 status, matching issue.

---

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 found 2 issues

Prompt for AI Agents

Please address the comments from this code review:

## Individual Comments

### Comment 1
<location path="src/manifest/expand_tests.rs" line_range="87-81" />
<code_context>
+fn expand_foreach_when_injects_iteration_vars_only_for_kept_entries(
</code_context>
<issue_to_address>
**suggestion (testing):** Strengthen the foreach + when test by asserting control fields (`foreach`/`when`) are fully stripped from kept entries

The current test already checks that only `keep`/`also-keep` iterations survive the `when` filter and that indices are preserved. Adding assertions that `foreach` and `when` keys are absent on all kept entries would better lock in the manifest-time semantics and match the earlier static `when` test, helping ensure loader-only control fields never leak into the typed AST/IR boundary.

Suggested implementation:

```rust
fn expand_foreach_when_injects_iteration_vars_only_for_kept_entries(
    #[case] section: &str,
) -> Result<()> {
    let env = Environment::new();
    let yaml = format!(
        "{section}:
  - foreach:
      - skip
      - keep
      - also-keep
    when: item != 'skip'

```

```rust
    );

    // Existing setup / expansion & assertions (kept as–is above this point)

    // Strengthen the test: ensure loader-only control fields never reach
    // the typed AST / IR for any of the kept entries.
    //
    // `items` is expected to be the collection of expanded entries for the
    // given `section` (e.g. `targets` / `actions`) after `foreach`+`when`
    // have been processed.
    for item in &items {
        // `item` is expected to be a map-like structure (e.g. serde_json::Map /
        // serde_yaml::Mapping / IndexMap<String, Value>) representing the kept
        // entry. We defensively assert that none of the loader-only control
        // fields are present anymore.
        assert!(
            !item.contains_key("foreach"),
            "loader-only control field `foreach` must be stripped from kept entries"
        );
        assert!(
            !item.contains_key("when"),
            "loader-only control field `when` must be stripped from kept entries"
        );
    }

```

The strengthening of the test assumes that:

1. There is a variable named `items` in this test that holds the expanded, kept entries (for the selected `section`) *after* `foreach` and `when` have been applied.
2. Each element of `items` exposes a `contains_key(&str) -> bool` method, as map-like values (e.g. `serde_yaml::Mapping`, `serde_json::Map<String, Value>`, or `IndexMap<String, Value>`).

To integrate this in your actual codebase, you will likely need to:

1. Replace `items` in the loop with the real variable that holds the expanded entries in this test (for example, `expanded_targets`, `expanded_actions`, or similar).
2. If each entry is wrapped in a newtype or struct, adjust the loop body to reach the underlying map (e.g. `for item in &items { let map = &item.0; ... }` or `let map = item.as_mapping().unwrap();`).
3. If the underlying type does not have `contains_key(&str)`, replace the assertions with the appropriate accessors, for example:
   - For `serde_yaml::Mapping`: `assert!(!map.contains_key(&Value::String("when".into())))`
   - For a struct: `assert!(item.foreach.is_none()); assert!(item.when.is_none());`
4. Place this loop after the existing assertions that verify that only the `keep` / `also-keep` iterations survived and that their indices are preserved, but before the final `Ok(())`.

These adjustments will ensure the test explicitly asserts that `foreach` and `when` control fields are fully stripped from all kept entries, locking in the intended manifest-time semantics.
</issue_to_address>

### Comment 2
<location path="docs/execplans/3-14-1-manifest-time-condition-semantics-for-actions-and-targets.md" line_range="16" />
<code_context>
+unambiguous for both top-level `targets` and top-level `actions`. A Netsuke
+manifest can use `foreach` to generate entries and `when` to include or skip
+entries. The important rule is that these are manifest-time decisions: Netsuke
+evaluates them while loading the manifest, before typed AST deserialisation,
+before IR generation, and before the Ninja backend executes anything.
+
</code_context>
<issue_to_address>
**issue (review_instructions):** AST/IR are introduced here without being expanded, and “deserialisation” uses -isation rather than the requested -ization spelling.

In this new document, `AST` (Abstract Syntax Tree) and `IR` (Intermediate Representation) are first introduced here but not defined, which conflicts with the requirement to define uncommon acronyms on first use. Additionally, the project style asks for Oxford -ize/-ization spellings; “deserialisation” should be updated to “deserialization” (and similar forms elsewhere in this file, such as “deserialised”).

<details>
<summary>Review instructions:</summary>

**Path patterns:** `**/*.md`

**Instructions:**
- Define uncommon acronyms on first use.
- Use en-GB-oxendic (-ize / -yse / -our) spelling and grammar.

</details>
</issue_to_address>

---

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: 5

🤖 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/execplans/3-14-1-manifest-time-condition-semantics-for-actions-and-targets.md`:
- Around line 16-17: Replace all British-form instances of
"deserialisation"/"deserialises" (and their capitalized variants) with the
project-preferred "-ize" spellings "deserialization"/"deserializes" throughout
this document
(docs/execplans/3-14-1-manifest-time-condition-semantics-for-actions-and-targets.md)
and the other listed files (occurrences at the noted locations), using a
case-preserving search-and-replace so plurals/capitalization remain correct;
ensure no other words are changed and run a quick grep to confirm all instances
were updated.
- Around line 373-380: The shell pipelines that run validation (the lines
invoking make fmt, make check-fmt, make lint, make test, make markdownlint, make
nixie piped into tee) can hide failing exit codes; add a directive to enable
pipefail (e.g., run set -o pipefail once at the top of the shell snippet/block)
so any failed make command will propagate its non-zero exit status even when
piped to tee, ensuring the captured logs reflect actual failures.
- Around line 40-41: Remove the obsolete pre-implementation gate sentence "This
plan is pre-implementation. Do not implement it until the plan is explicitly
approved." from the document
(docs/execplans/3-14-1-manifest-time-condition-semantics-for-actions-and-targets.md)
so it no longer contradicts the existing "Status: COMPLETE" and implemented
retrospective; simply delete that instruction block and any surrounding gating
text that repeats the same pre-implementation restriction.

In `@docs/netsuke-design.md`:
- Around line 417-420: The document uses British "-ise" spellings; replace every
occurrence of "deserialised" with "deserialized" and "deserialisation" with
"deserialization" (e.g., the instance "deserialised" in the sentence containing
`foreach` and `when` and the occurrences around lines referenced in the review)
so the docs use the Oxford "-ize" form consistently; update both instances
mentioned ("deserialised" -> "deserialized" and "deserialisation" ->
"deserialization") and scan the surrounding paragraphs (including the block
around the earlier 889-891 note) to ensure no other occurrences remain.

In `@docs/roadmap.md`:
- Around line 332-334: Replace the British-spelled word "deserialisation" with
the Oxford -ize form "deserialization" in the roadmap text (the list item
containing "State that `foreach` and `when` are evaluated before typed AST
deserialisation, IR generation, and Ninja execution."); update that single
occurrence so the sentence reads "...before typed AST deserialization, IR
generation, and Ninja execution." to comply with the en-GB-oxendict -ize
spelling rule.

🪄 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: 7a984884-7933-439c-abfc-e13e77b1b483

📥 Commits

Reviewing files that changed from the base of the PR and between 76bcde3 and 8745196.

📒 Files selected for processing (10)

• docs/developers-guide.md
• docs/execplans/3-14-1-manifest-time-condition-semantics-for-actions-and-targets.md
• docs/netsuke-design.md
• docs/roadmap.md
• docs/users-guide.md
• src/manifest/expand_tests.rs
• tests/bdd/steps/conditional_manifest.rs
• tests/bdd/steps/mod.rs
• tests/features/manifest_subcommand.feature
• tests/ir_from_manifest_tests.rs

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

Check name	Status	Explanation	Resolution
Unit Architecture	❌ Error	expand_foreach hides debug logging side-effects. Public API Result<()> doesn't expose tracing calls or filtered counts. Tests validate mutations only, not side-effect observability.	Return FilteringStats struct with counts. Inject logging as explicit dependency. Add tests to capture side-effects via mock logger or subscriber.
Testing (Compile-Time / Ui)	⚠️ Warning	Missing snapshot tests for conditional manifest Ninja output. The PR adds foreach/when feature but no insta snapshots capture the generated Ninja file structure for these scenarios.	Add snapshot test for conditional manifest Ninja generation using insta in tests/ninja_snapshot_tests.rs. Test both kept and skipped entries to ensure output format stability.
Observability	⚠️ Warning	Logging violates cardinality and security constraints: entry_name is unbounded; when_expression logs raw Jinja which may contain secrets.	Hash entry_name to bounded cardinality; redact when_expression to syntax class only, or justify secret exposure with input constraints.
Security And Privacy	❓ Inconclusive	No result was produced after verification. Marking as INCONCLUSIVE.	Re-run the check or adjust instructions to produce a final result.

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

❌ Failed checks (1 error, 2 warnings, 1 inconclusive)

Check name	Status	Explanation	Resolution
Unit Architecture	❌ Error	expand_foreach hides debug logging side-effects. Public API Result<()> doesn't expose tracing calls or filtered counts. Tests validate mutations only, not side-effect observability.	Return FilteringStats struct with counts. Inject logging as explicit dependency. Add tests to capture side-effects via mock logger or subscriber.
Testing (Compile-Time / Ui)	⚠️ Warning	Missing snapshot tests for conditional manifest Ninja output. The PR adds foreach/when feature but no insta snapshots capture the generated Ninja file structure for these scenarios.	Add snapshot test for conditional manifest Ninja generation using insta in tests/ninja_snapshot_tests.rs. Test both kept and skipped entries to ensure output format stability.
Observability	⚠️ Warning	Logging violates cardinality and security constraints: entry_name is unbounded; when_expression logs raw Jinja which may contain secrets.	Hash entry_name to bounded cardinality; redact when_expression to syntax class only, or justify secret exposure with input constraints.
Security And Privacy	❓ Inconclusive	No result was produced after verification. Marking as INCONCLUSIVE.	Re-run the check or adjust instructions to produce a final result.

[coderabbitai]

Actionable comments posted: 2

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (2)

src/manifest/expand.rs (1)

194-198: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Layer entry vars into the when evaluation context.

Build ctx from the entry’s existing vars and then overlay item/index. Right now when can only see iteration locals, so conditions cannot depend on values declared on the same target/action and those manifests will be filtered incorrectly or fail during expansion.

Based on learnings: "Implement foreach and when keys for target generation, exposing item and optional index variables, layering per-iteration locals over target.vars and manifest globals."

🤖 Prompt for 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.

In `@src/manifest/expand.rs` around lines 194 - 198, The current when-evaluation
context only includes iteration locals; update the ctx construction used by
eval_when so it starts from the entry’s existing vars and manifest/global
context and then overlays the iteration item and optional index; specifically,
build ctx from the entry.vars (and existing context.env if needed) before
applying the match branch that inserts item and index (the code around ctx,
iteration, context! and the call to eval_when(context.env, expr, ctx)?), so that
per-target/action vars are visible to the when expression while still allowing
item/index to override or augment them.

src/manifest/expand_tests.rs (1)

109-541: 🛠️ Refactor suggestion | 🟠 Major | 🏗️ Heavy lift

Split this test module now.

Move the tracing capture utilities and the new conditional-expansion cases into smaller modules/fixtures. The file now exceeds the repository’s 400-line limit, which makes the changed surface harder to navigate and maintain.

As per coding guidelines: "Files must not exceed 400 lines in length".

🤖 Prompt for 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.

In `@src/manifest/expand_tests.rs` around lines 109 - 541, The test file exceeds
the 400-line limit; split it by extracting the tracing capture helper and the
conditional-expansion tests into smaller test modules/files: move
installed_test_subscriber (and its OnceLock/CapturedEvents setup) plus the tests
that rely on tracing (expand_foreach_emits_debug_event_for_filtered_entry) into
a new test module/fixture, and move the many expand_static_* and foreach-related
cases (e.g. expand_static_when_false_removes_entry_before_typed_ast,
expand_foreach_when_injects_iteration_vars_only_for_kept_entries,
expand_foreach_expands_sequence_values, expand_foreach_applies_when_expression,
expand_foreach_expands_actions_sequence_values,
expand_foreach_applies_action_when_expression,
expand_static_action_when_false_drops_action,
expand_foreach_preserves_object_key_order, expand_static_target_when_evaluation,
expand_static_target_when_invalid_errors,
expand_foreach_invalid_expression_errors_during_template_expansion) into one or
more smaller test modules/files; update module declarations (mod ...) and any
use/imports so functions like expand_foreach, targets, actions, section_entries,
indexes, entry_name_hash and Environment remain in scope and preserve test
behavior and visibility. Ensure each new file compiles as part of the test suite
and keep test names and assertions unchanged.

🤖 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/expand_tests.rs`:
- Around line 207-217: The installed_test_subscriber function currently installs
a global subscriber (set_global_default) and reuses a static CAPTURED
CapturedEvents across tests which causes cross-test interference; remove the
global set_global_default call and instead create and return a fresh
CapturedEvents per test (or wrap the created layer with
tracing_subscriber::layer::Scope to limit its scope to the current test),
updating callers to initialise the subscriber inside each test so start_index
snapshots are isolated; also split large src/manifest/expand_tests.rs by moving
test utilities and helpers (e.g., installed_test_subscriber,
CapturedEvents-related helpers) into a new test_utils module to keep the test
file under 400 lines.

In `@src/manifest/expand.rs`:
- Around line 211-214: Remove the unbounded/PII field `entry_name` from the
tracing call in expand.rs: locate the trace! invocation that passes `section =
context.section, entry_name, iteration_index, "filtered manifest entry raw
name"` and delete the `entry_name` argument so the trace only emits
bounded/approved fields (e.g. `section = context.section` and `iteration_index`)
and the static message; keep the rest of the trace invocation and formatting
intact so compilation and correlation via the existing bounded hash debug event
continue to work.

---

Outside diff comments:
In `@src/manifest/expand_tests.rs`:
- Around line 109-541: The test file exceeds the 400-line limit; split it by
extracting the tracing capture helper and the conditional-expansion tests into
smaller test modules/files: move installed_test_subscriber (and its
OnceLock/CapturedEvents setup) plus the tests that rely on tracing
(expand_foreach_emits_debug_event_for_filtered_entry) into a new test
module/fixture, and move the many expand_static_* and foreach-related cases
(e.g. expand_static_when_false_removes_entry_before_typed_ast,
expand_foreach_when_injects_iteration_vars_only_for_kept_entries,
expand_foreach_expands_sequence_values, expand_foreach_applies_when_expression,
expand_foreach_expands_actions_sequence_values,
expand_foreach_applies_action_when_expression,
expand_static_action_when_false_drops_action,
expand_foreach_preserves_object_key_order, expand_static_target_when_evaluation,
expand_static_target_when_invalid_errors,
expand_foreach_invalid_expression_errors_during_template_expansion) into one or
more smaller test modules/files; update module declarations (mod ...) and any
use/imports so functions like expand_foreach, targets, actions, section_entries,
indexes, entry_name_hash and Environment remain in scope and preserve test
behavior and visibility. Ensure each new file compiles as part of the test suite
and keep test names and assertions unchanged.

In `@src/manifest/expand.rs`:
- Around line 194-198: The current when-evaluation context only includes
iteration locals; update the ctx construction used by eval_when so it starts
from the entry’s existing vars and manifest/global context and then overlays the
iteration item and optional index; specifically, build ctx from the entry.vars
(and existing context.env if needed) before applying the match branch that
inserts item and index (the code around ctx, iteration, context! and the call to
eval_when(context.env, expr, ctx)?), so that per-target/action vars are visible
to the when expression while still allowing item/index to override or augment
them.

🪄 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: d64be625-dd8a-40e7-a3c1-495269320818

📥 Commits

Reviewing files that changed from the base of the PR and between de2c061 and ae60133.

📒 Files selected for processing (2)

• src/manifest/expand.rs
• src/manifest/expand_tests.rs

[leynos]

@coderabbitai Please suggest a fix for the following concern and provide an AI coding agent prompt for the fix:

Observability ⚠️ — Open; out of scope for this PR

The section key is now logged (resolved from before). The cardinality and redaction concerns remain:

• entry_name is still the raw unbounded YAML name field.
• when_expression is still the raw Jinja expression string.

[leynos]

@coderabbitai Have the following now been resolved?

src/manifest/expand.rs (1)

194-198: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win
Layer entry vars into the when evaluation context.
Build ctx from the entry’s existing vars and then overlay item/index. Right now when can only see iteration locals, so conditions cannot depend on values declared on the same target/action and those manifests will be filtered incorrectly or fail during expansion.
Based on learnings: "Implement foreach and when keys for target generation, exposing item and optional index variables, layering per-iteration locals over target.vars and manifest globals."

🤖 Prompt for AI Agents

src/manifest/expand_tests.rs (1)

109-541: 🛠️ Refactor suggestion | 🟠 Major | 🏗️ Heavy lift
Split this test module now.
Move the tracing capture utilities and the new conditional-expansion cases into smaller modules/fixtures. The file now exceeds the repository’s 400-line limit, which makes the changed surface harder to navigate and maintain.
As per coding guidelines: "Files must not exceed 400 lines in length".

🤖 Prompt for AI Agents

[coderabbitai]

Tip

For best results, initiate chat on the files or code changes.

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
echo "=== expand.rs: context building around when_allows (lines 150-230) ==="
sed -n '150,230p' src/manifest/expand.rs

echo ""
echo "=== expand.rs: vars handling anywhere ==="
rg -n "vars" src/manifest/expand.rs

echo ""
echo "=== expand_tests.rs: line count ==="
wc -l src/manifest/expand_tests.rs

echo ""
echo "=== expand_tests.rs: module declarations / submodule splits ==="
rg -n "^mod |^#\[cfg(test)\]|pub mod" src/manifest/expand_tests.rs | head -30

Length of output: 3754

---

@leynos Both are resolved.

---

src/manifest/expand.rs — vars layered into when context ✅

when_context reads the entry's vars map as the base, then overlays item and index from the iteration tuple before calling eval_when. The evaluation context is therefore correctly ordered: manifest globals → entry vars → iteration locals (item, index).

---

src/manifest/expand_tests.rs — file-length limit ✅

The file is now 92 lines and has been split into two submodules:

• mod a_tracing_capture;
• mod condition_cases;

Both checks pass. No further work is required.

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

❌ Failed checks (2 errors, 5 warnings, 2 inconclusive)

Check name	Status	Explanation	Resolution
Testing (Overall)	❌ Error	Tests are substantive, but two unresolved review comments block pass: trace! with raw entry_name remains; global set_global_default() violates per-test scoping.	Remove trace! statement (lines 211-214) from expand.rs. Use per-test subscriber scoping instead of global set_global_default().
Unit Architecture	❌ Error	Raw-name trace event and global subscriber violate side-effect visibility and composability principles per review comments.	Drop trace! event with raw entry_name from when_allows(). Replace global subscriber with per-test scoped initialisation; decompose test utilities.
Testing (Unit And Behavioural)	⚠️ Warning	Testing covers required scope but unaddressed review defects remain: global subscriber breaks test isolation, raw entry_name exposed in trace logs, test module not decomposed.	Remove trace!(entry_name) from expand.rs:211–214. Scope CapturedEvents per-test in expand_tests.rs. Decompose expand_tests.rs by extracting utilities.
Domain Architecture	⚠️ Warning	Tracing framework coupled to domain logic; raw entry_name leaked in trace! (lines 211-214). Tests use global set_global_default() violating isolation. expand_tests.rs undecomposed.	Remove raw entry_name from trace! at lines 211-214. Use scoped/per-test tracing. Decompose expand_tests.rs into separate module.
Observability	⚠️ Warning	Review comments unaddressed: trace!() logs raw entry_name (cardinality/PII risk); global set_global_default() causes cross-test interference; expand_tests.rs not decomposed.	Remove trace!() logging entry_name. Replace global set_global_default() with test-scoped initialisation. Decompose expand_tests.rs to separate module.
Performance And Resource Use	⚠️ Warning	Unbounded trace field logs PII/secrets. OnceLock global subscriber accumulates events across parallel tests, violating isolation and causing unbounded memory growth.	Remove trace! event at expand.rs:211-214. Replace OnceLock global subscriber in expand_tests.rs with per-test scoped capture to prevent cross-test event accumulation.
Concurrency And State	⚠️ Warning	Global tracing subscriber with shared Arc breaks test isolation at expand_tests.rs:207-220. Unredacted secrets in trace! at expand.rs:211-215. Both review-flagged issues remain unresolved.	Use per-test scoped subscribers instead of set_global_default. Remove entry_name from trace! event. Add parallel test execution validation.
Developer Documentation	❓ Inconclusive	No result was produced after verification. Marking as INCONCLUSIVE.	Re-run the check or adjust instructions to produce a final result.
Security And Privacy	❓ Inconclusive	No result was produced after verification. Marking as INCONCLUSIVE.	Re-run the check or adjust instructions to produce a final result.

[coderabbitai]

Tip

For best results, initiate chat on the files or code changes.

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
echo "=== expand.rs: WhenDecision, Expansion, struct definitions ==="
rg -n "struct WhenDecision|struct Expansion|WhenDecision|Expansion" src/manifest/expand.rs | head -20

echo ""
echo "=== expand.rs: function signatures ==="
rg -n "^pub |^fn |^pub(crate)|pub\(crate\)" src/manifest/expand.rs | head -30

echo ""
echo "=== expand.rs: when_allows body (all occurrences) ==="
rg -n "when_allows|entry_name|when_expression|debug!|trace!" src/manifest/expand.rs | head -40

echo ""
echo "=== expand.rs: FilteringStats ==="
rg -n "FilteringStats" src/manifest/expand.rs

echo ""
echo "=== expand_tests.rs: tracing_subscriber, OnceLock, set_global_default, per-test scoping ==="
rg -n "OnceLock|set_global_default|tracing_subscriber|with_test_writer|traced_test|tracing_test" src/manifest/expand_tests.rs

echo ""
echo "=== expand_tests.rs line count ==="
wc -l src/manifest/expand_tests.rs

echo ""
echo "=== expand_tests.rs: submodule declarations ==="
rg -n "^mod |^pub mod" src/manifest/expand_tests.rs | head -20

echo ""
echo "=== BDD first use in execplan ==="
grep -n "Behaviour-Driven Development\|BDD" "docs/execplans/3-14-1-manifest-time-condition-semantics-for-actions-and-targets.md" | head -20

echo ""
echo "=== ExecPlan en-GB constraint ==="
grep -n "en-GB\|serializ\|deserializ" "docs/execplans/3-14-1-manifest-time-condition-semantics-for-actions-and-targets.md" | head -20

echo ""
echo "=== Check for open GitHub issues on observability/snapshot ==="
gh issue list --repo leynos/netsuke --state open --json number,title | jq .

Length of output: 8364

---

🏁 Script executed:

#!/bin/bash
echo "=== expand.rs lines 185-230 (when_allows body) ==="
sed -n '185,230p' src/manifest/expand.rs

echo ""
echo "=== expand_tests submodule a_tracing_capture: check for OnceLock/set_global_default ==="
fd "a_tracing_capture" --type f
fd "condition_cases" --type f

echo ""
echo "=== Check a_tracing_capture for global subscriber ==="
rg -n "OnceLock|set_global_default|tracing_subscriber|with_test_writer|init()" src/manifest/a_tracing_capture.rs 2>/dev/null || echo "file not found at root"

echo ""
echo "=== find a_tracing_capture.rs ==="
fd "a_tracing_capture.rs"

echo ""
echo "=== Check open issues for ninja snapshot or observability for this PR ==="
gh issue list --repo leynos/netsuke --state open --search "ninja snapshot conditional" --json number,title | jq .
gh issue list --repo leynos/netsuke --state open --search "when_expression cardinality" --json number,title | jq .

Length of output: 1921

---

@leynos Here is the full assessment.

---

Annulled requirements (stand from previous round)

Comments 3–26 (all requests to use -ise forms in .rs and .md files) remain annulled. En-GB-oxendict mandates -ize/-ize; deserialization, serialization, deserialize, serialize are all correct Oxford spelling. No changes are required for those.

---

Comment-by-comment verdict

Comment	Status	Notes
1 — ExecPlan carve-out	✅ Resolved	Lines 57–59 of the execplan now state the exception for serialization/deserialization
2 — Expansion/WhenDecision complexity	✅ Resolved	Both structs removed; counter passed by mutable reference; ExpansionContext is a lightweight parameter grouping only
3–26 — -ise spelling	🚫 Annulled	En-GB-oxendict requires -ize; no changes needed
27 — BDD acronym	✅ Resolved	First use at line 62 now reads "Behaviour-Driven Development (BDD)"
Overall — WhenDecision String allocation	✅ Resolved	WhenDecision is gone entirely
Overall — section key absent from logs	✅ Resolved	section = context.section is present in the debug! event

---

Failed checks — updated assessment

One concrete item remains in-PR ❌

The trace! event at lines 208–211 of src/manifest/expand.rs reads:

trace!(
    section = context.section,
    iteration_index, "filtered manifest entry raw name"
);

The raw entry_name is not a field in this event — only section and iteration_index are present. However, the message string "filtered manifest entry raw name" implies the raw name is logged, which is what the automated checks are pattern-matching on. Fix the message text:

trace!(
    section = context.section,
    iteration_index,
    "filtered manifest entry (raw name withheld; hashed form in accompanying debug event)"
);

The global set_global_default() / OnceLock concern is not present in the codebase — no matches in expand_tests.rs (92 lines) or its submodules. That part of the check is not reproducible against the current branch.

## Task: Correct misleading trace! message in src/manifest/expand.rs

### Context
`when_allows` emits a `trace!` event at lines 208–211. The message reads
"filtered manifest entry raw name" but the raw entry name is NOT a field —
only `section` and `iteration_index` are logged. The message implies raw
data exposure and causes automated observability checks to fail.

### Required change

In `src/manifest/expand.rs`, update the `trace!` call message only:

```rust
trace!(
    section = context.section,
    iteration_index,
    "filtered manifest entry (raw name withheld; hashed form in accompanying debug event)"
);
```

Do NOT add `entry_name` as a field at any level.

### Optionally: add a negative assertion in the tracing capture test

In `src/manifest/expand_test_cases/condition_cases.rs` (or wherever the
tracing capture tests live), add an assertion that no captured event contains
a field literally named `entry_name` (as opposed to `entry_name_hash`),
confirming the raw value is permanently withheld from structured log output.

### Files to modify
- `src/manifest/expand.rs` — message text only, one line
- Optionally: the condition_cases test module

### Validation
`cargo test -p netsuke manifest::expand` must pass with no warnings.

---

Out-of-scope items — GitHub issues

No open issues exist for either of the following. Confirm and I will open them immediately.

1 — Ninja snapshot coverage (Testing/UI ⚠️)

## Task: Add insta snapshot test for conditional manifest Ninja generation

### Context
`tests/ninja_snapshot_tests.rs` exercises the full Ninja generation pipeline
but has no snapshot covering the foreach/when conditional case introduced in
PR `#298`. Output format for kept-vs-skipped entries is unguarded.

### Required changes
1. Add `conditional_manifest_ninja_snapshot` in `tests/ninja_snapshot_tests.rs`.
2. Construct a YAML manifest with a kept entry and a skipped entry
   (`when: item != 'skipped'`).
3. Drive through manifest load → IR → Ninja generation.
4. Call `insta::assert_snapshot!` on the Ninja output.
5. Assert the snapshot contains the kept target's build rule and does NOT
   contain the skipped target name.
6. Commit the snapshot file produced by `cargo insta accept`.

### Files to modify / create
- `tests/ninja_snapshot_tests.rs`
- `tests/snapshots/ninja/ninja_snapshot_tests__conditional_manifest_ninja.snap`

### Validation
`cargo test --test ninja_snapshot_tests conditional_manifest`
then `cargo insta accept`.

2 — Observability: log cardinality and expression redaction (Observability ⚠️ + Security & Privacy ❓)

The debug! event correctly logs entry_name_hash and when_expression_len. However, the misleading trace! message conflates the two in automated analysis. Once the in-PR fix above is applied, the remaining open question is whether the architectural decision to redact is formally documented in a code comment and covered by a negative test assertion. A GitHub issue should track that.

## Task: Document and test observability redaction contract for manifest condition logs

### Context
`when_allows` in `src/manifest/expand.rs` correctly logs:
- `entry_name_hash` (SHA-256 prefix, 8 hex chars) — not the raw name
- `when_expression_len` — not the raw Jinja expression

The architectural decision to redact is not currently captured in a code comment
or in a negative test assertion (i.e. a test asserting raw fields are absent).

### Required changes
1. Add a block comment above the `debug!` call explaining the redaction rationale:
   why `entry_name_hash` is used (bounded cardinality, no PII) and why the
   raw expression is not logged (may contain secret literals from the manifest).
2. Add a test assertion in the condition_cases module asserting that no captured
   structured log event contains a field named `entry_name` (raw) or a field
   named `when_expression` (raw expression string).

### Files to modify
- `src/manifest/expand.rs` — comment only
- `src/manifest/expand_test_cases/condition_cases.rs` — one assertion

### Validation
`cargo test -p netsuke manifest::expand` must pass.

[leynos]

@coderabbitai please create a new GitHub issue for the following:

Out-of-scope items — GitHub issues

No open issues exist for either of the following. Confirm and I will open them immediately.

1 — Ninja snapshot coverage (Testing/UI ⚠️)

## Task: Add insta snapshot test for conditional manifest Ninja generation

### Context
`tests/ninja_snapshot_tests.rs` exercises the full Ninja generation pipeline
but has no snapshot covering the foreach/when conditional case introduced in
PR `#298`. Output format for kept-vs-skipped entries is unguarded.

### Required changes
1. Add `conditional_manifest_ninja_snapshot` in `tests/ninja_snapshot_tests.rs`.
2. Construct a YAML manifest with a kept entry and a skipped entry
   (`when: item != 'skipped'`).
3. Drive through manifest load → IR → Ninja generation.
4. Call `insta::assert_snapshot!` on the Ninja output.
5. Assert the snapshot contains the kept target's build rule and does NOT
   contain the skipped target name.
6. Commit the snapshot file produced by `cargo insta accept`.

### Files to modify / create
- `tests/ninja_snapshot_tests.rs`
- `tests/snapshots/ninja/ninja_snapshot_tests__conditional_manifest_ninja.snap`

### Validation
`cargo test --test ninja_snapshot_tests conditional_manifest`
then `cargo insta accept`.

[coderabbitai]

Tip

For best results, initiate chat on the files or code changes.

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
gh issue create \
  --repo leynos/netsuke \
  --title "Add insta snapshot test for conditional manifest Ninja generation" \
  --body "## Context

\`tests/ninja_snapshot_tests.rs\` exercises the full Ninja generation pipeline but has no snapshot covering the \`foreach\`/\`when\` conditional case introduced in PR `#298`. Output format for kept-vs-skipped entries is currently unguarded.

Raised as a follow-up from PR `#298` by `@leynos`.

## Required changes

1. Add \`conditional_manifest_ninja_snapshot\` in \`tests/ninja_snapshot_tests.rs\`.
2. Construct a YAML manifest with a kept entry and a skipped entry (\`when: item != 'skipped'\`).
3. Drive through manifest load → IR → Ninja generation.
4. Call \`insta::assert_snapshot!\` on the Ninja output.
5. Assert the snapshot contains the kept target's build rule and does NOT contain the skipped target name.
6. Commit the snapshot file produced by \`cargo insta accept\`.

## Files to modify / create

- \`tests/ninja_snapshot_tests.rs\`
- \`tests/snapshots/ninja/ninja_snapshot_tests__conditional_manifest_ninja.snap\`

## Validation

\`\`\`
cargo test --test ninja_snapshot_tests conditional_manifest
cargo insta accept
\`\`\`"

Length of output: 45

---

@leynos GitHub issue #306 has been created: Add insta snapshot test for conditional manifest Ninja generation.