Skip to content

Commit 64489f8

Browse files
os-zhuangclaude
andauthored
fix(docs-audit): derive package roots from the filesystem, not a special-cased regex (#4162) (#4206)
The drift mapper special-cased only `packages/plugins/*`, so the 30 packages nested under the other six container dirs collapsed into their container — whose missing package.json killed the npm-name matching arm. A doc naming `@objectstack/service-automation` but never the repo path was a guaranteed miss (`automation/flows.mdx` on a service-automation- only change). The root is now the deepest ancestor with a package.json, with no hardcoded container list to drift out of sync. Also from the issue thread: `<packageRoot>/scripts/**` is build tooling, not documented behaviour, and no longer counts as an implementation change (#4183); `package.json` and `src/scripts/**` stay counted. Both the derivation and the classifiers are now pinned by `--self-test`. Claude-Session: https://claude.ai/code/session_01J2x8Tie9WT1VgWFifqMKrR Co-authored-by: Claude <noreply@anthropic.com>
1 parent ea0cb39 commit 64489f8

4 files changed

Lines changed: 233 additions & 51 deletions

File tree

Lines changed: 38 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,38 @@
1+
---
2+
---
3+
4+
Tooling only — no package changes, nothing to release.
5+
6+
The docs-drift mapper (`scripts/docs-audit/affected-docs.mjs`) derives a changed
7+
file's package root from the filesystem — the deepest ancestor directory with a
8+
`package.json` — instead of a regex that special-cased only `packages/plugins/*`.
9+
Under the old derivation the 30 packages nested under the other six container
10+
directories (`services/`, `connectors/`, `apps/`, `qa/`, `triggers/`, `adapters/`)
11+
collapsed into their container, whose missing `package.json` left the npm name
12+
unresolved and the npm-name matching arm dead: any doc that names
13+
`@objectstack/service-automation` but never the repo path was a guaranteed miss
14+
(#4162), the one direction this tool promises to avoid. No hardcoded container
15+
list replaces the special case — that would fail the same way again on container
16+
number eight (#3786's pattern).
17+
18+
- Verified against real history: on #4161's commit the drift comment attribution
19+
changes from the directory name `packages/services` to
20+
`@objectstack/service-automation`. A synthetic service-automation-only change
21+
goes from 6 docs — all belonging to *other* services, matched via the coarse
22+
`packages/services` path token, with `automation/flows.mdx` absent — to the 4
23+
right ones, `flows.mdx` first among them.
24+
- Second arm (from #4162's comment thread): `<packageRoot>/scripts/**` is
25+
build/verification tooling and no longer counts as an implementation change
26+
(#4183 flagged 106 docs for a diff whose only code change was a new check
27+
script). Kept narrow: `package.json` and `src/scripts/**` stay counted.
28+
Publication check done — no package ships runtime code from `scripts/`; three
29+
plugins publish a lone `i18n-extract.config.ts` only for lack of a `files`
30+
field. A scripts-only change now maps to 0 docs (was 106); the exclusion is
31+
reported in the summary and as `scriptFilesSkipped` in `--json`, never silent.
32+
- `--self-test` now pins the package-root derivation and the tooling-script
33+
classifier too (32 cases, hermetic via an injected fake tree), closing the
34+
guard-of-the-guard hole: the original self-test pinned only the test-file
35+
matcher, so this bug was invisible to it. Includes the two invariants from the
36+
issue: a container directory must never come out as a package root, and
37+
`packages/x/package.json` is implementation while `packages/x/scripts/y.ts`
38+
is not.

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

Lines changed: 5 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -41,10 +41,11 @@ jobs:
4141
- name: Fetch base branch
4242
run: git fetch --no-tags origin "${{ github.base_ref }}"
4343

44-
# The mapper excludes test files (a test cannot make an implementation doc
45-
# stale). Self-test first, so a regression that widened the exclusion into
46-
# dropping real implementation changes fails loudly here instead of turning
47-
# this whole comment quietly empty.
44+
# The mapper excludes test files and package tooling scripts (neither can make
45+
# an implementation doc stale) and derives package roots from the filesystem.
46+
# Self-test first, so a regression that widened an exclusion into dropping real
47+
# implementation changes — or collapsed nested packages into their container
48+
# again (#4162) — fails loudly here instead of quietly skewing this comment.
4849
- name: Self-test the change → docs mapper
4950
run: node scripts/docs-audit/affected-docs.mjs --self-test
5051

scripts/docs-audit/README.md

Lines changed: 36 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -22,24 +22,46 @@ node scripts/docs-audit/affected-docs.mjs --json origin/main
2222
# every hand-written doc (full audit scope)
2323
node scripts/docs-audit/affected-docs.mjs --all
2424

25-
# check the test-file matcher (needs no repo state; CI runs this before the mapping)
25+
# pin the change classifiers + package-root derivation (needs no repo state; CI runs this before the mapping)
2626
node scripts/docs-audit/affected-docs.mjs --self-test
2727
```
2828

2929
Heuristic: a doc is *affected* by a changed package `P` if it mentions `P`'s npm
30-
name (`@objectstack/<x>`) or repo path (`packages/<x>`). Over-inclusion is preferred
31-
over misses; the periodic **full** audit (part 4) is the backstop for docs that
32-
describe a package without naming it.
33-
34-
**One exclusion:** changes to **test files** are dropped before the changed-package
35-
roots are derived. A test observes behaviour rather than defining it, so it cannot make
36-
an implementation-accuracy doc stale — yet counting them made every tests-only PR light
37-
up its packages' whole doc set, a class of finding that is always false. That is the one
38-
place over-inclusion actively hurt: a comment a reader learns to skip stops working on
39-
the PR where it is right. The count of excluded files is reported in the summary and as
40-
`testFilesSkipped` in `--json`, so the narrowing is never silent, and `--self-test`
41-
pins the matcher against paths that must and must not match (`commands/test.ts` is
42-
implementation; `foo.conformance.test.ts` is not).
30+
name (`@objectstack/<x>`) or repo path (`P`'s directory, e.g.
31+
`packages/services/service-automation`). Over-inclusion is preferred over misses; the
32+
periodic **full** audit (part 4) is the backstop for docs that describe a package
33+
without naming it.
34+
35+
**How a changed file maps to its package:** the package root is the **deepest ancestor
36+
directory with a `package.json`**, resolved from the filesystem — never a hand-kept
37+
list of container directories. (The mapper once special-cased only
38+
`packages/plugins/*`; the 30 packages nested under the other six containers collapsed
39+
into `packages/services` et al., whose missing `package.json` disabled the npm-name
40+
matching arm entirely, so a doc naming `@objectstack/service-automation` but not the
41+
repo path was a guaranteed miss — #4162.) A deleted package falls back to the coarse
42+
`packages/<x>` token, which still substring-matches any doc naming the deleted path.
43+
44+
**Two exclusions:** change classes that cannot make an implementation-accuracy doc
45+
stale are dropped before the changed-package roots are derived:
46+
47+
1. **Test files** (`*.test.*` / `*.spec.*` at any depth, plus `__tests__` /
48+
`__mocks__` / `__fixtures__`): a test observes behaviour rather than defining it —
49+
yet counting them made every tests-only PR light up its packages' whole doc set, a
50+
class of finding that is always false. That is the one place over-inclusion actively
51+
hurt: a comment a reader learns to skip stops working on the PR where it is right.
52+
2. **Package tooling scripts** (`<packageRoot>/scripts/**`): build/verification
53+
tooling, not the runtime behaviour docs describe (#4183 flagged 106 docs for a diff
54+
whose only code change was a new check script). Narrow on purpose: `src/scripts/**`
55+
is runtime code and stays counted, and so does `package.json` — exports/deps
56+
changes ARE implementation. No package publishes runtime code from `scripts/`
57+
(checked against every `files` allowlist; three plugins ship a lone
58+
`i18n-extract.config.ts` only for lack of a `files` field).
59+
60+
The excluded counts are reported in the summary line and as `testFilesSkipped` /
61+
`scriptFilesSkipped` in `--json`, so the narrowing is never silent. `--self-test` pins
62+
the classifiers *and* the package-root derivation against paths that must and must not
63+
match (`commands/test.ts` is implementation; `foo.conformance.test.ts` is not; a
64+
container directory must never come out as a package root).
4365

4466
**And one deliberate non-exclusion:** `packages/*/CHANGELOG.md` stays counted, even though
4567
release notes define behaviour no more than a test does. Extending the exclusion there

0 commit comments

Comments
 (0)