`visual-factory/claude-review.md`

30 lines

# Claude review of codex patches — visual-factory

## Verdict
MIXED — the core portability fix is correct and the X_OK follow-up is sound, but the `claude` PATH question is genuinely live and the deferred P0/P1 list remains a real operational risk for a 24/7 pm2 service.

## Where codex got it right
- `src/llm.js:10` and `server.js:222`: removing the hardcoded `/Users/stevestudio2/.claude/local/claude` path was the right call — that path does not exist on this machine (`~/.local/bin/claude` is the real binary). Had codex left the old path, every critic stage would silently fail.
- `server.js:224-241` (round 2): upgrading `assertExecutable` to use `fs.constants.X_OK` and reject directories closes the false-positive health-check hole rereview caught. Both branches (absolute path and PATH walk) now enforce execute permission.
- `.env.example:14`: `CLAUDE_CLI=claude` is the correct portable default and matches what the live process would resolve.
- Atomic manifest write (`server.js:117-119`): `.tmp` + rename pattern is correct; the `withManifestLock` mutex is adequate for a single Node process.
- `assertWithin` sandbox helper (`server.js:72-81`) and `ARTIFACT_NAME_RE` (`server.js:86`): clean path-traversal guards; the `path.sep` suffix prevents prefix-collision attacks.

## Where codex got it wrong or missed (file:line)
- `server.js:226`: `fs.stat` is called before `fs.access(..., X_OK)`. On a symlink to a non-executable file, `stat` follows the link and succeeds; `access` then catches it. This is fine, but the stat call is redundant for the `isDirectory()` check since `access(X_OK)` on a directory will itself throw `EISDIR` on most systems. Minor, not a regression.
- `server.js:384-388`: `/health/deep` now gates on `checks.ollama === 'ok'` strictly, meaning an `"ok but missing: qwen3:14b"` response yields 503. That is correct behavior for a live factory but codex did not document this as a breaking change — operators who relied on the route returning 200 when Ollama is up-but-model-unpulled will now see 503. Mention-worthy.
- The rereview noted "no missed call-sites" — correct. `CLAUDE_CLI` is only consumed at `src/llm.js:96` and `server.js:368`.

## Operational concerns codex missed
- PATH is confirmed safe: pm2 env 58 shows `/Users/stevestudio2/.local/bin` on PATH, and `which claude` resolves to `/Users/stevestudio2/.local/bin/claude`. The `claude` → PATH change is validated for this machine.
- 13 restarts are visible in pm2. Codex did not investigate the crash cause. The pm2 show output shows `unstable restarts: 0` now, meaning crashes were before the 5-minute stability window — likely the old bad `CLAUDE_CLI` path causing spawn failures at startup or during the first critic stage, which the fix resolves. No new crash source was introduced.
- The deferred P0 list (no auth on `/runs` POST/DELETE/activate, untrusted LLM HTML opened as `file://` with JS enabled, no viewport size clamping) represents real attack surface even for a loopback-only service. Any tab open in the user's browser can POST to `127.0.0.1:9892` without auth. For a system that `publish`es files to `~/Pictures/` this is worth a mitigation ticket.
- `server.js:469`: `force=true` retry on a `running` run still deletes events/artifacts and re-enqueues while the original pipeline may still be writing. The atomic UPDATE guard reduces the window but does not close it if the pipeline is in a non-DB-writing stage (e.g., Playwright render). This was deferred; note it is dangerous on a live system.

## Final action items (priorities)
1. (Critical) Add a minimal shared secret check on mutating routes (`/runs` POST, `/runs/:id/retry`, `/runs/:id/delete`, `/runs/:id/activate`). A single `X-Admin-Token` header checked against an env var is sufficient for a local service.
2. (Critical) Clamp LLM-returned viewport dimensions in `src/stages/render.js` before the Playwright call. Unbounded width/height can OOM the Mac mid-pipeline.
3. (High) Add `sandbox/disableExternalUrls` Playwright options or a request-interception hook in `src/stages/render.js` so the LLM-generated HTML cannot exfiltrate local files via `file://` callbacks or reach external hosts.
4. (Medium) Document the `/health/deep` strictness change: `ollama=ok but missing:...` now returns 503 — operators relying on the old behavior need to know.
5. (Low) Remove or fix `package.json` `run` script pointing to missing `scripts/run.js` to avoid developer confusion.