Broaden static-regex lint guard to cover supported lazy wrappers (#410) - #415
Open
lodyai[bot] wants to merge 3 commits into
Open
Conversation
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>
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
|
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
WalkthroughThe static regex lint scan now runs through a dedicated shell guard, wired from Make and documented for developers. Regression fixtures and tests cover six prohibited lazy wrappers, valid usage, and ripgrep failure propagation. ChangesStatic regex guard
Sequence Diagram(s)sequenceDiagram
participant Make
participant Guard
participant Ripgrep
participant RustFiles
Make->>Guard: run with configured RG command
Guard->>Ripgrep: scan *.rs for prohibited static regex wrappers
Ripgrep->>RustFiles: inspect source files
Ripgrep-->>Guard: return matches or scan status
Guard-->>Make: return lint status and diagnostic
Possibly related issues
Possibly related PRs
Suggested labels: Poem
Caution Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional.
❌ Failed checks (1 error)
✅ Passed checks (19 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
leynos
marked this pull request as ready for review
July 26, 2026 22:44
![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: 013311db03
ℹ️ 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".
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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 `@Makefile`:
- Line 45: Update the check-static-regexes invocation in the Makefile so RG is
passed as a single executable path, matching check-ripgrep’s firstword handling;
alternatively, revise check-static-regexes.sh to parse RG command arguments
consistently. Preserve support for RG values that include flags such as “rg
--pcre2” without treating the entire value as one executable.
In `@scripts/check-static-regexes.sh`:
- Line 29: Update the static regex pattern in the shell script to accept an
optional move keyword between the lazy initializer opening parenthesis and the
closure delimiter, covering both LazyLock and Lazy wrappers. Add regression
fixtures exercising move closures for each wrapper family while preserving
rejection of the existing non-move forms.
In `@tests/static_regex_lint.rs`:
- Around line 64-70: Update run_guard so the None branch explicitly clears the
child process’s inherited RG environment variable, ensuring default-path tests
use the standard ripgrep resolution; keep the existing override behavior for
Some(rg) unchanged.
- Around line 18-24: Update the integration test imports and filesystem
operations to use camino path types and cap_std::fs_utf8 APIs instead of
std::path and std::fs. Apply the replacements throughout the test while
preserving the existing behavior of Command execution and TempDir handling.
🪄 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: 021fb5e4-7bc6-4187-9bbc-0927c7eee7a4
📒 Files selected for processing (9)
Makefiledocs/developers-guide.mdscripts/check-static-regexes.shtests/data/static_regex/clean.rs.txttests/data/static_regex/lazylock_direct.rs.txttests/data/static_regex/lazylock_qualified.rs.txttests/data/static_regex/once_cell_lazy_direct.rs.txttests/data/static_regex/once_cell_lazy_qualified.rs.txttests/static_regex_lint.rs
Comment on lines
+18
to
+24
| use std::{ | ||
| path::{Path, PathBuf}, | ||
| process::Command, | ||
| }; | ||
|
|
||
| use rstest::rstest; | ||
| use tempfile::TempDir; |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy lift
Use the repository’s capability-oriented filesystem APIs.
Replace std::path and all std::fs calls in this integration test with camino and cap_std::fs_utf8. As per coding guidelines, Rust code must use cap_std/cap_std::fs_utf8 and camino instead of std::fs and std::path.
🤖 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/static_regex_lint.rs` around lines 18 - 24, Update the integration test
imports and filesystem operations to use camino path types and cap_std::fs_utf8
APIs instead of std::path and std::fs. Apply the replacements throughout the
test while preserving the existing behavior of Command execution and TempDir
handling.
Source: Coding guidelines
Owner
There was a problem hiding this comment.
@coderabbitai Has this now been resolved in the latest commit?
Use codegraph analysis to determine your answer.
If this comment is now resolved, please mark it as such using the API. Otherwise, please provide an AI agent prompt for the remaining work to be done to address this comment.
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- Codegraph: module outline ---'
ast-grep outline tests/static_regex_lint.rs --items all --view expanded || true
echo '--- Codegraph: imports and path/filesystem call sites ---'
ast-grep run --lang rust --pattern 'use std::{ $$$ };' tests/static_regex_lint.rs || true
ast-grep run --lang rust --pattern 'std::fs::$FUNC($$$ARGS)' tests/static_regex_lint.rs || true
ast-grep run --lang rust --pattern 'std::path::$TYPE' tests/static_regex_lint.rs || true
rg -n -C 3 '\b(std::(?:fs|path)|(?:fs|Path|PathBuf)::|Utf8Path|Utf8PathBuf|cap_std)' tests/static_regex_lint.rs
echo '--- Dependencies relevant to the required APIs ---'
fd -a '^Cargo\.toml$' . -x sh -c '
echo "### $1"
rg -n -C 2 "^(camino|cap-std|tempfile)\s*=" "$1" || true
' sh {}
echo '--- Review thread for the supplied discussion URL ---'
gh api graphql -F owner='leynos' -F name='mdtablefix' -F number=415 -f query='
query($owner: String!, $name: String!, $number: Int!) {
repository(owner: $owner, name: $name) {
pullRequest(number: $number) {
reviewThreads(first: 100) {
nodes {
id
isResolved
comments(first: 20) {
nodes { databaseId author { login } body path line originalLine }
}
}
}
}
}
}' --jq '
.data.repository.pullRequest.reviewThreads.nodes[]
| select(any(.comments.nodes[]; .databaseId == 3653618104))
'Length of output: 6314
@leynos, the comment remains unresolved. Codegraph analysis shows that tests/static_regex_lint.rs still imports std::path::{Path, PathBuf} and uses std::fs::{read_to_string, write, set_permissions, Permissions} at Lines 54, 61, 121, and 125. The current Cargo.toml output also shows no declared camino or cap-std dependency. I have left GitHub review thread PRRT_kwDOO7HqGM6T5p3b unresolved.
Use this AI agent prompt to complete the remaining work:
Verify the current implementation before editing. Address the unresolved CodeRabbit finding in `tests/static_regex_lint.rs` by replacing `std::path::{Path, PathBuf}` with `camino` path types and replacing all `std::fs` operations with capability-oriented `cap_std::fs_utf8` APIs.
Preserve the test behaviour:
- Keep `std::process::Command` for subprocess execution.
- Keep `tempfile::TempDir` for isolated temporary directories.
- Adapt `TempDir` paths explicitly and safely to the chosen UTF-8/capability-oriented path APIs.
- Preserve fixture loading, temporary `.rs` materialization, ripgrep-stub creation, and Unix executable-permission setup.
- Add the required `camino` and `cap-std` dependencies to the appropriate `Cargo.toml` if they are absent.
- Avoid `unwrap()` in production code; test-only `.expect(...)` calls are acceptable with useful messages.
- Keep the module documentation and public API documentation requirements intact.
Run `make check-fmt`, `make lint`, and `make test` after the changes. Report any unavailable validation command rather than claiming it passed.
You are interacting with an AI system.
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>
3 tasks
Collaborator
|
@coderabbitai review |
Contributor
✅ Action performedReview finished.
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@tests/static_regex_lint.rs`:
- Line 58: Update the Rust documentation comment for the fixture materialization
helper from “Materialise” to “Materialize,” preserving the repository’s
en-GB-oxendict spelling convention.
- Around line 66-79: The run_guard helper currently accepts only a path, so
tests cannot verify RG commands with arguments. Update run_guard and its callers
to accept an argument-bearing RG command or argv, preserve clearing RG when
unset, and add substantive regression coverage with a stub that asserts the
restored “rg --pcre2” command arguments used by the guard and Makefile.
🪄 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: bb427583-e1b3-420d-a0b0-b4c6c433d8fa
📒 Files selected for processing (11)
Makefiledocs/developers-guide.mdscripts/check-static-regexes.shtests/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/static_regex_lint.rs
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>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
The
check-static-regexesguard inmake lintonly detected theLazyLock::new(|| Regex::new(...))form, so a hand-rolled static regularexpression wrapped in
once_cell::sync::Lazy::new(|| Regex::new(...))slippedpast the lint and bypassed the repository's
lazy_regex!convention.This change broadens the scan to reject both supported lazy-wrapper
constructors —
std::sync::LazyLock::newandonce_cell::sync::Lazy::new—whether spelled directly or fully qualified.
lazy_regex!remains the solesanctioned idiom for static regex declarations.
Closes #410.
Changes
scripts/check-static-regexes.shas a single source oftruth, and broaden its pattern from
LazyLockto(?:LazyLock|Lazy)so bothwrapper families are rejected. The Makefile target now delegates to the
script; the existing diagnostic, clean-scan, and ripgrep-failure behaviours
are preserved unchanged.
tests/static_regex_lint.rs, which drives the script against fixturesfor every supported wrapper form (direct and fully qualified
LazyLockand
once_cellLazy), asserts a cleanlazy_regex!source passes, andasserts a ripgrep scan failure propagates its exit status.
tests/data/static_regex/with a.rs.txtextension sothe guard (which scans
*.rs) does not match them in place; the test copieseach into a temp dir as a
.rsfile before scanning.docs/developers-guide.md.Acceptance criteria
LazyLock::new(|| Regex::new(...))static regex is rejected.once_cell::sync::Lazy::new(|| Regex::new(...))static regex is rejected, including the qualified constructor spelling.make lintretains its existing result and diagnostic behaviour for clean scans, prohibited declarations, and ripgrep failures.check-fmt,lint,test,markdownlint,nixie); CodeRabbit review returned 0 findings.References
🤖 Generated with Claude Code