mirror of
https://github.com/civitai/civitai.git
synced 2026-09-20 22:08:18 +08:00
fix/test-cache-virtual-ids
16 Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
 Zachary Lowden
|
4ede37c9c5 |
ci: re-pin BASE_TESTS to 37,857 — the unit shard gate is red on main (#4621)
* ci: re-pin BASE_TESTS to 37,857 — the unit shard gate is red on main `scripts/ci/assert-shard-ran.mjs` pins `BASE_TESTS = 22157`, measured 2026-08-25. The suite has since grown to 37,857 executed across 1,626 files, so shards 3 and 4 exceed the derived ceiling of 9,971 and the job fails with every test passing. This is red on `main`, not just on PRs: runs 33900980756, 33901008291 and 33901251166 all fail the same two shards on unrelated commits. A permanently-red gate trains people to click through, so it is re-pinned rather than worked around. Re-measured from run 33900980756: 7177 + 5769 + 14724 + 10187 = 37,857. Cause (2) confirmed against cause (1) by the script's own discriminator: the four counts are unequal proportionate shares (2.55x spread) and each shard ran ~4 min against a ~10.4 min full suite. `--shard` failing to reach vitest would instead give four near-identical full-suite counts at full-suite runtime. Also corrects two claims in the comment that are no longer true and that are the stated justification for the 0.3 / 1.8 multipliers: - imbalance was "~5% off the mean"; the heaviest shard is now +55% - ceiling headroom was "~1.7x"; it is now 1.16x vitest shards by file, so a few test-dense files skew a shard badly and the next one trips the ceiling well before suite growth would. Recorded in the comment: if this reds again on a green suite, balance the shards rather than raising the number again. Verified: the new band [2839, 17036] admits all four current shards, and still rejects a collapsed shard (0, 99) and a shard that ran the whole suite (37857). * ci: make the shard-gate tests follow BASE_TESTS instead of restating it CI caught what the previous commit missed: re-pinning BASE_TESTS turned `Unit tests (3)` and `(4)` green but broke `Unit tests (1)`, because scripts/__tests__/assert-shard-ran.test.ts carried a SECOND copy of the constant — `const BASE_TESTS = 22157`, under a comment saying it was "kept in step with BASE_TESTS in the script". Kept in step by convention, which is what failed. Three boundary tests asserted a stale band while the script itself was correct. The test now reads the constant out of the script source. Parsing rather than importing is deliberate: the script is a CLI that reads process.argv and calls process.exit at module scope, so an import would execute it. The regex throws a named error if the constant is renamed, so that fails loudly instead of silently falling back to a default. Also fixes the third failing test, which read `const count = 3000` under a comment claiming it was "computed rather than hardcoded so it follows BASE_TESTS". It did not follow it. When BASE_TESTS moved the interesting region slid from 1662..3324 to 4259..5679 and 3000 fell out of it. It is now derived from the two bounds, with an assertion that the region is non-empty so a multiplier change fails loudly rather than silently picking a midpoint that satisfies neither bound. Validated rather than assumed: - constant moved to 50000 -> 14/14 still pass (the tests follow it) - constant renamed -> throws the named error, 1 file failed - restored -> byte-clean, 14/14 pass * ci: retrigger preview checks (empty) |
||
|
Zachary Lowden
|
6de64ee5a8 |
fix(tests): unzero the preview / component-tests tier, and make a zero-collected run say so (#4531)
* fix(tests): unzero the component tier — one mock factory was aborting the whole run `preview / component-tests` was reporting `failure` on `main` while executing ZERO tests. Reproduced deterministically (3/3 at |
||
|
Zachary Lowden
|
2ba7ecf186 |
refactor(apps): remove the orphaned 5-star AppBlockReview system (#4501)
* refactor(apps): remove the orphaned 5-star AppBlockReview system
Thumbs (AppListingReview) supersedes it. The 5-star write form had no
reachable entry point: its only two hosts were the /apps/[appBlockId]
detail route (retired, now redirects to the store detail) and
AppDetailsModal, which is opened only from AppBlockCard, which renders
only inside MarketplaceBody / RecentlyOpenedAppsView -- and MarketplaceBody
has had no importer in app code since /apps swapped to
AppListingsMarketplaceBody. The page's own header comment already recorded
this and flagged deciding the form's fate as the follow-up; this is it.
Removed:
- appBlockReview.service.ts and its two suites
- the blue-buzz appBlockReview reward + its registration and suite
- blocks.upsertReview / listReviews / getMyReview / setReviewExcluded
(setReviewExcluded had zero call sites at all) + their zod schemas
- AppBlockReviews.tsx and its browser suite
- the Bayesian machinery that was exclusive to it in
block-registry.service.ts: the AVG/COUNT/SUM correlated subqueries,
bayesianRatingSortKey, getGlobalMeanRating and the
app-rating:global-mean cache tag
- avgRating / reviewCount from AvailableBlock and PublicAppDetail, and
the card's rating chip
blocks.listAvailable no longer offers a "rating" sort and defaults to
"popular". The keyset cursor drops its pinned-mean third field; the decoder
still tolerates a stale 3-field cursor so an in-flight page resumes rather
than 500ing.
MARKETPLACE SORT: the live store is unaffected. /apps renders
AppListingsMarketplaceBody, which reads appListings.listAvailable and
defaults to sort "top-rated" -- a Bayesian shrinkage over
AppListingMetric.thumbsUpCount/thumbsDownCount in app-listing.service.ts,
with its own LISTING_BAYES_PRIOR and its own
app-listing:recommend-global-mean cache tag. It never touched
app_block_reviews. Only blocks.listAvailable (the retired grid) read the
star table.
DATABASE: the DROP TABLE migration is committed for history and is NOT
applied anywhere. This project does not run prisma migrate deploy; a human
applies it per environment, after this ships.
Kept deliberately: MarketplaceBody / AppBlockCard / AppDetailsModal (the
documented one-line rollback path for /apps, minus the star bits) and
listAppInsiderUserIds, which now has no production caller but carries a
documented displayed-vs-capability asymmetry that is under test.
* refactor(apps): address audit round 1 on the AppBlockReview removal
Four audit findings, none deploy-blocking.
F1 — the tolerant 3-field cursor decode was documented as a live back-compat
path ("an in-flight page resumes instead of 500ing"), and its guard did not
actually test that. Both halves are corrected:
- The comment now says what is true. A 3-field cursor was only ever minted by
the `rating` sort, and a client resuming that view sends `sort: 'rating'`
with it — a value `marketplaceSortSchema` no longer accepts, so zod rejects
at the router before the decoder runs. The tolerant split is defence in
depth over a door the sort removal already closed, not the thing keeping
that page alive.
- The guard is now real. It asserted `items.length` off a mocked return plus
a SQL-shape regex, but `capturedSql()` exposes only the assembled string
with `?` placeholders — the bound values are exactly what it cannot see, so
nothing could observe the decoded `cursorId`. Added `capturedValues()` and
asserted the parsed resume tuple directly.
Mutation-checked: rewriting the decoder to `decoded.slice(sep1 + 1)` (which
concatenates the dead mean onto the id and resumes at the wrong tuple)
SURVIVED the old guard and now dies on this guard's own assertion —
`expect(values).toContain('ab_5')`, 1 of 19 tests failing, the other 18
still green so the kill is attributable to this assertion rather than to
some other test's error.
F2 — the migration header undercounted the read paths and did not say that
applying it forecloses the revert. It named two; three queried the table at
base (listAvailable, getAppDetail and getFeaturedBlocks all projected
avg_rating off it). Header now states plainly that running the DROP makes the
PR-level revert a one-way door, and explicitly does not conflate that with the
store-grid rollback note, which stays safe.
F3 — regenerated the direct-mock allowlist with
scripts/test-perf/gen-mock-allowlist.mjs, dropping the entry for the test file
this PR deletes. The regeneration also sweeps in 43 entries that had accrued on
main since the file was last generated (all 43 exist at the base commit; the
canonical list is byte-identical, and totals are self-consistent again).
F4 — made the DROP's precondition executable instead of a comment asking a
human to run a COUNT. A DO block raises and aborts when the table is non-empty.
It carries its own to_regclass existence check so a re-run against an
already-dropped table stays a no-op, matching the IF EXISTS guards below it;
the COUNT is dynamic so it is never planned when the table is absent.
Verified against a throwaway PostgreSQL 17.10 cluster in all three states:
table absent (exit 0, no-op), table empty (exit 0, dropped), table with 3 rows
(exit 3, aborts with the row count, and the table, its rows and all three
indexes survive intact).
Full unit suite: 1519 files / 23963 tests passed, 0 failures.
typecheck: 0 errors. typecheck-tests-gate: 1030 errors/190 files at base ->
1010/188 here, unchanged by this commit.
* fix(db): make the app_block_reviews DROP guard hold under a plain psql -f
Self-review of the F4 guard added in the previous commit found it was only
effective under specific psql flags, which defeated its whole purpose.
A RAISE stops the rest of the FILE only when psql was invoked with
`-1`/`--single-transaction` or `-v ON_ERROR_STOP=1`. Under a plain
`psql -f migration.sql` — an entirely reasonable way to apply a
hand-applied migration — psql runs each statement in its own implicit
transaction and continues past the error: the guard raised, and the DROPs
then executed anyway.
Measured against a throwaway PostgreSQL 17.10 cluster before the fix: the
table and all 3 of its rows were destroyed and psql still exited 0. Silent
and total — the guard read as protection while providing none in that mode.
Wrapping the guard and the DROPs in one explicit BEGIN/COMMIT closes it. The
RAISE poisons the transaction, every later statement is rejected with
`current transaction is aborted`, and the COMMIT degrades to a ROLLBACK.
Re-verified across all 9 combinations of {plain `-f`, `-1`,
`-v ON_ERROR_STOP=1`} x {3 rows, empty, already dropped}: the table and its
rows survive every non-empty case, and the empty and absent cases drop and
no-op as intended. The header now also notes that without ON_ERROR_STOP psql
exits 0 even when the guard refused, so the exit code is not a usable success
signal and the operator should read the output.
* fix(db,test): stop the DROP guard hijacking the caller's transaction
Round 2's delta re-audit found that round 1's own fix introduced a worse
defect, plus one assertion that could not see the mutant it was written for.
1. The migration no longer issues BEGIN;/COMMIT;. Wrapping the guard and the
DROPs in an explicit transaction closed the plain-`psql -f` fail-open, but a
file that commits a transaction it did not open hijacks the caller's
transaction state, and that fails destructively in two shapes an operator
is likely to use:
- `BEGIN; \i this; \i next; COMMIT;` under ON_ERROR_STOP=1 (the header's
own recommended invocation, applied to more than one file). Our COMMIT
ends the operator's transaction early, so when the second file fails
the DROP is already committed and the table is gone anyway, together
with any unrelated row the operator had written in that transaction.
- `\set AUTOCOMMIT off` (the pgAdmin/DBeaver/Retool posture; Retool is a
documented apply path here). The operator's ROLLBACK; prints only
WARNING: there is no transaction in progress, exit 0.
It also silently defeated `psql -1`.
The two DROP INDEXes and the DROP TABLE now live inside the existing DO
block as EXECUTE statements. A lone DO block is one statement and therefore
atomic by itself, so the fail-open protection is retained without the file
taking any position on the caller's transaction state.
2. The legacy-cursor test pinned the bound values as a multiset, not a tuple.
toContain is order-blind, so a decoder that transposes its two return
fields still binds both required strings, merely swapped into the wrong
sides of (sort_key, ab.id) < (?, ?) -- a wrong page boundary, i.e. the
silent skip/duplicate the guard exists to prevent. That mutant survived all
19 tests. The assertion now pins the ordered tail of the bound array.
3. The header cited PostgreSQL 17.10. Production runs 18.3.
Verification, all on PostgreSQL 18.3 (server_version_num 180003, the
production server version):
- 15-cell invocation matrix over {plain -f, -1, -v ON_ERROR_STOP=1, a
failing-second-file batch, AUTOCOMMIT off + ROLLBACK} x {3 rows, empty,
already dropped}: rows survive every non-empty case, empty/absent drop and
no-op, and no case leaves the caller's transaction in a state it did not
choose.
- Negative control first: the guard-stripped file destroys all 3 rows under
-f, -1 and ON_ERROR_STOP=1, so the harness can observe destruction.
- The BEGIN;/COMMIT; draft reproduces the audited defect on 18.3: table
dropped-and-committed with the operator's pending row committed alongside
it, in both nested shapes.
- Transposition mutant: 1 failed / 18 passed, failing on the new tuple
assertion; the same mutant passes 19/19 against the previous toContain
form. The author's original mutant (dead mean concatenated onto the id)
still fails, on the same assertion.
- pnpm typecheck: 0 errors. typecheck-tests-gate (already red on main):
base 1030/190 vs HEAD 1010/188, the 68 unbaselined files identical.
* style(test): prettier-collapse the new cursor-tuple assertion
Formatting only, on the three lines added by the previous commit -- prettier
--check flagged them and CI runs that gate on changed files. Re-verified after
the reformat: 19/19 pass, and the transposition mutant still dies on this
assertion (1 failed / 18 passed, line 329).
* docs(db): correct four false or stale claims in the DROP migration header
Prose only. The SQL is byte-identical: stripped of comments and blanks, the
file matches
|
||
|
Zachary Lowden
|
afe84cf7c8 |
ci: shard the unit suite across 4 runners, 10.4m -> 3.8m (#4392)
Measured from the Actions API (n=12): the job was 555.5s of test work plus 66.5s of fixed overhead = 622s, the 10.4m median, and 3.4x the next-slowest job. T(N) = 66.5 + 555.5/N puts N=4 at ~3.4m. Measured after: 10.3m -> 3.8m on the critical path (2.7x) at +36% runner-minutes, reproduced across two runs with a 1.12-1.14x shard spread. N=4 rather than more because `App unit tests + typecheck` is 2.9m — past N=5 sharding optimises something that is no longer the bottleneck. Ships with a positive control, `scripts/ci/assert-shard-ran.mjs`. Sharding adds a failure this job did not have: a `--shard=i/N` selecting no files exits 0 and reports green, and four green checks look identical whether they ran 22,000 tests or none. The guard asserts per shard that work happened, counting EXECUTED assertions rather than `numTotalTests` (which includes skipped, so an all-self-skipping shard would pass a total-based floor), and its bounds scale with `total` so a change to the matrix does not trip them. It earned its keep immediately: on the first run it failed all four shards while `Unit tests` reported SUCCESS. `pnpm run … -- --shard=…` forwards the `--` literally, vitest DISCARDS everything after it, and so `--shard` and `--outputFile` were both silently dropped — each runner executed the entire suite (7m30s-9m54s per step) and wrote no report. Without the control this would have merged as four green checks that tested nothing verifiable. A subsequent adversarial audit found three further claims that measurement did not support, all fixed here: bounds hardcoded off a ten-day-stale suite size that would have tripped within weeks and blamed the wrong cause; a comment asserting post-`--` args become filename filters when they are discarded; and a "mutation-verified" claim that five of twelve mutants walked through. Four are now covered by named cases; the fifth was unreachable dead code and is removed rather than papered over with a test that could not kill it. Final sweep, re-run after the Prettier pass: 10 mutants, 10 killed, each by its own named test, with a no-op negative control that correctly survived. Suites 20/20. Verified live across two runs; the typecheck-scripts gate was confirmed to have executed rather than assumed green. Not verified: behaviour on a push to `main`, where `continue-on-error` is false and a red shard renders as a genuine failure. That path only exercises on merge. |
||
|
briant
|
6c9904b389 |
fix(tests): unbreak the unit suite on Windows — 16 failures across 7 files, and two of them could never pass
Measured on a Windows checkout: 7 files / 16 tests fail identically before and after any local
change, so every Windows developer has to hold "these are expected" in their head to read a suite
result. 868kubhjr tracked four of them; the set had grown to seven.
Five are the separator bug that ticket describes — `path.relative` yields backslashes and the
assertion compares against forward-slash literals — fixed with `.split(path.sep).join('/')`, the
prescribed shape. `standalone-boot-graph` is the same fault in a Node error string, so it matches
`/esm[\/]index\.js/` instead of a literal.
Two were NOT cosmetic, and would have kept failing after the separator fixes:
* `appListingStatChips` read `new URL(…).pathname`, which is `/C:/…` on Windows, so `readFileSync`
resolved it against the drive root as `C:\C:\…`. Now `fileURLToPath`.
* `dev-server-daemon-port` spawned `await import("C:\…")`, which node rejects with
ERR_UNSUPPORTED_ESM_URL_SCHEME — it drives a child and asserts exit 0, so on Windows it could
never observe the property it claims. Now `pathToFileURL`. Verified the harness is live again:
the real daemon with a bad port exits 0, a deliberately throwing module exits 1.
`source-nul-bytes` needs symlinks, which Windows refuses without SeCreateSymbolicLinkPrivilege.
Junctions need no privilege and the walk cannot tell them apart (`lstat().isSymbolicLink()` is true,
dangling ones included), so the three controls still run rather than skipping.
`typecheck-apps` needed the script itself. `spawnSync` skips PATHEXT so a bare `pnpm` is ENOENT on
Windows, and naming `pnpm.cmd` is EINVAL — node refuses to spawn a .cmd without a shell. Hence
`shell` on win32 only; ubuntu CI takes the false branch and is byte-identical.
That shell brings a hazard, so it does not arrive alone: `shell: true` applies NO per-argument
quoting, and `app.name` is read raw from `apps/*/package.json`. A name containing `&` ends the
command under cmd.exe, so cmd returns the trailing command's exit code and a genuinely-red typecheck
reports 0 — reproduced, and precisely the reports-success class this script exists to close. Names
are now validated with a hard error.
The suite's fake pnpm answered by exit code alone, so it could not see WHAT the script asked pnpm to
run: mutating the spawn to `--filter totally-wrong-<dir> run lint` left all tests green. It now
echoes argv and one case asserts it, which also pins argv fidelity through cmd.exe.
Windows unit suite: 16 failures -> 0. Nothing changes on ubuntu CI.
Closes 868kubhjr
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
||
|
Zachary Lowden
|
69b5ce5497 |
fix(typecheck): typecheck scripts/ — 15 non-test files now covered, scripts/__tests__ excluded like src/ (#4189)
* fix(typecheck): close the scripts/ blind spot — 31 files now checked, 5 quarantined behind a ratchet The root tsconfig named `scripts/local-dev/*.ts` in BOTH `include` and `exclude`. Exclude wins, so nothing under `scripts/` was typechecked at all — not even the one directory the include list appeared to name. Measured on |
||
|
Zachary Lowden
|
b8f84204d7 |
fix(auth): fix the two type errors gating apps/auth, and bring it into the CI typecheck gate (#4188)
apps/auth was the last app excluded from `scripts/ci/typecheck-apps.mjs` (#4156), on two pre-existing errors. Both are fixed at the root, not silenced, and the exclusion is gone — the gate now covers all 7 apps. providers.ts:165 — Parameter 'p' implicitly has an 'any' type The stub entry's key is a COMPUTED property (`['stub' as ProviderId]`) whose type is the whole ProviderId union rather than one literal, so TS cannot match it to a member of `Record<ProviderId, ProviderDef>` and drops contextual typing for the value. `satisfies ProviderDef` restores it. Measured, both arms: with `satisfies`, misspelling a required field (`scope` -> `scopes`) is caught; with it removed, the same misspelling is silently accepted and only the implicit-any resurfaces. So the whole entry was unchecked against ProviderDef, not just that one parameter. establish-session.test.ts:100 — Property '_store' does not exist on type 'never' The Cookies stub was cast `as never` to satisfy establishSession's parameter, but `never` has no properties, so reading `_store` back in an assertion was itself an error. Cookies has exactly five members, so the stub now implements all of them and needs no cast at all. Removing the exclusion breaks the guard suite, which is the real work here: six of its eight cases put an `auth` app in every fixture purely to satisfy the stale-exclusion guard, so an empty map made them fail for reasons unrelated to what they test. Rather than delete those guards, the exclusion map is now a parameter of an exported `runTypecheckApps({ excluded })`, and the tests drive it with a synthetic app. Deliberately a function parameter and not an env var: an env-var override would be a live way to un-gate an app in CI, which is the failure class the script exists to close. All ten cases now pass regardless of what the shipped map contains. Also neutralises the "six apps" counts in the script header and workflow comment, which this change would otherwise make wrong, and applies prettier to two pre-existing unformatted lines in the touched files. |
||
|
Zachary Lowden
|
23cecb57c0 |
ci: run per-app typecheck scripts in CI (#4156)
* ci: run per-app typecheck scripts in CI
The root tsconfig.json include is:
scripts/local-dev/*.ts src packages/*/src tests .next/types/**/*.ts
No apps/ entry, so `pnpm run typecheck` does not reach any sibling app.
Each app defines its own typecheck script but none were run by any CI job.
Six of seven apps pass today; wire them up as steps in the existing `apps`
(App unit tests) job, which already has the workspace installed.
app script result
event-engine tsc --noEmit -p tsconfig... clean
notifications tsc --noEmit clean
orchestrator-gateway tsc --noEmit clean
storage tsc --noEmit clean
creator-studio svelte-check --tsconfig ./... clean
moderator svelte-check --tsconfig ./... clean
auth svelte-check --tsconfig ./... 2 errors + 1 warning
apps/auth is excluded from the CI wiring. Its pre-existing errors:
src/lib/server/auth/providers.ts:165
Parameter 'p' implicitly has an 'any' type.
src/lib/server/auth/__tests__/establish-session.test.ts:100
Property '_store' does not exist on type 'nev
src/routes/login/+page.svelte:28
state_referenced_locally (warning)
Ticket: 868kt8pfu
* ci: make the app typecheck prove it ran, and report every failing app
Review follow-up on the six `pnpm --filter <pkg> run typecheck` steps. Two ways they could
report SUCCESS while checking nothing or hiding work, plus a blame problem.
🔴 `pnpm --filter <name> run <script>` EXITS 0 WHEN THE FILTER MATCHES NOTHING. Measured on
pnpm 10.28.1: a bogus package name prints "No projects matched the filters" and returns 0.
Six hardcoded package names are six chances for a rename to turn a gate into a green no-op
— the same shape as `prettier --check "$FILES"` reporting "All matched files use Prettier
code style!" across zero files. Nothing in the six steps could notice.
🔴 A NEW app under `apps/` is simply absent from a hardcoded list. The gate stays green and
the app is unchecked — this PR's own hole, reopened by the next person to add an app.
So the app set is now a LEDGER READ FROM DISK: every `apps/*` with a `typecheck` script must
run, each run must be proven to have selected a real package, and `auth`'s exemption is an
explicit entry that hard-errors if it goes stale. Adding an app wires it automatically. This
mirrors `scripts/ci/assert-workspace-suites-ran.mjs`, which exists for the same reason on
the vitest side.
Failures are COLLECTED rather than fatal on the first. Six sequential steps abort the job at
the first red app, so a shared type change breaking four of them reports one.
The job is renamed `App unit tests` -> `App unit tests + typecheck`. The typecheck steps were
added under the old name, so a type error would have rendered in the checks list as a failing
unit-test job, pointing the reader at the wrong suite. Safe: neither `main` nor `release` has
any required_status_checks — re-verified 2026-08-20 against the branch-protection API rather
than inherited from the note already in this file.
Kept in the `apps` job on purpose: `pnpm install` is the expensive part and this job has
already paid for it. A matrix would give parallel legs and per-app check names for the price
of six installs; the rename plus collected failures buys most of the legibility for free.
Eight guard tests in `scripts/__tests__/typecheck-apps.test.ts`, each driven against a stub
apps/ tree with a fake `pnpm` on PATH so the no-match and failure paths are exercised for
real: happy path, stale filter caught, red app, failures collected not masked, empty
discovery refused, new app auto-wired, stale exclusion hard-errors, auth excluded while the
rest still run. All 8 pass; the first draft had six failing for the wrong reason (fixtures
omitted `auth`, so the stale-exclusion guard fired first) — fixtures fixed, not the guard.
Two things deliberately NOT changed:
- The Svelte apps' bare `typecheck` script is fine as-is. Their tsconfig extends
`./.svelte-kit/tsconfig.json`, absent from a fresh checkout, which looks like a vacuous
green waiting to happen. It is not: `"prepare": "svelte-kit sync"` runs during
`pnpm install`. And if it ever stops, svelte-check FAILS LOUDLY rather than passing —
verified by moving `.svelte-kit` aside, which gave exit 1, "Cannot read file
.svelte-kit/tsconfig.json", `1 FILES 1 ERRORS`. An earlier review comment of mine claimed
it would go silently vacuous; that was wrong, and switching to `check` is unnecessary.
- `scripts/__tests__/` is outside every typecheck program (root `include` carries only
`scripts/local-dev/*.ts`), so this new test file is unchecked — as are the 12 already
there. Not introduced here; it is the `scripts/` half of this ticket's widening.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* style: prettier the two new files
The `Prettier (added files)` step is BLOCKING by design — a new file has no pre-existing
findings, so holding it to the rules is free. Both new files failed it and I did not run the
formatter before pushing. Reproduced locally with `prettier --list-different` (both listed),
fixed with `--write`, and re-ran the 8 guard tests after the reformat: still 8/8, so the
formatting did not disturb the fixture strings or the fake-pnpm heredocs.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
---------
Co-authored-by: DevPod Agent <agent@devpod.local>
Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
||
|
Zachary Lowden
|
f419a64461 |
chore(deps): delete the inert next@16.3.1 patch and correct the comments about it (#4086)
Next 16.3.1 is the first release containing upstream's SVG-loader fix (vercel/next.js#96681) — it ships 'VipsForeignLoadSvg' in the sharp.unblock list itself, in both the CJS and ESM image-optimizer copies. Our local patch was therefore inert: the installed files are byte-identical (sha256) to the published tarball, and the patch does not even apply (its hunk context wants Png immediately followed by Tiff; 16.3.1 has Svg between them). It looked healthy because pnpm silently no-ops an already-applied patch — install exits 0, prints no warning, and still creates a patch_hash= virtual-store directory. Deletes the patch and its patchedDependencies entry (@mantine/hooks untouched), regenerates the lockfile, and corrects three comments that asserted upstream still lacked the entry. The CI guard stays: it asserts the outcome (the installed Next unblocks the SVG loader), so it keeps protecting against a future Next regressing this the way 16.3.0 did. Verified post-removal: guard green, still goes red when the loader entry is stripped from both installed copies, and /api/og renders PNGs on both paths on the preview build. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
Zachary Lowden
|
c78f85dd5a |
fix(deps): Next 16.3.0 → 16.3.1, and make the compiled-branch gate hard (#3983) (#4075)
* fix(deps): Next 16.3.0 -> 16.3.1, and make the compiled-branch gate hard (#3983) This is the fix for #3983. The defect was in the bundler, not in our source. Turbopack's value analyzer in 16.3.0 models a bare `return someAsyncFn()` tail call as `Promise<Promise<T>>`. That is always truthy, so a caller that `await`s it is analysed as always-true and every statement after the resulting conditional is eliminated as dead code. `isAppListingsEnabled` ends in exactly that shape, which is why `resolveStoreVisibilityScopeUninstrumented` lost two of its three returns, fell off the end, and produced `undefined` for every non-privileged caller — served as the whole catalog on one read path (`?? 'full'`) and as an empty store on the other (`?? 'none'`). Upstream: vercel/next.js#96601 "[turbopack] Collapse nested promises in the analyzer", backported as #96675, shipped in 16.3.1. MEASURED, not inferred. Two production builds of THIS commit on one machine, same Node 24.19.0, differing only in the pinned Next: 16.3.0 async function S(e){if(await p(e))return"full"} 16.3.1 async function w(e){return await c(e)?"full":await y(e)?"public-external":"none"} Both read out of the emitted `.next/server` chunks by source-map attribution and identified by their source neighbour `STORE_SCOPE_FLAGS`, never by minified name. Note the fixed form is a TERNARY — `grep 'return"public-external"'` returns zero on the FIXED build too, which is why the gate reads source maps. `package.json` already allowed 16.3.1 (`^16.3.0`); only the lockfile pinned 16.3.0, so the substance here is the lockfile. The floor is raised to `^16.3.1` so a fresh resolution cannot land back on the broken compiler. `patches/next@…` is renamed and its `patchedDependencies` key updated — that patch is the unrelated libvips/SVG one-liner (vercel/next.js#96681), it still applies cleanly, and 16.3.1 still does not carry the loader entry upstream, so it stays. `--warn-only` is removed from `scripts/assert-compiled-branches.mjs` in the same commit. It existed only because the 16.3.0 build genuinely violated the gate, and a permanently-red gate trains everyone to click through. Keeping the bump and the strictness atomic means the gate's strictness always matches the toolchain: a revert of the bump turns it red instead of silently passing. Verified on this commit: - gate exit 0 (hard, no --warn-only) against the 16.3.1 build - gate exit 1 against a 16.3.0 build of the same tree — watched red - `scripts/ci/assert-next-svg-patch-applied.mjs` OK on both installed copies - unit suite 1149 files / 18,123 tests passed, 0 failed Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(build): ship @swc/helpers' module-sync branch into the standalone image (#3983) The bump built clean, passed every source-level gate — unit suites, typecheck, ESLint + Prettier, schema drift, the event-engine pin, and the now-hard compiled-branch gate — and the container could not boot: Error: Cannot find module '.../@swc/helpers/esm/_interop_require_default.js' at ... next/dist/server/require-hook.js code: 'MODULE_NOT_FOUND' Same shape as the defect this PR exists to fix: a correct source tree producing a broken artefact, invisible to everything that reads source. ROOT CAUSE, measured on the published artefact rather than inferred. `output: 'standalone'` does not ship node_modules; it ships the subset @vercel/nft traced. nft resolves a bare specifier under the `require`/`default` conditions. Node (>= 22.10) additionally honours `module-sync` for a CJS `require`. When a package's `exports` map points those two at different files, the build traces one and the running process asks for the other. next/dist/shared/lib/constants.js does `require('@swc/helpers/_/_interop_require_default')`, reached from the generated server.js via `next` -> config.js -> constants.js, i.e. before any application code. The relevant delta is not next itself but next's own dependency: next 16.3.0 next 16.3.1 @swc/helpers 0.5.15 0.5.23 ./_/_interop_require_default {import,default} {module-sync,webpack,import,default} require.resolve() under CJS cjs/...cjs esm/...js Both resolutions were RUN, not reasoned about. nft still traced the cjs file, so the published image carried that package as exactly cjs/_interop_require_default.cjs, cjs/_interop_require_wildcard.cjs and package.json — no esm/ directory at all. Adding only the missing esm/ directory to that exact image, nothing else changed, boots it: "Next.js 16.3.1 ... Ready". FIX. `outputFileTracingIncludes` force-includes BOTH condition branches of EVERY installed @swc/helpers copy — not the one file missing today, because which helper Next requires and which branch each resolver picks are upstream details that move. Globs are version- and hash-agnostic (`@swc+helpers@*`), plus a flat form for a hoisted layout. ~950 KB per copy. Verified on a local production build of this commit: both copies land in .next/standalone with complete esm/ (108 and 105 files) and cjs/, and next's virtual store links the 0.5.23 copy. Attached to three existing API-route keys rather than a `'**'` key. copyTracedFiles unions every entry's traced set into the single .next/standalone node_modules, so one entry carrying it is enough, while `'**'` would make all 572 entries read/parse/rewrite their .nft.json concurrently — 826 MB of JSON in one Promise.all — on a build already tuned against OOM. GATE, because a glob is a silent no-op once it stops matching. scripts/ci/assert-standalone-boot-graph.mjs runs in the Dockerfile's RUNNER stage: the first gate in that file to run against the runtime filesystem rather than the build tree, and the only one that can see this class of defect. It reads the GENERATED server.js for the specifiers that process requires at module scope and loads them in a child rooted at the shipped tree — no package, version, virtual-store path or patch hash hardcoded, so it keeps covering this after the next bump. Exit 2, never 0, when it cannot observe its input. It must run in the runner and not the builder: /app there is byte-for-byte what ships, whereas the builder's complete node_modules sits above .next/standalone on the resolution path and can satisfy a require the image cannot. Watched red and green on real artefacts, not only fixtures: - exit 1 with this exact MODULE_NOT_FOUND against the published broken image; - exit 0 against the same image with only the esm/ directory added; - exit 0 against the local production build of this commit, isolated from any parent node_modules; - exit 1 again after deleting exactly esm/_interop_require_default.js from that same local build. src/tests/build/standalone-boot-graph.test.ts pins the MECHANISM (a module-sync/default split with only the default branch present) rather than the package, and all 5 of its cases were watched to fail against a neutered gate. NOT VERIFIED. Nothing about production: this is only true of production once it merges, is promoted main -> release, is built and is serving. The gate covers the ENTRYPOINT's require graph; route chunks load lazily, so a condition mismatch reachable only from a route would still surface at request time. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * chore: retrigger preview The previous preview run (pr-preview-4075-b2wnw) never scheduled: its build-image and typecheck pods sat Pending with ExceededNodeResources for 82 minutes and the run hit the 1h30m PipelineRunTimeout. No verdict was produced — this was build-pool capacity contention, not a code failure. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
Justin Maier
|
4f619f0f39 |
fix(tests): clear the three type errors that reddened the typecheck-tests gate (#3956)
* fix(tests): clear the three type errors that reddened the tests gate `pnpm typecheck` excludes `src/**/__tests__/**`, so these landed green: - minimax-h3-license: `BaseModel` is re-imported, not re-exported, by `~/server/common/constants`. Take it from its origin. - cosmetic-phash: BigInt literals need ES2020; the repo targets ES2018. - sticker-placement: `createStickerPlacement` grew a required `spendType` and the shared fixture never did, so 17 call sites disagreed with it. The fixture carries 'yellow' deliberately — the escrow test asserts 'green', so a placement that dropped the caller's currency cannot pass by matching the fixture. Typing the fixture as `CreateStickerPlacement` means the next required field fails once, at the fixture, not at every call site. Baseline regenerated: sticker-placement leaves it entirely (16 -> 0), and remix-gallery drops 29 -> 28 from an unrelated merge. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * test(sticker-placement): make the currency guard structural, not a comment The escrow test's non-vacuity depended on the fixture default differing from the value it asserts. That was a comment; now the two are named constants and the test asserts they differ, so a future edit that collides them fails on the spot instead of going permanently green against a service that stopped forwarding the caller's currency. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
Zachary Lowden
|
dacc42e324 |
fix(ci): typecheck-tests gate matched __tests__ with the HOST separator (#3884)
`tsc --listFilesOnly` emits forward slashes on every platform, Windows included. The gate built its positive-control marker from `path.sep`, so on Windows the marker was `\__tests__\` and matched ZERO of the 948 `__tests__` lines tsc printed -- every run there died with `CANNOT MEASURE - POSITIVE CONTROL FAILED ... contains 0 file(s)`. `scripts/**/*.test.ts` is in the `unit` project's include, so 18 of the 134 cases in scripts/__tests__/typecheck-tests-gate.test.ts were red and `pnpm run test:unit:run` was RED on main for everyone on Windows. On Linux `path.sep` is `/`, so CI was green -- which is why it merged clean. Reported by a developer hitting it locally. Same class, second site: parseDiagnostics normalised diagnostic paths with `split(path.sep)`. Those strings are the keys of the committed baseline JSON and the input to `isGatedTestFile` (a `/`-anchored regex), so a backslash path was both unmatchable against the baseline and silently dropped as "outside src/**/__tests__/". Fix: one canonical separator, applied to the INPUT and never derived from the host. `toPosixPath` / `isTestFileLine` / `countTestFilesInProgram` move into the pure module -- the predicate previously existed only inline in a script reachable by spawning a process, which is why its platform assumption was never exercised. The `node_modules` exclusion normalises too; fixing only the marker leaves a control that passes by counting vendored test files. Neither module now reads `path.sep` in code. Coverage: the platform was a PINNED dimension -- every test ran on Linux, where the marker happened to equal the shape tsc emits, so no number of extra cases written the same way could have found this. The new cases never read `path.sep`; they name both separators via path.win32.sep / path.posix.sep and drive the gate end to end with a backslash-shaped `--listFilesOnly` listing, which reproduces the reporter's exact failure on Linux. 11 failed / 135 passed (146) before the fix, 146/146 after. 9-mutant battery: 7 breaking mutants died, both equivalent controls survived. Behaviour on Linux is unchanged: the gate reports the same 963 test files / 843 errors across 146 files with and without this change, and a --write-baseline regeneration is byte-identical under both. |
||
|
Zachary Lowden
|
a7d7c5da46 |
PROPOSAL (do not merge): pnpm typecheck is structurally blind to src/**/__tests__/ — 801 errors, and a ratchet to stop the inflow (#3868)
* proposal: pnpm typecheck cannot see any file under src/**/__tests__/ `tsconfig.json` excludes `src/**/__tests__/**`, so the typecheck is not lenient about test files — it is structurally blind to them. A planted type error in one is reported as "OK — 0 type errors", exit 0. Measured against main @f19574a9cb: 801 type errors in 141 of 870 test files (83.8% of the tree is clean). Zero of the 801 land outside a __tests__ directory and zero are TS2304, so the measurement config adds no artifacts of its own. The exclusion is not a perf decision: the line was added exactly once, in the same commit that added the repo's FIRST src/**/__tests__/ file, whose subject is about neither. Including tests costs +64s cold / +5s warm here. The debt is not legacy — 138 of the 141 dirty files are under three months old and 40 arrived in the first twelve days of August, so the gate's value is in stopping the inflow, not in ratcheting down sediment. Proposes a per-file ratchet in the shape this repo already uses for schema drift, with a proof of concept shown to pass on main, block on a planted error (naming the file, 801 -> 802), and pass again on revert. Nothing is fixed here and no existing file is modified; the PR is purely additive. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * proposal: make the typecheck-tests gate unable to report a confident zero The audit finding on this PR was about the INSTRUMENT, not the measurement. This gate's whole thesis is that a checker must not be able to report a confident zero — and this gate could, in four separate ways, all sharing one root cause: it inferred "clean" from "I parsed no diagnostics". Demonstrated live against this repository, not against a stub. Before -> after: TYPECHECK_HEAP_MB=abc (wrapper exits 2, no marker) PASS/0 -> exit 3 wrapper exits 127 (binary missing) PASS/0 -> exit 3 wrapper exits 134 (V8 heap abort) PASS/0 -> exit 3 tsc `pretty` output, 801 REAL diagnostics PASS/0 -> 801 across 141 --write-baseline while the check cannot run writes 0/0, exit 0 -> exit 3, file untouched The old "CRASH IS NOT CLEAN" control was SPELLED, not structural: it matched the literal string TYPECHECK CRASHED, which scripts/typecheck.mjs emits from exactly one of its four failure exits. The header comment claiming a crash could "never" read as 0 was false as written; it is corrected. Five structural controls replace it: an exit-status truth table (non-zero exit + zero parsed diagnostics is a FAILURE TO RUN, never a clean tree); a parse control (--pretty false forced, BOTH diagnostic formats parsed, ANSI stripped, and output containing `error TS` that parses to zero refused); a plausibility check (a zero against a non-empty baseline is refused, escape TYPECHECK_TESTS_ALLOW_EMPTY=1); a positive control that now runs through the SAME wrapper/-p/flags as the verdict run, with its floor derived from a test-file count stored in the baseline (938 -> 844) instead of a magic 400 that let 57% of the test tree vanish unnoticed; and a config-drift control asserting tsconfig.tests.json is the base exclude list minus exactly one entry. Also: the outsideTests predicate now matches the ACTUAL exclusion (src/**/__tests__/), so the 68 packages/*/src/**/__tests__ files — which are in BOTH programs — are no longer claimed by this gate. New: scripts/ci/typecheck-tests-compare.mjs (the pure half) and a 78-case suite in scripts/__tests__/typecheck-tests-gate.test.ts covering the branches the three proof-of-concept runs never touched: a baselined count rising, falling, going clean, deleted, renamed; two files sharing a basename; empty/malformed/absent/ wrong-config baselines; both diagnostic formats; every cell of the truth table. A pure `git mv` still BLOCKS by design — the gate cannot see file content — but it now names the probable former path instead of reading as a regression. Thirteen mutations, one per guard, were each confirmed to turn the suite red for THAT guard's cases; tree checksum-verified pristine afterwards. Measurement re-verified unchanged: 801 errors / 141 files, per-file map identical to the committed baseline, --write-baseline byte-stable across two runs. The three proof-of-concept runs re-run with real counts: PASS 801/141, BLOCKED 802/142 naming the planted file (while `pnpm typecheck` on the same tree says "OK — 0 type errors", 0 error TS lines, 0 mentions of the file), PASS 801/141 on revert. Doc: §2's history is CORRECTED. It claimed exif-parser.test.ts was "the first file the repository ever put in a src/**/__tests__/ directory" and that "nobody decided that test files should not be typechecked". Both are false — eight such files already existed at 1a7f89df98^, the earliest 2026-01-23, across six commits by different authors. The likelier reading, labelled as inference: the typecheck was already red with those files in the program and the exclude was a deliberate, undocumented silencing. That strengthens the ratchet recommendation rather than weakening it — deleting the exclude was effectively rejected once already. Still a proposal: no CI wiring, no attempt to fix the 801, tsconfig.json and `pnpm typecheck` untouched. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * proposal: close the four ways the gate's own self-checks could still be defeated A delta re-audit confirmed the previous round closed every finding structurally, and then found four ways the NEW controls could be defeated. Each is demonstrated before and after, against this repository rather than a stub. F3 (the CI blocker). TYPECHECK_TESTS_ALLOW_EMPTY=1 with a wrapper that measures nothing produced `0 error(s) across 0 file(s) (baseline: 801 across 141)` -> `141 file(s) now clean` -> PASS, exit 0, with nothing in the output recording that a control had been switched off. One env var in one job definition reinstated the exact failure this gate exists to eliminate. The hatch is now honoured on --write-baseline ONLY (so it cannot make a verdict run green), REFUSED rather than ignored on a verdict run, and requires a written reason instead of `=1` which is echoed into the output. Scoping it this way costs nothing: on the day the tree is genuinely clean the required action IS to regenerate the baseline, after which the control never fires again. before: rc=0, "PASS — no new or worsened test-file type errors" after: rc=3, "REFUSING TO RUN — ... is set on a VERDICT run" F1. The plausibility floor was a cliff at exactly zero. classifyRun({status:2, total:5, sawAnyErrorTs:801}) returned ok, so a 99% undercount passed and printed "136 file(s) now clean". The invariant is not "did it parse SOMETHING" but "did it account for EVERY line claiming to be a diagnostic": parseDiagnostics now returns `unparsed` plus a verbatim sample, and any divergence is refused. Strict equality is safe here, and correct rather than merely conservative: measured over the real corpus, unparsed === 0 across 801 diagnostics and 1451 output lines (650 of them message-continuation lines, which do not carry the marker). The divergent shapes that exist in tsc are the FILELESS diagnostics (TS18003, TS6053) — precisely the outputs meaning the program was not built as intended. A COLLAPSE (under 25% of the baseline) is refused too, split by reversibility: shouted on a verdict run, where it cannot hide a regression, and refused on --write-baseline, where it rewrites the ratchet and destroys the evidence together. F2. The derived floor had no lower bound, so replacing the magic 400 with a value read from a file made the control only as good as that value — while still logging "derived from the baseline". testFileFloor(1) returned {floor: 0}, a floor no program can fail. It is now max(fixed 400, 90% of the recorded count); a missing/null count falls back, and a corrupt one (non-integer, negative, zero, or absurd) is REFUSED rather than silently downgraded, because an absent value and a corrupt one are different facts. `derived` is now claimed only when the baseline actually raised the floor. F5. The drift control's WIRING was untested: mutating `if (!drift.ok)` to `if (false)` left the suite 78/78 green. diffExcludes was well covered and the control was live, but nothing pinned that the gate READ it — the same class this PR exists to close, inside its own suite. A TYPECHECK_TESTS_CONFIG_DIR seam now drives it end to end, with an undrifted control arm so a refusal attributes. The sweep for siblings found four more unpinned call sites (the positive-control arm's own verdict, the unusable-floor refusal, the escape-hatch refusal, and the tsconfig read error); each is now pinned on its SPECIFIC message, because these sites are outcome-equivalent and an exit-code assertion would survive the mutation. Also: validateBaseline cross-checks totalErrors against the sum of its entries (a hand-edited total was cosmetic and unnoticed); the three production env seams are echoed with [OVERRIDE] markers, so a stub-driven run is no longer byte-identical to a real one; and the doc's self-contradicting counts are reconciled and re-derived rather than trusted. Its history grep is corrected from `^src/.*/__tests__/` to `^src/(.*/)?__tests__/`, which cannot match src/__tests__/setup.ts — added by the same commit as two of the others — so the count is NINE, not eight. The load-bearing claims are unaffected: the convention was established, earliest 2026-01-23. Suite: 78 -> 134 cases, all green. A 19-mutant battery over the changed logic found and fixed one survivor (F5-floorresult-callsite, whose case used a value the Math.max fix had made valid) and carries two equivalent controls that must survive and two breaking controls that must die; final run has 0 survivors and 0 false kills, tree checksum-restored. One control was initially reported KILLED and it deserved to be — it renamed only a declaration, leaving ${pct} dangling, i.e. a breaking mutation mislabelled as equivalent. That is recorded in the doc rather than quietly corrected. Measurement re-verified unchanged on the exact committed content: 801 errors across 141 files, 938 test files in program, floor 844, and the committed baseline byte-identical to a fresh --write-baseline (same sha256). The three proof-of-concept runs re-run: PASS 801/141; BLOCKED 802/142 naming the planted file while `pnpm typecheck` on the same tree reports "OK — 0 type errors", exit 0, with 0 `error TS` lines and 0 mentions of that file; PASS 801/141 on revert. Still a proposal: no CI wiring, no attempt on the 801, tsconfig.json and `pnpm typecheck` untouched. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
Zachary Lowden
|
018f9a0e7f |
fix(og): unblock the SVG loader Next 16.3.0 leaves blocked, so /api/og stops 500ing (#3777)
Next 16.3.0's image optimizer applies a process-global libvips loader allowlist that omits SVG. next/og rasterizes satori's SVG through the same sharp instance, so every ImageResponse threw 'Input buffer contains unsupported image format' once the optimizer initialized — making /api/og return 500 on 100% of requests. Carries upstream vercel/next.js#96681 verbatim as a version-pinned pnpm patch (both the CJS and ESM image-optimizer copies), plus a CI guard that fails the build if the patch stops applying. Verified on the standalone preview build: /api/og returns a valid PNG on both the FallbackCard and OgCard paths after the optimizer is warmed to a cache miss. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
Zachary Lowden
|
ca699f5def |
ci(test): run the apps/* vitest suites — 369 tests CI had never executed (#3694)
The root vitest.config.mts registered `packages/*/vitest.config.*` as projects and nothing for `apps/*`, and CI runs only `--project unit` (root-relative `include`) and `--project '@civitai/*'`. So no test under apps/ has ever run in CI: 43 files / 369 tests across five apps, green only for whoever remembered `pnpm --filter <app> test` by hand. This is the same gap #3591 documented for packages/, in the sibling directory, missed when that one was closed. Register `apps/*/vitest.config.{ts,mts}` — globbed on the CONFIG FILE for the reason the packages comment already gives — and give each app project an explicit `app:` name. That name is load-bearing: every app is also published as `@civitai/*` (`@civitai/auth-app`, and `@civitai/orchestrator-gateway` with no suffix), so under default naming no pattern separates apps from packages and `--project '@civitai/*'` would have silently swept the apps into the packages job. Verified the packages selector is unchanged: same 9 packages, same 957 tests, zero app files in the report. apps/creator-studio gets a config it never had — it declared a `test` script and owned a vitest test file that no runner could reach. Generalize scripts/ci/assert-package-suites-ran.mjs to take the workspace dir and rename it to assert-workspace-suites-ran.mjs, so one ledger serves both jobs, and wire an `App unit tests` job that runs it. Per-app counts: auth 29 files/237, notifications 10/101, storage 1/17, orchestrator-gateway 2/6, creator-studio 1/8. All green. Positive controls (both with vitest exiting 0, assertion exiting 1 on its own `missing` branch): breaking one app's `include` so it collects nothing, and dropping one app's `test.name` so the selector no longer matches it. Also corrects an inherited premise in the script header — on vitest 4.0.18 a `--project` filter matching nothing fails startup and exits 1, it does not exit 0 as the comment claimed. Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
Zachary Lowden
|
902d222ed1 |
ci: run the nine packages/* test suites, which nothing has ever executed (#3642)
* ci: run the nine packages/* test suites, which nothing has ever executed The root `unit` Vitest project's `include` is root-relative (`src/**`, `scripts/**`), and the `Unit tests` job runs `vitest run --project unit`. So no CI job in this repo has ever invoked a workspace package's suite: 616 tests across nine `packages/*` packages ran only for whoever remembered `pnpm --filter <pkg> test` by hand. That is not theoretical. The schema-drift detector (#3591) shipped 81 tests into this gap, and two of them had been RED on `main` since #3592 landed — with nothing anywhere to say so. Root `vitest.config.mts` now globs each package's OWN config file, so every package keeps the config it was written against. The new `Package unit tests` job is BLOCKING: the reasoning that made `Unit tests` report-only does not transfer, because this is 616 tests with no browser, no database and no Next module graph, and the whole run is ~15s. Three pre-existing failures the job surfaced, all fixed here rather than skipped: * civitai-db-queries/src/infra/enum-array-parsers.explain.test.ts could never pass without a database. `describe.skipIf(!url)` skips the tests but Vitest still EXECUTES the describe callback during collection, so the eager `new Pool({ connectionString: noVerify(undefined) })` threw `TypeError: Invalid URL` and failed the whole FILE to import — which reports as "1 failed test file" with no test count, not as a failing test. The Pool moves into `beforeAll`, which genuinely does not run when skipped. * production-snapshot.test.ts pinned 235 nullability findings on seven `*Rank` tables. #3592 marked those columns optional to match the database and the findings correctly went away. The assertion now guards the REMEDIATION (Rank family at zero) with a positive control on that zero — the 11 findings that remain. * parse-prisma-schema.test.ts pinned Prisma's defaulted `onDelete` at one named site, `TagsOnImageNew.image`, and #3589 gave that relation an explicit `onDelete`. Re-anchored to the POPULATION of bare relations, which an ordinary schema edit cannot retire. A green vitest run is a claim, not evidence: `--project` matching nothing exits 0, and so does a config whose globs stopped resolving. scripts/ci/assert- package-suites-ran.mjs therefore asserts a ledger — every package with a vitest config and a test file on disk must appear in the results — so the job fails when the executed set SHRINKS, which the totals cannot see. Validated against three known-bad reports (empty run, one package dropped, missing report file) before being trusted. Test count, measured in CI: 811 files / 12,031 tests before, all from `src/`; 61 files / 616 tests added by the new job, of which 81 are the drift detector's. * fix(ci): correct the "blocking" claim, and close three ledger blind spots Audit follow-ups on the packages/* CI job. The job was described as BLOCKING. It is not, and the distinction is load-bearing for whoever next decides whether to flip `unit`: `main` has branch protection but no required_status_checks at all, so a red check here does not prevent a merge. What it actually buys is rendering RED instead of red-but-ignored. Corrected in the workflow comment, the README and the PR body, along with what would make it a real interlock. The Rank positive control was green for the wrong reason. Asserting the Rank family is at zero nullability findings is satisfied just as well by a differ that never visits a Rank table — measured: making compare.ts `continue` on every model whose table ends in `Rank` left that test GREEN and reddened only two neighbouring ones. The `nullability.length >= 11` control could not see it, because it proves the KIND is still emitted, not that those seven tables are in scope. Now pinned via findings of a different kind that the same seven models must still produce; re-running the identical mutation kills the test itself. Three ledger blind spots, each demonstrated with a probe before and after: - It only looked in `src/`. A package with its tests in a top-level `__tests__/` was invisible to the EXPECTED set, so it could never be noticed going missing. Now scans the whole package directory. - It only matched `.test.ts`/`.spec.tsx`. `.mts`, `.cts` and `.js` tests were invisible the same way. - It counted SKIPPED tests as having run, summing assertionResults.length. A package that self-skipped in its entirety still satisfied the ledger — and self-skipping on a missing DATABASE_URL is exactly the pattern these suites use, so that failure mode is live, not hypothetical. It now requires a non-zero EXECUTED count and reports skips beside it, so a package quietly turning itself off is visible rather than absent from the arithmetic. Both directions verified by exit code, not by reading output: real report 0; tests-outside-src probe 1; .mts probe 1; wholly-skipped package 1; empty run 1; one package dropped 1; missing report 1. Known remaining gap, deliberately not widened into this change: `apps/*` has four more vitest configs and ~43 test files that no CI job runs. Same one-line fix in the same projects array, plus teaching the ledger about `apps/` — it hardcodes `packages/` today. Follow-up PR. Also: the CI job's JSON report is gitignored (the documented command left an untracked artifact at the repo root), and the workflow comment's "four EXPLAIN tests" is corrected to 3 + 1 across two files. |