From 6e61e1821c9a38b47135e158ac1dc686e4476675 Mon Sep 17 00:00:00 2001 From: briant Date: Thu, 20 Aug 2026 13:07:22 -0600 Subject: [PATCH] test(preview): fail a stranded /moderator probe in the local suite, not only in a job nobody reads MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit #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) --- .github/workflows/lint.yml | 19 +- CLAUDE.md | 29 ++- package.json | 2 +- .../no-stale-moderator-route-probe.test.ts | 199 ++++++++++++++++++ tests/preview-auth-guard.spec.ts | 2 +- tests/preview-moderation.spec.ts | 30 +-- 6 files changed, 258 insertions(+), 23 deletions(-) create mode 100644 src/server/services/__tests__/no-stale-moderator-route-probe.test.ts diff --git a/.github/workflows/lint.yml b/.github/workflows/lint.yml index 1a99793fe5..490100c7d8 100644 --- a/.github/workflows/lint.yml +++ b/.github/workflows/lint.yml @@ -265,6 +265,21 @@ jobs: # # FLIP TO BLOCKING once a couple of weeks of runs show a clean pass rate. # 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 timeout-minutes: 30 steps: @@ -380,7 +395,9 @@ jobs: # 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. # - # `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 steps: - uses: actions/checkout@v4 diff --git a/CLAUDE.md b/CLAUDE.md index e4f9e12894..56c6479f33 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -111,13 +111,22 @@ CI already scopes itself this way and gates only on **added** files for exactly ### Testing ```bash -pnpm run test:unit:run # Vitest unit suite (the one you almost always want) -pnpm run test:component # Vitest component suite (browser mode — see Git Worktrees for NixOS) -pnpm run test:lint-rules # Convention guards (see below) -pnpm test # Playwright e2e -pnpm run test:ui # Playwright with UI +pnpm run test:unit:run # Vitest unit suite over src/ + scripts/ (the one you almost always want) +pnpm run test:packages:run # Vitest, the packages/* suites +pnpm run test:apps:run # Vitest, the apps/* suites +pnpm run test:component # Vitest component suite (browser mode — see Git Worktrees for NixOS) +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 — `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 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 **"The tests would catch a regression here" is a claim about the failure mode, not about coverage.** A green diff --git a/package.json b/package.json index 85c12c8838..e1e4c4d861 100644 --- a/package.json +++ b/package.json @@ -100,7 +100,7 @@ "test:packages:run": "vitest run --project '@civitai/*'", "test:apps": "vitest --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:watch": "vitest --project component", "meilisearch:migrate": "NODE_ENV=development tsx scripts/oneoffs/meilisearch-migration.ts", diff --git a/src/server/services/__tests__/no-stale-moderator-route-probe.test.ts b/src/server/services/__tests__/no-stale-moderator-route-probe.test.ts new file mode 100644 index 0000000000..b542c131ce --- /dev/null +++ b/src/server/services/__tests__/no-stale-moderator-route-probe.test.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(); + } + ); +}); diff --git a/tests/preview-auth-guard.spec.ts b/tests/preview-auth-guard.spec.ts index 6a36033197..296537ec59 100644 --- a/tests/preview-auth-guard.spec.ts +++ b/tests/preview-auth-guard.spec.ts @@ -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 // 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. -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 // _app guard must bounce them from /moderator specifically. diff --git a/tests/preview-moderation.spec.ts b/tests/preview-moderation.spec.ts index f883c196e1..a6720805a3 100644 --- a/tests/preview-moderation.spec.ts +++ b/tests/preview-moderation.spec.ts @@ -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` * is still a main-app guardedProcedure; `report.getAll` / `report.setStatus` * are not (they were deleted in #3573), so the ACTIONING half of that - * coverage cannot be asserted from here. 🔴 It is LOST, NOT RELOCATED — - * `apps/moderator` has ZERO test files (measured; positive control: 81 under - * `packages/`). Tracked as #4182. + * coverage cannot be asserted from here. It now lives in `apps/moderator`'s + * own suite (`app:moderator`) — see the note at the end of this file. * * 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. */ const MIGRATED_PATHS = [ - { path: '/moderator/reports', target: '/reports' }, - { path: '/moderator/images', target: '/images' }, + { path: '/moderator/reports', target: '/reports' }, // @migrated-route-probe + { path: '/moderator/images', target: '/images' }, // @migrated-route-probe ] as const; // 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 // 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 - // comment said it "belongs to apps/moderator's own suite" — there is no such - // suite. `apps/moderator` has ZERO test files (measured; positive control: 81 - // under `packages/`). This coverage is LOST, and the loss originates in #3573, - // not in the change that stopped asserting it. Tracked as #4182. + // The coverage itself was rebuilt where the code now lives, against the spoke's + // `setReportStatus` and the form actions over it: + // apps/moderator/src/lib/server/__tests__/reports-actioning.test.ts + // apps/moderator/src/routes/reports/[slug]/__tests__/report-{actions,queue}.test.ts + // 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 - // instruction, so a maintainer who checks its stated reason, finds no suite and - // concludes the comment is stale could try to restore procedures that no longer - // exist. The reason above — they were deleted — is the one that holds. + // The do-not-restore instruction still stands, and its reason is the one above: + // the procedures were deleted from this app. That is what makes restoring them + // here impossible, independently of what any other suite covers. }); });