Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 3 additions & 20 deletions scripts/layering/check.ts
Original file line number Diff line number Diff line change
Expand Up @@ -98,41 +98,24 @@ import { policyLead, policyViolation, ZONE_POLICIES } from './zone-policy.ts';
import { contractsImplementationAuthorityViolations } from './contracts-implementation-policy.ts';
import { selectorPipelineOwnershipViolations } from './selector-pipeline-ownership.ts';
import { recordRuntimeRegistryJoinViolations } from './record-runtime-registry-policy.ts';
import { listTrackedProductionSources, listTrackedTypeScriptFiles } from './tracked-sources.ts';

const repoRoot = execFileSync('git', ['rev-parse', '--show-toplevel'], {
encoding: 'utf8',
}).trim();

export function listTypeScriptFiles(): string[] {
// `src/**/*.ts` only matches nested files; root-level `src/*.ts` (e.g.
// src/cli.ts, src/command-catalog.ts) needs its own pathspec or it silently
// drops out of cycle/back-edge analysis.
// Workspace package sources are production files too (#1490 W0): R4 cycle
// rejection and the zone staleness guard must see them, and workspace
// specifiers resolve across the seam in resolveTargetFile.
const out = execFileSync(
'git',
['ls-files', 'src/*.ts', 'src/**/*.ts', 'packages/*/src/*.ts', 'packages/*/src/**/*.ts'],
{
cwd: repoRoot,
encoding: 'utf8',
},
);
return out.split('\n').filter(Boolean);
return listTrackedTypeScriptFiles(repoRoot);
}

export function listSourceFiles(): string[] {
return listTypeScriptFiles().filter(isProductionSourceFile);
return listTrackedProductionSources(repoRoot);
}

function readSources(files: readonly string[]): Map<string, string> {
return new Map(files.map((file) => [file, fs.readFileSync(path.join(repoRoot, file), 'utf8')]));
}

function isProductionSourceFile(file: string): boolean {
return file.endsWith('.ts') && !/(?:^|\/)__tests__\//.test(file) && !/\.test\.ts$/.test(file);
}

// R1-R3 are declared as a policy table in zone-policy.ts. This walks it; the boundaries
// themselves are data, so adding one is a table entry rather than a fourth predicate.
function checkLayeringRules(edges: readonly ResolvedImportEdge[]): LayeringViolation[] {
Expand Down
61 changes: 53 additions & 8 deletions scripts/layering/package-boundaries.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,13 +3,16 @@
// so a rule that stopped matching would look exactly like a rule being obeyed.

import assert from 'node:assert/strict';
import { execFileSync } from 'node:child_process';
import fs from 'node:fs';
import os from 'node:os';
import path from 'node:path';
import { test } from 'node:test';
import { listSourceFiles } from './check.ts';
import { readDirectNamedExports, readNamedExports, readReExportSources } from './facade-exports.ts';
import {
checkPackageBoundaries,
facadeEntryFiles,
checkPackageInternalSites,
checkRootSites,
readWorkspacePackages,
Expand Down Expand Up @@ -137,24 +140,66 @@ test('specifier sites carry 1-based lines for static and dynamic imports', () =>
);
});

test('readWorkspacePackages reads tracked manifests only', () => {
// R11's own committed-state property, and the source-level half of the #1965 review finding.
// `readWorkspacePackages` used to enumerate `packages/` with `readdirSync`, so an uncommitted
// scratch package contributed a name, export targets, and dependency edges to every rule built
// on it — R11 could fail on work a contributor had not committed. Filtering the OUTPUT of
// façade discovery hides that from one caller; this closes it for all of them.
const repo = fs.mkdtempSync(path.join(os.tmpdir(), 'package-boundaries-tracked-manifests-'));
const committed = path.join(repo, 'packages/committed');
fs.mkdirSync(path.join(committed, 'src'), { recursive: true });
fs.writeFileSync(
path.join(committed, 'package.json'),
JSON.stringify({ name: '@agent-device/committed', exports: { '.': './src/index.ts' } }),
);
fs.writeFileSync(path.join(committed, 'src/index.ts'), 'export const a = 1;\n');
execFileSync('git', ['init', '-q'], { cwd: repo });
execFileSync('git', ['add', '.'], { cwd: repo });
execFileSync(
'git',
['-c', 'user.name=Gate', '-c', 'user.email=gate@example.test', 'commit', '-qm', 'base'],
{ cwd: repo },
);

const scratch = path.join(repo, 'packages/scratch');
fs.mkdirSync(path.join(scratch, 'src'), { recursive: true });
fs.writeFileSync(
path.join(scratch, 'package.json'),
JSON.stringify({
name: '@agent-device/scratch',
exports: { '.': './src/index.ts' },
dependencies: { '@agent-device/committed': 'workspace:*' },
}),
);
fs.writeFileSync(path.join(scratch, 'src/index.ts'), 'export const b = 2;\n');

const names = readWorkspacePackages(repo).map((pkg) => pkg.name);
assert.deepEqual(names, ['@agent-device/committed']);
assert.ok(
!names.includes('@agent-device/scratch'),
'an uncommitted package directory is not part of the committed state R11 describes',
);
});

test('every workspace package façade names its exports explicitly (no bare `export *`)', () => {
// #1574 built a hand-maintained pin table (`facade-symbols.ts`, 816 symbols across every
// workspace-package façade) plus a ~200-line star-chain resolver (`readFacadeExports`) whose
// entire job was enumerating what `export *` hides. Once a façade names its exports explicitly,
// the façade file itself IS the pin — a widening shows up in the diff of the file that grew,
// not in a table two files away that only a gate failure would surface. This structural gate is
// what keeps that property true: every façade a package manifest declares (`exportTargets`),
// plus every file under a `packages/*/src/facades/` directory, must parse through
// plus every production file under a `src/facades/` directory, must parse through
// `readNamedExports` without hitting the bare-`export *`/`export default` rejection it already
// implements — reusing that check rather than writing a second, regex-based one that would have
// to independently rediscover every export form to be trustworthy.
const packages = readWorkspacePackages(repoRoot);
const facadeFiles = new Set<string>(packages.flatMap((pkg) => [...pkg.exportTargets.values()]));
for (const file of listSourceFiles()) {
if (file.includes('/src/facades/')) facadeFiles.add(file);
}
assert.ok(facadeFiles.size > 0, 'expected at least one workspace package façade to check');
for (const file of [...facadeFiles].sort()) {
//
// The façade set comes from `facadeEntryFiles`, the single owner of "what is an entry surface".
// The ADR-0019 eager-closure budget table consumes the same function, so a file this gate holds
// to an explicit export list is necessarily a file that gate holds to a loading-shape budget.
const facadeFiles = facadeEntryFiles(repoRoot);
assert.ok(facadeFiles.length > 0, 'expected at least one workspace package façade to check');
for (const file of facadeFiles) {
const source = fs.readFileSync(path.join(repoRoot, file), 'utf8');
try {
readNamedExports(source);
Expand Down
54 changes: 49 additions & 5 deletions scripts/layering/package-boundaries.ts
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@
import fs from 'node:fs';
import path from 'node:path';
import { parseImports } from './model.ts';
import { listTrackedPackageManifests, listTrackedProductionSources } from './tracked-sources.ts';

export type PackageBoundaryViolation = {
rule: string;
Expand Down Expand Up @@ -54,13 +55,21 @@ export function specifierSites(file: string, source: string): SpecifierSite[] {
return parseImports(source).map((edge) => ({ file, line: edge.line, specifier: edge.spec }));
}

/**
* Every workspace package a gate may reason about, read from TRACKED manifests only.
*
* A `readdirSync` of `packages/` would also pick up a directory a contributor created but never
* committed, and its `exports` map would then contribute entry surfaces to R11 and to the
* ADR-0019 loading-shape budgets -- gates whose whole claim is that they describe committed state
* (#1965 review). R13's `readTrackedPlatformPackageDeclarations` already enumerated its manifests
* this way; this closes the same hole for every workspace package, at the source rather than by
* filtering the output.
*/
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;
for (const manifestFile of listTrackedPackageManifests(repoRoot).sort()) {
const entry = path.posix.basename(path.posix.dirname(manifestFile));
const manifestPath = path.join(repoRoot, manifestFile);
const manifest = JSON.parse(fs.readFileSync(manifestPath, 'utf8')) as {
name?: string;
private?: boolean;
Expand Down Expand Up @@ -255,6 +264,41 @@ export function rootExternalDependencyRanges(repoRoot: string): Map<string, stri
return new Map(Object.entries(manifest.dependencies ?? {}));
}

/**
* Every workspace-package entry surface, repo-root-relative and sorted: whatever a package
* manifest's `exports` map points at, plus every production source file under a `src/facades/`
* directory.
*
* The single owner of that question. R11's façade gates and the ADR-0019 eager-closure budget
* table (`src/__tests__/eager-closure-budgets.ts`) both consume this, so the two cannot drift
* into disagreeing about what counts as a façade — a gate that scanned a narrower set would
* silently exempt files the other one covers, which is exactly the hole #1960 review found (a
* one-level `readdir` missed both nested façade files and the six `packages/platform-*`
* manifest façades, which have no `facades/` directory at all).
*
* The `src/facades/` side reads TRACKED production sources (`listTrackedProductionSources`), the
* same input every other layering scan uses, and is recursive so a nested façade cannot be
* covered by one gate and missed by another. Tracked-only matters: an uncommitted scratch file
* under a scanned path must stay invisible, or these gates start describing a contributor's
* working directory instead of the committed tree (#1965 review).
*/
export function facadeEntryFiles(repoRoot: string): string[] {
const tracked = new Set(listTrackedProductionSources(repoRoot));
const found = new Set<string>();
// Manifests are already tracked-only, but a tracked manifest's WORKING-TREE content can name a
// target that is not committed yet, so the targets are intersected too. Both origins go through
// the same tracked set: every path this returns is committed, whatever produced it.
for (const pkg of readWorkspacePackages(repoRoot)) {
for (const target of pkg.exportTargets.values()) {
if (tracked.has(target)) found.add(target);
}
}
for (const file of tracked) {
if (file.includes('/src/facades/')) found.add(file);
}
return [...found].filter((file) => fs.existsSync(path.join(repoRoot, file))).sort();
}

function walkTsFiles(repoRoot: string, relativeDir: string): string[] {
const absolute = path.join(repoRoot, relativeDir);
if (!fs.existsSync(absolute)) return [];
Expand Down
64 changes: 64 additions & 0 deletions scripts/layering/tracked-sources.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,64 @@
// The file list every layering scan reads: TRACKED production TypeScript, nothing else.
//
// A leaf module on purpose. `check.ts` owns the scan and imports `package-boundaries.ts`, so the
// boundary rules cannot import `check.ts` back for its file list; without a shared leaf the two
// would each grow their own enumerator, and the moment those disagree a rule silently changes
// scope. That is not hypothetical -- it is the #1965 review finding this module exists to fix:
// `facadeEntryFiles` was briefly implemented as a raw `readdir` walk, which quietly widened R11
// from tracked files to whatever happened to be on disk.
//
// Tracked-only is a hard rule, not an optimization. A layering gate describes COMMITTED state:
// `docs/adr/0019-request-bound-platform-runtime.md` requires review "from a clean committed tree
// with all production files present in HEAD", and `check.ts` separately fails closed when
// production TypeScript is untracked. A scratch file a contributor has not committed -- a
// throwaway `facades/experiment.ts`, a half-finished module -- must be invisible here, or a gate
// that is supposed to describe the repository starts failing on the contents of someone's working
// directory instead.

import { execFileSync } from 'node:child_process';

// `src/**/*.ts` only matches NESTED files, so root-level `src/*.ts` (src/cli.ts,
// src/command-catalog.ts) needs its own pathspec or it silently drops out of every scan.
// Workspace package sources are production files too (#1490 W0).
const TRACKED_SOURCE_PATHSPECS = [
'src/*.ts',
'src/**/*.ts',
'packages/*/src/*.ts',
'packages/*/src/**/*.ts',
];

/**
* Every tracked `packages/<pkg>/package.json`, repo-root-relative.
*
* The manifest half of the same rule. A package directory a contributor has created but not
* committed declares no entry surfaces as far as any gate is concerned -- otherwise scratch work
* changes what R11 and the loading-shape budgets police (#1965 review, second tracked-only pass).
* `platform-package-repository.ts` already reads its manifests this way for R13; this is the same
* enumeration widened to every workspace package.
*/
export function listTrackedPackageManifests(repoRoot: string): string[] {
const out = execFileSync('git', ['ls-files', 'packages/*/package.json'], {
cwd: repoRoot,
encoding: 'utf8',
});
return out.split('\n').filter(Boolean);
}

/** Every tracked `.ts` source file under the scanned roots, repo-root-relative. */
export function listTrackedTypeScriptFiles(repoRoot: string): string[] {
const out = execFileSync('git', ['ls-files', ...TRACKED_SOURCE_PATHSPECS], {
cwd: repoRoot,
encoding: 'utf8',
});
return out.split('\n').filter(Boolean);
}

/** Production sources only: test files and `__tests__/` trees are not layering subjects. */
export function isProductionSourceFile(file: string): boolean {
return file.endsWith('.ts') && !/(?:^|\/)__tests__\//.test(file) && !/\.test\.ts$/.test(file);
}

/** The canonical layering scan input: tracked, production, repo-root-relative. */
export function listTrackedProductionSources(repoRoot: string): string[] {
return listTrackedTypeScriptFiles(repoRoot).filter(isProductionSourceFile);
}
Loading
Loading