diff --git a/.github/workflows/docs-drift-check.yml b/.github/workflows/docs-drift-check.yml index f1f7275cd3..a3bf7b7f58 100644 --- a/.github/workflows/docs-drift-check.yml +++ b/.github/workflows/docs-drift-check.yml @@ -17,6 +17,13 @@ name: Docs Drift Check # because they document it through the SDK, which does not depend on the implementing # package. The comment now also states what the run could NOT see — the derivation being # read past its precision, with its silence taken for absence, is what #9192 records. +# +# And since #9519 it states WHICH TREE it read. On `pull_request`, actions/checkout gives +# the MERGE of base and head, so the row set is a fact about a commit that exists on no +# branch a reader can name — while the comment's own re-derivation command sent them to +# run the tool on their own worktree. A reader who did that got a different list and +# reported a correct row as a false positive, and the investigation of the non-existent +# defect cost a full round. Same posture, one more thing the run knows and used not to say. on: pull_request: @@ -98,7 +105,89 @@ jobs: const crossCutting = data.crossCuttingSymbols || []; const weak = data.weakAnchorsDropped || []; const coarse = data.packageMentionDocs || []; - const rederive = `node scripts/docs-audit/affected-docs.mjs --json origin/${baseRef}`; + // ── WHICH TREE THIS WAS COMPUTED ON (#9519) ───────────────────────────── + // The row set is a fact about two commits and this comment named neither. + // Pages are read with `readFileSync` from the WORKING TREE, and on a + // `pull_request` run `actions/checkout` gives us merge(base, head) — a tree + // that exists on no branch a reader can name and that GitHub drops once the + // PR closes. The change set is a diff whose base was published only as + // `origin/`, a moving NAME. The re-derivation command below then sent + // the reader to run the tool against THEIR tree, so a page that gained or + // lost an anchor token on the base branch after their worktree was cut + // produced a row they could not reproduce, with nothing here to say why. + // + // Measured: a reader did exactly that, grepped, found nothing, and reported + // a correct row as a false positive; the follow-up then investigated a + // defect class that does not exist in this tool — the anchor set is derived + // fresh per run, with no cache, index or snapshot anywhere — at the cost of + // a full round. Naming the tree is this file's own #9192 posture (say what + // the run could not see) applied to one more thing the run already knows. + // + // The mapper change behind this is strictly ADDITIVE: `computedOn` is read + // off git at the emit boundary, after every derivation has finished, so no + // row above depends on it. Verified byte-for-byte against the pre-change + // mapper on three refs — the `computedOn` block is the entire diff. + const computedOn = data.computedOn || {}; + const parents = Array.isArray(computedOn.headParents) ? computedOn.headParents : []; + // Parent ORDER on `refs/pull/N/merge` (base first, head second) is GitHub's + // convention, not git's, so it is CHECKED against the event payload rather + // than assumed — the pair is labelled only when the payload says which is + // which, and stays unlabelled otherwise. A confidently wrong label would + // send a reader off to rebuild the mirror image of the tree. + const prHeadSha = context.payload.pull_request.head.sha; + const mergedHead = parents.find(p => p === prHeadSha) || null; + const mergedBase = mergedHead ? (parents.find(p => p !== mergedHead) || null) : null; + // The COMMIT the mapper measured its diff from — the three-dot merge-base it + // had already resolved, not `origin/` re-read later. Naming the commit + // is what makes the command replayable from any clone; naming the branch is + // what made it a trap. + const diffBase = computedOn.diffBase || null; + const rederive = `node scripts/docs-audit/affected-docs.mjs --json ${diffBase || `origin/${baseRef}`}`; + const treeBlock = (() => { + // ⛔ Never degrade to silence here. An unnamed tree is the exact state this + // block exists to end, so a missing identity is SAID, not omitted. + if (!computedOn.head) { + return ['', '> ⚠️ This run could not identify the commit it read, so the list above cannot be' + + ' tied to a tree. Re-derive against your own checkout and compare by hand.']; + } + const lines = [ + '', + '
Which tree this was computed on', + '', + mergedHead && mergedBase + ? `This run read \`content/docs\` from \`${computedOn.head}\` — the merge of head` + + ` \`${mergedHead}\` into base \`${mergedBase}\`, which is what \`actions/checkout\`` + + ` gives a \`pull_request\` run. **Not** the PR head.` + : `This run read \`content/docs\` from \`${computedOn.head}\`.`, + '', + `A worktree cut from an older \`${baseRef}\` holds a different \`content/docs\`, so re-deriving` + + ` there can legitimately return a different list — that is a **different tree, not a wrong` + + ` row**. To answer on the same tree:`, + '', + '```sh', + ]; + if (mergedHead && mergedBase) { + lines.push( + '# while this PR is open — GitHub drops the merge commit once it closes', + `git fetch origin ${computedOn.head} && git checkout ${computedOn.head}`, + '# afterwards, rebuild it from the two parents, which stay fetchable', + `git fetch origin ${mergedBase} ${mergedHead} && git checkout -B drift-repro ${mergedBase} && git merge --no-ff ${mergedHead}`, + '', + ); + } else { + lines.push(`git fetch origin ${computedOn.head} && git checkout ${computedOn.head}`); + } + lines.push(rederive, '```'); + // A sha that misidentifies the tree is worse than no sha, so the one + // condition under which it does is stated right where the sha is. + if (computedOn.dirty === true) { + lines.push('', '⚠️ That checkout carried **uncommitted changes**, so the commit above does not fully identify what was read.'); + } else if (computedOn.dirty === null) { + lines.push('', '⚠️ This run could not check whether its checkout was clean, so the commit above may not fully identify what was read.'); + } + lines.push('
'); + return lines; + })(); const limits = []; if (anchorless.length) limits.push(`**${anchorless.length}** changed file(s) yielded no anchor (\`${anchorless.slice(0, 3).join('`, `')}\`${anchorless.length > 3 ? ', …' : ''}) — pages documenting those are invisible to this run`); if (crossCutting.length) limits.push(`**${crossCutting.length}** cross-cutting symbol(s) contributed no route anchor: \`${crossCutting.join('`, `')}\``); @@ -137,7 +226,13 @@ jobs: const headline = anchorList.length === 0 ? `Nothing in this diff resolved to a documentable surface (no symbol, route or SDK anchor derived from **${pkgs.length}** changed package(s)), so **this run has no opinion** about the docs.` : `**${anchorList.length}** anchor(s) derived from **${pkgs.length}** changed package(s); no hand-written page names any of them. ✅`; - body = [marker, '### 📓 Docs Drift Check', headline, ...limitsBlock].join('\n'); + // The tree identity is rendered here only when anchors WERE derived: that + // is the run with an opinion ("no page names any of them"), and a reader + // whose own tree does yield a row is owed the reason. With no anchors the + // run has no opinion to disagree with, and the bytes would be pure noise on + // every docs-tooling-only PR (#9037 — every byte here is relayed). + body = [marker, '### 📓 Docs Drift Check', headline, ...limitsBlock, + ...(anchorList.length ? treeBlock : [])].join('\n'); } else { const detail = (data.detail || []).reduce((m, d) => (m[d.doc] = d.via, m), {}); const row = d => `- \`${d}\`${detail[d] ? ` _(via ${detail[d].join(', ')})_` : ''}`; @@ -154,7 +249,7 @@ jobs: // fidelity is one command away, never lost. body.push( '', - `**${editable.length}** hand-written doc(s) name something this change touched — list omitted above ${EDITABLE_ROW_CAP} rows. Re-derive: \`node scripts/docs-audit/affected-docs.mjs --json origin/${baseRef}\`.`, + `**${editable.length}** hand-written doc(s) name something this change touched — list omitted above ${EDITABLE_ROW_CAP} rows. Re-derive on the tree named below: \`${rederive}\`.`, ); if (readOnly.length) { body.push( @@ -186,13 +281,21 @@ jobs: } } body.push(...limitsBlock); + body.push(...treeBlock); body.push( '', '> Advisory only, and a **precision-first** one (#9192): a page is listed because it names a', '> symbol, wire route or SDK method this diff touched — not because it mentions a changed', '> package. Each row says which anchor put it there, so a wrong row is reportable rather than', '> merely annoying. 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`.', + // Pinned to the commit the mapper measured from, and pointed at the tree + // named above (#9519): run this against a different tree and a different + // list is the CORRECT answer — which is exactly how a right row once got + // reported as a wrong one. + '> `node scripts/docs-audit/affected-docs.mjs ' + (diffBase || `origin/${baseRef}`) + '` → pass the list as', + computedOn.head + ? '> `args.docs`, on the commit named under **Which tree this was computed on**.' + : '> `args.docs`.', ); body = body.join('\n'); } diff --git a/scripts/docs-audit/affected-docs.mjs b/scripts/docs-audit/affected-docs.mjs index fab9c8e6d7..15c5d4ebb0 100644 --- a/scripts/docs-audit/affected-docs.mjs +++ b/scripts/docs-audit/affected-docs.mjs @@ -153,6 +153,11 @@ const args = process.argv.slice(2); const asJson = args.includes('--json'); const all = args.includes('--all'); const sinceRef = args.find((a) => !a.startsWith('--')) || 'origin/main'; +// The commit the change set is actually measured FROM, published in `computedOn` +// below (#9519). Declared up here so both `emit` call sites can read it — the `--all` +// arm returns long before §2 assigns it, and a `let` in §2 would put that arm in the +// temporal dead zone. `null` is the honest answer for `--all`: it diffs nothing. +let diffBaseRef = null; // --- 0. classifier constants ------------------------------------------------- // Declared up here, ahead of the `--self-test` short-circuit below, because `const` is @@ -515,6 +520,9 @@ let baseRef = sinceRef; if (threeDot) { try { baseRef = sh(`git merge-base ${sinceRef} HEAD`).trim() || sinceRef; } catch { /* keep sinceRef */ } } +// Publish it (#9519). READ-ONLY of a value §2 has already settled — this line adds no +// input to any derivation, it only lets the answer say what it was measured from. +diffBaseRef = baseRef; /** * A test file — it observes behaviour rather than defining it, so changing one cannot @@ -1727,6 +1735,46 @@ function selfTest() { ]; for (const [lit, want, label] of literalCases) check('literalAnchorsFromLines', label, lit, want, lits.has(lit)); + // ── `computedOn` (#9519): the record that names WHICH TREE the answer is about ── + // Pinned on the pure shaper, so these stay hermetic; the probing wrapper reads real + // git state by construction. Two properties carry the field's whole value: a merge + // commit's parents must survive as a PAIR — that pair is the only durable handle on + // an ephemeral `refs/pull/N/merge` tree — and "could not tell" must never be + // flattened into "checked, clean". + const mergeParents = '097fe96e1228f7da71f87e8f5ed95ae2739b53f1 047457ca3a8757012043460b8ded6090cbc9b114'; + const computedOnCases = [ + // [label, want, got] + ['a merge commit keeps BOTH parents, in order', + JSON.stringify(mergeParents.split(' ')), JSON.stringify(computedOnFrom('m', mergeParents, 'b', '').headParents)], + ['an ordinary commit has exactly one, trailing newline stripped', + JSON.stringify(['p1']), JSON.stringify(computedOnFrom('m', 'p1\n', 'b', '').headParents)], + ['a root commit has none — never a [""] entry', + JSON.stringify([]), JSON.stringify(computedOnFrom('m', '', 'b', '').headParents)], + ['a failed parent probe degrades to [] rather than throwing', + JSON.stringify([]), JSON.stringify(computedOnFrom('m', null, 'b', '').headParents)], + ['the head sha is trimmed', 'abc', computedOnFrom('abc\n', 'p', 'b', '').head], + ['a failed head probe is null, never the empty string', null, computedOnFrom(null, 'p', 'b', '').head], + ['`--all` diffs nothing, so it names no base', null, computedOnFrom('m', 'p', null, '').diffBase], + ['a clean checkout is dirty=false', false, computedOnFrom('m', 'p', 'b', '').dirty], + ['a modified page is dirty=true', true, computedOnFrom('m', 'p', 'b', ' M content/docs/x.mdx\n').dirty], + ['an UNTRACKED page counts too — walk() reads the filesystem, not the index', + true, computedOnFrom('m', 'p', 'b', '?? content/docs/new.mdx\n').dirty], + ['a failed status probe is null — "could not tell" is not "checked, clean"', + null, computedOnFrom('m', 'p', 'b', null).dirty], + ['the record carries exactly the four declared members', + 'head,headParents,diffBase,dirty', Object.keys(computedOnFrom('m', 'p', 'b', '')).join(',')], + ]; + for (const [label, want, got] of computedOnCases) check('computedOnFrom', label, 'computedOn', want, got); + + // PRESENCE, not merely shape. The field is worth nothing unless it reaches the JSON + // the workflow renders, and a rename or a dropped line there returns the comment to + // the unnamed-tree state this field exists to end — with every pin above still green. + // Read from source because the emitter writes to stdout under module-level flags and + // cannot be called hermetically. + const ownSource = readFileSync(new URL(import.meta.url), 'utf8'); + check('emit', 'the emitted JSON actually carries `computedOn`', 'affected-docs.mjs', + true, /\bcomputedOn:\s*computedOnIdentity\(/.test(ownSource)); + if (failed) { console.error(`\n✗ affected-docs self-test failed (${failed} case(s)).`); process.exit(1); @@ -2072,6 +2120,75 @@ emit( }, ); +/** + * Shape the `computedOn` record from raw git answers. PURE — every probe lives in + * `computedOnIdentity` below — so `--self-test` can pin the shape with no repo state. + * + * @param {string|null} head `git rev-parse HEAD` + * @param {string|null} parentLine `git log -1 --format=%P HEAD` — space-separated + * @param {string|null} diffBase the resolved commit the diff was measured from + * @param {string|null} porcelain `git status --porcelain`; null when the probe failed + */ +function computedOnFrom(head, parentLine, diffBase, porcelain) { + const one = (v) => (typeof v === 'string' && v.trim() ? v.trim() : null); + return { + head: one(head), + // A root commit has no parents and a failed probe answered nothing: both are the + // empty list, never a `['']` entry that reads downstream as a real commit. + headParents: typeof parentLine === 'string' ? parentLine.trim().split(/\s+/).filter(Boolean) : [], + diffBase: one(diffBase), + // "Could not tell" and "checked, clean" are DIFFERENT answers and must not render + // alike — the same distinction this tool's output draws everywhere else. + dirty: typeof porcelain === 'string' ? porcelain.trim().length > 0 : null, + }; +} + +/** + * Name the tree this run's answer is a fact ABOUT (#9519). + * + * The row set is a function of two commits and, until this field, the JSON named + * neither by anything stable. The pages are read with `readFileSync` from the WORKING + * TREE (`docTexts`, §3b) — not from any ref — and the change set is a diff whose base + * was published only as `sinceRef`, a moving NAME (`origin/main`), never a commit. + * + * On a `pull_request` run `actions/checkout` checks out merge(base, head), so the + * advisory is a fact about a tree that exists on no branch the reader can name and that + * GitHub drops once the PR closes. `headParents` is the durable handle on it: both + * parents stay fetchable, and re-merging them rebuilds the same tree. + * + * `diffBase` is the merge-base §2 already resolved, not `sinceRef` re-read here, and + * that distinction is what makes it reproducible: the diff is three-dot, so re-running + * with `origin/main` a day later measures from the same merge-base while re-running + * with THIS sha measures from it by construction — even from a clone whose `origin/main` + * has moved. Naming the commit is what makes the command replayable; naming the branch + * is what made it a trap. + * + * Measured cost of leaving all of it unsaid: a reader re-derived in a worktree cut from + * an older `main`, one page had gained an anchor token on `main` in between, and a + * correct row was reported as a false positive. The follow-up then investigated a defect + * class that does not exist in this tool — the anchor set is derived fresh per run, with + * no cache, index or snapshot anywhere — and cost a full round. + * + * `dirty` is this field's own correctness guard, not decoration: the tool reads the + * working tree, so with uncommitted changes present the shas do NOT identify what was + * read. A sha that misidentifies the tree is worse than no sha — the same defect, now + * wearing a credential. + * + * ⛔ Read-only, and deliberately evaluated HERE, at the emit boundary after every + * derivation has finished, so it cannot participate in deriving anything. Every probe + * degrades to `null` rather than throwing: this is a courtesy label on an advisory and + * must never be the reason a scan fails. + */ +function computedOnIdentity() { + const probe = (cmd) => { try { return sh(cmd); } catch { return null; } }; + return computedOnFrom( + probe('git rev-parse HEAD'), + probe('git log -1 --format=%P HEAD'), + diffBaseRef === null ? null : probe(`git rev-parse --verify --quiet ${JSON.stringify(`${diffBaseRef}^{commit}`)}`), + probe('git status --porcelain'), + ); +} + function emit(docList, changedPackages, summary, detail, skipped = {}, anchorInfo = {}) { const { testFilesSkipped = 0, scriptFilesSkipped = 0, devOnlyManifestsSkipped = 0 } = skipped; const { @@ -2085,6 +2202,10 @@ function emit(docList, changedPackages, summary, detail, skipped = {}, anchorInf { summary, sinceRef: all ? null : sinceRef, + // WHICH TREE THIS ANSWER IS A FACT ABOUT (#9519). `sinceRef` above is a NAME + // and names move; the pages were read from the WORKING TREE, which no field + // named at all. See `computedOnIdentity` for what each member is for. + computedOn: computedOnIdentity(), 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.