`visual-factory/review.md`

36 lines

## Snapshot
- Local “brief → HTML/PNG visual artifact” factory with review/activation UI.
- Stack: Node.js, Express, browser JS, Playwright, PostgreSQL, Ollama, Claude CLI.
- How it's run: `npm start`; optional ad-hoc PM2 script in `package.json:7-12`; no ecosystem file.
- Status: live: files and outputs modified Apr 30, 2026.

## Top Risks (P0 — fix this week)
- `server.js:223`, `server.js:257`, `server.js:279`, `server.js:326`, `server.js:362`: no auth/CSRF on create, retry, delete, activate, IG post. Any local webpage/process reaching `127.0.0.1:9892` can trigger destructive POSTs. Fix with required admin token plus Origin/CSRF checks.
- `src/stages/compose.js:34-44`, `src/stages/render.js:13`: untrusted LLM HTML is written then opened as `file://` with JS/network enabled. Prompt injection can embed `file://`/remote URLs and leak local data via screenshots or callbacks. Fix by sanitizing, disabling JS, using request interception/CSP, and blocking all `file:`/external loads.
- `server.js:263-271`: retry accepts `running`, deletes rows/output, then enqueues another pipeline while the original continues. Failure mode: corrupted events/artifacts and competing writes to the same run dir. Fix by refusing running runs or implementing cancellation/locking.
- `src/stages/intake.js:35-36`, `src/stages/render.js:11-16`: LLM-controlled dimensions are not clamped. A bad spec can request huge viewport/screenshot and exhaust memory. Fix with min/max bounds before DB/render.

## Notable Issues (P1 — fix this month)
- `server.js:307-309`: `/runs/:id/png` sends `png_path` from DB without sandbox validation. If artifact rows are corrupted, this becomes arbitrary local file read. Reuse the activation sandbox check before `sendFile`.
- `server.js:390`: activation validates `srcPng` but copies `art.html_path` without sandbox validation. A poisoned row can publish arbitrary local files as HTML. Validate `html_path` under `output/`.
- `src/stages/vision_check.js:39-47`, `src/stages/critic.js:21-23`: vision failure returns quality `75` with no issues, and critic policy approves `>=75`. Broken vision can approve unreviewed artifacts. Mark vision failure as blocking or low-confidence.
- `src/pipeline.js:181-183`: crash logging uses stale `run.current_stage`; inserted run starts at `0`, so many failures log as `unknown`. Track current stage locally or read from DB in catch.
- `server.js:381-390`, `server.js:414`: activation uses check-then-copy and manifest read-modify-write without locking. Concurrent activations can overwrite files or lose manifest entries. Use atomic writes/rename and a mutex or DB transaction.
- `src/stages/compose.js:16`, `src/stages/render.js:13`: compose asks for Google Fonts, then render waits for `networkidle`. Offline/font slowness can stall runs. Bundle fonts or block external requests consistently.
- `package.json:10`: `npm run run` points to missing `scripts/run.js`. Fix script or remove it.
- Dependencies: no obvious pinned-danger versions from lockfile; `npm audit` could not run because registry DNS is unavailable.

## Polish (P2 — when convenient)
- No README observed; add run/bootstrap notes outside SQL comments.
- `viewer.js:8-11` is likely obsolete after unified `server.js:46-48`.
- Output artifacts are checked into the working tree unless this is not a git repo; clarify retention/cleanup.

## Strengths
- SQL queries use parameters consistently; no SQL injection observed.
- Server binds to `127.0.0.1` in `server.js:479`, reducing exposure.
- Pipeline records stage events and deep health checks, which helps diagnose local model/tool failures.

## Open questions for the owner
- Is this ever exposed through a tunnel, reverse proxy, or shared LAN?
- Are briefs only entered by Steve, or by untrusted users/automation?
- Should generated artifacts be treated as publishable business assets or disposable experiments?