mirror of
https://github.com/civitai/civitai.git
synced 2026-09-20 22:08:18 +08:00
chore(moderator): stop builds freezing the editor, and add the missing review agent
VS Code was watching and indexing every SvelteKit build artifact: `files.watcherExclude` covered `node_modules`, `.next` and `dist` but was never updated when the SvelteKit apps landed, so `.svelte-kit` (947 files) and `apps/*/build` (845) were unexcluded. A build writes ~1,800 files / 25MB into the workspace. `defender-exclusions.ps1` defaulted to `C:\Dev\Repos\work`, which is not where this repo lives. Add-MpPreference accepts a nonexistent path, so run with defaults it reported success and excluded nothing. It now resolves the repo from its own location and throws rather than silently no-op when the root is missing. Hook changes: drop the `svelte-kit sync` / `pnpm check` guard (sync alone is cheap - 121 generated files, not the ~690 the docs claim) and guard `build` instead, which is the expensive one and is not a check. Set to ask rather than block: diagnosing a build-only failure and producing a real artifact are both legitimate. check-svelte-ts.mjs removes the main reason to run a build at all. Svelte 5 strips the type annotation from `(n?: number)` but leaves the `?`, so rollup rejects it while typecheck, dev and every review agent pass. It now fails on write. Verified against all 148 committed .svelte files: catches the defect, zero false positives (the first version had five, all `?` in type position). Adds retool-fidelity-review, the export-vs-build pass. It was prose in the skill that got performed by hand, differently each time, and skipped twice - despite being the only review that can see a capability nobody wrote. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This commit is contained in:
@@ -0,0 +1,122 @@
|
||||
---
|
||||
name: retool-fidelity-review
|
||||
description: The fourth review for a Retool migration slice — walks the export query by query and asks whether each behaviour is present in the build and matches. The only review that can see a capability that was never written; the three code reviews pass cleanly over a faithful implementation of the wrong thing. Use on every slice before calling it done.
|
||||
tools: Read, Grep, Glob, Bash
|
||||
---
|
||||
|
||||
# Retool export-vs-build fidelity review
|
||||
|
||||
You compare **the export to the build**. Not the build to itself.
|
||||
|
||||
`svelte-correctness-review`, `svelte-idiom-review` and `svelte-abstraction-review` read the code that
|
||||
was written. None of them opens the export, so **none of them can see code that was never written** —
|
||||
a missing capability passes all three cleanly. On Bulk Image Manager those three returned 14 findings
|
||||
and missed four real gaps; this pass found all four in one run. User Lookup passed three full review
|
||||
rounds while 97 of its 170 queries were unported.
|
||||
|
||||
That is the entire reason you exist. Absent behaviour is your subject.
|
||||
|
||||
**Never run `pnpm check`, `pnpm build`, `svelte-kit sync`, `typecheck`, or any `prettier` command.**
|
||||
They fight the dev server's file watcher and have frozen an editor for a full day; a PreToolUse hook
|
||||
guards some of them. Read and grep only.
|
||||
|
||||
## What you are given
|
||||
|
||||
The invoker names **one app**. Audit only that one.
|
||||
|
||||
| Source | Where |
|
||||
| --- | --- |
|
||||
| Every query with its SQL/URL and bindings | `docs/moderator-app/retool-exports/<app>.md` |
|
||||
| The bucket classification | `docs/moderator-app/retool-exports/<app>-audit.md` |
|
||||
| Tracker claims, deliberate omissions | `.claude/skills/retool-migration/MIGRATIONS.md` |
|
||||
| The build | `apps/moderator/src/routes/...`, `apps/moderator/src/lib/server/*.service.ts` |
|
||||
| Prior findings and their status | `docs/moderator-app/retool-exports/parity-findings.md` |
|
||||
| What Retool looked like | `C:/work/retool-screenshots/<app>/*.png` — outside the repo on purpose |
|
||||
|
||||
Screenshots are readable with the Read tool. **They are never committed and you must not quote a real
|
||||
username, email, id or buzz balance in your report** — this repository is public. Describe the field,
|
||||
not its value.
|
||||
|
||||
If the inventory predates the current extractor it will have no `## layout` or widget-options section.
|
||||
Say so rather than concluding the app had no tabs or no dropdowns.
|
||||
|
||||
## Method
|
||||
|
||||
Walk the export **query by query**, in the inventory's order. For each one:
|
||||
|
||||
1. **Read its SQL body. Never judge it by its name.** This is the single rule that catches the most.
|
||||
Ask: what does the `WHERE` filter, what columns does the `SELECT` list, and where does its input
|
||||
come from?
|
||||
2. **Find the corresponding behaviour in the build**, with `file:line`.
|
||||
3. **Verdict**: PRESENT · PARTIAL (say precisely what is missing) · ABSENT · DIVERGENT (present but
|
||||
behaves differently) · CORRECTLY-OMITTED (the audit gives a reason and the reason holds).
|
||||
|
||||
### Being NAMED in the audit is not being COVERED
|
||||
|
||||
This is the failure that survives every other check, so treat every audit row as a claim to verify, not
|
||||
as evidence. A row absorbs a query whose behaviour it does not carry. The four archetypes, all real:
|
||||
|
||||
- **A row that describes the query's name, not its content.** `UserQuery5000` was filed as covered by
|
||||
`resolveUserId` — it resolves an identifier to an id. The query's actual body was
|
||||
`WHERE i."nsfwLevel" = 32`: *the images already removed from this account*, i.e. the entire restore
|
||||
workflow. Absent from the build.
|
||||
- **The endpoint is named and the entry point is dropped.** `RemoveArrayOfImages` mapped to
|
||||
`/api/mod/remove-images`. The endpoint was built; the **pasted list of image ids** — how a ticket or
|
||||
script hands work over — was not. **A query whose input is a widget nobody built is not ported,
|
||||
whatever endpoint it maps to.**
|
||||
- **A mapping that silently changes blast radius.** `nukeUser` was mapped to `purgeAllContent`.
|
||||
`nukeUser` POSTs images only; `purgeAllContent` also takes models, posts, articles and comments — a
|
||||
*larger* radius under the same label. Check radius in both directions.
|
||||
- **Columns selected but never rendered.** Every finder selected `prompt`, `poi` and `minor`; none
|
||||
reached the DOM, so moderators set POI from a thumbnail with the prompt and the current flag state
|
||||
both invisible. Diff the `SELECT` list against what the component actually renders.
|
||||
|
||||
Apply the same scepticism to `equivalent` and `superseded` rows: name the *specific* thing that covers
|
||||
it and read that thing.
|
||||
|
||||
### Beyond the SQL
|
||||
|
||||
The queries are most of the spec, not all of it.
|
||||
|
||||
- **Panes are tabs.** The `## layout` section lists containers, panes and modals. A container with
|
||||
several panes is a tab group and should be sub-routes. A moderator who had two tabs and now scrolls
|
||||
past both reports the tool as broken even when both queries are ported.
|
||||
- **A modal is a dialog**, not an inlined panel.
|
||||
- **`only visible when` on a pane is a role or state gate that appears in NO query.** User Lookup's
|
||||
Buzz pane was gated on `Senior Mod`; porting the pane without the gate hands every moderator a
|
||||
restricted capability. Check every gate reached the build.
|
||||
- **Pane titles carry filter widgets** ("TOS Violation?", "Review Rating", "Search Review Content"). A
|
||||
table ported without its filter row is not the same tool.
|
||||
- **Dropdown option sets and button presets encode workflows that exist in no query** — canned amounts,
|
||||
duration presets, reason lists. Check the option source of every picker: a hardcoded list in the
|
||||
build where Retool scoped one by action is a real defect, and has been wrong twice.
|
||||
- **A `Function` query is never plumbing.** Only `State`, `Timer`, table grouping and pickers are.
|
||||
|
||||
## Verify your own findings before reporting
|
||||
|
||||
You will produce plausible-but-false findings; a previous run called `TOSImages` a dismissed mutation
|
||||
when the export says `//doesnt run anywhere, just a test`. Before you report anything ABSENT:
|
||||
|
||||
- Grep the whole app, not just the slice's route — a capability may live on a neighbouring page, and
|
||||
"already shipped elsewhere" is a legitimate answer.
|
||||
- Re-read the export comment on the query. Retool authors left notes about dead experiments.
|
||||
- Quote the evidence: the SQL fragment, or the `file:line` where you looked and did not find it.
|
||||
|
||||
A finding you cannot evidence is noise, and noise here is expensive — it sends someone to rebuild
|
||||
something that works.
|
||||
|
||||
## Rank by what a moderator would notice
|
||||
|
||||
1. **A screen that states something false** — a count excluding rows it implies, a confirmation naming
|
||||
the wrong blast radius, a label promising an action that does not happen.
|
||||
2. **An absent capability** — a filter, tab, table, action or entry point Retool had.
|
||||
3. **A capability that exists but is unreachable** — no link, no nav entry, no grant.
|
||||
4. Divergences that are defensible but undocumented.
|
||||
|
||||
## Report
|
||||
|
||||
- One line per query, in the inventory's order, with the verdict. Include the PRESENT ones — a clean
|
||||
walk is the result, and a reader needs to see coverage, not only exceptions.
|
||||
- Then the findings, ranked as above, each with `file:line` and the evidence that supports it.
|
||||
- Then, explicitly: **which audit rows are wrong**, so the audit can be corrected rather than re-trusted.
|
||||
- Do not fix anything. Report only.
|
||||
@@ -0,0 +1,61 @@
|
||||
#!/usr/bin/env node
|
||||
// PostToolUse on Write|Edit for .svelte files.
|
||||
//
|
||||
// Catches the one TypeScript-in-Svelte defect that NOTHING else in the loop sees: an optional
|
||||
// parameter in a function signature. Svelte 5's TS stripping erases the type annotation but leaves
|
||||
// the `?`, so rollup receives invalid JS — `pnpm build` fails while `typecheck`, the dev server and
|
||||
// every review agent pass. Two of these reached a commit before anyone noticed (2026-08-10).
|
||||
//
|
||||
// A `?` inside a TYPE is fine (`{ reset: (id?: string) => void }`) — the whole annotation is erased.
|
||||
// So this matches only a `?:` that follows an identifier directly inside a parameter list.
|
||||
|
||||
import fs from 'fs';
|
||||
|
||||
let raw = '';
|
||||
process.stdin.on('data', (c) => (raw += c));
|
||||
process.stdin.on('end', () => {
|
||||
let file;
|
||||
try {
|
||||
const input = JSON.parse(raw || '{}');
|
||||
file = input.tool_response?.filePath ?? input.tool_input?.file_path;
|
||||
} catch {
|
||||
process.exit(0);
|
||||
}
|
||||
if (!file || !file.endsWith('.svelte')) process.exit(0);
|
||||
|
||||
let src;
|
||||
try {
|
||||
src = fs.readFileSync(file, 'utf8');
|
||||
} catch {
|
||||
process.exit(0);
|
||||
}
|
||||
|
||||
// Only <script> blocks (a component may have two: instance and `module`); markup can contain anything.
|
||||
if (!/<script[^>]*>/.test(src)) process.exit(0);
|
||||
|
||||
// A `?` inside a TYPE is fine — the whole annotation is erased. So the paren list must be in VALUE
|
||||
// position: after `function f`, after `=` (arrow assignment), or a method shorthand at line start.
|
||||
// `[^(){}]*?` is what does the real work: it refuses to cross a `{`, which is the only thing
|
||||
// separating `= (n?: number) =>` from `= (x as { flag?: T })`.
|
||||
const OPTIONAL_PARAM =
|
||||
/(?:function\s+[\w$]*\s*|=\s*(?:async\s+)?|^\s*(?:async\s+)?[\w$]+\s*)\(\s*[^(){}]*?\b[A-Za-z_$][\w$]*\?\s*:/;
|
||||
|
||||
const hits = [];
|
||||
for (const block of src.matchAll(/<script[^>]*>([\s\S]*?)<\/script>/g)) {
|
||||
const startLine = src.slice(0, block.index).split('\n').length;
|
||||
block[1].split('\n').forEach((line, i) => {
|
||||
if (OPTIONAL_PARAM.test(line)) hits.push(` line ${startLine + i}: ${line.trim()}`);
|
||||
});
|
||||
}
|
||||
|
||||
if (!hits.length) process.exit(0);
|
||||
|
||||
console.error(
|
||||
`Optional parameter in a .svelte function signature — this breaks \`pnpm build\` ONLY.\n` +
|
||||
`Svelte 5 strips the type annotation but leaves the \`?\`, and rollup rejects it. typecheck and\n` +
|
||||
`dev both pass, so nothing else in the loop will tell you.\n\n` +
|
||||
`${file}\n${hits.join('\n')}\n\n` +
|
||||
`Use a default (\`n = 0\`) or an explicit union (\`e: SubmitEvent | null = null\`).`
|
||||
);
|
||||
process.exit(2);
|
||||
});
|
||||
@@ -42,9 +42,13 @@ const DANGEROUS_PATTERNS = [
|
||||
// Expensive or historically destructive, but sometimes legitimate — confirm rather than block.
|
||||
const GUARDED_PATTERNS = [
|
||||
{
|
||||
pattern: /svelte-kit\s+sync|pnpm\s+(run\s+)?check\b/i,
|
||||
// `svelte-kit sync` alone is cheap (121 generated files) and is deliberately NOT guarded.
|
||||
// A full build writes ~1,800 files / 25MB and is not a check — it catches nothing `svelte-check`
|
||||
// does not, apart from Svelte's TS stripping leaving `?` on optional parameters, which
|
||||
// check-svelte-ts.mjs now catches on write instead.
|
||||
pattern: /(apps[\/\\](moderator|auth|creator-studio)[^;&|]*\bbuild\b|\bvite\s+build\b)/i,
|
||||
reason:
|
||||
'`svelte-kit sync` regenerates ~690 files under .svelte-kit/, which the Vite dev server watches — the collision that froze this window repeatedly. Use `pnpm run typecheck` unless the route tree changed.',
|
||||
'A SvelteKit build writes ~1,800 files into the workspace and is NOT a verification step — use `pnpm --filter ./apps/<app> run typecheck`. Confirm only if you are diagnosing a build-only failure or producing a real artifact.',
|
||||
},
|
||||
{
|
||||
pattern: /prettier[^;&|]*--write/i,
|
||||
|
||||
@@ -10,6 +10,17 @@
|
||||
}
|
||||
]
|
||||
}
|
||||
],
|
||||
"PostToolUse": [
|
||||
{
|
||||
"matcher": "Write|Edit",
|
||||
"hooks": [
|
||||
{
|
||||
"type": "command",
|
||||
"command": "node \"$CLAUDE_PROJECT_DIR/.claude/hooks/check-svelte-ts.mjs\""
|
||||
}
|
||||
]
|
||||
}
|
||||
]
|
||||
},
|
||||
"enabledPlugins": {
|
||||
|
||||
@@ -8,12 +8,17 @@
|
||||
|
||||
#Requires -RunAsAdministrator
|
||||
|
||||
# Add-MpPreference accepts a path that does not exist, so a wrong root here reports success and
|
||||
# excludes nothing. Override with -ReposRoot on any machine that doesn't use this layout.
|
||||
param([string]$ReposRoot = 'C:\Dev\Repos\work')
|
||||
# Defaults to the repo this script lives in. A hardcoded root was wrong on any machine using a
|
||||
# different layout, and Add-MpPreference accepts a path that does not exist — so it reported success
|
||||
# and excluded nothing. Pass -ReposRoot to cover a whole repos parent directory instead.
|
||||
param([string]$ReposRoot = (& git -C $PSScriptRoot rev-parse --show-toplevel))
|
||||
|
||||
if (-not $ReposRoot -or -not (Test-Path $ReposRoot)) {
|
||||
throw "ReposRoot '$ReposRoot' does not exist. Add-MpPreference would accept it silently and exclude nothing."
|
||||
}
|
||||
|
||||
$paths = @(
|
||||
$ReposRoot,
|
||||
(Resolve-Path $ReposRoot).Path,
|
||||
"$env:LOCALAPPDATA\pnpm",
|
||||
"$env:LOCALAPPDATA\pnpm-store"
|
||||
)
|
||||
|
||||
@@ -345,14 +345,18 @@ weight that was a second copy of the number deciding whether a tag gets disabled
|
||||
that logged every deletion with an empty violation classification. Run it even when the slice looks
|
||||
finished; the misses are easiest to see once the code exists.
|
||||
|
||||
**Then run a FIFTH review, export-vs-build.** The three code reviews compare the code to itself and to
|
||||
this app's conventions, and the endpoint audit compares it to the main app's API — **none of them opens
|
||||
the export**, so all four pass cleanly over a faithful implementation of the wrong thing. Give an agent the inventory (`<app>.md`, which carries each query's
|
||||
SQL), the audit, and the built files, and ask one question: *walk the export query by query — is each
|
||||
behaviour present, and does it match?* On Bulk Image Manager the three code reviews returned 14
|
||||
findings and missed all four gaps above; the fidelity pass found them in one run. Verify its claims
|
||||
against the inventory yourself before acting — it will also produce plausible-but-false ones (it called
|
||||
`TOSImages` a dismissed mutation; the export says `//doesnt run anywhere, just a test`).
|
||||
**Then run the FIFTH review: the [`retool-fidelity-review`](../../agents/retool-fidelity-review.md)
|
||||
agent, on the app by name.** The three code reviews compare the code to itself and to this app's
|
||||
conventions, and the endpoint audit compares it to the main app's API — **none of them opens the
|
||||
export**, so all four pass cleanly over a faithful implementation of the wrong thing. This one walks
|
||||
the export query by query and asks whether each behaviour is present and matches. On Bulk Image Manager
|
||||
the three code reviews returned 14 findings and missed all four gaps above; the fidelity pass found them
|
||||
in one run. Verify its claims against the inventory yourself before acting — it will also produce
|
||||
plausible-but-false ones (it called `TOSImages` a dismissed mutation; the export says `//doesnt run
|
||||
anywhere, just a test`).
|
||||
|
||||
Run it on **every** slice, including ones already marked shipped. It is an agent rather than a
|
||||
paragraph because doing it ad-hoc meant it was done differently each time.
|
||||
|
||||
Then:
|
||||
|
||||
|
||||
Vendored
+15
-1
@@ -157,6 +157,20 @@
|
||||
"**/.next/**": true,
|
||||
"**/dist/**": true,
|
||||
"**/.git/objects/**": true,
|
||||
"**/.git/subtree-cache/**": true
|
||||
"**/.git/subtree-cache/**": true,
|
||||
"**/.svelte-kit/**": true,
|
||||
"**/apps/*/build/**": true,
|
||||
"**/.turbo/**": true,
|
||||
"**/.vite/**": true,
|
||||
"**/playwright-report/**": true,
|
||||
"**/test-results/**": true
|
||||
},
|
||||
"search.exclude": {
|
||||
"**/.svelte-kit": true,
|
||||
"**/apps/*/build": true,
|
||||
"**/.turbo": true
|
||||
},
|
||||
"files.exclude": {
|
||||
"**/.svelte-kit": true
|
||||
}
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user