From f8ec36f78212ab41e9f01dfe01667c732d1fdf51 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 30 Jul 2026 09:19:41 +0000 Subject: [PATCH 1/2] fix(tooling): docs-drift stops flagging docs for tests-only changes MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `affected-docs.mjs` derived changed-package roots from every file under `packages/`, including tests. A test observes behaviour rather than defining it, so it cannot make an implementation-accuracy doc stale — yet every tests-only PR lit up its packages' whole doc set. Three in a row did it: #4064 and #4078 each flagged 6 `packages/services` docs for a diff containing no production code, and one before them the same way. This is the one place the mapper's deliberate over-inclusion actively hurt. The script's own header says over-inclusion is preferred to misses, and for real code that is right — the periodic full audit is the backstop. But a category that is ALWAYS false is different: a reader who learns the comment is noise stops reading it, and then it fails on the PR where it is correct. Precision matters here for the same reason a flaky test is worse than no test. - Test files are dropped before the package roots are derived: `*.test.*` / `*.spec.*` at any depth (so `.integration.test.ts` and `.conformance.test.ts` are covered) plus anything under `__tests__` / `__mocks__` / `__fixtures__`. - The narrowing is NOT silent. The excluded count rides the summary line and `testFilesSkipped` in `--json` — a tool that quietly narrows its own scope reads as "nothing to see here" when it means "I did not look". - `--self-test` pins the matcher, following `check-error-code-casing` / `check-route-envelope`, and the drift workflow runs it before computing the mapping so a regression fails loudly instead of emptying the comment. Verified against real history rather than by inspection: - `46e86bad` (#4078, tests only) → 0 docs, was 6, reporting `1 test file(s) excluded` - `4965bfac` (#4059, engine.ts + formula) → the same 11 docs as before, `1` skipped - `--all` unchanged at 178 docs The self-test earns its place too: swapping the anchored match for a naive `path.includes('test')` fails 7 cases, among them `control-flow.zod.ts` and `latest.ts` being swallowed as tests — the direction that would turn this optimisation into a miss. Tooling only; empty changeset since nothing releases. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01QoA8AV99Ss1RRkLLAYmDzq --- .changeset/docs-drift-skip-test-files.md | 28 ++++++++ .github/workflows/docs-drift-check.yml | 7 ++ scripts/docs-audit/README.md | 13 ++++ scripts/docs-audit/affected-docs.mjs | 90 ++++++++++++++++++++++-- 4 files changed, 134 insertions(+), 4 deletions(-) create mode 100644 .changeset/docs-drift-skip-test-files.md diff --git a/.changeset/docs-drift-skip-test-files.md b/.changeset/docs-drift-skip-test-files.md new file mode 100644 index 0000000000..dc6d81c987 --- /dev/null +++ b/.changeset/docs-drift-skip-test-files.md @@ -0,0 +1,28 @@ +--- +--- + +Tooling only — no package changes, nothing to release. + +The docs-drift mapper (`scripts/docs-audit/affected-docs.mjs`) no longer treats a +test-file change as a reason to flag docs. A test observes behaviour rather than +defining it, so it cannot make an implementation-accuracy doc stale — yet every +tests-only PR lit up its packages' whole doc set. Three in a row (#4064, #4078, and one +before) each flagged 6 `packages/services` docs for a diff containing no production +code. + +That is the one place the mapper's deliberate over-inclusion actively hurt: a comment a +reader learns to skip stops doing its job on the PR where it is right. + +- Test files are dropped before the changed-package roots are derived: `*.test.*` / + `*.spec.*` at any depth, and anything under `__tests__` / `__mocks__` / `__fixtures__`. +- The narrowing is **not silent** — the excluded count appears in the summary line and + as `testFilesSkipped` in `--json`. +- A `--self-test` flag pins the matcher, following the convention of + `check-error-code-casing` / `check-route-envelope`, and the drift workflow now runs it + before computing the mapping. Verified to catch the dangerous direction: replacing the + anchored match with a naive substring check fails 7 cases, including + `control-flow.zod.ts` and `latest.ts` being swallowed as "tests". + +Verified against real history: the tests-only commit `46e86bad` (#4078) now maps to 0 +docs where it previously reported 6, while the implementation commit `4965bfac` (#4059) +still maps to the same 11 docs it did before. diff --git a/.github/workflows/docs-drift-check.yml b/.github/workflows/docs-drift-check.yml index 03d2cd059e..d992d1e69f 100644 --- a/.github/workflows/docs-drift-check.yml +++ b/.github/workflows/docs-drift-check.yml @@ -34,6 +34,13 @@ jobs: - name: Fetch base branch run: git fetch --no-tags origin "${{ github.base_ref }}" + # The mapper excludes test files (a test cannot make an implementation doc + # stale). Self-test first, so a regression that widened the exclusion into + # dropping real implementation changes fails loudly here instead of turning + # this whole comment quietly empty. + - name: Self-test the change → docs mapper + run: node scripts/docs-audit/affected-docs.mjs --self-test + - name: Compute affected docs id: affected run: | diff --git a/scripts/docs-audit/README.md b/scripts/docs-audit/README.md index 85b3f055ae..d1b4cb8a15 100644 --- a/scripts/docs-audit/README.md +++ b/scripts/docs-audit/README.md @@ -21,6 +21,9 @@ node scripts/docs-audit/affected-docs.mjs --json origin/main # every hand-written doc (full audit scope) node scripts/docs-audit/affected-docs.mjs --all + +# check the test-file matcher (needs no repo state; CI runs this before the mapping) +node scripts/docs-audit/affected-docs.mjs --self-test ``` Heuristic: a doc is *affected* by a changed package `P` if it mentions `P`'s npm @@ -28,6 +31,16 @@ name (`@objectstack/`) or repo path (`packages/`). Over-inclusion is prefe over misses; the periodic **full** audit (part 4) is the backstop for docs that describe a package without naming it. +**One exclusion:** changes to **test files** are dropped before the changed-package +roots are derived. A test observes behaviour rather than defining it, so it cannot make +an implementation-accuracy doc stale — yet counting them made every tests-only PR light +up its packages' whole doc set, a class of finding that is always false. That is the one +place over-inclusion actively hurt: a comment a reader learns to skip stops working on +the PR where it is right. The count of excluded files is reported in the summary and as +`testFilesSkipped` in `--json`, so the narrowing is never silent, and `--self-test` +pins the matcher against paths that must and must not match (`commands/test.ts` is +implementation; `foo.conformance.test.ts` is not). + ## 2. CI gate — `.github/workflows/docs-drift-check.yml` On any PR that touches `packages/**`, runs `affected-docs.mjs` against the base branch diff --git a/scripts/docs-audit/affected-docs.mjs b/scripts/docs-audit/affected-docs.mjs index e39c1cbbb5..9044088a62 100644 --- a/scripts/docs-audit/affected-docs.mjs +++ b/scripts/docs-audit/affected-docs.mjs @@ -7,6 +7,7 @@ // node scripts/docs-audit/affected-docs.mjs [sinceRef] # docs affected by changes since (default origin/main) // node scripts/docs-audit/affected-docs.mjs --all # every hand-written doc (full audit) // node scripts/docs-audit/affected-docs.mjs --json [...] # emit JSON {docs, changedPackages, ...} instead of a path list +// node scripts/docs-audit/affected-docs.mjs --self-test # check the test-file matcher (no repo state needed) // // Scope: hand-written docs only = content/docs/**/*.mdx MINUS content/docs/references/** // (references are generated from packages/spec and handled by a separate regenerate pass). @@ -15,6 +16,14 @@ // npm name (`@objectstack/`) or its repo path (`packages/`). Over-inclusion is // intentionally preferred over misses; the periodic FULL audit is the backstop for // docs that describe a package without naming it. +// +// One exclusion, though: a change to a TEST file cannot make an implementation-accuracy +// doc stale, because tests do not define behaviour — they observe it. Counting them made +// every tests-only PR light up its packages' whole doc set (three in a row on #4064 / +// #4078 / one before), which is a class of finding that is always false. A reader who +// learns the comment is usually noise stops reading it, and then it fails to do its job +// on the PR where it is right. So test files are dropped before deriving the changed +// package roots; everything else stays deliberately over-inclusive. import { execSync } from 'node:child_process'; import { readFileSync, readdirSync, existsSync, statSync } from 'node:fs'; @@ -26,6 +35,12 @@ const asJson = args.includes('--json'); const all = args.includes('--all'); const sinceRef = args.find((a) => !a.startsWith('--')) || 'origin/main'; +// Short-circuit before any git work — the self-test needs no repo state. +if (args.includes('--self-test')) { + selfTest(); + process.exit(0); +} + function sh(cmd) { return execSync(cmd, { cwd: repoRoot, stdio: ['ignore', 'pipe', 'ignore'] }).toString(); } @@ -61,9 +76,63 @@ try { changedFiles = sh(`git diff --name-only ${sinceRef} -- packages/`).split('\n').filter(Boolean); } +/** + * A test file — it observes behaviour rather than defining it, so changing one cannot + * make an implementation-accuracy doc stale. Covers the repo's conventions: `*.test.*` + * / `*.spec.*` at any depth (including `.integration.test.ts` and `.conformance.test.ts`) + * plus anything under a `__tests__` / `__mocks__` / `__fixtures__` directory. + * + * Verify with `--self-test`. + */ +function isTestFile(path) { + return /(^|\/)__(tests|mocks|fixtures)__\//.test(path) + || /(^|\/)[^/]+\.(test|spec)\.[^/]+$/.test(path); +} + +/** + * Check the test-file matcher against known-good and known-bad paths, so the + * exclusion cannot silently widen into dropping real implementation changes — the + * one way this optimisation could turn into a miss. + */ +function selfTest() { + const cases = [ + // [path, isTest, label] + ['packages/services/service-automation/src/builtin/config-schemas.test.ts', true, 'plain .test.ts'], + ['packages/rest/src/package-envelope.conformance.test.ts', true, 'compound .conformance.test.ts'], + ['packages/services/service-automation/src/runas-grant-resolution.integration.test.ts', true, '.integration.test.ts'], + ['packages/spec/src/data/object.spec.ts', true, '.spec.ts'], + ['packages/foo/src/__tests__/helper.ts', true, 'helper inside __tests__'], + ['packages/foo/src/__mocks__/driver.ts', true, '__mocks__'], + ['packages/foo/src/__fixtures__/stack.json', true, '__fixtures__'], + + ['packages/services/service-automation/src/engine.ts', false, 'implementation'], + ['packages/spec/src/automation/control-flow.zod.ts', false, 'a zod schema'], + ['packages/formula/src/validate.ts', false, 'implementation with a test-ish name'], + ['packages/cli/src/commands/test.ts', false, 'a command NAMED test is not a test file'], + ['packages/qa/src/testing.ts', false, 'testing.ts is implementation'], + ['packages/spec/src/latest.ts', false, 'no false positive on a bare name'], + ['packages/foo/src/tests-helper.ts', false, 'tests-helper is not __tests__'], + ]; + let failed = 0; + for (const [path, want, label] of cases) { + const got = isTestFile(path); + if (got !== want) { + console.error(` ✗ self-test "${label}": ${path} → expected isTestFile=${want}, got ${got}`); + failed++; + } + } + if (failed) { + console.error(`\n✗ affected-docs self-test failed (${failed} case(s)).`); + process.exit(1); + } + console.log(`✓ affected-docs self-test: ${cases.length} cases pass.`); +} + + // collect package roots: packages/ and packages/plugins/ const pkgRoots = new Set(); -for (const f of changedFiles) { +const implementationChanges = changedFiles.filter((f) => !isTestFile(f)); +for (const f of implementationChanges) { let m = f.match(/^(packages\/plugins\/[^/]+)\//) || f.match(/^(packages\/[^/]+)\//); if (m) pkgRoots.add(m[1]); } @@ -91,11 +160,24 @@ for (const doc of handwritten) { if (hits.length) affected.push({ doc, via: [...new Set(hits)] }); } -emit(affected.map((a) => a.doc), changedPackages, `${affected.length} docs affected by ${changedPackages.length} changed package(s) since ${sinceRef}`, affected); +// Report what was excluded rather than dropping it silently — a tool that quietly +// narrows its own scope reads as "nothing to see here" when it means "I did not look". +const testFilesSkipped = changedFiles.length - implementationChanges.length; +const skipNote = testFilesSkipped > 0 + ? ` (${testFilesSkipped} test file(s) excluded — tests cannot make an implementation doc stale)` + : ''; + +emit( + affected.map((a) => a.doc), + changedPackages, + `${affected.length} docs affected by ${changedPackages.length} changed package(s) since ${sinceRef}${skipNote}`, + affected, + testFilesSkipped, +); -function emit(docList, changedPackages, summary, detail) { +function emit(docList, changedPackages, summary, detail, testFilesSkipped = 0) { if (asJson) { - process.stdout.write(JSON.stringify({ summary, sinceRef: all ? null : sinceRef, changedPackages, docs: docList, detail: detail || null }, null, 2) + '\n'); + process.stdout.write(JSON.stringify({ summary, sinceRef: all ? null : sinceRef, changedPackages, docs: docList, detail: detail || null, testFilesSkipped }, null, 2) + '\n'); } else { process.stderr.write(`# ${summary}\n`); process.stdout.write(docList.join('\n') + (docList.length ? '\n' : '')); From 01d5dfd42d025150f3c7bda3edc6f1b2ef9c85d7 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 30 Jul 2026 09:38:20 +0000 Subject: [PATCH 2/2] fix(tooling): run the docs-drift workflow when the mapper itself changes MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit CI on the first push of this PR showed the gap: `Flag docs affected by code changes` never ran, because the workflow triggers on `paths: ['packages/**']` and this PR touches only `scripts/` and `.github/`. So the self-test step added one commit ago was added but never exercised, and editing `affected-docs.mjs` was the one change its own guard could structurally never see — the tool is unguarded exactly when it is being modified. Adds the mapper's own paths to the trigger. A PR touching only these gets the benign "No hand-written docs reference the 0 changed package(s). ✅" branch the comment step already has, which is the correct answer, and the self-test now runs on the change most likely to break it. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01QoA8AV99Ss1RRkLLAYmDzq --- .github/workflows/docs-drift-check.yml | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/.github/workflows/docs-drift-check.yml b/.github/workflows/docs-drift-check.yml index d992d1e69f..5ae0278bdd 100644 --- a/.github/workflows/docs-drift-check.yml +++ b/.github/workflows/docs-drift-check.yml @@ -11,6 +11,13 @@ on: types: [opened, synchronize, reopened] paths: - 'packages/**' + # The mapper and this workflow, so a change to the guard runs the guard. Without + # these, editing `affected-docs.mjs` was the one change its own self-test could + # never see — a `packages/**`-only trigger means the tool is unguarded exactly + # when it is being modified. A PR that touches only these gets the benign + # "0 changed package(s) ✅" comment, which is the correct answer. + - 'scripts/docs-audit/**' + - '.github/workflows/docs-drift-check.yml' permissions: contents: read