Implement manifest subcommand with stdout streaming and -C dir#250
Implement manifest subcommand with stdout streaming and -C dir#250
Conversation
Add ability for the `netsuke manifest` subcommand to write the generated Ninja file to stdout when given `-` as the output path. This enhancement involves: - Updating CLI docs and help messages to describe `-` as stdout. - Adjusting path resolution to handle the stdout sentinel. - Introducing `write_ninja_stdout` and `is_stdout_path` utilities. - Modifying the runner to detect `-` and stream output instead of writing a file. - Adding comprehensive tests and Cucumber step definitions for this behavior. This makes manifest output more flexible and suitable for piping or integration workflows, improving usability for power users. Co-authored-by: terragon-labs[bot] <terragon-labs[bot]@users.noreply.github.com>
|
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. WalkthroughEnable Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User as User
participant CLI as "CLI Parser"
participant Runner as "Runner"
participant IO as "File I/O / Stdout"
Note over CLI,Runner: User runs `netsuke manifest FILE` (FILE may be "-"), optional `-C` dir
User->>CLI: invoke manifest FILE, optional -C
CLI->>Runner: pass parsed args (FILE, -C)
Runner->>Runner: resolve_output_path(FILE, -C)
alt FILE == "-"
Runner->>IO: write_ninja_stdout(ninja_content)
IO-->>Runner: success / BrokenPipe handled
else FILE is path
Runner->>IO: write_ninja_file(resolved_path, ninja_content)
IO-->>Runner: success / error
end
Runner-->>CLI: exit status and captured stdout/stderr
CLI-->>User: present results
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
Reviewer's GuideImplements a fully functional Sequence diagram for netsuke manifest subcommand with stdout and -C handlingsequenceDiagram
actor User
participant Cli
participant Runner
participant ProcessFileIO
User->>Cli: netsuke manifest FILE [-C DIR]
Cli->>Runner: run(cli)
Runner->>Runner: generate_ninja(cli)
alt FILE is "-"
Runner->>ProcessFileIO: is_stdout_path(file)
ProcessFileIO-->>Runner: true
Runner->>ProcessFileIO: write_ninja_stdout(ninja)
ProcessFileIO-->>Runner: Result
else FILE is regular path
Runner->>ProcessFileIO: is_stdout_path(file)
ProcessFileIO-->>Runner: false
Runner->>Runner: resolve_output_path(cli, file)
Runner->>ProcessFileIO: write_ninja_file(resolved_path, ninja)
ProcessFileIO-->>Runner: Result
end
Runner-->>Cli: Ok(())
Cli-->>User: Command completes
Sequence diagram for build --emit path resolution with -CsequenceDiagram
actor User
participant Cli
participant Runner
participant ProcessFileIO
User->>Cli: netsuke build --emit FILE [-C DIR]
Cli->>Runner: run(cli)
Runner->>Runner: handle_build(cli, args)
Runner->>Runner: generate_ninja(cli)
alt args.emit is Some(FILE)
Runner->>Runner: resolve_output_path(cli, FILE)
Runner->>ProcessFileIO: write_ninja_file(emit_path, ninja)
ProcessFileIO-->>Runner: Result
Runner->>Runner: build_path = emit_path
else args.emit is None
Runner->>ProcessFileIO: create_temp_ninja_file(ninja)
ProcessFileIO-->>Runner: temp_file
Runner->>Runner: build_path = temp_file
end
Runner-->>Cli: Ok(())
Cli-->>User: Build completes
Class diagram for CLI, runner, and IO structures updated in this PRclassDiagram
class Cli {
+directory: Option_PathBuf
+default() Cli
}
class Commands {
<<enum>>
+Build(args: BuildArgs)
+Manifest(file: PathBuf)
+Clean
+Graph
}
class RunnerModule {
+run(cli: Cli) Result
+handle_build(cli: Cli, args: BuildArgs) Result
-resolve_manifest_path(cli: Cli) Utf8PathBuf
-resolve_output_path(cli: Cli, path: Path) PathBuf
}
class ProcessFileIO {
+create_temp_ninja_file(content: NinjaContent) NamedTempFile
+write_ninja_file(path: Path, content: NinjaContent) Result
+write_ninja_stdout(content: NinjaContent) Result
+is_stdout_path(path: Path) bool
}
class BuildArgs {
+emit: Option_PathBuf
}
class NinjaContent {
+as_str() &str
}
Cli --> Commands : parses
RunnerModule --> Cli : uses
RunnerModule --> Commands : dispatches
RunnerModule --> ProcessFileIO : calls
RunnerModule --> NinjaContent : produces
RunnerModule --> BuildArgs : consumes
ProcessFileIO --> NinjaContent : writes
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![codescene-delta-analysis[bot]](https://avatars.githubusercontent.com/in/53921?s=60&v=4){kind=small}
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/manifest_command_steps.rs Comment on lines +129 to +140 fn file_should_not_exist(world: &mut CliWorld, name: String) -> Result<()> {
let temp = world
.temp
.as_ref()
.context("temp dir has not been initialised")?;
ensure!(
!temp.path().join(&name).exists(),
"expected file {} to not exist",
temp.path().join(&name).display()
);
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: tests/steps/manifest_command_steps.rs Comment on file //! Step definitions for `netsuke manifest` behavioural tests.
❌ New issue: String Heavy Function Arguments |
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
…teps - Replace string parameters with newtype wrappers (DirectoryName, FileName, ManifestOutput, OutputFragment) for better type safety and clarity. - Introduce Deref implementations for newtypes to simplify usage. - Add helper functions get_temp_path, assert_output_contains, and assert_file_existence to remove code duplication. - Update step definitions in `manifest_command_steps.rs` to use newtypes and helper functions, improving readability and maintainability. Co-authored-by: terragon-labs[bot] <terragon-labs[bot]@users.noreply.github.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (14)
docs/netsuke-cli-design-document.mddocs/netsuke-design.mddocs/roadmap.mddocs/users-guide.mdsrc/cli.rssrc/runner/mod.rssrc/runner/process/file_io.rstests/assert_cmd_tests.rstests/cli_tests.rstests/cucumber.rstests/features/cli.featuretests/features/manifest_subcommand.featuretests/steps/manifest_command_steps.rstests/steps/mod.rs
🧰 Additional context used
📓 Path-based instructions (5)
**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
**/*.rs: Clippy warnings MUST be disallowed in Rust code.
Fix any warnings emitted during Rust tests in the code itself rather than silencing them.
In Rust, extract meaningfully named helper functions when a function is too long, adhering to separation of concerns and CQRS.
In Rust, group related parameters in meaningfully named structs when a function has too many parameters.
In Rust, consider usingArcto reduce data returned when a function is returning a large error.
Every Rust module must begin with a module level (//!) comment explaining the module's purpose and utility.
Document public APIs in Rust using Rustdoc comments (///) so documentation can be generated withcargo doc.
In Rust, prefer immutable data and avoid unnecessarymutbindings.
In Rust, handle errors with theResulttype instead of panicking where feasible.
Avoidunsafecode in Rust unless absolutely necessary, and document any usage clearly.
In Rust, place function attributes after doc comments.
In Rust, do not usereturnin single-line functions.
In Rust, use predicate functions for conditional criteria with more than two branches.
Lints in Rust must not be silenced except as a last resort.
Lint rule suppressions in Rust must be tightly scoped and include a clear reason.
In Rust, preferexpectoverallow.
In Rust tests, userstestfixtures for shared setup.
In Rust tests, replace duplicated tests with#[rstest(...)]parameterized cases.
In Rust, prefermockallfor mocks and stubs.
In Rust, useconcat!()to combine long string literals rather than escaping newlines with a backslash.
In Rust, prefer single line versions of functions where appropriate (e.g.,pub fn new(id: u64) -> Self { Self(id) }instead of multi-line).
Use NewTypes in Rust to model domain values and eliminate 'integer soup'. Reach fornewt-hypewhen introducing many homogeneous wrappers; add small shims for string-backed wrappers. For path-centric wrappers implementAsRef<Path>along...
Files:
tests/assert_cmd_tests.rstests/cucumber.rstests/cli_tests.rssrc/runner/mod.rssrc/runner/process/file_io.rssrc/cli.rstests/steps/mod.rstests/steps/manifest_command_steps.rs
⚙️ CodeRabbit configuration file
**/*.rs: * Seek to keep the cognitive complexity of functions no more than 9.
- 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()in tests..expect()and.unwrap()are forbidden outside of tests. Errors must be propagated.- 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...
Files:
tests/assert_cmd_tests.rstests/cucumber.rstests/cli_tests.rssrc/runner/mod.rssrc/runner/process/file_io.rssrc/cli.rstests/steps/mod.rstests/steps/manifest_command_steps.rs
docs/**/*.md
📄 CodeRabbit inference engine (AGENTS.md)
docs/**/*.md: When new decisions are made, requirements change, libraries are added/removed, or architectural patterns evolve, proactively update the relevant file(s) in thedocs/directory to reflect the latest state.
Documentation must use en-GB-oxendict spelling and grammar ("-ize" / "-yse" / "-our"), with exception for the LICENSE filename.
docs/**/*.md: Use British English (en-GB) spelling with -ize endings (realize, organization), -lyse endings (analyse, paralyse, catalyse), -our endings (colour, behaviour), -re endings (calibre, centre, fibre), double 'l' (cancelled, counsellor), maintained 'e' (likeable, liveable), and -ogue endings (analogue, catalogue)
Use the Oxford comma in documentation: 'ships, planes, and hovercraft'
Treat company names as collective nouns in documentation: 'Concordat Industries are expanding'
Write headings in sentence case in documentation
Use Markdown headings (#,##,###) in order without skipping levels
Follow markdownlint recommendations for Markdown formatting
Provide code blocks and lists using standard Markdown syntax
Always provide a language identifier for fenced code blocks; useplaintextfor non-code text
Use-as the first level bullet and renumber lists when items change in documentation
Prefer inline links using[text](url)or angle brackets around the URL in documentation
Ensure blank lines before and after bulleted lists and fenced blocks in documentation
Ensure tables have a delimiter line below the header row in documentation
Expand any uncommon acronym on first use in documentation (e.g., Continuous Integration (CI))
Wrap paragraphs at 80 columns in documentation
Wrap code at 120 columns in documentation
Do not wrap tables in documentation
Use GitHub-flavoured numeric footnotes referenced as[^1]in documentation
Number footnotes in order of appearance in the document
Caption every table and every diagram in documentation
Usefor embedding figures with brief alt te...
Files:
docs/roadmap.mddocs/netsuke-design.mddocs/netsuke-cli-design-document.mddocs/users-guide.md
**/*.md
📄 CodeRabbit inference engine (AGENTS.md)
**/*.md: Markdown paragraphs and bullet points must be wrapped at 80 columns.
Code blocks in Markdown must be wrapped at 120 columns.
Tables and headings in Markdown must not be wrapped.
Use dashes (-) for list bullets in Markdown.
Use GitHub-flavoured Markdown footnotes ([^1]) for references and footnotes.
Files:
docs/roadmap.mddocs/netsuke-design.mddocs/netsuke-cli-design-document.mddocs/users-guide.md
⚙️ CodeRabbit configuration file
**/*.md: * Avoid 2nd person or 1st person pronouns ("I", "you", "we")
- Use en-GB-oxendict (-ize / -yse / -our) spelling and grammar
- Headings must not be wrapped.
- Documents must start with a level 1 heading
- Headings must correctly increase or decrease by no more than one level at a time
- Use GitHub-flavoured Markdown style for footnotes and endnotes.
- Numbered footnotes must be numbered by order of appearance in the document.
Files:
docs/roadmap.mddocs/netsuke-design.mddocs/netsuke-cli-design-document.mddocs/users-guide.md
docs/**/{README,readme,GUIDE,guide,*.md}
📄 CodeRabbit inference engine (docs/adr-001-replace-serde-yml-with-serde-saphyr.md)
Document in Netsuke's README and user guide that the project now supports YAML 1.2 (not YAML 1.1) and that duplicate keys in manifests are no longer permitted
Files:
docs/roadmap.mddocs/netsuke-design.mddocs/netsuke-cli-design-document.mddocs/users-guide.md
docs/**/!(README).md
📄 CodeRabbit inference engine (docs/documentation-style-guide.md)
Avoid first and second person personal pronouns outside the
README.mdfile in documentation
Files:
docs/roadmap.mddocs/netsuke-design.mddocs/netsuke-cli-design-document.mddocs/users-guide.md
🧬 Code graph analysis (2)
tests/assert_cmd_tests.rs (2)
src/ninja_gen.rs (1)
join(155-157)test_support/src/lib.rs (1)
fake_ninja(95-127)
src/runner/mod.rs (2)
src/runner/process/file_io.rs (3)
is_stdout_path(16-18)write_ninja_stdout(91-100)write_ninja_file(82-89)src/cli.rs (1)
default(121-134)
🪛 LanguageTool
docs/users-guide.md
[typographical] ~495-~495: The word ‘When’ starts a question. Add a question mark (“?”) at the end of the sentence.
Context: ...--emitpaths are resolved under
. - manifest : Generates the bu...
(WRB_QUESTION_MARK)
[uncategorized] ~497-~497: Loose punctuation mark.
Context: ...lved under <DIR>. - manifest <FILE>: Generates the build.ninja file and wr...
(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~499-~499: Possible missing comma found.
Context: ...e generated Ninja file to stdout (for example netsuke manifest - | sed ...). When `...
(AI_HYDRA_LEO_MISSING_COMMA)
[style] ~499-~499: Consider using the typographical ellipsis character here instead.
Context: ...ted Ninja file to stdout (for example netsuke manifest - | sed ...). When -C/--directory is set, rela...
(ELLIPSIS)
[typographical] ~500-~500: The word ‘When’ starts a question. Add a question mark (“?”) at the end of the sentence.
Context: ... output paths are resolved under <DIR>. - clean: Removes build artefacts by ...
(WRB_QUESTION_MARK)
⏰ 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). (4)
- 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: release / build-macos (x86_64-apple-darwin, macos-15-intel, macos-x86_64, macos-x86_64) / Build macos artefacts
- GitHub Check: release / build-linux (aarch64-unknown-linux-gnu, linux-arm64, arm64) / Build linux artefacts
🔇 Additional comments (26)
docs/roadmap.md (1)
190-192: LGTM!The roadmap accurately reflects the completion of the manifest subcommand implementation, including both disk and stdout output modes.
docs/users-guide.md (2)
494-495: LGTM!The documentation clearly explains that relative
--emitpaths are resolved under the directory specified by-C/--directory.
497-500: LGTM!The documentation clearly describes both stdout streaming via
-and the-Cdirectory resolution behaviour for the manifest subcommand. The ellipsis in the code example is appropriate to indicate piping to further commands.tests/cli_tests.rs (1)
82-88: LGTM!The test case correctly verifies that the CLI parser accepts
-as the file argument for the manifest subcommand, following the established rstest pattern.src/cli.rs (1)
163-164: LGTM!The documentation clearly indicates that
-can be used to stream output to stdout, providing users with a concise usage hint.tests/steps/mod.rs (1)
10-10: LGTM!The new module declaration correctly integrates the manifest command step definitions into the test suite.
tests/features/cli.feature (1)
39-43: LGTM!The scenario comprehensively verifies that the CLI parser correctly handles the stdout streaming syntax for the manifest subcommand.
tests/features/manifest_subcommand.feature (1)
1-22: LGTM!The feature specification provides comprehensive coverage of the manifest subcommand, including the critical verification that streaming to stdout doesn't create a file named
-and that directory collision produces an appropriate error.src/runner/process/file_io.rs (4)
14-18: LGTM!The function correctly identifies the stdout sentinel path. The implementation is simple and the
#[must_use]attribute ensures callers don't ignore the result.
91-100: LGTM!The function correctly handles stdout writing with appropriate BrokenPipe handling (for piping to commands like
head) and ensures the output is flushed. The stdout lock prevents interleaving with other output.
8-8: LGTM!The
std::ioimport is required for the new stdout writing functionality.
161-172: LGTM!The parameterised test cases provide excellent coverage, including the important edge case that
./-is not treated as stdout, preventing potential path confusion.docs/netsuke-cli-design-document.md (1)
940-947: Documentation accurately reflects stdout streaming behavior.The updated text clearly documents that
netsuke manifest -streams to stdout, whilst file paths persist the generated manifest to disk. The wording aligns with the implementation and maintains consistency with the rest of the design document.tests/cucumber.rs (1)
40-43: LGTM: Test world state extended for CLI output capture.The new
command_stdoutandcommand_stderrfields enable Cucumber scenarios to assert on CLI process output. Documentation is clear, visibility is appropriate, and theOption<String>type correctly handles scenarios without CLI invocation.docs/netsuke-design.md (3)
965-968: Documentation correctly describes manifest stdout streaming.The command description clearly states that
-streams to stdout rather than persisting to disk. This aligns with the implementation and maintains consistency across documentation files.
2013-2013: Manifest stdout streaming documented in command behavior section.The addition clearly states that
-as the file argument streams to stdout. Placement in section 8.3 (Command Behaviour) is appropriate and the wording is concise.
2024-2028: Path resolution behavior clearly documented.The explanation of
-Cpath resolution is clear: relative output paths (formanifestandbuild --emit) are resolved under the specified directory whilst absolute paths remain unchanged. The reference to test coverage provides confidence in the implementation.tests/assert_cmd_tests.rs (2)
63-91: LGTM: Test correctly verifies -C path resolution for manifest output.The test properly exercises the
-Cdirectory option and confirms:
- Relative output path
out.ninjais resolved under the-Cdirectory- Output is not written outside the
-CdirectoryThe dual assertions (positive and negative) provide strong confidence in the path resolution logic.
121-157: LGTM: Test correctly verifies -C path resolution for build --emit.The test properly exercises the
-Cdirectory option forbuild --emitand confirms:
- Relative emit path is resolved under the
-Cdirectory- Output is not written outside the
-CdirectoryThe use of
fake_ninjaappropriately isolates the test from external dependencies whilst still validating the path resolution behavior.src/runner/mod.rs (3)
94-99: LGTM: Manifest command correctly handles stdout streaming and file output.The implementation properly:
- Detects stdout sentinel with
is_stdout_path- Streams to stdout when appropriate
- Resolves relative paths against
-Cdirectory for file output- Delegates I/O to process module helpers
The logic is clear and maintains separation of concerns.
141-143: Build emit path resolution consistent with manifest handling.The code correctly applies
resolve_output_pathto emit paths, ensuring-Cdirectory handling is consistent across commands. The ownedPathBufapproach is appropriate given that relative paths are transformed.
306-327: LGTM: Unit tests comprehensively cover path resolution scenarios.The parameterized test using
rstestcleanly covers the three key cases:
- No directory configured (path unchanged)
- Directory configured with relative path (path joined)
- Directory configured with absolute path (absolute path unchanged)
Test structure is clear and assertions are straightforward.
tests/steps/manifest_command_steps.rs (4)
115-147: LGTM: Helper functions effectively reduce duplication in step definitions.The three helpers (
get_temp_path,assert_output_contains,assert_file_existence) extract common patterns from the step functions, improving maintainability. Each returnsResultwith contextual error messages that will aid debugging when tests fail.
149-161: Given step correctly initialises workspace and resets state.The step properly:
- Creates an isolated temporary directory
- Copies the minimal manifest fixture
- Resets all scenario state including the new
command_stdoutandcommand_stderrfieldsExplicit state reset at line 156-159 prevents test pollution between scenarios.
163-171: Given step correctly creates named directories in the workspace.The step properly extracts the directory name via the
DirectoryNamenewtype and creates it within the temporary workspace. Error context clearly identifies the directory path being created.
173-219: LGTM: Step functions correctly implement Cucumber test scenarios.The when step (lines 173-195) properly:
- Invokes the netsuke binary via
assert_cmd- Captures stdout, stderr, and exit status
- Stores all outputs in world state for subsequent assertions
The then steps (lines 197-219) cleanly delegate to the helper functions introduced earlier, maintaining consistency and readability. The destructuring pattern
let TypeName(inner) = param;followed byhelper(world, inner.as_str())is uniform across all assertions.
|
@sourcery-ai review |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
- Added handling of broken pipe errors when writing ninja manifest to stdout in `write_ninja_stdout` to avoid failure on broken pipe. - Refactored writing and flushing logic to tolerate broken pipe errors. - Updated tests to cover manifest streaming with working directory changes (-C). - Improved path handling and code cleanup in runner module. - Upgraded derive_more crate and simplified string wrapper types in test steps. - Updated windows-sys and derive_more dependencies. This improves robustness of manifest command output and adds tests for streaming output with changed directories. Co-authored-by: terragon-labs[bot] <terragon-labs[bot]@users.noreply.github.com>
|
@coderabbitai Have the following now been resolved? |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
This comment was marked as resolved.
This comment was marked as resolved.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/assert_cmd_tests.rs (1)
138-142: Extract PATH construction into a helper function.The PATH construction logic is duplicated between
build_with_emit_writes_fileandbuild_with_emit_resolves_output_relative_to_directory. Extract this into a helper.🔎 Suggested refactor
Add a helper function at module level:
fn path_with_fake_ninja(ninja_dir: &tempfile::TempDir) -> Result<std::ffi::OsString> { let original = std::env::var_os("PATH").unwrap_or_default(); std::env::join_paths( std::iter::once(ninja_dir.path().to_path_buf()) .chain(std::env::split_paths(&original)), ) .context("construct PATH with fake ninja") }Then update both tests:
- let original = std::env::var_os("PATH").unwrap_or_default(); - let path = std::env::join_paths( - std::iter::once(ninja_dir.path().to_path_buf()).chain(std::env::split_paths(&original)), - ) - .context("construct PATH with fake ninja")?; + let path = path_with_fake_ninja(&ninja_dir)?;Also applies to: 168-172
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (5)
Cargo.tomlsrc/runner/mod.rssrc/runner/process/file_io.rstests/assert_cmd_tests.rstests/steps/manifest_command_steps.rs
🧰 Additional context used
📓 Path-based instructions (2)
**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
**/*.rs: Clippy warnings MUST be disallowed in Rust code.
Fix any warnings emitted during Rust tests in the code itself rather than silencing them.
In Rust, extract meaningfully named helper functions when a function is too long, adhering to separation of concerns and CQRS.
In Rust, group related parameters in meaningfully named structs when a function has too many parameters.
In Rust, consider usingArcto reduce data returned when a function is returning a large error.
Every Rust module must begin with a module level (//!) comment explaining the module's purpose and utility.
Document public APIs in Rust using Rustdoc comments (///) so documentation can be generated withcargo doc.
In Rust, prefer immutable data and avoid unnecessarymutbindings.
In Rust, handle errors with theResulttype instead of panicking where feasible.
Avoidunsafecode in Rust unless absolutely necessary, and document any usage clearly.
In Rust, place function attributes after doc comments.
In Rust, do not usereturnin single-line functions.
In Rust, use predicate functions for conditional criteria with more than two branches.
Lints in Rust must not be silenced except as a last resort.
Lint rule suppressions in Rust must be tightly scoped and include a clear reason.
In Rust, preferexpectoverallow.
In Rust tests, userstestfixtures for shared setup.
In Rust tests, replace duplicated tests with#[rstest(...)]parameterized cases.
In Rust, prefermockallfor mocks and stubs.
In Rust, useconcat!()to combine long string literals rather than escaping newlines with a backslash.
In Rust, prefer single line versions of functions where appropriate (e.g.,pub fn new(id: u64) -> Self { Self(id) }instead of multi-line).
Use NewTypes in Rust to model domain values and eliminate 'integer soup'. Reach fornewt-hypewhen introducing many homogeneous wrappers; add small shims for string-backed wrappers. For path-centric wrappers implementAsRef<Path>along...
Files:
src/runner/process/file_io.rstests/steps/manifest_command_steps.rssrc/runner/mod.rstests/assert_cmd_tests.rs
⚙️ CodeRabbit configuration file
**/*.rs: * Seek to keep the cognitive complexity of functions no more than 9.
- 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()in tests..expect()and.unwrap()are forbidden outside of tests. Errors must be propagated.- 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...
Files:
src/runner/process/file_io.rstests/steps/manifest_command_steps.rssrc/runner/mod.rstests/assert_cmd_tests.rs
Cargo.toml
📄 CodeRabbit inference engine (AGENTS.md)
Cargo.toml: In RustCargo.toml, use explicit version ranges and keep dependencies up-to-date.
All crate versions specified inCargo.tomlmust use SemVer-compatible caret requirements (e.g.,some-crate = "1.2.3") to allow safe non-breaking updates.
The use of wildcard (*) or open-ended inequality (>=) version requirements inCargo.tomlis strictly forbidden.
Tilde requirements (~) inCargo.tomlshould only be used where a dependency must be locked to patch-level updates for a specific, documented reason.
Files:
Cargo.toml
🧬 Code graph analysis (2)
src/runner/mod.rs (2)
src/runner/process/file_io.rs (3)
is_stdout_path(16-18)write_ninja_stdout(111-117)write_ninja_file(82-89)src/cli.rs (1)
default(121-134)
tests/assert_cmd_tests.rs (1)
test_support/src/lib.rs (1)
fake_ninja(95-127)
🔍 Remote MCP Deepwiki
Based on the context retrieved from the Netsuke repository, here is the additional relevant information for reviewing this PR:
Context on Manifest vs. Emit Functionality
The manifest subcommand is a standalone command for writing the Ninja manifest to a specified file without invoking Ninja, while emit functionality is an option within the build command that allows you to retain the generated Ninja file at a specified path while still proceeding with the build process. Both the manifest subcommand and the emit functionality utilize the common runner::generate_ninja function to produce the Ninja build file, which aligns with the PR's approach of using the same path resolution logic (resolve_output_path) for both features.
Existing Patterns for Stdout and Path Handling
The codebase has established patterns that are relevant to this PR's implementation:
-
Stdout Output Handling: The
run_ninjafunction configures theCommandto pipestdoutandstderr, and thespawn_and_stream_outputfunction creates separate threads to read from itsstdoutandstderrpipes, writing output to the parent process'sstdoutandstderrrespectively. This provides context for the PR's newwrite_ninja_stdoututility function approach. -
Path Resolution Patterns: The
resolve_manifest_pathfunction determines the path to the Netsukefile, taking into account a--directoryCLI option which, if present, is used as the base for joining with the manifest file name. The PR's newresolve_output_pathfunction extends this established pattern to output files. -
UTF-8 Path Handling: The codebase uses
canonicalize_utf8_pathto convert paths to their canonical, absolute representation while ensuring all paths are valid UTF-8, which is consistent with how the PR handles path operations.
Key Code Review Considerations
Based on this context, reviewers should verify that:
- The new
is_stdout_path(path: &Path) -> boolcheck correctly identifies the-sentinel within the existing Netsuke design patterns - The
resolve_output_pathfunction properly integrates with the established-C/--directoryhandling approach used elsewhere in the codebase (e.g., inresolve_manifest_path) - BrokenPipe error handling in
write_ninja_stdoutis consistent with how the existing codebase manages subprocess output interruptions,
🔇 Additional comments (17)
Cargo.toml (2)
148-148: Inconsistency flagged: AI summary mentions "two separate sections".The AI summary states the dependency was "added in two separate [dev-dependencies] sections", but only one addition appears at line 148 in the main
[dev-dependencies]section. The[target.'cfg(unix)'.dev-dependencies]section at line 151 shows no changes.
148-148: Remove the overengineering concern—derive_more is in active use.The search results confirm that
derive_moreis imported and actively used intests/steps/manifest_command_steps.rs. TheFromandDerefderives are applied to theDirectoryNameandFileNamenewtype wrappers, which are then used throughout the test step definitions.The version format
"1"is SemVer-compliant (equivalent to^1.x.x). The dev-dependency is necessary for this particular test code pattern.Note: The reviewer's concern about overengineering relates to whether the newtype wrapper pattern itself is the right design approach for tests, not whether this dependency is unused bloat. If the test refactor adopts a simpler approach without newtype wrappers, revisit this dependency.
src/runner/mod.rs (4)
92-105: LGTM!The manifest command handling correctly branches between stdout streaming and file output. The use of
is_stdout_pathprovides clear intent, andresolve_output_pathis applied only for file paths. TheCow<Path>return type fromresolve_output_pathis correctly consumed via.as_ref().
138-149: LGTM!The emit path resolution now correctly applies
resolve_output_pathto honour the-Cdirectory option. TheCow<Path>is directly assigned tobuild_path, avoiding unnecessary allocation when the path is absolute.
289-304: LGTM!The
resolve_output_pathfunction correctly implements-Csemantics: relative paths are joined with the configured directory whilst absolute paths pass through unchanged. TheCow<'a, Path>return type avoids allocation when no transformation is needed. Documentation clearly explains the behaviour.
306-328: LGTM!The parameterised test covers the three key cases:
- No directory configured (path unchanged)
- Relative path with directory (joined)
- Absolute path with directory (unchanged)
Good use of
rstestper coding guidelines.src/runner/process/file_io.rs (4)
14-18: LGTM!The
is_stdout_pathfunction correctly identifies the-sentinel by comparing the OS string directly. This is the appropriate check since-as a path should only match the literal dash, not a file named-in a subdirectory.
91-109: Well-structured BrokenPipe handling.The
is_broken_pipepredicate satisfies the coding guideline requiring predicates for conditionals with more than two branches. The helper functionswrite_all_ignoring_broken_pipeandflush_ignoring_broken_pipeprovide consistent BrokenPipe handling for both write and flush operations, addressing the previously flagged inconsistency.
111-117: LGTM!The
write_ninja_stdoutfunction correctly composes the helpers to write content to stdout with consistent BrokenPipe handling. The contextual error messages are informative.
178-189: Good edge-case coverage.The test correctly verifies:
"-"→true(stdout sentinel)"out.ninja"→false(normal file)"./-"→false(relative path to file literally named-)The
./-case is particularly important to ensure a file named-in the current directory is not mistaken for stdout.tests/steps/manifest_command_steps.rs (3)
16-34: Good use ofderive_moreto reduce boilerplate.The
cucumber_string_wrapper!macro cleanly generates theFrom<String>andDerefimplementations via derive_more, with onlyFromStrrequiring manual implementation (sinceStringdoes not implementFromStr). This addresses the previous review suggestion effectively.
36-68: LGTM!The helper functions cleanly extract common patterns:
get_temp_path: retrieves temp directory with proper error contextassert_output_contains: validates output fragments with clear error messagesassert_file_existence: checks file presence with parametrised expectationThis reduces duplication across step definitions.
118-140: LGTM!The assertion step definitions are concise and correctly delegate to the helper functions. The newtype destructuring pattern
let FileName(name) = name;is clear.tests/assert_cmd_tests.rs (4)
34-61: LGTM!The test correctly verifies:
- Command succeeds with
-argument- Stdout contains Ninja content markers (
ruleandbuild)- No file named
-is createdThe enhanced content validation addresses the previous review comment.
63-91: LGTM!The test correctly verifies that
-C workcauses the manifest output to be written inside theworkdirectory rather than the current directory. Both positive and negative assertions ensure the path resolution is working correctly.
93-128: LGTM!This test addresses the previous review suggestion to verify the interaction between
-Cand-(stdout streaming). It correctly ensures no file named-is created in either the working directory or the-Cdirectory.
158-193: LGTM!The test correctly verifies that
build --emitwith-Cwrites the emitted manifest to the-Cdirectory rather than the current working directory. The fake ninja setup ensures the build command completes successfully.
…unction Extracted duplicated command execution logic in manifest_command_steps.rs into a helper function run_netsuke_and_record. Also introduced a helper path_with_fake_ninja in assert_cmd_tests.rs to simplify PATH setup. These changes reduce code duplication and improve test code maintainability. Co-authored-by: terragon-labs[bot] <terragon-labs[bot]@users.noreply.github.com>
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
@coderabbitai Have the following now been resolved? |
This comment was marked as resolved.
This comment was marked as resolved.
- Added new test_support/netsuke.rs module for invoking netsuke binary with assert_cmd - Replaced direct assert_cmd usage in manifest_command_steps.rs with test_support netsuke helpers - Simplified step definitions by using plain String instead of wrapper newtypes - Updated Cargo.toml files to add assert_cmd dependency to test_support workspace - Cleaned up Cargo.lock to remove derive_more dependency This refactor improves test code reuse and clarity by centralizing netsuke command invocation logic. Co-authored-by: terragon-labs[bot] <terragon-labs[bot]@users.noreply.github.com>
|
@coderabbitai Have the following now been resolved? ❌ Outstanding Issues (2/5) NOT RESOLVED - The wrapper types (DirectoryName, FileName, ManifestOutput, OutputFragment) remain in tests/steps/manifest_command_steps.rs, though they've been simplified using a cucumber_string_wrapper! macro. Recommendation: Since cucumber can work directly with String parameters (which already implement FromStr), you can remove these wrappers entirely and accept String directly in the step functions. PARTIALLY RESOLVED - A helper function run_netsuke_and_record now exists and captures stdout/stderr, but it's private to the manifest_command_steps module. Recommendation: To make it reusable by other command steps, either: |
|
@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/manifest_command_steps.rs Comment on file //! Step definitions for `netsuke manifest` behavioural tests.
❌ New issue: String Heavy Function Arguments |
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
…larity - Introduced CommandOutput struct to encapsulate command output data. - Simplified function signatures by using &str instead of Option<&String> and avoiding unnecessary boxing. - Added clippy expect annotations to allow owned String args required by cucumber steps. - Cleaned up passing of string references and ownership for better readability and efficiency. Co-authored-by: terragon-labs[bot] <terragon-labs[bot]@users.noreply.github.com>
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/manifest_command_steps.rs Comment on file //! Step definitions for `netsuke manifest` behavioural tests.
❌ New issue: String Heavy Function Arguments |
This comment was marked as resolved.
This comment was marked as resolved.
- Introduced OutputType enum to represent stdout and stderr output streams - Updated assert_output_contains to accept OutputType instead of string - Improved error messages to use OutputType formatting - Updated test step definitions to use new enum for clarity and maintainability Co-authored-by: terragon-labs[bot] <terragon-labs[bot]@users.noreply.github.com>
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/assert_cmd_tests.rs: What lead to degradation?The module contains 4 functions with similar structure: build_with_emit_resolves_output_relative_to_directory,build_with_emit_writes_file,manifest_subcommand_resolves_output_relative_to_directory,manifest_subcommand_writes_file 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.
Refactored the assert_cmd tests by introducing helper functions to set up test workspaces and create the command instances. This removes repeated code for temp directory setup, manifest copying, and command creation, making tests clearer and more maintainable. Co-authored-by: terragon-labs[bot] <terragon-labs[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/assert_cmd_tests.rs (1)
47-192: Consider rstest parameterization to reduce test duplication.Several test pairs follow similar patterns:
- Lines 48-58 and 150-166: write file tests for manifest vs build --emit
- Lines 89-110 and 168-192: relative path resolution tests for manifest vs build --emit
Evaluate whether parameterizing these with
#[rstest]would improve maintainability without sacrificing clarity. Based on coding guidelines preference for parameterized cases.Example parameterization approach
#[rstest] #[case("manifest", &["manifest", "output.ninja"], "output.ninja", false)] #[case("build_emit", &["build", "--emit", "output.ninja"], "output.ninja", true)] fn subcommand_writes_file( #[case] name: &str, #[case] args: &[&str], #[case] output_file: &str, #[case] needs_ninja: bool, ) -> Result<()> { let temp = setup_simple_workspace(&format!("{} test", name))?; let output = temp.path().join(output_file); let path = if needs_ninja { let (ninja_dir, _) = fake_ninja(0u8)?; path_with_fake_ninja(&ninja_dir)? } else { OsString::from("") }; let mut cmd = create_netsuke_command(temp.path(), path)?; cmd.args(args).assert().success(); ensure!(output.exists(), "{} should create output file", name); Ok(()) }Only proceed if this improves rather than obscures test intent.
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
tests/assert_cmd_tests.rs
🧰 Additional context used
📓 Path-based instructions (1)
**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
**/*.rs: Clippy warnings MUST be disallowed in Rust code.
Fix any warnings emitted during Rust tests in the code itself rather than silencing them.
In Rust, extract meaningfully named helper functions when a function is too long, adhering to separation of concerns and CQRS.
In Rust, group related parameters in meaningfully named structs when a function has too many parameters.
In Rust, consider usingArcto reduce data returned when a function is returning a large error.
Every Rust module must begin with a module level (//!) comment explaining the module's purpose and utility.
Document public APIs in Rust using Rustdoc comments (///) so documentation can be generated withcargo doc.
In Rust, prefer immutable data and avoid unnecessarymutbindings.
In Rust, handle errors with theResulttype instead of panicking where feasible.
Avoidunsafecode in Rust unless absolutely necessary, and document any usage clearly.
In Rust, place function attributes after doc comments.
In Rust, do not usereturnin single-line functions.
In Rust, use predicate functions for conditional criteria with more than two branches.
Lints in Rust must not be silenced except as a last resort.
Lint rule suppressions in Rust must be tightly scoped and include a clear reason.
In Rust, preferexpectoverallow.
In Rust tests, userstestfixtures for shared setup.
In Rust tests, replace duplicated tests with#[rstest(...)]parameterized cases.
In Rust, prefermockallfor mocks and stubs.
In Rust, useconcat!()to combine long string literals rather than escaping newlines with a backslash.
In Rust, prefer single line versions of functions where appropriate (e.g.,pub fn new(id: u64) -> Self { Self(id) }instead of multi-line).
Use NewTypes in Rust to model domain values and eliminate 'integer soup'. Reach fornewt-hypewhen introducing many homogeneous wrappers; add small shims for string-backed wrappers. For path-centric wrappers implementAsRef<Path>along...
Files:
tests/assert_cmd_tests.rs
⚙️ CodeRabbit configuration file
**/*.rs: * Seek to keep the cognitive complexity of functions no more than 9.
- 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()in tests..expect()and.unwrap()are forbidden outside of tests. Errors must be propagated.- 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...
Files:
tests/assert_cmd_tests.rs
🧬 Code graph analysis (1)
tests/assert_cmd_tests.rs (1)
test_support/src/lib.rs (1)
fake_ninja(96-128)
⏰ 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 (x86_64-pc-windows-msvc, amd64, x64, windows-x86_64) / Build windows artefacts
- GitHub Check: release / build-macos (x86_64-apple-darwin, macos-15-intel, macos-x86_64, macos-x86_64) / Build macos artefacts
- GitHub Check: release / build-windows (aarch64-pc-windows-msvc, arm64, arm64, windows-aarch64) / Build windows artefacts
- GitHub Check: Sourcery review
- GitHub Check: build-test (ubuntu-latest, stable)
…ed setup - Upgrade assert_cmd from 2.0.17 to 2.1.1 and remove doc-comment dependency. - Use explicit netsuke executable path resolution instead of cargo_bin helper. - Refactor tests to use parameterized rstest for subcommands writing files and relative output paths. - Add helper functions to setup workspaces and manage PATH for fake ninja binaries. - Simplify test assertions and improve test coverage on manifest and build --emit subcommands. - Consolidate redundant tests and improve test clarity and maintainability. Co-authored-by: terragon-labs[bot] <terragon-labs[bot]@users.noreply.github.com>
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (3)
test_support/Cargo.toml (1)
17-17: LGTM: Dependency placement and version are correct.The version
"2.0"follows implicit semver caret conventions. Placingassert_cmdin[dependencies](not[dev-dependencies]) is appropriate because this test support crate exposes command execution utilities in its public API.Note: This resolves the past review feedback requesting the change from
"2.0.17"to"2.0".tests/assert_cmd_tests.rs (2)
16-53: LGTM: Helper functions are well-documented and correctly implemented.All helper functions now have clear doc comments addressing the past review feedback. Error handling uses
.context()throughout, and thecargo_bin_cmd!macro is used appropriately.Note: This resolves past review comments requesting doc comments on helpers.
224-251: LGTM: Test covers the -C with stdout streaming edge case.This test addresses the past review feedback by explicitly verifying that
manifest -combined with-Cstreams to stdout without creating a file named "-" in either the working directory or the-Cdirectory.Note: This resolves past review comments requesting coverage of the
-Cand-interaction.
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (4)
Cargo.tomltest_support/Cargo.tomltest_support/src/netsuke.rstests/assert_cmd_tests.rs
🧰 Additional context used
📓 Path-based instructions (2)
**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
**/*.rs: Clippy warnings MUST be disallowed in Rust code.
Fix any warnings emitted during Rust tests in the code itself rather than silencing them.
In Rust, extract meaningfully named helper functions when a function is too long, adhering to separation of concerns and CQRS.
In Rust, group related parameters in meaningfully named structs when a function has too many parameters.
In Rust, consider usingArcto reduce data returned when a function is returning a large error.
Every Rust module must begin with a module level (//!) comment explaining the module's purpose and utility.
Document public APIs in Rust using Rustdoc comments (///) so documentation can be generated withcargo doc.
In Rust, prefer immutable data and avoid unnecessarymutbindings.
In Rust, handle errors with theResulttype instead of panicking where feasible.
Avoidunsafecode in Rust unless absolutely necessary, and document any usage clearly.
In Rust, place function attributes after doc comments.
In Rust, do not usereturnin single-line functions.
In Rust, use predicate functions for conditional criteria with more than two branches.
Lints in Rust must not be silenced except as a last resort.
Lint rule suppressions in Rust must be tightly scoped and include a clear reason.
In Rust, preferexpectoverallow.
In Rust tests, userstestfixtures for shared setup.
In Rust tests, replace duplicated tests with#[rstest(...)]parameterized cases.
In Rust, prefermockallfor mocks and stubs.
In Rust, useconcat!()to combine long string literals rather than escaping newlines with a backslash.
In Rust, prefer single line versions of functions where appropriate (e.g.,pub fn new(id: u64) -> Self { Self(id) }instead of multi-line).
Use NewTypes in Rust to model domain values and eliminate 'integer soup'. Reach fornewt-hypewhen introducing many homogeneous wrappers; add small shims for string-backed wrappers. For path-centric wrappers implementAsRef<Path>along...
Files:
test_support/src/netsuke.rstests/assert_cmd_tests.rs
⚙️ CodeRabbit configuration file
**/*.rs: * Seek to keep the cognitive complexity of functions no more than 9.
- 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()in tests..expect()and.unwrap()are forbidden outside of tests. Errors must be propagated.- 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...
Files:
test_support/src/netsuke.rstests/assert_cmd_tests.rs
Cargo.toml
📄 CodeRabbit inference engine (AGENTS.md)
Cargo.toml: In RustCargo.toml, use explicit version ranges and keep dependencies up-to-date.
All crate versions specified inCargo.tomlmust use SemVer-compatible caret requirements (e.g.,some-crate = "1.2.3") to allow safe non-breaking updates.
The use of wildcard (*) or open-ended inequality (>=) version requirements inCargo.tomlis strictly forbidden.
Tilde requirements (~) inCargo.tomlshould only be used where a dependency must be locked to patch-level updates for a specific, documented reason.
Files:
Cargo.toml
🧬 Code graph analysis (1)
tests/assert_cmd_tests.rs (1)
test_support/src/lib.rs (1)
fake_ninja(96-128)
🪛 GitHub Actions: Release Dry Run
Cargo.toml
[error] 1-1: Cargo.toml parse failure. Could not read manifest. Command used: python3 .github/workflows/scripts/read_manifest.py --manifest-path "$manifest_path" name 2>/tmp/name.err
test_support/Cargo.toml
[error] 1-1: Cargo.toml parse failure. Could not read manifest. Command used: python3 .github/workflows/scripts/read_manifest.py --manifest-path "$manifest_path" name 2>/tmp/name.err
🔍 Remote MCP
Let me search the web for information about netsuke and this PR:The web search results did not return information about the netsuke project itself. Let me search more specifically:Based on the comprehensive context provided in the PR summary and AI analysis, I can now synthesize the relevant facts for the code review without needing additional web sources. The provided documentation is sufficiently detailed.
Summary of Relevant Context for PR #250 Review
Project Purpose & Architecture
The PR implements a netsuke manifest <FILE> subcommand for a Rust-based build tool that generates Ninja build manifests. Key architectural elements relevant to this review:
- Core Functionality: The netsuke CLI generates Ninja build files (manifest format) and can now either persist them to disk or stream them to stdout
- Module Structure:
src/cli.rs- CLI parsing and command definitionssrc/runner/mod.rs- Command execution logic with path resolutionsrc/runner/process/file_io.rs- File I/O utilities (newly extended)- Test modules with integration (assert_cmd) and behavior-driven (cucumber) testing
Key Technical Changes to Review
1. Public API Additions [::Deepwiki::]
is_stdout_path(path: &Path) -> bool- detects "-" sentinel valuewrite_ninja_stdout(content: &NinjaContent) -> AnyResult<()>- streams to stdout with BrokenPipe handlingresolve_output_path(cli, path)- resolves relative paths against-C/--directoryparameter
2. Behavior Changes
- Manifest command now routes output based on file argument: "-" triggers stdout streaming, otherwise disk write
--emitflag uses sameresolve_output_path()logic for directory-aware resolution- Success messaging preserved regardless of output destination
- No file creation when streaming to stdout (important for test verification)
3. Error Handling Improvements
- BrokenPipe errors now consistently ignored on both
write()andflush()operations to stdout - Refactored multi-branch match into helpers (
is_broken_pipe,write_all_ignoring_broken_pipe,flush_ignoring_broken_pipe) - Contextual error messages for path resolution failures
Test Coverage & Verification Points
Integration Tests [::Deepwiki::]
subcommand_writes_file- verifies disk output with fake Ninja binary supportmanifest_subcommand_streams_to_stdout_when_dash- confirms stdout stream and no file "-" creationsubcommand_resolves_output_relative_to_directory- validates-Cdirectory resolution for both manifest and--emitmanifest_subcommand_streams_to_stdout_with_directory- combined-Cand stdout streaming
Unit Tests
resolve_output_pathbehavior with relative/absolute pathsis_stdout_pathdetectionwrite_ninja_stdoutstdout handling including broken-pipe scenarios
Test Helpers Refactored
setup_simple_workspace()andsetup_workspace_with_subdir()for test setuppath_with_fake_ninja()for PATH override in tests- Parameterized test cases via
WritesFileCaseandRelativeOutputCaseenums
Documentation Updates
Design and user-facing documentation expanded to clarify:
-as stdout sentinel (e.g.,netsuke manifest - | ...)-C/--directorypath resolution semantics for relative output paths- No underlying behavior changes—documentation clarifies existing intent
Code Review Risk Areas
- Broken-Pipe Handling - Critical for robustness. Verify both
write()andflush()consistently ignore BrokenPipe - Path Resolution - Ensure relative path resolution respects directory context in all code paths (manifest and emit)
- Test Coverage - Integration tests should confirm no file named "-" created in any scenario (including with
-C) - String Parameters in Tests - Codescene metrics noted high string-argument count; refactoring with
CommandOutputstruct andOutputTypeenum addresses this
🔇 Additional comments (6)
test_support/src/netsuke.rs (2)
31-40: LGTM: Struct documentation is complete and clear.The
NetsukeRunstruct and its fields are properly documented with rustdoc comments. The minimalDebugderive is appropriate for a test utility type.
42-64: LGTM: Function is well-documented and correctly implemented.The
run_netsuke_infunction properly documents its behaviour and errors. ClearingPATHensures test isolation, and usingfrom_utf8_lossyis appropriate for capturing potentially non-UTF-8 output in tests.tests/assert_cmd_tests.rs (3)
95-116: LGTM: Parametrised test follows best practices.The test correctly uses
rstestparametrisation, helper functions, and propagates errors withensure!rather than panicking. This addresses past review feedback.
118-139: LGTM: Test correctly verifies stdout streaming behaviour.The test properly uses helper functions and checks both that stdout contains expected content and that no file named "-" is created. Error handling is consistent with project patterns.
188-222: LGTM: Test correctly validates -C directory resolution.The parametrised test properly verifies that output files are resolved relative to the
-Cdirectory, checking both positive (file exists in workdir) and negative (file doesn't exist outside workdir) conditions.test_support/Cargo.toml (1)
17-17: No action required. The manifest at test_support/Cargo.toml parses successfully. Theassert_cmd = "2.0"dependency is syntactically correct TOML with proper formatting and placement in[dependencies].Likely an incorrect or invalid review comment.
…nifest files - Introduced the WritesFileCase and RelativeOutputCase enums to describe variants of tests related to subcommands that write manifest files. - Added descriptive helper methods to these enums for context, CLI args, output filename, ninja dependency, and expected assertions. - Created new detailed tests checking output file creation and path resolution with -C option. - Added netsuke_executable helper to test_support for locating the test binary. - Updated assert_cmd dependency to exact 2.0.0 version in Cargo.toml files. Co-authored-by: terragon-labs[bot] <terragon-labs[bot]@users.noreply.github.com>
leynos
changed the title
1 participant
Summary
-.-C/--directoryoption.Changes
CLI
netsuke manifest <FILE><FILE>is-, stream manifest to stdout; otherwise write to the specified path.-streams to stdout.Runner
-), write to stdout.resolve_output_path(cli, path)to apply-C/--directorysemantics for output paths.resolve_output_pathfor--emitas well.IO Layer
is_stdout_path(path: &Path) -> boolsentinel checker (dash -> stdout).write_ninja_stdout(content)to stream manifest to stdout with proper error handling and flush.Tests
resolve_output_pathto ensure output paths are properly resolved relative to the configured directory.is_stdout_pathto detect-sentinel.write_ninja_stdoutto verify stdout streaming behavior (including broken pipe handling).--C <dir>andout.ninjawrites under<dir>Documentation
-in addition to a file path.-and updated behavior description about working directory resolution for output paths.-for manifest and how-C/--directoryaffects manifest output paths.Tests and CI
Test Plan
-C/--directoryfor manifest and build emits.-.How to Test Locally
If you’d like, I can add a short section to the docs with a quick example usage of
netsuke manifest -and a note on directory resolution for the -C option.🌿 Generated by Terry
ℹ️ Tag @terragon-labs to ask questions and address PR feedback
📎 Task: https://www.terragonlabs.com/task/5549d050-83cc-4282-8107-21683f8008c2