test(layering): pin exact façade symbols for all workspace packages - #1574
test(layering): pin exact façade symbols for all workspace packages#1574thymikee wants to merge 4 commits into
Conversation
#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.
Size Report
Startup median (7 runs, lower is better):
Top changed chunks: no changes in the largest emitted chunks. |
|
Reviewed exact head P1 — P1 — the test file violates the repository’s explicit context-safety rule. The all-packages gate is a good direction and CI is green, but these correctness and module-shape issues should be fixed before |
…e 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.
|
Both P1s fixed in P1 —
|
|
Re-reviewed exact head P1 — named re-export identity is not resolved transitively. At P1 — the context-safety extraction is incomplete. All exact-head CI checks are green and merge state is clean, but no |
…e-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.
|
Both fixed in P1 — transitive re-export identityYour scenario, run against the old code first: ESM resolves one Origins now resolve through the chain by asking the child's own already-resolved map — which is the whole fix, since that map carries fully-resolved origins by construction: return exportedNames(childPath).get(importedName) ?? `${childPath}#${importedName}`;A package specifier keeps a stable synthetic identity ( Two tests, each the other's guard:
P1 — extraction
Every layering file is now under the 500-line tripwire except the generated table, which the rule exempts. The split lands on the real seam: the boundary rules answer may this file import that one?, this module answers what does this façade name? Gates
Also updated the PR body last round — it had still been describing the pre-fix Still no cross-vendor review authorized on my side. Generated by Claude Code |
|
|
Re-reviewed exact head P1 — default re-export identity still cannot resolve transitively. All exact-head CI is green and the module sizes are now compliant, but no |
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.
|
Pushed The reported case resolves correctlyI built your five modules exactly as described ( The premise is that
So an intermediate What was actually brokenThat same named-entry fact meant
Also fixed: an entry re-exporting a default as Tests
Gates
One thing I did not change: Note the earlier iOS Smoke Tests failure on Generated by Claude Code |
Extends the exact exported-symbol gate #1555 introduced for
@agent-device/ad-replayto the rest of the workspace. The exports-subpath locks prove which files a package exposes; they say nothing about what those files name — so until now any façade except ad-replay's could grow a symbol silently, visible only in a diff review of the package, never of the gate.Body kept in sync with the code through three review rounds; it currently describes
fa949d4.What is pinned
All 29 exported subpaths across the remaining 8 packages:
ad-script,contracts(14 subpaths),kernel(8),maestro,provider-limrun,provider-webdriver,replay-test,xml. ad-replay keeps its existing assertion where #1555 put it, beside that package's own two-value façade rationale.The two providers weren't in the original scope note, but the gate is only honest at "all workspace packages" if it actually covers all of them — both are enumerable façades, so they're pinned too.
Lists are the honest current surface, untrimmed.
contracts/interactionalone names 140 symbols andcontracts/client101; 497 across contracts overall. Several façades are wider than their owners would design today — pinning the real number is exactly what makes the next widening visible. Narrowing one is a change to that package, with its own consumers to fix, not a silent edit to this table.The table is checked in both directions: a new package or subpath that nobody pinned fails rather than being silently skipped.
Module layout
facade-exports.tsfacade-exports.test.tspackage-boundaries.tspackage-boundaries.test.tsfacade-symbols.tsEverything is under the 500-line tripwire except the generated table, which AGENTS.md exempts.
The helper needed widening to reach contracts
13 of contracts' 14 façades are bare
export * from '../x.ts'barrels, andreadNamedExportsthrows on those by design — given only a source string, the set a star contributes is genuinely unknowable. Given the file it is not: the specifier names a sibling module the gate can read and enumerate in turn.readFacadeExports(entryFile)does that walk, modelling whatexport *actually re-exports:defaultis excluded — filtered at the star, not at the source. PerGetExportedNamesa star skipsdefault, and oxc labels the star's own importAllButDefault. Butexport { default } from './x.ts'is a named entry whose name isdefault, and it has to stay in the module's map so a laterexport { default as x }can resolve its binding. Filtering at the source would break identity resolution; filtering at the star is the spec's own split. A default on the entry file — declared, or re-exported under the namedefault— is a real default export of the façade and throws.ResolveExport, importing it is aSyntaxError, so unioning would pin a symbol no consumer can reach. Ambiguity throws, naming both origins.are-exportsxfromb,cre-exportsxfroma, façade stars both — is one binding, not a clash. An explicit export shadows a star-provided name of the same name, matching the spec's own precedence.node_modulesand re-entering another package'sexportsmap, precisely the unbounded widening this gate exists to refuse. A package specifier still gets a stable synthetic identity so two façades re-exporting one symbol from the same package agree. Cycles are visit-guarded.readNamedExportsitself is unchanged; its source-only throw is still load-bearing for single-file façades, and one test asserts both behaviours on the same barrel.Helper unit tests
The AST rewrite in #1555 already handles re-export chains within a file,
export typelists, aliases, andexport * as ns— verified against the merged code rather than re-derived, and not redone. What it handled but left unpinned, now covered:export { default as x } from './y.ts'— the local name isdefault, but what it binds isx, so it must be reported, anddefaultmust never appear in the list;export { … }list with nofrom;export const a = 1, b = 2.Each
readFacadeExportsrule is paired with the counterfactual that keeps it honest:defaultis droppeddefaultPlant-verify
Run on three distinct mechanisms, each reverted to green afterward, and re-verified after every review round.
contracts — the multi-subpath case, stray planted two files deep behind the
export *chain (src/session-surface.ts, reached viafacades/session.ts):ad-script — stray export on the façade itself, caught with the same named diff (
+ 'STRAY_AD_SCRIPT_SYMBOL').xml — a new
./typessubpath added to the manifest with no pinned list failed the bidirectional check (every exports-map subpath needs a pinned symbol list (and vice versa)).Gates
pnpm check:layering(71 tests, up from 53) /pnpm typecheck/pnpm lint/pnpm format:check— all green, and all 30 CI checks pass atfa949d4.One note on the requested gate list:
npx vitest run scripts/layeringreportsNo test files foundon this repo — the layering suite isnode:test, run viapnpm check:layering. That's how it was verified here.