[eslint-miner] Add ESLint rule: require-invalid-date-check-before-compare - #51354
Conversation
New custom rule in eslint-factory/src/rules that flags new Date(x) constructed from a non-literal argument and used in a relational comparison (<, >, <=, >=) without first validating it via Number.isNaN(x.getTime()) (or isNaN(x.getTime())). An Invalid Date compares as neither greater than nor less than any other date — all relational comparisons involving NaN evaluate to false — which silently defeats time-window/threshold checks instead of surfacing a parse error. Found two real, currently-live instances of this pattern while scanning actions/setup/js: - check_rate_limit.cjs:161 — runCreatedAt < thresholdTime with no NaN guard on the pagination cutoff check. - check_runs_helpers.cjs:43 — new Date(a) > new Date(b) comparing two unvalidated constructed dates when picking the latest check run. The rule intentionally excludes new Date() and new Date(Date.now()...) (and arithmetic derived from Date.now()) since those can never be Invalid Date, keeping false-positive risk low. Existing correctly validated call sites (check_stop_time.cjs, ephemerals.cjs, expired_entity_cleanup_helpers.cjs, create_project_status_update.cjs, unified_timeline.cjs) are unaffected — confirmed via full lint run, which shows zero regressions. Registered at "warn" in eslint.config.cjs, consistent with the rest of the ruleset. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
Great work! 🎯 This ESLint rule addition looks solid and ready for review. The PR demonstrates excellent craft:
The rule is registered at Ready for maintainer review! 👍
|
|
✅ PR Code Quality Reviewer completed the code quality review. Warning Threat Detection Engine Failure — The analysis engine could not complete. This is a tooling failure, not a security finding. What happenedThe threat detection engine failed to produce results. Review the workflow run logs for details. Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
To allow these domains, add them to the network:
allowed:
- defaults
- "api.individual.githubcopilot.com"See Network Configuration for more information.
|
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR does not have the 'implementation' label and has 0 new lines of code in business logic directories (threshold: 100).
|
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅
|
|
✅ Test Quality Sentinel completed test quality analysis. No test files were added or modified in this PR. Test Quality Sentinel skipped.
|
There was a problem hiding this comment.
Pull request overview
Adds a custom ESLint rule to detect relational comparisons involving potentially invalid Date values.
Changes:
- Implements invalid-date comparison detection.
- Adds rule tests.
- Registers and enables the rule at warning level.
Show a summary per file
| File | Description |
|---|---|
eslint-factory/src/rules/require-invalid-date-check-before-compare.ts |
Implements date validation analysis. |
eslint-factory/src/rules/require-invalid-date-check-before-compare.test.ts |
Tests reported and accepted patterns. |
eslint-factory/src/index.ts |
Registers the rule. |
eslint-factory/eslint.config.cjs |
Enables the rule as a warning. |
Review details
Tip
Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
Suppressed comments (1)
eslint-factory/src/rules/require-invalid-date-check-before-compare.ts:110
- A program-wide set keyed only by identifier spelling does not establish that this comparison is safe. For example,
d < cutoff; Number.isNaN(d.getTime())is accepted even though the check is too late; an ignored check or a check of a shadoweddalso suppresses the report. Track resolved variable bindings and verify that this comparison executes only on a path where that specific value has already been proven non-NaN (including the supported short-circuit forms).
if (!validated.has(name)) {
- Files reviewed: 4/4 changed files
- Comments generated: 2
- Review effort level: Balanced
| // `new Date(Date.now())` and `new Date(Date.now() + n)` are always valid — Date.now() cannot | ||
| // produce NaN, and arithmetic on a finite number stays finite. | ||
| if (isDateNowDerived(arg)) return false; | ||
|
|
||
| return true; | ||
| } | ||
|
|
||
| function isDateNowDerived(node: TSESTree.Node): boolean { | ||
| if (node.type === AST_NODE_TYPES.CallExpression && node.callee.type === AST_NODE_TYPES.MemberExpression) { | ||
| const { object, property } = node.callee; | ||
| if (object.type === AST_NODE_TYPES.Identifier && object.name === "Date" && property.type === AST_NODE_TYPES.Identifier && property.name === "now") { | ||
| return true; | ||
| } | ||
| } | ||
| if (node.type === AST_NODE_TYPES.BinaryExpression) { | ||
| return isDateNowDerived(node.left) || isDateNowDerived(node.right); | ||
| } | ||
| return false; |
There was a problem hiding this comment.
Fixed in cd5eaf8: isPotentiallyInvalidDateConstruction now only exempts the exact Date.now() call (no arithmetic). new Date(Date.now() - windowMs) is now flagged since windowMs isn't guaranteed numeric — added a test for this and updated check_rate_limit.cjs's finding accordingly.
| }, | ||
| schema: [], | ||
| messages: { | ||
| requireInvalidDateCheck: "{{subject}} is constructed with new Date(...) from a non-literal value and compared with a relational operator ({{operator}}) without ever being checked via Number.isNaN({{getTimeTarget}}.getTime()). An unparseable date silently fails every comparison instead of surfacing an error.", |
There was a problem hiding this comment.
Fixed in cd5eaf8: reworded the message to "{{subject}} may be an Invalid Date and is compared with..." which no longer claims the input is non-literal.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review: require-invalid-date-check-before-compare ESLint rule
The rule targets a real correctness issue (silently failed comparisons against Invalid Dates) and the overall approach is sound. Two blocking issues found:
1. Cross-scope false negatives (blocking) — dateVars, validated, and comparisons are module-level and never reset between function scopes. If two functions use the same variable name d, a getTime() NaN check in the first function will suppress errors in the second function. This makes the rule unreliable in real codebases with multiple functions.
2. Duplicate errors for inline new Date() on both sides of a comparison (blocking) — new Date(a) > new Date(b) reports two separate errors on the same BinaryExpression node. The test encodes this as expected behavior, but it is surprising and noisy. A single diagnostic per comparison node would be clearer.
Non-blocking suggestions
- Reporting on the
BinaryExpressionnode while the message says "is constructed with new Date(...)" can be confusing — consider pointing at the declarator too. - The test file uses a fixed URL for the docs check which will drift if the rule is renamed.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 · 27 AIC · ⌖ 6.78 AIC · ⊞ 5.5K
| const arg = node.arguments[0] as TSESTree.CallExpression; | ||
| const obj = (arg.callee as TSESTree.MemberExpression).object; | ||
| if (obj.type === AST_NODE_TYPES.Identifier) { | ||
| validated.add(obj.name); |
There was a problem hiding this comment.
Bug: Cross-scope false negatives due to module-level tracking
The dateVars, validated, and comparisons maps/sets span the entire program and are never reset between function scopes. This causes false negatives when the same variable name is used in multiple functions:
function a() {
const d = new Date(input);
if (Number.isNaN(d.getTime())) return;
if (d < threshold) { } // correctly passes
}
function b() {
const d = new Date(other); // different scope, same name
if (d < threshold) { } // SHOULD be flagged, but WON'T because 'd' is in `validated`
}Consider resetting or scoping these collections per function body using enter/exit visitors (FunctionDeclaration, ArrowFunctionExpression, FunctionExpression).
@copilot please address this.
There was a problem hiding this comment.
Fixed in cd5eaf8: dateVars/validated are now keyed by the scope-resolved Variable object (via sourceCode.getScope(...).set.get(name) walking .upper, matching the convention in core-method-resolve.ts) instead of by name string, so same-named locals in different functions no longer collide. Added a test for this exact repro.
| for (const side of [node.left, node.right]) { | ||
| // Direct relational use of an inline `new Date(...)` expression. | ||
| if (side.type === AST_NODE_TYPES.NewExpression && isPotentiallyInvalidDateConstruction(side)) { | ||
| comparisons.push({ name: "<inline>", operator: node.operator, node }); |
There was a problem hiding this comment.
Bug: Inline new Date() errors reported twice when both sides of a comparison are inline Date constructions
When both node.left and node.right are inline new Date(...) expressions (e.g. new Date(a) > new Date(b)), the same BinaryExpression node is pushed to comparisons twice, and reported twice in Program:exit. The test actually asserts two errors for this case, effectively enshrining the duplicate. However downstream tooling and users will see a confusing double error on the same AST node.
Consider deduplicating by the node itself (e.g. track reported nodes in a Set), or emit a single error that names both operands.
@copilot please address this.
There was a problem hiding this comment.
Fixed in cd5eaf8: when both sides of a comparison are unvalidated new Date(...), the rule now emits a single combined diagnostic ("Both operands of this comparison...") instead of two identical reports on the same node. Updated the test to expect one error.
| return { | ||
| VariableDeclarator(node) { | ||
| if (node.id.type === AST_NODE_TYPES.Identifier && node.init?.type === AST_NODE_TYPES.NewExpression && isPotentiallyInvalidDateConstruction(node.init)) { | ||
| dateVars.set(node.id.name, node); |
There was a problem hiding this comment.
Correctness: Deferred reporting in Program:exit misses the comparison node location
All errors are reported on the BinaryExpression node (the comparison site), regardless of whether the unvalidated new Date() construction is far away. This is useful, but the message text '{{subject}}' is constructed with new Date(...)... could be confusing since the highlighted code is the comparison, not the construction.
Consider also reporting the VariableDeclarator node location (or using suggest), so the user can quickly navigate to where validation should be inserted.
Non-blocking suggestion — just worth considering for developer experience.
@copilot please address this.
There was a problem hiding this comment.
Kept reporting on the comparison node — it's the site where the missing guard actually matters (a new Date(...) construction site alone isn't wrong, it's only a problem once it reaches an unguarded relational comparison), and it matches how ESLint typically flags the point of misuse. Since this was flagged as non-blocking, not adding a second report location to keep the diagnostic focused.
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /tdd — commenting with targeted issues; no blocking correctness errors, but one important logic gap worth addressing before merge.
📋 Key Themes & Highlights
Key Issues
- Scope blindness (highest impact):
dateVarsandvalidatedare keyed by variable name across the entire module. Two functions with a local nameddshare the same validated/unvalidated state — a false-negative that defeats the rule's core promise. See inline comment on line 88. - Deduplication gap: inline
new Date(...)diagnostics are not deduplicated; nested expressions could produce duplicate reports. See line 100. - Deferred flush: the
comparisonsarray includes entries that will be suppressed at flush time — a minor design smell. See line 83. - Test gap: no test exercises
isNaN(global) in a same-condition guard (versus early-return form). See test line 52.
Positive Highlights
- ✅ Excellent PR description — two real production instances identified, false-positive exclusions well-reasoned, validation steps documented.
- ✅
isDateNowDerivedcorrectly handles arithmetic onDate.now()(e.g.,Date.now() - windowMs) — a subtle correctness detail handled well. - ✅ Rule type is
"problem"(not"suggestion") — appropriate for a silent-failure correctness bug. - ✅ Registration at
"warn"consistent with existing ruleset; non-breaking rollout. - ✅
Program:exitflush pattern correctly avoids false positives when validation appears after construction but before comparison — the right design for file-level analysis.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 · 35.1 AIC · ⌖ 7.86 AIC · ⊞ 7.1K
Comment /matt to run again
| } | ||
| } | ||
| }, | ||
|
|
There was a problem hiding this comment.
[/tdd] The dateVars and validated sets are shared across the entire program scope. If two functions declare locals with the same name (e.g. d), and one validates it while the other does not, the validated set will suppress the warning on the un-guarded usage — a false negative that undermines the rule's core mission.
💡 Suggested fix
Use ESLint's scope analysis or key by node identity rather than variable name:
// Key by declarator node (identity) instead of string name
const dateVarNodes = new Set<TSESTree.Node>();Minimal repro that currently silently passes (should warn on b):
function a() { const d = new Date(x); if (Number.isNaN(d.getTime())) return; if (d > t) {} }
function b() { const d = new Date(y); if (d > t) {} } // no warning!@copilot please address this.
There was a problem hiding this comment.
Same fix as the sibling thread above — resolved in cd5eaf8 by keying on the scope-resolved Variable identity instead of the name string.
| } | ||
| if (side.type === AST_NODE_TYPES.Identifier && dateVars.has(side.name)) { | ||
| comparisons.push({ name: side.name, operator: node.operator, node }); | ||
| } |
There was a problem hiding this comment.
[/tdd] Inline new Date(...) expressions are reported but never de-duplicated across sides. When the same binary expression triggers two <inline> entries (as shown in the test for check_runs_helpers.cjs), the first comparisons.push for the left side and the second for the right side each emit a report against the same node. That produces two errors on one BinaryExpression node — which is the intended behaviour — but the logic has no guard against reporting the same inline node multiple times if it appears in a nested expression. Consider adding a reportedNodes WeakSet to avoid duplicate diagnostics on complex expressions.
@copilot please address this.
There was a problem hiding this comment.
Same fix as the sibling thread above — resolved in cd5eaf8 by collecting both sides per BinaryExpression and emitting one combined report instead of one per side.
| if (isGetTimeNaNCheck(node)) { | ||
| const arg = node.arguments[0] as TSESTree.CallExpression; | ||
| const obj = (arg.callee as TSESTree.MemberExpression).object; | ||
| if (obj.type === AST_NODE_TYPES.Identifier) { |
There was a problem hiding this comment.
[/tdd] The comparisons array accumulates entries and is only flushed in Program:exit. For a long file with many date variables this is fine, but the array grows proportionally with the number of relational comparisons — including valid ones that will be suppressed. Consider only pushing to comparisons when the variable is actually in dateVars, and skipping the final iteration over entries that are already in validated by checking at push-time (early exit pattern) rather than at flush-time.
💡 Why this matters
The current design means Program:exit iterates all collected comparisons even for variables that were validated far earlier in the file. For the codebase's actual files this is immaterial, but it's a minor design smell that could cause confusion during maintenance — the flush loop and the push loop are structurally separate with no obvious link.
@copilot please address this.
There was a problem hiding this comment.
Kept the deferred flush in Program:exit: validation (Number.isNaN(d.getTime())) can appear after the comparison in valid code paths, so validated membership can't be checked reliably until traversal completes. The comparisons list only ever contains sides already confirmed to reference a tracked dateVars entry (checked at push time), so it doesn't grow with unrelated comparisons — just with the ones that could still turn out to be guarded later in the file.
| `const d = new Date(input); if (isNaN(d.getTime())) { return; } if (d >= threshold) { doIt(); }`, | ||
| ], | ||
| invalid: [], | ||
| }); |
There was a problem hiding this comment.
[/tdd] The valid-pattern tests omit a guard idiom that the codebase uses: !Number.isNaN(d.getTime()) && d > threshold as the sole branch (without a preceding throw/return). The test at line 52 covers this form, but there's no test for isNaN (the global) in the same guard-then-compare position, only isNaN with an early return (line 56). Adding a test for isNaN(d.getTime()) || d > threshold — which is also a valid guarded comparison pattern — would strengthen confidence that the isNaNGlobal branch in isGetTimeNaNCheck is exercised through a real comparison path.
@copilot please address this.
There was a problem hiding this comment.
Added in cd5eaf8: a valid test case for isNaN(d.getTime()) || d > threshold.
|
@copilot run pr-finisher skill |
…rule Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
🎉 This pull request is included in a new release. Release: |
ESLint Miner — new rule:
require-invalid-date-check-before-compareMission
Add one high-signal, net-new custom ESLint rule for
actions/setup/js/**(per the daily ESLint Miner mandate).What was mined
Scanned
actions/setup/js/*.cjsfor recurringnew Date(...)construction patterns feeding relational comparisons (<,>,<=,>=). Cross-checked existing rules ineslint-factory/src/rules(includingrequire-nan-check-after-env-numeric-parse, which validates numeric parses fromprocess.envbut does not coverDateparsing) — confirmed no overlapping rule exists for Date validation.The bug pattern
new Date(x)wherexis unparseable produces an "Invalid Date" whose.getTime()isNaN. Every relational comparison (<,>,<=,>=) involvingNaNevaluates tofalse— silently, with no thrown error. In time-window/threshold logic (rate-limit cutoffs, freshness checks, "pick the latest" comparisons) this means malformed timestamps are quietly treated as "does not exceed/precede" instead of surfacing a parse failure.Two real, currently-live instances found:
actions/setup/js/check_rate_limit.cjs:161—const runCreatedAt = new Date(run.created_at); if (runCreatedAt < thresholdTime) { ... }— no NaN guard before the pagination-window cutoff check.actions/setup/js/check_runs_helpers.cjs:43—if (!existing || new Date(run.started_at ?? 0) > new Date(existing.started_at ?? 0))— two constructed dates compared directly with no validation.For contrast, several files in the same directory already do this correctly (
check_stop_time.cjs,ephemerals.cjs,expired_entity_cleanup_helpers.cjs,create_project_status_update.cjs,unified_timeline.cjs) — these all callNumber.isNaN(d.getTime())right after construction. The new rule recognizes and accepts that pattern.Rule design (low false-positive risk)
new Date(arg)whereargis not trivially always-valid: barenew Date()and anything derived fromDate.now()(includingDate.now() + narithmetic) are excluded, since those can never produce an Invalid Date.<,>,<=,>=) — not arbitrary use (e.g. formatting via.toISOString()is out of scope; that's a different failure mode already partially covered elsewhere).Number.isNaN(d.getTime())andisNaN(d.getTime())as valid guards, matching the codebase's established idiom."warn"ineslint.config.cjs, consistent with the rest of the ruleset.Validation
cd eslint-factory && npm install && npm run build— clean.npm run lint:setup-js— full run shows the two real findings above and zero regressions (pre-existing warning count for all other rules unchanged).require-fs-io-try-catch.test.tshas 5 pre-existing failures unrelated to this change (verified viagit stash— failures reproduce onmainbefore this PR).Files changed
eslint-factory/src/rules/require-invalid-date-check-before-compare.ts(new rule)eslint-factory/src/rules/require-invalid-date-check-before-compare.test.ts(new tests)eslint-factory/src/index.ts(registration)eslint-factory/eslint.config.cjs(enable atwarn)