Skip to content

fix(tooling): docs-drift stops flagging docs for tests-only changes - #4091

Merged
os-zhuang merged 2 commits into
mainfrom
claude/console-screen-flow-submit-jfpjy4
Jul 30, 2026
Merged

fix(tooling): docs-drift stops flagging docs for tests-only changes#4091
os-zhuang merged 2 commits into
mainfrom
claude/console-screen-flow-submit-jfpjy4

Conversation

@os-zhuang

@os-zhuang os-zhuang commented Jul 30, 2026

Copy link
Copy Markdown
Contributor

Tooling only, no package changes. Found while working the #4045 line — three of its PRs tripped it in a row.

The false positive

affected-docs.mjs derives changed-package roots from every file under packages/, tests included. So a PR that changes only a test file flags every hand-written doc that names those packages:

PR diff drift comment said
#4064 1 test file + empty changeset 6 packages/services docs
#4078 1 test file + empty changeset 6 packages/services docs
(one before) tests only same 6

A test observes behaviour rather than defining it, so it cannot make an implementation-accuracy doc stale. Every one of those was a guaranteed-false finding.

Why precision matters here specifically

The script's own header says over-inclusion is preferred to misses, and for real code changes that is right — the periodic full audit is the backstop. But a category that is always false is a different thing: a reader who learns the comment is noise stops reading it, and then it fails on the PR where it is correct. Same reason a flaky test is worse than no test.

The change

  • Test files are dropped before the package roots are derived: *.test.* / *.spec.* at any depth (covering .integration.test.ts and .conformance.test.ts), plus anything under __tests__ / __mocks__ / __fixtures__.
  • The narrowing is not silent. The excluded count rides the summary line and testFilesSkipped in --json. A tool that quietly narrows its own scope reads as "nothing to see here" when it means "I did not look".
  • --self-test, following the check-error-code-casing / check-route-envelope convention, run by the workflow before it computes the mapping — so a regression fails loudly instead of quietly emptying the comment.
  • The workflow now also triggers on the mapper's own paths (second commit — see below).

Verified against real history, not by inspection

Ran the patched mapper over the actual commits:

commit content before after
46e86bad (#4078) tests only 6 docs 0 docs, 1 test file(s) excluded
4965bfac (#4059) engine.ts + formula 11 docs 11 docs (unchanged), 1 skipped
--all full audit scope 178 178 (unchanged)

The second row is the one that matters — it proves implementation detection is intact, not just that the noise is gone.

The self-test earns its place

Replacing the anchored match with a naive path.includes('test') — the plausible wrong way to write this — fails 7 cases, including the direction that would turn the optimisation into a miss:

✗ "a zod schema": packages/spec/src/automation/control-flow.zod.ts → expected false, got true
✗ "no false positive on a bare name": packages/spec/src/latest.ts → expected false, got true
✗ "a command NAMED test is not a test file": packages/cli/src/commands/test.ts → expected false, got true
✗ "__mocks__": packages/foo/src/__mocks__/driver.ts → expected true, got false

latest.ts contains test as a substring, which is exactly the sneaky case an unanchored check swallows.

Correction, and a second gap CI surfaced

An earlier revision of this description said this PR's own drift comment "should now report 0 docs, which is itself a live demonstration of the fix". That was wrong. CI on the first push showed no drift comment at all: the workflow triggers on paths: ['packages/**'], and this PR touches only scripts/ and .github/, so Flag docs affected by code changes never ran.

Which surfaced a real gap worth fixing rather than working around — the self-test step had been added but was structurally unreachable by the change most likely to break it. Editing affected-docs.mjs was the one change its own guard could never see.

The second commit adds scripts/docs-audit/** and this workflow file to the trigger. A PR touching only those now runs the self-test and gets the comment step's existing No hand-written docs reference the 0 changed package(s). ✅ branch, which is the correct answer. This PR exercises it.

Not done here

A package's CHANGELOG.md is arguably the same class — changeset releases touch every packages/*/CHANGELOG.md and would light up everything — but those PRs are bot-generated and nobody reads the comment on them, so I left the scope to the observed problem rather than widening it unasked.

There is no test harness for root scripts/ (pnpm test is turbo run test, workspace packages only), which is why this uses the repo's --self-test convention rather than a vitest file.

🤖 Generated with Claude Code

https://claude.ai/code/session_01QoA8AV99Ss1RRkLLAYmDzq

`affected-docs.mjs` derived changed-package roots from every file under `packages/`,
including tests. A test observes behaviour rather than defining it, so it cannot make an
implementation-accuracy doc stale — yet every tests-only PR lit up its packages' whole
doc set. Three in a row did it: #4064 and #4078 each flagged 6 `packages/services` docs
for a diff containing no production code, and one before them the same way.

This is the one place the mapper's deliberate over-inclusion actively hurt. The script's
own header says over-inclusion is preferred to misses, and for real code that is right —
the periodic full audit is the backstop. But a category that is ALWAYS false is
different: a reader who learns the comment is noise stops reading it, and then it fails
on the PR where it is correct. Precision matters here for the same reason a flaky test
is worse than no test.

- Test files are dropped before the package roots are derived: `*.test.*` / `*.spec.*`
  at any depth (so `.integration.test.ts` and `.conformance.test.ts` are covered) plus
  anything under `__tests__` / `__mocks__` / `__fixtures__`.
- The narrowing is NOT silent. The excluded count rides the summary line and
  `testFilesSkipped` in `--json` — a tool that quietly narrows its own scope reads as
  "nothing to see here" when it means "I did not look".
- `--self-test` pins the matcher, following `check-error-code-casing` /
  `check-route-envelope`, and the drift workflow runs it before computing the mapping so
  a regression fails loudly instead of emptying the comment.

Verified against real history rather than by inspection:
- `46e86bad` (#4078, tests only) → 0 docs, was 6, reporting `1 test file(s) excluded`
- `4965bfac` (#4059, engine.ts + formula) → the same 11 docs as before, `1` skipped
- `--all` unchanged at 178 docs

The self-test earns its place too: swapping the anchored match for a naive
`path.includes('test')` fails 7 cases, among them `control-flow.zod.ts` and `latest.ts`
being swallowed as tests — the direction that would turn this optimisation into a miss.

Tooling only; empty changeset since nothing releases.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01QoA8AV99Ss1RRkLLAYmDzq
@vercel

vercel Bot commented Jul 30, 2026

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

1 Skipped Deployment
Project Deployment Actions Updated (UTC)
objectstack Ignored Ignored Jul 30, 2026 9:38am

Request Review

CI on the first push of this PR showed the gap: `Flag docs affected by code changes`
never ran, because the workflow triggers on `paths: ['packages/**']` and this PR
touches only `scripts/` and `.github/`. So the self-test step added one commit ago was
added but never exercised, and editing `affected-docs.mjs` was the one change its own
guard could structurally never see — the tool is unguarded exactly when it is being
modified.

Adds the mapper's own paths to the trigger. A PR touching only these gets the benign
"No hand-written docs reference the 0 changed package(s). ✅" branch the comment step
already has, which is the correct answer, and the self-test now runs on the change most
likely to break it.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01QoA8AV99Ss1RRkLLAYmDzq
@github-actions

Copy link
Copy Markdown
Contributor

📓 Docs Drift Check

No hand-written docs reference the 0 changed package(s). ✅

@os-zhuang
os-zhuang marked this pull request as ready for review July 30, 2026 09:41
@os-zhuang
os-zhuang merged commit 707dae1 into main Jul 30, 2026
16 checks passed
@os-zhuang
os-zhuang deleted the claude/console-screen-flow-submit-jfpjy4 branch July 30, 2026 09:42
os-zhuang added a commit that referenced this pull request Jul 30, 2026
…xclusion (#4136)

#4091 dropped test files before deriving the changed-package roots, because a
test cannot make an implementation-accuracy doc stale. `packages/*/CHANGELOG.md`
is the obvious next candidate by the same argument — release notes define
behaviour no more than a test does — and extending the exclusion there is a
provable no-op. Measured before writing anything down:

- `chore: version packages` is the only PR class that mass-touches those files,
  and it runs no GitHub Actions at all. `changesets/action` opens it with the
  repo's GITHUB_TOKEN, and GitHub does not create workflow runs from
  GITHUB_TOKEN-authored events. #3910 carries exactly one check run, from
  Vercel's own app — `ci.yml` matches that PR unconditionally and still never
  fired. So the drift gate never sees a release PR to be noisy on.
- Even if it did, `changeset version` writes `package.json` beside every
  `CHANGELOG.md` it appends to (45 against 46 on page 1 of #3910's diff), so
  dropping the CHANGELOGs would leave the derived package-root set identical.

The bump is not unverified, just verified later: `ci.yml` and `lint.yml` both run
on `push: main`, and `release.yml` gates publish on a green build — worth stating
so the absent PR checks are not read as a hole.

Recorded in the README beside the exclusion it neighbours, so the next reader
does not spend a PR rediscovering it.


Claude-Session: https://claude.ai/code/session_01QoA8AV99Ss1RRkLLAYmDzq

Co-authored-by: Claude <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ci/cd documentation Improvements or additions to documentation size/m tooling

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants