fix(eslint): escape dynamic RegExp interpolations in actions/setup/js#49678
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Resolves issue #49669 by safely handling dynamic RegExp interpolation in setup scripts.
Changes:
- Escapes interpolated literal values before constructing regular expressions.
- Documents and suppresses warnings for intentional regex fragments.
Show a summary per file
| File | Description |
|---|---|
actions/setup/js/comment_memory_helpers.cjs |
Escapes the memory tag and documents the numeric quantifier. |
actions/setup/js/frontmatter_hash_pure.cjs |
Escapes frontmatter keys. |
actions/setup/js/fuzz_template_substitution_harness.cjs |
Escapes template variable names. |
actions/setup/js/glob_pattern_helpers.cjs |
Documents intentional glob-derived regex interpolation. |
actions/setup/js/interpolate_prompt.cjs |
Fully escapes variable and experiment expressions. |
actions/setup/js/model_aliases.cjs |
Escapes qualifiers and documents version regex fragments. |
actions/setup/js/resolve_model_alias.cjs |
Documents intentional escaped glob interpolation. |
actions/setup/js/safe_output_type_validator.cjs |
Documents composed issue-reference regex fragments. |
actions/setup/js/sanitize_content_core.cjs |
Documents intentional fence regex components. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 9/9 changed files
- Comments generated: 0
- Review effort level: Balanced
|
@copilot quick triage for this PR:
Run details: https://github.com/github/gh-aw/actions/runs/30724810309
|
🔍 Triage Summary
Escapes dynamic RegExp interpolations per eslint rule. CI green, reviewer commented. Automated triage — see the triage report for full context.
|
|
@copilot ensure there is a comprehensive test suite for each of these regexes |
|
|
|
✅ 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 (default_business_additions=0). |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
✅ Test Quality Sentinel completed test quality analysis. No action needed: This PR contains only production code changes to 9 JavaScript files in actions/setup/js/. No test files were added or modified. Test Quality Sentinel evaluation is not applicable for production-only PRs. |
![github-actions[bot]](https://cdn.github.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
The changes correctly fix all dynamic RegExp interpolation warnings. Literal text values are properly escaped via the standard /[.*+?^${}()|[\]\]/g pattern, and the eslint-disable suppressions each carry clear rationale distinguishing intentional regex fragments from literal inputs. No blocking issues found.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 · 15.2 AIC · ⌖ 10.3 AIC · ⊞ 5.4K
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /tdd — one observation on test coverage, otherwise the changes look correct.
📋 Key Themes & Highlights
Key Themes
- Correct escaping strategy: The inline
.replace(/[.*+?^${}()|[\]\]/g, "\\$&")pattern is the right idiom for escaping arbitrary text before interpolating into aRegExp. - Well-reasoned suppressions: Every
eslint-disablecomment explains why the interpolation is intentional (regex fragment, numeric quantifier, already-escaped value). This is exactly the right approach. - Missing regression test:
parseBoolFromFrontmatter(and the similarinterpolateVariablesfunctions) now correctly escape keys/var names, but no test exercises a key that contains a regex-special character. A small test would lock in the fix and prevent silent regressions.
Positive Highlights
- ✅ Consistent application across all 9 affected files
- ✅ Clear, readable constant names (
ESCAPED_COMMENT_MEMORY_TAG) for pre-computed escape results - ✅
eslint-disablecomments are targeted and well-documented
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 · 25.7 AIC · ⌖ 9.7 AIC · ⊞ 7.1K
Comment /matt to run again
| @@ -39,7 +39,8 @@ async function defaultFileReader(filePath) { | |||
| * @returns {boolean} | |||
| */ | |||
There was a problem hiding this comment.
[/tdd] No regression test covers a key containing regex-special characters (e.g. key = "a.b"). Before this fix, such a key would silently match unrelated frontmatter lines; adding a test locks in the behaviour.
💡 Suggested test
it('parseBoolFromFrontmatter handles regex-special chars in key', () => {
expect(parseBoolFromFrontmatter('a.b: true', 'a.b')).toBe(true);
// Before this fix, 'a.b' matched 'aXb: true' because '.' was unescaped
expect(parseBoolFromFrontmatter('aXb: true', 'a.b')).toBe(false);
});@copilot please address this.
🧪 Test Quality Sentinel Reporti️ Test Quality Score: N/A — No Tests Changed
📊 PR Summary
Files Modified:
Verdict
|
Resolves a cluster of
gh-aw-custom/require-escaped-regexp-interpolationwarnings across 9 files inactions/setup/jswhere values were interpolated intonew RegExp()template literals without regex escaping.Escaped — dynamic literal fragments
Values that represent plain text (variable names, key names, qualifier strings) are now passed through
.replace(/[.*+?^${}()|[\]\\]/g, "\\$&")before interpolation:comment_memory_helpers.cjsCOMMENT_MEMORY_TAG→ newESCAPED_COMMENT_MEMORY_TAGconstantfrontmatter_hash_pure.cjskeyparameterfuzz_template_substitution_harness.cjsvarNameinterpolate_prompt.cjsvarName;exprForm(was dot-only escape, now full)model_aliases.cjsqualifierSuppressed — intentional regex fragments
Interpolations that are valid regex components (not literal text) are suppressed with targeted
eslint-disablecomments explaining the rationale:VERSION_SUFFIX_PATTERNinmodel_aliases.cjs— an explicit regex fragment stringISSUE_CLOSING_KEYWORDS/ISSUE_REFERENCE_PATTERNinsafe_output_type_validator.cjs— composed regex alternation patternsregexPatterninglob_pattern_helpers.cjs— output of glob-to-regex conversionescapeRegex(...).replace(/\*/g, "[^/]*")inresolve_model_alias.cjs— already escaped;*→[^/]*is intentionalfc/fenceLeninsanitize_content_core.cjs— fence character in a char class and numeric quantifierMAX_MEMORY_ID_LENGTHincomment_memory_helpers.cjs— numeric constant used as{1,N}quantifier