Implement steps parsing with phony default#30
Conversation
Reviewer's GuideAdds a custom deserializer for top-level steps in the AST to ensure they are always marked as phony, complemented by new unit and BDD tests, updated fixtures, and documentation and roadmap updates. Class diagram for NetsukeManifest steps deserializationclassDiagram
class NetsukeManifest {
Vec<Rule> rules
Vec<Target> steps
Vec<Target> targets
}
class Target {
bool phony
bool always
...
}
NetsukeManifest --> "*" Target : steps
NetsukeManifest --> "*" Target : targets
%% Custom deserializer for steps
class deserialize_steps {
<<function>>
}
NetsukeManifest : steps [deserialize_with = "deserialize_steps"]
deserialize_steps --> Target : marks as phony
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
Warning Rate limit exceeded@leynos has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 12 minutes and 0 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (2)
Summary by CodeRabbit
WalkthroughRename the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant ManifestLoader
participant NetsukeManifest
participant Target
User->>ManifestLoader: Load manifest YAML
ManifestLoader->>NetsukeManifest: Deserialize YAML
NetsukeManifest->>Target: Deserialize actions with custom logic
Target-->>NetsukeManifest: Set phony = true for each action
NetsukeManifest-->>ManifestLoader: Return manifest with actions marked phony
ManifestLoader-->>User: Manifest ready for build process
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~18 minutes Possibly related PRs
Poem
✨ Finishing Touches🧪 Generate unit tests
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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 (
|
![codescene-delta-analysis[bot]](https://avatars.githubusercontent.com/in/53921?s=60&v=4){kind=small}
There was a problem hiding this comment.
Gates Failed
(1 file with Code Duplication)
Gates Passed
See analysis details in CodeScene
Reason for failure
| Enforce advisory code health rules | Violations | Code Health Impact | |
|---|---|---|---|
| ast_tests.rs | 1 advisory rule | 10.00 → 9.39 | Suppress |
Quality Gate Profile: Pay Down Tech Debt
Want more control? Customize Code Health rules or catch issues early with our IDE extension and CLI tool.
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @leynos - I've reviewed your changes 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/ast_tests.rs:249` </location>
<code_context>
}
+
+#[test]
+fn steps_are_marked_phony() {
+ let yaml = r#"
+ netsuke_version: "1.0.0"
+ steps:
+ - name: setup
+ recipe:
+ kind: command
+ command: "echo hi"
+ targets:
+ - name: done
+ recipe:
+ kind: command
+ command: "true"
+ "#;
+ let manifest = serde_yml::from_str::<NetsukeManifest>(yaml).expect("parse");
+ let step = manifest.steps.first().expect("step");
+ assert!(step.phony);
+ assert!(!step.always);
+}
+
</code_context>
<issue_to_address>
Consider adding a test for multiple steps.
Adding a test with multiple steps will verify that all steps are correctly marked as phony, not just the first one.
Suggested implementation:
```rust
#[test]
fn steps_are_marked_phony() {
let yaml = r#"
netsuke_version: "1.0.0"
steps:
- name: setup
recipe:
kind: command
command: "echo hi"
targets:
```
```rust
}
#[test]
fn multiple_steps_are_marked_phony() {
let yaml = r#"
netsuke_version: "1.0.0"
steps:
- name: setup
recipe:
kind: command
command: "echo hi"
- name: build
recipe:
kind: command
command: "make build"
- name: test
recipe:
kind: command
command: "cargo test"
targets:
- name: done
recipe:
kind: command
command: "true"
"#;
let manifest = serde_yml::from_str::<NetsukeManifest>(yaml).expect("parse");
assert_eq!(manifest.steps.len(), 3);
for step in &manifest.steps {
assert!(step.phony, "Step '{}' should be phony", step.name);
assert!(!step.always, "Step '{}' should not be always", step.name);
}
}
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (2)
tests/data/action_invalid.yml (1)
10-11: Remove excessive blank lines to comply with YAML formatting standards.The file contains too many blank lines at the end. Remove the extra blank line to satisfy YAMLlint requirements.
- - +tests/data/actions.yml (1)
13-14: Remove excessive blank lines to comply with YAML formatting standards.The file contains too many blank lines at the end. Remove the extra blank line to satisfy YAMLlint requirements.
- +
♻️ Duplicate comments (1)
tests/ast_tests.rs (1)
331-360: LGTM! Addresses previous feedback on multiple actions testing.The test properly verifies that all actions in a list are marked as phony, addressing the previous suggestion to test multiple actions rather than just the first one.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (9)
docs/behavioural-testing-in-rust-with-cucumber.md(1 hunks)docs/netsuke-design.md(10 hunks)docs/roadmap.md(1 hunks)src/ast.rs(3 hunks)tests/ast_tests.rs(3 hunks)tests/data/action_invalid.yml(1 hunks)tests/data/actions.yml(1 hunks)tests/features/manifest.feature(1 hunks)tests/steps/manifest_steps.rs(1 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
docs/**/*.md
📄 CodeRabbit Inference Engine (AGENTS.md)
docs/**/*.md: Use the markdown files within the docs/ directory as a knowledge base and source of truth for project requirements, dependency choices, and architectural decisions.
Proactively update the relevant file(s) in the docs/ directory to reflect the latest state when new decisions are made, requirements change, libraries are added/removed, or architectural patterns evolve.
Documentation must use en-GB-oxendict spelling and grammar (with the exception of 'license' which is to be left unchanged for community consistency).
Files:
docs/behavioural-testing-in-rust-with-cucumber.mddocs/roadmap.mddocs/netsuke-design.md
**/*.md
📄 CodeRabbit Inference Engine (AGENTS.md)
**/*.md: Validate Markdown files using make markdownlint.
Run make fmt after any documentation changes to format all Markdown files and fix table markup.
Validate Mermaid diagrams in Markdown files by running make nixie.
Markdown paragraphs and bullet points must be wrapped at 80 columns.
Code blocks in Markdown must be wrapped at 120 columns.
Tables and headings in Markdown must not be wrapped.
Use dashes (-) for list bullets in Markdown.
Use GitHub-flavoured Markdown footnotes ([^1]) for references.
Files:
docs/behavioural-testing-in-rust-with-cucumber.mddocs/roadmap.mddocs/netsuke-design.md
⚙️ CodeRabbit Configuration File
**/*.md: * Avoid 2nd person or 1st person pronouns ("I", "you", "we")
- Use en-GB-oxendict (-ize / -our) spelling and grammar
- Paragraphs and bullets must be wrapped to 80 columns, except where a long URL would prevent this (in which case, silence MD013 for that line)
- Code blocks should be wrapped to 120 columns.
- 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/behavioural-testing-in-rust-with-cucumber.mddocs/roadmap.mddocs/netsuke-design.md
**/*.rs
📄 CodeRabbit Inference Engine (AGENTS.md)
**/*.rs: Clippy warnings MUST be disallowed.
Fix any warnings emitted during tests in the code itself rather than silencing them.
Where a function is too long, extract meaningfully named helper functions adhering to separation of concerns and CQRS.
Where a function has too many parameters, group related parameters in meaningfully named structs.
Where a function is returning a large error consider using Arc to reduce the amount of data returned.
Document public APIs using Rustdoc comments (///) so documentation can be generated with cargo doc.
Every module must begin with a module level (//!) comment explaining the module's purpose and utility.
Prefer immutable data and avoid unnecessary mut bindings.
Handle errors with the Result type instead of panicking where feasible.
Avoid unsafe code unless absolutely necessary and document any usage clearly.
Place function attributes after doc comments.
Do not use return in single-line functions.
Use predicate functions for conditional criteria with more than two branches.
Lints must not be silenced except as a last resort.
Lint rule suppressions must be tightly scoped and include a clear reason.
Prefer expect over allow.
Prefer .expect() over .unwrap().
Files:
tests/steps/manifest_steps.rssrc/ast.rstests/ast_tests.rs
⚙️ CodeRabbit Configuration File
**/*.rs: * Seek to keep the cyclomatic complexity of functions no more than 12.
Adhere to single responsibility and CQRS
Place function attributes after doc comments.
Do not use
returnin single-line functions.Move conditionals with >2 branches into a predicate function.
Avoid
unsafeunless absolutely necessary.Every module must begin with a
//!doc comment that explains the module's purpose and utility.Comments and docs must follow en-GB-oxendict (-ize / -our) spelling and grammar
Lints must not be silenced except as a last resort.
#[allow]is forbidden.- Only narrowly scoped
#[expect(lint, reason = "...")]is allowed.- No lint groups, no blanket or file-wide suppression.
- Include
FIXME:with link if a fix is expected.Use
rstestfixtures for shared setup and to avoid repetition between tests.Replace duplicated tests with
#[rstest(...)]parameterised cases.Prefer
mockallfor mocks/stubs.Prefer
.expect()over.unwrap()Ensure that any API or behavioural changes are reflected in the documentation in
docs/Ensure that any completed roadmap steps are recorded in the appropriate roadmap in
docs/Files must not exceed 400 lines in length
- Large modules must be decomposed
- Long match statements or dispatch tables should be decomposed by domain and collocated with targets
- Large blocks of inline data (e.g., test fixtures, constants or templates) must be moved to external files and inlined at compile-time or loaded at run-time.
Files:
tests/steps/manifest_steps.rssrc/ast.rstests/ast_tests.rs
**/*{_test,tests}.rs
📄 CodeRabbit Inference Engine (AGENTS.md)
**/*{_test,tests}.rs: Write unit and behavioural tests for new functionality. Run both before and after making any change.
Use rstest fixtures for shared setup.
Replace duplicated tests with #[rstest(...)] parameterised cases.
Prefer mockall for mocks/stubs.
Files:
tests/ast_tests.rs
🧠 Learnings (1)
docs/netsuke-design.md (1)
Learnt from: CR
PR: leynos/netsuke#0
File: docs/roadmap.md:0-0
Timestamp: 2025-07-20T02:49:27.706Z
Learning: Applies to docs/src/ninja_gen.rs : Write logic to generate Ninja rule statements from ir::Action structs and build statements from ir::BuildEdge structs.
🪛 YAMLlint (1.37.1)
tests/data/action_invalid.yml
[error] 10-10: too many blank lines (1 > 0)
(empty-lines)
tests/data/actions.yml
[error] 13-13: too many blank lines (1 > 0)
(empty-lines)
🔇 Additional comments (14)
tests/features/manifest.feature (1)
13-19: Well-structured test scenarios for the actions feature.The new scenarios effectively test the core functionality changes: enforcing phony behaviour for actions and validating parsing failures for invalid configurations. The Gherkin syntax is clear and follows established patterns.
docs/roadmap.md (1)
32-33: Appropriately marks the actions parsing feature as completed.The update correctly reflects the completion of the actions parsing implementation and removes the redundant "steps" terminology, maintaining consistency with the schema changes.
tests/data/actions.yml (1)
7-7: Verify that the explicit phony: false setting tests the deserialization override.The action explicitly sets
phony: false, which should be overridden totrueduring deserialization according to the PR implementation. Ensure the corresponding test verifies this behaviour correctly.tests/steps/manifest_steps.rs (2)
72-77: LGTM! Correct implementation of action phony verification.The step definition properly verifies that actions are marked as phony by default, aligning with the new deserialization behavior.
79-82: LGTM! Proper error verification implementation.The step definition correctly verifies that parsing failures are detected and stored in the test world state.
src/ast.rs (2)
21-32: LGTM! Correct custom deserializer implementation.The
deserialize_actionsfunction properly enforces the phony default behavior for actions during deserialization. The implementation follows serde patterns and handles errors appropriately.
72-75: LGTM! Clear documentation and proper deserializer configuration.The field documentation accurately describes the new
actionsbehavior, and thedeserialize_withattribute correctly applies the custom deserializer.docs/netsuke-design.md (4)
180-181: LGTM! Accurate description of actions behavior.The documentation correctly describes that actions are treated as phony by default, aligning with the implementation.
196-196: LGTM! Consistent ER diagram updates.The ER diagram correctly reflects the field rename from
stepstoactionswhile maintaining proper relationships.Also applies to: 226-226
424-424: LGTM! Code examples updated correctly.The code examples properly reflect the field rename from
stepstoactions.Also applies to: 522-522
593-598: LGTM! Clear explanation of custom deserializer behavior.The design decisions section correctly explains that actions are deserialised using a custom helper to always be treated as phony tasks, ensuring they never generate build artefacts.
tests/ast_tests.rs (3)
7-10: LGTM! Excellent addition of helper function.The
parse_manifesthelper reduces code duplication and improves test readability, addressing previous review feedback.
208-233: LGTM! Excellent use of parameterised tests.The
parsing_failurestest properly uses rstest to eliminate duplication while maintaining comprehensive coverage of failure scenarios. The test correctly usesactionsinstead of the oldstepsfield name.
266-329: LGTM! Comprehensive actions behavior testing.The parameterised test properly verifies that actions are always marked as phony regardless of explicit flag values, correctly testing the custom deserializer behavior.
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 (3)
docs/behavioural-testing-in-rust-with-cucumber.md(1 hunks)tests/data/action_invalid.yml(1 hunks)tests/data/actions.yml(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
docs/**/*.md
📄 CodeRabbit Inference Engine (AGENTS.md)
docs/**/*.md: Use the markdown files within the docs/ directory as a knowledge base and source of truth for project requirements, dependency choices, and architectural decisions.
Proactively update the relevant file(s) in the docs/ directory to reflect the latest state when new decisions are made, requirements change, libraries are added/removed, or architectural patterns evolve.
Documentation must use en-GB-oxendict spelling and grammar (with the exception of 'license' which is to be left unchanged for community consistency).
Files:
docs/behavioural-testing-in-rust-with-cucumber.md
**/*.md
📄 CodeRabbit Inference Engine (AGENTS.md)
**/*.md: Validate Markdown files using make markdownlint.
Run make fmt after any documentation changes to format all Markdown files and fix table markup.
Validate Mermaid diagrams in Markdown files by running make nixie.
Markdown paragraphs and bullet points must be wrapped at 80 columns.
Code blocks in Markdown must be wrapped at 120 columns.
Tables and headings in Markdown must not be wrapped.
Use dashes (-) for list bullets in Markdown.
Use GitHub-flavoured Markdown footnotes ([^1]) for references.
Files:
docs/behavioural-testing-in-rust-with-cucumber.md
⚙️ CodeRabbit Configuration File
**/*.md: * Avoid 2nd person or 1st person pronouns ("I", "you", "we")
- Use en-GB-oxendict (-ize / -our) spelling and grammar
- Paragraphs and bullets must be wrapped to 80 columns, except where a long URL would prevent this (in which case, silence MD013 for that line)
- Code blocks should be wrapped to 120 columns.
- 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/behavioural-testing-in-rust-with-cucumber.md
🔇 Additional comments (1)
tests/data/action_invalid.yml (1)
1-9: Keep as-is – fixture is fit for purposeThe YAML cleanly represents the negative test case:
phony: falseviolates the phony-by-default rule, field names match the updated schema, and quoting avoids YAML boolean coercion. No amendments required.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
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)
docs/behavioural-testing-in-rust-with-cucumber.md(1 hunks)tests/data/action_invalid.yml(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
docs/**/*.md
📄 CodeRabbit Inference Engine (AGENTS.md)
docs/**/*.md: Use the markdown files within the docs/ directory as a knowledge base and source of truth for project requirements, dependency choices, and architectural decisions.
Proactively update the relevant file(s) in the docs/ directory to reflect the latest state when new decisions are made, requirements change, libraries are added/removed, or architectural patterns evolve.
Documentation must use en-GB-oxendict spelling and grammar (with the exception of 'license' which is to be left unchanged for community consistency).
Files:
docs/behavioural-testing-in-rust-with-cucumber.md
**/*.md
📄 CodeRabbit Inference Engine (AGENTS.md)
**/*.md: Validate Markdown files using make markdownlint.
Run make fmt after any documentation changes to format all Markdown files and fix table markup.
Validate Mermaid diagrams in Markdown files by running make nixie.
Markdown paragraphs and bullet points must be wrapped at 80 columns.
Code blocks in Markdown must be wrapped at 120 columns.
Tables and headings in Markdown must not be wrapped.
Use dashes (-) for list bullets in Markdown.
Use GitHub-flavoured Markdown footnotes ([^1]) for references.
Files:
docs/behavioural-testing-in-rust-with-cucumber.md
⚙️ CodeRabbit Configuration File
**/*.md: * Avoid 2nd person or 1st person pronouns ("I", "you", "we")
- Use en-GB-oxendict (-ize / -our) spelling and grammar
- Paragraphs and bullets must be wrapped to 80 columns, except where a long URL would prevent this (in which case, silence MD013 for that line)
- Code blocks should be wrapped to 120 columns.
- 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/behavioural-testing-in-rust-with-cucumber.md
🪛 LanguageTool
docs/behavioural-testing-in-rust-with-cucumber.md
[typographical] ~1171-~1171: To join two clauses or introduce examples, consider using an em dash.
Context: ...ml> [^32]: CLI (command-line interface) - Cucumber Rust Book, accessed on 14 J...
(DASH_RULE)
🔇 Additional comments (1)
docs/behavioural-testing-in-rust-with-cucumber.md (1)
1167-1169: Footnote continuation indentation is correct – good fix
Four-space indentation matches the project’s Markdown rules and silences MD013 without disabling the rule.
No further changes required.
1 participant
Summary
Testing
make fmtmake lintmake testmake markdownlinthttps://chatgpt.com/codex/tasks/task_e_6881498e6da08322a1cf81533393c641
Summary by Sourcery
Enable parsing of a new 'steps' section in the manifest with a custom deserializer that treats all steps as phony tasks by default, update documentation and tests to cover this behavior
New Features:
Enhancements:
Documentation:
Tests: