Run the dist stub guard in a plugin hook; widen it; anchor the coverage globs - #827
Merged
sroussey merged 2 commits intoAug 17, 2026
Conversation
…import `vitest.config.ts` called `assertNoSourceStubs` as a MODULE-LEVEL side effect whenever the target was `dist`, and `scripts/workspaceSource.test.ts` imports that module with `WORKGLOW_TEST_TARGET` stubbed to `dist` to check plugin attachment. `scripts/*.test.ts` is unit-tier, so on a `use-source` tree an ordinary `bun run test:vitest:unit` died there with advice to run `use-dist` — undoing the documented no-build dev mode the run had never left. CI stayed green because CI trees are really built. Two further call sites in the same file import the config with AMBIENT env, so under `test:vitest:dist` they re-ran the whole scan too. The check is now a `configResolved` hook on a guard plugin attached to every project under `dist`. Vitest resolves each project's config at startup, so the run still dies before any suite can report a pass over stubs; the verdict is computed once and re-thrown, so one scan serves all twelve projects instead of printing a 41-entry message twelve times. Rejected alternatives: `buildStart` (vitest does not reliably drive build hooks for a test-only server, while `configResolved` is guaranteed by `resolveConfig` per project); exporting `buildProjects(target)` (leaves the config module itself un-exercised, which is what makes the attachment test worth having); an early check in `scripts/test.ts` (misses `vitest run --project <n>` and adds a second place that knows the rule). Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HJRf3YFa8DjmjsZvXz8xDT
… anchor coverage globs Four related follow-ups to the dist-target work. **The parity sweep is skipped under the source target.** Under `source` the resolving plugin rewrites `import(specifier)` to exactly the path `sourceCounterpart()` computes, so both sides are the SAME module and ~90 cases assert `X === X`. They now skip, so the report distinguishes "checked" from "not applicable" instead of showing 97 green rows that compared nothing. Nothing stops being loaded: `PublishedEntryImports.test.ts` imports every published specifier unconditionally and does carry signal under source. An anti-vacuity test asserts the target is one of the two known values, so a broken `test.env` cannot make every case skip and leave `test-vitest-dist` green having compared nothing. **The stub guard now covers every entry `use-source` writes.** It read only `exports`, but `use-source` also stubs `bin` targets, and two are named by no export entry at all: `examples/eval` declares NO `exports`, only a `bin`, and `examples/cli`'s `bin` is not among its export targets. Both were stubbed and unguarded, while `publish-workspaces.ts` (a full dist walk) did refuse them. `guardedDistTargets` adds them — deliberately not via `stubSpecsFor`, which also maps each target to a source counterpart and throws for a target that is neither `.js` nor `.d.ts`; the guard has no use for that mapping and should not acquire a new way to fail during config resolution. **An unreadable entry is unproven, not built.** `probeSourceStub` returns three states, because the two callers fail safe in opposite directions: removal must not delete an artifact it could not read, while the dist guard must not accept one as a real bundle. The guard's message has a section for each. **Coverage globs are anchored at the repo root.** A `--project` run — every package's own `test` script, and `turbo run test -- --coverage` — starts in a package directory, so a relative pattern's meaning depends on which directory the provider resolves it against. Measured on `@vitest/coverage-v8` 4.1.10 the two forms give an identical denominator from both the repo root and a package directory, so this is defensive rather than a fix for an observed miss; it is worth pinning because a denominator that silently loses its untested half reads as a BETTER number, not as an error. The per-package `/src/**` exclusions are anchored to match, or they subtract from nothing. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HJRf3YFa8DjmjsZvXz8xDT
Coverage Report
File CoverageNo changed files found. |
sroussey
merged commit Aug 17, 2026
0cd71d6
into
claude/coverage-dist-bundle-fix-ew0vj8
12 checks passed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Follow-ups to #741. The PR itself is sound — the
dist→srcmapping is TOTAL over all 153 runtime targets of all 41 workspaces, the coverageinclude/excludesemantics were verified against unpackedvitest@4.1.10and@vitest/coverage-v8@4.1.10, and there are no security issues. These are two follow-up commits.Commit 1 — the stub guard runs in a hook, not on config import
vitest.config.tscalledassertNoSourceStubs(workspacePackages)as a module-level side effect whenever the target wasdist, andscripts/workspaceSource.test.tsdeliberately re-imports that module withvi.stubEnv("WORKGLOW_TEST_TARGET","dist").scripts/*.test.tsis unit-tier, so on ause-sourcetree an ordinarybun run test:vitest:unitdied there, advising the developer to runuse-dist— undoing the documented no-build dev mode the run had never left. CI stayed green because CI trees are really built. Worse than reported: two further call sites in the same file import the config with ambient env, so undertest:vitest:distthey re-ran the whole scan too.Reproduced and fixed, on a genuinely stubbed tree:
Why a hook over the three alternatives.
buildStart— vitest does not reliably drive build hooks for a test-only server, whileconfigResolvedis guaranteed byresolveConfigper project. ExportingbuildProjects(target)— leaves the config module itself un-exercised, which is what makes the attachment test worth having. An early check inscripts/test.ts— missesvitest run --project <n>and adds a second place that knows the rule.The guard still kills a stubbed dist run at startup, before any test result prints:
No
Test Files/Testsline anywhere in the output.vi.doMockreach was verified by inversion rather than assumed. Against the pre-fix config the new test fails with the stack pointing atvitest.config.ts:126, so the mock genuinely intercepts the config's own transitive import and the test is not passing for the wrong reason.Commit 2 — three follow-ups
The parity sweep is skipped under
source. The resolving plugin rewritesimport(specifier)to exactly the pathsourceCounterpart()computes, so both sides are the same module and ~90 cases assertX === X. Now: 4 passed / 97 skipped under source, 101 passed under dist — the rows execute where they mean something. Nothing stops being loaded, becausePublishedEntryImports.test.tsimports every published specifier unconditionally and does carry signal under source. An anti-vacuity test asserts the target is one of the two known values, so brokentest.envplumbing cannot make every case skip and leavetest-vitest-distgreen having compared nothing.PublishedEntryIdentity.test.tsis out of scope (not reviewed).The guard now covers every entry
use-sourcewrites. It read onlyexports, butuse-sourcealso stubsbintargets — and two are named by no export entry at all:examples/evaldeclares noexports, only"bin": {"workglow-eval": "./dist/workglow-eval.js"}, andexamples/cli's"bin": "./dist/workglow.js"is not among its export targets.publish-workspaces.ts(a full dist walk) did refuse them; the dist guard looked straight past them. The newguards every entry \use-source` writes a stub into` test fails pre-fix with exactly:guardedDistTargetsis deliberately notstubSpecsFor, which additionally maps each target to a source counterpart and throws for one that is neither.jsnor.d.ts. The guard has no use for the mapping and should not acquire a new way to fail during config resolution.An unreadable entry is unproven, not built.
probeSourceStubreturns three states because the two callers fail safe in opposite directions: removal must not delete an artifact it could not read, while the dist guard must not accept one as a real bundle. Tested with a directory named like a dist entry (open succeeds, read fails EISDIR) rather thanchmod, which proves nothing when the suite runs as root, as it does in a container.The tinyglobby check — Option A shipped, with the claim corrected
The plan's mandatory empirical check ran on a real build. Neither the pass nor the fail criterion was met, and the reason is worth stating.
--project util, cwd = repo root8.52% (292/3427)8.52% (292/3427)--project util, cwd =packages/util8.52% (292/3427)8.52% (292/3427)--project scripts0/00/0The absolute pattern does not error and does not shrink the report to loaded files only — untested directories (
src/compressat 0%) stay in the denominator in every configuration. So Option A is safe and Option B was not needed.But the pass criterion as written (a
--project utilrun reachingpackages/ai/src/**) is not achievable with either form: vitest 4 scopes the coverage denominator by project root regardless of the include glob. The premise that the relative form silently loses the untested half is therefore not reproducible on@vitest/coverage-v84.1.10, and the docstring and test comment say so rather than asserting a failure I did not observe. Anchoring is shipped as defensive — consistent with the config's own "anything path-shaped must be ABSOLUTE" rule for the shared project options, and removing a dependency on an implementation detail nothing pins — not as a fix for a measured miss. The per-package/src/**exclusions are anchored to match, since a relative exclusion subtracts from nothing once the include is absolute.Dropped finding: Windows path normalisation
slash(),distToSource's[\\/], andimporterPackageOfwere dropped in planning and are not in this PR. The repo has no Windows CI (test.ymlisubuntu-latestthroughout), the root scripts are POSIX shell, andsourceStubs.ts:127writes#!/usr/bin/env bunshebangs — it buys nothing. Its POSIX-visible half (thepath.relative-derived coverage exclusion) is folded into the glob anchoring above.Verification
The one
utilfailure ispackages/util/src/utilities/__tests__/TestCredentialPreload.test.ts— a 15 s timeout in a KDF under heavy parallel load, unrelated to these changes and confirmed environmental: it passes in isolation (5 passed).git statusstayed clean through the wholeuse-source/use-distcycle, as designed.Risks
configResolvedtiming rests on vitest resolving every project's config at startup. If a future vitest defers project init the guard fires later than config load (still before that project's tests). The stubbed-treeWORKGLOW_TEST_TARGET=distrun above is the check; if any test result ever prints before the error, escalate to also callingassertNoSourceStubsfromscripts/test.tswhen the resolved target isdist.AggregateError. The scan runs once (asserted); the repetition is vitest listing one cachedErrorobject per project it failed to initialize, and the docstring says so rather than claiming the message prints once.WorkspacePackage.binis required, not optional, so every construction site must setbin: undefinedexplicitly rather than the field quietly defaulting — TS finds them all.PublishedEntryImportsstaying unconditional plus the anti-vacuity test.🤖 Generated with Claude Code
https://claude.ai/code/session_01HJRf3YFa8DjmjsZvXz8xDT
Generated by Claude Code