diff --git a/.github/workflows/docs-drift-check.yml b/.github/workflows/docs-drift-check.yml index 51a885e8f9..548454352a 100644 --- a/.github/workflows/docs-drift-check.yml +++ b/.github/workflows/docs-drift-check.yml @@ -65,24 +65,52 @@ jobs: const docs = data.docs || []; const pkgs = (data.changedPackages || []).map(p => p.name || p.dir); const marker = ''; + // The release-owned rows are PARTITIONED OUT of the editable list, never + // dropped (#6893, following the #4920 ruling). They keep getting audited ā€” + // `docs` above is still the full set the audit workflow is scoped to ā€” but + // listing them beside editable pages steers a reader who treats this comment + // as a worklist into the one edit AGENTS.md forbids outright. So they get + // their own section, carrying the instruction that makes them safe. + const readOnly = data.releaseOwnedDocs || []; + const editable = docs.filter(d => !readOnly.includes(d)); let body; if (docs.length === 0) { body = `${marker}\n### šŸ““ Docs Drift Check\nNo hand-written docs reference the ${pkgs.length} changed package(s). āœ…`; } else { const detail = (data.detail || []).reduce((m, d) => (m[d.doc] = d.via, m), {}); - const list = docs.map(d => `- \`${d}\`${detail[d] ? ` _(via ${detail[d].join(', ')})_` : ''}`).join('\n'); + const row = d => `- \`${d}\`${detail[d] ? ` _(via ${detail[d].join(', ')})_` : ''}`; body = [ marker, '### šŸ““ Docs Drift Check', `This PR changes **${pkgs.length}** package(s): ${pkgs.map(p => `\`${p}\``).join(', ')}.`, - '', - `**${docs.length}** hand-written doc(s) reference the affected code and may need an implementation-accuracy re-verification:`, - '', - list, + ]; + if (editable.length) { + body.push( + '', + `**${editable.length}** hand-written doc(s) reference the affected code and may need an implementation-accuracy re-verification:`, + '', + editable.map(row).join('\n'), + ); + } + if (readOnly.length) { + body.push( + '', + `ā›” **${readOnly.length}** release-owned page(s) ${editable.length ? 'also ' : ''}reference the affected code. These are **read-only**:`, + '', + readOnly.map(row).join('\n'), + '', + '> `content/docs/releases/` is RELEASE-OWNED (AGENTS.md "Documentation Guardrails"): release', + '> notes are written centrally at release time, and a code PR that edits them is the exact PR', + '> that guardrail exists to stop. They are still audited ā€” read-only. If one of them is actually', + '> wrong, **file an issue** or open a dedicated docs-only PR; do not edit it here.', + ); + } + body.push( '', '> Advisory only. To re-verify, run the `docs-accuracy-audit` workflow scoped to these files:', '> `node scripts/docs-audit/affected-docs.mjs origin/' + baseRef + '` ā†’ pass the list as `args.docs`.', - ].join('\n'); + ); + body = body.join('\n'); } const { data: comments } = await github.rest.issues.listComments({ owner: context.repo.owner, repo: context.repo.repo, issue_number: context.issue.number, diff --git a/scripts/docs-audit/README.md b/scripts/docs-audit/README.md index 2d8ebb5f39..b9cdc02edb 100644 --- a/scripts/docs-audit/README.md +++ b/scripts/docs-audit/README.md @@ -41,7 +41,7 @@ matching arm entirely, so a doc naming `@objectstack/service-automation` but not 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 +**Three 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__` / @@ -52,16 +52,46 @@ stale are dropped before the changed-package roots are derived: 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/` + is runtime code and stays counted. 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). +3. **Dev-only manifest edits** (#6893): a `/package.json` whose changed + **top-level keys** are all in `{scripts, devDependencies}`. This is the only + **field-level** exclusion ā€” `package.json` as a file stays counted, because + `exports` / `main` / `dependencies` / `files` / `version` changes ARE implementation. + + It is the residue of exclusion 2: #4183 dropped the check *script* but kept the + `package.json` line registering it, so the same PR still lit up the same doc set + through the manifest. Measured over 400 merged commits, five had a `package.json` as + their only `packages/**` implementation change, and **all five** touched nothing but + those two keys ā€” 152 doc-rows in total, none of which could be stale: + + | commit | keys changed | docs flagged | + |:--|:--|--:| + | `df0605ba5` | `scripts` | 12 | + | `2672f855f` | `scripts` | **113** ā€” #6893's headline number | + | `a64315556` | `devDependencies` | 10 | + | `77d9001c7` | `devDependencies` | 13 | + | `466bd9285` | `devDependencies` | 4 | + + The last three are `test(...)` commits: exactly the class exclusion 1 exists to kill, + leaking through the manifest instead. The allowlist is an allowlist on purpose ā€” an + unknown or newly-invented key falls on the **counted** side ā€” and unparseable, added + or deleted manifests are counted too. + + **Why it cannot narrow the net:** the classifier is per *file*. A PR that also touches + that package's `src/**` derives the package root from those files anyway, so this arm + only ever decides the case where the manifest is the package's sole change. Verified + both directions on the real diffs (#6893): adding an `exports` entry to + `packages/spec/package.json` still flags 113 docs, and a `scripts` entry *alongside* a + `src/` edit also still flags 113 ā€” with the manifest itself reported as skipped. 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). +`scriptFilesSkipped` / `devOnlyManifestsSkipped` 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; `dependencies` is never dev-only). **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 @@ -76,7 +106,9 @@ looks like the obvious next step and is a provable no-op, for two independent re 2. Even if it did run, `changeset version` writes `package.json` next to every `CHANGELOG.md` it appends to ā€” 45 of the former against 46 of the latter on the first page of #3910's diff ā€” so dropping the CHANGELOGs would leave the derived package-root - set bit-identical. + set bit-identical. Exclusion 3 does **not** undercut this: what `changeset version` + rewrites is `version` (and workspace `dependencies` ranges), neither of which is in + the dev-only allowlist, so those manifests stay counted. A hand-edited CHANGELOG outside a release is also close to nonexistent in practice. Left counted, and recorded here so the idea is not rediscovered as a gap. @@ -168,6 +200,27 @@ and posts/updates a single advisory PR comment listing the docs that reference t changed code. **Never fails the build** ā€” it only flags drift at the source, before it lands on `main`. Reviewers (or an on-demand audit run) decide whether to re-verify. +### The comment forks release-owned pages into a read-only section (#6893) + +Same ruling as [1b](#release-owned-pages-are-in-scope-and-read-only-4920), one level +down. The comment used to list `content/docs/releases/v17.mdx` in the same bulleted list +as editable pages ā€” so a reader treating the advisory as a worklist was being pointed at +the one edit AGENTS.md forbids outright. The specimen that made it concrete: PR #6921 +changed two diagnostic strings in `packages/lint` and got back three rows, one of them +that release page. + +They are **not filtered out**. `docs` in `--json` stays the full set (it is what scopes +the audit, and #4920 rejected excluding these pages for good reasons); `releaseOwnedDocs` +is a **partition** of it ā€” `releaseOwnedDocs āŠ† docs`, always ā€” and the comment renders it +under its own ā›” heading telling the reader to file an issue instead of editing. + +`affected-docs.mjs` therefore holds a third literal copy of `RELEASE_OWNED_PREFIX`, +alongside AGENTS.md's guardrail row and the audit workflow's own const. Copies, because +the workflow is evaluated in a sandbox VM that cannot import and a shared module would +leave *it* the only unanchored one. `check-audit-scope.mjs` iterates +`RELEASE_OWNED_CONSUMERS` and fails if any copy stops matching the guardrail row ā€” **add +a consumer, add it to that list.** + ## 3. `docs-accuracy-audit` workflow ā€” the LLM audit A Claude Code multi-agent workflow (`.claude/workflows/docs-accuracy-audit.js`). For each diff --git a/scripts/docs-audit/affected-docs.mjs b/scripts/docs-audit/affected-docs.mjs index abdff4f6e9..0623e74cfd 100644 --- a/scripts/docs-audit/affected-docs.mjs +++ b/scripts/docs-audit/affected-docs.mjs @@ -18,7 +18,7 @@ // over misses; the periodic FULL audit is the backstop for docs that describe a // package without naming it. // -// Two exclusions, though ā€” change classes that cannot make an implementation-accuracy +// Three 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 @@ -28,8 +28,35 @@ // 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. +// only code change was a new check script). +// - DEV-ONLY manifest edits (#6893): a `/package.json` whose changed +// TOP-LEVEL KEYS are all dev-time (`scripts`, `devDependencies`). The package.json +// as a whole stays counted ā€” `exports`/`main`/`dependencies` changes ARE +// implementation ā€” so this is a field-level, not a file-level, exclusion. +// +// This is the residue of the #4183 fix: that one excluded the check *script* but +// kept the `package.json` line that registers it, so the same PR still lit up the +// same doc set through the manifest. Measured over 400 merged commits, FIVE had a +// package.json as their only `packages/**` implementation change, and all five +// touched nothing but those two keys ā€” together flagging 152 doc-rows, none of +// which could be stale: +// df0605ba5 `scripts` ā†’ 12 docs (@objectstack/rest turbo wiring) +// 2672f855f `scripts` ā†’ 113 docs (@objectstack/spec build line; #6893's headline) +// a64315556 `devDependencies` ā†’ 10 docs (a test migrated to sqlite) +// 77d9001c7 `devDependencies` ā†’ 13 docs (a test's engine shape) +// 466bd9285 `devDependencies` ā†’ 4 docs (test-only, two packages) +// The last three are test-only commits, i.e. exactly the class the TEST exclusion +// above exists to kill ā€” leaking through the manifest instead. +// +// Why this cannot narrow the net: the classifier is per FILE. If a PR also touches +// that package's `src/**`, those files add the package root on their own, so this +// branch only ever decides the case where the manifest is the package's ONLY change. +// Unparseable, added or deleted manifests fall back to "counted". +// +// And one FORK, which is not an exclusion (#6893, following the #4920 ruling): +// `content/docs/releases/**` pages stay in `docs` ā€” they are audited, read-only ā€” but +// are reported separately so a reader is told to file an issue rather than edit them. +// See the RELEASE_OWNED_PREFIX block below. import { execSync } from 'node:child_process'; import { readFileSync, readdirSync, existsSync, statSync } from 'node:fs'; @@ -41,7 +68,51 @@ 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. +// --- 0. classifier constants ------------------------------------------------- +// Declared up here, ahead of the `--self-test` short-circuit below, because `const` is +// not hoisted: the self-test exercises the classifiers that read these, so leaving them +// down beside their functions makes `--self-test` die in the temporal dead zone. The +// functions themselves can stay where they read best ā€” those ARE hoisted. + +/** + * Release-owned pages ā€” AGENTS.md's Documentation Guardrails forbid a code PR from + * editing anything under this prefix, and `.claude/workflows/docs-accuracy-audit.js` + * routes the same prefix down a read-only channel (#4920). + * + * This is the THIRD literal copy of that path (AGENTS.md's guardrail row, the + * workflow's own const, and this one). Three copies is deliberate, not sloppiness: + * the workflow is evaluated in a sandbox VM and cannot import, so a shared module + * would leave its copy the only unanchored one ā€” the worst of both. Instead + * `scripts/docs-audit/check-audit-scope.mjs` anchors ALL of them to the guardrail row + * and goes red the moment one drifts, which is the same discipline #4851 billed us for. + * + * NOTE this is a REPORTING fork, never an exclusion. Release pages stay in `docs`, so + * the audit scoping command still returns them and they keep getting audited. #4920 + * considered excluding them and REJECTED it: the most-read pages in the docs would go + * permanently unaudited and silently, and a second definition of "docs this tooling + * covers" would grow next to the generated block. What forks is the DELIVERABLE ā€” the + * drift comment tells the reader to file an issue instead of editing (#6893: a comment + * listing `content/docs/releases/v17.mdx` next to editable pages steers a dev who + * treats the list as a worklist straight into the one edit the repo forbids). + */ +const RELEASE_OWNED_PREFIX = 'content/docs/releases/'; +const isReleaseOwned = (doc) => doc.startsWith(RELEASE_OWNED_PREFIX); + +/** + * The `package.json` top-level keys that are DEV-TIME ONLY ā€” a change confined to them + * cannot alter the runtime behaviour a doc describes, so it cannot make one stale. + * + * Deliberately tiny, and deliberately an allowlist rather than a denylist: an unknown + * or newly-invented key must fall on the "counted" side. `dependencies`, + * `peerDependencies`, `exports`, `main`, `module`, `types`, `bin`, `files`, `engines` + * and everything else are all implementation or publication surface and stay counted. + * + * Both entries have measured pull (#6893, 400 commits): `scripts` twice, + * `devDependencies` three times ā€” see the header table. + */ +const DEV_ONLY_PACKAGE_JSON_KEYS = new Set(['scripts', 'devDependencies']); + +// Short-circuit before any git or filesystem work ā€” the self-test needs no repo state. if (args.includes('--self-test')) { selfTest(); process.exit(0); @@ -74,14 +145,26 @@ if (all) { // --- 2. changed package roots since ----------------------------- let changedFiles = []; +let threeDot = true; try { // three-dot: changes on HEAD since the merge-base with sinceRef changedFiles = sh(`git diff --name-only ${sinceRef}...HEAD -- packages/`).split('\n').filter(Boolean); } catch { // fall back to two-dot (e.g. detached/ranges that lack a merge-base) + threeDot = false; changedFiles = sh(`git diff --name-only ${sinceRef} -- packages/`).split('\n').filter(Boolean); } +// The ref the diff is actually measured FROM ā€” needed to read a file's "before" side +// when a change class is decided by content rather than by path (the dev-only manifest +// rule below). Kept in lockstep with the diff above: three-dot measures from the +// merge-base, two-dot from sinceRef itself. A failing merge-base does NOT re-run the +// diff ā€” that would silently change which files are considered. +let baseRef = sinceRef; +if (threeDot) { + try { baseRef = sh(`git merge-base ${sinceRef} HEAD`).trim() || sinceRef; } catch { /* keep sinceRef */ } +} + /** * 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.*` @@ -148,6 +231,75 @@ function isToolingScript(file, hasPackageJson = dirHasPackageJson) { return root !== null && file.startsWith(`${root}/scripts/`); } +/** + * The top-level keys whose values differ between two package.json texts, or `null` if + * either side cannot be parsed as a JSON object. + * + * `null` (not `[]`) for unparseable input, because the two mean opposite things to the + * caller: "nothing changed" is safe to exclude, "I could not tell" must be counted. + */ +function changedManifestKeys(beforeText, afterText) { + let before; + let after; + try { + before = JSON.parse(beforeText); + after = JSON.parse(afterText); + } catch { + return null; + } + const isObj = (v) => v !== null && typeof v === 'object' && !Array.isArray(v); + if (!isObj(before) || !isObj(after)) return null; + const keys = new Set([...Object.keys(before), ...Object.keys(after)]); + return [...keys].filter((k) => JSON.stringify(before[k]) !== JSON.stringify(after[k])).sort(); +} + +/** + * Is this manifest diff confined to dev-time keys? + * + * An EMPTY changed-key set counts as dev-only: the file was touched but nothing + * semantically changed (reformatting, key reordering), which by definition cannot make + * a doc stale. An unparseable side is NOT dev-only ā€” see `changedManifestKeys`. + */ +function isDevOnlyManifestDiff(beforeText, afterText) { + const changed = changedManifestKeys(beforeText, afterText); + if (changed === null) return false; + return changed.every((k) => DEV_ONLY_PACKAGE_JSON_KEYS.has(k)); +} + +/** + * A dev-only manifest edit: `/package.json` whose changed top-level keys + * are all in `DEV_ONLY_PACKAGE_JSON_KEYS` (#6893). + * + * Only the package root's OWN manifest qualifies ā€” a `package.json` sitting anywhere + * else under the package (a fixture, a nested asset) is not the package manifest and + * stays counted. Added or deleted manifests stay counted too: a package appearing or + * disappearing is as implementation as a change gets. + * + * `io` is injectable so `--self-test` can pin this with no repo state; live, it reads + * the two sides out of git. + */ +function isDevOnlyManifestChange(file, io = liveManifestIo, hasPackageJson = dirHasPackageJson) { + const root = packageRootOf(file, hasPackageJson); + if (root === null || file !== `${root}/package.json`) return false; + const beforeText = io.base(file); + const afterText = io.head(file); + if (beforeText === null || afterText === null) return false; + return isDevOnlyManifestDiff(beforeText, afterText); +} + +const liveManifestIo = { + base: (file) => { + try { return sh(`git show ${baseRef}:${file}`); } catch { return null; } + }, + // HEAD, not the working tree: the diff above is measured against HEAD, so comparing + // against a dirty worktree could exclude a file on the strength of an edit that is + // not in the diff at all. Falls back to the worktree only if HEAD has no such path. + head: (file) => { + try { return sh(`git show HEAD:${file}`); } catch { /* fall through */ } + try { return readFileSync(join(repoRoot, file), 'utf8'); } catch { return null; } + }, +}; + /** * 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 @@ -239,6 +391,65 @@ function selfTest() { ]; for (const [path, want, label] of scriptCases) check('isToolingScript', label, path, want, isToolingScript(path, inFakeTree)); + // Dev-only manifest exclusion (#6893). Field-level, so the cases are (before, after) + // pairs rather than paths: the whole point is that the same FILE is excluded or + // counted depending on which top-level keys moved. + const pkg = (extra) => JSON.stringify({ name: '@objectstack/spec', version: '1.0.0', ...extra }); + const manifestCases = [ + // [beforeText, afterText, isDevOnly, label] + [pkg({ scripts: { build: 'tsup' } }), pkg({ scripts: { build: 'tsup && node ../../scripts/check-dev-prereqs.mjs --stamp' } }), true, 'scripts only (#6892, the 113-doc specimen)'], + [pkg({ devDependencies: { vitest: '^1' } }), pkg({ devDependencies: { vitest: '^1', 'better-sqlite3': '^11' } }), true, 'devDependencies only (a64315556)'], + [pkg({ scripts: { a: '1' }, devDependencies: { x: '1' } }), pkg({ scripts: { a: '2' }, devDependencies: { x: '2' } }), true, 'both dev-time keys at once'], + [pkg({ scripts: { build: 'tsup' } }), pkg({ scripts: { build: 'tsup' } }), true, 'nothing changed at all (reformat) is not drift'], + + [pkg({ dependencies: { zod: '^3' } }), pkg({ dependencies: { zod: '^4' } }), false, 'dependencies IS implementation'], + [pkg({ peerDependencies: { react: '^18' } }), pkg({ peerDependencies: { react: '^19' } }), false, 'peerDependencies IS implementation'], + [pkg({ exports: { '.': './dist/index.js' } }), pkg({ exports: { '.': './dist/index.js', './x': './dist/x.js' } }), false, 'a new export IS implementation'], + [pkg({ files: ['dist'] }), pkg({ files: ['dist', 'spec-changes.json'] }), false, 'the published file list IS implementation'], + [pkg({ scripts: { a: '1' }, dependencies: { zod: '^3' } }), pkg({ scripts: { a: '2' }, dependencies: { zod: '^4' } }), false, 'ONE non-dev key among dev ones still counts'], + [pkg({}), JSON.stringify({ name: '@objectstack/spec', version: '2.0.0' }), false, 'a version bump IS implementation'], + [pkg({ engines: { node: '>=20' } }), pkg({ engines: { node: '>=22' } }), false, 'engines is documented deployment surface'], + // The fail-open half: "I could not tell" must never read as "nothing changed". + ['{ not json', pkg({}), false, 'unparseable BEFORE falls back to counted'], + [pkg({}), '{ not json', false, 'unparseable AFTER falls back to counted'], + ['[]', pkg({}), false, 'a non-object manifest falls back to counted'], + ]; + for (const [before, after, want, label] of manifestCases) { + check('isDevOnlyManifestDiff', label, label, want, isDevOnlyManifestDiff(before, after)); + } + + // The path gate around it: only a package ROOT's own manifest is eligible, and an + // added/deleted one is always counted. `io` returns a scripts-only diff for every + // path, so any `false` here is the path gate talking, not the field comparison. + const scriptsOnlyIo = { + base: () => pkg({ scripts: { build: 'tsup' } }), + head: () => pkg({ scripts: { build: 'rollup' } }), + }; + const manifestPathCases = [ + // [path, io, isDevOnly, label] + ['packages/spec/package.json', scriptsOnlyIo, true, 'the package root manifest'], + ['packages/services/service-automation/package.json', scriptsOnlyIo, true, 'a nested package root manifest'], + ['packages/spec/src/__fixtures__/app/package.json', scriptsOnlyIo, false, 'a fixture package.json is not the manifest'], + ['packages/spec/src/index.ts', scriptsOnlyIo, false, 'not a package.json at all'], + ['packages/spec/package.json', { ...scriptsOnlyIo, base: () => null }, false, 'ADDED manifest (no before) is counted ā€” a new package'], + ['packages/spec/package.json', { ...scriptsOnlyIo, head: () => null }, false, 'DELETED manifest (no after) is counted ā€” a removed package'], + ]; + for (const [path, io, want, label] of manifestPathCases) { + check('isDevOnlyManifestChange', label, path, want, isDevOnlyManifestChange(path, io, inFakeTree)); + } + + // The release-owned FORK (#6893/#4920). This is the one classifier whose `true` must + // NOT remove the doc from the output ā€” it re-routes the reporting only. Pinning the + // predicate here keeps the prefix honest; `check-audit-scope.mjs` is what anchors it + // to AGENTS.md's guardrail row and to the audit workflow's copy. + const releaseOwnedCases = [ + ['content/docs/releases/v17.mdx', true, 'the specimen row from #6893'], + ['content/docs/releases/index.mdx', true, 'the releases index'], + ['content/docs/permissions/authorization.mdx', false, 'an editable hand-written doc'], + ['content/docs/deployment/releases/v9.mdx', false, 'a page that merely has "releases" deeper in its path'], + ]; + for (const [doc, want, label] of releaseOwnedCases) check('isReleaseOwned', label, doc, want, isReleaseOwned(doc)); + if (failed) { console.error(`\nāœ— affected-docs self-test failed (${failed} case(s)).`); process.exit(1); @@ -247,10 +458,18 @@ function selfTest() { } -// 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)); +// collect package roots from the implementation changes. One pass, because the +// dev-only-manifest arm shells out to git and must not be asked the same question twice. +let testFilesSkipped = 0; +let scriptFilesSkipped = 0; +let devOnlyManifestsSkipped = 0; +const implementationChanges = []; +for (const f of changedFiles) { + if (isTestFile(f)) { testFilesSkipped++; continue; } + if (isToolingScript(f)) { scriptFilesSkipped++; continue; } + if (isDevOnlyManifestChange(f)) { devOnlyManifestsSkipped++; continue; } + implementationChanges.push(f); +} const pkgRoots = new Set(); for (const f of implementationChanges) { const root = packageRootOf(f); @@ -277,7 +496,7 @@ for (const doc of handwritten) { if (name && text.includes(name)) hits.push(name); else if (text.includes(dir)) hits.push(dir); } - if (hits.length) affected.push({ doc, via: [...new Set(hits)] }); + if (hits.length) affected.push({ doc, via: [...new Set(hits)], releaseOwned: isReleaseOwned(doc) }); } // Report what was excluded rather than dropping it silently ā€” a tool that quietly @@ -285,6 +504,7 @@ for (const doc of handwritten) { 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`); +if (devOnlyManifestsSkipped > 0) skipNotes.push(`${devOnlyManifestsSkipped} package.json edit(s) excluded ā€” only dev-time keys (${[...DEV_ONLY_PACKAGE_JSON_KEYS].join('/')}) changed`); const skipNote = skipNotes.length ? ` (${skipNotes.join('; ')})` : ''; emit( @@ -292,13 +512,34 @@ emit( changedPackages, `${affected.length} docs affected by ${changedPackages.length} changed package(s) since ${sinceRef}${skipNote}`, affected, - testFilesSkipped, - scriptFilesSkipped, + { testFilesSkipped, scriptFilesSkipped, devOnlyManifestsSkipped }, ); -function emit(docList, changedPackages, summary, detail, testFilesSkipped = 0, scriptFilesSkipped = 0) { +function emit(docList, changedPackages, summary, detail, skipped = {}) { + const { testFilesSkipped = 0, scriptFilesSkipped = 0, devOnlyManifestsSkipped = 0 } = skipped; if (asJson) { - process.stdout.write(JSON.stringify({ summary, sinceRef: all ? null : sinceRef, changedPackages, docs: docList, detail: detail || null, testFilesSkipped, scriptFilesSkipped }, null, 2) + '\n'); + process.stdout.write( + JSON.stringify( + { + summary, + sinceRef: all ? null : sinceRef, + changedPackages, + // The FULL set, release-owned pages included ā€” this is what feeds the audit + // workflow's `args.docs`, and #4920 requires those pages to stay audited. + docs: docList, + // The reporting fork: the release-owned subset, called out so a consumer can + // route it (review + file an issue) instead of listing it as editable work. + // A partition, not a filter ā€” `releaseOwnedDocs āŠ† docs` always. + releaseOwnedDocs: docList.filter(isReleaseOwned), + detail: detail || null, + testFilesSkipped, + scriptFilesSkipped, + devOnlyManifestsSkipped, + }, + null, + 2, + ) + '\n', + ); } else { process.stderr.write(`# ${summary}\n`); process.stdout.write(docList.join('\n') + (docList.length ? '\n' : '')); diff --git a/scripts/docs-audit/check-audit-scope.mjs b/scripts/docs-audit/check-audit-scope.mjs index 72cc30466b..55a8ba50f0 100644 --- a/scripts/docs-audit/check-audit-scope.mjs +++ b/scripts/docs-audit/check-audit-scope.mjs @@ -245,20 +245,36 @@ export function findGuardrailRow(agentsMd) { ); } -/** The prefix the workflow actually routes on. */ -export function parseReleaseOwnedPrefix(source) { +/** + * The prefix a consumer actually routes on. + * + * `label` names the file being parsed, because there is now more than one consumer: + * the audit workflow (read-only channel, #4920) and the drift-check mapper (read-only + * SECTION in its PR comment, #6893). Both hold their own literal copy ā€” the workflow + * because it is evaluated in a sandbox VM that cannot import, the mapper because a + * shared module importable by only one of the two would leave the other unanchored. + * `checkReleaseOwned` below is what keeps every copy equal to AGENTS.md's guardrail row. + */ +export function parseReleaseOwnedPrefix(source, label = WORKFLOW_REL) { const m = source.match(/const RELEASE_OWNED_PREFIX = '([^']*)'/); if (!m) { throw new Error( - `${WORKFLOW_REL}: no \`const RELEASE_OWNED_PREFIX = '...'\` declaration. That constant is ` + - `how the workflow tells release-owned pages (read-only, findings only) from editable ones; ` + - `without it every page in scope is editable, including ${RELEASE_OWNED_PREFIX}** ā€” the ` + - `collision #4920 was filed for. Restore it.`, + `${label}: no \`const RELEASE_OWNED_PREFIX = '...'\` declaration. That constant is ` + + `how it tells release-owned pages (read-only) from editable ones; without it every ` + + `page it reports is editable, including ${RELEASE_OWNED_PREFIX}** ā€” the collision ` + + `#4920 was filed for, and the one #6893 hit again in the drift comment. Restore it.`, ); } return m[1]; } +/** + * Every file that holds its own literal copy of the release-owned prefix. Adding a + * consumer means adding it here ā€” that is the whole cost of the "copies, anchored" + * shape, and it is cheaper than the alternative #4851 billed us for. + */ +const RELEASE_OWNED_CONSUMERS = [WORKFLOW_REL, 'scripts/docs-audit/affected-docs.mjs']; + /** * Run the workflow the way it really runs ā€” free globals, stub agents ā€” and report * what each doc in scope was actually handed. @@ -521,14 +537,21 @@ async function checkReleaseOwned(source, derived) { process.exit(1); } - const prefix = parseReleaseOwnedPrefix(source); - if (prefix !== RELEASE_OWNED_PREFIX) { - console.error( - `āœ— ${WORKFLOW_REL}: RELEASE_OWNED_PREFIX is "${prefix}", but ${AGENTS_REL} marks\n` + - ` "${RELEASE_OWNED_PREFIX}" RELEASE-OWNED. The workflow would review the wrong set of pages\n` + - ` read-only ā€” and edit the release notes it no longer recognises.\n`, - ); - process.exit(1); + // Every consumer's literal copy must equal the guardrail row's path. One drifting + // copy is silent by construction: the file keeps running, it just protects a set of + // pages the repo no longer marks read-only. + for (const rel of RELEASE_OWNED_CONSUMERS) { + const consumerSource = rel === WORKFLOW_REL ? source : readFileSync(join(REPO_ROOT, rel), 'utf8'); + const prefix = parseReleaseOwnedPrefix(consumerSource, rel); + if (prefix !== RELEASE_OWNED_PREFIX) { + console.error( + `āœ— ${rel}: RELEASE_OWNED_PREFIX is "${prefix}", but ${AGENTS_REL} marks\n` + + ` "${RELEASE_OWNED_PREFIX}" RELEASE-OWNED. It would treat the wrong set of pages as\n` + + ` read-only ā€” the audit would edit release notes it no longer recognises (#4920), and\n` + + ` the drift comment would list them as ordinary work (#6893).\n`, + ); + process.exit(1); + } } // The pages must still BE in scope. Zero of them is not "nothing to protect": it is