Conversation
Reviewer's GuideThis PR adds dedicated file-system Jinja tests by introducing a standard-library module that registers type predicates in a table-driven loop (using cap-std and camino), integrates these tests into the template environment, and exercises them in new Unix-gated Cucumber scenarios. It also updates documentation with footnotes, removes placeholder dates, restricts the CI matrix to Linux, and extends the test harness to assert multiple target names for cross-platform consistency. Class diagram for new stdlib file-system test registrationclassDiagram
class Environment {
+add_test(name: &str, fn(String) -> Result<bool, Error>)
}
class Stdlib {
+register(env: &mut Environment)
+is_file_type(path: &Utf8Path, predicate: fn(FileType) -> bool) -> Result<bool, Error>
}
class FileType {
+is_dir() bool
+is_file() bool
+is_symlink() bool
+is_fifo() bool
+is_block_device() bool
+is_char_device() bool
}
class Dir {
+symlink_metadata(path: &Utf8Path) -> Metadata
}
class Utf8Path {
+parent() -> Option<Utf8Path>
+file_name() -> Option<&str>
}
Stdlib --> Environment : registers tests
Stdlib --> FileType : uses predicates
Stdlib --> Dir : uses for metadata
Stdlib --> Utf8Path : path handling
Class diagram for new file-system Jinja testsclassDiagram
class FileTest {
+name: &'static str
+predicate: fn(FileType) -> bool
}
class Stdlib {
+register(env: &mut Environment)
}
class Environment {
+add_test(name: &str, fn(String) -> Result<bool, Error>)
}
FileTest <.. Stdlib : used in registration
Stdlib --> Environment : adds tests
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
Note Reviews pausedUse the following commands to manage reviews:
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. Summary by CodeRabbit
WalkthroughAdd a public Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor CLI as CLI
participant Manifest as manifest::from_str_named
participant Jinja as Minijinja::Environment
participant Stdlib as stdlib::register
participant FS as cap_std::fs
CLI->>Manifest: Parse manifest string
Manifest->>Jinja: Build template environment
Manifest->>Stdlib: register(&mut env)
Stdlib-->>Jinja: Add tests (dir,file,symlink,pipe,...)
Manifest-->>CLI: Return parsed manifest
rect rgb(247,250,255)
note right of Jinja: When evaluating a `when` expression
CLI->>Jinja: Evaluate "{{ env('PATH') is file_type }}"
Jinja->>Stdlib: Invoke test predicate with path
Stdlib->>FS: symlink_metadata(path)
FS-->>Stdlib: FileType or error
Stdlib-->>Jinja: bool (false if missing) or templating error
Jinja-->>CLI: Condition result
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Pre-merge checks (3 passed)✅ Passed checks (3 passed)
Poem
Pre-merge checks (3 passed)✅ Passed checks (3 passed)
Poem
Pre-merge checks (3 passed)✅ Passed checks (3 passed)
Poem
Pre-merge checks (3 passed)✅ Passed checks (3 passed)
Poem
Pre-merge checks (3 passed)✅ Passed checks (3 passed)
Poem
Pre-merge checks (3 passed)✅ Passed checks (3 passed)
Poem
Pre-merge checks (3 passed)✅ Passed checks (3 passed)
Poem
Pre-merge checks (3 passed)✅ Passed checks (3 passed)
Poem
Pre-merge checks (3 passed)✅ Passed checks (3 passed)
Poem
Pre-merge checks (3 passed)✅ Passed checks (3 passed)
Poem
Pre-merge checks (3 passed)✅ Passed checks (3 passed)
Poem
Pre-merge checks (3 passed)✅ Passed checks (3 passed)
Poem
Pre-merge checks (3 passed)✅ Passed checks (3 passed)
Poem
Pre-merge checks (3 passed)✅ Passed checks (3 passed)
Poem
Pre-merge checks (3 passed)✅ Passed checks (3 passed)
Poem
Pre-merge checks (3 passed)✅ Passed checks (3 passed)
Poem
Pre-merge checks (3 passed)✅ Passed checks (3 passed)
Poem
Pre-merge checks (3 passed)✅ Passed checks (3 passed)
Poem
Pre-merge checks (3 passed)✅ Passed checks (3 passed)
Poem
Pre-merge checks (3 passed)✅ Passed checks (3 passed)
Poem
Pre-merge checks (3 passed)✅ Passed checks (3 passed)
Poem
Pre-merge checks (3 passed)✅ Passed checks (3 passed)
Poem
Pre-merge checks (3 passed)✅ Passed checks (3 passed)
Poem
Pre-merge checks (3 passed)✅ Passed checks (3 passed)
Poem
Pre-merge checks (3 passed)✅ Passed checks (3 passed)
Poem
Pre-merge checks (3 passed)✅ Passed checks (3 passed)
Poem
Pre-merge checks (3 passed)✅ Passed checks (3 passed)
Poem
Pre-merge checks (3 passed)✅ Passed checks (3 passed)
Poem
Pre-merge checks (3 passed)✅ Passed checks (3 passed)
Poem
Pre-merge checks (3 passed)✅ Passed checks (3 passed)
Poem
Pre-merge checks (3 passed)✅ Passed checks (3 passed)
Poem
Pre-merge checks (3 passed)✅ Passed checks (3 passed)
Poem
Pre-merge checks (3 passed)✅ Passed checks (3 passed)
Poem
Pre-merge checks (3 passed)✅ Passed checks (3 passed)
Poem
Pre-merge checks (3 passed)✅ Passed checks (3 passed)
Poem
Pre-merge checks (3 passed)✅ Passed checks (3 passed)
Poem
Pre-merge checks (3 passed)✅ Passed checks (3 passed)
Poem
Pre-merge checks (3 passed)✅ Passed checks (3 passed)
Poem
Pre-merge checks (3 passed)✅ Passed checks (3 passed)
Poem
Pre-merge checks (3 passed)✅ Passed checks (3 passed)
Poem
Pre-merge checks (3 passed)✅ Passed checks (3 passed)
Poem
Pre-merge checks (3 passed)✅ Passed checks (3 passed)
Poem
Pre-merge checks (3 passed)✅ Passed checks (3 passed)
Poem
Pre-merge checks (3 passed)✅ Passed checks (3 passed)
Poem
Pre-merge checks (3 passed)✅ Passed checks (3 passed)
Poem
Pre-merge checks (3 passed)✅ Passed checks (3 passed)
Poem
Pre-merge checks (3 passed)✅ Passed checks (3 passed)
Poem
Pre-merge checks (3 passed)✅ Passed checks (3 passed)
Poem
Pre-merge checks (3 passed)✅ Passed checks (3 passed)
Poem
Pre-merge checks (3 passed)✅ Passed checks (3 passed)
Poem
Pre-merge checks (3 passed)✅ Passed checks (3 passed)
Poem
Pre-merge checks (3 passed)✅ Passed checks (3 passed)
Poem
Pre-merge checks (3 passed)✅ Passed checks (3 passed)
Poem
Pre-merge checks (3 passed)✅ Passed checks (3 passed)
Poem
Pre-merge checks (3 passed)✅ Passed checks (3 passed)
Poem
Pre-merge checks (3 passed)✅ Passed checks (3 passed)
Poem
Pre-merge checks (3 passed)✅ Passed checks (3 passed)
Poem
Pre-merge checks (3 passed)✅ Passed checks (3 passed)
Poem
Pre-merge checks (3 passed)✅ Passed checks (3 passed)
Poem
Pre-merge checks (3 passed)✅ Passed checks (3 passed)
Poem
Pre-merge checks (3 passed)✅ Passed checks (3 passed)
Poem
Pre-merge checks (3 passed)✅ Passed checks (3 passed)
Poem
Pre-merge checks (3 passed)✅ Passed checks (3 passed)
Poem
Pre-merge checks (3 passed)✅ Passed checks (3 passed)
Poem
Pre-merge checks (3 passed)✅ Passed checks (3 passed)
Comment |
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".
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 (1)
docs/srgn.md (1)
83-116: Avoid 2nd-person pronouns per docs style guide.
Rewrite instructional sentences to remove “you/your”. Multiple instances exist in installation and recipes sections.Offer to generate a patch that rewrites affected sentences while preserving meaning. Do you want an automated edit across the file?
Also applies to: 145-149, 411-416, 441-443, 523-527
📜 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 (14)
.github/workflows/ci.yml(1 hunks)Cargo.toml(2 hunks)docs/netsuke-design.md(8 hunks)docs/roadmap.md(1 hunks)docs/srgn.md(2 hunks)src/lib.rs(1 hunks)src/manifest.rs(1 hunks)src/stdlib.rs(1 hunks)tests/cucumber.rs(1 hunks)tests/data/jinja_is.yml(1 hunks)tests/features_unix/fs_tests.feature(1 hunks)tests/steps/fs_steps.rs(1 hunks)tests/steps/manifest_steps.rs(1 hunks)tests/steps/mod.rs(1 hunks)
🧰 Additional context used
📓 Path-based instructions (6)
**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
**/*.rs: Use en-GB-oxendict spelling and grammar in code comments (exceptions allowed for external API names)
Function documentation must include clear examples; test documentation should omit redundant examples
Keep file size manageable: no Rust source file longer than 400 lines; split large switches/dispatch tables and move big test data to external files
Disallow Clippy warnings; fix warnings in code rather than silencing them
Extract helper functions for long functions; maintain separation of concerns and CQRS
Group many related parameters into meaningful structs
If returning a large error type, consider using Arc to reduce data copied/returned
Each Rust module must begin with a module-level //! comment stating purpose and utility
Document public APIs with Rustdoc (///) so cargo doc can generate documentation
Prefer immutable data; avoid unnecessary mut bindings
Handle errors with Result instead of 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 functions when conditional criteria 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
Use conditional compilation (#[cfg]/#[cfg_attr]) for functions unused under specific feature sets
Prefer .expect() over .unwrap()
Use concat!() for long string literals instead of escaping newlines with backslashes
Prefer single-line function bodies when appropriate (e.g., pub fn new(id: u64) -> Self { Self(id) })
Prefer semantic error enums (derive std::error::Error via thiserror) for inspectable conditions
Use an opaque eyre::Report only at the application boundary for human-readable logs; do not expose in public APIs
Never export eyre::Report from libraries; convert to domain error enums at API boundaries and to eyre only in main/top-level async tas...
Files:
src/lib.rstests/cucumber.rssrc/manifest.rstests/steps/manifest_steps.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/lib.rstests/cucumber.rssrc/manifest.rstests/steps/manifest_steps.rstests/steps/mod.rssrc/stdlib.rstests/steps/fs_steps.rs
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/srgn.mddocs/netsuke-design.md
docs/**/*.md
📄 CodeRabbit inference engine (AGENTS.md)
docs/**/*.md: Use docs/ as the knowledge base and source of truth for requirements, dependencies, and architecture
Proactively update docs/ when decisions, requirements, libraries, or architecture change to keep documentation current
Documentation must use en-GB-oxendict spelling and grammar (LICENSE name unchanged)
Files:
docs/roadmap.mddocs/srgn.mddocs/netsuke-design.md
**/*.md
📄 CodeRabbit inference engine (AGENTS.md)
**/*.md: Validate Markdown with make markdownlint
Run make fmt after documentation changes to format Markdown and fix table markup
Validate Mermaid diagrams in Markdown files using make nixie
Wrap Markdown paragraphs and bullet points at 80 columns; code blocks at 120; do not wrap tables or headings
Use dashes (-) for list bullets in Markdown
Use GitHub-flavoured Markdown footnotes ([^1]) for references
Files:
docs/roadmap.mddocs/srgn.mddocs/netsuke-design.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/srgn.mddocs/netsuke-design.md
tests/**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
tests/**/*.rs: Write unit and behavioural tests for new functionality; run before and after changes
Use rstest fixtures for shared setup and replace duplicated tests with #[rstest(...)] parameterised cases
Prefer mockall for mocks/stubs
Mock non-deterministic dependencies via dependency injection using mockable (traits like Env and Clock) where appropriate; follow docs/reliable-testing-in-rust-via-dependency-injection.md
Files:
tests/cucumber.rstests/steps/manifest_steps.rstests/steps/mod.rstests/steps/fs_steps.rs
Cargo.toml
📄 CodeRabbit inference engine (AGENTS.md)
Cargo.toml: Use explicit SemVer caret version requirements for dependencies; keep dependencies up-to-date
Mandate caret requirements for all dependencies (e.g., some-crate = "1.2.3")
Prohibit wildcard (*) and open-ended (>=) version requirements; use ~ only with a specific documented reason
Files:
Cargo.toml
🧬 Code graph analysis (1)
src/manifest.rs (1)
src/stdlib.rs (1)
register(37-58)
🔍 Remote MCP
Additional Context for Reviewing PR #154
-
Dependency Versions & Compatibility
- The
cap-stdcrate at version 3.4.4 (as added here) is the current release used by Bytecode Alliance projects such as Wasmtime (cap-std = "3.4.4") (gitee.com). cap-std’s fs_utf8 feature (which brings in thecaminocrate for UTF-8 paths) requires a minimum Rust version of 1.63 (docs.rs).- The
rustixcrate at version 1.0.8 is the latest stable release and provides safe bindings to POSIX/Linux syscalls; aside from itsnetmodule, most of its functionality (includingmknodatfor creating pipes and device nodes) is Unix-only and is disabled on Windows (docs.rs, github.com).
- The
-
CI Workflow Matrix
- GitHub Actions supports a matrix strategy where
runs-on: ${{ matrix.os }}runs one job per OS listed inmatrix.os(e.g.ubuntu-latest,windows-latest) (docs.github.com). - Restricting the matrix to
[ubuntu-latest]confines testing to Linux runners, intentionally avoiding Windows failures for the newly added Unix-specific tests.
- GitHub Actions supports a matrix strategy where
-
MiniJinja Custom Test Registration
- In MiniJinja, custom boolean tests (used via
{% if foo is test_name %}) are registered by callingenv.add_test("test_name", fn). The newregisterfunction insrc/stdlib.rsloops over a table of(name, predicate)pairs and invokesadd_testfor each filesystem predicate (dir,file,symlink, etc.) (docs.rs).
- In MiniJinja, custom boolean tests (used via
🔇 Additional comments (10)
Cargo.toml (1)
20-21: LGTM on adding cap-std + camino; they align with fs_utf8 usage.
No issues spotted with SemVer pins.tests/data/jinja_is.yml (1)
1-30: LGTM; targets map 1:1 to registered tests.
Naming and usage align with the Jinja tests and env-driven scenarios.tests/steps/fs_steps.rs (1)
54-57: Remove environment restoration recommendation
TheDropimplementation in tests/cucumber.rs (lines 34–38) already drains and restoresenv_varsviarestore_many(self.env_vars.drain().collect());Likely an incorrect or invalid review comment.
docs/netsuke-design.md (1)
826-831: Keep the behaviour note; it matches the implementation.“Missing paths evaluate to false; I/O errors raise a template error.” is accurate for the registered
is-*tests.src/lib.rs (1)
14-14: Expose stdlib publicly to make Jinja tests available.The new
pub mod stdlib;is the right integration point for downstream use.tests/steps/mod.rs (1)
7-8: Gate Unix-only steps behind cfg(unix).Keep platform builds green while enabling Unix scenarios on Linux runners.
tests/cucumber.rs (1)
44-45: Run Unix feature suite conditionally.Execute
tests/features_unixonly on Unix. This preserves Windows buildability while adding coverage where supported.src/manifest.rs (1)
541-541: Register stdlib tests before evaluation.Register tests on the environment prior to
foreach/whenevaluation sois-*predicates are usable in those expressions.tests/steps/manifest_steps.rs (1)
201-216: Assert exact target-name set for stronger behaviour guarantees.Split/trim the expected list and compare to actual names; this is precise and matches the feature intent.
tests/features_unix/fs_tests.feature (1)
3-19: Validate Unix file-type detection and missing-path semantics.Cover the happy path and the all-missing-paths case; steps compose cleanly with the workspace and env guards.
There was a problem hiding this comment.
Actionable comments posted: 6
♻️ Duplicate comments (9)
src/stdlib.rs (6)
8-33: Optional: collapse boilerplate via a small macro and const table.Generate
is_*predicates and a singleTESTStable; keep register loop trivial. This reduces repetition and aligns with prior feedback.Also applies to: 38-51
36-38: Add a Rustdoc usage example to the public API.Provide a minimal example so cargo doc is useful and meets docs policy.
Apply:
-/// Register standard library helpers with the Jinja environment. +/// Register standard library helpers with the Jinja environment. +/// +/// # Examples +/// ``` +/// # use minijinja::Environment; +/// # fn main() { +/// let mut env = Environment::new(); +/// netsuke::stdlib::register(&mut env); +/// // `env` now supports: `is dir`, `is file`, `is symlink` +/// // and (on Unix) `is pipe`, `is block_device`, `is char_device`, `is device`. +/// # } +/// ``` pub fn register(env: &mut Environment<'_>) {
1-7: Add a module-level //! doc comment.Document purpose, registered tests (and Unix-only variants), and error semantics (NotFound → false; other I/O → InvalidOperation) per guidelines.
Apply:
+//! Filesystem predicate tests for MiniJinja. +//! +//! Registers `{% if path is <test> %}` tests: +//! - dir, file, symlink (all platforms) +//! - pipe, block_device, char_device, and legacy device (Unix only) +//! +//! Missing paths yield `false`; other I/O errors raise `InvalidOperation`. use camino::Utf8Path;
53-57: Harden test registration: accept any Jinja value; treat non-strings as false.Avoid type errors when templates pass non-strings/undefined.
Apply:
- for &(name, pred) in TESTS { - env.add_test(name, move |path: String| { - is_file_type(Utf8Path::new(&path), pred) - }); - } + for &(name, pred) in TESTS { + env.add_test(name, move |val: minijinja::value::Value| -> minijinja::Result<bool> { + if let Some(s) = val.as_str() { + return is_file_type(Utf8Path::new(s), pred); + } + Ok(false) + }); + }Also align non‑Unix fallbacks accordingly (see next hunk).
59-63: Align non‑Unix fallbacks with the Value-accepting signature.Keep API consistent across platforms.
Apply:
- env.add_test("pipe", |_path: String| Ok(false)); - env.add_test("device", |_path: String| Ok(false)); + env.add_test("pipe", |_v: minijinja::value::Value| Ok(false)); + env.add_test("device", |_v: minijinja::value::Value| Ok(false));
73-76: Fix path splitting for roots and trailing separators.
unwrap_or("")can yield an empty component causing incorrect lookups; normalise to"."where appropriate.Apply:
- let (dir_path, file_name) = path.parent().map_or_else( - || (Utf8Path::new("."), path.as_str()), - |parent| (parent, path.file_name().unwrap_or("")), - ); + let (dir_path, file_name) = match (path.parent(), path.file_name()) { + (Some(parent), Some(name)) => (parent, name), + (Some(parent), None) => (parent, "."), // e.g. "dir/" + (None, Some(name)) => (Utf8Path::new("."), name), + (None, None) => (Utf8Path::new("."), "."), // e.g. "." + };docs/netsuke-design.md (2)
1669-1671: Fix footnote formatting and complete the access date.Remove the stray list marker and add the year.
Apply:
-[^1]: Ninja, a small build system with a focus on speed. Accessed on 12 July - - 1. <https://ninja-build.org/> +[^1]: Ninja, a small build system with a focus on speed. Accessed on 12 July 2025. + <https://ninja-build.org/>
811-825: Correct the file-system tests table to match implemented features.Do not document predicates that are not shipped in this PR.
Apply:
-| Test | True when the operand… | -| ----------------------------------------------------- | ---------------------------------------------------------------- | -| `dir` / `file` / `symlink` | …is that object type | -| `pipe` / `block_device` / `char_device` *(Unix-only)* | …is that object type | -| `device` (legacy, Unix-only) | …is a block or character device | -| `present` | …exists (any type) | -| `owned` | …is owned by the current UID | -| `readable` / `writable` / `executable` | …has the corresponding permission bit for current user | -| `empty` | …has size 0 bytes | -| `older_than(value)` | …has `mtime` < given value (seconds, `timedelta`, or file) | -| `newer_than(value)` | …has `mtime` > given value | -| `contains(substr)` | …file’s text contains **substr** | -| `matches(regex)` | …file’s text matches **regex** | -| `type(kind)` | …is of the file-type string supplied (`"file"`, `"dir"`, etc.) | +| Test | True when the operand… | +| ----------------------------------------------------- | -------------------------------- | +| `dir` / `file` / `symlink` | …is that object type | +| `pipe` / `block_device` / `char_device` *(Unix-only)* | …is that object type | +| `device` (legacy, Unix-only) | …is a block or character device |tests/steps/fs_steps.rs (1)
70-77: Use a hidden parent and pre-create it for stable missing pathsPrevent accidental collisions with real fixtures and keep semantics (file itself remains missing).
Apply this diff:
#[given(expr = "the environment variable {string} is set to a missing path")] fn set_missing_env_path(world: &mut CliWorld, key: String) { let temp = world.temp.as_ref().expect("file-type workspace tempdir"); - let path = Utf8PathBuf::from_path_buf(temp.path().join("__missing__").join(key.to_lowercase())) - .expect("utf8 missing path"); + let missing_dir = temp.path().join(".missing"); + std::fs::create_dir_all(&missing_dir).expect("create .missing parent"); + let path = Utf8PathBuf::from_path_buf(missing_dir.join(key.to_lowercase())) + .expect("utf8 missing path"); let previous = set_var(&key, path.as_std_path().as_os_str()); world.env_vars.entry(key).or_insert(previous); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (5)
docs/netsuke-design.md(8 hunks)src/stdlib.rs(1 hunks)tests/features_unix/fs_tests.feature(1 hunks)tests/steps/fs_steps.rs(1 hunks)tests/steps/manifest_steps.rs(2 hunks)
🧰 Additional context used
📓 Path-based instructions (5)
**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
**/*.rs: Use en-GB-oxendict spelling and grammar in code comments (exceptions allowed for external API names)
Function documentation must include clear examples; test documentation should omit redundant examples
Keep file size manageable: no Rust source file longer than 400 lines; split large switches/dispatch tables and move big test data to external files
Disallow Clippy warnings; fix warnings in code rather than silencing them
Extract helper functions for long functions; maintain separation of concerns and CQRS
Group many related parameters into meaningful structs
If returning a large error type, consider using Arc to reduce data copied/returned
Each Rust module must begin with a module-level //! comment stating purpose and utility
Document public APIs with Rustdoc (///) so cargo doc can generate documentation
Prefer immutable data; avoid unnecessary mut bindings
Handle errors with Result instead of 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 functions when conditional criteria 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
Use conditional compilation (#[cfg]/#[cfg_attr]) for functions unused under specific feature sets
Prefer .expect() over .unwrap()
Use concat!() for long string literals instead of escaping newlines with backslashes
Prefer single-line function bodies when appropriate (e.g., pub fn new(id: u64) -> Self { Self(id) })
Prefer semantic error enums (derive std::error::Error via thiserror) for inspectable conditions
Use an opaque eyre::Report only at the application boundary for human-readable logs; do not expose in public APIs
Never export eyre::Report from libraries; convert to domain error enums at API boundaries and to eyre only in main/top-level async tas...
Files:
tests/steps/manifest_steps.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:
tests/steps/manifest_steps.rssrc/stdlib.rstests/steps/fs_steps.rs
tests/**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
tests/**/*.rs: Write unit and behavioural tests for new functionality; run before and after changes
Use rstest fixtures for shared setup and replace duplicated tests with #[rstest(...)] parameterised cases
Prefer mockall for mocks/stubs
Mock non-deterministic dependencies via dependency injection using mockable (traits like Env and Clock) where appropriate; follow docs/reliable-testing-in-rust-via-dependency-injection.md
Files:
tests/steps/manifest_steps.rstests/steps/fs_steps.rs
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/netsuke-design.md
docs/**/*.md
📄 CodeRabbit inference engine (AGENTS.md)
docs/**/*.md: Use docs/ as the knowledge base and source of truth for requirements, dependencies, and architecture
Proactively update docs/ when decisions, requirements, libraries, or architecture change to keep documentation current
Documentation must use en-GB-oxendict spelling and grammar (LICENSE name unchanged)
Files:
docs/netsuke-design.md
**/*.md
📄 CodeRabbit inference engine (AGENTS.md)
**/*.md: Validate Markdown with make markdownlint
Run make fmt after documentation changes to format Markdown and fix table markup
Validate Mermaid diagrams in Markdown files using make nixie
Wrap Markdown paragraphs and bullet points at 80 columns; code blocks at 120; do not wrap tables or headings
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 / -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
🔇 Additional comments (3)
docs/netsuke-design.md (1)
826-833: LGTM (behaviour note aligns with code).The Unix/non‑Unix behaviour description matches src/stdlib.rs registration.
tests/steps/manifest_steps.rs (1)
202-217: LGTM: order-insensitive target-name checkUse of BTreeSet removes ordering concerns and matches intent.
tests/steps/fs_steps.rs (1)
1-2: Gate Unix-only steps behind cfg(unix)Prevent Windows build failures; this module uses symlink, FIFOs and device nodes.
Add a file-level gate to tests/steps/fs_steps.rs:
//! Steps for preparing file-system fixtures used in Jinja tests. +#![cfg(unix)]Guard the import in tests/steps/mod.rs with #[cfg(unix)] if you prefer to gate at the module level.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (10)
tests/steps/manifest_steps.rs (1)
226-241: Emit diff-friendly failure and ignore empty list itemsSkip empties from stray commas and surface missing/extra explicitly.
Apply:
fn manifest_has_targets_named(world: &mut CliWorld, names: String) { - let expected: BTreeSet<String> = names.split(',').map(|s| s.trim().to_string()).collect(); + let expected: BTreeSet<String> = names + .split(',') + .map(|s| s.trim()) + .filter(|s| !s.is_empty()) + .map(ToString::to_string) + .collect(); @@ - assert_eq!(actual, expected); + let missing: Vec<_> = expected.difference(&actual).cloned().collect(); + let extra: Vec<_> = actual.difference(&expected).cloned().collect(); + assert!( + missing.is_empty() && extra.is_empty(), + "target names mismatch; missing: {missing:?}, extra: {extra:?}" + ); }tests/steps/fs_steps.rs (2)
40-41: Fix invalid Errno variant: use ACCES (EACCES)Errno::ACCESS does not exist; compilation fails.
Apply:
- Err(e) if e == Errno::PERM || e == Errno::ACCESS => Utf8PathBuf::from("/dev/loop0"), + Err(e) if e == Errno::PERM || e == Errno::ACCES => Utf8PathBuf::from("/dev/loop0"),
50-52: Fix invalid Errno variant here as wellMirror the ACCES fix for the char-device fallback.
Apply:
- Err(e) if e == Errno::PERM || e == Errno::ACCESS => Utf8PathBuf::from("/dev/null"), + Err(e) if e == Errno::PERM || e == Errno::ACCES => Utf8PathBuf::from("/dev/null"),tests/features_unix/fs_tests.feature (2)
1-3: Tag as Unix-onlyMake Unix scope explicit for suite selection.
Apply:
-# Unix-specific file tests -Feature: File-system tests +# Unix-specific file tests +@unix +Feature: File-system tests
3-7: Add explicit cardinality assertion (7 targets)Assert the exact count alongside the names.
Apply:
Scenario: file system tests detect path types Given a file-type test workspace When the manifest file "tests/data/jinja_is.yml" is parsed + And the manifest has targets 7 And the manifest has targets named "is-dir, is-file, is-symlink, is-pipe, is-block-device, is-char-device, is-device"docs/netsuke-design.md (2)
811-825: Remove unimplemented file-system tests from the table.Document only what the code actually registers: dir, file, symlink, pipe (Unix), block_device (Unix), char_device (Unix), and legacy device (Unix). The extra rows create a false contract.
-| Test | True when the operand… | -| ----------------------------------------------------- | ---------------------------------------------------------------- | -| `dir` / `file` / `symlink` | …is that object type | -| `pipe` / `block_device` / `char_device` *(Unix-only)* | …is that object type | -| `device` (legacy, Unix-only) | …is a block or character device | -| `present` | …exists (any type) | -| `owned` | …is owned by the current UID | -| `readable` / `writable` / `executable` | …has the corresponding permission bit for current user | -| `empty` | …has size 0 bytes | -| `older_than(value)` | …has `mtime` < given value (seconds, `timedelta`, or file) | -| `newer_than(value)` | …has `mtime` > given value | -| `contains(substr)` | …file’s text contains **substr** | -| `matches(regex)` | …file’s text matches **regex** | -| `type(kind)` | …is of the file-type string supplied (`"file"`, `"dir"`, etc.) | +| Test | True when the operand… | +| ----------------------------------------------------- | -------------------------------------- | +| `dir` / `file` / `symlink` | …is that object type | +| `pipe` / `block_device` / `char_device` *(Unix-only)* | …is that object type | +| `device` (legacy, Unix-only) | …is a block or character device |
1670-1673: Fix footnote [^1] formatting and complete the access date.Remove the stray numbered list marker and add the year.
-[^1]: Ninja, a small build system with a focus on speed. Accessed on 12 July - - 1. <https://ninja-build.org/> +[^1]: Ninja, a small build system with a focus on speed. Accessed on 12 July 2025. + <https://ninja-build.org/>src/stdlib.rs (3)
1-7: Add the required module-level//!documentation.Comply with the docs mandate and clarify platform semantics.
+//! Filesystem predicate tests for MiniJinja. +//! +//! Registers `{% if path is <test> %}` tests: +//! - dir, file, symlink (all platforms) +//! - pipe, block_device, char_device, device (Unix-only; `device` is legacy) +//! +//! Missing paths yield `false`. Other I/O errors surface as `InvalidOperation`. use camino::Utf8Path;
36-38: Add a Rustdoc example toregister.Public APIs must include examples.
/// Register standard library helpers with the Jinja environment. +/// +/// # Examples +/// ``` +/// # use minijinja::Environment; +/// # fn main() { +/// let mut env = Environment::new(); +/// netsuke::stdlib::register(&mut env); +/// // env now supports: `is dir`, `is file`, `is symlink`, and (on Unix) device tests. +/// # } +/// ``` pub fn register(env: &mut Environment<'_>) {
134-167: Remove CAP_MKNOD requirements in unit tests; use /dev fallbacks.Creating block/char devices in tests fails on typical CI. Reuse existing nodes under
/dev.- let bdev = root.join("b"); - let cdev = root.join("c"); + let mut bdev = root.join("b"); // placeholder; replaced on Unix below + let mut cdev = root.join("c"); // placeholder; replaced on Unix below @@ - #[cfg(unix)] - mknodat( - &handle, - "b", - FileType::BlockDevice, - Mode::RUSR | Mode::WUSR, - Dev::default(), - ) - .expect("block"); - #[cfg(unix)] - mknodat( - &handle, - "c", - FileType::CharacterDevice, - Mode::RUSR | Mode::WUSR, - Dev::default(), - ) - .expect("char"); + #[cfg(unix)] + { + // Discover existing devices in /dev to avoid CAP_MKNOD. + let dev = Dir::open_ambient_dir("/dev", ambient_authority()).expect("open /dev"); + let mut found_block = None; + let mut found_char = None; + for entry in dev.entries().expect("list /dev") { + let md = entry.symlink_metadata().expect("md"); + let ft = md.file_type(); + if found_block.is_none() && ft.is_block_device() { + found_block = Some(camino::Utf8PathBuf::from(format!("/dev/{}", entry.file_name()))); + } + if found_char.is_none() && ft.is_char_device() { + found_char = Some(camino::Utf8PathBuf::from(format!("/dev/{}", entry.file_name()))); + } + if found_block.is_some() && found_char.is_some() { + break; + } + } + bdev = found_block.unwrap_or_else(|| camino::Utf8PathBuf::from("/dev/loop0")); + cdev = found_char.unwrap_or_else(|| camino::Utf8PathBuf::from("/dev/null")); + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (5)
docs/netsuke-design.md(8 hunks)src/stdlib.rs(1 hunks)tests/features_unix/fs_tests.feature(1 hunks)tests/steps/fs_steps.rs(1 hunks)tests/steps/manifest_steps.rs(4 hunks)
🧰 Additional context used
📓 Path-based instructions (5)
**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
**/*.rs: Use en-GB-oxendict spelling and grammar in code comments (exceptions allowed for external API names)
Function documentation must include clear examples; test documentation should omit redundant examples
Keep file size manageable: no Rust source file longer than 400 lines; split large switches/dispatch tables and move big test data to external files
Disallow Clippy warnings; fix warnings in code rather than silencing them
Extract helper functions for long functions; maintain separation of concerns and CQRS
Group many related parameters into meaningful structs
If returning a large error type, consider using Arc to reduce data copied/returned
Each Rust module must begin with a module-level //! comment stating purpose and utility
Document public APIs with Rustdoc (///) so cargo doc can generate documentation
Prefer immutable data; avoid unnecessary mut bindings
Handle errors with Result instead of 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 functions when conditional criteria 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
Use conditional compilation (#[cfg]/#[cfg_attr]) for functions unused under specific feature sets
Prefer .expect() over .unwrap()
Use concat!() for long string literals instead of escaping newlines with backslashes
Prefer single-line function bodies when appropriate (e.g., pub fn new(id: u64) -> Self { Self(id) })
Prefer semantic error enums (derive std::error::Error via thiserror) for inspectable conditions
Use an opaque eyre::Report only at the application boundary for human-readable logs; do not expose in public APIs
Never export eyre::Report from libraries; convert to domain error enums at API boundaries and to eyre only in main/top-level async tas...
Files:
tests/steps/fs_steps.rstests/steps/manifest_steps.rssrc/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:
tests/steps/fs_steps.rstests/steps/manifest_steps.rssrc/stdlib.rs
tests/**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
tests/**/*.rs: Write unit and behavioural tests for new functionality; run before and after changes
Use rstest fixtures for shared setup and replace duplicated tests with #[rstest(...)] parameterised cases
Prefer mockall for mocks/stubs
Mock non-deterministic dependencies via dependency injection using mockable (traits like Env and Clock) where appropriate; follow docs/reliable-testing-in-rust-via-dependency-injection.md
Files:
tests/steps/fs_steps.rstests/steps/manifest_steps.rs
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/netsuke-design.md
docs/**/*.md
📄 CodeRabbit inference engine (AGENTS.md)
docs/**/*.md: Use docs/ as the knowledge base and source of truth for requirements, dependencies, and architecture
Proactively update docs/ when decisions, requirements, libraries, or architecture change to keep documentation current
Documentation must use en-GB-oxendict spelling and grammar (LICENSE name unchanged)
Files:
docs/netsuke-design.md
**/*.md
📄 CodeRabbit inference engine (AGENTS.md)
**/*.md: Validate Markdown with make markdownlint
Run make fmt after documentation changes to format Markdown and fix table markup
Validate Mermaid diagrams in Markdown files using make nixie
Wrap Markdown paragraphs and bullet points at 80 columns; code blocks at 120; do not wrap tables or headings
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 / -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 (1)
tests/steps/manifest_steps.rs (1)
test_support/src/env.rs (4)
set_var(28-28)set_var(32-34)set_var(38-40)set_var(61-67)
🔍 Remote MCP Deepwiki
, which explains why the PR includes fallback logic for unprivileged device creation in tests.
Project Context from DeepWiki The netsuke project is a build system compiler that transforms declarative YAML manifests into efficient Ninja build graphs, with filesystem tests being part of the standard library to provide Jinja template capabilities for querying file properties. The stdlib module provides built-in helpers including file-system tests like dir, file, symlink, pipe, and device to check properties of files and directories within Netsukefile manifests. The manifest parsing in netsuke follows a two-stage pipeline: first Jinja templating to resolve dynamic content, then YAML deserialization. The Jinja environment is configured with custom functions and filters to provide build-specific functionality, serving as the "logic layer" for the build system.
Key Review Points
- Security considerations: The use of cap-std provides sandboxed filesystem access during template evaluation
- Cross-platform compatibility: The PR properly guards Unix-specific tests behind
#[cfg(unix)] - Test infrastructure: The addition of comprehensive test fixtures for different file types using appropriate system calls
- MiniJinja integration: Correct usage of the test registration API for extending template capabilities
🔇 Additional comments (8)
tests/steps/manifest_steps.rs (2)
9-9: LGTM: deterministic set chosenUsing BTreeSet for stable order is appropriate for diagnostics.
92-94: LGTM: expand before setExpanding the value prior to set_var aligns with the feature steps’ expectations.
tests/steps/fs_steps.rs (3)
1-2: LGTM: module purpose documentedModule-level doc comment meets guidelines.
22-30: LGTM: FIFO fixture creationUnprivileged FIFO via mknodat with 0600 is correct.
11-79: Leave as‑is — fs_steps already Unix‑gated.#[cfg(unix)] precedes mod fs_steps in tests/steps/mod.rs (line 7), so no additional cfg guard is required.
docs/netsuke-design.md (2)
826-834: LGTM: Behavioural notes match implementation.Non‑Unix registering only
pipeanddeviceas always‑false aligns with src/stdlib.rs.
1200-1215: LGTM: CLI forwarding and stdio piping.Forwarding
-j, resolving-Cinternally, and piping stdio is correct and coherent with Section 6.1.src/stdlib.rs (1)
80-105: LGTM: Error handling and NotFound semantics.Return
falseonNotFound; wrap other I/O errors asInvalidOperationwith source preserved.
|
@coderabbitai pause |
✅ Actions performedReviews paused. |
|
@coderabbitai Have the following now been 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.
This comment was marked as resolved.
0c7473c to
f161850
Compare
Summary
istests while retaining legacydevicedocs/srgn.mdreferences to footnotes and remove placeholder datesTesting
make fmtmake lintmake testmake markdownlintmake nixiehttps://chatgpt.com/codex/tasks/task_e_68c0a5d63d1483228887d76074c863ad
Summary by Sourcery
Implement file-system Jinja tests and integrate them into the build pipeline
New Features:
dir,file,symlink,pipe,block_device,char_device, and legacydevice) via a new stdlib moduleEnhancements:
CI:
Documentation:
Tests:
Chores: