mirror of
https://github.com/civitai/civitai.git
synced 2026-09-20 22:08:18 +08:00
e50d122cb2e37fefceb2cdff8f99264a3d5d2502
206 Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
 Zachary Lowden
|
e76db50b34 |
fix(blocks): stop enumerating capabilities in the ai:write:budgeted consent copy (#4891)
* fix(blocks): stop enumerating capabilities in the ai:write:budgeted consent copy
Operator-confirmed 2026-09-16: an inline customComfy graph CAN generate video.
That makes the sentence #4879 shipped -- "...generating images and running
language models" -- UNDER-name a reachable capability, which is the worse
direction for consent: the user agrees to "images" while the app spends their
Buzz on video.
The copy is now generic:
"Run AI generation services that spend the viewer's Buzz, with a per-call cap"
WHY GENERIC RATHER THAN A BETTER LIST. Enumerating was tried three times and was
wrong three times, each caught by a later audit round:
- "training models" -- over-promised; no implemented billing mode can carry
a variable-cost step, so training cannot be registered
- "and video" -- cut as unreachable on enum reasoning that covered every
arm EXCEPT the one bounded by no enum
- that cut itself -- wrong: customComfy mode:'inline' forwards an arbitrary
graph and the read path applies no media-type check
Each failure costs a re-consent of every live grant, because consent is stored
per (user, app) and no lookup reads a version. A generic term cannot be
falsified by a capability arriving or turning out to be reachable, which is the
property that makes ONE re-consent sufficient instead of a fourth. The guard is
re-pointed to match: it now pins the SHAPE (no modality nouns) rather than
asserting a list of things believed unreachable -- which is what it did before,
with `video` on that list, asserting something false about the product.
Watched red: 3 of 4 assertions fail against the sentence currently on main.
Also fixes the three comment defects flagged during #4879's audit ladder and
deliberately left out of it to avoid widening that diff mid-ladder:
- civitai-redis client.ts and block-registry.service.ts both put the
revocation marker TTL at 15 minutes; it is MAX_BLOCK_TOKEN_LIFETIME_SECONDS
= 14400s (4h), the dev-token lifetime it must outlive
- block-revocation.service.ts said the marker is written when "the publisher
is banned"; no such writer exists -- revokeInstance has exactly two
production call sites, and block-scope.middleware.ts marks publisher-ban
"(Phase 2)"
The SQL's "decide this before you run anything" block is replaced by the answer
and its consequence, including the precondition a merge does NOT satisfy:
civitai deploys from `release`, not `main`, so the generic sentence must be
confirmed rendering in the production consent modal before the grants are
re-taken.
194 files / 4929 tests green; typecheck 0 errors; prettier and eslint clean on
every changed file -- the last of those is the gate I missed on #4879 and only
caught at merge time.
* fix(blocks): use "AI work", not "AI generation services", and record decision 6 as superseded
Round 0 of the audit ladder on this PR found two defects in it. Both are fixed
here.
1. The proposed copy reused the one root word the re-consent exists to retire.
This file's own `.sql` header says, and has said since #4879: "The per-call
cap half is still true. 'Generations' is not: the scope reaches hosted LLM
inference (chatCompletion, registered and live), which is not a generation
in any sense a reader of that sentence would have understood." That sentence
is the entire justification for revoking 15 live grants.
"Run AI generation services that spend the viewer's Buzz" is built on the
same root. On Civitai the word is narrower still -- "Generate" and
"Train a LoRA" are two distinct top-level actions -- so the draft read
NARROWER than the sentence already on release, on both capabilities the
widening actually added. It satisfied "do not enumerate" and still
re-committed the defect it was written to fix.
The copy is now "Run AI work that spends the viewer's Buzz, with a per-call
cap" -- #4879's sentence with only the enumerating clause deleted.
A new guard pins this, because the draft was caught by a human reading it
and nothing else: `does not rebuild the sentence on the word the scope
outgrew` matches /generat/ as a prefix, so generation/generations/
generating/generative are all refused. Watched RED against the rejected
draft, failing with its own assertion message and not to a neighbouring
guard: 3 red / 2 green, the two greens being `per-call cap` and `does not
enumerate capabilities`, both correct since the draft enumerates nothing.
The draft is also added to SUPERSEDED, labelled as never-shipped.
2. The copy silently reversed an operator-taken decision.
Decision 6 of appblocks-no-allowlist-decision-2026-09-15.md §5a required
this sentence to say "an app may train a model on the viewer's Buzz" in
words. Neither the PR body nor any comment recorded that it was being
dropped.
Operator, 2026-09-16: decision 6 is SUPERSEDED, because training is not
reachable -- isBillingModeImplemented accepts 'prepaidFixed' only -- so
naming it would BANK permission for a widening that has not shipped, which
is the silent scope escalation this table exists to prevent. Recorded in
the constant, in the test's guard comment, and in the .sql, each with the
reason and the re-take obligation for when #599 lands.
Also fixes two things in the .sql that the round found actively wrong against
live production:
- The ORDERING block's recognition procedure told the operator to check that
"the new sentence names language models and training explicitly". After the
release cut at 21:49Z, production DOES name language models -- so that test
now PASSES against a sentence the arc has already retracted, which is
verbatim the failure the same file calls "THE ONE WAY TO GET THIS WRONG".
Replaced with a character-for-character match plus the ?ref=release read.
- Two blocks gave opposite standing instructions: one said the sentence names
only what is reachable and grants are re-taken whenever that changes, the
other said a generic term makes ONE re-consent sufficient. Reconciled: the
generic wording removes the MODALITY treadmill; a change of KIND still costs
a new sentence and fresh grants, and training is the live example.
Trimmed ~35 lines of narrative from the constant's comment block, which
duplicated in a seventh place an arc already recorded in SUPERSEDED next to
the test that goes red.
Verification: consent-copy suite 5/5 green; mutation control above; prettier
and eslint clean on both changed TS files, each with a positive control
watched to fire (prettier flags a misformatted probe and prints an explicit
"No files matching the pattern" on a non-match; eslint errors on a planted
no-var) -- this arc has run six rounds without ever running either.
* fix(blocks): stop the comment fixes from re-committing the rot they fix
Round 1 findings 3 and 4, both in the comment-fix half of this PR.
- block-revocation.service.ts: the new docblock says no publisher-ban writer
exists and "do not reason about a ban path from here", and fourteen lines
later the catch comment still said "an uninstall/toggle/ban write path".
A reader landing on revokeInstance saw a ban writer named inside the
function whose docblock had just denied one. Dropped the word.
- client.ts and block-registry.service.ts: both new comments restated the TTL
as a literal "14400s / 4h". block-token-lifetimes.ts says in its own header
that restating it as a number is what let the revocation TTL sit at 15min
for the whole time dev tokens lived 4h -- so the fix for a wrong hardcoded
figure introduced two fresh ones, in the same shape, guarded by nothing:
the live block-revocation-ttl.test.ts asserts ttl >= lifetime derived from
real signed tokens, so raising the dev lifetime keeps every test green and
silently makes both comments wrong in the same direction as the 15-minute
claim. Both already named MAX_BLOCK_TOKEN_LIFETIME_SECONDS; the parenthesised
number is now gone and each says why it is not restated.
Prettier and eslint clean on all three files.
* fix(blocks): drop depth-dependent line cites this PR itself invalidated
Round 2 findings F1 and F4.
F1 is the ladder's own signature failure: the previous round's comment growth
moved the target of a line-number cite onto the semantically OPPOSITE call.
`block-registry.service.ts:2393` held `revokeInstance` inside
`toggleEnabled(false)` on main and at the merge-base; after round 1 (+1) and
round 2 (+1) of comment edits it holds
`await BlockRevocation.clearInstance(...)` -- the revoke CLEAR.
Two files cited it: the one-off SQL's operator-facing runbook, and
`scope-grant.service.ts` (untouched by this PR, so nothing in the diff flagged
it, and neither file is touched on main since the merge-base, so the drift
survives the merge). An operator working the SQL header's "no endpoint exists
whose purpose is revocation" bullet would open :2393, land on the clear, and
conclude that toggling an install off REMOVES markers -- inverting the
containment reasoning that bullet list exists to support.
Fix is to delete the `:NNNN` suffixes rather than re-derive them. The function
names are unambiguous and depth-independent; a line number in a comment is a
cross-reference that goes stale silently and that no test pins.
F4: the client.ts comment told the reader to "read the constant" and named a
bare `block-token-lifetimes.ts`. That file is
`src/server/services/block-token-lifetimes.ts` in the Next app -- a different
workspace package with no import path from `packages/civitai-redis` -- so the
pointer was unfollowable from where it was written. Repo-relative path now,
plus the reason it cannot simply be imported.
prettier + eslint clean on both changed files; eslint's zero earned against a
planted no-var control on the same path and config (round 2 flagged that the
previous round's eslint zero was unproven).
* fix(blocks): sweep the publisher-ban claim, and bound the SQL's ~15min window
Round 3 findings. Two of the three are addressed here; the third was WRONG and
is refuted in the file rather than applied.
## The publisher-ban sweep (round 3's 🟡-3, round 2's F2)
This PR corrected "or the publisher is banned" LOUDLY in
`block-revocation.service.ts` -- "NO SUCH WRITER EXISTS ... do not reason about
a ban path from here" -- and nowhere else, leaving five other sites still
describing a ban as a live revocation-marker writer. That made the tree
self-contradictory in the direction that matters: a moderator reading
`apps.router.ts` would believe banning a publisher kills that publisher's live
block tokens within seconds. It does not; they run to natural `exp`.
Swept all five: `apps.router.ts`, `apps-shared.router.ts`,
`block-bridge-auth.service.ts`, and the two guard tests' header comments. Each
now carries the retraction rather than a bare deletion, so the next reader sees
that the claim was checked and found false. Verified by enumeration: the only
four remaining occurrences of the phrase in `src/` are all retractions.
Round 2 routed this to "the follow-up PR that will gate publishGenerationOutputs
on posts:write:self". Round 3 was right to reject that: no such PR or issue
exists, and adding a scope gate to one endpoint has no reason to touch five
comments about who writes a revocation marker. That was an object with no
closing condition and no owner. Fixing it here instead.
## The ~15 MINUTES window (round 3's 🟡-2) -- REFUTED, and the refutation is
## now in the file
Round 3 read item 1's "900s (300s settings-scoped, 4h dev)" together with its
"up to ~15 MINUTES" conclusion and called the window 16x understated. Round 2
had examined the same thing and cleared it. Round 2 was right:
- dev-token SCOPES do not come from `app_user_scope_grants` at all.
`dev-scoped-mint.service.ts` derives them from the manifest, clamped to
DEV_TOKEN_SCOPE_ALLOWLIST and the app's OAuth ceiling -- so revoking a
consent row cannot narrow a dev token;
- dev-token SPEND never consults consent.
`if (claims.dev === true) return { ...platform, consent: null }` returns
before `getConsentBuzzBudget` is reached.
So a dev token is unaffected by this script in both directions, and waiting 4h
would not close anything. Applying round 3's fix would have made the runbook
wrong in the opposite direction -- telling an operator to wait 16x longer than
the real consent-token window.
What WAS missing is that item 1 never said which population the bound covers.
It does now, with the mechanism and an explicit "do not fix this number upward",
because two audit rounds disagreed about it and the next one should not have to
re-derive it.
Verification: 41 tests across the consent-copy, revocation-TTL, ban-session and
bridge-token guard suites pass. prettier/eslint on the changed files add no new
findings -- `apps.router.ts` is unformatted on HEAD already (confirmed against a
clean checkout) and carries 7 pre-existing no-empty-function errors before and
after this edit, so the edit was kept minimal rather than running prettier
--write, which would have reformatted 787 lines of unrelated code.
* fix(blocks): sweep the ban claim by MEANING, not spelling, and fix the authority it cites
Round 4 findings. The previous round's sweep was the finding this time.
## The sweep matched a SPELLING, and the claim has another word order
`no-unguarded-block-bridge-token.test.ts:940` still read "a revoked install, a
suspended app and a banned publisher all still drive them until the token
expires". My enumeration matched `publisher ban` and `publisher-banned`;
"a banned publisher" is neither, so it survived a sweep that reported itself
complete -- in the same file whose header comment 930 lines above it WAS swept.
It is also the worst surviving instance, because it is a live assertion
message rather than a comment: it prints when the guard test goes red, so a
developer adding a bridge proc reads it at exactly the moment they are deciding
what to do, and it tells them routing through authorizeBlockBridgeToken
contains a banned publisher. It does not -- the guard's three steps are token
validity, the revocation marker and app_blocks.status, and toggleBan writes
none of the three.
Re-enumerated by MEANING this time (any word order, case-insensitive, over src/
and packages/, with a positive control confirming 141 files match "publisher"
so the pipeline was live). Every remaining co-occurrence is a retraction or the
unrelated PublisherSubscriptionBanner component.
## The authority four retractions cite said the opposite
Two retractions offered `block-scope.middleware.ts`'s "(Phase 2)" marking as
their ONLY evidence, and that line read "Uninstall, toggleEnabled(false), and
(Phase 2) publisher-ban all write a marker" -- main verb asserting all three DO.
"(Phase 2)" cannot carry "this one does not", and it does not mean unshipped in
this tree: dev-scoped-mint.service.ts and blocks.router.ts both label live,
tested surfaces "(Phase 2)". A reader following the pointer landed on a
contradiction of the thing it was cited for.
That line now states the enumeration itself -- two production call sites, named
-- so the citations resolve to evidence rather than to a label.
## DEV_BUZZ_BUDGET_CAP was paired with the wrong reason
The SQL said item 2's cap-lift cannot compound onto a dev token because it
"carries its own DEV_BUZZ_BUDGET_CAP". That cap is 250 PER CALL, not a daily
bound, and a dev token still reserves against the full platform per-day ceiling.
The actual reason is the claims.dev short-circuit named in the clause before it.
Reworded so an operator cannot read it as "dev tokens are capped at 250/day",
and the dev-token holder population widened from "the mod" to authors too, who
hold them via the cookie dev-tunnel branches.
Verification: 34 tests across the bridge-token-guard, revocation-TTL and
consent-copy suites; prettier and eslint clean on both changed files.
Also noting, since it cannot be amended into the commit it belongs to: the
"291 rules" figure in the round-3 correction comment does not reproduce --
measured 285-287 depending on the src/ path. The load-bearing half (2 rules and
no no-var under the packages/ root:true boundary config) reproduces exactly.
* fix(blocks): finish the ban-claim citation chain, and stop asserting a false universal
Round 5 findings, all four.
A. Four files cited "block-scope.middleware.ts marks publisher-ban (Phase 2)"
as their evidence. Round 4 rewrote that line -- and the replacement
explicitly says "(Phase 2)" could NOT carry that meaning and labels shipped
things elsewhere in this tree. So every pointer cited an authority that had
just repudiated the inference form the citation used. All four now carry the
two-call-site enumeration instead of the label.
B. A false universal this ladder introduced last round, in the block those four
files are pointed at: "toggleBan (user.service.ts) calls only
invalidateSession". It is a ~250-line function that also unpublishes every
model the user owns, cancels their subscription, blocks their media, deletes
their user links and flags their comments. A maintainer asking "what does
banning do?" would have read the authoritative-looking enumeration and
concluded a ban only logs you out. The correct narrow form was five lines
away in the test message the same commit wrote: it writes none of the THREE
THINGS THIS GUARD CHECKS. Now says that, and says explicitly not to read it
as "a ban only logs you out".
C. `appblocks-no-allowlist-decision-2026-09-15.md` was cited three times,
including from the payload, as though it were a path in this repo. It is not
in this repo at all -- it lives in the private civitai/talos-infra clone. The
one operator who most needs to check the superseded decision could not open
it and had no way to know why. All three sites now name the repo.
D. Round 4's fix put four lines of dated retraction history into a LIVE
assertion message -- the surface round 4 had just identified as the worst
place for exactly this, because it prints when CI goes red. Moved to the file
header; the message is back to the instruction a developer needs.
Verification: 34 tests across bridge-token-guard, revocation-TTL and
consent-copy. prettier clean on all 7 touched files. eslint reports 10 errors,
all pre-existing `no-empty-function` in apps-shared.router.ts (same count as
before this edit; line numbers shifted +1 by an added comment line) -- read
without a pipe, since `eslint | grep; echo rc=$?` reports grep's status.
This is the last round. Rationale in the PR comment: rounds 3-5 have audited
prose the ladder itself wrote, the executable payload has been unchanged since
|
||
|
Zachary Lowden
|
4a1535b2c7 |
feat(blocks): deny platform-internal orchestrator step types, and rewrite the ai:write:budgeted consent copy (#4879)
* feat(blocks): deny platform-internal orchestrator step types, and rewrite the ai:write:budgeted consent copy
App Blocks are moving to full orchestrator access with no per-app allowlist.
Under that direction the bound stops being "an allowlist of things we said yes
to" and becomes "everything except the things that are ours". This adds that
exception set, and re-words the consent sentence the widening invalidated.
The denylist is 15 of the 50 step types the LIVE orchestrator spec carries
today (the pinned @civitai/client knows only 47): scanners, moderation
classifiers, hashing/model ingestion, the catalog's own "Platform internals"
entries, and web egress.
training and chatCompletion are deliberately ALLOWED. Moving training out of
the platform-internal set reverses an earlier classification and was an
explicit operator decision -- it creates model versions and is the largest
per-call Buzz cost in the catalog, so the new consent copy names it in words
and a test pins it rather than leaving it to a comment.
ageClassification, mediaRating and wdTagging are in the denylist on a
moderation-oracle argument that is mine, not the operator's, and both the
module docblock and a named test mark them reversible: with moderation moved
to the publish boundary, an app that can call the platform's own classifiers
can iterate content until it passes.
SCOPE: the denylist bites in exactly one place today, and the code says so.
No current wire arm lets a block name an arbitrary $type -- kind:'step' is
enum-bound to REGISTERED_STEP_IDS and textToImage/customComfy build their own
-- so the live property is "a platform-internal type cannot be REGISTERED as a
block step", not "a block cannot submit one". When the wide kind:'steps' arm
lands it must call assertStepTypeAllowed on the submitted $type itself.
The guard was UNREACHABLE where it was first placed. Sitting after clause (9)
it never executed: the posture/$type agreement clause rejected a mutated entry
earlier, with its own error. Only the test asserting
PlatformInternalStepTypeError SPECIFICALLY exposed that -- a generic
.toThrow() would have been green over dead code. It is now clause (0a), first
after the identity check, which also gives an author the right error.
Consent copy: the per-call cap half of "Submit generations with a per-call Buzz
cap" is still true; "generations" is not. Rewriting the string does NOT re-ask
anybody -- consent is stored per (user, app) and the lookup never reads the
version column it stamps -- so the re-consent SQL is included, as raw SQL that
nothing in this repo executes. It is applied BY HAND and ONLY AFTER the copy is
confirmed live; reversed, users re-consent to the old sentence and both halves
still look done.
Tests, with the matrix watched rather than assumed:
- consent copy: 4/4 RED at origin/main, green here
- denylist seam: removing clause (0a) kills exactly the seam test, on the
absent PlatformInternalStepTypeError -- so the guard is reachable and its
deletion is detected
- blocks + schema suites: 194 files / 4928 tests green; typecheck 0 errors
Refs clawgate #598. Does not close task 188 (user-facing withdraw), which is
adjacent and deliberately untouched.
* fix(blocks): round-0 audit — retract 3 denylist entries, fix a false immutability claim, reconcile the mediaHash note
Round 0 (requirements & deletion) found four things, all verified before acting.
1. The exported denylist was NOT immutable and the docblock said it was.
`Object.freeze(new Set([...]))` is inert: Set.add/delete write internal
slots, not properties. Measured -- `.delete('xGuardModeration')` SUCCEEDED
on the frozen Set and the guard silently stopped denying it. A false safety
claim is worse than none, because it stops the next reader looking. Now a
frozen ARRAY (which genuinely rejects push/splice) plus a module-private
lookup Set that is never exported, and a test that pins it by attempting
the mutation. The same trap is documented one container over on
STEP_TYPE_ACCEPTABLE_POSTURES -- this module had repeated the mistake it
warns about.
2. ageClassification / mediaRating / wdTagging RETRACTED to ALLOW, 15 -> 12.
The moderation-oracle argument was mine, was flagged reversible, and did
not survive attack: it buys loop SPEED not capability denial (the same
grading signal returns from the publish boundary anyway); it denied the
good-citizen case, sharply so for ageClassification, where it stopped an
app checking whether an image depicts a minor BEFORE touching it; and
wdTagging is a WD14-family tagger, in the set only by association. This is
also the reading closer to the operator's own class list. The retraction
and its three reasons are recorded rather than deleted, and pinned by a
test, so they are not re-derived.
3. Denying mediaHash silently falsified a plan eight lines up in the file the
diff edits: "adding it later is one file plus one line here -- which is the
entire point of the registry". Following it would now be a BOOT failure,
not a test failure, since the registry fails fast at import. The two
judgements genuinely conflict; that note now records which one is newer and
that reopening the decision comes first.
4. The unused PLATFORM_INTERNAL_STEP_TYPE_LIST export is gone. Zero
production consumers -- its only test asserted that an export existing for
the test was sorted.
Also fixes a test comment that said "clause (9a)" where the code says (0a) --
mildly worse than a typo here, because clause (9) is exactly where the guard
was dead.
12 denied / 38 allowed, all 12 re-verified present in the live spec.
194 files / 4928 tests green; typecheck 0 errors.
Still open and NOT addressed here: the consent copy promises "training
models", which no wire arm and no implemented billing mode can deliver
(isBillingModeImplemented accepts 'prepaidFixed' only). That is a requirement
question for the operator, not a code fix.
* fix(blocks): the consent copy names only what is REACHABLE — drop the training promise
Round 0 finding F1, operator-resolved: do not promise a capability that does
not exist.
"training models" is gone from the sentence. Training is ALLOWED by the
denylist, but it is not reachable: no wire arm accepts it, and
isBillingModeImplemented accepts 'prepaidFixed' ONLY, so a variable-cost
training step cannot even be registered. The registry's own Tranche-1 note
rejected imageGen and imageUpscaler for exactly that reason.
Why it mattered rather than being a wording nit: the re-consent SQL is about
to revoke live grants so users re-agree. Re-consenting to a capability that
has not shipped BANKS the permission -- and when the wide wire does land,
nothing re-prompts, because consent is stored per (user, app) and no lookup
reads the version column it stamps. app_user_scope_grants exists to stop
exactly that; its migration header names it "silent scope escalation". The
fix preserves the re-prompt for the widening that actually needs it.
"running language models" stays: chatCompletion is registered and live, so
that half is a real widening beyond "generations" and is the gap this
re-consent legitimately closes.
Adds a NEGATIVE guard asserting the copy does not promise training, with the
reason in the test, so it is not pre-loaded back in by someone who reads the
denylist and assumes reachability follows. The SQL header now records the
same, including that a future capability means changing the sentence AND
re-taking the grants again -- the intended cost, not an oversight.
194 files / 4929 tests green.
* fix(blocks): round-1 audit — drop the unreachable "video" promise, pin the whole denylist, correct the SQL
Round 1 (nine axes) confirmed the guard is reachable and correctly placed
(deleting clause (0a) AND moving it below clause (1b) were both killed by the
seam test), and confirmed the consent mechanism claim. It then found five real
things. All verified before acting.
1. [BLOCKING] The copy promised "and video", which is NOT reachable -- the
identical defect the previous commit removed "training" for, one word over.
blockWorkflowBodySchema has three members; textToImage is bounded to
BLOCK_IMAGE_WORKFLOW_TYPES (txt2img/img2img/img2img:edit), both registered
recipes are image, both registered steps are convertImage/chatCompletion,
and workflow.schema.ts calls a non-image media class "a later phase".
Dropped.
The guard written to prevent exactly this was NARROWER THAN ITS OWN NAME:
titled "does NOT promise a capability that is not reachable yet" while
asserting a single literal, 'training'. A description claiming a CLASS over
a body checking one member -- and the gap was already occupied. It is now
named for what it does, checks the capabilities we have twice caught
ourselves pre-promising, and says in the file that it is a SPELLED guard
that a reword walks past. The real control is the whole-string pin plus a
human enumerating the wire arms, and that is now written down as the rule:
reach for the WIRE, not the denylist -- a $type being allowed says nothing
about whether any arm accepts it.
2. The denylist set was 3-of-12 mutation-covered. Measured: deleting
modelClamScan, imageScanning, shieldstralModeration, mediaHash,
modelParseMetadata, comfyNodepackSnapshot, qwenImageBench, webScrape or
webSearch left the suite FULLY GREEN. Nothing else in the repo pinned
membership. Now pinned as the whole sorted set -- re-verified by deleting
webScrape, which previously survived and is now killed.
3. The SQL's central claim was wrong in the LOOSENING direction. It said a
revoked row "reaches no spend path" and the read path is "fail-closed
today". Neither holds for already-minted tokens: block JWTs have no
revocation list and no jti, default TTL 900s (300s settings, 4h dev). Worse,
the revoke removes the user's OWN CAP FIRST -- getConsentBuzzBudget returns
null for a revoked row and reserveBlockBuzzSpendForClaims then takes the
no-consent-reservation branch, falling back to the platform ceiling alone.
A user with a 500/day app budget has it lifted for the life of their token.
Header now states the window and the inversion, and says to run it when
spend is quiet.
4. The SQL's verification gate told the operator to compare a column named
`revoked_now` that the query never emitted, against a total that conflated
this run with prior ones. Now emits revoked_by_this_run and
revoked_before_this_run separately, keyed on now() -- which is
transaction_timestamp() and therefore the same value STEP 2 wrote
(statement_timestamp() would NOT be, and would have reported zero).
The bare COMMIT is also gone: under `psql -f` every statement ran and it
committed before a human could read the gate, making the gate decorative.
It is now commented out with instructions to step through interactively.
5. Documented that the row-level revoke drops co-granted scopes
(user:read:self and friends) until re-consent, since that will generate
support reports that look unrelated to Buzz.
Also: stale clause label in a test comment, and two docblock claims that
contradicted the file they sit in (one still said the scope reaches "model
training"; one asserted a live grant count the SQL explicitly says not to take
from a document).
194 files / 4930 tests green; typecheck 0 errors.
NOT addressed, deliberately: round 1's finding that no ratchet forces a future
kind:'steps' arm to call assertStepTypeAllowed. A ratchet for a call site that
does not exist yet would be a declaration with zero instances; it has to land
with the arm. Recorded in the PR instead.
* fix(blocks): round-2 audit — a revocation primitive DOES exist, the runbook omitted BEGIN, and the source docblock still asserted the refuted claim
Round 2 was a delta re-audit of the round-1 fixes. It confirmed 3 of 8 claims
outright and found that 4 were only partially discharged. All verified before
acting. Every finding this round is in PROSE the previous round wrote -- which
is the documented shape of a fix round's next defect, and it held.
1. "Block tokens are JWTs with NO revocation list" -- FALSE, and I wrote it.
BlockRevocation (block-revocation.service.ts) sets a per-blockInstanceId
Redis marker with TTL = max token lifetime, and it IS checked on the
block-scope path: block-scope.middleware.ts, block-bridge-auth.service.ts,
apps.router.ts, apps-shared.router.ts. Claiming otherwise told the operator
not to look for the mitigation that exists. Corrected, with the three
caveats that decide whether it helps: it is per-INSTANCE not per-user, this
SQL does NOT trigger it (a Postgres write sets no Redis marker), and
isRevoked FAILS OPEN on a Redis error.
2. The interactive recipe never said BEGIN. The now() reasoning added last
round is correct Postgres semantics but is entirely conditional on a
transaction the recipe omitted. Followed literally under autocommit, STEP 2
self-commits, the ROLLBACK escape disappears, and STEP 3 evaluates now() in
a DIFFERENT transaction -- so revoked_by_this_run reads 0 and
revoked_before_this_run reads N: the exact inverse of the documented pass
condition, on a run that succeeded. BEGIN is now step zero and shown in the
sequence.
3. The -f warning described a hazard the same commit had removed: it warned
about a COMMIT that is now commented out. What -f actually does now is leave
the transaction open to EOF and roll back at disconnect -- AFTER STEP 3 has
printed a convincing success readout. Safe direction, useless signal. The
warning now says that instead.
4. The video question is elevated from a comment to a BLOCKING precondition at
the top of the SQL. customComfy mode:'inline' is the one arm bounded by no
enum, and the read path does not filter by media type -- workflow.service.ts
pushes every available output.blobs[].url into imageUrls with no check. So
if a stock-node graph can emit video, the copy UNDER-names a reachable
capability, which is the worse direction for consent. Answer before running,
not after; re-taking grants is the expensive half.
5. scope-grant.service.ts still asserted, in the docblock a code reader hits
first, exactly what the SQL header now calls false: "a revoked user's token
... can reach no spend path at all". Two in-tree documents disagreeing about
the one fact the operator needs. Rewritten, including the counter-intuitive
ordering -- the revoke drops the user's own ceiling BEFORE it drops the
scope -- and the "currently unreachable" framing is corrected, since this
PR ships the hand-applied writer that makes the branch reachable.
194 files / 4930 tests green; typecheck 0 errors.
* fix(blocks): round-3 audit — BlockRevocation is AUTOMATIC not operator-invoked, and the mitigation I told the operator to use is not followable
Round 3 found three things, all of them in prose, two of them introduced by
round 2's own fix. Verified before acting; the shipped behaviour is unchanged.
1. "operator-invoked" is WRONG. `revokeInstance` has exactly two production
call sites and both are AUTOMATIC -- uninstall (block-registry.service.ts
:2358) and toggleEnabled(false) (:2393). There is no admin router, no tRPC
procedure, no script; confirmed by enumerating every non-test reference.
The consequence is the opposite of what the word implies: the middleware's
403 branch is HOT, not cold, because a marker appears whenever a USER acts.
Round 2 set out to stop two in-tree documents disagreeing about this and
made it three.
2. The instruction round 2 added -- "revoke the affected instances as a
SEPARATE action" -- CANNOT BE FOLLOWED, which is worse in a runbook than
the omission it replaced. There is no operator surface; the only manual
routes are toggling every install off (which also DISABLES it, a different
and user-visible outcome) or hand-writing the Redis key. And the file gives
no way to enumerate the instances anyway: it works on
app_user_scope_grants, keyed (user_id, app_block_id), while
block_instance_id lives on block_user_subscriptions. The header now says
what is actually available -- time the window, do not try to close it --
and says why, rather than sketching an untested join.
3. The (0a) placement note cited "clause 7-ish" as the guard that pre-empted
it. The clause that rejects a posture/$type mismatch is (1b) at :1667;
clause (7) is the resource-policy gate and is unrelated. The mechanism the
note describes is correct -- only the label was wrong -- but a reader
checking (7), finding nothing that shadows (0a), would conclude the
placement note was bogus and move the guard back down. Now cited by label
and line.
Also: the test docblock said assertStepInvariants has "a dozen" clauses; it
has 20 labelled ones.
Round 3's once-per-ladder re-derivation against the current head checked every
count, version, path and cross-reference this PR asserts -- denylist size and
membership, REGISTERED_STEP_IDS, the three schema members, both recipes, the
BLOCK_IMAGE_WORKFLOW_TYPES bound, isBillingModeImplemented, the 900/300/14400
token lifetimes, the single non-test call site of assertStepTypeAllowed, and
every backticked file reference in the six touched files. All correct except
the clause label above.
194 files / 4930 tests green; typecheck 0 errors.
NOT fixed, deliberately out of scope: packages/civitai-redis/src/client.ts:2324
and block-registry.service.ts:2387 both describe the revocation marker TTL as
15 minutes; it is MAX_BLOCK_TOKEN_LIFETIME_SECONDS = 14400s (4h). Real, but in
files this PR does not otherwise touch -- widening the diff mid-ladder is how
ladders stop terminating. Flagged on the PR instead.
* fix(blocks): round-4 audit — "no tRPC procedure" was false, and the same commit contradicted itself
Round 4 found one thing. It is the THIRD wrong claim in a row about the same
sentence, which is why the fix states a narrower property instead of reaching
for another confident one.
The sequence, kept so nobody derives a fourth:
draft 1: "BlockRevocation is operator-invoked" -- FALSE
draft 2: "there is no admin router, no tRPC procedure, no script" -- ALSO
FALSE, on the middle term
now: "no call site exists whose PURPOSE is revocation"
Both call sites ARE reachable over tRPC -- blocks.router.ts:1848
(uninstallFromModel) and :1810 (toggleEnabled), both protectedProcedure -- and
assertCanManageBlocks early-returns for moderators (:1521). So a mod CAN set a
marker deliberately, against any user's install on any model. Verified by
enumerating every reference and then the callers of both methods.
Worse than an isolated error: the SAME COMMIT contradicted itself. The SQL
header said one available by-hand path is "toggling every affected install OFF
in the UI" -- which IS a tRPC procedure -- while scope-grant.service.ts said no
tRPC procedure existed. Its "only ways" enumeration also omitted
uninstallFromModel, which is mod-reachable for any model and writes the same
marker.
The property that actually holds, and the one an operator needs: you cannot set
a marker WITHOUT also uninstalling or disabling that install. Every route
carries a separate, user-visible outcome -- which is why none of them is a
quiet token revocation, and why the runbook's advice is unchanged: time the
window, do not try to close it.
Also corrects the inherited consequence. "A marker appears because a USER
acted, not because someone chose to write one" was drawn from the false claim
and is gone; the 403 branch is hot either way, because ordinary users hit both
paths routinely.
194 files / 4930 tests green; typecheck 0 errors. Delta is comment-only.
Still flagged, still deliberately unfixed: the two out-of-PR comments putting
the revocation TTL at 15 minutes (it is 14400s). Correct cite is
block-registry.service.ts:2388 -- my previous message said :2387, off by one.
* fix(blocks): round-5 audit — DELETE the "403 branch is HOT" claim instead of writing a fourth reason for it
Round 5 caught the shape this ladder keeps producing, in the commit that was
supposed to have stopped producing it.
Round 4 correctly deleted the REASON draft 2 gave for "the middleware's 403
branch is HOT" -- and then kept the CONCLUSION and supplied a fresh
justification: "because ordinary users hit both paths routinely". That is
draft 4 of one sentence, and it is false: both mutations carry
enforceAppBlocksFlag (blocks.router.ts:1811, :1849), and the live
app-blocks-enabled flag is base-false with a moderators-only segment
(:398, :442; app-blocks-flag.ts:207). An ordinary user cannot reach either
mutation at all.
So the fix is a DELETION. The comment now states that the branch's exercise
rate is NOT ESTABLISHED, records both dead justifications so neither is
re-derived, and says explicitly not to write a third -- the rate depends on
live Flipt state and install behaviour, neither readable from source. If you
need the number, measure it.
This is the rule the previous four rounds kept not applying: when a claim has
lost its reason, write that it has none. A justification found under pressure
to supply one is a hypothesis, not a finding.
What round 5 CONFIRMED, by complete enumeration rather than reading: draft 3
("no call site exists whose PURPOSE is revocation") is TRUE -- exactly two
writers, both side-effects, no admin script, job, webhook, queue consumer or
cascade; suspension uses a different mechanism entirely. "You cannot set a
marker without also uninstalling or disabling that install" is TRUE, and both
re-entry paths clear the marker so a disable/re-enable round trip leaves no
residual. All five cited line numbers exact. The two files now agree -- the
round-4 self-contradiction is gone.
Also fixes a counter this ladder introduced: "Four things decide whether it
helps you" headed FIVE bullets, because round 4 split two bullets into three
and left the total. Same class as the round-4 defect (a statement inconsistent
with its own commit's content), so the total is removed rather than corrected
-- a number kept beside what it counts will drift again. The dropped draft-2
wording now also carries an explicit retraction marker, matching the file's
three other retractions.
194 files / 4930 tests green; typecheck 0 errors. Delta is comment-only.
* style(blocks): prettier the two files the changed-files gate flagged
The ESLint + Prettier check was RED on the PR head: the new consent-copy test
file was unformatted, and scope-descriptions.constants.ts was too (only the
added-files arm reported, so the modified file would have slipped through).
Mine, and a gap in my own verification -- I ran the unit suites and typecheck
after every audit round and never once ran lint or prettier. Tests and
typecheck cannot see formatting, so a green local run said nothing about this
gate.
Formatting only. Verified the pinned consent STRING is byte-identical after
prettier, since a test asserts it exactly; eslint clean on the whole diff;
194 files / 4930 tests green; typecheck 0 errors.
|
||
|
Justin Maier
|
32239d2d4d |
feat(licensing): turn the SellMerge write paths on (#4874)
* feat(licensing): turn the SellMerge write paths on The contract half of yesterday's expand/contract. Every pod has known the label for eleven hours and prod carries zero rows using it, so the paths that were held back can open: the schema @default and the upload form's default set go back to five values, the "Sell merges using this model" option returns, and the refine that refused the member at the upsert contract is deleted. Measured before writing this, prod replica: 424,641 models hold Sell and lack SellMerge, and 0 hold SellMerge. Nothing wrote it while the paths were shut. The two decision cases are INVERTED rather than deleted, because what they guard was never really about SellMerge. Restrictions are emitted by ABSENCE, so a member missing from a default set produces no clause and therefore GRANTS the permission -- silently, with no badge a creator would notice and no type error. Both now derive the expected set from the enum (Object.values minus None) rather than from a hand-written list, so the next member added without wiring reddens. A list would have needed updating by whoever forgot. Both cases are renamed to what they now assert; a stale test name is a comment that survives every refactor. The decision docblock is deleted rather than reworded, since the decision expires with this PR. Controls: dropping SellMerge from the schema default, from the form default, or from the option list each redden, as does re-adding a withholding refine, as does the field ceasing to validate members at all -- that last one is what stops the accept-loop passing vacuously. An innocent comment reword stays green. The backfill does NOT ride this PR. It runs after this is deployed, because until then the refine rejects a save carrying the member and the edit form resubmits it untouched. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(licensing): assert the three sites AGREE, not that every member is granted Five lanes on #4874; two findings were against my own work. The docblock stated the absence rule backwards -- it claimed a member missing from the granted array produces no clause and so GRANTS the permission. The emitter returns '' when the array INCLUDES the member, so absence emits the clause and RESTRICTS. The real hazard is one file over and about the clause map, not the default set. The parent's commit message repeated the inversion. The guard asserted a policy: both defaults equal to every enum member requires the next CommercialUse addition to be granted to every model by default, which forbids the expand/contract manoeuvre shipped eleven hours ago. Rewritten as the invariant -- the two defaults and the option list must AGREE -- so withholding a new member everywhere at once stays legal, while a member granted by default and missing from the options is caught. Three attempts, each caught by a control: a whole-file count broke because Rent is a substring of RentCivit and Sell of SellMerge; a per-member count misfired on the cascade's legitimate references. The render-site guard pins that the list is mapped whole. It does NOT cover the disabled predicate -- no text guard expresses that without naming members, which reintroduces the policy. Stated in the test as a gap. From the other lanes: the local-dev DDL was missing the enum value, so a fresh checkout would reject the first model create; the migration header and the backfill endpoint carried claims this PR made false. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * chore(seed): seed the five-value permission shape alongside the legacy ones No seeded row granted SellMerge, so the sell/merge split was unexercisable in local data. Tracked despite scripts/local-dev being gitignored -- the ignore rule does not apply to already-tracked files. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
Zachary Lowden
|
4aab099c91 |
fix(app-blocks): a no-user flag eval returns the flag's BASE value, not a deny (#4807)
* fix(app-blocks): a no-user flag eval returns the flag's BASE value, not a deny
Several docblocks in app-blocks-flag.ts derived a security property from an
inference that does not hold:
no user -> a global eval that can never match a segment -> fail-closed
The premise is true. A no-user call reaches Flipt as entityId 'global' with an
empty context, and every identity/tier/cohort segment we have is a
STRING_COMPARISON_TYPE constraint that reads the context, so none can match. The
conclusion does not follow from it: when no rollout matches, Flipt answers with
the flag's own base 'enabled' value. The denial came from the BASE being false,
not from the segment miss — so a base-true widening of app-blocks-author or
app-blocks-enabled turns every no-user branch from a deny into a pass.
MEASURED against the real @flipt-io/flipt-client-js wasm engine over a real
evaluation snapshot (new test app-blocks-flag.base-enabled-flip.test.ts):
base enabled:true + a non-matching SEGMENT_ROLLOUT, no entityId/context -> true
base enabled:false + the same rollout, no entityId/context -> false
unknown flag key, no entityId/context -> false
Both flags are base-false with segment rollouts today (civitai/flipt-state,
civitai-app/default/features.yaml), so nothing is exposed now. This is the latent
gate closed before any base-true flip, not a live defect.
Code:
- isAppBlocksAuthorEnabled: an undefined user now returns false structurally
instead of falling through to a global eval. Enumerated: all 10 call sites pass
{ user }; none wants a global eval of this key.
- blocks.router assertAppBlocksEnabledForTokenUser / assertViewerIsAppDeveloper:
refuse an unhydratable token subject before consulting the flag, with a distinct
message. Same shape apps.router.ts already uses.
- apps-shared.router resolveSharedContext: separate a VANISHED subject (refuse)
from an ANON token (keep the global eval — that widening is intended). The
read ops list/get had no second belt behind the flag.
- isAppBlocksEnabled's no-user branch is deliberately KEPT: it has a real caller
(pages/api/v1/developer/block-manifests.ts) that wants the base value.
Comments: every fail-closed / fail-safe claim in app-blocks-flag.ts swept and
given an accurate statement of what makes it closed; a new GLOBAL-EVAL SEMANTICS
block records the mechanism and the measurement once.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* refactor(app-blocks): make the compiler the guard — require a subject on the author capability
Round-1 review: the structural guard I said needed type-level nullability analysis
is analysis this repo already runs on every PR (tekton / typecheck, App unit tests
+ typecheck). Measured, then implemented.
isAppBlocksAuthorEnabled's parameter is now REQUIRED and non-nullable:
(opts?: { user?: SessionUser }) -> (opts: { user: SessionUser })
Making it required errored at exactly 2 of the 10 call sites — both bare
middleware(...) whose ctx.user is not narrowed by the protectedProcedure they are
attached to (app-listings.router.ts, app-collaborators.router.ts). Each now refuses
explicitly instead of handing a possibly-undefined subject to an authz gate. The
other 8 already held a non-null subject. The runtime `if (!user) return false`
branch this makes dead is DELETED rather than kept as defence in depth, and the two
runtime tests that pinned it are deleted with it: they could only be re-added behind
an `as never` cast, i.e. testing a path the type system forbids while reading as
coverage. What replaces them is the typecheck gate, plus one test pinning the
residual the docblock now states — a cast-defeated call THROWS, never returns true.
isAppBlocksEnabled keeps its optional overload. That asymmetry is now justified on
SEMANTICS (a kill-switch answers "is the feature on at all", which a subject-less
machine path may legitimately ask and which the base value is; a capability answers
"may THIS subject", unanswerable without one) rather than on its sole no-arg caller
existing — block-manifests.ts is dormant, so that justification is the half that can
vanish.
Also:
- Delete fixtures/flipt-base-enabled-flip.snapshot.json (152 lines). Derived at
runtime from the sibling fixture by re-key + enabled flip. A checked-in twin cannot
track the original — re-capturing the source means re-anonymising it, so the copy
silently keeps the old segment shape while claiming production fidelity, and it had
already lagged a segment.
- Consolidate ~60 lines of duplicated harness into fixtures/flipt-fixture-server.ts,
shared with the pre-existing real-flipt-client integration suite. Only the snapshot
differs between them; the instrument is the same.
- Stop enumerating live flag state in the GLOBAL-EVAL SEMANTICS header — that is the
same rot class the paragraph warns about. Keep the imperative, point at flipt-state.
- apps-shared: say READ_OPS rather than naming two of its four members.
- Replace a rejects.not.toMatchObject(...) positive control, which passes on any
other rejection, with the specific NOT_FOUND / 'Block install not found' outcome.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* fix(app-blocks): watchlist the two new refusals — the type guard cannot see a bundler
Round-1 review, 2 x yellow. Both taken.
WATCHLIST (the important one). My stated defence for deleting the helper's runtime
branch — "deleting it is a type error rather than a silent re-opening" — is a claim
about SOURCE. Release 5.1.18 is the case where the source is correct and the emitted
artefact is not: the bundler dropped two of three returns from a function in
app-blocks-flag.ts and served the whole App-store catalog to anonymous callers
(civitai#3983). That is why scripts/compiled-branch-watchlist.mjs exists, enforced at
Dockerfile:103, and two of the three refusals this PR adds are pure runtime branches
whose loss silently restores the exposure it closes. Both are now listed:
shared-storage-subject-refusal apps-shared.router.ts — lost, every READ_OPS op
serves shared rows to a vanished subject
block-token-subject-refusal blocks.router.ts — lost, the kill-switch is
evaluated with no subject on 16 runtime procs
assertViewerIsAppDeveloper's guard is deliberately NOT listed, and that is measured:
isAppBlocksAuthorEnabled takes a non-nullable subject and dereferences it at once, so
losing that guard throws rather than passes.
VERIFIED AGAINST A REAL BUILD, not just added. Full `next build` (green), then the
gate exactly as the Dockerfile runs it:
positive 26,600 maps scanned; both new entries report OK with their control
anchors mapped (an unmapped control is exit 2, not a pass)
negative required anchors repointed at unmapped comment lines in the same
functions -> exit 1 naming both entry ids, controls still mapping
deletion the required line removed in a throwaway source copy, one entry at a
time -> exit 2 naming that entry and its anchor
The two refusal messages were identical strings under different codes, so neither was
anchorable and "separable in a log" was not true. The author-gate message is now
'app-authoring subject could not be resolved'.
ROT (the second yellow). The isAppBlocksAgenticReviewEnabled docblock I wrote for this
rot class had the rot: it called the absent-flag half "load-bearing while this flag
does not exist in Flipt", but app-blocks-agentic-review has existed since 2026-07-21
(live: base false, `moderators` segment). No live exposure — all three call sites
check isModerator first — but it is a false claim in the one paragraph my own sweep
rewrote. Re-derived every app-blocks flag's live state rather than fixing only the
reported line, and found a second instance I had authored: the dev-tunnel docblock
offered "absent" as a live unconditional closure for a flag that also exists. Both
corrected; the only flag in this file genuinely absent from Flipt is
app-blocks-backpay-enabled.
Nits: assertAppBlocksEnabledForTokenUser has 16 call sites, not 17 (17 is the
parseSubjectUserId count; the substantive claim was right). deriveSnapshotFromFlagShape
now rejects an EMPTY rollouts array, which its error text already claimed to reject.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* docs(app-blocks): resolve the two docblocks that answer the reader twice, oppositely
Follow-on from the same re-check that found the agentic-review rot. Eleven docblocks
in this file carry an inherited "the flag does NOT exist in Flipt at merge time"
sentence; the new header frames all of them as as-merged history, which is the
deliberate treatment (substituting a fresher enumeration would be the same rot one
generation on).
But in exactly TWO docblocks that inherited sentence sits in the SAME block as a
live-state sentence I wrote, so the reader gets opposite answers eight lines apart:
isAppListingsEnabled "does NOT exist" vs "Both are base-`false` today"
APP_BLOCKS_SHARED_STORAGE_FLAG "does NOT exist" vs "Closed today because the base
is `false`"
Both inherited sentences moved to past tense and marked as as-merged notes. Nothing
else changed: the other eleven are untouched, because they carry no competing claim.
Criterion for the split, so it can be re-applied: fix where a paragraph contradicts
itself; leave where the header already governs.
Re-verified after the edit rather than carrying the previous measurement over — these
are comments, but they shift line numbers in a WATCHLISTED module, and source/artefact
correspondence is exactly what this PR stopped assuming. Fresh `next build` (green),
then the compiled-branch gate: positive OK on all three entries (26,600 maps; controls
mapped), negative control still exit 1 naming both new entries.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* docs(app-blocks): say at each watchlisted refusal that it is watchlisted
The compiled-branch gate names the entry, module, line and reason when it fires — but
only at Docker build time, after someone has already deleted the branch and pushed. The
two refusals this PR puts on the watchlist carried no signal in the file itself, so a
reader deciding whether the guard is load-bearing had nothing to go on. Two comments,
one at each site, naming the entry id and what a bundler can do to a pure runtime
branch.
Deliberately scoped: moving or rewording the throw is fine (the gate resolves its
anchor from source at run time), deleting it is not.
Re-verified after the edit, for the same reason as the previous commit: comments shift
line numbers in watchlisted modules. Fresh `next build` (green), gate positive OK on
all three entries, negative control still exit 1 naming both new entries. That is the
third independent repetition of both controls.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* fix(app-blocks): re-anchor the watchlist on format-robust text, and stop telling people to reword it
Round 2. Five defects, all introduced by this PR's own later commits.
F1 (the important one). The comment I added in
|
||
|
Zachary Lowden
|
61a6b068aa |
Fix lost output before notification on background task exit (#4442)
When the dev-server test queue's waiter functions (cmdTestWait in cli.mjs,
runQueued in test-unit-run.mjs) finish polling and call process.exit(),
Node terminates immediately without flushing pending stdout/stderr writes.
This means:
- The exec/process tool's completion notification fires while the
capture file is still 0 bytes
- An agent checking the output sees an empty file and has no reason
to doubt the 'green' notification it just received
- Both the exit code and the log agree, but both are wrong — a red
run reads as green
Fix: replace process.exit() with drainThenExit()/exitAfterDrain() in
every waiter exit path that has just printed output. The helper writes
an empty string to both streams and waits for the kernel-level flush
callback before exiting. A 500ms safety timer prevents hangs if a
stream is in error state.
Fixes civitai/civitai issue 868ktvqf9
- The exit code is still from exitCodeFor() — no rule change
- The output file is now non-empty when the notification fires
- Applied to all three exit paths in cmdTestWait + the terminal path
in runQueued
Co-authored-by: DevPod Agent <agent@devpod.local>
|
||
|
briant
|
c681b72c9d |
chore(clickhouse): drop the one-shot user_activity_rollup apply script
The backfill it existed for ran on 2026-09-04 and the table is now kept current
by the user-activity-rollup cron, so this is a tool with no remaining caller.
Nothing is lost by deleting it. The statements themselves live in the tracked
migration (src/server/clickhouse/migrations/2026-09-04-user-activity-rollup.sql)
along with the measured actuals from the run, and the script is recoverable from
|
||
|
briant
|
a78e58362e |
chore(clickhouse): apply script for user_activity_rollup, and the real backfill numbers
The migration is 30 statements against a cloud console that drops connections, which is how half a backfill happens without anyone noticing. The script runs them in order with backoff retries on transport errors only — a syntax error fails immediately rather than burning five attempts and burying the message — and prints a resume command naming the partition it died on. Dry run by default; --apply writes. Applied to prod 2026-09-04, 246s, no retries needed. Replaced the predicted "~7.4M distinct users" with the actual 10,719,260 in both the migration and the script's threshold: the estimate only counted pageViews and hand-waved the rest, and the other three sources turned out to add 3.36M accounts that have loaded no page since 2024-09-26. The split is also the arm-by-arm correctness check, so it is written down: 10,719,260 total less 3,361,661 with no country leaves 7,357,599, against the 7,356,496 distinct users pageViews held a few hours earlier — and only pageViews can set a country. Verified end to end after applying: the page's own query returns in 0.56s for a real 15,000-follower set, and 86.2% of those followers have a country, matching the figure measured independently against pageViews before the table existed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
Justin Maier
|
8085d81651 |
chore(articles): remove the official-articles backfill script
It was a one-off and it has run. Justin's call: not worth keeping in the repo, and not worth a PR to remove. What it did, recorded here because the code is going and the fact should not go with it. Prod, 2026-09-04: --user-ids 12042163,1,3,43555,5418 --exclude-ids 6222,6339,6454,6338,5344,28893 --apply 214 articles marked — Maxfield 65, JustMaier 58, CivitaiOfficial 51, Faeia 38, theally 2. Read back afterwards rather than trusting the update count: 214 total, 0 marked outside those five accounts, all six excluded ids confirmed still false. Excluded five abandoned drafts titled "test"/"rtert", and 28893 "Farewell, Civitans!" — a departing staff member's goodbye rather than Civitai speaking. CivBot's 438 automated posts were deliberately left out. Anything marked from here on is marked by a moderator: the ⋯ menu on an article, the toggle in the editor, or `article.mjs --official`. To reverse the backfill, take the file back out of history: git show <this commit>^:scripts/backfill-official-articles.mjs > backfill.mjs node backfill.mjs --user-ids 12042163,1,3,43555,5418 --unmark --apply Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XHZuQDTCG159qcPrCkP7SF |
||
|
Zachary Lowden
|
4ede37c9c5 |
ci: re-pin BASE_TESTS to 37,857 — the unit shard gate is red on main (#4621)
* ci: re-pin BASE_TESTS to 37,857 — the unit shard gate is red on main `scripts/ci/assert-shard-ran.mjs` pins `BASE_TESTS = 22157`, measured 2026-08-25. The suite has since grown to 37,857 executed across 1,626 files, so shards 3 and 4 exceed the derived ceiling of 9,971 and the job fails with every test passing. This is red on `main`, not just on PRs: runs 33900980756, 33901008291 and 33901251166 all fail the same two shards on unrelated commits. A permanently-red gate trains people to click through, so it is re-pinned rather than worked around. Re-measured from run 33900980756: 7177 + 5769 + 14724 + 10187 = 37,857. Cause (2) confirmed against cause (1) by the script's own discriminator: the four counts are unequal proportionate shares (2.55x spread) and each shard ran ~4 min against a ~10.4 min full suite. `--shard` failing to reach vitest would instead give four near-identical full-suite counts at full-suite runtime. Also corrects two claims in the comment that are no longer true and that are the stated justification for the 0.3 / 1.8 multipliers: - imbalance was "~5% off the mean"; the heaviest shard is now +55% - ceiling headroom was "~1.7x"; it is now 1.16x vitest shards by file, so a few test-dense files skew a shard badly and the next one trips the ceiling well before suite growth would. Recorded in the comment: if this reds again on a green suite, balance the shards rather than raising the number again. Verified: the new band [2839, 17036] admits all four current shards, and still rejects a collapsed shard (0, 99) and a shard that ran the whole suite (37857). * ci: make the shard-gate tests follow BASE_TESTS instead of restating it CI caught what the previous commit missed: re-pinning BASE_TESTS turned `Unit tests (3)` and `(4)` green but broke `Unit tests (1)`, because scripts/__tests__/assert-shard-ran.test.ts carried a SECOND copy of the constant — `const BASE_TESTS = 22157`, under a comment saying it was "kept in step with BASE_TESTS in the script". Kept in step by convention, which is what failed. Three boundary tests asserted a stale band while the script itself was correct. The test now reads the constant out of the script source. Parsing rather than importing is deliberate: the script is a CLI that reads process.argv and calls process.exit at module scope, so an import would execute it. The regex throws a named error if the constant is renamed, so that fails loudly instead of silently falling back to a default. Also fixes the third failing test, which read `const count = 3000` under a comment claiming it was "computed rather than hardcoded so it follows BASE_TESTS". It did not follow it. When BASE_TESTS moved the interesting region slid from 1662..3324 to 4259..5679 and 3000 fell out of it. It is now derived from the two bounds, with an assertion that the region is non-empty so a multiplier change fails loudly rather than silently picking a midpoint that satisfies neither bound. Validated rather than assumed: - constant moved to 50000 -> 14/14 still pass (the tests follow it) - constant renamed -> throws the named error, 1 file failed - restored -> byte-clean, 14/14 pass * ci: retrigger preview checks (empty) |
||
|
Justin Maier
|
ac9117af6a |
feat(articles): official toggle in the editor, and an official filter on the feed (#4630)
* feat(articles): official toggle in the editor, and an official filter on the feed Follows #4624, which added the column, the moderator-only mutation and the badge. Justin asked for three more things: the mark reachable from the article editor rather than only the ⋯ menu, the agent CLI able to set it, and a way to browse official articles. The CLI half is in the civitai-user-skill repo; this is the app. 🔴 A non-moderator's `isOfficial` is DROPPED, not refused. That distinction is the whole design and it is not defensive coding: the edit form seeds itself from the article, so an owner editing an article a moderator had marked would send the flag straight back. Refusing there locks the author out of their own article with a bare UNAUTHORIZED and nothing explaining it — which is exactly the defect the review found in the tag version of this feature (#4618). Dropping it means their save succeeds and changes nothing about the mark, because `upsertArticle` leaves the column alone when the field is undefined. Both halves are tested, and the "still saves" half is the one a future tidy-up will break. The editor toggle sits in the moderator-only panel beside Locked properties, and the form omits the key entirely for everyone else rather than sending a value the server would discard. The feed filter is a `Civitai Official` chip in the filters dropdown, plus `?isOfficial=true` so an official feed is a link you can send. **Only `true` filters.** An explicit `false` is treated as absent, deliberately: nobody browses FOR community articles, and a `false` that filtered would let a stale url quietly hide every official article from someone's feed. That is asserted on the emitted SQL rather than on the arguments — a test that only checked the service was CALLED with the flag would pass for a service that ignored it, which is what "the filter does nothing" looks like to a user. Controls, every mutation red, pristine 8 passed (5 + 3): non-mod value passed through instead of dropped → 2 failed refuse instead of drop → 3 failed drop becomes `false` instead of undefined → 2 failed WHERE clause removed → 1 failed WHERE clause applied unconditionally → 2 failed `false` filters too → 1 failed ⚠️ One of those tests was written wrong first and is worth the warning it now carries: the SQL helper read only the template strings, but `getArticles` interpolates its WHERE clause as a VALUE, so it captured SQL that could never contain the filter — and both negative assertions passed against it. Every assertion in that file now carries a `FROM "Article" a` control so an empty capture fails loudly instead of quietly. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XHZuQDTCG159qcPrCkP7SF * feat(articles): backfill script for the official mark `isOfficial` starts false on every existing article, so the badge is invisible until the back catalogue is marked. The rule is `constants.system.officialUserId` (12042163) — the same id `resource-select.service.ts` already uses to mean "official" — with `--user-id` to point it elsewhere. 🔴 Dry run by default. It writes nothing without `--apply`, and `--unmark` reverses it. The column is a public provenance claim, so a backfill that marks the wrong author is a false claim on somebody else's writing. Two things it prints that nobody asks for and everybody wants afterwards: the database it is actually connected to (`.env` here has been swapped between dev and prod before), and the count of articles marked by SOMEONE ELSE that this rule would not have set — so a hand-marked article cannot be silently overwritten by the rule without you seeing it first. The apply path re-reads the count afterwards rather than trusting `rowCount`, and exits non-zero if the two disagree. Exercised against dev: dry run listed 49, `--apply` updated 49, read-back confirmed 51 (2 were marked by hand first). NOT run against prod. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XHZuQDTCG159qcPrCkP7SF --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
briant
|
938d281e4c |
fix(tests): make five suites pass on Windows
All eleven failures were path and spawn portability bugs in the tests, not defects in the code under test, so CI never saw them. Every fix is a no-op on Linux, so no guard is weakened. - credential-detection-superset.guard, appModeratorMessageForm.callSites: path.resolve() yields '\' on Windows while the ledgers are written with '/', so both compared two spellings of the same module. The credential guard was reporting bearer-token.ts as neither scanned nor declared. - blocks/tools/registry: new URL().pathname is '/C:/...', which readFileSync resolved to 'C:\C:\...'. Pass the URL itself. - assert-component-suite-ran: the gate names the missing file by its on-disk path, so the separator is the platform's; the assertion hard-coded a posix one. - test-perf/trace-flush: node_modules/.bin/vitest is an extensionless shell script that spawnSync cannot execute, so the child produced no output and every assertion failed as an empty stdout rather than as anything about the tracer. Spawn node with vitest.mjs, as the other two spawn sites in that file already do. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> |
||
|
Zachary Lowden
|
6de64ee5a8 |
fix(tests): unzero the preview / component-tests tier, and make a zero-collected run say so (#4531)
* fix(tests): unzero the component tier — one mock factory was aborting the whole run `preview / component-tests` was reporting `failure` on `main` while executing ZERO tests. Reproduced deterministically (3/3 at |
||
|
Zachary Lowden
|
2ba7ecf186 |
refactor(apps): remove the orphaned 5-star AppBlockReview system (#4501)
* refactor(apps): remove the orphaned 5-star AppBlockReview system
Thumbs (AppListingReview) supersedes it. The 5-star write form had no
reachable entry point: its only two hosts were the /apps/[appBlockId]
detail route (retired, now redirects to the store detail) and
AppDetailsModal, which is opened only from AppBlockCard, which renders
only inside MarketplaceBody / RecentlyOpenedAppsView -- and MarketplaceBody
has had no importer in app code since /apps swapped to
AppListingsMarketplaceBody. The page's own header comment already recorded
this and flagged deciding the form's fate as the follow-up; this is it.
Removed:
- appBlockReview.service.ts and its two suites
- the blue-buzz appBlockReview reward + its registration and suite
- blocks.upsertReview / listReviews / getMyReview / setReviewExcluded
(setReviewExcluded had zero call sites at all) + their zod schemas
- AppBlockReviews.tsx and its browser suite
- the Bayesian machinery that was exclusive to it in
block-registry.service.ts: the AVG/COUNT/SUM correlated subqueries,
bayesianRatingSortKey, getGlobalMeanRating and the
app-rating:global-mean cache tag
- avgRating / reviewCount from AvailableBlock and PublicAppDetail, and
the card's rating chip
blocks.listAvailable no longer offers a "rating" sort and defaults to
"popular". The keyset cursor drops its pinned-mean third field; the decoder
still tolerates a stale 3-field cursor so an in-flight page resumes rather
than 500ing.
MARKETPLACE SORT: the live store is unaffected. /apps renders
AppListingsMarketplaceBody, which reads appListings.listAvailable and
defaults to sort "top-rated" -- a Bayesian shrinkage over
AppListingMetric.thumbsUpCount/thumbsDownCount in app-listing.service.ts,
with its own LISTING_BAYES_PRIOR and its own
app-listing:recommend-global-mean cache tag. It never touched
app_block_reviews. Only blocks.listAvailable (the retired grid) read the
star table.
DATABASE: the DROP TABLE migration is committed for history and is NOT
applied anywhere. This project does not run prisma migrate deploy; a human
applies it per environment, after this ships.
Kept deliberately: MarketplaceBody / AppBlockCard / AppDetailsModal (the
documented one-line rollback path for /apps, minus the star bits) and
listAppInsiderUserIds, which now has no production caller but carries a
documented displayed-vs-capability asymmetry that is under test.
* refactor(apps): address audit round 1 on the AppBlockReview removal
Four audit findings, none deploy-blocking.
F1 — the tolerant 3-field cursor decode was documented as a live back-compat
path ("an in-flight page resumes instead of 500ing"), and its guard did not
actually test that. Both halves are corrected:
- The comment now says what is true. A 3-field cursor was only ever minted by
the `rating` sort, and a client resuming that view sends `sort: 'rating'`
with it — a value `marketplaceSortSchema` no longer accepts, so zod rejects
at the router before the decoder runs. The tolerant split is defence in
depth over a door the sort removal already closed, not the thing keeping
that page alive.
- The guard is now real. It asserted `items.length` off a mocked return plus
a SQL-shape regex, but `capturedSql()` exposes only the assembled string
with `?` placeholders — the bound values are exactly what it cannot see, so
nothing could observe the decoded `cursorId`. Added `capturedValues()` and
asserted the parsed resume tuple directly.
Mutation-checked: rewriting the decoder to `decoded.slice(sep1 + 1)` (which
concatenates the dead mean onto the id and resumes at the wrong tuple)
SURVIVED the old guard and now dies on this guard's own assertion —
`expect(values).toContain('ab_5')`, 1 of 19 tests failing, the other 18
still green so the kill is attributable to this assertion rather than to
some other test's error.
F2 — the migration header undercounted the read paths and did not say that
applying it forecloses the revert. It named two; three queried the table at
base (listAvailable, getAppDetail and getFeaturedBlocks all projected
avg_rating off it). Header now states plainly that running the DROP makes the
PR-level revert a one-way door, and explicitly does not conflate that with the
store-grid rollback note, which stays safe.
F3 — regenerated the direct-mock allowlist with
scripts/test-perf/gen-mock-allowlist.mjs, dropping the entry for the test file
this PR deletes. The regeneration also sweeps in 43 entries that had accrued on
main since the file was last generated (all 43 exist at the base commit; the
canonical list is byte-identical, and totals are self-consistent again).
F4 — made the DROP's precondition executable instead of a comment asking a
human to run a COUNT. A DO block raises and aborts when the table is non-empty.
It carries its own to_regclass existence check so a re-run against an
already-dropped table stays a no-op, matching the IF EXISTS guards below it;
the COUNT is dynamic so it is never planned when the table is absent.
Verified against a throwaway PostgreSQL 17.10 cluster in all three states:
table absent (exit 0, no-op), table empty (exit 0, dropped), table with 3 rows
(exit 3, aborts with the row count, and the table, its rows and all three
indexes survive intact).
Full unit suite: 1519 files / 23963 tests passed, 0 failures.
typecheck: 0 errors. typecheck-tests-gate: 1030 errors/190 files at base ->
1010/188 here, unchanged by this commit.
* fix(db): make the app_block_reviews DROP guard hold under a plain psql -f
Self-review of the F4 guard added in the previous commit found it was only
effective under specific psql flags, which defeated its whole purpose.
A RAISE stops the rest of the FILE only when psql was invoked with
`-1`/`--single-transaction` or `-v ON_ERROR_STOP=1`. Under a plain
`psql -f migration.sql` — an entirely reasonable way to apply a
hand-applied migration — psql runs each statement in its own implicit
transaction and continues past the error: the guard raised, and the DROPs
then executed anyway.
Measured against a throwaway PostgreSQL 17.10 cluster before the fix: the
table and all 3 of its rows were destroyed and psql still exited 0. Silent
and total — the guard read as protection while providing none in that mode.
Wrapping the guard and the DROPs in one explicit BEGIN/COMMIT closes it. The
RAISE poisons the transaction, every later statement is rejected with
`current transaction is aborted`, and the COMMIT degrades to a ROLLBACK.
Re-verified across all 9 combinations of {plain `-f`, `-1`,
`-v ON_ERROR_STOP=1`} x {3 rows, empty, already dropped}: the table and its
rows survive every non-empty case, and the empty and absent cases drop and
no-op as intended. The header now also notes that without ON_ERROR_STOP psql
exits 0 even when the guard refused, so the exit code is not a usable success
signal and the operator should read the output.
* fix(db,test): stop the DROP guard hijacking the caller's transaction
Round 2's delta re-audit found that round 1's own fix introduced a worse
defect, plus one assertion that could not see the mutant it was written for.
1. The migration no longer issues BEGIN;/COMMIT;. Wrapping the guard and the
DROPs in an explicit transaction closed the plain-`psql -f` fail-open, but a
file that commits a transaction it did not open hijacks the caller's
transaction state, and that fails destructively in two shapes an operator
is likely to use:
- `BEGIN; \i this; \i next; COMMIT;` under ON_ERROR_STOP=1 (the header's
own recommended invocation, applied to more than one file). Our COMMIT
ends the operator's transaction early, so when the second file fails
the DROP is already committed and the table is gone anyway, together
with any unrelated row the operator had written in that transaction.
- `\set AUTOCOMMIT off` (the pgAdmin/DBeaver/Retool posture; Retool is a
documented apply path here). The operator's ROLLBACK; prints only
WARNING: there is no transaction in progress, exit 0.
It also silently defeated `psql -1`.
The two DROP INDEXes and the DROP TABLE now live inside the existing DO
block as EXECUTE statements. A lone DO block is one statement and therefore
atomic by itself, so the fail-open protection is retained without the file
taking any position on the caller's transaction state.
2. The legacy-cursor test pinned the bound values as a multiset, not a tuple.
toContain is order-blind, so a decoder that transposes its two return
fields still binds both required strings, merely swapped into the wrong
sides of (sort_key, ab.id) < (?, ?) -- a wrong page boundary, i.e. the
silent skip/duplicate the guard exists to prevent. That mutant survived all
19 tests. The assertion now pins the ordered tail of the bound array.
3. The header cited PostgreSQL 17.10. Production runs 18.3.
Verification, all on PostgreSQL 18.3 (server_version_num 180003, the
production server version):
- 15-cell invocation matrix over {plain -f, -1, -v ON_ERROR_STOP=1, a
failing-second-file batch, AUTOCOMMIT off + ROLLBACK} x {3 rows, empty,
already dropped}: rows survive every non-empty case, empty/absent drop and
no-op, and no case leaves the caller's transaction in a state it did not
choose.
- Negative control first: the guard-stripped file destroys all 3 rows under
-f, -1 and ON_ERROR_STOP=1, so the harness can observe destruction.
- The BEGIN;/COMMIT; draft reproduces the audited defect on 18.3: table
dropped-and-committed with the operator's pending row committed alongside
it, in both nested shapes.
- Transposition mutant: 1 failed / 18 passed, failing on the new tuple
assertion; the same mutant passes 19/19 against the previous toContain
form. The author's original mutant (dead mean concatenated onto the id)
still fails, on the same assertion.
- pnpm typecheck: 0 errors. typecheck-tests-gate (already red on main):
base 1030/190 vs HEAD 1010/188, the 68 unbaselined files identical.
* style(test): prettier-collapse the new cursor-tuple assertion
Formatting only, on the three lines added by the previous commit -- prettier
--check flagged them and CI runs that gate on changed files. Re-verified after
the reformat: 19/19 pass, and the transposition mutant still dies on this
assertion (1 failed / 18 passed, line 329).
* docs(db): correct four false or stale claims in the DROP migration header
Prose only. The SQL is byte-identical: stripped of comments and blanks, the
file matches
|
||
|
Justin Maier
|
c930757e2c |
feat(cosmetic-phash): widen the lane to 256 bits, and make the badge tunable (#4454)
* feat(cosmetic-phash): widen the lane to 256 bits, and make the badge tunable At 64 bits the near-match panel could not separate a copy from a coincidence. Measured over the 1,719 hashable cosmetics, the two badges reported as imitations of official artwork ranked 7th and 116th of their corpus against a 1st percentile of 18 — inside the noise. The orchestrator now offers `perceptualDct256`, and at 256 bits the same two rank 1st and 5th. Both were confirmed by eye, along with three pixel-identical re-uploads of official badges that the submission-time sha256 cannot see, because official cosmetics carry no `imageHash`. The sweep drains the corpus on `pHashVersion`, so bumping the lane is the backfill. `COSMETIC_SIMILARITY_CLOSE_RATIO` is unchanged at 0.125, and that is a finding rather than an omission: it selects the same five cross-creator pairs anywhere between 8 and 40 of 256, so the value sits on a plateau rather than an edge. Being a fraction of the width is what carried it across the upgrade. Three things that were silent before: - A lane bump that moved only two of the three fields left the third disagreeing, and nothing failed. Two of those three mutations passed the suite. `COSMETIC_PHASH_LANE` is now asserted to spell one lane in all three fields, which kills both. - The near-identical decision moved out of the component. It is now decided server-side and carried on the match, because the threshold is operator- tunable at runtime and a client recomputing it from a bundled constant disagrees with the server until every tab reloads. - The threshold reads from a `KeyValue` row so it can be moved without a deploy. Read per call rather than memoised — a TTL would delay the change by the length of the TTL, which is the whole point of the row. It degrades to the built-in default on a malformed or out-of-range value rather than throwing, because this gates a badge and a throw would take out the ranking underneath it. Out-of-range is rejected, not clamped: clamping 1.5 to 1 would badge every match as near-identical and read as a working threshold. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(cosmetic-phash): close the gaps the five review lanes found The knob shipped wired to nothing, and the tests said otherwise. Two mutations survived the previous commit's suite: replacing the new `close` expression with a constant, and dropping the KeyValue read in favour of the built-in default. Both passed 36/36. `close` had no assertion at either layer, and every `getSimilarCosmetics` test reset the KeyValue mock to undefined, so the whole suite only ever exercised the fallback path. One test now drives the ranking at the default ratio and again at an operator-set one, and both mutations die. `getCosmeticSimilarityCloseRatio` guarded the value it read but not the read. An unguarded rejection escapes `getSimilarCosmetics` AFTER the ranking is complete, and the card renders that as "this artwork was not compared against anything" — false, and the exact confusion the card exists to remove. The read is wrapped; the docblock now describes what the code does. Three of the five malformed-value cases were passing on residue. `loggingMock.logToAxiom` is reset once per FILE, so a later case was satisfied by the first case's warning. With a per-test clear and `toHaveBeenCalledTimes`, a mutant that returns the default silently kills five tests instead of two. Dropped a test that asserted a ratio it claimed was "what the UI thresholds on". It wasn't: both imitation pairs sit above the shipped 0.125, so it checked nothing the neighbouring test didn't already pin exactly, and it hardcoded /256 so the next lane bump would have divided by the wrong denominator in silence. Boundary corrected. `0` is now accepted — "only an exact match is near-identical" is a coherent request, and rejecting it fell back to the LOOSER built-in, i.e. more red badges than the operator asked for. `1` is rejected instead, being the same degenerate outcome `1.5` was already refused for. `normalizeCosmeticHashHex` now refuses a hash wider than the lane. `padStart` returns an over-wide hash unchanged, so it would be stored at full width under the current version — accepted as a comparison target, then excluded from every candidate set by the length filter, reporting "nothing was close" forever on a row that looks correctly hashed. Adds `scripts/oneoffs/drain-cosmetic-phash-lane.ts`. The mitigation the PR depends on — crossing the two-lane window in minutes rather than the sweep's ~2h15m — existed only as a scratch file. It loops the tested sweep rather than issuing its own UPDATE, so the rules about what a correct row looks like stay in one place. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(cosmetic-phash): make a refused hash say so, and stop the drain script hiding a timeout The previous commit added a silent failure while removing silent failures. `normalizeCosmeticHashHex` now throws on an over-wide hash, and the sweep's catch was a bare `} catch {` with no log — so a lane whose `hexLength` was left behind would hash every row (billed), throw at store, count them all as `failed`, stamp them, and retry once a day forever, with nothing anywhere distinguishing that from dead artwork. The catch now names the row and the reason. It logs fire-and-forget with its own `.catch`, because a logger that throws inside a catch converts a diagnosable failure into a lost one. The drain script asserted a cause it cannot know. `failed` counts rows the sweep could not store this run, and three different things land in it identically: dead CDN artwork, an orchestrator still working when the 30s wait elapsed, and a hash the store refused. The old closing line called all of them "permanently unhashable", which is a guess printed in the voice of a measurement — and the operator's next move differs for each. It now says what was counted and points at the per-row log. Dropped the drain script to the cron's 200/5 from 500/10. `getPerceptualHash` returns `undefined` for both a real failure and a workflow still running, and the sweep stamps either for 24h. More concurrency means more timeouts, so the script written to shorten the window a row spends outside the lane could have EXTENDED it to a day for a slice of the corpus — then printed "done", because a stamped row drops out of the predicate and the next batch comes back empty. The speed was always in removing the 15-minute gap, not in working a tick harder. `MAX_BATCHES` 40 -> 20 with the smaller batch, ~2x the corpus rather than 11x. Corrected the lane docblock, which still described the pre-guard symptom: a stale `hexLength` no longer reaches a comparison-time throw, it fails at the store and writes nothing. The new log path is asserted rather than assumed — deleting it turns the sweep suite red, with the unmutated control green either side. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * docs(cosmetic-phash): say what the drain script's failure count can and cannot tell you Comment and console output only; no logic. The closing line has now asserted a cause twice and been wrong both times, in opposite directions. First it called every failure "permanently unhashable". Then, correcting that, it claimed the cause "is not recorded per row" and sent the operator away — but two of the three paths to an undefined hash DO log, and so does the store-refused throw. That version would have had an operator ignore records that exist, including the one class whose correct action is neither "nothing" nor "re-run" but "fix the lane". The coverage is now spelt out rather than summarised, because summarising it is what went wrong twice: a relative media url and any network error or abort log as `perceptual-hash`; a refused width logs as `cosmetic-phash-sweep`; a workflow that simply did not succeed logs nothing, and that silent case is both the 30s timeout and dead artwork — the two most likely reasons a row is in the count. Also drops "re-run tomorrow" for "re-run once 24h have elapsed". The stamp is a 24-hour window, not a calendar day, so a next-morning re-run of an overnight drain finds every row still suppressed, breaks immediately on an empty batch and prints a clean zero — indistinguishable from a completed drain, which is the silent-failure shape this script keeps trying not to reproduce. And it no longer calls a repeat failure dead artwork. A width that disagrees with the lane fails identically on every run forever, an unset NEXT_PUBLIC_IMAGE_LOCATION fails for every row, and timeouts are correlated rather than independent draws — so "failed twice" does not imply "dead". Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
Zachary Lowden
|
c576281835 |
fix(test-perf): the module tracer never flushed under Vitest 4, so a traced run wrote nothing (#4441)
* fix(test-perf): the module tracer never flushed under Vitest 4, so a traced run wrote nothing
scripts/test-perf/trace-setup.ts flushed its counters only from process.on('exit') and a 15s
interval. Vitest's forks pool kills its workers rather than letting them exit, so the exit
handler never ran, and the README's own workflow - "trace one file at a time" - is a 5-10s run
that never reached the interval either.
The failure mode is the expensive kind: .test-perf/trace was never created at all, and
trace-report.mjs answered "no .test-perf/trace - run a traced suite first", which reads as
operator error rather than as a dead instrument. Since graph.mjs's model of what a worker loads
is validated against this tracer, the validation was unreproducible for as long as it was dead.
Flush from afterAll, which runs inside the worker before the pool can kill it. The exit handler
and the interval stay as backstops.
Verified on origin/main in a clean worktree: before the change the documented workflow creates no
trace directory; after it, trace-report.mjs prints a real report (1 worker snapshot, 45 distinct
modules).
The regression test spawns a real traced run and requires a snapshot naming the module the
fixture imported - asserting that this file contains the string "afterAll" would pass just as
happily with the hook registered somewhere it never fires. Both mutants die for their own
reason: reverting to the old exit-only flush fails on "expected 0 to be greater than 0", and a
flush that writes an empty snapshot fails on the module-name assertion. It runs in 1.7s.
TESTPERF_TRACE_DIR is new, so that test can redirect its snapshots instead of clobbering a trace
someone is reading.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01CuaBZXbMVSzwPkpEtHhvxh
* fix(test-perf): name the traced project apart, or its dep cache eats the unit suite's
Audit round 1 on this PR found the regression test was deploy-blocking, and reproducing it
independently was worse than the report: 16 of 53 files red, not 5 of 6.
Vitest keys a project's dep-optimizer cache on sha1(projectName) and Vite's config hash includes
the plugin names, so the traced project - which spread the unit project and kept its name while
adding the tracer plugin - resolved to the SAME node_modules/.vite/vitest/<hash>/deps_ssr as the
normal unit suite and hashed differently. Vite responds by deleting and re-bundling that
directory, while unrelated workers in the same shard are importing chunks out of it:
"Cannot find module '.../deps_ssr/prom-client.js'". assert-shard-ran.mjs would not have caught
it either - losing ~400 of ~5539 tests stays inside its band.
Naming the traced project unit-trace gives it its own cache dir. Same 53-file selection:
53 passed, 0 deps_ssr errors, two cache dirs on disk.
Also from the audit:
- trace-report.mjs now honours TESTPERF_TRACE_DIR, which trace-setup.ts already read. Exporting
it used to reproduce this PR's own bug: snapshots on disk, report says "run a traced suite
first".
- The trace dir is cleared at the start of each traced run. The report SUMS every snapshot it
finds and snapshots outlive their run, so a second run silently reported roughly doubled
numbers - a wrong number rather than a missing one.
- Snapshots are keyed by a per-worker id rather than bare pid. forks gives one process per file
so pid was unique there, but threads puts every worker in one process (measured: 3 files gave
3 snapshots on forks, 1 on threads), and per-file flushing would have several workers writing
one path concurrently.
- The flush no longer swallows its error. A silently unwritten snapshot is the exact failure
this PR exists to close.
- The child run's interval backstop is pushed to an hour, so afterAll is the only path that can
satisfy the test. Otherwise a child that lives past 15s passes it through the timer with or
without the fix.
- README: the traced invocation is --project unit-trace, and the paragraph claiming a multi-file
traced run keeps only the last file's counters is no longer true.
Fixing the clear-at-start introduced its own regression, caught by re-running the battery: it
removed the caller-supplied directory rather than its snapshots, turning a clean assertion into
an ENOENT crash. It now unlinks *.json only.
Six mutants, six deaths, each on its own assertion: no afterAll -> expected 0 to be greater than
0; empty snapshot -> expected [] to include the fixture module; project renamed back to unit ->
startup error, no "1 passed"; clearing disabled -> expected 2 to be 1; bare pid -> filename shape.
The bare-pid case is killed by a filename-shape PROXY and is labelled as one in the test - the
real property needs a threads-pool child with several workers, which these tests do not build.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01CuaBZXbMVSzwPkpEtHhvxh
* fix(test-perf): close the rename's own coverage hole, and stop the fix round's new sharp edges
Delta re-audit of the previous round. Its verdict was "safe to merge"; its findings were not
empty, and the findings are what decide.
The gap that mattered: every assertion named unit-trace, so renaming the project back to unit in
BOTH the config and the test passed green while fully restoring the cache collision that took 16
of 53 files red. The previous tip was literally that configuration and shipped green. There is
now a case that runs the traced config with --project unit and requires it to FAIL with "No
projects matched", so the two spellings can no longer be reverted together in silence.
The bare-pid guard was a filename-shape proxy, and the audit was right that the real property is
cheap to test: two more three-line fixtures and one --pool=threads child. It now asserts the DATA
survived - all three fixture modules present in the merged snapshot - rather than the filename.
Reverting to a bare pid loses two workers' counters to a clean last-wins overwrite and the case
goes red.
That mutant also exposed a coupling the audit did not see: the writer's filename format and the
clear's delete predicate were two copies of one rule, and with a bare pid the clear silently
stopped matching, so a second run summed on top of the first. Both now come from
trace-snapshot-name.ts.
Other audit findings:
- trace-report.mjs created no .test-perf/ before writing trace.json, so with TESTPERF_TRACE_DIR
set on a fresh clone the documented workflow crashed with a raw ENOENT after doing all the
merge work. mkdirSync first.
- TESTPERF_TRACE_INTERVAL_MS ran through bare Number(), so abc/empty/-5 all became a 1ms
synchronous whole-snapshot write loop inside every worker - measured 45 fires in 50ms - which
charges its own cost to the measurement. Validated with the same idiom vitest.config.mts uses.
- The snapshot clear deleted every top-level *.json in a caller-supplied directory. Scoped to
this tool's own filename shape, and wrapped: a throw there happens at config load and would
abort the whole run with a raw stack.
- trace-config.mts advertised a bench.mjs invocation that its own rename breaks (bench hardcodes
--project unit). Docstring corrected rather than widening bench's filter, which would drag
unit-native into every yardstick run.
- Two comments corrected against measurement: the flush fires once per test FILE, not per suite,
and the thread-pool failure is a silent last-wins overwrite, not a truncated read.
- README now discloses what the rename costs - a second 87MB dep bundle - and that under --watch
only the first re-run clears.
Seven mutants, six dead on their own assertion: no afterAll -> expected 0 to be greater than 0;
empty snapshot -> expected [] to include; clearing disabled and bare pid -> expected 2 to be 1;
full rename -> expected +0 not to be 0. M7, reverting the interval validation, SURVIVES: no test
supplies garbage to that knob, so it is an unpinned defensive guard and is called one here rather
than counted as covered.
🔴 One of the previous round's mutants did not actually apply - backtick escaping - and its
"survived" reading was a fact about the harness, not the code. Every mutation in this round
asserts its own target was found before the run.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01CuaBZXbMVSzwPkpEtHhvxh
* fix(test-perf): pin the two behaviours the last round asserted but never checked
Round 3 of the delta audit. Verdict was "ship"; it still found two guards that no test could see,
and both are now killed by a mutant.
- The clear's PROTECTIVE half was unpinned. Widening SNAPSHOT_FILE_RE back to /\.json$/ left every
test green while destroying a caller's other JSON - exactly what the README promises it will not
do, and the whole reason trace-snapshot-name.ts exists. Only the drift direction was covered.
Test 1 now writes important-notes.json into the trace dir and requires it to survive.
- The interval validation was called an unpinnable defensive guard. That was giving up early: the
audit pointed out this PR had just established the pattern for pinning it. resolveIntervalMs is
now a pure helper in the shared module with a nine-case table, no child process, and reverting
it turns seven cases red.
Fixing that found a real hole in my own guard: Number.parseInt reads '1e10' as 1, which is the
1ms flush loop the check exists to prevent, and a case bare Number() handled correctly. It now
uses Number with an n >= 1 bound.
Also from the audit:
- The clear caught its error around the whole loop, so one undeletable snapshot abandoned every
other stale file - and the survivors are summed into this run's numbers. Now per entry.
- The docstring said a typo'd --project unit "fails loudly". True, but not harmlessly: the clear
runs at config load, before the project filter, so that invocation empties the trace directory
first. Said so.
- The suffix comment claimed once per worker; measured once per test FILE under isolation.
- Fixture c's stated rationale was wrong - two files already discriminate, because the tracer
re-initialises per file rather than per worker, so a scheduler batching them cannot hide an
overwrite. Deleted it and the threads case runs on two.
Nine mutants, nine deaths, each on its own assertion: no afterAll / bare pid / writer filename
drift -> expected 0 to be greater than 0; empty snapshot -> expected [] to include; clearing
disabled -> expected 2 to be 1; full rename -> expected +0; interval validation reverted ->
expected NaN to be 15000; regex widened -> expected false to be true. Every mutation asserts its
target matched exactly once before the run.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01CuaBZXbMVSzwPkpEtHhvxh
* fix(test-perf): the interval guard had an upper bound problem, and the test asserted the broken value
Round 4 of the delta audit, and it caught my round-3 fix landing on the same pathology from the
other side.
resolveIntervalMs bounded the input below but not above. setInterval stores its delay in a 32-bit
signed int, so anything over TIMEOUT_MAX (2147483647) is clamped to 1ms - measured on Node
24.19.0, 1e10 fired 281 times in a 300ms window with "TimeoutOverflowWarning: Timeout duration was
set to 1", and it reproduces end to end in a real traced run. That is the ~900 writes/second
inside every worker that the function's own docstring exists to prevent, reachable from the "make
it huge so it never fires" direction. Worse, the it.each table I added asserted 1e10 -> 1e10 as
the correct answer, so the defect was enshrined as deliberate. Bounded at both ends now, with
cases at MIN-1, MIN, MAX, MAX+1 and 1e10.
Four behaviours the audit showed were unpinned or unmodelled:
- The per-entry catch in the clear was real and load-bearing - 0 survivors vs 5 - and no test
could tell it from the pre-fix shape. The clear is now an exported clearStaleSnapshots with a
synthetic-directory test including an EISDIR entry; reverting it to a rethrow goes red.
- mergeSnapshots in the test had stopped modelling trace-report.mjs: it filtered by the snapshot
regex while the real reader took any *.json. Running the real reader against the directory this
PR's own test creates gave "20 distinct modules | NaN executions | NaNs self time". The reader
now accepts snapshots by SHAPE rather than extension - it is a .mjs file and the shared
constants are TS, so a value check is what cannot drift - and a test spawns it against a foreign
file and requires no NaN.
- Dropping the regex's $ anchor survived green: it still matches 1234-abcd.json.bak and .json.swp,
i.e. editor and backup files next to a caller's data. The survives-set now includes one.
- The summing merge survived green too, because with clearing on there is only ever one snapshot.
It is now pinned where two genuinely coexist - the threads case - against the sum of a module
common to both files.
Also: TESTPERF_TRACE_DIR used ?? in all three readers, so an exported-but-empty value kept '',
mkdirSync('') threw, no snapshot was written and the report said "run a traced suite first" - the
dead-instrument failure this whole change exists to remove. Now || with a trim, single-sourced.
And trace-setup.ts's header rationale claimed globalThis makes counters survive across files;
measured, each file writes a disjoint snapshot on both pools and the totals are correct because
the report SUMS. The stale per-worker framing is gone from the report's label too - that count is
test files.
Thirteen mutants, thirteen deaths, each on its own assertion. Every mutation asserts its target
matched exactly once before the run.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01CuaBZXbMVSzwPkpEtHhvxh
* fix(test-perf): guard the tool, not the test's mirror of it — closing three named gaps
Round 5 of the delta audit named three fixes and a mechanical closing condition: re-run five
specific mutants rather than open a sixth round. All three are here and all five now die.
The pattern the audit named, which is what actually cost the last two rounds: the tests were
guarding their own mirror of the tool rather than the tool.
- The interval table derived every expectation from the constant it was testing, so it could no
longer see a WRONG CONSTANT. DEFAULT -> 1 and MAX -> 2**31 both survived a green suite, and both
are the 1ms flush loop the function exists to prevent - reachable on the default path a
developer hits by not setting the variable at all. That is round 3's defect one notch out,
introduced by round 3's own fix. Two literal rows now sit beside the derived ones.
- The shipped reader's sum was unpinned. mergeSnapshots in the test is a reimplementation, and it
was the only thing any mutant could see; the single test that ran the real reader had a
one-snapshot fixture, where summing and overwriting are identical. The fixture now holds two
snapshots naming one module, asserted against the totals the tool itself prints.
- The .mjs reader cannot import the shared TS resolver, so it carries its own copy of the
empty-string rule - the same drift the shared module exists to stop, one layer out where the
module cannot reach. Reverting it to ?? was green while reintroducing half the dead-instrument
failure this PR removes. A second spawn with TESTPERF_TRACE_DIR='' closes it.
Free corrections from the same round: trace-config.mts still documented one snapshot "per worker"
while trace-snapshot-name.ts three files away says that is not true; a test title and the README
carried the same stale framing; fixture b's comment counted three fixtures when there are two.
Closing condition, each mutation asserted applied and its mutant text confirmed on disk before the
run: DEFAULT->1 dies on `expected 1 to be 15000`; MAX->2**31 on `expected 2147483648 to be 15000`;
reader loads += -> = and selfMs += -> = both on the printed totals; .mjs ?? on the fallback path.
28 tests, 4.6s.
Not taken, deliberately: the report still exits with a raw ENOTDIR stack when TESTPERF_TRACE_DIR
names a regular file, and isSnapshot's guards are wider than the one test covering them. Both were
called optional, neither is a live defect, and adding more scaffolding to a gitignored dev tool is
the thing this ladder has been overdoing.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01CuaBZXbMVSzwPkpEtHhvxh
---------
Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
||
|
Zachary Lowden
|
afe84cf7c8 |
ci: shard the unit suite across 4 runners, 10.4m -> 3.8m (#4392)
Measured from the Actions API (n=12): the job was 555.5s of test work plus 66.5s of fixed overhead = 622s, the 10.4m median, and 3.4x the next-slowest job. T(N) = 66.5 + 555.5/N puts N=4 at ~3.4m. Measured after: 10.3m -> 3.8m on the critical path (2.7x) at +36% runner-minutes, reproduced across two runs with a 1.12-1.14x shard spread. N=4 rather than more because `App unit tests + typecheck` is 2.9m — past N=5 sharding optimises something that is no longer the bottleneck. Ships with a positive control, `scripts/ci/assert-shard-ran.mjs`. Sharding adds a failure this job did not have: a `--shard=i/N` selecting no files exits 0 and reports green, and four green checks look identical whether they ran 22,000 tests or none. The guard asserts per shard that work happened, counting EXECUTED assertions rather than `numTotalTests` (which includes skipped, so an all-self-skipping shard would pass a total-based floor), and its bounds scale with `total` so a change to the matrix does not trip them. It earned its keep immediately: on the first run it failed all four shards while `Unit tests` reported SUCCESS. `pnpm run … -- --shard=…` forwards the `--` literally, vitest DISCARDS everything after it, and so `--shard` and `--outputFile` were both silently dropped — each runner executed the entire suite (7m30s-9m54s per step) and wrote no report. Without the control this would have merged as four green checks that tested nothing verifiable. A subsequent adversarial audit found three further claims that measurement did not support, all fixed here: bounds hardcoded off a ten-day-stale suite size that would have tripped within weeks and blamed the wrong cause; a comment asserting post-`--` args become filename filters when they are discarded; and a "mutation-verified" claim that five of twelve mutants walked through. Four are now covered by named cases; the fifth was unreachable dead code and is removed rather than papered over with a test that could not kill it. Final sweep, re-run after the Prettier pass: 10 mutants, 10 killed, each by its own named test, with a no-op negative control that correctly survived. Suites 20/20. Verified live across two runs; the typecheck-scripts gate was confirmed to have executed rather than assumed green. Not verified: behaviour on a push to `main`, where `continue-on-error` is false and a red shard renders as a genuine failure. That path only exercises on merge. |
||
|
Zachary Lowden
|
b34d90fd1c |
fix(apps): make the listing-completeness advisory KIND-AWARE (on-site copy lives in block.manifest.json) (#4370)
* fix(apps): make the listing-completeness advisory KIND-AWARE
`computeListingProblems` was kind-blind. It emitted `empty-description`,
`empty-tagline` and `empty-category` with the label "Missing <field>" for
EVERY listing. On an OFF-SITE listing that is correct — the author typed that
copy into the submit wizard and can go and fix it. On an ON-SITE listing it is
wrong, not merely terse: those scalars have NO author surface other than
`block.manifest.json`, and `approveRequest`'s (3b-sync) MANIFEST-GOVERNED COPY
RE-SYNC (`publish-request.service.ts`, scoped `kind: 'onsite'`) re-derives them
from the manifest on every subsequent-version approve. An on-site author who
found some other way to set a tagline would have it reverted at the next
approve — so `/apps/mine` was telling them to do something that cannot work.
KEEP-WITH-CORRECTED-LABELS, not suppress. The gap is real (the store page
genuinely has no tagline), so hiding it would trade wrong advice for no advice;
and suppression would silently kill the on-site branch of a released CLI that
consumes these codes. The codes and severities are therefore KIND-INVARIANT and
only the three labels move.
`kind` is a REQUIRED input, which is what makes "a caller was missed" a compile
error rather than a whole surface keeping the old advice. All three callers are
threaded, each PROJECTING the column rather than restating what its own filter
implies:
- appListings.listMine (app-access.service) — both kinds, one page
- appListings.listMySubmissions (offsite-listing.service) — on-site media
revisions appear here too, via its `{ kind: 'onsite' }` OR-branch
- blocks.listMyPublishRequests (blocks.router) — projects `kind` even
though its `where` filters on it
The kind lookup is an explicit equality branch, not a `TABLE[kind] ?? default`:
`kind` is an untrusted cast off the `app_listings.kind` column, and indexing an
object literal with an inherited key (`'constructor'`) returns something truthy,
which `??` accepts and which then yields a problem with NO label. An
unrecognised kind degrades to the original labels and never throws.
No component change: `ListingProblemsIndicator` renders whatever `label` it is
given.
Tests: 12 regression cases RED at origin/main
|
||
|
Justin Maier
|
06e2651d47 |
fix(dev-server): stop the daemon popping a terminal window that steals focus (#4373)
`startDaemon` spawned the daemon with `shell: true`. Node applies `windowsHide` only to the process it creates, so the flag landed on cmd.exe and never on the node cmd.exe then started. That node got a fresh console, Windows 11 handed it to the default terminal app, and a Windows Terminal window opened and took focus off whatever the user was doing. Measured: the window appeared on every run of the unpatched code and stole focus on four of six. Dropping the shell fixes that, but it also leaves the daemon with no console of its own — so every child it spawns is then in the same position and pops its own window, once per dev server started, per taskkill, per queued vitest run, per branch-watch `git`. Verified that too, so `windowsHide` goes on all of them. Guarded by scripts/__tests__/dev-server-no-console-window.test.ts, which fails if either half regresses (checked by mutating both back). Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
Justin Maier
|
0d1142ce4c |
fix(skills): a flag with no value is an error, not a silent true (#4263)
* fix(skills): a flag with no value is an error, not a silent true
metabase and cloudflare both degraded a value-taking flag whose value was
missing, empty, or --prefixed into a truthy placeholder: boolean true in one,
the string 'true' in the other. Every downstream guard tests truthiness, so
create-question posted native: { query: true } and Metabase stored a card with
no query on it — created successfully, opens blank, indistinguishable from a
permissions problem.
Both parsers now reject a missing value and accept --key=value for a value that
legitimately starts with --. create-question reads the card back and fails if
the stored SQL is not what was sent.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* docs(skills): document the flag-value rule in both SKILL.md files
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* fix(skills): keep --json parsing, close the --key= hole, and separate a failed read-back from a failed create
Review of #4263 found three things. --json is advertised in metabase's own usage
text, so rejecting it was a regression introduced by the fix rather than by the
bug. The --key=value form — the one the error message sends people to — accepted
an empty value the space-separated form rejects. And a cloudflare boolean flag
no longer consumed an explicitly spelled , which shifted positional[1].
The read-back now distinguishes 'created but could not be read back' from
'created with the wrong SQL': a GET that fails must not be reported as a create
that failed, since the card exists either way.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
---------
Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
||
|
Zachary Lowden
|
bdb55c9ef2 |
feat(observability): expose Flipt eval-cache stats — a hit rate alone cannot pick the right knob (#4202)
* feat(observability): expose Flipt eval-cache stats — a hit rate alone cannot pick the right knob
The per-eval TTL cache in `@civitai/flipt` tracked `hit` internally and threw the
result away, so from outside the process it was a black box. It has two tuning
knobs — `FLIPT_EVAL_CACHE_TTL_MS` and the `evalCacheMaxEntries` ceiling — and no
signal to choose between them.
That matters because the two failure modes are indistinguishable from a hit rate
and have OPPOSITE remedies:
* misses dominated by expired entries -> TTL-bound. A longer TTL converts them
into hits.
* generation rotations climbing -> capacity-bound. Entries are evicted before
they can expire, so a longer TTL recovers NOTHING; it is an inert change that
reads as a fix. The ceiling is the knob.
The cache key is (flag, entityId, context), so a per-user entityId multiplies the
key space by the active-user count — capacity-bound is the likelier of the two on
hot paths, which is exactly the case a bare hit rate would have hidden.
Adds cumulative counters to TtlCache (hits, misses, expiredMisses, rotations,
size), surfaces them via `getCacheStats()`, and exports them from the monolith as
`civitai_app_flipt_eval_cache_*` on the same default registry /api/metrics
scrapes. Registered by side-effect import there, matching its neighbours, so the
series exist from the first scrape — an absent series would read as "the cache is
idle" rather than "nobody loaded the module".
The package gains no new dependency: the counters are plain numbers and all
prom-client wiring lives app-side.
Verified rather than assumed. Every new guard was mutation-tested and each mutant
died by its OWN named test, with the control restored byte-identical:
* expiredMisses bumped on every miss -> killed by the cold-vs-expired test
* rotation counted on every set -> killed by the overflow test
* promoted read not counted as a hit -> killed by the promotion test
* reset() dropped from collect() -> killed by the double-count test
* boolean stats reported for both labels -> killed by the per-kind test
Typecheck: 317 errors at origin/main and 317 on this branch, with byte-identical
error sets (zero branch-only errors) — the baseline is a stale generated client,
not this change. eslint clean on the touched files and on packages/civitai-flipt,
verified with a negative control that the linter actually processed them.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* fix(flipt): pin the client on globalThis — the metric was measuring ONE of TWO caches per pod
Audit found the observability this PR adds was itself half-blind, in the same
shape the immediately-preceding merged PR (#4173) reverted the pg-pool gauges for.
MEASURED, not theorised: `src/server/flipt/client.ts` is emitted TWICE in the
production server build. `[flipt] eval cache TTL:` appears exactly 2x on every
pod — 488 lines across 244 pod streams — against one `[instrumentation] Running
in nodejs runtime`. Each emitted copy owned a private wasm client, a private 60s
config poller and a private pair of eval caches, and `getFliptCacheStats` closed
over whichever copy its chunk resolved.
That is worse than a scale error, because the deliverable is a KNOB DECISION and
the bias has a direction: rotations are superlinear in per-instance key space, so
one key space split across two caches at the same ceiling rotates far less than
one cache holding all of it. The split reads as "TTL-bound" and sends the reader
to the knob that changes nothing — the exact inert change these metrics exist to
prevent.
Pins the client on `globalThis.__civitaiFliptClient` using the repo's canonical
idiom and enrols the module as SHARED_STATE in server-graph-watchlist.mjs, so a
refactor that drops the pin fails that gate. Side benefit: one wasm engine and
one config poll per pod instead of two.
Also fixes three defects the audit found in this PR's own tests:
* The seam was untested. Deleting the side-effect import in
src/pages/api/metrics.ts left the whole src/server/metrics/ suite green
(131/131) — registered-but-unreached, this repo's #1 metric-death mode. Now
asserted by loading the module that SERVES the scrape, matching the
substitutions and bitdex-feed-serve seam tests.
* One asserted cell was vacuous: read() ended `?? 0`, so asserting the variant
cache's expiredMisses is 0 passed whether or not the series existed —
the absent-vs-zero ambiguity this metric exists to remove, reintroduced in
the test guarding it. Returns NaN on a missing label now.
* prettier failed on the new file, making CI red. The PR body claimed "lint
clean on the touched files"; that was true of eslint and false of prettier —
the negative control was run on the wrong instrument.
Mutation-verified, each killed by its own named test, sources restored
byte-identical with controls green:
* delete the side-effect import -> seam test (previously SURVIVED)
* gate the variant inc on non-zero -> per-kind test (previously SURVIVED)
* `??=` downgraded to `=` -> watchlist gate
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
---------
Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
||
|
Zachary Lowden
|
5770b83f4e |
ci: give a commit on main a CI verdict again, and pin that it keeps one (#4234)
* ci: give a commit on `main` a CI verdict again, and pin that it keeps one Nothing has produced a CI verdict for a commit on `main` since 2026-06-14. The cause was not a decision: `.github/workflows/pr-check.yml` was the only workflow this repo has ever had with `push: branches: [main]`, and #2547 deleted it while moving PR checks to Tekton. Every workflow written since — lint (#3362), submodule-pin-guard, schema-drift (#3643), windows-dev-env (#4162) — is `pull_request`-only, so the main-push trigger was never rebuilt and nothing said it was gone. Tekton does not close the gap. Its `pr-check` pipeline is driven by a resource that watches open pull requests targeting `main`; the only main-branch trigger builds a container image for the staging deployment, runs no test suite and posts no commit status. "Tekton covers main" is the assumption that let this persist for two months. The old June runs stayed `success`, so "is main green?" went on answering yes. That is the expensive half: the signal was not wrong, it was stale, and staleness is invisible unless you read the dates. |
||
|
Zachary Lowden
|
314663b1b1 |
fix(dev-server): give a queued test run a FILE, not a pipe — a child that exits without flushing was losing most of its log (#4190)
* fix(dev-server): give a queued test run a FILE, not a pipe — a child that exits without flushing was losing most of its log
Closes two ClickUp tickets that share one capture path but are different
defects, and the head-to-head measurement is what picked the fix.
868ktwgyc — output lost before the daemon ever received it. Node makes a child's
stdout SYNCHRONOUS when it refers to a regular file and ASYNCHRONOUS when it
refers to a pipe, so a child calling `process.exit()` discards whatever is still
queued on a pipe. Measured head-to-head, same child, same process, old
mechanism vs new:
consumer=slow PIPES received= 172 missing=4828 | FILE received=5000 missing=0
consumer=slow PIPES received= 172 missing=4828 | FILE received=5000 missing=0
consumer=slow PIPES received= 172 missing=4828 | FILE received=5000 missing=0
consumer=fast PIPES received=1473 missing=3527 | FILE received=5000 missing=0
consumer=fast PIPES received=5000 missing= 0 | FILE received=5000 missing=0
consumer=fast PIPES received=5000 missing= 0 | FILE received=5000 missing=0
Two separate runs would not have been a comparison; this is the same child
through both arms. Numbering the lines is what makes it readable — a bare count
cannot tell truncation from sampling, and every missing line here is a
contiguous tail, which is what identifies the mechanism.
That measurement also ruled out the two fixes the ticket proposed. The daemon
CANNOT detect the loss by reading: it sees a clean EOF with no signal to compare
against, because the bytes never reached the pipe. And "flush before exit" is
not ours to call — the child is vitest. A file needs no cooperation from the
thing losing the data.
868kubfd9 — the reader tore lines at chunk boundaries. `d.toString().split('\n')`
per chunk with no carry buffer split any line straddling a boundary in two and
recorded BOTH halves as lines: 5,000 in, 4,998 recognised, 6 fragments invented,
353,893 of 353,893 bytes. Complete delivery, corrupted log, and `logsDropped`
correctly reports 0 for it — which is what made it invisible behind a counter
readers had been trained to trust. The new reader carries the partial line.
Third thing, not in either ticket and the one that protects a reader most: the
final drain now runs BEFORE onExit. A waiter wakes on the terminal status and
then reads the log, so reporting the exit while lines are still unread produces
a complete-LOOKING log that is still filling — the same false green, reached a
different way. The old pipe path could do that, since 'exit' does not wait for
pending 'data'.
Trade-off, stated rather than buried: one file for both streams, so the
interleaving is the real one, at the cost of no longer distinguishing stdout
from stderr. Lines are recorded as `output` rather than claiming to be one or
the other — nothing renders the level for a test run (both consumers print
`entry.message`), and a label we cannot support is worse than an honest one.
Seven mutants, each alone, tree cmp-verified between:
carry buffer removed -> the straddle case (+2 more)
final flush removed -> the trailing-line case
back to pipes -> both seam cases
onExit before the drain -> the ordering case
drain reads a single chunk -> the 5,000-line case + straddle
stderr to another fd -> the shared-descriptor case
capture file not deleted -> the cleanup case
All killed. The ordering mutant is killed ONLY by the ordering test, which is
why it exists.
344 passed (15 files). typecheck 0 errors. New test file prettier-clean.
* fix(dev-server): the capture had no owner — a forced or shut-down run leaked two fds, an unbounded /tmp file and a live interval
Audit round on #4190. Both blockers share one root cause: `finish` was bound only
to the child's own 'exit', and there are two live paths where that event never
comes.
🔴 The sweep's force-release — the wedge this queue exists to prevent — set
`run.handle = null` and dropped the queue's last reference while the interval,
both descriptors and the capture file stayed alive inside the closure. Measured
on a forced run: 2 fds still open and the log still growing 2s after the run was
reported terminal (logIndex 75 -> 471), file at 102,465 bytes, never unlinked.
A reader fetching logs after the verdict would get lines emitted AFTER it, with
`logsDropped` reading 0. `shutdown()` had the same gap.
🔴 And it was already leaking on every CI run, from a test this PR did not
update: `dev-server-test-queue-spawn.test.ts` calls `defaultStartRun` twice and
never emits 'exit'. Not hypothetical — 15 stale `civitai-test-run-*.log` files
were sitting in this box's /tmp from my own runs, 420 KB. Verified fixed by
counting: 0 before, 0 after, where it was 2 per run.
Both closed by `dispose()` on the handle, called from the force-release and from
`shutdown()`. Idempotent, and deliberately does NOT call onExit — the caller has
already settled the run.
Also from the audit:
- A carry buffer fixes a torn LINE and does nothing for a torn CHARACTER.
Decoding each read independently turned a 3-byte `⎯` starting at byte 65535
into THREE U+FFFD with the original gone — and vitest builds its failure output
from `⎯`/`✓`/`×`/`❯`, one boundary per 64 KiB. `StringDecoder` now spans reads.
The existing straddle test is ASCII and structurally could not see this.
- The final drain no longer stops at the first short read. Truncating there would
report `logsDropped: 0` over a clipped log — the exact thing
`warnIfLogsDropped` exists to make impossible.
- The straddle test's positive control is pinned from both sides; `> WINDOW`
alone is also satisfied by padding that already exceeded it, which would put
the line wholly inside the second read and never exercise the hazard.
- The `output` level is now pinned, the read buffer is hoisted out of a 10x/s
allocation, and the spawn-failure paths return a handle with the same shape.
Seven mutants, each alone, tree cmp-verified between:
force-release stops disposing -> the queue seam case
shutdown stops disposing -> the queue seam case
per-read decode -> the multi-byte case
lines relabelled 'stdout' -> the level case
dispose also calls onExit -> the dispose-contract case
dispose not idempotent -> the dispose-contract case
final drain stops on short read-> SURVIVED, and it is honest that it does
That survivor is stated rather than papered over: on a regular file a short read
only happens at EOF, so no test on this filesystem can distinguish the two. It
is contract hardening, not covered behaviour.
🔴 The seam lesson again: the first version of the dispose test called
`handle.dispose()` directly, which proves the method EXISTS and not that the
queue ever calls it — a `dispose()` nobody invokes is exactly the shape this
missed the first time. The killing test now drives TestQueue and asserts on
both release paths.
MEASURED, not fixed here: a fully PASSING unit run emits 5,390 lines against
`MAX_LOG_LINES = 2000`, so now that the capture actually delivers everything the
"INCOMPLETE" warning will fire on every full-suite run — and a warning that
always fires is one people stop reading. Raising the cap has memory arithmetic
attached (`KEEP_FINISHED = 50` retained runs x the window), so it is a tuning
decision that does not belong inside a correctness fix.
349 passed. typecheck 0 errors. 0 leaked captures.
* fix(dev-server): shutdown disposed the handle and settled nothing — the fix for the leak wedged the queue
Delta re-audit round on #4190. The previous round's `dispose()` closed both
leaks and introduced one genuine regression, which is fixed here.
🔴-in-effect: `dispose()` and `finish()` share the `finished` flag, so disposing
DISABLES the child's exit callback. The sweep's force-release settles the run
itself right after disposing; `shutdown()` did not. The SIGKILLed child's 'exit'
then arrived and returned early, `onExit` never fired, the run stayed `running`
and `running.size` never dropped — so `pump()` could never start another run.
That is the wedge this queue exists to prevent, reintroduced by the fix for a
leak. Measured in both arms: 300 ms after shutdown(), `running` / size 1 at
HEAD, against `cancelled` / 0 with the dispose removed. `shutdown()` now
settles what it disposes.
Survivable today only because every caller exits the process straight after —
but each first awaits rgbProxy.stop(), authHub.stop() and stopSpokeApps(), and
if any of those hangs the daemon stays up serving a queue that can never run
anything again.
🔴 And the comment I wrote to justify that dispose was FALSE. It claimed the
capture file would otherwise "survive the daemon that created it, every time".
Measured in both arms: the file was unlinked either way, because the daemon does
linger long enough for the child's exit. The dispose earns its place by
releasing the interval and the descriptors deterministically — not by that. The
comment now says so, including that it was wrong.
Also from the re-audit, all on lines the previous round added:
- The multi-byte test was VACUOUS without a straddle — it passed with the
StringDecoder removed entirely, because "no U+FFFD" is equally satisfied by a
character that never crossed a boundary. Now pinned from both sides, the same
control its sibling got last round.
- `decoder.end()` had no test. A child killed mid-sequence leaves an incomplete
character; without the flush those bytes vanish silently instead of surfacing
as U+FFFD.
- `dispose()` draining before releasing had no test, on the one path where the
child is KNOWN to still be writing — `drain(true)` after `close()` emits
nothing at all.
- A read that THROWS on the final drain was silent. Same argument as the
short-read break beside it: it would report a clipped log with
`logsDropped: 0`. It now emits `[capture truncated: <code>]`.
- The `!final` comment overstated its scope: on the `finish()` path the writer
is already dead, so it changes nothing there. It has effect only on `dispose()`.
Battery, each mutant alone, tree cmp-verified:
shutdown disposes but never settles -> the new queue-level case
dispose closes before draining -> the drain-before-release case
decoder.end() removed -> the incomplete-character case
final-drain read error silent -> the truncation-marker case
dispose leaves the interval running -> SURVIVED
That survivor is stated, not papered over: once the descriptors are closed the
stray interval's read just throws and breaks, so it has no observable effect on
the log. The ordering is still correct and still load-bearing — fd numbers are
reused, so a surviving reader could splice another file's bytes into this run —
but nothing pins it without exposing internals.
🔴 The harness lied to me first. Four mutants reported SURVIVED with an EMPTY
test count, because the runner list was an unquoted `$T` and zsh does not
word-split — vitest matched zero files and printed project globs. That is the
exact trap I wrote into the auditors' own briefing. The battery now refuses to
report a verdict when no `Tests` line is present, and the baseline control
proves one appears.
353 passed. typecheck 0 errors. 0 leaked captures.
* fix(dev-server): freeing the SLOT must not sit downstream of IO that can throw — and I was wrong that the stray interval was harmless
Round-3 delta re-audit. It refuted two of my own claims with measurements; both
are corrected here rather than argued with.
🔴-in-effect: `dispose()` does real IO and calls back into `onLog`, and it sat
ABOVE the detach/release/settle that free the slot — in both `shutdown()` and
the sweep's force-release. A throw there skips all three and the queue is wedged:
the identical failure the settle was added to fix, one round earlier. Measured
with a throwing dispose: status `running`, running.size 1, against `cancelled`
/ 0 when it returns normally. It also aborts the loop, leaving every remaining
run unkilled, and because both daemon handlers are async the rejection means
`process.exit(0)` is never reached — SIGINT would appear to do nothing.
Worse, this round ADDED a throw source inside that block: `err.code` raises on a
null/undefined throw. The repo already documents this hazard one file over
(`scripts/test-unit-run.mjs` uses `err?.message ?? String(err)` with a comment
saying why). Now `err?.code ?? err?.message ?? String(err)`, and both dispose
call sites are guarded — releasing the slot matters more than releasing the fds.
🔴 RETRACTION. I reported the `clearInterval` mutant as surviving with "no
observable effect once the descriptors are closed". That is measurably false,
and my own test comment said so while I argued the opposite. Descriptor NUMBERS
are reused: once the next `openSync` claims the freed fd, a surviving interval
reads whatever file now owns it. Measured: 4,246 lines of an unrelated file
spliced into a terminal run's log, against 0 with the clearInterval in place.
It is now pinned rather than argued away.
That test needed a second try to be able to fail at all. `openSync` returns the
LOWEST free descriptor, and the tail reads from `readFd` — the HIGHER of the
pair the capture just closed. Claiming one descriptor takes `writeFd`'s number
and the mutant goes unobserved, which is exactly how the first version passed.
It now claims both.
🔴 SECOND RETRACTION. Last round's message said the multi-byte test was
"vacuous without a straddle" and passed with the StringDecoder removed. The
re-audit re-ran that mutant against the round-2 file: KILLED. The padding was
already `READ_WINDOW - 1`, so the straddle always existed — the "survived" came
from my own broken harness, the one that reported four false SURVIVED verdicts
with an empty test count. The added assertions were still worth having, but the
justification I gave for them was wrong.
The residual behind it was real and is fixed: the test held a private copy of
`64 * 1024`. Widen the module's buffer and both boundary controls go vacuous
while staying green — proven: at 128 KiB the whole suite passed with the
StringDecoder deleted outright. The window is now exported and imported, so the
control cannot drift from what it controls.
Also: the test named 'drains before releasing, and stops the tail before closing
the descriptors' asserted only the first half. Renamed to what it checks, and
the second half is now its own test.
Battery, each mutant alone, tree cmp-verified, every verdict gated on a `Tests`
line being present:
dispose leaves the tail running -> the fd-reuse case
shutdown's dispose unguarded -> frees-the-slot-on-shutdown
sweep force-release dispose unguarded -> frees-the-slot-on-force-release
read window widened + decoder removed -> the multi-byte case
356 passed. typecheck 0 errors. 0 leaked captures, 0 leaked probes.
* test(dev-server): assert the exact terminal status per release path, not either-of
`toContain(['cancelled','timeout'])` passes with the WRONG status for the path
under test — a shutdown reported as a timeout, or the reverse. That is the shape
of assertion that lets a real mix-up through, and this file now has two paths
that settle differently.
Controlled: making shutdown settle 'timeout' now fails two tests; before this it
failed neither.
* fix(dev-server): a throwing log consumer leaked both descriptors and the file, and the healthy path's tail guard was never pinned
Round-4 delta re-audit. It reported no 🔴 and confirmed the previous round's
fixes hold — each killed by a mutant dying for its own reason, which is the
first round that has been true. These are its two 🟡 follow-ups, folded in
rather than deferred.
Draining calls back into `onLog`. With the close outside a `finally`, a consumer
that throws left BOTH descriptors open and the capture file on disk — measured:
fds 20 before and 20 after, file still present, 1 of 3 lines lost. Per run, in a
daemon that runs for days. And the slot-freeing guard added last round would
then swallow it, so it was silent as well as leaky. Both `finish()` and
`dispose()` now close in a `finally`.
That guard is no longer silent either. Swallowing the error hid a clipped log
behind `logsDropped: 0`, which is the one outcome the module's own comment says
the log contract rules out — and which the `[capture truncated]` marker two
lines below exists to prevent. Both sites now `addLog(run, 'error', …)`.
🔴 The tail guard covered the RARER path. `finish()` and `dispose()` have the
identical clearInterval -> drain -> close shape, and last round pinned only
`dispose()`. `finish()` runs on every normal child exit, so the stray-interval
hazard the round proved is not harmless was pinned on the edge case and left
open on the common one. Removing `finish()`'s clearInterval survived the whole
suite; it now fails.
Also: the fd-reuse tests had no self-check that the reclamation they depend on
actually happened. If `openSync` ever stops handing back the released numbers,
`readSync` gets EBADF, `drain()` breaks silently, and the test goes on passing
while protecting nothing. Both now assert the descriptor was reclaimed.
And a sentence on why `kill(true)` beside the guarded dispose is NOT guarded —
`defaultStartRun`'s kill body is wholly inside its own try/catch and the daemon
always uses that runner — so the asymmetry does not have to be re-derived.
Battery, each alone, every verdict gated on a `Tests` line:
finish() loses clearInterval -> the healthy-path tail case (was surviving)
dispose closes outside finally -> the throwing-consumer case
finish closes outside finally -> the throwing-consumer case
CORRECTION to the previous message: I described `err?.code ?? err?.message ??
String(err)` as a fix. It is an EQUIVALENT MUTANT — `readSync` only ever throws
a Node SystemError, so reverting it is undetectable on any reachable input. It
costs nothing and I have kept it, but it is defensive hardening, not covered
behaviour, and I should not have implied otherwise.
358 passed. typecheck 0 errors. 0 leaked captures, 0 leaked probes.
* fix(dev-server): the test asserted deletion of files it did not own, and the round's headline change was untested
Round-5 delta re-audit. No 🔴 and no regression introduced — the first time that
has been true twice running. These are its three follow-ups.
🔴-in-practice: the throwing-consumer test snapshotted /tmp by PREFIX and
asserted every match was deleted. That matches captures owned by the operator's
own daemon, which creates one whenever it runs a queued test — so the suite goes
red with no defect present. Not theoretical: during the audit a single foreign
file made the UNMUTATED test fail, and made three mutants report a false KILLED.
The filename carries the owning pid for exactly this reason; both snapshots are
now scoped to `civitai-test-run-${process.pid}-`. Controlled: with a foreign
file planted, 359 pass and the file is left untouched.
The headline change of the previous round was untested. `finish()` and
`dispose()` have the identical drain-then-close shape and the throwing-consumer
test drove only `dispose()` — reverting `finish()`'s try/finally survived the
whole suite. `finish()` is the path every healthy run takes. It now has its own
case driving `child.emit('exit', 0)`, and the revert fails.
Nothing asserted the `capture release failed` line either, so both catch sites
could go back to a silent swallow with the suite green — which would restore
exactly the hidden-clipped-log outcome the line was added to prevent. Asserted
now; both silent-swallow mutants fail.
🔴 CORRECTION. The previous message claimed I had added a comment explaining why
`kill(true)` is unguarded while its neighbour is. That comment was NOT in the
tree — my edit's anchor did not match and I did not verify that one landed. The
claim itself was true and the auditor re-derived it independently, but I stated
as done something I had not checked. It is there now. This is the second false
claim I have put in a commit message on this branch; the pattern in both cases
was asserting an edit without grepping for it afterwards.
Battery — the three that survived the last round:
finish() reverts try/finally -> the normal-exit release case
sweep catch goes silent -> the sweep force-release case
shutdown catch goes silent -> the shutdown case
All killed. Every verdict gated on a `Tests` line.
359 passed. typecheck 0 errors. Clean /tmp before and after the shipped suite:
0 captures, 0 probes. (The one file that appeared mid-battery was the M3 MUTANT
leaking by design — content `boom one/boom two/x`, the `x` proving the
descriptor stayed open, which is what the kill detects.)
* test(dev-server): a positive control on the pid-scoped /tmp filter, so an empty match cannot pass as a no-op
Scoping the snapshot to this process fixed the cross-process false failure, and
introduced the opposite hazard: `for (const f of mine) expect(...)` over an
EMPTY list is a loop that runs zero assertions and passes. If the capture
filename format ever changes, both deletion tests would go on passing while
protecting nothing — the same vacuous-green shape this branch has now hit in
three different places.
Measured at exactly 1 match, and pinned there. Controlled: breaking the scope so
it matches nothing fails both tests, where before it would have been silent.
* test(dev-server): own captures by DELTA, so a sibling's defect stops failing this test on bookkeeping
Round-6 delta re-audit: no 🔴, no production defect, no regression — the range's
only non-test change was three comment lines. This is its one 🟡 plus two
accuracy fixes.
The capture snapshot claimed every live capture for this pid, and these cases
share a fork. So a defect in the SIBLING test's subject left its file behind and
this test failed on the file count rather than on its own assertion. The mutant
still died — at the wrong line, which silently over-credits whatever this test
was meant to cover. That is the same wrong-reason-kill problem this branch has
been chasing since round one, arriving in the fixture instead of the guard.
Now a before/after delta, so ownership is exact. Controlled: with dispose()'s
try/finally removed, ONE test now fails — the dispose one, at
`expected [Function] to throw an error`, its own assertion. Previously BOTH
failed and the finish() case reported
`expected [ …(2) ] to have a length of 1 but got 2`.
Two comments corrected:
- "it is the only `new TestQueue`" was false repo-wide — three more exist in the
test file, two passing a startRun. The load-bearing claim (production never
injects a runner) is true; the sentence was not, and the next reader greps it.
- The throwing-consumer leak is NOT reachable through the daemon today: its
`onLog` is `addLog`, which cannot throw. The round-5 comment described it as
happening "per run, in a long-lived daemon". It is cheap insurance against a
future consumer, and now says so.
359 passed. typecheck 0 errors. 0 leaked captures.
|
||
|
briant
|
c92c71e2a5 |
Merge remote-tracking branch 'origin/main' into moderator-feedback
# Conflicts: # scripts/__tests__/dev-server-daemon-port.test.ts # scripts/__tests__/typecheck-apps.test.ts # src/__tests__/source-nul-bytes.test.ts # src/components/Apps/__tests__/appListingStatChips.test.ts # src/server/services/__tests__/model-file-hash-writers.test.ts # src/tests/build/standalone-boot-graph.test.ts # src/utils/__tests__/rating-label.test.ts |
||
|
briant
|
ba7504aac3 |
fix(tests): make seven suites pass on Windows
17 tests across 7 files failed on a Windows checkout and pass on CI. None was a product defect; all were portability bugs in the harness code, and two of them were reporting the thing under test as broken when it was not. Path separators. `path.relative` yields backslashes on Windows while the ledgers these tests compare against are written with `/`, so the walk matched nothing and the ledger read as "no consumers" — `rating-label`, `model-file-hash-writers`, `dev-server-daemon-port`. Normalised at the walk. `new URL(..).pathname` gives `/C:/…`, which resolves to `C:\C:\…` and ENOENTs — `appListingStatChips` now uses `fileURLToPath`. `standalone-boot-graph` asserted on `esm/index.js` in output where Node prints a NATIVE path. `dev-server-daemon-port` spawned `import(<absolute windows path>)`, which ESM rejects with ERR_UNSUPPORTED_ESM_URL_SCHEME because the drive letter parses as a scheme. The child exited 1 and the test — "can be imported without throwing" — reported the daemon as broken for a reason unrelated to the daemon. Now `pathToFileURL`. Two are genuine environment limits and are SKIPPED rather than weakened, so the gap stays visible. The three symlink CONTROLs in `source-nul-bytes` need elevation on Windows (EPERM, reproduced); they are gated on a capability probe rather than `process.platform`, so Linux CI and an elevated shell still run them, and the main NUL-byte assertion is untouched. `typecheck-apps` puts a `#!/usr/bin/env bash` fake `pnpm` on PATH with `chmod 0755` — neither means anything on Windows — so it is skipped there; its `:` PATH separator is fixed to `path.delimiter` regardless. Counts, since skips can hide regressions: 19502 passed + 27 skipped + 17 failed before, 19506 passed + 40 skipped + 0 failed after. Same 19546 total; the 13 new skips are exactly the 3 symlink controls and the 10 typecheck-apps cases. Also clears two pre-existing eslint errors in a file already being touched. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> |
||
|
Zachary Lowden
|
94510da5f3 |
fix(metrics): bulkhead + pg-pool gauges emitted zero series — a globalThis flag guarding a per-graph prom registry (#4173)
* fix(metrics): a globalThis flag guarding a per-graph registry made the bulkhead gauges emit nothing for 74 days `civitai_app_heavy_bulkhead_active` / `_rejects` have produced ZERO series since they shipped in #2428 (2026-06-07), while every link in the chain looked healthy: registered in code, merged to release, in the running image, on a hot route, on pods emitting hundreds of thousands of other series. Two independent defects, and fixing either one alone leaves the metric inert. 1. REGISTRY. Next.js compiles instrumentation.ts into a separate webpack bundle from the pages/API bundle, and prom-client is not in serverExternalPackages, so each graph gets its own `client.register`. /api/metrics scrapes the PAGES graph's default registry plus the globalThis-pinned instrumentationRegistry; the instrumentation graph's default registry is scraped by nobody. The gauges were wrapped in `if (!global.heavyBulkheadGaugeInitialized)`, which pairs a PROCESS-scoped flag with a GRAPH-scoped registry. That is worse than no guard: instrumentation.node.ts -> eventloop-longtask -> prom/client runs at pod start, claims the flag, registers into its own unscraped registry, and the pages graph then takes the early-out and registers nothing. Now registered via registerInstrumentationMetric, which dedupes against the same shared registry it writes to, so the two scopes agree. 2. STATE. The gauges are collect()-based over `bulkheadSnapshot()`, and request-bulkhead's slot/reject Maps were module-local — so whichever graph won registration read its own permanently empty maps. Pinned on globalThis, the same mechanism instrumentationRegistry and __civitaiRedisMetrics already use. Admission control was always correct in the graph serving requests; this makes the cap genuinely per-POD rather than per-pod-per-graph, which is what it was always documented to mean. The nine `node_postgres_*` pool gauges carried defect 1 via `pgGaugeInitialized` and were measured at 0 series too. Fixed in the same commit: same bug, same file. Measured on production before the fix, which is what isolates the flag as the variable rather than the registry or the graph: civitai_app_heavy_bulkhead_{active,rejects} 0 series (default reg + flag) node_postgres_* (9 gauges) 0 series (default reg + flag) civitai_app_image_ingestion_backlog 640 series (shared registry) images_search_* (same request path) 181 series (default reg, no flag) Tests: 8 new cases in src/server/__tests__/prom-cross-graph-registration.test.ts, using vi.resetModules() between dynamic imports to reproduce the two-graph split (fresh module instance, intact globalThis — the exact asymmetry the bug lived in). Matrix: 8/8 RED at origin/main, 8/8 green here. With ONLY defect 1 fixed, 3 pass and the two seam cases still fail — the gauges register, get scraped, and render nothing — which is what proves the second half is load-bearing rather than tidy-up. Also corrects a comment in @civitai/telemetry that cited these guards as the working precedent for pinning on globalThis. They pinned the flag, not the thing it guarded, and every metric behind them was dead. The Grafana panel's `or vector(0)`, which turned this absence into a drawn zero line, is removed separately in the infra repo. Refs #2428. Refs clawgate task 299. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * style: prettier — formatting only, no behaviour change Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(metrics): audit round — guard the scrape, kill 5 surviving mutants, enrol the pin in the gate Adversarial audit findings. No deploy-blocker; these are the three 🟡s worth closing before merge, plus the fixture defect that let them hide. 1. GUARD THE SCRAPE (src/pages/api/metrics.ts). `Registry.metrics()` is a Promise.all over every metric's get(), so ONE collect() that throws rejects the whole call — and `Gauge.set` throws on a non-number. This PR moved 11 new collect() bodies into that awaited path, six of them reading pool counters unguarded, so a single bad read would 500 the ENTIRE scrape: default metrics, every instrumentation metric and the Prisma series with it, precisely when you most need to see the pod. Both registries now degrade to losing one block, with a seeded `registry_scrape_failures_total{registry}` so the loss is observable rather than silent. The file already wrapped a strictly smaller risk the same way. 2. THE TESTS DID NOT PIN THE PG GAUGES — proven, not assumed. The stub gave every pool `{totalCount: 0, idleCount: 0, waitingCount: 0}`. Identical, default values across four pools make every transposition render byte-identical output, and the nine gauges are (name, help, reader) triples where a wrong pool or wrong counter is a one-token slip no type checker can catch. Five mutants SURVIVED a fully green suite. Fixture values are now pairwise distinct (11/12/13, 21/22/23, 31/32/33, 41/42/43) and two cases assert rendered VALUES, including an absent-pool case that reaches the null-guard branch — which no all-pools-present fixture can. Battery re-run after every change in this commit, baseline green either side: M1 labelled write reads pgDbRead ........ KILLED (was SURVIVED) M2 read_idle reads .totalCount .......... KILLED (was SURVIVED) M3 write_waiting reads pgDbRead ......... KILLED (was SURVIVED) M4 drop the labelled null-guard ......... KILLED (was SURVIVED) M5 swap read_long/datapacket labels ..... KILLED (was SURVIVED) PC1 pin `??=` weakened to `=` ........... KILLED (the original bug, one character) PC2 un-pin entirely ..................... KILLED (10/10 fail) PC3 rejects gauge reports active ........ KILLED The harness itself needed two fixes first: the vitest summary is ANSI-prefixed so the grep matched nothing and scored every mutant SURVIVED including the positive control, and the copied tree's .envrc was un-allowed so nothing ran at all. Both produced a confident, uniform, meaningless result — hence the mutator now refuses to report when the file hash did not move or no summary line was found. 3. ENROL THE PIN IN THE GATE THAT EXISTS FOR THIS (scripts/server-graph-watchlist.mjs). The repo has a build gate that fails when a module needing process-wide identity loses its globalThis pin. This PR added such a pin and enrolled it nowhere, so a future refactor reintroduces the defect silently — which is how it happened. Both maps now live under ONE key, `__civitaiBulkheadState`. Two keys would mean two watchlist entries naming the same module, which the gate cannot express: its fixture emits one chunk per entry carrying only that entry's key, so each entry fails on the other's chunk. Measured — it red-lined the gate's own positive control. The pin also moves to the repo's canonical `??=` form, which the gate's test asserts against comment-stripped source so a key surviving only in prose cannot pass for a binding. Verified: typecheck 0 errors (103s); prettier clean on all 5 files; 109 files / 1895 tests green across src/server/__tests__, scripts/__tests__ and src/tests/api/v1; test:lint-rules 243/243. Refs clawgate task 299. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * revert(metrics): keep the pg pool gauges OFF the shared registry — moving them made them WRONG Reverts the pg-gauge half of this PR. The bulkhead fix is untouched. Two independent agents flagged the same unverified risk, so I measured it, and it is real. A collect()-based metric needs BOTH halves shared: the registry it registers into AND the state its closure reads. The bulkhead has both — request-bulkhead.ts pins its maps on globalThis. The pg gauges have only the first: src/server/db/pgDb.ts globalThis-pins the pools ONLY in its `!isProd` branch (pgDb.ts:26-42), so in production every emitted copy of that module builds its own pools. The graph that wins registration is the instrumentation graph, whose pools serve nothing but the ingestion-backlog query in prom/client.ts. Measured on a preview running exactly the reverted change: metric idle under 30 concurrent /api/v1/images node_postgres_read_total_count 1 1 node_postgres_read_waiting_count 0 0 node_postgres_write_total_count 0 0 node_postgres_pool_total_count 1 / 0 / 0 / 0 (unchanged under load) Frozen, plausible-looking and wrong. That is strictly WORSE than the honest absence they have today: an absent metric prompts a question, a confident 0 ends one — the same false-all-clear class as the `or vector(0)` this PR removes from the reject panel, which is the defect the whole change exists to fix. Shipping it would have re-enacted the bug while claiming to fix it. So they go back to exactly their `origin/main` form, under `global.pgGaugeInitialized`, with a long note at the site saying what was tried, what was measured, and why the next person must not repeat it. The test that asserted they reach the shared registry is INVERTED: it now pins that they stay off it, so a future well-meant move has to change the test and read the note. Making them real means pinning the pools in pgDb.ts for prod. That changes production DB connection topology (today: one pool set per emitted graph, which is also worth someone's attention on its own) and is its own change with its own blast radius, not a rider on a metrics fix. Consequence for the previous commit: its mutation battery covered the pg refactor that no longer exists. The five mutants it killed are moot; the three that matter — the `??=` pin weakened to `=`, the pin removed, and the rejects gauge reporting active — are all on the bulkhead half and still die. Re-verified after this revert: typecheck 0 errors, prettier clean, 109 files / 1893 tests green. Refs clawgate task 299. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(metrics): delta re-audit round — the pg test was VACUOUS, and the new guard had no test A blind delta re-audit of the previous two commits returned two blockers, both of them the half-revert signature: the test offered as the pin for the reverted pg change asserted nothing, and a comment claimed coverage that no assertion provided. 🔴 THE INVERTED PG TEST WAS VACUOUS — it asserted about code that never ran. `clearCrossGraphState` reset the bulkhead pin and the shared registry but NOT `global.pgGaugeInitialized`. The first test to import prom/client set that flag, so every later import took the early-out and the pg block never executed. Two mutants proved it, both SURVIVING a fully green run: adding `registers: [instrumentationRegistry]` to a pg gauge (the exact "future well-meant move" the comment said the test would catch), and disabling the whole pg block with `if (false)`. The test could not tell "deliberately on the default registry" from "deleted". Resetting the flag alone does not fix it — the re-audit found that too. `prom-client` is EXTERNALIZED, so `vi.resetModules()` hands every "graph" the same module instance and the same default `client.register`; re-running the pg block then throws "already registered" and reds four tests on unmutated code. Both must be reset together, so `clearCrossGraphState` now also clears the default registry. That is worth knowing on its own: this suite's two "graphs" share one default registry, so no test may infer graph identity from it. Written into the comment. The test now pins BOTH halves — present on the default registry (proving the block RAN) and absent from the shared one (the invariant). Without the first half the second is satisfied by a deleted block. 🔴 A NINE-LINE COMMENT CLAIMED TRANSPOSITION COVERAGE THAT DID NOT EXIST. The revert deleted the value assertions but left the pairwise-distinct fixture and a comment reading "EVERY ONE OF THESE TWELVE NUMBERS IS DISTINCT, AND THAT IS THE WHOLE POINT… Keep them distinct if you touch this." Setting all 24 numbers to 0 — the exact configuration the comment warns against — SURVIVED. Value assertions are restored against the default registry, so the apparatus and its comment are true again. Battery re-run, baseline green either side: A-M4 pg gauge moved to instrumentationRegistry ... KILLED (was SURVIVED) A-M3 whole pg block disabled `if (false)` ........ KILLED (was SURVIVED) A-M1 all 24 fixture numbers -> 0 ................. KILLED (was SURVIVED) T1 write_total reads pgDbRead ................... KILLED PC pin `??=` weakened to `=` .................... KILLED 🟡 THE SCRAPE GUARD HAD NO TEST — added in response to an audit finding, and nobody had watched it go red. New suite `metrics-endpoint-registry-failure.test.ts`, 5 cases, mirroring the sibling Prisma-failure harness. It plants a gauge whose collect() really throws rather than stubbing `metrics()`, so it walks the production rejection path. Verified red: dropping the guard and awaiting the registries directly fails exactly the 3 🔴 cases (5 passed -> 3 failed | 2 passed); the positive control and the seed test correctly stay green, since neither depends on the guard. 🟡 THE FAILURE COUNTER WAS DROPPED BY THE FAILURE IT COUNTS. It lived on `client.register`, so a default-registry rejection removed that whole block — counter included — leaving a permanently absent series exactly where a rising one is expected. Moved to `instrumentationRegistry`; one test now asserts a default-registry failure is reported in the SAME response that lost the block. Residual blind spot documented, not hidden: an instrumentation-registry failure still loses it, but that also removes every instrumentation metric at once. 🟡 THE RECOVERY PATH COULD THROW OUT OF THE GUARD. `registryScrapeFailures.inc` inside the catch was unguarded while the module-scope seed of the same counter was wrapped for exactly that reason. Unchecked `as` cast + a labelset collision would have thrown out of the block whose job is to prevent a 500. 🟡 A HEADING FALSIFIED BY THE REVERT: "WHY EVERY GAUGE BELOW USES registerInstrumentationMetric" sat above nine gauges that use the flag. Corrected, along with a cross-repo "this PR" reference a civitai reader cannot follow, and the fixture comment's description of a code shape the revert removed. Verified: typecheck 0 errors; prettier clean on 6 files; 110 files / 1900 tests green; test:lint-rules 243/243. Refs clawgate task 299. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * style: close the four cosmetic findings from the delta re-audit - Restore the blank line before the Buzz-escrow JSDoc that the pg revert swallowed, so the reverted block is now blank-line-identical to origin/main (verified: zero whitespace-only hunks remain in the diff for that file). - Restore the paragraph break the single-key note ran into, so two separate arguments stop reading as one run-on block. - "Destructured once" described two property reads; say what the code does. - A backtick standing in for an apostrophe inside a single-quoted string. No behaviour change. Verified: prettier clean, 66 files / 1208 tests green. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(metrics): round-3 audit — unbreak two unrelated suites, and a test that passed on residue Round 3 found a REGRESSION I shipped in round 2, reproduced independently by CI. 🔴 MOVING THE COUNTER LOOKUP TO MODULE SCOPE BROKE TWO UNRELATED SUITES. `src/__tests__/setup.ts` wholesale-mocks `~/server/prom/client` and omitted `instrumentationRegistry`. While that lookup sat inside the handler body nothing noticed; hoisted to module scope it throws during module EVALUATION, so any suite that merely IMPORTS the metrics page dies — `No "instrumentationRegistry" export is defined on the "~/server/prom/client" mock`. Measured per-ref, each in its own run: `f7a6ccc85f` 5 passed · `497876f896` 3 failed | 2 passed · `origin/main` 5 passed. Casualties were metrics-endpoint-seeds-substitutions and metrics-endpoint-seeds-bitdex-feed-serve — both import-only suites, which is exactly why every metrics-SPECIFIC suite stayed green: they each mock the module themselves. The blocking `Unit tests` job was red with the same error at the same line. Fixed in one place: the mock now supplies a REAL throwaway `Registry`, not a stub, because consumers call `.metrics()`, `.getSingleMetric()` and register into it — a vi.fn() stub would let a suite assert against a registry that never held anything. 🔴 MY GETTER-BASED pgDb MOCK RED-LINED A REPO-WIDE GATE. `pgDbMock.parity` scans `vi.mock` factories for the literal `name:` spelling, so `get pgDbRead()` — which does supply the export — read as "missing: pgDbRead, pgDbReadLong, pgDbWrite". Introduced in round 1 and missed by two audits. The factories are back to literal properties; the absent-pool case uses `vi.doMock` with its own COMPLETE factory rather than mutating a getter. I did not loosen the gate to fit my code. 🟡 THE SEEDING TEST PASSED ON RESIDUE. It ran last, and by then two earlier cases had incremented both labels on a module imported once per file — so deleting both `inc(…, 0)` calls SURVIVED the full-file run and failed only in isolation. The seeded-at-0 assertion moves into the POSITIVE CONTROL, which runs first, where it is the only place it can mean anything. The trailing case now asserts something the first cannot: both label series are still rendered while one registry is failing. 🟡 THE FIXTURE HAD A LIVE HALF AND A DEAD HALF. `beforeEach` overwrote `pools` from `POOL_DEFAULTS`, so the twelve numbers in the `pools` initializer were never read — zeroing them SURVIVED while zeroing the copy was caught, and the nine-line "keep these distinct" warning sat above the dead half. One declaration now, mutated in place. 🟢 Also: the absent-pool case covered 2 of the 3 labelled gauges (`?? 0` -> `?? -1` on the idle guard SURVIVED); the new suite un-mocked prom/client without mocking pgDb, so a real `pgDbRead.connect()` fired once per run; and a docblock named the wrong tests. Battery re-run over BOTH suites, baseline 15/15 green either side: M1 scrape guard removed .............. KILLED (3) M2 seeding inc(...,0) removed ........ KILLED (1) <- SURVIVED before this round M3 counter back on client.register ... KILLED (3) M4 POOL_VALUES all zeroed ............ KILLED (2) <- SURVIVED before this round M5 pg gauge -> instrumentationRegistry KILLED (2) M6 whole pg block if(false) .......... KILLED (3) M7 idle labelled guard ?? 0 -> ?? -1 . KILLED (1) <- SURVIVED before this round PC pin `??=` weakened to `=` ......... KILLED (5) Verified: typecheck 0 errors; prettier clean on 7 files; 123 files / 2034 tests green; test:lint-rules 243/243. Refs clawgate task 299. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
briant
|
6c9904b389 |
fix(tests): unbreak the unit suite on Windows — 16 failures across 7 files, and two of them could never pass
Measured on a Windows checkout: 7 files / 16 tests fail identically before and after any local
change, so every Windows developer has to hold "these are expected" in their head to read a suite
result. 868kubhjr tracked four of them; the set had grown to seven.
Five are the separator bug that ticket describes — `path.relative` yields backslashes and the
assertion compares against forward-slash literals — fixed with `.split(path.sep).join('/')`, the
prescribed shape. `standalone-boot-graph` is the same fault in a Node error string, so it matches
`/esm[\/]index\.js/` instead of a literal.
Two were NOT cosmetic, and would have kept failing after the separator fixes:
* `appListingStatChips` read `new URL(…).pathname`, which is `/C:/…` on Windows, so `readFileSync`
resolved it against the drive root as `C:\C:\…`. Now `fileURLToPath`.
* `dev-server-daemon-port` spawned `await import("C:\…")`, which node rejects with
ERR_UNSUPPORTED_ESM_URL_SCHEME — it drives a child and asserts exit 0, so on Windows it could
never observe the property it claims. Now `pathToFileURL`. Verified the harness is live again:
the real daemon with a bad port exits 0, a deliberately throwing module exits 1.
`source-nul-bytes` needs symlinks, which Windows refuses without SeCreateSymbolicLinkPrivilege.
Junctions need no privilege and the walk cannot tell them apart (`lstat().isSymbolicLink()` is true,
dangling ones included), so the three controls still run rather than skipping.
`typecheck-apps` needed the script itself. `spawnSync` skips PATHEXT so a bare `pnpm` is ENOENT on
Windows, and naming `pnpm.cmd` is EINVAL — node refuses to spawn a .cmd without a shell. Hence
`shell` on win32 only; ubuntu CI takes the false branch and is byte-identical.
That shell brings a hazard, so it does not arrive alone: `shell: true` applies NO per-argument
quoting, and `app.name` is read raw from `apps/*/package.json`. A name containing `&` ends the
command under cmd.exe, so cmd returns the trailing command's exit code and a genuinely-red typecheck
reports 0 — reproduced, and precisely the reports-success class this script exists to close. Names
are now validated with a hard error.
The suite's fake pnpm answered by exit code alone, so it could not see WHAT the script asked pnpm to
run: mutating the spawn to `--filter totally-wrong-<dir> run lint` left all tests green. It now
echoes argv and one case asserts it, which also pins argv fidelity through cmd.exe.
Windows unit suite: 16 failures -> 0. Nothing changes on ubuntu CI.
Closes 868kubhjr
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
||
|
Zachary Lowden
|
69b5ce5497 |
fix(typecheck): typecheck scripts/ — 15 non-test files now covered, scripts/__tests__ excluded like src/ (#4189)
* fix(typecheck): close the scripts/ blind spot — 31 files now checked, 5 quarantined behind a ratchet The root tsconfig named `scripts/local-dev/*.ts` in BOTH `include` and `exclude`. Exclude wins, so nothing under `scripts/` was typechecked at all — not even the one directory the include list appeared to name. Measured on |
||
|
briant
|
15c1408d3d |
fix(db-schema): the enum generator emitted unformatted output, so db:check-generated failed on a clean checkout
`pnpm run db:check-generated` has been failing for anyone who ran it, with no change of their own. `scripts/prisma-enum-generator.mjs` wrote each type alias on one line and never formatted the result, while the committed `enums.ts` is prettier-wrapped at the repo's printWidth of 100. Generator output could therefore never equal the committed file, and `postinstall` runs `db:generate` — so every `pnpm install` left the file dirty and every run of the check gate went red. CI does not run that gate, which is why this survived; the only person who hits it is someone following "Before Committing" step 5. The generator now formats with the resolved prettier config before writing. That is scoped to the one file this generator owns rather than a prettier pass over `packages/civitai-db-schema/src`, because the neighbouring generated files are NOT prettier-clean — `models.ts` is emitted with double quotes against a `singleQuote: true` config and passes the check exactly as it is. Formatting the directory would reformat it wholesale for no reason. With the generator fixed, the ~90 lines of churn collapse to one real staleness: `CollectionItemRejectionReason` was committed as raw generator output in `1ad8ae3443` while every other alias in the file was wrapped. That line is now regenerated, and `db:check-generated` passes. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
Zachary Lowden
|
b8f84204d7 |
fix(auth): fix the two type errors gating apps/auth, and bring it into the CI typecheck gate (#4188)
apps/auth was the last app excluded from `scripts/ci/typecheck-apps.mjs` (#4156), on two pre-existing errors. Both are fixed at the root, not silenced, and the exclusion is gone — the gate now covers all 7 apps. providers.ts:165 — Parameter 'p' implicitly has an 'any' type The stub entry's key is a COMPUTED property (`['stub' as ProviderId]`) whose type is the whole ProviderId union rather than one literal, so TS cannot match it to a member of `Record<ProviderId, ProviderDef>` and drops contextual typing for the value. `satisfies ProviderDef` restores it. Measured, both arms: with `satisfies`, misspelling a required field (`scope` -> `scopes`) is caught; with it removed, the same misspelling is silently accepted and only the implicit-any resurfaces. So the whole entry was unchecked against ProviderDef, not just that one parameter. establish-session.test.ts:100 — Property '_store' does not exist on type 'never' The Cookies stub was cast `as never` to satisfy establishSession's parameter, but `never` has no properties, so reading `_store` back in an assertion was itself an error. Cookies has exactly five members, so the stub now implements all of them and needs no cast at all. Removing the exclusion breaks the guard suite, which is the real work here: six of its eight cases put an `auth` app in every fixture purely to satisfy the stale-exclusion guard, so an empty map made them fail for reasons unrelated to what they test. Rather than delete those guards, the exclusion map is now a parameter of an exported `runTypecheckApps({ excluded })`, and the tests drive it with a synthetic app. Deliberately a function parameter and not an env var: an env-var override would be a live way to un-gate an app in CI, which is the failure class the script exists to close. All ten cases now pass regardless of what the shipped map contains. Also neutralises the "six apps" counts in the script header and workflow comment, which this change would otherwise make wrong, and applies prettier to two pre-existing unformatted lines in the touched files. |
||
|
Zachary Lowden
|
1891ce76aa |
fix(dev-server): the daemon never read DEV_DAEMON_PORT, so setting it broke the CLI instead of moving the daemon (#4181)
* fix(dev-server): the daemon never read DEV_DAEMON_PORT, so setting it broke the CLI instead of moving the daemon The variable was read where the client decides what to CONNECT to and ignored where the daemon decides what to LISTEN on. `DEV_DAEMON_PORT=9555 cli.mjs status` therefore pointed the CLI at :9555, spawned a daemon that bound :9444, and could not reach it — the daemon has always accepted `--port`, the spawn simply passed no arguments and the daemon read no environment. Four files each decided the port for themselves and two of them were wrong: cli.mjs and scripts/test-unit-run.mjs read the variable, console.mjs hardcoded 9444, and daemon.mjs saw only argv. That is not a bug any one of them contains — it is a bug in the set, so the number now lives in exactly one module and all four resolve it there. A daemon a client spawns inherits that client's environment, so both ends read the same variable through the same function; an explicit `--port` still wins for a daemon started by hand. resolveDaemonPort also refuses a value that is not a port rather than handing back parseInt's NaN, which used to reach a URL as `http://127.0.0.1:NaN` and fail a long way from its cause. Verified by reproducing the reported path, not by reading the code. On pre-change code with DEV_DAEMON_PORT=19461 the daemon logged `Daemon port: 9444`, emitted no ready line, and nothing ever listened on 19461. After the change it reports and binds 19461, and the pid it reports is the child that was spawned — a port that answers proves a listener, not THIS listener. Six isolated mutants, each killed by its own named assertion: daemon ignores the environment -> both behavioural cases daemon ignores --port -> the --port precedence case resolver stops validating -> the rejects-a-non-port case console.mjs re-hardcodes 9444 -> both halves of the ledger test-unit-run.mjs re-hardcodes -> both halves of the ledger the default drifts to 9445 -> the default case console.mjs is a TUI with no end-to-end case here, which is why the ledger exists: it fails when the set of files deciding the port grows or shrinks. Closes ClickUp 868kuaa4e. * fix(dev-server): the audit round — the test deleted the pid file it was written to protect, and the ledger could not see the set grow 🔴 The blocking one. `POST /shutdown` replies 200 and only THEN schedules `unlinkSync(pidFile); process.exit(0)` on a 100 ms timer, so the helper that saved and restored the developer's `daemon.pid` restored it ~100 ms before the daemon deleted it. `scripts/**/*.test.ts` is in the `unit` project, so every `pnpm test:unit:run` removed `.claude/skills/dev-server/daemon.pid` — and the file is only written at daemon START, so it did not come back. That breaks the recipe SKILL.md itself gives for checking the daemon's interpreter, and makes `cli.mjs shutdown`'s cleanup a no-op. `shutdown()` now waits for the port to stop answering before returning. The ledger scanned a FIXED list of four readers, so it could not detect the set GROWING — which the PR description claimed it did. A fifth hardcoded 9444 already existed while the test was green: `.claude/hooks/check-writable.mjs` baked it into the dev-port regex, so `DEV_DAEMON_PORT=9555` silently switched that nudge off for the daemon, the one long-lived server it most exists for. The ledger now walks the tree, so a file that does not exist yet is covered, and the hook reads the port from the module. Verified with a negative control: with DEV_DAEMON_PORT=9555, `curl :9555` nudges and `curl :7777` stays silent. The ledger's assertions were also spelled rather than structural. `includes('daemon-port.mjs')` is satisfied by a COMMENT — and this file's own prose names the module in several — and `includes('resolveDaemonPort(')` is satisfied by any call whatever is done with the result, so `resolveDaemonPort() + 1` passed all of it while console.mjs has no behavioural test. Both clients now take a whole URL from `resolveDaemonUrl()` and do no arithmetic on a port; the assertions pin an import line and that call. Three more the audit found: - `scripts/test-unit-run.mjs` lost its "never leave a caller unable to run tests" guarantee. Resolution moved inside `runQueued` but OUTSIDE its try, and `runQueued` is un-awaited, so a malformed DEV_DAEMON_PORT became an unhandled rejection and no tests ran. Measured against base: base printed "running directly", HEAD died. Restored, with the outer `.catch()` as the general net. - `--port` still open-coded `parseInt`, so the HIGHER-precedence input was the unvalidated one — `--port abc` gave ERR_SOCKET_BAD_PORT, which via cli.mjs is invisible (detached, stdio ignored) and reads only as "Failed to start daemon". Both inputs now go through `parsePort`, which names the input it is complaining about. - Resolving at module load made a bad DEV_DAEMON_PORT throw on merely IMPORTING daemon.mjs, which would have broken collection of the port-reservation suite. Resolution moved into `parseArgs`. Nits: SKILL.md said `cli.mjs:69`, the line is 70 — my own earlier check of that number had gone stale under a later edit. Out-of-range is now a different message from unparseable ('0' is a number, just not a port). The 9444 scan uses a word boundary so 19444 is not a false positive. Seven mutants, each applied alone, tree restored and cmp-verified between: shutdown() stops waiting -> the pid-file case a NEW file hardcodes 9444 -> the ledger (the growth case) the hook re-hardcodes 9444 -> the ledger console.mjs does resolvePort() + 1 -> the client-address case the targeted fallback is removed -> the degrade-to-direct case --port back to parseInt -> the argv validation case port resolved at module load -> the import-safety case 🔴 One of them SURVIVED the first run and the fix is the point: the fallback case asserted the generic phrase "running directly", which BOTH guards on that path print, so deleting the targeted guard left the suite green — the mutant died to the other guard. The assertion now names the specific message. scripts/__tests__ 324 passed. typecheck 0 errors. Hook selftest green. * fix(dev-server): round 3 — the last round's hook fix un-guarded the default port, and its catch would have double-run the suite Both regressions were introduced by the previous commit, not pre-existing, and a delta re-audit of 7dd65a9d62..85cb922567 found them. 🔴-in-effect: `DEV_DAEMON_PORT` REPLACED the guarded port instead of adding to it, so setting it un-guarded the shared daemon. Reproduced through the hook's real stdin contract: with the override set, `curl http://localhost:9444/...` went from ask to allowed, and the hook's own selftest went from `all green` to `1 FAILURES`. SKILL.md documents the override as standing a daemon BESIDE the shared one — both are live, so both need guarding. `daemonPortsGuarded()` now returns the union, and the selftest is green both plain and under the override. The other one is subtler and worse in effect. The outer `.catch()` I added wrapped the whole `runQueued` lifecycle, not just queue acquisition — so a socket dropped mid-poll abandoned a run the daemon had ALREADY ACCEPTED and started a second, unqueued full suite beside it. That defeats the serialisation this script exists for, and the file had already decided the opposite for the status-code form of the same condition (`Lost contact with the test queue` -> exit 2). It now tracks whether the run was accepted: before acceptance it degrades to a direct run, after acceptance it exits 2. `err?.message`, because a non-Error throw printed `(undefined)` and a null throw raised inside the handler — back to the unhandled rejection the catch exists to prevent. Also from the re-audit: - The port scrape in the hook is a dependency on one line's formatting in another file and nothing tested it. A vitest case now drives the real `unboundedDevRequest`, with a negative control on a port the skill does not use. Breaking the scrape regex now fails two tests; it used to fail none. - `console.mjs` had no behavioural coverage, and every structural assertion was walkable by resolving the URL correctly and then drifting it. It now runs for real against a stub daemon on an ephemeral port — via `--tail`, since the dashboard refuses to start without a TTY and exits before contacting anything. - `--base-dev-port` was still `parseInt`, which made "argv gets the same validation the environment gets" false about half of argv. - The port range's upper bound was untested: fixtures were 0 and 70000, so 65535 could drift to 65536 unnoticed. Now pinned ON the boundary. - SKILL.md's `console.mjs:88` — the previous commit fixed the cli.mjs citation and moved console.mjs's line to 89 in the same change. Now 89. - The module header claimed the scan covers "the whole skill ... anywhere". It covers three roots and four extensions; prose and test files are deliberately out of scope. Said so instead. 🔴 The ledger caught ME during this round: a comment I wrote explaining the un-guarding bug spelled the port, and `spells the port in exactly one source file` went red. That is the guard working on its author — reworded, not exempted. Round-3 battery, each mutant alone, tree cmp-verified between: outer .catch() deleted -> the accepted-run case accepted-check removed -> the accepted-run case hook override replaces default -> the union case hook scrape regex broken -> both hook cases upper bound 65535 -> 65536 -> the boundary case console.mjs drifts the URL -> the behavioural case + the ledger --base-dev-port back to parseInt -> the base-dev-port case All killed. One deliberate survivor: a copy OUTSIDE the three scanned roots, which is the documented scope and is now stated in the header rather than overclaimed. 330 passed. typecheck 0 errors. Hook selftest green plain and under override. * style: prettier the added test file — the CI gate checks ADDED files and I never ran it `ESLint + Prettier (changed files)` went red on the round-3 push for one reason: `scripts/__tests__/dev-server-daemon-port.test.ts` was not prettier-formatted. Four lines, all wrapping. Reproduced the gate's exact scope locally rather than guessing at it — it runs `prettier --list-different` over ADDED files only, which is why the four `.mjs` files this branch MODIFIES were never checked. Those four are unformatted, and the control says that is not mine: at the merge base `bc74ba06ea`, all four are already unformatted. Reformatting them would rewrite files this change does not own, which is the breadth rule in CLAUDE.md, so they are left alone. Local prettier is 2.8.8, the same version CI installs, so the local format is the one the gate will read. 21 tests still pass. * fix(dev-server): round-4 tidy — the test that proves the console bug was itself destroying the pid file on its red path Follow-ups from the round-3 delta re-audit, which found no 🔴 and confirmed round 3 did not reintroduce a regression. These are the 🟡/🟢 it did find. The console test was not wrapped in `withPidFilePreserved`, and the exposure is specifically on the FAILING path: on green the stub answers `/`, so the console never starts a daemon. Under the mutant the test exists to catch, it cannot reach the stub, falls through to its own `startDaemon`, and overwrites `daemon.pid` with a dead pid — the test that proves the bug also damaged the thing the rest of the file is careful about. Controlled both ways: with the drift mutant applied the test now goes red AND the pid file's md5 is unchanged. The port scrape in the hook is anchored on `export const` again. Dropping the anchor was justified as surviving a reformat, and that reasoning was wrong — a reformat does not rewrite `export const NAME =`. What it actually bought was letting the FIRST match anywhere in the file win, comments included: a `// historical: DEFAULT_DAEMON_PORT = 9999` line above the declaration made the hook guard 9999 and stop guarding the real port. Controlled: that same line now yields 9444. Three comments that no longer described their code. `test-unit-run.mjs` had the previous round's two lines left verbatim above the block that replaced them, so the same sentence appeared twice. The hook still said the port "is overridable via DEV_DAEMON_PORT" — the exact semantics round 3 removed, since the set is now additive — and its "resolved lazily" note sat above `daemonPortsGuarded`, which is neither lazy nor cached; the laziness is in `devPorts`. Numeric line references (`:123`, `:128`) replaced with named ones. Both had already rotted by three lines. This is the same class as the SKILL.md citation fixed last round, and a line number in a comment will rot again — a quoted message will not. Named the residual hole rather than implying it away: the daemon enqueues INSIDE the response write, so the slot is taken before the client can observe it. Lose the response between those points and `accepted` is still false while the run is queued. That window needs an idempotency key on the enqueue, not a flag here. Not changed, deliberately. `.claude/**/*.mjs` fails `prettier --check` at the merge base as well as here, so it is pre-existing and reformatting it would rewrite files this change does not own. And `scripts/__tests__/*.ts` is typechecked by nothing — `tsconfig.json`'s `include` omits it — so this PR's "typecheck 0 errors" says nothing about the new test file's annotations. Both are stated rather than quietly folded in. 330 passed. Hook selftest green plain and under the override. Test file prettier-clean. |
||
|
Zachary Lowden
|
23cecb57c0 |
ci: run per-app typecheck scripts in CI (#4156)
* ci: run per-app typecheck scripts in CI
The root tsconfig.json include is:
scripts/local-dev/*.ts src packages/*/src tests .next/types/**/*.ts
No apps/ entry, so `pnpm run typecheck` does not reach any sibling app.
Each app defines its own typecheck script but none were run by any CI job.
Six of seven apps pass today; wire them up as steps in the existing `apps`
(App unit tests) job, which already has the workspace installed.
app script result
event-engine tsc --noEmit -p tsconfig... clean
notifications tsc --noEmit clean
orchestrator-gateway tsc --noEmit clean
storage tsc --noEmit clean
creator-studio svelte-check --tsconfig ./... clean
moderator svelte-check --tsconfig ./... clean
auth svelte-check --tsconfig ./... 2 errors + 1 warning
apps/auth is excluded from the CI wiring. Its pre-existing errors:
src/lib/server/auth/providers.ts:165
Parameter 'p' implicitly has an 'any' type.
src/lib/server/auth/__tests__/establish-session.test.ts:100
Property '_store' does not exist on type 'nev
src/routes/login/+page.svelte:28
state_referenced_locally (warning)
Ticket: 868kt8pfu
* ci: make the app typecheck prove it ran, and report every failing app
Review follow-up on the six `pnpm --filter <pkg> run typecheck` steps. Two ways they could
report SUCCESS while checking nothing or hiding work, plus a blame problem.
🔴 `pnpm --filter <name> run <script>` EXITS 0 WHEN THE FILTER MATCHES NOTHING. Measured on
pnpm 10.28.1: a bogus package name prints "No projects matched the filters" and returns 0.
Six hardcoded package names are six chances for a rename to turn a gate into a green no-op
— the same shape as `prettier --check "$FILES"` reporting "All matched files use Prettier
code style!" across zero files. Nothing in the six steps could notice.
🔴 A NEW app under `apps/` is simply absent from a hardcoded list. The gate stays green and
the app is unchecked — this PR's own hole, reopened by the next person to add an app.
So the app set is now a LEDGER READ FROM DISK: every `apps/*` with a `typecheck` script must
run, each run must be proven to have selected a real package, and `auth`'s exemption is an
explicit entry that hard-errors if it goes stale. Adding an app wires it automatically. This
mirrors `scripts/ci/assert-workspace-suites-ran.mjs`, which exists for the same reason on
the vitest side.
Failures are COLLECTED rather than fatal on the first. Six sequential steps abort the job at
the first red app, so a shared type change breaking four of them reports one.
The job is renamed `App unit tests` -> `App unit tests + typecheck`. The typecheck steps were
added under the old name, so a type error would have rendered in the checks list as a failing
unit-test job, pointing the reader at the wrong suite. Safe: neither `main` nor `release` has
any required_status_checks — re-verified 2026-08-20 against the branch-protection API rather
than inherited from the note already in this file.
Kept in the `apps` job on purpose: `pnpm install` is the expensive part and this job has
already paid for it. A matrix would give parallel legs and per-app check names for the price
of six installs; the rename plus collected failures buys most of the legibility for free.
Eight guard tests in `scripts/__tests__/typecheck-apps.test.ts`, each driven against a stub
apps/ tree with a fake `pnpm` on PATH so the no-match and failure paths are exercised for
real: happy path, stale filter caught, red app, failures collected not masked, empty
discovery refused, new app auto-wired, stale exclusion hard-errors, auth excluded while the
rest still run. All 8 pass; the first draft had six failing for the wrong reason (fixtures
omitted `auth`, so the stale-exclusion guard fired first) — fixtures fixed, not the guard.
Two things deliberately NOT changed:
- The Svelte apps' bare `typecheck` script is fine as-is. Their tsconfig extends
`./.svelte-kit/tsconfig.json`, absent from a fresh checkout, which looks like a vacuous
green waiting to happen. It is not: `"prepare": "svelte-kit sync"` runs during
`pnpm install`. And if it ever stops, svelte-check FAILS LOUDLY rather than passing —
verified by moving `.svelte-kit` aside, which gave exit 1, "Cannot read file
.svelte-kit/tsconfig.json", `1 FILES 1 ERRORS`. An earlier review comment of mine claimed
it would go silently vacuous; that was wrong, and switching to `check` is unnecessary.
- `scripts/__tests__/` is outside every typecheck program (root `include` carries only
`scripts/local-dev/*.ts`), so this new test file is unchecked — as are the 12 already
there. Not introduced here; it is the `scripts/` half of this ticket's widening.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* style: prettier the two new files
The `Prettier (added files)` step is BLOCKING by design — a new file has no pre-existing
findings, so holding it to the rules is free. Both new files failed it and I did not run the
formatter before pushing. Reproduced locally with `prettier --list-different` (both listed),
fixed with `--write`, and re-ran the 8 guard tests after the reformat: still 8/8, so the
formatting did not disturb the fixture strings or the fake-pnpm heredocs.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
---------
Co-authored-by: DevPod Agent <agent@devpod.local>
Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
||
|
Zachary Lowden
|
481582d969 |
flake: own the dev toolchain, guard the pins, one command to a running app (#4107)
* feat(flake): make the Nix flake own the dev toolchain, and add one command to start The flake shipped nodejs_22 while package.json declares engines.node ">=24.0.0 <25", .nvmrc pins 24.19.0 and the Dockerfile builds production on node:24.19.0-alpine3.24. A NixOS developer was running a major the repo does not support, and nothing said so. Toolchain: - node and pnpm are now DERIVED from .nvmrc and package.json's packageManager rather than named twice. .nvmrc is treated as the authority because it is what every workflow's actions/setup-node reads and what the Dockerfile tracks. - flake.lock moved 2026-04-23 -> 2026-08-18 (117 days). At that rev nodejs_24 is exactly 24.19.0, which is what made agreeing with .nvmrc possible at all. - pnpm now comes from `pnpm_10`, not the unversioned `pkgs.pnpm`. At the new rev the unversioned attribute resolves to 11.21.0 -- a major bump that rewrites pnpm-lock.yaml -- so this bump would otherwise have shipped pnpm 11 to every dev shell silently. - postgresql_16 -> postgresql_17, matching the primary `db` container. The postgres/redis/clickhouse entries are CLIENTS for the compose-hosted servers; that is now stated in the file instead of left to be guessed. - npm_config_manage_package_manager_versions=false. Measured: without it, pnpm downloads and re-execs the exact version from the packageManager field, so the flake's pnpm pin was being defeated at runtime (`pnpm --version` returns 10.28.1 with the var unset, 10.34.5 with it set). Guards (`nix flake check`, 4 checks): - toolchain-pins: the flake's node must satisfy engines.node and equal .nvmrc, and its pnpm must share a major with packageManager. Deliberately does NOT re-check the .nvmrc/Dockerfile/engines triangle -- node-version-consistency.test.ts already owns that, and a predicate open-coded twice starts disagreeing. - prisma-pin: re-derives the resolved @prisma/client AND its engine commit from pnpm-lock.yaml and compares them to the values flake.nix hardcodes. These were correct but unguarded: package.json declares `^6.3.0`, a caret range, so a routine lockfile refresh moves the client while the flake's engines stay put, and the failure surfaces at runtime in every dev shell. - pin-guards-selftest: breaks each pin on purpose and requires the guard that owns it to fire while the others stay silent. - dev-scripts: builds the shell entrypoints, which is what runs their shellcheck. (`nix flake check` builds checks.* but only EVALUATES packages.*, measured.) Entrypoints: - `nix run .#dev` - docker preflight, submodule, .env.development, compose up, wait for postgres, pnpm install, then `next dev`. Every step idempotent and non-destructive; migrations and seeding stay opt-in. - `nix run .#dev-server` - runs the dev-server CLI on the flake's node. The daemon re-execs itself with process.execPath, so whichever node starts the CLI is the node it runs on until it is restarted. - `nix run .#doctor` - the same pin checks against the working tree. Compose project is pinned to `civitai` so every worktree shares the one local stack instead of each spawning a duplicate that fails on the port binds. * fix(flake): give `nix run` the same env as the dev shell, not just the shell Found by running the bootstrap on a genuinely clean worktree rather than reasoning about it. `mkShell`'s `env` applies to `nix develop` only, so both values it carried were absent from `nix run .#dev`: - `pnpm install`'s postinstall runs `prisma generate`. Without PRISMA_QUERY_ENGINE_LIBRARY et al, prisma tried to fetch an engine for platform `linux-nixos` and the bootstrap died on `404 ... /linux-nixos/libquery_engine.so.node.sha256`. - pnpm re-execed itself as 10.28.1 from the packageManager field even though PATH pointed at the flake's 10.34.5, so the app reported a pnpm the flake had not pinned. The env is now one attrset (`devEnv`) rendered two ways: `env` for the shell and an `export` preamble for the apps, so they cannot drift. `nix run .#dev-server` gets it too -- the daemon runs `pnpm install` / `db:generate` on its own when it sees the lockfile move, which would have hit the identical 404. * docs: describe the toolchain the repo actually has, not the one it used to Every claim below was checked against the code before rewriting, and the measurements are quoted where they are load-bearing. README.md - "Node.js (version 20 or later)" -> 24.19.0, with .nvmrc named as the authority. - `make init` was DEAD, not merely awkward: it ran `npm i`, and package.json's `preinstall` runs `only-allow pnpm`, which exits 1 under an npm user agent (measured, with the pnpm-user-agent control exiting 0). Both bootstrap paths the README offered went through it. - MinIO console is on :9001, not :9000 (:9000 is the S3 API). The instructions sent people to the wrong port to mint the keys the next step needs. - `git submodule update --recursive` -> `--init`; without `--init` it is a no-op on a fresh clone, which is precisely when it is being run. - Data Migrations step 1 pointed at `schema.prisma`, which is gitignored and regenerated from `schema.full.prisma` on every `db:generate`, so edits to it were silently discarded. - Adds the Nix path (`nix run .#dev`) and a real non-Nix sequence. - engines.node is ADVISORY, stated plainly: pnpm 10.34.5 under node 26.7.0 against ">=24.0.0 <25" prints `WARN Unsupported engine` and exits 0. An earlier draft of this very README claimed it refuses. It does not, and that is the reason the drift survived so long. Makefile - `npm i` -> `pnpm install` (see above). `npm-install` kept as an alias. - `gen-prisma` ran a bare `prisma generate`, which reads the gitignored slim schema that does not exist yet on a fresh clone; now `pnpm run db:generate`, which generates it first. - `dev` ran bare `cross-env`/`next`, requiring the caller to put node_modules/.bin on PATH by hand; now via `pnpm exec`. - `docker-compose` (EOL v1) -> `docker compose`. - COMPOSE_PROJECT_NAME pinned to `civitai`. Reproduced first: `make start` in a worktree died with `Bind for :::15434 failed: port is already allocated` because compose named the project after the directory. .envrc.example (new, tracked) + .gitignore - `.env*` matched `.envrc` too, so nothing tracked in the repo mentioned the flake at all -- the only reference was a line in CLAUDE.md filed under worktree hygiene. Placeholders only; the real .envrc stays ignored. .claude/skills/dev-server/SKILL.md - The skill said nothing about node. The daemon is spawned with `process.execPath` (cli.mjs:66, console.mjs:87) and hands its env to every `next dev` it supervises, so the first shell to run a CLI verb decides the node for everything, indefinitely. Measured on this box: daemon on 26.7.0, with no pnpm on PATH at all. Documents `nix run .#dev-server` and how to check. - `npm run dev:daemon` -> `pnpm run dev:daemon`, in a repo that bans npm. src/__tests__/node-version-consistency.test.ts - Comment-only. It said flake.nix "is on a different major" and could not be aligned because the pinned nixpkgs had no Node 24 this new. Both halves are now false, and a comment a maintainer might act on is worth correcting. Also: docs/pnpm-migration.md's "Node.js 18.x or later"; the generated-header line in scripts/generate-slim-schema.js telling readers to run `npm run db:generate`; CLAUDE.md's local-dev section (no node version, no services) and its stale "flake's 22.22.2" figure. NOT changed, because it could not be exercised here: the devcontainer pins typescript-node:1-22 (Node 22, outside engines.node). Flagged in README with the tag to use -- there is no `1-24`, the template major moved on, so `3-24`. * docs(flake): the four postgres containers are not all one version prisma-pit and db are postgres 17; notification-db and logical-db are 15. The comment justifying postgresql_17 read as though they were uniform, which would have made the next person's version decision from the wrong premise. * docs: keep the non-Nix path the default, demote the flake to optional The flake is used by one maintainer. Everyone else uses Docker + nvm, and that has to stay the path a contributor lands on. The previous revision inverted that: README's Installation section led with "With Nix (recommended...)" and titled the standard path "Without Nix" — framing the majority workflow as the fallback. CLAUDE.md opened "From nothing to a running app, one command:" with `nix run .#dev`, and the dev-server skill led its fix with "Start it through the flake and this cannot happen". None of that made Nix *required* — verified: `.github/` is untouched by this branch, no workflow references Nix (the apparent hits are substrings of `eslint-unix.json` and `--format unix`), and `nix flake check` is not wired to any CI gate. It was purely an ordering-and-emphasis problem, which is the kind that costs a new contributor twenty minutes before they find the section that applies to them. Changes, all editorial: - README: `#### Standard setup` now precedes `#### Optional: Nix flake`, and the Nix section opens with a blockquote saying it is not the supported default, that nothing requires it, and why it exists at all (NixOS has no published `linux-nixos` Prisma engine, so a flake is the practical way to work there). The signals/buzz instructions lead with `docker compose up -d` and mention `nix run .#dev -- --full` parenthetically. - CLAUDE.md: the bootstrap block is now the nvm/docker sequence, labelled as the default path, with the flake shown after it as NixOS-only and explicitly flagged as something not to assume a contributor has. The dev-server step no longer instructs going through `nix run .#dev-server`; it states the requirement (a shell whose node matches `.nvmrc`) and notes the flake does that for you on NixOS. - dev-server SKILL.md: the fix is now stated setup-agnostically — start the daemon from a shell whose node matches `.nvmrc` with pnpm on PATH, which `nvm use` gives you — with the flake wrapper presented as the optional NixOS convenience, and an explicit note that nothing in the document depends on Nix. No behaviour, tooling or gate changes: the Makefile, flake, guards and their tests are untouched by this commit. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
Zachary Lowden
|
f419a64461 |
chore(deps): delete the inert next@16.3.1 patch and correct the comments about it (#4086)
Next 16.3.1 is the first release containing upstream's SVG-loader fix (vercel/next.js#96681) — it ships 'VipsForeignLoadSvg' in the sharp.unblock list itself, in both the CJS and ESM image-optimizer copies. Our local patch was therefore inert: the installed files are byte-identical (sha256) to the published tarball, and the patch does not even apply (its hunk context wants Png immediately followed by Tiff; 16.3.1 has Svg between them). It looked healthy because pnpm silently no-ops an already-applied patch — install exits 0, prints no warning, and still creates a patch_hash= virtual-store directory. Deletes the patch and its patchedDependencies entry (@mantine/hooks untouched), regenerates the lockfile, and corrects three comments that asserted upstream still lacked the entry. The CI guard stays: it asserts the outcome (the installed Next unblocks the SVG loader), so it keeps protecting against a future Next regressing this the way 16.3.0 did. Verified post-removal: guard green, still goes red when the loader entry is stripped from both installed copies, and /api/og renders PNGs on both paths on the preview build. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
Zachary Lowden
|
7826ba9bfb |
fix(dev-server): one exit-code rule for both test-queue waiters, and a log that admits when it was clipped (#4102)
`exitCodeFor` in the queue module exists to stop a signal-killed run reporting a shell 255 -- its own comment says so. `test wait` used it; the wrapper behind `pnpm run test:unit:run` kept a second copy of the rule, `state.exitCode || 1`, which passes the recorded -1 straight to `process.exit`. Measured through a real daemon: a run cancelled mid-flight exited 255, where `test wait` on the same run exited 1. The copy is deleted rather than corrected -- the wrapper now imports `exitCodeFor` and uses it. The queue also kept the last 2000 lines of a run and dropped the rest in silence, so a fragment was byte-for-byte indistinguishable from a whole log. Measured: a child writing 5000 lines produced 1998 through `test wait` with nothing said. The window is unchanged; the drop is now counted (`logsDropped` on the run view, and `dropped` on the log response for callers that fetch logs directly) and both waiters name the number at the moment the verdict lands. `scripts/test-unit-run.mjs` now honours `DEV_DAEMON_PORT` like the CLI already did. That is what makes the verdict testable at all: with the port hardcoded there was no way to stand a stub daemon beside the shared one, which is why a decision this load-bearing had no test and was free to drift. Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
Zachary Lowden
|
c78f85dd5a |
fix(deps): Next 16.3.0 → 16.3.1, and make the compiled-branch gate hard (#3983) (#4075)
* fix(deps): Next 16.3.0 -> 16.3.1, and make the compiled-branch gate hard (#3983) This is the fix for #3983. The defect was in the bundler, not in our source. Turbopack's value analyzer in 16.3.0 models a bare `return someAsyncFn()` tail call as `Promise<Promise<T>>`. That is always truthy, so a caller that `await`s it is analysed as always-true and every statement after the resulting conditional is eliminated as dead code. `isAppListingsEnabled` ends in exactly that shape, which is why `resolveStoreVisibilityScopeUninstrumented` lost two of its three returns, fell off the end, and produced `undefined` for every non-privileged caller — served as the whole catalog on one read path (`?? 'full'`) and as an empty store on the other (`?? 'none'`). Upstream: vercel/next.js#96601 "[turbopack] Collapse nested promises in the analyzer", backported as #96675, shipped in 16.3.1. MEASURED, not inferred. Two production builds of THIS commit on one machine, same Node 24.19.0, differing only in the pinned Next: 16.3.0 async function S(e){if(await p(e))return"full"} 16.3.1 async function w(e){return await c(e)?"full":await y(e)?"public-external":"none"} Both read out of the emitted `.next/server` chunks by source-map attribution and identified by their source neighbour `STORE_SCOPE_FLAGS`, never by minified name. Note the fixed form is a TERNARY — `grep 'return"public-external"'` returns zero on the FIXED build too, which is why the gate reads source maps. `package.json` already allowed 16.3.1 (`^16.3.0`); only the lockfile pinned 16.3.0, so the substance here is the lockfile. The floor is raised to `^16.3.1` so a fresh resolution cannot land back on the broken compiler. `patches/next@…` is renamed and its `patchedDependencies` key updated — that patch is the unrelated libvips/SVG one-liner (vercel/next.js#96681), it still applies cleanly, and 16.3.1 still does not carry the loader entry upstream, so it stays. `--warn-only` is removed from `scripts/assert-compiled-branches.mjs` in the same commit. It existed only because the 16.3.0 build genuinely violated the gate, and a permanently-red gate trains everyone to click through. Keeping the bump and the strictness atomic means the gate's strictness always matches the toolchain: a revert of the bump turns it red instead of silently passing. Verified on this commit: - gate exit 0 (hard, no --warn-only) against the 16.3.1 build - gate exit 1 against a 16.3.0 build of the same tree — watched red - `scripts/ci/assert-next-svg-patch-applied.mjs` OK on both installed copies - unit suite 1149 files / 18,123 tests passed, 0 failed Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(build): ship @swc/helpers' module-sync branch into the standalone image (#3983) The bump built clean, passed every source-level gate — unit suites, typecheck, ESLint + Prettier, schema drift, the event-engine pin, and the now-hard compiled-branch gate — and the container could not boot: Error: Cannot find module '.../@swc/helpers/esm/_interop_require_default.js' at ... next/dist/server/require-hook.js code: 'MODULE_NOT_FOUND' Same shape as the defect this PR exists to fix: a correct source tree producing a broken artefact, invisible to everything that reads source. ROOT CAUSE, measured on the published artefact rather than inferred. `output: 'standalone'` does not ship node_modules; it ships the subset @vercel/nft traced. nft resolves a bare specifier under the `require`/`default` conditions. Node (>= 22.10) additionally honours `module-sync` for a CJS `require`. When a package's `exports` map points those two at different files, the build traces one and the running process asks for the other. next/dist/shared/lib/constants.js does `require('@swc/helpers/_/_interop_require_default')`, reached from the generated server.js via `next` -> config.js -> constants.js, i.e. before any application code. The relevant delta is not next itself but next's own dependency: next 16.3.0 next 16.3.1 @swc/helpers 0.5.15 0.5.23 ./_/_interop_require_default {import,default} {module-sync,webpack,import,default} require.resolve() under CJS cjs/...cjs esm/...js Both resolutions were RUN, not reasoned about. nft still traced the cjs file, so the published image carried that package as exactly cjs/_interop_require_default.cjs, cjs/_interop_require_wildcard.cjs and package.json — no esm/ directory at all. Adding only the missing esm/ directory to that exact image, nothing else changed, boots it: "Next.js 16.3.1 ... Ready". FIX. `outputFileTracingIncludes` force-includes BOTH condition branches of EVERY installed @swc/helpers copy — not the one file missing today, because which helper Next requires and which branch each resolver picks are upstream details that move. Globs are version- and hash-agnostic (`@swc+helpers@*`), plus a flat form for a hoisted layout. ~950 KB per copy. Verified on a local production build of this commit: both copies land in .next/standalone with complete esm/ (108 and 105 files) and cjs/, and next's virtual store links the 0.5.23 copy. Attached to three existing API-route keys rather than a `'**'` key. copyTracedFiles unions every entry's traced set into the single .next/standalone node_modules, so one entry carrying it is enough, while `'**'` would make all 572 entries read/parse/rewrite their .nft.json concurrently — 826 MB of JSON in one Promise.all — on a build already tuned against OOM. GATE, because a glob is a silent no-op once it stops matching. scripts/ci/assert-standalone-boot-graph.mjs runs in the Dockerfile's RUNNER stage: the first gate in that file to run against the runtime filesystem rather than the build tree, and the only one that can see this class of defect. It reads the GENERATED server.js for the specifiers that process requires at module scope and loads them in a child rooted at the shipped tree — no package, version, virtual-store path or patch hash hardcoded, so it keeps covering this after the next bump. Exit 2, never 0, when it cannot observe its input. It must run in the runner and not the builder: /app there is byte-for-byte what ships, whereas the builder's complete node_modules sits above .next/standalone on the resolution path and can satisfy a require the image cannot. Watched red and green on real artefacts, not only fixtures: - exit 1 with this exact MODULE_NOT_FOUND against the published broken image; - exit 0 against the same image with only the esm/ directory added; - exit 0 against the local production build of this commit, isolated from any parent node_modules; - exit 1 again after deleting exactly esm/_interop_require_default.js from that same local build. src/tests/build/standalone-boot-graph.test.ts pins the MECHANISM (a module-sync/default split with only the default branch present) rather than the package, and all 5 of its cases were watched to fail against a neutered gate. NOT VERIFIED. Nothing about production: this is only true of production once it merges, is promoted main -> release, is built and is serving. The gate covers the ENTRYPOINT's require graph; route chunks load lazily, so a condition mismatch reachable only from a route would still surface at request time. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * chore: retrigger preview The previous preview run (pr-preview-4075-b2wnw) never scheduled: its build-image and typecheck pods sat Pending with ExceededNodeResources for 82 minutes and the run hit the 1h30m PipelineRunTimeout. No verdict was produced — this was build-pool capacity contention, not a code failure. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
briant
|
394b4bc806 | migrate retool endpoints to moderator endpoints | ||
|
Zachary Lowden
|
8dd728eabe |
feat(build): assert security-relevant branches survive into the compiled output (#3983 detection) (#4068)
* feat(build): assert security-relevant branches survive into the compiled output
Release 5.1.18 shipped `resolveStoreVisibilityScopeUninstrumented` as
async function S(e){if(await p(e))return"full"}
Two of its three `return`s were absent from the emitted server chunk, so the
function fell off the end and produced `undefined` for every non-privileged
caller. One read path defaulted that to `'full'` and served the whole App-store
catalog to anonymous callers; the other defaulted the same missing value to
`'none'` and showed the intended cohort an empty store (#3983).
The TypeScript is correct — that is the whole problem. A 75-test unit suite, an
integration suite driving the real feature-flag client, and four rounds of
review were all structurally incapable of seeing it, because every one of them
exercises the source. Nothing looked at the artefact.
This adds a gate that looks at the artefact. It reads the emitted `.js.map`
`mappings` and asserts that each watched fail-closed branch still has a
representation in the output, attributing code to source modules by source map
rather than by grepping minified JS — minified names are per-chunk, the module
is inlined into 234 chunks, the literal `"public-external"` appears ~481 times
in the build without ever being a return, and the flag name
`app-listings-public-external` contains that literal as a substring.
Every entry carries a positive control: a branch known to survive that must also
be mapped. If the control is missing the gate exits 2 ("cannot observe") instead
of exit 1 ("violation"), so a build that simply did not emit the module is never
reported as a pile of violations.
Wired into the Dockerfile beside `check-server-graph-singletons.mjs`, and for
the same reason — `.next` exists in that stage, so there is no second build.
🔴 It runs `--warn-only` for now, because the underlying bundler defect is NOT
fixed: `main` and `release` carry byte-identical source for that function and
both emit it truncated, so a hard gate would fail every production build today.
`--warn-only` does not downgrade exit 2. Removing that flag is the definition of
done for #3983.
Verification: 13 cases, each asserting its own failure branch's specific message
and exit code, driven over synthetic `.next` trees with real base64-VLQ maps
encoded by an independent implementation. Six mutations of the gate were each
killed by their own case — one of them (breaking the VLQ sign branch) initially
SURVIVED, because every fixture listed source lines in ascending order and so
never produced a negative delta; the reordered-segments case was added to close
that. The gate was also run against the real 5.1.18 server artefact, where it
correctly reports both missing branches.
* style(build): prettier the compiled-branch gate + watchlist
|
||
|
Justin Maier
|
98f42aab65 |
fix(3d-models): correct the cutover constant and drop the superseded backfill (#4054)
* fix(3d-models): correct the cutover constant and drop the superseded backfill
The constant said 2026-08-18, picked by hand before the deploy. Tracking actually
started 2026-08-17 17:21:31 UTC, so the Creator Studio would have marked the
boundary a day late. Now read off production, with a note that the day is MIXED -
backfilled before 17:21:31, live beacons after - so a consumer marks that day and
not the one after it.
Removes scripts/oneoffs/backfill-model3d-views.{ts,helpers.ts} and its test. The
backfill now lives in civitai-scripts (backfill/model3d-views.js), and the copy
here still carried the design that shipped wrong: an --until DATE cannot express
the real boundary, because tracking starts mid-day. Any date either drops that
day's pre-deploy views or double-counts its post-deploy ones - measured on the
real cutover, 1,488 views before the first beacon and 296 after. The replacement
derives the boundary as min(time) of the first tracked day, so the two sources
abut exactly and there is no value left to typo.
Leaving a second, wrong copy in this repo is worse than having none: it is the
one a reader finds by grepping.
The DDL file is kept as the record of what ran, now marked applied.
Backfill result, verified independently: 11,336 rows / 175,964 views across 476
models, 2026-06-19 to the cutover. The boundary day reads 1,650 = 162 beacons +
1,488 backfilled.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0125bNeEK6nqERsbALjiDaUS
* refactor(3d-models): keep the cutover constant hardcoded, trim comments
The constant stays and stays hardcoded - the backfill derives its own boundary
from data and never reads it, so its only job is captioning the Studio chart,
and a hand-set date is fine for that.
Corrected to 2026-08-17, the day tracking actually began (17:21:31 UTC). The
comment now says the thing a reader needs and nothing else: the two spans are
different measurements, page loads ran ~1.5x the beacons that replaced them, so
a chart crossing that day steps down for reasons unrelated to the creator.
SQL comments cut 60 lines to 19, keeping only what a future editor would get
wrong without them - that MODIFY COLUMN replaces rather than appends, that
entityType is a sorting key so renumbering means a table rewrite, and why the
MV goes last.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0125bNeEK6nqERsbALjiDaUS
---------
Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
||
|
Zachary Lowden
|
088a9d09b1 |
fix(release): refuse an app release from a branch behind its released history (#4046)
`release-app.mjs` derives the next version from apps/<app>/package.json on the
CURRENT branch. When that branch is behind the app's released history the tag it
computes does not continue that line, in one of two ways:
* it already exists -> `git tag` aborts, but only after the release commit has
been made, leaving a junk commit on the branch;
* it is a different line (a minor/major bump off a stale base) -> nothing
collides, the tag becomes the HIGHEST for that app, and the Flux ImagePolicy
selects the highest semver rather than the most recent push. That stale build
is then what production runs.
apps/moderator is in exactly this state, measured 2026-08-17: 0.0.1 on main,
0.0.26 live, all 26 releases cut from `moderator-app-pages` — 211 commits and
+38,630 lines that never merged to main. `pnpm release:moderator` from main
collides on the existing 0.0.2 and aborts; `release:moderator:minor` computes
0.1.0, which does NOT exist, and would deploy main's stale copy to production.
One command, no collision.
So this does NOT bump moderator's version to 0.0.26. That is the obvious fix and
it is the wrong one: it removes the collision that is currently the only brake,
while leaving the app itself 211 commits stale. The real remedy is to land the
branch; the guard is what makes the trap loud until someone does.
Checked after `git pull --rebase` so the tag list is current, and before
`npm version` so a refusal leaves the tree exactly as it found it.
Coverage: 19 tests. The version arithmetic is unit-tested, and — because a guard
nothing calls is not a guard — release-app.mjs is also driven end-to-end as a
real process against a throwaway git repo with a local bare remote (offline; no
tags or commits touch this repository).
Mutation-tested, 4 mutants, each the narrowest expression that can be wrong:
invert the behind-comparison -> 5 tests fail
lexicographic highest-tag compare -> 6 tests fail
guard computed but never acted on -> ONLY the 2 behavioural tests fail,
which is what pins reachability
drop the unparseable-tag skip -> exactly 1 test fails
Baseline restored green (19/19) after the battery.
NOTE: scripts/**/*.test.ts runs in the `unit` vitest project, which is
`continue-on-error: true` in lint.yml — so these tests run but cannot currently
fail CI. That is tracked separately (868kp7fdr); it does not make them useless,
it makes them a local and post-fix gate.
Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
||
|
Justin Maier
|
eb848582ef |
feat(3d-models): track Model3D views, with a pageViews backfill (#3997)
* feat(3d-models): track Model3D views, with a pageViews backfill 3D models had no entity view tracking: no TrackView on any /3d-models route, no Model3D arm in the ClickHouse enums, nothing in daily_views. The history exists in pageViews but nothing reads it. Adds one TrackView on the detail page, reusing the existing TrackView -> /api/internal/pulse -> Tracker.view() path. No new component, no new endpoint, no new rollup table, no materialized view: daily_views is already ORDER BY (entityType, entityId, createdDate), so a per-creator query over at most 539 ids is a primary-key prefix seek. Ownership resolves from Postgres. A view is one load of a model's public detail page. /edit and /reviews are excluded by name rather than by path shape, because /3d-models/481/edit and /3d-models/481/my-slug are structurally identical - an anchored id regex matches the edit page too. Verified in ClickHouse's own RE2 against 30 days of prod pageViews: 82,319 counted, 399 edit+reviews excluded, 21 junk excluded. The DDL is not applied. It must run after the comics DDL (PR #3993), and it restates all twelve enum arms because MODIFY COLUMN replaces an Enum8 rather than appending to it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0125bNeEK6nqERsbALjiDaUS * fix(3d-models): address adversarial + simplification review Backfill correctness: - The shared ClickHouse client sets wait_for_async_insert: 0, so insert() resolved before the rows were queryable and the read-back raced the flush - a successful run could report "boundary mismatch, found 0" and then be blocked from retrying by its own guard. Wait on this one insert. - Verify the whole written range (count + sum) instead of one boundary day. A partial write that lost June and July but landed the last day passed before; a boundary day with zero rows verified nothing at all. - Assert the cutover constant against live data: the first day `views` holds a Model3DView IS the cutover. Nothing tied the constant to the deploy, so a deploy landing a day early or late left a doubled or permanently empty day that nothing detected. - Derive the id-extraction pattern and the filter from one constant. They had to agree or extract() returns '' and toUInt32('') throws mid-query. - The documented `npx ts-node` command could not resolve the ~/ imports. DDL: - Say to apply before deploying the emitting code, and to run steps 1-3 without a pause. Between step 2 and step 3 a Model3D row can reach an MV whose declared header is still nine arms, and every view type on the site shares that insert path. - State what step 0 expects rather than asking the operator to confirm against nothing. - Note that metadata-only holds only because every existing name->index pair is preserved; entityType is a sorting key, so renumbering means a rewrite. Local dev ClickHouse (containers/clickhouse/docker-init/init.sh) declared the nine-arm enums, so a fresh container silently dropped every Model3DView row. Tests: delete two DETAIL_PREDICATE assertions that could not fail on a revert - both matched substrings present in the surviving half of the predicate. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0125bNeEK6nqERsbALjiDaUS * fix(3d-models): harden the cutover guard against incidental tracking rows Applying the enum DDL removed an accidental protection: before it, a dev server or preview deploy on this branch writing a Model3DView row was rejected by the nine-arm enum. Now it succeeds. Local dev points at the same ClickHouse as prod, so one developer opening a 3D model page writes a real row - and the cutover guard took a bare min(createdDate) over them, which would move the detected cutover back to that day and silently drop every day between it and the real one. The cutover is now the first day clearing a floor no incidental page load reaches (50 rows against ~2,700/day of real traffic). Days below it that fall before the cutover are reported rather than ignored silently. The mismatch error also prescribed a lossy repair as if it were the fix: the mismatched day is almost always partial, since tracking starts mid-day. It now names both options and says not to take the first by default. Also: - select_sequential_consistency on the readback. Prod is a two-replica SharedMergeTree behind a load balancer, so the insert and the read can land on different replicas; wait_for_async_insert guarantees the write committed, not that the next query sees it. A stale read reports a false failure and the pre-flight guard then blocks the retry. - Restore the Number() coercion dropped in the last commit. It was what kept the script from depending on a client-level output format setting. - --dry-run works again before the deploy, which the live check had broken. - Restore an assertion on the edit/reviews exclusion. The previous commit claimed both deleted tests were vacuous; only one was. The other failed on a real revert, and it was dropped because ID_PATTERN broke its regex, not because it caught nothing. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0125bNeEK6nqERsbALjiDaUS --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
Justin Maier
|
ebe058067f |
fix(test-perf): stop the mock-allowlist generator reading a directory (#3976)
`globSync('src/**/*.test.{ts,tsx}')` matches directories as well as files, and
vitest browser mode names each snapshot directory after its spec — so
`__screenshots__/AppListingCard.browser.test.tsx` is a DIRECTORY that matches,
and the readFileSync below it dies with EISDIR before the generator produces
anything.
Those directories are gitignored, so the generator works on a fresh checkout and
breaks permanently for anyone who has ever run a browser test locally — which
reads as "this script is broken for me specifically" rather than as a bug.
Filter to regular files. Verified against a tree with three such directories:
EISDIR before, `canonical 218 -> 218 files (0 migrated)` after.
Claude-Session: https://claude.ai/code/session_01KvXBiAVpWyhNS85tsBMuDU
Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
||
|
Justin Maier
|
533640662d |
perf(tests): services a-m onto the canonical shared mocks (127 of 129) (#3973)
* perf(tests): move services a-m onto the canonical shared mocks 70 of 129 files in the services `__tests__` a-m slice: 62 converted by scripts/test-perf/codemod-shared-mocks.mjs, 8 finished by hand where it refused one specifier and took another. The hand cases were a hand-rolled auto-vivifying Proxy standing in for dbRead/dbWrite, and a `safeError: (e) => e` identity stub the canonical registration replaces with the real implementation. Neither was asserted on; both were scaffolding to keep the import graph off real infra, which is what the canonical mocks do centrally. Eleven of the converted files carried a hand-written REDIS_KEYS subset that disagreed with the real table and now get the real constant. Every dropped literal appeared exactly once, inside its own factory: no test asserted on one and none used one to select a fixture branch, so the swap is invisible in both directions rather than merely silent. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(tests): repair two files the shared-mock conversion broke `collection-collaborator.service.test.ts` collected ZERO tests, losing all 51. The codemod lifted a default out of a `vi.hoisted` block body into module scope, but the value it referenced — `OWNER_ID` — was a local of that block, so the file threw at import. It reported as one failed suite with no failing tests, which is the silent class the collected-count diff exists to catch. The file already sets the same default in `beforeEach`, so the surviving copy sits with the other module-scope constants. `creator-program.service.test.ts` asserted on `cp:*` cache keys it had invented in its own factory; the real ones are `packed:caches:creator- program:*`. Three tests went red on the swap. They now name `REDIS_KEYS.CREATOR_PROGRAM.*` so the test cannot re-invent a key. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * test(redis): pin the wire values the services a-m tests re-invented Nine files in that slice carried a hand-written REDIS_KEYS subset whose values disagreed with the real table — `packed:caches:announcements` for a real `packed:caches:announcement`, `kill` for `system:blocks:emergency-kill-list`, `cp:*` for `packed:caches:creator-program:*`. Every one was used only inside the factory that defined it, so no assertion could see the divergence and none of them could go red. A key's wire value addresses live entries written by deployed code, so a rename orphans whatever sits under the old name. Pinning them here makes that a deliberate decision rather than something a test silently follows. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * chore(tests): regenerate the direct-mock allowlist after services a-m 345 -> 275 canonical files; the guard passes in both directions. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * perf(tests): split the single-client db aliases in services a-m Eleven files mocked `~/server/db/client` with one local object serving both `dbRead` and `dbWrite`, so a write could satisfy a read assertion. Each is bound to the one client the module under test actually spells for every call it makes, checked against the service source rather than guessed from the local's name — `mockDbRead` in two of the block-registry files turned out to be right for the wrong reason, and `mockDb` in `account-deletion-images` was serving writes. Two things the split surfaced, both kept rather than papered over: `get-engaged-models-by-ids` carried a real in-memory fixture on `resourceReview.findMany`, not a restated default, so binding the client alone dropped four tests' data and they went red. The fixture is now an explicit `mockImplementation` on the canonical node, and the four tests are the evidence it is load-bearing. `account-deletion-images` asserted on `'pending-restores'`, a key it had invented; the real one is `system:pending-image-restores`. It now names `REDIS_SYS_KEYS.SYSTEM.PENDING_IMAGE_RESTORES`. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * chore(tests): regenerate the allowlist after the alias split 275 -> 264 canonical files. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * perf(tests): migrate 18 more services a-m tests onto the canonical mocks Seven bound automatically where a balanced behaviour check cleared them, nine converted by hand where it refused, and two whose single aliased local was split by routing each call site to the client the service actually spells for it. Three cases the run settled rather than the diff: `minor-hash` wired two separately-declared spies in as client leaves, so deleting the client literal left both alive, armed by `beforeEach`, and connected to nothing — the primary-vs-replica re-read then saw the canonical `null` default. Behaviour-free is not the same as safe to delete; both names now point at the canonical nodes. `article-` and `bounty-locked-properties` spread their transaction object into `dbWrite`, so the canonical `$transaction` default preserves the identity they relied on. `model-appeal` did NOT — its transaction client was a separate object, and inheriting the default would let an in-transaction write satisfy an assertion that means "written outside the transaction". It keeps its own `tx`. `model-version.{donation-goals-cache,idempotent}` each carried a second direct mock of `~/server/redis/client` alongside the db one; both are taken, so neither file is left half-converted. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * chore(tests): regenerate the allowlist after the second services a-m batch Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * test-perf: add the client-local binder, with the refusal a run taught it Binds a test's own client locals to the canonical mock nodes for the shape the codemod refuses as "hoisted entry is not a bare vi.fn()". It refuses two things rather than guessing. A leaf carrying behaviour a canonical default does not cover, extracted with balanced parens — a lazy `vi.fn\([^;]*?\)` stops inside an arrow's own parameter list and hands the check a truncated call that reads as behaviour-free, which is how five files passed a check written to stop them. And a leaf that is a bare identifier declared elsewhere. The client literal is the only thing wiring such a spy to the module, so deleting it leaves the spy alive, still armed by `beforeEach`, connected to nothing, while the code under test reads the canonical default instead. That cost two tests in minor-hash.service.test.ts and no static check caught it — behaviour-free is not the same as safe to delete. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * docs(test-perf): per-case analysis of the parameterised-client alias split Six files in the services a-m slice mock one local as both db clients against services that choose their client at runtime, so the usual "grep the source for dbRead.model.method" routing finds nothing. Corrects my earlier report: these are NOT permanent hand-work. No test in the six passes a `db` option, so every case falls to its entry point's default, and those are fixed and listed. Four call sites default to dbWrite and one inverts to dbRead, which is the trap. Records the 14 negative assertions individually, because routing one to the client the code never touches makes it pass trivially — the failure a run cannot show — and names the one path I could not resolve rather than guessing it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * perf(tests): migrate two more services a-m tests, and harden the binder `model-version-count` named its spy with ES6 shorthand (`{ groupBy }`), which the binder read as an empty object and would have orphaned; bound by hand. `cover-image.service.logging` declares its `logToAxiom` rejection on the canonical node — the rejection IS the fixture there, since the tests exist to prove the `.catch()` on the best-effort log calls, so it is moved rather than dropped. Three refusals added to the binder, each after catching it wrong on a real file: - one local bound to both clients is an alias, and binding it here would silently pick whichever came last; - ES6 shorthand entries, per the orphaning above; - a factory with a BLOCK body, which opened on the block's brace, read an empty object, found no exports to object to, and deleted the whole factory with every local it named. It cleared two files that way and nothing caught it but reading the diff. It now auto-converts none of the 23 remaining files in this slice, which is the honest number: what is left is hand work. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * chore(tests): regenerate the allowlist Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * perf(tests): migrate six more services a-m tests onto the canonical mocks All six by hand; the binder refuses every remaining file in this slice. `commentsv2-owner` wired ONE `findUnique` spy into twelve tables, so a case naming `entityType: 'challenge'` could be satisfied by any of them. Each entity type now arms its own table, and the two "no lookup happened" assertions check all twelve rather than the single shared spy — stronger than what they replaced. Four of the twelve have a case; the other eight were in the fixture because the service can read them, not because anything checks that it does. `leaderboard-rank-showcase` records every statement it issues into an array that every assertion reads. The canonical `$executeRaw` returns 0 and records nothing, so the recorder is declared rather than inherited — dropping it would empty the array and throw, which is the loud case. `challenge-results-notification` and the two `contest-entry-*` gates were aliases hiding nothing: every path they assert on is spelled `dbRead` in the service. `bust-caches-for-posts-empty` replaces a permissive Proxy whose `apply` trap answered every un-stubbed call with `[]`; the two assertions in it are on `$queryRaw`, whose canonical default is also `[]`. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * chore(tests): regenerate the allowlist after batch 4 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * docs(test-perf): add lag-selected clients as a second unresolvable mechanism `getDbWithoutLag` picks dbRead or dbWrite from runtime replication state, so anything reached through it has no fixed client in the source. Five more files in the slice route reads that way. Under test it does not even behave as production does: REPLICATION_LAG_DELAY is a zod `.default(0)` key absent from TEST_ENV_DEFAULTS, so the canonical env reads it undefined, and `undefined <= 0` is false where `0 <= 0` is true — the staleness branch production never takes runs in every test that reaches it without stubbing db-lag-helpers. 73 defaulted keys, 59 absent from the table, 40 of those numeric or boolean. Also records why the already-converted files were safe: they happen to mock db-lag-helpers, which pins the client. The safe and unsafe conversions are separated by that coincidence, so the population at risk cannot be read off which files converted cleanly. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * docs(test-perf): the lag-selected bucket is two files, not five Corrects the section I added an hour ago. Three of the five resolve by the entry point the test imports: deleteVersionById uses dbWrite only, and neither model-file service mentions getDbWithoutLag at all. Only earlyAccessPurchase (via getVersionById) and publishModelVersionById genuinely defer the choice to runtime. The mistake is the one the section is about — reading BOTH off a whole-module scan and treating it as a property of the file. A module containing both spellings says nothing; the entry point the test calls is what decides. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * docs(test-perf): handover for the services a-m mock migration The state a successor cannot recover from the diff: the four remaining buckets with the discriminator for each rather than just their names, the binder's five refusals with the case that taught each one, and what to check on the seam-blocked files after that change lands — they will look convertible the moment it does. Also splits the post-tooling-scare audit into three categories rather than two: claims re-verified explicitly, claims structurally immune because they came from node rather than a shell pipe (an accident of format, not a method), and claims not re-derived at all, named individually. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * perf(tests): migrate four more services a-m tests onto the canonical mocks `engagement-toggle.idempotent` was an alias hiding a real read/write split: `toggleUserBountyEngagement` reads bountyEngagement on dbRead and writes it on dbWrite, through one spy, so a case asserting the read could have been satisfied by a write. Routed per call site from the two entry points the file imports — `toggleModelEngagement` does every modelEngagement operation on dbWrite, and the dbRead spelling elsewhere in user.service belongs to a function this test never calls. `block-registry.slot-reservation` keeps its distinct read/write locals, so the parameterised-client question does not arise. Its `redis.scanIterator` is an empty async generator consumed with `for await`; the canonical node would vivify it as a spy returning undefined, which throws rather than iterating, so the generator is declared explicitly. `file-download-lookup` carried a hand-written `safeError` under a comment claiming the global setup does not provide one. It does — the canonical registration spreads the real module. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * chore(tests): regenerate the allowlist after batch 5 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * docs(test-perf): correct the handover after batch 5, and add two mock-system constraints 111 of 129, not the 112 the placeholder claimed. The ordinary bucket grew from 3 to 7 because three files moved back out of "unresolvable" when I re-checked them; the seam-blocked three are now unblocked and are the easiest remaining work, with the check that is not "do they pass". Adds two constraints that belong to the mock system rather than to this slice: a canonical mock cannot statically import anything reading mocked env at module scope, because setup.ts loads it earlier than every hoisted factory it registers — and the failure is zero tests collected reported as one failed suite. And ~25 typecheck errors on this base are phantom, with the tell being that they sit in files you did not touch. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * perf(tests): migrate the three withSysReadDeadline seam tests onto the canonical mocks These three were blocked until `withSysReadDeadline` became a seam node on the canonical redis mock ( |
||
|
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
|
||
|
Justin Maier
|
7048aadfa1 |
perf(tests): split sharp-executing tests onto their own pool, pre-bundle five externals (#3960)
* perf(tests): route sharp-executing tests to their own forks project sharp 0.32.6's addon is not context-aware, so a worker_threads worker that has run a libvips operation segfaults at thread teardown - after the tests pass and the summary prints. That takes the whole run down with an exit code and no failing test. It is a race, not a threshold. On the six affected files, three repeats per width: vmThreads crashed 3/3 at 1 worker, 2/3 at 2, 1/3 at 3, 0/3 at 4; threads crashed at every width. A green run is not evidence of safety. Importing sharp is harmless; only executing an operation arms it. 100 test files carry sharp in their static closure and exactly six call it. That set was measured by aliasing sharp to a recording proxy and running all 100 under forks, not by grepping and not by a crash-scan - with a race, "ran alone and didn't crash" builds the list out of the files that got lucky. - unit -> the suite minus those six, pool unchanged (forks) - unit-native -> pool: forks pinned, including only those six unit deliberately does NOT move to threads. threads measured 1.04x at 4 workers and 0.94x at 16 - no win - and it segfaults mid-run on the full suite at roughly 1 in 4, after completing hundreds of files cleanly and with no unit-native file having run, so a second crasher exists that is not sharp and is not diagnosed. What the split buys is that the sharp crash is deterministic and gone, so anyone experimenting with --pool=threads no longer has to fight it too. Excluded from unit rather than merely claimed by unit-native, so that a run naming a sharp file under --project unit reports "No test files found" rather than running it on a thread pool if one is ever selected. Shared settings are hoisted into one object both projects spread, so they cannot drift apart. Every selector moves to a unit* project pattern. Verified by file count rather than by a green summary: vitest list over unit* is 1065 files and unit-native is 6, summing to the pre-split baseline. no-sharp-outside-native-project.test.ts is the positive control, since nothing else in the suite can notice this breaking: the realistic failure is a rename, which stops matching unit-native's include AND unit's exclude. Mutation-tested, not assumed green. * perf(tests): pre-bundle five externals nothing mocks Every test file gets a fresh module registry - under forks with isolate: true it is literally a fresh child process per file (N files at maxWorkers=1 give N distinct pids), so Node's module cache dies with it and each externalised package is imported cold once per file that reaches it. Pre-bundling collapses a package's many-hundred-file native load into one chunk, paid once per run. Full suite, control then treatment in one window: control wall 217.2s collect 4730s 1066 files 16787 tests 17 failed treatment wall 192.9s collect 4082s 1066 files 16787 tests 17 failed The failure SET is unchanged, diffed both directions - nothing appeared, nothing cleared. This alters timing, not behaviour. The effect tracks exposure, which is what separates it from ambient drift: reaches 0 of the 5 353 files collect 113s -> 105s -6.9% reaches 1 115 files collect 150s -> 103s -31.2% reaches 2 209 files collect 529s -> 373s -29.6% reaches 3 18 files collect 69s -> 52s -24.3% reaches 5 371 files collect 3869s -> 3449s -10.9% Exposure 5 shows the smallest percentage and the largest absolute saving (420s of 648s) because those are the heavyweight files: five packages are a small share of a 1,300-module closure and a large share of a small one. Percentage tracks share-of-closure, absolute tracks file weight. A control-vs-control run to size run-to-run drift directly was attempted and died to an unrelated crash, so the residual drift term is unmeasured and the figure above should be read as an upper bound. The list is confined to packages nothing mocks, and that is load-bearing. Pre-bundling wraps a package as a CJS-interop chunk, so a vi.mock factory returning only named exports stops satisfying its consumers - adding redis and the mocked aws-sdk clients takes four mock-holding files from 92 tests passing to 7 collected. The importOriginal form does not protect against this. Those three are worth ~275s more and need a default export added to six mock factories first; that is a separate change. The treatment paid its cold optimize pass inside the measured run - the shared .vite cache was not cleared - so the number is not flattered by a warm cache, and a fresh CI runner pays the same thing. * test(perf): pin the native project's pool independently of unit's The guard asserted unit ran on threads, which was the state the split shipped in for about ten minutes. It caught its own config change, which is the behaviour wanted, but the assertion was aimed at the wrong invariant: what must hold is that unit-native stays on a process-based pool whatever unit is pointed at, not that unit is on any particular one. * test(perf): acceptance harness for the six external-mock factories The change that brings redis and the aws-sdk clients into the pre-bundling safelist cannot be verified on a tree that does not enable the optimizer: without pre-bundling the package is not wrapped as a CJS-interop chunk, the missing default export never bites, and a green run proves only that the old config still works. This runs the six affected files under a config with all three candidates pre-bundled. Compares per-file collected counts rather than the total. s3-utils is 66 of the 106, so a sum of 40 could be one file collecting zero and still read as a partial pass. Negative control on the unchanged tree: 5 of 6 files collect 0, and the harness exits 1 naming each one. * docs(test-perf): record the measurement envelope this box imposes Two identical full runs, back to back, nothing changed between them, came out +20.5% apart on collect. That pair was contaminated, so it is not a drift figure - but it demonstrates the box can move further than most of the effects measured today, which means any comparison assembled from two windows is unreadable. Collects the methodology that follows: quote in-pair controls rather than cross-window deltas; a control group must be comparable in cost and not merely in count; a dose-response on an axis confounded with file cost is suggestive rather than conclusive; a crashed run's wall clock is not a fast run; and check for the workload rather than for the runtime when deciding the box is quiet. A clean drift pair still has not been taken and is the denominator for everything else here. * docs(test-perf): scout Bun and node:test as vitest replacements Recommendation is to stay on vitest, but the measurement overturns the cost model we spent the day optimising against. Same 84-module first-party graph: vitest collect 5298ms, bun 3.3ms, node+tsx 8.5ms. Whole-suite arithmetic gives vitest 10.2ms per static module-instance against 0.04ms for bun. The cost is the module runner, not the modules - which matches this morning's tracer result (569 module bodies in ~0.4s against a 25.4s import phase) and locates the time in vite-node's per-module fetch/instantiate rather than in compile-and-evaluate. Unrealisable, though. Bun cannot load any graph reaching the React/Next side - it dies resolving use-sidecar's package exports - so the numbers are measured on the light stratum only, which is the flattering-slice trap: 383 of 1065 files have no infra dependency and the largest of those is an 86-module closure. Module-scope env aborts the import under both runtimes, and cache-helpers hung past 300s under bun after the env gate was satisfied. The mock surface is the wall: 1053 of 1065 files import from vitest, 3883 vi.mock sites across 651 files, plus 8053 vi.fn and the fake-timer, spy and importActual surface. The canonical mock system, its guard, the allowlist ratchet, reporter.mjs, the dashboard and the queue integration are all vitest-shaped as well. Retarget rather than switch: if per-module cost is vite-node overhead, shrinking the graph attacks a term worth ~0.04ms of real work per module, and the leverage is in how many times a module is INSTANTIATED - which is what isolate:false removes. * docs(test-perf): retract the per-module ratio in the runner scouting It divided collect by inventory.json's static module counts, and that artifact was wrong by up to 75x and selectively so - it followed lazy dynamic import edges that never execute and ignored vi.mock factories. Honest suite union is 1321, not 3230. The per-file wall clock the recommendation rests on needs no denominator and is unaffected: same 84-module closure, vitest collect 5298ms against bun 3.3ms and node+tsx 8.5ms. So is the tracer result behind it - 569 module bodies executing in ~0.4s against a 25.4s import phase, measured with no static count at all. No counterpart figure is quoted for bun, because its denominator came from the same artifact. * docs(test-perf): scrub the remaining per-module claims from the runner scout Two survivors of the retraction: an 'orders of magnitude per module' headline and a stratum characterisation quoting closure sizes, both resting on the same broken counts. Restated against the per-file wall clock, which needs no denominator, and the observed hard failure, which is not a count. * docs(test-perf): correct the runner comparison to like-for-like The headline compared vitest's collect for a TEST FILE against a probe importing only the SOURCE module underneath it - a different and much smaller graph. That is where '~1600x' came from. Like-for-like, on the same 82-module test-file closure: vitest collect 5298ms, bun 259ms (median of 5, 250-262). ~20x, not three orders of magnitude. node+tsx cannot import a test file at all - 'Vitest cannot be imported in a CommonJS module using require()'. Per-module refit against aidan's honest closures.json (mode: 'real') joined to the pre-ctl full run: 1065 files, 104797 real module-instances, collect 4729s -> vitest 45.1 ms/module, independently agreeing with aidan's 43.6. bun 3.2 ms/module on the file both can load. The recommendation is unchanged and the mechanism finding is unchanged; the size of the gap was overstated. * docs(vitest): say why the unit projects set no per-project maxWorkers Per-project maxWorkers does apply at runtime, but two projects with different counts need different sequence.groupOrder values, and different groups run serially. For a 1059/6 split that trades the concurrency between them for a knob nobody needs - the six-file project would gate the other 1059 instead of filling spare capacity beside it. Currently reads as an omission, so a future reader adds one and loses concurrency without knowing they traded for it. * docs(test-perf): final form of the runner scouting result Leads with both corrections stated in place rather than silently edited out, and records that neither changed the recommendation. Promotes the cross-validation to a finding of its own: 45.1 ms per module-instance here against aidan's independent 43.6, from a different artifact by a different route. Two wrong denominators would not have agreed, so the pair is what licenses everything downstream that divides by a module count. Names what both errors had in common - each a denominator error producing a number right about the thing it measured and wrong about what that thing was. Checking two runtimes are comparable is not checking the two quantities are. * fix(test): pin unit-native's pool against a CLI --pool, and correct the project selector `unit-native`'s static `pool: 'forks'` loses to a CLI `--pool=threads`: resolveProjects builds cliOverrides from a list that includes `pool` and spreads it after options.test, so the flag wins. The six sharp-executing files would then follow `unit` onto a thread pool and segfault AFTER printing a green summary. configureVitest hooks run after resolveProjects(cliOptions), and getFilePoolName -- `browser.enabled ? 'browser' : project.config.pool` -- is what stamps each spec's pool, so re-asserting there outranks the flag. The other two readers of project.config.pool populate task metadata from the same field and cannot disagree with it. The comment already claimed this guarantee; without the plugin it was false, and false in the reassuring direction. Also corrects CLAUDE.md: the unit suite is two projects now, so `--project unit` silently runs 1059 of 1065 files and exits 0. Select it as `--project 'unit*'`, which is what package.json's own scripts already do. Bound: the pin covers `pool` and nothing else. isolate, fileParallelism, sequence, testTimeout and retry are on the same cliOverrides list and remain overridable. Not verified by a run -- the mechanism was read from vitest 4.0.18's cli-api chunk twice, independently, by two readers. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UtTG4QQR29eWf7kjM6HiLU --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
Justin Maier
|
f4dce6a3c9 |
perf(tests): cut test-body time in the five slowest unit-suite files (#3962)
* perf(tests): move the challenge job-lock wiring assertion off the ladder suite challenge-ladder.test.ts was the slowest file in the unit suite at 37.9s of test-body time, and 35.3s of that was a single test dynamically importing ~/server/jobs/daily-challenge-processing to read one job option. The file is otherwise pure arithmetic (collect 476ms). challenge-jobs-scale.test.ts already loads that module behind a mock preamble, so the wiring half of the assertion lands there at no marginal cost. The arithmetic half (lock < interval) stays beside the constants. challenge-ladder.test.ts 37875ms -> 427ms test-body; challenge-jobs-scale 29ms -> 17ms with its collect unchanged. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * perf(tests): stub model.service’s graph in the orphan-relations regression suite prisma-inconsistent-orphan-relations.test.ts loaded the real model.service from a beforeAll with a 120s timeout, which billed the whole graph to test-body time: 31963ms duration against 237ms collect in the full-suite baseline. Replaced the wholesale @prisma/client Proxy stub with the mock scaffold seven sibling model.service suites already use, and hoisted every dynamic import to module scope. The scaffold cuts the graph rather than relocating it: measured in one window, 19.6s wall / 19148ms test-body -> 7.2s wall / 9ms test-body. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * perf(tests): stub the lazy DB read in the challenge-category fallback tests The "preset fallback (no DB available)" tests left ~/server/db/client unmocked and relied on a real connection attempt failing. That cost ~4s per call — four calls, 18400ms of test-body time — and made the branch under test depend on what the box could reach rather than on the code. Stubbing findMany to reject drives the same catch, in 39ms. The old suite could not tell the failure path from a successful read of zero rows: merging presets with an empty row set gives the same answer, so flipping the stub from reject to resolve([]) left all 16 tests green. Added the two assertions that separate them — the failure path deliberately does not cache, the success path does — so the stub cannot quietly become the thing under test. 18400ms -> 39ms test-body; 16 tests -> 18. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * perf(tests): drive the s3 upload retries on a fake clock, and cover the backoff Six tests in s3-upload.store.test.ts waited out real retry sleeps: withRetries holds a flat second between the three /api/upload/complete attempts, so each of them cost ~3s of wall clock. 16358ms of test-body time, 17.6s wall. They now run through a runUpload() helper that advances a fake clock until the upload settles. The loop is bounded and throws rather than draining while timers happen to exist: at the moment the upload is handed back no timer exists yet (the first is scheduled after fetch('/api/upload') resolves), so a getTimerCount() guard exits before the run begins and the test hangs. Bounding it also means a retry loop that stops terminating fails with a message in 113ms instead of wedging the runner, which is the failure mode the fake would otherwise create. The change exposed a gap it then closes: nothing exercised the TRANSIENT part retry path. Making 429 and 5xx non-retryable left all 17 tests green, because covering it meant sitting through 1s + 2s + 4s + 8s of backoff. On the fake clock it is free, so the path and its MAX_PART_ATTEMPTS bound are now pinned, and getPartRetryDelay - whose policy was observable only as elapsed wall clock - gets real unit tests beside isTerminalCompleteStatus. 16358ms -> 36ms test-body. 17 tests -> 19, and upload-retry 10 -> 17. Also records why audit-matching-equivalence.test.ts must stay slow: its brute-force oracle is valuable because it is a copy, and an optimised copy is a second implementation you believe is equivalent. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * perf(tests): hoist the listing-asset service imports out of the test bodies listing-asset-upload-integrity.test.ts reached app-listing-assets.service, offsite-listing.service and stored-object-integrity through `await import()` inside its helpers, so the whole graph was billed to whichever test ran first: 6664ms of the file's 6992ms sat on one test, behind a `collect` of 561ms. This is a RELOCATION, not a cut. Test-body time drops 6992ms -> 773ms and collect rises 561ms -> 6140ms; wall clock is unchanged within this box's noise. It is worth doing anyway because a graph billed to `duration` is invisible to the lane that attacks import cost - the file reads as cheap to import and expensive to run, when the opposite is true. No mock scaffold added. The file is a seam test that deliberately runs the real persist and attach procs with only the backend accessor replaced, so cutting the graph would cost it the thing it exists to check. Verified the suite is still live after the move - mocks apply because vi.mock is hoisted above imports - by making classifyStoredObjectIntegrity always report a match: 11 tests fail across all three asset kinds, including "REFUSES the attach once the stored object is no longer the measured one". Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * test(gate): pin that a wrapper which never launches cannot pass The typecheck-tests gate was checked for the spawn-laundering class found in the packages suite, where execFile on an extensionless node_modules/.bin script never started on Windows and the ENOENT landed in the same `code` field as a real exit status - so a process that never ran reported as one that exited 2. It is not present, and structurally cannot be: the gate spawns process.execPath, which always exists, and passes the wrapper as an argument. A missing wrapper is therefore node's own exit 1 with no diagnostics, which the FAILURE-TO-RUN rule already owns. There is no resolved-binary lookup on this path to fail silently. Verified against the real gate: res.error is undefined, status 3, CANNOT MEASURE. Nothing pinned that, so this adds the case. It asserts the run is refused rather than which guard refuses it, which is deliberate: with the FAILURE-TO-RUN rule disabled the positive-control floor catches the same run at 0 test files against a floor of 844, and pinning one mechanism would make the test fail on a change that left the property intact. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * perf(tests): stop paying the full port-claim deadline twice Two cases in dev-server-port-reservation.test.ts pass claimPortForReuse a 5000ms timeout and a port that never frees, so each polls for the whole deadline before moving: 10.0s of the file's 10.6s, in two tests. The file's own header claimed the opposite - that a 1ms poll with a generous deadline keeps every case count-bound rather than clock-bound. That holds for the cases where the port FREES, where a probe mock ends the loop. It cannot hold where the port never frees, because expiry is the mechanism under test. Those two now take a 60ms deadline. Shortening it cannot make them flaky: the assertion is that the session moved, and the move is what happens once the deadline passes, however few probes fit inside it. The comment now says which cases are which. 10600ms -> 734ms test-body, 37 tests unchanged. Mutation-tested, both directions, at the new deadline: drop `session.port = moved` -> expected 3101 to be 3102 (68ms) make findAvailablePort ignore other sessions' reservations -> expected 3100 to be 3102, and expected 3100 to be 3103 (67ms, 71ms) Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * perf(tests): drive the reward batch-retry backoff on a fake clock `(e) rethrows on update failure in the batch process path` waited out updateBuzzEvents' real retry budget - 5 retries at a 500ms backoff - costing 2556ms of the file's 3837ms. The budget is the behaviour under test, so shrinking it would change what the test covers. The clock is driven instead, with a bounded loop and the assertion awaited afterwards, so a retry chain that stopped terminating fails on the rejection rather than spinning. 3837ms -> 1270ms test-body, 10 tests unchanged. Adds an attempt-count assertion, which pins the retry budget and doubles as the control on the fake clock: without it driving the backoff those attempts would not have happened. That assertion also corrected a wrong reading of my own. It pins the literal `5` that updateBuzzEvents passes to withRetries, NOT BATCH_RETRY_COUNT, which is addBuzzEvent's default and which this path never reads - dropping BATCH_RETRY_COUNT to 1 leaves the file green, dropping the literal gives "expected 6 times, but got 2 times". The comment records both so the next reader does not credit the constant with coverage it does not have. Mutation-tested: batch path swallows instead of rethrowing -> promise resolved "undefined" instead of rejecting updateBuzzEvents retry literal 5 -> 1 -> expected "vi.fn()" to be called 6 times, but got 2 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
Justin Maier
|
21ab65ba14 |
fix(tests): make the packages and unit suites readable on Windows (48 failures -> 0) (#3961)
* fix(tests): run the drift CLIs through node, not the POSIX bin shim `node_modules/.bin/tsx` is a shell script, so `execFile` on Windows failed ENOENT before any CLI started — 32 tests across the three schema-drift files, i.e. every case in `test:packages:run` that spawns a CLI, red on any Windows checkout. `execFile` reports that in the same `code` field as a real exit status, so they read `expected 'ENOENT' to be 2`: an assertion about the CLI's behaviour rather than about a CLI that never ran. Resolve tsx's own entry and spawn it under `process.execPath` instead, and throw when a spawn produced no numeric exit code so the two can never be confused again. Mutation-tested: dropping `process.exitCode` in gate-cli gives `expected +0 to be 1`, and dropping the `capturedAt` stamp in cli.ts gives `expected undefined to be truthy`. * fix(tests): give the source-scanning guards posix path identifiers Four drift guards build a repo-relative path with `path.relative()` and then use it as an IDENTIFIER — matched against `/`-separated literals, or split on `/`. On Windows `path.relative()` returns backslashes, so the match can never hit and 14 tests were red on any Windows checkout. Normalise separators at the point the path stops being a filesystem path; nothing asserted changes, and the paths handed to the filesystem are untouched. One of these did more than go red. In app-spend-tier-privilege the mismatched key is `alwaysDecode`, whose stated job is to read the publisher-facing modules unconditionally so that a renamed or deleted path is loud rather than a quietly vacuous pass. With backslash keys it never hit, so it force-read nothing and could not tell a stale path from a present one — the mechanism against a vacuous pass was itself vacuous. Any Windows checkout has been in that state since the guard was written. Mutation-tested, each fix separately: - rename a PUBLISHER_REACHABLE entry -> `blocks.router.RENAMED.ts was not read — is the path stale?: expected undefined to be defined` - drop 'generation-resources' from KNOWN_STATIC_ENDPOINT_SEGMENTS -> `expected '/api/v1/blocks/:seg' to be '/api/v1/blocks/generation-resources'` - drop the 'user-settings:write' label -> `expected [ 'user-settings:write' ] to deeply equal []` - change orchestrator-chat's wait to 60000 -> `expected 60000 to be less than or equal to 150`, and the ledger diff names `server/services/comics/orchestrator-chat.ts:60000` * fix(scripts): name emitted chunks with posix separators, and cover exit 137 The server-graph gate keyed its chunk map on `relative()` output, so on Windows every violation named `chunks\ssr\b.js`. That key is what the report prints, so it is a name, not a path: normalise it, and keep the absolute path beside it so reading a chunk never goes back through the key. `typecheck.test.ts` simulated an outside kill with SIGKILL, which Windows cannot deliver — the child exits 1 with `signal === null` and the wrapper correctly reports a generic crash instead. Skip that case there for the stated reason and add the other half of the same branch, `exit 137`, which is what a container actually reports and which runs everywhere. Mutation-tested: dropping `|| code === 137` from the wrapper's classifier fails the new case with `expected '...TYPECHECK CRASHED...' to contain 'killed from outside'`. The gate's own synthetic negative control covers the chunk name. * fix(tests): resolve tsx per call, read the walked path, name a signal kill Three review findings on this branch, all one-liners, all the same shape as the bug the branch fixes: a failure reported as something other than what it is. `tsx/cli` was resolved at MODULE scope. All three drift test files import that module, so a resolve that throws — a tsx release dropping the `./cli` export subpath, a partial install — would take all three down during module evaluation and each would collect ZERO tests while the failure count stayed 0. Resolving inside the call surfaces it as a test failure naming the module instead. Latent, not live: tsx 4.20.3 declares `./cli`. `app-spend-tier-privilege` now reads the absolute path the walk produced rather than `join(ROOT, <normalised key>)`. The key is an identifier; handing a posix-separated string back to the filesystem works today and would not under a `\?\` prefixed path. This is also what the PR body already claimed it did. `runTsxCli` announced a signal-killed process as "did not run", which sends the reader looking for a spawn failure. A signal kill did run. |
||
|
Justin Maier
|
93ee73cd84 |
test(perf): instrument the unit suite and track the isolation migration (#3957)
* test(perf): instrument the unit suite and track the isolation migration
The unit suite spends 81% of its worker time importing, and nothing in the repo
could say which modules or which files. This adds the measurement that answers it,
plus a dashboard so the isolation migration has a burn-down nobody has to maintain.
The finding that motivated the shape of it: traced at 1 worker, the module BODIES
of two of the heaviest test files total ~0.4s against a 25.4s import phase. The
cost is vite-node's per-module fetch, which under the `forks` pool is a
child-process IPC round trip per module per file. So cost is linear in module
COUNT, not module weight, and the static import graph is the right thing to rank by.
- graph.mjs static first-party import graph + vi.mock inventory
- reporter.mjs per-file collect/setup/test timings from any run
- bench.mjs fixed 90-file stratified yardstick, so two measurements taken
hours apart by different people are comparable
- sweep.mjs pool x isolation x worker-count matrix, run back to back
- trace-* module-execution tracer; counts what actually ran, which the
static graph cannot know (a vi.mock factory stops the real
module and its subtree from executing)
- why.mjs shortest import path between two modules
- dashboard.mjs builds .test-perf/dashboard.html from whatever is on disk
Output goes to .test-perf/, gitignored. Nothing here runs in CI or changes how
any suite executes.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* test(perf): add the per-worker union report, and record three dead ends
Under isolate:false a worker keeps one module registry, so its cost is the UNION
of what its files import rather than the sum. order.mjs reports that union, which
is the number that bounds what removing isolation can deliver: measured at ~36
seconds per worker to build, near-constant at 8 and at 24 workers, and a wall-clock
floor more workers cannot shrink.
Three things measured today that do NOT help, written down so the next person does
not pay for them again:
- Affinity file ordering. A greedy graph-similarity sequencer gave a mean per-worker
union of 1139 modules at 31 workers against alphabetical's 1084 - slightly worse.
Alphabetical already groups by directory and directory already correlates with the
graph. The sequencer is deleted; the measurement is kept.
- NODE_COMPILE_CACHE. Cold 26.8s, warm 51.4s, warm again 33.9s on the yardstick. The
cache filled (5.3MB) so it was active; vite-node does not evaluate through the
loader it covers.
- vmThreads. Two clean 90-file runs, then the five sharp-executing files crashed or
passed on identical input at 2 and 3 workers. It is a race, and CI's 4 vCPU
resolves to the width measured at 1-in-3 SIGSEGV.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* docs(test-perf): three measurement rules this project had to learn twice
- The 90-file yardstick understates isolate:false and cannot judge it. That flag
amortises the registry build across the files a worker runs, so its win scales
with files-per-worker: 90 files at 16 workers is ~6 each and measures 1.65x,
while 1065 files at 8 workers is ~133 each and measures 16x on the same phase.
- No --no-isolate number is quotable without a per-file collected count. Its damage
is not only failing assertions: files silently collect ZERO tests, and how many is
width-dependent (9 of 90 at forks/4, 14 of 90 at threads/4, 0 at threads/16, same
input). A summary line cannot show this.
- The noise floor on a shared box is +/-30%: one configuration measured 53.3s and
76.6s in a single session. Below ~20%, quote phase numbers or nothing.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* test(perf): show the guard allowlist as the migration burn-down
The dashboard was deriving migration status from the static vi.mock inventory,
which is an estimate. The authoritative number is the length of the guard's
allowlist: it ratchets in both directions, so a new direct mock fails and a
migrated file left on the list fails too, and it therefore cannot drift from what
the suite will actually accept.
Reads it from the working tree when present, otherwise from the branch carrying
it, so a dashboard built on main still shows the real number rather than nothing.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* docs(test-perf): name the baseline, retract the IPC mechanism, fix the yardstick's promise
Three defects found by an adversarial review of this PR, all in the class of
asserting something nobody re-checked.
- The README quoted one baseline while shipping an artifact with different
numbers for the same tree - 4565.5s against 5476.3s of import, 36% apart. The
tree did not change; the box did. All three measurements of `3863adcbb0` are
now tabulated with the rule that a main-relative figure must name the run it
was measured against, because that spread is the same size as several of the
effects being measured against it. The derived import share is 81% or 84.3%
depending which row you pick, and both are now stated.
- The per-module IPC mechanism was retracted in mail hours ago and left standing
here. The pool sweep refutes it: `threads` beat `vmThreads` while paying a cold
fetch, which shipping module source cannot explain. Now labelled inferred, with
the competing reading and the note that V8 compile time was never instrumented.
- The yardstick claimed it made measurements "hours apart" comparable, three
bullets above a +/-30% noise floor measured inside one session. It fixes what is
measured, not when. Also records that a null from a 90-of-1065 sample is not
evidence of no effect - two changes measured flat there were later shown real.
And the gitignore claim is now scoped to "by this change", since it is false on
main until this merges.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* fix(test-perf): count the modules a worker loads, not the ones a bundler compiles
The static graph over-counted a page-gate test by ~75x, which put four of the
cheapest files in the suite at the top of the closure ranking. Their measured
worker time ranks 202-572 of 1065.
Four independent causes, each removing modules the naive walk counted:
- lazy `import()` is not followed, EXCEPT from a test file itself. A
`dynamic(() => import())` in a page never runs; an `await import()` in a
test body is what the test exists to do. Collapsing the two makes those
files either the top of the ranking or 1 module each.
- a `vi.mock` factory without `importOriginal` truncates the subtree behind
it. The mocked module itself is still counted -- registering the mock is
what causes the transform -- and is counted even when nothing imports it.
- `import { type X } from` erases the statement. Only `import type {` was
handled, and the inline form is the common shape here.
- a line filter cannot strip a multi-line `import type`: it leaves
`} from '...'` behind, and IMPORT_RE's lazy `[\s\S]*?` glues that orphan
onto the previous import, inventing an edge. Stripped as statements now.
Plus `event-engine-common` in SRC_DIRS -- a submodule imported by relative
path from src/server/services, so its modules and the civitai-db-queries
files reachable only through it were invisible.
Validated by diffing the model against a transform-hook trace as SETS, not
counts: 3 of 5 files exact with empty diffs both ways, 16 modules of
symmetric error across 691 traced (2.32%). Counts alone agreed often enough
to hide two of the four causes -- a count agreeing is not the rule agreeing.
`graphModules` is now the honest count and stays the default field, so
dashboard.mjs is corrected without a change of its own; `graphModulesRaw`
keeps the bundler view. closures.json gains `mode: 'real'` and a note
describing the truncation rules, so a consumer can refuse a naive one.
Also flags the two order.mjs union figures as pre-honest levels needing a
re-run; the ordering conclusion is a ratio and survives.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* fix(test-perf): stop the type-statement stripper eating real code, and record maxWorkers honestly
Three review findings, all mine.
1. graph.mjs -- the type-statement stripper swallowed spans of real code.
`TYPE_STATEMENT_RE` also started matching on a bodyless `export type X = ...`
(no `from`), and the lazy `[\s\S]*?from '...'` then scanned FORWARD across the
file to the next `from '...'`-shaped text -- inside comments and strings
included -- deleting every statement in between before IMPORT_RE saw them.
src/server/common/enums.ts lost a real `@civitai/notifications/constants`
edge across a 7,881-character span.
The gap is now tempered: between `type` and `from` a real type-import
statement holds only a binding clause, so it may not cross `;`, `=`, or
another `import`/`export` keyword. Measured across 5,294 files, spans of
more than 12 lines fall from 73 to 28, and every one of the 28 remaining is
a genuinely long multi-line type-import list. Re-validated against the
transform-hook traces: symmetric error 2.32% -> 2.03% over 691 traced
modules, so the fix is strictly in the right direction.
Direction of the bug was UNDER-count, and `graphModules` is the ranking key
for the dashboard, order.mjs and bench.mjs --make-subset. The earlier
validation did not cover it: 5 files of 1,065, none of the affected ones.
2. reporter.mjs -- the one unguarded filesystem call. It sits inside an awaited
vitest lifecycle hook, so a throw (read-only cwd, `.test-perf` existing as a
file, ENOSPC, an AV EPERM) would red a run whose tests all passed, and the
failure would be attributed to the code under test. A measurement tool must
never be able to fail a suite; losing the recording is the correct trade.
3. reporter.mjs -- `maxWorkers` is not on vitest 4's `ctx.config`. Verified by
execution: the emitted config was `{isolate, pool, argv}`. Every run ever
recorded therefore stored null, and dashboard.mjs rendered all of them as
"(default) workers" -- claiming a fact nobody measured. Recovered from argv
(`--max-workers=8`, `--max-workers 8`, `--maxWorkers=8`), falling back to
VITEST_MAX_WORKERS, and reported as 'unknown' rather than null when neither
is present. The env var and the flag are recorded separately because they do
not behave the same -- the flag reaches a queued run and the env var does
not. The dashboard now shows historical nulls as 'unrecorded', not
'default'.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
---------
Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
||
|
Justin Maier
|
6f54a21085 |
feat(dev-server): per-service dev/prod env modes on start (#3954)
* feat(dev-server): per-service dev/prod env modes on start A session picks one .env and now applies a per-service overlay on top of it: `start --prod buzz` moves Buzz to production and leaves everything else on dev. Groups come from a gitignored env-modes.local (db, buzz, search, signals, redis today), so adding a service is an edit to that file rather than to code. Every group defaults to dev. DEVSERVER_PROD_GROUPS in the skill .env moves a default when a dev service is unreliable, and a flag beats both. The flag applies to that start only, including when it takes over a dead session, so a prod choice never leaks into the next bare start. Asking for different modes while a session is already running that worktree is refused rather than answered with the running session. The orchestrator, payments, S3, ClickHouse, the notifications DB, the feeds proxy and OpenSearch have no dev counterpart at all, so the summary prints them after every mode line: dev mode is not a claim that nothing here is production. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(dev-server): close the ways an env mode could lie about itself Review found four paths where a mistake resolved quietly to the wrong environment, three of them ending on production while the session reported dev. A malformed section header (`[db prod]`, a missing bracket) matched neither the section nor the KEY=VALUE pattern, and the parser left the previous section open — so every following key landed under it, filing the production DATABASE_URL as `[db.dev]`. Any parse error now closes the open section, and the daemon refuses to start at all when the definitions file did not parse cleanly, rather than logging a warning nobody reads and carrying on. `all` was expanded before group names were validated, so `--prod all,typo` discarded the typo and `--prod all` against a machine with no env-modes.local reported success having moved nothing. Names are checked first, `all` errors when it matches no groups, and it now selects only groups that have that mode instead of failing the whole start over one dev-only group. `--prod=` with an empty value errors like the spaced form already did. A DEVSERVER_PROD_GROUPS entry that matches no group produces a note instead of silently leaving the service on dev — the failure an operator setting it is specifically trying to avoid. The busy-session refusal compared flag text, so it refused `--prod all` against a session started with every group named, and accepted a bare start against a session whose modes a bare start would no longer produce. It compares resolved modes now. That refusal also broke the dashboard: console.mjs sends no modes, so a 409 sent it down a fallback that picked whichever session was first in the map — another worktree, another branch, its logs, silently. It watches the refused session when one is named and otherwise looks only at this worktree. The example understated each group's key set: db without DATABASE_IS_PROD (which gates the S3 delete paths) or DATABASE_REPLICA_LONG_URL left a session writing to production and reading long queries from dev, and redis without REDIS_CLUSTER would point a single-node client at a cluster. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(dev-server): stop the mode guard reporting a stale or partial answer Second review round, on the hardening itself. The 409 mismatch check read `session.modes`, which only start() writes. On the takeover path the request's modes were stored and restart() awaited, so for the seconds spent stopping and reclaiming the port the session still described the run being torn down — long enough for another agent's bare start to match it and be handed 200 for a session coming up on production. The resolved modes are stamped before the await now, and cleared at the top of start() so a session that errors out stops reporting an env it never applied. The endpoint also resolved against a cached skill config while start() reloaded it, so an edited DEVSERVER_PROD_GROUPS made the two disagree; it reloads first. `--prod all --dev search` — everything on prod except one — threw a conflict, because the conflict check ran after `all` expanded. Expansion now skips groups named on the other flag and conflicts are judged on explicit names only, so the exception form works and `--prod all --dev all` is rejected on its own terms. A group named beside `all` that has no section for that mode is an error rather than a silent fallback to dev. Parser errors no longer echo the offending line: they reach an HTTP 400 body and a log buffer any agent can read, and a stray connection-string line splits on its first `=` with the password on the left. The example understated three more groups: search without METRICS_SEARCH_* is a second Meilisearch the app writes to, redis without REDIS_SYS_SENTINELS leaves the system client on production (with sentinels set, the URL supplies only the password), and db without DATAPACKET_DATABASE_RO_URL keeps a read pool on production Postgres. The dashboard counts a `base` group as production, since base means the .env value stood and this .env is production. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * test(dev-server): pin the env-mode rules, and stop the guard overreaching Third review round. The resolver was the most intricate code on the branch and had no test file — the checks proving it lived in a scratchpad, which protects nothing after this session ends. `scripts/__tests__/dev-server-env-modes.test.ts` now covers the resolution rules, the overlay, the parser and the session comparison, beside the queue's tests for the same reason (the daemon is not in the app's module graph). Both mutations fail legibly: reverting the parser fix prints `expected 'PROD-host' to be 'dev-host'`, and reverting the all/dev exception prints the wrong mode map. Fixes with it: The session comparison compared whole maps, so ADDING a group to env-modes.local made every already-running session disagree with every new bare start — `start`, which is meant to be idempotent, would 409 everywhere until each session was restarted. It compares the groups both sides resolved. A group only one side knows about is a definitions edit, not two agents wanting different environments. `DEVSERVER_PROD_GROUPS` is pinned on the session when it is created rather than re-read on every start, because start() also runs unattended — a branch switch, a crash restart — and an edit to that file would otherwise move a LIVE session onto production with nobody having asked. A start refused over its env modes answered 201 and the CLI exited 0, so `start && curl` proceeded as though a server had come up. It answers 5xx with the session's own error line, on both the new-session and the reuse path. The takeover stamp is `pendingModes`, separate from `modes`: a crashed session's process can still be serving on the old env for the length of the port wait, so `modes` stays true to what is running and the mismatch check reads what is coming. `PROD_ONLY_GROUPS` is filtered by what the definitions file defines, so defining `[s3.*]` no longer produces a summary that says `s3=dev` and `always prod: s3` on the same line. Documented rather than fixed: the auth hub is one shared process reading its own apps/auth/.env, so it cannot follow a per-session db mode — a `--prod db` login mints a token for a user id from the other database. And the build dir is keyed on branch, not mode, so changing search/signals mode on a warm `.next` can leave the previous NEXT_PUBLIC_* host inlined in client chunks; keying it on mode would multiply an 8GB cache per combination. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(dev-server): close the fail-open half of the mode comparison Fourth review round, and the worst finding was in the previous round's fix. Comparing only the groups both sides resolved made an EMPTY request match anything: with env-modes.local momentarily absent — an atomic-rename save, a move, a git clean — a bare start resolved to no groups, the mismatch guard did not fire, and the daemon handed back the running session as though it were the dev one asked for, while it ran on production. The comparison is asymmetric now. A group the request has and the session does not is an added section and still matches, which is what keeps `start` idempotent; a group the SESSION has and the request lost does not. `auth-hub` stays in the always-prod list whatever env-modes.local says. Defining [auth-hub.dev] cannot move the hub — it is a separate shared process reading its own apps/auth/.env — so letting a config edit delete that warning would leave the summary asserting something the mechanism cannot deliver. `--prod all` now leaves a note naming each group it could not move for want of a section, instead of being the one unhonourable request in this resolver that says nothing. The takeover stamp is cleared in a `finally`, so a restart that throws cannot leave a session advertising modes that will never be applied. And the dashboard reports a refused start even when it finds a session to attach to anyway — a malformed definitions file returns no session in the body, and repainting a healthy dashboard over the pre-edit session read as the edit having worked. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(dev-server): close the remaining fail-open directions in the mode guard Fifth review round, and again the findings were the mirror image of the previous round's fixes. Each is the direction where a mistake resolved to production while the summary said otherwise. A session that resolved NO groups came up before env-modes.local existed and is running the base .env. `every` over no keys is vacuously true, so once the file appeared, the first bare start was told its dev modes held while the process was still on the .env. An empty running side now matches only an empty request. `--dev all` left any group with no [x.dev] section on the .env — production — and succeeded with a note, while `--dev <group>` for the same group threw. The safety direction should not be the one that fails open, so it throws too. Skipping stays a note in the prod direction, where the group stays on dev. `formatModeSummary` dropped a prod-only service from the always-prod tail whenever it resolved to `base`. `base` means no section applied and the .env value stands, which for those services is production — so a half-written definition removed the warning exactly when it was most needed. Only a group that actually moved leaves the list now. `auth-hub` was described as unmovable but nothing enforced it: defining [auth-hub.dev] resolved normally and wrote its keys into the MAIN app's env, repointing the app at a hub that is not listening while deleting the warning. Defining it is refused at load. And a reuse that threw left the failed request's --prod set pinned to the session, so the next unattended restart would have brought it up on production off the back of a start that errored. The previous overrides are restored on that path. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(dev-server): a named group the session never had is a mismatch Sixth review round, two more directions into the same guard. Leniency about a group the running session does not know is only defensible when nobody asked for it. `--dev search` against a session that started before [search.*] was defined came back 200 "existing", CLI exit 0, while search was still on the base .env — production Meilisearch. Groups named on a flag are a hard requirement now; a bare start against a session missing a newly-defined group is still idempotent, which was the point of the leniency. The override restore lived only in `catch`, but start() reports a mode failure by setting status and RETURNING rather than throwing — the exact case the 500 branch handles. So a failed `--prod db` left the session pinned to it, and the dashboard's restart key would have brought it up on production off a start the CLI reported as failed. Also: the dashboard's mismatch notice was a 3-second flash, after which a dashboard attached to a session that is not what it asked for looked identical to a healthy one — it carries a sticky `!env` marker in the header now. And the redis example restates REDIS_CLUSTER_NODES and REDIS_SYS_SENTINEL_PASSWORD in both blocks, since the overlay applies onto the .env rather than onto the other mode, and the file's own rule says every key of a service moves together. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
Justin Maier
|
4f619f0f39 |
fix(tests): clear the three type errors that reddened the typecheck-tests gate (#3956)
* fix(tests): clear the three type errors that reddened the tests gate `pnpm typecheck` excludes `src/**/__tests__/**`, so these landed green: - minimax-h3-license: `BaseModel` is re-imported, not re-exported, by `~/server/common/constants`. Take it from its origin. - cosmetic-phash: BigInt literals need ES2020; the repo targets ES2018. - sticker-placement: `createStickerPlacement` grew a required `spendType` and the shared fixture never did, so 17 call sites disagreed with it. The fixture carries 'yellow' deliberately — the escrow test asserts 'green', so a placement that dropped the caller's currency cannot pass by matching the fixture. Typing the fixture as `CreateStickerPlacement` means the next required field fails once, at the fixture, not at every call site. Baseline regenerated: sticker-placement leaves it entirely (16 -> 0), and remix-gallery drops 29 -> 28 from an unrelated merge. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * test(sticker-placement): make the currency guard structural, not a comment The escrow test's non-vacuity depended on the fixture default differing from the value it asserts. That was a comment; now the two are named constants and the test asserts they differ, so a future edit that collides them fails on the spot instead of going permanently green against a service that stopped forwarding the caller's currency. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
Justin Maier
|
e056028dbe |
fix(dev-server): stop a queued test run from enqueueing itself
The daemon runs `pnpm run test:unit:run`, which is the script that routes to the queue, and passed its own environment through. The child saw CIVITAI_TEST_QUEUE still set, enqueued a second run and waited for it while the first held the slot that run needed: a deadlock on every full-suite run. One wedged run held the only slot for 20 minutes with another agent's run queued behind it. Raising concurrency does not fix it — each logical run would then occupy two slots, so agents starting together refill them with waiters. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |