fix(eslint): require-escaped-regexp-interpolation — const-binding resolution to eliminate false positives - #49782
Conversation
|
Thanks for starting this fix! 🚀 I see this is a draft PR with a detailed task checklist for the eslint-factory rule improvement (addressing #49281). Status: This PR is currently in draft mode with no code changes yet. The task checklist outlines the work needed:
What's needed to move forward:
Since this is an agentic workflow PR, keep
|
…st bindings - Add `findConstInitializer` to resolve const variable bindings via scope analysis - Add `isLiteralSafeForRegexp` to recognize safe literal values (numbers, booleans, metachar-free strings) - Extend `isRecognizedAsEscaped` with one level of const-binding resolution: const vars initialized to an escape-helper call or safe literal are now treated as safe - Add TSESLint import for SourceCode type - Add test coverage for all new valid and invalid paths - Fix glob_pattern_helpers.cjs line 74 with eslint-disable comment (intentional regex pattern building) Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Improves the regex-interpolation ESLint rule by resolving immutable constants while preserving warnings for mutable or unsafe values.
Changes:
- Resolves one-level
constinitializers. - Adds literal-safety handling and boundary tests.
- Documents an intentional glob-regex suppression.
Show a summary per file
| File | Description |
|---|---|
eslint-factory/src/rules/require-escaped-regexp-interpolation.ts |
Adds constant and literal resolution. |
eslint-factory/src/rules/require-escaped-regexp-interpolation.test.ts |
Adds valid and invalid boundary cases. |
actions/setup/js/glob_pattern_helpers.cjs |
Suppresses an intentional regex interpolation. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 3/3 changed files
- Comments generated: 1
- Review effort level: Balanced
| function isLiteralSafeForRegexp(node: TSESTree.Node): boolean { | ||
| if (node.type !== AST_NODE_TYPES.Literal) return false; | ||
| const { value } = node; | ||
| if (typeof value === "number" || typeof value === "boolean") return true; |
|
✅ Test Quality Sentinel completed test quality analysis. |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR does not have the 'implementation' label and has ≤100 new lines of code in business logic directories (0 additions detected in default business logic paths). |
|
✅ PR Code Quality Reviewer completed the code quality review. |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
The overall approach is sound — const-binding resolution is implemented conservatively (single def, no post-init writes, one level deep) and the test coverage is thorough.
One blocking issue: isLiteralSafeForRegexp incorrectly treats all numeric literals as safe. Numbers like 1.5 or 1e21 stringify to "1.5" and "1e+21" in template literals, both containing the . metacharacter — which is in REGEXP_META_CHARS. The fix is to check the string representation of the number: String(value), running it through the same per-character metacharacter check used for strings. The existing inline review comment on line 148 captures this.
Once that is corrected the PR is ready to merge.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 · 25.2 AIC · ⌖ 12.1 AIC · ⊞ 5.4K
🧪 Test Quality Sentinel Report✅ Test Quality Score: 100/100 — Excellent
📊 Metrics (7 tests)
Verdict
|
There was a problem hiding this comment.
✅ Test Quality Sentinel: 100/100 — Excellent. All 7 new tests verify the const-binding resolution design contract. 100% design test coverage, 0% implementation tests (threshold: 30%). Test-to-production ratio 0.99:1 (threshold: 2:1). No violations.
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /diagnosing-bugs — requesting changes on two correctness gaps before merge.
📋 Key Themes & Highlights
Issues
isEscapedNameReference(init)omitted (line 215): theescapedXnaming convention is not tested against the resolved initializer, leavingconst bar = escapedPatternas a false positive.- Numeric literals with metachar coercions (line 148):
1.5coerces to"1.5"and1e21to"1e+21", both containing.or+; the blankettypeof value === "number"guard silently suppresses these.
Positive Highlights
- ✅ Scope-chain walk with single-def + zero-post-init-write guard is a sound soundness bar — matches the existing alias-tracking pattern in the codebase.
- ✅ One-level-only resolution is a deliberate and appropriate conservatism.
- ✅ Test suite covers
let, reassignedconst, metachar strings, and non-escape initializers — good boundary coverage. - ✅ The
glob_pattern_helpers.cjsdisable comment is well-explained and intentional.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 · 36.2 AIC · ⌖ 8.12 AIC · ⊞ 7.1K
Comment /matt to run again
| if (sourceCode && node.type === AST_NODE_TYPES.Identifier) { | ||
| const init = findConstInitializer(sourceCode, node); | ||
| if (init !== null) { | ||
| if (isEscapeHelperCall(init) || isRegexEscapeReplaceCall(init) || isLiteralSafeForRegexp(init)) return true; |
There was a problem hiding this comment.
[/diagnosing-bugs] isEscapedNameReference(init) is missing from the initializer check — const bar = escapedFoo; remains a false positive because escapedFoo is an Identifier whose name starts with escaped, yet only isEscapeHelperCall, isRegexEscapeReplaceCall, and isLiteralSafeForRegexp are tested against init.
💡 Suggested fix
if (isEscapeHelperCall(init) || isRegexEscapeReplaceCall(init) || isEscapedNameReference(init) || isLiteralSafeForRegexp(init)) return true;Add a matching test case:
"const escapedName = escapedPattern; new RegExp(`^${escapedName}$`);"@copilot please address this.
| function isLiteralSafeForRegexp(node: TSESTree.Node): boolean { | ||
| if (node.type !== AST_NODE_TYPES.Literal) return false; | ||
| const { value } = node; | ||
| if (typeof value === "number" || typeof value === "boolean") return true; |
There was a problem hiding this comment.
[/diagnosing-bugs] (Corroborating the existing comment) Number literals that contain . or + after coercion (1.5 → "1.5", 1e21 → "1e+21") will silently pass isLiteralSafeForRegexp even though their string representation contains regex metacharacters. The literal value 1.5 passes the typeof value === "number" guard but its regex expansion is 1.5, where . matches any character.
💡 Suggested fix
Convert the number to its string representation and run it through the metachar check:
if (typeof value === "number") {
const s = String(value);
for (const char of s) {
if (REGEXP_META_CHARS.has(char)) return false;
}
return true;
}Or restrict to integer literals only:
if (typeof value === "number") return Number.isInteger(value) && value >= 0;Add test cases:
// invalid — 1.5 coerces to "1.5" which contains "."
"const STEP = 1.5; new RegExp(`a{${STEP}}`);"@copilot please address this.
There was a problem hiding this comment.
Review: no new blocking issues found
The const-binding resolution logic (findConstInitializer) is sound for its stated one-level scope, and test coverage for let/reassignment/non-escape-initializer/metachar-literal cases is solid.
💡 Notes
- There is already an unresolved review comment (id 3699223566) on line 148 pointing out that
isLiteralSafeForRegexptreats allnumber/booleanliterals as regex-safe without validating their stringified form —1e21→"1e+21"and any decimal like1.5→"1.5"both contain regex metacharacters (+,.). I confirmed this is a real false negative (String(1e21)="1e+21",String(1.5)="1.5"). This should be fixed before merge, but I'm not duplicating that comment. - The single-level-only const resolution (no recursive
const b = achaining) is explicitly documented as an intentional scope limitation in the docstring, not a defect — acceptable as-is. glob_pattern_helpers.cjsalready has broad existing test coverage forglobPatternToRegex, so the new eslint-disable suppression there is adequately backstopped.
No new comments added since the only confirmed correctness issue (numeric-literal false negative) is already tracked by an existing pending review comment.
🔎 Code quality review by PR Code Quality Reviewer · auto · 105.5 AIC · ⌖ 6.63 AIC · ⊞ 7.9K
Comment /review to run again
|
🎉 This pull request is included in a new release. Release: |
isRecognizedAsEscapedperformed no data-flow tracking, only inspecting the expression node at the interpolation site. Two patterns produced live false positives inactions/setup/js: aconstvariable holding the result of an escape-helper call (non-escaped*name), andconstliterals that can never contain regex metacharacters.Rule changes (
require-escaped-regexp-interpolation.ts)findConstInitializer(sourceCode, node)— walks the ESLint scope chain from anIdentifierto itsconstdeclarator; returns the initializer only when the binding has a single definition and zero post-init writes (same soundness bar asrequire-spawnsync-error-check's alias tracking).isLiteralSafeForRegexp(node)— acceptsnumber,boolean, and metachar-freestringliterals as inherently safe.isRecognizedAsEscapedextended with one level of const-binding resolution: if the expression is an identifier whoseconstinitializer is itself escape-recognized or a safe literal, it's accepted.let/var, reassignedconst, and non-resolvable initializers remain flagged.Boundary tests added
New valid cases covering both code paths; new invalid cases asserting
letbindings, reassignedconst, metachar-containing string literals, and non-escape initializers are still reported.glob_pattern_helpers.cjsline 74regexPatternis a reassignedletwhose value is an intentionally-built regex (glob*/**→[^/]*/.*after escaping other metacharacters). Added// eslint-disable-next-line gh-aw-custom/require-escaped-regexp-interpolationwith an explanatory comment since the rule is correctly detecting the metacharacters — the suppression is intentional, not a workaround.