From 7ca43b52e5cb56c6c174736bf293a679d0edde79 Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 3 Aug 2026 16:31:46 +0000 Subject: [PATCH] fix(tooling): fail check-doc-authoring by name when a declared ROOT is dead (#4916) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `collectFiles()` walked each root inside `try { walk(r, files); } catch {}`. Rename, move or delete any one of `.claude` / `skills` / `content` and its ENOENT was swallowed in place: the scan finished the remaining roots and printed `✓ doc authoring guard: N files clean`, exit 0. Measured on this tree with `.claude/` renamed away, the old code reports 215 files clean, exit 0, where the honest answer is 219 — "all three roots are clean" and "one root was never opened" are the same green line with a smaller N, and nobody reads N. `assertRootsResolvable()` now runs before any walking and throws a `DeadRootError` naming every root that is missing, unreadable, or not a directory; `main()` renders that as a red gate pointing at the dead root. No whitelist and no `optional` flag: all three roots are git-tracked directories with tracked files, so no checkout that can run this gate at the repo root is legitimately missing one, and an optional marker would be a supported way to silence the failure instead of following the rename — the empty catch, spelled politely. The inner try is gone too: an error during the walk also means the corpus was only partly read. The proof is bidirectional and permanent. `--self-test` (#4913) already walked a real temporary tree with the real walker; it now renames one root away mid-run and requires red naming that root and not the survivors, replaces another root with a file and requires the `not a directory` verdict, then restores both and requires green again. This closes what #4913's self-test could not: it stayed green with the repo's real `.claude/` renamed away, because it asserts over its own temp tree. Same discipline as #4690 / #4804 / #4835 / #4851 / #4868 / #4890. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_018iARDqtrhQgz6fVHDeDkbQ --- .../doc-authoring-dead-root-hard-error.md | 10 ++ scripts/check-doc-authoring.mjs | 121 +++++++++++++++++- 2 files changed, 126 insertions(+), 5 deletions(-) create mode 100644 .changeset/doc-authoring-dead-root-hard-error.md diff --git a/.changeset/doc-authoring-dead-root-hard-error.md b/.changeset/doc-authoring-dead-root-hard-error.md new file mode 100644 index 0000000000..c682bf1f83 --- /dev/null +++ b/.changeset/doc-authoring-dead-root-hard-error.md @@ -0,0 +1,10 @@ +--- +--- + +Tooling-only: `scripts/check-doc-authoring.mjs` now fails, by name, when one of its declared `ROOTS` cannot be resolved (#4916). Releases nothing — no package changes. + +The walk was `for (const r of ROOTS) { try { walk(r, files); } catch {} }`. Rename, move or delete any one root and its ENOENT was swallowed in place: the scan finished the *remaining* roots and printed `✓ doc authoring guard: N files clean`, exit 0. Measured on this tree — with `.claude/` renamed away, the old code reported **215 files clean, exit 0** where the honest answer is 219. From outside, "all three roots are clean" and "one root was never opened" are the same green line with a smaller N, and nobody reads N. That is the sixth instance this week of one shape: a check that runs, is green, and structurally cannot reach part of its subject (#4690 / #4804 / #4835 / #4868 / #4890 / #4851). + +`assertRootsResolvable()` now runs before any walking and throws a `DeadRootError` naming every root that is missing, unreadable, or not a directory; `main()` turns that into a red gate that says which root died and tells the author to follow the rename in `ROOTS` rather than restore a tolerant skip. **No whitelist and no `optional: true` flag**, deliberately: `.claude`, `skills` and `content` are all git-tracked directories with tracked files, so no checkout that can run this gate at the repo root is legitimately missing one. An optional marker added "just in case" would be a supported way to silence the failure instead of fixing the rename — the empty `catch {}`, spelled politely. Should a root ever become legitimately absent, that is a decision to record with its condition and a test, not a check to relax. The inner `try` is gone too: an error *during* the walk also means the corpus was only partly read, which must not print as a clean scan. + +The proof is bidirectional and permanent, not a one-off in the PR description. `--self-test` (#4913) already walked a real temporary tree with the real walker; it now also renames one root away mid-run and requires red naming that root and *not* the survivors, replaces another root with a file and requires the `not a directory` verdict, then restores both and requires green again. Observing green proves nothing about a gate whose failure mode is scanning less — so the self-test observes red first, every run. Note what this closes that #4913's self-test could not: the old self-test stayed green with the repo's real `.claude/` renamed away, because it asserts over its own temp tree. diff --git a/scripts/check-doc-authoring.mjs b/scripts/check-doc-authoring.mjs index b044b73d4b..22e8ed746f 100644 --- a/scripts/check-doc-authoring.mjs +++ b/scripts/check-doc-authoring.mjs @@ -26,7 +26,20 @@ // agents themselves read. The root is `.claude`, not `.claude/skills`, so the // next subdirectory added under it is covered on arrival rather than missed the // same way twice. -import { mkdirSync, mkdtempSync, readdirSync, readFileSync, rmSync, statSync, writeFileSync } from 'node:fs'; +// +// ## Dead roots are a hard error (#4916) +// +// `collectFiles()` used to walk each root inside `try { ... } catch {}`. Rename, +// move or delete any one of them and the ENOENT was swallowed in place: the scan +// finished the *remaining* roots and printed `✓ ... N files clean`, exit 0. From +// outside, "all three roots are clean" and "one root was never opened" are the +// same output with a smaller N, and nobody reads N. So every ROOT is now resolved +// at startup and an unresolvable one fails the gate **by name**. There is no +// optional root and no empty catch — see `assertRootsResolvable` for why a +// whitelist would be the wrong shape here rather than merely unnecessary. +import { + mkdirSync, mkdtempSync, readdirSync, readFileSync, renameSync, rmSync, statSync, writeFileSync, +} from 'node:fs'; import { tmpdir } from 'node:os'; import { dirname, join, sep } from 'node:path'; @@ -70,10 +83,60 @@ function walk(dir, out) { } } -/** Every Markdown/MDX file in scope, relative to the current working directory. */ +/** A declared ROOT that could not be resolved to a directory. Carries the names. */ +class DeadRootError extends Error { + constructor(dead) { + super(`unresolvable ROOT(s): ${dead.map((d) => `${d.root} — ${d.reason}`).join('; ')}`); + this.name = 'DeadRootError'; + this.dead = dead; + /** @type {string[]} just the root names, for callers that only need to point. */ + this.roots = dead.map((d) => d.root); + } +} + +/** + * Resolve every declared ROOT before scanning anything; throw naming the ones that + * are not directories. + * + * Deliberately no whitelist / no "optional root" flag. A whitelist is the right + * shape when a root is *legitimately* absent in some checkout form, and none of + * these three are: `.claude`, `skills` and `content` are all git-tracked + * directories with tracked files in them, so any checkout that can run + * `pnpm check:doc-authoring` at the repo root has all three. Adding an optional + * marker "just in case" would hand the next author a supported way to silence this + * failure (`optional: true`) instead of fixing the rename — which is the empty + * `catch {}` again, only spelled politely. If a root ever does become legitimately + * absent, that is a real decision: add the entry *with* its condition and a test, + * don't relax the check. + * + * @throws {DeadRootError} + */ +function assertRootsResolvable(roots = ROOTS) { + const dead = []; + for (const root of roots) { + let stat = null; + try { + stat = statSync(root); + } catch (err) { + dead.push({ root, reason: err?.code === 'ENOENT' ? 'does not exist' : `cannot be read (${err?.code ?? err})` }); + continue; + } + if (!stat.isDirectory()) dead.push({ root, reason: 'exists but is not a directory' }); + } + if (dead.length) throw new DeadRootError(dead); +} + +/** + * Every Markdown/MDX file in scope, relative to the current working directory. + * + * Nothing here is wrapped in a catch: an unreadable root fails loudly above, and an + * error *inside* `walk` (a vanished file, a permission fault) means the corpus was + * only partly read — which must not be reported as a clean scan either. + */ function collectFiles() { + assertRootsResolvable(); const files = []; - for (const r of ROOTS) { try { walk(r, files); } catch {} } + for (const r of ROOTS) walk(r, files); return files; } @@ -151,6 +214,38 @@ function selfTest() { expect('defineX factory form passes', violations.some((v) => v.file === 'skills/legit/SKILL.md'), false); expect('non-ts fence and prose pass', violations.some((v) => v.file === 'content/docs/ui/pages.mdx'), false); expect('total violations', violations.length, 2); + + // --- Reverse proof for the dead-root hard error (#4916), made permanent. --- + // Everything above ran green over a tree where all three roots resolve. That + // observation is worth nothing on its own: the defect being fixed here is a + // gate that goes green *because* it could not reach a root. So break one root + // the way a rename breaks it in the real repo, require red, require the red to + // name the root that died and not the survivors, then restore it and require + // green again. Red-then-green, in the same run, every run. + const renamedRoot = join(dir, '.claude-renamed-by-self-test'); + renameSync(join(dir, '.claude'), renamedRoot); + let deadErr = null; + try { collectFiles(); } catch (err) { deadErr = err; } + renameSync(renamedRoot, join(dir, '.claude')); + + expect('a renamed ROOT throws instead of quietly scanning less', deadErr instanceof DeadRootError, true); + expect('the failure names the dead root', deadErr?.roots?.join(',') ?? '', '.claude'); + expect('the failure does not blame the surviving roots', /skills|content/.test(deadErr?.message ?? ''), false); + + // A ROOT that exists but is not a directory is dead in the same way: the old + // `catch {}` swallowed its ENOTDIR exactly as it swallowed ENOENT. + renameSync(join(dir, 'skills'), join(dir, 'skills-renamed-by-self-test')); + writeFileSync(join(dir, 'skills'), 'not a directory'); + let notDirErr = null; + try { collectFiles(); } catch (err) { notDirErr = err; } + rmSync(join(dir, 'skills')); + renameSync(join(dir, 'skills-renamed-by-self-test'), join(dir, 'skills')); + + expect('a ROOT that is a file is dead too', notDirErr?.dead?.[0]?.reason ?? '', 'exists but is not a directory'); + + // ...and restoring both roots restores the green, so the red above was caused + // by the broken root and nothing else. + expect('restoring the roots makes the scan green again', collectFiles().length, files.length); } finally { process.chdir(cwd); rmSync(dir, { recursive: true, force: true }); @@ -160,13 +255,29 @@ function selfTest() { console.error(`\n✗ check-doc-authoring self-test failed:\n${failures.join('\n')}\n`); process.exit(1); } - console.log('✓ check-doc-authoring self-test: scope wiring (.claude in, .claude/worktrees out) and detection both hold.'); + console.log('✓ check-doc-authoring self-test: scope wiring (.claude in, .claude/worktrees out), detection, and the dead-root hard error (red when a ROOT is renamed, green when restored) all hold.'); } function main() { if (process.argv.includes('--self-test')) return selfTest(); - const files = collectFiles(); + let files; + try { + files = collectFiles(); + } catch (err) { + if (!(err instanceof DeadRootError)) throw err; + console.error(`\n✗ doc authoring guard: declared ROOT(s) do not resolve, so the scan would have been silently narrower:\n`); + for (const d of err.dead) console.error(` ${d.root} — ${d.reason}`); + console.error( + `\nEvery entry in ROOTS (scripts/check-doc-authoring.mjs) must be a directory in the checkout,` + + `\nand this check runs from the repo root. If a corpus directory was renamed or moved, update` + + `\nROOTS to follow it; if it was deleted, remove the entry deliberately. Do NOT restore a` + + `\ntolerant skip: this used to be \`catch {}\`, and a dead root simply shrank the reported file` + + `\ncount while the gate kept printing green (#4916).\n`, + ); + process.exit(1); + return; + } const violations = files.flatMap((file) => findViolations(readFileSync(file, 'utf8'), file)); if (violations.length === 0) {