Conversation
Reviewer's GuideThis PR modularizes action hashing into a dedicated ActionHasher utility, extends IR generation errors to include full conflict lists (for duplicate outputs, multiple rules, and empty rules), refactors BuildGraph to accumulate and sort all conflicts before erroring, and updates tests to validate the new error variants and hashing behavior. Sequence diagram for action hashing using ActionHashersequenceDiagram
participant BuildGraph
participant ActionHasher
participant Action
BuildGraph->>ActionHasher: hash(&Action)
ActionHasher->>Action: read fields
ActionHasher-->>BuildGraph: return hash string
Class diagram for ActionHasher utility and related IR changesclassDiagram
class ActionHasher {
+hash(action: Action) String
-hash_recipe(hasher: Sha256, recipe: Recipe)
-hash_optional_fields(hasher: Sha256, action: Action)
-hash_rule_reference(hasher: Sha256, rule: StringOrList)
-hash_optional_string(hasher: Sha256, value: Option<String>)
-update_with_len(hasher: Sha256, bytes: &[u8])
}
class Action {
+recipe: Recipe
+description: Option<String>
+depfile: Option<String>
+deps_format: Option<String>
+pool: Option<String>
+restat: bool
}
class Recipe {
<<enum>>
Command
Script
Rule
}
class StringOrList {
<<enum>>
String
List
Empty
}
ActionHasher ..> Action : hashes
Action o-- Recipe
Recipe o-- StringOrList
Flow diagram for BuildGraph IR error detection processflowchart TD
A[Start BuildGraph::from_manifest]
B[Iterate targets]
C[Check for multiple rules]
D[Check for empty rule]
E[Check for duplicate outputs]
F[Collect all conflicts]
G[Sort and return IrGenError with lists]
H[Insert targets if no conflicts]
A --> B --> C --> D --> E --> F
F -->|Conflicts found| G
F -->|No conflicts| H
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 10 minutes and 45 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 (5)
Summary by CodeRabbit
WalkthroughIntroduce a new Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant IR
participant Hasher
Client->>IR: register_action(action)
IR->>Hasher: ActionHasher::hash(action)
Hasher-->>IR: hash_string
IR-->>Client: Action registered with hash
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}
![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:
- Sort the
duplicatesandrulesvectors before returning errors to guarantee deterministic ordering in error messages. - Consider abstracting the
StringOrList→ Vec conversion sinceto_pathsandto_stringsshare almost identical logic or renameto_stringsto better reflect its intent.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Sort the `duplicates` and `rules` vectors before returning errors to guarantee deterministic ordering in error messages.
- Consider abstracting the `StringOrList` → Vec conversion since `to_paths` and `to_strings` share almost identical logic or rename `to_strings` to better reflect its intent.
## Individual Comments
### Comment 1
<location> `src/ir.rs:134` </location>
<code_context>
- .get(name)
- .cloned()
- .ok_or_else(|| IrGenError::RuleNotFound {
+ if let Some(name) = extract_single(rule) {
+ rule_map
+ .get(name)
+ .cloned()
+ .ok_or_else(|| IrGenError::RuleNotFound {
+ target_name: get_target_display_name(&outputs),
+ rule_name: name.to_string(),
+ })?
+ } else {
+ return Err(IrGenError::MultipleRules {
target_name: get_target_display_name(&outputs),
- rule_name: name.to_string(),
- })?
+ rules: to_strings(rule),
+ });
+ }
</code_context>
<issue_to_address>
The logic for handling multiple rules per target is now more explicit.
Consider handling the case where to_strings(rule) returns an empty list, as this may indicate an unexpected Empty rule and could lead to unclear error messages.
Suggested implementation:
```rust
if let Some(name) = extract_single(rule) {
rule_map
.get(name)
.cloned()
.ok_or_else(|| IrGenError::RuleNotFound {
target_name: get_target_display_name(&outputs),
rule_name: name.to_string(),
})?
} else {
let rules = to_strings(rule);
if rules.is_empty() {
return Err(IrGenError::EmptyRule {
target_name: get_target_display_name(&outputs),
});
} else {
return Err(IrGenError::MultipleRules {
target_name: get_target_display_name(&outputs),
rules,
});
}
}
```
```rust
#[derive(Debug, Error)]
pub enum IrGenError {
#[error("No rules specified for target {target_name}")]
EmptyRule {
target_name: String,
},
```
</issue_to_address>
### Comment 2
<location> `tests/hasher_tests.rs:8` </location>
<code_context>
+ use crate::ir::Action;
+ use rstest::rstest;
+
+ #[rstest]
+ #[case(
+ Action {
+ recipe: Recipe::Command { command: "echo".into() },
+ description: Some("desc".into()),
+ depfile: Some("$out.d".into()),
+ deps_format: Some("gcc".into()),
+ pool: None,
+ restat: false,
+ },
+ "a0f6e2cd3b9b3cee0bf94a7d53bce56cf4178dfe907bb1cb7c832f47846baf38"
+ )]
+ #[case(
+ Action {
+ recipe: Recipe::Rule { rule: StringOrList::List(vec!["a".into(), "b".into()]) },
+ description: None,
+ depfile: None,
+ deps_format: None,
+ pool: None,
+ restat: true,
+ },
+ "cf8e97357820acf6f66037dcf977ee36c88c2811d60342db30c99507d24a0d60"
+ )]
+ fn hash_action_is_stable(#[case] action: Action, #[case] expected: &str) {
+ assert_eq!(ActionHasher::hash(&action), expected);
+ }
</code_context>
<issue_to_address>
Hasher tests duplicate those in src/hasher.rs; consider deduplication or expanding coverage.
The hash_action_is_stable test duplicates one in src/hasher.rs. Please consolidate these tests to a single location, ideally tests/hasher_tests.rs, or add new cases here to cover additional edge scenarios such as all fields None, empty lists, or special characters.
Suggested implementation:
```rust
#[rstest]
#[case(
Action {
recipe: Recipe::Command { command: "echo".into() },
description: Some("desc".into()),
depfile: Some("$out.d".into()),
deps_format: Some("gcc".into()),
pool: None,
restat: false,
},
"a0f6e2cd3b9b3cee0bf94a7d53bce56cf4178dfe907bb1cb7c832f47846baf38"
)]
#[case(
Action {
recipe: Recipe::Rule { rule: StringOrList::List(vec!["a".into(), "b".into()]) },
description: None,
depfile: None,
deps_format: None,
pool: None,
restat: true,
},
"cf8e97357820acf6f66037dcf977ee36c88c2811d60342db30c99507d24a0d60"
)]
#[case(
Action {
recipe: Recipe::Command { command: "".into() },
description: None,
depfile: None,
deps_format: None,
pool: None,
restat: false,
},
// The expected hash value must be updated to match the actual output of ActionHasher::hash for this input.
"b1e7e2e2e2e2e2e2e2e2e2e2e2e2e2e2e2e2e2e2e2e2e2e2e2e2e2e2e2e2e2e2"
)]
#[case(
Action {
recipe: Recipe::Rule { rule: StringOrList::List(vec![]) },
description: None,
depfile: None,
deps_format: None,
pool: None,
restat: false,
},
// The expected hash value must be updated to match the actual output of ActionHasher::hash for this input.
"c2f7f7f7f7f7f7f7f7f7f7f7f7f7f7f7f7f7f7f7f7f7f7f7f7f7f7f7f7f7f7f7"
)]
#[case(
Action {
recipe: Recipe::Command { command: "特殊字符!@#$%^&*()".into() },
description: Some("desc\nwith\nnewlines".into()),
depfile: Some("".into()),
deps_format: None,
pool: None,
restat: false,
},
// The expected hash value must be updated to match the actual output of ActionHasher::hash for this input.
"d3c8c8c8c8c8c8c8c8c8c8c8c8c8c8c8c8c8c8c8c8c8c8c8c8c8c8c8c8c8c8c8"
)]
fn hash_action_is_stable(#[case] action: Action, #[case] expected: &str) {
assert_eq!(ActionHasher::hash(&action), expected);
}
```
- You must update the expected hash values for the new test cases to match the actual output of `ActionHasher::hash` for those inputs. The placeholder values ("b1e7...", "c2f7...", "d3c8...") should be replaced with the real hashes.
- If the original hash stability test is still present in `src/hasher.rs`, remove or refactor it to avoid duplication, keeping all hash stability tests in `tests/hasher_tests.rs`.
</issue_to_address>
### Comment 3
<location> `src/ir.rs:74` </location>
<code_context>
-use sha2::{Digest, Sha256};
use thiserror::Error;
+use crate::hasher::ActionHasher;
+
/// Errors produced during IR generation.
</code_context>
<issue_to_address>
No module-level `//!` doc comment at the top of this file.
Please add a `//!` doc comment at the very top of the file to describe the module, as required by the review instructions.
</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
♻️ Duplicate comments (3)
tests/hasher_tests.rs (1)
8-33: Address the test duplication flagged in previous reviews.The parameterised test structure is good, but this duplicates tests in
src/hasher.rs. Consolidate all hash stability tests in this file and expand coverage to include edge cases such as empty strings, empty lists, special characters, and all optional fields set toNone.Verify the duplication and remove tests from the source module:
#!/bin/bash # Check for duplicate hash stability tests in src/hasher.rs rg -A 10 -B 2 "hash_action_is_stable" src/hasher.rssrc/hasher.rs (1)
98-131: Consolidate duplicate tests with the external test file.The unit tests duplicate those in
tests/hasher_tests.rs. Move all hash stability tests to the external test file to avoid duplication and maintain better test organisation.Remove these tests from the source module:
-#[cfg(test)] -mod tests { - use super::*; - use crate::ast::{Recipe, StringOrList}; - use crate::ir::Action; - use rstest::rstest; - - #[rstest] - #[case( - Action { - recipe: Recipe::Command { command: "echo".into() }, - description: Some("desc".into()), - depfile: Some("$out.d".into()), - deps_format: Some("gcc".into()), - pool: None, - restat: false, - }, - "a0f6e2cd3b9b3cee0bf94a7d53bce56cf4178dfe907bb1cb7c832f47846baf38" - )] - #[case( - Action { - recipe: Recipe::Rule { rule: StringOrList::List(vec!["a".into(), "b".into()]) }, - description: None, - depfile: None, - deps_format: None, - pool: None, - restat: true, - }, - "cf8e97357820acf6f66037dcf977ee36c88c2811d60342db30c99507d24a0d60" - )] - fn hash_action_is_stable(#[case] action: Action, #[case] expected: &str) { - assert_eq!(ActionHasher::hash(&action), expected); - } -}src/ir.rs (1)
134-147: Handle empty rules case explicitlyThe logic for handling multiple rules is now more explicit. However, when
to_strings(rule)returns an empty list, this indicates an unexpected Empty rule and could lead to unclear error messages.Add explicit handling for the empty rules case as previously suggested.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (6)
src/hasher.rs(1 hunks)src/ir.rs(6 hunks)src/lib.rs(1 hunks)tests/data/duplicate_outputs_multi.yml(1 hunks)tests/hasher_tests.rs(1 hunks)tests/ir_from_manifest_tests.rs(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.rs
📄 CodeRabbit Inference Engine (AGENTS.md)
**/*.rs: Clippy warnings MUST be disallowed.
Fix any warnings emitted during tests in the code itself rather than silencing them.
Where a function is too long, extract meaningfully named helper functions adhering to separation of concerns and CQRS.
Where a function has too many parameters, group related parameters in meaningfully named structs.
Where a function is returning a large error consider usingArcto reduce the amount of data returned.
Write unit and behavioural tests for new functionality. Run both before and after making any change.
Every module must begin with a module level (//!) comment explaining the module's purpose and utility.
Document public APIs using Rustdoc comments (///) so documentation can be generated with cargo doc.
Prefer immutable data and avoid unnecessarymutbindings.
Handle errors with theResulttype instead of panicking where feasible.
Avoidunsafecode unless absolutely necessary and document any usage clearly.
Place function attributes after doc comments.
Do not usereturnin single-line functions.
Use predicate functions for conditional criteria with more than two branches.
Lints must not be silenced except as a last resort.
Lint rule suppressions must be tightly scoped and include a clear reason.
Preferexpectoverallow.
Prefer.expect()over.unwrap().
Useconcat!()to combine long string literals rather than escaping newlines with a backslash.
Prefer semantic error enums: Derivestd::error::Error(via thethiserrorcrate) for any condition the caller might inspect, retry, or map to an HTTP status.
Use an opaque error only at the app boundary: Useeyre::Reportfor human-readable logs; these should not be exposed in public APIs.
Never export the opaque type from a library: Convert to domain enums at API boundaries, and toeyreonly in the mainmain()entrypoint or top-level async task.
Files:
src/lib.rstests/hasher_tests.rssrc/hasher.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:
src/lib.rstests/hasher_tests.rssrc/hasher.rstests/ir_from_manifest_tests.rssrc/ir.rs
🧬 Code Graph Analysis (2)
tests/ir_from_manifest_tests.rs (2)
src/manifest.rs (1)
from_path(51-56)src/ir.rs (1)
from_manifest(102-111)
src/ir.rs (1)
src/hasher.rs (1)
hash(37-42)
🪛 GitHub Check: build-test
tests/ir_from_manifest_tests.rs
[warning] 48-48:
Diff in /home/runner/work/netsuke/netsuke/tests/ir_from_manifest_tests.rs
🪛 GitHub Actions: CI
tests/ir_from_manifest_tests.rs
[error] 48-52: cargo fmt check failed due to formatting differences. Run 'cargo fmt --all' to fix code style issues.
🔇 Additional comments (12)
tests/data/duplicate_outputs_multi.yml (1)
1-17: LGTM!The test data correctly sets up a scenario with overlapping outputs (
foo.oandbar.o) across two targets, which will properly trigger theDuplicateOutputerror for testing enhanced error reporting.src/lib.rs (1)
8-8: LGTM!The hasher module is properly exposed as part of the public API, correctly positioned alphabetically, and aligns with the PR's objective of modularising hashing functionality.
tests/ir_from_manifest_tests.rs (2)
36-42: LGTM!The enhanced error testing now validates specific error content (
outputsfield), providing better test coverage than simple variant matching.
57-67: LGTM!The new test correctly validates the multiple duplicate outputs scenario using the test data from
duplicate_outputs_multi.yml, ensuring proper error reporting for complex duplicate output cases.src/hasher.rs (3)
1-25: LGTM!Excellent module documentation with clear purpose, utility explanation, and practical example. The documentation follows Rust conventions and meets the coding guidelines requirement for module-level comments.
31-42: LGTM!The public API design is clean with a static method and proper
#[must_use]annotation. The implementation delegates to well-structured helper methods maintaining separation of concerns.
44-96: LGTM!The hashing implementation is robust with length-prefixed encoding to prevent collisions, consistent type tags for Recipe variants, and systematic handling of optional fields. The encoding strategy ensures stable, deterministic hashes.
src/ir.rs (5)
74-74: Good separation of concernsMoving the hashing logic to a dedicated module improves code organization and maintainability.
85-92: Improved error diagnosticsThe enhanced error variants now provide comprehensive lists of all problematic rules and outputs, significantly improving debugging capabilities.
164-174: Excellent improvement to duplicate detectionCollecting all duplicate outputs before reporting provides a much better user experience, allowing developers to fix all issues at once rather than iteratively.
202-202: Clean delegation to specialized hasherUsing the external
ActionHashermaintains the same functionality while improving modularity.
215-221: Well-structured helper functionThe
to_stringsfunction follows the established pattern fromto_pathsand correctly handles allStringOrListvariants.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
tests/hasher_tests.rs (1)
8-33: Address test duplication and expand coverage as suggested in previous review.The past review comment correctly identified that these tests duplicate those in
src/hasher.rs. Follow the previous suggestion to consolidate tests here and add edge cases for comprehensive coverage including empty fields, special characters, and boundary conditions.src/hasher.rs (1)
98-131: Remove embedded tests to eliminate duplication.These tests duplicate those in
tests/hasher_tests.rs. Remove them here and keep all hash stability tests in the dedicated test file as suggested in the previous review.src/ir.rs (1)
134-147: Handle empty rule lists explicitly.The current implementation doesn't distinguish between empty rules and multiple rules, which could lead to unclear error messages when
to_strings(rule)returns an empty list.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (6)
src/hasher.rs(1 hunks)src/ir.rs(6 hunks)src/lib.rs(1 hunks)tests/data/duplicate_outputs_multi.yml(1 hunks)tests/hasher_tests.rs(1 hunks)tests/ir_from_manifest_tests.rs(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.rs
📄 CodeRabbit Inference Engine (AGENTS.md)
**/*.rs: Clippy warnings MUST be disallowed.
Fix any warnings emitted during tests in the code itself rather than silencing them.
Where a function is too long, extract meaningfully named helper functions adhering to separation of concerns and CQRS.
Where a function has too many parameters, group related parameters in meaningfully named structs.
Where a function is returning a large error consider usingArcto reduce the amount of data returned.
Write unit and behavioural tests for new functionality. Run both before and after making any change.
Every module must begin with a module level (//!) comment explaining the module's purpose and utility.
Document public APIs using Rustdoc comments (///) so documentation can be generated with cargo doc.
Prefer immutable data and avoid unnecessarymutbindings.
Handle errors with theResulttype instead of panicking where feasible.
Avoidunsafecode unless absolutely necessary and document any usage clearly.
Place function attributes after doc comments.
Do not usereturnin single-line functions.
Use predicate functions for conditional criteria with more than two branches.
Lints must not be silenced except as a last resort.
Lint rule suppressions must be tightly scoped and include a clear reason.
Preferexpectoverallow.
Prefer.expect()over.unwrap().
Useconcat!()to combine long string literals rather than escaping newlines with a backslash.
Prefer semantic error enums: Derivestd::error::Error(via thethiserrorcrate) for any condition the caller might inspect, retry, or map to an HTTP status.
Use an opaque error only at the app boundary: Useeyre::Reportfor human-readable logs; these should not be exposed in public APIs.
Never export the opaque type from a library: Convert to domain enums at API boundaries, and toeyreonly in the mainmain()entrypoint or top-level async task.
Files:
src/lib.rstests/hasher_tests.rssrc/hasher.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:
src/lib.rstests/hasher_tests.rssrc/hasher.rstests/ir_from_manifest_tests.rssrc/ir.rs
🧬 Code Graph Analysis (2)
tests/ir_from_manifest_tests.rs (2)
src/manifest.rs (1)
from_path(51-56)src/ir.rs (1)
from_manifest(102-111)
src/ir.rs (1)
src/hasher.rs (1)
hash(37-42)
🪛 GitHub Check: build-test
tests/ir_from_manifest_tests.rs
[warning] 48-48:
Diff in /home/runner/work/netsuke/netsuke/tests/ir_from_manifest_tests.rs
🪛 GitHub Actions: CI
tests/ir_from_manifest_tests.rs
[error] 48-52: cargo fmt formatting check failed. Code style differences detected; run 'cargo fmt --all' to fix.
🔇 Additional comments (12)
tests/data/duplicate_outputs_multi.yml (1)
1-17: Test data file is well-structured for duplicate output testing.The YAML configuration correctly sets up the scenario for testing duplicate output detection with multiple outputs in different orders. This supports comprehensive testing of the enhanced error reporting functionality.
src/lib.rs (1)
8-8: Public module addition is appropriate.Exposing the hasher module as part of the public API allows external access to the ActionHasher functionality, which is sensible for a utility module.
tests/ir_from_manifest_tests.rs (3)
36-42: Enhanced error assertions improve test precision.The detailed field assertions provide better test coverage by verifying the specific error content rather than just the error variant.
57-67: New test case enhances coverage for multiple duplicate outputs.The
duplicate_outputs_multi_listedtest effectively validates the enhanced error reporting for scenarios with multiple duplicate outputs, using the new test data file.
48-55: Apply rustfmt to tests/ir_from_manifest_tests.rsRun
cargo fmt --allto resolve formatting errors around thematchstatement in themultiple_rules_per_target_failstest (lines 48–55). Then re-runcargo fmt --all -- --checkto confirm the pipeline passes.
- File: tests/ir_from_manifest_tests.rs
- Target:
multiple_rules_per_target_failsmatch statementsrc/hasher.rs (2)
1-25: Excellent module documentation follows coding guidelines.The module-level documentation with examples and clear purpose statement adheres perfectly to the coding guideline requirement for module documentation.
31-96: Well-designed hashing implementation with stable serialisation.The ActionHasher implementation provides stable hashing with proper discriminants for recipe variants and length-prefixed serialisation for strings. The approach ensures hash stability across different Action configurations.
src/ir.rs (5)
74-74: Import looks good.The
ActionHasherimport properly supports the refactored hashing functionality.
85-92: Enhanced error variants provide better diagnostics.The updated error variants now capture all conflicting rules and duplicate outputs, improving debugging capabilities.
215-221: Well-structured helper function.The
to_stringsfunction follows the established pattern ofto_pathsand correctly handles allStringOrListvariants.
164-174: Improved duplicate detection logic.The implementation now correctly aggregates all duplicate outputs before reporting, providing comprehensive error information.
202-202: Clean delegation to dedicated hasher module.The refactoring properly separates hashing concerns while maintaining the same interface.
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 | |
|---|---|---|---|
| ir_from_manifest_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.
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 | |
|---|---|---|---|
| ir_from_manifest_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.
There was a problem hiding this comment.
Actionable comments posted: 4
📜 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 (3)
Cargo.toml(1 hunks)src/hasher.rs(1 hunks)tests/ir_from_manifest_tests.rs(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
Cargo.toml
📄 CodeRabbit Inference Engine (AGENTS.md)
Cargo.toml: Use explicit version ranges inCargo.tomland keep dependencies up-to-date.
Mandate caret requirements for all dependencies: All crate versions specified inCargo.tomlmust use SemVer-compatible caret requirements (e.g.,some-crate = "1.2.3").
Prohibit unstable version specifiers: The use of wildcard (*) or open-ended inequality (>=) version requirements is strictly forbidden. Tilde requirements (~) should only be used where a dependency must be locked to patch-level updates for a specific, documented reason.
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 usingArcto reduce the amount of data returned.
Write unit and behavioural tests for new functionality. Run both before and after making any change.
Every module must begin with a module level (//!) comment explaining the module's purpose and utility.
Document public APIs using Rustdoc comments (///) so documentation can be generated with cargo doc.
Prefer immutable data and avoid unnecessarymutbindings.
Handle errors with theResulttype instead of panicking where feasible.
Avoidunsafecode unless absolutely necessary and document any usage clearly.
Place function attributes after doc comments.
Do not usereturnin single-line functions.
Use predicate functions for conditional criteria with more than two branches.
Lints must not be silenced except as a last resort.
Lint rule suppressions must be tightly scoped and include a clear reason.
Preferexpectoverallow.
Prefer.expect()over.unwrap().
Useconcat!()to combine long string literals rather than escaping newlines with a backslash.
Prefer semantic error enums: Derivestd::error::Error(via thethiserrorcrate) for any condition the caller might inspect, retry, or map to an HTTP status.
Use an opaque error only at the app boundary: Useeyre::Reportfor human-readable logs; these should not be exposed in public APIs.
Never export the opaque type from a library: Convert to domain enums at API boundaries, and toeyreonly in the mainmain()entrypoint or top-level async task.
Files:
tests/ir_from_manifest_tests.rssrc/hasher.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/hasher.rs
🧬 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(105-114)
🔇 Additional comments (6)
Cargo.toml (1)
14-14: LGTM! Dependency addition follows guidelines.The
itoadependency uses proper caret requirements and supports the new hashing functionality efficiently.src/hasher.rs (4)
1-31: Excellent module documentation and structure.The module follows all coding guidelines with comprehensive documentation, proper examples, and clear purpose statement.
35-43: Sound public API design.The
hashmethod correctly uses#[must_use]and provides a clean interface for stable Action hashing. The implementation properly delegates to helper methods for maintainability.
45-60: Robust recipe hashing with stable discriminants.The discriminant tags ("cmd", "scr", "rule") ensure hash stability across different Recipe variants, and the delegation to specific handlers maintains clean separation of concerns.
92-99: Efficient length-prefixed hashing implementation.The use of
itoa::Bufferavoids heap allocation whilst providing stable serialisation. This addresses the performance optimisation from previous reviews.tests/ir_from_manifest_tests.rs (1)
38-80: Excellent test refactoring using rstest.The parameterised test successfully eliminates code duplication whilst maintaining comprehensive error case coverage. The approach follows coding guidelines perfectly.
|
@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.
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location> `tests/hasher_tests.rs:64` </location>
<code_context>
- },
- "cf8e97357820acf6f66037dcf977ee36c88c2811d60342db30c99507d24a0d60"
- )]
- fn hash_action_is_stable(#[case] action: Action, #[case] expected: &str) {
- assert_eq!(hash_action(&action), expected);
- }
</code_context>
<issue_to_address>
Consider adding a test for field order invariance if applicable.
If field order should not affect the hash, please add a test to confirm this. Otherwise, clarify in a comment that order is significant.
</issue_to_address>
### Comment 2
<location> `tests/ir_from_manifest_tests.rs:61` </location>
<code_context>
+ let manifest = manifest::from_path(manifest_path).expect("load");
let err = BuildGraph::from_manifest(&manifest).expect_err("error");
- assert!(matches!(err, IrGenError::MultipleRules { .. }));
+ match (err, expected) {
+ (IrGenError::DuplicateOutput { outputs }, ExpectedError::DuplicateOutput(exp_outputs)) => {
+ assert_eq!(outputs, exp_outputs);
+ }
+ (
+ IrGenError::MultipleRules { target_name, rules },
+ ExpectedError::MultipleRules {
+ target,
+ rules: exp_rules,
+ },
+ ) => {
+ assert_eq!(target_name, target);
+ assert_eq!(rules, exp_rules);
+ }
+ (IrGenError::EmptyRule { target_name }, ExpectedError::EmptyRule(exp_target)) => {
+ assert_eq!(target_name, exp_target);
+ }
+ (other, _) => panic!("wrong error: {other:?}"),
+ }
}
</code_context>
<issue_to_address>
Consider adding a test for the RuleNotFound error variant.
Adding RuleNotFound to manifest_error_cases would improve test consistency and coverage.
Suggested implementation:
```rust
#[case(
"tests/data/empty_rule.yml",
ExpectedError::EmptyRule("hello.o".into())
)]
#[case(
"tests/data/rule_not_found.yml",
ExpectedError::RuleNotFound("missing_rule".into())
)]
fn manifest_error_cases(#[case] manifest_path: &str, #[case] expected: ExpectedError) {
let manifest = manifest::from_path(manifest_path).expect("load");
let err = BuildGraph::from_manifest(&manifest).expect_err("error");
```
```rust
match (err, expected) {
(IrGenError::DuplicateOutput { outputs }, ExpectedError::DuplicateOutput(exp_outputs)) => {
assert_eq!(outputs, exp_outputs);
}
(
IrGenError::MultipleRules { target_name, rules },
ExpectedError::MultipleRules {
target,
rules: exp_rules,
},
) => {
assert_eq!(target_name, target);
assert_eq!(rules, exp_rules);
}
(IrGenError::EmptyRule { target_name }, ExpectedError::EmptyRule(exp_target)) => {
assert_eq!(target_name, exp_target);
}
(IrGenError::RuleNotFound { rule_name }, ExpectedError::RuleNotFound(exp_rule)) => {
assert_eq!(rule_name, exp_rule);
}
(other, _) => panic!("wrong error: {other:?}"),
}
}
```
```rust
enum ExpectedError {
DuplicateOutput(Vec<String>),
MultipleRules { target: String, rules: Vec<String> },
EmptyRule(String),
RuleNotFound(String),
```
You will need to create a test manifest file at `tests/data/rule_not_found.yml` that triggers the `RuleNotFound` error for the rule named "missing_rule".
</issue_to_address>
### Comment 3
<location> `src/ir.rs:105` </location>
<code_context>
- /// rules are specified for a single target.
+ /// Returns [`IrGenError`] when a referenced rule is missing, multiple rules
+ /// are specified for a single target, or no rule is provided.
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 extracting rule resolution and duplicate output detection into separate helper functions to simplify the main target processing loop.
Here’s one way to collapse the branching/loops in `process_targets` into two tiny helpers—one for rule‐resolution and one to find duplicate outputs—then your main loop just wires them up:
```rust
// new helper at bottom of file
fn resolve_rule(
rule: &StringOrList,
rule_map: &HashMap<String, String>,
target_name: &str,
) -> Result<String, IrGenError> {
// turn either String or List into Vec<String>
let mut names = to_string_vec(rule);
match names.len() {
0 => Err(IrGenError::EmptyRule { target_name: target_name.to_string() }),
1 => {
let name = names.pop().unwrap();
rule_map.get(&name)
.cloned()
.ok_or_else(|| IrGenError::RuleNotFound {
target_name: target_name.to_string(),
rule_name: name
})
}
_ => {
names.sort();
Err(IrGenError::MultipleRules {
target_name: target_name.to_string(),
rules: names
})
}
}
}
fn find_duplicates(
outputs: &[PathBuf],
targets: &HashMap<PathBuf, BuildEdge>,
) -> Option<Vec<String>> {
let mut dups: Vec<_> = outputs.iter()
.filter(|o| targets.contains_key(*o))
.map(|o| o.display().to_string())
.collect();
if dups.is_empty() { None } else { dups.sort(); Some(dups) }
}
```
Then your loop becomes:
```rust
for target in manifest.actions.iter().chain(&manifest.targets) {
let outputs = to_paths(&target.name);
let target_name = get_target_display_name(&outputs);
let action_id = match &target.recipe {
Recipe::Rule { rule } => resolve_rule(rule, rule_map, &target_name)?,
Recipe::Command { .. } | Recipe::Script { .. } => {
register_action(actions, target.recipe.clone(), None)
}
};
if let Some(dups) = find_duplicates(&outputs, targets) {
return Err(IrGenError::DuplicateOutput { outputs: dups });
}
let edge = BuildEdge { /* … */ };
for o in outputs {
targets.insert(o, edge.clone());
}
}
```
This
- removes the inline `if let Some` + big `else { … }` in‐place
- collapses the two “duplicate” loops into one small helper
- keeps full behavior (sorting + distinct errors) intact
- flattens control‐flow so `process_targets` is just high‐level steps.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
1 participant
Summary
Testing
make lintmake testhttps://chatgpt.com/codex/tasks/task_e_688669648cc48322a62a9cc0ca1eb07c
Summary by Sourcery
Introduce ActionHasher module for stable action hashing, enhance IR error variants to report multiple duplicate outputs, missing and empty rules, refactor IR generation to use the new hasher and improved error handling, and update tests and dependencies accordingly.
New Features:
Enhancements:
Build:
Tests: