diff --git a/TEST-EXAMPLES.md b/TEST-EXAMPLES.md index 8825a3810..558cc572c 100644 --- a/TEST-EXAMPLES.md +++ b/TEST-EXAMPLES.md @@ -73,6 +73,51 @@ for (const scenario of cases) { } ``` +## Named Timeout Constants, Not Ad Hoc Literals + +A bare numeric `timeout`/`timeoutMs` guessed per call site can silently drift from — or worse, +exactly collide with — an unrelated timeout somewhere else. That collision is not hypothetical: a +test's outer subprocess-wait timeout once matched a worker's own inner `npm view` timeout exactly +(both hardcoded to `15000`), so a slow response raced two SIGKILLs at the same instant and lost — +only on Windows CI, only intermittently. See [`TESTING-STANDARDS.md` — "No ad hoc timeout +literals"](TESTING-STANDARDS.md#no-ad-hoc-timeout-literals) for the full incident and +`local/no-adhoc-timeout-literal` for the lint rule that now catches this. + +**Non-compliant — a guessed literal with no relationship to what it's actually bounding:** + +```javascript +test('worker run leaves a valid cache', (t) => { + const r = runHookSeam(WORKER_PATH, [], { timeoutMs: 15000 }); // why 15000? nobody knows + assert.equal(r.exitCode, 0); +}); +``` + +**Compliant — reuse a shared class-norm constant when the call is the same class of subprocess:** + +```javascript +const { PROBE_TIMEOUT_MS } = require('./helpers/timeouts.cjs'); + +test('gsd-tools reports the resolved config', (t) => { + const r = runNode([TOOLS_PATH, 'config', '--json'], { timeoutMs: PROBE_TIMEOUT_MS }); + assert.equal(r.exitCode, 0); +}); +``` + +**Compliant — a genuinely distinct class: name it, and size it relative to what it wraps:** + +```javascript +const { NPM_VIEW_TIMEOUT_MS } = require('../gsd-core/bin/check-latest-version.cjs'); + +// Real headroom beyond the inner timeout the worker itself is bounded by — not a +// second independent guess. See TESTING-STANDARDS.md's "No ad hoc timeout literals". +const WORKER_TEARDOWN_MARGIN_MS = 10_000; + +test('worker run leaves a valid cache', (t) => { + const r = runHookSeam(WORKER_PATH, [], { timeoutMs: NPM_VIEW_TIMEOUT_MS + WORKER_TEARDOWN_MARGIN_MS }); + assert.equal(r.exitCode, 0); +}); +``` + ## Parser Adversarial Fixtures Parser tests should cover malformed input and real-world file messiness. Prefer named fixtures under `tests/fixtures/adversarial//` when the input is reusable. diff --git a/TESTING-STANDARDS.md b/TESTING-STANDARDS.md index a2df72c9a..4a96cd87a 100644 --- a/TESTING-STANDARDS.md +++ b/TESTING-STANDARDS.md @@ -174,6 +174,38 @@ Invariant categories to consider: round-trip, monotonicity, boundary containment **Enforcement:** Code review verifies that property tests exist for modules in scope. Stryker mutation score below 80 % blocks merge (see next section). +### No ad hoc timeout literals + +Do not write a bare numeric `timeout`/`timeoutMs` option value at a test call site. Two independently-guessed copies of the same magic number can silently drift apart, or worse, collide exactly and produce a zero-margin race: `bin/check-latest-version.cjs`'s `timeout: 15_000` and this suite's independent `timeoutMs: 15000` could SIGKILL the whole process tree at the exact same instant, and it failed specifically on Windows CI (fixed in PR #4428). + +**Non-compliant:** + +```javascript +const r = runHookSeam(WORKER_PATH, [], { timeoutMs: 15000 }); +``` + +**Compliant — same class of subprocess as an existing class-norm:** + +```javascript +const { GIT_TIMEOUT_MS } = require('./helpers/timeouts.cjs'); + +const r = runHookSeam(WORKER_PATH, [], { timeoutMs: GIT_TIMEOUT_MS }); +``` + +**Compliant — a genuinely distinct class, declared locally with a margin over the thing it wraps** (the actual fix in PR #4428 — the worker's inner `npm view` call is bounded by its own named `NPM_VIEW_TIMEOUT_MS`, so the outer test imports it and adds explicit headroom instead of re-guessing a number): + +```javascript +const { NPM_VIEW_TIMEOUT_MS } = require('../gsd-core/bin/check-latest-version.cjs'); + +const WORKER_TEARDOWN_MARGIN_MS = 10_000; // real headroom beyond the inner timeout it wraps + +const r = runHookSeam(WORKER_PATH, [], { timeoutMs: NPM_VIEW_TIMEOUT_MS + WORKER_TEARDOWN_MARGIN_MS }); +``` + +Import an existing class-norm constant from `tests/helpers/timeouts.cjs` (`PROBE_TIMEOUT_MS`, `GIT_TIMEOUT_MS`, `BUILD_TIMEOUT_MS`, `INSTALL_TIMEOUT_MS`) when the call is the same class of subprocess, or declare a local one with a comment justifying why it is a distinct class — see CONTRIBUTING.md's "Use Centralized Test Helpers" section. + +**Enforcement:** `local/no-adhoc-timeout-literal` (ESLint, `error`). A non-literal value (an `Identifier`, `MemberExpression`, or `CallExpression`) is trusted; only a resolvable numeric literal is flagged. There is no marker-comment escape — the fix is always to extract a named constant. `allowlist` (`eslint-rules/no-adhoc-timeout-literal.allowlist.json`) exempts pre-existing legacy violations and only ever ratchets down. + ### Mutation testing — 80 % threshold Stryker runs in incremental mode (`--since origin/next`) on the `ubuntu-latest` / Node 24 CI leg as a PR-gating signal. The default threshold is **80 % mutation score** (killed / total mutants in the changed scope). PRs that drop below this threshold must either add tests that kill the surviving mutants or add the specific path to `stryker.config.mjs` with a documented reason. @@ -210,6 +242,7 @@ Real multi-process race tests are deleted once the corresponding deterministic c | `local/no-source-grep` | `error` (promoted by #3313) | `readFileSync` on source files + text assertions; `assert.match`/`doesNotMatch` on raw stdout/stderr | | `local/no-magic-sleep-in-tests` | `error` | `setTimeout`/`sleep`/`delay` calls inside `test()`/`it()`/`describe()` bodies | | `local/no-elapsed-assertion` | `error` (promoted by #3331, precondition delivered by #3314) | Assertions on `Date.now()` delta, `process.hrtime()`, `performance.now()` comparisons | +| `local/no-adhoc-timeout-literal` | `error` | Bare numeric `timeout`/`timeoutMs` option literal in `tests/**/*.cjs` (PR #4428) | | `no-only-tests/no-only-tests` | `error` | `test.only`/`describe.only`/`it.only` committed to non-scratch files | | `no-restricted-syntax` (ban 1) | `error` | Top-level `setTimeout` in `ExpressionStatement` | | `no-restricted-syntax` (ban 2) | `error` | `.only` member access on `test`/`it`/`describe` (belt-and-suspenders) | diff --git a/eslint-rules/no-adhoc-timeout-literal.allowlist.json b/eslint-rules/no-adhoc-timeout-literal.allowlist.json new file mode 100644 index 000000000..f09d1b4b2 --- /dev/null +++ b/eslint-rules/no-adhoc-timeout-literal.allowlist.json @@ -0,0 +1,128 @@ +[ + "tests/adr-612-bracket-coherence.test.cjs", + "tests/adr-612-bracket-read-tolerance.test.cjs", + "tests/adr-index-gate.test.cjs", + "tests/adr857-core-without-capabilities.test.cjs", + "tests/antigravity-upgrades.test.cjs", + "tests/api-coverage-gate-e2e.test.cjs", + "tests/api-coverage.test.cjs", + "tests/assumption-delta-checkpoint-e2e.test.cjs", + "tests/assumption-delta.test.cjs", + "tests/augment-upgrades.test.cjs", + "tests/capability-cli.test.cjs", + "tests/capability-probe-fallback.test.cjs", + "tests/capability-state.test.cjs", + "tests/capability-trust.test.cjs", + "tests/capability-validator-task-content-resolver.test.cjs", + "tests/capability-writer.test.cjs", + "tests/changeset-new.test.cjs", + "tests/check-env.test.cjs", + "tests/check-gap-analysis-plan-post-e2e.test.cjs", + "tests/check-glossary-refs.test.cjs", + "tests/check-predicate.test.cjs", + "tests/check-tdd-review-checkpoint-e2e.test.cjs", + "tests/ci-rebase-check.test.cjs", + "tests/cjs-command-router-adapter.test.cjs", + "tests/code-review-pipeline-regression.test.cjs", + "tests/code-review.test.cjs", + "tests/commands.test.cjs", + "tests/commit-files-pathspec.test.cjs", + "tests/config-get-default.test.cjs", + "tests/cursor-hook-workspace-roots.test.cjs", + "tests/cursor-hooks.test.cjs", + "tests/dispatcher.test.cjs", + "tests/effort-surface-axis.test.cjs", + "tests/effort-sync-installed-runtime.test.cjs", + "tests/emitted-ack-trailer.test.cjs", + "tests/emitted-attribution.test.cjs", + "tests/execute-wave-post-gate-pipeline-e2e.test.cjs", + "tests/faulty-deps.test.cjs", + "tests/feat-2483-review-claude-mds-guard.test.cjs", + "tests/federated-config.test.cjs", + "tests/fragment-single-edit-propagation.install.test.cjs", + "tests/gate-predicate-evaluator.test.cjs", + "tests/gemini-runtime-removed.test.cjs", + "tests/gen-context-index.test.cjs", + "tests/gen-health-docs.test.cjs", + "tests/gen-section-manifest.test.cjs", + "tests/gen-state-md-docs.test.cjs", + "tests/git-base-branch.test.cjs", + "tests/git-fixture.test.cjs", + "tests/graphify.test.cjs", + "tests/gsd-check-update-worker-platform-gate.test.cjs", + "tests/gsd-mcp-server-bin.test.cjs", + "tests/gsd-secret-read-guard.test.cjs", + "tests/gsd-statusline.test.cjs", + "tests/gsd-validate-commit-crash-policy.test.cjs", + "tests/gsd-write-guard.test.cjs", + "tests/health-validation.test.cjs", + "tests/helpers-process-isolation.test.cjs", + "tests/helpers.cjs", + "tests/hooks-commonjs-marker.test.cjs", + "tests/hooks-crash-policy.test.cjs", + "tests/init.test.cjs", + "tests/install-minimal-hooks.test.cjs", + "tests/install-regressions.test.cjs", + "tests/install-runtime-artifacts.test.cjs", + "tests/install-write-confinement.test.cjs", + "tests/install.test.cjs", + "tests/kilo-upgrades.test.cjs", + "tests/kimi-upgrades.test.cjs", + "tests/kimi-variant-disambiguation.test.cjs", + "tests/lint-docs-command-form.test.cjs", + "tests/locking-bugs-1909-1916-1925-1927.test.cjs", + "tests/loop-hooks-empty-points-e2e.test.cjs", + "tests/loop-hooks-ship-pre-e2e.test.cjs", + "tests/loop-hooks-verify-post-e2e.test.cjs", + "tests/loop-render-hooks.test.cjs", + "tests/loop-walk.qa.test.cjs", + "tests/milestone-lock.test.cjs", + "tests/no-pending-3212-markers.test.cjs", + "tests/npm-integrity-gate.test.cjs", + "tests/opencode-plugin-adapter.test.cjs", + "tests/packaging-shipped-scripts-require-only-shipped.test.cjs", + "tests/pattern.test.cjs", + "tests/perf-316-state-lock-buffer-alloc.test.cjs", + "tests/perf-317-context-monitor-fs.test.cjs", + "tests/phase.test.cjs", + "tests/phase6-capstone-conformance.test.cjs", + "tests/pi-config-dir-env-override.test.cjs", + "tests/plan-phase-stall-detection.test.cjs", + "tests/plan-pre-hook-e2e.test.cjs", + "tests/plan-review-convergence.test.cjs", + "tests/plugin-manifest.test.cjs", + "tests/policy-160-route0-resume.test.cjs", + "tests/pr-branch-planning-filter.test.cjs", + "tests/process-seam.test.cjs", + "tests/prohibition-enforcement.test.cjs", + "tests/prompt-injection-scan.security.test.cjs", + "tests/qa/tdd-walk.cjs", + "tests/quick-batch.test.cjs", + "tests/read-guard.test.cjs", + "tests/read-injection-scanner.property.test.cjs", + "tests/read-injection-scanner.security.test.cjs", + "tests/reapply-verify-hunks.test.cjs", + "tests/release-tarball-smoke.install.test.cjs", + "tests/representative-corpus.test.cjs", + "tests/review-lane-invocation.test.cjs", + "tests/reviewer-manifest-body.test.cjs", + "tests/reviewer-trust-disclosure.test.cjs", + "tests/run-tests-temp-root.test.cjs", + "tests/run-with-timeout.test.cjs", + "tests/secret-scan-lint.security.test.cjs", + "tests/security-prompt-injection.security.test.cjs", + "tests/security-scan.security.test.cjs", + "tests/security.test.cjs", + "tests/shared-hooks-dir-resolution.test.cjs", + "tests/shell-command-projection-dispatch.test.cjs", + "tests/ship-notes-wedged-pr.test.cjs", + "tests/slug-derivation-drift-guard.test.cjs", + "tests/state-document.test.cjs", + "tests/state-todos-render.test.cjs", + "tests/task-command-router-resolve-content.test.cjs", + "tests/task-content-resolution.test.cjs", + "tests/task-content-resolver-grammar-parity.test.cjs", + "tests/teams-status.test.cjs", + "tests/windsurf-hooks-bridge.test.cjs", + "tests/worktree-safety.test.cjs" +] diff --git a/eslint-rules/no-adhoc-timeout-literal.cjs b/eslint-rules/no-adhoc-timeout-literal.cjs new file mode 100644 index 000000000..5cb038175 --- /dev/null +++ b/eslint-rules/no-adhoc-timeout-literal.cjs @@ -0,0 +1,195 @@ +'use strict'; + +const path = require('path'); + +/** + * no-adhoc-timeout-literal + * + * Flag a bare numeric literal used as a `timeout`/`timeoutMs` option value in + * a test file. This is a **test-suite-only** convention rule — it does not + * touch production `src/`/`bin/` code, where a literal like `execNpm(args, { + * timeout: 15_000 })` bounds a real subprocess for real production + * resilience. CONTRIBUTING.md/CLAUDE.md separately mandate that production + * timeout bound on safety grounds (never hang); this rule is about naming a + * shared, reviewed ceiling instead of scattering ad hoc guesses. + * + * ## What this enforces + * + * A bare `timeout`/`timeoutMs` literal scattered per call site drifts + * silently from its siblings, or worse, collides exactly with one. That is + * not hypothetical: on 2026-09-06, `gsd-core/bin/check-latest-version.cjs` + * hardcoded `execNpm(args, { timeout: 15_000 })` and, independently, + * `tests/gsd-check-update-worker-atomic-cache.test.cjs` hardcoded + * `runHookSeam(WORKER_PATH, [], { timeoutMs: 15000 })` — two unrelated files, + * same guessed number, no shared reference. When the inner one's timeout + * fired, the outer one could SIGKILL the whole process tree at the exact + * same instant before it could degrade gracefully: a zero-margin race that + * failed specifically on Windows CI (fixed in PR #4428 by extracting a named + * `NPM_VIEW_TIMEOUT_MS` constant and referencing it with an explicit + * margin). This rule closes the gap CONTRIBUTING.md already documents: + * "A non-literal value (`timeout: GIT_TIMEOUT_MS`) is trusted — that is the + * shape you should be writing" — by actually enforcing that shape. + * + * ## Recognized shape + * + * Any non-computed `Property` node whose key is exactly `timeout` or + * `timeoutMs` (string or Identifier key form) is flagged when its `value` + * resolves, via `evalNumeric` (Literal number, unary +/-, or a `*`/`+`/`-`/`/` + * BinaryExpression chain — same logic as `no-unbounded-spawn.cjs`), to a + * concrete JS number. An `Identifier` value (including shorthand + * `{ timeoutMs }`), a `MemberExpression` (`opts.timeout`, + * `TIMEOUTS.PROBE`), or a `CallExpression` value all fail to resolve via + * `evalNumeric` and are trusted as-is — this rule does not attempt general + * expression evaluation, matching `no-unbounded-spawn`'s own philosophy of + * trusting anything it can't literally evaluate to a number. + * + * ## No marker-comment escape + * + * Unlike `no-unbounded-spawn`'s `// allow-spawn-timeout-ceiling: ` + * (which has a genuine "sometimes a call really does need >600s" exception), + * there is no legitimate reason a timeout value needs to stay an inline + * literal forever — the fix is always "extract to a named constant," which + * is trivial. The only escape here is the allowlist below, and it is a + * temporary migration aid, not a permanent one. + * + * ## Allowlist + * + * `allowlist` (repo-relative POSIX paths) exempts pre-existing legacy + * violations, with mechanics identical to `no-unbounded-spawn.cjs`: an + * allowlisted file's violations are counted internally but not reported: a + * listed file with zero violations reports `staleAllowlistEntry` so the dead + * entry gets deleted. The allowlist only ever ratchets down. + */ + +/** @type {import('eslint').Rule.RuleModule} */ +const rule = { + meta: { + type: 'problem', + docs: { + description: + 'Disallow a bare numeric literal as a timeout/timeoutMs option value in tests', + category: 'Reliability', + }, + schema: [ + { + type: 'object', + properties: { + allowlist: { type: 'array', items: { type: 'string' } }, + }, + additionalProperties: false, + }, + ], + messages: { + adhocTimeoutLiteral: + 'Bare numeric `{{key}}: {{value}}` literal: two independent hardcoded copies of the ' + + 'same guessed timeout can silently drift apart, or worse, collide exactly and produce ' + + 'a zero-margin race (this repo hit exactly that on 2026-09-06 — ' + + '`bin/check-latest-version.cjs`\'s `timeout: 15_000` and this suite\'s independent ' + + '`timeoutMs: 15000` could SIGKILL the whole tree at the same instant, PR #4428). ' + + 'Extract a named constant: import an existing one from `tests/helpers/timeouts.cjs` ' + + '(`PROBE_TIMEOUT_MS`, `GIT_TIMEOUT_MS`, `BUILD_TIMEOUT_MS`, `INSTALL_TIMEOUT_MS`) if this ' + + 'call is the same class of subprocess, or declare a local one with a comment justifying ' + + 'why it is a distinct class, per CONTRIBUTING.md\'s "Use Centralized Test Helpers" section.', + staleAllowlistEntry: + '{{file}} no longer contains an ad hoc timeout literal. Delete its line from ' + + 'eslint-rules/no-adhoc-timeout-literal.allowlist.json — the allowlist only ratchets down.', + }, + }, + + create(context) { + const options = context.options[0] || {}; + const allowlist = Array.isArray(options.allowlist) ? options.allowlist : []; + + const TIMEOUT_KEYS = new Set(['timeout', 'timeoutMs']); + + const filename = context.filename || context.getFilename(); + const cwd = context.cwd || (context.getCwd ? context.getCwd() : process.cwd()); + const rel = path.relative(cwd, filename).split(path.sep).join('/'); + const allowlisted = allowlist.includes(rel); + let violations = 0; + + /** + * Returns the string value of a Literal node, or null. + */ + function stringValue(node) { + if (node && node.type === 'Literal' && typeof node.value === 'string') { + return node.value; + } + return null; + } + + /** Recursion depth cap for evalNumeric — guards against a pathological + * nested-expression chain blowing the stack. */ + const MAX_EVAL_DEPTH = 20; + + /** + * Recursively evaluates a numeric-ish AST node to a JS number, or + * returns undefined if it's not one of the recognized numeric shapes. + * Handles a numeric Literal, a unary +/- of a recursively-numeric + * argument, and a BinaryExpression (*, +, -, /) where both sides are + * recursively numeric — so a multi-term chain like `60 * 60 * 1000` + * resolves instead of bailing out on the first nested BinaryExpression. + * Identical logic to `no-unbounded-spawn.cjs`'s `evalNumeric`. + */ + function evalNumeric(node, depth = 0) { + if (depth > MAX_EVAL_DEPTH) return undefined; + if (node.type === 'Literal' && typeof node.value === 'number') { + return node.value; + } + if (node.type === 'UnaryExpression' && (node.operator === '-' || node.operator === '+')) { + const arg = evalNumeric(node.argument, depth + 1); + if (arg === undefined) return undefined; + return node.operator === '-' ? -arg : arg; + } + if ( + node.type === 'BinaryExpression' && + (node.operator === '*' || node.operator === '+' || node.operator === '-' || node.operator === '/') + ) { + const left = evalNumeric(node.left, depth + 1); + const right = evalNumeric(node.right, depth + 1); + if (left === undefined || right === undefined) return undefined; + switch (node.operator) { + case '*': + return left * right; + case '+': + return left + right; + case '-': + return left - right; + case '/': + return left / right; + default: + return undefined; + } + } + return undefined; + } + + return { + Property(node) { + if (node.computed) return; + const keyName = node.key.type === 'Identifier' ? node.key.name : stringValue(node.key); + if (!keyName || !TIMEOUT_KEYS.has(keyName)) return; + + const numeric = evalNumeric(node.value); + if (numeric === undefined) return; + + violations += 1; + if (allowlisted) return; + + context.report({ + node, + messageId: 'adhocTimeoutLiteral', + data: { key: keyName, value: String(numeric) }, + }); + }, + + 'Program:exit'(node) { + if (allowlisted && violations === 0) { + context.report({ node, messageId: 'staleAllowlistEntry', data: { file: rel } }); + } + }, + }; + }, +}; + +module.exports = rule; diff --git a/eslint.config.mjs b/eslint.config.mjs index a017ad849..c20e84471 100644 --- a/eslint.config.mjs +++ b/eslint.config.mjs @@ -5,8 +5,10 @@ import pluginN from 'eslint-plugin-n'; import noOnlyTests from 'eslint-plugin-no-only-tests'; import { dirname } from 'path'; import { fileURLToPath } from 'url'; +import { createRequire } from 'module'; const __dirname = dirname(fileURLToPath(import.meta.url)); +const require = createRequire(import.meta.url); // Local plugin with custom AST rules import noSourceGrep from './eslint-rules/no-source-grep.cjs'; @@ -36,6 +38,9 @@ import noPrivateBinaryResolution from './eslint-rules/no-private-binary-resoluti import requireRegisteredExit from './eslint-rules/require-registered-exit.cjs'; import noSwallowedPrecondition from './eslint-rules/no-swallowed-precondition.cjs'; import noExactCaseEnvAccess from './eslint-rules/no-exact-case-env-access.cjs'; +import noAdhocTimeoutLiteral from './eslint-rules/no-adhoc-timeout-literal.cjs'; + +const adhocTimeoutLiteralAllowlist = require('./eslint-rules/no-adhoc-timeout-literal.allowlist.json'); const localPlugin = { rules: { @@ -66,6 +71,7 @@ const localPlugin = { 'require-registered-exit': requireRegisteredExit, 'no-swallowed-precondition': noSwallowedPrecondition, 'no-exact-case-env-access': noExactCaseEnvAccess, + 'no-adhoc-timeout-literal': noAdhocTimeoutLiteral, }, }; @@ -715,6 +721,11 @@ export default tseslint.config( // exemption surface. The only sanctioned escapes are an explicit `timeout` on // a raw spawn or the `// allow-spawn-timeout-ceiling: ` marker. 'local/no-unbounded-spawn': 'error', + // Ban a bare numeric `timeout`/`timeoutMs` literal in tests (DEFECT.AD-HOC-TIMEOUT-LITERAL, + // #4428): two independently-guessed copies of the same magic number can drift apart, or + // collide exactly into a zero-margin race. Allowlist starts empty; a pre-existing violation + // gets grandfathered in here as it's found, per eslint-rules/no-adhoc-timeout-literal.allowlist.json. + 'local/no-adhoc-timeout-literal': ['error', { allowlist: adhocTimeoutLiteralAllowlist }], // Ban a consolidation-epic folded suite appearing twice in one host file (#3271). // A second copy runs the same tests twice on every lane and drifts silently. 'local/no-duplicate-fold-marker': 'error',