Skip to content

Commit 6c834ab

Browse files
JSONboredJSONbored
authored
Merge pull request #7379 from JSONbored/claude/ci-improvements-batch
ci: cache cleanup, duration reporting, extension waste fix, composite setup action
2 parents 321a734 + fca1486 commit 6c834ab

11 files changed

Lines changed: 467 additions & 168 deletions

File tree

‎.claude/skills/contributing-to-loopover/reference.md‎

Lines changed: 14 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -14,12 +14,14 @@ for maintainer approval (CI shows unverified → the engine **holds**, never clo
1414

1515
## 1. Every CI check → local command → what fails it
1616

17-
The single **required** status check is **`validate`** (it aggregates `changes, lint, test, workers,
18-
mcp, ui, security`; a path-skipped job counts as success). **Codecov** posts `codecov/patch` (the real
19-
coverage gate) and `codecov/project` (informational) independently. The review engine also posts its
20-
own check run named **`LoopOver Orb Review Agent`** (`src/github/app.ts` `LOOPOVER_GATE_CHECK_NAME`) — the gate
21-
verdict (§3), separate from CI. On a PR, jobs run only if their
22-
path filter matched; on push to `main`, everything runs.
17+
The **required** status checks on `main` are **`validate`** (it aggregates `changes, lint, test,
18+
workers, mcp, ui, security`; a path-skipped job counts as success) and **`Superagent Security Scan`**
19+
(a separate third-party GitHub App check, not part of this repo's own workflow files — confirmed via
20+
`gh api repos/JSONbored/loopover/branches/main/protection/required_status_checks`). **Codecov** posts
21+
`codecov/patch` (the real coverage gate) and `codecov/project` (informational) independently. The
22+
review engine also posts its own check run named **`LoopOver Orb Review Agent`**
23+
(`src/github/app.ts` `LOOPOVER_GATE_CHECK_NAME`) — the gate verdict (§3), separate from CI. On a PR,
24+
jobs run only if their path filter matched; on push to `main`, everything runs.
2325

2426
| Check | Runs | Local command | Fails when |
2527
|---|---|---|---|
@@ -32,7 +34,7 @@ path filter matched; on push to `main`, everything runs.
3234
| 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 |
3335
| lint → observability | Grafana/Prometheus/alert config validation | `npm run selfhost:validate-observability` | a self-host observability config (dashboard/rule/datasource) is malformed |
3436
| lint → typecheck | `tsc --noEmit` | `npm run typecheck` | any backend type error |
35-
| test (1/2) | sharded vitest + coverage | `npm run test:coverage` (unsharded) | any failing `test/**/*.test.ts` (excl. `test/workers/**`) |
37+
| test (1/6..6/6) | sharded vitest + coverage | `npm run test:coverage` (unsharded) | any failing `test/**/*.test.ts` (excl. `test/workers/**`) |
3638
| workers | workers-pool vitest | `npm run test:workers` | any failing `test/workers/**` |
3739
| mcp → build | MCP pkg build | `npm run build:mcp` | MCP package build error |
3840
| 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:**
7173

7274
| Local command | Why it's not in the table above |
7375
|---|---|
74-
| `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`). |
76+
| `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`). |
7577
| `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. |
7678

7779
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.
9294
- **Ignored paths** (no coverage obligation): `apps/**`, `test/**`, `scripts/**`, `src/env.d.ts`.
9395
Coverage `include` is `src/**/*.ts` only. → A UI-only / test-only / script-only change owes **no**
9496
patch coverage; a backend `src/**` change owes coverage on **every changed line + branch**.
95-
- **Measure unsharded locally:** `npm run test:coverage`. CI shards into 2 and Codecov merges them,
97+
- **Measure unsharded locally:** `npm run test:coverage`. CI shards into 6 and Codecov merges them,
9698
so a single local shard under-reports — never trust it.
99+
- **Flaky tests are already tracked.** Every shard uploads a JUnit report (`report_type: test_results`),
100+
which auto-enables Codecov Test Analytics with no extra config — check a PR's "Tests" tab or its
101+
Codecov bot comment if a test needed a retry, rather than assuming it's pure infra noise.
97102

98103
---
99104

‎.github/actions/setup-workspace/action.yml‎

Lines changed: 85 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,85 @@
1+
name: Setup workspace
2+
description: >-
3+
Strip untrusted npm config, set up Node, and restore/install/save the fork/trusted-scoped
4+
node_modules cache. Extracted from ci.yml's validate-code, validate-tests, and validate-tests-merge
5+
jobs, which had this exact sequence copy-pasted three times -- the same kind of drift that caused a
6+
real cache-key mismatch bug this repo already hit once (two jobs' Turborepo cache pair silently
7+
diverged when one was edited and the other wasn't). This doesn't fix that specific bug on its own,
8+
but a future change to this sequence now only needs to happen here, not be remembered at every call
9+
site. Does NOT check out the repo itself -- a local `uses: ./path` action reference needs the repo
10+
already on disk to even find this file, so actions/checkout must run in the calling job BEFORE this
11+
action is invoked, not inside it.
12+
13+
inputs:
14+
save-cache:
15+
description: >-
16+
Whether to save the node_modules cache after a successful install. validate-tests-merge sets
17+
this to "false" -- it only ever reads the cache validate-code/validate-tests already populate,
18+
never writes to it, so a save here would just be redundant work.
19+
required: false
20+
default: "true"
21+
22+
outputs:
23+
cache-hit:
24+
description: Whether the node_modules cache was restored (passed through from actions/cache/restore).
25+
value: ${{ steps.node-modules-cache.outputs.cache-hit }}
26+
27+
runs:
28+
using: composite
29+
steps:
30+
- name: Neutralize untrusted npm config
31+
shell: bash
32+
run: rm -f .npmrc
33+
34+
- name: Setup Node
35+
uses: actions/setup-node@48b55a011bda9f5d6aeb4c2d9c7362e8dae4041e # v6
36+
with:
37+
node-version-file: .nvmrc
38+
cache: npm
39+
40+
# actions/checkout wipes node_modules (git clean -ffdx) on every run regardless of the self-hosted
41+
# runner's own persistence, and npm ci always deletes+reinstalls node_modules by design -- so
42+
# neither the runner nor npm ci gives node_modules any real cross-run reuse on its own. This
43+
# explicit restore/save pair (via GitHub's own cache service, not local disk) fills that gap: an
44+
# exact manifest+lockfile match skips npm ci entirely. Keep fork/trusted keys separate even though
45+
# both run on ubuntu-latest: fork PRs get read-only cache tokens, so a fork-keyed entry can never
46+
# actually be written; trusted PRs keep their reusable cache without crossing trust boundaries.
47+
- name: Restore node_modules cache
48+
id: node-modules-cache
49+
uses: actions/cache/restore@55cc8345863c7cc4c66a329aec7e433d2d1c52a9 # v6.1.0
50+
with:
51+
path: |
52+
node_modules
53+
apps/loopover-ui/node_modules
54+
# hashFiles('.nvmrc') matters as much as the package manifests and lockfile: a Node bump
55+
# with no lockfile change would otherwise still hit and silently reuse node_modules whose
56+
# native addons (sharp, workerd, fsevents) were compiled against the OLD Node's ABI. The
57+
# manifests matter too because npm ci validates package.json/package-lock.json consistency
58+
# and runs lifecycle scripts from package.json; a package.json-only change must not skip it.
59+
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') }}
60+
61+
- name: Install dependencies (retry on transient failures)
62+
if: ${{ steps.node-modules-cache.outputs.cache-hit != 'true' }}
63+
shell: bash
64+
run: |
65+
for attempt in 1 2 3; do
66+
if npm ci --prefer-offline --no-audit --no-fund; then
67+
exit 0
68+
fi
69+
echo "::warning::npm ci failed (attempt ${attempt}/3); retrying in 10s"
70+
sleep 10
71+
done
72+
echo "::error::npm ci failed after 3 attempts"
73+
exit 1
74+
75+
# Placed immediately after install (not as an automatic post-job hook) so a cache is only ever
76+
# saved once npm ci has actually succeeded -- a job that fails here never reaches this step, so a
77+
# broken/partial node_modules can never get written to the cache for a future run to inherit.
78+
- name: Save node_modules cache
79+
if: ${{ inputs.save-cache == 'true' && steps.node-modules-cache.outputs.cache-hit != 'true' }}
80+
uses: actions/cache/save@55cc8345863c7cc4c66a329aec7e433d2d1c52a9 # v6.1.0
81+
with:
82+
path: |
83+
node_modules
84+
apps/loopover-ui/node_modules
85+
key: ${{ steps.node-modules-cache.outputs.cache-primary-key }}

‎.github/workflows/cache-cleanup.yml‎

Lines changed: 56 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,56 @@
1+
name: Clean up closed-PR caches
2+
3+
# Every PR-triggered run in ci.yml (node_modules, Turborepo, tsbuildinfo) saves its cache scoped to
4+
# that PR's refs/pull/N/merge ref -- GitHub's own cache isolation, separate from the key string itself
5+
# (two different PRs can share an identical key but still get two separate physical cache entries,
6+
# because they're scoped to different refs). Once a PR closes, nothing can ever restore a cache scoped
7+
# to its ref again -- and this repo auto-closes a red contributor PR one-shot (no reopen, no retry on
8+
# the same PR), so most of those entries are written once and then permanently unreachable, just
9+
# waiting on GitHub's passive 7-day-unused eviction. Confirmed live before adding this workflow: repo
10+
# cache usage sat at ~10.7GB of the 10GB budget, with closed-PR-scoped node_modules caches alone
11+
# accounting for the large majority of that -- crowding out the much smaller, much more useful
12+
# Turborepo/tsbuildinfo caches for LRU survival. This deletes a PR's own cache entries the moment it
13+
# closes (merged or not) instead of waiting on eviction.
14+
#
15+
# pull_request_target, not pull_request: this job only ever calls the GitHub API using
16+
# github.event.pull_request.number -- a trusted value GitHub itself populates, never anything read
17+
# from the PR's own code or checked out from it -- so the classic pull_request_target risk (running
18+
# fork-controlled code with base-repo credentials) doesn't apply here. It has to be
19+
# pull_request_target specifically because a fork-triggered plain `pull_request` run is always capped
20+
# to a read-only token regardless of the permissions block below, and deleting a cache needs
21+
# actions: write.
22+
23+
on:
24+
pull_request_target:
25+
types: [closed]
26+
27+
permissions:
28+
actions: write
29+
30+
concurrency:
31+
group: cache-cleanup-${{ github.event.pull_request.number }}
32+
cancel-in-progress: true
33+
34+
jobs:
35+
cleanup:
36+
name: Delete this PR's caches
37+
runs-on: ubuntu-latest
38+
timeout-minutes: 5
39+
steps:
40+
# Scoped by ref, not by key prefix (e.g. "npm-fork-") -- ANY cache entry scoped to this PR's ref
41+
# (node_modules, Turborepo, tsbuildinfo, trusted or fork) is equally unreachable dead weight the
42+
# moment the PR closes, regardless of which of ci.yml's cache families wrote it.
43+
- name: Delete caches scoped to this PR
44+
env:
45+
GH_TOKEN: ${{ secrets.GITHUB_TOKEN }}
46+
PR_REF: refs/pull/${{ github.event.pull_request.number }}/merge
47+
run: |
48+
ids=$(gh api "repos/${{ github.repository }}/actions/caches?per_page=100" --paginate --jq ".actions_caches[] | select(.ref == \"$PR_REF\") | .id")
49+
if [ -z "$ids" ]; then
50+
echo "No caches found for $PR_REF"
51+
exit 0
52+
fi
53+
for id in $ids; do
54+
echo "Deleting cache $id ($PR_REF)"
55+
gh api -X DELETE "repos/${{ github.repository }}/actions/caches/$id" || echo "::warning::Failed to delete cache $id (may already be gone)"
56+
done

‎.github/workflows/ci-duration-report.yml‎

Lines changed: 55 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,55 @@
1+
name: CI duration report
2+
3+
# Nothing else in this repo tracks whether ci.yml is trending slower or flakier over time -- catching
4+
# that currently requires a manual audit (this workflow exists because one just happened). Purely
5+
# additive and read-only: it doesn't touch ci.yml, doesn't gate anything, and can't fail a PR. Mirrors
6+
# audit.yml's shape (scheduled + workflow_dispatch, single job).
7+
8+
on:
9+
schedule:
10+
- cron: "0 15 * * 1" # Mondays 15:00 UTC, an hour ahead of audit.yml so they don't contend
11+
workflow_dispatch:
12+
13+
permissions:
14+
contents: read
15+
actions: read
16+
17+
concurrency:
18+
group: ci-duration-report
19+
cancel-in-progress: true
20+
21+
jobs:
22+
report:
23+
name: report
24+
runs-on: ubuntu-latest
25+
timeout-minutes: 5
26+
steps:
27+
- name: Checkout
28+
uses: actions/checkout@9c091bb21b7c1c1d1991bb908d89e4e9dddfe3e0 # v7
29+
- name: Setup Node
30+
uses: actions/setup-node@48b55a011bda9f5d6aeb4c2d9c7362e8dae4041e # v6
31+
with:
32+
node-version-file: .nvmrc
33+
- name: Pull ci.yml run stats
34+
env:
35+
GITHUB_TOKEN: ${{ github.token }}
36+
run: node scripts/ci-duration-report.mjs --days=7 --output=ci-duration-report.json
37+
- name: Write summary
38+
run: |
39+
node <<'NODE'
40+
const fs = require("node:fs");
41+
const report = JSON.parse(fs.readFileSync("ci-duration-report.json", "utf8"));
42+
const fmt = (s) => (s === null ? "n/a" : `${Math.round(s / 60)}m ${Math.round(s % 60)}s`);
43+
const pct = (r) => (r === null ? "n/a" : `${Math.round(r * 100)}%`);
44+
const lines = [
45+
`## CI duration report (trailing ${report.windowDays} days)`,
46+
"",
47+
"| Trigger | Runs | p50 | p95 | Failure rate | Cancelled (excluded) |",
48+
"|---|---|---|---|---|---|",
49+
`| push | ${report.push.count} | ${fmt(report.push.p50Seconds)} | ${fmt(report.push.p95Seconds)} | ${pct(report.push.failureRate)} | ${report.push.excludedCancelled} |`,
50+
`| pull_request | ${report.pullRequest.count} | ${fmt(report.pullRequest.p50Seconds)} | ${fmt(report.pullRequest.p95Seconds)} | ${pct(report.pullRequest.failureRate)} | ${report.pullRequest.excludedCancelled} |`,
51+
"",
52+
"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.",
53+
];
54+
fs.appendFileSync(process.env.GITHUB_STEP_SUMMARY, `${lines.join("\n")}\n`);
55+
NODE

0 commit comments

Comments
 (0)