Skip to content

fix(docs-audit): derive the docs-drift advisory from what pages document, not package edges#9229

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
os-project-manager merged 1 commit into from
Aug 17, 2026
+965 −44
Merged

fix(docs-audit): derive the docs-drift advisory from what pages document, not package edges #9229

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
63 changes: 52 additions & 11 deletions .github/workflows/docs-drift-check.yml
View file
Open in desktop
Original file line numberDiff line numberDiff line change
@@ -1,10 +1,18 @@
name: Docs Drift Check

# When a PR changes packages/** code, flag the hand-written docs that reference the
# affected packages so they can be re-verified for implementation accuracy before the
# drift lands on main. Advisory only — posts a PR comment, never fails the build.
# The actual LLM audit is run on-demand / on a schedule via the `docs-accuracy-audit`
# workflow, scoped to exactly the docs this check lists.
# When a PR changes packages/** code, flag the hand-written docs that NAME something the
# change touched — a symbol, a wire route, or the SDK method a route ledger binds to it —
# so they can be re-verified for implementation accuracy before the drift lands on main.
# Advisory only: posts a PR comment, never fails the build. The actual LLM audit is run
# on-demand / on a schedule via the `docs-accuracy-audit` workflow, scoped to exactly the
# docs this check lists.
#
# It used to list pages by PACKAGE DEPENDENCY ("which docs mention @objectstack/x"), and
# #9192 measured that wrong in both directions on a real PR: 2 of 3 listed pages were
# irrelevant, and the 2 pages that actually documented the changed surface were missing
# 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.

on:
pull_request:
Expand DownExpand Up@@ -75,6 +83,30 @@ jobs:
const docs = data.docs || [];
const pkgs = (data.changedPackages || []).map(p => p.name || p.dir);
const marker = '<!-- docs-drift-check -->';
// #9192 — the derivation now matches pages that NAME something the change
// touched (symbol / route / SDK-method anchors) instead of pages that merely
// mention a changed package. Everything the derivation could NOT see is
// stated in the comment, at the point of use: the failure this fixed was not
// the tool lying, it was a reader taking its silence for absence.
const anchorList = data.anchors || [];
const anchorless = data.anchorlessChanges || [];
const overbroad = data.overbroadAnchors || [];
const crossCutting = data.crossCuttingSymbols || [];
const weak = data.weakAnchorsDropped || [];
const coarse = data.packageMentionDocs || [];
const rederive = `node scripts/docs-audit/affected-docs.mjs --json origin/${baseRef}`;
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('`, `')}\``);
if (overbroad.length) limits.push(`**${overbroad.length}** anchor(s) matched too much of the corpus to be a work list: \`${overbroad.join('`, `')}\``);
if (weak.length) limits.push(`**${weak.length}** name(s) were too generic to anchor anything (single lowercase words)`);
// Rendered whenever there is anything to say, INCLUDING when the only thing to
// say is "the wide net exists and holds N pages". A short list is the right
// answer here, but a reader must be able to tell a short list from a blind one
// without leaving the PR.
const limitsBlock = (limits.length || coarse.length)
? ['', '<details><summary>What this run could not see</summary>', '', ...limits.map(l => `- ${l}`), ...(limits.length ? [''] : []), `Coarse fallback — **${coarse.length}** page(s) merely *mention* a changed package (the pre-#9192 predicate, kept for the deliberately-wide backstop): \`${rederive}\` → \`packageMentionDocs\`.`, '</details>']
: [];
// 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
Expand All@@ -96,14 +128,19 @@ jobs:
const capped = editable.length > EDITABLE_ROW_CAP;
let body;
if (docs.length === 0) {
body = `${marker}\n### 📓 Docs Drift Check\nNo hand-written docs reference the ${pkgs.length} changed package(s). ✅`;
// "Nothing found" and "I derived nothing to look for" are DIFFERENT results
// and must never render alike — that conflation is #9192's own subject.
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');
} else {
const detail = (data.detail || []).reduce((m, d) => (m[d.doc] = d.via, m), {});
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(', ')}.`,
`This PR changes **${pkgs.length}** package(s): ${pkgs.map(p => `\`${p}\``).join(', ')}, touching **${anchorList.length}** documentable anchor(s).`,
];
if (capped) {
// NO row list above the cap — folding rows behind a details tag would
Expand All@@ -113,7 +150,7 @@ jobs:
// fidelity is one command away, never lost.
body.push(
'',
`**${editable.length}** hand-written doc(s) reference the affected code — 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: \`node scripts/docs-audit/affected-docs.mjs --json origin/${baseRef}\`.`,
);
if (readOnly.length) {
body.push(
Expand All@@ -125,15 +162,15 @@ jobs:
if (editable.length) {
body.push(
'',
`**${editable.length}** hand-written doc(s) reference the affected code and may need an implementation-accuracy re-verification:`,
`**${editable.length}** hand-written doc(s) NAME something this change touched 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.length}** release-owned page(s) ${editable.length ? 'also ' : ''}name something this change touched. These are **read-only**:`,
'',
readOnly.map(row).join('\n'),
'',
Expand All@@ -144,9 +181,13 @@ jobs:
);
}
}
body.push(...limitsBlock);
body.push(
'',
'> Advisory only. To re-verify, run the `docs-accuracy-audit` workflow scoped to these files:',
'> 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`.',
);
body = body.join('\n');
Expand Down
133 changes: 120 additions & 13 deletions scripts/docs-audit/README.md
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -9,8 +9,8 @@ The system has four parts, layered cheapest-and-earliest first:

## 1. `affected-docs.mjs` — change → docs mapping (the linchpin)

Maps a set of `packages/**` changes to the hand-written docs that reference the
affected packages, so an audit can be scoped to what actually changed.
Maps a set of `packages/**` changes to the hand-written docs that **name something the
change touched**, so an audit can be scoped to what actually changed.

```bash
# docs affected by changes on this branch vs origin/main
Expand All@@ -22,15 +22,114 @@ 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

# pin the change classifiers + package-root derivation (needs no repo state; CI runs this before the mapping)
# pin the classifiers, package-root and anchor derivations (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/<x>`) 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.
**Derivation (#9192): a doc is *affected* when it NAMES something the change touched.**
Not when it mentions the changed package — that predicate is a dependency-graph proxy
answering a semantic question, and it was measured wrong in *both* directions on PR #9191
(three read verbs in `@objectstack/metadata-protocol`): 3 pages listed of which 1 was
relevant, while the 2 pages that actually document the changed surface —
`api/client-sdk.mdx` and `kernel/contracts/metadata-service.mdx` — were absent, because
they document it through the **SDK** surface, which does not depend on the implementing
package at all.

Over-inclusion is not free, and that is the correction. A wrong-both-ways advisory trains
its reader to skip it, and then it fails on the PR where it is right — the same bill
exclusion 1 below already paid. The derivation is therefore **precision-first**: a shorter
right list beats a longer noisy one.

Three anchor kinds, each exact:

| anchor | what it is | how it is derived |
|:--|:--|:--|
| `symbol` | a documentable declaration the diff touched | the top-level declaration, or a member of a top-level **container** (class / interface / type / enum / schema object), enclosing each changed line — on **both** sides of the diff, so a removed export still anchors the pages naming it |
| `route` | a wire path the change touched | a path literal on a changed line, plus every route whose **registrar handler** references a changed symbol |
| `sdk` | the client method bound to an anchor route | the declared `route` ⟷ `client` rows in the repo's route ledgers |

The `route` and `sdk` hops are what carry the derivation across the surface boundary the
package graph cannot cross: `auditMetaItem` (changed) → `GET /api/v1/meta/:type/:name/audit`
(`rest-server.ts` registrar) → `meta.getAudit` (`rest-route-ledger.ts`) → the token
`api/client-sdk.mdx` actually contains.

**A local variable is not documentable surface.** That one rule is what drops the measured
false positive: `const singular = request.type;` inside a method body is not an anchor, so
`kernel/services-checklist.mdx` — whose only `singular` is a service *slot name* — is no
longer listed. A `const` object **is** a container (its keys are metadata property names,
which docs do name); a function body is not.

### Two guards, and both publish what they removed

The first build of this derivation was, on some PRs, *noisier* than the proxy it replaced
(134 rows where the old tool gave 26). Two guards fixed that, and both run **before** the
route bridge — a name left in the set does not merely add a noisy row, it mints noisy route
and SDK anchors from every registrar handler that mentions it:

1. **Shape** — an anchor must be code-shaped (camelCase / PascalCase / snake_case /
dotted). `label`, `object`, `start`, `locale` and `sections` all arrived as real
declarations and matched 82, 113, 43, 13 and 10 of 178 pages; confining them to code
spans does not help, because those words live in code spans too. Reported as
`weakAnchorsDropped`. The recall cost is a genuinely lowercase export (`parse`, `mask`).
2. **Corpus share** — an anchor matching more than 15% of the corpus is a hub term, not an
identifier. `ObjectQL` is code-shaped, genuinely changed, and named by 59 of 178 pages;
it cannot tell an author which page to re-read. Reported as `overbroadAnchors`, with the
count that condemned it.

Plus a cap on the route bridge itself: a symbol wired into more than 3 routes is a
cross-cutting helper, and "which routes mention this name" then answers *every* route.
Reported as `crossCuttingSymbols`. `SCREAMING_SNAKE` constants are kept out of the bridge
entirely — a data table is consulted by handlers, it is not their implementation.

### What it cannot see is reported, never implied

`anchorlessChanges` lists changed files that yielded no anchor at all; a non-empty value
means the list is incomplete **by a known amount**, and an empty `docs` beside it must
never be read as "no page documents this change". The superseded coarse set is still
computed and emitted as `packageMentionDocs`, labelled — an audit that deliberately wants
the wide net can still ask for it, and keeping it visible is how a reader tells a *narrow*
list from a *blind* one. The PR comment renders all of this in a collapsed section, because
the failure #9192 records was never the tool lying — it was the tool never signalling its
own limits at the point of use.

### Measured, before and after

Ten real PRs, each re-derived at its own merge base with its own docs corpus. `docs` rows:

| PR / commit | old (package-mention) | new (anchor) |
|:--|--:|--:|
| #9191 — the three metadata read verbs (the filing card's specimen) | 4 | **3** |
| `0668f02a6` fix(rest): closed `ErrorCode` union on the error responder | 26 | 14 |
| `75b7c240a` feat(spec): `master_detail` + `controlled_by_parent` | 113 | 32 |
| `07ad42463` fix(cli): `os meta resync` skip-count explanation | 22 | **0** |
| `7a537ce90` feat(spec): strict top-level stack keys | 113 | 13 |
| `445ae4deb` fix(auth): auth emails follow the deployment locale | 13 | 3 |
| `30b1c636a` feat(spec): register 9 REST wire codes | 113 | 4 |
| `650cd3daa` fix(objectql): delete-cascade registry reads | 14 | **0** |
| `3851f87f0` feat(spec,plugin-security): partial field masking | 116 | 19 |
| `d5156b965` refactor(metadata-protocol): drop dead `objects` tolerances | 4 | 4 |

The #9191 row reads 4 where the filing card says "the bot listed three pages": `docs` is
the full set and the comment partitions `content/docs/releases/v9.mdx` into its own
read-only section (#6893), so 3 editable rows + 1 release-owned row = 4.

On #9191 the change is qualitative, not just smaller: all three previously-listed pages
are gone and the two pages the filing card measured as *missing* are back, each with the
anchor that put it there (`getAudit`/`getReferences` for `client-sdk.mdx`, `getHistory`
for `metadata-service.mdx`).

The two zeroes are the honest shape of the trade, not a bug: `07ad42463` derives
`MetaResync` and `resyncSkipExplanationLine`, and no hand-written page names either, so the
run says so and points at the coarse set — where the old tool's 22 rows were every page
mentioning `@objectstack/cli`. A CLI **command name** (`os meta resync`) is exactly the
recall class the shape guard costs us: it is a lowercase word, so it cannot anchor.

**Cost** (the card's open question): the anchor derivation reads the same 178-page corpus
the old one did, plus the 18 route-registrar/ledger sources (~875 KB) and one `git show`
per changed file per side. Measured end-to-end on the ten PRs above, `node affected-docs.mjs`
went from 85-195 ms to 114-582 ms. The heaviest case is the widest diff; every case stays
well under a second, against a job that already spends seconds checking out the repo and
setting up Node. It is the right default for every PR.

**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
Expand DownExpand Up@@ -88,8 +187,8 @@ stale are dropped before the changed-package roots are derived:

The excluded counts are reported in the summary line and as `testFilesSkipped` /
`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;
silent. `--self-test` pins the classifiers, the package-root derivation *and* the anchor
derivation against inputs 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).

Expand DownExpand Up@@ -196,9 +295,17 @@ out of an in-memory copy and requires that check to go red.
## 2. CI gate — `.github/workflows/docs-drift-check.yml`

On any PR that touches `packages/**`, runs `affected-docs.mjs` against the base branch
and posts/updates a single advisory PR comment listing the docs that reference the
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.
and posts/updates a single advisory PR comment listing the docs that name something the
change touched — each row carrying **the anchor that put it there**, so a wrong row is
reportable rather than merely annoying. **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 also carries a collapsed **"What this run could not see"** section:
anchorless files, cross-cutting symbols, over-broad anchors, and the coarse
package-mention count. That is the point-of-use half of #9192 — every one of the three
derived-list failures in that shift was caught only because a dev widened the probe past
what the tool offered, never because the tool signalled its own limits where it was read.

### The comment forks release-owned pages into a read-only section (#6893)

Expand Down
Loading
Loading