mirror of
https://github.com/callstack/agent-device.git
synced 2026-09-14 20:06:34 +08:00
83322a3f2f
* 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>
339 lines
13 KiB
TypeScript
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`
|
|
);
|
|
}
|