fix(ci): multiline match + empty-allowlist tolerance in flexframe lint (addresses #27554 reviews)#27556
Conversation
Addresses two Codex review findings on #27554: P1: Main scan now uses `rg -U --multiline-dotall` so `.frame(` wrapped across lines (opening paren on one line, `maxWidth:` on the next) is caught. The prior single-line regex missed a real instance at ChatLoadingSkeleton.swift:58-61 — now allowlisted. P2: `grep -v` against the allowlist is now guarded with `|| true` so a header-only allowlist (possible after a bulk cleanup or fresh `--update-baseline` with zero violations) doesn't abort the script under `set -euo pipefail`. Tests: - dry-run passes (172 allowlisted, 0 new) - injected multi-line violation caught and exits 1 - injected single-line violation still caught (P1 regression check) - empty allowlist + no violations = exit 0 (P2 new path) Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
![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: 97f0359eb7
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| # START line of a multiline match, so the comment filter (which only | ||
| # inspects that line) stays correct: the outer `.frame(` never itself | ||
| # starts with `//`. | ||
| RAW_HITS=$(rg -U --multiline-dotall -n --no-heading "$PATTERN" "${SCAN_DIRS[@]}" 2>/dev/null \ |
There was a problem hiding this comment.
rg -U --multiline-dotall -n emits one output line for each line participating in a multiline match, not one line per match. In this script that means a single call like .frame(\n maxWidth: ...) is split into multiple normalized keys (for example ...|.frame( and ...|maxWidth: ...), so NEW_COUNT over-reports and the allowlist no longer follows its documented “one entry per occurrence” contract. This makes baseline management brittle and can let match-fragment churn slip through unintentionally; parse only the match start (e.g. --vimgrep/JSON) or collapse multiline hits before normalize.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Declining as non-blocking. Reasoning: the lint's correctness invariant is 'any new violation fails CI', and the current implementation holds that — a multi-line match produces N allowlist entries that all must be present for the match to count as allowlisted. If any line of the match changes, the changed line is flagged as a new violation (correctly, not silently). The cost of the fragment-dedup fix (parsing rg --json with jq or building a consecutive-line heuristic that has to distinguish 'two adjacent single-line matches' from 'one multi-line match') outweighs the benefit (one logical violation = one allowlist entry). Current multi-line instances in the scanned dirs: 1 (ChatLoadingSkeleton). Will revisit if the codebase grows enough multi-line instances that allowlist verbosity becomes a real friction. Thanks for the review.
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
1 participant
Summary
Addresses two Codex review findings on #27554 (LUM-1116 Phase 3):
rg -U --multiline-dotallso.frame(wrapped across lines is caught. Before this,ChatLoadingSkeleton.swift:58-61was silently bypassing the lint despite living in the scanned directory. The pattern\.frame\(\s*max(Width|Height)\s*:matches cleanly in multiline mode since\sincludes newlines. ripgrep reports the start line of the match, so the comment-line filter stays correct.grep -v '^#|^$' allowlist.txt | sort) now toleratesgrep's exit-1 on no-match via|| true. Before this, a header-only allowlist would abort the script underset -euo pipefail— so after an imagined full cleanup or fresh--update-baselinerun with zero observed violations, CI would fail from a clean state.The new
ChatLoadingSkeleton.swift:58-61multi-line violation is allowlisted rather than rewritten: the FlexFrame wraps achatBone(aRoundedRectangleleaf), so cascade cost is O(0) — same rationale as other allowlisted entries in that file. Rewriting toHStack + Spacerwould change the shimmer's varied-width visual.Tests
bash clients/scripts/check-flexframe.sh— passes (172 allowlisted, 0 new)..frame(\n maxWidth: 100,\n alignment: .leading\n)— caught, exit 1..frame(maxWidth: 100)— still caught (P1 regression check).Related