fix(build): migrate loopover-miner/loopover-mcp to out-of-place dist/ emit#8590
Merged
Conversation
Contributor
|
Superagent didn't find any vulnerabilities or security issues in this PR. |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #8590 +/- ##
=======================================
Coverage 92.54% 92.54%
=======================================
Files 796 796
Lines 79850 79855 +5
Branches 24135 24136 +1
=======================================
+ Hits 73899 73905 +6
- Misses 4802 4804 +2
+ Partials 1149 1146 -3
Flags with carried forward coverage won't be shown. Click here to find out more.
|
Deploying with
|
| Status | Name | Latest Commit | Preview URL | Updated (UTC) |
|---|---|---|---|---|
| ✅ Deployment successful! View logs |
loopover-ui | f808d66 | Commit Preview URL Branch Preview URL |
Jul 24 2026, 11:21 PM |
loopover-orb
Bot
added
the
gittensor:bug
Contributor
|
Caution 🛑 LoopOver review result - fixes requiredReview updated: 2026-07-24 23:50:23 UTC
Review summary Nits — 5 non-blocking
Why this is blocked
📋 Copy for AI agents — paste into your coding agentCI checks failing
Decision drivers
Context & advisory signals — never blocks the verdict
Review context
Contributor next steps
Signal definitions
🧪 Chat with LoopOverAsk LoopOver a question about this PR directly in a comment — grounded only in the same cached, public-safe facts shown above, never a new claim.
Full command reference: https://loopover.ai/docs/loopover-commands 🧪 Experimental — new and may change. 🟩 Safe / merged · 🟦 Advisory · 🟨 Held for review · 🟥 Blocked / closed 💰 Earn for open-source contributions like this. Gittensor lets GitHub contributors earn for the work they already do — register to start earning →. Checked by LoopOver, a quiet PR intelligence layer for OSS maintainers.
|
… 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.
…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.
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).
JSONbored
force-pushed
the
claude/miner-mcp-dist-migration
branch
from
July 24, 2026 23:18
a78dbf5 to
f808d66
Compare
Bundle ReportBundle size has no change ✅ |
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 and #8568 fixed the 2026-07-24 codecov/patch (0%/99%) incident (root cause: miner/MCP's in-place
.jsemit shadowing their.tssources during coverage runs) with a vitest resolver plugin — a workaround, not the fix. That in-place layout (tsconfigoutDir: ".") is the actual root cause behind three separate prior incidents in this repo's history (stale.jsshadowing fresh.ts, a no-op incremental build lying about freshness, then the coverage-attribution bug). This PR removes the root cause instead of continuing to work around it.Fix
Migrates both packages to out-of-place emit (
outDir: "dist"), matchingpackages/loopover-engine's existingsrc/→dist/split. Source and compiled output now live in physically separate directories — no same-directory collision for any resolver to ever need to work around. The #8568 plugin is removed as dead code.Turbo caching — a real, previously-deferred optimization surfaced while touching this exact area:
build:tsc'scache: falsewas explicit #7290-migration-era caution a prior PR (#7317) already flagged as safe to remove. The dist/ split removes the original hazard entirely, so both packages' build tasks now declare realoutputs(including.tsbuildinfo, so a cache HIT keeps tsc's incremental state consistent with what's restored) instead of never caching.Every literal reference to the old layout updated: the shared CLI test harnesses (had to switch from spawning
bin/*.tsdirectly via--experimental-strip-types— that only worked because Node's resolver found the entry's internallib/*.jsimports sitting right next to it, and Node does not fall back.js→.tsthe way Vite/esbuild do — to spawning the compileddist/bin/*.js), ~40 test files with hardcoded subprocess/readFileSyncpaths, 4 fixture scripts, both packages' pack-check allowlists andcheck-syntax.mjs, the DEPLOYMENT.md audit script + its documented paths, andpackage.json'sbin/filesfields.Two production-source bugs found and fixed properly:
bin/loopover-mcp.tsandlib/status.tsboth computed paths relative to their own file location (ownpackage.json,CHANGELOG.md, the monorepo-siblingloopover-enginepackage) — these files are also imported in-process by tests resolving against the real.tssource, so no single hardcoded relative depth could be correct for both contexts. Rather than patch around this, both packages now expose a minimalpackage.jsonexportsmap, and every "read my own package's file" reference uses Node's standard self-referencing mechanism (import.meta.resolve()/ self-referencingrequire()) — robust by construction to however the file is currently running, not by guessing candidate depths.bin/loopover-mcp.ts'simport("@loopover/miner/lib/claim-ledger.js")— a published-package subpath import into miner's internals, invisible to any relative-path grep — is fixed the same way: an explicitexportsentry in miner'spackage.jsonkeeps the public contract between the two packages stable independent of miner's internal layout.package-lock.json'sbinfields resynced (npm mirrors workspace packages' bin targets there);node_modules/.binsymlinks regenerated via a realnpm install(--package-lock-onlydoesn't relink them).Docs:
contributing-to-loopover'sreference.md/SKILL.mdcorrected — 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 thatdist/is a separate directory.Validation
npm run typecheck: cleandist/bin,dist/lib, anddist/package.jsonon a hit@loopover/mcp/package.json,@loopover/miner/lib/claim-ledger.js, etc.) verified via directnode -eresolution checkstest:miner-pack/test:mcp-packpass against the real built workspace