Skip to content

test: prove the dist→src rewrite is in effect, and make the dist job total - #778

Merged
sroussey merged 2 commits into
claude/coverage-dist-bundle-fix-ew0vj8from
claude/optimistic-goldberg-hxoj5s-coverage-guard
Aug 14, 2026
Merged

test: prove the dist→src rewrite is in effect, and make the dist job total#778
sroussey merged 2 commits into
claude/coverage-dist-bundle-fix-ew0vj8from
claude/optimistic-goldberg-hxoj5s-coverage-guard

Conversation

@sroussey

@sroussey sroussey commented Aug 14, 2026

Copy link
Copy Markdown
Collaborator

Targets claude/coverage-dist-bundle-fix-ew0vj8 (#741), not main.

Two review findings, both the same shape: a guard that does not guard what it claims.

1. Nothing verified the dist→src rewrite is actually in effect

resolveId — the one function the whole plugin exists for — had no test, and nothing asserted the plugin reaches the generated projects. workspaceSource.test.ts covered only the pure helpers. Both CI modes pass under either resolution: test-vitest-unit and test-vitest-dist run the same files with the same assertions, differing only in which files load, so nothing distinguished "resolved to src" from "resolved to dist".

The failure that leaves is silent. Hoisting plugins: [workspaceSourcePlugin(__dirname)] from the per-project object to the root defineConfig — a natural "one instance instead of N" cleanup, and the exact mistake the comment there warns about — makes every project resolve through exports to dist again. All tests pass, merge-vitest-coverage succeeds, and the only symptom is entry-point behavior reading as uncovered.

Extraction rather than casts. The hook body moves to resolveWorkspaceSourceId(packages, context, source, importer, options), which takes the plugin context as a parameter so a recording stub can stand in for Vite; workspaceSourcePlugin becomes a one-line adapter. Two details keep the call site cast-free, and I checked both against the installed vite typings rather than assuming:

  • WorkspaceResolveContext.resolve uses method syntax, so parameter bivariance lets Vite's real PluginContext satisfy the interface structurally.
  • The interface is generic in the result, so the adapter's return type stays ResolvedId. Non-generic with external?: boolean | string, the adapter fails to typecheck (Type 'string' is not assignable to type '"absolute" | "relative" | boolean | undefined') and would need the cast the extraction exists to avoid.

New describe("resolveId") with a recording stub covers: the dist→src rewrite; a non-workspace specifier short-circuiting without calling resolve; an external resolution left as-is; dist output with no source twin left on the built file; the workspace diagnostic thrown when resolution yields nothing (a branch with zero execution anywhere before this); and skipSelf: true plus verbatim forwarding of the hook's own kind/isEntry options.

New describe("plugin attachment") re-imports the real vitest.config.ts (with vi.resetModules(), since the config reads the variable at module scope) and asserts every project carries workglow:workspace-source under the default target and none does under dist. Both directions stub WORKGLOW_TEST_TARGET explicitly and vi.unstubAllEnvs() aftertest-vitest-dist runs this file with that variable ambient, so a test reading the ambient value would pass in one CI job and fail in the other.

2. The blocking bundle check was tier-shaped; two entries had no check at all

test-vitest-dist runs test:vitest:unit only, while the source rewrite applies to every vitest job. The integration/rag/provider suites used to load the bundles and now load src, so a bundle reachable only from an .integration.test.ts file lost its blocking check. Two lost every check — @workglow/openrouter/ai-runtime and @workglow/huggingface-inference/ai-runtime, imported only from provider-api integration files whose section the nightly Bun parity run also excludes. A bun build change dropping registerOpenRouterInline from providers/openrouter/dist/ai-runtime.js would leave the file in place, satisfy the dist-must-exist requirement, pass all of CI, and break consumers only after publish.

New packages/test/src/test/util/PublishedEntryImports.test.ts makes the check total instead of tier-shaped: it enumerates every workspace manifest's exports, resolves each subpath under the Node conditions only (walked in declaration order the way Node does, so types/browser/bun are stepped over rather than entered), and it.each dynamically imports each resulting specifier, asserting the module is non-empty. Under WORKGLOW_TEST_TARGET=dist that one unit-tier file loads every published bundle; under the default target it costs nothing, since it loads the same source the rest of the suite already does.

Adding "workglow": "workspace:*" to packages/test's devDependencies is the larger half — it brings the meta-package's own 28 entries and, transitively, the provider bundles those re-export. Total swept today: 98 entries across 38 packages, 97 imported (@workglow/cli exempt).

The review suggested generating this from stubSpecsFor(manifest). That does not work: it returns dist targets, not import specifiers, and packages/test is a composite project rooted at ./src, so importing from scripts/lib/ would put those files in its program and break build-types. The enumeration is therefore local.

Anti-vacuity assertions (over 60 entries across over 20 packages, every target matching ^\./dist/.+\.js$) keep a mis-typed walk from passing as a short list, and both exemption maps are staleness-checked against the enumeration. Two exemptions, each with its reason:

entry kind reason
@workglow/cli UNCHECKABLE an example app packages/test does not depend on — under isolated linking the specifier does not resolve from here at all
workglow/auto-bootstrap SIDE_EFFECT_ONLY still imported; only the non-empty assertion is lifted, since it registers providers as a side effect and exports nothing by design

New packages default to checked.

3. Docs

The .claude/CLAUDE.md paragraph this branch adds now says why the unit-tier dist job suffices — naming PublishedEntryImports.test.ts and the tier-shaped gap it closes — and documents the resolveId/attachment tests alongside it.

Verification

  • Full WORKGLOW_TEST_TARGET=dist bun run test:vitest:unit against a real bun run build: 534 files passed / 3 skipped, 6152 tests passed / 71 skipped, exit 0. No OOM; the sweep adds ~11s. That run is what proves all 97 entries load as bundles, not just as source.
  • Go-red on the review's exact scenario: replacing providers/openrouter/dist/ai-runtime.js with an export-less module (file still present) fails @workglow/openrouter/ai-runtime loads and workglow/openrouter/runtime loads, and nothing else — AssertionError: expected 0 to be greater than 0. Before this PR no CI job imported either.
  • bun scripts/test.ts scripts vitest — 4 files, 38 passed.
  • Go-red on finding 1: hoisting plugins to the root defineConfig fails "attaches the source rewrite to every project" and nothing else.
  • Typechecked scripts/lib/workspaceSource.ts against the installed vite typings (scripts/ is in no CI tsconfig, and vite does not resolve from the root, so the branch's existing import type { Plugin } from "vite" is effectively any there — pinned with a temporary paths mapping instead of trusting that).
  • Full bun run build — 84/84 tasks, so adding workglow to packages/test introduces no turbo cycle and build-types still passes.

🤖 Generated with Claude Code

claude added 2 commits August 14, 2026 15:50
`resolveId` — the one function the workspace-source plugin exists for — had
no test, and nothing asserted the plugin reaches the generated projects.
`workspaceSource.test.ts` covered only the pure helpers, and both CI modes
pass under either resolution: `test-vitest-unit` and `test-vitest-dist` run
the same files and the same assertions, differing only in which files load,
so no suite distinguished "resolved to src" from "resolved to dist".

The failure that leaves is silent. Hoisting `plugins:
[workspaceSourcePlugin(__dirname)]` from the per-project object to the root
`defineConfig` — a natural "one instance instead of N" cleanup, and the
exact mistake the comment there warns about — makes every project resolve
`@workglow/*` through `exports` to dist again. All tests pass,
merge-vitest-coverage succeeds, and the only symptom is entry-point
behavior reading as uncovered. The same silence covers a regression inside
`resolveId`.

Extract the hook body into `resolveWorkspaceSourceId(packages, context,
source, importer, options)`, which takes the plugin context as a parameter
so a recording stub can stand in for Vite; `workspaceSourcePlugin` becomes
a one-line adapter. `WorkspaceResolveContext` declares `resolve` with
method syntax on purpose — parameter bivariance is what lets Vite's real
`PluginContext` satisfy it — and is generic in the result so the adapter's
return type stays `ResolvedId`, which is what Vite's `resolveId` hook is
declared to return. Both together are what keep the call site cast-free
(checked against the installed vite typings, where the non-generic form
needs one).

New tests: the rewrite, a non-workspace specifier short-circuiting before
`this.resolve`, an external resolution left alone, dist output with no
source twin left on the built file, the unresolved-specifier diagnostic
(a branch with zero execution anywhere before this), and `skipSelf: true`
plus verbatim forwarding of the hook's own options. Plus a "plugin
attachment" block that re-imports the real `vitest.config.ts` and asserts
every project carries `workglow:workspace-source` under the default target
and none does under `dist`.

Both directions stub `WORKGLOW_TEST_TARGET` explicitly and unstub
afterwards: `test-vitest-dist` runs this file with that variable ambient,
so a test reading the ambient value would pass in one CI job and fail in
the other.
`test-vitest-dist` is the one blocking job that resolves `@workglow/*`
through `exports`, but it runs the UNIT tier only — while the source
rewrite applies to EVERY vitest job. The integration/rag/provider suites
previously loaded the bundles and now load src, so a bundle reachable only
from an `.integration.test.ts` file lost its blocking check.

Two entries lost every check: `@workglow/openrouter/ai-runtime` and
`@workglow/huggingface-inference/ai-runtime`, imported only from
provider-api integration files, whose section the nightly Bun parity run
also excludes. Concretely: a `bun build` change dropping
`registerOpenRouterInline` from `providers/openrouter/dist/ai-runtime.js`
leaves the file in place, satisfies the dist-must-exist requirement, passes
all of CI, and breaks consumers only after publish.

`PublishedEntryImports.test.ts` makes the check total instead of
tier-shaped: it enumerates every workspace manifest's `exports`, resolves
each subpath under the Node conditions only (`node`/`import`/`default`,
walked in declaration order the way Node does, so `types`/`browser`/`bun`
are stepped over rather than entered), and dynamically imports each
resulting specifier, asserting the module is non-empty. Under
`WORKGLOW_TEST_TARGET=dist` that one unit-tier file loads every published
bundle; under the default target it costs nothing, since it loads the same
source the rest of the suite already does.

Adding `workglow` to `packages/test`'s devDependencies is the larger half:
it brings the meta-package's own entries and, transitively, the provider
bundles those re-export.

The enumeration is local rather than shared with `scripts/lib/sourceStubs`:
`stubSpecsFor` returns dist targets rather than import specifiers, and
`packages/test` is a `composite` project rooted at `./src`, so importing
from `scripts/` would put those files in its program and break
`build-types`.

Anti-vacuity assertions (over 60 entries across over 20 packages, every
target `./dist/**.js`) keep a mis-typed walk from passing as a short list,
and both exemption maps are staleness-checked against the enumeration. Two
exemptions, each with its reason: `@workglow/cli` (uncheckable — an example
app `packages/test` does not depend on, so under isolated linking the
specifier does not resolve from here at all) and `workglow/auto-bootstrap`
(imported, but exempt from the non-empty assertion: it registers providers
as a side effect and exports nothing by design). New packages default to
checked.
@github-actions

Copy link
Copy Markdown

Coverage Report

Status Category Percentage Covered / Total
🔵 Lines 70.49% 29214 / 41444
🔵 Statements 69.34% 31246 / 45060
🔵 Functions 70.32% 5933 / 8437
🔵 Branches 60.45% 15991 / 26450
File CoverageNo changed files found.
Generated in workflow #3081 for commit d75ce7d by the Vitest Coverage Report Action

@sroussey
sroussey merged commit 05f3b38 into claude/coverage-dist-bundle-fix-ew0vj8 Aug 14, 2026
12 checks passed
@sroussey
sroussey deleted the claude/optimistic-goldberg-hxoj5s-coverage-guard branch August 14, 2026 16:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants