From 12bf6c0da3eabf051879ce5b7fb3d01e3ff64c79 Mon Sep 17 00:00:00 2001 From: Maurice Date: Thu, 8 Oct 2026 16:31:32 +0200 Subject: [PATCH] ci(server): fail the Postgres probe on an unmeasured baseline The probe shipped with scripts/pg-probe-baseline.json at `failing: null`, and an unmeasured baseline only reported. Every repository statement Postgres refused, new SQLite-only SQL included, still passed the job; only the dialect helper cases could fail it, so the ratchet was off until some later commit seeded the file. An unmeasured baseline now fails the run outside --update, and the log, step summary and annotation say which artifact file to commit. The job stays red until the first measurement lands, so the gate cannot reach dev holding nothing. A run that called no repository method (an unreadable tsconfig, a moved repositories directory) fails as well, also under --update, so a broken plan can neither pass nor seed an empty baseline. --- .github/workflows/test.yml | 8 ++-- CLAUDE.md | 2 +- server/CLAUDE.md | 2 +- server/scripts/pg-probe.ts | 26 +++++++++--- server/scripts/pg-probe/baseline.ts | 7 ++-- server/scripts/pg-probe/report.ts | 41 ++++++++++++++++--- .../unit/scripts/pg-probe/baseline.test.ts | 5 ++- .../unit/scripts/pg-probe/report.test.ts | 36 ++++++++++++++-- 8 files changed, 102 insertions(+), 25 deletions(-) diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index 6c9dfbd71..ba13cea66 100644 --- a/.github/workflows/test.yml +++ b/.github/workflows/test.yml @@ -530,9 +530,11 @@ jobs: # on SQLite and Postgres (every case must pass), and calls every public # repository method once, each statement in its own rolled-back transaction. # The statements Postgres refuses are held per method to - # server/scripts/pg-probe-baseline.json, which may only shrink. The measured - # baseline and the full report are uploaded either way, so a change that - # fixes a statement can lower the baseline without a local Postgres. + # server/scripts/pg-probe-baseline.json, which may only shrink; an unmeasured + # baseline (failing: null) or a run that called no method fails. The measured + # baseline and the full report are uploaded either way, so the first run's + # artifact seeds the file and a change that fixes a statement can lower it + # without a local Postgres. postgres-probe: name: Postgres Probe runs-on: ubuntu-24.04 diff --git a/CLAUDE.md b/CLAUDE.md index 744ad18c9..b29fb048d 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -119,7 +119,7 @@ These principles come out of a full-repo audit and shape all new code: ## Quality gates in CI -`.github/workflows/test.yml` runs, per package: typecheck (server also `typecheck:tests`), lint (client through `lint:warnings`, which also holds the warning count per rule), `lint:pages`, `lint:rtl`, `lint:size` (client and server), the server's `lint:boundaries`, the client's `lint:format`, `lint:layers`, `lint:offline`, `lint:skips` and `lint:i18n-keys`, shared's `contracts:open`, `check:plugin-facts`, `i18n:parity:strict` (with the untranslated-strings ratchet), the S3 contract suite, the Postgres probe (`postgres-probe`: every dialect helper on SQLite and Postgres, and every repository statement against a Postgres service, held to a baseline that only shrinks; see `server/CLAUDE.md`), the browser end-to-end flows (Chromium and WebKit), a client build with `check:gl-split`, coverage for all four packages, then a SonarCloud scan. It also builds the Docker image and boots it until `/api/health/ready` answers (`docker-image`), and lints and renders the Helm chart against the Kubernetes schemas and runs `docker compose config` on every compose file (`deploy-manifests`). `lint-prettier.yml` additionally runs `lint` + `format:check` on `shared/`. `plugin-sdk` has no lint script and is outside the eslint gates. **Not in CI**: `theme:lint` and the screenshot and help-media Playwright projects; run them locally. There are no git hooks; nothing runs before a commit except you. Other workflows build Docker images, scan them, and publish `trek-plugin-sdk` on `plugin-sdk-v*` tags. +`.github/workflows/test.yml` runs, per package: typecheck (server also `typecheck:tests`), lint (client through `lint:warnings`, which also holds the warning count per rule), `lint:pages`, `lint:rtl`, `lint:size` (client and server), the server's `lint:boundaries`, the client's `lint:format`, `lint:layers`, `lint:offline`, `lint:skips` and `lint:i18n-keys`, shared's `contracts:open`, `check:plugin-facts`, `i18n:parity:strict` (with the untranslated-strings ratchet), the S3 contract suite, the Postgres probe (`postgres-probe`: every dialect helper on SQLite and Postgres, and every repository statement against a Postgres service, held to a baseline that only shrinks and fails while it is still unmeasured; see `server/CLAUDE.md`), the browser end-to-end flows (Chromium and WebKit), a client build with `check:gl-split`, coverage for all four packages, then a SonarCloud scan. It also builds the Docker image and boots it until `/api/health/ready` answers (`docker-image`), and lints and renders the Helm chart against the Kubernetes schemas and runs `docker compose config` on every compose file (`deploy-manifests`). `lint-prettier.yml` additionally runs `lint` + `format:check` on `shared/`. `plugin-sdk` has no lint script and is outside the eslint gates. **Not in CI**: `theme:lint` and the screenshot and help-media Playwright projects; run them locally. There are no git hooks; nothing runs before a commit except you. Other workflows build Docker images, scan them, and publish `trek-plugin-sdk` on `plugin-sdk-v*` tags. - **`ci-ok` is the required check.** The branch ruleset requires `ci-ok` (plus `lint` and `check-target` from their own workflows), and `ci-ok` needs every job above except the scan. It fails if any of them failed or was cancelled and passes if they passed or were skipped. A new gate job counts only once it is listed in `ci-ok`'s `needs:`. - **What runs is decided by `changes`, not by `on.paths`**: `scripts/ci/changed-areas.mjs` maps the changed files to the `code`, `image` and `deploy` areas (tests: `npm run test:ci-scripts`). A path filter on the workflow would leave `ci-ok` unreported on a docs-only PR and block it. Add a new top-level file the build reads to the right area there. diff --git a/server/CLAUDE.md b/server/CLAUDE.md index 7300ef553..6c3cfb501 100644 --- a/server/CLAUDE.md +++ b/server/CLAUDE.md @@ -38,7 +38,7 @@ Single test: `npx vitest run tests/unit/nest/weather.controller.test.ts`, or `np **`lint:dialect`** (`scripts/dialect-lint.mjs`) reads every string and template literal under `src/` (comments do not count) and every `.insertId` read, and counts the SQLite-only spellings per file and rule: `INSERT OR IGNORE`/`REPLACE`, `datetime()`/`julianday()`/`unixepoch()` (also as the bare name handed to `eb.fn`), `strftime`, `json_extract` and its siblings, `GLOB`, `last_insert_rowid`, `AUTOINCREMENT`, a `PRAGMA` statement outside the `src/db/*.ts` lifecycle files and the migrations, and `||` concatenation in SQL. The counts are held to `scripts/dialect-baseline.json`; a new hit fails, and so does an entry above what its file holds now, so the change that removes one runs `npm run lint:dialect -- --update` (it never raises or adds an entry). The dialect layer `src/db/dialect/` is not scanned, and neither are the migrations up to `FROZEN_MIGRATIONS_THROUGH` in the script; a newer migration is held to the DML rules, while its DDL stays engine specific. -**`probe:pg`** (`scripts/pg-probe.ts`, CI job `postgres-probe`) measures what of the SQL runs on Postgres. Given an empty database (`TREK_PG_PROBE_URL`, or `--url=`; it refuses one holding tables it did not create) it derives a Postgres schema from the entities with MikroORM's schema generator (timestamps stay TEXT, SQLite-only defaults and `COLLATE NOCASE` get their Postgres spelling, no foreign keys), runs every dialect helper on an in-memory SQLite and on Postgres and checks both against the same expectation (`scripts/pg-probe/helper-cases.ts`; every case must pass), then calls every public method of every class in `src/db/repositories/*.ts` once. The arguments come from the parameter types, read with the TypeScript checker (`pg-probe/arg-samples.ts`), and the calls go through a MikroORM Postgres driver that runs each statement alone in a rolled-back transaction with a statement timeout and records whether Postgres accepted it (`pg-probe/recorder.ts`), so every statement meets empty tables and nothing persists. The distinct statements Postgres refuses are held per `Class.method` to `scripts/pg-probe-baseline.json`: a method may fail no more than its entry, and an entry above what it fails now fails too. The job uploads the measured baseline and a JSON report (`pg-probe-baseline` artifact), so the change that fixes a statement copies the lowered baseline from there or runs `npm run probe:pg -- --update` against a local Postgres (`docker run --rm -p 5432:5432 -e POSTGRES_PASSWORD=probe postgres:17`); `--update` never raises or adds an entry. While the baseline's `failing` is `null` it has never been measured: the run reports and passes, and the artifact holds the first measurement to commit. The unit tests under `tests/unit/scripts/pg-probe/` cover the probe's logic, including every helper case on SQLite. +**`probe:pg`** (`scripts/pg-probe.ts`, CI job `postgres-probe`) measures what of the SQL runs on Postgres. Given an empty database (`TREK_PG_PROBE_URL`, or `--url=`; it refuses one holding tables it did not create) it derives a Postgres schema from the entities with MikroORM's schema generator (timestamps stay TEXT, SQLite-only defaults and `COLLATE NOCASE` get their Postgres spelling, no foreign keys), runs every dialect helper on an in-memory SQLite and on Postgres and checks both against the same expectation (`scripts/pg-probe/helper-cases.ts`; every case must pass), then calls every public method of every class in `src/db/repositories/*.ts` once. The arguments come from the parameter types, read with the TypeScript checker (`pg-probe/arg-samples.ts`), and the calls go through a MikroORM Postgres driver that runs each statement alone in a rolled-back transaction with a statement timeout and records whether Postgres accepted it (`pg-probe/recorder.ts`), so every statement meets empty tables and nothing persists. The distinct statements Postgres refuses are held per `Class.method` to `scripts/pg-probe-baseline.json`: a method may fail no more than its entry, and an entry above what it fails now fails too. The job uploads the measured baseline and a JSON report (`pg-probe-baseline` artifact), so the change that fixes a statement copies the lowered baseline from there or runs `npm run probe:pg -- --update` against a local Postgres (`docker run --rm -p 5432:5432 -e POSTGRES_PASSWORD=probe postgres:17`); `--update` never raises or adds an entry. While the baseline's `failing` is `null` it has never been measured, and the run fails (outside `--update`) until the first measurement from the artifact is committed, so the gate cannot land on `dev` holding nothing. A run that called no repository method fails too, so a broken plan can neither pass nor seed an empty baseline. The unit tests under `tests/unit/scripts/pg-probe/` cover the probe's logic, including every helper case on SQLite. ## Architecture invariants diff --git a/server/scripts/pg-probe.ts b/server/scripts/pg-probe.ts index f522d171e..df26a210c 100644 --- a/server/scripts/pg-probe.ts +++ b/server/scripts/pg-probe.ts @@ -29,6 +29,11 @@ * --url= instead of TREK_PG_PROBE_URL * --update lower scripts/pg-probe-baseline.json to this run (never raises or adds an entry; * an unmeasured baseline gets its first measurement) + * + * A baseline whose `failing` is `null` has never been measured and fails the + * run (outside `--update`); the baseline the run writes with + * `--next-baseline` (the CI artifact) is the first measurement to commit. A + * run that called no repository method fails as well. * --next-baseline= write the baseline this run would leave (the CI artifact) * --report= write everything the run saw as JSON * --summary= append the Markdown summary (defaults to $GITHUB_STEP_SUMMARY) @@ -45,7 +50,7 @@ import { compareWithBaseline, formatBaseline, lowerBaseline, readBaseline } from import { connectProbeOrm, connectSetupOrm, postgresHelperEngine, prepareDatabase, probeRepositories, sqliteHelperEngine } from './pg-probe/engines'; import { runHelperCases, type HelperResult } from './pg-probe/helper-cases'; import { StatementRecorder } from './pg-probe/recorder'; -import { formatConsole, formatMarkdown, passed, summarize, type MethodResult, type ReportInput } from './pg-probe/report'; +import { formatConsole, formatMarkdown, passed, probedNothing, summarize, type MethodResult, type ReportInput } from './pg-probe/report'; const SERVER_ROOT = path.join(__dirname, '..'); export const BASELINE_PATH = path.join(SERVER_ROOT, 'scripts', 'pg-probe-baseline.json'); @@ -96,14 +101,16 @@ export function repositoryFiles(dir: string): string[] { } /** - * 0 when every helper case passed and no method fails more statements than + * 0 when every helper case passed, the probe called at least one repository + * method, the baseline is measured and no method fails more statements than * its entry allows. An entry above what its method fails now fails the run - * too, unless this run is the `--update` that lowers it. + * too, and so does an unmeasured baseline, unless this run is the `--update` + * that lowers or seeds it. */ -export function exitCode(input: Pick, update: boolean): number { +export function exitCode(input: Pick, update: boolean): number { if (passed(input)) return 0; const helpersOk = input.helpers.every((result) => result.failure === null); - return helpersOk && input.verdict.grown.length === 0 && update ? 0 : 1; + return helpersOk && !probedNothing(input.summary) && input.verdict.grown.length === 0 && update ? 0 : 1; } /** GitHub annotations when running in Actions, plain lines otherwise. */ @@ -186,7 +193,14 @@ async function main(): Promise { 'Lower it (--update, or the pg-probe-baseline artifact).', ); } - if (verdict.unseeded) annotate('notice', 'pg-probe: the baseline has not been measured yet; the pg-probe-baseline artifact holds the first one.'); + if (probedNothing(summary)) annotate('error', `pg-probe: no repository method was called (${summary.methods} planned).`); + if (verdict.unseeded) { + annotate( + 'error', + 'pg-probe: the baseline has not been measured yet. Commit pg-probe-baseline.json from the pg-probe-baseline artifact ' + + 'as server/scripts/pg-probe-baseline.json.', + ); + } for (const failure of schemaFailures) annotate('warning', `pg-probe schema: ${failure.message}`); return exitCode(input, args.update); } diff --git a/server/scripts/pg-probe/baseline.ts b/server/scripts/pg-probe/baseline.ts index 78cefd029..24844a05f 100644 --- a/server/scripts/pg-probe/baseline.ts +++ b/server/scripts/pg-probe/baseline.ts @@ -10,8 +10,9 @@ * `--update` lowers entries and drops the ones that reach zero or whose * method is gone; it never raises or adds one. The only exception is the * first run: a baseline whose `failing` is `null` has never been measured, - * so the check reports and passes, and `--update` (or the CI artifact) - * writes the first measurement. + * so there is nothing to compare and the run fails (scripts/pg-probe.ts) + * until the first measurement, written by `--update` or taken from the CI + * artifact, is committed. */ import { readFileSync } from 'node:fs'; @@ -22,7 +23,7 @@ export interface ProbeBaseline { } export interface BaselineVerdict { - /** The baseline has never been measured: report only. */ + /** The baseline has never been measured: nothing to compare, and the run fails until it is seeded. */ unseeded: boolean; /** Methods failing more statements than their entry allows. */ grown: { method: string; allowed: number; now: number }[]; diff --git a/server/scripts/pg-probe/report.ts b/server/scripts/pg-probe/report.ts index 5f4054cc6..6f1f61bdc 100644 --- a/server/scripts/pg-probe/report.ts +++ b/server/scripts/pg-probe/report.ts @@ -101,9 +101,28 @@ export function helperFailures(helpers: readonly HelperResult[]): HelperResult[] return helpers.filter((result) => result.failure !== null); } -/** True when the run passes: every helper case on both engines, and the ratchet. */ -export function passed(input: Pick): boolean { - return helperFailures(input.helpers).length === 0 && input.verdict.grown.length === 0 && input.verdict.stale.length === 0; +/** + * True when the probe called no repository method at all. A broken plan (an + * unreadable tsconfig, a moved repositories directory) would otherwise pass + * with nothing failing, and its empty measurement could seed the baseline. + */ +export function probedNothing(summary: Pick): boolean { + return summary.called === 0; +} + +/** + * True when the run passes: every helper case on both engines, at least one + * repository method called, a measured baseline, and the ratchet held. An + * unmeasured baseline fails, so the gate cannot merge before it holds anything. + */ +export function passed(input: Pick): boolean { + return ( + helperFailures(input.helpers).length === 0 && + !probedNothing(input.summary) && + !input.verdict.unseeded && + input.verdict.grown.length === 0 && + input.verdict.stale.length === 0 + ); } export function formatConsole(input: ReportInput): string { @@ -128,8 +147,14 @@ export function formatConsole(input: ReportInput): string { lines.push(` ${failed.method} ${failed.code} ${failed.message}`); lines.push(` ${oneLine(failed.sql)}`); } + if (probedNothing(summary)) { + lines.push(`FAIL No repository method was called (${summary.methods} planned); there is nothing to hold to the baseline.`); + } if (verdict.unseeded) { - lines.push('Baseline: not measured yet (failing is null). This run passes and writes the first measurement as the next baseline.'); + lines.push( + 'FAIL Baseline: not measured yet (failing is null). Commit the next baseline this run wrote ' + + '(the pg-probe-baseline artifact in CI) as scripts/pg-probe-baseline.json.', + ); } for (const entry of verdict.grown) { lines.push(`FAIL ${entry.method} sends ${entry.now} statement(s) Postgres refuses, ${entry.allowed} allowed.`); @@ -153,7 +178,13 @@ export function formatMarkdown(input: ReportInput): string { out.push(`| Distinct statements | ${summary.statements} |`); out.push(`| Refused by Postgres | ${summary.failedStatements.length} |`); out.push(''); - if (verdict.unseeded) out.push('The baseline has not been measured yet; the `pg-probe-baseline` artifact holds the first one.', ''); + if (probedNothing(summary)) out.push('No repository method was called, so there is nothing to hold to the baseline.', ''); + if (verdict.unseeded) { + out.push( + 'The baseline has not been measured yet, which fails the run. Commit `pg-probe-baseline.json` from the `pg-probe-baseline` artifact as `server/scripts/pg-probe-baseline.json`.', + '', + ); + } if (verdict.grown.length > 0 || verdict.stale.length > 0) { out.push('### Ratchet', '', '| Method | Allowed | Now |', '|---|---|---|'); for (const entry of [...verdict.grown, ...verdict.stale]) out.push(`| ${cell(entry.method)} | ${entry.allowed} | ${entry.now} |`); diff --git a/server/tests/unit/scripts/pg-probe/baseline.test.ts b/server/tests/unit/scripts/pg-probe/baseline.test.ts index 167a4f389..34598e2a6 100644 --- a/server/tests/unit/scripts/pg-probe/baseline.test.ts +++ b/server/tests/unit/scripts/pg-probe/baseline.test.ts @@ -3,7 +3,8 @@ * itself only runs in CI against a Postgres service, so the rules that decide * whether that job passes are pinned here: a method may not fail more * statements than its entry, a stale entry fails until it is lowered, an - * unmeasured baseline only reports, and --update never raises or adds. + * unmeasured baseline has nothing to compare (the run fails on it, see + * report.test.ts), and --update never raises or adds. */ import { readFileSync } from 'node:fs'; import path from 'node:path'; @@ -28,7 +29,7 @@ describe('pg-probe baseline', () => { expect(() => parseBaseline('{"failing": {"A.b": 1.5}}', 'b.json')).toThrow(/expected a positive integer/); }); - it('PGPROBE-003: an unmeasured baseline only reports', () => { + it('PGPROBE-003: an unmeasured baseline is flagged instead of compared', () => { expect(compareWithBaseline({ failing: null }, now({ 'A.b': 3 }))).toEqual({ unseeded: true, grown: [], stale: [] }); }); diff --git a/server/tests/unit/scripts/pg-probe/report.test.ts b/server/tests/unit/scripts/pg-probe/report.test.ts index 875088c84..4422092fc 100644 --- a/server/tests/unit/scripts/pg-probe/report.test.ts +++ b/server/tests/unit/scripts/pg-probe/report.test.ts @@ -11,7 +11,7 @@ import { describe, expect, it } from 'vitest'; import { exitCode, parseArgs, repositoryFiles } from '../../../../scripts/pg-probe'; import type { BaselineVerdict } from '../../../../scripts/pg-probe/baseline'; import type { HelperResult } from '../../../../scripts/pg-probe/helper-cases'; -import { formatConsole, formatMarkdown, oneLine, passed, summarize, type MethodResult, type ReportInput } from '../../../../scripts/pg-probe/report'; +import { formatConsole, formatMarkdown, oneLine, passed, probedNothing, summarize, type MethodResult, type ReportInput } from '../../../../scripts/pg-probe/report'; const ok = (sql: string) => ({ sql, outcome: { ok: true as const } }); const refused = (sql: string, code = '42883', message = 'function datetime(unknown) does not exist') => ({ @@ -48,7 +48,28 @@ describe('pg-probe report', () => { expect(passed(input({ helpers: [helper('expected 1, got 2')] }))).toBe(false); expect(passed(input({ verdict: { ...CLEAN, grown: [{ method: 'B.broken', allowed: 1, now: 2 }] } }))).toBe(false); expect(passed(input({ verdict: { ...CLEAN, stale: [{ method: 'B.broken', allowed: 3, now: 2 }] } }))).toBe(false); - expect(passed(input({ verdict: { unseeded: true, grown: [], stale: [] } }))).toBe(true); + }); + + it('PGPROBE-066: an unmeasured baseline fails the run, and only --update (which seeds it) passes', () => { + const unseeded = input({ verdict: { unseeded: true, grown: [], stale: [] } }); + expect(passed(unseeded)).toBe(false); + expect(exitCode(unseeded, false)).toBe(1); + expect(exitCode(unseeded, true)).toBe(0); + expect(exitCode(input({ verdict: { unseeded: true, grown: [], stale: [] }, helpers: [helper('threw: boom')] }), true)).toBe(1); + }); + + it('PGPROBE-067: a run that called no repository method fails, also under --update', () => { + const skippedOnly = RESULTS.filter((result) => result.unprobeable !== undefined); + for (const results of [[], skippedOnly]) { + const summary = summarize(results); + expect(probedNothing(summary)).toBe(true); + const empty = input({ summary, results }); + expect(passed(empty)).toBe(false); + expect(exitCode(empty, false)).toBe(1); + expect(exitCode(empty, true)).toBe(1); + expect(exitCode(input({ summary, results, verdict: { unseeded: true, grown: [], stale: [] } }), true)).toBe(1); + } + expect(probedNothing(summarize(RESULTS))).toBe(false); }); it('PGPROBE-062: --update forgives a stale entry but never a grown one or a failing helper', () => { @@ -75,7 +96,11 @@ describe('pg-probe report', () => { expect(text).toContain('FAIL B.broken sends 2 statement(s) Postgres refuses, 1 allowed.'); expect(text).toContain('FAIL Z.gone is held at 1 failing statement(s) but fails 0 now; lower the baseline.'); expect(text.trim().endsWith('Postgres probe failed.')).toBe(true); - expect(formatConsole(input({ verdict: { unseeded: true, grown: [], stale: [] } }))).toContain('Baseline: not measured yet'); + const unseeded = formatConsole(input({ verdict: { unseeded: true, grown: [], stale: [] } })); + expect(unseeded).toContain('FAIL Baseline: not measured yet'); + expect(unseeded).toContain('as scripts/pg-probe-baseline.json'); + expect(unseeded.trim().endsWith('Postgres probe failed.')).toBe(true); + expect(formatConsole(input({ summary: summarize([]), results: [] }))).toContain('FAIL No repository method was called (0 planned)'); }); it('PGPROBE-064: the step summary is a Markdown table with escaped cells', () => { @@ -85,7 +110,10 @@ describe('pg-probe report', () => { expect(md).toContain('| postgres | a\\|b | got \'x\' |'); expect(md).toContain('| 42883 | 1 |'); expect(md).toContain('| E.skipped | fn: a function parameter |'); - expect(formatMarkdown(input({ verdict: { unseeded: true, grown: [], stale: [] } }))).toContain('has not been measured yet'); + const unseeded = formatMarkdown(input({ verdict: { unseeded: true, grown: [], stale: [] } })); + expect(unseeded).toContain('Result: **failed**'); + expect(unseeded).toContain('has not been measured yet, which fails the run'); + expect(formatMarkdown(input({ summary: summarize([]), results: [] }))).toContain('No repository method was called'); }); it('PGPROBE-065: flattens and cuts SQL for a log line', () => {