refactor(contracts): name façade exports explicitly and retire the pin table - #1614
Conversation
Size Report
Startup median (7 runs, lower is better):
Top changed chunks: no changes in the largest emitted chunks. |
|
P1 — rebase this conversion onto current #1567 landed after this branch forked and added 13 exports that the existing The #1567 update to Separately, the iOS Smoke job is red on |
cdbfed0 to
328d1de
Compare
Review on #1614 caught this conversion silently narrowing the public surface. The explicit lists were generated against the surface at fork time; #1567 landed 13 exports meanwhile — `DragOptions`, the drag-gesture vocabulary (`COORDINATE_GESTURE_KINDS`, `CoordinateGesturePayload`, the three `DEFAULT_DRAG_*` constants, `DragGestureInput`, `DragGesturePayload`, `GestureCommandInput`, `buildDragGesturePlan`, `dragGesturePayloadFromPositionals`, `normalizeGestureCommandInput`) and `MultiTargetAnnotationV1`. The `export *` barrels had been forwarding all 13 automatically; the rebase dropped every one, and only a human diff caught it. The star-rejection gate could not: it only proves a façade does not WIDEN invisibly. Narrowing is the failure an explicit list newly makes possible, because `export *` could not narrow by construction. So the property the stars gave for free is now asserted directly — every name a re-exported source declares must appear in the façade. Scoped to `packages/*/src/facades/`, the barrels this PR converted. A hand-curated package `index.ts` is a different thing: `ad-replay` deliberately publishes two values out of a much larger `internal/`, and forcing exhaustiveness there would widen a surface its owner narrowed on purpose (#1555). A source that itself carries a bare `export *` is skipped — unknowable from that file alone, and reachable because the façade re-exports the starred module directly too, which IS checked. Red evidence: dropping `MultiTargetAnnotationV1` from facades/replay.ts — one of the 13 the old gate was blind to — fails with the file, the source and the symbol named. 13 pass / 0 fail once restored.
|
Follow-up on the earlier P1: the rebase correctly restores the 13 exports added by #1567. I independently compared every contracts subpath against current Two blockers remain:
|
…n table Thirteen of the fourteen `@agent-device/contracts` façades were bare `export *` barrels. `facades/snapshot.ts`, added by #1582, was the one exception — explicit named re-exports — and that is now the rule. Everything #1574 built to cope with `export *` goes with them: scripts/layering/facade-symbols.ts -980 (816 pinned names) scripts/layering/facade-exports.ts -192 (readFacadeExports) scripts/layering/facade-exports.test.ts -234 (star semantics) scripts/layering/package-boundaries.test.ts -55 `readFacadeExports` re-implemented ESM `GetExportedNames`/`ResolveExport` — star-chain resolution, ambiguity rejection, diamond binding identity, cycle guards, spec-accurate `default` filtering at the star rather than the source. All of it existed to enumerate what `export *` hides. 523 of the 816 pinned names belonged to contracts, i.e. to those thirteen files. Once a façade names its exports, the façade file IS the pin, and it is visible in the diff of the file that widened rather than in a separate table a reviewer has to cross-check. `readNamedExports` (20 lines) stays and is enough: it already throws on bare `export *` and on `export default`. The pin is replaced by one structural gate — no façade may contain a bare star — which reuses that rejection rather than adding a regex. Surface equivalence verified independently, not asserted: main's own `readFacadeExports` run over the new façades, compared against main's own `FACADE_SYMBOLS` table — 31 subpaths, 0 added, 0 removed. Red evidence for the new gate: planting `export * from '../request-progress.ts'` back into facades/progress.ts fails it with the file named and the reason quoted; 12 pass / 0 fail once reverted. Not included: the `lowerAndroidTouchPlan` tuple-assertion drive-by. It needs `sampleGestureOffsets` to carry a min-arity tuple through `.map()`, which TypeScript will not infer without a typed helper — a real change to the gesture-plan contract rather than a drive-by, so it stays out.
Review on #1614 caught this conversion silently narrowing the public surface. The explicit lists were generated against the surface at fork time; #1567 landed 13 exports meanwhile — `DragOptions`, the drag-gesture vocabulary (`COORDINATE_GESTURE_KINDS`, `CoordinateGesturePayload`, the three `DEFAULT_DRAG_*` constants, `DragGestureInput`, `DragGesturePayload`, `GestureCommandInput`, `buildDragGesturePlan`, `dragGesturePayloadFromPositionals`, `normalizeGestureCommandInput`) and `MultiTargetAnnotationV1`. The `export *` barrels had been forwarding all 13 automatically; the rebase dropped every one, and only a human diff caught it. The star-rejection gate could not: it only proves a façade does not WIDEN invisibly. Narrowing is the failure an explicit list newly makes possible, because `export *` could not narrow by construction. So the property the stars gave for free is now asserted directly — every name a re-exported source declares must appear in the façade. Scoped to `packages/*/src/facades/`, the barrels this PR converted. A hand-curated package `index.ts` is a different thing: `ad-replay` deliberately publishes two values out of a much larger `internal/`, and forcing exhaustiveness there would widen a surface its owner narrowed on purpose (#1555). A source that itself carries a bare `export *` is skipped — unknowable from that file alone, and reachable because the façade re-exports the starred module directly too, which IS checked. Red evidence: dropping `MultiTargetAnnotationV1` from facades/replay.ts — one of the 13 the old gate was blind to — fails with the file, the source and the symbol named. 13 pass / 0 fail once restored.
Two review findings, plus a third the gate caught on itself. P1 — the three `DEFAULT_DRAG_*` constants join the existing public-façade suppression, alongside `COORDINATE_GESTURE_KINDS` and `normalizePublicGesture` which the same conversion surfaced. All five are #1567's drag vocabulary, made individually visible to `--production` analysis for the first time because a bare star used to hide them from that exact check. Kept rather than narrowed, for the reason the existing entry already states: the façade's surface stays byte-identical to what the retired pin table asserted, and narrowing is a follow-up with its own review. P2 — the exhaustiveness gate skipped any source carrying a bare `export *`, which dropped that module's DIRECT exports from the check too. `gesture-plan.ts` stars `gesture-plan-types.ts`, so removing `buildDragGesturePlan` from the façade narrowed the public surface and still passed. `readDirectNamedExports` now reads exactly the names a module declares or re-exports BY NAME and ignores the star, so direct exports are checked while the starred set stays covered by the façade's own direct re-export of that module. Red evidence: removing `buildDragGesturePlan` from facades/interaction.ts now fails naming file, source and symbol; 13 pass / 0 fail restored. Third, and the reason the gate is worth having: rebasing onto main after #1612 merged silently dropped `TEXT_ENTRY_ROUTES`, `TextEntryRoute` and `TypeTextBackendResult` from the interaction façade — the same narrowing class as the #1567 one review caught by hand, one merge later. The gate failed on it before CI did. Restored.
328d1de to
5196cf7
Compare
|
Both blockers addressed on P1 — suppression extended. The three P2 — real hole, fixed. You were right that Red evidence: removing Third — the gate paid for itself during this fix. Rebasing onto main after #1612 merged silently dropped That is twice now that this conversion has narrowed the surface on a rebase, which I think settles whether the gate earns its keep. Gates: typecheck / lint / format / 🤖 Addressed by Claude Code |
|
Re-reviewed exact head The prior findings are resolved: all three The structural gates and all GitHub checks are green. This is a surface-preserving structural refactor, so runtime/device evidence is not required. No blockers found. |
thymikee
added
the
ready-for-human
|
1 participant
Thirteen of the fourteen
@agent-device/contractsfaçades were bareexport *barrels.facades/snapshot.ts, added by #1582, was the oneexception — explicit named re-exports — and that is now the rule.
Everything #1574 built to cope with
export *goes with them:scripts/layering/facade-symbols.ts -980 (816 pinned names)
scripts/layering/facade-exports.ts -192 (readFacadeExports)
scripts/layering/facade-exports.test.ts -234 (star semantics)
scripts/layering/package-boundaries.test.ts -55
readFacadeExportsre-implemented ESMGetExportedNames/ResolveExport— star-chain resolution, ambiguity rejection, diamond binding identity,
cycle guards, spec-accurate
defaultfiltering at the star rather thanthe source. All of it existed to enumerate what
export *hides. 523 ofthe 816 pinned names belonged to contracts, i.e. to those thirteen files.
Once a façade names its exports, the façade file IS the pin, and it is
visible in the diff of the file that widened rather than in a separate
table a reviewer has to cross-check.
readNamedExports(20 lines) stays and is enough: it already throws onbare
export *and onexport default. The pin is replaced by onestructural gate — no façade may contain a bare star — which reuses that
rejection rather than adding a regex.
Surface equivalence verified independently, not asserted: main's own
readFacadeExportsrun over the new façades, compared against main's ownFACADE_SYMBOLStable — 31 subpaths, 0 added, 0 removed.Red evidence for the new gate: planting
export * from '../request-progress.ts'back into facades/progress.ts fails it with the file named and the reason
quoted; 12 pass / 0 fail once reverted.
Not included: the
lowerAndroidTouchPlantuple-assertion drive-by. Itneeds
sampleGestureOffsetsto carry a min-arity tuple through.map(),which TypeScript will not infer without a typed helper — a real change to
the gesture-plan contract rather than a drive-by, so it stays out.
Gates: typecheck / lint / format /
check:layering/check:production-exportsgreen.Local gate note.
pnpm check:affected --runis flaky on a loaded dev host. Across six runs on three branches — plus a reproduction on unmodifiedorigin/mainby a parallel agent — failures came only from the poolandroid-lifecycle/android-recording/doctor/input-actions/daemon-client, always as timeouts, with a different subset each run. All pass in isolation (verified for this branch). Please treat the CI Integration Tests job on this head as authoritative forprovider-integration, per AGENTS.md ("GitHub remains authoritative for reported device/toolchain lanes").