Skip to content

fix(ci): remove in-place package build output before the coverage run#8564

Merged
JSONbored merged 1 commit into
mainfrom
claude/fix-inplace-emit-coverage-shadowing
Jul 24, 2026
Merged

fix(ci): remove in-place package build output before the coverage run#8564
JSONbored merged 1 commit into
mainfrom
claude/fix-inplace-emit-coverage-shadowing

Conversation

@JSONbored

Copy link
Copy Markdown
Owner

Problem

Since the 3-shard consolidation (#8514), every packages/** PR fails codecov/patch at (0%/99%) — the last several contributor PRs were auto-closed for it. The consolidation moved the --force miner/MCP builds into the same job as the coverage run. Both packages compile in place (tsconfig outDir: "."), so the builds leave compiled .js siblings next to every .ts under packages/loopover-{miner,mcp}/{lib,bin}. Once those exist, Vite resolves the packages' NodeNext .js-suffixed import specifiers to the real compiled files instead of the .ts fallback vitest.config.ts's coverage globs depend on — breaking the run twice over:

  • --changed's import-graph tracing can't relate a changed .ts to the tests that import it (the graph holds the .js identity)
  • executed-module coverage is attributed to the .js ids, flatlining every changed .ts to 0%

Reproduced + bisected locally on vitest 4.1.9 against a failing PR's diff shape: with lib/*.js present the scoped run selects 1 test file and reports the changed attempt-cli.ts at 0%; on a clean tree it selects 4 test files and reports 100% / 99.48% branch. The failing CI runs' uploads confirm it — the backend lcov reached Codecov containing only the changed file at 0% (project stayed green via rees/control-plane carryforward, masking the breakage).

Fix

  • git clean -fdX packages/loopover-miner packages/loopover-mcp after build verification, before "Test with coverage" (-fdX can only remove gitignored files; the turbo cache save above already captured the outputs)
  • check-miner-package.test.ts — the one test that genuinely needs the built artifacts (npm-pack dry-run; both CLI harnesses spawn the .ts entrypoints via --experimental-strip-types) — is now always excluded from the coverage run and re-run at the end of the job after a rebuild, gated exactly like its old in-suite inclusion (push, or minerTestHarness PRs)

Validated locally: YAML parses; the rebuild + standalone vitest run --coverage.enabled=false test/unit/check-miner-package.test.ts passes (17/17).

The 3-shard consolidation moved the miner/MCP --force builds into the same
job as the coverage run. Both packages compile IN PLACE (tsconfig outDir
"."), so the builds leave compiled .js siblings next to every .ts under
packages/loopover-{miner,mcp}/{lib,bin} -- and once those exist, Vite
resolves the packages' NodeNext .js-suffixed import specifiers to the real
compiled files instead of falling back to the .ts source. That flatlined
codecov/patch to (0%/99%) on every packages/** PR: --changed's import-graph
tracing could no longer relate a changed .ts to its tests, and executed
coverage was attributed to the .js identities.

Reproduced and bisected locally on vitest 4.1.9 against a real failing PR's
diff shape: with lib/*.js present, the scoped run selects 1 test file and
reports the changed attempt-cli.ts at 0%; on a clean tree it selects 4 test
files and reports 100%/99.48%.

Fix: git clean -fdX the two packages after the build-verification steps and
before "Test with coverage" (only gitignored files are removable by
construction). The only test that genuinely needs the built artifacts --
check-miner-package.test.ts's npm-pack dry-run; both CLI harnesses spawn the
.ts entrypoints via --experimental-strip-types -- is now always excluded
from the coverage run and re-run at the end of the job after a rebuild,
gated exactly like its old in-suite inclusion.
@JSONbored JSONbored self-assigned this Jul 24, 2026
@superagent-security

Copy link
Copy Markdown
Contributor

Superagent didn't find any vulnerabilities or security issues in this PR.

@codecov

codecov Bot commented Jul 24, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 89.58%. Comparing base (12aa3e6) to head (db0f354).
⚠️ Report is 3 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #8564      +/-   ##
==========================================
- Coverage   92.51%   89.58%   -2.93%     
==========================================
  Files         795       97     -698     
  Lines       79613    22706   -56907     
  Branches    24056     3872   -20184     
==========================================
- Hits        73653    20341   -53312     
+ Misses       4800     2187    -2613     
+ Partials     1160      178     -982     
Flag Coverage Δ
backend ?
control-plane 99.85% <ø> (ø)
rees 88.56% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.
see 698 files with indirect coverage changes

@JSONbored
JSONbored merged commit 0c19927 into main Jul 24, 2026
5 of 6 checks passed
@JSONbored
JSONbored deleted the claude/fix-inplace-emit-coverage-shadowing branch July 24, 2026 21:07
JSONbored added a commit that referenced this pull request Jul 24, 2026
…toring both coverage attribution and subprocess artifacts (#8568)

Second, complete fix for the 2026-07-24 codecov/patch (0%/99%) incident.
The first fix (#8564) deleted the miner/MCP in-place build output before
the coverage run, which restored .ts coverage attribution and --changed
tracing -- but broke the other class of tests: subprocess-spawning ones
(the CLI harnesses and MCP stdio tests) run under plain Node outside
Vite's resolver and genuinely need the built lib/bin .js at spawn time
(node's ESM resolver does not fall back from a .js specifier to a .ts
sibling). Scoped post-fix runs failed in miner-discover-cli/--help,
miner-init-wizard e2e, and all 14 mcp-feasibility-gate stdio tests.

This replaces that approach: a resolve-stage vitest plugin rewrites
relative .js-suffixed imports that land inside
packages/loopover-{miner,mcp}/{lib,bin} to their .ts sibling when it
exists, so in-process imports always carry the source identity (correct
v8 attribution + import-graph tracing) while the built .js stays on disk
for spawned subprocesses. The #8564 ci.yml surgery (pre-coverage clean,
unconditional check-miner-package exclusion, trailing pack-integrity
step) is reverted -- the workflow returns to the #8514 shape, with the
plugin making it correct.
JSONbored added a commit that referenced this pull request Jul 24, 2026
… emit

Completes the proper fix for the 2026-07-24 codecov/patch (0%/99%)
incident. #8564 deleted in-place build output before the coverage run;
#8568 replaced that with a vitest resolver plugin forcing in-process
resolution to .ts regardless of a coexisting .js. Both were workarounds
around the real root cause: packages/loopover-{miner,mcp} compiled
in-place (tsconfig outDir "."), so compiled .js sat in the same
directory as its .ts source -- the actual, recurring bug class (three
prior incidents: stale .js shadowing fresh .ts, a no-op incremental
build lying about freshness, then the coverage-attribution bug).

This migrates both packages to out-of-place emit (outDir "dist"),
matching how packages/loopover-engine already builds. .ts source and
compiled .js output now live in physically separate directories, so
there is never a same-directory collision for any resolver to work
around -- the #8568 plugin is removed as dead code.

Turbo caching fixed alongside the directory move (a real, deferred
optimization surfaced by grep'ing this exact history): build:tsc's
`cache: false` was explicit migration-era caution a prior PR (#7317)
already flagged as safe to remove once bin/lib became fully
compiler-owned; the dist/ split removes the original hazard entirely.
Both packages' build tasks now declare real outputs (including
.tsbuildinfo, so a cache HIT keeps tsc's own incremental state
consistent with what's restored) instead of never caching at all.

Every literal reference to the old in-place bin/lib layout is updated:
the shared CLI test harnesses (which had to give up spawning bin/*.ts
directly via --experimental-strip-types -- that only worked because
Node's resolver found the entry's internal lib/*.js imports sitting
right next to it; it does not fall back .js->.ts the way Vite/esbuild
do), ~40 test files with hardcoded subprocess/readFileSync paths, 4
fixture scripts, the pack-check scripts' allowlists, check-syntax.mjs
in both packages, the DEPLOYMENT.md audit script + its documented
paths, and package.json's bin/files fields.

Two production-source bugs found and fixed properly, not patched
around: bin/loopover-mcp.ts and lib/status.ts both compute paths
relative to their own file location (own package.json, CHANGELOG.md,
the monorepo-sibling loopover-engine package) -- these files are ALSO
imported in-process by tests that resolve against the real .ts source,
so a single hardcoded relative depth cannot be correct for both
contexts. Both now try the source-relative depth first, falling back
to the dist-relative depth, so they work correctly however they're
currently loaded. bin/loopover-mcp.ts's dynamic
`import("@loopover/miner/lib/claim-ledger.js")` -- a published-package
subpath import into miner's internals, invisible to any relative-path
grep -- is fixed via an explicit `exports` map in miner's package.json
instead of hardcoding "dist/" into mcp's own source, so the public
contract between the two packages stays stable independent of miner's
internal layout.

package-lock.json's bin fields resynced (npm mirrors workspace
packages' bin targets there); node_modules/.bin symlinks regenerated
via a real `npm install` (--package-lock-only does not relink them).

Docs: contributing-to-loopover's reference.md and SKILL.md corrected
-- they previously said miner/mcp tests "run straight off the .ts"
with no build needed, which is no longer true for the CLI-harness and
stdio-transport-spawning tests now that dist/ is a separate directory.

Validated: full unsharded suite (21,541/21,541 passing), typecheck
clean, both real compiled CLIs execute and self-report their version
correctly, turbo cache save+restore verified to actually restore
dist/bin, dist/lib, and dist/package.json on a hit, and the monorepo
package-check scripts pass against the real built workspace.
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.

1 participant