Conversation
Reviewer's GuideThis PR fleshes out IR generation by implementing BuildGraph::from_manifest (with rule-to-action mapping, deduplication via SHA-256 hashing, and validation of rule presence, single-rule enforcement, and duplicate outputs), introduces IrGenError for clear failure reporting, and updates documentation, tests, and CI accordingly. File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Summary by CodeRabbitNew Features
Bug Fixes
Documentation
Tests
Chores
WalkthroughUpdate the intermediate representation (IR) generation by introducing error handling for missing and multiple rules, action deduplication via SHA-256 hashing, and conversion from manifest to build graph. Extend documentation to clarify IR behaviour and update the roadmap. Add comprehensive tests, new test data, and supporting dependencies. Changes
Sequence Diagram(s)sequenceDiagram
participant Tester
participant ManifestLoader
participant BuildGraph
participant Error
Tester->>ManifestLoader: Load manifest from file
ManifestLoader->>BuildGraph: from_manifest(manifest)
BuildGraph->>BuildGraph: Deduplicate actions (SHA-256 hash)
BuildGraph->>BuildGraph: Validate rule references
alt Rule missing or multiple rules
BuildGraph->>Error: Return IrGenError
else Success
BuildGraph->>Tester: Return BuildGraph
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~18 minutes Possibly related PRs
Suggested reviewers
Poem
Warning Review ran into problems🔥 ProblemsErrors were encountered while retrieving linked issues. Errors (1)
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (5)
🧰 Additional context used📓 Path-based instructions (1)**/*.rs⚙️ CodeRabbit Configuration File
Files:
🧠 Learnings (1)src/ir.rs (1)Learnt from: CR 🧬 Code Graph Analysis (1)tests/steps/ir_steps.rs (2)
🔇 Additional comments (19)
✨ 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 Complex Method, Large Method)
Gates Passed
See analysis details in CodeScene
Reason for failure
| Enforce advisory code health rules | Violations | Code Health Impact | |
|---|---|---|---|
| ir.rs | 2 advisory rules | 10.00 → 9.29 | Suppress |
Quality Gate Profile: Pay Down Tech Debt
Want more control? Customize Code Health rules or catch issues early with our IDE extension and CLI tool.
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @leynos - I've reviewed your changes - here's some feedback:
- hash_action uses simple concatenation which can lead to accidental hash collisions; consider inserting explicit separators or length prefixes between fields and list items.
- The logic for creating and hashing actions in both the rule and command/script branches is nearly identical—extract it into a helper to reduce duplication.
- graph.targets.insert silently overwrites existing edges for duplicate outputs; consider detecting and erroring on duplicate target outputs instead of clobbering previous entries.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- hash_action uses simple concatenation which can lead to accidental hash collisions; consider inserting explicit separators or length prefixes between fields and list items.
- The logic for creating and hashing actions in both the rule and command/script branches is nearly identical—extract it into a helper to reduce duplication.
- graph.targets.insert silently overwrites existing edges for duplicate outputs; consider detecting and erroring on duplicate target outputs instead of clobbering previous entries.
## Individual Comments
### Comment 1
<location> `src/ir.rs:113` </location>
<code_context>
+ rule_map.insert(rule.name.clone(), hash);
+ }
+
+ for target in manifest.actions.iter().chain(&manifest.targets) {
+ let outputs = to_paths(&target.name);
+ let action_id = match &target.recipe {
</code_context>
<issue_to_address>
Combining actions and targets may lead to duplicate outputs.
Chaining `manifest.actions` and `manifest.targets` can cause duplicate output paths, with later entries overwriting earlier ones in `graph.targets`. If this is expected, please document or enforce uniqueness to prevent subtle bugs.
</issue_to_address>
### Comment 2
<location> `src/ir.rs:176` </location>
<code_context>
+ }
+}
+
+fn hash_action(action: &Action) -> String {
+ let mut hasher = Sha256::new();
+ match &action.recipe {
</code_context>
<issue_to_address>
Hashing may not be stable if field order or formatting changes.
Consider documenting the hash's dependency on field order and formatting, or use a more robust serialization method if hash stability is required.
</issue_to_address>
<suggested_fix>
<<<<<<< SEARCH
fn hash_action(action: &Action) -> String {
=======
/// Computes a hash for an `Action`.
///
/// Note: The hash is dependent on the order and formatting of fields as they are serialized here.
/// If hash stability is required across code or format changes, consider using a robust serialization
/// method (e.g., serde with canonical formats) to ensure consistent hashing.
fn hash_action(action: &Action) -> String {
>>>>>>> REPLACE
</suggested_fix>
### Comment 3
<location> `tests/ir_from_manifest_tests.rs:25` </location>
<code_context>
+ assert_eq!(graph.targets.len(), 2);
+}
+
+#[test]
+fn missing_rule_fails() {
+ let manifest = manifest::from_path("tests/data/missing_rule.yml").expect("load");
+ let err = BuildGraph::from_manifest(&manifest).expect_err("error");
</code_context>
<issue_to_address>
Test for multiple rules per target error.
Please add a test that verifies the MultipleRules error is triggered when multiple rules are specified for a single target.
Suggested implementation:
```rust
#[test]
fn missing_rule_fails() {
let manifest = manifest::from_path("tests/data/missing_rule.yml").expect("load");
let err = BuildGraph::from_manifest(&manifest).expect_err("error");
matches!(err, IrGenError::RuleNotFound { .. });
}
#[test]
fn multiple_rules_per_target_fails() {
let manifest = manifest::from_path("tests/data/multiple_rules_per_target.yml").expect("load");
let err = BuildGraph::from_manifest(&manifest).expect_err("error");
assert!(matches!(err, IrGenError::MultipleRules { .. }));
}
```
You will need to create a test manifest file at `tests/data/multiple_rules_per_target.yml` that defines a target with multiple rules, so that the test can trigger the `MultipleRules` error.
</issue_to_address>
### Comment 4
<location> `tests/ir_from_manifest_tests.rs:29` </location>
<code_context>
+fn missing_rule_fails() {
+ let manifest = manifest::from_path("tests/data/missing_rule.yml").expect("load");
+ let err = BuildGraph::from_manifest(&manifest).expect_err("error");
+ matches!(err, IrGenError::RuleNotFound { .. });
+}
</code_context>
<issue_to_address>
Assert the error type explicitly.
Use `assert!(matches!(err, IrGenError::RuleNotFound { .. }))` to ensure the test fails if the error type is not as expected.
</issue_to_address>
### Comment 5
<location> `src/ir.rs:95` </location>
<code_context>
+ ///
+ /// Returns [`IrGenError`] when a referenced rule is missing or multiple
+ /// rules are specified for a single target.
+ pub fn from_manifest(manifest: &NetsukeManifest) -> Result<Self, IrGenError> {
+ let mut graph = Self::default();
+ let mut rule_map = HashMap::new();
</code_context>
<issue_to_address>
Consider splitting the large from_manifest function into smaller helpers and refactoring hash_action to reduce repetition.
Here are two small, focused refactorings you can apply without changing behavior. They pull chunks out of your big `from_manifest` and DRY‐up `hash_action`.
1) Split `from_manifest` into three private helpers:
```rust
impl BuildGraph {
pub fn from_manifest(m: &NetsukeManifest) -> Result<Self, IrGenError> {
let mut graph = Self::default();
let rule_map = graph.register_rules(&m.rules);
graph.register_targets(&rule_map, m.actions.iter().chain(&m.targets))?;
graph.register_defaults(&m.defaults);
Ok(graph)
}
fn register_rules(&mut self, rules: &[Rule]) -> HashMap<String, String> {
let mut map = HashMap::new();
for rule in rules {
let action = Action { /* ... */ };
let hash = hash_action(&action);
self.actions.entry(hash.clone()).or_insert(action);
map.insert(rule.name.clone(), hash);
}
map
}
fn register_targets<I>(&mut self,
rule_map: &HashMap<String,String>,
targets: I
) -> Result<(), IrGenError>
where I: IntoIterator<Item=&NetsukeAction>
{
for tgt in targets {
let outputs = to_paths(&tgt.name);
let action_id = match &tgt.recipe {
Recipe::Rule { rule } => lookup_rule(rule, &outputs, rule_map)?,
_ => self.insert_dynamic_action(&tgt.recipe),
};
let edge = BuildEdge { action_id, inputs: to_paths(&tgt.sources), /* ... */ };
for o in outputs { self.targets.insert(o, edge.clone()); }
}
Ok(())
}
fn register_defaults(&mut self, defs: &[String]) {
self.default_targets
.extend(defs.iter().map(PathBuf::from));
}
fn insert_dynamic_action(&mut self, recipe: &Recipe) -> String {
let action = Action { recipe: recipe.clone(), /* … */ };
let hash = hash_action(&action);
self.actions.entry(hash.clone()).or_insert(action);
hash
}
}
// helper for rule lookup
fn lookup_rule(
rule: &StringOrList,
outputs: &[PathBuf],
rule_map: &HashMap<String,String>
) -> Result<String, IrGenError> {
let name = extract_single(rule)
.ok_or_else(|| IrGenError::MultipleRules {
target_name: outputs.get(0).map(|p| p.display().to_string()).unwrap_or_default()
})?;
rule_map.get(name)
.cloned()
.ok_or_else(|| IrGenError::RuleNotFound {
target_name: outputs.get(0).map(|p| p.display().to_string()).unwrap_or_default(),
rule_name: name.to_string(),
})
}
```
2) Collapse repetitive `hash_action` updates by iterating over fields and unifying `StringOrList`:
```rust
impl StringOrList {
fn iter_str(&self) -> Vec<&str> {
match self {
StringOrList::String(s) => vec![s],
StringOrList::List(v) => v.iter().map(String::as_str).collect(),
StringOrList::Empty => Vec::new(),
}
}
}
fn hash_action(a: &Action) -> String {
let mut hasher = Sha256::new();
// recipe prefix + its strings
let header = match &a.recipe {
Recipe::Command { command } => format!("cmd:{command}"),
Recipe::Script { script } => format!("scr:{script}"),
Recipe::Rule { rule } => {
let mut s = "rule:".to_string();
for r in rule.iter_str() { s.push_str(r) }
s
}
};
hasher.update(header);
// all optional String fields
for opt in [&a.description, &a.depfile, &a.deps_format, &a.pool] {
if let Some(v) = opt { hasher.update(v.as_bytes()); }
}
hasher.update(&[a.restat as u8]);
format!("{:x}", hasher.finalize())
}
```
These steps keep the same logic but turn one huge function into a few well‐named ones and remove the repeated `hasher.update`/`match` noise.
</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: 5
🔭 Outside diff range comments (1)
tests/features/ir.feature (1)
21-23: Clarify the failure assertion to reflect IR generation failure, not parsing failure.The step "parsing the manifest fails" is misleading. Based on the implementation, the manifest parsing succeeds but the IR generation fails when a rule is not found. Update the assertion to accurately reflect this.
- Scenario: Rule not found during IR generation - When the manifest file "tests/data/missing_rule.yml" is compiled to IR - Then parsing the manifest fails + Scenario: Rule not found during IR generation + When the manifest file "tests/data/missing_rule.yml" is compiled to IR + Then IR generation fails
📜 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 (9)
Cargo.toml(1 hunks)docs/netsuke-design.md(1 hunks)docs/roadmap.md(1 hunks)src/ir.rs(1 hunks)tests/data/duplicate_rules.yml(1 hunks)tests/data/missing_rule.yml(1 hunks)tests/features/ir.feature(1 hunks)tests/ir_from_manifest_tests.rs(1 hunks)tests/steps/ir_steps.rs(1 hunks)
🧰 Additional context used
📓 Path-based instructions (5)
Cargo.toml
📄 CodeRabbit Inference Engine (AGENTS.md)
Use explicit version ranges in Cargo.toml and keep dependencies up-to-date.
Files:
Cargo.toml
**/*.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/ir_steps.rstests/ir_from_manifest_tests.rssrc/ir.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/ir_steps.rstests/ir_from_manifest_tests.rssrc/ir.rs
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/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/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/roadmap.mddocs/netsuke-design.md
**/*{_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/ir_from_manifest_tests.rs
🧠 Learnings (1)
docs/roadmap.md (1)
Learnt from: CR
PR: leynos/netsuke#0
File: docs/netsuke-design.md:0-0
Timestamp: 2025-07-20T02:49:08.228Z
Learning: Applies to docs/src/ir.rs : The AST-to-IR transformation logic must be implemented in a function (e.g., ir::from_manifest) that consumes a NetsukeManifest and produces a BuildGraph, performing validation such as rule existence and cycle detection.
🧬 Code Graph Analysis (1)
tests/ir_from_manifest_tests.rs (2)
src/manifest.rs (1)
from_path(51-56)src/ir.rs (1)
from_manifest(95-173)
🔇 Additional comments (12)
Cargo.toml (1)
12-13: Pin dependency versions to concrete patch releasesLine-12 and Line-13 float on any
1.*or0.10.*release. This violates the “explicit version ranges” rule and risks reproducibility issues whenever a new patch lands.-thiserror = "1" -sha2 = "0.10" +thiserror = "1.0.59" # latest 1.x at time of writing +sha2 = "0.10.8" # latest 0.10.x at time of writingRun
cargo update -p thiserror sha2to verify the exact latest patch numbers.tests/data/duplicate_rules.yml (1)
1-22: Fixture looks correctThe manifest exercises rule deduplication and matches the spec. No issues.
tests/data/missing_rule.yml (1)
1-7: Fixture correctly triggers RuleNotFoundThe target references a non-existent rule, aligning with the negative-test intent.
tests/features/ir.feature (2)
10-14: LGTM!The scenario correctly tests the basic manifest compilation functionality with appropriate assertions.
16-19: LGTM!The scenario effectively validates the deduplication functionality by asserting that duplicate rules result in a single action with multiple targets.
tests/steps/ir_steps.rs (1)
30-48: LGTM!The implementation correctly handles both success and error paths, with appropriate state management. The Clippy suppression is properly scoped and justified for Cucumber's requirements.
tests/ir_from_manifest_tests.rs (2)
10-15: LGTM!The test correctly validates IR generation from a minimal manifest.
18-23: LGTM!The test effectively validates that duplicate rules are deduplicated into a single action.
src/ir.rs (4)
76-86: LGTM!The error enum is well-structured with clear, contextual error messages using thiserror.
96-111: LGTM!The rule processing and deduplication logic is well-implemented using a HashMap to track rule-to-action mappings.
176-208: LGTM! Ensure list order is preserved for deterministic hashing.The hashing implementation is comprehensive and correctly prefixes recipe types to avoid collisions. Note that for
StringOrList::List, the order of elements affects the hash, which is correct for deterministic builds.
210-224: LGTM!Both helper functions are well-implemented and handle all
StringOrListvariants correctly.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (4)
docs/netsuke-design.md (1)
1059-1065: Re-wrap bullet points to comply with documentation standards.The bullet points exceed 80 columns, violating the coding guidelines for Markdown files. Hard-wrap each line to ensure it stays within 80 characters whilst maintaining proper Markdown bullet formatting.
Apply this diff to fix the line wrapping:
-- Actions are deduplicated using a SHA-256 hash of their recipe and metadata. - Identical commands therefore share the same identifier which keeps the IR - deterministic for snapshot tests. -- Multiple rule references in a single target are not yet supported. The IR - generator reports `IrGenError::MultipleRules` when encountered. -- Duplicate output files are rejected. Attempting to define the same output - path twice results in `IrGenError::DuplicateOutput`. +- Actions are deduplicated using a SHA-256 hash of their recipe and + metadata. Identical commands therefore share the same identifier which + keeps the IR deterministic for snapshot tests. +- Multiple rule references in a single target are not yet supported. The IR + generator reports `IrGenError::MultipleRules` when encountered. +- Duplicate output files are rejected. Attempting to define the same output + path twice results in `IrGenError::DuplicateOutput`.tests/ir_from_manifest_tests.rs (1)
25-30: Fix test attributes and assertions for consistency.Use
#[rstest]instead of#[test]for consistency with other tests in the module, and wrap thematches!macro withassert!to actually perform the assertion.Apply this diff:
-#[test] +#[rstest] fn missing_rule_fails() { let manifest = manifest::from_path("tests/data/missing_rule.yml").expect("load"); let err = BuildGraph::from_manifest(&manifest).expect_err("error"); - matches!(err, IrGenError::RuleNotFound { .. }); + assert!(matches!(err, IrGenError::RuleNotFound { .. })); }src/ir.rs (2)
126-150: Extract repeated code for getting target display name.The code for getting the first output and converting it to a display string is repeated. Extract this into a helper function to reduce duplication.
Add this helper function before
from_manifest:+fn get_target_display_name(outputs: &[PathBuf]) -> String { + outputs + .first() + .map(|p| p.display().to_string()) + .unwrap_or_default() +}Then simplify the error construction:
- let name = extract_single(rule).ok_or_else(|| IrGenError::MultipleRules { - target_name: outputs - .first() - .cloned() - .unwrap_or_default() - .display() - .to_string(), - })?; + let name = extract_single(rule).ok_or_else(|| IrGenError::MultipleRules { + target_name: get_target_display_name(&outputs), + })?;And similarly for the RuleNotFound error:
- .ok_or_else(|| IrGenError::RuleNotFound { - target_name: outputs - .first() - .cloned() - .unwrap_or_default() - .display() - .to_string(), - rule_name: name.to_string(), - })? + .ok_or_else(|| IrGenError::RuleNotFound { + target_name: get_target_display_name(&outputs), + rule_name: name.to_string(), + })?
156-173: Remove unnecessary clone of action_id.The
action_idis cloned unnecessarily on line 157. Since it's already cloned when needed later, remove the redundant clone.let edge = BuildEdge { - action_id: action_id.clone(), + action_id, inputs: to_paths(&target.sources), explicit_outputs: outputs.clone(), implicit_outputs: Vec::new(), order_only_deps: to_paths(&target.order_only_deps), phony: target.phony, always: target.always, };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (6)
.github/workflows/ci.yml(1 hunks)docs/netsuke-design.md(1 hunks)src/ir.rs(1 hunks)tests/data/duplicate_outputs.yml(1 hunks)tests/features/ir.feature(1 hunks)tests/ir_from_manifest_tests.rs(1 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
**/*.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/ir_from_manifest_tests.rssrc/ir.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/ir_from_manifest_tests.rssrc/ir.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/ir_from_manifest_tests.rs
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/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/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/netsuke-design.md
🔇 Additional comments (9)
tests/data/duplicate_outputs.yml (1)
1-13: LGTM! Test data structure is correct.The YAML structure properly defines two targets with duplicate output names (
hello.o) but different sources, which will correctly trigger theIrGenError::DuplicateOutputvalidation error during IR generation..github/workflows/ci.yml (1)
32-38: LGTM! Improved token handling and action version update.The changes properly centralise token usage through an environment variable and update to a newer action version. The conditional check using the environment variable is more explicit than directly checking the secret.
tests/features/ir.feature (1)
10-26: LGTM! Comprehensive feature test coverage for IR generation.The scenarios effectively cover the key IR generation functionality including successful compilation, rule deduplication, and error handling for missing rules and duplicate outputs. The test structure aligns well with the implementation and provides good behavioural coverage.
src/ir.rs (6)
75-90: Well-structured error handling with clear messages.The error enum is properly designed with descriptive variants and helpful error messages using the
thiserrorcrate.
91-108: Excellent refactoring of the from_manifest method.The method has been successfully decomposed into focused helper methods, improving readability and adhering to single responsibility principle.
167-173: Proper validation for duplicate outputs.The duplicate output detection correctly prevents multiple targets from producing the same output file, with a clear error message.
203-261: Well-structured hashing implementation with proper serialisation.The refactored hashing functions successfully reduce complexity while maintaining deterministic behaviour. The length-prefixed encoding ensures unambiguous hashing.
263-277: Clean utility functions with proper edge case handling.The utility functions are well-implemented with clear single responsibilities.
279-310: Comprehensive hash stability tests using rstest.The parameterised tests effectively verify hash stability across different action configurations, ensuring deterministic behaviour.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
@sourcery-ai review |
There was a problem hiding this comment.
Hey @leynos - I've reviewed your changes - here's some feedback:
- Consider adopting a canonical serialization format (e.g., serde JSON canonical) for hashing Action so that the deduplication hash remains stable across code changes rather than relying on manual byte sequences.
- The current error messages only reference the first conflicting output or rule; including all conflicting values in IrGenError variants could make troubleshooting multi-output or multi-rule errors more straightforward.
- Extract the hashing routines into a separate module or Hasher type to decouple them from IR construction and facilitate unit testing and future extensions.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Consider adopting a canonical serialization format (e.g., serde JSON canonical) for hashing Action so that the deduplication hash remains stable across code changes rather than relying on manual byte sequences.
- The current error messages only reference the first conflicting output or rule; including all conflicting values in IrGenError variants could make troubleshooting multi-output or multi-rule errors more straightforward.
- Extract the hashing routines into a separate module or Hasher type to decouple them from IR construction and facilitate unit testing and future extensions.
## Individual Comments
### Comment 1
<location> `src/ir.rs:70` </location>
<code_context>
/// Run the command on every invocation regardless of timestamps.
pub always: bool,
}
+
+use crate::ast::{NetsukeManifest, Recipe, StringOrList};
+use sha2::{Digest, Sha256};
</code_context>
<issue_to_address>
The module is missing a `//!` comment at the top as required by the review instructions.
Please add a `//!` module-level comment at the very top of this file to describe its purpose, as per the project guidelines.
</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 new GitHub issue, please: Consider adopting a canonical serialization format (e.g., serde JSON canonical) for hashing Action so that the deduplication hash remains stable across code changes rather than relying on manual byte sequences. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain@leynos I'll create a GitHub issue for this improvement suggestion regarding canonical serialization for Action hashing. 🏁 Script executed: Length of output: 1017 @leynos Perfect! I've created GitHub issue #35: "Consider canonical serialization format for Action hashing stability" at #35 The issue captures your suggestion to replace the current manual byte sequence construction with a canonical serialisation format (like serde JSON canonical) for more stable and maintainable Action deduplication hashing. It includes proper context linking back to this PR and your comment, and has been assigned to you for future implementation. |
1 participant
Summary
BuildGraph::from_manifestwith action deduplicationIrGenErrorfor validation failuresrstestandcucumberTesting
make fmtmake lintmake testmake markdownlintmake nixie(fails: too many arguments)https://chatgpt.com/codex/tasks/task_e_68852d434cc88322b61cbab22ab5ca68
Summary by Sourcery
Implement IR generation pipeline by transforming parsed manifests into BuildGraph instances with rule deduplication, validation, and action hashing, alongside comprehensive tests and documentation updates.
New Features:
Enhancements:
Build:
CI:
Documentation:
Tests: