Require isNaN(getTime()) guard to precede and dominate the date comparison - #51532
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
…rison Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
PR Triage
|
There was a problem hiding this comment.
Pull request overview
Updates the invalid-date ESLint rule to consider guard ordering and reachability. However, the proposed ancestor-based analysis still misses bypass paths, mishandles switch cases, and includes unrelated workflow lockfile regeneration.
Changes:
- Records guard and comparison ancestor paths.
- Adds ordering/reachability tests.
- Regenerates numerous workflow lockfiles with additional safe-output counters.
Show a summary per file
| File | Description |
|---|---|
eslint-factory/src/rules/require-invalid-date-check-before-compare.ts |
Adds guard dominance analysis. |
eslint-factory/src/rules/require-invalid-date-check-before-compare.test.ts |
Adds ordering and branch tests. |
.github/workflows/*.lock.yml (all listed lockfiles) |
Adds generated safe-output counters; unrelated to the stated ESLint fix. |
Review details
Tip
Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
- Files reviewed: 285/285 changed files
- Comments generated: 3
- Review effort level: Balanced
| create_discussion_errors: ${{ steps.process_safe_outputs.outputs.create_discussion_errors }} | ||
| created_issue_number: ${{ steps.process_safe_outputs.outputs.created_issue_number }} | ||
| created_issue_url: ${{ steps.process_safe_outputs.outputs.created_issue_url }} | ||
| process_safe_outputs_items_applied: ${{ steps.process_safe_outputs.outputs.items_applied }} |
| case AST_NODE_TYPES.SwitchCase: | ||
| return parent.consequent.includes(child as TSESTree.Statement); |
| let divergence = 0; | ||
| while (divergence < guardPath.length && divergence < comparisonPath.length && guardPath[divergence] === comparisonPath[divergence]) { | ||
| divergence++; | ||
| } | ||
| // Guards and comparisons always share the Program node, so `divergence` is normally at least 1. | ||
| for (let i = Math.max(divergence, 1); i < guardPath.length; i++) { | ||
| if (isConditionalEdge(guardPath[i - 1], guardPath[i])) return false; |
|
🧠 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.
|
|
✅ 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).
|
|
✅ Ponytail Reviewer completed successfully! PR #51532's pre-fetched diff and metadata contain only 100 generated .lock.yml workflow files (mechanical additions of new safe-outputs step outputs), with no non-generated source files (e.g., the actual ESLint rule change described in the PR body) present to review. Per instructions to skip generated/lock files, there is nothing eligible for a ponytail over-engineering 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.
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 · 110.2 AIC · ⌖ 10.6 AIC · ⊞ 7.1K
Comment /matt to run again
| case AST_NODE_TYPES.SwitchCase: | ||
| return parent.consequent.includes(child as TSESTree.Statement); | ||
| case AST_NODE_TYPES.TryStatement: | ||
| return child === parent.handler; |
There was a problem hiding this comment.
[/diagnosing-bugs] TryStatement only marks the catch handler as a conditional edge, but the finally block (parent.finalizer) is not checked — a guard inside a try body is also conditional if code above it can throw. Consider also treating child === parent.finalizer as a conditional edge.
💡 Missed scenario
const d = new Date(input);
try {
somethingThatMightThrow();
if (Number.isNaN(d.getTime())) { return; } // skipped if throw above
} catch (e) {}
if (d > threshold) { doIt(); } // still fires with invalid date@copilot please address this.
| function guardDominatesComparison(guardPath: TSESTree.Node[], comparisonPath: TSESTree.Node[]): boolean { | ||
| const guard = guardPath[guardPath.length - 1]; | ||
| const comparison = comparisonPath[comparisonPath.length - 1]; | ||
| if (guard.range[1] > comparison.range[0]) return false; |
There was a problem hiding this comment.
[/diagnosing-bugs] The range-based ordering check (guard.range[1] > comparison.range[0]) assumes both nodes have valid ranges, but ESLint nodes can have range as undefined if the parser is not configured with range: true. This would throw at runtime rather than fail gracefully.
💡 Defensive fix
Add a guard:
if (!guard.range || !comparison.range) return false; // can't determine order without ranges
if (guard.range[1] > comparison.range[0]) return false;Or verify in test configuration that range is always enabled (the RuleTester default enables it, but real-world consumer configs may not).
@copilot please address this.
| divergence++; | ||
| } | ||
| // Guards and comparisons always share the Program node, so `divergence` is normally at least 1. | ||
| for (let i = Math.max(divergence, 1); i < guardPath.length; i++) { |
There was a problem hiding this comment.
[/diagnosing-bugs] Math.max(divergence, 1) skips index 0 of guardPath when the paths don't diverge until after index 0, but it also skips the check when divergence === 0 (guard and comparison share no common prefix beyond the implicit assumption). If divergence is 0 (e.g. two paths built from different root traversals), the loop starts at index 1 and the edge between index 0 and 1 is silently skipped. Add a test case where two paths share no common node to confirm this is intentionally safe.
@copilot please address this.
| }); | ||
| }); | ||
|
|
||
| it("invalid: guard nested in an unrelated conditional branch does not protect the comparison", () => { |
There was a problem hiding this comment.
[/tdd] The test name says "unrelated conditional branch" but the two cases cover if and for...of — there is no test for while, do...while, &&/|| short-circuit, or a switch case body. Adding at least one while loop and one logical-AND case as invalid examples would close the most likely regression paths.
💡 Suggested additions
// while loop guard — should be invalid
{ code: `const d = new Date(x); while (cond) { if (Number.isNaN(d.getTime())) return; } if (d > t) {}`, errors: [...] },
// logical-AND right side — should be invalid
{ code: `const d = new Date(x); true && (Number.isNaN(d.getTime()) ? true : false); if (d > t) {}`, errors: [...] },@copilot please address this.
| case AST_NODE_TYPES.LogicalExpression: | ||
| return child === parent.right; | ||
| case AST_NODE_TYPES.SwitchCase: | ||
| return parent.consequent.includes(child as TSESTree.Statement); |
There was a problem hiding this comment.
[/diagnosing-bugs] SwitchCase checks whether child is in parent.consequent (the statements of one case), but the SwitchStatement itself — which holds the discriminant and the array of cases — is never matched. This means a guard in a switch discriminant position (switch (Number.isNaN(d.getTime()))) would correctly fall through to the default: return false branch and not be treated as conditional. That's the correct outcome, but there is no test verifying it. A guard inside a case body is already covered by SwitchCase. Verify with a test that a guard in a case consequent is correctly rejected.
@copilot please address this.
require-invalid-date-check-before-comparemarked a variable validated whenever aNumber.isNaN(x.getTime())call existed anywhere in the traversal, with no positional or reachability check. A guard written after the risky comparison, or nested in a branch that doesn't protect it, silently suppressed the diagnostic.Changes
validated: Set<Variable>→guards: Map<Variable, Node[][]>— each guard is recorded with its ancestor path (sourceCode.getAncestors, matching the pattern in sibling rules). Comparisons record their path too.guardDominatesComparison(guardPath, comparisonPath)— a guard counts only when it ends before the comparison starts in source order and no conditional edge is taken on the guard's path below the deepest node the two share.isConditionalEdge(parent, child)— classifies edges that aren't guaranteed to execute:if/ternary consequent & alternate, right operand of&&/||, switch-case bodies,catchhandlers, loop bodies, function bodies. Always-executed positions (iftest, left operand of a logical expression,tryblock) are excluded, which is what keeps!Number.isNaN(d.getTime()) && d > tand guard-then-use valid.Program:exitfilters sides throughisValidatedBefore(variable, comparisonPath)instead of a set membership test.Tests
Invalid: guard after the comparison; guard inside a sibling
ifbranch; guard inside afor…ofbody.Valid (new): guard in the
iftest with the comparison in theelse if; guard preceding a loop that contains the comparison.Existing valid/invalid cases are unchanged, and the corpus lint output for
actions/setup/js/**/*.cjsreports the same two occurrences as before.Note: the pre-existing failures in
require-fs-io-try-catch.test.tsalso reproduce on the base commit and are untouched here.