Add code review report

REPORT.md

@@ -0,0 +1,84 @@
+# Fireclaw Code Review Report
+
+Generated 2026-04-08. Full codebase analysis.
+
+## Critical
+
+### 1. Shell injection in agent-manager.ts
+**File:** `src/agent-manager.ts:120-131`
+Config JSON and persona text interpolated directly into shell commands via `echo '${configJson}'`. If persona contains single quotes, shell breaks or injects arbitrary commands.
+**Fix:** Use `tee` with stdin instead of echo.
+
+### 2. IP pool has no real locking
+**File:** `src/network.ts:202-222`
+`openSync` creates a lock file but never acquires an actual `flock`. Under concurrency, two agents can allocate the same IP.
+**Fix:** Use `flock` syscall or atomic file operations.
+
+### 3. --no-snapshot flag broken
+**File:** `src/cli.ts:38`
+Commander parses `--no-snapshot` as `{ snapshot: false }`. Code checks `opts.snapshot === false` but passes it as `noSnapshot`. The flag may not work as intended.
+**Fix:** Verify Commander's behavior and align the check.
+
+## High
+
+### 4. SKILL.md parser fragile
+**File:** `agent/agent.py:69-138`
+- Requires exact 2-space/4-space indentation
+- No error reporting when skills fail to load
+- No validation of parameter types
+- Case-sensitive boolean parsing (`true` works, `True` doesn't)
+- Assumes Unix line endings
+
+### 5. SSH host key verification disabled
+**Files:** `src/ssh.ts:104`, `src/agent-manager.ts`, `src/overseer.ts`
+`hostVerifier: () => true` and `StrictHostKeyChecking=no` everywhere. Acceptable on private bridge network but should be a conscious documented decision.
+
+### 6. Memory not fully reloaded after save_memory
+**File:** `agent/agent.py:319-326`
+After save_memory, MEMORY.md index is reloaded but individual memory files are not re-read. System prompt uses stale memory until next VM restart.
+
+## Medium
+
+### 7. SSH options duplicated 4 times
+**Files:** `src/agent-manager.ts:305,410`, `src/overseer.ts:215,253`
+Same `["-o", "StrictHostKeyChecking=no", "-o", "UserKnownHostsFile=/dev/null", "-o", "ConnectTimeout=3", "-i", sshKeyPath]` array repeated. Extract to shared constant.
+
+### 8. Process termination inconsistent
+**Files:** `src/firecracker-vm.ts:140-164`, `src/agent-manager.ts:319-332`
+Two different implementations: one uses SIGTERM→wait→SIGKILL on ChildProcess, the other uses `process.kill(pid, 0)` polling. Should use one pattern.
+
+### 9. killall python3 hardcoded
+**Files:** `src/agent-manager.ts:310`, `src/agent-manager.ts:430`
+Assumes agent is always a python3 process. Fragile if agent wrapping changes.
+
+### 10. Test suite expects researcher template
+**File:** `tests/test-suite.sh:105`
+Asserts `researcher` template exists, but `scripts/install.sh` only creates worker, coder, quick.
+
+### 11. Bare exception handlers
+**File:** `src/agent-manager.ts` — 8+ `catch {}` blocks
+Swallow all errors silently. Makes debugging impossible.
+
+### 12. agent.py monolithic (598 lines)
+**File:** `agent/agent.py`
+Handles IRC, skill discovery, tool execution, memory, config reload in one file. Could split into modules but works for now.
+
+## Low
+
+### 13. Hardcoded network interface fallback
+**File:** `src/network.ts:56` — defaults to `"eno2"` if route parsing fails.
+
+### 14. Predictable mount point names
+**File:** `src/agent-manager.ts:94` — uses `Date.now()` instead of crypto random.
+
+### 15. No Firecracker binary hash verification
+**File:** `scripts/install.sh:115-124` — downloads binary without SHA256 check.
+
+### 16. Ollama response size unbounded
+**File:** `agent/agent.py:338` — `resp.read()` with no size limit.
+
+### 17. IRC message splitting at 400 chars
+**File:** `agent/agent.py:266` — IRC limit is 512 bytes including protocol overhead. Safe limit is ~380 chars.
+
+### 18. Thread safety on _last_response_time
+**File:** `agent/agent.py:485` — global updated without lock, two rapid requests could both pass cooldown.