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:
Caution Review failedFailed to post review comments 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
WalkthroughIntroduce StdlibState to track impurity; add platform-aware Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor T as Template Renderer
participant E as MiniJinja Environment
participant S as Stdlib::command
participant Sh as Platform Shell
participant P as Child Process
T->>E: apply filter "shell" / "grep"
E->>S: execute_shell/execute_grep(value, args)
note right of S #e6f4ff: Set impurity flag (StdlibState)
S->>Sh: spawn shell/program with quoted args
Sh->>P: run command, pipe stdin
alt success
P-->>S: stdout bytes
S-->>E: Value from bytes
E-->>T: Rendered output
else failure
P-->>S: non-zero exit or IO error
S-->>E: Template Error (InvalidOperation)
E-->>T: Propagate error
end
sequenceDiagram
autonumber
actor T as Template Renderer
participant E as MiniJinja Environment
participant N as Stdlib::network::fetch
participant C as Cache (.netsuke/fetch)
participant H as HTTP Client (ureq)
T->>E: call fetch(url, cache?, cache_dir?)
E->>N: fetch(url, kwargs)
alt cache enabled
N->>C: read(key=sha256(url))
alt hit
C-->>N: cached bytes
N-->>E: Value (string or bytes)
else miss
N->>H: GET url
H-->>N: response bytes or error
alt success
N->>C: write(key, bytes)
C-->>N: ok
N-->>E: Value
else failure
N-->>E: Template Error (InvalidOperation)
end
end
else cache disabled
N->>H: GET url
H-->>N: bytes or error
N-->>E: Value or Error
end
E-->>T: Result or Error
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
Pre-merge checks and finishing touches and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
![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
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (16)
Cargo.toml(1 hunks)docs/netsuke-design.md(1 hunks)docs/roadmap.md(2 hunks)src/manifest.rs(1 hunks)src/stdlib/command.rs(1 hunks)src/stdlib/mod.rs(3 hunks)src/stdlib/network.rs(1 hunks)tests/cucumber.rs(3 hunks)tests/features/stdlib.feature(1 hunks)tests/std_filter_tests.rs(1 hunks)tests/std_filter_tests/command_filters.rs(1 hunks)tests/std_filter_tests/hash_filters.rs(4 hunks)tests/std_filter_tests/io_filters.rs(4 hunks)tests/std_filter_tests/network_functions.rs(1 hunks)tests/std_filter_tests/support.rs(2 hunks)tests/steps/stdlib_steps.rs(6 hunks)
🧰 Additional context used
📓 Path-based instructions (6)
Cargo.toml
📄 CodeRabbit inference engine (AGENTS.md)
Cargo.toml: Use explicit version ranges in Cargo.toml and keep dependencies up to date
All dependencies must use SemVer caret requirements (e.g., "1.2.3")
Forbid wildcard (*) and open-ended (>=) version specifiers; use ~ only with documented, specific reasons
Files:
Cargo.toml
docs/**/*.md
📄 CodeRabbit inference engine (AGENTS.md)
docs/**/*.md: Use docs/ markdown as the reference for requirements, dependencies, and architectural decisions
Proactively update docs/ when decisions, requirements, or dependencies change
Documentation must use en-GB-oxendict spelling and grammar (LICENSE name unchanged)
Files:
docs/netsuke-design.mddocs/roadmap.md
**/*.md
📄 CodeRabbit inference engine (AGENTS.md)
**/*.md: Validate Markdown with make markdownlint
Run make fmt after doc changes to format Markdown and fix tables
Validate Mermaid diagrams in Markdown using make nixie
Wrap paragraphs and bullet points at 80 columns
Wrap code blocks at 120 columns
Do not wrap tables and headings
Use dashes (-) for list bullets
Use GitHub-flavoured Markdown footnotes ([^1]) for references and footnotes
Files:
docs/netsuke-design.mddocs/roadmap.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/netsuke-design.mddocs/roadmap.md
docs/**/*.{rs,md}
📄 CodeRabbit inference engine (docs/rust-doctest-dry-guide.md)
docs/**/*.{rs,md}: In fenced documentation code blocks, specify the language as rust (```rust)
Use assert!, assert_eq!, and assert_ne! in doctests to verify outcomes rather than relying on panic-free execution alone
Avoid using .unwrap() or .expect() in examples; handle errors idiomatically
For fallible examples, write an explicit fn main() -> Result<...> and hide boilerplate with # lines
Alternatively, use the (()) shorthand at the end of a code block (no whitespace) to create an implicit Result-returning main
Use hidden lines (#) to hide setup, main wrappers, and non-essential use statements in doctests
Prefer no_run over ignore for examples with side effects (e.g., network, filesystem)
Use should_panic for examples intended to panic; the test should fail if it does not panic
Use compile_fail for examples that should not compile, but recognize its brittleness across compiler versions
Use edition20xx (e.g., edition2018, edition2021) on code fences when demonstrating edition-specific syntax
Avoid relying solely on #[cfg(feature = "...")] inside doctest code blocks, which can yield misleading empty-ok tests when the feature is disabled
Files:
docs/netsuke-design.mddocs/roadmap.md
**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
**/*.rs: Keep functions small with single responsibility; apply command-query separation
Function documentation must include clear examples; test documentation should omit redundant examples
No single Rust code file may exceed 400 lines; split long switches/dispatch tables; move large test data to external files
Disallow Clippy warnings; code must be clean under clippy
Fix warnings emitted during tests in code, do not silence them
Extract helper functions when a function is too long; maintain separation of concerns and CQRS
Group many related parameters into well-named structs
Consider using Arc for large error values to reduce data returned
Every module must begin with a module-level //! comment describing purpose and utility
Document public APIs with Rustdoc /// comments so cargo doc can generate docs
Prefer immutable data; avoid unnecessary mut bindings
Use Result for error handling instead of panicking where feasible
Avoid unsafe code unless absolutely necessary; document any usage clearly
Place function attributes after doc comments
Do not use return in single-line functions
Use predicate functions for conditionals with more than two branches
Do not silence lints except as a last resort
When suppressing lints, scope narrowly and include a clear reason
Prefer #[expect(...)] over #[allow(...)] for lint handling
When functions are unused for certain features, gate with #[cfg]/#[cfg_attr]
Prefer .expect() over .unwrap()
Use concat!() for long string literals instead of escaping newlines with backslashes
Prefer single-line function bodies where appropriate (e.g., pub fn new(id: u64) -> Self { Self(id) })
Use cap-std for filesystem operations; avoid std::fs directly
Use camino for path handling; avoid std::path::PathBuf directly
Prefer semantic error enums deriving std::error::Error via thiserror for inspectable conditions
Use opaque eyre::Report only at the app boundary; do not expose in public APIs
Never export opaque error types from libraries; convert to domain...
Files:
tests/cucumber.rstests/std_filter_tests/network_functions.rstests/steps/stdlib_steps.rssrc/stdlib/network.rssrc/stdlib/mod.rstests/std_filter_tests/command_filters.rstests/std_filter_tests.rstests/std_filter_tests/io_filters.rssrc/manifest.rstests/std_filter_tests/support.rssrc/stdlib/command.rstests/std_filter_tests/hash_filters.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/cucumber.rstests/std_filter_tests/network_functions.rstests/steps/stdlib_steps.rssrc/stdlib/network.rssrc/stdlib/mod.rstests/std_filter_tests/command_filters.rstests/std_filter_tests.rstests/std_filter_tests/io_filters.rssrc/manifest.rstests/std_filter_tests/support.rssrc/stdlib/command.rstests/std_filter_tests/hash_filters.rs
tests/**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
tests/**/*.rs: Write unit and behavioural tests for new functionality; run before and after changes
Use rstest fixtures for shared setup
Replace duplicated tests with #[rstest(...)] parameterised cases
Prefer mockall for mocks/stubs
Mock nondeterministic deps via dependency injection using mockable traits (e.g., Env, Clock); follow docs/reliable-testing-in-rust-via-dependency-injection.md
Files:
tests/cucumber.rstests/std_filter_tests/network_functions.rstests/steps/stdlib_steps.rstests/std_filter_tests/command_filters.rstests/std_filter_tests.rstests/std_filter_tests/io_filters.rstests/std_filter_tests/support.rstests/std_filter_tests/hash_filters.rs
🧬 Code graph analysis (9)
tests/std_filter_tests/network_functions.rs (2)
tests/std_filter_tests/support.rs (1)
stdlib_env_with_state(22-26)src/stdlib/mod.rs (1)
is_impure(39-41)
tests/steps/stdlib_steps.rs (2)
src/stdlib/command.rs (1)
register(33-51)src/stdlib/mod.rs (2)
register(69-78)is_impure(39-41)
src/stdlib/mod.rs (2)
src/stdlib/command.rs (1)
register(33-51)src/stdlib/network.rs (1)
register_functions(18-20)
tests/std_filter_tests/command_filters.rs (2)
tests/std_filter_tests/support.rs (1)
stdlib_env_with_state(22-26)src/stdlib/mod.rs (1)
is_impure(39-41)
tests/std_filter_tests/io_filters.rs (1)
tests/std_filter_tests/support.rs (3)
filter_workspace(34-45)render(47-58)stdlib_env(28-31)
src/manifest.rs (2)
src/stdlib/command.rs (1)
register(33-51)src/stdlib/mod.rs (1)
register(69-78)
tests/std_filter_tests/support.rs (2)
src/stdlib/command.rs (1)
register(33-51)src/stdlib/mod.rs (1)
register(69-78)
src/stdlib/command.rs (1)
src/stdlib/mod.rs (1)
register(69-78)
tests/std_filter_tests/hash_filters.rs (1)
tests/std_filter_tests/support.rs (3)
filter_workspace(34-45)register_template(12-20)stdlib_env(28-31)
🪛 GitHub Actions: CI
tests/std_filter_tests/command_filters.rs
[error] 35-35: test failed: error should mention command exit status: invalid operation: shell command 'false' in template 'shell_fail' failed: Broken pipe (os error 32)
🔍 Remote MCP Deepwiki
I've gathered design details for the new stdlib network & command helpers and the functions implemented in the stdlib modules. Below is a concise summary of additional relevant context for the PR review:
Summary of relevant context:
-
docs/netsuke-design.md details:
- fetch(url, cache=false, cache_dir) function retrieves remote content with optional on-disk caching by SHA-256 URL digest, caches under
.netsuke/fetch. - shell and grep filters spawn a POSIX shell with single-quoted args via
shell-quote, mark templates impure inStdlibState.
- fetch(url, cache=false, cache_dir) function retrieves remote content with optional on-disk caching by SHA-256 URL digest, caches under
-
src/stdlib/network.rs:
- Implements
register_functions(env) -> ()to registerfetchhelper. fetch(url, kwargs): validates args, optionally reads/writes cache, performs HTTP GET viaureq, maps errors to TemplateErrors, returns bytes or UTF-8 string.
- Implements
-
src/stdlib/command.rs:
- Adds
shellandgrepfilters viaregister(env, impure_flag)markingStdlibStateimpure_flag on use. execute_shell(state, value, command): pipes input to command, captures stdout, maps failures.execute_grep(state, value, pattern, flags): builds grep command, parses flags, filters bytes.- Quoting via
quote(arg)andformat_command, runs via platform shell (run_command). - Error mapping with
CommandFailureandcommand_error.
- Adds
This context clarifies the implemented helpers' behavior and design considerations (security, caching, impurity), aiding review of the new stdlib modules.
⏰ 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-linux (aarch64-unknown-linux-gnu, linux-arm64, arm64)
- GitHub Check: release / build-macos (aarch64-apple-darwin, macos-14, macos-arm64, arm64)
- GitHub Check: release / build-macos (x86_64-apple-darwin, macos-13, macos-x86_64, x86_64)
- GitHub Check: release / build-windows (x86_64-pc-windows-msvc, amd64, x64)
- GitHub Check: release / build-linux (x86_64-unknown-linux-gnu, linux-amd64, amd64)
- GitHub Check: release / build-windows (aarch64-pc-windows-msvc, arm64, arm64)
🔇 Additional comments (13)
src/manifest.rs (1)
78-78: LGTM!Capturing the return value with an underscore prefix correctly suppresses the unused-value warning whilst maintaining code clarity.
docs/roadmap.md (2)
157-157: LGTM!The Template Standard Library has been implemented with the file-system tests, path filters, collection filters, network functions, command filters, and time helpers as documented in netsuke-design.md.
168-168: LGTM!The network and command functions/filters (fetch, shell, grep) have been implemented with impurity tracking via StdlibState, as confirmed by the test suite and documentation updates.
tests/std_filter_tests.rs (2)
3-4: LGTM!The command_filters module is correctly wired following the established pattern for test organization.
9-10: LGTM!The network_functions module is correctly wired following the established pattern for test organization.
tests/features/stdlib.feature (2)
75-88: LGTM!The shell and grep filter scenarios correctly verify both successful transformations and error handling, with proper impurity tracking assertions.
90-99: LGTM!The fetch scenarios correctly verify network retrieval behaviour and error handling, with the appropriate purity assertion (fetch is pure, unlike shell/grep).
tests/std_filter_tests/hash_filters.rs (1)
2-2: LGTM!The refactoring to use stdlib_env() centralises environment setup and reduces duplication across test files, improving maintainability.
Also applies to: 5-5, 51-51, 89-89, 105-105
tests/std_filter_tests/command_filters.rs (3)
6-19: LGTM!The test correctly verifies that the shell filter transforms input and marks templates impure, with proper state cleanup and assertions.
41-51: LGTM!The test correctly verifies that the grep filter extracts matching lines and marks templates impure.
53-67: LGTM!The test correctly verifies that invalid non-string flags are rejected with an appropriate error message.
docs/netsuke-design.md (1)
925-938: LGTM!The implementation details accurately document the fetch caching mechanism, shell command execution, and impurity tracking, providing valuable context for maintainers and contributors.
Cargo.toml (1)
47-47: Confirm ureq 2.10.5 is valid and secure.
Listed on crates.io; no known security advisories.
Run the grep filter directly on Windows to avoid cmd quoting bugs, reuse the binary output buffer when decoding, and ensure cucumber servers shut down cleanly with a non-brittle failure assertion.
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (4)
src/stdlib/command.rs(1 hunks)src/stdlib/network.rs(1 hunks)tests/cucumber.rs(3 hunks)tests/std_filter_tests/command_filters.rs(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
**/*.rs: Keep functions small with single responsibility; apply command-query separation
Function documentation must include clear examples; test documentation should omit redundant examples
No single Rust code file may exceed 400 lines; split long switches/dispatch tables; move large test data to external files
Disallow Clippy warnings; code must be clean under clippy
Fix warnings emitted during tests in code, do not silence them
Extract helper functions when a function is too long; maintain separation of concerns and CQRS
Group many related parameters into well-named structs
Consider using Arc for large error values to reduce data returned
Every module must begin with a module-level //! comment describing purpose and utility
Document public APIs with Rustdoc /// comments so cargo doc can generate docs
Prefer immutable data; avoid unnecessary mut bindings
Use Result for error handling instead of panicking where feasible
Avoid unsafe code unless absolutely necessary; document any usage clearly
Place function attributes after doc comments
Do not use return in single-line functions
Use predicate functions for conditionals with more than two branches
Do not silence lints except as a last resort
When suppressing lints, scope narrowly and include a clear reason
Prefer #[expect(...)] over #[allow(...)] for lint handling
When functions are unused for certain features, gate with #[cfg]/#[cfg_attr]
Prefer .expect() over .unwrap()
Use concat!() for long string literals instead of escaping newlines with backslashes
Prefer single-line function bodies where appropriate (e.g., pub fn new(id: u64) -> Self { Self(id) })
Use cap-std for filesystem operations; avoid std::fs directly
Use camino for path handling; avoid std::path::PathBuf directly
Prefer semantic error enums deriving std::error::Error via thiserror for inspectable conditions
Use opaque eyre::Report only at the app boundary; do not expose in public APIs
Never export opaque error types from libraries; convert to domain...
Files:
src/stdlib/network.rstests/cucumber.rstests/std_filter_tests/command_filters.rssrc/stdlib/command.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/network.rstests/cucumber.rstests/std_filter_tests/command_filters.rssrc/stdlib/command.rs
tests/**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
tests/**/*.rs: Write unit and behavioural tests for new functionality; run before and after changes
Use rstest fixtures for shared setup
Replace duplicated tests with #[rstest(...)] parameterised cases
Prefer mockall for mocks/stubs
Mock nondeterministic deps via dependency injection using mockable traits (e.g., Env, Clock); follow docs/reliable-testing-in-rust-via-dependency-injection.md
Files:
tests/cucumber.rstests/std_filter_tests/command_filters.rs
🧬 Code graph analysis (2)
tests/std_filter_tests/command_filters.rs (2)
tests/std_filter_tests/support.rs (1)
stdlib_env_with_state(22-26)src/stdlib/mod.rs (1)
is_impure(39-41)
src/stdlib/command.rs (1)
src/stdlib/mod.rs (1)
register(69-78)
🔍 Remote MCP Ref
Summary of additional concrete facts relevant to review
-
Public API changes
- New public struct StdlibState with Arc impurity flag; register(...) now returns StdlibState (callers updated in tests) — affects public API and requires consumers to handle the returned state.,
-
New stdlib helpers
- Command filters: added shell and grep template filters that execute shell commands via a platform shell; they set the StdlibState impure flag when used and convert failures into MiniJinja template errors with detailed context.
- Network function: added fetch(url, cache=false, cache_dir=...) that performs HTTP GET using the ureq crate and supports optional on-disk caching. Errors are mapped to template errors.
-
Dependency change
- Cargo.toml adds ureq = "2.10.5" (new runtime dependency for HTTP). Review for license/compat and transitive dependencies.
-
Caching behavior (implementation details)
- Cache key = SHA-256 hex digest of the URL; cache files stored under a cache directory (defaults to .netsuke/fetch when enabled). Cache read/write handles and error paths implemented; unit test verifies stable cache key generation.
-
Tests and fixtures
- New/updated tests cover command filters and network functions (including a local HTTP server test, cache persistence across restarts, impurity assertions). Test helpers now expose stdlib_env_with_state() returning (Environment, StdlibState) and tests assert StdlibState.is_impure/reset_impure behavior. Cucumber scenarios and steps added to exercise HTTP server and impurity checks. Tests update teardown to join test HTTP server threads.,
-
Docs and roadmap
- docs/netsuke-design.md documents the new fetch behavior (ureq usage, SHA-256 cache-keying, .netsuke/fetch), shell/grep quoting and impurity semantics. docs/roadmap.md marks Template Stdlib and network/command tasks as completed.
Potential review focus items (actionable)
- Verify the public API change (register -> returns StdlibState) is acceptable and documented for downstream callers.
- Audit ureq addition for license/compat and consider adding to changelog/release notes.
- Security review of shell/grep filters (command injection risk, quoting correctness, platform differences) and whether their use should be opt-in or documented with warnings.
- Validate cache path handling (relative vs absolute), file permissions, and error handling around cache corruption.
- Ensure tests reliably clean up HTTP server threads and that stdlib_state is not leaked across tests.
Sources
- Repository code and docs searched/read via Ref documentation search and read tools.,
⏰ 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, x86_64)
- GitHub Check: release / build-windows (x86_64-pc-windows-msvc, amd64, x64)
- GitHub Check: release / build-windows (aarch64-pc-windows-msvc, arm64, arm64)
- GitHub Check: release / build-linux (x86_64-unknown-linux-gnu, linux-amd64, amd64)
- GitHub Check: release / build-linux (aarch64-unknown-linux-gnu, linux-arm64, arm64)
- GitHub Check: build-test (ubuntu-latest, stable)
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (2)
src/stdlib/command.rs (1)
88-94: LGTM! Windows quoting issue resolved.The platform-specific approach correctly addresses the Windows cmd.exe quoting issue flagged in previous reviews. On Windows, grep invokes the programme directly with unquoted arguments. On Unix, the command string uses POSIX quoting through the shell. This ensures correct behaviour on both platforms.
tests/std_filter_tests/command_filters.rs (1)
21-41: LGTM! Brittle assertion fixed.The test now correctly handles multiple error message formats (lines 36-39), resolving the issue from previous reviews where "Broken pipe (os error 32)" failed the assertion. The broader check for "shell command", "failed", or "error" ensures the test passes for all legitimate failure modes whilst still validating that an error occurred.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (4)
src/stdlib/command.rs(1 hunks)src/stdlib/network.rs(1 hunks)tests/cucumber.rs(3 hunks)tests/std_filter_tests/command_filters.rs(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
**/*.rs: Keep functions small with single responsibility; apply command-query separation
Function documentation must include clear examples; test documentation should omit redundant examples
No single Rust code file may exceed 400 lines; split long switches/dispatch tables; move large test data to external files
Disallow Clippy warnings; code must be clean under clippy
Fix warnings emitted during tests in code, do not silence them
Extract helper functions when a function is too long; maintain separation of concerns and CQRS
Group many related parameters into well-named structs
Consider using Arc for large error values to reduce data returned
Every module must begin with a module-level //! comment describing purpose and utility
Document public APIs with Rustdoc /// comments so cargo doc can generate docs
Prefer immutable data; avoid unnecessary mut bindings
Use Result for error handling instead of panicking where feasible
Avoid unsafe code unless absolutely necessary; document any usage clearly
Place function attributes after doc comments
Do not use return in single-line functions
Use predicate functions for conditionals with more than two branches
Do not silence lints except as a last resort
When suppressing lints, scope narrowly and include a clear reason
Prefer #[expect(...)] over #[allow(...)] for lint handling
When functions are unused for certain features, gate with #[cfg]/#[cfg_attr]
Prefer .expect() over .unwrap()
Use concat!() for long string literals instead of escaping newlines with backslashes
Prefer single-line function bodies where appropriate (e.g., pub fn new(id: u64) -> Self { Self(id) })
Use cap-std for filesystem operations; avoid std::fs directly
Use camino for path handling; avoid std::path::PathBuf directly
Prefer semantic error enums deriving std::error::Error via thiserror for inspectable conditions
Use opaque eyre::Report only at the app boundary; do not expose in public APIs
Never export opaque error types from libraries; convert to domain...
Files:
src/stdlib/command.rstests/cucumber.rssrc/stdlib/network.rstests/std_filter_tests/command_filters.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/command.rstests/cucumber.rssrc/stdlib/network.rstests/std_filter_tests/command_filters.rs
tests/**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
tests/**/*.rs: Write unit and behavioural tests for new functionality; run before and after changes
Use rstest fixtures for shared setup
Replace duplicated tests with #[rstest(...)] parameterised cases
Prefer mockall for mocks/stubs
Mock nondeterministic deps via dependency injection using mockable traits (e.g., Env, Clock); follow docs/reliable-testing-in-rust-via-dependency-injection.md
Files:
tests/cucumber.rstests/std_filter_tests/command_filters.rs
🧬 Code graph analysis (2)
src/stdlib/command.rs (1)
src/stdlib/mod.rs (1)
register(69-78)
tests/std_filter_tests/command_filters.rs (2)
tests/std_filter_tests/support.rs (1)
stdlib_env_with_state(22-26)src/stdlib/mod.rs (1)
is_impure(39-41)
🔍 Remote MCP
Concise additional facts relevant to review
- New runtime state: StdlibState (pub) now exposes impure tracking (is_impure, reset_impure, impure_flag) and is returned from stdlib::register — callers must handle/propagate this state where needed (tests now use stdlib_env_with_state).
- Command filters (shell, grep):
- Spawn platform shell with quoted args (quote/format_command), pipe input bytes to subprocess, capture stdout, and map failures into Template errors with exit/status context. Using an impure_flag to mark templates impure when used.
- grep supports flags parsing (sequences or single string); invalid flag types produce InvalidOperation errors.
- Network fetch:
- fetch(url, cache=false, cache_dir) implemented with ureq (dependency added in Cargo.toml). Optional on-disk caching keyed by SHA-256 hex of URL; cache directory created under supplied path or default (.netsuke/fetch). Cache read/write use ambient authority and return UTF-8 string when possible, else raw bytes.
- Errors from HTTP or cache IO map to Template InvalidOperation errors with explanatory messages.
- Tests and fixtures:
- New unit/integration tests added:
- tests/std_filter_tests/{command_filters.rs,network_functions.rs} exercise shell/grep and fetch (including cache behavior).
- Cucumber feature scenarios added for shell/grep/fetch and a local HTTP test server helper (spawn_http_server) used in steps and tests.
- Test helper stdlib_env_with_state returns (Environment, StdlibState); stdlib_env() now discards the state.
- CliWorld in cucumber tests now stores stdlib_state, http_server handle, and stdlib_url; Drop impl joins server thread when present.
- New unit/integration tests added:
- Docs/roadmap updated to reflect implemented features and record design details (fetch cache key algorithm, cache location, impurity semantics for shell/grep, and shell quoting behavior).
- Public API changes to review for compatibility:
- Added public struct StdlibState and its methods.
- Changed signature: pub fn register(env: &mut Environment<'_>) -> StdlibState (was unit-like).
- Added pub(crate) fn register_functions(env: &mut Environment<'_>) for network registration.
- Dependency change:
- Added ureq = "2.10.5" to Cargo.toml (network fetch).
Sources used for this summary: provided PR context and diffs (files and tests listed in the PR).
⏰ 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, x86_64)
- GitHub Check: release / build-windows (x86_64-pc-windows-msvc, amd64, x64)
- GitHub Check: release / build-windows (aarch64-pc-windows-msvc, arm64, arm64)
- GitHub Check: release / build-linux (x86_64-unknown-linux-gnu, linux-amd64, amd64)
- GitHub Check: build-test (ubuntu-latest, stable)
🔇 Additional comments (4)
src/stdlib/command.rs (3)
1-7: LGTM!The module documentation clearly explains purpose, behaviour, and the impurity tracking mechanism.
126-144: Verify windows_quote correctness for cmd.exe.The
quotefunction usesshell_quote::windows::quoteto escape arguments for cmd.exe on Windows. Verify that this function produces output compatible with cmd.exe's parsing rules, as Windows command-line quoting has complex edge cases around special characters (e.g.,^,%,!). If shell_quote's Windows implementation targets CreateProcess rather than cmd.exe specifically, the quoting may not be safe.Run the following script to check the shell_quote documentation and verify cmd.exe compatibility:
Does the Rust shell_quote crate's windows::quote function produce output compatible with cmd.exe's parsing rules, or does it target CreateProcess? What special characters does it handle?
240-253: LGTM!The
CommandFailureenum andFrom<io::Error>implementation provide clear error categorisation and good ergonomics for error propagation.tests/std_filter_tests/command_filters.rs (1)
6-19: LGTM!The test correctly verifies that the shell filter marks templates impure and produces the expected output. The setup using
stdlib_env_with_stateand explicit impurity reset ensures clean test state.
Wire the stdlib impurity flag into fetch, harden cache directory handling for Windows paths, and document the shell security expectations. Add Windows-only command filter stubs plus fetch impurity assertions in unit and cucumber tests to cover the new behaviour.
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (7)
docs/netsuke-design.md(1 hunks)src/stdlib/command.rs(1 hunks)src/stdlib/mod.rs(3 hunks)src/stdlib/network.rs(1 hunks)tests/features/stdlib.feature(1 hunks)tests/std_filter_tests/command_filters.rs(1 hunks)tests/std_filter_tests/network_functions.rs(1 hunks)
🧰 Additional context used
📓 Path-based instructions (5)
**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
**/*.rs: Keep functions small with single responsibility; apply command-query separation
Function documentation must include clear examples; test documentation should omit redundant examples
No single Rust code file may exceed 400 lines; split long switches/dispatch tables; move large test data to external files
Disallow Clippy warnings; code must be clean under clippy
Fix warnings emitted during tests in code, do not silence them
Extract helper functions when a function is too long; maintain separation of concerns and CQRS
Group many related parameters into well-named structs
Consider using Arc for large error values to reduce data returned
Every module must begin with a module-level //! comment describing purpose and utility
Document public APIs with Rustdoc /// comments so cargo doc can generate docs
Prefer immutable data; avoid unnecessary mut bindings
Use Result for error handling instead of panicking where feasible
Avoid unsafe code unless absolutely necessary; document any usage clearly
Place function attributes after doc comments
Do not use return in single-line functions
Use predicate functions for conditionals with more than two branches
Do not silence lints except as a last resort
When suppressing lints, scope narrowly and include a clear reason
Prefer #[expect(...)] over #[allow(...)] for lint handling
When functions are unused for certain features, gate with #[cfg]/#[cfg_attr]
Prefer .expect() over .unwrap()
Use concat!() for long string literals instead of escaping newlines with backslashes
Prefer single-line function bodies where appropriate (e.g., pub fn new(id: u64) -> Self { Self(id) })
Use cap-std for filesystem operations; avoid std::fs directly
Use camino for path handling; avoid std::path::PathBuf directly
Prefer semantic error enums deriving std::error::Error via thiserror for inspectable conditions
Use opaque eyre::Report only at the app boundary; do not expose in public APIs
Never export opaque error types from libraries; convert to domain...
Files:
tests/std_filter_tests/command_filters.rssrc/stdlib/mod.rssrc/stdlib/network.rstests/std_filter_tests/network_functions.rssrc/stdlib/command.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/std_filter_tests/command_filters.rssrc/stdlib/mod.rssrc/stdlib/network.rstests/std_filter_tests/network_functions.rssrc/stdlib/command.rs
tests/**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
tests/**/*.rs: Write unit and behavioural tests for new functionality; run before and after changes
Use rstest fixtures for shared setup
Replace duplicated tests with #[rstest(...)] parameterised cases
Prefer mockall for mocks/stubs
Mock nondeterministic deps via dependency injection using mockable traits (e.g., Env, Clock); follow docs/reliable-testing-in-rust-via-dependency-injection.md
Files:
tests/std_filter_tests/command_filters.rstests/std_filter_tests/network_functions.rs
docs/**/*.md
📄 CodeRabbit inference engine (AGENTS.md)
docs/**/*.md: Use docs/ markdown as the reference for requirements, dependencies, and architectural decisions
Proactively update docs/ when decisions, requirements, or dependencies change
Documentation must use en-GB-oxendict spelling and grammar (LICENSE name unchanged)
Files:
docs/netsuke-design.md
**/*.md
📄 CodeRabbit inference engine (AGENTS.md)
**/*.md: Validate Markdown with make markdownlint
Run make fmt after doc changes to format Markdown and fix tables
Validate Mermaid diagrams in Markdown using make nixie
Wrap paragraphs and bullet points at 80 columns
Wrap code blocks at 120 columns
Do not wrap tables and headings
Use dashes (-) for list bullets
Use GitHub-flavoured Markdown footnotes ([^1]) for references and footnotes
Files:
docs/netsuke-design.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/netsuke-design.md
docs/**/*.{rs,md}
📄 CodeRabbit inference engine (docs/rust-doctest-dry-guide.md)
docs/**/*.{rs,md}: In fenced documentation code blocks, specify the language as rust (```rust)
Use assert!, assert_eq!, and assert_ne! in doctests to verify outcomes rather than relying on panic-free execution alone
Avoid using .unwrap() or .expect() in examples; handle errors idiomatically
For fallible examples, write an explicit fn main() -> Result<...> and hide boilerplate with # lines
Alternatively, use the (()) shorthand at the end of a code block (no whitespace) to create an implicit Result-returning main
Use hidden lines (#) to hide setup, main wrappers, and non-essential use statements in doctests
Prefer no_run over ignore for examples with side effects (e.g., network, filesystem)
Use should_panic for examples intended to panic; the test should fail if it does not panic
Use compile_fail for examples that should not compile, but recognize its brittleness across compiler versions
Use edition20xx (e.g., edition2018, edition2021) on code fences when demonstrating edition-specific syntax
Avoid relying solely on #[cfg(feature = "...")] inside doctest code blocks, which can yield misleading empty-ok tests when the feature is disabled
Files:
docs/netsuke-design.md
🧬 Code graph analysis (4)
tests/std_filter_tests/command_filters.rs (3)
tests/std_filter_tests/support.rs (1)
stdlib_env_with_state(22-26)src/stdlib/mod.rs (1)
is_impure(39-41)test_support/src/env_lock.rs (1)
acquire(17-21)
src/stdlib/mod.rs (2)
src/stdlib/command.rs (1)
register(43-61)src/stdlib/network.rs (1)
register_functions(24-28)
tests/std_filter_tests/network_functions.rs (2)
tests/std_filter_tests/support.rs (1)
stdlib_env_with_state(22-26)src/stdlib/mod.rs (1)
is_impure(39-41)
src/stdlib/command.rs (1)
src/stdlib/mod.rs (1)
register(69-79)
🔍 Remote MCP Deepwiki
Summary of additional facts relevant to review
-
Public API changes
- stdlib::register now returns StdlibState (holds Arc) and exposes methods is_impure, reset_impure, impure_flag — callers must capture/propagate state.
-
New stdlib features and behavior
- Command filters: new shell and grep filters execute external commands, mark templates impure on first use, perform platform-aware quoting and execution, and map subprocess failures into Template errors. Review arg quoting, Windows path/program handling, and CommandFailure → Template error mapping.
- Network fetch: fetch(url, cache=false, cache_dir) implemented using ureq (uvep added to Cargo), optional on-disk caching keyed by SHA-256(hex(url)), default cache at .netsuke/fetch, reads/writes use ambient authority and return UTF-8 when possible; remote fetches or cache writes mark stdlib impure. Validate HTTP error mapping and cache error handling.
-
Dependency change
- Added ureq = "2.10.5" for HTTP fetch support (affects licensing/attack surface).
-
Tests and fixtures added/updated
- New unit/integration tests: command_filters.rs, network_functions.rs; cucumber feature scenarios for shell/grep/fetch; test helpers stdlib_env_with_state and stdlib_env(); CliWorld in cucumber now holds stdlib_state, http_server handle and stdlib_url and Drop joins server thread. Ensure CI runs cover these new tests (cargo test --test cucumber).
-
Docs/roadmap updates
- docs/netsuke-design.md documents impurity semantics, cache-key algorithm (SHA-256 hex of URL), cache location and ambient-authority rationale, shell quoting strategy, and that use of external commands marks stdlib impure.
-
Areas to focus in review
- Correct propagation and lifetime/Clone semantics of StdlibState (thread-safety via Arc).
- Security of shell argument quoting and final command string validation (ensure no injection bypass, shlex validation where applicable).
- Windows-specific command execution paths (run_program vs shell invocation) and test coverage for Windows stubs.
- Robustness of cache path resolution (absolute vs relative, Windows drive prefixes) and error handling when writing cache (permissions/partial writes).
- Error messages mapped to Template InvalidOperation — verify they are actionable and not leaking sensitive info.
- CI/test ergonomics: ensure new cucumber tests and local HTTP server teardown/join logic are reliable under CI timeouts.
Sources
- Project docs and implementation overview pages (includes stdlib, network, command, tests, roadmap) retrieved from repository wiki/documentation.
⏰ 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-macos (x86_64-apple-darwin, macos-13, macos-x86_64, x86_64)
- GitHub Check: release / build-windows (x86_64-pc-windows-msvc, amd64, x64)
- GitHub Check: release / build-windows (aarch64-pc-windows-msvc, arm64, arm64)
- GitHub Check: build-test (ubuntu-latest, stable)
There was a problem hiding this comment.
Actionable comments posted: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/steps/stdlib_steps.rs (1)
205-220: Guard environment mutation with a shared lockWrap HOME/USERPROFILE mutation in a global env lock held for the scenario lifetime (store the guard in CliWorld) to satisfy the repository’s “env access is unsafe” guidance.
As per coding guidelines.
AI prompt for follow-up: Add a field env_lock: Option<test_support::env_lock::EnvLock> to CliWorld, acquire it when first mutating env (e.g., in stdlib_workspace or home_points_to_stdlib_root), and drop it in CliWorld::drop after restoring vars.
📜 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 (10)
Cargo.toml(1 hunks)src/stdlib/command.rs(1 hunks)src/stdlib/network.rs(1 hunks)test_support/src/command_helper.rs(1 hunks)test_support/src/lib.rs(1 hunks)tests/cucumber.rs(3 hunks)tests/features/stdlib.feature(1 hunks)tests/std_filter_tests/command_filters.rs(1 hunks)tests/std_filter_tests/network_functions.rs(1 hunks)tests/steps/stdlib_steps.rs(6 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
**/*.rs: Keep functions small with single responsibility; apply command-query separation
Function documentation must include clear examples; test documentation should omit redundant examples
No single Rust code file may exceed 400 lines; split long switches/dispatch tables; move large test data to external files
Disallow Clippy warnings; code must be clean under clippy
Fix warnings emitted during tests in code, do not silence them
Extract helper functions when a function is too long; maintain separation of concerns and CQRS
Group many related parameters into well-named structs
Consider using Arc for large error values to reduce data returned
Every module must begin with a module-level //! comment describing purpose and utility
Document public APIs with Rustdoc /// comments so cargo doc can generate docs
Prefer immutable data; avoid unnecessary mut bindings
Use Result for error handling instead of panicking where feasible
Avoid unsafe code unless absolutely necessary; document any usage clearly
Place function attributes after doc comments
Do not use return in single-line functions
Use predicate functions for conditionals with more than two branches
Do not silence lints except as a last resort
When suppressing lints, scope narrowly and include a clear reason
Prefer #[expect(...)] over #[allow(...)] for lint handling
When functions are unused for certain features, gate with #[cfg]/#[cfg_attr]
Prefer .expect() over .unwrap()
Use concat!() for long string literals instead of escaping newlines with backslashes
Prefer single-line function bodies where appropriate (e.g., pub fn new(id: u64) -> Self { Self(id) })
Use cap-std for filesystem operations; avoid std::fs directly
Use camino for path handling; avoid std::path::PathBuf directly
Prefer semantic error enums deriving std::error::Error via thiserror for inspectable conditions
Use opaque eyre::Report only at the app boundary; do not expose in public APIs
Never export opaque error types from libraries; convert to domain...
Files:
test_support/src/lib.rssrc/stdlib/command.rstests/cucumber.rssrc/stdlib/network.rstests/steps/stdlib_steps.rstests/std_filter_tests/command_filters.rstest_support/src/command_helper.rstests/std_filter_tests/network_functions.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/lib.rssrc/stdlib/command.rstests/cucumber.rssrc/stdlib/network.rstests/steps/stdlib_steps.rstests/std_filter_tests/command_filters.rstest_support/src/command_helper.rstests/std_filter_tests/network_functions.rs
Cargo.toml
📄 CodeRabbit inference engine (AGENTS.md)
Cargo.toml: Use explicit version ranges in Cargo.toml and keep dependencies up to date
All dependencies must use SemVer caret requirements (e.g., "1.2.3")
Forbid wildcard (*) and open-ended (>=) version specifiers; use ~ only with documented, specific reasons
Files:
Cargo.toml
tests/**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
tests/**/*.rs: Write unit and behavioural tests for new functionality; run before and after changes
Use rstest fixtures for shared setup
Replace duplicated tests with #[rstest(...)] parameterised cases
Prefer mockall for mocks/stubs
Mock nondeterministic deps via dependency injection using mockable traits (e.g., Env, Clock); follow docs/reliable-testing-in-rust-via-dependency-injection.md
Files:
tests/cucumber.rstests/steps/stdlib_steps.rstests/std_filter_tests/command_filters.rstests/std_filter_tests/network_functions.rs
🧬 Code graph analysis (4)
src/stdlib/command.rs (1)
src/stdlib/mod.rs (1)
register(69-79)
tests/steps/stdlib_steps.rs (3)
test_support/src/command_helper.rs (1)
compile_uppercase_helper(15-31)src/stdlib/command.rs (2)
register(49-67)from(329-331)src/stdlib/mod.rs (2)
register(69-79)is_impure(39-41)
tests/std_filter_tests/command_filters.rs (4)
test_support/src/command_helper.rs (1)
compile_uppercase_helper(15-31)tests/std_filter_tests/support.rs (1)
stdlib_env_with_state(22-26)src/stdlib/mod.rs (1)
is_impure(39-41)test_support/src/env_lock.rs (1)
acquire(17-21)
tests/std_filter_tests/network_functions.rs (2)
tests/std_filter_tests/support.rs (1)
stdlib_env_with_state(22-26)src/stdlib/mod.rs (1)
is_impure(39-41)
⏰ 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)
- GitHub Check: release / build-macos (x86_64-apple-darwin, macos-13, macos-x86_64, x86_64)
- GitHub Check: release / build-windows (x86_64-pc-windows-msvc, amd64, x64)
- GitHub Check: release / build-linux (x86_64-unknown-linux-gnu, linux-amd64, amd64)
- GitHub Check: build-test (ubuntu-latest, stable)
🔇 Additional comments (21)
Cargo.toml (1)
47-48: LGTM! Dependencies follow version guidelines.The ureq and wait-timeout dependencies use appropriate SemVer caret version specifiers as required by the coding guidelines.
src/stdlib/network.rs (9)
1-5: LGTM! Module documentation follows guidelines.The module begins with a clear
//!comment explaining its purpose and utility, as required by the coding guidelines.
24-28: LGTM! Function registration correctly threads impurity tracking.The fetch function is properly registered with the impure flag captured in the closure.
30-51: LGTM! Fetch logic correctly handles caching and impurity.The function properly marks templates impure for both cache hits (line 39) and remote fetches, addressing previous review feedback.
123-141: LGTM! Cache reading logic is robust.The function correctly handles missing files by returning
Noneand properly reports other I/O errors.
143-151: LGTM! Cache writing correctly marks impurity.The function appropriately marks the template impure before performing filesystem I/O.
153-156: LGTM! Cache key generation is secure.Using SHA-256 provides strong collision resistance for URL-based cache keys.
158-165: LGTM! Hex encoding is efficient.The function pre-allocates the string with the correct capacity and safely discards the write! Result since writing to a String is infallible.
167-172: LGTM! Value conversion avoids unnecessary allocations.The function efficiently consumes the input buffer and reuses it when UTF-8 decoding fails, addressing previous review feedback.
174-179: LGTM! Centralised error handling improves consistency.The helper function provides uniform error formatting across I/O operations.
test_support/src/lib.rs (1)
14-14: LGTM! Module declaration follows conventions.The addition of the
command_helpermodule is straightforward and maintains file length below the 400-line guideline.tests/features/stdlib.feature (4)
86-89: LGTM! Grep scenario is portable.The scenario uses inline text rather than external commands, ensuring cross-platform compatibility.
91-95: LGTM! Fetch scenario correctly tests remote content retrieval.The scenario appropriately uses an HTTP server fixture and validates both content and impurity tracking.
97-100: LGTM! Fetch error scenario validates error handling.The scenario correctly tests error reporting for network failures and confirms impurity tracking occurs even on errors.
75-80: ```bash
#!/bin/bash
sed -n '1,200p' test_support/src/command_helper.rs</blockquote></details> <details> <summary>tests/std_filter_tests/network_functions.rs (3)</summary><blockquote> `34-51`: **LGTM! Test correctly validates fetch and impurity tracking.** The test properly resets impure state, verifies content retrieval, confirms impurity marking, and cleans up the server thread. --- `53-87`: **LGTM! Test thoroughly validates caching behaviour.** The test correctly verifies both cache population and cache hits, confirming impurity tracking in both scenarios. --- `13-32`: **Remove thread cleanup warning**: tests already call `handle.join()` at lines 50 and 76, ensuring server threads terminate. > Likely an incorrect or invalid review comment. </blockquote></details> <details> <summary>tests/cucumber.rs (1)</summary><blockquote> `69-79`: **Avoid teardown hangs by using a timed connect** Use connect_timeout to ensure teardown never blocks if the URL is malformed or unreachable. [ suggest_recommended_refactor ] Apply: ```diff - { - let _ = TcpStream::connect(host); - } + { + if let Ok(sock) = host.parse::<std::net::SocketAddr>() { + let _ = std::net::TcpStream::connect_timeout( + &sock, + std::time::Duration::from_millis(100), + ); + } else { + // Fallback if parsing fails + let _ = TcpStream::connect(host); + } + }tests/std_filter_tests/command_filters.rs (1)
60-77: Good coverage for timeout behaviourThe timeout test exercises the default command timeout and error mapping. LGTM.
tests/steps/stdlib_steps.rs (1)
139-143: Capture and expose stdlib impurity stateRegistering stdlib, resetting impurity, and storing the state in world is correct and enables the new impurity assertions. LGTM.
![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.
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: 2
♻️ Duplicate comments (1)
tests/steps/stdlib_steps.rs (1)
186-202: HTTP server cleanup implemented per previous review.The step now pings the old server URL with
TcpStream::connectbefore joining, which should unblock theaccept()call and prevent hangs. This matches the pattern recommended in the previous review.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (7)
src/stdlib/command.rs(1 hunks)src/stdlib/network.rs(1 hunks)test_support/src/command_helper.rs(1 hunks)tests/features/stdlib.feature(1 hunks)tests/std_filter_tests/command_filters.rs(1 hunks)tests/std_filter_tests/network_functions.rs(1 hunks)tests/steps/stdlib_steps.rs(6 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
**/*.rs: Keep functions small with single responsibility; apply command-query separation
Function documentation must include clear examples; test documentation should omit redundant examples
No single Rust code file may exceed 400 lines; split long switches/dispatch tables; move large test data to external files
Disallow Clippy warnings; code must be clean under clippy
Fix warnings emitted during tests in code, do not silence them
Extract helper functions when a function is too long; maintain separation of concerns and CQRS
Group many related parameters into well-named structs
Consider using Arc for large error values to reduce data returned
Every module must begin with a module-level //! comment describing purpose and utility
Document public APIs with Rustdoc /// comments so cargo doc can generate docs
Prefer immutable data; avoid unnecessary mut bindings
Use Result for error handling instead of panicking where feasible
Avoid unsafe code unless absolutely necessary; document any usage clearly
Place function attributes after doc comments
Do not use return in single-line functions
Use predicate functions for conditionals with more than two branches
Do not silence lints except as a last resort
When suppressing lints, scope narrowly and include a clear reason
Prefer #[expect(...)] over #[allow(...)] for lint handling
When functions are unused for certain features, gate with #[cfg]/#[cfg_attr]
Prefer .expect() over .unwrap()
Use concat!() for long string literals instead of escaping newlines with backslashes
Prefer single-line function bodies where appropriate (e.g., pub fn new(id: u64) -> Self { Self(id) })
Use cap-std for filesystem operations; avoid std::fs directly
Use camino for path handling; avoid std::path::PathBuf directly
Prefer semantic error enums deriving std::error::Error via thiserror for inspectable conditions
Use opaque eyre::Report only at the app boundary; do not expose in public APIs
Never export opaque error types from libraries; convert to domain...
Files:
test_support/src/command_helper.rssrc/stdlib/network.rstests/steps/stdlib_steps.rstests/std_filter_tests/command_filters.rstests/std_filter_tests/network_functions.rssrc/stdlib/command.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/command_helper.rssrc/stdlib/network.rstests/steps/stdlib_steps.rstests/std_filter_tests/command_filters.rstests/std_filter_tests/network_functions.rssrc/stdlib/command.rs
tests/**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
tests/**/*.rs: Write unit and behavioural tests for new functionality; run before and after changes
Use rstest fixtures for shared setup
Replace duplicated tests with #[rstest(...)] parameterised cases
Prefer mockall for mocks/stubs
Mock nondeterministic deps via dependency injection using mockable traits (e.g., Env, Clock); follow docs/reliable-testing-in-rust-via-dependency-injection.md
Files:
tests/steps/stdlib_steps.rstests/std_filter_tests/command_filters.rstests/std_filter_tests/network_functions.rs
🧬 Code graph analysis (4)
tests/steps/stdlib_steps.rs (3)
test_support/src/command_helper.rs (2)
compile_failure_helper(68-70)compile_uppercase_helper(46-48)src/stdlib/command.rs (2)
register(49-67)from(323-325)src/stdlib/mod.rs (2)
register(69-79)is_impure(39-41)
tests/std_filter_tests/command_filters.rs (4)
test_support/src/command_helper.rs (3)
compile_failure_helper(68-70)compile_uppercase_helper(46-48)compile_rust_helper(98-114)tests/std_filter_tests/support.rs (1)
stdlib_env_with_state(22-26)src/stdlib/mod.rs (1)
is_impure(39-41)test_support/src/env_lock.rs (1)
acquire(17-21)
tests/std_filter_tests/network_functions.rs (2)
tests/std_filter_tests/support.rs (1)
stdlib_env_with_state(22-26)src/stdlib/mod.rs (1)
is_impure(39-41)
src/stdlib/command.rs (1)
src/stdlib/mod.rs (1)
register(69-79)
🔍 Remote MCP
Additional context summary relevant to reviewing PR #187:
- Dependency choices:
- ureq v2.10.5: default blocking HTTP client with connection pooling and configurable timeouts (implicit) [Ref::].
- Shell-quote crate ensures safe POSIX/Windows quoting for
shellandgrepfilters, preventing injection [Deepwiki::Overview]. - Stdlib registration in
src/stdlib/mod.rsnow returnsStdlibStateto track impurity across new network/command helpers [ai_summary]. wait-timeoutv0.2 dependency provides cross‐platform process timeout support forrun_command[ai_summary].- The new stdlib modules (
network.rs,command.rs) follow existing patterns for function registration and error propagation viathiserror/anyhow[Deepwiki::Template System].
⏰ 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 (aarch64-apple-darwin, macos-14, macos-arm64, arm64)
- GitHub Check: release / build-macos (x86_64-apple-darwin, macos-13, macos-x86_64, x86_64)
- GitHub Check: release / build-windows (aarch64-pc-windows-msvc, arm64, arm64)
- GitHub Check: release / build-windows (x86_64-pc-windows-msvc, amd64, x64)
- GitHub Check: release / build-linux (x86_64-unknown-linux-gnu, linux-amd64, amd64)
- GitHub Check: release / build-linux (aarch64-unknown-linux-gnu, linux-arm64, arm64)
- GitHub Check: Sourcery review
- GitHub Check: build-test (ubuntu-latest, stable)
🔇 Additional comments (25)
test_support/src/command_helper.rs (1)
1-114: LGTM! Module documentation and examples are comprehensive.The test-support helpers are well-structured with clear module-level documentation and complete Rustdoc for each public function. The use of
cap-stdfor filesystem operations andcaminofor path handling aligns with coding guidelines. Theexpect()calls are appropriate for test-support code.tests/features/stdlib.feature (1)
75-101: LGTM! Cross-platform compatibility achieved with compiled helpers.The new scenarios correctly test command and network filters whilst verifying impurity tracking. Using compiled Rust helpers (
uppercase stdlib command helper,failing stdlib command helper) instead of shell commands liketrandfalseensures the tests run reliably on Windows.tests/std_filter_tests/command_filters.rs (1)
1-238: LGTM! Comprehensive test coverage with proper platform handling.The test suite thoroughly exercises command filters with appropriate separation of Unix/Windows behaviour. Key strengths:
- Uses shared
compile_uppercase_helperandcompile_failure_helperfromtest_support- Verifies impurity marking in all code paths (success, failure, undefined input)
- Windows-specific tests properly use
EnvLockfixture to guardPATHmutations (lines 190, 217)- Platform-specific tests gated with
#[cfg(unix)]and#[cfg(windows)]tests/std_filter_tests/network_functions.rs (1)
1-107: LGTM! Network tests verify impurity on all code paths.The test suite correctly exercises fetch functionality including caching and error handling. The
start_serverhelper (lines 13-32) provides a simple but effective local HTTP server for testing. Impurity is properly verified on success (line 47), cached responses (lines 72, 84), and errors (lines 103-106).src/stdlib/network.rs (1)
1-189: LGTM! Network module correctly implements fetch with impurity tracking.The implementation properly addresses all past review feedback:
- Configures HTTP timeouts via
AgentBuilder(lines 56-61) to prevent indefinite hangs- Marks templates impure on remote fetch (line 55), cache hits (line 40), and cache writes (line 154)
- Handles absolute paths on Windows by parsing components (lines 88-123)
- Uses proper error handling without
expect()(lines 107-112)- Optimises
to_valueto avoid cloning by consuming the Vec (lines 178-181)tests/steps/stdlib_steps.rs (6)
12-20: Previous server cleanup concern appears addressed.The imports include
TcpStreamas recommended in the previous review for unblocking the HTTP server before joining.
141-157: LGTM! Correct impurity state management.The function correctly captures the stdlib state, resets impurity before rendering, and stores a clone for subsequent assertions. This aligns with the new stdlib API that returns
StdlibStatefromregister().
170-184: LGTM! Command helper setup follows consistent pattern.Both steps correctly compile the helper binaries and store the quoted command path. The
expectcalls are appropriate for test setup—panicking on failure is acceptable in test infrastructure.
268-286: LGTM! Rendering steps correctly propagate test fixtures.Both steps extract the stored values from
worldand inject them into the template context with appropriate keys. Theexpectcalls are suitable for test steps, ensuring fixtures are initialised.
370-386: LGTM! Impurity assertions correctly verify stdlib state.Both steps properly access the stored
StdlibStateand assert the impurity flag. The error messages clearly communicate expectations.
100-119: Remove single-request server critique
The tests spawn a fresh HTTP server per fetch and never reuse the same URL across multiple requests, so the single-request design is intentional and sufficient.Likely an incorrect or invalid review comment.
src/stdlib/command.rs (14)
1-13: LGTM! Module documentation includes security warnings.The module doc clearly describes purpose and includes a security section warning about command injection risks, addressing the previous review concern.
49-67: LGTM! Filter registration correctly tracks impurity.Both filters mark the stdlib state as impure before execution, using
Ordering::Relaxedwhich is appropriate for this use case. The pattern aligns with the network module.
69-81: LGTM! Shell execution follows single responsibility.The function validates input, executes the command, and converts output, with clear error propagation at each step.
83-110: LGTM! Grep execution handles platform differences correctly.Windows invokes
grepdirectly viarun_program, avoiding shell quoting issues, whilst Unix uses shell invocation with proper POSIX quoting. This addresses the previous review concern aboutcmd.exequoting.
112-136: LGTM! Flag collection handles various value types.The function correctly handles different value kinds and validates that all flags are strings. The error messages are specific to grep, which is appropriate since this is an internal helper for
execute_grep.
139-157: LGTM! Platform-appropriate quoting prevents injection.Windows uses
shell_quote::windows::quoteand Unix uses POSIX quoting, ensuring arguments are safely escaped for each platform's shell. This addresses the previous review concern.
159-172: LGTM! Generic error message addresses previous review.The error message "input value is undefined" is now generic and suitable for both
shellandgrepfilters.
174-183: LGTM! Shell invocation uses platform-specific constants.The function correctly configures the shell command with piped I/O and delegates to
run_childfor execution.
185-194: LGTM! Direct programme invocation avoids Windows shell issues.The Windows-specific
run_programfunction invokes the programme directly without a shell, sidesteppingcmd.exequoting complexities.
196-243: LGTM! Robust process execution with timeout and concurrent I/O.The function correctly:
- Handles broken pipes when the child closes stdin early
- Reads stdout/stderr concurrently to prevent deadlock
- Enforces a timeout with cleanup
- Returns detailed failure information including stderr
The complexity is justified given the IPC requirements. Past review concerns about timeout and broken pipe handling have been addressed.
245-250: LGTM! UTF-8 conversion with byte fallback.The function prefers UTF-8 strings but gracefully handles binary output, which is appropriate for command output that may not be text.
252-305: LGTM! Error messages use consistent "command" terminology.The function maps all failure modes to user-facing errors with clear context. The error messages now consistently use "command" rather than "shell command", addressing the previous review concern. The broken pipe error includes the explanation "(command closed input early)", which is helpful for debugging.
366-373: LGTM! Stderr formatting helper is clean and concise.The function correctly converts stderr to UTF-8 (lossily), trims whitespace, and appends to the error message with appropriate formatting.
328-340: Robustify kill error handling in wait_for_exit
Handle the race condition where the child may exit just before kill: ensure only InvalidInput is suppressed and any other Err from kill propagates as CommandFailure::Io.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/steps/stdlib_steps.rs (1)
1-4: Update the module doc to reflect network and command stepsDocument that this module also hosts network (HTTP server, fetch context) and command-helper steps, not only path/file filters.
♻️ Duplicate comments (3)
src/stdlib/network.rs (1)
88-123: Simplify absolute cache‑dir handlingRemove manual root/prefix splitting and use fs_utf8::create_dir_all with ambient authority, then reopen the directory. This reduces platform corner cases.
Apply this diff here:
- if utf_path.is_absolute() { - let mut root = String::new(); - for component in utf_path.components() { - match component { - Utf8Component::Prefix(prefix) => root.push_str(prefix.as_str()), - Utf8Component::RootDir => { - root.push('/'); - break; - } - _ => break, - } - } - if root.is_empty() { - root.push('/'); - } - let root_path = Utf8PathBuf::from(root); - - let root_dir = Dir::open_ambient_dir(&root_path, ambient_authority()) - .map_err(|err| io_error("open root cache dir", &root_path, &err))?; - let rel = utf_path.strip_prefix(&root_path).map_err(|_| { - Error::new( - ErrorKind::InvalidOperation, - format!("failed to compute relative path for '{utf_path}'"), - ) - })?; - if rel.as_str().is_empty() { - return Ok(root_dir); - } - - root_dir - .create_dir_all(rel) - .map_err(|err| io_error("create cache dir", utf_path, &err))?; - return root_dir - .open_dir(rel) - .map_err(|err| io_error("open cache dir", utf_path, &err)); - } + if utf_path.is_absolute() { + cap_std::fs_utf8::create_dir_all(utf_path, ambient_authority()) + .map_err(|err| io_error("create cache dir", utf_path, &err))?; + return Dir::open_ambient_dir(utf_path, ambient_authority()) + .map_err(|err| io_error("open cache dir", utf_path, &err)); + }Add the import:
- use cap_std::{ambient_authority, fs_utf8::Dir}; + use cap_std::{ambient_authority, fs_utf8::Dir}; + // uses fully-qualified cap_std::fs_utf8::create_dir_all above; alternatively import it: + // use cap_std::fs_utf8::{Dir, create_dir_all};test_support/src/command_helper.rs (1)
1-2: Good module-level documentation; resolves prior review askKeep this at the top of every module as per guidelines.
tests/steps/stdlib_steps.rs (1)
186-202: Unblock previous HTTP server before joining — well donePing the old URL to unblock accept() before join. This resolves the previously flagged hang risk.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (7)
src/stdlib/command.rs(1 hunks)src/stdlib/network.rs(1 hunks)test_support/src/command_helper.rs(1 hunks)tests/features/stdlib.feature(1 hunks)tests/std_filter_tests/command_filters.rs(1 hunks)tests/std_filter_tests/network_functions.rs(1 hunks)tests/steps/stdlib_steps.rs(6 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
**/*.rs: Keep functions small with single responsibility; apply command-query separation
Function documentation must include clear examples; test documentation should omit redundant examples
No single Rust code file may exceed 400 lines; split long switches/dispatch tables; move large test data to external files
Disallow Clippy warnings; code must be clean under clippy
Fix warnings emitted during tests in code, do not silence them
Extract helper functions when a function is too long; maintain separation of concerns and CQRS
Group many related parameters into well-named structs
Consider using Arc for large error values to reduce data returned
Every module must begin with a module-level //! comment describing purpose and utility
Document public APIs with Rustdoc /// comments so cargo doc can generate docs
Prefer immutable data; avoid unnecessary mut bindings
Use Result for error handling instead of panicking where feasible
Avoid unsafe code unless absolutely necessary; document any usage clearly
Place function attributes after doc comments
Do not use return in single-line functions
Use predicate functions for conditionals with more than two branches
Do not silence lints except as a last resort
When suppressing lints, scope narrowly and include a clear reason
Prefer #[expect(...)] over #[allow(...)] for lint handling
When functions are unused for certain features, gate with #[cfg]/#[cfg_attr]
Prefer .expect() over .unwrap()
Use concat!() for long string literals instead of escaping newlines with backslashes
Prefer single-line function bodies where appropriate (e.g., pub fn new(id: u64) -> Self { Self(id) })
Use cap-std for filesystem operations; avoid std::fs directly
Use camino for path handling; avoid std::path::PathBuf directly
Prefer semantic error enums deriving std::error::Error via thiserror for inspectable conditions
Use opaque eyre::Report only at the app boundary; do not expose in public APIs
Never export opaque error types from libraries; convert to domain...
Files:
src/stdlib/command.rstests/std_filter_tests/network_functions.rstests/std_filter_tests/command_filters.rstest_support/src/command_helper.rssrc/stdlib/network.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:
src/stdlib/command.rstests/std_filter_tests/network_functions.rstests/std_filter_tests/command_filters.rstest_support/src/command_helper.rssrc/stdlib/network.rstests/steps/stdlib_steps.rs
tests/**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
tests/**/*.rs: Write unit and behavioural tests for new functionality; run before and after changes
Use rstest fixtures for shared setup
Replace duplicated tests with #[rstest(...)] parameterised cases
Prefer mockall for mocks/stubs
Mock nondeterministic deps via dependency injection using mockable traits (e.g., Env, Clock); follow docs/reliable-testing-in-rust-via-dependency-injection.md
Files:
tests/std_filter_tests/network_functions.rstests/std_filter_tests/command_filters.rstests/steps/stdlib_steps.rs
🧬 Code graph analysis (4)
src/stdlib/command.rs (1)
src/stdlib/mod.rs (1)
register(69-79)
tests/std_filter_tests/network_functions.rs (2)
tests/std_filter_tests/support.rs (1)
stdlib_env_with_state(22-26)src/stdlib/mod.rs (1)
is_impure(39-41)
tests/std_filter_tests/command_filters.rs (4)
test_support/src/command_helper.rs (3)
compile_failure_helper(68-70)compile_uppercase_helper(46-48)compile_rust_helper(98-114)tests/std_filter_tests/support.rs (1)
stdlib_env_with_state(22-26)src/stdlib/mod.rs (1)
is_impure(39-41)test_support/src/env_lock.rs (1)
acquire(17-21)
tests/steps/stdlib_steps.rs (3)
test_support/src/command_helper.rs (2)
compile_failure_helper(68-70)compile_uppercase_helper(46-48)src/stdlib/command.rs (1)
register(49-67)src/stdlib/mod.rs (2)
register(69-79)is_impure(39-41)
⏰ 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 (aarch64-apple-darwin, macos-14, macos-arm64, arm64)
- GitHub Check: release / build-macos (x86_64-apple-darwin, macos-13, macos-x86_64, x86_64)
- GitHub Check: release / build-windows (aarch64-pc-windows-msvc, arm64, arm64)
- GitHub Check: release / build-windows (x86_64-pc-windows-msvc, amd64, x64)
- GitHub Check: release / build-linux (x86_64-unknown-linux-gnu, linux-amd64, amd64)
- GitHub Check: release / build-linux (aarch64-unknown-linux-gnu, linux-arm64, arm64)
- GitHub Check: Sourcery review
- GitHub Check: build-test (ubuntu-latest, stable)
🔇 Additional comments (10)
src/stdlib/command.rs (4)
148-157: Windows‑safe quoting is correctQuote arguments with shell_quote::windows::quote and build the command string safely. This fixes cmd.exe quoting pitfalls.
101-108: Bypass the shell for grep on WindowsInvoke grep directly with Command on Windows. This avoids cmd.exe mis‑parsing and ensures arguments are passed verbatim.
Also applies to: 185-194
252-304: Consistent, actionable error messagesNormalise phrasing to “command …” and include stderr/exit metadata. This improves diagnosability without leaking internals.
159-165: Generic undefined‑input errorReturn a shell‑agnostic error for undefined values. This keeps to_bytes reusable across filters.
src/stdlib/network.rs (1)
54-61: Good: explicit HTTP timeoutsConfigure connect/read/write/global timeouts on ureq Agent to prevent hangs. Robust default values.
tests/std_filter_tests/network_functions.rs (1)
34-51: Network tests are robust and preciseValidate fetch behaviour, cache hits, and error reporting with clear impurity assertions. Solid coverage.
Also applies to: 53-87, 89-107
tests/std_filter_tests/command_filters.rs (1)
20-39: Command/grep tests cover behaviour and portability wellExercise success, failure, undefined input, Windows quoting, PATH stubs, and timeouts with clear assertions and guards.
Also applies to: 41-66, 68-85, 87-97, 99-113, 115-133, 135-153, 155-159, 161-176, 188-213, 215-238
tests/features/stdlib.feature (1)
87-91: Ignore grep portability suggestion
The grep filter runs in-process and does not invoke the system grep, so no stub or@unixtag is needed.Likely an incorrect or invalid review comment.
test_support/src/command_helper.rs (1)
27-48: Public API docs and implementation read cleanly
- Provide clear, doctest-friendly examples with no_run.
- Use cap-std and camino correctly; avoid unwrap; use concat! for sources; return last expression.
LGTM.
Also applies to: 50-70, 72-114
tests/steps/stdlib_steps.rs (1)
224-239: Merge as is—environment mutations are serialised
test_support::env::set_var acquires EnvLock backed by a global Mutex, satisfying the project’s guard requirement.
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: Complex Conditionaltests/cucumber.rs: Drop.drop What lead to degradation?Drop.drop has 1 complex conditionals with 3 branches, threshold = 2 Why does this problem occur?A complex conditional is an expression inside a branch such as an if-statmeent which consists of multiple, logical operations. Example: if (x.started() && y.running()).Complex conditionals make the code even harder to read, and contribute to the Complex Method code smell. Encapsulate them. How to fix it?Apply the DECOMPOSE CONDITIONAL refactoring so that the complex conditional is encapsulated in a separate function with a good name that captures the business rule. Optionally, for simple expressions, introduce a new variable which holds the result of the complex conditional. Helpful refactoring examplesTo get a general understanding of what this code health issue looks like - and how it might be addressed - we have prepared some diffs for illustrative purposes. SAMPLE# complex_conditional.js
function messageReceived(message, timeReceived) {
- // Ignore all messages which aren't from known customers:
- if (!message.sender &&
- customers.getId(message.name) == null) {
+ // Refactoring #1: encapsulate the business rule in a
+ // function. A clear name replaces the need for the comment:
+ if (!knownCustomer(message)) {
log('spam received -- ignoring');
return;
}
- // Provide an auto-reply when outside business hours:
- if ((timeReceived.getHours() > 17) ||
- (timeReceived.getHours() < 8)) {
+ // Refactoring #2: encapsulate the business rule.
+ // Again, note how a clear function name replaces the
+ // need for a code comment:
+ if (outsideBusinessHours(timeReceived)) {
return autoReplyTo(message);
}
pingAgentFor(message);
+}
+
+function outsideBusinessHours(timeReceived) {
+ // Refactoring #3: replace magic numbers with
+ // symbols that communicate with the code reader:
+ const closingHour = 17;
+ const openingHour = 8;
+
+ const hours = timeReceived.getHours();
+
+ // Refactoring #4: simple conditional rules can
+ // be further clarified by introducing a variable:
+ const afterClosing = hours > closingHour;
+ const beforeOpening = hours < openingHour;
+
+ // Yeah -- look how clear the business rule is now!
+ return afterClosing || beforeOpening;
} |
|
@coderabbitai Please suggest a fix for this issue and supply a prompt for an AI coding agent to enable it to apply the fix: Complex Conditionaltests/steps/stdlib_steps.rs: http_server_returning What lead to degradation?http_server_returning has 1 complex conditionals with 3 branches, threshold = 2 Why does this problem occur?A complex conditional is an expression inside a branch such as an if-statmeent which consists of multiple, logical operations. Example: if (x.started() && y.running()).Complex conditionals make the code even harder to read, and contribute to the Complex Method code smell. Encapsulate them. How to fix it?Apply the DECOMPOSE CONDITIONAL refactoring so that the complex conditional is encapsulated in a separate function with a good name that captures the business rule. Optionally, for simple expressions, introduce a new variable which holds the result of the complex conditional. Helpful refactoring examplesTo get a general understanding of what this code health issue looks like - and how it might be addressed - we have prepared some diffs for illustrative purposes. SAMPLE# complex_conditional.js
function messageReceived(message, timeReceived) {
- // Ignore all messages which aren't from known customers:
- if (!message.sender &&
- customers.getId(message.name) == null) {
+ // Refactoring #1: encapsulate the business rule in a
+ // function. A clear name replaces the need for the comment:
+ if (!knownCustomer(message)) {
log('spam received -- ignoring');
return;
}
- // Provide an auto-reply when outside business hours:
- if ((timeReceived.getHours() > 17) ||
- (timeReceived.getHours() < 8)) {
+ // Refactoring #2: encapsulate the business rule.
+ // Again, note how a clear function name replaces the
+ // need for a code comment:
+ if (outsideBusinessHours(timeReceived)) {
return autoReplyTo(message);
}
pingAgentFor(message);
+}
+
+function outsideBusinessHours(timeReceived) {
+ // Refactoring #3: replace magic numbers with
+ // symbols that communicate with the code reader:
+ const closingHour = 17;
+ const openingHour = 8;
+
+ const hours = timeReceived.getHours();
+
+ // Refactoring #4: simple conditional rules can
+ // be further clarified by introducing a variable:
+ const afterClosing = hours > closingHour;
+ const beforeOpening = hours < openingHour;
+
+ // Yeah -- look how clear the business rule is now!
+ return afterClosing || beforeOpening;
} |
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.
|
@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/std_filter_tests/command_filters.rs Comment on lines +219 to +240 fn env_lock() -> EnvLock {
EnvLock::acquire()
}
#[cfg(windows)]
const GREP_STUB: &str = concat!(
"use std::io::{self, Read};\n",
"fn main() {\n",
" let mut args: Vec<String> = std::env::args().skip(1).collect();\n",
" let pattern = args.pop().expect(\"pattern\");\n",
" let mut input = String::new();\n",
" io::stdin().read_to_string(&mut input).expect(\"stdin\");\n",
" if pattern == \"^line2\" && input.contains(\"line2\") {\n",
" print!(\"line2\\n\");\n",
" } else {\n",
" eprintln!(\"pattern:{pattern} input:{input}\", pattern = pattern, input = input);\n",
" std::process::exit(1);\n",
" }\n",
"}\n",
);
#[cfg(windows)]❌ New issue: Code Duplication |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 9485 @leynos: I'll analyse the code duplication and suggest a refactoring. AnalysisThe duplication occurs between the Windows-specific tests
The existing Suggested FixExtend struct CommandFixture {
_temp: tempfile::TempDir,
env: Environment<'static>,
state: netsuke::stdlib::StdlibState,
command: String,
#[cfg(windows)]
_env_guards: Vec<test_support::EnvVarGuard>,
}
impl CommandFixture {
fn new(compiler: impl Fn(&Dir, &Utf8PathBuf, &str) -> Utf8PathBuf, binary: &str) -> Self {
let temp = tempdir().expect("tempdir");
let root = Utf8PathBuf::from_path_buf(temp.path().to_path_buf()).expect("utf8");
let dir = Dir::open_ambient_dir(&root, ambient_authority()).expect("dir");
let helper = compiler(&dir, &root, binary);
let command = format!("\"{}\"", helper.as_str());
let (env, state) = stdlib_env_with_state();
state.reset_impure();
Self {
_temp: temp,
env,
state,
command,
#[cfg(windows)]
_env_guards: Vec::new(),
}
}
#[cfg(windows)]
fn with_path_prefix(mut self) -> Self {
let root = Utf8PathBuf::from_path_buf(self._temp.path().to_path_buf()).expect("utf8");
let mut path_value = OsString::from(root.as_str());
path_value.push(";");
path_value.push(std::env::var_os("PATH").unwrap_or_default());
self._env_guards.push(EnvVarGuard::set("PATH", &path_value));
self
}
fn env(&mut self) -> &mut Environment<'static> {
&mut self.env
}
fn command(&self) -> &str {
&self.command
}
fn state(&self) -> &netsuke::stdlib::StdlibState {
&self.state
}
fn root_path(&self) -> Utf8PathBuf {
Utf8PathBuf::from_path_buf(self._temp.path().to_path_buf()).expect("utf8")
}
}Then refactor the Windows tests to use this extended fixture: #[cfg(windows)]
#[rstest]
fn grep_on_windows_bypasses_shell(_env_lock: EnvLock) {
let compiler = |dir: &Dir, root: &Utf8PathBuf, name: &str| {
compile_rust_helper(dir, root, name, GREP_STUB)
};
let mut fixture = CommandFixture::new(compiler, "grep").with_path_prefix();
fixture.env().add_template(
"grep_win",
r#"{{ 'line1\nline2\n' | grep('^line2') | trim }}"#,
).expect("template");
let template = fixture.env().get_template("grep_win").expect("get template");
let rendered = template.render(context! {}).expect("render");
assert_eq!(rendered, "line2");
assert!(fixture.state().is_impure(), "grep should mark template impure");
}
#[cfg(windows)]
#[rstest]
fn shell_preserves_cmd_meta_characters(_env_lock: EnvLock) {
let compiler = |dir: &Dir, root: &Utf8PathBuf, name: &str| {
compile_rust_helper(dir, root, name, ARGS_STUB)
};
let fixture = CommandFixture::new(compiler, "echo_args").with_path_prefix();
let command = format!("{} \"literal %%^!\"", fixture.command());
fixture.env().add_template("shell_meta", "{{ '' | shell(cmd) }}")
.expect("template");
let template = fixture.env().get_template("shell_meta").expect("get template");
let rendered = template.render(context!(cmd => command)).expect("render shell");
assert_eq!(rendered.trim(), "literal %^!");
assert!(fixture.state().is_impure(), "shell filter should mark template impure");
}AI Agent Prompt |
There was a problem hiding this comment.
Actionable comments posted: 4
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (7)
src/stdlib/command.rs(1 hunks)src/stdlib/network.rs(1 hunks)tests/cucumber.rs(3 hunks)tests/std_filter_tests/command_filters.rs(1 hunks)tests/std_filter_tests/network_functions.rs(1 hunks)tests/steps/mod.rs(1 hunks)tests/steps/stdlib_steps.rs(6 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
**/*.rs: Keep functions small with single responsibility; apply command-query separation
Function documentation must include clear examples; test documentation should omit redundant examples
No single Rust code file may exceed 400 lines; split long switches/dispatch tables; move large test data to external files
Disallow Clippy warnings; code must be clean under clippy
Fix warnings emitted during tests in code, do not silence them
Extract helper functions when a function is too long; maintain separation of concerns and CQRS
Group many related parameters into well-named structs
Consider using Arc for large error values to reduce data returned
Every module must begin with a module-level //! comment describing purpose and utility
Document public APIs with Rustdoc /// comments so cargo doc can generate docs
Prefer immutable data; avoid unnecessary mut bindings
Use Result for error handling instead of panicking where feasible
Avoid unsafe code unless absolutely necessary; document any usage clearly
Place function attributes after doc comments
Do not use return in single-line functions
Use predicate functions for conditionals with more than two branches
Do not silence lints except as a last resort
When suppressing lints, scope narrowly and include a clear reason
Prefer #[expect(...)] over #[allow(...)] for lint handling
When functions are unused for certain features, gate with #[cfg]/#[cfg_attr]
Prefer .expect() over .unwrap()
Use concat!() for long string literals instead of escaping newlines with backslashes
Prefer single-line function bodies where appropriate (e.g., pub fn new(id: u64) -> Self { Self(id) })
Use cap-std for filesystem operations; avoid std::fs directly
Use camino for path handling; avoid std::path::PathBuf directly
Prefer semantic error enums deriving std::error::Error via thiserror for inspectable conditions
Use opaque eyre::Report only at the app boundary; do not expose in public APIs
Never export opaque error types from libraries; convert to domain...
Files:
tests/steps/mod.rssrc/stdlib/network.rssrc/stdlib/command.rstests/std_filter_tests/network_functions.rstests/steps/stdlib_steps.rstests/cucumber.rstests/std_filter_tests/command_filters.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.rssrc/stdlib/network.rssrc/stdlib/command.rstests/std_filter_tests/network_functions.rstests/steps/stdlib_steps.rstests/cucumber.rstests/std_filter_tests/command_filters.rs
tests/**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
tests/**/*.rs: Write unit and behavioural tests for new functionality; run before and after changes
Use rstest fixtures for shared setup
Replace duplicated tests with #[rstest(...)] parameterised cases
Prefer mockall for mocks/stubs
Mock nondeterministic deps via dependency injection using mockable traits (e.g., Env, Clock); follow docs/reliable-testing-in-rust-via-dependency-injection.md
Files:
tests/steps/mod.rstests/std_filter_tests/network_functions.rstests/steps/stdlib_steps.rstests/cucumber.rstests/std_filter_tests/command_filters.rs
🧬 Code graph analysis (6)
src/stdlib/network.rs (1)
tests/std_filter_tests/command_filters.rs (2)
env(37-39)new(21-35)
src/stdlib/command.rs (2)
src/stdlib/mod.rs (1)
register(69-79)tests/std_filter_tests/command_filters.rs (4)
env(37-39)state(45-47)command(41-43)new(21-35)
tests/std_filter_tests/network_functions.rs (2)
tests/std_filter_tests/support.rs (1)
stdlib_env_with_state(22-26)src/stdlib/mod.rs (1)
is_impure(39-41)
tests/steps/stdlib_steps.rs (3)
test_support/src/command_helper.rs (2)
compile_failure_helper(68-70)compile_uppercase_helper(46-48)src/stdlib/command.rs (1)
register(56-74)src/stdlib/mod.rs (2)
register(69-79)is_impure(39-41)
tests/cucumber.rs (2)
tests/steps/stdlib_steps.rs (2)
spawn_http_server(106-125)server_host(102-104)test_support/src/env.rs (1)
restore_many(98-112)
tests/std_filter_tests/command_filters.rs (4)
test_support/src/command_helper.rs (3)
compile_failure_helper(68-70)compile_uppercase_helper(46-48)compile_rust_helper(98-114)tests/std_filter_tests/support.rs (1)
stdlib_env_with_state(22-26)src/stdlib/mod.rs (1)
is_impure(39-41)test_support/src/env_lock.rs (1)
acquire(17-21)
⏰ 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 (aarch64-apple-darwin, macos-14, macos-arm64, arm64)
- GitHub Check: release / build-macos (x86_64-apple-darwin, macos-13, macos-x86_64, x86_64)
- GitHub Check: release / build-linux (x86_64-unknown-linux-gnu, linux-amd64, amd64)
- GitHub Check: release / build-windows (aarch64-pc-windows-msvc, arm64, arm64)
- GitHub Check: release / build-windows (x86_64-pc-windows-msvc, amd64, x64)
- GitHub Check: build-test (ubuntu-latest, stable)
🔇 Additional comments (18)
tests/cucumber.rs (3)
66-72: LGTM!The early return via
let-elseand handle cleanup is correct and idiomatic.
74-81: LGTM!The helper successfully simplifies the conditional logic in
Drop::dropandstart_http_server, resolving the "Complex Conditional" code smell flagged in the PR objectives. The doc comment clearly explains the caller's usage.
83-89: LGTM!The early return optimisation and drain-collect pattern are correct. Environment restoration is properly isolated via the
test_support::env::restore_manyhelper.src/stdlib/command.rs (15)
76-88: LGTM!Input validation, error mapping, and command execution are correct. The trim check prevents empty commands from reaching the shell.
90-117: LGTM!Pattern validation and platform-specific dispatch (direct
grepinvocation on Windows, shell command on non-Windows) are correct. The conditional compilation ensures proper quoting for each platform.
119-144: LGTM!The match statement handles all relevant
ValueKindvariants: undefined values return empty args, sequences/iterables are mapped to strings, and scalar strings are wrapped in a vector. Error messages are clear.
146-164: LGTM!Platform-specific quoting (Windows via
windows_quote, non-Windows via POSIX quoting) ensures arguments with spaces or metacharacters are correctly escaped for each shell.
166-179: LGTM!The undefined check prevents filter failures on missing values, and the fallback to
to_string()ensures non-byte values are converted correctly.
181-190: LGTM!Shell invocation with piped stdio is correct. The command string is passed as a single argument to the shell's
-cor/Cflag.
192-201: LGTM!Direct program invocation on Windows (avoiding
cmd.exeforgrep) ensures arguments are not misinterpreted by the shell.
203-250: LGTM!Process management is robust:
- Broken pipe errors are caught and preserved (lines 205-217, 234-240), allowing the command to exit early if it rejects input (e.g.,
grepwith invalid flags).- Pipe readers are spawned in separate threads (lines 219-220) to prevent deadlocks when stdout/stderr buffers fill.
- Timeout handling (line 222) prevents indefinite hangs.
- Reader handles are joined after the process exits (lines 231-232), ensuring all output is captured.
252-257: LGTM!UTF-8 decoding with fallback to raw bytes ensures all command output is representable as a
Value, regardless of encoding.
259-312: LGTM!Error messages include full context (command, template, status/signal, stderr) for debugging. The broken pipe variant (lines 276-291) disambiguates early command exit from write failures.
314-333: LGTM!The
CommandFailureenum covers all failure modes: spawn errors, I/O errors, broken pipes (with status and stderr), exit failures, and timeouts. TheFrom<io::Error>implementation simplifies error propagation.
335-347: LGTM!Timeout handling is correct: if
wait_timeoutreturnsNone(timeout), the child is killed andTimeoutis returned. TheInvalidInputerror check (line 340) correctly handles the case where the process has already exited.
349-371: LGTM!Pipe readers are spawned in separate threads to prevent deadlocks. The join logic (lines 363-370) handles thread panics by mapping them to I/O errors.
373-380: LGTM!The stderr trimming and conditional append ensure error messages include diagnostic output without excessive whitespace.
362-371: Confirm MSRV supports io::Error::other. Cargo.toml sets rust-version = "1.89.0" (≥1.74), so no replacement required.
| pub(crate) fn register(env: &mut minijinja::Environment<'_>, impure: Arc<AtomicBool>) { | ||
| let shell_flag = Arc::clone(&impure); | ||
| env.add_filter( | ||
| "shell", | ||
| move |state: &State, value: Value, command: String| { | ||
| shell_flag.store(true, Ordering::Relaxed); | ||
| execute_shell(state, &value, &command) | ||
| }, | ||
| ); | ||
|
|
||
| let grep_flag = impure; | ||
| env.add_filter( | ||
| "grep", | ||
| move |state: &State, value: Value, pattern: String, flags: Option<Value>| { | ||
| grep_flag.store(true, Ordering::Relaxed); | ||
| execute_grep(state, &value, &pattern, flags) | ||
| }, | ||
| ); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Add doc comment for register function.
The coding guidelines require that public APIs (including pub(crate) functions) have doc comments so cargo doc can generate documentation. Document the purpose of this function, the impure flag's role in marking templates impure, and the filters it registers.
Add this doc comment:
+/// Registers shell-oriented filters in the MiniJinja environment.
+///
+/// The `shell` filter executes arbitrary shell commands and returns their
+/// stdout. The `grep` filter searches input text using the `grep` utility.
+/// Both filters mark the provided `impure` flag to signal that template
+/// evaluation has side effects and should not be cached.
+///
+/// # Security
+///
+/// Only use these filters with trusted templates. See module-level docs.
pub(crate) fn register(env: &mut minijinja::Environment<'_>, impure: Arc<AtomicBool>) {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| pub(crate) fn register(env: &mut minijinja::Environment<'_>, impure: Arc<AtomicBool>) { | |
| let shell_flag = Arc::clone(&impure); | |
| env.add_filter( | |
| "shell", | |
| move |state: &State, value: Value, command: String| { | |
| shell_flag.store(true, Ordering::Relaxed); | |
| execute_shell(state, &value, &command) | |
| }, | |
| ); | |
| let grep_flag = impure; | |
| env.add_filter( | |
| "grep", | |
| move |state: &State, value: Value, pattern: String, flags: Option<Value>| { | |
| grep_flag.store(true, Ordering::Relaxed); | |
| execute_grep(state, &value, &pattern, flags) | |
| }, | |
| ); | |
| } | |
| /// Registers shell-oriented filters in the MiniJinja environment. | |
| /// | |
| /// The `shell` filter executes arbitrary shell commands and returns their | |
| /// stdout. The `grep` filter searches input text using the `grep` utility. | |
| /// Both filters mark the provided `impure` flag to signal that template | |
| /// evaluation has side effects and should not be cached. | |
| /// | |
| /// # Security | |
| /// | |
| /// Only use these filters with trusted templates. See module-level docs. | |
| pub(crate) fn register(env: &mut minijinja::Environment<'_>, impure: Arc<AtomicBool>) { | |
| let shell_flag = Arc::clone(&impure); | |
| env.add_filter( | |
| "shell", | |
| move |state: &State, value: Value, command: String| { | |
| shell_flag.store(true, Ordering::Relaxed); | |
| execute_shell(state, &value, &command) | |
| }, | |
| ); | |
| let grep_flag = impure; | |
| env.add_filter( | |
| "grep", | |
| move |state: &State, value: Value, pattern: String, flags: Option<Value>| { | |
| grep_flag.store(true, Ordering::Relaxed); | |
| execute_grep(state, &value, &pattern, flags) | |
| }, | |
| ); | |
| } |
🤖 Prompt for AI Agents
In src/stdlib/command.rs around lines 56 to 74, the register function lacks a
doc comment; add a triple-slash (///) Rust doc comment immediately above
pub(crate) fn register explaining that this function registers the "shell" and
"grep" Minijinja filters on the provided Environment, describing the role of the
impure Arc<AtomicBool> parameter (it is set true when either filter is used to
mark templates as impure), and briefly summarizing behavior of each registered
filter (shell executes shell commands, grep filters values by pattern and
optional flags); keep the comment concise and idiomatic for cargo doc
generation.
| impl CliWorld { | ||
| pub(crate) fn start_http_server(&mut self, body: String) { | ||
| if let Some(host) = self.extract_host_from_stdlib_url() { | ||
| let _ = TcpStream::connect(host); | ||
| } | ||
| self.shutdown_http_server(); | ||
| let (url, handle) = spawn_http_server(body); | ||
| self.stdlib_url = Some(url); | ||
| self.http_server = Some(handle); | ||
| } |
There was a problem hiding this comment.
Guard against hangs if URL parsing fails.
The unblocking logic (lines 57-59) assumes that when http_server is Some, extract_host_from_stdlib_url succeeds. If URL parsing fails unexpectedly (e.g., malformed URL from a future refactor), shutdown_http_server (line 60) calls join() without unblocking the listener, causing the test suite to hang indefinitely. Add a defensive check: only call shutdown_http_server if the host extraction succeeds, or .expect() on the host extraction with a clear panic message that pinpoints the root cause.
Apply this diff to fail fast on URL parsing errors:
pub(crate) fn start_http_server(&mut self, body: String) {
if let Some(host) = self.extract_host_from_stdlib_url() {
let _ = TcpStream::connect(host);
+ } else if self.http_server.is_some() {
+ panic!(
+ "Cannot extract host from stdlib_url; server teardown will hang. URL: {:?}",
+ self.stdlib_url
+ );
}
self.shutdown_http_server();📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| impl CliWorld { | |
| pub(crate) fn start_http_server(&mut self, body: String) { | |
| if let Some(host) = self.extract_host_from_stdlib_url() { | |
| let _ = TcpStream::connect(host); | |
| } | |
| self.shutdown_http_server(); | |
| let (url, handle) = spawn_http_server(body); | |
| self.stdlib_url = Some(url); | |
| self.http_server = Some(handle); | |
| } | |
| impl CliWorld { | |
| pub(crate) fn start_http_server(&mut self, body: String) { | |
| if let Some(host) = self.extract_host_from_stdlib_url() { | |
| let _ = TcpStream::connect(host); | |
| } else if self.http_server.is_some() { | |
| panic!( | |
| "Cannot extract host from stdlib_url; server teardown will hang. URL: {:?}", | |
| self.stdlib_url | |
| ); | |
| } | |
| self.shutdown_http_server(); | |
| let (url, handle) = spawn_http_server(body); | |
| self.stdlib_url = Some(url); | |
| self.http_server = Some(handle); | |
| } | |
| } |
🤖 Prompt for AI Agents
In tests/cucumber.rs around lines 55–64, the current code calls
shutdown_http_server unconditionally after attempting to connect to the host
extracted from stdlib_url, which can hang if extract_host_from_stdlib_url fails;
update the logic so you either (A) only call shutdown_http_server inside the
same if-let block that succeeded in extracting the host (i.e., move the shutdown
call into that branch) or (B) change the host extraction to use .expect("failed
to parse stdlib_url host for unblocking; ensure stdlib_url is valid") so the
test fails fast with a clear message if URL parsing fails.
| impl Drop for CliWorld { | ||
| fn drop(&mut self) { | ||
| if !self.env_vars.is_empty() { | ||
| restore_many(self.env_vars.drain().collect()); | ||
| if let Some(host) = self.extract_host_from_stdlib_url() { | ||
| let _ = TcpStream::connect(host); | ||
| } | ||
| self.shutdown_http_server(); | ||
| self.restore_environment(); | ||
| } | ||
| } |
There was a problem hiding this comment.
Guard against hangs in Drop.
The same issue from start_http_server applies here: if extract_host_from_stdlib_url returns None whilst http_server is Some, shutdown_http_server (line 110) calls join() without unblocking the listener, causing the destructor to hang indefinitely. Since Drop::drop cannot panic (undefined behaviour), add a defensive check: log a warning or skip shutdown entirely if host extraction fails.
Apply this diff to prevent hangs in Drop:
impl Drop for CliWorld {
fn drop(&mut self) {
if let Some(host) = self.extract_host_from_stdlib_url() {
let _ = TcpStream::connect(host);
+ } else if self.http_server.is_some() {
+ eprintln!(
+ "Warning: Cannot extract host from stdlib_url; skipping server shutdown to avoid hang. URL: {:?}",
+ self.stdlib_url
+ );
+ // Leak the handle rather than blocking forever
+ let _ = self.http_server.take();
+ self.restore_environment();
+ return;
}
self.shutdown_http_server();📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| impl Drop for CliWorld { | |
| fn drop(&mut self) { | |
| if !self.env_vars.is_empty() { | |
| restore_many(self.env_vars.drain().collect()); | |
| if let Some(host) = self.extract_host_from_stdlib_url() { | |
| let _ = TcpStream::connect(host); | |
| } | |
| self.shutdown_http_server(); | |
| self.restore_environment(); | |
| } | |
| } | |
| impl Drop for CliWorld { | |
| fn drop(&mut self) { | |
| if let Some(host) = self.extract_host_from_stdlib_url() { | |
| let _ = TcpStream::connect(host); | |
| } else if self.http_server.is_some() { | |
| eprintln!( | |
| "Warning: cannot extract host from stdlib_url; skipping server shutdown to avoid hang. URL: {:?}", | |
| self.stdlib_url | |
| ); | |
| // Leak the handle rather than blocking forever | |
| let _ = self.http_server.take(); | |
| self.restore_environment(); | |
| return; | |
| } | |
| self.shutdown_http_server(); | |
| self.restore_environment(); | |
| } | |
| } |
🤖 Prompt for AI Agents
In tests/cucumber.rs around lines 105 to 113, the Drop impl for CliWorld can
hang if extract_host_from_stdlib_url() returns None while http_server is Some
because shutdown_http_server() will call join() without unblocking the listener;
change the Drop logic to first check extract_host_from_stdlib_url(), and only
attempt the TcpStream::connect + shutdown_http_server() sequence when you get
Some(host); if extract_host_from_stdlib_url() returns None, log a warning and
skip calling shutdown_http_server() (do not panic) so the destructor cannot
block indefinitely.
| #[allow( | ||
| clippy::collapsible_if, | ||
| reason = "Review requested explicit nested host extraction checks" | ||
| )] |
There was a problem hiding this comment.
Replace the forbidden #[allow] with #[expect].
Stop using #[allow] here; the coding rules demand #[expect(..., reason = "...")] for lint handling.
Apply this diff to comply:
-#[allow(
+#[expect(
clippy::collapsible_if,
reason = "Review requested explicit nested host extraction checks"
)]As per coding guidelines
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| #[allow( | |
| clippy::collapsible_if, | |
| reason = "Review requested explicit nested host extraction checks" | |
| )] | |
| #[expect( | |
| clippy::collapsible_if, | |
| reason = "Review requested explicit nested host extraction checks" | |
| )] |
🤖 Prompt for AI Agents
In tests/steps/stdlib_steps.rs around lines 204 to 207, the code uses
#[allow(clippy::collapsible_if, reason = "Review requested explicit nested host
extraction checks")]; replace this forbidden #[allow] with
#[expect(clippy::collapsible_if, reason = "Review requested explicit nested host
extraction checks")] so the lint is handled via #[expect] with the same lint
name and reason; update the attribute accordingly and ensure formatting matches
surrounding attributes.
* Use Path.open in Windows output normaliser * Parametrise staging config validation tests * Refactor staging tests setup * Normalise CLI inputs and safe staging logging * Extract staging duplicate validation helpers * Re-export glob helpers from staging * Simplify release uploader CLI fallbacks
There was a problem hiding this comment.
Gates Failed
(1 file with Bumpy Road Ahead)
(6 files with Complex Method, Excess Number of Function Arguments, Code Duplication)
Gates Passed
See analysis details in CodeScene
Reason for failure
| Enforce critical code health rules | Violations | Code Health Impact | |
|---|---|---|---|
| glob_utils.py | 1 critical rule | 9.54 | Suppress |
| Enforce advisory code health rules | Violations | Code Health Impact | |
|---|---|---|---|
| test_stage_common_staging.py | 2 advisory rules | 9.10 | Suppress |
| glob_utils.py | 1 advisory rule | 9.54 | Suppress |
| staging.py | 1 advisory rule | 9.54 | Suppress |
| config.py | 1 advisory rule | 9.69 | Suppress |
| _release_upload_deps.py | 1 advisory rule | 9.69 | Suppress |
| test_stage_common_config.py | 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.
| def _normalise_alternatives( | ||
| value: object, index: int, config_path: Path | ||
| ) -> list[str]: | ||
| """Return ``value`` as a list of alternative glob patterns.""" | ||
|
|
||
| if value is None: | ||
| return [] | ||
| if isinstance(value, str): | ||
| return [value] if value else [] | ||
| if not isinstance(value, list): | ||
| message = ( | ||
| "Alternatives must be a list of strings " | ||
| f"(entry #{index} in {config_path})" | ||
| ) | ||
| raise StageError(message) | ||
| alternatives: list[str] = [] | ||
| for alternative in value: | ||
| if not isinstance(alternative, str): | ||
| message = ( | ||
| "Alternatives must be strings " | ||
| f"(entry #{index} in {config_path})" | ||
| ) | ||
| raise StageError(message) | ||
| if alternative: | ||
| alternatives.append(alternative) | ||
| return alternatives |
There was a problem hiding this comment.
❌ New issue: Complex Method
_normalise_alternatives has a cyclomatic complexity of 9, threshold = 9
| def match_candidate_path(workspace: Path, rendered: str) -> Path | None: | ||
| """Return the newest path matching ``rendered`` relative to ``workspace``.""" | ||
|
|
||
| candidate = Path(rendered) | ||
| base = candidate if candidate.is_absolute() else workspace / candidate | ||
| if any(ch in rendered for ch in "*?[]"): | ||
| if candidate.is_absolute(): | ||
| root_text, pattern = glob_root_and_pattern(candidate) | ||
| root = Path(root_text) | ||
| candidates = [ | ||
| path for path in root.glob(pattern) if path.is_file() | ||
| ] | ||
| else: | ||
| windows_candidate = PureWindowsPath(rendered) | ||
| if windows_candidate.is_absolute(): | ||
| anchor = Path(windows_candidate.anchor) | ||
| relative = PureWindowsPath(*windows_candidate.parts[1:]).as_posix() | ||
| candidates = [ | ||
| path for path in anchor.glob(relative) if path.is_file() | ||
| ] | ||
| else: | ||
| candidates = [ | ||
| path for path in workspace.glob(rendered) if path.is_file() | ||
| ] | ||
| return max(candidates, key=_mtime_key) if candidates else None | ||
| return base if base.is_file() else None |
There was a problem hiding this comment.
❌ New issue: Complex Method
match_candidate_path has a cyclomatic complexity of 10, threshold = 9
| def match_candidate_path(workspace: Path, rendered: str) -> Path | None: | ||
| """Return the newest path matching ``rendered`` relative to ``workspace``.""" | ||
|
|
||
| candidate = Path(rendered) | ||
| base = candidate if candidate.is_absolute() else workspace / candidate | ||
| if any(ch in rendered for ch in "*?[]"): | ||
| if candidate.is_absolute(): | ||
| root_text, pattern = glob_root_and_pattern(candidate) | ||
| root = Path(root_text) | ||
| candidates = [ | ||
| path for path in root.glob(pattern) if path.is_file() | ||
| ] | ||
| else: | ||
| windows_candidate = PureWindowsPath(rendered) | ||
| if windows_candidate.is_absolute(): | ||
| anchor = Path(windows_candidate.anchor) | ||
| relative = PureWindowsPath(*windows_candidate.parts[1:]).as_posix() | ||
| candidates = [ | ||
| path for path in anchor.glob(relative) if path.is_file() | ||
| ] | ||
| else: | ||
| candidates = [ | ||
| path for path in workspace.glob(rendered) if path.is_file() | ||
| ] | ||
| return max(candidates, key=_mtime_key) if candidates else None | ||
| return base if base.is_file() else None |
There was a problem hiding this comment.
❌ New issue: Bumpy Road Ahead
match_candidate_path has 2 blocks with nested conditional logic. Any nesting of 2 or deeper is considered. Threshold is 2 blocks per function
| def stage_artefacts(config: StagingConfig, github_output_file: Path) -> StageResult: | ||
| """Copy artefacts into ``config``'s staging directory. | ||
|
|
||
| Parameters | ||
| ---------- | ||
| config : StagingConfig | ||
| Fully resolved configuration describing the artefacts to stage. | ||
| github_output_file : Path | ||
| Path to the ``GITHUB_OUTPUT`` file used to export workflow outputs. | ||
|
|
||
| Returns | ||
| ------- | ||
| StageResult | ||
| Summary object describing the staging directory, staged artefacts, | ||
| exported outputs, and checksum digests. | ||
|
|
||
| Raises | ||
| ------ | ||
| StageError | ||
| Raised when required artefacts are missing or configuration templates | ||
| render invalid destinations. | ||
|
|
||
| Examples | ||
| -------- | ||
| >>> from pathlib import Path # doctest: +SKIP | ||
| >>> from stage_common.config import load_config # doctest: +SKIP | ||
| >>> cfg = load_config( # doctest: +SKIP | ||
| ... Path(".github/release-staging.toml"), | ||
| ... "linux-x86_64", | ||
| ... ) | ||
| >>> result = stage_artefacts(cfg, Path("github_output.txt")) # doctest: +SKIP | ||
| >>> result.staged_artefacts[0].name # doctest: +SKIP | ||
| 'netsuke' | ||
| """ | ||
| staging_dir = config.staging_dir() | ||
| context = config.as_template_context() | ||
|
|
||
| if staging_dir.exists(): | ||
| shutil.rmtree(staging_dir) | ||
| staging_dir.mkdir(parents=True, exist_ok=True) | ||
|
|
||
| staged_paths: list[Path] = [] | ||
| outputs: dict[str, Path] = {} | ||
| checksums: dict[str, str] = {} | ||
| seen_destinations: set[Path] = set() | ||
| seen_outputs: set[str] = set() | ||
|
|
||
| for artefact in config.artefacts: | ||
| outcome = _stage_single_artefact( | ||
| config=config, | ||
| artefact=artefact, | ||
| staging_dir=staging_dir, | ||
| context=context, | ||
| ) | ||
| if outcome is None: | ||
| continue | ||
| _validate_and_record_destination( | ||
| outcome.path, artefact.source, seen_destinations | ||
| ) | ||
| staged_paths.append(outcome.path) | ||
| checksums[outcome.path.name] = outcome.digest | ||
| if outcome.output_key: | ||
| _validate_and_record_output( | ||
| outcome.output_key, outcome.path, seen_outputs, outputs | ||
| ) | ||
|
|
||
| if not staged_paths: | ||
| message = "No artefacts were staged." | ||
| raise StageError(message) | ||
|
|
||
| staged_file_names = [path.name for path in sorted(staged_paths)] | ||
| artefact_map_json = json.dumps( | ||
| {key: path.as_posix() for key, path in sorted(outputs.items())} | ||
| ) | ||
| checksum_map_json = json.dumps(dict(sorted(checksums.items()))) | ||
|
|
||
| if colliding_keys := RESERVED_OUTPUT_KEYS & outputs.keys(): | ||
| message = ( | ||
| "Artefact outputs collide with reserved keys: " | ||
| f"{sorted(colliding_keys)}. Please rename these keys in your " | ||
| "artefact configuration to avoid using reserved names: " | ||
| f"{sorted(RESERVED_OUTPUT_KEYS)}." | ||
| ) | ||
| raise StageError(message) | ||
|
|
||
| exported_outputs: dict[str, str | list[str]] = { | ||
| "artifact_dir": staging_dir.as_posix(), | ||
| "dist_dir": staging_dir.parent.as_posix(), | ||
| "staged_files": staged_file_names, | ||
| "artefact_map": artefact_map_json, | ||
| "checksum_map": checksum_map_json, | ||
| } | {key: path.as_posix() for key, path in outputs.items()} | ||
|
|
||
| write_github_output(github_output_file, exported_outputs) | ||
|
|
||
| return StageResult(staging_dir, staged_paths, outputs, checksums) |
There was a problem hiding this comment.
❌ New issue: Complex Method
stage_artefacts has a cyclomatic complexity of 12, threshold = 9
| def run_cli( | ||
| support: CycloptsSupport, | ||
| *, | ||
| coerce_bool: Callable[[object], bool], | ||
| main: Callable[..., int], | ||
| tokens: Sequence[str] | None = None, | ||
| ) -> int: | ||
| """Execute the uploader CLI using Cyclopts or ``argparse`` fallback.""" | ||
|
|
||
| arguments = list(tokens) if tokens is not None else None | ||
|
|
||
| if support.available: | ||
| return support.app(arguments) | ||
|
|
||
| parser = ArgumentParser(description=main.__doc__ or "Upload release assets.") | ||
| parser.add_argument("--release-tag") | ||
| parser.add_argument("--bin-name") | ||
| parser.add_argument("--dist-dir") | ||
| parser.add_argument("--dry-run", action="store_true") | ||
| args = parser.parse_args(arguments if arguments is not None else sys.argv[1:]) | ||
|
|
||
| release_tag = args.release_tag or os.environ.get("INPUT_RELEASE_TAG") | ||
| bin_name = args.bin_name or os.environ.get("INPUT_BIN_NAME") | ||
| dist_dir_value = args.dist_dir or os.environ.get("INPUT_DIST_DIR") or "dist" | ||
| dry_run_flag = args.dry_run | ||
|
|
||
| if not dry_run_flag and (env_flag := os.environ.get("INPUT_DRY_RUN")): | ||
| dry_run_flag = coerce_bool(env_flag) | ||
|
|
||
| missing = [ | ||
| label | ||
| for label, present in ( | ||
| ("--release-tag", release_tag), | ||
| ("--bin-name", bin_name), | ||
| ) | ||
| if not present | ||
| ] | ||
| if missing: | ||
| joined = ", ".join(missing) | ||
| parser.exit(status=1, message=f"Missing required argument(s): {joined}\n") | ||
|
|
||
| return main( | ||
| release_tag=release_tag, | ||
| bin_name=bin_name, | ||
| dist_dir=Path(dist_dir_value), | ||
| dry_run=dry_run_flag, | ||
| ) |
There was a problem hiding this comment.
❌ New issue: Complex Method
run_cli has a cyclomatic complexity of 11, threshold = 9
| def test_load_config_validation_errors( | ||
| stage_common: object, | ||
| workspace: Path, | ||
| config_content: str, | ||
| error_substring: str, | ||
| test_id: str, | ||
| ) -> None: | ||
| """``load_config`` should raise ``StageError`` for invalid configurations.""" | ||
|
|
||
| config_file = workspace / "release-staging.toml" | ||
| config_file.write_text(config_content, encoding="utf-8") | ||
|
|
||
| with pytest.raises(stage_common.StageError) as exc: | ||
| stage_common.load_config(config_file, "test") | ||
|
|
||
| message = str(exc.value) | ||
| assert error_substring in message, f"{test_id} missing expected substring" | ||
|
|
||
| if test_id == "requires_common_bin_name": | ||
| assert "[common]" in message | ||
| elif test_id == "requires_target_platform": | ||
| assert "[targets.test]" in message | ||
| elif test_id == "requires_artefact_source": | ||
| assert "entry #1" in message |
There was a problem hiding this comment.
❌ New issue: Excess Number of Function Arguments
test_load_config_validation_errors has 5 arguments, max arguments = 4
| def test_stage_artefacts_uses_alternative_glob( | ||
| stage_common: object, prepared_workspace: Path, standard_target: str | ||
| ) -> None: | ||
| """Fallback paths should be used when the preferred template is absent.""" | ||
|
|
||
| generated = ( | ||
| prepared_workspace | ||
| / "target" | ||
| / "generated-man" | ||
| / standard_target | ||
| / "release" | ||
| / "netsuke.1" | ||
| ) | ||
| generated.unlink() | ||
|
|
||
| build_dir = ( | ||
| prepared_workspace / "target" / standard_target / "release" / "build" | ||
| ) | ||
| first = build_dir / "1" / "out" / "netsuke.1" | ||
| second = build_dir / "2" / "out" / "netsuke.1" | ||
| first.parent.mkdir(parents=True, exist_ok=True) | ||
| second.parent.mkdir(parents=True, exist_ok=True) | ||
| first.write_text(".TH 1", encoding="utf-8") | ||
| second.write_text(".TH 2", encoding="utf-8") | ||
| os.utime(first, (first.stat().st_atime, first.stat().st_mtime - 100)) | ||
|
|
||
| config = create_staging_config( | ||
| stage_common, | ||
| prepared_workspace, | ||
| artefacts=[ | ||
| stage_common.ArtefactConfig( | ||
| source="target/generated-man/{target}/release/{bin_name}.1", | ||
| required=True, | ||
| output="man_path", | ||
| alternatives=["target/{target}/release/build/*/out/{bin_name}.1"], | ||
| ), | ||
| ], | ||
| target=standard_target, | ||
| ) | ||
|
|
||
| github_output = prepared_workspace / "outputs.txt" | ||
| result = stage_common.stage_artefacts(config, github_output) | ||
| staged_path = result.outputs["man_path"] | ||
| assert ( | ||
| staged_path.read_text(encoding="utf-8") == ".TH 2" | ||
| ), "Fallback glob should pick the newest man page" |
There was a problem hiding this comment.
❌ New issue: Code Duplication
The module contains 2 functions with similar structure: test_stage_artefacts_glob_selects_newest_candidate,test_stage_artefacts_uses_alternative_glob
| def test_stage_artefacts_validation_errors( | ||
| stage_common: object, | ||
| prepared_workspace: Path, | ||
| standard_target: str, | ||
| artefact_definitions: list[dict[str, object]], | ||
| expected_match: str, | ||
| ) -> None: | ||
| """Staging should fail fast when reserved names or duplicates are present.""" | ||
|
|
||
| artefacts = [ | ||
| stage_common.ArtefactConfig(**definition) | ||
| for definition in artefact_definitions | ||
| ] | ||
|
|
||
| config = create_staging_config( | ||
| stage_common, | ||
| prepared_workspace, | ||
| artefacts=artefacts, | ||
| target=standard_target, | ||
| ) | ||
|
|
||
| with pytest.raises(stage_common.StageError, match=expected_match): | ||
| stage_common.stage_artefacts(config, prepared_workspace / "outputs.txt") |
There was a problem hiding this comment.
❌ New issue: Excess Number of Function Arguments
test_stage_artefacts_validation_errors has 5 arguments, max arguments = 4
There was a problem hiding this comment.
Actionable comments posted: 9
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (7)
src/stdlib/command.rs(1 hunks)src/stdlib/network.rs(1 hunks)tests/cucumber.rs(3 hunks)tests/std_filter_tests/command_filters.rs(1 hunks)tests/std_filter_tests/network_functions.rs(1 hunks)tests/steps/mod.rs(1 hunks)tests/steps/stdlib_steps.rs(6 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
**/*.rs: Keep functions small with single responsibility; apply command-query separation
Function documentation must include clear examples; test documentation should omit redundant examples
No single Rust code file may exceed 400 lines; split long switches/dispatch tables; move large test data to external files
Disallow Clippy warnings; code must be clean under clippy
Fix warnings emitted during tests in code, do not silence them
Extract helper functions when a function is too long; maintain separation of concerns and CQRS
Group many related parameters into well-named structs
Consider using Arc for large error values to reduce data returned
Every module must begin with a module-level //! comment describing purpose and utility
Document public APIs with Rustdoc /// comments so cargo doc can generate docs
Prefer immutable data; avoid unnecessary mut bindings
Use Result for error handling instead of panicking where feasible
Avoid unsafe code unless absolutely necessary; document any usage clearly
Place function attributes after doc comments
Do not use return in single-line functions
Use predicate functions for conditionals with more than two branches
Do not silence lints except as a last resort
When suppressing lints, scope narrowly and include a clear reason
Prefer #[expect(...)] over #[allow(...)] for lint handling
When functions are unused for certain features, gate with #[cfg]/#[cfg_attr]
Prefer .expect() over .unwrap()
Use concat!() for long string literals instead of escaping newlines with backslashes
Prefer single-line function bodies where appropriate (e.g., pub fn new(id: u64) -> Self { Self(id) })
Use cap-std for filesystem operations; avoid std::fs directly
Use camino for path handling; avoid std::path::PathBuf directly
Prefer semantic error enums deriving std::error::Error via thiserror for inspectable conditions
Use opaque eyre::Report only at the app boundary; do not expose in public APIs
Never export opaque error types from libraries; convert to domain...
Files:
src/stdlib/command.rstests/std_filter_tests/command_filters.rssrc/stdlib/network.rstests/steps/mod.rstests/std_filter_tests/network_functions.rstests/steps/stdlib_steps.rstests/cucumber.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/command.rstests/std_filter_tests/command_filters.rssrc/stdlib/network.rstests/steps/mod.rstests/std_filter_tests/network_functions.rstests/steps/stdlib_steps.rstests/cucumber.rs
tests/**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
tests/**/*.rs: Write unit and behavioural tests for new functionality; run before and after changes
Use rstest fixtures for shared setup
Replace duplicated tests with #[rstest(...)] parameterised cases
Prefer mockall for mocks/stubs
Mock nondeterministic deps via dependency injection using mockable traits (e.g., Env, Clock); follow docs/reliable-testing-in-rust-via-dependency-injection.md
Files:
tests/std_filter_tests/command_filters.rstests/steps/mod.rstests/std_filter_tests/network_functions.rstests/steps/stdlib_steps.rstests/cucumber.rs
🧬 Code graph analysis (6)
src/stdlib/command.rs (2)
src/stdlib/mod.rs (1)
register(69-79)tests/std_filter_tests/command_filters.rs (4)
env(37-39)state(45-47)command(41-43)new(21-35)
tests/std_filter_tests/command_filters.rs (4)
test_support/src/command_helper.rs (3)
compile_failure_helper(68-70)compile_uppercase_helper(46-48)compile_rust_helper(98-114)tests/std_filter_tests/support.rs (1)
stdlib_env_with_state(22-26)src/stdlib/mod.rs (1)
is_impure(39-41)test_support/src/env_lock.rs (1)
acquire(17-21)
src/stdlib/network.rs (1)
tests/std_filter_tests/command_filters.rs (2)
env(37-39)new(21-35)
tests/std_filter_tests/network_functions.rs (2)
tests/std_filter_tests/support.rs (1)
stdlib_env_with_state(22-26)src/stdlib/mod.rs (1)
is_impure(39-41)
tests/steps/stdlib_steps.rs (3)
test_support/src/command_helper.rs (2)
compile_failure_helper(68-70)compile_uppercase_helper(46-48)src/stdlib/command.rs (1)
register(56-74)src/stdlib/mod.rs (2)
register(69-79)is_impure(39-41)
tests/cucumber.rs (2)
tests/steps/stdlib_steps.rs (2)
spawn_http_server(106-125)server_host(102-104)test_support/src/env.rs (1)
restore_many(98-112)
⏰ 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). (7)
- GitHub Check: release / build-macos (aarch64-apple-darwin, macos-14, macos-arm64, arm64)
- GitHub Check: release / build-macos (x86_64-apple-darwin, macos-13, macos-x86_64, x86_64)
- GitHub Check: release / build-linux (aarch64-unknown-linux-gnu, linux-arm64, arm64)
- GitHub Check: release / build-linux (x86_64-unknown-linux-gnu, linux-amd64, amd64)
- GitHub Check: release / build-windows (aarch64-pc-windows-msvc, arm64, arm64)
- GitHub Check: release / build-windows (x86_64-pc-windows-msvc, amd64, x64)
- GitHub Check: build-test (ubuntu-latest, stable)
🔇 Additional comments (4)
tests/steps/mod.rs (1)
13-13: LGTMExpose stdlib_steps to the crate; matches the new shared state usage.
tests/steps/stdlib_steps.rs (3)
106-125: LGTM: Avoid panics/noisy logs in server thread.Only write a response when bytes were read and ignore write errors; this unblocks zero‑byte connects cleanly.
106-126: Keep spawn_http_server; it’s used in tests/cucumber.rs.
239-253: Resolve env mutation helper guards mutations
test_support::env::set_var acquires EnvLock to serialise environment changes, adhering to test safety policy.
| fn run_child(mut command: Command, input: &[u8]) -> Result<Vec<u8>, CommandFailure> { | ||
| let mut child = command.spawn().map_err(CommandFailure::Spawn)?; | ||
| let mut broken_pipe = None; | ||
| if let Some(mut stdin) = child.stdin.take() { | ||
| match stdin.write_all(input) { | ||
| Ok(()) => {} | ||
| Err(err) => { | ||
| if err.kind() == io::ErrorKind::BrokenPipe { | ||
| broken_pipe = Some(err); | ||
| } else { | ||
| return Err(CommandFailure::Io(err)); | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| let mut stdout_reader = spawn_pipe_reader(child.stdout.take()); | ||
| let mut stderr_reader = spawn_pipe_reader(child.stderr.take()); | ||
|
|
||
| let status = match wait_for_exit(&mut child, COMMAND_TIMEOUT) { | ||
| Ok(status) => status, | ||
| Err(err) => { | ||
| let _ = join_reader(stdout_reader.take()); | ||
| let _ = join_reader(stderr_reader.take()); | ||
| return Err(err); | ||
| } | ||
| }; | ||
|
|
||
| let stdout = join_reader(stdout_reader.take()).map_err(CommandFailure::Io)?; | ||
| let stderr = join_reader(stderr_reader.take()).map_err(CommandFailure::Io)?; | ||
|
|
||
| if let Some(err) = broken_pipe { | ||
| return Err(CommandFailure::BrokenPipe { | ||
| source: err, | ||
| status: status.code(), | ||
| stderr, | ||
| }); | ||
| } | ||
|
|
||
| if status.success() { | ||
| Ok(stdout) | ||
| } else { | ||
| Err(CommandFailure::Exit { | ||
| status: status.code(), | ||
| stderr, | ||
| }) | ||
| } | ||
| } |
There was a problem hiding this comment.
Eliminate deadlock when writing to child stdin
Do not block on stdin.write_all before starting readers and before applying the timeout. A child that doesn’t read stdin and writes enough to stdout/stderr can deadlock this thread forever (timeout never triggers). Spawn stdout/stderr readers first and write on a dedicated thread; always join the writer and readers on exit/timeout.
Apply this diff:
@@
-fn run_child(mut command: Command, input: &[u8]) -> Result<Vec<u8>, CommandFailure> {
- let mut child = command.spawn().map_err(CommandFailure::Spawn)?;
- let mut broken_pipe = None;
- if let Some(mut stdin) = child.stdin.take() {
- match stdin.write_all(input) {
- Ok(()) => {}
- Err(err) => {
- if err.kind() == io::ErrorKind::BrokenPipe {
- broken_pipe = Some(err);
- } else {
- return Err(CommandFailure::Io(err));
- }
- }
- }
- }
-
- let mut stdout_reader = spawn_pipe_reader(child.stdout.take());
- let mut stderr_reader = spawn_pipe_reader(child.stderr.take());
+fn run_child(mut command: Command, input: &[u8]) -> Result<Vec<u8>, CommandFailure> {
+ let mut child = command.spawn().map_err(CommandFailure::Spawn)?;
+ // Start readers first to avoid pipe back‑pressure deadlocks.
+ let mut stdout_reader = spawn_pipe_reader(child.stdout.take());
+ let mut stderr_reader = spawn_pipe_reader(child.stderr.take());
+ // Write stdin on a separate thread so a non‑reading child cannot block us.
+ let stdin_handle = child.stdin.take().map(|mut stdin| {
+ let buf = input.to_vec();
+ thread::spawn(move || {
+ let res = stdin.write_all(&buf);
+ drop(stdin); // close to signal EOF
+ res
+ })
+ });
@@
- let status = match wait_for_exit(&mut child, COMMAND_TIMEOUT) {
+ let status = match wait_for_exit(&mut child, COMMAND_TIMEOUT) {
Ok(status) => status,
Err(err) => {
- let _ = join_reader(stdout_reader.take());
- let _ = join_reader(stderr_reader.take());
+ let _ = join_reader(stdout_reader.take());
+ let _ = join_reader(stderr_reader.take());
+ if let Some(handle) = stdin_handle {
+ let _ = handle.join();
+ }
return Err(err);
}
};
- let stdout = join_reader(stdout_reader.take()).map_err(CommandFailure::Io)?;
- let stderr = join_reader(stderr_reader.take()).map_err(CommandFailure::Io)?;
-
- if let Some(err) = broken_pipe {
- return Err(CommandFailure::BrokenPipe {
- source: err,
- status: status.code(),
- stderr,
- });
- }
+ let stdout = join_reader(stdout_reader.take()).map_err(CommandFailure::Io)?;
+ let stderr = join_reader(stderr_reader.take()).map_err(CommandFailure::Io)?;
+ if let Some(handle) = stdin_handle {
+ match handle.join() {
+ Ok(Ok(())) => {}
+ Ok(Err(err)) => {
+ if err.kind() == io::ErrorKind::BrokenPipe {
+ return Err(CommandFailure::BrokenPipe {
+ source: err,
+ status: status.code(),
+ stderr,
+ });
+ }
+ return Err(CommandFailure::Io(err));
+ }
+ Err(_) => return Err(CommandFailure::Io(io::Error::other("stdin writer panicked"))),
+ }
+ }
@@
- Err(CommandFailure::Exit {
- status: status.code(),
- stderr,
- })
+ Err(CommandFailure::Exit { status: status.code(), stderr })
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| fn run_child(mut command: Command, input: &[u8]) -> Result<Vec<u8>, CommandFailure> { | |
| let mut child = command.spawn().map_err(CommandFailure::Spawn)?; | |
| let mut broken_pipe = None; | |
| if let Some(mut stdin) = child.stdin.take() { | |
| match stdin.write_all(input) { | |
| Ok(()) => {} | |
| Err(err) => { | |
| if err.kind() == io::ErrorKind::BrokenPipe { | |
| broken_pipe = Some(err); | |
| } else { | |
| return Err(CommandFailure::Io(err)); | |
| } | |
| } | |
| } | |
| } | |
| let mut stdout_reader = spawn_pipe_reader(child.stdout.take()); | |
| let mut stderr_reader = spawn_pipe_reader(child.stderr.take()); | |
| let status = match wait_for_exit(&mut child, COMMAND_TIMEOUT) { | |
| Ok(status) => status, | |
| Err(err) => { | |
| let _ = join_reader(stdout_reader.take()); | |
| let _ = join_reader(stderr_reader.take()); | |
| return Err(err); | |
| } | |
| }; | |
| let stdout = join_reader(stdout_reader.take()).map_err(CommandFailure::Io)?; | |
| let stderr = join_reader(stderr_reader.take()).map_err(CommandFailure::Io)?; | |
| if let Some(err) = broken_pipe { | |
| return Err(CommandFailure::BrokenPipe { | |
| source: err, | |
| status: status.code(), | |
| stderr, | |
| }); | |
| } | |
| if status.success() { | |
| Ok(stdout) | |
| } else { | |
| Err(CommandFailure::Exit { | |
| status: status.code(), | |
| stderr, | |
| }) | |
| } | |
| } | |
| fn run_child(mut command: Command, input: &[u8]) -> Result<Vec<u8>, CommandFailure> { | |
| let mut child = command.spawn().map_err(CommandFailure::Spawn)?; | |
| - let mut broken_pipe = None; | |
| - if let Some(mut stdin) = child.stdin.take() { | |
| - match stdin.write_all(input) { | |
| - Ok(()) => {} | |
| - Err(err) => { | |
| - if err.kind() == io::ErrorKind::BrokenPipe { | |
| - broken_pipe = Some(err); | |
| - } else { | |
| - return Err(CommandFailure::Io(err)); | |
| - } | |
| - } | |
| - } | |
| - } | |
| - | |
| - let mut stdout_reader = spawn_pipe_reader(child.stdout.take()); | |
| // Start readers first to avoid pipe back-pressure deadlocks. | |
| let mut stdout_reader = spawn_pipe_reader(child.stdout.take()); | |
| let mut stderr_reader = spawn_pipe_reader(child.stderr.take()); | |
| // Write stdin on a separate thread so a non-reading child cannot block us. | |
| let stdin_handle = child.stdin.take().map(|mut stdin| { | |
| let buf = input.to_vec(); | |
| thread::spawn(move || { | |
| let res = stdin.write_all(&buf); | |
| drop(stdin); // close to signal EOF | |
| res | |
| }) | |
| }); | |
| let status = match wait_for_exit(&mut child, COMMAND_TIMEOUT) { | |
| Ok(status) => status, | |
| Err(err) => { | |
| - let _ = join_reader(stdout_reader.take()); | |
| let _ = join_reader(stdout_reader.take()); | |
| let _ = join_reader(stderr_reader.take()); | |
| if let Some(handle) = stdin_handle { | |
| let _ = handle.join(); | |
| } | |
| return Err(err); | |
| } | |
| }; | |
| - let stdout = join_reader(stdout_reader.take()).map_err(CommandFailure::Io)?; | |
| - let stderr = join_reader(stderr_reader.take()).map_err(CommandFailure::Io)?; | |
| - | |
| - if let Some(err) = broken_pipe { | |
| - return Err(CommandFailure::BrokenPipe { | |
| - source: err, | |
| - status: status.code(), | |
| - stderr, | |
| - }); | |
| let stdout = join_reader(stdout_reader.take()).map_err(CommandFailure::Io)?; | |
| let stderr = join_reader(stderr_reader.take()).map_err(CommandFailure::Io)?; | |
| if let Some(handle) = stdin_handle { | |
| match handle.join() { | |
| Ok(Ok(())) => {} | |
| Ok(Err(err)) => { | |
| if err.kind() == io::ErrorKind::BrokenPipe { | |
| return Err(CommandFailure::BrokenPipe { | |
| source: err, | |
| status: status.code(), | |
| stderr, | |
| }); | |
| } | |
| return Err(CommandFailure::Io(err)); | |
| } | |
| Err(_) => return Err(CommandFailure::Io(io::Error::other("stdin writer panicked"))), | |
| } | |
| } | |
| if status.success() { | |
| Ok(stdout) | |
| } else { | |
| - Err(CommandFailure::Exit { | |
| - status: status.code(), | |
| - stderr, | |
| Err(CommandFailure::Exit { status: status.code(), stderr }) | |
| } | |
| } |
🤖 Prompt for AI Agents
In src/stdlib/command.rs around lines 203 to 250, the current run_child
implementation writes to the child's stdin synchronously before spawning
stdout/stderr readers and before applying the timeout, which can deadlock if the
child fills its stdout/stderr; to fix it, first spawn the stdout and stderr
reader threads (spawn_pipe_reader) and then perform the stdin.write_all on a
dedicated writer thread (or spawn_blocking) that captures any BrokenPipe or IO
error into a shared channel/Result; wait for child exit with wait_for_exit as
before, and on exit/timeout always join the writer thread and both reader
threads (join_reader) to ensure they terminate and return their output/errors;
propagate the writer error as BrokenPipe/Io as appropriate, and ensure reader
joins are mapped to CommandFailure::Io when collecting stdout/stderr.
| fn value_from_bytes(bytes: Vec<u8>) -> Value { | ||
| match String::from_utf8(bytes) { | ||
| Ok(text) => Value::from(text), | ||
| Err(err) => Value::from_bytes(err.into_bytes()), | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
DRY up byte-to-Value conversion across stdlib
Unify value_from_bytes with network::to_value to avoid duplication and inconsistent behaviour. Extract a shared helper (e.g., stdlib::util::to_value(Vec) -> Value) and reuse here and in network.rs.
🤖 Prompt for AI Agents
In src/stdlib/command.rs around lines 252 to 256, the local value_from_bytes
implementation duplicates logic present in network::to_value; extract a shared
function (e.g., pub(crate) fn to_value(bytes: Vec<u8>) -> Value) into
src/stdlib/util.rs, implement the same String::from_utf8 -> Ok(text) =>
Value::from(text) / Err(err) => Value::from_bytes(err.into_bytes()) behavior
there, export it, then replace the body of value_from_bytes with a call to
util::to_value(bytes) and update network.rs to call the new shared
util::to_value as well (adding the necessary use/imports).
| fn open_cache_dir(path: &str) -> Result<Dir, Error> { | ||
| let utf_path = Utf8Path::new(path); | ||
| if utf_path.as_str().is_empty() { | ||
| return Err(Error::new( | ||
| ErrorKind::InvalidOperation, | ||
| "cache_dir must not be empty", | ||
| )); | ||
| } | ||
|
|
||
| if utf_path.is_absolute() { | ||
| let mut root = String::new(); | ||
| for component in utf_path.components() { | ||
| match component { | ||
| Utf8Component::Prefix(prefix) => root.push_str(prefix.as_str()), | ||
| Utf8Component::RootDir => { | ||
| root.push('/'); | ||
| break; | ||
| } | ||
| _ => break, | ||
| } | ||
| } | ||
| if root.is_empty() { | ||
| root.push('/'); | ||
| } | ||
| let root_path = Utf8PathBuf::from(root); | ||
|
|
||
| let root_dir = Dir::open_ambient_dir(&root_path, ambient_authority()) | ||
| .map_err(|err| io_error("open root cache dir", &root_path, &err))?; | ||
| let rel = utf_path.strip_prefix(&root_path).map_err(|_| { | ||
| Error::new( | ||
| ErrorKind::InvalidOperation, | ||
| format!("failed to compute relative path for '{utf_path}'"), | ||
| ) | ||
| })?; | ||
| if rel.as_str().is_empty() { | ||
| return Ok(root_dir); | ||
| } | ||
|
|
||
| root_dir | ||
| .create_dir_all(rel) | ||
| .map_err(|err| io_error("create cache dir", utf_path, &err))?; | ||
| return root_dir | ||
| .open_dir(rel) | ||
| .map_err(|err| io_error("open cache dir", utf_path, &err)); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Reject filesystem root as a cache directory
Prevent accidental writes to the filesystem root (e.g., "/" or "C:/") when cache_dir is absolute and equals the root. Fail fast with InvalidOperation to avoid surprising behaviour and permissions issues.
Agent prompt:
Add a guard after computing root_path/rel in open_cache_dir: if rel.as_str().is_empty() then return Err(Error::new(ErrorKind::InvalidOperation, "cache_dir must not be the filesystem root")).
🤖 Prompt for AI Agents
In src/stdlib/network.rs around lines 79 to 123, after computing root_path and
rel for an absolute cache_dir, add a guard that rejects the filesystem root: if
rel.as_str().is_empty() then return Err(Error::new(ErrorKind::InvalidOperation,
"cache_dir must not be the filesystem root")); this prevents treating "/" (or
equivalent on Windows) as a writable cache directory and fails fast with
InvalidOperation instead of proceeding to create/open the root directory.
| pub(crate) fn start_http_server(&mut self, body: String) { | ||
| if let Some(host) = self.extract_host_from_stdlib_url() { | ||
| let _ = TcpStream::connect(host); | ||
| } | ||
| self.shutdown_http_server(); | ||
| let (url, handle) = spawn_http_server(body); | ||
| self.stdlib_url = Some(url); | ||
| self.http_server = Some(handle); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Centralise server unblock-and-join logic in shutdown_http_server
Avoid duplicating the “connect to host to unblock accept, then join” sequence in both start and Drop. Perform it inside shutdown_http_server, call that in start and Drop.
Apply this diff:
@@
impl CliWorld {
pub(crate) fn start_http_server(&mut self, body: String) {
- if let Some(host) = self.extract_host_from_stdlib_url() {
- let _ = TcpStream::connect(host);
- }
- self.shutdown_http_server();
+ self.shutdown_http_server();
let (url, handle) = spawn_http_server(body);
self.stdlib_url = Some(url);
self.http_server = Some(handle);
}
pub(crate) fn shutdown_http_server(&mut self) {
- let Some(handle) = self.http_server.take() else {
- return;
- };
- let _ = handle.join();
- self.stdlib_url = None;
+ if let Some(host) = self.extract_host_from_stdlib_url() {
+ let _ = TcpStream::connect(host);
+ }
+ if let Some(handle) = self.http_server.take() {
+ let _ = handle.join();
+ }
+ self.stdlib_url = None;
}
@@
impl Drop for CliWorld {
fn drop(&mut self) {
- if let Some(host) = self.extract_host_from_stdlib_url() {
- let _ = TcpStream::connect(host);
- }
- self.shutdown_http_server();
+ self.shutdown_http_server();
self.restore_environment();
}
}Also applies to: 66-72, 105-112
🤖 Prompt for AI Agents
In tests/cucumber.rs around lines 56-64 (also apply same change to 66-72 and
105-112), the logic that connects to the server host to unblock accept and then
joins the server is duplicated in start_http_server and Drop; move the
unblock-and-join sequence into shutdown_http_server so it performs: if
stdlib_url/host exists, connect to host to unblock accept, then join the
http_server handle and clear stdlib_url/http_server; update start_http_server to
call shutdown_http_server before spawning the new server (removing the duplicate
connect/join there), and update Drop to call shutdown_http_server instead of
repeating the logic; ensure shutdown_http_server is responsible for safely
handling None values and joining the handle only once.
| #[rstest] | ||
| #[case::uppercase( | ||
| compile_uppercase_helper, | ||
| "cmd_upper", | ||
| "shell_upper", | ||
| "{{ 'hello' | shell(cmd) | trim }}", | ||
| ShellExpectation::Success("HELLO") | ||
| )] | ||
| #[case::failure( | ||
| compile_failure_helper, | ||
| "cmd_fail", | ||
| "shell_fail", | ||
| "{{ 'data' | shell(cmd) }}", | ||
| ShellExpectation::Failure { | ||
| substrings: &["command", "exited"], | ||
| }, | ||
| )] | ||
| fn shell_filter_behaviour( |
There was a problem hiding this comment.
Broaden failure assertions to tolerate different failure modes
Do not force “exited” specifically; IO failures (e.g., Broken pipe) and signal terminations are also valid. Accept any canonical failure phrasing emitted by command_error.
Apply this diff:
- ShellExpectation::Failure {
- substrings: &["command", "exited"],
- },
+ ShellExpectation::Failure {
+ substrings: &["command", "failed"]
+ },📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| #[rstest] | |
| #[case::uppercase( | |
| compile_uppercase_helper, | |
| "cmd_upper", | |
| "shell_upper", | |
| "{{ 'hello' | shell(cmd) | trim }}", | |
| ShellExpectation::Success("HELLO") | |
| )] | |
| #[case::failure( | |
| compile_failure_helper, | |
| "cmd_fail", | |
| "shell_fail", | |
| "{{ 'data' | shell(cmd) }}", | |
| ShellExpectation::Failure { | |
| substrings: &["command", "exited"], | |
| }, | |
| )] | |
| fn shell_filter_behaviour( | |
| #[rstest] | |
| #[case::uppercase( | |
| compile_uppercase_helper, | |
| "cmd_upper", | |
| "shell_upper", | |
| "{{ 'hello' | shell(cmd) | trim }}", | |
| ShellExpectation::Success("HELLO") | |
| )] | |
| #[case::failure( | |
| compile_failure_helper, | |
| "cmd_fail", | |
| "shell_fail", | |
| "{{ 'data' | shell(cmd) }}", | |
| ShellExpectation::Failure { | |
| substrings: &["command", "failed"] | |
| }, | |
| )] | |
| fn shell_filter_behaviour( | |
| compile_helper: fn(&str) -> Utf8PathBuf, | |
| cmd_name: &str, | |
| shell_name: &str, | |
| template_str: &str, | |
| expectation: ShellExpectation, | |
| ) { | |
| // … | |
| } |
🤖 Prompt for AI Agents
tests/std_filter_tests/command_filters.rs around lines 64 to 81: the failure
case currently asserts the error contains the specific word "exited", which is
too strict; change the test to accept any canonical failure phrasing from
command_error by removing the strict "exited" requirement—either assert only
that "command" appears in the message or replace the substrings array with a
broader set of possible failure tokens (e.g., "exited", "failed", "broken pipe",
"killed", "signal") so the test passes for IO failures, signal terminations, or
exit-status failures.
| @@ -0,0 +1,107 @@ | |||
| use std::{ | |||
There was a problem hiding this comment.
Add a module-level doc comment.
Document the module purpose per guidelines.
Apply:
+//! Tests for stdlib network helpers: fetch success, cache behaviour, and error/impurity.
use std::{📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| use std::{ | |
| //! Tests for stdlib network helpers: fetch success, cache behaviour, and error/impurity. | |
| use std::{ |
🤖 Prompt for AI Agents
In tests/std_filter_tests/network_functions.rs at line 1, add a module-level doc
comment at the top of the file that succinctly states the module purpose per
project guidelines; insert a triple-slash (///) or module-level block comment
(/*! ... */) before the first use statement with a one-line summary of what the
tests in this module cover, optionally followed by a brief sentence about key
behaviors tested or relevant configuration, and ensure the comment follows the
same style/formatting used across the codebase.
| let temp = tempdir().expect("tempdir"); | ||
| let cache_dir = temp.path().join("cache"); | ||
| let cache_str = cache_dir.to_str().expect("utf8 cache dir").to_owned(); | ||
| let (url, handle) = start_server("cached"); |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Use camino for UTF‑8 paths; avoid to_str on std::path.
Avoid lossy conversions; build the cache dir with Utf8PathBuf and take .as_str().
Apply:
- let temp = tempdir().expect("tempdir");
- let cache_dir = temp.path().join("cache");
- let cache_str = cache_dir.to_str().expect("utf8 cache dir").to_owned();
+ let temp = tempdir().expect("tempdir");
+ let cache_root = camino::Utf8PathBuf::from_path_buf(temp.path().to_path_buf())
+ .expect("utf8 temp path");
+ let cache_dir = cache_root.join("cache");
+ let cache_str = cache_dir.as_str().to_owned();
let (url, handle) = start_server("cached");Also add the import outside this hunk:
use camino::Utf8PathBuf; // or qualify as shown aboveAs per coding guidelines
🤖 Prompt for AI Agents
In tests/std_filter_tests/network_functions.rs around lines 55–58, the test is
using std::path::Path::to_str which can be lossy; replace that logic by
constructing the cache directory as a camino::Utf8PathBuf (join "cache" on an
Utf8PathBuf) and use its .as_str() for the UTF‑8 string, avoiding
to_str/to_owned on std::path; also add the import use camino::Utf8PathBuf near
the top of the file.
| env.add_template( | ||
| "fetch_cache", | ||
| "{{ fetch(url, cache=True, cache_dir=cache_dir) }}", | ||
| ) |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
🧩 Analysis chain
Verify MiniJinja boolean literal casing.
Jinja/MiniJinja booleans are typically lower‑case (true/false). Ensure capitalised True parses, or switch to lower‑case.
Apply if needed:
- "{{ fetch(url, cache=True, cache_dir=cache_dir) }}",
+ "{{ fetch(url, cache=true, cache_dir=cache_dir) }}",Run to confirm parsing:
🏁 Script executed:
#!/bin/bash
# Show all templates using cache=True to fix casing if required
rg -nP --type=rust -C2 'cache\s*=\s*True'Length of output: 371
Does MiniJinja accept capitalised boolean literals (True/False), or are they required to be lower-case (true/false) per Jinja spec?
Lower-case boolean literal in MiniJinja template Change cache=True to cache=true in tests/std_filter_tests/network_functions.rs:63.
🤖 Prompt for AI Agents
In tests/std_filter_tests/network_functions.rs around lines 61 to 64, the
MiniJinja template uses an uppercase Python-style boolean literal `cache=True`;
change it to the MiniJinja lowercase boolean `cache=true` in the
env.add_template call so the template parses and evaluates correctly.
| // The reviewer requested nested optional checks to keep the host parsing explicit | ||
| // for readability, so silence the lint that prefers collapsing them. | ||
| #[allow( | ||
| clippy::collapsible_if, | ||
| reason = "Review requested explicit nested host extraction checks" | ||
| )] | ||
| #[given(regex = r#"^an HTTP server returning "(.+)"$"#)] | ||
| fn http_server_returning(world: &mut CliWorld, body: String) { | ||
| if let Some(url) = world.stdlib_url.as_ref() { | ||
| if let Some(host) = extract_host_from_url(url) { | ||
| let _ = TcpStream::connect(host); | ||
| } | ||
| } | ||
| world.start_http_server(body); | ||
| } |
There was a problem hiding this comment.
Remove forbidden #[allow] and eliminate the lint via code.
Do not use #[allow]; refactor to avoid clippy::collapsible_if.
Apply:
-// The reviewer requested nested optional checks to keep the host parsing explicit
-// for readability, so silence the lint that prefers collapsing them.
-#[allow(
- clippy::collapsible_if,
- reason = "Review requested explicit nested host extraction checks"
-)]
#[given(regex = r#"^an HTTP server returning "(.+)"$"#)]
fn http_server_returning(world: &mut CliWorld, body: String) {
- if let Some(url) = world.stdlib_url.as_ref() {
- if let Some(host) = extract_host_from_url(url) {
- let _ = TcpStream::connect(host);
- }
- }
+ if let Some(host) = world
+ .stdlib_url
+ .as_deref()
+ .and_then(extract_host_from_url)
+ {
+ let _ = TcpStream::connect(host);
+ }
world.start_http_server(body);
}If you must justify a lint expectation elsewhere, prefer:
#[expect(clippy::..., reason = "why it is safe/necessary")]As per coding guidelines
🤖 Prompt for AI Agents
In tests/steps/stdlib_steps.rs around lines 202 to 216, remove the forbidden
#[allow(...)] and replace the nested if-let blocks with a single let-chain
conditional so the nested host extraction remains explicit but
clippy::collapsible_if is no longer triggered; specifically, remove the
attribute and change the two nested if-let statements into one if-let chain that
checks world.stdlib_url.as_ref() and then extract_host_from_url(url) in the same
conditional, keeping the TcpStream::connect(host) call and the subsequent
world.start_http_server(body) call unchanged.
There was a problem hiding this comment.
Actionable comments posted: 35
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (31)
.github/actions/stage/action.yml(1 hunks).github/actions/stage/scripts/stage.py(1 hunks).github/actions/stage/scripts/stage_common/__init__.py(1 hunks).github/actions/stage/scripts/stage_common/checksum_utils.py(1 hunks).github/actions/stage/scripts/stage_common/config.py(1 hunks).github/actions/stage/scripts/stage_common/environment.py(1 hunks).github/actions/stage/scripts/stage_common/errors.py(1 hunks).github/actions/stage/scripts/stage_common/fs_utils.py(1 hunks).github/actions/stage/scripts/stage_common/github_output.py(1 hunks).github/actions/stage/scripts/stage_common/glob_utils.py(1 hunks).github/actions/stage/scripts/stage_common/staging.py(1 hunks).github/actions/stage/scripts/stage_common/template_utils.py(1 hunks).github/release-staging.toml(1 hunks).github/workflows/release.yml(4 hunks).github/workflows/scripts/normalise_windows_paths.py(1 hunks).github/workflows/scripts/stage_common.py(0 hunks).github/workflows/scripts/stage_macos.py(0 hunks).github/workflows/scripts/stage_windows.py(0 hunks)codex/refactor-python-staging-scripts-into-github-action(1 hunks)cyclopts/__init__.pyi(1 hunks)docs/netsuke-design.md(3 hunks)scripts/_release_upload_deps.py(1 hunks)scripts/upload_release_assets.py(6 hunks)tests_python/conftest.py(1 hunks)tests_python/stage_test_helpers.py(1 hunks)tests_python/test_stage_action.py(1 hunks)tests_python/test_stage_cli.py(1 hunks)tests_python/test_stage_common.py(0 hunks)tests_python/test_stage_common_config.py(1 hunks)tests_python/test_stage_common_module.py(1 hunks)tests_python/test_stage_common_staging.py(1 hunks)
💤 Files with no reviewable changes (4)
- .github/workflows/scripts/stage_common.py
- tests_python/test_stage_common.py
- .github/workflows/scripts/stage_windows.py
- .github/workflows/scripts/stage_macos.py
🧰 Additional context used
📓 Path-based instructions (4)
**/*.py
⚙️ CodeRabbit configuration file
**/*.py: - Keep cyclomatic complexity ≤ 12
- Follow single responsibility and CQRS (command/query segregation)
- Docstrings must follow the
numpystyle guide. Use a single-line summary for private functions and methods, and full structured docs for all public interfaces.- Move conditionals with >2 branches to predicate/helper functions
- Avoid
eval,exec,pickle, monkey-patching,ctypes, unsafe shell- Every module must begin with a triple-quoted docstring explaining its purpose, utility, and usage, including example calls if appropriate.
- Comments and docs must follow en-GB-oxendict (-ize / -yse / -our) spelling and grammar
- Lint suppressions:
- Blanket
# noqa, file-level skips, and categories are forbidden- Only narrow in-line disables (
# noqa: XYZ) are permitted, and must be accompanied byFIXME:or a ticket link, and used only as a last resort.- Use
pytestfixtures for shared setup (conftest.pyorfixtures/)- Replace duplicate tests with
@pytest.mark.parametrize- Prefer
pytest-mockorunittest.mockfor stubs/mocks- Use
assert …, "message"over bare asserts- Reflect all API/behaviour changes in
docs/and update roadmap on completion- Files must not exceed 400 logical lines:
- Decompose large modules into subpackages
- Split large
match/caseor dispatch tables by domain and collocate with targets if appropriate- Move bulky data (fixtures, templates) to external files for parsing at runtime
- Mutable defaults and shadowed built-ins are forbidden
- All code must have clear type hints using modern style (
A | B,list[str],class Foo[A]:,type Bar = int, etc.), with ABC imports drawn from the correct stdlib module.- All path manipulation must be performed using pathlib for cross platform safety. Do not use string manipulation for extraction of path components or filename elements.
Files:
tests_python/conftest.pyscripts/_release_upload_deps.pytests_python/stage_test_helpers.pytests_python/test_stage_common_module.pytests_python/test_stage_common_config.pytests_python/test_stage_cli.pytests_python/test_stage_action.pytests_python/test_stage_common_staging.pyscripts/upload_release_assets.py
docs/**/*.md
📄 CodeRabbit inference engine (AGENTS.md)
docs/**/*.md: Use docs/ markdown as the reference for requirements, dependencies, and architectural decisions
Proactively update docs/ when decisions, requirements, or dependencies change
Documentation must use en-GB-oxendict spelling and grammar (LICENSE name unchanged)
Files:
docs/netsuke-design.md
**/*.md
📄 CodeRabbit inference engine (AGENTS.md)
**/*.md: Validate Markdown with make markdownlint
Run make fmt after doc changes to format Markdown and fix tables
Validate Mermaid diagrams in Markdown using make nixie
Wrap paragraphs and bullet points at 80 columns
Wrap code blocks at 120 columns
Do not wrap tables and headings
Use dashes (-) for list bullets
Use GitHub-flavoured Markdown footnotes ([^1]) for references and footnotes
Files:
docs/netsuke-design.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/netsuke-design.md
docs/**/*.{rs,md}
📄 CodeRabbit inference engine (docs/rust-doctest-dry-guide.md)
docs/**/*.{rs,md}: In fenced documentation code blocks, specify the language as rust (```rust)
Use assert!, assert_eq!, and assert_ne! in doctests to verify outcomes rather than relying on panic-free execution alone
Avoid using .unwrap() or .expect() in examples; handle errors idiomatically
For fallible examples, write an explicit fn main() -> Result<...> and hide boilerplate with # lines
Alternatively, use the (()) shorthand at the end of a code block (no whitespace) to create an implicit Result-returning main
Use hidden lines (#) to hide setup, main wrappers, and non-essential use statements in doctests
Prefer no_run over ignore for examples with side effects (e.g., network, filesystem)
Use should_panic for examples intended to panic; the test should fail if it does not panic
Use compile_fail for examples that should not compile, but recognize its brittleness across compiler versions
Use edition20xx (e.g., edition2018, edition2021) on code fences when demonstrating edition-specific syntax
Avoid relying solely on #[cfg(feature = "...")] inside doctest code blocks, which can yield misleading empty-ok tests when the feature is disabled
Files:
docs/netsuke-design.md
🧬 Code graph analysis (18)
tests_python/conftest.py (1)
tests_python/test_stage_cli.py (1)
workspace(36-41)
.github/workflows/scripts/normalise_windows_paths.py (1)
.github/actions/stage/scripts/stage.py (1)
main(34-57)
.github/actions/stage/scripts/stage_common/environment.py (1)
.github/actions/stage/scripts/stage_common/errors.py (1)
StageError(4-5)
.github/actions/stage/scripts/stage_common/__init__.py (4)
.github/actions/stage/scripts/stage_common/config.py (3)
ArtefactConfig(38-81)StagingConfig(85-193)load_config(196-251).github/actions/stage/scripts/stage_common/environment.py (1)
require_env_path(13-30).github/actions/stage/scripts/stage_common/errors.py (1)
StageError(4-5).github/actions/stage/scripts/stage_common/staging.py (2)
StageResult(58-64)stage_artefacts(119-214)
.github/actions/stage/scripts/stage_common/fs_utils.py (2)
.github/actions/stage/scripts/stage_common/errors.py (1)
StageError(4-5).github/actions/stage/scripts/stage_common/config.py (1)
staging_dir(167-169)
scripts/_release_upload_deps.py (2)
cyclopts/__init__.pyi (3)
App(11-14)Parameter(8-9)default(13-13)scripts/upload_release_assets.py (1)
main(224-276)
cyclopts/__init__.pyi (1)
tests_python/test_stage_cli.py (1)
default(25-27)
tests_python/test_stage_common_module.py (3)
tests_python/conftest.py (2)
stage_common(16-24)workspace(28-34).github/actions/stage/scripts/stage_common/errors.py (1)
StageError(4-5).github/actions/stage/scripts/stage_common/environment.py (1)
require_env_path(13-30)
tests_python/test_stage_common_config.py (3)
tests_python/conftest.py (2)
stage_common(16-24)workspace(28-34).github/actions/stage/scripts/stage_common/config.py (4)
StagingConfig(85-193)as_template_context(176-193)load_config(196-251)staging_dir(167-169).github/actions/stage/scripts/stage_common/errors.py (1)
StageError(4-5)
.github/actions/stage/scripts/stage.py (6)
tests_python/conftest.py (2)
stage_common(16-24)workspace(28-34).github/actions/stage/scripts/stage_common/errors.py (1)
StageError(4-5).github/actions/stage/scripts/stage_common/config.py (2)
load_config(196-251)staging_dir(167-169).github/actions/stage/scripts/stage_common/environment.py (1)
require_env_path(13-30).github/actions/stage/scripts/stage_common/staging.py (1)
stage_artefacts(119-214)cyclopts/__init__.pyi (2)
App(11-14)default(13-13)
tests_python/test_stage_cli.py (2)
tests_python/stage_test_helpers.py (2)
decode_output_file(10-37)write_workspace_inputs(40-52).github/actions/stage/scripts/stage.py (1)
main(34-57)
.github/actions/stage/scripts/stage_common/glob_utils.py (1)
tests_python/conftest.py (1)
workspace(28-34)
.github/actions/stage/scripts/stage_common/staging.py (7)
.github/actions/stage/scripts/stage_common/checksum_utils.py (1)
write_checksum(11-21).github/actions/stage/scripts/stage_common/config.py (4)
ArtefactConfig(38-81)StagingConfig(85-193)staging_dir(167-169)as_template_context(176-193).github/actions/stage/scripts/stage_common/errors.py (1)
StageError(4-5).github/actions/stage/scripts/stage_common/fs_utils.py (1)
safe_destination_path(12-21).github/actions/stage/scripts/stage_common/github_output.py (1)
write_github_output(12-26).github/actions/stage/scripts/stage_common/glob_utils.py (2)
glob_root_and_pattern(38-51)match_candidate_path(10-35).github/actions/stage/scripts/stage_common/template_utils.py (2)
render_template(30-37)resolve_artefact_source(40-52)
tests_python/test_stage_action.py (1)
tests_python/conftest.py (1)
stage_common(16-24)
.github/actions/stage/scripts/stage_common/config.py (3)
.github/actions/stage/scripts/stage_common/environment.py (1)
require_env_path(13-30).github/actions/stage/scripts/stage_common/errors.py (1)
StageError(4-5)tests_python/conftest.py (1)
workspace(28-34)
.github/actions/stage/scripts/stage_common/template_utils.py (3)
.github/actions/stage/scripts/stage_common/errors.py (1)
StageError(4-5).github/actions/stage/scripts/stage_common/glob_utils.py (1)
match_candidate_path(10-35).github/actions/stage/scripts/stage_common/config.py (1)
ArtefactConfig(38-81)
tests_python/test_stage_common_staging.py (5)
tests_python/stage_test_helpers.py (2)
decode_output_file(10-37)write_workspace_inputs(40-52)tests_python/conftest.py (2)
stage_common(16-24)workspace(28-34).github/actions/stage/scripts/stage_common/config.py (3)
StagingConfig(85-193)ArtefactConfig(38-81)staging_dir(167-169).github/actions/stage/scripts/stage_common/staging.py (1)
stage_artefacts(119-214).github/actions/stage/scripts/stage_common/errors.py (1)
StageError(4-5)
scripts/upload_release_assets.py (1)
scripts/_release_upload_deps.py (3)
load_cyclopts(37-59)load_plumbum(62-93)run_cli(96-142)
🪛 Ruff (0.13.3)
.github/actions/stage/scripts/stage_common/__init__.py
8-17: __all__ is not sorted
Apply an isort-style sorting to __all__
(RUF022)
scripts/_release_upload_deps.py
9-9: Import from collections.abc instead: Sequence
Import from collections.abc
(UP035)
43-43: Local variable exc is assigned to but never used
Remove assignment to unused variable exc
(F841)
48-48: Undefined name exc
(F821)
56-56: Boolean positional value in function call
(FBT003)
59-59: Boolean positional value in function call
(FBT003)
72-72: Local variable exc is assigned to but never used
Remove assignment to unused variable exc
(F841)
79-79: Undefined name exc
(F821)
.github/actions/stage/scripts/stage_common/staging.py
45-51: __all__ is not sorted
Apply an isort-style sorting to __all__
(RUF022)
.github/actions/stage/scripts/stage_common/template_utils.py
41-41: Remove quotes from type annotation
Remove quotes
(UP037)
tests_python/test_stage_common_staging.py
203-203: Unused function argument: stage_common
(ARG001)
361-361: Unused function argument: stage_common
(ARG001)
372-372: Unused noqa directive (unused: ARG001)
Remove unused noqa directive
(RUF100)
373-373: Unused function argument: stage_common
(ARG001)
386-386: Unused noqa directive (unused: ARG001)
Remove unused noqa directive
(RUF100)
387-387: Unused function argument: stage_common
(ARG001)
395-395: Probable insecure usage of temporary file or directory: "/tmp/dist/*.zip"
(S108)
400-400: Unused noqa directive (unused: ARG001)
Remove unused noqa directive
(RUF100)
401-401: Unused function argument: stage_common
(ARG001)
🔍 Remote MCP
Here is a concise summary of additional context relevant for reviewing PR #187:
- ureq crate (v2.10.5) provides a synchronous HTTP client supporting GET with timeout configuration via
ureq::Agent::new().timeout_read()…([Ref::Ref_search_documentation—failed to fetch docs]). - wait-timeout crate (v0.2) supplies a
WaitTimeouttrait onstd::process::Child, enablingchild.wait_timeout(Duration)to implement timeouts for external commands ([Ref::Ref_search_documentation—failed to fetch docs]). - In
tests/cucumber.rs, the suggested refactoring matches existing helper signatures:fn extract_host_from_stdlib_url(&self) -> Option<&str>is consistent withfn extract_host_from_url(url: &str) -> Option<&str>patterns found intests/steps/stdlib_steps.rs([Deepwiki::read_wiki_contents]). CommandFixtureintests/std_filter_tests/command_filters.rsalready encapsulates process spawning and environment, so extending it with a Windows-onlywith_path_prefix()aligns with its existing builder-like API ([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). (6)
- GitHub Check: release / build-macos (aarch64-apple-darwin, macos-14, macos-arm64, arm64, macos-aarch64)
- GitHub Check: release / build-macos (x86_64-apple-darwin, macos-13, macos-x86_64, x86_64, macos-x86_64)
- GitHub Check: release / build-windows (x86_64-pc-windows-msvc, amd64, x64, windows-x86_64)
- GitHub Check: release / build-linux (x86_64-unknown-linux-gnu, linux-amd64, amd64)
- GitHub Check: release / build-windows (aarch64-pc-windows-msvc, arm64, arm64, windows-aarch64)
- GitHub Check: build-test (ubuntu-latest, stable)
🔇 Additional comments (40)
docs/netsuke-design.md (1)
925-939: Record the clarified impurity contract.Document how
fetch,shell, andgrepflipStdlibStateand note the SHA-256 cache key so readers understand the caching boundaries.cyclopts/__init__.pyi (1)
13-13: Align stub signature with implementation
Verify the runtime signature ofApp.defaultin cyclopts/init.py and update the stub at line 13 to match—useCallable[P, T]if preserved orCallable[..., object]otherwise..github/actions/stage/scripts/stage_common/errors.py (1)
1-5: LGTM!The error class is appropriately defined with a clear docstring.
.github/actions/stage/scripts/stage_common/environment.py (1)
13-30: LGTM!The function correctly validates environment variables and follows numpy-style docstring conventions with Parameters and Raises sections.
.github/release-staging.toml (7)
1-10: LGTM!The common configuration section correctly defines the staging structure, artefact sources, and fallback paths for man page discovery.
12-16: LGTM!Windows x86_64 target configuration is correct.
18-22: LGTM!Windows aarch64 target configuration is correct.
24-27: LGTM!macOS x86_64 target configuration is correct.
29-32: LGTM!macOS aarch64 target configuration is correct.
34-37: LGTM!Linux x86_64 target configuration is correct.
39-42: LGTM!Linux aarch64 target configuration is correct.
.github/actions/stage/scripts/stage.py (3)
8-19: LGTM!The module docstring appropriately explains the CLI entry point's purpose and provides a clear usage example.
21-30: LGTM!Imports and application setup are correct. The help text uses proper en-GB spelling.
60-61: LGTM!The
__main__guard correctly invokes the CLI application..github/actions/stage/scripts/stage_common/fs_utils.py (1)
1-9: LGTM!Module setup is correct. The docstring appropriately describes the module's purpose.
tests_python/test_stage_common_config.py (6)
1-9: LGTM!Module setup is correct.
12-34: LGTM!The test correctly verifies template context exposure.
36-70: LGTM!The test correctly verifies configuration merging behaviour.
72-87: LGTM!The test validates the repository's actual configuration file, ensuring it parses correctly.
89-115: LGTM!The test correctly verifies environment variable requirement.
117-223: LGTM!The parametrised test comprehensively covers configuration validation errors with appropriate assertions for error messages and context markers.
.github/workflows/release.yml (5)
175-179: LGTM!The
target_keyadditions correctly identify Windows targets for the new staging system.
199-202: LGTM!The workflow correctly invokes the new staging action, replacing inline Bash logic.
207-210: LGTM!The path normalisation step correctly consumes the
artefact_mapoutput from the staging action. Spelling is correct (en-GB).
249-254: LGTM!The
target_keyadditions correctly identify macOS targets for the new staging system.
275-278: LGTM!The workflow correctly invokes the new staging action for macOS targets.
.github/actions/stage/action.yml (3)
1-10: LGTM!Action metadata and input definitions are correct. Spelling follows en-GB conventions.
12-36: LGTM!Output definitions are comprehensive. The descriptions correctly use en-GB spelling ("artefact", "licence"), whilst the output identifiers follow programmatic naming conventions.
38-59: LGTM!The run steps correctly install uv, verify its availability, and execute the staging script. The use of commit SHA for the external action enhances security.
.github/actions/stage/scripts/stage_common/__init__.py (1)
1-6: LGTM!Module docstring and imports correctly establish the public API surface.
tests_python/stage_test_helpers.py (1)
1-7: LGTM!Module setup is correct.
scripts/_release_upload_deps.py (1)
96-109: Verify CycloptsApp.__call__accepts a positionalargumentssequence.
return support.app(arguments)assumes theAppis called with argv as the first positional parameter. Confirm the 2.x API matches this usage.Check whether Cyclopts v2.x `App.__call__` accepts an optional `argv: Sequence[str] | None` positional argument.scripts/upload_release_assets.py (6)
48-56: Good abstraction: decouple backends behind support objects.Map Cyclopts/Plumbum into local aliases cleanly; keeps the module importable without optional deps.
57-61: Type-checking guard forBoundCommandis correct.Expose the runtime fallback via the support object while preserving static types.
97-102: Efficient and readable candidate discovery.Yield a sorted iterator with precise filtering; annotation with
Iterator[Path]is apt.
168-175: Summariser acceptsIterableand avoids materialising unnecessarily.Keep memory usage low and API flexible.
178-179: BroadenassetstoIterablewithout changing behaviour.Maintain compatibility while enabling streaming sources.
279-306: Route CLI throughrun_clifor optional-dep resilience.Delegate parsing to Cyclopts when available, else fall back to argparse with env overrides; this aligns with the new loader.
codex/refactor-python-staging-scripts-into-github-action (2)
1-2: LGTM: Strict error handling configured correctly.The shebang and bash options enforce robust error handling, appropriate for a quality gate script.
7-7: Verify redundancy of path specification
Both--extra-search-path .github/actions/stage/scripts/and.github/actions/stage/scripts/*.pypoint to the same directory; confirm ifty checkauto-discovers files via the search path and drop the explicit glob if redundant.
| __all__ = [ | ||
| "ArtefactConfig", | ||
| "RESERVED_OUTPUT_KEYS", | ||
| "StageError", | ||
| "StageResult", | ||
| "StagingConfig", | ||
| "load_config", | ||
| "require_env_path", | ||
| "stage_artefacts", | ||
| ] |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Sort __all__ alphabetically.
Static analysis indicates that __all__ is not sorted. Whilst functional, alphabetical ordering improves maintainability.
As per coding guidelines:
Apply this diff:
__all__ = [
"ArtefactConfig",
+ "load_config",
"RESERVED_OUTPUT_KEYS",
+ "require_env_path",
+ "stage_artefacts",
"StageError",
"StageResult",
"StagingConfig",
- "load_config",
- "require_env_path",
- "stage_artefacts",
]Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 Ruff (0.13.3)
8-17: __all__ is not sorted
Apply an isort-style sorting to __all__
(RUF022)
🤖 Prompt for AI Agents
In .github/actions/stage/scripts/stage_common/__init__.py around lines 8 to 17,
the __all__ export list is not alphabetically ordered; reorder the identifiers
in that list into ascending alphabetical order (by name) and update the list
in-place so entries like ArtefactConfig, RESERVED_OUTPUT_KEYS, StageError,
StageResult, StagingConfig, load_config, require_env_path, stage_artefacts are
sorted lexicographically.
| def write_checksum(path: Path, algorithm: str) -> str: | ||
| """Write the checksum sidecar for ``path`` using ``algorithm``.""" | ||
|
|
||
| hasher = hashlib.new(algorithm) | ||
| with path.open("rb") as handle: | ||
| for chunk in iter(lambda: handle.read(8192), b""): | ||
| hasher.update(chunk) | ||
| digest = hasher.hexdigest() | ||
| checksum_path = path.with_name(f"{path.name}.{algorithm}") | ||
| checksum_path.write_text(f"{digest} {path.name}\n", encoding="utf-8") | ||
| return digest |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Expand docstring with structured sections.
Public functions must follow numpy-style docstrings including Parameters and Returns sections.
Apply this diff:
def write_checksum(path: Path, algorithm: str) -> str:
- """Write the checksum sidecar for ``path`` using ``algorithm``."""
+ """Write the checksum sidecar for ``path`` using ``algorithm``.
+
+ Parameters
+ ----------
+ path:
+ Path to the file for which the checksum will be computed.
+ algorithm:
+ Hashing algorithm name supported by :mod:`hashlib` (e.g., ``"sha256"``).
+
+ Returns
+ -------
+ str
+ Hexadecimal digest of the computed checksum.
+ """As per coding guidelines.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def write_checksum(path: Path, algorithm: str) -> str: | |
| """Write the checksum sidecar for ``path`` using ``algorithm``.""" | |
| hasher = hashlib.new(algorithm) | |
| with path.open("rb") as handle: | |
| for chunk in iter(lambda: handle.read(8192), b""): | |
| hasher.update(chunk) | |
| digest = hasher.hexdigest() | |
| checksum_path = path.with_name(f"{path.name}.{algorithm}") | |
| checksum_path.write_text(f"{digest} {path.name}\n", encoding="utf-8") | |
| return digest | |
| def write_checksum(path: Path, algorithm: str) -> str: | |
| """Write the checksum sidecar for ``path`` using ``algorithm``. | |
| Parameters | |
| ---------- | |
| path: | |
| Path to the file for which the checksum will be computed. | |
| algorithm: | |
| Hashing algorithm name supported by :mod:`hashlib` (e.g., ``"sha256"``). | |
| Returns | |
| ------- | |
| str | |
| Hexadecimal digest of the computed checksum. | |
| """ | |
| hasher = hashlib.new(algorithm) | |
| with path.open("rb") as handle: | |
| for chunk in iter(lambda: handle.read(8192), b""): | |
| hasher.update(chunk) | |
| digest = hasher.hexdigest() | |
| checksum_path = path.with_name(f"{path.name}.{algorithm}") | |
| checksum_path.write_text(f"{digest} {path.name}\n", encoding="utf-8") | |
| return digest |
🤖 Prompt for AI Agents
In .github/actions/stage/scripts/stage_common/checksum_utils.py around lines 11
to 21, the write_checksum function's docstring is currently a single-line
description; update it to a numpy-style docstring that includes a short summary,
a Parameters section documenting 'path' (Path) and 'algorithm' (str) with
descriptions, and a Returns section describing the returned str digest; keep
existing one-line summary, add types and brief explanations for each parameter,
and document that the function writes a sidecar file named
"<filename>.<algorithm>" containing the checksum and filename before returning
the hex digest.
| for index, entry in enumerate(entries, start=1): | ||
| source = entry.get("source") | ||
| if not isinstance(source, str) or not source: | ||
| message = ( | ||
| "Missing required artefact key 'source' " | ||
| f"in entry #{index} of {config_path}" | ||
| ) | ||
| raise StageError(message) | ||
| alternatives = _normalise_alternatives( | ||
| entry.get("alternatives", []), index, config_path | ||
| ) | ||
| artefacts.append( | ||
| ArtefactConfig( | ||
| source=source, | ||
| required=entry.get("required", True), | ||
| output=entry.get("output"), | ||
| destination=entry.get("destination"), | ||
| alternatives=alternatives, | ||
| ) | ||
| ) |
There was a problem hiding this comment.
Validate artefact entries before accessing keys.
Fix the loop to reject non-mapping artefact entries with a StageError; the current version will explode with AttributeError when the TOML contains an incorrect array item, bypassing the intended configuration error reporting.
artefacts: list[ArtefactConfig] = []
for index, entry in enumerate(entries, start=1):
+ if not isinstance(entry, dict):
+ message = (
+ "Artefact entries must be tables of key/value pairs "
+ f"(entry #{index} in {config_path})"
+ )
+ raise StageError(message)
source = entry.get("source")📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| for index, entry in enumerate(entries, start=1): | |
| source = entry.get("source") | |
| if not isinstance(source, str) or not source: | |
| message = ( | |
| "Missing required artefact key 'source' " | |
| f"in entry #{index} of {config_path}" | |
| ) | |
| raise StageError(message) | |
| alternatives = _normalise_alternatives( | |
| entry.get("alternatives", []), index, config_path | |
| ) | |
| artefacts.append( | |
| ArtefactConfig( | |
| source=source, | |
| required=entry.get("required", True), | |
| output=entry.get("output"), | |
| destination=entry.get("destination"), | |
| alternatives=alternatives, | |
| ) | |
| ) | |
| artefacts: list[ArtefactConfig] = [] | |
| for index, entry in enumerate(entries, start=1): | |
| if not isinstance(entry, dict): | |
| message = ( | |
| "Artefact entries must be tables of key/value pairs " | |
| f"(entry #{index} in {config_path})" | |
| ) | |
| raise StageError(message) | |
| source = entry.get("source") | |
| if not isinstance(source, str) or not source: | |
| message = ( | |
| "Missing required artefact key 'source' " | |
| f"in entry #{index} of {config_path}" | |
| ) | |
| raise StageError(message) | |
| alternatives = _normalise_alternatives( | |
| entry.get("alternatives", []), index, config_path | |
| ) | |
| artefacts.append( | |
| ArtefactConfig( | |
| source=source, | |
| required=entry.get("required", True), | |
| output=entry.get("output"), | |
| destination=entry.get("destination"), | |
| alternatives=alternatives, | |
| ) | |
| ) |
🤖 Prompt for AI Agents
In .github/actions/stage/scripts/stage_common/config.py around lines 297 to 316,
the loop assumes each entry is a mapping and calls entry.get(...), which will
raise AttributeError for non-mapping TOML items; update the loop to validate
each entry is a mapping (e.g., isinstance(entry, dict)) before accessing keys
and raise a StageError with a clear message including the entry index and
config_path when it is not; then proceed to call entry.get(...) only after the
type check so malformed array items produce a controlled StageError instead of
an AttributeError.
| def safe_destination_path(staging_dir: Path, destination: str) -> Path: | ||
| """Return ``destination`` resolved beneath ``staging_dir``.""" | ||
|
|
||
| target = (staging_dir / destination).resolve() | ||
| staging_root = staging_dir.resolve() | ||
| if not target.is_relative_to(staging_root): | ||
| message = f"Destination escapes staging directory: {destination}" | ||
| raise StageError(message) | ||
| target.parent.mkdir(parents=True, exist_ok=True) | ||
| return target |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Expand the docstring to full numpy-style format.
This function is public (listed in __all__) and implements path traversal protection, but the docstring lacks Parameters, Returns, and Raises sections required by coding guidelines.
Apply this diff:
def safe_destination_path(staging_dir: Path, destination: str) -> Path:
- """Return ``destination`` resolved beneath ``staging_dir``."""
+ """Return ``destination`` resolved beneath ``staging_dir``.
+
+ Parameters
+ ----------
+ staging_dir : Path
+ Base directory under which the destination must reside.
+ destination : str
+ Relative path to resolve beneath ``staging_dir``.
+
+ Returns
+ -------
+ Path
+ Absolute resolved path with parent directories created.
+
+ Raises
+ ------
+ StageError
+ Raised when ``destination`` attempts to escape ``staging_dir`` via path
+ traversal (e.g. using ``..``).
+ """📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def safe_destination_path(staging_dir: Path, destination: str) -> Path: | |
| """Return ``destination`` resolved beneath ``staging_dir``.""" | |
| target = (staging_dir / destination).resolve() | |
| staging_root = staging_dir.resolve() | |
| if not target.is_relative_to(staging_root): | |
| message = f"Destination escapes staging directory: {destination}" | |
| raise StageError(message) | |
| target.parent.mkdir(parents=True, exist_ok=True) | |
| return target | |
| def safe_destination_path(staging_dir: Path, destination: str) -> Path: | |
| """Return ``destination`` resolved beneath ``staging_dir``. | |
| Parameters | |
| ---------- | |
| staging_dir : Path | |
| Base directory under which the destination must reside. | |
| destination : str | |
| Relative path to resolve beneath ``staging_dir``. | |
| Returns | |
| ------- | |
| Path | |
| Absolute resolved path with parent directories created. | |
| Raises | |
| ------ | |
| StageError | |
| Raised when ``destination`` attempts to escape ``staging_dir`` via path | |
| traversal (e.g. using ``..``). | |
| """ | |
| target = (staging_dir / destination).resolve() | |
| staging_root = staging_dir.resolve() | |
| if not target.is_relative_to(staging_root): | |
| message = f"Destination escapes staging directory: {destination}" | |
| raise StageError(message) | |
| target.parent.mkdir(parents=True, exist_ok=True) | |
| return target |
🤖 Prompt for AI Agents
In .github/actions/stage/scripts/stage_common/fs_utils.py around lines 12 to 21,
the public function safe_destination_path lacks a full numpy-style docstring;
update it to include a short description plus "Parameters" (staging_dir: Path —
root staging directory; destination: str — relative destination path), "Returns"
(Path — resolved destination path beneath staging_dir), "Raises" (StageError —
if the resolved target is not under staging_dir), and note side effects such as
creating parent directories; ensure types and brief descriptions follow numpy
docstring conventions.
| def write_github_output( | ||
| file: Path, values: Mapping[str, str | Sequence[str]] | ||
| ) -> None: | ||
| """Append ``values`` to ``file`` using GitHub's multiline syntax.""" | ||
|
|
||
| file.parent.mkdir(parents=True, exist_ok=True) | ||
| with file.open("a", encoding="utf-8") as handle: | ||
| for key, value in values.items(): | ||
| delimiter = f"EOF_{uuid.uuid4().hex}" | ||
| handle.write(f"{key}<<{delimiter}\n") | ||
| if isinstance(value, Sequence) and not isinstance(value, str): | ||
| handle.write("\n".join(value)) | ||
| else: | ||
| handle.write(str(value)) | ||
| handle.write(f"\n{delimiter}\n") |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Expand docstring with Parameters section.
Public functions must follow numpy-style docstrings including Parameters sections.
Apply this diff:
def write_github_output(
file: Path, values: Mapping[str, str | Sequence[str]]
) -> None:
- """Append ``values`` to ``file`` using GitHub's multiline syntax."""
+ """Append ``values`` to ``file`` using GitHub's multiline syntax.
+
+ Parameters
+ ----------
+ file:
+ Path to the GitHub Actions output file (typically ``GITHUB_OUTPUT``).
+ values:
+ Mapping of output keys to string or sequence values. Sequence values
+ are joined with newlines.
+ """As per coding guidelines.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def write_github_output( | |
| file: Path, values: Mapping[str, str | Sequence[str]] | |
| ) -> None: | |
| """Append ``values`` to ``file`` using GitHub's multiline syntax.""" | |
| file.parent.mkdir(parents=True, exist_ok=True) | |
| with file.open("a", encoding="utf-8") as handle: | |
| for key, value in values.items(): | |
| delimiter = f"EOF_{uuid.uuid4().hex}" | |
| handle.write(f"{key}<<{delimiter}\n") | |
| if isinstance(value, Sequence) and not isinstance(value, str): | |
| handle.write("\n".join(value)) | |
| else: | |
| handle.write(str(value)) | |
| handle.write(f"\n{delimiter}\n") | |
| def write_github_output( | |
| file: Path, values: Mapping[str, str | Sequence[str]] | |
| ) -> None: | |
| """Append ``values`` to ``file`` using GitHub's multiline syntax. | |
| Parameters | |
| ---------- | |
| file: | |
| Path to the GitHub Actions output file (typically ``GITHUB_OUTPUT``). | |
| values: | |
| Mapping of output keys to string or sequence values. Sequence values | |
| are joined with newlines. | |
| """ | |
| file.parent.mkdir(parents=True, exist_ok=True) | |
| with file.open("a", encoding="utf-8") as handle: | |
| for key, value in values.items(): | |
| delimiter = f"EOF_{uuid.uuid4().hex}" | |
| handle.write(f"{key}<<{delimiter}\n") | |
| if isinstance(value, Sequence) and not isinstance(value, str): | |
| handle.write("\n".join(value)) | |
| else: | |
| handle.write(str(value)) | |
| handle.write(f"\n{delimiter}\n") |
🤖 Prompt for AI Agents
.github/actions/stage/scripts/stage_common/github_output.py around lines 12 to
26: the function docstring is missing a numpy-style Parameters section; update
the docstring to include a Parameters heading that documents `file` (Path) and
`values` (Mapping[str, str | Sequence[str]]) with brief descriptions, and also
include a Returns section stating it returns None (and any Raises if
applicable). Keep the numpy-style formatting consistent with project guidelines.
| def test_public_interface(stage_common: object) -> None: | ||
| """The package should expose the documented public API.""" | ||
|
|
||
| expected = { | ||
| "ArtefactConfig", | ||
| "RESERVED_OUTPUT_KEYS", | ||
| "StageError", | ||
| "StageResult", | ||
| "StagingConfig", | ||
| "load_config", | ||
| "require_env_path", | ||
| "stage_artefacts", | ||
| } | ||
| assert set(stage_common.__all__) == expected |
There was a problem hiding this comment.
Add assert message to the assertion.
Line 23 uses a bare assert. The coding guidelines require: "Use assert …, "message" over bare asserts".
Apply this diff:
- assert set(stage_common.__all__) == expected
+ assert set(stage_common.__all__) == expected, (
+ f"Public API mismatch: expected {expected}, got {set(stage_common.__all__)}"
+ )As per coding guidelines.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def test_public_interface(stage_common: object) -> None: | |
| """The package should expose the documented public API.""" | |
| expected = { | |
| "ArtefactConfig", | |
| "RESERVED_OUTPUT_KEYS", | |
| "StageError", | |
| "StageResult", | |
| "StagingConfig", | |
| "load_config", | |
| "require_env_path", | |
| "stage_artefacts", | |
| } | |
| assert set(stage_common.__all__) == expected | |
| def test_public_interface(stage_common: object) -> None: | |
| """The package should expose the documented public API.""" | |
| expected = { | |
| "ArtefactConfig", | |
| "RESERVED_OUTPUT_KEYS", | |
| "StageError", | |
| "StageResult", | |
| "StagingConfig", | |
| "load_config", | |
| "require_env_path", | |
| "stage_artefacts", | |
| } | |
| assert set(stage_common.__all__) == expected, ( | |
| f"Public API mismatch: expected {expected}, got {set(stage_common.__all__)}" | |
| ) |
🤖 Prompt for AI Agents
In tests_python/test_stage_common_module.py around lines 10 to 23, the final
assertion is a bare assert; update it to include an assertion message that
describes the mismatch (e.g., showing expected vs actual) by changing the assert
to include a comma and a descriptive string so test failures are clearer, for
example: assert set(stage_common.__all__) == expected, "public API does not
match expected: ..." but ensure message is concise and mentions expected and
actual.
| def test_stage_error_is_runtime_error(stage_common: object) -> None: | ||
| """``StageError`` should subclass :class:`RuntimeError`.""" | ||
|
|
||
| error = stage_common.StageError("boom") | ||
| assert isinstance(error, RuntimeError) | ||
| assert str(error) == "boom" |
There was a problem hiding this comment.
Add assert messages to all assertions.
Lines 30-31 use bare asserts. The coding guidelines require: "Use assert …, "message" over bare asserts".
Apply this diff:
- assert isinstance(error, RuntimeError)
- assert str(error) == "boom"
+ assert isinstance(error, RuntimeError), (
+ "StageError must subclass RuntimeError"
+ )
+ assert str(error) == "boom", (
+ f"StageError message incorrect: expected 'boom', got '{error}'"
+ )As per coding guidelines.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def test_stage_error_is_runtime_error(stage_common: object) -> None: | |
| """``StageError`` should subclass :class:`RuntimeError`.""" | |
| error = stage_common.StageError("boom") | |
| assert isinstance(error, RuntimeError) | |
| assert str(error) == "boom" | |
| def test_stage_error_is_runtime_error(stage_common: object) -> None: | |
| """``StageError`` should subclass :class:`RuntimeError`.""" | |
| error = stage_common.StageError("boom") | |
| assert isinstance(error, RuntimeError), ( | |
| "StageError must subclass RuntimeError" | |
| ) | |
| assert str(error) == "boom", ( | |
| f"StageError message incorrect: expected 'boom', got '{error}'" | |
| ) |
🤖 Prompt for AI Agents
In tests_python/test_stage_common_module.py around lines 26 to 31, the two
assertions use bare asserts; update them to include assertion messages per
guidelines. Change assert isinstance(error, RuntimeError) to assert
isinstance(error, RuntimeError), "StageError should subclass RuntimeError" and
change assert str(error) == "boom" to assert str(error) == "boom", "StageError
string representation should be 'boom'".
| def test_require_env_path_returns_path(stage_common: object, workspace: Path) -> None: | ||
| """The environment helper should return a ``Path`` when set.""" | ||
|
|
||
| path = stage_common.require_env_path("GITHUB_WORKSPACE") | ||
| assert path == workspace |
There was a problem hiding this comment.
Add assert message to the assertion.
Line 38 uses a bare assert. The coding guidelines require: "Use assert …, "message" over bare asserts".
Apply this diff:
- assert path == workspace
+ assert path == workspace, (
+ f"require_env_path returned incorrect path: expected {workspace}, got {path}"
+ )As per coding guidelines.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def test_require_env_path_returns_path(stage_common: object, workspace: Path) -> None: | |
| """The environment helper should return a ``Path`` when set.""" | |
| path = stage_common.require_env_path("GITHUB_WORKSPACE") | |
| assert path == workspace | |
| def test_require_env_path_returns_path(stage_common: object, workspace: Path) -> None: | |
| """The environment helper should return a ``Path`` when set.""" | |
| path = stage_common.require_env_path("GITHUB_WORKSPACE") | |
| assert path == workspace, ( | |
| f"require_env_path returned incorrect path: expected {workspace}, got {path}" | |
| ) |
🤖 Prompt for AI Agents
In tests_python/test_stage_common_module.py around lines 34 to 38, the test uses
a bare assert on line 38; update the assertion to include an assertion message
following the project's guideline. Replace the bare assert path == workspace
with an assert that includes a descriptive message (e.g., stating which
environment variable was expected and showing the actual path) so the assertion
becomes assert path == workspace, "expected GITHUB_WORKSPACE to resolve to
<expected>, got <actual>" (interpolating the actual values).
| stage_common: object, workspace: Path, monkeypatch: pytest.MonkeyPatch | ||
| ) -> None: | ||
| """Absolute Windows-style globs should resolve relative to the drive root.""" |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Rename unused fixture parameters and drop unused noqa directives
Stop suppressing ARG001; rename unused fixture arguments to _stage_common and remove # noqa: ARG001. This keeps the fixture for import side effects without lint noise. As per coding guidelines.
-def test_match_candidate_path_handles_windows_drive(
- stage_common: object, workspace: Path, monkeypatch: pytest.MonkeyPatch
+def test_match_candidate_path_handles_windows_drive(
+ _stage_common: object, workspace: Path, monkeypatch: pytest.MonkeyPatch
) -> None:
@@
-def test_glob_root_and_pattern_handles_windows_drive(stage_common: object) -> None:
+def test_glob_root_and_pattern_handles_windows_drive(_stage_common: object) -> None:
@@
-def test_glob_root_and_pattern_returns_wildcard_for_root_only( # noqa: ARG001
- stage_common: object,
+def test_glob_root_and_pattern_returns_wildcard_for_root_only(
+ _stage_common: object,
) -> None:
@@
-def test_glob_root_and_pattern_handles_posix_absolute( # noqa: ARG001
- stage_common: object,
+def test_glob_root_and_pattern_handles_posix_absolute(
+ _stage_common: object,
) -> None:
@@
-def test_glob_root_and_pattern_rejects_relative_paths( # noqa: ARG001
- stage_common: object,
+def test_glob_root_and_pattern_rejects_relative_paths(
+ _stage_common: object,
) -> None:Also applies to: 361-365, 372-375, 386-389, 400-403
🧰 Tools
🪛 Ruff (0.13.3)
203-203: Unused function argument: stage_common
(ARG001)
🤖 Prompt for AI Agents
In tests_python/test_stage_common_staging.py around lines 203-205 (and also
apply the same change at 361-365, 372-375, 386-389, 400-403), rename the unused
fixture parameter stage_common to _stage_common in the function signatures and
remove the trailing “# noqa: ARG001” comments; keep the parameter present only
for import/side-effect purposes but mark it as unused by prefixing with an
underscore and delete the noqa suppression so the linter no longer reports
ARG001.
| root, pattern = helper(PurePosixPath("/tmp/dist/*.zip")) | ||
| assert root == "/" | ||
| assert pattern.endswith("dist/*.zip"), pattern |
There was a problem hiding this comment.
Avoid Bandit S108 by not using “/tmp” in tests
Use a neutral absolute POSIX path that is not “/tmp”, since this path is only parsed, not accessed.
- root, pattern = helper(PurePosixPath("/tmp/dist/*.zip"))
+ root, pattern = helper(PurePosixPath("/sandbox/dist/*.zip"))📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| root, pattern = helper(PurePosixPath("/tmp/dist/*.zip")) | |
| assert root == "/" | |
| assert pattern.endswith("dist/*.zip"), pattern | |
| root, pattern = helper(PurePosixPath("/sandbox/dist/*.zip")) | |
| assert root == "/" | |
| assert pattern.endswith("dist/*.zip"), pattern |
🧰 Tools
🪛 Ruff (0.13.3)
395-395: Probable insecure usage of temporary file or directory: "/tmp/dist/*.zip"
(S108)
🤖 Prompt for AI Agents
In tests_python/test_stage_common_staging.py around lines 395 to 397, the test
uses the literal "/tmp" which triggers Bandit S108; change the input to a
neutral absolute POSIX path that won’t be interpreted as a temporary directory
(for example use "/abs/dist/*.zip" or "/some/path/dist/*.zip") since the path is
only parsed and not accessed, update the assertion accordingly to still expect
root == "/" and pattern.endswith("dist/*.zip").
|
@coderabbitai pause |
✅ Actions performedReviews paused. |
1 participant
Summary
Testing
https://chatgpt.com/codex/tasks/task_e_68e7d0a7807c83228d0dbf3ddd828811