diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index 6098fad60..46b58614b 100644 --- a/.github/workflows/test.yml +++ b/.github/workflows/test.yml @@ -16,6 +16,21 @@ concurrency: cancel-in-progress: true jobs: + # Static lint: no source-grep tests in the test suite. + # Runs once (not per matrix node version) since it is a file-content check. + lint-tests: + runs-on: ubuntu-latest + timeout-minutes: 2 + steps: + - uses: actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd # v6.0.2 + - name: Set up Node.js + uses: actions/setup-node@53b83947a5a98c8d113130e565377fae1a50d02f # v6.3.0 + with: + node-version: 24 + - name: Lint — no source-grep tests + shell: bash + run: node scripts/lint-no-source-grep.cjs + test: runs-on: ${{ matrix.os }} timeout-minutes: 10 diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 681804b89..0defdbba0 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -229,6 +229,73 @@ const content = ` `; ``` +### Prohibited: Source-Grep Tests + +**Never read source-code `.cjs` files with `readFileSync` to assert that strings exist within them.** This is source-grep theater: it proves a literal is present in a file, not that the feature works at runtime. + +```javascript +// BAD — source-grep theater +const configSrc = fs.readFileSync( + path.join(GSD_ROOT, 'bin', 'lib', 'config-schema.cjs'), 'utf-8' +); +assert.ok( + configSrc.includes("'workflow.plan_bounce'"), + 'VALID_CONFIG_KEYS should contain workflow.plan_bounce' +); +``` + +This test passes even if `workflow.plan_bounce` is present but misspelled in the schema, removed from the validation path, or moved to a different file under a different name. It survives every behavioral regression and fails only on trivial renames. + +The correct pattern for config key tests — use the CLI: + +```javascript +// GOOD — behavioral test via the CLI +test('config-set accepts workflow.plan_bounce', (t) => { + const tmpDir = createTempProject(); + t.after(() => cleanup(tmpDir)); + + const result = runGsdTools('config-set workflow.plan_bounce true', tmpDir); + assert.ok(result.success, `config-set should accept workflow.plan_bounce: ${result.error}`); + + const configPath = path.join(tmpDir, '.planning', 'config.json'); + const config = JSON.parse(fs.readFileSync(configPath, 'utf-8')); + assert.strictEqual(config.workflow?.plan_bounce, true, 'value must be persisted'); +}); +``` + +This single test covers key registration in `VALID_CONFIG_KEYS`, the key's namespace resolution in `KNOWN_TOP_LEVEL`, and value persistence — all behaviors that the source-grep test could not touch. + +**Why this pattern broke at scale:** Commit `990c3e64` in this repo updated 5 source-grep tests in one pass when `VALID_CONFIG_KEYS` moved between files. Zero of those tests were testing behavior. If they had been behavioral tests, the migration would have been invisible. + +**CI enforcement:** A linter (`scripts/lint-no-source-grep.cjs`, run as `npm run lint:tests`) detects violations. Any test file that calls `readFileSync` on a `.cjs` path in a source directory without the exemption annotation below will fail the `lint-tests` CI job. + +### Exception: `allow-test-rule: ` + +Some tests legitimately read source files. There are six recognized categories: + +| Reason | When to use | +|--------|-------------| +| `source-text-is-the-product` | Agent `.md`, workflow `.md`, command `.md` files — their text IS what the runtime loads. Testing text content tests the deployed contract. | +| `architectural-invariant` | Implementation must use a specific primitive (e.g., `Atomics.wait`, atomic file writes) that cannot be tested by observing outputs. | +| `structural-regression-guard` | A specific code pattern must (or must not) exist to prevent a class of bug (e.g., regex global-state misuse). Behavioral tests cannot distinguish which pattern was used. | +| `docs-parity` | A reference doc must stay in sync with source-defined constants (e.g., `CONFIG_DEFAULTS`). The source is the canonical list; there is no runtime API to enumerate it. | +| `integration-test-input` | A source file is used as a real fixture input to a transformation function under test — the file is not inspected for strings but passed as data. | +| `structural-implementation-guard` | A feature's interception or wiring point is not reachable end-to-end via `runGsdTools`. Used temporarily until a behavioral path exists. | + +Annotate with a standalone `//` comment before the file's opening block comment: + +```javascript +// allow-test-rule: architectural-invariant +// state.cjs locking must use Atomics.wait(), not a spin-loop. Behavioral tests +// cannot observe which sleep primitive was chosen — only source inspection can. + +/** + * Regression tests for locking bugs #1909... + */ +``` + +The annotation **must** be a standalone `// allow-test-rule:` line, not inside a `/** */` block comment — the CI linter scans for the pattern `// allow-test-rule:`. + ### Node.js Version Compatibility **Node 22 is the minimum supported version.** Node 24 is the primary CI target. All tests must pass on both. @@ -278,6 +345,16 @@ node --test tests/core.test.cjs npm run test:coverage ``` +### CI Test Quality Checks + +The following checks run on every PR in addition to the test suite: + +| Job | What it checks | How to pass | +|-----|----------------|-------------| +| `lint-tests` | No source-grep tests (see above) | Replace with `runGsdTools()` behavioral tests, or add `// allow-test-rule: ` | + +Run locally before pushing: `npm run lint:tests` + ### Test Requirements by Contribution Type The required tests differ depending on what you are contributing: diff --git a/package.json b/package.json index 1e295fdda..b69c68a00 100644 --- a/package.json +++ b/package.json @@ -58,6 +58,7 @@ "prepublishOnly": "npm run build:hooks && npm run build:sdk", "pretest": "npm run build:sdk", "pretest:coverage": "npm run build:sdk", + "lint:tests": "node scripts/lint-no-source-grep.cjs", "test": "node scripts/run-tests.cjs", "test:coverage": "c8 --check-coverage --lines 70 --reporter text --include 'get-shit-done/bin/lib/*.cjs' --exclude 'tests/**' --all node scripts/run-tests.cjs" } diff --git a/scripts/lint-no-source-grep.cjs b/scripts/lint-no-source-grep.cjs new file mode 100644 index 000000000..968030b04 --- /dev/null +++ b/scripts/lint-no-source-grep.cjs @@ -0,0 +1,112 @@ +#!/usr/bin/env node +/** + * lint-no-source-grep.cjs + * + * Enforces the "no source-grep tests" rule: + * Tests must NOT read source-code .cjs files with readFileSync to assert string + * presence. That pattern (source-grep theater) proves a literal exists in source, + * not that the runtime behavior is correct. + * + * ALLOWED: + * - require('../get-shit-done/bin/lib/foo.cjs') -- runs the module, not text inspection + * - readFileSync on .md / .json / .txt files -- product-content or config output + * - Files annotated: // allow-test-rule: + * + * DISALLOWED (without allow-test-rule): + * - readFileSync where the path argument ends in a .cjs filename literal + * - A path constant (e.g. CONFIG_PATH) assigned to a .cjs lib file, used in readFileSync + * + * Exit 0 = clean. Exit 1 = violations found (with diagnostics). + */ + +'use strict'; + +const fs = require('fs'); +const path = require('path'); + +const TESTS_DIR = path.join(__dirname, '..', 'tests'); +const ALLOW_ANNOTATION = /\/\/\s*allow-test-rule:\s*\S/; + +// Matches constant definitions that hold a .cjs path in a SOURCE directory. +// Requires a source-dir indicator ('bin', 'lib', 'get-shit-done') to avoid +// flagging temp files like path.join(tmpDir, 'example.cjs'). +// const CONFIG_PATH = path.join(__dirname, '..', 'get-shit-done', 'bin', 'lib', 'config-schema.cjs'); +const CJS_PATH_CONST_RE = /(?:const|let|var)\s+(\w+)\s*=\s*path\.join\s*\([^)]*(?:'bin'|"bin"|'lib'|"lib"|'get-shit-done'|"get-shit-done")[^)]*['"][^'"]*\.cjs['"]/gm; + +// Matches readFileSync with a named variable as first arg +const READ_WITH_CONST_RE = /readFileSync\s*\(\s*([A-Za-z_][A-Za-z0-9_]*)\s*,/gm; + +// Matches readFileSync with an inline path.join(.cjs) as first arg +const READ_WITH_INLINE_CJS_RE = /readFileSync\s*\([^,)]*path\.join\s*\([^)]*(?:'bin'|"bin"|'lib'|"lib"|'get-shit-done'|"get-shit-done")[^)]*['"][^'"]*\.cjs['"]/; + +function setFromMatches(content, re) { + const found = new Set(); + let m; + const cloned = new RegExp(re.source, re.flags); + while ((m = cloned.exec(content)) !== null) found.add(m[1]); + return found; +} + +function check(filepath) { + const content = fs.readFileSync(filepath, 'utf-8'); + const rel = path.relative(path.join(__dirname, '..'), filepath); + + if (ALLOW_ANNOTATION.test(content)) return null; + + // Pattern A: readFileSync(path.join(..., 'foo.cjs'), ...) + if (READ_WITH_INLINE_CJS_RE.test(content)) { + return { + file: rel, + reason: 'readFileSync with inline .cjs path literal', + fix: 'Replace with runGsdTools() behavioral test, or add // allow-test-rule: ', + }; + } + + // Pattern B: const FOO_PATH = path.join(..., 'foo.cjs') + readFileSync(FOO_PATH, ...) + const cjsConsts = setFromMatches(content, CJS_PATH_CONST_RE); + if (cjsConsts.size > 0) { + const readConsts = setFromMatches(content, READ_WITH_CONST_RE); + const overlap = [...cjsConsts].filter(c => readConsts.has(c)); + if (overlap.length > 0) { + return { + file: rel, + reason: `source .cjs path constant(s) used in readFileSync: ${overlap.join(', ')}`, + fix: 'Replace with runGsdTools() behavioral test, or add // allow-test-rule: ', + }; + } + } + + return null; +} + +function findTestFiles(dir) { + const results = []; + for (const entry of fs.readdirSync(dir, { withFileTypes: true })) { + const full = path.join(dir, entry.name); + if (entry.isDirectory()) { + results.push(...findTestFiles(full)); + } else if (entry.name.endsWith('.test.cjs')) { + results.push(full); + } + } + return results; +} + +const testFiles = findTestFiles(TESTS_DIR); + +const violations = testFiles.map(check).filter(Boolean); + +if (violations.length === 0) { + console.log(`ok lint-no-source-grep: ${testFiles.length} test files checked, 0 violations`); + process.exit(0); +} + +process.stderr.write(`\nERROR lint-no-source-grep: ${violations.length} violation(s) found\n\n`); +for (const v of violations) { + process.stderr.write(` ${v.file}\n`); + process.stderr.write(` Problem : ${v.reason}\n`); + process.stderr.write(` Fix : ${v.fix}\n\n`); +} +process.stderr.write('See CONTRIBUTING.md "Prohibited: Source-Grep Tests" for guidance.\n'); +process.stderr.write('Structural tests that legitimately read source files: add // allow-test-rule: \n\n'); +process.exit(1); diff --git a/tests/bug-1891-file-resolution.test.cjs b/tests/bug-1891-file-resolution.test.cjs index c781916be..f85699697 100644 --- a/tests/bug-1891-file-resolution.test.cjs +++ b/tests/bug-1891-file-resolution.test.cjs @@ -1,3 +1,9 @@ +// allow-test-rule: structural-implementation-guard +// gsd-tools.cjs @file: resolution is a low-level stdout interception that cannot be +// exercised end-to-end via runGsdTools without a real workflow that emits @file: output. +// These structural tests guard the interception wiring until a behavioral integration +// test suite for the full @file: path is added. + /** * Regression tests for bug #1891 * diff --git a/tests/config-field-docs.test.cjs b/tests/config-field-docs.test.cjs index e327d24fa..a9c31d515 100644 --- a/tests/config-field-docs.test.cjs +++ b/tests/config-field-docs.test.cjs @@ -1,3 +1,8 @@ +// allow-test-rule: docs-parity +// Extracts CONFIG_DEFAULTS keys from core.cjs source to verify planning-config.md +// stays in sync. The canonical list of defaults lives in source; there is no runtime +// API to enumerate them. Source inspection is the only practical parity check here. + /** * Verify planning-config.md documents all config fields from source code. */ diff --git a/tests/copilot-install.test.cjs b/tests/copilot-install.test.cjs index bd19261aa..d563cb5d5 100644 --- a/tests/copilot-install.test.cjs +++ b/tests/copilot-install.test.cjs @@ -1,3 +1,9 @@ +// allow-test-rule: integration-test-input +// Reads verify.cjs as real test fixture input to the convertClaudeToCopilotContent() +// function under test. The file is not inspected for string presence; it is the +// input whose *transformation* is being asserted. This is the correct level of testing +// for format-conversion functions where a real source file is the canonical test case. + /** * GSD Tools Tests - Copilot Install Plumbing * diff --git a/tests/discuss-mode.test.cjs b/tests/discuss-mode.test.cjs index ed74dceed..8f91af26c 100644 --- a/tests/discuss-mode.test.cjs +++ b/tests/discuss-mode.test.cjs @@ -1,3 +1,9 @@ +// allow-test-rule: structural-implementation-guard +// init.cjs cmdInitPlanPhase must expose text_mode in its returned flags object. +// The behavioral alternative (run plan-phase init and inspect JSON output) is +// fragile across runtime variations. Structural inspection guards the contract +// until a stable behavioral API test is in place. + /** * Discuss Mode Config Tests * diff --git a/tests/locking-bugs-1909-1916-1925-1927.test.cjs b/tests/locking-bugs-1909-1916-1925-1927.test.cjs index df4b7afe7..b9d8d1ece 100644 --- a/tests/locking-bugs-1909-1916-1925-1927.test.cjs +++ b/tests/locking-bugs-1909-1916-1925-1927.test.cjs @@ -1,3 +1,8 @@ +// allow-test-rule: architectural-invariant +// state.cjs locking must use Atomics.wait() (not a spin-loop) and register an exit +// handler. These are implementation primitives, not string literals — behavioral tests +// cannot verify which sleep primitive was chosen. Source inspection is the right level. + /** * Regression tests for locking bugs #1909, #1916, #1925, #1927. * diff --git a/tests/milestone-regex-global.test.cjs b/tests/milestone-regex-global.test.cjs index 01524c4f4..99f68334f 100644 --- a/tests/milestone-regex-global.test.cjs +++ b/tests/milestone-regex-global.test.cjs @@ -1,3 +1,9 @@ +// allow-test-rule: structural-regression-guard +// milestone.cjs must use replace()+compare, not test()+replace(), to avoid regex +// lastIndex corruption with global flags. A behavioral test cannot distinguish which +// pattern was used — it can only observe wrong output after multiple calls, which is +// fragile. Structural inspection locks the correct fix in place. + /** * Regression tests for regex global state bug in milestone.cjs * diff --git a/tests/orphan-worktree-detection.test.cjs b/tests/orphan-worktree-detection.test.cjs index 3b20073db..ef3c9b1c6 100644 --- a/tests/orphan-worktree-detection.test.cjs +++ b/tests/orphan-worktree-detection.test.cjs @@ -1,3 +1,8 @@ +// allow-test-rule: architectural-invariant +// verify.cjs must contain the W017 warning code and the worktree list invocation. +// These checks guard the existence of the detection feature, not its text output. +// Behavioral tests cover the detection flow; structural tests guard the implementation contract. + /** * GSD Tools Tests - Orphan/Stale Worktree Detection (W017) * diff --git a/tests/workspace.test.cjs b/tests/workspace.test.cjs index 67f4a551e..c44520322 100644 --- a/tests/workspace.test.cjs +++ b/tests/workspace.test.cjs @@ -11,7 +11,7 @@ const fs = require('fs'); const path = require('path'); const os = require('os'); const { execSync } = require('child_process'); -const { runGsdTools, createTempDir, cleanup } = require('./helpers.cjs'); +const { runGsdTools, createTempProject, createTempDir, cleanup } = require('./helpers.cjs'); const { detectChildRepos } = require('../get-shit-done/bin/lib/init.cjs'); // ─── detectChildRepos ──────────────────────────────────────────────────────── @@ -368,30 +368,37 @@ describe('workspace command files', () => { // ─── Routing in gsd-tools ─────────────────────────────────────────────────── describe('workspace routing in gsd-tools', () => { + let tmpDir; + + beforeEach(() => { tmpDir = createTempProject(); }); + afterEach(() => { cleanup(tmpDir); }); + + // Behavioral routing tests: verify each command is recognized by the router + // (does not return "Unknown init workflow: ..."). The exact command output is + // covered by the functional tests above; these guard against routing deletions. + test('init new-workspace is routed correctly', () => { - const toolsContent = fs.readFileSync( - path.join(__dirname, '..', 'get-shit-done', 'bin', 'gsd-tools.cjs'), - 'utf8' + const result = runGsdTools('init new-workspace test-ws', tmpDir); + const stderr = result.error || ''; + assert.ok( + !stderr.includes('Unknown init workflow'), + `init new-workspace must be a recognized command; got: ${stderr}` ); - assert.ok(toolsContent.includes("case 'new-workspace'")); - assert.ok(toolsContent.includes('cmdInitNewWorkspace')); }); test('init list-workspaces is routed correctly', () => { - const toolsContent = fs.readFileSync( - path.join(__dirname, '..', 'get-shit-done', 'bin', 'gsd-tools.cjs'), - 'utf8' - ); - assert.ok(toolsContent.includes("case 'list-workspaces'")); - assert.ok(toolsContent.includes('cmdInitListWorkspaces')); + const result = runGsdTools('init list-workspaces', tmpDir); + assert.ok(result.success, `init list-workspaces should succeed: ${result.error}`); + const parsed = JSON.parse(result.output); + assert.ok(Array.isArray(parsed.workspaces), 'list-workspaces must return a workspaces array'); }); test('init remove-workspace is routed correctly', () => { - const toolsContent = fs.readFileSync( - path.join(__dirname, '..', 'get-shit-done', 'bin', 'gsd-tools.cjs'), - 'utf8' + const result = runGsdTools('init remove-workspace nonexistent-ws', tmpDir); + const stderr = result.error || ''; + assert.ok( + !stderr.includes('Unknown init workflow'), + `init remove-workspace must be a recognized command; got: ${stderr}` ); - assert.ok(toolsContent.includes("case 'remove-workspace'")); - assert.ok(toolsContent.includes('cmdInitRemoveWorkspace')); }); });