diff --git a/.changeset/docs-drift-nested-package-roots.md b/.changeset/docs-drift-nested-package-roots.md new file mode 100644 index 0000000000..f5093863c4 --- /dev/null +++ b/.changeset/docs-drift-nested-package-roots.md @@ -0,0 +1,38 @@ +--- +--- + +Tooling only — no package changes, nothing to release. + +The docs-drift mapper (`scripts/docs-audit/affected-docs.mjs`) derives a changed +file's package root from the filesystem — the deepest ancestor directory with a +`package.json` — instead of a regex that special-cased only `packages/plugins/*`. +Under the old derivation the 30 packages nested under the other six container +directories (`services/`, `connectors/`, `apps/`, `qa/`, `triggers/`, `adapters/`) +collapsed into their container, whose missing `package.json` left the npm name +unresolved and the npm-name matching arm dead: any doc that names +`@objectstack/service-automation` but never the repo path was a guaranteed miss +(#4162), the one direction this tool promises to avoid. No hardcoded container +list replaces the special case — that would fail the same way again on container +number eight (#3786's pattern). + +- Verified against real history: on #4161's commit the drift comment attribution + changes from the directory name `packages/services` to + `@objectstack/service-automation`. A synthetic service-automation-only change + goes from 6 docs — all belonging to *other* services, matched via the coarse + `packages/services` path token, with `automation/flows.mdx` absent — to the 4 + right ones, `flows.mdx` first among them. +- Second arm (from #4162's comment thread): `/scripts/**` is + build/verification tooling and no longer counts as an implementation change + (#4183 flagged 106 docs for a diff whose only code change was a new check + script). Kept narrow: `package.json` and `src/scripts/**` stay counted. + Publication check done — no package ships runtime code from `scripts/`; three + plugins publish a lone `i18n-extract.config.ts` only for lack of a `files` + field. A scripts-only change now maps to 0 docs (was 106); the exclusion is + reported in the summary and as `scriptFilesSkipped` in `--json`, never silent. +- `--self-test` now pins the package-root derivation and the tooling-script + classifier too (32 cases, hermetic via an injected fake tree), closing the + guard-of-the-guard hole: the original self-test pinned only the test-file + matcher, so this bug was invisible to it. Includes the two invariants from the + issue: a container directory must never come out as a package root, and + `packages/x/package.json` is implementation while `packages/x/scripts/y.ts` + is not. diff --git a/.github/workflows/docs-drift-check.yml b/.github/workflows/docs-drift-check.yml index 5ae0278bdd..51a885e8f9 100644 --- a/.github/workflows/docs-drift-check.yml +++ b/.github/workflows/docs-drift-check.yml @@ -41,10 +41,11 @@ 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. + # The mapper excludes test files and package tooling scripts (neither can make + # an implementation doc stale) and derives package roots from the filesystem. + # Self-test first, so a regression that widened an exclusion into dropping real + # implementation changes — or collapsed nested packages into their container + # again (#4162) — fails loudly here instead of quietly skewing this comment. - name: Self-test the change → docs mapper run: node scripts/docs-audit/affected-docs.mjs --self-test diff --git a/scripts/docs-audit/README.md b/scripts/docs-audit/README.md index ecb3fdd060..b4236e11d1 100644 --- a/scripts/docs-audit/README.md +++ b/scripts/docs-audit/README.md @@ -22,24 +22,46 @@ 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) +# pin the change classifiers + package-root derivation (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 -name (`@objectstack/`) or repo path (`packages/`). Over-inclusion is preferred -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). +name (`@objectstack/`) or repo path (`P`'s directory, e.g. +`packages/services/service-automation`). Over-inclusion is preferred over misses; the +periodic **full** audit (part 4) is the backstop for docs that describe a package +without naming it. + +**How a changed file maps to its package:** the package root is the **deepest ancestor +directory with a `package.json`**, resolved from the filesystem — never a hand-kept +list of container directories. (The mapper once special-cased only +`packages/plugins/*`; the 30 packages nested under the other six containers collapsed +into `packages/services` et al., whose missing `package.json` disabled the npm-name +matching arm entirely, so a doc naming `@objectstack/service-automation` but not the +repo path was a guaranteed miss — #4162.) A deleted package falls back to the coarse +`packages/` token, which still substring-matches any doc naming the deleted path. + +**Two exclusions:** change classes that cannot make an implementation-accuracy doc +stale are dropped before the changed-package roots are derived: + +1. **Test files** (`*.test.*` / `*.spec.*` at any depth, plus `__tests__` / + `__mocks__` / `__fixtures__`): a test observes behaviour rather than defining it — + 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. +2. **Package tooling scripts** (`/scripts/**`): build/verification + tooling, not the runtime behaviour docs describe (#4183 flagged 106 docs for a diff + whose only code change was a new check script). Narrow on purpose: `src/scripts/**` + is runtime code and stays counted, and so does `package.json` — exports/deps + changes ARE implementation. No package publishes runtime code from `scripts/` + (checked against every `files` allowlist; three plugins ship a lone + `i18n-extract.config.ts` only for lack of a `files` field). + +The excluded counts are reported in the summary line and as `testFilesSkipped` / +`scriptFilesSkipped` in `--json`, so the narrowing is never silent. `--self-test` pins +the classifiers *and* the package-root derivation against paths that must and must not +match (`commands/test.ts` is implementation; `foo.conformance.test.ts` is not; a +container directory must never come out as a package root). **And one deliberate non-exclusion:** `packages/*/CHANGELOG.md` stays counted, even though release notes define behaviour no more than a test does. Extending the exclusion there diff --git a/scripts/docs-audit/affected-docs.mjs b/scripts/docs-audit/affected-docs.mjs index 9044088a62..442fd9c79c 100644 --- a/scripts/docs-audit/affected-docs.mjs +++ b/scripts/docs-audit/affected-docs.mjs @@ -7,23 +7,29 @@ // 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) +// node scripts/docs-audit/affected-docs.mjs --self-test # pin the change classifiers + package-root derivation (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). // // Heuristic: a doc is "affected" by a changed package P if the doc text mentions P's -// 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. +// npm name (`@objectstack/`) or its repo path (the package's directory, e.g. +// `packages/services/service-automation`). 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. +// Two exclusions, though — change classes that cannot make an implementation-accuracy +// doc stale, dropped before the changed package roots are derived (everything else +// stays deliberately over-inclusive): +// - TEST files: 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), 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. +// - TOOLING scripts (`/scripts/**`): build/verification tooling, not +// the runtime behaviour docs describe (#4183 flagged 106 docs for a diff whose +// only code change was a new check script). `package.json` itself stays counted — +// exports/deps changes ARE implementation. import { execSync } from 'node:child_process'; import { readFileSync, readdirSync, existsSync, statSync } from 'node:fs'; @@ -90,12 +96,78 @@ function isTestFile(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. + * The package root that owns a changed path: the DEEPEST ancestor directory with a + * package.json. Derived from the filesystem, not from a hand-kept list of container + * directories — the old regex special-cased `packages/plugins/*` only, so the other + * six containers (services/, connectors/, apps/, qa/, triggers/, adapters/) collapsed + * to the container dir, which has no package.json, so `name` stayed null and the + * npm-name matching arm never fired for those 30 nested packages (#4162 — a doc that + * says `@objectstack/service-automation` but never the path was a guaranteed miss). + * A hardcoded list would fail the same way again on container dir number eight + * (#3786's pattern), so: filesystem. + * + * A path with no package.json anywhere up it (a deleted package) falls back to the + * top-level `packages/` segment: the npm name is unresolvable either way, and the + * coarse path token still substring-matches every doc that mentions the deleted + * package's path. + * + * `hasPackageJson` is injectable so `--self-test` can pin this with no repo state. + */ +function packageRootOf(file, hasPackageJson = dirHasPackageJson) { + const segs = file.split('/'); + if (segs[0] !== 'packages' || segs.length < 3) return null; // not a file inside a package + for (let depth = segs.length - 1; depth >= 2; depth--) { + const dir = segs.slice(0, depth).join('/'); + if (hasPackageJson(dir)) return dir; + } + return segs.slice(0, 2).join('/'); +} + +function dirHasPackageJson(dir) { + return existsSync(join(repoRoot, dir, 'package.json')); +} + +/** + * A tooling script — `/scripts/**` holds build/verification tooling + * (generators, check scripts, i18n-extract configs), not the runtime behaviour docs + * describe, so changing one cannot make an implementation-accuracy doc stale. Same + * reasoning as the test-file exclusion, same measured symptom (#4183: a new check + * script plus its package.json registration, zero `src/` changes, 106 docs flagged). + * Deliberately narrow: + * - only `scripts/` directly under the package root — `src/scripts/**` is runtime + * code and stays counted; + * - `package.json` is NOT excluded: exports/main/deps changes are implementation; + * - generator output that docs do consume lands in `content/docs/references/**`, + * which this tool already scopes out. + * Publication check (the one way this could hide runtime code): no package's `files` + * allowlist ships `scripts/`; three plugins ship it only incidentally (no `files` + * field at all) and it holds a lone `i18n-extract.config.ts` — tooling, not runtime. + */ +function isToolingScript(file, hasPackageJson = dirHasPackageJson) { + const root = packageRootOf(file, hasPackageJson); + return root !== null && file.startsWith(`${root}/scripts/`); +} + +/** + * Pin the change classifiers and the package-root derivation against known-good and + * known-bad paths. The two ways this tool turns into a miss: an exclusion silently + * widens into dropping real implementation changes, or the root derivation collapses + * a nested package into its container again (#4162 — the guard's own guard had a + * hole: the original self-test pinned only `isTestFile`). Needs no repo state: the + * filesystem lookup is injected as a fake tree. */ function selfTest() { - const cases = [ + let failed = 0; + let total = 0; + const check = (fn, label, path, want, got) => { + total++; + if (got !== want) { + console.error(` ✗ self-test "${label}": ${path} → expected ${fn}=${JSON.stringify(want)}, got ${JSON.stringify(got)}`); + failed++; + } + }; + + const testFileCases = [ // [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'], @@ -113,28 +185,76 @@ function selfTest() { ['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++; - } + for (const [path, want, label] of testFileCases) check('isTestFile', label, path, want, isTestFile(path)); + + // Package-root derivation, against a fake tree so the self-test stays hermetic. + // One package.json per shape the repo has: a direct child package plus a nested + // package per container class. The containers themselves have NO package.json. + const fakeTree = new Set([ + 'packages/spec', + 'packages/services/service-automation', + 'packages/plugins/plugin-audit', + 'packages/connectors/connector-slack', + ]); + const inFakeTree = (dir) => fakeTree.has(dir); + const liveRootCases = [ + // [path, expected package root, label] + ['packages/spec/src/latest.ts', 'packages/spec', 'direct child package'], + ['packages/spec/package.json', 'packages/spec', 'the package.json itself'], + ['packages/services/service-automation/src/engine.ts', 'packages/services/service-automation', 'nested under services/ — NOT the container'], + ['packages/plugins/plugin-audit/src/index.ts', 'packages/plugins/plugin-audit', 'nested under plugins/ (formerly the only special case)'], + ['packages/connectors/connector-slack/src/client.ts', 'packages/connectors/connector-slack', 'nested under connectors/'], + ]; + for (const [path, want, label] of liveRootCases) check('packageRootOf', label, path, want, packageRootOf(path, inFakeTree)); + + // The invariant behind the whole fix: a container directory (a parent of nested + // packages, itself without a package.json) must never come out as a package root + // for a file that lives inside one of its packages. + const containers = new Set( + [...fakeTree].map((d) => d.split('/').slice(0, -1).join('/')).filter((d) => d !== 'packages'), + ); + for (const [path] of liveRootCases) { + check('containerIsRoot', 'a container dir is never a live package\'s root', path, false, containers.has(packageRootOf(path, inFakeTree))); } + + // Deliberate fallbacks, pinned so they don't silently change: a path whose + // package.json is gone (deleted package) degrades to the coarse top-level token — + // over-inclusive substring matching still catches docs naming the deleted path. + const fallbackRootCases = [ + ['packages/gone/src/x.ts', 'packages/gone', 'deleted top-level package falls back to packages/'], + ['packages/services/service-gone/src/x.ts', 'packages/services', 'deleted nested package falls back to the coarse container token'], + ['packages/README.md', null, 'a file directly under packages/ belongs to no package'], + ]; + for (const [path, want, label] of fallbackRootCases) check('packageRootOf', label, path, want, packageRootOf(path, inFakeTree)); + + // Tooling-script exclusion: `/scripts/**` is build tooling; the + // package.json next to it and anything under `src/` are implementation. + const scriptCases = [ + // [path, isToolingScript, label] + ['packages/spec/scripts/check-generated.ts', true, 'package check script (#4183)'], + ['packages/plugins/plugin-audit/scripts/i18n-extract.config.ts', true, 'nested package tooling config'], + ['packages/spec/package.json', false, 'package.json IS implementation (exports/deps)'], + ['packages/spec/src/scripts/runner.ts', false, 'src/scripts/** is runtime code'], + ['packages/services/service-automation/src/engine.ts', false, 'implementation'], + ]; + for (const [path, want, label] of scriptCases) check('isToolingScript', label, path, want, isToolingScript(path, inFakeTree)); + 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.`); + console.log(`✓ affected-docs self-test: ${total} cases pass.`); } -// collect package roots: packages/ and packages/plugins/ +// collect package roots from the implementation changes +const testFilesSkipped = changedFiles.filter((f) => isTestFile(f)).length; +const scriptFilesSkipped = changedFiles.filter((f) => !isTestFile(f) && isToolingScript(f)).length; +const implementationChanges = changedFiles.filter((f) => !isTestFile(f) && !isToolingScript(f)); const pkgRoots = new Set(); -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]); + const root = packageRootOf(f); + if (root) pkgRoots.add(root); } // resolve each root to its npm name + keep the path token @@ -162,10 +282,10 @@ for (const doc of handwritten) { // 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)` - : ''; +const skipNotes = []; +if (testFilesSkipped > 0) skipNotes.push(`${testFilesSkipped} test file(s) excluded — tests cannot make an implementation doc stale`); +if (scriptFilesSkipped > 0) skipNotes.push(`${scriptFilesSkipped} tooling script(s) excluded — a package's scripts/ dir is build tooling, not documented behaviour`); +const skipNote = skipNotes.length ? ` (${skipNotes.join('; ')})` : ''; emit( affected.map((a) => a.doc), @@ -173,11 +293,12 @@ emit( `${affected.length} docs affected by ${changedPackages.length} changed package(s) since ${sinceRef}${skipNote}`, affected, testFilesSkipped, + scriptFilesSkipped, ); -function emit(docList, changedPackages, summary, detail, testFilesSkipped = 0) { +function emit(docList, changedPackages, summary, detail, testFilesSkipped = 0, scriptFilesSkipped = 0) { if (asJson) { - process.stdout.write(JSON.stringify({ summary, sinceRef: all ? null : sinceRef, changedPackages, docs: docList, detail: detail || null, testFilesSkipped }, null, 2) + '\n'); + process.stdout.write(JSON.stringify({ summary, sinceRef: all ? null : sinceRef, changedPackages, docs: docList, detail: detail || null, testFilesSkipped, scriptFilesSkipped }, null, 2) + '\n'); } else { process.stderr.write(`# ${summary}\n`); process.stdout.write(docList.join('\n') + (docList.length ? '\n' : ''));