`critic/SELF-CRITIQUE.md`

83 lines
# sf-critic — Self-Critique

Written 2026-04-30 by the worker about itself. Brutal-honest list of what's
missing for production and where the heuristics will lie.

## Missing for production

1. **No LLM-backed semantic review.** (high)
   The whole point of the Manager critic is "code-reviewer + architect-reviewer
   + style-lint + a11y-check subagents." This worker only ships the cheap static
   layer. Real reviewer-quality findings (does the prop drilling make sense?
   does this component belong in `app/` or `components/`? does the copy match
   the brand voice?) require a model. Add a second mode `mode: 'llm'` that fans
   out to the four subagents in parallel and merges with these heuristic
   findings.

2. **No rate limit on `/critique`.** (high)
   The orchestrator could fan out 11 stages × 3 sites × N retries and hammer
   this worker. There's no in-flight cap, no per-site queue, and no debounce.
   Also no auth — anything on the box can post arbitrary findings to the
   orchestrator via `/critique/stub`. Add (a) `express-rate-limit`, (b) a
   shared-secret header, (c) a Bottleneck queue keyed by `site_id`.

3. **No screenshot a11y / visual diff.** (medium)
   `a11y.js` accepts `screenshots[]` and explicitly logs that it's deferring.
   Visual regression, contrast on rendered pixels, focus-ring presence, and
   tap-target size all need image analysis (axe-core via Playwright, or an LLM
   vision pass). Until then, "passes a11y" is misleading — it only means
   "passes static a11y."

4. **No dedupe / suppression.** (medium)
   Re-running `/critique` on the same paths re-posts identical findings every
   time. The orchestrator's `manager_findings` table will grow without bound.
   Need a `(site_id, stage, source, title)` unique-or-update key, or a fingerprint
   column, with `resolved=false` reused across runs.

5. **No Prometheus / health metrics beyond `{ok:true}`.** (low)
   Should expose check-duration, findings-by-severity counters, post failure
   rate. Currently a silent failure (orchestrator down, FK error, network
   timeout) reduces `findings_posted` and that's the only signal.

6. **`paths[]` trust.** (medium)
   The worker reads anything the caller hands it. A malicious or accidental
   `paths: ['/']` would walk the whole filesystem under glob. Need a base-dir
   allowlist (e.g. only under `~/Projects/site-factory/sites/`).

## False-positive classes the heuristics will produce

- **`code.js` "unhandled async"** will fire on async functions that *do* handle
  errors via `.catch()` at the call site, or via an outer `try` in the parent.
  The body-only scan can't see those. Expected FP rate ~15–25 % on real
  codebases. Severity overstated as `medium`.

- **`a11y.js` "icon-only button"** will flag buttons whose text comes from a
  child component (e.g. `<button><Tooltip>Save</Tooltip></button>`) or from a
  translated string in a JSX expression `{t('save')}`. Both are stripped by the
  scanner.

- **`security.js` password regex** will match Tailwind class strings
  (`password: "input input-bordered"`) and example/test fixtures
  (`password: "password123"` in a `__tests__` dir). The current skip-list only
  excludes `.env.example`, `*.md`, and `node_modules`. A `__tests__` /
  `*.test.*` skip would help.

- **`architect.js` site-root detection** assumes the convention that a site is
  any directory containing an `app/` child. A monorepo with `apps/web/app`
  won't be discovered unless the caller passes the `apps/web` path explicitly.

## Severity per item

| Item                                           | Severity |
|-----------------------------------------------|----------|
| No LLM-backed semantic review                  | high     |
| No rate limit / no auth on /critique           | high     |
| No screenshot a11y                             | medium   |
| No dedupe / suppression                        | medium   |
| No metrics                                     | low      |
| paths[] trust / no base-dir allowlist          | medium   |
| async no-try false positives                   | medium   |
| icon-button false positives                    | low      |
| password-regex false positives                 | medium   |
| site-root detection misses monorepos           | low      |