Files
msd-core/eslint-rules/no-tautological-assert.cjs
Tom Boucher 5fa4dcd78c fix: recover silently-excluded test dirs + test-architecture audit hardening (#1195)
* fix: recurse test discovery so subdir test suites actually run

scripts/run-tests.cjs discovered tests with a flat readdirSync(testDir),
silently excluding tests/observability/ (4 files), tests/dispatch/ (1) and
tests/installer-migrations/ (1) — 94 passing tests — from `npm test` and all
CI lanes. Walk the tree recursively (relative subpaths preserved), classify
suites by basename, and add a fail-on-zero-executed guard for suite/default
runs (escape hatch GSD_ALLOW_EMPTY_SUITE=1) while preserving the empty
--files/--files-from path the CI inert lane relies on.

Unit suite 735 -> 741 files; surfaces ADR-227's observability/dispatch seam.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

* test: retire 5 verified-worthless tests

Adversarial verification confirmed these 5 prove nothing — their coverage is
provided more strictly elsewhere:
- enh-2790 'has a name: field' spot-checks (command-contract enforces /^gsd[:-]/)
- command-routing-hub duplicate construct + duplicate ERROR_KINDS assertions
- no-cjs-sdk-handsync-tooling (guarded files that never existed on main; bug-190
  covers the real retired SDK artifacts)
- runtime-artifact-layout cline edge case (subsumed by the explicit-global test
  and bug-782-cline-skills-emission)

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

* test: add ADR-218 release version-validation coverage

ADR-218 (reject leading-zero versions like 1.01.0; npm duplicate pre-check) had
zero tests — the logic lived only in release.yml bash. Add a test that extracts
the actual rejection regexes from the workflow and exercises them against a
boundary table (leading-zero/malformed rejected, valid accepted) plus structural
wiring assertions. Goes red if the regex is reverted to [0-9]+.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

* test: redesign weak tests into behavioral, deterministic assertions

Per the ADR test audit, rewrite 27 weak test files (test-only, no source
changes) so each can go red for the defect it guards:
- kill pass-always assert.ok(true) placeholders (research-cli, worktree-baseref,
  bug-260 security guard, eslint-rules x24, clusters '|| true')
- replace source-text grep with behavioral calls (install Kilo, sh-hook-paths,
  plan-review-convergence) and add a repo-layout governance test
- de-flake real-clock/Math.random coupling (phase last_updated, bug-3707 mtime,
  context-utilization property, feat-3594)
- fix independence/shared-state violations (bug-492 singleton, issue-844 tmpRoot,
  core reapStaleTempFiles, active-workstream TTY, feat-488 GSD_HOME)
- strengthen property/shape-only tests (research-provider/store classification +
  collision) and unconditional plugin.json schema validation (issue-766)

Verified: all 28 files run together 1220 pass / 0 fail / 1 skip.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

* chore: add no-tautological-assert lint rule, error in test suite

New custom ESLint rule (eslint-rules/no-tautological-assert.cjs) bans asserts
that can never fail: assert(true)/assert.ok(<always-truthy literal>),
'cond || true' inside an assert, and equality asserts comparing two identical
literals. Wired as error on tests/**; full sweep confirmed zero existing
violations so the suite stays green. Prevents the placeholder-assert regressions
the audit redesigns just removed. RuleTester coverage added (6 valid, 8 invalid).

Note: no-only-tests was already enforced via eslint-plugin-no-only-tests, so no
duplicate rule was added.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

* chore: gate new allow-test-rule exemptions to require an issue ref

ADR-456 requires any allow-test-rule exemption added after the ADR to carry a
tracking issue number, but nothing enforced it. New ratchet gate
(scripts/lint-allow-test-rule-refs.cjs, wired into lint:ci) fails when a NEW
allow-test-rule comment lacks a #NNN/URL reference; the 323 existing untracked
exemptions are grandfathered in an allowlist that ratchets down as they gain
refs. Red-green verified (novel untracked offender fails; compliant passes).

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

* docs: add ADR test-audit evidence report (#1192)

Full risk-first qa-test-architect audit of the ADR portfolio (37 ADRs + 4
platform lenses, adversarial verification of retire verdicts) that drove the
P0 discovery fix, ADR-218 coverage, 5 retires, 27 redesigns, and the two new
lint gates. Filed as point-in-time evidence under docs/issueevidence/, named
for tracking issue #1192.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

* test: replace pre-existing raw NUL byte with escape in feat-3594 fixture

feat-3594's null-byte parser fixture contained a literal NUL byte (pre-existing
on next at b10e5681 — confirmed: base blob has 1 NUL, this fix has 0), which
made git treat the file as binary and would break grep/editors. Switch to the
\x00 escape; the runtime string value (a real NUL in the parser input) is
unchanged.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

* test: address adversarial-review findings

Codex adversarial pass over the branch:
- capability-registry drift test no longer mutates the committed generated
  capability-registry.cjs in place (concurrency hazard) — uses in-memory
  checkPipeline comparison instead.
- allow-test-rule ratchet now detects exemptions in ALL comment forms (block
  /* */ too, matching no-source-grep) so a block comment can't bypass it;
  one newly-surfaced pre-existing offender grandfathered (323->324).
- install.test Kilo case asserts on what install(false,'kilo') actually writes
  rather than manually calling configureKiloPermissions (masked the call site).
- issue-766 drops the undeclared transitive ajv dep for explicit structural
  assertions from the schema fixture.
- adr-218 test notes the hotfix leading-zero gap is tracked in #1186.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

* fix: address code-review findings (subdir discovery, rule + test gaps)

xhigh code review surfaced 15 confirmed issues, all fixed:
- run-tests.cjs --files now resolves subdir tests by bare basename + handles
  Windows backslash paths (ambiguous basenames error clearly).
- affected-tests-lib.cjs listTestFiles made recursive — the targeted CI lane was
  silently dropping changed subdir tests (same false-green class the audit fixed).
- no-tautological-assert now catches 'true || cond' and empty []/{}  equality.
- verify-test-quality: restore provenance-classification coverage, tighten the
  writeFile circular-detection check, guard the module-level file read.
- sh-hook-paths: cover the global-install .sh delegation branch (#2045 guard).
- active-workstream null-guard runs deterministically (no longer skipped on TTY).
- adr-218 structural guards tightened (major/minor leading-zero; needs: membership).
- repo-layout AGENTS.md guard no longer false-alarms on equivalent refactors.
- cross-ai ordering guard fails red when the step is missing.
- issue-766 parses required fields from the schema fixture (auto-enforced).

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

* test: stub USERPROFILE alongside HOME in feat-488 (Windows parity)

The feat-488 redesign stubbed process.env.HOME but not USERPROFILE; os.homedir()
resolves from USERPROFILE on Windows, so the home stub was not hermetic there —
caught by windows-test-parity-guard (stubsHomeNoUserProfile). Save/set/restore
USERPROFILE symmetrically with HOME (delete-if-originally-undefined).

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

* chore: reconcile allow-test-rule allowlist after rebase onto next

Rebasing onto current next pulled in merged PR #1170, which added
inventory-headings-countfree.test.cjs (a baseline allow-test-rule exemption) and
deleted inventory-counts.test.cjs. Grandfather the former and prune the latter so
the ratchet matches the merged tree. No new debt from this PR.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
2026-06-13 23:35:08 -04:00

204 lines
6.5 KiB
JavaScript

'use strict';
/**
* no-tautological-assert
*
* Flag assert*() calls whose argument(s) can never fail — i.e. the assertion
* is tautologically true at the AST level and therefore provides no test value.
*
* Two categories:
*
* (a) Truthiness asserts — assert(x) / assert.ok(x) — where x is an
* always-truthy literal:
* - boolean literal `true`
* - non-zero numeric Literal (1, 42, …)
* - non-empty string Literal ("always", …)
* - RegExp, Array, or Object expression (always truthy objects)
* - UnaryExpression !!<literal> (double-bang a literal)
* - LogicalExpression `cond || true` (right side is true)
*
* (b) Equality asserts — assert.strictEqual / assert.equal /
* assert.deepEqual / assert.deepStrictEqual — where the first two
* arguments are identical literals (same type AND same value).
*/
/** @type {import('eslint').Rule.RuleModule} */
const rule = {
meta: {
type: 'problem',
docs: {
description:
'Disallow assertions that can never fail due to always-truthy or identical literal arguments',
category: 'Best Practices',
},
schema: [],
messages: {
tautologicalTruthiness:
'Tautological assertion: the argument is always truthy so this assert will never fail. Assert on an actual test value instead.',
tautologicalEquality:
'Tautological assertion: both arguments are the same literal value so this equality assert will always pass. Assert on an actual test value instead.',
},
},
create(context) {
// Method names for truthiness asserts
const TRUTHINESS_METHODS = new Set(['ok']); // bare assert() is handled separately
// Method names for equality asserts
const EQUALITY_METHODS = new Set([
'strictEqual',
'equal',
'deepEqual',
'deepStrictEqual',
]);
/**
* Returns true when the node is an always-truthy literal (per the spec).
*/
function isAlwaysTruthyLiteral(node) {
if (!node) return false;
// boolean `true`
if (node.type === 'Literal' && node.value === true) return true;
// non-zero numeric literal
if (node.type === 'Literal' && typeof node.value === 'number' && node.value !== 0) return true;
// non-empty string literal
if (node.type === 'Literal' && typeof node.value === 'string' && node.value !== '') return true;
// RegExp literal /foo/
if (node.type === 'Literal' && node.regex != null) return true;
// Array expression [] or [...]
if (node.type === 'ArrayExpression') return true;
// Object expression {} or {...}
if (node.type === 'ObjectExpression') return true;
// UnaryExpression !!<literal>
if (
node.type === 'UnaryExpression' &&
node.operator === '!' &&
node.argument.type === 'UnaryExpression' &&
node.argument.operator === '!'
) {
return isAlwaysTruthyLiteral(node.argument.argument);
}
// LogicalExpression `cond || true` OR `true || cond`
// Either form short-circuits to always be truthy.
if (
node.type === 'LogicalExpression' &&
node.operator === '||'
) {
if (node.right.type === 'Literal' && node.right.value === true) return true;
if (node.left.type === 'Literal' && node.left.value === true) return true;
}
return false;
}
/**
* Returns true when both nodes are Literals of the SAME type and SAME value,
* OR when both are empty ArrayExpressions ([]) or empty ObjectExpressions ({}).
* Empty [] and {} are always deep-equal to each other.
*/
function areIdenticalLiterals(a, b) {
if (!a || !b) return false;
// Two empty array literals: [] deepStrictEqual [] is always true
if (
a.type === 'ArrayExpression' &&
b.type === 'ArrayExpression' &&
a.elements.length === 0 &&
b.elements.length === 0
) {
return true;
}
// Two empty object literals: {} deepStrictEqual {} is always true
if (
a.type === 'ObjectExpression' &&
b.type === 'ObjectExpression' &&
a.properties.length === 0 &&
b.properties.length === 0
) {
return true;
}
if (a.type !== 'Literal' || b.type !== 'Literal') return false;
// Compare by type tag and value
if (typeof a.value !== typeof b.value) return false;
return a.value === b.value;
}
/**
* Determine whether this call expression is `assert(...)` (bare identifier)
* or `assert.ok(...)` / `assert.strictEqual(...)` etc.
*
* Returns:
* { kind: 'bare' } — assert(...)
* { kind: 'method', name } — assert.<name>(...)
* null — not an assert call
*/
function classifyAssertCall(node) {
const callee = node.callee;
// assert(...) — bare identifier
if (callee.type === 'Identifier' && callee.name === 'assert') {
return { kind: 'bare' };
}
// assert.<method>(...) — member expression on the assert identifier
if (
callee.type === 'MemberExpression' &&
!callee.computed &&
callee.object.type === 'Identifier' &&
callee.object.name === 'assert' &&
callee.property.type === 'Identifier'
) {
return { kind: 'method', name: callee.property.name };
}
return null;
}
return {
CallExpression(node) {
const classification = classifyAssertCall(node);
if (!classification) return;
const args = node.arguments;
if (classification.kind === 'bare') {
// assert(<arg>) — check arg[0] for always-truthy literal
if (args.length >= 1 && isAlwaysTruthyLiteral(args[0])) {
context.report({ node, messageId: 'tautologicalTruthiness' });
}
return;
}
const methodName = classification.name;
if (TRUTHINESS_METHODS.has(methodName)) {
// assert.ok(<arg>) — check arg[0] for always-truthy literal
if (args.length >= 1 && isAlwaysTruthyLiteral(args[0])) {
context.report({ node, messageId: 'tautologicalTruthiness' });
}
return;
}
if (EQUALITY_METHODS.has(methodName)) {
// assert.strictEqual(a, b) etc. — check if both are identical literals
if (args.length >= 2 && areIdenticalLiterals(args[0], args[1])) {
context.report({ node, messageId: 'tautologicalEquality' });
}
return;
}
},
};
},
};
module.exports = rule;