eslint-factory: add gh-aw-custom/prefer-number-isnan rule for unsafe global isNaN usage#43365
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
gh-aw-custom/prefer-number-isnan rule for unsafe global isNaN usage
There was a problem hiding this comment.
Pull request overview
This PR extends the eslint-factory custom ESLint plugin with a new rule that discourages coercive global isNaN(...) usage in favor of Number.isNaN(...), helping standardize safer numeric validation patterns in gh-aw JavaScript runtime scripts.
Changes:
- Added a new custom rule
gh-aw-custom/prefer-number-isnanwith a suggestion to replace globalisNaNcallees withNumber.isNaN. - Added rule tests covering valid patterns, global access patterns, shadowed-binding exclusions, and suggestion output.
- Exported and enabled the rule at
warnseverity in the factory plugin and config.
Show a summary per file
| File | Description |
|---|---|
| eslint-factory/src/rules/prefer-number-isnan.ts | Implements the new lint rule and suggestion behavior for global/coercive isNaN callsites. |
| eslint-factory/src/rules/prefer-number-isnan.test.ts | Adds RuleTester coverage for valid/invalid patterns and suggestion output. |
| eslint-factory/src/index.ts | Exports the new rule via the plugin rule map. |
| eslint-factory/eslint.config.cjs | Enables gh-aw-custom/prefer-number-isnan at warning level. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 4/4 changed files
- Comments generated: 1
- Review effort level: Low
| function isIsNaNProperty(node: TSESTree.MemberExpression): boolean { | ||
| const property = node.property; | ||
| const isDirectAccess = property.type === "Identifier" && property.name === "isNaN"; | ||
| const isComputedAccess = property.type === "Literal" && property.value === "isNaN"; | ||
|
|
||
| return isDirectAccess || isComputedAccess; | ||
| } |
|
|
|
✅ 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. |
|
✅ Test Quality Sentinel completed test quality analysis. |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
🧪 Test Quality Sentinel Report✅ Test Quality Score: 86/100 — Excellent
📊 Metrics (5 tests)
Verdict
Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
network:
allowed:
- defaults
- "awmgmcpg"See Network Configuration for more information.
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review: gh-aw-custom/prefer-number-isnan
High-quality, well-tested implementation. The new rule closely follows the established pattern in require-parseInt-radix — same hasLocalBinding helper, same computed-vs-direct MemberExpression check, same shadow-exclusion logic, and consistent test structure.
Design decisions look correct:
Numberis intentionally absent fromGLOBAL_IS_NAN_OBJECTS(unlikerequire-parseInt-radixwhich includes it), becauseNumber.isNaNis the target, not a flagged form.- Suggestion-only (no autofix) is the right call: replacing the callee without reviewing the argument can silently change semantics (
isNaN("42")→true;Number.isNaN("42")→false). - Shadow-binding exclusion correctly prevents false positives for locally declared
isNaN/globalThis/window/global.
Minor pre-existing gap (not blocking): The docs.url for all rules points to https://github.com/github/gh-aw/tree/main/eslint-factory#<rule-name>, but README.md has no per-rule anchors. This is pre-existing across every rule; a follow-up to add rule docs to the README would help all rules at once.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · 53.4 AIC · ⌖ 5.87 AIC · ⊞ 4.9K
There was a problem hiding this comment.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · 64.4 AIC · ⌖ 5.71 AIC · ⊞ 6.6K
Comment /matt to run again
|
|
||
| describe("prefer-number-isnan", () => { | ||
| it("uses the correct docs URL", () => { | ||
| expect(preferNumberIsNanRule.meta.docs.url).toBe("https://github.com/github/gh-aw/tree/main/eslint-factory#prefer-number-isnan"); |
There was a problem hiding this comment.
[/grill-with-docs] The docs URL points to a README anchor (#prefer-number-isnan) that doesn't exist yet — clicking this link in an IDE will land on a 404 fragment.
💡 Add the anchor to README.md
Add a rules table or section to eslint-factory/README.md so the URL resolves. For example:
## Rules
### `prefer-number-isnan`
Prefer `Number.isNaN()` over global `isNaN()` to avoid silent coercion of non-numeric inputs.This makes every rule's docs URL a meaningful destination for developers who click through from their IDE lint warning.
@copilot please address this.
| }, | ||
| ], | ||
| }); | ||
| } |
There was a problem hiding this comment.
[/tdd] Missing edge case: the suggestion replaces only the callee (isNaN → Number.isNaN) but preserves the original argument unchanged. If the argument is a raw string (e.g., isNaN(process.env.PORT)) the replacement is still wrong — Number.isNaN(process.env.PORT) will always return false. There's no test covering this unsafe-argument scenario.
💡 Consider a suggestion message that surfaces the risk
The current suggestion message says "review whether the argument should be wrapped with Number(...)" which is good, but a test that applies the suggestion to an obviously unsafe argument (e.g., isNaN(envVar)) would confirm the suggestion text actually guides the author toward the right fix.
// add to the invalid cases section
{
code: `isNaN(process.env.PORT);`,
errors: [{ messageId: "preferNumberIsNaN",
suggestions: [{ messageId: "replaceWithNumberIsNaN",
output: `Number.isNaN(process.env.PORT);` }] }],
},This makes the "review the argument" note testable and visible in the test suite as documentation.
@copilot please address this.
| function hasLocalBinding(node: TSESTree.Node, name: string): boolean { | ||
| let scope: SourceCodeScope | null = sourceCode.getScope(node); | ||
|
|
||
| while (scope) { |
There was a problem hiding this comment.
[/codebase-design] The hasLocalBinding function walks the entire scope chain up to the module/program scope, which means it also returns true for module-level import bindings (which have defs.length > 0). This could suppress the rule when isNaN is imported from a utility module — an import is not a local shadow that makes the call safe.
💡 Scope the "local binding" check to user-defined variables only
ESLint's scope defs have a type property. Consider restricting to Variable and Parameter def types so that import bindings don't trigger the exclusion:
if (variable?.defs.some(d => d.type === "Variable" || d.type === "Parameter")) {
return true;
}Or verify this can't happen in practice by adding a test case like:
// This should still be flagged — isNaN is imported, not a real local shadow
import { isNaN } from "./compat";
isNaN(value);@copilot please address this.
| cjsRuleTester.run("prefer-number-isnan", preferNumberIsNanRule, { | ||
| valid: [`Number.isNaN(value);`, `Number["isNaN"](value);`, `foo.isNaN(value);`], | ||
| invalid: [], | ||
| }); |
There was a problem hiding this comment.
[/tdd] The valid-cases test for Number.isNaN and non-global forms runs only under the CJS RuleTester — there's no ESM counterpart for these valid paths. Given that the rule is intended for actions/setup/js (CJS) this is acceptable, but worth an explicit comment or a matching ESM valid-case block to make the intent clear.
💡 Either add an ESM counterpart or add an intent comment
// valid cases under ESM — mirrors CJS coverage above
esmRuleTester.run("prefer-number-isnan", preferNumberIsNanRule, {
valid: [`Number.isNaN(value);`, `foo.isNaN(value);`],
invalid: [],
});Or add a brief comment: // CJS-only: actions/setup/js targets CommonJS so future readers know the asymmetry is deliberate.
@copilot please address this.
| if (node.callee.type === "Identifier" && node.callee.name === "isNaN" && !hasLocalBinding(node, "isNaN")) { | ||
| report(node); | ||
| return; | ||
| } |
There was a problem hiding this comment.
[/tdd] The CallExpression visitor handles isNaN(...) but not isNaN used as a value (e.g., passed as a callback: values.some(isNaN)). There's no test for this pattern, so the intent (flag or ignore?) is undocumented.
💡 Add a test to document the intended behaviour
// Should this be flagged? Document the decision with a test.
it("valid/invalid: isNaN used as a callback reference", () => {
cjsRuleTester.run("prefer-number-isnan", preferNumberIsNanRule, {
valid: [`values.some(isNaN);`], // intentionally not a CallExpression
invalid: [],
});
});If the intent is to also catch reference-style usage, a separate Identifier visitor would be needed.
@copilot please address this.
There was a problem hiding this comment.
Non-blocking: one real bug, one missing test
The overall design is solid — @typescript-eslint/utils typed helpers, proper scope-chain walk via context.sourceCode.getScope(), and separate isIsNaNProperty for computed/direct dispatch. One correctness bug exists in that helper and is already flagged by the existing review comment; one inline comment here tracks the matching missing test.
Key findings
Bug: isIsNaNProperty false positive on globalThis[isNaN](x) (existing comment covers the fix in prefer-number-isnan.ts)
The isDirectAccess branch checks property.type === "Identifier" && property.name === "isNaN" without guarding !node.computed. A computed MemberExpression like globalThis[isNaN](x) — where the property is a variable reference, not the string literal "isNaN" — incorrectly satisfies this check and gets flagged. Fix: add !node.computed && before the identifier test.
Missing regression test (inline comment on test file)
The valid test suite has no case for globalThis[isNaN](value), so the false positive has no test harness to catch it before or after the fix.
Everything else — hasLocalBinding scope walk, suggestion fixer, index/config wiring, the foo.isNaN() exclusion — is correct.
🔎 Code quality review by PR Code Quality Reviewer · 172.8 AIC · ⌖ 6.35 AIC · ⊞ 5.4K
Comment /review to run again
| ], | ||
| invalid: [], | ||
| }); | ||
| }); |
There was a problem hiding this comment.
Missing valid test for dynamic computed property access: globalThis[isNaN](x) (where isNaN is a variable reference, not a string literal) should be a valid case, but there is no test covering it — this is the regression guard for the isIsNaNProperty false-positive bug noted in the existing review comment.
💡 Suggested addition
Add to the valid tests:
esmRuleTester.run('prefer-number-isnan', preferNumberIsNanRule, {
valid: [
// Dynamic computed access — identifier property, not string literal 'isNaN'
`globalThis[isNaN](value);`,
],
invalid: [],
});The isDirectAccess branch of isIsNaNProperty checks property.type === "Identifier" && property.name === "isNaN" but does not guard against node.computed === true. That means globalThis[isNaN](x) — where the property is an identifier reference, not the literal string "isNaN" — satisfies isDirectAccess and produces a false positive. The fix is !node.computed && property.type === "Identifier" && property.name === "isNaN"; this test case is the proof it holds.
|
@copilot please run the Unresolved review feedback to address:
|
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Addressed all unresolved feedback:
|
Global
isNaN()coercion can silently accept invalid raw inputs (e.g. env/API strings), which makes numeric validation brittle inactions/setup/js. This PR adds a custom lint rule to standardize on strictNumber.isNaN()semantics and surface risky callsites as warnings.Rule addition:
prefer-number-isnaneslint-factory/src/rules/prefer-number-isnan.ts.isNaN(x)globalThis.isNaN(x)/globalThis["isNaN"](x)window.isNaN(x)/window["isNaN"](x)global.isNaN(x)/global["isNaN"](x)isNaN,globalThis,window,global).Developer ergonomics
hasSuggestions: truewith callee-only replacement toNumber.isNaN.Number(...)).Factory wiring
eslint-factory/src/index.ts.warnseverity ineslint-factory/eslint.config.cjsas:gh-aw-custom/prefer-number-isnan.Focused coverage
eslint-factory/src/rules/prefer-number-isnan.test.tscovering:Number.isNaN(...)usage