Standardize static regex declarations (#363) - #404
Conversation
Route every static regular expression through `lazy_regex!` so initialization uses one idiom and reports a pattern-specific failure. Document and enforce the convention, and lock the non-trivial fence capture contract with generated round-trip coverage.
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
|
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 PR standardises static regex declarations on ChangesStatic regex standardisation
Possibly related PRs
Suggested labels: Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 20✅ Passed checks (20 passed)
📋 Issue PlannerBuilt with CodeRabbit's Coding Plans for faster development and fewer bugs. View plan used: ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
@coderabbitai review |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Scan every Rust source for qualified hand-rolled static regex declarations and fail closed when the scanner cannot complete. Migrate the missed integration-test regex and document each inline module so the repository-wide policy checks hold consistently.
Install a pinned ripgrep release through the cargo-binstall provisioned by the shared Rust setup action so the static-regex lint guard can run.
|
@coderabbitai Have the following failed checks now been resolved? If further work is required, please provide an AI agent prompt for the remaining work to be done to address these failures. Do not treat warnings as optional or aspirational. Where a change is out of scope for this PR, propose a GitHub issue unless one exists already. (Treat documentation and validation coverage as in scope). ❌ Failed checks (1 error, 1 warning, 1 inconclusive)
|
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '== PR diff summary =='
git diff --stat "$(git merge-base HEAD origin/main)" HEAD 2>/dev/null || git diff --stat
echo
echo '== Rust module declarations =='
rg -n -U --type rust '(^|\n)\s*(?:#\[[^\]]+\]\s*)*(?:pub(?:\([^)]*\))?\s+)?mod\s+[A-Za-z_][A-Za-z0-9_]*\s*\{' src
echo
echo '== Static LazyLock regex declarations and Regex::new call context =='
rg -n -U --type rust -C 4 'static\s+[A-Z0-9_]+\s*:\s*(?:std::sync::)?LazyLock\s*<\s*Regex\s*>\s*=|LazyLock::new\s*\(\s*\|\|\s*Regex::new\s*\(' src
echo
echo '== reflow module outline and relevant source =='
if [ -f src/reflow.rs ]; then
ast-grep outline src/reflow.rs --items all
sed -n '1,280p' src/reflow.rs
fi
echo
echo '== Documentation convention and static-regex gate =='
sed -n '1,260p' docs/developers-guide.md
sed -n '1,240p' Makefile
echo
echo '== Direct scan for hand-rolled static regex construction =='
rg -n -U --type rust 'LazyLock::new\s*\(\s*\|\|\s*Regex::new\s*\(' src || trueLength of output: 44368 🏁 Script executed: #!/bin/bash
set -euo pipefail
base="$(git merge-base HEAD origin/main 2>/dev/null || true)"
if [ -n "$base" ]; then
echo '== Documentation-related PR diff =='
git diff --unified=3 "$base" HEAD -- \
src/breaks.rs src/code_emphasis.rs src/ellipsis.rs src/ellipsis/protected.rs \
src/footnotes/mod.rs src/frontmatter.rs src/headings.rs src/io.rs src/lists.rs \
src/process.rs src/table.rs src/textproc.rs src/wrap/block.rs src/wrap/fence.rs \
src/wrap/inline/fragment.rs src/wrap/inline/normalize.rs \
src/wrap/inline/predicates.rs src/wrap/link_reference.rs \
src/wrap/paragraph/code_span_trim.rs src/wrap/tokenize/link_span.rs \
tests/cli_matrix/invariants.rs tests/cli_matrix/support.rs
fi
echo
echo '== All LazyLock construction sites in Rust sources =='
rg -n -U --type rust -C 3 '\bLazyLock::new\s*\(' src tests || true
echo
echo '== Static Regex declarations not visibly initialized with lazy_regex! =='
rg -n -U --type rust -C 2 '\bstatic\b[^;=]*\bRegex\b[^;=]*=' src tests || true
echo
echo '== Nested module declarations lacking an adjacent preceding //! doc line =='
python3 - <<'PY'
from pathlib import Path
import re
module = re.compile(r'^\s*(?:pub(?:\([^)]*\))?\s+)?mod\s+[A-Za-z_][A-Za-z0-9_]*\s*(?:\{|;)\s*$')
for root in (Path("src"), Path("tests")):
if not root.exists():
continue
for path in root.rglob("*.rs"):
lines = path.read_text(encoding="utf-8").splitlines()
for i, line in enumerate(lines):
if not module.match(line):
continue
j = i - 1
while j >= 0 and (not lines[j].strip() or lines[j].lstrip().startswith("#[")):
j -= 1
if j < 0 or not lines[j].lstrip().startswith("//!"):
print(f"{path}:{i + 1}: {line.strip()}")
PYLength of output: 31902
No further implementation work or follow-up GitHub issue is required for these findings. Re-run the repository’s full validation gate in CI to record the final executable verification. |
|
@coderabbitai review |
✅ Action performedReview finished.
|
Add an explicit ripgrep availability check before scanning Rust sources. Document the lint guard and its local and CI tool prerequisite so contributors receive a clear failure when `rg` is unavailable.
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Makefile (1)
44-52: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winBroaden
check-static-regexesbeyondLazyLock::new.
Match the other sanctioned lazy-wrapper shapes too;once_cell::sync::Lazy::newcan slip past this guard. Add a regression case for each accepted form.🤖 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 `@Makefile` around lines 44 - 52, Broaden the regex in the Makefile target check-static-regexes to detect sanctioned lazy-wrapper forms beyond LazyLock::new, including once_cell::sync::Lazy::new and the other accepted wrapper shapes. Add regression cases covering each supported form, while preserving the existing rejection and ripgrep-error handling.
🤖 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.
Outside diff comments:
In `@Makefile`:
- Around line 44-52: Broaden the regex in the Makefile target
check-static-regexes to detect sanctioned lazy-wrapper forms beyond
LazyLock::new, including once_cell::sync::Lazy::new and the other accepted
wrapper shapes. Add regression cases covering each supported form, while
preserving the existing rejection and ripgrep-error handling.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 6eea5718-2585-4b5c-b123-62cc7d62e657
📒 Files selected for processing (2)
Makefiledocs/developers-guide.md
|
@coderabbitai please create a new GitHub issue for the following: Please address the following concerns and ensure all commit gates succeed: Makefile (1)
Match the other sanctioned lazy-wrapper shapes too; 🤖 Detailed instructionsVerify each finding against current code. Fix only still-valid issues, skip the In |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '== repository =='
git remote get-url origin
echo
echo '== Makefile static-regex guard =='
nl -ba Makefile | sed -n '35,65p'
echo
echo '== lazy-wrapper regex construction sites =='
rg -n -U --type rust -C 2 \
'(?:LazyLock|Lazy|OnceLock)::new\s*\(\s*\|\|\s*(?:Regex|regex::Regex)::new\s*\(' \
src tests || true
echo
echo '== static-regex guard references and regression coverage =='
rg -n -i -C 3 \
'check-static-regexes|static.regex|lazylock|once_cell::sync::Lazy|Lazy::new' \
Makefile tests src docs || true
echo
echo '== originating PR comment permalink =='
gh api repos/leynos/mdtablefix/issues/404/comments \
--paginate \
--jq '.[] | select(.user.login == "leynos" and (.body | contains("Broaden `check-static-regexes` beyond `LazyLock::new`."))) | .html_url'Length of output: 1254 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '== Makefile static-regex guard (Lines 35–65) =='
sed -n '35,65p' Makefile
echo
echo '== lazy-wrapper regex construction sites =='
rg -n -U --type rust -C 2 \
'(?:LazyLock|Lazy|OnceLock)::new\s*\(\s*\|\|\s*(?:Regex|regex::Regex)::new\s*\(' \
src tests || true
echo
echo '== static-regex guard references and regression coverage =='
rg -n -i -C 3 \
'check-static-regexes|static.regex|lazylock|once_cell::sync::Lazy|Lazy::new' \
Makefile tests src docs || true
echo
echo '== originating PR comment permalink =='
gh api repos/leynos/mdtablefix/issues/404/comments \
--paginate \
--jq '.[] | select(.user.login == "leynos" and (.body | contains("Broaden `check-static-regexes` beyond `LazyLock::new`."))) | .html_url' || trueLength of output: 20544 🏁 Script executed: #!/bin/bash
set -euo pipefail
repo="leynos/mdtablefix"
issue_url="$(
gh issue create \
--repo "$repo" \
--assignee "leynos" \
--title "Broaden static-regex lint guard to cover supported lazy wrappers" \
--body "$(cat <<'EOF'
## Summary
Broaden `Makefile`'s `check-static-regexes` scan so hand-rolled static regex declarations cannot bypass the repository's `lazy_regex!` convention by using lazy-wrapper constructors other than `std::sync::LazyLock::new`.
## Rationale
The current pattern at `Makefile` Line 46 only detects `LazyLock::new(... Regex::new(...))`. A declaration using `once_cell::sync::Lazy::new(|| Regex::new(...))` is not detected, allowing a prohibited static-regex construction to pass `make lint`.
## Affected area
- `Makefile`: `check-static-regexes`
- Add focused regression coverage or fixtures for the lint pattern.
## Required changes
1. Extend the scan to detect direct and fully qualified supported lazy-wrapper constructors, including `once_cell::sync::Lazy::new`, in addition to `LazyLock::new`.
2. Define the supported wrapper forms in the regression coverage so each form is rejected when it directly wraps `Regex::new`.
3. Preserve the existing behaviour:
- a matching prohibited declaration fails with the existing static-regex diagnostic;
- no matches succeed;
- ripgrep scan failures propagate with the existing error handling.
4. Keep the change minimal and retain `lazy_regex!` as the sole sanctioned idiom for static regex declarations.
5. Run every applicable formatting, lint, and test gate before closing the issue.
## Acceptance criteria
- [ ] A hand-rolled `LazyLock::new(|| Regex::new(...))` static regex is rejected.
- [ ] A hand-rolled `once_cell::sync::Lazy::new(|| Regex::new(...))` static regex is rejected, including qualified constructor spelling.
- [ ] Regression coverage exercises every lazy-wrapper constructor form supported by the guard.
- [ ] `make lint` retains its existing result and diagnostic behaviour for clean scans, prohibited declarations, and ripgrep failures.
- [ ] All applicable commit gates pass.
## Backlinks
- Originating pull request: https://github.com/leynos/mdtablefix/pull/404
- Originating review comment: https://github.com/leynos/mdtablefix/pull/404#issuecomment-5062833885
- Requested by: `@leynos`
EOF
)"
)"
printf '%s\n' "$issue_url"Length of output: 2543
The issue records the verified bypass, requires regression coverage for each guarded wrapper form, preserves existing scan-error handling, and requires all applicable commit gates to pass. |
3 participants
Summary
This branch standardizes static regular-expression declarations on the
canonical
lazy_regex!macro so initialization uses one idiom and failuresidentify the pattern that could not compile.
Closes #363.
It also documents and enforces the convention, and adds generated coverage for
the non-trivial wrapping-fence capture contract.
Review walkthrough
src/breaks.rs,src/lists.rs,src/fences.rs,src/table.rsandsrc/wrap/fence.rs.Makefile,which makes the static-regex check a prerequisite of
make lint.docs/developers-guide.mdand the fence-capture property test in
src/wrap/fence.rs.Validation
make check-fmt: passed.make test: passed, including 676 library tests and the integration,property, snapshot and trybuild suites.
make typecheck: passed.make lint: passed, includingcheck-static-regexes.make markdownlint: passed with no errors.make nixie: passed.mbake validate Makefile: passed.cargo test fence_captures_round_trip_generated_delimiters: passed; adeliberate temporary three-to-four-character fence-threshold mutation was
detected before restoration.
coderabbit review --agent --type committed --base origin/main: passedwith zero findings on commit
2575e80.Notes
migration with no state transitions or bounded structural invariant.
behavioural suites cover the affected table, list, fence, break and wrapping
paths and remained green.
References