Improve expand_foreach test coverage: foreach removal, edge cases, and Jinja filters

[coderabbitai]

Background

During review of PR #294, the following gaps in src/manifest/expand_tests.rs were identified. Addressing them is out of scope for that documentation-and-expansion PR.

Work Required

File to edit: src/manifest/expand_tests.rs

1. In expand_foreach_expands_sequence_values (Test 1), after confirming targets.len() == 2, add an assertion for each expanded target that its object map does not contain the key "foreach".

2. In expand_foreach_applies_when_expression (Test 2), after confirming the filtered count, add an assertion for each remaining target that its object map does not contain the key "foreach".

3. In expand_foreach_applies_action_when_expression (Test 4), after confirming the filtered count, add an assertion for each remaining action that its object map does not contain the key "foreach".

4. Add a test expand_foreach_empty_foreach_produces_no_entries that passes a target with foreach: [] and asserts the expanded targets slice is empty.

5. Add a test expand_foreach_non_object_entry_is_passed_through that passes a targets array containing a bare string value (non-object) and asserts it survives unexpanded.

6. Add a test expand_foreach_iteration_vars_do_not_overwrite_globals that passes a target whose vars map already contains a key "item" with a value, and asserts the iteration-injected item value (from foreach) takes precedence for each expanded entry (or whichever precedence rule expand_section implements — assert the actual observed behaviour).

7. Add a test expand_foreach_jinja_filter_in_name that passes a target whose name uses a Jinja filter (e.g. {{ item | upper }}) with a foreach list of lowercase strings and asserts the expanded names are uppercased correctly.

Do not remove or modify any existing tests.

AI Agent Prompt

File to edit: src/manifest/expand_tests.rs

1. In `expand_foreach_expands_sequence_values` (Test 1), after confirming
   `targets.len() == 2`, add an assertion for each expanded target that its
   object map does not contain the key `foreach`.

2. In `expand_foreach_applies_when_expression` (Test 2), after confirming
   the filtered count, add an assertion for each remaining target that its
   object map does not contain the key `foreach`.

3. In `expand_foreach_applies_action_when_expression` (Test 4), after
   confirming the filtered count, add an assertion for each remaining action
   that its object map does not contain the key `foreach`.

4. Add a test `expand_foreach_empty_foreach_produces_no_entries` that
   passes a target with `foreach: []` and asserts the expanded `targets`
   slice is empty.

5. Add a test `expand_foreach_non_object_entry_is_passed_through` that
   passes a `targets` array containing a bare string value (non-object) and
   asserts it survives unexpanded.

6. Add a test `expand_foreach_iteration_vars_do_not_overwrite_globals` that
   passes a target whose `vars` map already contains a key `item` with a
   value, and asserts the iteration-injected `item` value (from `foreach`)
   takes precedence for each expanded entry (or whichever precedence rule
   `expand_section` implements — assert the actual observed behaviour).

7. Add a test `expand_foreach_jinja_filter_in_name` that passes a target
   whose `name` uses a Jinja filter (e.g. `{{ item | upper }}`) with a
   `foreach` list of lowercase strings and asserts the expanded names are
   uppercased correctly.

Do not remove or modify any existing tests.

Raised By

@leynos — identified during review of PR #294 (#294).

Rescope (2026-08-12)

The tests have since moved from src/manifest/expand_tests.rs into src/manifest/expand_test_cases/. Progress against the numbered items:

• Items 1–3 (foreach-key-removal assertions): done — !map.contains_key("foreach") assertions exist in expand_test_cases/action_condition_cases.rs:27 and condition_cases.rs:88, and the proptest foreach_key_is_removed_from_all_entries (property_cases.rs:97, merged via Add proptest property tests for expand_foreach (#295) #359) covers the invariant generally.
• Item 6 (iteration vars vs entry vars): effectively covered by expand_foreach_when_item_overrides_entry_vars (condition_cases.rs:126).
• Items 4, 5, and 7 (empty foreach, non-object passthrough, Jinja filter in name): outstanding — no such tests exist under src/manifest/. PR Extend expand_foreach test coverage (#296) #358 is in flight to cover these.

[leynos]

@coderabbitai looking at the codebase, is this issue still relevant? If the issue has been fixed in the code, does the documentation reflect this?

If it is still relevant, please provide an assessment of the issue criticality (critical, high, medium or low), based on risk of the issue impacting the user and the anticipated blast radius.\n\nOnce a decision has been made, apply the appropriate label.

If the issue is documentation ONLY (i.e., no code changes needed), apply the documentation label in addition to the appropriate criticality label.

Please also provide an impact of the required changes.

[coderabbitai]

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Fetch issue `#296` details
gh issue view 296 --repo leynos/netsuke --json title,body,labels,state,comments

# List available labels
gh label list --repo leynos/netsuke

Length of output: 5725

---

🏁 Script executed:

#!/bin/bash
# Read the expand_tests.rs file
fd expand_tests.rs --exec cat {}

# Also check if there are any recent commits touching this file
git log --oneline -10 -- src/manifest/expand_tests.rs

Length of output: 3072

---

🏁 Script executed:

#!/bin/bash
# Check for each of the 7 required items in the issue

echo "=== Test 1: expand_foreach_expands_sequence_values ==="
rg -n "expand_foreach_expands_sequence_values" 

echo "=== Test 2: expand_foreach_applies_when_expression ==="
rg -n "expand_foreach_applies_when_expression"

echo "=== Test 3/4: expand_foreach_applies_action_when_expression ==="
rg -n "expand_foreach_applies_action_when_expression"

echo "=== New test 4: expand_foreach_empty_foreach_produces_no_entries ==="
rg -n "expand_foreach_empty_foreach_produces_no_entries"

echo "=== New test 5: expand_foreach_non_object_entry_is_passed_through ==="
rg -n "expand_foreach_non_object_entry_is_passed_through"

echo "=== New test 6: expand_foreach_iteration_vars_do_not_overwrite_globals ==="
rg -n "expand_foreach_iteration_vars_do_not_overwrite_globals"

echo "=== New test 7: expand_foreach_jinja_filter_in_name ==="
rg -n "expand_foreach_jinja_filter_in_name"

echo "=== foreach key removal assertion ==="
rg -n "foreach" src/manifest/expand_tests.rs

echo "=== Full file line count ==="
fd expand_tests.rs --exec wc -l {}

Length of output: 2218

---

🏁 Script executed:

#!/bin/bash
# Read the full condition_cases.rs file
cat src/manifest/expand_test_cases/condition_cases.rs

echo ""
echo "=== Check for foreach key removal assertions in condition_cases.rs ==="
rg -n "foreach" src/manifest/expand_test_cases/condition_cases.rs

Length of output: 15363

---

🏁 Script executed:

#!/bin/bash
# Apply the 'low' label to issue `#296`
gh issue edit 296 --repo leynos/netsuke --add-label "low"

Length of output: 197

---

---

@leynos — the issue is still relevant. All 7 items remain outstanding. Here is the full breakdown:

---

Current State

The tests live in src/manifest/expand_test_cases/condition_cases.rs (not expand_tests.rs as originally named in the issue — the file has been reorganised since #294). Checking each item:

#	Requirement	Status
1	Add !contains_key("foreach") assertion to expand_foreach_expands_sequence_values	❌ Missing
2	Add !contains_key("foreach") assertion to expand_foreach_applies_when_expression	❌ Missing
3	Add !contains_key("foreach") assertion to expand_foreach_applies_action_when_expression	❌ Missing
4	New test expand_foreach_empty_foreach_produces_no_entries	❌ Missing
5	New test expand_foreach_non_object_entry_is_passed_through	❌ Missing
6	New test expand_foreach_iteration_vars_do_not_overwrite_globals	❌ Missing (partial coverage exists via expand_foreach_when_item_overrides_entry_vars but that is a distinct test with different focus)
7	New test expand_foreach_jinja_filter_in_name	❌ Missing

Notable partial mitigations already in place:

• expand_foreach_when_injects_iteration_vars_only_for_kept_entries already asserts foreach key removal for kept entries in both sections.
• expand_foreach_expands_actions_sequence_values already asserts foreach removal for actions.

These do not satisfy the specific named tests or assertions required by the issue.

---

Criticality: low

This is purely a test-coverage gap. There are no production code defects and no user-facing functionality affected. The blast radius is limited to the test file. The low label has been applied.

---

Impact

Changes are confined to src/manifest/expand_test_cases/condition_cases.rs:

• Add three !map.contains_key("foreach") assertion blocks inside existing test loops.
• Add four new #[test] functions totalling approximately 80–120 lines.
• No production code changes required.
• No documentation changes required.

[leynos]

@CodeRabbit plan

[coderabbitai]

Coding Plan

Summary

• Modifications to existing tests (items 1–3) add foreach key removal assertions following the established anyhow::ensure! pattern
• New edge case tests (items 4–6) operate at the expand layer using serde_saphyr parsing and the expand_foreach function directly
• The Jinja filter test (item 7) uses the full manifest::from_str pipeline to verify end-to-end rendering behaviour, as expand_foreach alone does not render template expressions

Design Choices

Design Choice 1: Test 7 requires rendering to verify uppercased names

Options Considered:

1. Call manifest::from_str to run the full pipeline, then inspect typed manifest.targets[n].name
2. Call expand_foreach followed by manual deserialisation and render_manifest with a configured Environment
3. Only verify that vars.item contains the expected value post-expand, deferring filter verification to integration tests

Chosen Option: 1

Rationale: The full pipeline via manifest::from_str matches the existing integration test pattern in tests/manifest_jinja_tests.rs and avoids duplicating Environment setup logic.

💡 User Tips

Regenerate the plan with different choices with @coderabbitai <feedback>.

Implementation Steps

Phase 1: Strengthen Existing Tests with foreach Key Removal Assertions

Extend three existing tests in the condition-cases submodule to assert that the foreach key is removed from every expanded entry, complementing the existing when key removal checks.

Task 1: Add foreach Removal Assertion to expand_foreach_expands_sequence_values

Add an assertion verifying foreach key removal after confirming correct entry count.

• Locate expand_foreach_expands_sequence_values in src/manifest/expand_test_cases/condition_cases.rs
• After the existing targets.len() == 2 check, iterate over each target entry
• For each entry, extract the object map and call ensure!(!map.contains_key("foreach"), "foreach should be removed after expansion")
• Follow the established assertion style using anyhow::ensure! with a descriptive message

Task 2: Add foreach Removal Assertion to expand_foreach_applies_when_expression

Add an assertion verifying foreach key removal from entries surviving the when filter.

• Locate expand_foreach_applies_when_expression in src/manifest/expand_test_cases/condition_cases.rs
• After the existing filtered-count assertions, iterate over each remaining target
• Assert !map.contains_key("foreach") for every surviving entry
• Use the same assertion pattern as Task 1.1

Task 3: Add foreach Removal Assertion to expand_foreach_applies_action_when_expression

Add an assertion verifying foreach key removal from actions surviving the when filter.

• Locate expand_foreach_applies_action_when_expression in src/manifest/expand_test_cases/condition_cases.rs
• After the existing assertions, iterate over each remaining action entry
• Assert !map.contains_key("foreach") for every surviving action
• Use the same assertion pattern as previous tasks

🤖 Prompt for AI agents

Extend three existing test functions in
`src/manifest/expand_test_cases/condition_cases.rs` to assert that the `foreach`
key is removed from every expanded entry after `expand_foreach` runs.

For each of the three tests listed below, add the `foreach` key removal
assertion after the existing entry-count/filter assertions:

- **`expand_foreach_expands_sequence_values`**
  - After the `targets.len() == 2` check, iterate over each target entry
- Extract the object map from each entry and call
`ensure!(!map.contains_key("foreach"), "foreach should be removed after
expansion")`

- **`expand_foreach_applies_when_expression`**
- After the existing filtered-count assertions, iterate over each remaining
target
  - Assert `!map.contains_key("foreach")` for every surviving entry

- **`expand_foreach_applies_action_when_expression`**
  - After the existing assertions, iterate over each remaining action entry
  - Assert `!map.contains_key("foreach")` for every surviving action

Use `anyhow::ensure!` with descriptive error messages throughout, matching the
established assertion style in the file.

Phase 2: Add New Edge Case and Behaviour Tests

Add four new tests to src/manifest/expand_tests.rs covering empty foreach handling, non-object passthrough, iteration variable precedence, and Jinja filter application.

Task 1: Add Test for Empty foreach Producing No Entries

Verify that foreach: [] results in the entry being dropped entirely.

• Create test function expand_foreach_empty_foreach_produces_no_entries in src/manifest/expand_tests.rs
• Construct YAML with a single target containing foreach: []
• Parse via serde_saphyr::from_str, call expand_foreach, then use targets() helper
• Assert targets.is_empty() — the entry should vanish, not produce zero-length expansion

Task 2: Add Test for Non-Object Entry Passthrough

Verify that bare string entries in a section array pass through unchanged without expansion processing.

• Create test function expand_foreach_non_object_entry_is_passed_through in src/manifest/expand_tests.rs
• Construct YAML where targets contains a bare string value (e.g., "bare_string_target") alongside or instead of object entries
• After expand_foreach, extract the targets array and confirm the bare string survives as a ManifestValue::String
• Assert the value equals the original string — no transformation applied

Task 3: Add Test for Iteration Variable Precedence

Verify that foreach-injected item overrides any pre-existing vars.item in the expanded entry.

• Create test function expand_foreach_iteration_vars_override_entry_vars in src/manifest/expand_tests.rs
• Construct YAML with a target containing vars: { item: "original" } and foreach: '["injected"]'
• After expand_foreach, extract the single expanded target's vars.item
• Assert vars.item == "injected" — the iteration variable takes precedence over the declared var
• This documents the actual observed behaviour per the exploration findings

Task 4: Add Test for Jinja Filter in Name Field

Verify that Jinja filters in name fields render correctly when combined with foreach iteration variables.

• Create test function expand_foreach_jinja_filter_in_name in src/manifest/expand_tests.rs
• Construct a complete manifest YAML string (including netsuke_version header) with a target using name: "{{ item | upper }}" and foreach: '["alpha", "beta"]'
• Call manifest::from_str to run the full parsing-expansion-rendering pipeline
• Extract the rendered target names from the typed NetsukeManifest
• Assert the names are ["ALPHA", "BETA"] — confirming filter application post-expansion

🤖 Prompt for AI agents

Add four new test functions to `src/manifest/expand_tests.rs`, each covering a
distinct edge case or behaviour of `expand_foreach`.

**`expand_foreach_empty_foreach_produces_no_entries`**
- Construct YAML with a single target containing `foreach: []`
- Parse via `serde_saphyr::from_str` and call `expand_foreach`
- Use the `targets()` helper and assert `targets.is_empty()`

**`expand_foreach_non_object_entry_is_passed_through`**
- Construct YAML where `targets` contains a bare string value (e.g.,
`"bare_string_target"`)
- After `expand_foreach`, extract the targets array
- Confirm the bare string survives as a `ManifestValue::String` and assert the
value equals the original string

**`expand_foreach_iteration_vars_override_entry_vars`**
- Construct YAML with a target containing `vars: { item: "original" }` and
`foreach: '["injected"]'`
- After `expand_foreach`, extract the single expanded target's `vars.item`
- Assert `vars.item == "injected"` — the iteration variable takes precedence
over the declared var

**`expand_foreach_jinja_filter_in_name`**
- Construct a complete manifest YAML string (including `netsuke_version` header)
with a target using `name: "{{ item | upper }}"` and `foreach: '["alpha",
"beta"]'`
- Call `manifest::from_str` to run the full parsing-expansion-rendering pipeline
- Extract the rendered target names from the typed `NetsukeManifest`
- Assert the names are `["ALPHA", "BETA"]`

Use `anyhow::ensure!` with descriptive messages throughout. Follow the existing
test structure and helper usage patterns in `expand_tests.rs`.

Research

The manifest expansion pipeline lives in src/manifest/expand.rs, with unit tests split across src/manifest/expand_tests.rs and its submodule src/manifest/expand_test_cases/condition_cases.rs (included via #[path]). Tests use raw YAML strings parsed via serde_saphyr::from_str, a basic minijinja::Environment, and helper functions (targets(), actions(), indexes()) to extract and inspect expanded entries. Assertions use anyhow::ensure! with descriptive error messages. The expand_foreach function mutates a ManifestValue in place and returns FilteringStats; it removes foreach and when keys during expansion but does not render Jinja templates in string fields—that occurs later in render_manifest.

---

🚀 Next Steps

🤖 All AI agent prompts combined

Task: 1

Extend three existing test functions in
`src/manifest/expand_test_cases/condition_cases.rs` to assert that the `foreach`
key is removed from every expanded entry after `expand_foreach` runs.

For each of the three tests listed below, add the `foreach` key removal
assertion after the existing entry-count/filter assertions:

- **`expand_foreach_expands_sequence_values`**
  - After the `targets.len() == 2` check, iterate over each target entry
- Extract the object map from each entry and call
`ensure!(!map.contains_key("foreach"), "foreach should be removed after
expansion")`

- **`expand_foreach_applies_when_expression`**
- After the existing filtered-count assertions, iterate over each remaining
target
  - Assert `!map.contains_key("foreach")` for every surviving entry

- **`expand_foreach_applies_action_when_expression`**
  - After the existing assertions, iterate over each remaining action entry
  - Assert `!map.contains_key("foreach")` for every surviving action

Use `anyhow::ensure!` with descriptive error messages throughout, matching the
established assertion style in the file.
===============================================================================

Task: 2

Add four new test functions to `src/manifest/expand_tests.rs`, each covering a
distinct edge case or behaviour of `expand_foreach`.

**`expand_foreach_empty_foreach_produces_no_entries`**
- Construct YAML with a single target containing `foreach: []`
- Parse via `serde_saphyr::from_str` and call `expand_foreach`
- Use the `targets()` helper and assert `targets.is_empty()`

**`expand_foreach_non_object_entry_is_passed_through`**
- Construct YAML where `targets` contains a bare string value (e.g.,
`"bare_string_target"`)
- After `expand_foreach`, extract the targets array
- Confirm the bare string survives as a `ManifestValue::String` and assert the
value equals the original string

**`expand_foreach_iteration_vars_override_entry_vars`**
- Construct YAML with a target containing `vars: { item: "original" }` and
`foreach: '["injected"]'`
- After `expand_foreach`, extract the single expanded target's `vars.item`
- Assert `vars.item == "injected"` — the iteration variable takes precedence
over the declared var

**`expand_foreach_jinja_filter_in_name`**
- Construct a complete manifest YAML string (including `netsuke_version` header)
with a target using `name: "{{ item | upper }}"` and `foreach: '["alpha",
"beta"]'`
- Call `manifest::from_str` to run the full parsing-expansion-rendering pipeline
- Extract the rendered target names from the typed `NetsukeManifest`
- Assert the names are `["ALPHA", "BETA"]`

Use `anyhow::ensure!` with descriptive messages throughout. Follow the existing
test structure and helper usage patterns in `expand_tests.rs`.

💡 Iterate on the plan with: `@coderabbitai` <feedback>

Example Feedback
- `@coderabbitai` You can skip phase 3. Add a simple unit test case for phase 2.
- `@coderabbitai` For design choice 1 go ahead with option 3 and replan.

---

💬 Have feedback or questions? Drop into our discord!