mirror of
https://github.com/civitai/civitai.git
synced 2026-09-20 22:08:18 +08:00
Zachary Lowden
95562f7755
* 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>