prefer-number-isnan: treat ESM import bindings as local shadows by Copilot · Pull Request #43500 · github/gh-aw

Copilot · 2026-07-05T07:25:32Z

prefer-number-isnan treated identifiers bound only via ImportBinding as if they were globals, which created false positives for imported/shimmed isNaN/window/global in ESM and could suggest behavior-changing replacements. This change makes import-bound names follow the same shadowing semantics as other local bindings and pins that behavior with ESM rule tests.

Rule semantics
- Updated local-binding detection to treat any local definition (including ImportBinding) as shadowing the global candidate.
- This prevents diagnostics on imported/shimmed names that are not the real global symbols.
Behavior coverage (ESM)
- Added explicit valid cases for:
  - import { isNaN } from "lodash"; isNaN(value);
  - import { window } from "./browser-shim"; window.isNaN(value);
  - import { global } from "./server-shim"; global["isNaN"](value);
Intent documentation in code
- Added a focused inline comment in hasLocalBinding clarifying that local defs, including imports, shadow globals.

// Any local definition shadows the global (including ESM ImportBinding).
if (variable && variable.defs.length > 0) {
  return true;
}

…-number-isnan Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>

github-actions · 2026-07-05T07:58:29Z

✅ Test Quality Sentinel completed test quality analysis.

github-actions · 2026-07-05T07:58:31Z

⚠️ PR Code Quality Reviewer failed during code quality review.

github-actions · 2026-07-05T07:58:34Z

🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅

github-actions · 2026-07-05T07:58:35Z

✅ Design Decision Gate 🏗️ completed the design decision gate check.

No ADR enforcement needed: PR #43500 does not have the 'implementation' label and has 0 new lines of code in business logic directories (threshold: 100).

Copilot

Pull request overview

This pull request updates the prefer-number-isnan ESLint rule to treat ESM ImportBinding identifiers as locally shadowing bindings, preventing false positives when isNaN/window/global are imported or shimmed rather than referring to the real globals.

Changes:

Updated hasLocalBinding to treat any local definition (including ImportBinding) as shadowing the global candidate.
Added ESM-focused valid test cases covering imported/shimmed isNaN, window, and global.
Added an inline comment clarifying the intended shadowing semantics.

Show a summary per file

File	Description
eslint-factory/src/rules/prefer-number-isnan.ts	Treats any in-scope definition (including ESM imports) as a local shadow to avoid behavior-changing suggestions on imported/shimmed globals.
eslint-factory/src/rules/prefer-number-isnan.test.ts	Adds ESM `sourceType: "module"` valid cases to lock in the updated shadowing behavior for imports.

Review details

Tip

Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Files reviewed: 2/2 changed files
Comments generated: 0
Review effort level: Low

github-actions · 2026-07-05T08:05:15Z

🧪 Test Quality Sentinel Report

✅ Test Quality Score: 100/100 — Excellent

Analyzed 3 test scenario(s): 3 design, 0 implementation, 0 violation(s).

Note: Test file is .test.ts (TypeScript/vitest). Treated as JavaScript for scoring.

📊 Metrics (3 tests)

Metric	Value
Analyzed	3 (Go: 0, JS: 3)
✅ Design	3 (100%)
⚠️ Implementation	0 (0%)
Edge/error coverage	3 (100%)
Duplicate clusters	0
Inflation	No (1.5:1 ratio)
🚨 Violations	0

Test	File	Classification	Issues
`import { isNaN } from "lodash"; isNaN(value);`	`prefer-number-isnan.test.ts:37`	design_test / behavioral_contract / high_value	None
`import { window } from "./browser-shim"; window.isNaN(value);`	`prefer-number-isnan.test.ts:41`	design_test / behavioral_contract / high_value	None
`import { global } from "./server-shim"; global["isNaN"](value);`	`prefer-number-isnan.test.ts:42`	design_test / behavioral_contract / high_value	None

Verdict

✅ Passed. 0% implementation tests (threshold: 30%). All 3 new test scenarios directly verify the ESM ImportBinding fix as behavioral contracts, covering the exact regression the PR addresses (direct import shadow, member-access via imported object, and computed member-access via imported object).

Warning

Firewall blocked 1 domain

The following domain was blocked by the firewall during workflow execution:

awmgmcpg

To allow these domains, add them to the network.allowed list in your workflow frontmatter:

network:
  allowed:
    - defaults
    - "awmgmcpg"

See Network Configuration for more information.

🧪 Test quality analysis by Test Quality Sentinel · 24.6 AIC · ⌖ 11.7 AIC · ⊞ 6.8K · ◷
_{Comment /review to run again}

github-actions

✅ Test Quality Sentinel: 100/100. 0% implementation tests (threshold: 30%). All 3 new scenarios are high-value behavioral contracts targeting the ESM ImportBinding fix directly.

github-actions

Review: prefer-number-isnan — ESM ImportBinding fix

The change is correct and complete.

Root cause & fix

The old predicate variable?.defs.some(d => d.type !== "ImportBinding") returned false when every def was an ImportBinding, so ESM-imported names were never treated as local shadows — causing false-positive diagnostics on imported isNaN/window/global shims.

The new predicate variable && variable.defs.length > 0 is both simpler and semantically correct: any locally-defined variable (regardless of def type) shadows the global candidate.

Tests

The three new valid cases use esmRuleTester (sourceType: module), which is required for import syntax to parse — that's correct placement.

Minor gap (pre-existing, not introduced here)

There are no invalid-case ESM tests confirming that an unbound isNaN(value); in a module file is still flagged. All invalid cases run under cjsRuleTester. Adding one ESM invalid case would close this gap, but it isn't a regression introduced by this PR.

No blocking issues. LGTM.

🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · 21.5 AIC · ⌖ 5.99 AIC · ⊞ 4.9K

github-actions

Skills-Based Review 🧠

Applied /diagnosing-bugs and /tdd — the bug fix itself is correct and well-scoped; two non-blocking test-coverage gaps flagged as suggestions.

📋 Key Themes & Highlights

Key Themes

Fix is correct: Changing defs.some(d => d.type !== "ImportBinding") → variable && variable.defs.length > 0 properly treats every local definition (function, const, and ImportBinding alike) as shadowing the global. The old condition returned false for a pure ImportBinding def-list, creating the false positive.
Missing globalThis import-shadow test: window and global now have ESM import cases but globalThis does not — leaving a gap in symmetry with GLOBAL_IS_NAN_OBJECTS.
No ESM invalid tests: All invalid blocks use cjsRuleTester. Adding a small ESM invalid block would prove the rule still fires correctly in sourceType: "module" without a shadow.

Positive Highlights

✅ Minimal, surgical change — only 3 lines changed in the implementation
✅ Regression tests added for each new valid ESM pattern
✅ Intent comment clearly documents the ImportBinding inclusion rationale
✅ PR description is thorough and links behaviour to the code change

🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · 34.2 AIC · ⌖ 6.8 AIC · ⊞ 6.6K
_{Comment /matt to run again}

Comments that could not be inline-anchored

eslint-factory/src/rules/prefer-number-isnan.test.ts:42

[/tdd] The three GLOBAL_IS_NAN_OBJECTS members (globalThis, window, global) are tested for const-shadowing, but only window and global get ESM import-shadow coverage — globalThis is missing.

<details>
<summary>💡 Suggested addition</summary>

Add a parallel case to keep the set symmetric:

`import { globalThis } from &quot;./global-shim&quot;; globalThis.isNaN(value);`,

This mirrors the window/global import cases and prevents a future regression if the globalThis bra…

eslint-factory/src/rules/prefer-number-isnan.test.ts:48

[/tdd] There are no invalid ESM test cases confirming the rule still fires in ESM source when the global is not shadowed. The existing invalid block uses cjsRuleTester, so it does not exercise the ESM path.

<details>
<summary>💡 Suggested addition</summary>

Add a minimal esmRuleTester invalid block, e.g. inside the same it block or a dedicated one:

esmRuleTester.run(&quot;prefer-number-isnan&quot;, preferNumberIsNanRule, {
  valid: [],
  invalid: [
    {
      code: `isNaN(value)…

</details>

github-actions

Review: prefer-number-isnan ESM import binding fix

The core logic change is correct — is the right way to detect locally-bound identifiers in ESLint's scope model. Globals registered via environment configs appear with in the global scope, so the condition correctly skips them. The case now works as it should.

Two test coverage gaps need attention:

Findings

Medium — Missing ESM invalid test (blocking gap in regression coverage)
All invalid: tests use cjsRuleTester. The scope-walking behavior differs across sourceType values in some ESLint scope implementations. If variable.defs.length > 0 inadvertently suppresses global detection in ESM mode (e.g., ESLint's module scope providing a synthetic variable record), there is no test that would catch it. An esmRuleTester invalid: case for bare isNaN(value); and window.isNaN(value); would close this gap.

Low — Incomplete GLOBAL_IS_NAN_OBJECTS symmetry
The new ESM valid cases cover window and global import shims but skip globalThis. All three are in GLOBAL_IS_NAN_OBJECTS; the test suite should mirror that uniformly.

🔎 Code quality review by PR Code Quality Reviewer · 131.6 AIC · ⌖ 9.05 AIC · ⊞ 5.4K
_{Comment /review to run again}

github-actions · 2026-07-05T08:16:23Z

        `const window = { isNaN(value) { return value; } }; window["isNaN"](value);`,
        `const global = { isNaN(value) { return value; } }; global.isNaN(value);`,
+        `import { window } from "./browser-shim"; window.isNaN(value);`,
+        `import { global } from "./server-shim"; global["isNaN"](value);`,


Missing ESM invalid test: no test proves the rule still flags isNaN() in ESM modules after this change, leaving a correctness regression undetectable.

💡 Explanation and suggested fix

All invalid: test cases use cjsRuleTester. The fix changes scope-walking behavior, which is determined per sourceType. If the new variable && variable.defs.length > 0 branch misbehaves in ESM mode (e.g., due to how eslint scope analysis treats globals under sourceType: "module"), no test would catch it.

Consider adding to the new esmRuleTester.run block:

esmRuleTester.run("prefer-number-isnan", preferNumberIsNanRule, { valid: [ /* existing */ ], invalid: [ { code: `isNaN(value);`, errors: [{ messageId: "preferNumberIsNaN", suggestions: [{ messageId: "replaceWithNumberIsNaN", output: `Number.isNaN(value);` }] }], }, { code: `window.isNaN(value);`, errors: [{ messageId: "preferNumberIsNaN", suggestions: [{ messageId: "replaceWithNumberIsNaN", output: `Number.isNaN(value);` }] }], }, ], });

Without this, the PR's test coverage only validates the non-regression path (valid cases), not that the rule still fires in ESM when no import shadowing is present.

github-actions · 2026-07-05T08:16:23Z

        `const global = { isNaN(value) { return value; } }; global.isNaN(value);`,
+        `import { window } from "./browser-shim"; window.isNaN(value);`,
+        `import { global } from "./server-shim"; global["isNaN"](value);`,
        // Dynamic computed access — identifier property reference, not string literal "isNaN"


Incomplete test symmetry: globalThis is in GLOBAL_IS_NAN_OBJECTS but has no import-shim valid case.

💡 Suggested addition

The three new valid ESM tests cover import { isNaN }, import { window }, and import { global } — but not import { globalThis }. Since globalThis is an equal member of GLOBAL_IS_NAN_OBJECTS, a missing test here leaves asymmetric coverage.

Consider adding:

`import { globalThis } from "./global-shim"; globalThis.isNaN(value);`,

Low risk by itself, but the gap creates a mental model mismatch between the rule's constant and the test suite.

gh-aw-bot · 2026-07-05T09:32:19Z

@copilot please run the pr-finisher skill, review the remaining feedback, ensure the branch is current, and rerun checks.

Generated by 👨‍🍳 PR Sous Chef · 16.1 AIC · ⌖ 6.23 AIC · ⊞ 4.6K · ◷
_{Comment /souschef to run again}

…nvalid tests Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>

Copilot · 2026-07-05T09:43:22Z

@copilot please run the pr-finisher skill, review the remaining feedback, ensure the branch is current, and rerun checks.

...

Done. Addressed all feedback from the bot reviewers:

Added import { globalThis } from "./global-shim"; globalThis.isNaN(value); to the ESM valid block for symmetry with the window/global import-shadow cases (raised by Matt Pocock Skills Reviewer and PR Code Quality Reviewer).
Added an esmRuleTester invalid block with isNaN(value) and window.isNaN(value) to confirm the rule still fires in ESM sourceType: "module" without a shadow (raised as medium-priority by PR Code Quality Reviewer, and noted by Impeccable Skills Reviewer and Matt Pocock Skills Reviewer).

make fmt, make lint, and tsc --noEmit all pass. The pre-existing TestPRSousChefWorkflowAddCommentTargetContract failure in pkg/cli is unrelated to these changes.

CI must be re-triggered by a maintainer before merge.

gh-aw-bot · 2026-07-05T11:58:59Z

@copilot please run the pr-finisher skill, address any unresolved review feedback, ensure the branch is up to date with the base branch, and rerun checks.

Generated by 👨‍🍳 PR Sous Chef · 8.37 AIC · ⌖ 7.92 AIC · ⊞ 3.7K · ◷
_{Comment /souschef to run again}

…mber-isnan-issue Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>

Copilot · 2026-07-05T12:38:11Z

@copilot please run the pr-finisher skill, address any unresolved review feedback, ensure the branch is up to date with the base branch, and rerun checks.

...

Branch updated from main (merge commit 7857489). All review feedback was already addressed in the previous pass. Local validation confirms:

make fmt, make lint — pass
make test-unit — all pass, including pkg/cli which was previously failing (main included a fix for TestPRSousChefWorkflowAddCommentTargetContract)

CI must be re-triggered by a maintainer before merge.

github-actions · 2026-07-05T12:54:00Z

🤖 PR Triage — Run §28741077056

Field	Value
Category	`bug`
Risk	🟢 Low
Priority Score	48/100 — impact:22 urgency:13 quality:13
Action	📦 `batch_review`
Batch	`pr-batch:eslint-isnan`

Rationale: Companion fix to #43515 — makes import-bound isNaN shadow globals rather than trigger false positives. 2× APPROVED. Review together with #43515.

Generated by 🔧 PR Triage Agent · 91.2 AIC · ⌖ 11.9 AIC · ⊞ 5.5K · ◷

gh-aw-bot · 2026-07-05T12:59:13Z

@copilot please run the pr-finisher skill, address unresolved review comments, and rerun checks once the branch is up to date.

Generated by 👨‍🍳 PR Sous Chef · 8.12 AIC · ⌖ 9.29 AIC · ⊞ 3.7K · ◷
_{Comment /souschef to run again}

Initial plan

d5c8615

Copilot AI assigned Copilot and pelikhan Jul 5, 2026

Copilot started work on behalf of pelikhan July 5, 2026 07:31 View session

Copilot AI linked an issue Jul 5, 2026 that may be closed by this pull request

prefer-number-isnan: hasLocalBinding excludes ImportBinding defs — undocumented, untested FP vector for imported/shimmed isNaN/w [Content truncated due to length] #43487

Closed

3 tasks

fix(eslint-factory): treat import bindings as local shadows in prefer…

69d37b5

…-number-isnan Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>

Copilot AI changed the title ~~[WIP] Fix prefer-number-isnan: handle ImportBinding definitions correctly~~ prefer-number-isnan: treat ESM import bindings as local shadows Jul 5, 2026

Copilot finished work on behalf of pelikhan July 5, 2026 07:45

Copilot AI requested a review from pelikhan July 5, 2026 07:45

pelikhan marked this pull request as ready for review July 5, 2026 07:57

Copilot AI review requested due to automatic review settings July 5, 2026 07:57

Copilot started reviewing on behalf of pelikhan July 5, 2026 07:58 View session

Copilot AI reviewed Jul 5, 2026

View reviewed changes

github-actions Bot mentioned this pull request Jul 5, 2026

[aw] No-Op Runs #43499

Open

github-actions Bot approved these changes Jul 5, 2026

View reviewed changes

github-actions Bot reviewed Jul 5, 2026

View reviewed changes

github-actions Bot mentioned this pull request Jul 5, 2026

[pr-sous-chef] Run report — 13 nudged, 0 skipped #43518

Closed

Copilot started work on behalf of gh-aw-bot July 5, 2026 09:32 View session

test(eslint-factory): add globalThis ESM import-shadow case and ESM i…

1e59acd

…nvalid tests Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>

Copilot finished work on behalf of gh-aw-bot July 5, 2026 09:43

Copilot AI requested a review from gh-aw-bot July 5, 2026 09:43

github-actions Bot mentioned this pull request Jul 5, 2026

[pr-sous-chef] Run report — 15 nudged, 2 skipped #43535

Closed

Copilot started work on behalf of gh-aw-bot July 5, 2026 12:27 View session

Merge remote-tracking branch 'origin/main' into copilot/fix-prefer-nu…

7857489

…mber-isnan-issue Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>

Copilot finished work on behalf of gh-aw-bot July 5, 2026 12:38

github-actions Bot mentioned this pull request Jul 5, 2026

prefer-number-isnan: autofix for provably-numeric args, drop misleading caveat #43515

Closed

github-actions Bot added pr-action:batch_review pr-agent:copilot pr-priority:medium pr-risk:low pr-type:bug pr-batch:eslint-isnan labels Jul 5, 2026

github-actions Bot mentioned this pull request Jul 5, 2026

[PR Triage Report] [PR Triage] Copilot Agent PR Report — 2026-07-05 (Run #8, §28741077056) #43540

Closed

pelikhan merged commit 2caa9c2 into main Jul 5, 2026
6 checks passed

pelikhan deleted the copilot/fix-prefer-number-isnan-issue branch July 5, 2026 12:58

This was referenced Jul 5, 2026

[pr-sous-chef] Run report — 15 nudged, 2 skipped #43543

Closed

[PR Triage Report] PR Triage Report — 2026-07-05 (Run §28750744185) #43610

Open

Uh oh!

Conversation

Copilot AI commented Jul 5, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions Bot commented Jul 5, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions Bot commented Jul 5, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions Bot commented Jul 5, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions Bot commented Jul 5, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Review details

Uh oh!

github-actions Bot commented Jul 5, 2026

🧪 Test Quality Sentinel Report

Verdict

Uh oh!

github-actions Bot left a comment

Choose a reason for hiding this comment

Uh oh!

github-actions Bot left a comment

Choose a reason for hiding this comment

Review: prefer-number-isnan — ESM ImportBinding fix

Uh oh!

github-actions Bot left a comment

Choose a reason for hiding this comment

Skills-Based Review 🧠

Key Themes

Positive Highlights

Comments that could not be inline-anchored

Uh oh!

github-actions Bot left a comment

Choose a reason for hiding this comment

Review: prefer-number-isnan ESM import binding fix

Uh oh!

github-actions Bot Jul 5, 2026

Choose a reason for hiding this comment

Uh oh!

github-actions Bot Jul 5, 2026

Choose a reason for hiding this comment

Uh oh!

gh-aw-bot commented Jul 5, 2026

Uh oh!

Copilot AI commented Jul 5, 2026

Uh oh!

gh-aw-bot commented Jul 5, 2026

Uh oh!

Copilot AI commented Jul 5, 2026

Uh oh!

github-actions Bot commented Jul 5, 2026

🤖 PR Triage — Run §28741077056

Uh oh!

Uh oh!

gh-aw-bot commented Jul 5, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Copilot AI commented Jul 5, 2026 •

edited

Loading

github-actions Bot commented Jul 5, 2026 •

edited

Loading

github-actions Bot commented Jul 5, 2026 •

edited

Loading

github-actions Bot commented Jul 5, 2026 •

edited

Loading

github-actions Bot commented Jul 5, 2026 •

edited

Loading