Conversation
Reviewer's GuideRefactored the monolithic command implementation into focused submodules and introduced a new registration API, layering structured configuration, execution, piping, quoting, and unified error handling to enforce per‐filter output limits and improve maintainability and testability. Entity relationship diagram for command tempfile and output limit enforcementerDiagram
COMMAND_CONFIG ||--o| COMMAND_TEMP_DIR : manages
COMMAND_TEMP_DIR ||--o| COMMAND_TEMP_FILE : creates
COMMAND_CONFIG ||--o| PIPE_SPEC : configures
PIPE_SPEC ||--o| PIPE_LIMIT : enforces
PIPE_LIMIT ||--o| COMMAND_FAILURE : triggers
COMMAND_TEMP_FILE ||--o| PIPE_OUTCOME : produces
PIPE_OUTCOME ||--o| STDOUT_RESULT : returns
STDOUT_RESULT ||--o| COMMAND_FAILURE : triggers
Class diagram for refactored command module subcomponentsclassDiagram
class CommandConfig {
+max_capture_bytes: u64
+max_stream_bytes: u64
-temp_dir: CommandTempDir
+new(max_capture_bytes, max_stream_bytes, workspace_root, workspace_root_path)
+create_tempfile(label)
}
class CommandTempDir {
-workspace_root: Arc<Dir>
-workspace_root_path: Option<Arc<Utf8PathBuf>>
-relative: Utf8PathBuf
+new(workspace_root, workspace_root_path)
+create(label)
}
class CommandTempFile {
-file: NamedTempFile
+new(file)
+as_file_mut()
+path()
+into_path()
}
class PipeSpec {
-stream: OutputStream
-mode: OutputMode
-limit: u64
+new(stream, mode, limit)
+stream()
+mode()
+limit()
+into_limit()
}
class PipeLimit {
-spec: PipeSpec
-consumed: u64
+record(read)
}
class CommandOptions {
-stdout_mode: OutputMode
+from_value(options)
+from_mode_str(mode)
+stdout_mode()
}
class CommandContext {
-config: Arc<CommandConfig>
-options: CommandOptions
+new(config, options)
+stdout_mode()
+config()
+config_handle()
}
class GrepCall {
+pattern: str
+flags: Option<Value>
+new(pattern, flags)
}
class CommandLocation {
+template: str
+command: str
+new(template, command)
+describe()
}
class CommandFailure {
+Spawn(io::Error)
+Io(io::Error)
+BrokenPipe
+Exit
+OutputLimit
+Timeout
}
class StdoutResult {
+Bytes(Vec<u8>)
+Tempfile(Utf8PathBuf)
}
class PipeOutcome {
+Bytes(Vec<u8>)
+Tempfile(Utf8PathBuf)
}
CommandConfig --> CommandTempDir
CommandTempDir --> CommandTempFile
PipeSpec --> PipeLimit
CommandContext --> CommandConfig
CommandContext --> CommandOptions
GrepCall --> CommandContext
CommandFailure --> CommandLocation
PipeOutcome --> CommandTempFile
StdoutResult --> PipeOutcome
File-Level Changes
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
Note Reviews pausedUse the following commands to manage reviews:
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. Summary by CodeRabbit
WalkthroughSummarise the new command-execution subsystem: workspace-scoped tempfile management, output modes and per-pipe byte limits, command context and error translation, cross-platform execution with timeouts, threaded pipe readers for capture or tempfile streaming, platform-aware quoting, shell/grep filters, result types, tests and docs edits. Changes
Sequence Diagram(s)sequenceDiagram
participant Template
participant Filter as Filter (shell/grep)
participant Context as CommandContext
participant Exec as Execution
participant Pipes as Pipe Readers
participant OS as OS Process
Template->>Filter: call filter(value, options)
Filter->>Context: construct CommandContext (config + options)
Filter->>Exec: invoke run_command / run_program
Exec->>OS: spawn child process
Exec->>Pipes: spawn stdout/stderr readers (Capture | Tempfile)
Pipes-->>Exec: stream data → Bytes or Tempfile path
Exec->>OS: wait_for_exit (with timeout)
OS-->>Exec: return exit status
alt Success
Exec->>Filter: return StdoutResult (Bytes | Tempfile)
Filter->>Template: return Value (bytes or filepath)
else Failure
Exec->>Filter: return CommandFailure
Filter->>Template: map to MiniJinja Error via command_error
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related issues
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
Comment |
![codescene-delta-analysis[bot]](https://avatars.githubusercontent.com/in/53921?s=60&v=4){kind=small}
… handling Introduce a new command module for executing shell commands and grep within MiniJinja templates. This includes: - Command configuration for output byte limits and tempfile management. - Command execution helpers with timeout and platform-specific shell support. - Filters for `shell` and `grep` commands with argument quoting and input handling. - Pipe reader management enforcing output limits and optionally streaming to temp files. - Detailed error handling converting command failures to template errors. - Platform-aware quoting utilities for safe shell argument escaping. - Context and options propagation for commands within templates. This adds a robust and secure foundation for running shell commands in templates, with careful resource and error management, supporting both capture and streaming output modes. Co-authored-by: terragon-labs[bot] <terragon-labs[bot]@users.noreply.github.com>
leynos
force-pushed
the
terragon/refactor-command-module-submodules-5g6gle
branch
from
7640260 to
53d2d38
Compare
…sistency Co-authored-by: terragon-labs[bot] <terragon-labs[bot]@users.noreply.github.com>
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey there - I've reviewed your changes and they look great!
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location> `src/stdlib/command/config.rs:22` </location>
<code_context>
+use super::error::CommandFailure;
+
+#[derive(Clone)]
+pub(crate) struct CommandConfig {
+ pub(crate) max_capture_bytes: u64,
+ pub(crate) max_stream_bytes: u64,
</code_context>
<issue_to_address>
**issue (complexity):** Consider removing the CommandTempDir and CommandTempFile types and handling tempfile creation directly in CommandConfig to simplify the code.
You can collapse the two new types (`CommandTempDir`/`CommandTempFile`) into one method on `CommandConfig`, and just return a `NamedTempFile`. That flattens all of the “builder + UTF-8 conversions + error wrapping” logic into a single place without losing any functionality.
For example, replace:
```rust
pub(crate) struct CommandConfig { … temp_dir: CommandTempDir, }
impl CommandConfig {
pub(super) fn create_tempfile(&self, label: &str) -> io::Result<CommandTempFile> {
self.temp_dir.create(label)
}
}
// lots of indirection in CommandTempDir::create & CommandTempFile
```
with:
```rust
impl CommandConfig {
pub(crate) fn create_tempfile(&self, label: &str) -> io::Result<NamedTempFile> {
// ensure we have a UTF-8 workspace root
let root = self
.workspace_root_path
.as_ref()
.ok_or_else(|| io::Error::new(
io::ErrorKind::InvalidInput,
"workspace root path must be configured for command tempfiles",
))?;
// prepare on-disk directory
let tmp_dir = root.join(DEFAULT_COMMAND_TEMP_DIR);
self.workspace_root.create_dir_all(DEFAULT_COMMAND_TEMP_DIR)?;
fs::create_dir_all(tmp_dir.as_std_path())?;
// build & create tempfile
Builder::new()
.prefix(&sanitize_label(label))
.suffix(".tmp")
.tempfile_in(tmp_dir.as_std_path())
.map_err(|e| io::Error::new(e.kind(), format!("failed to create command tempfile for '{label}': {e}")))
}
}
// you can keep sanitize_label as you have it
fn sanitize_label(label: &str) -> String { … }
```
Then drop both `CommandTempDir` and `CommandTempFile`. Whenever you need to persist, just call:
```rust
let mut f = config.create_tempfile("stdout")?;
f.flush()?;
let p = f.into_temp_path().keep()?; // same as your old into_path
```
This removes two entire types and ~60 lines of indirection, without changing any of your existing tests or behavior.
</issue_to_address>
### Comment 2
<location> `src/stdlib/command/config.rs:111` </location>
<code_context>
+ &mut self.file
+ }
+
+ #[cfg(test)]
+ pub(super) fn path(&self) -> &std::path::Path {
+ self.file.path()
</code_context>
<issue_to_address>
**issue (review_instructions):** Add behavioural tests for new feature; only unit tests are present.
The tests provided are unit tests for the temp file and label sanitization logic, but there are no behavioural tests demonstrating end-to-end usage or integration of the command configuration in a real scenario. Add behavioural tests to cover typical usage patterns.
<details>
<summary>Review instructions:</summary>
**Path patterns:** `**/*`
**Instructions:**
For any new feature or change to an existing feature, both behavioural *and* unit tests are required.
</details>
</issue_to_address>
### Comment 3
<location> `src/stdlib/command/pipes.rs:9` </location>
<code_context>
+ &mut self.file
+ }
+
+ #[cfg(test)]
+ pub(super) fn path(&self) -> &std::path::Path {
+ self.file.path()
</code_context>
<issue_to_address>
**issue (review_instructions):** Add behavioural tests for new feature; only unit tests are present.
The tests provided are unit tests for the pipe reading logic, but there are no behavioural tests demonstrating end-to-end usage or integration of the pipe reader in a real scenario. Add behavioural tests to cover typical usage patterns.
<details>
<summary>Review instructions:</summary>
**Path patterns:** `**/*`
**Instructions:**
For any new feature or change to an existing feature, both behavioural *and* unit tests are required.
</details>
</issue_to_address>
### Comment 4
<location> `src/stdlib/command/error.rs:12` </location>
<code_context>
+ context::CommandLocation,
+};
+
+#[derive(Debug)]
+pub(super) enum CommandFailure {
+ Spawn(io::Error),
</code_context>
<issue_to_address>
**issue (review_instructions):** Add behavioural and unit tests for error handling logic.
There are no tests provided for the error handling logic. Add both unit and behavioural tests to verify that error variants are correctly constructed and translated into Jinja errors.
<details>
<summary>Review instructions:</summary>
**Path patterns:** `**/*`
**Instructions:**
For any new feature or change to an existing feature, both behavioural *and* unit tests are required.
</details>
</issue_to_address>
### Comment 5
<location> `src/stdlib/command/quote.rs:86` </location>
<code_context>
+ }
+}
+
+#[cfg(all(windows, test))]
+mod tests {
+ use super::*;
</code_context>
<issue_to_address>
**issue (review_instructions):** Add behavioural tests for quoting logic; only unit tests are present.
The tests provided are unit tests for the quoting logic, but there are no behavioural tests demonstrating quoting in the context of actual command execution. Add behavioural tests to cover typical usage patterns.
<details>
<summary>Review instructions:</summary>
**Path patterns:** `**/*`
**Instructions:**
For any new feature or change to an existing feature, both behavioural *and* unit tests are required.
</details>
</issue_to_address>
### Comment 6
<location> `src/stdlib/command/filters.rs:19` </location>
<code_context>
+ value_from_bytes,
+};
+
+pub(super) fn execute_shell(
+ state: &State,
+ value: &Value,
</code_context>
<issue_to_address>
**issue (review_instructions):** Add behavioural and unit tests for filter implementations.
There are no tests provided for the filter implementations. Add both unit and behavioural tests to verify that the shell and grep filters work as expected, including error handling and edge cases.
<details>
<summary>Review instructions:</summary>
**Path patterns:** `**/*`
**Instructions:**
For any new feature or change to an existing feature, both behavioural *and* unit tests are required.
</details>
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
…ve error tests - Removed CommandTempDir and CommandTempFile wrappers, merging tempfile creation directly into CommandConfig. - Changed tempfile creation to return tempfile::NamedTempFile directly. - Added persist_tempfile helper to handle tempfile persistence consistently. - Updated pipes module to use new tempfile handling helpers. - Added tests for command error formatting of exit status and timeouts. - Other misc cleanups around tempfile lifetime and usage. This refactoring removes unnecessary abstractions around command tempfiles, improving code clarity and maintainability. Added test coverage increases robustness. Co-authored-by: terragon-labs[bot] <terragon-labs[bot]@users.noreply.github.com>
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 (6)
docs/ortho-config-users-guide.md(3 hunks)src/stdlib/command/config.rs(1 hunks)src/stdlib/command/error.rs(1 hunks)src/stdlib/command/pipes.rs(1 hunks)src/stdlib/command/quote.rs(1 hunks)tests/std_filter_tests/command_filters.rs(2 hunks)
🧰 Additional context used
📓 Path-based instructions (6)
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/ortho-config-users-guide.md
**/*.{rs,md}
📄 CodeRabbit inference engine (AGENTS.md)
Use en-GB-oxendict spelling and grammar in comments and documentation (exceptions allowed for external API names)
Files:
docs/ortho-config-users-guide.mdsrc/stdlib/command/error.rssrc/stdlib/command/quote.rstests/std_filter_tests/command_filters.rssrc/stdlib/command/pipes.rssrc/stdlib/command/config.rs
docs/**/*.md
📄 CodeRabbit inference engine (AGENTS.md)
docs/**/*.md: Use docs/ as the reference knowledge base; proactively update docs/ when decisions, requirements, dependencies, or architecture change
Documentation in docs/ must use en-GB-oxendict spelling and grammar (LICENSE file name exempt)
docs/**/*.md: The word “outwith” is acceptable in documentation
Use the Oxford comma where it aids comprehension
Treat company names as collective nouns (e.g., “Lille Industries are …”)
Write headings in sentence case
Use Markdown headings (#,##,###, …) in order without skipping levels
Follow markdownlint recommendations
Provide code blocks and lists using standard Markdown syntax
Always provide a language identifier for fenced code blocks; useplaintextfor non-code text
Use-as the first-level bullet; renumber ordered lists when items change
Prefer inline links[text](url)or angle brackets<url>
Ensure blank lines before and after bulleted lists and fenced code blocks
Ensure tables have a delimiter line below the header row
Expand uncommon acronyms on first use (e.g., Continuous Integration (CI))
Wrap paragraphs at 80 columns
Wrap code at 120 columns in documentation
Do not wrap tables in documentation
Use footnotes referenced with[^label]
Include Mermaid diagrams where they add clarity
When embedding figures, useand provide brief, descriptive alt text
Add a short descriptive sentence before each Mermaid diagram for screen readers
Files:
docs/ortho-config-users-guide.md
**/*.md
📄 CodeRabbit inference engine (AGENTS.md)
**/*.md: Validate Markdown using make markdownlint; run make fmt after documentation changes to format Markdown and fix table markup
Validate Mermaid diagrams in Markdown using make nixie
Wrap Markdown paragraphs and bullet points at 80 columns
Wrap code blocks in Markdown at 120 columns
Do not wrap tables and headings in Markdown
Use dashes (-) for list bullets in Markdown
Use GitHub-flavoured Markdown footnotes ([^1]) for references
Files:
docs/ortho-config-users-guide.md
⚙️ CodeRabbit configuration file
**/*.md: * Avoid 2nd person or 1st person pronouns ("I", "you", "we")
- Use en-GB-oxendict (-ize / -yse / -our) spelling and grammar
- Headings must not be wrapped.
- Documents must start with a level 1 heading
- Headings must correctly increase or decrease by no more than one level at a time
- Use GitHub-flavoured Markdown style for footnotes and endnotes.
- Numbered footnotes must be numbered by order of appearance in the document.
Files:
docs/ortho-config-users-guide.md
**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
**/*.rs: Every Rust module must begin with a module-level comment using //! explaining the module's purpose and utility
Document public APIs with Rustdoc comments (///) so documentation can be generated with cargo doc
Prefer immutable data; avoid unnecessary mut bindings
Handle errors with Result instead of panicking where feasible
Prefer semantic error enums deriving std::error::Error via thiserror for inspectable conditions
Clippy warnings must be denied; do not silence lints except as a last resort
Lint suppressions must be tightly scoped and include a clear reason; prefer #[expect(...)] over #[allow(...)]
Place function attributes after doc comments
Do not use return in single-line functions
Prefer single-line function definitions where appropriate (e.g., pub fn new(id: u64) -> Self { Self(id) })
Use predicate functions for conditional criteria with more than two branches
Where a function is too long, extract meaningfully named helpers adhering to separation of concerns and CQRS
Where a function has too many parameters, group related parameters into meaningfully named structs
When returning large error data, consider using Arc to reduce data moved
Prefer .expect() over .unwrap() on Results/Options
Use concat!() to combine long string literals instead of escaping newlines with backslashes
Use NewTypes to model domain values; prefer explicit tuple structs; leverage newt-hype and the-newtype crates as described; provide appropriate From/AsRef/TryFrom implementations and avoid orphan-rule-violating impls
Keep each Rust code file under 400 lines; split long switches/dispatch tables by feature; move large test data to external files
Use functions and composition to avoid repetition; extract reusable logic; prefer declarative constructs (iterators/generators/comprehensions) when readable
Small, single-responsibility functions; obey command/query segregation
Name things precisely; booleans should use is/has/should prefixes
Comment why, not what; explain assumptions, e...
Files:
src/stdlib/command/error.rssrc/stdlib/command/quote.rstests/std_filter_tests/command_filters.rssrc/stdlib/command/pipes.rssrc/stdlib/command/config.rs
⚙️ CodeRabbit configuration file
**/*.rs: * Seek to keep the cognitive complexity of functions no more than 9.
- Adhere to single responsibility and CQRS
- Place function attributes after doc comments.
- Do not use
returnin single-line functions.- Move conditionals with >2 branches into a predicate function.
- Avoid
unsafeunless absolutely necessary.- Every module must begin with a
//!doc comment that explains the module's purpose and utility.- Comments and docs must follow en-GB-oxendict (-ize / -yse / -our) spelling and grammar
- Lints must not be silenced except as a last resort.
#[allow]is forbidden.- Only narrowly scoped
#[expect(lint, reason = "...")]is allowed.- No lint groups, no blanket or file-wide suppression.
- Include
FIXME:with link if a fix is expected.- Where code is only used by specific features, it must be conditionally compiled or a conditional expectation for unused_code applied.
- Use
rstestfixtures for shared setup and to avoid repetition between tests.- Replace duplicated tests with
#[rstest(...)]parameterised cases.- Prefer
mockallfor mocks/stubs.- Prefer
.expect()over.unwrap()in tests..expect()and.unwrap()are forbidden outside of tests. Errors must be propagated.- Ensure that any API or behavioural changes are reflected in the documentation in
docs/- Ensure that any completed roadmap steps are recorded in the appropriate roadmap in
docs/- Files must not exceed 400 lines in length
- Large modules must be decomposed
- Long match statements or dispatch tables should be decomposed by domain and collocated with targets
- Large blocks of inline data (e.g., test fixtures, constants or templates) must be moved to external files and inlined at compile-time or loaded at run-time.
- Environment access (env::set_var and env::remove_var) are always unsafe in Rust 2024 and MUST be marked as such
- For testing of functionality depending upon environment variables, dependency injection and...
Files:
src/stdlib/command/error.rssrc/stdlib/command/quote.rstests/std_filter_tests/command_filters.rssrc/stdlib/command/pipes.rssrc/stdlib/command/config.rs
**/tests/**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
**/tests/**/*.rs: Use rstest fixtures for shared setup
Replace duplicated tests with #[rstest(...)] parameterised cases
Prefer mockall for mocks and stubs
Files:
tests/std_filter_tests/command_filters.rs
🧬 Code graph analysis (4)
src/stdlib/command/error.rs (2)
src/stdlib/command/context.rs (5)
config(24-26)new(16-18)new(39-41)new(51-53)describe(55-57)src/stdlib/command/config.rs (7)
new(27-40)new(135-141)stream(143-145)mode(147-149)limit(151-153)describe(90-95)describe(105-110)
tests/std_filter_tests/command_filters.rs (2)
tests/std_filter_tests/support.rs (2)
stdlib_env_with_state(46-48)register_template(26-36)src/stdlib/mod.rs (1)
is_impure(321-323)
src/stdlib/command/pipes.rs (2)
src/stdlib/command/mod_backup.rs (22)
config(421-423)spawn_pipe_reader(1020-1029)read_pipe(1054-1069)join_reader(1031-1049)mode(280-282)create_empty_tempfile(1123-1129)new(101-112)new(127-133)new(186-188)new(268-274)new(413-415)new(436-438)new(448-450)new(464-466)cleanup_readers(822-834)join_pipe_for_cleanup(836-851)handle_stdin_result(853-881)limit(284-286)read_pipe_capture(1073-1088)read_pipe_tempfile(1092-1121)path(199-201)stream(276-278)src/stdlib/command/config.rs (5)
mode(147-149)new(27-40)new(135-141)limit(151-153)stream(143-145)
src/stdlib/command/config.rs (3)
src/stdlib/command/error.rs (3)
new(149-151)new(162-168)from(34-36)src/stdlib/command/context.rs (5)
new(16-18)new(39-41)new(51-53)stdout_mode(20-22)config(24-26)src/stdlib/mod.rs (1)
workspace_root_path(144-146)
🪛 LanguageTool
docs/ortho-config-users-guide.md
[style] ~514-~514: Since ownership is already implied, this phrasing may be redundant.
Context: ...E__POOL_SIZE. If the nested struct has its own prefix (DB`), then the environment var...
(PRP_OWN)
⏰ Context from checks skipped due to timeout of 120000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)
- GitHub Check: release / build-macos (x86_64-apple-darwin, macos-13, macos-x86_64, macos-x86_64) / Build macos artefacts
- GitHub Check: release / build-macos (aarch64-apple-darwin, macos-14, macos-arm64, macos-aarch64) / Build macos artefacts
- GitHub Check: release / build-windows (aarch64-pc-windows-msvc, arm64, arm64, windows-aarch64) / Build windows artefacts
- GitHub Check: release / build-windows (x86_64-pc-windows-msvc, amd64, x64, windows-x86_64) / Build windows artefacts
- GitHub Check: release / build-linux (aarch64-unknown-linux-gnu, linux-arm64, arm64) / Build linux artefacts
- GitHub Check: release / build-linux (x86_64-unknown-linux-gnu, linux-amd64, amd64) / Build linux artefacts
- GitHub Check: Sourcery review
- GitHub Check: build-test (ubuntu-latest, stable)
🔇 Additional comments (4)
docs/ortho-config-users-guide.md (1)
170-174: Keep the re-export guidance tightenedKeep this wording; it clarifies the serde_json default toggle and the optional parser paths for readers.
src/stdlib/command/quote.rs (1)
24-136: Retain the Windows quoting coverageRetain the newline guard and the comprehensive metacharacter tests; they lock down cmd.exe edge cases across %, !, carets, and embedded quotes.
tests/std_filter_tests/command_filters.rs (1)
219-510: Preserve the empty command validation suitePreserve these scenarios; they pin the user-facing error text and the impure state contract when validation short-circuits shell or grep execution.
src/stdlib/command/pipes.rs (1)
130-198: Keep the tempfile persistence flowKeep the flush-before-persist sequence; it prevents buffered data loss, and the UTF-8 guard shields downstream consumers from invalid paths.
…pport Introduce CommandConfig struct to manage shell helper configurations including max_capture_bytes and max_stream_bytes limits, and workspace root handles. Add create_tempfile method to create scoped temp files beneath configured directories. Also modify error message formatting to use as_secs_f64 for timeout errors in command error handling. Co-authored-by: terragon-labs[bot] <terragon-labs[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (1)
src/stdlib/command/error.rs (1)
112-120: Past timeout formatting issue already resolved.The previous review comment suggested using
duration.as_secs_f64()instead ofduration.as_secs()to avoid truncating sub-second timeouts. Line 118 already implements this fix correctly.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
src/stdlib/command/config.rs(1 hunks)src/stdlib/command/error.rs(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
**/*.rs: Every Rust module must begin with a module-level comment using //! explaining the module's purpose and utility
Document public APIs with Rustdoc comments (///) so documentation can be generated with cargo doc
Prefer immutable data; avoid unnecessary mut bindings
Handle errors with Result instead of panicking where feasible
Prefer semantic error enums deriving std::error::Error via thiserror for inspectable conditions
Clippy warnings must be denied; do not silence lints except as a last resort
Lint suppressions must be tightly scoped and include a clear reason; prefer #[expect(...)] over #[allow(...)]
Place function attributes after doc comments
Do not use return in single-line functions
Prefer single-line function definitions where appropriate (e.g., pub fn new(id: u64) -> Self { Self(id) })
Use predicate functions for conditional criteria with more than two branches
Where a function is too long, extract meaningfully named helpers adhering to separation of concerns and CQRS
Where a function has too many parameters, group related parameters into meaningfully named structs
When returning large error data, consider using Arc to reduce data moved
Prefer .expect() over .unwrap() on Results/Options
Use concat!() to combine long string literals instead of escaping newlines with backslashes
Use NewTypes to model domain values; prefer explicit tuple structs; leverage newt-hype and the-newtype crates as described; provide appropriate From/AsRef/TryFrom implementations and avoid orphan-rule-violating impls
Keep each Rust code file under 400 lines; split long switches/dispatch tables by feature; move large test data to external files
Use functions and composition to avoid repetition; extract reusable logic; prefer declarative constructs (iterators/generators/comprehensions) when readable
Small, single-responsibility functions; obey command/query segregation
Name things precisely; booleans should use is/has/should prefixes
Comment why, not what; explain assumptions, e...
Files:
src/stdlib/command/error.rssrc/stdlib/command/config.rs
⚙️ CodeRabbit configuration file
**/*.rs: * Seek to keep the cognitive complexity of functions no more than 9.
- Adhere to single responsibility and CQRS
- Place function attributes after doc comments.
- Do not use
returnin single-line functions.- Move conditionals with >2 branches into a predicate function.
- Avoid
unsafeunless absolutely necessary.- Every module must begin with a
//!doc comment that explains the module's purpose and utility.- Comments and docs must follow en-GB-oxendict (-ize / -yse / -our) spelling and grammar
- Lints must not be silenced except as a last resort.
#[allow]is forbidden.- Only narrowly scoped
#[expect(lint, reason = "...")]is allowed.- No lint groups, no blanket or file-wide suppression.
- Include
FIXME:with link if a fix is expected.- Where code is only used by specific features, it must be conditionally compiled or a conditional expectation for unused_code applied.
- Use
rstestfixtures for shared setup and to avoid repetition between tests.- Replace duplicated tests with
#[rstest(...)]parameterised cases.- Prefer
mockallfor mocks/stubs.- Prefer
.expect()over.unwrap()in tests..expect()and.unwrap()are forbidden outside of tests. Errors must be propagated.- Ensure that any API or behavioural changes are reflected in the documentation in
docs/- Ensure that any completed roadmap steps are recorded in the appropriate roadmap in
docs/- Files must not exceed 400 lines in length
- Large modules must be decomposed
- Long match statements or dispatch tables should be decomposed by domain and collocated with targets
- Large blocks of inline data (e.g., test fixtures, constants or templates) must be moved to external files and inlined at compile-time or loaded at run-time.
- Environment access (env::set_var and env::remove_var) are always unsafe in Rust 2024 and MUST be marked as such
- For testing of functionality depending upon environment variables, dependency injection and...
Files:
src/stdlib/command/error.rssrc/stdlib/command/config.rs
**/*.{rs,md}
📄 CodeRabbit inference engine (AGENTS.md)
Use en-GB-oxendict spelling and grammar in comments and documentation (exceptions allowed for external API names)
Files:
src/stdlib/command/error.rssrc/stdlib/command/config.rs
🧬 Code graph analysis (2)
src/stdlib/command/error.rs (2)
src/stdlib/command/context.rs (5)
config(24-26)new(16-18)new(39-41)new(51-53)describe(55-57)src/stdlib/command/config.rs (7)
new(37-50)new(148-154)stream(156-158)mode(160-162)limit(164-166)describe(103-108)describe(118-123)
src/stdlib/command/config.rs (5)
src/stdlib/command/error.rs (3)
new(149-151)new(162-168)from(34-36)src/stdlib/command/context.rs (5)
new(16-18)new(39-41)new(51-53)stdout_mode(20-22)config(24-26)src/stdlib/command/mod_backup.rs (29)
new(101-112)new(127-133)new(186-188)new(268-274)new(413-415)new(436-438)new(448-450)new(464-466)new(477-483)from(999-1001)create_tempfile(114-116)sanitize_label(162-175)stream(276-278)mode(280-282)limit(284-286)into_limit(288-293)record(302-318)from_value(327-364)default(387-391)from_mode_str(366-379)stdout_mode(381-383)stdout_mode(417-419)test_command_config(1152-1165)path(199-201)config(421-423)sanitize_label_replaces_disallowed_characters(1168-1170)command_tempfile_drop_removes_file(1173-1183)command_tempfile_into_path_persists_file(1186-1194)command_tempdir_requires_workspace_root_path(1322-1338)src/stdlib/mod.rs (1)
workspace_root_path(144-146)src/stdlib/command/pipes.rs (1)
test_command_config(208-221)
🔍 Remote MCP Deepwiki
Found repository-level facts that matter for reviewing this PR — things to verify in the new command submodules (short checklist with sources):
-
Template engine: MiniJinja is used with Strict undefined behavior; env(), glob(), and the stdlib filters are registered on the Environment. Confirm the PR’s register() wiring and that filters (shell/grep) are added consistently and mark impure when intended.
-
Shell-quoting / command safety: The project requires POSIX shell escaping via shell-quote and validates final commands with shlex. Review quote.rs, cmd interpolation, and ninja_gen integration for correctness (POSIX vs Windows differences, no unvalidated concatenation).
-
Windows quoting: The codebase documents a distinct Windows quoting strategy (reject line breaks, special escaping/percent handling). Verify PR’s Windows branch matches those constraints/tests.
-
Filesystem / tempfiles: I/O must use cap-std and camino (UTF‑8 paths); tempfiles are managed via NamedTempFile + RAII and runner uses Cow for emit vs temp. Check create_tempfile, temp persistence, UTF‑8 path validation, and workspace-root handling.
-
Error handling pattern: Project standard is thiserror for domain errors + anyhow for contextual propagation (+ miette diagnostics). Ensure CommandFailure → command_error and conversions follow that pattern and produce actionable messages.
-
Tests & CI expectations: Tests use rstest, insta snapshot, cucumber BDD; CI enforces cargo fmt, clippy (deny warnings), and full test suite. Confirm new unit/integration tests follow existing fixture/style and snapshots are deterministic (sorted output, stable IDs).
-
Runner / lifecycle integration: The runner and process code rely on particular tempfile and process patterns (create temp ninja file, keep guard until subprocess ends). Verify the new modules’ tempfile lifecycle and error mapping integrate cleanly with runner/process expectations.
-
Note on related issue #233: I cannot access external issue trackers from here, so I couldn't fetch the live text of issue #233 — if you need that context, please paste it or request a web lookup.
Sources/tools used: DeepWiki repository docs (architecture, template system, manifest/runner/CI/testing pages).,,
⏰ 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, macos-aarch64) / Build macos artefacts
- GitHub Check: release / build-linux (aarch64-unknown-linux-gnu, linux-arm64, arm64) / Build linux artefacts
- GitHub Check: release / build-linux (x86_64-unknown-linux-gnu, linux-amd64, amd64) / Build linux artefacts
- GitHub Check: release / build-macos (x86_64-apple-darwin, macos-13, macos-x86_64, macos-x86_64) / Build macos artefacts
- GitHub Check: release / build-windows (x86_64-pc-windows-msvc, amd64, x64, windows-x86_64) / Build windows artefacts
- GitHub Check: release / build-windows (aarch64-pc-windows-msvc, arm64, arm64, windows-aarch64) / Build windows artefacts
- GitHub Check: build-test (ubuntu-latest, stable)
- GitHub Check: Sourcery review
…helpers - Introduce OutputMode and OutputStream enums to manage command output handling. - Implement PipeSpec and PipeLimit to track output limits and mode per stream. - Enhance CommandOptions to parse stdout mode from options. - Augment error messages in command error handling with more detailed output info. - Add tests covering spawn errors, broken pipe, and output limit errors for robustness. Co-authored-by: terragon-labs[bot] <terragon-labs[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
src/stdlib/command/error.rs (1)
34-38: Format as single-line per coding guidelines.Collapse this impl to a single line for consistency with the project's style guidelines for simple functions.
[As per coding guidelines]
impl From<io::Error> for CommandFailure { - fn from(err: io::Error) -> Self { - Self::Io(err) - } + fn from(err: io::Error) -> Self { Self::Io(err) } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
src/stdlib/command/config.rs(1 hunks)src/stdlib/command/error.rs(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
**/*.rs: Every Rust module must begin with a module-level comment using //! explaining the module's purpose and utility
Document public APIs with Rustdoc comments (///) so documentation can be generated with cargo doc
Prefer immutable data; avoid unnecessary mut bindings
Handle errors with Result instead of panicking where feasible
Prefer semantic error enums deriving std::error::Error via thiserror for inspectable conditions
Clippy warnings must be denied; do not silence lints except as a last resort
Lint suppressions must be tightly scoped and include a clear reason; prefer #[expect(...)] over #[allow(...)]
Place function attributes after doc comments
Do not use return in single-line functions
Prefer single-line function definitions where appropriate (e.g., pub fn new(id: u64) -> Self { Self(id) })
Use predicate functions for conditional criteria with more than two branches
Where a function is too long, extract meaningfully named helpers adhering to separation of concerns and CQRS
Where a function has too many parameters, group related parameters into meaningfully named structs
When returning large error data, consider using Arc to reduce data moved
Prefer .expect() over .unwrap() on Results/Options
Use concat!() to combine long string literals instead of escaping newlines with backslashes
Use NewTypes to model domain values; prefer explicit tuple structs; leverage newt-hype and the-newtype crates as described; provide appropriate From/AsRef/TryFrom implementations and avoid orphan-rule-violating impls
Keep each Rust code file under 400 lines; split long switches/dispatch tables by feature; move large test data to external files
Use functions and composition to avoid repetition; extract reusable logic; prefer declarative constructs (iterators/generators/comprehensions) when readable
Small, single-responsibility functions; obey command/query segregation
Name things precisely; booleans should use is/has/should prefixes
Comment why, not what; explain assumptions, e...
Files:
src/stdlib/command/error.rssrc/stdlib/command/config.rs
⚙️ CodeRabbit configuration file
**/*.rs: * Seek to keep the cognitive complexity of functions no more than 9.
- Adhere to single responsibility and CQRS
- Place function attributes after doc comments.
- Do not use
returnin single-line functions.- Move conditionals with >2 branches into a predicate function.
- Avoid
unsafeunless absolutely necessary.- Every module must begin with a
//!doc comment that explains the module's purpose and utility.- Comments and docs must follow en-GB-oxendict (-ize / -yse / -our) spelling and grammar
- Lints must not be silenced except as a last resort.
#[allow]is forbidden.- Only narrowly scoped
#[expect(lint, reason = "...")]is allowed.- No lint groups, no blanket or file-wide suppression.
- Include
FIXME:with link if a fix is expected.- Where code is only used by specific features, it must be conditionally compiled or a conditional expectation for unused_code applied.
- Use
rstestfixtures for shared setup and to avoid repetition between tests.- Replace duplicated tests with
#[rstest(...)]parameterised cases.- Prefer
mockallfor mocks/stubs.- Prefer
.expect()over.unwrap()in tests..expect()and.unwrap()are forbidden outside of tests. Errors must be propagated.- Ensure that any API or behavioural changes are reflected in the documentation in
docs/- Ensure that any completed roadmap steps are recorded in the appropriate roadmap in
docs/- Files must not exceed 400 lines in length
- Large modules must be decomposed
- Long match statements or dispatch tables should be decomposed by domain and collocated with targets
- Large blocks of inline data (e.g., test fixtures, constants or templates) must be moved to external files and inlined at compile-time or loaded at run-time.
- Environment access (env::set_var and env::remove_var) are always unsafe in Rust 2024 and MUST be marked as such
- For testing of functionality depending upon environment variables, dependency injection and...
Files:
src/stdlib/command/error.rssrc/stdlib/command/config.rs
**/*.{rs,md}
📄 CodeRabbit inference engine (AGENTS.md)
Use en-GB-oxendict spelling and grammar in comments and documentation (exceptions allowed for external API names)
Files:
src/stdlib/command/error.rssrc/stdlib/command/config.rs
🧬 Code graph analysis (2)
src/stdlib/command/error.rs (2)
src/stdlib/command/context.rs (5)
config(24-26)new(16-18)new(39-41)new(51-53)describe(55-57)src/stdlib/command/config.rs (5)
new(37-50)new(162-168)stream(171-173)describe(107-112)describe(126-131)
src/stdlib/command/config.rs (3)
src/stdlib/command/context.rs (5)
new(16-18)new(39-41)new(51-53)stdout_mode(20-22)config(24-26)src/stdlib/command/mod_backup.rs (15)
new(101-112)new(127-133)new(186-188)new(268-274)new(413-415)new(436-438)new(448-450)new(464-466)new(477-483)stream(276-278)mode(280-282)stdout_mode(381-383)stdout_mode(417-419)path(199-201)config(421-423)src/stdlib/mod.rs (1)
workspace_root_path(144-146)
🔍 Remote MCP Deepwiki
Relevant repository-level facts to keep in mind when reviewing this refactor (concise):
-
Template/stdlib integration: filters/functions are registered on a minijinja Environment configured with UndefinedBehavior::Strict; stdlib provides env(), glob(), many path/file helpers used by templates and filters — ensure new command.register wiring follows this pattern and marks filters impure as intended.,
-
Filesystem & tempfile policy: codebase uses cap-std for capability-based I/O and camino Utf8PathBuf for UTF‑8 paths. Temporary files are expected to be managed via tempfile + RAII and persisted as Utf8PathBuf when returning tempfile paths — verify CommandConfig.create_tempfile, persist_tempfile, and workspace_root handling match these constraints.
-
Security & quoting: project mandates shell-quote (POSIX/sh) escaping and shlex validation for commands; Windows has special quoting rules. Review quote.rs, cmd interpolation, and any shlex checks in execution/ninja generation for parity with existing policy.
-
Error handling conventions: library/domain errors use thiserror enums, application layers add anyhow::Context, and diagnostics aim toward miette-style rich messages. Ensure CommandFailure → command_error translation and contextual .with_context() usage align with repository patterns.
-
IR/ninja expectations and determinism: generated command strings and temp-file paths must be deterministic/sorted where needed (used by insta snapshots); action/edge ordering and UTF‑8 path normalization are important for tests. Confirm new modules preserve deterministic ordering and stable IDs.
-
Test & CI expectations: repo enforces cargo fmt, strict clippy (deny warnings), extensive unit/BDD/snapshot tests (rstest, cucumber, insta). New tests in stdlib/command must follow existing fixture/style and be included in cargo test; snapshots must be deterministic. CI matrix includes MSRV 1.89.0 — ensure no MSRV regressions.
-
Public API surface: main public crate-facing items are CommandConfig and register(env,..). Confirm visibility (pub(crate)/pub(super)) choices do not unintentionally expose or hide required APIs for existing callers.
Tools used:
- Deepwiki_read_wiki_structure (repo index)
- Deepwiki_read_wiki_contents (wiki pages referenced above)
⏰ 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, macos-aarch64) / Build macos artefacts
- GitHub Check: release / build-macos (x86_64-apple-darwin, macos-13, macos-x86_64, macos-x86_64) / Build macos artefacts
- GitHub Check: release / build-linux (aarch64-unknown-linux-gnu, linux-arm64, arm64) / Build linux artefacts
- GitHub Check: release / build-linux (x86_64-unknown-linux-gnu, linux-amd64, amd64) / Build linux artefacts
- GitHub Check: release / build-windows (aarch64-pc-windows-msvc, arm64, arm64, windows-aarch64) / Build windows artefacts
- GitHub Check: release / build-windows (x86_64-pc-windows-msvc, amd64, x64, windows-x86_64) / Build windows artefacts
- GitHub Check: Sourcery review
- GitHub Check: build-test (ubuntu-latest, stable)
…flow checks - Extract helper functions `read_size_to_u64` and `add_checked` in `PipeLimit` to panic on impossible overflows instead of returning errors. - Enhance `CommandFailure` enum with more detailed documentation comments. - Simplify `From<io::Error>` impl for `CommandFailure` with a single-line definition. - Improve error message construction in `command_error` by using panic on write failures instead of debug asserts. - Overall code cleanup and improved clarity in error handling and overflow check logic. Co-authored-by: terragon-labs[bot] <terragon-labs[bot]@users.noreply.github.com>
leynos
changed the title
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 (2)
src/stdlib/command/config.rs(1 hunks)src/stdlib/command/error.rs(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
**/*.rs: Every Rust module must begin with a module-level comment using //! explaining the module's purpose and utility
Document public APIs with Rustdoc comments (///) so documentation can be generated with cargo doc
Prefer immutable data; avoid unnecessary mut bindings
Handle errors with Result instead of panicking where feasible
Prefer semantic error enums deriving std::error::Error via thiserror for inspectable conditions
Clippy warnings must be denied; do not silence lints except as a last resort
Lint suppressions must be tightly scoped and include a clear reason; prefer #[expect(...)] over #[allow(...)]
Place function attributes after doc comments
Do not use return in single-line functions
Prefer single-line function definitions where appropriate (e.g., pub fn new(id: u64) -> Self { Self(id) })
Use predicate functions for conditional criteria with more than two branches
Where a function is too long, extract meaningfully named helpers adhering to separation of concerns and CQRS
Where a function has too many parameters, group related parameters into meaningfully named structs
When returning large error data, consider using Arc to reduce data moved
Prefer .expect() over .unwrap() on Results/Options
Use concat!() to combine long string literals instead of escaping newlines with backslashes
Use NewTypes to model domain values; prefer explicit tuple structs; leverage newt-hype and the-newtype crates as described; provide appropriate From/AsRef/TryFrom implementations and avoid orphan-rule-violating impls
Keep each Rust code file under 400 lines; split long switches/dispatch tables by feature; move large test data to external files
Use functions and composition to avoid repetition; extract reusable logic; prefer declarative constructs (iterators/generators/comprehensions) when readable
Small, single-responsibility functions; obey command/query segregation
Name things precisely; booleans should use is/has/should prefixes
Comment why, not what; explain assumptions, e...
Files:
src/stdlib/command/config.rssrc/stdlib/command/error.rs
⚙️ CodeRabbit configuration file
**/*.rs: * Seek to keep the cognitive complexity of functions no more than 9.
- Adhere to single responsibility and CQRS
- Place function attributes after doc comments.
- Do not use
returnin single-line functions.- Move conditionals with >2 branches into a predicate function.
- Avoid
unsafeunless absolutely necessary.- Every module must begin with a
//!doc comment that explains the module's purpose and utility.- Comments and docs must follow en-GB-oxendict (-ize / -yse / -our) spelling and grammar
- Lints must not be silenced except as a last resort.
#[allow]is forbidden.- Only narrowly scoped
#[expect(lint, reason = "...")]is allowed.- No lint groups, no blanket or file-wide suppression.
- Include
FIXME:with link if a fix is expected.- Where code is only used by specific features, it must be conditionally compiled or a conditional expectation for unused_code applied.
- Use
rstestfixtures for shared setup and to avoid repetition between tests.- Replace duplicated tests with
#[rstest(...)]parameterised cases.- Prefer
mockallfor mocks/stubs.- Prefer
.expect()over.unwrap()in tests..expect()and.unwrap()are forbidden outside of tests. Errors must be propagated.- Ensure that any API or behavioural changes are reflected in the documentation in
docs/- Ensure that any completed roadmap steps are recorded in the appropriate roadmap in
docs/- Files must not exceed 400 lines in length
- Large modules must be decomposed
- Long match statements or dispatch tables should be decomposed by domain and collocated with targets
- Large blocks of inline data (e.g., test fixtures, constants or templates) must be moved to external files and inlined at compile-time or loaded at run-time.
- Environment access (env::set_var and env::remove_var) are always unsafe in Rust 2024 and MUST be marked as such
- For testing of functionality depending upon environment variables, dependency injection and...
Files:
src/stdlib/command/config.rssrc/stdlib/command/error.rs
**/*.{rs,md}
📄 CodeRabbit inference engine (AGENTS.md)
Use en-GB-oxendict spelling and grammar in comments and documentation (exceptions allowed for external API names)
Files:
src/stdlib/command/config.rssrc/stdlib/command/error.rs
🧬 Code graph analysis (2)
src/stdlib/command/config.rs (2)
src/stdlib/command/error.rs (2)
new(171-173)new(184-190)src/stdlib/command/context.rs (5)
new(16-18)new(39-41)new(51-53)stdout_mode(20-22)config(24-26)
src/stdlib/command/error.rs (2)
src/stdlib/command/context.rs (5)
config(24-26)new(16-18)new(39-41)new(51-53)describe(55-57)src/stdlib/command/config.rs (5)
new(37-50)new(162-168)stream(171-173)describe(107-112)describe(126-131)
⏰ 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, macos-aarch64) / Build macos artefacts
- GitHub Check: release / build-windows (aarch64-pc-windows-msvc, arm64, arm64, windows-aarch64) / Build windows artefacts
- GitHub Check: release / build-macos (x86_64-apple-darwin, macos-13, macos-x86_64, macos-x86_64) / Build macos artefacts
- GitHub Check: release / build-linux (aarch64-unknown-linux-gnu, linux-arm64, arm64) / Build linux artefacts
- GitHub Check: release / build-windows (x86_64-pc-windows-msvc, amd64, x64, windows-x86_64) / Build windows artefacts
- GitHub Check: release / build-linux (x86_64-unknown-linux-gnu, linux-amd64, amd64) / Build linux artefacts
- GitHub Check: build-test (ubuntu-latest, stable)
- GitHub Check: Sourcery review
Replaced usages of unwrap_or_else for panics with expect for simpler and clearer error handling messages in command module's config.rs and error.rs files. Co-authored-by: terragon-labs[bot] <terragon-labs[bot]@users.noreply.github.com>
leynos
changed the title
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
src/stdlib/command/config.rs(1 hunks)src/stdlib/command/error.rs(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
**/*.rs: Every Rust module must begin with a module-level comment using //! explaining the module's purpose and utility
Document public APIs with Rustdoc comments (///) so documentation can be generated with cargo doc
Prefer immutable data; avoid unnecessary mut bindings
Handle errors with Result instead of panicking where feasible
Prefer semantic error enums deriving std::error::Error via thiserror for inspectable conditions
Clippy warnings must be denied; do not silence lints except as a last resort
Lint suppressions must be tightly scoped and include a clear reason; prefer #[expect(...)] over #[allow(...)]
Place function attributes after doc comments
Do not use return in single-line functions
Prefer single-line function definitions where appropriate (e.g., pub fn new(id: u64) -> Self { Self(id) })
Use predicate functions for conditional criteria with more than two branches
Where a function is too long, extract meaningfully named helpers adhering to separation of concerns and CQRS
Where a function has too many parameters, group related parameters into meaningfully named structs
When returning large error data, consider using Arc to reduce data moved
Prefer .expect() over .unwrap() on Results/Options
Use concat!() to combine long string literals instead of escaping newlines with backslashes
Use NewTypes to model domain values; prefer explicit tuple structs; leverage newt-hype and the-newtype crates as described; provide appropriate From/AsRef/TryFrom implementations and avoid orphan-rule-violating impls
Keep each Rust code file under 400 lines; split long switches/dispatch tables by feature; move large test data to external files
Use functions and composition to avoid repetition; extract reusable logic; prefer declarative constructs (iterators/generators/comprehensions) when readable
Small, single-responsibility functions; obey command/query segregation
Name things precisely; booleans should use is/has/should prefixes
Comment why, not what; explain assumptions, e...
Files:
src/stdlib/command/error.rssrc/stdlib/command/config.rs
⚙️ CodeRabbit configuration file
**/*.rs: * Seek to keep the cognitive complexity of functions no more than 9.
- Adhere to single responsibility and CQRS
- Place function attributes after doc comments.
- Do not use
returnin single-line functions.- Move conditionals with >2 branches into a predicate function.
- Avoid
unsafeunless absolutely necessary.- Every module must begin with a
//!doc comment that explains the module's purpose and utility.- Comments and docs must follow en-GB-oxendict (-ize / -yse / -our) spelling and grammar
- Lints must not be silenced except as a last resort.
#[allow]is forbidden.- Only narrowly scoped
#[expect(lint, reason = "...")]is allowed.- No lint groups, no blanket or file-wide suppression.
- Include
FIXME:with link if a fix is expected.- Where code is only used by specific features, it must be conditionally compiled or a conditional expectation for unused_code applied.
- Use
rstestfixtures for shared setup and to avoid repetition between tests.- Replace duplicated tests with
#[rstest(...)]parameterised cases.- Prefer
mockallfor mocks/stubs.- Prefer
.expect()over.unwrap()in tests..expect()and.unwrap()are forbidden outside of tests. Errors must be propagated.- Ensure that any API or behavioural changes are reflected in the documentation in
docs/- Ensure that any completed roadmap steps are recorded in the appropriate roadmap in
docs/- Files must not exceed 400 lines in length
- Large modules must be decomposed
- Long match statements or dispatch tables should be decomposed by domain and collocated with targets
- Large blocks of inline data (e.g., test fixtures, constants or templates) must be moved to external files and inlined at compile-time or loaded at run-time.
- Environment access (env::set_var and env::remove_var) are always unsafe in Rust 2024 and MUST be marked as such
- For testing of functionality depending upon environment variables, dependency injection and...
Files:
src/stdlib/command/error.rssrc/stdlib/command/config.rs
**/*.{rs,md}
📄 CodeRabbit inference engine (AGENTS.md)
Use en-GB-oxendict spelling and grammar in comments and documentation (exceptions allowed for external API names)
Files:
src/stdlib/command/error.rssrc/stdlib/command/config.rs
🧬 Code graph analysis (2)
src/stdlib/command/error.rs (2)
src/stdlib/command/context.rs (5)
config(24-26)new(16-18)new(39-41)new(51-53)describe(55-57)src/stdlib/command/config.rs (5)
new(37-50)new(162-168)stream(171-173)describe(107-112)describe(126-131)
src/stdlib/command/config.rs (2)
src/stdlib/command/context.rs (5)
new(16-18)new(39-41)new(51-53)stdout_mode(20-22)config(24-26)src/stdlib/mod.rs (1)
workspace_root_path(144-146)
🪛 GitHub Actions: CI
src/stdlib/command/config.rs
[error] 221-221: Clippy error: used expect() on a Result value. (cargo clippy: "used expect() on a Result value" at src/stdlib/command/config.rs:221)
🪛 GitHub Check: build-test (ubuntu-latest, 1.89.0)
src/stdlib/command/error.rs
[failure] 148-148:
used expect() on a Result value
src/stdlib/command/config.rs
[failure] 225-225:
used expect() on an Option value
[failure] 224-224:
this could be a const fn
[failure] 221-221:
used expect() on a Result value
🪛 GitHub Check: build-test (ubuntu-latest, stable)
src/stdlib/command/error.rs
[failure] 148-148:
used expect() on a Result value
src/stdlib/command/config.rs
[failure] 225-225:
used expect() on an Option value
[failure] 224-224:
this could be a const fn
[failure] 221-221:
used expect() on a Result value
🔍 Remote MCP Deepwiki
Summary — key repository facts to check when reviewing this PR
-
Stdlib registration pattern: modules expose a register function (add filters to a minijinja::Environment) and a top-level stdlib register wires them into the Environment — ensure the new command::register matches this pattern.
-
Template evaluation: minijinja is configured strict (undefined vars fail) and the codebase uses a two‑phase approach (structural expansion — foreach/when — then string rendering). New command filters must respect that lifecycle and error semantics.
-
Filesystem / tempfile conventions: code uses cap‑std::fs::Dir and camino::Utf8PathBuf for sandboxed UTF‑8 paths; temporary files are created with tempfile::NamedTempFile and managed via RAII (runner keeps a guard to persist/remove). CommandConfig.create_tempfile should follow these conventions (workspace Dir, Utf8PathBuf, NamedTempFile semantics).
-
Shell/quoting security: existing pipeline uses shell‑quote (POSIX sh mode) and validates with shlex; quoting/validation behavior in quote.rs and cmd interpolation must match this for cross-module consistency.
-
Error mapping conventions: stdlib filters produce minijinja::Error (ErrorKind::InvalidOperation / SyntaxError); the new command_error translation should follow the same ErrorKind choices and include template/command context as other stdlib errors do.
-
Tests & CI expectations: repo uses rstest, cucumber (BDD) and insta snapshots; CI treats snapshot/regression strictly — new tests (unit/BDD/snapshots) should use existing fixtures (test_support, CliWorld) and ensure deterministic output (sorted paths, stable hashes).
-
Platform differences: quoting and execution have Windows-specific behavior (separate implementations/tests). Ensure Windows branches are conditional (cfg(windows)) and tests are gated appropriately.
-
Impurity/caching: existing shell-like filters mark templates as impure to disable caching — the new register path already sets an impure flag; confirm it matches the repo pattern so template caching/invalidations remain correct.
Tools/sources queried:,,
⏰ Context from checks skipped due to timeout of 120000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: release / build-windows (x86_64-pc-windows-msvc, amd64, x64, windows-x86_64) / Build windows artefacts
- GitHub Check: release / build-windows (aarch64-pc-windows-msvc, arm64, arm64, windows-aarch64) / Build windows artefacts
- GitHub Check: release / build-macos (x86_64-apple-darwin, macos-13, macos-x86_64, macos-x86_64) / Build macos artefacts
- GitHub Check: release / build-linux (aarch64-unknown-linux-gnu, linux-arm64, arm64) / Build linux artefacts
- GitHub Check: release / build-linux (x86_64-unknown-linux-gnu, linux-amd64, amd64) / Build linux artefacts
|
@coderabbitai pause |
✅ Actions performedReviews paused. |
…or pipe limits - Enhanced panic messages to include error details in read_size_to_u64 - Converted add_checked to const fn and replaced expect with match for clarity - Changed write! assertion to use assert! macro in append_exit_status for consistency These changes improve code clarity, reliability, and const correctness in pipe limit handling and error formatting. Co-authored-by: terragon-labs[bot] <terragon-labs[bot]@users.noreply.github.com>
…d add clippy annotations Replaced unwrap_or_else with expect for usize to u64 conversion and u64 addition. Added #[expect(clippy::expect_used)] annotations with reasons to clarify safety assumptions. This improves code clarity while maintaining panic behavior on unexpected overflows. Co-authored-by: terragon-labs[bot] <terragon-labs[bot]@users.noreply.github.com>
1 participant
Summary
Changes
New modules
-- src/stdlib/command/mod_backup.rs: legacy content of the previous command module for compatibility.
+- src/stdlib/command/mod_backup.rs: legacy content of the previous command module for compatibility.
Refactor and behavior
Rationale
Compatibility
How to test
🌿 Generated by Terry
ℹ️ Tag @terragon-labs to ask questions and address PR feedback
📎 Task: https://www.terragonlabs.com/task/3b2d6a9b-4ccf-4c1e-b440-811791f60122
Summary by Sourcery
Refactor the command module by splitting it into focused submodules and introduce a new registration API for MiniJinja, while enforcing per-filter output limits with unified error handling and proper tempfile management.
New Features:
Tests: