Skip to content

Commit f105bbf

Browse files
committed
fix(layering): exact-shape include pin and honest dual-load wording
Cubic re-review on 63fd297: - P2: the per-package include pin filtered `*.global.d.ts` by suffix, so an unrelated include in that shape class drifted past the compiled-set predicate silently. The pin now compares exact include ARRAYS against the two shapes that exist today ([src] and replay-port/session-journal's command-registry global.d.ts sibling); extending it is a deliberate edit that says whether insideCompiledSources must widen too. Mutation: adding a made-up src/generated-env.global.d.ts include to xml goes red. - P3: the escape branch's rationale claimed every tunnel duplicates the module; duplication needs the SAME module also loaded through its package specifier (and type-only tunnels cost nothing at runtime). Doc block and violation message now state the mixing/dual-route condition, matching the file header's existing precise phrasing.
1 parent 63fd297 commit f105bbf

2 files changed

Lines changed: 27 additions & 11 deletions

File tree

‎scripts/layering/package-boundaries.ts‎

Lines changed: 12 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -179,11 +179,15 @@ export function insideCompiledSources(file: string): boolean {
179179
* the redirect to kernel's `dist-types` keeps every emitted file under host-kit's `rootDir`,
180180
* so neither TS6059 nor TS6307 fires (the root-escape tunnel DOES fail: TS2307, since `../..
181181
* /../kernel/...` lands outside every include and no package owns `kernel` at that depth).
182-
* What the sibling tunnel breaks is runtime identity — Node loads kernel once by path and
183-
* once by specifier, so two AppError classes coexist and `instanceof` silently fails. That
184-
* is invisible to any compiler. The remaining resolution failures (unknown package,
185-
* subpath the exports map does not name) are owned by NodeNext for compiled files with
186-
* TS2307; uncompiled files never reach either check, so for them R11 holds every branch.
182+
* What the sibling tunnel risks is runtime identity: Node's ESM loader does not realpath
183+
* specifiers, so when the same module is ALSO loaded through its package specifier — the
184+
* normal route for every other consumer — the two keys instantiate it twice in one process
185+
* (duplicate AppError, broken `instanceof`). Type-only tunnels carry no runtime cost, but
186+
* the branch cannot condition on that from syntax alone and the layering claim fails either
187+
* way. Any duplicate-instance risk invisible to the compiler is R11's to hold; the
188+
* remaining resolution failures (unknown package, subpath the exports map does not name)
189+
* are owned by NodeNext for compiled files with TS2307; uncompiled files never reach either
190+
* check, so for them R11 holds every branch.
187191
*/
188192
export function checkPackageInternalSites(
189193
pkg: WorkspacePackage,
@@ -204,8 +208,9 @@ export function checkPackageInternalSites(
204208
line: site.line,
205209
message:
206210
`'${site.specifier}' escapes ${pkg.dir}/ — a workspace package may not reach ` +
207-
`outside its own directory. Import a sibling through its specifier (Node would ` +
208-
`load the tunnelled module twice), or move the shared code below ${pkg.dir}.`,
211+
`outside its own directory. Import a sibling through its specifier (mixing the ` +
212+
`two routes instantiates the module twice), or move the shared code below ` +
213+
`${pkg.dir}.`,
209214
});
210215
}
211216
continue;

‎scripts/layering/project-references.test.ts‎

Lines changed: 15 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -72,13 +72,24 @@ test('the compiled-set predicate is licensed by the tsconfig include/exclude sha
7272
'scripts/help-conformance-command-validator.ts',
7373
'packages/command-registry/src/global.d.ts',
7474
]);
75+
// `src` everywhere, plus the one shared ambient-declaration entry replay-port and
76+
// session-journal include. Exact include ARRAYS, not a `global.d.ts` suffix class: a
77+
// new include of any name must extend this allow-list deliberately, or the drift fails
78+
// the test (#3289 cubic review P2 — a suffix filter would silently accept unrelated
79+
// `*.global.d.ts` additions, which is the drift this pin exists to catch).
80+
const knownIncludeShapes = [
81+
JSON.stringify(['src']),
82+
JSON.stringify(['src', '../command-registry/src/global.d.ts']),
83+
];
7584
for (const pkg of readWorkspacePackages(repoRoot)) {
7685
const config = readTsconfig(`${pkg.dir}/tsconfig.json`);
7786
assert.equal(config.exclude, undefined, `${pkg.dir}/tsconfig.json must not declare exclude`);
78-
assert.deepEqual(
79-
(config.include ?? ['**/*']).filter((entry) => !entry.endsWith('global.d.ts')),
80-
['src'],
81-
`${pkg.dir}/tsconfig.json must keep including exactly src/`,
87+
const include = JSON.stringify(config.include ?? ['**/*']);
88+
assert.ok(
89+
knownIncludeShapes.includes(include),
90+
`${pkg.dir}/tsconfig.json includes ${include}; the compiled-set predicate assumes ` +
91+
`the known shapes ${knownIncludeShapes.join(' / ')} — extend the allow-list here ` +
92+
'only after widening insideCompiledSources to match',
8293
);
8394
}
8495
});

0 commit comments

Comments
 (0)