Add file-system Jinja tests #151
Conversation
Reviewer's GuideThis PR adds a dedicated stdlib module that registers custom Jinja Sequence diagram for registering Jinja file-system tests in manifest parsingsequenceDiagram
participant manifest
participant stdlib
participant jinja_env
manifest->>jinja_env: add_function("env", ...)
manifest->>jinja_env: add_function("glob", ...)
manifest->>stdlib: register(&mut jinja_env)
stdlib->>jinja_env: add_test("dir", ...)
stdlib->>jinja_env: add_test("file", ...)
stdlib->>jinja_env: add_test("symlink", ...)
stdlib->>jinja_env: add_test("pipe", ...)
stdlib->>jinja_env: add_test("device", ...)
ER diagram for Jinja file-system test registration and usageerDiagram
ENVIRONMENT {
string id
}
JINJA_TEST {
string name
function predicate
}
ENVIRONMENT ||--o{ JINJA_TEST : registers
JINJA_TEST }|--|{ FILE_SYSTEM_TYPE : tests
FILE_SYSTEM_TYPE {
string type_name
}
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
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. Warning Rate limit exceeded@leynos has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 0 minutes and 36 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (8)
Summary by CodeRabbit
WalkthroughAdd a public stdlib module and register it in manifest parsing to expose five Jinja tests for Unix file types (dir, file, symlink, pipe, device). Add dev-dependency on nix for tests. Introduce test data, Cucumber steps, and feature scenarios. Update documentation and roadmap to describe and mark these tests as complete. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant CLI as CLI / Tests
participant Parser as Manifest Parser
participant Jinja as Jinja Environment
participant Stdlib as stdlib::register
participant FS as OS File System
CLI->>Parser: parse(manifest)
Parser->>Jinja: create Environment
Parser->>Jinja: register env/glob helpers
Parser->>Stdlib: register(&mut Environment)
Stdlib->>Jinja: add tests: dir, file, symlink, pipe, device
Parser-->>CLI: parsed manifest (tests available)
sequenceDiagram
autonumber
participant Eval as Template Eval
participant Test as file-type test
participant FS as OS File System
Eval->>Test: evaluate env(PATH) is <type>
Test->>FS: symlink_metadata(PATH)
alt path exists
Test->>FS: md.file_type().is_<type>()
Test-->>Eval: true/false
else path missing (NotFound)
Test-->>Eval: false
else other I/O error
Test-->>Eval: template error (InvalidOperation)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
✨ Finishing Touches
🧪 Generate unit tests
Comment |
![codescene-delta-analysis[bot]](https://avatars.githubusercontent.com/in/53921?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
![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 found some issues that need to be addressed.
- The nix crate is only used in tests—move it to dev-dependencies to avoid pulling it into production builds.
- The file-system tests rely on Unix-only APIs; consider adding conditional compilation or alternative implementations to support Windows or document the Unix-only limitation.
- In is_file_type, wrapping IO errors into ErrorKind::InvalidOperation loses original error context—consider preserving the source error for better debugging.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The nix crate is only used in tests—move it to dev-dependencies to avoid pulling it into production builds.
- The file-system tests rely on Unix-only APIs; consider adding conditional compilation or alternative implementations to support Windows or document the Unix-only limitation.
- In is_file_type, wrapping IO errors into ErrorKind::InvalidOperation loses original error context—consider preserving the source error for better debugging.
## Individual Comments
### Comment 1
<location> `src/stdlib.rs:17` </location>
<code_context>
+ env.add_test("pipe", |path: String| {
+ is_file_type(&path, |ft| ft.is_fifo())
+ });
+ env.add_test("device", |path: String| {
+ is_file_type(&path, |ft| ft.is_block_device() || ft.is_char_device())
+ });
+}
</code_context>
<issue_to_address>
The 'device' test combines block and char devices, which may be ambiguous for template users.
Consider adding distinct tests for block and character devices, or update documentation to clarify how 'device' should be used.
Suggested implementation:
```rust
env.add_test("pipe", |path: String| {
is_file_type(&path, |ft| ft.is_fifo())
});
env.add_test("block_device", |path: String| {
is_file_type(&path, |ft| ft.is_block_device())
});
env.add_test("char_device", |path: String| {
is_file_type(&path, |ft| ft.is_char_device())
});
// The 'device' test is retained for backward compatibility, but users are encouraged to use 'block_device' or 'char_device' for clarity.
env.add_test("device", |path: String| {
is_file_type(&path, |ft| ft.is_block_device() || ft.is_char_device())
});
}
```
If you have documentation for these tests (e.g., in comments or external docs), update it to mention the new 'block_device' and 'char_device' tests and clarify the meaning of 'device'.
</issue_to_address>
### Comment 2
<location> `src/stdlib.rs:6` </location>
<code_context>
+use std::{fs, io};
+
+/// Register standard library helpers with the Jinja environment.
+pub fn register(env: &mut Environment<'_>) {
+ env.add_test("dir", |path: String| is_file_type(&path, |ft| ft.is_dir()));
+ env.add_test("file", |path: String| {
</code_context>
<issue_to_address>
Consider refactoring the repeated test registration logic into a loop over a static list of test names and predicates.
```rust
use minijinja::{Environment, Error, ErrorKind};
use std::{fs, io};
use std::os::unix::fs::{FileTypeExt, FileType};
/// Register standard library helpers with the Jinja environment.
pub fn register(env: &mut Environment<'_>) {
// a static list of (test_name, predicate) pairs
const TESTS: &[(&str, fn(FileType) -> bool)] = &[
("dir", FileType::is_dir),
("file", FileType::is_file),
("symlink", FileTypeExt::is_symlink),
("pipe", FileTypeExt::is_fifo),
("device", |ft| ft.is_block_device() || ft.is_char_device()),
];
for (name, pred) in TESTS {
// call the same helper with each predicate
env.add_test(*name, move |path: String| is_file_type(&path, *pred));
}
}
/// Determine whether `path` matches the given file type predicate.
///
/// Returns `Ok(false)` if the path does not exist.
fn is_file_type<F>(path: &str, predicate: F) -> Result<bool, Error>
where
F: Fn(fs::FileType) -> bool,
{
match fs::symlink_metadata(path) {
Ok(md) => Ok(predicate(md.file_type())),
Err(err) if err.kind() == io::ErrorKind::NotFound => Ok(false),
Err(err) => Err(Error::new(ErrorKind::InvalidOperation, err.to_string())),
}
}
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| use std::{fs, io}; | ||
|
|
||
| /// Register standard library helpers with the Jinja environment. | ||
| pub fn register(env: &mut Environment<'_>) { |
There was a problem hiding this comment.
issue (complexity): Consider refactoring the repeated test registration logic into a loop over a static list of test names and predicates.
use minijinja::{Environment, Error, ErrorKind};
use std::{fs, io};
use std::os::unix::fs::{FileTypeExt, FileType};
/// Register standard library helpers with the Jinja environment.
pub fn register(env: &mut Environment<'_>) {
// a static list of (test_name, predicate) pairs
const TESTS: &[(&str, fn(FileType) -> bool)] = &[
("dir", FileType::is_dir),
("file", FileType::is_file),
("symlink", FileTypeExt::is_symlink),
("pipe", FileTypeExt::is_fifo),
("device", |ft| ft.is_block_device() || ft.is_char_device()),
];
for (name, pred) in TESTS {
// call the same helper with each predicate
env.add_test(*name, move |path: String| is_file_type(&path, *pred));
}
}
/// Determine whether `path` matches the given file type predicate.
///
/// Returns `Ok(false)` if the path does not exist.
fn is_file_type<F>(path: &str, predicate: F) -> Result<bool, Error>
where
F: Fn(fs::FileType) -> bool,
{
match fs::symlink_metadata(path) {
Ok(md) => Ok(predicate(md.file_type())),
Err(err) if err.kind() == io::ErrorKind::NotFound => Ok(false),
Err(err) => Err(Error::new(ErrorKind::InvalidOperation, err.to_string())),
}
}There was a problem hiding this comment.
Has this now been resolved?
There was a problem hiding this comment.
Yes, this has been resolved! The current implementation in src/stdlib.rs now uses a static TESTS array containing the test names and their corresponding predicates, and registers them in a loop:
const TESTS: &[FileTest] = &[
("dir", is_dir),
("file", is_file),
("symlink", is_symlink),
// ...
];
for (name, pred) in TESTS {
env.add_test(*name, move |path: String| {
is_file_type(Utf8Path::new(&path), *pred)
});
}This matches the refactoring suggestion and removes the previous repetition. Let me know if you need any further details!
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
Codex Review: Here are some suggestions.
Reply with @codex fix comments to fix any unresolved comments.
About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you open a pull request for review, mark a draft as ready, or comment "@codex review". If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex fix this CI failure" or "@codex address that feedback".
| use minijinja::{Environment, Error, ErrorKind}; | ||
| use std::os::unix::fs::FileTypeExt; | ||
| use std::{fs, io}; | ||
|
|
There was a problem hiding this comment.
[P1] Gate Unix-specific file tests for Windows compatibility
This module imports std::os::unix::fs::FileTypeExt and registers pipe/device tests without any #[cfg(unix)] guarding. On Windows the std::os::unix namespace is absent, so the crate fails to build before the tests run. Other Unix-only code in the project is conditionally compiled; these helpers should be similarly gated or given cross-platform fallbacks so Windows builds continue to succeed.
Useful? React with 👍 / 👎.
| use crate::CliWorld; | ||
| use cucumber::given; | ||
| use nix::{sys::stat::Mode, unistd::mkfifo}; | ||
| use std::{os::unix::fs as unix_fs, path::PathBuf}; | ||
| use test_support::env::set_var; |
There was a problem hiding this comment.
[P1] Restrict Unix-only Cucumber steps
The file-system step definitions unconditionally use nix::unistd::mkfifo and std::os::unix::fs, but the test harness is compiled on every target. Neither of those modules exists on Windows and the nix dev-dependency does not compile there, so cargo test will fail outright. These steps and their module inclusion should be wrapped in #[cfg(unix)] (or an alternative implementation provided) to avoid breaking non-Unix CI.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 14
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
docs/srgn.md (1)
691-713: Convert reference list to GFM footnotes and remove placeholder dates
- Replace the reference list in docs/srgn.md (lines 691–713) with GitHub-Flavoured Markdown footnotes and drop entries using “accessed on January 1, 1970”.
- Run markdownlint to confirm no lint errors:
make markdownlinttests/steps/fs_steps.rs (1)
1-33: Align CI with Unix-only scope.Restrict the test matrix to Unix runners or exclude Windows for jobs that compile test steps, to avoid spurious failures.
Add or adjust your GitHub Actions job:
jobs: test: runs-on: ubuntu-latest strategy: fail-fast: false matrix: os: [ubuntu-latest] # exclude windows/macos if stdlib uses Unix-only tests steps: - uses: actions/checkout@v4 - uses: dtolnay/rust-toolchain@stable - run: cargo test --all-features
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (11)
Cargo.toml(1 hunks)docs/netsuke-design.md(8 hunks)docs/roadmap.md(1 hunks)docs/srgn.md(3 hunks)src/lib.rs(1 hunks)src/manifest.rs(1 hunks)src/stdlib.rs(1 hunks)tests/data/jinja_is.yml(1 hunks)tests/features/fs_tests.feature(1 hunks)tests/steps/fs_steps.rs(1 hunks)tests/steps/mod.rs(1 hunks)
🧰 Additional context used
📓 Path-based instructions (7)
**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
**/*.rs: Comment why, not what; document assumptions, edge cases, trade-offs, and complexity without restating obvious code
Extract reusable logic into functions; prefer composition and declarative constructs when readable
Keep functions small, single-responsibility, and observe command/query separation
Use precise, descriptive names; boolean names should start with is/has/should
Use en-GB-oxendict spelling/grammar in comments (except external API references)
Function docs must include clear usage examples; avoid redundant examples in test docs
No Rust source file may exceed 400 lines; split long switches/tables; move large test data to external files
Fix test warnings in code; do not silence them
Extract helpers when functions are too long; group many parameters into well-named structs
Consider using Arc when returning large errors to reduce data movement
Document public APIs using Rustdoc (///) so cargo doc can generate documentation
Prefer immutable data; avoid unnecessary mut bindings
Prefer Result-based error handling over panicking where feasible
Avoid unsafe code unless absolutely necessary and document any usage clearly
Place function attributes after doc comments
Do not use return in single-line functions
Use predicate helper functions when conditionals have more than two branches
Do not silence lints except as a last resort; suppressions must be tightly scoped and include a clear reason
Prefer #[expect(...)] over #[allow(...)] for lint management
Prefer .expect() over .unwrap()
Use conditional compilation (#[cfg]/#[cfg_attr]) for functions unused under specific feature sets
Use concat!() to join long string literals rather than using backslash-newline escapes
Prefer single-line function bodies where appropriate (e.g., pub fn new(id: u64) -> Self { Self(id) })
Prefer semantic error enums deriving std::error::Error via thiserror for caller-inspectable conditions
Files:
src/manifest.rssrc/lib.rstests/steps/mod.rssrc/stdlib.rstests/steps/fs_steps.rs
⚙️ CodeRabbit configuration file
**/*.rs: * Seek to keep the cyclomatic complexity of functions no more than 12.
Adhere to single responsibility and CQRS
Place function attributes after doc comments.
Do not use
returnin single-line functions.Move conditionals with >2 branches into a predicate function.
Avoid
unsafeunless absolutely necessary.Every module must begin with a
//!doc comment that explains the module's purpose and utility.Comments and docs must follow en-GB-oxendict (-ize / -our) spelling and grammar
Lints must not be silenced except as a last resort.
#[allow]is forbidden.- Only narrowly scoped
#[expect(lint, reason = "...")]is allowed.- No lint groups, no blanket or file-wide suppression.
- Include
FIXME:with link if a fix is expected.Where code is only used by specific features, it must be conditionally compiled or a conditional expectation for unused_code applied.
Use
rstestfixtures for shared setup and to avoid repetition between tests.Replace duplicated tests with
#[rstest(...)]parameterised cases.Prefer
mockallfor mocks/stubs.Prefer
.expect()over.unwrap()Ensure that any API or behavioural changes are reflected in the documentation in
docs/Ensure that any completed roadmap steps are recorded in the appropriate roadmap in
docs/Files must not exceed 400 lines in length
- Large modules must be decomposed
- Long match statements or dispatch tables should be decomposed by domain and collocated with targets
- Large blocks of inline data (e.g., test fixtures, constants or templates) must be moved to external files and inlined at compile-time or loaded at run-time.
Environment access (env::set_var and env::remove_var) are always unsafe in Rust 2024 and MUST be marked as such
- For testing of functionality depending upon environment variables, dependency injection and the
mockablecrate are the preferred option.- If mockable cannot be used, env mutations in tests ...
Files:
src/manifest.rssrc/lib.rstests/steps/mod.rssrc/stdlib.rstests/steps/fs_steps.rs
src/**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
src/**/*.rs: Every module must begin with a //! module-level comment explaining its purpose and utility
Never export eyre::Report from library code; convert to domain error enums at API boundaries
Files:
src/manifest.rssrc/lib.rssrc/stdlib.rs
tests/**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
tests/**/*.rs: Use rstest fixtures for shared setup and replace duplicated tests with #[rstest(...)] parameterised cases
Prefer mockall for mocks/stubs
Mock non-deterministic dependencies via dependency injection (e.g., Env/Clock traits) using mockable crate; follow docs/reliable-testing-in-rust-via-dependency-injection.md
Files:
tests/steps/mod.rstests/steps/fs_steps.rs
docs/**/*.md
📄 CodeRabbit inference engine (AGENTS.md)
docs/**/*.md: Treat docs/ markdown as the reference source of truth for requirements and decisions; keep updated with changes
Use en-GB-oxendict spelling/grammar in documentation (LICENSE name unchanged)
Files:
docs/roadmap.mddocs/netsuke-design.mddocs/srgn.md
**/*.md
📄 CodeRabbit inference engine (AGENTS.md)
**/*.md: Validate Markdown with make markdownlint
Wrap Markdown paragraphs and bullet points at 80 columns; do not wrap tables or headings
Wrap code blocks in Markdown at 120 columns
Use dashes (-) for list bullets
Use GitHub-flavoured Markdown footnotes ([^1]) for references
Validate Mermaid diagrams in Markdown using make nixie
Files:
docs/roadmap.mddocs/netsuke-design.mddocs/srgn.md
⚙️ CodeRabbit configuration file
**/*.md: * Avoid 2nd person or 1st person pronouns ("I", "you", "we")
- Use en-GB-oxendict (-ize / -our) spelling and grammar
- Headings must not be wrapped.
- Documents must start with a level 1 heading
- Headings must correctly increase or decrease by no more than one level at a time
- Use GitHub-flavoured Markdown style for footnotes and endnotes.
- Numbered footnotes must be numbered by order of appearance in the document.
Files:
docs/roadmap.mddocs/netsuke-design.mddocs/srgn.md
docs/**/*.{rs,md}
📄 CodeRabbit inference engine (docs/rust-doctest-dry-guide.md)
In fenced code blocks for docs, explicitly mark code fences with rust (```rust) for clarity
Files:
docs/roadmap.mddocs/netsuke-design.mddocs/srgn.md
Cargo.toml
📄 CodeRabbit inference engine (AGENTS.md)
Cargo.toml: Use explicit version ranges and keep dependencies up to date
Mandate SemVer caret requirements for all dependencies (e.g., "1.2.3")
Prohibit wildcard (*) and open-ended (>=) version specifiers; use ~ only with a specific, documented reason
Files:
Cargo.toml
🧬 Code graph analysis (1)
src/manifest.rs (1)
src/stdlib.rs (1)
register(6-20)
🔍 Remote MCP
Relevant points for review:
-
The new tests in
src/stdlib.rsandtests/steps/fs_steps.rsusestd::os::unix::fs::FileTypeExtto detect special file types (FIFO, socket, block/char devices). That trait is available only on Unix platforms, so these tests (and the addedregistercode that callsis_file_type) will fail to compile on Windows unless they’re gated behind#[cfg(unix)]or the CI matrix is restricted to Unix runners . -
The FIFO creation in the test fixture uses
nix::unistd::mkfifo, which is provided only when thefsfeature of thenixcrate is enabled. The PR correctly addsnix = { version = "0.30.1", default-features = false, features = ["fs"] }, satisfying this requirement . -
Cucumber feature tests refer to
/dev/nullfor device detection; this path and the Unix-specific APIs will not exist or behave the same on Windows, causing functional test failures on non-Unix CI runners.
Recommendations:
-
Add
#[cfg(unix)]to the Unix-specific modules/tests (or isolate them into a Unix-only test suite). -
Ensure the GitHub Actions (or other CI) workflow either excludes Windows runners or accounts for these OS-specific tests.
⏰ 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). (1)
- GitHub Check: build-test (stable)
🔇 Additional comments (6)
docs/roadmap.md (1)
157-158: LGTM: Roadmap accurately marks the basic file-system tests as done.src/lib.rs (1)
15-15: LGTM: Expose stdlib publicly.Public surface matches intended registration in manifest processing.
src/manifest.rs (1)
541-541: Verify Unix API gating and Windows build
Verify manually that all Unix-specific stdlib calls are gated bycfg(unix)and confirm successful Windows compilation.tests/data/jinja_is.yml (2)
3-22: LGTM: concise test data covers the five file-type predicates.Targets are minimal and focused; commands are harmless; expressions exercise
dir,file,symlink,pipe, anddevice.
1-1: Confirm netsuke_version "0.1.0" remains valid
The parser deserialises netsuke_version using semver::Version, which accepts “0.1.0” without error, so the Jinja tests will execute as intended.tests/steps/fs_steps.rs (1)
20-30: Environment lock and cleanup confirmed
set_varacquires the globalEnvLockto serialise mutations andCliWorld’sDropinvokesrestore_manyto revert all saved variables, preventing cross-scenario leakage.
tests/features/fs_tests.feature
Outdated
| Scenario: file system tests detect path types | ||
| Given a file-type test workspace | ||
| When the manifest file "tests/data/jinja_is.yml" is parsed | ||
| Then the manifest has targets 5 |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Assert target identities, not just the count.
Validate the resolved target names to catch false positives where unrelated targets inflate the count. If the step library lacks such an assertion, add a “Then the manifest has targets named …” step.
🤖 Prompt for AI Agents
In tests/features/fs_tests.feature around lines 2 to 5, the scenario only
asserts the manifest target count which can miss false positives; update the
feature to assert specific target identities by adding a step like "Then the
manifest has targets named ..." with the expected list (e.g. the five exact
target names), and if the step definition is missing implement a step in the
step library that parses the manifest result and compares the set/order of
target names to the expected values, failing on any mismatch.
| Scenario: file system tests return false for missing paths | ||
| Given a file-type test workspace | ||
| And the environment variable "DIR_PATH" is set to "/no/such" | ||
| And the environment variable "FILE_PATH" is set to "/no/such" | ||
| And the environment variable "SYMLINK_PATH" is set to "/no/such" | ||
| And the environment variable "PIPE_PATH" is set to "/no/such" | ||
| And the environment variable "DEVICE_PATH" is set to "/no/such" | ||
| When the manifest file "tests/data/jinja_is.yml" is parsed | ||
| Then the manifest has targets 0 |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Make the “missing path” assertions OS-neutral.
Avoid hard-coding /no/such. Drive non-existence via the workspace (e.g., set each var to <workspace>/__missing__/<type> and ensure it does not exist in the fixture step), making behaviour consistent across platforms.
🤖 Prompt for AI Agents
In tests/features/fs_tests.feature around lines 7 to 15, the scenario hard-codes
non-existent paths as "/no/such" which is platform-dependent; change each
environment variable assignment to point inside the test workspace (e.g., set
"DIR_PATH" to "<workspace>/__missing__/dir", "FILE_PATH" to
"<workspace>/__missing__/file", "SYMLINK_PATH" to
"<workspace>/__missing__/symlink", "PIPE_PATH" to
"<workspace>/__missing__/pipe", "DEVICE_PATH" to
"<workspace>/__missing__/device") and update the "Given a file-type test
workspace" fixture to ensure those <workspace>/__missing__/* entries do not
exist (remove or never create them) so the manifest parsing asserts missing
paths in an OS-neutral way.
| let temp = tempfile::tempdir().expect("tempdir"); | ||
| let dir = temp.path().join("dir"); | ||
| std::fs::create_dir(&dir).expect("dir"); | ||
| let file = temp.path().join("file"); | ||
| std::fs::write(&file, b"data").expect("file"); | ||
| let link = temp.path().join("link"); | ||
| unix_fs::symlink(&file, &link).expect("symlink"); | ||
| let pipe = temp.path().join("pipe"); | ||
| mkfifo(&pipe, Mode::S_IRUSR | Mode::S_IWUSR).expect("fifo"); | ||
| let entries = [ |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Improve .expect() messages for faster failure diagnosis.
Make failure causes explicit to speed up triage.
Apply this diff:
- let temp = tempfile::tempdir().expect("tempdir");
+ let temp = tempfile::tempdir().expect("create tempdir for file-type workspace");
- let dir = temp.path().join("dir");
- std::fs::create_dir(&dir).expect("dir");
+ let dir = temp.path().join("dir");
+ std::fs::create_dir(&dir).expect("create dir fixture");
- let file = temp.path().join("file");
- std::fs::write(&file, b"data").expect("file");
+ let file = temp.path().join("file");
+ std::fs::write(&file, b"data").expect("write file fixture");
- let link = temp.path().join("link");
- unix_fs::symlink(&file, &link).expect("symlink");
+ let link = temp.path().join("link");
+ unix_fs::symlink(&file, &link).expect("create symlink fixture");
- let pipe = temp.path().join("pipe");
- mkfifo(&pipe, Mode::S_IRUSR | Mode::S_IWUSR).expect("fifo");
+ let pipe = temp.path().join("pipe");
+ mkfifo(&pipe, Mode::S_IRUSR | Mode::S_IWUSR).expect("create fifo fixture");📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| let temp = tempfile::tempdir().expect("tempdir"); | |
| let dir = temp.path().join("dir"); | |
| std::fs::create_dir(&dir).expect("dir"); | |
| let file = temp.path().join("file"); | |
| std::fs::write(&file, b"data").expect("file"); | |
| let link = temp.path().join("link"); | |
| unix_fs::symlink(&file, &link).expect("symlink"); | |
| let pipe = temp.path().join("pipe"); | |
| mkfifo(&pipe, Mode::S_IRUSR | Mode::S_IWUSR).expect("fifo"); | |
| let entries = [ | |
| let temp = tempfile::tempdir().expect("create tempdir for file-type workspace"); | |
| let dir = temp.path().join("dir"); | |
| std::fs::create_dir(&dir).expect("create dir fixture"); | |
| let file = temp.path().join("file"); | |
| std::fs::write(&file, b"data").expect("write file fixture"); | |
| let link = temp.path().join("link"); | |
| unix_fs::symlink(&file, &link).expect("create symlink fixture"); | |
| let pipe = temp.path().join("pipe"); | |
| mkfifo(&pipe, Mode::S_IRUSR | Mode::S_IWUSR).expect("create fifo fixture"); | |
| let entries = [ |
🤖 Prompt for AI Agents
In tests/steps/fs_steps.rs around lines 11 to 20, the chained .expect(...) calls
use generic messages like "tempdir", "dir", "file", "symlink", and "fifo" which
hinder quick diagnosis; update each .expect() to include a clear, specific
message that describes the attempted operation and the path/variable involved
(e.g., "creating tempdir", "creating dir at {dir:?}", "writing file at
{file:?}", "creating symlink from {file:?} to {link:?}", "creating fifo at
{pipe:?}") so test failures show actionable context.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (4)
src/stdlib.rs (4)
6-20: Gate Unix-only tests and provide cross-platform stubs.The
pipeanddevicetests use Unix-onlyFileTypeExtmethods that don't exist on Windows. Register them conditionally to maintain template compatibility across platforms.Apply this diff:
pub fn register(env: &mut Environment<'_>) { env.add_test("dir", |path: String| is_file_type(&path, |ft| ft.is_dir())); env.add_test("file", |path: String| { is_file_type(&path, |ft| ft.is_file()) }); env.add_test("symlink", |path: String| { is_file_type(&path, |ft| ft.is_symlink()) }); - env.add_test("pipe", |path: String| { - is_file_type(&path, |ft| ft.is_fifo()) - }); - env.add_test("device", |path: String| { - is_file_type(&path, |ft| ft.is_block_device() || ft.is_char_device()) - }); + + // Unix-only: FIFO and block/char devices + #[cfg(unix)] + { + env.add_test("pipe", |path: String| is_file_type(&path, |ft| ft.is_fifo())); + env.add_test("device", |path: String| { + is_file_type(&path, |ft| ft.is_block_device() || ft.is_char_device()) + }); + } + + // Non-Unix: expose tests but always return false + #[cfg(not(unix))] + { + env.add_test("pipe", |_path: String| Ok(false)); + env.add_test("device", |_path: String| Ok(false)); + } }
37-37: Gate test module for Unix platforms only.The test module uses Unix-specific features (FIFOs,
/dev/null) and will fail to compile on Windows.Apply this diff:
-#[cfg(test)] +#[cfg(all(test, unix))]
25-25: Use generic predicate parameter for flexibility.The concrete function pointer restricts callers to function pointers only. A generic parameter allows closures without coercion.
Apply this diff:
-fn is_file_type(path: &str, predicate: fn(fs::FileType) -> bool) -> Result<bool, Error> { +fn is_file_type<F>(path: &str, predicate: F) -> Result<bool, Error> +where + F: Fn(fs::FileType) -> bool, +{
42-42: Remove redundant FileTypeExt import in tests.This import duplicates line 2 and creates an unused import warning when the module import is conditionally compiled.
Apply this diff:
- use std::os::unix::fs::FileTypeExt;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
src/stdlib.rs(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
**/*.rs: Comment why, not what; document assumptions, edge cases, trade-offs, and complexity without restating obvious code
Extract reusable logic into functions; prefer composition and declarative constructs when readable
Keep functions small, single-responsibility, and observe command/query separation
Use precise, descriptive names; boolean names should start with is/has/should
Use en-GB-oxendict spelling/grammar in comments (except external API references)
Function docs must include clear usage examples; avoid redundant examples in test docs
No Rust source file may exceed 400 lines; split long switches/tables; move large test data to external files
Fix test warnings in code; do not silence them
Extract helpers when functions are too long; group many parameters into well-named structs
Consider using Arc when returning large errors to reduce data movement
Document public APIs using Rustdoc (///) so cargo doc can generate documentation
Prefer immutable data; avoid unnecessary mut bindings
Prefer Result-based error handling over panicking where feasible
Avoid unsafe code unless absolutely necessary and document any usage clearly
Place function attributes after doc comments
Do not use return in single-line functions
Use predicate helper functions when conditionals have more than two branches
Do not silence lints except as a last resort; suppressions must be tightly scoped and include a clear reason
Prefer #[expect(...)] over #[allow(...)] for lint management
Prefer .expect() over .unwrap()
Use conditional compilation (#[cfg]/#[cfg_attr]) for functions unused under specific feature sets
Use concat!() to join long string literals rather than using backslash-newline escapes
Prefer single-line function bodies where appropriate (e.g., pub fn new(id: u64) -> Self { Self(id) })
Prefer semantic error enums deriving std::error::Error via thiserror for caller-inspectable conditions
Files:
src/stdlib.rs
⚙️ CodeRabbit configuration file
**/*.rs: * Seek to keep the cyclomatic complexity of functions no more than 12.
Adhere to single responsibility and CQRS
Place function attributes after doc comments.
Do not use
returnin single-line functions.Move conditionals with >2 branches into a predicate function.
Avoid
unsafeunless absolutely necessary.Every module must begin with a
//!doc comment that explains the module's purpose and utility.Comments and docs must follow en-GB-oxendict (-ize / -our) spelling and grammar
Lints must not be silenced except as a last resort.
#[allow]is forbidden.- Only narrowly scoped
#[expect(lint, reason = "...")]is allowed.- No lint groups, no blanket or file-wide suppression.
- Include
FIXME:with link if a fix is expected.Where code is only used by specific features, it must be conditionally compiled or a conditional expectation for unused_code applied.
Use
rstestfixtures for shared setup and to avoid repetition between tests.Replace duplicated tests with
#[rstest(...)]parameterised cases.Prefer
mockallfor mocks/stubs.Prefer
.expect()over.unwrap()Ensure that any API or behavioural changes are reflected in the documentation in
docs/Ensure that any completed roadmap steps are recorded in the appropriate roadmap in
docs/Files must not exceed 400 lines in length
- Large modules must be decomposed
- Long match statements or dispatch tables should be decomposed by domain and collocated with targets
- Large blocks of inline data (e.g., test fixtures, constants or templates) must be moved to external files and inlined at compile-time or loaded at run-time.
Environment access (env::set_var and env::remove_var) are always unsafe in Rust 2024 and MUST be marked as such
- For testing of functionality depending upon environment variables, dependency injection and the
mockablecrate are the preferred option.- If mockable cannot be used, env mutations in tests ...
Files:
src/stdlib.rs
src/**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
src/**/*.rs: Every module must begin with a //! module-level comment explaining its purpose and utility
Never export eyre::Report from library code; convert to domain error enums at API boundaries
Files:
src/stdlib.rs
🔍 Remote MCP
Additional context for reviewing PR #151
-
nix 0.30.1 features
Thefsfeature in nix 0.30.1 is empty and not enabled by default; enabling it does not pull in unrelated modules. Explicitly specifyingnix = { version = "0.30.1", default-features = false, features = ["fs"] }
in
[dev-dependencies]correctly gates only the filesystem APIs needed for tests, without extra dependencies (docs.rs). -
mkfifo availability
The functionpub fn mkfifo<P: ?Sized + NixPath>(path: &P, mode: Mode) -> Result<()>
lives in
nix::unistdand is only available when the crate featurefsis enabled. This matches the PR’s use ofnix::unistd::mkfifo(...)to create named pipes in tests (docs.rs). -
Unix file‐type detection (
FileTypeExt)
On Unix,std::fs::symlink_metadatareturns aFileTypethat implementsstd::os::unix::fs::FileTypeExt, which provides:is_block_device()is_char_device()is_fifo()is_socket()
The PR’s helper
is_file_typecan rely on these methods to detect pipes (is_fifo), devices (is_char_device), symlinks, directories, and files (web.mit.edu). -
MiniJinja custom tests registration
According to MiniJinja docs, tests are registered on anEnvironmentviaenv.add_test("name", test_fn);
where
test_fnreturnsbool. The PR’sstdlib::registerfunction adding testsdir,file,symlink,pipe, anddevicefollows this documented pattern for custom “is” tests (docs.rs).
⏰ 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). (1)
- GitHub Check: build-test (stable)
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Summary
istests for file-system types and register them in the template standard libraryrstestunit tests and Cucumber featuresTesting
make fmtmake lintmake testmake markdownlintmake nixie(fails: KeyboardInterrupt)https://chatgpt.com/codex/tasks/task_e_68c0a5d63d1483228887d76074c863ad
Summary by Sourcery
Add and register file-system type tests in the Jinja template standard library and update related docs, tests, and dependencies
New Features:
istests fordir,file,symlink,pipe, anddeviceEnhancements:
Build:
fsfeatureDocumentation:
Tests: