Conversation
Register user-defined Jinja macros from the manifest before rendering so they are callable during template evaluation. Add schema support for the macros list, cover parsing with rstest cases, and extend cucumber scenarios for happy and error paths. Document the loader behaviour and mark the roadmap item complete.
Reviewer's GuideAdds support for manifest-defined Jinja macros by extending the AST schema, providing parsing and registration helpers, integrating macro compilation into the manifest load flow, and updating tests, documentation, and dependencies accordingly. Sequence diagram for manifest macro registration during manifest parsingsequenceDiagram
participant ManifestLoader
participant ManifestName
participant Environment
participant MacroDefinition
participant ManifestYAML
ManifestLoader->ManifestYAML: Parse YAML manifest
ManifestLoader->Environment: Create Jinja environment
ManifestLoader->ManifestYAML: Extract macros list
loop For each MacroDefinition
ManifestLoader->Environment: register_macro(macro_def, index)
Environment->Environment: Compile macro template
Environment->Environment: Register macro function
end
ManifestLoader->Environment: register_manifest_macros(doc, env)
ManifestLoader->ManifestYAML: Expand foreach and render manifest
ER diagram for manifest macros in NetsukeManifesterDiagram
NETSUKE_MANIFEST {
netsuke_version string
vars object
macros MacroDefinition[]
rules Rule[]
actions Action[]
targets Target[]
}
MACRO_DEFINITION {
signature string
body string
}
NETSUKE_MANIFEST ||--o{ MACRO_DEFINITION : contains
Class diagram for MacroDefinition and updated NetsukeManifestclassDiagram
class MacroDefinition {
+String signature
+String body
}
class NetsukeManifest {
+Version netsuke_version
+Vars vars
+Vec~MacroDefinition~ macros
+Vec~Rule~ rules
+Vec~Action~ actions
+Vec~Target~ targets
}
NetsukeManifest "1" o-- "*" MacroDefinition: macros
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 CodeRabbitRelease Notes
WalkthroughAdd MacroDefinition and a top‑level Changes
*Files in manifest rendering area updated to integrate macros and registration flow. Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant Loader as ManifestLoader
participant YAML as YAML Parser
participant Registry as MacroRegistry
participant Jinja as MiniJinja
User->>Loader: from_str_named(yaml, name)
Loader->>YAML: Parse manifest (includes macros)
YAML-->>Loader: NetsukeManifest{macros,...}
rect rgba(230,245,255,0.6)
note right of Loader: Register manifest macros
loop each MacroDefinition
Loader->>Loader: parse_macro_name(signature)
Loader->>Jinja: compile body -> template
Loader->>Registry: register wrapper(fn)
note right of Registry: wrapper builds fresh State per call
end
end
Loader->>Loader: continue manifest expansion
Loader->>Registry: invoke macro wrapper during render
Registry->>Jinja: evaluate with fresh State (args/kwargs)
Jinja-->>Loader: rendered string
alt Invalid macro
Loader-->>User: Error (signature / parameter list)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: ASSERTIVE Plan: Pro 📒 Files selected for processing (3)
🧰 Additional context used📓 Path-based instructions (6)docs/**/*.{rs,md}📄 CodeRabbit inference engine (docs/rust-doctest-dry-guide.md)
Files:
**/*.{rs,md}📄 CodeRabbit inference engine (AGENTS.md)
Files:
docs/**/*.md📄 CodeRabbit inference engine (AGENTS.md)
Files:
**/*.md📄 CodeRabbit inference engine (AGENTS.md)
Files:
⚙️ CodeRabbit configuration file
Files:
**/*.rs📄 CodeRabbit inference engine (AGENTS.md)
Files:
⚙️ CodeRabbit configuration file
Files:
**/*{test,tests}.rs📄 CodeRabbit inference engine (AGENTS.md)
Files:
🧬 Code graph analysis (2)src/manifest/tests.rs (1)
src/manifest.rs (2)
⏰ 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). (7)
🔇 Additional comments (7)
Comment |
![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}
There was a problem hiding this comment.
Hey there - I've reviewed your changes and found some issues that need to be addressed.
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location> `tests/manifest_jinja_tests.rs:184` </location>
<code_context>
+#[rstest]
+fn registers_manifest_macros() {
</code_context>
<issue_to_address>
**suggestion (testing):** Consider adding tests for macro argument edge cases.
Tests for macros with no arguments, default arguments, and complex argument types are missing. Including these will improve coverage and robustness.
</issue_to_address>
### Comment 2
<location> `src/manifest.rs:108` </location>
<code_context>
+ Ok(())
+}
+
+fn register_manifest_macros(doc: &YamlValue, env: &mut Environment) -> Result<()> {
+ let Some(items) = doc.get("macros") else {
+ return Ok(());
</code_context>
<issue_to_address>
**issue (complexity):** Consider using Serde to deserialize macro definitions and refactoring macro registration helpers for clearer, more concise code.
Here’s one way to collapse most of that YAML-walking boilerplate into Serde, and clean up the macro‐registration builder/closure nesting. You’ll end up with:
1) A small `MacroDef` you can `derive(Deserialize)` on instead of manually calling `as_mapping`/`get`/`as_str`:
```rust
// add at top
#[derive(serde::Deserialize)]
struct MacroDef {
signature: String,
body: String,
}
```
2) Replace `register_manifest_macros` completely with a simple `from_value` + loop:
```rust
fn register_manifest_macros(doc: &YamlValue, env: &mut Environment) -> Result<()> {
// if there’s no “macros” key just return
let macros_val = match doc.get("macros") {
Some(v) => v.clone(),
None => return Ok(()),
};
// let Serde enforce that it’s a sequence of { signature, body }
let defs: Vec<MacroDef> = serde_yml::from_value(macros_val)
.context("macros must be a sequence of mappings with string signature/body")?;
for (idx, MacroDef{ signature, body }) in defs.into_iter().enumerate() {
register_macro(env, &signature, &body, idx)
.with_context(|| format!("register macro '{}'", signature))?;
}
Ok(())
}
```
3) Factor out your closure into a little helper so `register_macro` stays linear:
```rust
fn register_macro(
env: &mut Environment,
signature: &str,
body: &str,
idx: usize,
) -> Result<()> {
let name = parse_macro_name(signature)?;
let template_name = format!("__manifest_macro_{}_{}", idx, name);
let source = format!("{{% macro {} %}}{}{{% endmacro %}}", signature, body);
env.add_template_owned(template_name.clone(), source)
.context(format!("compile macro '{}'", name))?;
env.add_function(name.clone(), make_macro_fn(template_name, name));
Ok(())
}
fn make_macro_fn(
template_name: String,
macro_name: String,
) -> impl Fn(&State, Rest<Value>) -> Result<Value, Error> {
move |state, Rest(args)| {
let tpl = state.env().get_template(&template_name)?;
let st = tpl.eval_to_state(())?;
let val = st.lookup(¯o_name)
.ok_or_else(|| {
Error::new(
ErrorKind::InvalidOperation,
format!("macro '{}' not defined in '{}'", macro_name, template_name),
)
})?;
val.call(&st, &args)
}
}
```
With this:
- Serde handles all the YAML shape‐checking
- `register_manifest_macros` is only 10 lines
- `register_macro` builds its strings in one place
- the closure itself is factored out for clarity and zero extra clones beyond what you already had.
</issue_to_address>
### Comment 3
<location> `src/manifest.rs:63` </location>
<code_context>
}
}
+fn parse_macro_name(signature: &str) -> Result<String> {
+ let trimmed = signature.trim();
+ let Some((name, _rest)) = trimmed.split_once('(') else {
</code_context>
<issue_to_address>
**issue (review_instructions):** Add behavioural and unit tests for macro parsing and registration logic.
The new macro parsing and registration logic in `parse_macro_name`, `register_macro`, and `register_manifest_macros` must be covered by both behavioural and unit tests to ensure correctness and prevent regressions. Please add comprehensive tests for these functions.
<details>
<summary>Review instructions:</summary>
**Path patterns:** `**/*`
**Instructions:**
For any new feature or change to an existing feature, both behavioural *and* unit tests are required.
</details>
</issue_to_address>
### Comment 4
<location> `src/ast.rs:59` </location>
<code_context>
/// assert_eq!(manifest.targets.len(), 1);
/// # Ok(()) }
/// ```
+#[derive(Debug, Clone, Deserialize, Serialize)]
+#[serde(deny_unknown_fields)]
+pub struct MacroDefinition {
</code_context>
<issue_to_address>
**issue (review_instructions):** Add tests for MacroDefinition struct usage and serialization.
The new `MacroDefinition` struct should be exercised by both behavioural and unit tests, including its serialization and deserialization. Please ensure these cases are covered.
<details>
<summary>Review instructions:</summary>
**Path patterns:** `**/*`
**Instructions:**
For any new feature or change to an existing feature, both behavioural *and* unit tests are required.
</details>
</issue_to_address>
### Comment 5
<location> `src/manifest.rs:63` </location>
<code_context>
}
}
+fn parse_macro_name(signature: &str) -> Result<String> {
+ let trimmed = signature.trim();
+ let Some((name, _rest)) = trimmed.split_once('(') else {
</code_context>
<issue_to_address>
**issue (review_instructions):** Missing `//!` module-level comment at the top of the file.
Please add a `//!` comment at the beginning of this module to describe its purpose, as required by the review instructions.
<details>
<summary>Review instructions:</summary>
**Path patterns:** `**/*.rs`
**Instructions:**
Every module **must** begin with a `//!` comment.
</details>
</issue_to_address>
### Comment 6
<location> `src/ast.rs:59` </location>
<code_context>
/// assert_eq!(manifest.targets.len(), 1);
/// # Ok(()) }
/// ```
+#[derive(Debug, Clone, Deserialize, Serialize)]
+#[serde(deny_unknown_fields)]
+pub struct MacroDefinition {
</code_context>
<issue_to_address>
**issue (review_instructions):** Missing `//!` module-level comment at the top of the file.
Please add a `//!` comment at the beginning of this module to describe its purpose, as required by the review instructions.
<details>
<summary>Review instructions:</summary>
**Path patterns:** `**/*.rs`
**Instructions:**
Every module **must** begin with a `//!` comment.
</details>
</issue_to_address>
### Comment 7
<location> `docs/netsuke-design.md:724` </location>
<code_context>
built-in definition. This mirrors Jinja's behaviour and follows `minijinja`
semantics where later definitions shadow earlier ones.
+The manifest loader compiles each macro definition into an internal template
+and registers a wrapper function that evaluates the macro on demand. The
+wrapper constructs a fresh MiniJinja state for every invocation so macro calls
</code_context>
<issue_to_address>
**suggestion (review_instructions):** This paragraph exceeds the 80 column wrapping limit for prose.
Please wrap this paragraph so that no line exceeds 80 columns, as per documentation style guidelines.
<details>
<summary>Review instructions:</summary>
**Path patterns:** `**/*.md`
**Instructions:**
Paragraphs and bullets must be wrapped to 80 columns
</details>
</issue_to_address>
### Comment 8
<location> `docs/netsuke-design.md:725` </location>
<code_context>
semantics where later definitions shadow earlier ones.
+The manifest loader compiles each macro definition into an internal template
+and registers a wrapper function that evaluates the macro on demand. The
+wrapper constructs a fresh MiniJinja state for every invocation so macro calls
+do not depend on the lifetime of the manifest parsing state. This preserves
</code_context>
<issue_to_address>
**suggestion (review_instructions):** This line is part of a paragraph that exceeds the 80 column wrapping limit.
Ensure all lines in this paragraph are wrapped to 80 columns for consistency.
<details>
<summary>Review instructions:</summary>
**Path patterns:** `**/*.md`
**Instructions:**
Paragraphs and bullets must be wrapped to 80 columns
</details>
</issue_to_address>
### Comment 9
<location> `docs/netsuke-design.md:726` </location>
<code_context>
+The manifest loader compiles each macro definition into an internal template
+and registers a wrapper function that evaluates the macro on demand. The
+wrapper constructs a fresh MiniJinja state for every invocation so macro calls
+do not depend on the lifetime of the manifest parsing state. This preserves
+MiniJinja's argument handling, including keyword parameters and `caller`
</code_context>
<issue_to_address>
**suggestion (review_instructions):** This line is part of a paragraph that exceeds the 80 column wrapping limit.
Please wrap this paragraph to 80 columns as required by the documentation standards.
<details>
<summary>Review instructions:</summary>
**Path patterns:** `**/*.md`
**Instructions:**
Paragraphs and bullets must be wrapped to 80 columns
</details>
</issue_to_address>
### Comment 10
<location> `docs/netsuke-design.md:727` </location>
<code_context>
+The manifest loader compiles each macro definition into an internal template
+and registers a wrapper function that evaluates the macro on demand. The
+wrapper constructs a fresh MiniJinja state for every invocation so macro calls
+do not depend on the lifetime of the manifest parsing state. This preserves
+MiniJinja's argument handling, including keyword parameters and `caller`
+support, while allowing later macros to override earlier ones.
</code_context>
<issue_to_address>
**suggestion (review_instructions):** This line is part of a paragraph that exceeds the 80 column wrapping limit.
Wrap this paragraph to 80 columns for improved readability and compliance.
<details>
<summary>Review instructions:</summary>
**Path patterns:** `**/*.md`
**Instructions:**
Paragraphs and bullets must be wrapped to 80 columns
</details>
</issue_to_address>
### Comment 11
<location> `docs/netsuke-design.md:728` </location>
<code_context>
+and registers a wrapper function that evaluates the macro on demand. The
+wrapper constructs a fresh MiniJinja state for every invocation so macro calls
+do not depend on the lifetime of the manifest parsing state. This preserves
+MiniJinja's argument handling, including keyword parameters and `caller`
+support, while allowing later macros to override earlier ones.
+
</code_context>
<issue_to_address>
**suggestion (review_instructions):** This line is part of a paragraph that exceeds the 80 column wrapping limit.
Please ensure this paragraph is wrapped to 80 columns as per the documentation guidelines.
<details>
<summary>Review instructions:</summary>
**Path patterns:** `**/*.md`
**Instructions:**
Paragraphs and bullets must be wrapped to 80 columns
</details>
</issue_to_address>
### Comment 12
<location> `docs/netsuke-design.md:729` </location>
<code_context>
+wrapper constructs a fresh MiniJinja state for every invocation so macro calls
+do not depend on the lifetime of the manifest parsing state. This preserves
+MiniJinja's argument handling, including keyword parameters and `caller`
+support, while allowing later macros to override earlier ones.
+
### 4.4 Essential Custom Functions
</code_context>
<issue_to_address>
**suggestion (review_instructions):** This line is part of a paragraph that exceeds the 80 column wrapping limit.
Wrap this paragraph to 80 columns to maintain consistency with documentation standards.
<details>
<summary>Review instructions:</summary>
**Path patterns:** `**/*.md`
**Instructions:**
Paragraphs and bullets must be wrapped to 80 columns
</details>
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
|
@coderabbitai Please suggest a fix for this issue and supply a prompt for an AI coding agent to enable it to apply the fix: Comment on file use crate::ast::NetsukeManifest;
use anyhow::{Context, Result};
use minijinja::{Environment, Error, ErrorKind, UndefinedBehavior, value::Value};
use minijinja::{❌ New issue: String Heavy Function Arguments |
|
Preserve keyword arguments when invoking manifest macros Manifest macros are registered by wrapping each macro with |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 7198 @leynos, I've analysed the string-heavy arguments issue in Suggested FixRefactor Current signature: fn register_macro(env: &mut Environment, signature: &str, body: &str, index: usize) -> Result<()>Refactored signature: fn register_macro(env: &mut Environment, macro_def: &MacroDefinition, index: usize) -> Result<()>Changes required:
This refactoring will:
Optional EnhancementFor further reduction, consider introducing a newtype wrapper for manifest names: pub struct ManifestName(String);
impl ManifestName {
pub fn new(name: impl Into<String>) -> Self {
Self(name.into())
}
pub fn as_str(&self) -> &str {
&self.0
}
}Then update AI Coding Agent Prompt |
Refactor macro loading to use serde, cover edge cases with unit and behavioural tests, and extend fixtures for argument handling.
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
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (7)
src/manifest.rs(4 hunks)tests/ast_tests.rs(1 hunks)tests/data/jinja_macro_arguments.yml(1 hunks)tests/data/jinja_macro_missing_parens.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 (2)
**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
**/*.rs: Keep functions small with single responsibility; apply command-query separation
Function documentation must include clear examples; test documentation should omit redundant examples
No single Rust code file may exceed 400 lines; split long switches/dispatch tables; move large test data to external files
Disallow Clippy warnings; code must be clean under clippy
Fix warnings emitted during tests in code, do not silence them
Extract helper functions when a function is too long; maintain separation of concerns and CQRS
Group many related parameters into well-named structs
Consider using Arc for large error values to reduce data returned
Every module must begin with a module-level //! comment describing purpose and utility
Document public APIs with Rustdoc /// comments so cargo doc can generate docs
Prefer immutable data; avoid unnecessary mut bindings
Use Result for error handling instead of panicking where feasible
Avoid unsafe code unless absolutely necessary; document any usage clearly
Place function attributes after doc comments
Do not use return in single-line functions
Use predicate functions for conditionals with more than two branches
Do not silence lints except as a last resort
When suppressing lints, scope narrowly and include a clear reason
Prefer #[expect(...)] over #[allow(...)] for lint handling
When functions are unused for certain features, gate with #[cfg]/#[cfg_attr]
Prefer .expect() over .unwrap()
Use concat!() for long string literals instead of escaping newlines with backslashes
Prefer single-line function bodies where appropriate (e.g., pub fn new(id: u64) -> Self { Self(id) })
Use cap-std for filesystem operations; avoid std::fs directly
Use camino for path handling; avoid std::path::PathBuf directly
Prefer semantic error enums deriving std::error::Error via thiserror for inspectable conditions
Use opaque eyre::Report only at the app boundary; do not expose in public APIs
Never export opaque error types from libraries; convert to domain...
Files:
tests/ast_tests.rstests/steps/manifest_steps.rstests/manifest_jinja_tests.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 / -yse / -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.
Environment access (env::set_var and env::remove_var) are always unsafe in Rust 2024 and MUST be marked as such
- For testing of functionality depending upon environment variables, dependency injection and the
mockablecrate are the preferred option.- If mockable cannot be used, env mutations in...
Files:
tests/ast_tests.rstests/steps/manifest_steps.rstests/manifest_jinja_tests.rssrc/manifest.rs
tests/**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
tests/**/*.rs: Write unit and behavioural tests for new functionality; run before and after changes
Use rstest fixtures for shared setup
Replace duplicated tests with #[rstest(...)] parameterised cases
Prefer mockall for mocks/stubs
Mock nondeterministic deps via dependency injection using mockable traits (e.g., Env, Clock); follow docs/reliable-testing-in-rust-via-dependency-injection.md
Files:
tests/ast_tests.rstests/steps/manifest_steps.rstests/manifest_jinja_tests.rs
🧬 Code graph analysis (2)
tests/ast_tests.rs (2)
tests/steps/manifest_steps.rs (1)
parse_manifest(124-126)src/manifest.rs (1)
serde_yml(278-278)
tests/manifest_jinja_tests.rs (1)
src/manifest.rs (1)
from_str(178-180)
🔍 Remote MCP Ref
Summary of additional, review-relevant facts
-
MiniJinja call ABI — Value::call accepts positional and keyword arguments: Value::call forwards Rest (positional) and Kwargs (named) to the called template/function; manifest macro wrapper must therefore accept and forward both to preserve keyword args and caller semantics.
-
MiniJinja environment functions registration — env.add_function can register closures that take (State, Rest, Kwargs) (or at least receive Rest and Kwargs) so wrappers must capture/forward the engine state plus both arg kinds; otherwise named parameters and caller blocks are lost, producing "unexpected keyword argument" errors. This matches the bug reported in the PR comments: current wrapper only accepts Rest and drops kwargs.
Concrete review actionables (minimal)
- Fix wrapper to accept both Rest and Kwargs and call compiled_value.call(state, args, kwargs). This addresses the functional bug reported and preserves caller/keyword behavior.
Tools used
- Ref_ref_search_documentation (Ref) — queries for MiniJinja Rust calling conventions.
- Ref_ref_read_url (Ref) — MiniJinja Value docs (call signature).
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
♻️ Duplicate comments (1)
src/manifest.rs (1)
119-137: Preserve keyword arguments and caller; stop smuggling kwargs into positionalStop converting Kwargs into a positional Value and omitting them from Value::call. This drops named parameters and the implicit caller. Forward both Rest and Kwargs to Value::call.
Apply this diff:
-) -> impl Fn(&State, Rest<Value>, Kwargs) -> Result<Value, Error> { - move |state, Rest(mut args), kwargs| { +) -> impl Fn(&State, Rest<Value>, Kwargs) -> Result<Value, Error> { + move |state, Rest(args), kwargs| { let template = state.env().get_template(&template_name)?; let macro_state = template.eval_to_state(())?; let value = macro_state.lookup(¯o_name).ok_or_else(|| { Error::new( ErrorKind::InvalidOperation, format!("macro '{macro_name}' not defined in template '{template_name}'"), ) })?; - if kwargs.args().next().is_some() { - args.push(Value::from(kwargs)); - } - value.call(¯o_state, &args) + value.call(¯o_state, &args, &kwargs) } }Based on learnings
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
src/manifest.rs(4 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
**/*.rs: Keep functions small with single responsibility; apply command-query separation
Function documentation must include clear examples; test documentation should omit redundant examples
No single Rust code file may exceed 400 lines; split long switches/dispatch tables; move large test data to external files
Disallow Clippy warnings; code must be clean under clippy
Fix warnings emitted during tests in code, do not silence them
Extract helper functions when a function is too long; maintain separation of concerns and CQRS
Group many related parameters into well-named structs
Consider using Arc for large error values to reduce data returned
Every module must begin with a module-level //! comment describing purpose and utility
Document public APIs with Rustdoc /// comments so cargo doc can generate docs
Prefer immutable data; avoid unnecessary mut bindings
Use Result for error handling instead of panicking where feasible
Avoid unsafe code unless absolutely necessary; document any usage clearly
Place function attributes after doc comments
Do not use return in single-line functions
Use predicate functions for conditionals with more than two branches
Do not silence lints except as a last resort
When suppressing lints, scope narrowly and include a clear reason
Prefer #[expect(...)] over #[allow(...)] for lint handling
When functions are unused for certain features, gate with #[cfg]/#[cfg_attr]
Prefer .expect() over .unwrap()
Use concat!() for long string literals instead of escaping newlines with backslashes
Prefer single-line function bodies where appropriate (e.g., pub fn new(id: u64) -> Self { Self(id) })
Use cap-std for filesystem operations; avoid std::fs directly
Use camino for path handling; avoid std::path::PathBuf directly
Prefer semantic error enums deriving std::error::Error via thiserror for inspectable conditions
Use opaque eyre::Report only at the app boundary; do not expose in public APIs
Never export opaque error types from libraries; convert to domain...
Files:
src/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 / -yse / -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.
Environment access (env::set_var and env::remove_var) are always unsafe in Rust 2024 and MUST be marked as such
- For testing of functionality depending upon environment variables, dependency injection and the
mockablecrate are the preferred option.- If mockable cannot be used, env mutations in...
Files:
src/manifest.rs
🧬 Code graph analysis (1)
src/manifest.rs (1)
src/stdlib/network.rs (2)
kwargs(42-42)kwargs(43-43)
🔍 Remote MCP Ref
Summary of relevant findings for review
- MiniJinja Value::call supports forwarding both positional (Rest) and keyword (Kwargs) arguments; manifest macro wrappers must accept and forward both to preserve named-parameter and caller semantics.
Recommended code changes to address the reported functional bug
- Change the macro wrapper registered with env.add_function to accept (State, Rest, Kwargs) and invoke compiled_value.call(state, args, kwargs) so keyword args and caller blocks are preserved.
Tests to verify
- Existing macro tests that exercise defaulted/named parameters and caller behavior should pass after the fix (e.g., tests/data/jinja_macro_arguments.yml and feature scenarios added in tests/features/manifest.feature). (Tests added in the PR already cover argument variants.)
⏰ 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). (7)
- GitHub Check: release / build-linux (aarch64-unknown-linux-gnu, linux-arm64, arm64)
- GitHub Check: release / build-macos (aarch64-apple-darwin, macos-14, macos-arm64, arm64, macos-aarch64)
- GitHub Check: release / build-windows (x86_64-pc-windows-msvc, amd64, x64, windows-x86_64)
- GitHub Check: release / build-macos (x86_64-apple-darwin, macos-13, macos-x86_64, x86_64, macos-x86_64)
- GitHub Check: release / build-linux (x86_64-unknown-linux-gnu, linux-amd64, amd64)
- GitHub Check: release / build-windows (aarch64-pc-windows-msvc, arm64, arm64, windows-aarch64)
- GitHub Check: build-test (ubuntu-latest, stable)
🔇 Additional comments (5)
src/manifest.rs (5)
63-82: Correct, tight identifier extractionParse and validate the macro identifier robustly; inputs like defaults, varargs, and keyword-only params are handled. LGTM.
84-97: Good API surface: pass MacroDefinition by referenceRegister macros from a single cohesive type and add helpful context on failure. This aligns with the “group related parameters” guideline. LGTM.
169-170: Register macros before expansionRegister manifest macros prior to foreach expansion so templates can invoke them. Correct ordering. LGTM.
244-273: Tests exercise positional, defaults, and named argsThese cases validate keyword propagation and will pass once kwargs are forwarded in make_macro_fn. Keep them.
Run make test after applying the kwargs fix.
291-300: Multi-macro integration test looks goodCompiles two macros and verifies composition. LGTM.
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.
|
@coderabbitai Please suggest a fix for this issue and supply a prompt for an AI coding agent to enable it to apply the fix: String Heavy Function Argumentssrc/manifest.rs: What lead to degradation?In this module, 53.8% of all arguments to its 8 functions are strings. The threshold for string arguments is 39.0% Why does this problem occur?String is a generic type that fail to capture the constraints of the domain object it represents. In this module, 54 % of all function arguments are string types. How to fix it?Heavy string usage indicates a missing domain language. Introduce data types that encapsulate the semantics. For example, a user_name is better represented as a constrained User type rather than a pure string, which could be anything. |
This comment was marked as resolved.
This comment was marked as resolved.
|
@coderabbitai Would the newt-hype macro help with this? https://crates.io/crates/newt-hype |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🌐 Web query: 💡 Result: The crate newt-hype is a tiny, no_std-friendly helper for creating newtype (wrapper) structs via macros so you can get strongly typed wrappers with minimal boilerplate. [1][2] Main features
Primary use cases
Sources: docs.rs newt_hype (crate docs) and libraries.io cargo listing. [1][2] @leynos, AnalysisPros of newt-hype:
Cons for this use case:
RecommendationFor now: Use the manual implementation I suggested. It's clear, self-contained, and proportionate to the need. Consider newt-hype later if: You introduce multiple similar domain types (e.g., The string argument ratio will still drop to 38.5% with the manual approach, meeting your code quality threshold. Would you like me to proceed with the manual |
There was a problem hiding this comment.
Actionable comments posted: 4
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
src/manifest.rs(4 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
**/*.rs: Keep functions small with single responsibility; apply command-query separation
Function documentation must include clear examples; test documentation should omit redundant examples
No single Rust code file may exceed 400 lines; split long switches/dispatch tables; move large test data to external files
Disallow Clippy warnings; code must be clean under clippy
Fix warnings emitted during tests in code, do not silence them
Extract helper functions when a function is too long; maintain separation of concerns and CQRS
Group many related parameters into well-named structs
Consider using Arc for large error values to reduce data returned
Every module must begin with a module-level //! comment describing purpose and utility
Document public APIs with Rustdoc /// comments so cargo doc can generate docs
Prefer immutable data; avoid unnecessary mut bindings
Use Result for error handling instead of panicking where feasible
Avoid unsafe code unless absolutely necessary; document any usage clearly
Place function attributes after doc comments
Do not use return in single-line functions
Use predicate functions for conditionals with more than two branches
Do not silence lints except as a last resort
When suppressing lints, scope narrowly and include a clear reason
Prefer #[expect(...)] over #[allow(...)] for lint handling
When functions are unused for certain features, gate with #[cfg]/#[cfg_attr]
Prefer .expect() over .unwrap()
Use concat!() for long string literals instead of escaping newlines with backslashes
Prefer single-line function bodies where appropriate (e.g., pub fn new(id: u64) -> Self { Self(id) })
Use cap-std for filesystem operations; avoid std::fs directly
Use camino for path handling; avoid std::path::PathBuf directly
Prefer semantic error enums deriving std::error::Error via thiserror for inspectable conditions
Use opaque eyre::Report only at the app boundary; do not expose in public APIs
Never export opaque error types from libraries; convert to domain...
Files:
src/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 / -yse / -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.
Environment access (env::set_var and env::remove_var) are always unsafe in Rust 2024 and MUST be marked as such
- For testing of functionality depending upon environment variables, dependency injection and the
mockablecrate are the preferred option.- If mockable cannot be used, env mutations in...
Files:
src/manifest.rs
🔍 Remote MCP Deepwiki
Summary of additional relevant facts for reviewing this PR (sources cited):
- The design/docs state manifest macros are registered into the Jinja environment before any template evaluation, and user-defined macros override built-in functions (macros are registered prior to structural expansion and string rendering).
- The manifest loading pipeline is two-phase: (1) parse YAML → eval structural directives (foreach/when) with a configured minijinja Environment (custom funcs like env() and glob() registered there), then (2) deserialize to AST and perform final string rendering using that same environment; this implies macros must be available during both expansion and later rendering stages.
- The project uses minijinja with strict undefined behavior (errors on undefined variables) and registers custom functions (env, glob, stdlib) into Environment during manifest parsing, so macro registration must occur on the same Environment instance and before expansion to affect foreach/when and rendering.
- Tests and BDD features were added to cover manifest macros, argument variants, missing-signature and missing-parens errors; CI/test guidance in repo: run make lint and make test (the PR already includes unit tests, cucumber scenarios, and YAML fixtures exercising macros) — reviewers should run the test suite after code changes.
- The docs/roadmap were updated to mark “Support user-defined Jinja macros declared in a top-level macros list” as completed; design doc describes macros being compiled and wrapper functions registered into the Environment for on-demand evaluation (so wrappers must preserve keywords/caller semantics).
Tools used:
- Deepwiki: Deepwiki_read_wiki_contents
🔇 Additional comments (5)
src/manifest.rs (5)
63-82: LGTM!The signature validation logic correctly handles empty strings, missing parentheses, and empty identifiers before the parameter list. Error messages are clear and actionable.
84-97: LGTM!The function correctly compiles the macro body into an internal template and registers the wrapper function with the extracted name. Taking
&MacroDefinitionavoids unnecessary cloning.
99-112: LGTM!The function correctly deserialises macros from YAML using Serde, validates the shape, and iterates by reference to avoid unnecessary clones. The error context clearly identifies which macro failed to register.
170-170: LGTM!Registering manifest macros before
expand_foreachensures macros are available during structural expansion, aligning with the documented two-phase manifest loading pipeline.
207-302: LGTM!The test coverage is comprehensive: signature parsing (valid and invalid cases), macro registration with various argument patterns (positional, keyword, defaults), shape validation, and multi-macro scenarios. Using
rstestfixtures and parameterised cases avoids duplication.
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
src/manifest.rs(4 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
**/*.rs: Keep functions small with single responsibility; apply command-query separation
Function documentation must include clear examples; test documentation should omit redundant examples
No single Rust code file may exceed 400 lines; split long switches/dispatch tables; move large test data to external files
Disallow Clippy warnings; code must be clean under clippy
Fix warnings emitted during tests in code, do not silence them
Extract helper functions when a function is too long; maintain separation of concerns and CQRS
Group many related parameters into well-named structs
Consider using Arc for large error values to reduce data returned
Every module must begin with a module-level //! comment describing purpose and utility
Document public APIs with Rustdoc /// comments so cargo doc can generate docs
Prefer immutable data; avoid unnecessary mut bindings
Use Result for error handling instead of panicking where feasible
Avoid unsafe code unless absolutely necessary; document any usage clearly
Place function attributes after doc comments
Do not use return in single-line functions
Use predicate functions for conditionals with more than two branches
Do not silence lints except as a last resort
When suppressing lints, scope narrowly and include a clear reason
Prefer #[expect(...)] over #[allow(...)] for lint handling
When functions are unused for certain features, gate with #[cfg]/#[cfg_attr]
Prefer .expect() over .unwrap()
Use concat!() for long string literals instead of escaping newlines with backslashes
Prefer single-line function bodies where appropriate (e.g., pub fn new(id: u64) -> Self { Self(id) })
Use cap-std for filesystem operations; avoid std::fs directly
Use camino for path handling; avoid std::path::PathBuf directly
Prefer semantic error enums deriving std::error::Error via thiserror for inspectable conditions
Use opaque eyre::Report only at the app boundary; do not expose in public APIs
Never export opaque error types from libraries; convert to domain...
Files:
src/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 / -yse / -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.
Environment access (env::set_var and env::remove_var) are always unsafe in Rust 2024 and MUST be marked as such
- For testing of functionality depending upon environment variables, dependency injection and the
mockablecrate are the preferred option.- If mockable cannot be used, env mutations in...
Files:
src/manifest.rs
⏰ 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). (5)
- GitHub Check: release / build-linux (x86_64-unknown-linux-gnu, linux-amd64, amd64)
- GitHub Check: release / build-macos (x86_64-apple-darwin, macos-13, macos-x86_64, x86_64, macos-x86_64)
- GitHub Check: release / build-windows (aarch64-pc-windows-msvc, arm64, arm64, windows-aarch64)
- GitHub Check: release / build-windows (x86_64-pc-windows-msvc, amd64, x64, windows-x86_64)
- GitHub Check: build-test (ubuntu-latest, stable)
🔇 Additional comments (4)
src/manifest.rs (4)
10-15: String-heavy metric partially mitigated.The static analysis warning about string-heavy function arguments (53.8% vs 39% threshold) has been partially addressed by accepting
&MacroDefinitioninregister_macrorather than separate string parameters. No further refactoring required here.
170-170: LGTM!Macro registration is correctly placed before
expand_foreach, ensuring macros are available during manifest expansion.
207-302: Test coverage is comprehensive.The tests cover:
- Valid and invalid macro signature parsing
- Macro registration with varied argument patterns including keyword arguments (lines 254-258)
- YAML validation errors
- Multiple macro registration and composition
Ensure the test at lines 254-258 (
show('Netsuke', excited=true)) passes with the current kwargs forwarding implementation to confirm keyword arguments work correctly.
114-138: Verify keyword argument forwarding and add documentation. The closure appendsKwargsas aValueto the positional args rather than using the separate kwargs parameter inValue::call. Confirm this matches theminijinja::value::Value::callsignature in v2.11.0 and adjust to pass keyword arguments correctly if needed. Add a Rustdoc comment onmake_macro_fndescribing its purpose, parameters and error conditions.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/manifest.rs (1)
225-241: Incorrect kwargs forwarding breaks macro calls with named arguments.The conditional logic at lines 231-237 wraps
kwargsas aValueand appends it to the positional args, then callsvalue.call(state, call_args.as_slice())without a separate kwargs parameter. This is incorrect. MiniJinja'sValue::callexpects kwargs as a distinct parameter, not embedded in the args slice. The comment claiming MiniJinja encodes kwargs as a trailing value is false. Any macro invoked with keyword arguments (e.g.,{{ show('Netsuke', excited=true) }}) or using the implicitcallerparameter will fail with "unexpected keyword argument" errors.Replace the entire function body with a direct call to
Value::callthat forwards both positional and keyword arguments:fn call_macro_value( value: &Value, state: &State, args: Vec<Value>, kwargs: Kwargs, ) -> Result<Value, Error> { - if kwargs.args().next().is_some() { - let mut call_args = args; - // MiniJinja encodes keyword arguments as a trailing `Kwargs` value on the - // argument slice. Push the wrapper rather than attempting to call with - // a separate parameter so the runtime extracts keywords correctly. - call_args.push(Value::from(kwargs)); - value.call(state, call_args.as_slice()) - } else { - value.call(state, args.as_slice()) - } + value.call(state, args.as_slice(), kwargs) }🤖 Prompt for AI Agents
In src/manifest.rs at lines 225-241, remove the conditional logic that wraps kwargs as a Value and appends to the args vector; instead call value.call(state, args.as_slice(), kwargs) directly so both positional and keyword arguments are forwarded correctly to the macro invocation, preserving named parameters and the implicit caller block.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
src/manifest.rs(6 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
**/*.rs: Keep functions small with single responsibility; apply command-query separation
Function documentation must include clear examples; test documentation should omit redundant examples
No single Rust code file may exceed 400 lines; split long switches/dispatch tables; move large test data to external files
Disallow Clippy warnings; code must be clean under clippy
Fix warnings emitted during tests in code, do not silence them
Extract helper functions when a function is too long; maintain separation of concerns and CQRS
Group many related parameters into well-named structs
Consider using Arc for large error values to reduce data returned
Every module must begin with a module-level //! comment describing purpose and utility
Document public APIs with Rustdoc /// comments so cargo doc can generate docs
Prefer immutable data; avoid unnecessary mut bindings
Use Result for error handling instead of panicking where feasible
Avoid unsafe code unless absolutely necessary; document any usage clearly
Place function attributes after doc comments
Do not use return in single-line functions
Use predicate functions for conditionals with more than two branches
Do not silence lints except as a last resort
When suppressing lints, scope narrowly and include a clear reason
Prefer #[expect(...)] over #[allow(...)] for lint handling
When functions are unused for certain features, gate with #[cfg]/#[cfg_attr]
Prefer .expect() over .unwrap()
Use concat!() for long string literals instead of escaping newlines with backslashes
Prefer single-line function bodies where appropriate (e.g., pub fn new(id: u64) -> Self { Self(id) })
Use cap-std for filesystem operations; avoid std::fs directly
Use camino for path handling; avoid std::path::PathBuf directly
Prefer semantic error enums deriving std::error::Error via thiserror for inspectable conditions
Use opaque eyre::Report only at the app boundary; do not expose in public APIs
Never export opaque error types from libraries; convert to domain...
Files:
src/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 / -yse / -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.
Environment access (env::set_var and env::remove_var) are always unsafe in Rust 2024 and MUST be marked as such
- For testing of functionality depending upon environment variables, dependency injection and the
mockablecrate are the preferred option.- If mockable cannot be used, env mutations in...
Files:
src/manifest.rs
🧬 Code graph analysis (1)
src/manifest.rs (1)
src/manifest/diagnostics.rs (1)
map_yaml_error(84-108)
🔍 Remote MCP Ref
Summary of additional, review-relevant facts
-
minijinja function wrappers must accept both positional varargs and keyword args plus preserve caller blocks. The registered wrapper should take (Rest, Kwargs) and forward both to the compiled template via Value::call to avoid “unexpected keyword argument” errors and to keep caller semantics.,
-
Coding/lint recommendation: change register_macro to accept &MacroDefinition (crate::ast) instead of separate signature/body &str parameters, and update register_manifest_macros call sites. This groups related data and reduces string-parameter coupling (addresses static analysis flagged in comments).
-
API/type-safety recommendation: add a small ManifestName newtype (Display + as_str) and change from_str_named/from_str/from_path to use &ManifestName to lower the proportion of bare string parameters and satisfy the linter. Implement manually (avoid new dependency). Re-run make lint && make test after changes.
Tests & verification
- The PR adds unit/Cucumber tests and YAML fixtures that exercise valid macro rendering (including nested calls) and invalid cases (missing signature / missing parentheses). Ensure the wrapper change preserves those behaviors and that all tests pass.
⏰ 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). (6)
- GitHub Check: release / build-linux (x86_64-unknown-linux-gnu, linux-amd64, amd64)
- GitHub Check: release / build-macos (x86_64-apple-darwin, macos-13, macos-x86_64, x86_64, macos-x86_64)
- GitHub Check: release / build-windows (x86_64-pc-windows-msvc, amd64, x64, windows-x86_64)
- GitHub Check: release / build-windows (aarch64-pc-windows-msvc, arm64, arm64, windows-aarch64)
- GitHub Check: Sourcery review
- GitHub Check: build-test (ubuntu-latest, stable)
🔇 Additional comments (7)
src/manifest.rs (7)
19-38: Well-designed newtype reduces string parameter coupling.The
ManifestNamewrapper provides type safety and addresses the static analysis threshold concern. The implementation is clean with appropriate accessor andDisplaytrait.
84-119: LGTM!The signature parsing logic is correct, error handling is comprehensive, and the documentation includes clear examples as required by the coding guidelines.
121-144: Well-refactored to accept MacroDefinition.The function now accepts
&MacroDefinitioninstead of separate signature/body strings, reducing coupling and addressing the static analysis concern. The error context is appropriate and the implementation is clear.
146-169: LGTM!The Serde-based deserialisation eliminates manual YAML walking and provides clear shape validation. The loop over definitions is straightforward with proper error context.
171-223: LGTM!The wrapper function correctly accepts both
Rest<Value>andKwargsin its signature, and the documentation includes a comprehensive example demonstrating keyword argument usage.
251-307: ManifestName integration and macro registration are correct.The
ManifestNamenewtype is consistently used across all entry points (from_str_named,from_str,from_path), and the call toregister_manifest_macrosat line 273 is correctly positioned before template expansion.
349-378: Verify manually that keyword-argument tests pass after applying the kwargs forwarding fix.
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (2)
Cargo.toml(1 hunks)src/manifest.rs(6 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
Cargo.toml
📄 CodeRabbit inference engine (AGENTS.md)
Cargo.toml: Use explicit version ranges in Cargo.toml and keep dependencies up to date
All dependencies must use SemVer caret requirements (e.g., "1.2.3")
Forbid wildcard (*) and open-ended (>=) version specifiers; use ~ only with documented, specific reasons
Files:
Cargo.toml
**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
**/*.rs: Keep functions small with single responsibility; apply command-query separation
Function documentation must include clear examples; test documentation should omit redundant examples
No single Rust code file may exceed 400 lines; split long switches/dispatch tables; move large test data to external files
Disallow Clippy warnings; code must be clean under clippy
Fix warnings emitted during tests in code, do not silence them
Extract helper functions when a function is too long; maintain separation of concerns and CQRS
Group many related parameters into well-named structs
Consider using Arc for large error values to reduce data returned
Every module must begin with a module-level //! comment describing purpose and utility
Document public APIs with Rustdoc /// comments so cargo doc can generate docs
Prefer immutable data; avoid unnecessary mut bindings
Use Result for error handling instead of panicking where feasible
Avoid unsafe code unless absolutely necessary; document any usage clearly
Place function attributes after doc comments
Do not use return in single-line functions
Use predicate functions for conditionals with more than two branches
Do not silence lints except as a last resort
When suppressing lints, scope narrowly and include a clear reason
Prefer #[expect(...)] over #[allow(...)] for lint handling
When functions are unused for certain features, gate with #[cfg]/#[cfg_attr]
Prefer .expect() over .unwrap()
Use concat!() for long string literals instead of escaping newlines with backslashes
Prefer single-line function bodies where appropriate (e.g., pub fn new(id: u64) -> Self { Self(id) })
Use cap-std for filesystem operations; avoid std::fs directly
Use camino for path handling; avoid std::path::PathBuf directly
Prefer semantic error enums deriving std::error::Error via thiserror for inspectable conditions
Use opaque eyre::Report only at the app boundary; do not expose in public APIs
Never export opaque error types from libraries; convert to domain...
Files:
src/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 / -yse / -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.
Environment access (env::set_var and env::remove_var) are always unsafe in Rust 2024 and MUST be marked as such
- For testing of functionality depending upon environment variables, dependency injection and the
mockablecrate are the preferred option.- If mockable cannot be used, env mutations in...
Files:
src/manifest.rs
🧬 Code graph analysis (1)
src/manifest.rs (2)
src/manifest/diagnostics.rs (1)
map_yaml_error(84-108)src/manifest/expand.rs (1)
expand_foreach(12-27)
🔍 Remote MCP Ref
Summary of additional review-relevant facts
- MiniJinja call semantics: wrapper functions registered with Environment must accept positional varargs (Rest) and keyword args (Kwargs) and forward both when invoking a compiled template Value; otherwise keyword arguments and caller blocks are lost and callers receive "unexpected keyword argument" errors. See MiniJinja API docs for Value/call usage.
Recommendation (concise)
- Change the macro wrapper used in env.add_function to accept (Rest, Kwargs) and forward both to template_value.call(...) so keyword args and caller semantics are preserved (addresses the functional bug reported in comments).
Sources used
- MiniJinja documentation (Value / call / function wrapper behavior).
- Search for MiniJinja function wrapper guidance.
⏰ 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). (7)
- GitHub Check: release / build-windows (aarch64-pc-windows-msvc, arm64, arm64, windows-aarch64)
- GitHub Check: release / build-macos (aarch64-apple-darwin, macos-14, macos-arm64, arm64, macos-aarch64)
- GitHub Check: release / build-windows (x86_64-pc-windows-msvc, amd64, x64, windows-x86_64)
- GitHub Check: release / build-macos (x86_64-apple-darwin, macos-13, macos-x86_64, x86_64, macos-x86_64)
- GitHub Check: release / build-linux (x86_64-unknown-linux-gnu, linux-amd64, amd64)
- GitHub Check: Sourcery review
- GitHub Check: build-test (ubuntu-latest, stable)
🔇 Additional comments (8)
src/manifest.rs (7)
10-15: Imports align with macro registration requirements.The added imports support the macro wrapper signature and AST integration.
19-38: ManifestName newtype implementation is sound.The newtype provides semantic clarity for manifest source names and reduces string-argument coupling as intended.
100-119: Signature parsing logic is correct.The function correctly validates and extracts macro identifiers. Documentation is clear.
131-144: Macro registration logic is correct.The function correctly compiles the macro body into a template and registers the wrapper.
156-169: Manifest macro registration is correctly implemented.The function appropriately deserialises macro definitions and registers each with proper error context.
207-223: Wrapper closure signature is correct.The function correctly creates a closure that accepts both positional and keyword arguments. However, the downstream
call_macro_valueandcall_value_with_kwargsfunctions mishandle kwargs (see separate comment).
264-296: Manifest parsing correctly integrates macro registration.The macro registration call at lines 286-287 occurs at the appropriate point in the parsing pipeline (after vars setup, before foreach expansion). The
ManifestNameusage is correct.Cargo.toml (1)
23-23: MiniJinja version bump is safe.The update from 2.11.0 to 2.12.0 introduces no breaking changes (per learnings: new |zip filter, wordwrap fixes, and improved error propagation). The caret requirement is correct per coding guidelines.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
src/manifest.rs(6 hunks)src/manifest/tests.rs(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
**/*.rs: Keep functions small with single responsibility; apply command-query separation
Function documentation must include clear examples; test documentation should omit redundant examples
No single Rust code file may exceed 400 lines; split long switches/dispatch tables; move large test data to external files
Disallow Clippy warnings; code must be clean under clippy
Fix warnings emitted during tests in code, do not silence them
Extract helper functions when a function is too long; maintain separation of concerns and CQRS
Group many related parameters into well-named structs
Consider using Arc for large error values to reduce data returned
Every module must begin with a module-level //! comment describing purpose and utility
Document public APIs with Rustdoc /// comments so cargo doc can generate docs
Prefer immutable data; avoid unnecessary mut bindings
Use Result for error handling instead of panicking where feasible
Avoid unsafe code unless absolutely necessary; document any usage clearly
Place function attributes after doc comments
Do not use return in single-line functions
Use predicate functions for conditionals with more than two branches
Do not silence lints except as a last resort
When suppressing lints, scope narrowly and include a clear reason
Prefer #[expect(...)] over #[allow(...)] for lint handling
When functions are unused for certain features, gate with #[cfg]/#[cfg_attr]
Prefer .expect() over .unwrap()
Use concat!() for long string literals instead of escaping newlines with backslashes
Prefer single-line function bodies where appropriate (e.g., pub fn new(id: u64) -> Self { Self(id) })
Use cap-std for filesystem operations; avoid std::fs directly
Use camino for path handling; avoid std::path::PathBuf directly
Prefer semantic error enums deriving std::error::Error via thiserror for inspectable conditions
Use opaque eyre::Report only at the app boundary; do not expose in public APIs
Never export opaque error types from libraries; convert to domain...
Files:
src/manifest.rssrc/manifest/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 / -yse / -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.
Environment access (env::set_var and env::remove_var) are always unsafe in Rust 2024 and MUST be marked as such
- For testing of functionality depending upon environment variables, dependency injection and the
mockablecrate are the preferred option.- If mockable cannot be used, env mutations in...
Files:
src/manifest.rssrc/manifest/tests.rs
🧬 Code graph analysis (2)
src/manifest.rs (1)
src/manifest/diagnostics.rs (1)
map_yaml_error(84-108)
src/manifest/tests.rs (1)
src/manifest.rs (3)
parse_macro_name(100-119)register_macro(131-144)register_manifest_macros(156-169)
🔍 Remote MCP Ref
Summary of additional review-relevant facts (concise)
-
MiniJinja function-wrapper must accept positional varargs (Rest) and keyword args (Kwargs) and forward both when invoking a compiled template Value via Value::call; otherwise keyword arguments and caller blocks are dropped and callers see "unexpected keyword argument" errors. Cite: MiniJinja Value docs and API guidance.,
-
Concrete fix for the functional bug in comments: change the env.add_function wrapper to accept (Rest, Kwargs) and call template_value.call(pos_args, kwargs) (preserving caller semantics). This aligns with MiniJinja examples for registering callable wrappers.
-
Refactor suggestions from PR discussion remain applicable and review-relevant:
- Change register_macro to take &MacroDefinition (reduces string-parameter smell and groups related fields).
- Introduce ManifestName newtype (replace &str name parameters) to improve diagnostics and reduce proportion of raw string parameters. These refactors also address static-analysis string-parameter warnings mentioned in PR comments. (No extra docs needed; these are code-style suggestions noted in PR discussion.)
Sources used
- MiniJinja API documentation (Value, call, keyword args / varargs examples).
- MiniJinja wrapper/function registration guidance (search results).
⏰ 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). (7)
- GitHub Check: release / build-macos (aarch64-apple-darwin, macos-14, macos-arm64, arm64, macos-aarch64)
- GitHub Check: release / build-macos (x86_64-apple-darwin, macos-13, macos-x86_64, x86_64, macos-x86_64)
- GitHub Check: release / build-windows (x86_64-pc-windows-msvc, amd64, x64, windows-x86_64)
- GitHub Check: release / build-windows (aarch64-pc-windows-msvc, arm64, arm64, windows-aarch64)
- GitHub Check: release / build-linux (aarch64-unknown-linux-gnu, linux-arm64, arm64)
- GitHub Check: release / build-linux (x86_64-unknown-linux-gnu, linux-amd64, amd64)
- GitHub Check: build-test (ubuntu-latest, stable)
Document the ManifestName accessors for the new public API and call the ValueCallExt helper directly from the manifest macro wrapper.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
src/manifest.rs(6 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
**/*.rs: Keep functions small with single responsibility; apply command-query separation
Function documentation must include clear examples; test documentation should omit redundant examples
No single Rust code file may exceed 400 lines; split long switches/dispatch tables; move large test data to external files
Disallow Clippy warnings; code must be clean under clippy
Fix warnings emitted during tests in code, do not silence them
Extract helper functions when a function is too long; maintain separation of concerns and CQRS
Group many related parameters into well-named structs
Consider using Arc for large error values to reduce data returned
Every module must begin with a module-level //! comment describing purpose and utility
Document public APIs with Rustdoc /// comments so cargo doc can generate docs
Prefer immutable data; avoid unnecessary mut bindings
Use Result for error handling instead of panicking where feasible
Avoid unsafe code unless absolutely necessary; document any usage clearly
Place function attributes after doc comments
Do not use return in single-line functions
Use predicate functions for conditionals with more than two branches
Do not silence lints except as a last resort
When suppressing lints, scope narrowly and include a clear reason
Prefer #[expect(...)] over #[allow(...)] for lint handling
When functions are unused for certain features, gate with #[cfg]/#[cfg_attr]
Prefer .expect() over .unwrap()
Use concat!() for long string literals instead of escaping newlines with backslashes
Prefer single-line function bodies where appropriate (e.g., pub fn new(id: u64) -> Self { Self(id) })
Use cap-std for filesystem operations; avoid std::fs directly
Use camino for path handling; avoid std::path::PathBuf directly
Prefer semantic error enums deriving std::error::Error via thiserror for inspectable conditions
Use opaque eyre::Report only at the app boundary; do not expose in public APIs
Never export opaque error types from libraries; convert to domain...
Files:
src/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 / -yse / -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.
Environment access (env::set_var and env::remove_var) are always unsafe in Rust 2024 and MUST be marked as such
- For testing of functionality depending upon environment variables, dependency injection and the
mockablecrate are the preferred option.- If mockable cannot be used, env mutations in...
Files:
src/manifest.rs
🧬 Code graph analysis (1)
src/manifest.rs (1)
src/manifest/diagnostics.rs (1)
map_yaml_error(84-108)
⏰ 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). (6)
- GitHub Check: release / build-macos (aarch64-apple-darwin, macos-14, macos-arm64, arm64, macos-aarch64)
- GitHub Check: release / build-linux (x86_64-unknown-linux-gnu, linux-amd64, amd64)
- GitHub Check: release / build-macos (x86_64-apple-darwin, macos-13, macos-x86_64, x86_64, macos-x86_64)
- GitHub Check: release / build-windows (x86_64-pc-windows-msvc, amd64, x64, windows-x86_64)
- GitHub Check: release / build-windows (aarch64-pc-windows-msvc, arm64, arm64, windows-aarch64)
- GitHub Check: build-test (ubuntu-latest, stable)
🔇 Additional comments (5)
src/manifest.rs (5)
19-56: LGTM! ManifestName type is well-implemented.The newtype wrapper provides clear semantics for manifest source names, includes proper documentation with examples, and implements Display for convenient string conversion. This addresses the static analysis concern about excessive string parameters.
102-137: LGTM! Signature parsing is robust.The function correctly validates macro signatures, extracts identifiers, and provides clear error messages for malformed input. Documentation includes examples covering both valid and invalid cases.
139-162: LGTM! Macro registration is clear and well-documented.The function properly compiles the macro body into a template, generates a unique template name, and registers the callable wrapper. Error context is appropriate.
164-187: LGTM! Serde-based deserialization is clean.The function leverages Serde to deserialize macro definitions, eliminating manual YAML traversal. The early return for missing macros key is appropriate, and error messages include the macro signature for clarity.
268-325: LGTM! Integration of ManifestName and macro registration is correct.The functions properly construct ManifestName instances for diagnostics, call
register_manifest_macrosbefore expansion, and pass the manifest name through to error reporting. The integration follows the established pattern.
Clarify why manifest macro wrappers append a Kwargs value before invoking minijinja::Value::call so keyword arguments reach macro implementations.
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
src/manifest.rs(6 hunks)src/manifest/tests.rs(1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
**/*.rs: Every Rust module must begin with a module-level comment using //! explaining the module's purpose and utility
Document public APIs with Rustdoc comments (///) so documentation can be generated with cargo doc
Prefer immutable data; avoid unnecessary mut bindings
Handle errors with Result instead of panicking where feasible
Prefer semantic error enums deriving std::error::Error via thiserror for inspectable conditions
Clippy warnings must be denied; do not silence lints except as a last resort
Lint suppressions must be tightly scoped and include a clear reason; prefer #[expect(...)] over #[allow(...)]
Place function attributes after doc comments
Do not use return in single-line functions
Prefer single-line function definitions where appropriate (e.g., pub fn new(id: u64) -> Self { Self(id) })
Use predicate functions for conditional criteria with more than two branches
Where a function is too long, extract meaningfully named helpers adhering to separation of concerns and CQRS
Where a function has too many parameters, group related parameters into meaningfully named structs
When returning large error data, consider using Arc to reduce data moved
Prefer .expect() over .unwrap() on Results/Options
Use concat!() to combine long string literals instead of escaping newlines with backslashes
Use NewTypes to model domain values; prefer explicit tuple structs; leverage newt-hype and the-newtype crates as described; provide appropriate From/AsRef/TryFrom implementations and avoid orphan-rule-violating impls
Keep each Rust code file under 400 lines; split long switches/dispatch tables by feature; move large test data to external files
Use functions and composition to avoid repetition; extract reusable logic; prefer declarative constructs (iterators/generators/comprehensions) when readable
Small, single-responsibility functions; obey command/query segregation
Name things precisely; booleans should use is/has/should prefixes
Comment why, not what; explain assumptions, e...
Files:
src/manifest.rssrc/manifest/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 / -yse / -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.
Environment access (env::set_var and env::remove_var) are always unsafe in Rust 2024 and MUST be marked as such
- For testing of functionality depending upon environment variables, dependency injection and the
mockablecrate are the preferred option.- If mockable cannot be used, env mutations in...
Files:
src/manifest.rssrc/manifest/tests.rs
**/*.{rs,md}
📄 CodeRabbit inference engine (AGENTS.md)
Use en-GB-oxendict spelling and grammar in comments and documentation (exceptions allowed for external API names)
Files:
src/manifest.rssrc/manifest/tests.rs
**/*{test,tests}.rs
📄 CodeRabbit inference engine (AGENTS.md)
Write unit and behavioural tests for new functionality and run them before and after changes
Files:
src/manifest/tests.rs
🧬 Code graph analysis (2)
src/manifest.rs (1)
src/manifest/diagnostics.rs (1)
map_yaml_error(84-108)
src/manifest/tests.rs (1)
src/manifest.rs (3)
parse_macro_name(119-138)register_macro(150-163)register_manifest_macros(175-188)
🪛 GitHub Actions: CI
src/manifest.rs
[error] 266-266: Clippy error: allow attribute without specifying a reason.
🪛 GitHub Check: build-test (ubuntu-latest, 1.89.0)
src/manifest.rs
[failure] 267-267:
reference as raw pointer
[failure] 267-267:
as casting between raw pointers without changing their constness
[failure] 267-267:
casting raw pointers to the same type and constness is unnecessary (*const minijinja::State<'_, '_> -> *const minijinja::State<'_, '_>)
[failure] 266-266:
allow attribute without specifying a reason
🪛 GitHub Check: build-test (ubuntu-latest, stable)
src/manifest.rs
[failure] 267-267:
reference as raw pointer
[failure] 267-267:
as casting between raw pointers without changing their constness
[failure] 267-267:
casting raw pointers to the same type and constness is unnecessary (*const minijinja::State<'_, '_> -> *const minijinja::State<'_, '_>)
[failure] 266-266:
allow attribute without specifying a reason
🔍 Remote MCP Ref
Summary of additional facts relevant to review
-
Functional bug: macro wrapper currently registers with only positional Rest, dropping keyword args and implicit caller — causes "unexpected keyword argument" at runtime. Fix: wrapper must accept and forward Kwargs and caller to compiled template.call.,
-
Suggested API-quality refactor: add ManifestName newtype (new/as_str/Display) and change from_str_named/from_str/from_path to take &ManifestName to reduce excessive string-parameter usage. This addresses static-analysis findings and clarifies diagnostic naming.,
-
Tests and fixtures added exercise macro parsing, registration, rendering, and error cases (including missing signature and missing-parens errors). Ensure CI runs make lint and make test after applying wrapper and refactor fixes.,
-
Dependency bump: minijinja updated 2.11.0 → 2.12.0; this version supports keyword-argument handling for custom functions and aligns with the required wrapper change. Verify compatibility with the new function signatures when implementing the fix.,
Sources used: repository PR page for PR #206 (leynos/netsuke) via repository search and page read.,
⏰ 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). (5)
- GitHub Check: release / build-windows (x86_64-pc-windows-msvc, amd64, x64, windows-x86_64)
- GitHub Check: release / build-macos (aarch64-apple-darwin, macos-14, macos-arm64, arm64, macos-aarch64)
- GitHub Check: release / build-macos (x86_64-apple-darwin, macos-13, macos-x86_64, x86_64, macos-x86_64)
- GitHub Check: release / build-linux (x86_64-unknown-linux-gnu, linux-amd64, amd64)
- GitHub Check: release / build-windows (aarch64-pc-windows-msvc, arm64, arm64, windows-aarch64)
🔇 Additional comments (5)
src/manifest.rs (3)
103-138: Doc and validation look solidThe signature parsing docs and errors read clearly and follow the guidelines. No changes needed.
226-256: Discard refactor usinglookupandValue::call
Neitherlookuponmacro_statenor the proposedValue::calloverload and alteredCallerAdapter::newsignature exist; retaincall_macroand current kwargs encoding.Likely an incorrect or invalid review comment.
258-280: Eliminate unsound raw-pointer state capture and unsafe impls in CallerAdapter
- Remove
state: *const State<'static, 'static>field, itsnew(state, caller)constructor and unsafeSend/Sync; simplify struct to only holdcaller(src/manifest.rs:258–280).- Use the
state: &Stateparameter inObject::calldirectly.- Update all
CallerAdapter::new(state, value)calls (e.g. inmake_macro_fn) toCallerAdapter::new(value).- Run
make lint && make testto verify no regressions.src/manifest/tests.rs (2)
71-81: Great coverage forcallerpropagationThis test will validate the
CallerAdapterfix. Keep it.
40-69: Solid argument-variant testsThese cases exercise positional and keyword arguments and multi-macro registration. No changes needed.
Run: make lint && make test
Also applies to: 99-108
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
docs/netsuke-design.md(3 hunks)src/manifest.rs(6 hunks)
🧰 Additional context used
📓 Path-based instructions (5)
**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
**/*.rs: Every Rust module must begin with a module-level comment using //! explaining the module's purpose and utility
Document public APIs with Rustdoc comments (///) so documentation can be generated with cargo doc
Prefer immutable data; avoid unnecessary mut bindings
Handle errors with Result instead of panicking where feasible
Prefer semantic error enums deriving std::error::Error via thiserror for inspectable conditions
Clippy warnings must be denied; do not silence lints except as a last resort
Lint suppressions must be tightly scoped and include a clear reason; prefer #[expect(...)] over #[allow(...)]
Place function attributes after doc comments
Do not use return in single-line functions
Prefer single-line function definitions where appropriate (e.g., pub fn new(id: u64) -> Self { Self(id) })
Use predicate functions for conditional criteria with more than two branches
Where a function is too long, extract meaningfully named helpers adhering to separation of concerns and CQRS
Where a function has too many parameters, group related parameters into meaningfully named structs
When returning large error data, consider using Arc to reduce data moved
Prefer .expect() over .unwrap() on Results/Options
Use concat!() to combine long string literals instead of escaping newlines with backslashes
Use NewTypes to model domain values; prefer explicit tuple structs; leverage newt-hype and the-newtype crates as described; provide appropriate From/AsRef/TryFrom implementations and avoid orphan-rule-violating impls
Keep each Rust code file under 400 lines; split long switches/dispatch tables by feature; move large test data to external files
Use functions and composition to avoid repetition; extract reusable logic; prefer declarative constructs (iterators/generators/comprehensions) when readable
Small, single-responsibility functions; obey command/query segregation
Name things precisely; booleans should use is/has/should prefixes
Comment why, not what; explain assumptions, e...
Files:
src/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 / -yse / -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.
Environment access (env::set_var and env::remove_var) are always unsafe in Rust 2024 and MUST be marked as such
- For testing of functionality depending upon environment variables, dependency injection and the
mockablecrate are the preferred option.- If mockable cannot be used, env mutations in...
Files:
src/manifest.rs
**/*.{rs,md}
📄 CodeRabbit inference engine (AGENTS.md)
Use en-GB-oxendict spelling and grammar in comments and documentation (exceptions allowed for external API names)
Files:
src/manifest.rsdocs/netsuke-design.md
docs/**/*.{rs,md}
📄 CodeRabbit inference engine (docs/rust-doctest-dry-guide.md)
docs/**/*.{rs,md}: In fenced documentation code blocks, specify the language as rust (```rust)
Use assert!, assert_eq!, and assert_ne! in doctests to verify outcomes rather than relying on panic-free execution alone
Avoid using .unwrap() or .expect() in examples; handle errors idiomatically
For fallible examples, write an explicit fn main() -> Result<...> and hide boilerplate with # lines
Alternatively, use the (()) shorthand at the end of a code block (no whitespace) to create an implicit Result-returning main
Use hidden lines (#) to hide setup, main wrappers, and non-essential use statements in doctests
Prefer no_run over ignore for examples with side effects (e.g., network, filesystem)
Use should_panic for examples intended to panic; the test should fail if it does not panic
Use compile_fail for examples that should not compile, but recognize its brittleness across compiler versions
Use edition20xx (e.g., edition2018, edition2021) on code fences when demonstrating edition-specific syntax
Avoid relying solely on #[cfg(feature = "...")] inside doctest code blocks, which can yield misleading empty-ok tests when the feature is disabled
Files:
docs/netsuke-design.md
docs/**/*.md
📄 CodeRabbit inference engine (AGENTS.md)
docs/**/*.md: Use docs/ as the reference knowledge base; proactively update docs/ when decisions, requirements, dependencies, or architecture change
Documentation in docs/ must use en-GB-oxendict spelling and grammar (LICENSE file name exempt)
docs/**/*.md: The word “outwith” is acceptable in documentation
Use the Oxford comma where it aids comprehension
Treat company names as collective nouns (e.g., “Lille Industries are …”)
Write headings in sentence case
Use Markdown headings (#,##,###, …) in order without skipping levels
Follow markdownlint recommendations
Provide code blocks and lists using standard Markdown syntax
Always provide a language identifier for fenced code blocks; useplaintextfor non-code text
Use-as the first-level bullet; renumber ordered lists when items change
Prefer inline links[text](url)or angle brackets<url>
Ensure blank lines before and after bulleted lists and fenced code blocks
Ensure tables have a delimiter line below the header row
Expand uncommon acronyms on first use (e.g., Continuous Integration (CI))
Wrap paragraphs at 80 columns
Wrap code at 120 columns in documentation
Do not wrap tables in documentation
Use footnotes referenced with[^label]
Include Mermaid diagrams where they add clarity
When embedding figures, useand provide brief, descriptive alt text
Add a short descriptive sentence before each Mermaid diagram for screen readers
Files:
docs/netsuke-design.md
**/*.md
📄 CodeRabbit inference engine (AGENTS.md)
**/*.md: Validate Markdown using make markdownlint; run make fmt after documentation changes to format Markdown and fix table markup
Validate Mermaid diagrams in Markdown using make nixie
Wrap Markdown paragraphs and bullet points at 80 columns
Wrap code blocks in Markdown at 120 columns
Do not wrap tables and headings in Markdown
Use dashes (-) for list bullets in Markdown
Use GitHub-flavoured Markdown footnotes ([^1]) for references
Files:
docs/netsuke-design.md
⚙️ CodeRabbit configuration file
**/*.md: * Avoid 2nd person or 1st person pronouns ("I", "you", "we")
- Use en-GB-oxendict (-ize / -yse / -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
🧬 Code graph analysis (1)
src/manifest.rs (2)
src/manifest/diagnostics.rs (1)
map_yaml_error(84-108)src/manifest/expand.rs (1)
expand_foreach(12-27)
🔍 Remote MCP Ref
Summary of additional, review-relevant facts
-
Functional bug confirmed: macro wrapper registers functions that accept only positional Rest, dropping Kwargs and caller forwarding — causes "unexpected keyword argument" for macros with named params. Fix: wrapper must accept Kwargs (and preserve caller) and forward both to template.call.,
-
Public API / surface changes to inspect during review:
- New AST type MacroDefinition { signature: String, body: String } and new NetsukeManifest.macros: Vec. Verify serde attributes (deny_unknown_fields / #[serde(default]).
- New ManifestName newtype and changed signatures: from_str_named(&ManifestName) and callers updated to construct ManifestName from path or default "Netsukefile". Check Display/as_str implementations and updated error messages using ManifestName.
-
Behavior & implementation notes:
- register_manifest_macros compiles macro bodies into templates and registers wrappers in the Environment before expansion — ensure wrapper forwards positional Rest, Kwargs, and caller to compiled_template.call to preserve keyword args and caller blocks. Also verify parse_macro_name validates signatures (parentheses present) and yields clear parse errors used by tests.
-
Tests & fixtures added (coverage to rely on in CI):
- Unit tests: ast_tests.rs, manifest_jinja_tests.rs, manifest/tests.rs — cover parsing, registration, rendering, and malformed-signature errors.
- Cucumber scenarios: tests/features/manifest.feature plus step implementations (manifest_steps.rs) asserting manifest.macros and signatures.
- YAML fixtures: tests/data/jinja_macros.yml, jinja_macro_arguments.yml, jinja_macro_invalid.yml, jinja_macro_missing_parens.yml. Verify tests exercise keyword-arg and caller cases where relevant.
-
Dependency change: minijinja bumped 2.11.0 → 2.12.0. Confirm that the newer minijinja API supports registering functions that accept Kwargs and Rest and that compiled template.call signature expectations align with the wrapper fix.
Actionable checklist for reviewing this PR
- Ensure the macro wrapper used in register_macro/register_manifest_macros accepts (positional Rest, kwargs: Kwargs, caller: Option<State/Caller>) and forwards both when invoking compiled_template.call.
- Confirm parse_macro_name rejects signatures without parentheses and yields the test-expected error text.
- Verify serde annotations on MacroDefinition and NetsukeManifest.macros (deny_unknown_fields, #[serde(default)]).
- Validate updated public API uses ManifestName consistently and that Display/as_str are implemented for diagnostics.
- Run make lint && make test locally / in CI (tests added target both unit and feature tests).
Sources
- Repository PR branch files and README references (search + file reads),
⏰ 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). (8)
- GitHub Check: release / build-macos (x86_64-apple-darwin, macos-13, macos-x86_64, x86_64, macos-x86_64)
- GitHub Check: release / build-macos (aarch64-apple-darwin, macos-14, macos-arm64, arm64, macos-aarch64)
- GitHub Check: release / build-linux (aarch64-unknown-linux-gnu, linux-arm64, arm64)
- GitHub Check: release / build-windows (aarch64-pc-windows-msvc, arm64, arm64, windows-aarch64)
- GitHub Check: release / build-linux (x86_64-unknown-linux-gnu, linux-amd64, amd64)
- GitHub Check: release / build-windows (x86_64-pc-windows-msvc, amd64, x64, windows-x86_64)
- GitHub Check: Sourcery review
- GitHub Check: build-test (ubuntu-latest, stable)
🔇 Additional comments (12)
src/manifest.rs (8)
1-17: LGTM! Imports align with macro support.The updated imports correctly support manifest macro registration:
MacroDefinitionfor the AST type,KwargsandObjectfor argument forwarding, andptr/Arcfor theCallerAdapterwrapper.
19-62: LGTM! ManifestName newtype improves API clarity.The newtype provides semantic clarity for diagnostic source names, addressing the string-heavy parameter issue. Doc comments follow guidelines with examples, and the
DisplayandAsRef<str>impls enable ergonomic usage at call sites.
108-143: LGTM! Function is well-documented and validates signatures correctly.The Rustdoc clearly explains the signature format requirement and error conditions. The implementation correctly validates and extracts the macro identifier.
145-168: LGTM! Function signature and documentation follow refactoring recommendations.The function now accepts a
MacroDefinitionparameter instead of separate strings, grouping related data. The Rustdoc clearly documents the purpose, parameters, and error conditions.
170-193: LGTM! Macro registration follows the refactored serde-based pattern.The function correctly deserializes macro definitions via
serde_yml::from_valueand registers each macro with clear error context. The implementation matches the PR objectives and previous refactoring suggestions.
195-230: LGTM! Comprehensive documentation for the macro wrapper.The Rustdoc provides a complete example demonstrating the wrapper's usage with keyword arguments, parameters, return value, and error conditions. The example correctly shows how positional and keyword arguments are forwarded.
286-351: LGTM! Macro registration correctly integrated into parsing flow.The
register_manifest_macroscall (line 316) is placed after globals are added but beforeexpand_foreach, ensuring macros are available during template expansion. TheManifestNamenewtype is correctly threaded through for error diagnostics.
231-261: Verify call_macro keyword-argument encoding
Verify thatcall_macroaccepts keyword arguments passed as a trailingValue::from(Kwargs). Confirm by running locally:cargo test --lib manifest::tests::register_macro_handles_arguments -- --exactdocs/netsuke-design.md (4)
169-172: LGTM! Schema documentation accurately describes the macros field.The description matches the implementation: macros is an optional list with
signatureandbodyfields, using YAML|block style for multi-line bodies. The documentation correctly states macros are registered before rendering other sections.
325-328: LGTM! Per-target macros documented correctly.The documentation accurately describes that targets can define local macros with the same signature and body structure. This matches the schema and allows for target-scoped macro definitions.
420-423: LGTM! Maintenance risk acknowledged appropriately.The note correctly documents the known maintenance and soundness issues with
serde_ymland plans a migration toserde_yaml_ng. This aligns with the retrieved learnings about the archived repository and advisories. Deferring to a follow-up ADR is appropriate for a design document.Based on learnings
698-735: LGTM! Implementation description is accurate and comprehensive.The documentation correctly describes the manifest macro implementation:
- Compilation into internal templates (✓ lines 158-161 in src/manifest.rs)
- Wrapper function registration (✓ line 166)
- Fresh state per invocation (✓ line 237)
- Keyword parameter preservation (✓ lines 238-252)
- Caller block support (✓ lines 243-246 via CallerAdapter)
- Override behaviour (✓ MiniJinja add_function semantics)
The explanation provides appropriate detail for a design document without over-specifying implementation details.
- explain the raw pointer invariants around CallerAdapter so review safety concerns are addressed - capture follow-up ADR and migration spike tasks for replacing serde_yml as requested during review
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
docs/netsuke-design.md(3 hunks)src/manifest.rs(6 hunks)
🧰 Additional context used
📓 Path-based instructions (5)
docs/**/*.{rs,md}
📄 CodeRabbit inference engine (docs/rust-doctest-dry-guide.md)
docs/**/*.{rs,md}: In fenced documentation code blocks, specify the language as rust (```rust)
Use assert!, assert_eq!, and assert_ne! in doctests to verify outcomes rather than relying on panic-free execution alone
Avoid using .unwrap() or .expect() in examples; handle errors idiomatically
For fallible examples, write an explicit fn main() -> Result<...> and hide boilerplate with # lines
Alternatively, use the (()) shorthand at the end of a code block (no whitespace) to create an implicit Result-returning main
Use hidden lines (#) to hide setup, main wrappers, and non-essential use statements in doctests
Prefer no_run over ignore for examples with side effects (e.g., network, filesystem)
Use should_panic for examples intended to panic; the test should fail if it does not panic
Use compile_fail for examples that should not compile, but recognize its brittleness across compiler versions
Use edition20xx (e.g., edition2018, edition2021) on code fences when demonstrating edition-specific syntax
Avoid relying solely on #[cfg(feature = "...")] inside doctest code blocks, which can yield misleading empty-ok tests when the feature is disabled
Files:
docs/netsuke-design.md
**/*.{rs,md}
📄 CodeRabbit inference engine (AGENTS.md)
Use en-GB-oxendict spelling and grammar in comments and documentation (exceptions allowed for external API names)
Files:
docs/netsuke-design.mdsrc/manifest.rs
docs/**/*.md
📄 CodeRabbit inference engine (AGENTS.md)
docs/**/*.md: Use docs/ as the reference knowledge base; proactively update docs/ when decisions, requirements, dependencies, or architecture change
Documentation in docs/ must use en-GB-oxendict spelling and grammar (LICENSE file name exempt)
docs/**/*.md: The word “outwith” is acceptable in documentation
Use the Oxford comma where it aids comprehension
Treat company names as collective nouns (e.g., “Lille Industries are …”)
Write headings in sentence case
Use Markdown headings (#,##,###, …) in order without skipping levels
Follow markdownlint recommendations
Provide code blocks and lists using standard Markdown syntax
Always provide a language identifier for fenced code blocks; useplaintextfor non-code text
Use-as the first-level bullet; renumber ordered lists when items change
Prefer inline links[text](url)or angle brackets<url>
Ensure blank lines before and after bulleted lists and fenced code blocks
Ensure tables have a delimiter line below the header row
Expand uncommon acronyms on first use (e.g., Continuous Integration (CI))
Wrap paragraphs at 80 columns
Wrap code at 120 columns in documentation
Do not wrap tables in documentation
Use footnotes referenced with[^label]
Include Mermaid diagrams where they add clarity
When embedding figures, useand provide brief, descriptive alt text
Add a short descriptive sentence before each Mermaid diagram for screen readers
Files:
docs/netsuke-design.md
**/*.md
📄 CodeRabbit inference engine (AGENTS.md)
**/*.md: Validate Markdown using make markdownlint; run make fmt after documentation changes to format Markdown and fix table markup
Validate Mermaid diagrams in Markdown using make nixie
Wrap Markdown paragraphs and bullet points at 80 columns
Wrap code blocks in Markdown at 120 columns
Do not wrap tables and headings in Markdown
Use dashes (-) for list bullets in Markdown
Use GitHub-flavoured Markdown footnotes ([^1]) for references
Files:
docs/netsuke-design.md
⚙️ CodeRabbit configuration file
**/*.md: * Avoid 2nd person or 1st person pronouns ("I", "you", "we")
- Use en-GB-oxendict (-ize / -yse / -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
**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
**/*.rs: Every Rust module must begin with a module-level comment using //! explaining the module's purpose and utility
Document public APIs with Rustdoc comments (///) so documentation can be generated with cargo doc
Prefer immutable data; avoid unnecessary mut bindings
Handle errors with Result instead of panicking where feasible
Prefer semantic error enums deriving std::error::Error via thiserror for inspectable conditions
Clippy warnings must be denied; do not silence lints except as a last resort
Lint suppressions must be tightly scoped and include a clear reason; prefer #[expect(...)] over #[allow(...)]
Place function attributes after doc comments
Do not use return in single-line functions
Prefer single-line function definitions where appropriate (e.g., pub fn new(id: u64) -> Self { Self(id) })
Use predicate functions for conditional criteria with more than two branches
Where a function is too long, extract meaningfully named helpers adhering to separation of concerns and CQRS
Where a function has too many parameters, group related parameters into meaningfully named structs
When returning large error data, consider using Arc to reduce data moved
Prefer .expect() over .unwrap() on Results/Options
Use concat!() to combine long string literals instead of escaping newlines with backslashes
Use NewTypes to model domain values; prefer explicit tuple structs; leverage newt-hype and the-newtype crates as described; provide appropriate From/AsRef/TryFrom implementations and avoid orphan-rule-violating impls
Keep each Rust code file under 400 lines; split long switches/dispatch tables by feature; move large test data to external files
Use functions and composition to avoid repetition; extract reusable logic; prefer declarative constructs (iterators/generators/comprehensions) when readable
Small, single-responsibility functions; obey command/query segregation
Name things precisely; booleans should use is/has/should prefixes
Comment why, not what; explain assumptions, e...
Files:
src/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 / -yse / -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.
Environment access (env::set_var and env::remove_var) are always unsafe in Rust 2024 and MUST be marked as such
- For testing of functionality depending upon environment variables, dependency injection and the
mockablecrate are the preferred option.- If mockable cannot be used, env mutations in...
Files:
src/manifest.rs
🧬 Code graph analysis (1)
src/manifest.rs (1)
src/manifest/diagnostics.rs (1)
map_yaml_error(84-108)
🔍 Remote MCP Ref
Summary of additional review-relevant facts
-
Confirmed functional bug: the macro wrapper currently registers functions that accept only positional Rest, which drops Kwargs and the caller; this causes "unexpected keyword argument" for macros with named parameters. The wrapper must accept both Rest and Kwargs (and preserve caller) and forward them to the compiled template's call.,
-
Public API/AST changes to verify:
- New AST type MacroDefinition { signature: String, body: String } with serde deny_unknown_fields.
- New NetsukeManifest.macros: Vec with #[serde(default)].
- New ManifestName newtype used in from_str_named/from_str/from_path for diagnostics (ensure Display/as_str). Verify all call sites updated.
-
Behavior to validate in register_macro/register_manifest_macros:
- Macros are compiled into internal templates and a wrapper is registered into the Environment before expansion.
- Wrapper must accept (positional: Rest, kwargs: Kwargs, caller) and call compiled_template.call with both positional args and kwargs (and pass caller) to preserve keyword arguments and caller blocks.
-
Tests & fixtures to rely on in CI:
- Unit tests: tests/ast_tests.rs, tests/manifest_jinja_tests.rs, manifest/tests.rs (cover parsing, registration, rendering, malformed signatures).
- Cucumber features: tests/features/manifest.feature and steps in tests/steps/manifest_steps.rs.
- YAML fixtures: tests/data/jinja_macros.yml, jinja_macro_arguments.yml, jinja_macro_invalid.yml, jinja_macro_missing_parens.yml. Ensure tests that expect keyword/caller behavior are exercising the fixed wrapper.
-
Dependency note:
- minijinja bumped 2.11.0 → 2.12.0; confirm the minijinja API in 2.12.0 supports registering functions that accept Kwargs and Rest and that compiled_template.call accepts forwarded kwargs/caller as required.
Actions to request from the author (concise)
- Change macro wrapper to accept Rest and Kwargs (and preserve caller) and forward both to compiled_template.call.
- (Optional but recommended) Refactor register_macro signature to take &MacroDefinition and add a ManifestName newtype to reduce string-heavy arguments; run make lint && make test after changes.
Tools used
- Ref_ref_search_documentation (Ref)
- Ref_ref_read_url (Ref)
⏰ 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). (8)
- GitHub Check: release / build-linux (aarch64-unknown-linux-gnu, linux-arm64, arm64)
- GitHub Check: release / build-macos (aarch64-apple-darwin, macos-14, macos-arm64, arm64, macos-aarch64)
- GitHub Check: release / build-windows (x86_64-pc-windows-msvc, amd64, x64, windows-x86_64)
- GitHub Check: release / build-linux (x86_64-unknown-linux-gnu, linux-amd64, amd64)
- GitHub Check: release / build-windows (aarch64-pc-windows-msvc, arm64, arm64, windows-aarch64)
- GitHub Check: release / build-macos (x86_64-apple-darwin, macos-13, macos-x86_64, x86_64, macos-x86_64)
- GitHub Check: Sourcery review
- GitHub Check: build-test (ubuntu-latest, stable)
🔇 Additional comments (11)
src/manifest.rs (7)
10-18: Imports look correct for macro functionality.The additions bring in MacroDefinition from the AST, MiniJinja types for macro invocation (State, Kwargs, Object, Rest, Value), and std primitives (ptr, Arc) for the CallerAdapter. All are used in the implementation below.
19-62: ManifestName newtype is well-implemented.The newtype provides type safety for diagnostic naming, reducing string-heavy parameter issues flagged by static analysis. The doc comments include examples, Display and AsRef implementations enable ergonomic use at call sites, and the pattern matches the coding guidelines for domain newtypes.
108-143: parse_macro_name validation is correct.The function validates signature format (must contain parentheses), extracts the identifier before the opening paren, and rejects empty identifiers. Error messages are clear and actionable. The logic handles edge cases like leading/trailing whitespace.
145-168: register_macro implementation is sound.The function parses the macro name, synthesises a unique internal template name, wraps the macro body in Jinja macro syntax, compiles it via add_template_owned, and registers the wrapper function. Context is added to compilation errors. The flow matches the documented design.
170-193: register_manifest_macros correctly uses Serde.The function returns early when no macros are present, uses serde_yml::from_value to deserialise the macro list with a clear error message when the shape is invalid, and registers each macro with context. This addresses previous feedback to use Serde instead of manual YAML walking.
263-304: CallerAdapter safety is documented but fragile.The adapter stores a raw pointer to a State with a cast lifetime (line 286), dereferences it unsafely in Object::call (line 301), and implements Send/Sync without synchronisation (lines 296-297). The doc comment explains the safety contract: the original State must outlive all adapter invocations, no async suspension, and no cross-thread migration. This is acceptable given MiniJinja's synchronous execution model, but any future async support or threading changes would break these assumptions.
Confirm that MiniJinja's call_macro and Object::call execute synchronously on the same thread that created the CallerAdapter. Document this constraint in the module-level comment so future maintainers are aware of the threading assumptions.
314-370: ManifestName integration is correct.The from_str_named signature now takes
&ManifestNameinstead of&str(line 314), usesname.as_ref()to extract the string when needed (lines 316, 342), and from_str/from_path construct a ManifestName via ManifestName::new (lines 357, 369). This reduces string-heavy parameters and improves type safety, addressing the static analysis findings.docs/netsuke-design.md (4)
169-172: Macro documentation is accurate.The description matches the implementation: macros is an optional list at the top level, each item has signature and body strings, body uses YAML block syntax, and macros are registered before rendering other sections. This aligns with the code in src/manifest.rs.
420-431: Maintenance risk note is appropriate.The note acknowledges serde_yml's archived status and unsoundness advisories, mentions the candidate replacement (serde_yaml_ng), and outlines follow-up actions (ADR drafting and migration spike). This is honest communication about technical debt and provides a clear mitigation plan.
Based on learnings
554-554: Example initialisation is correct.The example AST snippet shows
macros: vec![]which matches the #[serde(default)] behaviour in the NetsukeManifest struct (the field defaults to an empty Vec when not present in YAML). This accurately represents the typed structure.
736-741: Macro loading documentation requires correction.The documentation states that macros are "registered with a fresh MiniJinja state for every invocation" and "preserves MiniJinja's argument handling, including keyword parameters and
callersupport", but the current implementation in src/manifest.rs (lines 231-261) does not correctly forward keyword arguments. The code appends kwargs as a Value to positional args instead of passing them as a separate parameter to Value::call, which breaks keyword and caller support. This documentation describes the intended behaviour but is inconsistent with the actual implementation.Update this section after the critical kwargs forwarding bug in src/manifest.rs is fixed. The documentation should note that keyword arguments and caller blocks work correctly only after the wrapper calls
value.call(¯o_state, args, kwargs)with kwargs as a separate parameter, not embedded in the positional args list.
|
@sourcery-ai review |
There was a problem hiding this comment.
Hey there - I've reviewed your changes and found some issues that need to be addressed.
- Given the size of the new macro‐related code in manifest.rs, consider refactoring it into a dedicated jinja_macros module to keep manifest.rs more focused and maintainable.
- The raw pointer use in CallerAdapter is fragile—document the safety invariants clearly or encapsulate the unsafety behind a safer API to prevent misuse down the road.
- Since each macro invocation builds a fresh State and re‐evaluates the template, you may want to benchmark hot code paths and consider caching the compiled state if performance becomes an issue.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Given the size of the new macro‐related code in manifest.rs, consider refactoring it into a dedicated jinja_macros module to keep manifest.rs more focused and maintainable.
- The raw pointer use in CallerAdapter is fragile—document the safety invariants clearly or encapsulate the unsafety behind a safer API to prevent misuse down the road.
- Since each macro invocation builds a fresh State and re‐evaluates the template, you may want to benchmark hot code paths and consider caching the compiled state if performance becomes an issue.
## Individual Comments
### Comment 1
<location> `src/manifest.rs:293-295` </location>
<code_context>
+ // MiniJinja requires keyword arguments to be appended as a trailing
+ // `Kwargs` value within the positional slice. Build that value lazily so
+ // we avoid allocating when no keywords were supplied.
+ let mut entries: Vec<(String, Value)> = Vec::new();
+ for key in kwargs.args() {
+ let mut value = kwargs.peek::<Value>(key)?;
+ if key == "caller" {
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Special handling for 'caller' argument may need further validation.
Currently, 'caller' is wrapped without verifying if it's callable, which may cause runtime errors if an invalid value is provided. Please add a type check before wrapping.
```suggestion
if key == "caller" {
// Ensure the value is callable before wrapping
if value.is_callable() {
value = Value::from_object(CallerAdapter::new(state, value));
} else {
return Err(Error::new(
ErrorKind::InvalidOperation,
format!("'caller' argument must be callable, got {:?}", value.kind())
));
}
}
```
</issue_to_address>
### Comment 2
<location> `src/manifest/tests.rs:95-22` </location>
<code_context>
- let pattern = format!("{}/missing*", tmp.path().display());
- let result = glob_paths(&pattern).expect("glob ok");
- assert!(result.is_empty());
+fn register_manifest_macros_validates_shape(mut strict_env: Environment) {
+ let mut mapping = Mapping::new();
+ mapping.insert(
+ YamlValue::from("macros"),
+ YamlValue::from(vec![YamlValue::from(42)]),
+ );
+ let doc = YamlValue::Mapping(mapping);
+ let err = register_manifest_macros(&doc, &mut strict_env).expect_err("shape error");
</code_context>
<issue_to_address>
**suggestion (testing):** Test for invalid macro shape is good, but missing test for missing 'body' field.
Please add a test where the macro definition omits the 'body' field to verify error handling for missing required fields.
</issue_to_address>
### Comment 3
<location> `tests/manifest_jinja_tests.rs:247-261` </location>
<code_context>
+}
+
+#[rstest]
+fn manifest_macro_with_missing_signature_errors() {
+ let yaml = manifest_yaml(concat!(
+ "macros:\n",
+ " - body: |\n",
+ " hi\n",
+ "targets:\n",
+ " - name: noop\n",
+ " command: noop\n",
+ ));
+
+ let err = manifest::from_str(&yaml).expect_err("macro signature required");
+ let msg = format!("{err:?}");
+ assert!(msg.contains("signature"), "error message: {msg}");
+}
+
</code_context>
<issue_to_address>
**suggestion (testing):** Missing macro signature error is tested, but missing test for missing 'body'.
Please add a test case for a macro definition missing the 'body' field to verify error handling for all required fields.
```suggestion
#[rstest]
fn manifest_macro_with_missing_signature_errors() {
let yaml = manifest_yaml(concat!(
"macros:\n",
" - body: |\n",
" hi\n",
"targets:\n",
" - name: noop\n",
" command: noop\n",
));
let err = manifest::from_str(&yaml).expect_err("macro signature required");
let msg = format!("{err:?}");
assert!(msg.contains("signature"), "error message: {msg}");
}
#[rstest]
fn manifest_macro_with_missing_body_errors() {
let yaml = manifest_yaml(concat!(
"macros:\n",
" - signature: \"greet(name)\"\n",
"targets:\n",
" - name: noop\n",
" command: noop\n",
));
let err = manifest::from_str(&yaml).expect_err("macro body required");
let msg = format!("{err:?}");
assert!(msg.contains("body"), "error message: {msg}");
}
```
</issue_to_address>
### Comment 4
<location> `src/manifest.rs:19` </location>
<code_context>
-use std::{fs, path::Path};
+use std::{fs, path::Path, ptr, sync::Arc};
+
+/// A display name for a manifest source, used in error reporting.
+#[derive(Debug, Clone)]
+pub struct ManifestName(String);
</code_context>
<issue_to_address>
**issue (complexity):** Consider moving macro-related logic and ManifestName into separate submodules to keep manifest.rs focused and improve code organization.
```markdown
You’ve added a lot of new “macro plumbing” (parsing, registering, calling, the `CallerAdapter`), plus `ManifestName`, all into one huge `manifest.rs`. I’d recommend moving all of that into two small sub‐modules:
1. `name.rs` — just your `ManifestName` newtype.
2. `macros.rs` — all of your `parse_macro_name`, `register_*`, `make_macro_fn`, `call_macro_value`, and `CallerAdapter`.
That way `manifest.rs` stays focused on loading/parsing/rendering YAML+Jinja, and all the macro code lives elsewhere.
Here’s a sketch:
```rust
// src/manifest/name.rs
#[derive(Debug, Clone)]
pub struct ManifestName(String);
impl ManifestName {
pub fn new(s: impl Into<String>) -> Self { Self(s.into()) }
pub fn as_str(&self) -> &str { &self.0 }
}
impl Display for ManifestName {
fn fmt(&self, f: &mut Formatter<'_>) -> fmt::Result {
write!(f, "{}", self.0)
}
}
```
```rust
// src/manifest/macros.rs
use minijinja::{
Environment,
value::{Rest, Kwargs, Value},
State, Error, ErrorKind
};
use crate::ast::MacroDefinition;
use serde_yml::Value as YamlValue;
use anyhow::Result;
use std::{sync::Arc, ptr};
pub fn parse_macro_name(sig: &str) -> Result<String> { /* ... */ }
pub fn register_macro(env: &mut Environment, def: &MacroDefinition, idx: usize) -> Result<()> { /* ... */ }
pub fn register_manifest_macros(doc: &YamlValue, env: &mut Environment) -> Result<()> { /* ... */ }
pub fn call_macro_value(
state: &State,
mv: &Value,
pos: &[Value],
kw: Option<Kwargs>,
) -> Result<Value, Error> { /* ... */ }
pub fn make_macro_fn(
tmpl: String,
name: String
) -> impl Fn(&State, Rest<Value>, Kwargs) -> Result<Value, Error> {
move |state, Rest(args), kwargs| {
/* ... */
}
}
#[derive(Debug)]
struct CallerAdapter {
inner: Value,
ptr: *const State<'static, 'static>,
}
// safety comment…
unsafe impl Send for CallerAdapter {}
unsafe impl Sync for CallerAdapter {}
impl Object for CallerAdapter {
fn call(self: &Arc<Self>, _st: &State, args: &[Value]) -> Result<Value, Error> {
let st = unsafe { &*self.ptr };
self.inner.call(st, args)
}
}
```
Then in `manifest.rs`:
```rust
mod name;
mod macros;
use name::ManifestName;
use macros::register_manifest_macros;
// …later in from_str_named:
let mut jinja = Environment::new();
// …
register_manifest_macros(&doc, &mut jinja)?;
```
This keeps each file ~50 lines, clarifies intent, and separates concerns without losing functionality.
</issue_to_address>
### Comment 5
<location> `src/manifest.rs:180` </location>
<code_context>
+///
+/// Returns an error if the YAML shape is invalid, any macro signature is
+/// malformed, or template compilation fails.
+fn register_manifest_macros(doc: &YamlValue, env: &mut Environment) -> Result<()> {
+ let Some(macros) = doc.get("macros").cloned() else {
+ return Ok(());
</code_context>
<issue_to_address>
**issue (review_instructions):** Add behavioural and unit tests for manifest macro registration, including error cases and macro invocation.
The new macro registration logic (register_manifest_macros, register_macro, make_macro_fn, etc.) must be covered by both behavioural and unit tests. Ensure tests demonstrate correct macro registration, invocation, error handling for invalid signatures, and integration with the manifest loader.
<details>
<summary>Review instructions:</summary>
**Path patterns:** `**/*`
**Instructions:**
For any new feature or change to an existing feature, both behavioural *and* unit tests are required.
</details>
</issue_to_address>
### Comment 6
<location> `src/ast.rs:59` </location>
<code_context>
/// assert_eq!(manifest.targets.len(), 1);
/// # Ok(()) }
/// ```
+#[derive(Debug, Clone, Deserialize, Serialize)]
+#[serde(deny_unknown_fields)]
+pub struct MacroDefinition {
</code_context>
<issue_to_address>
**issue (review_instructions):** Add unit and behavioural tests for MacroDefinition struct and its deserialization from YAML.
The MacroDefinition struct and its deserialization should be covered by unit tests (for struct correctness and edge cases) and behavioural tests (for manifest parsing and macro usage).
<details>
<summary>Review instructions:</summary>
**Path patterns:** `**/*`
**Instructions:**
For any new feature or change to an existing feature, both behavioural *and* unit tests are required.
</details>
</issue_to_address>
### Comment 7
<location> `src/ast.rs:59` </location>
<code_context>
/// assert_eq!(manifest.targets.len(), 1);
/// # Ok(()) }
/// ```
+#[derive(Debug, Clone, Deserialize, Serialize)]
+#[serde(deny_unknown_fields)]
+pub struct MacroDefinition {
</code_context>
<issue_to_address>
**issue (review_instructions):** Missing `//!` module-level comment at the top of the file.
Please add a `//!` doc comment at the very top of this module to describe its purpose, as required by the review instructions.
<details>
<summary>Review instructions:</summary>
**Path patterns:** `**/*.rs`
**Instructions:**
Every module **must** begin with a `//!` comment.
</details>
</issue_to_address>
### Comment 8
<location> `docs/netsuke-design.md:422` </location>
<code_context>
+**Maintenance risk.** `serde_yml` is archived upstream and carries unsoundness
+advisories. Netsuke relies on it today, but we will investigate a maintained
+successor such as `serde_yaml_ng`. A follow-up ADR will outline the migration
+plan and compatibility testing.
+
</code_context>
<issue_to_address>
**suggestion (review_instructions):** The phrase "we will investigate" uses a first person pronoun, which should be avoided per the instructions.
Consider rephrasing to passive voice, e.g., "A maintained successor such as `serde_yaml_ng` will be investigated."
<details>
<summary>Review instructions:</summary>
**Path patterns:** `**/*.md`
**Instructions:**
Avoid 2nd person or 1st person pronouns ("I", "you", "we")
</details>
</issue_to_address>
### Comment 9
<location> `docs/netsuke-design.md:422` </location>
<code_context>
+**Maintenance risk.** `serde_yml` is archived upstream and carries unsoundness
+advisories. Netsuke relies on it today, but we will investigate a maintained
+successor such as `serde_yaml_ng`. A follow-up ADR will outline the migration
+plan and compatibility testing.
+
</code_context>
<issue_to_address>
**suggestion (review_instructions):** This paragraph line exceeds 80 columns and should be wrapped accordingly.
Please wrap the paragraph so that no line exceeds 80 columns, except for headings and code blocks.
<details>
<summary>Review instructions:</summary>
**Path patterns:** `**/*.md`
**Instructions:**
Paragraphs and bullets must be wrapped to 80 columns
</details>
</issue_to_address>
### Comment 10
<location> `docs/netsuke-design.md:736` </location>
<code_context>
built-in definition. This mirrors Jinja's behaviour and follows `minijinja`
semantics where later definitions shadow earlier ones.
+The manifest loader compiles each macro definition into an internal template
+and registers a wrapper function that evaluates the macro on demand. The
+wrapper constructs a fresh MiniJinja state for every invocation so macro calls
</code_context>
<issue_to_address>
**suggestion (review_instructions):** This paragraph line exceeds 80 columns and should be wrapped accordingly.
Please wrap the paragraph so that no line exceeds 80 columns, except for headings and code blocks.
<details>
<summary>Review instructions:</summary>
**Path patterns:** `**/*.md`
**Instructions:**
Paragraphs and bullets must be wrapped to 80 columns
</details>
</issue_to_address>
### Comment 11
<location> `docs/roadmap.md:124` </location>
<code_context>
+
+ - [ ] Draft an ADR evaluating maintained replacements for `serde_yml`
+ (for example `serde_yaml_ng`) and record the migration decision.
+ - [ ] Run a migration spike with the preferred crate, exercising the manifest
+ fixtures to capture compatibility notes and required mitigations.
+
</code_context>
<issue_to_address>
**suggestion (review_instructions):** This bullet line exceeds 80 columns and should be wrapped accordingly.
Please wrap the bullet so that no line exceeds 80 columns.
<details>
<summary>Review instructions:</summary>
**Path patterns:** `**/*.md`
**Instructions:**
Paragraphs and bullets must be wrapped to 80 columns
</details>
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| if key == "caller" { | ||
| value = Value::from_object(CallerAdapter::new(state, value)); | ||
| } |
There was a problem hiding this comment.
suggestion (bug_risk): Special handling for 'caller' argument may need further validation.
Currently, 'caller' is wrapped without verifying if it's callable, which may cause runtime errors if an invalid value is provided. Please add a type check before wrapping.
| if key == "caller" { | |
| value = Value::from_object(CallerAdapter::new(state, value)); | |
| } | |
| if key == "caller" { | |
| // Ensure the value is callable before wrapping | |
| if value.is_callable() { | |
| value = Value::from_object(CallerAdapter::new(state, value)); | |
| } else { | |
| return Err(Error::new( | |
| ErrorKind::InvalidOperation, | |
| format!("'caller' argument must be callable, got {:?}", value.kind()) | |
| )); | |
| } | |
| } |
| fn normalize_separators_preserves_bracket_escape_variants(#[case] pat: &str) { | ||
| assert_eq!(normalize_separators(pat), pat); | ||
| #[case("greet(name)", "greet")] | ||
| #[case("shout(text='hi')", "shout")] |
There was a problem hiding this comment.
suggestion (testing): Test for invalid macro shape is good, but missing test for missing 'body' field.
Please add a test where the macro definition omits the 'body' field to verify error handling for missing required fields.
| #[rstest] | ||
| fn manifest_macro_with_missing_signature_errors() { | ||
| let yaml = manifest_yaml(concat!( | ||
| "macros:\n", | ||
| " - body: |\n", | ||
| " hi\n", | ||
| "targets:\n", | ||
| " - name: noop\n", | ||
| " command: noop\n", | ||
| )); | ||
|
|
||
| let err = manifest::from_str(&yaml).expect_err("macro signature required"); | ||
| let msg = format!("{err:?}"); | ||
| assert!(msg.contains("signature"), "error message: {msg}"); | ||
| } |
There was a problem hiding this comment.
suggestion (testing): Missing macro signature error is tested, but missing test for missing 'body'.
Please add a test case for a macro definition missing the 'body' field to verify error handling for all required fields.
| #[rstest] | |
| fn manifest_macro_with_missing_signature_errors() { | |
| let yaml = manifest_yaml(concat!( | |
| "macros:\n", | |
| " - body: |\n", | |
| " hi\n", | |
| "targets:\n", | |
| " - name: noop\n", | |
| " command: noop\n", | |
| )); | |
| let err = manifest::from_str(&yaml).expect_err("macro signature required"); | |
| let msg = format!("{err:?}"); | |
| assert!(msg.contains("signature"), "error message: {msg}"); | |
| } | |
| #[rstest] | |
| fn manifest_macro_with_missing_signature_errors() { | |
| let yaml = manifest_yaml(concat!( | |
| "macros:\n", | |
| " - body: |\n", | |
| " hi\n", | |
| "targets:\n", | |
| " - name: noop\n", | |
| " command: noop\n", | |
| )); | |
| let err = manifest::from_str(&yaml).expect_err("macro signature required"); | |
| let msg = format!("{err:?}"); | |
| assert!(msg.contains("signature"), "error message: {msg}"); | |
| } | |
| #[rstest] | |
| fn manifest_macro_with_missing_body_errors() { | |
| let yaml = manifest_yaml(concat!( | |
| "macros:\n", | |
| " - signature: \"greet(name)\"\n", | |
| "targets:\n", | |
| " - name: noop\n", | |
| " command: noop\n", | |
| )); | |
| let err = manifest::from_str(&yaml).expect_err("macro body required"); | |
| let msg = format!("{err:?}"); | |
| assert!(msg.contains("body"), "error message: {msg}"); | |
| } |
| use std::{fs, path::Path}; | ||
| use std::{fs, path::Path, ptr, sync::Arc}; | ||
|
|
||
| /// A display name for a manifest source, used in error reporting. |
There was a problem hiding this comment.
issue (complexity): Consider moving macro-related logic and ManifestName into separate submodules to keep manifest.rs focused and improve code organization.
You’ve added a lot of new “macro plumbing” (parsing, registering, calling, the `CallerAdapter`), plus `ManifestName`, all into one huge `manifest.rs`. I’d recommend moving all of that into two small sub‐modules:
1. `name.rs` — just your `ManifestName` newtype.
2. `macros.rs` — all of your `parse_macro_name`, `register_*`, `make_macro_fn`, `call_macro_value`, and `CallerAdapter`.
That way `manifest.rs` stays focused on loading/parsing/rendering YAML+Jinja, and all the macro code lives elsewhere.
Here’s a sketch:
```rust
// src/manifest/name.rs
#[derive(Debug, Clone)]
pub struct ManifestName(String);
impl ManifestName {
pub fn new(s: impl Into<String>) -> Self { Self(s.into()) }
pub fn as_str(&self) -> &str { &self.0 }
}
impl Display for ManifestName {
fn fmt(&self, f: &mut Formatter<'_>) -> fmt::Result {
write!(f, "{}", self.0)
}
}// src/manifest/macros.rs
use minijinja::{
Environment,
value::{Rest, Kwargs, Value},
State, Error, ErrorKind
};
use crate::ast::MacroDefinition;
use serde_yml::Value as YamlValue;
use anyhow::Result;
use std::{sync::Arc, ptr};
pub fn parse_macro_name(sig: &str) -> Result<String> { /* ... */ }
pub fn register_macro(env: &mut Environment, def: &MacroDefinition, idx: usize) -> Result<()> { /* ... */ }
pub fn register_manifest_macros(doc: &YamlValue, env: &mut Environment) -> Result<()> { /* ... */ }
pub fn call_macro_value(
state: &State,
mv: &Value,
pos: &[Value],
kw: Option<Kwargs>,
) -> Result<Value, Error> { /* ... */ }
pub fn make_macro_fn(
tmpl: String,
name: String
) -> impl Fn(&State, Rest<Value>, Kwargs) -> Result<Value, Error> {
move |state, Rest(args), kwargs| {
/* ... */
}
}
#[derive(Debug)]
struct CallerAdapter {
inner: Value,
ptr: *const State<'static, 'static>,
}
// safety comment…
unsafe impl Send for CallerAdapter {}
unsafe impl Sync for CallerAdapter {}
impl Object for CallerAdapter {
fn call(self: &Arc<Self>, _st: &State, args: &[Value]) -> Result<Value, Error> {
let st = unsafe { &*self.ptr };
self.inner.call(st, args)
}
}Then in manifest.rs:
mod name;
mod macros;
use name::ManifestName;
use macros::register_manifest_macros;
// …later in from_str_named:
let mut jinja = Environment::new();
// …
register_manifest_macros(&doc, &mut jinja)?;This keeps each file ~50 lines, clarifies intent, and separates concerns without losing functionality.
|
|
||
| **Maintenance risk.** `serde_yml` is archived upstream and carries unsoundness | ||
| advisories. Netsuke relies on it today, but we will investigate a maintained | ||
| successor such as `serde_yaml_ng`. A follow-up ADR will outline the migration |
There was a problem hiding this comment.
suggestion (review_instructions): The phrase "we will investigate" uses a first person pronoun, which should be avoided per the instructions.
Consider rephrasing to passive voice, e.g., "A maintained successor such as serde_yaml_ng will be investigated."
Review instructions:
Path patterns: **/*.md
Instructions:
Avoid 2nd person or 1st person pronouns ("I", "you", "we")
1 participant
Summary
Testing
https://chatgpt.com/codex/tasks/task_e_68ed4ccd9de88322ac11ca4910a9c367
Summary by Sourcery
Add support for user-defined Jinja macros in Netsuke manifests by parsing a top-level
macroslist and registering each macro in the rendering environment before processing templates.New Features:
macrosfield.Enhancements:
MacroDefinitionandManifestNametypes for macro schema and diagnostics.parse_macro_name,register_macro,register_manifest_macros,make_macro_fn,call_macro_value) to integrate manifest macros into the Jinja engine.NetsukeManifestand to register them prior to template expansion.Build:
minijinjadependency to v2.12.0.Documentation:
serde_ymlcrate.Tests: