Skip to content

Enforce command output limits and add streaming support#231

Merged
leynos merged 4 commits intomainfrom
codex/enforce-command-output-size-limits
Nov 8, 2025
Merged

Enforce command output limits and add streaming support#231
leynos merged 4 commits intomainfrom
codex/enforce-command-output-size-limits

Conversation

@leynos
Copy link
Owner

@leynos leynos commented Nov 4, 2025

Summary

  • introduce bounded pipe readers for shell and grep helpers, supporting capture limits and tempfile streaming
  • extend stdlib configuration and test helpers to configure command stream limits, updating cucumber world/steps accordingly
  • document the streaming design, note the completed audit remediation, and cover the new behaviour in unit and behavioural tests

Testing

  • make check-fmt
  • make lint
  • make test

https://chatgpt.com/codex/tasks/task_e_690a482e97d48322a291415de136b283

Summary by Sourcery

Enforce configurable output limits for shell and grep filters and introduce tempfile-based streaming support

New Features:

  • Allow configuring maximum captured and streamed command output sizes via StdlibConfig
  • Enable shell and grep filters to stream large outputs to temporary files using a 'mode' option

Enhancements:

  • Enforce byte limits incrementally on pipe readers to abort over-budget output early
  • Refactor command registration to pass a dedicated CommandConfig through the execution context
  • Centralize command error formatting with specialized helper functions and detailed diagnostics

Documentation:

  • Document configurable command capture and streaming limits in the user guide, security audit, and design docs

Tests:

  • Add unit and behavior tests covering output limits and streaming for shell and grep filters
  • Extend Cucumber steps to configure command output/stream limits and validate temporary file results

@sourcery-ai
Copy link
Contributor

sourcery-ai bot commented Nov 4, 2025

Reviewer's Guide

This PR introduces configurable output limits and optional streaming for the shell and grep helpers by extending the command execution pipeline with limit-aware pipe readers and tempfile support, refactoring the configuration API, updating filter registration to accept per-call options, expanding tests for limit enforcement and streaming, and updating documentation and audit notes accordingly.

Sequence diagram for shell/grep filter execution with output limit and streaming support

sequenceDiagram
    participant Template
    participant "MiniJinja Environment" as Env
    participant "shell/grep filter"
    participant "CommandContext"
    participant "PipeReader"
    participant "Tempfile (optional)"
    Template->>Env: Call shell/grep filter with options
    Env->>"shell/grep filter": Pass input, command, options
    "shell/grep filter"->>CommandContext: Parse options, set output mode/limits
    "shell/grep filter"->>PipeReader: Spawn pipe reader with PipeSpec
    alt OutputMode: Capture
        PipeReader->>"shell/grep filter": Return bytes (up to max_capture_bytes)
        "shell/grep filter"->>Env: Return captured bytes
    else OutputMode: Tempfile
        PipeReader->>Tempfile: Stream output to temp file (up to max_stream_bytes)
        PipeReader->>"shell/grep filter": Return temp file path
        "shell/grep filter"->>Env: Return temp file path
    end
Loading

ER diagram for updated StdlibConfig and CommandConfig relationship

erDiagram
    STDLIBCONFIG ||--o| COMMANDCONFIG : exposes
    STDLIBCONFIG {
        string fetch_cache_relative
        string network_policy
        integer fetch_max_response_bytes
        integer command_max_output_bytes
        integer command_max_stream_bytes
    }
    COMMANDCONFIG {
        integer max_capture_bytes
        integer max_stream_bytes
    }
Loading

Class diagram for new and updated command output limit and streaming types

classDiagram
    class StdlibConfig {
        +fetch_cache_relative: Utf8PathBuf
        +network_policy: NetworkPolicy
        +fetch_max_response_bytes: u64
        +command_max_output_bytes: u64
        +command_max_stream_bytes: u64
        +with_command_max_output_bytes(max_bytes: u64): Result<StdlibConfig>
        +with_command_max_stream_bytes(max_bytes: u64): Result<StdlibConfig>
        +into_components(): (NetworkConfig, CommandConfig)
    }
    class CommandConfig {
        +max_capture_bytes: u64
        +max_stream_bytes: u64
    }
    class CommandOptions {
        +stdout_mode: OutputMode
        +from_value(options: Option<Value>): Result<CommandOptions, Error>
        +from_mode_str(mode: &str): Result<CommandOptions, Error>
        +stdout_mode(): OutputMode
    }
    class OutputMode {
        <<enum>>
        Capture
        Tempfile
        +describe(): &str
    }
    class OutputStream {
        <<enum>>
        Stdout
        Stderr
        +describe(): &str
    }
    class PipeSpec {
        +stream: OutputStream
        +mode: OutputMode
        +limit: u64
        +new(stream, mode, limit): PipeSpec
        +into_limit(): PipeLimit
    }
    class PipeLimit {
        +spec: PipeSpec
        +consumed: u64
        +record(read: usize): Result<(), CommandFailure>
    }
    class CommandContext {
        +config: CommandConfig
        +options: CommandOptions
        +new(config, options): CommandContext
        +stdout_mode(): OutputMode
        +config(): &CommandConfig
    }
    class CommandFailure {
        <<enum>>
        Spawn(io::Error)
        Io(io::Error)
        BrokenPipe
        Exit
        OutputLimit
        StreamPathNotUtf8
        Timeout
    }
    StdlibConfig --> CommandConfig
    CommandContext --> CommandConfig
    CommandContext --> CommandOptions
    PipeLimit --> PipeSpec
    PipeSpec --> OutputStream
    PipeSpec --> OutputMode
    CommandFailure --> OutputStream
    CommandFailure --> OutputMode
Loading

File-Level Changes

Change Details Files
Enforce capture and stream byte limits in command execution
  • Introduce CommandConfig, OutputMode, OutputStream, PipeSpec, and PipeLimit types
  • Refactor spawn_pipe_reader/join_reader/read_pipe to track and enforce per-pipe budgets
  • Update run_command/run_program/run_child to use CommandContext and return StdoutResult variants
  • Abort readers early when limits are exceeded with OutputLimit errors
src/stdlib/command.rs
Extend StdlibConfig with command output and streaming settings
  • Add command_max_output_bytes and command_max_stream_bytes fields and defaults
  • Implement with_command_max_output_bytes and with_command_max_stream_bytes builders
  • Refactor into_components to emit CommandConfig alongside NetworkConfig
  • Update register to pass CommandConfig into command::register
src/stdlib/mod.rs
Enhance shell and grep filters to accept output mode options
  • Update register signature to accept an options argument
  • Parse CommandOptions from arbitrary Value (string or map)
  • Pass CommandContext into execute_shell and execute_grep
  • Return file paths for tempfile mode and bytes otherwise
src/stdlib/command.rs
Add tests and helpers for output limits and streaming
  • Implement compile_large_output_helper in test_support
  • Add Cucumber steps and scenarios for command output and stream limits
  • Write unit tests for shell/grep limit enforcement and tempfile streaming
  • Extend step definitions to validate streamed file contents and size
tests/std_filter_tests/command_filters.rs
tests/steps/stdlib_steps/assertions.rs
tests/steps/stdlib_steps/config.rs
tests/steps/stdlib_steps/workspace.rs
tests/features/stdlib.feature
test_support/src/command_helper.rs
Update documentation and audit notes for new behavior
  • Document command output limits and streaming options in users-guide.md
  • Add design details on PipeSpec/PipeLimit and StdlibConfig changes in netsuke-design.md
  • Mark the output buffer audit item as done and describe remediation in security-network-command-audit.md
docs/users-guide.md
docs/netsuke-design.md
docs/security-network-command-audit.md

Tips and commands

Interacting with Sourcery

  • Trigger a new review: Comment @sourcery-ai review on the pull request.
  • Continue discussions: Reply directly to Sourcery's review comments.
  • Generate a GitHub issue from a review comment: Ask Sourcery to create an
    issue from a review comment by replying to it. You can also reply to a
    review comment with @sourcery-ai issue to create an issue from it.
  • Generate a pull request title: Write @sourcery-ai anywhere in the pull
    request title to generate a title at any time. You can also comment
    @sourcery-ai title on the pull request to (re-)generate the title at any time.
  • Generate a pull request summary: Write @sourcery-ai summary anywhere in
    the pull request body to generate a PR summary at any time exactly where you
    want it. You can also comment @sourcery-ai summary on the pull request to
    (re-)generate the summary at any time.
  • Generate reviewer's guide: Comment @sourcery-ai guide on the pull
    request to (re-)generate the reviewer's guide at any time.
  • Resolve all Sourcery comments: Comment @sourcery-ai resolve on the
    pull request to resolve all Sourcery comments. Useful if you've already
    addressed all the comments and don't want to see them anymore.
  • Dismiss all Sourcery reviews: Comment @sourcery-ai dismiss on the pull
    request to dismiss all existing Sourcery reviews. Especially useful if you
    want to start fresh with a new review - don't forget to comment
    @sourcery-ai review to trigger a new review!

Customizing Your Experience

Access your dashboard to:

  • Enable or disable review features such as the Sourcery-generated pull request
    summary, the reviewer's guide, and others.
  • Change the review language.
  • Add, remove or edit custom review instructions.
  • Adjust other review settings.

Getting Help

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Nov 4, 2025

Note

Reviews paused

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Note

Other AI code review bot(s) detected

CodeRabbit 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

Release Notes

  • New Features

    • Configurable output limits for shell and grep commands with separate capture and streaming budgets
    • Streaming mode to write large command outputs to temporary files instead of buffering in memory
    • Enhanced error messages that display configured output budgets when limits are exceeded
  • Documentation

    • Updated design documentation and user guide with output limit and streaming configuration details

Walkthrough

Introduce configurable stdout capture and streaming limits for shell/grep execution (defaults: 1 MiB capture, 64 MiB stream), add tempfile streaming mode, enforce per-pipe budgets during drain with explicit diagnostics, propagate command configuration through StdlibConfig → CommandConfig → CommandContext, and update tests and docs accordingly.

Changes

Cohort / File(s) Summary
Documentation
docs/netsuke-design.md, docs/security-network-command-audit.md, docs/users-guide.md
Add design notes and class diagram for pipe-budgeting and tempfile streaming; mark remediation done and fix user-guide formatting and examples.
Stdlib configuration
src/stdlib/mod.rs, src/manifest/mod.rs
Add defaults and builders: with_workspace_root_path, with_command_max_output_bytes, with_command_max_stream_bytes; introduce into_components() returning (NetworkConfig, command::CommandConfig) and propagate workspace root into components.
Command execution
src/stdlib/command.rs
Implement command I/O-limiting framework: CommandConfig, CommandContext, CommandOptions, OutputMode/Stream, CommandTempDir/File, PipeSpec/PipeLimit, PipeOutcome/StdoutResult, richer CommandFailure (including OutputLimit, StreamPathNotUtf8), spawn/read-pipe infrastructure (capture vs tempfile), and updated shell/grep filter wiring to accept per-call options and context.
Test helpers
test_support/src/command_helper.rs
Add LARGE_OUTPUT_SOURCE and compile_large_output_helper() to build large-output test binaries.
Cucumber state
tests/cucumber.rs
Add stdlib_command_max_output_bytes: Option<u64>, stdlib_command_stream_max_bytes: Option<u64>, and stdlib_text: Option<String> to CliWorld; clear stdlib_text on drop.
Feature scenarios
tests/features/stdlib.feature
Add scenarios covering capture limits, tempfile streaming, and stream-limit enforcement for shell/grep filters.
Unit/integration tests
tests/std_filter_tests/command_filters.rs
Add extensive tests for capture limits, streaming to tempfile, stream-budget enforcement, Windows-specific stubs and harnesses, and fixture helpers that accept StdlibConfig.
Cucumber step assertions
tests/steps/stdlib_steps/assertions.rs
Add file-based assertions: assert_stdlib_output_file_min_size(), assert_stdlib_output_file_uniform(), assert_stdlib_output_equals_text().
Cucumber configuration steps
tests/steps/stdlib_steps/config.rs
Add steps: configure_command_output_limit(), configure_command_stream_limit(), configure_stdlib_text().
Cucumber rendering steps
tests/steps/stdlib_steps/rendering.rs
Use with_workspace_root_path when building StdlibConfig; bind optional command limits; add render_stdlib_template_with_text().
Cucumber workspace steps
tests/steps/stdlib_steps/workspace.rs
Add large_output_stdlib_command_helper() to compile and register a large-output helper.
Test env helpers
tests/std_filter_tests/network_functions.rs
Update env_with_workspace_policy() to accept and forward workspace_path to StdlibConfig.

Sequence Diagram(s)

sequenceDiagram
    participant Template as Template Engine
    participant Filter as Shell/Grep Filter
    participant Context as CommandContext
    participant Reader as Pipe Reader Thread
    participant Storage as Memory/Tempfile
    participant Caller as Template Caller

    Template->>Filter: call(command, options?)
    Filter->>Context: build from CommandConfig + per-call options
    Filter->>Reader: spawn_pipe_reader(PipeSpec, PipeLimit)
    Reader->>Reader: read loop (drain bytes, update PipeLimit)
    alt within limit
        Reader->>Storage: write to buffer or tempfile
        Storage->>Filter: return StdoutResult (Bytes or Path)
        Filter->>Caller: return output
    else limit exceeded
        Reader->>Filter: surface OutputLimit diagnostic
        Filter->>Caller: return CommandFailure::OutputLimit
    end
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~60 minutes

  • Inspect src/stdlib/command.rs for correctness of pipe-limit accounting, thread handling, and error propagation.
  • Verify src/stdlib/mod.rs and src/manifest/mod.rs correctly propagate workspace path and limits to components.
  • Review tests in tests/std_filter_tests/command_filters.rs and Windows-specific helpers for robustness and platform differences.
  • Confirm Cucumber steps and helper wiring (tests/*) correctly set and consume new CliWorld fields.

Poem

⚡ Streams kept tidy, bytes counted with care,
Tempfiles catch overflow from a runaway share.
Limits speak plainly when outputs run wide,
Shell and grep now steward the data tide. 🎉

Pre-merge checks and finishing touches

✅ Passed checks (3 passed)
Check name Status Explanation
Title check ✅ Passed The title accurately summarises the main objectives: enforcing command output limits and introducing streaming support for shell and grep filters.
Description check ✅ Passed The description clearly relates to the changeset, covering bounded pipe readers, stdlib configuration extensions, command stream limits, and documentation updates.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

Comment @coderabbitai help to get the list of available commands and usage tips.

codescene-delta-analysis[bot]

This comment was marked as outdated.

Copy link
Contributor

@sourcery-ai sourcery-ai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey there - I've reviewed your changes and found some issues that need to be addressed.

Prompt for AI Agents
Please address the comments from this code review:

## Individual Comments

### Comment 1
<location> `src/stdlib/command.rs:935-938` </location>
<code_context>
-            Ok(buf)
-        })
-    })
+            let mut chunk = [0_u8; 8192];
+            loop {
+                let read = reader.read(&mut chunk).map_err(CommandFailure::Io)?;
+                if read == 0 {
+                    break;
+                }
+                limit.record(read)?;
+                buf.extend(chunk.iter().take(read).copied());
+            }
+            Ok(PipeOutcome::Bytes(buf))
+        }
+        OutputMode::Tempfile => {
+            let mut file = NamedTempFile::new().map_err(CommandFailure::Io)?;
+            let mut chunk = [0_u8; 8192];
+            loop {
+                let read = reader.read(&mut chunk).map_err(CommandFailure::Io)?;
+                if read == 0 {
+                    break;
+                }
+                limit.record(read)?;
+                let slice = chunk.get(..read).ok_or_else(|| {
+                    CommandFailure::Io(io::Error::other("pipe read out of range"))
+                })?;
</code_context>

<issue_to_address>
**suggestion:** Redundant bounds check on chunk slice.

Since Read::read guarantees that read <= chunk.len(), the explicit bounds check with chunk.get(..read) can be removed; using &chunk[..read] is safe here.

```suggestion
                file.write_all(&chunk[..read]).map_err(CommandFailure::Io)?;
```
</issue_to_address>

### Comment 2
<location> `src/stdlib/command.rs:81` </location>
<code_context>
 // while still surfacing timeouts for misbehaving commands.
 const COMMAND_TIMEOUT: Duration = Duration::from_secs(5);

+#[derive(Clone)]
+pub(crate) struct CommandConfig {
+    pub(crate) max_capture_bytes: u64,
</code_context>

<issue_to_address>
**issue (review_instructions):** Add behavioural and unit tests for new command output limiting and streaming features.

The new output limiting and streaming logic (e.g., CommandConfig, OutputMode, PipeSpec, etc.) must be covered by both behavioural and unit tests to verify correct enforcement of limits, error handling, and streaming to tempfiles. Please add tests that exercise these code paths, including cases where limits are exceeded.

<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/mod.rs:40` </location>
<code_context>
 /// Default upper bound for network helper responses (8 MiB).
 pub(crate) const DEFAULT_FETCH_MAX_RESPONSE_BYTES: u64 = 8 * 1024 * 1024;
+/// Default upper bound for captured command output (1 MiB).
+pub(crate) const DEFAULT_COMMAND_MAX_OUTPUT_BYTES: u64 = 1024 * 1024;
+/// Default upper bound for streamed command output files (64 MiB).
+pub(crate) const DEFAULT_COMMAND_MAX_STREAM_BYTES: u64 = 64 * 1024 * 1024;
</code_context>

<issue_to_address>
**issue (review_instructions):** Add tests for new configuration options in StdlibConfig related to command output limits.

The new configuration options (DEFAULT_COMMAND_MAX_OUTPUT_BYTES, DEFAULT_COMMAND_MAX_STREAM_BYTES, and related builder methods) must be covered by unit tests to ensure correct validation and propagation of limits. Please add tests for these.

<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.rs:81` </location>
<code_context>
 // while still surfacing timeouts for misbehaving commands.
 const COMMAND_TIMEOUT: Duration = Duration::from_secs(5);

+#[derive(Clone)]
+pub(crate) struct CommandConfig {
+    pub(crate) max_capture_bytes: u64,
</code_context>

<issue_to_address>
**issue (review_instructions):** The module is missing a `//!` comment at the top, which is required for documentation.

Please add a `//!` comment at the very top of this module to describe its purpose, as per the documentation requirements.

<details>
<summary>Review instructions:</summary>

**Path patterns:** `**/*.rs`

**Instructions:**
Every module **must** begin with a `//!` comment.

</details>
</issue_to_address>

Sourcery is free for open source - if you like our reviews please consider sharing them ✨
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

📜 Review details

Configuration used: CodeRabbit UI

Review profile: ASSERTIVE

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 4fa628f and 6255aba.

📒 Files selected for processing (13)
  • docs/netsuke-design.md (1 hunks)
  • docs/security-network-command-audit.md (1 hunks)
  • docs/users-guide.md (3 hunks)
  • src/stdlib/command.rs (12 hunks)
  • src/stdlib/mod.rs (5 hunks)
  • test_support/src/command_helper.rs (2 hunks)
  • tests/cucumber.rs (2 hunks)
  • tests/features/stdlib.feature (1 hunks)
  • tests/std_filter_tests/command_filters.rs (8 hunks)
  • tests/steps/stdlib_steps/assertions.rs (3 hunks)
  • tests/steps/stdlib_steps/config.rs (1 hunks)
  • tests/steps/stdlib_steps/rendering.rs (2 hunks)
  • tests/steps/stdlib_steps/workspace.rs (2 hunks)
🧰 Additional context used
📓 Path-based instructions (6)
**/*.rs

📄 CodeRabbit inference engine (AGENTS.md)

**/*.rs: Every Rust module must begin with a module-level comment using //! explaining the module's purpose and utility
Document public APIs with Rustdoc comments (///) so documentation can be generated with cargo doc
Prefer immutable data; avoid unnecessary mut bindings
Handle errors with Result instead of panicking where feasible
Prefer semantic error enums deriving std::error::Error via thiserror for inspectable conditions
Clippy warnings must be denied; do not silence lints except as a last resort
Lint suppressions must be tightly scoped and include a clear reason; prefer #[expect(...)] over #[allow(...)]
Place function attributes after doc comments
Do not use return in single-line functions
Prefer single-line function definitions where appropriate (e.g., pub fn new(id: u64) -> Self { Self(id) })
Use predicate functions for conditional criteria with more than two branches
Where a function is too long, extract meaningfully named helpers adhering to separation of concerns and CQRS
Where a function has too many parameters, group related parameters into meaningfully named structs
When returning large error data, consider using Arc to reduce data moved
Prefer .expect() over .unwrap() on Results/Options
Use concat!() to combine long string literals instead of escaping newlines with backslashes
Use NewTypes to model domain values; prefer explicit tuple structs; leverage newt-hype and the-newtype crates as described; provide appropriate From/AsRef/TryFrom implementations and avoid orphan-rule-violating impls
Keep each Rust code file under 400 lines; split long switches/dispatch tables by feature; move large test data to external files
Use functions and composition to avoid repetition; extract reusable logic; prefer declarative constructs (iterators/generators/comprehensions) when readable
Small, single-responsibility functions; obey command/query segregation
Name things precisely; booleans should use is/has/should prefixes
Comment why, not what; explain assumptions, e...

Files:

  • tests/steps/stdlib_steps/rendering.rs
  • tests/cucumber.rs
  • tests/steps/stdlib_steps/config.rs
  • tests/steps/stdlib_steps/workspace.rs
  • tests/steps/stdlib_steps/assertions.rs
  • test_support/src/command_helper.rs
  • src/stdlib/mod.rs
  • tests/std_filter_tests/command_filters.rs
  • src/stdlib/command.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 return in single-line functions.
  • Move conditionals with >2 branches into a predicate function.
  • Avoid unsafe unless 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 rstest fixtures for shared setup and to avoid repetition between tests.
  • Replace duplicated tests with #[rstest(...)] parameterised cases.
  • Prefer mockall for mocks/stubs.
  • Prefer .expect() over .unwrap() in tests.
  • .expect() and .unwrap() are forbidden outside of tests. Errors must be propagated.
  • Ensure that any API or behavioural changes are reflected in the documentation in docs/
  • Ensure that any completed roadmap steps are recorded in the appropriate roadmap in docs/
  • Files must not exceed 400 lines in length
    • Large modules must be decomposed
    • Long match statements or dispatch tables should be decomposed by domain and collocated with targets
    • Large blocks of inline data (e.g., test fixtures, constants or templates) must be moved to external files and inlined at compile-time or loaded at run-time.
  • Environment access (env::set_var and env::remove_var) are always unsafe in Rust 2024 and MUST be marked as such
    • For testing of functionality depending upon environment variables, dependency injection and...

Files:

  • tests/steps/stdlib_steps/rendering.rs
  • tests/cucumber.rs
  • tests/steps/stdlib_steps/config.rs
  • tests/steps/stdlib_steps/workspace.rs
  • tests/steps/stdlib_steps/assertions.rs
  • test_support/src/command_helper.rs
  • src/stdlib/mod.rs
  • tests/std_filter_tests/command_filters.rs
  • src/stdlib/command.rs
**/*.{rs,md}

📄 CodeRabbit inference engine (AGENTS.md)

Use en-GB-oxendict spelling and grammar in comments and documentation (exceptions allowed for external API names)

Files:

  • tests/steps/stdlib_steps/rendering.rs
  • docs/users-guide.md
  • docs/security-network-command-audit.md
  • tests/cucumber.rs
  • tests/steps/stdlib_steps/config.rs
  • tests/steps/stdlib_steps/workspace.rs
  • tests/steps/stdlib_steps/assertions.rs
  • docs/netsuke-design.md
  • test_support/src/command_helper.rs
  • src/stdlib/mod.rs
  • tests/std_filter_tests/command_filters.rs
  • src/stdlib/command.rs
**/tests/**/*.rs

📄 CodeRabbit inference engine (AGENTS.md)

**/tests/**/*.rs: Use rstest fixtures for shared setup
Replace duplicated tests with #[rstest(...)] parameterised cases
Prefer mockall for mocks and stubs

Files:

  • tests/steps/stdlib_steps/rendering.rs
  • tests/cucumber.rs
  • tests/steps/stdlib_steps/config.rs
  • tests/steps/stdlib_steps/workspace.rs
  • tests/steps/stdlib_steps/assertions.rs
  • tests/std_filter_tests/command_filters.rs
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/users-guide.md
  • docs/security-network-command-audit.md
  • docs/netsuke-design.md
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; use plaintext for 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, use ![alt text](path) and provide brief, descriptive alt text
Add a short descriptive sentence before each Mermaid diagram for screen readers

Files:

  • docs/users-guide.md
  • docs/security-network-command-audit.md
  • docs/netsuke-design.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/users-guide.md
  • docs/security-network-command-audit.md
  • 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/users-guide.md
  • docs/security-network-command-audit.md
  • docs/netsuke-design.md
🧬 Code graph analysis (4)
tests/steps/stdlib_steps/workspace.rs (1)
test_support/src/command_helper.rs (1)
  • compile_large_output_helper (86-92)
src/stdlib/mod.rs (2)
src/stdlib/command.rs (2)
  • config (276-278)
  • register (348-382)
src/stdlib/network/mod.rs (1)
  • register_functions (47-56)
tests/std_filter_tests/command_filters.rs (3)
test_support/src/command_helper.rs (4)
  • compile_failure_helper (81-83)
  • compile_large_output_helper (86-92)
  • compile_uppercase_helper (59-61)
  • compile_rust_helper (120-143)
tests/std_filter_tests/support.rs (1)
  • stdlib_env_with_config (38-44)
src/stdlib/mod.rs (2)
  • new (92-106)
  • default (248-252)
src/stdlib/command.rs (1)
src/stdlib/mod.rs (3)
  • new (92-106)
  • default (248-252)
  • value_from_bytes (346-351)
🪛 LanguageTool
docs/users-guide.md

[uncategorized] ~421-~421: Loose punctuation mark.
Context: ...4 MiB). - grep(pattern, flags=None): Filters input lines matching pattern....

(UNLIKELY_OPENING_PUNCTUATION)

docs/security-network-command-audit.md

[uncategorized] ~68-~68: Possible missing comma found.
Context: ...an unbounded stream will lead to memory exhaustion or at least prolonged blocking. *Reme...

(AI_HYDRA_LEO_MISSING_COMMA)


[uncategorized] ~79-~79: Possible missing comma found.
Context: ... Pipe readers enforce their budgets incrementally so long-running commands fail fast once...

(AI_HYDRA_LEO_MISSING_COMMA)

🔍 Remote MCP Deepwiki

Relevant facts to help review this PR

  • shell/grep purpose: the stdlib shell filter pipes a value to an arbitrary shell command and marks the template “impure” (disables caching); grep is implemented as a convenience wrapper around shell.

  • Existing command-safety controls (must be preserved): Netsuke already automatically shell-escapes interpolated arguments using the shell-quote crate (POSIX/sh mode) and validates command parseability with shlex; minijinja is configured strict (undefined variables fail). The PR’s changes to quoting/streaming/limits must keep these invariants.

  • Filesystem/path rules (relevant to tempfile streaming): filesystem I/O is capability-scoped via cap‑std and paths are handled as UTF‑8 via camino (Utf8PathBuf). Tempfile streaming should preserve capability-based access and UTF‑8 path semantics.

  • Tests & CI expectations: the repo uses unit/snapshot tests and BDD (cucumber) feature tests and enforces formatting/lint/tests via Makefile targets (make check-fmt, make lint, make test). New tests in the PR should integrate with that harness and CI.

  • Rationale for output limits: prior security/remediation work called out unbounded buffering of helper stdout/stderr; enforcing per-pipe budgets and a tempfile streaming mode prevents excessive memory/disk use and enables fast failure when outputs exceed configured budgets.

Sources/tools used:,,

⏰ 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, macos-aarch64) / Build macos artefacts
  • GitHub Check: release / build-linux (aarch64-unknown-linux-gnu, linux-arm64, arm64) / Build linux artefacts
  • GitHub Check: release / build-windows (aarch64-pc-windows-msvc, arm64, arm64, windows-aarch64) / Build windows artefacts
  • GitHub Check: release / build-linux (x86_64-unknown-linux-gnu, linux-amd64, amd64) / Build linux 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)
🔇 Additional comments (5)
docs/users-guide.md (1)

409-419: Document the command limits accurately.
Keep this addition; it records the new defaults and tempfile mode in the user guide with the required precision.

src/stdlib/mod.rs (1)

161-217: Validate command limits on construction.
Retain these builders and the into_components split; they enforce positive budgets and hand the command and network modules the exact configuration they expect.

test_support/src/command_helper.rs (1)

29-92: Supply high-volume helper for streaming scenarios.
Keep this helper; it reuses the existing compiler path and emits a deterministic payload large enough to drive the new tests.

tests/steps/stdlib_steps/workspace.rs (1)

10-86: Expose large-output helper step.
Keep this step; it mirrors the existing helpers and wires the compiled binary into CliWorld so scenarios can trigger streaming behaviour.

tests/features/stdlib.feature (1)

87-110: Exercise streaming behaviour end-to-end.
Keep these scenarios; they verify failure on capture limits and success for tempfile streaming across both shell and grep filters.

@leynos
Copy link
Owner Author

leynos commented Nov 6, 2025

@coderabbitai Please suggest a fix for this issue and supply a prompt for an AI coding agent to enable it to apply the fix:

src/stdlib/command.rs

Comment on file

use std::{
    fmt::{self},
    io::{self, Read, Write},
    path::{Path, PathBuf},

❌ New issue: Low Cohesion
This module has at least 9 different responsibilities amongst its 47 functions, threshold = 3

@leynos
Copy link
Owner Author

leynos commented Nov 6, 2025

@coderabbitai Please suggest a fix for this issue and supply a prompt for an AI coding agent to enable it to apply the fix:

src/stdlib/command.rs

Comment on lines +907 to +913

fn read_pipe<R>(mut reader: R, spec: PipeSpec) -> Result<PipeOutcome, CommandFailure>
where
    R: Read,
{
    let mut limit = spec.into_limit();
    match spec.mode() {
        OutputMode::Capture => {

❌ New issue: Bumpy Road Ahead
read_pipe has 2 blocks with nested conditional logic. Any nesting of 2 or deeper is considered. Threshold is 2 blocks per function

@leynos
Copy link
Owner Author

leynos commented Nov 6, 2025

@coderabbitai Please suggest a fix for this issue and supply a prompt for an AI coding agent to enable it to apply the fix:

tests/steps/stdlib_steps/rendering.rs

Comment on lines +130 to +139

pub(crate) fn render_stdlib_template_with_text(
    world: &mut CliWorld,
    template_content: TemplateContent,
) -> Result<()> {
    let text = world
        .stdlib_text
        .clone()
        .context("expected stdlib template text to be configured")?;
    render_template_with_context(world, &template_content, context!(text => text))
}

❌ New issue: Code Duplication
The module contains 3 functions with similar structure: render_stdlib_template_with_command,render_stdlib_template_with_text,render_stdlib_template_with_url

@coderabbitai

This comment was marked as resolved.

@coderabbitai

This comment was marked as resolved.

@coderabbitai

This comment was marked as resolved.

@leynos
Copy link
Owner Author

leynos commented Nov 6, 2025

@coderabbitai please create a new GitHub issue for the following:

Task: Refactor src/stdlib/command.rs to resolve low cohesion by splitting into focused sub-modules.

Context:

  • Current file: src/stdlib/command.rs (1022 lines, 47 functions, 9 responsibilities)
  • Target: Split into 8 sub-modules + coordinator under src/stdlib/command/
  • Preserve all existing functionality, tests, and platform-specific behaviour
  • Maintain existing public API surface via re-exports in mod.rs

Steps:

  1. Create directory src/stdlib/command/ and move src/stdlib/command.rs to src/stdlib/command/mod_backup.rs

  2. Create src/stdlib/command/config.rs:

    • Move: CommandConfig, OutputMode, OutputStream, PipeSpec, PipeLimit, CommandOptions (struct and impl blocks)
    • Add derives/visibility as needed
    • Keep all methods and Default impl for CommandOptions
  3. Create src/stdlib/command/error.rs:

    • Move: CommandFailure enum with all variants and Fromio::Error impl
    • Move: ExitDetails, LimitExceeded structs with impl blocks
    • Move: All error constructor functions (spawn_error, io_error, broken_pipe_error, exit_error, timeout_error, output_limit_error, stream_path_error, command_error)
    • Move: append_exit_status, append_stderr helper functions
    • Import necessary types from super::config and super::context
  4. Create src/stdlib/command/context.rs:

    • Move: CommandContext, GrepCall, CommandLocation structs with all impl blocks
    • Import necessary types from super::config
  5. Create src/stdlib/command/result.rs:

    • Move: StdoutResult, PipeOutcome enums
  6. Create src/stdlib/command/execution.rs:

    • Move: run_command, run_program (cfg-gated), run_child, wait_for_exit
    • Move: SHELL, SHELL_ARGS, COMMAND_TIMEOUT constants
    • Import from super::{config, error, result, pipes, quote}
  7. Create src/stdlib/command/pipes.rs:

    • Move: spawn_pipe_reader, join_reader, read_pipe, create_empty_tempfile
    • Move: cleanup_readers, join_pipe_for_cleanup, handle_stdin_result
    • Import from super::{config, error, result}
  8. Create src/stdlib/command/filters.rs:

    • Move: execute_shell, execute_grep
    • Move: collect_flag_args, format_command, to_bytes
    • Import from super::{config, context, error, result, execution, quote}
  9. Create src/stdlib/command/quote.rs:

    • Move: quote function (both Windows and non-Windows versions with cfg gates)
    • Move: QuoteError enum with Display impl
    • Move: The quote_escapes_cmd_metacharacters test under #[cfg(test)] mod tests
    • Add necessary imports (fmt, shell_quote for non-Windows)
  10. Create src/stdlib/command/mod.rs:

    • Add module documentation (move from original file header)
    • Declare sub-modules: mod config; mod error; mod context; mod result; mod execution; mod pipes; mod filters; mod quote;
    • Add necessary imports (Arc, AtomicBool, minijinja types, etc.)
    • Move: register() function (import needed types from sub-modules)
    • Add pub(crate) use statements to re-export types used by parent stdlib module
    • Import value_from_bytes from super::value_from_bytes
  11. Update imports:

    • Ensure each sub-module imports only what it needs from siblings via super::module_name
    • Use pub(crate) or pub(super) visibility appropriately
    • Maintain platform-specific cfg gates for Windows/non-Windows code
  12. Verification:

    • Run: make check-fmt
    • Run: make lint
    • Run: make test
    • Ensure all tests pass and no functionality changed

Notes:

  • Keep platform-specific cfg attributes with their respective code
  • Preserve all comments and documentation
  • Use pub(crate) for items used outside the command module, pub(super) for items used between sub-modules
  • The public API surface should remain identical via re-exports in mod.rs

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Nov 6, 2025

Tip

For best results, initiate chat on the files or code changes.

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Create a GitHub issue for the command.rs refactoring task

gh issue create \
  --title "Refactor src/stdlib/command.rs to resolve low cohesion" \
  --body "## Context

**Related PR:** #231
**Requested by:** @leynos

The current \`src/stdlib/command.rs\` file has grown to 1,022 lines with 47 functions spanning at least 9 different responsibilities, exceeding the cohesion threshold of 3. This issue tracks the refactoring work to split the module into focused sub-modules.

### Current State
- File: \`src/stdlib/command.rs\` (1,022 lines, 47 functions, 9 responsibilities)
- Issue: Low cohesion detected by CodeScene analysis

### Target
- Split into 8 sub-modules + coordinator under \`src/stdlib/command/\`
- Preserve all existing functionality, tests, and platform-specific behaviour
- Maintain existing public API surface via re-exports in \`mod.rs\`

## Implementation Steps

### 1. Create Directory Structure
Create directory \`src/stdlib/command/\` and move \`src/stdlib/command.rs\` to \`src/stdlib/command/mod_backup.rs\`

### 2. Create \`src/stdlib/command/config.rs\`
- Move: \`CommandConfig\`, \`OutputMode\`, \`OutputStream\`, \`PipeSpec\`, \`PipeLimit\`, \`CommandOptions\` (struct and impl blocks)
- Add derives/visibility as needed
- Keep all methods and Default impl for \`CommandOptions\`

### 3. Create \`src/stdlib/command/error.rs\`
- Move: \`CommandFailure\` enum with all variants and \`From<io::Error>\` impl
- Move: \`ExitDetails\`, \`LimitExceeded\` structs with impl blocks
- Move: All error constructor functions (\`spawn_error\`, \`io_error\`, \`broken_pipe_error\`, \`exit_error\`, \`timeout_error\`, \`output_limit_error\`, \`stream_path_error\`, \`command_error\`)
- Move: \`append_exit_status\`, \`append_stderr\` helper functions
- Import necessary types from \`super::config\` and \`super::context\`

### 4. Create \`src/stdlib/command/context.rs\`
- Move: \`CommandContext\`, \`GrepCall\`, \`CommandLocation\` structs with all impl blocks
- Import necessary types from \`super::config\`

### 5. Create \`src/stdlib/command/result.rs\`
- Move: \`StdoutResult\`, \`PipeOutcome\` enums

### 6. Create \`src/stdlib/command/execution.rs\`
- Move: \`run_command\`, \`run_program\` (cfg-gated), \`run_child\`, \`wait_for_exit\`
- Move: \`SHELL\`, \`SHELL_ARGS\`, \`COMMAND_TIMEOUT\` constants
- Import from \`super::{config, error, result, pipes, quote}\`

### 7. Create \`src/stdlib/command/pipes.rs\`
- Move: \`spawn_pipe_reader\`, \`join_reader\`, \`read_pipe\`, \`create_empty_tempfile\`
- Move: \`cleanup_readers\`, \`join_pipe_for_cleanup\`, \`handle_stdin_result\`
- Import from \`super::{config, error, result}\`

### 8. Create \`src/stdlib/command/filters.rs\`
- Move: \`execute_shell\`, \`execute_grep\`
- Move: \`collect_flag_args\`, \`format_command\`, \`to_bytes\`
- Import from \`super::{config, context, error, result, execution, quote}\`

### 9. Create \`src/stdlib/command/quote.rs\`
- Move: \`quote\` function (both Windows and non-Windows versions with cfg gates)
- Move: \`QuoteError\` enum with Display impl
- Move: The \`quote_escapes_cmd_metacharacters\` test under \`#[cfg(test)] mod tests\`
- Add necessary imports (\`fmt\`, \`shell_quote\` for non-Windows)

### 10. Create \`src/stdlib/command/mod.rs\`
- Add module documentation (move from original file header)
- Declare sub-modules: \`mod config; mod error; mod context; mod result; mod execution; mod pipes; mod filters; mod quote;\`
- Add necessary imports (\`Arc\`, \`AtomicBool\`, minijinja types, etc.)
- Move: \`register()\` function (import needed types from sub-modules)
- Add \`pub(crate) use\` statements to re-export types used by parent stdlib module
- Import \`value_from_bytes\` from \`super::value_from_bytes\`

### 11. Update Imports
- Ensure each sub-module imports only what it needs from siblings via \`super::module_name\`
- Use \`pub(crate)\` or \`pub(super)\` visibility appropriately
- Maintain platform-specific cfg gates for Windows/non-Windows code

### 12. Verification
- Run: \`make check-fmt\`
- Run: \`make lint\`
- Run: \`make test\`
- Ensure all tests pass and no functionality changed

## Important Notes

- Keep platform-specific cfg attributes with their respective code
- Preserve all comments and documentation
- Use \`pub(crate)\` for items used outside the command module, \`pub(super)\` for items used between sub-modules
- The public API surface should remain identical via re-exports in \`mod.rs\`
- Each sub-module should have a single, clear responsibility, reducing the cohesion metric from 9 to 1–2 per module

## Expected Outcome

After refactoring:
- Each sub-module focuses on a single responsibility
- Cohesion metric improves from 9 to 1–2 per module
- All existing tests pass
- Public API remains unchanged
- Code is more maintainable and easier to navigate" \
  --assignee leynos

Length of output: 44


@leynos: I've created GitHub issue #233 for the refactoring task: #233

The issue includes:

  • Full context about the low cohesion problem (1,022 lines, 47 functions, 9 responsibilities)
  • All 12 implementation steps you provided
  • Important notes about preserving platform-specific code and API surface
  • Expected outcomes and verification steps
  • Backlink to this PR Enforce command output limits and add streaming support #231
  • Assignment to you

* refactor(stdlib): split read_pipe into separate capture and tempfile functions

Refactored the read_pipe function in src/stdlib/command.rs by splitting its logic based on OutputMode into two distinct functions, read_pipe_capture and read_pipe_tempfile. This improves code clarity and separation of concerns while maintaining existing behavior.

Additionally, refactored template rendering test helpers in tests/steps/stdlib_steps/rendering.rs to consolidate context construction into a single helper function render_with_single_context, reducing code duplication in tests.

Co-authored-by: terragon-labs[bot] <terragon-labs[bot]@users.noreply.github.com>

* feat(stdlib/command): enforce byte limits and use workspace-rooted tempfiles for command output

- Introduce distinct configurable byte limits for stdout capture and streaming in CommandConfig.
- Implement workspace-rooted temporary file creation for streaming command outputs, replacing system temp directory usage.
- Modify child process pipe readers to enforce configured limits and produce bytes or workspace-based tempfiles accordingly.
- Propagate workspace root path into stdlib configuration, enabling temp file placement inside project directories.
- Add tests validating capturing and streaming output within limits and error handling on limit exceedance.
- Enhance documentation with a Mermaid class diagram illustrating pipe reading abstractions.
- Update stdlib configuration defaults to include workspace root path for consistent temp file handling.
- Adjust tests and feature files to verify new output limit enforcement and streaming behavior.

This improves resource control and workspace hygiene for command output handling in Netsuke standard library.

Co-authored-by: terragon-labs[bot] <terragon-labs[bot]@users.noreply.github.com>

* refactor(stdlib/command): use references for CommandContext to avoid cloning

Changed functions to accept `&CommandContext` instead of consuming `CommandContext` by value to improve efficiency and avoid unnecessary cloning. Made `CommandContext::new` a `const fn`. Updated related pipe reader functions to accept config by reference rather than Arc cloning. Fixed a slice indexing panic by validating bounds before write.

Co-authored-by: terragon-labs[bot] <terragon-labs[bot]@users.noreply.github.com>

---------

Co-authored-by: terragon-labs[bot] <terragon-labs[bot]@users.noreply.github.com>
codescene-delta-analysis[bot]

This comment was marked as outdated.

@leynos
Copy link
Owner Author

leynos commented Nov 6, 2025

@coderabbitai Please suggest a fix for this issue and supply a prompt for an AI coding agent to enable it to apply the fix:

tests/steps/stdlib_steps/rendering.rs

Comment on file

        .clone()
        .context("expected stdlib HTTP server to be initialised")?;
    render_template_with_context(world, &template_content, context!(url => url))
    render_with_single_context(world, &template_content, "url", url)

❌ New issue: Code Duplication
The module contains 3 functions with similar structure: render_stdlib_template_with_command,render_stdlib_template_with_text,render_stdlib_template_with_url

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

♻️ Duplicate comments (2)
src/stdlib/command.rs (2)

1-1257: File length critically exceeds the 400-line guideline.

At 1,257 lines this module violates the repository's file-length limit and the Low Cohesion finding. Issue #233 tracks the planned decomposition into sub-modules; complete that split to restore compliance.


1068-1071: Simplify slice extraction by trusting Read's contract.

Read::read guarantees read ≤ chunk.len(), so &chunk[..read] is safe. Remove the redundant bounds check.

Apply this diff:

-        let slice = chunk
-            .get(..read)
-            .ok_or_else(|| CommandFailure::Io(io::Error::other("pipe read out of range")))?;
-        file.write_all(slice).map_err(CommandFailure::Io)?;
+        file.write_all(&chunk[..read]).map_err(CommandFailure::Io)?;
📜 Review details

Configuration used: CodeRabbit UI

Review profile: ASSERTIVE

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 6255aba and 66b3f80.

📒 Files selected for processing (7)
  • docs/netsuke-design.md (1 hunks)
  • src/manifest/mod.rs (1 hunks)
  • src/stdlib/command.rs (15 hunks)
  • src/stdlib/mod.rs (10 hunks)
  • tests/features/stdlib.feature (1 hunks)
  • tests/std_filter_tests/network_functions.rs (2 hunks)
  • tests/steps/stdlib_steps/rendering.rs (5 hunks)
🧰 Additional context used
📓 Path-based instructions (6)
**/*.rs

📄 CodeRabbit inference engine (AGENTS.md)

**/*.rs: Every Rust module must begin with a module-level comment using //! explaining the module's purpose and utility
Document public APIs with Rustdoc comments (///) so documentation can be generated with cargo doc
Prefer immutable data; avoid unnecessary mut bindings
Handle errors with Result instead of panicking where feasible
Prefer semantic error enums deriving std::error::Error via thiserror for inspectable conditions
Clippy warnings must be denied; do not silence lints except as a last resort
Lint suppressions must be tightly scoped and include a clear reason; prefer #[expect(...)] over #[allow(...)]
Place function attributes after doc comments
Do not use return in single-line functions
Prefer single-line function definitions where appropriate (e.g., pub fn new(id: u64) -> Self { Self(id) })
Use predicate functions for conditional criteria with more than two branches
Where a function is too long, extract meaningfully named helpers adhering to separation of concerns and CQRS
Where a function has too many parameters, group related parameters into meaningfully named structs
When returning large error data, consider using Arc to reduce data moved
Prefer .expect() over .unwrap() on Results/Options
Use concat!() to combine long string literals instead of escaping newlines with backslashes
Use NewTypes to model domain values; prefer explicit tuple structs; leverage newt-hype and the-newtype crates as described; provide appropriate From/AsRef/TryFrom implementations and avoid orphan-rule-violating impls
Keep each Rust code file under 400 lines; split long switches/dispatch tables by feature; move large test data to external files
Use functions and composition to avoid repetition; extract reusable logic; prefer declarative constructs (iterators/generators/comprehensions) when readable
Small, single-responsibility functions; obey command/query segregation
Name things precisely; booleans should use is/has/should prefixes
Comment why, not what; explain assumptions, e...

Files:

  • tests/std_filter_tests/network_functions.rs
  • src/manifest/mod.rs
  • src/stdlib/mod.rs
  • tests/steps/stdlib_steps/rendering.rs
  • src/stdlib/command.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 return in single-line functions.
  • Move conditionals with >2 branches into a predicate function.
  • Avoid unsafe unless 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 rstest fixtures for shared setup and to avoid repetition between tests.
  • Replace duplicated tests with #[rstest(...)] parameterised cases.
  • Prefer mockall for mocks/stubs.
  • Prefer .expect() over .unwrap() in tests.
  • .expect() and .unwrap() are forbidden outside of tests. Errors must be propagated.
  • Ensure that any API or behavioural changes are reflected in the documentation in docs/
  • Ensure that any completed roadmap steps are recorded in the appropriate roadmap in docs/
  • Files must not exceed 400 lines in length
    • Large modules must be decomposed
    • Long match statements or dispatch tables should be decomposed by domain and collocated with targets
    • Large blocks of inline data (e.g., test fixtures, constants or templates) must be moved to external files and inlined at compile-time or loaded at run-time.
  • Environment access (env::set_var and env::remove_var) are always unsafe in Rust 2024 and MUST be marked as such
    • For testing of functionality depending upon environment variables, dependency injection and...

Files:

  • tests/std_filter_tests/network_functions.rs
  • src/manifest/mod.rs
  • src/stdlib/mod.rs
  • tests/steps/stdlib_steps/rendering.rs
  • src/stdlib/command.rs
**/*.{rs,md}

📄 CodeRabbit inference engine (AGENTS.md)

Use en-GB-oxendict spelling and grammar in comments and documentation (exceptions allowed for external API names)

Files:

  • tests/std_filter_tests/network_functions.rs
  • docs/netsuke-design.md
  • src/manifest/mod.rs
  • src/stdlib/mod.rs
  • tests/steps/stdlib_steps/rendering.rs
  • src/stdlib/command.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/network_functions.rs
  • tests/steps/stdlib_steps/rendering.rs
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
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; use plaintext for 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, use ![alt text](path) and provide brief, descriptive alt text
Add a short descriptive sentence before each Mermaid diagram for screen readers

Files:

  • docs/netsuke-design.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/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
🧬 Code graph analysis (5)
tests/std_filter_tests/network_functions.rs (3)
tests/std_filter_tests/support.rs (1)
  • stdlib_env_with_config (38-44)
src/stdlib/command.rs (8)
  • new (102-113)
  • new (128-134)
  • new (213-219)
  • new (358-360)
  • new (381-383)
  • new (393-395)
  • new (409-411)
  • new (422-428)
src/stdlib/mod.rs (1)
  • new (96-111)
src/manifest/mod.rs (2)
src/stdlib/command.rs (8)
  • new (102-113)
  • new (128-134)
  • new (213-219)
  • new (358-360)
  • new (381-383)
  • new (393-395)
  • new (409-411)
  • new (422-428)
src/stdlib/mod.rs (1)
  • new (96-111)
src/stdlib/mod.rs (2)
src/stdlib/command.rs (10)
  • new (102-113)
  • new (128-134)
  • new (213-219)
  • new (358-360)
  • new (381-383)
  • new (393-395)
  • new (409-411)
  • new (422-428)
  • config (366-368)
  • register (442-476)
src/stdlib/network/mod.rs (5)
  • path (119-121)
  • new (111-117)
  • new (328-333)
  • new (348-354)
  • register_functions (47-56)
tests/steps/stdlib_steps/rendering.rs (2)
src/stdlib/command.rs (8)
  • new (102-113)
  • new (128-134)
  • new (213-219)
  • new (358-360)
  • new (381-383)
  • new (393-395)
  • new (409-411)
  • new (422-428)
src/stdlib/mod.rs (1)
  • new (96-111)
src/stdlib/command.rs (1)
src/stdlib/mod.rs (3)
  • value_from_bytes (373-378)
  • new (96-111)
  • default (264-272)
🔍 Remote MCP Deepwiki

Summary — additional repository context relevant to reviewing this PR

  • Design & rationale: Netsuke is a build-system compiler with strict two‑phase templating (structural expansion then string rendering), capability‑scoped I/O (cap‑std), UTF‑8 paths (camino), automatic shell‑escaping for command interpolation (shell‑quote + shlex validation), and strict diagnostics (miette/anyhow/thiserror). This explains why the PR enforces per‑command output budgets and tempfile streaming rather than changing quoting/escape semantics.,

  • Template stdlib & filters: stdlib functions (env, glob, file tests) and filters (basename, with_suffix, shell_escape, etc.) are registered into the minijinja environment — shell/grep are implemented as wrappers around shell(), and shell_escape is the canonical escaping path. Any change to command behavior must preserve these registrations and strict undefined behavior.,

  • Manifest & parsing flow: manifest parsing is YAML → expand(foreach/when) → deserialize → render strings. Foreach/when expansion runs on serde_yml::Value before deserialization; env()/glob() are available during expansion. StdlibConfig/workspace_root changes relate to how templates and globbing resolve paths and how the command tempdir is derived. This impacts tests and CLI cucumber world wiring added in the PR.

  • IR & command safety: IR generation expands {{ins}}/{{outs}}, shell‑quotes each path (shell‑quote Sh mode), and validates with shlex. The PR’s CommandConfig/CommandContext should integrate with the same interpolation/quoting/validation path so behavior and security guarantees remain identical for capture vs tempfile modes.

  • Testing & CI expectations: repo uses unit + insta snapshots + cucumber BDD; CI enforces fmt/lint/tests. New tests added by the PR must be deterministic (sorted outputs, stable hashes) and fit into existing test harness (CliWorld, test_support). The PR already updates tests and cucumber steps accordingly.

Tools/sources used

  • Repository wiki & docs (search/read): Deepwiki_read_wiki_structure, Deepwiki_read_wiki_contents, Deepwiki_ask_question — used to extract architecture, manifest/template/stdlib, IR & ninja generation, security, and testing/CI details.,,
⏰ 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 (x86_64-apple-darwin, macos-13, macos-x86_64, macos-x86_64) / Build macos artefacts
  • GitHub Check: release / build-macos (aarch64-apple-darwin, macos-14, macos-arm64, macos-aarch64) / Build macos artefacts
  • GitHub Check: release / build-linux (aarch64-unknown-linux-gnu, linux-arm64, arm64) / Build linux artefacts
  • GitHub Check: release / build-windows (aarch64-pc-windows-msvc, arm64, arm64, windows-aarch64) / Build windows artefacts
  • GitHub Check: release / build-windows (x86_64-pc-windows-msvc, amd64, x64, windows-x86_64) / Build windows artefacts
  • GitHub Check: Sourcery review
  • GitHub Check: build-test (ubuntu-latest, stable)
🔇 Additional comments (32)
src/stdlib/mod.rs (11)

40-45: LGTM! Clear defaults for command limits.

The constants are well-named and the values (1 MiB capture, 64 MiB streaming) provide sensible guardrails.


67-67: LGTM! Field integrates workspace path tracking.

The optional path enables capability-scoped tempfile creation.


71-72: LGTM! Command limit fields complete the configuration surface.


96-111: LGTM! Constructor correctly initialises new configuration fields.


173-182: LGTM! Validation correctly enforces positive limits.


184-197: LGTM! Stream limit builder mirrors the capture limit pattern.


206-233: LGTM! Method correctly decomposes configuration into component configs.

The Arc wrapping and cloning patterns ensure shared ownership where needed.


264-273: LGTM! Default implementation correctly resolves workspace path.

The panics are appropriate for a Default implementation when environment invariants are violated.


329-340: LGTM! Error handling correctly propagates failures.

The function properly threads the workspace path through to the configuration.


360-371: LGTM! Registration correctly wires command configuration.


494-556: LGTM! Tests comprehensively verify command limit configuration.

The test coverage includes defaults, builder validation, and propagation through component construction.

src/stdlib/command.rs (21)

8-15: LGTM! Documentation clearly explains the new limit enforcement.


94-118: LGTM! Configuration struct encapsulates command limits and temp directory.


151-159: LGTM! Wrapper provides a typed interface for temporary files.


161-203: LGTM! Enums cleanly model output modes and streams.


205-239: LGTM! Specification type cleanly encapsulates pipe configuration.


241-264: LGTM! Limit tracker correctly enforces boundaries with overflow protection.


266-337: LGTM! Options parser correctly handles string and map forms.

The multiple aliases for streaming mode improve user ergonomics.


339-349: LGTM! Distinct types for internal and external outcomes enhance type safety.


351-373: LGTM! Context object correctly combines shared and per-call configuration.


375-429: LGTM! Helper structs cleanly encapsulate command execution state.


442-476: LGTM! Registration correctly threads configuration into filter closures.


478-499: LGTM! Function correctly adapts stdout result to template value.


501-532: LGTM! Grep execution mirrors shell execution pattern.


674-703: LGTM! Command builders correctly thread context through to execution.


705-765: LGTM! Child execution correctly orchestrates pipe reading with limits.

The stderr tempfile warning (line 747) provides useful diagnostics for unexpected conditions.


978-1007: LGTM! Reader lifecycle correctly handles missing pipes.


1012-1027: LGTM! Refactored pipe reader eliminates nested conditionals.

Delegation to mode-specific helpers addresses the previous "Bumpy Road Ahead" finding.


1031-1046: LGTM! Capture path correctly accumulates bytes within the limit.


1082-1093: LGTM! Empty tempfile creation follows the same persistence pattern.


851-922: LGTM! Error constructors provide consistent, descriptive diagnostics.


1116-1256: LGTM! Tests comprehensively verify pipe reading with limits.

Coverage includes both capture and tempfile modes under normal and over-limit conditions.

@leynos
Copy link
Owner Author

leynos commented Nov 6, 2025

@coderabbitai pause

@coderabbitai

This comment was marked as resolved.

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Nov 6, 2025

✅ Actions performed

Reviews paused.

* refactor(stdlib,manifest): improve workspace root handling and render helpers

- Resolve workspace root as absolute path in manifest loading, supporting relative paths by joining with current working directory.
- Enforce absolute workspace root path in StdlibConfig with panic on invalid input.
- Return error if workspace root not configured when creating command temp files.
- Replace unsafe slice indexing with safe slice usage in command file writes.
- Introduce macro in stdlib_steps for DRY rendering with world string fields, simplifying template rendering functions.

Co-authored-by: terragon-labs[bot] <terragon-labs[bot]@users.noreply.github.com>

* feat(manifest): add workspace root path handling and related tests

- Handle resolution of relative and absolute workspace root paths with UTF-8 validation
- Expose workspace_root_path() getter in StdlibConfig
- Add tests for resolving workspace roots and rejecting non-UTF-8 paths
- Add test ensuring command tempdir requires workspace root path
- Add manifest subcommand test for relative manifest paths

These changes improve manifest workspace handling by enforcing UTF-8 constraints and providing better API visibility and test coverage.

Co-authored-by: terragon-labs[bot] <terragon-labs[bot]@users.noreply.github.com>

* refactor(manifest): extract absolute workspace root resolution to helper

Refactored workspace root resolution logic in stdlib_config_for_manifest into a
new function resolve_absolute_workspace_root for clarity and reuse. Updated
related tests to combine relative and absolute workspace root checks into a
single parameterized test for better coverage and reduced duplication.

Additionally, introduced assert_output_limit_error helper in stdlib/command tests
to consolidate output limit error assertions in multiple test cases.

This improves code maintainability and test clarity.

Co-authored-by: terragon-labs[bot] <terragon-labs[bot]@users.noreply.github.com>

---------

Co-authored-by: terragon-labs[bot] <terragon-labs[bot]@users.noreply.github.com>
codescene-delta-analysis[bot]

This comment was marked as outdated.

@leynos
Copy link
Owner Author

leynos commented Nov 7, 2025

@coderabbitai Please suggest a fix for this issue and supply a prompt for an AI coding agent to enable it to apply the fix:

tests/steps/stdlib_steps/rendering.rs

Comment on lines +128 to +134

    render_with_world_string_field!(
        world,
        &template_content,
        "url",
        stdlib_url,
        "expected stdlib HTTP server to be initialised"
    )

❌ New issue: Code Duplication
The module contains 3 functions with similar structure: render_stdlib_template_with_command,render_stdlib_template_with_text,render_stdlib_template_with_url

@coderabbitai

This comment was marked as resolved.

* refactor(command): replace tempfile crate with cap-std for command temp files

Reworked command temporary file management to use cap-std crate instead of tempfile.
Implemented a custom unique tempfile naming strategy with atomic counter.
Improved tempfile lifecycle handling and removal on drop when not persisted.
Standardized sanitation of tempfile labels for safer filenames.
Added robust error handling for tempfile creation attempts.
Smoothed tempfile usage in pipe reading and empty tempfile creation.
Changed workspace root path validation message for clarity.

Co-authored-by: terragon-labs[bot] <terragon-labs[bot]@users.noreply.github.com>

* refactor(command): replace manual tempfile creation with tempfile crate

Replaced the custom logic for creating temporary files within the command module with the tempfile crate's Builder API. This simplifies tempfile creation, removes counters and manual uniqueness handling, and improves error handling. Dropped the custom CommandTempFile's complex management for a simpler wrapper around NamedTempFile, ensuring proper cleanup and persistence semantics. Added tests for label sanitization and tempfile lifecycle behavior.

Co-authored-by: terragon-labs[bot] <terragon-labs[bot]@users.noreply.github.com>

* style(stdlib/command): simplify flush call by chaining method calls

Co-authored-by: terragon-labs[bot] <terragon-labs[bot]@users.noreply.github.com>

---------

Co-authored-by: terragon-labs[bot] <terragon-labs[bot]@users.noreply.github.com>
Copy link

@codescene-delta-analysis codescene-delta-analysis bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Gates Failed
Enforce critical code health rules (1 file with Low Cohesion)
Enforce advisory code health rules (1 file with Code Duplication)

Gates Passed
4 Quality Gates Passed

See analysis details in CodeScene

Reason for failure
Enforce critical code health rules Violations Code Health Impact
command.rs 1 critical rule 10.00 → 8.55 Suppress
Enforce advisory code health rules Violations Code Health Impact
rendering.rs 1 advisory rule 10.00 → 9.39 Suppress

Quality Gate Profile: Pay Down Tech Debt
Want more control? Customize Code Health rules or catch issues early with our IDE extension and CLI tool.

@leynos leynos merged commit 3f59804 into main Nov 8, 2025
14 of 15 checks passed
@leynos leynos deleted the codex/enforce-command-output-size-limits branch November 8, 2025 02:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant