Conversation
Summary by CodeRabbit
WalkthroughExpand the codebase to support parsing and deserialising Netsuke YAML manifests into a structured Abstract Syntax Tree (AST). Add new AST data structures, update documentation, extend tests with manifest parsing scenarios, and introduce supporting dependencies. Integrate the AST module into the library’s public API and test infrastructure. Changes
Sequence Diagram(s)sequenceDiagram
participant TestRunner
participant FileSystem
participant SerdeYaml
participant AST
TestRunner->>FileSystem: Read manifest YAML file
FileSystem-->>TestRunner: Return YAML string
TestRunner->>SerdeYaml: Deserialize YAML into AST structs
SerdeYaml-->>AST: Populate AST data structures
SerdeYaml-->>TestRunner: Return AST or error
TestRunner->>TestRunner: Store AST or error in test world
Possibly related PRs
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (3)
🧰 Additional context used📓 Path-based instructions (2)**/*.rsInstructions used from: Sources: **/*.mdInstructions used from: Sources: 🧠 Learnings (1)src/ast.rs (1)🪛 LanguageTooldocs/netsuke-design.md[uncategorized] ~180-~180: Loose punctuation mark. (UNLIKELY_OPENING_PUNCTUATION) [uncategorized] ~183-~183: Loose punctuation mark. (UNLIKELY_OPENING_PUNCTUATION) 🔇 Additional comments (20)
✨ Finishing Touches
🧪 Generate unit tests
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Reviewer's GuideIntroduces initial AST support by defining serde_yml-powered data structures, exposing the AST module publicly, updating docs and dependencies accordingly, marking the roadmap entry complete, and adding both unit and behavioural tests for manifest parsing. Entity Relationship diagram for the new Netsuke manifest ASTerDiagram
NETSUKE_MANIFEST {
string netsuke_version
map vars
list rules
list steps
list targets
list defaults
}
RULE {
string name
Recipe recipe
string description
StringOrList deps
}
TARGET {
StringOrList name
Recipe recipe
StringOrList sources
StringOrList deps
StringOrList order_only_deps
map vars
bool phony
bool always
}
RECIPE {
enum kind
string command
string script
StringOrList rule
}
STRING_OR_LIST {
enum value
}
NETSUKE_MANIFEST ||--o{ RULE : contains
NETSUKE_MANIFEST ||--o{ TARGET : has_steps
NETSUKE_MANIFEST ||--o{ TARGET : has_targets
RULE }o--|| RECIPE : uses
TARGET }o--|| RECIPE : uses
TARGET }o--|| STRING_OR_LIST : uses
RECIPE }o--|| STRING_OR_LIST : uses
Class diagram for the new AST data structuresclassDiagram
class NetsukeManifest {
+Version netsuke_version
+HashMap<String, String> vars
+Vec<Rule> rules
+Vec<Target> steps
+Vec<Target> targets
+Vec<String> defaults
}
class Rule {
+String name
+Recipe recipe
+Option<String> description
+Option<StringOrList> deps
}
class Target {
+StringOrList name
+Recipe recipe
+StringOrList sources
+StringOrList deps
+StringOrList order_only_deps
+HashMap<String, String> vars
+bool phony
+bool always
}
class Recipe {
<<enumeration>>
Command(command: String)
Script(script: String)
Rule(rule: StringOrList)
}
class StringOrList {
<<enumeration>>
Empty
String(String)
List(Vec<String>)
}
NetsukeManifest "1" o-- "*" Rule
NetsukeManifest "1" o-- "*" Target : steps
NetsukeManifest "1" o-- "*" Target : targets
Rule "1" o-- "1" Recipe
Target "1" o-- "1" Recipe
Target "1" o-- "1" StringOrList : name
Target "1" o-- "1" StringOrList : sources
Target "1" o-- "1" StringOrList : deps
Target "1" o-- "1" StringOrList : order_only_deps
Recipe "1" o-- "1" StringOrList : rule
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![codescene-delta-analysis[bot]](https://avatars.githubusercontent.com/in/53921?s=60&v=4){kind=small}
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @leynos - I've reviewed your changes and found some issues that need to be addressed.
- Consider using StringOrList for the Rule.deps field so that it can accept both single and multiple dependencies and stay consistent with Target deps.
- It might be worth parsing netsuke_version into a semantic version type (e.g. via the semver crate) instead of a raw String to catch invalid version formats early.
- The manifest.vars map uses serde_yaml::Value while Target.vars is HashMap<String,String>—aligning these types could simplify variable handling and validation.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Consider using StringOrList for the Rule.deps field so that it can accept both single and multiple dependencies and stay consistent with Target deps.
- It might be worth parsing netsuke_version into a semantic version type (e.g. via the semver crate) instead of a raw String to catch invalid version formats early.
- The manifest.vars map uses serde_yaml::Value while Target.vars is HashMap<String,String>—aligning these types could simplify variable handling and validation.
## Individual Comments
### Comment 1
<location> `src/ast.rs:23` </location>
<code_context>
+ pub rules: Vec<Rule>,
+
+ #[serde(default)]
+ pub actions: Vec<Target>,
+
+ pub targets: Vec<Target>,
</code_context>
<issue_to_address>
The 'actions' field may overlap conceptually with 'targets', potentially causing confusion.
Consider clarifying the distinction between 'actions' and 'targets' or renaming them for better clarity.
Suggested implementation:
```rust
#[serde(default)]
pub steps: Vec<Target>,
pub targets: Vec<Target>,
```
```rust
#[serde(default)]
pub steps: Vec<Target>,
pub targets: Vec<Target>,
```
You will also need to update any code that references the `actions` field elsewhere in the codebase (e.g., deserialization, usage in logic, tests) to use the new `steps` name.
If the distinction between `steps` and `targets` is still unclear, consider adding documentation comments to each field explaining their purpose.
</issue_to_address>
### Comment 2
<location> `tests/ast_tests.rs:6` </location>
<code_context>
+use netsuke::ast::*;
+use rstest::rstest;
+
+#[rstest]
+fn parse_minimal_manifest() {
+ let yaml = r#"
+ netsuke_version: "1.0"
+ targets:
+ - name: hello
+ recipe:
+ kind: command
+ command: "echo hi"
+ "#;
+
+ let manifest: NetsukeManifest = serde_yaml::from_str(yaml).expect("parse");
+
+ assert_eq!(manifest.netsuke_version, "1.0");
+ let first = manifest.targets.first().expect("target");
+ if let StringOrList::String(name) = &first.name {
+ assert_eq!(name, "hello");
+ } else {
+ panic!("target name should be String");
+ }
+}
</code_context>
<issue_to_address>
Unit tests only cover minimal manifest; edge cases are missing.
Please add tests for edge cases, including missing required fields, unknown fields, empty lists/maps, all `StringOrList` variants, presence/absence of optional fields, and invalid enum variants. This will improve deserialization robustness and error handling coverage.
Suggested implementation:
```rust
#[rstest]
fn parse_minimal_manifest() {
let yaml = r#"
netsuke_version: "1.0"
targets:
- name: hello
recipe:
kind: command
command: "echo hi"
"#;
let manifest: NetsukeManifest = serde_yaml::from_str(yaml).expect("parse");
assert_eq!(manifest.netsuke_version, "1.0");
let first = manifest.targets.first().expect("target");
if let StringOrList::String(name) = &first.name {
assert_eq!(name, "hello");
} else {
panic!("target name should be String");
}
}
#[test]
fn missing_required_fields() {
// Missing netsuke_version
let yaml = r#"
targets:
- name: hello
recipe:
kind: command
command: "echo hi"
"#;
assert!(serde_yaml::from_str::<NetsukeManifest>(yaml).is_err());
// Missing targets
let yaml = r#"
netsuke_version: "1.0"
"#;
assert!(serde_yaml::from_str::<NetsukeManifest>(yaml).is_err());
// Missing name in target
let yaml = r#"
netsuke_version: "1.0"
targets:
- recipe:
kind: command
command: "echo hi"
"#;
assert!(serde_yaml::from_str::<NetsukeManifest>(yaml).is_err());
}
#[test]
fn unknown_fields() {
// Unknown field at root
let yaml = r#"
netsuke_version: "1.0"
targets:
- name: hello
recipe:
kind: command
command: "echo hi"
extra_field: 42
"#;
assert!(serde_yaml::from_str::<NetsukeManifest>(yaml).is_err());
// Unknown field in target
let yaml = r#"
netsuke_version: "1.0"
targets:
- name: hello
recipe:
kind: command
command: "echo hi"
unexpected: true
"#;
assert!(serde_yaml::from_str::<NetsukeManifest>(yaml).is_err());
}
#[test]
fn empty_lists_and_maps() {
// Empty targets list
let yaml = r#"
netsuke_version: "1.0"
targets: []
"#;
let manifest = serde_yaml::from_str::<NetsukeManifest>(yaml).expect("parse");
assert!(manifest.targets.is_empty());
// Empty recipe (should fail if recipe is required)
let yaml = r#"
netsuke_version: "1.0"
targets:
- name: hello
recipe: {}
"#;
assert!(serde_yaml::from_str::<NetsukeManifest>(yaml).is_err());
}
#[test]
fn string_or_list_variants() {
// String variant
let yaml = r#"
netsuke_version: "1.0"
targets:
- name: hello
recipe:
kind: command
command: "echo hi"
"#;
let manifest = serde_yaml::from_str::<NetsukeManifest>(yaml).expect("parse");
if let StringOrList::String(name) = &manifest.targets[0].name {
assert_eq!(name, "hello");
} else {
panic!("Expected String variant");
}
// List variant
let yaml = r#"
netsuke_version: "1.0"
targets:
- name:
- hello
- world
recipe:
kind: command
command: "echo hi"
"#;
let manifest = serde_yaml::from_str::<NetsukeManifest>(yaml).expect("parse");
if let StringOrList::List(names) = &manifest.targets[0].name {
assert_eq!(names, &vec!["hello".to_string(), "world".to_string()]);
} else {
panic!("Expected List variant");
}
// Empty list variant
let yaml = r#"
netsuke_version: "1.0"
targets:
- name: []
recipe:
kind: command
command: "echo hi"
"#;
let manifest = serde_yaml::from_str::<NetsukeManifest>(yaml).expect("parse");
if let StringOrList::List(names) = &manifest.targets[0].name {
assert!(names.is_empty());
} else {
panic!("Expected List variant");
}
}
#[test]
fn optional_fields() {
// If there are optional fields in the manifest, test their presence/absence.
// Example: assume 'description' is optional in target.
let yaml = r#"
netsuke_version: "1.0"
targets:
- name: hello
description: "A target"
recipe:
kind: command
command: "echo hi"
"#;
let manifest = serde_yaml::from_str::<NetsukeManifest>(yaml).expect("parse");
// If description is Option<String>, check it
if let Some(desc) = &manifest.targets[0].description {
assert_eq!(desc, "A target");
}
// Absence of optional field
let yaml = r#"
netsuke_version: "1.0"
targets:
- name: hello
recipe:
kind: command
command: "echo hi"
"#;
let manifest = serde_yaml::from_str::<NetsukeManifest>(yaml).expect("parse");
assert!(manifest.targets[0].description.is_none());
}
#[test]
fn invalid_enum_variants() {
// Invalid kind in recipe
let yaml = r#"
netsuke_version: "1.0"
targets:
- name: hello
recipe:
kind: not_a_kind
command: "echo hi"
"#;
assert!(serde_yaml::from_str::<NetsukeManifest>(yaml).is_err());
}
```
- If the `description` field or other optional fields do not exist in your schema, remove or adapt the relevant test code.
- If your `StringOrList` or enum types differ, adjust the test YAML and assertions accordingly.
- If your error handling is not strict (e.g., unknown fields are ignored), you may need to adjust the assertions for those tests.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| pub rules: Vec<Rule>, | ||
|
|
||
| #[serde(default)] | ||
| pub actions: Vec<Target>, |
There was a problem hiding this comment.
suggestion: The 'actions' field may overlap conceptually with 'targets', potentially causing confusion.
Consider clarifying the distinction between 'actions' and 'targets' or renaming them for better clarity.
Suggested implementation:
#[serde(default)]
pub steps: Vec<Target>,
pub targets: Vec<Target>, #[serde(default)]
pub steps: Vec<Target>,
pub targets: Vec<Target>,You will also need to update any code that references the actions field elsewhere in the codebase (e.g., deserialization, usage in logic, tests) to use the new steps name.
If the distinction between steps and targets is still unclear, consider adding documentation comments to each field explaining their purpose.
| #[rstest] | ||
| fn parse_minimal_manifest() { | ||
| let yaml = r#" | ||
| netsuke_version: "1.0" | ||
| targets: | ||
| - name: hello | ||
| recipe: | ||
| kind: command | ||
| command: "echo hi" | ||
| "#; |
There was a problem hiding this comment.
suggestion (testing): Unit tests only cover minimal manifest; edge cases are missing.
Please add tests for edge cases, including missing required fields, unknown fields, empty lists/maps, all StringOrList variants, presence/absence of optional fields, and invalid enum variants. This will improve deserialization robustness and error handling coverage.
Suggested implementation:
#[rstest]
fn parse_minimal_manifest() {
let yaml = r#"
netsuke_version: "1.0"
targets:
- name: hello
recipe:
kind: command
command: "echo hi"
"#;
let manifest: NetsukeManifest = serde_yaml::from_str(yaml).expect("parse");
assert_eq!(manifest.netsuke_version, "1.0");
let first = manifest.targets.first().expect("target");
if let StringOrList::String(name) = &first.name {
assert_eq!(name, "hello");
} else {
panic!("target name should be String");
}
}
#[test]
fn missing_required_fields() {
// Missing netsuke_version
let yaml = r#"
targets:
- name: hello
recipe:
kind: command
command: "echo hi"
"#;
assert!(serde_yaml::from_str::<NetsukeManifest>(yaml).is_err());
// Missing targets
let yaml = r#"
netsuke_version: "1.0"
"#;
assert!(serde_yaml::from_str::<NetsukeManifest>(yaml).is_err());
// Missing name in target
let yaml = r#"
netsuke_version: "1.0"
targets:
- recipe:
kind: command
command: "echo hi"
"#;
assert!(serde_yaml::from_str::<NetsukeManifest>(yaml).is_err());
}
#[test]
fn unknown_fields() {
// Unknown field at root
let yaml = r#"
netsuke_version: "1.0"
targets:
- name: hello
recipe:
kind: command
command: "echo hi"
extra_field: 42
"#;
assert!(serde_yaml::from_str::<NetsukeManifest>(yaml).is_err());
// Unknown field in target
let yaml = r#"
netsuke_version: "1.0"
targets:
- name: hello
recipe:
kind: command
command: "echo hi"
unexpected: true
"#;
assert!(serde_yaml::from_str::<NetsukeManifest>(yaml).is_err());
}
#[test]
fn empty_lists_and_maps() {
// Empty targets list
let yaml = r#"
netsuke_version: "1.0"
targets: []
"#;
let manifest = serde_yaml::from_str::<NetsukeManifest>(yaml).expect("parse");
assert!(manifest.targets.is_empty());
// Empty recipe (should fail if recipe is required)
let yaml = r#"
netsuke_version: "1.0"
targets:
- name: hello
recipe: {}
"#;
assert!(serde_yaml::from_str::<NetsukeManifest>(yaml).is_err());
}
#[test]
fn string_or_list_variants() {
// String variant
let yaml = r#"
netsuke_version: "1.0"
targets:
- name: hello
recipe:
kind: command
command: "echo hi"
"#;
let manifest = serde_yaml::from_str::<NetsukeManifest>(yaml).expect("parse");
if let StringOrList::String(name) = &manifest.targets[0].name {
assert_eq!(name, "hello");
} else {
panic!("Expected String variant");
}
// List variant
let yaml = r#"
netsuke_version: "1.0"
targets:
- name:
- hello
- world
recipe:
kind: command
command: "echo hi"
"#;
let manifest = serde_yaml::from_str::<NetsukeManifest>(yaml).expect("parse");
if let StringOrList::List(names) = &manifest.targets[0].name {
assert_eq!(names, &vec!["hello".to_string(), "world".to_string()]);
} else {
panic!("Expected List variant");
}
// Empty list variant
let yaml = r#"
netsuke_version: "1.0"
targets:
- name: []
recipe:
kind: command
command: "echo hi"
"#;
let manifest = serde_yaml::from_str::<NetsukeManifest>(yaml).expect("parse");
if let StringOrList::List(names) = &manifest.targets[0].name {
assert!(names.is_empty());
} else {
panic!("Expected List variant");
}
}
#[test]
fn optional_fields() {
// If there are optional fields in the manifest, test their presence/absence.
// Example: assume 'description' is optional in target.
let yaml = r#"
netsuke_version: "1.0"
targets:
- name: hello
description: "A target"
recipe:
kind: command
command: "echo hi"
"#;
let manifest = serde_yaml::from_str::<NetsukeManifest>(yaml).expect("parse");
// If description is Option<String>, check it
if let Some(desc) = &manifest.targets[0].description {
assert_eq!(desc, "A target");
}
// Absence of optional field
let yaml = r#"
netsuke_version: "1.0"
targets:
- name: hello
recipe:
kind: command
command: "echo hi"
"#;
let manifest = serde_yaml::from_str::<NetsukeManifest>(yaml).expect("parse");
assert!(manifest.targets[0].description.is_none());
}
#[test]
fn invalid_enum_variants() {
// Invalid kind in recipe
let yaml = r#"
netsuke_version: "1.0"
targets:
- name: hello
recipe:
kind: not_a_kind
command: "echo hi"
"#;
assert!(serde_yaml::from_str::<NetsukeManifest>(yaml).is_err());
}- If the
descriptionfield or other optional fields do not exist in your schema, remove or adapt the relevant test code. - If your
StringOrListor enum types differ, adjust the test YAML and assertions accordingly. - If your error handling is not strict (e.g., unknown fields are ignored), you may need to adjust the assertions for those tests.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
🔭 Outside diff range comments (1)
src/lib.rs (1)
1-4: Update outdated module documentation.The module documentation stating the library "only exposes the command line interface definitions" is now incorrect with the addition of the ast module.
-//! Currently this library only exposes the command line interface -//! definitions used by the binary and tests. +//! This library exposes the command line interface definitions and +//! Abstract Syntax Tree (AST) structures for manifest parsing.
📜 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 (11)
Cargo.toml(1 hunks)docs/netsuke-design.md(1 hunks)docs/roadmap.md(1 hunks)src/ast.rs(1 hunks)src/lib.rs(1 hunks)tests/ast_tests.rs(1 hunks)tests/cucumber.rs(1 hunks)tests/data/minimal.yml(1 hunks)tests/features/manifest.feature(1 hunks)tests/steps/manifest_steps.rs(1 hunks)tests/steps/mod.rs(1 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
Cargo.toml
Instructions used from:
Sources:
📄 CodeRabbit Inference Engine
- AGENTS.md
docs/**/*.md
Instructions used from:
Sources:
📄 CodeRabbit Inference Engine
- AGENTS.md
**/*.md
Instructions used from:
Sources:
📄 CodeRabbit Inference Engine
- AGENTS.md
⚙️ CodeRabbit Configuration File
**/*.rs
Instructions used from:
Sources:
📄 CodeRabbit Inference Engine
- AGENTS.md
⚙️ CodeRabbit Configuration File
🧠 Learnings (1)
src/ast.rs (1)
Learnt from: CR
PR: leynos/netsuke#0
File: docs/roadmap.md:0-0
Timestamp: 2025-07-18T19:04:19.414Z
Learning: Applies to docs/src/ast.rs : Define the core Abstract Syntax Tree (AST) data structures (NetsukeManifest, Rule, Target, StringOrList, Recipe) in src/ast.rs.
🧬 Code Graph Analysis (1)
src/ast.rs (1)
tests/steps/manifest_steps.rs (1)
serde_yaml(11-11)
🪛 LanguageTool
docs/netsuke-design.md
[uncategorized] ~557-~557: Use a comma before ‘so’ if it connects two independent clauses (unless they are closely connected and short).
Context: ...Listprovides a defaultEmpty` variant so optional lists are trivial to represent...
(COMMA_COMPOUND_SENTENCE_2)
🔇 Additional comments (12)
Cargo.toml (1)
8-8: Well-chosen serde configuration.The serde dependency with derive feature is appropriately configured for AST deserialization needs.
tests/steps/mod.rs (1)
2-2: Clean module organization.The addition of the manifest_steps module follows the established pattern and maintains clear separation of test step concerns.
docs/roadmap.md (1)
19-21: Accurate roadmap progress tracking.The completion status correctly reflects the implemented AST data structures, maintaining documentation accuracy with the actual codebase state.
tests/features/manifest.feature (1)
1-6: Well-structured behaviour test.The feature test follows proper BDD structure with clear scenario definition and concrete assertions for manifest parsing verification.
src/lib.rs (1)
6-6: Proper module exposure for AST functionality.The public ast module declaration correctly exposes the AST data structures for use in tests and CLI integration.
tests/data/minimal.yml (1)
1-6: LGTM! Clean minimal test fixture.This test fixture correctly implements the basic manifest structure with the required
netsuke_versionfield and a simple target definition. The YAML syntax is valid and the structure aligns with the design documentation.tests/cucumber.rs (1)
7-8: LGTM! Proper extension of test state.The addition of
manifestandmanifest_errorfields correctly extends the shared test state to support manifest parsing scenarios. TheOptionwrapping enables proper error handling in Cucumber steps.tests/ast_tests.rs (1)
1-2: LGTM! Proper module documentation.The module-level documentation clearly states the purpose. Note the correct en-GB spelling of "deserialisation".
tests/steps/manifest_steps.rs (2)
1-2: LGTM! Clear module documentation.The module purpose is clearly stated and follows the required documentation standards.
23-31: LGTM! Proper Clippy warning handling.The
#[expect]attribute is correctly scoped with a clear reason. The step implementation is straightforward and correct.src/ast.rs (2)
1-5: Excellent module documentation.The module documentation clearly explains the purpose and utility, properly references the design document, and follows the required //! format.
7-8: Clean and necessary imports.The imports are minimal and all used appropriately in the code below.
| ### 3.4 Design Decisions | ||
|
|
||
| The AST structures are implemented in `src/ast.rs` and derive `Deserialize`. | ||
| Unknown fields are rejected to surface user errors early. `StringOrList` | ||
| provides a default `Empty` variant so optional lists are trivial to represent. | ||
| This keeps YAML manifests concise while ensuring forward compatibility. |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Fix grammar and expand design rationale.
The design decisions section provides valuable context. However, address the grammatical issue and consider expanding the rationale.
Apply this fix for the comma usage:
-Unknown fields are rejected to surface user errors early. `StringOrList`
-provides a default `Empty` variant so optional lists are trivial to represent.
+Unknown fields are rejected to surface user errors early. `StringOrList`
+provides a default `Empty` variant, so optional lists are trivial to represent.Consider expanding on why strict field validation is beneficial for user experience and how the design choices support forward compatibility.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| ### 3.4 Design Decisions | |
| The AST structures are implemented in `src/ast.rs` and derive `Deserialize`. | |
| Unknown fields are rejected to surface user errors early. `StringOrList` | |
| provides a default `Empty` variant so optional lists are trivial to represent. | |
| This keeps YAML manifests concise while ensuring forward compatibility. | |
| ### 3.4 Design Decisions | |
| The AST structures are implemented in `src/ast.rs` and derive `Deserialize`. | |
| Unknown fields are rejected to surface user errors early. `StringOrList` | |
| provides a default `Empty` variant, so optional lists are trivial to represent. | |
| This keeps YAML manifests concise while ensuring forward compatibility. |
🧰 Tools
🪛 LanguageTool
[uncategorized] ~557-~557: Use a comma before ‘so’ if it connects two independent clauses (unless they are closely connected and short).
Context: ...Listprovides a defaultEmpty` variant so optional lists are trivial to represent...
(COMMA_COMPOUND_SENTENCE_2)
🤖 Prompt for AI Agents
In docs/netsuke-design.md around lines 553 to 558, fix the grammatical issue by
correcting comma usage in the design decisions paragraph. Expand the rationale
by explaining that strict field validation helps catch user errors early,
improving user experience, and clarify how using a default Empty variant in
StringOrList supports forward compatibility by allowing optional lists without
breaking existing manifests. Make the explanation clear and concise to enhance
reader understanding.
| #[then(expr = "the first target name is {string}")] | ||
| fn first_target_name(world: &mut CliWorld, name: String) { | ||
| let manifest = world.manifest.as_ref().expect("manifest"); | ||
| let first = manifest.targets.first().expect("targets"); | ||
| match &first.name { | ||
| StringOrList::String(value) => assert_eq!(value, &name), | ||
| other => panic!("unexpected variant {other:?}"), | ||
| } | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Enhance error message consistency.
The step implementation works correctly but improve error message format for consistency.
Apply this improvement:
#[then(expr = "the first target name is {string}")]
fn first_target_name(world: &mut CliWorld, name: String) {
let manifest = world.manifest.as_ref().expect("manifest");
let first = manifest.targets.first().expect("targets");
match &first.name {
StringOrList::String(value) => assert_eq!(value, &name),
- other => panic!("unexpected variant {other:?}"),
+ other => panic!("Expected target name to be StringOrList::String, got: {other:?}"),
}
}This provides clearer error context when the assertion fails.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| #[then(expr = "the first target name is {string}")] | |
| fn first_target_name(world: &mut CliWorld, name: String) { | |
| let manifest = world.manifest.as_ref().expect("manifest"); | |
| let first = manifest.targets.first().expect("targets"); | |
| match &first.name { | |
| StringOrList::String(value) => assert_eq!(value, &name), | |
| other => panic!("unexpected variant {other:?}"), | |
| } | |
| } | |
| #[then(expr = "the first target name is {string}")] | |
| fn first_target_name(world: &mut CliWorld, name: String) { | |
| let manifest = world.manifest.as_ref().expect("manifest"); | |
| let first = manifest.targets.first().expect("targets"); | |
| match &first.name { | |
| StringOrList::String(value) => assert_eq!(value, &name), | |
| other => panic!("Expected target name to be StringOrList::String, got: {other:?}"), | |
| } | |
| } |
🤖 Prompt for AI Agents
In tests/steps/manifest_steps.rs around lines 37 to 45, improve the error
message consistency by updating the panic message in the match arm to a clearer
and more consistent format. Replace the current panic message with one that
explicitly states the unexpected variant encountered, ensuring it matches the
style used elsewhere for error messages in this file.
| /// A helper for fields that accept either a string or list of strings. | ||
| #[derive(Debug, Deserialize, Default)] | ||
| #[serde(untagged)] | ||
| pub enum StringOrList { | ||
| #[default] | ||
| Empty, | ||
| String(String), | ||
| List(Vec<String>), | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add comprehensive documentation with usage examples.
The StringOrList enum needs detailed /// documentation explaining its purpose and showing how it handles different YAML inputs.
Apply this diff to add proper documentation:
-/// A helper for fields that accept either a string or list of strings.
+/// A helper for fields that accept either a string or list of strings.
+///
+/// This enum allows manifest fields to accept flexible YAML syntax,
+/// automatically deserialising both single strings and string arrays
+/// into a unified type.
+///
+/// # Examples
+///
+/// ## Single string input
+/// ```
+/// use netsuke::ast::StringOrList;
+/// use serde_yaml;
+///
+/// let yaml = r#""single-item""#;
+/// let result: StringOrList = serde_yaml::from_str(yaml).unwrap();
+/// ```
+///
+/// ## List input
+/// ```
+/// use netsuke::ast::StringOrList;
+/// use serde_yaml;
+///
+/// let yaml = r#"["item1", "item2", "item3"]"#;
+/// let result: StringOrList = serde_yaml::from_str(yaml).unwrap();
+/// ```
+///
+/// ## Empty default
+/// ```
+/// use netsuke::ast::StringOrList;
+///
+/// let empty = StringOrList::default();
+/// assert!(matches!(empty, StringOrList::Empty));
+/// ```📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| /// A helper for fields that accept either a string or list of strings. | |
| #[derive(Debug, Deserialize, Default)] | |
| #[serde(untagged)] | |
| pub enum StringOrList { | |
| #[default] | |
| Empty, | |
| String(String), | |
| List(Vec<String>), | |
| } | |
| /// A helper for fields that accept either a string or list of strings. | |
| /// | |
| /// This enum allows manifest fields to accept flexible YAML syntax, | |
| /// automatically deserialising both single strings and string arrays | |
| /// into a unified type. | |
| /// | |
| /// # Examples | |
| /// | |
| /// ## Single string input | |
| /// |
🤖 Prompt for AI Agents
In src/ast.rs around lines 79 to 87, the StringOrList enum lacks detailed
documentation and usage examples. Add comprehensive /// doc comments above the
enum definition explaining its purpose as a helper for fields accepting either a
string or list of strings. Include example code snippets demonstrating
deserialization from YAML for a single string, a list of strings, and the
default empty variant, using serde_yaml and showing how to instantiate and match
the enum variants.
| /// Execution style for rules and targets. | ||
| #[derive(Debug, Deserialize)] | ||
| #[serde(tag = "kind", rename_all = "lowercase")] | ||
| pub enum Recipe { | ||
| #[serde(alias = "command")] | ||
| Command { command: String }, | ||
| #[serde(alias = "script")] | ||
| Script { script: String }, | ||
| #[serde(alias = "rule")] | ||
| Rule { rule: StringOrList }, | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add comprehensive documentation with examples for each variant.
The Recipe enum needs detailed /// documentation explaining each variant and their use cases, with examples as required for public APIs.
Apply this diff to add proper documentation:
-/// Execution style for rules and targets.
+/// Execution style for rules and targets.
+///
+/// Defines how a rule or target should be executed, supporting different
+/// execution models from simple commands to complex scripts or rule references.
+///
+/// # Examples
+///
+/// ## Command variant
+/// ```
+/// use netsuke::ast::Recipe;
+/// use serde_yaml;
+///
+/// let yaml = r#"
+/// kind: command
+/// command: "cargo build --release"
+/// "#;
+/// let recipe: Recipe = serde_yaml::from_str(yaml).unwrap();
+/// ```
+///
+/// ## Script variant
+/// ```
+/// use netsuke::ast::Recipe;
+/// use serde_yaml;
+///
+/// let yaml = r#"
+/// kind: script
+/// script: |
+/// echo "Building..."
+/// cargo build
+/// "#;
+/// let recipe: Recipe = serde_yaml::from_str(yaml).unwrap();
+/// ```
+///
+/// ## Rule reference variant
+/// ```
+/// use netsuke::ast::Recipe;
+/// use serde_yaml;
+///
+/// let yaml = r#"
+/// kind: rule
+/// rule: "compile"
+/// "#;
+/// let recipe: Recipe = serde_yaml::from_str(yaml).unwrap();
+/// ```🤖 Prompt for AI Agents
In src/ast.rs around lines 41 to 51, the Recipe enum lacks detailed
documentation explaining each variant and their use cases. Add comprehensive ///
doc comments above the enum and each variant, including usage examples for
Command, Script, and Rule variants using serde_yaml deserialization snippets as
shown in the provided diff. This will improve clarity and usability for public
API consumers.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (10)
Cargo.toml (1)
8-10: Replace deprecated serde_yaml dependency immediately.The serde_yaml version "0.9" is deprecated and potentially insecure. This dependency must be updated to a maintained alternative.
Switch to the
serde_ymlcrate which is the maintained successor:-serde_yaml = "0.9" +serde_yml = "0.11"Update all imports in the codebase from
serde_yamltoserde_yml.tests/ast_tests.rs (1)
7-30: Enhance test robustness and error messages.The test validates core functionality but needs improved error handling and additional assertions.
Apply these improvements:
#[rstest] fn parse_minimal_manifest() { let yaml = r#" - netsuke_version: "1.0.0" - targets: - - name: hello - recipe: - kind: command - command: "echo hi" +netsuke_version: "1.0.0" +targets: + - name: hello + recipe: + kind: command + command: "echo hi" "#; let manifest: NetsukeManifest = serde_yaml::from_str(yaml).expect("parse"); assert_eq!( manifest.netsuke_version, Version::parse("1.0.0").expect("ver") ); let first = manifest.targets.first().expect("target"); - if let StringOrList::String(name) = &first.name { - assert_eq!(name, "hello"); - } else { - panic!("target name should be String"); - } + let StringOrList::String(name) = &first.name else { + panic!("Expected target name to be StringOrList::String, got: {:?}", first.name); + }; + assert_eq!(name, "hello"); + + // Verify recipe fields + if let Recipe::Command { command } = &first.recipe { + assert_eq!(command, "echo hi"); + } else { + panic!("Expected command recipe, got: {:?}", first.recipe); + }tests/steps/manifest_steps.rs (2)
8-21: Improve error handling for file operations.The step implementation uses
expect()which panics on file read errors. Replace with proper error handling to distinguish between file I/O errors and parsing errors.
37-45: Enhance error message consistency.The panic message could be clearer about expectations. Update to explicitly state what variant was expected.
docs/netsuke-design.md (1)
541-550: Fix grammar in design decisions section.Add a comma before "so" to connect the independent clauses properly.
src/ast.rs (5)
11-31: Add comprehensive Rustdoc documentation for the public API.The struct needs detailed
///documentation explaining each field and providing usage examples as required by the coding guidelines.
33-41: Add comprehensive documentation with examples.The
Rulestruct requires detailed///documentation explaining its purpose and providing usage examples for the public API.
43-53: Add comprehensive documentation with examples for each variant.The
Recipeenum needs detailed///documentation explaining each variant and their use cases, with examples as required for public APIs.
55-79: Add comprehensive documentation with field explanations.The
Targetstruct needs detailed///documentation explaining each field's purpose and providing examples as required for public APIs.
81-89: Add comprehensive documentation with usage examples.The
StringOrListenum needs detailed///documentation explaining its purpose and showing how it handles different YAML inputs.
📜 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 (8)
Cargo.toml(1 hunks)docs/netsuke-design.md(6 hunks)docs/roadmap.md(2 hunks)src/ast.rs(1 hunks)tests/ast_tests.rs(1 hunks)tests/data/minimal.yml(1 hunks)tests/features/manifest.feature(1 hunks)tests/steps/manifest_steps.rs(1 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
Cargo.toml
Instructions used from:
Sources:
📄 CodeRabbit Inference Engine
- AGENTS.md
docs/**/*.md
Instructions used from:
Sources:
📄 CodeRabbit Inference Engine
- AGENTS.md
**/*.md
Instructions used from:
Sources:
📄 CodeRabbit Inference Engine
- AGENTS.md
⚙️ CodeRabbit Configuration File
**/*.rs
Instructions used from:
Sources:
📄 CodeRabbit Inference Engine
- AGENTS.md
⚙️ CodeRabbit Configuration File
🧠 Learnings (1)
src/ast.rs (1)
Learnt from: CR
PR: leynos/netsuke#0
File: docs/roadmap.md:0-0
Timestamp: 2025-07-18T19:04:19.414Z
Learning: Applies to docs/src/ast.rs : Define the core Abstract Syntax Tree (AST) data structures (NetsukeManifest, Rule, Target, StringOrList, Recipe) in src/ast.rs.
🪛 LanguageTool
docs/netsuke-design.md
[uncategorized] ~180-~180: Loose punctuation mark.
Context: ... Ninja build statement.[^3] - steps: A secondary list of build targets. Any ...
(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~183-~183: Loose punctuation mark.
Context: ...ways: false }by default. -defaults`: An optional list of target names to be ...
(UNLIKELY_OPENING_PUNCTUATION)
[style] ~187-~187: Would you like to use the Oxford spelling “summarizes”? The spelling ‘summarises’ is also correct.
Context: ...t statement.[^3] The E-R diagram below summarises the structure of a Netsukefile and th...
(OXFORD_SPELLING_Z_NOT_S)
[uncategorized] ~545-~545: Use a comma before ‘so’ if it connects two independent clauses (unless they are closely connected and short).
Context: ...Listprovides a defaultEmpty` variant so optional lists are trivial to represent...
(COMMA_COMPOUND_SENTENCE_2)
🔇 Additional comments (16)
tests/features/manifest.feature (1)
1-6: LGTM - Clean Cucumber feature definition.The feature file correctly defines a focused end-to-end test for manifest parsing functionality using proper Gherkin syntax.
tests/data/minimal.yml (1)
1-6: Well-structured test fixture.The minimal YAML manifest provides an appropriate baseline test case with correct semantic versioning and simple command recipe structure.
docs/roadmap.md (2)
19-21: Accurate roadmap progress tracking.The completion marking and terminology update correctly reflect the implemented AST structures and maintain consistency across project documentation.
32-33: Terminology consistency maintained.The change from "actions list" to "steps list" aligns with the updated manifest schema terminology.
tests/ast_tests.rs (7)
1-5: Comprehensive test module structure.The module documentation and imports are well-organised, providing clear context for AST deserialisation testing.
32-56: Thorough validation of required field enforcement.The test correctly verifies that serde fails when required fields are missing, ensuring robust deserialisation.
58-81: Proper unknown field rejection testing.The test validates that
deny_unknown_fieldsis working correctly at both root and target levels.
83-99: Good edge case coverage for empty structures.The test appropriately handles empty lists and validates that empty recipe objects are rejected.
101-152: Comprehensive StringOrList variant testing.The test thoroughly covers all variants of the StringOrList enum including edge cases like empty lists.
154-197: Effective optional field validation.The test properly verifies both presence and absence of optional fields in the AST structures.
199-210: Appropriate enum validation testing.The test ensures invalid enum variants are properly rejected during deserialisation.
tests/steps/manifest_steps.rs (1)
23-31: Well-structured test step with appropriate lint handling.The lint suppression is properly justified, and the implementation correctly validates the manifest version.
docs/netsuke-design.md (4)
160-162: Version format correctly aligned with semantic versioning.The change from
"1.0"to"1.0.0"properly reflects the use of thesemvercrate for version validation.
180-183: Documentation correctly updated to reflect AST changes.The rename from
actionstostepsis properly documented and aligns with the implementation.
191-232: E-R diagram accurately represents the AST structure.The diagram correctly shows the relationships between manifest components and aligns with the implementation in
src/ast.rs.
415-439: AST documentation correctly updated.The field updates accurately reflect the implementation:
actions→stepsrenamedepstype changed toOption<StringOrList>for consistency
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (4)
src/ast.rs (4)
72-77: Add comprehensive documentation with examples.The
Rulestruct needs more detailed documentation explaining its purpose and showing usage examples, as previously suggested.Add this enhanced documentation:
/// A reusable command template. /// /// A rule encapsulates a snippet of work that can be referenced by multiple /// targets. It may define a command line, a script block, or delegate to another /// named rule. Dependencies are expressed as a whitespace separated list. +/// +/// # Examples +/// +/// ```rust +/// use netsuke::ast::{Rule, Recipe}; +/// use serde_yml; +/// +/// let yaml = r#" +/// name: "compile" +/// recipe: +/// kind: command +/// command: "gcc -c $<" +/// description: "Compile C source files" +/// "#; +/// +/// let rule: Rule = serde_yml::from_str(yaml).unwrap(); +/// assert_eq!(rule.name, "compile"); +/// ```
90-94: Add examples for each Recipe variant.The
Recipeenum needs examples demonstrating each execution style, as previously suggested.Enhance the documentation:
/// Execution style for rules and targets. /// /// The variant is selected using the `kind` field in the manifest. Each variant /// corresponds to a different way of specifying how a command should run. +/// +/// # Examples +/// +/// ## Command variant +/// ```rust +/// use netsuke::ast::Recipe; +/// use serde_yml; +/// +/// let yaml = r#" +/// kind: command +/// command: "cargo build --release" +/// "#; +/// let recipe: Recipe = serde_yml::from_str(yaml).unwrap(); +/// ``` +/// +/// ## Script variant +/// ```rust +/// use netsuke::ast::Recipe; +/// use serde_yml; +/// +/// let yaml = r#" +/// kind: script +/// script: | +/// echo "Building..." +/// cargo build +/// "#; +/// let recipe: Recipe = serde_yml::from_str(yaml).unwrap(); +/// ``` +/// +/// ## Rule reference variant +/// ```rust +/// use netsuke::ast::Recipe; +/// use serde_yml; +/// +/// let yaml = r#" +/// kind: rule +/// rule: "compile" +/// "#; +/// let recipe: Recipe = serde_yml::from_str(yaml).unwrap(); +/// ```
108-113: Add usage examples to Target documentation.The
Targetstruct would benefit from examples showing typical usage patterns.Add this example to the documentation:
/// Targets describe the files produced by a rule and their dependencies. /// `phony` targets are always considered out of date, while `always` targets are /// regenerated even if their inputs are unchanged. +/// +/// # Examples +/// +/// ```rust +/// use netsuke::ast::{Target, Recipe, StringOrList}; +/// use serde_yml; +/// +/// let yaml = r#" +/// name: "build" +/// recipe: +/// kind: command +/// command: "cargo build" +/// sources: ["src/**/*.rs"] +/// phony: true +/// "#; +/// +/// let target: Target = serde_yml::from_str(yaml).unwrap(); +/// assert!(target.phony); +/// ```
146-160: Add Rust usage examples for StringOrList.The enum documentation should include Rust examples demonstrating deserialisation, as previously suggested.
Enhance the documentation:
/// ```yaml /// # Scalar /// name: hello /// # Sequence /// name: /// - hello /// - world /// ``` +/// +/// # Examples +/// +/// ## Single string input +/// ```rust +/// use netsuke::ast::StringOrList; +/// use serde_yml; +/// +/// let yaml = r#""single-item""#; +/// let result: StringOrList = serde_yml::from_str(yaml).unwrap(); +/// assert!(matches!(result, StringOrList::String(_))); +/// ``` +/// +/// ## List input +/// ```rust +/// use netsuke::ast::StringOrList; +/// use serde_yml; +/// +/// let yaml = r#"["item1", "item2", "item3"]"#; +/// let result: StringOrList = serde_yml::from_str(yaml).unwrap(); +/// assert!(matches!(result, StringOrList::List(_))); +/// ``` +/// +/// ## Empty default +/// ```rust +/// use netsuke::ast::StringOrList; +/// +/// let empty = StringOrList::default(); +/// assert!(matches!(empty, StringOrList::Empty)); +/// ```
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (6)
Cargo.toml(1 hunks)docs/netsuke-design.md(13 hunks)docs/roadmap.md(2 hunks)src/ast.rs(1 hunks)tests/ast_tests.rs(1 hunks)tests/steps/manifest_steps.rs(1 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
Cargo.toml
Instructions used from:
Sources:
📄 CodeRabbit Inference Engine
- AGENTS.md
docs/**/*.md
Instructions used from:
Sources:
📄 CodeRabbit Inference Engine
- AGENTS.md
**/*.md
Instructions used from:
Sources:
📄 CodeRabbit Inference Engine
- AGENTS.md
⚙️ CodeRabbit Configuration File
**/*.rs
Instructions used from:
Sources:
📄 CodeRabbit Inference Engine
- AGENTS.md
⚙️ CodeRabbit Configuration File
🧠 Learnings (2)
docs/roadmap.md (1)
Learnt from: CR
PR: leynos/netsuke#0
File: docs/roadmap.md:0-0
Timestamp: 2025-07-18T19:04:19.414Z
Learning: Applies to docs/src/ast.rs : Implement the two-pass parsing mechanism: the first pass renders the manifest as a Jinja template, and the second pass parses the resulting pure YAML string with serde_yaml.
src/ast.rs (1)
Learnt from: CR
PR: leynos/netsuke#0
File: docs/roadmap.md:0-0
Timestamp: 2025-07-18T19:04:19.414Z
Learning: Applies to docs/src/ast.rs : Define the core Abstract Syntax Tree (AST) data structures (NetsukeManifest, Rule, Target, StringOrList, Recipe) in src/ast.rs.
🪛 LanguageTool
docs/netsuke-design.md
[uncategorized] ~180-~180: Loose punctuation mark.
Context: ... Ninja build statement.[^3] - steps: A secondary list of build targets. Any ...
(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~183-~183: Loose punctuation mark.
Context: ...ways: false }by default. -defaults`: An optional list of target names to be ...
(UNLIKELY_OPENING_PUNCTUATION)
🔇 Additional comments (19)
Cargo.toml (1)
8-10: LGTM! Dependency choices align with design requirements.The addition of
serdewith derive features,serde_ymlfor YAML parsing, andsemverwith serde support correctly implements the AST deserialization requirements. The switch from the deprecatedserde_yamltoserde_ymladdresses previous security concerns.docs/roadmap.md (2)
19-21: Documentation accurately reflects implementation progress.Marking the AST data structures task as completed is correct, and the terminology updates from "actions" to "steps" align with the actual implementation.
25-25: Consistent dependency naming updated.The correction from "serde_yaml" to "serde_yml" maintains consistency with the new dependency choice in Cargo.toml.
tests/ast_tests.rs (5)
1-6: Excellent module documentation and imports.The module-level documentation clearly states the purpose, and the imports are well-organised with proper use of semver::Version for version handling.
7-36: Robust minimal manifest test with proper assertions.The test correctly validates all key aspects of manifest parsing: version validation using semver, target name extraction with proper error handling, and recipe field verification. The improved error messages in the expect calls provide clear context for debugging.
38-62: Comprehensive coverage of missing required fields.The test thoroughly validates that all required fields (netsuke_version, targets, target names) are properly enforced during deserialization, ensuring robust error handling.
64-87: Proper validation of unknown field rejection.The tests confirm that
#[serde(deny_unknown_fields)]is working correctly at both root and nested levels, which helps surface user configuration errors early.
107-158: Thorough StringOrList variant testing.All three variants (String, List, Empty list) are properly tested with clear assertions, ensuring the flexible field handling works correctly across different manifest formats.
tests/steps/manifest_steps.rs (4)
1-7: Clear module documentation and well-organised imports.The module purpose is clearly stated, and imports are properly structured for the Cucumber testing framework integration.
8-28: Robust error handling for manifest parsing.The step correctly handles both file I/O errors and YAML parsing errors separately, providing clear error messages that will aid in test debugging. The improved error handling addresses previous review feedback effectively.
30-38: Proper clippy lint suppression with clear justification.The
#[expect]attribute correctly suppresses the needless_pass_by_value lint with a clear reason explaining the Cucumber framework requirement.
44-52: Improved error message consistency.The panic message provides clear context about the expected variant, following the improved error message format from previous review feedback.
docs/netsuke-design.md (6)
159-162: Semantic versioning adoption properly documented.The change to "1.0.0" format and integration with the semver crate provides robust version validation, which is essential for schema evolution.
180-181: Terminology update maintains consistency.The rename from "actions" to "steps" is consistently applied and aligns with the actual AST implementation.
191-232: Entity-relationship diagram improves clarity.The new Mermaid ER diagram provides a clearer representation of the manifest structure and relationships compared to a class diagram, making the schema more accessible to users.
376-395: Correct dependency documentation updates.The comprehensive updates from serde_yaml to serde_yml throughout the section maintain consistency with the actual dependency choices and provide accurate implementation guidance.
542-549: Valuable design decisions section added.The new section effectively explains the rationale behind key design choices, particularly the strict field validation and StringOrList default variant, which aids understanding and future maintenance.
1305-1306: Implementation roadmap updated correctly.The reference to serde_yml instead of serde_yaml maintains consistency with the actual dependency choices and provides accurate implementation guidance.
src/ast.rs (1)
52-69: Place doc comments before field attributes.According to the coding guidelines, doc comments must come before attributes. The current placement violates this rule.
Apply this diff to fix the attribute ordering:
- #[serde(default)] /// Global key/value pairs available to recipes. + #[serde(default)] pub vars: HashMap<String, String>, - #[serde(default)] /// Named rule templates that can be referenced by targets. + #[serde(default)] pub rules: Vec<Rule>, /// Optional top-level steps executed before normal targets. #[serde(default)] pub steps: Vec<Target>, /// Primary build targets. pub targets: Vec<Target>, - #[serde(default)] /// Names of targets built when no command line target is supplied. + #[serde(default)] pub defaults: Vec<String>,Likely an incorrect or invalid review comment.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
@sourcery-ai review |
There was a problem hiding this comment.
Hey @leynos - I've reviewed your changes and found some issues that need to be addressed.
- I noticed references to serde_yaml were swapped to serde_yml—please double-check that the crate rename is intentional and update any lingering serde_yaml mentions in code, docs, and comments to avoid confusion.
- The doc comment for Rule.deps still describes a whitespace-separated string, but the field is now a StringOrList; please update the documentation to reflect the new scalar-or-list semantics.
- The ER diagram in the design doc has field names and relationships (e.g. steps vs. actions) that may not exactly match the AST structs—consider verifying and syncing the diagrams with the current code definitions.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- I noticed references to serde_yaml were swapped to serde_yml—please double-check that the crate rename is intentional and update any lingering serde_yaml mentions in code, docs, and comments to avoid confusion.
- The doc comment for Rule.deps still describes a whitespace-separated string, but the field is now a StringOrList; please update the documentation to reflect the new scalar-or-list semantics.
- The ER diagram in the design doc has field names and relationships (e.g. steps vs. actions) that may not exactly match the AST structs—consider verifying and syncing the diagrams with the current code definitions.
## Individual Comments
### Comment 1
<location> `src/ast.rs:87` </location>
<code_context>
pub recipe: Recipe,
pub description: Option<String>,
- pub deps: Option<String>,
+ pub deps: Option<StringOrList>,
// Additional fields like 'pool' or 'restat' can be added here
// to map to more advanced Ninja features.
</code_context>
<issue_to_address>
Inconsistent use of Option<StringOrList> vs. defaulted StringOrList.
Standardize the type and defaulting of 'deps' across Rule and Target to avoid confusion and simplify downstream handling.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/ast.rs (1)
9-17: Fix the example by importingStringOrList.The example uses
StringOrList::Stringwithout importing it, which would cause a compilation error.Apply this diff to fix the example:
//! ```rust //! use netsuke::ast::NetsukeManifest; +//! use netsuke::ast::StringOrList; //! //! let yaml = r#"netsuke_version: \"1.0.0\"\ntargets:\n - name: hello\n recipe:\n kind: command\n command: \"echo hi\""#; //! let manifest: NetsukeManifest = serde_yml::from_str(yaml).expect("parse");
📜 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 (4)
Cargo.toml(1 hunks)README.md(7 hunks)src/ast.rs(1 hunks)tests/steps/manifest_steps.rs(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.md
Instructions used from:
Sources:
⚙️ CodeRabbit Configuration File
**/*.rs
Instructions used from:
Sources:
⚙️ CodeRabbit Configuration File
🧠 Learnings (1)
src/ast.rs (1)
Learnt from: CR
PR: leynos/netsuke#0
File: docs/roadmap.md:0-0
Timestamp: 2025-07-18T19:04:19.414Z
Learning: Applies to docs/src/ast.rs : Define the core Abstract Syntax Tree (AST) data structures (NetsukeManifest, Rule, Target, StringOrList, Recipe) in src/ast.rs.
🪛 LanguageTool
README.md
[uncategorized] ~85-~85: Loose punctuation mark.
Context: ... ``` Targets can also define: - deps: explicit dependencies - `order_only_dep...
(UNLIKELY_OPENING_PUNCTUATION)
[grammar] ~176-~176: Make sure that the adjective ‘small’ is correct. Possibly, it should be an adverb (typically ~ly) that modifies ‘carved’. Possibly, it should be the first word in a compound adjective (hyphenated adjective). Possibly, it is correct.
Context: ... ## Why “Netsuke”? A netsuke is a small carved object used to fasten things securely t...
(ADVERB_OR_HYPHENATED_ADJECTIVE)
[style] ~177-~177: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...It’s not the sword. It’s not the pouch. It’s the thing that connects them. That’s...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[locale-violation] ~182-~182: License must be spelled with a “c” when used as a noun in British English. Use “licence”.
Context: ...ent and the tool that gets it done. ## License [ISC](https://opensource.org/licenses/...
(LICENCE_LICENSE_NOUN_SINGULAR)
🔇 Additional comments (8)
Cargo.toml (1)
8-10: Dependency additions support AST implementation correctly.The new dependencies appropriately support the AST module:
serdewith derive features for deserialization,serde_ymlas a maintained YAML parser (addressing the deprecated serde_yaml concern), andsemverwith serde support for version handling.README.md (1)
3-25: Formatting improvements enhance readability.The text reflowing improves adherence to the 80-column wrapping guideline whilst maintaining content clarity.
tests/steps/manifest_steps.rs (1)
1-57: Step definitions implement manifest parsing correctly.The Cucumber step implementations properly handle file reading errors, YAML parsing errors, and provide clear test assertions. The error handling improvements from previous reviews have been correctly implemented.
src/ast.rs (5)
46-70: AST structures correctly implement the manifest schema.The
NetsukeManifeststruct properly defines all required fields with appropriate types, including the use ofsemver::Versionfor semantic versioning andStringOrListfor flexible field parsing.
77-88: Rule structure supports reusable command templates effectively.The
Rulestruct correctly encapsulates reusable command patterns with proper field types and serde attributes for strict deserialization.
94-106: Recipe enum provides comprehensive execution styles.The tagged enum design with aliases correctly supports different execution models (command, script, rule reference) whilst maintaining clear YAML syntax.
113-144: Target structure comprehensively models build targets.The
Targetstruct correctly defines all necessary fields for build target specification, with proper defaults and flexible string/list handling throughStringOrList.
160-170: StringOrList enum enables flexible YAML syntax.The untagged enum design correctly handles both scalar and sequence YAML inputs, providing the flexibility needed for manifest field specification.
| That’s what this is: a tidy connector between your intent and the tool that | ||
| gets it done. | ||
|
|
||
| ## License |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Use British English spelling for "licence".
Apply the British English spelling as required by the coding guidelines:
-## License
+## Licence📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| ## License | |
| ## Licence |
🧰 Tools
🪛 LanguageTool
[locale-violation] ~182-~182: License must be spelled with a “c” when used as a noun in British English. Use “licence”.
Context: ...ent and the tool that gets it done. ## License [ISC](https://opensource.org/licenses/...
(LICENCE_LICENSE_NOUN_SINGULAR)
🤖 Prompt for AI Agents
In README.md at line 182, change the heading "License" to use British English
spelling by replacing it with "Licence" to comply with the coding guidelines.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
1 participant
Summary
src/ast.rsTesting
make fmtmake lintmake testhttps://chatgpt.com/codex/tasks/task_e_687afee89400832283cdd3ed610e1159
Summary by Sourcery
Implement initial manifest AST support: introduce AST structs with serde_yml deserialization, expose the AST module, rename actions to steps, update docs and dependencies, and add comprehensive unit and behavioural tests.
New Features:
Enhancements:
Build:
Documentation:
Tests:
Chores: