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.
JSONbored
added a commit
that referenced
this pull request
Jul 25, 2026
… emit (#8590) * fix(build): migrate loopover-miner/loopover-mcp to out-of-place dist/ 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. * refactor(build): replace relative-depth self-reference guessing with Node's own package self-referencing The previous commit's fix for the cross-context (in-process test vs compiled dist/) self-relative-path problem was a "try depth N, then depth N+1" heuristic (resolveOwnPackageSiblingPath / resolveMonorepoSiblingPath). It worked, but it's not the correct tool for the job: Node has a purpose-built, standard mechanism for exactly this -- a package importing its own files by its own published name ("self-referencing"), resolved via the package.json "exports" map the same way any external "@loopover/x/..." import would be. That's robust by construction (walks up through node_modules from the calling file's own location, landing on the one real file regardless of that file's current directory depth) rather than by guessing candidate depths. Adds a minimal "exports" map to both packages' package.json (own package.json + the one or two other self-referenced files each needs) and switches every "own package.json"-style self-reference -- bin/loopover-mcp.ts (package.json, CHANGELOG.md), lib/version.ts's static JSON import, lib/status.ts's two require() calls, and bin/loopover-miner-mcp.ts -- to import.meta.resolve()/self-referencing require() against the published package name instead. The one remaining depth-guessing fallback (lib/status.ts's monorepo-sibling loopover-engine lookup) is intentionally left as-is: it's a last-resort fallback for when real node_modules resolution has already failed, so a self-referencing lookup (which uses the identical resolution mechanism) wouldn't diversify the fallback at all -- trying both plausible sibling-directory depths is the actually-correct strategy for that specific case, not a shortcut. Validated: typecheck clean, both packages rebuild cleanly, full unsharded suite still 21,541/21,541 passing, and both self-referencing subpaths resolve correctly via direct node -e checks. * fix(build): remove two remaining build-order dependencies CI caught Both bugs share one root cause: I locally validated everything against a worktree where dist/ was already built from earlier test runs, so I never actually exercised a truly clean checkout. CI did, on both validate-tests and validate-code, and caught what local testing missed. 1. bin/loopover-mcp.ts's dynamic `import("@loopover/miner/lib/ claim-ledger.js")` — now resolvable via the "exports" map added in the prior commit — made tsc attempt to statically resolve and type that module during MCP's own build. CI's validate-tests job builds MCP before miner (unchanged, pre-existing order), so miner's dist/ doesn't exist yet at that point: TS2307. This import is explicitly optional and already try/catch-guarded at runtime for "loopover- miner genuinely unresolvable" -- it should never have had a build-time dependency on miner in the first place. Fixed by building the specifier from concatenated string parts instead of a literal, so tsc doesn't attempt static resolution at all, paired with a hand-shaped local type (no coupling to miner's actual types needed for three destructured fields), matching the code's own already-optional runtime shape. 2. test/unit/miner-deployment-docs-audit.test.ts's `import type { DeploymentDocsReality } from ".../dist/lib/deployment-docs- audit.d.ts"` (added in the prior commit) made the root `npm run typecheck` -- which validate-code runs without building miner first -- fail the same way. Unlike the runtime readFileSync calls in that same file (which genuinely need real compiled output on disk and correctly still point at dist/), a TYPE-ONLY import has no such requirement: NodeNext module resolution already resolves a plain .js-suffixed specifier to its sibling .ts source for type information, the exact mechanism the adjacent value import on the line above already relies on. Pointing the type import at the same lib/deployment-docs-audit.js specifier (not dist/) gets real types with zero build dependency, verified by re-running `npm run typecheck` against a directory tree with no dist/ built for either package at all. Verified this time: full clean state (rm -rf both packages' dist/ and .tsbuildinfo) --> typecheck passes with nothing built --> full rebuild --> unsharded suite 21,568/21,568 passing --> both pack-check scripts pass. Rebased onto origin/main first (11 commits had landed, including overlapping changes to package.json/package-lock.json). * fix(ci): update the last stale bin/ path references in ci.yml Missed this during the migration despite reading this exact file multiple times earlier: the "Verify MCP/miner CLI binaries were actually built" tripwire step still checked the old in-place packages/loopover-{mcp,miner}/bin/loopover-*.js paths. Since those binaries now build into dist/bin/ (see tsconfig.json's outDir comment), the tripwire was correctly firing -- the files genuinely aren't at the old path anymore -- failing validate-tests right after the two build steps it's meant to guard, exactly as designed, just against the wrong path. Also corrected the explanatory comment above that step and the "Build MCP"/"Build miner CLI" steps: it described the pre-migration mechanism (CLI harnesses spawning bin/*.ts directly via --experimental-strip- types, relying on the compiled .js sitting in-place next to it) which this migration already replaced (the harnesses now spawn the real compiled dist/bin/*.js directly). Left everything else the earlier grep flagged alone -- the other packages/loopover-miner/lib/*.js mentions elsewhere in ci.yml are comments about *.js-suffixed import SPECIFIERS (which still correctly resolve to their sibling .ts either way, unaffected by where compiled output lands) or pre-existing staleness unrelated to this migration (release-selfhost.yml's "(committed, pre-built)" predates even the #7290 gitignore-the- compiled-output migration). Verified this time by exactly replicating the failing job's own sequence locally: build engine, force-build MCP, force-build miner, then the verify step's own loop -- both paths now resolve. Also reconfirmed clean-state typecheck and the full unsharded suite (21,568/21,568) after this change.
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
1 participant
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).