`consensus/visual-factory.md`

47 lines

# Consensus — visual-factory
Generated 2026-04-30T23:48:02-07:00. Source: codex review/fix/rereview/round-2 artifacts (round-3 was quota-blocked).

## Codex pass-1 review (top risks)
## 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.

## Round-1 applied
## Applied
- src/llm.js:10 — changed the Claude CLI fallback from a user-specific absolute path to `claude` from `PATH` — removes a hardcoded `/Users/...` path from source.
- server.js:107 — added the same `CLAUDE_CLI || 'claude'` default for deep health checks — keeps server and LLM adapter behavior aligned.
- server.js:109 — added a small executable lookup helper for absolute paths, relative paths, and `PATH` commands — lets `/health/deep` validate `claude` without a hardcoded home directory.
- server.js:204 — changed the Claude CLI health probe to use the new lookup helper — avoids checking Steve's local absolute path.
- .env.example:14 — changed `CLAUDE_CLI` to `claude` — documents the portable default without copying a real local path.

## Codex re-review verdict: FIX BEFORE SHIP

## New issues introduced
server.js:225 — `fs.access(command)` defaults to existence only, so a non-executable absolute/relative `CLAUDE_CLI` passes `/health/deep` while `spawn()` later fails with EACCES — severity P2
server.js:231 — PATH lookup also uses existence only, so a non-executable file named `claude` earlier in PATH can make health green while critic execution fails — severity P2

## Round-2 applied
## Round 2 Applied
- server.js:5 — imported `fsConstants` from `node:fs` — needed so executable checks can use `X_OK` instead of existence-only access (CODEX_REREVIEW.md “New issues introduced” server.js:225/server.js:231 and “Recommendations”).
- server.js:224 — updated `assertExecutable()` to reject directories and require execute permission for absolute/relative `CLAUDE_CLI` paths and `PATH` candidates — fixes `/health/deep` false positives where non-executable files could report `claude_cli: ok` while `spawn()` would later fail (CODEX_REREVIEW.md “STATUS concern” and P2 new issues at server.js:225/server.js:231).

## Still deferred for Steve
- P0 — server.js:223 — create/retry/delete/activate/IG POST routes lack auth/CSRF — deferred because token, Origin, and CSRF policy are outside the safe fix list.
- P0 — src/stages/compose.js:34, src/stages/render.js:21 — LLM HTML is written and opened as `file://` with JS/network behavior unresolved — deferred because sanitization/CSP/request-blocking choices need product/security decisions.
- P0 — server.js:259 — retry/cancellation semantics for live runs still need an owner decision, especially `force=true` behavior — deferred because cancellation/locking is outside the safe fix list.
- P0 — src/stages/intake.js:35, src/stages/render.js:19 — generated dimensions are not clamped before rendering — deferred because bounds policy is not in the safe fix list.
- P1 — server.js:333 — `/runs/:id/png` sends DB paths without sandbox validation — deferred because artifact path trust policy is outside the requested safe fixes.
- P1 — server.js:405 — activation validates `png_path` but not `html_path` before copying — deferred for the same sandbox policy reason.
- P1 — src/stages/vision_check.js:40, src/stages/critic.js:21 — failed vision can still approve at quality 75 — deferred because changing approval policy affects generated asset workflow.
- P1 — src/pipeline.js:195 — crash logging can use stale `run.current_stage` — deferred because it is not in safe fix items 1-7.
- P1 — server.js:413, server.js:442 — activation copy and manifest update are not locked/atomic — deferred because concurrency design is outside the safe fix list.
- P1 — src/stages/compose.js:16, src/stages/render.js:21 — Google Fonts plus `networkidle` can stall offline runs — deferred because asset bundling/request policy is outside the safe fix list.
- P1 — package.json:10 — `npm run run` points to a missing script — deferred because script changes are not in safe fix items 1-7.