Skip to content

test(layering): pin exact façade symbols for all workspace packages - #1574

Open
thymikee wants to merge 4 commits into
mainfrom
claude/fervent-ramanujan-8813gx
Open

test(layering): pin exact façade symbols for all workspace packages#1574
thymikee wants to merge 4 commits into
mainfrom
claude/fervent-ramanujan-8813gx

Conversation

@thymikee

@thymikee thymikee commented Aug 3, 2026

Copy link
Copy Markdown
Member

Extends the exact exported-symbol gate #1555 introduced for @agent-device/ad-replay to 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/interaction alone names 140 symbols and contracts/client 101; 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

file lines answers
facade-exports.ts 242 what does this façade name?
facade-exports.test.ts 307 …per export form
package-boundaries.ts 338 may this file import that one?
package-boundaries.test.ts 450 …per R11 rule, plus the pinned-table check
facade-symbols.ts 887 the generated table (data only)

Everything 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, and readNamedExports throws 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 what export * actually re-exports:

  • default is excluded — filtered at the star, not at the source. Per GetExportedNames a star skips default, and oxc labels the star's own import AllButDefault. But export { default } from './x.ts' is a named entry whose name is default, and it has to stay in the module's map so a later export { 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 name default — is a real default export of the façade and throws.
  • A name two star sources resolve differently is ambiguous, not exported. Per ResolveExport, importing it is a SyntaxError, so unioning would pin a symbol no consumer can reach. Ambiguity throws, naming both origins.
  • Origins resolve transitively to the binding a name ultimately names, by asking the child's own already-resolved map rather than synthesizing identity from the immediate specifier. So a diamonda re-exports x from b, c re-exports x from a, 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.
  • Resolution stays narrow: a relative specifier only — a package-specifier star still throws, since enumerating it means resolving node_modules and re-entering another package's exports map, 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.

readNamedExports itself 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 type lists, aliases, and export * 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 is default, but what it binds is x, so it must be reported, and default must never appear in the list;
  • a local export { … } list with no from;
  • multi-declarator export const a = 1, b = 2.

Each readFacadeExports rule is paired with the counterfactual that keeps it honest:

rule counterfactual guarding it
hidden default excluded from a star a named sibling in the same leaf still comes through — the leaf is read, only default is dropped
star does not re-export a name called default removing the filter fails the test
entry-file default rejected same source text as the leaf case, only its position changed
star ambiguity throws a diamond and an explicit-shadowing case must still resolve
chain resolves transitively a same-depth chain bottoming out in two distinct declarations must still throw
two paths to one default binding resolves to one name, not ambiguity

Plant-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 via facades/session.ts):

not ok 54 - every workspace package façade exports exactly its pinned symbol list
    @agent-device/contracts/session exports exactly its pinned symbol list
    + actual - expected

      [
        'SESSION_SURFACES',
    +   'STRAY_DEEP_IN_BARREL',
        'SessionAction',
        'SessionSurface',
        'parseSessionSurface'
      ]

ad-script — stray export on the façade itself, caught with the same named diff (+ 'STRAY_AD_SCRIPT_SYMBOL').

xml — a new ./types subpath 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 at fa949d4.

One note on the requested gate list: npx vitest run scripts/layering reports No test files found on this repo — the layering suite is node:test, run via pnpm check:layering. That's how it was verified here.

#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.
@github-actions

github-actions Bot commented Aug 3, 2026

Copy link
Copy Markdown

Size Report

Metric Base Current Diff
JS raw 1.96 MB 1.96 MB 0 B
JS gzip 626.6 kB 626.6 kB 0 B
npm tarball 746.3 kB 746.3 kB +10 B
npm unpacked 2.61 MB 2.61 MB +40 B

Startup median (7 runs, lower is better):

Scenario Base Current Diff
CLI --version 27.2 ms 28.5 ms +1.3 ms
CLI --help 66.1 ms 66.3 ms +0.2 ms

Top changed chunks: no changes in the largest emitted chunks.

@thymikee

thymikee commented Aug 3, 2026

Copy link
Copy Markdown
Member Author

Reviewed exact head 0d11fa3777b43fe33afa43ac6c1506ce57c7f7ce — not ready.

P1 — readFacadeExports does not model export * façade semantics. It recursively unions every child name and throws on every child default. In ESM, export * excludes default, so the test at lines 182–195 codifies a false positive: a private default in a leaf does not widen the façade. Multiple star sources can also make a duplicate name ambiguous rather than exported; blindly unioning cannot represent that actual surface (even where TypeScript separately rejects the source). Please exclude hidden defaults and resolve/reject star ambiguities explicitly, with counterfactual tests for both.

P1 — the test file violates the repository’s explicit context-safety rule. scripts/layering/package-boundaries.test.ts grows from 469 to 1,455 lines; tests are not exempt, and files past 1,000 lines are architecture debt. Move the 850+ line FACADE_SYMBOLS table into a focused sibling fixture/data module and keep the behavioral tests local.

The all-packages gate is a good direction and CI is green, but these correctness and module-shape issues should be fixed before ready-for-human. No device evidence is needed. Residual risk: no separately authorized cross-vendor review was run.

…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.

thymikee commented Aug 3, 2026

Copy link
Copy Markdown
Member Author

Both P1s fixed in c1baea5. You were right on both, including that my own test codified the false positive.

P1 — export * semantics

Confirmed against the spec rather than argued: GetExportedNames excludes default for star exports, and ResolveExport returns ambiguous when star resolution finds two distinct bindings. The walk now models both.

Hidden defaults are excluded, not rejected. A leaf's export default is unreachable through the barrel and does not widen the façade, so it is passed over. The test that asserted a throw here was the false positive you identified; it is replaced by one that pins the correct behaviour with a counterfactual — a named sibling in the same leaf still comes through, proving the leaf is genuinely read and the default specifically is what got dropped:

'.leaf-probe.ts': 'export default function hidden() {}\nexport const reachable = 1;\n',
'.barrel-probe.ts': "export * from './.leaf-probe.ts';\n",
// → ['reachable']

A default on the entry file is still a real default export of the façade and still throws — same source text as the leaf above, only its position changed, which is what makes the pair meaningful.

Ambiguity throws instead of unioning. Two star sources resolving one name differently means no consumer can import it, so pinning it would be a lie. The error names both origins.

The subtlety that made a naive "two paths reached this name" check wrong is handled by tracking origins as declaringModule#name rather than by path taken, so both counterfactuals hold and are tested:

  • a diamond (two barrels reaching one shared declaration) is one binding, not a clash — resolves normally;
  • an explicit export shadows a star-provided name of the same name, matching the spec's own precedence.

Named re-exports resolve their origin through to the source module, so two façades re-exporting one shared symbol agree on its identity instead of reading as a clash.

On your parenthetical: agreed that TypeScript separately rejects some of these sources — the gate no longer leans on that, since it now has to be right about the surface on its own.

P1 — module size

FACADE_SYMBOLS (885 lines of generated table) moved to scripts/layering/facade-symbols.ts. The behavioral tests are back to 642 lines, from 1,455. AGENTS.md exempts generated data and fixture snapshots from the 1,000-line tripwire, so the table would have been defensible where it was — but the file was answering two questions, which is the part of the rule that actually bites, so it's split regardless.

Gates

check:layering 66 tests (up from 63) / typecheck / lint / format:check — green. Contracts plant re-verified under the corrected semantics: a stray planted two files deep behind the export * chain still fails with a named diff and reverts to green.

Noted on residual risk: no cross-vendor review was run, and I haven't authorized one.


Generated by Claude Code

@thymikee

thymikee commented Aug 3, 2026

Copy link
Copy Markdown
Member Author

Re-reviewed exact head c1baea56b5c70790aae78fdc1ea1faa2030c3e89. The amendment correctly fixes hidden-default handling, direct star ambiguity, and the oversized data table, but the PR is still not ready.

P1 — named re-export identity is not resolved transitively. At package-boundaries.ts:211–221, export { x } from './a.ts' is identified as a.ts#x, even when a.ts itself re-exports the binding from another module. Consequently, two paths to the same ultimate binding can be assigned different origins and falsely rejected as ambiguous. For example, a re-exports x from b, c re-exports x from a, and a façade stars both a and c: ESM resolves one b#x binding, while this walker sees b#x versus a#x. Resolve relative named re-exports to their ultimate binding (or use a real ResolveExport implementation) and add this counterfactual test.

P1 — the context-safety extraction is incomplete. package-boundaries.test.ts is still 642 lines and package-boundaries.ts is now 528; the repository rule requires extraction before adding behavior past 500, with tests explicitly not exempt. Move façade export resolution into a focused facade-exports.ts module and mirror its tests in facade-exports.test.ts; the 887-line symbol-only data module is appropriately isolated.

All exact-head CI checks are green and merge state is clean, but no ready-for-human label until these are resolved. Residual risk: no separately authorized cross-vendor review was run.

…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.

thymikee commented Aug 3, 2026

Copy link
Copy Markdown
Member Author

Both fixed in d17fd6d. The transitive-identity bug reproduced exactly as you described before I touched anything.

P1 — transitive re-export identity

Your scenario, run against the old code first:

THREW: readFacadeExports found 'x' re-exported by 2 different 'export *' sources
in .probe/facade.ts (./a.ts, ./c.ts). ESM resolves that to `ambiguous`...

ESM resolves one b#x binding; the walker saw b#x versus a#x, because reExportOrigin synthesized identity from the immediate specifier rather than following it.

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 (${specifier}#${name}) since it isn't a file this gate reads, so two façades re-exporting one symbol from the same package still agree. A cycle in progress falls back to the immediate source.

Two tests, each the other's guard:

  • the chain diamonda from b, c from a, façade stars both → resolves to ['x']. Counterfactual-verified: I reverted the resolution to the old immediate-source identity and confirmed not ok 23 - readFacadeExports follows a named re-export chain to its ultimate binding, then restored it.
  • same depth, genuinely distinct bindings — the two branches bottom out in different declarations → still throws ambiguous. Without this, "resolve transitively" could be satisfied by collapsing every duplicate, which would silently disarm the ambiguity rule you asked for in the first round.

P1 — extraction

facade-exports.ts (219) + facade-exports.test.ts (245), registered in check:layering. package-boundaries.ts drops 528 → 338 and its test file 642 → 450.

file lines
facade-exports.ts 219
facade-exports.test.ts 245
package-boundaries.ts 338
package-boundaries.test.ts 450
facade-symbols.ts (generated data) 887

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

check:layering 68 tests (up from 66) / typecheck / lint / format:check — green. Contracts plant re-verified after the restructure: a stray two files deep behind the export * chain still fails with a named diff, reverts to green.

Also updated the PR body last round — it had still been describing the pre-fix export * behavior, which would have misled anyone reading it against the current code.

Still no cross-vendor review authorized on my side.


Generated by Claude Code

@github-actions

github-actions Bot commented Aug 3, 2026

Copy link
Copy Markdown
PR Preview Action v1.8.1

QR code for preview link

🚀 View preview at
https://callstack.github.io/agent-device/pr-preview/pr-1574/

Built to branch gh-pages at 2026-08-03 20:07 UTC.
Preview will be ready when the GitHub Pages deployment is complete.

@thymikee

thymikee commented Aug 3, 2026

Copy link
Copy Markdown
Member Author

Re-reviewed exact head d17fd6de2944e0510007f6620608666c58ea93c6. The transitive named-reexport fix and focused-module split resolve the previous findings, but one P1 remains.

P1 — default re-export identity still cannot resolve transitively. reExportOrigin asks exportedNames(child).get(importedName), while exportedNames deliberately omits every non-entry default because stars must exclude it. Those are two different questions: a star must not enumerate default, but export { default as x } must still resolve that binding. With leaf exporting default, a re-exporting that default, b exporting a’s default as x, c exporting leaf’s default as x, and a façade starring b plus c, ESM exposes one leaf#default binding; this walker falls back to different intermediate identities and falsely reports ambiguity. Preserve a default-aware internal ResolveExport map while filtering default only from star enumeration, and add this counterfactual.

All exact-head CI is green and the module sizes are now compliant, but no ready-for-human label until this final resolver hole is fixed. Residual risk: no separately authorized cross-vendor review was run.

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.

thymikee commented Aug 3, 2026

Copy link
Copy Markdown
Member Author

Pushed fa949d4. The reported scenario doesn't reproduce — but chasing it found a real spec violation next to it, in the opposite direction.

The reported case resolves correctly

I built your five modules exactly as described (leaf declares a default, a re-exports it, b names a's default x, c names leaf's default x, façade stars b + c) and ran it against d17fd6d:

RESULT: [ 'x' ]

The premise is that exportedNames omits every non-entry default. It only omits a declared one. Dumping what oxc actually reports:

source exportName importName
export default function hidden() {} {kind: "Default"} {kind: "None"}
export { default } from './leaf.ts' {kind: "Name", name: "default"} {kind: "Name", name: "default"}
export { default as x } from './a.ts' {kind: "Name", name: "x"} {kind: "Name", name: "default"}

So an intermediate export { default } from … is a named entry called default, kept in the map, and it already resolves transitively. For the terminal declared default the lookup does miss — but the fallback is ${child}#default, which is precisely the canonical binding, so both paths agree and no false ambiguity arises. It's now pinned by a test so it can't silently regress.

What was actually broken

That same named-entry fact meant default sat in a module's map and got copied wholesale by star enumeration:

export * from './mid.ts'   // where mid.ts has `export { default } from './leaf.ts'`
→ ['default']              // before
→ []                       // after

GetExportedNames skips default for stars — oxc even labels the star's own import AllButDefault — so the gate was pinning a name no consumer can obtain through that star.

default is now filtered at the star, not at the source, and that placement is the whole 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 re-broken the identity resolution the previous round fixed. Your instinct that these are two different questions was right — the split just belongs one level further out than the diagnosis put it.

Also fixed: an entry re-exporting a default as default now throws. It carries a default export exactly as export default … does; only the parse shape differed, and only the declared form was caught.

Tests

  • star filter — counterfactual-verified: removing the one-line filter fails not ok 25 - a star export does not re-export a name called 'default'. A sibling name in the same leaf still comes through, proving default is filtered rather than the module dropped.
  • entry-level rejection of the re-exported form.
  • two paths to one default binding['x'], the case from your report.

Gates

check:layering 71 tests (up from 68) / typecheck / lint / format:check — green. Contracts plant re-verified.

One thing I did not change: readNamedExports (source-only, from #1555) still reports a re-exported default as the name default rather than throwing. No façade currently uses that form, so it's latent, and it's merged code outside this PR's remit — happy to tighten it here if you'd rather it match.

Note the earlier iOS Smoke Tests failure on d17fd6d was a re-run flake, not a code issue: the diff touches zero runtime code, the other three smoke legs passed on the same commit, and the failing step was a macOS frontmost-window precondition. It went green on re-run.


Generated by Claude Code

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants