Rule
require-invalid-date-check-before-compare (eslint-factory/src/rules/require-invalid-date-check-before-compare.ts) — first review since it shipped; no prior issues filed.
What the rule does
Flags a relational comparison (<, >, <=, >=) involving a new Date(nonTrivialArg) value unless that value was validated via Number.isNaN(x.getTime()) / isNaN(x.getTime()) somewhere in the file. Tracking is scope-aware (uses resolved Variable objects, so same-named locals in different function scopes are correctly kept separate — verified by the existing test "same variable name declared in a different function scope is not treated as validated").
The gap
The validated Set<Variable> is populated purely by whether a Number.isNaN(x.getTime()) call for that variable exists anywhere in the traversal, with no check that the guard is reachable and executes before the risky comparison. Concretely, in create():
CallExpression visitor adds a variable to validated unconditionally whenever isGetTimeNaNCheck matches, regardless of source position.BinaryExpression visitor unconditionally records every relational comparison into comparisons.Program:exit only checks validated.has(side.variable) — no ordering, dominance, or branch-reachability check against the comparison node.
So a guard that appears after the unguarded use, or only in a branch that doesn't actually protect the comparison, is treated as full validation and the diagnostic is suppressed:
const d = new Date(input);
if (d > threshold) { doIt(); } // <-- unguarded here, should be flagged
if (Number.isNaN(d.getTime())) { return; } // check comes too late to help the line aboveconst d = new Date(input);
if (someUnrelatedFlag) {
if (Number.isNaN(d.getTime())) { return; } // only reachable when someUnrelatedFlag is true
}
if (d > threshold) { doIt(); } // still unguarded on the other path, not flaggedBoth are currently silently accepted by the rule (verified by reading the implementation; no live corpus occurrence was found in actions/setup/js/**/*.cjs today — the codebase's actual new Date(...) comparisons, e.g. actions/setup/js/check_rate_limit.cjs:161 and actions/setup/js/check_runs_helpers.cjs:43, have no Number.isNaN guard at all in those files, so the rule correctly flags them as-is). This is the same class of soundness gap previously found and fixed/tracked in sibling rules in this factory (no-unsafe-catch-error-property #42189, no-unsafe-promise-catch-error-property #42915: "whole-block/branch-order false negative" — presence-anywhere-in-scope instead of control-flow-ordered validation).
Suggested fix
At minimum, require the validating Number.isNaN(...) call's node range to start before the comparison node's range for that variable (cheap, catches the reordered case). Better: reuse or extend the ordering/ancestor-walk approach already used by try-catch-rule-utils.ts's isInsideTryBlock/crossedDeferredBoundary helpers elsewhere in this factory to check the guard's containing if actually dominates (encloses or precedes-and-returns-from) the comparison's statement, not just "appears somewhere in the Program".
Acceptance criteria
Generated by 🤖 ESLint Refiner · agent · 224.5 AIC · ⌖ 40.7 AIC · ⊞ 4.9K · ◷
Rule
require-invalid-date-check-before-compare(eslint-factory/src/rules/require-invalid-date-check-before-compare.ts) — first review since it shipped; no prior issues filed.What the rule does
Flags a relational comparison (
<,>,<=,>=) involving anew Date(nonTrivialArg)value unless that value was validated viaNumber.isNaN(x.getTime())/isNaN(x.getTime())somewhere in the file. Tracking is scope-aware (uses resolvedVariableobjects, so same-named locals in different function scopes are correctly kept separate — verified by the existing test"same variable name declared in a different function scope is not treated as validated").The gap
The
validatedSet<Variable>is populated purely by whether aNumber.isNaN(x.getTime())call for that variable exists anywhere in the traversal, with no check that the guard is reachable and executes before the risky comparison. Concretely, increate():CallExpressionvisitor adds a variable tovalidatedunconditionally wheneverisGetTimeNaNCheckmatches, regardless of source position.BinaryExpressionvisitor unconditionally records every relational comparison intocomparisons.Program:exitonly checksvalidated.has(side.variable)— no ordering, dominance, or branch-reachability check against the comparison node.So a guard that appears after the unguarded use, or only in a branch that doesn't actually protect the comparison, is treated as full validation and the diagnostic is suppressed:
Both are currently silently accepted by the rule (verified by reading the implementation; no live corpus occurrence was found in
actions/setup/js/**/*.cjstoday — the codebase's actualnew Date(...)comparisons, e.g.actions/setup/js/check_rate_limit.cjs:161andactions/setup/js/check_runs_helpers.cjs:43, have noNumber.isNaNguard at all in those files, so the rule correctly flags them as-is). This is the same class of soundness gap previously found and fixed/tracked in sibling rules in this factory (no-unsafe-catch-error-property#42189,no-unsafe-promise-catch-error-property#42915: "whole-block/branch-order false negative" — presence-anywhere-in-scope instead of control-flow-ordered validation).Suggested fix
At minimum, require the validating
Number.isNaN(...)call's node range to start before the comparison node's range for that variable (cheap, catches the reordered case). Better: reuse or extend the ordering/ancestor-walk approach already used bytry-catch-rule-utils.ts'sisInsideTryBlock/crossedDeferredBoundaryhelpers elsewhere in this factory to check the guard's containingifactually dominates (encloses or precedes-and-returns-from) the comparison's statement, not just "appears somewhere in the Program".Acceptance criteria
actions/setup/js/**/*.cjs.