Zachary Lowden ec49115e55 feat(user-restriction): make the pending-review mute type a parameter (#4609) ...

* 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.

2026-09-03 21:33:02 -05:00

2.5 KiB

Raw Permalink Blame History

View Raw