Skip to content

Commit 02ebb6f

Browse files
os-steveclaude
andauthored
fix(ci): docs-drift advisory names the tree it was computed on (#9519) (#9524)
On `pull_request`, actions/checkout gives the MERGE of base and head, and the mapper reads every page off that working tree with readFileSync. The row set was therefore a fact about a commit that exists on no branch a reader can name — and the comment's own re-derivation instruction sent them to run the tool against their own worktree, where a page that gained or lost an anchor token on the base branch after their branch was cut yields a different list. A reader did that, grepped, found nothing, and reported a correct row as a false positive; the follow-up investigated a defect class this tool does not have and cost a round. - affected-docs.mjs emits `computedOn` = { head, headParents, diffBase, dirty }. `diffBase` is the three-dot merge-base §2 had already resolved, not `sinceRef` re-read — naming the commit is what makes the command replayable from a clone whose `origin/main` has moved; naming the branch is what made it a trap. `dirty` is the field's own correctness guard: the tool reads the working tree, so with uncommitted changes present the shas do not identify what was read. - The workflow renders it beside the re-derivation commands, with a recipe that lands the reader on the same tree — the ephemeral merge commit while the PR is open, its two parents (which stay fetchable) afterwards. - The re-derivation commands are pinned to `diffBase` instead of `origin/<base>`. Strictly additive: `computedOn` is read off git at the emit boundary, after every derivation has finished. Verified byte-for-byte against the pre-change mapper on three refs — the `computedOn` block is the entire diff. Self-test 220 -> 233. Claude-Session: https://claude.ai/code/session_01XqDQYVU5smx29ts9pAErja Co-authored-by: Claude <noreply@anthropic.com>
1 parent 53fc099 commit 02ebb6f

2 files changed

Lines changed: 228 additions & 4 deletions

File tree

.github/workflows/docs-drift-check.yml

Lines changed: 107 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,13 @@ name: Docs Drift Check
1717
# because they document it through the SDK, which does not depend on the implementing
1818
# package. The comment now also states what the run could NOT see — the derivation being
1919
# read past its precision, with its silence taken for absence, is what #9192 records.
20+
#
21+
# And since #9519 it states WHICH TREE it read. On `pull_request`, actions/checkout gives
22+
# the MERGE of base and head, so the row set is a fact about a commit that exists on no
23+
# branch a reader can name — while the comment's own re-derivation command sent them to
24+
# run the tool on their own worktree. A reader who did that got a different list and
25+
# reported a correct row as a false positive, and the investigation of the non-existent
26+
# defect cost a full round. Same posture, one more thing the run knows and used not to say.
2027

2128
on:
2229
pull_request:
@@ -98,7 +105,89 @@ jobs:
98105
const crossCutting = data.crossCuttingSymbols || [];
99106
const weak = data.weakAnchorsDropped || [];
100107
const coarse = data.packageMentionDocs || [];
101-
const rederive = `node scripts/docs-audit/affected-docs.mjs --json origin/${baseRef}`;
108+
// ── WHICH TREE THIS WAS COMPUTED ON (#9519) ─────────────────────────────
109+
// The row set is a fact about two commits and this comment named neither.
110+
// Pages are read with `readFileSync` from the WORKING TREE, and on a
111+
// `pull_request` run `actions/checkout` gives us merge(base, head) — a tree
112+
// that exists on no branch a reader can name and that GitHub drops once the
113+
// PR closes. The change set is a diff whose base was published only as
114+
// `origin/<base>`, a moving NAME. The re-derivation command below then sent
115+
// the reader to run the tool against THEIR tree, so a page that gained or
116+
// lost an anchor token on the base branch after their worktree was cut
117+
// produced a row they could not reproduce, with nothing here to say why.
118+
//
119+
// Measured: a reader did exactly that, grepped, found nothing, and reported
120+
// a correct row as a false positive; the follow-up then investigated a
121+
// defect class that does not exist in this tool — the anchor set is derived
122+
// fresh per run, with no cache, index or snapshot anywhere — at the cost of
123+
// a full round. Naming the tree is this file's own #9192 posture (say what
124+
// the run could not see) applied to one more thing the run already knows.
125+
//
126+
// The mapper change behind this is strictly ADDITIVE: `computedOn` is read
127+
// off git at the emit boundary, after every derivation has finished, so no
128+
// row above depends on it. Verified byte-for-byte against the pre-change
129+
// mapper on three refs — the `computedOn` block is the entire diff.
130+
const computedOn = data.computedOn || {};
131+
const parents = Array.isArray(computedOn.headParents) ? computedOn.headParents : [];
132+
// Parent ORDER on `refs/pull/N/merge` (base first, head second) is GitHub's
133+
// convention, not git's, so it is CHECKED against the event payload rather
134+
// than assumed — the pair is labelled only when the payload says which is
135+
// which, and stays unlabelled otherwise. A confidently wrong label would
136+
// send a reader off to rebuild the mirror image of the tree.
137+
const prHeadSha = context.payload.pull_request.head.sha;
138+
const mergedHead = parents.find(p => p === prHeadSha) || null;
139+
const mergedBase = mergedHead ? (parents.find(p => p !== mergedHead) || null) : null;
140+
// The COMMIT the mapper measured its diff from — the three-dot merge-base it
141+
// had already resolved, not `origin/<base>` re-read later. Naming the commit
142+
// is what makes the command replayable from any clone; naming the branch is
143+
// what made it a trap.
144+
const diffBase = computedOn.diffBase || null;
145+
const rederive = `node scripts/docs-audit/affected-docs.mjs --json ${diffBase || `origin/${baseRef}`}`;
146+
const treeBlock = (() => {
147+
// ⛔ Never degrade to silence here. An unnamed tree is the exact state this
148+
// block exists to end, so a missing identity is SAID, not omitted.
149+
if (!computedOn.head) {
150+
return ['', '> ⚠️ This run could not identify the commit it read, so the list above cannot be'
151+
+ ' tied to a tree. Re-derive against your own checkout and compare by hand.'];
152+
}
153+
const lines = [
154+
'',
155+
'<details><summary>Which tree this was computed on</summary>',
156+
'',
157+
mergedHead && mergedBase
158+
? `This run read \`content/docs\` from \`${computedOn.head}\` — the merge of head`
159+
+ ` \`${mergedHead}\` into base \`${mergedBase}\`, which is what \`actions/checkout\``
160+
+ ` gives a \`pull_request\` run. **Not** the PR head.`
161+
: `This run read \`content/docs\` from \`${computedOn.head}\`.`,
162+
'',
163+
`A worktree cut from an older \`${baseRef}\` holds a different \`content/docs\`, so re-deriving`
164+
+ ` there can legitimately return a different list — that is a **different tree, not a wrong`
165+
+ ` row**. To answer on the same tree:`,
166+
'',
167+
'```sh',
168+
];
169+
if (mergedHead && mergedBase) {
170+
lines.push(
171+
'# while this PR is open — GitHub drops the merge commit once it closes',
172+
`git fetch origin ${computedOn.head} && git checkout ${computedOn.head}`,
173+
'# afterwards, rebuild it from the two parents, which stay fetchable',
174+
`git fetch origin ${mergedBase} ${mergedHead} && git checkout -B drift-repro ${mergedBase} && git merge --no-ff ${mergedHead}`,
175+
'',
176+
);
177+
} else {
178+
lines.push(`git fetch origin ${computedOn.head} && git checkout ${computedOn.head}`);
179+
}
180+
lines.push(rederive, '```');
181+
// A sha that misidentifies the tree is worse than no sha, so the one
182+
// condition under which it does is stated right where the sha is.
183+
if (computedOn.dirty === true) {
184+
lines.push('', '⚠️ That checkout carried **uncommitted changes**, so the commit above does not fully identify what was read.');
185+
} else if (computedOn.dirty === null) {
186+
lines.push('', '⚠️ This run could not check whether its checkout was clean, so the commit above may not fully identify what was read.');
187+
}
188+
lines.push('</details>');
189+
return lines;
190+
})();
102191
const limits = [];
103192
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`);
104193
if (crossCutting.length) limits.push(`**${crossCutting.length}** cross-cutting symbol(s) contributed no route anchor: \`${crossCutting.join('`, `')}\``);
@@ -137,7 +226,13 @@ jobs:
137226
const headline = anchorList.length === 0
138227
? `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.`
139228
: `**${anchorList.length}** anchor(s) derived from **${pkgs.length}** changed package(s); no hand-written page names any of them. ✅`;
140-
body = [marker, '### 📓 Docs Drift Check', headline, ...limitsBlock].join('\n');
229+
// The tree identity is rendered here only when anchors WERE derived: that
230+
// is the run with an opinion ("no page names any of them"), and a reader
231+
// whose own tree does yield a row is owed the reason. With no anchors the
232+
// run has no opinion to disagree with, and the bytes would be pure noise on
233+
// every docs-tooling-only PR (#9037 — every byte here is relayed).
234+
body = [marker, '### 📓 Docs Drift Check', headline, ...limitsBlock,
235+
...(anchorList.length ? treeBlock : [])].join('\n');
141236
} else {
142237
const detail = (data.detail || []).reduce((m, d) => (m[d.doc] = d.via, m), {});
143238
const row = d => `- \`${d}\`${detail[d] ? ` _(via ${detail[d].join(', ')})_` : ''}`;
@@ -154,7 +249,7 @@ jobs:
154249
// fidelity is one command away, never lost.
155250
body.push(
156251
'',
157-
`**${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}\`.`,
252+
`**${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}\`.`,
158253
);
159254
if (readOnly.length) {
160255
body.push(
@@ -186,13 +281,21 @@ jobs:
186281
}
187282
}
188283
body.push(...limitsBlock);
284+
body.push(...treeBlock);
189285
body.push(
190286
'',
191287
'> Advisory only, and a **precision-first** one (#9192): a page is listed because it names a',
192288
'> symbol, wire route or SDK method this diff touched — not because it mentions a changed',
193289
'> package. Each row says which anchor put it there, so a wrong row is reportable rather than',
194290
'> merely annoying. To re-verify, run the `docs-accuracy-audit` workflow scoped to these files:',
195-
'> `node scripts/docs-audit/affected-docs.mjs origin/' + baseRef + '` → pass the list as `args.docs`.',
291+
// Pinned to the commit the mapper measured from, and pointed at the tree
292+
// named above (#9519): run this against a different tree and a different
293+
// list is the CORRECT answer — which is exactly how a right row once got
294+
// reported as a wrong one.
295+
'> `node scripts/docs-audit/affected-docs.mjs ' + (diffBase || `origin/${baseRef}`) + '` → pass the list as',
296+
computedOn.head
297+
? '> `args.docs`, on the commit named under **Which tree this was computed on**.'
298+
: '> `args.docs`.',
196299
);
197300
body = body.join('\n');
198301
}

scripts/docs-audit/affected-docs.mjs

Lines changed: 121 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -153,6 +153,11 @@ const args = process.argv.slice(2);
153153
const asJson = args.includes('--json');
154154
const all = args.includes('--all');
155155
const sinceRef = args.find((a) => !a.startsWith('--')) || 'origin/main';
156+
// The commit the change set is actually measured FROM, published in `computedOn`
157+
// below (#9519). Declared up here so both `emit` call sites can read it — the `--all`
158+
// arm returns long before §2 assigns it, and a `let` in §2 would put that arm in the
159+
// temporal dead zone. `null` is the honest answer for `--all`: it diffs nothing.
160+
let diffBaseRef = null;
156161

157162
// --- 0. classifier constants -------------------------------------------------
158163
// Declared up here, ahead of the `--self-test` short-circuit below, because `const` is
@@ -515,6 +520,9 @@ let baseRef = sinceRef;
515520
if (threeDot) {
516521
try { baseRef = sh(`git merge-base ${sinceRef} HEAD`).trim() || sinceRef; } catch { /* keep sinceRef */ }
517522
}
523+
// Publish it (#9519). READ-ONLY of a value §2 has already settled — this line adds no
524+
// input to any derivation, it only lets the answer say what it was measured from.
525+
diffBaseRef = baseRef;
518526

519527
/**
520528
* A test file — it observes behaviour rather than defining it, so changing one cannot
@@ -1727,6 +1735,46 @@ function selfTest() {
17271735
];
17281736
for (const [lit, want, label] of literalCases) check('literalAnchorsFromLines', label, lit, want, lits.has(lit));
17291737

1738+
// ── `computedOn` (#9519): the record that names WHICH TREE the answer is about ──
1739+
// Pinned on the pure shaper, so these stay hermetic; the probing wrapper reads real
1740+
// git state by construction. Two properties carry the field's whole value: a merge
1741+
// commit's parents must survive as a PAIR — that pair is the only durable handle on
1742+
// an ephemeral `refs/pull/N/merge` tree — and "could not tell" must never be
1743+
// flattened into "checked, clean".
1744+
const mergeParents = '097fe96e1228f7da71f87e8f5ed95ae2739b53f1 047457ca3a8757012043460b8ded6090cbc9b114';
1745+
const computedOnCases = [
1746+
// [label, want, got]
1747+
['a merge commit keeps BOTH parents, in order',
1748+
JSON.stringify(mergeParents.split(' ')), JSON.stringify(computedOnFrom('m', mergeParents, 'b', '').headParents)],
1749+
['an ordinary commit has exactly one, trailing newline stripped',
1750+
JSON.stringify(['p1']), JSON.stringify(computedOnFrom('m', 'p1\n', 'b', '').headParents)],
1751+
['a root commit has none — never a [""] entry',
1752+
JSON.stringify([]), JSON.stringify(computedOnFrom('m', '', 'b', '').headParents)],
1753+
['a failed parent probe degrades to [] rather than throwing',
1754+
JSON.stringify([]), JSON.stringify(computedOnFrom('m', null, 'b', '').headParents)],
1755+
['the head sha is trimmed', 'abc', computedOnFrom('abc\n', 'p', 'b', '').head],
1756+
['a failed head probe is null, never the empty string', null, computedOnFrom(null, 'p', 'b', '').head],
1757+
['`--all` diffs nothing, so it names no base', null, computedOnFrom('m', 'p', null, '').diffBase],
1758+
['a clean checkout is dirty=false', false, computedOnFrom('m', 'p', 'b', '').dirty],
1759+
['a modified page is dirty=true', true, computedOnFrom('m', 'p', 'b', ' M content/docs/x.mdx\n').dirty],
1760+
['an UNTRACKED page counts too — walk() reads the filesystem, not the index',
1761+
true, computedOnFrom('m', 'p', 'b', '?? content/docs/new.mdx\n').dirty],
1762+
['a failed status probe is null — "could not tell" is not "checked, clean"',
1763+
null, computedOnFrom('m', 'p', 'b', null).dirty],
1764+
['the record carries exactly the four declared members',
1765+
'head,headParents,diffBase,dirty', Object.keys(computedOnFrom('m', 'p', 'b', '')).join(',')],
1766+
];
1767+
for (const [label, want, got] of computedOnCases) check('computedOnFrom', label, 'computedOn', want, got);
1768+
1769+
// PRESENCE, not merely shape. The field is worth nothing unless it reaches the JSON
1770+
// the workflow renders, and a rename or a dropped line there returns the comment to
1771+
// the unnamed-tree state this field exists to end — with every pin above still green.
1772+
// Read from source because the emitter writes to stdout under module-level flags and
1773+
// cannot be called hermetically.
1774+
const ownSource = readFileSync(new URL(import.meta.url), 'utf8');
1775+
check('emit', 'the emitted JSON actually carries `computedOn`', 'affected-docs.mjs',
1776+
true, /\bcomputedOn:\s*computedOnIdentity\(/.test(ownSource));
1777+
17301778
if (failed) {
17311779
console.error(`\n✗ affected-docs self-test failed (${failed} case(s)).`);
17321780
process.exit(1);
@@ -2072,6 +2120,75 @@ emit(
20722120
},
20732121
);
20742122

2123+
/**
2124+
* Shape the `computedOn` record from raw git answers. PURE — every probe lives in
2125+
* `computedOnIdentity` below — so `--self-test` can pin the shape with no repo state.
2126+
*
2127+
* @param {string|null} head `git rev-parse HEAD`
2128+
* @param {string|null} parentLine `git log -1 --format=%P HEAD` — space-separated
2129+
* @param {string|null} diffBase the resolved commit the diff was measured from
2130+
* @param {string|null} porcelain `git status --porcelain`; null when the probe failed
2131+
*/
2132+
function computedOnFrom(head, parentLine, diffBase, porcelain) {
2133+
const one = (v) => (typeof v === 'string' && v.trim() ? v.trim() : null);
2134+
return {
2135+
head: one(head),
2136+
// A root commit has no parents and a failed probe answered nothing: both are the
2137+
// empty list, never a `['']` entry that reads downstream as a real commit.
2138+
headParents: typeof parentLine === 'string' ? parentLine.trim().split(/\s+/).filter(Boolean) : [],
2139+
diffBase: one(diffBase),
2140+
// "Could not tell" and "checked, clean" are DIFFERENT answers and must not render
2141+
// alike — the same distinction this tool's output draws everywhere else.
2142+
dirty: typeof porcelain === 'string' ? porcelain.trim().length > 0 : null,
2143+
};
2144+
}
2145+
2146+
/**
2147+
* Name the tree this run's answer is a fact ABOUT (#9519).
2148+
*
2149+
* The row set is a function of two commits and, until this field, the JSON named
2150+
* neither by anything stable. The pages are read with `readFileSync` from the WORKING
2151+
* TREE (`docTexts`, §3b) — not from any ref — and the change set is a diff whose base
2152+
* was published only as `sinceRef`, a moving NAME (`origin/main`), never a commit.
2153+
*
2154+
* On a `pull_request` run `actions/checkout` checks out merge(base, head), so the
2155+
* advisory is a fact about a tree that exists on no branch the reader can name and that
2156+
* GitHub drops once the PR closes. `headParents` is the durable handle on it: both
2157+
* parents stay fetchable, and re-merging them rebuilds the same tree.
2158+
*
2159+
* `diffBase` is the merge-base §2 already resolved, not `sinceRef` re-read here, and
2160+
* that distinction is what makes it reproducible: the diff is three-dot, so re-running
2161+
* with `origin/main` a day later measures from the same merge-base while re-running
2162+
* with THIS sha measures from it by construction — even from a clone whose `origin/main`
2163+
* has moved. Naming the commit is what makes the command replayable; naming the branch
2164+
* is what made it a trap.
2165+
*
2166+
* Measured cost of leaving all of it unsaid: a reader re-derived in a worktree cut from
2167+
* an older `main`, one page had gained an anchor token on `main` in between, and a
2168+
* correct row was reported as a false positive. The follow-up then investigated a defect
2169+
* class that does not exist in this tool — the anchor set is derived fresh per run, with
2170+
* no cache, index or snapshot anywhere — and cost a full round.
2171+
*
2172+
* `dirty` is this field's own correctness guard, not decoration: the tool reads the
2173+
* working tree, so with uncommitted changes present the shas do NOT identify what was
2174+
* read. A sha that misidentifies the tree is worse than no sha — the same defect, now
2175+
* wearing a credential.
2176+
*
2177+
* ⛔ Read-only, and deliberately evaluated HERE, at the emit boundary after every
2178+
* derivation has finished, so it cannot participate in deriving anything. Every probe
2179+
* degrades to `null` rather than throwing: this is a courtesy label on an advisory and
2180+
* must never be the reason a scan fails.
2181+
*/
2182+
function computedOnIdentity() {
2183+
const probe = (cmd) => { try { return sh(cmd); } catch { return null; } };
2184+
return computedOnFrom(
2185+
probe('git rev-parse HEAD'),
2186+
probe('git log -1 --format=%P HEAD'),
2187+
diffBaseRef === null ? null : probe(`git rev-parse --verify --quiet ${JSON.stringify(`${diffBaseRef}^{commit}`)}`),
2188+
probe('git status --porcelain'),
2189+
);
2190+
}
2191+
20752192
function emit(docList, changedPackages, summary, detail, skipped = {}, anchorInfo = {}) {
20762193
const { testFilesSkipped = 0, scriptFilesSkipped = 0, devOnlyManifestsSkipped = 0 } = skipped;
20772194
const {
@@ -2085,6 +2202,10 @@ function emit(docList, changedPackages, summary, detail, skipped = {}, anchorInf
20852202
{
20862203
summary,
20872204
sinceRef: all ? null : sinceRef,
2205+
// WHICH TREE THIS ANSWER IS A FACT ABOUT (#9519). `sinceRef` above is a NAME
2206+
// and names move; the pages were read from the WORKING TREE, which no field
2207+
// named at all. See `computedOnIdentity` for what each member is for.
2208+
computedOn: computedOnIdentity(),
20882209
changedPackages,
20892210
// The FULL set, release-owned pages included — this is what feeds the audit
20902211
// workflow's `args.docs`, and #4920 requires those pages to stay audited.

0 commit comments

Comments
 (0)