Require positive timeouts for synchronous child processes - #52978
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
✅ PR Code Quality Reviewer completed the code quality review.
|
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR #52978 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.
|
|
✅ Ponytail Reviewer completed successfully! Lean already. Ship.
|
Comment MemoryNote This comment is managed by comment memory.It stores persistent context for this thread in the code block at the top of this comment.
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Verdict
REQUEST_CHANGES — this tightens the obvious timeout: 0 hole, but it still leaves statically-invalid timeout forms unflagged, so the rule can be bypassed while appearing compliant.
Blocking themes
- The new check only rejects numeric literals and unary-negative literals.
- Constant expressions such as
NaN,Infinity,1n, or parenthesized zero/negative values still pass even though they are not valid positive millisecond timeouts for these APIs. - That keeps the rule's safety guarantee weaker than the updated diagnostic claims.
🔎 Code quality review by PR Code Quality Reviewer · gpt54 · 2.83 AIC · ⌖ 5.32 AIC · ⊞ 6.9K
Comment /review to run again
There was a problem hiding this comment.
The logic is correct and comprehensive. The isMissingTimeout predicate correctly covers all zero/negative cases: numeric literals <= 0, unary - applied to any numeric literal (including -0), and null/undefined. The updated error message accurately describes the requirement. Tests cover all new cases including the edge case of -0. LGTM.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 · 14.7 AIC · ⌖ 7.03 AIC · ⊞ 5.6K
There was a problem hiding this comment.
Pull request overview
Updates the ESLint rule to require positive synchronous child-process timeouts.
Changes:
- Rejects zero and negative literal timeouts.
- Improves the diagnostic message.
- Adds regression coverage for literal and dynamic values.
Show a summary per file
| File | Description |
|---|---|
eslint-factory/src/rules/require-sync-exec-timeout.ts |
Validates positive timeout values and updates diagnostics. |
eslint-factory/src/rules/require-sync-exec-timeout.test.ts |
Adds timeout validation test cases. |
Review details
💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
Suppressed comments (2)
eslint-factory/src/rules/require-sync-exec-timeout.ts:132
- Unary
+0is also statically zero and disables Node's timeout, but this branch only rejects unary-, soexecSync(cmd, { timeout: +0 })remains unreported despite the new positive-timeout contract. Handle unary plus when its numeric literal is zero and add a regression case.
(value.type === AST_NODE_TYPES.UnaryExpression && value.operator === "-" && value.argument.type === AST_NODE_TYPES.Literal && typeof value.argument.value === "number") ||
eslint-factory/src/rules/require-sync-exec-timeout.ts:154
- The suggested object order does not guarantee the advertised protection: a following
...otherOptionscan overwrite the positive timeout with0orundefined. Put the explicit timeout after the spread so the diagnostic's fix always leaves the effective value positive.
"{{method}}({{arg}}) has no positive `timeout` option. `timeout: 0` disables the timeout; pass `{ timeout: <positive milliseconds>, ...otherOptions }` so a hung or runaway child process cannot block the job indefinitely.",
- Files reviewed: 2/2 changed files
- Comments generated: 2
- Review effort level: Balanced
| const isMissingTimeout = | ||
| (value.type === AST_NODE_TYPES.Literal && (value.value == null || (typeof value.value === "number" && value.value <= 0))) || | ||
| (value.type === AST_NODE_TYPES.UnaryExpression && value.operator === "-" && value.argument.type === AST_NODE_TYPES.Literal && typeof value.argument.value === "number") || | ||
| (value.type === AST_NODE_TYPES.Identifier && value.name === "undefined"); | ||
| if (!isMissingTimeout) return true; |
| } | ||
|
|
||
| /** Returns true when the options-object argument for the call statically carries a non-nullish `timeout` property. */ | ||
| /** Returns true when the options-object argument for the call statically carries a positive `timeout` property. */ |
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /tdd — requesting changes on two focused gaps.
📋 Key Themes & Highlights
Issues Found
timeout: +0not caught — theUnaryExpressionbranch only guards against unary minus;+0(unary plus on0) evaluates to0at runtime and bypasses the check. Line 132 of the implementation.- Missing coverage for
execFileSync/spawnSync— the new zero/negative test cases only exerciseexecSync; the other two methods share the same logic but have no regression tests for this fix.
Positive Highlights
- ✅ Correct fix for the core bug:
timeout: 0was silently treated as valid despite Node's documented behaviour. - ✅ Good diagnostic message improvement — explicitly calling out that
timeout: 0disables the timeout is actionable. - ✅
-0(negative zero literal) correctly handled through theUnaryExpressionbranch. - ✅ Non-literal values (
userConfig.timeout) correctly remain allowed.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 · 26.3 AIC · ⌖ 8.53 AIC · ⊞ 7.7K
Comment /matt to run again
| if (!isNullish) return true; | ||
| const isMissingTimeout = | ||
| (value.type === AST_NODE_TYPES.Literal && (value.value == null || (typeof value.value === "number" && value.value <= 0))) || | ||
| (value.type === AST_NODE_TYPES.UnaryExpression && value.operator === "-" && value.argument.type === AST_NODE_TYPES.Literal && typeof value.argument.value === "number") || |
There was a problem hiding this comment.
[/tdd] The UnaryExpression branch catches timeout: -1 and timeout: -0, but timeout: +0 slips through — unary + applied to 0 evaluates to 0 at runtime, which Node treats as "no timeout".
💡 Suggested fix
Evaluate the unary expression numerically instead of only checking operator === "-":
(value.type === AST_NODE_TYPES.UnaryExpression &&
(value.operator === "-" || value.operator === "+") &&
value.argument.type === AST_NODE_TYPES.Literal &&
typeof value.argument.value === "number" &&
(value.operator === "-" ? -value.argument.value : +value.argument.value) <= 0)And add a regression test case:
{ code: `const { execSync } = require("child_process"); execSync("git status", { timeout: +0 });`, errors: [{ messageId: "requireTimeout" }] },@copilot please address this.
| code: `const { execSync } = require("child_process"); execSync("git status", { timeout: -0 });`, | ||
| errors: [{ messageId: "requireTimeout" }], | ||
| }, | ||
| ], |
There was a problem hiding this comment.
[/tdd] The new zero/negative timeout test cases only cover execSync. execFileSync and spawnSync share the same hasTimeoutOption path but have no parallel coverage for timeout: 0 or timeout: -1.
💡 Suggested addition
Add at least one case per method to the existing "invalid: execFileSync and spawnSync without timeout option" describe block:
{ code: `const { execFileSync } = require("child_process"); execFileSync("git", ["status"], { timeout: 0 });`, errors: [{ messageId: "requireTimeout" }] },
{ code: `const { spawnSync } = require("child_process"); spawnSync("git", ["status"], { timeout: 0 });`, errors: [{ messageId: "requireTimeout" }] },@copilot please address this.
🧪 Test Quality Sentinel Report — PR #52978Score: 66/100 SummaryThis PR adds 4 new test cases that directly verify the stricter timeout validation logic introduced in the source changes. All new tests target high-value edge cases ( Test Changes
Detailed Test AnalysisNew and Modified Tests (5 cases)
Quality Observations✅ Strengths:
CalculationVerdict✅ No violations — Implementation tests represent ≤30% of coverage; all tests are high-value and directly verify the design contract (synchronous child processes must have positive timeouts).
|
|
@copilot Quick triage for maintainer-ready follow-up: Please refresh the branch if needed, address the remaining maintainer-facing follow-up, and run the Outstanding review items (newest first):
Failed checks from the compact candidate set:
Branch update was requested automatically for this run when GitHub allows it.
|
|
🎉 This pull request is included in a new release. Release: |
require-sync-exec-timeoutpreviously acceptedtimeout: 0, even though Node treats it as no timeout. This left synchronous child processes able to block indefinitely while appearing compliant.Rule behavior
timeout: 0and negative numeric literals.Diagnostic
timeout: 0disables the timeout and require a positive millisecond value.Regression coverage
timeout: 0as satisfying the requirement, but 0 means "no timeout" #52645