Conversation
Reviewer's GuideThis PR enables Jinja Class diagram for updated NetsukeManifest and Target vars typesclassDiagram
class NetsukeManifest {
Version netsuke_version
HashMap<String, serde_yml::Value> vars
Vec<Rule> rules
...
}
class Target {
StringOrList order_only_deps
HashMap<String, serde_yml::Value> vars
...
}
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. Summary by CodeRabbit
Summary by CodeRabbit
WalkthroughChange the manifest AST to map Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Loader as ManifestLoader
participant Engine as Minijinja
User->>Loader: Call from_str(yaml)
Loader->>Loader: Parse YAML (lenient pass) -> capture vars mapping
Loader->>Engine: Inject each var via Engine.global(name, Value::from_serialize(value))
Loader->>Engine: Render templates (second pass)
Engine-->>Loader: Rendered YAML text
Loader->>Loader: Strict deserialize to NetsukeManifest
Loader-->>User: Return parsed NetsukeManifest
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~15 minutes Possibly related PRs
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (6)
🧰 Additional context used📓 Path-based instructions (1)**/*.rs⚙️ CodeRabbit Configuration File
Files:
🔍 MCP Research (1 server)Deepwiki:
🔇 Additional comments (22)
✨ 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 comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![codescene-delta-analysis[bot]](https://avatars.githubusercontent.com/in/53921?s=60&v=4){kind=small}
There was a problem hiding this comment.
Gates Failed
(1 file with Code Duplication)
Gates Passed
See analysis details in CodeScene
Reason for failure
| Enforce advisory code health rules | Violations | Code Health Impact | |
|---|---|---|---|
| manifest_steps.rs | 1 advisory rule | 10.00 → 9.39 | Suppress |
Quality Gate Profile: Pay Down Tech Debt
Want more control? Customize Code Health rules or catch issues early with our IDE extension and CLI tool.
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @leynos - I've reviewed your changes - here's some feedback:
- Consider reusing the Jinja environment or caching compiled templates across multiple manifest parses to improve performance when rendering large manifests.
- Add a unit test for undefined variables inside a Jinja conditional or loop to verify that strict undefined behavior surfaces errors as expected.
- Extract the duplicated YAML+Jinja string construction in the unit tests into a shared helper to reduce boilerplate and improve maintainability.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Consider reusing the Jinja environment or caching compiled templates across multiple manifest parses to improve performance when rendering large manifests.
- Add a unit test for undefined variables inside a Jinja conditional or loop to verify that strict undefined behavior surfaces errors as expected.
- Extract the duplicated YAML+Jinja string construction in the unit tests into a shared helper to reduce boilerplate and improve maintainability.
## Individual Comments
### Comment 1
<location> `src/manifest.rs:51` </location>
<code_context>
- // mistakes early.
- for (key, value) in vars {
- env.add_global(key, Value::from(value));
+ if let Some(vars) = doc.get("vars").and_then(|v| v.as_mapping()) {
+ // Copy each key-value pair into the environment, preserving native YAML
+ // types so control structures like `{% if %}` and `{% for %}` can operate
+ // on booleans and sequences.
+ for (k, v) in vars {
+ if let Some(key) = k.as_str() {
+ env.add_global(key, Value::from_serialize(v.clone()));
+ }
</code_context>
<issue_to_address>
Consider handling non-string keys in 'vars' mapping explicitly.
If non-string keys are not supported, consider raising an error or warning when encountered to prevent silent failures.
Suggested implementation:
```rust
if let Some(vars) = doc.get("vars").and_then(|v| v.as_mapping()) {
// Copy each key-value pair into the environment, preserving native YAML
// types so control structures like `{% if %}` and `{% for %}` can operate
// on booleans and sequences.
for (k, v) in vars {
if let Some(key) = k.as_str() {
env.add_global(key, Value::from_serialize(v.clone()));
} else {
// Log a warning about non-string keys in 'vars'
log::warn!("Non-string key in 'vars' mapping: {:?}", k);
}
}
}
```
- Ensure that the `log` crate is included and initialized in your project if you want to use `log::warn!`. If you prefer to raise an error instead of logging, you may need to refactor the function to return a `Result` and propagate the error accordingly.
- If you want stricter behavior (e.g., aborting on non-string keys), replace `log::warn!` with an error return.
</issue_to_address>
### Comment 2
<location> `tests/manifest_jinja_tests.rs:104` </location>
<code_context>
+}
+
+#[rstest]
+fn for_loop_non_iterable_errors() {
+ let yaml = r#"
+netsuke_version: 1.0.0
+vars:
+ items: 1
+targets:
+{% for item in items %}
+ - name: "{{ item }}"
+ command: "echo {{ item }}"
+{% endfor %}
+"#;
+
+ assert!(manifest::from_str(yaml).is_err());
+}
</code_context>
<issue_to_address>
Test for error on non-iterable in for-loop is good, but could check error details.
Consider asserting that the error message includes 'not iterable' to verify that the feedback is clear and informative.
</issue_to_address>
<suggested_fix>
<<<<<<< SEARCH
#[rstest]
fn for_loop_non_iterable_errors() {
let yaml = r#"
netsuke_version: 1.0.0
vars:
items: 1
targets:
{% for item in items %}
- name: "{{ item }}"
command: "echo {{ item }}"
{% endfor %}
"#;
assert!(manifest::from_str(yaml).is_err());
}
=======
#[rstest]
fn for_loop_non_iterable_errors() {
let yaml = r#"
netsuke_version: 1.0.0
vars:
items: 1
targets:
{% for item in items %}
- name: "{{ item }}"
command: "echo {{ item }}"
{% endfor %}
"#;
let err = manifest::from_str(yaml).unwrap_err();
let err_msg = err.to_string();
assert!(
err_msg.contains("not iterable"),
"Expected error message to mention 'not iterable', got: {err_msg}"
);
}
>>>>>>> REPLACE
</suggested_fix>
### Comment 3
<location> `tests/steps/manifest_steps.rs:126` </location>
<code_context>
+ assert_eq!(manifest.targets.len(), count);
+}
+
+#[then(expr = "the target {int} name is {string}")]
+fn target_name_n(world: &mut CliWorld, index: usize, name: String) {
+ let manifest = world.manifest.as_ref().expect("manifest");
+ let target = manifest
</code_context>
<issue_to_address>
Consider adding a step for checking target command with variables.
Adding a step to check a target's command by name would improve test coverage, especially for manifests where target order may vary.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (10)
docs/netsuke-design.md(5 hunks)docs/roadmap.md(1 hunks)src/ast.rs(2 hunks)src/manifest.rs(2 hunks)tests/data/jinja_for.yml(1 hunks)tests/data/jinja_for_invalid.yml(1 hunks)tests/data/jinja_if.yml(1 hunks)tests/features/manifest.feature(1 hunks)tests/manifest_jinja_tests.rs(1 hunks)tests/steps/manifest_steps.rs(1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
docs/**/*.md
📄 CodeRabbit Inference Engine (AGENTS.md)
docs/**/*.md: Use the markdown files within thedocs/directory as a knowledge base and source of truth for project requirements, dependency choices, and architectural decisions.
Proactively update the relevant file(s) in thedocs/directory to reflect the latest state when new decisions are made, requirements change, libraries are added/removed, or architectural patterns evolve.
Files:
docs/roadmap.mddocs/netsuke-design.md
**/*.md
📄 CodeRabbit Inference Engine (AGENTS.md)
**/*.md: Documentation must use en-GB-oxendict spelling and grammar, except for the naming of the "LICENSE" file.
Validate Markdown files usingmake markdownlint.
Runmake fmtafter any documentation changes to format all Markdown files and fix table markup.
Validate Mermaid diagrams in Markdown files by runningmake nixie.
Markdown paragraphs and bullet points must be wrapped at 80 columns.
Code blocks in Markdown must be wrapped at 120 columns.
Tables and headings in Markdown must not be wrapped.
Use dashes (-) for list bullets in Markdown.
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
**/*.rs
📄 CodeRabbit Inference Engine (AGENTS.md)
**/*.rs: Clippy warnings MUST be disallowed.
Fix any warnings emitted during tests in the code itself rather than silencing them.
Where a function is too long, extract meaningfully named helper functions adhering to separation of concerns and CQRS.
Where a function has too many parameters, group related parameters in meaningfully named structs.
Where a function is returning a large error consider usingArcto reduce the amount of data returned.
Write unit and behavioural tests for new functionality. Run both before and after making any change.
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.
Prefer immutable data and avoid unnecessarymutbindings.
Handle errors with theResulttype instead of panicking where feasible.
Avoidunsafecode unless absolutely necessary and document any usage clearly.
Place function attributes after doc comments.
Do not usereturnin single-line functions.
Use predicate functions for conditional criteria with more than two branches.
Lints must not be silenced except as a last resort.
Lint rule suppressions must be tightly scoped and include a clear reason.
Preferexpectoverallow.
Prefer.expect()over.unwrap().
Useconcat!()to combine long string literals rather than escaping newlines with a backslash.
Prefer semantic error enums: Derivestd::error::Error(via thethiserrorcrate) for any condition the caller might inspect, retry, or map to an HTTP status.
Use an opaque error only at the app boundary: Useeyre::Reportfor human-readable logs; these should not be exposed in public APIs.
Never export the opaque type from a library: Convert to domain enums at API boundaries, and toeyreonly in the mainmain()entrypoint or top-level async task.
Files:
src/manifest.rssrc/ast.rstests/manifest_jinja_tests.rstests/steps/manifest_steps.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.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.rssrc/ast.rstests/manifest_jinja_tests.rstests/steps/manifest_steps.rs
🧬 Code Graph Analysis (2)
src/ast.rs (1)
src/manifest.rs (1)
serde_yml(68-68)
tests/manifest_jinja_tests.rs (1)
src/manifest.rs (1)
from_str(38-69)
🪛 YAMLlint (1.37.1)
tests/data/jinja_for.yml
[error] 7-7: syntax error: found character '%' that cannot start any token
(syntax)
tests/data/jinja_for_invalid.yml
[error] 5-5: syntax error: found character '%' that cannot start any token
(syntax)
🔍 MCP Research (2 servers)
Deepwiki:
-
The architectural design enforces a two-pass pipeline: first, the raw Netsukefile YAML with embedded Jinja templates is rendered by
minijinja::Environmentusing thevarscontext; second, the rendered pure YAML string is parsed into theNetsukeManifestAST usingserde_yml. This separation ensures that dynamic logic is resolved before static parsing and validation. (docs/netsuke-design.md) -
The
varsfields are used to seed the Jinja templating context globally (NetsukeManifest::vars) and locally per target (Target::vars), with target-scoped variables overriding global ones within their scope. This design supports reusable and declarative build configurations. (docs/netsuke-design.md) -
The manifest parsing (
src/manifest.rs) was updated to remove explicit string-only filtering ofvarsand instead directly insert each key-value pair from the YAML into the Jinja environment globals usingValue::from_serialize. This preserves the richer types through the template rendering stage. (src/manifest.rs) -
New test data files (
jinja_if.yml,jinja_for.yml,jinja_for_invalid.yml) and corresponding feature tests and Rust unit tests were added to verify correct rendering of Jinja conditionals and loops, as well as error handling when loops iterate over non-iterable values. This validates the new capability enabled by thevarstype change. (tests/data/jinja_if.yml, tests/data/jinja_for.yml, tests/data/jinja_for_invalid.yml, tests/features/manifest.feature, tests/manifest_jinja_tests.rs)
Context7:
-
MiniJinja supports Jinja control flow constructs such as
{% if %},{% elif %},{% else %},{% for %}, and loop controls like{% break %}and{% continue %}within loops. These constructs enable conditional rendering and iteration control in templates, which is essential for dynamic manifest generation. (Source: /mitsuhiko/minijinja, https://github.com/mitsuhiko/minijinja/blob/main/minijinja-cli/src/syntax_help.txt#_snippet_36) -
MiniJinja's
Valuetype can represent various literal types including booleans, null, integers, floats, lists, dictionaries, and strings. This flexibility allows template variables to hold rich YAML-native types, supporting complex data structures in manifests. TheValuetype supports iteration and can be used directly as context for rendering templates. (Source: /mitsuhiko/minijinja, https://github.com/mitsuhiko/minijinja/blob/main/minijinja/tests/lexer-inputs/literals.txt#_snippet_0) -
The
serde_yaml::Valuetype represents YAML data in Rust and can hold strings, numbers, booleans, sequences (lists), and mappings (dictionaries). Usingserde_yaml::Valuefor thevarsfield preserves native YAML types, which is necessary for Jinja templating control flow constructs like{% if %}and{% for %}to function correctly in the manifest processing. (Source: /serde-rs/serde_yaml, official crate documentation)
⏰ 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/data/jinja_for.yml (1)
7-10: LGTM: correct loop-driven target generation.The loop renders a valid targets list post-template with stable ordering; quoting around interpolations avoids YAML parsing issues after render.
tests/data/jinja_for_invalid.yml (1)
1-8: LGTM: negative test data correctly triggers non-iterable error.This template will fail at render time as intended, validating error handling for
{% for %}over non-iterables.docs/roadmap.md (1)
91-93: Marking Jinja control structures as complete is correct and aligned with the two-pass design.The checklist accurately reflects implemented
{% if %}and{% for %}support and complies with the documented two-pass rendering pipeline.tests/data/jinja_if.yml (1)
5-6: LGTM: inline conditional renders to a clean scalar command.Quoting ensures a plain string value post-render; this remains valid YAML before and after templating.
tests/features/manifest.feature (1)
71-74: Assert a stable error message fragment for non-iterables.Fail with a clear, actionable error. Match on a stable substring (e.g., "not iterable") from MiniJinja rather than a full string to reduce flakiness.
Augment step definitions to capture and assert the error message. Verify presence with a partial match:
- Expected: error message contains "iterable" or "cannot iterate".
If step definitions lack this capability, add a step such as:
Then the error message contains "not iterable"src/ast.rs (1)
62-65: Adopt typed YAML vars to enable Jinja control flow (LGTM).Preserve native YAML types for vars. This aligns with the two-pass render design and tests for
{% if %}/{% for %}.Also applies to: 190-191
src/manifest.rs (2)
10-10: No review needed for import change.
51-59: Preserve native YAML types in Jinja globals (LGTM).Insert vars with
Value::from_serializeto unlock control flow. This complies with the documented two-pass pipeline.tests/manifest_jinja_tests.rs (1)
103-117: Validate error on non-iterable loop (LGTM).Assert failure when iterating over a scalar. Matches strict undefined behaviour on second pass.
tests/steps/manifest_steps.rs (2)
120-124: Assert target count (LGTM).Keep this focused step; it complements scenario assertions well.
139-151: Assert target command by index (LGTM).The recipe pattern match is correct and the panic path is explicit.
docs/netsuke-design.md (3)
164-167: Document typedvarsenabling control flow (LGTM).Align documentation with implementation: values may be booleans, numbers, and sequences and drive Jinja flow.
416-418: Keep docs and code snippets in sync (LGTM).Reflect
HashMap<String, serde_yml::Value>in both manifest and target structs.Also applies to: 468-470
588-596: Clarify design decisions on typed vars (LGTM).State preservation of YAML types for control flow and actions deserialisation rules.
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (7)
tests/data/jinja_for.yml (1)
7-10: Silence YAMLlint for Jinja-templated fixtures; keep Jinja-first.Exclude these files from YAMLlint instead of altering the template to be YAML-valid pre-render. Add a repo-level .yamllint ignore:
# .yamllint extends: default ignore: | tests/data/jinja_for*.ymltests/data/jinja_for_invalid.yml (1)
5-8: Exclude negative Jinja fixture from YAMLlint; keep scalar to test error path.Retain items: 1 to exercise the "not iterable" failure. Silence YAMLlint for this path:
# .yamllint extends: default ignore: | tests/data/jinja_for_invalid.ymltests/manifest_jinja_tests.rs (2)
39-61: Fix invalid Jinja control block delimiters in renders_if_blocks.Use {% ... %} for control blocks, not {{% ... %}}.
- " command: {{% if flag %}}echo on{{% else %}}echo off{{% endif %}}\n" + " command: {% if flag %}echo on{% else %}echo off{% endif %}\n"
92-99: Assert error details for non-iterable loop for clearer diagnostics.Verify the error mentions "iterable" to ensure user-facing feedback is actionable.
- assert!(manifest::from_str(&yaml).is_err()); + let err = manifest::from_str(&yaml).unwrap_err(); + let msg = err.to_string(); + assert!( + msg.contains("iterable"), + "Expected error message to mention 'iterable', got: {msg}" + );docs/netsuke-design.md (1)
668-674: Correct the MiniJinja API name to Value::from_serialize.Use the exact method name to avoid confusion.
-The parser copies `vars` values into the environment using -`Value::from_serializable`. This preserves native YAML types so Jinja's +The parser copies `vars` values into the environment using +`Value::from_serialize`. This preserves native YAML types so Jinja'stests/steps/manifest_steps.rs (2)
124-131: Replace #[allow] with a narrowly scoped #[expect] per lint policy.Apply the same change here.
-#[allow( +#[expect( clippy::needless_pass_by_value, reason = "Cucumber step requires owned String" -)] +)]
78-85: Replace #[allow] with a narrowly scoped #[expect] per lint policy.Follow the linting guideline to avoid blanket allows.
-#[allow( +#[expect( clippy::needless_pass_by_value, reason = "Cucumber step requires owned String" -)] +)]
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (8)
docs/netsuke-design.md(5 hunks)src/ast.rs(3 hunks)src/manifest.rs(2 hunks)tests/data/jinja_for.yml(1 hunks)tests/data/jinja_for_invalid.yml(1 hunks)tests/features/manifest.feature(1 hunks)tests/manifest_jinja_tests.rs(2 hunks)tests/steps/manifest_steps.rs(4 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.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:
src/ast.rstests/manifest_jinja_tests.rstests/steps/manifest_steps.rssrc/manifest.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/ast.rstests/manifest_jinja_tests.rstests/steps/manifest_steps.rssrc/manifest.rs
**/*.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/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
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/netsuke-design.md
🧬 Code Graph Analysis (3)
src/ast.rs (1)
src/manifest.rs (1)
serde_yml(69-69)
tests/manifest_jinja_tests.rs (1)
src/manifest.rs (1)
from_str(38-70)
tests/steps/manifest_steps.rs (1)
tests/ast_tests.rs (1)
parse_manifest(8-10)
🪛 YAMLlint (1.37.1)
tests/data/jinja_for_invalid.yml
[error] 9-9: too many blank lines (1 > 0)
(empty-lines)
[error] 5-5: syntax error: found character '%' that cannot start any token
(syntax)
tests/data/jinja_for.yml
[error] 11-11: too many blank lines (1 > 0)
(empty-lines)
[error] 7-7: syntax error: found character '%' that cannot start any token
(syntax)
🔍 MCP Research (2 servers)
Deepwiki:
- Netsuke's manifest processing includes a "Stage 2: Jinja Evaluation" where the raw template is processed with a Jinja engine to evaluate expressions and control structures (if/for), producing a static YAML string for subsequent deserialization. (https://deepwiki.com/search/support-jinja-control-flow-in_5dd004f3-fcef-4ee4-81bf-ee16116c2449)
- Implementation uses the minijinja crate; the minijinja::Environment is configured with custom functions (examples: glob(), env()). (https://deepwiki.com/search/support-jinja-control-flow-in_5dd004f3-fcef-4ee4-81bf-ee16116c2449)
- The processing pipeline is: read Netsukefile as raw template → minijinja evaluates templates/control flow → produce pure YAML string → pass to serde_yaml for deserialization into Rust types. (https://deepwiki.com/search/support-jinja-control-flow-in_5dd004f3-fcef-4ee4-81bf-ee16116c2449)
Context7:
- Minijinja exposes Value::from and can render templates from Value-based contexts (https://github.com/mitsuhiko/minijinja/blob/main/README.md)
- Minijinja’s Object API accepts serde-backed Value instances; examples implement Object/get_value returning Value and use Value::from (https://github.com/mitsuhiko/minijinja/blob/main/UPDATING.md#_snippet_5)
- Minijinja examples demonstrate deserializing/rendering with serde-supported types (https://github.com/mitsuhiko/minijinja/blob/main/examples/deserialize/README.md#_snippet_0)
- Minijinja provides detailed template error reporting that includes referenced variables and their values in error output (https://github.com/mitsuhiko/minijinja/blob/main/examples/value-tracking/README.md#_snippet_0)
🔇 Additional comments (7)
src/ast.rs (1)
62-65: Adopt YAML-native values for vars (LGTM).Preserve native YAML types in vars to unlock control flow in templates. This aligns with the documented pipeline and keeps deserialisation strict.
tests/features/manifest.feature (1)
65-73: Strengthen loop assertions (LGTM).Assert target count, order, names, and commands. This tightens the contract of the loop rendering.
tests/manifest_jinja_tests.rs (1)
6-8: Helper for test YAML prelude (LGTM).Keep manifest_yaml to reduce duplication and keep tests focused.
src/manifest.rs (2)
51-60: Seed Jinja with YAML-native vars and fail fast on invalid keys (LGTM).Preserve booleans/sequences via Value::from_serialize and bail on non-string keys. This aligns with the documented two-pass pipeline and strict undefined behaviour.
73-76: Keep contextual render errors (LGTM).Maintain pass-labelled context in render errors for friendlier diagnostics.
docs/netsuke-design.md (1)
164-167: Document YAML-native vars (LGTM).Keep keys as strings and values as YAML-native types to drive Jinja control flow deterministically.
tests/steps/manifest_steps.rs (1)
133-137: Add indexed step assertions (LGTM).Assert target count, names, and commands by index with clear failure messages. This reduces duplication and improves diagnostics.
Also applies to: 139-154, 156-172
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
1 participant
Summary
varsso Jinja{% if %}and{% for %}blocks can drive manifest generationTesting
make lintmake testmake markdownlintmake nixie(fails: libatk-1.0.so.0: cannot open shared object file)https://chatgpt.com/codex/tasks/task_e_68968181f9508322907fa84a09dcddf3
Summary by Sourcery
Enable Jinja control flow in manifest generation by preserving native YAML types for
varsand integrating{% if %}and{% for %}blocks into renderingNew Features:
{% if %}and{% for %}control structures in manifestsEnhancements:
varsfields to useserde_yaml::Valueso booleans and sequences are preservedDocumentation:
varstype and Jinja control flow behaviorTests: