Skip to content

Commit 04ee9f8

Browse files
os-trumpclaude
andauthored
Gate every in-repo isMissingTableError call on naming the object it read (#14346)
* feat(types): gate every in-repo isMissingTableError call on naming the object it read #13324 gave `isMissingTableError` a `readObject` argument so a driver fault naming a different relation can no longer be answered "this table is not provisioned yet". The parameter had to ship optional -- `@objectstack/types` is published (17.2.0) and re-exported from `@objectstack/metadata/errors`, so a required parameter is a major bump -- which left the in-repo obligation stated only in JSDoc. Prose is exactly what #13324 proved insufficient: a caller that omits the argument silently receives the pre-#13324 wide verdict, and on the authz path that resolves a permission-store outage to `[]` permissions. `driver-error-classification.callers.test.ts` walks every TypeScript source under `packages/` with the TypeScript compiler API and fails any call of the predicate that omits `readObject` or passes it as `undefined`/`null`, naming each site file:line with the remedy. The only exemption is this module's own contract tests, which exercise the published one-argument form on purpose. Two positive controls keep an empty violation set from being indistinguishable from a broken scanner: with the exemption disabled the defining test file must yield a substantial one-argument population (30 today), and the two-argument population outside `packages/types` must be substantial (18 today, across 5 packages). A third check fails if a renamed import binding appears, since the callee matcher is by name. Registration, required by check:cross-package-test-inputs for any test that reads outside its own package: one entry in the declaration table and one turbo task, both scoped to `packages/**`. The repo root is reached by arithmetic off this package's manifest rather than by a marker-file walk precisely to keep that radius inside `packages/**`, which ci.yml's `core:` filter already covers -- so no scheduler change is needed. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016yfqQh2dBgPAymYd7xipza * fix(types): scan .ts only, so the caller gate stops covering a .tsx residue specimen The first shape declared `globs: ['packages/**']`. That entry's globs are inherited as watch hints by `check:cross-package-test-inputs`, and it was the only entry in the table covering a `.tsx` file, which turned one dispatch-gates self-test case red: it pins that no hint of that family reaches the `realtime-hooks.test.tsx` specimen in `packages/client-react` -- the live member of "a test class the hint route cannot reach". Narrowed on both sides at once, which is the only honest way to move either: the scanner now takes `.ts` alone via a named `SOURCE_FILE` predicate, and the declaration and turbo inputs are `packages/**/*.ts` to match. Measured under `packages/` on this commit: 5181 `.ts` tracked and 48 mention the predicate; `.tsx` 8 tracked / 0 mention; `.mts` 17 / 0; `.cts` 0 / 0 -- so the narrowing loses no call site today. `coversDirectory` never applied here: the roster for this package holds exactly one literal and no directory entry, so `packages/**/*.ts` both covers the roster and is held by it. Two pins keep the boundary from decaying into a comment: the extension set is asserted directly (pure predicate, no I/O, so it adds nothing to the declared radius), and a filename-only walk asserts `.tsx` files really exist under `packages/`, so the exclusion stays a decision about a real population. Both the header and the table entry name that specimen file in two halves, because the registrar collects quoted whole paths out of comments -- spelling it in full put it on this package's roster and demanded the very `.tsx` glob the change exists to avoid. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016yfqQh2dBgPAymYd7xipza * docs(types): anchor the extension-boundary reading to a named commit The `.ts` total in that header was measured before this branch merged `origin/main` and read 5181; the tree it now describes has 5193. Only the three ZEROS in that table are load-bearing -- they are what makes "narrowing to `.ts` loses no call site" true -- and the total moves with every merge, so the reading now names the commit it belongs to rather than implying "now". Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016yfqQh2dBgPAymYd7xipza --------- Co-authored-by: Claude <noreply@anthropic.com>
1 parent 2e471e3 commit 04ee9f8

4 files changed

Lines changed: 432 additions & 0 deletions

File tree

Lines changed: 386 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,386 @@
1+
// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license.
2+
3+
/**
4+
* #13440 — every in-repo call of `isMissingTableError` must name the object it
5+
* was reading. This file is the mechanism that makes that true; the JSDoc on
6+
* the function is only the explanation.
7+
*
8+
* ── The defect class ─────────────────────────────────────────────────────────
9+
*
10+
* #13324 repaired the predicate by giving it `readObject`, so a driver fault
11+
* naming a DIFFERENT relation can no longer be answered "this table is not
12+
* provisioned yet". The parameter had to ship OPTIONAL: `@objectstack/types` is
13+
* published (17.2.0, `exports` `.` and `./node`), and re-exported again from
14+
* `@objectstack/metadata/errors`, so a required parameter is a breaking change
15+
* to a published API — a major bump, which is a maintainer's call and not a
16+
* side effect of a bug fix.
17+
*
18+
* Optional is right for the world outside this repo and wrong for the inside of
19+
* it. `isMissingTableError(err)` still compiles, still type-checks, and still
20+
* returns the pre-#13324 WIDE verdict — silently. On the authz path
21+
* (`packages/core/src/security/resolve-authz-context.ts`) that verdict resolves
22+
* a permission-store OUTAGE to `[]` permissions instead of failing loud, so the
23+
* omission fails in the OPEN direction. That is the same declared-but-not-
24+
* enforced shape #13324 existed to close, one level up: the obligation is
25+
* stated in prose, and prose is exactly what #13324 proved insufficient.
26+
*
27+
* ── Why a gate and not a required parameter ──────────────────────────────────
28+
*
29+
* A gate binds only callers inside this repository, so it buys the enforcement
30+
* without the major bump: external consumers keep the optional form the
31+
* published API promises them. Making the parameter required remains available
32+
* as a follow-up and stays a human decision.
33+
*
34+
* ── The exemption axis, and why it is exactly this narrow ────────────────────
35+
*
36+
* `driver-error-classification.test.ts` calls the one-argument form ~30 times
37+
* ON PURPOSE: those are the tests OF the optional form, pinning that
38+
* `isMissingTableError(err)` still behaves for the external consumers the
39+
* optional parameter protects. A gate written to the naive rule would fail
40+
* every one of them, and the obvious "fix" — passing a read object — would
41+
* delete the coverage of the published one-argument contract.
42+
*
43+
* So the exemption is the DEFINING PACKAGE'S OWN CONTRACT TESTS and nothing
44+
* else: `packages/types/src/driver-error-classification*.test.ts`. Everything
45+
* else under `packages/` — production and test code alike — must pass the read
46+
* object. The defining module itself is deliberately NOT exempt: the predicate
47+
* delegates to `matchesDriverError` and never calls itself, so a
48+
* self-referential one-argument call there would be a new fact worth failing on.
49+
*
50+
* ── Why the checks below are not just "green on the current tree" ────────────
51+
*
52+
* A scanner that silently stops matching yields the same empty violation set as
53+
* a clean repo, and the assertion cannot tell them apart. Two positive controls
54+
* separate them, and they fail in different directions:
55+
*
56+
* SEES THE EXEMPT FILE — with the exemption disabled, the defining test file
57+
* must yield a substantial one-argument population
58+
* (30 on the commit this landed). Zero there means the
59+
* call matcher is broken, not that the repo is clean.
60+
* REACHES OTHER PACKAGES — the two-argument production population outside
61+
* `packages/types` must be substantial (18 on the same
62+
* commit). Zero there means the directory walk never
63+
* left the defining package, which is the failure that
64+
* would make the whole gate vacuous.
65+
*
66+
* A third check guards the matcher's one structural blind spot. Callees are
67+
* matched BY NAME, so a renamed import binding
68+
* (`import { isMissingTableError as x }`) would be invisible. None exists today;
69+
* if one appears, this fails and asks for the matcher to be taught about it,
70+
* rather than letting the population quietly shrink.
71+
*
72+
* ── Boundary, stated rather than discovered later ────────────────────────────
73+
*
74+
* The scanned surface is `packages/` — the surface the ruling on #13440 names.
75+
* Measured when this landed, `apps/`, `examples/`, `e2e/` and `scripts/` call
76+
* the predicate zero times in total, so the narrower surface loses nothing
77+
* today; widening it is the `SCANNED_TREE` constant below plus a wider glob in
78+
* `CROSS_PACKAGE_TEST_INPUTS` (and, for a NEW top-level root, a matching entry
79+
* in ci.yml's `crosspkg:` filter — `check-ci-filter-parity.mjs` is the gate
80+
* that says so).
81+
*
82+
* ⚠️ The EXTENSION boundary is `.ts` alone, and unlike the tree above that one
83+
* is not free — it is a deliberate trade with a second gate. Measured under
84+
* `packages/` on f60061a460, which is a NAMED commit rather than "now" on
85+
* purpose: only the three zeros are load-bearing, and the `.ts` total moves
86+
* with every merge, so a reading with no commit on it rots silently.
87+
*
88+
* .ts 5193 tracked, 48 mention the predicate <- the scanned set
89+
* .tsx 8 tracked, 0 mention the predicate <- excluded
90+
* .mts 17 tracked, 0 mention the predicate <- excluded
91+
* .cts 0 tracked, 0 mention the predicate <- excluded
92+
*
93+
* So nothing is lost today. What forbids simply widening it is that this
94+
* package's declared radius is INHERITED as watch hints by
95+
* `check:cross-package-test-inputs`, and the dispatch-gates self-test pins that
96+
* no hint of that family reaches the `realtime-hooks.test.tsx` file in
97+
* `packages/client-react` — the live specimen for "a test class the hint route
98+
* cannot reach". A glob here that covers `.tsx` makes that case fail. It is a
99+
* real red and not a nuisance: the specimen is how that tool proves its residue
100+
* classes are not empty.
101+
*
102+
* (That file is named in two halves rather than as one quoted path on purpose.
103+
* This gate's own registrar collects quoted whole paths out of COMMENTS, so
104+
* spelling it here would put it on this package's roster and demand the very
105+
* `.tsx` glob the paragraph exists to forbid — measured, it fails exactly that
106+
* way.)
107+
*
108+
* ⇒ If a `.tsx` (or `.mts`) caller of this predicate ever appears, widening
109+
* `SOURCE_FILE` below is only HALF the change: the declared glob must widen
110+
* with it, and re-pointing that self-test specimen is a `scripts/pm/` edit
111+
* owned by another lane. Do not widen the scanner alone — that reads as
112+
* coverage while turbo never re-runs this test for the files it now claims to
113+
* judge, which is the #7802 shape the declaration table exists to prevent. The
114+
* two pins below keep this paragraph honest rather than decorative.
115+
*/
116+
117+
import { describe, expect, it } from 'vitest';
118+
import { existsSync, readdirSync, readFileSync } from 'node:fs';
119+
import { dirname, join, relative, resolve, sep } from 'node:path';
120+
import ts from 'typescript';
121+
122+
/**
123+
* This package is CJS-typed (no `"type": "module"`), so `module: NodeNext`
124+
* forbids `import.meta` here — the same constraint `node-isolation.test.ts`
125+
* records. Walk up from the CWD to this package's own manifest instead, which
126+
* works wherever vitest is invoked from.
127+
*/
128+
function findUp(marker: (dir: string) => boolean, what: string): string {
129+
let dir = process.cwd();
130+
for (;;) {
131+
if (marker(dir)) return dir;
132+
const parent = dirname(dir);
133+
if (parent === dir) throw new Error(`could not locate ${what} walking up from ${process.cwd()}`);
134+
dir = parent;
135+
}
136+
}
137+
138+
const PACKAGE_ROOT = findUp((dir) => {
139+
const manifest = join(dir, 'package.json');
140+
if (!existsSync(manifest)) return false;
141+
const { name } = JSON.parse(readFileSync(manifest, 'utf8')) as { name?: string };
142+
return name === '@objectstack/types';
143+
}, 'the @objectstack/types package root');
144+
145+
/**
146+
* The repo root reached by ARITHMETIC from this package rather than by a second
147+
* marker-file walk, and that is deliberate. A walk keyed on a workspace-root
148+
* marker would NAME that root file, which
149+
* `check-cross-package-test-inputs.mjs` then requires this package to declare —
150+
* and a declared root-level path is a top-level root that
151+
* `check-ci-filter-parity.mjs` in turn requires in ci.yml's `crosspkg:` filter.
152+
* (Both gates named bare rather than by path on purpose: the first one's
153+
* literal collector takes quoted whole paths out of COMMENTS too, so spelling
154+
* one here would force this package to declare a radius it never reads.)
155+
* Anchoring off the manifest keeps this gate's whole declared radius inside
156+
* `packages/**`, which ci.yml's `core:` filter already covers, so the gate costs
157+
* one table entry and one turbo task and no scheduler surgery.
158+
*
159+
* The arithmetic is not trusted on faith: the anchor test below requires the
160+
* walk to find this package's OWN defining module, which no wrong root can
161+
* satisfy.
162+
*/
163+
const REPO_ROOT = resolve(PACKAGE_ROOT, '../..');
164+
165+
/** The tree this gate binds. See the boundary note in the header. */
166+
const SCANNED_TREE = join(REPO_ROOT, 'packages');
167+
168+
/** Build output and vendored code are not in-repo call sites. */
169+
const SKIP_DIRS = new Set(['node_modules', 'dist', 'build', 'coverage', '.turbo', '.next']);
170+
171+
/**
172+
* The scanned extension set, spelled ONCE so the pins below can assert it and
173+
* so it stays in exact correspondence with this package's declared glob in
174+
* `CROSS_PACKAGE_TEST_INPUTS` (`packages/**\/*.ts`). Read the extension
175+
* boundary in the header before changing either — they widen together or not
176+
* at all.
177+
*/
178+
const SOURCE_FILE = (name: string): boolean => name.endsWith('.ts') && !name.endsWith('.d.ts');
179+
180+
/**
181+
* The defining package's own contract tests — the tests OF the optional form.
182+
* The glob is deliberately anchored to the whole repo-relative path: a
183+
* same-named file in another package is not a contract test of this predicate.
184+
*/
185+
const EXEMPT = /^packages\/types\/src\/driver-error-classification[^/]*\.test\.ts$/;
186+
187+
const PREDICATE = 'isMissingTableError';
188+
189+
interface CallSite {
190+
readonly path: string;
191+
readonly line: number;
192+
readonly column: number;
193+
readonly text: string;
194+
readonly argumentCount: number;
195+
/** A second argument written as `undefined` / `null` / `void 0`. */
196+
readonly readObjectDiscarded: boolean;
197+
}
198+
199+
interface RenamedImport {
200+
readonly path: string;
201+
readonly line: number;
202+
readonly local: string;
203+
}
204+
205+
function sourceFilesUnder(root: string): string[] {
206+
const out: string[] = [];
207+
const walk = (dir: string): void => {
208+
for (const entry of readdirSync(dir, { withFileTypes: true })) {
209+
if (entry.isDirectory()) {
210+
if (!SKIP_DIRS.has(entry.name)) walk(join(dir, entry.name));
211+
continue;
212+
}
213+
if (!entry.isFile()) continue;
214+
if (!SOURCE_FILE(entry.name)) continue;
215+
out.push(join(dir, entry.name));
216+
}
217+
};
218+
walk(root);
219+
return out;
220+
}
221+
222+
/** `undefined`, `null` and `void 0` all mean "cannot say" to the predicate. */
223+
function discardsReadObject(argument: ts.Expression): boolean {
224+
if (ts.isIdentifier(argument) && argument.text === 'undefined') return true;
225+
if (argument.kind === ts.SyntaxKind.NullKeyword) return true;
226+
return ts.isVoidExpression(argument);
227+
}
228+
229+
function analyse(files: readonly string[]): { calls: CallSite[]; renamedImports: RenamedImport[] } {
230+
const calls: CallSite[] = [];
231+
const renamedImports: RenamedImport[] = [];
232+
233+
for (const file of files) {
234+
const text = readFileSync(file, 'utf8');
235+
if (!text.includes(PREDICATE)) continue;
236+
const path = relative(REPO_ROOT, file).split(sep).join('/');
237+
const sourceFile = ts.createSourceFile(file, text, ts.ScriptTarget.Latest, true, ts.ScriptKind.TS);
238+
239+
const visit = (node: ts.Node): void => {
240+
if (ts.isCallExpression(node)) {
241+
const callee = node.expression;
242+
const name = ts.isIdentifier(callee)
243+
? callee.text
244+
: ts.isPropertyAccessExpression(callee)
245+
? callee.name.text
246+
: undefined;
247+
if (name === PREDICATE) {
248+
const start = node.getStart(sourceFile);
249+
const { line, character } = sourceFile.getLineAndCharacterOfPosition(start);
250+
const second = node.arguments[1];
251+
calls.push({
252+
path,
253+
line: line + 1,
254+
column: character + 1,
255+
text: node.getText(sourceFile).replace(/\s+/g, ' '),
256+
argumentCount: node.arguments.length,
257+
readObjectDiscarded: second !== undefined && discardsReadObject(second),
258+
});
259+
}
260+
}
261+
// A renamed binding would make the by-name match above blind.
262+
if (ts.isImportSpecifier(node) && node.propertyName?.text === PREDICATE) {
263+
const { line } = sourceFile.getLineAndCharacterOfPosition(node.getStart(sourceFile));
264+
renamedImports.push({ path, line: line + 1, local: node.name.text });
265+
}
266+
ts.forEachChild(node, visit);
267+
};
268+
visit(sourceFile);
269+
}
270+
271+
return { calls, renamedImports };
272+
}
273+
274+
const FILES = sourceFilesUnder(SCANNED_TREE);
275+
const { calls: ALL_CALLS, renamedImports: RENAMED_IMPORTS } = analyse(FILES);
276+
277+
const offends = (call: CallSite): boolean => call.argumentCount < 2 || call.readObjectDiscarded;
278+
279+
const REMEDY =
280+
'Pass the object you were reading as the second argument — ' +
281+
"`isMissingTableError(err, object)`. Without it the predicate returns the pre-#13324 WIDE verdict: " +
282+
'a fault naming some OTHER relation is answered "this table is not provisioned yet", ' +
283+
'which on a read path means an outage is silently reported as "no rows".';
284+
285+
function render(sites: readonly CallSite[]): string {
286+
return sites
287+
.map((c) => ` ${c.path}:${c.line}:${c.column} ${c.text}`)
288+
.join('\n');
289+
}
290+
291+
describe('isMissingTableError — every in-repo call names the object it read (#13440)', () => {
292+
it('the scan is anchored to the real workspace root', () => {
293+
expect(existsSync(SCANNED_TREE)).toBe(true);
294+
expect(relative(REPO_ROOT, PACKAGE_ROOT).split(sep).join('/')).toBe('packages/types');
295+
// Self-referential: a mis-anchored walk cannot reach the module under test.
296+
const scanned = new Set(FILES.map((f) => relative(REPO_ROOT, f).split(sep).join('/')));
297+
expect(scanned.has('packages/types/src/driver-error-classification.ts')).toBe(true);
298+
});
299+
300+
// ── POSITIVE CONTROL ──────────────────────────────────────────────────────
301+
// An empty violation set is the passing state, and a broken scanner produces
302+
// the identical empty set. These two say the scanner is looking.
303+
304+
it('POSITIVE CONTROL: sees the exempt contract tests (~30 one-argument calls)', () => {
305+
const inExemptFiles = ALL_CALLS.filter((c) => EXEMPT.test(c.path) && c.argumentCount < 2);
306+
expect(
307+
inExemptFiles.length,
308+
'the defining contract tests exercise the one-argument published form ~30 times ' +
309+
'(30 when this landed); finding none means the call matcher stopped matching, ' +
310+
'not that the repo is clean',
311+
).toBeGreaterThanOrEqual(20);
312+
});
313+
314+
it('POSITIVE CONTROL: the walk reaches packages other than the defining one', () => {
315+
const elsewhere = ALL_CALLS.filter(
316+
(c) => !c.path.startsWith('packages/types/') && c.argumentCount >= 2,
317+
);
318+
const packages = new Set(elsewhere.map((c) => c.path.split('/').slice(0, 2).join('/')));
319+
expect(
320+
elsewhere.length,
321+
'production call sites outside packages/types pass the read object (18 when this ' +
322+
'landed); finding none means the directory walk never left the defining package, ' +
323+
'which would make this gate vacuous',
324+
).toBeGreaterThanOrEqual(15);
325+
expect(packages.size).toBeGreaterThanOrEqual(3);
326+
});
327+
328+
// ── THE EXTENSION BOUNDARY ────────────────────────────────────────────────
329+
// The header explains why this gate reads `.ts` and nothing else. These two
330+
// keep that paragraph from becoming decoration.
331+
332+
it('the scanned extension set is exactly `.ts`, matching the declared glob', () => {
333+
// Pure predicate assertions — no I/O, so this adds nothing to the radius
334+
// this package must declare. Widening any line here without widening
335+
// `packages/**\/*.ts` in CROSS_PACKAGE_TEST_INPUTS is the #7802 shape:
336+
// the scan would judge files turbo never re-runs it for.
337+
expect(SOURCE_FILE('engine.ts')).toBe(true);
338+
expect(SOURCE_FILE('engine.d.ts')).toBe(false);
339+
expect(SOURCE_FILE('realtime-hooks.test.tsx')).toBe(false);
340+
expect(SOURCE_FILE('thing.mts')).toBe(false);
341+
expect(SOURCE_FILE('thing.cts')).toBe(false);
342+
expect(FILES.every((f) => f.endsWith('.ts') && !f.endsWith('.d.ts'))).toBe(true);
343+
});
344+
345+
it('POSITIVE CONTROL: `.tsx` files really do exist under packages/, so excluding them is a decision', () => {
346+
// Filename-only: this counts directory ENTRIES and never opens a `.tsx`
347+
// file, so the exclusion cannot smuggle in a content dependence on files
348+
// outside the declared glob. A floor, not a pin — adding `.tsx` files can
349+
// never redden it, and finding zero would mean the header's measurement
350+
// (8 when this landed) had quietly become a statement about nothing.
351+
const countTsx = (dir: string): number => {
352+
let n = 0;
353+
for (const entry of readdirSync(dir, { withFileTypes: true })) {
354+
if (entry.isDirectory()) {
355+
if (!SKIP_DIRS.has(entry.name)) n += countTsx(join(dir, entry.name));
356+
} else if (entry.isFile() && entry.name.endsWith('.tsx')) n += 1;
357+
}
358+
return n;
359+
};
360+
expect(countTsx(SCANNED_TREE)).toBeGreaterThanOrEqual(1);
361+
});
362+
363+
it('no renamed import hides a call from the by-name matcher', () => {
364+
expect(
365+
RENAMED_IMPORTS,
366+
`${PREDICATE} is matched by callee NAME, so a renamed binding would be invisible to ` +
367+
'this gate. One now exists — teach the matcher the local name before this can pass:\n' +
368+
RENAMED_IMPORTS.map((r) => ` ${r.path}:${r.line} as ${r.local}`).join('\n'),
369+
).toEqual([]);
370+
});
371+
372+
// ── THE GATE ──────────────────────────────────────────────────────────────
373+
374+
it('no in-repo call omits or discards the read object', () => {
375+
const violations = ALL_CALLS.filter((c) => !EXEMPT.test(c.path) && offends(c));
376+
expect(
377+
violations,
378+
`${violations.length} call site(s) of ${PREDICATE}() do not name the object being read:\n` +
379+
`${render(violations)}\n\n${REMEDY}\n\n` +
380+
'The only exemption is the defining package\'s own contract tests ' +
381+
'(packages/types/src/driver-error-classification*.test.ts), which pin the published ' +
382+
'one-argument form on purpose. If your call genuinely has no read object by ' +
383+
'construction, that is a decision for the card, not a widening of this gate.',
384+
).toEqual([]);
385+
});
386+
});

0 commit comments

Comments
 (0)