`REVIEW-2026-05-04.md`

63 lines
# the-ai-factory — overnight debate-team review (2026-05-04)

Code-reviewer + architect-reviewer parallel run. **6 patches applied** (3 P0 + 2 P1 + 1 P2 standing-rule fix). Architecture roadmap (factory-core extraction theme matches site-factory recommendation).

## P0 patches APPLIED

| # | File:line | Issue | Fix |
|---|---|---|---|
| 1 | `server.js:67,95` | `ADMIN_TOKEN` declared in .env but **no middleware enforced it**. POST /runs and POST /runs/:id/activate were unauth — any local process could trigger pipeline runs or activate artifacts into ~/.claude/ | Added `requireToken` middleware; both routes require `Authorization: Bearer $ADMIN_TOKEN`; 503 if token unset |
| 2 | `server.js:124` | Activate handler used `art.artifact_name` from DB in `path.join(CLAUDE_AGENTS_DIR, name)` with no re-validation. Tampered DB row could write `~/.ssh/authorized_keys` | Added regex re-check + `path.resolve()` containment check (allowedRoot per artifact_type) |
| 3 | `src/stages/scaffold.js:63` | Same risk in scaffold — `spec.artifact_name` from LLM used in path.join with no sandbox containment after the LLM emits it | Re-asserted regex + `path.resolve(SANDBOX_ROOT)` containment check before write |

## P1 patches APPLIED

| # | File:line | Issue | Fix |
|---|---|---|---|
| 4 | `server.js:31` | Wildcard CORS with comment "safe since loopback" — but CORS only restricts BROWSERS. Any malicious page user opens can fetch 127.0.0.1:9890 with credentials and trigger /runs | Allowlist `VIEWER_ORIGIN` (default `http://127.0.0.1:9891`) only; sets Vary + Allow-Credentials only on match |
| 5 | `src/stages/memory_write.js:23` | LLM-generated `spec.scope` + `spec.triggers` written verbatim into `MEMORY.md`. Poisoned response could inject newlines + markdown headers (e.g. `\n# OVERWRITE`) corrupting Steve's memory index | `sanitize()` strips `\r\n` + collapses whitespace + caps length per field |

## P2 patch APPLIED — standing-rule alignment

| # | File:line | Was | Now |
|---|---|---|---|
| 6 | `src/llm.js:9` | `OLLAMA_HOST` defaulted to `http://127.0.0.1:11434` (Mac2) | `http://192.168.1.133:11434` (MS1) per `feedback_ollama_default_ms1.md` — Mac2 froze 2026-05-02 under v3+v5+codex GPU contention |

`node --check` passes on all 4 modified files.

## P1 / P2 deferred (in REVIEW)

- `server.js:104` — `?force=true` undocumented bypass for critic gate. Replace with explicit `POST /runs/:id/force-activate` requiring `{reason}` body, logged at WARN.
- `server.js:187` — `/runs/:id/events` unauth, leaks full pipeline payloads (specs, error stacks).
- `src/pipeline.js:52,100` — no global pipeline timeout. If MS1 Ollama hangs, run sits in `running` forever holding pg connection.
- `output/` not in `.gitignore`.
- `package.json:15` — `@anthropic-ai/sdk` declared but never imported (contradicts standing rule "no Anthropic API"). Drop dep.
- `scripts/run.js` — no max prompt-length guard before sending to Ollama.
- No `helmet` on either server.

## Architecture roadmap (deferred)

**Architectural Impact: MEDIUM.** Pipeline disciplined (sandbox-then-promote, critic-iterate-recritic, manual activate gate). Structural problems mirror site-factory:

### R1 — Extract `factory-core/` shared package
**Same recommendation as site-factory review.** `src/llm.js` (ollama + claudeCli with semaphore) + `src/pipeline.js` skeleton (setStage / logEvent / finishRun) + sandbox-promote logic + Stage interface contract. site-factory + ai-factory + visual-factory all consume it. Land before visual-factory grows divergent copies. ~400 LOC dedup across 3 projects.

### R2 — Delete `viewer.js` (15-LOC stub)
Same anti-pattern as site-factory's sf-viewer. Mount `app.use('/ui', express.static(viewerDir))` in orchestrator. Saves a pm2 slot, port, log pair, and the CORS layer (`server.js:31-37` becomes "not needed" since UI is same-origin).

### R3 — Move `memory_write` back into pipeline
Currently runs inside the activate HTTP handler (`server.js:154-179`), recovers `spec` by string-matching `events.message='spec extracted'` (line 157). Fragile coupling — change the event message in `pipeline.js:48` and memory_write silently breaks. The 11-stage diagram in `pipeline.js:13-16` lies (stages 9 + 10 are deferred to a separate code path). Make it a real stage. Inject paths via config object.

### R4 — Rename `smoke_test` → `lint_frontmatter`, add a real smoke test
Current "smoke test" is a static frontmatter linter — never executes the artifact, never verifies Claude can load the skill. Real test: write to tmpdir under `~/.claude/skills/`, run `claude --list-skills | grep <name>`, delete. For subagent: `js-yaml.load` the frontmatter. The current "passing 6/6 checks" creates false confidence.

### R5 — Replace `?force=true` with auditable explicit endpoint
New `POST /runs/:id/force-activate` requiring `{reason: string}` in body, logged into `events` with `level='warn'`. The critic gate becomes meaningful.

## Files touched

- `/Users/stevestudio2/Projects/the-ai-factory/server.js`
- `/Users/stevestudio2/Projects/the-ai-factory/src/stages/scaffold.js`
- `/Users/stevestudio2/Projects/the-ai-factory/src/stages/memory_write.js`
- `/Users/stevestudio2/Projects/the-ai-factory/src/llm.js`