Add foreach target expansion and value-level Jinja rendering#123
Add foreach target expansion and value-level Jinja rendering#123
Conversation
Reviewer's GuideThis PR overhauls manifest ingestion into a YAML-first pipeline with value-level Jinja rendering, introduces a dedicated foreach/when expansion pass, and aligns documentation, tests, and examples with the new approach. Flow diagram for the new YAML-first manifest ingestion pipelineflowchart TD
A[Parse manifest as YAML] --> B[Expand foreach/when keys]
B --> C[Deserialize to NetsukeManifest AST]
C --> D[Render Jinja expressions in value fields]
D --> E[Ready for execution]
Flow diagram for foreach/when target expansionflowchart TD
A[Iterate over targets in YAML] --> B{Has foreach?}
B -- Yes --> C[Evaluate foreach expression]
C --> D[For each item:]
D --> E{Has when?}
E -- Yes --> F[Evaluate when expression]
F -- True --> G[Expand target with item/index]
F -- False --> H[Skip target]
E -- No --> G
B -- No --> I[Keep target as is]
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. Warning Rate limit exceeded@leynos has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 1 minutes and 5 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (6)
Summary by CodeRabbit
WalkthroughSummarise the YAML-first, multi-stage ingestion: parse YAML → expand foreach/when into per-item targets (inject item and index) → deserialize into the public NetsukeManifest AST → render remaining templated string fields per-field. Update docs, examples and tests; keep public API signatures unchanged. Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant Manifest as manifest.rs
participant YAML as YAML Parser
participant Exp as Foreach/When Expander
participant Serde as Deserialiser
participant Render as Renderer
Caller->>Manifest: from_path / from_str(yaml)
Manifest->>YAML: Parse YAML to YamlValue
YAML-->>Manifest: YamlValue
Manifest->>Exp: Expand foreach + when → per-item target entries (inject item/index)
Exp-->>Manifest: Expanded YamlValue
Manifest->>Serde: Deserialize to NetsukeManifest (AST)
Serde-->>Manifest: NetsukeManifest
Manifest->>Render: Render string fields per-target (vars, targets, rules)
Render-->>Manifest: Rendered NetsukeManifest
Manifest-->>Caller: NetsukeManifest
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
✨ Finishing Touches
🧪 Generate unit tests
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![codescene-delta-analysis[bot]](https://avatars.githubusercontent.com/in/53921?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
🔭 Outside diff range comments (5)
examples/writing.yml (1)
23-23: Declare deps as a list to avoid type ambiguity.If deps accepts multiple entries, keep the type consistent by always using a list, even for singletons. This reduces downstream branching and future-proofs the schema.
- deps: "{{ build_dir }}" + deps: + - "{{ build_dir }}"docs/netsuke-design.md (2)
1074-1104: Fix broken fenced code blocks for Ninja snippetsThe Ninja examples have mis-nested/duplicated fences, which breaks Markdown rendering. Close each code block once, and avoid stray “```ninja” lines.
Proposed corrected snippet structure:
```ninja # Generated from an ir::Action rule cc command = gcc -c -o $out $in description = CC $out depfile = $out.d deps = gcc# Generated from an ir::BuildEdge build foo.o: cc foo.c build bar.o: cc bar.c build my_app: link foo.o bar.o | lib_dependency.aRun “make markdownlint” and “make fmt” to normalise fences and wrapping. I can sweep the doc for fence issues and fix them in one pass. --- `1610-1699`: **Repair broken footnotes/links and normalise citation syntax** Several citations display malformed URLs (e.g., stray “<<<”, broken anchors). This will fail markdownlint and is confusing to readers. - Replace malformed links with valid Markdown links. - Keep footnotes as GitHub-flavoured Markdown footnotes, ordered by first appearance. - Wrap paragraphs at 80 columns. Offer: I can produce a cleaned “Works cited” section adhering to the house style. </blockquote></details> <details> <summary>tests/features/manifest.feature (1)</summary><blockquote> `85-95`: **Broaden assertions to cover phony/always flags and rule exclusivity** The “Rendering all target fields” scenario is strong. Add checks for: - phony/always defaults on targets 1–3 - exclusivity enforcement across rule/command/script (e.g., a dedicated failure scenario) I can extend the feature and add a small invalid manifest fixture to test mutual exclusivity errors. </blockquote></details> <details> <summary>tests/manifest_jinja_tests.rs (1)</summary><blockquote> `66-92`: **Use `rstest` parameterisation to reduce duplication in foreach tests.** The `expands_foreach_targets` and `foreach_when_filters_items` tests share similar logic. Use `rstest` parameterisation to combine them. ```diff -#[rstest] -fn expands_foreach_targets() { - let yaml = manifest_yaml( - "targets:\n - foreach: \"['a', 'b']\"\n name: '{{ item }}'\n command: 'echo {{ item }}'\n", - ); - - let manifest = manifest::from_str(&yaml).expect("parse"); - assert_eq!(manifest.targets.len(), 2); - let names: Vec<_> = manifest - .targets - .iter() - .map(|t| match &t.name { - netsuke::ast::StringOrList::String(s) => s.clone(), - other => panic!("Expected String, got: {other:?}"), - }) - .collect(); - assert_eq!(names, vec!["a", "b"]); - - let commands: Vec<_> = manifest - .targets - .iter() - .map(|t| match &t.recipe { - Recipe::Command { command } => command.clone(), - other => panic!("Expected command recipe, got: {other:?}"), - }) - .collect(); - assert_eq!(commands, vec!["echo a", "echo b"]); -} +#[rstest] +#[case( + "targets:\n - foreach: \"['a', 'b']\"\n name: '{{ item }}'\n command: 'echo {{ item }}'\n", + vec!["a", "b"], + vec!["echo a", "echo b"] +)] +#[case( + "targets:\n - foreach: \"['a', 'skip', 'b']\"\n when: item != 'skip'\n name: '{{ item }}'\n command: 'echo {{ item }}'\n", + vec!["a", "b"], + vec!["echo a", "echo b"] +)] +fn expands_foreach_targets( + #[case] yaml_body: &str, + #[case] expected_names: Vec<&str>, + #[case] expected_commands: Vec<&str>, +) { + let yaml = manifest_yaml(yaml_body); + + let manifest = manifest::from_str(&yaml).expect("parse"); + assert_eq!(manifest.targets.len(), expected_names.len()); + + let names: Vec<_> = manifest + .targets + .iter() + .map(|t| match &t.name { + netsuke::ast::StringOrList::String(s) => s.clone(), + other => panic!("Expected String, got: {other:?}"), + }) + .collect(); + assert_eq!(names, expected_names); + + let commands: Vec<_> = manifest + .targets + .iter() + .map(|t| match &t.recipe { + Recipe::Command { command } => command.clone(), + other => panic!("Expected command recipe, got: {other:?}"), + }) + .collect(); + assert_eq!(commands, expected_commands); +}Also applies to: 140-156
♻️ Duplicate comments (4)
src/manifest.rs (4)
48-63: Splitexpand_foreachinto smaller functions to reduce complexity.The
expand_foreachfunction's cyclomatic complexity can be reduced by extracting the target expansion logic into a separate helper function.fn expand_foreach(doc: &mut YamlValue, env: &Environment) -> Result<()> { + expand_foreach_targets(doc, env) +} + +fn expand_foreach_targets(doc: &mut YamlValue, env: &Environment) -> Result<()> { let Some(targets) = doc.get_mut("targets").and_then(|v| v.as_sequence_mut()) else { return Ok(()); }; let mut expanded = Vec::new(); for target in std::mem::take(targets) { match target { YamlValue::Mapping(map) => expanded.extend(expand_target(map, env)?), other => expanded.push(other), } } *targets = expanded; Ok(()) }
65-83: Extract conditional logic inexpand_targetto improve readability.The
expand_targetfunction contains nested conditional logic that could be simplified by extracting helper predicates.+fn has_foreach_key(map: &YamlMapping) -> bool { + map.contains_key(&YamlValue::String("foreach".into())) +} + +fn process_foreach_item( + map: &YamlMapping, + env: &Environment, + item: &Value, + index: usize, +) -> Result<Option<YamlValue>> { + let mut clone = map.clone(); + clone.remove(&YamlValue::String("foreach".into())); + if !when_allows(&mut clone, env, item, index)? { + return Ok(None); + } + inject_iteration_vars(&mut clone, item, index)?; + Ok(Some(YamlValue::Mapping(clone))) +} + fn expand_target(map: YamlMapping, env: &Environment) -> Result<Vec<YamlValue>> { - let foreach_key = YamlValue::String("foreach".into()); - if let Some(expr_val) = map.get(&foreach_key) { + if has_foreach_key(&map) { + let expr_val = map.get(&YamlValue::String("foreach".into())).expect("foreach exists"); let values = parse_foreach_values(expr_val, env)?; let mut items = Vec::new(); for (index, item) in values.into_iter().enumerate() { - let mut clone = map.clone(); - clone.remove(&foreach_key); - if !when_allows(&mut clone, env, &item, index)? { - continue; + if let Some(target) = process_foreach_item(&map, env, &item, index)? { + items.push(target); } - inject_iteration_vars(&mut clone, &item, index)?; - items.push(YamlValue::Mapping(clone)); } Ok(items) } else { Ok(vec![YamlValue::Mapping(map)]) } }
181-201: Add unit tests forrender_targetto ensure all fields are rendered correctly.The
render_targetfunction handles rendering of multiple fields and recipe variants but lacks dedicated unit tests. Add comprehensive tests to verify correct rendering and error handling.Would you like me to generate comprehensive unit tests for the
render_targetfunction that cover all fields and recipe variants?
13-14: Extract repeated error context strings into constants.Multiple error context strings are repeated throughout the code. Extract these into named constants to improve maintainability.
const ERR_INITIAL_YAML_PARSE: &str = "initial YAML parse error"; const ERR_MANIFEST_PARSE: &str = "manifest parse error"; +const ERR_RENDER_RULE_DESC: &str = "render rule description"; +const ERR_RENDER_RULE_CMD: &str = "render rule command"; +const ERR_RENDER_RULE_SCRIPT: &str = "render rule script"; +const ERR_RENDER_TARGET_CMD: &str = "render target command"; +const ERR_RENDER_TARGET_SCRIPT: &str = "render target script"; +const ERR_RENDER_STRING: &str = "render string value"; +const ERR_RENDER_LIST: &str = "render list value"; +const ERR_FOREACH_PARSE: &str = "foreach expression parse error"; +const ERR_FOREACH_EVAL: &str = "foreach evaluation error"; +const ERR_WHEN_PARSE: &str = "when expression parse error"; +const ERR_WHEN_EVAL: &str = "when evaluation error"; fn as_str<'a>(value: &'a YamlValue, field: &str) -> Result<&'a str> { value .as_str() .with_context(|| format!("{field} must be a string expression")) } fn eval_expression(env: &Environment, name: &str, expr: &str, ctx: Value) -> Result<Value> { + let parse_err = match name { + "foreach" => ERR_FOREACH_PARSE, + "when" => ERR_WHEN_PARSE, + _ => "expression parse error", + }; + let eval_err = match name { + "foreach" => ERR_FOREACH_EVAL, + "when" => ERR_WHEN_EVAL, + _ => "evaluation error", + }; env.compile_expression(expr) - .with_context(|| format!("{name} expression parse error"))? + .context(parse_err)? .eval(ctx) - .with_context(|| format!("{name} evaluation error")) + .context(eval_err) }Then update the render functions:
fn render_rule(rule: &mut crate::ast::Rule, env: &Environment) -> Result<()> { if let Some(desc) = &mut rule.description { *desc = env .render_str(desc, context! {}) - .context("render rule description")?; + .context(ERR_RENDER_RULE_DESC)?; } render_string_or_list(&mut rule.deps, env, &Vars::new())?; match &mut rule.recipe { Recipe::Command { command } => { *command = env .render_str(command, context! {}) - .context("render rule command")?; + .context(ERR_RENDER_RULE_CMD)?; } Recipe::Script { script } => { *script = env .render_str(script, context! {}) - .context("render rule script")?; + .context(ERR_RENDER_RULE_SCRIPT)?; } Recipe::Rule { rule: r } => render_string_or_list(r, env, &Vars::new())?, } Ok(()) }Also applies to: 133-134, 138-141, 161-162, 168-169, 173-174, 189-190, 195-196, 208-209, 218-218, 222-222
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
💡 Knowledge Base configuration:
- Jira integration is disabled
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (15)
docs/netsuke-design.md(2 hunks)docs/roadmap.md(1 hunks)examples/photo_edit.yml(1 hunks)examples/visual_design.yml(1 hunks)examples/website.yml(1 hunks)examples/writing.yml(1 hunks)src/manifest.rs(1 hunks)tests/data/foreach.yml(1 hunks)tests/data/foreach_invalid.yml(1 hunks)tests/data/jinja_for.yml(0 hunks)tests/data/jinja_for_invalid.yml(0 hunks)tests/data/render_target.yml(1 hunks)tests/features/manifest.feature(1 hunks)tests/manifest_jinja_tests.rs(3 hunks)tests/steps/manifest_steps.rs(2 hunks)
💤 Files with no reviewable changes (2)
- tests/data/jinja_for_invalid.yml
- tests/data/jinja_for.yml
🧰 Additional context used
📓 Path-based instructions (3)
**/*.md
📄 CodeRabbit Inference Engine (AGENTS.md)
**/*.md: Markdown files must use en-GB-oxendict ("-ize" / "-yse" / "-our") spelling and grammar. (EXCEPTION: the naming of the "LICENSE" file, which is to be left unchanged for community consistency.)
Validate Markdown files using make markdownlint.
Run make fmt after any documentation changes to format all Markdown files and fix table markup.
Validate Mermaid diagrams in Markdown files by running make nixie.
Markdown paragraphs and bullet points must be wrapped at 80 columns.
Code blocks must be wrapped at 120 columns.
Tables and headings must not be wrapped.
Use dashes (-) for list bullets.
Use GitHub-flavoured Markdown footnotes ([^1]) for references and footnotes.
Files:
docs/roadmap.mddocs/netsuke-design.md
⚙️ CodeRabbit Configuration File
**/*.md: * Avoid 2nd person or 1st person pronouns ("I", "you", "we")
- Use en-GB-oxendict (-ize / -our) spelling and grammar
- Headings must not be wrapped.
- Documents must start with a level 1 heading
- Headings must correctly increase or decrease by no more than one level at a time
- Use GitHub-flavoured Markdown style for footnotes and endnotes.
- Numbered footnotes must be numbered by order of appearance in the document.
Files:
docs/roadmap.mddocs/netsuke-design.md
docs/**/*.md
📄 CodeRabbit Inference Engine (AGENTS.md)
docs/**/*.md: Reference: Use the markdown files within the docs/ directory as a knowledge base and source of truth for project requirements, dependency choices, and architectural decisions.
Update: When new decisions are made, requirements change, libraries are added/removed, or architectural patterns evolve, proactively update the relevant file(s) in the docs/ directory to reflect the latest state. Ensure the documentation remains accurate and current.
Files:
docs/roadmap.mddocs/netsuke-design.md
**/*.rs
📄 CodeRabbit Inference Engine (AGENTS.md)
**/*.rs: Comment why, not what. Explain assumptions, edge cases, trade-offs, or complexity. Don't echo the obvious.
Use functions and composition. Avoid repetition by extracting reusable logic. Prefer generators or comprehensions, and declarative code to imperative repetition when readable.
Small, meaningful functions. Functions must be small, clear in purpose, single responsibility, and obey command/query segregation.
Name things precisely. Use clear, descriptive variable and function names. For booleans, prefer names with is, has, or should.
Every module must begin with a module level (//! ) comment explaining the module's purpose and utility.
Document public APIs using Rustdoc comments (///) so documentation can be generated with cargo doc.
Place function attributes after doc comments.
Do not use return in single-line functions.
Prefer immutable data and avoid unnecessary mut bindings.
Handle errors with the Result type instead of panicking where feasible.
Prefer .expect() over .unwrap().
Use concat!() to combine long string literals rather than escaping newlines with a backslash.
Prefer single line versions of functions where appropriate.
Clippy warnings MUST be disallowed.
Lints must not be silenced except as a last resort.
Lint rule suppressions must be tightly scoped and include a clear reason.
Prefer expect over allow.
Keep file size manageable. No single code file may be longer than 400 lines. Long switch statements or dispatch tables should be broken up by feature and constituents colocated with targets. Large blocks of test data should be moved to external data files.
Illustrate with clear examples. Function documentation must include clear examples demonstrating the usage and outcome of the function. Test documentation should omit examples where the example serves only to reiterate the test logic.
Prefer semantic error enums. Derive std::error::Error (via the thiserror crate) for any condition the caller might inspect, retry, or map to an HTTP status.
...
Files:
tests/steps/manifest_steps.rssrc/manifest.rstests/manifest_jinja_tests.rs
⚙️ CodeRabbit Configuration File
**/*.rs: * Seek to keep the cyclomatic complexity of functions no more than 12.
Adhere to single responsibility and CQRS
Place function attributes after doc comments.
Do not use
returnin single-line functions.Move conditionals with >2 branches into a predicate function.
Avoid
unsafeunless absolutely necessary.Every module must begin with a
//!doc comment that explains the module's purpose and utility.Comments and docs must follow en-GB-oxendict (-ize / -our) spelling and grammar
Lints must not be silenced except as a last resort.
#[allow]is forbidden.- Only narrowly scoped
#[expect(lint, reason = "...")]is allowed.- No lint groups, no blanket or file-wide suppression.
- Include
FIXME:with link if a fix is expected.Where code is only used by specific features, it must be conditionally compiled or a conditional expectation for unused_code applied.
Use
rstestfixtures for shared setup and to avoid repetition between tests.Replace duplicated tests with
#[rstest(...)]parameterised cases.Prefer
mockallfor mocks/stubs.Prefer
.expect()over.unwrap()Ensure that any API or behavioural changes are reflected in the documentation in
docs/Ensure that any completed roadmap steps are recorded in the appropriate roadmap in
docs/Files must not exceed 400 lines in length
- Large modules must be decomposed
- Long match statements or dispatch tables should be decomposed by domain and collocated with targets
- Large blocks of inline data (e.g., test fixtures, constants or templates) must be moved to external files and inlined at compile-time or loaded at run-time.
Files:
tests/steps/manifest_steps.rssrc/manifest.rstests/manifest_jinja_tests.rs
🧬 Code Graph Analysis (1)
tests/manifest_jinja_tests.rs (2)
src/manifest.rs (1)
from_str(24-45)src/runner.rs (1)
is_empty(96-98)
🪛 LanguageTool
docs/roadmap.md
[uncategorized] ~93-~93: Use a comma before ‘so’ if it connects two independent clauses (unless they are closely connected and short).
Context: ...move the global first-pass Jinja parsing so manifests are valid YAML before...
(COMMA_COMPOUND_SENTENCE_2)
[uncategorized] ~93-~93: Possible missing preposition found.
Context: ...ss Jinja parsing so manifests are valid YAML before any templating occurs. ...
(AI_HYDRA_LEO_MISSING_TO)
🔍 Remote MCP
Summary of Relevant Context for PR #123 Review:
-
Design Documentation Changes
- docs/netsuke-design.md updated to describe a data-first pipeline:
- YAML parsed before any templating.
- Three stages: template expansion (foreach/when), deserialization, final Jinja rendering on string fields. [ai_summary]
- docs/roadmap.md marks removal of global first-pass Jinja parsing done, and implementation of foreach/when. [ai_summary]
- docs/netsuke-design.md updated to describe a data-first pipeline:
-
Implementation Changes
- src/manifest.rs now implements single-pass YAML-first flow:
- parse YAML via
from_str, expandforeachand evaluatewhen, then render remaining Jinja in strings. [ai_summary]
- parse YAML via
- No new public API signatures; existing
from_strandfrom_pathremain. [ask_question::list_public_api_functions] - No ERR_* constants; error handling uses
anyhow::Context. [ask_question::error_constants] - No new functions in src/manifest.rs related to foreach; logic moved to Jinja pass outside this file. [ask_question::new_functions]
- src/manifest.rs now implements single-pass YAML-first flow:
-
Examples and Tests
- Updated examples use bare expressions for foreach instead of templated strings. [ai_summary]
- Tests in tests/data/ and tests/features/ updated: removed Jinja-based for tests, added foreach-based scenarios. [ai_summary]
- New Cucumber steps assert target sources, deps, order-only_deps, scripts, rules. [ai_summary]
- IrGenError variants remain unchanged: RuleNotFound, MultipleRules, DuplicateOutput in src/ir.rs. [deepwiki]
No further issues found within iteration limits.
⏰ Context from checks skipped due to timeout of 120000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build-test
🔇 Additional comments (8)
examples/website.yml (1)
17-17: LGTM: adopt YAML-first foreach expression.This aligns with the new pipeline: expand foreach on data, then render strings. Keep it.
examples/visual_design.yml (1)
16-16: LGTM: migrate foreach to a bare expression.This matches the documented YAML-first, per-field rendering pipeline.
examples/photo_edit.yml (1)
17-21: Adopt bare Jinja expression for foreach; verify glob() availabilityEnsure the Jinja
glob()function is exposed in your pipeline before using it in examples/photo_edit.yml (lines 17–21); otherwise expansion will fail at example-run time. Add a guard in your CI to skip this example untilglob()support lands, or rewrite the example to use a static file list for CI portability.
- File: examples/photo_edit.yml
- Lines: 17–21
docs/roadmap.md (1)
99-103: LGTM: Foreach/when status and semantics updatedMarking foreach/when as implemented with item/index and precedence layering matches the implementation and tests.
tests/data/render_target.yml (2)
10-18: Nice coverage of per-target renderingExercising name, sources, deps, order_only_deps, and command in one target is a good end-to-end check for the final rendering pass.
8-11: Ignore unquoting suggestionThe
foreachhandler callsparse_foreach_values, which starts withas_str(expr_val, "foreach")and only accepts a string. Quoting"[1]"is required so thateval_expressioncan parse it as a list literal. A bare YAML sequence (foreach: [1]) would fail atas_strand break iteration.No changes needed in tests/data/render_target.yml.
Likely an incorrect or invalid review comment.
docs/netsuke-design.md (1)
596-600: Ignore stage numbering update
Stage numbering is consistent: Stage 5 is “IR Generation & Validation” in both the diagram and Section 1.2. No changes required.Likely an incorrect or invalid review comment.
tests/features/manifest.feature (1)
80-84: Negative test for non-iterable foreach is appropriateKeep this to guard error messaging and type-checking in foreach evaluation.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 4
🔭 Outside diff range comments (2)
tests/manifest_jinja_tests.rs (2)
1-1: Update module docs to reflect YAML-first, value-level rendering.Align the module header with the new three-stage pipeline.
-//! Tests for Jinja-templated manifest parsing. +//! Tests for the YAML-first manifest pipeline: parse YAML, expand foreach/when, then render Jinja only in string values.
66-92: Add coverage for theindexvariable exposed by foreach.Exercise
indexto lock down its availability and ordering semantics.Example new test:
#[rstest] fn foreach_exposes_index() { let yaml = manifest_yaml( "targets:\n - foreach: \"['x', 'y']\"\n name: '{{ index }}:{{ item }}'\n command: 'echo {{ index }} {{ item }}'\n", ); let manifest = manifest::from_str(&yaml).expect("parse"); let names: Vec<_> = manifest .targets .iter() .map(|t| match &t.name { netsuke::ast::StringOrList::String(s) => s.clone(), other => panic!("Expected String, got: {other:?}"), }) .collect(); assert_eq!(names, vec!["0:x", "1:y"]); let commands: Vec<_> = manifest .targets .iter() .map(|t| match &t.recipe { Recipe::Command { command } => command.clone(), other => panic!("Expected command recipe, got: {other:?}"), }) .collect(); assert_eq!(commands, vec!["echo 0 x", "echo 1 y"]); }
♻️ Duplicate comments (1)
tests/manifest_jinja_tests.rs (1)
172-207: Deduplicate StringOrList assertions via helpers.Extract repeated StringOrList pattern matches into helpers and use them here.
Add these helpers near the top of the file:
fn assert_string_or_list_eq(actual: &netsuke::ast::StringOrList, expected: &str, field: &str) { match actual { netsuke::ast::StringOrList::String(s) => assert_eq!(s, expected), netsuke::ast::StringOrList::List(list) if list.len() == 1 => { assert_eq!(&list[0], expected); } other => panic!("Expected String for {field}, got: {other:?}"), } } fn assert_string_or_list_contains( actual: &netsuke::ast::StringOrList, expected: &[String], field: &str, ) { match actual { netsuke::ast::StringOrList::List(list) => assert_eq!(list, expected), other => panic!("Expected List for {field}, got: {other:?}"), } }Then apply this diff in this test:
- match &target.name { - netsuke::ast::StringOrList::String(n) => assert_eq!(n, "base1"), - other => panic!("Expected String, got: {other:?}"), - } + assert_string_or_list_eq(&target.name, "base1", "name"); - match &target.sources { - netsuke::ast::StringOrList::List(list) => assert_eq!(list, &["base1.src".to_string()]), - other => panic!("Expected List, got: {other:?}"), - } + assert_string_or_list_contains(&target.sources, &["base1.src".to_string()], "sources"); - match &target.deps { - netsuke::ast::StringOrList::List(list) => assert_eq!(list, &["base1.dep".to_string()]), - other => panic!("Expected List, got: {other:?}"), - } + assert_string_or_list_contains(&target.deps, &["base1.dep".to_string()], "deps"); - match &target.order_only_deps { - netsuke::ast::StringOrList::List(list) => { - assert_eq!(list, &["base1.ord".to_string()]); - } - other => panic!("Expected List, got: {other:?}"), - } + assert_string_or_list_contains( + &target.order_only_deps, + &["base1.ord".to_string()], + "order_only_deps", + );
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
💡 Knowledge Base configuration:
- Jira integration is disabled
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
tests/manifest_jinja_tests.rs(3 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.rs
📄 CodeRabbit Inference Engine (AGENTS.md)
**/*.rs: Comment why, not what. Explain assumptions, edge cases, trade-offs, or complexity. Don't echo the obvious.
Use functions and composition. Avoid repetition by extracting reusable logic. Prefer generators or comprehensions, and declarative code to imperative repetition when readable.
Small, meaningful functions. Functions must be small, clear in purpose, single responsibility, and obey command/query segregation.
Name things precisely. Use clear, descriptive variable and function names. For booleans, prefer names with is, has, or should.
Every module must begin with a module level (//! ) comment explaining the module's purpose and utility.
Document public APIs using Rustdoc comments (///) so documentation can be generated with cargo doc.
Place function attributes after doc comments.
Do not use return in single-line functions.
Prefer immutable data and avoid unnecessary mut bindings.
Handle errors with the Result type instead of panicking where feasible.
Prefer .expect() over .unwrap().
Use concat!() to combine long string literals rather than escaping newlines with a backslash.
Prefer single line versions of functions where appropriate.
Clippy warnings MUST be disallowed.
Lints must not be silenced except as a last resort.
Lint rule suppressions must be tightly scoped and include a clear reason.
Prefer expect over allow.
Keep file size manageable. No single code file may be longer than 400 lines. Long switch statements or dispatch tables should be broken up by feature and constituents colocated with targets. Large blocks of test data should be moved to external data files.
Illustrate with clear examples. Function documentation must include clear examples demonstrating the usage and outcome of the function. Test documentation should omit examples where the example serves only to reiterate the test logic.
Prefer semantic error enums. Derive std::error::Error (via the thiserror crate) for any condition the caller might inspect, retry, or map to an HTTP status.
...
Files:
tests/manifest_jinja_tests.rs
⚙️ CodeRabbit Configuration File
**/*.rs: * Seek to keep the cyclomatic complexity of functions no more than 12.
Adhere to single responsibility and CQRS
Place function attributes after doc comments.
Do not use
returnin single-line functions.Move conditionals with >2 branches into a predicate function.
Avoid
unsafeunless absolutely necessary.Every module must begin with a
//!doc comment that explains the module's purpose and utility.Comments and docs must follow en-GB-oxendict (-ize / -our) spelling and grammar
Lints must not be silenced except as a last resort.
#[allow]is forbidden.- Only narrowly scoped
#[expect(lint, reason = "...")]is allowed.- No lint groups, no blanket or file-wide suppression.
- Include
FIXME:with link if a fix is expected.Where code is only used by specific features, it must be conditionally compiled or a conditional expectation for unused_code applied.
Use
rstestfixtures for shared setup and to avoid repetition between tests.Replace duplicated tests with
#[rstest(...)]parameterised cases.Prefer
mockallfor mocks/stubs.Prefer
.expect()over.unwrap()Ensure that any API or behavioural changes are reflected in the documentation in
docs/Ensure that any completed roadmap steps are recorded in the appropriate roadmap in
docs/Files must not exceed 400 lines in length
- Large modules must be decomposed
- Long match statements or dispatch tables should be decomposed by domain and collocated with targets
- Large blocks of inline data (e.g., test fixtures, constants or templates) must be moved to external files and inlined at compile-time or loaded at run-time.
Files:
tests/manifest_jinja_tests.rs
🧬 Code Graph Analysis (1)
tests/manifest_jinja_tests.rs (1)
src/manifest.rs (1)
from_str(24-45)
🔍 Remote MCP Deepwiki
Relevant findings to aid review (concise, factual):
-
Design/docs: The repo contains a detailed design describing a five-stage pipeline and template system; docs/netsuke-design.md and docs/roadmap.md document the ingestion pipeline, template functions (env(), glob()), security (shell-quote), and the roadmap item to remove the global first-pass Jinja parse. This is the authoritative place the PR updates to a YAML-first, three-stage (expand_foreach → deserialise → render strings) narrative should align with.,
-
Template engine & functions: The project uses minijinja with registered functions/filters (env, glob, basename, shell_escape, etc.); docs describe intended behavior and security (auto shell-escaping, explicit raw opt-out). Relevant when verifying that moving evaluation into value-level rendering preserves security/semantics.
-
Manifest parsing & API surface: manifest parsing and public API entry points (from_str, from_path) live in src/manifest.rs; docs and module layout show manifest.rs is the intended place for template/parse changes and that public API signatures are expected to remain stable. Confirm the PR kept from_str/from_path signatures.
-
Tests & test infra: The repo uses rstest, cucumber-rs (BDD), and insta snapshots. Tests and feature files live under tests/, with step definitions expecting target/source/dep assertions. The PR added/updated many tests (foreach.yml, render_target.yml, updated features and steps) — ensure step changes match step definitions and BDD expectations.
-
Examples: Examples under examples/ use foreach expressions and rely on template functions (glob, path ops). The PR replaced templated-string foreach values with bare expressions (e.g., "{{ glob(...) }}" → glob(...)); verify examples still conform to documented template function semantics and that eval timing (pre/post-deserialise) matches docs.
-
IR & security expectations: IR generation (src/ir.rs) enforces validation (RuleNotFound, DuplicateOutput, etc.) and action deduplication via stable hashing; rendering changes must not break these invariants or introduce nondeterminism in hashes.
Tools used:
- read_wiki_structure (Deepwiki) — repository wiki pages listing and structure.
- read_wiki_contents (Deepwiki) — full docs/pages content referenced above.
⏰ Context from checks skipped due to timeout of 120000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build-test
🔇 Additional comments (6)
tests/manifest_jinja_tests.rs (6)
50-51: Quote the Jinja control block to preserve it as a YAML string.Quoting the command fixes the YAML parse and matches value-level rendering semantics.
120-142: LGTM: single-item foreach expansion is correct.
210-222: LGTM: script recipe rendering is correct.
225-240: LGTM: rule recipe resolves with value-level rendering as expected.
243-246: LGTM: missing variable in target fields yields an error as intended.
252-258: LGTM: undefined in Jinjaifproduces a parse/render error under Strict undefined.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 3
🔭 Outside diff range comments (3)
docs/netsuke-design.md (3)
548-550: Align wording with the new multi-stage pipelineReplace “two-pass” with “multi-stage” to match the YAML-first, three-stage ingestion described in this section.
- The integration of a templating engine like Jinja fundamentally shapes the - parsing pipeline, mandating a two-pass approach. It is impossible to parse the + The integration of a templating engine like Jinja fundamentally shapes the + parsing pipeline, mandating a multi-stage approach. It is not feasible to parse the - user's `Netsukefile` file with `serde_yml` in a single step. + user's `Netsukefile` file with `serde_yml` in a single step.
1076-1084: Repair broken code fences in the Ninja snippetThe fenced block is malformed with a duplicate opening fence; markdownlint will fail and rendering will break.
```ninja # Generated from an ir::Action rule cc command = gcc -c -o $out $in description = CC $out depfile = $out.d deps = gcc - -```ninja ---- `1094-1111`: **Fix malformed Ninja and default snippets; close/open fences correctly** The build edge example and the subsequent default statement have broken fences and stray text. Rewrite as two valid fenced blocks. ```diff - ```ninja - # Generated from an ir::BuildEdge - build foo.o: cc foo.c build bar.o: cc bar.c build my_app: link foo.o bar.o | - - ``` - -| lib_dependency.a | - -4\. **Write Defaults:** Finally, write the `default` statement, listing all -paths from `graph.default_targets`.ninja - -default my_app - -``` + ```ninja + # Generated from an ir::BuildEdge + build foo.o: cc foo.c + build bar.o: cc bar.c + build my_app: link foo.o bar.o | lib_dependency.a + ``` + +4. **Write Defaults:** Finally, write the `default` statement, listing all +paths from `graph.default_targets`. + +```ninja +default my_app +```
♻️ Duplicate comments (9)
docs/roadmap.md (1)
93-95: Grammar/style fix — LGTMThe completed item now conforms to the style guide and reads clearly.
tests/data/foreach.yml (1)
3-5: Use native YAML list for foreach data.Drive the YAML-first contract by supplying a typed YAML list instead of a stringified Python list. This also avoids an unnecessary Jinja evaluation hop.
- - foreach: "['foo', 'bar']" + - foreach: + - foo + - bartests/features/manifest.feature (1)
71-78: Assert index semantics explicitly (0-based) if exposed by the AST.Lock down the 0-based index guarantee with explicit assertions. If step defs expose
index, add steps like the following to prevent regressions.Scenario: Generating targets with foreach Given the manifest file "tests/data/foreach.yml" is parsed When the manifest is checked Then the manifest has 2 targets And the target 1 name is "0-foo" And the target 1 command is "echo 0 foo" + And the target 1 index is 0 And the target 2 name is "1-bar" And the target 2 command is "echo 1 bar" + And the target 2 index is 1If you want, I can add the step implementation to read
indexfrom the parsed target.tests/steps/manifest_steps.rs (2)
147-153: Reduce repetition: use a StringOrList-to-string helper.Avoid pattern-matching in every assertion that expects a single string. Extract a helper to validate and extract a single string for fields that are semantically singular.
fn assert_target_name(world: &CliWorld, index: usize, name: &str) { - let target = get_target(world, index); - match &target.name { - StringOrList::String(value) => assert_eq!(value, name), - other => panic!("Expected StringOrList::String, got: {other:?}"), - } + let target = get_target(world, index); + let actual = get_string_from_string_or_list(&target.name, "name"); + assert_eq!(&actual, name); }Add this helper near
assert_list_contains:fn get_string_from_string_or_list(value: &StringOrList, field_name: &str) -> String { match value { StringOrList::String(s) => s.clone(), StringOrList::List(list) if list.len() == 1 => list[0].clone(), other => panic!("Expected String for {field_name}, got: {other:?}"), } }
238-253: Unify rule-name extraction via the same helper.Apply the same helper to the rule-name assertion to keep the tests DRY and error messages consistent.
fn target_rule_is(world: &mut CliWorld, index: usize, rule_name: String) { let target = get_target(world, index); if let Recipe::Rule { rule } = &target.recipe { - match rule { - StringOrList::String(name) => assert_eq!(name, &rule_name), - other => panic!("Expected String, got: {other:?}"), - } + let actual = get_string_from_string_or_list(rule, "rule"); + assert_eq!(actual, rule_name); } else { panic!("Expected rule recipe, got: {:?}", target.recipe); } }tests/manifest_jinja_tests.rs (4)
66-92: Add a test for typed YAML lists in foreach.Exercise the YAML-first flow by adding a test that uses a native YAML array (no quotes) for
foreach. This locks down support beyond the stringified list form.#[rstest] fn expands_foreach_targets_typed_yaml_list() { let yaml = manifest_yaml( "targets:\n - foreach: ['a', 'b']\n name: '{{ item }}'\n command: 'echo {{ item }}'\n", ); let manifest = manifest::from_str(&yaml).expect("parse"); let names: Vec<_> = manifest .targets .iter() .map(|t| match &t.name { netsuke::ast::StringOrList::String(s) => s.clone(), other => panic!("Expected String, got: {other:?}"), }) .collect(); assert_eq!(names, vec!["a", "b"]); }
95-101: Fix boolean semantics in when-case (do not quote false).Quoting false produces a non-empty string, which is truthy in Jinja/minijinja. This invalidates the intent of the test.
#[rstest] #[case("[]", "", "no targets should be generated for empty foreach list")] -#[case( - "['a', 'b']", - "'false'", - "no targets should be generated when condition is always false" -)] +#[case("['a', 'b']", "false", "no targets should be generated when condition is always false")] fn no_targets_generated_scenarios(
161-177: Also assert commands to fully validate per-item rendering.Strengthen the test by asserting rendered commands alongside names.
assert_eq!(names, vec!["a", "b"]); + let commands: Vec<_> = manifest + .targets + .iter() + .map(|t| match &t.recipe { + Recipe::Command { command } => command.clone(), + other => panic!("Expected command recipe, got: {other:?}"), + }) + .collect(); + assert_eq!(commands, vec!["echo a", "echo b"]);
179-215: Extract StringOrList assertions into helpers to remove duplication.Replace repetitive matches with small helpers for equality and list-membership. This makes failures clearer and future changes cheaper.
Add helpers:
fn assert_string_or_list_eq(actual: &netsuke::ast::StringOrList, expected: &str, field: &str) { match actual { netsuke::ast::StringOrList::String(s) => assert_eq!(s, expected), netsuke::ast::StringOrList::List(list) if list.len() == 1 => assert_eq!(&list[0], expected), other => panic!("Expected String for {field}, got: {other:?}"), } } fn assert_string_or_list_contains( actual: &netsuke::ast::StringOrList, expected: &[String], field: &str, ) { match actual { netsuke::ast::StringOrList::List(list) => assert_eq!(list, expected), other => panic!("Expected List for {field}, got: {other:?}"), } }Then refactor the assertions in this test:
- match &target.name { - netsuke::ast::StringOrList::String(n) => assert_eq!(n, "base1"), - other => panic!("Expected String, got: {other:?}"), - } + assert_string_or_list_eq(&target.name, "base1", "name"); - match &target.sources { - netsuke::ast::StringOrList::List(list) => assert_eq!(list, &["base1.src".to_string()]), - other => panic!("Expected List, got: {other:?}"), - } + assert_string_or_list_contains(&target.sources, &["base1.src".to_string()], "sources"); - match &target.deps { - netsuke::ast::StringOrList::List(list) => assert_eq!(list, &["base1.dep".to_string()]), - other => panic!("Expected List, got: {other:?}"), - } + assert_string_or_list_contains(&target.deps, &["base1.dep".to_string()], "deps"); - match &target.order_only_deps { - netsuke::ast::StringOrList::List(list) => { - assert_eq!(list, &["base1.ord".to_string()]); - } - other => panic!("Expected List, got: {other:?}"), - } + assert_string_or_list_contains( + &target.order_only_deps, + &["base1.ord".to_string()], + "order_only_deps", + );
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
💡 Knowledge Base configuration:
- Jira integration is disabled
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (9)
docs/netsuke-design.md(9 hunks)docs/roadmap.md(1 hunks)examples/visual_design.yml(1 hunks)examples/website.yml(1 hunks)examples/writing.yml(1 hunks)tests/data/foreach.yml(1 hunks)tests/features/manifest.feature(1 hunks)tests/manifest_jinja_tests.rs(3 hunks)tests/steps/manifest_steps.rs(4 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.md
📄 CodeRabbit Inference Engine (AGENTS.md)
**/*.md: Markdown files must use en-GB-oxendict ("-ize" / "-yse" / "-our") spelling and grammar. (EXCEPTION: the naming of the "LICENSE" file, which is to be left unchanged for community consistency.)
Validate Markdown files using make markdownlint.
Run make fmt after any documentation changes to format all Markdown files and fix table markup.
Validate Mermaid diagrams in Markdown files by running make nixie.
Markdown paragraphs and bullet points must be wrapped at 80 columns.
Code blocks must be wrapped at 120 columns.
Tables and headings must not be wrapped.
Use dashes (-) for list bullets.
Use GitHub-flavoured Markdown footnotes ([^1]) for references and footnotes.
Files:
docs/roadmap.mddocs/netsuke-design.md
⚙️ CodeRabbit Configuration File
**/*.md: * Avoid 2nd person or 1st person pronouns ("I", "you", "we")
- Use en-GB-oxendict (-ize / -our) spelling and grammar
- Headings must not be wrapped.
- Documents must start with a level 1 heading
- Headings must correctly increase or decrease by no more than one level at a time
- Use GitHub-flavoured Markdown style for footnotes and endnotes.
- Numbered footnotes must be numbered by order of appearance in the document.
Files:
docs/roadmap.mddocs/netsuke-design.md
docs/**/*.md
📄 CodeRabbit Inference Engine (AGENTS.md)
docs/**/*.md: Reference: Use the markdown files within the docs/ directory as a knowledge base and source of truth for project requirements, dependency choices, and architectural decisions.
Update: When new decisions are made, requirements change, libraries are added/removed, or architectural patterns evolve, proactively update the relevant file(s) in the docs/ directory to reflect the latest state. Ensure the documentation remains accurate and current.
Files:
docs/roadmap.mddocs/netsuke-design.md
**/*.rs
📄 CodeRabbit Inference Engine (AGENTS.md)
**/*.rs: Comment why, not what. Explain assumptions, edge cases, trade-offs, or complexity. Don't echo the obvious.
Use functions and composition. Avoid repetition by extracting reusable logic. Prefer generators or comprehensions, and declarative code to imperative repetition when readable.
Small, meaningful functions. Functions must be small, clear in purpose, single responsibility, and obey command/query segregation.
Name things precisely. Use clear, descriptive variable and function names. For booleans, prefer names with is, has, or should.
Every module must begin with a module level (//! ) comment explaining the module's purpose and utility.
Document public APIs using Rustdoc comments (///) so documentation can be generated with cargo doc.
Place function attributes after doc comments.
Do not use return in single-line functions.
Prefer immutable data and avoid unnecessary mut bindings.
Handle errors with the Result type instead of panicking where feasible.
Prefer .expect() over .unwrap().
Use concat!() to combine long string literals rather than escaping newlines with a backslash.
Prefer single line versions of functions where appropriate.
Clippy warnings MUST be disallowed.
Lints must not be silenced except as a last resort.
Lint rule suppressions must be tightly scoped and include a clear reason.
Prefer expect over allow.
Keep file size manageable. No single code file may be longer than 400 lines. Long switch statements or dispatch tables should be broken up by feature and constituents colocated with targets. Large blocks of test data should be moved to external data files.
Illustrate with clear examples. Function documentation must include clear examples demonstrating the usage and outcome of the function. Test documentation should omit examples where the example serves only to reiterate the test logic.
Prefer semantic error enums. Derive std::error::Error (via the thiserror crate) for any condition the caller might inspect, retry, or map to an HTTP status.
...
Files:
tests/steps/manifest_steps.rstests/manifest_jinja_tests.rs
⚙️ CodeRabbit Configuration File
**/*.rs: * Seek to keep the cyclomatic complexity of functions no more than 12.
Adhere to single responsibility and CQRS
Place function attributes after doc comments.
Do not use
returnin single-line functions.Move conditionals with >2 branches into a predicate function.
Avoid
unsafeunless absolutely necessary.Every module must begin with a
//!doc comment that explains the module's purpose and utility.Comments and docs must follow en-GB-oxendict (-ize / -our) spelling and grammar
Lints must not be silenced except as a last resort.
#[allow]is forbidden.- Only narrowly scoped
#[expect(lint, reason = "...")]is allowed.- No lint groups, no blanket or file-wide suppression.
- Include
FIXME:with link if a fix is expected.Where code is only used by specific features, it must be conditionally compiled or a conditional expectation for unused_code applied.
Use
rstestfixtures for shared setup and to avoid repetition between tests.Replace duplicated tests with
#[rstest(...)]parameterised cases.Prefer
mockallfor mocks/stubs.Prefer
.expect()over.unwrap()Ensure that any API or behavioural changes are reflected in the documentation in
docs/Ensure that any completed roadmap steps are recorded in the appropriate roadmap in
docs/Files must not exceed 400 lines in length
- Large modules must be decomposed
- Long match statements or dispatch tables should be decomposed by domain and collocated with targets
- Large blocks of inline data (e.g., test fixtures, constants or templates) must be moved to external files and inlined at compile-time or loaded at run-time.
Files:
tests/steps/manifest_steps.rstests/manifest_jinja_tests.rs
🧬 Code Graph Analysis (1)
tests/manifest_jinja_tests.rs (2)
src/manifest.rs (1)
from_str(24-45)src/runner.rs (1)
is_empty(96-98)
🔍 Remote MCP Deepwiki
Relevant facts to aid reviewing PR #123 (concise):
-
Design & pipeline
- Repo docs describe a five-stage pipeline; PR shifts to YAML-first, three-stage flow: parse YAML → expand foreach/when → render templated values in string fields. (docs explain same pipeline and required semantics)
-
Public AST / types
- Docs show AST shapes and fields (NetsukeManifest, Rule, Target, Recipe, StringOrList). PR exposes concrete public AST structs matching the documented schema.
-
Manifest parsing (src/manifest.rs)
- The manifest module is the intended place for template/parse changes; docs map manifest::from_str/from_path as public API entry points — PR keeps those signatures while changing internal flow to parse-first then foreach expansion and per-field rendering.
-
Template semantics & functions
- Template engine (minijinja) functions (env, glob, basename, shell_escape, etc.) are documented as registered in the environment; docs emphasise security (automatic shell-escape) and that templating should be restricted to string values — the PR implements value-level rendering accordingly.
-
foreach/when behavior
- Roadmap & docs specify foreach/when semantics (item/index iteration, per-iteration locals layered over target.vars) — PR implements foreach expansion phase (evaluate foreach, apply when filter, inject item/index vars) consistent with roadmap.
-
Examples & tests
- Examples under examples/ were converted from moustache-quoted foreach strings to bare expressions (e.g., "{{ glob(...) }}" → glob(...)); docs show glob() returns lists used by templating and pipeline expectations. Tests updated/added (tests/data/*, features, steps, and unit tests) to validate foreach expansion, when filtering, rendering of command/script/rule fields, and error cases for non-iterable foreach. Check that step definitions and BDD scenarios align with updated step helpers and assertions.
-
IR & determinism concerns
- Docs: IR (src/ir.rs) deduplicates actions via stable hashing and expects deterministic ordering; rendering changes must preserve deterministic outputs for snapshots/IR hashing. PR adds render_manifest then IR generation — verify ordering and hashing unchanged.
-
Error handling
- Docs describe structured errors (thiserror/anyhow) and validation flow. PR added error constants (ERR_INITIAL_YAML_PARSE, ERR_MANIFEST_PARSE) and uses anyhow for contextual errors — ensure messages and propagation match project patterns.
-
Developer / CI expectations
- Repo Makefile and docs require make fmt, lint, test, markdownlint, nixie; PR author ran these locally (nixie failed due to external mermaid-cli bun error). Ensure CI checks (format, lint, tests, snapshots) are green and that nixie failure is external, not code-related.
Tools/sources used:
- Repository wiki / documentation fetch: read_wiki_contents, read_wiki_structure (Deepwiki),
⏰ Context from checks skipped due to timeout of 120000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build-test
🔇 Additional comments (7)
examples/writing.yml (1)
19-24: Stabilise iteration order — LGTMSwitching to
sorted(glob(...))forforeachenforces deterministic target generation and resolves prior flakiness concerns.examples/website.yml (1)
17-25: Use consistent| sortfilter across examples
- Replace
| sortedwith| sortinexamples/website.ymlto match other example files.- Update all other examples (e.g.
examples/writing.ymlLine 27) to use| sortfor uniformity.- Verify that the templating environment registers a
sortfilter and notsorted.- sources: "{{ glob(site_dir ~ '/*.html') | exclude(site_dir ~ '/index.html') | sorted }}" + sources: "{{ glob(site_dir ~ '/*.html') | exclude(site_dir ~ '/index.html') | sort }}"examples/visual_design.yml (1)
16-20: Deterministic iteration order — LGTMUsing
sorted(glob(...))forforeachlocks in stable processing of SVGs and aligns with the YAML-first, value-level templating model.docs/roadmap.md (1)
99-103: Document foreach/when semantics — LGTMMarking
foreach/whenas implemented and documenting theitem/indexlayering matches the code and tests added in this PR.docs/netsuke-design.md (1)
596-600: Reaffirm the YAML-first separation — LGTMThe ingestion pipeline description matches the implementation: YAML parse → foreach/when expansion → post-deserialisation string rendering.
tests/steps/manifest_steps.rs (1)
37-43: Good: Centralise target lookup with get_target.The helper removes repeated manifest/target plumbing and improves readability across step implementations.
tests/manifest_jinja_tests.rs (1)
50-50: LGTM: Quote the command Jinja block in YAML.Quoting the Jinja conditional in the YAML string is correct for the value-level rendering phase.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 3
🔭 Outside diff range comments (2)
examples/writing.yml (1)
27-27: Fix undefined or inconsistent sorting filter.Align with the rest of the examples and the documented function set; use
sortedrather thansortto avoid a runtime template error.- sources: "{{ glob(build_dir ~ '/*.tex') | sort }}" + sources: "{{ glob(build_dir ~ '/*.tex') | sorted }}"docs/netsuke-design.md (1)
561-565: Align wording with the YAML-first multi-stage approachDo not refer to this as a “two-pass” approach in a section titled “YAML-First Multi-Stage Ingestion”.
Apply this fix:
-The integration of a templating engine like Jinja fundamentally shapes the -parsing pipeline, mandating a two-pass approach. It is impossible to parse the -user's `Netsukefile` file with `serde_yml` in a single step. +The integration of a templating engine like Jinja fundamentally shapes the +parsing pipeline, mandating a multi-stage approach. It is impractical to parse +the user's `Netsukefile` into the final AST in a single step.
♻️ Duplicate comments (4)
examples/visual_design.yml (1)
16-16: Sort the globbed inputs to stabilise target generation order.Ensure deterministic builds by sorting the foreach sequence.
- - foreach: glob(src_dir ~ '/*.svg') + - foreach: sorted(glob(src_dir ~ '/*.svg'))examples/website.yml (1)
17-17: Enforce deterministic build order by sorting the glob.Stabilise target ordering across platforms and runs by sorting the foreach input.
- - foreach: glob(pages_dir ~ '/*.md') + - foreach: sorted(glob(pages_dir ~ '/*.md'))examples/writing.yml (1)
19-19: Sort the foreach sequence for deterministic output.Stabilise chapter processing order to avoid flaky diffs and builds.
- - foreach: glob(chapters_dir ~ '/*.md') + - foreach: sorted(glob(chapters_dir ~ '/*.md'))tests/steps/manifest_steps.rs (1)
147-153: Extract string retrieval from StringOrList to a helper and reuseAvoid repeating the pattern match on
StringOrListacross assertion functions.Apply this refactor to use a shared helper:
fn assert_target_name(world: &CliWorld, index: usize, name: &str) { - let target = get_target(world, index); - match &target.name { - StringOrList::String(value) => assert_eq!(value, name), - other => panic!("Expected StringOrList::String, got: {other:?}"), - } + let target = get_target(world, index); + let actual = get_string_from_string_or_list(&target.name, "name"); + assert_eq!(actual, name); }Add this helper near the other local helpers:
fn get_string_from_string_or_list<'a>( value: &'a StringOrList, field_name: &str, ) -> &'a str { match value { StringOrList::String(s) => s.as_str(), StringOrList::List(list) if list.len() == 1 => list[0].as_str(), other => panic!("Expected String for {field_name}, got: {other:?}"), } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
💡 Knowledge Base configuration:
- Jira integration is disabled
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (10)
docs/netsuke-design.md(10 hunks)examples/visual_design.yml(1 hunks)examples/website.yml(1 hunks)examples/writing.yml(1 hunks)src/manifest.rs(1 hunks)tests/data/foreach.yml(1 hunks)tests/data/foreach_invalid.yml(1 hunks)tests/features/manifest.feature(1 hunks)tests/manifest_jinja_tests.rs(4 hunks)tests/steps/manifest_steps.rs(4 hunks)
🧰 Additional context used
📓 Path-based instructions (5)
**/*.rs
📄 CodeRabbit Inference Engine (AGENTS.md)
**/*.rs: Comment why, not what; document assumptions, edge cases, trade-offs, and complexity without restating obvious code
Extract reusable logic into functions; prefer composition and declarative constructs when readable
Keep functions small, single-responsibility, and observe command/query separation
Use precise, descriptive names; boolean names should start with is/has/should
Use en-GB-oxendict spelling/grammar in comments (except external API references)
Function docs must include clear usage examples; avoid redundant examples in test docs
No Rust source file may exceed 400 lines; split long switches/tables; move large test data to external files
Fix test warnings in code; do not silence them
Extract helpers when functions are too long; group many parameters into well-named structs
Consider using Arc when returning large errors to reduce data movement
Document public APIs using Rustdoc (///) so cargo doc can generate documentation
Prefer immutable data; avoid unnecessary mut bindings
Prefer Result-based error handling over panicking where feasible
Avoid unsafe code unless absolutely necessary and document any usage clearly
Place function attributes after doc comments
Do not use return in single-line functions
Use predicate helper functions when conditionals have more than two branches
Do not silence lints except as a last resort; suppressions must be tightly scoped and include a clear reason
Prefer #[expect(...)] over #[allow(...)] for lint management
Prefer .expect() over .unwrap()
Use conditional compilation (#[cfg]/#[cfg_attr]) for functions unused under specific feature sets
Use concat!() to join long string literals rather than using backslash-newline escapes
Prefer single-line function bodies where appropriate (e.g., pub fn new(id: u64) -> Self { Self(id) })
Prefer semantic error enums deriving std::error::Error via thiserror for caller-inspectable conditions
Files:
src/manifest.rstests/steps/manifest_steps.rstests/manifest_jinja_tests.rs
⚙️ CodeRabbit Configuration File
**/*.rs: * Seek to keep the cyclomatic complexity of functions no more than 12.
Adhere to single responsibility and CQRS
Place function attributes after doc comments.
Do not use
returnin single-line functions.Move conditionals with >2 branches into a predicate function.
Avoid
unsafeunless absolutely necessary.Every module must begin with a
//!doc comment that explains the module's purpose and utility.Comments and docs must follow en-GB-oxendict (-ize / -our) spelling and grammar
Lints must not be silenced except as a last resort.
#[allow]is forbidden.- Only narrowly scoped
#[expect(lint, reason = "...")]is allowed.- No lint groups, no blanket or file-wide suppression.
- Include
FIXME:with link if a fix is expected.Where code is only used by specific features, it must be conditionally compiled or a conditional expectation for unused_code applied.
Use
rstestfixtures for shared setup and to avoid repetition between tests.Replace duplicated tests with
#[rstest(...)]parameterised cases.Prefer
mockallfor mocks/stubs.Prefer
.expect()over.unwrap()Ensure that any API or behavioural changes are reflected in the documentation in
docs/Ensure that any completed roadmap steps are recorded in the appropriate roadmap in
docs/Files must not exceed 400 lines in length
- Large modules must be decomposed
- Long match statements or dispatch tables should be decomposed by domain and collocated with targets
- Large blocks of inline data (e.g., test fixtures, constants or templates) must be moved to external files and inlined at compile-time or loaded at run-time.
Files:
src/manifest.rstests/steps/manifest_steps.rstests/manifest_jinja_tests.rs
src/**/*.rs
📄 CodeRabbit Inference Engine (AGENTS.md)
src/**/*.rs: Every module must begin with a //! module-level comment explaining its purpose and utility
Never export eyre::Report from library code; convert to domain error enums at API boundaries
Files:
src/manifest.rs
docs/**/*.md
📄 CodeRabbit Inference Engine (AGENTS.md)
docs/**/*.md: Treat docs/ markdown as the reference source of truth for requirements and decisions; keep updated with changes
Use en-GB-oxendict spelling/grammar in documentation (LICENSE name unchanged)
Files:
docs/netsuke-design.md
**/*.md
📄 CodeRabbit Inference Engine (AGENTS.md)
**/*.md: Validate Markdown with make markdownlint
Wrap Markdown paragraphs and bullet points at 80 columns; do not wrap tables or headings
Wrap code blocks in Markdown at 120 columns
Use dashes (-) for list bullets
Use GitHub-flavoured Markdown footnotes ([^1]) for references
Validate Mermaid diagrams in Markdown using make nixie
Files:
docs/netsuke-design.md
⚙️ CodeRabbit Configuration File
**/*.md: * Avoid 2nd person or 1st person pronouns ("I", "you", "we")
- Use en-GB-oxendict (-ize / -our) spelling and grammar
- Headings must not be wrapped.
- Documents must start with a level 1 heading
- Headings must correctly increase or decrease by no more than one level at a time
- Use GitHub-flavoured Markdown style for footnotes and endnotes.
- Numbered footnotes must be numbered by order of appearance in the document.
Files:
docs/netsuke-design.md
tests/**/*.rs
📄 CodeRabbit Inference Engine (AGENTS.md)
tests/**/*.rs: Use rstest fixtures for shared setup and replace duplicated tests with #[rstest(...)] parameterised cases
Prefer mockall for mocks/stubs
Mock non-deterministic dependencies via dependency injection (e.g., Env/Clock traits) using mockable crate; follow docs/reliable-testing-in-rust-via-dependency-injection.md
Files:
tests/steps/manifest_steps.rstests/manifest_jinja_tests.rs
🧬 Code Graph Analysis (1)
tests/manifest_jinja_tests.rs (1)
src/manifest.rs (1)
from_str(24-45)
🔍 Remote MCP Deepwiki
Relevant additional facts for reviewing PR #123
-
Design & pipeline: repo docs describe shifting from the original five-stage flow to a YAML-first, three-stage ingestion: parse YAML → expand foreach/when → render string-valued templates; this is documented and aligned with the PR’s intent.,
-
Public AST: docs explicitly define the public AST shapes (NetsukeManifest, Rule, Target, Recipe, StringOrList) with serde attributes and deny_unknown_fields — matches PR’s added pub structs in src/ast.rs.
-
Manifest module responsibilities: manifest::from_str/from_path are the public entry points; manifest.rs is the expected location for parse/templating changes (rendering + foreach expansion belong here). PR keeps signatures but changes internal flow.
-
Foreach/when semantics: the roadmap/docs describe foreach/when as producing per-iteration targets with injected item/index vars layered over target.vars; glob() and env() functions are part of the templating function set. Tests and examples were updated to use bare expressions for foreach (no moustache wrapper).
-
Rendering & security: docs require templating be restricted to string values and mandate automatic shell-escaping (shell-quote + shlex validation) for commands — PR’s move to value-level rendering must preserve this escaping/validation.
-
IR determinism: IR generation deduplicates actions by stable hashing and requires deterministic ordering (snapshots rely on this). Rendering changes must not break ordering or hash stability used by ir::from_manifest and snapshot tests.
-
Error handling conventions: project uses thiserror for structured errors and anyhow for contextual propagation; manifest module in PR adds ERR_INITIAL_YAML_PARSE and ERR_MANIFEST_PARSE — ensure these follow existing patterns and provide comparable context/messages.
-
Tests & examples: tests were converted from Jinja for-loops to foreach-driven fixtures (new tests under tests/data/*, feature scenario updates, new step helpers). Examples changed foreach values from "{{ glob(...) }}" to glob(...). Review required: ensure tests cover non-iterable foreach errors, when-filtering, rendering of command/script/rule recipe types, and missing-var errors.
-
CI / developer tooling: Makefile targets (make fmt, check-fmt, lint, test, markdownlint, nixie) are expected; author ran them locally (nixie failed externally). Verify CI will run same checks and that any mermaid/nixie failure is external.
Tools/sources used
- Repo wiki structure listing and page contents:,
⏰ Context from checks skipped due to timeout of 120000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build-test
🔇 Additional comments (32)
tests/data/foreach_invalid.yml (1)
1-5: LGTM: Strong invalid-foreach fixture.Use of a bare integer for foreach robustly exercises the “not iterable” error path.
tests/data/foreach.yml (1)
3-7: LGTM: Showcase YAML-first foreach with safe echoing.Drive home the YAML-first design and avoid word-splitting by quoting the echoed item. This is a solid positive-case fixture.
tests/features/manifest.feature (2)
71-80: Lock in foreach generation and index semanticsThe scenario asserts both names and index (0-based) and quotes the echoed item. This aligns with the YAML-first + foreach pipeline.
82-97: Exercise error paths and field rendering comprehensivelyThe added scenarios validate non-iterable foreach errors and per-field rendering (sources, deps, order-only, script, rule). This coverage is appropriate and aligned with the new AST semantics.
tests/steps/manifest_steps.rs (2)
37-43: Introduce a single source of truth to fetch targetsThe
get_targethelper removes duplication and improves readability across steps. Keep it.
210-269: Additions expand behavioural coverage over all relevant fieldsThe new steps covering sources, deps, order-only deps, script, and rule variants look correct and reuse the
get_targethelper effectively.src/manifest.rs (11)
3-6: Document the YAML-first contract at module entryThe module-level docs clearly state the separation: parse YAML, expand foreach/when, then render string values. Keep this as the source of truth.
25-45: Enforce strict undefineds and seed environment globals onceThe flow (parse YAML → seed env globals from top-level vars → expand foreach → deserialise → render) matches the design and retains strict undefined behaviour to surface errors early.
47-64: Delegate foreach expansion cleanly and preserve orderThe pass walks only
targetsand maintains deterministic ordering. This aligns with the YAML-first pipeline.
65-83: Keep foreach-less mappings intact; expand iterables with when/vars layeringThe helper split (expand_target/when_allows/inject_iteration_vars) keeps complexity contained and readable.
85-96: Accept both native YAML sequences and string expressions in foreachHandling typed YAML lists directly while evaluating string expressions via minijinja is correct and user-friendly.
97-111: Treat when as a proper expression over item/indexUsing
is_true()over a strict environment gives predictable filtering semantics.
147-160: Render actions, targets, and rules in a single traversalThe final rendering pass is cohesive and keeps per-entity concerns isolated. Good separation of concerns.
161-182: Render rule fields using env globals onlyRendering
description,deps, and the recipe with an empty local context relies on env globals, which is correct for rules.
184-204: Render target fields with layered vars contextRendering name/sources/deps/order-only and recipe against
target.vars(on top of env globals) matches the documented precedence.
206-216: Snapshot target vars before rendering to avoid self-referential churnCloning vars up-front prevents cascading mutations. This is the right choice for determinism.
218-231: Render StringOrList variants in placeIn-place rendering maintains structure and supports both single and list forms cleanly.
tests/manifest_jinja_tests.rs (10)
10-19: Centralise StringOrList assertionsThe helpers reduce repetition and clarify intent across tests. Keep them.
71-71: Stabilise command templating with proper quoting in the string literalQuoting the Jinja-if command string avoids YAML parsing ambiguity. Good fix.
87-113: Validate foreach expansion and per-item command renderingAsserting both names and the quoted commands hardens the behaviour contract.
141-160: Assert single-item foreach expansion end-to-endThe test covers both target naming and command rendering for the single-item case. Looks good.
162-176: Parameterise non-iterable foreach error casesExercising both typed and string forms improves error-path coverage. Keep it.
178-195: Filter foreach items via when with item contextThis test confirms
itemvisibility and filtering semantics. Good coverage.
197-218: Render all core target fields with local vars precedenceThe test robustly verifies name/sources/deps/order-only and command rendering with layered vars. Good.
220-249: De-duplicate recipe-type tests via rstest casesConsolidating script and rule cases into a single parameterised test keeps the suite DRY without losing coverage. Good refactor.
251-268: Exercise error cases aligned with value-level renderingAsserting errors on sources and inline if-blocks reflects the YAML-first, string-only rendering stage. Keep these.
115-139: Fix boolean semantics in when: do not quote falseRemove quotes around false so Jinja treats it as boolean false. No other quoted 'false' patterns remain in tests.
Apply this change:
-#[case( - "['a', 'b']", - "'false'", - "no targets should be generated when condition is always false" -)] +#[case("['a', 'b']", "false", "no targets should be generated when condition is always false")]docs/netsuke-design.md (5)
261-263: Clarify enum variant selection by field presenceThe updated sentence makes the selection criterion explicit. Keep it.
352-365: Document foreach/when expansion flow with a clear diagramThe mermaid diagram matches the implemented expansion semantics. Keep it.
401-409: Use en-GB spelling and accurate derive guidanceThe text now uses “deserialise” and points to
#[derive(Deserialize)]explicitly. Good editorial correction.
421-426: Reference concrete derive attributes for AST typesThe derive guidance matches the public AST and is clear. Keep it.
610-614: Reinforce the three-stage ingestion within the docsThis paragraph correctly reiterates YAML-first parsing → expansion → final string rendering. Good alignment with code.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
tests/manifest_jinja_tests.rs (1)
223-239: Also assert rendered commands to fully validate per-item rendering.Strengthen the test by checking the recipe, not just names.
Apply this diff:
let manifest = manifest::from_str(&yaml).expect("parse"); assert_eq!(manifest.targets.len(), 2); let names: Vec<_> = manifest .targets .iter() .map(|t| match &t.name { netsuke::ast::StringOrList::String(s) => s.clone(), other => panic!("Expected String, got: {other:?}"), }) .collect(); assert_eq!(names, vec!["a", "b"]); + + let commands: Vec<_> = manifest + .targets + .iter() + .map(|t| match &t.recipe { + Recipe::Command { command } => command.clone(), + other => panic!("Expected command recipe, got: {other:?}"), + }) + .collect(); + assert_eq!(commands, vec!["echo 'a'", "echo 'b'"]);
♻️ Duplicate comments (1)
tests/manifest_jinja_tests.rs (1)
159-166: Fix boolean semantics in when-case: do not quote false.Quoting false produces a non-empty string which is truthy in Jinja/minijinja, defeating the test intent.
Apply this diff:
#[rstest] #[case("[]", "", "no targets should be generated for empty foreach list")] #[case( "['a', 'b']", - "'false'", + "false", "no targets should be generated when condition is always false" )]
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
💡 Knowledge Base configuration:
- Jira integration is disabled
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
tests/manifest_jinja_tests.rs(5 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.rs
📄 CodeRabbit Inference Engine (AGENTS.md)
**/*.rs: Comment why, not what; document assumptions, edge cases, trade-offs, and complexity without restating obvious code
Extract reusable logic into functions; prefer composition and declarative constructs when readable
Keep functions small, single-responsibility, and observe command/query separation
Use precise, descriptive names; boolean names should start with is/has/should
Use en-GB-oxendict spelling/grammar in comments (except external API references)
Function docs must include clear usage examples; avoid redundant examples in test docs
No Rust source file may exceed 400 lines; split long switches/tables; move large test data to external files
Fix test warnings in code; do not silence them
Extract helpers when functions are too long; group many parameters into well-named structs
Consider using Arc when returning large errors to reduce data movement
Document public APIs using Rustdoc (///) so cargo doc can generate documentation
Prefer immutable data; avoid unnecessary mut bindings
Prefer Result-based error handling over panicking where feasible
Avoid unsafe code unless absolutely necessary and document any usage clearly
Place function attributes after doc comments
Do not use return in single-line functions
Use predicate helper functions when conditionals have more than two branches
Do not silence lints except as a last resort; suppressions must be tightly scoped and include a clear reason
Prefer #[expect(...)] over #[allow(...)] for lint management
Prefer .expect() over .unwrap()
Use conditional compilation (#[cfg]/#[cfg_attr]) for functions unused under specific feature sets
Use concat!() to join long string literals rather than using backslash-newline escapes
Prefer single-line function bodies where appropriate (e.g., pub fn new(id: u64) -> Self { Self(id) })
Prefer semantic error enums deriving std::error::Error via thiserror for caller-inspectable conditions
Files:
tests/manifest_jinja_tests.rs
⚙️ CodeRabbit Configuration File
**/*.rs: * Seek to keep the cyclomatic complexity of functions no more than 12.
Adhere to single responsibility and CQRS
Place function attributes after doc comments.
Do not use
returnin single-line functions.Move conditionals with >2 branches into a predicate function.
Avoid
unsafeunless absolutely necessary.Every module must begin with a
//!doc comment that explains the module's purpose and utility.Comments and docs must follow en-GB-oxendict (-ize / -our) spelling and grammar
Lints must not be silenced except as a last resort.
#[allow]is forbidden.- Only narrowly scoped
#[expect(lint, reason = "...")]is allowed.- No lint groups, no blanket or file-wide suppression.
- Include
FIXME:with link if a fix is expected.Where code is only used by specific features, it must be conditionally compiled or a conditional expectation for unused_code applied.
Use
rstestfixtures for shared setup and to avoid repetition between tests.Replace duplicated tests with
#[rstest(...)]parameterised cases.Prefer
mockallfor mocks/stubs.Prefer
.expect()over.unwrap()Ensure that any API or behavioural changes are reflected in the documentation in
docs/Ensure that any completed roadmap steps are recorded in the appropriate roadmap in
docs/Files must not exceed 400 lines in length
- Large modules must be decomposed
- Long match statements or dispatch tables should be decomposed by domain and collocated with targets
- Large blocks of inline data (e.g., test fixtures, constants or templates) must be moved to external files and inlined at compile-time or loaded at run-time.
Files:
tests/manifest_jinja_tests.rs
tests/**/*.rs
📄 CodeRabbit Inference Engine (AGENTS.md)
tests/**/*.rs: Use rstest fixtures for shared setup and replace duplicated tests with #[rstest(...)] parameterised cases
Prefer mockall for mocks/stubs
Mock non-deterministic dependencies via dependency injection (e.g., Env/Clock traits) using mockable crate; follow docs/reliable-testing-in-rust-via-dependency-injection.md
Files:
tests/manifest_jinja_tests.rs
🧬 Code Graph Analysis (1)
tests/manifest_jinja_tests.rs (1)
src/manifest.rs (1)
from_str(24-45)
🔍 Remote MCP Deepwiki
Summary of additional relevant facts for reviewing PR #123
-
Project documentation already describes and prescribes the YAML-first, multi-stage pipeline (parse YAML → template expansion/foreach → deserialize → render string fields → IR → Ninja); the PR aligns with that design and exposes the public AST (NetsukeManifest, Rule, Target, Recipe, StringOrList) with serde deny_unknown_fields semantics.
-
Module mapping / responsibilities:
- src/manifest.rs is the canonical place for manifest ingestion/rendering; from_str/from_path are public entry points (signatures unchanged). The manifest module is documented to perform templating (minijinja) before serde deserialisation in the intended pipeline; PR changes manifest.rs to parse YAML first, run foreach expansion, then deserialize and render string fields — consistent with docs.
-
Foreach/when semantics and template functions:
- Roadmap/docs specify foreach/when should generate per-iteration targets with injected item/index and restrict templating to string values; built-in functions include glob() and env(). Examples in tests and examples were updated to use bare expressions for foreach (e.g., foreach: glob(...)). PR changes to examples/tests follow this.
-
Tests updated/added:
- BDD scenarios (tests/features/*.feature), cucumber step helpers, and unit tests were converted from Jinja loop fixtures to foreach-driven fixtures; new test data files added (tests/data/foreach.yml, foreach_invalid.yml, render_target.yml) and old Jinja-for tests removed. Tests cover: foreach expansion, when filtering, non-iterable foreach errors, rendering of name/sources/deps/order-only-deps/command/script/rule, and missing-var errors.
-
Security and determinism concerns to verify in review:
- Docs mandate automatic shell-escaping (shell-quote) and shlex validation for commands and require IR determinism (stable ordering, SHA-based action deduplication). The PR introduces a new render phase and foreach expansion that must preserve escaping/validation and not alter ordering/hash stability used by ir::from_manifest and snapshot tests. Confirm that render_manifest + render helpers apply shell-escape + shlex validation where required and that action hashing remains stable.
-
Error handling conventions:
- Project uses thiserror for structured errors and anyhow for contexts. PR adds ERR_INITIAL_YAML_PARSE and ERR_MANIFEST_PARSE constants — verify they follow existing error messaging/context conventions.
-
CI / tooling:
- Makefile targets (make fmt, check-fmt, lint, test, markdownlint, nixie) are standard; author ran them locally (nixie failed due to external mermaid-cli bun error). Ensure CI runs the same gates and note nixie/mermaid is an external dependency if doc changes include mermaid diagrams.
Files/sections to focus review on (most relevant):
- src/manifest.rs (foreach expansion, parse→expand→deserialize→render flow, error contexts)
- src/ast.rs (new pub structs/enums, serde attrs)
- tests/** (new/updated feature scenarios and data files)
- examples/*.yml (foreach expression changes)
- ir module / hashing logic (ensure no regression in determinism)
- places where shell-quote / shlex validation are invoked after the new render phase
Sources used: repository wiki pages and docs (Overview / Architecture / Manifest Parsing / Roadmap / Testing) via read_wiki_structure and read_wiki_contents.,
⏰ Context from checks skipped due to timeout of 120000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build-test
🔇 Additional comments (14)
tests/manifest_jinja_tests.rs (14)
1-2: Document the three-stage pipeline upfront (LGTM).Keep this succinct module doc; it aligns with the YAML-first pipeline and satisfies the module-level doc requirement.
11-19: Extract assertion helper for StringOrList (LGTM).Improve test readability and failure messages without duplicating pattern matches.
21-30: Provide list assertion helper (LGTM).Lock down field shape and ordering, which also reinforces determinism expectations.
32-37: Deduplicate target extraction with a generic helper (LGTM).Eliminate structural duplication flagged by CodeScene by composing over a closure.
39-44: Name extraction uses precise shape checks (LGTM).Panic early on unexpected shapes to surface regressions fast in tests.
46-51: Command extraction is scoped to the Command recipe (LGTM).Keep this strict; add more extractors if future tests cover Script/Rule extraction.
93-93: Quote the templated command string (LGTM).Quote to avoid YAML parsing ambiguities and ensure the Jinja block is treated as a single scalar.
108-131: Parameterise foreach tests to cover typed and string list inputs (LGTM).Exercise both native YAML arrays and stringified lists, and assert both item and index use.
131-157: Assert names and commands with shared extractors (LGTM).Keep transformations local and assertions clear; this locks down stable ordering.
185-204: Single-item foreach path (LGTM).Assert exact cardinality and the rendered command; this is a useful guard for off-by-one bugs.
206-220: Non-iterable foreach error coverage (LGTM).Exercise both typed and string forms; this closes a common misconfiguration gap.
241-262: Per-target field rendering under foreach (LGTM).Validate name/sources/deps/order-only-deps and the shell-quoted command; this is aligned with the string-only rendering rule.
265-293: Unify recipe-type rendering checks (LGTM).Parameterise Script and Rule cases to remove duplication, preserving strict matching and clear panics on mismatched recipe types.
295-312: Error-path coverage for missing vars and undefined in if (LGTM).Assert strict undefined behaviour post-deserialisation for both list elements and inline conditionals.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 5
🔭 Outside diff range comments (1)
tests/manifest_jinja_tests.rs (1)
243-258: Also assert rendered commands to fully validate filtering.Validate both names and commands after when-based filtering to exercise per-item rendering end-to-end.
let names: Vec<_> = manifest .targets .iter() .map(|t| match &t.name { netsuke::ast::StringOrList::String(s) => s.clone(), other => panic!("Expected String, got: {other:?}"), }) .collect(); assert_eq!(names, vec!["a", "b"]); + + let commands: Vec<_> = manifest + .targets + .iter() + .map(|t| match &t.recipe { + Recipe::Command { command } => command.clone(), + other => panic!("Expected command recipe, got: {other:?}"), + }) + .collect(); + assert_eq!(commands, vec!["echo 'a'", "echo 'b'"]);
♻️ Duplicate comments (3)
tests/steps/manifest_steps.rs (3)
10-10: Resolve previous nit: use a named constant for injected index key — good.Eliminate magic strings and reduce typo risk when accessing vars["index"].
157-161: Deduplicate StringOrList extraction — good refactor.Use the shared extractor to keep assertions consistent and terse.
163-170: Target retrieval reuse — good.Route all field assertions through get_target to remove repeated manifest/target lookups.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
💡 Knowledge Base configuration:
- Jira integration is disabled
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
tests/manifest_jinja_tests.rs(5 hunks)tests/steps/manifest_steps.rs(4 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.rs
📄 CodeRabbit Inference Engine (AGENTS.md)
**/*.rs: Comment why, not what; document assumptions, edge cases, trade-offs, and complexity without restating obvious code
Extract reusable logic into functions; prefer composition and declarative constructs when readable
Keep functions small, single-responsibility, and observe command/query separation
Use precise, descriptive names; boolean names should start with is/has/should
Use en-GB-oxendict spelling/grammar in comments (except external API references)
Function docs must include clear usage examples; avoid redundant examples in test docs
No Rust source file may exceed 400 lines; split long switches/tables; move large test data to external files
Fix test warnings in code; do not silence them
Extract helpers when functions are too long; group many parameters into well-named structs
Consider using Arc when returning large errors to reduce data movement
Document public APIs using Rustdoc (///) so cargo doc can generate documentation
Prefer immutable data; avoid unnecessary mut bindings
Prefer Result-based error handling over panicking where feasible
Avoid unsafe code unless absolutely necessary and document any usage clearly
Place function attributes after doc comments
Do not use return in single-line functions
Use predicate helper functions when conditionals have more than two branches
Do not silence lints except as a last resort; suppressions must be tightly scoped and include a clear reason
Prefer #[expect(...)] over #[allow(...)] for lint management
Prefer .expect() over .unwrap()
Use conditional compilation (#[cfg]/#[cfg_attr]) for functions unused under specific feature sets
Use concat!() to join long string literals rather than using backslash-newline escapes
Prefer single-line function bodies where appropriate (e.g., pub fn new(id: u64) -> Self { Self(id) })
Prefer semantic error enums deriving std::error::Error via thiserror for caller-inspectable conditions
Files:
tests/steps/manifest_steps.rstests/manifest_jinja_tests.rs
⚙️ CodeRabbit Configuration File
**/*.rs: * Seek to keep the cyclomatic complexity of functions no more than 12.
Adhere to single responsibility and CQRS
Place function attributes after doc comments.
Do not use
returnin single-line functions.Move conditionals with >2 branches into a predicate function.
Avoid
unsafeunless absolutely necessary.Every module must begin with a
//!doc comment that explains the module's purpose and utility.Comments and docs must follow en-GB-oxendict (-ize / -our) spelling and grammar
Lints must not be silenced except as a last resort.
#[allow]is forbidden.- Only narrowly scoped
#[expect(lint, reason = "...")]is allowed.- No lint groups, no blanket or file-wide suppression.
- Include
FIXME:with link if a fix is expected.Where code is only used by specific features, it must be conditionally compiled or a conditional expectation for unused_code applied.
Use
rstestfixtures for shared setup and to avoid repetition between tests.Replace duplicated tests with
#[rstest(...)]parameterised cases.Prefer
mockallfor mocks/stubs.Prefer
.expect()over.unwrap()Ensure that any API or behavioural changes are reflected in the documentation in
docs/Ensure that any completed roadmap steps are recorded in the appropriate roadmap in
docs/Files must not exceed 400 lines in length
- Large modules must be decomposed
- Long match statements or dispatch tables should be decomposed by domain and collocated with targets
- Large blocks of inline data (e.g., test fixtures, constants or templates) must be moved to external files and inlined at compile-time or loaded at run-time.
Files:
tests/steps/manifest_steps.rstests/manifest_jinja_tests.rs
tests/**/*.rs
📄 CodeRabbit Inference Engine (AGENTS.md)
tests/**/*.rs: Use rstest fixtures for shared setup and replace duplicated tests with #[rstest(...)] parameterised cases
Prefer mockall for mocks/stubs
Mock non-deterministic dependencies via dependency injection (e.g., Env/Clock traits) using mockable crate; follow docs/reliable-testing-in-rust-via-dependency-injection.md
Files:
tests/steps/manifest_steps.rstests/manifest_jinja_tests.rs
🧬 Code Graph Analysis (1)
tests/manifest_jinja_tests.rs (1)
src/manifest.rs (1)
from_str(24-45)
🔍 Remote MCP Deepwiki
Relevant facts found for reviewing PR #123
-
Repo docs already specify the YAML-first multi-stage pipeline (parse YAML → template expansion/foreach → deserialize → render string fields → IR → Ninja). The PR’s changes to src/manifest.rs (parse YAML first, run foreach expansion, then render string fields) align with that design intent.,
-
The AST surface is public and strict (NetsukeManifest, Rule, Target, Recipe, StringOrList) with serde deny_unknown_fields; deserialisation is expected to be strict and is a key contract for manifest parsing. Changes to src/ast.rs (new pub structs/enums) must preserve serde behavior.
-
Manifest parsing responsibilities: manifest::from_str/from_path are the public entry points; manifest.rs now does: parse YAML → load vars into minijinja env → expand foreach/when → serde_yml::from_value → render string fields. Pay attention to new error constants ERR_INITIAL_YAML_PARSE and ERR_MANIFEST_PARSE and use of anyhow for context.
-
Foreach/when semantics: foreach and when are intended to generate per-iteration targets with injected item/index vars; docs and roadmap mark foreach/when as implemented and examples/tests updated to use bare expressions (e.g., foreach: glob(...)). Ensure expand_foreach and inject_iteration_vars implement item/index scoping and layering over target.vars/manifest.vars as described.
-
Tests and examples: many BDD feature files, test data, and step helpers were added/updated to exercise foreach expansion, when filtering, non-iterable foreach errors, rendering of all target fields (sources/deps/order-only-deps/command/script/rule), and per-target index injection. Validate test expectations match implementation (string rendering only in string fields; foreach evaluated prior to deserialisation).
-
Security / determinism concerns called out in docs that must be preserved by the new flow:
- Automatic shell-escaping (shell-quote) and shlex validation of final commands.
- IR determinism (stable ordering, SHA-based action deduplication) — verify render_manifest and foreach expansion do not break ordering or hashing used by ir::from_manifest and snapshot tests.
-
CI/tooling note: Makefile targets include make nixie to validate mermaid diagrams; author ran nixie locally and it failed due to external mermaid-cli bun error. If docs added/changed include mermaid diagrams (PR does), CI may fail or require the external tool.
Tools/sources used: functions.read_wiki_structure, functions.read_wiki_contents (wiki content exported above).,
⏰ Context from checks skipped due to timeout of 120000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build-test
🔇 Additional comments (6)
tests/steps/manifest_steps.rs (3)
172-181: Index assertion helper reads cleanly and is type-safe.Using INDEX_KEY and usize conversion is clear and robust.
218-247: Add list-field steps using the shared helpers — good coverage with low duplication.The steps for sources/dep/order-only dep lean on get_target and assert_list_contains appropriately.
248-275: Recipe-specific step defs are precise.Keep the panic messages recipe-specific to ease failures triage.
tests/manifest_jinja_tests.rs (3)
1-2: Document the YAML-first pipeline up-front — good orientation.The module-level docs succinctly anchor the new parse → foreach/when → render flow.
93-93: Quote the Jinja command string correctly in YAML — good fix.Use of double-quotes prevents YAML from mangling braces and matches the new “render string values” phase.
283-312: Parameterise recipe-type assertions — good deduplication.Collapse script/rule variants into one test while keeping variant-specific checks readable.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Reject targets with a non-mapping `vars` field so configuration errors are surfaced.\n\nAlso clarify when Jinja rendering occurs in the design doc and add a regression test.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
leynos
deleted the
codex/implement-dynamic-features-for-custom-functions
branch
1 participant
Summary
foreach/whenexpansion and render Jinja only in value fieldsTesting
make fmtmake check-fmtmake lintmake testmake markdownlintmake nixie(fails: Error running command bun x --bun @mermaid-js/mermaid-cli ...)https://chatgpt.com/codex/tasks/task_e_689fd546e328832294c3173c362fb38c
Summary by Sourcery
Parse manifests as YAML before templating, implement
foreach/whentarget expansion, and restrict Jinja evaluation to value-level string fields.New Features:
foreachandwhenkeys to generate targets dynamically withitemandindexvariables.Enhancements:
foreach, then render templated values.Documentation:
Tests:
foreachexpansion,whenfiltering, and related error cases in parsing.Chores:
foreachsyntax.