diff --git a/.claude/skills/contributing-to-loopover/reference.md b/.claude/skills/contributing-to-loopover/reference.md index eaada3a44e..126a0f5c7e 100644 --- a/.claude/skills/contributing-to-loopover/reference.md +++ b/.claude/skills/contributing-to-loopover/reference.md @@ -14,12 +14,14 @@ for maintainer approval (CI shows unverified → the engine **holds**, never clo ## 1. Every CI check → local command → what fails it -The single **required** status check is **`validate`** (it aggregates `changes, lint, test, workers, -mcp, ui, security`; a path-skipped job counts as success). **Codecov** posts `codecov/patch` (the real -coverage gate) and `codecov/project` (informational) independently. The review engine also posts its -own check run named **`LoopOver Orb Review Agent`** (`src/github/app.ts` `LOOPOVER_GATE_CHECK_NAME`) — the gate -verdict (§3), separate from CI. On a PR, jobs run only if their -path filter matched; on push to `main`, everything runs. +The **required** status checks on `main` are **`validate`** (it aggregates `changes, lint, test, +workers, mcp, ui, security`; a path-skipped job counts as success) and **`Superagent Security Scan`** +(a separate third-party GitHub App check, not part of this repo's own workflow files — confirmed via +`gh api repos/JSONbored/loopover/branches/main/protection/required_status_checks`). **Codecov** posts +`codecov/patch` (the real coverage gate) and `codecov/project` (informational) independently. The +review engine also posts its own check run named **`LoopOver Orb Review Agent`** +(`src/github/app.ts` `LOOPOVER_GATE_CHECK_NAME`) — the gate verdict (§3), separate from CI. On a PR, +jobs run only if their path filter matched; on push to `main`, everything runs. | Check | Runs | Local command | Fails when | |---|---|---|---| @@ -32,7 +34,7 @@ path filter matched; on push to `main`, everything runs. | lint → miner-env-reference | miner/AMS env-var doc drift | `npm run miner:env-reference:check` | committed `packages/loopover-miner/docs/env-reference.md` / `apps/loopover-ui/src/lib/ams-env-reference.ts` is stale (run `npm run miner:env-reference`) — miner/AMS twin of the selfhost check above | | lint → observability | Grafana/Prometheus/alert config validation | `npm run selfhost:validate-observability` | a self-host observability config (dashboard/rule/datasource) is malformed | | lint → typecheck | `tsc --noEmit` | `npm run typecheck` | any backend type error | -| test (1/2) | sharded vitest + coverage | `npm run test:coverage` (unsharded) | any failing `test/**/*.test.ts` (excl. `test/workers/**`) | +| test (1/6..6/6) | sharded vitest + coverage | `npm run test:coverage` (unsharded) | any failing `test/**/*.test.ts` (excl. `test/workers/**`) | | workers | workers-pool vitest | `npm run test:workers` | any failing `test/workers/**` | | mcp → build | MCP pkg build | `npm run build:mcp` | MCP package build error | | mcp → pack | tarball hygiene | `npm run test:mcp-pack` | unexpected/forbidden file or stale README in the npm tarball | @@ -71,7 +73,7 @@ these for a normal PR:** | Local command | Why it's not in the table above | |---|---| -| `npm run test:engine-parity`, `npm run test:live-gate-parity`, `npm run test:driver-parity` | Plain `test/contract/*.test.ts` files — no dedicated CI job, but they DO run in CI as part of whichever `test (1/2)` shard happens to contain them (sharded `vitest run`). | +| `npm run test:engine-parity`, `npm run test:live-gate-parity`, `npm run test:driver-parity` | Plain `test/contract/*.test.ts` files — no dedicated CI job, but they DO run in CI as part of whichever `test (1/6..6/6)` shard happens to contain them (sharded `vitest run`). | | `npm run test --workspace @loopover/engine` | The engine package's own `node --test` suite. **Not run by `ci.yml` on a PR at all** — only by `.github/workflows/publish-engine.yml` at release time. A regression here is invisible to Codecov and to every PR-gating CI check; `test:ci` locally is the only pre-merge signal. | This is a real, previously-hit gap, not a hypothetical: a past PR shipped a genuine, undetected @@ -92,8 +94,11 @@ checks go green) is the only way to know you didn't break it. - **Ignored paths** (no coverage obligation): `apps/**`, `test/**`, `scripts/**`, `src/env.d.ts`. Coverage `include` is `src/**/*.ts` only. → A UI-only / test-only / script-only change owes **no** patch coverage; a backend `src/**` change owes coverage on **every changed line + branch**. -- **Measure unsharded locally:** `npm run test:coverage`. CI shards into 2 and Codecov merges them, +- **Measure unsharded locally:** `npm run test:coverage`. CI shards into 6 and Codecov merges them, so a single local shard under-reports — never trust it. +- **Flaky tests are already tracked.** Every shard uploads a JUnit report (`report_type: test_results`), + which auto-enables Codecov Test Analytics with no extra config — check a PR's "Tests" tab or its + Codecov bot comment if a test needed a retry, rather than assuming it's pure infra noise. --- diff --git a/.github/actions/setup-workspace/action.yml b/.github/actions/setup-workspace/action.yml new file mode 100644 index 0000000000..28e90275c7 --- /dev/null +++ b/.github/actions/setup-workspace/action.yml @@ -0,0 +1,85 @@ +name: Setup workspace +description: >- + Strip untrusted npm config, set up Node, and restore/install/save the fork/trusted-scoped + node_modules cache. Extracted from ci.yml's validate-code, validate-tests, and validate-tests-merge + jobs, which had this exact sequence copy-pasted three times -- the same kind of drift that caused a + real cache-key mismatch bug this repo already hit once (two jobs' Turborepo cache pair silently + diverged when one was edited and the other wasn't). This doesn't fix that specific bug on its own, + but a future change to this sequence now only needs to happen here, not be remembered at every call + site. Does NOT check out the repo itself -- a local `uses: ./path` action reference needs the repo + already on disk to even find this file, so actions/checkout must run in the calling job BEFORE this + action is invoked, not inside it. + +inputs: + save-cache: + description: >- + Whether to save the node_modules cache after a successful install. validate-tests-merge sets + this to "false" -- it only ever reads the cache validate-code/validate-tests already populate, + never writes to it, so a save here would just be redundant work. + required: false + default: "true" + +outputs: + cache-hit: + description: Whether the node_modules cache was restored (passed through from actions/cache/restore). + value: ${{ steps.node-modules-cache.outputs.cache-hit }} + +runs: + using: composite + steps: + - name: Neutralize untrusted npm config + shell: bash + run: rm -f .npmrc + + - name: Setup Node + uses: actions/setup-node@48b55a011bda9f5d6aeb4c2d9c7362e8dae4041e # v6 + with: + node-version-file: .nvmrc + cache: npm + + # actions/checkout wipes node_modules (git clean -ffdx) on every run regardless of the self-hosted + # runner's own persistence, and npm ci always deletes+reinstalls node_modules by design -- so + # neither the runner nor npm ci gives node_modules any real cross-run reuse on its own. This + # explicit restore/save pair (via GitHub's own cache service, not local disk) fills that gap: an + # exact manifest+lockfile match skips npm ci entirely. Keep fork/trusted keys separate even though + # both run on ubuntu-latest: fork PRs get read-only cache tokens, so a fork-keyed entry can never + # actually be written; trusted PRs keep their reusable cache without crossing trust boundaries. + - name: Restore node_modules cache + id: node-modules-cache + uses: actions/cache/restore@55cc8345863c7cc4c66a329aec7e433d2d1c52a9 # v6.1.0 + with: + path: | + node_modules + apps/loopover-ui/node_modules + # hashFiles('.nvmrc') matters as much as the package manifests and lockfile: a Node bump + # with no lockfile change would otherwise still hit and silently reuse node_modules whose + # native addons (sharp, workerd, fsevents) were compiled against the OLD Node's ABI. The + # manifests matter too because npm ci validates package.json/package-lock.json consistency + # and runs lifecycle scripts from package.json; a package.json-only change must not skip it. + key: npm-${{ (github.event_name == 'pull_request' && github.event.pull_request.head.repo.fork == true) && 'fork' || 'trusted' }}-${{ hashFiles('.nvmrc') }}-${{ hashFiles('package.json', 'apps/*/package.json', 'packages/*/package.json', 'package-lock.json') }} + + - name: Install dependencies (retry on transient failures) + if: ${{ steps.node-modules-cache.outputs.cache-hit != 'true' }} + shell: bash + run: | + for attempt in 1 2 3; do + if npm ci --prefer-offline --no-audit --no-fund; then + exit 0 + fi + echo "::warning::npm ci failed (attempt ${attempt}/3); retrying in 10s" + sleep 10 + done + echo "::error::npm ci failed after 3 attempts" + exit 1 + + # Placed immediately after install (not as an automatic post-job hook) so a cache is only ever + # saved once npm ci has actually succeeded -- a job that fails here never reaches this step, so a + # broken/partial node_modules can never get written to the cache for a future run to inherit. + - name: Save node_modules cache + if: ${{ inputs.save-cache == 'true' && steps.node-modules-cache.outputs.cache-hit != 'true' }} + uses: actions/cache/save@55cc8345863c7cc4c66a329aec7e433d2d1c52a9 # v6.1.0 + with: + path: | + node_modules + apps/loopover-ui/node_modules + key: ${{ steps.node-modules-cache.outputs.cache-primary-key }} diff --git a/.github/workflows/cache-cleanup.yml b/.github/workflows/cache-cleanup.yml new file mode 100644 index 0000000000..bac2147d06 --- /dev/null +++ b/.github/workflows/cache-cleanup.yml @@ -0,0 +1,56 @@ +name: Clean up closed-PR caches + +# Every PR-triggered run in ci.yml (node_modules, Turborepo, tsbuildinfo) saves its cache scoped to +# that PR's refs/pull/N/merge ref -- GitHub's own cache isolation, separate from the key string itself +# (two different PRs can share an identical key but still get two separate physical cache entries, +# because they're scoped to different refs). Once a PR closes, nothing can ever restore a cache scoped +# to its ref again -- and this repo auto-closes a red contributor PR one-shot (no reopen, no retry on +# the same PR), so most of those entries are written once and then permanently unreachable, just +# waiting on GitHub's passive 7-day-unused eviction. Confirmed live before adding this workflow: repo +# cache usage sat at ~10.7GB of the 10GB budget, with closed-PR-scoped node_modules caches alone +# accounting for the large majority of that -- crowding out the much smaller, much more useful +# Turborepo/tsbuildinfo caches for LRU survival. This deletes a PR's own cache entries the moment it +# closes (merged or not) instead of waiting on eviction. +# +# pull_request_target, not pull_request: this job only ever calls the GitHub API using +# github.event.pull_request.number -- a trusted value GitHub itself populates, never anything read +# from the PR's own code or checked out from it -- so the classic pull_request_target risk (running +# fork-controlled code with base-repo credentials) doesn't apply here. It has to be +# pull_request_target specifically because a fork-triggered plain `pull_request` run is always capped +# to a read-only token regardless of the permissions block below, and deleting a cache needs +# actions: write. + +on: + pull_request_target: + types: [closed] + +permissions: + actions: write + +concurrency: + group: cache-cleanup-${{ github.event.pull_request.number }} + cancel-in-progress: true + +jobs: + cleanup: + name: Delete this PR's caches + runs-on: ubuntu-latest + timeout-minutes: 5 + steps: + # Scoped by ref, not by key prefix (e.g. "npm-fork-") -- ANY cache entry scoped to this PR's ref + # (node_modules, Turborepo, tsbuildinfo, trusted or fork) is equally unreachable dead weight the + # moment the PR closes, regardless of which of ci.yml's cache families wrote it. + - name: Delete caches scoped to this PR + env: + GH_TOKEN: ${{ secrets.GITHUB_TOKEN }} + PR_REF: refs/pull/${{ github.event.pull_request.number }}/merge + run: | + ids=$(gh api "repos/${{ github.repository }}/actions/caches?per_page=100" --paginate --jq ".actions_caches[] | select(.ref == \"$PR_REF\") | .id") + if [ -z "$ids" ]; then + echo "No caches found for $PR_REF" + exit 0 + fi + for id in $ids; do + echo "Deleting cache $id ($PR_REF)" + gh api -X DELETE "repos/${{ github.repository }}/actions/caches/$id" || echo "::warning::Failed to delete cache $id (may already be gone)" + done diff --git a/.github/workflows/ci-duration-report.yml b/.github/workflows/ci-duration-report.yml new file mode 100644 index 0000000000..2b62bdfc06 --- /dev/null +++ b/.github/workflows/ci-duration-report.yml @@ -0,0 +1,55 @@ +name: CI duration report + +# Nothing else in this repo tracks whether ci.yml is trending slower or flakier over time -- catching +# that currently requires a manual audit (this workflow exists because one just happened). Purely +# additive and read-only: it doesn't touch ci.yml, doesn't gate anything, and can't fail a PR. Mirrors +# audit.yml's shape (scheduled + workflow_dispatch, single job). + +on: + schedule: + - cron: "0 15 * * 1" # Mondays 15:00 UTC, an hour ahead of audit.yml so they don't contend + workflow_dispatch: + +permissions: + contents: read + actions: read + +concurrency: + group: ci-duration-report + cancel-in-progress: true + +jobs: + report: + name: report + runs-on: ubuntu-latest + timeout-minutes: 5 + steps: + - name: Checkout + uses: actions/checkout@9c091bb21b7c1c1d1991bb908d89e4e9dddfe3e0 # v7 + - name: Setup Node + uses: actions/setup-node@48b55a011bda9f5d6aeb4c2d9c7362e8dae4041e # v6 + with: + node-version-file: .nvmrc + - name: Pull ci.yml run stats + env: + GITHUB_TOKEN: ${{ github.token }} + run: node scripts/ci-duration-report.mjs --days=7 --output=ci-duration-report.json + - name: Write summary + run: | + node <<'NODE' + const fs = require("node:fs"); + const report = JSON.parse(fs.readFileSync("ci-duration-report.json", "utf8")); + const fmt = (s) => (s === null ? "n/a" : `${Math.round(s / 60)}m ${Math.round(s % 60)}s`); + const pct = (r) => (r === null ? "n/a" : `${Math.round(r * 100)}%`); + const lines = [ + `## CI duration report (trailing ${report.windowDays} days)`, + "", + "| Trigger | Runs | p50 | p95 | Failure rate | Cancelled (excluded) |", + "|---|---|---|---|---|---|", + `| push | ${report.push.count} | ${fmt(report.push.p50Seconds)} | ${fmt(report.push.p95Seconds)} | ${pct(report.push.failureRate)} | ${report.push.excludedCancelled} |`, + `| pull_request | ${report.pullRequest.count} | ${fmt(report.pullRequest.p50Seconds)} | ${fmt(report.pullRequest.p95Seconds)} | ${pct(report.pullRequest.failureRate)} | ${report.pullRequest.excludedCancelled} |`, + "", + "Duration is wall-clock (queue time included), not summed job time. \"Failure rate\" excludes cancelled runs (almost always a rapid re-push superseding its predecessor, not CI breaking) from both the count and the denominator.", + ]; + fs.appendFileSync(process.env.GITHUB_STEP_SUMMARY, `${lines.join("\n")}\n`); + NODE diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index a86f58bd77..8b90bba94a 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -202,62 +202,17 @@ jobs: env: VITE_LOOPOVER_API_ORIGIN: https://api.loopover.ai steps: + # Shallow (default depth): this job no longer uploads to Codecov -- that moved to validate-tests + # (#ci-shard-coverage) -- so it has no reason to fetch full history anymore. - name: Checkout uses: actions/checkout@9c091bb21b7c1c1d1991bb908d89e4e9dddfe3e0 # v7 with: - # Shallow (default depth): this job no longer uploads to Codecov -- that moved to validate-tests - # (#ci-shard-coverage) -- so it has no reason to fetch full history anymore. ref: ${{ github.event_name == 'pull_request' && github.event.pull_request.head.sha || github.sha }} - - name: Neutralize untrusted npm config - run: rm -f .npmrc - - name: Setup Node - uses: actions/setup-node@48b55a011bda9f5d6aeb4c2d9c7362e8dae4041e # v6 - with: - node-version-file: .nvmrc - cache: npm - # actions/checkout wipes node_modules (git clean -ffdx) on every run regardless of the self-hosted - # runner's own persistence, and npm ci always deletes+reinstalls node_modules by design -- so - # neither the runner nor npm ci gives node_modules any real cross-run reuse on its own. This - # explicit restore/save pair (via GitHub's own cache service, not local disk) fills that gap: an - # exact manifest+lockfile match skips npm ci entirely. Keep fork/trusted keys separate even though - # both run on ubuntu-latest: fork PRs get read-only cache tokens, so a fork-keyed entry can never - # actually be written; trusted PRs keep their reusable cache without crossing trust boundaries. - - name: Restore node_modules cache - id: node-modules-cache - uses: actions/cache/restore@55cc8345863c7cc4c66a329aec7e433d2d1c52a9 # v6.1.0 - with: - path: | - node_modules - apps/loopover-ui/node_modules - # hashFiles('.nvmrc') matters as much as the package manifests and lockfile: a Node bump - # with no lockfile change would otherwise still hit and silently reuse node_modules whose - # native addons (sharp, workerd, fsevents) were compiled against the OLD Node's ABI. The - # manifests matter too because npm ci validates package.json/package-lock.json consistency - # and runs lifecycle scripts from package.json; a package.json-only change must not skip it. - key: npm-${{ (github.event_name == 'pull_request' && github.event.pull_request.head.repo.fork == true) && 'fork' || 'trusted' }}-${{ hashFiles('.nvmrc') }}-${{ hashFiles('package.json', 'apps/*/package.json', 'packages/*/package.json', 'package-lock.json') }} - - name: Install dependencies (retry on transient failures) - if: ${{ steps.node-modules-cache.outputs.cache-hit != 'true' }} - run: | - for attempt in 1 2 3; do - if npm ci --prefer-offline --no-audit --no-fund; then - exit 0 - fi - echo "::warning::npm ci failed (attempt ${attempt}/3); retrying in 10s" - sleep 10 - done - echo "::error::npm ci failed after 3 attempts" - exit 1 - # Placed immediately after install (not as an automatic post-job hook) so a cache is only ever - # saved once npm ci has actually succeeded -- a job that fails here never reaches this step, so a - # broken/partial node_modules can never get written to the cache for a future run to inherit. - - name: Save node_modules cache - if: ${{ steps.node-modules-cache.outputs.cache-hit != 'true' }} - uses: actions/cache/save@55cc8345863c7cc4c66a329aec7e433d2d1c52a9 # v6.1.0 - with: - path: | - node_modules - apps/loopover-ui/node_modules - key: ${{ steps.node-modules-cache.outputs.cache-primary-key }} + # See .github/actions/setup-workspace for what this covers (npmrc, Node, node_modules cache) -- + # the checkout above stays a separate, explicit step since a local `uses: ./path` action needs + # the repo already on disk to even find its own action.yml. + - name: Setup workspace + uses: ./.github/actions/setup-workspace # apps/loopover-ui/.source (fumadocs-mdx codegen) is regenerated by "Generate docs content # collections" below, gated on push||ui==true -- nothing between here and there reads it (docs-drift, # env-reference, manifest/engine-parity/branding-drift checks all read raw source files directly, and @@ -748,44 +703,12 @@ jobs: # Full history so Codecov can resolve the merge base for patch coverage. fetch-depth: 0 ref: ${{ github.event_name == 'pull_request' && github.event.pull_request.head.sha || github.sha }} - - name: Neutralize untrusted npm config - run: rm -f .npmrc - - name: Setup Node - uses: actions/setup-node@48b55a011bda9f5d6aeb4c2d9c7362e8dae4041e # v6 - with: - node-version-file: .nvmrc - cache: npm - # Same cache key formula as validate-code's own restore/save pair, so a lockfile-unchanged PR gets a - # cache hit here too and skips npm ci entirely. Concurrent jobs racing to save the same key is safe: - # actions/cache/save no-ops (warns, doesn't fail) if another job already wrote that exact key. - - name: Restore node_modules cache - id: node-modules-cache - uses: actions/cache/restore@55cc8345863c7cc4c66a329aec7e433d2d1c52a9 # v6.1.0 - with: - path: | - node_modules - apps/loopover-ui/node_modules - key: npm-${{ (github.event_name == 'pull_request' && github.event.pull_request.head.repo.fork == true) && 'fork' || 'trusted' }}-${{ hashFiles('.nvmrc') }}-${{ hashFiles('package.json', 'apps/*/package.json', 'packages/*/package.json', 'package-lock.json') }} - - name: Install dependencies (retry on transient failures) - if: ${{ steps.node-modules-cache.outputs.cache-hit != 'true' }} - run: | - for attempt in 1 2 3; do - if npm ci --prefer-offline --no-audit --no-fund; then - exit 0 - fi - echo "::warning::npm ci failed (attempt ${attempt}/3); retrying in 10s" - sleep 10 - done - echo "::error::npm ci failed after 3 attempts" - exit 1 - - name: Save node_modules cache - if: ${{ steps.node-modules-cache.outputs.cache-hit != 'true' }} - uses: actions/cache/save@55cc8345863c7cc4c66a329aec7e433d2d1c52a9 # v6.1.0 - with: - path: | - node_modules - apps/loopover-ui/node_modules - key: ${{ steps.node-modules-cache.outputs.cache-primary-key }} + # See .github/actions/setup-workspace (npmrc, Node, node_modules cache -- same cache key formula + # as validate-code, so a lockfile-unchanged PR gets a cache hit here too and skips npm ci + # entirely; concurrent jobs racing to save the same key is safe, actions/cache/save no-ops if + # another job already wrote it). + - name: Setup workspace + uses: ./.github/actions/setup-workspace # Any backend test run needs the engine package's dist/ built first -- see the identical step's # comment in validate-code (#ci-engine-build-order) for why. Same accumulating .turbo/cache # restore/save pattern as validate-code's pair, but a DIFFERENT key prefix ("turbo-tests-" vs. @@ -975,35 +898,14 @@ jobs: uses: actions/checkout@9c091bb21b7c1c1d1991bb908d89e4e9dddfe3e0 # v7 with: ref: ${{ github.event_name == 'pull_request' && github.event.pull_request.head.sha || github.sha }} - - name: Neutralize untrusted npm config - run: rm -f .npmrc - - name: Setup Node - uses: actions/setup-node@48b55a011bda9f5d6aeb4c2d9c7362e8dae4041e # v6 - with: - node-version-file: .nvmrc - cache: npm # Same cache key as validate-code/validate-tests -- a cache hit here is the common case since those - # jobs run concurrently and one of them usually wins the race to populate it first. - - name: Restore node_modules cache - id: node-modules-cache - uses: actions/cache/restore@55cc8345863c7cc4c66a329aec7e433d2d1c52a9 # v6.1.0 + # jobs run concurrently and one of them usually wins the race to populate it first. save-cache: + # false -- this job only ever reads that cache, it never writes to it (see + # .github/actions/setup-workspace). + - name: Setup workspace + uses: ./.github/actions/setup-workspace with: - path: | - node_modules - apps/loopover-ui/node_modules - key: npm-${{ (github.event_name == 'pull_request' && github.event.pull_request.head.repo.fork == true) && 'fork' || 'trusted' }}-${{ hashFiles('.nvmrc') }}-${{ hashFiles('package.json', 'apps/*/package.json', 'packages/*/package.json', 'package-lock.json') }} - - name: Install dependencies (retry on transient failures) - if: ${{ steps.node-modules-cache.outputs.cache-hit != 'true' }} - run: | - for attempt in 1 2 3; do - if npm ci --prefer-offline --no-audit --no-fund; then - exit 0 - fi - echo "::warning::npm ci failed (attempt ${attempt}/3); retrying in 10s" - sleep 10 - done - echo "::error::npm ci failed after 3 attempts" - exit 1 + save-cache: "false" - name: Download all shards' blob reports uses: actions/download-artifact@3e5f45b2cfb9172054b4087a40e8e0b5a5461e7c # v8.0.1 with: diff --git a/.github/workflows/ui-preview.yml b/.github/workflows/ui-preview.yml index f9f32cc8f1..d333409c55 100644 --- a/.github/workflows/ui-preview.yml +++ b/.github/workflows/ui-preview.yml @@ -60,6 +60,12 @@ jobs: - name: Install dependencies run: npm ci + # Same steps as the root `ui:build` script, except the last one: that script also builds + # @loopover/ui-miner (`npm --workspace @loopover/ui-miner run build`), a full separate Vite app + # this workflow never uploads or deploys (only apps/loopover-ui/dist below) -- every UI PR was + # paying for that build and throwing away the result. `npx turbo run build --filter=@loopover/ui` + # replaces the last two `ui:build` steps (`extension:build && miner-extension:build && ui build` + # already covered by @loopover/ui#build's own turbo.json dependsOn) and skips ui-miner entirely. - name: Build UI env: VITE_LOOPOVER_API_ORIGIN: https://api.loopover.ai @@ -68,7 +74,7 @@ jobs: # production build (ui-deploy.yml) does NOT set this, so the escape hatch is dead-code-eliminated # from prod. (#authed-route-preview) VITE_PREVIEW: "1" - run: npm run ui:build + run: npm run ui:kit:build && npx turbo run build --filter=@loopover/engine && npm run ui:openapi && npx turbo run build --filter=@loopover/ui # The trusted deploy workflow downloads this by name + run-id. It contains only the built bundle # (server/ + client/) — no secrets, no source needed downstream. diff --git a/scripts/ci-duration-report.mjs b/scripts/ci-duration-report.mjs new file mode 100644 index 0000000000..813ef1ce51 --- /dev/null +++ b/scripts/ci-duration-report.mjs @@ -0,0 +1,90 @@ +#!/usr/bin/env node +// Reports p50/p95 wall-clock duration and failure rate for the ci.yml workflow over a trailing +// window, split by trigger (push vs pull_request) since they run different amounts of work (a push +// to main always runs the full unscoped suite; a PR can hit scoped test selection). Nothing currently +// tracks whether CI is trending slower over time -- this closes that blind spot without touching +// ci.yml itself. Duration is measured as updated_at - created_at (the run's real wall-clock span, +// including queue time), not the sum of individual job durations. + +import { writeFileSync } from "node:fs"; + +const WINDOW_DAYS = Number(process.argv.find((a) => a.startsWith("--days="))?.split("=")[1] ?? 7); +const outputArg = process.argv.find((a) => a.startsWith("--output=")); +const OUTPUT_PATH = outputArg ? outputArg.split("=")[1] : null; + +const repo = process.env.GITHUB_REPOSITORY; +if (!repo) throw new Error("GITHUB_REPOSITORY is required"); +const token = process.env.GITHUB_TOKEN; +if (!token) throw new Error("GITHUB_TOKEN is required"); + +async function fetchAllRuns(sinceIso) { + const runs = []; + let page = 1; + for (;;) { + const response = await fetch( + `https://api.github.com/repos/${repo}/actions/workflows/ci.yml/runs?status=completed&created=>=${sinceIso}&per_page=100&page=${page}`, + { + headers: { + Authorization: `Bearer ${token}`, + Accept: "application/vnd.github+json", + "X-GitHub-Api-Version": "2022-11-28", + }, + }, + ); + if (!response.ok) { + throw new Error(`GitHub API error ${response.status}: ${await response.text()}`); + } + const body = await response.json(); + runs.push(...body.workflow_runs); + if (body.workflow_runs.length < 100) break; + page += 1; + if (page > 20) break; // hard stop -- ~2000 runs is far more than a 7-day window should ever return + } + return runs; +} + +function durationSeconds(run) { + return (new Date(run.updated_at).getTime() - new Date(run.created_at).getTime()) / 1000; +} + +function percentile(sortedValues, p) { + if (sortedValues.length === 0) return null; + const index = Math.min(sortedValues.length - 1, Math.ceil((p / 100) * sortedValues.length) - 1); + return sortedValues[Math.max(0, index)]; +} + +function summarize(allRuns) { + // "cancelled" excluded entirely, not just from the failure count: this workflow's own + // cancel-in-progress concurrency setting means a cancelled run is almost always a rapid re-push + // superseding its predecessor mid-run, not CI breaking -- counting it as a failure (or even as a + // completed run for duration purposes, since it stopped partway through) would misrepresent both + // metrics. "skipped" is real (a path-filtered job counting as success) and stays in the success side. + const runs = allRuns.filter((r) => r.conclusion !== "cancelled"); + const durations = runs.map(durationSeconds).sort((a, b) => a - b); + const failures = runs.filter((r) => r.conclusion !== "success" && r.conclusion !== "skipped").length; + return { + count: runs.length, + excludedCancelled: allRuns.length - runs.length, + p50Seconds: percentile(durations, 50), + p95Seconds: percentile(durations, 95), + failureRate: runs.length > 0 ? failures / runs.length : null, + failures, + }; +} + +const since = new Date(Date.now() - WINDOW_DAYS * 24 * 60 * 60 * 1000).toISOString(); +const runs = await fetchAllRuns(since); + +const report = { + windowDays: WINDOW_DAYS, + generatedAt: new Date().toISOString(), + push: summarize(runs.filter((r) => r.event === "push")), + pullRequest: summarize(runs.filter((r) => r.event === "pull_request")), +}; + +const json = JSON.stringify(report, null, 2); +if (OUTPUT_PATH) { + writeFileSync(OUTPUT_PATH, json); +} else { + process.stdout.write(`${json}\n`); +} diff --git a/test/unit/ci-composite-setup-workspace.test.ts b/test/unit/ci-composite-setup-workspace.test.ts new file mode 100644 index 0000000000..5a84239311 --- /dev/null +++ b/test/unit/ci-composite-setup-workspace.test.ts @@ -0,0 +1,122 @@ +import { readFileSync } from "node:fs"; +import { parse } from "yaml"; +import { describe, expect, it } from "vitest"; + +function readYaml(path: string): Record { + return record(parse(readFileSync(path, "utf8")), path); +} + +function record(value: unknown, label: string): Record { + if (!value || typeof value !== "object" || Array.isArray(value)) { + throw new Error(`${label} must be an object`); + } + return value as Record; +} + +function recordArray(value: unknown, label: string): Array> { + if (!Array.isArray(value)) throw new Error(`${label} must be an array`); + return value.map((entry, index) => record(entry, `${label}[${index}]`)); +} + +function step(steps: Array>, name: string): Record { + const found = steps.find((candidate) => candidate.name === name); + if (!found) throw new Error(`step "${name}" not found`); + return found; +} + +function jobSteps(workflow: Record, jobName: string): Array> { + const job = record(record(workflow.jobs, "jobs")[jobName], jobName); + return recordArray(job.steps, `${jobName}.steps`); +} + +const ACTION_PATH = ".github/actions/setup-workspace/action.yml"; + +// .github/actions/setup-workspace holds logic that used to be hand-copied across validate-code, +// validate-tests, and validate-tests-merge -- the same drift risk that caused a real cache-key +// mismatch bug this repo already hit once (two jobs' Turborepo cache pair silently diverged when one +// was edited and the other wasn't). It does NOT check out the repo itself (a local `uses: ./path` +// reference needs the repo already on disk to find its own action.yml), so every call site must run +// its own actions/checkout step immediately before invoking this action. +describe("setup-workspace composite action", () => { + it("is a composite action with the expected inputs/outputs", () => { + const action = readYaml(ACTION_PATH); + expect(record(action.runs, "runs").using).toBe("composite"); + expect(record(action.inputs, "inputs")["save-cache"]).toBeDefined(); + expect(record(action.outputs, "outputs")["cache-hit"]).toBeDefined(); + }); + + it("skips npm ci only on an exact node_modules cache hit, and saves the cache only after a successful install", () => { + const action = readYaml(ACTION_PATH); + const steps = recordArray(record(action.runs, "runs").steps, "runs.steps"); + + const restore = step(steps, "Restore node_modules cache"); + expect(restore.uses).toContain("actions/cache/restore@"); + const restoreWith = record(restore.with, "restore.with"); + expect(String(restoreWith.path)).toContain("node_modules"); + expect(String(restoreWith.path)).toContain("apps/loopover-ui/node_modules"); + expect(String(restoreWith.key)).toContain("hashFiles('package.json', 'apps/*/package.json', 'packages/*/package.json', 'package-lock.json')"); + expect(String(restoreWith.key)).toContain("package.json"); + expect(String(restoreWith.key)).toContain("apps/*/package.json"); + expect(String(restoreWith.key)).toContain("packages/*/package.json"); + expect(String(restoreWith.key)).toContain("package-lock.json"); + // A Node bump (.nvmrc) with no lockfile change must still bust the cache -- otherwise a hit would + // silently reuse node_modules whose native addons were compiled against the OLD Node's ABI. + expect(String(restoreWith.key)).toContain("hashFiles('.nvmrc')"); + expect(String(restoreWith.key)).toContain("fork"); + expect(String(restoreWith.key)).toContain("trusted"); + + const install = step(steps, "Install dependencies (retry on transient failures)"); + expect(String(install.if)).toContain("steps.node-modules-cache.outputs.cache-hit != 'true'"); + + const save = step(steps, "Save node_modules cache"); + expect(String(save.if)).toContain("inputs.save-cache == 'true'"); + expect(String(save.if)).toContain("steps.node-modules-cache.outputs.cache-hit != 'true'"); + expect(save.uses).toContain("actions/cache/save@"); + const saveWith = record(save.with, "save.with"); + expect(saveWith.key).toBe("${{ steps.node-modules-cache.outputs.cache-primary-key }}"); + + // Save must come after install (a broken/partial node_modules from a failed install step is never reached). + const stepNames = steps.map((s) => s.name); + expect(stepNames.indexOf("Save node_modules cache")).toBeGreaterThan(stepNames.indexOf("Install dependencies (retry on transient failures)")); + + // Every run: step inside a composite action needs its own explicit shell (unlike a top-level + // workflow job, which defaults to bash on a Linux runner) -- a missing one is a silent hard failure + // at actual run time, not a parse-time error, so it's worth asserting here where it's cheap to catch. + for (const s of steps) { + if (s.run !== undefined) expect(s.shell, `step "${s.name}" has a run: but no shell:`).toBeDefined(); + } + }); + + it.each([ + { job: "validate-code", saveCache: undefined }, + { job: "validate-tests", saveCache: undefined }, + { job: "validate-tests-merge", saveCache: "false" }, + ])("$job invokes the composite action (save-cache: $saveCache)", ({ job, saveCache }) => { + const steps = jobSteps(readYaml(".github/workflows/ci.yml"), job); + + // Each call site still runs its own actions/checkout step first -- the composite action can't do + // this itself (see the action's own description). + const checkoutIndex = steps.findIndex((s) => s.name === "Checkout"); + expect(checkoutIndex, `${job} has no Checkout step`).toBeGreaterThanOrEqual(0); + expect(steps[checkoutIndex]?.uses).toContain("actions/checkout@"); + + const setupIndex = steps.findIndex((s) => s.name === "Setup workspace"); + expect(setupIndex, `${job} has no Setup workspace step`).toBeGreaterThan(checkoutIndex); + const setupStep = steps[setupIndex]; + expect(setupStep?.uses).toBe("./.github/actions/setup-workspace"); + + if (saveCache === undefined) { + // Default (unset with: block, or a with: block that omits save-cache) -- the action's own + // default is "true", so nothing further to assert here. + expect(record((setupStep?.with as Record | undefined) ?? {}, "with")["save-cache"]).toBeUndefined(); + } else { + expect(record(setupStep?.with, `${job}.with`)["save-cache"]).toBe(saveCache); + } + }); + + it("validate-tests' Checkout uses fetch-depth: 0 (Codecov needs full history for the merge base)", () => { + const steps = jobSteps(readYaml(".github/workflows/ci.yml"), "validate-tests"); + const checkout = step(steps, "Checkout"); + expect(record(checkout.with, "checkout.with")["fetch-depth"]).toBe(0); + }); +}); diff --git a/test/unit/ci-dependency-cache.test.ts b/test/unit/ci-dependency-cache.test.ts index 4d414ff0b1..0e02791e05 100644 --- a/test/unit/ci-dependency-cache.test.ts +++ b/test/unit/ci-dependency-cache.test.ts @@ -29,45 +29,13 @@ function jobSteps(workflow: Record, jobName: string): Array { - it("root npm ci is skipped only on an exact node_modules cache hit, and the cache is saved only after a successful install", () => { - const steps = jobSteps(readYaml(".github/workflows/ci.yml"), "validate-code"); - - const restore = step(steps, "Restore node_modules cache"); - expect(restore.uses).toContain("actions/cache/restore@"); - const restoreWith = record(restore.with, "restore.with"); - expect(String(restoreWith.path)).toContain("node_modules"); - expect(String(restoreWith.path)).toContain("apps/loopover-ui/node_modules"); - expect(String(restoreWith.key)).toContain("hashFiles('package.json', 'apps/*/package.json', 'packages/*/package.json', 'package-lock.json')"); - expect(String(restoreWith.key)).toContain("package.json"); - expect(String(restoreWith.key)).toContain("apps/*/package.json"); - expect(String(restoreWith.key)).toContain("packages/*/package.json"); - expect(String(restoreWith.key)).toContain("package-lock.json"); - // A Node bump (.nvmrc) with no lockfile change must still bust the cache -- otherwise a hit would - // silently reuse node_modules whose native addons were compiled against the OLD Node's ABI. - expect(String(restoreWith.key)).toContain("hashFiles('.nvmrc')"); - expect(String(restoreWith.key)).toContain("fork"); - expect(String(restoreWith.key)).toContain("trusted"); - - const install = step(steps, "Install dependencies (retry on transient failures)"); - expect(String(install.if)).toContain("steps.node-modules-cache.outputs.cache-hit != 'true'"); - - const save = step(steps, "Save node_modules cache"); - expect(String(save.if)).toContain("steps.node-modules-cache.outputs.cache-hit != 'true'"); - expect(save.uses).toContain("actions/cache/save@"); - const saveWith = record(save.with, "save.with"); - expect(saveWith.key).toBe("${{ steps.node-modules-cache.outputs.cache-primary-key }}"); - - // Save must come after install (a broken/partial node_modules from a failed install step is never reached). - const stepNames = steps.map((s) => s.name); - expect(stepNames.indexOf("Save node_modules cache")).toBeGreaterThan(stepNames.indexOf("Install dependencies (retry on transient failures)")); - }); - it("review-enrichment's install is cached separately (its own lockfile, not an npm workspace member)", () => { const steps = jobSteps(readYaml(".github/workflows/ci.yml"), "validate-code"); diff --git a/test/unit/observability-ci.test.ts b/test/unit/observability-ci.test.ts index 6b369b04df..c973f8ce84 100644 --- a/test/unit/observability-ci.test.ts +++ b/test/unit/observability-ci.test.ts @@ -33,7 +33,12 @@ describe("observability config CI guard", () => { const validateCode = nestedRecord(workflow, ["jobs", "validate-code"]); const validateSteps = recordArray(validateCode.steps, "jobs.validate-code.steps"); - const neutralizeStep = validateSteps.find((step) => step.name === "Neutralize untrusted npm config"); + // "Setup workspace" (a local composite action, .github/actions/setup-workspace) replaced the + // inline "Neutralize untrusted npm config"/Node-setup/node_modules-cache steps that used to live + // directly in this job -- see ci-composite-setup-workspace.test.ts for what that action itself + // contains. Checked here only as a parse-sanity canary (confirms validateSteps is really this + // job's step list), same role the removed npmrc-step check served. + const setupWorkspaceStep = validateSteps.find((step) => step.name === "Setup workspace"); const validateStep = validateSteps.find((step) => step.name === "Validate observability configs"); expect(outputs.observability).toBe("${{ steps.filter.outputs.observability }}"); @@ -42,8 +47,8 @@ describe("observability config CI guard", () => { expect(filters).toContain("observability:"); expect(filters).toContain("grafana/dashboards/**"); expect(filters).toContain("prometheus/rules/**"); - expect(neutralizeStep).toBeDefined(); - expect(neutralizeStep!.run).toBe("rm -f .npmrc"); + expect(setupWorkspaceStep).toBeDefined(); + expect(setupWorkspaceStep!.uses).toBe("./.github/actions/setup-workspace"); expect(validateStep).toBeDefined(); // Not gated on `backend`: scripts/validate-observability-configs.mjs only ever reads // grafana/dashboards/*.json and prometheus/rules/alerts.yml, both fully covered by the diff --git a/vitest.config.ts b/vitest.config.ts index c4dedc6522..c3086e660d 100644 --- a/vitest.config.ts +++ b/vitest.config.ts @@ -19,6 +19,11 @@ export default defineConfig({ // Retry a failed test once before failing the run. The loopover gate auto-CLOSES a contributor PR // on a red required CI, so a single transient flake must not kill an honest PR; a deterministic // failure still fails both attempts (and vitest flags the retried test as flaky so it stays visible). + // "Stays visible" means Codecov Test Analytics, not just the vitest run log: ci.yml already uploads + // each shard's JUnit report with report_type: test_results, which auto-enables Codecov's flaky-test + // detection with no extra config -- it's live today (see the "Tests" tab on any recent PR/commit in + // Codecov, or that PR's own Codecov bot comment). No dashboard is wired up to surface it proactively, + // so check it deliberately if a retry shows up in CI output rather than assuming it's pure infra noise. retry: 1, include: ["test/**/*.test.ts"], exclude: ["test/workers/**/*.test.ts"],