[eslint-miner] eslint-factory: add require-lastindex-reset-before-global-exec-loop rule - #52875
Draft
github-actions[bot] wants to merge 1 commit into
Conversation
Adds a new custom ESLint rule that flags module-scoped regexes with the
'g' or 'y' flag that are consumed in a `while ((match = RE.exec(str)))`
loop without an explicit `RE.lastIndex = 0` reset beforehand.
Motivation: `.exec()` on a stateful ('g'/'y') regex resumes scanning
from `lastIndex` on each call. When such a regex is declared at module
scope and reused across multiple invocations of the enclosing
function, a prior call that exits early (throws, returns before the
loop drains, or scans shorter input) leaves `lastIndex` pointing past
position 0. The next call then silently skips content or misses
matches entirely, since scanning resumes mid-string instead of from
the start.
Found via a real, unfixed instance of this bug in
actions/setup/js/temporary_id.cjs: `TEMPORARY_ID_CANDIDATE_PATTERN` is
correctly reset with `.lastIndex = 0` at its two call sites, but the
sibling `TEMPORARY_ID_PATTERN` (used to extract '#aw_XXXX' temporary ID
references from safe-output message text) has no such reset in its
exec loop. Running lint:setup-js with the new rule enabled confirms it
fires exactly once, precisely on that call site, with zero false
positives across the rest of actions/setup/js.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.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
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
Adds one new custom ESLint rule:
require-lastindex-reset-before-global-exec-loop.It flags module-scoped regex literals declared with the
goryflag that are consumed in the commonwhile ((match = RE.exec(str)) !== null)idiom without an explicitRE.lastIndex = 0reset beforehand.Why
.exec()on a stateful (g/y) regex resumes scanning fromRE.lastIndexon every call. When such a regex is declared at module scope (shared across every invocation of the enclosing function), and a prior call exits the loop early — throws, returns before exhausting matches, or is run against shorter input —lastIndexis left pointing partway through the previous string. The next call then silently resumes scanning mid-string instead of from position 0, which can skip content entirely or miss matches, with no error raised.Real bug found in
actions/setup/jstemporary_id.cjsdeclares two sibling module-level global regexes:TEMPORARY_ID_CANDIDATE_PATTERNis correctly reset with.lastIndex = 0before both of its exec loops.TEMPORARY_ID_PATTERN, used inextractTemporaryIdReferences()to pull#aw_XXXXtemporary-ID references out of safe-output messagebody/title/descriptiontext, has no such reset in its exec loop — a genuine, unfixed latent bug that can cause safe-output temporary ID cross-references to be silently dropped depending on call history.Running
npm run lint:setup-jswith the new rule enabled confirms it fires exactly once, precisely at that call site (temporary_id.cjs:649), with zero false positives across the rest ofactions/setup/js.Changes
eslint-factory/src/rules/require-lastindex-reset-before-global-exec-loop.ts— new rule implementationeslint-factory/src/rules/require-lastindex-reset-before-global-exec-loop.test.ts— unit tests (valid: reset present, non-global regex, function-local regex; invalid: missing reset, including a case modeled directly on the realtemporary_id.cjsbug, and theysticky flag variant)eslint-factory/src/index.ts— registers the new rule in the plugineslint-factory/eslint.config.cjs— enables the rule aswarnforactions/setup/jsThis PR only adds the lint rule; it does not fix the flagged bug in
temporary_id.cjs, per the mining task scope.Validation
cd eslint-factory && npm installcd eslint-factory && npm run build— passescd eslint-factory && npx vitest run src/rules/require-lastindex-reset-before-global-exec-loop.test.ts— 2/2 passingcd eslint-factory && npm run lint:setup-js— new rule fires once (the real bug above), no false positives; pre-existing unrelated warnings unaffectedNote: the full
vitest runsuite has 5 pre-existing failures inrequire-fs-io-try-catch.test.tsunrelated to this change (verified before and after).Warning
Firewall blocked 1 domain
The following domain was blocked by the firewall during workflow execution:
proxy.golang.orgTo allow these domains, add them to the
network.allowedlist in your workflow frontmatter:See Network Configuration for more information.