Skip to content

Commit 83322a3

Browse files
thymikeeclaude
andauthored
test(layering): pin exact façade symbols for all workspace packages (#1574)
* test(layering): pin exact façade symbols for all workspace packages #1555 added the repo's first exact exported-symbol gate, pinning @agent-device/ad-replay's named export list. Every other workspace package was still covered only by the exports-subpath locks, which prove which files a package exposes but say nothing about what those files name — so any façade could grow a symbol silently. Pin all 29 exported subpaths across the remaining 8 packages: ad-script, contracts (14), kernel (8), maestro, provider-limrun, provider-webdriver, replay-test, and xml. The lists are the honest current surface, untrimmed — contracts/interaction alone names 140 symbols, and pinning the real number is what makes the next widening visible. The table is checked in both directions, so a new package or subpath that nobody pinned fails rather than being silently skipped. Pinning contracts needed the export-discovery helper widened: 13 of its 14 façades are bare `export * from '../x.ts'` barrels, and readNamedExports throws on those by design, because given only a source string the contributed set is genuinely unknowable. Given the FILE it is not, so readFacadeExports resolves the relative re-export chain and enumerates it. Resolution stays narrow — a package-specifier star still throws (that would mean re-entering another package's exports map, the unbounded widening the gate exists to refuse), cycles are visit-guarded, and a default export still throws through a barrel. Helper unit tests cover the shapes the merged AST scan handles but left unpinned: `export { default as x }` (the form between the two rejection rules — named, so reported, never `default`), a local `export { … }` list with no `from`, and multi-declarator `export const a = 1, b = 2`. Plant-verified per package rather than asserted: a stray export on ad-script, one two files deep behind contracts' `export *` chain, and an unpinned new subpath on xml each failed with a named diff; each reverted to green. Gates: check:layering (63 tests, up from 53) / typecheck / lint / format:check — green. * fix(layering): model real `export *` semantics; split the pinned table out Addresses both P1 findings on #1574. P1 — `readFacadeExports` did not model `export *` façade semantics. It unioned every child name and threw on every child default. Both are wrong: - Per GetExportedNames, a star export excludes the child's `default`, so a private `export default` in a leaf is not reachable through the barrel and does not widen the façade. It is now passed over rather than rejected; the previous test codified that false positive and is replaced. A default on the ENTRY file is still a real default export of the façade and still throws. - Per ResolveExport, a name two star sources resolve differently is `ambiguous` — importing it is a SyntaxError, so it is not part of the surface at all. Unioning would pin a symbol no consumer can import; ambiguity now throws and names both origins. Origins are tracked by declaring module rather than by path taken, so a diamond (two barrels reaching one declaration) resolves normally, and an explicit export shadows a star-provided name of the same name as the spec's own precedence does. Both counterfactuals are tested alongside the two rejection cases. P1 — module size. The 885-line generated FACADE_SYMBOLS table moves to a focused sibling, scripts/layering/facade-symbols.ts, leaving the behavioral tests at 642 lines (from 1,455) so the test file stays one bounded read per AGENTS.md. Gates: check:layering (66 tests, up from 63) / typecheck / lint / format:check — green. Contracts plant re-verified under the corrected semantics: a stray two files deep behind the `export *` chain still fails with a named diff, and reverts to green. * fix(layering): resolve re-export identity transitively; extract facade-exports Addresses both P1 findings on the second review round. P1 — named re-export identity stopped at the immediate source. Given `a` re-exporting `x` from `b`, `c` re-exporting `x` from `a`, and a façade starring both, ESM resolves ONE binding (b's `x`), but the walker identified the two paths as `b#x` and `a#x` and falsely rejected the façade as ambiguous. Reproduced before fixing. Origins now resolve through the chain to the binding a name ultimately names, by asking the child's own already-resolved map instead of synthesizing an identity from the specifier. A package specifier keeps a stable synthetic identity (it is not a file this gate reads), and a cycle in progress falls back to the immediate source. Two tests, counterfactual-verified against each other: the chain diamond now resolves to one name (confirmed failing with the old immediate-source identity, passing with the fix), and a same-depth chain whose branches bottom out in two genuinely distinct declarations still throws — so the fix cannot be satisfied by simply collapsing every duplicate. P1 — context-safety extraction was incomplete. Façade export enumeration moves to scripts/layering/facade-exports.ts (219 lines) with its own facade-exports.test.ts (245), registered in check:layering. package-boundaries.ts drops to 338 from 528 and its test file to 450 from 642: the boundary rules answer "may this file import that one?", this module answers "what does this façade name?". Every layering file is now under the 500-line tripwire except the generated symbol table, which the rule exempts. Gates: check:layering (68 tests, up from 66) / typecheck / lint / format:check — green. Contracts plant re-verified after the split. * fix(layering): filter `default` at the star, not at its source The reported P1 does not reproduce: intermediate `export { default } from './x.ts'` links are reported by oxc as kind `Name` with the name `default`, not kind `Default`, so they already resolve transitively; and for a terminal `export default <decl>`, the fallback identity `${child}#default` is exactly the canonical binding, so both paths agree. The exact five-module scenario from the review returns ['x']. That behavior is now pinned by a test so it cannot silently regress. Investigating it did surface a real spec violation in the opposite direction. Because a re-exported `default` is a named entry, it landed in the module's map and was then copied wholesale by star enumeration, so `export * from './mid.ts'` reported `default` as part of the surface — a name `GetExportedNames` explicitly skips, and which oxc itself labels `AllButDefault` on the star's own import. `default` is now filtered at the star rather than at the source. That placement is the point: the name has to stay in the module's map so a later `export { default as x }` can resolve its binding, while never being reachable through a star. Filtering at the source would have broken identity resolution — the very thing the review round before this one fixed. A façade entry re-exporting a default under the name `default` is now rejected too. It carries a default export exactly as `export default …` does; only the parse shape differs, and only the declared form was being caught. Three tests: the star filter (counterfactual-verified — removing the filter fails it — with a sibling name proving the module is still read), entry-level rejection, and the two-paths-to-one-default-binding case from the review. Gates: check:layering (71 tests, up from 68) / typecheck / lint / format:check — green. Contracts plant re-verified. --------- Co-authored-by: Claude <noreply@anthropic.com>
1 parent 6baa5d9 commit 83322a3

6 files changed

Lines changed: 1465 additions & 104 deletions

File tree

package.json

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -129,7 +129,7 @@
129129
"check:affected:test": "node --experimental-strip-types --test scripts/check-affected/model.test.ts scripts/check-affected/run.test.ts",
130130
"check:coverage-changed": "node --experimental-strip-types scripts/coverage-changed/run.ts",
131131
"check:coverage-changed:test": "node --experimental-strip-types --test scripts/coverage-changed/model.test.ts scripts/coverage-changed/run.test.ts",
132-
"check:layering": "node --experimental-strip-types --test scripts/layering/model.test.ts scripts/layering/zone-policy.test.ts scripts/layering/daemon-modularity.test.ts scripts/layering/package-boundaries.test.ts && node --experimental-strip-types scripts/layering/check.ts",
132+
"check:layering": "node --experimental-strip-types --test scripts/layering/model.test.ts scripts/layering/zone-policy.test.ts scripts/layering/daemon-modularity.test.ts scripts/layering/package-boundaries.test.ts scripts/layering/facade-exports.test.ts && node --experimental-strip-types scripts/layering/check.ts",
133133
"depgraph": "node --experimental-strip-types scripts/depgraph/build.ts",
134134
"depgraph:test": "node --experimental-strip-types --test scripts/depgraph/model.test.ts scripts/depgraph/affected.test.ts",
135135
"check:production-exports": "fallow dead-code --config fallow-production-exports.json --production --unused-exports --fail-on-issues",
Lines changed: 307 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,307 @@
1+
// Façade export enumeration, tested directly: what `readNamedExports` and
2+
// `readFacadeExports` report for each export FORM, independently of the R11
3+
// boundary rules that consume them.
4+
//
5+
// The `readFacadeExports` cases write throwaway modules under a real package
6+
// rather than using committed fixtures — a fixture would pin the walker
7+
// against a file shape this repo never actually ships.
8+
9+
import assert from 'node:assert/strict';
10+
import fs from 'node:fs';
11+
import path from 'node:path';
12+
import { test } from 'node:test';
13+
import { readFacadeExports, readNamedExports } from './facade-exports.ts';
14+
15+
const repoRoot = path.resolve(import.meta.dirname, '../..');
16+
17+
test('readNamedExports collects re-export and direct-declaration forms, resolving aliases', () => {
18+
const source = [
19+
"export { a, b } from './x.ts';",
20+
"export type { C, D } from './y.ts';",
21+
"export { e as f } from './z.ts';",
22+
"export type { g as h } from './z.ts';",
23+
'export function i() {}',
24+
'export const j = 1;',
25+
'export type K = string;',
26+
'export interface L {}',
27+
"export {\n m,\n n,\n} from './multi.ts';",
28+
].join('\n');
29+
assert.deepEqual(
30+
readNamedExports(source),
31+
['D', 'C', 'K', 'L', 'a', 'b', 'f', 'h', 'i', 'j', 'm', 'n'].sort(),
32+
);
33+
});
34+
35+
test('readNamedExports never reports the original name behind an `as` alias', () => {
36+
const source = "export { internalOnly as publicName } from './x.ts';";
37+
const names = readNamedExports(source);
38+
assert.deepEqual(names, ['publicName']);
39+
assert.ok(!names.includes('internalOnly'));
40+
});
41+
42+
test('readNamedExports resolves `export * as ns` to its one real bound name', () => {
43+
// Unlike bare `export *`, this binds exactly one importable name (`ns`) —
44+
// enumerable, not a widening blind spot.
45+
const source = "export * as ns from './x.ts';";
46+
assert.deepEqual(readNamedExports(source), ['ns']);
47+
});
48+
49+
// #1555 review P1 (second pass, "the gate also ignores export-star
50+
// declarations, so it can miss future widening"): a facade pinned to an
51+
// exact named-export list must not silently accept a form that widens its
52+
// real surface with no enumerable name at all. These two forms throw instead
53+
// of contributing nothing to the list — plant-verified (temporarily reverted
54+
// to a no-op, confirmed both tests failed, restored) rather than merely
55+
// asserted.
56+
test('readNamedExports rejects a bare `export *` re-export', () => {
57+
const source = "export { runAdReplay } from './step-loop.ts';\nexport * from './leak.ts';\n";
58+
assert.throws(() => readNamedExports(source), /export \* from/);
59+
});
60+
61+
test('readNamedExports rejects a default export', () => {
62+
assert.throws(() => readNamedExports('export default function leak() {}'), /export default/);
63+
assert.throws(() => readNamedExports('export default 42;'), /export default/);
64+
});
65+
66+
test('readNamedExports reports `export { default as x }` as the named symbol x', () => {
67+
// The one form that sits between the two rejection rules above: the LOCAL
68+
// name is `default`, but what it binds in this module — and the only thing
69+
// a consumer can import — is `x`. Enumerable, so it must be reported, not
70+
// thrown; and `default` must never appear in the list.
71+
const names = readNamedExports("export { default as x, b } from './y.ts';");
72+
assert.deepEqual(names, ['b', 'x']);
73+
assert.ok(!names.includes('default'));
74+
});
75+
76+
test('readNamedExports collects a local `export { … }` list with no `from`', () => {
77+
// The re-export tests above all carry a `from`; a façade that declares
78+
// first and exports at the bottom is the same public surface.
79+
assert.deepEqual(
80+
readNamedExports('const a = 1;\ntype T = string;\nexport { a };\nexport type { T };'),
81+
['T', 'a'],
82+
);
83+
});
84+
85+
test('readNamedExports collects every declarator of a multi-declarator export', () => {
86+
// Documented in the helper's contract; the direct-declaration test above
87+
// only exercises a single declarator, so the second name went unpinned.
88+
assert.deepEqual(readNamedExports('export const a = 1, b = 2;'), ['a', 'b']);
89+
});
90+
91+
// `readFacadeExports` is the same enumeration widened from one source string
92+
// to the re-export CHAIN behind a file — the form every `contracts` façade
93+
// is built from. These use the real tree's own barrels rather than fixtures:
94+
// a fixture would pin the walker against a file this repo never ships.
95+
test('readFacadeExports resolves a bare `export *` chain the source-only reader refuses', () => {
96+
const barrel = path.join(repoRoot, 'packages/contracts/src/facades/session.ts');
97+
// Source-only: unknowable, so it throws (the merged contract, unchanged).
98+
assert.throws(() => readNamedExports(fs.readFileSync(barrel, 'utf8')), /export \* from/);
99+
// Given the FILE, the same barrel is fully enumerable.
100+
assert.deepEqual(readFacadeExports(barrel), [
101+
'SESSION_SURFACES',
102+
'SessionAction',
103+
'SessionSurface',
104+
'parseSessionSurface',
105+
]);
106+
});
107+
108+
test('readFacadeExports refuses a bare `export *` across a package specifier', () => {
109+
// A relative star names a module this gate can read; a package star means
110+
// resolving node_modules into another package's exports map — unbounded
111+
// widening, the exact thing the gate refuses.
112+
const scratch = path.join(repoRoot, 'packages/contracts/src/facades/.export-star-probe.ts');
113+
fs.writeFileSync(scratch, "export * from '@agent-device/kernel/errors';\n");
114+
try {
115+
assert.throws(() => readFacadeExports(scratch), /only a relative re-export/);
116+
} finally {
117+
fs.rmSync(scratch);
118+
}
119+
});
120+
121+
/** Write throwaway modules next to a real façade; always clean them up. */
122+
function withProbeModules(files: Record<string, string>, run: (dir: string) => void): void {
123+
const dir = path.join(repoRoot, 'packages/contracts/src/facades');
124+
const written = Object.entries(files).map(([name, source]) => {
125+
const file = path.join(dir, name);
126+
fs.writeFileSync(file, source);
127+
return file;
128+
});
129+
try {
130+
run(dir);
131+
} finally {
132+
for (const file of written) fs.rmSync(file, { force: true });
133+
}
134+
}
135+
136+
test('readFacadeExports excludes a default that a star export cannot reach', () => {
137+
// #1574 review P1: `export *` skips the child's default per
138+
// GetExportedNames, so a private default in a leaf is NOT part of the
139+
// barrel's surface. Counterfactual: the named sibling still comes through,
140+
// proving the leaf is genuinely being read and the default specifically —
141+
// not the whole module — is what got dropped.
142+
withProbeModules(
143+
{
144+
'.leaf-probe.ts': 'export default function hidden() {}\nexport const reachable = 1;\n',
145+
'.barrel-probe.ts': "export * from './.leaf-probe.ts';\n",
146+
},
147+
(dir) => {
148+
assert.deepEqual(readFacadeExports(path.join(dir, '.barrel-probe.ts')), ['reachable']);
149+
},
150+
);
151+
});
152+
153+
test('readFacadeExports still rejects a default on the façade entry itself', () => {
154+
// The other side of the same rule: the entry's own default IS a default
155+
// export of the façade, and a façade pinned to a named list must not carry
156+
// one. Same source text as the leaf above — only its position changed.
157+
withProbeModules({ '.entry-default-probe.ts': 'export default function leak() {}\n' }, (dir) => {
158+
assert.throws(
159+
() => readFacadeExports(path.join(dir, '.entry-default-probe.ts')),
160+
/export default/,
161+
);
162+
});
163+
});
164+
165+
test('readFacadeExports rejects a name two star sources resolve differently', () => {
166+
// ESM resolves this to `ambiguous`, so `clash` is not importable at all;
167+
// unioning would pin a symbol no consumer can reach.
168+
withProbeModules(
169+
{
170+
'.clash-a-probe.ts': 'export const clash = 1;\nexport const onlyA = 1;\n',
171+
'.clash-b-probe.ts': 'export const clash = 2;\n',
172+
'.clash-barrel-probe.ts':
173+
"export * from './.clash-a-probe.ts';\nexport * from './.clash-b-probe.ts';\n",
174+
},
175+
(dir) => {
176+
assert.throws(() => readFacadeExports(path.join(dir, '.clash-barrel-probe.ts')), /ambiguous/);
177+
},
178+
);
179+
});
180+
181+
test('readFacadeExports resolves a diamond and lets an explicit export shadow a star', () => {
182+
// The two counterfactuals to the ambiguity rule, both of which a naive
183+
// "two paths reached this name" check would wrongly reject. One shared
184+
// declaration reached by two barrels is ONE binding, not a clash; and an
185+
// explicit re-export of a name a star also provides is the spec's own
186+
// precedence, not ambiguity.
187+
withProbeModules(
188+
{
189+
'.shared-probe.ts': 'export const shared = 1;\n',
190+
'.mid-one-probe.ts': "export * from './.shared-probe.ts';\n",
191+
'.mid-two-probe.ts': "export * from './.shared-probe.ts';\n",
192+
'.diamond-probe.ts':
193+
"export * from './.mid-one-probe.ts';\nexport * from './.mid-two-probe.ts';\n",
194+
'.shadow-src-probe.ts': 'export const shadowed = 1;\nexport const other = 2;\n',
195+
'.shadow-probe.ts':
196+
"export * from './.shadow-src-probe.ts';\nexport { shadowed } from './.shared-probe.ts';\n",
197+
},
198+
(dir) => {
199+
assert.deepEqual(readFacadeExports(path.join(dir, '.diamond-probe.ts')), ['shared']);
200+
assert.deepEqual(readFacadeExports(path.join(dir, '.shadow-probe.ts')), [
201+
'other',
202+
'shadowed',
203+
]);
204+
},
205+
);
206+
});
207+
208+
test('readFacadeExports follows a named re-export chain to its ultimate binding', () => {
209+
// #1574 review P1: `a` re-exports `x` from `b`, `c` re-exports `x` from
210+
// `a`, and the façade stars both. ESM resolves ONE binding (`b`'s `x`), so
211+
// this is a diamond, not a clash. Identifying a re-export by its immediate
212+
// source would see `b#x` vs `a#x` and falsely reject the façade — the
213+
// counterfactual that fails without transitive origin resolution.
214+
withProbeModules(
215+
{
216+
'.chain-b-probe.ts': 'export const x = 1;\n',
217+
'.chain-a-probe.ts': "export { x } from './.chain-b-probe.ts';\n",
218+
'.chain-c-probe.ts': "export { x } from './.chain-a-probe.ts';\n",
219+
'.chain-facade-probe.ts':
220+
"export * from './.chain-a-probe.ts';\nexport * from './.chain-c-probe.ts';\n",
221+
},
222+
(dir) => {
223+
assert.deepEqual(readFacadeExports(path.join(dir, '.chain-facade-probe.ts')), ['x']);
224+
},
225+
);
226+
});
227+
228+
test('readFacadeExports keeps rejecting two genuinely distinct bindings behind a chain', () => {
229+
// The guard against over-correcting the above: resolving through chains
230+
// must not collapse two REAL declarations into one. Same chain depth as the
231+
// diamond, but the two branches bottom out in different modules.
232+
withProbeModules(
233+
{
234+
'.split-one-probe.ts': 'export const y = 1;\n',
235+
'.split-two-probe.ts': 'export const y = 2;\n',
236+
'.split-a-probe.ts': "export { y } from './.split-one-probe.ts';\n",
237+
'.split-c-probe.ts': "export { y } from './.split-two-probe.ts';\n",
238+
'.split-facade-probe.ts':
239+
"export * from './.split-a-probe.ts';\nexport * from './.split-c-probe.ts';\n",
240+
},
241+
(dir) => {
242+
assert.throws(() => readFacadeExports(path.join(dir, '.split-facade-probe.ts')), /ambiguous/);
243+
},
244+
);
245+
});
246+
247+
// #1574 review, third round. `export { default } from './x.ts'` is reported
248+
// by oxc as kind `Name` with the name `default` — the same fact as
249+
// `export default …` wearing a different parse shape. It has to stay in a
250+
// module's map so a later `export { default as x }` can resolve its binding,
251+
// while never being reachable through a star. These three pin that split.
252+
test('a star export does not re-export a name called `default`', () => {
253+
// Per GetExportedNames a star skips `default` — oxc names the star's own
254+
// import `AllButDefault`. Counterfactual: the ordinary sibling name in the
255+
// same module still comes through, so this is `default` being filtered and
256+
// not the whole module being dropped.
257+
withProbeModules(
258+
{
259+
'.dstar-leaf-probe.ts': 'export default function hidden() {}\nexport const kept = 1;\n',
260+
'.dstar-mid-probe.ts':
261+
"export { default } from './.dstar-leaf-probe.ts';\n" +
262+
"export { kept } from './.dstar-leaf-probe.ts';\n",
263+
'.dstar-facade-probe.ts': "export * from './.dstar-mid-probe.ts';\n",
264+
},
265+
(dir) => {
266+
assert.deepEqual(readFacadeExports(path.join(dir, '.dstar-facade-probe.ts')), ['kept']);
267+
},
268+
);
269+
});
270+
271+
test('a façade re-exporting a default under the name `default` is rejected', () => {
272+
// The entry carries a default export either way; only the parse shape
273+
// differs from the `export default …` case above.
274+
withProbeModules(
275+
{
276+
'.dentry-leaf-probe.ts': 'export default function leak() {}\n',
277+
'.dentry-facade-probe.ts': "export { default } from './.dentry-leaf-probe.ts';\n",
278+
},
279+
(dir) => {
280+
assert.throws(
281+
() => readFacadeExports(path.join(dir, '.dentry-facade-probe.ts')),
282+
/must not carry one/,
283+
);
284+
},
285+
);
286+
});
287+
288+
test('two paths to one default binding resolve to a single name, not ambiguity', () => {
289+
// `leaf` declares a default; `a` re-exports it; `b` names a's default `x`
290+
// while `c` names leaf's default `x`; a façade stars both. ESM resolves ONE
291+
// `leaf#default` binding, so `x` is exported rather than ambiguous — the
292+
// intermediate `export { default } from` link has to carry identity through
293+
// for the two paths to agree.
294+
withProbeModules(
295+
{
296+
'.dchain-leaf-probe.ts': 'export default function shared() {}\n',
297+
'.dchain-a-probe.ts': "export { default } from './.dchain-leaf-probe.ts';\n",
298+
'.dchain-b-probe.ts': "export { default as x } from './.dchain-a-probe.ts';\n",
299+
'.dchain-c-probe.ts': "export { default as x } from './.dchain-leaf-probe.ts';\n",
300+
'.dchain-facade-probe.ts':
301+
"export * from './.dchain-b-probe.ts';\nexport * from './.dchain-c-probe.ts';\n",
302+
},
303+
(dir) => {
304+
assert.deepEqual(readFacadeExports(path.join(dir, '.dchain-facade-probe.ts')), ['x']);
305+
},
306+
);
307+
});

0 commit comments

Comments
 (0)