From ec49115e55d7c85722ec6223ec342c36df59c9d4 Mon Sep 17 00:00:00 2001 From: Zachary Lowden Date: Thu, 3 Sep 2026 21:33:02 -0500 Subject: [PATCH] feat(user-restriction): make the pending-review mute type a parameter (#4609) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * feat(user-restriction): make the pending-review mute type a parameter Adds an optional `type` to `applyPendingReviewMute`, defaulting to 'generation' so both existing callers are byte-for-byte unchanged, and teaches the moderator queue to show a second type. This is the enabling seam for a bot-account detector that must file into the SAME review queue rather than a new board; no detector logic ships here. `UserRestriction.type` is free text with a [type, status] index, so a new type needs no migration. Dedupe is now scoped per type. Scoped to the user alone, the first queue to mute an account would permanently silence every other queue for it: a later finding of a different kind returns deduped against a row about something else and files nothing a moderator can see. Notifications are an opt-in per-type map, with null meaning "say nothing". createNotification validates its type against nothing, so an unregistered value is persisted and increments the unread badge while the bell drops it at render, leaving a phantom count with no click target; and reusing 'generation-muted' would tell a user their generation access was restricted for something unrelated to generation. A new type therefore stays silent until a processor is registered for it. Verdicts are still generation-shaped, so the moderator resolve and ban actions refuse a row of any other type rather than sending a misleading notice. Parameterising that path is deliberately left out of scope. * test(user-restriction): close two tests that passed against base code Both were found by running the new suites against pre-change source, and both were green there for a reason unrelated to what they claim to test. - The moderator ban refusal asserted only status 400. An invalid ban payload is also 400 with setBanned untouched, and the fixture used a reasonCode outside BAN_REASONS — so the schema rejected it before the type check could run. Uses a valid payload now, asserts the refusal message, and adds the generation-row positive control that proves the refusal is not simply rejecting every ban. - The SELECT-list assertion matched the whole statement, so it was satisfied by the `"ur"."type" = $1` in the WHERE that every version emits. Sliced to the select list. Also fixes a comment naming a function that does not exist. * fix(user-restriction): refuse an unrulable type inside the verdict path itself The type refusal added with the queue lived at ONE of the ruling surfaces. Five callers reach `resolveUserRestriction` — the tRPC router, `/api/mod/restriction/ resolve` (which is what both moderator-app ruling surfaces post through: the audit queue and the retool User Lookup panel), and `overturnPendingReviewMute` — and only the audit queue checked. Reaching the verdict path with a non-generation row would send a "your generation access has been restored" notice and an email, and call `resetProhibitedRequestCount`, wiping the account's real prompt-violation counter over an unrelated case. Moved down rather than replicated at a third route: a predicate open-coded at N sites is wrong at N-1 of them. `RULINGS_WIRED_FOR` and `unwiredRulingReason` now live beside the type vocabulary, and the refusal happens in the service, before any write and before the already-resolved check. Also validates `type` at runtime in `applyPendingReviewMute`. That seam exists to accept a caller-supplied type across an HTTP boundary and a JSON body, where the compiler's word is worth nothing; an out-of-vocabulary value used to mute the account, file a row no queue can select, and send no notification. Latent today — one writer, both callers pass no type, so no non-generation row can exist. Both are closed before a detector ships. Tests, red at the PR head and green here: - refuses Overturn and Uphold on a bot-account row, with no write, no notification, no subscription change and no counter reset - refuses before it argues about the row's status - rejects three out-of-vocabulary types with nothing muted and nothing filed - the seam test pins the moderator app's copy of the wired-for list AND the refusal wording to this one, in both directions The fake's `userRestriction.findUnique` now honours `select` for `type` alone, so a service that refuses non-generation rows but forgets to select the column reads `undefined` and fails the positive controls instead of passing vacuously. * fix(moderator): stop the lookup panel hiding an open case, and stop offering rulings that cannot land Three things, all from the audit on this PR. 1. User Lookup showed the newest restriction of ANY type — three correlated subqueries, `ORDER BY ur.id DESC LIMIT 1`, no type predicate. That was sound while a user could hold at most one open row. Per-type dedupe lets two coexist, so a Pending generation case sitting behind a later Upheld bot-account row rendered as no open restriction at all: the account stays muted, the ruling form is never drawn, and nobody can see the open case. Now `ORDER BY (ur.status = 'Pending') DESC, ur.id DESC` — a Pending case outranks a merely newer one; among several Pending rows, the newest wins; a resolved row is shown only when there is no open one. The panel still speaks for ONE row (a header chip and a single form; the audit queue is where a list belongs), and the ordering is now written once and called three times rather than copied per column, so the three cannot stop naming the same row. 2. `RULINGS_WIRED_FOR` / `unwiredRulingReason` move into `$lib/restriction-types` and are imported by the audit route instead of re-spelled there. The refusal that protects the account now lives in the main app's `resolveUserRestriction`; the route check is KEPT, not for defence in depth but for ordering — `ban` bans and THEN rules, so a refusal arriving inside the verdict call would leave the account banned against a restriction nobody can close. The seam test pins this copy to the main app's in both directions, list and wording. 3. The Bot account queue rendered live Uphold / Remove / Ban forms whose only possible outcome was a 400, and the retool panel offered Overturn / Uphold on a row the verdict path refuses. Both now disable those controls and say why. The server-side refusals are unchanged — they hold against a posted id, which nothing rendered can. Also: `restrictionById`'s comment claimed `unwiredRuling` governs which types an action can be handed; `flagSuspicious` calls it with `type: 'any'` and rules on nothing. The comment now says what the code does and why flagging is deliberately type-agnostic. And `capturingDb`'s `params` is `unknown[][]`, not a `readonly` contract cast away at its one use. Tests: new `user-lookup-restriction-row.test.ts` asserts the Pending-first ordering, the total tiebreak and the per-account scope on all three compiled subqueries; new `restriction-types.test.ts` covers the shared predicate the disabled controls read. The audit route gains an invariant guard that its refusal is the shared predicate's own output rather than a second copy. There is no component-test harness in this app, so the rendering half of (3) is not covered by a test. * fix(user-restriction): make the seam guard read the moderator vocabulary by executing it Round-2 audit F1. The guard that pins the two apps' restriction-type lists to each other read the moderator app's module as TEXT, and it passed green over a real divergence. Its regex captured to the FIRST closing bracket after the '=', so a comment naming an index truncated the capture, and it extracted single-quoted strings only, so a differently-quoted entry vanished. Measured here: with the moderator list written as export const RULINGS_WIRED_FOR: readonly RestrictionType[] = [ 'generation', // matches RESTRICTION_TYPES[0] 'bot-account', ]; the seam file reported 8 passed / 0 failed while the two lists genuinely disagreed. The length > 0 positive control could not see it either, because the first entry survives the truncation. Same green for a mixed-quote entry and for a list assembled with a spread. That is not cosmetic. If the moderator app believes a type is rulable, its audit queue bans the account and posts the ruling afterwards -- a ruling the main app refuses -- leaving a banned account with a Pending row nobody can close. Not fixed by a wider regex: a guard that pins source text by PATTERN is walkable by rewriting the text, and the rewrites that walk it are ordinary (a Prettier reflow, a comment, a quote style). The reader now IMPORTS AND EXECUTES the module and compares values, so formatting cannot be the difference between agreeing and disagreeing. This is available because the moderator vocabulary module has no imports of its own; a comment there says so and says to keep it that way. - moderator-restriction-vocabulary.harness.ts: the reader, plus runtime shape validation that throws (naming the export) rather than returning an empty list. - moderator-restriction-vocabulary.test.ts: 20 tests. Five fixtures, each a REAL divergence written in a different shape -- multi-line, a comment containing a closing bracket, double-quoted entries, a trailing comma, and values assembled at runtime -- plus seven refusal cases for the shape validation. - The refusal SENTENCE is now called rather than parsed out of a template literal, so a message built from constants is compared on what it produces. - apps/moderator restriction-types.test.ts: pin RULINGS_WIRED_FOR BY VALUE. The old unwired.length > 0 caught a widened list only by accident -- bot-account being the only unwired type. Measured: with a third filed type present, the length check passes over a wrongly-wired bot-account and the value pin is the only thing that fails. - .prettierignore: the fixtures' formatting IS the fixture; Prettier would normalise four distinct cases into one. Measured, old reader vs new, both run against the same mutated moderator module: comment containing ']' old 8/8 GREEN new RED (2 failures) mixed quote style old 8/8 GREEN new RED (2 failures) spread / computed list old 8/8 GREEN new RED (2 failures) all-double-quoted old RED (empty-list control) new RED multi-line, no comment old RED new RED single-line trailing , old RED new RED And with the reader reverted to the old text parser, 6 of the 20 new tests fail (the comment, double-quote and computed fixtures, on both their list and message cases); the multi-line and trailing-comma fixtures pass under both and are labelled as declared coverage rather than regression coverage. * fix(user-restriction): let the ruling refusal survive as a 400, and make two claims true Round-2 audit F3, F4 and F5. F4 (behaviour). resolveUserRestriction threw a plain Error, so handleEndpointError fell to its non-TRPCError branch and the refusal reached the wire as 500 "An unexpected error occurred" -- the moderator's panel rendered "Restriction ruling: An unexpected error occurred." and the reason was destroyed. Reachable today from the retool User Lookup panel, which has no local guard. Now throwBadRequestError, which keeps the status and the message. Scope decision on that one: the two neighbouring guards in the same function had the identical defect, and I fixed them in the same change rather than leaving one of three converted. "Restriction record not found" is now a 404 and "Restriction has already been resolved" a 400; both are facts about the request, neither is a server fault, and leaving two of three as opaque 500s would have recreated the same predicate spelled two ways one line apart. All three are covered. Covered by three tests that drive the REAL handleEndpointError over the REAL thrown value, not by asserting the message alone -- a message assertion stays green through exactly the 500 this fixes. Watched red at ff97751d20: "expected 500 to be 400", "expected 500 to be 404", "expected 500 to be 400". F5 (latent). The User Lookup panel's ORDER BY (ur.status = 'Pending') DESC is correct only while status is NOT NULL: Postgres defaults DESC to NULLS FIRST, so a NULL would outrank a genuinely Pending row and hide the open case -- the exact failure the preference exists to prevent, arriving through the column's nullability. DESC NULLS LAST makes it independent of that. The column is a NOT NULL enum today, so this is an unstated precondition made explicit, not a live defect. Two tests red at ff97751d20 on the compiled SQL text. F3 (comment truth). The runtime type guard's comment claimed the values reaching it "cross an HTTP boundary and a JSON body". Nothing does: neither production caller passes a type, and mute-user-pending-review.ts's zod schema has no type key, so no request body can supply one. The guard stays -- what it is actually for is the shape of the NEXT caller (this seam exists so a detector can file into the queue, and the obvious wiring is a route forwarding a JSON field) and the callers TypeScript cannot vouch for today (an `as` cast, a value read back off the free-text column, a JS caller). Corrected in the service and in the test file's docblock, which carried the same false sentence. * test(user-restriction): close the vocabulary guard's environment blind spot Round-4 delta audit, F-1/F-3/F-4. F-1. The execute-based reader resolves the moderator app's vocabulary IN THE MAIN APP'S TEST PROCESS, so an environment-conditional list is read under Vitest and never under the moderator app's production build. Reproduced: with export const RULINGS_WIRED_FOR: readonly RestrictionType[] = import.meta.env.DEV ? ['generation'] : ['generation', 'bot-account']; the seam + vocabulary suites report 28 passed / 0 failed while the shipped build carries both types — the ban-then-strand hazard, reached with every pinning guard green. The base commit's TEXT parser goes RED on that same module, so the reader that replaced it was not strictly stronger; it traded a formatting blind spot for a runtime-environment one. Keeps a TEXT assertion alongside the execute check rather than replacing it: the module's source may contain no import.meta and no process.env. The two mechanisms are complementary. With it, the mutant above fails the seam suite at its beforeAll and the vocabulary suite's real-module case, both naming the constant. Comments are stripped before the scan. Without that the guard is matched by its own documentation — the module has to be able to name the shapes it refuses, and a raw-text scan fires on the sentence forbidding the thing rather than on the thing. Covered by a control asserting a commented mention does not trip it. The moderator module's precondition comment said only 'keep this module import-free'. import.meta.env and process.env need no import statement, so that sentence never covered this; it now states both constraints and says they are separate. F-3. All five vocabulary fixtures wrote 'return RULINGS_WIRED_FOR.includes(type)' while the real module writes '(RULINGS_WIRED_FOR as readonly string[])'. The old parser's message regex requires 'return (RULINGS_WIRED_FOR', so it could not read the message out of ANY fixture and M8 was measuring the old reader against a shape the module does not have. Fixtures now carry the cast. Re-measured, M8 is 6 of 21 — the three fixture pairs the body names (comment-with-bracket, double-quoted, computed), on both their list and message cases. The audit's 8 was the drift; the multi-line-array and trailing-comma fixtures are read correctly by the old parser again, messages included. computed.ts's comment now distinguishes 'assembled from constants declared in this file' — the same value everywhere, which is what the execute reader can certify — from 'assembled from the environment', which is refused. It was the fixture that made the hazardous shape look sanctioned. F-4. The harness docblock now names the cross-app build coupling: resolving the moderator path makes Vite load apps/moderator/tsconfig.json, which extends the gitignored generated .svelte-kit/tsconfig.json, so without svelte-kit sync both suites fail with a TSConfckParseError naming a tsconfig rather than the seam. CI is unaffected; a fresh clone or worktree is not. * fix(moderator): read the refusal out of the envelope the endpoint actually sends Round-4 delta audit, F-2. handleEndpointError's 4xx pass-through emits { message } and no error key, while every other refusal from defineModeratorEndpoint emits { error, message, code }. The moderator app's readError read body.error and nothing else, so all three refusals added last round came back null and the operator saw 'Restriction ruling returned 400.' — the reason destroyed again, one layer further out than the opaque 500 that change removed. Fixed at the CONSUMER, not the emitter, and the choice is measured rather than assumed. handleEndpointError is the shared chokepoint for 36 REST route files; its 4xx and 503 pass-through bodies are pinned toStrictEqual({ message }) by endpoint-helpers-error-envelope.test.ts, the 503 case as an explicit documented carve-out; and restErrorBody needs a RestErrorCode that is not derivable at that point without a new status-to-code map, which would then have to be reconciled with the closed key ledger rest-error-envelope-ledger.test.ts enforces. Widening the reader costs one expression and makes every endpoint's 4xx legible to this app; widening the emitter changes the wire format of 36 routes and needs its own PR. The rule moves to apps/moderator/src/lib/server/rest-error-reason.ts, kept import-free so the main app's suite can load it by filesystem path — the same mechanism as the vocabulary harness. That is what lets the new test drive the REAL emitter into the REAL reader in one process, over all three refusals. Two suites each mocking the other side is precisely the arrangement that cannot see a disagreement about a field name, which is why the previous round's tests were green: they drove the real helper but asserted body.message, which the consumer never looked at. Red before the fix with 'expected null not to be null'; the reader's own null-capability is pinned as a positive control so the three not-null assertions cannot hold against a function that always answers. * docs(test): scope the environment guard's docblock to what it actually refuses Round 4 of the audit ladder found the guard's own documentation claimed more than the regex delivers. A guard description that reads as coverage while providing less is worse than none, because it stops the next reader looking. Two sentences, no behaviour change: - The scope note now says this refuses two spellings and NOT the class, and names the three measured escapes (an aliased global, a computed member access, and a regex literal containing a double slash on the same line as the read, which the comment stripper truncates). It also records what IS covered, so the note does not read as an indictment: the spellings a maintainer would plausibly reach for are caught, and the $app/environment and $env imports break loudly as a missing module. - assertEnvironmentIndependent now documents that string literals are deliberately NOT stripped, so the scanned module may not mention these tokens in a string either. That fails safe -- red with the guard's own message, never silently green -- but it is a real constraint, and a comment is the supported way to write one. Verified: the three affected suites are 99 passed / 3 files, unchanged. --- .prettierignore | 8 + .../lib/__tests__/restriction-types.test.ts | 70 ++ apps/moderator/src/lib/restriction-types.ts | 69 ++ .../__tests__/restriction-type-filter.test.ts | 130 ++++ .../user-lookup-restriction-row.test.ts | 124 ++++ .../src/lib/server/rest-error-reason.ts | 49 ++ .../src/lib/server/user-actions.service.ts | 13 +- .../src/lib/server/user-lookup.service.ts | 55 +- .../lib/server/user-restriction.service.ts | 33 +- .../generator-restrictions/+page.server.ts | 67 +- .../audit/generator-restrictions/+page.svelte | 19 +- .../RestrictionDetail.svelte | 21 +- .../RestrictionFilters.svelte | 27 +- .../__tests__/type-queue.test.ts | 244 +++++++ .../user-lookup/AccountActionsPanel.svelte | 26 +- apps/moderator/src/test/capture-sql.ts | 20 +- .../__tests__/pending-review-mute.test.ts | 612 +++++++++++++++++- .../comment-with-bracket.ts | 22 + .../moderator-vocabulary/computed.ts | 25 + .../moderator-vocabulary/double-quoted.ts | 13 + .../moderator-vocabulary/multi-line-array.ts | 21 + .../moderator-vocabulary/trailing-comma.ts | 13 + ...oderator-restriction-vocabulary.harness.ts | 215 ++++++ .../moderator-restriction-vocabulary.test.ts | 189 ++++++ .../__tests__/restriction-type-seam.test.ts | 148 +++++ .../user-restriction-resolve.service.ts | 31 +- .../services/user-restriction.service.ts | 131 +++- 27 files changed, 2332 insertions(+), 63 deletions(-) create mode 100644 apps/moderator/src/lib/__tests__/restriction-types.test.ts create mode 100644 apps/moderator/src/lib/restriction-types.ts create mode 100644 apps/moderator/src/lib/server/__tests__/restriction-type-filter.test.ts create mode 100644 apps/moderator/src/lib/server/__tests__/user-lookup-restriction-row.test.ts create mode 100644 apps/moderator/src/lib/server/rest-error-reason.ts create mode 100644 apps/moderator/src/routes/audit/generator-restrictions/__tests__/type-queue.test.ts create mode 100644 src/server/services/__tests__/__fixtures__/moderator-vocabulary/comment-with-bracket.ts create mode 100644 src/server/services/__tests__/__fixtures__/moderator-vocabulary/computed.ts create mode 100644 src/server/services/__tests__/__fixtures__/moderator-vocabulary/double-quoted.ts create mode 100644 src/server/services/__tests__/__fixtures__/moderator-vocabulary/multi-line-array.ts create mode 100644 src/server/services/__tests__/__fixtures__/moderator-vocabulary/trailing-comma.ts create mode 100644 src/server/services/__tests__/moderator-restriction-vocabulary.harness.ts create mode 100644 src/server/services/__tests__/moderator-restriction-vocabulary.test.ts create mode 100644 src/server/services/__tests__/restriction-type-seam.test.ts diff --git a/.prettierignore b/.prettierignore index 9df5ea64de..44b8cbdcff 100644 --- a/.prettierignore +++ b/.prettierignore @@ -26,3 +26,11 @@ packages/civitai-db-schema/src/schema-drift/__tests__/fixtures/catalog-productio packages/civitai-db-schema/src/enums.ts packages/civitai-db-schema/src/models.ts packages/civitai-db-schema/src/kysely/ + +# 🔴 THE FORMATTING IS THE FIXTURE. These modules exist so +# `src/server/services/__tests__/moderator-restriction-vocabulary.test.ts` can prove its reader sees a +# divergence in the moderator app's restriction vocabulary regardless of how that vocabulary is +# written — double-quoted entries, a trailing comma, a multi-line array, a comment containing a `]`. +# Prettier normalises exactly those differences away, which would silently collapse four distinct +# cases into one and leave the suite green while testing a third of what it claims to. +src/server/services/__tests__/__fixtures__/moderator-vocabulary/ diff --git a/apps/moderator/src/lib/__tests__/restriction-types.test.ts b/apps/moderator/src/lib/__tests__/restriction-types.test.ts new file mode 100644 index 0000000000..a083c767c1 --- /dev/null +++ b/apps/moderator/src/lib/__tests__/restriction-types.test.ts @@ -0,0 +1,70 @@ +import { describe, expect, it } from 'vitest'; +import { + RESTRICTION_TYPE, + RESTRICTION_TYPES, + RESTRICTION_TYPE_LABELS, + RULINGS_WIRED_FOR, + unwiredRulingReason, +} from '$lib/restriction-types'; + +/** + * The one predicate three surfaces read: the audit queue's `resolve`/`ban` refusal, the audit queue's + * disabled ruling buttons, and the retool User Lookup panel's disabled ruling form. It was open-coded + * in the route before #4609's audit; a predicate spelled at N sites is wrong at N−1 of them, and here + * the sites disagreeing means a live button whose only possible outcome is a rejected call. + * + * The refusal that MATTERS is enforced by the main app, inside `resolveUserRestriction` — this list is + * the same rule read forward so a form that cannot succeed is never offered. The two are pinned to + * each other by `src/server/services/__tests__/restriction-type-seam.test.ts`. + */ +describe('unwiredRulingReason', () => { + it('permits a ruling on every wired-for type', () => { + // Non-vacuous: there is at least one, and it is the queue's default. + expect(RULINGS_WIRED_FOR.length).toBeGreaterThan(0); + expect(RULINGS_WIRED_FOR).toContain(RESTRICTION_TYPE); + for (const type of RULINGS_WIRED_FOR) expect(unwiredRulingReason(type)).toBeNull(); + }); + + /** + * 🔴 PINNED BY VALUE, not by "at least one type is refused". + * + * The test below used to lean on `unwired.length > 0` to stay non-vacuous, and that made it catch + * a widened `RULINGS_WIRED_FOR` only BY ACCIDENT — `bot-account` being the sole unwired type is + * the only reason wiring it in emptied the list. Add a third filed type and the accident is gone: + * `RULINGS_WIRED_FOR` could claim a verdict path for `bot-account` while `unwired` still holds the + * third type, so the length check passes and the loop passes and nothing here notices. + * + * A value pin does not decay that way. It is the mirror of the main app's own + * (`expect([...RULINGS_WIRED_FOR]).toEqual(['generation'])` in + * `src/server/__tests__/pending-review-mute.test.ts`), and widening this list on either side is + * supposed to be a deliberate act with the verdict path parameterised first. + */ + it('claims a verdict path for generation and for nothing else', () => { + expect([...RULINGS_WIRED_FOR]).toEqual(['generation']); + }); + + it('refuses every filed type that has no verdict path, naming it', () => { + const unwired = RESTRICTION_TYPES.filter((t) => !RULINGS_WIRED_FOR.includes(t)); + // Kept only as a vacuity guard. It is NOT what catches a widened `RULINGS_WIRED_FOR` any more — + // the value pin above is, and it does not decay as the vocabulary grows. + expect(unwired.length).toBeGreaterThan(0); + + for (const type of unwired) { + const reason = unwiredRulingReason(type); + // The type is named because a moderator has to be able to tell WHICH queue is review-only, and + // the message doubles as the audit route's `fail(400)` body. + expect(reason).toContain(`"${type}"`); + expect(reason).toContain('NOT resolved'); + } + }); + + it('refuses a type nobody has heard of, rather than defaulting it in', () => { + // The value reaching this can come off a database row, so it is not confined to the union. + for (const type of ['', 'GENERATION', 'generation ', 'nonsense']) + expect(unwiredRulingReason(type)).not.toBeNull(); + }); + + it('keeps a label for every filed type, so a refused queue can still be named on screen', () => { + expect(Object.keys(RESTRICTION_TYPE_LABELS).sort()).toEqual([...RESTRICTION_TYPES].sort()); + }); +}); diff --git a/apps/moderator/src/lib/restriction-types.ts b/apps/moderator/src/lib/restriction-types.ts new file mode 100644 index 0000000000..0b070f8f17 --- /dev/null +++ b/apps/moderator/src/lib/restriction-types.ts @@ -0,0 +1,69 @@ +/** + * The kinds of review that file into the moderator mute queue. + * + * `UserRestriction.type` is free text in the database with a `[type, status]` index, so a new kind of + * review costs no migration — it files into the same queue under a different type. The list is + * enumerated here rather than derived from the rows so the queue's filter cannot be driven to an + * arbitrary string from the URL, and so a type nobody built a view for cannot render an empty page + * that reads as "no work to do". + * + * 🔴 This file is deliberately OUTSIDE `$lib/server/`. `RestrictionFilters.svelte` renders the type + * picker and therefore needs these as VALUES; SvelteKit rejects a value import of `$lib/server/*` from + * client-reachable code, and the sibling components only get away with importing from the service + * because theirs are `import type` and erase. Keep the list here and re-export it from the service. + * + * Mirrored for the main app in `src/server/services/user-restriction.service.ts`; the two lists are + * pinned to each other by `src/server/services/__tests__/restriction-type-seam.test.ts`, which + * imports and executes this module rather than reading it as text. + */ +export const RESTRICTION_TYPES = ['generation', 'bot-account'] as const; +export type RestrictionType = (typeof RESTRICTION_TYPES)[number]; + +/** What the queue shows when the URL names no type — the only type that existed before the seam. */ +export const RESTRICTION_TYPE: RestrictionType = 'generation'; + +export const RESTRICTION_TYPE_LABELS: Record = { + generation: 'Generation', + 'bot-account': 'Bot account', +}; + +/** + * The types a VERDICT can be handed to — narrower than `RESTRICTION_TYPES`, which is what may be + * filed and reviewed. + * + * The refusal itself is enforced by the main app, one level below every ruling surface, in + * `resolveUserRestriction` — this app's ruling forms all post through `/api/mod/restriction/resolve`. + * This list is the same rule read forward instead of backward: it is what lets a form that cannot + * possibly succeed be disabled rather than merely rejected, and what lets the audit queue refuse a + * ban BEFORE it bans (that action bans and then rules, so a late refusal would leave the account + * banned against a restriction nobody can close). + * + * 🔴 Kept identical to the main app's `RULINGS_WIRED_FOR` by + * `src/server/services/__tests__/restriction-type-seam.test.ts`, which IMPORTS AND EXECUTES this + * module and compares the resulting values. The two apps are separate builds with no runtime import + * path between them, so a pinned copy is the strongest available form of "one rule, one place" — do + * not fork it by hand. + * + * 🔴 That guard used to parse this file as TEXT, and passed green over a real divergence: a `]` in a + * trailing comment truncated its capture. Two preconditions come out of the replacement, and they are + * SEPARATE — the second is not implied by the first: + * + * 1. KEEP THIS MODULE IMPORT-FREE. It has no imports today, and that is the only reason the main + * app's Vitest project can load it across the app boundary; adding a `$lib/…` import here breaks + * the seam guard loudly rather than silently, but it does break it. + * 2. KEEP EVERY VALUE HERE ENVIRONMENT-INDEPENDENT — no `import.meta.env`, no `process.env`, and + * nothing derived from them. Neither needs an import statement, so rule 1 does not cover this. + * The guard EXECUTES this module inside the main app's test process: a list that branches on the + * environment is read under Vitest and never under this app's production build, so the two apps + * can ship different lists with every guard on both sides green. Assembling a value from + * constants declared in this file is fine — that is the same value everywhere. Reading one from + * the environment is not, and is refused by name. + */ +export const RULINGS_WIRED_FOR: readonly RestrictionType[] = ['generation']; + +/** Why a ruling may not be handed to a row of this type, or `null` when it may. */ +export function unwiredRulingReason(type: string): string | null { + return (RULINGS_WIRED_FOR as readonly string[]).includes(type) + ? null + : `Rulings are not yet available for "${type}" restrictions — the verdict path still sends generation-specific notices. This restriction was NOT resolved.`; +} diff --git a/apps/moderator/src/lib/server/__tests__/restriction-type-filter.test.ts b/apps/moderator/src/lib/server/__tests__/restriction-type-filter.test.ts new file mode 100644 index 0000000000..75b957e824 --- /dev/null +++ b/apps/moderator/src/lib/server/__tests__/restriction-type-filter.test.ts @@ -0,0 +1,130 @@ +import { describe, expect, it, vi } from 'vitest'; + +/** + * `UserRestriction.type` is what separates one review queue from another. The rows carry no other + * marker — same table, same status vocabulary, same shape — so if the predicate is dropped, weakened, + * or bound to the wrong value, one queue simply renders another's cases and a moderator rules on them + * under the wrong assumptions. + * + * 🔴 Asserted against the COMPILED SQL **and its bound parameters**. The text alone cannot see the bug + * this file exists for: `where('ur.type','=',x)` emits `"ur"."type" = $1` whatever `x` is, so a version + * that ignores its argument and always filters `generation` produces byte-identical SQL. The parameter + * is the only place the difference is visible. + * + * The count query is checked alongside the row query on purpose — they are separately compiled off a + * shared builder, and a filter that reaches one but not the other gives a pager whose total counts + * every type's rows. + */ + +const captured = vi.hoisted(() => [] as string[]); +const capturedParams = vi.hoisted(() => [] as unknown[][]); + +// Built inside the factory, not in `vi.hoisted`: hoisted blocks run before this file's own imports, so +// constructing the client there reads it before initialisation. +vi.mock('$lib/server/db', async () => { + const { capturingDb } = await import('../../../test/capture-sql'); + const db = capturingDb(captured, [], capturedParams); + return { dbRead: db, dbWrite: db }; +}); + +const { getGenerationRestrictions, RESTRICTION_TYPE } = await import('../user-restriction.service'); + +type Compiled = { sql: string; params: unknown[] }; + +const compile = async ( + query: Parameters[0] +): Promise => { + captured.length = 0; + capturedParams.length = 0; + await getGenerationRestrictions(query); + // 🔴 Assert the COUNT, not just the contents. A chain that stops early leaves `captured` short while + // every assertion over what IS in it still passes — on a query that was never built. + expect(captured).toHaveLength(2); + return captured.map((sql, i) => ({ sql, params: capturedParams[i] })); +}; + +const base = { page: 1, limit: 20 } as const; + +/** The `ur.type` predicate's bound value, or `undefined` if the query emitted no such predicate. */ +const boundType = ({ sql, params }: Compiled): unknown => { + const match = /"ur"\."type" = \$(\d+)/.exec(sql); + return match ? params[Number(match[1]) - 1] : undefined; +}; + +describe('getGenerationRestrictions — type scoping', () => { + it('filters on the generation type when the caller names none', async () => { + const compiled = await compile({ ...base }); + + // Both statements, not just the first: the second is the pager's total. + for (const c of compiled) expect(boundType(c)).toBe('generation'); + expect(RESTRICTION_TYPE).toBe('generation'); + }); + + it('filters on the type it was given', async () => { + const compiled = await compile({ ...base, type: 'bot-account' }); + + for (const c of compiled) expect(boundType(c)).toBe('bot-account'); + }); + + // 🔴 The leak, stated as a property rather than as one example: whatever type is asked for, no other + // type's rows can satisfy the query. A predicate bound to the requested value is what guarantees it. + it.each(['generation', 'bot-account'] as const)( + 'binds %s and no other type, in both the list and the count', + async (type) => { + const compiled = await compile({ ...base, type }); + + for (const c of compiled) { + expect(boundType(c)).toBe(type); + // Exactly one type PREDICATE — a second, differently-bound one would AND/OR in another queue. + // Matched with the `= $n` so this counts predicates and not the `ur.type` in the SELECT list. + expect(c.sql.match(/"ur"\."type" = \$\d+/g)).toHaveLength(1); + } + } + ); + + it('keeps the type predicate when other filters are also applied', async () => { + // The type predicate is unconditional while the rest are `$if`s. A refactor that folded it in with + // them is exactly how it would go missing, and only a query carrying both shapes can see that. + const compiled = await compile({ + ...base, + type: 'bot-account', + status: 'Pending', + username: 'someone', + }); + + for (const c of compiled) { + expect(boundType(c)).toBe('bot-account'); + expect(c.sql).toMatch(/"ur"\."status" = \$/); + } + }); + + /** + * The one caller allowed past the filter: a lookup by primary key. A form posts to `?/resolve`, which + * replaces the query string, so an action never learns which queue the moderator was in — filtering a + * by-id lookup by the DEFAULT type would 404 every row outside it. A primary key cannot be made more + * correct by a type predicate, so the predicate is dropped rather than guessed. + */ + it("drops the predicate only for the explicit 'any'", async () => { + const compiled = await compile({ ...base, type: 'any', restrictionId: 7 }); + + for (const c of compiled) { + expect(boundType(c)).toBeUndefined(); + // The PREDICATE is gone. `ur.type` still appears in the row query's SELECT list, which is what + // lets the caller read back the type it did not filter on. + expect(c.sql).not.toMatch(/"ur"\."type" = \$/); + expect(c.sql).toMatch(/"ur"\."id" = \$/); + } + }); + + it('selects the row type, so a caller can tell what it got back', async () => { + // `restrictionById` reads it to decide whether a ruling is wired for the row; without it in the + // SELECT that check silently compares against `undefined`. + const [list] = await compile({ ...base }); + + // 🔴 Sliced to the SELECT list first. Asserted against the whole statement, this passes on the + // `"ur"."type" = $1` in the WHERE — which every pre-change version also emitted — so the guard + // would report coverage of a column that is not being selected at all. + const selectList = list.sql.slice(0, list.sql.indexOf(' from "UserRestriction"')); + expect(selectList).toContain('"ur"."type"'); + }); +}); diff --git a/apps/moderator/src/lib/server/__tests__/user-lookup-restriction-row.test.ts b/apps/moderator/src/lib/server/__tests__/user-lookup-restriction-row.test.ts new file mode 100644 index 0000000000..1fb0c8d629 --- /dev/null +++ b/apps/moderator/src/lib/server/__tests__/user-lookup-restriction-row.test.ts @@ -0,0 +1,124 @@ +import { describe, expect, it, vi } from 'vitest'; + +/** + * Which UserRestriction row the User Lookup header and its ruling form speak for. + * + * 🔴 The panel shows ONE row, and until #4609 it picked "newest of any type" — `ORDER BY ur.id DESC + * LIMIT 1`, no type predicate. That was sound only while a user could hold at most one open row. + * Restrictions now dedupe PER TYPE, so two open cases can coexist, and the old ordering had a silent + * failure mode: a Pending generation case sitting behind a LATER Upheld bot-account row rendered as + * *no open restriction at all* — the account stays muted, the ruling form is never drawn, and nobody + * looking at the account can see there is an open case. + * + * Asserted against the COMPILED SQL. There is no database in this tier, and the ordering is the whole + * behaviour — it decides which of several rows the moderator is shown, and it typechecks and lints + * identically either way. Same instrument the sibling `restriction-type-filter.test.ts` uses, for the + * same reason. + */ + +const captured = vi.hoisted(() => [] as string[]); + +// Built inside the factory, not in `vi.hoisted`: hoisted blocks run before this file's own imports. +vi.mock('$lib/server/db', async () => { + const { capturingDb } = await import('../../../test/capture-sql'); + // One canned row, so `executeTakeFirst` resolves and the chain does not stop early. + const db = capturingDb(captured, [{ id: 1 }]); + return { dbRead: db, dbWrite: db }; +}); + +// The identity query is the only thing under test here; its module's other exports reach a second +// database and an HTTP service, and none of them is on this path. +const { getIdentity } = await import('../user-lookup.service'); + +const identitySql = async (): Promise => { + captured.length = 0; + await getIdentity(42); + // 🔴 Count first. A chain that stopped early would leave `captured` empty while every assertion over + // its contents passed vacuously. + expect(captured).toHaveLength(1); + return captured[0].replace(/\s+/g, ' '); +}; + +/** + * The three correlated subqueries that read the account's restriction, sliced from the head of each + * `FROM "UserRestriction" ur` to its `LIMIT 1`. + */ +const restrictionSubqueries = (sql: string): string[] => + sql + .split('FROM "UserRestriction" ur') + .slice(1) + .map((chunk) => { + const end = chunk.indexOf('LIMIT 1'); + expect(end).toBeGreaterThan(-1); + return chunk.slice(0, end).trim(); + }); + +describe('user lookup — which restriction the panel speaks for', () => { + it('reads three restriction columns, which is what the panel needs to render a ruling form', async () => { + const sql = await identitySql(); + + // Positive control on the slicer, and the thing that makes every assertion below non-vacuous: a + // rename or a fourth column shows up here rather than silently reducing the loops to no-ops. + expect(restrictionSubqueries(sql)).toHaveLength(3); + for (const alias of ['restrictionStatus', 'restrictionType', 'restrictionId']) + expect(sql).toContain(`as "${alias}"`); + }); + + /** + * 🔴 The regression. A Pending row outranks a merely newer one, so an open case cannot be hidden + * behind a later resolved one of another type. + */ + it('prefers a Pending row over a newer one, in every restriction column', async () => { + const sql = await identitySql(); + + for (const sub of restrictionSubqueries(sql)) + expect(sub).toContain(`ORDER BY (ur.status = 'Pending') DESC NULLS LAST, ur.id DESC`); + }); + + /** + * 🔴 The null placement, asserted on its own so it cannot be dropped by someone tidying the + * ordering above back to a bare `DESC`. + * + * Postgres defaults `DESC` to `NULLS FIRST`. A NULL `(ur.status = 'Pending')` would therefore sort + * ABOVE a genuinely Pending row and hide the open case — exactly the failure the preference exists + * to prevent, arriving through the column's nullability rather than through the ordering. The + * column is a NOT NULL enum today, so this is an unstated precondition being made explicit rather + * than a live defect; spelled out, the ordering no longer depends on it. + */ + it('places nulls last, so the ordering does not depend on status being NOT NULL', async () => { + const sql = await identitySql(); + const subs = restrictionSubqueries(sql); + + // Non-vacuous: three subqueries exist to check. (The slicer's own control lives in the first + // test; this repeats the count because an empty list would make the loop below pass.) + expect(subs).toHaveLength(3); + for (const sub of subs) { + expect(sub).toContain(`'Pending') DESC NULLS LAST`); + // And not a bare `DESC` on that expression, which is what the default null placement is. + expect(sub).not.toMatch(/'Pending'\)\s+DESC\s*,/); + } + }); + + /** + * The tiebreak has to be TOTAL, or the three subqueries are free to resolve to different rows and + * the panel renders one row's status against another's id — a ruling form posting an id that does + * not belong to the case it is describing. + */ + it('breaks the tie on a unique column, so the three columns name one row', async () => { + const sql = await identitySql(); + const subs = restrictionSubqueries(sql); + + for (const sub of subs) expect(sub).toContain('ur.id DESC'); + // Identical apart from the column each selects — which is the only reason they agree. (An + // INVARIANT GUARD: true before this change too. Kept because the fix replaced three hand-written + // copies with one helper, and this is what stops them being hand-written again.) + const withoutSelectList = subs.map((s) => s.slice(s.indexOf('WHERE'))); + expect(new Set(withoutSelectList).size).toBe(1); + }); + + it('scopes each subquery to the account being looked up', async () => { + const sql = await identitySql(); + + for (const sub of restrictionSubqueries(sql)) expect(sub).toContain(`ur."userId" = u.id`); + }); +}); diff --git a/apps/moderator/src/lib/server/rest-error-reason.ts b/apps/moderator/src/lib/server/rest-error-reason.ts new file mode 100644 index 0000000000..dfc5476244 --- /dev/null +++ b/apps/moderator/src/lib/server/rest-error-reason.ts @@ -0,0 +1,49 @@ +/** + * The refusal a main-app endpoint wrote for the operator, read out of its JSON error body. + * + * 🔴 TWO ENVELOPE SHAPES REACH THIS APP, and reading only one of them silently destroys the reason. + * `defineModeratorEndpoint` hands a throw to the main app's `handleEndpointError` + * (`src/server/utils/endpoint-helpers.ts`), and that helper emits: + * + * - `{ error, message, code }` — every `restErrorBody(...)` path, which is what the 5xx branch and + * the genericized-4xx branch use; + * - `{ message }` alone — the 4xx/503 PASS-THROUGH branch, i.e. exactly the statuses that carry a + * refusal somebody wrote FOR a human (`throwBadRequestError`, `throwNotFoundError`, …). + * + * Reading `error` only therefore came back `null` for the entire second group, and the caller fell + * back to `"