Files
civitai__civitai/CONTRIBUTING.md
T
Zachary Lowden 6de64ee5a8 fix(tests): unzero the preview / component-tests tier, and make a zero-collected run say so (#4531)
* fix(tests): unzero the component tier — one mock factory was aborting the whole run

`preview / component-tests` was reporting `failure` on `main` while executing ZERO
tests. Reproduced deterministically (3/3 at d353f785c3, and in isolation): the whole
`component` project aborts before any reporter prints, with no `Test Files` line, no
per-file results, and exit 1.

Root cause, one file:
`src/tests/pages/apps/review/review-queue-nav.browser.test.tsx:30` mocks
`~/providers/FeatureFlagsProvider` with a WHOLESALE factory naming only
`useFeatureFlags`. The review queue page's row now renders the review entry point,
which reads flags through `useOptionalFeatureFlags`, so the named import has nothing
to bind to:

  SyntaxError: The requested module '/src/providers/FeatureFlagsProvider.tsx'
  does not provide an export named 'useOptionalFeatureFlags'

In the node `unit` project that would fail ONE file. In BROWSER mode it kills the run:
vitest resolves a manual mock over the browser-to-node channel inside a Playwright
route handler that does not catch (`@vitest/browser-playwright/dist/index.js`,
`await module.resolve()` inside `page.route`), so the rejection escapes as an
Unhandled Rejection in the orchestrator.

The printed error names neither the file nor the real cause. It is wrapped twice --
once by the browser mocker, once again on the node side -- and the innermost `cause`
is dropped in transit, so all you see is the generic "[vitest] There was an error when
mocking a module ... make sure there are no top level variables inside", which points
at hoisting and is wrong. The root cause above was recovered by temporarily patching
`createHelpfulError` in the browser tester bundle to inline `cause.stack`; the file was
then confirmed by bisecting the 50 candidate files down to one.

This is the SECOND time this class has bitten (see the header of
`src/components/AppBlocks/__tests__/featureFlagsMockCompleteness.test.ts`, which fixed
six sibling suites in the AppBlocks directory and deliberately scoped its guard there).

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

* test(ci): make the component tier fail LOUDLY on zero collected, not silently

The `preview / component-tests` tier could report `failure` having executed ZERO
tests, and nothing anywhere said so. The shared `npm-report-only-suite` Tekton task
computes its verdict from the runner's EXIT CODE alone, so an abort that collected
nothing and a genuine list of red assertions both render as `component:fail` /
"Component suite failed" -- same words, same colour, same place. That is the shape
that trains people to click through a tier.

`pnpm test:component` now runs through `scripts/test-component-run.mjs`, which asks
vitest for a JSON report and hands it to `scripts/ci/assert-component-suite-ran.mjs`.
That gate prints a ledger -- `N executed, N skipped, across N files; N failed suites,
N failed tests` -- and fails when nothing was collected, or when the executed count
falls below a floor (1240, ~55% of the 2254 measured on a full green run of 201 files
on 2026-08-31).

Deliberate limits, each of which is a way this could have been wrong:

- It can only ever ADD a failure. Vitest's own exit code is passed straight through
  when the ledger is satisfied, so a red suite stays red for its own reason.
- EXECUTED, not TOTAL. `numTotalTests` counts skipped tests, so a suite that
  self-skipped wholesale would satisfy a total-based floor having run nothing.
- `failed` counts as executed. A guard that scored a red run as "did not run" would
  fire on every genuine failure, and the tier would then be red for two different
  reasons that nobody could tell apart -- the exact confusion this removes.
- A signal-killed runner short-circuits the gate entirely. The CI task wraps this in
  `timeout(1)` and distinguishes "timeout" from "fail" on purpose; a killed run also
  writes no report, so relabelling it "collected nothing" would be a wrong answer
  rather than a missing one.
- A narrowed run (a file argument, or `-t`) skips the FLOOR but not the zero check:
  the collected count is then a property of the filter, but a single-file run that
  collects nothing is the cheapest reproduction of the abort and is precisely when
  someone is debugging it.

The message enumerates every cause it cannot tell apart rather than asserting one --
an absence is the observable the most causes share, and a guard that names the wrong
one sends the next reader hunting a bug that is not there. All three named have been
observed on this suite; two of them were observed while writing this change, and the
browser-crash one arrives wearing the mock error's headline with the real cause on
the `Caused by:` line.

Verification:
- 16 unit tests over JSON fixtures (`scripts/__tests__/`), covering zero, the floor
  boundary both sides, all-red, all-skipped, narrowed both ways, import-failed
  suites, and a missing/unparseable report.
- Mutation-checked: 9 mutants of the gate, ALL KILLED, each by the specific named
  test written for it (verified per-test, not "some test failed"), against a green
  16/16 baseline.
- End-to-end negative control: reverting the one-line fix in the previous commit and
  running through the wrapper produces the ledger's abort message and exit 1, from
  the missing-report branch.
- End-to-end positive: the full suite through `pnpm run test:component`.

Docs corrected while here: CONTRIBUTING said component tests "don't run in CI at all"
and put the file count at 106; they do run, report-only, in the preview pipeline, and
there are 201 files with ~2,250 tests.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

* fix(tests): the zero-collected message overstated a PARTIAL abort

The headline said "THE COMPONENT SUITE COLLECTED NOTHING ... nothing executed", and
the code contradicts it in the case that actually happens most: the JSON reporter
writes at the END of a run, so an abort part-way through leaves no report at all.
Measured while writing this: a run that aborted 68 files into 201 had 68 files
scrolled past as green, wrote nothing, and got told nothing executed.

Both halves of that were wrong to assert. Tests HAD executed, and a reader who
scrolls up and sees green lines is entitled to believe the message is confused --
or, worse, to believe the green lines are coverage.

So the headline is now "THIS RUN PRODUCED NO ACCOUNTABLE RESULT", which is true of
both shapes, and the diagnosis says explicitly that an abort can land part-way and
that whatever scrolled past is unaccounted for rather than confirmed. The
missing-report branch says why the report is absent (the reporter writes at the end)
and that a partial run and a run that never started are indistinguishable from
there -- which is the reason neither counts as one that ran.

No behaviour change: the same runs fail, with the same exit code. The four test
assertions that pinned the old headline move with it.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

* fix(tests): close six findings from the adversarial audit of this PR

An adversarial audit of #4531 found two ways the new guard reproduced the very
pathology it was written to remove, plus four smaller gaps. All six are fixed here;
nothing about the payload fix in c32aa06e8d changes.

1. A SIGNAL WAS RELABELLED AS A TEST FAILURE. `child.on('exit')` returned a hardcoded
   143 for EVERY signal. `report-only-suite-task.yaml` branches on 137 to report
   `oom-killed` -- "this is an OUT-OF-MEMORY kill, not a timeout; raise the task's
   memory limit" -- and before this wrapper existed an OOM-killer SIGKILL on vitest
   reached that task as 137, because pnpm re-raises. With a constant 143 it matched no
   branch and fell through to `RC=1`, verdict `fail`, rendered "Component suite
   failed". A memory problem reported as a test failure, on the same tier, in the same
   words. Now `128 + os.constants.signals[sig]`, so 137/143/130 come out right and the
   mapping cannot drift from the names node hands back. The comment above the branch
   claimed it passed the status through; it does now.

2. A CALLER `--outputFile` MADE A GREEN RUN REPORT "COLLECTED NOTHING". Measured by
   the auditor: `pnpm test:component <file> --outputFile=/tmp/x.json` wrote an 18/18
   green report to the caller's path, left the wrapper's path empty, and printed the
   full "this tier verified NOTHING on this commit" diagnosis naming three causes,
   none of them real. `.github/workflows/lint.yml` runs the SIBLING unit tier with
   exactly that flag, so it is a copy-paste away. Now refused up front with the fix in
   the message.

3. `isNarrowed` WAS WRONG IN BOTH DIRECTIONS ON SPACE-SEPARATED FLAGS. `--max-workers 1`
   -- the form CONTRIBUTING steers people towards for sizing a run on a shared box --
   put `1` in a positional slot, so the run scored "narrowed" and THE FLOOR WAS SILENTLY
   TURNED OFF; same for `--reporter`, `--retry`, `--bail`, `--project`, `--pool`. In the
   other direction `--shard=1/4` scored NOT narrowed, so a healthy sharded run would
   fail the floor while being told "Do NOT lower the floor to make this green" --
   misdirection, not merely a false red. Value-taking flags now consume their value, and
   `--shard`/`--changed`/`--related` narrow explicitly.

4. WINDOWS. `spawn()` of `vitest.cmd` without `shell` has failed since the node
   18.20.2/20.12.2 CVE fix; `scripts/test-unit-run.mjs` already sets
   `shell: process.platform === 'win32'` for this reason. Added. And a spawn failure
   (rc 127) now short-circuits instead of handing the gate a missing report and getting
   forty lines about mock factories and dead browsers.

5. THE REPORT IS NOW DELETED BEFORE THE RUN, not only after. Cleanup after the run is
   skipped by exactly the paths that leave a stale report behind (a signal death, a
   throw), so a healthy 2254-test report could survive into a later run that aborted
   before writing one -- the gate reading it, printing a green ledger, and passing:
   silently inert in precisely its own use case.

6. THE GATE COMPUTED A FILE COUNT AND NEVER CHECKED IT. Replaced with a LEDGER, which
   is the stronger of the two checks: it walks `src/` for every `*.browser.test.tsx` and
   fails when one is absent from the report, NAMING it. The test floor sits at ~55%, so
   ~45% of the suite could stop being collected while the gate stayed green -- and the
   incident this whole guard descends from is exactly that shape (six files contributing
   0 of 438, nothing red). The expectation is re-derived every run, so there is no
   constant to go stale. Skipped when narrowed, and when the walk finds nothing -- an
   empty walk is not a measurement, and it SAYS so rather than passing quietly.

Also: the fixed factory now names all THREE hooks the flags module exports.
`useFeatureFlagsReady` is the third and has four live consumers; none is in this page's
graph today, which is the only reason naming two loads at all. The comment said "BOTH
hooks", which reads as "the module has exactly two" -- and it is the template the PR
proposes copying to fifty files.

Verification:
- 27 unit tests (was 16), including the on-disk ledger against a fixture tree that
  contains a `node_modules/` and a non-browser `.test.tsx` neither of which may count.
- Mutation-checked: 17 mutants, ALL KILLED, each by its own named test. One SURVIVED on
  the first sweep -- `out.length > 0 ? out : null` was unreachable from the fixture,
  which had no `src/` at all, so the walk returned early. A second case with a `src/`
  that holds no browser tests reaches it; an empty array is TRUTHY, so that mutant would
  otherwise have armed a ledger over zero expected files and passed everything.
- The on-disk ledger run against the REAL tree: 201/201 green, and dropping one real
  file from the report fails and names it.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

* fix(tests): close audit round 2 — two regressions the round-1 fixes introduced

A delta re-audit of `6ceac37f33..7f005f0482` confirmed 4 of the 7 claims outright
(the signal mapping, the pre-run clear, the file ledger, the fixed factory) with its
own positive and negative controls, and found that two of the fixes had introduced
new defects of their own. Both are here, with the smaller findings.

🔴 THE ON-DISK LEDGER FALSE-FAILED A LEGITIMATE RUN, WITH A MESSAGE FORBIDDING THE FIX.
`--exclude`, `--dir` and `--root` were added to VALUE_FLAGS but not to
NARROWING_FLAGS. All three take a value AND genuinely shrink the collected file set,
so `pnpm test:component --exclude 'src/tests/**'` scored as a FULL run and the ledger
failed it naming up to 200 files -- asserting the include broke or the run died, and
telling the reader "Do NOT silence this by narrowing the walk", which is the only
thing that would have fixed it. `--config` joins them: it can replace the project's
`include` outright, which is the assumption the walk is built on. This is precisely
the shape the round was convened to prevent, produced by the round's own fix.

🔴 `shell: win32` WENT ON THE SHARED `run()`, SO IT ALSO WRAPPED THE GATE SPAWN.
The claim said this matched `scripts/test-unit-run.mjs`; it did not -- that file puts
`shell` on its vitest spawn only and deliberately leaves its `process.execPath` spawn
alone. Node with `shell: true` concatenates argv UNESCAPED (DEP0190), and the gate is
spawned as `process.execPath`, which on Windows is `C:\Program Files\nodejs\node.exe`.
So on the one platform the option exists to support, every `pnpm test:component` would
have failed at the gate step with a cmd.exe parse error rather than any of this
wrapper's messages. `shell` is now per-call.

Also:

- KEBAB AND CAMEL ARE THE SAME FLAG TO VITEST (cac camelCases every option key), so a
  hand-enumerated list has a hole wherever it carries one spelling and not the other --
  and it did: `--max-workers`/`--maxWorkers` were both listed, `--test-timeout` was
  not, so the kebab form silently disabled both checks. Spellings are now canonicalised
  rather than enumerated, and the test asserts them in PAIRS.
- `--output-file` (kebab) was NOT refused, so the exact defect the refusal exists for
  was still reachable -- under a test named "catches every spelling". Meanwhile
  `--outputFile.junit=` and `--outputFile.html=` WERE refused, though neither touches
  the `.json` key: object-form output paths are per-reporter, so that was over-strict
  and the stated reason ("the bare form sets the path for EVERY reporter") is true only
  of the bare form. Both directions fixed.
- The rc-127 short-circuit is keyed on a spawn-failure FLAG, not on the number. 127 is
  an exit code a runner can produce on its own, and relabelling that as "the binary
  could not be started" is a second wrong answer that also skips the gate on a run that
  happened.
- `--repo-root <dir>` did not consume its value, so with the flag FIRST the directory
  was read as the report path: "EISDIR: illegal operation on a directory" plus the
  whole abort diagnosis. Every test passed it last, which is why nothing caught it; the
  ledger tests now pass it first.
- `--related` removed. It is a vitest SUBCOMMAND, and this wrapper always spawns
  `vitest run …`, so it cannot arrive -- the entry and its assertion were both inert,
  the assertion pinning behaviour for an input the runner cannot receive.
- The factory now names all FOUR runtime exports of the flags module, including
  `FeatureFlagsProvider`. The comment said "EVERY runtime hook", which was narrower
  than the module -- and "not in this graph today" is exactly the reasoning that put
  this file in the diff.
- A comment the previous round made false: it said the positional rule would catch
  `-t foo` anyway. It stopped being true the moment `-t` was listed as value-taking --
  the value would now be CONSUMED -- so the explicit narrowing branch is load-bearing.
- CONTRIBUTING documents the refusal, the file ledger, and which flags narrow.

Coverage for the two claims that shipped with none: `main` now takes its collaborators
as injected defaults, so the ORDER of effects can be asserted rather than inspected.
The two `clearReport()` calls are byte-identical statements -- only their position
carries the meaning, so a refactor moving the first below the run reopens "a stale
report satisfies the next run's ledger" with every other test green.

Verification: 35 unit tests (was 27). Mutation-checked at 17 mutants, ALL KILLED, zero
survivors -- including re-running round 1's guards, because an audit fix resets the
gate. Real-tree controls re-run with the new argument parsing, flag first and flag
absent: 201/201 green, and dropping one real file fails and names it. ESLint on the
touched files is back to the base count; two `no-empty-function` errors this change
introduced are fixed.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

* fix(tests): close audit round 3 — the canonicaliser re-opened the class it closed

Round 3 verified 8 of round 2's 9 claims against the diff or against vitest's own
source (it read cac's `setDotProp` to confirm the `--outputFile.junit=` allowance is
genuinely safe, and `cliOptionsConfig` to confirm `-r`/`-c` really are root/config).
It found one regression and one coverage hole, both here.

🔴 A BOOLEAN FLAG SWALLOWED THE FILENAME AFTER IT — the same shape round 2 fixed,
re-introduced by round 2's own mechanism. `canonicalFlag` collapsed `.subkey` onto the
parent, so every dot-subkey inherited its parent's value-consuming behaviour, and the
two entries `--coverage.reporter`/`--coverage.provider` became a single `coverage` that
also matched the BARE `--coverage` — which takes no value (`argument: ""` in vitest's
`cliOptionsConfig`). So `pnpm test:component --coverage <file>` ate the FILE as
`--coverage`'s value, scored the run as full, and failed an 18/18 green single-file run
naming ~200 files as ABSENT while telling the reader not to narrow the walk. Same for
`--coverage.enabled <file>` and `--browser.headless <file>`. Measured base-vs-head, all
three flipped `true` to `false`. Matching is now on the full canonical PATH, so a
subkey is value-taking only if it is listed as one.

🔴 THREE MUTANTS SURVIVED A GREEN 35-TEST SUITE, AND EACH WAS THIS PR'S OWN HEADLINE
FAILURE. The injected fakes took no parameter, so nothing could observe the argv `main`
builds — the seam between the two modules this change exists to wire together. Dropping
`--narrowed` from the gate argv makes every narrowed run hard-fail both the floor and
the on-disk ledger; dropping `--outputFile.json=` makes every run report "does not
exist" plus the whole abort diagnosis; dropping `...argv` makes
`pnpm test:component <file>` silently run the entire suite behind a healthy-looking
ledger. Testing `isNarrowed` in isolation cannot see whether its answer is ever USED.
The fakes now capture their argument and three cases assert it; all three mutants die.

Also:
- `--repo-root` with no value was a silent fall-through to the real repo root, so a
  fixture report got graded against the 201 real files and failed with a confident
  diagnosis about the include breaking — produced by a typo. Now a usage error (exit 2).
- CONTRIBUTING: the sample ledger omitted `measured <date>`, which the real output
  carries; and "skips both checks but not the zero check" counted a different pair than
  the two enumerated four lines above. The checks are now numbered and the sentence
  names which ones a narrowed run skips.

Verification: 39 unit tests (was 35). Mutation-checked at 19 mutants, ALL KILLED, zero
survivors — the three that survived round 3, the round-3 fixes themselves, and every
guard from rounds 1 and 2 re-run, because an audit fix resets the gate. Real-tree
controls re-run: 201/201 green, one missing file still fails. ESLint clean on the
changed test file.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

* fix(tests): close audit round 4 — replace the flag LIST with a shape rule

Round 4 measured `isNarrowed` at base vs head across every one of vitest 4.1.11's 72
boolean options and 73 value-taking ones, and found the flag list had traded 22 loud
wrong answers for 25 quiet ones. Splitting `coverage` into two subkeys left
`--retry.count 2`, `--browser.name chromium` and sixteen more `coverage.*` paths
reading their VALUE as a filename, scoring a FULL run as narrowed and switching the
file ledger and the floor off with a one-line note. That is the direction this file's
own comments repeatedly name as the worse of the two.

🔴 THE LIST WAS THE PROBLEM, AND THREE ROUNDS WERE SPENT ON IT. Enumerating flag NAMES
missed `--test-timeout` next to `--testTimeout`. Canonicalising spellings then made
`--coverage <file>` swallow the file. Splitting into subkeys produced the 25 above.
vitest 4.1.11 has a 164-path option tree; a hand-maintained copy of it is wrong the day
it is written, and each fix moved the wrongness rather than removing it.

So the rule is now about the ARGUMENT, not the flag: a positional is a file filter if
it looks like a path, or if nothing before it could have been expecting a value. A
non-path token straight after a flag is that flag's value, WHATEVER the flag is —
correct for all 73 value-taking options without naming one of them, and correct for all
72 booleans too. `VALUE_FLAGS` is deleted. What remains is `NARROWING_FLAGS` (flags that
genuinely shrink the run and must be named, because omitting one fails LOUDLY) and a
five-entry `PATH_VALUE_FLAGS` for the only residual the shape cannot decide: a value
that is itself a path.

🔴 AND THE DECISION IS NOW A SENTENCE, NOT A BOOLEAN. No rule over an unknowable flag
list is right always; what must never happen is being wrong SILENTLY, because
`--narrowed` disables the two checks this whole change exists to add. `narrowingReason`
returns why, and the runner prints it: `--retry.count 2` reading `2` as a file filter is
obvious on sight and invisible otherwise.

Also:
- `canonicalFlag` camelCases only the FIRST dot segment, matching cac's own
  `camelcaseOptionName` (`name.split(".").map((v,i) => i===0 ? camelcase(v) : v)`).
  Camel-casing all of them made this wrapper accept `--coverage.reports-directory`,
  a spelling vitest does not — so the two would disagree about the next token.
- `--repo-root=<dir>` is parsed. Matching only the space form left the inline spelling
  falling through BOTH branches to the real repo root — byte-for-byte the failure the
  missing-value guard was added to close, reachable through one extra character. A
  value that is itself a flag is rejected too.

Verification: 45 unit tests (was 44 after the round's own additions; 39 before).
Mutation-checked at 23 mutants, ALL KILLED, zero survivors, each by its OWN named test.

🔴 THE FIRST SWEEP REPORTED FOUR SURVIVORS AND WAS WRONG ABOUT ALL FOUR, WHICH IS WORTH
RECORDING BECAUSE THE HARNESS HAD THIS PR'S OWN DEFECT. Two were mislabelled — killed by
a different test than the one named, which the sweep scores as SURVIVED. Of the other
two, one was a real gap (`--reporter=json AppNameCrumb`, now covered) and one was
`canonicalFlag`'s dot handling, which is equivalent for every input that has no dashed
subkey (also now covered). Along the way the harness itself was found reusing a JSON
report path without clearing it — the same stale-report defect fixed in the product two
commits ago — and now clears it and verifies the mutation reached disk before running.
A mutant reported SURVIVED is a claim about the instrument until the instrument has been
controlled.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

* wip(tests): stop guessing which vitest flags take values

Safety commit of in-progress round-5 rework so it is not lost; the agent was
interrupted mid mutation-sweep. Verification is NOT complete — the sweep, the
red/green pair and the merged-tree re-run have not been reported. Do not merge
on this commit.

Round 5 measured both prior approaches against vitest 4.1.11's real option
table (170 long options, 74 boolean, 96 value-taking): the hand-maintained
list was wrong 73 times, the shape heuristic 74. The error count never moved,
only its direction. This removes the question instead of answering it.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01AEa6GDJyTiu2R146ndYsLK

* test(ci): finish the verification bacb8a2cf5 was pushed without

bacb8a2cf5 landed as a rescue commit marked `wip` because the session that wrote it
died with the change only in a working tree. The change itself is unaltered; this is
the verification it was missing, plus the merge of a main that moved twice underneath
it. Nothing here modifies the rule.

WHY THE RULE CHANGED, WITH THE NUMBER THAT JUSTIFIES IT. Round 5 of the audit
enumerated vitest 4.1.11's REAL CLI option table -- by calling `createCLI()` and reading
each cac option's `isBoolean` rather than by hand -- and ran `isNarrowed(['--<flag>',
'VALUE'])` against both revisions imported side by side:

  hand-maintained flag list   wrong on 73 options (every value-taking one)  QUIET
  shape heuristic             wrong on 74 options (every boolean one)       LOUD

73 versus 74. The heuristic did not beat the list; it moved the wrongness off one half
of the table onto the other. The direction improved, which is worth something, but the
error count did not -- and `pnpm test:component --coverage AppNameCrumb` would run one
test and then be failed against the 1240 floor with "the include broke or the run died".
That is a mis-posed question, so it is no longer asked: any argument at all means
narrowed.

Re-measured against the same table on the merged tree, with the same method:

  164 long options (72 boolean, 92 value-taking)
  UNSAFE-LOUD -- rule claims FULL so the floor and ledger fire on a partial run: 0
  no-argument invocation (what CI runs): NOT narrowed, so all three checks arm

(My enumeration walks the global + `run` commands and sees 164/72/92 where round 5's
saw 170/74/96; the traversal differs slightly, the conclusion does not.)

🔴 The comparison is one-directional ON PURPOSE, and the other direction is a real cost
rather than a rounding error: all 164 options now score as narrowed, so an arg-ful run
does not get the floor or the file ledger even when it was genuinely full. That is
affordable for one measured reason -- `pr-preview-pipeline.yaml` invokes
`pnpm run test:component` with NO arguments, so CI is the `argv.length === 0` path and
always gets all three. What is given up is enforcing a floor on an ad-hoc local run.
`VITEST_MAX_WORKERS=4` in the environment sizes a run without giving that up.

Verification on the MERGED tree (d77dd394db), all at load <= 16 with no browser-session
errors in any run quoted:

  no-arg full run (the CI path)   202 files / 2257 tests, exit 0, no NARROWED line,
                                  floor 1240 and the 202-file ledger both ARMED
  guard red arm                   exit 1, "THIS RUN PRODUCED NO ACCOUNTABLE RESULT",
                                  failing on the missing-report branch
  guard green arm                 exit 0, "2 executed, 0 skipped, across 1 files"
  gate unit tests                 37 passed
  mutation sweep                  20 mutants, ALL KILLED, zero survivors

🔴 The red arm was run NARROWED, deliberately: it proves `--narrowed` disables the floor
and the ledger and NOT the zero-collected check, which is the one failure this whole
change exists to catch.

THE MERGE. main moved twice; both times the only conflict was `package.json`, and both
times it was the same semantic hazard -- main editing `test:lint-rules` and this branch
editing `test:component`, adjacent lines of one object, where taking either side
wholesale silently reverts the other and makes the entire guard inert with every test
green. Resolved by parsing the merged JSON, not by reading the diff: `test:lint-rules`
now matches origin/main byte-for-byte and `test:component` is the wrapper. The wiring
guard added for exactly this was then checked against the bad resolution -- applying
main's `package.json` wholesale fails one test, the one written for it.

Round 5's two remaining 🟢 items are closed by the rework itself rather than separately:
the stale JSDoc described `VALUE_FLAGS`, which no longer exists, and the test whose
description claimed "a BOOLEAN flag does not swallow the filename after it" while
passing entirely through the path check is gone with the heuristic it tested. That one
was load-bearing -- a description asserting coverage its body did not provide is what
let the class through -- so its replacement asserts a property with no free parameters
instead, across both halves of the option table.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

* style(tests): format the gate test file

The rescue commit was pushed before a prettier pass could run over the last edit to
this file, and `ESLint + Prettier (changed files)` caught it: the file is ADDED by this
PR, so it is covered by the added-files prettier gate. One line.

This red was MINE, not inherited — unlike `Unit tests (1)/(2)` (a redis integration
test that arrived from main) and `preview / smoke-tests` (#4516 changing
`reaction.toggle`), both of which are verified as pre-existing and are left alone.

CONTRIBUTING.md also reports unformatted, and that one is NOT actionable here: it is
already unformatted on origin/main, and it is modified rather than added, so the gate
(which reads `added.txt`) does not cover it. Reformatting it would bury this PR under an
unrelated whole-file diff.

---------

Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
2026-09-01 10:57:49 -05:00

12 KiB

Contributing to Civitai

Thanks for contributing. This document covers the things that are easy to get wrong here and hard to discover on your own — what CI actually checks, how to verify your work locally, and a few conventions that have bitten us before.

For getting the app running at all, see README.md.

Fork PRs: what CI does and doesn't check

civitai/civitai is public, and a pull_request from a fork gets no repository secrets. That is deliberate and is not going to change. The practical consequences:

  • Workflows don't start on their own. A fork PR parks at action_required until a maintainer clicks Approve and run workflows. If your PR looks like nothing is happening, that's why — please just ping in the PR.
  • Two things block. Typecheck (full repo), and ESLint errors + Prettier on files your PR adds. New files start clean, so holding them to the rules is free. Typecheck runs on fork PRs too — it needs no secret, because the event-engine-common submodule is public and fetched over HTTPS.
  • Everything else is report-only. ESLint and Prettier on files you modify run with continue-on-error, because a formatter has no changed-line granularity — 789 of 4,116 src files fail prettier --check today, so blocking on them would turn a three-line bugfix into a 200-line reformat. You will see a failed-but-ignored marker in the checks list. That's expected.
  • Unit tests run but don't block yet. CI runs the unit suite (676 files, ~8,900 tests) report-only while we establish its pass rate on a CI runner — a handful of tests are slow enough to trip the 60s per-test timeout under load, though they pass in isolation. So a red unit job is a signal to look, not proof you broke something, and a green overall check doesn't mean the tests passed. Read the job output.
  • Component tests run in the PR-preview pipeline, report-only. The 201 *.browser.test.tsx files need real Chromium; they do not run in GitHub Actions, but the in-cluster preview pipeline runs them and posts a preview / component-tests commit status. It never blocks, so read it rather than relying on it.

A green check on a fork PR therefore means much less than it looks like. Verify locally.

The event-engine-common submodule

The repo depends on a submodule at event-engine-common/. It is public and fetchable over HTTPS, but it is not checked out automatically:

git submodule update --init event-engine-common

Without it, pnpm typecheck fails with a wall of Cannot find module errors and several dozen test files fail to collect — noise that looks like your change broke something when it didn't. Same applies to any new git worktree; worktrees don't check out submodules for you.

If you cloned before the URL moved to HTTPS

Run this once:

git submodule sync --recursive

git submodule init writes the URL into .git/config the first time it runs and never overwrites it afterwards — including when that first attempt failed. So if you ever tried --init while the submodule was still private and got an auth error, your clone has the old SSH URL recorded and --init will keep failing with the identical error no matter how many times you pull. sync is the fix, and for that case it is required, not optional.

The recorded URL also lives in the shared config rather than per-worktree, so every existing and future worktree of that clone inherits it until you sync.

Verifying locally

All of these need the repo's own toolchain — node 24.19.0 (the version in .nvmrc) and pnpm 10.x. Nothing stops you running them on the wrong node: engines.node is advisory, so pnpm install prints WARN Unsupported engine and continues. That is why this is worth stating rather than leaving to the tooling — the wrong major shows up as spurious test failures attributed to your branch, not as an error at install time. On NixOS there is a second, louder failure: outside the dev shell there are no PRISMA_*_ENGINE_* paths, so Prisma goes looking for a linux-nixos engine that has never been published.

nvm use picks the right node up from .nvmrc; with Nix, prefix any of these with nix develop -c, or use direnv (cp .envrc.example .envrc && direnv allow).

pnpm typecheck                 # full repo
pnpm test:unit:run             # ~8,900 unit tests, node env
pnpm test:component            # ~2,250 component tests in real Chromium, slower
pnpm exec prettier --check <files you added>
pnpm exec eslint <files you added>

Use pnpm exec prettier --write <file>, not pnpm prettier:write -- <file>. The latter ignores the argument and reformats the entire repository.

Both suites use Vitest's own worker count (cpus - 1, or min(12, cpus - 1) for the browser one). To leave the machine usable while a suite runs, size it for that run with --max-workers=8, or with VITEST_MAX_WORKERS=8 in the environment. The number applies to every project and is not clamped, so a value above 12 raises the Chromium instance count rather than lowering it.

Compare against a baseline, not against zero

pnpm test:component can report two extra failing files on a cold optimizeDeps cache, both Vitest failed to find the runner. Adding any *.browser.test.tsx perturbs optimizeDeps.entries — see the comment at vitest.config.mts:98-124. Re-run and it settles. If a file passes in isolation, that's what happened.

The reliable method for any of these commands is to run it on unmodified main first, save the output, then compare. A failure that also happens on main isn't yours.

pnpm test:component fails on ZERO COLLECTED, not only on red tests

The browser suite has a failure mode that runs no tests at all and, until you read the numbers, looks exactly like an ordinary failure. pnpm test:component runs through scripts/test-component-run.mjs, which asks vitest for a JSON report and hands it to scripts/ci/assert-component-suite-ran.mjs. That gate prints a ledger and applies three checks:

test:component ledger: 2254 executed, 0 skipped, across 201 files; 0 failed suites,
0 failed tests (baseline 2254 tests / 201 files measured 2026-08-31; 201 on disk;
floor 1240)
  1. Nothing collected fails — always, on every invocation.
  2. A *.browser.test.tsx on disk that is absent from the report fails, and is named. A file that stops being collected reports as absence, so no failure count and no per-test list can show it.
  3. A floor on executed tests, as a backstop for a partial collapse that still leaves every file present.

The gate can only ever add a failure — vitest's own exit code is passed straight through otherwise.

Three things to know before you run it with arguments:

  • Any argument at all skips checks 2 and 3. Check 1 always applies. Passing anything — a filename, --shard, even --max-workers 4 — means what the run should have collected is not knowable from the arguments, so the floor and the file ledger are not asserted. Run with no arguments to get all three; that is what CI does. To size a run without giving up the checks, use the environment instead: VITEST_MAX_WORKERS=4 pnpm test:component. (This is deliberately not a parser. Deciding which vitest flags consume the next token was tried twice and lost twice — measured against vitest's real option table, a hand-maintained flag list was wrong on 73 options and the shape heuristic that replaced it was wrong on 74. The rule above cannot be wrong about any of them.)
  • --outputFile is refused (exit 2), in every spelling that would collide with the report the gate reads. Run pnpm exec vitest run --project component directly if you want your own report.

Why it exists: a vi.mock factory that throws is resolved inside a Playwright route handler that does not catch, so the rejection escapes as an Unhandled Rejection in the orchestrator and kills the whole run — no Test Files line, no per-file results, exit 1. The preview / component-tests tier reads only the exit code, so that abort and a genuine list of red assertions rendered identically. One such file zeroed all 201 suites on main at d353f785c3. A browser crash under host load produces the same shape, wearing the same misleading headline. The gate's message enumerates the causes it cannot tell apart; read the error printed above it.

The commonest cause is a wholesale factory that stops naming an export something in the file's module graph imports. local-rules/no-wholesale-module-mock guards that for a listed set of modules, and src/components/AppBlocks/__tests__/featureFlagsMockCompleteness.test.ts guards the feature-flags module — but only under src/components/AppBlocks, so neither covers you by default.

Where tests go

Never put test files under src/pages. Next.js treats every .ts/.tsx file there as a route — including nested __tests__/ directories — and next build runs a route-type validator over them. A Vitest file in that tree fails the build with Property 'default' is missing, and only next build catches it: typecheck, vitest and every CI job pass. It reaches the preview build before anyone notices.

Put handler tests in a __tests__/ directory outside src/pages (e.g. src/server/__tests__/) and import the handler through the ~/pages/... alias.

Database migrations

We do not use prisma migrate deploy. Migrations are applied by hand, per environment. Files in packages/civitai-db-schema/prisma/migrations/ exist for review and history; they are never auto-run, and the _prisma_migrations table is not a source of truth.

So: write the SQL, commit it, and say so in your PR description — a maintainer applies it. Don't suggest prisma migrate deploy or prisma migrate resolve.

Create migrations with pnpm run db:migrate:empty "brief description". They must land in packages/civitai-db-schema/prisma/migrations/, not the prisma/migrations/ directory at the repo root, which predates the monorepo and Prisma no longer reads.

Branching: no stacked PRs

Base every PR directly on main (or on a feature integration branch), never on another open PR's branch.

Stacked PRs mis-merge silently here: a squash-merged parent doesn't retarget its child, so the child lands on the orphaned parent branch instead of the real base and its changes vanish. This has cost us real work.

If your change depends on an unmerged PR, wait for it to merge and branch off the updated base, or fold both changes into one PR.

Scope and PR size

Smaller is genuinely better here — reviewer attention is the bottleneck. If a PR contains one clearly-correct one-line fix plus a larger feature, split it. The one-liner will merge in a day; the feature might take a week, and there's no reason for the fix to wait.

If you find a second bug while fixing the first, prefer a separate issue or PR over widening the one you're in.

Writing it up

A good PR description explains why the change is correct, not just what it does. Especially valuable:

  • What you verified, and how. "Ran X, got Y" beats "should work".
  • What you didn't change and why — deliberate omissions read as oversights otherwise.
  • Anything you're unsure about. Flagging a shaky assumption is more useful than quietly hoping nobody checks.

If you discover your description was wrong after opening the PR, correct it in a comment. That's a normal and welcome thing to do, not an admission of anything.

Comments in code

Bias toward none. Comment the non-obvious why — a rationale, tradeoff, gotcha, or workaround a reader can't recover from the code. Never narrate what the next line does, and don't describe the current behaviour of nearby code; that's exactly what goes stale. Comments aren't type-checked, so they rot silently.

Getting help

Open an issue, or join the Community Development Team.