diff --git a/CONTEXT.md b/CONTEXT.md index f0e2f69e6..5758f3725 100644 --- a/CONTEXT.md +++ b/CONTEXT.md @@ -136,6 +136,21 @@ Per-task runtime gate in `/gsd-execute-phase` that, when both `MVP_MODE` and `TD ### SPIDR Splitting Five-axis story decomposition discipline (**S**pike, **P**aths, **I**nterfaces, **D**ata, **R**ules) used by `/gsd-mvp-phase` when a User Story is too large for one phase. Full interactive flow per PRD #2826 Q3 (not a lightweight filter). Reference: `get-shit-done/references/spidr-splitting.md`. +### Clock seam +An injectable time abstraction accepted as an optional parameter by production code (`{ clock = Date } = {}`). Test code substitutes `node:test` `mock.timers` to control time deterministically without waiting for real OS scheduler events. Canonical pattern established by ADR 456 (`docs/adr/456-test-rigor-architecture.md`). + +### Deterministic scheduler +Test-execution model in which all timing and concurrency outcomes are fully controlled by the test (via clock seam, explicit `await` ordering, or synchronous stepping) rather than by the OS thread scheduler. Opposed to real-race tests, which are non-deterministic on loaded CI runners. + +### Property-based test +A test that generates many adversarial inputs automatically (via `fast-check`) and asserts that a stated invariant holds for all of them, rather than asserting on a fixed set of hand-chosen examples. Invariant categories used in this codebase: round-trip, monotonicity, boundary containment, idempotency. See `RULESET.TESTS.property-based-testing`. + +### Mutation testing / mutation score +Stryker injects small code mutations (e.g., flipping a `>` to `>=`, deleting a `return` statement) and reruns the test suite for each. A mutation is "killed" if at least one test fails; "surviving" if all tests pass despite the mutation. Mutation score = killed / total. Score below 80 % on the changed scope blocks PR merge. See `RULESET.TESTS.mutation-score`. + +### ESLint harness +The canonical lint infrastructure adopted in ADR 452 (`docs/adr/452-eslint-lint-harness.md`): ESLint flat config (`eslint.config.mjs`) with `typescript-eslint`, `eslint-plugin-n`, `eslint-plugin-no-only-tests`, and a local AST-rule plugin at `scripts/eslint-rules/`. Replaces the homegrown `scripts/lint-*.cjs` regex scanners. The three custom test-rigor rules (`local/no-source-grep`, `local/no-magic-sleep-in-tests`, `local/no-elapsed-assertion`) initially ship at `warn`; they become `error` after the cleanup sweep tracked at issue #453 merges. + --- ## Test rules and lint @@ -155,6 +170,13 @@ Five-axis story decomposition discipline (**S**pike, **P**aths, **I**nterfaces, `RULESET.TESTS.boundary-coverage.anti-pattern=test suites that pair budget:1_000_000 (trivially fits) with budget:1 (trivially overflows) and skip the boundary region; failure mode that shipped PR #3708 UNNEEDED_TRIM + FALSE_HARDFAIL regressions (commit 2df566ed, fixed bde1ae8f)` `LEARNING.prompt-budget.boundary-gap=PR #3708 commit 2df566ed reserved NOTE_RESERVE_TOKENS in pressure-threshold AND in minSet pre-check; both buggy paths only fire when baseTokens ∈ (effectiveBudget - NOTE_RESERVE_TOKENS, effectiveBudget]; original test suite used budgets far from that band so neither path was exercised; fix bde1ae8f confines NOTE_RESERVE accounting to post-trim assembly path only; future budget/limit code MUST add boundary fixtures per RULESET.TESTS.boundary-coverage.fixtures` +`RULESET.TESTS.no-timing-assertion=do not assert on wall-clock elapsed time (Date.now() delta, performance.now(), process.hrtime() comparison); such assertions test the host machine not the SUT and flake on loaded CI runners; enforcement: local/no-elapsed-assertion ESLint rule (warn → error after #453); canonical replacement: clock-seam pattern with node:test mock.timers` +`RULESET.TESTS.clock-seam=concurrency logic must accept an optional {clock=Date} parameter; tests control time via t.mock.timers.enable(['Date']) + t.mock.timers.setTime(0) + t.mock.timers.tick(N); real OS scheduler races are not a permitted test pattern after ADR 456 (2026-05-28); real-race tests are deleted once deterministic seam tests cover the same logical path` +`RULESET.TESTS.property-based-testing=modules implementing parsing / transformation / budget-limit / bijective contracts must include at least one fast-check (fc) property test asserting a domain invariant; invariant categories: round-trip, monotonicity, boundary-containment, idempotency; property tests live in *.test.cjs alongside unit tests; CI signal: Stryker mutation score below 80% blocks merge` +`RULESET.TESTS.mutation-score=Stryker runs incremental (--since origin/next) on ubuntu-latest/Node24 CI leg; default threshold 80% killed/total; surviving mutants in scope block merge unless path is listed in stryker.config.mjs with documented reason; treat surviving mutant as a failing test specification` +`RULESET.TESTS.delete-bad-tests=pass-always / vacuous-truth / source-grep / elapsed-time / real-race / permanent-allow-test-rule tests are DELETED and replaced with compliant tests in the same PR; not skipped, not commented out, not permanently exempted; replacement must cover the same logical path via typed-surface assertion or clock-seam pattern` +`RULESET.TESTS.eslint-harness=ADR 452 (2026-05-28): ESLint flat config + typescript-eslint + eslint-plugin-n + eslint-plugin-no-only-tests + local plugin at scripts/eslint-rules/; replaces scripts/lint-*.cjs regex scanners; three test-rigor rules (local/no-source-grep, local/no-magic-sleep-in-tests, local/no-elapsed-assertion) ship at warn, promoted to error after #453 cleanup sweep merges` + `RULESET.WORKFLOW_MARKDOWN.FENCES=preserve opening language fence when editing shell snippets in workflow markdown; malformed fence creates fresh CR threads (MD040)` `RULESET.WORKFLOW_SIZE_BUDGET=workflow-size-budget can fail otherwise-valid review fixes; XL workflows <=1800 lines or trim prose before final checks` `RULESET.WORKFLOW_FILE_NAMES=workflow files use hyphens; XML attributes must match (extract-learnings not extract_learnings); tests should pin exact hyphenated name` diff --git a/TESTING-STANDARDS.md b/TESTING-STANDARDS.md new file mode 100644 index 000000000..8d05fa1e8 --- /dev/null +++ b/TESTING-STANDARDS.md @@ -0,0 +1,187 @@ +# Testing Standards + +This document is the authoritative reference for test correctness contracts, enforcement rules, and the test-rigor policies adopted in ADR 456 (`docs/adr/456-test-rigor-architecture.md`). + +It orients you to the existing docs without duplicating them: + +- **Suite naming, CI matrix, per-suite scripts** → [`docs/TESTING-SUITES.md`](docs/TESTING-SUITES.md) +- **Test runner imports, setup/teardown patterns, fixture formatting, QA matrix** → [`CONTRIBUTING.md` — "Testing Standards"](CONTRIBUTING.md#testing-standards) +- **Concrete demo tests for each requirement** → [`TEST-EXAMPLES.md`](TEST-EXAMPLES.md) +- **Machine-greppable predicates (`RULESET.TESTS.*`)** → [`CONTEXT.md` — "Test rules and lint"](CONTEXT.md) + +--- + +## Six test-rigor contracts + +Every test in this project must satisfy these six contracts. They apply to new tests and to revisions of existing tests. + +### 1. Exercise real code, not source or output text + +Tests call exported functions or run the CLI and parse structured output. They do not `readFileSync` a source file and assert on its text content. They do not assert on raw stdout/stderr strings beyond exit-code confirmation. + +**Compliant:** + +```javascript +const { stdout } = await runGsdTools(['plan', '--json']); +const result = JSON.parse(stdout); +assert.strictEqual(result.phases[0].id, 'plan-1.1'); +``` + +**Non-compliant:** + +```javascript +const src = readFileSync('./bin/lib/plan.cjs', 'utf8'); +assert(src.includes('plan-1.1')); // never do this +``` + +**Enforcement:** `local/no-source-grep` (ESLint, currently `warn`; becomes `error` after issue #453 merges). + +### 2. No vacuous-truth assertions + +Assertions must be capable of failing given a plausible defect in the SUT. An assertion whose left-hand side is always truthy regardless of SUT behavior does not add coverage. + +**Non-compliant:** + +```javascript +assert(true); +assert.ok(output !== undefined); // output is unconditionally set above +``` + +**Compliant:** Assert on a value that the SUT computed and that a mutation of the SUT could change. + +**Enforcement:** Code review + `local/no-source-grep` (catches a common vacuous sub-pattern). No automated rule covers all shapes; code review is the primary gate. + +### 3. No pass-always tests + +A test that passes regardless of whether the feature it describes is implemented is worse than no test: it inflates the count while providing false confidence. + +The test must be capable of failing if the feature is absent or broken. Write the test first (red phase of TDD), confirm it fails with a stub implementation, then implement. + +**Enforcement:** `local/no-source-grep`, code review, and Stryker mutation score (surviving mutants in covered paths signal pass-always tests). + +### 4. Test the claimed path + +The test name describes a behavior. The test body must exercise that behavior through the implementation path, not through a mock that replaces the entire SUT. + +If the test name says "acquireLock expires after TTL," the test must call `acquireLock` (not a hand-rolled stub that does nothing) and assert that a lock acquired at time T is expired at time T + TTL + 1ms. + +**Enforcement:** Code review. Stryker mutation score on uncovered paths. + +### 5. Complete mocks + +When mocking a dependency, mock only the dependency — not the SUT behavior itself. A mock that returns a hardcoded value from inside the function under test is a pass-always test in disguise. + +External I/O (filesystem, network, clock) is the appropriate scope for mocking. Business logic inside the SUT is not mocked; it is exercised. + +**Enforcement:** Code review. + +### 6. Counter-tests for negative space + +For every behavioral contract, at least one test must exercise an input that the SUT should reject or handle differently from the happy path. Examples: missing required argument, value at boundary + 1, hostile input. + +See [`CONTRIBUTING.md` — "QA Matrix Requirements"](CONTRIBUTING.md#qa-matrix-requirements) for the twelve-case matrix. Apply the cases relevant to the changed surface. + +**Enforcement:** Code review, `no-only-tests/no-only-tests` ESLint rule (prevents happy-path-only merges via `test.only`). + +--- + +## New policies (ADR 456) + +### No timing or elapsed-time assertions + +Do not assert on wall-clock elapsed time. Such assertions test the host machine, not the SUT, and fail spuriously on loaded CI runners. + +**Non-compliant:** + +```javascript +const start = Date.now(); +await doWork(); +assert(Date.now() - start < 200, 'must complete in 200ms'); +``` + +**Enforcement:** `local/no-elapsed-assertion` (ESLint, currently `warn`; becomes `error` after issue #453 merges). Also `no-restricted-syntax` ban on `performance.now()` comparisons in assertions. + +### Clock-seam pattern for concurrency + +Concurrency logic must be tested via an injectable clock seam backed by `node:test` `mock.timers`. Real OS scheduler races are non-deterministic on loaded CI runners and are not a permitted test pattern. + +**Compliant pattern:** + +```javascript +// Production code +function acquireLock(resource, { clock = Date } = {}) { + const deadline = clock.now() + LOCK_TTL_MS; + // ... implementation uses clock.now() +} + +// Test +test('lock expires after TTL', (t) => { + t.mock.timers.enable(['Date']); + t.mock.timers.setTime(0); + acquireLock('res'); + t.mock.timers.tick(LOCK_TTL_MS + 1); + assert.strictEqual(isLockExpired('res'), true); +}); +``` + +**Enforcement:** `local/no-magic-sleep-in-tests` (bans `setTimeout`/`sleep`/`delay` inside test bodies; ESLint, currently `warn`; becomes `error` after issue #453 merges). Code review catches the race pattern directly. + +### Property-based testing tier + +Modules that implement parsing, transformation, budget/limit logic, or any bijective contract must include at least one `fast-check` (`fc`) property test asserting a domain invariant. Property tests live in `*.test.cjs` files alongside unit tests; no separate suite tag is required. + +Invariant categories to consider: round-trip, monotonicity, boundary containment, idempotency. + +**Threshold:** No hard per-file threshold is enforced by CI tooling; the gate is Stryker mutation score (see below). Property tests are the mechanism that drives mutation score above the threshold on logic-heavy paths. + +**Enforcement:** Code review verifies that property tests exist for modules in scope. Stryker mutation score below 80 % blocks merge (see next section). + +### 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. + +A surviving mutant is a concrete specification of missing coverage. Treat it as a failing test, not as a metric. + +**Enforcement:** `stryker run --since origin/next` in CI. Threshold configured in `stryker.config.mjs`. + +### Delete-bad-tests policy + +Tests in the following categories are **deleted** and replaced with compliant tests in the same PR. They are not commented out, not skipped, and not annotated with a permanent `// allow-test-rule` exemption: + +| Category | Signal | +|---|---| +| Pass-always | Assertion always evaluates truthy regardless of SUT state | +| Vacuous-truth | LHS is computed from the same expression as the SUT input | +| Source-grep | `readFileSync` on a source file + text assertion | +| Elapsed-time | Assertion on `Date.now()` delta or `performance.now()` comparison | +| Real-race | Test outcome depends on OS scheduler timing | +| Permanent `allow-test-rule` | Exemption with no tracking issue and no deadline | + +"Replaced" means: in the same PR, add a behavioral test that exercises the logical path the deleted test was intended to cover, using the typed-surface mandate (contract 1 above) and, where concurrency is involved, the clock-seam pattern. + +Real multi-process race tests are deleted once the corresponding deterministic clock-seam test covers the same logical path. No permanent quarantine. + +**Enforcement:** ESLint rules catch source-grep, magic-sleep, and elapsed-assertion shapes. Code review is the gate for pass-always and vacuous-truth. The delete-bad-tests sweep (tracked separately) addresses the backlog of pre-ADR 456 tests. + +--- + +## ESLint rule reference + +| Rule | Severity | What it catches | +|---|---|---| +| `local/no-source-grep` | `warn` → `error` (#453) | `readFileSync` on source files + text assertions; `assert.match`/`doesNotMatch` on raw stdout/stderr | +| `local/no-magic-sleep-in-tests` | `warn` → `error` (#453) | `setTimeout`/`sleep`/`delay` calls inside `test()`/`it()`/`describe()` bodies | +| `local/no-elapsed-assertion` | `warn` → `error` (#453) | Assertions on `Date.now()` delta, `process.hrtime()`, `performance.now()` comparisons | +| `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) | + +All three `local/*` rules currently ship at `warn`. They become `error` after the cleanup sweep tracked at [#453](https://github.com/open-gsd/get-shit-done-redux/issues/453) merges. New violations added after the acceptance of ADR 456 are out of policy regardless of the current ESLint severity. + +ESLint harness details: [`docs/adr/452-eslint-lint-harness.md`](docs/adr/452-eslint-lint-harness.md). + +--- + +## Markdownlint compliance + +This file uses fenced code blocks with explicit language tags (`javascript`, `text`) as required by MD040. All tables use consistent column counts (MD056). diff --git a/docs/adr/452-eslint-lint-harness.md b/docs/adr/452-eslint-lint-harness.md new file mode 100644 index 000000000..b4980cecf --- /dev/null +++ b/docs/adr/452-eslint-lint-harness.md @@ -0,0 +1,86 @@ +# ADR 452: Adopt standard ESLint flat-config lint harness + +- **Status:** Accepted +- **Date:** 2026-05-28 + +This codebase adopts ESLint flat config (eslint ≥ 9) with `typescript-eslint`, `eslint-plugin-n`, `eslint-plugin-no-only-tests`, and a local AST-rule plugin as the canonical lint harness, replacing the homegrown regex-based `scripts/lint-*.cjs` scripts. The ESLint harness becomes the single enforcement point for import-graph, Node API, and test-rigor rules. The three test-rigor rules (`local/no-source-grep`, `local/no-magic-sleep-in-tests`, `local/no-elapsed-assertion`) initially ship at `warn`; a follow-up issue (tracked at #453) promotes them to `error` after the cleanup phases merge. + +## Context + +### Existing homegrown regex harness + +`scripts/lint-no-source-grep.cjs`, `scripts/lint-no-magic-sleep.cjs`, and related scripts implement test-rigor guards as regular-expression line scanners over raw source text. This approach has several structural weaknesses: + +- **False positives** — regex on raw text fires on string literals, comments, and doc blocks that are not code paths. +- **No AST context** — the regex scanners cannot distinguish a banned call inside a helper wrapper from a banned call inside a live test assertion. +- **No incremental mode** — the scripts always scan the entire tree; they have no ESLint-style `--cache`, `--fix`, or `--changed` modes. +- **No editor integration** — IDEs speak LSP/ESLint, not project-local shell scripts; contributors see violations only in CI. +- **Maintenance cost** — each new rule requires a new bespoke script with its own exit-code wiring. + +### Type-aware linting stopgap + +`tsconfig.lint.json` was introduced to give `tsc --noEmit` access to the hand-written `.cjs` files alongside the generated ones. It is explicitly described in the codebase as a stopgap pending a proper type-aware ESLint setup. + +### Generated vs hand-written split + +Approximately 59 hand-written and 13 generated `.cjs` files currently coexist in `get-shit-done/bin/lib/`. The hand-written files are not checked by `typescript-eslint` type-aware rules because `tsconfig.lint.json` is not wired into an ESLint project. ADR 457 (`457-generated-cjs-single-source.md`) proposes collapsing this split; the present ADR is a prerequisite: the ESLint harness must exist before the collapse can surface type errors. + +## Decision + +1. **Adopt ESLint flat config** (`eslint.config.mjs` at repo root) with: + - `typescript-eslint` (type-aware rules enabled via `tsconfig.lint.json` project reference) + - `eslint-plugin-n` for Node API and `require()` graph enforcement + - `eslint-plugin-no-only-tests` (`no-only-tests/no-only-tests`) to prevent `test.only` / `describe.only` leaking into CI + - A local plugin at `scripts/eslint-rules/` that exposes the AST-rewrite of the three homegrown test-rigor rules: + - `local/no-source-grep` — bans `readFileSync` on source files + `.includes()/.match()/.startsWith()` on the bound variable; also bans `assert.match/doesNotMatch` on `.stdout/.stderr` without JSON round-trip + - `local/no-magic-sleep-in-tests` — bans `setTimeout`/`sleep`/`delay` calls inside `test()` / `it()` / `describe()` bodies + - `local/no-elapsed-assertion` — bans `assert` on elapsed-time values (e.g., `Date.now() - start > N`, `process.hrtime`, `performance.now()` comparisons in assertions) + +2. **Phase in at `warn`**. All three `local/*` rules ship at severity `warn` from the initial merge. The CI lint gate (`npm run lint`) does not fail on warnings; it does emit them as annotation. A dedicated follow-up (tracked at #453) flips all three to `error` after the cleanup phases that eliminate existing violations have merged. + +3. **Retire homegrown scripts**. `scripts/lint-no-source-grep.cjs` and all sibling regex-scanner scripts are deleted in the same PR that introduces the ESLint config. The CI step that called them is replaced by a single `npm run lint` invocation. + +4. **`no-restricted-syntax` bans** (in `eslint.config.mjs`, severity `error` from day one): + - `CallExpression[callee.name='setTimeout']` inside `Program > ExpressionStatement` (top-level sleeps — catches a different shape than `local/no-magic-sleep-in-tests`) + - `MemberExpression[property.name='only'][object.name=/^(test|it|describe)$/]` as a belt-and-suspenders backstop alongside `eslint-plugin-no-only-tests` + +5. **`eslint-plugin-n`** enforces: + - `n/no-missing-require` — catches import-graph drift for hand-written CJS files + - `n/no-unsupported-features/es-syntax` against `engines.node` (`>=22.0.0`) + +6. **Editor integration**. Commit the recommended `.vscode/extensions.json` entry for `dbaeumer.vscode-eslint` and an `.editorconfig` fallback so contributors see inline violations without running CI. + +## Consequences + +### For contributors + +- ESLint runs in CI (`npm run lint`) alongside the test suite. A clean lint is required before a PR is mergeable. +- The three test-rigor rules fire as warnings initially; they become errors after #453 merges. Violations added after the initial cleanup phase will block CI. +- Existing `// allow-test-rule: ` comments in `.cjs` files translate to ESLint `// eslint-disable-next-line local/no-source-grep -- ` comments. The old exemption syntax is no longer recognized. +- `test.only` / `describe.only` committed to any non-scratch file fail CI immediately (`error` from day one). + +### For the test-rigor audit + +- Violations of `local/no-source-grep`, `local/no-magic-sleep-in-tests`, and `local/no-elapsed-assertion` are now surfaced in the IDE and in CI annotations before a PR is opened, removing the current pattern of discovering violations only in PR review. +- The `no-elapsed-assertion` rule enforces the clock-seam pattern codified in ADR 456 (`456-test-rigor-architecture.md`): tests that assert on elapsed time must use the injectable clock seam instead of wall-clock assertions. + +### For ADR 457 + +- The ESLint harness with `typescript-eslint` type-aware rules is a prerequisite for collapsing the hand-written/generated `.cjs` split. Once `tsconfig.lint.json` is wired into ESLint's project references, `typescript-eslint` will surface type drift between the hand-written CJS surface and the TS source. + +## Rejected Alternatives + +**(a) Keep the homegrown regex harness.** Rejected. False positives, no AST context, no editor integration, and per-rule maintenance cost all compound over time. The homegrown scripts solved the immediate gap but are not a sustainable lint surface. + +**(b) Legacy `.eslintrc` format.** Rejected. ESLint 9 deprecated `.eslintrc`; flat config is the supported path for new plugins and type-aware rules. Starting on a deprecated format incurs migration debt immediately. + +**(c) Per-file `ts-check` only (no ESLint).** Rejected. `@ts-check` in `.cjs` files gives type feedback inside the file but does not enforce import-graph, test-rigor, or no-only-tests rules. It is a supplementary aid, not a lint harness. + +## References + +- Tracking issue: [#452](https://github.com/open-gsd/get-shit-done-redux/issues/452) +- Follow-up (warn → error): [#453](https://github.com/open-gsd/get-shit-done-redux/issues/453) +- Test-rigor architecture: `456-test-rigor-architecture.md` +- Generated CJS collapse (future): `457-generated-cjs-single-source.md` +- Homegrown scripts retired: `scripts/lint-no-source-grep.cjs`, `scripts/lint-no-magic-sleep.cjs` +- Stopgap: `tsconfig.lint.json` diff --git a/docs/adr/456-test-rigor-architecture.md b/docs/adr/456-test-rigor-architecture.md new file mode 100644 index 000000000..86b398a79 --- /dev/null +++ b/docs/adr/456-test-rigor-architecture.md @@ -0,0 +1,152 @@ +# ADR 456: Test-rigor architecture — deterministic scheduling, antagonistic tier, typed-surface mandate, and delete-bad-tests policy + +- **Status:** Accepted +- **Date:** 2026-05-28 + +Four interrelated policies establish the test-rigor architecture for this codebase: (a) concurrency is tested deterministically via an injectable clock seam rather than real OS races, (b) fast-check property tests and Stryker mutation testing form a PR-gating antagonistic tier, (c) tests assert on typed `--json`/IR fields and exported registries, never on rendered text or source literals, and (d) pass-always, vacuous, source-grep, and racing tests are deleted and replaced with real coverage in the same change, never preserved behind permanent lint-rule debt or permanent quarantine. Real multi-process race tests are deleted once deterministic tests cover the logic. + +## Context + +### Observed test-quality failures + +Several failure modes reached production or required significant rework: + +- **Real-time race tests** — tests that rely on actual OS scheduler timing (e.g., two `Promise.race` branches whose winner depends on wall-clock latency) are non-deterministic on loaded CI runners. PR #432 / issue #407 documented a 40 % flake rate for a lock-race test that was fixed in PR #450 by replacing the real-race pattern with an injectable clock seam. +- **Source-grep tests** — tests that `readFileSync` a source file and assert on `.includes('someString')` create false confidence: the source string exists but the behavioral path it represents may be dead. `scripts/lint-no-source-grep.cjs` (now superseded by ESLint rule `local/no-source-grep`, per ADR 452) already bans this pattern, but violations still surface in PRs. +- **Vacuous / pass-always tests** — tests that assert `true` unconditionally, or that assert on a value that is computed from the same expression as the SUT input (circular), inflate test counts without exercising any behavioral path. +- **Elapsed-time assertions** — assertions such as `assert(Date.now() - start > 100)` fail spuriously on slow CI runners and pass spuriously on fast ones. They test the host, not the SUT. +- **Mutation survival** — PR #432 revealed that a mutation to the lock-release path survived the existing test suite for weeks: the test was asserting on a property that the mutant also produced correctly. Mutation testing would have caught this at PR time. + +### Existing enforcement gaps + +- No property-based testing framework is wired into the test suite or CI. +- No mutation testing runs in CI (Stryker was evaluated but not integrated). +- The `local/no-elapsed-assertion` ESLint rule exists (per ADR 452) but ships at `warn` initially; it needs a matching architectural policy so the rule has a canonical replacement pattern. +- The `// allow-test-rule` exemption mechanism was intended as a migration aid for the `no-source-grep` rule but has become a permanent home for tests that should be rewritten. + +## Decision + +### (a) Deterministic-over-racing + +Concurrency logic must be tested via an **injectable clock seam** backed by `node:test` `mock.timers`, not by real OS scheduler races. + +The clock seam pattern: + +```javascript +// Production code — accepts an optional clock parameter +function acquireLock(resource, { clock = Date } = {}) { + const deadline = clock.now() + LOCK_TTL_MS; + // ... implementation using clock.now() for time checks +} + +// Test code — controls time explicitly +test('lock expires after TTL', (t) => { + t.mock.timers.enable(['Date']); + t.mock.timers.setTime(0); + acquireLock('res'); + t.mock.timers.tick(LOCK_TTL_MS + 1); + assert.strictEqual(isLockExpired('res'), true); +}); +``` + +Real multi-process race tests (where two OS processes genuinely compete for a resource) are **deleted** once the corresponding deterministic clock-seam test covers the same logical path. They are not quarantined or skipped — deleted. A deleted racing test must be replaced by a deterministic seam test in the same commit. + +Wall-clock timing in production code paths that cannot accept an injectable clock (e.g., third-party integrations) must be wrapped behind an adapter interface so tests can substitute a controlled clock. + +### (b) Antagonistic tier — property-based and mutation testing + +Two tools form the antagonistic tier: + +**fast-check** — property-based tests use `fast-check` (`fc`) to generate adversarial inputs. Property tests live in `*.test.cjs` files alongside unit tests; they do not require a separate suite tag. A property test must specify at least one invariant that must hold for all generated inputs, not just the set that a developer would think to write. Example invariant categories: + +- Round-trip (serialize → parse → equal original) +- Monotonicity (larger input → larger or equal output) +- Boundary (output ∈ allowed-range for all inputs in domain) +- Idempotency (applying twice = applying once) + +**Stryker mutation testing** — Stryker runs in incremental mode (`--since origin/next`) as a PR-gating signal. The default mutation score threshold is **80 %** (killed / total). Surviving mutants block merge unless the surviving mutant is in a path that is explicitly excluded in `stryker.config.mjs` with a documented reason. Stryker runs on the `ubuntu-latest` / Node 24 CI leg; it does not multiply across the OS/runtime matrix. + +The three custom ESLint rules (`local/no-source-grep`, `local/no-magic-sleep-in-tests`, `local/no-elapsed-assertion`) ship at `warn` initially; a follow-up tracked at #453 flips them to `error` after the cleanup phases merge. This ADR establishes the architectural intent regardless of the current severity level. + +### (c) Typed-surface mandate + +Tests assert on typed `--json` fields, exported registries, and structured IR objects. They do not assert on: + +- Rendered CLI text (stdout/stderr string content beyond exit-code and parse-able JSON) +- Source file literals (content of `.cjs`, `.ts`, or `.md` files read via `readFileSync`) +- Internal implementation details not exposed through a module's public API + +When a CLI command must be tested, the pattern is: + +```javascript +// GOOD — parse JSON, assert on typed fields +const { stdout } = await runGsdTools(['plan', '--json']); +const result = JSON.parse(stdout); +assert(Array.isArray(result.phases), `expected Array, got: ${stdout}`); +assert.strictEqual(result.phases[0].id, 'plan-1.1'); + +// BAD — assert on rendered text +assert(stdout.includes('Phase 1.1'), stdout); +``` + +When a module exports a registry (e.g., a command map, a label enum, a plugin list), tests import and assert on the registry directly, not on the output of a command that serializes it. + +### (d) Delete-bad-tests policy + +Tests that fall into any of the following categories are **deleted** (not commented out, not skipped, not annotated with `// allow-test-rule`) and replaced with real coverage in the same change: + +| Category | Example | +|---|---| +| Pass-always | `assert(true)` or `assert.ok(someVar !== undefined)` where `someVar` is always defined | +| Vacuous-truth | Assertion computed from the same expression as the SUT input | +| Source-grep | `assert(src.includes('someIdentifier'))` where `src` is a `readFileSync` of a source file | +| Elapsed-time | `assert(Date.now() - start > N)` or equivalent | +| Real-race | Two OS processes competing for a resource with a pass/fail outcome determined by scheduler timing | +| Permanent `allow-test-rule` | `// allow-test-rule` exemptions that have been in place for more than one release cycle without a tracked cleanup issue | + +"Replace with real coverage" means: in the same PR or commit, add a behavioral test that exercises the logical path the deleted test was intended to cover, using the typed-surface mandate (c) and, where concurrency is involved, the clock-seam pattern (a). + +Tests must not be moved to a permanent quarantine directory or marked with a skip that has no associated tracking issue and deadline. If a test cannot be fixed now, file a tracking issue and delete the test rather than leaving a permanently-skipped test inflating the file count. + +## Consequences + +### For test authors + +- Any new test asserting on rendered CLI text, source file content, or elapsed time will be caught by ESLint (`local/no-source-grep`, `local/no-elapsed-assertion`) — initially as a warning, later as an error after #453 merges. +- Concurrency tests must use `node:test` `mock.timers` seam from the point this ADR is accepted. Real-race tests written after this date are out of policy from the first commit. +- PRs touching modules with surviving Stryker mutants at or above the 80 % threshold must either add tests that kill the mutants or add the path to `stryker.config.mjs` with a documented reason. +- `// allow-test-rule` exemptions are a temporary migration aid. Any exemption added after this ADR is accepted must include a tracking issue number in the comment (`// allow-test-rule: see #NNN`) and will be reviewed at the issue's cleanup milestone. + +### For CI + +- Stryker runs incrementally (`--since origin/next`) on the `ubuntu-latest` / Node 24 leg. Cold runs (no prior cache) are expected to take 10–20 minutes; the job has a 30-minute timeout. +- `fast-check` failures are reported as standard `node:test` failures; no CI change is needed to surface them. +- The ESLint lint gate (`npm run lint`) runs alongside the test suite. A clean lint is a merge prerequisite once #453 promotes the three rules to `error`. + +### For the delete-bad-tests sweep + +A dedicated cleanup sweep (tracked separately from this ADR) will: + +1. Enumerate all existing `// allow-test-rule` exemptions. +2. For each: either rewrite the test to comply with this ADR or file a tracking issue and delete. +3. Enumerate all existing tests that match the bad-test categories above. +4. Delete and replace each in isolated PRs, one test file per PR. + +The sweep must complete before the three ESLint rules flip to `error` (i.e., before #453 merges). + +## Rejected Alternatives + +**(a) Annotate-and-migrate forever.** Leave existing bad tests in place with `// allow-test-rule` permanently and migrate on an unscheduled timeline. Rejected: the annotation mechanism is already used as permanent debt shelter. Unscheduled migration never happens. The delete policy with replacement creates a hard boundary. + +**(b) Keep real-race tests with retry logic.** Wrap flaky race tests in a `retries: 3` loop. Rejected: retries add wall-clock latency without eliminating the non-determinism. A test that fails 1 in 10 runs will still fail on loaded runners and will not fail locally on idle machines, making the retry indistinguishable from masking. + +**(c) Mutation testing as a nightly-only signal.** Run Stryker on the main branch nightly rather than on PRs. Rejected: nightly signals are not actionable at PR time. By the time a surviving mutant is reported nightly, the PR is merged and the fix requires a new PR, CI run, and review cycle. Incremental `--since origin/next` mode keeps the Stryker scope small enough for PR CI. + +## References + +- Tracking issue: [#456](https://github.com/open-gsd/get-shit-done-redux/issues/456) +- ESLint harness: `452-eslint-lint-harness.md` +- Warn-to-error follow-up: [#453](https://github.com/open-gsd/get-shit-done-redux/issues/453) +- Lock-race determinism fix: PR #450, issue #432 / #407 +- `TESTING-STANDARDS.md` — full rule-to-enforcement table +- `docs/TESTING-SUITES.md` — suite naming and CI matrix diff --git a/docs/adr/457-generated-cjs-single-source.md b/docs/adr/457-generated-cjs-single-source.md new file mode 100644 index 000000000..f3e2593ea --- /dev/null +++ b/docs/adr/457-generated-cjs-single-source.md @@ -0,0 +1,82 @@ +# ADR 457: Collapse hand-written CJS to generated single-source [Proposed] + +- **Status:** Proposed +- **Date:** 2026-05-28 + +> **Not yet executed.** This ADR records the agreed direction and rationale. No code has been changed under this decision. Implementation is tracked separately and requires the ESLint harness (ADR 452) to be in place first. + +This ADR proposes collapsing the ~59 hand-written `get-shit-done/bin/lib/*.cjs` files into TypeScript sources compiled (generated) into `.cjs` output, eliminating the hand-written/generated split, enabling type-aware linting and CJS/TS parity as first-class CI signals, and removing `tsconfig.lint.json` as a stopgap. + +## Context + +### Today's split + +The `get-shit-done/bin/lib/` directory currently holds two kinds of files: + +- **Hand-written `.cjs`** (~59 files) — authored directly as CommonJS. These are the runtime entry points for CLI commands, library seams, and utilities. Type checking relies on `tsconfig.lint.json` and `@ts-check` comments; coverage is uneven. +- **Generated `.cjs`** (~13 files) — compiled from `.ts` sources in `sdk/src/` or `get-shit-done/src/` via `tsc`. These files carry a `// @generated` header; they must never be hand-edited. The CJS/TS parity tests (`tests/cjs-ts-parity.test.cjs`) assert that the generated surface matches the TS declarations. + +This split creates three structural problems: + +1. **Type-aware linting gap.** `tsconfig.lint.json` is not wired into an ESLint project reference. `typescript-eslint` type-aware rules (`@typescript-eslint/no-floating-promises`, `@typescript-eslint/strict-boolean-expressions`, etc.) do not run on hand-written `.cjs` files. ADR 452 introduces the ESLint harness as a prerequisite but cannot close the type-aware gap until the sources are TypeScript. + +2. **CJS/TS parity is partial.** The parity tests only cover the generated surface (~13 files). The hand-written surface (~59 files) has no equivalent parity signal. Regressions on the hand-written surface are caught only by behavioral tests, not by type or surface comparison. + +3. **`tsconfig.lint.json` as a permanent stopgap.** The file was introduced with an explicit `// stopgap` annotation in its header comment. It adds a non-standard compilation path that must be kept in sync with `tsconfig.json` and `tsconfig.build.json`. Every time a new `.cjs` file is added, `tsconfig.lint.json` must be manually updated. + +### Relationship to the retired SDK boundary + +ADR 0174 (`0174-retire-gsd-sdk-package-boundary.md`) retired the `@opengsd/gsd-sdk` package boundary and collapsed the runtime onto a single `src/` TS surface. The generated `.cjs` files are the downstream artifact of that collapse. This ADR extends the same direction to the hand-written files. + +## Decision [Proposed] + +1. **Single TS source.** Each hand-written `get-shit-done/bin/lib/*.cjs` module is rewritten as `get-shit-done/src/.ts` (or the equivalent path inside the unified `src/` tree established by ADR 0174). The TS source is the canonical artifact; `.cjs` output is generated by `tsc` and checked in as part of the build step. + +2. **No hand-written runtime `.cjs`.** After the collapse, the only `.cjs` files in `get-shit-done/bin/lib/` are generated. Hand-authoring a `.cjs` file in that directory is a policy violation caught by a new `scripts/lint-no-hand-written-cjs.cjs` check (or equivalent ESLint rule). + +3. **`tsconfig.lint.json` deleted.** Once all sources are TypeScript, `tsconfig.lint.json` is no longer needed. The ESLint project reference in `eslint.config.mjs` (ADR 452) points directly to `tsconfig.json`. `@ts-check` comments in `.cjs` files are removed as part of the migration. + +4. **CJS/TS parity becomes total.** The parity tests (`tests/cjs-ts-parity.test.cjs`) expand to cover the full `bin/lib/` surface. A generated `.cjs` file that drifts from its TS source fails CI. + +5. **Migration is incremental.** Files are migrated module by module in separate PRs. Each PR: (a) rewrites one hand-written `.cjs` as TS, (b) adds it to the `tsc` build, (c) updates parity tests, (d) removes `tsconfig.lint.json` includes for the migrated file. The final PR removes `tsconfig.lint.json` entirely. + +6. **Prerequisites.** This ADR is not implemented until: + - ADR 452 (ESLint harness) is merged and CI-green. + - A migration tracking issue is opened with the full list of ~59 files and a per-module plan. + - At least one pilot migration (chosen for low coupling) is completed and reviewed. + +## Consequences + +### Positive + +- Type-aware `typescript-eslint` rules apply to the full runtime surface, not just the generated subset. +- CJS/TS parity is a total, automated signal rather than a partial one. +- `tsconfig.lint.json` stopgap is removed; one fewer compilation path to maintain. +- Contributors write TypeScript for all new runtime code; no new hand-authored `.cjs` files. + +### Negative + +- **Migration cost.** ~59 files must be migrated. Each migration may surface latent type errors that require fixes before the module can compile as TypeScript. +- **Build step added.** Currently, hand-written `.cjs` files are runtime-ready without compilation. After migration, every change to a source `.ts` file requires a `tsc` step before the `.cjs` output is updated. CI must gate on build output being up to date. +- **Generated file commits.** If `.cjs` outputs are checked in (as is current practice for the generated subset), contributors must remember to commit both the `.ts` source and the generated `.cjs`. A pre-commit hook or CI check enforces this. + +### For testing + +- Tests that currently import hand-written `.cjs` files directly (`require('../bin/lib/foo.cjs')`) continue to work unchanged; only the source behind the `.cjs` changes. +- No test file changes are required as part of the migration itself, though new type-aware lint rules may surface existing test-code issues. + +## Rejected Alternatives + +**(a) Keep the hand-written/generated split indefinitely.** Rejected. The split creates a permanent type-aware linting gap and requires `tsconfig.lint.json` as an indefinite stopgap. The direction established by ADR 0174 is single-runtime collapse; this ADR extends that to the remaining hand-written files. + +**(b) Full ESM rewrite.** Rewrite all `.cjs` files as `.mjs` ES modules instead of TypeScript-compiled CJS. Rejected. The package currently ships CommonJS to support `require()` consumers. An ESM rewrite would be a breaking change requiring a semver-major bump and coordination with downstream consumers. The TS-compiled CJS approach achieves type safety without the compatibility break. + +**(c) Keep `tsconfig.lint.json` and wire it into ESLint.** Extend the ADR 452 ESLint harness to use `tsconfig.lint.json` as the project reference for hand-written files. Rejected as a permanent solution. `tsconfig.lint.json` is explicitly a stopgap; wiring it into ESLint makes the stopgap permanent. The correct solution is to make the files TypeScript so the standard `tsconfig.json` is sufficient. + +## References + +- Tracking issue: [#457](https://github.com/open-gsd/get-shit-done-redux/issues/457) +- ESLint harness prerequisite: `452-eslint-lint-harness.md` +- Single-runtime collapse: `0174-retire-gsd-sdk-package-boundary.md` +- CJS/TS parity tests: `tests/cjs-ts-parity.test.cjs` +- Stopgap: `tsconfig.lint.json` diff --git a/docs/adr/README.md b/docs/adr/README.md index f15347d0d..26278e119 100644 --- a/docs/adr/README.md +++ b/docs/adr/README.md @@ -49,6 +49,9 @@ See **[CONTRIBUTING.md — "Proposing an ADR or PRD"](../../CONTRIBUTING.md#prop | [3524-cjs-sdk-hard-seam.md](3524-cjs-sdk-hard-seam.md) | CJS↔SDK hard seam — single canonical owner per responsibility (#3524) | Superseded by ADR-0174 | | [3660-runtime-artifact-layout-module.md](3660-runtime-artifact-layout-module.md) | Runtime Artifact Layout Module owns per-runtime artifact placement | Proposed | | [0174-retire-gsd-sdk-package-boundary.md](0174-retire-gsd-sdk-package-boundary.md) | Retire @opengsd/gsd-sdk package boundary — single-runtime collapse | Accepted | +| [452-eslint-lint-harness.md](452-eslint-lint-harness.md) | Adopt standard ESLint flat-config lint harness; retire homegrown regex scanners | Accepted | +| [456-test-rigor-architecture.md](456-test-rigor-architecture.md) | Test-rigor architecture — deterministic scheduling, antagonistic tier, typed-surface mandate, delete-bad-tests policy | Accepted | +| [457-generated-cjs-single-source.md](457-generated-cjs-single-source.md) | Collapse hand-written CJS to generated single-source | Proposed | ## Seam map