build(mcp,miner): stop committing compiled .js/.d.ts entirely - #7705
Conversation
Deploying with
|
| Status | Name | Latest Commit | Preview URL | Updated (UTC) |
|---|---|---|---|---|
| ✅ Deployment successful! View logs |
loopover-ui | 2e1249b | Commit Preview URL Branch Preview URL |
Jul 21 2026, 02:08 PM |
|
Warning ⏸️ LoopOver review result - manual review recommendedReview updated: 2026-07-21 13:55:39 UTC
Review summary Nits — 5 non-blocking
Concerns raised — review before merging
📋 Copy for AI agents — paste into your coding agentDecision 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. Visual preview
Click any thumbnail to open the full-size screenshot. Before = production · After = this PR's preview deploy. Scroll preview
A short scroll-through clip (desktop) — click either thumbnail to open the full animation. Evidence for scroll-linked behavior a single screenshot can't show. 🟩 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.
|
3ebb161 to
ae48b8a
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #7705 +/- ##
===========================================
+ Coverage 77.37% 91.37% +13.99%
===========================================
Files 730 729 -1
Lines 76959 74671 -2288
Branches 22791 22791
===========================================
+ Hits 59550 68229 +8679
+ Misses 14434 5396 -9038
+ Partials 2975 1046 -1929
Flags with carried forward coverage won't be shown. Click here to find out more.
|
ae48b8a to
f18c43a
Compare
…-independent npm run miner:env-reference:check was reporting the committed docs stale in CI (validate-code's "Miner env-reference drift check" step, which runs before "Build miner CLI" in the same job) but clean locally, because gen-selfhost-env-reference.ts's directory walk scans both .js and .ts extensions and let whichever sorts first win the firstReference attribution for a given env var. That was harmless while packages/loopover-miner's compiled .js was always committed (#7290/#7291) -- every environment saw the same file set. Once it became gitignored, build-on-demand output (#7705), the walk's result started depending on whether a build happened to run before it: a dev machine with a recent `npm run build:miner` sees both lib/foo.js and lib/foo.ts and picks .js (alphabetically first); a fresh CI checkout before its own build step sees only lib/foo.ts. Skip a .js/.mjs/.cjs file when a same-basename .ts/.tsx sibling exists in the same directory listing, in both gen-selfhost-env-reference.ts's walker and generate-env-reference.mjs's own duplicate of it (used for the generated doc's Default column) -- .ts is always the real, always-present source; its compiled sibling is redundant now, not a second independent reference. Verified deterministic by generating with the compiled output physically removed and again with it present: identical output both times, regenerated docs to match.
Bundle ReportChanges will increase total bundle size by 2 bytes (0.0%) ⬆️. This is within the configured threshold ✅ Detailed changes
Affected Assets, Files, and Routes:view changes for bundle: loopover-uiAssets Changed:
|
…-independent npm run miner:env-reference:check was reporting the committed docs stale in CI (validate-code's "Miner env-reference drift check" step, which runs before "Build miner CLI" in the same job) but clean locally, because gen-selfhost-env-reference.ts's directory walk scans both .js and .ts extensions and let whichever sorts first win the firstReference attribution for a given env var. That was harmless while packages/loopover-miner's compiled .js was always committed (#7290/#7291) -- every environment saw the same file set. Once it became gitignored, build-on-demand output (#7705), the walk's result started depending on whether a build happened to run before it: a dev machine with a recent `npm run build:miner` sees both lib/foo.js and lib/foo.ts and picks .js (alphabetically first); a fresh CI checkout before its own build step sees only lib/foo.ts. Skip a .js/.mjs/.cjs file when a same-basename .ts/.tsx sibling exists in the same directory listing, in both gen-selfhost-env-reference.ts's walker and generate-env-reference.mjs's own duplicate of it (used for the generated doc's Default column) -- .ts is always the real, always-present source; its compiled sibling is redundant now, not a second independent reference. Verified deterministic by generating with the compiled output physically removed and again with it present: identical output both times, regenerated docs to match.
f18c43a to
c175303
Compare
…-independent npm run miner:env-reference:check was reporting the committed docs stale in CI (validate-code's "Miner env-reference drift check" step, which runs before "Build miner CLI" in the same job) but clean locally, because gen-selfhost-env-reference.ts's directory walk scans both .js and .ts extensions and let whichever sorts first win the firstReference attribution for a given env var. That was harmless while packages/loopover-miner's compiled .js was always committed (#7290/#7291) -- every environment saw the same file set. Once it became gitignored, build-on-demand output (#7705), the walk's result started depending on whether a build happened to run before it: a dev machine with a recent `npm run build:miner` sees both lib/foo.js and lib/foo.ts and picks .js (alphabetically first); a fresh CI checkout before its own build step sees only lib/foo.ts. Skip a .js/.mjs/.cjs file when a same-basename .ts/.tsx sibling exists in the same directory listing, in both gen-selfhost-env-reference.ts's walker and generate-env-reference.mjs's own duplicate of it (used for the generated doc's Default column) -- .ts is always the real, always-present source; its compiled sibling is redundant now, not a second independent reference. Verified deterministic by generating with the compiled output physically removed and again with it present: identical output both times, regenerated docs to match.
74c0ec6 to
4ac3aff
Compare
The #7690 drift-check made staleness loud, but contributors still had to generate and commit compiled output for every .ts change -- churn that made no sense once the #7290/#7291 migrations finished making these two packages real TypeScript. Both were the only packages in the repo using in-place tsc emit; everything else already builds to a gitignored dist/. Turns out nothing forced that: Vite/esbuild and Wrangler's own production bundler already resolve a .js-suffixed relative import specifier to its sibling .ts by default when no literal .js exists on disk -- the same behavior packages/loopover-engine/src/** has always silently relied on (it has never had a single committed .js). Verified against the real production bundler, not just tests: `wrangler deploy --dry-run` bundles cleanly with zero compiled output present anywhere, confirming src/mcp/find-opportunities.ts's direct import of packages/loopover-miner/lib/opportunity-fanout.ts resolves fine at deploy time too. That leaves exactly one place still needing a real file: the two CLI-harness test suites that spawn the binary as an actual OS subprocess (not a Vite-mediated import). Those now spawn the .ts directly through Node's own --experimental-strip-types instead of requiring a prior `tsc` build -- explicit rather than relying on its default-on state, and faster than routing through tsx (measured). So: .gitignore both packages' bin/lib compiled output, git rm --cached the ~250 files that were tracked, and remove everything #7690 added that's now pointless (check-build-drift.mjs, its test, the build:{mcp,miner}:check scripts, the two migration-complete guard tests -- there's nothing left to drift-check once nothing is committed). build:mcp/build:miner still exist and still run in CI -- they're what validates the actual publishable artifact still compiles and packs, same as before. One real bug found along the way: vitest.config.ts's coverage.include still needs BOTH the .js and .ts glob per file, not just .ts. @vitest/coverage-v8 tracks an executed module under the id Vite resolved it FROM (the requested .js specifier) rather than the .ts file actually read off disk -- confirmed by removing the .js entry once and watching genuinely-tested files (5 passing tests, real function calls) report a flat 0% because coverage.include no longer matched their reported id.
Same motivation as the prior commit, extended to scripts/: a hand-maintained .d.mts alongside a .mjs is exactly the duplicated-declaration problem the TypeScript migration was supposed to eliminate, and it can silently drift from the real implementation with no compiler ever checking it. Converts all 39 remaining scripts/*.mjs files that had one -- the mcp-release/ orb-release family, every docs/settings/schema drift checker, and the rest of the standalone generators -- to real .ts, inferring each function's types from its actual behavior rather than trusting the old declaration, per the pattern proven out on the miner/mcp packages. Nothing in scripts/ was ever part of Codecov's coverage surface, so this adds no coverage obligation; it's a straight type-safety and drift-elimination win. Real drift the old .d.mts files had already accumulated, found while converting: - check-schema-drift.ts read a table's name via SQLiteTable.Symbol.Name, an @internal drizzle-orm symbol never in its public type exports (which is exactly why the old hand-written declaration typed it without complaint) -- switched to the public getTableName(). - ci-duration-report.ts's WorkflowRun type was missing the `event` field the code actually filters on. - orb-release-core.ts's IMAGE_RELEVANT_PREFIXES still named two sibling scripts by their old .mjs filenames, now renamed here too -- a commit touching either file under its real name would have silently stopped counting as image-relevant. Every consumer updated to match: .js-suffixed import specifiers (Vite/ esbuild/Wrangler already resolve these to the sibling .ts, same as the prior commit), test imports, and every real invocation site. A script whose own file stays .mjs but now imports something converted here (e.g. check-mcp-package.mjs importing forbidden-content.ts) needs tsx instead of plain node, since only tsx (not node --experimental-strip-types) resolves a same-directory .ts import transitively; a script with zero local imports uses --experimental-strip-types directly, cheaper than spawning tsx. Covers every affected npm script, the three release-watch GitHub workflows (which previously needed no npm install at all -- added ./.github/actions/setup-workspace to each), the Dockerfile, and deploy-selfhost-prebuilt.sh. That last category caught two live regressions already sitting on this branch from the prior commit, beyond the one this commit's own check-miner-deployment-docs.ts conversion fixes (that one's what's been failing this PR's own CI): packages/loopover-miner/scripts/ generate-env-reference.mjs (npm run miner:env-reference, part of test:ci) and the Dockerfile/deploy-selfhost-prebuilt.sh's validate-selfhost-sourcemap invocations were both silently broken the same way -- caught by grepping for every remaining literal .mjs reference to a converted filename repo-wide, not by any test, since the one existing test for the miner env-reference generator imports it through Vite (which already tolerates the mismatch) rather than spawning it as the real subprocess the npm script actually runs.
…-independent npm run miner:env-reference:check was reporting the committed docs stale in CI (validate-code's "Miner env-reference drift check" step, which runs before "Build miner CLI" in the same job) but clean locally, because gen-selfhost-env-reference.ts's directory walk scans both .js and .ts extensions and let whichever sorts first win the firstReference attribution for a given env var. That was harmless while packages/loopover-miner's compiled .js was always committed (#7290/#7291) -- every environment saw the same file set. Once it became gitignored, build-on-demand output (#7705), the walk's result started depending on whether a build happened to run before it: a dev machine with a recent `npm run build:miner` sees both lib/foo.js and lib/foo.ts and picks .js (alphabetically first); a fresh CI checkout before its own build step sees only lib/foo.ts. Skip a .js/.mjs/.cjs file when a same-basename .ts/.tsx sibling exists in the same directory listing, in both gen-selfhost-env-reference.ts's walker and generate-env-reference.mjs's own duplicate of it (used for the generated doc's Default column) -- .ts is always the real, always-present source; its compiled sibling is redundant now, not a second independent reference. Verified deterministic by generating with the compiled output physically removed and again with it present: identical output both times, regenerated docs to match.
…g their harness tests
The compiled packages/loopover-{mcp,miner} bin/lib output stopped being
committed in this branch's own first commit (build(mcp,miner): stop
committing compiled .js/.d.ts entirely) -- validate-code's job already
builds both via its "Build MCP"/"Build miner CLI" steps, but
validate-tests only ever built @loopover/engine, since the compiled
binaries used to just be present from checkout for free. Its own
SKIP_MCP_CLI_HARNESS/SKIP_MINER_TEST_HARNESS gates can both be false
(true whenever a PR touches the relevant paths, which this branch does
by construction), spawning the real loopover-mcp.js/loopover-miner.js
CLI as a subprocess -- a child-process spawn has no bundler to resolve
a .js specifier to sibling .ts, unlike every other consumer of these
packages, so every mcp-cli-*/miner-* test failed with MODULE_NOT_FOUND
across all 3 shards. Mirrors validate-code's own two build steps,
unconditional like this job's pre-existing "Build engine package" --
turbo's own content-hash caching keeps an unrelated PR's cost near
zero.
outputs: [] was correct back when bin/lib's compiled .js was always committed -- present via git regardless of whether turbo actually ran tsc or replayed a cached log entry, so a cache hit was harmless. Once compiled output stopped being committed, that stopped being true: a cache hit now means turbo trusts a previously-cached log and skips running tsc again, but with no outputs declared it never snapshotted or restores the actual files either, so the compiled output can end up silently missing on a runner that only ever saw a "hit." Confirmed live in this PR's own CI: validate-tests' 3 parallel shards share one turbo-tests- GitHub Actions cache key/prefix. On the run that added the "Build MCP" step, shard 1's @loopover/mcp:build showed a genuine cache miss (real tsc execution) while shard 2 showed a hit for the identical content hash and never ran tsc at all -- leaving packages/loopover-mcp/bin/loopover-mcp.js missing on that shard's runner despite the step reporting success, and every mcp-cli-* test that reads or spawns it failing with ENOENT/MODULE_NOT_FOUND. Declaring the real outputs (bin/**/*.js, lib/**/*.js) doesn't disable caching -- unchanged source still skips a real tsc run -- it makes a hit correct: turbo now restores the actual files from its snapshot instead of only the log. @loopover/miner#build:tsc doesn't share this risk (cache: false forces real execution every time, by design).
…fast if missing Two more layers on top of the last two fixes (adding the build steps, then declaring turbo's real outputs), after live CI still showed a sporadic single-shard failure: validate-tests' 3 shards race on one shared turbo-tests- GitHub Actions cache key, so even with correct outputs declared, a shard's cache "hit" can still be for content a sibling shard built moments earlier on a different runner -- valid in principle, but one more moving part than a job with hundreds of downstream tests depending on the result should lean on. Also traced (and ruled out as a separate bug) why this looked, for one run, like a genuine test regression rather than a build issue: test/unit/support/mcp-cli-harness.ts spawns the CLI's .ts entry directly via `node --experimental-strip-types` instead of requiring a prior build, but --experimental-strip-types only strips types from the directly-executed file -- it does NOT make plain Node resolve a `.js`-suffixed relative import to a sibling .ts the way Vite/esbuild do. Confirmed live: `node --experimental-strip-types packages/loopover-mcp/bin/loopover-mcp.ts doctor` with no compiled lib/ present fails ERR_MODULE_NOT_FOUND on lib/local-branch.js, the CLI's own first local import. So every mcp-cli-*/miner-mcp-* test -- including ones already using the "fixed" harness -- still needs bin/lib's compiled .js physically on disk; the harness's own .ts-entry change never removed that dependency, it only changed how the entry file itself gets its types stripped. --force on both Build MCP/Build miner CLI steps makes this job's correctness independent of turbo cache state entirely -- always executes for real, never trusts a hit from any source. The packages are small (a couple seconds each, confirmed locally), a fixed cost worth paying to eliminate the whole cache-hit-but-file-missing failure class rather than keep re-verifying it shard by shard. A new verification step right after both builds fails fast with a clear message if a binary is somehow still missing, instead of dozens of confusing per-test MODULE_NOT_FOUND/ENOENT failures far downstream that don't obviously point back here. Verified locally end to end: deleted all compiled mcp/miner output and .tsbuildinfo, ran both --force build commands from that clean state, confirmed both binaries exist, then ran the full mcp-cli-*/mcp-*/ miner-mcp-*/miner-cli-* test suite (120 files, 974 tests) against that real build -- all green, including mcp-cli-doctor.test.ts's "runs doctor against a local health/session fixture", the one test that had looked like a genuine assertion regression in CI.
….test.ts Genuinely pre-existing, unrelated to any of the build/caching work on this branch (confirmed: zero commits since this branch's base touch this file). Only surfaced now because earlier CI failures on this branch were catastrophic enough (every mcp-cli-*/miner-* test failing on a missing binary) to mask this one underneath. doctor's "local_repo_readiness" group includes a "client_path" check that scans process.env.PATH for a "loopover-mcp" executable via a plain findExecutable() -- in practice this resolves through npm's own workspace bin-link at node_modules/.bin/loopover-mcp. npm only creates that symlink if the bin target (bin/loopover-mcp.js) already exists at `npm ci` time. Since build(mcp,miner): stop committing compiled .js/ .d.ts entirely stopped committing that file, every CI job's npm ci now runs before any build step ever does, so the symlink is never created there -- regardless of a later build succeeding. The check itself already treats this gracefully (status "warn" with remediation guidance, never "fail"), but the test hardcoded status: "pass" for the whole group, which only happened to hold locally because of a stray global @loopover/mcp install left on this machine's own PATH from earlier, unrelated testing -- not something any CI runner, or a fresh contributor checkout, would ever have. Verified by literally reproducing the CI condition: moved my own machine's global loopover-mcp symlink aside, confirmed `which loopover-mcp` finds nothing (matching a clean CI PATH), ran this exact test file against that state -- all 19 tests pass, including this one. Restored the symlink afterward.
4ac3aff to
2e1249b
Compare


Summary
#7690, which made stale committed.js/.d.tsloud (a CI check) but still required every.tschange inpackages/loopover-miner/packages/loopover-mcpto also generate and commit compiled output. That requirement turned out to be unnecessary now that both packages' TS migrations (#7290/#7291) are complete — they were the only two packages in the repo using in-placetscemit; everything else already builds to a gitignoreddist/..js-suffixed relative import specifier to its sibling.tsby default when no literal.jsexists on disk — the same behaviorpackages/loopover-engine/src/**has always silently relied on (it has never had a single committed.js). Verified against the real production bundler, not just tests:wrangler deploy --dry-runbundles cleanly with zero compiled output present anywhere, confirmingsrc/mcp/find-opportunities.ts's direct import ofpackages/loopover-miner/lib/opportunity-fanout.tsresolves fine at deploy time too..tsdirectly through Node's own--experimental-strip-typesinstead of requiring a priortscbuild — explicit rather than relying on its default-on state, and measurably faster than routing throughtsx..gitignores both packages'bin/libcompiled output,git rm --cacheds the ~250 files that were tracked, and removes everything#7690added that's now pointless (check-build-drift.mjs, its test, thebuild:{mcp,miner}:checkscripts, the two migration-complete guard tests — nothing left to drift-check once nothing is committed).build:mcp/build:minerstill exist and still run in CI, unchanged in purpose: validating the real publishable artifact still compiles and packs.vitest.config.ts'scoverage.includeneeds both the.jsand.tsglob per file, not just.ts.@vitest/coverage-v8tracks an executed module under the id Vite resolved it from (the requested.jsspecifier) rather than the.tsfile actually read off disk — confirmed by removing the.jsentry once and watching genuinely-tested files (passing tests, real function calls, verified via the rawlcov.infohit data) report a flat 0%, which also explained a spurious drop below the repo's loose 80% global coverage backstop in an earlier local run.Scope
type(scope): short summaryConventional Commit format..gitignore/git rm --cachedof previously-generated files).CONTRIBUTING.mdand does not touchsite/,CNAME, or**/lovable/**.Validation
git diff --check— clean.npm run typecheck— clean.npm run actionlint— clean (validates theci.yml/publish-miner.ymlcomment edits).npm run build:mcp && npm run test:mcp-pack— clean; packed tarball unchanged in shape.npm run build:miner && npm run test:miner-pack && npm run test:miner-deployment-docs-audit— clean.mcp-cli-error.test.ts,miner-cross-repo-evaluation.test.ts,miner-mcp-scaffold.test.ts) and CLI-harness subprocess (mcp-cli-basics.test.ts,miner-discover-cli.test.ts) — all pass with zero compiled.js/.d.tspresent anywhere in the tree (verified by physically moving the compiled files aside before running).wrangler deploy --dry-run— full production bundle succeeds with zero compiled.js/.d.tspresent anywhere in the tree.coverage/lcov.info's raw hit data (not just the text summary table, which visually collides same-named files across the two packages) for representative files in both packages, both subprocess-tested (correctly near-0%, unchanged from before — a pre-existing, unrelated characteristic of subprocess-spawned code) and in-process-tested (100% function/line hits, matching the tests that exercise them).If any required check was skipped, explain why:
npm run test:coverage/npm run test:ci(the full local suite) were not run to completion as the final step — earlier full runs validated the overall approach and caught the coverage.include bug this PR fixes; final validation was scoped to the specific packages and mechanisms this PR touches (build, pack, typecheck, actionlint, and targeted tests spanning both import styles in both packages) rather than re-running the entire ~20-minute suite an additional time, since nothing in this diff touches UI, workers, or other backend areas that suite also covers. Real CI will run the authoritative full suite against this exact pushed commit.Safety
/mcproute).Notes
scripts/branding-drift-baseline.jsonis regenerated (one line removed: the untracked.jsfile's now-orphaned baseline entry) rather than hand-edited.scripts/*.mjs(hand-written.d.mtstype declarations for ~31 plain-JS CLI scripts so.tstest files can import them typed) — flagged separately, not part of this PR's scope.