Adopt camino/cap-std paths across integration tests (#418) - #428
Adopt camino/cap-std paths across integration tests (#418)#428lodyai[bot] wants to merge 6 commits into
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
Summary
Validation
WalkthroughThe PR extracts static-regex validation into a tested script, adds shared capability-scoped test directories, updates matrix fixture I/O, migrates integration tests, and adds Whitaker and repository guidance. ChangesLint and test filesystem updates
Sequence Diagram(s)sequenceDiagram
participant Makefile
participant StaticRegexScript
participant Ripgrep
Makefile->>StaticRegexScript: Pass RG and scan directory
StaticRegexScript->>Ripgrep: Scan Rust files for prohibited declarations
Ripgrep-->>StaticRegexScript: Return matches or scan status
StaticRegexScript-->>Makefile: Return validation status
Possibly related issues
Possibly related PRs
Suggested reviewers: Poem
Caution Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional.
❌ Failed checks (1 error, 4 inconclusive)
✅ Passed checks (15 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Reviewer's GuideThis PR refactors all file-backed integration tests to use UTF-8 capability-scoped filesystem access via camino and cap-std, centralizes temporary directory handling in a shared TestDir helper, extracts CLI matrix fixture handling into a dedicated capability module, and adds a scripted static-regex guard with regression tests and documentation including an imported Whitaker user’s guide. Sequence diagram for the new static regex lint script integrationsequenceDiagram
participant Makefile
participant check_static_regexes_sh as check_static_regexes_sh
participant rg
Makefile->>check_static_regexes_sh: invoke RG="$(RG)" scripts/check-static-regexes.sh .
check_static_regexes_sh->>rg: run_rg_pattern
rg-->>check_static_regexes_sh: exit_status
alt [rg exit_status == 0]
check_static_regexes_sh-->>Makefile: print "static regular expressions must use lazy_regex!" and exit 1
else [rg exit_status == 1]
check_static_regexes_sh-->>Makefile: exit 0 (no prohibited declaration)
else [rg exit_status > 1]
check_static_regexes_sh-->>Makefile: print scan failure and exit rg status
end
File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
![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 automated review suggestions for this pull request.
Reviewed commit: d901ee33d3
ℹ️ 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
- 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 address that feedback".
| # `once_cell::sync::` in `once_cell::sync::Lazy::new`), so both the direct and | ||
| # fully qualified spellings of each supported constructor are rejected. | ||
| # `(?:move\s+)?` covers `move` closures such as `LazyLock::new(move || ...)`. | ||
| pattern='\bstatic\b[^;=]*=\s*(?:[[:alnum:]_]+::)*(?:LazyLock|Lazy)::new\s*\(\s*(?:move\s+)?\|\|\s*(\{\s*)?(?:[[:alnum:]_]+::)*Regex::new' |
There was a problem hiding this comment.
When a lazy initializer contains a comment before Regex::new, such as Lazy::new(|| { /* rationale */ Regex::new("x").unwrap() }), this pattern reports no match because the optional block accepts only whitespace between { and the constructor. Such declarations therefore bypass make lint, despite the script and developer guide claiming that every directly wrapped Regex::new is rejected; extend the pattern and add a fixture covering comments in the closure body.
AGENTS.md reference: AGENTS.md:L172-L174
Useful? React with 👍 / 👎.
The `check-static-regexes` guard only rejected `LazyLock::new(... Regex::new(...))`, so a hand-rolled static regex wrapped in `once_cell::sync::Lazy::new(|| Regex::new(...))` slipped past `make lint`. Extract the scan into `scripts/check-static-regexes.sh` as the single source of truth and broaden its pattern to reject both supported lazy-wrapper constructors — `std::sync::LazyLock::new` and `once_cell::sync::Lazy::new` — whether spelled directly or fully qualified. `lazy_regex!` remains the sole sanctioned idiom. Add `tests/static_regex_lint.rs`, which drives the script against fixtures for every supported wrapper form, asserts a clean source passes, and asserts a ripgrep scan failure propagates its exit status. Fixtures live under `tests/data/static_regex/` with a `.rs.txt` extension so the guard does not match them in place. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Address review findings on the broadened static-regex lint: - Restore support for `RG` overrides that carry arguments (for example `RG='rg --pcre2'`). Extracting the scan into the shell script replaced the Makefile's word-splitting `$(RG)` expansion with a quoted `"$RG"`, which treated the whole value as one executable name. Split `RG` into an array so arguments are preserved, matching `check-ripgrep`'s `firstword` handling. - Reject `move` closures. The pattern only matched `|| Regex::new(...)`, so a hand-rolled `LazyLock::new(move || Regex::new(...))` (or the `once_cell` `Lazy` equivalent) slipped through. Add `(?:move\s+)?` and fixtures for both wrapper families. - Make the regression tests deterministic: `run_guard` now clears any ambient `RG` on default-path runs so the guard's own `rg` default is exercised. Skipped the suggestion to migrate the test to camino/cap-std path types: it would add two dependencies absent from the tree and diverge from the existing test suite, which uses std::fs and tempfile. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Address review findings on the static-regex lint regression tests: - Widen `run_guard`'s `rg` parameter from `Option<&Path>` to `Option<&str>` so tests can supply an `RG` value that carries arguments, and add `preserves_arguments_supplied_through_rg`. It drives the guard with `RG='<stub> --pcre2'` and asserts, via a stub that records its argv, that the override's arguments are forwarded ahead of the guard's own and that the scan directory remains last. Reverting the array split makes this test fail with exit 127, the exact symptom the fix removed. - Extract the executable-stub setup into a `write_stub` helper shared by the scan-failure and argument-preservation tests. - Spell the `scan_dir_with` doc comment "Materialize", matching the repository's en-GB-oxendict Oxford-spelling convention. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Use UTF-8 paths and directory capabilities throughout file-backed integration tests. Centralize temporary-directory setup while keeping ambient access at explicit repository and process boundaries. Preserve existing command execution, fixtures, snapshots, and behavioural assertions.
Record the integration suite's UTF-8 path and directory-capability convention, including the ownership boundary for shared test support. Explain why the production dependencies are reused by tests and import the current Whitaker user's guide for path-level lint exclusion guidance.
Retain main's tracing-test guidance and the branch's capability-scoped filesystem convention while removing the duplicate test-macros section introduced by semantic replay.
d901ee3 to
239c7f6
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/whitaker-users-guide.md`:
- Around line 1-9: Update all Markdown headings in the Whitaker user's guide to
sentence case, including the visible headings “Whitaker User's Guide,” “Quick
Setup,” “Lint Configuration,” “Localized Diagnostics,” “Available Lints,”
“Standard vs Experimental Lints,” and “Clone Detection: AST Feature Extraction,”
while preserving their existing heading levels and meaning.
- Around line 646-648: Update the documentation near the single-site exemption
tip to keep excluded_paths as the preferred approach, remove the direct
#[allow(no_std_fs_operations)] example, and document a narrowly scoped, reasoned
suppression using Dylint’s cfg_attr pattern as a last resort; do not recommend
#[expect] because the dynamically loaded lint may be unknown to rustc.
In `@scripts/check-static-regexes.sh`:
- Around line 28-32: Update the regex pattern in the static-regex check to allow
only std::sync::LazyLock, once_cell::sync::Lazy, LazyLock, and Lazy; do not
accept arbitrary module-qualified lazy wrappers. Add a clean-scan regression
fixture covering an unrelated qualified wrapper and verify it is not reported.
In `@tests/cli_matrix/support_tests.rs`:
- Around line 19-27: Update non_wrap_signature_ignores_wrap_variant to invoke
the signature-building caller with two physical cases that differ only in wrap
variant, then assert their resulting signatures are equal. Remove the unrelated
unwrapped/wrapped boolean assertion and avoid comparing two identical
non_wrap_signature inputs so the test detects incorrect wrap-variant
integration.
In `@tests/wrap/cli_files.rs`:
- Around line 21-28: Update the file-backed CLI test around TestDir, write the
input using a non-ASCII filename such as “漢字.md”, and join that same filename
with dir.path() before passing it to Command. Keep the write relative to TestDir
and leave the existing command invocation unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 9d3ac5fc-2bea-4073-8566-478c4f304ef5
📒 Files selected for processing (27)
Cargo.tomlMakefiledocs/contents.mddocs/developers-guide.mddocs/repository-layout.mddocs/whitaker-users-guide.mdscripts/check-static-regexes.shtests/cli.rstests/cli_frontmatter.rstests/cli_matrix.rstests/cli_matrix/fixture_io.rstests/cli_matrix/invariants.rstests/cli_matrix/support.rstests/cli_matrix/support_tests.rstests/code_emphasis.rstests/common/fs.rstests/data/static_regex/clean.rs.txttests/data/static_regex/lazylock_direct.rs.txttests/data/static_regex/lazylock_move.rs.txttests/data/static_regex/lazylock_qualified.rs.txttests/data/static_regex/once_cell_lazy_direct.rs.txttests/data/static_regex/once_cell_lazy_move.rs.txttests/data/static_regex/once_cell_lazy_qualified.rs.txttests/parallel.rstests/static_regex_lint.rstests/wrap/cli_files.rstests/wrap_cli.rs
| # Whitaker User's Guide | ||
|
|
||
| Whitaker is a collection of opinionated Dylint lints for Rust. This guide | ||
| explains how to integrate the lints into a project and configure them. | ||
|
|
||
| For contributors who want to develop new lints or work on Whitaker itself, see | ||
| the [Developer's Guide](developers-guide.md). | ||
|
|
||
| ## Quick Setup |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Set headings to sentence case throughout the guide.
Change headings such as # Whitaker User's Guide, ## Quick Setup, ## Lint Configuration, ## Localized Diagnostics, ## Available Lints, ### Standard vs Experimental Lints, and ## Clone Detection: AST Feature Extraction to sentence case.
Triage: [type:docstyle]
As per coding guidelines and path instructions, Markdown headings must use sentence case.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/whitaker-users-guide.md` around lines 1 - 9, Update all Markdown
headings in the Whitaker user's guide to sentence case, including the visible
headings “Whitaker User's Guide,” “Quick Setup,” “Lint Configuration,”
“Localized Diagnostics,” “Available Lints,” “Standard vs Experimental Lints,”
and “Clone Detection: AST Feature Extraction,” while preserving their existing
heading levels and meaning.
Sources: Coding guidelines, Path instructions
| > **Tip:** For an ad hoc, single-site exemption that travels with the code, a | ||
| > standard `#[allow(no_std_fs_operations)]` attribute on the item or module | ||
| > also works, since the lint honours Rust's lint-level attributes. |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- candidate files ---'
git ls-files | rg '(^|/)(docs/(contents|repository-layout|whitaker-users-guide|documentation-style-guide)\.md|Cargo\.(toml|lock)|.*whitaker.*)$' || true
printf '%s\n' '--- relevant documentation ---'
for f in docs/contents.md docs/repository-layout.md docs/whitaker-users-guide.md docs/documentation-style-guide.md; do
if [ -f "$f" ]; then
printf '\n--- %s ---\n' "$f"
if [ "$f" = docs/whitaker-users-guide.md ]; then
sed -n '610,675p' "$f"
else
sed -n '1,220p' "$f"
fi
fi
done
printf '%s\n' '--- Whitaker references and lint policies ---'
rg -n -C 3 'Whitaker|whitaker|no_std_fs_operations|excluded_paths|#\[allow|#\[expect' . --glob '!target' --glob '!node_modules' || trueRepository: leynos/mdtablefix
Length of output: 41636
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- repository configuration files ---'
git ls-files | rg '(^|/)(dylint\.toml|rust-toolchain(\.toml)?|AGENTS\.md|CRUSH\.md|Cargo\.toml|Cargo\.lock|Makefile)$'
printf '%s\n' '--- Cargo.toml ---'
cat -n Cargo.toml
printf '%s\n' '--- Whitaker setup and version references ---'
sed -n '1,125p' docs/whitaker-users-guide.md
rg -n -C 5 'no_std_fs_operations|allow|expect|excluded_paths|whitaker|dylint|version|tag|rev' Cargo.toml Cargo.lock dylint.toml rust-toolchain.toml rust-toolchain AGENTS.md CRUSH.md Makefile 2>/dev/null || true
printf '%s\n' '--- tracked allow/expect policy ---'
rg -n -C 3 '#\[allow|#\[expect|no_allow|allow.*suppress|suppression' --glob '*.rs' --glob '*.md' --glob '*.toml' . || trueRepository: leynos/mdtablefix
Length of output: 50375
🌐 Web query:
Find the official leynos/whitaker repository or release documentation and determine whether the pinned Whitaker release supports Rust #[expect(lint, reason = "...")]attributes forno_std_fs_operations.
💡 Result:
The leynos/whitaker repository is a collection of opinionated Dylint lints for Rust [1][2]. It is not a tool that implements the Rust compiler's internal lint attribute system, nor is it a release-based package in the traditional sense; rather, it is a repository of lint libraries [1][3]. The project uses an installer CLI tool, whitaker-installer, to build, stage, and manage these Dylint libraries [2][3]. The #[expect(lint, reason = ...)] attribute is a native feature of the Rust compiler (stable as of Rust 1.81.0) and is part of the language's diagnostic system, not a feature of the Whitaker lint suite [4][5]. Because the #[expect] attribute is a core language feature processed by the compiler and not by Dylint or Whitaker specifically, its support for any specific code—including no_std_fs_operations—is determined by the Rust compiler version you are using, not by the Whitaker release. Whitaker lints are external plugins for the cargo-dylint tool [1][2], and they do not influence the Rust compiler's support for standard attributes like #[expect]. If you use a modern Rust toolchain (1.81.0 or later), you can use #[expect(lint, reason = "...")] attributes in your code, and the compiler will process them regardless of whether the Whitaker lint suite is active in your workspace [5].
Citations:
- 1: https://github.com/leynos/whitaker
- 2: https://crates.io/crates/whitaker-installer
- 3: https://docs.rs/crate/whitaker-installer/latest
- 4: https://doc.rust-lang.org/reference/attributes/diagnostics.html
- 5: https://stackoverflow.com/questions/73538183/how-to-allow-and-expect-a-compiler-warning-in-rust
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- all tracked Whitaker-related paths and configuration ---'
git ls-files | rg -i 'whitaker|dylint|github|workflow|toolchain|metadata' || true
printf '%s\n' '--- root configuration ---'
for f in rust-toolchain.toml AGENTS.md CRUSH.md Makefile; do
if [ -f "$f" ]; then
printf '\n--- %s ---\n' "$f"
case "$f" in
AGENTS.md|CRUSH.md) sed -n '150,210p' "$f" ;;
*) cat -n "$f" ;;
esac
fi
done
printf '%s\n' '--- exact project-wide policy and Whitaker configuration references ---'
git grep -n -E 'workspace\.metadata\.dylint|whitaker|dylint|no_std_fs_operations|excluded_paths|#\[allow|#\[expect' -- ':!Cargo.lock' ':!docs/whitaker-users-guide.md' || true
printf '%s\n' '--- repository state ---'
git status --shortRepository: leynos/mdtablefix
Length of output: 9896
🌐 Web query:
Search the official leynos/whitaker source for no_std_fs_operationsand#[expectorlint-level handling, and identify any release or commit pin used by leynos/mdtablefix.
💡 Result:
In the leynos/whitaker source, the lint no_std_fs_operations is a tool that forbids direct usage of std::fs operations, encouraging the use of capability-based filesystem access via cap_std instead [1][2]. Regarding the lint-level handling, leynos/whitaker provides the no_expect_outside_tests lint, which explicitly bans.expect calls on Option or Result outside of test contexts [1][2][3]. While the source documentation highlights how to fix violations by using proper error handling like? or map_err [1], there is no information indicating that leynos/whitaker implements or handles the native Rust #[expect(lint-level)] attribute. There is no publicly available information in the provided results regarding a release commit pin used by leynos/mdtablefix [4][5]. Although mdtablefix is a Rust crate owned by leynos [5], the available sources do not document any internal reliance on specific git commit pins for its releases [4][5][6].
Citations:
- 1: https://github.com/leynos/whitaker/blob/main/docs/users-guide.md
- 2: https://github.com/leynos/whitaker
- 3: https://lib.rs/crates/whitaker-common
- 4: https://crates.io/crates/mdtablefix
- 5: https://lib.rs/crates/mdtablefix
- 6: https://docs.rs/mdtablefix/latest/mdtablefix/
🏁 Script executed:
#!/bin/bash
set -eu
repo='https://api.github.com/repos/leynos/whitaker'
tree_json=$(curl -fsSL "$repo/git/trees/main?recursive=1")
printf '%s\n' '--- matching Whitaker source paths ---'
printf '%s' "$tree_json" | jq -r '.tree[] | select(.path | test("no_std_fs_operations|lint|users-guide"; "i")) | .path' | head -80
printf '%s\n' '--- matching source excerpts ---'
printf '%s' "$tree_json" |
jq -r '.tree[] | select(.type == "blob" and (.path | test("no_std_fs_operations|lint"; "i"))) | .path' |
while IFS= read -r path; do
printf '\n--- %s ---\n' "$path"
curl -fsSL "https://raw.githubusercontent.com/leynos/whitaker/main/$path" |
rg -n -C 4 'no_std_fs_operations|register_tool|declare_lint|allow|expect|lint_level|LintLevel' || true
done
printf '%s\n' '--- Whitaker tags ---'
curl -fsSL "$repo/tags?per_page=30" | jq -r '.[] | [.name, .commit.sha] | `@tsv`'Repository: leynos/mdtablefix
Length of output: 206
🌐 Web query:
Search official Rust and Dylint documentation for whether #[expect(custom_lint, reason = "...")]applies to custom lints emitted by Dylint, including lints such asno_std_fs_operations.
💡 Result:
The #[expect(lint_name, reason = "...")] attribute is a feature of the Rust compiler designed to manage built-in lints and those registered with the compiler (such as Clippy lints) [1][2]. It does not natively apply to custom lints emitted by Dylint because Dylint lints are loaded dynamically as external libraries rather than being registered as built-in tools by the compiler [3][4][5]. When you use #[expect(custom_lint)] with a Dylint-provided lint, the Rust compiler will typically emit an "unknown lint" warning because it is not aware of the custom lint name during standard compilation [3][4][5]. To manage Dylint lints, the recommended approach is to use conditional compilation with cfg_attr [3][4][5]. This allows you to apply allow (or similar) attributes only when Dylint is actively running, preventing "unknown lint" warnings when compiling with standard rustc [3][4][5]. For example: #[cfg_attr(dylint_lib = "LIBRARY_NAME", allow(LINT_NAME))] For pre-expansion lints where cfg_attr may not be sufficient, the Dylint documentation suggests allowing the built-in unknown_lints lint [3][4][5]. Because Dylint lints are not integrated into the compiler's primary lint registration system, the #[expect] attribute—which requires the compiler to know about the lint and track its emission to verify the expectation—is not supported for them [1][2][3].
Citations:
- 1: https://doc.rust-lang.org/reference/attributes/diagnostics.html
- 2: https://rust-lang.github.io/rfcs/2383-lint-reasons.html
- 3: https://docs.rs/crate/dylint/latest
- 4: https://github.com/trailofbits/dylint/
- 5: https://trailofbits.github.io/dylint/
🌐 Web query:
Search the official Dylint documentation and source for cfg_attr(dylint_lib, unknown_lints, custom lint registration, and #[expect]. Determine whether Dylint lints are registered early enough for Rust to honour #[expect(custom_lint, reason = "...")].
💡 Result:
Dylint lints are not registered early enough for the Rust compiler to honor the #[expect(custom_lint)] attribute [1][2][3]. The Dylint documentation explicitly states that while it uses --cfg=dylint_lib="LIBRARY_NAME" to allow standard #[allow] attributes, this approach does not work for pre-expansion lints [4][1][2]. Furthermore, the fundamental design of Dylint involves loading lints from dynamic libraries, which occurs after the compiler has already processed standard lint attributes like #[expect] or #[allow] in many contexts [1][3]. Because of this, if you attempt to use #[expect] with a custom Dylint lint, the compiler will treat it as an unknown lint, resulting in an "unknown lint" warning or error unless the #[allow(unknown_lints)] attribute is used [1][2][3]. The official workaround for pre-expansion lints, as documented by Dylint, is to allow the compiler's built-in unknown_lints lint [4][5][6]. Additionally, to manage the use of conditional compilation attributes like #[cfg_attr(dylint_lib =...)] in modern Rust (which checks for unexpected cfg names), you must configure your Cargo.toml to register the dylint_lib configuration flag [4][1][2]: [lints.rust.unexpected_cfgs] level = "warn" check-cfg = ["cfg(dylint_lib, values(any))"]
Citations:
- 1: https://github.com/trailofbits/dylint/blob/master/README.md
- 2: https://trailofbits.github.io/dylint/
- 3: https://github.com/trailofbits/dylint/
- 4: https://docs.rs/crate/dylint/latest
- 5: https://docs.rs/crate/dylint/3.5.0
- 6: https://docs.rs/crate/dylint/3.5.1
Document a Dylint-compatible last-resort suppression.
#[expect(no_std_fs_operations, reason = "...")] is not a valid replacement because Dylint loads this custom lint dynamically, so rustc can report it as unknown. Keep excluded_paths as the preferred option. Document a narrowly scoped, reasoned #[allow] using Dylint’s cfg_attr pattern only as a last resort.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/whitaker-users-guide.md` around lines 646 - 648, Update the
documentation near the single-site exemption tip to keep excluded_paths as the
preferred approach, remove the direct #[allow(no_std_fs_operations)] example,
and document a narrowly scoped, reasoned suppression using Dylint’s cfg_attr
pattern as a last resort; do not recommend #[expect] because the dynamically
loaded lint may be unknown to rustc.
Sources: Coding guidelines, Path instructions
| # `(?:[[:alnum:]_]+::)*` absorbs any module qualification (for example the | ||
| # `once_cell::sync::` in `once_cell::sync::Lazy::new`), so both the direct and | ||
| # fully qualified spellings of each supported constructor are rejected. | ||
| # `(?:move\s+)?` covers `move` closures such as `LazyLock::new(move || ...)`. | ||
| pattern='\bstatic\b[^;=]*=\s*(?:[[:alnum:]_]+::)*(?:LazyLock|Lazy)::new\s*\(\s*(?:move\s+)?\|\|\s*(\{\s*)?(?:[[:alnum:]_]+::)*Regex::new' |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
workdir="$(mktemp -d)"
trap 'rm -rf "$workdir"' EXIT
printf '%s\n' \
'static RE: project::Lazy<Regex> = project::Lazy::new(|| Regex::new("x"));' \
>"$workdir/custom.rs"
pattern='\bstatic\b[^;=]*=\s*(?:[[:alnum:]_]+::)*(?:LazyLock|Lazy)::new\s*\(\s*(?:move\s+)?\|\|\s*(\{\s*)?(?:[[:alnum:]_]+::)*Regex::new'
if rg -U --glob '*.rs' "$pattern" "$workdir"; then
echo "unexpected match for an unsupported qualified wrapper" >&2
exit 1
fiRepository: leynos/mdtablefix
Length of output: 313
Restrict qualified lazy wrappers to documented paths.
Match only std::sync::LazyLock, once_cell::sync::Lazy, and the two unqualified forms. Add a clean-scan regression fixture for an unrelated qualified wrapper.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@scripts/check-static-regexes.sh` around lines 28 - 32, Update the regex
pattern in the static-regex check to allow only std::sync::LazyLock,
once_cell::sync::Lazy, LazyLock, and Lazy; do not accept arbitrary
module-qualified lazy wrappers. Add a clean-scan regression fixture covering an
unrelated qualified wrapper and verify it is not reported.
| fn non_wrap_signature_ignores_wrap_variant() { | ||
| let flags = [TransformFlag::Renumber, TransformFlag::Fences]; | ||
| let (unwrapped, wrapped) = (false, true); | ||
| assert_ne!(unwrapped, wrapped); | ||
| assert_eq!( | ||
| non_wrap_signature("fixture.dat", &flags), | ||
| non_wrap_signature("fixture.dat", &flags) | ||
| ); | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Replace the tautological wrap-variant test.
At Lines 21-26, test the caller that builds signatures for two physical cases that
differ only by the wrap variant. Remove the unrelated boolean assertion. The two
non_wrap_signature calls have identical inputs, so this test cannot detect an
incorrect wrap-variant integration.
As per coding guidelines, “New functionality and behavioral changes require
substantive tests that would fail for plausible incorrect implementations.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/cli_matrix/support_tests.rs` around lines 19 - 27, Update
non_wrap_signature_ignores_wrap_variant to invoke the signature-building caller
with two physical cases that differ only in wrap variant, then assert their
resulting signatures are equal. Remove the unrelated unwrapped/wrapped boolean
assertion and avoid comparing two identical non_wrap_signature inputs so the
test detects incorrect wrap-variant integration.
Source: Coding guidelines
| let dir = TestDir::new()?; | ||
| dir.directory().write("input.md", input)?; | ||
| let file_path = dir.path().join("input.md"); | ||
|
|
||
| let mut command = Command::cargo_bin("mdtablefix")?; | ||
| let output = command | ||
| .arg("--wrap") | ||
| .arg(file.path()) | ||
| .arg(file_path.as_std_path()) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Exercise a non-ASCII filename at the CLI boundary.
Use a filename such as 漢字.md in this file-backed test. The migrated tests currently use ASCII filenames, so they do not verify UTF-8 filename handling across the process boundary. Keep the write relative to TestDir and pass the joined UTF-8 path to Command.
Proposed adjustment
let dir = TestDir::new()?;
- dir.directory().write("input.md", input)?;
- let file_path = dir.path().join("input.md");
+ let file_name = "漢字.md";
+ dir.directory().write(file_name, input)?;
+ let file_path = dir.path().join(file_name);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/wrap/cli_files.rs` around lines 21 - 28, Update the file-backed CLI
test around TestDir, write the input using a non-ASCII filename such as “漢字.md”,
and join that same filename with dir.path() before passing it to Command. Keep
the write relative to TestDir and leave the existing command invocation
unchanged.
|
@coderabbitai review |
✅ Action performedReview finished.
|
2 participants
Summary
This branch adopts UTF-8 paths and capability-scoped filesystem access across
the integration-test suite so file-backed tests follow the same boundary as
the production CLI.
It centralizes temporary-directory capability setup, migrates the pending
static-regex guard tests from #415 alongside the existing suite, documents the
convention and imports the latest Whitaker user's guide for path-level
no_std_fs_operationsexclusion guidance.Closes #418.
Review walkthrough
tests/common/fs.rsfor the sharedTempDir,Utf8PathBuf, andcap_std::fs_utf8::Dirboundary.tests/static_regex_lint.rsandtests/cli_matrix/fixture_io.rsfor capability-scoped temporary and repository fixture access.Cargo.toml,docs/developers-guide.md, anddocs/whitaker-users-guide.mdfor the dependency rationale, ownership rules, and imported lint reference.Validation
make check-fmt: passedmake lint: passedmake test: passedmake markdownlint: passedmake nixie: passedmbake validate Makefile: passedgit diff --check: passedcoderabbit review --agent: three milestone reviews completed with zero findingsNotes
caminoandcap-stdwere added to the production dependency set by #406before this issue was implemented. The integration tests therefore reuse the
existing declarations rather than adding redundant dev-dependency entries;
no lockfile change is required.
The branch preserves the exact commits from the still-open originating PR
#415. Once #415 merges, GitHub will narrow this pull request's effective diff
to the #418 migration and documentation commits.
References