mirror of
https://github.com/civitai/civitai.git
synced 2026-09-20 22:08:18 +08:00
main
2 Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
 Justin Maier
|
d757478088 |
fix(block): enforce blocking on legacy model comments and review edits (#4072)
* fix(block): enforce blocking on legacy model comments and review edits The write guard covers the CommentsV2 surfaces, but the older model-comment path was never wired into it. The value it passed as the block target is the comment's author — the caller themselves on a create — so a create resolved to a check about the caller and the model owner's block was never consulted. A reply checked only the parent comment's author. Resolve the model owner (and the parent author, for a reply) through the shared block-check service instead, and enforce on edits as well as creates: an edit is scoped by comment id while writing modelId/parentId through from the request, so both the comment's stored home and the one the request names are checked. Refused writes 404, matching the other write paths. Resource reviews had the create-only half of the same shape — a review written before a block stayed editable afterwards — and an edit can re-point modelId, so both models are checked there too. Tests: each negative assertion was verified by mutating the guard away and confirming it fails in under two seconds. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(block): guard the review procedures the UI calls, and 3D model reviews Review round on the previous commit found the review half aimed at the wrong procedure. `resourceReview.upsert` is only reached from the edit-review modal; the app calls `resourceReview.create` and `.update`, and neither had a block check at all — so an author refused on `upsert` had an unguarded path to the same row. Both are guarded now. `update` carries no modelId, so it resolves the review's model from the stored row. 3D model reviews had no check on either branch, while comment threads hanging off such a review were already guarded — which made the surface look covered. Re-homing is already refused there, so one check against the target model does both branches. The legacy-comment resolver now answers through `getBlockCheckOwnerIds` rather than reading the two rows itself, so a rule added to either entity type reaches this path too. `commentOld` resolves the model owner alongside the comment author, matching its CommentsV2 counterpart — author alone still let a blocked user react under a blocker's model. Tests: mocks key on the id being asked for rather than on call order, after identical reply-resolution failures under load turned out to be an empty default being indistinguishable from a real answer. Every guard was mutation-checked individually. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * test(block): assert the reaction behaviour the widened commentOld arm changes `commentOld` has one consumer, `toggleReaction` — the legacy comment surface has no reaction of its own — so resolving the model owner there changes reaction behaviour: reacting to somebody else's comment under a blocker's model now refuses. That arrived as a side effect of routing the resolver through the shared switch; give it its own assertions, in both directions. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(block): restore the moderator exemption on the 3D-review guard A reviewer's mutation probe was live in the working tree and got swept into the previous commit by a broad `git add`, leaving the guard with a hardcoded `isModerator: false` — so a moderator blocked by a creator would be refused on that creator's 3D-model reviews, letting any user lock a moderator out of a review surface by blocking them. The suite catches it (`exempts moderators`); the commit that introduced it was pushed on the strength of a full-suite run that predated the sweep. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * test(block): pin the handler wiring, and read the guard's target from the writer The review handlers are the half of the fix that reaches production — the guard only knows who is acting because the handler passes it — and no test touched them. Dropping `userId: ctx.user.id` left the guard asking whether an undefined user is blocked: a silent no-op on the path the guard exists to close, with nothing red. Pinned per procedure, along with the moderator status. `userId`/`isModerator` are guard inputs, not columns, and reach the service in one object with the review's fields; a destructure that stops pulling them out sends them into the Prisma payload, which on update rewrites the review's author. Asserted on both payloads. The stored-review lookup read the replica while the write it guards goes through the writer, so a review created seconds earlier resolved no owner and the write proceeded — the fall-through was to a real write, not to a not-found. It reads through the writer now. Also: the moderator-exemption test now covers the upsert branch it was named for, and the new handler test uses the canonical redis mock rather than a direct one. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * refactor(block): one owner map, one statement of the moderator rule The widened `commentOld` arm still read the model row itself, one arm away from `case 'model'` answering the same question — so a rule added there would reach every path except that one. It recurses now, the way `case 'comment'` already does. `throwIfBlockedByModelOwners` stated the moderator bypass a second time before handing the same flag to the shared guard. Dropped the local early return; the argument stays, and deleting it fails `exempts moderators on every branch`. Also documents a measurement hazard found while verifying this branch: two vitest runs in one worktree make configured mock lookups return their empty default, so a targeted run beside a full suite produces reds the code does not explain. 10/10 green frozen and alone, 1/5 red with a suite alongside, 6/6 green under CPU load with no rival vitest. The mechanism is unproven and the text says so. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
Justin Maier
|
51eb5d7bf0 |
test(perf): canonical shared-module mocks, a migration ratchet, and the first 100 files (#3959)
* test(perf): canonical shared-module mocks for running without per-file isolation
The unit suite spends ~81% of worker-time importing modules. `vitest --no-isolate`
collapses that but breaks ~1,500 tests, and the mechanism is narrower than
"vi.mock is per file": a test file that does not mock a module still gets the real
one. The damage travels through ordinary source modules — a module that imports a
mocked module is evaluated ONCE per worker and captures its bindings then, in
whatever mock context that first evaluation happened to occur. Every later file
reuses it, still pointing at the first file's mock object.
So the fix must keep the mocked shape complete and function identities stable for
the worker's lifetime, and swap only behaviour. src/__tests__/mocks/ does that: a
hybrid node that is both a vi.fn() and a proxy vivifying cached children, one
canonical mock per specifier, registered globally in setup.ts and reset per file
(setupFiles re-run per file in both isolation modes).
Inert under isolation, where it is registered but nothing can leak: 174 files,
2089 tests, 0 failed.
Ships the tooling to continue the migration: a codemod that converts only shapes it
can prove equivalent and reports every refusal with a reason, an allowlist
generator that refuses to grow, and a run/compare pair that diffs per-file
collected counts — because under --no-isolate a file whose module scope throws
collects ZERO tests and the run still reads as green.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* test(perf): migrate 83 files onto the canonical mocks, and ratchet the rest
Converted by scripts/test-perf/codemod-shared-mocks.mjs. The test bodies are
untouched: only the vi.mock factory and its vi.hoisted spies go, replaced by consts
bound to canonical nodes, so the change is mechanical and reviewable as such.
Measured on the 83 files that are clean for all three specifiers, against the same
files isolated at 4 workers (856 tests, 0 failed):
--no-isolate, 4 workers 846 tests 11 failed wall 62.6s -> 22.9s
--no-isolate, 12 workers 853 tests 1 failed wall 62.6s -> 18.8s
The residual failures are not mock shape. eventloop-longtask.ts registers prom
metrics at module scope, so under --no-isolate the first file to import it in a
worker takes the registration and later files see an empty registry; that belongs
with no-module-scope-cache.
Migration is all-or-nothing per specifier — one hold-out re-poisons its whole
worker — so no-direct-shared-module-mock guards it with an allowlist at 433 files.
It ratchets both ways: a new direct mock fails, and a migrated file left on the list
fails too, so the count cannot be padded.
purge-review-snapshots proved "the gate reads the primary" with
`expect(dbRead.x.findFirst).toBeUndefined()` — the replica fixture simply lacked the
method. The canonical mock vivifies every method, so absence stops being observable;
rewritten as `not.toHaveBeenCalled()`, which asserts the behaviour rather than the
fixture's shape and survives another file populating that method.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* test(perf): type the shared-mock self-tests against the canonical nodes
`tsconfig.tests.json` sees these files even though `pnpm typecheck` does not, and
calling through the re-exported `dbRead`/`redis` demanded real Prisma `where`
objects and literal Redis key unions — 8 errors about argument shapes, in tests
about default return values. The canonical nodes are the same objects (asserted in
the file), so routing the calls through them keeps each test about what it is for.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* test(perf): migrate 17 more files, and teach the codemod the real REDIS_KEYS
The codemod now static-parses REDIS_KEYS_UNPREFIXED / REDIS_SYS_KEYS out of
packages/civitai-redis/src/client.ts and deep-compares each hand-written literal
against it: dropped where every leaf matches, refused and printed where one does
not. That was the largest refusal class.
42 files diverge across 73 leaves. Some are placeholders ("rl", "kill"); others
read as real and are wrong — CACHE_LOCKS as "caches:lock" against a real
"cache-lock", TRPC.LIMIT.BASE as "trpc:rate-limit" against "packed:trpc:limit", a
system:-prefixed sys key written without the prefix, and a cache key still on v1
after the real one was bumped to -2. None are live bugs: a test asserting against
its own copy uses the same wrong string on both sides, which is why nothing in the
repo would ever surface them.
Allowlist 433 -> 416.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* test(perf): teach the codemod three more shapes, migrate 31 more files
Repo-wide convertible goes 10 -> 84 on the current allowlist. New shapes:
- a whole client built as an object literal of spies, collapsed to the canonical
root: binding the root makes every leaf vivify at exactly the path the literal
named, so `mockDbRead: { collection: { findFirstOrThrow: vi.fn() } }` becomes
`const mockDbRead = dbMock.dbRead` and the test body needs no edits. Leaves
carrying real behaviour are still refused rather than dropped.
- `vi.hoisted` with a block body whose returned property names a local; the local
is removed too, but only when the block reads it exactly once, so a `make()`
helper shared by two clients is left alone.
- an import alias when a test's own local collides with the canonical mock's name.
`const { redisMock } = vi.hoisted(...)` is real in this repo, and lifting it
produced the self-referential `const redisMock = redisMock.redis`.
Verified on the 120 files now clean for all three specifiers, against the same
files isolated (1268 tests, 0 failed): 1268/1268 collected under `--no-isolate` at
both 4 and 12 workers, no file lost tests. The remaining failures are other shared
specifiers that have no canonical mock yet — `~/server/flipt/client` is the loudest
— which is the same mechanism, not a regression in these conversions.
Allowlist 416 -> 396.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* test(perf): widen the mock guard to every shared specifier, counted honestly
The three specifiers with canonical mocks are not the whole job, and an allowlist
covering only them would reach zero with `isolate: false` still unflippable.
Measured: a 120-file set clean for all three still failed 110 tests at 4 workers
under --no-isolate, entirely through specifiers nobody had listed — loudest being
`No "isFliptSync" export is defined on the "~/server/flipt/client" mock`. Same
mechanism, different module.
So the guard now tracks 15 specifiers, split by obligation:
CANONICAL (3, enforced) 562 sites across 396 files
PENDING (12, counted) 856 sites across 392 files
PENDING is counted rather than enforced because those modules have no canonical
mock to migrate to; enforcing would push every new test file onto the allowlist and
measure churn instead of work. Its recorded count is asserted against reality, so
the remaining scope cannot silently understate itself.
The specifier lists live in one TS module and the generator PARSES them rather than
holding a second copy — a generator that disagreed with the guard about which
modules are guarded is the one failure this pair must not have.
Stated in the guard, because it is the part that is easy to get wrong: the flip
criterion is not "the allowlist reaches zero", it is "every specifier a test file
shares with another test file has a canonical mock".
Next specifier to do is `~/env/server`: 109 sites, every one partial, and it is
already globally mocked in setup.ts with a Proxy — so the canonical shape exists
and the work is per-file behaviour plus a reset, not a new design.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* test(perf): give the six external mock factories a `default` key
Pre-bundling wraps a CJS dep for interop, so the consumer resolves through
`default` and a factory that returns only named exports yields undefined. Adding
`default` lets `redis`, `@aws-sdk/client-s3` and `@aws-sdk/lib-storage` join the
SSR pre-bundling safelist.
Measured under a pre-bundling config, same six files, back to back:
without `default` 6 files 7 tests collected 6 files failed to load
with `default` 6 files 106 tests collected 0 failed
7 -> 106. Note the failure mode: the run does not report 99 failures, it reports
almost no tests. A file whose module scope throws collects nothing, the failure
count barely moves, and a summary line reads as a pass. The acceptance check is
therefore the collected count, per file — `s3-utils.test.ts` alone is 66 of the
106, so the total can look healthy while that one file is empty.
`importOriginal` does NOT protect against this, despite the repo mandating that
form: the spread copies the original module NAMED exports and does not synthesise
a `default`. `s3-utils.test.ts` already used `importOriginal` and still collected
nothing without the key.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* fix(test-perf): stop the canonical mocks evaluating the real db/redis shims
`~/server/db/client` and `~/server/redis/client` are shims: they re-export their
package wholesale AND construct real clients at module scope. Registering them with
an `importOriginal` spread therefore forced real Prisma/Redis construction into
EVERY test file, where before only files without their own db mock paid it.
That is a correctness surface, and it fails in the worst available shape. A file
whose own `@prisma/client` mock omits `PrismaClient` died during module evaluation
with `PrismaClient is not a constructor`, so it collected ZERO tests — the failure
count stayed at 0 and the run read as green. arabella found it on a control run
before converting anything, which is the only reason it was found at all.
The package re-exports are all the spread was ever protecting, so the fix is to
spread `@civitai/db/client` / `@civitai/redis/client` directly. Same exports, no
construction, and the precondition for the whole failure class disappears rather
than being enumerated: a file no longer needs its own db mock to be protected.
Verified: `process-vault-items.test.ts` collects 15 tests again; all ten files that
mock `@prisma/client` collect (134 tests, 0 failed); and a file simulated into the
MIGRATED state — its own db mock removed, its `@prisma/client` factory still
lacking `PrismaClient` — collects 8/8, so the five files predicted to die when
migrated no longer will.
The 120 migrated files were unaffected: identical per-file collected counts before
and after (1268), 0 zero-collect files in either.
Pinned by a test asserting neither shim's globalThis client cache is populated —
absent globals are direct evidence the module body never ran.
Also adds the canonical `~/env/server` mock (worker-level defaults + per-file
overrides + reset) and drops three per-file env mocks onto it. Under `--no-isolate`
at 4 workers the 120-file set goes 110 -> 58 failures, 1268/1268 collected.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* docs(test-perf): the shim-vs-package rule and env's two buckets
Both are things a migrator gets wrong silently rather than loudly.
Spreading a shim evaluates a module that constructs clients; spreading its package
does not. Stated with the failure shape, because the symptom is a file collecting
zero tests behind a green run rather than anything that looks like an error.
`~/env/server` splits where the other specifiers do not: a per-file override cannot
reach a value read at module scope, since under `isolate: false` that module is
evaluated once per worker. Module-load values belong in the worker defaults,
call-time values in per-file `setEnv`. Recorded with the reason it is not a
shortcoming of the design — a per-file `vi.mock` factory has the same problem — so
nobody reverts to per-file mocks trying to fix it.
Plus the `Object.defineProperty` trap: an inconsistent proxy descriptor throws on
the SECOND call, which surfaces as a dozen unrelated failures pointing nowhere near
the cause.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* docs(test-perf): make the whole-suite collected-count diff a blocking flip gate
Per-slice verification is sound and insufficient at the same time. Every slice
owner diffing per-file collected counts over their own files is correct practice
and still leaves a gap: a file nobody owns can collect zero and no owner's control
covers it.
That is not hypothetical. The `importOriginal`-on-a-shim regression made one file
die at module scope and contribute nothing; the 120-file pilot was verified by
per-file collected counts and could not have caught it, because the affected file
was outside the set. It was found by someone taking a control run on a different
slice before converting anything. The failure was not in anyone's work — it was in
the gap between everyone's work.
So the gate is whole-suite, diffed per file against a `main` control taken in the
same window, zero files losing tests and totals matching exactly — and it runs
immediately before the flip, because the property is only true of the tree that
ships. Cheap: the integration run was 1069 files / 16806 tests in about four
minutes.
Records the general form too, since it outlives this migration: the day's two most
valuable findings each came from someone else running a control on another person's
work, and a third correction went the other way. Authors verify what they changed;
nobody verifies what changed around them.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* test(perf): four more codemod shapes, taking convertible from 53 to 204
Widening the tool beats converting files by hand: every shape lands on four slices
at once. Two of the four came from arabella's refusal taxonomy, and one of those
was a case of the tool asking the wrong question rather than being too conservative.
1. LIFT inline behaviour instead of refusing it. A leaf carrying behaviour is
already an expression, so it becomes an explicit assignment on the canonical
node - mockImplementation / mockResolvedValue - emitted at module scope. A spy
reached through an object (`mocks.findMany`) is WRAPPED rather than rebound, so
`expect(mocks.findMany).toHaveBeenCalled()` still holds and the assertions do
not move. Skipped entirely in a file that calls resetAllMocks or
restoreAllMocks, where a module-scope assignment would be wiped before the
first test runs.
2. A client literal may now carry behaviour-bearing leaves. `$executeRaw:
vi.fn().mockResolvedValue(undefined)` beside four bare spies was the only thing
separating several files from the object-literal collapse.
3. Factory-supplied constants are DELETED, not proved equal. The registration
spreads @civitai/redis/client, where both key tables are defined, so the
factory's copy is redundant whatever it contained - and proving the literal
equals the real constant is unanswerable for `completeKeys({ ... })` and is not
the question that decides safety. What decides it is whether the test names the
constant outside the factory, which is a check the tool can make. Divergences
are still reported, as findings rather than blockers.
4. Any export other than the client roots is dropped on the same rule - safeError,
withSysReadDeadline - since the spread already supplies them. The factories only
declared them because replacing the module wholesale meant they had to.
Also: a block-body factory may now hold arbitrary locals. It used to refuse
anything that was not `const actual = await importOriginal()`, which rejected a
`make()` key-proxy helper for no reason - the whole factory is deleted, so its
locals go with it.
Verified on the 11 files this converts in my slice, against a control of the same
27: 582/582 tests collected, 0 failed, isolated. One file went red and it is the
finding the docs predict: middleware.trpc.test.ts asserted
`stringContaining('trpc:rate-limit:')` against its own fixture's copy of
REDIS_KEYS.TRPC.LIMIT.BASE. The real key is `packed:trpc:limit`, so both sides of
that comparison were the fixture's invention - it could never fail and never
matched production. The assertion now names the real key.
Allowlist 396 -> 385.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* test(perf): widen mock-guard DETECTION to .tsx, leave conversion on .ts
A .browser.test.tsx adding a direct canonical mock is a class the guard was
structurally unable to observe, which is different from a class that happens to be
empty (it is: 0 such files today). Detection is the half with value now.
The codemod deliberately stays .ts. Converting a browser-mode file would put it in
a regime the canonical mocks have never been proven in, and a glob change would
make that look like a supported path. Proving browser mode is real work, not a
one-word edit. (donovan's finding, and his own suggested split.)
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* test(perf): lift behaviour on declarations and hoisted entries too, 204 -> 223
The lift rule already applied to a factory leaf; a spy declared WITH behaviour —
`mockUpdateMany: vi.fn(async (args) => ({ count: 1 }))` in a vi.hoisted object —
still blocked its whole file. Same treatment now: the behaviour becomes a
mockImplementation on the canonical node and the binding converts.
Validated on a real file rather than by the report: agent-report-callback.test.ts
converts with zero refusals and passes 14/14. Its own slice owner is mid-probe on
that directory, so the file was restored afterwards and nothing there is committed.
Remaining refusals are now dominated by the dbRead/dbWrite aliasing class (43),
which is deliberately manual — splitting an alias needs someone to know which
client the code under test exercises, and some of those are supposed to go red.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* docs(test-perf): record the browser-mode gap as work, not verification
The codemod stays .ts while the guard detects .tsx. Worth stating that there are
ZERO .tsx files mocking a canonical specifier today, so closing the gap means
writing one rather than migrating one — a piece of work, not a check somebody can
tick off. (donovan's framing, sharper than mine.)
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* fix(test-perf): three unsound codemod shapes, found by two probe runs
223 -> 164 convertible. The drop is the point: 59 files that were converting
silently wrong now refuse. All three were caught by other people running a
control on the tool's output, and two of the three do not fail loudly.
1. DROPPING A NON-ROOT EXPORT removed a test's control surface. The registration
does supply the real `withSysReadDeadline` / `safeError` — but a factory that
replaces one with a SPY is using it to drive the behaviour under test.
session-verifier.test.ts injected a timeout through it; with the spy gone the
deadline never fired and two fail-open legs asserted nothing while still
passing. Available is not the same as redundant. Only plain DATA drops now;
anything carrying behaviour refuses.
2. LIFTING AT A CLIENT ROOT treated a whole client object as a leaf spy.
`redis: mainFake.client` became `redisMock.redis.mockImplementation(...)`,
which loses every method on the object — `scanIterator` yields nothing, `del`
is gone. It does not throw, it returns empty, so a test asserting "nothing was
deleted" would have gone GREEN. Lifting is now confined to method positions.
The wiring bug underneath it is worth naming: the guard was passed
`spec.flat`, which is `undefined` for db and redis, and `undefined` triggers
the parameter's `= true` default. The check was present and inert.
3. LIFTING AN EXPRESSION OUT OF A HOISTED BLOCK broke its references.
`$queryRaw: vi.fn(queryRaw)` became a module-scope `mockImplementation(queryRaw)`
while `queryRaw` was a const inside the `vi.hoisted` body the conversion
deletes — `ReferenceError` at import, so the file collects zero tests. A lifted
expression may now only reference names declared at the file's top level. Type
annotations are skipped, since they are erased before any of this runs.
Verified: all four files from the two probes now refuse, and a known-good
conversion still converts and passes 14/14.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* docs(test-perf): state what the flip gate cannot catch
Collected counts and residual-mocks detect ABSENCE. Neither can see a test that
still runs, still passes, and no longer asserts anything real — which is what two
of the three codemod defects produced. Both files converted with zero refusals,
kept every test, and read clean on residuals; one of them returned empty instead of
throwing, so a test asserting 'nothing was deleted' would have gone green.
What caught them was a control pair at assertion level, on a small set, run by
someone who did not write the tool. Recorded beside the gate as the half the gate
cannot do, so nobody reads a clean gate as completeness. (archer's point.)
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* fix(test-perf): the migration guard was inert in every full-suite run
It scanned the test tree in the `describe` body, so a throw during that scan was a
COLLECTION failure: the file contributed zero tests, the suite's failure count did
not move, and the guard was simply absent. It passed whenever invoked as a named
file, which is how it was checked all day. The allowlist, the ratchet and the
both-directions property were all unenforced in the run that matters.
Three changes, because the symptom and the shape both needed fixing:
- The scan runs INSIDE the tests. The same fault is now a red test with a stack
instead of a file that quietly is not there.
- It skips anything that is not a regular file. A full-suite run creates
directories under `src/` while the walk is happening, and one whose name matches
the glob reaches readFileSync as EISDIR.
- A POSITIVE CONTROL asserts the walk actually saw the tree (>800 files against a
real ~1,200). Every claim this guard makes is about a set of files; an empty walk
makes all of them vacuously true. Every other guard on this project has such a
control and this one did not, which is how it got here.
`compare-runs.mjs` gains the condition that would have caught it: every file the
candidate ADDS must collect at least one test. A new file has nothing on the
control to lose against, so it can collect zero and diff perfectly clean — the
gate's own blind spot, found by running the gate rather than by reasoning about it.
Also records that the PENDING specifier list is a FLOOR rather than an inventory.
It was assembled from a static scan, and specifiers keep arriving from the other
direction, as failures in a --no-isolate run of files already clean for everything
listed. flipt/client arrived that way; middleware/block-scope.middleware (27 files)
is a live candidate. The remaining work is discovered, not known.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* docs(test-perf): the unit of work is a cluster of specifiers, not one
Canonicalising block-scope.middleware took one pair from 13 failures to 7, and the
remaining 7 were a different class — so that pair shares at least one more
poisoning specifier nobody has named. 13 -> 7 is the dangerous shape: it reads as
progress and is not completion.
A set is done when its --no-isolate failures reach zero, not when the specifier you
were working on stops appearing. (arabella's measurement.)
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* perf(tests): migrate src/server (other) onto the canonical mocks
20 of the 56 candidate files in the slice — src/server/** minus services/,
jobs/ and routers/, which belong to other slices. Converted with the codemod at
95ac2f9e6a; nothing hand-written.
Verified on the full protocol rather than on a green:
control -> convert -> per-file collected-count diff -> pass/fail diff -> mutation
20 files, collected 162 -> 162, 0 regressed, 0 failures
Mutation sample across the mock kinds, on the converted files:
purgeCache remove the fail-soft catch -> 1 failed, bites
stored-image-probe drop the ETag passthrough -> 1 failed, bites
base.reward rethrow a transient CH error -> 5 failed converted,
5 failed unconverted
The base.reward pair is the claim worth making: the conversion preserves the
file's discriminating power, not merely its pass/fail state.
Two mutations came back green and are NOT findings — the same mutations were
equally green against the unconverted files, so they were mis-aimed at paths
those files never exercise. A mutation that does not bite is evidence of nothing
until it has been shown to bite on the original.
Three files keep a direct mock for a specifier the codemod refused, having been
converted for the others: challenge-helpers (redis), challenge-winner-payout-dedupe
and challenge-winner-persistence (logging). Per-specifier atomicity holds — no
file is half-converted for any one specifier — but those specifiers stay poisoned
for the worker, so the slice is not a clean read for redis or logging on its own.
The codemod also reported 9 constants the fixtures had invented, e.g.
REDIS_KEYS.CACHE_LOCKS real "cache-lock" vs test "caches:lock". None of the
affected tests named those constants outside their factory, which is why nothing
went red — both sides of any such comparison would have been the fixture's own
invention. The factory copies are gone; the tests now see the real values.
* test(perf): migrate the six aliased download fixtures by hand
All six aliased dbRead and dbWrite onto ONE spy, so a read routed to the primary
would have satisfied a replica assertion silently. Every handler reads the replica
only - dbRead.keyValue.findUnique for the blocklists, plus dbRead.vaultItem and
dbRead.modelVersion in the vault route - so each binds dbRead alone. That is
strictly more discriminating than the fixture it replaces.
Codemod refuses this class deliberately: splitting an alias needs someone to read
the production path and decide which client the code exercises, and getting it
wrong produces a passing test asserting the wrong thing.
7 files, 71 tests, 0 failed.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* chore(test-perf): regenerate the allowlist after merging archer's slice
383 -> 366 canonical files, 518 sites. The allowlist is derived state and three
people are converting on separate branches, so it is regenerated ONCE here rather
than churned on each of them — a both-directions ratchet in a shared JSON is a pure
conflict generator otherwise.
Guard green after the merge, which is the case it exists for: a merge that resolves
a converted test file leaves its entry stale, and that is the direction that has
already caught something today.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* docs(test-perf): a drifted constant nothing asserts on is invisible both ways
The doc already said to expect redness when the real constant swaps in. The more
common case is the opposite: in one slice nine invented constants were replaced and
none went red, because no affected test named them outside its own factory — so
both sides of any comparison were the fixture's invention.
That is worse than a failure, not better. It cannot fail and it cannot be reviewed,
and six of the nine were plausible variants of the real key
(new-order:sanity-check-failures against a real new-order:sanity-failures) which is
exactly how they survive a reading. Log what the codemod reports as drifted even
when the suite stays green. (archer's slice.)
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* docs(test-perf): why the PENDING list misses what it misses, and how to find it
The list was built from repo-wide mock counts. Services is a fan-in problem and
the list covers it — every high-fan-in specifier is on it, first undiscovered one
ranks 12th. Routers is a clique: locally dense, repo-wide rare, so a repo-wide
count structurally cannot see it. Two different problems needing two different
searches, not more of the same one.
The method that finds the missing ones is a per-pair shared-specifier graph for the
directory being worked on, not a sorted repo-wide count.
Also records arabella's untested idea, which is the highest-value thing nobody has
tried: a third of services pairs share NO specifier, so a worker assignment
grouping non-overlapping files could make much of that suite clean under
isolate:false without canonicalising anything — statically evaluable from the same
graph before any code is written.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* test(perf): migrate arabella's routers/jobs slice, 22 files
Codemod at
|