fix(test): resolve in-place package emit to .ts source, fixing #8564's incomplete fix#8568
Merged
Merged
Conversation
…toring both coverage attribution and subprocess artifacts 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.
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 #8568 +/- ##
==========================================
+ Coverage 89.58% 92.53% +2.94%
==========================================
Files 97 795 +698
Lines 22706 79657 +56951
Branches 3872 24070 +20198
==========================================
+ Hits 20341 73710 +53369
- Misses 2187 4800 +2613
- Partials 178 1147 +969
Flags with carried forward coverage won't be shown. Click here to find out more. |
loopover-orb
Bot
added
the
gittensor:bug
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
#8564 fixed the 2026-07-24 codecov/patch (0%/99%) incident (root cause: miner/MCP's in-place
.jsemit shadowing their.tssources during the coverage run — full writeup there) by deleting the build output before the coverage run. That restored coverage attribution but broke the OTHER class of test that needs the built artifacts: subprocess-spawning tests run under plain Node, outside Vite's resolver, and Node's ESM resolver does not fall back from a missing.jsspecifier to a sibling.tsfile.Confirmed on a live post-#8564 PR run (#8563):
validate-testsfailed with 2 failures inminer-discover-cli.test.ts(the--dry-run/--helpsubprocess cases), 1 inminer-claim-ledger.test.ts, 1 e2e case inminer-init-wizard.test.ts, and all 14 cases inmcp-feasibility-gate.test.ts(the MCP stdio harness) — every one a subprocess spawn hitting a now-missing compiled file.Fix
Replace deletion with resolution: a resolve-stage vitest plugin (
vitest.config.ts) rewrites relative.js-suffixed imports landing insidepackages/loopover-{miner,mcp}/{lib,bin}to their.tssibling when it exists, scoped by regex to exactly those two in-place-emit packages (engine emits todist/and is untouched). In-process imports now always carry the.tssource identity — correct v8 coverage attribution and correct--changedimport-graph tracing — while the built.jsstays on disk for subprocess spawns that need it. #8564's ci.yml surgery (pre-coveragegit clean, unconditionalcheck-miner-packageexclusion, trailing pack-integrity step) is reverted; the workflow returns to its #8514 shape, with the plugin making that shape correct instead of broken.Validation (local)
attempt-cli.tschanged → 4 test files selected (not 1), 100% / 99.48% branch coverage (was 0%)mcp-feasibility-gate(14/14),miner-discover-cli,miner-init-wizard,check-miner-package,miner-claim-ledger— 168/168 totaltsc --noEmit --incremental falseclean on the plugin change