From f73eaf7dc0764ff74fd91b6123f8a94a325f4b44 Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 3 Aug 2026 16:55:27 +0000 Subject: [PATCH] fix(scripts): dead scan roots are a hard error in three more check gates (#4930) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `check-single-authz-resolver`, `check-startup-registry-verdict` and `check-driver-conformance` each concluded from a walk that opened with `try { readdirSync(dir) } catch { return out/[]; }`. A root that was renamed, moved or made unreadable produced zero files, and zero files produce zero findings — the same verdict as a clean workspace. Same shape as #4916/#4934. Each script now resolves every declared root before drawing any conclusion from the scan, and fails naming the root and the reason ("does not exist" / "exists but is not a directory"). The swallows inside the walks are gone too: an error mid-walk means the corpus was only partly read, which must not print as a clean pass either. No whitelist and no `optional: true` flag — every root is a git-tracked directory, so an optional marker would be a supported way to silence the failure instead of fixing the rename. Measured on this tree, pre-fix: only check-single-authz-resolver was genuinely vacuous end to end (it went red by luck, because both DELEGATORS live under the same root, reporting "Delegator missing" for the wrong cause). The other two already failed, but named the wrong cause — startup-registry-verdict's `existsSync` accepted a file and then blamed an empty corpus, and driver-conformance answered a renamed `packages/spec/src/data` with five "CASE_SETS names X, which no longer exports" errors that read as five deliberate deletions. Both now name the directory. The proofs are bidirectional and permanent, not one-offs in the PR body: each `--self-test` renames a root away mid-run and requires red naming that root, replaces one with a file and requires the not-a-directory verdict, dangles a symlink inside the walk and requires the error rather than a smaller corpus, then restores everything and requires green again. check-single-authz-resolver had no self-test and gained one covering both of its original invariants; `package.json` chains `--self-test &&` into `check:authz-resolver` to match the other ten `check:*` entries. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_018iARDqtrhQgz6fVHDeDkbQ --- ...dead-scan-roots-hard-error-three-checks.md | 14 + package.json | 2 +- scripts/check-driver-conformance.mjs | 190 +++++++++-- scripts/check-single-authz-resolver.mjs | 300 +++++++++++++++--- scripts/check-startup-registry-verdict.mjs | 183 ++++++++++- 5 files changed, 616 insertions(+), 73 deletions(-) create mode 100644 .changeset/dead-scan-roots-hard-error-three-checks.md diff --git a/.changeset/dead-scan-roots-hard-error-three-checks.md b/.changeset/dead-scan-roots-hard-error-three-checks.md new file mode 100644 index 0000000000..77a35843aa --- /dev/null +++ b/.changeset/dead-scan-roots-hard-error-three-checks.md @@ -0,0 +1,14 @@ +--- +--- + +Tooling-only: three more `scripts/check-*.mjs` gates now fail, by name, when a declared scan root cannot be resolved (#4930). Releases nothing — no package changes. + +Same shape as #4916/#4934, in three more scripts: a walker that opens with `try { readdirSync(dir) } catch { return out; }` (or `catch { return []; }`) cannot distinguish "this root is clean" from "this root was never opened", because both produce zero findings. Each script now resolves every declared root up front and fails with the root's name and reason ("does not exist" / "exists but is not a directory") before drawing any conclusion from the scan, and the swallows inside the walks are gone — an error mid-walk means the corpus was only partly read, which must not print as a clean pass either. + +- `check-single-authz-resolver.mjs` — `SCAN_ROOTS` (`packages/`). The duplicate-resolver scan was genuinely vacuous on a dead root; the script only went red by luck, because both `DELEGATORS` happen to live under that same root, so it reported "Delegator missing" for the wrong cause. Move a delegator, or add a second scan root, and the vacuous scan goes fully silent. +- `check-startup-registry-verdict.mjs` — the `packages/` scan root. The old `existsSync` guard accepted a *file* and then blamed the empty corpus; and one level in, a directory the walk could not stat shrank the corpus while `files.length` stayed non-zero, so the loud empty-corpus guard never fired. +- `check-driver-conformance.mjs` — `DRIVERS_DIR`, `CASE_SETS_DIR`, and each driver's `src/`. DISCOVERED/CLASSIFIED did catch a dead root, but as a *consequence* naming the wrong cause: a renamed `packages/spec/src/data` printed five "CASE_SETS names X, which no longer exports" errors, which reads as five deliberate deletions rather than one directory that moved. + +No whitelist and no `optional: true` flag in any of the three, deliberately — every one of these roots is a git-tracked directory with tracked files, so no checkout that can run these gates at the repo root is legitimately missing one. An optional marker would be a supported way to silence the failure instead of fixing the rename: the empty `catch {}`, spelled politely. + +Each proof is bidirectional and permanent rather than a one-off in the PR description: every script's `--self-test` now renames a root away mid-run and requires red naming that root (and not the survivors), replaces one with a file and requires the `not a directory` verdict, dangles a symlink inside the walk and requires the error rather than a smaller corpus, then restores everything and requires green again. `check-single-authz-resolver.mjs` had no self-test at all and gained one, with its two original invariants driven over a real temporary tree; `package.json` chains `--self-test &&` into `check:authz-resolver` to match the other ten `check:*` entries. diff --git a/package.json b/package.json index 9cfa23fbb2..5845fdb4f2 100644 --- a/package.json +++ b/package.json @@ -38,7 +38,7 @@ "check:role-word": "node scripts/check-role-word.mjs", "check:adr-anchors": "node scripts/check-adr-anchors.mjs", "check:org-identifier": "node scripts/check-org-identifier.mjs", - "check:authz-resolver": "node scripts/check-single-authz-resolver.mjs", + "check:authz-resolver": "node scripts/check-single-authz-resolver.mjs --self-test && node scripts/check-single-authz-resolver.mjs", "check:slot-lookup": "node scripts/check-slot-lookup-ratchet.mjs", "check:service-providers": "node scripts/check-service-providers.mjs", "check:route-envelope": "node scripts/check-route-envelope.mjs --self-test && node scripts/check-route-envelope.mjs", diff --git a/scripts/check-driver-conformance.mjs b/scripts/check-driver-conformance.mjs index 1dad5438fe..2582badb6e 100644 --- a/scripts/check-driver-conformance.mjs +++ b/scripts/check-driver-conformance.mjs @@ -70,8 +70,26 @@ // Every entry was measured against `main`. To clear one: write the suite, then // delete the entry in the same PR. Deleting without the suite fails CONSUMED; // keeping the entry alongside the suite fails RECONCILED. +// +// ## Dead scan roots are a hard error (#4930) +// +// Both axes of the matrix are read off disk, from two declared directories: +// DRIVERS_DIR and CASE_SETS_DIR. `listDir` used to be +// `try { return readdirSync(dir); } catch { return []; }`, so a root that was +// renamed, moved or made unreadable simply produced an empty axis. DISCOVERED +// and CLASSIFIED do catch that today — but they catch it as a *consequence*, +// and they name the wrong cause: a renamed `packages/spec/src/data` reports five +// separate "CASE_SETS names X, which no longer exports" errors, which +// reads as five deliberate deletions rather than one directory that moved. The +// author's next action follows the message, so the message has to be the cause. +// +// Both roots are therefore resolved before anything is discovered, and a dead +// one fails BY NAME up front. The `listDir` swallow is gone with them: an error +// during a walk means the corpus was only partly read, and partial evidence of +// coverage is exactly the wrong thing to resolve in coverage's favour. +// Deliberately no whitelist and no optional-root flag — see `assertRootsResolvable`. -import { mkdirSync, readdirSync, readFileSync, rmSync, statSync, writeFileSync } from 'node:fs'; +import { mkdirSync, readdirSync, readFileSync, rmSync, statSync, symlinkSync, writeFileSync } from 'node:fs'; import { join, dirname } from 'node:path'; import { fileURLToPath } from 'node:url'; @@ -144,13 +162,58 @@ const LEDGER = []; // ── Discovery ─────────────────────────────────────────────────────────────── -const listDir = (dir) => { - try { - return readdirSync(dir); - } catch { - return []; +/** A declared scan root that could not be resolved to a directory. Carries the names. */ +class DeadRootError extends Error { + constructor(dead) { + super(`unresolvable scan root(s): ${dead.map((d) => `${d.root} — ${d.reason}`).join('; ')}`); + this.name = 'DeadRootError'; + this.dead = dead; + /** @type {string[]} just the root paths, for callers that only need to point. */ + this.roots = dead.map((d) => d.root); } -}; +} + +/** + * Resolve every declared scan root before discovering anything; throw naming the + * ones that are not directories. + * + * Deliberately no whitelist and no `optional: true` marker. `packages/plugins`, + * `packages/spec/src/data` and every driver's `src/` are git-tracked directories + * with tracked files in them, so any checkout that can run + * `pnpm check:driver-conformance` has all of them. An optional marker "just in + * case" would hand the next author a supported way to silence this failure + * instead of fixing the rename — the empty `catch { return []; }` again, only + * spelled politely. If a root ever does become legitimately absent, that is a real + * decision: record it with its condition and a test, don't relax the check. + * + * @throws {DeadRootError} + */ +function assertRootsResolvable(roots) { + const dead = []; + for (const root of roots) { + let st = null; + try { + st = statSync(root); + } catch (err) { + dead.push({ + root, + reason: err?.code === 'ENOENT' ? 'does not exist' : `cannot be read (${err?.code ?? err})`, + }); + continue; + } + if (!st.isDirectory()) dead.push({ root, reason: 'exists but is not a directory' }); + } + if (dead.length) throw new DeadRootError(dead); +} + +/** + * The entries of a directory the caller has already asserted is a scan root. + * + * No catch: an unresolvable root fails loudly in `assertRootsResolvable`, and an + * error here means the axis was only partly read — which must not resolve in + * coverage's favour (#4930). + */ +const listDir = (dir) => readdirSync(dir); /** Driver packages, from disk — never a hardcoded list. */ function discoverDrivers() { @@ -201,18 +264,26 @@ function discoverCaseSets() { return found; } -/** Every `.ts` file under a directory, recursively. */ +/** + * Every `.ts` file under a directory, recursively. + * + * A driver's `src/` is a scan root like the other two: "this driver does not run + * the shared cases" must mean the files were read and the marker was absent, never + * that the directory could not be opened. So it is asserted, and nothing in the + * walk is swallowed (#4930). + */ function walkTs(dir, out = []) { - for (const entry of listDir(dir)) { + assertRootsResolvable([dir]); + walkTsInto(dir, out); + return out; +} + +function walkTsInto(dir, out) { + for (const entry of readdirSync(dir)) { if (entry === 'node_modules' || entry === 'dist') continue; const full = join(dir, entry); - let s; - try { - s = statSync(full); - } catch { - continue; - } - if (s.isDirectory()) walkTs(full, out); + const s = statSync(full); + if (s.isDirectory()) walkTsInto(full, out); else if (entry.endsWith('.ts')) out.push(full); } return out; @@ -243,6 +314,11 @@ function consumes(driverDir, marker) { // ── The run ───────────────────────────────────────────────────────────────── function audit() { + // Both axes come off disk, so both roots must resolve before a single cell of + // the matrix is believed. Throws DeadRootError — `report()` turns it into a red + // that names the directory rather than the five downstream symptoms (#4930). + assertRootsResolvable([DRIVERS_DIR, CASE_SETS_DIR]); + const drivers = discoverDrivers(); const errors = []; const rows = []; @@ -311,8 +387,30 @@ function audit() { return { drivers, rows, errors }; } +function reportDeadRoots(err) { + console.error('\n x check-driver-conformance: declared scan root(s) do not resolve, so the matrix would\n' + + ' have been built from an axis nothing could read:\n'); + for (const d of err.dead) console.error(` ${d.root.startsWith(ROOT) ? d.root.slice(ROOT.length + 1) : d.root} — ${d.reason}`); + console.error( + '\n DRIVERS_DIR and CASE_SETS_DIR (scripts/check-driver-conformance.mjs) must both be' + + '\n directories in the checkout. If one was renamed or moved, point the constant at it; if it' + + '\n was deleted, that is a deliberate decision to record. Do NOT restore a tolerant skip: this' + + '\n used to be `catch { return []; }`, and a dead root produced an empty axis whose downstream' + + '\n errors named the wrong cause (#4930).\n', + ); +} + function report() { - const { drivers, rows, errors } = audit(); + let audited; + try { + audited = audit(); + } catch (err) { + if (!(err instanceof DeadRootError)) throw err; + reportDeadRoots(err); + process.exit(1); + return; + } + const { drivers, rows, errors } = audited; const covered = rows.filter((r) => r.state === 'covered').length; const debt = rows.filter((r) => r.state === 'debt').length; @@ -410,12 +508,68 @@ function selfTest() { expect('a discovery that found something is not', discoveredErrors(['driver-anything']).length === 0); expect('discovers driver packages from disk', discoverDrivers().length > 0); + // --- Reverse proof for the dead-root hard error (#4930), made permanent. --- + // Everything above ran over roots that resolve, which proves nothing about a + // gate whose failure mode is discovering an empty axis. So break a root the way + // a rename breaks it, require red naming that root and not the survivor, then + // restore it and require green again. Red-then-green, in the same run, every run. + const tmpRoots = join(ROOT, 'node_modules', '.check-driver-conformance-selftest-roots'); + try { + mkdirSync(join(tmpRoots, 'live'), { recursive: true }); + const missing = join(tmpRoots, 'renamed-away'); + let deadErr = null; + try { assertRootsResolvable([join(tmpRoots, 'live'), missing]); } catch (err) { deadErr = err; } + expect('a renamed scan root throws instead of yielding an empty axis', deadErr instanceof DeadRootError); + expect('the failure names the dead root', deadErr?.roots?.join(',') === missing); + expect('the failure does not blame the surviving root', !/live/.test(deadErr?.message ?? '')); + expect('the failure says why', deadErr?.dead?.[0]?.reason === 'does not exist'); + + // A root that exists but is not a directory is dead in the same way: the old + // `catch { return []; }` swallowed its ENOTDIR exactly as it swallowed ENOENT. + const asFile = join(tmpRoots, 'a-file'); + writeFileSync(asFile, 'not a directory'); + let notDirErr = null; + try { assertRootsResolvable([asFile]); } catch (err) { notDirErr = err; } + expect('a scan root that is a file is dead too', + notDirErr?.dead?.[0]?.reason === 'exists but is not a directory'); + + // An entry the walk cannot stat inside a driver's src/ is the same defect one + // level in: `catch { continue; }` used to drop it, and a dropped file that + // held the marker reads as "this driver does not run the case-set". + mkdirSync(join(tmpRoots, 'pkg', 'src'), { recursive: true }); + writeFileSync(join(tmpRoots, 'pkg', 'src', 'a.ts'), 'export const a = 1;\n'); + expect('a readable src/ walks clean', walkTs(join(tmpRoots, 'pkg', 'src')).length === 1); + symlinkSync(join(tmpRoots, 'no-such-target'), join(tmpRoots, 'pkg', 'src', 'dangling')); + let partialErr = null; + try { walkTs(join(tmpRoots, 'pkg', 'src')); } catch (err) { partialErr = err; } + expect('an entry the walk cannot stat is an error, not a smaller corpus', partialErr?.code === 'ENOENT'); + rmSync(join(tmpRoots, 'pkg', 'src', 'dangling')); + + // ...and roots that resolve are green, so the reds above were caused by the + // broken roots and nothing else. + let restored = null; + try { assertRootsResolvable([join(tmpRoots, 'live'), join(tmpRoots, 'pkg', 'src')]); } catch (err) { restored = err; } + expect('roots that resolve raise nothing', restored === null); + expect('restoring the tree makes the walk green again', walkTs(join(tmpRoots, 'pkg', 'src')).length === 1); + + // The real roots this gate runs against resolve — the assertion is wired in, + // not merely defined. + let realErr = null; + try { assertRootsResolvable([DRIVERS_DIR, CASE_SETS_DIR]); } catch (err) { realErr = err; } + expect('the real DRIVERS_DIR and CASE_SETS_DIR both resolve', realErr === null); + } finally { + rmSync(tmpRoots, { recursive: true, force: true }); + } + if (failures.length) { for (const f of failures) console.error(` x self-test: ${f}`); console.error(`\ncheck-driver-conformance --self-test: ${failures.length} failure(s).\n`); process.exit(1); } - console.log('OK self-test: detects driven / unused / re-declared fixtures, and discovers both axes.'); + console.log( + 'OK self-test: detects driven / unused / re-declared fixtures, discovers both axes, and holds the ' + + 'dead-root hard error (red when a scan root is renamed, green when restored).', + ); } if (process.argv.includes('--self-test')) selfTest(); diff --git a/scripts/check-single-authz-resolver.mjs b/scripts/check-single-authz-resolver.mjs index 751a81c310..b4021e53d8 100644 --- a/scripts/check-single-authz-resolver.mjs +++ b/scripts/check-single-authz-resolver.mjs @@ -13,9 +13,42 @@ // and `sys_user_permission_set` is doing request-context role+permission // aggregation — the resolver's job — and must be the canonical module (or an // explicitly allow-listed non-resolver, e.g. seed definitions). +// +// node scripts/check-single-authz-resolver.mjs +// node scripts/check-single-authz-resolver.mjs --self-test +// +// ## Dead scan roots are a hard error (#4930) +// +// Check (1) is a *scan*: it concludes "no duplicate resolver exists" from having +// read every `.ts` under SCAN_ROOTS. `walk()` used to open with +// `try { entries = readdirSync(dir); } catch { return out; }`, so a root that was +// renamed, moved or made unreadable produced zero files — and zero files produce +// zero errors, which is character-for-character the same verdict as a clean +// workspace. The scan cannot tell you it never ran; only its (unprinted) file +// count could, and nobody reads a count that is not printed. +// +// Today a whole-`packages/` rename happens to be caught downstream, because both +// DELEGATORS live under that same root and check (2) reports them missing. That +// is luck, not coverage, and it misdirects the diagnosis: the operator is told +// two files are missing when the actual event is that the duplicate-resolver +// scan read nothing at all. Move either delegator out of `packages/`, or add a +// second scan root, and the vacuous scan goes fully silent — the #4916 shape, +// one refactor away. So the roots are resolved up front and a dead one fails the +// gate BY NAME, before any conclusion is drawn from the scan. +// +// Deliberately no whitelist and no `optional: true` marker. `packages/` is a +// git-tracked directory with tracked files; no checkout that can run +// `pnpm check:authz-resolver` at the repo root is legitimately missing it. An +// optional marker "just in case" is a supported way to silence this failure +// instead of fixing the rename — the empty `catch {}` again, 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. -import { readFileSync, readdirSync, statSync } from 'node:fs'; -import { join } from 'node:path'; +import { + mkdirSync, mkdtempSync, readFileSync, readdirSync, renameSync, rmSync, statSync, symlinkSync, writeFileSync, +} from 'node:fs'; +import { tmpdir } from 'node:os'; +import { dirname, join } from 'node:path'; const ROOT = process.cwd(); const CANONICAL = 'packages/core/src/security/resolve-authz-context.ts'; @@ -33,53 +66,246 @@ const DELEGATORS = [ 'packages/runtime/src/security/resolve-execution-context.ts', ]; +// Every directory check (1) claims to have read. Relative to the repo root. +const SCAN_ROOTS = ['packages']; + +const SKIP_DIRS = new Set(['node_modules', 'dist', '__tests__']); + +/** A declared scan root that could not be resolved to a directory. Carries the names. */ +class DeadRootError extends Error { + constructor(dead) { + super(`unresolvable scan 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 scan root before reading anything; throw naming the ones + * that are not directories. See the header for why there is no optional-root flag. + * + * @throws {DeadRootError} + */ +function assertRootsResolvable(root = ROOT, roots = SCAN_ROOTS) { + const dead = []; + for (const rel of roots) { + let st = null; + try { + st = statSync(join(root, rel)); + } catch (err) { + dead.push({ + root: rel, + reason: err?.code === 'ENOENT' ? 'does not exist' : `cannot be read (${err?.code ?? err})`, + }); + continue; + } + if (!st.isDirectory()) dead.push({ root: rel, reason: 'exists but is not a directory' }); + } + if (dead.length) throw new DeadRootError(dead); +} + +/** + * Every non-test `.ts` file under `dir`, recursively. + * + * Nothing here is wrapped in a catch: an unresolvable root fails loudly above, and + * an error *inside* the walk (a vanished file, a permission fault) means the corpus + * was only partly read — which must not be reported as "no duplicate resolver". + */ function walk(dir, out = []) { - let entries; - try { entries = readdirSync(dir); } catch { return out; } - for (const e of entries) { - if (e === 'node_modules' || e === 'dist' || e === '__tests__') continue; + for (const e of readdirSync(dir)) { + if (SKIP_DIRS.has(e)) continue; const p = join(dir, e); - let st; - try { st = statSync(p); } catch { continue; } + const st = statSync(p); if (st.isDirectory()) walk(p, out); else if (e.endsWith('.ts') && !e.endsWith('.test.ts') && !e.endsWith('.d.ts')) out.push(p); } return out; } -const errors = []; - -// (1) No duplicate request-context resolver. -for (const abs of walk(join(ROOT, 'packages'))) { - const rel = abs.slice(ROOT.length + 1); - if (ALLOW.has(rel)) continue; - const src = readFileSync(abs, 'utf8'); - if (src.includes('sys_user_role') && src.includes('sys_user_permission_set')) { - errors.push( - `Possible duplicate authorization resolver: ${rel}\n` + - ` references BOTH sys_user_role and sys_user_permission_set. Request-context\n` + - ` role/permission resolution must live ONLY in ${CANONICAL} (resolveAuthzContext),\n` + - ` shared by every transport. If this file needs both for a non-resolution reason,\n` + - ` add it to ALLOW in scripts/check-single-authz-resolver.mjs.`, - ); +/** The corpus check (1) reasons over. Throws {@link DeadRootError} if a root is dead. */ +function collectScanFiles(root = ROOT, roots = SCAN_ROOTS) { + assertRootsResolvable(root, roots); + const out = []; + for (const rel of roots) walk(join(root, rel), out); + return out; +} + +/** + * Both invariants over `root`. Throws {@link DeadRootError} rather than returning a + * verdict when the scan could not read what it claims to have read. + */ +function audit(root = ROOT) { + const errors = []; + + // (1) No duplicate request-context resolver. + for (const abs of collectScanFiles(root)) { + const rel = abs.slice(root.length + 1); + if (ALLOW.has(rel)) continue; + const src = readFileSync(abs, 'utf8'); + if (src.includes('sys_user_role') && src.includes('sys_user_permission_set')) { + errors.push( + `Possible duplicate authorization resolver: ${rel}\n` + + ` references BOTH sys_user_role and sys_user_permission_set. Request-context\n` + + ` role/permission resolution must live ONLY in ${CANONICAL} (resolveAuthzContext),\n` + + ` shared by every transport. If this file needs both for a non-resolution reason,\n` + + ` add it to ALLOW in scripts/check-single-authz-resolver.mjs.`, + ); + } + } + + // (2) Entry points still delegate to the shared resolver. + for (const rel of DELEGATORS) { + let src; + try { src = readFileSync(join(root, rel), 'utf8'); } catch { errors.push(`Delegator missing: ${rel}`); continue; } + if (!src.includes('resolveAuthzContext')) { + errors.push( + `${rel} no longer delegates to resolveAuthzContext.\n` + + ` Every HTTP entry point must resolve authorization via the shared\n` + + ` @objectstack/core resolver — do not re-inline session/role/permission reads.`, + ); + } } + + return errors; } -// (2) Entry points still delegate to the shared resolver. -for (const rel of DELEGATORS) { - let src; - try { src = readFileSync(join(ROOT, rel), 'utf8'); } catch { errors.push(`Delegator missing: ${rel}`); continue; } - if (!src.includes('resolveAuthzContext')) { - errors.push( - `${rel} no longer delegates to resolveAuthzContext.\n` + - ` Every HTTP entry point must resolve authorization via the shared\n` + - ` @objectstack/core resolver — do not re-inline session/role/permission reads.`, - ); +function reportDeadRoots(err) { + console.error('\n✗ check:authz-resolver: declared scan root(s) do not resolve, so the duplicate-resolver\n' + + ' scan would have concluded "none found" from zero files:\n'); + for (const d of err.dead) console.error(` ${d.root} — ${d.reason}`); + console.error( + `\nEvery entry in SCAN_ROOTS (scripts/check-single-authz-resolver.mjs) must be a directory in` + + `\nthe checkout, and this check runs from the repo root. If a directory was renamed or moved,` + + `\nupdate SCAN_ROOTS to follow it; if it was deleted, remove the entry deliberately. Do NOT` + + `\nrestore a tolerant skip: this used to be \`catch { return out; }\`, and a dead root simply` + + `\nmade the scan read zero files while the gate kept printing green (#4930).\n`, + ); +} + +// ── Self-test ─────────────────────────────────────────────────────────────── +// +// A guard that cannot fail is not a guard. Both invariants are driven over a real +// temporary tree with the real walker, and the dead-root failure is proved in both +// directions — red when a root is renamed away, green when it is restored. + +function selfTest() { + const failures = []; + const expect = (label, got, want) => { + if (got !== want) failures.push(` ✗ ${label}: expected ${JSON.stringify(want)}, got ${JSON.stringify(got)}`); + }; + + const dir = mkdtempSync(join(tmpdir(), 'check-authz-resolver-selftest-')); + const write = (rel, body) => { + mkdirSync(join(dir, dirname(rel)), { recursive: true }); + writeFileSync(join(dir, rel), body); + }; + try { + // A tree that mirrors the real one: the canonical resolver, an allow-listed + // seed, two delegating entry points, and one innocent file. + write(CANONICAL, "sys_user_role sys_user_permission_set\n"); + write('packages/plugins/plugin-security/src/objects/default-permission-sets.ts', + "sys_user_role sys_user_permission_set\n"); + for (const d of DELEGATORS) write(d, "import { resolveAuthzContext } from '@objectstack/core';\n"); + write('packages/core/src/unrelated.ts', 'export const x = 1;\n'); + + expect('a compliant tree passes', audit(dir).length, 0); + + // (1) — a second file reading both role tables is a duplicate resolver. + write('packages/rest/src/my-own-resolver.ts', "sys_user_role sys_user_permission_set\n"); + const dupErrors = audit(dir); + expect('a duplicate resolver is flagged', dupErrors.length, 1); + expect('the duplicate is named', + dupErrors[0]?.startsWith('Possible duplicate authorization resolver: packages/rest/src/my-own-resolver.ts'), true); + rmSync(join(dir, 'packages/rest/src/my-own-resolver.ts')); + + // (1) — the walker must not report test/type files or skipped directories. + write('packages/rest/src/__tests__/fake.ts', "sys_user_role sys_user_permission_set\n"); + write('packages/rest/src/x.test.ts', "sys_user_role sys_user_permission_set\n"); + write('packages/rest/dist/x.ts', "sys_user_role sys_user_permission_set\n"); + expect('tests, .d.ts and dist/ are out of scope', audit(dir).length, 0); + + // (2) — an entry point that stops delegating. + write(DELEGATORS[0], '// re-inlined the session/role reads here\n'); + const delErrors = audit(dir); + expect('a non-delegating entry point is flagged', delErrors.length, 1); + expect('the entry point is named', delErrors[0]?.startsWith(`${DELEGATORS[0]} no longer delegates`), true); + write(DELEGATORS[0], "import { resolveAuthzContext } from '@objectstack/core';\n"); + expect('restoring the delegation clears it', audit(dir).length, 0); + + // --- Reverse proof for the dead-root hard error (#4930), made permanent. --- + // Everything above ran green over a tree whose scan root resolves. That + // observation is worth nothing on its own: the defect being fixed here is a + // scan that concludes "clean" *because* it could not open its root. So break + // the root the way a rename breaks it in the real repo, require red, require + // the red to name the root, then restore it and require green again. + // Red-then-green, in the same run, every run. + const renamed = join(dir, 'packages-renamed-by-self-test'); + renameSync(join(dir, 'packages'), renamed); + let deadErr = null; + try { audit(dir); } catch (err) { deadErr = err; } + expect('a renamed scan root throws instead of quietly scanning nothing', + deadErr instanceof DeadRootError, true); + expect('the failure names the dead root', deadErr?.roots?.join(',') ?? '', 'packages'); + expect('the failure says why', deadErr?.dead?.[0]?.reason ?? '', 'does not exist'); + + // A root that exists but is not a directory is dead in the same way: the old + // `catch { return out; }` swallowed its ENOTDIR exactly as it swallowed ENOENT. + writeFileSync(join(dir, 'packages'), 'not a directory'); + let notDirErr = null; + try { audit(dir); } catch (err) { notDirErr = err; } + expect('a scan root that is a file is dead too', + notDirErr?.dead?.[0]?.reason ?? '', 'exists but is not a directory'); + + rmSync(join(dir, 'packages')); + renameSync(renamed, join(dir, 'packages')); + + // An entry the walk cannot stat INSIDE the root is the same defect one level + // in: `catch { continue; }` used to drop it and carry on, so the scan reached + // a "no duplicate resolver" verdict over files it never opened. A dangling + // symlink is that case, deterministically. + symlinkSync(join(dir, 'no-such-target'), join(dir, 'packages', 'dangling')); + let partialErr = null; + try { audit(dir); } catch (err) { partialErr = err; } + expect('an entry the walk cannot stat is an error, not a smaller corpus', partialErr?.code, 'ENOENT'); + rmSync(join(dir, 'packages', 'dangling')); + + // ...and restoring the tree restores the green, so the reds above were caused + // by the broken root and nothing else. + expect('restoring the root makes the audit green again', audit(dir).length, 0); + } finally { + rmSync(dir, { recursive: true, force: true }); + } + + if (failures.length) { + console.error(`\n✗ check-single-authz-resolver self-test failed:\n${failures.join('\n')}\n`); + process.exit(1); } + console.log( + '✓ check-single-authz-resolver self-test: duplicate detection, delegation, and the dead-root ' + + 'hard error (red when the scan root is renamed, green when restored) all hold.', + ); } -if (errors.length) { - console.error('✗ check:authz-resolver failed:\n\n' + errors.join('\n\n') + '\n'); - process.exit(1); +function main() { + if (process.argv.includes('--self-test')) return selfTest(); + + let errors; + try { + errors = audit(); + } catch (err) { + if (!(err instanceof DeadRootError)) throw err; + reportDeadRoots(err); + process.exit(1); + return; + } + + if (errors.length) { + console.error('✗ check:authz-resolver failed:\n\n' + errors.join('\n\n') + '\n'); + process.exit(1); + } + console.log('✓ check:authz-resolver: single shared authorization resolver intact; both entry points delegate.'); } -console.log('✓ check:authz-resolver: single shared authorization resolver intact; both entry points delegate.'); + +main(); diff --git a/scripts/check-startup-registry-verdict.mjs b/scripts/check-startup-registry-verdict.mjs index a98e67b9eb..eaf57b8afa 100644 --- a/scripts/check-startup-registry-verdict.mjs +++ b/scripts/check-startup-registry-verdict.mjs @@ -119,9 +119,33 @@ * it at a checkout of a pre-fix commit and the gate must REPORT; point it at * `main` and it must be clean. A gate that has only ever been green is * indistinguishable from a gate that matches nothing (#4690). + * + * ## Dead scan roots are a hard error (#4930) + * + * This audit's verdict — "none recording a verdict the boot can contradict" — + * is drawn from having read every `.ts` under the scan root. `collectSourceFiles` + * used to open with `try { entries = readdirSync(dir); } catch { return out; }`, + * so an unreadable directory contributed zero files and the walk carried on. At + * the root that was caught downstream (`files.length === 0` refuses to report + * success), but one level in it was not: a subdirectory that cannot be read + * silently shrinks the corpus while `files.length` stays comfortably non-zero, + * and the gate prints a green line over a scan that never opened part of its + * subject. That is the shape this script's own header spends a screen warning + * about, turned on the script itself. + * + * So the scan root is now resolved up front by `assertRootsResolvable`, which + * fails BY NAME and distinguishes "does not exist" from "exists but is not a + * directory" (the old `existsSync` accepted a file and then blamed the empty + * result), and the walk carries no catch at all: an error during it means the + * corpus was only partly read, which must not be reported as a clean audit. + * Deliberately no optional-root flag — see `assertRootsResolvable`. */ -import { readFileSync, readdirSync, statSync, existsSync } from 'node:fs'; +import { + existsSync, mkdirSync, mkdtempSync, readFileSync, readdirSync, renameSync, rmSync, statSync, symlinkSync, + writeFileSync, +} from 'node:fs'; +import { tmpdir } from 'node:os'; import { join, relative, sep, resolve } from 'node:path'; import { fileURLToPath } from 'node:url'; import ts from 'typescript'; @@ -214,22 +238,63 @@ const SKIP_DIRS = new Set(['node_modules', 'dist', 'build', '.git', '.turbo', 'c // ── Generic AST helpers ────────────────────────────────────────────────────── -function collectSourceFiles(dir, out = []) { - let entries; - try { - entries = readdirSync(dir); - } catch { - return out; +/** A declared scan root that could not be resolved to a directory. Carries the names. */ +class DeadRootError extends Error { + constructor(dead) { + super(`unresolvable scan root(s): ${dead.map((d) => `${d.root} — ${d.reason}`).join('; ')}`); + this.name = 'DeadRootError'; + this.dead = dead; + /** @type {string[]} just the root paths, for callers that only need to point. */ + this.roots = dead.map((d) => d.root); } - for (const entry of entries) { - if (SKIP_DIRS.has(entry)) continue; - const full = join(dir, entry); - let st; +} + +/** + * Resolve every declared scan root before reading anything; throw naming the ones + * that are not directories. + * + * Deliberately no whitelist and no `optional: true` marker. The scan root is a + * git-tracked directory (or a path the operator passed to `--packages-dir` and is + * therefore asserting exists); no checkout that can run this gate is legitimately + * missing it. An optional marker "just in case" would hand the next author a + * supported way to silence this failure 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: record it with its condition and a + * test, don't relax the check. + * + * @throws {DeadRootError} + */ +function assertRootsResolvable(roots) { + const dead = []; + for (const root of roots) { + let st = null; try { - st = statSync(full); - } catch { + st = statSync(root); + } catch (err) { + dead.push({ + root, + reason: err?.code === 'ENOENT' ? 'does not exist' : `cannot be read (${err?.code ?? err})`, + }); continue; } + if (!st.isDirectory()) dead.push({ root, reason: 'exists but is not a directory' }); + } + if (dead.length) throw new DeadRootError(dead); +} + +/** + * Every auditable `.ts` file under `dir`, recursively. + * + * Nothing here is wrapped in a catch: an unresolvable root fails loudly in + * `assertRootsResolvable`, and an error *inside* the walk (a vanished file, a + * permission fault) means the corpus was only partly read — which must not be + * reported as a clean audit either (#4930). + */ +function collectSourceFiles(dir, out = []) { + for (const entry of readdirSync(dir)) { + if (SKIP_DIRS.has(entry)) continue; + const full = join(dir, entry); + const st = statSync(full); if (st.isDirectory()) { collectSourceFiles(full, out); } else if ( @@ -839,11 +904,28 @@ function baselineKey(f) { function run({ list = false, packagesDir } = {}) { const scanRoot = packagesDir ? resolve(packagesDir) : join(ROOT, 'packages'); const relBase = packagesDir ? resolve(packagesDir, '..') : ROOT; - if (!existsSync(scanRoot)) { - console.error(`✗ startup-registry-verdict: nothing to scan — ${scanRoot} does not exist.`); + // The scan root must resolve BEFORE anything is concluded from the scan. The + // old guard was `existsSync`, which accepts a file: the run then fell through + // to `files.length === 0` and blamed an empty corpus for what is really a dead + // root, naming the wrong cause on the one line the operator reads (#4930). + let files; + try { + assertRootsResolvable([scanRoot]); + files = collectSourceFiles(scanRoot); + } catch (err) { + if (!(err instanceof DeadRootError)) throw err; + console.error('\n✗ startup-registry-verdict: the scan root does not resolve, so the audit would have\n' + + ' reported a verdict over a corpus it never opened:\n'); + for (const d of err.dead) console.error(` ${d.root} — ${d.reason}`); + console.error( + '\nThis check scans the workspace `packages/` tree (override with --packages-dir

). If the' + + '\ndirectory was renamed or moved, point the check at it; if it was deleted, that is a' + + '\ndeliberate decision to record. Do NOT restore a tolerant skip: the walk used to open with' + + '\n`catch { return out; }`, which turned an unreadable directory into a silently smaller' + + '\ncorpus (#4930).\n', + ); return 1; } - const files = collectSourceFiles(scanRoot); if (files.length === 0) { // "Absence must be loud" — a scan that found no input must never exit 0 // and read as a pass (#4690). @@ -1277,11 +1359,78 @@ function selfTest() { console.log(` ✓ ${c.name}`); } } + // --- Reverse proof for the dead-root hard error (#4930), made permanent. --- + // Every case above ran green over source text held in memory, which says + // nothing about whether the scan can reach the source text on disk. The defect + // fixed here is an audit that reports a clean verdict *because* it could not + // open (part of) its root. So drive the real walker over a real temporary + // tree: require green, break the root the way a rename breaks it, require red + // naming that root, then restore it and require the same green again. + const expectRoot = (label, got, want) => { + if (got !== want) { + failures++; + console.error(` ✗ ${label}: expected ${JSON.stringify(want)}, got ${JSON.stringify(got)}`); + } else { + console.log(` ✓ ${label}`); + } + }; + const dir = mkdtempSync(join(tmpdir(), 'check-startup-registry-verdict-selftest-')); + try { + mkdirSync(join(dir, 'packages', 'a', 'src'), { recursive: true }); + mkdirSync(join(dir, 'packages', 'b', 'src'), { recursive: true }); + writeFileSync(join(dir, 'packages', 'a', 'src', 'x.ts'), 'export const x = 1;\n'); + writeFileSync(join(dir, 'packages', 'a', 'src', 'x.test.ts'), 'export const t = 1;\n'); + writeFileSync(join(dir, 'packages', 'b', 'src', 'y.ts'), 'export const y = 2;\n'); + const scanRoot = join(dir, 'packages'); + + expectRoot('the walker reads both packages and skips the test file', + collectSourceFiles(scanRoot).length, 2); + + // A root that is renamed away must fail by name, not scan zero files. + const renamed = join(dir, 'packages-renamed-by-self-test'); + renameSync(scanRoot, renamed); + let deadErr = null; + try { assertRootsResolvable([scanRoot]); } catch (err) { deadErr = err; } + expectRoot('a renamed scan root throws instead of quietly scanning nothing', + deadErr instanceof DeadRootError, true); + expectRoot('the failure names the dead root', deadErr?.roots?.join(',') ?? '', scanRoot); + expectRoot('the failure says why', deadErr?.dead?.[0]?.reason ?? '', 'does not exist'); + + // A root that exists but is not a directory is dead in the same way — and + // is exactly what the old `existsSync` guard waved through. + writeFileSync(scanRoot, 'not a directory'); + let notDirErr = null; + try { assertRootsResolvable([scanRoot]); } catch (err) { notDirErr = err; } + expectRoot('a scan root that is a file is dead too', + notDirErr?.dead?.[0]?.reason ?? '', 'exists but is not a directory'); + expectRoot('...and existsSync alone would have accepted it', existsSync(scanRoot), true); + + // An entry the walk cannot stat INSIDE the root is the same defect one + // level in: `catch { continue; }` used to shrink the corpus while + // `files.length` stayed comfortably non-zero, so the loud empty-corpus + // guard never fired and the audit reported a clean verdict over a subject + // it had only partly read. A dangling symlink is that case, deterministically. + rmSync(scanRoot); + renameSync(renamed, scanRoot); + symlinkSync(join(dir, 'no-such-target'), join(scanRoot, 'c')); + let partialErr = null; + try { collectSourceFiles(scanRoot); } catch (err) { partialErr = err; } + expectRoot('an entry the walk cannot stat is an error, not a smaller corpus', + partialErr?.code, 'ENOENT'); + + // ...and restoring the tree restores the green, so the reds above were + // caused by the broken roots and nothing else. + rmSync(join(scanRoot, 'c')); + expectRoot('restoring the tree makes the walk green again', collectSourceFiles(scanRoot).length, 2); + } finally { + rmSync(dir, { recursive: true, force: true }); + } + if (failures > 0) { console.error(`\n✗ self-test: ${failures} case(s) failed\n`); return 1; } - console.log(`\n✓ self-test: ${cases.length} case(s) passed\n`); + console.log(`\n✓ self-test: ${cases.length} analysis case(s) + the dead-root hard error (red when the scan root is renamed, green when restored) all passed\n`); return 0; }