fix(ci): remove in-place package build output before the coverage run#8564
Merged
Conversation
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.
Contributor
|
Superagent didn't find any vulnerabilities or security issues in this PR. |
Codecov Report✅ All modified and coverable lines are covered by tests. 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
Flags with carried forward coverage won't be shown. Click here to find out more. |
This was referenced Jul 24, 2026
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.
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.
Problem
Since the 3-shard consolidation (#8514), every packages/** PR fails
codecov/patchat (0%/99%) — the last several contributor PRs were auto-closed for it. The consolidation moved the--forceminer/MCP builds into the same job as the coverage run. Both packages compile in place (tsconfigoutDir: "."), so the builds leave compiled.jssiblings next to every.tsunderpackages/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.tsfallback vitest.config.ts's coverage globs depend on — breaking the run twice over:--changed's import-graph tracing can't relate a changed.tsto the tests that import it (the graph holds the.jsidentity).jsids, flatlining every changed.tsto 0%Reproduced + bisected locally on vitest 4.1.9 against a failing PR's diff shape: with
lib/*.jspresent the scoped run selects 1 test file and reports the changedattempt-cli.tsat 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-mcpafter build verification, before "Test with coverage" (-fdXcan 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.tsentrypoints 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, orminerTestHarnessPRs)Validated locally: YAML parses; the rebuild + standalone
vitest run --coverage.enabled=false test/unit/check-miner-package.test.tspasses (17/17).