mirror of
https://github.com/civitai/civitai.git
synced 2026-09-20 22:08:18 +08:00
main
26331 Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
 Luis Rojas
|
55a75c7656 |
feat(generation): accept raw orchestrator-AIR training epochs as resources
The generator now takes a training epoch's weights blob directly —
urn:air:<ecosystem>:lora:orchestrator:blob@<key> — as an additional LoRA without minting a
draft ModelVersion, behind the mod-fallback `generationAirResources` flag (fliptKey
generation-air-resources; must widen in LOCKSTEP with form-graph-generator, noted on the
definition, since the /generate?air= entry lives in the form-graph lane).
- Schema: the graph resourceSchema (and the form-graph defs twin that serializes the
whatIf/generate payloads) carries optional air/workflowId/name; shared vocabulary in
shared/utils/air.ts (parseRawAirResourceUrn pins orchestrator/lora/blob; rawAirResourceId is
a deterministic negative FNV id, so distinct blobs never share an id or whatif fingerprint).
- Validation (validateRawAirResources): ownership by fetching the named workflow with the
CALLER'S orchestrator token and matching the AIR's blob key against the epochs (typed walk in
training/training-epoch-blobs.ts via getConsumerBlobId); the same 15-day canGenerateWithEpoch
window as the ModelVersion epoch path, status-aware so a canceled/failed run with no
completion date cannot hold its epochs generatable; subscription parity via hasPrivateOrEpoch;
ecosystem must root-match the request. NSFW/POI/canGenerate skips are deliberate (caller's own
training output) and documented. Tokenless paths (App Blocks bridge) fail closed. The derived
{blobKeys, completedAt, stepStatus} view is cached (TRAINING_EPOCH_BLOBS, 5min TTL, errors
never cached) and validated concurrently with getResourceData.
- Handlers unchanged: synthetic negative ids seed the existing StrictAirMap, so every ecosystem
handler resolves raw AIRs through its normal ctx.airs path.
- Form: /generate?air=&workflowId=&name= seeds a hydrated removable LoRA pill with strength,
strips the params, and the embed page exposes generateUrl only when both flags are on.
Reviewed by the five civitai-review lanes across two rounds (all findings resolved; deferred:
subscription-lookup caching before the flag broadens past mods). 30 new tests incl.
mutation-verified ownership/token-threading pins. E2E-verified in the browser end to end:
studio links → pill → payload carries the AIR → whatif prices the job (2 Buzz incl. the
additional-network charge) with no submission.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01DV3Ku4Eu9qTtzd61zt19dZ
|
||
|
Luis E. Rojas Cabrera
|
e87287e40e |
Merge pull request #4836 from civitai/fix/read-collectedcount-for-writer-bookmark-scoring
fix(leaderboard): read collectedCount for writer bookmark scoring |
||
|
Koen
|
76a6f0795a |
feed: feed-service-primary serves the page from the feed service and Postgres, never Meilisearch (#4849)
* feed: hydrate feed-served pages from Postgres behind feed-service-hydrate-db When FEED_SERVICE_PRIMARY and FEED_SERVICE_HYDRATE_DB both match, getAllImagesIndex asks the feed service for the page and hydrates its ids with getAllImages in ids mode, so a served page never queries Meilisearch. Anything the feed cannot serve falls through to the existing path once, not twice. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> * feed: drop the Meilisearch hydration path; feed-service-primary means Postgres end to end One flag: when it matches, the feed picks the page and Postgres supplies the rows; when it does not, Meilisearch serves the page. The second flag and the feedIds hydrate query are gone. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> * feed: review fixes for Postgres hydration An id page that hydrates to nothing falls through instead of ending the feed (getAllImages swallows its statement timeout as an empty page). The hydrate query drops the period, which getAllImages would cut on createdAt. Remix filters are refused by the feed mapper since getAllImages does not apply them. Seam test drives getAllImagesIndex with the flag on and off. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> * feed: hydrate query pins period to AllTime period is a required field of the image query input; pinning it disables the createdAt cut the same way omitting it would. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> --------- Co-authored-by: Claude Fable 5.1 <noreply@anthropic.com> |
||
|
Zachary Lowden
|
efb111ad7d |
chore(event-engine): delete the nested build-and-deploy workflow — it is never scheduled (#4851)
* chore(event-engine): delete the nested build-and-deploy workflow — it is never scheduled `apps/event-engine/.github/workflows/build-and-deploy.yml` came in with the lift-and-shift from the standalone repo. GitHub reads workflows only from the repo-root `.github/workflows/`, so a nested copy is structurally never scheduled — it has never run and cannot run where it sits. Verified against GitHub rather than asserted: the Actions API lists 10 workflows for this repo and ZERO under `apps/`, while all four root workflow paths ARE listed, which is the control proving the query sees registered workflows. Nothing in the tree references the file. It is also superseded: the app is built by the Tekton tag-webhook, and MIGRATION.md item 7 already records that wiring as done. Worth stating because it is the reason not to just leave it: the file triggers on `push: tags: 'v*'`. Inert nested, but this monorepo pushes tags routinely, so anyone who ever relocated it to the root would get an unexpected image build on every release tag — using `secrets.PAT`. Deleting it removes a latent trap rather than only tidying. Deleting one .yml takes the tracked population 14 -> 13, clear of the floor of 10 in src/__tests__/source-nul-bytes.test.ts (the guard that #4850 had to adjust). * chore(event-engine): also delete scripts/release.mjs — the live half of the same class Round 0 of the audit ladder found this PR deleted the INERT member of the standalone repo's release machinery and left the EXECUTABLE one behind. Its requirement was scoped too narrowly: the real content is "retire the standalone repo's release machinery from apps/event-engine", and that has two members. apps/event-engine/scripts/release.mjs — 437 lines, wired to no package.json script, referenced by nothing in the tree, and executable as-is. Verified what it does rather than assuming: RELEASE_BRANCH = 'release' (:25), checkout release (:229), rebase onto main (:257), bump this app's version, tag a bare `v<version>` (:112), push the tag (:129) and push release (:275). In this repo `release` is the production deploy branch — root package.json's `release:base` performs exactly that checkout/rebase/push dance — so one run is an unreviewed production deploy of whatever main holds, plus a tag in the ROOT release namespace. This app's real release path is `pnpm release:event-engine:*` -> scripts/release-app.mjs, which tags `event-engine-v*`. That is a strictly larger blast radius than the workflow this PR already deletes: the workflow needs someone to RELOCATE it before it can fire; this one runs today. Also from round 0: - MIGRATION.md still described `.github/` as "kept as legacy reference only" while this PR removes the directory entirely (it held exactly one tracked file). It now records the deletion in the same shape #4850 used for `k8s/`, keeping the structural-inertness reasoning in the tree rather than only in a commit message, and naming why release.mjs was the dangerous half. - The `.yml` floor comment is refreshed 14 -> 13 to match the walk. The file's own docstring says those comments are the measured counts, and #4850 round 2 established that a stale one in this table is not cosmetic: it is what led that round to cite a wrong precedent. 13 is still clear of the floor of 10. * docs(event-engine): correct three claims this PR's own prose got wrong Round 1 returned no blockers and no should-fixes; all five of the PR's claims hold. Its three findings are all defects in prose the previous round wrote, which is the documented failure mode of a fix round, so they are fixed here rather than filed. 1. The hazard sentence overstated the blast radius by one condition. `:264` ran `git add package.json` RELATIVE TO THE PROCESS CWD while the version write resolved the app's own manifest, so invoked from the repo root it staged the untouched root manifest, the commit exited non-zero, and it never reached the branch switch. The push required `cwd = apps/event-engine`. The blob was also 100644, so `./scripts/release.mjs` would not run. The deletion was right either way; the claim is now stated with its precondition, because a later reader re-deriving it would have found it wrong and had no way to tell which half. 2. `docs/reference/release-script-example.js` was justified as retained because it is "not wired to anything" — the exact criterion the bullet four lines above declares insufficient. It is a 446-line near-copy of the file just deleted, sharing its branch constants and its createGitTag/pushRelease path. What actually stops it is an ENOENT in the version bump (its helpers resolve `<dir>/../<name>/package.json` against a `docs/` parent that holds none), thrown after an ensureOnMain checkout but before the release branch is touched. Verified: apps/event-engine/docs/package.json is absent, with the app's own package.json as the positive control. Recorded, and the keep-or-delete question is marked OPEN rather than left reading as settled — otherwise the next reader concludes the runnable-release-script class was cleared from this app while a runnable copy sits in the same tree. 3. "listing all four root paths" was the TREE's count, not the API's. The registry retains rows for deleted workflow files, so it returns more root entries than the tree has and a re-verifier cannot reproduce the four. The load-bearing half — ZERO entries under `apps/` — is unaffected and is what the text now quotes. * docs(event-engine): the correcting sentence was itself wrong about the call order Round 2 found the sentence the previous round added to fix an overstatement was inaccurate in the other direction. It said the repo-root invocation "never reached the branch switch". Verified against the deleted blob's own main(): ensureOnMain() at :351, updatePackageVersion() at :359, commitVersionBump() at :362 (the call that fails from the repo root), setupReleaseBranch() at :365. So it DID check out main and DID write a bumped version into the app's manifest before failing — leaving a dirty tree — and what it never reached was the switch to `release`. Worse than a wrong detail: the same commit described the near-identical twin correctly twenty lines below ("after an ensureOnMain() checkout but before the release branch is touched"), so one commit described identical code two inconsistent ways. That reads as the retained copy being more invasive than the deleted one in a respect where they behave the same — and the paragraph's own stated purpose was that a later reader should not have to re-derive and disbelieve it. This is the third consecutive round whose only finding was in prose the previous round wrote while fixing the round before it. Recorded here because the pattern is the point: the code in this PR has been correct since the first commit. * docs(event-engine): sweep the same shape at every site, not just where it was reported The prose-ladder stop criterion requires the recurring SHAPE be swept everywhere it occurs before stopping, so this closes the three remaining instances the last round listed but did not file. All three are the same defect: a stated cause narrower than the effect it explains. 1. "the registry retains rows for workflow files that have since been deleted" explained one of two extra rows. Measured: the Actions registry returns 6 root rows against 4 tracked root workflow files; `pr-bot.yml` was deleted, but `retool-sanitiser-guard.yml` never existed on the default branch at all. The cause is now stated as what it is — rows that do not correspond to the default branch's tree, from either source — with the measured pair beside it. 2. "throws ENOENT inside the version bump" named the wrong call. The first failing resolve is in `getCurrentVersion()` — the version READ, before any write — and the `<dir>/../<name>/package.json` notation described a different helper. Both corrected; the conclusion (unreachable tag, unreachable push) is unchanged, which is why it was reported as an imprecision rather than a defect. 3. "the same criterion four lines above" did not land on the criterion it meant, and a positional cross-reference rots on the next edit regardless. It now names the bullet. Stopping here, and naming why rather than leaving it implicit: no round of this ladder has produced a 🔴 or a 🟡; the blast radius of everything remaining is "the document contains a false sentence"; and with this commit the shape has been swept at every site rather than only where it was reported. The last three rounds found defects exclusively in prose the previous round wrote while fixing the round before it, which is the non-terminating shape that criterion exists for. The PR's code has been correct since its first commit. |
||
|
Zachary Lowden
|
0485098106 |
chore(event-engine): delete the legacy k8s/ manifests — they deploy nothing (#4850)
* chore(event-engine): delete the legacy k8s/ manifests — they deploy nothing
`apps/event-engine/k8s/` was carried into the monorepo as part of the
lift-and-shift and MIGRATION.md kept it "as legacy reference only", noting it
should be relocated to the ops repo. That relocation has since happened: the
Kafka/Debezium manifests, the Kafka UI and the app's own Deployment all live in
the ops repo now and are what actually deploys. These nine files deploy nothing.
Verified before deleting rather than assumed:
- nothing outside MIGRATION.md referenced the directory (0 files repo-wide);
- no Dockerfile, package.json or build config reads from it — `port-forward-k8s.ts`
is a script under `scripts/`, unrelated to these manifests;
- the live counterparts exist in the ops repo, including the Kafka cluster,
Kafka Connect, the connector and the Kafka UI.
MIGRATION.md is updated in the same commit so it does not end up citing files
that no longer exist: the three places that pointed at `k8s/` now say what
happened, and the DevOps item that said to port from `k8s/09-metric-watcher-app.yml`
is struck through and marked done. The historical record is kept rather than
rewritten.
Left alone deliberately: the runtime identifiers (consumer group, `mew_` metric
prefix, `app` label) stay as they are — that is what let the cutover resume from
existing offsets, and it is still true.
* fix(event-engine): correct the docker-compose claim, and delete the stale CI/CD task-prompt
Round 0 of the audit ladder caught two defects in the previous commit, both in
prose I had just written.
1. MIGRATION.md called `docker-compose.yml` inert legacy CI. It is neither. It is
the live local-dev Kafka/Debezium harness, driven by README.md,
scripts/produce-comic-event.ts and scripts/setup-digitalocean.ts. The previous
commit REWROTE that line and narrowed the parenthetical to "(the old CI)",
re-asserting the error rather than inheriting it — a reader applying the PR's
own rule (inert legacy gets deleted) would have removed the documented local
setup. The two files are now stated separately, with the reason `.github/` is
genuinely inert: GitHub reads workflows only from the repo root.
2. `docs/plans/ci-cd.md` is deleted. It is a task-prompt for the CI/CD work that
MIGRATION.md item 7 now marks DONE, and it referenced the deleted manifest as
`k8s\09-metric-watcher-app.yml` — a BACKSLASH separator, which is why the
previous commit's `k8s/` grep reported zero external references and why the PR
body's "0 files repo-wide" claim was wrong by exactly one. Deleting the file
is what makes that claim true rather than patching a stale document.
Re-verified after the change with BOTH separators, plus a positive control
proving the backslash grep matches at the pre-deletion tree: 0 dangling
references either way.
* fix(tests): lower the .yml population floor — deleting 7 manifests crossed it
Round 1 of the audit ladder found this PR turns CI red, and confirmed it in the
real tier rather than inferring it: `Unit tests (4)` on head
|
||
|
Zachary Lowden
|
028af195da |
feat(moderator): tri-state server-side sort on the feedback queue, with a compound keyset (#4820)
* feat(moderator): tri-state server-side sort on the feedback queue, with a compound keyset Clicking a column header cycles that column ascending → descending → none, and the ordering is applied in SQL by `getFeedbackList` — never in the browser. Why server-side matters here: the list is keyset-paged at `FEEDBACK_PAGE_SIZE = 50`. A client-side `.sort()` orders the page that happens to be loaded and presents it as an ordering of the queue — right on every screen and wrong for any queue past its first page. Against 26 live rows it is also indistinguishable from a correct implementation, so it would have shipped clean. The keyset is compound, `(<sortColumn>, f.id)`, because every sortable column except the id repeats: `area`, `status` and a handler's username all tie freely, and a keyset on a non-unique key either repeats or skips the rows sharing a boundary value. The id half stays as the unique tie-break, so `?cursor=` still means what it meant before and `?cursorValue=` carries the boundary row's value in the sorted column. No sort key is a timestamp, deliberately. The value half round-trips through the URL, and `createdAt`/`handledAt` are `timestamp WITHOUT time zone` whose driver handling is asymmetric between production and this app's PGlite test tier — a boundary shifted by the local offset skips rows and the test tier cannot see it. So Age sorts on `f.id` (which IS arrival order on this insert-only table, the argument the default ordering already makes) and Handled sorts on the HANDLER, which is also what that cell renders. The 📎 column is deliberately not sortable: the count is derived from JSONB by `feedbackAttachmentCount`, so a SQL ordering means an unindexed expression over `context` AND a second implementation of that arithmetic that nothing makes agree with the first. Sort state lives in the URL, reached by real links. Every successful write calls `invalidateAll()`, so component state resets under the operator; and `replaceState` does not update `page.url` in `@sveltejs/kit@2.66.0`, so shallow routing here would be inert while looking live. Tests: the ordering and the keyset are executed against a real Postgres over a MANUFACTURED page boundary — 60 rows with heavy ties and real null blocks on every sortable column, paged all the way through at the real page size and at 7, asserting the union of the pages is the whole set exactly once, in an order computed independently from the fixture. Production never crosses a boundary, so nothing else could have seen a wrong one. * fix(moderator): resolve the feedback-sort review findings `svelte-review` (correctness, idiom, abstraction) plus an adversarial audit over the diff. Everything below is a finding that was reproduced before it was fixed. CORRECTNESS * The next-page link kept the PREVIOUS page's `?cursorValue=` when the new boundary's value was null, because it only ever SET the param. That is the ordinary transition into the trailing null block — `handled` is null on every untriaged row — and the server then read a non-null boundary, whose predicate admits the whole null block with no id bound. The same page came back, its own boundary row included, and `Next →` never advanced. The builder now always writes that half, and it moved into `$lib/feedback-sort.ts` so a test can reach it at all. * Ascending Age showed the OLDEST report first. The cell renders a duration, which grows as the id shrinks, so `ORDER BY f.id ASC` put `12d` above `10m` under a header reading `Age ↑` and an `aria-sort="ascending"` a screen reader has no way to check. The map now carries an `invert` flag and both the ORDER BY and the keyset operator read one function, so they cannot disagree. * `Number` is not a parser: `''` and `' '` became the boundary 0, `'0x10'` 16, `'1e3'` 1000. The shape is matched before `Number()` now. The test that named `''` had been passing for the wrong reason — every seeded `bugId` is ≥ 1, so a boundary of 0 on `issue asc` admits every row and looks like page one; it runs both directions now, and the two coercion classes are separate cases so a mutant cannot die on its neighbour's input. * A REJECTED `?cursorValue=` was indistinguishable from an ABSENT one after `.catch(undefined)`, and those are opposite instructions: absent means "the boundary's value is null", a real position. It now drops the whole cursor. * The queue header said "newest first" unconditionally over twelve sorted states. IDIOM / ABSTRACTION * The sort links carried none of the three `data-sveltekit-*` modifiers the tab strip on the same page documents, so every sort click scrolled the operator away from the row they had open and dropped keyboard focus to the top. * The sortable header moved to `FeedbackSortHeader.svelte`; `+page.svelte` is back under the standard's threshold. `colspan` reads `COLUMNS.length`. * `clearFeedbackPaging` was a second door onto a rule seven files already reach for through `clearPaging` — the delete moved into `$lib/paging` beside `IMAGE_PAGE_PARAM`, and the wrapper is gone. * The `↑`/`↓` marker is `aria-hidden`; `aria-sort` already announces it. * Comments trimmed to the standard's bar (breakage guards only), and the claims that were wider than the code they described were corrected — `satisfies` does not check that `ref` and `field` name the same column, and the fixture's "no cycle length divides PAGE" was false for the pair it mattered for. Two new tripwires, both mutation-checked: every `FEEDBACK_SORT_COLUMNS` member must have a clickable header (and no header a column the server refuses), and both link-driven controls must keep their navigation modifiers. * fix(moderator): close the delta-audit findings on the feedback-sort fix round A re-audit scoped to commit 2 — the fix round itself, since a fix made in response to a review is a code change like any other. All six fix areas came back clean on substance; what follows is what it found around them. One real guard defect: * The `?open=` scan could not see the spelling a new `$lib` writer would use. It matched `open:` only, while the choke point writes `{ [FEEDBACK_OPEN_PARAM]: id }` — harmless while the scan was `.svelte`-only, and a false NEGATIVE the moment commit 2 widened it to `$lib/feedback*.ts`, because copying that line out of `feedback-tabs.ts` is exactly how a second `.ts` writer gets added. Both spellings are matched now and the choke point is excluded by FILENAME rather than by being unmatchable. One seam nothing covered: * Every test in this arc was scoped to one surface — the href builder in isolation, the keyset in isolation (threading `cursorValue` as a variable and never as a URL), the loader's arguments in isolation. All three were green over the stale-`cursorValue` bug commit 2 fixed, because none of them handed one surface's output to the next. Two cases now walk builder → loader → service args, including the null-boundary transition that was broken. Three comment claims that were wider or wronger than the code: * The fixture's group-size paragraph replaced one false quantitative claim with two — "every group is 12–20 rows wide, far wider than either page size, inside a group in every ordering" is wrong for `issue` (26/25/9), wrong against a page size of 50, and wrong for `age`, whose groups are singletons. It now carries the measured table and states plainly that `age` is the exception and why that is benign (its sort ref IS the tie-break column, so the tie arm is dead rather than merely unvisited). * "seven files reach for `clearPaging`" is ten. It was doing argumentative work, so a reader re-deriving it got a different number. * The colspan pin's title claims it derives every colspan; it pins a spelling. `colspan="9"` walks past it, and a correctly written third colspan reddens it. Said so, rather than leaving a guard that reads wider than it checks. And two placement/clarity fixes: the column-ledger docstring was orphaned above the colspan test, describing a test two `it`s away; and the extraction's `inline-flex` → `flex w-full` change went unremarked — it buys a full-cell hit target and overrides the cell's `text-align`, so the first right-aligned sortable column would silently left-align. Left as-is, deliberately: `age asc` is now byte-for-byte the default ordering, so an operator's first Age click changes only the arrow. That is correct — the default view IS newest-first — but it makes the `age`/`asc` arm of the paging loops unable to distinguish "applied" from "ignored", which is now stated where it is read. The `desc` arm discriminates. * fix(moderator): the service refuses a bad sort by throwing, and Age loses its no-op state Two operator decisions from the round-0 audit. The compound keyset is unchanged and deliberately so. 1. getFeedbackList THROWS on a sort state it does not have, where it used to degrade to the default ordering. A silent degradation returns a page of real rows in an ordering the caller did not ask for, with no signal — the same silently-wrong shape this PR's own headline argument condemns on the client, where a .sort() over one loaded page presents itself as an ordering of the whole queue. At the service layer the input is an argument, not a URL, so an unreachable state is a programming error. The URL layer is UNCHANGED and still degrades: a hand-typed ?sort=garbage must not 500 a queue nobody can then open. The two layers are now deliberately different and the code says so at both ends. What holds the asymmetry safe is a new relationship guard — everything parseFeedbackSort can emit, isFeedbackSortState accepts — since nothing in the type system links them. This also closes the M7 complaint properly rather than papering over it. The advertised fallback was unpinnable: the mutant that removed the guard died on a TypeError out of the map read before any fallback assertion could be reached, so the behaviour had no test that observed it. Deleting the advertisement removes the unprovable claim; the refusal is now asserted by ERROR CLASS, which a TypeError fails. The guard's clause order is load-bearing and is now pinned: the reachability clause indexes an object literal with the untrusted column, and FEEDBACK_SORT_DIRECTIONS['__proto__'] is Object.prototype while ['toString'] is a function — neither has .includes. The allowlist check short-circuiting before it is the only reason the lookup is total. 2. Age collapses to a two-state toggle: default <-> oldest-first. Measured against production, age's ascending state was byte-identical to the default ordering — "youngest first" IS id DESC — so the first column in the table cycled click, arrow appears, nothing moves; click, reverses; click, arrow vanishes, nothing moves. Oldest-first is the one view the default cannot express, so it is the one state this column keeps. The other five keep the tri-state. Falling out of that, and the reason the collapse was worth making: - the service's `invert` flag is GONE, and with it sqlAscending. The direction token is now the SQL direction on every column without exception. - the one display flip lives in READS_INVERTED, next to the arrow and the aria-sort it exists for. It reaches a glyph and an ARIA token and nothing else, where the flag it replaces was consulted by the ORDER BY and by the keyset's comparison operator — so applying it to one and not the other produced an ordering the cursor walks backwards through. That hazard pair no longer exists to be tested. - the SQL direction is read ONCE into a local shared by both readers, so the two cannot be changed independently. - the age/no-op arm of the paging loops is gone, so no arm of that suite is left unable to distinguish "the sort was applied" from "the sort was ignored". A one-state column writes no ?dir= at all: the token would say `asc` under a down arrow and an aria-sort of "descending", and a direction param that contradicts the screen is the defect this column already shipped once. Suite: 928 -> 937 passed, 40 skipped unchanged, 0 failures. svelte-check 0/0, vite build green, prettier and eslint clean, each with a live negative control. 13 mutants run against the changed guards: 12 killed on their own assertion; 1 survives by design and is documented as a type narrowing whose runtime check is subsumed. |
||
|
Zachary Lowden
|
036e1b6beb |
refactor(bot-account-detection): delete the dead comment-text fingerprint source, and fix this area's invisible test-fixture type errors (#4848)
* refactor(bot-account-detection): delete the unused comment-text fingerprint source
`content-templating` had two fingerprint sources behind one heuristic id: uploaded
filenames and posted comment text. The text source has never scored a single account
in any run it shipped in, and the cause is structural rather than transient — new
accounts on this site do not comment, so the source has no input to find a signal in
and waiting does not change that. Nothing else in the tree reads the property.
Deleted:
- the prose normaliser and its link/digit masking
- `MIN_FINGERPRINT_CHARS` / `MIN_FINGERPRINT_TOKENS` and `contentFingerprint`
- `MAX_CONTENT_CHARS`, `MAX_CONTENT_SAMPLES`, `ContentSampleRow`, `contentSampleArgs`
- `TEXT_FINGERPRINT_PREFIX`
- `EvidenceReader.listContentSamples` and the `comment`/`commentV2` members of
`EvidenceDb`, i.e. two per-run `findMany` statements on the comment tables
- the content walk in `collectCohortSignals` with its budget, its availability flag
(`sources.contentSamples`), its failure flag and its two sample counters
- the run counters `heuristic:content-templating:fired_text`,
`evidence_distinct_content_fingerprints`, `evidence_content_samples`,
`evidence_content_budget`, `evidence_content_budget_exhausted`,
`evidence_members_sampled_for_content`, `evidence_content_read_failed`
- the two content disclosure sentences in the report summary
Kept deliberately: the heuristic's registry entry, its `content-templating` id, its
weight of 1, and `heuristic:content-templating:fired_filename`. The text source was
one of two sources INSIDE one heuristic, never a registry entry, so
`BOT_ACCOUNT_HEURISTICS.length` stays at four and the blend denominator,
`MIN_REPORTED_CONFIDENCE`, `LONE_SIGNAL_CUT` and `SOLE_SIGNAL_DOMINANCE` are untouched.
`FILENAME_FINGERPRINT_PREFIX` and `unprefixFingerprint` also stay: `run.ts` counts
`evidence_distinct_filename_fingerprints` by that prefix, so dropping it would redefine
an existing series rather than remove one.
`listContentSamples` had a known open defect — a global `take` over
`WHERE userId IN (…) ORDER BY id DESC`, under which one prolific commenter's newest
rows evict every other account in the chunk. Its only consumer was this source, so the
defect is closed by deletion rather than by a fix.
Counters that disappear do so as ABSENT KEYS, not as zeros. A key reporting 0 would
assert the source was read and found nothing; a key that stops appearing says it is no
longer read. `evidence_source_read_failures` now sums three terms rather than four, so
a run's total can only fall as a result of this change, never rise.
Measured before/after over six synthetic cohorts driven end to end through
`runBotAccountDetection`: where no comment fingerprint clusters — the production shape —
every counter, every finding and the whole report summary are byte-identical. Where one
does cluster, an account's score falls exactly when its largest text cluster was
strictly larger than its largest shared filename.
The database-operation ledger in `no-write-surface.test.ts` goes from eight reads to six.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* fix(bot-account-detection): repair the type errors in evidence.test.ts
`tsconfig.json` excludes `src/**/__tests__/**` from the program and vitest does not
typecheck, so a type error in a test file in this repo is seen by no gate at all. This
file carried 26 of them on a fully green suite. Type-correctness only — no test's
assertions change, and the suite is 396 passing before and after.
Four classes, all in fixtures:
- TS2741 x15 — the shared `sources` fixture in the `buildCohortSignals` block omits
`readFailures`. Pre-existing: introduced when an earlier PR widened
`CohortSignals['sources']` and did not update the fixture. Fixed by adding the
member AND annotating the fixture `CohortSignals['sources']`, so the next field
added to that type is one error on the fixture rather than fifteen on its call
sites — which is the shape that let this go unnoticed.
- TS2322 x6 — the shared `image.findMany` double returns `{ userId, name }`, which is
not assignable to `EvidenceDb['image']['findMany']`'s staged overload
(`StagedImageRow` requires `createdAt`). It cannot satisfy both overloads while
returning one row shape, so the assignment is cast at a single named seam rather
than at each call site. The cases that drive the staged read through this double
assert its CALL ARGUMENTS and never its rows; the cases that need real staged rows
already build their own double. Not fixed by adding `createdAt` to the returned row,
which would have changed what the filename pass-through case asserts.
- TS2339 x4 — `Parameters<typeof createEvidenceReader>[0]['db']`. The parameter has a
default, so `Parameters<...>[0]` is `{…} | undefined` and indexing it does not
compile. Wrapped in `NonNullable` and named once as `ReaderDeps`.
- TS2493 x1 — the double is declared with no parameters, so its recorded calls are
typed `[]` and `([a]) => …` destructures an empty tuple. The parameter is declared
on `vi.fn`'s type argument rather than on the implementation, so the double still
ignores what it is handed without leaving an unused binding.
Verified with `tsc -p` against a config that re-includes only this module's test files:
26 errors before, 0 after. Instrument validated in both directions — reintroducing the
`readFailures` omission reports 1 x TS2741 and reintroducing the empty-tuple parameter
reports 1 x TS2493 at its own line, and the same run compiles the rest of `src` clean,
so the 0 is program-wide rather than scoped to one file.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* docs(bot-account-detection): state the fired_filename identity, and retract an unreachable worked example
Two comment defects of one class — a note asserting coverage the code no
longer has — found auditing the comment-text source deletion.
D3. `heuristic:content-templating:fired_filename` is now EQUAL BY
CONSTRUCTION to `heuristic:content-templating:fired`, not merely equal so
far. Both count the same population; one filters on `file:`, and with the
comment source deleted that is the only namespace the index holds, so the
filter rejects nothing. The old wording ("currently equal; they need not
stay so") reads as a caveat, and an operator charting the pair sees two
identical lines and takes the agreement as evidence the decomposition is
being exercised — it would agree just as perfectly if the decomposition
were broken.
The counter is KEPT rather than dropped. Its present measurement value is
nil and the note says so; what removing it would cost is this module's own
convention for a vanishing key, which the run says twice and a test
asserts: a key that stops appearing means THE SOURCE IS NO LONGER READ.
`fired_text` stopping says something true. `fired_filename` stopping would
say the filename source went dark on a run where it is the only source
there is — a false statement on the one surface that renders these, which
lists every counter key on the run page. Nothing keys on the name; no
alert, dashboard or query reads it, so the visible change either way is a
row on that page.
The trigger that ends the identity is named — the first run whose index
carries a second namespace — and made mechanical rather than left to rot:
`evidence.test.ts` now fails if a second `*_FINGERPRINT_PREFIX` is declared
in `fingerprint-keys.ts`, and `run.test.ts` pins the equality itself behind
the existing non-zero assertion as its positive control. Both are labelled
invariant tripwires, not regression coverage.
D4. The `sole_signal` docstring justified itself with a worked example that
this change makes unreachable: a generation-parameter paste fingerprinting
identically to the same line with different numbers. That was a property of
the deleted source twice over — the strings were comments, and what made
two pastes collide was the prose normaliser's digit masking, which
`normalizeFilename` deliberately does not apply and records a measurement
for. The example is retracted in place, rather than swapped out quietly, so
nobody tunes against a population that cannot appear.
The counter stays justified by the false positive that IS reachable: a
generic filename several unrelated new accounts happen to upload under,
defended by the three-distinct-member cluster floor plus an all-sub-24h
cohort. The replacement worked example is executed rather than asserted —
the same fixture now drives the trace-exclusion test, at the shipped
registry size of four rather than three, and its arithmetic (leader 0.5,
trace 0.1111, blend 0.1528, sole at four and not at five) is what that test
measures.
Comment and fixture changes only; no production behaviour moves.
* docs(bot-account-detection): correct two claims in the notes just added
Both were wrong in the small way a doc claim usually is — a pointer that no
longer points where it says, and a scope that overstates.
- `run.ts` said the vanishing-key convention was stated "four lines up".
The paragraph it meant is 26 lines up, because the note I inserted sits
between them. Name the paragraph instead of a distance, which cannot go
stale when the next author inserts something.
- `similarity.ts` said `contentTemplatingSourceScore` is "the one caller"
that passes a prefix to `largestContentCluster`. The tests pass one too —
`heuristics.test.ts` hand-builds a second namespace for exactly that
purpose and labels those cases invariant guards. The claim I meant is
about PRODUCTION callers; it now says so, and points at the tests that
are the exception rather than leaving a reader to find them and conclude
the sentence is false.
---------
Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
||
|
ZacxDev
|
3bac90199d | 5.1.99 v5.1.99 | ||
|
Zachary Lowden
|
d2218f54da |
fix(images): stop /api/v1/images coercing an all-digit username to a number (#4839)
Server half of civitai/cli#513 / #4768. Pairs with the submodule fix civitai/event-engine-common#13 (5da5cc6467), which this pins. The coercion was never in Meilisearch, which is what #4768's body still says. The index document's user.username never reaches the response: getImagesFromFeedSearch -> ImagesFeed.populatedQuery builds `user` from the Postgres-backed userData Redis cache and spreads it AFTER the doc, overriding it. A Redis hash stores only strings, so createCache serialised each field on write and guessed the type back on read -- isNaN(Number(v)) ? v : Number(v). Number('0222') is 222. Discriminating observation, cache-busted with cf-cache-status MISS on every row: ?username=0222&limit=11 returned "0222" (Redis miss, raw Postgres row) and limit=12..16 returned 222 (Redis hit, decoded). A Meilisearch document cannot change between two requests seconds apart. Ships both halves: the field-type declarations in the in-repo fork of the cache (apps/event-engine/src/common/caches/, which writes the SAME Redis keys), the submodule pin, and a belt-labelled String() cast at the emit site. 447 of the lines are tests. Consumer audit posted on #13: the only LIVE defect is the username one. The 'false'-as-truthy-string and array-as-raw-text cases are real in the decoder but latent -- nothing reads modelData.nsfw from the cache (all three .nsfw reads come from a direct pg.query) and nothing calls Array.isArray on the cached arrays. NOT verified: nothing was exercised against a real Redis or a deploy, and the consumer audit covers static field reads only -- services/cache.ts uses a namespace import, so a dynamic access would not have appeared. /api/v1/blocks/images shares runImageSearch but returns 401 Block token required, so it is covered by code identity, not measurement. |
||
|
Zachary Lowden
|
d0d01fbc60 |
refactor(apps): drop the frozen sub-nav predicate oracle (#4841)
The test carried ORIGIN_MAIN_PREDICATES, a hand transcription of the six
`visible` expressions from AppsSubNav.tsx's SUB_NAV_LINKS as they stood at
merge-base
|
||
|
Zachary Lowden
|
95562f7755 |
feat(new-order): file KoN abuse detection on the moderator abuse board (#4823)
* feat(new-order): file KoN abuse detection on the moderator abuse board
The daily `new-order-abuse-detection` scan found its suspects, posted a
Discord embed capped at 10 of them, and persisted nothing. A webhook
message is not reviewable, not searchable, not attributable to an account,
and cannot record that a detection was deliberately left alone — which is
most of what this scan produces.
It now files every suspect on the moderator app's abuse-detection board,
the surface built for exactly this, alongside the Axiom logging it already
did. Two detectors already post there; this follows their structure.
Where it differs from both of them, and the one subtle part: they hardcode
`actioned: false` because neither holds a write client. This scan DOES act
— when `autoSmiteAbusers` is on it smites the strict-signal subset — so
`actioned` is a per-finding fact. The wire contract's superRefine rejects
both halves of the wrong pairing, and one rejection loses the entire batch
rather than the offending row, so the findings are built AFTER the smite
loop, from the set of accounts `smitePlayer` actually succeeded on. A
target whose write threw stays an open case.
- add `src/server/services/new-order-abuse-detection/report.ts`: reason
rendering (all six query columns go in the text — the contract has no
structured-metrics field), a confidence band that is the queue's sort
order rather than a probability, the actioned/action pairing minted as
two whole branches, and the report builder
- replace the Discord block with `moderatorApp.abuseReport`; keep the
Axiom logging. `DISCORD_WEBHOOK_MOD_ALERTS` keeps its other consumers
and stays in the env schema
- record real `startedAt`/`finishedAt`; the job recorded neither, and the
contract refuses a transposed pair
- single-source the 24h lookback as `ABUSE_SCAN_WINDOW_HOURS`, used by
both the query and the reason text
- add `lockExpiration`; the job had none, and a duplicate concurrent run
appends a second near-identical run rather than replacing the first
- no threshold goes in `counters`. The scan's tunables are held in Redis
so they are not readable from the public source tree, and the board has
a wider audience than this job's logs. The counters are outcomes only
Tests: the report mapping, a regression guard on the actioned/action pair
in both directions with the contract's own parse as the oracle, and the
job→board seam including a suspect whose smite threw.
* style: apply Prettier to the two files the lint gate flagged
* fix(new-order): correct three overclaimed justifications, and rethrow a report failure that protects nothing
A requirements audit refuted the stated grounds for three of this PR's
decisions. The shipped mapping was not at fault; the prose defending it was,
and one of the decisions was wrong on its merits.
lockExpiration — the premise was false. createJob already defaults every job
to a 5-minute lock (job.ts), on main and on this branch, so this is a widening
from 5 to 10 and not an introduction. The value matches the sibling detector
reaction-withdrawal-detection and nothing else supports it: no run of this scan
has been measured exceeding 5 minutes. The comment now says that, including
that the size is borrowed rather than derived. The value is unchanged.
Threshold withholding — the claim was false as written and had propagated to
five sites. "The confidence score cannot be inverted to recover a threshold" is
true of confidenceFor in isolation and false of the row it ships on: the reason
text publishes the values the selection rule compares, and the query selects on
HAVING totalRatings >= minTotalRatings, so the smallest totalRatings visible on
the board converges on that tunable from above within a few runs; the smallest
dominant share among auto-smited rows converges the same way. The sibling claim
that Redis keeps the values off the public source tree is also false — this repo
is public and a pre-existing checked-in test of the smite path carries live
values. The withholding is KEPT, on proportionality: nothing a moderator does
with this board needs a tunable, and the surface this replaced carried the same
observed values. Every site now says that instead, including the two
pre-existing ones that were the origin of the claim.
Report failure — the swallow blinded the detector's only dark-signal, and the
precedents do not support it. bot-account-detection logs under a stable key and
RETHROWS; reaction-withdrawal-detection does not catch at all. With the token
rotated, every post would 401, the job would return success, and the job error
counter would stay flat with the detector dark and nothing anywhere to say so.
Scan-originated smites are rare, so the unconditional catch paid that blindness
on nearly every run to cover a case that nearly never occurs. It now rethrows
when the run smited nobody and swallows only when smites are already written.
The free-text log sentence is replaced by a stable, alertable key in the
precedents' style.
Whether a failed run is retried at all could NOT be established — the scheduler
lives outside this repo and the run-jobs route has no retry — so the remaining
swallow is documented as a precaution against an unconfirmed retry, not a
response to a measured one.
Axiom payload cut to an aggregate. The stated ground, that both precedents log
alongside the board post, holds only for aggregates: both log run-level counts
and neither logs per-account detail. The board now renders those columns with
attribution and reviewed-state, so the array duplicated the sensitive half of
the payload into a surface with different retention and no review workflow.
Tests: both halves of the rethrow split are pinned and each was watched to fail
for its own reason. Removing the guard fails only the propagate case
("promise resolved undefined instead of rejecting"); making the rethrow
unconditional fails only the swallow case ("promise rejected Error: 400 bad
request instead of resolving"). The aggregate cut is pinned as the whole details
key set, which was watched to reject a re-added per-account array under a
different key name.
Local: pnpm typecheck 0 errors; 82 tests passed across the two new-order suites.
* fix(new-order): key board membership to the smite ROW, not to the call returning
Round-1 audit fixes. The one that matters is the first.
A smite that was APPLIED but whose call threw afterwards was filed on the
abuse board as an open case. `smitePlayer` commits the smite row first and
then does a tail of non-durable work — an active-smite count, a possible
career reset, a Redis counter increment, a signal, a notification. The
counter increment is the sharp one: it calls `getCount`, which re-throws a
non-connection ClickHouse error, then writes to `sysRedis` with no guard at
all, unlike the fail-open `setCacheValue` beside it. Any throw past the
create leaves the penalty live in Postgres while the job's catch skipped
`smitedUserIds.add`. The finding then read `actioned: false` with "No action
was taken by this scan — filed for a moderator to review." about an account
carrying a live smite — the exact inverse of the invariant `report.ts`
declares load-bearing, and an invitation to apply a second penalty.
`smitePlayer` now takes an optional `onSmiteCreated` hook, fired
synchronously the instant the row is committed and before any of the tail.
A return value cannot carry this, because the case that matters is the one
where there is no return. The hook is additive: the other three call sites
pass nothing and are unchanged.
So `actioned: true` now means exactly "a smite row was written for this
account by this run". It does NOT claim the player was notified, that their
counter moved, or that a third-strike career reset completed — each of those
is in the tail and can fail independently of the penalty. Both `report.ts`
comments that described the old, stronger semantic are corrected to say
that, rather than left claiming more than the code delivers.
- `smitePlayer`: add `onSmiteCreated`, called once right after the
`newOrderSmite.create`
- the scan's smite loop: record membership from the hook
- `report.ts`: `toFinding` and `BuildReportArgs.smitedUserIds` now state the
durable-write semantic, and name both directions the flag can be wrong in
- the duplicate-run comment on `lockExpiration` said the worst case of a
concurrent run is "a moderator sees the same cohort twice". That is the
cosmetic half. `smitePlayer` is not idempotent — every call inserts
another smite row, and the row that carries an account to the third-strike
rule chains into `resetPlayer`, wiping that player's career with a
notification. Comment only; the hazard is unchanged
- the per-player smite failure logged an interpolated sentence as the Axiom
`name`, three lines above the comment articulating why that is wrong.
Stable key `new-order-abuse-detection:auto-smite-failed`, id in the
details, plus whether the penalty landed anyway
- `MAX_REASON_LENGTH` is now exported from the contract and imported here,
mirroring `MAX_FINDINGS_PER_REPORT`, instead of being a second literal
that can drift from the `.max(...)` it is supposed to track. Its comment
no longer implies live protection: measured, `renderReason`'s longest
possible output is 311 characters with every numeric field at
`Number.MAX_SAFE_INTEGER` (269 smited, 311 open), so the truncation is
defence in depth against a future template, not something that fires today
- one unrelated prettier hunk in `new-order.service.ts`: the file was
already prettier-dirty at HEAD and the lint gate checks changed files
Tests: a new `smite-durable-write.test.ts` exercises the REAL `smitePlayer`
and pins the seam — the hook fires before the tail, still fires when the
tail throws, and does NOT fire when the row was never written. Moving the
hook to the end of the function kills two of those, each for its own reason.
The scan suite gains the mirror pair: a smite that throws with nothing
written files `actioned: false`; one whose row landed and then threw files
`actioned: true, action: 'smite'`. The second was watched RED on pre-change
code. Its fake was also wrong — a successful `smitePlayer` fires the hook,
so a fake that merely resolves models a call that wrote nothing.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01E5Fk4Kuu5MjW1Ap51VWaPg
* fix(new-order): kill a surviving mutant on the third-strike path, and stop four comments overclaiming
Round-2 delta audit. The important one is the first; the rest are claims the
code does not support.
1. The seam guard could not see the worst path. `smite-durable-write.test.ts`
named itself "before any of the non-durable tail runs" but asserted ordering
inside `smitesCounter.increment` — the LAST tail step — while
`mockCount.mockResolvedValue(1)` kept the third-strike branch unreachable in
every case in the file. Moving `onSmiteCreated?.(smite)` below the
`if (activeSmiteCount >= 3) return resetPlayer(...)` block survived the whole
suite: on a third strike the branch returns early, the hook never fires, and
an irreversible career reset files as actioned:false, "No action was taken by
this scan" — the exact inverse invariant this work exists to remove.
The guard now pins ordering against the FIRST tail step (the active-smite
count), leaving no step for the hook to sink past, and adds a third-strike
case that asserts the hook fired before resetPlayer's cleanse. Both go red on
that mutation. The branch case checks the cleanse really ran and that the
increment below the `if` was not reached, so it cannot pass vacuously.
2. toFinding's docblock stopped one sentence short. It correctly said
actioned:true does not claim a career reset completed; it did not say that on
a third strike resetPlayer cleanses every active smite INCLUDING the one just
written. A moderator opening that row finds zero active smites and a reset
account — a larger action than the row states. Behaviour unchanged, predates
this work; the comment now names the observable.
3. The exported MAX_REASON_LENGTH's comment read as though importing it made
producer truncation and parser cap equal by construction. It does for one
producer of three: bot-account-detection and reaction-withdrawal-detection
still declare their own literal. Reworded to say what was done, plus a
ledger test pinning the set of producers holding a local copy — failing if it
grows or shrinks. Migrating the other two is out of scope; each has its own
suite pinning the literal.
4. truncateReason's comment said an over-limit reason "400s the REPORT".
moderatorApp.abuseReport parses before the fetch, so it throws a ZodError in
the job's own process and no request is made. toFinding's docblock 100 lines
below already had this right. The same wrong phrasing is pre-existing house
style in bot-account-detection/report.ts and was left alone.
5. The new parameter carried a prose precondition — "must not throw" — on a
newly-exported seam past the point of no return. Wrapped in try/catch so a
future caller's bug cannot strand an account with a live smite and no tail.
The comment no longer claims a throw can abort anything; a test covers it.
6. report.test.ts called Number.MAX_SAFE_INTEGER "the ceiling for a JSON number
off ClickHouse". It is not one — JSON.parse yields a double and a UInt64
exceeds it by ~2,000x. The guard's conclusion is unaffected; only its stated
reason was wrong, so the reason is replaced rather than the guard.
Verified: typecheck OK, 0 type errors. 93 tests pass across the 11 affected
suites (up from 87). Both mutations above were watched to fail and the guards
watched to kill them.
* fix(new-order): close a silent-pass hole in the ledger guard, contain async hook rejections, and stop three comments naming the wrong failure
Round-3 delta audit. Four items, scoped to nothing else.
1. max-reason-length-ledger: delete stripComments, read raw source.
The stripper's block-comment regex was blind to string literals, so a producer
whose report.ts carried the comment-opening pair inside any string (a URL, a
route glob) opened a phantom comment that ran to the next terminator and
deleted the real MAX_REASON_LENGTH declaration before LOCAL_DECLARATION ever
saw it. That is a silent pass in the exact direction the guard exists for.
It also bought nothing: LOCAL_DECLARATION anchors to whitespace-only before
the keyword, so a leading comment marker can never match it, which was the
only motive the stripper's docblock cited. Watched it work -- a synthetic
producer of that shape passed green before and now fails the ledger by name.
The residual risk inverts to the safe direction (an unprefixed declaration
inside a block comment now over-reports, failing loudly).
Also removed the docblock claim that the controls pin both directions -- they
name the three known files by hand, so they say nothing about a new one -- and
added the nearest-neighbour limit the list was missing: a copy under a
different identifier survives green.
2. smitePlayer's onSmiteCreated seam: the try/catch delivered less than it said.
(a) It was sync-only. TypeScript's void-return rule accepts an async function
at a "=> void" position, so a rejecting hook produced a genuine unhandled
rejection -- this repo installs no global unhandledRejection handler. Both
shapes are now contained, and the order matters: Promise.resolve(hook())
cannot catch a synchronous throw, because the hook is evaluated as the
argument before Promise.resolve runs. The try covers that; a .catch on the
result covers the rejection. The parameter type now admits Promise<void>
explicitly rather than accepting it silently.
(b) The swallow was unobservable and its stated reason was self-undermining.
The catch was bare while handleLogError was already imported three lines
below. Before this seam existed a hook throw reached the job's own
handleLogError; afterwards it reached nothing. Now logged under a stable,
opaque key with the id in the details.
Mutation-checked both halves independently: removing the .catch fails only
the rejection case (and Vitest reports the unhandled rejection directly);
restoring the bare catch fails only the synchronous case.
3. The exported constant's note no longer claims a NEW producer cannot quietly
copy the literal. It catches the common shape, not every shape -- a copy under
a different name, or a producer laid out differently, stays green -- and it now
says so instead of replacing one confident sentence with another.
4. "400s the REPORT" was wrong at all three live sites, not the one the previous
round scoped. Every producer sends through moderatorApp.abuseReport, which
runs abuseReportInput.parse before the fetch, so an over-long reason throws a
ZodError locally and nothing is ever sent. The wrong wording sends a reader
hunting a spoke-side 4xx that cannot exist. Fixed in both producer modules and
the one test comment; re-enumerated to zero against a positive control.
Verification: typecheck 0 errors; 13 affected suites, 428 tests, all passing;
Prettier clean on all 7 changed files against a negative control.
* style(new-order): reflow the ledger docblock, and record why a naive glob does not reproduce the stripper hole
* fix(new-order): normalise a non-Error hook throw before logging, and widen the hook return type
`reportHookFailure` handed its value straight to `handleLogError`, which builds
`new Error(e.message ?? ...)` with no guard. A non-`Error` throw value therefore made the
LOGGER throw a TypeError, defeating the containment it was called from:
- sync `throw null` — the TypeError escaped the `catch` meant to contain it, so
`smitePlayer` rejected and the tail (counter, signal, notification) never ran. That is
exactly the half-applied smite this block exists to prevent: the row is committed, the
derived state is not.
- async rejection — the TypeError left the `.catch` handler, so the derived promise
rejected with nothing to catch it: an unhandled rejection.
Normalise with `e instanceof Error ? e : new Error(String(e))`. Two new cases cover both
shapes, asserting the tail completes and that an `Error` reaches the logger. Both were
watched failing against the previous commit first. The suite's `handleLogError` mock is now
faithful to the real function's unguarded deref — a bare `vi.fn()` accepts `null` happily,
which would have made both cases pass against code that throws in production.
Separately, `onSmiteCreated` returned `void | Promise<void>` and the docblock presented that
as purely permissive. It is the opposite: TypeScript's void-return exemption applies only to
a target of exactly `void`, and a union does not get it. Measured with tsc 5.9.2, the union
rejects an expression-bodied arrow whose body returns a value — including the idiomatic
`(s) => set.add(s.id)`, since `Set.add` returns the Set. `=> unknown` accepts every shape
probed; `void | Promise<unknown>` still rejects that one, so it was not taken. The docblock
is corrected, including the claim that spelling out `Promise<void>` is what prevents an
unhandled rejection — the `.catch` is what does that.
Finally, the comment asserting there is no global `unhandledRejection` handler "in this
repo" now says "in this process", which is what is actually checkable as written.
* fix(new-order): stop the hook-failure normaliser from throwing, and correct a test comment that stated the opposite of the test
The normalisation added last round used String(e), which itself throws for any
value whose primitive conversion throws — a null-prototype object, an object
with a throwing toString, a revoked Proxy. It runs inside the catch that exists
to contain the hook, so those shapes reproduced the very failure the block was
written to close: measured, smitePlayer rejected with the smite row already
committed and smitesCounter.increment took 0 calls.
Carry the value as `cause` instead. new Error(msg, { cause: e }) stores the
reference and reads no property of e, so it runs no user code for any throw
value; verified on node v24.19.0 that it is safe for all three shapes while
String(e) throws on all three. Object.prototype.toString.call(e) was rejected as
the alternative — same check shows it still throws on a revoked Proxy.
New red-first case, `Object.create(null)`: before the fix it failed with
"TypeError: Cannot convert object to primitive value" raised at the String(e)
site, and with the rejection absorbed the tail assertion read 0 calls.
Also corrects the mock comment in smite-durable-write.test.ts, which claimed the
faithful handleLogError mock is what keeps the two non-Error cases honest. It is
not. Re-measured here: with a bare vi.fn() AND the normalisation reverted both
cases still fail, at expect.any(Error) (- Any<Error> + null). The mock adds
production-symptom fidelity on the sync case only — the failure presents as a
rejected promise rather than an argument mismatch; on the async case it changes
nothing. The comment now names expect.any(Error) as the guard and warns against
relaxing it to expect.anything(), which is the edit that would make both cases
vacuous. The file already said the true version 270 lines further down.
Finally, finishes the `=> void` retirement: the two local aliases in
auto-smite.test.ts still mirrored the parameter as `=> void` while the parameter
is `=> unknown`. Enumerated all 21 onSmiteCreated occurrences across 5 files;
exactly those 2 carried `=> void`, and both are now aligned.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01E5Fk4Kuu5MjW1Ap51VWaPg
* style(new-order): drop a line-position claim from the new test comment that was about evaluation order
* chore: retrigger CI after the PR-preview kubeconfig repair (talos-infra#1526)
---------
Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
||
|
Zachary Lowden
|
526fbabd53 |
refactor(app-blocks): drop the redundant mod-Discord notify from the shared-storage report path (#4824)
* feat(app-blocks): file shared-storage user reports on the mod abuse board
A user reporting an App Blocks shared-storage row wrote a `shared_kv_reports`
row that NOTHING read. Ordinary abuse — harassment, brigading, spam that
dodged the synchronous content audit — was therefore invisible to moderators,
and a fire-and-forget mod-Discord webhook was bolted onto the report path as
its only reader. A webhook post is not a queue: it cannot be triaged, ranked,
assigned or ruled on, and it is gone as soon as it scrolls.
Replace it with the surface that already does all of that: the moderator
abuse-detection board at /abuse, the same one bot-account-detection and
reaction-withdrawal-detection write to. No new page, no Prisma model, no
migration.
New: a daily sweep job + service, mirroring both precedents.
src/server/jobs/shared-storage-report-sweep.ts
src/server/services/shared-storage-report-sweep/{reader,report,run}.ts
- ONE run per window, not one run per report: the board is run-shaped and
its index lists runs, so a run per report would push every other detector
off the page.
- ONE finding per reported row, not per report: five users flagging one row
is one moderator decision, and a stronger one — distinct reporter count is
what raises the finding's confidence, i.e. its place in the queue.
- Cadence and window are one number in two places: 24h lookback, cron
"0 7 * * *". The run does not dedupe across runs, so a shorter window
drops reports and a longer one re-files them for ever. Pinned by a test.
- finding.userId is the AUTHOR of the reported row — the account the finding
is about — never the reporter. A report whose row has been purged has no
subject; it is counted and named in the summary rather than filed against
the person who flagged it.
- actioned: false is a literal and `action` is absent by construction; the
contract's refinement rejects the whole batch otherwise. The sweep holds
no write client.
- Overflow past MAX_FINDINGS_PER_REPORT splits into further reports rather
than truncating — dropping a finding would be the original hole again.
Removed from apps-shared.router.ts: notifyModsOfSharedReport and its call
site, plus sanitizeDiscordText and its test block. The logToAxiom emit stays
— that is separate observability, not the thing being replaced, and the
comment beside it now says so.
The reporter free-text hardening did NOT go away with the webhook: it moved
to sanitizeReportReason in the sweep's report.ts, widened for its new reader.
Discord markdown was a Discord hazard; the board renders the reason through
Svelte text interpolation, so nothing there can execute or link. What the
board still needs, and now gets, is a hard length cap (an over-length reason
does not lose one finding, it 400s the report and loses every finding in the
batch), single-lining including the control characters \s does not match, and
the same markdown/masked-link strip kept because it costs one regex and this
file cannot promise what the board's renderer will be.
METADATA ONLY, enforced twice: the per-app query never selects
shared_kv.value, and nothing downstream has a field to put it in. Guarded by
a sentinel-fixture regression test watched to fail on a deliberate leak.
DISCORD_WEBHOOK_MOD_ALERTS stays in the env schema — other consumers remain.
* fix(app-blocks): write the group-key separator as an escape, not a raw NUL byte
The (app, key) group key in the shared-storage report sweep separated its two
parts with a LITERAL NUL byte. It worked, and it was invisible: an editor and
`git diff` both render it as nothing, `grep` refuses the file, and git
classified `run.ts` as BINARY — `Bin 0 -> 8204 bytes` in the diffstat, with no
reviewable content at all.
Write it as a `\u0000` escape and say in a comment why a NUL rather than a space: a
`shared_kv` key is arbitrary text up to 64 chars while a slug matches
`^[a-z][a-z0-9_]{2,40}$`, so only a character outside the slug alphabet makes
the pair unambiguous.
No behaviour change — the runtime string is identical, and the suite is green
either side of it.
* fix(app-blocks): state the report SPAN on a finding, and satisfy Prettier
Two things, both found after the first push.
1. `firstReportedAt`/`lastReportedAt` were collected by `groupReports` and then
never rendered — dead fields on a type whose whole job is to carry only what
a moderator needs. They are now the thing that makes a multi-reporter row
readable: five reports inside four minutes is a coordinated brigade against
the reported row's AUTHOR and may itself be the abuse; five spread over
twenty hours is five people independently agreeing. The count alone cannot
tell those apart, and they want opposite actions.
Stated only when there is more than one report — "over 0 second(s)" on a
single report would read as a claim rather than as an absence. `renderSpan`
floors, never rounds up, because the number feeds a "was this coordinated"
judgement and has to be safe to take literally.
Guarded, and the guard was watched to fail: replacing the min/max tracking in
`groupReports` with first-seen/last-seen turns the out-of-order arm red
("expected … to contain 'over 4 minute(s)'", received "over 0 second(s)").
A reader is promised to return rows, not sorted rows.
2. `report.ts` and `run.test.ts` failed `prettier --check` (CI's "ESLint +
Prettier (changed files)" job). Reformatted. The other eight touched files
were already clean — and were flagged as such in the same invocation that
flagged these two, so the pass on them is a real verdict rather than a glob
that matched nothing.
* fix(app-blocks): do not sweep MODERATOR audit rows onto the abuse board
THREE things write `shared_kv_reports`, not two, and the filter only knew about
one of the other two.
1. the USER-report path — `key` set, reporter = reporting user, free text
2. the auto content-safety path — `key IS NULL`, `auto:<category>`
3. `apps.mod.purgeSharedRow` — `key` set, reporter = THE ACTING MODERATOR,
`mod:<action>`
(3) sets the same two columns a real user report does, so `reporter_user_id IS
NOT NULL AND key IS NOT NULL` swept it up. Every moderator action on a shared
row would have been re-published to the abuse board as a user report, naming the
moderator as the reporter and the row they had just dealt with as the subject —
noise, on the surface whose whole value is that it is triageable.
And the comment asserting (2) was excluded because it has "no reporter" was
wrong on its own terms: the auto path writes the BLOCKED WRITER's uid. It is
excluded by its NULL key, which is a different mechanism from the one the
comment named. Corrected, and the reason for each exclusion is now pinned by a
test rather than asserted in prose.
The fix is deliberately TWO tests joined by AND, because each is walkable alone:
- the `mod:` prefix ALONE is a guard on a USER-SUPPLIED STRING. The reason on
a real report is the reporter's own free text, so anybody could type
`mod:purge` and remove their own report from the only surface that would
have shown it — an evasion of exactly what this job provides.
- the "reporter is a moderator" test ALONE would swallow a moderator's own
genuine user report, which carries an ordinary reason and has to reach the
board like anyone else's.
The moderator half is one bounded `dbRead.user.findMany` per run, over only the
rows the prefix already flagged — so a run with no `mod:`-prefixed rows makes no
query at all, and it can never become one lookup per report row.
All three mutations die, each to the arm that covers it:
- prefix only → the non-moderator-evasion test fails (1 vs 0 skipped)
- moderator only → the moderator's-own-report test and the
lookup-scope test fail
- filter removed → the discard test and the all-mod-rows-is-quiet test
fail
Counters `user_reports` and `mod_action_rows_skipped` are added so "the board is
quiet" stays distinguishable from "everything got filtered".
Also pins the cross-file coupling: `MOD_ACTION_REASON_PREFIX` is exported and
`job-wiring.test.ts` asserts it against the router's actual write site. There is
no type between those two files, so changing the prefix at the write site breaks
nothing, type-errors nothing, and silently reopens this.
142 tests green; typecheck 0 errors.
* docs(app-blocks): say why mod review-sandbox reports cannot reach the abuse board
The apprev_<publishRequestId> preview schemas carry the same shared_kv_reports table, and a moderator running an unapproved app "for real" can file rows in one. Those are scribbles on a disposable preview, not abuse, and must never be published. They cannot be: every schema name this discovery builds is app_${slug}, and the apprev_ prefix can never alias an app_ one. That is exclusion by construction rather than by a filter someone could drop — which is exactly why it is invisible, and why it is now written down.
* docs(app-blocks): state the content-leak guard reproduction against the right file
The docstring said the leak had been reproduced by editing report.ts. It was not — the mutation was in run.ts, in groupReports, folding the source row value into the group reasons. Anyone following the comment to re-break the guard would have edited a file with no such code and concluded the guard could not be broken, which is the worst possible reading of a guard. The exact mutation, the exact failure text and the one-failed-of-seventeen count are now written out, because a guard nobody can re-break is a guard nobody will maintain.
* refactor(app-blocks): drop the sweep half of this PR, keep the mod-notify removal
The sweep job this PR added cannot do anything in production, on two
independent measurements:
1. It has no database to read. The reader it depends on resolves to null in
the environment app jobs actually run in, so every run takes its skip
branch and files nothing — silently, forever. Nothing about that is
visible from the job's own logs or from a green test suite.
2. There is nothing for it to read. `shared_kv_reports` has held four rows in
its entire production history: two from the automatic content audit
(excluded by the sweep's `key IS NOT NULL` filter) and two moderator
purge-audit rows (excluded by its moderator filter). User reports to date:
zero.
So the whole sweep is removed: the job, its service directory and tests, and
its registration in the jobs array (that file is now byte-identical to main).
What remains is the change originally asked for — the removal of the
fire-and-forget mod-Discord notify from the shared-storage report path, which
was redundant with the structured Axiom event emitted beside it. The Axiom
emit stays; it is separate observability and was never in scope.
`sanitizeDiscordText` goes with the webhook rather than being replaced. Its
only job was neutralising Discord markdown — a masked link rendering live in
a mod channel — and that renderer is now gone. Nothing else consumes the
reporter's free text: it is stored raw in the report row and logged raw as a
structured Axiom field (deliberately, so the record is not corrupted), and
its length bound comes from the input schema's `z.string().max(500)`, not
from that helper. There is no remaining rendering sink to harden, so this
drops a guard whose hazard no longer exists, not the handling of hostile
input.
Verified: `pnpm typecheck` OK, 0 type errors. The router suite is 105/105,
and it was watched red first — suppressing the kept Axiom emit fails exactly
3 of those tests, so they cover the behaviour this commit preserves.
|
||
|
Zachary Lowden
|
73e62594e2 |
refactor(apps): drop the redundant publish-request mod-notify (#4822)
The App Blocks "new publish request" Discord notify duplicated a surface we already have: /apps/review lists every pending publish request, and /apps/review/[publishRequestId] is the detail view mods actually work from. The webhook embed carried nothing the queue page does not already show (slug, version, submitter, change counts, request id), so it was a second, lossier copy of the queue that had to be kept in sync by hand. Removes notifyModsOfNewRequest and its single call site in submitVersion, along with the dbRead.user lookup that existed only to populate the embed's "Submitted by" field. submitVersion now returns immediately after the optional draft-listing create. env.DISCORD_WEBHOOK_MOD_ALERTS stays in the server schema - four other consumers still read it (mod-alert, auto-feature-health-check, new-order-jobs, and the blocks git-push route). Test changes are confined to what the deletion made dead: the two notify tests, the fetch stub and afterEach that existed only for them, the DISCORD_WEBHOOK_MOD_ALERTS env overrides in both publish-request suites, the now-unused dbRead.user mock delegate, and the comments referencing any of it. Verified: pnpm typecheck OK, 0 type errors. vitest unit over src/server/services/blocks/__tests__/ - 171 files, 4100 tests, all passing; the orchestration suite itself goes 121 -> 119 tests, exactly the two notify cases removed. |
||
|
Zachary Lowden
|
f8cde8e007 |
feat(bot-account-detection): add the asset-staging heuristic (#4825)
* feat(bot-account-detection): add the asset-staging heuristic
Adds a fourth heuristic, `asset-staging`, and the evidence source behind it.
WHAT IT DETECTS
An account whose uploaded images are STAGED rather than published: each image
carries no generation metadata (a SQL NULL or the JSON literal null, so it was
not produced on this site) and is attached to no post. The score rises with how
many such uploads an account has, and rises faster when several of them were
created inside one second — a batch a program submitted rather than a person
picking files.
It is the first heuristic in the registry that is not a ring detector. The other
three ask how many OTHER new accounts share a registration IP, an email domain,
a comment or a filename, so all three are blind to an account working alone.
This one scores an account on its own uploads and needs no second account to
exist.
THE BLEND DENOMINATOR
`scoreAccount` divides by the total weight of the WHOLE registry, not by the
heuristics that had input, so a fourth equally-weighted entry multiplies every
existing account's confidence by 3/4 — a flat 25% haircut with no heuristic
having changed its mind.
`MIN_REPORTED_CONFIDENCE` is a literal. Left at 0.15 it would have gone on
admitting only a signal ~0.6 convinced, and every account whose sub-scores summed
to [0.45, 0.60) — i.e. whose confidence sat in [0.15, 0.20) — would have dropped
off the board with no counter recording it. It is re-derived to 0.1125, which is
the same "one signal about half convinced" cut its docstring has always argued
for, so the reported population is unchanged for every account the new heuristic
scores 0 on. The relationship is now pinned by a test that multiplies the cut by
the registry's actual length.
The sole-signal dominance multiple was a literal `3` whose own derivation is
"the smallest integer greater than n - 1", i.e. n. It is now computed from the
registry size: unchanged at three entries, 4 at four.
DOCSTRING CORRECTION
`BotAccountHeuristic.weight` claimed the blend "divides by the total weight of
the heuristics that ran, so adding one does not silently dilute the others'
meaning". That contradicted `scoreAccount` in the same file and was wrong in the
most expensive direction — it told the one person in a position to cause the
dilution that it cannot happen.
EVIDENCE LAYER
One read per member with a per-member cap, its own budget, and its own
availability and read-failure flags, following the shape the filename read was
rewritten into. A dead staged read leaves this heuristic asserting that every
account published what it uploaded, which is a claim about each account rather
than a weaker claim about the cohort, so the run summary and the counters name
it explicitly.
* fix(bot-account-detection): re-derive the asset-staging firing point to two
The heuristic's boundaries were calibrated against a predicate it does not
ship. The backtest behind them measured a RATIO — "all of this account's
images are staged" — while the code ships an unratioed COUNT. Re-measured
against the shipped predicate on a matured cohort, the volume boundary of 8
selected a population too small to carry any signal at all, and a firing point
of two separated strongly on a population large enough to mean something. The
measurement lives in the private infra repo; this repository is public, so the
constants are attributed here without its figures.
THE BOUNDARIES, AND WHERE THEY COME FROM
STAGED_ONE_AT 8 -> 3 and BURST_ONE_AT 4 -> 3, both zeroAt unchanged at 1.
Neither is picked. The blend divides by the WHOLE registry, so a lone
sub-score s becomes s/n and is compared against MIN_REPORTED_CONFIDENCE, which
is LONE_SIGNAL_CUT/n — the n's cancel, and a lone signal is reported exactly
when s >= LONE_SIGNAL_CUT. Requiring a count of two to clear that cut gives
1 / (oneAt - 1) >= 0.45 <=> oneAt <= 1 + 1/0.45 = 3.22...
so 3 is the largest integer that works. A pair scores 0.5, clear of the cut by
more than a rounding step; three saturates. oneAt = 4 would put a pair at
0.333 and leave it unreported, which is the behaviour being removed.
THE FIRING POINT IS PINNED, NOT ASSUMED
A new case runs the real registry through the real blend and the real
partition and asserts the REPORTED/SUPPRESSED verdict: one staged upload is
suppressed, two is reported. Against the pre-change constants it fails with
"expected 0 to be 1" — the defect itself, not a ramp value someone has to
translate. The same property is asserted a second time through the run's real
reader, evidence layer and report rendering in run.test.ts.
THE BURST ARM IS NOW INERT, AND THE CODE SAYS SO
A same-second group is a SUBSET of the staged rows, so burst <= count always;
both halves now run through identical boundaries and rampScore is monotonic,
so max(volume, burst) is identically volume. The burst arm can change neither
whether the heuristic fires nor how high it scores.
That is stated rather than papered over. Its previous rationale — a tighter
boundary pair expressing "concentration is the stronger claim" — was consumed
by the move to two, and no replacement is invented: the re-measurement does
show same-second concentration separating harder one count further up, but on
too few members to author a constant from, so it has not been used. What the
arm still produces is the separately-reported half-score behind
fired_volume/fired_burst, which is how the shadow phase will decide whether to
re-tighten it or delete it, and the moderator clause naming the batch.
max is kept rather than collapsed to volume because the identity is a property
of the two boundary pairs being equal, not of the model — a hardcoded volume
would silently drop the arm the moment either pair moved. Measured and
recorded in the comment: the "return volume" mutant SURVIVES a fully green
suite, because it is semantically equivalent today and not because the tests
are thin.
The false-positive class got bigger and that cost is named in the header: a
two-file abandoned drag now scores enough to be reported on its own, so this
signal's precision rests on the shadow phase measuring it rather than on the
boundary being cautious.
LONE_SIGNAL_CUT IS MARKED PROVISIONAL, NOT DEFENDED
Its value is unchanged. Its docstring claimed it "carries the JUDGEMENT" and
the test pinning it defended the literal as a decision. Nobody made that
decision: 0.45 is the back-derivation of the previous literal cut, 0.15
against a registry of three. Extracting it made the relationship checkable,
which is real, but extraction is not calibration. Both the docstring and the
test comment now say inherited and provisional, and the earlier claim is
retracted in place rather than left to be cited. The relationship assertion —
the guard doing the work — is untouched, and the literal pin is kept for its
one remaining job: stopping the relationship being satisfied by moving both
numbers at once.
VERIFICATION
Red at base: 10 cases fail against the pre-change constants, each on its own
assertion naming the pre-change behaviour. Mutation battery on the changed
guards: 7 of 8 killed, each by the expected case on that case's own assertion
(STAGED_ONE_AT 3->4 killed by the firing-point verdict; BURST_ONE_AT 3->2 by
the subset pin; LONE_SIGNAL_CUT 0.45->0.5 by the relationship assertion, not
by the literal pin; MIN_REPORTED_CONFIDENCE 0.1125->0.15 by the firing point,
which proves the two are genuinely coupled). The eighth is the equivalent
mutant recorded above. 418 tests over 8 files, all collected and passing;
typecheck, ESLint and Prettier clean.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* docs(bot-account-detection): say the burst half is invisible to the score
`assetStagingHalfScores` still framed its purpose as answering "is the burst
half doing work the volume half was not already doing" as though the sub-score
could contribute to that answer. It cannot: with both boundary pairs equal,
max(volume, burst) is identically volume, so the score carries no information
about this half at all and the counters built from these two values are the
whole of the evidence that will decide whether the arm is re-tightened or
deleted.
Also states the trap a reader of this function can walk into — burst > 0 here
does NOT mean the burst half contributed to the account's sub-score — and that
the reverse case (burst non-zero while volume is zero) is unreachable for any
index the evidence layer can build.
Comment only; no behaviour change. 418 tests pass, Prettier clean.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* docs(bot-account-detection): the burst counters no longer settle what they claimed
The comment over `stagedHalfFired` said the two counters were there to settle
"whether the same-second half ever fires on an account the volume half did not
already carry". Since the firing point moved to two, that question has a known
answer -- NEVER -- and it is known by arithmetic rather than by measurement: a
same-second group is a subset of the staged rows, so a burst of two implies a
count of at least two, and both halves now share boundaries. The combination
`fired_burst > 0 && fired_volume == 0` is unreachable.
That is a description claiming coverage the counter cannot provide, which is
worse than no description: it would have had someone watch a counter for a
signal that cannot arrive and read its silence as an answer.
Replaced with the question these counters CAN still settle, and which is the
reason to keep them: whether the accounts whose staged uploads arrived in one
batch are actioned at a different rate than the accounts carried by volume
alone. That is a grading question over outcomes -- a join against moderation
results, not either counter read on its own -- and its answer is what decides
whether the burst arm gets a tighter boundary again or is deleted.
Comment only; no behaviour change. 418 tests pass, Prettier clean.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
---------
Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
||
|
Zachary Lowden
|
26b47fa98e |
docs(app-blocks): de-line six stale file:line cross-references, and correct a false justification (#4838)
* docs(app-blocks): de-line six stale file:line cross-references
Six citations named a file:line that no longer held what the citing
sentence claimed. Each was re-measured against main and replaced with the
SYMBOL, function or branch it means — a fresh line number would be wrong
again on the next edit, which is exactly how these six got here.
docs/features/app-blocks.md (4):
- CACHE_TTL_SECONDS :39 was an import stmt -> name the file only
- invalidateModelCache :461 was a kill-list note-> name the file only
- KILL_LIST_CACHE_TTL_MS :480 was a catch comment -> name the file only
- MAX_BLOCKS_PER_SLOT :1849 was an NSFW comment -> installOnModel
Two bare line-lists in the SAME two sentences were also stale and are
de-lined with them: the four invalidateModelCache callers (cited 1950 /
2019 / 2055 / 2108, actually installOnModel / uninstallFromModel /
toggleEnabled / updateSettings) and the kill-list filter on a cache hit
(cited 657-664, actually the kill.has(r.blockId) filter on listForModel's
cache-hit branch). Leaving a known-wrong number attached to a clause whose
other half was just corrected would ship a statement measured false.
src/shared/constants/block-effective-scopes.ts (2):
- blocks.router.ts:2787-2789 was a pinned-install count mapping
-> grantScopes' `const ceiling = new Set(effectiveBlockScopes(...))`
- scope-grant.service.ts:222-224 was the buzzBudgetPerDay opts field
-> recordScopeGrant's `const incoming = Array.from(new Set(...))`
Matches the de-lining style three citations in that same file already use.
NOT changed, because they were re-measured and are ACCURATE — they are the
positive control proving this audit discriminates rather than rewriting
everything it touches: block-registry.service.ts:329 (the
OwnedNonApprovedPageBlockResolution docblock sentence), block-tokens
:1054 / :469 / :650 / :375-389 / :455-459, block-registry:2117-2119 and
:2104-2114, and block-manifest-validator.service.ts:478-496. Nine of the
eleven citations checked in that file are exact.
Comment/docs only; no behaviour change.
* docs(app-blocks): the full-bleed ledger's reason is an operator decision
The `WHY THIS IS CSS AND NOT A MANIFEST FIELD` block argued from a real
premise to a conclusion that does not follow. It said the manifest schema
is mirrored across three repos and this host is pinned to
@civitai/app-sdk@^0.14.0 while guests ship 0.35.x, "so a new manifest field
would be UNTYPED at exactly the point the host consumes it."
The pin is real. The conclusion was measured false: the host never reads a
manifest through an SDK type at all.
- src/server/services/block-manifest-validator.service.ts imports nothing
from @civitai/app-sdk (0 matches; 6 import statements as a positive
control) and validates against this repo's own rules.
- src/components/AppBlocks/types.ts DECLARES the host's own BlockManifest.
Its one "@civitai/app-sdk" hit is prose in a docblock ("Matches
@civitai/app-sdk/blocks v1"); the file has no import statements at all.
The conclusion — keep full bleed in CSS — stands, but on its actual author
of record: the repo owner decided full bleed should be managed by styling,
not a manifest field. PR #4812 built the manifest field and was closed
unmerged on that call. That decision is now stated as the reason, and the
false technical reason is kept alongside it, explicitly retracted, so it is
not rediscovered and acted on. Deliberately NOT replaced with a freshly
constructed technical argument: inventing a replacement reason is the
failure that produced the false one.
The three-repo mirroring IS true and is kept, now as the measured COST of
the alternative rather than as the reason. Verified first-hand that all
three copies declare `page` with additionalProperties:false and the
identical four keys: public/schemas/app-block/v1.json (canonical), the Go
CLI's schema/app-block.manifest.schema.json, and app-sdk@0.14.0's vendored
schemas/app-block/v1.json. Against the released CLI 0.1.101, adding an
undeclared key under `page` makes `civitai app validate` exit 1 with
"page: additional properties 'fullBleed' not allowed", locally, before any
network call (negative control); the unmodified manifest exits 0 (positive
control).
Comment-only; no CSS rule, selector or declaration changed. `/*` and `*/`
counts equal at 31/31 before and after.
ledgerSelectorSurvivesProdStrip.test.ts parses selectors out of this file
INCLUDING its comments, so it was instrument-validated rather than merely
run green: injecting a data-testid-keyed selector into this comment turns
it red naming data-testid (1 failed / 6 passed), and it is green at
16/16 with pageBlockHostMaxWidth.test.ts once reverted.
* docs(globals): cut the retraction to what is measured, not a fourth draft
The replacement prose asserted three things beyond the measurement, in the
paragraph that replaced a claim retracted for exactly that:
- "and it is strict" of the three schema mirrors. Measured false today:
`@civitai/app-sdk@0.14.0`'s vendored copy is missing five top-level
properties the canonical declares, and the Go CLI is missing one. The
mirror is loose, and the drift sat there unnoticed.
- a re-vendor cost attributed to all three copies. Only the CLI was ever
measured to block; nothing consumes the SDK's copy as a validator, and a
manifest's $schema names the canonical URL.
- "its 'Matches @civitai/app-sdk/blocks v1' line" bound to `BlockManifest`,
which has no docblock — that line belongs to the BLOCK_INIT payload type.
Cut rather than redrafted. Every sentence here is a claim that can rot, and
this block has now been wrong twice; a shorter one has less to be wrong
about. The comparative clauses ("one line", "needs no schema motion") went
with it: they argued for the mechanism under a label saying they did not.
Guard re-validated on this tree, not inherited: a literal `*/` planted in
the new paragraph turns ledgerSelectorSurvivesProdStrip and
pageBlockHostMaxWidth red (2 failed / 14 passed); restored, 16/16 green and
the comment balance is 31/31.
|
||
|
briant
|
959ebbbc1f |
fix(seo): name a deleted review author "Civitai user" in ld+json
A review by a deleted user shipped `author: undefined`, so the Review schema had no author at all. `deleteUser` also nulls `username`, so the page title and description interpolated it raw — /reviews/31461 rendered "Reviewed by null". Image detail had the same gap on `creator`. `creditText` and `copyrightNotice` stay undefined there: those assert rights, and naming a gone owner in them would be a claim we can't make. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BGe2oiydDx5r6j3V163Ezd |
||
|
Zachary Lowden
|
730a6b14e2 |
feat(apps): one nav instead of two — a collapsible left rail for /apps/* (#4813)
Replaces the second horizontal nav bar on /apps/* with a collapsible left rail, following the AccountLayout section-registry pattern and the CollectionsLayout rail mechanics. 260px open (default), 56px collapsed, localStorage plus a cookie mirror for the SSR seed, one global state across all 12 routes so the chrome alignment invariant holds, Drawer below 1300px. Also ships a repo-wide ESLint rule (no-ssr-divergent-media-query) banning the four media-query hooks with no server answer, and removes ~742 lines: a hand-rolled AST walk whose hazard was already covered by the SSR render test, and the localStorage half of the rail store. No store column ladder change ships. The re-tune was reverted: a rail-state-aware rung is not expressible as one width-keyed ladder, and a flat one made viewers with no rail on screen pay a column. The density cost is now confined to viewers who opened the rail, in the 1600-1920 band. Verified: step-level green across 13 check-runs and 7 commit statuses on the merged tree; unscoped unit project 1,796/1,800 files passing, with the single failure established pre-existing by a control run at origin/main. Not verified: the page has never been loaded in a browser at any viewport. See the accepted-exposure comment on the PR. |
||
|
Zachary Lowden
|
1b886706a1 |
fix(app-blocks): refuse block REST calls from a non-approved app (#4818)
* fix(app-blocks): refuse block REST calls from a non-approved app A moderator suspension and an uninstall are two deliberately disjoint signals. Uninstall and toggle-off write a Redis revocation marker; a suspension flips app_blocks.status and writes no marker. withBlockScope read only the first, so after a takedown every block REST route kept serving for the remaining life of any already-minted token, while the tRPC bridge (which gained the status check in #4806) refused. Traced at this revision: of the 13 page routes that wrap withBlockScope, 11 have nothing below them that reads app_blocks.status. Executed, not inferred: at the base revision a suspended app driving POST /api/v1/blocks/tip called createBuzzTipTransactionHandler, and one driving POST /api/v1/blocks/collections/:id/follow called addContributorToCollection. withBlockScope now resolves the backing app_blocks row after the revocation check and refuses a non-approved one — 403 not approved, 404 no row, 503 on a lookup failure (fail-closed, unlike revocation, which fails open inside its own primitive). Exempts claims.dev === true, the same predicate the bridge guard applies, so the moderator review sandbox and the owner dev-tunnel keep working on a non-approved app. Exposure was already bounded: the mint endpoint requires approved, so a suspended app cannot obtain a new token. What was open was the tail of the tokens it held. Also adds a refusal counter split by reason, so the gate can be watched before it is relied upon: not_approved is the gate working, not_found is a healthy app being refused and is the signal to back it out. * fix(app-blocks): serve a missing app_blocks row instead of refusing it, and share one approval predicate Two audit findings on the REST approved-status gate. 1. A MISSING ROW NOW SERVES. Every moderator takedown leaves a row whose status is not 'approved' -- a suspension flips the status, it does not delete the row. So the whole of this gate's protective value is the not_approved branch. A missing row is the opposite shape: a signature-valid, non-dev token whose (appId, blockId) resolves to nothing is a HEALTHY app -- a row deleted or re-keyed mid-session, blockId drift, an id-minting bug -- so that branch carried all of the false-positive risk and none of the value, while 404-ing a live public endpoint on deploy with no flag to pull. withBlockScope now counts the verdict, logs the ids, and SERVES. not_approved still refuses 403 and lookup_failed still fails closed with a 503; neither changed. The counter's own comment had already said a non-zero not_found rate meant back the gate out, next to a branch that shipped enforcing anyway. The counter is renamed with it, because the name is the most durable claim of the three: civitai_app_block_rest_approval_refusals_total -> civitai_app_block_rest_approval_verdicts_total. Two of its three reasons refuse and one does not, so summing across the label was going to add requests that were turned away to requests that were served. The help text now says which is which. Nothing consumes the old name yet -- it has not shipped. 2. ONE APPROVAL PREDICATE, TWO POLICIES. block-approval.service and block-bridge-auth.service had become two copies of the same three steps: the dev exemption, the appId_blockId lookup, and status === 'approved'. resolveAppBlockApprovalVerdict is now the only place the row is read and 'approved' is compared; assertAppBlockApproved maps the verdict onto its existing TRPCErrors. What is shared is the lookup and the verdict. What is NOT shared is the response mapping, because after change 1 the two callers deliberately disagree: REST serves a missing row, the bridge still answers NOT_FOUND. The predicate also does not catch, so a failed read still propagates on the bridge exactly as before while REST converts it to a 503 in its own wrapper. Consolidating those mappings would have silently moved the bridge on both points. The bridge's observable behaviour is unchanged and its existing tests were not edited to accommodate this. GUARDS. The structural check counted resolveRestApprovalVerdict call lines in block-scope.middleware alone, so its "ONE place" claim could not see a second predicate in another file -- and there was one, the whole time. It is now a repo-wide ledger over every non-test file that resolves an app_blocks row by its (appId, blockId) unique, asserted as a set in both directions, with a rationale per entry and its text-not-a-call-graph limits written down rather than implied. A planted third copy fails it. The bridge's spelling check asserted status !== 'approved' over the whole file, which a docblock quoting that expression satisfied -- weakening the real comparison to !== 'suspended' left it green. It now reads code lines only. ALSO. The docblock sized the closed window as "900s default, 300s settings-scoped, 4h dev" six lines above the claims.dev exemption that leaves the 4h class open by design. Exempting it is correct -- the moderator review sandbox and the owner dev-tunnel both depend on it -- but a reader sizing residual risk from three lifetimes presented as one closed set concludes the largest is closed when it is the one that is not. And the bridge docblock still asserted the REST wrapper had no approved-status gate and that the asymmetry was real and unclosed. True when it was written, false since the gate landed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * docs(app-blocks): state the REST verdict mapping instead of counting it "only TWO of the four non-ok results refuse" counted dev_exempt among the candidates, which is a pass, not a refusal. A reader checking the arithmetic gets a different answer than the code. Enumerate the five verdicts and what each one does instead -- the mapping is the thing worth stating, and it is short enough to state. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(app-blocks): address round-1 audit findings on the REST approved-status gate Five findings, all re-verified before fixing. One of them did not hold as stated and is corrected below. 1. BEHAVIOUR — the not_found log stringified a function on one route. The warn interpolated `opts.endpoint` raw. That option is `AppBlockEndpoint | ((req) => AppBlockEndpoint)`, and on the one call site that passes a resolver (blocks/tools) the line emitted the function's SOURCE TEXT instead of naming the endpoint. It is more than cosmetic: the counter deliberately carries no app_block_id (an unbounded label retained in the Node heap per pod), so this line is the entire attribution mechanism for the one branch the gate serves -- and tools is one of the four no-requiredScope catalog routes, the thinnest-gated half of the set. Fixed by consolidating the resolution already open-coded at the metric site into one resolveEndpointLabel() used by both readers. Watched red: the assertion received endpoint=(req2) => req2.method === "POST" ? ... 2. GUARD — the verdict counter had no real-registry test. Every reference mocked it, so nothing executed the emitter. The emitter is try/catch-swallowed, so a metric-name typo or label mismatch produces ZERO and never throws; a flat zero is also the healthy steady state, which makes an inert counter indistinguishable from a quiet fleet. Its two siblings each have such a test and one of their docblocks names this exact failure class. Adds a scrape-level suite modelled on those siblings: exact metric name, exact label key, all three label values, the declared-labelNames cardinality bound, and the poisoned-registry guard with its reachability control. Three mutants, all killed on their own assertions -- renaming the metric (10 failed), declaring the label as 'verdict' while the emitter passes 'reason' (9 failed, silent zero confirmed by "expected +0 to be 1"), emitter made inert (9 failed). 3. POLICY — lookup_failed now fails closed only where there is exposure. It previously 503'd all 13 wrapped routes. That read is NEW on this surface (before the gate the catalog routes made no DB read at all), so failing every route closed does not restore a previous posture -- it introduces a coupling from all 13 to one replica, turning a blip into a fleet-wide simultaneous 503. Same argument the not_found branch already won, applied to the other verdict that is not a takedown. not_approved, which carries all of the gate's value, stays mandatory and is not opt-outable. New per-route opt `onApprovalLookupFailure: 'serve'`; absent means 503, so a route added later fails closed by default. Declared on the four unscoped catalog routes plus models/[id]. THE AUDIT'S SUGGESTED DERIVATION DOES NOT HOLD. It proposed deriving the split from whether a route declares a requiredScope. models/[id] DOES declare one ('models:read:self') and is nevertheless the clearest no-exposure route in the table -- dual-auth, so an anonymous caller already gets the same body. So the proxy inverts on the most important entry and is not used. Nothing else in the code separates the sets either, so the declaration is explicit and guarded by a set-asserted ledger in both directions with a rationale per entry, plus a CROSS-LEDGER check: every serve route must be classified 'READ' in the existing REST_ROUTE_RATIONALE. That is what mechanically keeps tip (SPEND) and the two WRITE routes out -- verified by mutation, including the harder case where the author also adds a plausible ledger entry. Middleware mutants, all killed: opt-out ignored, opt-out inverted, and the catastrophic one where it leaks into not_approved. Companion cost: the lookup-failure warn fired once per affected request, so a replica incident was a log-volume event at full REST rate. Now THROTTLED -- time-based, not sampled, because a 1-in-N sampler drops the first occurrence (N-1)/N of the time and "when did this start" is what the log is for. First failure logs immediately, then at most one line per 60s window, each carrying droppedSinceLastLog so the rate stays recoverable. Same shape and field name as the per-pod limiter in server/logging/trpc-serialize-log. The COUNTER is untouched and unthrottled -- it remains the alerting signal, and it still fires on serve routes, so opting a route out never opts it out of the signal. 4. COMMENT — the module's stated reason for existing was falsified by its consumer. block-approval.service's docblock said it is a separate module because a top-level dbRead import in the middleware would make a working Prisma client a load-time prerequisite for its pure routing helpers. But the middleware imports this module STATICALLY and this module imports dbRead statically. Reproduced with a static import-graph walk (import type and await import excluded): at HEAD there is a 3-hop path middleware -> approval service -> db/client, and with that one import line stripped there is NO path (37 files reached). Both arms moved, so the walker discriminates. The claim is corrected rather than the architecture: runtime impact is low and the test-seam benefit is real and is now stated as the whole case. Left as it was, it tells the next author that inlining the predicate would cost something it would not. 5. GUARD — the third and FOURTH instance of the docblock-satisfaction shape. A whole-file toMatch is satisfiable by the prose that describes the check rather than by the check. The two assertions below it had already been hardened to code-lines-only for exactly this reason, with the measurement in the comment. The named instance was non-vacuous only by luck of punctuation -- the docblock's one occurrence is followed by a backtick, which \s* cannot bridge to a paren -- so one ordinary future sentence re-inerts it. A FOURTH instance was found while fixing it: the same regex over the same file in no-unguarded-block-rest-token, on the identical luck. Both are now filtered, as is the sibling BlockRevocation.isRevoked assertion, via a documented codeLinesOnly helper recording all four measured instances. The matrix that makes this concrete: with the bridge's approval delegation gutted from the code and the name surviving only in a docblock, the OLD whole-file form passed 37/37 while the hardened form fails 2 assertions. Gates: typecheck 0 errors; test:lint-rules 567 passed; the middleware, blocks, metrics and api/v1/blocks suites 937 passed across 50 files. Full unit run 3 failed / 40989 passed -- the 3 are eventloop-watchdog.capture, byte-identical to base along with the module it tests, importing nothing touched here, and failing the same way in isolation (a worker-thread CPU-ratio classifier). * test(app-blocks): pin that serving a lookup_failed does not bypass the scope check `v1/models/[id]` is a SCOPED serve route -- it declares both `requiredScope: 'models:read:self'` and `onApprovalLookupFailure: 'serve'` -- so that combination is live in the tree, and it is the one a reader is most likely to misread as "the opt-out lets the request through". It does not: the opt-out decides ONE branch of the approval gate, and the per-scope authorization check plus enforceContextBinding still run afterwards. Nothing asserted that, and the existing serve-arm test uses an UNSCOPED route, so the property was untested in both directions. Adds the pair -- scope missing is still 403 while the approval status is unknown, and the same route does serve once the token carries the scope. The second half is what stops the first passing if the opt-out stopped working altogether. Mutation: widening the scope-check condition to `opts.requiredScope !== undefined && opts.onApprovalLookupFailure !== 'serve'` (i.e. serve bypasses authorization) fails exactly the new case, on its own assertion -- 1 failed | 44 passed. Unmutated: 45 passed. * fix(app-blocks): address round-2 audit findings on the REST approved-status gate Four findings, one of them a merge blocker. Each was re-verified by mutation before being fixed, and each fix is demonstrated by the same mutation now failing. Counts are from real runs. F1 (BLOCKER) — the opt-in ledger detected a single-quoted literal, so the Buzz-spend route could opt in past all three guards. LOOKUP_FAILURE_SERVE_RE pinned the VALUE as /onApprovalLookupFailure\s*:\s*'serve'/. Nothing else in the stack cares about quote style: the runtime check is `opts.onApprovalLookupFailure !== 'serve'`, a plain string comparison, and tsc accepts double quotes because the literal type is identical. This was the sole detection point — the both-directions set assertion, the cross-ledger check, the wrapped-route check and the hardcoded fail-closed list all derive from it. Measured before: adding `onApprovalLookupFailure: "serve",` to blocks/tip.ts — a real irreversible Buzz transfer — gave 21/21 PASS. Not one guard fired. Measured after: 3 failed | 21 passed (24). Fixed by deriving the population on the bare identifier, so any spelling of any value forces a ledger entry. A wider quote-class regex was rejected: it still enumerates spellings. Added a control over five spellings (single, double, template literal, bare const, padded). F2 — the cross-ledger check did not exclude viewer-scoped reads, though the option's own docblock states that it should. The rule is "do not add it to a route that spends, writes, or discloses anything scoped to the viewer". The check tested startsWith('READ ') on a prose prefix, which expresses the first two clauses and says nothing about the third. Three routes classified as reads are squarely viewer-scoped and were admissible; the hardcoded fail-closed list was missing all three. Measured before: opting collections/[id]/index.ts in — which returns the viewer's PRIVATE collections when the token carries collections:read:private — gave 21/21 PASS. Measured after: 2 failed | 22 passed (24). Fixed with a derivation rather than a longer hand-list: the exposure classification is now an enumerated field (SPEND, WRITE, READ_VIEWER_SCOPED, READ_APP_SCOPED, READ_PUBLIC) read by identity, so the third clause is closed for every route at once rather than for the three that were noticed. The hand list is kept as an independent second statement and is now set-asserted in both directions against the derived set, so reclassifying a route to get the option requires a paired, visible edit. Verified: reclassifying tip.ts to READ_PUBLIC alone fails that assertion. This narrows what may opt in; it does not widen it. The five current serve routes are unchanged and are exactly the five READ_PUBLIC entries. F3 — the reason label no longer determines the outcome, and three artefacts still said it did. lookup_failed is route-dependent: refused on 8 wrapped routes, served on 5. The counter carries only reason, deliberately, for the cardinality bound — so no aggregation of it can recover the refused-vs-served split. The stale claim and its remedy ("always split by reason") were corrected in the metrics test docblock, in the metric's own help string, and in the PR description. What replaced it is stated at the scope actually established: the split is recovered structurally from the machine-checked route ledger, and only correlated — never joined — against the sibling RED counter's endpoint label. That cross-reference is marked as read from the middleware source rather than exercised by a test, because the middleware suites stub res.on as a no-op. F4 — the bridge copy of codeLinesOnly had no positive control. Replacing its body with `return source;` left the file at 21/21. The REST copy is controlled and the same mutation fails there, so the asymmetry was measured. The helper is deliberately duplicated rather than shared, which is what makes the REST control non-transferable, so the bridge copy gets its own. Measured after: 1 failed | 21 passed (22). Also — reconciled a two-comment contradiction in api/v1/models/[id].ts. The cache-bypass comment said block-scoped responses "may be identity-bearing / differ from the pure-public body" while the new opt-in rationale says the body is identical. The code agrees with the new claim: browsingLevel is region-derived, isBlockScoped is read at exactly one place, and both branches call the same builder with the same arguments. The bypass is restated as a standing invariant rather than a description of a current divergence, and the coupling is made explicit — if that branch is ever made to differ, the opt-out must be removed in the same change. Gates, counts not exit codes: - typecheck: 0 type errors - test:lint-rules: 40 files, 571 tests passed - middleware + blocks + metrics + api/v1/blocks suites: 38 files, 716 tests passed - suspended-app-rest-refusal.test.ts (real JWT, the money path): 5 passed Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(app-blocks): close two fail-open spelled guards the F1 sweep turned up The round-2 brief asked for a spelled-guard sweep of the two structural guard files after F1. It found more of the same class. Two are fail-OPEN, are in the REST guard's own core assertions, and were verified by mutation here rather than taken on the sweep's word — those are fixed. The rest are reported, not fixed. 1. THE RELATIONSHIP check read the RAW file, so a COMMENTED-OUT wrapper satisfied it while the bare handler was exported. // export default withBlockScope(baseHandler, { endpoint: 'me', ... }); export default baseHandler; Measured on blocks/me.ts: 24/24 PASS. No token verification, no revocation check, no approved-status gate on a live route — and the guard whose stated purpose is exactly this regression (#2 in its own header docblock) said nothing. The population half already stripped comments; the relationship half did not, so the two disagreed about what counts as code. Fixed by routing it through codeLinesOnly. Measured after: 1 failed | 25 passed (26). Added a control that pins both readings of the same fixture, so the raw-file form can never be restored silently. 2. The one-place predicate check counted LINES, not calls, and did not strip comments. The middleware names resolveRestApprovalVerdict four times — an import, the real call, and two comments — and the check returned 1 only because none of the comment mentions happens to be followed by an open paren. That is the same luck-of-punctuation the bridge sibling records as its own instances (3) and (4), on an assertion whose failure message says "exactly once". Both directions were wrong: a future sentence writing the call with an argument list would have made it a false RED, and with such a sentence present, deleting the real call would have left the count at 1 and the gate gone, GREEN. Measured after, both arms: a prose mention alone no longer fails (26/26); the gate deleted with a prose mention left behind now fails (1 failed | 25 passed). Counting regex matches rather than matching lines also fixes a ternary with two calls on one line scoring 1. NOT fixed here, reported instead — each needs its own verification round and none is a merge blocker: - bridge PROC_RE pins the procedure spelling to `*[Pp]rocedure`, so the canonical tRPC `t.procedure`, a quoted key, or a factory-built proc falls out of the population root. Backstopped only by a >50 magnitude control against a measured 73. - bridge GUARD_CALL_RE runs on raw chunk text, so a comment naming the guard with an argument list can satisfy the file's central RELATIONSHIP assertion. - bridge `scan(read(GUARD)).direct === 1` is unfiltered; confirmed by inspection that the guard file names verifyBlockToken four times and counts 1 only by punctuation luck. - the opt-out identifier regex repaired in F1 still does not see the option arriving through an object spread from another module. - bridge RESERVED_WORDS is unpinned while its neighbour MODULE_EXEMPTIONS is, so the anti-suppression pin is evadable via the adjacent set. - bridge `status === 'approved'` and friends are satisfiable by a STRING LITERAL — codeLinesOnly strips comments, not strings. The file already owns stripNonCode, which would. Gates, counts not exit codes: - typecheck: 0 type errors - test:lint-rules: 40 files, 573 tests passed - services + metrics + middleware + api/v1/blocks: 475 files, 6882 passed, 9 skipped Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
briant
|
3ad5538813 |
fix(generation): stop substituting 512x512 when a source image fails to load
Sending a generated image to a workflow read its size by loading it, and any load failure silently became 512x512. Hires-fix derives its output size from the source dimensions, so a portrait source came back squared and the user was still charged. A source whose size can't be read now raises an error instead of reaching the form. The load retries twice first, and the source-image input's size check no longer marks a failed URL as verified, so it can retry. Refs CU 868m4r5dw. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
Justin Maier
|
7739bc26de |
fix(models): don't offer the resource search before the saved list loads (#4828)
* fix(models): don't offer the resource search before the saved list loads The "Manage Suggested Resources" modal rendered its search dropdown while its own associations query was still in flight. The local list starts empty and the effect that seeds it from the fetched data only assigns when the list is empty, so a selection made before the query resolved left the list holding one row and the saved rows never arrived. `setAssociatedResources` is a set-replace, so saving that deleted every other association on the model, with no warning and no undo. Gate the dropdown on the query having succeeded. Adds the first test file for this component: the absence assertion is anchored by a positive control that the same stub renders once the query resolves, and the payload test starts pending so the seeding effect is actually exercised rather than satisfied by the initial state. Closes 868m47y3u Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0128YedQgKr6fbRnDHpPW7kF * fix(models): correct a stale suggested-resources list instead of refusing to Review of the render guard found a second route to the same delete-everything, with the query already resolved throughout. Close the modal before a save's mutation resolves and `invalidate`'s default `refetchType: 'active'` finds no observer, so the next open snapshots the pre-save cache and the refetch lands after `useState(data)`. Seeding only when the local list is empty then refuses that correction forever, and the next save is a set-replace over a payload missing the row it never saw. Seed whenever the user has not edited, and depend on the query's own array rather than the `?? []` fallback, which is a fresh array per render. Also change the dropdown's predicate from `isSuccess` to whether the saved list was delivered — an error on a background refetch keeps the rows but flips status, which pulled the search box out from under an open edit — and give a failed load its own copy, so it no longer falls through to "no resources yet, search above" pointing at a box the guard had just removed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0128YedQgKr6fbRnDHpPW7kF * fix(models): withhold the resource search while the saved list is being corrected Two review lanes independently found a third route to the same set-replace deletion. A save whose invalidate finds no active observer leaves the cache stale without refetching, so reopening the modal shows rows while a refetch is still in flight. An edit started in that window sets `changed`, the seeding effect then refuses the correction, and the save deletes the row the modal never saw. Withhold the dropdown while the list is being refetched, so an edit cannot begin against a list that is about to change. The fixtures now carry every field the component's guards read. They did not, and that was a ratchet: reverting the gate to `isSuccess` reddened tests only because `isSuccess` was undefined in the fixtures, so the obvious later tidy — filling the shape in — would have turned that revert green and restored the deletion. The new refetch-failure test pins the predicate positively instead. Also pin the tradeoff when a correction lands on top of an edit in progress: the edit wins and the correction is dropped, which is only acceptable because the refetch gate stops the user reaching that state. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0128YedQgKr6fbRnDHpPW7kF * fix(models): make every edit inert until the list on screen is the saved list Review found two more routes to the same set-replace deletion, and the previous guard covered neither. `isFetching` is `fetchStatus === 'fetching'` and nothing else, so a background refetch that FAILS goes back to idle while `data` is still the last successful, pre-correction payload — rows that look healthy and are not what is saved. Offline-paused is the same shape. `isStale` is set by `invalidate` and cleared only by a successful fetch, so it covers the in-flight, failed and paused cases in one clause. And the gate only ever covered the add affordance. Removing a row, dragging one, or ticking link-back all set `changed` too, so a removal during an in-flight correction reproduced the deletion one affordance over. All four now ask one derived question, and the row controls are visibly disabled with a line saying why rather than silently doing nothing. The dropdown is disabled rather than unmounted: a remount rebuilds the InstantSearch tree and re-issues a ~158 KB search, and a box that vanishes tells the user nothing. Seeding keys on `dataUpdatedAt` rather than on the query's array, which is reference-stable only by grace of structural sharing. Two test corrections matter more than the code. The failed-refetch case was asserted BACKWARDS, with a comment calling it the safe case — a passing test whose comment argues the hole open. And the fixtures now say why a failed refetch keeps `data` while leaving `isSuccess` false, because "correcting" that to look tidy would make the rejected predicate pass. Deleted the array-identity test rather than ship it: it passed with and without its mutation. Under that fixture the component renders exactly once, so the effect never re-runs and the harness cannot observe the loop it claimed to pin. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0128YedQgKr6fbRnDHpPW7kF * fix(models): stop the frozen rows being draggable, and explain an empty list Two review lanes found the same pair independently. `SortableItem`'s `disabled` prop never reached dnd-kit — it only changed the cursor. So rows stayed fully draggable while every button beside them was greyed out, and a reorder sets `changed`, which permanently stops the seeding effect adopting a correction. The handler early-return was the only control, not the defence-in-depth it looked like. Passing `disabled` into `useSortable` also makes `SortableGrid` behave the way its own prop advertises, and it gives the guard an observable — dnd-kit publishes it as `aria-disabled`. The empty-list branch got neither the explanation nor a truthful instruction: an empty array is truthy, so a model with no saved resources whose list could not be verified rendered "search above to add one" at a disabled box, with the paused notice living inside the other branch. The notice is hoisted above the split and the instruction is conditional. Save is now gated on `canEdit` as well as `changed`. Four affordances going inert while the button that writes them stays live is the same gap one level up; it is unreachable only because this component's own save is the key's sole invalidator, which is the kind of coincidence this PR has already been bitten by twice. The paused notice said "for a moment", which is false for a refetch paused offline — a state `isStale` deliberately covers. Tests: the notice assertions now name WHICH copy, since both branches contained the phrase being matched and an inverted condition stayed green. Adds an empty-unverified fixture, which nothing covered in either direction, and pins that an unticked link-back chip sends no reciprocal. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0128YedQgKr6fbRnDHpPW7kF * test(models): pin the paused notice's delivery guard, and drop a dead assertion The notice is gated on the saved list having been delivered; without that it renders above the loader during a first load, and above "couldn't load" when there is no list at all — two contradictory explanations at once. Nothing asserted its absence in either state. Reverting that guard now reddens both. The forced-selection test asserted Save was disabled and said `changed` was why. That stopped being true when Save gained `!canEdit`: the fixture has `canEdit` false, so the assertion held whether or not the handler refused anything. Dropped rather than left reading as coverage; the row-absent and mutate-not-called assertions are what discriminate there. Also corrects a comment that named `SortableGrid` as depending on this prop. It has its own local SortableItem and never imported this one — the claim was false, and it was on the file whose next reader will be doing a blast-radius check. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0128YedQgKr6fbRnDHpPW7kF * test(models): assert disabled by attribute, because toBeDisabled did not fail Building the final control inventory turned up an assertion that could not fail. `toBeDisabled` on the remove button passed with the button's own `disabled` prop deleted — it was reporting the row's `aria-disabled` ancestor, which this branch added when it made `SortableItem`'s prop real. So the assertion started life measuring something and stopped, silently, two commits later. Measured while diagnosing it: on an enabled control neither `expect(el)` nor `await expect.element(locator)` fails that matcher here, so it was not the synchronous form at fault. Asserting the attribute directly does fail, which is the property both forms were supposed to carry. Every disabled/enabled assertion in the file now reads the attribute, and the four affected controls redden again: the remove button, the dropdown, the sortable row, and the derived flag itself. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0128YedQgKr6fbRnDHpPW7kF --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
Luis Rojas
|
10b38da2da |
fix(leaderboard): read collectedCount for writer bookmark scoring
The writer leaderboard was scoring bookmarks off ArticleMetric.favoriteCount, a legacy column no longer written since favorites were deprecated. Real bookmarks are tracked as collects in ArticleMetric.collectedCount, matching how ArticleSort.MostBookmarks already sorts them. This caused all recent bookmarks to score as 0. Since bookmarks are the heaviest term in the scoring formula (√bookmarks * 10), this materially distorted writer rankings. This migration updates the leaderboard query to read from collectedCount instead. No backfill needed; collectedCount already holds the historical totals and will repopulate on the next leaderboard run. |
||
|
Manuel Emilio Urena
|
6d5bdd8c60 |
feat(testing): chatCompletion text-scan spike endpoint (#4830)
* feat(testing): chatCompletion text-scan spike endpoint Submits a raw `chatCompletion` workflow step instead of `xGuardModeration`, so we can measure whether a general instruct model can replace the purpose-built guard model (YuFeng-XGuard-Reason-8B) behind text moderation. Each unknown the spike has to settle is a separate toggle — responseFormat, chatTemplateKwargs, logprobs, model — so they can be probed independently rather than as one pass/fail. Outcome classification keeps refusal, truncation, malformed JSON and missing labels as distinct states. A naive parser reads all four as "no labels triggered", and that conflation is the specific risk of moving to a general model; the raw content is returned for every non-ok outcome so the classification can be checked rather than trusted. Label policy text is an input, never a default in this file — policy text and thresholds must not be committed to a public repo (CLAUDE.md Security §2). Refs CU 868m4de9j. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Ah9zExvcax46sDBVV3YyqW * feat(testing): surface step status and job errors on the scan spike A workflow can reach `failed` without the step producing output, and the reason lives on the step or its jobs rather than at workflow level. Without these fields a fast failure is indistinguishable from an empty model reply — which is how three minutes went into diagnosing a model that simply does not resolve. Refs CU 868m4de9j. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Ah9zExvcax46sDBVV3YyqW * docs(testing): drop internal references from the scan endpoint comments The file is in a public repo, so its comments should carry only what a future editor of this file needs. Removes the tracker id, the build-history aside, the pointer at the policy-export tooling, and the framing that described this as one team's investigation rather than what the endpoint does. Keeps the invariant that matters: label definitions and policy text are inputs, never defaults here. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MKVUjr4zC8myRXE4b65Bjn * fix(testing): route the scan endpoint's 500 through handleEndpointError The catch served `error.message` and `error.stack` straight back to the caller, which the REST error-envelope ledger flags: a serialized error object carries its enumerable own props, and for a driver error those are the table and column — or, for a pg 23505, the offending row value. `handleEndpointError` genericizes the response while still logging the un-redacted, cause-walked error, so nothing is lost for debugging. Also declares the endpoint's `wait` in the runtime-valued wait ledger. It is clamped to a seconds envelope by the request schema, matching the sibling testing endpoint already listed there. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MKVUjr4zC8myRXE4b65Bjn --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
Luis Rojas
|
c5898ea396 |
feat(training): add Kohya maintenance warning
Add a warning alert in the training submission form that appears when users select Kohya as the training engine. The alert informs users that Kohya is no longer actively maintained and may become less stable over time, with a recommendation to use AI-Toolkit instead for more reliable training. The alert uses a yellow warning icon and is displayed conditionally whenever any run in the submission uses the Kohya engine. |
||
|
Luis E. Rojas Cabrera
|
1216d71d16 |
Merge pull request #4781 from civitai/elise/training-studio-polish
feat(training-studio): design pass on the Select step, base-model cards and run list |
||
|
Koen
|
a7b6e324d2 |
fix(feed-shadow): freeze the paged set at the cursor minute only for Newest (#4831)
The Meilisearch path applies the cursor's entry timestamp as a sortAt cut only when sorting by sortAt desc; ranked sorts page a live set. The shadow mapper sent `before` for every sort, so every paged Most Reactions / Collected comparison measured two different sets (overlap 0.19, and empty feed pages whenever the cursor's minute predated the period). Co-authored-by: Koen <koen@civitai.com> Co-authored-by: Claude Fable 5.1 <noreply@anthropic.com> |
||
|
Luis Rojas
|
853384eaee |
fix(chat): linkify civitai.red URLs in messages
civitai.red links were not recognized as valid URLs in chat, so they rendered as plain text instead of clickable hyperlinks. This is a problem because .red domain links are used to share mature content that would be rejected on .com. Add civitai.red to externalRegex with domain boundary validation (?=[/:?#]|$) to prevent false matches on domains that prefix it (e.g., civitai.red.evil.com). The lookahead ensures a path, query, fragment, or end-of-string follows the domain. Export validateLink and renderLink to enable test coverage that verifies the fix will fail on revert. |
||
|
Justin Maier
|
a25de4e1eb |
fix(models): show the New badge beside the paid badge, not instead of it (#4816)
* fix(models): show the New badge beside the paid badge, not instead of it A model that was both newly published and paid-gated lost its New badge. The card had one status slot and #4678 put `isPaidAccess` ahead of `New` in its ternary, so the recency state was computed correctly and then discarded at render. 56 of the 726 models published in the last 24h on prod are gated, and every one of them is affected. Splits the slot in two. The money badge becomes a Buzz bolt rather than the word "Paid" so both chips fit; Early Access keeps its text, since the two words are deliberately not interchangeable. The New/Updated rule was written out three times — ModelCard, ResourceSelectCard and ModelCategoryCard, the last with its own module-scope cutoff beside it — so only one of the three learned about the paid badge. All three now call one helper, the private cutoff is gone, and a guard pins both the rule and the cutoff to a single definition. `ModelCard.browser.test.tsx` hand-listed its mock of `model-card.utils`. A second import from that module would have killed the file at import and reported `Tests no tests`, so it spreads the real module now. Its fixture also set `publishedAt: null`, which is why no test in the file could ever see New compete with the money badge. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WL4HetKe4Q4mWuKFHcq8J9 * test(models): record what the recency guard cannot catch Two probes: a copy-pasted fourth site reddens it, a fourth site derived in its own words does not. Closing that needs a marker matching six unrelated modules, and a six-file name exemption is how the third copy of the paid-gate predicate got written unseen. Narrow guard, limit written down. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WL4HetKe4Q4mWuKFHcq8J9 * fix(models): give the bolt an accessible name, and pin the clause nothing pinned Review round over the badge split. Four things it found: The bolt had no accessible name. `aria-label` sat on a Mantine Badge, whose root is a role-less `div`, and ARIA drops a name from a generic element — so a screen reader that previously heard "Paid" got nothing. `role="img"` exposes it; the Tooltip could not stand in, since Badge renders no tabIndex and it is hover-only. `getModelRecency`'s `lastVersionAt > cutoff` clause was untested. Every fixture put lastVersionAt well past the cutoff, so deleting the clause left the whole diff green — while making every model that ever shipped a second version read "Updated" forever. The consolidation argued the helper is the one thing that has to be right, and half its predicate was unpinned. The guard checked the recency badge's gating CONDITION but not its BODY, so the original bug could be put back inside the badge with the guard still green. It now reads both, and is named for what it checks. The prohibition tests had no non-empty control: a zero from an empty corpus and a zero from a clean one are the same zero. Also: the Buzz bolt goes through CurrencyIcon, as the sale chip beside it in the same row already does; the Tooltip no longer wraps the Early Access arm, whose visible text already names it; and "Paid access"/"Paid" collapse to one spelling. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WL4HetKe4Q4mWuKFHcq8J9 * test(models): close the gate the guard was not reading Second review round, all of it on the tests written in the first one. The recency guard was blind to the bug it is named for. Its regex read only inside the paren group, so `(isNew || isUpdated) && !isPaidAccess && (` — the single-slot precedence written with `&&` instead of a ternary — left `rest` empty and passed. It now reads the run of characters between the gate and the `<Badge` it guards. This matters more than it looks: `component` is ungated in CI, so the browser test that does catch that shape fails no check, and this textual guard is the only gated thing standing in front of it. The corpus control asserted a floor of 500 against a corpus of 4,301, and named a file every detector allowlists away — so it proved the walk reaches somewhere the detectors never look. It now asserts a real floor and two scanned files. The detectors had no positive control: typo either marker and all three prohibitions pass forever over a corpus they can no longer match. Adding one immediately caught a live defect — `\b` had been written into the new regex as a literal backspace byte, so it matched nothing. Repaired, and the file is clean of control characters. Also: `querySelector('svg')` accepted any icon under a title naming the bolt; the accessible-name test read two attributes rather than asking for the name; and `lastVersionAt > cutoff` was pinned an hour clear of its boundary. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WL4HetKe4Q4mWuKFHcq8J9 * test(models): make the missing-role failure fast as well as correct The role query proves the accessible name resolves, but it is a polling locator: on a missing role it can only fail by exhausting the 15s budget. Measured 15071ms to learn one thing. A structural check in front of it says the same thing in 191ms with a named cause, and the role query still answers the question that matters. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WL4HetKe4Q4mWuKFHcq8J9 --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
briant
|
baf6700027 | 5.1.98 v5.1.98 | ||
|
briant
|
ede4510a5f |
feat(generation): store gate rules and generator messages per entry
Gate rules and generator messages move from one JSON array in `system:features` to a sysRedis hash per store (field = id), so saving or deleting one entry never rewrites the rest. Each store migrates itself on first read, save or delete, copying the legacy array with hSetNX before setting its marker; the legacy array stays as a backup. The /moderator/generation-config page now lists compact read-only cards and edits each rule or message in a modal, one save per entry. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> |
||
|
briant
|
b3184e184f |
feat(gallery): hide or unhide a whole post from a model gallery
Adds a confirmed "Hide post from gallery" moderation action beside the per-image one, so hiding a post no longer takes one click per image. The post's full image id list comes from a new post.getImageIds query, since the card may not have loaded every image in the post. ClickUp: https://app.clickup.com/t/868km6kc7 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
briant
|
6b92ccb01a |
docs(prompt-analysis): record that the ideogram guide is live and unmeasured
STATUS.md listed `ideogram` as not deployed, but the orchestrator serves the authored guide byte-for-byte; it went live after 2026-08-06 with no measurement run. It was also sourced from the hosted Ideogram product rather than the open-weights Ideogram 4 the generator now runs (PR #4766), so its Magic Prompt and style-preset advice does not apply. Note that and mark it for revision and measurement. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> |
||
|
briant
|
04e96f4d3f |
fix(form-graph): restore the Enhance prompt button
The prompt:enhance panel was ported but nothing triggered it. Add a PromptLabel helper carrying the Enhance button and use it on the image, video, audio and 3D prompt fields, matching the data-graph form. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
briant
|
fbfaf02755 |
docs(skills): decide when onboarding adds an ecosystem or base model
onboard-generator-model Phase 0 listed cases A-D without a rule for choosing between them. It now settles the kind first, then applies: API-only releases in an existing line add no records; the first API-only model in a line gets an ecosystem; hosted weights compatible with existing resources add a base model in the ecosystem; incompatible ones get a new versioned ecosystem. add-ecosystem now documents its base-model-only mode, which case B relied on, and stops for API-only releases that need no constants change. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
briant
|
20a8248633 |
feat(generation): widen GPT Image v2/v2.5 aspect ratios
v2 and both 2.5 builds take free width/height, so give them their own list (21:9 through 9:21, sized within fal's limits: multiples of 16, max edge 3840, ratio <= 3:1, 655,360-8,294,400 px). v1/v1.5 keep the three sizes OpenAI's size enum accepts. Applied identically in the data-graph and form-graph lanes, with parity rows for a gpt2-only ratio on both variants. The form-graph image and video forms ignored the aspect-ratio meta's priorityOptions; they now honour it with the same slice fallback as the generation_v2 form. ClickUp: 868m42g0m Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
briant
|
07f9464c38 |
fix(form-graph): restore trigger-word chips under the generator prompt
The form-graph image, video and audio forms received meta.triggerWords but never rendered them, so LoRA trained words stopped showing in the generator (ClickUp 868m4r5du). Extract the data-graph form's prompt shell and trigger word strip into PromptEditorShell and use it in all three form-graph forms; this also restores the gen:prompt tour anchor on that lane. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
briant
|
fd05fcac06 |
fix(seo): emit a placeholder JSON-LD author for deleted users
Model pages emitted aggregateRating without an author when the creator was deleted or had no username, which Search Console flags as a critical Review snippets error (missing field "author"). Deleted/username-less authors now render as a "Civitai user" Person with no profile URL, on model and article pages alike. ClickUp: 868m47mg9 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
Briant Diehl
|
2b3b4e9d36 |
Merge pull request #4817 from civitai/fix/form-graph-source-image-badges
fix(form-graph): restore source-image annotations, AI-meta badges and… |
||
|
briant
|
04c1b65667 |
fix(form-graph): restore source-image annotations, AI-meta badges and metadata actions
The form-graph port rendered source images with a bare ImageUploadMultipleInput, dropping what generation_v2's ImagesInput wired up: the moderator "AI Meta" / "No AI Meta" badges, graph annotations (upscale "Excluded" / "No dims"), the per-image metadata extraction + apply action, drawing on img2img:edit, and the square/wrap layout. Extract the annotation logic into useSourceImageAnnotations (shared by both forms) and add a form-graph SourceImagesInput that reads workflow, annotations and resources off the store. Used by the image and video forms. ClickUp: 868m4r83b Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
Koen
|
89a1dee64e |
feat(feed): shadow the image feed against the PostgreSQL feed service, behind a Flipt flag (#4739)
* Shadow the image-feed search against the candidate feed service Behind the sysRedis hash system:feed-shadow (sampleRate, until, timeoutMs, maxInflight) and FEED_SHADOW_URL, a sampled share of getImagesFromSearch calls is mapped to the candidate feed's query and fired in the background with a timeout and an inflight cap. Each comparison lands in ClickHouse feedShadow: both id lists and latencies, overlap, top-10 overlap, first order break; shapes the mapper cannot express are recorded with a skip reason so coverage is measured too. The viewer's response is never touched. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> * Serve the image feed from the feed service behind a Flipt flag feed-service-primary, evaluated per request with the user's context, makes the PostgreSQL feed service answer getImagesFromSearch instead of Meilisearch. The feed selects and orders the page; Meilisearch hydrates exactly those ids through the same search function, so every post-filter still applies. Pages continue with a feed:<key>:<id> cursor that the Meilisearch path reads as a restart. Anything the feed cannot answer (unmapped shape, timeout, error) falls back to Meilisearch per request, with the shadow comparison as before. Everyone outside the flag is unaffected. FEED_SHADOW_URL becomes FEED_SERVICE_URL. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> * Leave impossible-id feed queries to Meilisearch Prod sends `tags: [0]`. The mapper dropped the id and emitted no tag filter at all, which turns a query Meilisearch answers with an empty page into a request for the whole feed. Skip those instead, so shadow records them and the primary path falls back. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * Implement error handling for data hydration Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> * Write feed shadow rows through the tracker, not an in-process buffer The buffer, its flush timer and its row threshold duplicated batching that already happens twice over: the shared ClickHouse client sets async_insert, so the server accumulates rows and forms parts itself, and Tracker.trackMany is the helper the last three new tables use for exactly this. One row per comparison now goes straight through it. Direct insert, like entityChanges and chatAudit, so table DDL stays the only dependency — track would need a route registered in the tracker service as well. The table name and row type move to a constants module, matching chat-audit.constants, so the tracker can name the row without importing the service back. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Fable 5.1 <noreply@anthropic.com> Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> Co-authored-by: Koen <koen@civitai.com> |
||
|
ZacxDev
|
4daf83e288 | chore(moderator): release moderator-v0.0.62 moderator-v0.0.62 | ||
|
Zachary Lowden
|
790a2eb765 |
feat(moderator): decode browsingLevel, tab the feedback detail panel, add an attachment lightbox (#4809)
* feat(moderator): decode browsingLevel, tab the feedback detail panel, add an attachment lightbox Display-only pass over the moderator /feedback triage queue. No write path, pagination or sorting changes. 1. browsingLevel renders as labels, not a raw number. It is a BITMASK — the values live rows carry are 1, 3, 7, 28, 30 and 31, so a direct browsingLevelLabels[value] lookup is right for 1 and silently wrong for the other five. Decoded through the shared parseBitwiseBrowsingLevel, via a per-(area, key) formatter registry rather than a global key match, because browsingLevel means a bitmask on bitdex-image-feed and nothing anywhere else. 2. FEEDBACK_AREAS moved into @civitai/shared with a re-export shim at the old main-app path, and the moderator app's hand-written FEEDBACK_KNOWN_AREAS mirror is deleted. That mirror's own comment named this fix. 3. The detail panel is now Message / Context / Attachments / Triage / Issue, with the tab in the URL. Triggers are links, not bits-ui Tabs, so the no-JS surface survives. Refusals render once, above the tab strip, naming the owning tab. 4. Attachments open in a lightbox with Esc, focus restore, arrow paging and the provenance caption carried into the large view. It is fed only from the already IMAGE_KEY-filtered context, never from raw row.context. 5. Narrow widths scroll rather than collapse; the ultrawide cap already exists in +layout.svelte and is not duplicated. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QhF8uK5A5xoKjYV3qFNNAR * fix(moderator): key the attachment each-block by index, not by image id `splitContext` deduplicates `images` among THEMSELVES; nothing compares `screenshotId` against them. So a row where the reporter attached the same file the opt-in page capture produced carries the id twice, and Svelte THROWS on a duplicate `{#each ... (key)}` in production as well as in dev - making that report permanently unopenable. That is the exact failure `splitContext`'s dedup exists to prevent, reintroduced one layer up by folding the two fields into a single list. Index keying is safe here: the list is derived from immutable row data and never reorders. Both frames are kept rather than collapsed - they are two different claims about the same file, and the captions are what say so. A test pins the PRECONDITION (ids in the item list are not unique), which is what makes the keying decision necessary; the template keying itself is not reachable from the node test tier. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QhF8uK5A5xoKjYV3qFNNAR * fix(moderator): control the lightbox dialog with a function binding bits-ui declares `open` as `$bindable` and WRITES to it on every interaction (bits-ui@2.18.1, dialog/components/dialog.svelte) - Escape, an overlay click and the close button all land there. Handed a plain `open={...}` prop, that write becomes a child-local override which Svelte only discards when the parent yields a DIFFERENT value, so any close the parent does not observe leaves `openIndex` set against an already-closed dialog and re-clicking the SAME thumbnail then does nothing. FeedbackFilters.svelte already records this exact hazard about the same primitive family; the lightbox now follows it. The getter makes the parent the only source of truth and the setter is the single place a close becomes state. Verified by a full `vite build` of apps/moderator (7,926 modules, SSR + client, prerender analysis) - the only gate in this app that compiles .svelte at all. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QhF8uK5A5xoKjYV3qFNNAR * fix(moderator): let bits-ui own the lightbox focus restore, and key paging by index Two corrections to the lightbox, both found by reading bits-ui rather than assuming. 1. The hand-rolled focus restore is removed. It rested on the premise that a dialog with no `Dialog.Trigger` gives the library nothing to restore to, and that premise is FALSE: focus-scope-manager.js:14-26 captures `document.activeElement` at `register()` - which `mount()` calls BEFORE `#handleOpenAutoFocus` - and focus-scope.svelte.js:72-90 focuses it again on unmount, guarded by `document.contains` and a try/catch. No trigger is involved; whatever had focus when the scope opened gets it back, which is exactly the thumbnail button that was clicked. Preventing that to run our own replaced a guarded implementation with an unguarded one whose capture point raced the library's `requestAnimationFrame` focusFirst. The comment asserting otherwise was a false claim and is replaced by one citing the source. 2. `{#key}` now keys on the index, not `current.id`. Ids are not unique - same reason the thumbnail each-block is index-keyed - so paging between two frames that share an id was a no-op. Verified by `vite build` of apps/moderator (7,926 SSR + 7,697 client modules). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QhF8uK5A5xoKjYV3qFNNAR * refactor(moderator): cut the filter-formatter registry, put attachments back beside the message Two operator decisions on the feedback display pass. 1. The per-(area, key) formatter registry is gone. It was a two-level ReadonlyMap with prototype-key hardening on both axes, serving exactly one entry. Measured this round: the live feedback-context builders are appsStoreFeedbackContext.ts (kind/category/sort/query) and FeedbackDrawer.tsx (path only) - neither writes browsingLevel - and BitDex was decommissioned 2026-09-01, so the second axis served only the 23 historical bitdex-image-feed rows. Replaced by two === checks in formatFeedbackFilterValue, which is still the single entry point the panel calls. No keyed lookup on an untrusted key survives, so there is no longer a prototype chain to harden; if one ever comes back it must be a Map, and the file says so. The bitmask decoder and every one of its tests are untouched, including the 1/3/7/28/30/31 production fixtures, the +<bit> unlabelled-bit case, the int4 bound and the null fall-through. A new hand-typed table pins the exact rendered output for every (area, key, value) triple the panel can be handed today, so the removal is a refactor and not a behaviour change. The prototype-key cases are kept but relabelled as an invariant guard: with the registry gone they no longer have a live hazard behind them. 2. Attachments are visible with the report message again. Message + attachment is the core triage pairing and the tab split cost two navigations for something that previously cost none; the containment it bought was marginal, because thumbnails already only mount once a ROW is expanded. Shape chosen: no attachments tab at all - FeedbackAttachments renders on the default Message tab, below the message. An old ?tab=attachments link degrades to the default via feedbackTabFromUrl, which is now where the attachments are. Everything load-bearing survives: tab triggers stay links (the no-JS surface), the refusal banner stays above the strip with no auto-switch, the lightbox is still fed only from feedbackAttachmentItems(splitContext(row.context)), the {#each} stays index-keyed, and the Dialog open stays a function binding. Mutation checks on the replacement guard, each restored byte-identical after: - area half dropped -> 9 failed / 43 passed; own assertion "leaves the SAME key alone under a different area", expected { text: 'R, X, XXX', title: '28' } to deeply equal { text: '28', title: null } - key half dropped -> 6 failed / 46 passed; own assertion "leaves a different key alone under the decoded area", same shape The key-half run found a real gap first time round: every fixture on the decoded area was a string, and formatBrowsingLevel rejects non-numbers, so none of them could reach the decoder at all and none could see the mutant. A numeric fixture was added to make that assertion reachable. Gates: typecheck 0 errors; svelte-check 9483 files / 0 errors (negative control went red); apps/moderator 61 passed | 1 skipped (62) / 844 passed | 40 skipped (884); main-app feedback suites 7 files / 159 tests passed; vite build 7926 modules transformed, done; prettier clean with a named control file still warned; eslint rc=0 with a prefer-const negative control going red. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(moderator): stop a tab click destroying typed text, hiding the newer refusal, and following a row Round-1 audit findings on the feedback detail panel. All three share one cause — a tab is a destructive navigation, and the panel still reasoned as if both forms were on screen at once — but they need three separate fixes, not one. F1 A tab switch silently destroyed the operator's typed triage note. The note box was an unbound value= and the issue title/summary/bugId were uncontrolled, so the text lived only in DOM nodes that the {#if activeTab} chain destroys. Both drafts now live in FeedbackDetail, which survives the navigation, and are bind:value-d. The promote draft is handed down as a $state proxy. The note is re-seeded from the reloaded column in onSuccess, so reset:false keeps its meaning without depending on the bound expression changing. The trade-off (a draft now shadows another moderator's concurrent edit to the same column) is written down at the declaration rather than left to be discovered. F2 Two refusals could be live at once and the older won, hiding the newer. Each form's onSubmit now clears the other's error, so at most one is live; and the selection moved into $lib/feedback-refusal.ts, where it prefers the ACTIVE tab's own form and is testable. The old comment claimed both errors "cannot be" set and its argument did not reach the case; that claim is retracted in the new docstring rather than replaced with another one. F3 ?tab= was sticky across rows, so a report could open straight onto the triage buttons with its text off-screen. New feedbackOpenHref() is the single choke point for opening a row and deletes ?tab=. Both call sites now use it — rowHref, and FeedbackPromote's siblingHref, which built its URL by hand and only renders ON the Issue tab, so every sibling link opened the next report onto the one panel showing none of what that reporter wrote. F5 The (area, key, value) table's docstring claimed to enumerate "every triple the panel can actually be handed today". It cannot: filters is schemaless JSONB and area is free text, so the population is open. Sentence narrowed to what the list covers; the two shapes the audit read off production (a period key, 'Most Collected'-shaped sort) added as rows. Tests: +31 in the node tier. feedback-refusal.test.ts is real behavioural coverage of the selection rule; feedback-panel-tripwires.test.ts is explicitly labelled TRIPWIRES, NOT COVERAGE — normalised source-text pins over .svelte files, which have no test tier in this app, walkable by rewording and unable to certify that anything works. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(moderator): bind the promote draft, and retract the unreachability claim a second time Round-2 audit findings on the feedback detail panel. 1. The hoisted draft raised `ownership_invalid_mutation` on every keystroke. `FeedbackPromote` mutates `draft`, which `FeedbackDetail` passed as a plain prop; Svelte's dev ownership validator wants a SETTER on the props descriptor (`is_bound_or_unset`, svelte@5.56.3 internal/client/dev/ownership.js:71-80) and a getter-only prop has none. `$bindable()` in the child plus `bind:draft=` in the parent; `promoteDraft` becomes `let` because `bind:` over a `const` is the compile error `constant_binding`. Measured in a compiled two-component repro: 2 keystrokes gave 2 warnings unbound, 0 bound. Production was never affected. The measurement needs `--conditions=browser --conditions=development`, as two separate flags: `--conditions=browser` alone resolves esm-env to production, `DEV` is false, and the count is 0 whatever the code does. 2. The "the two refusals can no longer both be live" claim is FALSE, and it failed the same way the sentence it replaced did — a true statement about one ordering read as a claim about another. "Every submit starts by clearing its counterpart" does not order a submit start against a previously started submit's response. The reachable path, walked through the sources: a triage submit starts; the tab links are not disabled, so the operator moves to Issue; the branch is destroyed but `use:enhance`'s `destroy()` only removes the submit listener (kit@2.66.0 runtime/app/forms.js:227-231) and nothing aborts the AbortController it handed to the submit hook; they submit promote; both responses land and set both errors. Behaviour is already correct, so this is a prose fix. Aborting was considered and rejected: `enhance` returns early on AbortError without invoking the callback, so a committed write would produce neither a confirmation nor a refusal. 3. A tripwire titled "never writes the open param by hand" asserted one spelling and was false in the file it scanned. Widened to the property worth holding — clearing by hand is fine, SETTING an id must go through `feedbackOpenHref` — over the whole route directory rather than a hardcoded pair, which is how the existing third site (`FeedbackFilters.svelte`) had gone unscanned. Positive control is per pattern. 4. Recorded both halves of the cross-clearing trade, including the half it cost: a standing promote refusal is discarded by an unrelated triage save that SUCCEEDS, leaving a pre-filled form with no explanation on screen. 5. The success re-seed no longer discards text typed while the request was in flight. The decision is `reseedTriageNote` in `$lib/feedback-drafts`, a pure function with its own tests, because the panel has no test tier. 6. `feedback-filters.test.ts`'s `reachable` table renamed `pinnedRenderings`: the identifier asserted in one word the completeness its own docstring had already retracted. Also recorded the precondition nothing stated: draft survival across a tab click needs the row to come back in `data.items` under the same id, so a concurrent status change that evicts it from the active filter destroys the panel anyway. Gates: svelte-check 0/0 with a negative control taken to 5 errors in 3 files; apps/moderator suite 875 -> 884 passing (63 -> 64 files); vite build green; prettier clean against a deliberately misformatted control in the same invocation; eslint clean against controls using rules enabled for each path (`no-debugger` for .svelte, where `prefer-const` is off; `prefer-const` for .ts). Every new or changed guard was mutation-checked and killed by its own assertion — including one that SURVIVED first time, because it matched its own docstring prose rather than the template. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(moderator): strip comments before scanning, and sweep the two recurring comment shapes Round 3 of the audit ladder, and the last one. The two reported findings are one-sentence corrections; the work that matters is sweeping the two shapes that have each now recurred, at every site rather than only where reported. Shape A — a text pin satisfied by PROSE rather than CODE (3 instances). Fixed structurally instead of one string at a time: `source()`/`componentSource()` in feedback-panel-tripwires.test.ts now strip markup, block and line comments before scanning, so every assertion in the file is a claim about code only. The stripper carries two controls — a synthetic fixture covering all three comment syntaxes, and a real-data one that watches `reset: false` in FeedbackDetail.svelte go from 5 witnesses to 2 and `bind:draft={promoteDraft}` from 2 to 1. Neutering the stripper reddens both. F-R3-2: the `re-seeds the note ... on a successful save` pin asserted a bare `row.triageNote ?? ''` (4 witnesses, 2 of them prose) and survived deletion of the whole re-seed. Repointed and retitled to `seeds the note box from the stored column`, which its one code witness actually supports and which was pinned nowhere before. The re-seed itself stays covered by the sibling `re-seeds the note through reseedTriageNote` pin, verified to fail on exactly that deletion. Shape B — a docstring asserting reachability or impossibility whose named evidence does not establish it (7 sites). - feedback-refusal.ts (F-R3-1): the 5-step walk proves two errors can be live at once; it does NOT reach the `?? raised[0]` fallback, because step 5 ends on the tab that owns the refusal. Decoupled: the fallback needs only ONE refusal read from a tab owning neither form, which is the ordinary case and is already tested. - FeedbackDetail.svelte, feedback-drafts.ts, FeedbackPromote.svelte: "the row's `{#if open}` never goes false" narrowed to "stays true across a tab click", which is what `feedbackTabHref` preserving `?open=` actually buys. The wider claim was contradicted by FeedbackDetail's own precondition paragraph. - FeedbackDetail.svelte: "the only thing that can change under it is the stored note" is false; status/bugId/handledAt all change under the instance. - FeedbackDetail.svelte: the auto-switch rejection cited a reload "that would discard" the message. It would not — the message is component state that survives a load re-run. Race retracted and deliberately not replaced. - feedback-filters.ts: "the only area that can carry that key AT ALL" softened to "no live producer writes it"; a repo enumeration cannot bound a schemaless JSONB column. - FeedbackAttachments.svelte, feedback.test.ts: "no shape of this page can route raw JSONB" replaced with what a structural type actually buys. Also adds the missing half of `+page.svelte`'s "unreachable with JS" claim, citing the SvelteKit line that nulls `form` on navigation. Prose, pins and comments only. No behaviour change. Gates: svelte-check 0 errors (control: 7); apps/moderator 884 -> 886 passed, 0 failed (+2 = the new stripper controls); vite build clean; prettier clean with a named control flagged in the same run; eslint clean with a no-debugger control firing on both a .svelte and a .ts file. 17 mutants run against the tripwire pins, all killed, each on its own assertion. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
Briant Diehl
|
d341910096 |
Merge pull request #4815 from civitai/feat/generation-gate-rules-and-messages
feat(generation): generator messages, selectable disabled gates, expe… |
||
|
Zachary Lowden
|
b0711ee568 |
feat(apps): let an App Block publish a real post from its own outputs (#4811)
* feat(apps): let an App Block publish a real post from its own outputs
Adds the host half of a new App Blocks capability: a block can ask the host
to create a REAL, published Post on the viewer's profile, built from the
app's own generation outputs and/or images it previously published, with an
optional model-version gallery attach.
This is a SIBLING of the existing shared-grid publish bridge, not an
extension of it. Two reasons decided that:
* the existing publish payload carries a single required workflow id and
cannot express a post built from several sources;
* its consent copy says the images "become visible to other viewers of
this app", which is false for a profile post — and that sentence is the
security control, not decoration.
New scope `posts:write:self`, mapped to the existing "upload media & create
posts" OAuth bit. It is SENSITIVE (a manifest declaring it must justify it)
and CONSENT-PROMPTED (deliberately not consent-exempt), so a token carries
it only after the viewer grants it. Wired end to end: registry, sensitive
set, runtime binding, consent description, canonical manifest schema, and
both dev-mint allowlists. It is withheld from BOTH moderator-review mint
allowlists — a mod previewing an unapproved third-party app must never
publish public content under their own name.
Two procedures, because the consent dialog has to be trustworthy:
`previewPostFromApp` resolves, server-side, everything the confirm renders
(the exact copy, the tag names that will actually apply, host-fetched model
and version names, real thumbnails); `createPostFromApp` re-runs every guard
and writes. The block is sandboxed and cannot be trusted to display
truthfully, so the dialog asserts what the SERVER resolved, never what the
block sent. The preview confers no authority.
Controls, each with a negative test:
* SELF-DEALING — refuses a gallery attach whose model is owned by the
calling app's own publisher. This is the one control that removes the
payoff of routing viewers' posts at your own models; everything else
only raises the cost. A colluding second account still defeats it, which
is what the attribution marker below is for.
* The gallery gate is otherwise strictly stricter than the native path,
which checks nothing at all: published + public + undeleted, or refused.
* A block may NEVER mint a site tag. Requested names resolve against
existing tags only; unmatched names are dropped and shown in the confirm.
Moderation, system and admin-only tags are excluded even when they match.
* Server-side text bounds and a link refusal on title and detail, plus
blocked-content screening over title, detail AND the resolved tags (the
native path screens only the first two).
* Server-authoritative attribution in post metadata, using the same key
and semantics as the image-side marker so one moderation sweep reads
both. No post input schema exposes that field, so no client can forge or
suppress it, and the badge must render from the column rather than from
block-supplied copy. An index ships with it rather than after an incident.
* A DEDICATED rate bucket for posts, separate from the image-weighted
publish bucket; the per-image origin cost is still charged to the latter
so this path cannot be used to bypass it.
* A durable audit row for every outcome once the request is admitted,
with a named Activity-feed sentence rather than the generic fallback.
* Its own kill switch, independent of the App Blocks runtime flag, so
widening that flag toward GA does not arm public post creation on the
same day. SHIPS OFF: the flag does not exist yet, so an absent flag
resolves false for everyone and the whole capability is dark as merged.
* The confirm's exactly-once reply latch is the FIXED shape, so "declined"
means no post exists even if the dialog is dismissed mid-write.
* The write refuses if the resolved image set no longer matches the count
the viewer confirmed — the one divergence every authorization check
would happily wave through.
The write is atomic: create, adopt images, publish in one transaction. Image
adoption is a bounded update whose matched count IS the ownership and
provenance proof, so a row that changes underneath us aborts the whole thing
instead of leaving a public artefact nobody agreed to.
Known consequence, documented for app authors: adopting a previously
published image into a post removes it from the app's own shared grid, since
that read is scoped to post-less rows. An app cannot both keep an image in
its grid and let the viewer post it.
The min-trust gate moved to its own module now that it has a second caller;
the rule, its signals and its exact messages are unchanged.
The block-side message pair and the CLI manifest mirror are co-requisites
that ship separately and AFTER this deploys, since their drift guards
compare against the live published schema.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* fix(apps): declare the two new post procs in every host-rendering trpc mock
23 suites render PageBlockHost against a hand-listed mock of the tRPC client —
one entry per procedure the component reads. The component now reads two more,
so every one of those mocks returned `undefined` for them and the whole
component threw at render.
🔴 THE FAILURE DOES NOT LOOK LIKE A MISSING MOCK. The DOM comes back EMPTY, so
each assertion fails with "no element with data-testid=…" — which reads as the
element having been removed, in files that measure layout and have nothing to
do with posting. The geometry suite's own POSITIVE CONTROL failed too, and that
fixture is hand-built markup that never touches the new code; a reader would
reasonably conclude the harness was broken rather than the mock incomplete.
Caught by the geometry project, not by the new tests: every test added with the
feature mocks the service layer directly and none of them render the host, so
the seam between "the component reads a procedure" and "the mock declares it"
was owned by no suite that changed. That is the gap, not the fix.
Also adds a typed constant for the refusal codes the HOST itself emits on this
bridge, so a block can branch on them and a typo is a compile error. It is
explicitly NOT an allowlist: the reply's error field must stay shape-checked,
because server messages travel through the same field and a reply that fails
validation is dropped before correlation — wedging the block for ten minutes
instead of showing the reason.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* docs(apps): name the scan-timing constraint on posting an already-published image
An image published through the grid bridge is not immediately postable: that
bridge returns ids before any scan runs, and the adopt path requires a terminal
scan. An app that publishes and posts in one breath gets a refusal that reads
as a bug rather than as a wait.
* docs(apps): add posts:write:self to the block-scope table, and mark the table non-exhaustive
The table was already missing the shared-storage and collections scopes.
Adding one row while leaving four absent would make it look complete, so it
now says plainly that the constant is the authority and names what is missing.
* fix(apps): app-level post ceiling, refuse a non-terminal workflow, drop a dead re-export and three dangling citations
Four round-0 audit fixes on the app-post path.
1. AGGREGATION GAP — add an APP-scoped post ceiling.
checkBlockPostRateLimit keys on blockInstanceId, which bounds ONE install, so
an app with N installs gets N times the ceiling and nothing sees the total.
Adds checkBlockPostAppRateLimit(appId) on its own ':post-app:' sub-namespace,
checked alongside (not instead of) the per-instance bucket. 300/hour.
The number is NOT data-derived and the code says so: it is 100x the
per-instance ceiling, i.e. 100 distinct installs each at their own hourly max
in the same hour, and it is a starting value to be revised from the
block_scope_invocations audit rows. Sized loose on purpose — a too-tight
aggregate throttles a popular legitimate app and reaches users as "posting is
broken", which is worse and quieter than the abuse it prevents. The
per-instance 3/hour is unchanged and still labelled a guess.
Fails open on a Redis error like every sibling limiter, stated at the call
site so nobody reads either bucket as a hard cap.
2. PREVIEW/WRITE DIVERGENCE — refuse a non-terminal workflow source.
The generator of the race is a workflow that can still gain an output between
the two phases. projectAppWorkflow already computes status and
resolveOwnedWorkflowOutputs was discarding it, so the refusal costs zero extra
IO and binds every caller. pending/processing are refused with BAD_REQUEST;
succeeded/failed/expired/canceled are admitted because the output set is
frozen in all four. An unrecognised upstream status fails closed. The terminal
list is bound to the wire union with satisfies, so a status rename breaks the
build rather than widening the gate.
confirmedImageCount is KEPT and made REQUIRED, not deleted: the terminality
gate removes one known race, the count pins the whole resolved set, so a
future source arm or an expiring blob still surfaces as a refusal. An
integrity check a caller may decline is not an invariant. It moves out of the
shared preview/write payload shape — it is the write's echo of what the
preview returned, so the preview cannot be asked for it.
3. DEAD RE-EXPORT + a comment that misstated a dependency. The re-export claimed
to keep "every existing importer" working; the population is empty. Every
importer of apps-shared.router takes appsSharedRouter/appsModRouter,
sanitizeDiscordText, or the counter helpers — none takes assertSharedWriteTrust,
MIN_ACCOUNT_AGE_MS or REQUIRE_PAID_TIER. Removed, with the enumeration recorded
in place of the false claim.
4. THREE CITATIONS POINTING NOWHERE — "operator decision 3", "operator decision 4"
and "the design note calling for Scanned on every image" referenced a document
that is not in the tree. Inlined the substance at each site, written as
decisions with the rejected alternative named, and dropped the pointers. No new
doc: a pointer to a doc that can rot is what produced this.
Tests: app-ceiling refusal plus a different-app-unaffected case (the half that
proves app-scoping rather than a global key); the full pending/processing/
succeeded/failed/expired/canceled matrix plus an unrecognised status; ordering
against both ownership proofs; required-vs-optional on the confirm count.
* fix(apps): an app-created post was invisible to everyone but its author
Both `Post` triggers this path depends on are declared `AFTER UPDATE OF
"publishedAt"` — INSERT is in neither event list — and `writeBlockPost`
writes `publishedAt` inside the `post.create`. So neither fired, and
nothing else on this path writes `Post.nsfwLevel`.
The post therefore kept its schema default `nsfwLevel = 0` forever, and
both non-owner reads gate on it: `getPostDetail` admits a non-owner only
on `{ publishedAt: { lt: now }, nsfwLevel: { not: 0 } }`, and
`getPostsInfinite` masks on `(p."nsfwLevel" & browsingLevel) != 0`. A
post whose images are all `published` sources was a permanent 404 for
everyone but its author and absent from the profile tab and every feed,
while its images stayed visible in galleries — which reads as a cache
bug. Only that arm was broken: a post containing a fresh output recovers
by accident, because the output's later scan fires the Image trigger and
the job bit_ors over every image of the post.
`applyBlockPostPublishEffects` now re-issues both triggers. It ENQUEUES
`JobQueue(Post, UpdateNsfwLevel)` rather than calling
`updatePostNsfwLevels` directly, because the enqueue is what the trigger
does, so the consumer behaves natively and walks the post to the model
version it is attached to. Same fix seeds the `PostMetric(AllTime)` row
the metrics trigger would have created, so `ageGroup` is not left NULL.
Also in this pass:
- `droppedTags` reached the consent dialog unsanitised. It is the
block's own tag strings echoed back, so a block could put bidi
overrides and zero-width padding into the host surface that IS the
security control. Stripped at the source in tag normalisation,
re-sanitized at the render point, and included in the blocklist
screen. The module doctrine comment claimed the app name was the only
block-influenced value; corrected.
- `resolveOwnedWorkflowOutputs` claimed to return the same ordered
projection the block saw and did not — it filtered the host
allowlist, which silently renumbered later outputs, so an index could
select a different image than the one it named. It now blanks the slot
in place and the selection site refuses a blanked index.
- `applyBlockPostPublishEffects` had no direct test; its only coverage
mocked it wholesale. Added a ledger test asserting the exact effect
set, failing when it grows or shrinks, plus a test that drives the
visibility symptom end to end rather than asserting a call.
- ClickHouse `nsfw` was hardcoded false for every app-created post. It
is now derived from the adopted images, which is what the level the
job queue computes will be built from.
- Two comments corrected: the write-trust module claimed a re-export
that the other file explicitly denies, and the shared post preamble
claimed preview and write can never drift when write-trust is
deliberately write-only.
* fix(apps): make the nsfw-level enqueue atomic with the publish, and correct the trigger ledger
The enqueue that keeps an app-created post visible was issued from
applyBlockPostPublishEffects — a function the router calls AFTER
writeBlockPost's transaction has committed, and whose every rejection it
swallows into a log line. So a connection reset, pool exhaustion or
statement timeout on that single INSERT produced a committed post with
nsfwLevel = 0 forever: a permanent 404 to non-owners, absent from the
profile tab and every feed, with no reconciliation sweep (the temp
backfill covers ModelVersion/Model only) and no alert. The trigger it
stands in for fires INSIDE the publishing transaction, so the parity was
never exact.
It is now issued on the transaction client, as the trigger's own
statement, ON CONFLICT DO NOTHING included. The effects function's
docblock says why nothing visibility-deciding may live there: every
effect it issues must survive being silently dropped.
Tested at the symptom rather than the call: the publish ALONE, with the
post-commit path never invoked, must still leave the post readable by the
two non-owner predicates; and it must still do so when every post-commit
statement fails and the router swallows it. A "was enqueueJobs called"
assertion passes in both designs, which is why it was replaced. Plus the
structural half — the insert lands on the transaction client, not the
global one — and the rollback arm.
Also in this pass:
- The trigger enumeration claimed TWO Post triggers and that INSERT was
in neither event list. There are FOUR, and one of them is in a
migration rather than programmability, so a sweep of programmability
alone gets this wrong. post_published_at_change is a third UPDATE-only
one; it is not re-issued because its effect already happens by accident
— image_sort_at_before fires on the adopt, which runs after post.create
in the same transaction. That coverage is incidental and fragile, so
its ordering half is now pinned by a test and its SQL half is marked
unverified. The fourth, trg_moderation_post, DOES fire on INSERT, so
the block-supplied copy is queued for moderation with no help from us.
- A workflow source that named NO indexes over outputs with one
off-allowlist slot refused the entire post. The refusal exists because
skipping an index the block NAMED would publish a different image than
the one named; an omitted imageIndexes names nothing and is documented
as "every available output". It now skips the blanked slot there and
refuses only an explicitly named one.
- That made the all-blanked guard load-bearing rather than redundant, and
it is now pinned: deleting it changes the refusal to two different
messages that point an author at the wrong problem. Measured before:
deleting it left the file green, because the mutant died to the
downstream guard's identical message.
- The ClickHouse nsfw flag reduced over published members only, so a
MIXED post reported SFW permanently even after its fresh output scanned
X. A fresh member now unrates the whole post, matching the all-fresh
arm's conservative direction.
- Two claims narrowed to what they actually cover: the effect ledger
records only calls through the mocked modules, and the Tag.name
no-format-characters premise is an unverified empirical assumption
whose character class includes ZWJ/ZWNJ.
* docs(apps): correct five round-3 audit findings in the post-from-app comments
Comment and test-assertion corrections only. No behaviour change: the diffs in
both source files are comment-only, verified mechanically.
1. The SQL offered for settling the Tag.name premise did not run. PostgreSQL's
regex engine has no \p{...} property classes, so the suggested
`name ~ '[\p{Cf}]'` fails with "invalid regular expression: invalid escape \
sequence" — leaving an operator with a syntax error and the assumption still
unverified, which is the one thing that note exists to prevent. Replaced with
an explicit enumeration of the full Cf set written in ARE \uXXXX / \UXXXXXXXX
escapes, so the query also carries no invisible characters of its own, plus a
line saying why it enumerates so nobody simplifies it back. Verified on a
throwaway PostgreSQL 17.10 with both controls. The \p{Cf} in the code is
JavaScript's engine and is correct; it is untouched.
2. The cap docblock claimed a refusal rather than a truncation unconditionally.
The maxCount +1 trick is what makes an over-cap request visible, but the skip
arm for an unnamed index on a blanked slot consumes a selected slot without
contributing, absorbing that headroom. Docblock corrected to state the
exception; behaviour deliberately left alone, since the preview runs the same
resolver so the published set still matches the consented thumbnails.
3. The ON CONFLICT assertion targeted calls[0] positionally, two lines after the
same test computed a content-based selector. Now selects the statement by its
INSERT INTO "JobQueue" token, with a length check as a positive control on the
selector itself.
4. "byte-for-byte the one create_job_queue_record runs" was false — the trigger's
VALUES clause has no ::integer and different value sources. The statement is
character-identical to enqueueJobs' per-row SQL; cited that instead, and
stated the parity that actually matters (identical bare conflict target
against the JobQueue primary key).
5. The trigger ledger is complete for this repo, and the repo is not a complete
record. bitdex_post_54f0a619 appears only as a DROP on "Post", with no CREATE
anywhere in the tree, so this table has already carried a trigger the
enumeration method structurally could not see. Named pg_trigger on the
production database as the authority and gave the query, so "re-derive" is
actionable.
Also fixed a duplicated clause left by an incomplete edit in the
publish_post_metrics_trigger comment.
Gates: typecheck 0 errors; test:lint-rules 38 files / 530 tests; the post,
blocks.router and AppBlocks suites 71 files / 1450 tests; prettier clean (both
checks run against a validated instrument).
Mutation for 3: deleting ON CONFLICT DO NOTHING from the JobQueue insert fails
the new assertion at its own line with its own message, after the positive
control passes. Adding an earlier raw statement that also carries the phrase
while stripping it from the JobQueue insert PASSES 12/12 under the old positional
assertion and FAILS under the new one — the regression this change prevents,
demonstrated.
---------
Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
||
|
briant
|
2f721c0f94 |
feat(generation): generator messages, selectable disabled gates, experimental alert fix
- Fix: the form-graph generator never mounted ExperimentalRulesSync, so the experimental flask and alert never rendered there. - Gate and experimental alerts are no longer dismissible and render below the model selector instead of in the footer's priority-alert slot. - A rule-disabled ecosystem, workflow or model version stays selectable. The whatIf request and generate button are blocked, the graph still refuses disabled ecosystems/workflows, and validateInput refuses a disabled model version. - getGateRules parses rules one at a time, so a single unreadable rule can no longer drop every gate rule on an older build. - Generator messages: a separate store (Redis generation:messages) for mod-authored copy above the submit row, targeted by ecosystem, workflow or model version and by audience (members, non-members, tiers), with an optional per-message dismissal that re-shows when the copy is edited. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> |
||
|
Luis Rojas
|
cb8d553b16 |
fix(comments): copy link returns direct URL instead of patching current
Previously, the copy comment link button patched the current page URL with a highlight parameter. When copying a comment link from within a notification thread, this approach preserved the threadId and commentParentId query parameters from the notification, causing the copied link to resolve to the original notification's comment instead of the selected one. Now the function generates a direct `/comments/v2/<id>` URL for each comment, ensuring copied links always open to the correct comment regardless of the current page's context or parameters. |
||
|
Zachary Lowden
|
4aab099c91 |
fix(app-blocks): a no-user flag eval returns the flag's BASE value, not a deny (#4807)
* fix(app-blocks): a no-user flag eval returns the flag's BASE value, not a deny
Several docblocks in app-blocks-flag.ts derived a security property from an
inference that does not hold:
no user -> a global eval that can never match a segment -> fail-closed
The premise is true. A no-user call reaches Flipt as entityId 'global' with an
empty context, and every identity/tier/cohort segment we have is a
STRING_COMPARISON_TYPE constraint that reads the context, so none can match. The
conclusion does not follow from it: when no rollout matches, Flipt answers with
the flag's own base 'enabled' value. The denial came from the BASE being false,
not from the segment miss — so a base-true widening of app-blocks-author or
app-blocks-enabled turns every no-user branch from a deny into a pass.
MEASURED against the real @flipt-io/flipt-client-js wasm engine over a real
evaluation snapshot (new test app-blocks-flag.base-enabled-flip.test.ts):
base enabled:true + a non-matching SEGMENT_ROLLOUT, no entityId/context -> true
base enabled:false + the same rollout, no entityId/context -> false
unknown flag key, no entityId/context -> false
Both flags are base-false with segment rollouts today (civitai/flipt-state,
civitai-app/default/features.yaml), so nothing is exposed now. This is the latent
gate closed before any base-true flip, not a live defect.
Code:
- isAppBlocksAuthorEnabled: an undefined user now returns false structurally
instead of falling through to a global eval. Enumerated: all 10 call sites pass
{ user }; none wants a global eval of this key.
- blocks.router assertAppBlocksEnabledForTokenUser / assertViewerIsAppDeveloper:
refuse an unhydratable token subject before consulting the flag, with a distinct
message. Same shape apps.router.ts already uses.
- apps-shared.router resolveSharedContext: separate a VANISHED subject (refuse)
from an ANON token (keep the global eval — that widening is intended). The
read ops list/get had no second belt behind the flag.
- isAppBlocksEnabled's no-user branch is deliberately KEPT: it has a real caller
(pages/api/v1/developer/block-manifests.ts) that wants the base value.
Comments: every fail-closed / fail-safe claim in app-blocks-flag.ts swept and
given an accurate statement of what makes it closed; a new GLOBAL-EVAL SEMANTICS
block records the mechanism and the measurement once.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* refactor(app-blocks): make the compiler the guard — require a subject on the author capability
Round-1 review: the structural guard I said needed type-level nullability analysis
is analysis this repo already runs on every PR (tekton / typecheck, App unit tests
+ typecheck). Measured, then implemented.
isAppBlocksAuthorEnabled's parameter is now REQUIRED and non-nullable:
(opts?: { user?: SessionUser }) -> (opts: { user: SessionUser })
Making it required errored at exactly 2 of the 10 call sites — both bare
middleware(...) whose ctx.user is not narrowed by the protectedProcedure they are
attached to (app-listings.router.ts, app-collaborators.router.ts). Each now refuses
explicitly instead of handing a possibly-undefined subject to an authz gate. The
other 8 already held a non-null subject. The runtime `if (!user) return false`
branch this makes dead is DELETED rather than kept as defence in depth, and the two
runtime tests that pinned it are deleted with it: they could only be re-added behind
an `as never` cast, i.e. testing a path the type system forbids while reading as
coverage. What replaces them is the typecheck gate, plus one test pinning the
residual the docblock now states — a cast-defeated call THROWS, never returns true.
isAppBlocksEnabled keeps its optional overload. That asymmetry is now justified on
SEMANTICS (a kill-switch answers "is the feature on at all", which a subject-less
machine path may legitimately ask and which the base value is; a capability answers
"may THIS subject", unanswerable without one) rather than on its sole no-arg caller
existing — block-manifests.ts is dormant, so that justification is the half that can
vanish.
Also:
- Delete fixtures/flipt-base-enabled-flip.snapshot.json (152 lines). Derived at
runtime from the sibling fixture by re-key + enabled flip. A checked-in twin cannot
track the original — re-capturing the source means re-anonymising it, so the copy
silently keeps the old segment shape while claiming production fidelity, and it had
already lagged a segment.
- Consolidate ~60 lines of duplicated harness into fixtures/flipt-fixture-server.ts,
shared with the pre-existing real-flipt-client integration suite. Only the snapshot
differs between them; the instrument is the same.
- Stop enumerating live flag state in the GLOBAL-EVAL SEMANTICS header — that is the
same rot class the paragraph warns about. Keep the imperative, point at flipt-state.
- apps-shared: say READ_OPS rather than naming two of its four members.
- Replace a rejects.not.toMatchObject(...) positive control, which passes on any
other rejection, with the specific NOT_FOUND / 'Block install not found' outcome.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* fix(app-blocks): watchlist the two new refusals — the type guard cannot see a bundler
Round-1 review, 2 x yellow. Both taken.
WATCHLIST (the important one). My stated defence for deleting the helper's runtime
branch — "deleting it is a type error rather than a silent re-opening" — is a claim
about SOURCE. Release 5.1.18 is the case where the source is correct and the emitted
artefact is not: the bundler dropped two of three returns from a function in
app-blocks-flag.ts and served the whole App-store catalog to anonymous callers
(civitai#3983). That is why scripts/compiled-branch-watchlist.mjs exists, enforced at
Dockerfile:103, and two of the three refusals this PR adds are pure runtime branches
whose loss silently restores the exposure it closes. Both are now listed:
shared-storage-subject-refusal apps-shared.router.ts — lost, every READ_OPS op
serves shared rows to a vanished subject
block-token-subject-refusal blocks.router.ts — lost, the kill-switch is
evaluated with no subject on 16 runtime procs
assertViewerIsAppDeveloper's guard is deliberately NOT listed, and that is measured:
isAppBlocksAuthorEnabled takes a non-nullable subject and dereferences it at once, so
losing that guard throws rather than passes.
VERIFIED AGAINST A REAL BUILD, not just added. Full `next build` (green), then the
gate exactly as the Dockerfile runs it:
positive 26,600 maps scanned; both new entries report OK with their control
anchors mapped (an unmapped control is exit 2, not a pass)
negative required anchors repointed at unmapped comment lines in the same
functions -> exit 1 naming both entry ids, controls still mapping
deletion the required line removed in a throwaway source copy, one entry at a
time -> exit 2 naming that entry and its anchor
The two refusal messages were identical strings under different codes, so neither was
anchorable and "separable in a log" was not true. The author-gate message is now
'app-authoring subject could not be resolved'.
ROT (the second yellow). The isAppBlocksAgenticReviewEnabled docblock I wrote for this
rot class had the rot: it called the absent-flag half "load-bearing while this flag
does not exist in Flipt", but app-blocks-agentic-review has existed since 2026-07-21
(live: base false, `moderators` segment). No live exposure — all three call sites
check isModerator first — but it is a false claim in the one paragraph my own sweep
rewrote. Re-derived every app-blocks flag's live state rather than fixing only the
reported line, and found a second instance I had authored: the dev-tunnel docblock
offered "absent" as a live unconditional closure for a flag that also exists. Both
corrected; the only flag in this file genuinely absent from Flipt is
app-blocks-backpay-enabled.
Nits: assertAppBlocksEnabledForTokenUser has 16 call sites, not 17 (17 is the
parseSubjectUserId count; the substantive claim was right). deriveSnapshotFromFlagShape
now rejects an EMPTY rollouts array, which its error text already claimed to reject.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* docs(app-blocks): resolve the two docblocks that answer the reader twice, oppositely
Follow-on from the same re-check that found the agentic-review rot. Eleven docblocks
in this file carry an inherited "the flag does NOT exist in Flipt at merge time"
sentence; the new header frames all of them as as-merged history, which is the
deliberate treatment (substituting a fresher enumeration would be the same rot one
generation on).
But in exactly TWO docblocks that inherited sentence sits in the SAME block as a
live-state sentence I wrote, so the reader gets opposite answers eight lines apart:
isAppListingsEnabled "does NOT exist" vs "Both are base-`false` today"
APP_BLOCKS_SHARED_STORAGE_FLAG "does NOT exist" vs "Closed today because the base
is `false`"
Both inherited sentences moved to past tense and marked as as-merged notes. Nothing
else changed: the other eleven are untouched, because they carry no competing claim.
Criterion for the split, so it can be re-applied: fix where a paragraph contradicts
itself; leave where the header already governs.
Re-verified after the edit rather than carrying the previous measurement over — these
are comments, but they shift line numbers in a WATCHLISTED module, and source/artefact
correspondence is exactly what this PR stopped assuming. Fresh `next build` (green),
then the compiled-branch gate: positive OK on all three entries (26,600 maps; controls
mapped), negative control still exit 1 naming both new entries.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* docs(app-blocks): say at each watchlisted refusal that it is watchlisted
The compiled-branch gate names the entry, module, line and reason when it fires — but
only at Docker build time, after someone has already deleted the branch and pushed. The
two refusals this PR puts on the watchlist carried no signal in the file itself, so a
reader deciding whether the guard is load-bearing had nothing to go on. Two comments,
one at each site, naming the entry id and what a bundler can do to a pure runtime
branch.
Deliberately scoped: moving or rewording the throw is fine (the gate resolves its
anchor from source at run time), deleting it is not.
Re-verified after the edit, for the same reason as the previous commit: comments shift
line numbers in watchlisted modules. Fresh `next build` (green), gate positive OK on
all three entries, negative control still exit 1 naming both new entries. That is the
third independent repetition of both controls.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* fix(app-blocks): re-anchor the watchlist on format-robust text, and stop telling people to reword it
Round 2. Five defects, all introduced by this PR's own later commits.
F1 (the important one). The comment I added in
|
||
|
ZacxDev
|
84b4c0182e | 5.1.97 v5.1.97 | ||
|
Zachary Lowden
|
a0fff5e177 |
fix(apps): stop the block bridge honouring a revoked install until the token expires (#4806)
* fix(apps): stop the block bridge honouring a revoked install until the token expires
The tRPC procedures behind the host<->block postMessage bridge each called
verifyBlockToken directly - thirteen open-coded copies of the same two lines -
and checked nothing else. verifyBlockToken answers one question: is this a token
we signed, for this issuer/audience, not yet expired. It cannot see an uninstall,
a toggle-off, a publisher ban or a suspended app. So revoking or uninstalling an
app did not stop it driving the bridge; the app kept polling the orchestrator,
cancelling workflows and calling publishGenerationOutputs (which persists public
Image rows) for the rest of the token lifetime.
The REST wrapper never had this gap (withBlockScope has always called
BlockRevocation.isRevoked), and neither do the two tRPC resolvers beside this one
- resolveStorageContext in apps.router and resolveSharedContext in
apps-shared.router. The bridge was the remaining hole.
All thirteen call sites now resolve their claims through one exported helper,
authorizeBlockBridgeToken, which checks in order: token validity, per-instance
revocation, then the backing app_blocks row's approved status. No direct
verifyBlockToken call remains in blocks.router.ts.
One exemption, named rather than silent: the approved-status check is skipped for
a dev token. /api/v1/block-tokens' tryDevTunnelOwnedNonApprovedMint deliberately
mints a dev token carrying an app's REAL ids for an app that is NOT approved - a
suspended/pending/deprecated app stays runnable by its OWNER in the owner's own
dev tunnel (ownership enforced in the query, an active tunnel required, self-bound,
forced-SFW, budget-capped, never public) so they can diagnose it back into review.
Enforcing approval here would break that. Revocation is NOT exempted: every dev
and review-sandbox mint stamps a revocable instance id, so those tokens stay
killable.
Consolidating the thirteen copies surfaced two disagreements, both preserved
rather than papered over: getImagesByIds and updateUserSettings never checked a
consent scope at all (unchanged here - that is a separate question), and a
docblock above assertAppBlocksEnabledForTokenUser claimed the caller had "already
rejected invalid/expired/revoked tokens" when the caller ran a bare
verifyBlockToken and had never checked revocation. That sentence is now true.
Guarded by a structural ledger test in the test:lint-rules family
(no-unguarded-block-bridge-token) that pins the relationship, not a count: it
fails when a bridge call site disappears from the guarded set AND when one
appears that is not ledgered, plus separately when any direct verifyBlockToken
call reappears in the router.
The six existing blocks.router suites that exercise a bridge proc now declare an
approved app_blocks row; the shared db mock answers null by default, which the
new lookup would otherwise read as a deleted app.
* docs(apps): correct two claims this PR's own comments made — REST parity, and what the spelling guard covers
Round 0 of the audit ladder found both. Comments only; no behaviour change, both
guard suites still green (18 tests).
1. The helper's docblock said "The REST wrapper has always known this ... The bridge
procs ... checked neither", which reads as REST enforcing BOTH revocation and
approved status. Measured: withBlockScope calls BlockRevocation.isRevoked and has
NO approved-status gate, and neither does any handler it wraps. So the approved
check added here has no REST counterpart -- after a moderator suspension (which
flips app_blocks.status and writes no revocation marker) the tRPC bridge refuses
while REST keeps serving for the rest of the token lifetime. That asymmetry is
real, is NOT closed by this PR, and is now stated as out of scope rather than
implied away.
2. The ledger test was titled "has the two checks the guard exists for" while its body
only greps for three strings. It is walkable both ways: a semantically identical
rewrite fails it, and a comparison against the wrong value spelled the same way
passes it. Retitled and documented as a spelling check, pointing at the file that
actually pins the behaviour, so nobody reads it as coverage it does not provide.
Neither edit invents a new rationale for anything -- where the reason is that the
scope was not decided, it now says so.
* fix(apps): make the ledger see an unguarded bridge token, and stop three comments claiming an order that changed
Two round-1 audit findings on this branch.
1. The structural ledger could not see the hazard its filename names.
no-unguarded-block-bridge-token.test.ts keyed on calls to
authorizeBlockBridgeToken and on the literal spelling verifyBlockToken( —
both of which a procedure that verifies NOTHING satisfies vacuously.
Measured: adding a proc to blocks.router.ts that takes blockToken in its
input and base64-decodes the JWT payload inline left the file at 7 passed /
0 failed. An import alias walked through the same way.
The fix pins the RELATIONSHIP rather than the call sites. A second ledger,
BRIDGE_INPUT_LEDGER, names the population — every procedure whose .input()
carries a blockToken, derived from the router text (12 inline, 3 arriving
through schemas imported from buzz.schema, re-derived here rather than taken
from the audit) — and a new assertion requires every member of it to reach
the guard, directly or through a router-local helper resolved to a fixpoint.
Both ledgers stay SET comparisons in both directions. The alias hole is
closed structurally: the router may not import verifyBlockToken under any
local name.
Mutation matrix, each mutant inserted, run, and removed:
- unguarded proc (decodes the payload, verifies nothing) -> RED, and RED on
the relationship assertion ALONE once the proc is also added to the
population ledger, so the assertion is reachable and dies for its own
reason rather than to a neighbouring ledger mismatch
- direct verifyBlockToken( call inside an already-ledgered proc -> RED,
1 failed / 12 passed, isolated
- guard call site added but not ledgered (growth) -> RED
- guard call site removed (shrink) -> RED
- import { verifyBlockToken as verify } + an aliased call -> RED on three
assertions including the new import check
- clean tree -> 13 passed / 0 failed
What is still out of reach is stated in the file rather than implied away: a
token carried under a different input field name, verification performed in
a module this scan does not read, and a schema chain deeper than the depth
cap. An .input() identifier that cannot be resolved to a definition is
asserted as a failure, not scored as "no token".
2. Three comments asserted a rate-limiter ordering that the revocation guard
made false. All five in-resolver checkBlockCatalogRateLimit calls now sit
downstream of authorizeBlockBridgeToken, so a Redis GET and an indexed
appBlock.findUnique on the replica are spent before the limiter can refuse
anything. Corrected in authorizeBlockBuzzRead's docblock (which claimed
"BEFORE any db/ClickHouse work" and whose order list did not mention the two
new checks at all), in cancelAppWorkflow ("BEFORE any orchestrator
read/DELETE or DB query") and in getMyViewer ("Runs BEFORE the db read").
queryAppWorkflows' comment says "BEFORE the orchestrator call", which is
still true, and was left alone.
The ordering question was evaluated, not assumed. The limiter is keyed on
claims.blockInstanceId, so it can never precede verification — that is a hard
floor. Moving it to sit between verification and the approval read would
apply a 120-req/10s ceiling to all fifteen bridge procedures instead of the
seven that opted in, pollWorkflow among them, which is an availability change
rather than a cleanup; and it would save one Redis GET plus one replica
findUnique only on requests already over the ceiling. So the order stands,
and the guard's docblock now says so along with the per-request cost a reader
meets there: one Redis GET plus one indexed findUnique on every bridge call,
polling ones included.
Comments and tests only — no behaviour change.
Gates: typecheck 0 errors; test:lint-rules 38 files / 523 passed;
blocks.router.bridgeTokenGuard 11 passed; all 25 blocks.router.* suites
25 files / 733 passed.
* fix(apps): close four ways the bridge-token population could go unread, and two sentences wider than their code
Round-2 audit findings on this branch. Every one is the same shape as round 1 —
a description claiming more than the implementation delivers — so each is either
widened to match the sentence or narrowed to match the code, and nothing is
justified a third way.
1. The `unresolved` ledger was narrower than its own docstring. It promised an
unreadable schema is never "silently scored as carrying no token", but only
recorded one when the ENTIRE `.input()` argument was a bare identifier.
Measured with an unguarded proc that decodes the JWT and verifies nothing:
`.input(mysteryBridgeInput)` was loud, `.input(mysteryBridgeInput.extend({
page }))` gave `procs: []` / `unresolved: []` and left the whole file at
13 passed / 0 failed. Same for `.merge(b)`, a factory call, and any schema
behind a relative-path or package import.
The rule is now the argument's SCHEMA POSITIONS, not its shape: every
identifier in an argument that is not a member name, an object key, a locally
bound arrow parameter, a keyword, or the zod namespace must resolve. Comments
and string literals are blanked first by a character scanner — without that,
tokenising the router's annotated inline arguments yields 990 proc-identifier
pairs over 492 English words across 38 procs, which is how an `unresolved`
ledger becomes unusable and gets switched off. With it: 0.
Contributing cause, fixed explicitly: the suite had NO positive control that
`unresolved` can ever be non-empty. Both existing controls asserted `[]`, so a
probe wired to nothing was indistinguishable from a clean tree. There is now a
control feeding all four unreadable shapes and requiring 5 entries, plus a
negative control requiring 0 on an annotated inline argument, and the pair is
reported together.
2. Three more ways a proc could leave the population unread, all closed:
- the depth cap. The claim "the real corpus resolves every `.input()`
identifier within 2" was false: at a cap of 5 the walk reached depth 6 and
truncated 9 calls across 7 identifiers on the committed tree. No verdict
moved, but nothing said so. The walk terminates on its own at depth 8 with
no change in wall time, so the cap is 12 and truncations are now a ledger
asserted empty rather than a blind spot described in prose.
- a proc chunk cut short by a column-zero line, e.g. a multi-line template
literal continuation before `.input(`. Asserted against directly: every proc
chunk must still contain the `.mutation(` / `.query(` / `.subscription(`
that terminates it. 73 of 73 intact.
- a proc nested in a sub-router. `PROC_RE` pinned a two-space indent, so a
four-space sub-router yielded an EMPTY population. Widened; zero such procs
exist today, so this is a latent shape closed, not a bug fixed.
3. A router-local helper written as `const h = async (...) =>` made every proc
behind it read as UNGUARDED — fail-closed, but a false red on a legitimate
refactor, and wider than the docstring's promise that router-local delegation
is covered generally. `FN_RE` now matches both declaration forms.
4. "must not import verifyBlockToken under ANY local name" was wider than the
assertion: `importMap` parses static imports only, so the dynamic destructured
rename this router uses 89 times for other modules walks through it. Scope
narrowed on that test, AND a wider one added — the identifier may appear in
the router only on comment lines, which sees the static import, the dynamic
destructure, a namespace member access and a direct call alike.
5. Two sentences corrected, both closed enumerations that omitted something.
- the seven rate-limited procs were described as four procs "and the three
`getMyBuzz*` procs" behind `authorizeBlockBuzzRead`. The count of 7 is
right, the membership is wrong in both directions: `getMyDailyCompensation`
is behind the helper and is not a `getMyBuzz*` name, and `getMyBuzzBalance`
IS one, reaches the guard directly, and has NO limiter at all. So a reader
enumerating the seven both missed a throttled proc and counted the one
unthrottled buzz read as throttled. Named explicitly now, in the service,
in the call-site ledger and in the population ledger.
- "what the reorder would save is one Redis GET + one replica findUnique ...
roughly one op" omitted the priciest step. Measured ordering at all five
limiter sites: guard, then assertAppBlocksEnabledForTokenUser, then the
limiter. That middle step resolves the full SessionUser through a cached
read that falls through to an auth-hub fetch on a miss, then evaluates the
flag. On a cache miss the saving is a network round-trip, not one op. The
CONCLUSION is unchanged and was not rewritten: it rests on the availability
argument, which was verified independently.
What is still out of reach is stated in the file, not implied away: a token under
a different field name, verification in a module this scan does not read, the
verifier reached without spelling its name, and reachability being a textual call
graph rather than a proof the guard is awaited on every path.
Mutation matrix, each mutant inserted, run, and removed:
- unguarded proc behind `.extend(...)` -> RED on the `unresolved` assertion
alone, 1 failed / 19 passed; the SAME mutant against the pre-round-2 file:
13 passed / 0 failed, i.e. completely invisible
- `unresolved` rule reverted to bare-identifier-only -> RED on the positive
control, showing 1 of 5 shapes seen
- column-zero continuation inside a non-bridge proc -> RED on the chunk-integrity
assertion, isolated; same mutant pre-round-2: 13 passed / 0 failed
- `PROC_RE` reverted to a two-space indent -> RED on the sub-router control
- `FN_RE` reverted to `function` only -> RED on the arrow-helper control
- `const { verifyBlockToken: vbt } = await import(...)` + an aliased call ->
RED on three assertions including the new prose-only one, which names the
offending line; the import-alias and direct-call checks both stayed GREEN,
which is the overclaim being fixed
- depth cap lowered to 5 -> RED on the truncation ledger, reproducing all 9
truncations
- a second name added to the exemption set -> RED on the exemption pin
- `stripNonCode` neutered -> RED on the negative control and on the real-router
population scan
- clean tree -> 20 passed / 0 failed
Comments and tests only, on both files -- no behaviour change.
Gates: typecheck 0 errors; test:lint-rules 38 files / 530 passed;
blocks.router.bridgeTokenGuard 11 passed; all 25 blocks.router.* suites
25 files / 733 passed.
|
||
|
Zachary Lowden
|
9ade945de2 |
fix(bot-account-detection): the filename evidence read never completed, and never covered the cohort (#4808)
The filename evidence read failed on every run, and could not have covered the cohort on the runs where it completed. Root cause: `LIMIT` + `ORDER BY id DESC` over a highly-selective `userId IN (…)` on a very large table. The planner's row estimate is ~2,400x the true count, so a backward primary-key scan looks cheap — it expects to reach the LIMIT immediately. The cohort owns *fewer rows than the LIMIT asks for*, so the early exit never fires and the scan walks the table until the connection times out. Chunking is not the fix and was measured to be strictly worse: a shorter id list lowers the estimate and the LIMIT together, giving the same plan once per chunk. Second, independent defect: the `take` was global across the chunk, so a few prolific uploaders could consume the whole allowance and evict every other account — silently, with healthy-looking counters. Across ten consecutive cohorts the two failure modes are mutually exclusive: the LIMIT is reachable exactly when coverage collapses, and unreachable otherwise. There is no day on which this read was both complete and correct. Fix: one read per cohort member, equality on the `(userId, id)` index, with a per-member cap and batched concurrency. Measured at 0.1-0.2 ms per account. This is the same correction `MAX_IPS_PER_ACCOUNT` already made on the ClickHouse side, whose docstring diagnoses this exact failure. Coverage is unconditional up to `MAX_FILENAME_SAMPLES / MAX_FILENAMES_PER_MEMBER` members; past that the global budget still truncates in member order. That bound is stated in the docstring rather than claimed away. Observability: `sources.readFailures` distinguishes a source that FAILED from one that found nothing — previously indistinguishable. It is emitted in the run counters and the structured log payload, and the report summary now says which of the two occurred. Note the counters have no automated consumer yet; the log payload and report summary are the surfaces that work today. No migration required: the index this relies on is already declared. Known and filed, not fixed here: - the registration-IP discard has the same untested shape as the filename one had - `listContentSamples` still takes a whole-result cap with an ordering under it, i.e. the same eviction defect this change removes for filenames |
||
|
Zachary Lowden
|
2803e825b1 |
feat(app-blocks): give sensei a full-bleed exemption from the page-width cap (#4804)
* feat(app-blocks): give `sensei` a full-bleed exemption from the page-width cap
Adds `sensei` as the second member of the FULL-BLEED OPT-OUT LEDGER in
`src/styles/globals.css`, keyed on `[data-app-page-frame][data-block-id='sensei']`
— the same selector shape the `playable-collections` rule uses after its
production fix, NOT the `data-testid` spelling that `next.config.mjs` compiles out
of the live DOM.
This is a PRODUCT DECISION by the repo owner, not a bug fix. Nothing about the cap
is malfunctioning for this app; sensei is one of the two apps the cap's own census
was written about, and the trade is being taken the other way for it. Notepad, the
sibling case in that same census line, is deliberately NOT changed here.
The membership assertion was watched fail before it was updated: with the rule in
and the expectation untouched, `pageBlockHostMaxWidth.test.ts` went red naming the
set delta (`+ "sensei"`, expected `['playable-collections']`), which is the
designed workflow for this ledger.
Also corrects the ledger's membership claim in every place it was restated, which
turned out to be three rather than two:
· `PageBlockHost.tsx` still asserted "NO LEDGER ENTRY IS WRITTEN TODAY, and the
ledger's expected set … is `[]`". The test's real expectation was already
`['playable-collections']`, so the comment was the stale side and had been
false since that entry landed. It no longer restates the membership at all.
· `globals.css`'s ledger header said "One member today". It now names no count —
the rules below it are the authority.
· `PageBlockHostMaxWidth.browser.test.tsx` said "THE LEDGER'S ONE REAL MEMBER"
and named `playable-collections` in its own title. Its green arm is now DERIVED
from the rules it already parses out of `globals.css`, so every member is
measured and no count is stated; the enumeration that must fail on growth AND
shrink stays in the node tier, which is a different claim.
The publisher-facing HOW-TO in `docs/features/app-blocks.md` gains the entry with
its reason and likewise drops the count.
Verified (both vitest tiers read, per the two-tier rule):
· node `unit` — `pageBlockHostMaxWidth.test.ts` + `ledgerSelectorSurvivesProdStrip.test.ts`
16/16 green; red→green on the membership assertion shown above.
· browser `component` — `PageBlockHostMaxWidth.browser.test.tsx` 11/11 green,
and the new derived arm was mutation-tested: setting only the sensei rule's
value to `1500px` failed with THIS arm's own message ("the app 'sensei' is NOT
full-bleed", 1500 vs 2560) while the other 10 tests stayed green.
· `pnpm run typecheck` 0 errors; `prettier:check` clean; eslint on the touched
TS files 0 errors (1 pre-existing unrelated hooks warning); `test:lint-rules`
510/510.
NOT verified: a green browser tier is specifically NOT evidence the selector works
on civitai.com — vitest never runs with `NODE_ENV=production`, so the
`reactRemoveProperties` strip never applies, which is exactly how the `data-testid`
spelling shipped broken with that suite passing. The production-safety claim rests
on `ledgerSelectorSurvivesProdStrip.test.ts` (which compares the two
configurations) and on mirroring the already-fixed rule's shape. A CSS rule's real
proof is a rendered page on a wide viewport, which was not reached.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01WKwVejr8dHqb8nydsfqHk3
* docs(app-blocks): make the full-bleed ledger's bar admit its second member by the rule
The ledger's admission criterion excluded the entry the same PR added. It read
"an app whose surface is the canvas … NOT for 'it looks bigger'", and the sensei
entry openly does not clear it — it says the cap binds exactly as designed and
the owner decided the trade should go the other way. A bar the second of two
members openly fails, with nothing amended, is decoration: the hazard is the
THIRD entry, which now has precedent for "the owner said so" as a ground the
written criterion still excludes.
So the criterion now names the category actually used. Two grounds, and an entry
must say which it claims: (1) the surface is the canvas, unchanged; (2) an
explicit product decision by the repo owner, admissible only if it records who
decided, what trade was accepted, and what it is worth as a measured
display-width class. The "NOT for 'it looks bigger'" exclusion is KEPT and is
what ground (2) is distinguished against — a preference has no author, no stated
cost and no measured reach, so the three bullets refuse it by the same sentence.
Ground (2) does not waive ground (1) generally; it admits the entry that shows
its cost, one at a time. The sensei entry then records those three things, with
the record kept to what is actually on it and no extrapolation of the owner's
reasoning.
The entry's other half was an unasserted cross-repo reading that disclaimed
itself — "no ref recorded, no fixture reproducing it … the sensei repo was not
re-read for this entry" — while member 1 one slot up carries a deployed ref and
five file:line citations. Same slot, opposite evidentiary standard. The reading
has now been taken and the census HOLDS, so the citations replace the
disclaimer, matching member 1's convention: root shell, main row, the fixed
240px sidebar, the chat pane, and the absence of any max-width on the transcript
path, each at its own file:line. Recorded as `trunk 6fd63c0` and explicitly
flagged as a BRANCH TIP rather than a resolved deployed ref, because none was
resolved for sensei — claiming a deployed ref that was never resolved is the
error this flag exists to prevent.
Two deletions, both reviewability:
- `PageBlockHost.tsx` quoted the old false sentence verbatim, declared "THE
MEMBERSHIP IS DELIBERATELY NOT RESTATED HERE", and then restated the
membership five lines later. Both paragraphs collapse to one line; cutting
the second is what makes the first's stated policy true. `git log -p` holds
the old sentence and the enumeration test already fails on growth and shrink.
- `docs/features/app-blocks.md` had swapped a rotting COUNT for a rotting
LIST. Nothing asserts the doc's member list matches `globals.css` — the
prod-strip guard pins the doc's selector SHAPE only, never its membership —
so it reduces to one sentence pointing at the ledger, which the doc already
calls the authority.
No selector and no assertion logic changed: comments, one docs paragraph and the
criterion only. Verified on both tiers in an installed worktree (Round 0 could
run neither). Unit — `scripts/test-unit-run.mjs`, 16 tests across
`pageBlockHostMaxWidth.test.ts` + `ledgerSelectorSurvivesProdStrip.test.ts`,
green before and after. Component — `scripts/test-component-run.mjs`, 11 tests
in `PageBlockHostMaxWidth.browser.test.tsx`, green before and after, with the
membership and opt-out render arms unmoved. `typecheck` clean.
The generic prod-strip guard was confirmed to cover the new rule by mutation
rather than by reading: keying the sensei rule on `data-testid` turns exactly one
test red — "no shipped ledger rule depends on an attribute production strips" —
with that guard's own message and a payload naming the sensei selector, while the
membership assertion stays green. That guard came from PR #4590, not this PR.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01WKwVejr8dHqb8nydsfqHk3
* fix(app-blocks): pin the ledger walk against the file, and retract a criterion claim that does not hold
Round 1's fix round traded away a mutation class, and the ledger's admission
criterion claimed more than it can enforce. Four findings, no runtime change —
one test assertion, comments, and one docs word.
F1 — the derived green arm could miss a member without failing.
`ledgerFromGlobals` walks the CSSOM and descends into `CSSLayerBlockRule` only,
so a member rule nested in `@media`/`@supports`/`@container` drops out of
`ledger.ids`. The rewritten control only asserted the list was non-empty, and a
count cannot see a PARTIAL loss: the OTHER member keeps it non-empty, the loop
never mounts the lost one, and the test passes. Measured, not theorised —
wrapping the sensei rule in `@media (min-width: 3000px)` left this file 11/11
and the two node-tier guard files 16/16 while sensei rendered capped at 1600 on
a 2560 display. Neither of the other guards can see it: both read raw text,
where the id is still present.
So the relationship is pinned instead — the ids the CSSOM walk could REACH must
equal the ids the file textually contains, comments stripped. That keeps the
"no membership restated here" property (both sides derive from the shipped file)
and fails the moment a rule moves somewhere the walk cannot reach. Watched red
under the `@media` wrap at 1 failed / 10 by its own message, naming the delta
`- "sensei"`; green with the rule at the top level. The id regex is now shared
by both parses so the comparison can only ever be about reachability, never
about quoting.
F2 — the admission criterion does not refuse "it looks bigger", and said it did.
Ground (2)'s three bullets were claimed to exclude a bare preference because a
preference has "no author, no stated cost and no measured reach". That inference
does not hold: every request has a requester, WHAT TRADE is the same ~150px
gutter arithmetic for every app (a property of the 1600 cap, not of the app),
and WHAT IT IS WORTH measures the reach of the EXEMPTION, so two different apps
get a literally identical answer. The entry admitted under it proves the point —
it records that no app-specific justification exists and clears the bar anyway.
The strict repair would make sensei's own entry inadmissible, so the claim is
RETRACTED rather than replaced by a stronger one: ground (2) is an owner
override, the bullets make an entry recorded and reviewable rather than
justified, and the `NOT for "it looks bigger"` exclusion is moved onto ground
(1), where there is a claim about the surface to be wrong about. The shape to
watch for — a run of entries each copied from the last — is named, with the
honest alternative (move or drop the 1600 default) stated. The sensei entry now
says outright that it is not a template. Two sub-points with it: the WHO bullet
accepts a role, since this repo is world-readable and a role is what is actually
applied; and "this changes nothing at all for where the traffic is" is marked as
a claim about display WIDTHS, not about a population this repo measures nowhere.
F3 — the cap's value-justification still named Sensei as a case it governs.
After this branch Sensei gets the viewport, not ~1350px. The paragraph now
describes the two-pane SHAPE the value is justified against and names no app,
which is also the only phrasing that does not restate membership in a file that
deliberately refuses to.
F4 — docs: "the two grounds" is the count the same sentence says is not mirrored.
Re-verified unchanged after the F1 rewrite: the membership assertion still fails
in both directions (expectation reverted -> `+ "sensei"`; CSS rule deleted ->
`- "sensei"`, browser tier correctly still green there, since the shrink claim
is the node tier's), the derived arm's `1500px` mutation still dies with its own
message at 1 failed / 10, and the prod-strip mutation still isolates at
1 failed / 15.
node `unit` 16/16 (scripts/test-unit-run.mjs, the two guard files), browser
`component` 11/11 (scripts/test-component-run.mjs), scripts/typecheck.mjs 0
errors, prettier:check clean.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01WKwVejr8dHqb8nydsfqHk3
---------
Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
|