ci(gate): cut PR validation cost — cached test fixtures, correct worker count, narrower UI scope - #2426
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2426 +/- ##
=======================================
Coverage 95.88% 95.88%
=======================================
Files 224 224
Lines 25143 25143
Branches 9143 9143
=======================================
Hits 24109 24109
Misses 421 421
Partials 613 613 🚀 New features to boost your workflow:
|
|
Warning 🟨🟨🟨🟨🟨🟨🟨🟨🟨🟨🟨🟨 ⏸️ Gittensory review result - manual review recommendedReview updated: 2026-07-02 00:17:29 UTC
⏸️ Suggested Action - Manual Review
Review summary Nits — 6 non-blocking
Review context
Contributor next steps
Signal definitions
🟩 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 Gittensory, a quiet PR intelligence layer for OSS maintainers.
|
2eb3d59 to
71fb241
Compare
TestD1Database's constructor rebuilt the whole schema from scratch on every instantiation, reading and executing all migrations/*.sql files sequentially (~1500 call sites across the suite). Benchmarked at ~33ms/call, almost entirely migration replay -- a meaningful, avoidable share of the full test:coverage run. Build the migrated schema once per worker process, serialize it, and clone that snapshot for every subsequent instance via node:sqlite's serialize()/ deserialize() instead of re-executing every migration file each time. Measured locally: ~1000x faster per call (33ms -> 0.03ms); full-suite coverage run time dropped from 2:07 to 0:38 with identical coverage numbers.
…cope Two independent CI-cost fixes found while profiling why validate-code was taking 6-10 minutes per PR on the self-hosted runner: - Test with coverage used --maxWorkers=100%. Node's os.cpus() reports the HOST's full core count inside a container, ignoring the Docker cgroup CPU quota, so this oversubscribed the vitest worker pool far past what the runner can actually execute in parallel (confirmed: `docker exec` into the runner reports 8 cores while its Docker CPU limit was 2, since raised to 4). Pin to --maxWorkers=4 to match the runner's real quota instead of a number Node can't correctly derive. - The `ui` changes filter included `src/**`, so any backend-only PR ran the full ui:lint/typecheck/test/build toolchain even when it couldn't possibly affect the UI. Split it: `ui` (the UI app/extension's own code, plus dependency bumps) still triggers the full toolchain; a new `uiContract` category (src/**, the openapi-writer script) triggers only the lightweight ui:openapi:check drift check. If a backend change is genuinely UI-relevant, regenerating openapi.json per CONTRIBUTING.md lands a change under `ui` and correctly re-triggers the full toolchain anyway. UI/MCP version audit is re-scoped to `ui || mcp` (it checks an MCP package-version string, never the OpenAPI contract, so it was never really a `src/**`-driven check to begin with).
vitest run --changed=<ref> selects only tests whose module graph is affected by the diff against that ref, via real import-graph analysis (not just path globs). Useful for the local edit-test-fix loop; the authoritative pre-push check stays npm run test:coverage / test:ci — this is not wired into CI or the Codecov gate.
test/unit/mcp-cli-*.test.ts, mcp-discovery.test.ts, and mcp-output-schemas.test.ts (~115s combined) are slow -- subprocess-per-test for the CLI-harness files, real MCP protocol handshakes for the others -- and were unconditionally bundled into Test with coverage on every backend PR, regardless of whether anything relevant to them changed. Their real dependency surface is narrower than "any backend change": the CLI-harness tests spawn packages/gittensory-mcp against a fixture server (never importing src/ directly, confirmed by grep) seeded via src/db/repositories and src/registry/*, and mcp-discovery/mcp-output-schemas exercise src/mcp/server.ts in-process. Added a new mcpCliCluster changes-filter scoped to exactly those paths, and skip the cluster (via vitest's --exclude) on a PR where it's false. Push events always run the full suite unchanged, matching the existing policy for every other path-filtered step. This is safe in a way a naive "just skip unrelated tests" isn't: skipping a test can only ever make Codecov's patch-coverage number look WORSE, never let a real regression through silently. If a PR's changed lines happen to be covered only by this cluster, patch coverage just fails loudly and the fix is to touch one of the mcpCliCluster paths (already true for any PR that legitimately affects this code) or run the cluster manually.
…anged Two review findings from the Gittensory Orb Review Agent on this same PR: - Blocker: mcpCliCluster omitted package.json/package-lock.json, so a dependency bump could change MCP CLI/server runtime behavior (the MCP SDK, node:sqlite, etc.) and still skip the only test cluster that exercises it. Unlike a changed-lines gap, that risk is NOT caught by Codecov's patch-coverage number, so the earlier "skipping can only make coverage look worse" safety argument didn't cover this case. Added both files to the filter, matching how the pre-existing mcp filter already treats them as relevant. - Nit: test:changed had no documented workflow. Added a mention to the contributing skill's "iterate fast" guidance.
Third review finding on this same filter (after the missing dependency files fix): omitted .github/workflows/**, test/helpers/**, vitest*.config.ts, and migrations/** -- so a change to the skip logic itself, the shared D1 test fixture (the very thing this PR's first commit touches), Vitest's runtime config, or DB schema could all set SKIP_MCP_CLI_CLUSTER=true while changing something this cluster actually depends on. Patching each flagged path individually was the wrong shape of fix -- it kept missing things because it was re-deriving a second, narrower opinion of "what's backend-relevant" instead of reusing the one `backend` already has. Redesigned: mcpCliCluster is now backend's filter verbatim for every category except src/**/test/**, which are narrowed to the specific files this cluster touches (that's where the actual savings are: most backend PRs touch some src/test file, but rarely these specific ones). Every other category -- config, scripts, deps, migrations, the workflow file itself -- is copied unchanged, so it can't drift out of sync with what `backend` already knows can affect a test run.
…iant Non-blocking nits from the same review pass: - Cross-reference comments between backend and mcpCliCluster so a future edit to one is more likely to prompt mirroring the other. - Document that the migrated-schema snapshot cache assumes migrations/*.sql is read-only for the life of a worker process; nothing currently mutates a migration file in-process, but this makes the assumption explicit.
71fb241 to
6179ec3
Compare
DatabaseSync.prototype.serialize()/deserialize() do not exist on Node 22 (this repo's .nvmrc) at all -- confirmed absent on 22.23.1, present only from Node 24+. The snapshot-clone cache added in the previous commit passed locally on a newer Node but crashed every test in CI (`db.deserialize is not a function`), since CI runs the pinned Node 22 on the self-hosted runner. Cache the concatenated migration SQL instead of a serialized snapshot: still avoids re-reading and re-sorting ~90 migration files on every one of the ~1500 TestD1Database construction sites, using only stable, always-available DatabaseSync/fs APIs. Verified green on both Node 22.23.1 (matching CI) and the local Node, with identical coverage numbers.
…endency mcpCliCluster's hand-picked src/** allowlist (src/mcp/**, src/auth/security.ts, src/db/repositories.ts, src/registry/normalize.ts, src/registry/sync.ts, src/services/repo-outcome-patterns.ts) was meant to gate when the whole cluster could be safely skipped. It missed the mark for mcp-output-schemas.test.ts specifically: that file imports src/mcp/server.ts in-process, and server.ts alone directly imports ~40 other src/ modules (src/github/app.ts, src/signals/slop.ts, src/settings/autonomy.ts, src/orb/analytics.ts, most of src/services/** and src/signals/**) that were never in the allowlist. A backend change to any of those files would skip mcp-output-schemas.test.ts with no rerun to catch a regression, while `backend` itself still treated the same change as test-relevant -- a reachable CI blind spot on a required, one-shot gate. Verified by direct-import inspection that the other 6 files in the cluster (test/unit/mcp-cli-*.test.ts and mcp-discovery.test.ts) are genuinely self-contained: they import nothing from root src/, only spawning the built packages/gittensory-mcp CLI as a subprocess. Split the filter accordingly: mcpCliHarness now scopes only those 6 self-contained files, mirroring the existing `mcp` filter's own self-contained-package trust boundary. mcp-output-schemas.test.ts is no longer excludable at all -- it always runs whenever `backend` does, same as the rest of the suite.
Summary
Fixes found while profiling why
validate-codewas taking 6-10 minutes per PR on the self-hosted runner (real step timings pulled from a recent run:Test with coveragealone was 5m39s of a 7m52s job):test/helpers/d1.ts:TestD1Database's constructor rebuilt the whole schema from scratch on every instantiation (~1500 call sites across the suite), reading and executing allmigrations/*.sqlfiles sequentially. Cache the concatenated migration SQL once per worker process instead of re-listing and re-reading ~90 files on every call. This still calls.exec()per instance (unavoidable on this repo's pinned Node 22 -- see below), so it removes only the redundant filesystem work, not the DDL execution itself..github/workflows/ci.yml— worker count:Test with coverageused--maxWorkers=100%. Node'sos.cpus()reports the HOST's full core count inside a container, ignoring the Docker cgroup CPU quota — confirmed directly on the runner (docker execinto it reports 8 cores while its actual Docker CPU limit was 2, since raised to 4). This meant vitest was very likely oversubscribing its worker pool well past what the runner could actually execute in parallel. Pinned to--maxWorkers=4to match the runner's real quota..github/workflows/ci.yml— UI path scope: theuichanges filter includedsrc/**, so any backend-only PR ran the fullui:lint/ui:typecheck/ui:test/ui:buildtoolchain even though it couldn't possibly affect the UI. Split intoui(the UI app/extension's own code, plus dependency bumps — still triggers the full toolchain) and a newuiContractcategory (src/**, the openapi-writer script — triggers only the lightweightui:openapi:checkdrift check).UI/MCP version auditis re-scoped toui || mcpsince it only checks an MCP package-version string and never reads the OpenAPI contract..github/workflows/ci.yml— MCP CLI-harness scope:test/unit/mcp-cli-*.test.tsandmcp-discovery.test.ts(6 files, subprocess-per-test for the CLI-harness files, a real subprocess-spawned MCP protocol handshake formcp-discovery) were unconditionally bundled intoTest with coverageregardless of whether anything relevant to them changed. Verified by direct-import inspection that all 6 are genuinely self-contained w.r.t. rootsrc/**— they only importnode:*builtins,vitest, and their own test harness, and spawn the builtpackages/gittensory-mcpCLI as a real subprocess. This mirrors the existingmcpfilter's own established trust boundary (a self-contained package build). Added anmcpCliHarnessfilter scoped to exactly those 6 files +packages/gittensory-mcp/**, and skip them viavitest --excludewhen it's false on a PR.test/unit/mcp-output-schemas.test.tsis deliberately not included in this filter or the exclude list: it importssrc/mcp/server.tsin-process, andserver.tsalone directly imports ~40 othersrc/modules, so its real dependency surface is practically all ofbackend— it always runs wheneverbackenddoes. Push events always run the full suite unchanged.package.json: addedtest:changed(vitest run --changed=origin/main) for the local edit-test-fix loop — real import-graph-aware test selection, not just path globs. Not wired into CI or the Codecov gate; the authoritative pre-push check staystest:coverage/test:ci.Also raised the 3
gittensory-labeled self-hosted runner containers' Docker CPU limit from 2.0 to 4.0 (the VPS has 8 total vCPUs), applied live viadocker update --cpus(no container restart, no disruption to in-flight jobs) and persisted indocker-compose.override.ymlon the host — not part of this diff since that file isn't committed to the repo.Update: two follow-up fixes after the first push
Test with coveragefailing): the originaltest/helpers/d1.tscached aDatabaseSync.serialize()/deserialize()snapshot to clone per instance. Those methods don't exist on Node 22 at all (confirmed absent on 22.23.1, present only from Node 24+) — this repo's.nvmrcpins Node 22, so every test that touchesTestD1Databasecrashed in CI (db.deserialize is not a function) while passing locally on a newer Node. Replaced with a Node-22-safe cache of the concatenated migration SQL (see thetest/helpers/d1.tsbullet above) — verified green on Node 22.23.1 with identical coverage numbers to the pre-fix run.mcpCliClusterfilter hand-picked a handful ofsrc/**files it assumed weremcp-output-schemas.test.ts's real dependency surface. That guess missed dozens ofsrc/mcp/server.ts's actual direct imports (src/github/app.ts,src/signals/slop.ts,src/settings/autonomy.ts, etc.), so a backend change outside the guessed list could skip that test with no rerun to catch a regression. Fixed by narrowing the skippable set to only the 6 files verified genuinely self-contained (see themcpCliHarnessbullet above);mcp-output-schemas.test.tsis no longer excludable under any condition.Scope
type(scope): short summaryConventional Commit format.CONTRIBUTING.md.Validation
git diff --checknpm run actionlintnpm run typecheck(verified on Node 22.23.1, matching CI's pinned.nvmrc)npx vitest run test/unit/workflow-runner-labels.test.ts test/unit/codecov-policy.test.ts test/unit/change-guardrail.test.ts test/unit/agent-guardrail-paths.test.ts test/unit/path-matchers.test.ts(every test that readsci.yml's content) — all pass unchanged.npm run test:ci— run on both Node 22.23.1 (matching CI's pinned.nvmrc) and a newer local Node, all green, identical coverage numbers on both.npm run test:workers— passes on Node 22.23.1.mcpCliHarness--exclude/skip logic locally (both the skip and no-skip branches): with the 6 harness files excluded, onlymcp-output-schemas.test.tsremains and passes; with nothing excluded, all 3 sampled files pass.npm audit --audit-level=moderate: 0 vulnerabilities.Notes
review-enrichment's test suite has no flaky-test retry (unlike the main suite'sretry: 1) — observed one non-deterministic failure there during this investigation, unrelated to any change here.copyFileSyncinstead of re-executing DDL) is possible and measured ~35x faster per call in a microbenchmark, but needs its own temp-file lifecycle/cleanup design to be safe on this repo's persistent self-hosted runner (a crash mid-suite could otherwise leak files across runs indefinitely). Left as a follow-up rather than folded into this fix.