feat(new-order): file KoN abuse detection on the moderator abuse board (#4823)

* feat(new-order): file KoN abuse detection on the moderator abuse board

The daily `new-order-abuse-detection` scan found its suspects, posted a
Discord embed capped at 10 of them, and persisted nothing. A webhook
message is not reviewable, not searchable, not attributable to an account,
and cannot record that a detection was deliberately left alone — which is
most of what this scan produces.

It now files every suspect on the moderator app's abuse-detection board,
the surface built for exactly this, alongside the Axiom logging it already
did. Two detectors already post there; this follows their structure.

Where it differs from both of them, and the one subtle part: they hardcode
`actioned: false` because neither holds a write client. This scan DOES act
— when `autoSmiteAbusers` is on it smites the strict-signal subset — so
`actioned` is a per-finding fact. The wire contract's superRefine rejects
both halves of the wrong pairing, and one rejection loses the entire batch
rather than the offending row, so the findings are built AFTER the smite
loop, from the set of accounts `smitePlayer` actually succeeded on. A
target whose write threw stays an open case.

- add `src/server/services/new-order-abuse-detection/report.ts`: reason
  rendering (all six query columns go in the text — the contract has no
  structured-metrics field), a confidence band that is the queue's sort
  order rather than a probability, the actioned/action pairing minted as
  two whole branches, and the report builder
- replace the Discord block with `moderatorApp.abuseReport`; keep the
  Axiom logging. `DISCORD_WEBHOOK_MOD_ALERTS` keeps its other consumers
  and stays in the env schema
- record real `startedAt`/`finishedAt`; the job recorded neither, and the
  contract refuses a transposed pair
- single-source the 24h lookback as `ABUSE_SCAN_WINDOW_HOURS`, used by
  both the query and the reason text
- add `lockExpiration`; the job had none, and a duplicate concurrent run
  appends a second near-identical run rather than replacing the first
- no threshold goes in `counters`. The scan's tunables are held in Redis
  so they are not readable from the public source tree, and the board has
  a wider audience than this job's logs. The counters are outcomes only

Tests: the report mapping, a regression guard on the actioned/action pair
in both directions with the contract's own parse as the oracle, and the
job→board seam including a suspect whose smite threw.

* style: apply Prettier to the two files the lint gate flagged

* fix(new-order): correct three overclaimed justifications, and rethrow a report failure that protects nothing

A requirements audit refuted the stated grounds for three of this PR's
decisions. The shipped mapping was not at fault; the prose defending it was,
and one of the decisions was wrong on its merits.

lockExpiration — the premise was false. createJob already defaults every job
to a 5-minute lock (job.ts), on main and on this branch, so this is a widening
from 5 to 10 and not an introduction. The value matches the sibling detector
reaction-withdrawal-detection and nothing else supports it: no run of this scan
has been measured exceeding 5 minutes. The comment now says that, including
that the size is borrowed rather than derived. The value is unchanged.

Threshold withholding — the claim was false as written and had propagated to
five sites. "The confidence score cannot be inverted to recover a threshold" is
true of confidenceFor in isolation and false of the row it ships on: the reason
text publishes the values the selection rule compares, and the query selects on
HAVING totalRatings >= minTotalRatings, so the smallest totalRatings visible on
the board converges on that tunable from above within a few runs; the smallest
dominant share among auto-smited rows converges the same way. The sibling claim
that Redis keeps the values off the public source tree is also false — this repo
is public and a pre-existing checked-in test of the smite path carries live
values. The withholding is KEPT, on proportionality: nothing a moderator does
with this board needs a tunable, and the surface this replaced carried the same
observed values. Every site now says that instead, including the two
pre-existing ones that were the origin of the claim.

Report failure — the swallow blinded the detector's only dark-signal, and the
precedents do not support it. bot-account-detection logs under a stable key and
RETHROWS; reaction-withdrawal-detection does not catch at all. With the token
rotated, every post would 401, the job would return success, and the job error
counter would stay flat with the detector dark and nothing anywhere to say so.
Scan-originated smites are rare, so the unconditional catch paid that blindness
on nearly every run to cover a case that nearly never occurs. It now rethrows
when the run smited nobody and swallows only when smites are already written.
The free-text log sentence is replaced by a stable, alertable key in the
precedents' style.

Whether a failed run is retried at all could NOT be established — the scheduler
lives outside this repo and the run-jobs route has no retry — so the remaining
swallow is documented as a precaution against an unconfirmed retry, not a
response to a measured one.

Axiom payload cut to an aggregate. The stated ground, that both precedents log
alongside the board post, holds only for aggregates: both log run-level counts
and neither logs per-account detail. The board now renders those columns with
attribution and reviewed-state, so the array duplicated the sensitive half of
the payload into a surface with different retention and no review workflow.

Tests: both halves of the rethrow split are pinned and each was watched to fail
for its own reason. Removing the guard fails only the propagate case
("promise resolved undefined instead of rejecting"); making the rethrow
unconditional fails only the swallow case ("promise rejected Error: 400 bad
request instead of resolving"). The aggregate cut is pinned as the whole details
key set, which was watched to reject a re-added per-account array under a
different key name.

Local: pnpm typecheck 0 errors; 82 tests passed across the two new-order suites.

* fix(new-order): key board membership to the smite ROW, not to the call returning

Round-1 audit fixes. The one that matters is the first.

A smite that was APPLIED but whose call threw afterwards was filed on the
abuse board as an open case. `smitePlayer` commits the smite row first and
then does a tail of non-durable work — an active-smite count, a possible
career reset, a Redis counter increment, a signal, a notification. The
counter increment is the sharp one: it calls `getCount`, which re-throws a
non-connection ClickHouse error, then writes to `sysRedis` with no guard at
all, unlike the fail-open `setCacheValue` beside it. Any throw past the
create leaves the penalty live in Postgres while the job's catch skipped
`smitedUserIds.add`. The finding then read `actioned: false` with "No action
was taken by this scan — filed for a moderator to review." about an account
carrying a live smite — the exact inverse of the invariant `report.ts`
declares load-bearing, and an invitation to apply a second penalty.

`smitePlayer` now takes an optional `onSmiteCreated` hook, fired
synchronously the instant the row is committed and before any of the tail.
A return value cannot carry this, because the case that matters is the one
where there is no return. The hook is additive: the other three call sites
pass nothing and are unchanged.

So `actioned: true` now means exactly "a smite row was written for this
account by this run". It does NOT claim the player was notified, that their
counter moved, or that a third-strike career reset completed — each of those
is in the tail and can fail independently of the penalty. Both `report.ts`
comments that described the old, stronger semantic are corrected to say
that, rather than left claiming more than the code delivers.

- `smitePlayer`: add `onSmiteCreated`, called once right after the
  `newOrderSmite.create`
- the scan's smite loop: record membership from the hook
- `report.ts`: `toFinding` and `BuildReportArgs.smitedUserIds` now state the
  durable-write semantic, and name both directions the flag can be wrong in
- the duplicate-run comment on `lockExpiration` said the worst case of a
  concurrent run is "a moderator sees the same cohort twice". That is the
  cosmetic half. `smitePlayer` is not idempotent — every call inserts
  another smite row, and the row that carries an account to the third-strike
  rule chains into `resetPlayer`, wiping that player's career with a
  notification. Comment only; the hazard is unchanged
- the per-player smite failure logged an interpolated sentence as the Axiom
  `name`, three lines above the comment articulating why that is wrong.
  Stable key `new-order-abuse-detection:auto-smite-failed`, id in the
  details, plus whether the penalty landed anyway
- `MAX_REASON_LENGTH` is now exported from the contract and imported here,
  mirroring `MAX_FINDINGS_PER_REPORT`, instead of being a second literal
  that can drift from the `.max(...)` it is supposed to track. Its comment
  no longer implies live protection: measured, `renderReason`'s longest
  possible output is 311 characters with every numeric field at
  `Number.MAX_SAFE_INTEGER` (269 smited, 311 open), so the truncation is
  defence in depth against a future template, not something that fires today
- one unrelated prettier hunk in `new-order.service.ts`: the file was
  already prettier-dirty at HEAD and the lint gate checks changed files

Tests: a new `smite-durable-write.test.ts` exercises the REAL `smitePlayer`
and pins the seam — the hook fires before the tail, still fires when the
tail throws, and does NOT fire when the row was never written. Moving the
hook to the end of the function kills two of those, each for its own reason.
The scan suite gains the mirror pair: a smite that throws with nothing
written files `actioned: false`; one whose row landed and then threw files
`actioned: true, action: 'smite'`. The second was watched RED on pre-change
code. Its fake was also wrong — a successful `smitePlayer` fires the hook,
so a fake that merely resolves models a call that wrote nothing.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01E5Fk4Kuu5MjW1Ap51VWaPg

* fix(new-order): kill a surviving mutant on the third-strike path, and stop four comments overclaiming

Round-2 delta audit. The important one is the first; the rest are claims the
code does not support.

1. The seam guard could not see the worst path. `smite-durable-write.test.ts`
   named itself "before any of the non-durable tail runs" but asserted ordering
   inside `smitesCounter.increment` — the LAST tail step — while
   `mockCount.mockResolvedValue(1)` kept the third-strike branch unreachable in
   every case in the file. Moving `onSmiteCreated?.(smite)` below the
   `if (activeSmiteCount >= 3) return resetPlayer(...)` block survived the whole
   suite: on a third strike the branch returns early, the hook never fires, and
   an irreversible career reset files as actioned:false, "No action was taken by
   this scan" — the exact inverse invariant this work exists to remove.

   The guard now pins ordering against the FIRST tail step (the active-smite
   count), leaving no step for the hook to sink past, and adds a third-strike
   case that asserts the hook fired before resetPlayer's cleanse. Both go red on
   that mutation. The branch case checks the cleanse really ran and that the
   increment below the `if` was not reached, so it cannot pass vacuously.

2. toFinding's docblock stopped one sentence short. It correctly said
   actioned:true does not claim a career reset completed; it did not say that on
   a third strike resetPlayer cleanses every active smite INCLUDING the one just
   written. A moderator opening that row finds zero active smites and a reset
   account — a larger action than the row states. Behaviour unchanged, predates
   this work; the comment now names the observable.

3. The exported MAX_REASON_LENGTH's comment read as though importing it made
   producer truncation and parser cap equal by construction. It does for one
   producer of three: bot-account-detection and reaction-withdrawal-detection
   still declare their own literal. Reworded to say what was done, plus a
   ledger test pinning the set of producers holding a local copy — failing if it
   grows or shrinks. Migrating the other two is out of scope; each has its own
   suite pinning the literal.

4. truncateReason's comment said an over-limit reason "400s the REPORT".
   moderatorApp.abuseReport parses before the fetch, so it throws a ZodError in
   the job's own process and no request is made. toFinding's docblock 100 lines
   below already had this right. The same wrong phrasing is pre-existing house
   style in bot-account-detection/report.ts and was left alone.

5. The new parameter carried a prose precondition — "must not throw" — on a
   newly-exported seam past the point of no return. Wrapped in try/catch so a
   future caller's bug cannot strand an account with a live smite and no tail.
   The comment no longer claims a throw can abort anything; a test covers it.

6. report.test.ts called Number.MAX_SAFE_INTEGER "the ceiling for a JSON number
   off ClickHouse". It is not one — JSON.parse yields a double and a UInt64
   exceeds it by ~2,000x. The guard's conclusion is unaffected; only its stated
   reason was wrong, so the reason is replaced rather than the guard.

Verified: typecheck OK, 0 type errors. 93 tests pass across the 11 affected
suites (up from 87). Both mutations above were watched to fail and the guards
watched to kill them.

* fix(new-order): close a silent-pass hole in the ledger guard, contain async hook rejections, and stop three comments naming the wrong failure

Round-3 delta audit. Four items, scoped to nothing else.

1. max-reason-length-ledger: delete stripComments, read raw source.

   The stripper's block-comment regex was blind to string literals, so a producer
   whose report.ts carried the comment-opening pair inside any string (a URL, a
   route glob) opened a phantom comment that ran to the next terminator and
   deleted the real MAX_REASON_LENGTH declaration before LOCAL_DECLARATION ever
   saw it. That is a silent pass in the exact direction the guard exists for.

   It also bought nothing: LOCAL_DECLARATION anchors to whitespace-only before
   the keyword, so a leading comment marker can never match it, which was the
   only motive the stripper's docblock cited. Watched it work -- a synthetic
   producer of that shape passed green before and now fails the ledger by name.
   The residual risk inverts to the safe direction (an unprefixed declaration
   inside a block comment now over-reports, failing loudly).

   Also removed the docblock claim that the controls pin both directions -- they
   name the three known files by hand, so they say nothing about a new one -- and
   added the nearest-neighbour limit the list was missing: a copy under a
   different identifier survives green.

2. smitePlayer's onSmiteCreated seam: the try/catch delivered less than it said.

   (a) It was sync-only. TypeScript's void-return rule accepts an async function
   at a "=> void" position, so a rejecting hook produced a genuine unhandled
   rejection -- this repo installs no global unhandledRejection handler. Both
   shapes are now contained, and the order matters: Promise.resolve(hook())
   cannot catch a synchronous throw, because the hook is evaluated as the
   argument before Promise.resolve runs. The try covers that; a .catch on the
   result covers the rejection. The parameter type now admits Promise<void>
   explicitly rather than accepting it silently.

   (b) The swallow was unobservable and its stated reason was self-undermining.
   The catch was bare while handleLogError was already imported three lines
   below. Before this seam existed a hook throw reached the job's own
   handleLogError; afterwards it reached nothing. Now logged under a stable,
   opaque key with the id in the details.

   Mutation-checked both halves independently: removing the .catch fails only
   the rejection case (and Vitest reports the unhandled rejection directly);
   restoring the bare catch fails only the synchronous case.

3. The exported constant's note no longer claims a NEW producer cannot quietly
   copy the literal. It catches the common shape, not every shape -- a copy under
   a different name, or a producer laid out differently, stays green -- and it now
   says so instead of replacing one confident sentence with another.

4. "400s the REPORT" was wrong at all three live sites, not the one the previous
   round scoped. Every producer sends through moderatorApp.abuseReport, which
   runs abuseReportInput.parse before the fetch, so an over-long reason throws a
   ZodError locally and nothing is ever sent. The wrong wording sends a reader
   hunting a spoke-side 4xx that cannot exist. Fixed in both producer modules and
   the one test comment; re-enumerated to zero against a positive control.

Verification: typecheck 0 errors; 13 affected suites, 428 tests, all passing;
Prettier clean on all 7 changed files against a negative control.

* style(new-order): reflow the ledger docblock, and record why a naive glob does not reproduce the stripper hole

* fix(new-order): normalise a non-Error hook throw before logging, and widen the hook return type

`reportHookFailure` handed its value straight to `handleLogError`, which builds
`new Error(e.message ?? ...)` with no guard. A non-`Error` throw value therefore made the
LOGGER throw a TypeError, defeating the containment it was called from:

- sync `throw null` — the TypeError escaped the `catch` meant to contain it, so
  `smitePlayer` rejected and the tail (counter, signal, notification) never ran. That is
  exactly the half-applied smite this block exists to prevent: the row is committed, the
  derived state is not.
- async rejection — the TypeError left the `.catch` handler, so the derived promise
  rejected with nothing to catch it: an unhandled rejection.

Normalise with `e instanceof Error ? e : new Error(String(e))`. Two new cases cover both
shapes, asserting the tail completes and that an `Error` reaches the logger. Both were
watched failing against the previous commit first. The suite's `handleLogError` mock is now
faithful to the real function's unguarded deref — a bare `vi.fn()` accepts `null` happily,
which would have made both cases pass against code that throws in production.

Separately, `onSmiteCreated` returned `void | Promise<void>` and the docblock presented that
as purely permissive. It is the opposite: TypeScript's void-return exemption applies only to
a target of exactly `void`, and a union does not get it. Measured with tsc 5.9.2, the union
rejects an expression-bodied arrow whose body returns a value — including the idiomatic
`(s) => set.add(s.id)`, since `Set.add` returns the Set. `=> unknown` accepts every shape
probed; `void | Promise<unknown>` still rejects that one, so it was not taken. The docblock
is corrected, including the claim that spelling out `Promise<void>` is what prevents an
unhandled rejection — the `.catch` is what does that.

Finally, the comment asserting there is no global `unhandledRejection` handler "in this
repo" now says "in this process", which is what is actually checkable as written.

* fix(new-order): stop the hook-failure normaliser from throwing, and correct a test comment that stated the opposite of the test

The normalisation added last round used String(e), which itself throws for any
value whose primitive conversion throws — a null-prototype object, an object
with a throwing toString, a revoked Proxy. It runs inside the catch that exists
to contain the hook, so those shapes reproduced the very failure the block was
written to close: measured, smitePlayer rejected with the smite row already
committed and smitesCounter.increment took 0 calls.

Carry the value as `cause` instead. new Error(msg, { cause: e }) stores the
reference and reads no property of e, so it runs no user code for any throw
value; verified on node v24.19.0 that it is safe for all three shapes while
String(e) throws on all three. Object.prototype.toString.call(e) was rejected as
the alternative — same check shows it still throws on a revoked Proxy.

New red-first case, `Object.create(null)`: before the fix it failed with
"TypeError: Cannot convert object to primitive value" raised at the String(e)
site, and with the rejection absorbed the tail assertion read 0 calls.

Also corrects the mock comment in smite-durable-write.test.ts, which claimed the
faithful handleLogError mock is what keeps the two non-Error cases honest. It is
not. Re-measured here: with a bare vi.fn() AND the normalisation reverted both
cases still fail, at expect.any(Error) (-  Any<Error>  +  null). The mock adds
production-symptom fidelity on the sync case only — the failure presents as a
rejected promise rather than an argument mismatch; on the async case it changes
nothing. The comment now names expect.any(Error) as the guard and warns against
relaxing it to expect.anything(), which is the edit that would make both cases
vacuous. The file already said the true version 270 lines further down.

Finally, finishes the `=> void` retirement: the two local aliases in
auto-smite.test.ts still mirrored the parameter as `=> void` while the parameter
is `=> unknown`. Enumerated all 21 onSmiteCreated occurrences across 5 files;
exactly those 2 carried `=> void`, and both are now aligned.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01E5Fk4Kuu5MjW1Ap51VWaPg

* style(new-order): drop a line-position claim from the new test comment that was about evaluation order

* chore: retrigger CI after the PR-preview kubeconfig repair (talos-infra#1526)

---------

Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
This commit is contained in:
Zachary Lowden
2026-09-14 18:35:25 -05:00
committed by GitHub
parent 526fbabd53
commit 95562f7755
12 changed files with 1714 additions and 94 deletions
+27 -1
View File
@@ -54,6 +54,32 @@ const MAX_INT4 = 2_147_483_647;
*/
export const MAX_FINDINGS_PER_REPORT = 1_000;
/**
* Characters one finding's `reason` may carry.
*
* EXPORTED for the same structural reason as the cap above, one step earlier in the pipe: a producer
* that renders a reason has to truncate it to this bound BEFORE it calls, because an over-long reason
* does not lose the finding — it fails the parse and loses the whole report, every correctly-built
* finding beside it included. A producer that restates the literal holds a copy that can drift
* silently: lowering the bound here would leave it trimming to the old, now-invalid length, and the
* first sign of it would be that detector's runs vanishing from the board.
*
* ⚠️ IMPORTING THIS MAKES ONE PRODUCER'S TRUNCATION EQUAL TO THE PARSER'S CAP — IT DOES NOT MAKE
* THEM ALL EQUAL, and the export by itself enforces nothing. `new-order-abuse-detection` imports it;
* `bot-account-detection` and `reaction-withdrawal-detection` still declare their own local
* `MAX_REASON_LENGTH = 2_000`, and each has a suite pinning that literal. They are consistent with
* this value today by coincidence of the number, not by construction. The set of producers still
* holding a local copy is pinned as an explicit ledger in
* `src/server/services/new-order-abuse-detection/__tests__/max-reason-length-ledger.test.ts`, which
* fails if it grows or shrinks.
*
* That ledger catches the COMMON shape, not every shape. It matches the identifier
* `MAX_REASON_LENGTH` declared in a `src/server/services/<name>/report.ts`, so a copy under a
* different name (`const REASON_CAP = 2_000`), or one in a producer laid out differently, stays
* green. Read it as a tripwire on the likely case rather than a guarantee that no copy can appear.
*/
export const MAX_REASON_LENGTH = 2_000;
/** Declared once and used by both timestamp fields, so neither can regress without the other. */
const isoWithOffset = z.iso.datetime({ offset: true });
@@ -72,7 +98,7 @@ const abuseFinding = z
confidence: z.number().min(0).max(1),
// Why. The evidence-citing sentence, which is the whole value of the row to a moderator, so an
// empty one is not a finding.
reason: z.string().min(1).max(2_000),
reason: z.string().min(1).max(MAX_REASON_LENGTH),
// 🔴 Whether the producer ACTED. False is the common case and the interesting one: it is a
// detection the system chose not to act on, which is exactly what no existing surface can
// represent and what a human review queue needs.
@@ -8,12 +8,14 @@ const {
mockGetVotingRateLimitConfig,
mockSmitePlayer,
mockHandleLogError,
mockAbuseReport,
counterStub,
} = vi.hoisted(() => ({
mockClickhouseQuery: vi.fn().mockResolvedValue([]),
mockGetVotingRateLimitConfig: vi.fn().mockResolvedValue(null),
mockSmitePlayer: vi.fn().mockResolvedValue(undefined),
mockSmitePlayer: vi.fn(),
mockHandleLogError: vi.fn(),
mockAbuseReport: vi.fn().mockResolvedValue(undefined),
counterStub: {
increment: vi.fn(),
decrement: vi.fn(),
@@ -50,6 +52,13 @@ vi.mock('~/server/services/games/new-order.service', () => ({
clearRatedImages: vi.fn(),
}));
vi.mock('~/server/utils/errorHandling', () => ({ handleLogError: mockHandleLogError }));
// The abuse board sink. Stubbed rather than spread-over-actual because the real module builds a
// configured HTTP client at import time from `env`, and every assertion here is about the payload
// this job HANDS it — the contract validation that client performs is asserted directly, against
// `abuseReportInput`, in the report module's own suite.
vi.mock('~/server/services/moderator-app.service', () => ({
moderatorApp: { abuseReport: mockAbuseReport },
}));
vi.mock('~/server/services/buzz.service', () => ({
createBuzzTransactionMany: vi.fn(),
}));
@@ -62,7 +71,9 @@ vi.mock('~/utils/logging', () => ({ createLogger: () => () => undefined }));
vi.mock('~/env/server', () => ({ env: { DISCORD_WEBHOOK_MOD_ALERTS: undefined } }));
// Import AFTER mocks
import { abuseReportInput, type AbuseReportInput } from '@civitai/moderation';
import { runAbuseDetectionScan } from '~/server/jobs/new-order-jobs';
import { NEW_ORDER_ABUSE_DETECTOR } from '~/server/services/new-order-abuse-detection/report';
import { constants, newOrderConfig } from '~/server/common/constants';
import { loggingMock } from '~/__tests__/mocks/logging.mock';
import { dbMock } from '~/__tests__/mocks/db.mock';
@@ -71,6 +82,22 @@ const mockLogToAxiom = loggingMock.logToAxiom;
const SYSTEM_USER_ID = constants.system.user.id;
const AUTO_SMITE_SIZE = newOrderConfig.smiteSize * 50;
/**
* 🔴 A SUCCESSFUL `smitePlayer` FIRES `onSmiteCreated`, SO A FAKE THAT ONLY RESOLVES IS NOT ONE.
*
* The real service commits the smite row and invokes the hook there, before a tail of non-durable
* work that can throw with the penalty already live (measured in `smite-durable-write.test.ts`).
* The job records board membership from that hook, so a fake that resolves WITHOUT firing it models
* a call that wrote nothing — and every "this one smites" case here would silently be asserting the
* failure path while reading like the success one.
*/
type SmiteArgs = { playerId: number; onSmiteCreated?: (smite: { id: number }) => unknown };
let nextSmiteId = 1;
const smiteSucceeds = async (args: SmiteArgs) => {
args.onSmiteCreated?.({ id: nextSmiteId++ });
return undefined;
};
const strictSuspect = (overrides: Partial<Record<string, number>> = {}) => ({
userId: 100,
totalRatings: 200,
@@ -88,9 +115,20 @@ beforeEach(() => {
mockGetVotingRateLimitConfig.mockReset();
mockGetVotingRateLimitConfig.mockResolvedValue(null);
mockSmitePlayer.mockReset();
mockSmitePlayer.mockResolvedValue(undefined);
mockSmitePlayer.mockImplementation(smiteSucceeds);
mockAbuseReport.mockReset();
mockAbuseReport.mockResolvedValue(undefined);
});
/** The single report this run filed, parsed against the wire contract it will really be judged by. */
const filedReport = (): AbuseReportInput => {
expect(mockAbuseReport).toHaveBeenCalledTimes(1);
// 🔴 `.parse`, not a cast. `moderatorApp.abuseReport` parses before the network call, so a payload
// this job builds wrong throws THERE and the whole run is lost — mocking the client away would
// otherwise make every assertion below pass on a payload the board would never accept.
return abuseReportInput.parse(mockAbuseReport.mock.calls[0][0]);
};
describe('runAbuseDetectionScan auto-smite branch', () => {
it('does not smite when autoSmiteAbusers flag is off', async () => {
mockClickhouseQuery.mockResolvedValue([strictSuspect()]);
@@ -129,11 +167,14 @@ describe('runAbuseDetectionScan auto-smite branch', () => {
await runAbuseDetectionScan();
expect(mockSmitePlayer).toHaveBeenCalledTimes(1);
// The WHOLE argument set, not a subset — so dropping the durable-write hook fails here as well
// as in the outcome cases below, which is the cheaper place to notice it.
expect(mockSmitePlayer).toHaveBeenCalledWith({
playerId: 100,
modId: SYSTEM_USER_ID,
reason: expect.stringContaining('only 1 unique rating value'),
size: AUTO_SMITE_SIZE,
onSmiteCreated: expect.any(Function),
});
expect(mockLogToAxiom).toHaveBeenCalledWith(
expect.objectContaining({
@@ -217,9 +258,7 @@ describe('runAbuseDetectionScan auto-smite branch', () => {
await runAbuseDetectionScan();
expect(mockSmitePlayer).toHaveBeenCalledTimes(1);
expect(mockSmitePlayer).toHaveBeenCalledWith(
expect.objectContaining({ playerId: 500 })
);
expect(mockSmitePlayer).toHaveBeenCalledWith(expect.objectContaining({ playerId: 500 }));
});
it('honors custom smiteMaxUniqueRatings (allows 2-value spam as bot signal)', async () => {
@@ -258,16 +297,273 @@ describe('runAbuseDetectionScan auto-smite branch', () => {
autoSmiteAbusers: true,
});
mockSmitePlayer
.mockResolvedValueOnce(undefined)
.mockImplementationOnce(smiteSucceeds)
.mockRejectedValueOnce(new Error('db down'))
.mockResolvedValueOnce(undefined);
.mockImplementationOnce(smiteSucceeds);
await runAbuseDetectionScan();
expect(mockSmitePlayer).toHaveBeenCalledTimes(3);
// 🔴 The WHOLE key, not a substring, and the player id in the DETAILS. `handleLogError`'s second
// argument becomes the Axiom `name` an alert matches on; this used to interpolate the id into a
// free-text sentence there, which is both unmatchable and unbounded in cardinality — one distinct
// alert name per failing player. A `stringContaining` assertion would keep passing if someone put
// the sentence back.
expect(mockHandleLogError).toHaveBeenCalledWith(
expect.any(Error),
expect.stringContaining('auto-smite failed for player 401')
'new-order-abuse-detection:auto-smite-failed',
expect.objectContaining({ playerId: 401, smited: false })
);
});
});
/**
* The seam between the scan and the board.
*
* The report module's own suite proves the MAPPING is right; none of it can see whether this job
* hands the mapper the right arguments. These cases build the combined state — a run that smites, a
* run that does not, a run where a smite threw — and read the payload the client would have parsed.
*/
describe('runAbuseDetectionScan abuse-board report', () => {
it('files every suspect, not-actioned, when the auto-smite flag is off', async () => {
mockClickhouseQuery.mockResolvedValue([
strictSuspect({ userId: 100 }),
strictSuspect({ userId: 101 }),
]);
mockGetVotingRateLimitConfig.mockResolvedValue({
perMinute: 1,
perHour: 1,
perDay: 1,
autoSmiteAbusers: false,
});
await runAbuseDetectionScan();
const report = filedReport();
expect(report.detector).toBe(NEW_ORDER_ABUSE_DETECTOR);
expect(report.findings).toHaveLength(2);
for (const f of report.findings) {
expect(f.actioned).toBe(false);
expect(f.action ?? null).toBeNull();
}
expect(report.counters).toMatchObject({ suspects: 2, auto_smited: 0, filed_for_review: 2 });
});
it('marks the accounts it smited as actioned and the rest as open, in one report', async () => {
// 100 is a strict signal and gets smited; 300 is a soft signal (pace only) and does not.
mockClickhouseQuery.mockResolvedValue([
strictSuspect({ userId: 100, uniqueRatings: 1, dominantPct: 100 }),
strictSuspect({ userId: 300, uniqueRatings: 5, dominantPct: 40, avgPerMinute: 20 }),
]);
mockGetVotingRateLimitConfig.mockResolvedValue({
perMinute: 1,
perHour: 1,
perDay: 1,
autoSmiteAbusers: true,
});
await runAbuseDetectionScan();
expect(mockSmitePlayer).toHaveBeenCalledTimes(1);
const report = filedReport();
const byUser = new Map(report.findings.map((f) => [f.userId, f]));
expect(byUser.get(100)).toMatchObject({ actioned: true, action: 'smite' });
expect(byUser.get(100)?.reason).toContain('Auto-smited');
expect(byUser.get(300)?.actioned).toBe(false);
expect(byUser.get(300)).not.toHaveProperty('action');
expect(byUser.get(300)?.reason).toContain('No action was taken');
expect(report.counters).toMatchObject({ suspects: 2, auto_smited: 1, filed_for_review: 1 });
});
/**
* 🔴 THE TWO SHAPES OF "THE SMITE CALL THREW", AND THEY MUST BE ASSERTED AS A PAIR.
*
* `smitePlayer` commits the smite ROW and then does a pile of non-durable work — a count, a
* possible career reset, a Redis counter increment, a signal, a notification — any of which can
* throw with the penalty already live. So a throw says nothing on its own about whether the account
* was penalised, and the two cases want OPPOSITE rows on the board:
*
* - threw with NOTHING written → open case, `actioned: false`. Claiming otherwise tells a
* moderator an account was dealt with when it was not.
* - threw AFTER the row landed → a live penalty, `actioned: true`. Filing it open puts "No action
* was taken by this scan" beside a smite that already exists and invites a second one.
*
* Either case alone passes with the flag collapsed in the direction it happens to want — recording
* membership after the `await` passes the first and fails the second; recording it before the call
* passes the second and fails the first. The pair is what pins membership to the durable write.
*/
it('files a suspect whose smite threw with NOTHING WRITTEN as not-actioned', async () => {
// 401 was selected for smiting and the row was never created, so it is still an open case.
mockClickhouseQuery.mockResolvedValue([
strictSuspect({ userId: 400 }),
strictSuspect({ userId: 401 }),
]);
mockGetVotingRateLimitConfig.mockResolvedValue({
perMinute: 1,
perHour: 1,
perDay: 1,
autoSmiteAbusers: true,
});
// Rejects without ever invoking `onSmiteCreated` — the shape of the `create` itself failing.
mockSmitePlayer
.mockImplementationOnce(smiteSucceeds)
.mockRejectedValueOnce(new Error('db down'));
await runAbuseDetectionScan();
const report = filedReport();
const byUser = new Map(report.findings.map((f) => [f.userId, f]));
expect(byUser.get(400)).toMatchObject({ actioned: true, action: 'smite' });
expect(byUser.get(401)?.actioned).toBe(false);
expect(byUser.get(401)).not.toHaveProperty('action');
expect(byUser.get(401)?.reason).toContain('No action was taken');
expect(report.counters).toMatchObject({ auto_smited: 1, filed_for_review: 1 });
});
it('files a suspect whose smite ROW WAS WRITTEN but whose call then threw as actioned', async () => {
// 501's penalty is live in the database; only the non-durable tail failed. The board must say so,
// or a moderator reads an already-penalised account as an untouched one.
mockClickhouseQuery.mockResolvedValue([
strictSuspect({ userId: 500 }),
strictSuspect({ userId: 501 }),
]);
mockGetVotingRateLimitConfig.mockResolvedValue({
perMinute: 1,
perHour: 1,
perDay: 1,
autoSmiteAbusers: true,
});
mockSmitePlayer
.mockImplementationOnce(smiteSucceeds)
.mockImplementationOnce(
async (args: { onSmiteCreated?: (smite: { id: number }) => unknown }) => {
// The durable half succeeded — this is the signal `smitePlayer` fires the instant the row is
// committed — and the throw is everything after it.
args.onSmiteCreated?.({ id: 987 });
throw new Error('counter backend unavailable');
}
);
await runAbuseDetectionScan();
const report = filedReport();
const byUser = new Map(report.findings.map((f) => [f.userId, f]));
expect(byUser.get(500)).toMatchObject({ actioned: true, action: 'smite' });
expect(byUser.get(501)).toMatchObject({ actioned: true, action: 'smite' });
expect(byUser.get(501)?.reason).toContain('Auto-smited');
expect(byUser.get(501)?.reason).not.toContain('No action was taken');
expect(report.counters).toMatchObject({ suspects: 2, auto_smited: 2, filed_for_review: 0 });
// The failure is still recorded — a live penalty whose tail broke is not a silent success.
expect(mockHandleLogError).toHaveBeenCalledWith(
expect.any(Error),
'new-order-abuse-detection:auto-smite-failed',
expect.objectContaining({ playerId: 501, smited: true })
);
});
it('files a run that found nobody, so a quiet detector is distinguishable from a clean day', async () => {
mockClickhouseQuery.mockResolvedValue([]);
await runAbuseDetectionScan();
const report = filedReport();
expect(report.findings).toEqual([]);
expect(report.counters).toMatchObject({ suspects: 0 });
});
it('stamps a real, ordered pair of producer timestamps', async () => {
// The job recorded neither before this change; an unordered pair is refused by the contract and
// loses the whole run.
mockClickhouseQuery.mockResolvedValue([strictSuspect()]);
await runAbuseDetectionScan();
const report = filedReport();
expect(Date.parse(report.finishedAt)).toBeGreaterThanOrEqual(Date.parse(report.startedAt));
});
/**
* 🔴 THE TWO HALVES OF THE REPORT-FAILURE SPLIT, AND THEY MUST BE ASSERTED AS A PAIR.
*
* Either one alone passes with the branch collapsed in the direction it happens to want, so the
* pair is what pins the condition rather than the outcome: rethrow when the run produced nothing
* but the report, swallow only when smites are already written.
*
* The consequence of getting the first one wrong is invisible by construction. A swallowed failure
* on a run that smited nobody means the run produced no output at all and still returned success —
* the job's error counter never moves, so the detector can be dark indefinitely with no signal.
*/
it('PROPAGATES a report failure when the run smited nobody, so the job registers an error', async () => {
// The flag is off, so nothing was written and there is nothing a failed run would be retrying.
// This is the shape of the overwhelming majority of runs.
mockClickhouseQuery.mockResolvedValue([strictSuspect()]);
mockGetVotingRateLimitConfig.mockResolvedValue({
perMinute: 1,
perHour: 1,
perDay: 1,
autoSmiteAbusers: false,
});
mockAbuseReport.mockRejectedValue(new Error('400 bad request'));
await expect(runAbuseDetectionScan()).rejects.toThrow('400 bad request');
expect(mockSmitePlayer).not.toHaveBeenCalled();
});
it('SWALLOWS a report failure when smites are already written, and logs under a stable key', async () => {
// The one case the swallow is for: the enforcement half already happened, so failing the run
// asks for a retry of work that is done.
mockClickhouseQuery.mockResolvedValue([strictSuspect({ userId: 100 })]);
mockGetVotingRateLimitConfig.mockResolvedValue({
perMinute: 1,
perHour: 1,
perDay: 1,
autoSmiteAbusers: true,
});
mockAbuseReport.mockRejectedValue(new Error('400 bad request'));
await expect(runAbuseDetectionScan()).resolves.toBeUndefined();
expect(mockSmitePlayer).toHaveBeenCalledTimes(1);
// 🔴 The WHOLE key, not a substring. `handleLogError`'s second argument becomes the Axiom `name`
// an alert would match on, so a free-text sentence there is unalertable — which is what this
// used to pass. A `stringContaining` assertion would keep passing if someone put the sentence
// back around the key.
expect(mockHandleLogError).toHaveBeenCalledWith(
expect.any(Error),
'new-order-abuse-detection:report-failed',
expect.objectContaining({ smited: 1, suspects: 1 })
);
});
it('logs the scan to Axiom as an AGGREGATE, with no per-account array', async () => {
// 🔴 Pinned as the whole `details` key set, not as an absence check for today's field name: a
// check that only forbids `suspects` cannot see per-account detail coming back under any other
// key, and the failure would be a duplicated disclosure rather than a red test. Both precedents
// log run-level counts beside their board post; the per-account half belongs on the board, which
// renders it with attribution and reviewed-state that a log line has no way to carry.
mockClickhouseQuery.mockResolvedValue([
strictSuspect({ userId: 100, totalRatings: 200 }),
strictSuspect({ userId: 101, totalRatings: 50 }),
]);
await runAbuseDetectionScan();
const scanLog = mockLogToAxiom.mock.calls
.map((c: unknown[]) => c[0] as { name?: string; details?: Record<string, unknown> })
.find((p) => p?.name === 'new-order-abuse-detection-scan');
expect(scanLog, 'the scan must still log its run to Axiom').toBeDefined();
expect(Object.keys(scanLog?.details ?? {}).sort()).toEqual(['ratings', 'suspectCount']);
expect(scanLog?.details).toMatchObject({ suspectCount: 2, ratings: 250 });
});
it('posts to no Discord webhook', async () => {
// The surface this replaced. `fetch` is the only way this job could reach one, and nothing else
// in the scan calls it.
const fetchSpy = vi.spyOn(globalThis, 'fetch');
mockClickhouseQuery.mockResolvedValue([strictSuspect()]);
await runAbuseDetectionScan();
expect(fetchSpy).not.toHaveBeenCalled();
fetchSpy.mockRestore();
});
});
@@ -0,0 +1,403 @@
import { describe, it, expect, vi, beforeEach } from 'vitest';
/**
* 🔴 THE SEAM BETWEEN `smitePlayer` AND ANY CALLER THAT RECORDS WHETHER AN ACCOUNT WAS PENALISED.
*
* `smitePlayer` commits the smite ROW — the durable penalty — and then does a tail of non-durable
* work: an active-smite count, a possible career reset, a Redis counter increment, a signal, a
* notification. The counter increment is the sharp one: it calls `getCount`, which re-throws a
* non-connection ClickHouse error, and then writes to `sysRedis` UNGUARDED, so a Redis or ClickHouse
* blip throws out of `smitePlayer` with the penalty already live in Postgres.
*
* A caller that infers "was smited" from the call returning therefore under-counts a real penalty.
* `onSmiteCreated` exists to carry that fact out regardless, and `new-order-abuse-detection` files
* its board findings from it — an account it gets wrong reads as an open case beside a live smite,
* which invites a moderator to apply a second one.
*
* This file exercises the REAL `smitePlayer` (only the db, counters, signal and notification are
* mocked) so the abuse-scan suite's fake — which fires the hook and then throws — is a model of
* MEASURED behaviour rather than of an assumption. A suite that only ever tests its own fake cannot
* see the two drift apart.
*/
const {
smitesCounterStub,
counterStub,
mockCreateNotification,
mockSignalSend,
mockFetchThroughCache,
mockHandleLogError,
} = vi.hoisted(() => {
const stub = () => ({
increment: vi.fn(),
decrement: vi.fn(),
reset: vi.fn(),
getCount: vi.fn(),
getCountBatch: vi.fn(),
getAll: vi.fn(),
exists: vi.fn(),
key: 'stub',
});
return {
smitesCounterStub: stub(),
counterStub: stub(),
mockCreateNotification: vi.fn(),
mockSignalSend: vi.fn(),
mockFetchThroughCache: vi.fn(),
// 🔴 FAITHFUL TO THE REAL `handleLogError`, WHICH DEREFS `e.message` WITH NO GUARD
// (`src/server/utils/errorHandling.ts`) — BUT IT IS NOT WHAT MAKES THE NON-`Error` CASES GO RED.
// `expect.any(Error)` is: measured, with a bare `vi.fn()` here AND the normalisation reverted,
// those cases still fail, at the argument match (`- Any<Error> + null`). What the faithful
// mock adds is production-symptom fidelity on the SYNC case — the failure presents as a rejected
// promise, the way it would in production, instead of an argument mismatch. On the ASYNC case it
// changes nothing.
//
// So do not relax `expect.any(Error)` to `expect.anything()` on the strength of this mock. That
// is the single edit that makes both cases vacuous, and it is the one a reader who believes the
// mock is the guard would feel free to make.
mockHandleLogError: vi.fn((e: Error) => {
void new Error(e.message ?? 'Unexpected error occurred', { cause: e });
}),
};
});
// Heavy transitive deps the service imports at module load — stubbed so importing the real service
// doesn't drag in db/redis/env/otel/signal machinery. Mirrors `sanity-check-buffer.test.ts`.
vi.mock('~/server/games/new-order/utils', () => ({
acolyteFailedJudgments: counterStub,
allJudgmentsCounter: counterStub,
blessedBuzzCounter: counterStub,
correctJudgmentsCounter: counterStub,
expCounter: counterStub,
fervorCounter: counterStub,
pendingBuzzCounter: counterStub,
recentlyGrantedBuzzCounter: counterStub,
sanityCheckFailuresCounter: counterStub,
smitesCounter: smitesCounterStub,
poolCounters: {},
DEFAULT_POOL_QUOTAS: {},
checkVotingRateLimit: vi.fn(),
computePoolTargets: vi.fn(),
getActiveSlot: vi.fn(),
getImageRatingsCounter: vi.fn(),
getVotingCooldownUntil: vi.fn(),
getVotingRateLimitConfig: vi.fn(),
}));
vi.mock('~/server/clickhouse/client', () => ({ clickhouse: null }));
vi.mock('~/server/utils/errorHandling', () => ({
handleLogError: mockHandleLogError,
throwBadRequestError: vi.fn(),
throwInternalServerError: vi.fn(),
throwNotFoundError: vi.fn(),
throwRateLimitError: vi.fn(),
}));
vi.mock('~/server/utils/otel-helpers', () => ({
withSpan: (_n: string, fn: () => unknown) => fn(),
}));
vi.mock('~/server/utils/game-helpers', () => ({ getLevelProgression: vi.fn() }));
vi.mock('~/server/utils/cache-helpers', () => ({ fetchThroughCache: mockFetchThroughCache }));
vi.mock('~/server/utils/distributed-lock', () => ({ withDistributedLock: vi.fn() }));
vi.mock('~/server/services/image.service', () => ({
handleBlockImages: vi.fn(),
updateImageNsfwLevel: vi.fn(),
}));
vi.mock('~/server/services/notification.service', () => ({
createNotification: mockCreateNotification,
}));
vi.mock('~/server/services/report.service', () => ({ createReport: vi.fn() }));
vi.mock('~/server/services/user.service', () => ({ claimCosmetic: vi.fn() }));
vi.mock('~/utils/signal-client', () => ({
signalClient: { send: mockSignalSend, topicSend: vi.fn() },
}));
// Import AFTER mocks — the real `smitePlayer` stays in play.
import { smitePlayer } from '~/server/services/games/new-order.service';
import { dbMock } from '~/__tests__/mocks/db.mock';
const mockCreate = dbMock.dbWrite.newOrderSmite.create;
const mockCount = dbMock.dbWrite.newOrderSmite.count;
/**
* The cleanse inside `resetPlayer`'s transaction — the observable that the THIRD-STRIKE branch
* really executed. Asserting only that the hook fired would pass identically on the sub-threshold
* path, which is what makes the branch case vacuous without it.
*/
const mockCleanseSmites = dbMock.dbWrite.newOrderSmite.updateMany;
const SMITE_ROW = {
id: 4321,
targetPlayerId: 7,
givenById: 1,
reason: 'r',
size: 50,
remaining: 50,
};
// Mirrors the parameter's own type, `unknown`, so the async and non-`Error` cases below are real
// calls rather than ones smuggled past a narrower local alias.
const call = (onSmiteCreated?: (smite: { id: number }) => unknown) =>
smitePlayer({ playerId: 7, modId: 1, reason: 'r', size: 50, onSmiteCreated });
beforeEach(() => {
vi.clearAllMocks();
mockCreate.mockResolvedValue(SMITE_ROW);
// Below the third-strike rule, so the career-reset branch stays out of the way. The branch is NOT
// left untested by that — the dedicated case below raises this and asserts on it.
mockCount.mockResolvedValue(1);
smitesCounterStub.increment.mockResolvedValue(1);
mockSignalSend.mockResolvedValue(undefined);
mockCreateNotification.mockResolvedValue(undefined);
// `resetPlayer` reads the rank table through this cache on its way out; without a real-shaped
// answer it throws on `ranks.find` and the third-strike case would fail for the wrong reason.
mockFetchThroughCache.mockResolvedValue([
{ type: 'Acolyte', name: 'Acolyte', minExp: 0, iconUrl: null },
]);
});
describe('smitePlayer — onSmiteCreated is the durable-write signal', () => {
it('fires once with the committed row, before any of the non-durable tail runs', async () => {
const seen: Array<{ id: number }> = [];
// 🔴 Ordering is the claim, not just the call, and it is pinned against the FIRST tail step —
// the active-smite `count` — not against the counter increment further down.
//
// Why that distinction is the whole test: between the count and the increment sits
// `if (activeSmiteCount >= 3) return resetPlayer(...)`. A hook moved below that `if` is still
// above the increment, so an ordering assertion written against the increment holds and the
// move survives — while on the third strike, where the branch returns early, the hook now never
// fires at all. The severest outcome an account can reach would file as "No action was taken by
// this scan". Pinning the first tail step leaves no step for the hook to sink past unnoticed.
const order: string[] = [];
mockCount.mockImplementation(async () => {
order.push('count');
return 1;
});
smitesCounterStub.increment.mockImplementation(async () => {
order.push('increment');
return 1;
});
await call((smite) => {
order.push('hook');
seen.push(smite);
});
expect(order).toEqual(['hook', 'count', 'increment']);
expect(seen).toEqual([{ ...SMITE_ROW }]);
expect(smitesCounterStub.increment).toHaveBeenCalledTimes(1);
});
it('🔴 fires BEFORE a third-strike career reset — the severest path, and the one that returns early', async () => {
// The branch the rest of this file deliberately stays under, exercised here because it is the
// one where the hook is load-bearing and the one no other assertion can reach: `smitePlayer`
// RETURNS `resetPlayer(...)`, so every step below the `if` is skipped. A caller that learned
// "smited" from anything downstream of that branch learns nothing about a reset account.
mockCount.mockResolvedValue(3);
const order: string[] = [];
mockCleanseSmites.mockImplementation(() => {
order.push('cleanse');
return undefined;
});
await call(() => {
order.push('hook');
});
// The branch really ran. Without this the case is vacuous — "the hook fired" is equally true on
// the sub-threshold path, so the assertion would pass while testing nothing new.
expect(mockCleanseSmites).toHaveBeenCalledWith({
where: { targetPlayerId: 7, cleansedAt: null },
data: expect.objectContaining({ cleansedAt: expect.any(Date) }),
});
// …and the early return really happened: the increment below the `if` was never reached.
expect(smitesCounterStub.increment).not.toHaveBeenCalled();
expect(order).toEqual(['hook', 'cleanse']);
});
it('🔴 still fires when the tail THROWS — the penalty is live and the caller must hear about it', async () => {
// The measured mechanism behind the board bug: `smitesCounter.increment` re-throws a
// non-connection error out of `getCount` and then writes to `sysRedis` with no guard at all,
// unlike the fail-open `setCacheValue` beside it. The row is already committed at that point.
smitesCounterStub.increment.mockRejectedValue(new Error('counter backend unavailable'));
const seen: Array<{ id: number }> = [];
await expect(
call((smite) => {
seen.push(smite);
})
).rejects.toThrow('counter backend unavailable');
// Both halves, as a pair: the call failed AND the penalty landed. Either alone is the wrong
// story to tell a moderator.
expect(mockCreate).toHaveBeenCalledTimes(1);
expect(seen).toEqual([{ ...SMITE_ROW }]);
});
it('does NOT fire when the row itself was never written', async () => {
// The negative control. Without it, a hook wired to fire unconditionally would pass the case
// above and file every failed smite as an applied one — the exact inverse error.
mockCreate.mockRejectedValue(new Error('db down'));
const onSmiteCreated = vi.fn();
await expect(call(onSmiteCreated)).rejects.toThrow('db down');
expect(onSmiteCreated).not.toHaveBeenCalled();
});
it('a hook that THROWS does not abort the tail, and the failure is LOGGED', async () => {
// The precondition used to be prose on the parameter — "must not throw" — which is a request,
// not a guard, on a seam any future caller can reach. `smitePlayer` has already committed the
// penalty by this point, so a caller's own bug must not be able to strand the account with a
// live smite and no counter, signal or notification.
await expect(
call(() => {
throw new Error('caller hook exploded');
})
).resolves.not.toThrow();
expect(smitesCounterStub.increment).toHaveBeenCalledTimes(1);
expect(mockSignalSend).toHaveBeenCalledTimes(1);
// 🔴 CONTAINED IS NOT THE SAME AS SWALLOWED, and the catch was bare until this assertion existed.
// Before this seam was exported the throw reached the abuse-detection job's own `handleLogError`;
// a silent catch here removed that without replacing it, so a caller's bug became invisible on
// the one path where the penalty is already live. The KEY is asserted, not just the call: an
// alert can match a stable key and cannot match a sentence, and a bare `toHaveBeenCalled()`
// would pass on any unrelated log the tail happens to emit.
expect(mockHandleLogError).toHaveBeenCalledWith(
expect.objectContaining({ message: 'caller hook exploded' }),
'new-order:smite-hook-failed',
{ smiteId: SMITE_ROW.id }
);
});
it('🔴 a hook that REJECTS is contained too — the shape the `try` alone cannot reach', async () => {
// An `async` hook is the shape a future caller is most likely to write, and it is NOT covered by
// the `try` alone. `Promise.resolve(onSmiteCreated?.(smite))` cannot catch it either way round: a
// sync throw happens during argument evaluation, before `Promise.resolve` runs, while a
// rejection happens after the `try` block has already exited. Only the `.catch` on the result
// reaches it, and without that it is a genuine unhandled rejection — this process installs no
// global `unhandledRejection` handler to fall back on.
const unhandled: unknown[] = [];
const onUnhandled = (reason: unknown) => unhandled.push(reason);
process.on('unhandledRejection', onUnhandled);
try {
await expect(
call(async () => {
throw new Error('caller hook rejected');
})
).resolves.not.toThrow();
// The tail ran to completion, exactly as for the synchronous shape.
expect(smitesCounterStub.increment).toHaveBeenCalledTimes(1);
expect(mockSignalSend).toHaveBeenCalledTimes(1);
expect(mockHandleLogError).toHaveBeenCalledWith(
expect.objectContaining({ message: 'caller hook rejected' }),
'new-order:smite-hook-failed',
{ smiteId: SMITE_ROW.id }
);
// A macrotask turn, so a rejection left without a handler has actually been reported by the
// time this is read. Asserting it directly is what makes this a test of the defect rather than
// of the log line: the log could be produced and the rejection still escape.
await new Promise((resolve) => setTimeout(resolve, 0));
expect(unhandled).toEqual([]);
} finally {
process.off('unhandledRejection', onUnhandled);
}
});
it('🔴 contains a SYNCHRONOUS non-`Error` throw — `throw null`, which the logger cannot deref', async () => {
// `handleLogError` builds `new Error(e.message ?? …)` with no guard, so handing it `null`
// throws a TypeError from INSIDE the `catch` block — past the only handler there is. The tail
// is then skipped entirely and `smitePlayer` rejects, which is the half-applied smite the
// containment is there to prevent: the row is committed, the counter, signal and notification
// are not. `throw null` is not exotic — it is what a rethrown API payload or a bare
// `Promise.reject(err)` value looks like once it has been through a serialiser.
await expect(
call(() => {
throw null;
})
).resolves.not.toThrow();
// The tail really ran. This is the assertion the faithful `handleLogError` mock exists for —
// against a bare `vi.fn()` it would pass without the fix.
expect(smitesCounterStub.increment).toHaveBeenCalledTimes(1);
expect(mockSignalSend).toHaveBeenCalledTimes(1);
// …and the failure was still reported, as an `Error` the logger can actually consume. Pinning
// `expect.any(Error)` is the structural half: it fails on the raw `null` regardless of whether
// the mock happens to deref it.
expect(mockHandleLogError).toHaveBeenCalledWith(
expect.any(Error),
'new-order:smite-hook-failed',
{ smiteId: SMITE_ROW.id }
);
});
it('🔴 contains a throw value that cannot be STRINGIFIED — `Object.create(null)`', async () => {
// A different mechanism from `throw null` above, and it strikes first: there the logger could
// not deref the value, here the NORMALISATION cannot convert it. `String(e)` on a null-prototype
// object throws `TypeError: Cannot convert object to primitive value`, and it runs inside the
// `catch` that exists to contain the hook — so the tail is skipped and `smitePlayer` rejects
// with the row already committed, the same half-applied smite. An object with a throwing
// `toString` and a revoked `Proxy` are the same defect through the same line; this pins the
// shape that needs no setup to build.
await expect(
call(() => {
throw Object.create(null);
})
).resolves.not.toThrow();
// Both halves, as a pair. Before the value was carried as `cause`, the call rejected with
// `TypeError: Cannot convert object to primitive value` and this line read 0 — the tail skipped
// outright, which is the whole defect and not something the rejection alone establishes.
expect(smitesCounterStub.increment).toHaveBeenCalledTimes(1);
expect(mockSignalSend).toHaveBeenCalledTimes(1);
expect(mockHandleLogError).toHaveBeenCalledWith(
expect.any(Error),
'new-order:smite-hook-failed',
{ smiteId: SMITE_ROW.id }
);
});
it('🔴 contains an ASYNC non-`Error` rejection — the same value, arriving down the `.catch`', async () => {
// Same unguarded deref, reached the other way: the TypeError is thrown inside the `.catch`
// handler, so the derived promise rejects with nothing left to catch it. The `void` in front
// means it is never awaited, so this surfaces as an unhandled rejection rather than a failed
// call — the tail completes and the defect is invisible from the call site.
const unhandled: unknown[] = [];
const onUnhandled = (reason: unknown) => unhandled.push(reason);
process.on('unhandledRejection', onUnhandled);
try {
await expect(
call(async () => {
throw null;
})
).resolves.not.toThrow();
expect(smitesCounterStub.increment).toHaveBeenCalledTimes(1);
expect(mockSignalSend).toHaveBeenCalledTimes(1);
expect(mockHandleLogError).toHaveBeenCalledWith(
expect.any(Error),
'new-order:smite-hook-failed',
{ smiteId: SMITE_ROW.id }
);
// A macrotask turn, so a rejection left without a handler has been reported by the time this
// is read. Without it the assertion would run before the report and pass either way.
await new Promise((resolve) => setTimeout(resolve, 0));
expect(unhandled).toEqual([]);
} finally {
process.off('unhandledRejection', onUnhandled);
}
});
it('is optional — a caller that passes nothing is unaffected', async () => {
// Three of the four call sites do not hook this, so the absent case is the common one.
await expect(call()).resolves.toBeDefined();
expect(mockCreate).toHaveBeenCalledTimes(1);
});
});
+16 -17
View File
@@ -54,12 +54,7 @@ function createCounter<TId extends number | string = number | string>({
try {
await zAddWithTTL(sysRedis, key, value, id.toString(), ttl * 1000);
} catch (error) {
logSysRedisFailOpen(
'write-degraded',
'new-order.createCounter.zAdd',
error,
{ key, id }
);
logSysRedisFailOpen('write-degraded', 'new-order.createCounter.zAdd', error, { key, id });
}
} else {
await sysRedis.zAdd(key, { score: value, value: id.toString() });
@@ -70,12 +65,7 @@ function createCounter<TId extends number | string = number | string>({
try {
await hSetWithTTL(sysRedis, key, id.toString(), value, ttl * 1000);
} catch (error) {
logSysRedisFailOpen(
'write-degraded',
'new-order.createCounter.hSet',
error,
{ key, id }
);
logSysRedisFailOpen('write-degraded', 'new-order.createCounter.hSet', error, { key, id });
}
} else {
await sysRedis.hSet(key, id.toString(), value);
@@ -826,11 +816,20 @@ export type VotingRateLimitConfig = {
/** Have the daily abuse-detection job auto-smite suspects matching strict signals. Off by default. */
autoSmiteAbusers?: boolean;
/**
* Tunable thresholds for the daily abuse-detection job. Live values come
* from Redis so they're not leaked via the public source tree — defaults
* here are a non-load-bearing fallback for first-boot before ops seeds the
* config. Calibrate against real queue composition and revisit as the
* NSFW sampling rate / pool mix shifts.
* Tunable thresholds for the daily abuse-detection job. Live values come from
* Redis so ops can retune them without a deploy — defaults here are a
* non-load-bearing fallback for first-boot before ops seeds the config.
*
* ⚠️ This said the Redis indirection keeps the values "not leaked via the
* public source tree". It does not, and treating it as a confidentiality
* control is the mistake to avoid: this repo is public, the fallbacks below
* are literals in it, a checked-in test of the smite path carries live values,
* and the abuse board publishes the observed numbers these are compared
* against, which bound them from above over a few runs. Redis buys
* retune-without-deploy, nothing more.
*
* Calibrate against real queue composition and revisit as the NSFW sampling
* rate / pool mix shifts.
*/
abuseDetection?: {
/** HAVING totalRatings >= X — minimum daily vote count to be considered. */
+141 -59
View File
@@ -1,4 +1,3 @@
import { env } from '~/env/server';
import dayjs from '~/shared/utils/dayjs';
import { chunk } from 'lodash-es';
import { clickhouse } from '~/server/clickhouse/client';
@@ -28,6 +27,12 @@ import {
processFinalRatings,
smitePlayer,
} from '~/server/services/games/new-order.service';
import { moderatorApp } from '~/server/services/moderator-app.service';
import {
ABUSE_SCAN_WINDOW_HOURS,
buildAbuseReport,
type AbuseSuspect,
} from '~/server/services/new-order-abuse-detection/report';
import { limitConcurrency } from '~/server/utils/concurrency-helpers';
import { handleLogError } from '~/server/utils/errorHandling';
import { TransactionType } from '~/shared/constants/buzz.constants';
@@ -497,16 +502,35 @@ const newOrderChangeRateTarget = createJob(
);
// Periodic abuse detection: identify users with suspicious rating patterns.
// Runs daily at 23:00 UTC, logs to Axiom and Discord for monitoring, and (when
// `autoSmiteAbusers` is enabled in Redis config) auto-smites suspects matching
// strict signals via the system actor.
// Runs daily at 23:00 UTC, logs to Axiom, files every suspect on the moderator
// app's abuse-detection board, and (when `autoSmiteAbusers` is enabled in Redis
// config) auto-smites suspects matching strict signals via the system actor.
//
// 🔴 THE BOARD POST HAPPENS AFTER THE SMITE LOOP, AND THE ORDER IS LOAD-BEARING.
// Each finding carries the contract's `actioned`/`action` pair, which is a claim
// about what this run already DID — so the findings cannot be built until the
// smiting is over and it is known which accounts it actually succeeded on. The
// contract rejects a mispaired finding on the producer's side of the wire, and
// one rejection loses the whole batch, not the offending row.
//
// Replaces the Discord webhook this scan used to post to: the board is durable,
// reviewable, and the only surface that can represent "detected and deliberately
// not acted on", which is most of what this scan produces.
export async function runAbuseDetectionScan() {
// Captured before the first await so the board's "when did this run" reading is
// the producer's own start, not the instant the report happened to be built.
const startedAt = new Date();
if (!clickhouse) return;
log('AbuseDetection :: Scanning for suspicious rating patterns');
// All tunable thresholds live in Redis so the operational values aren't
// visible to anyone reading the public source tree. Defaults below are a
// first-boot fallback; once ops seeds the config, those take precedence.
// All tunable thresholds live in Redis, so the live operational values are not
// set from this file. ⚠️ That is a DEPLOYMENT fact, not a secrecy guarantee, and
// this comment used to claim the second: this repo is public, the fallbacks
// below are literals in it, a checked-in test of the smite path carries live
// values, and the observed numbers this scan now publishes on the abuse board
// bound the thresholds from above over a few runs. Treat them as operationally
// convenient to retune, not as hidden. Defaults below are a first-boot
// fallback; once ops seeds the config, those take precedence.
// Every value is coerced to a finite number before being interpolated into
// the ClickHouse query because `formatSqlType` passes strings through
// unquoted — a non-numeric value sneaking into the config blob would
@@ -524,20 +548,13 @@ export async function runAbuseDetectionScan() {
const smiteDominantPct = asFinite(det.smiteDominantPct, 100);
const smiteMaxUniqueRatings = asFinite(det.smiteMaxUniqueRatings, 1);
const suspects = await clickhouse.$query<{
userId: number;
totalRatings: number;
uniqueRatings: number;
dominantRating: number;
dominantPct: number;
avgPerMinute: number;
}>`
const suspects = await clickhouse.$query<AbuseSuspect>`
WITH user_dominant AS (
SELECT
userId,
topK(1)(rating)[1] as dominantRating
FROM knights_new_order_image_rating FINAL
WHERE createdAt >= now() - INTERVAL 24 HOUR
WHERE createdAt >= now() - INTERVAL ${ABUSE_SCAN_WINDOW_HOURS} HOUR
AND rank != 'Acolyte'
GROUP BY userId
)
@@ -550,7 +567,7 @@ export async function runAbuseDetectionScan() {
count() / greatest(uniq(toStartOfMinute(r.createdAt)), 1) as avgPerMinute
FROM knights_new_order_image_rating r FINAL
JOIN user_dominant d ON r.userId = d.userId
WHERE r.createdAt >= now() - INTERVAL 24 HOUR
WHERE r.createdAt >= now() - INTERVAL ${ABUSE_SCAN_WINDOW_HOURS} HOUR
AND r.rank != 'Acolyte'
GROUP BY r.userId, d.dominantRating
HAVING totalRatings >= ${minTotalRatings}
@@ -559,56 +576,31 @@ export async function runAbuseDetectionScan() {
LIMIT 50
`;
// The accounts this run wrote a smite ROW for — MEMBERSHIP, not selection, and keyed on the
// durable write rather than on the smite call returning. The loop below swallows a per-player
// failure and carries on; a target whose row was never written is still an open case and must not
// be filed on the board as one that was dealt with, and a target whose row WAS written must not be
// filed as open just because the non-durable tail of `smitePlayer` threw after it.
const smitedUserIds = new Set<number>();
if (suspects.length > 0) {
log(`AbuseDetection :: Found ${suspects.length} suspicious users`);
await logToAxiom({
type: 'warning',
name: 'new-order-abuse-detection-scan',
// AGGREGATE ONLY. This used to carry a per-account array, which neither precedent does — both
// `reaction-withdrawal-detection` and `bot-account-detection` log run-level counts beside their
// board post and nothing else. The per-account detail now has a better home: the board renders
// all six columns per finding, plus attribution and reviewed-state, which a log line cannot.
// Keeping a second copy here duplicated the sensitive half of the payload into a surface with
// different retention and no review workflow, to say something the board already says better.
details: {
suspectCount: suspects.length,
suspects: suspects.map((s) => ({
userId: s.userId,
totalRatings: s.totalRatings,
uniqueRatings: s.uniqueRatings,
dominantRating: s.dominantRating,
dominantPct: Math.round(s.dominantPct),
avgPerMinute: Math.round(s.avgPerMinute * 10) / 10,
})),
ratings: suspects.reduce((sum, s) => sum + s.totalRatings, 0),
},
message: `Abuse detection scan found ${suspects.length} suspicious users in the last 24 hours`,
message: `Abuse detection scan found ${suspects.length} suspicious users in the last ${ABUSE_SCAN_WINDOW_HOURS} hours`,
}).catch(() => null);
// Alert moderators via Discord webhook
if (env.DISCORD_WEBHOOK_MOD_ALERTS) {
const suspectLines = suspects
.slice(0, 10) // Cap at 10 to keep the embed manageable
.map(
(s) =>
`• **User ${s.userId}** — ${s.totalRatings} votes, ${s.uniqueRatings} unique rating(s), ` +
`${Math.round(s.dominantPct)}% same value, ${(
Math.round(s.avgPerMinute * 10) / 10
).toFixed(1)}/min`
)
.join('\n');
await fetch(env.DISCORD_WEBHOOK_MOD_ALERTS, {
method: 'POST',
headers: { 'Content-Type': 'application/json' },
body: JSON.stringify({
embeds: [
{
title: `⚠️ KoN Abuse Detection (24h) — ${suspects.length} suspect(s)`,
description:
suspectLines +
(suspects.length > 10 ? `\n... and ${suspects.length - 10} more` : ''),
color: 0xff9800,
timestamp: new Date().toISOString(),
},
],
}),
}).catch(() => null);
}
// Auto-smite branch: gated by Redis config flag (off by default). Smite
// filter applies the tighter `smite*` thresholds against the broader
// detection pool; the `having*` thresholds (looser) feed the alert path
@@ -636,6 +628,20 @@ export async function runAbuseDetectionScan() {
modId: constants.system.user.id,
reason,
size: newOrderConfig.smiteSize * 50,
// 🔴 MEMBERSHIP IS RECORDED FROM THE DURABLE WRITE, NOT FROM THE CALL RETURNING.
//
// `smitePlayer` commits the smite row FIRST and then does a pile of non-durable work —
// an active-smite count, a possible career reset, a Redis counter increment, a signal,
// a notification. A throw anywhere in that tail leaves the penalty live in Postgres.
// Recording membership after the `await` would then omit an account that IS smited, and
// the board would file it as an open case with "No action was taken by this scan" on a
// player who had just been penalised — inviting a moderator to apply a second one.
//
// The hook fires the instant the row is committed, so the set means exactly "a smite
// row exists for this account because of this run". See `smitePlayer`'s own comment.
onSmiteCreated: () => {
smitedUserIds.add(s.userId);
},
});
await logToAxiom({
type: 'warning',
@@ -644,19 +650,95 @@ export async function runAbuseDetectionScan() {
message: `Auto-smite issued for player ${s.userId}`,
}).catch(() => null);
} catch (e) {
handleLogError(e as Error, `auto-smite failed for player ${s.userId}`);
// 🔴 A STABLE KEY, WITH THE ID IN THE DETAILS. `handleLogError`'s second argument becomes
// the Axiom `name`: an alert can match a key and cannot match a sentence, and an
// interpolated player id makes every failure its own unbounded, unmatchable name. Same
// rule, same spelling as `new-order-abuse-detection:report-failed` below.
//
// `smited` records whether the penalty landed anyway — the row can be written and the
// call still throw, and those two failures want different responses.
handleLogError(e as Error, 'new-order-abuse-detection:auto-smite-failed', {
playerId: s.userId,
smited: smitedUserIds.has(s.userId),
});
}
}
}
} else {
log('AbuseDetection :: No suspicious users found');
}
// Filed even with no suspects: a run row with zero findings is how the board says "this detector
// ran and found nothing", which is a different and necessary claim from the detector having gone
// quiet — and the counters carry the population it looked at either way.
//
// 🔴 RETHROWN WHEN THERE IS NOTHING TO PROTECT, SWALLOWED ONLY WHEN THERE IS.
//
// This report is the detector's only durable output. On a run that smited nobody — which is the
// overwhelming majority of runs, because enforcement here is rare — swallowing a failure means the
// run produced NOTHING and still reported success: the job's error counter stays flat, every
// success signal is unchanged, and the detector is dark with nothing anywhere to say so. That is
// the failure mode this branch exists to make visible, and the unconditional catch it replaces was
// paying it on nearly every run to cover a case that nearly never occurs.
//
// The swallow survives for the one genuinely awkward shape: smites are already written to the
// database, so failing the run asks for a retry of work whose enforcement half already happened.
// ⚠️ Whether a failed run is retried AT ALL could not be established — the scheduler that calls
// `/api/webhooks/run-jobs` lives outside this repo, and the route itself has no retry logic; it
// returns 500 and stops. So this branch is a precaution against a retry we have not confirmed
// exists, not a response to a measured one. If someone establishes the scheduler does not retry a
// 500, the right move is to delete the split and rethrow unconditionally.
//
// The log key is stable and opaque, in the precedents' style (`bot-account-detection:report-failed`)
// rather than the free-text sentence this used to pass: an alert can match a key and cannot match
// a sentence.
try {
await moderatorApp.abuseReport(
buildAbuseReport({ suspects, smitedUserIds, startedAt, finishedAt: new Date() })
);
log(
`AbuseDetection :: Filed ${suspects.length} finding(s) on the abuse board ` +
`(${smitedUserIds.size} auto-smited)`
);
} catch (e) {
if (smitedUserIds.size === 0) throw e;
handleLogError(e as Error, 'new-order-abuse-detection:report-failed', {
smited: smitedUserIds.size,
suspects: suspects.length,
});
}
}
const newOrderAbuseDetection = createJob(
'new-order-abuse-detection',
'0 23 * * *',
runAbuseDetectionScan
runAbuseDetectionScan,
// ⚠️ A WIDENING, NOT AN INTRODUCTION. `createJob` already defaults every job to a 5-minute
// `lockExpiration` (see `job.ts`), so this job was never unlocked — an earlier version of this
// comment and of the PR description both said it was, and that was wrong.
//
// Why 10 rather than the inherited 5: it matches the sibling detector that writes the same board,
// `reaction-withdrawal-detection`, and nothing more. NO measurement supports either number — this
// scan has never been observed running past 5 minutes, so the widening is precautionary and its
// size is borrowed, not derived. If a run ever does exceed the lock, the fix is to measure the run
// and size it from that, not to widen again by analogy.
//
// What the lock is worth, which is the part that IS established: the run-jobs route caps the hold
// at exactly this value and then releases it while the run continues, so past that point a retry
// can start a second concurrent run.
//
// 🔴 AND THE WORST CASE OF THAT IS DOUBLE ENFORCEMENT, NOT A DUPLICATED PAGE. An earlier version of
// this comment said a second run only means "a moderator sees the same cohort twice"; that is the
// cosmetic half and it understated the rest. `smitePlayer` is NOT idempotent — every call INSERTS
// another smite row, so a concurrent run smites the same cohort a second time, and on the account
// that the second row carries to the third-strike rule `smitePlayer` chains into `resetPlayer`,
// which wipes that player's New Order career and notifies them. That is an irreversible penalty
// applied because a lock expired, and nothing downstream de-duplicates it.
//
// The cosmetic half is real too: each run files its own board report, the receiving table's
// idempotency key is `(detector, started_at)`, and two different start instants APPEND a
// near-identical run rather than replacing the first.
{ lockExpiration: 10 * 60 }
);
export const newOrderJobs = [
@@ -31,10 +31,12 @@ type AbuseFinding = AbuseReportInput['findings'][number];
const MAX_REASON_LENGTH = 2_000;
/**
* 🔴 A reason over the contract's limit does not lose the finding, it 400s the REPORT and loses
* every finding in the batch. So it is truncated here rather than left to fail, and the ellipsis is
* the record that something was cut. Reason text is generated from bounded facts plus a username and
* a per-heuristic list, both of which can grow.
* 🔴 A reason over the contract's limit does not lose the finding, it loses the whole REPORT every
* finding in the batch with it. The failure is LOCAL, not a spoke 4xx: `moderatorApp.abuseReport`
* runs `abuseReportInput.parse(input)` before the fetch, so an over-long reason throws a ZodError in
* this process and nothing is ever sent. So it is truncated here rather than left to fail, and the
* ellipsis is the record that something was cut. Reason text is generated from bounded facts plus a
* username and a per-heuristic list, both of which can grow.
*/
export function truncateReason(reason: string, max = MAX_REASON_LENGTH): string {
if (reason.length <= max) return reason;
+79 -2
View File
@@ -195,7 +195,42 @@ export async function smitePlayer({
modId,
reason,
size,
}: SmitePlayerInput & { modId: number }) {
onSmiteCreated,
}: SmitePlayerInput & {
modId: number;
/**
* 🔴 THE DURABLE-WRITE SIGNAL, AND THE ONLY THING THAT CAN CARRY IT OUT OF A FAILED CALL.
*
* Called once, synchronously, the instant the smite ROW is committed before the active-smite
* count, before any career reset, before the counter increment, the signal and the notification.
* Everything past the `create` is either derived state or best-effort delivery; the row is the
* penalty, and once it exists the player is smited whether or not this function returns.
*
* A return value cannot express that, because the case that matters is the one where there IS no
* return: `smitesCounter.increment` re-throws a non-connection ClickHouse error out of `getCount`
* and then writes to `sysRedis` unguarded, so a Redis blip throws AFTER the penalty is live. A
* caller that records "smited" from the call succeeding therefore under-counts a live penalty; a
* caller that hooks this over-counts nothing, because the row is already committed when it fires.
*
* Optional, and no existing caller passes it behaviour for everyone else is unchanged.
*
* Keep it to recording a fact. Both failure shapes are contained at the call site and logged under
* `new-order:smite-hook-failed`: a synchronous throw by the `try`, and a rejected promise by a
* `.catch` on the result. The `.catch` is what stops a rejection going unhandled; the return type
* documents the async case but cannot contain it.
*
* The return type is `unknown` because the value is discarded. A narrower `void | Promise<void>`
* is not more permissive but LESS TypeScript's void-return exemption applies only to a target of
* exactly `void`, and a union does not get it, so it rejects an expression-bodied arrow whose body
* returns a value. Measured with tsc 5.9.2: that rejects `(s) => smitedUserIds.add(s.id)`, because
* `Set.add` returns the Set.
*
* TWO LIMITS. The tail does NOT await an async hook, so its write may land after this function
* returns and it cannot be depended on for ordering. And a hook that fails still records nothing
* the log is the only trace, so the caller is back to not knowing.
*/
onSmiteCreated?: (smite: { id: number }) => unknown;
}) {
const smite = await dbWrite.newOrderSmite.create({
data: {
targetPlayerId: playerId,
@@ -205,6 +240,46 @@ export async function smitePlayer({
remaining: size,
},
});
// The tail must not depend on a caller's hook. This is a newly-exported seam on a function that
// has already committed the penalty, and the prose asking callers not to throw was the only thing
// holding — structural here, so a future caller cannot turn its own bug into a half-applied smite.
//
// 🔴 BOTH SHAPES, AND THE ORDER MATTERS. `Promise.resolve(onSmiteCreated?.(smite))` cannot catch a
// SYNCHRONOUS throw on its own: the hook is evaluated as the argument, so it throws before
// `Promise.resolve` is ever called and before any `.catch` is attached. The `try` covers that one;
// the `.catch` covers a rejected promise, which an `async` hook produces and which would otherwise
// be an unhandled rejection — there is no global `unhandledRejection` handler in this process to
// fall back on.
//
// Logged, not swallowed. A stable, opaque key in this file's established style, so an alert can
// match it; the id goes in the details rather than the name, which would make every failure its
// own unmatchable key. The rationale this replaced — "the hook's own failure is the hook's to
// report" — cannot hold: a hook that threw has by construction not reported. Before this seam
// existed the throw reached the job's own `handleLogError`; without a log here it now reaches
// nothing at all.
// Normalised, not cast. `handleLogError` reads `e.message` with no guard, so a non-`Error` throw
// value — `throw null`, a rejected non-`Error` payload — makes the LOGGER throw a TypeError: out
// of the `catch` below on the sync path, taking the tail with it, and out of the `.catch` on the
// async path as an unhandled rejection. Both are the failures this block exists to prevent.
//
// 🔴 AND THE NORMALISATION ITSELF MUST NOT THROW, which is why the value is carried as `cause`
// rather than stringified. `String(e)` reintroduces exactly those two failures for any value whose
// primitive conversion throws — a null-prototype object, an object with a throwing `toString`, a
// revoked `Proxy` — because it runs INSIDE the handler that is supposed to contain them; measured,
// the tail was skipped and the counter took 0 calls. `new Error(msg, { cause: e })` stores the
// reference and reads no property of `e`, so it runs no user code for any throw value.
// `Object.prototype.toString.call(e)` is NOT equivalent: it still throws on a revoked `Proxy`.
const reportHookFailure = (e: unknown) =>
handleLogError(
e instanceof Error ? e : new Error('non-Error hook throw', { cause: e }),
'new-order:smite-hook-failed',
{ smiteId: smite.id }
);
try {
void Promise.resolve(onSmiteCreated?.(smite)).catch(reportHookFailure);
} catch (e) {
reportHookFailure(e);
}
const activeSmiteCount = await dbWrite.newOrderSmite.count({
where: { targetPlayerId: playerId, cleansedAt: null },
@@ -1896,7 +1971,9 @@ export async function getPlayerHistory({
// page 1 — acceptable for a history view.)
if (cursor)
HAVING.push(
`(max(createdAt), imageId) < (parseDateTimeBestEffort('${cursor.createdAt.toISOString()}'), ${cursor.imageId})`
`(max(createdAt), imageId) < (parseDateTimeBestEffort('${cursor.createdAt.toISOString()}'), ${
cursor.imageId
})`
);
const judgments = await clickhouse.$query<{
@@ -0,0 +1,148 @@
import { describe, it, expect } from 'vitest';
import fs from 'node:fs';
import path from 'node:path';
import { MAX_REASON_LENGTH } from '@civitai/moderation';
/**
* Drift guard for the `reason` cap across every abuse-report PRODUCER.
*
* WHAT IT PINS: the RELATIONSHIP between the contract's `MAX_REASON_LENGTH` and the producers that
* truncate to it. Each producer's own suite pins its own truncation, and that is exactly what none
* of them can see: a producer restating the literal agrees with the contract only while the two
* numbers happen to match. Lowering the contract's bound would leave that producer trimming to a
* length the parser now rejects, and the first sign of it is that detector's runs vanishing from the
* board rather than any test here going red.
*
* WHAT IT IS NOT stated plainly, because the export it guards was described once as making the
* caps "equal by construction" and that was true of one producer out of three:
*
* - It does NOT make the two local copies correct. They are consistent with the contract today by
* coincidence of the number. Migrating them is deliberate work with its own suites to update, and
* was left out of the change that added this file.
* - It is a SOURCE-TEXT check over a directory listing, not a type or dependency check. It cannot
* see a cap reached indirectly a bound read from a helper, a re-export, an object field, or a
* producer that lives somewhere other than `src/server/services/<name>/report.ts`.
* - It matches the IDENTIFIER, so the nearest-neighbour shape walks straight past it: a producer
* that writes `const REASON_CAP = 2_000` holds exactly the copy this file exists to catch and
* stays off the ledger, green. Pinning the literal instead would fire on every unrelated 2_000 in
* a producer, so the name is the tractable half but it is the half a rename defeats.
* - A green run says the SET did not change. It says nothing about whether the values agree; the
* behavioural pinning of the imported bound lives in `report.test.ts` beside this file.
*
* So: a tripwire that forces a conscious decision on the common shape, in both directions.
*/
/** `src/server/services` — the parent of every producer directory. */
const SERVICES_DIR = path.resolve(__dirname, '../..');
/**
* Every producer that still declares its OWN `MAX_REASON_LENGTH` instead of importing the contract's.
*
* Adding a name here is a deliberate act and so is removing one: the assertions below fail if
* reality and this list diverge in EITHER direction. Growing means a new producer copied the
* literal; shrinking means one was migrated and this ledger plus the note on the exported constant
* in `packages/civitai-moderation/src/schema.ts` is now overclaiming and must be updated with it.
*/
const LOCAL_DECLARATION_LEDGER = ['bot-account-detection', 'reaction-withdrawal-detection'].sort();
/**
* Producers known to exist at all. Separate from the ledger above and asserted separately, because
* a discovery wired to nothing returns an empty set that satisfies an "equals the ledger" check only
* when the ledger is empty but returns an empty set that looks like PROGRESS once the ledger
* shrinks to zero. Pinning the population makes a scan of no files loud instead of reassuring.
*/
const KNOWN_PRODUCERS = [
'bot-account-detection',
'new-order-abuse-detection',
'reaction-withdrawal-detection',
].sort();
/**
* A DECLARATION of the identifier, not a mention of it, and applied to RAW source.
*
* The `[ \t]*` between the line anchor and the keyword admits whitespace and nothing else, which is
* what makes reading raw source safe for the ordinary comment shapes: `/` is not `[ \t]`, so neither
* a commented-out `// const MAX_REASON_LENGTH = 2_000` nor a JSDoc ` * const MAX_REASON_LENGTH` can
* satisfy it, and a trailing `foo(); // const MAX_REASON_LENGTH` fails the anchor outright. All
* three of these files name the identifier in prose and none of those mentions match.
*
* It is NOT comment-aware, and that is deliberate. A previous version stripped comments first with
* `/\/\*[\s\S]*?\*\//g`, which is blind to string literals: a producer whose `report.ts` contained
* `/*` inside any string a URL, a route glob opened a phantom comment that ran to the next
* block-comment terminator and deleted the real declaration before this regex ever saw it, so the
* producer passed green. That is a SILENT PASS in the exact direction this file exists to prevent,
* bought for nothing, since the anchor above already handles every motive the stripper cited.
* (Note for anyone rebuilding the case: a recursive-directory glob does NOT reproduce it there the
* opening pair is immediately followed by a terminator, so the phantom comment closes inside the
* string. The opening pair has to be the LAST thing in the string, as in a trailing route wildcard.)
*
* The residual risk runs the other way and is the safe one: an unprefixed `const MAX_REASON_LENGTH`
* inside a block comment now matches, so the ledger GROWS and the assertion fails loudly. A guard
* that over-reports gets looked at; one that under-reports does not.
*/
const LOCAL_DECLARATION =
/(?:^|\n)[ \t]*(?:export[ \t]+)?(?:const|let|var)[ \t]+MAX_REASON_LENGTH\b/;
/** Every `src/server/services/<name>/report.ts`, by directory name. */
function discoverProducers(): string[] {
return fs
.readdirSync(SERVICES_DIR, { withFileTypes: true })
.filter((e) => e.isDirectory() && fs.existsSync(path.join(SERVICES_DIR, e.name, 'report.ts')))
.map((e) => e.name)
.sort();
}
const read = (name: string) => fs.readFileSync(path.join(SERVICES_DIR, name, 'report.ts'), 'utf8');
describe('MAX_REASON_LENGTH — producer drift ledger', () => {
it('finds the producer files it claims to scan', () => {
// 🔴 The positive control. Without it every assertion below is satisfiable by scanning zero
// files, and a guard that scans zero files passes forever — the shape this whole file exists to
// catch in the code it guards.
const found = discoverProducers();
expect(found).toEqual(expect.arrayContaining(KNOWN_PRODUCERS));
expect(found.length).toBeGreaterThanOrEqual(KNOWN_PRODUCERS.length);
// And it read real bytes, not empty strings a regex would never match.
for (const name of KNOWN_PRODUCERS) expect(read(name).length).toBeGreaterThan(100);
});
it('detects a local declaration where one exists — the regex is not wired to nothing', () => {
// The other half of the control: `LOCAL_DECLARATION` must actually match the known copies. A
// regex that matched nothing would report a clean ledger of zero, which reads as the migration
// being complete.
for (const name of LOCAL_DECLARATION_LEDGER) {
expect(LOCAL_DECLARATION.test(read(name))).toBe(true);
}
});
it('🔴 no producer outside the ledger declares its own MAX_REASON_LENGTH', () => {
const withLocalCopy = discoverProducers().filter((name) => LOCAL_DECLARATION.test(read(name)));
expect(
withLocalCopy,
`Producers declaring a local MAX_REASON_LENGTH changed.\n` +
` expected (ledger): ${LOCAL_DECLARATION_LEDGER.join(', ') || '(none)'}\n` +
` found: ${withLocalCopy.join(', ') || '(none)'}\n` +
`A file that appeared here copied the contract's literal instead of importing ` +
`MAX_REASON_LENGTH from '@civitai/moderation' — the copy drifts silently and the first ` +
`sign is that detector's runs vanishing from the board. A file that DISAPPEARED was ` +
`migrated: update this ledger and the note on the exported constant in ` +
`packages/civitai-moderation/src/schema.ts, which names these two by hand.`
).toEqual(LOCAL_DECLARATION_LEDGER);
});
it('the migrated producer really imports the contract bound', () => {
// Pins the relationship the ledger's absence-of-a-declaration cannot: `new-order-abuse-detection`
// is off the ledger because it IMPORTS the constant, not because it stopped bounding the reason.
// Requires a real module edge carrying the specifier, so deleting the import while leaving the
// identifier in a comment does not pass.
const source = read('new-order-abuse-detection');
expect(source).toMatch(
/(?:^|\n)\s*import\s*\{[^}]*\bMAX_REASON_LENGTH\b[^}]*\}\s*from\s*['"]@civitai\/moderation['"]/
);
// And the module it imports really exports a usable number, so this file cannot vouch for an
// edge onto a binding that no longer exists.
expect(typeof MAX_REASON_LENGTH).toBe('number');
expect(MAX_REASON_LENGTH).toBeGreaterThan(0);
});
});
@@ -0,0 +1,315 @@
import { describe, expect, it } from 'vitest';
import { MAX_REASON_LENGTH, abuseReportInput } from '@civitai/moderation';
import {
ABUSE_SCAN_WINDOW_HOURS,
NEW_ORDER_ABUSE_DETECTOR,
SMITE_ACTION,
buildAbuseReport,
confidenceFor,
renderReason,
renderSummary,
toFinding,
truncateReason,
type AbuseSuspect,
} from '../report';
/**
* Three properties here fail silently, and each one fails in the direction of a moderator trusting a
* page that is wrong.
*
* A finding that omits the numbers the rule used renders as a bare accusation the reviewer cannot
* check. A finding whose `actioned`/`action` pair is half-set is refused by the contract on the
* PRODUCER's side of the wire, which loses the whole batch every correctly-built finding beside it
* included so the board shows nothing at all rather than showing something wrong. And a threshold
* leaking into `counters` puts a tunable on a page that has no use for one.
*
* That last guard is about proportionality, NOT secrecy the observed values on the findings
* already bound the thresholds, and the header of `../report` says how. The key set is pinned anyway
* so that adding a threshold counter is a decision someone made, not an accident nobody saw.
*/
const suspect = (overrides: Partial<AbuseSuspect> = {}): AbuseSuspect => ({
userId: 100,
totalRatings: 200,
uniqueRatings: 1,
dominantRating: 3,
dominantPct: 100,
avgPerMinute: 5,
...overrides,
});
describe('renderReason', () => {
it('states all six columns the query selected', () => {
// 🔴 The contract has NO structured-metrics field, so if a number is not in this sentence it is
// nowhere on the board. Every field of the query row is asserted individually — an assertion on
// the whole string would pass on a template that silently dropped one and kept the rest.
const reason = renderReason(
suspect({
userId: 4242,
totalRatings: 1_500,
uniqueRatings: 2,
dominantRating: 5,
dominantPct: 87.4,
avgPerMinute: 12.36,
}),
false
);
expect(reason).toContain('4242'); // userId
expect(reason).toContain('1,500'); // totalRatings
expect(reason).toContain('2 distinct rating value(s)'); // uniqueRatings
expect(reason).toContain('the value 5'); // dominantRating
expect(reason).toContain('87%'); // dominantPct
expect(reason).toContain('12.4 rating(s) per active minute'); // avgPerMinute
expect(reason).toContain(`last ${ABUSE_SCAN_WINDOW_HOURS}h`);
});
it('floors the dominant share rather than rounding it up to a stronger claim', () => {
// 99.6% rounding to "100%" asserts that EVERY rating was the same value. A moderator acts on
// that sentence, so it has to be literally true.
expect(renderReason(suspect({ dominantPct: 99.6 }), false)).toContain('99%');
expect(renderReason(suspect({ dominantPct: 99.6 }), false)).not.toContain('100%');
});
it('says what was done, matching the actioned flag on the same row', () => {
expect(renderReason(suspect(), true)).toContain('Auto-smited');
const open = renderReason(suspect(), false);
expect(open).toContain('No action was taken');
expect(open).not.toContain('Auto-smited');
});
});
describe('truncateReason', () => {
it('trims against the CONTRACTs bound, not a local copy of it', () => {
// 🔴 The cap the producer trims to and the cap the parser enforces must be ONE number. They were
// two literals in two packages, which drift silently and in both damaging directions — trimming
// to a length the parser already rejects, or not trimming where it would have. The value is
// pinned as well as the identifier, so lowering the contract's bound is a deliberate act that
// shows up here rather than a silent re-interpretation of every producer's truncation.
expect(MAX_REASON_LENGTH).toBe(2_000);
expect(truncateReason('a'.repeat(MAX_REASON_LENGTH))).toHaveLength(MAX_REASON_LENGTH);
const cut = truncateReason('a'.repeat(MAX_REASON_LENGTH + 1));
expect(cut).toHaveLength(MAX_REASON_LENGTH);
expect(cut.endsWith('…')).toBe(true);
});
it('is defence in depth: the generated reason cannot reach the cap on any input', () => {
// ⚠️ This records HEADROOM, not a save. Every numeric field at `Number.MAX_SAFE_INTEGER` — NOT
// a ceiling on anything ClickHouse can hand us (`JSON.parse` yields a double, and a `UInt64`
// exceeds this by ~2,000x), just an absurdly large input that is still a number — renders 269
// characters smited and 311 open, against a cap of 2,000, and a realistic finding is ~220. A
// larger magnitude would not move that much: these render through `toLocaleString`/`toFixed`, so
// the length grows with the DIGIT COUNT, and the headroom below absorbs several more digits. So
// `truncateReason` has never trimmed anything
// and cannot with this template; it guards a future one that interpolates an unbounded string.
// Asserted against a quarter of the cap rather than the cap, which a 6x longer template clears.
const absurd = suspect({
userId: Number.MAX_SAFE_INTEGER,
totalRatings: Number.MAX_SAFE_INTEGER,
uniqueRatings: Number.MAX_SAFE_INTEGER,
dominantRating: Number.MAX_SAFE_INTEGER,
dominantPct: Number.MAX_SAFE_INTEGER,
avgPerMinute: Number.MAX_SAFE_INTEGER,
});
for (const smited of [true, false]) {
const reason = renderReason(absurd, smited);
expect(reason.length).toBeLessThan(MAX_REASON_LENGTH / 4);
expect(reason.endsWith('…')).toBe(false); // nothing was cut
}
expect(abuseReportInput.safeParse(reportOf([suspect()], new Set([100]))).success).toBe(true);
});
});
describe('confidenceFor', () => {
it('stays inside the band the board sorts on', () => {
const extremes = [
suspect({ uniqueRatings: 1, dominantPct: 100, totalRatings: 10_000 }),
suspect({ uniqueRatings: 20, dominantPct: 0, totalRatings: 0 }),
suspect({ uniqueRatings: 0, dominantPct: 150, totalRatings: -5 }),
];
for (const s of extremes) {
const c = confidenceFor(s);
expect(c).toBeGreaterThanOrEqual(0.5);
expect(c).toBeLessThanOrEqual(1);
}
});
it('ranks a single-value spammer above a varied heavy voter', () => {
const scripted = suspect({ uniqueRatings: 1, dominantPct: 100, totalRatings: 200 });
const human = suspect({ uniqueRatings: 5, dominantPct: 40, totalRatings: 200 });
expect(confidenceFor(scripted)).toBeGreaterThan(confidenceFor(human));
});
it('does not restate the Acted column — it is not a function of `actioned`', () => {
// 🔴 If confidence moved with the smite outcome, the sort order would rank the rows a moderator
// has NOTHING left to do on above the ones they have to triage.
const s = suspect();
expect(toFinding(s, true).confidence).toBe(toFinding(s, false).confidence);
});
});
/**
* 🔴 THE REGRESSION GUARD. The contract's `superRefine` refuses BOTH halves of the wrong pairing
* `actioned: true` with no `action`, and `actioned: false` with one and `moderatorApp.abuseReport`
* parses before the network call, so a single mispaired finding throws and the entire run, including
* every correct finding beside it, never reaches the board.
*
* Both directions are asserted, and the payload is then parsed, because the two checks catch
* different mistakes: the field assertions catch a mapping that is inverted, and the parse catches a
* mapping that is merely inconsistent with the schema the fields will be judged by.
*/
describe('actioned/action pairing', () => {
it('marks an auto-smited suspect actioned, naming the action', () => {
const finding = toFinding(suspect({ userId: 111 }), true);
expect(finding.actioned).toBe(true);
expect(finding.action).toBe(SMITE_ACTION);
expect(SMITE_ACTION).toBe('smite');
});
it('marks a non-smited suspect not-actioned, with no action key at all', () => {
const finding = toFinding(suspect({ userId: 222 }), false);
expect(finding.actioned).toBe(false);
// `not.toHaveProperty`, not `toBeUndefined`: the contract accepts an absent key AND an explicit
// null, so omitting it is what makes the forbidden combination unrepresentable rather than
// merely unset. `toBeUndefined` would pass on `action: undefined`, which is a field that exists.
expect(finding).not.toHaveProperty('action');
});
it('files a mixed run — one smited, one not — as a payload the contract accepts', () => {
const smited = suspect({ userId: 111, uniqueRatings: 1, dominantPct: 100 });
const open = suspect({ userId: 222, uniqueRatings: 4, dominantPct: 55, avgPerMinute: 30 });
const report = reportOf([smited, open], new Set([111]));
const parsed = abuseReportInput.safeParse(report);
// The whole point: a mispaired finding makes THIS false, and the board gets nothing.
expect(parsed.success).toBe(true);
expect(report.findings.map((f) => [f.userId, f.actioned, f.action])).toEqual([
[111, true, 'smite'],
[222, false, undefined],
]);
});
it('parses each direction on its own, so one arm cannot mask the other', () => {
expect(abuseReportInput.safeParse(reportOf([suspect()], new Set([100]))).success).toBe(true);
expect(abuseReportInput.safeParse(reportOf([suspect()], new Set())).success).toBe(true);
});
it('refuses the inverted pairing — the negative control on the contract itself', () => {
// 🔴 THE FINDINGS HERE ARE WRITTEN OUT IN FULL, NOT SPREAD OVER A `toFinding` RESULT. A control
// built from the function under test is a second sample of it, not a control: with the mapping
// inverted, spreading `{ actioned: true }` over a non-smited finding would inherit that
// finding's `action: 'smite'` and parse CLEANLY — so the control would go red for the mutant's
// reason instead of staying green and proving the contract still refuses the pair. Measured:
// that is exactly what the earlier version of this case did.
const envelope = {
detector: NEW_ORDER_ABUSE_DETECTOR,
startedAt: '2026-09-14T23:00:00.000Z',
finishedAt: '2026-09-14T23:00:42.000Z',
summary: null,
counters: null,
};
const valid = { userId: 100, confidence: 0.9, reason: 'evidence' };
expect(abuseReportInput.safeParse({ ...envelope, findings: [valid] }).success).toBe(false);
expect(
abuseReportInput.safeParse({ ...envelope, findings: [{ ...valid, actioned: true }] }).success
).toBe(false);
expect(
abuseReportInput.safeParse({
...envelope,
findings: [{ ...valid, actioned: false, action: SMITE_ACTION }],
}).success
).toBe(false);
// The positive control on this instrument: the same envelope with a correctly-paired finding
// MUST parse, or the three refusals above prove nothing about the pair specifically.
expect(
abuseReportInput.safeParse({
...envelope,
findings: [
{ ...valid, actioned: true, action: SMITE_ACTION },
{ ...valid, userId: 101, actioned: false },
],
}).success
).toBe(true);
});
});
describe('buildAbuseReport', () => {
it('stamps the producer key and ISO timestamps the contract accepts', () => {
const report = reportOf([suspect()], new Set());
expect(report.detector).toBe(NEW_ORDER_ABUSE_DETECTOR);
// 🔴 `isoWithOffset`, not the bare `.datetime()` — but a `Z` string must still pass, because
// that is what `Date.prototype.toISOString` emits and it is the only producer clock here.
expect(report.startedAt).toMatch(/Z$/);
expect(abuseReportInput.safeParse(report).success).toBe(true);
});
it('floors finishedAt at startedAt rather than losing the run to a backwards clock', () => {
const report = buildAbuseReport({
suspects: [suspect()],
smitedUserIds: new Set(),
startedAt: new Date('2026-09-14T23:00:05Z'),
finishedAt: new Date('2026-09-14T23:00:00Z'),
});
expect(report.finishedAt).toBe('2026-09-14T23:00:05.000Z');
expect(abuseReportInput.safeParse(report).success).toBe(true);
});
it('files a run with no suspects rather than staying silent', () => {
// "No report today" and "a report with zero findings" look the same to a reader otherwise, and
// the first is what a BROKEN producer looks like.
const report = reportOf([], new Set());
expect(report.findings).toEqual([]);
expect(report.counters).toMatchObject({ suspects: 0, auto_smited: 0, filed_for_review: 0 });
expect(report.summary).toContain('No accounts matched');
expect(abuseReportInput.safeParse(report).success).toBe(true);
});
it('counts the accounts actually smited, not the ones named in the set', () => {
// A stale id in the set — an account that dropped out of the cohort — must not inflate the
// "already dealt with" figure a moderator reads off the summary.
const report = reportOf([suspect({ userId: 1 }), suspect({ userId: 2 })], new Set([2, 999]));
expect(report.counters).toMatchObject({ suspects: 2, auto_smited: 1, filed_for_review: 1 });
expect(report.summary).toContain('1 were auto-smited');
});
it('publishes no threshold in counters', () => {
// 🔴 A counter is a wider and longer-lived disclosure than a log line, and no threshold belongs
// on a page with no use for one. ⚠️ This does not make the thresholds unrecoverable — the
// observed values on the findings bound them; see the header of `../report`. Pinned as the WHOLE
// key set,
// not as an absence check: a check that only forbids today's threshold names cannot see a new
// one being added, and the failure would be a leak rather than a red test.
const report = reportOf([suspect()], new Set([100]));
expect(Object.keys(report.counters ?? {}).sort()).toEqual([
'auto_smited',
'filed_for_review',
'lookback_hours',
'suspects',
]);
});
});
describe('renderSummary', () => {
it('splits the cohort into acted-on and waiting', () => {
const summary = renderSummary(
[suspect({ userId: 1, totalRatings: 10 }), suspect({ userId: 2, totalRatings: 5 })],
1
);
expect(summary).toContain('2 account(s)');
expect(summary).toContain('15 rating(s)');
expect(summary).toContain('1 were auto-smited');
expect(summary).toContain('1 were filed for review');
});
});
/** The report a real run would build, at a fixed clock. */
function reportOf(suspects: AbuseSuspect[], smitedUserIds: Set<number>) {
return buildAbuseReport({
suspects,
smitedUserIds,
startedAt: new Date('2026-09-14T23:00:00Z'),
finishedAt: new Date('2026-09-14T23:00:42Z'),
});
}
@@ -0,0 +1,268 @@
import { MAX_REASON_LENGTH, type AbuseReportInput } from '@civitai/moderation';
/**
* Turning the Knights of New Order rating-abuse scan into abuse-board reports.
*
* 🔴 THIS DETECTOR IS NOT IN SHADOW MODE, AND THAT IS THE ONE WAY IT DIFFERS FROM THE TWO
* DETECTORS ALREADY ON THIS BOARD. `reaction-withdrawal-detection` and `bot-account-detection` both
* hardcode `actioned: false` because neither holds a write client. This scan DOES act: when the
* `autoSmiteAbusers` flag is on it calls `smitePlayer` on the strict-signal subset before it files.
* So `actioned` is a per-finding fact about what already happened, not an invariant which is
* exactly the case the contract's `actioned`/`action` pair exists to express, and exactly the case
* its `superRefine` refuses to let a producer get half-right.
*
* 🔴 NO THRESHOLD IS AN INPUT TO THIS MODULE AND NONE IS RENDERED AS A FIELD. Everything below is an
* OBSERVED value off one account's own behaviour counts, a percentage, a pace never the number it
* was compared against.
*
* WHAT THAT BUYS, AND WHAT IT DOES NOT. An earlier version of this comment said the tunables are
* held in Redis so they are "not readable from the public source tree", and that the confidence
* score "cannot be inverted to recover" one. Both overclaim, so they are corrected here rather than
* left to be cited:
*
* - The reason text publishes the four values the selection rule compares. The query selects on
* `HAVING totalRatings >= minTotalRatings`, so the SMALLEST `totalRatings` visible across a few
* runs of the board converges on that tunable from above; the smallest dominant share among rows
* reading "Auto-smited by this scan." converges on the smite tunable the same way. Withholding
* the fields does not change that the row itself is the disclosure.
* - The source tree is not a barrier either: this repo is public, and a checked-in test of the
* smite path already carries live values.
*
* The withholding is kept anyway, on PROPORTIONALITY rather than secrecy: nothing a moderator does
* with this board needs a threshold, so putting one on it is a disclosure that buys the reader
* nothing. And the surface this replaced a moderator Discord channel carried the same observed
* values, so the board is not a widening of what was already published.
*/
/** Stable producer key. Opaque: it groups this detector's runs on the board and namespaces its
* counters, so it is not a display string and renaming it orphans the run history. */
export const NEW_ORDER_ABUSE_DETECTOR = 'new-order-abuse-detection';
/**
* The scan's lookback, in hours.
*
* Exported and interpolated into the ClickHouse query rather than restated beside it, because the
* reason text on every finding states the window as a fact about the evidence ("in the last 24h").
* Two literals would drift, and the drift would be invisible: the sentence would keep reading
* correctly while describing a different window than the one that was measured.
*/
export const ABUSE_SCAN_WINDOW_HOURS = 24;
/** The one `action` string this detector can record. A literal in one place, so the finding builder
* and every test assert the same token. */
export const SMITE_ACTION = 'smite';
/** One row of the detection query. Structural, not imported from the job, so this module can be
* exercised without dragging in ClickHouse, Redis and the smite service. */
export type AbuseSuspect = {
userId: number;
totalRatings: number;
uniqueRatings: number;
dominantRating: number;
/** 0..100. Share of this account's ratings that were the dominant value. */
dominantPct: number;
avgPerMinute: number;
};
type Finding = AbuseReportInput['findings'][number];
/**
* A reason over the contract's limit does not lose the finding, it loses every finding in the batch.
* `moderatorApp.abuseReport` runs `abuseReportInput.parse(input)` BEFORE the fetch, so an over-long
* reason throws a ZodError inside the job's own process no request is made and there is no 400 to
* read. Truncated here; the ellipsis is the record that something was cut.
*
* DEFENCE IN DEPTH, NOT LIVE PROTECTION it has never trimmed anything and cannot with today's
* template. `renderReason`'s longest possible output is 311 characters, measured by rendering every
* numeric field at `Number.MAX_SAFE_INTEGER` in both branches (269 smited, 311 open the open
* sentence is the longer of the two), against a cap of `MAX_REASON_LENGTH`. A typical finding is
* ~220. So this guards a FUTURE template that adds a producer-supplied or unbounded string, not any
* input the query can hand it; do not cite it as the thing keeping today's reasons in bounds.
*
* The bound is IMPORTED from the contract rather than restated. It used to be a local literal beside
* the contract's own `.max(...)`, which is a copy that can drift silently: the two disagreeing means
* either a trim to a length the parser has already rejected, or no trim where one was needed, and
* both present as a detector's runs disappearing from the board rather than as a failure here.
*/
export function truncateReason(reason: string, max = MAX_REASON_LENGTH): string {
return reason.length <= max ? reason : `${reason.slice(0, max - 1)}`;
}
/**
* 🔴 EVERY NUMBER THE RULE LOOKED AT GOES IN THE REASON.
*
* The contract has no structured-metrics field `reason` is the only place a moderator can be told
* how the detector knows, and a row without the numbers is a bare accusation they cannot check. So
* all six columns the query selects are stated: the account, the volume, how many distinct values it
* used, which value dominated, what share that was, and the pace.
*
* The pair to read is the DISTINCT-VALUE COUNT against the volume. 200 ratings using one value is a
* script; 200 ratings using five with one at 40% is a person with an opinion. Both can reach this
* board, and only the sentence can tell them apart.
*/
export function renderReason(suspect: AbuseSuspect, smited: boolean): string {
// Floored, not rounded: 99.6% rounding to "100%" states that EVERY rating was the same value,
// which is a strictly stronger claim than the data supports and the one a moderator would act on.
const share = Math.floor(suspect.dominantPct);
const pace = (Math.round(suspect.avgPerMinute * 10) / 10).toFixed(1);
const parts = [
`Account ${suspect.userId} cast ${suspect.totalRatings.toLocaleString()} rating(s) in the ` +
`last ${ABUSE_SCAN_WINDOW_HOURS}h using ${suspect.uniqueRatings.toLocaleString()} distinct ` +
`rating value(s).`,
`${share}% of them were the value ${suspect.dominantRating}.`,
`Pace ${pace} rating(s) per active minute.`,
// 🔴 The sentence says what was DONE, matching the `actioned` flag on the same row. A moderator
// reading the board sees the "Acted" cell and the prose together, and the two disagreeing is
// worse than either being absent — an account that was already smited must not read as an open
// case, and one that was not must not read as handled.
smited
? `Auto-smited by this scan.`
: `No action was taken by this scan — filed for a moderator to review.`,
];
return truncateReason(parts.join(' '));
}
/**
* 🔴 CONFIDENCE IS THE QUEUE'S SORT ORDER, NOT A PROBABILITY.
*
* The board renders findings `confidence DESC`, and this detector's selection rule is a disjunction:
* an account is here because ONE of several signals fired, so a per-account probability would be
* invented. What the band does is put the rows a moderator should open first at the top.
*
* Deliberately computed from the account's OWN observed values and nothing else, so no threshold is
* an input to it. That is a fact about THIS FUNCTION, not about the row it ships on: the reason
* text on the same finding publishes the values the selection rule compared, and those bound the
* thresholds regardless of what this function takes. See the header.
*
* 🔴 NOT a function of `actioned`. A smited account scores high because its numbers are extreme, not
* because it was smited; coupling the two would make the sort order restate the "Acted" column
* instead of ranking the un-acted rows a moderator actually has to triage.
*/
export function confidenceFor(suspect: AbuseSuspect): number {
// 0..1 each. Uniformity is the sharpest of the three — one distinct value scores 1, two scores
// 0.5 — which is the shape a script has and a heavy human voter does not.
const uniformity = suspect.uniqueRatings > 0 ? 1 / suspect.uniqueRatings : 0;
const share = Math.min(1, Math.max(0, suspect.dominantPct / 100));
// Volume saturates: past a couple of hundred ratings in a day the count stops discriminating, and
// an unbounded term would let a single heavy day outrank a perfectly uniform one.
const volume = Math.min(1, suspect.totalRatings / 200);
const blended = 0.4 * uniformity + 0.4 * share + 0.2 * volume;
// Floored at 0.5 so the whole band sits in the top half — every row here matched the rule, and a
// 0.1 would read as "probably nothing" on a page that mixes detectors.
return Math.round((0.5 + 0.5 * blended) * 100) / 100;
}
/**
* One finding.
*
* 🔴 THE `actioned`/`action` PAIR IS THE WHOLE POINT OF THIS FUNCTION, AND GETTING IT HALF-RIGHT
* COSTS THE ENTIRE RUN. The contract's `superRefine` rejects in BOTH directions `actioned: true`
* with no `action`, and `actioned: false` with an `action` and `moderatorApp.abuseReport` parses
* before the network call, so one mispaired finding throws and the whole batch, including every
* correctly-built finding beside it, never reaches the board.
*
* So the pair is minted in ONE place, as two whole branches rather than as two independently-set
* fields. There is no code path that can set one without the other: the `false` branch omits
* `action` entirely rather than passing `null`, which makes the forbidden combination
* unrepresentable instead of merely absent today.
*
* 🔴 `smited` MEANS "A SMITE ROW WAS WRITTEN FOR THIS ACCOUNT BY THIS RUN" the durable penalty,
* not the intent to apply one and not the smite call returning cleanly. All three are different
* sets, and the board is wrong in a different direction for each of the two it is not:
*
* - SELECTED-for-smiting over-claims. The job's loop swallows a per-player failure and carries on,
* so an account whose write never happened would read as dealt with when nothing was done.
* - CALL-RETURNED under-claims, which is the worse of the two because it reads as the safe option.
* `smitePlayer` commits the row first and then does non-durable work that can throw (see its
* `onSmiteCreated` comment); an account caught by that IS penalised, and filing it `actioned:
* false` puts "No action was taken by this scan" on the board beside a live smite, inviting a
* moderator to apply a second one.
*
* So the job hooks the write itself and this flag carries exactly that fact no more. It does NOT
* claim the player was notified, that their counter moved, or that a third-strike career reset
* completed; each of those is in the tail that can fail independently of the penalty.
*
* AND ON THE THIRD STRIKE THE ROW UNDERSTATES WHAT HAPPENED, which is the one direction a
* moderator can act on wrongly. When the new smite is the account's third active one, `smitePlayer`
* writes it so the hook fires and this files `actioned: true`, "Auto-smited by this scan." and
* then `resetPlayer` cleanses EVERY active smite, that one included, and wipes the career back to
* Acolyte with all counters at zero. A moderator who opens the row looking for the live smite it
* names finds zero active smites and a reset account: a LARGER action than the row states, not a
* smaller one, and nothing here distinguishes it from an ordinary first strike.
*
* The mechanism predates this flag and is not ours to change from here; the reason it is written
* down is that "actioned: true" plus a smite that no longer exists reads as a bug in the board.
*/
export function toFinding(suspect: AbuseSuspect, smited: boolean): Finding {
const base = {
userId: suspect.userId,
confidence: confidenceFor(suspect),
reason: renderReason(suspect, smited),
};
if (smited) return { ...base, actioned: true, action: SMITE_ACTION };
return { ...base, actioned: false };
}
export function renderSummary(suspects: AbuseSuspect[], smitedCount: number): string {
if (!suspects.length)
return `No accounts matched the rating-pattern scan over the last ${ABUSE_SCAN_WINDOW_HOURS}h.`;
const ratings = suspects.reduce((sum, s) => sum + s.totalRatings, 0);
return (
`${suspects.length.toLocaleString()} account(s) matched the rating-pattern scan over the last ` +
`${ABUSE_SCAN_WINDOW_HOURS}h, between them ${ratings.toLocaleString()} rating(s). ` +
`${smitedCount.toLocaleString()} were auto-smited by the scan; ` +
`${(
suspects.length - smitedCount
).toLocaleString()} were filed for review with no action taken.`
);
}
export type BuildReportArgs = {
suspects: AbuseSuspect[];
/** The accounts this run committed a smite row for. Membership, not selection, and keyed on the
* durable write rather than on the call returning see `toFinding`. */
smitedUserIds: ReadonlySet<number>;
/** The producer's clock at the start of the run. */
startedAt: Date;
/** The producer's clock when the scan (including any smiting) finished. */
finishedAt: Date;
};
/**
* The whole run, as the one report the endpoint will accept.
*
* 🔴 NO THRESHOLD GOES IN `counters` but not because that keeps one secret. The observed values on
* the findings already bound the thresholds (see the header); the reason is that nothing a moderator
* does with this board needs a tunable, and a counter is a longer-lived and wider-read disclosure
* than a log line. The three below are outcomes of this run how many matched, how many were acted
* on, how many are waiting plus the lookback, which is a plain literal in the query already.
*
* 🔴 `finishedAt` is floored at `startedAt`. Both are the producer's own clock so the pair is
* normally ordered, but the contract refuses a transposed pair outright and losing a whole run to a
* clock stepping backwards mid-scan is not a trade worth taking.
*
* 🔴 ONE report, not batches, and that holds only while the scan's own `LIMIT` stays under
* `MAX_FINDINGS_PER_REPORT` (1,000 against 50 today). Raising that limit past it does not truncate
* the report, it makes the contract refuse the whole run at which point this needs the chunking
* `bot-account-detection/report.ts` already implements, including its per-batch `startedAt` offset,
* because `(detector, started_at)` is the receiving table's idempotency key and two batches sharing
* one start REPLACE each other rather than appending.
*/
export function buildAbuseReport(args: BuildReportArgs): AbuseReportInput {
const findings = args.suspects.map((s) => toFinding(s, args.smitedUserIds.has(s.userId)));
const smitedCount = findings.filter((f) => f.actioned).length;
const finishedAt = new Date(Math.max(args.finishedAt.getTime(), args.startedAt.getTime()));
return {
detector: NEW_ORDER_ABUSE_DETECTOR,
startedAt: args.startedAt.toISOString(),
finishedAt: finishedAt.toISOString(),
summary: renderSummary(args.suspects, smitedCount),
counters: {
suspects: args.suspects.length,
auto_smited: smitedCount,
filed_for_review: args.suspects.length - smitedCount,
lookback_hours: ABUSE_SCAN_WINDOW_HOURS,
},
findings,
};
}
@@ -151,7 +151,9 @@ describe('the finding a moderator reads', () => {
});
it('truncates a reason rather than losing the whole report', () => {
// 🔴 One over-long reason 400s the REPORT, not the row — every finding in the batch is lost.
// 🔴 One over-long reason loses the whole REPORT, not just the row — every finding in the batch
// goes with it. It fails `abuseReportInput.parse` locally, inside `moderatorApp.abuseReport` and
// before the network call, so the symptom is a ZodError in this process, not a 4xx from the spoke.
expect(truncateReason('x'.repeat(2_500))).toHaveLength(2_000);
expect(truncateReason('short')).toBe('short');
});
@@ -46,8 +46,10 @@ export function renderReason(a: WithdrawalAccount): string {
return truncateReason(parts.join(' '));
}
/** 🔴 A reason over the contract's limit does not lose the finding, it 400s the REPORT and loses
* every finding in the batch. Truncated here; the ellipsis is the record that something was cut. */
/** 🔴 A reason over the contract's limit does not lose the finding, it loses the whole REPORT and
* every finding in the batch with it. The failure is LOCAL, not a spoke 4xx: `moderatorApp.abuseReport`
* runs `abuseReportInput.parse(input)` before the fetch, so it throws a ZodError in this process and
* nothing is sent. Truncated here; the ellipsis is the record that something was cut. */
export function truncateReason(reason: string, max = MAX_REASON_LENGTH): string {
return reason.length <= max ? reason : `${reason.slice(0, max - 1)}`;
}