mirror of
https://github.com/civitai/civitai.git
synced 2026-09-20 22:08:18 +08:00
main
26331 Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
 Zachary Lowden
|
24a0240db6 |
fix(stripe): stop fractional Buzz amounts reaching Stripe as 500s (#4952)
* fix(stripe): reject fractional Buzz amounts at the trust boundary, not as a 500
Stripe amounts are in the currency's minor unit and must be whole. The Buzz
purchase form derives USD cents by dividing the Buzz amount by 10, so any
free-typed Buzz amount that is not a multiple of 10 (10,004) produced 1000.4
cents. Stripe answered `Invalid integer: 1000.4`, a raw throw out of
paymentIntents.create that reached the client as a tRPC INTERNAL_SERVER_ERROR.
Three changes, all on the same route:
- `paymentIntentCreationSchema.unitAmount` gains `.int()`. The tRPC input schema
is the trust boundary, so a fractional amount is now a BAD_REQUEST naming the
field rather than a 500 naming nothing.
- The form ceils the derived cents value, so a legitimate purchase cannot
produce the fraction in the first place — without this the schema would turn
the 500 into a 400 for a purchase that ought to succeed. Ceil rather than
round, matching the minBuzzAmount derivation beside it and never granting
more Buzz than is charged for. The submitted Buzz amount is re-derived from
this value, so the pair stays consistent with the server's
`unitAmount === buzzAmount / 10` check.
- The amount-tamper guard throws a typed BAD_REQUEST instead of a bare `Error`.
`getTRPCErrorFromUnknown` maps a plain Error to INTERNAL_SERVER_ERROR, so
rejected input on this route also answered with a 500. The condition is
unchanged; only its type.
Regression matrix, both files watched red before the change:
at origin/main (
|
||
|
Zachary Lowden
|
a042ff3121 |
refactor(stripe): delete the dead createBuzzSession path instead of hardening it (#4955)
`createBuzzSession` had a missing integer bound — it hands
`unit_amount: customAmount * 100` to Stripe — so the obvious move was to add
`.int()` beside it. It is deleted instead, because nothing calls it and it is
an authenticated Stripe-calling surface.
Enumeration, complete over tracked files at origin/main rather than sampled:
git grep createBuzzSession origin/main -> 12 hits, all inside the chain itself
git grep createCheckoutSession origin/main -> 3 hits: the definition, the object
it is returned on, and one docs line
about the unrelated
membershipGift.createCheckoutSession
git grep useQueryBuzzPackages origin/main -> 7 hits; the two consumers destructure
{ completeStripeBuzzPurchaseMutation }
and { packages, isLoading, processing }
So the chain trpc.stripe.createBuzzSession -> createBuzzSessionHandler ->
createBuzzSession was reachable only over the network. It was annotated DEAD
CODE in two places already (the service comment, and the hook wrapper's own
"DEAD CODE: no callers"), and Buzz purchases have used the PaymentIntent flow
for some time.
Removed: the service function, the controller handler, the tRPC procedure, the
input schema and its inferred type, the client mutation and its
`createCheckoutSession` wrapper, the two now-unused type imports, AND the
service's six-line DEAD CODE comment block. That last one is not cosmetic: an
earlier revision of this change deleted the function and left the comment, which
then sat directly above `export const upsertSubscription` — the function the
`customer.subscription.*` webhook calls to sync `customerSubscription` — where
it reads as "DEAD CODE: no live callers" describing a live webhook path. The
toolchain checks nothing about comments, so only reading the file back catches
it. Verified at this commit: `createBuzzSession` has 0 occurrences under src/,
`upsertSubscription` carries no preceding comment, and the hook wrapper's own
DEAD CODE comment went with the function it described.
`getBuzzPackages` is untouched — it is live, and both components read `packages`
from it.
The limits of the evidence, stated rather than implied: the procedure carried
`.meta({ requiredScope: TokenScope.Full })`, so an API-token holder could in
principle have been calling it directly, which no grep over this repo can see.
The telemetry check below covers 14 days and the org's own traffic only. The one
check that would close it is Stripe-side — sessions created by this function
would appear in the Stripe dashboard — and it was not run.
Verification at this commit (worktree off origin/main
|
||
|
Zachary Lowden
|
f213521be0 |
fix(blocks): close six fail-open spelled guards in the block-token guard tests (#4984)
* fix(blocks): close six fail-open spelled guards in the block-token guard tests Six weaknesses in the App Blocks bridge-token guard and its REST sibling, each fail-open, each measured GREEN under its own evasion before the repair and RED after (clawgate #589). Guard/scaffolding layer only — no runtime file is touched. The class is not theoretical: the same shape recently let a live REST route pass 24/24 with no token verification, no revocation check and no approved-status gate, because the assertion whose stated purpose was that regression read the raw file and a commented-out wrapper satisfied it. The six, with the walk that was open and what closes it: 1. PROC_RE pinned the procedure BUILDER's spelling, so `evasiveProc: t.procedure` taking a blockToken and guarding nothing was outside the derived population. Closed by a derived cross-check on a different surface: every tRPC terminator in the router must land in exactly one PROC_RE-named chunk, counted from the parse so a computed `['mutation'](` is counted too. 2. GUARD_CALL_RE ran on RAW chunk text, so commenting out a proc's guard call and decoding the token instead still read as reaching the guard. `chunks` now carries a normalised slice and every reachability decision reads it. 3. `scan(read(GUARD)).direct` counted matching LINES of raw text, so one prose sentence writing `verifyBlockToken(blockToken)` let the REAL call be deleted while the count stayed at 1. It now counts CALLS on normalised code. 4. The REST opt-out population could not see `onApprovalLookupFailure` arriving by object spread, so tip.ts — the irreversible Buzz transfer — opted out of failing closed invisibly. Closed by pinning the options object's SHAPE. 5. RESERVED_WORDS was unpinned while its neighbour MODULE_EXEMPTIONS was pinned, so the anti-suppression pin was evadable through the adjacent set. Closed by a subset test against a language-level word list. 6. `status === 'approved'` and friends were satisfiable by a STRING literal, because the filter stripped comment lines and nothing else. Closed by normalising literals behind a sentinel no literal body can forge. The normaliser is built on the TypeScript parser rather than a hand-rolled lexer, because the lexer was measured wrong on this corpus in three ways that are all fail-open: a nested template inverts which regions are code (21 live instances under src/pages/api), a regex literal desyncs it (`/^https?:\/\//` appears twice in blocks.router.ts), and `${}` interpolation is real code it swallowed. Every normaliser entry point has a positive control: an identity `return source;` on any of the six fails at least one assertion. Full mutation matrix in the PR body. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * docs(blocks): correct the nested-template count to a figure I measured The docstrings said "21 nested templates under src/pages/api", a number taken from a review report rather than measured here. Re-derived from the TypeScript parse (a template literal lexically inside another): 35 across 14 files, 2026-09-19. The claim the number supports is unchanged and if anything stronger — the shape is ordinary, not exotic — but a figure standing in a committed docstring has to be one this change actually took. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * docs(blocks): state the lexer-vs-parser measurement at the scope it was taken The docstrings attributed "disagreed on roughly half" to the 345 files these two suites read. The sweep that produced it covered 1,724 files under src/server and src/pages, and found 858 disagreements — a wider population than the sentence named, so the rate did not belong to the set it was attached to. Restated with the real denominator and an explicit note that it is the wider sweep, so it reads as "endemic in the corpus" rather than as a rate for the scanned set. No assertion changes; 65 tests still pass. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
Zachary Lowden
|
e601fa4f5c |
refactor(app-blocks): route the four per-call budget gates through one helper (#4983)
No behaviour change. The four submit gates each spelled the same
`cost > claims.buzzBudget` comparison inline; they now call one exported
helper, `blockPerCallBudget(claims, { pricesAuthorFee })`, which returns
`claims.buzzBudget` on both classifications. Every gate compares the same
number it compared before.
The gates are not interchangeable even though they spell the same thing:
two add the per-generation author fee into the value they compare, and two
do not — and on the pass-through path the value that clears the gate is the
value reserved and the value the terminal settle bills. Any future change to
the ceiling is correct for at most one of those populations, so it has to be
made somewhere that knows which gate is asking. `pricesAuthorFee` is how each
gate declares which it is; it is classification only and no branch reads it.
Adds `no-direct-block-budget-claim-read`, a structural guard: every
occurrence of `claims.buzzBudget` in production source must match one of four
enumerated allowed forms (the presence pre-check, the single mint-site write,
the helper's own return, the two read-surface projections) or it fails,
naming file, line and text. It scans every production .ts/.tsx under src/,
blanks comments before matching, and finds call sites by balanced-paren
extraction, so a new spelling, a gate in another module, or a prettier
line-wrap cannot hide one.
This replaces an earlier version of this branch that granted author-fee
headroom at token mint time. That is withdrawn: the raised ceiling was not
consumed by a fee on the two fee-free gates, where it would have been
reserved and billed above the ceiling the app's manifest declared, and it was
equally unconsumed on a fee-pricing gate whenever the fee prices to zero.
|
||
|
Zachary Lowden
|
43d48b42c4 |
fix(blocks): narrow the dev-token exemption so a suspended app stops driving the bridge for 4h (#4980)
* fix(blocks): the dev-token exemption no longer skips the approved check for every dev token
The App Blocks approved-status predicate short-circuited on `claims.dev === true`,
so a moderator suspension left already-minted dev tokens driving both halves of the
runtime — the 15 tRPC bridge procedures and every `withBlockScope` REST route — for
the remaining life of the token. Dev tokens live 14400s against a 900s default, so
that window was 16x every other token's, on the class with the widest scopes.
The `dev` claim is stamped unconditionally by `signDevScopedPageToken`, which six
mint paths reach. Three of them must run a non-approved app; three must not. Keying
on the bare boolean exempted all six, and the docblock justified it by listing belts
the guard re-checked none of — ownership, an active dev tunnel, the author flags,
the approved-scope clamp.
The predicate now separates them:
- a signed `reviewRunForReal` claim short-circuits ahead of the read, which is the
moderator review sandbox and the one population that must run a non-approved app
without owning it;
- no backing row means nothing to be approved, covering the synthetic-id mints;
- a REAL row that is not approved is exempt only when the subject IS the app's
owner AND that owner has an ACTIVE dev tunnel for the slug — the two preconditions
the owner-dev-tunnel mint enforces, re-derived rather than assumed;
- anything else with a real, non-approved row is refused, which is the `dev:live`
token whose mint required approval and that has simply outlived it.
Ownership comes from one extra selected column on the lookup this path already
performs. The tunnel check is reached only on the dev + real-row + not-approved
path, so no approved app and no non-dev token pays for it.
Revocation is untouched — it was never exempted — and the 4h lifetime is unchanged.
Refs clawgate #571.
* fix(blocks): resolve the dev-token owner in the branch, not as a second query on every request
Findings from this repo's five-lane review, applied. Three of them were wrong
in ways a green suite could not show.
PERF — the owner was read as `app: { select: { userId: true } }` on the row
lookup, and two comments asserted that was "a join on the FK, not a second
query". It is not: the schema enables only `previewFeatures = ["metrics"]`, so
with no `relationJoins` Prisma resolves a nested relation with another round
trip, and because `appId` is a REQUIRED relation that query cannot be skipped on
an empty FK set. It would have fired for every token whose row exists — every
bridge call including the timer-driven `pollWorkflow`, and every block-JWT REST
request — to read a column only the dev branch consults. `claims.appId` IS the
`OauthClient.id`, so the lookup moves into the branch as the same primary-key
read, issued only when needed. The row read returns to `select: { status: true }`
and a test now pins that literal.
TESTS — moving the `dev` check behind the row read silently made the existing
`dev=%p is NOT exempt` table VACUOUS. Its fixture carried no owner, so the
mutant `!claims.dev` fell through to the ownership guard and was refused there:
the table stayed green while the guard it claims to pin never executed. The
repair is in the fixture — clear every other reason to refuse, so the verdict is
attributable to the `dev` comparison alone. Six mutants now each die to their own
test, including this one.
NARROWING — `reviewRunForReal` was exempting without requiring `dev`. Every mint
stamps both, but `sign` accepts the field independently, and a bypass keyed on one
signed boolean without narrowing it is the defect this change exists to fix; alone
it would have been WIDER than the blanket exemption it replaced.
POSTURE — `getActiveDevTunnel` attaches its `.catch()` to the result of
`sysRedis.get(...)`, so a synchronous client throw escapes it, as can the dynamic
import. Unwrapped that becomes a 503 blamed on the replica read. Now wrapped, so
the fail-closed posture is written rather than inherited.
REUSE — `subjectForUserId` already existed in `block-revocation.service` under a
docblock claiming to be "THE ONE PLACE this format is written on the WRITE side".
It was not — the mint open-coded the same template — and this change would have
been a third copy, each pinned by its own literal so no test could see them
diverge. Moved to a zero-import leaf both now use, re-exported so no importer
changes.
Plus the written half: the population table the code referred to now exists as a
table, the `ai:write:budgeted` containment claim is scoped to the population it
is actually true of, and the two sibling resolvers each carry a cross-reference
to the other two.
Refs clawgate #571.
* fix(blocks): log the dev-tunnel re-check failure instead of folding it into the refusal count
Delta round on the previous commit's fixes. The wrapper added there was right
about the verdict and wrong about observability.
A throw out of the dev-tunnel re-check used to reach `resolveRestApprovalVerdict`,
get logged through the throttled limiter with its error message, and answer
`lookup_failed`. Wrapping it turned that into a bare `catch` returning
`not_approved` — correct as a verdict, but it increments the SAME series this
whole change ships to be watched on. A sysRedis fault would have pushed every
population-E owner into `not_approved` fleet-wide with nothing logged anywhere,
and the predicate's own docblock reads that series as "the 4h window closing" —
so the incident would have read as the narrowing working. The one leg the change
added was the one leg it made unobservable.
It now logs, with its own message and its own throttle window. Deliberately a log
rather than a new verdict: a `tunnel_lookup_failed` would have to be mapped by both
callers, and the REST mapping for an unrecognised verdict is 503 — the exact
misattribution (a cache fault blamed on the replica read) the wrapper exists to
avoid. Separate windows because two failure modes sharing one would suppress each
other, and the one you did not see would be the one you needed.
The throttle logic is now written once and closed over per caller, rather than
open-coded twice.
Also from the same round:
- `parseSubjectUserId` and five self-scope gates still spelled 'anon' as a literal
while the new leaf claimed one spelling for the format. They use ANON_SUBJECT now,
so the claim is true rather than nearly true.
- `publisher-ban-revocation` took `subjectForUserId` through
`block-revocation.service`'s re-export — a module wholesale-mocked in a dozen
suites with a factory exporting only `BlockRevocation`, which is the shape the
leaf was extracted to avoid. It imports the leaf directly.
Refs clawgate #571.
* docs(blocks): correct four claims the last round's fix made false
Round 3 of the audit ladder. No behaviour change — every finding was a comment
that the code contradicts, which is the class this file keeps producing because
its docblocks carry the reasoning rather than just describing it.
1. The new tunnel-logger docblock said a dedicated verdict was avoided because
"the REST mapping for an unknown verdict is 503". It is not. The chain in
`withBlockScope` is not_approved -> 403, lookup_failed -> 503, and then an
`else` that asserts `satisfies 'not_found'`, logs "SERVING (observe-only)" and
falls through to the handler. REST's runtime default for an unrecognised
verdict is to SERVE, and it would log the request as a missing row it is not.
The bridge is the opposite: `satisfies never` then an unconditional FORBIDDEN.
So the paragraph told the next author REST fails closed on the exact branch
where it fails open. The real reasons — the two-caller mapping burden, forced
by the compile error at that `satisfies`, and REST's serve-by-default — are
now what it says. It also inverted the trade: a dedicated verdict would be
BETTER attribution than the log, since it would carry its own reason= label
instead of sharing not_approved. Recorded as such, so "add the verdict" stays
available as the deliberate change rather than looking already-rejected.
2. `LOOKUP_FAILURE_LOG_WINDOW_MS`'s docblock now sits above both loggers while
asserting "THE COUNT IS NOT THE ALERTING SIGNAL — the unthrottled
reason=lookup_failed series is". True of the replica-read logger, false of the
tunnel one, whose failures resolve to not_approved and have no dedicated label:
there the throttled log IS the only signal, so a suppressed line is lost
information rather than redundant prose. Same window, opposite relationship to
the metrics.
3. The predicate's "IT DOES NOT CATCH" heading is a blanket claim the previous
commit falsified 130 lines below it, and it names as an anti-pattern exactly
what the tunnel leg now does. Scoped to the ROW reads, with the reason the
exception is right there and wrong for them: an unreachable replica means "we
cannot establish whether this app may run", which is a different question per
caller; an unreachable tunnel cache means "no live tunnel", which is the same
fail-closed answer everywhere.
4. The bridge's "a read that THROWS propagates as the tRPC internal error" is no
longer true of the tunnel read — that caller now gets FORBIDDEN plus a warn
this path never emitted. Scoped, and flagged as the behaviour change it is.
Also: the new log test relied on being the first tunnel failure in the process
for its toHaveBeenCalledTimes(1). It resets the window instead, so it no longer
breaks based on where it sits in the file.
Refs clawgate #571.
* fix(blocks): give the dev-tunnel failure its own verdict — the log it had cannot be read here
Round 4 of the ladder, and it overturns round 2's fix rather than refining it.
Round 2 found the dev-tunnel re-check swallowing a throw into `not_approved`
with no signal, and answered it with a throttled `console.warn`. That answer is
inert on this deployment: `app-block-runtime.metrics.ts` states twice, and
designs around, the fact that application-container logs are NOT collected here
— "the `console.error` shape used elsewhere in the repo would be invisible to a
later investigator". So the fix swapped a silent swallow for an unreadable one,
and the paragraph arguing the log was the signal separating a cache incident
from the stale-token population was wrong about its own environment.
The leg now returns `tunnel_lookup_failed`. It refuses identically to
`not_approved` on both callers — same status, same message, deliberately, since
a bearer learning that the dev-tunnel cache is down would be an infrastructure
oracle — but it carries its own `reason=` label, so an operator can tell a
sysRedis fault from the population this change exists to create. That matters
because `not_approved` is the series the whole narrowing is watched on: folded in,
an incident reads as the fix working.
NOT reused `lookup_failed`, which was the tidier-looking option: that verdict
means the REPLICA read failed, answers 503, and is SERVED on the five routes
declaring `onApprovalLookupFailure: 'serve'`. Both would be wrong — a cache fault
blamed on the database, and a non-approved app served on some routes.
Four edit sites, all compiler-forced: the verdict union, both caller mappings,
and the metric's reason union. The cardinality guard went 3 -> 4 series; its
budget is a deliberate literal, so the new label is argued in place rather than
waved through, and the test now drives the new reason instead of only declaring
it.
Three prose corrections from the same round, each a claim the code denies:
- "the window is shared because the rate argument is identical" — it is not. The
replica logger's case is fleet-wide simultaneity; the tunnel logger is reachable
on one owner's one app. By this repo's own reasoning that shape needs no throttle
at all. Kept, with the real reason.
- "a dedicated verdict would carry its own label" was true only for REST: the
bridge records no verdict metric at all, and the label union is a third edit
site rather than derived.
- "`lookup_failed` -> 503" is route-dependent, the same shape of flat claim round 3
existed to remove, one verdict over.
Refs clawgate #571.
* docs(blocks): teach the verdict docs about the fourth reason, and stop claiming the bridge is covered
Round 5. No behaviour change. Adding `tunnel_lookup_failed` last round left six
places describing a three-verdict world, including the two an operator actually
reads, and repeated this change's own recurring mistake: writing a justification
one surface wider than the fix reaches.
THE ONE THAT MATTERS. The claim that a suppressed log now "loses prose and not
information" is true on REST only. `recordBlockRestApprovalVerdict` has a single
production call site, in `withBlockScope`; `assertAppBlockApproved` resolves the
same verdict and records nothing. So a tunnel-cache fault reached through the
BRIDGE emits no counter at all, and its only trace is the throttled warn — on a
deployment that does not collect container logs. That gap predates this work and
is equally true of `not_approved` (the bridge has never recorded a verdict), so
closing it is a bridge-metrics change rather than a guard one and is not taken
here. What is taken is saying so, in all three places that would otherwise let a
reader infer from the REST series that the bridge is covered — including the
explicit warning that a zero on `reason="tunnel_lookup_failed"` does not mean the
leg is healthy, given the bridge is the higher-rate surface (`pollWorkflow` is
timer-driven).
The operator-facing docs now know the label exists. The metric's `help` string
and its reader table enumerated three reasons and read as exhaustive, which is
the one place a description of `reason` reaches whoever is looking at the series
in Grafana — the whole point of the label was that someone can tell a sysRedis
fault from the stale-token population, and it was undocumented. Likewise the
verdict table in `block-scope.middleware`, which is the gate's own explanation of
the branch chain the last commit edited fourteen lines below it. That table now
carries the row that does NOT bend: `onApprovalLookupFailure` is scoped to
`lookup_failed` alone, so a route wanting lookup-failure tolerance does not get
it here — serving a known non-approved app because a cache was down is not
tolerating an unknown.
Count corrections: 3 -> 4 across the metric docblocks, the middleware table, the
emitter's own docblock and the metrics-test prose.
Two more claims the code denied:
- `resolveRestApprovalVerdict`'s docblock is the canonical mapping (the middleware
points at it rather than restating), and it still listed three verdicts AND
repeated the flat "`lookup_failed` (503) refuse" that the previous commit had
corrected 240 lines above. Fixed one copy, left the authoritative one.
- The `tunnelFailureLog` docblock still opened, present tense, with "converts a
throw into `not_approved`" while its own third paragraph said otherwise.
And two of my own from last round, which is the pattern:
- The new metrics-test comment claimed it proved a production caller emits the
reason. It cannot — the loop iterates the union, so a phantom nobody emits
satisfies it identically. The real pin is in the approved-gate suite, against
the real middleware; this one is the cardinality half and now says so.
- "That second shape does not need throttling at all" understated the tunnel
logger's case: a sysRedis incident hits every pod at once too, so it is the same
simultaneity over a smaller population, not a different shape.
Refs clawgate #571.
* docs(blocks): finish the fourth reason — the count said four where the list said three
Round 6. No behaviour change. Both findings are the previous commit's own half-done
edits, which is the pattern this ladder keeps producing.
The metrics test's reader block was bumped to "the four reasons" over an
enumeration that still listed three. Before that bump it was stale but coherent;
after it, a reader counting rows finds one reason undocumented and has to go
looking for which — and the missing row is the one carrying the new operational
fact, that `tunnel_lookup_failed` is 403 on every route because
`onApprovalLookupFailure` does not reach it. That block is the third reader table
in the same family as the metric `help` string and the middleware table; the other
two got the row and it did not.
Same edit, second instance: a test title went "the three reasons are SEPARATE
series" -> "the four", while its body still drove three. The fourth reason's
separateness was covered elsewhere, so nothing was unproven — but the title read
as the proof and was not, which is exactly the over-claim the same file corrects
two cases below. The case now drives what its title counts.
And the REST-only scoping from last round reached three sites but not the two
that most needed it:
- The `catch` comment inside the SHARED predicate said "the counter is the
signal", flat. That function is the one both callers use and the place a reader
is standing when they ask whether this is observable — and on the bridge there
is no counter.
- The `tunnelFailureLog` docblock closed with "no longer load-bearing", which
directly contradicted the paragraph ninety lines above it stating that a bridge
tunnel failure has this line as its only trace. Both cannot be true of one
logger. The asymmetry is now stated as the argument it is: for giving the bridge
a verdict counter, not for trusting the log.
Refs clawgate #571.
* docs(blocks): the comment welded two slips into one moment, and pointed at the wrong case
Round 7, and both findings are in the five-line comment round 6 added.
It said the reason was "left out when the reason was added — the title said four
while the body drove three". Checked against the branch: at the commit that added
the reason the title still said THREE, and the case was internally consistent —
under-covering the new reason, but claiming nothing it did not prove. The title
was bumped to four in the following docs pass, and that is where it became a
coverage claim wider than the test. Two slips, one commit apart, welded onto a
moment neither of them happened at. The commit message for that pass had the
history right; the in-file comment was a degraded restatement of it.
And "two cases below" is off by one — the case that corrects the same shape is
`emits AT MOST 4 series`, three below. Named rather than counted now, so it
cannot drift again when a case is inserted.
Refs clawgate #571.
|
||
|
Zachary Lowden
|
e81bcc973f |
fix(test-cache): normalise the leading slash POSIX fileURLToPath adds to a Windows file URL (#4981)
* fix(test-cache): normalise the leading slash POSIX fileURLToPath adds to a Windows file URL
`scripts/__tests__/test-cache-core.test.ts` has been red on every pull request
opened since 07:13Z today, failing one assertion of 32:
× gives the same path for the same file in two worktrees, URL or path
AssertionError: expected null to be 'src/a.ts'
Root cause, established by executing the function rather than reading it:
`fileURLToPath` is platform-dependent. Given `file:///C:/Dev/wt/two/src/a.ts`
it returns `C:\Dev\wt\two\src\a.ts` on Windows but `/C:/Dev/wt/two/src/a.ts`
on POSIX — with a leading slash. Neither drive-letter comparison in `toRel`
can see past that slash, so the path stops matching `root` and `isAbsolute()`
returns null instead of the relative path. The test asserts the Windows
result; CI runs Linux.
The fix normalises the leading slash away immediately after the conversion, so
the drive-letter forms below it read the same shape whichever platform
resolved the URL. It is one line in the shared prefix of every path reaching
this function, which is why the diff carries more comment than code.
Verified:
- red at the pre-fix commit, green at HEAD. With the fix reverted and the test
file unchanged: 2 failed | 32 passed. With the fix: 34 passed.
- Six control cases executed before and after — both POSIX spellings, a POSIX
path outside the root, an already-relative path, the Windows non-URL path,
and a Windows path outside the root. All unchanged by the fix. Only the
failing case moves.
Two tests added, and they are labelled from what they were MEASURED to do at
the pre-fix commit rather than from what they were written to do. The first
draft called both "invariant"; the run showed the second one fails at base, so
it is regression coverage and is now named that. The POSIX one does pass at
base and stays labelled an invariant guard, so nobody counts it as regression
coverage it does not provide.
Scope note: this is a fix for a defect on main, deliberately kept out of the
unrelated PR that surfaced it. #4971, which introduced the test, merged at
07:13:43Z with this same shard already failing on its own head commit.
One judgement worth flagging for review: a POSIX path whose first segment is
literally a single letter and a colon (`/C:/…`) would now be rewritten. That
spelling is pathological on POSIX and cannot be produced by `fileURLToPath`
from a non-Windows URL, but it is the one input whose handling this changes.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* fix(test-cache): scope the normalisation to the file:// branch, and drop a subsumed test
Round 0 of the pre-merge audit found two things, both acted on here. Neither
is a correctness defect in the shipped fix — CI was fully green at 20282b3e77,
19 entries, 18 success, 1 skipped, shard 2 passing.
1. NARROWED (R1). The normalisation ran on every input, so it also rewrote a
genuinely POSIX path whose first segment is a letter and a colon. Measured:
toRel('/C:/notes/x.md', '/C:') returned 'notes/x.md' before and null after.
That was the one behaviour change the PR body had to flag for review.
Only a file:// id can carry the platform artefact, so only a file:// id
needs the repair. Moving it inside that branch satisfies the requirement
exactly and leaves every non-URL input byte-for-byte as it was. The flagged
behaviour change is gone rather than documented.
Measured across all three variants on 8 cases: base fails only the Windows
file:// URL case; the unscoped draft fixes that but breaks the POSIX
pseudo-drive case; the narrowed version is correct on all 8.
2. DELETED a test I added (the "regression" equality at :93-103). The audit
ran a mutation table I had not. Against four mutants — normalisation
deleted, inverted, prefix-compare broken, and "strip any leading slash" —
that test killed only the first, which the pre-existing assertion at :71-72
already kills, and it PASSED both the inverted and broken-prefix mutants.
An equality with no anchor is satisfied when both sides return null, which
is the property I had described as its strength. It is subsumed, and
strictly weaker than the assertion that surfaced the bug.
A comment now records why it was removed, so it is not re-added as an
apparent improvement.
The invariant guard survives and is unchanged: it is the only thing in the
file that kills the "strip any leading slash" mutant, and every other toRel
assertion here is Windows-shaped while CI and every Linux/macOS dev run POSIX.
A new invariant guard pins the POSIX pseudo-drive case the narrowing protects.
Re-verified after the change, because an audit fix resets the verification
gate: 34 passed at HEAD; at the pre-fix commit 1 failed | 33 passed, the single
failure being the genuine regression guard at :71-72. That is a cleaner control
than the previous revision, where two failed because the subsumed test failed
alongside it.
Round 0 reports and does not move the ladder; its advisory verdict was safe to
merge, and the requirement survived questioning — the Windows fixture is this
repo's only Windows coverage for toRel, since no workflow runs vitest on a
Windows runner, so platform-gating or deleting it would zero that coverage.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
---------
Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
||
|
Zachary Lowden
|
981843387e |
Apps earnings copy no longer promises a payout cadence that does not run (#4979)
* fix(apps): earnings copy no longer promises a payout cadence that does not run
Three user-facing strings asserted an automated payout pipeline that is not
wired, on surfaces the app-author cohort reaches today.
`mintPayoutForOwner` is the only writer of `paidOutAt`/`payoutId` and has no
production caller — every non-test reference is a prose comment. The weekly
`bulk-payout-block-attributions` job IS registered and does run, but is an
explicit stub: it aggregates, logs, and writes nothing. So the cadence existed
only in the copy.
- src/pages/apps/revenue.tsx: "Payouts are batched weekly" -> confirmed
earnings accrue, automated payouts are not yet enabled. The pointer to Apps
for managing installations is unchanged.
- src/components/AppBlocks/RevenuePanel.tsx: the Confirmed (unpaid) tooltip no
longer promises inclusion in a "next payout"; it states accrual.
- src/components/Apps/AppEarningsPanel.tsx: the docblock claimed the earnings
proc "grants it to any accepted editor with a session". It does not —
`getAppEarnings` is an `appDeveloperProcedure`, so it refuses any caller
outside the app-author cohort, and such an editor never reaches the panel
because the authoring-context proc carries the same middleware. The
paragraph's rationale for the panel existing is kept; only the access claim
is corrected.
Adds src/components/AppBlocks/__tests__/payout-copy-truthfulness.test.ts,
which pins the two user-facing strings WHOLE and normalised (a banned-word
grep is walkable by rewording) and ties them to two state guards: the payout
rail has no production caller, and the weekly job performs no writes. Wiring
the rail fails the state guard first, which is the signal to revisit the copy.
Red at origin/main, green at HEAD: the three copy/docblock assertions fail on
pre-change content while the state guards stay green, so each fails for its
own reason rather than behind an earlier throw.
Not changed, deliberately: the payout rail itself, the Earnings tab and
capability, the invite disclosure copy (it promises visibility, not money),
the "Paid out" / "Confirmed (unpaid)" card labels (real status buckets), and
the Pending "settles after the refund window" tooltip (settlement and payout
are different claims).
* fix(apps): address review findings on the earnings-copy guard
Four of the five pre-completion review lanes reported; findings resolved:
- Reuse: the guard hand-rolled a `stripComments` that already exists as a shared
module (`test/strip-comments.ts`), and the local copy was the weaker one — it
collapsed block comments to '' rather than ' ' (which can join tokens across a
stripped comment) and did not strip STRING literals at all. Now imports the
shared `stripComments`/`stripCommentsAndStrings`. The caller scan uses the
strings-stripped variant, since a name inside a string literal is not a call.
- Correctness: the guard's own header claimed "three user-visible strings" when
one of the three is a docblock, reachable by no cohort. Corrected — in a file
whose thesis is that committed prose must be machine-checkable, that was
exactly the kind of unchecked claim it exists to forbid.
- Correctness: the "do not soften the disclosure" rationale in AppEarningsPanel
was built on the sentence this change retracts, so it no longer followed from
anything. Re-anchored to the reason that does hold: the cohort gate is a
runtime Flipt toggle, so copy softened to today's narrow gate becomes an
under-disclosure the moment the flag widens, with no code change and no PR.
- Intent + Correctness: the caller scan walked `src/` only, making its headline
("no production caller") wider than what it measured. Now walks `src`,
`packages`, `apps` and `scripts`. The residual gaps it still cannot see — a
renaming import, a computed member access — are stated in the file rather than
implied away.
- Reuse: the guard's population was two hand-named files, i.e. only the SHRINK
half of a ledger. Nothing failed when a THIRD cadence sentence appeared on an
unlisted surface, which is the condition that let one wrong claim become three.
Adds a GROW half: a cadence-phrase scan over the App Blocks / Apps / apps-pages
money surfaces, carrying both a positive control (it must catch the two
sentences this PR removed) and a negative control (the legitimate "Paid out" /
"Not paid out" bucket labels and the new accrual copy must not trip it).
Also drops the negative grep for the retracted docblock sentence. The rewrite now
QUOTES that sentence so the next reader knows what was wrong, so a "must not
contain" assertion would have forbidden the clearest way to document the
correction — and it was a spelled guard regardless. Replaced with a state tie:
the router declares `getAppEarnings` on `appDeveloperProcedure`, trpc.ts defines
that as `protectedProcedure.use(hasAppBlocksAuthor)` throwing FORBIDDEN, and the
docblock names both the procedure and the cohort. Widened the job write-scan to
cover delete/deleteMany and the raw-SQL escape hatches.
Red at origin/main, green at HEAD, re-established for the revised guard: the
three copy/docblock cases AND the new GROW case fail on pre-change content while
the sanity and two state guards stay green — so each fails for its own reason.
The GROW half going red at main is the direct evidence it would have caught this.
Perf lane: no findings — the added tree walk is the 97th in the unit suite and
about 1% of a cost already paid; sharing it is impossible under `pool: 'forks'`
with `isolate: true`.
* fix(apps): harden the earnings-copy guard against the test lane's findings
The test-review lane measured two ways this guard could pass while wrong.
F1 — POLARITY INVERSION (the serious one). `test/strip-comments` documents
itself as biased toward over-stripping because that "turns the guard RED, which
is the safe direction". That holds for its other callers, which assert a call IS
present. This guard asserts ABSENCE, so the same bias turns it GREEN: a real
call the stripper ate reads as "no caller". Measured, ~105 files across the
scanned roots have real code hidden from the stripper (a `/*` inside a `//`
comment, or a regex literal ending `\/`), including files under services/blocks
— the payout rail's own neighbourhood.
The caller scan is now an exact per-file OCCURRENCE ledger over RAW text: three
files, with counts. It fails when the set grows, when it shrinks, or when any
count moves. That also closes the shapes a call-shaped regex missed for free —
renaming import, bare callback reference, `.call`/`.apply`, computed access —
because the bare identifier is what is counted and the import is the tripwire.
Stripping is now used only for "this code is NOT here" checks on files already
pinned by name, where over-stripping cannot manufacture a pass on its own.
F2 — the cadence-phrase GROW scan is REMOVED, not tuned. Measured: ten realistic
re-promises evaded it ("Payouts run weekly", "disbursed every Monday", "You get
paid every week"), and six TRUE statements tripped it, including "Payouts are
processed manually until the automated rail lands" and anything using "will be
paid" — so it forbade the accrual-truthful phrasing this PR institutionalises.
It also read raw text, so a comment quoting the retracted sentence as
documentation would have failed the build: an unlandable guard, which is worse
than none because it gets deleted rather than obeyed. English cadence is not a
regex problem.
Replaced with a STRUCTURAL ledger of the files that render settlement buckets.
That population is stable (this PR's scope deliberately keeps those labels), so
a third money surface fails and its author has to decide consciously whether it
needs the accrual disclosure the other two carry.
Also from the lane:
- F3: the docblock case's name was wider than its body — two bare identifier
probes would pass a docblock that re-asserted the retracted claim. Now pins
the correction sentence WHOLE, normalised through a `prose()` helper so the
pin does not also encode where prettier wrapped.
- F4: the three added caller roots had no positive control; `src` alone clears
any plausible file-count threshold. Now probes one path per root.
- F5: the job write-scan missed `pgDbWrite`, Kysely's `.updateTable(` and any
helper indirection, and its negatives had no control proving they could fire.
Now carries the repo's real write idioms, a positive control per idiom, and a
pin on the job's awaited calls so a write hidden behind a helper fails too.
- The tooltip anchor now asserts it was found, so a missing card reports as a
missing card rather than as a copy mismatch via `slice(-1)`.
Four header docblocks described the previous design and were corrected with it —
in a change about stale claims, shipping stale comments is the same defect.
|
||
|
Zachary Lowden
|
bf43398486 |
feat(app-blocks): count the fifth bridge silence — validator_rejected (#4977)
* feat(app-blocks): count the fifth bridge silence — validator_rejected
The receiving half of the App Blocks bridge's fifth drop path. The other four
are already counted on civitai_app_block_bridge_messages_total (#4946); this one
no code here can observe, because the SDK's validator runs in the iframe AFTER
this host has replied — from here the exchange completed and the dispatcher
already counted it `handled`. The block is the only witness, so it reports over
the new fire-and-forget BLOCK_MESSAGE_REJECTED and the shared dispatcher
translates that into outcome="validator_rejected".
It is the one of the five with a confirmed production incident: on 2026-09-18
custom-generators served "Couldn't load your kept images just now." from relist
until a human found it by hand, while civitai_app_block_renders_total read
result=ok, error_class=none throughout.
- bridgeLabels: a sixth BRIDGE_MESSAGE_OUTCOMES value. The beacon's
z.enum(BRIDGE_MESSAGE_OUTCOMES) and the client emitter both derive from that
array, so nothing else on the wire path needed respelling.
- usePostMessage: one branch, in the SHARED dispatcher above the subscriber
lookup — the message is telemetry, not a feature either host implements, so
it reaches no onMessage subscriber by design and a per-host handler would be
the same predicate written twice.
- hostHandlerParity: the new type as an N/A-for-every-host entry, because the
parity test greps hosts for a registration that must not exist here.
- the label-product arithmetic in three docblocks: (A+1) x 47 x 2 x 5 becomes
(A+1) x 48 x 2 x 6, ~29k series per pod at 50 approved apps.
The `type` on the new outcome is the block->host REQUEST left hanging
(GET_IMAGES_BY_IDS), not the rejected reply (IMAGES_RESULT). Deliberate and
measured: boundBridgeMessageType bounds the label against INVENTORY, which holds
no *_RESULT key, so a reply type would clamp to 'other' and collapse every
rejection in the protocol onto one label. Pinned by a test that asserts both
halves of that fact.
The clamp runs at the extraction site rather than in the sink. Found by the new
browser test rather than by reasoning: onOutcome is a documented seam, so a value
pulled from an untrusted payload and bounded only by the default sink is bounded
by nothing a reader of the branch can see — the test's own sink observed the raw
`NOT_A_REAL_MESSAGE`.
Watched to fail first. Disabling the dispatcher branch turns 6 of the 7 new
browser tests red; the one that stays green is the negative control (a healthy
exchange reports no validator_rejected). Removing the sixth outcome value fails
exactly one unit test; removing the INVENTORY entry fails exactly one other.
Verified: pnpm typecheck 0 errors (59s); vitest unit over src/components/AppBlocks
+ src/tests/api/track + src/server/metrics + src/server/schema 110 files / 1820
tests; test:lint-rules 46 files / 633 tests; component (chromium)
usePostMessageOutcomes.browser.test.tsx 15 tests.
Pairs with civitai/civitai-app-starters#317, which emits it.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* fix(app-blocks): round-0 audit fixes — six wrong or stale counts, three false rationales
Net -29 lines. No behaviour change except the one noted below; everything else is
prose that did not survive its own first extension. Each item's evidence was
re-verified before acting.
ARITHMETIC, re-derived rather than adjusted:
- bridgeLabels.ts said this sixth outcome grew the label product "BY 20% —
(approved apps + 1) x 47 x 2 x 6". Both halves wrong: 20% is the outcome axis
alone and the type axis grew too, and the `x 47` omits the `'other'` slot the
other two sites include. Re-derived at 50 approved apps: 51 x 48 x 2 x 6 =
29,376 against 51 x 47 x 2 x 5 = 23,970, i.e. +22.6%. Also recorded that the
product is a CEILING, not allocated heap — nothing pre-initialises the label
space, so the sixth value costs zero series until a rejection occurs.
- block-message.ts said "roughly 9x the existing renders_total product". The
only renders_total figure in the repo is ~2,040, so it is ~14x. (The base said
"7x" against 11.75x, so this one was already wrong before this branch — but it
was rewritten rather than re-derived, which is the same defect.)
STALE COUNTS the change should have touched and did not:
- bridgeLabels.ts "46-key INVENTORY" -> 47-key
- bridgeLabels.ts "wrong for THREE of the five outcomes" -> FOUR of the six
- bridgeMessageBeacon.ts "three of the five outcomes are reported above the
bridge's inbound limiter" -> four of the six; validator_rejected is the fourth,
which this branch's own comment already said
- usePostMessageOutcomes.browser.test.tsx header "The four DISPATCHER outcomes …
The fifth, no_token" -> five of the six are dispatcher-side now
FALSE RATIONALES — each would have led a reader to the wrong conclusion:
- usePostMessageOutcomes.browser.test.tsx said "`report` -> `recordBridgeMessage`
-> `boundBridgeMessageType` does the clamping; this pins that the branch routes
through it". That is the PRE-correction design: these tests supply their own
`onOutcome`, so `recordBridgeMessage` never runs and the clamp under test is the
branch's own. A reader following the old comment would conclude the branch's
clamp is dead code, delete it, redden four rows, and read the failure as "the
test is wrong".
- bridgeTelemetry.test.ts justified the new INVENTORY entry by the dispatcher's
`no_handler` bookkeeping — a path this same change makes UNREACHABLE, since the
new branch returns above it. The entry's real and only current purpose is
hostHandlerParity's one-directional compile-time gate, i.e. it is what lets this
repo bump @civitai/app-sdk past the version adding the message. Also recorded
the cost of keeping it: boundBridgeMessageType now passes
'BLOCK_MESSAGE_REJECTED' through for a forged POST instead of clamping it.
- bridgeLabels.ts called this "the fifth silence". Retracted: the SDK's own
handleMessage still drops silently and uncounted on an origin mismatch, on a
malformed envelope, and on a well-formed reply whose requestId matches no
pending request. This value covers the validator path only.
DELETED:
- 10 of the 37 comment lines in usePostMessage.ts's new branch. Five of its six
paragraphs restated bridgeLabels.ts, and the duplication had ALREADY drifted
from the original inside this same branch — which is the defect the six counts
above are. It now points at bridgeLabels.ts for the reading rules and keeps only
what is specific to the branch.
- one of the four clamp test.each rows. `{ type: 42 }` and `{}` reach the same
`typeof !== 'string'` arm; each row costs a full renderWithProviders + iframe
mount in chromium. `undefined` is kept — it is the only row exercising the `?.`.
Re-verified after the changes, since an audit fix resets the gate: pnpm typecheck
0 errors; vitest unit over src/components/AppBlocks + src/tests/api/track +
src/server/metrics + src/server/schema 1820 tests; component (chromium) 14 tests
(was 15 — one row removed). Mutation matrix re-run: disabling the dispatcher branch
reddens 5 of the 6 new tests, the survivor being the negative control; removing the
extraction-site clamp reddens exactly 1, printing the leaked value.
Pairs with civitai/civitai-app-starters#317.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* fix(app-blocks): the HELP text asserted an SDK emit budget the emitter deleted
Round-1 audit findings. No 🔴; every item is a false or stale claim on a surface
an operator reads.
🟡 THE SHIPPED HELP STRING WAS FALSE, on the axis this whole change exists to get
right. It told every scrape "the SDK budgets its reports at 30 per 10s per
transport so a sustained break UNDERCOUNTS — read it as which types and when it
started, never as a total." The emitting half deleted that budget in
civitai-app-starters#317 (its own docblock: "NO EMIT BUDGET, AND THE ONE AN
EARLIER REVISION CARRIED WAS JUSTIFIED BY A FALSEHOOD"), so the reading
instruction inverted the truth: an operator seeing >30 per 10s per transport
would conclude the count is structurally impossible and chase a forged beacon
instead of the rejection loop that produced it. Corrected in all three places it
was stated — the HELP string, bridgeLabels, and the flood test's comment.
🟡 AND THE CAVEAT THAT SHOULD HAVE BEEN THERE INSTEAD: a zero is not evidence of
health. The emitter ships inside each block's OWN bundle — every app pins
@civitai/blocks-react itself — so the series stays at zero until every app has
been rebuilt AND redeployed against a version carrying it, not merely until the
package publishes. A flat-zero diagnostic read as health is the exact failure
this outcome exists to end, and nothing said so.
🟡 THE BRIDGE_MESSAGE_COUNT_MAX DOCBLOCK'S HEADLINE OUTRAN ITS OWN ENUMERATION.
The previous commit changed "wrong for THREE of the five outcomes" to "FOUR of
the six" without touching the bullets below it, which still accounted for five,
or the prose after them, which still said "those three" and "the other three".
validator_rejected — the outcome this PR adds, reported above the limiter, and
whose emitter now has no cap either — appeared nowhere in the list a reader
consults to learn which outcomes can drive a key to the 100,000 clamp. At base
that docblock was internally consistent; this branch made it inconsistent, so it
is a regression in the artefact and not inherited rot.
🟡 The dispatched message type was a bare literal in the `if`, with no
compile-time link to the protocol — and the tests use the same literal, so an SDK
rename would silently disable the branch, return the series to zero, and falsify
hostHandlerParity's entry with nothing red anywhere. Now a constant bound with
`satisfies keyof typeof INVENTORY`; INVENTORY was already in this module's import
graph, so the binding costs nothing. It catches a rename that reaches the
inventory, not one that has only happened upstream — the upstream half is
hostHandlerParity's own gate, and the comment says so rather than overclaiming.
🟢 An in-code comment still said "deleting it reddens these four rows" above a
three-row test.each (the previous commit removed a row), and a byte figure I had
edited from ~6.7 to ~6.8 KB was never measured — replaced with "several KB"
rather than inventing precision.
Verified: pnpm typecheck 0 errors; vitest unit over src/components/AppBlocks +
src/tests/api/track + src/server/metrics + src/server/schema 1820 tests;
component (chromium) 14 tests.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* fix(app-blocks): a zero is per-APP, and the satisfies binding is narrower than it claimed
Round-2 delta findings. No 🔴; every one is an assertion on a surface an operator
or a maintainer reads.
🟡 THE ZERO CAVEAT STATED FLEET GRANULARITY WHERE THE MECHANISM IS PER-APP, and so
licensed the inference it was added to block. The counter is labelled
`app_block_id`; the series does NOT stay at zero until every app is rebuilt — it
goes non-zero the moment the FIRST rebuilt app hits a rejection, while every
un-rebuilt app's slice sits at a zero that means nothing. As written, an operator
seeing three apps report would conclude the fleet had picked it up and read the
other zeros as health — the flat-zero-read-as-health failure, one level up. Both
the shipped HELP string and bridgeLabels now say: read it WITH `app_block_id`;
for a given app a zero is "no rejections" OR "this app has not shipped a carrying
blocks-react", and another app reporting does not settle it.
🟡 THE `satisfies keyof typeof INVENTORY` BINDING IS NARROWER THAN "an SDK rename
is a type error", which is what its own comment and the PR body both claimed. It
fires only on a rename that reaches the inventory AND DROPS THE OLD KEY — and
hostHandlerParity's coverage gate is one-directional BY DESIGN, so the documented,
gate-satisfying way to track an upstream rename is to ADD the new key and leave
the old one (it carries three such keys today). In exactly that state the binding
stays green, the branch never fires, and validator_rejected returns to a permanent
zero the HELP now tells a reader to interpret as a rollout gap — the
silent-dead-branch failure the binding was added to end, surviving it.
No better guard was reached for, deliberately. The comment now states the real
boundary and says plainly that the add-and-keep window is OPEN rather than
supplying a fresh justification for a guard that does not cover it. The binding
still earns its place: it catches an outright key deletion and a typo either side.
🟡 "Two consequences" over three bullets — round 1's finding in the
BRIDGE_MESSAGE_COUNT_MAX docblock (headline outran its own list), re-made by the
commit that fixed it there, in the docblock next door. Now three, and the bullet
says so.
🟢 "The SDK shipped a 30-per-10s emit budget and then DELETED it" asserted a
release history that never happened: the budget existed only in an unmerged
revision of the sibling PR and reached no published package. Read as release
history it tells an operator that older bundles in the field DO undercount, which
is the inverted reading this whole fix removes.
🟢 An in-code comment's referent ("every row below" — the table is above).
Also corrected in the PR body: "~22 keys ahead" is the published union's SIZE, not
the gap — INVENTORY has 47 keys against 22 published members, so the gap is 25;
and the clamp mutation row said "exactly 1 red" where the ISOLATED mutant reddens
3. The earlier number was measured against a weaker mutant that removed the clamp
AND kept a hardcoded 'other' fallback — two changes, not the narrowest expression
that can be wrong. Re-measured with the clamp call alone deleted: 3.
Verified: pnpm typecheck 0 errors; unit over src/components/AppBlocks +
src/tests/api/track + src/server/metrics + src/server/schema 1820; component
(chromium) 14.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* fix(app-blocks): 25 of 47 keys run ahead, not three — and say what type='other' means here
Round-3 delta findings. All prose; zero executable change beyond one import
becoming type-only.
🟡 THE BOUNDARY STATEMENT'S OWN COUNT WAS WRONG BY 8x AND CONTRADICTED THE SAME
COMMIT'S PR-BODY EDIT. The docblock said the inventory "today carries three" keys
ahead of the published dist. Measured: 47 INVENTORY keys against the 22 members
the installed @civitai/app-sdk@0.14.0 block->host union declares, so 25 are ahead
— while that same commit edited the PR body to say 25. One commit asserted both
numbers for one quantity, and the wrong one was the one a maintainer meets first.
It is load-bearing, not decoration: the docblock's argument is that add-and-keep
is the documented way to track a rename, the window is open, and no better guard
was reached for. A reader told the window is a three-instance curiosity weighs
that differently from one told it is the state of 25 of 47 keys — i.e. the norm.
⚠️ And the "three" was inherited from hostHandlerParity.ts:56-58's parenthetical,
which is itself stale: it names CANCEL_WORKFLOW, REQUEST_SIGN_IN and
REQUEST_CONSENT as the ahead-of-published keys and ALL THREE are present in
0.14.0, so zero of them is ahead. The docblock now says to measure rather than
read that list. (Correcting the parenthetical is base rot, not this branch's.)
🟡 WHAT type='other' MEANS ON THIS OUTCOME WAS NOWHERE, and both surfaces implied
the opposite. The HELP said the type "is the block->host REQUEST left hanging" and
separately that an unknown type clamps to 'other'. Against the emitter's current
head, 'other' is also what it sends for a rejected host PUSH (nothing was awaiting
it, so nothing hangs) and for a reply it could not attribute. So an operator
applying those two sentences to a validator_rejected{type="other"} row concludes
an unrecognised request is hanging to a 30s timeout — both halves wrong, and the
two cases share the bucket. Stated on the HELP string and in bridgeLabels: 'other'
is the one value on this outcome you must NOT read as "a request is hanging".
🟡 The docblock also overstated the consequence in the SAFE direction, which is
still wrong: a renamed-and-kept key does not silently zero the signal, it files it
under no_handler with the NEW type (unclamped, since that would be an INVENTORY
key). Harder to notice than an absence, not easier.
🟢 The INVENTORY import is used only in `typeof INVENTORY`, so
@typescript-eslint/consistent-type-imports (error severity) reports it. It blocks
nothing — lint.yml gates on ADDED files and this one is modified, and the
in-cluster pr-check pipeline runs typecheck and tests, not eslint — but it is a
one-token fix and the annotation would have sat on the diff. Now `import type`.
Verified: pnpm typecheck 0 errors; unit over src/components/AppBlocks +
src/tests/api/track + src/server/metrics + src/server/schema 1820; component
(chromium) 14. The 25-vs-3 count was re-derived independently by enumerating both
sets, not taken from the audit.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* fix(app-blocks): type='other' is OVERLOADED four ways, and the clause said two
Round-4 delta finding, and the last one on this PR — see the closing note.
🟡 The `type='other'` clause added last round asserted an exclusivity the same PR's
code contradicts, and it contradicted the line two above it. It said 'other' "DOES
NOT MEAN an unrecognised type", directly below the sentence explaining the clamp
that produces exactly that case. FOUR things reach the bucket, not two:
(a) the SDK rejected a host PUSH — nothing was awaiting it, so nothing hangs;
(b) the SDK could not attribute the reply to one of its pending requests;
(c) the block named a type this host's INVENTORY does not declare, or named
nothing — usePostMessage's own boundBridgeMessageType clamp, which a browser
test in this very PR exercises with NOT_A_REAL_MESSAGE;
(d) the SDK clamped an undeclared requestType while a request genuinely DOES hang.
So 'other' cannot be read in EITHER direction: not as "a request is hanging" (a and
c may hang nothing) and not as "nothing is hanging" (b and d may). The previous
wording gave an operator a protocol diagnosis — "a rejected push, or an
unattributable reply" — for what may be a real exchange the emitter mislabelled.
⚠️ (c) is reachable today only for a malformed or undeclared value: the SDK's
47-entry type array and this repo's 47-key INVENTORY were measured EQUAL at both
current heads, so no legitimate protocol type clamps. Recorded as a measurement
across two moving repos, not as an invariant — the docblock 20 lines above is
precisely about drift in that relationship.
NOT FIXED, deliberately, and named so it is visible rather than absent:
hostHandlerParity.ts:685-686 carries a SECOND copy of the stale ahead-of-published
parenthetical (CANCEL_WORKFLOW, REQUEST_SIGN_IN, REQUEST_CONSENT "today" — all
three are in 0.14.0, so zero of them is ahead), and last round's warning points at
only the first copy. Both are pre-existing `main` text that no commit on this
branch touches. Correcting them is a one-line edit somebody should make; widening
this PR to do it would put an unrelated doc change in a bridge-telemetry diff.
🔴 CLOSING THIS PR'S AUDIT LADDER HERE, on a stated criterion rather than a feeling.
Rounds 3 and 4 both changed ZERO executable lines on this PR — round 3's only
non-comment edit was an import becoming type-only (erased at build), round 4's is
comment text and one HELP literal. The attribution gate's two-consecutive-
zero-payload condition cannot fire mechanically because comments in payload files
count as payload lines, but the condition it exists to detect is met: the rounds are
auditing the ladder's own prose, not the PR. The code has been unchanged and green
since
|
||
|
Zachary Lowden
|
4642bbfc79 |
fix(search): stop QuickSearchDropdown's index selector deselecting itself (#4975)
Mantine's `Select` is deselectable by default, so clicking the currently
selected option clears it and hands `null` to `onChange`. `QuickSearchDropdown`
passes that through to `handleTargetChange`, which falls back to `models` — and
the caller goes on reading the picked entity as whatever type its
`supportedIndexes` named, while the dropdown is now searching a different index.
This is most visible where the caller offers a single index, because then the
only clickable option is the one already selected, and a stray click is enough.
`CosmeticShopItemUpsertForm` is that case: `supportedIndexes={['users']}`, under
a "Funds Distribution" label reading "The cost of this item will be split evenly
among the selected users", writing what is picked into `meta.paidToUserIds`. So
the index the search is pointed at is not a display detail there.
`allowDeselect={false}` is the Mantine-level fix: the selector stops emitting
`null` at all. `AutocompleteSearch`'s equivalent selector already carries it.
Not tested here, said out loud rather than left as a silent omission: the
behaviour needs a rendered Mantine `Select` to observe, which is the browser
tier, and `pnpm run test:component` does not run on the machine this was written
on (its pinned Playwright store carries a different chromium-headless-shell
build than the runner asks for, and reports "no tests" rather than failing). A
check on the prop's spelling in the source would be an invariant guard, not
regression coverage — it would be green with or without the defect present in
any other shape.
|
||
|
Zachary Lowden
|
854595f7d3 |
fix(search): rebuild the dropdown search provider when its index changes, and carry the typed text across (#4953)
* fix(search): drop a dropdown search whose filters target the previous index
The header autocomplete and the quick-search dropdown render
`<InstantSearch indexName={...}>` with no `key={indexName}`.
react-instantsearch-core calls `helper.setIndex(indexName).search()` in its
RENDER body, and `<InstantSearch>` renders before its children, so a target
switch fires a search while the helper still carries the previous target's
`<Configure filters>`. The models filter set then lands on another index and
the search backend answers 400 `invalid_search_filter`, which the resilient
client swallows into an empty dropdown. Measured in production RUM: hundreds
of such rejections a day, dominated by models-only attributes arriving at the
images index and by `poi` arriving at indexes no code path ever intends it for.
`SearchLayout` fixes this with `key={indexName}` and says so in a comment. The
dropdowns cannot copy that: remounting clears the query the user is typing.
So the request is rejected in the client instead. `withSearchFilterGuard`
validates each request's filter, facet and numeric-filter attributes against
what its target index declares in `src/server/search-index/filterable-attributes.ts`
and resolves a doomed request to the ordinary empty-result shape without
sending it. Valid requests in the same batch still go to the backend and keep
their position in the response. No UX change: a rejected request already
rendered empty.
It is not silent. A rejection still pushes a Faro RUM error, under its own type
(`SearchFilterAttributeError`) so a locally-rejected request stays tellable
apart from a backend-rejected one (`MeiliSearchQueryError`). Note that a
population which used to beacon under the old type now beacons under the new
one, since it no longer reaches the backend at all.
The guard covers exactly the leaks that name an attribute the new index cannot
filter on — the ones that produce a 400. When the previous target's attributes
are all declared on the new index the stale request is valid there and is sent,
missing whatever clauses that target never built; that direction returns 200,
appears in no error signal, and no attribute check can see it. The module
documents this rather than reading as though it closes the class.
Wiring: the three browser search clients were three hand-rolled compositions
with the same 18-line empty-query short-circuit copied into two of them and
absent from the third. They now come from one `createSearchClient` factory, and
each is exported from its own module so the wiring is reachable from a node
test. That is load-bearing for the tests, not tidying — while the clients were
built inside the `.tsx` files the only possible check was a grep of the
component source, and a source check cannot tell a guarded client that is USED
from one that is merely constructed.
Tests: every shipped client is exercised through its own export, asserting the
negative — the doomed request must not be SENT — plus a positive control that a
filter set built for the index it targets still is. With the guard bypassed at
the factory, those three tests fail on that assertion. A derived ledger
enumerates every module constructing a search client and requires each to be
either the guarded factory or an exclusion whose stated reason is asserted, so
it fails when the population grows and when an exclusion stops holding.
* fix(search): rebuild the dropdown search provider when its index changes
`SearchLayout` keys its `<InstantSearch>` on the index name and says why:
"Needs re-render. Otherwise the prev. index will screw up the app." The header
autocomplete and the quick-search dropdown never got that key, so they carry the
defect the comment describes.
react-instantsearch-core calls `helper.setIndex(indexName).search()` in its
RENDER body (`lib/useInstantSearchApi.js`), and the provider renders before its
children. So on a target switch the search fires against the NEW index while the
helper still holds the PREVIOUS target's `Configure` parameters — the children
that own `filters` have not re-rendered yet. Keying the provider makes React
build a fresh one instead, and the children mount their parameters onto it
before it searches.
This is additive to the request-level guard already on this branch, and it
covers a direction that guard structurally cannot. The guard drops a request
naming an attribute its target index does not declare. When the previous
target's attributes are all declared on the new index — `articles` and
`collections` are subsets of `models` — the stale parameter set is perfectly
valid there, so it is sent, and the clauses the new target builds only for
itself are simply missing from it. That answers 200 and appears in no error
signal. The key closes it at the source: there is no stale parameter set to
send.
The reason the dropdowns could not just copy `SearchLayout` is that remounting
clears the text the user is typing, which lives inside the provider's subtree.
So each root now holds that text in a ref ABOVE the keyed boundary and the
remounted input is seeded from it (`useCarriedSearchText`). Seeding is not only
cosmetic: a rebuilt helper reports an empty query, so the seeded text differs
from it, and that difference is what makes each component's existing "push the
text into the helper" effect fire again — the search is RE-RUN on the new index
rather than the input merely re-displaying the old text. An empty carrier falls
back to the helper's own query, which is what a first mount did before.
Every write to that text goes through the setter the hook returns, so the
carrier can never hold a value the input no longer shows.
Tests, in `src/components/Search/__tests__/dropdown-index-remount.test.ts`:
- A ledger of every `<InstantSearch>` root in `src/`, derived from the tree so it
fails when the population grows or shrinks. Each keyed root must key on the
very expression it passes as `indexName` — not merely carry some key, which
could disagree with the index. `CollectionSelectModal` is the one exclusion and
its stated reason is asserted, not taken on trust: its index is a fixed member
of `searchIndexMap`, so there is no switch to survive.
- The carry, asserted structurally on both dropdown roots: the ref is declared
above the provider, threaded into the content component, and read through the
hook — and the `useState(query)` shape it replaced, which comes back empty on
a remount, is banned.
- The hook's behaviour, exercised in the node tier against a real React remount:
text typed before a key change survives it, and comes back differing from the
rebuilt helper's empty query. The negative control is the same tree seeded the
old way, which loses the text — without it, a harness that silently never
remounted would pass every other assertion while asserting nothing.
Matrix: the four structural tests are RED at `bbeffcf71e` (no `key` prop, no
carrier) and green at HEAD. The behavioural and ledger tests are new-behaviour
guards, not regression tests, and are green at both. Three mutants of the hook
were each killed by their own assertion, with the source restored by digest
after each and a green re-run after the sweep.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* fix(search): keep the chosen category when the search provider is rebuilt
Review follow-up to
|
||
|
Zachary Lowden
|
a75fb42290 |
fix(blocks): an unreadable post subject is no longer reported as "posting from apps is not enabled" (#4976)
`authorizeBlockPostRequest` passed an unhydratable session subject straight
into `isAppBlocksPostCreationEnabled({ user: subjectUser ?? undefined })` --
the no-entity arm. A segment-scoped rollout cannot match a no-entity eval, so
it answers false, and a failed identity read was rendered to the viewer as
"posting from apps is not enabled". Two different facts, one message, and only
one of them is about permission.
Refuse an unhydratable subject on its own terms before the flag is consulted,
matching the two sibling guards in this router family. The flag denial keeps
its message unchanged, so the two stay separable.
This does NOT widen who may post: an unreadable subject is still refused, and
a dedicated case pins that it is still refused under a base-enabled flag. It
also does not assert a mechanism for any particular production refusal -- a
subject carrying a stale isModerator and a transient flag-evaluation failure
produce the identical observable and neither is excluded.
- BlockPostRequestAuth.subjectUser is now non-nullable, so widening it back is
a type error rather than a silent re-conflation.
- New counter civitai_app_block_post_subject_refusals_total{surface}, following
the existing emitters in app-block-runtime.metrics.ts. A log line would not
have worked: application-container logs are not collected for this
deployment, which is why the original refusal was unattributable.
- Watchlisted as post-subject-refusal -- losing the branch to a bundler falls
back to the no-entity arm, which returns the flag's BASE value.
Regression matrix (5 of 6 new cases): red at
|
||
|
Justin Maier
|
2df0241a4f |
feat(test-cache): skip unit test files unchanged since they last passed (#4971)
* feat(test-cache): shadow-record what a content-keyed result cache would skip Phase 1 of a test-result cache: nothing is skipped. A vitest reporter keys each test file on the content of every first-party module it depends on, plus the inputs no import records (lockfile, vitest config, tsconfig, node, platform), and records files that passed in full. A later run whose key matches is one the cache WOULD skip; if that file then fails, the key missed a dependency and the run is logged as a false skip. Dependencies come from vite's server-side ssr module graph, not diagnostic().importDurations: on a fixture, importDurations missed an `await import()` made inside a test body, and the ssr graph caught it cold and warm. The graph also leaves out the subtree behind a vi.mock factory, which never executes, while keeping the mocked module itself. The store lives in the COMMON git dir, shared by every worktree, and keys use repo-relative paths, so one tree's green run covers every tree whose files are identical. Files that read the filesystem, spawn processes, or import a non-literal specifier always run (243 of 1,880, 6.5% of modelled worker time). The queue gains a hot-configurable cache mode (`test config --cache shadow`, TEST_CACHE_MODE in the skill .env). In shadow mode a queued unit run gets the primary checkout's reporter appended, plus `--reporter=default` when the caller named none, so turning it on never strips a run's normal output. Fixture sequence, each step as predicted: cold 0 skipped; unchanged 1/1; runtime-imported dep changed 0; unchanged again 1/1; dep behind a mock changed still 1/1; env-driven failure with an unchanged key flagged as 1 false skip. 89-file yardstick, cold then warm: 89/89 passed both times, warm would skip 85/89 (98% of worker time), 0 false skips. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * feat(test-cache): skip unit test files unchanged since they last passed Turns the shadow recorder into a real cache. Before a queued unit run, a custom sequencer re-fingerprints each test file's recorded dependencies and drops the files whose key still matches a recorded pass; vitest runs exactly the list the sequencer returns. Reusing the OLD dependency list is sound: gaining a dependency means editing a file already in the list, which changes the key. A key covers the test file, every first-party module it imports (from vite's module graph, in whichever environment loaded it, so happy-dom files count too), every file or directory it read at runtime (a setup-file fs tracker, directories fingerprinted by their whole subtree), and the lockfile, configs, node, platform, vitest and the cache's own code. Records are shared by every worktree through the common git dir, up to 8 per test file, so two trees on different code both stay fast. Still always run: files that spawn, glob, or import a computed specifier (19 files, 0.7% of modelled worker time). Known blind spot: environment variables are not in the key. A random ~5% of skippable files run anyway. If one fails, the cache predicted a pass it could not deliver; it writes TRIPPED.json and runs everything until a human removes it. Modes off|shadow|on are hot-configurable on the queue (`test config --cache on`); never on in CI, never applied to a named-files run. Measured, 89-file yardstick: cold 294s, warm 24s (85 skipped, 3 re-sampled, 0 false skips). Fixture scenarios: runtime-imported dep edited, fixture file edited, dependency of a happy-dom test edited each re-ran exactly the affected file; an env-driven failure was flagged as a false skip and tripped the cache. Fixes found by those checks: a fully cached run exited 1 ("no test files"); 44 happy-dom files were never cached; a builtin heuristic dropped top-level directories like `src` from the key. Full unit suite, cache off: Test Files 1 failed | 1904 passed | 3 skipped (1908); the one failure is rest-error-envelope-ledger, which fails identically on origin/main CI. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(test-cache): close three false-skip paths found by review Adversarial review of |
||
|
Justin Maier
|
fdc1437da1 |
fix(users): scrub name, profile and links on account deletion; stop persisting the OAuth name (#4970)
* fix(users): scrub name, profile and links on account deletion; stop persisting the OAuth name Account deletion is a soft delete, so no FK cascade fires. On prod, of 1,330,849 deleted accounts, 733,857 still carried `name` and 192,791 a UserProfile row. - apps/auth: stop writing User.name on OAuth signup. It is unverified, user-controlled data that outlives a soft delete. It still seeds the generated username from the transient profile. - deleteUser: also null `name` and delete the UserProfile row and every UserLink row, inside the transaction. - Move the paddleCustomerId purge out of the transaction into a `finally` after the subscription cancels. cancelSubscriptionPlan falls back to reading it, so while it was nulled in the transaction that fallback could never fire on a deletion. The `finally` keeps the purge unskippable when an earlier unwrapped await throws. customerId is deliberately NOT purged here: deleteUser's own cancelSubscription triggers a Stripe webhook that resolves the user by customerId and throws before deleting the CustomerSubscription row, so nulling it would leave the row `active` forever. It is purged by the GDPR scrub, which must reach Stripe first. Pinned by a test named for it. Staff accounts created after this change file NCMEC reports without a reporter firstName; the live report path reads `name` for nothing else. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(users): keep paddleCustomerId in the deletion transaction; harden the scrub tests Reverts the paddleCustomerId ordering change from the previous commit. Moving the null after the subscription cancels let cancelSubscriptionPlan's no-row fallback run, but with seven live Paddle subscriptions (none on a deleted account) it only added a live Paddle API call per deletion, a false cancel-paddle-subscription error for nearly every one, and an unbounded wait on a client with no timeout. paddleCustomerId is nulled inside the transaction again, exactly as on main, and the try/finally that existed only to protect that later null is removed, restoring main's tail. deleteUser's net change is now only the GDPR scrub: null `name` and delete the UserProfile and UserLink rows inside the transaction. Tests, from the five-lane review: - the customerId scan serialises BigInt instead of falling back to String(call), which turned an object into "[object Object]" and reported a false absence; a CONTROL pins it - the scan's uncovered write paths are listed (kyselyWrite, updateManyAndReturn), alongside pgDbWrite and interactive transactions - tests that only made sense for the reverted ordering are removed, and one pins paddleCustomerId inside the transaction Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * test(users): pin the soft delete inside the transaction; cover interactive transactions From the test-lane re-review of #4970: - Nothing asserted that the soft-delete user.update is itself one of the $transaction ops. Awaiting it outside the array passed every test, including the ones named "inside the transaction". Both the transaction test and the paddleCustomerId test now assert its identity in the ops array. - A test-local $transaction override returned its argument unrun, which hid customerId writes made inside an interactive transaction. The shared mock runs the callback, so the override is removed and a CONTROL proves the scan now sees that route. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
Zachary Lowden
|
85a3786116 |
test(rest-envelope-ledger): record admin/huggingface-import.ts, fixing main's red (#4973)
The REST error-envelope ledger fails on main and therefore on every open PR,
because GitHub Actions builds the merge commit: the offender set gained
`admin/huggingface-import.ts`, added by
|
||
|
Justin Maier
|
8540f2fe8d |
perf(shop): count sold per page instead of aggregating the whole purchases table (#4974)
* perf(shop): count sold per page instead of aggregating the whole purchases table Prisma resolves a relation _count by aggregating all of UserCosmeticShopPurchases once per query, so every shop read paid for the whole table regardless of page size, and MostPopular paid twice (value and orderBy). The display reads now take the count from one query restricted to the ids they returned. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * test(shop): pin the sold-count statement and the call-site selects The fake answered by table name alone, so a renamed alias, a dropped int cast or the wrong WHERE column all stayed green. Rows are now projected onto the statement's SELECT list and the statement is pinned once. Each read that dropped the whole-table _count now asserts it at its call site, and getCreatorShopManageItems gets its first tests. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * test(shop): pin per-item sold mapping on multi-item pages and the resale filter Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
Justin Maier
|
8113e62777 |
fix(metrics): exclude metric-suppressed accounts from Postgres reaction sums (#4959)
* fix(metrics): exclude metric-suppressed accounts from Postgres reaction sums The reaction counts shown on posts, articles and bounty entries are summed in Postgres from ImageReaction/ArticleReaction/BountyEntryReaction with no exclusion predicate, so they count accounts the reaction-abuse detector already suppressed from every other reaction surface. Unlike the ClickHouse totals these never decay: the jobs recompute the same unfiltered sum from the same rows, so the numbers stay wrong until the queries filter. Post has two live reaction queries, not one — post.metrics.ts delegates to post.metrics-old.ts whenever the simplified-post-metrics flag reads false, which includes Flipt being unreachable. Both filter now. The jobs read the list through a new getMetricExcludedUserIdsOrThrow rather than the existing lenient reader. The lenient one degrades to [] so the reaction milestone keeps firing during an outage; a metric job doing that would write an unfiltered total that nothing later recomputes, because a job only revisits an entity that receives another reaction. Rejecting instead leaves the cursor and the queue untouched in createMetricProcessor, so the window is recomputed next run. Answer and Question reaction metrics have the same shape and are left alone — out of the scope this was asked for, and named as exemptions in the guard rather than skipped silently. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(metrics): zero the entities whose reactions are ALL excluded, and filter the milestones too Five review lanes over 9cb53ed. Everything here is a finding they raised, each verified against the code or the replica before acting on it. Filtering the aggregate was not enough. An entity whose remaining countable reactions are zero produces NO ROW from a GROUP BY over the reaction table, and a missing row means "no change" to every writer downstream — so the pre-exclusion total survived even a full recompute. Measured on the replica: 18 of 594 affected articles and 338 of 1,154 affected bounty entries are in that state. Post and article seed zeros into ctx.updates before the aggregate overwrites them; bountyEntry has no JS intermediate, so its CTE now drives from the affected ids with a LEFT JOIN. That LEFT JOIN needed timeframeSum to be NULL-safe. Its leading `WHEN NOT (cond) THEN 0` is NULL for an unmatched row and falls through to the AllTime arm, counting a reaction that is not there. `(cond) IS NOT TRUE` is identical for every inner-joined caller. Verified on the replica: with the old form a fully-excluded entry returns 1 heart / 1 like, with the new form 0. The seeding was only safe once a pre-existing bug was fixed. Both post jobs bound their chunk with `BETWEEN ids[0] AND ids[ids.length - 1]` over an unordered Set, so roughly half of all chunks matched nothing. Seeding zeros into a chunk that matches nothing would have written zeros over real counts. The chunk is sorted now. The article and bounty-entry milestone notifications counted unfiltered. Before this work both halves were unfiltered and therefore agreed; filtering only the displayed half would have manufactured, for those two entities, the exact display-vs-notification divergence this defect is a sibling of. They use the LENIENT reader on purpose — a notification should degrade to the old count, not to silence. The guard now covers the notifications too, and three mutations that were demonstrated to pass against it: a `.catch(() => [])` on the strict read, a filter spliced inside an SQL line comment, and a wrong column argument. The last is fixed by construction — the column is hardcoded rather than passed, since a raw-SQL parameter beside an integer guard reads as though the guard covered it. Also: the strict reader now reports the outage to Axiom instead of surfacing only as a generic job error, and a comment claiming coercion parity with metric-reaction-repair.service.ts was false and now says so. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * test(metrics): assert the emitted SQL, not that a token appears in the file Round 2 of review demonstrated six mutations that pass the source guard, each measured rather than argued: a second unfiltered count of the same table in the same template literal; a consistent alias swap so `r."userId"` names the image owner instead of the reactor; `.then().catch()` and a plain try/catch around the strict read; a key moved into the exemption list, which had no length pin; a `/* */` comment around the splice; and the bounty-entry filter moved out of the LEFT JOIN's ON into a WHERE, which collapses it to an inner join and restores the no-row defect the rewrite exists to fix. A source guard checks that a token appears in a file. It cannot see what the composed statement does, which is why six separate textual assertions each missed one of these in their own way. `post-reaction-metrics-sql.test.ts` calls the real getReactionTasks with a fake pg that captures every statement, and asserts on the SQL that was actually sent — one test that catches the alias swap, the swallow, the commented splice, the second count, the missing sort and the missing zero-fill. Its fixture crosses the 30,000-image chunk boundary and returns a LOWER run of post ids second. That is not decoration: the first version of the sort control PASSED, because `getAffected` sorts its own return, so a single-chunk fixture cannot produce the out-of-order set the bug needs. It was an assertion that could not fail for the case it was named after. The guard keeps the cases it can see, hardened: block comments as well as line comments, a requirement that the filter be built from a direct `await` of the reader rather than any expression with somewhere to swallow a rejection, a requirement that the alias `r` is bound to the reaction table and to nothing else, a shape pin on the bounty-entry ON clause, and a length pin on the exemption list. Two fixes to the round-1 fix. The Axiom report was effectively unreachable: one latch shared by both readers, and the lenient one runs on every reaction toggle, so it wins every race and the only line for an incident would say a notification degraded while the metric jobs stalled silently. Keyed per outcome now. And `post.metrics.ts` chunked image ids from a ClickHouse query with no ORDER BY under the same inverted-BETWEEN bug fixed one block below; post.metrics-old.ts has that ORDER BY, the live path did not. Both `!clickhouse` branches had no test at all, in any file, because every other test supplies a client. Backfill note, recorded here because a squash merge takes commit messages and not the PR body: this does NOT close ClickUp 868m6vftv. The filter only corrects an entity the next time it is affected, so the already-wrong rows stay wrong until a backfill recomputes them. That ships separately against main, not stacked on this branch. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(notifications): keep the ClickHouse client out of the client bundle The milestone filter pulled the exclusion-list reader — and through it the ClickHouse client — into the notification processor files. Those files are in the _app client graph, because prepareMessage renders there. no-server-infra-in-app-graph caught it: it ran and failed in 44ms before the full suite was killed by a daemon restart, so the one real result that run produced was this. A lazy import inside prepareQuery does not fix it. The guard says why and is right: a dynamic import() still compiles the chunk into the client bundle. So the processors no longer read the list at all. The server-only runner, send-notifications.ts, reads it once per job run with the lenient reader and passes it through NotificationProcessorRunInput, which prepareQuery already receives. The pure SQL builder moves to ~/shared/utils/excluded-reactor-filter.ts with no imports, and metrics/metric-helpers re-exports it. One read per run instead of one per processor. The field is optional and the two reaction milestones default a missing list to [], because degrading to the pre-exclusion count is already the posture these notifications want, and ten existing processor tests construct the input without it. That makes the runner the single point deciding whether milestones are filtered at all, so the guard now pins it: it must read with the lenient reader and pass the list to every prepareQuery, and a processor may import neither reader. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * test(notifications): pin the runner-to-milestone hand-off by behaviour, not spelling Round 3 of review measured eight mutations that ship the reaction milestones unfiltered with the source guard green, because the guard pinned how the hand-off is SPELLED rather than what is passed: a shadowing `const excludedUserIds = []` inside the batch loop, `excludedUserIds.length = 0` after the read, a spread that overrides the shorthand, a processor that ignores its input, a filter built but never spliced, and the filter moved into the `affected` CTE — valid SQL that narrows which entities are revisited while the COUNT stays unfiltered. send-notifications.excluded.test.ts runs the real job with the real processors, captures the SQL they send, and asserts the filter lands in the CTE that COUNTS. All six of the lane's mutations fail it by name; the processor-level ones fail only their own milestone. It also asserts no processor logged an error, because the runner swallows a per-processor throw and a milestone whose SQL no longer builds would otherwise read as "no query". NotificationProcessorRunInput.excludedUserIds is now required but nullable rather than optional. The milestones default a missing list to [], so a second runner that simply omitted the key would ship every milestone unfiltered without a sound. Required, tsc rejects it. Verified that the protection is real rather than vacuous: typecheck does not read __tests__, which is why ten fixtures building this input still pass, so the control was on the production caller — dropping the key from send-notifications.ts fails with TS2345 at line 49. Plus direct tests of the shared SQL builder's empty-list and non-integer branches, which only the non-empty path had reached. With the integer guard removed, the three non-integer cases fail. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * test(notifications): bound the counting-CTE slice instead of failing open countingCte sliced from `affected_value AS (` to the next `), ` and, finding none, fell back to the end of the query. Round 4 of review measured that as a green mutant: put the next CTE's name on its own line and move the filter into `reaction_milestone`, a CTE that counts nothing, and the slice ran on far enough to include it. The helper now bounds the slice by the next CTE header and requires it to find one. That mutant is red now. The same round confirmed the other six mutants go red on the assertion named for their defect rather than incidentally, and found an alias-revert mutant (bounty table back to `br` beside a filter that names `r`) that this test does not catch because it never executes the SQL. It does not need to: no-unfiltered-reaction-metric-sum's alias assertion fails on it, verified. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * test(notifications): give the pgDb mock its full export set pgDbMock.parity requires every inline `~/server/db/pgDb` factory to list the complete export set, because kyselyDb.ts destructures all of them at module-eval time and Vitest throws on any omitted name — during module LOAD, so a suite that reaches it dies at collection and reports zero tests rather than failing. The job test listed only pgDbRead. It passed because kyselyDb is not in its graph, which is the case the guard exists to stop depending on. Caught by the full suite, the only thing that runs it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
Zachary Lowden
|
165da40294 |
feat(app-blocks): author-fee viewer-charge path (slice 2b, dark) (#4958)
* feat(app-blocks): slice 2b base — the author-fee settlement rail, rebased onto the merged ledger Slice 2a (the accrual ledger) merged as |
||
|
Justin Maier
|
aed40bc958 |
fix(reactions): show an unknown reaction count instead of a silent zero (#4962)
* fix(reactions): show an unknown reaction count instead of a silent zero A reaction count the ClickHouse read could not resolve was indistinguishable from a real zero by the time it reached the component, and on a card both rendered as nothing at all: `ReactionButton` drops a zero-count badge when `noEmpty` is set, which is the default. A metrics outage therefore read as "nobody reacted". Carry the distinction as one additive `statsUnknown` flag on `ImageV2Stats` rather than nullable counts. `getImageMetricsObject` builds all seven fields of an image from a single ClickHouse row, so a count can never be unresolved on its own - a per-count shape would be able to represent states the read cannot produce, and a null could reach `ReactionButton`'s `initialCount + 1` and surface a fabricated number. Seven call sites had each derived the stats block by hand as `match?.x ?? 0`. They now share `toImageV2Stats`, so the absent-vs-zero decision has one definition instead of seven. On a card an unknown count renders a "Couldn't load" badge in place of the empty row; expanded, the badges render an en dash and never a number, including after the viewer reacts. Not covered: /api/v1/images builds its stats from event-engine-common's ImageStats, which has already collapsed an absent row to zero. That path is JSON only and reached by no rendered feed, so it is pinned to false with a comment; carrying the flag there needs the submodule shape changed first. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(reactions): cover the metric timeout, gate the unknown badge on a flag Review of c443a291e4 found the first commit covered only one of getImageMetricsObject's two failure exits. The outer catch returns {}, so ids are absent and read as unknown. The soft timeout did not: the loop after withTimeoutFallback wrote an all-null entry for every requested id, so `match` was present and a timeout still rendered as a real zero. That is the documented, instrumented path (the CH read runs ~4.6s p50 against a 3s budget), not an edge case. Both exits now leave ids absent. The same review found three more gaps: - The readonly feed card forces `hasReactions` so an unknown count survives the readonly early-return, and nothing tested it; deleting the line left every case green while restoring the original bug on that surface. - BuzzTippingBadge renders outside the placeholder branch and read its count off the same unresolved row, so an unknown card showed "Couldn't load" beside a confident 0. It now renders a dash too. - toImageV2Stats asserted four of nine fields, so a swapped mapping survived; it feeds seven call sites. The unknown state is gated behind `reactionCountsUnknown` (Flipt key `reaction-counts-unknown`). Off renders exactly the previous behaviour: the server still marks the counts, the component ignores them. A missing flag reads as off, so this ships dark until the flag is created. Read through `useFeatureFlags` rather than `useOptionalFeatureFlags`: 51 suites mock the provider by hand and list only the former, so importing the latter killed them at link time with zero tests collected. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(reactions): keep the unknown-count flag dark for moderators too `reactionCountsUnknown` was declared `availability: ['mod']`. The client's `isEnabledSync` swallows "flag not found" and falls through to that static list, so every moderator would have seen the badge from deploy, before the flag existed in Flipt. `[]` keeps it off for everyone until Flipt says otherwise. A seam test now runs the real evaluator with only the Flipt edge stubbed, since the browser tests mock the flag hook and cannot see this. Also closes three gaps in the kill switch and the failure path: - The flag-off cases never mounted the tip badge, so a tip that read the raw server mark instead of the gated one passed them all. They now pass `targetUserId`. - No case combined flag-off with `readonly`. - The outer catch's `{}` was claimed in a comment and pinned by nothing. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * test(reactions): prove the unknown-count flag is dark for every audience The seam test was named for "everyone" but built only free users, so any paid-tier availability (`member`, `founder`, `bronze`, `silver`, `gold`) or `granted` would have leaked the badge to that audience at deploy with every case green. The static fallback matches `user.tier` exactly, so one paid user does not stand in for another; each tier is its own case, each also a moderator holding an explicit grant, plus an anonymous visitor. All nine non-empty availability values the fallback understands now turn at least one case red. The file also now primes the lazily loaded Flipt module, as its peer seam tests do, so the OFF cases exercise "Flipt says the key does not exist" rather than "Flipt not loaded yet", and the moderator case pins that Flipt was asked for this key. The flag-off + readonly browser case now asserts the row has no children, so it no longer depends on the tip badge being mounted to see a regression. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
Justin Maier
|
46ee3b5fa9 |
fix(posts): render the drafts/scheduled feed in publish order (#4963)
* fix(posts): render the drafts/scheduled feed in publish order Masonry places each card in whichever column is currently shortest, so the server's descending publish order only survives down a column. Read across a row it comes out scrambled, which is what the reporter saw: 6h, 7h, 8h on one row then 5h, 4h, 3h on the next. Swap the draftOnly feed onto MasonryGridVirtual, the row-major virtualized grid articles, models and bounties already use. It consumes the same MasonryProvider the page mounts, so nothing else moves; the published feed keeps masonry. A scheduled post also showed no time at all, only a clock icon whose tooltip said "Scheduled". It now shows the time itself, and an unscheduled draft says so rather than showing nothing. DaysFromNow ticks every 15s per mounted card; Countdown ticks every second under an hour, which is where a publish queue sits. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(posts): fill the card with its media, and tighten the order tests Review found the media was never constrained to the cell the grid sizes: the card link is a flex item with no flex-grow, so its height came from the image and `.image`'s height: 100% resolved against auto. A landscape draft left dead background under the image with the badge floating in it, and a portrait one clipped harder than masonry ever did. Tests, all with the mutation that reddens them: - assert row-major GEOMETRY, not just document order. `direction: rtl` on the row keeps the ids in sequence while the queue reads backwards, and printed `expected [ 666, 350, 34 ] to deeply equal [ 34, 350, 666 ]`. - pin masonry's actual permutation on the published feed instead of "not the server order", which accepts any other wrong grouping. - bound the mounted window from below as well: a collapsed one row window is a feed that goes blank on scroll and satisfies every upper bound. - ten posts, so the grid's ragged last row is exercised. - cover the badge itself, which the ordering test mocks away. Forcing it to always say Draft prints `expected 'Draft' to contain 'in 3 days'`; forcing it always on prints `expected 'Draft' to be null`. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * feat(posts): order the drafts view as a publish queue and drop its sort picker From UI review: the drafts view read soonest-publishing LAST, because it inherited the feed's Newest/Oldest sort (drafts first, then scheduled posts furthest-out first). The picker does not mean anything for a queue, so the drafts view now has one fixed order: unscheduled drafts newest first, then scheduled posts soonest first, and the page no longer shows the picker there. Those are two directions and the keyset cursor takes one, so scheduled times are mirrored about the epoch into a single descending timestamp key, below every draft (which sit a millennium up). Checked against the dev database for a user with 245 drafts and 510 scheduled posts: rows 1-245 are drafts newest first, row 246 onward is the queue soonest first, and a keyset page taken across that seam re-serves the cursor row and continues without a skip. Also from review of the previous round: the row-major check selected the first row by shared `top`, which let a layout flowing DOWN the columns pass, since the head of every column shares the first card's top. It now takes the first row by DOM index, and a case feeds it a CSS-columns fixture to prove it is rejected. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(posts): keep the drafts queue key in range, and make its test see the SQL Review of the previous commit found three things. The mirrored-timestamp key left timestamp range for a far-future publishedAt (nothing bounds it on write; only the schedule modal caps it, client-side): year 9999 raised `timestamp out of range`, taking down the owner's and any moderator's drafts tab. The key is now epoch milliseconds, negated for scheduled posts and lifted by 1e15 for drafts, as float8 (exact below 2^53, which a year 9999 value is). Re-checked on the dev database: same order, and a keyset page across the draft/scheduled seam continues without a skip. `?section=draft&sort=Recently Added` began to 400 once the client stopped rewriting the sort; the drafts branch now runs before the Recently Added check, and the service guard skips drafts. The keyset suite could not see an edit to the SQL: its evaluator matched on the imported constant, so any rewrite still hit that case and checked the model against itself. It now matches a literal copy. Flipping the scheduled sign in a copy of the module prints `post-sort pager cannot evaluate sort expression`. Also: the CSS-columns fixture now puts more cards in column one than there are columns, and a control asserts the previous top-based check accepts it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * test(posts): say what the keyset pager's literal SQL label does and does not check The label pins the drafts-queue SQL's spelling; nothing in the suite executes it. Say so where the next editor will read it, so a pasted label is not taken for a re-verified query. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
Justin Maier
|
9de45506cf |
refactor(shop): narrow shop item payloads to the fields the client renders (#4969)
Every surface that shows a shop item to a buyer publishes its meta through one display list, `shopItemDisplayMeta(meta, soldCount)`, with `purchases` taken from the purchase-row count. The cosmetic-store editor reads are moderator-only with the default token scope and keep the full record, which the moderator form writes back. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
Justin Maier
|
7e15bca183 |
chore(clickhouse): upgrade @clickhouse/client from 0.2.10 to 1.23.1 (#4972)
Upgrades @clickhouse/client from 0.2.10 to 1.23.1 in the root package.json and packages/civitai-clickhouse. apps/event-engine was already on 1.x, so the workspace now holds one version of the driver. A version upgrade, not a fix for the ClickHouse socket hang-ups. The pre-upgrade rate was recorded before this change so the post-deploy rate can be compared. - ResultSet.json<T>() returns T[] in 1.x: 13 call sites, 3 in src/ and 10 in apps/moderator, which the root typecheck does not cover. - host -> url; keep_alive.socket_ttl + retry_on_expired_socket -> idle_socket_ttl. - Three 1.x default changes held at their 0.2.x values: max_open_connections Infinity, request_timeout 300000, response compression on. - keep_alive.eagerly_destroy_stale_sockets: true is a deliberate non-default, more permissive than 0.2.x's retry, standing in for it. It confounds the before/after comparison. - 1.x adds ~1ms per request (await sleep(0)); not configurable. - apps/moderator ships on its own release. - New src/server/clickhouse/__tests__/client-config-pins.test.ts pins the values. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
Justin Maier
|
9282fd5deb |
fix(shop): read the sold count from the purchase rows, not the meta counter (#4942)
* fix(shop): read the sold count from the purchase rows, not the meta counter A listing had two live answers to "how many sold". `_count.purchases` counts real purchase rows and is what the sold-out gate, the quantity floor, the delete guard and the MostPopular sort use. `meta.purchases` is a denormalised JSONB counter, and it was what every displayed count read. They disagree on 47 of 1,902 prod listings. One of those renders "20 remaining" on a sold-out item behind a buy button that throws. Two selects gain `_count` — the shared `cosmeticShopItemSelect` and `getPackDetail`'s own, which does not use it. Four sanitizers emit the row count. Three further paths return `meta` to the client as-is and have no whitelist to change, so `withSoldCount` writes the row count onto the key they read; without it the same ShopItem component would show a correct number on a creator storefront and the drifting one on /shop. The counter is still written and nothing is backfilled. Fixing the writer is a separate PR. The index is required and goes in BEFORE the deploy — not because a reader would 500 without it, but so the first shop page after the deploy is not the one that discovers the seq scan. Applied by hand. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(shop): keep the sold-count change a read, and pin the selects that carry it Review round on #4942. Three real defects in the first pass. The change was not read-only. `getShopItemById` seeds the moderator item editor, that form posts the whole `meta` object back, and `purchases` is a declared key so zod keeps it — so every save wrote the derived row count into the stored counter, from a client cache that can be older than the value it replaced. The update now keeps what is stored; only a purchase moves it. The response still reports the rows, like every other read. Both `_count` select lines were pinned by nothing. Prisma mocks ignore `select` and every fixture hand-writes `_count`, so deleting either line left the whole suite green and threw on six read paths in production. Asserted against the query the code emits. The migration and schema comments measured a plan Prisma does not produce. A relation `_count` is a LEFT JOIN to one whole-table GROUP BY, not a correlated per-row subquery: 13.7 ms and 1,190 buffers, not 140 ms and 71,400, and the cost does not scale with page size. Rewritten with the real plans, where the index actually pays (single-item reads), and what it does not fix. Also: `getSectionById` and the upsert return were the two paths still serving the counter; both now go through `withSoldCount`, with controls. The `StickerShopPanel` comment justified its non-interleaved shelves on a sort-key mismatch this change removes. Every guard here was reverted and re-run; each fails naming the wrong value. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(shop): pin the selects and the response the last round left unguarded Second review round, run mutants rather than argued ones. Three things the fix round itself introduced. Two more selects were pinned by nothing, the same shape as the two the last round closed: deleting `meta: true` from the existing-item select left every write-back test green while every moderator save would write `purchases: 0` over the stored counter, and redefining `_count` on `creatorStorefrontItemSelect` killed the sold count on the creator storefront and the community hub with nothing red. Both now assert the query the code emitted. Tightening one test replaced an assertion instead of adding to it, and the behaviour it covered went in the same change — so the upsert response was unpinned in both directions. It is deliberately NOT mapped through `withSoldCount`: its only consumer invalidates and discards the payload, so mapping it fixed nothing and pinned a value nobody reads. That decision is now recorded by an assertion rather than left to the next reader. The migration header claimed the index would let multi-row paths read the index instead of the heap. It will not: every buffer is a `shared hit`, the table is fully cached, and `relallvisible` is 550 of 1,190 pages. Also names `getCommunityCosmetics` as the heaviest consumer — under MostPopular it carries two whole-table aggregates, confirmed by reading Prisma's emitted SQL. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(shop): correct the pin comments, cover the create path, drop a dead aggregate Round-3 review findings. Three comments claimed the test assertions were the only thing holding a `select` line. They are not: Prisma narrows the row to the select, so dropping a field is `TS2339` at the read. Reworded to what is true — a fast readable second signal, and the condition under which it would become the gate. The create branch's `purchases: 0` was covered by nothing: deleting it leaves 603 tests and typecheck green, because `meta` is Json. A new listing has sold nothing and `purchases` is client-supplied, so the zero is imposed, not trusted. Removing `withSoldCount` from the upsert response left the `_count` in that transaction's select dead — a whole-table aggregate on the primary, inside an open write transaction, that nothing reads. The write path now selects without it. The index migration is marked NOT APPROVED: the owner's answer was to replace the Prisma query with raw SQL first and re-measure, since the doubled aggregate is a Prisma artifact rather than a database necessity. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
Justin Maier
|
a658c3f6df |
perf(dev-server): let the test queue cap each run's vitest pool (#4965)
* perf(dev-server): let the test queue cap each run's vitest pool The queue serialises full-suite runs at concurrency 1, which makes a run wait behind every other agent's. Measured over 23.4h of the daemon's own history (50 runs, 12 worktrees): median run 549s, median wait 186s, mean wait 405s, worst 2247s. Raising concurrency is the only lever that helps a change whose closure reaches the hot services, but it cannot be raised alone: vitest sizes its pool at `cpus - 1`, so two uncapped runs ask for 62 workers on a 32-core box. VITEST_MAX_WORKERS cannot carry the cap here — the daemon spawns the child with the daemon's own environment, so the caller's copy never arrives and the daemon's is fixed at start. The CLI flag is the only channel that reaches a queued run, and it is forwarded through `pnpm run` into vitest. Verified by pool id rather than by argv alone: 8 files at --max-workers=2 ran on workers [1 2]; the same 8 uncapped ran on [1 2 3 4 5 6 7 8]. Adds a runtime setter beside it so the width can be tuned without a second daemon restart, and each key of `test config` is applied only when sent — a concurrency change must not silently drop the cap. Also replaces the "~75s" figure in the full-suite hook, which was off by 7x against the measured median and was what every agent budgeted against. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * perf(dev-server): queue full typechecks in their own lane A full `pnpm run typecheck` is one core and up to an 8 GB heap, and several agents starting one at once pegs the box the same way concurrent suites did. With CIVITAI_TEST_QUEUE set it now goes through the dev-server queue. The queue gains run kinds with separate limits rather than one pool, because the loads differ: a suite saturates every core, tsc is effectively single-threaded. One shared limit would either hold a typecheck behind every queued suite or let two suites run at once. Each lane takes only its own head of the queue, and a run's position is reported within its lane. The scalar concurrency every existing caller passes still sets the unit lane only; reading it as "every lane" would raise the typecheck limit on any machine that had only ever tuned the suite. A typecheck stays direct in CI, with any argument (the scripts gate's `-p tsconfig.scripts.json`), with the tsc test seam in use (otherwise the typecheck tests would queue behind real runs and assert on the daemon's REAL tsc), and with a heap override (a queued run gets the daemon's environment, so the override would be silently dropped). typecheck.mjs reuses test-unit-run.mjs's queue client rather than a copy. Also fixes the worker cap missing a caller's camelCase `--maxWorkers`, which vitest treats as the same flag — the queue would have appended a second, conflicting width after it. Nine revert controls, each red on its own named test, restores verified by hash — including the one nothing else catches: a typecheck posted without its kind is accepted as a unit run and spawns a full suite. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * feat(hooks): send full-program tsc runs to the queued typecheck script A direct `npx tsc --noEmit` skips the typecheck lane, so agents doing it at once are N single-core 8 GB heaps pegging the box. It is also the wrong check: tsc at node's default heap can abort part-way with zero diagnostics and a log that reads clean. scripts/typecheck.mjs raises the heap and names that crash. The hook now denies a full-program tsc (no -p, or -p at the root tsconfig) and points at `pnpm run typecheck`. Narrow runs pass untouched: a sub-project (`-p tsconfig.scripts.json`, which the scripts gate itself recommends), named files, --build, and informational flags. TYPECHECK_DIRECT=1 opts out for diagnosing tsc itself. Selftest: 68 rows green. Controls: disabling the guard fails all 10 block rows; matching `tsc\b` instead of `tsc(?=\s|$)` fails only "tsc-alias is not tsc". Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
briant
|
1299d1099f | 5.1.113 v5.1.113 | ||
|
briant
|
b9ce4a81b0 |
fix(generation): ungate YuE2 and lock the audio model picker
YuE2 was hidden behind the mod-only yue2Generator flag, so non-mods never
saw it in the audio picker, and the model page's Create button fell back to
ACE because a hidden ecosystem is dropped at the ecosystem field. Remove the
flag from both lanes.
The form-graph audio form also never passed modelLocked through to the
resource select, so locked audio models (ACE, MiniMax Music 3, YuE2) showed
a swap control. Pass allowSwap={!meta?.modelLocked} as the image and video
forms already do.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
||
|
Briant Diehl
|
12aa477882 |
Merge pull request #4964 from civitai/feat/huggingface-import-api
Feat/huggingface import api |
||
|
Zachary Lowden
|
f196799338 |
fix(app-blocks): a publisher ban revokes their live block instances (#4947)
* fix(app-blocks): a publisher ban now revokes their live block instances
Banning a publisher already unpublished their models, cancelled the
subscription, blocked their media and invalidated their sessions — but it
wrote none of the three markers the block-token runtime guards read, so
every block token their apps already held kept authenticating against the
REST wrapper and the tRPC bridge until its natural exp (900s default,
14400s for a dev token). That residual, and only that residual, is what
this closes.
Adds the third production writer of BlockRevocation.revokeInstance:
revokeBlockInstancesForPublisher (blocks/publisher-ban-revocation.service.ts),
called from toggleBan's ban fan-out alongside the model unpublish and the
media block. It marks every live instance of every block the banned user
OWNS (app.userId).
Deliberately narrow, both ways:
- Owner only, never a seated collaborator. Widening it would let a ban on
one account revoke the live tokens of an app owned by another account
that was not banned. Recorded in the app-ownership gate ledger.
- No enabled filter. A disabled install's earlier marker is TTL-bound and
may have lapsed; re-marking costs one Redis SET and only narrows exposure.
- The unban branch clears nothing. The markers expire with one token
lifetime and re-minting is the recovery path.
- isRevoked still fails OPEN on a Redis error — untouched, and now pinned
by a live assertion rather than left to a diff review. Inverting it would
refuse every block during a Redis incident; that is a separate decision.
The guard comment at block-scope.middleware.ts is cited by five other files
as the authority on this behaviour and said a ban writes none of the three,
so it moves with the code — along with block-revocation.service,
block-bridge-auth.service, apps.router, apps-shared.router,
scope-grant.service and the bridge-token guard test's header.
Closes clawgate #618.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* fix(app-blocks): reach all five instance-id namespaces, and make a ban marker unclearable
Review round 1 (civitai-review, five lanes) found the first cut of this writer was
wrong in the direction its own comments denied.
1. A blockInstanceId is not always a stored column. Four of the five namespaces are
SYNTHESISED: bus_pub_<busId> and bus_view_<busId> off a blanket subscription's
row id, pdb_<appBlockId> and page_<appBlockId> off the app block. The original
`where: { blockInstanceId: { not: null } }` reached ONE of five, and its comment
justified that with "a blanket subscription has no minted token keyed to a stored
one" — true, and irrelevant: the token is keyed to the synthesised id and
isRevoked compares claims.blockInstanceId verbatim. publisher_all_my_models
blanket is the publisher's own default install shape, so the most likely case was
the one it missed. The writer now enumerates subscriptions AND owned app blocks
and emits all five.
Pinned by a new seam guard, publisher-ban-revocation.namespaces.test.ts, against
deriveScopeFromInstanceId — the canonical parser. Fails on GROWTH (the parser
learns a prefix the writer does not emit) and on SHRINK/typo (the writer emits one
no token carries). Four mutations watched red, each on its own assertion.
2. A ban marker was clearable by a third party. clearInstance is called
unconditionally by toggleEnabled(true) and installOnModel, both driven by the
install's CONSUMER — the model owner, a different and un-banned account — and
blockInstanceId survives a disable. Toggling off and on undid the moderation
action. The marker now carries its cause as its value ('install' | 'ban'; the
legacy '1' reads as install) and clearInstance refuses a ban marker. It fails
CLOSED on a read error, the opposite of isRevoked and deliberately so: an
un-cleared marker expires within one token lifetime, a wrongly-cleared ban marker
needs a second moderator action.
3. Three mutations survived the first test suite, all found by running them:
- the Redis fake resolved in a microtask, so dropping the await on the fan-out and
detaching the leg from toggleBan's awaited Promise.all both passed. AC-1 is a
happens-before claim and the fake could not express it. Every fake now yields a
macrotask tick; both mutations are killed.
- "wrote a marker for each id AND FOR NO OTHER" filtered the key set to the
fixture's own ids, so over-revocation was invisible — the exact hazard the
ownership filter exists to prevent. It now asserts the whole key set plus the
call count; a mutant revoking a foreign id is killed.
- mint() hardcoded one appBlockId, so the fixture's "across two blocks" claim was
not exercised. It now carries each row's own.
Also: the chunk-and-await loop is replaced by limitConcurrency (the repo's helper) —
the old shape was a barrier, not the ceiling its own docblock described; the returned
count is now logged rather than discarded; and a call-site ledger pins the "exactly
three production call sites" sentence, which five files restate and which those same
comments record as having already been wrong twice.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* fix(app-blocks): split the ban keyspace, route ownership canonically, make revocation observable
Audit round 1 findings F1-F5, plus the operator-approved counter. One commit so round 2
audits a single delta.
🔴 F1 (deploy-blocker) — the cause-on-one-key design did not hold. `toggleEnabled(false)`
calls `revokeInstance` with no cause, and the write was unconditional, so an ordinary
un-banned model owner disabling a banned publisher's install DOWNGRADED the ban marker to
`install`; `toggleEnabled(true)` then cleared it and the pre-ban token was served again.
`clearInstance` had been hardened; the write in front of it had not.
Fixed STRUCTURALLY rather than with a value guard: ban markers get their own Redis
keyspace (`blocks:revoked-instance-ban`) written by a separate method,
`revokeInstanceForBan`. A read-then-set would have been a read-modify-write with a race
in it, and this wrapper exposes no value-guarded SET; separate keys make the downgrade
UNREPRESENTABLE — the install path cannot name the key. `isRevoked` checks both in ONE
round trip via `mGet`, so the per-request cost claim in block-bridge-auth stays true.
`clearInstance` addresses the install keyspace only and needs no branch at all.
And the guard that hid it: the consumer-re-enable test modelled `clearInstance` ALONE,
omitting the `revokeInstance` leg the real pair always runs first, so it was green in both
arms. It now drives the REAL `BlockRegistry.toggleEnabled(false)` then `(true)` pair, with
a positive control that an UNBANNED publisher's install still restores.
🟡 F2 — "re-minting is the recovery path" was false, including in this card's AC-3.
Instance ids are stable across a re-mint, so after ban→unban a freshly minted token was
still 403 for up to 14400s with no product-level remedy. The unban branch now clears the
ban keyspace for that publisher's instances, through the SAME enumeration the ban used
(a clear addressing fewer ids is no remedy). Install markers are untouched, so lifting a
ban cannot silently re-enable an install its own consumer switched off. The three comments
and `block-registry.service.ts`'s re-enable comment now agree.
🟡 F3 — `page_` is three mint shapes, not one. Added `page_pubreq_<publishRequestId>` via
`appBlockPublishRequest` (submittedByUserId + pending, mirroring the mint). `page_local_*`
is DOCUMENTED as uncoverable: it exists precisely because no server row ties the slug to a
user, so there is nothing to enumerate — closing it needs a different mechanism, not a
wider query.
🟡 F4 — the seam guard pinned `deriveScopeFromInstanceId`, which its own docblock calls the
client-side path, while the mint dispatches on `BlockRegistry.resolveBlockInstance`. Adding
a 6th prefix to the resolver alone left it green 18/18. It now pins the resolver, the
client parser, AND the two against each other; that same mutation is killed by two
assertions. The call-site ledger is split per writer, so rewriting a ban site to call the
install writer is a red test.
🟡 F5 — settled by enumeration, not assumed. `BlockTokenService.sign` has exactly two
non-test call sites and NO mint path reads `AppListing` or a listing `kind` at all, so an
offsite-owned block CAN mint: mintability and canonical ownership are decided by disjoint
column sets. Ownership now routes through `resolveCanonicalListingOwner` as a three-branch
predicate. Two paths produce the divergence, and only one is a mod action —
`acceptTransfer` moves `OauthClient.userId` only under `isOnsite`. The "offsite 5 rows, 0
with a block" production count is recorded as the stale empirical claim it is, not as a
justification. Pinned by an executable branch-for-branch equivalence test against the real
resolver; dropping branch 1 (listing-less blocks, most of the fleet) and reverting to
`app: { userId }` are both killed on the right rows.
OBSERVABILITY (operator-approved, pre-existing gap) — the revocation 403 returns before
`recordScopeInvocation` registers its `res.on('finish')`, so it could never write a
`block_scope_invocations` row and the mechanism's firing was unfalsifiable. Adds
`civitai_app_block_revocation_refusals_total{surface,namespace}`, emitted from both guards.
The namespace label is bounded and would have made F3's gap readable — `page_pubreq_` and
`page_local_` are bucketed before `page_`, which is the collapse that hid them.
Not in scope, deliberately: ban durability beyond one token lifetime is clawgate #620 and
is layered on these markers rather than replacing them (the status flip is replica-read and
lag-delayed; these kill a live session at Redis speed). The two eventloop-watchdog timing
failures are pre-existing on origin/main.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* fix(app-blocks): cover the ephemeral + mod-review page shapes, guard the ban TTL, correct three false claims
Audit round 2: four 🟡 and one 🟢. One commit so round 3 audits a single delta.
🟡1 — the `page_` enumeration was incomplete and the narrowing was false. It is FIVE mint
shapes, not three, and TWO more were uncovered, both `dev:true` (4h):
- `page_ephemeral-<blockId>` (block-tokens/index.ts) — an unsubmitted app over a live
dev tunnel, held by the AUTHOR: precisely the publisher this control exists to cut
off. Filed as uncoverable last round; that was wrong. An ephemeral app has no
AppBlock row, but a live tunnel IS server state. dev-tunnel.service.ts now maintains
a per-user SET of tunnelled blockIds (`listActiveDevTunnelBlockIds`) so a ban can
enumerate them — `userBlockKey` can only answer "is THIS pair live", and the
alternative was a cluster-wide SCAN inside a fan-out Bulk Ban multiplies. Index
writes are try/catch, not a trailing `.catch()`: a missing method throws
SYNCHRONOUSLY and would otherwise take a developer's tunnel start with it — which is
exactly how the dev-tunnel suite broke the moment those lines were added.
- `page_<pubreq_ULID>` (publish-request.service.ts) — the MOD review preview, SINGLE
`pubreq_`, so dev-token's double-prefixed spelling never matched it. Both spellings
are now emitted from the same pending rows.
`page_local_<slug>` remains genuinely uncoverable and is documented as a statement about
the mechanism: no server row of any kind ties that slug to a user. Both the writer's and
the middleware's shape claims are corrected; the middleware no longer restates a count.
And the seam guard's structural blindness is closed rather than just confessed. Every
prefix ledger is blind to a new mint surface that REUSES an existing prefix —
`deriveScopeFromInstanceId('page_ephemeral-foo')` returns `viewer_global` via the bare
`page_` branch — which is how `page_` came to be documented as one shape when it is five.
Added a MINT-SITE ledger over the four files that CONSTRUCT a blockInstanceId, with each
one's coverage. Verified: a sixth `page_` shape added to an unledgered file leaves all
prefix assertions green and fails the mint-site ledger. Its residual (an EXISTING file
growing a new shape internally) is stated in its docblock rather than left implied.
🟡2 — a surviving mutant on a safety-critical invariant. The TTL suite probed only
`revokeInstance`, so `revokeInstanceForBan`'s EX was guarded by nothing: mutating it to
900 stayed green across 5 files / 116 tests while a banned publisher's dev token lives to
14400s — silently un-revoked from T+900s. `markerTtlSeconds` is now parameterised over
both writers as a cross product with the token kinds, plus a ledger that reads the
service's own `static async revoke*` surface so a THIRD keyspace cannot ship unprobed the
way the second did.
🟡3 — "ONE ROUND TRIP, NOT TWO" was false and is deleted, not repaired. This repo's client
wraps `mGet` into `Promise.all(keys.map(get))` to avoid CROSSSLOT, so the array path never
reaches the native MGET: it is two GETs. Wall-clock is likely unchanged (same tick,
pipelined) but the COMMAND RATE against the cache cluster is doubled on every REST and
bridge request. Reworded here and in the four stale "ONE Redis GET" lines in
block-bridge-auth. The read logic itself is untouched — it was verified correct.
🟡4 — the new counter had zero test coverage while its own docblock called the branch
order load-bearing. Added a suite pinning each real minted shape to its label plus a
structural check that no prefix is tested after one it extends. Round 2 could not run this
mutation; it runs now and is killed by four assertions.
🟢5 — an inverted empirical claim: "most app blocks predate W13 and have no listing row"
is 23-of-24 the other way. The conclusion (keep branch 1) is unchanged — that single
listing-less block is the entire reason — but the magnitude is corrected and labelled.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* fix(app-blocks): subject-scope the ephemeral ban marker, make the index write atomic and owned
Audit round 3: four 🟡, two 🟢, plus one out-of-range stale sentence. One commit so round 4
audits the delta from
|
||
|
briant
|
d6d5f856a6 |
feat(models): a server-side Hugging Face transfer API that attaches on completion
Our own tooling can now queue a repo's files onto a model version in one call and poll, rather than driving the moderator page by hand. `WEBHOOK_TOKEN` guards it, so it is the same internal surface as the rest of `api/admin`. The transfer job attaches each file when its bytes land, which is what makes one call enough: `attachVersionId` and `attachType` are recorded at enqueue — their own columns, because `modelVersionId` means "attached to" and detach clears it. Detach clears the attach target too, or the sweep would re-attach a file a moderator just removed and mint a second `ModelFile` beside the one detach leaves alive. A file whose sha256 we already store is attached without transferring anything. Hugging Face publishes each LFS file's sha before any bytes move, so a text encoder shared by a dozen repos costs one lookup we already run. The match is on the sha and never the filename: `ae.safetensors` names different bytes in different repos, and the wrong weights on a version stay invisible until someone generates. The attach reads the primary. It runs microseconds after its own completion write, and a replica that had not caught up reported the row as still transferring — recorded as a permanent failure, which the sweep's `error: null` filter then excluded from recovery forever. The sweep takes the same claim the transfer does and runs inside the job's deadline. Unclaimed, two runs could both pass its read and create a file, with `linkImportToFile` picking a winner only after both existed. `createFileHandler`'s body is now `createModelFile`, taking `userId`, `isModerator` and `track` rather than a request context, because a cron tick has no session to borrow. The tRPC path passes its own session through. Both migrations are applied to production. The second is a partial index for the sweep, which orders by `completedAt` — a column no other index covers. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015cyrXpr87t9Tj3bnzRrUhp |
||
|
briant
|
17acf6b062 |
fix(files): give the two awaited external calls in the create path a deadline
`registerFileLocation` was the only function in `storage-resolver.ts` without an `AbortSignal` — the three deregister calls already carry one. The model-file scan submit had none either. Both inherit undici's 300s default, and both are awaited: by the upload response, and by the Hugging Face import job inside its own lock. One hung call there holds a lock far past the budget it was sized for, which is what lets a second run start on work the first still owns. Registration gets 10s rather than the 30s its neighbours use: it is one small write a caller waits on, where those are bulk post-commit cleanups nobody waits on. The scan submit gets 15s, matching the image-ingest submit above it — that number is sized against a measured ~4.7s P99, and nothing records one for this call. The submit passes no `wait`, so it is an enqueue and returns as soon as the orchestrator accepts the workflow. Neither timeout changes a failure path. A failed registration is already caught and logged by `safeRegisterFileLocation`; a failed scan submit leaves `scanRequestedAt` null, so `scanFilesFallbackJob` re-submits within five minutes. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015cyrXpr87t9Tj3bnzRrUhp |
||
|
Zachary Lowden
|
efa277ebb6 |
fix(stripe): charge the membership price in the customer pinned currency instead of 500ing (#4945)
* fix(stripe): charge the sibling membership price in the customer's pinned currency
Stripe pins a customer to ONE billing currency the first time they are invoiced
(`customer.currency`) and it is immutable. Every later subscription price for
that customer must be payable in it, or Stripe rejects the call:
The price specified only supports `usd`. This doesn't match the expected
currency: `aud`.
That is a raw throw out of `checkout.sessions.create` / `subscriptions.update`,
so it reached the client as a tRPC INTERNAL_SERVER_ERROR (HTTP 500).
The first version of this branch read the situation as "our membership Prices
are single-currency USD, so a pinned customer cannot subscribe at all" and
classified the error into a 4xx. That premise is wrong. Checked against the
Product/Price rows: every active Stripe membership tier carries seven active
monthly sibling Prices — aud, cad, eur, gbp, jpy, krw, usd — exactly one per
currency, and `getPlans` already ships the whole set to the pricing page. The
membership IS purchasable on a pinned account; nothing was resolving which
sibling to charge.
So the remedy is a substitution, not a classification.
Server-side, in `createSubscribeSession`, because that is the only place the
answer is knowable. `customer.currency` is a Stripe-side fact with no column in
our database and no endpoint that exposes it, so the price picker cannot choose
correctly however it is written. Resolving here also covers the plan-change
path and any caller that never goes through the pricing page. The resolution is
scoped to the same product, active, recurring, the same interval and the same
interval_count — each of those narrows a way the substitute could be the wrong
thing to charge, and the product scope comes off the Stripe Price object, which
keeps the lookup inside Stripe's catalog rather than reaching a row belonging to
the other payment provider.
A typed BAD_REQUEST survives only as the genuine fallback, in the two cases
where no single correct answer exists: the membership is not sold in that
currency at all, or more than one active price matches and charging either would
be charging an amount nobody chose. Neither message claims the membership cannot
be purchased — the old one did, and that sentence was false.
Client-side, the plan card now preselects the sibling in the currency of the
member's existing subscription. The server substitutes either way, so this is
not what makes the purchase work; it is so the figure on the card is the figure
that gets charged. That matters most on the plan-change path, which takes the
money immediately with no Stripe-hosted confirmation screen in between. The
existing subscription's price is the only pinned-currency evidence available to
the browser, and it is sound evidence: Stripe accepted that price, so by its own
rule its currency is the pinned one.
Currency case is normalised on every comparison. Not cosmetic here: the
Product/Price tables hold the same currencies in both spellings because the two
payment providers differ (Stripe lower-case, the other upper-case), the plan
card is provider-generic, and Stripe returns `customer.currency` lower-case. A
case-sensitive comparison matches nothing for one of the two catalogs. The
currency dropdown's labels are upper-cased in the data rather than only by the
`uppercase` CSS class, so the label does not depend on which catalog a product
came from.
Regression matrix — the server test was watched fail on pre-change code:
at origin/main (
|
||
|
Zachary Lowden
|
c442286109 |
fix(models): chunk the sale-badge lookup so a scrolled feed stops 400ing (#4948)
* fix(models): chunk the sale-badge lookup so a scrolled feed stops 400ing `model.getActiveSales` caps `ids` at 500. `useModelSaleBadges` fed it the whole accumulated list of an infinite feed, so from roughly the fifth page of cards onward EVERY call was rejected by input validation before the resolver ran. The sale badge then disappeared from the entire grid for anyone who scrolled — and because a rejected input is a 400, nothing watching server errors ever saw it. Chunked client-side rather than raising the cap. The cap is protecting real per-id work: `getActiveSalesForModels` resolves each id through the per-id cache, whose `packed.mGet` decomposes into one Redis GET per id on a cluster, and every id that misses lands in a raw `IN (…)` across a five-table join on the read replica. The procedure is public, so the length of that array is the only thing bounding the work — a bigger number would just move the wall. The cap and the chunk size are now one exported constant, so the client cannot drift past what the server accepts. Reuses the existing arrival-order chunker instead of writing a third copy of it. Moved it from `Sticker/sticker.util` to `shared/utils/chunk-ids` and renamed it `chunkIds`: two callers already had nothing to do with stickers, and importing it from there would have pulled the cosmetics/zustand graph into the model feed bundle. Arrival order is load-bearing, not incidental — sorting reshuffles every chunk boundary as a feed appends, changing every key and refetching the whole surface each page. Regression coverage drives the hook and validates each request it builds against the procedure's own schema, so a raised chunk size or a lowered cap both fail. Red at origin/main with `expected [ 1200 ] to deeply equal []` (one request of 1200 ids) and `expected 1 to be 3`; green at HEAD. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(models): keep badges on screen while a chunk loads, and size the chunk to the page Review round on the chunking fix. Four code findings, all verified against the source or the installed package before acting on them. 1. `placeholderData: (prev) => prev` is INERT under `trpc.useQueries`, so the first cut would have blanked every sale badge in the grid on every page of scroll — for feeds under the cap, i.e. exactly the population the original 400 never touched. Measured against the installed @tanstack/query-core, both arms in one run: `QueriesObserver` matches previous observers by `queryHash` alone, so a changed key builds a fresh `QueryObserver` whose `#lastQueryWithDefinedData` is empty and the placeholder resolves to `undefined`; `QueryObserver` (what `useQuery` uses, the positive control) keeps one observer for the component's life and does carry it across. The option is dropped and keep-previous is done in the hook. 2. Chunk size split from the cap and matched to the feed's page size. The trailing partial chunk re-keys on every page, so ids asked per distinct id is (chunk/page + 1)/2 — 3.0x at 500/100, 1.0x at 100/100, for the SAME number of requests per page. It also keeps a request inside both wire budgets in `~/utils/trpc`, so it stays a batchable GET instead of an unbatchable POST. The cap stays 500: it is protecting the per-id Redis fan-out, which the schema now says instead of blaming the SQL — measured on the replica, execution time is flat from 100 to 5000 ids and only planning grows. 3. `endsAt` rode the wire and nothing branched on it — the badge only formatted it. A full chunk's key is now stable, so a mounted feed could keep serving a sale that had ended, advertising a discount the model page and the charge path both refuse. Both hooks re-apply the end edge. 4. Reuse: `discountType` takes `SaleDiscountKind` from the package the server declares the output with rather than a local union restatement, and the fourth open-coded copy of the chunker folds into `chunkIds`. The schema module moves to `src/server/schema/`, which is where this repo's tRPC input contracts live and where the sibling cap constant this mirrors already sits; the app-graph guard passes with it imported client-side. Tests: the chunker's coverage moves with the chunker, and the hook's file now re-renders, so the merge memo key, the partial-load path and keep-previous are observable rather than asserted into a single synchronous render. Added a seam guard pinning the router to the shared schema — the drift that reproduces the outage was previously unguarded. Every guard was watched to fail: nine mutations, each killed by its own assertion with its own message, restored green after. The fixture is a literal above the CAP, not derived from the chunk. Sized off the chunk it sat at 220, under the cap, and the reverted hook was measurably GREEN on the headline assertion — a positive control on the fixture now keeps that honest. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(models): gate the sale badge on read, and make the cap guard behavioural Second review round. Two lanes independently found the same defect in the first round's own fix, and the test lane found that the fix shipped a red suite. The end-edge check was applied where the merged map was BUILT, which is a stamp, not a gate. The keep-previous map added in the same commit therefore escaped it and could re-serve a window that had closed since it was stored — measured at 100 of 100 expired entries returned. The gate moves onto the map being handed out, and its clock is re-read on every event that changes which map that is: a chunk arriving, and falling back or recovering. The fallback transition is the load-bearing one, because it hands out the same object and an identity-keyed memo would not re-check it — the first attempt at this fix was keyed that way and the new test caught it. Residual, stated rather than implied: a feed left mounted and idle re-reads nothing, so a window closing with no scroll and no refetch stays badged. That was equally true before this hook chunked — the map was never re-checked at all — and closing it needs the gate at the per-card read in `ModelCard`, whose only tests are browser-mode and cannot run in this environment. Registering the new guard: adding a `no-divergent-*` test without its three companion entries turns `no-lint-rules-script-drift` red (5 failed / 6 passed, verified). Wired into the `test:lint-rules` script and both guard inventories — without which it also never ran under that script at all. The guard itself was spelled rather than structural and was walkable: declaring a private `const getActiveSalesSchema` with a lower cap in the router left every string check green while every request 400d. It now parses real arrays through the procedure's real parser, so a cap wrong in either direction fails wherever it was spelled. Four more mutants that survived the previous round now die: `endsAt` read as a Date only (the string payload the comment describes would throw inside a render), the single-card hook's end-edge check, the chunk size returning to the cap, and the merged map losing referential stability. The fake was also lying in a way that hid two of those: it stamped `dataUpdatedAt` on every call rather than per resolved id-set, so the merge memo never memoized under test. It now stamps per id-set and carries a string `endsAt` arm. Every guard added here was watched to fail: six mutations, each killed by its own assertion, restored green after. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * docs(models): say what the id cap actually bounds The schema comment claimed the cap was "the only thing standing between an anonymous caller and that fan-out". That is true of ONE request and wrong about a caller: the cap bounds how WIDE a single call may be, and says nothing about how many calls arrive. Reading it as an abuse control overstates it. Both copies of the claim are reworded to state the property the constant really has — one request's fan-out stays proportional to a page of cards rather than to an accumulated feed — and the ModelCardContext copy drops the same aside. * docs(models): narrow the cap guard to what it checks, and decouple a control Round-2 review found three places where a claim was wider than what backs it. The cap guard's docstring said the router cap "and the size its card surfaces chunk to are one contract across two files", but the body never reads the card surface. Measured: swapping chunkIds(modelIds, MODEL_SALE_IDS_PER_REQUEST) for a literal 400 leaves no-divergent-active-sales-cap at 4 passed (4), while useModelSaleBadges goes 3 failed | 15 passed (18). Narrowed the sentence rather than widening the body -- the only way to read the call site from a node-project guard is a source-text match, and this file already records that a spelled version of this guard was written once and shown to be walkable. It now states its scope first, names the blind spot with both measured arms, and points at the file that does pin the card-surface half, including that that file runs in the full unit suite and not in test:lint-rules. The residual comment in ModelCardContext said "left mounted and IDLE", which reads as an edge case. refetchOnWindowFocus is false app-wide and the only per-query override is staleTime, so the end-edge memo re-reads the clock only on a chunk resolving, a reconnect or a remount -- which means on any surface that has stopped growing (a profile's OnSaleSection, a search page nobody is paging, a feed scrolled to the end) the gate freezes for the life of the mount. Stated as the steady state it is, with the reason a refetchInterval is the wrong lever: it would re-issue the per-id Redis fan-out the cap exists to bound, per tick per mounted feed per user, to fix a display staleness. The GET-budget positive control built its over-budget fixture from MODEL_SALE_IDS_PER_QUERY. At 7-digit ids the serialized input is 8N + 9 chars against MAX_GET_INPUT_LENGTH (2500), so break-even is N = 312 -- lowering the cap below that turns the control's true into a false and reds the test for a reason unrelated to the property under test. Now a fixed literal at 400 (3209 chars). Comments and a test fixture only; no behaviour change. Test Files 4 passed (4) / Tests 40 passed (40). Red arm re-derived against the shipped tree (merge-base ModelCardContext + HEAD's tests): 12 failed | 10 passed (22), headline "expected 1 to be 12". typecheck 0 errors; eslint --no-cache 0 problems on all three files; prettier clean against a negative control. * docs(models): retract the cap-guard coverage claim in the two agent-facing docs Round 2 found the retracted claim surviving verbatim in the two docs this PR adds it to, while the guard's own docstring had already been corrected. Measured: the string is absent at origin/main and present twice at the PR head, so this PR introduced both copies. Both now state the guard's real scope (server side only; it cannot see the call site) and name the file that pins the other half, plus the fact that file is not in test:lint-rules -- so a test:lint-rules run alone does not cover the seam. no-lint-rules-script-drift: 11 passed (it pins names and counts, not these parentheticals, so it was always green either way). --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
briant
|
997eaaa15e | 5.1.112 v5.1.112 | ||
|
briant
|
de8cf569b6 | Merge branch 'main' of https://github.com/civitai/civitai | ||
|
Zachary Lowden
|
e16409074c |
feat(app-blocks): measure and answer every postMessage bridge drop (#4946)
* feat(app-blocks): measure and answer every postMessage bridge drop
The host<->block bridge could discard a message five distinct ways, all
producing the same observable -- nothing -- and none of them measurable.
civitai_app_block_renders_total cannot see any of it: it fires once per host
mount, so every failure after BLOCK_READY is structurally invisible to it. A
custom-generators gallery read was dead for a full 15-day retention window
while 100% of render series across all 11 rendering apps read result=ok.
Both halves land in the shared dispatcher, so one change covers all 46 message
types rather than 46 per-handler edits:
- civitai_app_block_bridge_messages_total{app_block_id,type,host,outcome},
outcome in {handled,no_handler,rate_limited,deduped,no_token}, emitted via a
coalescing browser beacon to a new /api/track/block-message. Coalescing, not
one POST per message: the bridge's own inbound limit is 30 msg/sec/host and a
polling generator app is the common case; counts aggregate losslessly, so the
series is identical to a per-message beacon's. Both client-supplied labels are
clamped to code-owned sets on BOTH sides -- server-side it is the prom
cardinality bound, client-side it bounds the buffer and the POST body.
- An error reply instead of a silent return. No handler for a REQUEST-style type
now gets the protocol's own error variant for that type, so a block fails in
milliseconds rather than hanging to its SDK timeout class (30s / 120s / 600s).
The reply shape is per-family and cannot be uniform -- the workflow family
needs a failureSnapshot or the SDK validator drops it -- and that resolution
lives in one place. GET_WILDCARD_PACK and REQUEST_TOKEN are exempt with the
reason recorded: their failure replies would be dropped by the SDK validator,
so they are counted but not answered.
The card named 6 token-falsy sites; enumerating the class found 31. Nineteen now
route through the shared nack, eleven that already replied in a bespoke shape
gained reportNoToken (otherwise the no_token series would cover 18 of 30 while
its help text claimed all of them, missing exactly the money-adjacent paths),
and one counts only. noSilentTokenDrop.test.ts is the ratchet: every !token
branch in either host must answer, or be counted under a type the exemption
ledger names.
Red before, green after: the two host suites report 17 failed / 3 passed against
a clean origin/main worktree carrying only the new leaf modules, 20 passed at
HEAD. Three mutations named in review as surviving -- default sink to no-op,
nack dropping its report, host/app label swap -- are each confirmed caught.
* fix(app-blocks): close the delta-audit findings on the bridge telemetry
A round-2 audit over the round-1 review fixes found nine items. Every one that
was a real defect is closed here; the two that were only claims are corrected
rather than quietly dropped.
- A SPARSE-ARRAY HOLE landed in PageBlockHost's CREATE_POST_FROM_APP dependency
list. Harmless at runtime (React compares by index and undefined === undefined)
but a corrupted list in a 31-effect file, and structurally invisible to every
green signal we had: no-sparse-arrays is not in this repo's eslint config, TS
allows holes, and prettier preserves them. Found by running the rule explicitly.
- noSilentTokenDrop's "wider net" check could NOT see the regression the file
exists for. It tested for a response anywhere in the enclosing onMessage chunk,
and every handler's SUCCESS path contains a send(), so a planted 32nd handler
with a bare `if (!token) return;` scored clean and moved no ledger number. The
walk is now a real paren/brace match over each `!token` guard's OWN consequent,
handler ranges are bounded by the call's parens instead of bleeding to EOF, and
the unbraced one-liner is forbidden outright. That mutant now fails three
assertions, including the one whose title names it.
- BRIDGE_MESSAGE_COUNT_MAX's derivation was wrong for three of the five outcomes.
no_handler and deduped are reported ABOVE the inbound rate limiter, deliberately,
and rate_limited only exists past it -- so "30/sec x 10s = 300, nothing real can
reach it" was true only of handled and no_token, and a backgrounded tab's flush
timer is throttled besides. Raised 2,000 -> 100,000 with an honest derivation:
it is a sanity ceiling that keeps a flood VISIBLE rather than exact, not a rate
control.
- The route comment claimed COUNT_MAX "caps what one request can add to a single
series". It does not -- nothing enforces row uniqueness. Corrected to say what
the route does enforce (cardinality, the prom-heap axis) and what it cannot.
- IframeHost counted a credential-less REQUEST_TOKEN only when a requestId was
present, while PageBlockHost counted unconditionally -- and three comments plus
a test title asserted they were in step. A requestId-less REQUEST_TOKEN is an
explicitly documented protocol shape, and on it the count is the only observable,
so IframeHost reported nothing. Both hosts now use reportNoToken unconditionally
and the parity is asserted for BOTH hosts rather than described.
- The visibilitychange flush was untested: deleting the listener left the beacon
suite fully green while the module's "no loss on navigation" claim and the whole
flush-window argument rest on it, and a mobile tab switch fires only that event.
Added, with the visible-state negative arm.
- The route test's count boundary was the literal 10_001, stale against the new
cap and pinned to nothing; it now reads the constant.
- appBlockId was the last field reaching the buffer unclamped and is the only one
that could still fail the schema on LENGTH and reject a whole batch.
Verified: typecheck 0 errors; eslint 0 errors; prettier clean; no sparse arrays in
any changed file; full unit project 1879 files / 42418 tests pass (the one failing
file, eventloop-watchdog.capture, fails at BASE too); AppBlocks component project
48 files / 595 tests pass; the two host suites still report 17 failed / 3 passed
against origin/main carrying only the new leaf modules.
* fix(app-blocks): the structural guard was reading a corrupted copy of the host
Round 3 of the audit ladder. Every finding was in the round-2 FIXES, which is
the point of re-auditing the delta each round.
THE BIG ONE. `noSilentTokenDrop`'s comment stripper was a regex,
`/(^|[^:])\/\/.*$/gm`. PageBlockHost contains `cleaned.includes('//')` — a `//`
inside a STRING — so the regex ate the rest of that line, deleting two `)` and a
`{` and leaving one `onMessage(` unmatched. The paren-bounding round 2 added to
stop handler ranges bleeding to EOF was therefore INOPERATIVE over 76% of the
file: one range ran 71,358 chars to EOF. Two consequences, both measured by the
auditor: a legitimate lifecycle `!token` guard added below that point failed with
a message about dropped requests (a booby trap for the next person), and three
real silent-drop shapes were caught only BY the corruption — repair the string and
they went green.
The stripper is now a state machine that blanks the CONTENTS of comments, strings
and template literals while preserving every byte offset, so no bracket inside a
string or comment can move a walk. A self-check asserts what the regex version
would have failed: brackets balance, the largest handler range is bounded, and no
range ends at EOF.
With the walk actually operating, four more holes were closable and are closed —
each confirmed by planting the mutant and watching it go red:
- a handler registered BY REFERENCE (hoisted into a useCallback) is invisible to
any walk bounded by the registration's parens. The shape is now refused outright.
- a generic containing `=>` drove the angle counter negative, so the call's `(`
was never found and the whole handler silently left the population.
- the exemption escape hatch matched an exempt type STRING in ANY handler, so a
new handler could buy silence by copying the REQUEST_TOKEN guard and forgetting
to change the argument — which also mislabels the telemetry. The exempt type must
now BE the handler's own registered type.
- the REQUEST_TOKEN parity assertion read only INSIDE the consequent, so hoisting
`if (requestId !== undefined)` one level out restored the round-2 defect with the
test still green. It now also requires the `!token` guard to be the handler's
first `if`, and selects the guard by the handler's REGISTERED TYPE rather than by
the string appearing somewhere in a consequent.
Also:
- the retracted "6.6x, unreachable by a real client" derivation survived in a THIRD
file, 25 lines above an edit the same commit made. Corrected.
- `appBlockId`'s clamp claimed "only the length can fail the schema". zod runs
`.trim()` before `.min(1)`, so a whitespace-only id is truthy here, trims to ''
server-side, and 400s the whole batch — the exact loss the clamp exists to
prevent, from the one direction a length check cannot see. `.trim()` first, with
a test.
- two wording nits in the count-cap derivation that its own next paragraph
contradicted.
The file's "what it does not claim" section now also names the silent-drop
spellings the `!token` population does NOT cover (`if (token) {…}` with no else,
an aliased token, `== null`, `!props.token`) rather than leaving a reader to
over-read the guard.
Verified: typecheck 0 errors; eslint 0 errors over all 17 changed files; prettier
clean; AppBlocks component project 48 files / 596 tests pass; the node-side
AppBlocks + track tests 45 files / 644 tests pass. Five mutants planted and all
five red: unbalanced paren in a string, handler by reference, generic with `=>`,
borrowed exempt type, hoisted requestId gate.
* fix(app-blocks): parse the hosts with the TypeScript AST, not by hand
Round 5 of the audit ladder, and the root-cause fix rather than a fifth patch.
Three revisions of `noSilentTokenDrop.test.ts` hand-rolled a parse of the two
host files, and an adversarial audit found a NEW defect in every one. Every
finding was a PARSING bug; not one was a logic bug:
r1 it looked for a response anywhere in the enclosing handler — which every
handler's success path supplies with its own send('<X>_RESULT', …);
r2 it stripped comments with a regex, which ate a `//` inside a STRING
literal, left an `onMessage(` unmatched, and produced a 71,358-char
"handler range" running to EOF;
r3 it scanned characters correctly but sliced the CONTENT out of the RAW
file, so a comment inside a branch could satisfy the response check and a
real silent drop passed — a capability the r2 version had and r3 lost.
The same revision also had five ways to fail CORRECT code: a comment between the
type argument and the handler read as "registered by reference"; the payload-shape
guard every sibling handler opens with tripped the REQUEST_TOKEN parity check; a
handler 21 comment-lines longer than today's largest tripped a size bound whose
failure message blamed the scanner; and two ordinary regex literals — one of which
sits two lines from this file's own subject matter — either unbalanced the
brackets or silently blanked a line of real code.
All of that is a property of hand-parsing a language. `typescript` is already a
dependency, so the walk now uses `ts.createSourceFile` and asks about AST shape:
comments, strings, template literals and regex literals are not expressions, so
none of them can supply a call, a type name, or a bracket. The size bound and the
bracket-balance self-check are deleted outright — they existed only to detect the
hand-scanner corrupting its input.
With a real parser, four checks became expressible that were not before, and each
is the property that was actually meant rather than a proxy for it:
- the registered type is read from the string-literal ARGUMENT NODE, so a
comment can no longer hand a handler another type's NACK exemption;
- "unconditional" is an ancestor walk, so `&&`, a ternary and a nested `if` are
all caught — where counting preceding `if (`s saw none of them, and fired on
correct code;
- a call inside a NESTED FUNCTION does not count as executed by the branch,
which closes the alias shape (`const count = () => reportNoToken(…)` then
`if (…) count()`) that survived every other spelling;
- the same bar now applies to the response check, whose name already claimed it.
Battery: 13 mutations, ALL 13 red, including every shape the audit named as
surviving. Four correct-code controls, all green; the fifth (a genuinely new
32nd handler) fails only the ledger count, which is that assertion working.
typecheck 0 errors, eslint clean, prettier clean, node-side AppBlocks + track
tests 45 files / 644 tests pass.
* revert(app-blocks): drop the structural no-silent-drop guard
It failed adversarial review five rounds running, in BOTH directions, and the
last round found more false positives than true ones. Shipping it would hand the
next contributor a trap, so it goes.
WHAT IT WAS. `noSilentTokenDrop.test.ts` tried to assert a CLASS guarantee the
behavioural suites cannot: that the thirty-second handler, written by copying the
thirty-first, cannot silently drop a credential-less request. Worth wanting — that
defect is invisible by construction, and it is the whole subject of this PR.
WHY IT GOES ANYWAY. Four revisions, four audits, a new defect every time, and
never a logic bug — every one was a defect in reading the source:
r2 it looked for a response anywhere in the enclosing handler, which every
handler's success path supplies with its own send('<X>_RESULT', …);
r3 a comment-stripping regex ate a `//` inside a STRING literal, leaving an
`onMessage(` unmatched and a 71,358-char "handler range" running to EOF;
r5 the character scanner matched brackets on the blanked copy but read CONTENT
from the raw file, so a comment in a branch satisfied the response check —
a capability the previous revision had and this one lost;
r7 rewritten on the TypeScript AST, it still missed four shapes, the sharpest
being an early `return` placed BEFORE the responder rather than around it.
`isUnconditionalWithin` walks ancestors, so it sees syntactic dominance and
not reachability. That is the parity test's own stated defect re-spelled,
and the idiom it needs already sits two lines above the guard in production.
The direction that decides it is the other one. Three ORDINARY, CORRECT
refactorings turned it red, each with a message naming the wrong cause:
- `else if (!token)` instead of two sequential `if`s — semantically identical —
reported as "nested inside another conditional";
- a local helper arrow inside a handler reported the same way, and moved two
ledger numbers as well;
- a `useCallback`-wrapped inline handler — the idiomatic shape for a handler
registered in a `useEffect` with a dep array, which is how all 44 of these are
registered — reported as "registered by reference".
A guard that reads as coverage while providing none is worse than none, because
it stops anyone looking. A guard that fails correct code with a misleading message
is worse still: the next person's options are to contort the code or to delete the
test, and whichever they pick, this file taught them the wrong thing.
WHAT IS STILL COVERED, AND WHAT IS NOT.
Covered: `PageBlockHostNoTokenNack.browser.test.tsx` drives 12 message types on
the real host with a null token and asserts the per-family reply shape; both hosts'
REQUEST_TOKEN branch is exercised; `IframeHostUnhandledNack.browser.test.tsx`
covers the unhandled-type NACK; the seam tests pin the rows reaching the real
beacon buffer. Those were red at base — 17 failed / 3 passed — and are the tests
the task asked for.
NOT covered: the class. A thirty-second handler that drops a credential-less
request silently will not be caught by anything in this repo. That is a real,
named gap, and it is a smaller cost than a guard nobody can trust in either
direction. The comment in `IframeHost` that pointed at this file now says the two
hosts stay in step by hand.
* docs(app-blocks): stop pointing at the guard that was just removed
The REQUEST_TOKEN branch's comment cited `noSilentTokenDrop.test.ts` as the thing
asserting both hosts count unconditionally. That file is gone, so the citation was
a dead pointer to a guarantee nothing provides any more — the exact shape of rot
this PR's own doc-hygiene argument is about.
It now says what is true: the two hosts stay in step BY HAND, and the reason the
structural guard is not there is in the PR description.
|
||
|
briant
|
f13756080f | 5.1.111 v5.1.111 | ||
|
briant
|
d273a67a09 | fix meta description on user profile page | ||
|
Zachary Lowden
|
720ed3e087 |
fix(app-blocks): make /api/v1/blocks/me and blocks.getMyViewer agree on authorization (#4950)
* fix(app-blocks): make /api/v1/blocks/me and blocks.getMyViewer agree on authorization
Two front doors to one capability disagreed about who may read viewer identity,
and the disagreement was masked by the Flipt audience rather than absent.
GET /api/v1/blocks/me carried a hardcoded isModerator -> 403 ("Phase 2: App
Blocks is moderator-only until GA"), no App-Blocks flag gate and no rate
limiter. Its tRPC twin blocks.getMyViewer had the flag gate and the rate
limiter, no moderator literal, and a docblock claiming it mirrored me.ts
EXACTLY. The live app-blocks-enabled audience is mostly moderators, for whom the
literal refused nobody the flag would have admitted -- but it also holds
hand-allowlisted non-moderators, and for every one of them the REST door 403'd
while the bridge returned 200. Widening the audience makes that the general
case.
The decision (operator, 2026-09-18) is that Flipt is the gate; a code-level
availability:['mod'] is documented as a Flipt-DOWN fallback only. So:
- Drop the moderator literal from me.ts, and the isModerator column from its
select.
- Move assertAppBlocksEnabledForTokenUser out of blocks.router.ts into
src/server/services/blocks/block-token-access.service.ts so both doors run ONE
implementation. A Next API route cannot import the tRPC router, and a second
copy of the predicate is how the two came to disagree.
- Give me.ts that gate plus checkBlockCatalogRateLimit, same bucket, same
position (before the primary read) as getMyViewer.
- Replace the false "mirrors EXACTLY" docblock with explicit SHARED and
NOT-SHARED lists, where SHARED is exactly what the parity test exercises and
NOT-SHARED names the three places the doors genuinely differ -- including two
pre-existing bridge-side divergences this change records rather than fixes.
- Repoint scripts/compiled-branch-watchlist.mjs's module: the watchlisted
fail-closed branch block-token-subject-refusal moved with the function.
me.ts renders both kill-switch refusals with its own literal and does NOT echo
the gate's message: rest-error-envelope-ledger.test.ts blocks a REST route from
serialising a caught error's .message, and the unhydratable-subject message is a
compiled-branch anchor that must stay unique app-wide. Detection is duck-typed
on .code rather than instanceof, matching the sibling routes, because
instanceof fails across a duplicated @trpc/server instance in an API bundle.
Tests: blocks.router.me-parity.test.ts drives BOTH doors with one subject and
compares a normalised verdict. Measured in a CLEAN checkout of the base commit
with the HEAD test files dropped in -- at
|
||
|
Zachary Lowden
|
ad3134ccc6 |
fix(bot-detection): stop asset-staging's top rung outscoring its strongest rung (#4954)
* fix(bot-detection): stop asset-staging's top rung outscoring its strongest rung
The `asset-staging` volume ramp ran (zeroAt 1, oneAt 3), scoring a staged
count of 1 at 0, 2 at 0.5 and 3-or-more at 1.0. That ordering is backwards
against what the heuristic is for.
A rising ramp puts its top rung on the largest counts, and on this predicate
the largest counts are the wrong population: a coordinated upload clusters at
exactly TWO assets, an avatar and a header, which is what a profile needs and
no more, while a legitimate profile setup -- a business, or a creator bringing
in a kit -- runs to THREE OR MORE. So the rung the heuristic weighted highest
was the rung carrying disproportionately many legitimate accounts, while the
shape it exists to find sat at half of what it selected.
Move STAGED_ONE_AT from 3 to 2 so the ramp saturates at the firing point:
count 1 scores 0, count 2 and above score 1. A plateau, not a suppression --
the 3+ arm still scores its maximum and still reaches a moderator on its own.
It is the worse of the two arms, not an empty one, so the requirement is that
it stop OUTSCORING a pair, never that it stop scoring. rampScore is monotone
non-decreasing by construction and throws on oneAt <= zeroAt, so a declining
shape is not expressible in the shared helper without a second ramp term; of
the shapes that are expressible, the plateau is one constant.
Costs, recorded in the code rather than left to be discovered:
- The volume half now has no gradient -- it is a step -- so the sub-score
cannot express "more staged than that". The count itself is still disclosed
verbatim to a moderator by explain().
- The two rungs are no longer distinguishable in any counter a run emits, so
a future re-shape has to be graded against moderation outcomes rather than
read off the shadow-phase counters.
- Two series move on deploy with no account behaving differently: a lone
asset-staging blend goes 0.125 -> 0.25 (one confidence bucket for a
lone-signal account), and asset-staging's sole_signal inflates because the
dominance test's runner-up tolerance scales with the leader's score.
The reported population is unchanged at the shipped cut.
Tests: adds "THE ORDERING", which pins score(2) >= score(3) as a comparison
rather than as two literals, and also pins that the 3+ arm keeps scoring and
stays independently reportable -- so the ordering cannot be satisfied by
suppressing it. Watched red against the unmodified ramp
("expected 0.5 to be greater than or equal to 1") and green after.
Existing expectations that moved are the arithmetic consequence of the new
boundary. One case, "scores 0 for ONE staged upload and fires from two", is
deleted: its assertions had become a subset of the saturation case's, so no
mutant could separate them.
Comment changes in ramp.ts, run.ts, scoring.ts and the test files correct
statements this change falsified -- chiefly two that said the volume and burst
halves share boundaries, and one giving a degenerate step as the reason for
rampScore's throw, which the new adjacent-integer pair makes false.
MIN_REPORTED_CONFIDENCE, LONE_SIGNAL_CUT, the heuristic registry and the burst
boundaries are untouched.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* docs(tests): qualify two false absolutes about which boundary pairs rampScore clamps
Review round 8 found one finding, in prose only; no assertion, fixture or
expected value changes, and the file's code is byte-identical (verified by
stripping comments with the TypeScript parser).
The asset-staging loop-vacuity note said `burst <= 1 === volume` holds for
"every boundary mutation, since rampScore clamps". It does not. rampScore
guards `!(oneAt > zeroAt)` and THROWS on a degenerate pair, so such a pair
produces no output in [0, 1] at all and the loop goes red rather than holding
-- measured, `BURST_ONE_AT -> 1` with its pin updated turns 83 of 398 cases
red on that error. The hypothesis the sentence rests on ("output stays in
[0, 1]") and both of its named counter-examples were already correct; only
the appositive was too wide.
That matters more than a nit because the same file retracts this exact class
by name two hundred lines earlier, where the same edit is described as
throwing and taking ~80 cases with it. The two paragraphs disagreed, and the
newer one was the wider.
Second instance of the same shape, same file: the ORDERING case said its
comparisons hold for "EVERY boundary pair with oneAt <= 2" -- a pair with
zeroAt >= oneAt satisfies that quantifier and throws.
Both errors ran in the safe direction: they overstated how vacuous the
assertions are, i.e. understated coverage, so neither gave false confidence
in a guard.
Also corrects a positional reference that the round-7 reorder invalidated --
`spread.burst` is no longer "at the foot" of its case, it is the penultimate
block, since the count-1 control was deliberately moved below it.
Suite 398/398, ESLint and Prettier clean.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
---------
Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
||
|
Koen
|
0829f6ab0f |
fix(feed): count deep-offset requests as rejected, not unmapped (#4960)
Requests past the feed's offset limit have returned an error since #4898, but the primary counter still filed them under `unmapped`, next to the requests that fall back to Meilisearch. They now carry outcome `rejected`; the reason label is unchanged. Co-authored-by: Claude Fable 5.1 <noreply@anthropic.com> |
||
|
Briant Diehl
|
0af959da62 |
Merge pull request #4956 from civitai/yue2-model-card
Enable YuE2 model-card generation |
||
|
briant
|
8afaa0cfef |
fix(ingestion): log the real error and elapsed time when a scan submit gets no response
A submit that never gets a response is the one case where the error's identity is
the whole diagnosis, and it was the one thing the log did not carry:
JSON.stringify(new Error()) is `{}` because Error has no enumerable own properties,
so every no-response failure recorded `error: {}`. Pass it through safeError, which
the repo already uses for exactly this, and add the wall time across all attempts —
the attempt count alone cannot tell three 15s aborts from an instant rejection.
The test pins the serialization rather than the call: reverting to the raw error
fails with `expected '{}' not to be '{}'`.
The logging mock in the covering suite was hand-listed and silently dropped
safeError; spread the original instead, since that module is pure apart from
logToAxiom.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
||
|
rmatif
|
403bfaed8d | Enable YuE2 generation from its model card | ||
|
Zachary Lowden
|
dce428a492 |
feat(app-blocks): author-fee accrual ledger (slice 2a, dark) (#4944)
* feat(app-blocks): author-fee accrual ledger + daily settlement rail (slice 2a, dark) The rail that pays an app author the per-generation fee slice 1 (#4922) learned to compute. Adds the ledger, the daily settlement job and the clawback path. NOTHING WRITES TO THE LEDGER YET. The viewer-charge path is deliberately not in this PR — see "What is NOT here" below. So this moves no money: the table stays empty, the settlement job no-ops on an empty scan, and the whole path is behind app-blocks-author-fee-enabled, which is false. TWO HOPS, MIRRORING THE MODEL LICENSING FEE. The licensing rail charges the viewer at generation time and writes a per-resource fee row, then deliver-creator-compensation mints to the creator daily. Same two hops and the same externalTransactionId dedup discipline here, in a civitai-owned table — because orchestration.resourceCompensations is keyed on a modelVersionId and written by the orchestrator, and an app fee has no model version. That fork is recorded rather than left for a reader to rediscover. NO FRACTIONAL ACCRUAL, AND THAT REVERSES THE DESIGN IT INHERITED. The licensing fee is fractional because it is priced per-image at 0.01 buzz and the viewer pays the ceiling of the sum, so the creator's share genuinely has sub-buzz resolution. This fee does not: max(flat, pct x base) is floored to whole Buzz before the viewer is shown or charged it (D7 requires the viewer see the exact number before the run, and Buzz cannot express a fraction). The author is credited exactly what the viewer was debited — the platform is a conduit and takes no cut. The daily batch therefore exists for ledger volume, not rounding. Consequence, stated in three places because slice 3 must surface it: an author who sets a 0 flat leg and a low percentage earns nothing on cheap generations, forever. Same shape as the $0.00 spend bounty this arc replaced; the difference is it is now the author's explicit choice and the platform default avoids it. DECISIONS IMPLEMENTED D6 blue Buzz in, blue Buzz out — settlement groups by buzz type and never coerces. A collapsed bucket would convert non-withdrawable Buzz into withdrawable earnings and no total would change. D10 the percent leg prices off base only (no code change; recorded). Self-dealing is excluded at accrual, and counted rather than dropped. The app owner is snapshotted at WRITE time so an ownership transfer cannot retroactively move earnings already accrued. CLAWBACK. The orchestrator refunds undelivered work after submit, so the fee has to follow or an author earns on a generation the viewer got refunded. Not a new policy — CalculateLicenseFees already weights every fee by delivered fraction. Before settlement the accrual is voided in place; after it, a negative carry-forward row nets against the next run. A bucket whose net goes non-positive is HELD, not forgiven at zero, so the debt stays visible. Mint happens BEFORE the status flip, deliberately: a crash between them settles late rather than paying twice, and the deterministic dedup key is what makes the retry safe. MIGRATION IS MANUAL-APPLY, AND SHIPS BEFORE THE CODE (rule 8, the #4903 precedent). Committed for history only. VERIFICATION pnpm typecheck OK - 0 type errors in 165s settlement suite 17 passed mutation sweep 6/6 killed, EACH by its own named test: self-dealing guard, D6 bucket key, dedup-key composition, non-positive hold, clawback sign, zero-fee guard positive control applied +1000 to the minted amount -> 2 tests red, proving the harness executes the code under test What is NOT here, and why: the viewer-charge path. The fee must be priced from the whatIf base and added to BOTH reservations before submit — otherwise it escapes the viewer's per-app consent budget entirely, which is the one real safety hole this design found. That touches four submit paths on the hot billing path and deserves its own focused audit rather than being folded in behind a new table. It is the next PR. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(app-blocks): FK targets app_blocks, not AppBlock — and the entry_type CHECK is unreachable Two corrections the dev-first apply caught, both in the migration only. 1. The app_block_id foreign key referenced "AppBlock", which does not exist. The Prisma model is AppBlock but it carries @@map("app_blocks"), so the physical table is app_blocks — confirmed against the existing block_spend_attribution_app_block_id_fkey, which points at app_blocks. Applied as written this would have failed outright on every database. Hand-writing the migration rather than generating it is what lost the mapped name. 2. The entry_type CHECK is SUBSUMED by the amount-sign CHECK and cannot fire. The sign check reads (entry_type='accrual' AND fee>=0) OR (entry_type='clawback' AND fee<=0), so any third value makes both disjuncts false and is rejected there first. Measured, not reasoned: an insert with entry_type='bogus' on dev came back rejected by ..._amount_sign_check, never by ..._entry_type_check. The constraint stays as an explicit statement of the allowed set, but it is now labelled as not-a-reachable-guard so nobody reads it as coverage. If the sign check is ever loosened this becomes live and needs its own negative control. APPLIED AND VERIFIED — dev first, then prod, per rule 8. dev cnpg-cluster-dev-1 / cnpg-database-dev prod cnpg-cluster-nvme0-5 / cnpg-database (primary re-derived live), with SET lock_timeout='5s' — the four FKs take a lock on User, a hot table, so failing fast beats queueing behind it Both databases: 18 columns, 4 foreign keys, 5 checks, 4 indexes, 0 rows, with a positive control (a bogus column name returns 0, so the query discriminates). block_spend_attribution unchanged at 602 rows. DDL replayed to both prod standbys (-4 and -7 report 18 columns). Constraint behaviour was exercised on dev, not merely confirmed present: a valid row inserts (positive control), and 7 negative controls are all rejected — 6 of them by their own named constraint. The seventh is finding 2 above. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(app-blocks): round 0 — two comments asserted seams that do not exist, and the new-table rationale was false Round 0 (requirements & deletion) found five things wrong with what this PR ASSERTS rather than with what it does. Four are fixed here; the fifth is a deletion decision for the operator and is left open. 1. THE NEW-TABLE RATIONALE WAS FALSE, in both the migration and the Prisma docblock. It claimed block_spend_attribution is "IMMUTABLE by design". It is not — that model carries status, voidedReason, confirmedAt, voidedAt, paidOutAt and payoutId, the same accrued/settled/voided lifecycle plus a payout key. Those columns are merely dead today because the rail that wrote them was removed, which is a different claim. A reader who checked the schema would have found the lifecycle columns and concluded the reasoning was wrong. The requirement survives on a better reason, verified rather than substituted: recordSpendAttribution runs inside `void (async () => { … })()` so that a failed attribution write can never break a generation — droppable telemetry. An accrual is a money obligation and must be awaited. The seam, not the row shape, is what separates them. 2. A comment named `chargeBlockAuthorFee` "in the router" as the caller that performs the debit. That function has never existed anywhere in this repository; the name appeared only in that sentence. 3. A comment claimed "the charge path reads this same predicate before taking the money" about the self-dealing exclusion. There is no charge path and no shared predicate — slice 1 has no self-dealing check at all. Corrected, and turned into an explicit obligation on slice 2b: call this before the debit or extract it, because the exclusion is NOT enforced upstream today. 4. BlockAuthorFeeAccrualStatus and BlockAuthorFeeEntryType were exported and then never referenced, including inside their own file — every status was written as a bare literal, so a typo'd 'setled' would have compiled and matched nothing. Now bound to named constants used at all ten write and compare sites. 5. The settlement job's getJobDate/setLastRun cursor gated nothing. settleBlockAuthorFees scans `status: 'accrued'` with no date filter, so the cursor was read only to interpolate into a log line and then written back — two DB round-trips and a persisted KeyValue row no branch consulted. The real idempotency mechanism is the deterministic externalTransactionId. Removed; a cursor that reads like a run-once guard while guarding nothing is worse than none. Also: the D<n> labels had no referent inside this repository — they index an internal decision memo that is not here, so a bare "D6" was authority a reader could not resolve. D1, D6 and D7 are now stated in full at the top of the service, and the note records that D8 and D10 implement nothing in this file. And a caveat that was missing entirely: a newly added cron is NOT picked up by a deploy. Jobs are discovered through /api/internal/get-jobs and the external scheduler needs an explicit refresh, so this job will not be dispatched until someone performs that out-of-band step. Merged does not mean running. VERIFICATION after the fixes pnpm typecheck OK - 0 type errors settlement suite 17 passed mutation sweep 6/6 still killed, each by its own named test — re-run after the literal-to-constant refactor, since that touched all ten status/entry comparison sites LEFT OPEN, deliberately — operator decisions, not mine: * clawbackBlockAuthorFee has ZERO production callers, and its negative carry-forward arm is unreachable until something has settled (two PRs away). * The base rate: bulk-payout-block-attributions.ts is the same machine — registered daily job, idempotent mint, clawback carry-forward, net<=0 hold — built 2026-05-31 and still unwired. mintPayoutForOwner has no production caller to this day. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * refactor(app-blocks): retire the clawback from slice 2a — it had zero callers and an unreachable arm Round 0's deletion candidate, taken. The clawback reverses a charge this PR deliberately does not make, so it cannot be needed yet: * clawbackBlockAuthorFee had ZERO production callers — only its own tests. * Its negative carry-forward arm is reachable only once a row has status 'settled'. Nothing has settled and nothing can until the charge path lands and the job runs for a day — at least two PRs away. The base rate is what made this decisive rather than tidy. bulk-payout-block-attributions.ts is the same machine — registered daily job, idempotent mint, clawback carry-forward, net<=0 hold — built 2026-05-31 and still unwired; mintPayoutForOwner has no production caller to this day. The way that rail rotted is that it shipped ahead of its consumer. Slice 2b brings the clawback back together with the refund path that drives it. REMOVED clawbackBlockAuthorFee, ClawbackReason, ClawbackBlockAuthorFeeResult the entry_type column, its composite unique, and BlockAuthorFeeEntryType the amount-SIGN check (there are no negative rows now) the entry_type check — already measured unreachable, shadowed by the sign check the 'clawed_back' status the non-positive bucket hold, bucketsSkippedNonPositive, and its test 5 clawback tests REPLACED, so the invariant lives where it can actually be violated CHECK (fee_buzz > 0). The settlement job's old "a bucket can never sum to <= 0" branch was unreachable the moment negative rows went away, and an unreachable guard reads as coverage while providing none. The rule is now enforced at the write (accrueBlockAuthorFee refuses fee <= 0) and in the schema, not in a dead branch. Unique key is now workflow_id alone. VERIFICATION pnpm typecheck OK - 0 type errors (one real error caught first: the job still logged the removed bucketsSkippedNonPositive field) settlement suite 11 passed (was 17; 6 removed with the clawback) mutation sweep 4/4 killed, each by its own named test — self-dealing, D6 bucket key, dedup-key composition, zero-fee guard positive control +1000 on the minted amount -> 2 tests red, so the harness genuinely executes the code SCHEMA RE-APPLIED — dev then prod, table dropped and recreated (it was empty and nothing referenced it). The drop was GUARDED: a DO block re-counts rows and inbound constraints at the moment of the drop and raises rather than destroying anything, so a row that landed in between aborts the transaction. Guard printed "0 rows, 0 inbound references" on both. dev cnpg-cluster-dev-1 17 cols, 4 checks, 0 rows prod cnpg-cluster-nvme0-5 17 cols, 4 checks, 0 rows (lock_timeout 5s) standbys -4 and -7 converged to the same after WAL replay; -7 lagged ~30s and was polled to convergence rather than assumed block_spend_attribution unchanged Constraints exercised on dev, not merely counted: a valid row inserts, and 6 negative controls are each rejected BY THEIR OWN named constraint — no shadowing now that the redundant entry_type check is gone. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(app-blocks): round 1 — the settlement rail could lose money one way and pay twice the other Round 1 returned "needs rework" with four blockers. All four fixed. CI was RED at the previous head and I had not checked it after pushing; that is what surfaced three of these. 1. 🔴 THE MINT RESULT WAS DISCARDED, AND ROWS FLIPPED REGARDLESS. createBuzzTransactionMany does NOT throw on a per-transaction failure — its own comment says an insufficientFunds or otherwise-rejected result "is dropped from BOTH arrays: the money did NOT move and it is otherwise invisible". It also filters out transactions failing `fromAccountId !== toAccountId && amount > 0` before calling the service. The old code awaited it, ignored the return, and flipped every row to settled: the author was never paid while the ledger asserted they were, and status='accrued' was the only handle that could have found it afterwards. Now reconciled by COUNT (successes come back as opaque ids, so the count is the only thing that reconciles); a conflict counts as settled because the money already moved under that key. If the count does not account for every bucket, NOTHING is flipped — we cannot tell which dropped, so the rows stay accrued and retry. The precedent is challenge-funding.ts, which carries the same reconciliation and the same comment. 2. 🔴 THE DEDUP KEY NAMED A DAY BUT THE SCAN WAS UNBOUNDED, so it was wrong in BOTH directions, and the module comment confidently asserted the opposite. (a) SILENT LOSS — a second run on the same day swept up rows accrued since the first, minted them under the SAME key, got a benign-looking conflict, and flipped them settled. Money gone. Compounded by (1), which made it invisible. (b) DOUBLE PAY — if the flip failed after a good mint, rows stayed accrued until the next run 24h later, under a DIFFERENT date key, and were minted again. The comment claimed "the next run re-derives the same key"; on a daily cron it never does. Fixed by bounding the scan at the day boundary: a run for day D settles only rows accrued strictly before the start of D. Every row now belongs to exactly one settlement day, derivable from the row, so (a) cannot sweep fresh rows and (b) re-derives the same key and conflicts instead of paying twice. 🔴 This is also why deleting getJobDate/setLastRun last round was not the whole story. It gated nothing AS WRITTEN — that part was right — but it was standing in for a run-once guard, and removing it without replacing the mechanism left (a) exposed. The boundary is the replacement, and it is stronger: it bounds the ROWS rather than the invocations, so it holds under the concurrent runs createJob's lock expiry can produce. 3. 🔴 CI RED — three checks, all mine: * block-spend-attribution-status-default — and this one was emitting a FALSE claim about a table this PR does not touch. The guard filtered migration files with a file-level `sql.includes(TABLE)`, which reads PROSE: this migration's comment header explains why it is a SEPARATE table from block_spend_attribution, which passed the filter; the `"status" TEXT … DEFAULT` regex then matched THIS table's own column and won on last-wins sort order. It reported `provisions DEFAULT 'accrued' … while the schema says @default("tracked")` about a column nobody had changed. That is worse than a false red: the guard exists because a default the payout read does not select makes rows invisible to payout with no error (#4036), so anyone "fixing" it by following its message ships that defect. Fixed in the GUARD — comments stripped, file split into statements, only statements naming the table scanned — not by rewording this migration, which would leave the next table to trip it. * app-access.call-site-ledger — the guard fails on GROWTH because it forces a collaborator decision. Registered: the author fee accrues to the app OWNER only, deliberately not widened to ACCEPTED collaborators. That is the ledger's existing D4 applied to a new earnings surface, not a new decision — earnings are appOwnerUserId-keyed precisely so an ex-owner keeps what they accrued before transferring an app away, and widening the WRITE would make that inexpressible. Resolving the owner at settlement instead would retroactively re-route earnings on every transfer. * ESLint + Prettier — both new files were unformatted. Formatted. Plus no-direct-shared-module-mock: the test now uses the canonical db/logging mocks via the repo's own codemod. 4. 🟡 buzz_type had no CHECK — the one money-critical column without one, while status and governing_leg both had theirs. BuzzAccountType also contains BANK types (creatorProgramBank, cashPending, cashSettled, club), and settlement cast the column to it unchecked, so a junk or bank value would surface only as a silently-dropped transaction. Now CHECK IN ('blue','green','yellow','red'), verified on dev: 'creatorProgramBank' rejected by that constraint by name, 'blue' accepted as a positive control. Also fixed: the settlement scan read the REPLICA while the flip wrote the primary (replica lag would re-bucket an already-settled row); buzzMinted was incremented even when updateMany matched 0 rows, so the job logged an affirmative "minted" for money it had not moved; and the settlement key was built from two independent spellings, only one of which was pinned. TESTS — the absences round 1 named were structural, not oversight: the fake resolved createBuzzTransactionMany to `undefined`, which can express neither a conflict nor a drop. It now returns `{ transactions, conflicts }` like the real one, and five tests were added: mint-did-not-reconcile flips nothing, a conflict counts as settled, the day boundary is midnight of the settled day, a concurrent flip claims no buzz, and one key serves both the mint and the row stamp. VERIFICATION pnpm typecheck OK - 0 type errors the four red suites 50 passed (4 files) prettier --check clean (positive control: the earlier --write listed all five paths by name, so they do resolve) mutation sweep 6 real mutants, 6 KILLED, each by its own named test: discarded mint result, missing day boundary, replica read, unconditional buzz count, duplicated key spelling positive control +1000 on the minted amount -> 2 tests red 1 SURVIVOR, and it is an EQUIVALENT MUTANT, not a gap: dateStr off `date` vs off `boundary` is the same string for every input (toISOString is always UTC and boundary is midnight of that same UTC day; checked across day edges and a year boundary). Recorded in-code so nobody writes a test asserting a difference that cannot exist. SCHEMA re-applied dev then prod for the buzz_type CHECK, same guarded drop (re-counts rows and inbound references at the moment of the drop, raises rather than destroying). All three prod instances: 17 cols, 5 checks, 0 rows. Standby -4 lagged ~30s and was polled to convergence rather than assumed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(app-blocks): round 2 — the settlement key named the RUN day, so a deferred retry paid twice Round 2 found that round 1's flagship fix did not do what it said, and that its fail-closed branch made things worse. Both confirmed against the code before acting; both were mine. 🔴 F1 — THE KEY WAS DERIVED FROM THE INVOCATION, NOT FROM THE ROW. `dateStr` came from `args.date` — the day the job ran — while the scan admitted every accrued row below the boundary, i.e. all of history, bucketed by (owner, buzzType) with no accrual day. So a row whose flip failed on 09-18 was re-scanned on 09-19 and minted under `…-2026-09-19-…`: a NEW key, no conflict, owner paid twice. That is exactly the failure mode the module comment claimed the day boundary had fixed — asserted three times, false every time. The sentence "an unflipped row still belongs to its original day, so its key is unchanged" was the worst of them: the day never came from a row. 🔴 F2 — THE FAIL-CLOSED BRANCH FED F1. One dropped bucket meant NOTHING in the batch was flipped, including buckets whose money had already moved — and those then sat `accrued` until the next day, where F1 re-minted them. Round 1 introduced this while fixing an under-payment, and its comment claimed the trade was "money owed, which is recoverable". It was money PAID TWICE, which is not. 🟡 F3 — a same-day second run still lost money via `take` truncation: run 1 mints a partial bucket and flips, run 2 picks up the remainder, builds the same key, gets a conflict, and flips those rows without ever paying for them. A conflict says that KEY minted; it does not say those ROWS were in it. THE REDESIGN — one complete accrual day at a time, keyed on the row. * The loop settles the OLDEST unsettled accrual day, scanning exactly [dayStart, dayEnd) rather than everything below a boundary. * The bucket carries `accrualDay`, and `keyForBucket` builds the key from it. Every component now comes from the rows; nothing comes from the clock. A retry tomorrow, next week, or after a month with the flag off re-derives the SAME key and conflicts. F1 closed. * A day that exceeds `limit` is SKIPPED WHOLE and reported (`daysTruncated`), never cut. `take: limit + 1` makes the overflow detectable instead of silent. A partial bucket is what F3 lost money to, so the rule is that a bucket is always settled whole. F3 closed. * ONE BUCKET PER MINT CALL, so a drop is attributable. createBuzzTransactionMany reports only counts and opaque ids, which is why the batched version could not say which bucket failed and answered by flipping nothing. Per-bucket makes it answerable: this bucket moved, or it did not — and its peers are unaffected. F2 closed. 🟢 F6 — the "EQUIVALENT MUTANT" comment was FALSE as written, and I had checked it with fixtures that could not see the counter-example. Date.UTC maps years 0-99 to 1900+y, so year 0026 gives `0026-…` from one spelling and `1926-…` from the other. Unreachable in production, but the comment forbade writing the test that would have found it. Gone with the rewrite. 🟡 F4 — the job's prose contradicted the service: it still said the scan had "no date filter" (the recorded rationale for deleting the cursor) and "since the last run". Both corrected, and the cursor's real justification stated: idempotency lives in the ROWS, not the invocation, which is what holds when createJob's lock expires and two runs overlap. 🟢 F5 — the status-default guard stripped only `--` comments while Prisma-generated migrations in this repo open with `/* Warnings */` headers, so the prose was wider than the code. Both comment forms are stripped now. VERIFICATION pnpm typecheck OK - 0 type errors affected suites 52 passed (4 files) prettier clean mutation sweep 7 mutants, 7 KILLED, each by its own named test: run-day key, truncated oversized day, flip-on-drop, un-day-scoped scan, D6 bucket collapse, unconditional buzz count positive control +1000 on the minted amount -> 2 tests red No SURVIVORS this round. The previous round reported one and explained it away as equivalent; that explanation was wrong (F6), which is a reason to distrust a survivor-with-a-story rather than to be reassured by one. Schema unchanged — no migration in this commit. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(app-blocks): round 3 — the day loop could never advance past a day it could not finish Round 3's verdict was "merge after fixing 🔴-1". The money path held this time — eleven of thirteen claims landed clean and no double-payment or silent-loss path survived — but the rewrite introduced a LIVENESS defect, and my own mutation sweep could not have caught it. 🔴 A DAY THE LOOP COULD NOT FINISH BLOCKED EVERY LATER DAY. The loop re-derived "the oldest unsettled day" on every iteration with nothing to advance past one that did not complete. Two arms: * an OVERSIZED day hit `break` and stayed the oldest forever, so every later day was blocked permanently — and since the job passes no `limit`, the only recovery was a code change and a deploy; * a persistently REJECTED bucket left its rows `accrued`, so the next iteration re-selected the same day and re-minted it, burning all 30 iterations on one stuck owner while every other author stopped being paid. Neither risks money — the row-derived key still makes a retry conflict — but both halt settlement for everyone else behind a single log line. The previous round's comment even stated the rule for the second arm ("the day is still the oldest, so the next iteration would select it again and spin") sixty lines above the code that did exactly that. FIXED with a monotonic per-run cursor: the day selection asks for the oldest day AT OR AFTER `cursorFrom`, and the cursor advances to `dayEnd` BEFORE any early exit, so every path leaves its day behind. A stuck day now costs one iteration and is retried on the next run instead of wedging the queue. The oversized and emptied-day arms `continue` rather than `break`, which is only safe because the cursor makes a spin impossible. 🟡 THE MINT IS NOW WRAPPED. The buzz client THROWS on any non-2xx and its retry allowlist covers only connection-level errors, so a 5xx throws on the first response. Unwrapped, bucket 1 of N aborted buckets 2..N, the day loop and the completion log — and splitting one batched call into N per-bucket calls multiplied that exposure by N, so this was a hazard the previous round's own fix created. A throw is now handled exactly like a drop: rows stay `accrued`, the same key is re-derived next run, peers still settle. 🟢 Two claims corrected rather than reworded: the module header still described a `Math.floor` the previous commit deleted (the only remaining `Math.floor` in the file was inside the sentence about it), and "a conflict is the same payment" was stated unconditionally when it holds only while no row can JOIN an already-minted (day, owner, currency) group. That precondition is now written down, along with what breaks it — a backfill writing a historical `accrued_at` would turn it into a silent underpayment. 🔴 MY OWN MUTATION SWEEP WAS THE REAL FINDING, and it is the one worth carrying forward. Every settlement test used a single-day fixture, so the loop body ran exactly once in every test. The multi-day walk, the cursor, `maxDays` and the continue-vs-break semantics were structurally unreachable — a mutant over any of them would have killed nothing. Last round's "7 mutants, 7 killed" was therefore a true statement about seven mutants that did not include the control flow that round had just rewritten. A sweep is only as wide as the mutants you imagined. Fixed by a `days()` fixture that drives N days, plus 7 tests: the multi-day walk, an oversized day not blocking later days, a dropped bucket not blocking later days, a thrown mint treated as a drop, the cursor advancing strictly, `maxDays`, and a day that empties under the loop. ⚠️ AND THAT FIXTURE IMMEDIATELY EXPOSED A TEST-ISOLATION LEAK: `vi.clearAllMocks()` clears call history but NOT `mockResolvedValueOnce` queues, so a day queue left by one test was consumed by the next and the suite was order-dependent. The mocks fed with `...Once` are now explicitly reset. VERIFICATION pnpm typecheck OK - 0 type errors affected suites 59 passed (4 files) prettier clean mutation sweep 11 mutants, 11 KILLED, each by its own named test — five of them loop control flow (no cursor advance, cursor absent from the query, oversized-day break, empty-day break, maxDays ignored) and six money path (run-day key, truncate oversized day, uncaught throw, D6 collapse, ...) positive control +1000 on the minted amount -> 2 tests red Two mutants needed a second pass and both are recorded rather than quietly re-run: `empty_day_breaks_run` SURVIVED the first sweep — a genuine gap, closed by the new empty-day test — and `oversized_breaks_run` was SKIPPED because its pattern matched 10 sites, which is a sweep that reports nothing while looking like it ran. Schema unchanged; no migration in this commit. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(app-blocks): round 4 — the wrap stopped one statement short, and a false claim about retries Round 4's verdict was SAFE TO MERGE with no blockers: the three liveness defects round 3 set out to fix are genuinely fixed, verified by watching five mutants go red rather than by reading the claims. What it found was four 🟡s of one shape — the code is right and the comment claims more than the code does — plus two gaps in my own mutation sweep. 🟡 THE RETRY CLAIM WAS FALSE, IN THE REASSURING DIRECTION. The comment said the buzz client's "retry allowlist covers only connection-level errors — a 5xx or a 504 throws on the first response". Traced and wrong: `createTransactions` calls `post` with no options, so `shouldRetry` is undefined, and `withRetries` reads `shouldRetry ? predicate(...) : true` under a comment saying "Absent predicate keeps the historical behaviour: retry everything". `isSafeToRetry` is exported and never applied on this path. So a 5xx IS retried at the client default (3), and a run with N payable buckets can issue up to 4N POSTs during an outage. That paragraph was the file's only statement about mint cost, so it is what someone would have sized a timeout or an alert against. 🟡 THE DIAGNOSTIC COULD NOT TELL PERMANENT FROM TRANSIENT. `buzzService` is built with a `mapError`, so every non-2xx arrives as a TRPCError carrying the fixed string "An unexpected error ocurred, please try again later". A permanent 400 and a transient 503 produced BYTE-IDENTICAL log lines, repeated daily forever, so the permanent one was indistinguishable from noise — and the repo already ships `getBuzzApiStatus` to read the real status back through the wrapper. Now logged. 🟡 THE WRAP STOPPED ONE STATEMENT SHORT. The mint was wrapped; `updateMany` one line below it was not — and the sentence justifying the wrap ("bucket 1 of N throwing aborts buckets 2..N, the day loop and the completion log") stayed true, verbatim, of the unwrapped call. `id: { in: rowIds }` can carry up to `limit` ids, so a statement timeout there is not exotic. Money was safe either way; what it cost was every remaining bucket and every later day in the run. Wrapped, with its own log line — a flip that throws after a successful mint is the one case where money moved and the rows do not say so. 🟡 THE CURSOR BOUNDS BLOCKING WITHIN A RUN, NOT ACROSS RUNS — and round 3's comment claimed the stronger property ("nothing is abandoned permanently"). Both stuck-day arms are permanent: an oversized day is unsettleable until someone raises `limit`, and a persistently-rejected owner's rows stay accrued forever. Each is re-selected on every later run, so 30 accumulated stuck days would consume the whole budget and settlement would stop for everyone — the same end state the cursor was added to prevent, reached 30 days later. `maxDays` now counts only PRODUCTIVE days, with `maxIterations` (default maxDays * 4) as the absolute bound. 🟢 Removed an unkillable clause rather than leaving it to read as a guard: `threw === null &&` in the `moved` expression. `mint` is assigned only inside the `try`, so it is null on every throw path and the expression was already false there — measured, deleting it left the suite green. 🟢 Completed the test reset list: the comment claimed EVERY `...Once` mock was reset, while `create` — fed `mockRejectedValueOnce` twice — was not. 🔴 TWO GAPS IN MY OWN SWEEP, both closed, both worth recording: * `maxIterations`'s DEFAULT was unpinned — the only test exercising the cap passed one explicitly, so a mutant replacing the default SURVIVED. * `threwStatus` was unpinned — a mutant nulling it SURVIVED. And the first fix was initially WORSE than the gap: feeding the same stuck day forever "killed" the mutant only by HANGING the suite — 43s, `tests 0ms`, no named failure, indistinguishable from a CI timeout. It was also a mock artifact: in production the cursor is monotonic and bounded by `boundary`, so an unbounded default could never hang. The day supply is finite now and the mutant fails an assertion in 5ms. ⚠️ AND A REPORTING CORRECTION I OWE: round 3's sweep was reported as "11 killed, each by its own named test". Two of the five loop mutants share one killer, so the independence claim was overstated. This round's sweep prints the FULL killer list per mutant rather than the first. VERIFICATION pnpm typecheck OK - 0 type errors affected suites 64 passed (4 files) prettier clean mutation sweep 9 mutants, 9 KILLED. Killer counts reported rather than assumed: flip-unwrapped 1, stuck-days-toward-budget 1, maxIterations-default 1, threwStatus 1, mint-throw 1, D6 collapse 1, no-cursor-advance 1, run-day key 5. positive control +1000 on the minted amount -> 2 tests red Two mutants needed a precise anchor rather than a shell pattern (18 and 10 incidental matches respectively) — a sweep that SKIPS reports nothing while looking like it ran, which is the same failure shape as a silent zero. Schema unchanged; no migration in this commit. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(app-blocks): round 5 — the productive-day fix covered one of the two arms it named, and a test I reported as closing a gap did not Round 5 returned three 🟡s. Two of them refute claims made in the previous round's commit message, and I verified both against the code before acting. 🟡 F1 — THE PRODUCTIVE-DAY COUNT DELIVERED HALF OF WHAT ITS COMMENT PROMISED. `productiveDays += 1` fired as soon as a day had rows, BEFORE a single mint was attempted. The comment enumerated two permanent stuck arms and said the count now excluded both. Only the oversized arm was excluded — it `continue`s above the increment. The persistently-rejected-owner arm, named verbatim in the same comment, reached the increment and was counted productive even when every bucket dropped and zero rows flipped. So one permanently-rejected owner still accrues one stuck-but-"productive" day per day, and thirty of them exhaust `maxDays` on stuck days alone — settlement stops for everyone, which is the failure the count was added to prevent. The test named for that behaviour only ever exercised the oversized arm (`limit: 1`), so it passed while its own arm was uncovered. Now counted AFTER the bucket loop, conditional on something landing, and pinned by a test that drops every bucket on two days and asserts the third still settles. 🟡 F2 — THE MOTIVATING EXAMPLE WAS THE ONE CASE WHERE THE CLAIM IS FALSE. The new `threwStatus` comment said "a permanent 400 and a transient 503 produce BYTE-IDENTICAL log lines". `mapError` names 400, 404 and 409 explicitly ("Your request is invalid", "Not found", "There is a conflict with the transaction"), so those three were already distinguishable. The fix is still worth having — 401, 403, 408, 429, 500, 502 and 503 all fall to `default` and genuinely are identical, so a permanent auth failure versus a transient outage was the real indistinguishable pair. The example was wrong, not the reason; corrected rather than reworded. 🟡 F3 — A TEST I REPORTED AS CLOSING A MUTATION GAP DID NOT CLOSE IT. The `threwStatus` test used a plain Error carrying a `status` property and asserted `toHaveProperty('threwStatus')` — a single-argument EXISTENCE check. `getBuzzApiStatus` returns undefined for that shape, so the field logged `null`, and the mutant `threwStatus: null` SURVIVED while the test passed. Re-measured here before fixing: mutant applied, 30/30 still green. The previous commit and PR comment both recorded that gap as closed. It now uses a real `BuzzApiError` inside a `TRPCError` cause — the shape production actually produces — and asserts the VALUE is 503. 🟡 F4 — WRAPPING THE FLIP MADE ITS FAILURE SILENT. Before the wrap an `updateMany` throw failed the job and was loud; after it, the run returns success with only an Axiom line. A systemic flip failure would report `rowsSettled: 0` on a job that says it succeeded, every night, while money left on day one of each bucket. Added `flipFailures` to the result and the job log — the one counter that means money moved without a settled row — and put the amount on the log line, which carried `settlementKey` but not the sum. 🟢 F5 — the flip-failure message asserted a state the code cannot observe. A connection drop after the UPDATE commits raises with the rows already flipped, so "money moved, rows still accrued" would be the opposite of the truth on a line an operator acts on. Now "mint landed, flip did not confirm". 🟢 F6 — the `buzzMinted` comment claimed `count > 0` distinguishes "a payment this run did not make". It does not: it cannot tell a fresh mint from a conflict on a key an earlier run already paid, and the wrapped flip makes that path ordinary. The cross-run total stays correct, so this is reporting, not money — stated rather than reworded, because the exact claim is what a reader would rely on. VERIFICATION pnpm typecheck OK - 0 type errors affected suites 65 passed (4 files) prettier clean mutation sweep 3 mutants re-run against the new guards, 3 KILLED, each by its own named test: threwStatus nulled (this one SURVIVED before the fix — re-measured, not assumed), productiveDays counted unconditionally, flipFailures not counted. ⚠️ SCOPE OF THAT SWEEP, STATED BECAUSE THE LAST FOUR ROUNDS OVERSTATED THEIRS: it covers the three guards this round added or repaired. The nine mutants from round 4 were not re-run. Schema unchanged; no migration in this commit. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * refactor(app-blocks): split slice 2 — this PR is the ACCRUAL LEDGER only, settlement goes to 2b The settlement rail has no consumer. `accrueBlockAuthorFee` has zero callers on main, so the table it writes accrues nothing, so the daily job it feeds settles nothing — a cron, a Buzz-minting path and 25 tests whose first real exercise would be the day a charge path lands. That is a lot of unexercised money-moving code to carry through review on the strength of a table that is empty by construction. It moves to `zach/app-blocks-author-fee-slice2b` and comes back with the viewer-charge path that gives it rows. What leaves this PR: - `settleBlockAuthorFees`, `SettlementBucket`, `SettleBlockAuthorFeesResult`, `utcDayStart`, `keyForBucket` - `src/server/jobs/settle-block-author-fees.ts` and its registration in the run-jobs webhook (reverted to the main version verbatim) - the 25-test settlement suite What stays — the ledger, which is the part that has to exist first: - the migration, unchanged and NOT re-timestamped; it is already hand-applied to dev and all three prod instances - `accrueBlockAuthorFee` and the six tests covering it - the `bafa_` id helper - the call-site-ledger registration and the migration-comment filtering fix RENAMED `author-fee-settlement.service.ts` to `author-fee-accrual.service.ts`. A file named for settlement that settles nothing is the same defect this PR's own audit ladder found four times in rounds 4 and 5 — a name claiming more than the code does. The call-site ledger keys on the file path and fails on both GROWTH and SHRINK, so its entry moved with the file. `STATUS_SETTLED` is now exported despite nothing in this slice reading or writing it. It names one of the two states of a CHECK-constrained column this slice's migration ships, and 2b is the writer; exporting it keeps one spelling of the literal across both slices rather than letting 2b re-declare it, where a typo would match no row and fail silently. Its comment says so, so its presence is not read as evidence that anything settles. Kept as a commit on top rather than a force-push: the six audit rounds behind the settlement code are the record of how it got correct, and 2b inherits that history. The PR diff is computed against the merge base, so what reviewers see is the ledger-only slice either way. Rail is still DARK: 0 callers of `accrueBlockAuthorFee`, flag `app-blocks-author-fee-enabled` is `enabled: false`. Tests: 4323 -> 4298 in the blocks suite, exactly the 25 settlement tests, moved not deleted. The four-suite run goes 65 -> 40. Typecheck 0 errors; the 43-file lint-rules suite is green at 599. --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
Justin Maier
|
8aa9e5cef2 |
revert(remix): drop the verify hint from the remix menu (#4951)
Justin reviewed it on a dev server and does not want it there. The menu is back to exactly its pre-#4939 state — same three options, same labels, no per-option annotation. What stays from that PR is remixClaimState in utils/remix-claim.ts, which has no user-visible surface and is what keeps the 0.75 rule to one derivation for the free-path work. Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
Justin Maier
|
c4c402ef40 |
fix(search): page the models delta scan by keyset, not OFFSET (#4938)
* fix(search): page the models delta scan by keyset, not OFFSET prepareBatches walked the set of models updated since the last run with an unordered OFFSET/LIMIT loop. The set is re-evaluated on every page and its membership moves while the scan runs: an edit that unpublishes a model, or flips it to Unsearchable, removes a row from under the cursor and shifts every later page down by one, so a model eligible for the whole scan is silently never indexed. At ~1,659 published edits per day a multi-page scan meets that routinely, and this loop is also what an index repair leans on. Ordering the OFFSET query would not have fixed it -- order was never the problem, membership was. Page by a forward-only id cursor instead, which is unreachable by a membership change because ids are immutable. prepareBatches is hoisted to an exported prepareModelsBatches, mirroring prepareUsersBatches, so the paging can be driven by a test. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * test(search): make the keyset paging fake refuse what it cannot read Review found the fake answered queries it had not understood, so three mutations of the production query passed: - ORDER BY id DESC passed all four cases. The fake sorted ascending in both arms whatever the SQL asked, and /ORDER BY id/ matches DESC. Against a real database that mutation walks the cursor backwards from the top of the table and the scan never terminates. - A literal LIMIT 2000 fell through to a members.size default, handing back the whole set on page one -- at which point the headline case passes under an OFFSET implementation too. - id >= instead of id > fell through to an uncursored read and reddened with "the scan is not advancing", which is not what that defect does. The fake now honours ORDER BY direction and throws on a query whose LIMIT or cursor it cannot find. Case 1 also asserts the mid-scan edit actually landed -- keyset is meant to be unmoved by it, so nothing else in that case could tell a dead mutation from a working one -- and pins batchSize against the production constant so page-size drift names itself. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * test(search): pin the page query's eligibility predicates The paging fake models membership as an opaque set of ids, so it cannot see which rows the WHERE clause selects. Two review lanes independently demonstrated the consequence: deleting any one of the three predicates left the whole suite green. Dropping the updatedAt bound turns the delta scan into a full scan of every published model every 15 minutes; dropping the availability bound puts Unsearchable models into the public index. Both are the shape of an ordinary WHERE-clause tidy-up, and this is the only test that reads this query. Pinned textually rather than by teaching the fake to carry per-row timestamps: one assertion covers all three predicates where a behavioural fixture would only cover updatedAt, and a smaller fix round is the safer one. The watermark comment now names the symbols it depends on instead of restating base.search-index's semantics, which would rot silently if that file changed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * test(search): pin the page query by whole clause, values, and the empty page Round three of review found three mutations of the query that left the suite green. Substring assertions were the common cause. - Widening. Appending `OR availability = 'Unsearchable'` leaves every asserted fragment present while AND binds tighter than OR, so every Unsearchable model is returned on every page. The sibling test next door already carried a written record of an adversarial round that beat this same assertion shape, so its `norm`/`renderTag`/`whereClausesOf` helpers move to `sql-shape.test-utils` and both files now pin a whole normalised clause with toBe rather than substrings. - Value corruption. `renderTag` renders a bind param as `?`, so the clause is blind to values and `Availability.Unsearchable` -> `Private` was green. The page query's bind values are pinned separately. - The empty-page break was unreachable: every fixture ended on a short page, so deleting `if (!ids.length) break` left the suite green while production reads `ids[ids.length - 1].id` off an empty array and kills the index job on any run where the eligible set is an exact multiple of the page size, or empty. A fixture of exactly READ_BATCH_SIZE members reaches it; the deletion now fails with that same TypeError. No production change in this commit. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * test(search): pin that the processor runs the function under test Round four of review demonstrated the cheapest possible revert of this PR: leave the exported prepareModelsBatches alone and re-inline the old OFFSET body at the wiring site. Production goes back to row-losing paging and all 17 tests stay green, because every case imports the exported function directly and nothing in the repo read modelsSearchIndex.prepareBatches. The test file's own header claimed such a revert would redden it. That was false. createSearchIndexUpdateProcessor now returns prepareBatches, alongside the updateSyncChunkSize it already exposed for the same reason, and the test asserts the processor runs the function it drives. Measured cost, recorded in the test: a behaviour-preserving wrapper at the wiring site also fails this. That is inherent to an identity assertion, and the fix is to keep the wiring a direct reference. Also drops `toBe` from the sql-shape docstring, which named an assertion neither of its two callers uses. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * test(search): pin the bounds query and the id range it returns prepareModelsBatches returns four things; this file pinned two. Nothing read startId/endId, and the fake short-circuited the MIN/MAX statement on text.includes('MIN(id)') and handed back a hardcoded row, so the whole bounds query was invisible. Review demonstrated two mutants that left every case green: - swapping the aliases to MAX(id) as "startId", MIN(id) as "endId" makes endId - startId negative in base.search-index's range fan-out, so newly created models silently stop being indexed on every run; - deleting the bounds query guts the full rebuild, which then indexes zero models while the case named "issues no page query at all on a full rebuild" stays green, because it only asserts no page query fired. The fake now answers the aggregate the statement asked for, rather than returning fixed numbers under fixed names, so an alias swap produces a different id. The bounds statement's WHERE clause and binds are pinned the way the page query's already were. The "createdAt" bound there against "updatedAt" on the page query is pre-existing and deliberate, and is now pinned so a one-word change between the two cannot pass unnoticed. The file header claimed the fake refuses a query it cannot read. The bounds query was the one place it defaulted instead, which is why this survived six rounds; the header now says what the fake actually does. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * test(search): pin the rebuild path's bounds, and both statements' table The rebuild branch was the one place startId/endId are the entire output, and the only case on it asserted that updateIds was empty and no page query fired. Both of those are also true of a rebuild that does nothing: returning { startId: 0, endId: 0, updateIds: [] } early for a missing watermark passed every case, and makes base.search-index's range fan-out zero tasks, so a whole index rebuild creates no batches and indexes nothing. Neither statement's table was pinned either. whereClausesOf captures from WHERE onward, so FROM "Model" -> FROM "ModelVersion" in either query left the file green while the scan paged a different entity. Also corrects the header's frequency claim. It said a multi-page scan meets concurrent edits as a matter of routine, citing ~1,659 edits a day; at a 15-minute cadence that is ~17 rows against a 2,000-row page, which argues the single-page case. The PR description was corrected for this and the source was not, which left the retracted claim in the file the next reader actually opens. The corrected text also states the mechanism that does not need an edit at all: no ORDER BY over a parallel seq scan lets synchronize_seqscans cut successive pages out of different orderings. That paragraph first shipped broken, because a cron string in a block comment closes it. The comment now says so rather than leaving the next person to rediscover it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
briant
|
c6da1a2dc4 |
fix(ingestion): stop the retry cron double-submitting a scan already in flight
The Image INSERT trigger queues a new image immediately, but ingestImage stamps scanRequestedAt only once its upload-path submit returns. A cron run landing inside that window read the NULL as "never submitted" and submitted a second workflow for the same image. Measured on prod 2026-09-18: of 41,865 images scanned in 12h, 166 carried two workflow ids; 161 of those were created within 30s of a cron tick, against a 9.8% uniform baseline, spread over 112 users. A Pending image with no scanRequestedAt is now deferred for SUBMIT_IN_FLIGHT_GRACE minutes. It stays in the JobQueue while deferred — without that it prunes as stale and an image whose submit died silently would never be re-driven, which is the trigger's whole purpose. The deferral count is reported alongside waitingForRetry. Two existing fixtures modelled "new image, never submitted" as createdAt: now, which the grace defers; aged them past it so each test still exercises its own subject. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> |
||
|
Justin Maier
|
d7038c5aa8 |
fix(home-blocks): hydrate the viewer's own reactions on cached image blocks (#4935)
* fix(home-blocks): hydrate the viewer own reactions on cached image blocks Home block payloads are stored in one Redis entry with no user segment AND served through edgeCacheIt with canCache left true, so every viewer is handed the same image objects with `reactions: []`. reaction.toggle acts on the database row rather than on what is drawn, so a viewer whose reaction shows un-highlighted clicks it and deletes the reaction they already had. Keeps the shared payload anonymous and hydrates on the client instead: a new reaction.getMyImageReactions procedure, and a useHydratedImageReactions hook that the three home blocks rendering ImageCard call on the list they hand to ImagesProvider, which is the same window the image detail dialog browses. The query and its grouping already existed twice inside image.service.ts; both now call one exported getUserReactionsForImages, which the new procedure is the third caller of. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(home-blocks): make the hydrated list the only one a block can render Review found the guard proved the blocks MENTION the hook, not that the hydrated array reaches the cards: keeping the call and rendering a second, un-hydrated binding restored the bug in full with the suite green. Fixed structurally rather than by a stronger string match. The hook result is now passed straight into useDedupedCappedItems, so the capped list is the only array in scope, and the guard asserts that shape. Hydrating before the cap also stops the query key churning as earlier blocks publish their dedupe claims. Also from review: - reactionQueryChunks is pure and exported, so the signed-out gate has a test. Losing it is one UNAUTHORIZED request per home block for the majority of front-page traffic. - The chunk size and the input schema's max are two copies of one number; a test pins them equal. Divergence fails zod, React Query swallows it, and the grid silently stays un-hydrated. - `reactions` is optional on the hook's item type, so the collection blocks pass their union in without a cast, and the merge reads it with `?? []`. - chunkIds moves to array-helpers; sticker.util, StickerPlacementBatchProvider and RemixGalleryBatchProvider had three copies of one body between them. - Dropped the `toContain('useHydratedImageReactions')` assertion: deleting the call leaves the import, so it barely fails. The structural assertion subsumes it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(home-blocks): correct the measured cost, and stop re-asking per mount Round 2 of review. The cost model I wrote into the hook's doc comment was wrong in the direction that understates it: a FeaturedCollections block renders one section per pick and its renderCount is 5, so its picks do not share a call, and one of the three Feed blocks carries models and asks nothing. Nine requests on today's prod home page, not six. The per-request figure was measured at the post-cap shape this branch replaced; over a block's whole pre-cap pool it is 0.4-1.2 ms, about 4.6 ms of replica time for the page. Both numbers corrected rather than dropped, since a follow-up ticket quotes them. Two behaviour changes, both from the same review: - The ids are sorted before chunking, so a repeat visit can reuse the answer. Every block shuffles its pool on mount, which made the query key fresh every time and `staleTime` nearly inert. The sort lives here and NOT in `chunkIds`, whose other callers page and need insertion order. - Hydration waits on hidden preferences. `useApplyHiddenPreferences` does not block, so between the payload landing and the preference maps resolving it hands back the unfiltered pool; asking about that first spent a whole extra round of requests per cold load. The guard now accepts either nesting order. The property that kills the un-hydrated binding is the inlining, not which hook is outermost, and hydrating after the cap is a defensible shape this guard has no business forbidding - it was this branch's own design one commit ago. Its comment also stops claiming the bug is impossible to write: `filtered` is still a binding. What the guard removes is the INVITED mutation. The router-rung assertion is an allow-list of authed procedures rather than one spelling, so it reds on a loosening and not on a tightening. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(home-blocks): take the entity, not a boolean the caller derives Review found a green mutant against the subject: flipping `{ enabled: !loadingPreferences }` to `{ enabled: loadingPreferences }` switched hydration off on every render that draws a card, because the blocks return a skeleton while that flag is true. Every check in this repo stayed green - the guard reads the composition rather than the argument's value, and the pure tests prove the gate behaves correctly with the boolean it is handed, never that the boolean handed to it is right. So the boolean is gone rather than guarded. The hook takes the entity type, required and typed, and derives `entity === 'image'` in one place the existing pure tests already reach. A mistyped 'images' is now a compile error; there is no default to flip; and `type === 'image'` is no longer restated at three call sites. The `!loadingPreferences` half is deleted outright rather than moved. It was a no-op: `filterPreferences` returns `items: []` while preferences load, so the hook was already being handed an empty pool, never the unfiltered one. The comment claiming otherwise was the stated rationale for the gate, duplicated in all three blocks. The rung check reads every occurrence rather than `match`'s first. A doc comment above the declaration that quotes it - the natural thing a future editor writes - would otherwise satisfy a first-match check while the declaration underneath said something else. Three comment corrections, all of them claims that had stopped being true: - The blocks said inlining left "the only array in scope". It does not; `filtered` is still there. What inlining removes is the INVITED mistake, which is the one that happens. - The pre-cap move was justified partly on the detail dialog browsing the wider window. It does not: all three blocks hand `ImagesProvider` the capped list. The decision rests on the re-keying alone. - `chunkIds`' docstring argues against sorting and now has a caller that sorts first; it says why. The randomised shuffle control is reversed instead, so it is certainly red rather than almost surely. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * test(home-blocks): cover the hook body, which nothing in the repo executed Two lanes independently found the same class: `reactionQueryChunks` and `mergeUserImageReactions` are well covered, and the composition between them was covered by nothing. Three one-token edits inside the hook body switch hydration off on the front page with typecheck, eslint, the guard and the whole unit suite green - the `byImageId` memo dep, the final memo's dep list, and the `query.data ?? {}` fallback. Nothing else can see those. `react-hooks/exhaustive-deps` arrives as a WARNING via next/core-web-vitals, `lint` is `eslint src/` with no `--max-warnings`, and the CI workflow says so out loud - "Errors only (no --max-warnings): the repo has 3,470 warnings". So dependency-array correctness has no automated enforcement in this repo, and one of the two dep lists sits under an `eslint-disable-next-line react-hooks/exhaustive-deps`, which makes "clean up the disabled rule" an ordinary edit for someone who has never read this ticket. The test renders the hook with a stubbed `useQueries`, asserts the un-hydrated state synchronously as a negative control, then makes the queries report data and asserts the hydrated state. It asserts a state that ARRIVES, so there is nothing on a timer to race. Two things this round got wrong first and are worth recording: - The stub originally returned its canned results whatever the hook asked for, so the non-image case passed data to a surface that had issued no query. It now returns one result per chunk, as the real `useQueries` does. The negative control is what caught it. - The first sort-direction assertion, `chunks[0][0]).toBe(1)`, could not fail: 1 sorts first lexicographically too. Measured, not assumed - the mutant stayed green on that line. It now asserts where the two orders actually disagree, at the second element. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * test(home-blocks): make the query stub faithful in content, not only in count Follow-up on the hook-body test, from the same lane that asked for it. The stub kept the descriptor COUNT honest and discarded what was in them, so `{ imageIds: chunk }` changed to `{ imageIds: chunks[0] }` stayed green — and in production that makes every chunk after the first ask about the first one's ids, so the back half of a large grid never hydrates and its cards go back to deleting on click. The stub now keeps the descriptors and a test asserts the hook asked each chunk about its own ids. Control: that mutation reds with `expected [ 100, 100 ] to deeply equal [ 100, 50 ]`. That path is unreachable on today's config — `FEED_FETCH_CEILING` and the collection limit are both 100, which is `REACTION_FETCH_CHUNK`, so a block's pool is always exactly one chunk. The test is there for the day one of those numbers goes up, which is a one-token edit nothing else connects to this hook. The comment says so rather than implying live coverage. Three smaller things in the same family: - The stub mapped one result per descriptor instead of slicing. A `queryResults` shorter than the descriptor list silently handed the hook a shape `useQueries` cannot produce. - `queryResults` is reset in `beforeEach`. Inheriting a previous test's value would inherit it as HYDRATED data, which is the direction that produces a false green. - `images` being hoisted out of the probe is load-bearing for the dep-list control — a fresh identity each render makes the memo recompute regardless — and nothing said so. Now it does. The `entity: 'model'` test keeps its assertion and loses its claim: the stub is what withholds the answer there, so it cannot tell a hook that filters from one that never asked. What it does prove is that the gate survives the whole body, which the pure tests next door cannot. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * test(home-blocks): assert what came back, not only what was asked Round 6. The multi-chunk test proved the hook asked each chunk about its own ids and never looked at the hook's output, so the response half of the same shape was open: collapsing `Object.assign({}, ...queries.map(q => q.data ?? {}))` to `queries[0]?.data ?? {}` was green, and in production that drops every chunk after the first — images 101+ render un-hydrated and the first click deletes. It now asserts both ends, and the assertion on the LAST image is the only place in the suite where a chunk other than the first has to land. Same round, same root cause one argument to the right: the stub captured the descriptor's input and discarded its options, so deleting `{ staleTime: 60_000 }` was green. That is the option the round-3 sort exists to make worth having — a repeating query key buys nothing if nothing holds the answer — and the pure sort tests structurally cannot see it, because it lives in the hook. The stub now captures both arguments and the test pins it. Controls, applied and reverted: - `Object.assign(...)` -> `queries[0]?.data ?? {}`: red, `expected [] to deeply equal [ { userId: 9266475, … } ]` - `{ staleTime: 60_000 }` deleted: red - `{ imageIds: chunk }` -> `{ imageIds: chunks[0] }`: red, `expected [ 100, 100 ] to deeply equal [ 100, 50 ]` Multi-chunk remains unreachable on today's config — both ceilings are 100, which is the chunk size — and the test says so. The point of closing both halves rather than one is that the asymmetry would be invisible to whoever raises that number. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * test(home-blocks): cover the render production actually performs first Round 7 found the third dependency array, and unlike the multi-chunk pair this one is reachable today on every page load. `useApplyHiddenPreferences` returns `items: []` unconditionally while hidden preferences load, so every home block hands the hook an EMPTY array on its first render and the real pool on a later one. Drop the id join from the `chunks` memo's deps and it freezes at `chunkIds([], 100)`: nothing is ever asked, the merge is a permanent no-op, every card renders un-hydrated, and the first click deletes. Typecheck, eslint, the guard and all three existing hook-body cases stayed green — they all start with a NON-EMPTY pool, so a frozen `chunks` is frozen at the right value in every one of them. The new case mounts empty and populates on rerender, which is the sequence production performs. It is deliberately a separate case rather than an amendment to an existing one, and the file now says why: the fixture decides which dep list a case can see. A stable `images` binding can see the `byImageId` and final memos and cannot see `chunks`; a growing `images` reaches `chunks` and cannot see the final memo, because a fresh identity makes that one recompute regardless of its deps. The two shapes are mutually blind, so folding them together would close one and silently unarm the other. Controls, applied and reverted, all three dep lists at once to prove the new case disarmed neither of the existing ones: - `chunks` deps -> `[userId, entity]`: red on the new case - `[images, byImageId, userId]` -> `[images, userId]`: still red - `byImageId` deps -> `[queries.length]`: still red Also renamed the stub's captured parameter from `imageIds` to `input`, since the first descriptor argument is itself an object with an `imageIds` key and `d.imageIds.imageIds` read like a typo. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * test(home-blocks): pin the pool CHANGING, not merely growing Round 8. Keying the `chunks` memo on `images.length` instead of the id join was green against all four cases, because every one of them either holds the pool still or only grows it. In production `useApplyHiddenPreferences` hands back the PREVIOUS items during a refetch and then the new ones, and a home block's payload size comes from its config rather than its content — so a same-sized, different-membership swap is the ordinary refetch, not an exotic one. Under that mutant the hook keeps asking about the previous ids, the new images render un-hydrated, and the first click deletes. Closed by extending the existing empty-then-populated case with a same-length swap rather than adding a fifth fixture, so it disarms nothing. The fixture rule in the file's doc comment is restated to one that generalises: `chunks` has three deps and each needs something to VARY across a rerender to be visible at all, while the other two memos need something to HOLD STILL. "Stable versus growing" read as an exhaustive pair and is not one — chunk count is its own axis, and `userId` and `entity` are axes no case varies today, since `useCurrentUser` is a constant mock. Dropping either of those from the `chunks` deps is green against every case in this file. Recorded in the comment rather than quietly left out. Controls, applied and reverted, all five at once so a new case cannot silently unarm an older one: - `chunks` deps -> `[images.length, userId, entity]`: red on the extended case - `chunks` deps -> `[userId, entity]`: still red - `[images, byImageId, userId]` -> `[images, userId]`: still red - `byImageId` deps -> `[queries.length]`: still red, two cases - response merge -> `queries[0]?.data ?? {}`: still red Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * test(home-blocks): assert the non-image case asks for nothing, as its name says Round 9 found no green mutant. Its one free item: the non-image case asserted only the hook's OUTPUT, never that no query was issued — which the pure `it.each` next door already covers. Its name has promised the stronger thing for four rounds, and the `asked` capture that makes it possible arrived two rounds after the case was written and the case was never revisited. With the assertion it is the only place pinning that no query is issued outside `chunks`. Control: removing `entity !== 'image'` from the gate reds it with `expected [ Array(1) ] to deeply equal []`. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
Justin Maier
|
0340f692bf |
docs(tests): correct the seam guard's prose, and the false absolutes each fix introduced (#4940)
* docs(tests): correct the seam guard's header and cut what argues rather than informs The header claimed "you cannot call the function without importing the module it lives in". A consumer reached through a re-exporting barrel matches neither half of the detector; what saves the ledger is that the BARREL matches the `from` clause and joins it, one file away from the consumer that gates. Stated, with the limit, because a reader trusting the absolute would stop looking. Cuts the dated "85/85 green" count, the change-log narration of what the ledger used to be, the summary of the assertions below it, and a clause arguing the guard is correct. The mutants are the proof now; the header does not need to make the case. Comment-only: the diff contains no non-comment lines. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * docs(tests): restore the truncated clause on the corpus-loop control The comment above `expectedCorpus` ended mid-sentence, dropping the half that explains why scoping the corpus loop is green: no detector fixture can observe that loop at all, because `verdictFor` seeds `SOURCE` directly and runs past it; and every corpus member already carries the token such a filter would scope by, so the filter excludes nothing. Green there means inert, not caught, and the re-derivation below is what would actually disagree. Comment-only: the diff contains no non-comment lines. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * docs(tests): correct the detector limit and move it onto the code it constrains The header said a consumer reached through a re-exporting barrel matches neither half of the detector. It matches the symbol half: a name-preserving re-export still writes `classifyGatedImageForViewer(`, so the consumer joins the ledger at its own path. Only a barrel hop that also renames escapes both. That mattered more than wording, because the barrel case is the symbol half's ONLY unique contribution - an alias, a namespace import and a re-export from the logic module all carry the `from` clause - so the header handed a future tidier an argument for deleting it. The corrected statement lives on `isCallSite` rather than in the header, where it sits beside the expression it describes instead of drifting from it. The same docblock justified the symbol half as catching a namespace import or a re-export, both of which the import half already catches. The header keeps the rule and drops what restated it: the detection mechanism (stated twice more, on `LOGIC_MODULE_IMPORT` and `isCallSite`), the paragraph arguing a per-file suite could not catch this, and a summary of two sibling suites' assertions - which also over-claimed, since the grid withholds an unrated image's url from its author too when the image is flagged or scan-refused. The corpus-loop comment now states the fact rather than the mutation-testing note: every corpus member's path contains the token, so narrowing the loop by it excludes nothing. Comment-only: the diff contains no non-comment lines. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * docs(tests): state the detector's escape shapes as a property, not a list The previous round's docblock said only a renaming barrel hop escapes both halves. It does not. A renaming dynamic import escapes both with no barrel anywhere: `await import('…logic')` carries no `from` clause for the import half, and a renamed destructure puts a `:` where the symbol half needs `(`. The file still enters the corpus, is scanned, and comes back not-a-call-site. A helper handed the function as a value escapes the same way. That distinction is the safety-relevant part and it is now stated: a renaming barrel hop reddens this suite at the barrel, while a renaming `import()` or a helper reddens nothing at all. Written as a property of what escapes rather than an enumeration of shapes, so finding a fourth shape does not make it false again. Restores the clause saying each file type-checks and each file's own suite passes. It was cut as self-justification, but it is the only statement of why the two per-consumer suites cannot substitute for this one, it lives nowhere else, and being about the nature of cross-file defects rather than about any code, it cannot drift. Comment-only: the diff contains no non-comment lines. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * docs(tests): state both halves as spellings, and stop claiming the per-file suites are blind Two false statements, both introduced by the previous two rounds of this branch. The restored clause said each file type-checks and each file's own suite passes, so this is the class of defect no per-file suite can see. Not true of the tree as it stands: `block-post.service.test.ts` has an `it.each` whose first two rows are `{ ingestion: 'Pending' }` and `{ nsfwLevel: 0 }`, both reaching the gate at `block-post.service.ts:592`, so rewriting that gate as `=== 'hidden'` fails them. It was true when the seam was created and stopped being true when those cases were written. The narrower statement is the one that does not rot: neither file is wrong on its own, so nothing fails until someone writes a per-consumer case for the new state - which is exactly what a third consumer would not have. The escape condition said a file escapes both halves by naming neither the module nor the symbol. A renaming `import()` names the module in full and escapes anyway, because the import half keys on a `from` clause rather than on the module's name - so the condition excluded a case the same paragraph listed two clauses later. Both halves are now stated as what they are, spellings, with the `from` forms left to the regex's own docblock instead of restated fifty lines away. Every false absolute on this file has been a claim about that regex written far from it. Comment-only: the diff contains no non-comment lines. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * docs(tests): cut the detector's consequence claims, and make its referent exact The docblock claimed that when an escape shape lands in an already-ledgered file, the `status !== 'visible'` containment still holds the line. False for half the ledger: `targets` is `EXPECTED_CALL_SITES` minus `MAY_BRANCH_ON_HIDDEN`, pinned by name to `block-post.service.ts` alone, so the grid projection is ledgered with no content assertion against it at all. The claim read as a backstop that does not exist for the one consumer allowed to branch on `=== 'hidden'`. Its companion - that a barrel hop still reddens at the barrel - was unconditional in the same way: a barrel re-exporting via an extension the regex does not list matches neither half itself, so that hop reddens nowhere either. Both are deleted rather than qualified. This is the fifth false statement in this docblock in four rounds, every one of them a consequence claim about a text matcher; a deletion is the only edit here that cannot produce a sixth. What remains is the part that has survived every round: each half pins a spelling, and a file writing neither is not a call site. That sentence defers to `LOGIC_MODULE_IMPORT`'s own docblock for the forms, which makes it load-bearing, and it under-described them - it named the rooted, relative and extensionless spellings while the regex also accepts `.ts`, `.tsx`, `.js` and `.jsx`. A reader following the pointer to check a `.js` specifier was told by implication it was not covered. Now stated exactly, with its closed end. Comment-only: the diff contains no non-comment lines. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
Justin Maier
|
e50d122cb2 |
feat(remix): say at click time which remix modes we can verify (#4939)
* feat(remix): say at click time which remix modes we can verify A prompt-reuse remix feeds no image to the job, so the server resolves no sourceImageIds and the remix gallery correctly refuses the free submission. The submit modal already explains that (RemixGallerySubmitModal renders freeUnavailableReason outside the free/paid block, deliberately). What is missing is earlier: the three remix options look alike at the moment of choosing, so the difference is only discoverable after generating. Mark the two that feed the image itself. On the options that HAVE the property rather than the one that lacks it — reusing a prompt is a legitimate remix and the menu should not read as warning someone off it. It says we can verify, not that the submission will be free: whether free is on offer is five more rungs in freeSubmissionOffer, and a menu that promised it would be overruled at the modal. remixClaimState is split out of remixClaimHolds so the claim's outcome — holds, carrier, why not, and the score where the prompt is the carrier — has one derivation. The predicate is now a wrapper over it. A surface that computed its own similarity would be a second copy of the 0.75 threshold, and the copy that drifts is the one telling someone their remix still counts while the submit is about to drop it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * test(remix): assert the reason, not just the boolean Review round. The comment on remixClaimState said the drift notice renders it. There is no drift notice — it moved to the following PR — so the comment asserted a consumer that does not exist. Corrected to state the instruction that is actually load-bearing: this is the only derivation, reuse it rather than recomputing the threshold. Every existing test asserted through the holds boolean, so carrier, reason and score could take any value and stay green. That is the half the next PR branches on. The pair that matters is drifted against uncarried: both are holds:false and nothing else separates them, so reporting drifted for a cleared prompt box would tell someone they had changed their mind at the moment they cleared it to retype. Asserted field by field rather than with toMatchObject, which truncates to "expected { holds: false, ...(3) } to match object { holds: false, ...(3) }" and never names the value that was wrong. Verified by reverting: swapping the reason on the cleared-prompt branch now fails with "expected 'drifted' to be 'uncarried'". Three comment blocks trimmed to the fact they carry. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * test(remix): cover the three branches a mutation passed through Second review round found the first one incomplete. Three branches had no assertion at the state level, so mutating them printed nothing: the no-remix reason, the branch where the remix seeded no prompt at all — which is a different branch from the form's prompt being cleared, and carries nothing rather than the prompt — and the media branch's reason. The drifted score assertion was vacuous by fixture rather than by shape. The two prompts share no token after cleaning, so every term in the similarity is zero and the score is exactly 0; toBeLessThan(0.75) is then true of any bounded wrong answer, including a constant. Replaced with an ordering over three fixtures whose scores were measured rather than assumed: 0.7601 for two tags changed, 0.2969 for four of eleven left, 0 for disjoint. A constant score now fails with "expected 0.1 to be greater than 0.1". Comments cut, not trimmed. "A server-side check is coming" was the same unfalsifiable forward claim as the one this round already corrected, for a PR that does not exist. The block comment over the tests restated what the test names say and what remix-claim.ts states three lines from the branch it describes, and the note defending the assertion style was the fix round arguing with a reviewer inside the file. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * test(remix): drop the comment defending the assertion, keep the calibration The note above the ordering assertions described the previous version of the test rather than this one, and claimed the null fallback guarded a case that cannot occur at that call site — all three scores come from the branch that always returns a number. The test name already says what the assertions say. The fixture docs keep what a reader cannot recover by eye, that the overlap is calibrated rather than incidental, and lose the measured decimals. Those are pinned by no assertion, so a retuned similarity would leave them wrong with everything still green. The numbers are in the PR body, which is dated by nature. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * test(remix): assert the expired branch carries nothing Third review round, third branch whose mutation printed nothing: the expired claim asserted its reason, its holds and its score, and not its carrier. Verified silent by running it — 15 passed with carrier changed to 'prompt' — and now fails with "expected 'prompt' to be null". Two comments corrected rather than trimmed. The fixture docs said eleven tags where the seed has nine; eleven is its token count, which is what the similarity works on, but the sentence says tags. The state doc claimed expired and uncarried are not caused by the person, which is false for the branch where they cleared the prompt box themselves — the inline comment two lines below says exactly that. The predicate's own doc restated its signature and went. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * test(remix): assert the whole state on every branch Three review rounds each found one more field unasserted on one more branch, a different field each time. Assertions written per branch pin what their author was thinking about, so a fourth round would have sampled the same hole rather than closed it. toEqual over the whole object on every branch, table-driven. A field-level mutation cannot survive it, because there is no field that no assertion mentions. Verified against all five survivors the rounds found, plus a sixth chosen on a branch none of them touched: all six red, the sixth printing carrier prompt against media. score is expect.any(Number) where the prompt carries the claim. Its value is pinned by the ordering beside it, which rules out a constant and a wrong ranking and does not rule out a monotone-but-wrong score. That is the ceiling of an ordering property rather than a gap here. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
briant
|
5ff986be11 | chore(moderator): release moderator-v0.0.69 moderator-v0.0.69 |