From fca1486848d98fc93e81310f1d3db0bcac1545c7 Mon Sep 17 00:00:00 2001 From: JSONbored <49853598+JSONbored@users.noreply.github.com> Date: Sun, 19 Jul 2026 16:06:40 -0700 Subject: [PATCH] ci: cache cleanup, duration reporting, extension waste fix, composite setup action Six independent CI hygiene/reliability improvements from the "is everything 100% optimized" follow-up audit, bundled since each is small and none conflict: - Clean up dead fork-PR caches (.github/workflows/cache-cleanup.yml): live cache usage was at ~10.7GB of the 10GB budget, 87% of it single-use node_modules caches from contributor PRs that auto-closed one-shot and can never be restored again. Deletes a PR's own cache entries (any prefix -- node_modules, Turborepo, tsbuildinfo -- scoped by ref, not key string) the moment it closes, via pull_request_target since deleting a cache needs actions:write and a fork-triggered pull_request run is always capped to a read-only token. Never checks out or executes anything from the PR's own code, only ever reads the trusted PR number GitHub itself populates. - Weekly CI duration report (.github/workflows/ci-duration-report.yml + scripts/ci-duration-report.mjs): nothing currently tracks whether ci.yml is trending slower over time. Pulls p50/p95 wall-clock duration and failure rate for the trailing week, split by push vs pull_request since they run different amounts of work, excluding cancelled runs from both (confirmed live: ~9% of recent runs are cancelled, almost always a rapid re-push superseding its predecessor, not CI breaking -- counting them as failures would misrepresent the real rate). Purely additive, read-only, can't fail a PR. - Stop ui-preview.yml building @loopover/ui-miner: it ran the full ui:build aggregate, which also builds a full separate Vite app this workflow never uploads or deploys. Swapped to the same turbo-routed build ci.yml's own "UI build" step already uses, minus the wasted half. - Extracted .github/actions/setup-workspace, a composite action covering the neutralize-npmrc/Setup-Node/restore-install-save-node_modules sequence that was hand-copied identically across validate-code, validate-tests, and validate-tests-merge -- the same kind of drift that caused a real cache-key mismatch bug this repo already hit once this session (two jobs' Turborepo cache pair silently diverged when one was edited and the other wasn't). Does NOT include the checkout step itself: a local `uses: ./path` action needs the repo already on disk to find its own action.yml, so each call site keeps its own actions/checkout (fetch-depth varies) immediately before invoking this action. - Surfaced Codecov Test Analytics (already auto-enabled from the JUnit uploads ci.yml already sends, just not linked from anywhere) and fixed two stale claims in the contributing skill docs: "the single required status check is validate" (Superagent Security Scan is also required, confirmed via the live branch protection API) and "CI shards into 2" (it's 6). test/unit/ci-dependency-cache.test.ts's node_modules-cache assertions moved to the new test/unit/ci-composite-setup-workspace.test.ts (now checking the composite action's own content plus each call site's invocation), since that logic no longer lives directly in ci.yml's step list. test/unit/observability-ci.test.ts's incidental npmrc-step canary check was swapped for a "Setup workspace" one serving the same parse-sanity purpose. Note: this repo's own actionlint wrapper (scripts/actionlint.mjs) only scans .github/workflows/**, not .github/actions/**, and the underlying tool doesn't auto-detect composite action files the way it does for workflows (confirmed by trying and reverting a widen-the-glob attempt -- it produced false-positive "missing jobs/on section" errors). The new composite action's YAML was hand-validated for syntax/structure instead; this gap is real and worth closing properly in a future PR, not silently routed around here. --- .../contributing-to-loopover/reference.md | 23 +-- .github/actions/setup-workspace/action.yml | 85 +++++++++++ .github/workflows/cache-cleanup.yml | 56 ++++++++ .github/workflows/ci-duration-report.yml | 55 +++++++ .github/workflows/ci.yml | 136 +++--------------- .github/workflows/ui-preview.yml | 8 +- scripts/ci-duration-report.mjs | 90 ++++++++++++ .../unit/ci-composite-setup-workspace.test.ts | 122 ++++++++++++++++ test/unit/ci-dependency-cache.test.ts | 44 +----- test/unit/observability-ci.test.ts | 11 +- vitest.config.ts | 5 + 11 files changed, 467 insertions(+), 168 deletions(-) create mode 100644 .github/actions/setup-workspace/action.yml create mode 100644 .github/workflows/cache-cleanup.yml create mode 100644 .github/workflows/ci-duration-report.yml create mode 100644 scripts/ci-duration-report.mjs create mode 100644 test/unit/ci-composite-setup-workspace.test.ts 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"],