Tightens lint policy, documents API, and hardens runtime paths#222
Tightens lint policy, documents API, and hardens runtime paths#222
Conversation
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
|
Note Reviews pausedUse the following commands to manage reviews:
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. Summary by CodeRabbit
WalkthroughSummarise the PR: convert many panic/expect paths into Result-based error handling with contextual errors, enable stricter Clippy/Rust lints, add MacroDefinition and manifest type aliases, add Action.restat, refactor ninja rendering and runner I/O for streaming, and migrate a large test and test-support surface to fallible APIs. Changes
Sequence Diagram(s)sequenceDiagram
%%{init: {"themeVariables":{"actorBkg":"#f6f9ff","noteBkg":"#fff7e6"}}}%%
participant CLI as CLI (user)
participant Resolve as resolve_manifest_path()
participant Parser as parse_manifest / generate_ir
participant NinjaGen as generate_ninja()
participant Runner as run_ninja / file I/O
CLI->>Resolve: call resolve_manifest_path(cli)
Note right of Resolve: returns Result<Utf8PathBuf>
Resolve-->>CLI: Ok(path) / Err(err)
alt Ok(path)
CLI->>Parser: parse manifest and build IR
Parser-->>CLI: Ok(build_graph) / Err(parse_err)
alt Ok(build_graph)
CLI->>NinjaGen: generate_ninja(build_graph)
NinjaGen-->>CLI: Ok(ninja_content) / Err(ninja_err)
alt Ok(ninja_content)
CLI->>Runner: run_ninja(ninja_content) -- spawn forwarders, stream stdout/stderr
Runner-->>CLI: Ok() / Err(io_err)
else Err(ninja_err)
CLI->>CLI: record ninja_error and return Err
end
else Err(parse_err)
CLI->>CLI: record manifest_error and return Err
end
else Err(err)
CLI->>CLI: propagate path resolution Err
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Assess breadth: heterogeneous, cross-cutting edits touching public APIs (Action.restat, MacroDefinition, ManifestValue/Map), runner/process concurrency and streaming, macro-instance safety, extensive test-support and BDD step surface, and broad lint configuration. Possibly related PRs
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
Comment |
![codescene-delta-analysis[bot]](https://avatars.githubusercontent.com/in/53921?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 63
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
tests/std_filter_tests/support.rs (1)
21-30: Approve variable renaming; improve expect message.The refactored variable names
template_nameandtemplate_sourceimprove clarity over the previous implementation.Enhance the expect message to be more descriptive:
env.add_template_owned(template_name, template_source) - .expect("template"); + .expect("failed to register template in MiniJinja environment");src/manifest/expand.rs (2)
31-36: Preallocateitemsto avoid growth churn.Compute capacity before consuming
valuesand initialise withVec::with_capacity(capacity). No behaviour change; small allocation win.- let mut items = Vec::new(); - for (index, item) in values.into_iter().enumerate() { + let capacity = values.len(); + let mut items = Vec::with_capacity(capacity); + for (index, item) in values.into_iter().enumerate() {
77-95: Eliminate unreachable error path and typevarsdirectly.The earlier match guarantees an object; the follow-up
as_object_mut().ok_or_else(..)cannot fail and the message is misleading. Match to&mut ManifestMaponce and improve the invariant message.- let vars_value = match map.entry("vars") { - Entry::Vacant(slot) => slot.insert(ManifestValue::Object(ManifestMap::new())), - Entry::Occupied(slot) => { - let value = slot.into_mut(); - match value { - ManifestValue::Object(_) => value, - other => { - return Err(anyhow::anyhow!( - "target.vars must be an object, got: {other:?}" - )); - } - } - } - }; - - let vars = vars_value - .as_object_mut() - .ok_or_else(|| anyhow::anyhow!("vars entry ensured to be an object"))?; + let vars: &mut ManifestMap = match map.entry("vars") { + Entry::Vacant(slot) => match slot.insert(ManifestValue::Object(ManifestMap::new())) { + ManifestValue::Object(ref mut m) => m, + _ => return Err(anyhow::anyhow!( + "internal invariant violated: inserted vars was not an object" + )), + }, + Entry::Occupied(slot) => match slot.into_mut() { + ManifestValue::Object(ref mut m) => m, + other => return Err(anyhow::anyhow!( + "target.vars must be an object, got: {other:?}" + )), + }, + };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (68)
Cargo.toml(1 hunks)build.rs(2 hunks)clippy.toml(1 hunks)src/ast.rs(3 hunks)src/cli.rs(1 hunks)src/ir/cmd_interpolate.rs(2 hunks)src/ir/cycle.rs(1 hunks)src/ir/from_manifest.rs(5 hunks)src/ir/graph.rs(7 hunks)src/main.rs(1 hunks)src/manifest/diagnostics.rs(8 hunks)src/manifest/expand.rs(5 hunks)src/manifest/glob.rs(5 hunks)src/manifest/jinja_macros.rs(6 hunks)src/manifest/mod.rs(2 hunks)src/manifest/render.rs(2 hunks)src/manifest/tests.rs(8 hunks)src/ninja_gen.rs(3 hunks)src/runner/mod.rs(5 hunks)src/runner/process/file_io.rs(1 hunks)src/runner/process/mod.rs(4 hunks)src/runner/process/redaction.rs(1 hunks)src/stdlib/collections.rs(5 hunks)src/stdlib/command.rs(6 hunks)src/stdlib/mod.rs(3 hunks)src/stdlib/network.rs(2 hunks)src/stdlib/path/filters.rs(3 hunks)src/stdlib/path/hash_utils.rs(2 hunks)src/stdlib/path/io_helpers.rs(1 hunks)src/stdlib/path/path_utils.rs(3 hunks)src/stdlib/time/mod.rs(1 hunks)src/stdlib/time/tests.rs(3 hunks)test_support/src/http.rs(1 hunks)test_support/src/lib.rs(1 hunks)tests/assert_cmd_tests.rs(1 hunks)tests/ast_tests.rs(5 hunks)tests/cli_tests.rs(1 hunks)tests/command_escaping_tests.rs(1 hunks)tests/cucumber.rs(4 hunks)tests/env_path_tests.rs(1 hunks)tests/hasher_tests.rs(1 hunks)tests/ir_from_manifest_tests.rs(1 hunks)tests/ir_tests.rs(3 hunks)tests/manifest_env_tests.rs(1 hunks)tests/manifest_glob_tests.rs(1 hunks)tests/manifest_jinja_tests.rs(6 hunks)tests/ninja_env_tests.rs(1 hunks)tests/ninja_gen_tests.rs(2 hunks)tests/ninja_snapshot_tests.rs(2 hunks)tests/path_guard_tests.rs(1 hunks)tests/runner_tests.rs(1 hunks)tests/std_filter_tests/collection_filters.rs(1 hunks)tests/std_filter_tests/command_filters.rs(1 hunks)tests/std_filter_tests/hash_filters.rs(1 hunks)tests/std_filter_tests/io_filters.rs(1 hunks)tests/std_filter_tests/mod.rs(1 hunks)tests/std_filter_tests/network_functions.rs(3 hunks)tests/std_filter_tests/path_filters.rs(1 hunks)tests/std_filter_tests/support.rs(2 hunks)tests/steps/cli_steps.rs(6 hunks)tests/steps/fs_steps.rs(2 hunks)tests/steps/ir_steps.rs(1 hunks)tests/steps/manifest_steps.rs(4 hunks)tests/steps/mod.rs(1 hunks)tests/steps/ninja_steps.rs(3 hunks)tests/steps/process_steps.rs(3 hunks)tests/steps/stdlib_steps.rs(6 hunks)tests/yaml_error_tests.rs(1 hunks)
🧰 Additional context used
📓 Path-based instructions (7)
**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
**/*.rs: Every Rust module must begin with a module-level comment using //! explaining the module's purpose and utility
Document public APIs with Rustdoc comments (///) so documentation can be generated with cargo doc
Prefer immutable data; avoid unnecessary mut bindings
Handle errors with Result instead of panicking where feasible
Prefer semantic error enums deriving std::error::Error via thiserror for inspectable conditions
Clippy warnings must be denied; do not silence lints except as a last resort
Lint suppressions must be tightly scoped and include a clear reason; prefer #[expect(...)] over #[allow(...)]
Place function attributes after doc comments
Do not use return in single-line functions
Prefer single-line function definitions where appropriate (e.g., pub fn new(id: u64) -> Self { Self(id) })
Use predicate functions for conditional criteria with more than two branches
Where a function is too long, extract meaningfully named helpers adhering to separation of concerns and CQRS
Where a function has too many parameters, group related parameters into meaningfully named structs
When returning large error data, consider using Arc to reduce data moved
Prefer .expect() over .unwrap() on Results/Options
Use concat!() to combine long string literals instead of escaping newlines with backslashes
Use NewTypes to model domain values; prefer explicit tuple structs; leverage newt-hype and the-newtype crates as described; provide appropriate From/AsRef/TryFrom implementations and avoid orphan-rule-violating impls
Keep each Rust code file under 400 lines; split long switches/dispatch tables by feature; move large test data to external files
Use functions and composition to avoid repetition; extract reusable logic; prefer declarative constructs (iterators/generators/comprehensions) when readable
Small, single-responsibility functions; obey command/query segregation
Name things precisely; booleans should use is/has/should prefixes
Comment why, not what; explain assumptions, e...
Files:
src/stdlib/path/io_helpers.rstests/manifest_env_tests.rstests/steps/fs_steps.rstests/runner_tests.rstests/yaml_error_tests.rstests/std_filter_tests/collection_filters.rstests/hasher_tests.rstests/steps/process_steps.rstests/path_guard_tests.rstests/std_filter_tests/network_functions.rstests/command_escaping_tests.rstests/manifest_glob_tests.rssrc/stdlib/path/hash_utils.rssrc/stdlib/time/tests.rssrc/stdlib/collections.rstests/ninja_gen_tests.rstests/std_filter_tests/path_filters.rssrc/manifest/render.rssrc/stdlib/mod.rstests/assert_cmd_tests.rssrc/stdlib/network.rstests/std_filter_tests/command_filters.rssrc/manifest/tests.rssrc/main.rstests/steps/ir_steps.rstests/env_path_tests.rssrc/manifest/expand.rssrc/manifest/mod.rstests/std_filter_tests/io_filters.rssrc/runner/process/file_io.rstests/std_filter_tests/support.rstests/steps/mod.rstests/std_filter_tests/hash_filters.rssrc/ir/cmd_interpolate.rstests/manifest_jinja_tests.rstest_support/src/lib.rssrc/manifest/diagnostics.rssrc/ast.rstests/ir_tests.rssrc/stdlib/path/filters.rssrc/stdlib/path/path_utils.rssrc/ir/cycle.rstests/ninja_snapshot_tests.rstests/ninja_env_tests.rstests/steps/stdlib_steps.rsbuild.rssrc/runner/process/mod.rstests/ir_from_manifest_tests.rstest_support/src/http.rssrc/manifest/glob.rssrc/cli.rssrc/stdlib/time/mod.rstests/ast_tests.rstests/cli_tests.rssrc/stdlib/command.rssrc/manifest/jinja_macros.rstests/steps/ninja_steps.rssrc/ir/graph.rssrc/ninja_gen.rstests/std_filter_tests/mod.rssrc/runner/mod.rstests/cucumber.rstests/steps/manifest_steps.rssrc/runner/process/redaction.rssrc/ir/from_manifest.rstests/steps/cli_steps.rs
⚙️ CodeRabbit configuration file
**/*.rs: * Seek to keep the cyclomatic complexity of functions no more than 12.
Adhere to single responsibility and CQRS
Place function attributes after doc comments.
Do not use
returnin single-line functions.Move conditionals with >2 branches into a predicate function.
Avoid
unsafeunless absolutely necessary.Every module must begin with a
//!doc comment that explains the module's purpose and utility.Comments and docs must follow en-GB-oxendict (-ize / -yse / -our) spelling and grammar
Lints must not be silenced except as a last resort.
#[allow]is forbidden.- Only narrowly scoped
#[expect(lint, reason = "...")]is allowed.- No lint groups, no blanket or file-wide suppression.
- Include
FIXME:with link if a fix is expected.Where code is only used by specific features, it must be conditionally compiled or a conditional expectation for unused_code applied.
Use
rstestfixtures for shared setup and to avoid repetition between tests.Replace duplicated tests with
#[rstest(...)]parameterised cases.Prefer
mockallfor mocks/stubs.Prefer
.expect()over.unwrap()Ensure that any API or behavioural changes are reflected in the documentation in
docs/Ensure that any completed roadmap steps are recorded in the appropriate roadmap in
docs/Files must not exceed 400 lines in length
- Large modules must be decomposed
- Long match statements or dispatch tables should be decomposed by domain and collocated with targets
- Large blocks of inline data (e.g., test fixtures, constants or templates) must be moved to external files and inlined at compile-time or loaded at run-time.
Environment access (env::set_var and env::remove_var) are always unsafe in Rust 2024 and MUST be marked as such
- For testing of functionality depending upon environment variables, dependency injection and the
mockablecrate are the preferred option.- If mockable cannot be used, env mutations in...
Files:
src/stdlib/path/io_helpers.rstests/manifest_env_tests.rstests/steps/fs_steps.rstests/runner_tests.rstests/yaml_error_tests.rstests/std_filter_tests/collection_filters.rstests/hasher_tests.rstests/steps/process_steps.rstests/path_guard_tests.rstests/std_filter_tests/network_functions.rstests/command_escaping_tests.rstests/manifest_glob_tests.rssrc/stdlib/path/hash_utils.rssrc/stdlib/time/tests.rssrc/stdlib/collections.rstests/ninja_gen_tests.rstests/std_filter_tests/path_filters.rssrc/manifest/render.rssrc/stdlib/mod.rstests/assert_cmd_tests.rssrc/stdlib/network.rstests/std_filter_tests/command_filters.rssrc/manifest/tests.rssrc/main.rstests/steps/ir_steps.rstests/env_path_tests.rssrc/manifest/expand.rssrc/manifest/mod.rstests/std_filter_tests/io_filters.rssrc/runner/process/file_io.rstests/std_filter_tests/support.rstests/steps/mod.rstests/std_filter_tests/hash_filters.rssrc/ir/cmd_interpolate.rstests/manifest_jinja_tests.rstest_support/src/lib.rssrc/manifest/diagnostics.rssrc/ast.rstests/ir_tests.rssrc/stdlib/path/filters.rssrc/stdlib/path/path_utils.rssrc/ir/cycle.rstests/ninja_snapshot_tests.rstests/ninja_env_tests.rstests/steps/stdlib_steps.rsbuild.rssrc/runner/process/mod.rstests/ir_from_manifest_tests.rstest_support/src/http.rssrc/manifest/glob.rssrc/cli.rssrc/stdlib/time/mod.rstests/ast_tests.rstests/cli_tests.rssrc/stdlib/command.rssrc/manifest/jinja_macros.rstests/steps/ninja_steps.rssrc/ir/graph.rssrc/ninja_gen.rstests/std_filter_tests/mod.rssrc/runner/mod.rstests/cucumber.rstests/steps/manifest_steps.rssrc/runner/process/redaction.rssrc/ir/from_manifest.rstests/steps/cli_steps.rs
**/*.{rs,md}
📄 CodeRabbit inference engine (AGENTS.md)
Use en-GB-oxendict spelling and grammar in comments and documentation (exceptions allowed for external API names)
Files:
src/stdlib/path/io_helpers.rstests/manifest_env_tests.rstests/steps/fs_steps.rstests/runner_tests.rstests/yaml_error_tests.rstests/std_filter_tests/collection_filters.rstests/hasher_tests.rstests/steps/process_steps.rstests/path_guard_tests.rstests/std_filter_tests/network_functions.rstests/command_escaping_tests.rstests/manifest_glob_tests.rssrc/stdlib/path/hash_utils.rssrc/stdlib/time/tests.rssrc/stdlib/collections.rstests/ninja_gen_tests.rstests/std_filter_tests/path_filters.rssrc/manifest/render.rssrc/stdlib/mod.rstests/assert_cmd_tests.rssrc/stdlib/network.rstests/std_filter_tests/command_filters.rssrc/manifest/tests.rssrc/main.rstests/steps/ir_steps.rstests/env_path_tests.rssrc/manifest/expand.rssrc/manifest/mod.rstests/std_filter_tests/io_filters.rssrc/runner/process/file_io.rstests/std_filter_tests/support.rstests/steps/mod.rstests/std_filter_tests/hash_filters.rssrc/ir/cmd_interpolate.rstests/manifest_jinja_tests.rstest_support/src/lib.rssrc/manifest/diagnostics.rssrc/ast.rstests/ir_tests.rssrc/stdlib/path/filters.rssrc/stdlib/path/path_utils.rssrc/ir/cycle.rstests/ninja_snapshot_tests.rstests/ninja_env_tests.rstests/steps/stdlib_steps.rsbuild.rssrc/runner/process/mod.rstests/ir_from_manifest_tests.rstest_support/src/http.rssrc/manifest/glob.rssrc/cli.rssrc/stdlib/time/mod.rstests/ast_tests.rstests/cli_tests.rssrc/stdlib/command.rssrc/manifest/jinja_macros.rstests/steps/ninja_steps.rssrc/ir/graph.rssrc/ninja_gen.rstests/std_filter_tests/mod.rssrc/runner/mod.rstests/cucumber.rstests/steps/manifest_steps.rssrc/runner/process/redaction.rssrc/ir/from_manifest.rstests/steps/cli_steps.rs
**/*{test,tests}.rs
📄 CodeRabbit inference engine (AGENTS.md)
Write unit and behavioural tests for new functionality and run them before and after changes
Files:
tests/manifest_env_tests.rstests/runner_tests.rstests/yaml_error_tests.rstests/hasher_tests.rstests/path_guard_tests.rstests/command_escaping_tests.rstests/manifest_glob_tests.rssrc/stdlib/time/tests.rstests/ninja_gen_tests.rstests/assert_cmd_tests.rssrc/manifest/tests.rstests/env_path_tests.rstests/manifest_jinja_tests.rstests/ir_tests.rstests/ninja_snapshot_tests.rstests/ninja_env_tests.rstests/ir_from_manifest_tests.rstests/ast_tests.rstests/cli_tests.rs
**/tests/**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
**/tests/**/*.rs: Use rstest fixtures for shared setup
Replace duplicated tests with #[rstest(...)] parameterised cases
Prefer mockall for mocks and stubs
Files:
tests/manifest_env_tests.rstests/steps/fs_steps.rstests/runner_tests.rstests/yaml_error_tests.rstests/std_filter_tests/collection_filters.rstests/hasher_tests.rstests/steps/process_steps.rstests/path_guard_tests.rstests/std_filter_tests/network_functions.rstests/command_escaping_tests.rstests/manifest_glob_tests.rstests/ninja_gen_tests.rstests/std_filter_tests/path_filters.rstests/assert_cmd_tests.rstests/std_filter_tests/command_filters.rstests/steps/ir_steps.rstests/env_path_tests.rstests/std_filter_tests/io_filters.rstests/std_filter_tests/support.rstests/steps/mod.rstests/std_filter_tests/hash_filters.rstests/manifest_jinja_tests.rstests/ir_tests.rstests/ninja_snapshot_tests.rstests/ninja_env_tests.rstests/steps/stdlib_steps.rstests/ir_from_manifest_tests.rstests/ast_tests.rstests/cli_tests.rstests/steps/ninja_steps.rstests/std_filter_tests/mod.rstests/cucumber.rstests/steps/manifest_steps.rstests/steps/cli_steps.rs
**/src/main.rs
📄 CodeRabbit inference engine (AGENTS.md)
Convert domain errors to eyre only in main() or top-level async tasks; keep opaque errors at the app boundary
Files:
src/main.rs
Cargo.toml
📄 CodeRabbit inference engine (AGENTS.md)
Cargo.toml: Mandate SemVer-compatible caret requirements (e.g., some-crate = "1.2.3") for all dependencies
Prohibit wildcard (*) and open-ended (>=) version requirements; use tilde (~) only with documented reasons to lock to patch updates
Use explicit version ranges and keep dependencies up to date
Files:
Cargo.toml
**/src/lib.rs
📄 CodeRabbit inference engine (AGENTS.md)
Never export the opaque eyre::Report type from libraries; expose semantic domain error enums instead
Files:
test_support/src/lib.rs
🧬 Code graph analysis (10)
tests/steps/process_steps.rs (1)
test_support/src/lib.rs (1)
fake_ninja(70-125)
tests/std_filter_tests/network_functions.rs (1)
test_support/src/http.rs (1)
spawn_http_server(55-69)
src/stdlib/path/hash_utils.rs (1)
src/hasher.rs (1)
write(38-41)
src/manifest/tests.rs (3)
src/manifest/jinja_macros.rs (1)
register_manifest_macros(100-113)test_support/src/http.rs (1)
spawn_http_server(55-69)test_support/src/hash.rs (1)
sha256_hex(18-26)
tests/std_filter_tests/support.rs (1)
tests/std_filter_tests/command_filters.rs (1)
env(39-41)
tests/manifest_jinja_tests.rs (3)
tests/command_escaping_tests.rs (1)
manifest_yaml(19-21)tests/manifest_env_tests.rs (1)
manifest_yaml(14-16)tests/manifest_glob_tests.rs (1)
manifest_yaml(37-39)
src/stdlib/path/filters.rs (2)
src/stdlib/path/path_utils.rs (1)
with_suffix(22-53)src/stdlib/path/hash_utils.rs (2)
compute_hash(19-54)compute_digest(55-61)
src/runner/mod.rs (3)
src/manifest/diagnostics.rs (4)
new(20-22)new(69-71)as_str(26-28)as_str(75-77)src/runner/process/redaction.rs (2)
new(8-10)as_str(12-14)test_support/src/lib.rs (1)
resolve_manifest_path(174-189)
tests/cucumber.rs (2)
test_support/src/env.rs (1)
restore_many(98-112)test_support/src/http.rs (1)
spawn_http_server(55-69)
tests/steps/cli_steps.rs (1)
tests/cli_tests.rs (1)
parse_cli(44-58)
⏰ Context from checks skipped due to timeout of 120000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)
- GitHub Check: release / build-macos (x86_64-apple-darwin, macos-13, macos-x86_64, macos-x86_64) / Build macos artefacts
- GitHub Check: release / build-linux (x86_64-unknown-linux-gnu, linux-amd64, amd64) / Build linux artefacts
- GitHub Check: release / build-linux (aarch64-unknown-linux-gnu, linux-arm64, arm64) / Build linux artefacts
- GitHub Check: release / build-macos (aarch64-apple-darwin, macos-14, macos-arm64, macos-aarch64) / Build macos artefacts
- GitHub Check: release / build-windows (aarch64-pc-windows-msvc, arm64, arm64, windows-aarch64) / Build windows artefacts
- GitHub Check: release / build-windows (x86_64-pc-windows-msvc, amd64, x64, windows-x86_64) / Build windows artefacts
- GitHub Check: build-test (ubuntu-latest, stable)
- GitHub Check: build-test (ubuntu-latest, 1.89.0)
🔇 Additional comments (94)
tests/steps/process_steps.rs (2)
36-38: LGTM! Parameter rename improves clarity.Renaming
codetoexit_codealigns with the parameter name intest_support::fake_ninjaand makes the intent explicit.
142-150: LGTM! Parameter rename clarifies intent.Renaming
expectedtoexpected_fragmentaccurately conveys that the assertion checks for a substring match rather than an exact match.src/stdlib/path/path_utils.rs (3)
15-15: Idiomatic &str conversion.The change from
.to_string()to.to_owned()is the correct choice for converting&strtoString. This avoids the unnecessary overhead of invoking theDisplaytrait and aligns with idiomatic Rust patterns.
57-57: Idiomatic &str conversion.The change from
.to_string()to.to_owned()correctly uses the more idiomatic conversion for&strtoString.
111-111: Idiomatic &str conversion.The change from
.to_string()to.to_owned()maintains consistency with the other conversions in this file and uses the more idiomatic approach for&strtoString.src/stdlib/path/filters.rs (4)
25-28: Improved clarity with extracted variables.Extracting
count_valueandsep_valueas intermediate variables improves readability and makes the function call on line 27 clearer. The variable names are more descriptive than inline unwrapping.
50-51: Improved naming and idiomatic conversion.The variable rename to
chosen_encodingis more descriptive and the change to.to_owned()is consistent with the idiomatic&strtoStringconversions applied inpath_utils.rs.
66-67: More descriptive variable naming.Renaming
algtoalgorithmimproves clarity and aligns with the coding guideline to "name things precisely".
73-75: More descriptive variable naming.Renaming
lentodigest_lenandalgtoalgorithmimproves clarity and maintains consistency with the changes in thehashfilter above.src/main.rs (1)
12-17: LGTM! Clean refactoring improves readability.Computing
max_levelbefore passing it to the formatter is clearer than inline conditional logic.tests/steps/fs_steps.rs (1)
112-112: LGTM!The change from
to_string()toto_owned()is functionally equivalent for string slices and maintains correct behaviour.src/stdlib/command.rs (5)
352-362: LGTM!The addition of structured logging for thread-join failures improves observability and aligns with the PR's goal of hardening runtime paths. The warnings will help diagnose issues that were previously silent.
421-425: LGTM!Guarding the
write_fmtcall prevents a panic during error-message construction, which aligns with the PR's objective of reducing runtime panics.
484-486: LGTM!Logging the wait failure after timeout improves diagnostics for edge cases where the child process cannot be reaped.
507-508: LGTM!Renaming to
join_handleimproves clarity.
516-517: LGTM!Renaming to
stderr_textclarifies that the value is the text representation after lossy UTF-8 conversion.src/stdlib/collections.rs (3)
31-31: Correct use ofto_owned()for string conversion.The change from
to_string()toto_owned()is idiomatic when converting&strtoString. Well done.
120-120: Correct use ofto_owned()for string literal conversion.The change from
to_string()toto_owned()is idiomatic and consistent with the other changes in this file.
178-178: Explicit formatting withformat!macro.The change from
value.to_string()toformat!("{value}")is functionally equivalent but more explicit about the formatting operation. Acceptable refactoring.Also applies to: 187-187
tests/std_filter_tests/mod.rs (1)
3-3: LGTM!The module-level documentation clearly explains the module's purpose and adheres to the requirement for module doc comments.
src/stdlib/network.rs (1)
188-190: Approve the defensive assertion.The guarded write with
debug_assert!appropriately catches an unlikely formatting failure whilst avoiding production panics. The silent fallback (incomplete hex string) is acceptable given that write-to-String failures for simple hex formatting are effectively impossible.src/stdlib/path/hash_utils.rs (1)
91-93: Approve the defensive assertion.The guarded write with
debug_assert!matches the pattern insrc/stdlib/network.rs(lines 188-190) and appropriately handles the unlikely formatting failure whilst avoiding production panics. The approach is consistent across the codebase.src/stdlib/mod.rs (3)
81-83: Improved panic context for constant validation.The explicit if-let pattern with formatted error interpolation is clearer than the previous unwrap/expect approach and provides better diagnostics if this defence-in-depth check ever fails.
97-99: Clearer variable naming.Renaming the bound value to
relative_pathbetter distinguishes the convertedUtf8PathBuffrom the genericIntoparameter and improves readability.
141-142: Enhanced error context in Default implementation.Using
unwrap_or_elsewith formatted panic interpolation provides richer diagnostics than the previousexpectapproach when opening the ambient directory fails, consistent with the PR's error-handling improvements.src/ninja_gen.rs (3)
15-27: LGTM!The error variant documentation is comprehensive and follows Rustdoc conventions. Each variant and field is properly documented with clear, actionable descriptions.
182-224: LGTM!The helper method extraction successfully decomposes the rendering logic whilst maintaining clear separation of concerns. The defensive
debug_assert!checks (lines 214-217, 221) appropriately catch programming errors during development whilst the release-mode fallbacks provide graceful degradation, aligning well with the PR's goal to harden runtime paths.
226-232: LGTM!The refactored Display implementation clearly sequences the rendering phases (rule header, recipe, metadata) whilst properly propagating errors. The structure is maintainable and follows idiomatic Rust patterns.
src/runner/process/mod.rs (4)
27-28: Good: doctest‑only re‑exports guarded with scoped expectation.Using
#[cfg_attr(not(doctest), expect(unused_imports, reason = "..."))]preserves doctest ergonomics without muting the lint in release builds.
124-126: Good: expand error docs to include missing stdio.Document the unavailable stdin/stdout/stderr case; aligns with the new runtime checks.
142-149: Good: fail fast when child pipes are missing.Return a descriptive
io::Errorrather than panicking on absent stdout/stderr.
171-176: Good: surface panics from forwarding threads.Log thread panics rather than discarding them; improves observability without failing the build.
src/runner/mod.rs (4)
30-44: Good: small, documented wrapper API for NinjaContent.Provide
new,as_str, andinto_stringwith#[must_use]and concise docs; matches module doc style and CQRS intent.
53-68: Good: lightweight, documented façade for BuildTargets.Expose
new,as_slice, andis_empty; lifetime‑friendly and zero‑alloc. ManualDefaultbelow is appropriate.
70-78: Good: narrowly scoped expectation justifying manual Default.Use
#[expect(clippy::derivable_impls, ...)]with a clear reason; complies with the no‑blanket‑suppressions rule.
173-173: Good: propagate manifest‑path errors.Use
?onresolve_manifest_pathto surface path conversion failures with context; aligns with “avoid panics, prefer Results”.clippy.toml (1)
2-4: Verify whetherallow-expect-in-testsmakes test-file lint suppressions redundant.The
allow-expect-in-tests = truesetting should automatically permitexpect()calls in test contexts. Verify whether this configuration applies to integration tests in thetests/directory, as it typically targets#[test]functions and#[cfg(test)]modules. If it does apply, the explicit#![allow(clippy::expect_used)]attributes added throughout test files may be unnecessary duplication.#!/bin/bash # Description: Check Clippy documentation for allow-expect-in-tests scope and verify test file suppressions # Expected: Determine if the setting covers integration tests or only unit tests echo "=== Searching for test files with expect_used suppressions ===" rg -n --type=rust '#!\[allow\(clippy::expect_used' tests/ echo "" echo "=== Checking if tests use cfg(test) or are integration tests ===" fd --type f '\.rs$' tests/ --exec head -5Does Clippy's allow-expect-in-tests configuration apply to integration tests in the tests/ directory, or only to #[test] functions and #[cfg(test)] modules?tests/env_path_tests.rs (1)
34-36: LGTM: Proper use of guards for unsafe environment mutations.The unsafe
env::set_varandenv::remove_varcalls are correctly wrapped withEnvLock::acquire()guards, following the coding guideline that environment mutations in tests must be protected by shared guards and mutexes.As per coding guidelines.
Also applies to: 48-53, 62-64, 74-79
tests/ninja_snapshot_tests.rs (1)
32-32: Excellent: structured logging replaces stderr.Switching from
eprintln!totracing::warn!aligns with the PR's goal of introducing structured logging and improves observability.src/ir/graph.rs (3)
25-41: LGTM!Documentation added for all Action fields is clear and aligns with the deny(missing_docs) lint policy.
110-242: LGTM!Error variant documentation is comprehensive, includes examples, and follows proper en-GB spelling.
39-40: All Action construction sites properly initialise the restat field—no action required.Verification confirms all seven construction sites across production and test code have been updated to include the new
restatfield. Every site initialises it tofalse, maintaining backwards-compatible behaviour. The documentation adequately describes the field's purpose.tests/ninja_gen_tests.rs (1)
23-35: LGTM!The switch from
eprintln!totracing::warn!for skip notices aligns with the PR's structured logging objectives and provides better observability.src/stdlib/time/mod.rs (1)
281-299: LGTM!The replacement of
unwrapwithunwrap_or_elseplus a debug assertion correctly hardens the runtime path. The fallback tounsigned_abs()ensures the function never panics in production whilst the debug assertion catches logic errors during development.build.rs (2)
95-97: LGTM!Replacing
expectwithmap_errand?provides a clearer error message and aligns with robust error handling practices for build scripts.
107-115: LGTM!Renaming
extra_dirtoextra_dir_pathimproves clarity and aligns with precise naming guidelines.tests/steps/ninja_steps.rs (1)
36-86: LGTM!Parameter renaming throughout the step definitions improves clarity and aligns with precise naming guidelines. All usages correctly updated.
src/stdlib/time/tests.rs (2)
76-85: LGTM!Restructuring the error handling with clearer variable names improves test readability whilst preserving functionality.
136-144: LGTM!Consistent with the error handling restructuring in
now_rejects_invalid_offset. Variable naming is clear and improves readability.src/manifest/glob.rs (4)
59-66: LGTM!Replacing
to_string()withto_owned()for string literals is more idiomatic and marginally more efficient.
96-96: LGTM!Using
to_owned()for the string literal is more idiomatic.
406-406: LGTM!Using
to_owned()for String cloning is idiomatic.
422-425: LGTM!The replacement of
expectwithunwrap_or_elseplus a debug assertion correctly hardens the runtime path. The fallback to raw pattern ensures the function never panics in production whilst the debug assertion catches logic errors during development.Cargo.toml (1)
55-104: LGTM!The comprehensive lint configuration aligns with the PR's objective to adopt a stricter lint policy. Enabling
unwrap_used,expect_used,missing_docs, andcognitive_complexityexplains the corresponding code changes throughout the repository, including test file lint suppressions and documentation additions.src/cli.rs (1)
43-43: LGTM!Documentation accurately describes the field behaviour and aligns with the default command logic in
with_default_command.src/manifest/diagnostics.rs (6)
18-28: LGTM!Documentation clearly describes the constructor and accessor methods for
ManifestSource.
67-77: LGTM!Documentation appropriately describes the constructor and accessor for
ManifestName.
126-133: LGTM!Renaming the intermediate variables (
without_newline,cleaned_line) improves code clarity by making the transformation steps explicit.
176-179: LGTM!Using
actual_locinstead oflocbetter distinguishes the unwrapped value from theOptionparameter, and the early-return guard pattern is idiomatic.
210-217: LGTM!Added documentation clarifies the enum variant and its associated diagnostic field.
245-245: LGTM!Using
to_owned()instead ofto_string()is semantically correct for cloning a&strinto an ownedString.tests/runner_tests.rs (1)
6-7: LGTM!Module-level documentation appropriately describes the test suite's purpose.
tests/std_filter_tests/network_functions.rs (2)
19-28: LGTM on the graceful error handling.The match-based error handling for
PermissionDeniedprovides a graceful test skip with structured logging, whilst other errors panic with descriptive context. This aligns with the PR's goal of hardening runtime paths.
50-59: LGTM on the graceful error handling.The error handling pattern is consistent with the earlier test case, providing appropriate fallback behaviour.
src/manifest/mod.rs (2)
36-39: LGTM on the documented public type aliases.The doc comments clarify the purpose of these type aliases, and exposing them supports the PR's goal of presenting a clearer public surface.
104-104: LGTM on the parameter rename for clarity.Renaming
nametovar_nameimproves readability by disambiguating environment variable names from other identifier types.tests/ast_tests.rs (1)
286-287: LGTM on usingto_owned()for owned String construction.This aligns with the coding guideline preferring
to_owned()overto_string()for constructing owned Strings from string literals.As per coding guidelines.
src/manifest/render.rs (2)
160-185: LGTM on the test helper functions.The extracted helpers reduce duplication and improve test readability by providing clear error messages when assertions fail.
208-208: LGTM on usingto_owned()for owned String construction.This aligns with the coding guideline preferring
to_owned()overto_string()for constructing owned Strings from string literals.As per coding guidelines.
tests/ir_tests.rs (2)
72-81: LGTM on the explicit Option semantics and defensive else branch.The renamed variables (
first_insert,second_insert) clarify the insertion order, and the explicitelsebranch withpanic!ensures recipe type mismatches are caught immediately.
105-108: LGTM on the consistent Option semantics.The variable renames maintain consistency with the actions test case and clearly document the expected insertion behaviour.
src/ir/from_manifest.rs (3)
110-112: LGTM on the descriptive variable rename.Renaming
cycletodetected_cycleclarifies the semantic meaning in the error construction context.
127-137: LGTM on the descriptive variable rename.Renaming
recipetoresolved_recipeclarifies that command interpolation has occurred, distinguishing it from the original recipe.
186-202: LGTM on usingto_owned()for owned String construction.These changes align with the coding guideline preferring
to_owned()overto_string()for constructing owned Strings from string literals.As per coding guidelines.
tests/manifest_jinja_tests.rs (4)
16-59: LGTM on the domain-specific test enums.The documented
EnvVarandFieldNameenums withconst fn as_str()accessors improve type safety and readability in tests, replacing magic strings with semantic types.
409-409: LGTM on usingto_owned()for owned String construction.This aligns with the coding guideline preferring
to_owned()overto_string()for constructing owned Strings from string literals.As per coding guidelines.
448-448: LGTM on usingto_owned()for owned String construction.This aligns with the coding guideline preferring
to_owned()overto_string()for constructing owned Strings from string literals.As per coding guidelines.
512-518: LGTM on usingto_owned()for owned String construction.These changes align with the coding guideline preferring
to_owned()overto_string()for constructing owned Strings from string literals.As per coding guidelines.
test_support/src/http.rs (1)
55-69: Propagate I/O errors from server setup (good hardening).Return
io::Result, use?, and wrap withOk(..). This aligns with “no panics in prod code” even in test support. LGTM.src/manifest/tests.rs (2)
115-117: MacroDefinition ownership fixes (good).Use
.to_owned()forsignature/bodyand thedoc_valuebinding to clarify ownership and silence lints. LGTM.Also applies to: 183-189, 215-218
241-250: Test robustness for HTTP and cache checks (good).Skip on
PermissionDenied, otherwise fail with context; useanyhow::ensure!/bail!for clearer messages. LGTM.Also applies to: 269-274, 278-285
src/ast.rs (2)
51-60: Add MacroDefinition (good).Public struct with
deny_unknown_fieldsand docs improves schema clarity. LGTM.
142-156: Variant docs (good).Inline Rustdoc on
Recipevariants clarifies the public surface without changing behaviour. LGTM.tests/steps/manifest_steps.rs (1)
371-374: Parameter and comparison updates (good).Use explicit
target_index/expected_*and borrow&expected_scriptfor equality. LGTM.Also applies to: 381-385, 395-400
src/manifest/jinja_macros.rs (3)
39-57: Tighten signature parsing (good).Clearer local naming and precise errors; behaviour preserved. LGTM.
141-155: Keyword-argument handling (good).Append
Kwargsas trailing positional; naming clarifies intent. LGTM.
195-205: Initialisation guard and cache semantics (good).Fail fast if the macro cache is uninitialised, handle
OnceLock::setraces, and exposeOption<&MacroInstance>. This hardens runtime behaviour. LGTM.Also applies to: 209-222, 270-273, 277-279
tests/steps/cli_steps.rs (2)
19-19: LGTM: Clearer intent withto_owned().Prefer
to_owned()overto_string()for string slices when cloning rather than formatting.
54-184: LGTM: Parameter renamings improve test clarity.The updated names (
cli_args,expected_target,manifest_path_str, etc.) follow the guideline to "Name things precisely" and make assertion intent clearer.tests/steps/stdlib_steps.rs (1)
243-432: LGTM: Consistent parameter renamings enhance test clarity.The updated names (
template_source,raw_path,expected_output,expected_fragment, etc.) improve readability and follow the guideline to name things precisely.tests/cucumber.rs (4)
18-59: LGTM: Comprehensive field documentation.The doc comments clarify the purpose of each
CliWorldfield, improving the public test API surface.
73-74: LGTM: Appropriate expect for test fixture setup.The
.expect()with a descriptive message provides clear diagnostics when HTTP server spawning fails during test setup.
120-123: LGTM: Structured logging enhances observability.Replacing
eprintln!withtracing::warn!provides better structured diagnostics and aligns with the PR's logging improvements.Also applies to: 154-157, 166-166
153-158: LGTM: Proactive platform compatibility check.The upfront TCP binding check prevents test failures on platforms lacking network support, with a clear warning explaining the skip reason.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 22
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
tests/ninja_gen_tests.rs (1)
19-31: Centralise Ninja availability checks as a shared fixture.Extract
skip_if_ninja_unavailable/ninja_integration_setupinto a reusable helper in test_support to de-duplicate logic across suites. As per coding guidelines (use fixtures for shared setup).Also applies to: 41-55
tests/manifest_jinja_tests.rs (1)
59-61: Optionally centralisemanifest_yamlhelper.Move
manifest_yamlto a shared test_support module to avoid duplicate implementations across suites.Also applies to: 293-296
test_support/src/lib.rs (1)
114-125: Fix doctestexpectunder-D clippy::expect_used.The
no_rundoctest still compiles and will trip the lint gate. Ignore it or scope an inner expectation.Apply this diff:
-/// ```rust,no_run +/// ```rust,ignore /// use camino::{Utf8Path, Utf8PathBuf}; /// use tempfile::TempDir; /// use test_support::ensure_manifest_exists; /// -/// let temp = TempDir::new().expect("temp dir"); -/// let temp_path = Utf8Path::from_path(temp.path()).expect("utf-8 path"); +/// let temp = TempDir::new().expect("temp dir"); +/// let temp_path = Utf8Path::from_path(temp.path()).expect("utf-8 path"); /// let cli_file = Utf8PathBuf::from("manifest.yml"); /// let manifest = ensure_manifest_exists(temp_path, &cli_file) /// .expect("manifest"); /// assert!(manifest.exists()); /// ```Alternative: keep
no_runand add an inner attribute as the first line of the block:#![expect(clippy::expect_used, reason = "doctest uses `expect` for brevity")]
♻️ Duplicate comments (7)
tests/steps/process_steps.rs (1)
2-6: Replace module-level#![allow(...)]with function-scoped#[expect(...)]attributes.The coding guidelines explicitly forbid
#[allow]and permit only narrowly scoped#[expect(lint, reason = "...")]attributes. This module-level suppression violates the "no blanket or file-wide suppression" rule. This issue was flagged in a previous review but remains only partially addressed.As per coding guidelines.
Remove the module-level allowance:
-#![allow( - clippy::shadow_reuse, - clippy::shadow_unrelated, - reason = "Cucumber step macros rebind capture identifiers" -)]Add function-scoped attributes to each step function that triggers these lints due to Cucumber macro-generated code. For example, apply to
install_fake_ninja(line 35),fake_ninja_check(line 46), and all other step functions:#[expect( clippy::shadow_reuse, clippy::shadow_unrelated, reason = "Cucumber step macros rebind capture identifiers" )]Functions at lines 23-26, 99-102, and 170-173 already demonstrate the correct pattern.
🤖 Prompt for AI Agents
In tests/steps/process_steps.rs at lines 2-6, remove the module-level #![allow(clippy::shadow_reuse, clippy::shadow_unrelated, reason = "Cucumber step macros rebind capture identifiers")] entirely. Then add a function-scoped #[expect(clippy::shadow_reuse, clippy::shadow_unrelated, reason = "Cucumber step macros rebind capture identifiers")] attribute immediately before each Cucumber step function definition (install_fake_ninja at line 35, fake_ninja_check at line 46, no_ninja at line 59, cli_uses_temp_dir at line 69, build_dir_exists at line 84, run at line 103, command_should_succeed at line 146, command_should_fail at line 156, and command_should_fail_with_error at line 174). Place each #[expect(...)] attribute after the doc comment and before any existing #[expect(...)] attributes or the #[given]/#[when]/#[then] attribute, mirroring the pattern used at lines 23-26, 99-102, and 170-173.src/ir/cmd_interpolate.rs (2)
177-209: Add narrowly scoped#[expect(...)]attributes to each test function.The test functions use
.expect()but lack the required#[expect(clippy::expect_used, reason = "...")]attributes. Per the coding guidelines and PR objectives, expect is only allowed in tests with explicit allowances.Apply this diff to add the required attributes:
+ #[expect(clippy::expect_used, reason = "fixtures favour expect for clarity")] #[test] fn interpolate_command_rejects_unbalanced_backticks() {Then add the same attribute to the other test functions at lines 194 and 202:
+ #[expect(clippy::expect_used, reason = "fixtures favour expect for clarity")] #[test] fn interpolate_command_replaces_placeholders() {+ #[expect(clippy::expect_used, reason = "fixtures favour expect for clarity")] #[test] fn interpolate_command_preserves_backtick_tokens() {As per coding guidelines.
28-36: Remove the unnecessary clone and update the misleading comment.The bytes are cloned at line 30, but this is wasteful:
- If
from_utf8succeeds, the clone was unnecessary.- If it fails,
from_utf8_lossyuses&bytes, not the cloned version.Additionally, the comment at line 28 now misleads—lossy conversion IS used as a fallback.
Apply this diff to fix both issues:
- // Utf8PathBuf guarantees UTF-8; avoid lossy conversion. + // Utf8PathBuf guarantees UTF-8; shell quoting should preserve it. let bytes: Vec<u8> = p.as_str().quoted(Sh); - match String::from_utf8(bytes.clone()) { + match String::from_utf8(bytes) { Ok(text) => text, Err(err) => { debug_assert!(false, "shell quoting produced non UTF-8 bytes: {err}"); - String::from_utf8_lossy(&bytes).into_owned() + String::from_utf8_lossy(err.as_bytes()).into_owned() } }src/stdlib/command.rs (1)
267-274: Remove the unnecessary clone.
String::from_utf8consumes theVec<u8>, so cloning before the call wastes an allocation. On failure, the error contains the original bytes.Apply this diff to eliminate the clone:
- let bytes: Vec<u8> = arg.quoted(Sh); - match String::from_utf8(bytes.clone()) { + let bytes = arg.quoted(Sh); + match String::from_utf8(bytes) { Ok(text) => Ok(text), Err(err) => { debug_assert!(false, "quoted args must be valid UTF-8: {err}"); - Ok(String::from_utf8_lossy(&bytes).into_owned()) + Ok(String::from_utf8_lossy(err.as_bytes()).into_owned()) } }src/manifest/expand.rs (1)
135-140: DRY the repeated “targets sequence missing” extraction in tests.Extract a tiny helper or rstest fixture to avoid repetition and sharpen failures.
Example:
fn targets<'a>(doc: &'a ManifestValue) -> anyhow::Result<&'a [ManifestValue]> { doc.get("targets").and_then(|v| v.as_array()).context("targets sequence missing") }Also applies to: 176-181, 220-224
tests/cucumber.rs (1)
1-5: Remove file-wide#![allow]and scope#[expect]narrowly.This file-wide suppression violates coding guidelines: "
#[allow]is forbidden" and "No lint groups, no blanket or file-wide suppression."Remove lines 1-4 entirely and apply
#[expect(clippy::expect_used, reason = "...")]narrowly to the specific line or function that calls.expect()(e.g., line 111 inshutdown_http_server_with).As per coding guidelines.
src/manifest/tests.rs (1)
38-41: Include the target directory in the warning.The warning omits the path we tried to restore, making triage harder when restoration fails.
Apply this diff:
fn drop(&mut self) { - if let Err(err) = std::env::set_current_dir(&self.original) { - tracing::warn!("failed to restore working directory: {err}"); - } + if let Err(err) = std::env::set_current_dir(&self.original) { + tracing::warn!( + "failed to restore working directory to {}: {err}", + self.original.display() + ); + } }
📜 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 (45)
src/ir/cmd_interpolate.rs(1 hunks)src/manifest/diagnostics.rs(7 hunks)src/manifest/expand.rs(4 hunks)src/manifest/glob.rs(6 hunks)src/manifest/render.rs(2 hunks)src/manifest/tests.rs(7 hunks)src/ninja_gen.rs(6 hunks)src/runner/process/file_io.rs(1 hunks)src/runner/process/mod.rs(3 hunks)src/stdlib/collections.rs(5 hunks)src/stdlib/command.rs(6 hunks)src/stdlib/network.rs(4 hunks)src/stdlib/time/tests.rs(6 hunks)test_support/Cargo.toml(1 hunks)test_support/src/check_ninja.rs(3 hunks)test_support/src/command_helper.rs(5 hunks)test_support/src/env.rs(1 hunks)test_support/src/env_lock.rs(1 hunks)test_support/src/http.rs(2 hunks)test_support/src/lib.rs(5 hunks)tests/assert_cmd_tests.rs(1 hunks)tests/ast_tests.rs(8 hunks)tests/cli_tests.rs(3 hunks)tests/command_escaping_tests.rs(2 hunks)tests/cucumber.rs(4 hunks)tests/hasher_tests.rs(2 hunks)tests/ir_from_manifest_tests.rs(6 hunks)tests/ir_tests.rs(2 hunks)tests/manifest_env_tests.rs(3 hunks)tests/manifest_glob_tests.rs(7 hunks)tests/manifest_jinja_tests.rs(11 hunks)tests/ninja_env_tests.rs(4 hunks)tests/ninja_gen_tests.rs(9 hunks)tests/ninja_snapshot_tests.rs(4 hunks)tests/runner_tests.rs(10 hunks)tests/std_filter_tests/collection_filters.rs(6 hunks)tests/std_filter_tests/command_filters.rs(6 hunks)tests/std_filter_tests/hash_filters.rs(2 hunks)tests/std_filter_tests/io_filters.rs(1 hunks)tests/std_filter_tests/mod.rs(1 hunks)tests/std_filter_tests/network_functions.rs(1 hunks)tests/std_filter_tests/path_filters.rs(15 hunks)tests/std_filter_tests/support.rs(1 hunks)tests/steps/process_steps.rs(7 hunks)tests/steps/stdlib_steps.rs(7 hunks)
🧰 Additional context used
📓 Path-based instructions (5)
**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
**/*.rs: Every Rust module must begin with a module-level comment using //! explaining the module's purpose and utility
Document public APIs with Rustdoc comments (///) so documentation can be generated with cargo doc
Prefer immutable data; avoid unnecessary mut bindings
Handle errors with Result instead of panicking where feasible
Prefer semantic error enums deriving std::error::Error via thiserror for inspectable conditions
Clippy warnings must be denied; do not silence lints except as a last resort
Lint suppressions must be tightly scoped and include a clear reason; prefer #[expect(...)] over #[allow(...)]
Place function attributes after doc comments
Do not use return in single-line functions
Prefer single-line function definitions where appropriate (e.g., pub fn new(id: u64) -> Self { Self(id) })
Use predicate functions for conditional criteria with more than two branches
Where a function is too long, extract meaningfully named helpers adhering to separation of concerns and CQRS
Where a function has too many parameters, group related parameters into meaningfully named structs
When returning large error data, consider using Arc to reduce data moved
Prefer .expect() over .unwrap() on Results/Options
Use concat!() to combine long string literals instead of escaping newlines with backslashes
Use NewTypes to model domain values; prefer explicit tuple structs; leverage newt-hype and the-newtype crates as described; provide appropriate From/AsRef/TryFrom implementations and avoid orphan-rule-violating impls
Keep each Rust code file under 400 lines; split long switches/dispatch tables by feature; move large test data to external files
Use functions and composition to avoid repetition; extract reusable logic; prefer declarative constructs (iterators/generators/comprehensions) when readable
Small, single-responsibility functions; obey command/query segregation
Name things precisely; booleans should use is/has/should prefixes
Comment why, not what; explain assumptions, e...
Files:
test_support/src/env.rstest_support/src/check_ninja.rssrc/stdlib/time/tests.rssrc/runner/process/file_io.rstest_support/src/env_lock.rssrc/runner/process/mod.rssrc/manifest/render.rssrc/stdlib/collections.rstests/ninja_gen_tests.rssrc/ir/cmd_interpolate.rssrc/stdlib/network.rssrc/manifest/expand.rssrc/manifest/glob.rstests/ninja_env_tests.rstest_support/src/http.rstests/ninja_snapshot_tests.rstest_support/src/command_helper.rstests/assert_cmd_tests.rstests/cli_tests.rstests/std_filter_tests/hash_filters.rstest_support/src/lib.rstests/std_filter_tests/io_filters.rstests/std_filter_tests/network_functions.rstests/manifest_env_tests.rstests/manifest_jinja_tests.rstests/ir_tests.rstests/std_filter_tests/mod.rssrc/stdlib/command.rstests/std_filter_tests/support.rssrc/manifest/diagnostics.rstests/std_filter_tests/command_filters.rstests/std_filter_tests/collection_filters.rstests/steps/process_steps.rssrc/manifest/tests.rstests/manifest_glob_tests.rstests/ir_from_manifest_tests.rstests/cucumber.rstests/ast_tests.rstests/hasher_tests.rstests/runner_tests.rstests/command_escaping_tests.rstests/std_filter_tests/path_filters.rssrc/ninja_gen.rstests/steps/stdlib_steps.rs
⚙️ CodeRabbit configuration file
**/*.rs: * Seek to keep the cyclomatic complexity of functions no more than 12.
Adhere to single responsibility and CQRS
Place function attributes after doc comments.
Do not use
returnin single-line functions.Move conditionals with >2 branches into a predicate function.
Avoid
unsafeunless absolutely necessary.Every module must begin with a
//!doc comment that explains the module's purpose and utility.Comments and docs must follow en-GB-oxendict (-ize / -yse / -our) spelling and grammar
Lints must not be silenced except as a last resort.
#[allow]is forbidden.- Only narrowly scoped
#[expect(lint, reason = "...")]is allowed.- No lint groups, no blanket or file-wide suppression.
- Include
FIXME:with link if a fix is expected.Where code is only used by specific features, it must be conditionally compiled or a conditional expectation for unused_code applied.
Use
rstestfixtures for shared setup and to avoid repetition between tests.Replace duplicated tests with
#[rstest(...)]parameterised cases.Prefer
mockallfor mocks/stubs.Prefer
.expect()over.unwrap()Ensure that any API or behavioural changes are reflected in the documentation in
docs/Ensure that any completed roadmap steps are recorded in the appropriate roadmap in
docs/Files must not exceed 400 lines in length
- Large modules must be decomposed
- Long match statements or dispatch tables should be decomposed by domain and collocated with targets
- Large blocks of inline data (e.g., test fixtures, constants or templates) must be moved to external files and inlined at compile-time or loaded at run-time.
Environment access (env::set_var and env::remove_var) are always unsafe in Rust 2024 and MUST be marked as such
- For testing of functionality depending upon environment variables, dependency injection and the
mockablecrate are the preferred option.- If mockable cannot be used, env mutations in...
Files:
test_support/src/env.rstest_support/src/check_ninja.rssrc/stdlib/time/tests.rssrc/runner/process/file_io.rstest_support/src/env_lock.rssrc/runner/process/mod.rssrc/manifest/render.rssrc/stdlib/collections.rstests/ninja_gen_tests.rssrc/ir/cmd_interpolate.rssrc/stdlib/network.rssrc/manifest/expand.rssrc/manifest/glob.rstests/ninja_env_tests.rstest_support/src/http.rstests/ninja_snapshot_tests.rstest_support/src/command_helper.rstests/assert_cmd_tests.rstests/cli_tests.rstests/std_filter_tests/hash_filters.rstest_support/src/lib.rstests/std_filter_tests/io_filters.rstests/std_filter_tests/network_functions.rstests/manifest_env_tests.rstests/manifest_jinja_tests.rstests/ir_tests.rstests/std_filter_tests/mod.rssrc/stdlib/command.rstests/std_filter_tests/support.rssrc/manifest/diagnostics.rstests/std_filter_tests/command_filters.rstests/std_filter_tests/collection_filters.rstests/steps/process_steps.rssrc/manifest/tests.rstests/manifest_glob_tests.rstests/ir_from_manifest_tests.rstests/cucumber.rstests/ast_tests.rstests/hasher_tests.rstests/runner_tests.rstests/command_escaping_tests.rstests/std_filter_tests/path_filters.rssrc/ninja_gen.rstests/steps/stdlib_steps.rs
**/*.{rs,md}
📄 CodeRabbit inference engine (AGENTS.md)
Use en-GB-oxendict spelling and grammar in comments and documentation (exceptions allowed for external API names)
Files:
test_support/src/env.rstest_support/src/check_ninja.rssrc/stdlib/time/tests.rssrc/runner/process/file_io.rstest_support/src/env_lock.rssrc/runner/process/mod.rssrc/manifest/render.rssrc/stdlib/collections.rstests/ninja_gen_tests.rssrc/ir/cmd_interpolate.rssrc/stdlib/network.rssrc/manifest/expand.rssrc/manifest/glob.rstests/ninja_env_tests.rstest_support/src/http.rstests/ninja_snapshot_tests.rstest_support/src/command_helper.rstests/assert_cmd_tests.rstests/cli_tests.rstests/std_filter_tests/hash_filters.rstest_support/src/lib.rstests/std_filter_tests/io_filters.rstests/std_filter_tests/network_functions.rstests/manifest_env_tests.rstests/manifest_jinja_tests.rstests/ir_tests.rstests/std_filter_tests/mod.rssrc/stdlib/command.rstests/std_filter_tests/support.rssrc/manifest/diagnostics.rstests/std_filter_tests/command_filters.rstests/std_filter_tests/collection_filters.rstests/steps/process_steps.rssrc/manifest/tests.rstests/manifest_glob_tests.rstests/ir_from_manifest_tests.rstests/cucumber.rstests/ast_tests.rstests/hasher_tests.rstests/runner_tests.rstests/command_escaping_tests.rstests/std_filter_tests/path_filters.rssrc/ninja_gen.rstests/steps/stdlib_steps.rs
**/*{test,tests}.rs
📄 CodeRabbit inference engine (AGENTS.md)
Write unit and behavioural tests for new functionality and run them before and after changes
Files:
src/stdlib/time/tests.rstests/ninja_gen_tests.rstests/ninja_env_tests.rstests/ninja_snapshot_tests.rstests/assert_cmd_tests.rstests/cli_tests.rstests/manifest_env_tests.rstests/manifest_jinja_tests.rstests/ir_tests.rssrc/manifest/tests.rstests/manifest_glob_tests.rstests/ir_from_manifest_tests.rstests/ast_tests.rstests/hasher_tests.rstests/runner_tests.rstests/command_escaping_tests.rs
**/tests/**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
**/tests/**/*.rs: Use rstest fixtures for shared setup
Replace duplicated tests with #[rstest(...)] parameterised cases
Prefer mockall for mocks and stubs
Files:
tests/ninja_gen_tests.rstests/ninja_env_tests.rstests/ninja_snapshot_tests.rstests/assert_cmd_tests.rstests/cli_tests.rstests/std_filter_tests/hash_filters.rstests/std_filter_tests/io_filters.rstests/std_filter_tests/network_functions.rstests/manifest_env_tests.rstests/manifest_jinja_tests.rstests/ir_tests.rstests/std_filter_tests/mod.rstests/std_filter_tests/support.rstests/std_filter_tests/command_filters.rstests/std_filter_tests/collection_filters.rstests/steps/process_steps.rstests/manifest_glob_tests.rstests/ir_from_manifest_tests.rstests/cucumber.rstests/ast_tests.rstests/hasher_tests.rstests/runner_tests.rstests/command_escaping_tests.rstests/std_filter_tests/path_filters.rstests/steps/stdlib_steps.rs
**/src/lib.rs
📄 CodeRabbit inference engine (AGENTS.md)
Never export the opaque eyre::Report type from libraries; expose semantic domain error enums instead
Files:
test_support/src/lib.rs
🧬 Code graph analysis (28)
src/stdlib/time/tests.rs (2)
src/manifest/expand.rs (1)
eval_expression(110-115)src/stdlib/time/mod.rs (3)
now(38-49)new(307-309)new(344-346)
tests/ninja_gen_tests.rs (2)
src/ninja_gen.rs (2)
generate(76-80)generate_into(113-152)tests/std_filter_tests/command_filters.rs (1)
new(22-41)
src/stdlib/network.rs (2)
tests/std_filter_tests/command_filters.rs (1)
new(22-41)src/stdlib/mod.rs (1)
new(78-88)
tests/ninja_env_tests.rs (1)
test_support/src/env.rs (1)
override_ninja_env(221-229)
tests/ninja_snapshot_tests.rs (3)
src/manifest/mod.rs (1)
from_str(146-148)src/ir/from_manifest.rs (1)
from_manifest(24-35)src/ninja_gen.rs (1)
generate(76-80)
tests/assert_cmd_tests.rs (1)
test_support/src/lib.rs (1)
fake_ninja(66-98)
tests/cli_tests.rs (1)
src/cli.rs (1)
parse_from_with_default(61-69)
tests/std_filter_tests/hash_filters.rs (1)
tests/std_filter_tests/support.rs (3)
filter_workspace(54-70)stdlib_env(50-52)register_template(26-36)
test_support/src/lib.rs (1)
test_support/src/error.rs (1)
std(20-25)
tests/std_filter_tests/io_filters.rs (1)
tests/std_filter_tests/support.rs (4)
filter_workspace(54-70)stdlib_env(50-52)render(72-86)register_template(26-36)
tests/std_filter_tests/network_functions.rs (4)
test_support/src/http.rs (1)
spawn_http_server(58-72)tests/std_filter_tests/support.rs (3)
stdlib_env_with_state(46-48)register_template(26-36)stdlib_env_with_config(38-44)src/stdlib/mod.rs (2)
is_impure(163-165)new(78-88)test_support/src/hash.rs (1)
sha256_hex(18-26)
tests/manifest_env_tests.rs (3)
tests/command_escaping_tests.rs (1)
manifest_yaml(15-17)test_support/src/env.rs (2)
set(132-137)unset(140-145)src/manifest/mod.rs (1)
from_str(146-148)
tests/manifest_jinja_tests.rs (6)
tests/std_filter_tests/command_filters.rs (2)
env_lock(290-292)command(47-49)tests/command_escaping_tests.rs (1)
manifest_yaml(15-17)tests/manifest_env_tests.rs (1)
manifest_yaml(10-12)tests/manifest_glob_tests.rs (1)
manifest_yaml(33-35)src/manifest/mod.rs (1)
from_str(146-148)test_support/src/env_var_guard.rs (2)
set(40-48)remove(57-65)
tests/std_filter_tests/support.rs (1)
tests/std_filter_tests/command_filters.rs (3)
env(43-45)new(22-41)state(51-53)
src/manifest/diagnostics.rs (1)
src/manifest/mod.rs (1)
from_str(146-148)
tests/std_filter_tests/command_filters.rs (2)
test_support/src/command_helper.rs (3)
compile_failure_helper(70-72)compile_uppercase_helper(48-50)compile_rust_helper(100-123)tests/std_filter_tests/support.rs (2)
stdlib_env_with_state(46-48)register_template(26-36)
tests/std_filter_tests/collection_filters.rs (1)
tests/std_filter_tests/support.rs (2)
stdlib_env(50-52)register_template(26-36)
tests/steps/process_steps.rs (3)
test_support/src/lib.rs (2)
fake_ninja(66-98)ensure_manifest_exists(126-145)test_support/src/env.rs (1)
mocked_path_env(48-55)test_support/src/check_ninja.rs (1)
fake_ninja_check_build_file(13-59)
src/manifest/tests.rs (3)
src/manifest/jinja_macros.rs (8)
parse_macro_name(38-57)register_macro(69-88)call_macro_value(140-155)new(259-265)new(299-317)new(339-345)new(402-409)register_manifest_macros(100-113)test_support/src/http.rs (2)
spawn_http_server(58-72)join(30-36)test_support/src/hash.rs (1)
sha256_hex(18-26)
tests/manifest_glob_tests.rs (2)
src/manifest/mod.rs (1)
from_str(146-148)test_support/src/error.rs (1)
display_error_chain(17-27)
tests/ir_from_manifest_tests.rs (2)
src/manifest/mod.rs (1)
from_path(155-162)src/ir/from_manifest.rs (1)
from_manifest(24-35)
tests/cucumber.rs (2)
test_support/src/env.rs (1)
restore_many(98-112)test_support/src/http.rs (1)
spawn_http_server(58-72)
tests/ast_tests.rs (2)
tests/yaml_error_tests.rs (1)
err(95-97)src/manifest/mod.rs (2)
from_str(146-148)from_path(155-162)
tests/hasher_tests.rs (1)
src/hasher.rs (1)
hash(56-64)
tests/runner_tests.rs (4)
test_support/src/check_ninja.rs (1)
fake_ninja_check_build_file(13-59)test_support/src/path_guard.rs (1)
new(27-31)test_support/src/env.rs (2)
prepend_dir_to_path(175-191)override_ninja_env(221-229)test_support/src/lib.rs (1)
fake_ninja(66-98)
tests/command_escaping_tests.rs (3)
tests/manifest_env_tests.rs (1)
manifest_yaml(10-12)src/manifest/mod.rs (1)
from_str(146-148)src/ir/from_manifest.rs (1)
from_manifest(24-35)
tests/std_filter_tests/path_filters.rs (1)
tests/std_filter_tests/support.rs (4)
stdlib_env(50-52)register_template(26-36)filter_workspace(54-70)render(72-86)
tests/steps/stdlib_steps.rs (3)
src/stdlib/mod.rs (2)
new(78-88)is_impure(163-165)test_support/src/command_helper.rs (2)
compile_uppercase_helper(48-50)compile_failure_helper(70-72)test_support/src/hash.rs (1)
sha256_hex(18-26)
⏰ Context from checks skipped due to timeout of 120000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: release / build-macos (x86_64-apple-darwin, macos-13, macos-x86_64, macos-x86_64) / Build macos artefacts
- GitHub Check: release / build-macos (aarch64-apple-darwin, macos-14, macos-arm64, macos-aarch64) / Build macos artefacts
- GitHub Check: release / build-linux (aarch64-unknown-linux-gnu, linux-arm64, arm64) / Build linux artefacts
- GitHub Check: release / build-windows (aarch64-pc-windows-msvc, arm64, arm64, windows-aarch64) / Build windows artefacts
- GitHub Check: release / build-windows (x86_64-pc-windows-msvc, amd64, x64, windows-x86_64) / Build windows artefacts
- GitHub Check: build-test (ubuntu-latest, stable)
🔇 Additional comments (128)
src/stdlib/collections.rs (4)
31-31: LGTM – Idiomatic string conversion.Using
to_owned()for&strtoStringconversion is semantically clearer thanto_string()and aligns with theToOwnedtrait.
120-120: Good – Consistent string allocation.The change to
to_owned()maintains consistency with other&strtoStringconversions in this file.
163-264: Excellent – Result-based test signatures improve error context.The migration to
anyhow::Resultwithensure!assertions and?propagation provides richer error messages and aligns with the project's explicit error-handling strategy. The tests now surface failure context rather than panicking.
162-163: Confirmed – Past lint violation resolved.The forbidden
#![allow]attribute flagged in the previous review is no longer present. The code now complies with the lint policy requiring narrowly scoped#[expect]only.tests/std_filter_tests/collection_filters.rs (5)
1-12: Excellent module documentation and imports.The module-level doc comment clearly explains the purpose and scope of these tests. The import additions (anyhow traits, fallible submodule) directly support the Result-based error handling pattern applied throughout.
14-86: Exemplary Result-based test conversion.The transformation of these four tests demonstrates a consistent, thorough approach:
- Fallible helpers with
?propagation replace panic-prone unwrap/expect- Template operations enriched with
.context()for diagnostic clarity- Success paths validated via
ensure!with descriptive failure messages- Error-path tests use
match+bail!to catch unexpected successThis pattern delivers the comprehensive error context promised in the PR objectives.
94-172: Consistent application of the Result-based pattern.The group_by tests maintain the same rigorous error-handling discipline seen earlier. The inline struct definition (lines 149-152) is appropriately scoped to its single use case.
174-208: Parameterised error test follows the established pattern.The rstest parameterisation correctly avoids test duplication (per coding guidelines) whilst maintaining the Result-based error validation pattern. The dual
ensure!checks validate both error kind and message content.
1-208: Previous review concern resolved by Result-based conversion.The past review flagged a file-wide
#![allow(clippy::expect_used)]and requested narrowly scoped#[expect]attributes. The current implementation supersedes that concern entirely: the code no longer usesexpect()anywhere, having migrated to Result-based error propagation throughout. This eliminates the need for any lint suppressions and delivers the "richer error messages" and "comprehensive error context" objectives of this PR.src/stdlib/network.rs (4)
206-206: LGTM!The anyhow imports support rich error context in tests, aligning with the PR's goal of providing detailed error messages.
215-215: LGTM!The type alias improves test readability by providing semantic meaning to the fixture tuple.
218-225: LGTM!The fixture's conversion to
Result-based error handling with contextual messages aligns with the PR's objectives and matches the pattern used in other test fixtures across the codebase.
260-368: LGTM!All test functions consistently adopt Result-based error handling with descriptive context via
anyhow. The pattern of acceptingResult<CacheWorkspace>and unwrapping immediately with?provides excellent error propagation whilst maintaining clean test code.src/runner/process/file_io.rs (1)
84-189: Test error handling correctly migrated to Result-based flow.The tests properly return
Result<()>, use?propagation, and replace assertions withensure!for richer error context. The past review concern about a module-level#![allow(clippy::expect_used)]at line 86 has been resolved—no such attribute is present in the current code.src/runner/process/mod.rs (3)
25-29: Lint suppression correctly migrated toexpect.Replacing
allowwithexpectadheres to the coding guidelines. The reason is clear and the scope appropriate.
122-126: Documentation accurately reflects error conditions.The updated doc comment correctly documents that unavailable standard streams will cause an error, matching the implementation at lines 142–149.
142-149: Error handling for missing pipes and thread panics is sound.Converting missing stdout/stderr pipes to
io::Errorviaok_or_elseat lines 142–149 prevents panics and provides clear error messages. Logging thread panics at lines 171–176 withtracing::warn!ensures that forwarding failures are visible without aborting the process.Also applies to: 171-176
src/stdlib/command.rs (2)
352-362: LGTM: graceful thread cleanup.The warning logs provide visibility into thread join failures without disrupting the cleanup path. The use of
tracing::warn!follows the structured logging guideline.
484-486: LGTM: appropriate warning on reap failure.Logging the wait failure while still returning the timeout error provides useful observability without altering control flow.
tests/steps/stdlib_steps.rs (7)
7-10: Past review concern resolved; file-wide expectation justified.The previous file-wide
#![allow(clippy::expect_used)]has been removed. The migration to Result-based error handling eliminated the need forexpectentirely—all test steps now use?withcontext()for richer error messages, which is superior.The remaining file-wide
#![expect(clippy::shadow_reuse)]is justified: Cucumber's step macro generates code that shadows parameter identifiers during regex capture binding, and this cannot be suppressed at function scope.
107-133: Excellent error context throughout workspace setup.The migration to
Result<Utf8PathBuf>with contextual error messages at each fallible operation provides clear diagnostics for tempdir creation, UTF-8 validation, directory opening, and fixture writes.
135-169: Template rendering helpers correctly propagate setup errors.Both
render_template_with_contextandrender_templatenow returnResult<()>and propagate workspace/directory errors via?, whilst deliberately capturing template render errors intoworld.stdlib_errorfor assertion by test steps.
172-256: Given steps consistently return Result with clear context.All setup steps (
stdlib_workspace, command helpers, HTTP fixtures, file writes, and HOME configuration) now propagate errors viaResult<()>with descriptive context at each fallible operation.
271-313: When steps propagate errors with prerequisite context.All rendering steps now return
Result<()>and provide clear error messages when expected state (HTTP server, command helper) is missing.
320-330: Assertion steps comprehensively migrated to ensure! macro.All Then steps now return
Result<()>and useensure!for assertions with descriptive failure messages. The parsing helpers (parse_iso_timestamp,parse_expected_offset) usewith_contextwith closures for lazy error formatting.Also applies to: 399-419, 422-429, 432-448, 451-474, 477-508
315-318: Narrowly scoped expectations correctly justified.The three
#[expect(clippy::needless_pass_by_value)]attributes are appropriately scoped to individual functions where Cucumber's regex capture macro requires ownedStringarguments.Also applies to: 394-397, 493-496
src/ninja_gen.rs (8)
15-27: Document the public error type thoroughly.Keep this; it satisfies missing_docs and clarifies failure modes.
64-70: Modernize the doc-test to return Result and use '?'.Good change; this keeps examples panic-free and compilable under denied lints.
102-108: Adopt Result in generate_into example.Good; mirrors the public API and avoids expect/unwrap in docs.
190-201: Split recipe rendering into helpers.Good separation of concerns; reduces cognitive complexity and improves readability.
234-239: Sequence header → recipe → metadata in Display.Good flow; the extra trailing newline is deliberate and matches tests.
271-272: Useanyhow::{Result, ensure}in tests.Good switch; removes
.expect()and aligns with lint gate.
275-275: ReturnResult<()>from the test.Good; enables
?and richer error messages in failures.
300-312: Assert full Ninja output with a clear diff on failure.Good use of
concat!andensure!; this is robust and readable.tests/std_filter_tests/mod.rs (1)
1-2: LGTM!The module-level documentation correctly identifies this as an integration test suite and fulfils the requirement that every Rust module must begin with a
//!doc comment.test_support/Cargo.toml (1)
15-15: LGTM!The addition of
anyhowsupports the PR's migration to Result-based error handling in test utilities, enabling contextual error messages throughout the test suite.tests/command_escaping_tests.rs (4)
14-17: LGTM!The
manifest_yamlhelper correctly places the#[inline]attribute after the doc comment and provides clear documentation.
34-47: LGTM!The function correctly propagates errors with contextual messages using
anyhow::Contextandbail!. The doc example properly demonstrates Result-based usage.
49-118: LGTM!All test functions correctly adopt Result-based error handling with
ensure!for assertions and?for error propagation. The contextual error messages improve test failure diagnostics.
120-138: LGTM!The parameterised test correctly verifies that invalid commands produce expected errors, using
let Err(...) else bail!(...)to assert failure andensure!to validate the error message.tests/ir_tests.rs (2)
67-80: LGTM!The refactoring improves test clarity by separating HashMap insertion from assertion and using explicit pattern matching. The
let Some(...) elsepattern is idiomatic and provides clear failure messages.
103-113: LGTM!The refactoring mirrors the pattern from
build_graph_duplicate_action_ids, improving test structure with explicit intermediate variables and clear assertions.tests/std_filter_tests/network_functions.rs (2)
104-132: LGTM!The test correctly verifies error handling paths using
matchto assert failure, thenensure!to validate error kind and message content. The verification that failed fetches still mark templates as impure is a valuable behavioural check.
134-161: LGTM!The test properly validates that unsupported
cache_dirarguments are rejected before network requests, and correctly verifies that pre-request failures do not mark templates as impure (unlike actual fetch attempts).tests/std_filter_tests/io_filters.rs (4)
9-45: Solid migration to fallible tests for contents/linecount.Propagate errors with
Result<()>, useensure!and contextualrender/I/O; this aligns with the stricter lint gate and improves diagnosability.
48-76: Correct error-kind validation for unsupported encodings.Match on
ErrorKind::InvalidOperationand assert message hints; good negative testing.
79-88: LGTM: size filter happy path.Parse to
u64with context and assert exact byte count.
91-116: LGTM: size filter missing-file error path.Assert
InvalidOperationand include missing path in message; robust.tests/ninja_gen_tests.rs (6)
133-149: LGTM: deterministic manifest generation scenarios.Build graphs, call
generate, and assert exact text output withensure!; concise and deterministic.
151-157: LGTM: empty graph case.Assert empty output via
Result<()>pattern.
159-203: Snapshot test is robust and explicit.Write to fixed snapshot dir, assert encoding strategy (
printf %bwith\\n). Clear and stable.
295-360: LGTM: end-to-end Ninja integration with precise assertions.Use ambient authority safely, contextualise I/O, and verify status/file exist/content variants.
362-383: LGTM: missing-action error variant check.Pattern-match
NinjaGenError::MissingActionwithensure!.
386-428: LGTM: writer failure maps toFormaterror.Use a failing
fmt::Writeimpl to validate error mapping.tests/ninja_snapshot_tests.rs (2)
14-23: LGTM: falliblerun_okhelper.Return
Result<String>, assert success withensure!, and require UTF‑8 stdout; concise and clear.
60-88: LGTM: snapshot and dry‑run checks.Write build file with context, run Ninja tools, and assert “no work to do” on second pass.
src/stdlib/time/tests.rs (5)
7-40: LGTM: fallible helpers with clear diagnostics.Evaluate expressions with context, downcast to domain objects, and surface precise messages for type mismatches.
42-62: LGTM:now()semantics tests.Assert UTC default and custom offsets with bounded delta; precise and robust.
64-82: LGTM: invalid offset negative tests.Compile once, assert
InvalidOperationat evaluation, and produce actionable failure messages.
84-110: LGTM: timedelta construction, overflow, and ISO‑8601 properties.Exercise zero, accumulation, negatives, overflow detection, and ISO‑8601 formatting for timestamps/durations via
ensure!.Also applies to: 112-136, 138-149, 151-176, 178-197
2-6: Fix missing imports forErrorKindandUtcOffset(tests do not compile).
ErrorKind(from minijinja) andUtcOffset(from time) are referenced but not imported. Bring them into scope.Apply:
-use anyhow::{Context, Result, anyhow, ensure}; -use minijinja::{Environment, context, value::Value}; -use time::{Duration, OffsetDateTime, macros::datetime}; +use anyhow::{Context, Result, anyhow, ensure}; +use minijinja::{Environment, context, value::Value, ErrorKind}; +use time::{Duration, OffsetDateTime, UtcOffset, macros::datetime};Run
cargo clippy --all-targets -- -D warningsto verify no further unresolved items remain. As per coding guidelines.tests/manifest_jinja_tests.rs (5)
10-32: LGTM: strongly-typed test keys and fields.Model environment variables and manifest field names as enums with
as_str()accessors to reduce stringly-typed mistakes.Also applies to: 34-53
68-114: LGTM: assertion helpers forStringOrList.Normalise comparisons and produce targeted diagnostics; improves readability and reduces duplication.
137-171: LGTM: globals/env rendering tests with guarded environment mutation.Acquire
EnvLock, useEnvVarGuard, and assert command substitution viaensure!.
173-205: LGTM: missing env var diagnostics.Discriminate
ManifestErrorvs underlyingUndefinedErrorand assert messages include offending variable.
217-244: LGTM: comprehensive manifest/Jinja coverage with fallible flows.Exercise macros, control-flow, foreach expansions, typing/validation, per-field rendering, and error paths. Assertions use
ensure!with explicit messages; helpers keep tests DRY.Also applies to: 287-309, 311-328, 330-346, 348-353, 355-382, 407-435, 456-481, 483-504, 506-524, 526-542, 544-560, 562-590, 592-626, 628-652
tests/std_filter_tests/command_filters.rs (4)
21-41: Robust fixture constructor and error contexts — LGTMGood UTF‑8 validation, capability‑scoped Dir open, and precise contexts. Keep as is.
87-140: Clear success/failure paths with strong diagnostics — LGTMAccurately asserts error kind and message fragments; propagates via Result with ensure/bail.
142-168: Guard against timeout‑test flakiness on slow CIRunning
sleep 10relies on the filter’s timeout and may elongate suites on constrained runners. Parameterize or lower the exercised duration to keep wall‑clock tight; verify stability on your slowest CI tier.
358-386: Windows meta‑characters preserved — LGTMQuoting strategy and assertion look correct; PATH isolation via EnvLock/EnvVarGuard avoids bleed‑through.
tests/std_filter_tests/support.rs (3)
26-36: Fallible template registration with context — LGTMReturns Result with targeted context; aligns with stricter lints and avoids panics.
54-70: Deterministic workspace scaffolding — LGTMUTF‑8 path handling, capability‑scoped I/O, and OS‑specific link setup are correct.
72-86: Render helper composes registration/fetch with context — LGTMKeeps failures inspectable without leaking panics.
tests/std_filter_tests/hash_filters.rs (3)
54-68: Algorithm/fixture branching is correct — LGTMCreates empty fixtures only for
*-emptycases and normalises the algorithm name appropriately.
97-124: Legacy‑digest gating validated via error shape — LGTMAsserts
InvalidOperationand feature hint precisely.
126-182: Unknown algorithm path validated for both hash and digest — LGTMConsistent error‑kind and message checks; clean propagation.
tests/manifest_glob_tests.rs (2)
37-46: Result‑basedtarget_nameswith precise diagnostics — LGTMAvoids silent panics when encountering list names; surfaces actionable errors.
124-163: Parametrized glob behaviour test reads clearly — LGTMGood normalisation of path separators and exact equality checks with
ensure!.tests/std_filter_tests/path_filters.rs (4)
29-41: Filter env setup returns Result — LGTMCentralises env creation and keeps callers fallible; matches lint policy.
55-84: Error‑shape assertion helper is tight and reusable — LGTMAsserts both kind and substring; emits helpful names in failures.
119-149: Success‑path helper asserts value and propagates context — LGTMUses EnvLock‑guarded HOME shaping and a single render path via
fallible::render.
213-227: dirname filter test is self‑contained and deterministic — LGTMUses workspace root and checks exact output; no external dependencies.
test_support/src/env_lock.rs (1)
18-21: Excellent graceful recovery from mutex poisoning.The change from
expect()tounwrap_or_else(|poisoned| poisoned.into_inner())prevents test failures when the lock is poisoned, extracting the guard and allowing tests to continue. This hardening aligns perfectly with the PR objective of introducing graceful fallbacks.tests/hasher_tests.rs (1)
77-84: LGTM! Excellent Result-based test conversion.The test now returns
Result<()>and usesensure!for assertions with descriptive error messages. The?operator propagates errors fromActionHasher::hash(), providing better diagnostic context when failures occur.tests/ninja_env_tests.rs (2)
18-42: LGTM! Thorough Result-based test conversion with rich context.The test function now returns
Result<()>and useswith_context()to provide descriptive error messages for environment variable retrieval. Theensure!macro replacements offer clearer assertion failure messages than the previous pattern. This aligns excellently with the PR's goal of context-rich error handling.
46-78: LGTM! Consistent Result-based error handling.The test mirrors the pattern established in the first test, with appropriate context messages and clear assertions. The restoration logic at lines 66-76 correctly maintains test isolation whilst preserving the unsafe environment mutation within the
EnvLockguard.src/manifest/diagnostics.rs (5)
18-22: LGTM! Documentation added for public ManifestSource methods.The doc comments clearly explain the constructor and accessor, with appropriate examples. This fulfils the PR objective of documenting previously undocumented public APIs.
Also applies to: 24-28
67-77: LGTM! Documentation added for public ManifestName methods.The doc comments provide clear descriptions and examples for the constructor and accessor, completing the public API surface documentation.
126-133: LGTM! Improved clarity with intermediate variables.The refactoring introduces
without_newlineandcleaned_linevariables that make the newline-stripping logic more readable. The change from computing the column offset directly to using the cleaned line improves maintainability.
176-185: LGTM! Cleaner early-return pattern.The refactored
has_tab_indentuses an early return withlet Some(actual_loc) = location else { return false }, which is more idiomatic than nestedif letormatch. The variable renaming fromloctolocationalso improves clarity.
288-305: LGTM! Result-based test with improved error handling.The test now returns
Result<()>and uses pattern matching on the parse result withanyhow!for error construction andensure!for validation. The error messages are descriptive and aid debugging.test_support/src/check_ninja.rs (1)
13-59: Excellent Result-based error propagation with rich context.The function signature change to
Result<(TempDir, PathBuf)>replaces panic-based failures with explicit error propagation. Each fallible operation includes descriptive context via.context()or.with_context(), providing clear diagnostic information including file paths. This exemplifies the PR objective of replacing unchecked operations with fallible paths and richer error messages.tests/ir_from_manifest_tests.rs (2)
12-26: LGTM! Result-based test with descriptive assertions.The test now returns
Result<()>and uses.context()for error propagation andensure!for assertions with clear messages including actual vs expected counts. This provides excellent diagnostic information when failures occur.
98-171: LGTM! Comprehensive error validation with rich context.The parameterised test now returns
Result<()>and useswith_context(),ensure!, andbail!for thorough error case validation. The match arms compare actual and expected error variants with descriptive failure messages. The fallbackbail!at line 168 catches unexpected error types, providing clear diagnostics.src/manifest/glob.rs (3)
59-59: LGTM! Prefer.to_owned()for&strtoStringconversions.The changes from
.to_string()to.to_owned()are more semantically appropriate for converting&strtoString, as the source is already a string type rather than requiringDisplayformatting.Also applies to: 66-66, 96-96, 406-406
474-493: LGTM! Result-based test with proper error validation.The test now returns
Result<()>and uses pattern matching withanyhow!for unexpected success andensure!to validate the error kind. The error handling is robust and provides clear diagnostic messages.
497-520: LGTM! Result-based test with context-rich error handling.The Unix-specific test correctly returns
Result<()>and uses.context()for the ambient directory opening. The match arms provide clear error messages for both unexpected success and incorrect error kinds.src/manifest/expand.rs (1)
76-102: Good: harden var injection and error surfaces.Preserve map order, validate
varsshape, and return typed failures with context.test_support/src/lib.rs (1)
66-98: Good: makefake_ninjafallible with contextual I/O errors.Return
Result, add precisewith_context, and preserve platform‑specific behaviour.tests/cli_tests.rs (1)
48-56: Avoid calling a panicking CLI parser in tests.Cli::parse_from_with_default panics on parse failure; keep tests fully fallible under -D warnings.
Apply this diff:
- let cli = Cli::parse_from_with_default(argv.clone()); + let cli = Cli::try_parse_from(argv.clone()) + .map_err(|e| anyhow::anyhow!("CLI parsing failed: {e}"))? + .with_default_command();Optionally add a non-panicking API alongside and migrate call sites:
// src/cli.rs pub fn try_parse_from_with_default<I, T>(args: I) -> clap::error::Result<Self> where I: IntoIterator<Item = T>, T: Into<std::ffi::OsString> + Clone, { Self::try_parse_from(args).map(|s| s.with_default_command()) }Run the checks below to surface any remaining crate- or module‑wide allows and panicking parsers:
tests/assert_cmd_tests.rs (2)
7-32: LGTM!The Result-based migration is correct. Context-rich error handling with
ensure!provides clear diagnostics.
34-60: LGTM!Error propagation and context usage are appropriate throughout the test.
tests/manifest_env_tests.rs (3)
3-12: LGTM!Imports and helper function are appropriate for the Result-based test flow.
14-45: LGTM!Parameterised test correctly uses Result propagation and contextual error messages.
47-99: LGTM!Error validation tests correctly use match expressions to verify expected failures and inspect error chains.
test_support/src/command_helper.rs (3)
48-50: LGTM!The function correctly delegates to
compile_rust_helperand returns its Result directly.
70-72: LGTM!Identical delegation pattern; correctly returns the Result from
compile_rust_helper.
100-123: LGTM!The implementation correctly propagates errors with context and uses
bail!for non-success status.tests/runner_tests.rs (6)
3-15: LGTM!Imports are appropriate for the Result-based test infrastructure.
24-29: LGTM!Fixture correctly propagates errors from
fake_ninja_check_build_file().
39-46: LGTM!Parameterised fixture correctly propagates errors and uses rstest default values.
49-55: LGTM!Helper function uses appropriate context messages for file operations.
58-116: LGTM!Tests correctly validate error conditions using
bailandensurewith descriptive messages.
118-269: LGTM!Test suite comprehensively migrated to Result-based flow with appropriate assertions and error context.
tests/cucumber.rs (4)
72-79: LGTM!The migration to Result-based error handling is correct, with appropriate context on the spawn operation.
122-125: LGTM!Replacing
eprintln!withtracing::warn!provides structured logging for diagnostics.
155-160: LGTM!Early return with structured logging when TCP binding fails is appropriate for test harness.
168-168: LGTM!Using
tracing::warn!for Unix filesystem feature skipping is appropriate.src/manifest/tests.rs (4)
61-87: LGTM!Parameterised tests correctly use
ensure!for positive cases andmatchwithanyhow!for error validation.
116-172: LGTM!Macro registration tests correctly propagate errors and use
ensure!for validation.
175-272: LGTM!Manifest macro tests correctly validate both success and error paths with descriptive context.
275-332: LGTM!The test gracefully handles permission errors and thoroughly validates cache file placement with descriptive assertions.
tests/ast_tests.rs (6)
3-44: LGTM!The minimal manifest test correctly uses
ensure!for assertions andbail!for type mismatches.
45-138: LGTM!Tests correctly validate required fields and error cases using
ensure!with descriptive messages.
140-321: LGTM!Tests thoroughly validate empty collections and variant types with appropriate error handling.
323-406: LGTM!Macro definition tests correctly validate parsing and round-trip serialisation with Unicode characters.
448-569: LGTM!Flag validation tests correctly use
ensure!for boolean assertions across multiple scenarios.
572-618: LGTM!File loading tests correctly use Result-based error propagation. The
invalid_manifests_failtest appropriately usesassert!for simple failure checks.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
tests/cucumber.rs (3)
60-64: Add doc comments toHttpShutdownModefor consistency.The enum and its variants lack documentation. Given the thorough documentation elsewhere in this file, add doc comments explaining the Strict vs Lenient teardown semantics.
Apply this diff to document the enum:
+/// Controls how the HTTP server fixture teardown behaves when the host cannot be extracted. #[derive(Copy, Clone)] enum HttpShutdownMode { + /// Panic if the host cannot be extracted, ensuring test failures are visible. Strict, + /// Log a warning and skip join if the host cannot be extracted, avoiding hangs. Lenient, }
67-128: Add doc comments topub(crate)methods for consistency.The struct fields are thoroughly documented, but the internal methods lack doc comments. Add /// comments to
start_http_server,shutdown_http_server,restore_environment, andshutdown_http_server_withto explain their purpose and any invariants.As per coding guidelines.
123-123: Remove redundant explicitdrop.The explicit
drop(server)is unnecessary; the value is dropped implicitly at the end of the block. Remove it for cleaner code.Apply this diff:
HttpShutdownMode::Lenient => { tracing::warn!( "Warning: Cannot extract host from stdlib_url; skipping server shutdown to avoid hang. URL: {:?}", self.stdlib_url ); - drop(server); }tests/runner_tests.rs (1)
89-116: Replace Unix-specific/dev/nullwith a benign path in the cross-platform test.The
cli.filevalue is unused byrun_ninja(which only accessescli.directoryandcli.jobsinconfigure_ninja_command). Remove platform-specific references that cause Windows CI failures:- let cli = Cli { - file: PathBuf::from("/dev/null"), + let cli = Cli { + file: PathBuf::from("Netsukefile"),
♻️ Duplicate comments (14)
src/ir/cmd_interpolate.rs (2)
177-192: Verify that test functions have#[expect(clippy::expect_used)]attributes.Line 185 uses
.expect_err()and line 190 usespanic!, but no#[expect(clippy::expect_used, reason = "...")]attribute is visible on the test function. Per the coding guidelines,#[allow]is forbidden and narrowly scoped#[expect]is required for lint suppressions. Ifclippy::expect_usedis denied (as suggested by the tightened lint policy), add the attribute.Run this script to check whether the lint is active and whether expects are properly attributed:
#!/bin/bash # Verify clippy::expect_used lint configuration and test annotations # Check if clippy::expect_used is denied in clippy configuration echo "=== Checking clippy configuration ===" fd -t f "clippy.toml|.clippy.toml" --exec cat {} # Check if expect_used appears in Cargo.toml lint configuration echo "" echo "=== Checking Cargo.toml lint configuration ===" rg -A 5 "clippy.*expect" Cargo.toml # Search for #[expect] attributes in test modules echo "" echo "=== Checking for #[expect] attributes in tests ===" rg -n "#\[expect\(.*expect" src/ir/cmd_interpolate.rs # Verify the test functions lack #[expect] attributes echo "" echo "=== Test functions in cmd_interpolate.rs ===" ast-grep --pattern $'#[test] fn $FUNC_NAME() { $$$ }'
28-36: Useerr.as_bytes()to avoid an unnecessary allocation.Line 34 uses
&err.into_bytes(), which consumes the error to extract theVec<u8>, then immediately borrows it. Replace witherr.as_bytes()to borrow the bytes directly without allocating.Apply this diff:
match String::from_utf8(bytes) { Ok(text) => text, Err(err) => { debug_assert!(false, "shell quoting produced non UTF-8 bytes: {err}"); - String::from_utf8_lossy(&err.into_bytes()).into_owned() + String::from_utf8_lossy(err.as_bytes()).into_owned() } }src/stdlib/command.rs (3)
417-429: Simplify: String formatting cannot fail in practice.Lines 421-425 guard
write_fmtonStringwith an error check. Writing format_args containing only ani32to aStringcannot fail unless allocation fails (OOM). Simplify by usingwrite!directly with.expect()or rely on the documented impossibility.Apply this diff:
let mut msg = format!( "command '{command}' in template '{template}' failed: {source} (command closed input early)" ); if let Some(code) = status { - if let Err(fmt_err) = - FmtWrite::write_fmt(&mut msg, format_args!("; exited with status {code}")) - { - tracing::warn!("failed to append exit status: {fmt_err}"); - } + write!(&mut msg, "; exited with status {code}") + .expect("writing to String cannot fail"); } else { msg.push_str("; terminated by signal"); }
267-274: Useerr.as_bytes()to avoid an unnecessary allocation.Line 272 uses
&err.into_bytes(), which consumes the error to extract theVec<u8>, then immediately borrows it. Replace witherr.as_bytes()to borrow the bytes directly without allocating.Apply this diff:
let bytes: Vec<u8> = arg.quoted(Sh); match String::from_utf8(bytes) { Ok(text) => Ok(text), Err(err) => { debug_assert!(false, "quoted args must be valid UTF-8: {err}"); - Ok(String::from_utf8_lossy(&err.into_bytes()).into_owned()) + Ok(String::from_utf8_lossy(err.as_bytes()).into_owned()) } }
528-581: Well-structured data-driven test with simplification opportunity.The refactoring to
anyhow::Resultand data-driven cases improves error messages and reduces duplication.Simplify the error-case assertions at lines 565-579:
for (input, expected) in error_cases { - match quote(input) { - Ok(actual) => { - return Err(anyhow!( - "quote({input:?}) succeeded with {actual:?} but expected error {expected:?}" - )); - } - Err(err) => { - ensure!( - err == expected, - "quote({input:?}) returned error {err:?}, expected {expected:?}" - ); - } - } + let err = quote(input).expect_err(&format!( + "quote({input:?}) should fail with {expected:?}" + )); + ensure!( + err == expected, + "quote({input:?}) -> {err:?}, expected {expected:?}" + ); }tests/steps/mod.rs (1)
5-9: Replace module-wide#![allow]with narrowly scoped#[expect]attributes.This module-level suppression violates the coding guideline that "
#[allow]is FORBIDDEN" and "no blanket or file-wide suppression". Apply#[expect(lint, reason = "...")]to individual step functions in the re-exported modules where the lints are triggered, not to this module root.As per coding guidelines.
tests/steps/process_steps.rs (1)
2-6: Replace module-level#![allow]with function-scoped#[expect]attributes.The coding guidelines explicitly forbid
#[allow]and permit only narrowly scoped#[expect(lint, reason = "...")]attributes. This module-level suppression violates the "no blanket or file-wide suppression" rule.As per coding guidelines.
Apply function-scoped
#[expect(...)]to individual step functions that triggerclippy::shadow_reuseorclippy::shadow_unrelated(e.g.,install_fake_ninja,fake_ninja_check,no_ninja, etc.):#[expect( clippy::shadow_reuse, clippy::shadow_unrelated, reason = "Cucumber step macros rebind capture identifiers" )] #[given(expr = "a fake ninja executable that exits with {int}")] fn install_fake_ninja(world: &mut CliWorld, exit_code: i32) -> Result<()> { // ... }Then remove the top-level
#![allow(...)]entirely.src/runner/mod.rs (1)
172-183: LGTM: propagate manifest path errors and document them.The
?onresolve_manifest_pathplus the new “Errors” Rustdoc satisfy prior guidance and improve diagnostics.Also applies to: 185-208
src/manifest/tests.rs (1)
38-44: LGTM: warning includes target directory.The log now reports the intended path, improving triage.
tests/steps/ir_steps.rs (1)
2-6: Replace forbidden file‑wide #[allow] with narrowly scoped #[expect] on step functions.Do not silence lints at module scope. Scope
shadow_*expectations to individual step fns that Cucumber rebinding actually affects. Remove the blanket attribute and add targeted expectations.Apply:
-#![allow( - clippy::shadow_reuse, - clippy::shadow_unrelated, - reason = "Cucumber step macros rebind capture names and steps prefer expect" -)]Add per‑fn expectations (sample set; repeat on other step fns in this file that trigger the lints):
@@ #[given("a new BuildGraph is created")] -fn create_graph(world: &mut CliWorld) { +#[expect(clippy::shadow_reuse, clippy::shadow_unrelated, reason = "Cucumber macros rebind capture names")] +fn create_graph(world: &mut CliWorld) { @@ #[then(expr = "the graph has {int} actions")] -fn graph_actions(world: &mut CliWorld, count: usize) -> Result<()> { +#[expect(clippy::shadow_reuse, clippy::shadow_unrelated, reason = "Cucumber macros rebind capture names")] +fn graph_actions(world: &mut CliWorld, count: usize) -> Result<()> { @@ #[then(expr = "the graph has {int} targets")] -fn graph_targets(world: &mut CliWorld, count: usize) -> Result<()> { +#[expect(clippy::shadow_reuse, clippy::shadow_unrelated, reason = "Cucumber macros rebind capture names")] +fn graph_targets(world: &mut CliWorld, count: usize) -> Result<()> { @@ #[then(expr = "the graph has {int} default targets")] -fn graph_defaults(world: &mut CliWorld, count: usize) -> Result<()> { +#[expect(clippy::shadow_reuse, clippy::shadow_unrelated, reason = "Cucumber macros rebind capture names")] +fn graph_defaults(world: &mut CliWorld, count: usize) -> Result<()> { @@ #[given(expr = "the manifest file {string} is compiled to IR")] #[when(expr = "the manifest file {string} is compiled to IR")] -fn compile_manifest(world: &mut CliWorld, path: String) { +#[expect(clippy::shadow_reuse, clippy::shadow_unrelated, reason = "Cucumber macros rebind capture names")] +fn compile_manifest(world: &mut CliWorld, path: String) { @@ #[when("its contents are checked")] -fn graph_checked(world: &mut CliWorld) -> Result<()> { +#[expect(clippy::shadow_reuse, clippy::shadow_unrelated, reason = "Cucumber macros rebind capture names")] +fn graph_checked(world: &mut CliWorld) -> Result<()> { @@ #[when("the graph contents are checked")] -fn graph_contents_checked(world: &mut CliWorld) -> Result<()> { +#[expect(clippy::shadow_reuse, clippy::shadow_unrelated, reason = "Cucumber macros rebind capture names")] +fn graph_contents_checked(world: &mut CliWorld) -> Result<()> { @@ #[when("the generation result is checked")] -fn generation_result_checked(world: &mut CliWorld) -> Result<()> { +#[expect(clippy::shadow_reuse, clippy::shadow_unrelated, reason = "Cucumber macros rebind capture names")] +fn generation_result_checked(world: &mut CliWorld) -> Result<()> { @@ #[then("IR generation fails")] -fn ir_generation_fails(world: &mut CliWorld) -> Result<()> { +#[expect(clippy::shadow_reuse, clippy::shadow_unrelated, reason = "Cucumber macros rebind capture names")] +fn ir_generation_fails(world: &mut CliWorld) -> Result<()> { @@ #[when("an action is removed from the graph")] -fn remove_action(world: &mut CliWorld) -> Result<()> { +#[expect(clippy::shadow_reuse, clippy::shadow_unrelated, reason = "Cucumber macros rebind capture names")] +fn remove_action(world: &mut CliWorld) -> Result<()> {Run to confirm no
#[allow]remain and no unused#[expect]appear:#!/bin/bash rg -nP '^\s*#!\[(allow|expect)\(' -g 'tests/steps/**/*.rs' cargo clippy --all-targets -- -D warningstests/steps/ninja_steps.rs (1)
2-6: Replace file‑wide #[allow] with narrowly scoped #[expect].Remove the blanket suppression and attach
shadow_*expectations to only the affected step fns.Apply:
-#![allow( - clippy::shadow_reuse, - clippy::shadow_unrelated, - reason = "Cucumber step macros rebind capture names and steps prefer expect" -)]Add per‑fn expectations (sample):
@@ -#[when("the ninja file is generated")] -fn generate_ninja(world: &mut CliWorld) -> Result<()> { +#[expect(clippy::shadow_reuse, clippy::shadow_unrelated, reason = "Cucumber macros rebind capture names")] +#[when("the ninja file is generated")] +fn generate_ninja(world: &mut CliWorld) -> Result<()> { @@ -#[then(expr = "the ninja file contains {string}")] -fn ninja_contains(world: &mut CliWorld, expected_fragment: String) -> Result<()> { +#[expect(clippy::shadow_reuse, clippy::shadow_unrelated, reason = "Cucumber macros rebind capture names")] +#[then(expr = "the ninja file contains {string}")] +fn ninja_contains(world: &mut CliWorld, expected_fragment: String) -> Result<()> {Repeat for other step fns that trigger the lints.
Run:
#!/bin/bash rg -nP '^\s*#!\[(allow|expect)\(' tests/steps/ninja_steps.rs cargo clippy --all-targets -- -D warningstests/steps/fs_steps.rs (1)
2-6: Replace file‑wide #[allow] with scoped #[expect] on only offending step fns.Delete the blanket
allowand attach per‑fn expectations where Cucumber rebinding causes shadowing.Apply:
-#![allow( - clippy::shadow_reuse, - clippy::shadow_unrelated, - reason = "Cucumber step macros rebind capture names and steps prefer expect" -)]Add per‑fn expectations as needed, for example on
file_type_workspace.#!/bin/bash rg -nP '^\s*#!\[(allow|expect)\(' tests/steps/fs_steps.rs cargo clippy --all-targets -- -D warningstests/steps/cli_steps.rs (1)
5-9: Remove file-wide lint suppression; apply narrowly scoped expectations.This file-wide
#![allow(...)]block violates the coding guidelines: "#[allow]is forbidden. Only narrowly scoped#[expect(lint, reason = "...")]is allowed."Remove the file-wide suppression and add individual
#[expect(clippy::shadow_reuse, reason = "Cucumber macro rebinds capture")]or#[expect(clippy::shadow_unrelated, reason = "Cucumber macro rebinds unrelated capture")]attributes only to those step functions where the cucumber macro forces shadowing.As per coding guidelines.
tests/steps/manifest_steps.rs (1)
2-6: Remove file-wide lint suppression; use narrowly scoped expectations.This file-wide
#![allow(...)]block violates the coding guidelines: "#[allow]is forbidden. Only narrowly scoped#[expect(lint, reason = "...")]is allowed. No lint groups, no blanket or file-wide suppression."Remove this block and add individual
#[expect(clippy::shadow_reuse, reason = "...")]or#[expect(clippy::shadow_unrelated, reason = "...")]attributes only to the specific step functions where the cucumber macro forces shadowing.As per coding guidelines.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (23)
src/ast.rs(3 hunks)src/ir/cmd_interpolate.rs(1 hunks)src/main.rs(1 hunks)src/manifest/expand.rs(5 hunks)src/manifest/jinja_macros.rs(6 hunks)src/manifest/tests.rs(7 hunks)src/runner/mod.rs(5 hunks)src/runner/process/mod.rs(6 hunks)src/stdlib/command.rs(6 hunks)src/stdlib/path/hash_utils.rs(2 hunks)test_support/src/env.rs(2 hunks)test_support/src/lib.rs(6 hunks)tests/cucumber.rs(5 hunks)tests/env_path_tests.rs(4 hunks)tests/runner_tests.rs(10 hunks)tests/steps/cli_steps.rs(4 hunks)tests/steps/fs_steps.rs(4 hunks)tests/steps/ir_steps.rs(3 hunks)tests/steps/manifest_steps.rs(4 hunks)tests/steps/mod.rs(1 hunks)tests/steps/ninja_steps.rs(2 hunks)tests/steps/process_steps.rs(7 hunks)tests/yaml_error_tests.rs(2 hunks)
🧰 Additional context used
📓 Path-based instructions (6)
**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
**/*.rs: Every Rust module must begin with a module-level comment using //! explaining the module's purpose and utility
Document public APIs with Rustdoc comments (///) so documentation can be generated with cargo doc
Prefer immutable data; avoid unnecessary mut bindings
Handle errors with Result instead of panicking where feasible
Prefer semantic error enums deriving std::error::Error via thiserror for inspectable conditions
Clippy warnings must be denied; do not silence lints except as a last resort
Lint suppressions must be tightly scoped and include a clear reason; prefer #[expect(...)] over #[allow(...)]
Place function attributes after doc comments
Do not use return in single-line functions
Prefer single-line function definitions where appropriate (e.g., pub fn new(id: u64) -> Self { Self(id) })
Use predicate functions for conditional criteria with more than two branches
Where a function is too long, extract meaningfully named helpers adhering to separation of concerns and CQRS
Where a function has too many parameters, group related parameters into meaningfully named structs
When returning large error data, consider using Arc to reduce data moved
Prefer .expect() over .unwrap() on Results/Options
Use concat!() to combine long string literals instead of escaping newlines with backslashes
Use NewTypes to model domain values; prefer explicit tuple structs; leverage newt-hype and the-newtype crates as described; provide appropriate From/AsRef/TryFrom implementations and avoid orphan-rule-violating impls
Keep each Rust code file under 400 lines; split long switches/dispatch tables by feature; move large test data to external files
Use functions and composition to avoid repetition; extract reusable logic; prefer declarative constructs (iterators/generators/comprehensions) when readable
Small, single-responsibility functions; obey command/query segregation
Name things precisely; booleans should use is/has/should prefixes
Comment why, not what; explain assumptions, e...
Files:
src/main.rssrc/ir/cmd_interpolate.rstest_support/src/lib.rssrc/stdlib/path/hash_utils.rssrc/manifest/expand.rstests/steps/ir_steps.rstests/yaml_error_tests.rssrc/runner/process/mod.rstest_support/src/env.rssrc/stdlib/command.rstests/steps/mod.rssrc/runner/mod.rssrc/manifest/jinja_macros.rstests/env_path_tests.rstests/cucumber.rstests/steps/process_steps.rstests/steps/fs_steps.rstests/steps/ninja_steps.rssrc/manifest/tests.rstests/steps/cli_steps.rstests/runner_tests.rssrc/ast.rstests/steps/manifest_steps.rs
⚙️ CodeRabbit configuration file
**/*.rs: * Seek to keep the cyclomatic complexity of functions no more than 12.
Adhere to single responsibility and CQRS
Place function attributes after doc comments.
Do not use
returnin single-line functions.Move conditionals with >2 branches into a predicate function.
Avoid
unsafeunless absolutely necessary.Every module must begin with a
//!doc comment that explains the module's purpose and utility.Comments and docs must follow en-GB-oxendict (-ize / -yse / -our) spelling and grammar
Lints must not be silenced except as a last resort.
#[allow]is forbidden.- Only narrowly scoped
#[expect(lint, reason = "...")]is allowed.- No lint groups, no blanket or file-wide suppression.
- Include
FIXME:with link if a fix is expected.Where code is only used by specific features, it must be conditionally compiled or a conditional expectation for unused_code applied.
Use
rstestfixtures for shared setup and to avoid repetition between tests.Replace duplicated tests with
#[rstest(...)]parameterised cases.Prefer
mockallfor mocks/stubs.Prefer
.expect()over.unwrap()Ensure that any API or behavioural changes are reflected in the documentation in
docs/Ensure that any completed roadmap steps are recorded in the appropriate roadmap in
docs/Files must not exceed 400 lines in length
- Large modules must be decomposed
- Long match statements or dispatch tables should be decomposed by domain and collocated with targets
- Large blocks of inline data (e.g., test fixtures, constants or templates) must be moved to external files and inlined at compile-time or loaded at run-time.
Environment access (env::set_var and env::remove_var) are always unsafe in Rust 2024 and MUST be marked as such
- For testing of functionality depending upon environment variables, dependency injection and the
mockablecrate are the preferred option.- If mockable cannot be used, env mutations in...
Files:
src/main.rssrc/ir/cmd_interpolate.rstest_support/src/lib.rssrc/stdlib/path/hash_utils.rssrc/manifest/expand.rstests/steps/ir_steps.rstests/yaml_error_tests.rssrc/runner/process/mod.rstest_support/src/env.rssrc/stdlib/command.rstests/steps/mod.rssrc/runner/mod.rssrc/manifest/jinja_macros.rstests/env_path_tests.rstests/cucumber.rstests/steps/process_steps.rstests/steps/fs_steps.rstests/steps/ninja_steps.rssrc/manifest/tests.rstests/steps/cli_steps.rstests/runner_tests.rssrc/ast.rstests/steps/manifest_steps.rs
**/*.{rs,md}
📄 CodeRabbit inference engine (AGENTS.md)
Use en-GB-oxendict spelling and grammar in comments and documentation (exceptions allowed for external API names)
Files:
src/main.rssrc/ir/cmd_interpolate.rstest_support/src/lib.rssrc/stdlib/path/hash_utils.rssrc/manifest/expand.rstests/steps/ir_steps.rstests/yaml_error_tests.rssrc/runner/process/mod.rstest_support/src/env.rssrc/stdlib/command.rstests/steps/mod.rssrc/runner/mod.rssrc/manifest/jinja_macros.rstests/env_path_tests.rstests/cucumber.rstests/steps/process_steps.rstests/steps/fs_steps.rstests/steps/ninja_steps.rssrc/manifest/tests.rstests/steps/cli_steps.rstests/runner_tests.rssrc/ast.rstests/steps/manifest_steps.rs
**/src/main.rs
📄 CodeRabbit inference engine (AGENTS.md)
Convert domain errors to eyre only in main() or top-level async tasks; keep opaque errors at the app boundary
Files:
src/main.rs
**/src/lib.rs
📄 CodeRabbit inference engine (AGENTS.md)
Never export the opaque eyre::Report type from libraries; expose semantic domain error enums instead
Files:
test_support/src/lib.rs
**/tests/**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
**/tests/**/*.rs: Use rstest fixtures for shared setup
Replace duplicated tests with #[rstest(...)] parameterised cases
Prefer mockall for mocks and stubs
Files:
tests/steps/ir_steps.rstests/yaml_error_tests.rstests/steps/mod.rstests/env_path_tests.rstests/cucumber.rstests/steps/process_steps.rstests/steps/fs_steps.rstests/steps/ninja_steps.rstests/steps/cli_steps.rstests/runner_tests.rstests/steps/manifest_steps.rs
**/*{test,tests}.rs
📄 CodeRabbit inference engine (AGENTS.md)
Write unit and behavioural tests for new functionality and run them before and after changes
Files:
tests/yaml_error_tests.rstests/env_path_tests.rssrc/manifest/tests.rstests/runner_tests.rs
🧬 Code graph analysis (11)
tests/yaml_error_tests.rs (2)
tests/ast_tests.rs (1)
err(128-131)src/manifest/mod.rs (1)
from_str(146-148)
test_support/src/env.rs (3)
test_support/src/env_lock.rs (1)
acquire(17-22)test_support/src/env_guard.rs (1)
new(59-61)test_support/src/path_guard.rs (1)
new(27-31)
src/runner/mod.rs (1)
test_support/src/lib.rs (1)
resolve_manifest_path(147-162)
tests/env_path_tests.rs (4)
tests/std_filter_tests/command_filters.rs (2)
env(43-45)new(22-41)test_support/src/env.rs (9)
mocked_path_env(49-56)original(149-151)original(232-234)prepend_dir_to_path(176-190)set_var(29-29)set_var(33-35)set_var(39-41)set_var(62-68)remove_var(74-80)test_support/src/env_lock.rs (1)
acquire(17-22)test_support/src/path_guard.rs (1)
new(27-31)
tests/cucumber.rs (1)
test_support/src/http.rs (1)
spawn_http_server(58-72)
tests/steps/process_steps.rs (3)
test_support/src/env.rs (2)
prepend_dir_to_path(176-190)mocked_path_env(49-56)test_support/src/lib.rs (2)
fake_ninja(66-98)ensure_manifest_exists(126-145)test_support/src/check_ninja.rs (1)
fake_ninja_check_build_file(13-59)
tests/steps/ninja_steps.rs (2)
src/runner/mod.rs (1)
generate_ninja(172-183)tests/steps/ir_steps.rs (1)
graph(13-18)
src/manifest/tests.rs (3)
src/manifest/jinja_macros.rs (8)
parse_macro_name(38-57)register_macro(69-88)call_macro_value(140-155)new(259-265)new(299-317)new(339-346)new(403-410)register_manifest_macros(100-113)test_support/src/http.rs (2)
spawn_http_server(58-72)join(30-36)test_support/src/hash.rs (1)
sha256_hex(18-26)
tests/steps/cli_steps.rs (1)
tests/cli_tests.rs (1)
parse_cli(40-63)
tests/runner_tests.rs (3)
test_support/src/check_ninja.rs (1)
fake_ninja_check_build_file(13-59)test_support/src/env.rs (2)
prepend_dir_to_path(176-190)override_ninja_env(220-228)test_support/src/lib.rs (1)
fake_ninja(66-98)
tests/steps/manifest_steps.rs (3)
test_support/src/error.rs (2)
std(20-25)display_error_chain(17-27)tests/std_filter_tests/command_filters.rs (3)
env(43-45)new(22-41)command(47-49)test_support/src/env.rs (5)
remove_var(74-80)set_var(29-29)set_var(33-35)set_var(39-41)set_var(62-68)
⏰ Context from checks skipped due to timeout of 120000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: release / build-windows (x86_64-pc-windows-msvc, amd64, x64, windows-x86_64) / Build windows artefacts
- GitHub Check: release / build-windows (aarch64-pc-windows-msvc, arm64, arm64, windows-aarch64) / Build windows artefacts
- GitHub Check: release / build-macos (x86_64-apple-darwin, macos-13, macos-x86_64, macos-x86_64) / Build macos artefacts
- GitHub Check: release / build-linux (aarch64-unknown-linux-gnu, linux-arm64, arm64) / Build linux artefacts
- GitHub Check: Sourcery review
- GitHub Check: build-test (ubuntu-latest, stable)
🔇 Additional comments (53)
src/main.rs (2)
12-17: Clean refactoring of logger initialization.The computed
max_levelvariable eliminates conditional branching and makes the logger setup more declarative. The logic is clear: DEBUG for verbose mode, ERROR otherwise.
21-21: Structured logging properly implemented.The structured field syntax with a named
errorfield and descriptive message fully addresses the previous review feedback. Logging backends can now index and query the error field effectively.tests/yaml_error_tests.rs (2)
11-13: Excellent migration to Result-based error handling.The switch from
.expect()to.context()provides richer error messages and eliminates panics in the helper function. This is superior to the previously suggested#[expect]attribute approach.
88-102: Result-based test pattern is cleaner and more idiomatic.The migration to
Result<()>withbail!,ensure!, and the?operator eliminates panics whilst maintaining clear test failures. Thelet Err(err) = ... else { bail!(...) }pattern correctly validates that parsing fails as expected. This approach surpasses the previously requested#[expect]attributes by removing the need for lint suppressions entirely.tests/cucumber.rs (3)
14-55: Excellent field-level documentation.The comprehensive doc comments on all public fields substantially improve the discoverability and maintainability of the test harness state.
67-74: Good conversion to Result-based error handling.Returning
Result<()>and using.context()aligns with the coding guidelines and improves error messages for test failures.
152-157: Approve the defensive TCP binding check.Checking TCP availability upfront and skipping tests gracefully is sound defensive programming for a test harness that relies on HTTP fixtures. The use of
tracing::warn!provides clear visibility.src/ast.rs (3)
51-59: LGTM! Well-structured macro definition.The struct follows guidelines: public API is documented,
deny_unknown_fieldsenforces strict deserialisation, and both fields have clear doc comments.
164-164: Past issue resolved.The addition of
deny_unknown_fieldsaddresses the previous review comment. Extra keys at the flattenedRecipelevel will now trigger errors rather than being silently swallowed.
142-155: LGTM! Comprehensive documentation.The doc comments for Recipe variants and their fields are clear and follow Rustdoc conventions.
src/stdlib/command.rs (4)
347-363: LGTM: robust cleanup with diagnostic logging.The function appropriately logs warnings when reader joins or stdin writer joins fail rather than propagating errors during cleanup. This ensures graceful degradation and provides diagnostic information via structured logging.
475-489: LGTM: appropriate diagnostic logging for edge case.Lines 484-486 log a warning when
child.wait()fails after a timeout and kill. This provides useful diagnostic information without affecting the timeout error path.
504-513: LGTM: improved parameter naming.Renaming the parameter to
join_handleat line 507 enhances clarity and makes the type relationship more explicit.
515-522: LGTM: clear variable naming.The variable naming at lines 516-517 improves readability by making the UTF-8 lossy conversion and trimming explicit.
src/stdlib/path/hash_utils.rs (1)
94-96: LGTM: defensive handling for infallible write.Writing to a
Stringviawrite!cannot fail, so the guarded path withdebug_assert!is sound defensive programming. The assertion catches the impossible case in debug builds while release builds continue gracefully.test_support/src/env.rs (1)
176-189: LGTM: error propagation successfully replaces panic.The function signature now returns
Result<PathGuard>and propagates join errors with contextual messages. This aligns with the broader PR shift towards Result-based error handling and successfully addresses the past review concern.test_support/src/lib.rs (2)
66-97: LGTM: comprehensive Result-based error handling.
fake_ninjanow returnsResult<(TempDir, PathBuf)>and propagates all errors with contextual messages. The platform-specific branches (Unix and Windows) both handle file creation, writing, and permissions uniformly with.context()and.with_context().
291-363: LGTM: tests adopt Result-based flows.All test functions now return
Result<()>and useanyhow::Contextfor error propagation andanyhow::ensure!for assertions with custom messages. This provides clearer diagnostics on test failures.src/manifest/jinja_macros.rs (2)
195-204: LGTM: improved error handling for uninitialised macro.The closure now handles
cache.instance()returningNoneby producing a descriptiveInvalidOperationerror. This aligns with the updatedMacroCache::instance()signature returningOption<&MacroInstance>and provides clear diagnostics when a macro is invoked before initialisation.
270-272: LGTM: explicit handling of OnceLock set error.Explicitly dropping the returned instance when
OnceLock::setfails (due to concurrent initialisation) is clearer than discarding theResult. This acknowledges the rare race and avoids leaking the redundant instance.tests/steps/process_steps.rs (1)
27-38: LGTM: comprehensive Result-based error handling in step functions.All step functions now return
Result<()>and propagate errors with contextual messages usinganyhow::Context,anyhow::ensure!, andanyhow::bail!. This provides clearer diagnostics when tests fail and aligns with the broader PR migration towards Result-based flows.Also applies to: 42-48, 52-56, 64-69, 73-84, 88-95, 107-146, 150-156, 160-170, 178-192
tests/env_path_tests.rs (1)
15-37: LGTM: tests adopt Result-based flows with rich diagnostics.All three test functions now return
Result<()>and useanyhow::Contextfor error propagation andanyhow::ensure!for assertions with custom messages. This provides clearer diagnostics when PATH manipulation tests fail and aligns with the broader test-suite migration to Result-based patterns.Also applies to: 42-74, 78-109
src/manifest/expand.rs (3)
92-94: LGTM: replaces panic with error propagation.Changing from
.expect("vars entry ensured to be an object")to.ok_or_else(|| anyhow::anyhow!("..."))replaces a potential panic with a propagated error. This aligns with the PR's broader shift towards Result-based error handling.
122-127: LGTM: helper reduces test duplication.The new
targets()helper extracts the targets sequence with context, eliminating repeatedget/and_then/as_array/contextblocks across test functions. This improves maintainability and provides consistent error messages.
130-167: LGTM: tests adopt Result-based flows with rich diagnostics.All test functions now return
Result<()>and use the newtargets()helper,anyhow::ensure!,anyhow::bail!, and.context()for assertions and error propagation. This provides clearer diagnostics on test failures and aligns with the broader repository pattern.Also applies to: 171-202, 206-231
tests/steps/ir_steps.rs (2)
13-18: Good: adopt Result-based step helpers with context.Retain these non‑panicking flows; error messages are precise and aid triage.
Also applies to: 20-25, 27-33, 95-99, 102-105, 108-110, 113-119, 122-131
41-49: Good: replace asserts with ensure for cardinality checks.Keep using
ensure!with actual/expected in the message for crisp failures.Also applies to: 52-60, 63-71
src/runner/mod.rs (2)
26-45: LGTM: simple, well‑documented NinjaContent wrapper.Keep
new/as_str/into_stringas is; this is idiomatic and testable.
53-68: LGTM: BuildTargets API and Default.The manual Default with an explicit reasoned
#[expect]is appropriate.Also applies to: 70-78
tests/runner_tests.rs (2)
24-29: LGTM: fixtures return Result and guard unsafe env mutation.Fixtures serialise PATH mutations via
EnvLock/guards and propagate errors cleanly.Also applies to: 39-46, 48-55
120-145: LGTM: runner behaviours validated with clear diagnostics.Keep these checks on non‑persistence, explicit emit paths, nested dirs, manifest subcommand, and env override.
Also applies to: 150-186, 191-218, 220-244, 248-269
tests/steps/ninja_steps.rs (1)
58-88: LGTM: robust ninja parsing and guarded assertions.Preserve 1‑based indexing for the step,
with_contexton OOB, andensure!for equality.Also applies to: 90-93, 100-110, 112-127
tests/steps/fs_steps.rs (1)
17-24: LGTM: fallible workspace/device fixture setup with precise context.Keep the
Utf8PathBufconversions and explicit context messages; they harden diagnostics.Also applies to: 26-44, 53-66, 68-75, 77-101, 126-139, 142-150
src/manifest/tests.rs (1)
64-71: LGTM: Result‑based tests with contextual errors and shaped assertions.Retain
AnyResult,ensure!, and crisp messages across macro parsing/registration and cache‑placement tests.Also applies to: 125-134, 137-157, 159-175, 178-196, 199-223, 225-246, 249-264, 267-275, 277-335
tests/steps/cli_steps.rs (3)
18-38: Well-structured CLI test helper.The
apply_clifunction properly handles both successful parsing and error cases, populating the appropriate world state fields. The default command logic for Build is appropriate for test scenarios.
40-47: Excellent Result-based error handling.The refactor from panic/expect to
Resultwith contextual error messages improves testability and debuggability. The error contexts clearly identify which state is missing.
49-235: Consistent Result-based error propagation throughout step definitions.All cucumber step functions properly migrate from panic-based assertions to
Result<()>with contextual error messages. The narrowly scoped#[expect(clippy::needless_pass_by_value, reason = "Cucumber requires owned String arguments")]attributes are appropriately justified and comply with the coding guidelines. Error contexts clearly identify the failure point (CLI not parsed, command missing, assertion failed with expected vs actual values).tests/steps/manifest_steps.rs (7)
21-38: Robust StringOrList extraction with clear error contexts.The
get_string_from_string_or_listhelper properly validates each variant (String, single-item List, Empty) and provides field-specific error contexts. The length check prevents panics on empty lists.
54-68: Clear assertion helpers.The
assert_manifestandassert_parsedhelpers provide straightforward validation with helpful error messages, correctly distinguishing between successful parsing and any parsing attempt.
70-80: Proper index validation and conversion.The
get_targethelper correctly enforces 1-based indexing for user-facing test steps whilst converting to 0-based for internal vec access. The validation prevents off-by-one errors.
116-139: Validated environment variable mutation.The
set_env_varandunset_env_varsteps properly validate non-empty keys and delegate to thetest_support::envhelpers (which handle global locking per the relevant snippets). The previous value capture enables proper teardown.
146-167: Validated manifest parsing steps.Both the
given_parse_manifestandparse_manifeststeps validate non-empty paths before invoking the parser. The Result return enables proper error propagation in the test harness.
174-181: Flexible validation dispatcher.The
when_item_checkedstep appropriately routes to different assertions based on the regex capture, with a clear error for unexpected values.
188-545: Comprehensive Result-based migration across all manifest validation steps.All step functions and assertion helpers consistently adopt
Result<()>returns with contextual error messages. Notable improvements include:
- 1-based index validation in multiple helpers (
get_target,macro_signature_is,assert_target_index) with clear error contexts.- Efficient list membership checks using
list.iter().any(|entry| entry == expected)(line 434) without unnecessary allocations.- Proper use of
ensure!,bail!, and.context()throughout for debugging-friendly test failures.- Clear distinction between missing state (manifest not parsed), invalid structure (unexpected recipe type), and assertion failures (expected vs actual mismatches).
The narrowly scoped
#[expect(clippy::needless_pass_by_value, reason = "Cucumber step requires owned String")]attributes comply with the coding guidelines.src/runner/process/mod.rs (9)
125-127: Docs: error conditions clarified. LGTM.
158-177: Thread join handling and diagnostics: OK.
181-186: ForwardStats shape: OK.
188-207: CountingWriter: OK; keep u64 and clamp on report.
209-211: Clamping helper: OK; no trait import required.This works without importing
TryFromexplicitly (see earlier comment).
276-283: Test imports: OK.
285-310: FailingWriter helper: OK.Accurately simulates early
BrokenPipe; thread‑safe viaArc<AtomicUsize>.
335-344: Test: parent alive path validates counters. LGTM.
346-358: Remove reference to non-existentCountingReader; the implementation already correctly drains bytes after write failure.The code implements the correct logic for this test without requiring
CountingReader. Theforward_child_outputfunction already:
- Sets
bytes_readto bytes written so far upon write failure (line 236)- Drains remaining bytes via
io::copy(&mut reader, &mut io::sink())(line 240)- Accumulates those drained bytes into
bytes_read(line 243)The test assertions align with the current implementation, so no code changes are needed.
Likely an incorrect or invalid review comment.
|
@coderabbitai Please suggest a fix for this issue and supply a prompt for an AI coding agent to enable it to apply the fix: Comment on lines +143 to +155 let stdout = child
.stdout
.take()
.ok_or_else(|| io::Error::other("child process missing stdout pipe"))?;
let stderr = child
.stderr
.take()
.ok_or_else(|| io::Error::other("child process missing stderr pipe"))?;
let out_handle =
thread::spawn(move || forward_child_output(BufReader::new(stdout), io::stdout(), "stdout"));
let err_handle =
thread::spawn(move || forward_child_output(BufReader::new(stderr), io::stderr(), "stderr"));❌ New issue: Complex Method |
|
@coderabbitai Please suggest a fix for this issue and supply a prompt for an AI coding agent to enable it to apply the fix: Comment on lines +143 to +155 let stdout = child
.stdout
.take()
.ok_or_else(|| io::Error::other("child process missing stdout pipe"))?;
let stderr = child
.stderr
.take()
.ok_or_else(|| io::Error::other("child process missing stderr pipe"))?;
let out_handle =
thread::spawn(move || forward_child_output(BufReader::new(stdout), io::stdout(), "stdout"));
let err_handle =
thread::spawn(move || forward_child_output(BufReader::new(stderr), io::stderr(), "stderr"));❌ New issue: Bumpy Road Ahead |
|
@coderabbitai Please suggest a fix for this issue and supply a prompt for an AI coding agent to enable it to apply the fix: Comment on lines +188 to +198 fn manifest_version(world: &mut CliWorld, version: String) -> Result<()> {
let manifest = world
.manifest
.as_ref()
.context("manifest has not been parsed")?;
let actual = manifest.netsuke_version.to_string();
ensure!(
actual == version,
"expected manifest version '{version}', got '{actual}'"
);
Ok(())❌ New issue: Code Duplication |
|
@coderabbitai Please suggest a fix for this issue and supply a prompt for an AI coding agent to enable it to apply the fix: Comment on file reason = "Cucumber requires owned String arguments"
)]
fn ninja_contains(world: &mut CliWorld, text: String) {
fn ninja_contains(world: &mut CliWorld, expected_fragment: String) -> Result<()> {❌ New issue: Code Duplication |
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 20
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (6)
tests/steps/stdlib_steps.rs (1)
1-513: Decompose the file to comply with the 400-line limit.This file contains 513 lines, exceeding the mandated 400-line maximum by 113 lines. The coding guidelines explicitly require: "Files must not exceed 400 lines in length" and "Large modules must be decomposed."
Split this module into focused sub-modules:
- workspace.rs — workspace setup, fixture creation, and directory operations (ensure_workspace, write_stdlib_file, home_points_to_stdlib_root, compile helper steps)
- rendering.rs — template rendering logic and steps (render_template_with_context, render_template, render_stdlib_template variations)
- assertions.rs — output and state assertion steps (assert_stdlib_output, assert_stdlib_impure, assert_fetch_cache_present, timestamp assertion steps)
- parsing.rs — parsing utilities (parse_iso_timestamp, parse_expected_offset, extract_host_from_url)
- types.rs — newtype wrappers (TemplatePath, TemplateContent, FileContent, RelativePath)
Re-export step functions in the parent mod.rs so Cucumber discovers them. This decomposition improves cohesion, reduces file size, and satisfies the coding guidelines.
As per coding guidelines.
🤖 Prompt for AI Agents
In tests/steps/stdlib_steps.rs, decompose the 513-line file into five focused sub-modules to comply with the 400-line limit: 1. Create tests/steps/stdlib_steps/types.rs and move lines 33-101 (TemplatePath, TemplateContent, FileContent, RelativePath newtype definitions) into it, keeping all From impls and methods intact. 2. Create tests/steps/stdlib_steps/parsing.rs and move lines 200-208 (extract_host_from_url), 351-354 (parse_iso_timestamp), and 356-391 (parse_expected_offset) into it, adding necessary imports (anyhow, time crate items). 3. Create tests/steps/stdlib_steps/workspace.rs and move lines 107-133 (ensure_workspace), 178-187 (uppercase_stdlib_command_helper), 189-198 (failing_stdlib_command_helper), 210-216 (http_server_returning), 218-238 (write_stdlib_file), 240-256 (home_points_to_stdlib_root), and 258-268 (resolve_template_path) into it, adding use crate::CliWorld and other necessary imports. 4. Create tests/steps/stdlib_steps/rendering.rs and move lines 135-160 (render_template_with_context), 162-169 (render_template), 172-176 (stdlib_workspace step), 270-281 (render_stdlib_template step), 283-290 (render_stdlib_template_without_path step), 292-300 (render_stdlib_template_with_url step), and 302-313 (render_stdlib_template_with_command step) into it, adding necessary imports. 5. Create tests/steps/stdlib_steps/assertions.rs and move lines 319-330 (assert_stdlib_output), 332-342 (stdlib_root_and_output), 344-349 (stdlib_output), 397-408 (assert_stdlib_error), 410-418 (assert_stdlib_impure), 420-428 (assert_stdlib_pure), 430-453 (assert_fetch_cache_present), 455-463 (assert_stdlib_output_is_root), 465-479 (assert_stdlib_output_is_workspace_path), 481-496 (assert_stdlib_output_is_utc_timestamp), and 502-513 (assert_stdlib_output_offset) into it, adding necessary imports. 6. Convert tests/steps/stdlib_steps.rs into tests/steps/stdlib_steps/mod.rs, retaining only lines 1-25 (module docs, file-wide expect attribute, use declarations, LINES_FIXTURE constant) and lines 103-105 (server_host helper if used elsewhere), and adding `pub mod types; pub mod parsing; pub mod workspace; pub mod rendering; pub mod assertions;` to expose the sub-modules. Re-export step functions using `pub use workspace::{stdlib_workspace, ...}; pub use rendering::{render_stdlib_template, ...}; pub use assertions::{assert_stdlib_output, ...};` so Cucumber discovers them. Ensure each new module begins with a //! doc comment explaining its purpose (e.g., "Workspace setup and fixture management for stdlib Cucumber steps."). Preserve all function signatures, attributes, doc comments, error messages, and en-GB spelling throughout.src/manifest/glob.rs (1)
142-147: Deduplicate and centralise “normalised-or-raw” fallback logicAvoid repeating the unwrap_or_else + debug_assert pattern. Add a small helper and reuse it here and in open_root_dir to keep semantics in one place.
Apply these diffs:
- let candidate = pattern - .normalized - .as_deref() - .unwrap_or(pattern.raw.as_str()); + let candidate = normalized_or_raw(pattern);- let normalized_pattern = pattern_state.normalized.as_deref().unwrap_or_else(|| { - debug_assert!(false, "normalized pattern must be present"); - pattern_state.raw.as_str() - }); + let normalized_pattern = normalized_or_raw(&pattern_state);Add this helper (near other small helpers in this module):
fn normalized_or_raw<'a>(p: &'a GlobPattern) -> &'a str { p.normalized.as_deref().unwrap_or_else(|| { debug_assert!(false, "normalised pattern must be present"); p.raw.as_str() }) }Prompt for AI agents: - Insert fn normalized_or_raw<'a>(p: &'a GlobPattern) -> &'a str as shown. - Replace both call sites: open_root_dir() and glob_paths() after assigning pattern_state.normalized. - Do not alter error handling or logging; keep debug_assert message as-is (en‑GB spelling “normalised” is preferred). - Run cargo clippy with -D warnings to confirm no new lints.Also applies to: 423-426
tests/ninja_snapshot_tests.rs (2)
71-76: Add debug trace for Ninja invocations.Log the build file and args before execution to speed up flake triage.
let ninja_cmd = |args: &[&str]| { let mut cmd = Command::new("ninja"); cmd.arg("-f").arg(&build_file).args(args); cmd.current_dir(&dir); + tracing::debug!(build_file = %build_file.display(), args = ?args, "running ninja"); run_ok(&mut cmd) };AI agent prompt: Insert the tracing line above inside the `ninja_cmd` closure in tests/ninja_snapshot_tests.rs.
1-7: Replace the file-level#![allow(...)]lint suppression intests/steps/mod.rs.The scan found a forbidden blanket
#![allow(...)]at tests/steps/mod.rs:5–7 coveringshadow_reuseandshadow_unrelated. Refactor to narrowly scoped#[expect(...)]with reason annotations applied directly to the functions or code blocks requiring exceptions, per policy.src/stdlib/time/tests.rs (1)
17-21: Replacebuild_envwith anrstestfixture to remove repetition.Define an
#[fixture]that returnsEnvironment<'static>and inject it into tests instead of callingbuild_env()repeatedly.- Add `use rstest::{rstest, fixture};`. - Create fixture: #[fixture] fn env() -> Environment<'static> { let mut e = Environment::new(); register_functions(&mut e); e } - Update tests to accept `env: Environment<'static>` parameter: #[rstest] fn now_defaults_to_utc(env: Environment<'static>) -> Result<()> { … } …and remove `let env = build_env();` lines. - Delete `build_env()` once all call sites are migrated.As per coding guidelines.
src/manifest/jinja_macros.rs (1)
309-317: Document and verify the lifetime safety contract for transmute—the current code is unsound.The
SAFETYcomment at line 312 claims "manifest macros are registered in an Environment<'static>" butregister_macro()acceptsEnvironment<'_>without enforcing 'static. Production code insrc/manifest/mod.rs:101creates a local (non-'static) Environment, passes it toregister_manifest_macros(), which callsregister_macro()where the transmute occurs. TheMacroInstancecontaining the transmutedState<'static, 'static>is stored in a long-livedArc<MacroCache>closure. When the local Environment is dropped at the end offrom_str_named(), any subsequent macro invocation would reference deallocated memory—a use-after-free bug.Fix: Either add a compile-time lifetime parameter
Environment<'static>toregister_macro(), or add a runtime assertion that validates the Environment is 'static before the transmute. Document the requirement explicitly inregister_macro()andregister_manifest_macros().
♻️ Duplicate comments (9)
tests/steps/process_steps.rs (1)
36-42: Add#[expect]attribute for parameter shadowing.The explicit rebinding of
exit_codeon line 37 triggersclippy::shadow_reuse. Apply a function-scoped#[expect]attribute to suppress the lint with a clear reason.Apply this diff to add the attribute:
+#[expect( + clippy::shadow_reuse, + reason = "convert i32 from cucumber macro to u8 for fake_ninja" +)] #[given(expr = "a fake ninja executable that exits with {int}")] fn install_fake_ninja(world: &mut CliWorld, exit_code: i32) -> Result<()> {🤖 Prompt for AI Agents
In tests/steps/process_steps.rs at line 36, add a function-scoped #[expect] attribute immediately before the #[given] attribute on the install_fake_ninja function to suppress clippy::shadow_reuse; the attribute must include reason = "convert i32 from cucumber macro to u8 for fake_ninja" and be placed on its own lines before the cucumber macro attribute.test_support/src/http.rs (2)
74-83: Scope panics with #[expect] — resolved.Annotate test‑fixture panics with reasons; aligns with lint gate.
As per coding guidelines.
91-111: Accept/read helpers correctly annotated and bounded.Keep fast‑fail panics scoped to tests with
#[expect]; time‑boxed accept/read loops look sane.As per coding guidelines.
src/ast.rs (2)
158-164: Enforce strict deserialisation on RawRecipe — fixed.Add
#[serde(deny_unknown_fields)]to avoid silently accepting extra keys.As per coding guidelines.
171-199: Match logic is precise; computepresentonly on error — fixed.Build the
presentvector lazily in the wildcard arm; success paths stay cheap.Run this to ensure all Raw* deserialisation helpers are strict:
#!/bin/bash set -euo pipefail # List Raw* structs without preceding #[serde(deny_unknown_fields)] rg -nP --type=rust -C1 '^\s*struct\s+Raw\w+' \ | awk ' BEGIN{pending=0;file="";line=0} /--/ {next} { if ($0 ~ /:\d+:/) { file=$1; sub(/:.*/,"",file) } if ($0 ~ /struct\s+Raw/) { if (prev !~ /serde\(deny_unknown_fields\)/) { print "Possible missing deny_unknown_fields near:", $0 } } prev=$0 }'src/stdlib/path/hash_utils.rs (1)
77-80: Fix the fallback to hash only valid bytes.The fallback
buffer.as_slice()hashes the entire 8192-byte buffer even when only the firstreadbytes contain valid data. This produces an incorrect hash in the fallback path.Apply this diff to hash only the valid bytes:
- let chunk = buffer.get(..read).unwrap_or_else(|| { - debug_assert!(false, "read beyond buffer capacity: {read} bytes"); - buffer.as_slice() - }); + let chunk = buffer.get(..read).unwrap_or_else(|| { + debug_assert!(false, "read beyond buffer capacity: {read} bytes"); + let end = read.min(buffer.len()); + &buffer[..end] + });🤖 Prompt for AI Agents
In src/stdlib/path/hash_utils.rs around lines 77 to 80, fix the fallback branch in the unwrap_or_else closure: replace buffer.as_slice() with a slice that contains only valid bytes by computing end = read.min(buffer.len()) and returning &buffer[..end] so that when read exceeds the buffer capacity the hash is computed over only the bytes that were actually read rather than the entire 8192-byte buffer.src/manifest/jinja_macros.rs (1)
339-346: Trim the panic wrapper for NonNull.Use .expect(...) to shorten the unwrap_or_else panic path. This matches prior feedback.
Apply:
- let ptr_non_null = NonNull::new(ptr).unwrap_or_else(|| { - panic!("Box::into_raw cannot return a null pointer"); - }); - Self { ptr: ptr_non_null } + let ptr = NonNull::new(ptr).expect("Box::into_raw cannot return a null pointer"); + Self { ptr }tests/steps/ninja_steps.rs (1)
45-51: Resolved: switched to narrowly scoped #[expect].The prior file‑wide #[allow] is gone; per‑fn #[expect] aligns with policy.
tests/steps/cli_steps.rs (1)
1-11: Resolved: file‑wide #[allow] removed; per‑fn #[expect] in use.Matches project lint policy.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (32)
src/ast.rs(2 hunks)src/ir/cmd_interpolate.rs(1 hunks)src/manifest/glob.rs(7 hunks)src/manifest/jinja_macros.rs(6 hunks)src/manifest/render.rs(2 hunks)src/ninja_gen.rs(6 hunks)src/runner/process/mod.rs(1 hunks)src/stdlib/command.rs(7 hunks)src/stdlib/network.rs(4 hunks)src/stdlib/path/hash_utils.rs(2 hunks)src/stdlib/time/tests.rs(6 hunks)test_support/src/http.rs(3 hunks)test_support/src/lib.rs(7 hunks)test_support/src/manifest.rs(1 hunks)test_support/src/ninja.rs(1 hunks)tests/command_escaping_tests.rs(1 hunks)tests/cucumber.rs(6 hunks)tests/manifest_env_tests.rs(3 hunks)tests/manifest_glob_tests.rs(7 hunks)tests/manifest_jinja_tests.rs(11 hunks)tests/ninja_gen_tests.rs(9 hunks)tests/ninja_snapshot_tests.rs(4 hunks)tests/runner_tests.rs(10 hunks)tests/std_filter_tests/io_filters.rs(1 hunks)tests/std_filter_tests/network_functions.rs(1 hunks)tests/steps/cli_steps.rs(4 hunks)tests/steps/fs_steps.rs(4 hunks)tests/steps/ir_steps.rs(3 hunks)tests/steps/manifest_steps.rs(4 hunks)tests/steps/ninja_steps.rs(2 hunks)tests/steps/process_steps.rs(7 hunks)tests/steps/stdlib_steps.rs(7 hunks)
🧰 Additional context used
📓 Path-based instructions (5)
**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
**/*.rs: Every Rust module must begin with a module-level comment using //! explaining the module's purpose and utility
Document public APIs with Rustdoc comments (///) so documentation can be generated with cargo doc
Prefer immutable data; avoid unnecessary mut bindings
Handle errors with Result instead of panicking where feasible
Prefer semantic error enums deriving std::error::Error via thiserror for inspectable conditions
Clippy warnings must be denied; do not silence lints except as a last resort
Lint suppressions must be tightly scoped and include a clear reason; prefer #[expect(...)] over #[allow(...)]
Place function attributes after doc comments
Do not use return in single-line functions
Prefer single-line function definitions where appropriate (e.g., pub fn new(id: u64) -> Self { Self(id) })
Use predicate functions for conditional criteria with more than two branches
Where a function is too long, extract meaningfully named helpers adhering to separation of concerns and CQRS
Where a function has too many parameters, group related parameters into meaningfully named structs
When returning large error data, consider using Arc to reduce data moved
Prefer .expect() over .unwrap() on Results/Options
Use concat!() to combine long string literals instead of escaping newlines with backslashes
Use NewTypes to model domain values; prefer explicit tuple structs; leverage newt-hype and the-newtype crates as described; provide appropriate From/AsRef/TryFrom implementations and avoid orphan-rule-violating impls
Keep each Rust code file under 400 lines; split long switches/dispatch tables by feature; move large test data to external files
Use functions and composition to avoid repetition; extract reusable logic; prefer declarative constructs (iterators/generators/comprehensions) when readable
Small, single-responsibility functions; obey command/query segregation
Name things precisely; booleans should use is/has/should prefixes
Comment why, not what; explain assumptions, e...
Files:
src/stdlib/path/hash_utils.rssrc/manifest/render.rssrc/ir/cmd_interpolate.rstests/steps/ir_steps.rstest_support/src/manifest.rstests/std_filter_tests/network_functions.rstests/std_filter_tests/io_filters.rssrc/runner/process/mod.rstest_support/src/lib.rstests/ninja_snapshot_tests.rstests/manifest_jinja_tests.rssrc/ast.rssrc/stdlib/time/tests.rstests/manifest_env_tests.rssrc/manifest/glob.rstests/steps/ninja_steps.rstests/steps/fs_steps.rstest_support/src/ninja.rstests/ninja_gen_tests.rssrc/stdlib/command.rstests/steps/process_steps.rstests/command_escaping_tests.rstests/cucumber.rssrc/ninja_gen.rstests/manifest_glob_tests.rstest_support/src/http.rssrc/manifest/jinja_macros.rstests/steps/cli_steps.rstests/runner_tests.rssrc/stdlib/network.rstests/steps/stdlib_steps.rstests/steps/manifest_steps.rs
⚙️ CodeRabbit configuration file
**/*.rs: * Seek to keep the cyclomatic complexity of functions no more than 12.
Adhere to single responsibility and CQRS
Place function attributes after doc comments.
Do not use
returnin single-line functions.Move conditionals with >2 branches into a predicate function.
Avoid
unsafeunless absolutely necessary.Every module must begin with a
//!doc comment that explains the module's purpose and utility.Comments and docs must follow en-GB-oxendict (-ize / -yse / -our) spelling and grammar
Lints must not be silenced except as a last resort.
#[allow]is forbidden.- Only narrowly scoped
#[expect(lint, reason = "...")]is allowed.- No lint groups, no blanket or file-wide suppression.
- Include
FIXME:with link if a fix is expected.Where code is only used by specific features, it must be conditionally compiled or a conditional expectation for unused_code applied.
Use
rstestfixtures for shared setup and to avoid repetition between tests.Replace duplicated tests with
#[rstest(...)]parameterised cases.Prefer
mockallfor mocks/stubs.Prefer
.expect()over.unwrap()Ensure that any API or behavioural changes are reflected in the documentation in
docs/Ensure that any completed roadmap steps are recorded in the appropriate roadmap in
docs/Files must not exceed 400 lines in length
- Large modules must be decomposed
- Long match statements or dispatch tables should be decomposed by domain and collocated with targets
- Large blocks of inline data (e.g., test fixtures, constants or templates) must be moved to external files and inlined at compile-time or loaded at run-time.
Environment access (env::set_var and env::remove_var) are always unsafe in Rust 2024 and MUST be marked as such
- For testing of functionality depending upon environment variables, dependency injection and the
mockablecrate are the preferred option.- If mockable cannot be used, env mutations in...
Files:
src/stdlib/path/hash_utils.rssrc/manifest/render.rssrc/ir/cmd_interpolate.rstests/steps/ir_steps.rstest_support/src/manifest.rstests/std_filter_tests/network_functions.rstests/std_filter_tests/io_filters.rssrc/runner/process/mod.rstest_support/src/lib.rstests/ninja_snapshot_tests.rstests/manifest_jinja_tests.rssrc/ast.rssrc/stdlib/time/tests.rstests/manifest_env_tests.rssrc/manifest/glob.rstests/steps/ninja_steps.rstests/steps/fs_steps.rstest_support/src/ninja.rstests/ninja_gen_tests.rssrc/stdlib/command.rstests/steps/process_steps.rstests/command_escaping_tests.rstests/cucumber.rssrc/ninja_gen.rstests/manifest_glob_tests.rstest_support/src/http.rssrc/manifest/jinja_macros.rstests/steps/cli_steps.rstests/runner_tests.rssrc/stdlib/network.rstests/steps/stdlib_steps.rstests/steps/manifest_steps.rs
**/*.{rs,md}
📄 CodeRabbit inference engine (AGENTS.md)
Use en-GB-oxendict spelling and grammar in comments and documentation (exceptions allowed for external API names)
Files:
src/stdlib/path/hash_utils.rssrc/manifest/render.rssrc/ir/cmd_interpolate.rstests/steps/ir_steps.rstest_support/src/manifest.rstests/std_filter_tests/network_functions.rstests/std_filter_tests/io_filters.rssrc/runner/process/mod.rstest_support/src/lib.rstests/ninja_snapshot_tests.rstests/manifest_jinja_tests.rssrc/ast.rssrc/stdlib/time/tests.rstests/manifest_env_tests.rssrc/manifest/glob.rstests/steps/ninja_steps.rstests/steps/fs_steps.rstest_support/src/ninja.rstests/ninja_gen_tests.rssrc/stdlib/command.rstests/steps/process_steps.rstests/command_escaping_tests.rstests/cucumber.rssrc/ninja_gen.rstests/manifest_glob_tests.rstest_support/src/http.rssrc/manifest/jinja_macros.rstests/steps/cli_steps.rstests/runner_tests.rssrc/stdlib/network.rstests/steps/stdlib_steps.rstests/steps/manifest_steps.rs
**/tests/**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
**/tests/**/*.rs: Use rstest fixtures for shared setup
Replace duplicated tests with #[rstest(...)] parameterised cases
Prefer mockall for mocks and stubs
Files:
tests/steps/ir_steps.rstests/std_filter_tests/network_functions.rstests/std_filter_tests/io_filters.rstests/ninja_snapshot_tests.rstests/manifest_jinja_tests.rstests/manifest_env_tests.rstests/steps/ninja_steps.rstests/steps/fs_steps.rstests/ninja_gen_tests.rstests/steps/process_steps.rstests/command_escaping_tests.rstests/cucumber.rstests/manifest_glob_tests.rstests/steps/cli_steps.rstests/runner_tests.rstests/steps/stdlib_steps.rstests/steps/manifest_steps.rs
**/src/lib.rs
📄 CodeRabbit inference engine (AGENTS.md)
Never export the opaque eyre::Report type from libraries; expose semantic domain error enums instead
Files:
test_support/src/lib.rs
**/*{test,tests}.rs
📄 CodeRabbit inference engine (AGENTS.md)
Write unit and behavioural tests for new functionality and run them before and after changes
Files:
tests/ninja_snapshot_tests.rstests/manifest_jinja_tests.rssrc/stdlib/time/tests.rstests/manifest_env_tests.rstests/ninja_gen_tests.rstests/command_escaping_tests.rstests/manifest_glob_tests.rstests/runner_tests.rs
🧬 Code graph analysis (19)
src/stdlib/path/hash_utils.rs (1)
src/runner/process/mod.rs (1)
read(201-205)
tests/std_filter_tests/network_functions.rs (4)
test_support/src/http.rs (1)
spawn_http_server(58-72)tests/std_filter_tests/support.rs (3)
stdlib_env_with_state(46-48)register_template(26-36)stdlib_env_with_config(38-44)src/stdlib/mod.rs (2)
is_impure(163-165)new(78-88)test_support/src/hash.rs (1)
sha256_hex(18-26)
tests/std_filter_tests/io_filters.rs (1)
tests/std_filter_tests/support.rs (4)
filter_workspace(54-70)stdlib_env(50-52)render(72-86)register_template(26-36)
src/runner/process/mod.rs (3)
src/runner/process/redaction.rs (6)
redact_sensitive_args(101-103)contains_sensitive_keyword(42-46)is_sensitive_arg(58-60)redact_argument(76-85)new(8-10)as_str(12-14)src/runner/process/file_io.rs (3)
create_temp_ninja_file(13-29)write_ninja_file(75-82)write_ninja_file_utf8(31-50)src/runner/process/paths.rs (1)
canonicalize_utf8_path(9-30)
tests/ninja_snapshot_tests.rs (4)
test_support/src/lib.rs (1)
ensure_binaries_available(120-140)src/ninja_gen.rs (1)
generate(76-80)src/manifest/mod.rs (1)
from_str(146-148)src/ir/from_manifest.rs (1)
from_manifest(24-35)
tests/manifest_jinja_tests.rs (4)
tests/steps/manifest_steps.rs (1)
manifest(383-387)test_support/src/manifest.rs (1)
manifest_yaml(4-6)src/manifest/mod.rs (1)
from_str(146-148)test_support/src/env_var_guard.rs (2)
set(40-48)remove(57-65)
src/stdlib/time/tests.rs (2)
src/manifest/expand.rs (1)
eval_expression(110-115)src/stdlib/time/mod.rs (3)
now(38-49)new(307-309)new(344-346)
tests/manifest_env_tests.rs (3)
test_support/src/manifest.rs (1)
manifest_yaml(4-6)test_support/src/env.rs (2)
set(133-138)unset(141-146)src/manifest/mod.rs (1)
from_str(146-148)
tests/steps/ninja_steps.rs (2)
src/runner/mod.rs (1)
generate_ninja(172-183)tests/steps/ir_steps.rs (1)
graph(8-13)
tests/ninja_gen_tests.rs (2)
test_support/src/ninja.rs (1)
ninja_integration_workspace(57-60)src/ninja_gen.rs (2)
generate(76-80)generate_into(113-152)
tests/steps/process_steps.rs (3)
test_support/src/env.rs (2)
prepend_dir_to_path(176-190)mocked_path_env(49-56)test_support/src/lib.rs (2)
fake_ninja(69-101)ensure_manifest_exists(168-187)test_support/src/check_ninja.rs (1)
fake_ninja_check_build_file(13-59)
tests/command_escaping_tests.rs (3)
test_support/src/manifest.rs (1)
manifest_yaml(4-6)src/manifest/mod.rs (1)
from_str(146-148)src/ir/from_manifest.rs (1)
from_manifest(24-35)
tests/cucumber.rs (2)
test_support/src/env.rs (1)
restore_many(99-113)test_support/src/http.rs (1)
spawn_http_server(58-72)
tests/manifest_glob_tests.rs (4)
tests/steps/manifest_steps.rs (1)
manifest(383-387)test_support/src/error.rs (2)
std(20-25)display_error_chain(17-27)test_support/src/manifest.rs (1)
manifest_yaml(4-6)src/manifest/mod.rs (1)
from_str(146-148)
tests/steps/cli_steps.rs (1)
tests/cli_tests.rs (1)
parse_cli(40-63)
tests/runner_tests.rs (5)
test_support/src/check_ninja.rs (1)
fake_ninja_check_build_file(13-59)src/runner/mod.rs (4)
new(32-34)new(55-57)default(75-77)run(85-106)test_support/src/path_guard.rs (1)
new(27-31)test_support/src/env.rs (2)
prepend_dir_to_path(176-190)override_ninja_env(220-228)test_support/src/lib.rs (1)
fake_ninja(69-101)
src/stdlib/network.rs (1)
src/stdlib/mod.rs (1)
new(78-88)
tests/steps/stdlib_steps.rs (4)
tests/std_filter_tests/command_filters.rs (4)
env(43-45)new(22-41)command(47-49)state(51-53)src/stdlib/mod.rs (2)
new(78-88)is_impure(163-165)test_support/src/command_helper.rs (2)
compile_uppercase_helper(48-50)compile_failure_helper(70-72)test_support/src/hash.rs (1)
sha256_hex(18-26)
tests/steps/manifest_steps.rs (2)
test_support/src/error.rs (2)
std(20-25)display_error_chain(17-27)test_support/src/env.rs (5)
remove_var(74-80)set_var(29-29)set_var(33-35)set_var(39-41)set_var(62-68)
⏰ Context from checks skipped due to timeout of 120000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: release / build-windows (aarch64-pc-windows-msvc, arm64, arm64, windows-aarch64) / Build windows artefacts
- GitHub Check: release / build-macos (x86_64-apple-darwin, macos-13, macos-x86_64, macos-x86_64) / Build macos artefacts
- GitHub Check: release / build-linux (aarch64-unknown-linux-gnu, linux-arm64, arm64) / Build linux artefacts
- GitHub Check: release / build-windows (x86_64-pc-windows-msvc, amd64, x64, windows-x86_64) / Build windows artefacts
- GitHub Check: build-test (ubuntu-latest, stable)
🔇 Additional comments (65)
tests/steps/stdlib_steps.rs (1)
356-391: Acknowledge complexity threshold — current implementation is acceptable.This function has a cyclomatic complexity of 9, which sits at the policy threshold. However, the logic is clear, well-structured, and handles multiple offset formats (UTC literal, signed hours, optional minutes/seconds) with proper error context. The early returns, match expression, and
map_orpatterns maintain readability.The current implementation strikes a good balance between functionality and clarity. Retain as-is unless future requirements necessitate additional complexity, at which point extract format-specific parsing into predicate helpers.
tests/std_filter_tests/io_filters.rs (4)
1-47: Excellent fallible test implementation.The module documentation, imports, and test function demonstrate exemplary adherence to the coding guidelines. The test correctly uses
Result<()>, fallible helpers from the support module,.context()for error enrichment, andensure!for assertions. Error handling is comprehensive and all paths properly propagate errors rather than panicking.
49-78: Excellent error path verification.The test properly verifies the error scenario using
matchto capture the error,bail!for unexpected success, andensure!to validate both the error kind and message content. This is the correct pattern for error path testing.
80-90: Clean implementation.The test is concise and follows all guidelines correctly, using fallible helpers, proper error contexts, and
ensure!for the assertion.
92-118: Proper error path verification.The test correctly verifies the missing file scenario using the same solid pattern:
matchwithbail!for unexpected success, andensure!for error kind and message validation.tests/std_filter_tests/network_functions.rs (4)
16-46: Excellent error handling and graceful test skipping.The test properly addresses the past review comment by using
bail!instead ofpanic!at line 26. The graceful handling ofPermissionDenied(lines 20-25) with structured logging is particularly well done, allowing the test to skip cleanly when HTTP listener binding fails due to permissions. The thread join error mapping (lines 42-44) correctly converts panics into anyhow errors.
48-102: Well-structured cache behaviour test.The test properly addresses the past review comment by using
bail!instead ofpanic!at line 61. The UTF-8 path validation with.map_err()(lines 51-52) is particularly good practice. The comprehensive verification of cache behaviour (initial fetch, server shutdown, cached response, and file existence) demonstrates thorough testing. Error handling is excellent throughout.
104-132: Thorough error path verification.The test properly verifies the error scenario with the correct pattern:
matchwithbail!for unexpected success, andensure!for error kind and message validation. The verification that failed fetch operations still mark the template impure (lines 127-130) is a valuable test case.
134-161: Excellent early rejection verification.The test properly verifies that invalid
cache_dirarguments are rejected before any network request is issued. The check that the template remains pure (lines 156-159) is particularly valuable, confirming that rejection occurs before side effects.tests/steps/fs_steps.rs (1)
1-145: Approve this well-executed refactoring from panics to Result-based error handling.The migration is comprehensive and idiomatic:
- All fallible operations now use
context,bail, orensurewith clear error messages.- UTF-8 validation uses
map_errwith informative diagnostics.- Helper functions compose cleanly via
?operator.- Both
FileTypeExtimports (lines 6, 9) are necessary: the std trait enables methods onstd::fs::FileType(line 53), whilst the cap_std trait (aliased to avoid collision) enables methods oncap_std::fs::FileType(lines 79-80).- Module-level doc comment, appropriate function sizing, and no complexity concerns.
The code builds correctly under the stricter lint policy and eliminates runtime panics whilst preserving test fixture behaviour.
src/stdlib/network.rs (1)
188-193: LGTM! Infallibility invariant now properly enforced.The change from
debug_assert!(false, ...)to.expect("...")with a narrowly scoped#[expect]attribute correctly addresses the past review feedback. The implementation now fails fast with a clear message if the format operation fails, documenting that writing hex to aStringis infallible in practice.tests/steps/process_steps.rs (1)
21-32: Excellent migration to Result-based error handling.The conversion of all step functions from
unwrap/expecttoResult<()>with rich context messages is well-executed. The consistent use ofanyhow::Context,ensure!, and descriptive error messages significantly improves debuggability whilst eliminating panic paths.Also applies to: 36-89, 101-186
src/manifest/glob.rs (4)
59-60: Good micro-optimisation with to_owned()Switching default-detail construction to to_owned() is fine and marginally leaner. No behaviour change.
Also applies to: 66-67, 96-98
394-396: Doc/behaviour mismatch resolved (fallback clarified)The panic section now documents the non-panicking fallback with a debug assertion. This addresses the prior review thread.
475-494: Tests: adopt Result and precise assertionsSwitching to Result-returning tests with ensure! and explicit ErrorKind checks improves diagnostics and aligns with lint policy.
498-521: Tests: validate non‑UTF‑8 path error kindThe test correctly asserts InvalidOperation for non‑UTF‑8 paths and adds useful context.
tests/manifest_glob_tests.rs (3)
33-42: Target-name extraction now fallibleReturning Result and rejecting non‑String names is appropriate for these tests. Keep as-is.
44-54: Fixtures hardened with Context-rich I/OUsing Result with with_context() in helpers improves failure diagnostics. Good.
Also applies to: 56-63
67-73: Scoped lint expectation for expect() in fixtureThe narrowly scoped #[expect(clippy::expect_used, …)] satisfies the lint gate whilst keeping the code readable.
tests/ninja_snapshot_tests.rs (5)
8-8: LGTM on anyhow import set.Import
ensureandContextto standardize fallible test paths.
13-13: LGTM on shared binary guard.Reuse
test_support::ensure_binaries_availableto centralize tool checks. As per coding guidelines.
27-36: Good skip-path for missing tools.Guard both
ninjaandpython3, log a single joined warning, and exit early withOk(()). Matches earlier review direction.
48-51: LGTM on fallible pipeline.Propagate errors from manifest parsing → IR generation → Ninja emission.
82-87: LGTM on idempotency assertion.Assert the second pass yields “no work to do”; message includes captured output for context.
src/stdlib/time/tests.rs (2)
1-5: Good: module‑level docs added.Meet the crate’s rule for
//!at module start; scope and behaviours are clear.
47-57: Tests read well and match intent.
- Correctly validate UTC default, custom offsets, error kinds, accumulation, negatives, overflow, and ISO‑8601 string forms.
- Result‑returning tests with
ensure!keep failures descriptive.Also applies to: 60-67, 76-87, 90-96, 99-115, 132-141, 144-154, 173-181, 197-202
test_support/src/http.rs (2)
32-35: Idempotent join is correct.Make
HttpServer::joinsafe after prior joins; returningOk(())onNonematches the Drop semantics.
58-66: All call sites correctly propagate errors — no action needed.The verification confirms that the API change to
io::Result<(String, HttpServer)>has been fully propagated. All call sites handle the Result type:
tests/cucumber.rs:80uses?operator for error propagationtests/std_filter_tests/network_functions.rsandsrc/manifest/tests.rsuse explicitmatchpatterns with properOk/ErrhandlingNo stale tuple-destructuring or unchecked calls remain.
src/ast.rs (1)
51-60: Well‑scoped, documented MacroDefinition.Derives and
deny_unknown_fieldsare appropriate for public API stability.tests/cucumber.rs (2)
71-84: Return Result from start_http_server — good.Plumb failure with
Context, replace implicit panics, and store state atomically.
175-180: Guard Cucumber start on TCP bind — good.Skip gracefully when sockets are unavailable; use
tracing::warn!instead of stderr.src/stdlib/path/hash_utils.rs (1)
90-92: LGTM—defensive guard is appropriate.The
debug_assertcatches the impossible case where formatting toStringfails, whilst the release build gracefully ignores the error. This is sound defensive coding for an infallible operation.tests/steps/ir_steps.rs (1)
8-126: Excellent Result-based error handling migration.The introduction of
graph()andgraph_mut()helper functions consolidates error handling and provides clear contextual messages. Step functions consistently returnResult<()>and useensure!for assertions, eliminating panics whilst maintaining readable test code.tests/ninja_gen_tests.rs (1)
8-411: Comprehensive Result-based test migration completed well.All test functions consistently return
Result<()>and useensure!for assertions with descriptive error messages. Theninja_integration_setupfixture gracefully handles unavailable Ninja, and error propagation via?maintains clean test code whilst improving debuggability.src/ninja_gen.rs (2)
190-253: Well-factored helper methods improve maintainability.The decomposition of
NamedActionformatting intowrite_recipe,write_script_command,write_metadata, and helper guards (assert_shell_command,reject_rule_recipe) improves readability and separation of concerns. The debug-only validation inassert_shell_commandand the cfg-gated panic inreject_rule_recipeappropriately handle edge cases.
288-368: Test improvements enhance coverage and robustness.The migration to Result-based tests with
ensure!assertions improves debuggability. The newgenerate_script_ninja_round_tripstest specifically exercises adversarial script inputs (quotes, dollars, backticks, newlines), addressing the previous review feedback about edge-case coverage.src/runner/process/mod.rs (3)
139-152: Excellent helper extraction reduces duplication.The
handle_forwarding_thread_resulthelper consolidates the thread-join handling logic, eliminating duplication and improving maintainability. The differentiated logging (debugfor closed pipes,warnfor panics) provides appropriate operational visibility.
154-177: spawn_and_stream_output refactoring successfully addresses complexity concerns.The extraction of
handle_forwarding_thread_resultandterminate_childhelpers has reduced cyclomatic complexity and eliminated nested conditional blocks previously flagged as "Bumpy Road Ahead". Thelet Some(...) elseguards provide clear early returns with proper cleanup, and the locked stdout/stderr prevent interleaved output.
233-278: Correct byte tracking with CountingReader/CountingWriter.The implementation properly tracks
bytes_readviaCountingReader, ensuring the count includes all bytes consumed from the child (even after write failure). Draining toio::sink()after write errors prevents child deadlock, addressing critical past review feedback.tests/runner_tests.rs (1)
3-269: Comprehensive Result-based test migration improves maintainability.The introduction of the
create_test_manifesthelper and migration of all fixtures and tests to returnResultprovides explicit error handling with contextual messages. Tests usebail!andensure!for clear assertions, and error chain validation ensures proper error propagation throughout the runner.test_support/src/manifest.rs (1)
1-6: Simple, useful test helper for standardising manifest fixtures.The
manifest_yamlhelper eliminates duplication by providing a consistent way to prefix manifest bodies with the version header across test suites. Clear and straightforward implementation.src/ir/cmd_interpolate.rs (1)
28-36: Corrected UTF-8 handling eliminates unnecessary allocation.The updated implementation removes the unnecessary
bytes.clone()and correctly useserr.as_bytes()in the lossy conversion path. Thedebug_assertprovides development-time visibility whilst the fallback ensures graceful handling of unexpected non-UTF-8 bytes from shell quoting.src/manifest/render.rs (3)
156-186: Good: tightly scoped panic allowances in test helpers.Scope is correct, lifetimes simplified, and reasons are explicit. This complies with the “no blanket allow” guideline.
188-201: Good: consolidated target assertions reduce duplication.Helper-based assertions improve clarity and keep failures precise.
219-225: Blanket allow at tests/steps/mod.rs:5 requires verification.The code at lines 219-225 appropriately uses .context() for Option unwraps. Verification uncovered a file-level #![allow(...)] at tests/steps/mod.rs:5, which violates guidelines requiring only narrowly scoped #[expect(lint, reason = "...")] suppressions. Confirm whether this is a PR change or pre-existing, and address as needed.
src/stdlib/command.rs (3)
267-274: LGTM: remove unnecessary clone and use error bytes.Consumes the Vec and uses err.as_bytes(), eliminating an allocation.
347-363: Good: reader joins are non-fatal with structured warnings.Do not propagate join failures during teardown; tracing keeps visibility without flakiness.
473-486: Good: timeout path cleans up and warns on reap failures.Kill on timeout, warn if wait fails, and return a semantic Timeout error.
tests/manifest_jinja_tests.rs (3)
64-90: Good: assert helpers for StringOrList reduce repetition.The helpers centralise shape checks and messages, improving maintainability.
151-167: Good: serialised env mutation guarded by EnvLock/EnvVarGuard.This honours the env-mutation guideline for tests.
403-431: Good: foreach expansion assertions read cleanly.Extractors + ensure! keep failures targeted and reduce duplication.
tests/manifest_env_tests.rs (2)
19-41: Good: parameterised env tests now fallible.Return Result with ensure!/Context for clearer failures; serial gate prevents races.
82-95: Good: explicit non‑UTF‑8 path tested under Unix.Asserts on error text without panicking.
src/manifest/jinja_macros.rs (1)
190-247: Good: macro wrapper enforces keyword handling and caller safety.Validate 'caller' and preserve auto-escape semantics; debug-assert thread affinity.
test_support/src/lib.rs (2)
69-101: Good: fake_ninja now fallible and cross‑platform.Contextual errors and correct script semantics on both Unix and Windows.
332-405: Good: path/IO tests verify error messages include paths.Assert on io::ErrorKind, include UTF‑8 conversions safely, and check content was written.
tests/command_escaping_tests.rs (2)
24-37: Good: command_words now fallible with precise context.Avoid panic paths and attach source locations for triage.
110-128: Good: negative cases assert semantic error text.bail early on unexpected success and check message clarity.
tests/steps/ninja_steps.rs (4)
15-27: LGTM: clear, contextual assertion helper.Good reuse of Option context and ensure; messages are precise.
29-43: LGTM: fallible generation path with stable state updates.Good separation of success/error branches and explicit Ok(()) tail.
91-94: Confirm “first command” maps to index 2.The step name implies the first command, yet this delegates to index 2. Verify that the intended command line is indeed the second “command = …” occurrence in generated Ninja; otherwise switch to 1.
110-124: LGTM: robust error/id correlation.Good use of Context and ensure to assert mention of the removed action id.
tests/steps/cli_steps.rs (1)
12-32: LGTM: default Build fallback with clean state handling.Good defensive parsing and state reset; keeps scenarios deterministic.
tests/steps/manifest_steps.rs (2)
76-104: LGTM: helper extraction removes duplication and improves messages.assert_manifest_collection_len, assert_field_eq, assert_target_* and assert_list_contains centralize patterns and yield clearer failures.
Also applies to: 320-332, 346-365, 397-432, 434-444
135-151: LGTM: environment mutation guarded and validated.Input validation plus lock‑backed set_var/remove_var comply with the project’s env mutation policy for tests.
Also applies to: 154-163
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 24
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
tests/steps/manifest_steps.rs (1)
1-535: Decompose file to stay under 400-line limit.This file has 535 lines; project policy caps modules at 400. Split into separate modules: move helpers (
get_string_from_string_or_list,manifest,assert_manifest_collection_len,assert_field_eq,get_target,assert_target_name,assert_target_command,assert_target_index,assert_list_contains) to a siblingmanifest_steps/helpers.rs, moveparse_env_token,expand_env,parse_manifest_innertomanifest_steps/parsing.rs, keep step functions inmanifest_steps/mod.rs, and re-export as needed.As per coding guidelines.
🤖 Prompt for AI Agents
In tests/steps/manifest_steps.rs (535 lines, exceeding the 400-line limit), decompose into a module directory: 1. Create tests/steps/manifest_steps/ directory 2. Create tests/steps/manifest_steps/helpers.rs containing: - get_string_from_string_or_list - manifest - assert_manifest_collection_len - assert_field_eq - get_target - assert_target_name - assert_target_command - assert_target_index - assert_list_contains 3. Create tests/steps/manifest_steps/parsing.rs containing: - parse_env_token - expand_env - parse_manifest_inner 4. Create tests/steps/manifest_steps/mod.rs containing: - All cucumber step functions (#[given], #[when], #[then]) - INDEX_KEY constant - use super::CliWorld - pub(super) use helpers::*; - pub(super) use parsing::*; 5. Update tests/steps/mod.rs to reference manifest_steps as a directory module 6. Ensure each new file has appropriate //! module doc comments explaining its purpose 7. Preserve all function signatures, attributes, and behaviour 8. Verify each file is under 400 lines and cargo test passes Use en-GB-oxendict spelling in comments.src/manifest/tests.rs (1)
9-12: Fix missing import for UndefinedBehavior (compile error).Import the enum to use UndefinedBehavior::Strict.
-use minijinja::{ - Environment, - value::{Kwargs, Value}, -}; +use minijinja::{ + Environment, + UndefinedBehavior, + value::{Kwargs, Value}, +};Also applies to: 52-56
♻️ Duplicate comments (4)
src/manifest/jinja_macros.rs (1)
350-354: Simplify pointer creation per past review and coding guidelines.The
unwrap_or_else(|| panic!(...))pattern is more verbose than necessary. Past review comments suggested using.expect()for conciseness. Box::into_raw cannot return null, so the error path is unreachable.Apply this diff to follow the coding guidelines ("Prefer
.expect()over.unwrap()"):fn new(state: State<'static, 'static>) -> Self { let boxed = Box::new(state); let ptr = Box::into_raw(boxed); - let ptr_non_null = NonNull::new(ptr).unwrap_or_else(|| { - panic!("Box::into_raw cannot return a null pointer"); - }); + let ptr = NonNull::new(ptr) + .expect("Box::into_raw cannot return a null pointer"); Self { ptr: ptr_non_null } }🤖 Prompt for AI Agents
In src/manifest/jinja_macros.rs at lines 350-354, replace the verbose `unwrap_or_else(|| panic!(...))` pattern with `.expect("Box::into_raw cannot return a null pointer")` to be more concise per coding guidelines. Update the variable name from `ptr_non_null` to `ptr` and use it in the Self construction.tests/steps/manifest_steps.rs (1)
2-5: Remove file‑wide #[expect]; scope to individual step functions.Project policy forbids blanket suppressions. Add
#[expect(clippy::shadow_reuse, reason = "Cucumber step macro rebinds capture names")]only on the specific step functions where the cucumber macros force shadow violations.As per coding guidelines.
🤖 Prompt for AI Agents
In tests/steps/manifest_steps.rs at lines 2-5, remove the file-wide #![expect(clippy::shadow_reuse, ...)] attribute. Then audit each function decorated with #[given], #[when], or #[then] attributes: compile with clippy and identify which specific functions trigger clippy::shadow_reuse. Add #[expect(clippy::shadow_reuse, reason = "Cucumber step macro rebinds capture names")] immediately before the function signature (after the doc comment if present, before the cucumber attribute) for only those functions. Ensure the reason is concise. Run cargo clippy --all-targets to confirm no file-wide suppression remains and only the necessary functions carry the attribute.src/manifest/glob.rs (1)
398-400: Resolve prior doc/behaviour mismatch.Doc now matches the intended semantics (debug assert in debug builds, graceful fall‑back in release). Good fix.
src/manifest/tests.rs (1)
38-43: Include target directory in warning: LGTM.Message now includes the intended directory, aiding triage.
📜 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 (25)
src/manifest/glob.rs(8 hunks)src/manifest/jinja_macros.rs(9 hunks)src/manifest/tests.rs(7 hunks)src/stdlib/network.rs(4 hunks)src/stdlib/time/tests.rs(5 hunks)test_support/Cargo.toml(1 hunks)test_support/src/lib.rs(7 hunks)test_support/src/ninja.rs(1 hunks)tests/cucumber.rs(6 hunks)tests/manifest_glob_tests.rs(7 hunks)tests/ninja_snapshot_tests.rs(3 hunks)tests/steps/cli_steps.rs(3 hunks)tests/steps/common.rs(1 hunks)tests/steps/ir_steps.rs(3 hunks)tests/steps/manifest_steps.rs(4 hunks)tests/steps/mod.rs(1 hunks)tests/steps/ninja_steps.rs(2 hunks)tests/steps/process_steps.rs(7 hunks)tests/steps/stdlib_steps.rs(0 hunks)tests/steps/stdlib_steps/assertions.rs(1 hunks)tests/steps/stdlib_steps/mod.rs(1 hunks)tests/steps/stdlib_steps/parsing.rs(1 hunks)tests/steps/stdlib_steps/rendering.rs(1 hunks)tests/steps/stdlib_steps/types.rs(1 hunks)tests/steps/stdlib_steps/workspace.rs(1 hunks)
💤 Files with no reviewable changes (1)
- tests/steps/stdlib_steps.rs
🧰 Additional context used
📓 Path-based instructions (5)
**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
**/*.rs: Every Rust module must begin with a module-level comment using //! explaining the module's purpose and utility
Document public APIs with Rustdoc comments (///) so documentation can be generated with cargo doc
Prefer immutable data; avoid unnecessary mut bindings
Handle errors with Result instead of panicking where feasible
Prefer semantic error enums deriving std::error::Error via thiserror for inspectable conditions
Clippy warnings must be denied; do not silence lints except as a last resort
Lint suppressions must be tightly scoped and include a clear reason; prefer #[expect(...)] over #[allow(...)]
Place function attributes after doc comments
Do not use return in single-line functions
Prefer single-line function definitions where appropriate (e.g., pub fn new(id: u64) -> Self { Self(id) })
Use predicate functions for conditional criteria with more than two branches
Where a function is too long, extract meaningfully named helpers adhering to separation of concerns and CQRS
Where a function has too many parameters, group related parameters into meaningfully named structs
When returning large error data, consider using Arc to reduce data moved
Prefer .expect() over .unwrap() on Results/Options
Use concat!() to combine long string literals instead of escaping newlines with backslashes
Use NewTypes to model domain values; prefer explicit tuple structs; leverage newt-hype and the-newtype crates as described; provide appropriate From/AsRef/TryFrom implementations and avoid orphan-rule-violating impls
Keep each Rust code file under 400 lines; split long switches/dispatch tables by feature; move large test data to external files
Use functions and composition to avoid repetition; extract reusable logic; prefer declarative constructs (iterators/generators/comprehensions) when readable
Small, single-responsibility functions; obey command/query segregation
Name things precisely; booleans should use is/has/should prefixes
Comment why, not what; explain assumptions, e...
Files:
tests/steps/mod.rstests/ninja_snapshot_tests.rssrc/stdlib/network.rstest_support/src/ninja.rssrc/stdlib/time/tests.rstests/steps/stdlib_steps/rendering.rstests/steps/ninja_steps.rstests/steps/stdlib_steps/parsing.rstests/steps/stdlib_steps/workspace.rstests/steps/stdlib_steps/types.rstests/steps/stdlib_steps/assertions.rstests/steps/stdlib_steps/mod.rstests/steps/process_steps.rstests/steps/common.rstests/cucumber.rstest_support/src/lib.rssrc/manifest/jinja_macros.rstests/manifest_glob_tests.rstests/steps/ir_steps.rstests/steps/cli_steps.rssrc/manifest/glob.rssrc/manifest/tests.rstests/steps/manifest_steps.rs
⚙️ CodeRabbit configuration file
**/*.rs: * Seek to keep the cyclomatic complexity of functions no more than 12.
Adhere to single responsibility and CQRS
Place function attributes after doc comments.
Do not use
returnin single-line functions.Move conditionals with >2 branches into a predicate function.
Avoid
unsafeunless absolutely necessary.Every module must begin with a
//!doc comment that explains the module's purpose and utility.Comments and docs must follow en-GB-oxendict (-ize / -yse / -our) spelling and grammar
Lints must not be silenced except as a last resort.
#[allow]is forbidden.- Only narrowly scoped
#[expect(lint, reason = "...")]is allowed.- No lint groups, no blanket or file-wide suppression.
- Include
FIXME:with link if a fix is expected.Where code is only used by specific features, it must be conditionally compiled or a conditional expectation for unused_code applied.
Use
rstestfixtures for shared setup and to avoid repetition between tests.Replace duplicated tests with
#[rstest(...)]parameterised cases.Prefer
mockallfor mocks/stubs.Prefer
.expect()over.unwrap()Ensure that any API or behavioural changes are reflected in the documentation in
docs/Ensure that any completed roadmap steps are recorded in the appropriate roadmap in
docs/Files must not exceed 400 lines in length
- Large modules must be decomposed
- Long match statements or dispatch tables should be decomposed by domain and collocated with targets
- Large blocks of inline data (e.g., test fixtures, constants or templates) must be moved to external files and inlined at compile-time or loaded at run-time.
Environment access (env::set_var and env::remove_var) are always unsafe in Rust 2024 and MUST be marked as such
- For testing of functionality depending upon environment variables, dependency injection and the
mockablecrate are the preferred option.- If mockable cannot be used, env mutations in...
Files:
tests/steps/mod.rstests/ninja_snapshot_tests.rssrc/stdlib/network.rstest_support/src/ninja.rssrc/stdlib/time/tests.rstests/steps/stdlib_steps/rendering.rstests/steps/ninja_steps.rstests/steps/stdlib_steps/parsing.rstests/steps/stdlib_steps/workspace.rstests/steps/stdlib_steps/types.rstests/steps/stdlib_steps/assertions.rstests/steps/stdlib_steps/mod.rstests/steps/process_steps.rstests/steps/common.rstests/cucumber.rstest_support/src/lib.rssrc/manifest/jinja_macros.rstests/manifest_glob_tests.rstests/steps/ir_steps.rstests/steps/cli_steps.rssrc/manifest/glob.rssrc/manifest/tests.rstests/steps/manifest_steps.rs
**/*.{rs,md}
📄 CodeRabbit inference engine (AGENTS.md)
Use en-GB-oxendict spelling and grammar in comments and documentation (exceptions allowed for external API names)
Files:
tests/steps/mod.rstests/ninja_snapshot_tests.rssrc/stdlib/network.rstest_support/src/ninja.rssrc/stdlib/time/tests.rstests/steps/stdlib_steps/rendering.rstests/steps/ninja_steps.rstests/steps/stdlib_steps/parsing.rstests/steps/stdlib_steps/workspace.rstests/steps/stdlib_steps/types.rstests/steps/stdlib_steps/assertions.rstests/steps/stdlib_steps/mod.rstests/steps/process_steps.rstests/steps/common.rstests/cucumber.rstest_support/src/lib.rssrc/manifest/jinja_macros.rstests/manifest_glob_tests.rstests/steps/ir_steps.rstests/steps/cli_steps.rssrc/manifest/glob.rssrc/manifest/tests.rstests/steps/manifest_steps.rs
**/tests/**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
**/tests/**/*.rs: Use rstest fixtures for shared setup
Replace duplicated tests with #[rstest(...)] parameterised cases
Prefer mockall for mocks and stubs
Files:
tests/steps/mod.rstests/ninja_snapshot_tests.rstests/steps/stdlib_steps/rendering.rstests/steps/ninja_steps.rstests/steps/stdlib_steps/parsing.rstests/steps/stdlib_steps/workspace.rstests/steps/stdlib_steps/types.rstests/steps/stdlib_steps/assertions.rstests/steps/stdlib_steps/mod.rstests/steps/process_steps.rstests/steps/common.rstests/cucumber.rstests/manifest_glob_tests.rstests/steps/ir_steps.rstests/steps/cli_steps.rstests/steps/manifest_steps.rs
**/*{test,tests}.rs
📄 CodeRabbit inference engine (AGENTS.md)
Write unit and behavioural tests for new functionality and run them before and after changes
Files:
tests/ninja_snapshot_tests.rssrc/stdlib/time/tests.rstests/manifest_glob_tests.rssrc/manifest/tests.rs
**/src/lib.rs
📄 CodeRabbit inference engine (AGENTS.md)
Never export the opaque eyre::Report type from libraries; expose semantic domain error enums instead
Files:
test_support/src/lib.rs
🧬 Code graph analysis (16)
tests/ninja_snapshot_tests.rs (4)
test_support/src/lib.rs (1)
ensure_binaries_available(139-159)src/manifest/mod.rs (1)
from_str(146-148)src/ir/from_manifest.rs (1)
from_manifest(24-35)src/ninja_gen.rs (1)
generate(76-80)
src/stdlib/network.rs (1)
src/stdlib/mod.rs (1)
new(78-88)
src/stdlib/time/tests.rs (2)
src/manifest/expand.rs (1)
eval_expression(110-115)src/stdlib/time/mod.rs (4)
new(307-309)new(344-346)register_functions(33-36)now(38-49)
tests/steps/stdlib_steps/rendering.rs (3)
tests/steps/stdlib_steps/workspace.rs (2)
ensure_workspace(16-42)resolve_template_path(125-135)src/stdlib/mod.rs (1)
register_with_config(223-233)tests/steps/stdlib_steps/types.rs (8)
as_path(7-9)as_str(28-30)as_str(58-60)from(13-15)from(19-21)from(34-36)from(49-51)from(68-70)
tests/steps/ninja_steps.rs (2)
tests/steps/common.rs (1)
build_graph_available(10-12)src/runner/mod.rs (1)
generate_ninja(172-183)
tests/steps/stdlib_steps/workspace.rs (2)
test_support/src/command_helper.rs (2)
compile_failure_helper(70-72)compile_uppercase_helper(48-50)tests/steps/stdlib_steps/types.rs (7)
as_str(28-30)as_str(58-60)from(13-15)from(19-21)from(34-36)from(49-51)from(68-70)
tests/steps/stdlib_steps/assertions.rs (4)
tests/steps/stdlib_steps/parsing.rs (2)
parse_expected_offset(20-55)parse_iso_timestamp(15-18)src/stdlib/mod.rs (1)
is_impure(163-165)test_support/src/hash.rs (1)
sha256_hex(18-26)tests/steps/stdlib_steps/types.rs (7)
as_str(28-30)as_str(58-60)from(13-15)from(19-21)from(34-36)from(49-51)from(68-70)
tests/steps/stdlib_steps/mod.rs (2)
tests_python/conftest.py (1)
workspace(60-65)tests/steps/stdlib_steps/parsing.rs (4)
extract_host_from_url(8-13)parse_expected_offset(20-55)parse_iso_timestamp(15-18)server_host(4-6)
tests/steps/process_steps.rs (4)
test_support/src/env.rs (2)
prepend_dir_to_path(176-190)mocked_path_env(49-56)test_support/src/lib.rs (2)
fake_ninja(88-120)ensure_manifest_exists(187-206)test_support/src/check_ninja.rs (1)
fake_ninja_check_build_file(13-59)test_support/src/path_guard.rs (1)
new(27-31)
tests/cucumber.rs (2)
test_support/src/env.rs (1)
restore_many(99-113)test_support/src/http.rs (1)
spawn_http_server(58-72)
src/manifest/jinja_macros.rs (2)
src/stdlib/time/tests.rs (1)
env(22-26)tests/std_filter_tests/command_filters.rs (2)
env(43-45)new(22-41)
tests/manifest_glob_tests.rs (4)
tests/steps/manifest_steps.rs (2)
manifest(363-367)manifest(397-402)test_support/src/error.rs (2)
std(20-25)display_error_chain(17-27)test_support/src/manifest.rs (1)
manifest_yaml(4-6)src/manifest/mod.rs (1)
from_str(146-148)
tests/steps/ir_steps.rs (1)
tests/steps/common.rs (2)
build_graph_generated(14-16)build_graph_generated_mut(18-20)
tests/steps/cli_steps.rs (1)
tests/cli_tests.rs (1)
parse_cli(40-63)
src/manifest/tests.rs (3)
src/manifest/jinja_macros.rs (8)
parse_macro_name(38-57)register_macro(75-94)call_macro_value(149-164)new(268-274)new(308-326)new(348-355)new(412-419)register_manifest_macros(106-122)test_support/src/http.rs (2)
spawn_http_server(58-72)join(30-36)test_support/src/hash.rs (1)
sha256_hex(18-26)
tests/steps/manifest_steps.rs (3)
test_support/src/error.rs (2)
std(20-25)display_error_chain(17-27)test_support/src/env.rs (5)
remove_var(74-80)set_var(29-29)set_var(33-35)set_var(39-41)set_var(62-68)tests/ast_tests.rs (1)
parse_manifest(9-11)
🔍 Remote MCP Deepwiki
Summary — additional facts to help reviewing PR #222
-
Clippy & CI enforcement
- The repo enforces strict clippy rules (many lints denied) and CI runs clippy/format/tests with "deny warnings" and MSRV checks; expect lint-driven, cross-cutting changes and CI failures if any lint remains.
-
Error-handling standardisation
- Widespread adoption of anyhow/context + thiserror for richer context; many production code paths replaced unwrap/expect with Result flows and .with_context() calls (tests often still use expect under explicit allowances).
-
Template & manifest behaviour
- Manifest parsing uses a two-pass approach (expand foreach/when on serde_yml::Value, then deserialize and render strings with minijinja). Custom functions (env(), glob(), stdlib filters) are registered in the environment; template errors are strict/early.
-
IR / Ninja generation & safety
- IR generation deduplicates Actions by content hash, performs cycle detection and duplicate-output checks; commands are shell-quoted (shell-quote) and validated with shlex before Ninja emission — changes that tighten safety and affect command interpolation code paths.
-
Notable public/API surface changes introduced by the PR (review targets)
- New public struct: MacroDefinition (src/ast.rs).
- New public type aliases: ManifestValue and ManifestMap (src/manifest/mod.rs).
- New public field on Action: restat: bool (src/ir/graph.rs) — affects callers constructing Actions.
- Runner API additions: NinjaContent::new/as_str/into_string and BuildTargets::new/as_slice; resolve_manifest_path now returns Result (src/runner/mod.rs) — impacts runner callers.
- test_support crate: added anyhow and thiserror deps, new functions (fake_ninja now returns Result), and new public test helpers/modules — affects tests and fixtures.
-
Tests & test harness changes (impact on reviewers)
- Large-scale migration of tests to Result-based style (anyhow + ensure) across unit, integration, snapshot and cucumber BDD tests (many test function signatures changed to return Result<()>). Search/verify updated test signatures and anywhere test fixtures are consumed.
-
Build / release considerations
- build.rs now produces the man page and uses SOURCE_DATE_EPOCH handling (affects build outputs); release CI builds multi-platform artifacts and the CI workflow enforces formatting/lint/tests before merges.
Relevant docs/source used: Deepwiki repo overview and design pages (read via Deepwiki_read_wiki_contents).
⏰ Context from checks skipped due to timeout of 120000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: release / build-macos (x86_64-apple-darwin, macos-13, macos-x86_64, macos-x86_64) / Build macos artefacts
- GitHub Check: release / build-linux (aarch64-unknown-linux-gnu, linux-arm64, arm64) / Build linux artefacts
- GitHub Check: release / build-windows (aarch64-pc-windows-msvc, arm64, arm64, windows-aarch64) / Build windows artefacts
- GitHub Check: release / build-windows (x86_64-pc-windows-msvc, amd64, x64, windows-x86_64) / Build windows artefacts
- GitHub Check: build-test (ubuntu-latest, stable)
🔇 Additional comments (43)
test_support/Cargo.toml (2)
15-16: Addition of anyhow and thiserror aligns with PR's error-handling refactor.These dependencies support the widespread migration to Result-based error handling with contextual errors across test_support and the broader codebase. The loose version specifications are reasonable for stable, actively-maintained crates with strong semver discipline, and are consistent with other dependencies like
mockable = "0.3".
15-16: Approve dependency additions; MSRV compatibility confirmed.Both anyhow 1.x and thiserror 1.x support Rust 1.89.0 as stable-channel crates. The loose version specification "1" is appropriate for these stable crates and poses no MSRV risk. No changes required.
src/stdlib/network.rs (6)
188-193: LGTM! Excellent infallibility assertion.The hex formatting now correctly documents its infallibility invariant with a narrowly scoped
#[expect]and a clear.expect()message. This addresses the previous review feedback perfectly.
209-228: LGTM! Clean Result-based test fixture.The
CacheWorkspacetype alias and Result-returning fixture align well with the PR's error-handling migration. Contextual error messages are clear and helpful.
237-252: LGTM! Well-designed helper consolidates rejection logic.The
assert_open_cache_dir_rejectshelper successfully eliminates duplicated match/ensure patterns. Clear doc comment and parameterised error messages make it reusable.
303-320: LGTM! Effective helper eliminates verification duplication.The
assert_cache_entry_existshelper consolidates the write/drop/assert pattern cleanly. Taking ownership ofdirto drop it is the correct design choice.
323-328: LGTM! Clean composition of helpers.Successfully combines file setup with the rejection helper, eliminating duplication whilst maintaining clear test intent.
331-336: LGTM! Tests now use the cache entry helper effectively.Both tests leverage
assert_cache_entry_existsto verify filesystem side effects, significantly reducing duplication whilst preserving test clarity.Also applies to: 340-349
src/manifest/jinja_macros.rs (8)
45-56: LGTM: Variable naming clarifies intent.The renaming from
name→name_segment→identifiermakes the extraction and validation steps more explicit.
157-160: LGTM: Parameter renaming improves clarity.The
macro_kwargsname better distinguishes this from the internalKwargsconstruction within the closure.
204-213: LGTM: Optional return and explicit error improve safety.The change from direct access to
ok_or_elseerror handling makes the uninitialised macro case explicit and provides a clear error message.
218-230: LGTM: Consistent parameter renaming throughout the function.The
macro_kwargsiteration and extraction logic correctly preserves the special "caller" handling whilst using the renamed parameter.
279-281: LGTM: Explicit drop clarifies error-path resource handling.The explicit
drop(returned_instance)onset()failure makes the cleanup intent clear, even though the drop would occur automatically.
286-288: LGTM: Optional return encodes initialisation state in the type.Returning
Option<&MacroInstance>forces callers to handle the uninitialised case explicitly, preventing null-dereference-like errors.
318-320: LGTM: Safety comment correctly documents transmute justification.The updated comment clearly links the
unsafetransmute to theEnvironment<'static>requirement inregister_macro, explaining why the lifetime extension is sound.
65-76: All callsites satisfy the 'static lifetime requirement.The documentation and signature change are correctly implemented:
- Test callsites (src/manifest/tests.rs:130, 165, 185, 213, 257, 273) all receive
strict_env: Environment<'static>via rstest fixture injection- Production callsite (src/manifest/mod.rs:126) creates
Environment::new(), which returnsEnvironment<'static>- The
'staticgeneric parameter constrains references stored in the environment, not the environment's own lifetime; macro bytecode persists in the macro instances themselves (per the SAFETY comment at line 318)- No callers require modification
src/stdlib/time/tests.rs (4)
12-19: Good: contextualise parse/eval failures.Keeping the expression text in the error context speeds diagnosis. LGTM.
44-51: Good: assert iso8601 is a string.Avoiding
to_string()on non-strings prevents false positives. LGTM.
146-155: Good: overflow path asserts engine-level error kind.Checking
ErrorKind::InvalidOperationvalidates the template engine contract. LGTM.
174-182: Good: ISO‑8601 property checks for timestamp and duration.Helpers enforce type and formatting invariants. LGTM.
Also applies to: 198-203
tests/manifest_glob_tests.rs (1)
65-71: Good: DRY error‑message assertions and adopt Result‑based tests.
parse_error_msgremoves repeated match blocks whilst preserving messages.- Test entrypoints now return
Result<()>, improving failure context.- File‑scoped
#[allow]was replaced with narrowly scoped#[expect]in the fixture. LGTM.Also applies to: 176-183, 190-199, 206-215, 218-224, 227-233, 243-251, 256-265, 271-278, 281-304, 308-323
tests/steps/mod.rs (1)
6-6: Good: factor shared helpers intocommon.Module docs are present and the surface is tidy. LGTM.
tests/ninja_snapshot_tests.rs (1)
15-25: Good upgrade: propagate failure with status and UTF‑8 validation.The Result-based runner with
ensure!and explicit status improves diagnostics and aligns with the new lint policy.tests/steps/common.rs (1)
10-20: LGTM: centralize BuildGraph access with consistent context.Accessors are concise, reuse helpers, and keep messages uniform.
Also applies to: 22-24, 26-31
tests/steps/stdlib_steps/mod.rs (1)
3-6: LGTM: narrow lint expectations with explicit reasons and re-export surface.Scoped
#[expect(...)]aligns with the “no blanket suppression” rule while keeping Cucumber discovery intact.Also applies to: 14-40
tests/steps/ninja_steps.rs (2)
92-94: Verify intended 1‑based index for “the command”.The helper is 1‑based, but this passes 2. Confirm behaviour and update to 1 if “the command” refers to the first occurrence.
Please confirm whether “the command” should map to index 1. If yes, change: ninja_command_tokens(world, 1, expected_tokens)
62-74: Good: guard the 1‑based index and avoid usize underflow.The precondition and indexed get() are correct and remove the panic risk.
tests/steps/ir_steps.rs (2)
27-35: LGTM: assert via ensure on action count.Result-based checks with contextual messages align with the tightened lint/error-handling policy.
109-117: LGTM: safe action removal with explicit error on empty graph.Clear negative path and world state update look good.
tests/steps/process_steps.rs (1)
25-36: LGTM: install_test_ninja now fallible and scoped PATH guard.Error contexts are clear; guard is stored for teardown.
tests/cucumber.rs (1)
118-127: LGTM: scope the intentional panic in strict teardown.The #[expect(clippy::panic,...)] is correctly placed and documented; join panics are logged instead of crashing.
tests/steps/cli_steps.rs (1)
38-45: LGTM: return slices from extract_build.Exposing &[String] and &Option avoids tying callers to Vec and matches read-only intent.
tests/steps/manifest_steps.rs (3)
77-105: Excellent consolidation of duplicate patterns.These helpers successfully implement the refactoring outlined in the PR objectives, reducing duplication across
manifest_version,manifest_has_targets,manifest_has_macros,first_rule_name,macro_signature_is, andassert_target_command.Optional: The
debug_assert!at lines 83–86 is redundant (callers already validate viamanifest(world)?), but it provides defence in depth and may catch future refactoring errors, so keeping it is reasonable.
19-36: Comprehensive Result-based error handling implementation.The wholesale conversion to
Result<()>withensure!,bail!, and contextual error messages significantly improves test diagnostics. Validation guards (non-empty keys/paths, 1-based index checks) and rich error messages (including expected vs. actual values) are excellent.Also applies to: 141-164, 171-192, 199-206, 213-535
13-13: Remove unnecessaryTryFromimport.
TryFromis in the Rust 2021 prelude; the explicit import is redundant.Apply this diff:
-use std::{collections::BTreeSet, convert::TryFrom, ffi::OsStr}; +use std::{collections::BTreeSet, ffi::OsStr};Likely an incorrect or invalid review comment.
src/manifest/glob.rs (6)
59-73: Error detail construction: LGTM.Use of to_owned() and improved IO detail wiring is clear and allocation‑efficient.
96-98: Clarify non‑UTF‑8 match error: LGTM.Error messaging is precise and routed through existing context plumbing.
142-147: Helper for fall‑back selection: LGTM.Centralize the “normalised‑or‑raw” choice and keep the debug‑only assertion.
150-156: Root dir selection via normalised pattern: LGTM.Use the helper to ensure consistent absolute/relative detection.
411-413: Ownership choice for raw pattern: LGTM.Use to_owned() for clarity and consistency with other allocations.
427-428: Use the helper consistently: LGTM.Avoid ad‑hoc unwrap chains by reusing normalized_or_raw.
src/manifest/tests.rs (2)
64-71: Result‑based tests and clearer assertions: LGTM.Adopt AnyResult, use ensure!/context(), and keep step outputs precise. This aligns with the stricter lint gate and improves failure diagnostics.
Also applies to: 77-90, 124-133, 137-157, 160-175, 178-195, 199-223, 226-245, 249-263, 267-277
288-297: Remove this review comment—HttpServer already implements Drop with proper thread cleanup.The HttpServer struct already implements the Drop trait, which automatically joins the thread upon deallocating. The implementation calls
shutdown_listener()to signal the server to exit, then invokeshandle.join()to await thread completion. This means early returns trigger Rust's RAII semantics and guarantee thread cleanup on all code paths.The suggested ServerJoinGuard wrapper is redundant and adds unnecessary complexity. No refactoring is required.
Likely an incorrect or invalid review comment.
|
@coderabbitai Please suggest a fix for this issue and supply a prompt for an AI coding agent to enable it to apply the fix: Comment on file //! Step definitions for Ninja file generation scenarios.
#
leynos
force-pushed
the
additional-lints
branch
from
8a0f73a to
a1ffb68
Compare
Allow the FileExists integration test to validate command side effects without requiring the explicit Ninja output to match the inspected target. With the relaxed assertion, the phony action scenario generates its expected file and the full RUSTFLAGS="-D warnings" cargo test --all-targets --all-features
running 111 tests
test ir::cycle::tests::canonicalize_cycle_handles_reverse_direction ... ok
test ir::cmd_interpolate::tests::interpolate_command_rejects_unbalanced_backticks ... ok
test ir::cycle::tests::canonicalize_cycle_rotates_smallest_node ... ok
test ir::cmd_interpolate::tests::interpolate_command_replaces_placeholders ... ok
test ir::cmd_interpolate::tests::interpolate_command_preserves_backtick_tokens ... ok
test ir::cycle::tests::cycle_detector_detects_self_edge_cycle ... ok
test ir::cycle::tests::cycle_detector_records_missing_dependencies ... ok
test ir::cycle::tests::find_cycle_identifies_cycle ... ok
test ir::cycle::tests::cycle_detector_marks_nodes_visited_after_traversal ... ok
test manifest::diagnostics::byte_index_tests::byte_index_advances_over_previous_lines ... ok
test manifest::diagnostics::byte_index_tests::byte_index_clamps_past_line_end ... ok
test manifest::diagnostics::byte_index_tests::byte_index_accounts_for_multibyte_characters ... ok
test manifest::diagnostics::byte_index_tests::byte_index_handles_crlf_lines ... ok
test manifest::diagnostics::tests::map_yaml_error_defaults_location_when_missing ... ok
test manifest::diagnostics::tests::location_to_index_handles_utf8 ... ok
test manifest::diagnostics::tests::map_yaml_error_includes_tab_hint ... ok
test manifest::glob::tests::process_glob_entry_rejects_non_utf8_paths ... ok
test manifest::diagnostics::tests::map_yaml_error_span_skips_carriage_return ... ok
test manifest::glob::tests::validate_brace_matching_rejects_unmatched_closing ... ok
test manifest::expand::tests::expand_foreach_expands_sequence_values ... ok
test manifest::expand::tests::expand_foreach_preserves_object_key_order ... ok
test manifest::expand::tests::expand_foreach_applies_when_expression ... ok
test manifest::glob::tests::validate_brace_matching_accepts_balanced_braces ... ok
test manifest::tests::parse_macro_name_errors::case_2 ... ok
test manifest::tests::parse_macro_name_errors::case_3 ... ok
test manifest::tests::parse_macro_name_extracts_identifier::case_1 ... ok
test manifest::tests::parse_macro_name_errors::case_1 ... ok
test manifest::tests::manifest_macros_with_non_string_keys_fail_to_parse ... ok
test manifest::tests::parse_macro_name_extracts_identifier::case_2 ... ok
test manifest::tests::parse_macro_name_extracts_identifier::case_3 ... ok
test manifest::tests::parse_macro_name_extracts_identifier::case_4 ... ok
test manifest::tests::parse_macro_name_extracts_identifier::case_5 ... ok
test manifest::render::tests::render_manifest_renders_targets_and_rules ... ok
test manifest::tests::call_macro_value_supports_kwargs ... ok
test manifest::tests::register_macro_handles_arguments::case_1 ... ok
test manifest::tests::register_macro_handles_arguments::case_2 ... ok
test manifest::tests::register_manifest_macros_rejects_non_string_values ... ok
test manifest::tests::register_manifest_macros_requires_body ... ok
test ninja_gen::tests::assert_shell_command_tolerates_complex_syntax ... ok
test manifest::tests::register_macro_handles_arguments::case_5 ... ok
test ninja_gen::tests::generate_simple_ninja ... ok
test manifest::tests::register_macro_handles_arguments::case_4 ... ok
test manifest::tests::register_macro_handles_arguments::case_3 ... ok
test manifest::tests::register_macro_handles_arguments::case_6 ... ok
test runner::process::file_io::tests::create_temp_ninja_file_supports_reopen ... ok
test runner::process::file_io::tests::write_ninja_file_handles_absolute_paths ... ok
test manifest::tests::register_manifest_macros_validates_shape ... ok
test runner::process::file_io::tests::write_ninja_file_utf8_creates_parent_directories ... ok
test manifest::tests::register_macro_is_reusable ... ok
test ninja_gen::tests::generate_script_ninja_round_trips ... ok
test runner::process::redaction::tests::contains_sensitive_keyword_only_flags_known_keys ... ok
test runner::process::redaction::tests::redact_argument_preserves_non_sensitive_pairs ... ok
test runner::process::tests::forward_output_continues_draining_after_write_failure ... ok
test runner::process::tests::forward_output_writes_all_bytes_when_parent_alive ... ok
test runner::process::tests::resolve_ninja_program_utf8_defaults_without_override ... ok
test manifest::tests::register_manifest_macros_supports_multiple ... ok
test runner::process::tests::resolve_ninja_program_utf8_ignores_invalid_utf8_override ... ok
test runner::process::tests::resolve_ninja_program_utf8_prefers_env_override ... ok
test stdlib::network::tests::cache_key_stable ... ok
test stdlib::network::tests::hex_string_formats_bytes ... ok
test stdlib::collections::tests::flatten_filter_flattens_deeply_nested_sequences ... ok
test stdlib::collections::tests::flatten_filter_rejects_scalars ... ok
test stdlib::network::tests::fetch_rejects_template_cache_dir_argument ... ok
test stdlib::collections::tests::group_by_filter_rejects_empty_attribute ... ok
test stdlib::collections::tests::uniq_filter_removes_duplicates ... ok
test stdlib::network::tests::open_cache_dir_rejects_invalid_paths::case_1 ... ok
test stdlib::collections::tests::group_by_filter_clusters_items_and_preserves_key_types ... ok
test stdlib::network::tests::open_cache_dir_rejects_invalid_paths::case_2 ... ok
test stdlib::network::tests::fetch_cache_opens_default_directory ... ok
test stdlib::network::tests::open_cache_dir_creates_relative_directory ... ok
test stdlib::network::tests::to_value_preserves_utf8 ... ok
test stdlib::network::tests::open_cache_dir_errors_for_file_path ... ok
test stdlib::network::tests::to_value_returns_bytes_for_invalid_utf8 ... ok
test stdlib::network::tests::open_cache_dir_rejects_invalid_paths::case_3 ... ok
test stdlib::path::io_helpers::tests::io_error_kind_label_matches_expected::case_1 ... ok
test stdlib::path::io_helpers::tests::io_error_kind_label_matches_expected::case_2 ... ok
test stdlib::path::io_helpers::tests::io_to_error_includes_label::case_1 ... ok
test stdlib::path::io_helpers::tests::io_to_error_includes_label::case_2 ... ok
test stdlib::path::io_helpers::tests::io_to_error_includes_label::case_3 ... ok
test stdlib::tests::validate_cache_relative_accepts_workspace_relative_paths ... ok
test stdlib::tests::validate_cache_relative_rejects_absolute_paths ... ok
test stdlib::tests::validate_cache_relative_rejects_empty ... ok
test stdlib::tests::validate_cache_relative_rejects_parent_components ... ok
test stdlib::time::tests::now_defaults_to_utc ... ok
test stdlib::time::tests::now_applies_custom_offset ... ok
test stdlib::time::tests::now_rejects_invalid_offset::case_1_nonsense ... ok
test stdlib::time::tests::now_rejects_invalid_offset::case_4_minutes_out_of_range ... ok
test stdlib::time::tests::now_rejects_invalid_offset::case_2_missing_sign ... ok
test stdlib::time::tests::timedelta_iso8601_property::case_1 ... ok
test stdlib::time::tests::now_rejects_invalid_offset::case_6_empty ... ok
test stdlib::time::tests::now_rejects_invalid_offset::case_5_seconds_out_of_range ... ok
test stdlib::time::tests::now_rejects_invalid_offset::case_3_hours_out_of_range ... ok
test stdlib::time::tests::timedelta_iso8601_property::case_2 ... ok
test stdlib::time::tests::timedelta_defaults_to_zero ... ok
test stdlib::time::tests::timedelta_iso8601_property::case_4 ... ok
test stdlib::time::tests::timedelta_accumulates_components ... ok
test stdlib::time::tests::timedelta_iso8601_property::case_3 ... ok
test stdlib::time::tests::timedelta_detects_overflow ... ok
test stdlib::time::tests::timestamp_iso8601_property::case_1 ... ok
test stdlib::time::tests::timestamp_iso8601_property::case_2 ... ok
test stdlib::time::tests::timedelta_supports_negative_values::case_1 ... ok
test stdlib::time::tests::timestamp_iso8601_property::case_3 ... ok
test stdlib::time::tests::timedelta_supports_negative_values::case_3 ... ok
test stdlib::time::tests::timedelta_supports_negative_values::case_2 ... ok
test stdlib::time::tests::timestamp_iso8601_property::case_4 ... ok
test stdlib::time::tests::timedelta_supports_negative_values::case_4 ... ok
test stdlib::time::tests::timedelta_supports_negative_values::case_6 ... ok
test stdlib::time::tests::timedelta_supports_negative_values::case_7 ... ok
test stdlib::time::tests::timedelta_supports_negative_values::case_5 ... ok
test stdlib::time::tests::timedelta_supports_negative_values::case_8 ... ok
test manifest::tests::from_path_uses_manifest_directory_for_caches ... ok
test result: ok. 111 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.01s
running 0 tests
test result: ok. 0 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.00s
running 2 tests
test manifest_subcommand_writes_file ... ok
test build_with_emit_writes_file ... ok
test result: ok. 2 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.01s
running 26 tests
test invalid_manifests_fail::case_2 ... ok
test invalid_manifests_fail::case_1 ... ok
test invalid_manifests_fail::case_3 ... ok
test actions_behaviour::case_2_explicit_phony_false ... ok
test load_manifest_from_file ... ok
test actions_behaviour::case_3_explicit_always_true ... ok
test invalid_manifests_fail::case_4 ... ok
test actions_behaviour::case_1_default_flags ... ok
test load_manifest_missing_file ... ok
test macro_serialization_with_special_characters_round_trips ... ok
test macro_definition_rejects_invalid_types ... ok
test parse_example_manifests::case_1 ... ok
test parse_example_manifests::case_2 ... ok
test empty_lists_and_maps ... ok
test multiple_actions_are_marked_phony ... ok
test parse_example_manifests::case_3 ... ok
test missing_required_fields ... ok
test optional_fields ... ok
test parse_minimal_manifest ... ok
test parsing_failures::case_1_invalid_enum_variant ... ok
test parsing_failures::case_2_actions_missing_recipe ... ok
test vars_section_must_be_object ... ok
test parses_macro_definitions ... ok
test unknown_fields ... ok
test phony_and_always_flags ... ok
test string_or_list_variants ... ok
test result: ok. 26 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.00s
running 10 tests
test parse_cli::case_1 ... ok
test parse_cli::case_3 ... ok
test parse_cli::case_4 ... ok
test parse_cli_errors::case_2 ... ok
test parse_cli_errors::case_1 ... ok
test parse_cli::case_5 ... ok
test parse_cli::case_2 ... ok
test parse_cli_errors::case_3 ... ok
test parse_cli_errors::case_4 ... ok
test parse_cli_errors::case_5 ... ok
test result: ok. 10 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.00s
running 9 tests
test invalid_command_errors::case_1 ... ok
test invalid_command_errors::case_3 ... ok
test command_without_placeholders_remains_valid ... ok
test newline_in_paths_is_quoted ... ok
test output_variable_overlap_not_rewritten ... ok
test inputs_and_outputs_are_quoted ... ok
test multiple_inputs_outputs_with_special_chars_are_quoted ... ok
test variable_name_overlap_not_rewritten ... ok
test invalid_command_errors::case_2 ... ok
test result: ok. 9 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.00s
Feature: CLI parsing
Scenario: Build is the default command
✔ When the CLI is parsed with ""
✔ Then parsing succeeds
✔ And the command is build
Scenario: Clean command runs
✔ When the CLI is parsed with "-C work clean"
✔ Then parsing succeeds
✔ And the command is clean
✔ And the working directory is "work"
Scenario: Graph command with jobs
✔ When the CLI is parsed with "-j 2 graph"
✔ Then parsing succeeds
✔ And the command is graph
✔ And the job count is 2
Scenario: Manifest file can be overridden
✔ When the CLI is parsed with "--file alt.yml build target"
✔ Then parsing succeeds
✔ And the manifest path is "alt.yml"
✔ And the first target is "target"
Scenario: Build command writes Ninja file
✔ When the CLI is parsed with "build --emit out.ninja target"
✔ Then parsing succeeds
✔ And the command is build
✔ And the emit path is "out.ninja"
✔ And the first target is "target"
Scenario: Manifest subcommand writes Ninja file
✔ When the CLI is parsed with "manifest out.ninja"
✔ Then parsing succeeds
✔ And the command is manifest
✔ And the manifest command path is "out.ninja"
Scenario: Manifest subcommand requires a path
✔ When the CLI is parsed with invalid arguments "manifest"
✔ Then an error should be returned
✔ And the error message should contain "<FILE>"
Scenario: Unknown command fails
✔ When the CLI is parsed with invalid arguments "unknown"
✔ Then an error should be returned
✔ And the error message should contain "unknown"
Scenario: Missing file argument value
✔ When the CLI is parsed with invalid arguments "--file"
✔ Then an error should be returned
✔ And the error message should contain "--file"
Scenario: Directory flag sets working directory
✔ When the CLI is parsed with "-C work build"
✔ Then parsing succeeds
✔ And the working directory is "work"
Scenario: Jobs flag sets parallelism
✔ When the CLI is parsed with "-j 4"
✔ Then parsing succeeds
✔ And the job count is 4
Scenario: Missing directory argument value
✔ When the CLI is parsed with invalid arguments "-C"
✔ Then an error should be returned
✔ And the error message should contain "--directory"
Scenario: Missing jobs argument value
✔ When the CLI is parsed with invalid arguments "-j"
✔ Then an error should be returned
✔ And the error message should contain "--jobs"
Scenario: Non-numeric jobs value
✔ When the CLI is parsed with invalid arguments "-j notanumber"
✔ Then an error should be returned
✔ And the error message should contain "notanumber"
Feature: BuildGraph
Scenario: New BuildGraph is empty
? When a new BuildGraph is created
Step skipped: tests/features/ir.feature:4:5
Scenario: BuildGraph from manifest
✔ When the manifest file "tests/data/rules.yml" is compiled to IR
✔ Then the graph has 1 actions
✔ And the graph has 1 targets
Scenario: Duplicate rules emit distinct actions (see docs/netsuke-design.md#55-design-decisions)
✔ When the manifest file "tests/data/duplicate_rules.yml" is compiled to IR
✔ Then the graph has 2 actions
✔ And the graph has 2 targets
Scenario: Rule not found during IR generation
✔ When the manifest file "tests/data/missing_rule.yml" is compiled to IR
✔ Then IR generation fails
Scenario: Multiple rules specified for target
✔ When the manifest file "tests/data/multiple_rules_per_target.yml" is compiled to IR
✔ Then IR generation fails
Scenario: Duplicate target outputs
✔ When the manifest file "tests/data/duplicate_outputs.yml" is compiled to IR
✔ Then IR generation fails
Scenario: Circular dependency detection
✔ When the manifest file "tests/data/circular.yml" is compiled to IR
✔ Then IR generation fails
Feature: Intermediate Representation (IR) Generation
Scenario: A new, empty BuildGraph has no content
✔ Given a new BuildGraph is created
✔ When its contents are checked
✔ Then the graph has 0 actions
✔ And the graph has 0 targets
✔ And the graph has 0 default targets
Scenario: Compiling a valid manifest with one rule and one target
✔ Given the manifest file "tests/data/rules.yml" is compiled to IR
✔ When the graph contents are checked
✔ Then the graph has 1 actions
✔ And the graph has 1 targets
Scenario: Identical rules emit distinct actions during IR generation (see docs/netsuke-design.md#55-design-decisions)
✔ Given the manifest file "tests/data/duplicate_rules.yml" is compiled to IR
✔ When the graph contents are checked
✔ Then the graph has 2 actions
✔ And the graph has 2 targets
Scenario: IR generation fails if a target references a rule that does not exist
✔ Given the manifest file "tests/data/missing_rule.yml" is compiled to IR
✔ When the generation result is checked
✔ Then IR generation fails
Scenario: IR generation fails if a target specifies multiple rules
✔ Given the manifest file "tests/data/multiple_rules_per_target.yml" is compiled to IR
✔ When the generation result is checked
✔ Then IR generation fails
Scenario: IR generation fails if multiple targets produce the same output file
✔ Given the manifest file "tests/data/duplicate_outputs.yml" is compiled to IR
✔ When the generation result is checked
✔ Then IR generation fails
Scenario: IR generation fails if there is a circular dependency between targets
✔ Given the manifest file "tests/data/circular.yml" is compiled to IR
✔ When the generation result is checked
✔ Then IR generation fails
Feature: Manifest Parsing
Scenario: Parsing a minimal valid manifest
✔ Given the manifest file "tests/data/minimal.yml" is parsed
✔ When the version is checked
✔ Then the manifest version is "1.0.0"
✔ And the first target name is "hello"
Scenario: Parsing a manifest with phony and always flags
✔ Given the manifest file "tests/data/phony.yml" is parsed
✔ When the flags are checked
✔ Then the target 1 is phony
✔ And the target 1 is always rebuilt
Scenario: A target in the 'actions' block is implicitly phony
✔ Given the manifest file "tests/data/actions.yml" is parsed
✔ When the flags are checked
✔ Then the first action is phony
Scenario: Parsing a manifest with rules
✔ Given the manifest file "tests/data/rules.yml" is parsed
✔ When the rules are checked
✔ Then the first rule name is "compile"
✔ And the first target name is "hello.o"
Scenario: Parsing fails for a manifest with an unknown top-level field
✔ Given the manifest file "tests/data/unknown_field.yml" is parsed
✔ When the parsing result is checked
✔ Then parsing the manifest fails
Scenario: Parsing fails for a manifest with an invalid version string
✔ Given the manifest file "tests/data/invalid_version.yml" is parsed
✔ When the parsing result is checked
✔ Then parsing the manifest fails
Scenario: Parsing fails for a target that is missing a recipe
✔ Given the manifest file "tests/data/missing_recipe.yml" is parsed
✔ When the parsing result is checked
✔ Then parsing the manifest fails
Scenario: Parsing fails for an action that is missing a recipe
✔ Given the manifest file "tests/data/action_invalid.yml" is parsed
✔ When the parsing result is checked
✔ Then parsing the manifest fails
Scenario: Rendering Jinja variables in a manifest
✔ Given the manifest file "tests/data/jinja_vars.yml" is parsed
✔ When the manifest is checked
✔ Then the first target command is "echo world"
Scenario: Parsing fails when a Jinja variable is undefined
✔ Given the manifest file "tests/data/jinja_undefined.yml" is parsed
✔ When the parsing result is checked
✔ Then parsing the manifest fails
Scenario: Reading environment variables in a manifest
✔ Given the environment variable "NETSUKE_TEST_ENV" is set to "world"
✔ And the manifest file "tests/data/jinja_env.yml" is parsed
✔ When the manifest is checked
✔ Then the first target command is "echo world"
Scenario: Rendering manifest macros
✔ Given the manifest file "tests/data/jinja_macros.yml" is parsed
✔ When the manifest is checked
✔ Then the first target command is "HELLO NETSUKE!"
Scenario: Rendering manifest macros with varied signatures
✔ Given the manifest file "tests/data/jinja_macro_arguments.yml" is parsed
✔ When the manifest is checked
✔ Then the manifest has 4 macros
✔ And the macro 1 signature is "no_args()"
✔ And the target 1 command is "ready"
✔ And the target 2 command is "Hi world"
✔ And the target 3 command is "a,b,c"
✔ And the target 4 command is "Netsuke!"
Scenario: Parsing fails when an environment variable is undefined
✔ Given the environment variable "NETSUKE_UNDEFINED_ENV" is unset
✔ And the manifest file "tests/data/jinja_env_missing.yml" is parsed
✔ When the parsing result is checked
✔ Then parsing the manifest fails
Scenario: Parsing fails when a macro is missing its signature
✔ Given the manifest file "tests/data/jinja_macro_invalid.yml" is parsed
✔ When the parsing result is checked
✔ Then parsing the manifest fails
✔ And the error message contains "signature"
Scenario: Parsing fails when a macro omits parentheses
✔ Given the manifest file "tests/data/jinja_macro_missing_parens.yml" is parsed
✔ When the parsing result is checked
✔ Then parsing the manifest fails
✔ And the error message contains "parameter list"
Scenario: Rendering Jinja conditionals in a manifest
✔ Given the manifest file "tests/data/jinja_if.yml" is parsed
✔ When the manifest is checked
✔ Then the first target name is "hello"
✔ And the first target command is "echo on"
Scenario: Rendering Jinja conditionals in a manifest (disabled)
✔ Given the manifest file "tests/data/jinja_if_disabled.yml" is parsed
✔ When the manifest is checked
✔ Then the first target name is "hello"
✔ And the first target command is "echo off"
Scenario: Generating targets with foreach
✔ Given the manifest file "tests/data/foreach.yml" is parsed
✔ When the manifest is checked
✔ Then the manifest has 2 targets
✔ And the target 1 name is "foo"
✔ And the target 1 command is "echo 'foo'"
✔ And the target 1 index is 0
✔ And the target 2 name is "bar"
✔ And the target 2 command is "echo 'bar'"
✔ And the target 2 index is 1
Scenario: Generating targets with glob
✔ Given the manifest file "tests/data/glob.yml" is parsed
✔ When the manifest is checked
✔ Then the manifest has 2 targets
✔ And the target 1 name is "a.out"
✔ And the target 1 index is 0
✔ And the target 2 name is "b.out"
✔ And the target 2 index is 1
Scenario: Generating targets with glob using Windows separators
✔ Given the manifest file "tests/data/glob_windows.yml" is parsed
✔ When the manifest is checked
✔ Then the manifest has 2 targets
✔ And the target 1 name is "a.out"
✔ And the target 2 name is "b.out"
✔ And the target 1 index is 0
✔ And the target 2 index is 1
Scenario: Parsing fails for an invalid glob pattern
✔ Given the manifest file "tests/data/glob_invalid.yml" is parsed
✔ When the parsing result is checked
✔ Then parsing the manifest fails
✔ And the error message contains "glob pattern"
Scenario: Parsing fails for an invalid glob brace pattern
✔ Given the manifest file "tests/data/glob_invalid_brace.yml" is parsed
✔ When the parsing result is checked
✔ Then parsing the manifest fails
✔ And the error message contains "glob pattern"
✔ And the error message contains "unmatched"
Scenario: Parsing fails when a foreach expression is not iterable
✔ Given the manifest file "tests/data/foreach_invalid.yml" is parsed
✔ When the parsing result is checked
✔ Then parsing the manifest fails
Scenario: Rendering all target fields
✔ Given the manifest file "tests/data/render_target.yml" is parsed
✔ When the manifest is checked
✔ Then the target 1 name is "base1"
✔ And the target 1 has source "base1.src"
✔ And the target 1 has dep "base1.dep"
✔ And the target 1 has order-only dep "base1.ord"
✔ And the target 1 command is "echo base1"
✔ And the target 2 script is "run base.sh"
✔ And the target 3 rule is "base-rule"
Scenario Outline: Targets default flags are false
✔ Given the manifest file "tests/data/target_defaults.yml" is parsed
✔ When the manifest is checked
✔ Then the manifest has 3 targets
✔ And the target 1 is not phony
✔ And the target 1 is not always rebuilt
Scenario Outline: Targets default flags are false
✔ Given the manifest file "tests/data/target_defaults.yml" is parsed
✔ When the manifest is checked
✔ Then the manifest has 3 targets
✔ And the target 2 is not phony
✔ And the target 2 is not always rebuilt
Scenario Outline: Targets default flags are false
✔ Given the manifest file "tests/data/target_defaults.yml" is parsed
✔ When the manifest is checked
✔ Then the manifest has 3 targets
✔ And the target 3 is not phony
✔ And the target 3 is not always rebuilt
Scenario: Parsing fails when rule and command are both defined
✔ Given the manifest file "tests/data/rule_command_conflict.yml" is parsed
✔ When the parsing result is checked
✔ Then parsing the manifest fails
Feature: Ninja file generation
Scenario: Generate build statements
✔ When the manifest file "tests/data/rules.yml" is compiled to IR
✔ And the ninja file is generated
✔ Then the ninja file contains "rule"
✔ And the ninja file contains "build hello.o:"
Scenario: Phony target runs its command
✔ When the manifest file "tests/data/phony.yml" is compiled to IR
✔ And the ninja file is generated
✔ Then the ninja file contains "build clean:"
✔ And the ninja file contains "rm -rf build"
Scenario: Inputs and outputs are shell-quoted
✔ When the manifest file "tests/data/quote.yml" is compiled to IR
✔ And the ninja file is generated
✔ Then shlex splitting the command yields "cat, in file, >, out file"
Scenario: Edge-case paths are shell-quoted
✔ When the manifest file "tests/data/quote.yml" is compiled to IR
✔ And the ninja file is generated
✔ Then shlex splitting command 3 yields "printf, %s, -in file, >, o'utfile"
Scenario: Missing action is reported
✔ When the manifest file "tests/data/rules.yml" is compiled to IR
✔ And an action is removed from the graph
✔ And the ninja file is generated
✔ Then ninja generation fails mentioning the removed action id
Feature: Ninja process execution
Scenario: Ninja succeeds
✔ Given a fake ninja executable that exits with 0
✔ And the CLI is parsed with ""
✔ When the ninja process is run
✔ Then the command should succeed
Scenario: Ninja fails
✔ Given a fake ninja executable that exits with 1
✔ And the CLI is parsed with ""
✔ When the ninja process is run
✔ Then the command should fail with error "ninja exited with exit status: 1"
Scenario: Ninja missing
✔ Given no ninja executable is available
✔ And the CLI is parsed with ""
✔ When the ninja process is run
✔ Then the command should fail with error "No such file or directory"
Scenario: Build file missing
✔ Given a fake ninja executable that checks for the build file
✔ And the CLI is parsed with ""
✔ And the CLI uses the temporary directory
✔ When the ninja process is run
✔ Then the command should fail with error "ninja exited with exit status: 1"
Scenario: Build file is not a regular file
✔ Given a fake ninja executable that checks for the build file
✔ And the CLI is parsed with ""
✔ And the CLI uses the temporary directory
✔ And a directory named build.ninja exists
✔ When the ninja process is run
✔ Then the command should fail with error "ninja exited with exit status: 1"
Feature: Template stdlib filters
Scenario: Rendering basename for a file path
✔> Given a stdlib workspace
✔ When I render "{{ path | basename }}" with stdlib path "file"
✔ Then the stdlib output is "file"
Scenario: Dirname resolves to the workspace root
✔> Given a stdlib workspace
✔ When I render "{{ path | dirname }}" with stdlib path "file"
✔ Then the stdlib output equals the workspace root
Scenario: relative_to returns the child component
✔> Given a stdlib workspace
✔ Given the stdlib file "nested/file.txt" contains "nested"
✔ When I render "{{ path | relative_to(path | dirname) }}" with stdlib path "nested/file.txt"
✔ Then the stdlib output is "file.txt"
Scenario: with_suffix rewrites extensions
✔> Given a stdlib workspace
✔ When I render "{{ path | with_suffix('.log') }}" with stdlib path "file.tar.gz"
✔ Then the stdlib output is the workspace path "file.tar.log"
Scenario: expanduser expands the home directory
✔> Given a stdlib workspace
✔ Given HOME points to the stdlib workspace root
✔ When I render "{{ path | expanduser }}" with stdlib path "~/workspace"
✔ Then the stdlib output is the workspace path "workspace"
Scenario: contents reads a file
✔> Given a stdlib workspace
✔ When I render "{{ path | contents }}" with stdlib path "file"
✔ Then the stdlib output is "data"
Scenario: linecount counts newline-delimited lines
✔> Given a stdlib workspace
✔ When I render "{{ path | linecount }}" with stdlib path "lines.txt"
✔ Then the stdlib output is "3"
Scenario: size returns the byte length
✔> Given a stdlib workspace
✔ When I render "{{ path | size }}" with stdlib path "file"
✔ Then the stdlib output is "4"
Scenario: Size filter reports errors for missing files
✔> Given a stdlib workspace
✔ When I render "{{ path | size }}" with stdlib path "missing"
✔ Then the stdlib error contains "not found"
Scenario: hash computes the sha256 digest
✔> Given a stdlib workspace
✔ When I render "{{ path | hash('sha256') }}" with stdlib path "file"
✔ Then the stdlib output is "3a6eb0790f39ac87c94f3856b2dd2c5d110e6811602261a9a923d3bb23adc8b7"
Scenario: digest truncates the hash output
✔> Given a stdlib workspace
✔ When I render "{{ path | digest(8, 'sha256') }}" with stdlib path "file"
✔ Then the stdlib output is "3a6eb079"
Scenario: uniq removes duplicates
✔> Given a stdlib workspace
✔ When I render "{{ ['a', 'a', 'b'] | uniq | join(',') }}" with stdlib path "file"
✔ Then the stdlib output is "a,b"
Scenario: flatten merges deeply nested lists
✔> Given a stdlib workspace
✔ When I render "{{ [[['a']], [['b']], [['c']]] | flatten | join(',') }}" with stdlib path "file"
✔ Then the stdlib output is "a,b,c"
Scenario: flatten reports errors for scalar items
✔> Given a stdlib workspace
✔ When I render "{{ [['a'], 'b'] | flatten }}" with stdlib path "file"
✔ Then the stdlib error contains "flatten expected sequence items"
Scenario: group_by clusters items by attribute
✔> Given a stdlib workspace
✔ When I render "{{ ([{'name': 'one', 'kind': 'tool'}, {'name': 'two', 'kind': 'tool'}, {'name': 'three', 'kind': 'material'}] | group_by('kind')).tool | length }}" with stdlib path "file"
✔ Then the stdlib output is "2"
Scenario: group_by clusters items with non-string keys
✔> Given a stdlib workspace
✔ When I render "{{ ([{'kind': 1}, {'kind': 1}, {'kind': 2}] | group_by('kind'))[1] | length }}" with stdlib path "file"
✔ Then the stdlib output is "2"
Scenario: group_by reports errors for missing attributes
✔> Given a stdlib workspace
✔ When I render "{{ ([{'name': 'one'}] | group_by('kind')) }}" with stdlib path "file"
✔ Then the stdlib error contains "could not resolve"
Scenario: shell filter transforms text and marks templates impure
✔> Given a stdlib workspace
✔ Given an uppercase stdlib command helper
✔ When I render the stdlib template "{{ 'hello' | shell(cmd) | trim }}" using the stdlib command helper
✔ Then the stdlib output is "HELLO"
✔ And the stdlib template is impure
Scenario: shell filter reports command failures
✔> Given a stdlib workspace
✔ Given a failing stdlib command helper
✔ When I render the stdlib template "{{ 'data' | shell(cmd) }}" using the stdlib command helper
✔ Then the stdlib error contains "exited"
✔ And the stdlib template is impure
Scenario: grep filter extracts matching lines
✔> Given a stdlib workspace
✔ When I render the stdlib template "{{ 'alpha\nbeta\n' | grep('beta') | trim }}"
✔ Then the stdlib output is "beta"
✔ And the stdlib template is impure
Scenario: fetch retrieves remote content and marks templates impure
✔> Given a stdlib workspace
✔ Given an HTTP server returning "payload"
✔ When I render "{{ fetch(url) }}" with stdlib url
✔ Then the stdlib output is "payload"
✔ And the stdlib template is impure
Scenario: fetch reports network errors
✔> Given a stdlib workspace
✔ When I render the stdlib template "{{ fetch('http://127.0.0.1:9') }}"
✔ Then the stdlib error contains "fetch failed"
✔ And the stdlib template is impure
Scenario: fetch caches responses inside the workspace
✔> Given a stdlib workspace
✔ Given an HTTP server returning "cached"
✔ When I render "{{ fetch(url, cache=true) }}" with stdlib url
✔ Then the stdlib output is "cached"
✔ And the stdlib template is impure
✔ And the stdlib workspace contains the fetch cache for stdlib url
Scenario: fetch rejects cache_dir overrides
✔> Given a stdlib workspace
✔ Given an HTTP server returning "payload"
? When I render the stdlib template "{{ fetch(url, cache=true, cache_dir='.netsuke/cache') }}" with stdlib url
Step skipped: tests/features/stdlib.feature:112:5
Scenario: fetch rejects absolute cache_dir overrides
✔> Given a stdlib workspace
✔ Given an HTTP server returning "payload"
? When I render the stdlib template "{{ fetch(url, cache=true, cache_dir='/etc/netsuke-cache') }}" with stdlib url
Step skipped: tests/features/stdlib.feature:118:5
Scenario: fetch rejects parent-relative cache_dir overrides
✔> Given a stdlib workspace
✔ Given an HTTP server returning "payload"
? When I render the stdlib template "{{ fetch(url, cache=true, cache_dir='../cache') }}" with stdlib url
Step skipped: tests/features/stdlib.feature:124:5
Feature: Template time helpers
Scenario: Rendering now() yields a UTC timestamp
✔ Given a stdlib workspace
✔ When I render the stdlib template "{{ now() }}"
✔ Then the stdlib output is an ISO8601 UTC timestamp
Scenario: Rendering now() with an offset preserves the offset
✔ Given a stdlib workspace
✔ When I render the stdlib template "{{ now(offset='+02:00').iso8601 }}"
✔ Then the stdlib output offset is "+02:00"
Scenario: Timedelta composes multiple components
✔ Given a stdlib workspace
✔ When I render the stdlib template "{{ timedelta(days=1, hours=2, minutes=30, seconds=5, milliseconds=750, microseconds=250).iso8601 }}"
✔ Then the stdlib output is "P1DT2H30M5.75025S"
Scenario: Timedelta captures nanosecond precision
✔ Given a stdlib workspace
✔ When I render the stdlib template "{{ timedelta(nanoseconds=1).iso8601 }}"
✔ Then the stdlib output is "PT0.000000001S"
Scenario: Timedelta supports negative values
✔ Given a stdlib workspace
✔ When I render the stdlib template "{{ timedelta(hours=-1).iso8601 }}"
✔ Then the stdlib output is "-PT1H"
Scenario: Timedelta overflow surfaces an error
✔ Given a stdlib workspace
✔ When I render the stdlib template "{{ timedelta(days=9223372036854775807) }}"
✔ Then the stdlib error contains "overflow"
Scenario: now() rejects invalid offsets
✔ Given a stdlib workspace
✔ When I render the stdlib template "{{ now(offset='bogus') }}"
✔ Then the stdlib error contains "invalid"
[Summary]
8 features
100 scenarios (96 passed, 4 skipped)
371 steps (367 passed, 4 skipped)
Feature: File-system tests
Scenario: file system tests detect path types
✔ Given a file-type test workspace
✔ When the manifest file "tests/data/jinja_is.yml" is parsed
? And the manifest has targets named "is-dir, is-file, is-symlink, is-pipe, is-block-device, is-char-device, is-device"
Step skipped: tests/features_unix/fs_tests.feature:7:5
Scenario: file system tests return false for missing paths
✔ Given a file-type test workspace
✔ And the environment variable "DIR_PATH" is set to "${WORKSPACE}/.missing/dir"
✔ And the environment variable "FILE_PATH" is set to "${WORKSPACE}/.missing/file"
✔ And the environment variable "SYMLINK_PATH" is set to "${WORKSPACE}/.missing/symlink"
✔ And the environment variable "PIPE_PATH" is set to "${WORKSPACE}/.missing/pipe"
✔ And the environment variable "BLOCK_DEVICE_PATH" is set to "${WORKSPACE}/.missing/block"
✔ And the environment variable "CHAR_DEVICE_PATH" is set to "${WORKSPACE}/.missing/char"
✔ And the environment variable "DEVICE_PATH" is set to "${WORKSPACE}/.missing/device"
✔ When the manifest file "tests/data/jinja_is.yml" is parsed
? Then the manifest has targets 0
Step skipped: tests/features_unix/fs_tests.feature:19:5
Feature: Unix stdlib filters
Scenario: realpath resolves symlinks
✔ Given a stdlib workspace
✔ When I render "{{ path | realpath }}" with stdlib path "link"
✔ Then the stdlib output is the workspace path "file"
[Summary]
2 features
3 scenarios (1 passed, 2 skipped)
16 steps (14 passed, 2 skipped)
running 3 tests
test prepend_dir_to_path_handles_empty_path ... ok
test prepend_dir_to_path_sets_and_restores ... ok
test prepend_dir_to_path_handles_missing_path ... ok
test result: ok. 3 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.00s
running 6 tests
test hash_action_is_stable::case_1 ... ok
test hash_action_is_stable::case_2 ... ok
test hash_action_is_stable::case_3 ... ok
test hash_action_is_stable::case_4 ... ok
test hash_action_is_stable::case_6 ... ok
test hash_action_is_stable::case_5 ... ok
test result: ok. 6 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.00s
running 9 tests
test manifest_error_cases::case_3 ... ok
test manifest_error_cases::case_4 ... ok
test manifest_error_cases::case_1 ... ok
test manifest_error_cases::case_5 ... ok
test manifest_error_cases::case_2 ... ok
test duplicate_rules_emit_distinct_actions ... ok
test missing_rule_fails ... ok
test minimal_manifest_to_ir ... ok
test manifest_error_cases::case_6 ... ok
test result: ok. 9 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.00s
running 4 tests
test build_graph_default_is_empty ... ok
test build_graph_duplicate_action_ids ... ok
test build_graph_duplicate_targets ... ok
test create_action_and_edge ... ok
test result: ok. 4 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.00s
running 5 tests
test missing_env_var_errors ... ok
test env_var_renders_parameterized::case_1 ... ok
test non_unicode_env_var_errors ... ok
test env_var_renders_parameterized::case_2 ... ok
test env_var_renders_parameterized::case_3 ... ok
test result: ok. 5 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.00s
running 28 tests
test glob_unmatched_brace_errors::case_1 ... ok
test glob_braces_in_classes_and_nested::case_1 ... ok
test glob_braces_in_classes_and_nested::case_4 ... ok
test glob_braces_in_classes_and_nested::case_3 ... ok
test glob_escaped_braces_are_literals::case_6 ... ok
test glob_escaped_braces_are_literals::case_2 ... ok
test glob_escaped_braces_are_literals::case_1 ... ok
test glob_escaped_braces_are_literals::case_4 ... ok
test glob_escaped_braces_are_literals::case_3 ... ok
test glob_escaped_braces_are_literals::case_5 ... ok
test glob_braces_in_classes_and_nested::case_2 ... ok
test glob_unmatched_brace_errors::case_3 ... ok
test glob_unmatched_closing_brace_reports_position ... ok
test glob_unmatched_brace_errors_with_escapes::case_3 ... ok
test glob_accepts_windows_path_separators ... ok
test glob_unmatched_brace_errors_with_escapes::case_2 ... ok
test glob_unmatched_brace_errors::case_4 ... ok
test test_glob_behavior::case_2 ... ok
test test_glob_behavior::case_3 ... ok
test test_glob_behavior::case_1 ... ok
test glob_unmatched_bracket_errors ... ok
test glob_unmatched_brace_errors::case_2 ... ok
test glob_unmatched_opening_brace_reports_position ... ok
test glob_unmatched_brace_errors_with_escapes::case_1 ... ok
test test_glob_behavior::case_7 ... ok
test test_glob_behavior::case_5 ... ok
test test_glob_behavior::case_4 ... ok
test test_glob_behavior::case_6 ... ok
test result: ok. 28 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.01s
running 29 tests
test foreach_vars_must_be_mapping ... ok
test foreach_non_iterable_errors::case_2 ... ok
test foreach_non_iterable_errors::case_1 ... ok
test manifest_macro_with_missing_signature_errors ... ok
test expands_single_item_foreach_targets ... ok
test foreach_when_filters_items ... ok
test expands_foreach_with_item_and_index::case_2 ... ok
test no_targets_generated_scenarios::case_1 ... ok
test no_targets_generated_scenarios::case_3 ... ok
test expands_foreach_with_item_and_index::case_1 ... ok
test no_targets_generated_scenarios::case_2 ... ok
test manifest_macro_with_missing_body_errors ... ok
test registers_manifest_macro_argument_variants::case_1 ... ok
test registers_manifest_macro_argument_variants::case_2 ... ok
test registers_manifest_macro_argument_variants::case_3 ... ok
test registers_manifest_macro_argument_variants::case_4 ... ok
test registers_manifest_macros ... ok
test render_target_missing_var_errors ... ok
test renders_global_vars ... ok
test renders_target_fields_recipe_types::case_1 ... ok
test renders_env_function_missing_var ... ok
test renders_if_blocks::case_2 ... ok
test renders_env_function ... ok
test syntax_error_errors ... ok
test undefined_variable_errors ... ok
test renders_if_blocks::case_1 ... ok
test renders_target_fields_recipe_types::case_2 ... ok
test renders_target_fields_command ... ok
test undefined_in_if_errors ... ok
test result: ok. 29 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.00s
running 2 tests
test override_ninja_env_sets_and_restores ... ok
test override_ninja_env_unset_removes_variable ... ok
test result: ok. 2 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.00s
running 11 tests
test generate_empty_graph ... ok
test errors_when_action_missing ... ok
test generate_format_error ... ok
test generate_ninja_scenarios::case_1_phony_target_runs_command ... ok
test generate_ninja_scenarios::case_3_complex_dependencies ... ok
test generate_ninja_scenarios::case_2_standard_build ... ok
test ninja_integration_tests::case_1_multiline_script_valid ... ok
test ninja_integration_tests::case_2_script_with_percent ... ok
test ninja_integration_tests::case_3_script_with_backtick ... ok
test ninja_integration_tests::case_4_phony_action_executes_command ... ok
test generate_multiline_script_snapshot ... ok
test result: ok. 11 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.02s
running 1 test
test touch_manifest_ninja_validation ... ok
test result: ok. 1 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.00s
running 1 test
test restores_path_without_touching_real_env ... ok
test result: ok. 1 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.00s
running 7 tests
test run_ninja_not_found ... ok
test run_exits_with_manifest_error_on_invalid_version ... ok
test run_manifest_subcommand_writes_file ... ok
test run_build_with_emit_creates_parent_dirs ... ok
test run_build_with_emit_keeps_file ... ok
test run_respects_env_override_for_ninja ... ok
test run_executes_ninja_without_persisting_file ... ok
test result: ok. 7 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.01s
running 9 tests
test yaml_diagnostics_are_actionable::case_3 ... ok
test yaml_diagnostics_are_actionable::case_4 ... ok
test yaml_diagnostics_are_actionable::case_1 ... ok
test yaml_diagnostics_are_actionable::case_5 ... ok
test yaml_diagnostics_are_actionable::case_2 ... ok
test yaml_diagnostics_are_actionable::case_9 ... ok
test yaml_diagnostics_are_actionable::case_6 ... ok
test yaml_diagnostics_are_actionable::case_7 ... ok
test yaml_diagnostics_are_actionable::case_8 ... ok
test result: ok. 9 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.00s suite passes again.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Add deny rules for unknown and renamed/removed lints and reorganize documentation linting. Move crate-level missing-docs into the rustdoc lint group while keeping general missing-docs under rust lints. This strengthens lint validation (catching misnamed or deprecated lints) and clarifies rust vs rustdoc responsibilities for documentation checks.
|
@coderabbitai Please suggest a fix for this issue and supply a prompt for an AI coding agent to enable it to apply the fix: tests/steps/stdlib_steps/types.rs Comment on file //! Test-only wrapper types that provide lightweight structure around stdlib
❌ New issue: String Heavy Function Arguments Use NewTypes to model domain values and eliminate "string soup". Reach for newt-hype when introducing many homogeneous wrappers that share behaviour; add small shims such as From<&str> and AsRef for string-backed wrappers. For path-centric wrappers implement AsRef alongside into_inner() and to_path_buf(); avoid attempting impl From for PathBuf because of the orphan rule. Prefer explicit tuple structs whenever bespoke validation or tailored trait surfaces are required, customising Deref, AsRef, and TryFrom per type. Use the-newtype when defining traits and needing blanket implementations that apply across wrappers satisfying Newtype + AsRef/AsMut, or when establishing a coherent internal convention that keeps trait forwarding consistent without per-type boilerplate. Combine approaches: lean on newt-hype for the common case, tuple structs for outliers, and the-newtype to unify behaviour when you own the trait definitions. |
This comment was marked as resolved.
This comment was marked as resolved.
Switch the stdlib file-writing Cucumber step to accept `RelativePath` and `FileContent`, consuming them via `into_path_buf`/`into_bytes` so we no longer rely on raw strings. Add `FromStr` and `into_bytes` helpers to `FileContent` so cucumber captures map cleanly and Clippy stays quiet. Validation: make check-fmt, make lint, make test.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Adds a RUSTDOC_FLAGS default and runs cargo doc with RUSTDOCFLAGS before clippy in the lint task. Enforces documentation warnings as errors (via --cfg docsrs -D warnings) so doc lints are caught during the lint step, improving CI/local feedback and keeping docs quality consistent with code lints.
Add stricter rustdoc lint denies to catch broken intra-doc links, private intra-doc links, bare URLs, invalid HTML tags, invalid codeblock attributes, and unescaped backticks. Tighten documentation checks so doc-related errors fail early in CI and prevent regressions. Also normalize lint formatting for consistency.
leynos
force-pushed
the
additional-lints
branch
from
20e65e8 to
7261304
Compare
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
@coderabbitai Please suggest a fix for this issue and supply a prompt for an AI coding agent to enable it to apply the fix: Code Duplicationtests/manifest_jinja_tests.rs: What lead to degradation?The module contains 7 functions with similar structure: manifest_macro_with_missing_body_errors,manifest_macro_with_missing_signature_errors,render_target_missing_var_errors,renders_env_function and 3 more functions Why does this problem occur?Duplicated code often leads to code that's harder to change since the same logical change has to be done in multiple functions. More duplication gives lower code health. How to fix it?A certain degree of duplicated code might be acceptable. The problems start when it is the same behavior that is duplicated across the functions in the module, ie. a violation of the Don't Repeat Yourself (DRY) principle. DRY violations lead to code that is changed together in predictable patterns, which is both expensive and risky. DRY violations can be identified using CodeScene's X-Ray analysis to detect clusters of change coupled functions with high code similarity. Read More |
This comment was marked as resolved.
This comment was marked as resolved.
Ensure the `ninja --version` probe runs with fully detached stdio by adding `.stdin(Stdio::null())`, and treat timeout cleanup as best-effort so failures killing or waiting on the child do not mask the intended `ProbeTimeout` error. Validation: make check-fmt, make lint, make test.
Add narrowly scoped expectations to the Ninja and stdlib rendering steps and adjust the helpers to borrow , ensuring the lint stays satisfied without sacrificing Cucumber's owned captures. Removed the now-unused . Validation: make check-fmt, make lint, make test.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
@coderabbitai Please suggest a fix for this issue and supply a prompt for an AI coding agent to enable it to apply the fix: tests/steps/stdlib_steps/types.rs Comment on file //! Test-only wrapper types that provide lightweight structure around stdlib
❌ New issue: String Heavy Function Arguments Use NewTypes to model domain values and eliminate "string soup". Reach for newt-hype when introducing many homogeneous wrappers that share behaviour; add small shims such as From<&str> and AsRef for string-backed wrappers. For path-centric wrappers implement AsRef alongside into_inner() and to_path_buf(); avoid attempting impl From for PathBuf because of the orphan rule. Prefer explicit tuple structs whenever bespoke validation or tailored trait surfaces are required, customising Deref, AsRef, and TryFrom per type. Use the-newtype when defining traits and needing blanket implementations that apply across wrappers satisfying Newtype + AsRef/AsMut, or when establishing a coherent internal convention that keeps trait forwarding consistent without per-type boilerplate. Combine approaches: lean on newt-hype for the common case, tuple structs for outliers, and the-newtype to unify behaviour when you own the trait definitions. |
|
@coderabbitai Please suggest a fix for this issue and supply a prompt for an AI coding agent to enable it to apply the fix: Comment on lines +27 to +63 fn probe_ninja() -> Result<(), NinjaWorkspaceError> {
let mut child = Command::new("ninja")
.arg("--version")
.stdin(Stdio::null())
.stdout(Stdio::null())
.stderr(Stdio::null())
.spawn()
.map_err(NinjaWorkspaceError::ProbeSpawn)?;
let timeout = Duration::from_secs(2);
let poll_sleep = Duration::from_millis(50);
let start = Instant::now();
loop {
match child.try_wait() {
Ok(Some(status)) => {
if status.success() {
return Ok(());
}
return Err(NinjaWorkspaceError::ProbeFailed(status));
}
Ok(None) => {
if start.elapsed() >= timeout {
match child.kill() {
Ok(_) | Err(_) => {}
}
match child.wait() {
Ok(_) | Err(_) => {}
}
return Err(NinjaWorkspaceError::ProbeTimeout(timeout));
}
thread::sleep(poll_sleep);
}
Err(err) => return Err(NinjaWorkspaceError::ProbeSpawn(err)),
}
}
}❌ New issue: Bumpy Road Ahead |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 2546 @leynos: I've analysed the nested conditionals in Suggested FixExtract the timeout cleanup logic into a helper function: /// Best-effort cleanup of a child process that has timed out.
/// Attempts to kill the process and wait for it to exit, ignoring any errors.
fn cleanup_timed_out_child(child: &mut std::process::Child) {
// Try to kill; if it fails, the process may have already exited.
let _ = child.kill();
// Try to reap the child; if it fails, we've done our best.
let _ = child.wait();
}
fn probe_ninja() -> Result<(), NinjaWorkspaceError> {
let mut child = Command::new("ninja")
.arg("--version")
.stdin(Stdio::null())
.stdout(Stdio::null())
.stderr(Stdio::null())
.spawn()
.map_err(NinjaWorkspaceError::ProbeSpawn)?;
let timeout = Duration::from_secs(2);
let poll_sleep = Duration::from_millis(50);
let start = Instant::now();
loop {
match child.try_wait() {
Ok(Some(status)) => {
if status.success() {
return Ok(());
}
return Err(NinjaWorkspaceError::ProbeFailed(status));
}
Ok(None) => {
if start.elapsed() >= timeout {
cleanup_timed_out_child(&mut child);
return Err(NinjaWorkspaceError::ProbeTimeout(timeout));
}
thread::sleep(poll_sleep);
}
Err(err) => return Err(NinjaWorkspaceError::ProbeSpawn(err)),
}
}
}This refactoring:
AI Coding Agent Prompt🐰 |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 6777 🏁 Script executed: Length of output: 630 🏁 Script executed: Length of output: 62 @leynos: I've analysed the current state of AnalysisThe 47.9% string-argument metric is counting trait implementations rather than business logic functions: What the file contains:
Why the metric is high: impl From<String> for TypeName { ... } // Required for Cucumber
impl From<&str> for TypeName { ... } // Convenience
impl FromStr for TypeName { ... } // Standard trait
impl AsRef<str> for TypeName { ... } // Standard trait
// etc.These trait implementations are essential infrastructure for the newtype pattern to work with Cucumber—they enable automatic conversion from captured regex groups (strings) into your domain types. Why No Fix Is Needed
This is analogous to a module full of RecommendationNo code changes required. The static analysis tool is flagging necessary trait overhead as a code smell. The module correctly implements the newtype pattern, and attempting to reduce string parameters would break the design. If you wish to suppress this false positive in future CodeScene reports, you might want to configure the tool to exclude trait implementation files from the string-heavy-arguments check, or adjust the threshold for modules that are primarily trait implementations. 🐰✨ |
Add a dedicated helper and invoke it from the timeout branch so we can reuse the best-effort kill/wait logic without extra nesting. Behaviour, timeouts, and error reporting stay the same. Validation: make check-fmt, make lint, make test.
There was a problem hiding this comment.
Gates Failed
(5 files with Code Duplication, Excess Number of Function Arguments, String Heavy Function Arguments)
Gates Passed
See analysis details in CodeScene
Reason for failure
| Enforce advisory code health rules | Violations | Code Health Impact | |
|---|---|---|---|
| manifest_jinja_tests.rs | 1 advisory rule | 10.00 → 9.39 | Suppress |
| collection_filters.rs | 1 advisory rule | 10.00 → 9.39 | Suppress |
| command_filters.rs | 1 advisory rule | 10.00 → 9.39 | Suppress |
| path_filters.rs | 2 advisory rules | 9.39 → 8.82 | Suppress |
| types.rs | 1 advisory rule | 9.69 | 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.
|
@coderabbitai Please suggest a fix for this issue and supply a prompt for an AI coding agent to enable it to apply the fix: Excess Number of Function Argumentstests/std_filter_tests/path_filters.rs: assert_filter_success_with_env What lead to degradation?assert_filter_success_with_env has 6 arguments, max arguments = 4 Why does this problem occur?Functions with many arguments indicate either a) low cohesion where the function has too many responsibilities, or b) a missing abstraction that encapsulates those arguments. How to fix it?Start by investigating the responsibilities of the function. Make sure it doesn't do too many things, in which case it should be split into smaller and more cohesive functions. Consider the refactoring INTRODUCE PARAMETER OBJECT to encapsulate arguments that refer to the same logical concept. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 14940 @leynos: I've analysed Suggested FixIntroduce a parameter object to encapsulate the template-related parameters: /// Specification for a filter success test case.
struct FilterSuccessSpec<'a> {
/// Template name for registration.
name: &'static str,
/// Template source code.
template: &'static str,
/// Path value to pass to the template.
path: &'a Utf8PathBuf,
}
fn assert_filter_success_with_env<F>(
filter_workspace: Workspace,
home_value: Option<&str>,
spec: FilterSuccessSpec<'_>,
expected: F,
) -> Result<()>
where
F: FnOnce(&Utf8Path) -> String,
{
with_filter_env(filter_workspace, |root, env| {
let home = home_value.map(|value| {
if value.is_empty() {
root.as_str()
} else {
value
}
});
with_clean_env_vars(home, || {
let result = fallible::render(env, spec.name, spec.template, spec.path)
.with_context(|| format!("render template '{}'", spec.name))?;
let expected_value = expected(root);
ensure!(
result == expected_value,
"expected '{expected_value}' but rendered {result}"
);
Ok(())
})
})
}Update the call site (line 445): #[rstest]
fn expanduser_filter() -> Result<()> {
let workspace = fallible::filter_workspace()?;
let path = Utf8PathBuf::from("~/workspace");
assert_filter_success_with_env(
workspace,
Some(""),
FilterSuccessSpec {
name: "expanduser",
template: "{{ path | expanduser }}",
path: &path,
},
|root| root.join("workspace").as_str().to_owned(),
)
}This refactoring:
AI Coding Agent Prompt🐰✨ |
1 participant
Why
Adopting a more opinionated lint configuration surfaces latent defects,
encourages defensive coding, and provides users with better‐documented
APIs. The patch adds the missing documentation and refactors panic-prone
code so the project builds cleanly with the newly denied lints.
What
clippy::*groups,missing_docs, andmissing_crate_level_docs.expectonlyinside tests.
ir/,manifest/) to attach crate-leveldocs.
unwrap/expectin production code withfallible paths and rich error messages; keeps them in tests behind an
explicit allowance.
(e.g. string conversions, thread joins, HTTP fixtures).
eprintln!totracingfor structured logging.previously undocumented.
Outcome
The crate now builds with
cargo clippy --all-targets -- -D warnings,has a clearer public surface, and is less prone to runtime panics or
invalid UTF-8 issues while preserving existing behaviour in tests and
features.