From 040e72a8175db4a9622668642526a56cb2f79782 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Pierzcha=C5=82a?= Date: Tue, 28 Jul 2026 16:23:27 +0000 Subject: [PATCH 1/9] obs: quiet quality-delta PR comment + metric history on main Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com> --- .github/workflows/ci.yml | 74 +++++- .github/workflows/repo-health-history.yml | 79 ++++++ .github/workflows/size.yml | 19 +- package.json | 2 + scripts/coverage-changed/run.ts | 10 +- scripts/quality-delta/README.md | 115 +++++++++ scripts/quality-delta/append-history.ts | 55 +++++ scripts/quality-delta/fixtures.ts | 114 +++++++++ scripts/quality-delta/history.test.ts | 63 +++++ scripts/quality-delta/history.ts | 103 ++++++++ scripts/quality-delta/model.test.ts | 147 +++++++++++ scripts/quality-delta/model.ts | 283 ++++++++++++++++++++++ scripts/quality-delta/run.test.ts | 156 ++++++++++++ scripts/quality-delta/run.ts | 133 ++++++++++ scripts/quality-delta/thresholds.test.ts | 35 +++ scripts/quality-delta/thresholds.ts | 240 ++++++++++++++++++ scripts/vitest-slow-test-budgets.ts | 17 ++ scripts/vitest-slow-test-reporter.ts | 23 +- vitest.config.ts | 8 + 19 files changed, 1667 insertions(+), 9 deletions(-) create mode 100644 .github/workflows/repo-health-history.yml create mode 100644 scripts/quality-delta/README.md create mode 100644 scripts/quality-delta/append-history.ts create mode 100644 scripts/quality-delta/fixtures.ts create mode 100644 scripts/quality-delta/history.test.ts create mode 100644 scripts/quality-delta/history.ts create mode 100644 scripts/quality-delta/model.test.ts create mode 100644 scripts/quality-delta/model.ts create mode 100644 scripts/quality-delta/run.test.ts create mode 100644 scripts/quality-delta/run.ts create mode 100644 scripts/quality-delta/thresholds.test.ts create mode 100644 scripts/quality-delta/thresholds.ts diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 9ec149dca..4ccf37d91 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -240,6 +240,13 @@ jobs: name: Coverage runs-on: ubuntu-latest timeout-minutes: 30 + permissions: + contents: read + # The quality-delta comment (#1424) is posted from this job, and reads the Size workflow's + # size-report artifact for the same head sha. Fork PRs get a read-only token, so the posting + # step is skipped there and the delta degrades to job-summary output. + pull-requests: write + actions: read steps: - name: Checkout uses: actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd # v6.0.2 @@ -255,6 +262,9 @@ jobs: - name: Run coverage env: OUTPUT_ECONOMY_BASE: ${{ github.event_name == 'pull_request' && github.event.pull_request.base.sha || github.event.before }} + # The slow-test reporter persists this run's over-budget tests, which become one + # quality-delta row instead of a line only visible in this job's stderr. + AGENT_DEVICE_SLOW_TEST_REPORT: .tmp/slow-tests.json run: pnpm test:coverage # Reuses the lcov the coverage step just wrote (never runs coverage twice) @@ -265,7 +275,69 @@ jobs: if: always() && github.event_name == 'pull_request' env: AGENT_DEVICE_COVERAGE_WAIVER: ${{ contains(github.event.pull_request.labels.*.name, 'coverage-waiver') }} - run: pnpm check:coverage-changed --base "${{ github.event.pull_request.base.sha }}" + run: pnpm check:coverage-changed --base "${{ github.event.pull_request.base.sha }}" --json .tmp/changed-coverage.json + + # --- Quality-delta comment (#1424) ------------------------------------------------------ + # Non-gating reporting, ~20s total on top of this job: it reuses the lcov-derived numbers + # above, the size artifact the Size workflow already produced, and one repo-health snapshot + # (~1s, no network). Nothing below can fail the job. + + # Best effort by design: when the Size workflow has not published yet (or is skipped, e.g. + # a fork PR), the comment simply carries no size rows and says so in the job summary. + - name: Fetch the Size workflow's report for this head sha + if: always() && github.event_name == 'pull_request' + continue-on-error: true + env: + GH_TOKEN: ${{ github.token }} + HEAD_SHA: ${{ github.event.pull_request.head.sha }} + run: | + set -uo pipefail + id=$(gh api "repos/${GITHUB_REPOSITORY}/actions/artifacts?name=size-report-json&per_page=100" \ + --jq "[.artifacts[] | select(.workflow_run.head_sha == \"${HEAD_SHA}\") | .id] | first") + if [ -z "$id" ] || [ "$id" = "null" ]; then + echo "No size-report artifact for ${HEAD_SHA} yet; size rows will be omitted." + exit 0 + fi + mkdir -p .tmp + gh api "repos/${GITHUB_REPOSITORY}/actions/artifacts/${id}/zip" > .tmp/size-report.zip + unzip -o -q -d .tmp .tmp/size-report.zip + + # Read-only extraction of the history ref written by the Repo Health History workflow — no + # checkout, no write, absent until that workflow has run once. + - name: Fetch the main-branch metric history + if: always() && github.event_name == 'pull_request' + continue-on-error: true + run: | + set -uo pipefail + mkdir -p .tmp/history-ref + if git fetch --quiet --depth 1 origin +refs/heads/metrics/repo-health:refs/remotes/origin/metrics/repo-health; then + git archive origin/metrics/repo-health | tar -x -C .tmp/history-ref + else + echo "No metrics/repo-health ref yet; the comment will report no baseline." + fi + + - name: Render quality delta + if: always() && github.event_name == 'pull_request' + continue-on-error: true + run: | + pnpm repo-health --out .tmp/repo-health.json + pnpm quality-delta \ + --head .tmp/repo-health.json \ + --history-dir .tmp/history-ref \ + --base-sha "${{ github.event.pull_request.base.sha }}" \ + --changed-coverage .tmp/changed-coverage.json \ + --slow-test .tmp/slow-tests.json \ + --markdown .tmp/quality-delta.md + + # One sticky comment per PR, updated in place by the size-report marker machinery. Fork PRs + # cannot write comments with their read-only token, so they stop at the job summary above. + - name: Comment the quality delta on the PR + if: always() && github.event_name == 'pull_request' && github.event.pull_request.head.repo.full_name == github.repository + continue-on-error: true + env: + GITHUB_TOKEN: ${{ github.token }} + GITHUB_PR_NUMBER: ${{ github.event.pull_request.number }} + run: node scripts/size-report.mjs --post-comment .tmp/quality-delta.md typecheck: name: Typecheck diff --git a/.github/workflows/repo-health-history.yml b/.github/workflows/repo-health-history.yml new file mode 100644 index 000000000..2e400efd6 --- /dev/null +++ b/.github/workflows/repo-health-history.yml @@ -0,0 +1,79 @@ +name: Repo Health History + +# Appends one repo-health snapshot (#1423) per push to main to the JSONL metric history the +# quality-delta PR comment (#1424) reads its baseline from. See scripts/quality-delta/README.md for +# why the history lives on a dedicated orphan ref instead of an artifact chain or gh-pages. + +on: + push: + branches: + - main + +permissions: + # Only to append to the history ref. Nothing here touches the source tree. + contents: write + +# First half of the serialization contract: two merges landing seconds apart queue instead of +# racing, and cancel-in-progress is OFF because a cancelled run is a lost history entry. The second +# half is the compare-and-swap push below, which covers anything that slips past this group. +concurrency: + group: repo-health-history + cancel-in-progress: false + +jobs: + append-snapshot: + name: Append snapshot to metric history + runs-on: ubuntu-latest + timeout-minutes: 20 + steps: + - name: Checkout + uses: actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd # v6.0.2 + + - name: Setup toolchain + uses: ./.github/actions/setup-node-pnpm + + # The snapshot READS .tmp/size-report.json rather than measuring size itself, so main's + # entry needs a build first — this is the whole cost of the job (~5 min), and it runs only on + # main, never on the PR gate. + - name: Measure package size + run: | + pnpm build + pnpm size:markdown + + - name: Collect snapshot + run: pnpm repo-health --out .tmp/repo-health.json + + # Compare-and-swap with one retry: clone the history ref's tip, append, push. A push rejected + # because another run advanced the ref re-runs the whole function against the new tip. + # Appending is keyed by commit sha, so a retry can never double-write an entry. + - name: Append to the history ref + env: + GITHUB_TOKEN: ${{ github.token }} + HISTORY_REF: metrics/repo-health + run: | + set -uo pipefail + remote="https://x-access-token:${GITHUB_TOKEN}@github.com/${GITHUB_REPOSITORY}.git" + append_and_push() { + rm -rf .tmp/history-ref + if git ls-remote --exit-code --heads "$remote" "$HISTORY_REF" >/dev/null 2>&1; then + git clone --quiet --depth 1 --single-branch --branch "$HISTORY_REF" "$remote" .tmp/history-ref || return 1 + else + mkdir -p .tmp/history-ref + git -C .tmp/history-ref init --quiet --initial-branch "$HISTORY_REF" + fi + pnpm repo-health:history --snapshot .tmp/repo-health.json --history-dir .tmp/history-ref || return 1 + git -C .tmp/history-ref add history || return 1 + if git -C .tmp/history-ref diff --cached --quiet; then + echo "History already contains ${GITHUB_SHA}; nothing to push." + return 0 + fi + git -C .tmp/history-ref \ + -c user.name='github-actions[bot]' \ + -c user.email='41898282+github-actions[bot]@users.noreply.github.com' \ + commit --quiet -m "repo-health: ${GITHUB_SHA}" || return 1 + git -C .tmp/history-ref push --quiet "$remote" "HEAD:refs/heads/$HISTORY_REF" + } + append_and_push || { + echo "History ref moved under us; retrying once against the new tip." + append_and_push + } diff --git a/.github/workflows/size.yml b/.github/workflows/size.yml index 150d3e11d..a7f1438e5 100644 --- a/.github/workflows/size.yml +++ b/.github/workflows/size.yml @@ -13,7 +13,6 @@ on: permissions: contents: read - pull-requests: write concurrency: group: size-${{ github.workflow }}-${{ github.ref }} @@ -80,8 +79,16 @@ jobs: - name: Add job summary run: cat .tmp/size-report.md >> "$GITHUB_STEP_SUMMARY" - - name: Comment on PR - env: - GITHUB_TOKEN: ${{ github.token }} - GITHUB_PR_NUMBER: ${{ github.event.pull_request.number }} - run: pnpm size --post-comment .tmp/size-report.md + # The size table itself is job-summary output now: the sticky PR comment it used to post is + # the quality-delta comment (#1424), rendered from the repo-health snapshot in the CI + # workflow's Coverage job under the SAME marker, so a PR still carries exactly one comment. + # Size rows reach it through this artifact rather than a second base/head measurement. + - name: Upload size report JSON + uses: actions/upload-artifact@ea165f8d65b6e75b540449e92b4886f43607fa02 # v4.6.2 + with: + name: size-report-json + path: .tmp/size-report.json + # .tmp is a dotted directory, which upload-artifact skips by default. + include-hidden-files: true + retention-days: 7 + if-no-files-found: error diff --git a/package.json b/package.json index 6602bdde2..58bd49b09 100644 --- a/package.json +++ b/package.json @@ -133,6 +133,8 @@ "depgraph": "node --experimental-strip-types scripts/depgraph/build.ts", "depgraph:test": "node --experimental-strip-types --test scripts/depgraph/model.test.ts scripts/depgraph/affected.test.ts", "repo-health": "node --experimental-strip-types scripts/repo-health/run.ts", + "repo-health:history": "node --experimental-strip-types scripts/quality-delta/append-history.ts", + "quality-delta": "node --experimental-strip-types scripts/quality-delta/run.ts", "check:production-exports": "fallow dead-code --config fallow-production-exports.json --production --unused-exports --fail-on-issues", "check:bundle-owner-files": "node --experimental-strip-types scripts/check-bundle-owner-files.ts", "check:command-docs": "vitest run --project unit-core src/__tests__/command-doc-coverage.test.ts", diff --git a/scripts/coverage-changed/run.ts b/scripts/coverage-changed/run.ts index 10f159f5b..047245283 100644 --- a/scripts/coverage-changed/run.ts +++ b/scripts/coverage-changed/run.ts @@ -21,7 +21,7 @@ import { type ChangedCoverageResult, } from './model.ts'; -const USAGE = 'Usage: pnpm check:coverage-changed [--base ]\n'; +const USAGE = 'Usage: pnpm check:coverage-changed [--base ] [--json ]\n'; const LCOV_PATH = 'coverage/lcov.info'; function fmtPct(pct: number | null): string { @@ -58,6 +58,8 @@ function render(result: ChangedCoverageResult): string { export function run(argv: readonly string[], cwd?: string): number { const values = parseScriptArgs(argv, USAGE, { base: { type: 'string', default: 'origin/main' }, + // The quality-delta comment (#1424) reads these numbers rather than scraping the markdown. + json: { type: 'string' }, }); const base = values.base ?? 'origin/main'; // The `coverage-waiver` PR label sets this env var in CI (and locally). @@ -84,6 +86,12 @@ export function run(argv: readonly string[], cwd?: string): number { }, }); + if (typeof values.json === 'string') { + const jsonPath = path.resolve(values.json); + fs.mkdirSync(path.dirname(jsonPath), { recursive: true }); + fs.writeFileSync(jsonPath, `${JSON.stringify(result, null, 2)}\n`); + } + const report = render(result); process.stdout.write(report); const summaryPath = process.env.GITHUB_STEP_SUMMARY; diff --git a/scripts/quality-delta/README.md b/scripts/quality-delta/README.md new file mode 100644 index 000000000..8f59f8682 --- /dev/null +++ b/scripts/quality-delta/README.md @@ -0,0 +1,115 @@ +# Quality delta (quiet PR comment + main metric history) + +```sh +pnpm repo-health --out .tmp/repo-health.json # the snapshot this consumes (#1423) +pnpm quality-delta --head .tmp/repo-health.json \ + --history-dir .tmp/history-ref --base-sha \ + --changed-coverage .tmp/changed-coverage.json \ + --slow-test .tmp/slow-tests.json \ + --markdown .tmp/quality-delta.md # comment body + job summary + +pnpm repo-health:history --snapshot .tmp/repo-health.json --history-dir +``` + +Issue #1424 (Track C of #1412). Two halves: + +1. **History on main** — every push to main appends one repo-health snapshot to a JSONL store. +2. **One quiet PR comment** — the sticky comment `scripts/size-report.mjs` already owned, evolved to + show only metrics whose delta against that main baseline crosses a threshold. + +The maintainer requirement is the design constraint: **do not drown people in information.** A PR +where nothing crossed gets exactly one line — `No notable quality deltas.` — and no table at all. + +## What earns a row + +Every candidate metric, its threshold, and the gate that owns it live in one config block, +[`thresholds.ts`](./thresholds.ts). Nothing else hard-codes a number: `model.ts` compares and +renders, `run.ts` does I/O. Two kinds of metric: + +- **Delta metrics** (size bytes, R6/R9 ratchets, fallow suppressions, redundant graph edges) are + compared against the main baseline and shown when `|delta| >= minAbsDelta`. Ratchet-style metrics + can be `worseOnly`, so removing a suppression stays silent. +- **PR-local metrics** (changed-line and changed-branch coverage, changed lines excluded from + coverage, slow-test budget breaches) have no baseline by construction — they are defined against + the diff or the run — so they are scored against a bound. The coverage bound is the gate's own + threshold (`CHANGED_LINE_COVERAGE_THRESHOLD`) plus a margin, so approaching the gate costs one + quiet line instead of a surprise failure next commit. + +Every row links the file that owns the number and names the command that reproduces it +(`Gate · fix`). `thresholds.test.ts` enumerates the config and fails if a row's path does not exist +or its fix is not a real package script — a dead link in a PR comment is worse than no comment. + +Nothing here gates. The comment is a report; the gates it points at keep their own thresholds. + +## The line budget + +`MAX_COMMENT_LINES` (20, marker line included) is a hard cap. When more rows cross than fit, the +least severe are dropped from the comment — severity is how many multiples of its own threshold a +row crossed by — and replaced with `+N more, see the job summary`. The job summary always carries +every row plus the notes for inputs that were unavailable. + +## Refusals and honesty + +- **Schema versions are not diffed.** The snapshot's field names are the delta contract (#1423 + amendment). If the baseline's `schemaVersion` differs from head's, every baseline-derived row is + dropped and the comment says so; PR-local rows still show. To resume diffing, add a migration note + under "Schema migrations" below and bump the reader accordingly. +- **Stale inputs are labelled, not laundered.** repo-health flags a read artifact (coverage, size) + as `stale` when a source file is newer than it. Rows derived from a stale artifact render as + `JS gzip (stale artifact)` rather than silently claiming to describe the head commit. +- **Missing inputs cost rows, not runs.** No baseline, no size artifact, no coverage JSON — each + costs the rows it would have produced plus a job-summary note. Fork PRs (read-only token, no + secrets) therefore degrade to job-summary output: the comment step is skipped, never failed. + +## Where the history lives, and why + +The store is a **dedicated orphan ref, `metrics/repo-health`**, holding `history/YYYY-MM.jsonl` — +one compact snapshot per line, one file per month. + +- *Not an artifact chain*: artifacts expire (90 days, less with retention policy) and are keyed by + run, not commit, so "what did this metric look like at sha X" becomes an API crawl. +- *Not gh-pages*: that branch is the published website's deploy target; mixing a machine-read data + store into it couples this to the docs deploy and invites conflicts. +- *An orphan ref* keeps the data out of main's history (no source diff noise, no rebuild triggers), + is permanent, is fetchable shallowly (`--depth 1`), and is queryable by sha with plain git. + +Monthly shards keep a lookup a bounded read: a snapshot serializes to ~11 kB, so a busy month is +about 1 MB rather than an ever-growing single file. + +Query one commit: + +```sh +git fetch --depth 1 origin +refs/heads/metrics/repo-health:refs/remotes/origin/metrics/repo-health +git show origin/metrics/repo-health:history/2026-07.jsonl | grep "$(git rev-parse HEAD)" | jq .metrics +``` + +A PR whose exact base sha is not in the store yet falls back to the newest entry (a near baseline +states a real delta; no baseline states nothing), and the job summary records which was used. + +### Serializing concurrent writes + +Two merges landing seconds apart must not lose an entry, so the history job uses both mechanisms: + +1. A workflow-level `concurrency: { group: repo-health-history, cancel-in-progress: false }` — runs + queue instead of racing, and a queued run is never cancelled. +2. **Compare-and-swap with one retry** on the ref itself: clone the tip, append, push. A push + rejected because the ref moved re-runs the whole append against the new tip. Appending is keyed + by commit sha and is a no-op for a commit already stored, so a retry can never double-write. + +## Wiring + +| Where | What it does | +| --- | --- | +| `.github/workflows/repo-health-history.yml` | push-to-main: build → `pnpm size:markdown` → snapshot → append to `metrics/repo-health`. Main only; not on the PR gate. | +| `.github/workflows/ci.yml` (Coverage job) | PR: reuses the coverage run's lcov numbers and slow-test report, pulls the Size workflow's `size-report-json` artifact for the same head sha, snapshots, renders, and posts the sticky comment. | +| `.github/workflows/size.yml` | still measures base vs head into the job summary, and now publishes `size-report-json` instead of posting its own comment. | + +The comment reuses `scripts/size-report.mjs --post-comment` and its `COMMENT_MARKER` verbatim, so a +PR keeps exactly one marker-tracked comment that is updated in place. `run.test.ts` fails if the +marker in `run.ts` and the one in `size-report.mjs` ever drift apart. + +## Schema migrations + +None yet. `schemaVersion` 1 is the only version the reader has seen. When #1423's schema bumps, add +an entry here saying what moved and how a v1 baseline maps onto it, then teach `model.ts` the +mapping — until then it refuses to diff across versions on purpose. diff --git a/scripts/quality-delta/append-history.ts b/scripts/quality-delta/append-history.ts new file mode 100644 index 000000000..62465b909 --- /dev/null +++ b/scripts/quality-delta/append-history.ts @@ -0,0 +1,55 @@ +// Append one repo-health snapshot to the main-branch metric history (#1424): +// +// pnpm repo-health:history --snapshot .tmp/repo-health.json --history-dir +// +// The push-to-main workflow owns git (fetch the history ref, run this, commit, push with +// compare-and-swap and one retry); this command owns only the file fold, so the retry after a +// losing push is "re-read the ref and run it again". Appending the same commit twice is a no-op, +// which is what makes that retry safe. + +import fs from 'node:fs'; +import path from 'node:path'; +import { parseScriptArgs } from '../lib/cli-args.ts'; +import { runAsMain } from '../lib/run-as-main.ts'; +import type { RepoHealthSnapshot } from '../repo-health/model.ts'; +import { appendEntry, historyShardName, HISTORY_DIR } from './history.ts'; + +const USAGE = `Usage: pnpm repo-health:history --snapshot --history-dir + +Options: + --snapshot Repo-health snapshot JSON to append (pnpm repo-health --out). + --history-dir Working copy of the history ref; the shard lands in its ${HISTORY_DIR}/. +`; + +function run(argv: readonly string[]): number { + const values = parseScriptArgs(argv, USAGE, { + snapshot: { type: 'string', default: '.tmp/repo-health.json' }, + 'history-dir': { type: 'string' }, + }); + if (typeof values['history-dir'] !== 'string') throw new Error('--history-dir is required'); + + const snapshot = JSON.parse( + fs.readFileSync(path.resolve(values.snapshot ?? ''), 'utf8'), + ) as RepoHealthSnapshot; + const shard = path.join( + path.resolve(values['history-dir']), + HISTORY_DIR, + historyShardName(snapshot.provenance.generatedAt), + ); + fs.mkdirSync(path.dirname(shard), { recursive: true }); + const existing = fs.existsSync(shard) ? fs.readFileSync(shard, 'utf8') : ''; + const outcome = appendEntry(existing, snapshot); + if (!outcome.appended) { + process.stdout.write( + `repo-health:history: ${snapshot.provenance.commit.slice(0, 12)} is already in ${path.basename(shard)}; nothing to do.\n`, + ); + return 0; + } + fs.writeFileSync(shard, outcome.text); + process.stdout.write( + `repo-health:history: appended ${snapshot.provenance.commit.slice(0, 12)} to ${path.basename(shard)}.\n`, + ); + return 0; +} + +runAsMain(import.meta.url, 'repo-health:history', run); diff --git a/scripts/quality-delta/fixtures.ts b/scripts/quality-delta/fixtures.ts new file mode 100644 index 000000000..cbf9e4ee8 --- /dev/null +++ b/scripts/quality-delta/fixtures.ts @@ -0,0 +1,114 @@ +// Shared fixtures for the quality-delta tests: a repo-health snapshot with plausible numbers, and +// the two PR-local reports the comment can consume. Named exports in a sibling module rather than +// literals repeated per test (AGENTS.md "shared fixtures"). + +import type { ChangedCoverageResult } from '../coverage-changed/model.ts'; +import type { RepoHealthSnapshot } from '../repo-health/model.ts'; + +export type SnapshotOverrides = { + commit?: string; + schemaVersion?: number; + generatedAt?: string; + sizeStale?: boolean; + jsGzipBytes?: number; + npmTarballBytes?: number; + typeInversionTotal?: number; + typeCycleBaseline?: number; + suppressions?: number; + redundantEdges?: number; +}; + +/** A snapshot whose every metric sits on the baseline values, so a test moves exactly one number. */ +export function snapshotFixture(overrides: SnapshotOverrides = {}): RepoHealthSnapshot { + const commit = overrides.commit ?? 'a'.repeat(40); + const generatedAt = overrides.generatedAt ?? '2026-07-20T10:00:00.000Z'; + const schemaVersion = overrides.schemaVersion ?? 1; + return { + schemaVersion, + provenance: { + schemaVersion, + commit, + ref: 'main', + node: 'v22.14.0', + generatedAt, + tool: { depgraph: 'abc123456789', layering: 'def123456789' }, + inputs: { + sourceFiles: 900, + lockfile: { path: 'pnpm-lock.yaml', sha256: '0123456789ab' }, + coverageSummary: { + path: 'coverage/coverage-summary.json', + sha256: 'cccccccccccc', + stale: false, + }, + sizeReport: { + path: '.tmp/size-report.json', + sha256: 'ssssssssssss', + stale: overrides.sizeStale ?? false, + }, + }, + }, + metrics: { + depgraph: { + files: 900, + edges: 4700, + valueCycles: 0, + typeCycles: 7, + dynamicCycles: 1, + redundantEdges: overrides.redundantEdges ?? 1300, + backEdges: 0, + typeInversionEdges: overrides.typeInversionTotal ?? 7, + }, + layering: { + typeInversionBaseline: { 'commands -> client': overrides.typeInversionTotal ?? 7 }, + typeInversionTotal: overrides.typeInversionTotal ?? 7, + typeCycleBaseline: overrides.typeCycleBaseline ?? 102, + }, + coverage: { + available: true, + lines: { total: 1000, covered: 830, pct: 83 }, + statements: { total: 1000, covered: 800, pct: 80 }, + functions: { total: 200, covered: 170, pct: 85 }, + branches: { total: 400, covered: 300, pct: 75 }, + }, + size: { + available: true, + jsRawBytes: 3_000_000, + jsGzipBytes: overrides.jsGzipBytes ?? 800_000, + npmTarballBytes: overrides.npmTarballBytes ?? 1_200_000, + npmUnpackedBytes: 4_000_000, + }, + fallow: { + deadCodeFindings: 0, + healthFindings: 161, + suppressions: { + ignoredExports: overrides.suppressions ?? 19, + ignoredDependencies: 0, + total: overrides.suppressions ?? 19, + }, + }, + slowTest: { unitBudgetMs: 2500, integrationBudgetMs: 15000, enforceFactor: 2 }, + bench: { cases: 19, topics: 10 }, + skillgym: { cases: 5 }, + components: { byZone: [] }, + mainSequence: { concreteHighFanIn: [] }, + }, + consistency: { r6: { ok: true, expected: {}, actual: {} } }, + }; +} + +export function changedCoverageFixture( + overrides: Partial = {}, +): ChangedCoverageResult { + return { + threshold: 70, + coveredLines: 95, + totalLines: 100, + pct: 95, + waived: false, + passed: true, + offenders: [], + branch: { covered: 18, total: 20, pct: 90 }, + excluded: { files: [], totalLines: 0 }, + ...overrides, + }; +} diff --git a/scripts/quality-delta/history.test.ts b/scripts/quality-delta/history.test.ts new file mode 100644 index 000000000..a4fd8c616 --- /dev/null +++ b/scripts/quality-delta/history.test.ts @@ -0,0 +1,63 @@ +import { expect, test } from 'vitest'; +import { snapshotFixture } from './fixtures.ts'; +import { + appendEntry, + findBaseline, + historyShardName, + parseHistory, + serializeEntry, +} from './history.ts'; + +test('a snapshot lands in the shard for the month it was generated in (UTC)', () => { + expect(historyShardName('2026-07-20T10:00:00.000Z')).toBe('2026-07.jsonl'); + // Late-UTC December must not roll into the next year via local time. + expect(historyShardName('2026-12-31T23:59:59.000Z')).toBe('2026-12.jsonl'); +}); + +test('appending is keyed by commit, so a rerun of the same main build is a no-op', () => { + const snapshot = snapshotFixture({ commit: 'c'.repeat(40) }); + const first = appendEntry('', snapshot); + expect(first.appended).toBe(true); + expect(first.text.endsWith('\n')).toBe(true); + + const second = appendEntry(first.text, snapshot); + expect(second.appended).toBe(false); + expect(second.text).toBe(first.text); + + const other = appendEntry(first.text, snapshotFixture({ commit: 'd'.repeat(40) })); + expect(other.appended).toBe(true); + expect(parseHistory(other.text).entries).toHaveLength(2); +}); + +test('appending repairs a shard whose last line lost its newline', () => { + const line = serializeEntry(snapshotFixture({ commit: 'e'.repeat(40) })); + const appended = appendEntry(line, snapshotFixture({ commit: 'f'.repeat(40) })); + expect(parseHistory(appended.text).entries).toHaveLength(2); +}); + +test('malformed lines are counted, never silently dropped', () => { + const text = ['{"not":"a snapshot"}', 'not json at all', serializeEntry(snapshotFixture())].join( + '\n', + ); + const parsed = parseHistory(text); + expect(parsed.entries).toHaveLength(1); + expect(parsed.malformed).toBe(2); +}); + +test('a baseline lookup prefers the requested commit and falls back to the newest entry', () => { + const older = snapshotFixture({ + commit: '1'.repeat(40), + generatedAt: '2026-07-01T00:00:00.000Z', + }); + const newer = snapshotFixture({ + commit: '2'.repeat(40), + generatedAt: '2026-07-09T00:00:00.000Z', + }); + const entries = [newer, older]; + + expect(findBaseline(entries, '1'.repeat(40))).toEqual({ snapshot: older, exact: true }); + // A short sha from `git rev-parse --short` still resolves. + expect(findBaseline(entries, '1'.repeat(7))?.exact).toBe(true); + expect(findBaseline(entries, 'deadbee')).toEqual({ snapshot: newer, exact: false }); + expect(findBaseline([], 'deadbee')).toBe(null); +}); diff --git a/scripts/quality-delta/history.ts b/scripts/quality-delta/history.ts new file mode 100644 index 000000000..25acc5775 --- /dev/null +++ b/scripts/quality-delta/history.ts @@ -0,0 +1,103 @@ +// The main-branch metric history: an append-only JSONL store of repo-health snapshots (#1424). +// +// One line per push to main, one file per month (`history/YYYY-MM.jsonl`), so looking a commit up +// is a bounded read of the month it landed in rather than a growing whole-file parse (a snapshot +// serializes to ~11 kB, so a month of merges is ~1 MB). Lines are append-only and never rewritten, +// which is what makes the store queryable by commit sha and safe to fetch shallowly. +// +// This module is pure: it parses, keys, and folds text. The workflow owns git and run.ts owns fs. + +import type { RepoHealthSnapshot } from '../repo-health/model.ts'; + +/** Directory inside the history ref holding the monthly shards. */ +export const HISTORY_DIR = 'history'; + +/** The shard a snapshot belongs to, keyed by the month it was generated in (UTC). */ +export function historyShardName(generatedAt: string): string { + const at = new Date(generatedAt); + const month = `${at.getUTCMonth() + 1}`.padStart(2, '0'); + return `${at.getUTCFullYear()}-${month}.jsonl`; +} + +/** One snapshot as one line — compact, so a shard stays greppable by sha. */ +export function serializeEntry(snapshot: RepoHealthSnapshot): string { + return JSON.stringify(snapshot); +} + +export type ParsedHistory = { + entries: readonly RepoHealthSnapshot[]; + /** Lines that were not parseable snapshots. Reported, never silently dropped. */ + malformed: number; +}; + +export function parseHistory(text: string): ParsedHistory { + const entries: RepoHealthSnapshot[] = []; + let malformed = 0; + for (const line of text.split('\n')) { + if (line.trim() === '') continue; + try { + const parsed = JSON.parse(line) as RepoHealthSnapshot; + if ( + typeof parsed.schemaVersion === 'number' && + typeof parsed.provenance?.commit === 'string' + ) { + entries.push(parsed); + } else { + malformed += 1; + } + } catch { + malformed += 1; + } + } + return { entries, malformed }; +} + +export type AppendOutcome = { + text: string; + /** False when an entry for the same commit is already stored — reruns of main are idempotent. */ + appended: boolean; +}; + +/** + * Append one snapshot to a shard's text. A commit already present wins: the history is keyed by + * commit sha, so a re-run of a main build must not create a second, differently-timestamped entry + * for the same tree. + */ +export function appendEntry(existingText: string, snapshot: RepoHealthSnapshot): AppendOutcome { + const { entries } = parseHistory(existingText); + if (entries.some((entry) => entry.provenance.commit === snapshot.provenance.commit)) { + return { text: existingText, appended: false }; + } + const prefix = + existingText === '' || existingText.endsWith('\n') ? existingText : `${existingText}\n`; + return { text: `${prefix}${serializeEntry(snapshot)}\n`, appended: true }; +} + +export type BaselineLookup = { + snapshot: RepoHealthSnapshot; + /** True when the store held the requested commit; false when this is the newest entry instead. */ + exact: boolean; +}; + +/** + * The baseline for a PR: the entry for `commit` when main has one, otherwise the newest entry. + * The fallback exists because a PR's base commit may not have finished its history job yet — a + * near baseline states a real delta, while no baseline states nothing at all. + */ +export function findBaseline( + entries: readonly RepoHealthSnapshot[], + commit: string | null, +): BaselineLookup | null { + if (entries.length === 0) return null; + if (commit !== null && commit !== '') { + const exact = entries.find( + (entry) => entry.provenance.commit === commit || entry.provenance.commit.startsWith(commit), + ); + if (exact) return { snapshot: exact, exact: true }; + } + const newest = [...entries].sort((left, right) => + left.provenance.generatedAt.localeCompare(right.provenance.generatedAt), + ); + const last = newest.at(-1); + return last === undefined ? null : { snapshot: last, exact: false }; +} diff --git a/scripts/quality-delta/model.test.ts b/scripts/quality-delta/model.test.ts new file mode 100644 index 000000000..934465f96 --- /dev/null +++ b/scripts/quality-delta/model.test.ts @@ -0,0 +1,147 @@ +import { expect, test } from 'vitest'; +import { changedCoverageFixture, snapshotFixture } from './fixtures.ts'; +import { computeQualityDelta, NO_DELTAS_LINE, renderComment, renderJobSummary } from './model.ts'; +import { MAX_COMMENT_LINES, type DeltaSources } from './thresholds.ts'; + +const CONTEXT = { linkBase: 'https://github.com/o/r/blob/head', summaryUrl: 'https://run/1' }; + +function sources(overrides: Partial = {}): DeltaSources { + return { + head: snapshotFixture({ commit: 'b'.repeat(40) }), + base: snapshotFixture(), + changedCoverage: null, + slowTest: null, + ...overrides, + }; +} + +function commentOf(overrides: Partial = {}): string { + return renderComment(computeQualityDelta(sources(overrides)), CONTEXT); +} + +test('an unchanged tree collapses the whole comment to one line', () => { + expect(commentOf()).toBe(`${NO_DELTAS_LINE}\n`); +}); + +test('a size move under its threshold stays silent', () => { + const head = snapshotFixture({ jsGzipBytes: 800_000 + 4_000 }); + expect(commentOf({ head })).toBe(`${NO_DELTAS_LINE}\n`); +}); + +test('a size move over its threshold earns one row naming its gate and fix command', () => { + const head = snapshotFixture({ jsGzipBytes: 800_000 + 20_000 }); + const comment = commentOf({ head }); + expect(comment).toContain('| JS gzip | 800.0 kB | 820.0 kB | +20.0 kB |'); + expect(comment).toContain( + '[Size / Bundle Size](https://github.com/o/r/blob/head/.github/workflows/size.yml)', + ); + expect(comment).toContain('`pnpm size`'); + // Quiet by default: the other metrics did not move, so they contribute nothing. + expect(comment).not.toContain('npm tarball'); +}); + +test('a ratchet improvement is shown, but a removed suppression is not', () => { + const better = commentOf({ head: snapshotFixture({ typeInversionTotal: 5 }) }); + expect(better).toContain('| R6 type inversions | 7 | 5 | -2 |'); + + // fallowSuppressions is worseOnly: shrinking it is good news the Fallow job already reports. + expect(commentOf({ head: snapshotFixture({ suppressions: 17 }) })).toBe(`${NO_DELTAS_LINE}\n`); + expect(commentOf({ head: snapshotFixture({ suppressions: 21 }) })).toContain( + '| Fallow suppressions | 19 | 21 | +2 |', + ); +}); + +test('a metric read from a stale artifact is marked rather than silently attributed to head', () => { + const head = snapshotFixture({ jsGzipBytes: 900_000, sizeStale: true }); + expect(commentOf({ head })).toContain('| JS gzip (stale artifact) |'); +}); + +test('PR-local coverage rows are scored against a bound, not a baseline', () => { + // 95% is comfortably above the gate threshold + margin, so it earns no row. + expect(commentOf({ changedCoverage: changedCoverageFixture() })).toBe(`${NO_DELTAS_LINE}\n`); + + const weak = changedCoverageFixture({ pct: 61.5, coveredLines: 61, passed: false }); + const comment = commentOf({ changedCoverage: weak }); + expect(comment).toContain('| Changed-line coverage | — | 61.50% | < 80.00% |'); + expect(comment).toContain('`pnpm check:coverage-changed`'); +}); + +test('slow-test breaches show as a row with the budget file as the fix target', () => { + const slowTest = { + offenders: [{ key: 'src/a.test.ts :: waits', durationMs: 6000, budgetMs: 2500, enforce: true }], + }; + const comment = commentOf({ slowTest }); + expect(comment).toContain('| Slow-test budget breaches | — | 1 | > 0 |'); + expect(comment).toContain('scripts/vitest-slow-test-budgets.ts'); +}); + +test('differing schemaVersions are refused instead of diffed, PR-local rows survive', () => { + const report = computeQualityDelta( + sources({ + base: snapshotFixture({ schemaVersion: 1 }), + head: snapshotFixture({ schemaVersion: 2, jsGzipBytes: 900_000 }), + changedCoverage: changedCoverageFixture({ pct: 40 }), + }), + ); + expect(report.schemaMismatch).toEqual({ base: 1, head: 2 }); + const comment = renderComment(report, CONTEXT); + expect(comment).toContain('Refusing to diff snapshot schemaVersion 1 against 2'); + expect(comment).toContain('migration note'); + expect(comment).not.toContain('JS gzip'); + expect(comment).toContain('Changed-line coverage'); +}); + +test('without a baseline the comment says so in one line and notes it in the summary', () => { + const report = computeQualityDelta(sources({ base: null })); + const comment = renderComment(report, CONTEXT); + expect(comment.trim().split('\n')).toHaveLength(1); + expect(comment).toContain('no main baseline'); + expect(renderJobSummary(report, CONTEXT)).toContain('The history job appends one snapshot'); +}); + +test('the comment never exceeds the line budget and summarizes the overflow', () => { + const head = snapshotFixture({ + jsGzipBytes: 900_000, + npmTarballBytes: 1_400_000, + typeInversionTotal: 20, + typeCycleBaseline: 150, + suppressions: 40, + redundantEdges: 1900, + }); + const report = computeQualityDelta( + sources({ + head, + changedCoverage: changedCoverageFixture({ + pct: 10, + branch: { covered: 1, total: 20, pct: 5 }, + excluded: { files: [], totalLines: 80 }, + }), + slowTest: { offenders: [{ key: 'a', durationMs: 9000, budgetMs: 2500, enforce: true }] }, + }), + ); + // Every configured metric crossed, so this is the worst case the cap has to survive. + expect(report.rows).toHaveLength(10); + + const comment = renderComment(report, { ...CONTEXT, maxLines: 10 }); + const lines = comment.trimEnd().split('\n'); + // maxLines counts the marker line run.ts prepends, which renderComment does not emit. + expect(lines.length).toBe(9); + expect(lines.at(-1)).toBe('+6 more, see the [job summary](https://run/1).'); + // The dropped rows are the least severe ones, and the summary still carries all ten. + const summaryRows = renderJobSummary(report, CONTEXT) + .split('\n') + .filter((line) => line.startsWith('| ')); + expect(summaryRows).toHaveLength(12); +}); + +test('the default cap leaves room for every configured metric plus its header', () => { + const report = computeQualityDelta( + sources({ + head: snapshotFixture({ jsGzipBytes: 900_000 }), + changedCoverage: changedCoverageFixture({ pct: 10 }), + }), + ); + expect(renderComment(report, CONTEXT).trimEnd().split('\n').length).toBeLessThanOrEqual( + MAX_COMMENT_LINES - 1, + ); +}); diff --git a/scripts/quality-delta/model.ts b/scripts/quality-delta/model.ts new file mode 100644 index 000000000..cbfb20a75 --- /dev/null +++ b/scripts/quality-delta/model.ts @@ -0,0 +1,283 @@ +// Pure model for the quiet quality-delta PR comment (#1424, Track C of #1412). +// +// It takes a head repo-health snapshot (#1423), the main-branch baseline for it (read from the +// JSONL history in history.ts), and the two PR-local reports that have no baseline by construction +// (changed-line coverage #1418, slow-test offenders), and answers one question per metric: is this +// worth a line? Only metrics that cross their own threshold in thresholds.ts become rows. When +// nothing crosses, the comment is exactly one line — the maintainer requirement is "do not drown +// people in information", so silence is the default output, not a fallback. +// +// Two refusals are deliberate: +// - schemaVersion mismatch between baseline and head: the snapshot's field names are the delta +// contract, so a version bump means the numbers may not mean the same thing. The comment says +// so and drops every baseline-derived row instead of diffing across versions. +// - a head metric read from a STALE artifact (repo-health provenance flags coverage/size when a +// source file is newer than the artifact): the row is kept but marked, never silently paired +// with the head commit. +// +// All I/O — reading files, resolving links, posting the comment — belongs to run.ts. + +import type { RepoHealthSnapshot } from '../repo-health/model.ts'; +import { + MAX_COMMENT_LINES, + QUALITY_DELTA_METRICS, + type DeltaSources, + type MetricFormat, + type MetricOwner, + type MetricSpec, +} from './thresholds.ts'; + +export const COMMENT_HEADING = 'Quality delta'; +export const NO_DELTAS_LINE = 'No notable quality deltas.'; + +export type QualityDeltaRow = { + key: string; + label: string; + format: MetricFormat; + owner: MetricOwner; + /** null for PR-local metrics, which are scored against a bound rather than a baseline. */ + base: number | null; + head: number; + delta: number | null; + /** The bound or |delta| threshold the value crossed, for the "why is this here" column. */ + crossed: string; + /** How many multiples of its threshold the row crossed by — the overflow drop order. */ + severity: number; + /** The head value came from a read artifact that is older than the tree (repo-health provenance). */ + stale: boolean; +}; + +export type QualityDeltaReport = { + rows: readonly QualityDeltaRow[]; + head: { commit: string; schemaVersion: number }; + base: { commit: string; schemaVersion: number } | null; + /** Baseline and head disagree on schemaVersion, so baseline-derived rows are refused. */ + schemaMismatch: { base: number; head: number } | null; + /** Quiet degradations (missing inputs, absent baseline). Job summary only. */ + notes: readonly string[]; +}; + +function formatValue(value: number, format: MetricFormat): string { + if (format === 'pct') return `${value.toFixed(2)}%`; + if (format === 'count') return String(value); + const absolute = Math.abs(value); + if (absolute < 1000) return `${value} B`; + if (absolute < 1_000_000) return `${(value / 1000).toFixed(1)} kB`; + return `${(value / 1_000_000).toFixed(2)} MB`; +} + +function formatSigned(value: number, format: MetricFormat): string { + const sign = value > 0 ? '+' : value < 0 ? '-' : ''; + return `${sign}${formatValue(Math.abs(value), format)}`; +} + +function staleInputFlag(snapshot: RepoHealthSnapshot, spec: MetricSpec): boolean { + const inputs = snapshot.provenance.inputs; + if (spec.staleInput === 'sizeReport') return inputs.sizeReport?.stale === true; + if (spec.staleInput === 'coverageSummary') return inputs.coverageSummary?.stale === true; + return false; +} + +function deltaRow(spec: MetricSpec, sources: DeltaSources): QualityDeltaRow | null { + if (spec.kind !== 'delta' || sources.base === null) return null; + const base = spec.read(sources.base); + const head = spec.read(sources.head); + if (base === null || head === null) return null; + const delta = head - base; + const worse = spec.direction === 'lower-is-better' ? delta > 0 : delta < 0; + if (spec.worseOnly === true && !worse) return null; + if (Math.abs(delta) < spec.minAbsDelta) return null; + return { + key: spec.key, + label: spec.label, + format: spec.format, + owner: spec.owner, + base, + head, + delta, + crossed: formatSigned(delta, spec.format), + severity: Math.abs(delta) / spec.minAbsDelta, + stale: staleInputFlag(sources.head, spec), + }; +} + +function absoluteRow(spec: MetricSpec, sources: DeltaSources): QualityDeltaRow | null { + if (spec.kind !== 'absolute') return null; + const head = spec.read(sources); + if (head === null) return null; + const higherIsBetter = spec.direction === 'higher-is-better'; + const crossed = higherIsBetter ? head < spec.bound : head > spec.bound; + if (!crossed) return null; + const comparator = higherIsBetter ? '<' : '>'; + const distance = Math.abs(head - spec.bound); + return { + key: spec.key, + label: spec.label, + format: spec.format, + owner: spec.owner, + base: null, + head, + delta: null, + crossed: `${comparator} ${formatValue(spec.bound, spec.format)}`, + severity: distance / Math.max(spec.bound, 1), + stale: staleInputFlag(sources.head, spec), + }; +} + +function missingInputNotes(sources: DeltaSources): string[] { + const notes: string[] = []; + if (sources.base === null) { + notes.push( + 'No main-branch baseline for this PR yet — baseline-derived rows are omitted. ' + + 'The history job appends one snapshot per push to main (see scripts/quality-delta/README.md).', + ); + } + if (sources.head.metrics.size.available !== true) { + notes.push( + 'Size metrics unavailable in the head snapshot (`pnpm size:markdown` produces them).', + ); + } + if (sources.changedCoverage === null) { + notes.push('Changed-line coverage report unavailable (`pnpm check:coverage-changed --json`).'); + } + if (sources.slowTest === null) { + notes.push( + 'Slow-test report unavailable (the vitest reporter writes it during the coverage run).', + ); + } + return notes; +} + +export function computeQualityDelta(sources: DeltaSources): QualityDeltaReport { + const schemaMismatch = + sources.base !== null && sources.base.schemaVersion !== sources.head.schemaVersion + ? { base: sources.base.schemaVersion, head: sources.head.schemaVersion } + : null; + // Refusing to diff across schema versions is the point, not an edge case: the field names ARE + // the contract (#1423 amendment), so a bump needs a migration note before deltas mean anything. + const comparable: DeltaSources = schemaMismatch === null ? sources : { ...sources, base: null }; + const rows = QUALITY_DELTA_METRICS.flatMap((spec) => { + const row = deltaRow(spec, comparable) ?? absoluteRow(spec, comparable); + return row === null ? [] : [row]; + }).sort((left, right) => right.severity - left.severity); + + return { + rows, + head: { commit: sources.head.provenance.commit, schemaVersion: sources.head.schemaVersion }, + base: + sources.base === null + ? null + : { commit: sources.base.provenance.commit, schemaVersion: sources.base.schemaVersion }, + schemaMismatch, + notes: schemaMismatch === null ? missingInputNotes(sources) : missingInputNotes(comparable), + }; +} + +// ---- Rendering -------------------------------------------------------------------------------- + +export type RenderContext = { + /** `https://host/owner/repo/blob/`; when absent (local runs) paths render as plain code. */ + linkBase: string | null; + /** Where overflow rows and every note live. */ + summaryUrl: string | null; + /** Overridable only so the test can prove the cap, not a knob for callers. */ + maxLines?: number; +}; + +function ownerCell(owner: MetricOwner, linkBase: string | null): string { + const target = + linkBase === null ? `\`${owner.path}\`` : `[${owner.gate}](${linkBase}/${owner.path})`; + return `${target} · \`${owner.fix}\``; +} + +function rowLine(row: QualityDeltaRow, linkBase: string | null): string { + const label = row.stale ? `${row.label} (stale artifact)` : row.label; + const base = row.base === null ? '—' : formatValue(row.base, row.format); + return `| ${label} | ${base} | ${formatValue(row.head, row.format)} | ${row.crossed} | ${ownerCell(row.owner, linkBase)} |`; +} + +function shortSha(commit: string): string { + return commit.slice(0, 7); +} + +function headingLine(report: QualityDeltaReport): string { + const against = + report.base === null ? 'no main baseline' : `main@\`${shortSha(report.base.commit)}\``; + return `### ${COMMENT_HEADING} vs ${against}`; +} + +function summaryLink(summaryUrl: string | null): string { + return summaryUrl === null ? 'the job summary' : `the [job summary](${summaryUrl})`; +} + +function migrationRefusalLine(report: QualityDeltaReport): string | null { + const mismatch = report.schemaMismatch; + if (mismatch === null) return null; + return ( + `Refusing to diff snapshot schemaVersion ${mismatch.base} against ${mismatch.head}: ` + + 'write a migration note in scripts/quality-delta/README.md, then rerun.' + ); +} + +/** + * The sticky-comment body, without the marker line the caller owns (scripts/size-report.mjs + * `--post-comment` machinery keeps it a single in-place comment). Never longer than + * `MAX_COMMENT_LINES` minus that marker: overflow rows are dropped lowest-severity-first and + * summarized, because a 60-row comment is the failure mode this issue exists to prevent. + */ +export function renderComment(report: QualityDeltaReport, context: RenderContext): string { + const maxLines = (context.maxLines ?? MAX_COMMENT_LINES) - 1; + const refusal = migrationRefusalLine(report); + if (report.rows.length === 0) { + if (refusal !== null) return `${refusal}\n`; + if (report.base === null) { + return `${COMMENT_HEADING}: no main baseline for \`${shortSha(report.head.commit)}\` yet — see ${summaryLink(context.summaryUrl)}.\n`; + } + return `${NO_DELTAS_LINE}\n`; + } + + const header = [ + headingLine(report), + ...(refusal === null ? [] : [refusal]), + '', + '| Metric | Base | Head | Δ / bound | Gate · fix |', + '| --- | --- | --- | --- | --- |', + ]; + const budget = maxLines - header.length; + const overflows = report.rows.length > budget; + const shown = overflows ? report.rows.slice(0, Math.max(budget - 1, 0)) : report.rows; + const footer = overflows + ? [`+${report.rows.length - shown.length} more, see ${summaryLink(context.summaryUrl)}.`] + : []; + + return `${[...header, ...shown.map((row) => rowLine(row, context.linkBase)), ...footer].join('\n')}\n`; +} + +/** The unabridged report: every row, every note, and the provenance the rows were derived from. */ +export function renderJobSummary(report: QualityDeltaReport, context: RenderContext): string { + const lines = [ + `## ${COMMENT_HEADING}`, + '', + `Head snapshot \`${shortSha(report.head.commit)}\` (schemaVersion ${report.head.schemaVersion}); ` + + (report.base === null + ? 'no main baseline.' + : `baseline \`${shortSha(report.base.commit)}\` (schemaVersion ${report.base.schemaVersion}).`), + '', + ]; + const refusal = migrationRefusalLine(report); + if (refusal !== null) lines.push(refusal, ''); + if (report.rows.length === 0) { + lines.push(NO_DELTAS_LINE, ''); + } else { + lines.push( + '| Metric | Base | Head | Δ / bound | Gate · fix |', + '| --- | --- | --- | --- | --- |', + ); + for (const row of report.rows) lines.push(rowLine(row, context.linkBase)); + lines.push(''); + } + if (report.notes.length > 0) { + lines.push('Notes:', ...report.notes.map((note) => `- ${note}`), ''); + } + return `${lines.join('\n')}\n`; +} diff --git a/scripts/quality-delta/run.test.ts b/scripts/quality-delta/run.test.ts new file mode 100644 index 000000000..772109fca --- /dev/null +++ b/scripts/quality-delta/run.test.ts @@ -0,0 +1,156 @@ +// End-to-end coverage of the two quality-delta CLIs. Both are spawned as subprocesses (they are +// exercised through argument handling and the files they write), which is why this file lives in +// the `subprocess-stub` vitest project rather than `unit-core` — see #1412 coordination rule 4. + +import { spawnSync } from 'node:child_process'; +import { existsSync, mkdirSync, mkdtempSync, readFileSync, writeFileSync } from 'node:fs'; +import { tmpdir } from 'node:os'; +import { join } from 'node:path'; +import { expect, test } from 'vitest'; +import { changedCoverageFixture, snapshotFixture } from './fixtures.ts'; +import { HISTORY_DIR } from './history.ts'; +import { STICKY_COMMENT_MARKER } from './run.ts'; + +function runScript( + script: string, + args: readonly string[], +): { status: number | null; stdout: string; stderr: string } { + const result = spawnSync( + process.execPath, + ['--experimental-strip-types', `scripts/quality-delta/${script}`, ...args], + { encoding: 'utf8', env: { ...process.env, GITHUB_STEP_SUMMARY: '' } }, + ); + return { status: result.status, stdout: result.stdout ?? '', stderr: result.stderr ?? '' }; +} + +function workdir(): string { + return mkdtempSync(join(tmpdir(), 'quality-delta-')); +} + +function writeJson(dir: string, name: string, value: unknown): string { + const target = join(dir, name); + writeFileSync(target, `${JSON.stringify(value, null, 2)}\n`); + return target; +} + +test('the sticky-comment marker stays the one scripts/size-report.mjs posts', () => { + // The comment IS the evolved size-report comment: a drifting marker would silently produce a + // second comment per PR instead of updating one in place. + const sizeReport = readFileSync('scripts/size-report.mjs', 'utf8'); + expect(sizeReport).toContain(`const COMMENT_MARKER = '${STICKY_COMMENT_MARKER}'`); +}); + +test('the CLI writes a marker-prefixed comment and prints the full summary', () => { + const dir = workdir(); + const head = writeJson( + dir, + 'head.json', + snapshotFixture({ commit: 'b'.repeat(40), jsGzipBytes: 900_000 }), + ); + const base = writeJson(dir, 'base.json', snapshotFixture()); + const coverage = writeJson( + dir, + 'coverage.json', + changedCoverageFixture({ pct: 55, passed: false }), + ); + const markdown = join(dir, 'comment.md'); + + const { status, stdout } = runScript('run.ts', [ + '--head', + head, + '--baseline', + base, + '--changed-coverage', + coverage, + '--markdown', + markdown, + ]); + expect(status, stdout).toBe(0); + + const comment = readFileSync(markdown, 'utf8'); + expect(comment.startsWith(`${STICKY_COMMENT_MARKER}\n`)).toBe(true); + expect(comment).toContain('JS gzip'); + expect(comment).toContain('Changed-line coverage'); + expect(comment.trimEnd().split('\n').length).toBeLessThanOrEqual(20); + expect(stdout).toContain('## Quality delta'); +}); + +test('the CLI resolves its baseline out of the JSONL history by base sha', () => { + const dir = workdir(); + const historyDir = join(dir, 'history-ref'); + mkdirSync(join(historyDir, HISTORY_DIR), { recursive: true }); + const baseSha = '3'.repeat(40); + writeFileSync( + join(historyDir, HISTORY_DIR, '2026-07.jsonl'), + [ + JSON.stringify( + snapshotFixture({ commit: '9'.repeat(40), generatedAt: '2026-07-02T00:00:00.000Z' }), + ), + JSON.stringify( + snapshotFixture({ + commit: baseSha, + generatedAt: '2026-07-08T00:00:00.000Z', + typeInversionTotal: 7, + }), + ), + '', + ].join('\n'), + ); + const head = writeJson( + dir, + 'head.json', + snapshotFixture({ commit: 'b'.repeat(40), typeInversionTotal: 5 }), + ); + + const { status, stdout } = runScript('run.ts', [ + '--head', + head, + '--history-dir', + historyDir, + '--base-sha', + baseSha, + ]); + expect(status, stdout).toBe(0); + expect(stdout).toContain(`baseline \`${baseSha.slice(0, 7)}\``); + expect(stdout).toContain('R6 type inversions'); +}); + +test('a missing head snapshot fails loudly, missing optional inputs do not', () => { + const dir = workdir(); + const missing = runScript('run.ts', ['--head', join(dir, 'nope.json')]); + expect(missing.status).toBe(1); + expect(missing.stderr).toContain('pnpm repo-health --out'); + + const head = writeJson(dir, 'head.json', snapshotFixture()); + const degraded = runScript('run.ts', ['--head', head]); + expect(degraded.status, degraded.stderr).toBe(0); + expect(degraded.stdout).toContain('no main baseline'); +}); + +test('appending to the history is idempotent per commit', () => { + const dir = workdir(); + const snapshot = writeJson(dir, 'snapshot.json', snapshotFixture({ commit: '7'.repeat(40) })); + const historyDir = join(dir, 'history-ref'); + + const first = runScript('append-history.ts', [ + '--snapshot', + snapshot, + '--history-dir', + historyDir, + ]); + expect(first.status, first.stderr).toBe(0); + expect(first.stdout).toContain('appended'); + const shard = join(historyDir, HISTORY_DIR, '2026-07.jsonl'); + expect(existsSync(shard)).toBe(true); + expect(readFileSync(shard, 'utf8').trimEnd().split('\n')).toHaveLength(1); + + const second = runScript('append-history.ts', [ + '--snapshot', + snapshot, + '--history-dir', + historyDir, + ]); + expect(second.status).toBe(0); + expect(second.stdout).toContain('already in'); + expect(readFileSync(shard, 'utf8').trimEnd().split('\n')).toHaveLength(1); +}); diff --git a/scripts/quality-delta/run.ts b/scripts/quality-delta/run.ts new file mode 100644 index 000000000..369267501 --- /dev/null +++ b/scripts/quality-delta/run.ts @@ -0,0 +1,133 @@ +// The quality-delta report command (#1424, Track C of #1412): +// +// pnpm quality-delta --head .tmp/repo-health.json --history-dir .tmp/metric-history \ +// --base-sha --markdown .tmp/quality-delta.md +// +// Renders the sticky PR comment body (posted in place by scripts/size-report.mjs +// `--post-comment`, whose marker this file reuses — one comment surface, no new bots) plus the +// unabridged job summary. Thresholds and gate ownership live in thresholds.ts; the comparison and +// the line budget live in model.ts; this file only reads inputs and writes outputs. +// +// It never gates. Every input except the head snapshot is optional: a missing baseline, coverage +// report, or slow-test report costs the rows it would have produced and a job-summary note, which +// is the quiet degradation fork PRs and docs-only PRs rely on. + +import fs from 'node:fs'; +import path from 'node:path'; +import { parseScriptArgs } from '../lib/cli-args.ts'; +import { runAsMain } from '../lib/run-as-main.ts'; +import type { ChangedCoverageResult } from '../coverage-changed/model.ts'; +import type { RepoHealthSnapshot } from '../repo-health/model.ts'; +import type { SlowTestReport } from '../vitest-slow-test-budgets.ts'; +import { findBaseline, HISTORY_DIR, parseHistory } from './history.ts'; +import { computeQualityDelta, renderComment, renderJobSummary } from './model.ts'; + +const USAGE = `Usage: pnpm quality-delta [options] + +Options: + --head Head repo-health snapshot JSON (default .tmp/repo-health.json). + --history-dir Checkout of the main-branch history ref (its ${HISTORY_DIR}/ shards). + --baseline Use this snapshot as the baseline instead of the history. + --base-sha Commit to key the baseline on (default GITHUB_BASE_SHA). + --changed-coverage Changed-line coverage JSON from pnpm check:coverage-changed --json. + --slow-test Slow-test offenders JSON written during the coverage run. + --markdown Write the comment body (marker included) for --post-comment. +`; + +/** + * The sticky-comment marker owned by scripts/size-report.mjs. The comment this command renders IS + * that comment, evolved: same marker, same update-in-place machinery, so a PR keeps exactly one. + * scripts/quality-delta/run.test.ts fails if the two ever drift apart. + */ +export const STICKY_COMMENT_MARKER = ''; + +function readJsonIfExists(filePath: string | undefined): T | null { + if (filePath === undefined || !fs.existsSync(filePath)) return null; + return JSON.parse(fs.readFileSync(filePath, 'utf8')) as T; +} + +/** Newest shard first: the baseline is almost always in the current month. */ +function shardPaths(historyDir: string): string[] { + const dir = path.join(historyDir, HISTORY_DIR); + if (!fs.existsSync(dir)) return []; + return fs + .readdirSync(dir) + .filter((name) => name.endsWith('.jsonl')) + .sort((left, right) => right.localeCompare(left)) + .map((name) => path.join(dir, name)); +} + +function readBaselineFromHistory( + historyDir: string, + baseSha: string | null, +): RepoHealthSnapshot | null { + let newest: RepoHealthSnapshot | null = null; + for (const shard of shardPaths(historyDir)) { + const { entries } = parseHistory(fs.readFileSync(shard, 'utf8')); + const found = findBaseline(entries, baseSha); + if (found === null) continue; + if (found.exact) return found.snapshot; + newest ??= found.snapshot; + } + return newest; +} + +function githubUrls(commit: string): { linkBase: string | null; summaryUrl: string | null } { + const { GITHUB_SERVER_URL, GITHUB_REPOSITORY, GITHUB_RUN_ID } = process.env; + if (!GITHUB_SERVER_URL || !GITHUB_REPOSITORY) return { linkBase: null, summaryUrl: null }; + const repoUrl = `${GITHUB_SERVER_URL}/${GITHUB_REPOSITORY}`; + return { + linkBase: `${repoUrl}/blob/${commit}`, + summaryUrl: GITHUB_RUN_ID ? `${repoUrl}/actions/runs/${GITHUB_RUN_ID}` : null, + }; +} + +function writeFile(filePath: string, contents: string): void { + const resolved = path.resolve(filePath); + fs.mkdirSync(path.dirname(resolved), { recursive: true }); + fs.writeFileSync(resolved, contents); +} + +function run(argv: readonly string[]): number { + const values = parseScriptArgs(argv, USAGE, { + head: { type: 'string', default: '.tmp/repo-health.json' }, + 'history-dir': { type: 'string' }, + baseline: { type: 'string' }, + 'base-sha': { type: 'string' }, + 'changed-coverage': { type: 'string' }, + 'slow-test': { type: 'string' }, + markdown: { type: 'string' }, + }); + + const head = readJsonIfExists(values.head); + if (head === null) { + throw new Error( + `no repo-health snapshot at ${values.head}. Run \`pnpm repo-health --out\` first.`, + ); + } + const baseSha = values['base-sha'] ?? process.env.GITHUB_BASE_SHA ?? null; + const base = + readJsonIfExists(values.baseline) ?? + (values['history-dir'] === undefined + ? null + : readBaselineFromHistory(values['history-dir'], baseSha)); + + const report = computeQualityDelta({ + head, + base, + changedCoverage: readJsonIfExists(values['changed-coverage']), + slowTest: readJsonIfExists(values['slow-test']), + }); + + const context = githubUrls(head.provenance.commit); + const comment = `${STICKY_COMMENT_MARKER}\n${renderComment(report, context)}`; + if (typeof values.markdown === 'string') writeFile(values.markdown, comment); + + const summary = renderJobSummary(report, context); + process.stdout.write(summary); + const summaryPath = process.env.GITHUB_STEP_SUMMARY; + if (summaryPath) fs.appendFileSync(summaryPath, summary); + return 0; +} + +runAsMain(import.meta.url, 'quality-delta', run); diff --git a/scripts/quality-delta/thresholds.test.ts b/scripts/quality-delta/thresholds.test.ts new file mode 100644 index 000000000..5c774679e --- /dev/null +++ b/scripts/quality-delta/thresholds.test.ts @@ -0,0 +1,35 @@ +// The comment's promise is that every row it shows names a real gate and a command you can run. +// That promise is only worth as much as its enumeration: these tests derive from +// QUALITY_DELTA_METRICS itself, so a metric added with a stale path or a made-up script fails here +// rather than shipping a dead link into a PR comment. + +import { existsSync, readFileSync } from 'node:fs'; +import { expect, test } from 'vitest'; +import { QUALITY_DELTA_METRICS } from './thresholds.ts'; + +const packageScripts = ( + JSON.parse(readFileSync('package.json', 'utf8')) as { scripts: Record } +).scripts; + +test('every metric names a file that exists in the tree', () => { + for (const spec of QUALITY_DELTA_METRICS) { + expect(existsSync(spec.owner.path), `${spec.key} -> ${spec.owner.path}`).toBe(true); + } +}); + +test('every metric names a fix command that is a real package script', () => { + for (const spec of QUALITY_DELTA_METRICS) { + const script = spec.owner.fix.replace(/^pnpm /, ''); + expect(packageScripts[script], `${spec.key} -> ${spec.owner.fix}`).toBeDefined(); + } +}); + +test('every metric has a distinct key and a non-trivial threshold', () => { + const keys = QUALITY_DELTA_METRICS.map((spec) => spec.key); + expect(new Set(keys).size).toBe(keys.length); + for (const spec of QUALITY_DELTA_METRICS) { + const threshold = spec.kind === 'delta' ? spec.minAbsDelta : spec.bound; + expect(threshold, spec.key).toBeGreaterThanOrEqual(0); + if (spec.kind === 'delta') expect(spec.minAbsDelta, spec.key).toBeGreaterThan(0); + } +}); diff --git a/scripts/quality-delta/thresholds.ts b/scripts/quality-delta/thresholds.ts new file mode 100644 index 000000000..3aa960990 --- /dev/null +++ b/scripts/quality-delta/thresholds.ts @@ -0,0 +1,240 @@ +// The ONE config block for the quiet quality-delta comment (#1424, Track C of #1412). +// +// Every metric the PR comment can show is declared here, with the threshold that decides whether +// it is worth a line and the gate that owns the fix. Nothing else in this directory hard-codes a +// number or a gate name: model.ts compares, run.ts does I/O. +// +// The hard maintainer requirement is "do not drown people in information". A metric earns a row +// only when it crosses its own threshold, so raising the bar for one metric is a one-line edit +// here — and `MAX_COMMENT_LINES` caps the whole comment regardless of how many crossed. +// +// Thresholds are deliberately coarse: they are noise filters, not gates. Nothing here fails a +// build. Every gating number in this repo still lives with its own gate (the changed-line +// threshold in scripts/coverage-changed/model.ts, the ratchets in scripts/layering/check.ts, the +// budgets in scripts/vitest-slow-test-budgets.ts), and the specs below only reference them. + +import { + CHANGED_LINE_COVERAGE_THRESHOLD, + type ChangedCoverageResult, +} from '../coverage-changed/model.ts'; +import type { RepoHealthSnapshot } from '../repo-health/model.ts'; +import type { SlowTestReport } from '../vitest-slow-test-budgets.ts'; + +/** + * Hard cap on the rendered comment, marker line included. Rows past the cap are dropped from the + * comment (lowest severity first) and summarized as "+N more, see the job summary"; the job + * summary always carries every row. + */ +export const MAX_COMMENT_LINES = 20; + +/** Inputs a row can be derived from. Each is optional except the head snapshot. */ +export type DeltaSources = { + head: RepoHealthSnapshot; + /** The main-branch baseline for `head`, read from the JSONL history. */ + base: RepoHealthSnapshot | null; + /** This PR's changed-line coverage report (#1418), which has no main baseline by construction. */ + changedCoverage: ChangedCoverageResult | null; + /** Slow-test offenders recorded by the vitest reporter during the coverage run. */ + slowTest: SlowTestReport | null; +}; + +export type MetricFormat = 'bytes' | 'count' | 'pct'; +export type MetricDirection = 'lower-is-better' | 'higher-is-better'; + +/** Where a reader goes to act on a row: the gate that owns the number, and the command that fixes it. */ +export type MetricOwner = { + /** CI job name, so the row points at the check whose failure it predicts. */ + gate: string; + /** Repo-relative file that owns the number (linked in the comment). */ + path: string; + /** Command a contributor runs locally to reproduce or fix it. */ + fix: string; +}; + +type CommonSpec = { + key: string; + label: string; + format: MetricFormat; + direction: MetricDirection; + owner: MetricOwner; + /** + * The repo-health read artifact this metric comes from, when it is one the snapshot did not + * produce itself. Its `stale` provenance flag then marks the row instead of the row silently + * claiming to describe the head commit. + */ + staleInput?: 'sizeReport' | 'coverageSummary'; +}; + +/** A metric compared against the main baseline; shown when |delta| reaches `minAbsDelta`. */ +export type DeltaMetricSpec = CommonSpec & { + kind: 'delta'; + minAbsDelta: number; + /** Ratchet-style metrics where only movement in the bad direction is notable. */ + worseOnly?: boolean; + read: (snapshot: RepoHealthSnapshot) => number | null; +}; + +/** + * A PR-local metric with no main baseline (changed-line coverage is defined against the diff, not + * against a commit). Shown when the value is worse than `bound`. + */ +export type AbsoluteMetricSpec = CommonSpec & { + kind: 'absolute'; + bound: number; + read: (sources: DeltaSources) => number | null; +}; + +export type MetricSpec = DeltaMetricSpec | AbsoluteMetricSpec; + +const SIZE_OWNER: MetricOwner = { + gate: 'Size / Bundle Size', + path: '.github/workflows/size.yml', + fix: 'pnpm size', +}; + +const COVERAGE_OWNER: MetricOwner = { + gate: 'CI / Coverage', + path: 'scripts/coverage-changed/model.ts', + fix: 'pnpm check:coverage-changed', +}; + +const LAYERING_OWNER: MetricOwner = { + gate: 'CI / Layering Guard', + path: 'scripts/layering/check.ts', + fix: 'pnpm check:layering', +}; + +/** + * Coverage rows use the gate's own threshold plus a margin, so a PR that is merely *approaching* + * the gate gets one quiet line instead of a surprise failure on the next commit. + */ +const CHANGED_LINE_WARN_MARGIN_PCT = 10; + +export const QUALITY_DELTA_METRICS: readonly MetricSpec[] = [ + { + kind: 'delta', + key: 'jsGzipBytes', + label: 'JS gzip', + format: 'bytes', + direction: 'lower-is-better', + // ~4 kB gzip is roughly a new module's worth of emitted code; below that the number is build + // noise. Raw bytes stay in the Size job summary rather than earning a second row here. + minAbsDelta: 4_096, + owner: SIZE_OWNER, + staleInput: 'sizeReport', + read: (snapshot) => snapshot.metrics.size.jsGzipBytes ?? null, + }, + { + kind: 'delta', + key: 'npmTarballBytes', + label: 'npm tarball', + format: 'bytes', + direction: 'lower-is-better', + minAbsDelta: 10_240, + owner: SIZE_OWNER, + staleInput: 'sizeReport', + read: (snapshot) => snapshot.metrics.size.npmTarballBytes ?? null, + }, + { + kind: 'delta', + key: 'typeInversionTotal', + label: 'R6 type inversions', + format: 'count', + direction: 'lower-is-better', + // A ratchet: one pair moving either way is worth a line (it may only shrink, and shrinking is + // the standing direction in CONTEXT.md). + minAbsDelta: 1, + owner: LAYERING_OWNER, + read: (snapshot) => snapshot.metrics.layering.typeInversionTotal, + }, + { + kind: 'delta', + key: 'typeCycleBaseline', + label: 'R9 largest type cycle', + format: 'count', + direction: 'lower-is-better', + minAbsDelta: 1, + owner: LAYERING_OWNER, + read: (snapshot) => snapshot.metrics.layering.typeCycleBaseline, + }, + { + kind: 'delta', + key: 'fallowSuppressions', + label: 'Fallow suppressions', + format: 'count', + direction: 'lower-is-better', + minAbsDelta: 1, + // New suppressions are the signal; removing one is good news the Fallow job already shows. + worseOnly: true, + owner: { + gate: 'CI / Fallow Code Quality', + path: '.fallowrc.json', + fix: 'pnpm check:fallow', + }, + read: (snapshot) => snapshot.metrics.fallow.suppressions.total, + }, + { + kind: 'delta', + key: 'redundantEdges', + label: 'Redundant graph edges', + format: 'count', + direction: 'lower-is-better', + // Reachability, not removability (scripts/depgraph/README.md): only a sizeable jump says the + // import graph's shape changed, so the threshold sits well above per-PR churn. + minAbsDelta: 25, + owner: { + gate: 'CI / Layering Guard', + path: 'scripts/depgraph/README.md', + fix: 'pnpm depgraph', + }, + read: (snapshot) => snapshot.metrics.depgraph.redundantEdges, + }, + { + kind: 'absolute', + key: 'changedLineCoveragePct', + label: 'Changed-line coverage', + format: 'pct', + direction: 'higher-is-better', + bound: CHANGED_LINE_COVERAGE_THRESHOLD + CHANGED_LINE_WARN_MARGIN_PCT, + owner: COVERAGE_OWNER, + read: (sources) => sources.changedCoverage?.pct ?? null, + }, + { + kind: 'absolute', + key: 'changedBranchCoveragePct', + label: 'Changed-branch coverage', + format: 'pct', + direction: 'higher-is-better', + // Non-gating by #1418's amendment, so the bar is low: only badly uncovered new branching. + bound: 50, + owner: COVERAGE_OWNER, + read: (sources) => sources.changedCoverage?.branch.pct ?? null, + }, + { + kind: 'absolute', + key: 'changedLinesExcluded', + label: 'Changed lines excluded from coverage', + format: 'count', + direction: 'lower-is-better', + // Exclusions absorbing new logic (#1418 amendment). A handful is normal; a pile is a choice. + bound: 10, + owner: COVERAGE_OWNER, + read: (sources) => sources.changedCoverage?.excluded.totalLines ?? null, + }, + { + kind: 'absolute', + key: 'slowTestBreaches', + label: 'Slow-test budget breaches', + format: 'count', + direction: 'lower-is-better', + // Any test over its wall-clock budget is worth a line, including the ones inside the 2x + // load-variance band that the gate reports without failing. + bound: 0, + owner: { + gate: 'CI / Coverage', + path: 'scripts/vitest-slow-test-budgets.ts', + fix: 'pnpm test:unit', + }, + read: (sources) => sources.slowTest?.offenders.length ?? null, + }, +]; diff --git a/scripts/vitest-slow-test-budgets.ts b/scripts/vitest-slow-test-budgets.ts index e1d42efdb..606df4de7 100644 --- a/scripts/vitest-slow-test-budgets.ts +++ b/scripts/vitest-slow-test-budgets.ts @@ -13,6 +13,23 @@ export const INTEGRATION_BUDGET_MS = 15_000; // budget and 2x budget the gate reports without failing. export const ENFORCE_FACTOR = 2; +/** One test over its wall-clock budget, as the reporter classifies it. */ +export type SlowTestOffender = { + key: string; + durationMs: number; + budgetMs: number; + /** Past ENFORCE_FACTOR x budget, i.e. this one fails the run rather than only reporting. */ + enforce: boolean; +}; + +/** + * The offenders of one run, persisted when SLOW_TEST_REPORT_ENV names a path, so the quality-delta + * comment (#1424) can show budget breaches without re-running or scraping the suite's stderr. + */ +export type SlowTestReport = { offenders: SlowTestOffender[] }; + +export const SLOW_TEST_REPORT_ENV = 'AGENT_DEVICE_SLOW_TEST_REPORT'; + /** The ratchet as one record, for callers that report it rather than enforce it. */ export const SLOW_TEST_RATCHET = { unitBudgetMs: UNIT_BUDGET_MS, diff --git a/scripts/vitest-slow-test-reporter.ts b/scripts/vitest-slow-test-reporter.ts index 6963d812b..d452ee398 100644 --- a/scripts/vitest-slow-test-reporter.ts +++ b/scripts/vitest-slow-test-reporter.ts @@ -1,8 +1,12 @@ +import fs from 'node:fs'; +import path from 'node:path'; import type { Reporter, TestCase, TestModule } from 'vitest/node'; import { ENFORCE_FACTOR, INTEGRATION_BUDGET_MS, + SLOW_TEST_REPORT_ENV, UNIT_BUDGET_MS, + type SlowTestOffender, } from './vitest-slow-test-budgets.ts'; /** @@ -14,7 +18,23 @@ import { * This reporter fails the run when a unit test exceeds the enforced budget. * The budgets themselves live in ./vitest-slow-test-budgets.ts. */ -type Offender = { key: string; durationMs: number; budgetMs: number; enforce: boolean }; +type Offender = SlowTestOffender; + +/** + * Persist the run's offenders when SLOW_TEST_REPORT_ENV names a path, so the quality-delta comment + * (#1424) reads budget breaches from data instead of scraping this reporter's stderr. Best-effort: + * a report nobody asked for must not fail an otherwise green run. + */ +function writeSlowTestReport(offenders: readonly Offender[]): void { + const target = process.env[SLOW_TEST_REPORT_ENV]; + if (!target) return; + try { + fs.mkdirSync(path.dirname(path.resolve(target)), { recursive: true }); + fs.writeFileSync(path.resolve(target), `${JSON.stringify({ offenders }, null, 2)}\n`); + } catch { + // A missing directory or read-only path is not worth failing a green run over. + } +} function budgetForPath(relativePath: string): number { return relativePath.startsWith('src/') ? UNIT_BUDGET_MS : INTEGRATION_BUDGET_MS; @@ -91,6 +111,7 @@ export default function slowTestGateReporter(): Reporter { if (offender) offenders.push(offender); }, onTestRunEnd(): void { + writeSlowTestReport(offenders); // eslint-disable-next-line no-console if (reportSlowTests(offenders, (message) => console.error(message))) { process.exitCode = 1; diff --git a/vitest.config.ts b/vitest.config.ts index 7bdc3c39c..226ffe3ba 100644 --- a/vitest.config.ts +++ b/vitest.config.ts @@ -25,6 +25,9 @@ export const SUBPROCESS_STUB_TESTS = [ // Spawns the `pnpm repo-health` CLI as a subprocess to exercise its arg handling and the JSON // it writes over the real tree (#1423). 'scripts/repo-health/run.test.ts', + // Spawns the `pnpm quality-delta` / `pnpm repo-health:history` CLIs to exercise the comment they + // write and the JSONL they append (#1424). + 'scripts/quality-delta/run.test.ts', ]; export default defineConfig({ @@ -54,6 +57,11 @@ export default defineConfig({ // subprocess or device work, so it runs in the fast lane. The CLI's subprocess test // lives in the subprocess-stub project. 'scripts/repo-health/model.test.ts', + // Quality-delta comment model (#1424): pure threshold comparison and rendering over + // fixture snapshots. Its CLI's subprocess test lives in the subprocess-stub project. + 'scripts/quality-delta/model.test.ts', + 'scripts/quality-delta/history.test.ts', + 'scripts/quality-delta/thresholds.test.ts', // Parses CI configuration only, so this action guard needs no device or subprocess lane. 'test/ci/upload-agent-device-artifacts.test.ts', 'test/skillgym/suites/local-cli-help-policy.test.ts', From 926c163ba2e0dab9463c21c743f6c767a6861df0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Pierzcha=C5=82a?= Date: Tue, 28 Jul 2026 16:28:40 +0000 Subject: [PATCH 2/9] quality-delta: track the producerInputs field added to artifact provenance Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com> --- scripts/quality-delta/fixtures.ts | 2 ++ 1 file changed, 2 insertions(+) diff --git a/scripts/quality-delta/fixtures.ts b/scripts/quality-delta/fixtures.ts index cbf9e4ee8..fa44d7a8c 100644 --- a/scripts/quality-delta/fixtures.ts +++ b/scripts/quality-delta/fixtures.ts @@ -38,11 +38,13 @@ export function snapshotFixture(overrides: SnapshotOverrides = {}): RepoHealthSn coverageSummary: { path: 'coverage/coverage-summary.json', sha256: 'cccccccccccc', + producerInputs: ['src', 'test', 'vitest.config.ts'], stale: false, }, sizeReport: { path: '.tmp/size-report.json', sha256: 'ssssssssssss', + producerInputs: ['src', 'package.json'], stale: overrides.sizeStale ?? false, }, }, From da3a1aba37f01b23b770aa35a52c6da2d371adb0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Pierzcha=C5=82a?= Date: Tue, 28 Jul 2026 16:31:48 +0000 Subject: [PATCH 3/9] quality-delta: unexport the heading and split the history append out of the CLI shell Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com> --- scripts/quality-delta/append-history.ts | 42 ++++++++++++++----------- scripts/quality-delta/model.ts | 2 +- 2 files changed, 24 insertions(+), 20 deletions(-) diff --git a/scripts/quality-delta/append-history.ts b/scripts/quality-delta/append-history.ts index 62465b909..c48952709 100644 --- a/scripts/quality-delta/append-history.ts +++ b/scripts/quality-delta/append-history.ts @@ -21,34 +21,38 @@ Options: --history-dir Working copy of the history ref; the shard lands in its ${HISTORY_DIR}/. `; +/** The shard file this snapshot belongs in, created empty if the month has no entries yet. */ +function shardFor(historyDir: string, snapshot: RepoHealthSnapshot): string { + const shard = path.join( + path.resolve(historyDir), + HISTORY_DIR, + historyShardName(snapshot.provenance.generatedAt), + ); + fs.mkdirSync(path.dirname(shard), { recursive: true }); + return shard; +} + +function appendSnapshot(historyDir: string, snapshot: RepoHealthSnapshot): string { + const shard = shardFor(historyDir, snapshot); + const outcome = appendEntry(fs.existsSync(shard) ? fs.readFileSync(shard, 'utf8') : '', snapshot); + const commit = snapshot.provenance.commit.slice(0, 12); + if (!outcome.appended) return `${commit} is already in ${path.basename(shard)}; nothing to do.`; + fs.writeFileSync(shard, outcome.text); + return `appended ${commit} to ${path.basename(shard)}.`; +} + function run(argv: readonly string[]): number { const values = parseScriptArgs(argv, USAGE, { snapshot: { type: 'string', default: '.tmp/repo-health.json' }, 'history-dir': { type: 'string' }, }); - if (typeof values['history-dir'] !== 'string') throw new Error('--history-dir is required'); + const historyDir = values['history-dir']; + if (typeof historyDir !== 'string') throw new Error('--history-dir is required'); const snapshot = JSON.parse( fs.readFileSync(path.resolve(values.snapshot ?? ''), 'utf8'), ) as RepoHealthSnapshot; - const shard = path.join( - path.resolve(values['history-dir']), - HISTORY_DIR, - historyShardName(snapshot.provenance.generatedAt), - ); - fs.mkdirSync(path.dirname(shard), { recursive: true }); - const existing = fs.existsSync(shard) ? fs.readFileSync(shard, 'utf8') : ''; - const outcome = appendEntry(existing, snapshot); - if (!outcome.appended) { - process.stdout.write( - `repo-health:history: ${snapshot.provenance.commit.slice(0, 12)} is already in ${path.basename(shard)}; nothing to do.\n`, - ); - return 0; - } - fs.writeFileSync(shard, outcome.text); - process.stdout.write( - `repo-health:history: appended ${snapshot.provenance.commit.slice(0, 12)} to ${path.basename(shard)}.\n`, - ); + process.stdout.write(`repo-health:history: ${appendSnapshot(historyDir, snapshot)}\n`); return 0; } diff --git a/scripts/quality-delta/model.ts b/scripts/quality-delta/model.ts index cbfb20a75..29185a94a 100644 --- a/scripts/quality-delta/model.ts +++ b/scripts/quality-delta/model.ts @@ -27,7 +27,7 @@ import { type MetricSpec, } from './thresholds.ts'; -export const COMMENT_HEADING = 'Quality delta'; +const COMMENT_HEADING = 'Quality delta'; export const NO_DELTAS_LINE = 'No notable quality deltas.'; export type QualityDeltaRow = { From 5d560facdb795a9bd4ee059aa18b9eb3015bdb13 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Pierzcha=C5=82a?= Date: Tue, 28 Jul 2026 16:45:37 +0000 Subject: [PATCH 4/9] quality-delta: render after both producers finish, label inexact baselines, generalize the comment marker Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com> --- .github/workflows/ci.yml | 81 +++----------- .github/workflows/quality-delta.yml | 140 ++++++++++++++++++++++++ package.json | 1 + scripts/quality-delta/README.md | 38 +++++-- scripts/quality-delta/model.test.ts | 19 +++- scripts/quality-delta/model.ts | 28 ++++- scripts/quality-delta/producers-run.ts | 68 ++++++++++++ scripts/quality-delta/producers.test.ts | 85 ++++++++++++++ scripts/quality-delta/producers.ts | 69 ++++++++++++ scripts/quality-delta/run.test.ts | 40 ++++++- scripts/quality-delta/run.ts | 62 +++++++---- scripts/quality-delta/thresholds.ts | 6 + scripts/size-report.mjs | 12 +- vitest.config.ts | 1 + 14 files changed, 547 insertions(+), 103 deletions(-) create mode 100644 .github/workflows/quality-delta.yml create mode 100644 scripts/quality-delta/producers-run.ts create mode 100644 scripts/quality-delta/producers.test.ts create mode 100644 scripts/quality-delta/producers.ts diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 4ccf37d91..f7ba49716 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -240,13 +240,6 @@ jobs: name: Coverage runs-on: ubuntu-latest timeout-minutes: 30 - permissions: - contents: read - # The quality-delta comment (#1424) is posted from this job, and reads the Size workflow's - # size-report artifact for the same head sha. Fork PRs get a read-only token, so the posting - # step is skipped there and the delta degrades to job-summary output. - pull-requests: write - actions: read steps: - name: Checkout uses: actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd # v6.0.2 @@ -277,67 +270,23 @@ jobs: AGENT_DEVICE_COVERAGE_WAIVER: ${{ contains(github.event.pull_request.labels.*.name, 'coverage-waiver') }} run: pnpm check:coverage-changed --base "${{ github.event.pull_request.base.sha }}" --json .tmp/changed-coverage.json - # --- Quality-delta comment (#1424) ------------------------------------------------------ - # Non-gating reporting, ~20s total on top of this job: it reuses the lcov-derived numbers - # above, the size artifact the Size workflow already produced, and one repo-health snapshot - # (~1s, no network). Nothing below can fail the job. - - # Best effort by design: when the Size workflow has not published yet (or is skipped, e.g. - # a fork PR), the comment simply carries no size rows and says so in the job summary. - - name: Fetch the Size workflow's report for this head sha - if: always() && github.event_name == 'pull_request' - continue-on-error: true - env: - GH_TOKEN: ${{ github.token }} - HEAD_SHA: ${{ github.event.pull_request.head.sha }} - run: | - set -uo pipefail - id=$(gh api "repos/${GITHUB_REPOSITORY}/actions/artifacts?name=size-report-json&per_page=100" \ - --jq "[.artifacts[] | select(.workflow_run.head_sha == \"${HEAD_SHA}\") | .id] | first") - if [ -z "$id" ] || [ "$id" = "null" ]; then - echo "No size-report artifact for ${HEAD_SHA} yet; size rows will be omitted." - exit 0 - fi - mkdir -p .tmp - gh api "repos/${GITHUB_REPOSITORY}/actions/artifacts/${id}/zip" > .tmp/size-report.zip - unzip -o -q -d .tmp .tmp/size-report.zip - - # Read-only extraction of the history ref written by the Repo Health History workflow — no - # checkout, no write, absent until that workflow has run once. - - name: Fetch the main-branch metric history - if: always() && github.event_name == 'pull_request' - continue-on-error: true - run: | - set -uo pipefail - mkdir -p .tmp/history-ref - if git fetch --quiet --depth 1 origin +refs/heads/metrics/repo-health:refs/remotes/origin/metrics/repo-health; then - git archive origin/metrics/repo-health | tar -x -C .tmp/history-ref - else - echo "No metrics/repo-health ref yet; the comment will report no baseline." - fi - - - name: Render quality delta + # Inputs for the quality-delta comment (#1424), which is rendered by + # .github/workflows/quality-delta.yml AFTER both this workflow and Size finish — rendering + # here would race the concurrently produced size report and permanently omit its rows. + # Publishing them costs one upload; nothing about the comment can fail this job. + - name: Upload quality-delta inputs if: always() && github.event_name == 'pull_request' continue-on-error: true - run: | - pnpm repo-health --out .tmp/repo-health.json - pnpm quality-delta \ - --head .tmp/repo-health.json \ - --history-dir .tmp/history-ref \ - --base-sha "${{ github.event.pull_request.base.sha }}" \ - --changed-coverage .tmp/changed-coverage.json \ - --slow-test .tmp/slow-tests.json \ - --markdown .tmp/quality-delta.md - - # One sticky comment per PR, updated in place by the size-report marker machinery. Fork PRs - # cannot write comments with their read-only token, so they stop at the job summary above. - - name: Comment the quality delta on the PR - if: always() && github.event_name == 'pull_request' && github.event.pull_request.head.repo.full_name == github.repository - continue-on-error: true - env: - GITHUB_TOKEN: ${{ github.token }} - GITHUB_PR_NUMBER: ${{ github.event.pull_request.number }} - run: node scripts/size-report.mjs --post-comment .tmp/quality-delta.md + uses: actions/upload-artifact@ea165f8d65b6e75b540449e92b4886f43607fa02 # v4.6.2 + with: + name: quality-delta-inputs + path: | + .tmp/changed-coverage.json + .tmp/slow-tests.json + # .tmp is a dotted directory, which upload-artifact skips by default. + include-hidden-files: true + retention-days: 7 + if-no-files-found: warn typecheck: name: Typecheck diff --git a/.github/workflows/quality-delta.yml b/.github/workflows/quality-delta.yml new file mode 100644 index 000000000..3edaa19bc --- /dev/null +++ b/.github/workflows/quality-delta.yml @@ -0,0 +1,140 @@ +name: Quality Delta + +# The single sticky PR comment (#1424). It renders from two workflows that run CONCURRENTLY on the +# same head sha — CI (changed-line coverage, slow-test breaches) and Size (the size report) — so it +# runs on their completion instead of inside either one: whichever finishes LAST triggers the render, +# and earlier firings exit at the readiness gate below. Rendering inside CI would lose the race +# whenever Size finished second, permanently omitting size rows from that head's comment. +# +# Fork PRs are skipped deliberately, twice over: `workflow_run` runs with a write token in the BASE +# repo, so checking out and installing fork code here would be a pwn-request. Fork PRs therefore get +# the producers' own job summaries, which is the documented degradation (never a failure). + +on: + workflow_run: + workflows: + - CI + - Size + types: + - completed + +permissions: + contents: read + actions: read + pull-requests: write + +# One render per head sha, and never two comment writes at once. Superseding is safe and wanted: a +# newer run describes a newer head. +concurrency: + group: quality-delta-${{ github.event.workflow_run.head_sha }} + cancel-in-progress: true + +jobs: + comment: + name: Quality Delta + if: github.event.workflow_run.event == 'pull_request' && github.event.workflow_run.head_repository.full_name == github.repository + runs-on: ubuntu-latest + timeout-minutes: 15 + steps: + # The base repo's tree at the producers' head sha. Safe because the fork guard above means this + # sha is always a branch in this repository. + - name: Checkout + uses: actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd # v6.0.2 + with: + ref: ${{ github.event.workflow_run.head_sha }} + fetch-depth: 0 + + # The race policy, as a decision the unit tests own: scripts/quality-delta/producers.ts. + - name: Check that every producer finished + id: producers + env: + GH_TOKEN: ${{ github.token }} + HEAD_SHA: ${{ github.event.workflow_run.head_sha }} + run: | + set -euo pipefail + mkdir -p .tmp + gh api "repos/${GITHUB_REPOSITORY}/actions/runs?head_sha=${HEAD_SHA}&per_page=100" > .tmp/runs.json + node --experimental-strip-types scripts/quality-delta/producers-run.ts \ + --runs .tmp/runs.json --head-sha "${HEAD_SHA}" + + - name: Setup toolchain + if: steps.producers.outputs.ready == 'true' + uses: ./.github/actions/setup-node-pnpm + + # Both downloads are best effort: a producer that failed before uploading costs the rows it + # owned plus a job-summary note, never the comment. + - name: Download producer artifacts + if: steps.producers.outputs.ready == 'true' + continue-on-error: true + env: + GH_TOKEN: ${{ github.token }} + CI_RUN_ID: ${{ steps.producers.outputs.ci_run_id }} + SIZE_RUN_ID: ${{ steps.producers.outputs.size_run_id }} + run: | + set -uo pipefail + download() { + local run_id="$1" name="$2" + local id + id=$(gh api "repos/${GITHUB_REPOSITORY}/actions/runs/${run_id}/artifacts" \ + --jq "[.artifacts[] | select(.name == \"${name}\" and .expired == false) | .id] | first") + if [ -z "$id" ] || [ "$id" = "null" ]; then + echo "run ${run_id} published no ${name}; its rows will be omitted." + return 0 + fi + gh api "repos/${GITHUB_REPOSITORY}/actions/artifacts/${id}/zip" > ".tmp/${name}.zip" + unzip -o -q -d .tmp ".tmp/${name}.zip" + } + mkdir -p .tmp + download "${SIZE_RUN_ID}" size-report-json + download "${CI_RUN_ID}" quality-delta-inputs + + # Read-only extraction of the history ref the Repo Health History workflow writes on main — no + # checkout, no write, absent until that workflow has run once. + - name: Fetch the main-branch metric history + if: steps.producers.outputs.ready == 'true' + continue-on-error: true + run: | + set -uo pipefail + mkdir -p .tmp/history-ref + if git fetch --quiet --depth 1 origin +refs/heads/metrics/repo-health:refs/remotes/origin/metrics/repo-health; then + git archive origin/metrics/repo-health | tar -x -C .tmp/history-ref + else + echo "No metrics/repo-health ref yet; the comment will report no baseline." + fi + + - name: Resolve the pull request and its base + if: steps.producers.outputs.ready == 'true' + id: pr + env: + GH_TOKEN: ${{ github.token }} + HEAD_SHA: ${{ github.event.workflow_run.head_sha }} + run: | + set -euo pipefail + gh api "repos/${GITHUB_REPOSITORY}/commits/${HEAD_SHA}/pulls" > .tmp/pulls.json + number=$(jq -r '[.[] | select(.state == "open") | .number] | first // empty' .tmp/pulls.json) + base=$(jq -r '[.[] | select(.state == "open") | .base.sha] | first // empty' .tmp/pulls.json) + echo "number=${number}" >> "$GITHUB_OUTPUT" + echo "base_sha=${base}" >> "$GITHUB_OUTPUT" + + - name: Render quality delta + if: steps.producers.outputs.ready == 'true' && steps.pr.outputs.number != '' + continue-on-error: true + run: | + pnpm repo-health --out .tmp/repo-health.json + pnpm quality-delta \ + --head .tmp/repo-health.json \ + --history-dir .tmp/history-ref \ + --base-sha "${{ steps.pr.outputs.base_sha }}" \ + --changed-coverage .tmp/changed-coverage.json \ + --slow-test .tmp/slow-tests.json \ + --markdown .tmp/quality-delta.md + + # ONE comment per PR: the marker machinery in scripts/size-report.mjs updates it in place, and + # no other workflow posts a comment. + - name: Comment on the PR + if: steps.producers.outputs.ready == 'true' && steps.pr.outputs.number != '' + continue-on-error: true + env: + GITHUB_TOKEN: ${{ github.token }} + GITHUB_PR_NUMBER: ${{ steps.pr.outputs.number }} + run: node scripts/size-report.mjs --post-comment .tmp/quality-delta.md diff --git a/package.json b/package.json index 58bd49b09..b96811ec9 100644 --- a/package.json +++ b/package.json @@ -135,6 +135,7 @@ "repo-health": "node --experimental-strip-types scripts/repo-health/run.ts", "repo-health:history": "node --experimental-strip-types scripts/quality-delta/append-history.ts", "quality-delta": "node --experimental-strip-types scripts/quality-delta/run.ts", + "quality-delta:producers": "node --experimental-strip-types scripts/quality-delta/producers-run.ts", "check:production-exports": "fallow dead-code --config fallow-production-exports.json --production --unused-exports --fail-on-issues", "check:bundle-owner-files": "node --experimental-strip-types scripts/check-bundle-owner-files.ts", "check:command-docs": "vitest run --project unit-core src/__tests__/command-doc-coverage.test.ts", diff --git a/scripts/quality-delta/README.md b/scripts/quality-delta/README.md index 8f59f8682..8c44564ad 100644 --- a/scripts/quality-delta/README.md +++ b/scripts/quality-delta/README.md @@ -83,8 +83,10 @@ git fetch --depth 1 origin +refs/heads/metrics/repo-health:refs/remotes/origin/m git show origin/metrics/repo-health:history/2026-07.jsonl | grep "$(git rev-parse HEAD)" | jq .metrics ``` -A PR whose exact base sha is not in the store yet falls back to the newest entry (a near baseline -states a real delta; no baseline states nothing), and the job summary records which was used. +A PR whose exact base sha is not in the store yet falls back to the newest entry — a near baseline +states a real delta; no baseline states nothing — but it is **labelled**: the comment heading reads +`vs nearest main@ (PR base not in history yet)` and the job summary says deltas may include +main-side changes the PR did not make. Only an exact base-sha hit renders as a plain `vs main@`. ### Serializing concurrent writes @@ -101,12 +103,32 @@ Two merges landing seconds apart must not lose an entry, so the history job uses | Where | What it does | | --- | --- | | `.github/workflows/repo-health-history.yml` | push-to-main: build → `pnpm size:markdown` → snapshot → append to `metrics/repo-health`. Main only; not on the PR gate. | -| `.github/workflows/ci.yml` (Coverage job) | PR: reuses the coverage run's lcov numbers and slow-test report, pulls the Size workflow's `size-report-json` artifact for the same head sha, snapshots, renders, and posts the sticky comment. | -| `.github/workflows/size.yml` | still measures base vs head into the job summary, and now publishes `size-report-json` instead of posting its own comment. | - -The comment reuses `scripts/size-report.mjs --post-comment` and its `COMMENT_MARKER` verbatim, so a -PR keeps exactly one marker-tracked comment that is updated in place. `run.test.ts` fails if the -marker in `run.ts` and the one in `size-report.mjs` ever drift apart. +| `.github/workflows/ci.yml` (Coverage job) | PR: writes the changed-line coverage JSON and slow-test report and uploads them as `quality-delta-inputs`. Does not render or comment. | +| `.github/workflows/size.yml` | still measures base vs head into the job summary, and publishes `size-report-json` instead of posting its own comment. | +| `.github/workflows/quality-delta.yml` | `workflow_run` on **CI and Size**: renders and posts the one sticky comment, after both producers finished. | + +### Why rendering is its own workflow + +The rows come from two workflows that run **concurrently** on the same head sha. Rendering inside +either one is a race whose loser is permanent: the comment is written once, so a size report that +lands a minute later never reaches it. So the render runs on `workflow_run` for both producers and +[`producers.ts`](./producers.ts) decides readiness — the **last** producer to complete renders, and +earlier firings exit cleanly because the later completion fires the trigger again. A re-run of one +producer supersedes its earlier run. `producers.test.ts` owns that policy, including the assertion +that no other workflow contains `--post-comment`. + +Fork PRs are skipped there deliberately: `workflow_run` carries a **write token in the base repo**, +so checking out and installing fork code in it would be a pwn-request. Fork PRs keep the producers' +job summaries, which is the documented degradation. + +### One comment, not one bot per metric + +The comment reuses `scripts/size-report.mjs --post-comment` and its marker machinery, so a PR keeps +exactly one marker-tracked comment updated in place — size is now one row family inside it, which is +why the marker is `` rather than the size-specific one it grew out +of. The old marker stays in `LEGACY_COMMENT_MARKERS` so comments posted before the rename are updated +instead of duplicated. `run.test.ts` fails if the marker in `run.ts` and the ones in +`size-report.mjs` drift apart. ## Schema migrations diff --git a/scripts/quality-delta/model.test.ts b/scripts/quality-delta/model.test.ts index 934465f96..16e2eedf3 100644 --- a/scripts/quality-delta/model.test.ts +++ b/scripts/quality-delta/model.test.ts @@ -9,6 +9,7 @@ function sources(overrides: Partial = {}): DeltaSources { return { head: snapshotFixture({ commit: 'b'.repeat(40) }), base: snapshotFixture(), + baseExact: true, changedCoverage: null, slowTest: null, ...overrides, @@ -91,8 +92,24 @@ test('differing schemaVersions are refused instead of diffed, PR-local rows surv expect(comment).toContain('Changed-line coverage'); }); +test('a baseline that is not the PR base is labelled as the nearest stored snapshot', () => { + const head = snapshotFixture({ commit: 'b'.repeat(40), typeInversionTotal: 20 }); + const report = computeQualityDelta(sources({ head, baseExact: false })); + const comment = renderComment(report, CONTEXT); + // Otherwise main-side movement since the PR's base gets attributed to the PR. + expect(comment).toContain('vs nearest main@`aaaaaaa` (PR base not in history yet)'); + expect(report.base?.exact).toBe(false); + expect(renderJobSummary(report, CONTEXT)).toContain( + 'Deltas may include main-side changes this PR did not make.', + ); + + const exact = renderComment(computeQualityDelta(sources({ head })), CONTEXT); + expect(exact).toContain('vs main@`aaaaaaa`'); + expect(exact).not.toContain('nearest'); +}); + test('without a baseline the comment says so in one line and notes it in the summary', () => { - const report = computeQualityDelta(sources({ base: null })); + const report = computeQualityDelta(sources({ base: null, baseExact: false })); const comment = renderComment(report, CONTEXT); expect(comment.trim().split('\n')).toHaveLength(1); expect(comment).toContain('no main baseline'); diff --git a/scripts/quality-delta/model.ts b/scripts/quality-delta/model.ts index 29185a94a..dbfc0f44e 100644 --- a/scripts/quality-delta/model.ts +++ b/scripts/quality-delta/model.ts @@ -15,6 +15,10 @@ // source file is newer than the artifact): the row is kept but marked, never silently paired // with the head commit. // +// And one label: a baseline that is NOT the PR's own base commit (the history has no entry for it +// yet) renders as the nearest stored main snapshot, because such a delta can include main-side +// movement the PR did not cause. +// // All I/O — reading files, resolving links, posting the comment — belongs to run.ts. import type { RepoHealthSnapshot } from '../repo-health/model.ts'; @@ -50,7 +54,7 @@ export type QualityDeltaRow = { export type QualityDeltaReport = { rows: readonly QualityDeltaRow[]; head: { commit: string; schemaVersion: number }; - base: { commit: string; schemaVersion: number } | null; + base: { commit: string; schemaVersion: number; exact: boolean } | null; /** Baseline and head disagree on schemaVersion, so baseline-derived rows are refused. */ schemaMismatch: { base: number; head: number } | null; /** Quiet degradations (missing inputs, absent baseline). Job summary only. */ @@ -131,6 +135,12 @@ function missingInputNotes(sources: DeltaSources): string[] { 'No main-branch baseline for this PR yet — baseline-derived rows are omitted. ' + 'The history job appends one snapshot per push to main (see scripts/quality-delta/README.md).', ); + } else if (!sources.baseExact) { + notes.push( + "The history has no entry for this PR's base commit yet, so the baseline is the nearest " + + `stored main snapshot (\`${shortSha(sources.base.provenance.commit)}\`). Deltas may include ` + + 'main-side changes this PR did not make.', + ); } if (sources.head.metrics.size.available !== true) { notes.push( @@ -167,7 +177,11 @@ export function computeQualityDelta(sources: DeltaSources): QualityDeltaReport { base: sources.base === null ? null - : { commit: sources.base.provenance.commit, schemaVersion: sources.base.schemaVersion }, + : { + commit: sources.base.provenance.commit, + schemaVersion: sources.base.schemaVersion, + exact: sources.baseExact, + }, schemaMismatch, notes: schemaMismatch === null ? missingInputNotes(sources) : missingInputNotes(comparable), }; @@ -201,9 +215,13 @@ function shortSha(commit: string): string { } function headingLine(report: QualityDeltaReport): string { - const against = - report.base === null ? 'no main baseline' : `main@\`${shortSha(report.base.commit)}\``; - return `### ${COMMENT_HEADING} vs ${against}`; + if (report.base === null) return `### ${COMMENT_HEADING} vs no main baseline`; + const sha = `main@\`${shortSha(report.base.commit)}\``; + // An inexact baseline is labelled in the heading, not buried in the job summary: the reader is + // about to attribute these numbers to the PR, and part of them may belong to main. + return report.base.exact + ? `### ${COMMENT_HEADING} vs ${sha}` + : `### ${COMMENT_HEADING} vs nearest ${sha} (PR base not in history yet)`; } function summaryLink(summaryUrl: string | null): string { diff --git a/scripts/quality-delta/producers-run.ts b/scripts/quality-delta/producers-run.ts new file mode 100644 index 000000000..177e878cb --- /dev/null +++ b/scripts/quality-delta/producers-run.ts @@ -0,0 +1,68 @@ +// CLI shell for the producer-readiness decision (#1424): +// +// gh api "repos/$REPO/actions/runs?head_sha=$SHA&per_page=100" > .tmp/runs.json +// pnpm quality-delta:producers --runs .tmp/runs.json --head-sha "$SHA" +// +// Writes `ready`, `ci_run_id`, and `size_run_id` to $GITHUB_OUTPUT so the rendering workflow can +// skip cleanly while a producer is still running. Exit code stays 0 either way: "not yet" is a +// normal outcome, not a failure. + +import fs from 'node:fs'; +import { parseScriptArgs } from '../lib/cli-args.ts'; +import { runAsMain } from '../lib/run-as-main.ts'; +import { producerReadiness, type ProducerReadiness, type WorkflowRunSummary } from './producers.ts'; + +const USAGE = `Usage: pnpm quality-delta:producers --runs --head-sha + +Options: + --runs JSON from GitHub's actions/runs API (its workflow_runs array). + --head-sha The head commit the comment will describe. +`; + +/** The run ids the rendering steps need, so they can download from the right runs. */ +function writeStepOutputs(readiness: ProducerReadiness): void { + const outputPath = process.env.GITHUB_OUTPUT; + if (!outputPath) return; + fs.appendFileSync( + outputPath, + [ + `ready=${readiness.ready}`, + `ci_run_id=${readiness.runIds.CI ?? ''}`, + `size_run_id=${readiness.runIds.Size ?? ''}`, + '', + ].join('\n'), + ); +} + +function requiredArgs(argv: readonly string[]): { runsPath: string; headSha: string } { + const values = parseScriptArgs(argv, USAGE, { + runs: { type: 'string' }, + 'head-sha': { type: 'string' }, + }); + const runsPath = values.runs; + const headSha = values['head-sha']; + if (typeof runsPath !== 'string' || typeof headSha !== 'string') { + throw new Error('--runs and --head-sha are required'); + } + return { runsPath, headSha }; +} + +function readRuns(runsPath: string): readonly WorkflowRunSummary[] { + const payload = JSON.parse(fs.readFileSync(runsPath, 'utf8')) as { + workflow_runs?: WorkflowRunSummary[]; + }; + return payload.workflow_runs ?? []; +} + +function run(argv: readonly string[]): number { + const { runsPath, headSha } = requiredArgs(argv); + const readiness = producerReadiness(readRuns(runsPath), headSha); + const verdict = readiness.ready + ? `every producer completed for ${headSha.slice(0, 12)}` + : readiness.reason; + process.stdout.write(`quality-delta: ${verdict}.\n`); + writeStepOutputs(readiness); + return 0; +} + +runAsMain(import.meta.url, 'quality-delta:producers', run); diff --git a/scripts/quality-delta/producers.test.ts b/scripts/quality-delta/producers.test.ts new file mode 100644 index 000000000..e60777e2d --- /dev/null +++ b/scripts/quality-delta/producers.test.ts @@ -0,0 +1,85 @@ +// The artifact-race policy (#1424): the comment renders only after BOTH producers finish, and the +// wiring that guarantees it is a `workflow_run` trigger on both of them. Both halves are pinned +// here, because the failure mode is silent — a comment that permanently lacks size rows still looks +// like a perfectly good comment. + +import { readFileSync } from 'node:fs'; +import { expect, test } from 'vitest'; +import { parse } from 'yaml'; +import { producerReadiness, PRODUCER_WORKFLOWS, type WorkflowRunSummary } from './producers.ts'; + +const HEAD = 'a'.repeat(40); + +function run(overrides: Partial): WorkflowRunSummary { + return { id: 1, name: 'CI', head_sha: HEAD, status: 'completed', ...overrides }; +} + +test('rendering waits until every producer completed for this head sha', () => { + const both = [run({ id: 1, name: 'CI' }), run({ id: 2, name: 'Size' })]; + expect(producerReadiness(both, HEAD)).toEqual({ + ready: true, + runIds: { CI: 1, Size: 2 }, + reason: '', + }); + + const sizeRunning = [ + run({ id: 1, name: 'CI' }), + run({ id: 2, name: 'Size', status: 'in_progress' }), + ]; + const deferred = producerReadiness(sizeRunning, HEAD); + expect(deferred.ready).toBe(false); + expect(deferred.reason).toContain('Size (in_progress)'); + // The later completion fires the trigger again, so deferring loses nothing. + expect(deferred.reason).toContain('last producer to complete renders the comment'); + + const onlyCi = [run({ id: 1, name: 'CI' })]; + expect(producerReadiness(onlyCi, HEAD).reason).toContain('Size (no run for this sha)'); +}); + +test('runs for another head sha never make this one look ready', () => { + const other = [ + run({ id: 1, name: 'CI' }), + run({ id: 2, name: 'Size', head_sha: 'b'.repeat(40) }), + ]; + const readiness = producerReadiness(other, HEAD); + expect(readiness.ready).toBe(false); + expect(readiness.runIds.Size).toBe(null); +}); + +test('a re-run supersedes the earlier run of the same producer', () => { + const rerun = [ + run({ id: 1, name: 'CI' }), + run({ id: 2, name: 'Size', status: 'completed' }), + run({ id: 9, name: 'Size', status: 'in_progress' }), + ]; + const readiness = producerReadiness(rerun, HEAD); + expect(readiness.ready).toBe(false); + expect(readiness.runIds.Size).toBe(9); +}); + +test('the rendering workflow triggers on both producers and gates every step on readiness', () => { + const workflow = parse(readFileSync('.github/workflows/quality-delta.yml', 'utf8')) as { + on: { workflow_run: { workflows: string[]; types: string[] } }; + jobs: Record; + }; + expect(workflow.on.workflow_run.workflows.toSorted()).toEqual([...PRODUCER_WORKFLOWS].toSorted()); + expect(workflow.on.workflow_run.types).toEqual(['completed']); + + const job = workflow.jobs.comment!; + // Fork PRs must not be checked out by a write-token workflow, and get job summaries instead. + expect(job.if).toContain('head_repository.full_name == github.repository'); + + const gated = job.steps.filter((step) => + step.if?.includes("steps.producers.outputs.ready == 'true'"), + ); + // Everything after the readiness check is gated on it; the checkout and the check itself are not. + expect(gated).toHaveLength(job.steps.length - 2); +}); + +test('no other workflow posts a PR comment', () => { + // Comment fatigue is the constraint (#1424): exactly one workflow may write the sticky comment. + const posting = ['ci', 'size', 'quality-delta', 'repo-health-history'].filter((name) => + readFileSync(`.github/workflows/${name}.yml`, 'utf8').includes('--post-comment'), + ); + expect(posting).toEqual(['quality-delta']); +}); diff --git a/scripts/quality-delta/producers.ts b/scripts/quality-delta/producers.ts new file mode 100644 index 000000000..255342355 --- /dev/null +++ b/scripts/quality-delta/producers.ts @@ -0,0 +1,69 @@ +// Who has to finish before the quality-delta comment can be rendered (#1424). +// +// The comment's rows come from two workflows that run CONCURRENTLY on the same head sha: CI +// (changed-line coverage, slow-test breaches) and Size (the size report). Rendering from inside +// either one is a race whose loser is permanent — the comment is posted once, so a size report that +// lands a minute later never reaches it. So rendering happens in a `workflow_run`-triggered +// workflow that fires on BOTH producers' completion and renders only on the LAST one to finish; +// earlier firings exit cleanly, because the later completion will fire the trigger again. +// +// This module is the pure decision: given the runs GitHub reports for a head sha, is everything the +// comment needs available, and from which run ids? + +/** The fields this decision needs from GitHub's `actions/runs` payload. */ +export type WorkflowRunSummary = { + id: number; + name: string; + head_sha: string; + /** 'queued' | 'in_progress' | 'completed' (GitHub also uses several waiting states). */ + status: string; +}; + +/** Workflow names that must have completed for the comment to carry all of its rows. */ +export const PRODUCER_WORKFLOWS = ['CI', 'Size'] as const; + +export type ProducerReadiness = { + /** Every producer has a completed run for this head sha. */ + ready: boolean; + /** Newest run id per producer, or null when it has not started for this sha. */ + runIds: Readonly>; + /** Why rendering is deferred, for the job log. Empty when ready. */ + reason: string; +}; + +/** + * The newest run per producer wins: a re-run of Size supersedes the earlier one, and its artifact is + * the one a fresh comment should describe. + */ +function newestRunFor( + runs: readonly WorkflowRunSummary[], + workflow: string, + headSha: string, +): WorkflowRunSummary | null { + const matching = runs + .filter((run) => run.name === workflow && run.head_sha === headSha) + .sort((left, right) => right.id - left.id); + return matching[0] ?? null; +} + +export function producerReadiness( + runs: readonly WorkflowRunSummary[], + headSha: string, +): ProducerReadiness { + const runIds: Record = {}; + const pending: string[] = []; + for (const workflow of PRODUCER_WORKFLOWS) { + const run = newestRunFor(runs, workflow, headSha); + runIds[workflow] = run?.id ?? null; + if (run === null) pending.push(`${workflow} (no run for this sha)`); + else if (run.status !== 'completed') pending.push(`${workflow} (${run.status})`); + } + return { + ready: pending.length === 0, + runIds, + reason: + pending.length === 0 + ? '' + : `waiting for ${pending.join(', ')}; the last producer to complete renders the comment`, + }; +} diff --git a/scripts/quality-delta/run.test.ts b/scripts/quality-delta/run.test.ts index 772109fca..225deebc6 100644 --- a/scripts/quality-delta/run.test.ts +++ b/scripts/quality-delta/run.test.ts @@ -14,11 +14,12 @@ import { STICKY_COMMENT_MARKER } from './run.ts'; function runScript( script: string, args: readonly string[], + env: Record = {}, ): { status: number | null; stdout: string; stderr: string } { const result = spawnSync( process.execPath, ['--experimental-strip-types', `scripts/quality-delta/${script}`, ...args], - { encoding: 'utf8', env: { ...process.env, GITHUB_STEP_SUMMARY: '' } }, + { encoding: 'utf8', env: { ...process.env, GITHUB_STEP_SUMMARY: '', ...env } }, ); return { status: result.status, stdout: result.stdout ?? '', stderr: result.stderr ?? '' }; } @@ -38,6 +39,8 @@ test('the sticky-comment marker stays the one scripts/size-report.mjs posts', () // second comment per PR instead of updating one in place. const sizeReport = readFileSync('scripts/size-report.mjs', 'utf8'); expect(sizeReport).toContain(`const COMMENT_MARKER = '${STICKY_COMMENT_MARKER}'`); + // The pre-#1424 marker stays recognized, or PRs that already carry that comment get a second one. + expect(sizeReport).toContain("LEGACY_COMMENT_MARKERS = ['']"); }); test('the CLI writes a marker-prefixed comment and prints the full summary', () => { @@ -113,6 +116,20 @@ test('the CLI resolves its baseline out of the JSONL history by base sha', () => expect(status, stdout).toBe(0); expect(stdout).toContain(`baseline \`${baseSha.slice(0, 7)}\``); expect(stdout).toContain('R6 type inversions'); + + // A base sha the history has not stored yet falls back to the newest entry — allowed, but it must + // say so, or main-side movement since the base reads as this PR's doing. + const fallback = runScript('run.ts', [ + '--head', + head, + '--history-dir', + historyDir, + '--base-sha', + 'f'.repeat(40), + ]); + expect(fallback.status, fallback.stdout).toBe(0); + expect(fallback.stdout).toContain('nearest'); + expect(fallback.stdout).toContain('main-side changes this PR did not make'); }); test('a missing head snapshot fails loudly, missing optional inputs do not', () => { @@ -127,6 +144,27 @@ test('a missing head snapshot fails loudly, missing optional inputs do not', () expect(degraded.stdout).toContain('no main baseline'); }); +test('the producer gate reports readiness and run ids through $GITHUB_OUTPUT', () => { + const dir = workdir(); + const headSha = 'c'.repeat(40); + const runs = writeJson(dir, 'runs.json', { + workflow_runs: [ + { id: 11, name: 'CI', head_sha: headSha, status: 'completed' }, + { id: 22, name: 'Size', head_sha: headSha, status: 'in_progress' }, + ], + }); + const output = join(dir, 'github-output'); + writeFileSync(output, ''); + + const waiting = runScript('producers-run.ts', ['--runs', runs, '--head-sha', headSha], { + GITHUB_OUTPUT: output, + }); + // Not ready is a normal outcome: Size's completion fires the workflow again. + expect(waiting.status, waiting.stderr).toBe(0); + expect(readFileSync(output, 'utf8')).toContain('ready=false'); + expect(readFileSync(output, 'utf8')).toContain('size_run_id=22'); +}); + test('appending to the history is idempotent per commit', () => { const dir = workdir(); const snapshot = writeJson(dir, 'snapshot.json', snapshotFixture({ commit: '7'.repeat(40) })); diff --git a/scripts/quality-delta/run.ts b/scripts/quality-delta/run.ts index 369267501..c76aab92c 100644 --- a/scripts/quality-delta/run.ts +++ b/scripts/quality-delta/run.ts @@ -19,7 +19,7 @@ import { runAsMain } from '../lib/run-as-main.ts'; import type { ChangedCoverageResult } from '../coverage-changed/model.ts'; import type { RepoHealthSnapshot } from '../repo-health/model.ts'; import type { SlowTestReport } from '../vitest-slow-test-budgets.ts'; -import { findBaseline, HISTORY_DIR, parseHistory } from './history.ts'; +import { findBaseline, HISTORY_DIR, parseHistory, type BaselineLookup } from './history.ts'; import { computeQualityDelta, renderComment, renderJobSummary } from './model.ts'; const USAGE = `Usage: pnpm quality-delta [options] @@ -35,11 +35,11 @@ Options: `; /** - * The sticky-comment marker owned by scripts/size-report.mjs. The comment this command renders IS - * that comment, evolved: same marker, same update-in-place machinery, so a PR keeps exactly one. - * scripts/quality-delta/run.test.ts fails if the two ever drift apart. + * The sticky-comment marker owned by scripts/size-report.mjs, whose `--post-comment` machinery + * posts this body and updates it in place. One marker, one comment per PR, no second bot — size is + * now one row family inside it. scripts/quality-delta/run.test.ts fails if the two drift apart. */ -export const STICKY_COMMENT_MARKER = ''; +export const STICKY_COMMENT_MARKER = ''; function readJsonIfExists(filePath: string | undefined): T | null { if (filePath === undefined || !fs.existsSync(filePath)) return null; @@ -57,17 +57,22 @@ function shardPaths(historyDir: string): string[] { .map((name) => path.join(dir, name)); } +/** + * The baseline and whether it is the PR's own base commit. An inexact hit is kept (a near baseline + * states a real delta) but reported as such, so the comment can say the numbers may include + * main-side movement instead of attributing all of it to the PR. + */ function readBaselineFromHistory( historyDir: string, baseSha: string | null, -): RepoHealthSnapshot | null { - let newest: RepoHealthSnapshot | null = null; +): BaselineLookup | null { + let newest: BaselineLookup | null = null; for (const shard of shardPaths(historyDir)) { const { entries } = parseHistory(fs.readFileSync(shard, 'utf8')); const found = findBaseline(entries, baseSha); if (found === null) continue; - if (found.exact) return found.snapshot; - newest ??= found.snapshot; + if (found.exact) return found; + newest ??= found; } return newest; } @@ -88,6 +93,26 @@ function writeFile(filePath: string, contents: string): void { fs.writeFileSync(resolved, contents); } +/** The baseline to diff against: an explicitly passed snapshot, or the history's best hit. */ +function resolveBaseline( + baselinePath: string | undefined, + historyDir: string | undefined, + baseSha: string | null, +): BaselineLookup | null { + // An explicitly passed baseline is the caller's assertion that it IS the base. + const explicit = readJsonIfExists(baselinePath); + if (explicit !== null) return { snapshot: explicit, exact: true }; + if (historyDir === undefined) return null; + return readBaselineFromHistory(historyDir, baseSha); +} + +/** stdout always, the job summary when GitHub gave us one: it is the comment's overflow surface. */ +function emitSummary(summary: string): void { + process.stdout.write(summary); + const summaryPath = process.env.GITHUB_STEP_SUMMARY; + if (summaryPath) fs.appendFileSync(summaryPath, summary); +} + function run(argv: readonly string[]): number { const values = parseScriptArgs(argv, USAGE, { head: { type: 'string', default: '.tmp/repo-health.json' }, @@ -105,16 +130,16 @@ function run(argv: readonly string[]): number { `no repo-health snapshot at ${values.head}. Run \`pnpm repo-health --out\` first.`, ); } - const baseSha = values['base-sha'] ?? process.env.GITHUB_BASE_SHA ?? null; - const base = - readJsonIfExists(values.baseline) ?? - (values['history-dir'] === undefined - ? null - : readBaselineFromHistory(values['history-dir'], baseSha)); + const lookup = resolveBaseline( + values.baseline, + values['history-dir'], + values['base-sha'] ?? process.env.GITHUB_BASE_SHA ?? null, + ); const report = computeQualityDelta({ head, - base, + base: lookup?.snapshot ?? null, + baseExact: lookup?.exact ?? false, changedCoverage: readJsonIfExists(values['changed-coverage']), slowTest: readJsonIfExists(values['slow-test']), }); @@ -123,10 +148,7 @@ function run(argv: readonly string[]): number { const comment = `${STICKY_COMMENT_MARKER}\n${renderComment(report, context)}`; if (typeof values.markdown === 'string') writeFile(values.markdown, comment); - const summary = renderJobSummary(report, context); - process.stdout.write(summary); - const summaryPath = process.env.GITHUB_STEP_SUMMARY; - if (summaryPath) fs.appendFileSync(summaryPath, summary); + emitSummary(renderJobSummary(report, context)); return 0; } diff --git a/scripts/quality-delta/thresholds.ts b/scripts/quality-delta/thresholds.ts index 3aa960990..34ced6911 100644 --- a/scripts/quality-delta/thresholds.ts +++ b/scripts/quality-delta/thresholds.ts @@ -32,6 +32,12 @@ export type DeltaSources = { head: RepoHealthSnapshot; /** The main-branch baseline for `head`, read from the JSONL history. */ base: RepoHealthSnapshot | null; + /** + * Whether `base` is the PR's own base commit. False means the history had no entry for it and + * this is the nearest stored main snapshot, so the delta may include main-side changes the PR + * did not cause — a difference the comment states rather than hides. + */ + baseExact: boolean; /** This PR's changed-line coverage report (#1418), which has no main baseline by construction. */ changedCoverage: ChangedCoverageResult | null; /** Slow-test offenders recorded by the vitest reporter during the coverage run. */ diff --git a/scripts/size-report.mjs b/scripts/size-report.mjs index 42e3a522a..96191e12b 100644 --- a/scripts/size-report.mjs +++ b/scripts/size-report.mjs @@ -5,7 +5,12 @@ import { execFileSync } from 'node:child_process'; import { performance } from 'node:perf_hooks'; import { gzipSync } from 'node:zlib'; -const COMMENT_MARKER = ''; +// One sticky comment per PR for all repo-quality reporting (#1424): this started as the size +// report, which is now one section of the quality-delta comment, so the marker is deliberately not +// size-specific. The legacy marker stays recognized so comments posted before the rename are +// updated in place instead of being duplicated by a second bot comment. +const COMMENT_MARKER = ''; +const LEGACY_COMMENT_MARKERS = ['']; const VALUE_ARGS = new Map([ ['--cwd', 'cwd'], ['--json', 'json'], @@ -366,7 +371,10 @@ async function postGitHubComment(markdownPath, explicitPrNumber) { const body = fs.readFileSync(markdownPath, 'utf8'); const commentsUrl = buildCommentsUrl(config.repository, config.prNumber); const comments = await listGitHubComments(commentsUrl, config.headers); - const existing = comments.find((comment) => comment.body?.includes(COMMENT_MARKER)); + const markers = [COMMENT_MARKER, ...LEGACY_COMMENT_MARKERS]; + const existing = comments.find((comment) => + markers.some((marker) => comment.body?.includes(marker)), + ); await writeGitHubComment(commentsUrl, config.headers, body, existing?.url); } diff --git a/vitest.config.ts b/vitest.config.ts index 226ffe3ba..36cada0c9 100644 --- a/vitest.config.ts +++ b/vitest.config.ts @@ -62,6 +62,7 @@ export default defineConfig({ 'scripts/quality-delta/model.test.ts', 'scripts/quality-delta/history.test.ts', 'scripts/quality-delta/thresholds.test.ts', + 'scripts/quality-delta/producers.test.ts', // Parses CI configuration only, so this action guard needs no device or subprocess lane. 'test/ci/upload-agent-device-artifacts.test.ts', 'test/skillgym/suites/local-cli-help-policy.test.ts', From 90e4762a0e57624940d4ac162439cdbdff48936f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Pierzcha=C5=82a?= Date: Tue, 28 Jul 2026 17:42:44 +0000 Subject: [PATCH 5/9] obs: refuse to render a superseded head in the quality-delta comment Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com> --- .github/workflows/quality-delta.yml | 36 +++----- scripts/quality-delta/README.md | 12 ++- scripts/quality-delta/producers-run.ts | 116 +++++++++++++++++------- scripts/quality-delta/producers.test.ts | 31 ++++++- scripts/quality-delta/producers.ts | 26 ++++++ scripts/quality-delta/run.test.ts | 66 ++++++++++---- 6 files changed, 212 insertions(+), 75 deletions(-) diff --git a/.github/workflows/quality-delta.yml b/.github/workflows/quality-delta.yml index 3edaa19bc..3b0eeeb0a 100644 --- a/.github/workflows/quality-delta.yml +++ b/.github/workflows/quality-delta.yml @@ -23,8 +23,9 @@ permissions: actions: read pull-requests: write -# One render per head sha, and never two comment writes at once. Superseding is safe and wanted: a -# newer run describes a newer head. +# One render per head sha at a time. This cannot serialize ACROSS heads (the PR number is not known +# at trigger time), so the render gate below additionally refuses to post for a sha that is no longer +# the PR's head — that, not this group, is what keeps a late run from overwriting newer metrics. concurrency: group: quality-delta-${{ github.event.workflow_run.head_sha }} cancel-in-progress: true @@ -44,8 +45,10 @@ jobs: ref: ${{ github.event.workflow_run.head_sha }} fetch-depth: 0 - # The race policy, as a decision the unit tests own: scripts/quality-delta/producers.ts. - - name: Check that every producer finished + # The race policy, as a decision the unit tests own: scripts/quality-delta/producers.ts. It + # answers both halves of "may this run post" — every producer finished, AND this sha is still + # the open PR's head, so a superseded head renders nothing instead of overwriting the comment. + - name: Check that every producer finished and this head is current id: producers env: GH_TOKEN: ${{ github.token }} @@ -54,8 +57,9 @@ jobs: set -euo pipefail mkdir -p .tmp gh api "repos/${GITHUB_REPOSITORY}/actions/runs?head_sha=${HEAD_SHA}&per_page=100" > .tmp/runs.json + gh api "repos/${GITHUB_REPOSITORY}/commits/${HEAD_SHA}/pulls" > .tmp/pulls.json node --experimental-strip-types scripts/quality-delta/producers-run.ts \ - --runs .tmp/runs.json --head-sha "${HEAD_SHA}" + --runs .tmp/runs.json --pulls .tmp/pulls.json --head-sha "${HEAD_SHA}" - name: Setup toolchain if: steps.producers.outputs.ready == 'true' @@ -102,29 +106,15 @@ jobs: echo "No metrics/repo-health ref yet; the comment will report no baseline." fi - - name: Resolve the pull request and its base - if: steps.producers.outputs.ready == 'true' - id: pr - env: - GH_TOKEN: ${{ github.token }} - HEAD_SHA: ${{ github.event.workflow_run.head_sha }} - run: | - set -euo pipefail - gh api "repos/${GITHUB_REPOSITORY}/commits/${HEAD_SHA}/pulls" > .tmp/pulls.json - number=$(jq -r '[.[] | select(.state == "open") | .number] | first // empty' .tmp/pulls.json) - base=$(jq -r '[.[] | select(.state == "open") | .base.sha] | first // empty' .tmp/pulls.json) - echo "number=${number}" >> "$GITHUB_OUTPUT" - echo "base_sha=${base}" >> "$GITHUB_OUTPUT" - - name: Render quality delta - if: steps.producers.outputs.ready == 'true' && steps.pr.outputs.number != '' + if: steps.producers.outputs.ready == 'true' continue-on-error: true run: | pnpm repo-health --out .tmp/repo-health.json pnpm quality-delta \ --head .tmp/repo-health.json \ --history-dir .tmp/history-ref \ - --base-sha "${{ steps.pr.outputs.base_sha }}" \ + --base-sha "${{ steps.producers.outputs.pr_base_sha }}" \ --changed-coverage .tmp/changed-coverage.json \ --slow-test .tmp/slow-tests.json \ --markdown .tmp/quality-delta.md @@ -132,9 +122,9 @@ jobs: # ONE comment per PR: the marker machinery in scripts/size-report.mjs updates it in place, and # no other workflow posts a comment. - name: Comment on the PR - if: steps.producers.outputs.ready == 'true' && steps.pr.outputs.number != '' + if: steps.producers.outputs.ready == 'true' continue-on-error: true env: GITHUB_TOKEN: ${{ github.token }} - GITHUB_PR_NUMBER: ${{ steps.pr.outputs.number }} + GITHUB_PR_NUMBER: ${{ steps.producers.outputs.pr_number }} run: node scripts/size-report.mjs --post-comment .tmp/quality-delta.md diff --git a/scripts/quality-delta/README.md b/scripts/quality-delta/README.md index 8c44564ad..c571afca6 100644 --- a/scripts/quality-delta/README.md +++ b/scripts/quality-delta/README.md @@ -114,8 +114,16 @@ either one is a race whose loser is permanent: the comment is written once, so a lands a minute later never reaches it. So the render runs on `workflow_run` for both producers and [`producers.ts`](./producers.ts) decides readiness — the **last** producer to complete renders, and earlier firings exit cleanly because the later completion fires the trigger again. A re-run of one -producer supersedes its earlier run. `producers.test.ts` owns that policy, including the assertion -that no other workflow contains `--post-comment`. +producer supersedes its earlier run. + +The same gate answers the other half of "may this run post": the sha must still be the **current head +of an open PR** (`selectRenderTarget`). Workflow concurrency is keyed by head sha and therefore cannot +cancel across heads, so a producer run for an older head could otherwise complete late and replace the +comment with obsolete metrics. A superseded head renders nothing, and the PR number the comment is +posted to comes from that same verified lookup. + +`producers.test.ts` owns both policies, including the assertion that no other workflow contains +`--post-comment`. Fork PRs are skipped there deliberately: `workflow_run` carries a **write token in the base repo**, so checking out and installing fork code in it would be a pwn-request. Fork PRs keep the producers' diff --git a/scripts/quality-delta/producers-run.ts b/scripts/quality-delta/producers-run.ts index 177e878cb..06183faca 100644 --- a/scripts/quality-delta/producers-run.ts +++ b/scripts/quality-delta/producers-run.ts @@ -1,67 +1,119 @@ -// CLI shell for the producer-readiness decision (#1424): +// CLI shell for the render-readiness decision (#1424): // // gh api "repos/$REPO/actions/runs?head_sha=$SHA&per_page=100" > .tmp/runs.json -// pnpm quality-delta:producers --runs .tmp/runs.json --head-sha "$SHA" +// gh api "repos/$REPO/commits/$SHA/pulls" > .tmp/pulls.json +// pnpm quality-delta:producers --runs .tmp/runs.json --pulls .tmp/pulls.json --head-sha "$SHA" // -// Writes `ready`, `ci_run_id`, and `size_run_id` to $GITHUB_OUTPUT so the rendering workflow can -// skip cleanly while a producer is still running. Exit code stays 0 either way: "not yet" is a -// normal outcome, not a failure. +// Writes `ready`, `ci_run_id`, `size_run_id`, `pr_number`, and `pr_base_sha` to $GITHUB_OUTPUT. +// `ready` is false both while a producer is still running and when the sha has been SUPERSEDED (the +// PR's head moved on), so a late renderer cannot overwrite the sticky comment with older metrics. +// Exit code stays 0 either way: "not this run" is a normal outcome, not a failure. import fs from 'node:fs'; import { parseScriptArgs } from '../lib/cli-args.ts'; import { runAsMain } from '../lib/run-as-main.ts'; -import { producerReadiness, type ProducerReadiness, type WorkflowRunSummary } from './producers.ts'; +import { + producerReadiness, + selectRenderTarget, + type ProducerReadiness, + type PullRequestSummary, + type RenderTarget, + type WorkflowRunSummary, +} from './producers.ts'; -const USAGE = `Usage: pnpm quality-delta:producers --runs --head-sha +const USAGE = `Usage: pnpm quality-delta:producers --runs --pulls --head-sha Options: --runs JSON from GitHub's actions/runs API (its workflow_runs array). + --pulls JSON from GitHub's commits//pulls API. --head-sha The head commit the comment will describe. `; -/** The run ids the rendering steps need, so they can download from the right runs. */ -function writeStepOutputs(readiness: ProducerReadiness): void { +type RenderDecision = { + ready: boolean; + reason: string; + runIds: Readonly>; + target: RenderTarget | null; +}; + +/** An absent value is an EMPTY output, which the workflow's `!= ''` guards read as "do not use". */ +function optional(value: string | number | null | undefined): string { + return value === null || value === undefined ? '' : String(value); +} + +function outputLines(decision: RenderDecision): string[] { + return [ + `ready=${decision.ready}`, + `ci_run_id=${optional(decision.runIds.CI)}`, + `size_run_id=${optional(decision.runIds.Size)}`, + `pr_number=${optional(decision.target?.number)}`, + `pr_base_sha=${optional(decision.target?.baseSha)}`, + '', + ]; +} + +function writeStepOutputs(decision: RenderDecision): void { const outputPath = process.env.GITHUB_OUTPUT; if (!outputPath) return; - fs.appendFileSync( - outputPath, - [ - `ready=${readiness.ready}`, - `ci_run_id=${readiness.runIds.CI ?? ''}`, - `size_run_id=${readiness.runIds.Size ?? ''}`, - '', - ].join('\n'), - ); + fs.appendFileSync(outputPath, outputLines(decision).join('\n')); } -function requiredArgs(argv: readonly string[]): { runsPath: string; headSha: string } { +function requiredArgs(argv: readonly string[]): { + runsPath: string; + pullsPath: string; + headSha: string; +} { const values = parseScriptArgs(argv, USAGE, { runs: { type: 'string' }, + pulls: { type: 'string' }, 'head-sha': { type: 'string' }, }); const runsPath = values.runs; + const pullsPath = values.pulls; const headSha = values['head-sha']; - if (typeof runsPath !== 'string' || typeof headSha !== 'string') { - throw new Error('--runs and --head-sha are required'); + if ( + typeof runsPath !== 'string' || + typeof pullsPath !== 'string' || + typeof headSha !== 'string' + ) { + throw new Error('--runs, --pulls and --head-sha are required'); } - return { runsPath, headSha }; + return { runsPath, pullsPath, headSha }; +} + +function readJson(filePath: string): T { + return JSON.parse(fs.readFileSync(filePath, 'utf8')) as T; } function readRuns(runsPath: string): readonly WorkflowRunSummary[] { - const payload = JSON.parse(fs.readFileSync(runsPath, 'utf8')) as { - workflow_runs?: WorkflowRunSummary[]; - }; - return payload.workflow_runs ?? []; + return readJson<{ workflow_runs?: WorkflowRunSummary[] }>(runsPath).workflow_runs ?? []; } -function run(argv: readonly string[]): number { - const { runsPath, headSha } = requiredArgs(argv); +/** Why this run is not rendering, for the job log. Empty when it is. */ +function deferralReason( + readiness: ProducerReadiness, + target: RenderTarget | null, + headSha: string, +): string { + if (!readiness.ready) return readiness.reason; + if (target === null) return `${headSha.slice(0, 12)} is not the head of an open PR any more`; + return ''; +} + +/** Both halves of "may this run post": every producer finished, and the sha is still the PR's head. */ +function decide(argv: readonly string[]): RenderDecision { + const { runsPath, pullsPath, headSha } = requiredArgs(argv); const readiness = producerReadiness(readRuns(runsPath), headSha); - const verdict = readiness.ready - ? `every producer completed for ${headSha.slice(0, 12)}` - : readiness.reason; + const target = selectRenderTarget(readJson(pullsPath), headSha); + const reason = deferralReason(readiness, target, headSha); + return { ready: reason === '', reason, runIds: readiness.runIds, target }; +} + +function run(argv: readonly string[]): number { + const decision = decide(argv); + const verdict = decision.ready ? `rendering PR #${decision.target?.number}` : decision.reason; process.stdout.write(`quality-delta: ${verdict}.\n`); - writeStepOutputs(readiness); + writeStepOutputs(decision); return 0; } diff --git a/scripts/quality-delta/producers.test.ts b/scripts/quality-delta/producers.test.ts index e60777e2d..1940561c7 100644 --- a/scripts/quality-delta/producers.test.ts +++ b/scripts/quality-delta/producers.test.ts @@ -6,7 +6,13 @@ import { readFileSync } from 'node:fs'; import { expect, test } from 'vitest'; import { parse } from 'yaml'; -import { producerReadiness, PRODUCER_WORKFLOWS, type WorkflowRunSummary } from './producers.ts'; +import { + producerReadiness, + PRODUCER_WORKFLOWS, + selectRenderTarget, + type PullRequestSummary, + type WorkflowRunSummary, +} from './producers.ts'; const HEAD = 'a'.repeat(40); @@ -57,6 +63,24 @@ test('a re-run supersedes the earlier run of the same producer', () => { expect(readiness.runIds.Size).toBe(9); }); +test('a superseded head renders nothing, so a late run cannot overwrite newer metrics', () => { + const pull = (head: string, state = 'open'): PullRequestSummary => ({ + number: 1477, + state, + head: { sha: head }, + base: { sha: 'base'.repeat(10) }, + }); + + expect(selectRenderTarget([pull(HEAD)], HEAD)).toEqual({ + number: 1477, + baseSha: 'base'.repeat(10), + }); + // The PR's head moved on while this run was finishing: its metrics describe an old tree. + expect(selectRenderTarget([pull('b'.repeat(40))], HEAD)).toBe(null); + expect(selectRenderTarget([pull(HEAD, 'closed')], HEAD)).toBe(null); + expect(selectRenderTarget([], HEAD)).toBe(null); +}); + test('the rendering workflow triggers on both producers and gates every step on readiness', () => { const workflow = parse(readFileSync('.github/workflows/quality-delta.yml', 'utf8')) as { on: { workflow_run: { workflows: string[]; types: string[] } }; @@ -74,6 +98,11 @@ test('the rendering workflow triggers on both producers and gates every step on ); // Everything after the readiness check is gated on it; the checkout and the check itself are not. expect(gated).toHaveLength(job.steps.length - 2); + + // Posting uses the PR the gate verified is still on this head, never a second, ungated lookup. + const post = job.steps.find((step) => step.name === 'Comment on the PR')!; + expect(post.if).toContain("steps.producers.outputs.ready == 'true'"); + expect(JSON.stringify(post)).toContain('steps.producers.outputs.pr_number'); }); test('no other workflow posts a PR comment', () => { diff --git a/scripts/quality-delta/producers.ts b/scripts/quality-delta/producers.ts index 255342355..41235e62a 100644 --- a/scripts/quality-delta/producers.ts +++ b/scripts/quality-delta/producers.ts @@ -46,6 +46,32 @@ function newestRunFor( return matching[0] ?? null; } +/** The fields this decision needs from GitHub's `commits//pulls` payload. */ +export type PullRequestSummary = { + number: number; + state: string; + head: { sha: string }; + base: { sha: string }; +}; + +export type RenderTarget = { number: number; baseSha: string }; + +/** + * The PR this render may post to: open, and whose CURRENT head is still the sha being rendered. + * + * A producer run for an older head can complete (or be cancelled and re-reported) after a newer head + * already rendered; concurrency keyed by head sha does not cancel across heads. Without this check + * such a late renderer would overwrite the sticky comment with obsolete metrics, so a superseded head + * renders nothing and the comment keeps describing the head it actually measured. + */ +export function selectRenderTarget( + pulls: readonly PullRequestSummary[], + headSha: string, +): RenderTarget | null { + const current = pulls.find((pull) => pull.state === 'open' && pull.head.sha === headSha); + return current === undefined ? null : { number: current.number, baseSha: current.base.sha }; +} + export function producerReadiness( runs: readonly WorkflowRunSummary[], headSha: string, diff --git a/scripts/quality-delta/run.test.ts b/scripts/quality-delta/run.test.ts index 225deebc6..29aee5ecd 100644 --- a/scripts/quality-delta/run.test.ts +++ b/scripts/quality-delta/run.test.ts @@ -144,25 +144,57 @@ test('a missing head snapshot fails loudly, missing optional inputs do not', () expect(degraded.stdout).toContain('no main baseline'); }); -test('the producer gate reports readiness and run ids through $GITHUB_OUTPUT', () => { +test('the render gate reports readiness, run ids and the PR through $GITHUB_OUTPUT', () => { const dir = workdir(); const headSha = 'c'.repeat(40); - const runs = writeJson(dir, 'runs.json', { - workflow_runs: [ - { id: 11, name: 'CI', head_sha: headSha, status: 'completed' }, - { id: 22, name: 'Size', head_sha: headSha, status: 'in_progress' }, - ], - }); - const output = join(dir, 'github-output'); - writeFileSync(output, ''); - - const waiting = runScript('producers-run.ts', ['--runs', runs, '--head-sha', headSha], { - GITHUB_OUTPUT: output, - }); - // Not ready is a normal outcome: Size's completion fires the workflow again. - expect(waiting.status, waiting.stderr).toBe(0); - expect(readFileSync(output, 'utf8')).toContain('ready=false'); - expect(readFileSync(output, 'utf8')).toContain('size_run_id=22'); + const baseSha = 'd'.repeat(40); + const openPr = [{ number: 7, state: 'open', head: { sha: headSha }, base: { sha: baseSha } }]; + const pulls = writeJson(dir, 'pulls.json', openPr); + + function gate(name: string, runs: unknown, pullsPath: string): string { + const output = join(dir, `${name}-output`); + writeFileSync(output, ''); + const result = runScript( + 'producers-run.ts', + [ + '--runs', + writeJson(dir, `${name}-runs.json`, runs), + '--pulls', + pullsPath, + '--head-sha', + headSha, + ], + { GITHUB_OUTPUT: output }, + ); + // Not rendering is a normal outcome, never a failure. + expect(result.status, result.stderr).toBe(0); + return readFileSync(output, 'utf8'); + } + + const completed = [ + { id: 11, name: 'CI', head_sha: headSha, status: 'completed' }, + { id: 22, name: 'Size', head_sha: headSha, status: 'completed' }, + ]; + const ready = gate('ready', { workflow_runs: completed }, pulls); + expect(ready).toContain('ready=true'); + expect(ready).toContain('size_run_id=22'); + expect(ready).toContain('pr_number=7'); + expect(ready).toContain(`pr_base_sha=${baseSha}`); + + // Size still running: its completion fires the workflow again. + const waiting = gate( + 'waiting', + { workflow_runs: [completed[0], { ...completed[1], status: 'in_progress' }] }, + pulls, + ); + expect(waiting).toContain('ready=false'); + + // Superseded head: the PR moved on, so posting would replace newer metrics with older ones. + const moved = writeJson(dir, 'moved.json', [{ ...openPr[0], head: { sha: 'e'.repeat(40) } }]); + const superseded = gate('superseded', { workflow_runs: completed }, moved); + expect(superseded).toContain('ready=false'); + expect(superseded).toContain('pr_number='); + expect(superseded).not.toContain('pr_number=7'); }); test('appending to the history is idempotent per commit', () => { From bbaee7f5e0192601573d57aeea7cc4e209af7473 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Pierzcha=C5=82a?= Date: Tue, 28 Jul 2026 18:14:27 +0000 Subject: [PATCH 6/9] quality-delta: consume the merged artifact freshness status from #1423 Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com> --- scripts/quality-delta/README.md | 9 ++++++--- scripts/quality-delta/fixtures.ts | 8 ++++---- scripts/quality-delta/model.test.ts | 25 +++++++++++++++++++++++++ scripts/quality-delta/model.ts | 29 ++++++++++++++++++++++++----- 4 files changed, 59 insertions(+), 12 deletions(-) diff --git a/scripts/quality-delta/README.md b/scripts/quality-delta/README.md index c571afca6..040d79596 100644 --- a/scripts/quality-delta/README.md +++ b/scripts/quality-delta/README.md @@ -54,9 +54,12 @@ every row plus the notes for inputs that were unavailable. amendment). If the baseline's `schemaVersion` differs from head's, every baseline-derived row is dropped and the comment says so; PR-local rows still show. To resume diffing, add a migration note under "Schema migrations" below and bump the reader accordingly. -- **Stale inputs are labelled, not laundered.** repo-health flags a read artifact (coverage, size) - as `stale` when a source file is newer than it. Rows derived from a stale artifact render as - `JS gzip (stale artifact)` rather than silently claiming to describe the head commit. +- **Stale inputs are labelled, not laundered.** repo-health proves a read artifact's freshness + (`provenance.inputs.*.status`) only from a producing commit the artifact stamps in its own bytes. + A row whose artifact is `stale` renders as `JS gzip (stale artifact)` rather than silently claiming + to describe the head commit. `unknown` — today's honest answer for every artifact, since no + producer stamps a commit — gets one job-summary note instead of a caveat on every size and coverage + row, which is the comment fatigue this comment exists to remove. - **Missing inputs cost rows, not runs.** No baseline, no size artifact, no coverage JSON — each costs the rows it would have produced plus a job-summary note. Fork PRs (read-only token, no secrets) therefore degrade to job-summary output: the comment step is skipped, never failed. diff --git a/scripts/quality-delta/fixtures.ts b/scripts/quality-delta/fixtures.ts index fa44d7a8c..977ecfa41 100644 --- a/scripts/quality-delta/fixtures.ts +++ b/scripts/quality-delta/fixtures.ts @@ -38,14 +38,14 @@ export function snapshotFixture(overrides: SnapshotOverrides = {}): RepoHealthSn coverageSummary: { path: 'coverage/coverage-summary.json', sha256: 'cccccccccccc', - producerInputs: ['src', 'test', 'vitest.config.ts'], - stale: false, + producerCommit: commit, + status: 'fresh', }, sizeReport: { path: '.tmp/size-report.json', sha256: 'ssssssssssss', - producerInputs: ['src', 'package.json'], - stale: overrides.sizeStale ?? false, + producerCommit: overrides.sizeStale === true ? 'f'.repeat(40) : commit, + status: overrides.sizeStale === true ? 'stale' : 'fresh', }, }, }, diff --git a/scripts/quality-delta/model.test.ts b/scripts/quality-delta/model.test.ts index 16e2eedf3..a881f54b3 100644 --- a/scripts/quality-delta/model.test.ts +++ b/scripts/quality-delta/model.test.ts @@ -20,6 +20,10 @@ function commentOf(overrides: Partial = {}): string { return renderComment(computeQualityDelta(sources(overrides)), CONTEXT); } +function summaryOf(overrides: Partial = {}): string { + return renderJobSummary(computeQualityDelta(sources(overrides)), CONTEXT); +} + test('an unchanged tree collapses the whole comment to one line', () => { expect(commentOf()).toBe(`${NO_DELTAS_LINE}\n`); }); @@ -57,6 +61,27 @@ test('a metric read from a stale artifact is marked rather than silently attribu expect(commentOf({ head })).toContain('| JS gzip (stale artifact) |'); }); +test('an unproven (`unknown`) artifact caveats the job summary, not every row', () => { + // #1423 reports `unknown` for every artifact whose producer stamps no commit, i.e. all of them + // today; marking each row would caveat size and coverage forever. + const head = snapshotFixture({ jsGzipBytes: 900_000 }); + const sizeReport = head.provenance.inputs.sizeReport!; + const unknown = { + ...head, + provenance: { + ...head.provenance, + inputs: { + ...head.provenance.inputs, + sizeReport: { ...sizeReport, producerCommit: null, status: 'unknown' as const }, + }, + }, + }; + const comment = commentOf({ head: unknown }); + expect(comment).toContain('| JS gzip |'); + expect(comment).not.toContain('stale artifact'); + expect(summaryOf({ head: unknown })).toContain('freshness as `unknown`'); +}); + test('PR-local coverage rows are scored against a bound, not a baseline', () => { // 95% is comfortably above the gate threshold + margin, so it earns no row. expect(commentOf({ changedCoverage: changedCoverageFixture() })).toBe(`${NO_DELTAS_LINE}\n`); diff --git a/scripts/quality-delta/model.ts b/scripts/quality-delta/model.ts index dbfc0f44e..5ffd40c63 100644 --- a/scripts/quality-delta/model.ts +++ b/scripts/quality-delta/model.ts @@ -21,7 +21,7 @@ // // All I/O — reading files, resolving links, posting the comment — belongs to run.ts. -import type { RepoHealthSnapshot } from '../repo-health/model.ts'; +import type { ArtifactStatus, RepoHealthSnapshot } from '../repo-health/model.ts'; import { MAX_COMMENT_LINES, QUALITY_DELTA_METRICS, @@ -75,11 +75,21 @@ function formatSigned(value: number, format: MetricFormat): string { return `${sign}${formatValue(Math.abs(value), format)}`; } -function staleInputFlag(snapshot: RepoHealthSnapshot, spec: MetricSpec): boolean { +function artifactStatus(snapshot: RepoHealthSnapshot, spec: MetricSpec): ArtifactStatus | null { const inputs = snapshot.provenance.inputs; - if (spec.staleInput === 'sizeReport') return inputs.sizeReport?.stale === true; - if (spec.staleInput === 'coverageSummary') return inputs.coverageSummary?.stale === true; - return false; + if (spec.staleInput === 'sizeReport') return inputs.sizeReport?.status ?? null; + if (spec.staleInput === 'coverageSummary') return inputs.coverageSummary?.status ?? null; + return null; +} + +/** + * Only a PROVEN stale artifact marks its row. #1423 reports `unknown` whenever the producer stamps + * no commit, which is every artifact today, so marking that too would put a caveat on every size and + * coverage row forever — the comment fatigue this issue exists to remove. `unknown` gets one job + * summary note instead. + */ +function staleInputFlag(snapshot: RepoHealthSnapshot, spec: MetricSpec): boolean { + return artifactStatus(snapshot, spec) === 'stale'; } function deltaRow(spec: MetricSpec, sources: DeltaSources): QualityDeltaRow | null { @@ -155,6 +165,15 @@ function missingInputNotes(sources: DeltaSources): string[] { 'Slow-test report unavailable (the vitest reporter writes it during the coverage run).', ); } + const unverified = QUALITY_DELTA_METRICS.filter( + (spec) => artifactStatus(sources.head, spec) === 'unknown', + ); + if (unverified.length > 0) { + notes.push( + 'Size and coverage are read artifacts whose producers stamp no commit, so repo-health reports ' + + 'their freshness as `unknown` (#1423): treat those rows as not provably current with the head tree.', + ); + } return notes; } From 8b24768807298697eb1582361460e0fb28fc8fff Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Pierzcha=C5=82a?= Date: Tue, 28 Jul 2026 18:52:44 +0000 Subject: [PATCH 7/9] quality-delta: re-verify the PR head inside the comment write path Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com> --- .github/workflows/quality-delta.yml | 8 ++- scripts/quality-delta/README.md | 10 ++++ scripts/quality-delta/producers.test.ts | 2 + scripts/quality-delta/run.test.ts | 65 ++++++++++++++++++++++++- scripts/size-report.mjs | 45 +++++++++++++++-- 5 files changed, 124 insertions(+), 6 deletions(-) diff --git a/.github/workflows/quality-delta.yml b/.github/workflows/quality-delta.yml index 3b0eeeb0a..62a0f7169 100644 --- a/.github/workflows/quality-delta.yml +++ b/.github/workflows/quality-delta.yml @@ -120,11 +120,15 @@ jobs: --markdown .tmp/quality-delta.md # ONE comment per PR: the marker machinery in scripts/size-report.mjs updates it in place, and - # no other workflow posts a comment. + # no other workflow posts a comment. `--expect-head` re-reads the PR head inside the write path + # and refuses the write if it moved during setup/download/render — the gate above cannot cover + # that window, and sha-scoped concurrency does not cancel this run when a newer head starts. - name: Comment on the PR if: steps.producers.outputs.ready == 'true' continue-on-error: true env: GITHUB_TOKEN: ${{ github.token }} GITHUB_PR_NUMBER: ${{ steps.producers.outputs.pr_number }} - run: node scripts/size-report.mjs --post-comment .tmp/quality-delta.md + run: | + node scripts/size-report.mjs --post-comment .tmp/quality-delta.md \ + --expect-head "${{ github.event.workflow_run.head_sha }}" diff --git a/scripts/quality-delta/README.md b/scripts/quality-delta/README.md index 040d79596..3eb59794d 100644 --- a/scripts/quality-delta/README.md +++ b/scripts/quality-delta/README.md @@ -125,6 +125,16 @@ cancel across heads, so a producer run for an older head could otherwise complet comment with obsolete metrics. A superseded head renders nothing, and the PR number the comment is posted to comes from that same verified lookup. +A push can also land *after* that gate, while setup, artifact download, and rendering run. So the +write path re-reads the head itself and **fails closed**: + +``` +node scripts/size-report.mjs --post-comment .tmp/quality-delta.md --expect-head +``` + +If the PR's head is no longer that commit — or the lookup does not answer — nothing is written and +the existing comment keeps its newer metrics. + `producers.test.ts` owns both policies, including the assertion that no other workflow contains `--post-comment`. diff --git a/scripts/quality-delta/producers.test.ts b/scripts/quality-delta/producers.test.ts index 1940561c7..e4b656198 100644 --- a/scripts/quality-delta/producers.test.ts +++ b/scripts/quality-delta/producers.test.ts @@ -103,6 +103,8 @@ test('the rendering workflow triggers on both producers and gates every step on const post = job.steps.find((step) => step.name === 'Comment on the PR')!; expect(post.if).toContain("steps.producers.outputs.ready == 'true'"); expect(JSON.stringify(post)).toContain('steps.producers.outputs.pr_number'); + // …and the write path re-verifies the head itself, covering the gap between gate and write. + expect(JSON.stringify(post)).toContain('--expect-head'); }); test('no other workflow posts a PR comment', () => { diff --git a/scripts/quality-delta/run.test.ts b/scripts/quality-delta/run.test.ts index 29aee5ecd..b36be5520 100644 --- a/scripts/quality-delta/run.test.ts +++ b/scripts/quality-delta/run.test.ts @@ -2,8 +2,10 @@ // exercised through argument handling and the files they write), which is why this file lives in // the `subprocess-stub` vitest project rather than `unit-core` — see #1412 coordination rule 4. -import { spawnSync } from 'node:child_process'; +import { spawn, spawnSync } from 'node:child_process'; import { existsSync, mkdirSync, mkdtempSync, readFileSync, writeFileSync } from 'node:fs'; +import { createServer } from 'node:http'; +import type { AddressInfo } from 'node:net'; import { tmpdir } from 'node:os'; import { join } from 'node:path'; import { expect, test } from 'vitest'; @@ -43,6 +45,67 @@ test('the sticky-comment marker stays the one scripts/size-report.mjs posts', () expect(sizeReport).toContain("LEGACY_COMMENT_MARKERS = ['']"); }); +/** + * Post through a stand-in GitHub API that reports `currentHead` and records every call, so an + * attempted write cannot pass unnoticed. The child is spawned asynchronously on purpose: a blocking + * spawn would stall the event loop this stub server answers on. + */ +async function postCommentAgainst( + currentHead: string, + expectHead: string, +): Promise<{ calls: string[]; code: number | null; stdout: string }> { + const calls: string[] = []; + const server = createServer((request, response) => { + calls.push(`${request.method} ${request.url}`); + response.writeHead(200, { 'content-type': 'application/json' }); + response.end(request.url?.includes('/pulls/') ? `{"head":{"sha":"${currentHead}"}}` : '[]'); + }); + await new Promise((resolve) => server.listen(0, '127.0.0.1', resolve)); + const markdown = join(workdir(), 'comment.md'); + writeFileSync(markdown, `${STICKY_COMMENT_MARKER}\nQuality delta\n`); + + const child = spawn( + process.execPath, + [ + 'scripts/size-report.mjs', + '--post-comment', + markdown, + '--pr', + '7', + '--expect-head', + expectHead, + ], + { + env: { + ...process.env, + GITHUB_API_URL: `http://127.0.0.1:${(server.address() as AddressInfo).port}`, + GITHUB_TOKEN: 'stub-token', + GITHUB_REPOSITORY: 'callstack/agent-device', + }, + }, + ); + let stdout = ''; + child.stdout.on('data', (chunk: Buffer) => (stdout += chunk.toString())); + const code = await new Promise((resolve) => child.on('close', resolve)); + await new Promise((resolve) => server.close(() => resolve())); + return { calls, code, stdout }; +} + +test('a head that advances before the write refuses the comment instead of overwriting it', async () => { + const rendered = 'a'.repeat(40); + + // The gate passed minutes ago; the PR was pushed to while this run built the report. + const moved = await postCommentAgainst('b'.repeat(40), rendered); + expect(moved.code).toBe(0); + expect(moved.stdout).toContain('Refusing to comment'); + expect(moved.calls.some((call) => /^(POST|PATCH)/.test(call))).toBe(false); + + // Same head: the write happens exactly as before. + const current = await postCommentAgainst(rendered, rendered); + expect(current.code).toBe(0); + expect(current.calls).toContain('POST /repos/callstack/agent-device/issues/7/comments'); +}); + test('the CLI writes a marker-prefixed comment and prints the full summary', () => { const dir = workdir(); const head = writeJson( diff --git a/scripts/size-report.mjs b/scripts/size-report.mjs index 96191e12b..aa4d0baae 100644 --- a/scripts/size-report.mjs +++ b/scripts/size-report.mjs @@ -18,6 +18,7 @@ const VALUE_ARGS = new Map([ ['--compare', 'compare'], ['--post-comment', 'postComment'], ['--pr', 'pr'], + ['--expect-head', 'expectHead'], ['--startup-runs', 'startupRuns'], ]); @@ -30,7 +31,7 @@ const args = parseArgs(process.argv.slice(2)); const cwd = path.resolve(args.cwd ?? process.cwd()); if (args.postComment) { - await postGitHubComment(args.postComment, args.pr); + await postGitHubComment(args.postComment, args.pr, args.expectHead); process.exit(0); } @@ -84,6 +85,7 @@ Options: --startup-runs Measure startup medians for side-effect-free CLI commands. --post-comment Post or update the markdown report on the current PR. --pr Pull request number for --post-comment. + --expect-head Refuse to write unless the PR's head is still this commit. `); process.exit(0); } @@ -366,8 +368,40 @@ function writeFile(filePath, contents) { fs.writeFileSync(filePath, contents); } -async function postGitHubComment(markdownPath, explicitPrNumber) { +/** + * Re-read the PR's head immediately before writing, and refuse the write unless it is still the + * commit this report describes. Any doubt — a moved head, or a lookup that did not answer — refuses: + * the comment keeps whatever newer metrics are already there. The producer gate checks the same + * thing minutes earlier, but a push during setup, artifact download, and rendering lands inside that + * window, and workflow concurrency is sha-scoped so it cannot cancel the older run. + */ +async function fetchCurrentHead(repository, prNumber, headers) { + const [owner, repo] = repository.split('/'); + const response = await fetch(`${apiBaseUrl()}/repos/${owner}/${repo}/pulls/${prNumber}`, { + headers, + }); + if (!response.ok) return `unreadable (HTTP ${response.status})`; + return (await response.json())?.head?.sha ?? 'unknown'; +} + +async function headIsStill(repository, prNumber, expectedHead, headers) { + const currentHead = await fetchCurrentHead(repository, prNumber, headers); + if (currentHead === expectedHead) return true; + process.stdout.write( + `Refusing to comment: PR #${prNumber} head is ${currentHead}, not the ${expectedHead} ` + + 'this report describes.\n', + ); + return false; +} + +async function postGitHubComment(markdownPath, explicitPrNumber, expectedHead) { const config = readGitHubCommentConfig(explicitPrNumber); + if ( + expectedHead && + !(await headIsStill(config.repository, config.prNumber, expectedHead, config.headers)) + ) { + return; + } const body = fs.readFileSync(markdownPath, 'utf8'); const commentsUrl = buildCommentsUrl(config.repository, config.prNumber); const comments = await listGitHubComments(commentsUrl, config.headers); @@ -409,9 +443,14 @@ function buildGitHubHeaders(token) { }; } +/** Actions sets GITHUB_API_URL; it is also the seam the comment tests point at a local stub. */ +function apiBaseUrl() { + return process.env.GITHUB_API_URL ?? 'https://api.github.com'; +} + function buildCommentsUrl(repository, prNumber) { const [owner, repo] = repository.split('/'); - return `https://api.github.com/repos/${owner}/${repo}/issues/${prNumber}/comments`; + return `${apiBaseUrl()}/repos/${owner}/${repo}/issues/${prNumber}/comments`; } async function listGitHubComments(commentsUrl, headers) { From b33911b303b9d44dee1d719128cf1a885d87a776 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Pierzcha=C5=82a?= Date: Tue, 28 Jul 2026 19:00:01 +0000 Subject: [PATCH 8/9] quality-delta: post through the pnpm size script after main's workflow refactor Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com> --- .github/workflows/quality-delta.yml | 2 +- .github/workflows/size.yml | 6 +++--- scripts/quality-delta/README.md | 2 +- 3 files changed, 5 insertions(+), 5 deletions(-) diff --git a/.github/workflows/quality-delta.yml b/.github/workflows/quality-delta.yml index 62a0f7169..a81757ad6 100644 --- a/.github/workflows/quality-delta.yml +++ b/.github/workflows/quality-delta.yml @@ -130,5 +130,5 @@ jobs: GITHUB_TOKEN: ${{ github.token }} GITHUB_PR_NUMBER: ${{ steps.producers.outputs.pr_number }} run: | - node scripts/size-report.mjs --post-comment .tmp/quality-delta.md \ + pnpm size --post-comment .tmp/quality-delta.md \ --expect-head "${{ github.event.workflow_run.head_sha }}" diff --git a/.github/workflows/size.yml b/.github/workflows/size.yml index a7f1438e5..91620c7ef 100644 --- a/.github/workflows/size.yml +++ b/.github/workflows/size.yml @@ -80,9 +80,9 @@ jobs: run: cat .tmp/size-report.md >> "$GITHUB_STEP_SUMMARY" # The size table itself is job-summary output now: the sticky PR comment it used to post is - # the quality-delta comment (#1424), rendered from the repo-health snapshot in the CI - # workflow's Coverage job under the SAME marker, so a PR still carries exactly one comment. - # Size rows reach it through this artifact rather than a second base/head measurement. + # the quality-delta comment (#1424), rendered by quality-delta.yml under the SAME marker once + # this workflow and CI have both finished, so a PR still carries exactly one comment. Size rows + # reach it through this artifact rather than a second base/head measurement. - name: Upload size report JSON uses: actions/upload-artifact@ea165f8d65b6e75b540449e92b4886f43607fa02 # v4.6.2 with: diff --git a/scripts/quality-delta/README.md b/scripts/quality-delta/README.md index 3eb59794d..6635f5038 100644 --- a/scripts/quality-delta/README.md +++ b/scripts/quality-delta/README.md @@ -129,7 +129,7 @@ A push can also land *after* that gate, while setup, artifact download, and rend write path re-reads the head itself and **fails closed**: ``` -node scripts/size-report.mjs --post-comment .tmp/quality-delta.md --expect-head +pnpm size --post-comment .tmp/quality-delta.md --expect-head ``` If the PR's head is no longer that commit — or the lookup does not answer — nothing is written and From 74e41a37a87dc60e34786427f9ba094707830644 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Pierzcha=C5=82a?= Date: Tue, 28 Jul 2026 19:45:34 +0000 Subject: [PATCH 9/9] quality-delta: serialize comment writes per pull request, not per head sha Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com> --- .github/workflows/quality-delta.yml | 10 ++++++---- scripts/quality-delta/README.md | 25 +++++++++++++++++-------- scripts/quality-delta/producers.test.ts | 7 +++++++ 3 files changed, 30 insertions(+), 12 deletions(-) diff --git a/.github/workflows/quality-delta.yml b/.github/workflows/quality-delta.yml index a81757ad6..96324aab8 100644 --- a/.github/workflows/quality-delta.yml +++ b/.github/workflows/quality-delta.yml @@ -23,11 +23,13 @@ permissions: actions: read pull-requests: write -# One render per head sha at a time. This cannot serialize ACROSS heads (the PR number is not known -# at trigger time), so the render gate below additionally refuses to post for a sha that is no longer -# the PR's head — that, not this group, is what keeps a late run from overwriting newer metrics. +# Serialized by PULL REQUEST, so a renderer for an older head is cancelled the moment the newer +# head's renderer starts — a sha-keyed group could not do that, and the loser could still be inside +# its comment write. `pull_requests` is empty for fork triggers (skipped below anyway), hence the +# sha fallback. The `--expect-head` check in the write path stays as defense in depth for the +# cancellation window itself. concurrency: - group: quality-delta-${{ github.event.workflow_run.head_sha }} + group: quality-delta-${{ github.event.workflow_run.pull_requests[0].number || github.event.workflow_run.head_sha }} cancel-in-progress: true jobs: diff --git a/scripts/quality-delta/README.md b/scripts/quality-delta/README.md index 6635f5038..1cd38ffe6 100644 --- a/scripts/quality-delta/README.md +++ b/scripts/quality-delta/README.md @@ -119,14 +119,23 @@ lands a minute later never reaches it. So the render runs on `workflow_run` for earlier firings exit cleanly because the later completion fires the trigger again. A re-run of one producer supersedes its earlier run. -The same gate answers the other half of "may this run post": the sha must still be the **current head -of an open PR** (`selectRenderTarget`). Workflow concurrency is keyed by head sha and therefore cannot -cancel across heads, so a producer run for an older head could otherwise complete late and replace the -comment with obsolete metrics. A superseded head renders nothing, and the PR number the comment is -posted to comes from that same verified lookup. - -A push can also land *after* that gate, while setup, artifact download, and rendering run. So the -write path re-reads the head itself and **fails closed**: +Writing the comment is serialized **per pull request**, not per head sha: + +```yaml +concurrency: + group: quality-delta-${{ github.event.workflow_run.pull_requests[0].number || github.event.workflow_run.head_sha }} + cancel-in-progress: true +``` + +so the renderer for an older head is cancelled the moment the newer head's renderer starts, even if +it is already inside its comment write. (`pull_requests` is empty for fork triggers, which are +skipped anyway — hence the sha fallback.) + +Two checks back that up. The gate answers the other half of "may this run post": the sha must still be +the **current head of an open PR** (`selectRenderTarget`), so a superseded head renders nothing and +the PR number posted to comes from that same verified lookup. And because a push can land *after* the +gate, while setup, artifact download, and rendering run, the write path re-reads the head itself and +**fails closed**: ``` pnpm size --post-comment .tmp/quality-delta.md --expect-head diff --git a/scripts/quality-delta/producers.test.ts b/scripts/quality-delta/producers.test.ts index e4b656198..7afdce300 100644 --- a/scripts/quality-delta/producers.test.ts +++ b/scripts/quality-delta/producers.test.ts @@ -84,11 +84,18 @@ test('a superseded head renders nothing, so a late run cannot overwrite newer me test('the rendering workflow triggers on both producers and gates every step on readiness', () => { const workflow = parse(readFileSync('.github/workflows/quality-delta.yml', 'utf8')) as { on: { workflow_run: { workflows: string[]; types: string[] } }; + concurrency: { group: string; 'cancel-in-progress': boolean }; jobs: Record; }; expect(workflow.on.workflow_run.workflows.toSorted()).toEqual([...PRODUCER_WORKFLOWS].toSorted()); expect(workflow.on.workflow_run.types).toEqual(['completed']); + // Serialized per PR (sha fallback for fork triggers), so a newer head cancels an older renderer + // even while it is mid-write; a sha-keyed group could not. + expect(workflow.concurrency.group).toContain('workflow_run.pull_requests[0].number'); + expect(workflow.concurrency.group).toContain('workflow_run.head_sha'); + expect(workflow.concurrency['cancel-in-progress']).toBe(true); + const job = workflow.jobs.comment!; // Fork PRs must not be checked out by a write-token workflow, and get job summaries instead. expect(job.if).toContain('head_repository.full_name == github.repository');