diff --git a/.claude/CLAUDE.md b/.claude/CLAUDE.md index b50b278ac..2b776e8c9 100644 --- a/.claude/CLAUDE.md +++ b/.claude/CLAUDE.md @@ -396,6 +396,17 @@ and rewrites only the RESULT, `/dist/.js` → `/src/.ts` package, every subpath export, and every package added later is covered with no list to maintain, and unlike `use-source` it writes nothing into `dist`. +Rewriting the result rather than aliasing has one consequence worth knowing: resolution +still goes through `exports`, which point at `./dist/*`, so **`dist/.js` must exist +even though the plugin immediately rewrites it to `src`**. When it does not, resolution +fails before the rewrite can happen. The plugin therefore throws its own error naming the +specifier, the owning package and the remedy — and branches on whether `/dist` holds +any built entries, because "never built, run `bun run build`" and "a new `exports` subpath +was added without rebuilding, so `dist` is stale" call for different actions and the second +reads as wrong advice to anyone looking at a populated `dist` directory. (`bun run clean` +and `use-dist --no-build` both leave an EMPTY `dist` behind, which is the first case, not +the second.) + This is what makes the numbers mean anything. `packages/test` reaches what it exercises by package specifier, so with bundles in play v8 attributes those executed lines to `packages/ai/dist/node.js` and `packages/ai/src/**` reads as barely covered — a package diff --git a/scripts/lib/workspaceSource.ts b/scripts/lib/workspaceSource.ts index 82f99c2f7..580b8b7ee 100644 --- a/scripts/lib/workspaceSource.ts +++ b/scripts/lib/workspaceSource.ts @@ -96,6 +96,71 @@ export function distToSource(id: string): string | undefined { return undefined; } +/** + * The workspace package a bare specifier belongs to, or `undefined` when none + * owns it. + * + * Matching is on the package BOUNDARY, not on string prefix: `@workglow/util` + * owns `@workglow/util/schema` but not a hypothetical `@workglow/utilities`. + */ +export function ownerOf( + packages: readonly WorkspacePackage[], + source: string +): WorkspacePackage | undefined { + return packages.find((pkg) => source === pkg.name || source.startsWith(`${pkg.name}/`)); +} + +/** + * The diagnostic for a workspace specifier that resolved to nothing. + * + * Worth building by hand because the default is actively misleading: this + * plugin rewrites the RESULT of resolution, so resolution itself still goes + * through the package's `exports`, which point at `./dist/*`. With no built + * entry there, resolution fails before the rewrite can happen and the error + * blames the manifest, naming neither this plugin nor anything to do about it. + * + * `distHasBuiltEntries` is passed in rather than probed here so the message + * stays a pure function of its inputs. The two branches need opposite + * responses, which is why they are not one sentence: an empty or absent `dist` + * means the package has simply never been built, whereas a POPULATED `dist` + * that still does not carry this entry is the "added an `exports` subpath and + * did not rebuild" case — where "run build" on its own reads as wrong advice to + * someone looking at a directory full of bundles. + */ +export function unresolvedWorkspaceMessage( + source: string, + owner: WorkspacePackage, + distHasBuiltEntries: boolean, + importer: string | undefined +): string { + const from = importer === undefined ? "" : ` (imported from ${importer})`; + const remedy = distHasBuiltEntries + ? `${owner.dir}/dist carries built entries but none for this specifier. A new "exports" ` + + `subpath was most likely added without rebuilding, so dist is stale rather than absent: ` + + `re-run \`bun run build\` (or \`bun run use-source\`).` + : `${owner.dir}/dist is missing or empty — ${owner.name} has never been built in this ` + + `checkout. Run \`bun run build\`, or \`bun run use-source\` to write source stubs into dist.`; + return ( + `[workglow:workspace-source] cannot resolve "${source}"${from}. ` + + `It is owned by the workspace package ${owner.name}. Resolution goes through that ` + + `package's "exports", which point at ./dist/*, so the built entry has to exist even ` + + `though this plugin then rewrites it to src. ${remedy}` + ); +} + +/** + * Whether a package's `dist` holds anything at all. `bun run clean` and + * `use-dist --no-build` both leave the directory in place but empty, which is + * "never built" rather than "stale". + */ +function hasBuiltEntries(packageDir: string): boolean { + try { + return readdirSync(join(packageDir, "dist")).length > 0; + } catch { + return false; + } +} + /** * Redirect workspace package imports from `dist` to `src`. * @@ -105,9 +170,7 @@ export function distToSource(id: string): string | undefined { * resolution in an alias table is what a per-package fix would have to do. */ export function workspaceSourcePlugin(root: string): Plugin { - const names = listWorkspacePackages(root).map((p) => p.name); - const ownsSpecifier = (source: string): boolean => - names.some((name) => source === name || source.startsWith(`${name}/`)); + const packages = listWorkspacePackages(root); return { name: "workglow:workspace-source", @@ -116,9 +179,17 @@ export function workspaceSourcePlugin(root: string): Plugin { // Bare workspace specifiers only: a relative import already points at // source, and resolving every third-party specifier twice would tax the // whole run for nothing. - if (!ownsSpecifier(source)) return null; + const owner = ownerOf(packages, source); + if (owner === undefined) return null; const resolved = await this.resolve(source, importer, { ...options, skipSelf: true }); - if (!resolved || resolved.external) return resolved; + if (!resolved) { + // Throwing, not warning: an unresolvable @workglow/* specifier already + // fails the run a moment later. This only replaces the message. + throw new Error( + unresolvedWorkspaceMessage(source, owner, hasBuiltEntries(owner.dir), importer) + ); + } + if (resolved.external) return resolved; const sourceFile = distToSource(resolved.id); return sourceFile === undefined ? resolved : { ...resolved, id: sourceFile }; }, diff --git a/scripts/workspaceSource.test.ts b/scripts/workspaceSource.test.ts index c6247915b..afb3b206c 100644 --- a/scripts/workspaceSource.test.ts +++ b/scripts/workspaceSource.test.ts @@ -9,7 +9,13 @@ import { join } from "node:path"; import { describe, expect, it } from "vitest"; import { stubSpecsFor, type PackageManifest } from "./lib/sourceStubs"; import { ROOT } from "./lib/testDiscovery"; -import { distToSource, listWorkspacePackages, WORKSPACE_GROUPS } from "./lib/workspaceSource"; +import { + distToSource, + listWorkspacePackages, + ownerOf, + unresolvedWorkspaceMessage, + WORKSPACE_GROUPS, +} from "./lib/workspaceSource"; const packages = listWorkspacePackages(ROOT); @@ -84,4 +90,63 @@ describe("workspace source resolution", () => { ); expect(missing).toEqual([]); }); + + /** + * `resolveId` itself needs Vite's plugin context to drive, so the owner + * lookup and the message are separated out and tested directly. The lookup is + * what makes an actionable message possible at all: the old plugin kept only + * package NAMES, so at the point resolution failed it could not say which + * package's `dist` to look at. + */ + describe("owner lookup", () => { + it("attributes a subpath specifier to its package", () => { + expect(ownerOf(packages, "@workglow/util/schema")?.name).toBe("@workglow/util"); + expect(ownerOf(packages, "@workglow/util")?.name).toBe("@workglow/util"); + }); + + it("matches on the package boundary, not a string prefix", () => { + // `@workglow/util` is a string prefix of this, but the specifier belongs + // to no package — attributing it would point the diagnostic at an + // unrelated directory. + expect(ownerOf(packages, "@workglow/utilities")).toBeUndefined(); + expect(ownerOf(packages, "vitest")).toBeUndefined(); + }); + }); + + describe("unresolved specifier diagnostic", () => { + const owner = { name: "@workglow/ai", dir: "/repo/packages/ai" }; + + it("names the specifier, the owning package and the importer", () => { + const message = unresolvedWorkspaceMessage( + "@workglow/ai/worker", + owner, + false, + "/repo/providers/hft/src/x.ts" + ); + expect(message).toContain("@workglow/ai/worker"); + expect(message).toContain("@workglow/ai"); + expect(message).toContain("/repo/providers/hft/src/x.ts"); + expect(message).toContain("workglow:workspace-source"); + }); + + // The two cases need opposite responses, so they must not read the same. + it("distinguishes a never-built package from a stale dist", () => { + const neverBuilt = unresolvedWorkspaceMessage("@workglow/ai/worker", owner, false, undefined); + const staleDist = unresolvedWorkspaceMessage("@workglow/ai/worker", owner, true, undefined); + + expect(neverBuilt).toContain("missing or empty"); + expect(neverBuilt).toContain("never been built"); + // "run build" alone would read as wrong advice to someone looking at a + // populated dist directory, so the stale case has to say why. + expect(staleDist).toContain("carries built entries but none for this specifier"); + expect(staleDist).toContain("stale rather than absent"); + expect(staleDist).not.toContain("never been built"); + }); + + it("omits the importer clause when there is no importer", () => { + expect(unresolvedWorkspaceMessage("@workglow/ai", owner, true, undefined)).not.toContain( + "imported from" + ); + }); + }); });