Refactor rendering tests with macro and absolute workspace roots#234
Conversation
… 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>
Reviewer's GuideThis PR refactors rendering tests by introducing a macro for string‐field rendering, strengthens workspace root path resolution in manifest and stdlib modules through absolute path enforcement and clearer errors, and refines command tempfile creation and buffer writes with explicit error handling and safe slicing. Class diagram for updated StdlibConfig and CommandTempDirclassDiagram
class StdlibConfig {
+with_workspace_root_path(path: Utf8PathBuf) StdlibConfig
workspace_root_path: Option<Utf8PathBuf>
}
class CommandTempDir {
+create_tempfile(builder, dir_path)
}
StdlibConfig <|-- CommandTempDir: uses workspace_root_path
Class diagram for new render_with_world_string_field macro usage in testsclassDiagram
class TestWorld {
string_field: String
}
class render_with_world_string_field {
+macro(world: TestWorld, template: Template)
}
TestWorld --> render_with_world_string_field: used by
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the 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. ✨ Finishing touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
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/manifest/mod.rs:210` </location>
<code_context>
- "failed to open workspace directory '{utf8_parent}' for manifest '{manifest_label}'"
- )
- })?;
+ let workspace_root = if utf8_parent.is_absolute() {
+ utf8_parent.to_path_buf()
+ } else {
</code_context>
<issue_to_address>
**issue (complexity):** Consider simplifying the workspace root resolution by building and converting the path only once, eliminating redundant variables and conversions.
```suggestion
// Instead of manually cloning, converting twice, and introducing `workspace_root_for_error`,
// you can build a single PathBuf, convert once to Utf8PathBuf, and use it directly:
use std::path::PathBuf;
let base: PathBuf = if utf8_parent.is_absolute() {
utf8_parent.to_path_buf()
} else {
env::current_dir()
.context("resolve current directory for manifest workspace root")?
.join(utf8_parent.as_str())
};
// Optionally canonicalize to resolve symlinks:
// let base = fs::canonicalize(&base).context("canonicalize workspace root")?;
let workspace_root = Utf8PathBuf::from_path_buf(base.clone())
.map_err(|_| anyhow!(
"workspace root '{}' contains non-UTF-8 components",
base.display()
))?;
let dir = Dir::open_ambient_dir(workspace_root.as_std_path(), ambient_authority())
.with_context(|| {
format!(
"failed to open workspace directory '{}' for manifest '{}'",
workspace_root, manifest_label
)
})?;
Ok(StdlibConfig::new(dir)
.with_workspace_root_path(workspace_root)
.with_network_policy(policy))
```
- Removes the extra `cwd_utf8`, `workspace_root_for_error`, and redundant `.clone()`.
- Joins and (optionally) canonicalizes once on a `PathBuf`.
- Converts just once to `Utf8PathBuf` before opening the directory.
</issue_to_address>
### Comment 2
<location> `src/manifest/mod.rs:210` </location>
<code_context>
- "failed to open workspace directory '{utf8_parent}' for manifest '{manifest_label}'"
- )
- })?;
+ let workspace_root = if utf8_parent.is_absolute() {
+ utf8_parent.to_path_buf()
+ } else {
</code_context>
<issue_to_address>
**issue (review_instructions):** Add behavioural and unit tests for new workspace root resolution logic.
The logic for resolving the workspace root from relative paths is new and non-trivial. You must add both behavioural and unit tests to verify correct handling of absolute and relative paths, including error cases for non-UTF-8 paths.
<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.rs:145` </location>
<code_context>
builder.tempfile_in(dir_path.as_std_path())?
} else {
- builder.tempfile()?
+ return Err(io::Error::new(
+ io::ErrorKind::InvalidInput,
+ "workspace root path must be configured for command tempfiles",
</code_context>
<issue_to_address>
**issue (review_instructions):** Demonstrate bug fix for missing workspace root path with a test.
You added an error for missing workspace root path when creating command tempfiles. You must add a test that demonstrates this bug fix, ensuring the error is triggered as expected.
<details>
<summary>Review instructions:</summary>
**Path patterns:** `**/*`
**Instructions:**
Bug fixes *must* be demonstrated by a test.
</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.
- 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>
|
@coderabbitai Please suggest a fix for this issue and supply a prompt for an AI coding agent to enable it to apply the fix: Comment on lines +299 to +313 fn stdlib_config_for_manifest_resolves_relative_workspace_root() -> AnyResult<()> {
let temp = tempdir().context("create temp workspace")?;
let _guard = CurrentDirGuard::change_to(temp.path())?;
let config = stdlib_config_for_manifest(Path::new("Netsukefile"), NetworkPolicy::default())?;
let recorded = config
.workspace_root_path()
.context("workspace root path should be recorded")?;
let expected = camino::Utf8Path::from_path(temp.path())
.context("temp workspace path should be valid UTF-8")?;
ensure!(
recorded == expected,
"expected workspace root {expected}, got {recorded}"
);
Ok(())
}❌ New issue: Code Duplication |
|
@coderabbitai Please suggest a fix for this issue and supply a prompt for an AI coding agent to enable it to apply the fix: Comment on file builder.tempfile_in(dir_path.as_std_path())?
} else {
builder.tempfile()?
return Err(io::Error::new(❌ New issue: Code Duplication |
|
@coderabbitai Please suggest a fix for this issue and supply a prompt for an AI coding agent to enable it to apply the fix: Comment on lines +210 to +232 let workspace_base = if utf8_parent.is_absolute() {
utf8_parent.to_path_buf().into_std_path_buf()
} else {
env::current_dir()
.context("resolve current directory for manifest workspace root")?
.join(utf8_parent.as_std_path())
};
let workspace_root = match Utf8PathBuf::from_path_buf(workspace_base) {
Ok(valid) => valid,
Err(invalid) => {
return Err(anyhow!(
"workspace root '{}' contains non-UTF-8 components",
invalid.display()
));
}
};
let dir = Dir::open_ambient_dir(workspace_root.as_path(), ambient_authority()).with_context(
|| {
format!(
"failed to open workspace directory '{workspace_root}' \
for manifest '{manifest_label}'"
)
},❌ New issue: Complex Method |
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
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>
e4704da
into
codex/enforce-command-output-size-limits
* Enforce bounded command output and stream large results * Refactor read_pipe and workspace-scoped tempfile plumbing (#232) * 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> * Refactor rendering tests with macro and absolute workspace roots (#234) * 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> * Enforce workspace-scoped tempfile creation with UTF-8 paths (#235) * 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> --------- Co-authored-by: terragon-labs[bot] <terragon-labs[bot]@users.noreply.github.com>
Summary
Changes
Tests
Manifest handling
Stdlib
Why
Testing & Validation
🌿 Generated by Terry
ℹ️ Tag @terragon-labs to ask questions and address PR feedback
📎 Task: https://www.terragonlabs.com/task/62f29cac-61af-4f6f-8e9a-42c9a330b901
Summary by Sourcery
Refactor rendering tests to use a new macro for extracting string fields and tighten workspace root handling across manifest and stdlib modules with clearer errors and safety checks
New Features:
Enhancements: