Skip to content

Commit b1d9ca9

Browse files
dmealingclaude
andcommitted
fix(gate): the subpath gate must compare against the DECLARED target, not just find a file
Six findings from `/code-review high` on the previous commit. The one that mattered: the gate checked that `src/<subpath>` resolves and never compared that result to what the package's `bun` condition declares — so tsc and the runtime could read DIFFERENT modules and the gate stayed green. Proven by reverting `./library`'s `bun` to `library-sources.ts` with `index.ts` still present: the old gate passed while `paths` read the index and bun read the other file. That is the type-level form of the identity defect `tsconfig.scripts.json`'s own rationale is written against, and following `library/index.ts`'s instruction to re-export new modules there would have typechecked clean and been `undefined` at runtime. The gate now resolves both sides and compares them by realpath; the same mutation now fails it, naming both files. Three more holes in the gate itself, all of which let something escape unchecked: a subpath declaring no `bun` condition was skipped rather than failed, though it is precisely a TS subpath the scripts typecheck cannot reach through source; a subpath PATTERN (`./templates/*`) was treated as a literal and would have reported a false miss against a correct package; and a `paths` target not shaped `<pkgRoot>/src/*` made the derived manifest path miss, which `continue`d silently and dropped that whole package from the gate. Patterns are now skipped explicitly, the other two fail loudly. And a premise correction, in three headers and the commit before this one. "A directory deeper than the substitution can reach" is WRONG: `paths` `*` matches across `/`, which is why `@metaobjectsdev/codegen-ts/templates/entity-file` resolves to `src/templates/entity-file.ts` without trouble. The actual rule is that the subpath NAME must mirror the layout under `src/` — `vocabulary-rewrite-yaml` named a module living at `src/core/vocabulary-rewrite-yaml.ts`. Stated the wrong way, the next maintainer concludes a nested module can never back a subpath and flattens files out of `core/` for no reason. Renaming the export to `./core/vocabulary-rewrite-yaml` would have satisfied the rule more cheaply; the shim stays because that subpath is public API of a published package. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011cQxyVuTkdPmAduLiedNEg
1 parent 8502803 commit b1d9ca9

4 files changed

Lines changed: 135 additions & 46 deletions

File tree

‎server/typescript/packages/cli/test/library-manifest-resolved.test.ts‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -193,9 +193,9 @@ describe("every library manifest fact is resolved against the thing it claims",
193193
// pattern that silently stops matching would make every containment check above
194194
// vacuously true, which is the way a gate of this shape stops gating.
195195
expect(Object.keys(EMBEDS).sort(), "a port with no embed mapping").toEqual([...SERVER_LANGS].sort());
196-
// De-duplicated: the right-hand side comes from a Set, so a ref legitimately declared
197-
// by two layers of one library would otherwise fail every port on a set the embeds
198-
// carry correctly.
196+
// De-duplicated: this is the side built from manifests, and it is compared against a
197+
// set extracted from each embed — so a ref legitimately declared by two layers of one
198+
// library would otherwise fail every port against embeds that are correct.
199199
const allRefs = [...new Set(NAMES.flatMap(declaredRefs))].sort();
200200
expect(allRefs.length, "no library declares any ref").toBeGreaterThan(0);
201201

‎server/typescript/packages/cli/test/subpath-resolves-under-scripts-paths.test.ts‎

Lines changed: 112 additions & 32 deletions
Original file line numberDiff line numberDiff line change
@@ -1,22 +1,29 @@
1-
// Gate — every exports-map subpath must resolve under `tsconfig.scripts.json`'s `paths`.
1+
// Gate — every exports-map subpath must resolve, under `tsconfig.scripts.json`'s `paths`,
2+
// to THE SAME MODULE the package declares.
23
//
34
// That file maps `@metaobjectsdev/<pkg>/*` to `<pkg>/src/*` so the repo-root `scripts/`
4-
// typecheck reads workspace SOURCE rather than build output. The substitution resolves a
5-
// subpath the way TypeScript resolves any module: `src/<name>.ts`, or `src/<name>/index.ts`
6-
// if `<name>` is a directory. A subpath whose `package.json` entry points somewhere else —
7-
// `./src/core/vocabulary-rewrite-yaml.ts`, say, which is a directory deeper than the
8-
// substitution can reach — has nothing to land on.
5+
// typecheck reads workspace SOURCE rather than build output. The `*` matches across `/`,
6+
// so nesting is fine — `./templates/entity-file` substitutes to
7+
// `src/templates/entity-file.ts` and is correct. The rule is narrower than "no nesting":
8+
// the subpath NAME must mirror the layout under `src/`. `./vocabulary-rewrite-yaml` whose
9+
// source sits at `src/core/vocabulary-rewrite-yaml.ts` breaks it, because the substitution
10+
// yields `src/vocabulary-rewrite-yaml` and that is not where the module is.
911
//
1012
// Missing is not the same as failing. tsc falls through to `node_modules`, finds the
1113
// package's own `dist/**/*.d.ts`, and compiles clean — on a machine that has built. The
1214
// `gates` lane runs `bun install` and never builds, so `dist/` is absent there and the same
1315
// specifier is `TS2307`. Green locally, red on CI, and only for whoever imports it first.
16+
// `./library` shipped in exactly that state and took the lane down after the FR-043 merge.
1417
//
15-
// That is not hypothetical: `./library` shipped in exactly this state and took the lane
16-
// down after the FR-043 merge. This gate is the general form of that fix, because the same
17-
// mistake is available to every subpath added from here on.
18+
// Resolving to SOMETHING is not enough, which is the second half. If `paths` lands on one
19+
// module and the `bun` condition names another, tsc and the runtime disagree silently —
20+
// the type-level form of the cross-package identity defect `tsconfig.scripts.json`'s own
21+
// rationale is written against. Following `src/library/index.ts`'s instruction to
22+
// re-export new modules there, while `bun` still pointed at `library-sources.ts`, would
23+
// typecheck clean and be `undefined` at runtime. So both are asserted: the substitution
24+
// must land, and it must land on the declared file.
1825
import { describe, test, expect } from "bun:test";
19-
import { existsSync, readFileSync } from "node:fs";
26+
import { existsSync, readFileSync, realpathSync } from "node:fs";
2027
import { dirname, join } from "node:path";
2128

2229
function findRepoRoot(start: string): string {
@@ -30,14 +37,38 @@ function findRepoRoot(start: string): string {
3037
}
3138

3239
const ROOT = findRepoRoot(import.meta.dir);
40+
const MODULE_EXTS = [".ts", ".tsx", ".d.ts", ".js", ".jsx"] as const;
3341

34-
/** What TypeScript will try for a `paths` substitution that lands on `candidate`. */
35-
function resolvesAsModule(candidate: string): boolean {
36-
const asFile = [".ts", ".tsx", ".d.ts", ".js", ".jsx"].some((ext) => existsSync(candidate + ext));
37-
const asDir = [".ts", ".tsx", ".d.ts", ".js", ".jsx"].some((ext) =>
38-
existsSync(join(candidate, `index${ext}`)),
39-
);
40-
return asFile || asDir;
42+
/** Where TypeScript lands for a `paths` substitution of `candidate`, or undefined. */
43+
function resolveModule(candidate: string): string | undefined {
44+
for (const ext of MODULE_EXTS) if (existsSync(candidate + ext)) return candidate + ext;
45+
for (const ext of MODULE_EXTS) {
46+
const idx = join(candidate, `index${ext}`);
47+
if (existsSync(idx)) return idx;
48+
}
49+
return undefined;
50+
}
51+
52+
type ExportTarget = string | Record<string, unknown>;
53+
54+
/**
55+
* The TypeScript SOURCE a subpath declares, by this repo's convention that the `bun`
56+
* condition names it. Undefined for an export that is not TypeScript source at all — an
57+
* asset like `./form.css`, whose target is the stylesheet and which a bundler resolves and
58+
* tsc never does.
59+
*
60+
* A conditions object that declares no `bun` is NOT silently skipped; it comes back as
61+
* `null` so the caller can fail it. Swallowing that is how a broken TS subpath would
62+
* escape a gate written to catch broken TS subpaths.
63+
*/
64+
function declaredSource(target: ExportTarget): string | undefined | null {
65+
if (typeof target === "string") return /\.tsx?$/.test(target) ? target : undefined;
66+
const bun = target["bun"];
67+
if (typeof bun === "string") return /\.tsx?$/.test(bun) ? bun : undefined;
68+
// No `bun` condition. If nothing here looks like a module at all, treat it as an asset;
69+
// otherwise it is a TS export the scripts typecheck cannot reach through source.
70+
const anyModule = JSON.stringify(target).includes(".js") || JSON.stringify(target).includes(".ts");
71+
return anyModule ? null : undefined;
4172
}
4273

4374
interface Wildcard {
@@ -76,46 +107,95 @@ describe("every exports-map subpath resolves under the scripts typecheck's paths
76107
expect(entries.length, "no @metaobjectsdev/*/* wildcard entries found").toBeGreaterThan(10);
77108
});
78109

79-
test("no declared subpath falls through to node_modules", () => {
110+
test("every mapped package's manifest is findable", () => {
111+
// `pkgDir` assumes the target is `<pkgRoot>/src/*`. A target a segment deeper would
112+
// point this at a directory with no `package.json`, and skipping it quietly would drop
113+
// that package from the gate with no signal — so it fails here instead.
114+
const unfindable = entries
115+
.filter(({ pkgDir }) => !existsSync(join(ROOT, pkgDir, "package.json")))
116+
.map(({ pkg, pkgDir }) => `${pkg} — no package.json at ${pkgDir}`);
117+
expect(unfindable, "a paths entry whose package root could not be derived").toEqual([]);
118+
});
119+
120+
test("each subpath lands, and lands on the module the package declares", () => {
80121
const misses: string[] = [];
122+
const diverged: string[] = [];
123+
const unreachable: string[] = [];
81124
let checked = 0;
82125

83126
for (const { pkg, srcDir, pkgDir } of entries) {
84127
const manifestPath = join(ROOT, pkgDir, "package.json");
85-
if (!existsSync(manifestPath)) continue; // a mapping with no package is its own problem
128+
if (!existsSync(manifestPath)) continue; // reported by the test above
86129
const manifest = JSON.parse(readFileSync(manifestPath, "utf8")) as {
87-
exports?: Record<string, string | Record<string, string>>;
130+
exports?: Record<string, ExportTarget>;
88131
};
132+
89133
for (const [sub, target] of Object.entries(manifest.exports ?? {})) {
90134
// `.` is the root entry, which `paths` maps separately (without the `/*`).
91135
// `./package.json` is resolved by `require.resolve`, never as a type-level import.
92136
if (sub === "." || sub === "./package.json") continue;
93-
// Only TypeScript modules are subject to this. An ASSET export — `./form.css`,
94-
// whose target is the stylesheet itself — is resolved by a bundler, never by tsc,
95-
// so `paths` having nothing to offer it is correct rather than a defect.
96-
const source = typeof target === "string" ? target : (target.bun ?? target.types ?? "");
97-
if (!/\.tsx?$/.test(source)) continue;
137+
// A subpath PATTERN stands for many modules, so "does it resolve" has no single
138+
// answer; whatever it expands to is covered by the files themselves.
139+
if (sub.includes("*")) continue;
140+
141+
const source = declaredSource(target);
142+
if (source === undefined) continue; // asset export — not tsc's to resolve
143+
if (source === null) {
144+
unreachable.push(`${pkg}${sub.slice(1)} — declares no \`bun\` condition naming source`);
145+
continue;
146+
}
147+
98148
checked++;
99-
if (!resolvesAsModule(join(ROOT, srcDir, sub.slice(2)))) {
149+
const viaPaths = resolveModule(join(ROOT, srcDir, sub.slice(2)));
150+
if (viaPaths === undefined) {
100151
misses.push(
101152
`${pkg}${sub.slice(1)} — paths yields "${join(srcDir, sub.slice(2))}", ` +
102-
`and neither that file nor an index.ts under it exists`,
153+
`and neither that file nor an index under it exists`,
154+
);
155+
continue;
156+
}
157+
const declaredAbs = join(ROOT, pkgDir, source.replace(/^\.\//, ""));
158+
if (!existsSync(declaredAbs)) {
159+
misses.push(`${pkg}${sub.slice(1)} — declared source "${source}" does not exist`);
160+
continue;
161+
}
162+
if (realpathSync(viaPaths) !== realpathSync(declaredAbs)) {
163+
diverged.push(
164+
`${pkg}${sub.slice(1)} — paths reads "${viaPaths.slice(ROOT.length + 1)}" ` +
165+
`but \`bun\` declares "${source}"`,
103166
);
104167
}
105168
}
106169
}
107170

108171
expect(checked, "no subpaths were checked at all").toBeGreaterThan(0);
172+
109173
expect(
110174
misses,
111175
"A declared subpath that `tsconfig.scripts.json` cannot resolve to source.\n" +
112-
"tsc will fall through to node_modules and read the package's built `dist/`, so this\n" +
113-
"compiles on a machine that has built and is TS2307 on a fresh CI checkout, where the\n" +
176+
"tsc falls through to node_modules and reads the package's built `dist/`, so this\n" +
177+
"compiles where someone has built and is TS2307 on a fresh CI checkout, where the\n" +
114178
"`gates` lane runs `bun install` and never builds.\n" +
115-
"Fix the package: point the subpath at `src/<name>.ts` or `src/<name>/index.ts`.\n" +
116-
"Do NOT add a per-subpath entry to `tsconfig.scripts.json` — the mapping is the\n" +
117-
"contract, and widening it per package is how it stops being one.\n " +
179+
"Fix the package: the subpath NAME must mirror the layout under `src/` (nesting is\n" +
180+
"fine — the `*` matches across `/`). Do NOT add a per-subpath entry to\n" +
181+
"`tsconfig.scripts.json`; the mapping is the contract.\n " +
118182
misses.join("\n "),
119183
).toEqual([]);
184+
185+
expect(
186+
diverged,
187+
"A subpath where tsc and the runtime read DIFFERENT modules. Types would be taken\n" +
188+
"from one file and values from another — an export added to only one of them\n" +
189+
"typechecks clean and is `undefined` at runtime.\n " +
190+
diverged.join("\n "),
191+
).toEqual([]);
192+
193+
expect(
194+
unreachable,
195+
"A TypeScript subpath with no `bun` condition. This repo's convention is that `bun`\n" +
196+
"names the source file; without it the scripts typecheck has only `dist/` to read,\n" +
197+
"which is the failure this gate exists to prevent.\n " +
198+
unreachable.join("\n "),
199+
).toEqual([]);
120200
});
121201
});

‎server/typescript/packages/metadata/src/library/index.ts‎

Lines changed: 5 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1,9 +1,11 @@
11
// @metaobjectsdev/metadata/library — the shipped-library surface (FR-043).
22
//
33
// The invariant is not "every subpath has an index" — `./constants` is a plain
4-
// `src/constants.ts` and is fine. It is that the subpath must land on something the
5-
// `paths` substitution can reach. This one pointed at `library-sources.ts` under a
6-
// directory, so it reached nothing, and that was not cosmetic: the repo-root
4+
// `src/constants.ts` and is fine — nor is it "no nesting", since the `paths` `*` matches
5+
// across `/`. It is that the subpath NAME must mirror the layout under `src/`. A subpath
6+
// named `library` substitutes to `src/library`, so the module has to be `src/library.ts`
7+
// or `src/library/index.ts`; it was `src/library/library-sources.ts`, which the
8+
// substitution never reaches. That was not cosmetic: the repo-root
79
// `tsconfig.scripts.json` maps `@metaobjectsdev/metadata/*` to
810
// `packages/metadata/src/*` so that `scripts/` typechecks against workspace
911
// SOURCE rather than a build output. With no `index.ts` here that mapping had
Lines changed: 15 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -1,14 +1,21 @@
11
// @metaobjectsdev/metadata/vocabulary-rewrite-yaml — the YAML arm of `meta upgrade`.
22
//
33
// The implementation stays under `core/`, beside the canonical-JSON rewriter it mirrors.
4-
// This file exists so the SUBPATH has something to resolve to: `tsconfig.scripts.json`
5-
// maps `@metaobjectsdev/metadata/*` to `src/*`, and a subpath declared as
6-
// `src/core/vocabulary-rewrite-yaml.ts` is a directory deeper than that substitution can
7-
// reach. Without an entry here, tsc misses, falls through to `node_modules`, and reads
8-
// `dist/` — which exists on a machine that has built and not on a fresh CI checkout, where
9-
// the `gates` lane runs `bun install` and never builds.
4+
// This file exists so the SUBPATH NAME mirrors the layout under `src/`, which is what
5+
// `tsconfig.scripts.json`'s `@metaobjectsdev/metadata/*` → `src/*` mapping requires.
106
//
11-
// `./library` shipped in exactly that state and took the lane down; this one had not been
12-
// imported from anywhere `scripts/` typechecks yet, so it was latent rather than red.
7+
// Nesting is NOT the problem — the `*` matches across `/`, which is why
8+
// `@metaobjectsdev/codegen-ts/templates/entity-file` resolves to
9+
// `src/templates/entity-file.ts` perfectly well. The problem was a subpath NAMED
10+
// `vocabulary-rewrite-yaml` whose source sat at `src/core/vocabulary-rewrite-yaml.ts`:
11+
// the substitution yields `src/vocabulary-rewrite-yaml`, and that is not where the module
12+
// is. Renaming the export to `./core/vocabulary-rewrite-yaml` would also have satisfied
13+
// the rule and was the cheaper edit, but this package is published and that subpath is
14+
// public API, so the name stays and the layout moves to meet it.
15+
//
16+
// When the mapping misses, tsc falls through to `node_modules` and reads `dist/` — present
17+
// for anyone who has built, absent on a fresh CI checkout where the `gates` lane runs
18+
// `bun install` and never builds. `./library` shipped that way and took the lane down;
19+
// this one had not been imported from anywhere `scripts/` typechecks, so it was latent.
1320
// `cli/test/subpath-resolves-under-scripts-paths.test.ts` now gates the class.
1421
export { rewriteYamlDocument, type YamlRewriteResult } from "./core/vocabulary-rewrite-yaml.js";

0 commit comments

Comments
 (0)