Files
callstack__agent-device/scripts/layering/package-boundaries.ts
Michał Pierzchała 83322a3f2f 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>
2026-08-04 10:34:57 +02:00

339 lines
13 KiB
TypeScript

// R11 package-boundaries: the workspace rules of #1490, as data the gate walks.
//
// Package resolution already makes a deep `@agent-device/*` specifier a runtime
// resolution error; these checks close the bypasses resolution alone cannot see:
// a package reaching back into root `src/`, a root file tunnelling into
// `packages/*/src` with a relative path, an import of a workspace package the
// manifest never declared, and a specifier subpath the owning `exports` map
// does not name.
//
// The single tolerated relative route into a package is an R8 zero-dep script
// importing an exports-named source target. That exception is exactly
// co-extensive with safety: Node's ESM loader does not realpath specifiers, so
// a module loaded BOTH relatively and via its package specifier instantiates
// twice in one process (duplicate AppError, broken instanceof). A zero-dep
// closure can never coexist with specifier loads — no node_modules — which is
// the only reason the exception exists at all. Production src/test files never
// qualify.
import fs from 'node:fs';
import path from 'node:path';
import { parseImports } from './model.ts';
export type PackageBoundaryViolation = {
rule: string;
file: string;
line: number;
message: string;
};
export type WorkspacePackage = {
/** Repo-relative package dir, e.g. `packages/kernel`. */
dir: string;
name: string;
/** Full import specifier -> repo-relative source target, from `exports`. */
exportTargets: ReadonlyMap<string, string>;
/** Declared `workspace:*` dependencies on sibling internal packages. */
workspaceDependencies: ReadonlySet<string>;
/** Non-workspace dependencies that the root build must externalize. */
externalDependencies: ReadonlyMap<string, string>;
};
export type SpecifierSite = {
file: string;
line: number;
specifier: string;
};
/**
* Every import specifier in `source`, with its 1-based line — through the
* layering model's own parser, so static/dynamic/side-effect/re-export sites
* and both quote styles are covered by one scanner instead of a private regex
* that silently missed double-quoted routes.
*/
export function specifierSites(file: string, source: string): SpecifierSite[] {
return parseImports(source).map((edge) => ({ file, line: edge.line, specifier: edge.spec }));
}
export function readWorkspacePackages(repoRoot: string): WorkspacePackage[] {
const packagesDir = path.join(repoRoot, 'packages');
if (!fs.existsSync(packagesDir)) return [];
const packages: WorkspacePackage[] = [];
for (const entry of fs.readdirSync(packagesDir).sort()) {
const manifestPath = path.join(packagesDir, entry, 'package.json');
if (!fs.existsSync(manifestPath)) continue;
const manifest = JSON.parse(fs.readFileSync(manifestPath, 'utf8')) as {
name?: string;
private?: boolean;
exports?: Record<string, { default?: string } | string>;
dependencies?: Record<string, string>;
};
if (!manifest.name) continue;
const exportTargets = new Map<string, string>();
for (const [subpath, target] of Object.entries(manifest.exports ?? {})) {
const targetFile = typeof target === 'string' ? target : target.default;
if (!targetFile) continue;
exportTargets.set(
path.posix.join(manifest.name, subpath),
path.posix.join('packages', entry, path.posix.normalize(targetFile)),
);
}
const workspaceDependencies = new Set(
Object.entries(manifest.dependencies ?? {})
.filter(([, range]) => range.startsWith('workspace:'))
.map(([name]) => name),
);
const externalDependencies = new Map(
Object.entries(manifest.dependencies ?? {}).filter(
([, range]) => !range.startsWith('workspace:'),
),
);
packages.push({
dir: `packages/${entry}`,
name: manifest.name,
exportTargets,
workspaceDependencies,
externalDependencies,
});
}
return packages;
}
function packageByName(packages: readonly WorkspacePackage[], name: string) {
return packages.find((pkg) => pkg.name === name);
}
function specifierPackageName(specifier: string): string | undefined {
const match = /^(@[^/]+\/[^/]+)/.exec(specifier);
return match?.[1];
}
/**
* Rules for files INSIDE a package: no relative escape past the package dir;
* exported package self-references are legal; any sibling-package import must
* be declared `workspace:*`; and every package specifier must name an export.
*/
export function checkPackageInternalSites(
pkg: WorkspacePackage,
sites: readonly SpecifierSite[],
allPackages: readonly WorkspacePackage[],
): PackageBoundaryViolation[] {
const violations: PackageBoundaryViolation[] = [];
for (const site of sites) {
if (site.specifier.startsWith('.')) {
const resolved = path.posix.normalize(
path.posix.join(path.posix.dirname(site.file), site.specifier),
);
if (!resolved.startsWith(`${pkg.dir}/`)) {
violations.push({
rule: 'R11 package-boundaries',
file: site.file,
line: site.line,
message:
`'${site.specifier}' escapes ${pkg.dir}/ — a workspace package may not reach root ` +
`code. Depend on another package's specifier, or move the shared code below this package.`,
});
}
continue;
}
const name = specifierPackageName(site.specifier);
if (!name || !name.startsWith('@agent-device/')) continue;
const target = packageByName(allPackages, name);
if (!target) {
violations.push({
rule: 'R11 package-boundaries',
file: site.file,
line: site.line,
message: `'${site.specifier}' names an unknown workspace package.`,
});
continue;
}
if (name !== pkg.name && !pkg.workspaceDependencies.has(name)) {
violations.push({
rule: 'R11 package-boundaries',
file: site.file,
line: site.line,
message:
`${pkg.name} imports '${site.specifier}' without declaring "${name}": "workspace:*" ` +
`in ${pkg.dir}/package.json dependencies.`,
});
}
if (!target.exportTargets.has(site.specifier)) {
violations.push({
rule: 'R11 package-boundaries',
file: site.file,
line: site.line,
message:
`'${site.specifier}' is not named by ${target.dir}/package.json#exports — import an ` +
`exported subpath or earn a new one with a real consumer.`,
});
}
}
return violations;
}
/**
* Rules for files OUTSIDE packages/ (src, test, scripts): workspace specifiers
* must be root-declared and exports-named; relative paths into `packages/·/src`
* are forbidden except the R8 zero-dep exception — a `scripts/` file importing
* an exports-named source target.
*/
export function checkRootSites(
sites: readonly SpecifierSite[],
packages: readonly WorkspacePackage[],
rootWorkspaceDependencies: ReadonlySet<string>,
zeroDepClosureFiles: ReadonlySet<string>,
): PackageBoundaryViolation[] {
const violations: PackageBoundaryViolation[] = [];
const exportedSources = new Set(packages.flatMap((pkg) => [...pkg.exportTargets.values()]));
for (const site of sites) {
if (site.specifier.startsWith('.')) {
const resolved = path.posix.normalize(
path.posix.join(path.posix.dirname(site.file), site.specifier),
);
if (!/^packages\/[^/]+\//.test(resolved)) continue;
// The R8 exception requires BOTH membership in an actual zero-dep job
// closure (no node_modules -> no coexisting specifier loads -> no dual
// instantiation) AND an exports-named target. `scripts/` placement alone
// proves neither.
const inZeroDepClosure = zeroDepClosureFiles.has(site.file);
if (inZeroDepClosure && exportedSources.has(resolved)) continue;
violations.push({
rule: 'R11 package-boundaries',
file: site.file,
line: site.line,
message: inZeroDepClosure
? `'${site.specifier}' targets a non-exported package source — the R8 exception only ` +
`covers files named by the package's exports map.`
: `'${site.specifier}' bypasses the package boundary — import the package specifier ` +
`instead. The relative route is reserved for files inside an R8 zero-dep job ` +
`closure; anywhere else, dual specifier/relative loads would instantiate the ` +
`module twice.`,
});
continue;
}
const name = specifierPackageName(site.specifier);
if (!name || !name.startsWith('@agent-device/')) continue;
const target = packageByName(packages, name);
if (!target) {
violations.push({
rule: 'R11 package-boundaries',
file: site.file,
line: site.line,
message: `'${site.specifier}' names an unknown workspace package.`,
});
continue;
}
if (!rootWorkspaceDependencies.has(name)) {
violations.push({
rule: 'R11 package-boundaries',
file: site.file,
line: site.line,
message:
`'${site.specifier}' is used but "${name}" is not a "workspace:*" entry in the root ` +
`package.json devDependencies.`,
});
}
if (!target.exportTargets.has(site.specifier)) {
violations.push({
rule: 'R11 package-boundaries',
file: site.file,
line: site.line,
message:
`'${site.specifier}' is not named by ${target.dir}/package.json#exports — deep imports ` +
`into package internals are a resolution error; import an exported subpath.`,
});
}
}
return violations;
}
/** Root-manifest `workspace:*` names, from dependencies + devDependencies. */
export function rootWorkspaceDependencyNames(repoRoot: string): Set<string> {
const manifest = JSON.parse(fs.readFileSync(path.join(repoRoot, 'package.json'), 'utf8')) as {
dependencies?: Record<string, string>;
devDependencies?: Record<string, string>;
};
return new Set(
Object.entries({ ...manifest.dependencies, ...manifest.devDependencies })
.filter(([, range]) => range.startsWith('workspace:'))
.map(([name]) => name),
);
}
/** Root runtime dependency ranges used by the published bundle. */
export function rootExternalDependencyRanges(repoRoot: string): Map<string, string> {
const manifest = JSON.parse(fs.readFileSync(path.join(repoRoot, 'package.json'), 'utf8')) as {
dependencies?: Record<string, string>;
};
return new Map(Object.entries(manifest.dependencies ?? {}));
}
function walkTsFiles(repoRoot: string, relativeDir: string): string[] {
const absolute = path.join(repoRoot, relativeDir);
if (!fs.existsSync(absolute)) return [];
const files: string[] = [];
const queue = [absolute];
while (queue.length > 0) {
const dir = queue.pop()!;
for (const entry of fs.readdirSync(dir, { withFileTypes: true })) {
const full = path.join(dir, entry.name);
if (entry.isDirectory()) {
if (entry.name !== 'node_modules' && entry.name !== 'dist-types') queue.push(full);
} else if (entry.name.endsWith('.ts') && !entry.name.endsWith('.d.ts')) {
files.push(path.relative(repoRoot, full).replaceAll(path.sep, '/'));
}
}
}
return files.sort();
}
/** Flat `specifier -> repo-relative source` map across all workspace packages. */
export function workspaceSpecifierTargets(repoRoot: string): Map<string, string> {
const targets = new Map<string, string>();
for (const pkg of readWorkspacePackages(repoRoot)) {
for (const [specifier, target] of pkg.exportTargets) targets.set(specifier, target);
}
return targets;
}
/** The real-tree R11 run used by check.ts. */
export function checkPackageBoundaries(
repoRoot: string,
zeroDepClosure: ReadonlySet<string>,
): PackageBoundaryViolation[] {
const packages = readWorkspacePackages(repoRoot);
if (packages.length === 0) return [];
const rootDependencies = rootWorkspaceDependencyNames(repoRoot);
const violations: PackageBoundaryViolation[] = [];
for (const pkg of packages) {
for (const file of walkTsFiles(repoRoot, pkg.dir)) {
const source = fs.readFileSync(path.join(repoRoot, file), 'utf8');
violations.push(...checkPackageInternalSites(pkg, specifierSites(file, source), packages));
}
}
for (const root of ['src', 'test', 'scripts']) {
for (const file of walkTsFiles(repoRoot, root)) {
// Gate tests under scripts/ carry import syntax inside fixture strings
// (same reason R8 parses module records instead of scanning lines);
// src/ and test/ suites stay covered — they import packages for real.
if (root === 'scripts' && file.endsWith('.test.ts')) continue;
const source = fs.readFileSync(path.join(repoRoot, file), 'utf8');
violations.push(
...checkRootSites(specifierSites(file, source), packages, rootDependencies, zeroDepClosure),
);
}
}
return violations;
}
/** Success-line fragment for check.ts's report. */
export function packageBoundariesSummary(repoRoot: string): string {
const packages = readWorkspacePackages(repoRoot);
const exported = packages.reduce((sum, pkg) => sum + pkg.exportTargets.size, 0);
return (
`R11 holds ${packages.length} workspace package(s) behind ${exported} exported subpath(s) ` +
`with zero root back-imports`
);
}