Skip to content

Derive workspace groups; check bundle identity on the dist target - #797

Merged
sroussey merged 1 commit into
claude/coverage-dist-bundle-fix-ew0vj8from
claude/optimistic-goldberg-onotd9-coverage-derivation
Aug 15, 2026
Merged

Derive workspace groups; check bundle identity on the dist target#797
sroussey merged 1 commit into
claude/coverage-dist-bundle-fix-ew0vj8from
claude/optimistic-goldberg-onotd9-coverage-derivation

Conversation

@sroussey

Copy link
Copy Markdown
Collaborator

Stacked on #741 (claude/coverage-dist-bundle-fix-ew0vj8). Three findings from reviewing that PR, fixed together because they share a root cause: a hand-copied list of workspace groups, and a coverage denominator that was not derived from it.

H1 — bundle-identity checking is lost for every non-unit tier

Seven vitest jobs run, and only test-vitest-dist sets WORKGLOW_TEST_TARGET=dist. It runs the unit tier. Every integration tier now resolves source, so nothing anywhere checks that the built bundles are wired correctly beyond "the entry loads and exports at least one symbol".

The mechanism is CLASS identity, not registry identity

The review premise named a registry-identity check. That check would be vacuously green — do not ask for it. packages/util/src/di/Container.ts stashes globalContainer on Symbol.for("@workglow/util/di/globalContainer") precisely so duplicated bundle copies resolve one instance, and createServiceToken returns a plain string id. A duplicated copy of @workglow/ai therefore resolves the same registry; a === assertion on it can never fail.

What actually breaks under inlining is class identity, exactly as packages/task-graph/src/test-entry.ts documents it. registerAnthropicInline constructs new AnthropicQueuedProvider(...) from a relative import inside the ai-runtime module graph, while that class extends a base built from AiProvider imported by specifier. Inline @workglow/ai into ai-runtime.js — a bundler flag, a dropped external, an export * rewritten to export { … } from — and the constructed instance stops being instanceof the AiProvider every consumer holds, while every existing check stays green.

Two new UNIT-tier files, zero new CI jobs

Both run inside the two existing unit jobs (seconds against the current 232s step). Crucially, register*Inline needs no API key: it constructs the provider and calls registerProviderInline, which is registry bookkeeping and a strategy resolver — no network.

  • PublishedEntryIdentity.test.ts — for every package publishing both ./ai and ./ai-runtime (16 today), imports <pkg>/ai-runtime, finds its register*Inline export, calls it, and asserts every provider it registered is an instanceof an AiProvider that @workglow/ai publishes and serves ≥1 run function.
  • PublishedEntryExportParity.test.ts — imports each published entry alongside the source module it was built from and compares the export name sets. A bundle that lost a re-export still resolves and still evaluates, so the existing > 0 bound passes over a bundle carrying one symbol out of ninety; names are the only observable that says the entry is intact.

Both files note in-file that under the default source target they pass trivially, so a green source run is not a bundle check — the run that means something is test-vitest-dist.

Verified non-vacuous

Rebuilt @workglow/anthropic's ai-runtime with @workglow/ai inlined instead of external, then ran the dist target:

AssertionError: these providers are not an instanceof any AiProvider that
@workglow/ai publishes, ...
+ [ "@workglow/anthropic -> ANTHROPIC (via registerAnthropicInline)" ]

Bundle restored; the check is green again.

Why not an integration-tier dist job

Rejected on cost, not minutes. That tier makes 131 live-API calls per run, including a 53.8s two-image OpenAI generation — a second run doubles vendor spend indefinitely. And it buys nothing on fork PRs, where secrets.WORKGLOW_SECRETS_PASSPHRASE is unavailable, so describe.skipIf means register() never runs at all.

Two things found along the way

  • @workglow/ai publishes two AiProvider classes. . and ./worker are separate bun build invocations, so each bundle carries its own copy, and the five providers with a worker runtime (chrome-ai, tf-mediapipe, cactus, huggingface-transformers, node-llama-cpp) extend the worker copy on purpose. A consumer holding AiProvider from @workglow/ai and testing a Chrome AI provider with instanceof gets false today. That seam is pre-existing, intentional, and belongs to the @workglow/ai build rather than to this check, so the test accepts either published class and pins the count at ≤2. The third copy — one inlined into a provider's own bundle — matches neither and is what fails.
  • The NEEDS_NATIVE_RUNTIME exemption map ships EMPTY. The four providers it was expected to need (node-llama-cpp, huggingface-transformers, cactus, stable-diffusion-server) all register cleanly with no native runtime present: every SDK, binding and server probe sits behind a run-fn, reached only by an actual generation call. Exempting them would have been a false statement costing five of sixteen candidates their coverage. The map stays as the seam, with the staleness guard, so a provider that genuinely cannot register has somewhere to go.

M1 — the workspace group list was hardcoded in four places

WORKSPACE_GROUPS (scripts/lib/workspaceSource.ts), a verbatim copy in PublishedEntryImports.test.ts, PACKAGE_GROUPS (scripts/lib/testDiscovery.ts), and the coverage include globs. The root workspaces field was read by nothing but scripts/lib/util.ts. Adding a group to package.json therefore silently no-oped everywhere: nothing errors, the walks just never look.

New scripts/lib/workspaceGroups.ts owns the derivation and nothing else — neither existing module should, for the same reason testDiscovery.ts gives for owning its own ("Both must agree, so neither owns it"). Node-portable, no Bun.Glob, because vitest.config.ts loads it under Node. It throws on a pattern that does not reduce to exactly one scannable directory, rather than guessing a prefix and recreating the silent no-op.

PublishedEntryImports.test.ts cannot import from scripts/ — its own comment explains why (packages/test is a composite project rooted at ./src; importing from scripts/ pulls those files into its program and breaks build-types). It re-derives in three lines from the same field, with a comment saying it is duplicated derivation code, not a duplicated list. The two new test files do the same.

M2 — examples/web diluted the denominator

Confirmed: publishConfig.access: "none", exports: {}, no main/bin. The justifying comments at vitest.config.ts and scripts/workspaceSource.test.ts claimed all three example packages were published and non-private — factually wrong, and both are rewritten.

WorkspacePackage grows a publishes field (false only for access: "none"), and non-publishing workspaces are subtracted in coverage.exclude by package path, so the group globs stay derived and the exception carries its own reason. The gate is access: "none", not private: packages/test, providers/aws and providers/cloudflare are all private: true, and aws/cloudflare have real suites that must stay counted.

One correction to the premise: examples/web/src holds 37 .ts/.tsx files, of which 3 are tests (nodeUsage.test.ts, TaskNode.test.ts, UsageStatus.test.ts) — not zero, as stated. It has a test script and a web vitest project. That does not change the decision (the denominator is published API, and this package publishes none), only the wording, and no comment in this PR claims otherwise.

Also included

  • coverage.exclude splices coverageConfigDefaults.exclude, not configDefaults.exclude — which is vitest's test-file list. It is [] in vitest 4 while configDefaults.exclude is not, so the swap would have silently dropped **/node_modules/** and **/.git/**; both are now stated explicitly. configDefaults is kept for shared.exclude, which genuinely is a test-file list.
  • **/testing/** dropped from coverage.exclude. Those 11 files are published API (@workglow/task-graph/test, @workglow/util/test — repository contracts, shared fake tasks, testing logger). **/__tests__/** already removes the one test file among them.
  • listWorkspacePackages's bare catch now continues only on ENOENT (the ordinary "not a package dir" case) and re-throws anything else naming the path — a JSON syntax error or EACCES silently dropped a package, which makes the source-rewrite plugin no-op for it and collapses that one package's coverage back onto dist/*.
  • The workspace scan and the plugin are hoisted out of the per-project .map(). One shared, stateless instance instead of 12 projects × 41 manifests ≈ 500 file reads at config load. workspaceSourcePlugin takes an optional second parameter defaulting to a fresh scan, so no caller changes.

Skipped deliberately: Windows separator portability. The repo is POSIX-only by construction (rm -rf in clean/build-clean, for f in …; do in typecheck:tests, bun engines), so fixing separators in these files would be misleading maintenance for a platform nothing else supports.

Tests

The line-95 coverage guard becomes tautological once both sides derive, so it is replaced. Its original requirement — "must fail when a group is added to package.json and nowhere else" — is no longer satisfiable literally, because with derivation nothing else needs to change. The guard's real job is to fail when the derivation is broken or bypassed:

  1. derives the groups from the root workspaces field, not from a list — parses package.json independently inside the test; fails the moment the array is hardcoded back.
  2. finds at least one package in every declared workspace group — fires on a mishandled pattern or a typo'd directory.
  3. builds the coverage denominator from those same groups — reads the real config and compares coverage.include exactly, so a hand-edited glob is caught.
  4. Fixture test via mkdtemp: ["./packages/*","./integrations/*"]["packages","integrations"]; ["./a/**/c"] throws.
  5. distinguishes a malformed manifest from a non-package directory.
  6. keeps packages that publish nothing out of the denominator — asserts the examples/web/src exclusion and re-reads examples/web/package.json to assert publishConfig.access === "none", so the exception dies with the property that justifies it.
  7. counts every package that does publish — no access: "public" package's src is excluded.
  8. attaches one plugin instance to every project — reference equality across all projects, the only thing stopping the hoist being undone.

⚠️ The reported coverage number will move

Do not read the PR coverage comment as a regression. Dropping examples/web's 37 files from the denominator raises the figure; un-excluding the 11 published testing/ files nudges it. Both are corrections to what the denominator should have been.

Verification (actual output)

$ npx vitest run scripts/workspaceSource.test.ts \
    packages/test/src/test/util/PublishedEntryImports.test.ts \
    packages/test/src/test/util/PublishedEntryIdentity.test.ts \
    packages/test/src/test/util/PublishedEntryExportParity.test.ts
 Test Files  4 passed (4)
      Tests  239 passed (239)

$ bun run build:packages
 Tasks:    81 successful, 81 total
  Time:    1m51.543s

$ WORKGLOW_TEST_TARGET=dist npx vitest run \
    packages/test/src/test/util/PublishedEntryIdentity.test.ts \
    packages/test/src/test/util/PublishedEntryExportParity.test.ts \
    packages/test/src/test/util/PublishedEntryImports.test.ts
 Test Files  3 passed (3)
      Tests  206 passed (206)

$ bun scripts/test.ts --check-sections
Every test file is discovered and reachable by section+kind selection.

$ npx eslint            # `bun run lint` is not a root script
ESLINT_EXIT=0

The dist run is the one that matters for H1: it needs the built bundles and no API keys.


Generated by Claude Code

Three findings from the coverage-derivation review, fixed together because
they all come from the same place: a hand-copied list of workspace groups
and a denominator that was not derived from it.

H1 — bundle-identity checking was lost for every non-unit tier. Only
`test-vitest-dist` sets `WORKGLOW_TEST_TARGET=dist`, and it runs the unit
tier, so nothing checks that the built bundles are wired correctly beyond
"the entry loads and exports something". The failure that hides there is
CLASS identity: a provider's `register*Inline` constructs its provider class
from a relative import while that class extends `AiProvider` imported by
specifier, so inlining `@workglow/ai` into `<provider>/ai-runtime.js`
produces an object that is no longer `instanceof` the base every consumer
holds — and nothing goes red.

Two new UNIT-tier files strengthen the dist sweep rather than adding a CI
job (`register*Inline` needs no API key, so this is affordable where it
already runs):

- `PublishedEntryIdentity.test.ts` registers every provider publishing both
  `./ai` and `./ai-runtime` and asserts each registered provider is an
  instanceof an `AiProvider` that `@workglow/ai` publishes, and serves at
  least one run function. Verified non-vacuous by rebuilding
  `@workglow/anthropic`'s `ai-runtime` with `@workglow/ai` inlined: the
  check fails, naming the package.
- `PublishedEntryExportParity.test.ts` imports each published entry
  alongside the source it was built from and compares export NAME sets. A
  bundle that lost a re-export still resolves and evaluates, so the existing
  "exports something" bound passes over it.

Asserting on the SERVICE registry instead would have been vacuous: the
global DI container is stashed on a `Symbol.for` key so duplicated bundle
copies share one instance, and tokens are plain strings.

M1 — the workspace group list was written out by hand in four places.
`scripts/lib/workspaceGroups.ts` now derives it from the root manifest's
`workspaces` field (Node-portable, since `vitest.config.ts` loads it under
Node) and throws on a pattern that does not reduce to one scannable
directory. `WORKSPACE_GROUPS` is gone; `PACKAGE_GROUPS` and the coverage
`include` globs derive from it. `PublishedEntryImports.test.ts` cannot
import from `scripts/` (composite project rooted at `./src`), so it
re-derives locally, with a comment saying why.

M2 — `examples/web` diluted the denominator. It declares
`publishConfig.access: "none"`, `exports: {}`, and no `main`/`bin`: none of
its source is published API. `WorkspacePackage` grows a `publishes` field
and non-publishing workspaces are subtracted from `coverage.exclude` by
path. The gate is `access: "none"`, NOT `private` — `packages/test`,
`providers/aws` and `providers/cloudflare` are private and the latter two
carry real suites. The two comments justifying the old behavior were
factually wrong and are rewritten.

Also:

- `coverage.exclude` splices `coverageConfigDefaults.exclude`, not
  `configDefaults.exclude` (a test-file list); the two entries the latter
  was silently supplying are now stated.
- `**/testing/**` dropped from `coverage.exclude`: those 11 files are
  published API (`@workglow/task-graph/test`, `@workglow/util/test`).
- `listWorkspacePackages` re-throws a non-ENOENT manifest error naming the
  path instead of dropping the package silently.
- The workspace scan and the source-resolving plugin are hoisted out of the
  per-project map — one shared, stateless instance instead of ~500 manifest
  reads at config load.

The line-95 coverage guard was tautological once both sides derive, so it is
replaced by tests that fail when the derivation is bypassed, when a declared
group scans to nothing, when the denominator stops matching those groups,
when the plugin stops being shared, and when the `examples/web` exclusion
outlives the property that justifies it.

Reported coverage numbers will move: dropping `examples/web`'s 37 files
raises the figure, un-excluding the 11 published `testing/` files nudges it.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01UW1Qr5mxetAQr61YKEY9nz
@sroussey
sroussey merged commit 758c0b2 into claude/coverage-dist-bundle-fix-ew0vj8 Aug 15, 2026
11 of 12 checks passed
sroussey added a commit that referenced this pull request Aug 16, 2026
Three findings from the coverage-derivation review, fixed together because
they all come from the same place: a hand-copied list of workspace groups
and a denominator that was not derived from it.

H1 — bundle-identity checking was lost for every non-unit tier. Only
`test-vitest-dist` sets `WORKGLOW_TEST_TARGET=dist`, and it runs the unit
tier, so nothing checks that the built bundles are wired correctly beyond
"the entry loads and exports something". The failure that hides there is
CLASS identity: a provider's `register*Inline` constructs its provider class
from a relative import while that class extends `AiProvider` imported by
specifier, so inlining `@workglow/ai` into `<provider>/ai-runtime.js`
produces an object that is no longer `instanceof` the base every consumer
holds — and nothing goes red.

Two new UNIT-tier files strengthen the dist sweep rather than adding a CI
job (`register*Inline` needs no API key, so this is affordable where it
already runs):

- `PublishedEntryIdentity.test.ts` registers every provider publishing both
  `./ai` and `./ai-runtime` and asserts each registered provider is an
  instanceof an `AiProvider` that `@workglow/ai` publishes, and serves at
  least one run function. Verified non-vacuous by rebuilding
  `@workglow/anthropic`'s `ai-runtime` with `@workglow/ai` inlined: the
  check fails, naming the package.
- `PublishedEntryExportParity.test.ts` imports each published entry
  alongside the source it was built from and compares export NAME sets. A
  bundle that lost a re-export still resolves and evaluates, so the existing
  "exports something" bound passes over it.

Asserting on the SERVICE registry instead would have been vacuous: the
global DI container is stashed on a `Symbol.for` key so duplicated bundle
copies share one instance, and tokens are plain strings.

M1 — the workspace group list was written out by hand in four places.
`scripts/lib/workspaceGroups.ts` now derives it from the root manifest's
`workspaces` field (Node-portable, since `vitest.config.ts` loads it under
Node) and throws on a pattern that does not reduce to one scannable
directory. `WORKSPACE_GROUPS` is gone; `PACKAGE_GROUPS` and the coverage
`include` globs derive from it. `PublishedEntryImports.test.ts` cannot
import from `scripts/` (composite project rooted at `./src`), so it
re-derives locally, with a comment saying why.

M2 — `examples/web` diluted the denominator. It declares
`publishConfig.access: "none"`, `exports: {}`, and no `main`/`bin`: none of
its source is published API. `WorkspacePackage` grows a `publishes` field
and non-publishing workspaces are subtracted from `coverage.exclude` by
path. The gate is `access: "none"`, NOT `private` — `packages/test`,
`providers/aws` and `providers/cloudflare` are private and the latter two
carry real suites. The two comments justifying the old behavior were
factually wrong and are rewritten.

Also:

- `coverage.exclude` splices `coverageConfigDefaults.exclude`, not
  `configDefaults.exclude` (a test-file list); the two entries the latter
  was silently supplying are now stated.
- `**/testing/**` dropped from `coverage.exclude`: those 11 files are
  published API (`@workglow/task-graph/test`, `@workglow/util/test`).
- `listWorkspacePackages` re-throws a non-ENOENT manifest error naming the
  path instead of dropping the package silently.
- The workspace scan and the source-resolving plugin are hoisted out of the
  per-project map — one shared, stateless instance instead of ~500 manifest
  reads at config load.

The line-95 coverage guard was tautological once both sides derive, so it is
replaced by tests that fail when the derivation is bypassed, when a declared
group scans to nothing, when the denominator stops matching those groups,
when the plugin stops being shared, and when the `examples/web` exclusion
outlives the property that justifies it.

Reported coverage numbers will move: dropping `examples/web`'s 37 files
raises the figure, un-excluding the 11 published `testing/` files nudges it.

Claude-Session: https://claude.ai/code/session_01UW1Qr5mxetAQr61YKEY9nz

Co-authored-by: Claude <noreply@anthropic.com>
@sroussey
sroussey deleted the claude/optimistic-goldberg-onotd9-coverage-derivation branch August 24, 2026 18:48
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