test(preview): fail a stranded /moderator probe in the local suite, not only in a job nobody reads

#3573 migrated the moderator surfaces to the standalone app and four preview specs kept probing
paths that now 302 off-origin. `preview / smoke-tests` went red and STAYED red on every PR based
after it, and because that job is report-only nothing stopped: three PRs merged through it in the
four hours before someone looked, and an agent nearly attributed a genuine `main` breakage to their
own PR because it arrived inside an already-red set. #4179 fixed the four failures.

This is the recurrence guard. It scans the preview specs for `/moderator/*` literals and fails when
one has migrated or resolves to no page, so the next migration reddens the machine of whoever
performs it — `test:unit:run` is on the before-committing list — as one named test carrying the path
and the fix, rather than as browser assertions in a job whose red is ambient.

Be precise about what that buys, because the obvious reading is wrong: it does NOT make the fact
blocking. The `unit` job is `continue-on-error: true` and `main` has no required status checks, so
this is report-only too. What changes is where and how the failure appears.

The three specs that assert the redirect on purpose carry an inline `@migrated-route-probe` marker,
checked in both directions — a path that comes BACK to this app strands the assertion the same way.
A marked line may hold only one probe, or the marker would excuse the others silently.

Two properties the scan needs and did not get for free: the positive control asserts the ENFORCED
partition rather than the total, because `it.each([])` registers zero tests and exits 0 (measured),
so marking every line would empty the guard with nothing to show for it; and the capture stops
before `?`, since `/moderator/reports?status=Pending` is the natural shape of a queue probe and the
exact route the incident was about.

Documentation this turned up as stale: the convention-guard list named four of seven, `test:lint-
rules` is invoked by no workflow (those guards run because they match the `unit` project), the root
test-command list omitted the packages and apps suites entirely, and the SvelteKit standard had no
testing section at all despite all three apps having one.

The `unit` job's flip-to-blocking note now carries what was measured today rather than leaving it to
be rediscovered: 8 of 39 recent runs had a red `Unit tests` STEP across 8 distinct branches, which
reads as flake but was trunk-red from a ledger test failing on every PR (fixed by #4191); 5 of 5
green after it. Plus the two traps — the run-level `conclusion` says success while the step under it
failed, and this workflow is `pull_request`-only, so a trunk-red test shows up as every PR reddening
at once.

Refs 868kubuz6

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This commit is contained in:
briant
2026-08-20 13:07:22 -06:00
parent baf9a54b99
commit 6e61e1821c
6 changed files with 258 additions and 23 deletions
+18 -1
View File
@@ -265,6 +265,21 @@ jobs:
# #
# FLIP TO BLOCKING once a couple of weeks of runs show a clean pass rate. # FLIP TO BLOCKING once a couple of weeks of runs show a clean pass rate.
# Remove `continue-on-error` and this comment together. # Remove `continue-on-error` and this comment together.
#
# WHERE THAT STANDS, measured 2026-08-20 so the next reader does not repeat the
# archaeology. Over the 39 most recent completed runs the `Unit tests` STEP was red
# in 8, across 8 DISTINCT branches — which reads as flake but was not: every one
# predates #4191, which fixed a ledger test that had been TRUNK red and therefore
# failed on every PR regardless of its contents. Of the runs started after that
# merge, 5 of 5 are green across 4 branches. Encouraging, and far too short a
# window to act on — 36 minutes is not "a couple of weeks".
#
# Two things to know before re-measuring. The run-level `conclusion` is USELESS
# here: `continue-on-error` makes it `success` while the step underneath is
# `failure`, so read `steps[].conclusion` from the jobs endpoint, not the run.
# And this workflow is `pull_request`-only, so the suite never runs against `main`
# itself — a trunk-red test shows up as every PR going red at once, which is the
# signature to look for and exactly what the 8 above turned out to be.
continue-on-error: true continue-on-error: true
timeout-minutes: 30 timeout-minutes: 30
steps: steps:
@@ -380,7 +395,9 @@ jobs:
# Next module graph, ~7s locally. As there, RED here does not block a merge — `main` has # Next module graph, ~7s locally. As there, RED here does not block a merge — `main` has
# no required_status_checks — it is a signal someone has to look at and dismiss. # no required_status_checks — it is a signal someone has to look at and dismiss.
# #
# `apps/moderator` is absent on purpose: it has no vitest config, so nothing selects it. # Selection is by vitest config, so an app without one is silently not covered here. Every app
# under `apps/` has one today; scripts/ci/assert-workspace-suites-ran.mjs is what notices if that
# stops being true.
timeout-minutes: 15 timeout-minutes: 15
steps: steps:
- uses: actions/checkout@v4 - uses: actions/checkout@v4
+23 -6
View File
@@ -111,13 +111,22 @@ CI already scopes itself this way and gates only on **added** files for exactly
### Testing ### Testing
```bash ```bash
pnpm run test:unit:run # Vitest unit suite (the one you almost always want) pnpm run test:unit:run # Vitest unit suite over src/ + scripts/ (the one you almost always want)
pnpm run test:component # Vitest component suite (browser mode — see Git Worktrees for NixOS) pnpm run test:packages:run # Vitest, the packages/* suites
pnpm run test:lint-rules # Convention guards (see below) pnpm run test:apps:run # Vitest, the apps/* suites
pnpm test # Playwright e2e pnpm run test:component # Vitest component suite (browser mode — see Git Worktrees for NixOS)
pnpm run test:ui # Playwright with UI pnpm run test:lint-rules # Convention guards (see below)
pnpm test # Playwright e2e
pnpm run test:ui # Playwright with UI
``` ```
These are **separate suites over disjoint directories**, not layers of one — `test:unit:run` does not
run a single test under `packages/` or `apps/`, because the `unit` project's `include` is root-relative.
Which of them CI runs, and which of those can actually fail a check, differs per suite: see the job
comments in `.github/workflows/lint.yml`. `main` has no required status checks, so no suite blocks a
merge; the strongest a red one gets is rendering red for a human to notice, and a job marked
`continue-on-error` does not even do that.
The vitest suites are projects in `vitest.config.mts`. The unit suite is **two** projects — The vitest suites are projects in `vitest.config.mts`. The unit suite is **two** projects —
`unit` and `unit-native` — so select it as **`--project 'unit*'`**, never `--project unit`. `unit` and `unit-native` — so select it as **`--project 'unit*'`**, never `--project unit`.
@@ -163,7 +172,15 @@ Use a top-level `import type * as PromClient` — an inline `typeof import('...'
#### Convention guards run as tests #### Convention guards run as tests
Several repo conventions are enforced by tests, not by eslint — `pnpm run test:lint-rules` runs all of them: Several repo conventions are enforced by tests, not by eslint — `pnpm run test:lint-rules` runs all of them:
`no-wholesale-module-mock` (the `importOriginal` rule above), `no-io-in-transaction`, `no-module-scope-cache`, `no-unloadable-image-fixture`. They live in `src/server/services/__tests__/`. If one fails, fix the code — don't add an exemption without saying why. `no-wholesale-module-mock` (the `importOriginal` rule above), `no-io-in-transaction`, `no-module-scope-cache`,
`no-server-infra-in-app-graph`, `no-stale-moderator-route-probe`, `no-unbounded-paging-fake`,
`no-unloadable-image-fixture`. They live in `src/server/services/__tests__/`. If one fails, fix the code — don't
add an exemption without saying why. **Add a new guard to the `test:lint-rules` script when you write one**, or it
is discoverable only by reading the directory.
`test:lint-rules` is a convenience selector, not the enforcement point: these files match the `unit` project's
`include`, so they already run in `pnpm run test:unit:run` and in CI's `Unit tests` job. No workflow invokes
`test:lint-rules` itself.
#### A passing test says nothing about how it FAILS — check the revert #### A passing test says nothing about how it FAILS — check the revert
**"The tests would catch a regression here" is a claim about the failure mode, not about coverage.** A green **"The tests would catch a regression here" is a claim about the failure mode, not about coverage.** A green
+1 -1
View File
@@ -100,7 +100,7 @@
"test:packages:run": "vitest run --project '@civitai/*'", "test:packages:run": "vitest run --project '@civitai/*'",
"test:apps": "vitest --project 'app:*'", "test:apps": "vitest --project 'app:*'",
"test:apps:run": "vitest run --project 'app:*'", "test:apps:run": "vitest run --project 'app:*'",
"test:lint-rules": "vitest run --project 'unit*' src/server/services/__tests__/no-io-in-transaction.test.ts src/server/services/__tests__/no-module-scope-cache.test.ts src/server/services/__tests__/no-server-infra-in-app-graph.test.ts src/server/services/__tests__/no-unbounded-paging-fake.test.ts src/server/services/__tests__/no-unloadable-image-fixture.test.ts src/server/services/__tests__/no-wholesale-module-mock.test.ts", "test:lint-rules": "vitest run --project 'unit*' src/server/services/__tests__/no-io-in-transaction.test.ts src/server/services/__tests__/no-module-scope-cache.test.ts src/server/services/__tests__/no-server-infra-in-app-graph.test.ts src/server/services/__tests__/no-stale-moderator-route-probe.test.ts src/server/services/__tests__/no-unbounded-paging-fake.test.ts src/server/services/__tests__/no-unloadable-image-fixture.test.ts src/server/services/__tests__/no-wholesale-module-mock.test.ts",
"test:component": "vitest run --project component", "test:component": "vitest run --project component",
"test:component:watch": "vitest --project component", "test:component:watch": "vitest --project component",
"meilisearch:migrate": "NODE_ENV=development tsx scripts/oneoffs/meilisearch-migration.ts", "meilisearch:migrate": "NODE_ENV=development tsx scripts/oneoffs/meilisearch-migration.ts",
@@ -0,0 +1,199 @@
import { describe, expect, it } from 'vitest';
import { readdirSync, existsSync, readFileSync, statSync } from 'fs';
import path from 'path';
import { MIGRATED_ROUTES, migratedRouteKey } from '~/shared/constants/migrated-moderator-routes';
/**
* 🔴 SOURCE GATE — a preview smoke spec may not probe a `/moderator/*` path this app no longer serves.
*
* Redundant-looking, since the specs are tests already. It is not: they only run in
* `preview / smoke-tests`, which is report-only, so #3573 left them failing on every PR for hours
* with three merges going through. This fails on the machine of whoever performs the next migration.
*
* Route EXISTENCE only — a page that still resolves but stopped rendering its anchor is not covered,
* and a path assembled from variables is invisible, so keep writing them literally.
*/
const REPO_ROOT = path.resolve(__dirname, '../../../..');
const TESTS_DIR = path.join(REPO_ROOT, 'tests');
const MODERATOR_PAGES = path.join(REPO_ROOT, 'src/pages/moderator');
/**
* Wider than `playwright.preview.config.ts`'s `testMatch` on purpose: it also takes the `preview-*.ts`
* helpers every spec imports, which are the natural home for a shared path constant.
*/
const PREVIEW_FILE = /^preview-.*\.ts$/;
/**
* Opt-out for a probe that targets a migrated path on purpose. Must sit on the same line as the
* literal, or it drifts off the thing it excuses.
*/
const DELIBERATE = '@migrated-route-probe';
/** Newlines preserved, or every reported line number is wrong. */
const stripBlockComments = (s: string) =>
s.replace(/\/\*[\s\S]*?\*\//g, (block) => block.replace(/[^\n]/g, ''));
/**
* Not anchored to a quote, so `${PREVIEW_URL}/moderator/x` is seen; capture stops before `?`, so a
* query string does not hide the whole literal.
*/
const PROBE = /\/moderator\/([a-zA-Z0-9\-_/]+)/g;
/** Drop a `//` line comment WITHOUT eating the `//` in `http://`. Same idiom as the writer ledger. */
const stripLineComment = (line: string) => line.replace(/(^|[^:])\/\/.*$/, '$1');
type Probe = { file: string; line: number; probePath: string; deliberate: boolean };
function previewFiles(dir: string, out: string[] = []): string[] {
for (const entry of readdirSync(dir, { withFileTypes: true })) {
const full = path.join(dir, entry.name);
if (entry.isDirectory()) previewFiles(full, out);
else if (PREVIEW_FILE.test(entry.name)) out.push(full);
}
return out.sort();
}
/** Lines carrying the marker alongside more than one probe — the marker cannot say which it means. */
const overloadedMarkers: string[] = [];
function probesIn(file: string): Probe[] {
const found: Probe[] = [];
const rel = path.relative(REPO_ROOT, file).split(path.sep).join('/');
stripBlockComments(readFileSync(file, 'utf8'))
.split('\n')
.forEach((line, index) => {
const marked = line.includes(DELIBERATE);
const matches = [...stripLineComment(line).matchAll(PROBE)];
if (marked && matches.length > 1) overloadedMarkers.push(`${rel}:${index + 1}`);
for (const match of matches) {
found.push({
file: rel,
line: index + 1,
probePath: `/moderator/${match[1]}`,
deliberate: marked && matches.length === 1,
});
}
});
return found;
}
/**
* `[...slug].tsx` — the migration catchall — deliberately does not count as a page: answering every
* path is what makes a stale probe look alive. A single dynamic segment (`[id].tsx`) does.
*/
function pageExists(sub: string): boolean {
const segments = sub.split('/').filter(Boolean);
if (!segments.length) return false;
const dynamicIn = (dir: string, wantDir: boolean) => {
if (!existsSync(dir)) return undefined;
return readdirSync(dir).find((entry) => {
const name = entry.replace(/\.tsx?$/, '');
if (!name.startsWith('[') || name.startsWith('[...') || name.startsWith('[[')) return false;
const isDir = statSync(path.join(dir, entry)).isDirectory();
return wantDir ? isDir : !isDir && /\.tsx?$/.test(entry);
});
};
let dir = MODERATOR_PAGES;
for (const [index, segment] of segments.entries()) {
if (index === segments.length - 1) {
const base = path.join(dir, segment);
if (['.tsx', '.ts'].some((ext) => existsSync(base + ext))) return true;
if (
existsSync(base) &&
statSync(base).isDirectory() &&
['index.tsx', 'index.ts'].some((f) => existsSync(path.join(base, f)))
)
return true;
return dynamicIn(dir, false) !== undefined;
}
const next = path.join(dir, segment);
if (existsSync(next) && statSync(next).isDirectory()) {
dir = next;
continue;
}
const dyn = dynamicIn(dir, true);
if (!dyn) return false;
dir = path.join(dir, dyn);
}
return false;
}
const files = previewFiles(TESTS_DIR);
const probes = files.flatMap(probesIn);
const enforced = probes.filter((p) => !p.deliberate);
const deliberate = probes.filter((p) => p.deliberate);
describe('preview smoke specs probe only moderator routes this app still serves', () => {
/**
* 🔴 Assert the ENFORCED partition, not the total: `it.each([])` registers zero tests and exits 0
* (measured), so marking every line would empty both blocks below with nothing to show for it.
*/
it('actually scanned the preview files, and something is still being enforced', () => {
expect(files.map((f) => path.basename(f))).toEqual(
expect.arrayContaining([
'preview-moderation.spec.ts',
'preview-auth-guard.spec.ts',
'preview-auth.setup.ts',
])
);
expect(enforced.length).toBeGreaterThan(0);
});
// Pinned rather than counted: a marker appearing is the event that should need a reviewer.
it('has exactly the deliberate migrated-route probes it is supposed to have', () => {
expect(deliberate.map((p) => `${p.file}:${p.line} ${p.probePath}`).sort()).toEqual([
'tests/preview-auth-guard.spec.ts:23 /moderator/reports',
'tests/preview-moderation.spec.ts:98 /moderator/reports',
'tests/preview-moderation.spec.ts:99 /moderator/images',
]);
});
it('never lets one marker excuse several paths on a line', () => {
expect(
overloadedMarkers,
`A \`${DELIBERATE}\` line holds more than one /moderator path, so the marker cannot say which ` +
'it excuses — and it would excuse all of them. Put the marked literal on its own line.'
).toEqual([]);
});
it.each(enforced)(
'$file:$line $probePath has not migrated to the moderator app',
({ probePath }) => {
const sub = probePath.slice('/moderator/'.length);
const key = migratedRouteKey(sub);
expect(
key,
`This path now 302s to ${
MIGRATED_ROUTES[key ?? '']
} on the standalone moderator app, so the probe measures a redirect rather than a page. ` +
'Repoint it at a route that stayed, or mark the line ' +
`\`${DELIBERATE}\` if asserting the hop IS the point.`
).toBeUndefined();
}
);
it.each(enforced)('$file:$line $probePath resolves to a page in this app', ({ probePath }) => {
const sub = probePath.slice('/moderator/'.length);
expect(
pageExists(sub),
'No page under src/pages/moderator/ serves this path, so the catchall answers it. A probe ' +
'against a deleted page fails in preview smoke, where nothing blocks on it.'
).toBe(true);
});
it.each(deliberate)(
'$file:$line $probePath is marked deliberate and really has migrated',
({ probePath }) => {
// The marker rots too: a path that comes BACK leaves a spec asserting a redirect that is gone.
const sub = probePath.slice('/moderator/'.length);
expect(
migratedRouteKey(sub),
`Marked \`${DELIBERATE}\` but this path is not in MIGRATED_ROUTES, so there is no hop to ` +
'assert. Drop the marker.'
).toBeDefined();
}
);
});
+1 -1
View File
@@ -20,7 +20,7 @@ import { storageStatePath } from './preview-fixtures';
// for the guard: the guard runs in _app.getInitialProps, ahead of the catchall's getServerSideProps, so a // for the guard: the guard runs in _app.getInitialProps, ahead of the catchall's getServerSideProps, so a
// non-mod is bounced before the migration redirect can matter (which is why the two bounce tests below are // non-mod is bounced before the migration redirect can matter (which is why the two bounce tests below are
// unaffected by the migration), and for a mod the Location tells us WHICH hop happened. // unaffected by the migration), and for a mod the Location tells us WHICH hop happened.
const MODERATOR_PATH = '/moderator/reports'; const MODERATOR_PATH = '/moderator/reports'; // @migrated-route-probe — asserting the hop IS the point
// tester (Flipt allowlist) + gold (allowlist + tier) both CLEAR the preview gate but lack isModerator, so the // tester (Flipt allowlist) + gold (allowlist + tier) both CLEAR the preview gate but lack isModerator, so the
// _app guard must bounce them from /moderator specifically. // _app guard must bounce them from /moderator specifically.
+16 -14
View File
@@ -28,9 +28,8 @@ import { trpcMutation, uniqueToken } from './preview-trpc';
* 3. The report-CREATION leg of the old end-to-end action test. `report.create` * 3. The report-CREATION leg of the old end-to-end action test. `report.create`
* is still a main-app guardedProcedure; `report.getAll` / `report.setStatus` * is still a main-app guardedProcedure; `report.getAll` / `report.setStatus`
* are not (they were deleted in #3573), so the ACTIONING half of that * are not (they were deleted in #3573), so the ACTIONING half of that
* coverage cannot be asserted from here. 🔴 It is LOST, NOT RELOCATED * coverage cannot be asserted from here. It now lives in `apps/moderator`'s
* `apps/moderator` has ZERO test files (measured; positive control: 81 under * own suite (`app:moderator`) see the note at the end of this file.
* `packages/`). Tracked as #4182.
* *
* Only runs under playwright.preview.config.ts (needs PREVIEW_URL + minted states). * Only runs under playwright.preview.config.ts (needs PREVIEW_URL + minted states).
* *
@@ -96,8 +95,8 @@ const MODERATOR_SURFACES = [
* (MODERATOR_APP_URL), so only the path is pinned. * (MODERATOR_APP_URL), so only the path is pinned.
*/ */
const MIGRATED_PATHS = [ const MIGRATED_PATHS = [
{ path: '/moderator/reports', target: '/reports' }, { path: '/moderator/reports', target: '/reports' }, // @migrated-route-probe
{ path: '/moderator/images', target: '/images' }, { path: '/moderator/images', target: '/images' }, // @migrated-route-probe
] as const; ] as const;
// Mirror preview-smoke.spec.ts: assert we cleared the preview gate. // Mirror preview-smoke.spec.ts: assert we cleared the preview gate.
@@ -200,15 +199,18 @@ test.describe('moderation surface (mod)', () => {
// from a preview of THIS app is not possible. Do not "restore" them here: the // from a preview of THIS app is not possible. Do not "restore" them here: the
// procedures do not exist, so it cannot be made to work. // procedures do not exist, so it cannot be made to work.
// //
// 🔴 AND DO NOT READ THAT AS "the coverage moved". An earlier version of this // The coverage itself was rebuilt where the code now lives, against the spoke's
// comment said it "belongs to apps/moderator's own suite" — there is no such // `setReportStatus` and the form actions over it:
// suite. `apps/moderator` has ZERO test files (measured; positive control: 81 // apps/moderator/src/lib/server/__tests__/reports-actioning.test.ts
// under `packages/`). This coverage is LOST, and the loss originates in #3573, // apps/moderator/src/routes/reports/[slug]/__tests__/report-{actions,queue}.test.ts
// not in the change that stopped asserting it. Tracked as #4182. // Unit-level rather than end-to-end, so it is not the same coverage — it does not
// exercise a real report through a real database. What it does buy is a tier that
// renders RED: the `App unit tests` job is not `continue-on-error`, unlike this
// preview job and unlike `unit`. It still does not block a merge — `main` has no
// required_status_checks — so read it as a louder signal, not as a gate.
// //
// The distinction matters here specifically: this is a do-not-restore // The do-not-restore instruction still stands, and its reason is the one above:
// instruction, so a maintainer who checks its stated reason, finds no suite and // the procedures were deleted from this app. That is what makes restoring them
// concludes the comment is stale could try to restore procedures that no longer // here impossible, independently of what any other suite covers.
// exist. The reason above — they were deleted — is the one that holds.
}); });
}); });