From 02e3cdb330c001a75c01d28ad8b57f48a4544f0a Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 13 Aug 2026 03:44:37 +0000 Subject: [PATCH] fix(test): explain an unresolvable workspace specifier MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The source-redirect 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, Vite's resolution fails first, `this.resolve` yields nothing, the plugin returns null, and the run dies with a generic `Cannot find package '@workglow/ai/worker' imported from …` that blames the manifest and names neither the plugin nor anything to do about it. Keeps the workspace list as WorkspacePackage rather than bare names, so the owning directory is available at the point resolution fails, and throws a message naming the specifier, the owner, the importer and the remedy. Throwing rather than warning is right: an unresolvable @workglow/* specifier already fails the run, so this replaces a misleading message with an actionable one. The remedy branches on whether the owner's dist holds any built entries, since "never built" and "a new exports subpath was added without rebuilding" call for different actions and the second reads as wrong advice to someone looking at a populated dist. An empty dist directory — what `bun run clean` and `use-dist --no-build` both leave behind — counts as never built. ownerOf and unresolvedWorkspaceMessage are separated out as pure functions because resolveId needs Vite's plugin context to drive and cannot be unit tested; the message was additionally verified end to end by emptying dist and running a suite. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01H797qbH356jjznKgUax63o --- .claude/CLAUDE.md | 11 +++++ scripts/lib/workspaceSource.ts | 81 +++++++++++++++++++++++++++++++-- scripts/workspaceSource.test.ts | 66 ++++++++++++++++++++++++++- 3 files changed, 152 insertions(+), 6 deletions(-) diff --git a/.claude/CLAUDE.md b/.claude/CLAUDE.md index e7d559e8e..18efcc263 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 0eeef134f..da3a5d995 100644 --- a/scripts/workspaceSource.test.ts +++ b/scripts/workspaceSource.test.ts @@ -9,7 +9,12 @@ 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 } from "./lib/workspaceSource"; +import { + distToSource, + listWorkspacePackages, + ownerOf, + unresolvedWorkspaceMessage, +} from "./lib/workspaceSource"; const packages = listWorkspacePackages(ROOT); @@ -61,4 +66,63 @@ describe("workspace source resolution", () => { expect(distToSource(invented)).toBeUndefined(); expect(distToSource(join(ROOT, "packages/ai/src/node.ts"))).toBeUndefined(); }); + + /** + * `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" + ); + }); + }); });