# Fireclaw Code Review Report

Generated 2026-04-08. Full codebase analysis.

## Critical

### 1. Shell injection in agent-manager.ts — FIXED
**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:** Replaced with `tee` via stdin. No shell interpolation.

### 2. IP pool has no real locking — FIXED
**File:** `src/network.ts:202-222`
`openSync` created a lock file but never acquired an actual `flock`. Under concurrency, two agents could allocate the same IP.
**Fix:** Atomic writes via `writeFileSync` + `renameSync`. Removed fake lock.

### 3. --no-snapshot flag broken — FALSE ALARM
**File:** `src/cli.ts:38`
Commander parses `--no-snapshot` as `{ snapshot: false }`. Verified: `opts.snapshot === false` is correct. No fix needed.

## High

### 4. SKILL.md parser fragile — FIXED
**File:** `agent/agent.py:69-138`
**Fix:** Added error logging on parse failures, flexible indent detection (2+ spaces), CRLF normalization, boolean case-insensitive (`true`/`True`/`yes`/`1`), parameter type validation with warnings.

### 5. SSH host key verification disabled — DOCUMENTED
**Files:** `src/ssh.ts:104`, `src/agent-manager.ts`, `src/overseer.ts`
`hostVerifier: () => true` and `StrictHostKeyChecking=no` everywhere. Acceptable on private bridge network (172.16.0.0/24) — VMs are ephemeral and host keys change on every boot. Conscious design decision, not an oversight.

### 6. Memory not fully reloaded after save_memory — FIXED
**File:** `agent/agent.py:319-326`
After save_memory, only MEMORY.md index was reloaded. Individual memory files were not re-read into system prompt.
**Fix:** Extracted `reload_memory()` function that reloads index + all memory/*.md files.

## Medium

### 7. SSH options duplicated — FIXED
**Files:** `src/agent-manager.ts`, `src/overseer.ts`
Same SSH options array repeated 4+ times.
**Fix:** Extracted `SSH_OPTS` constant in both files.

### 8. Process termination inconsistent — OPEN (low risk)
**Files:** `src/firecracker-vm.ts:140-164`, `src/agent-manager.ts:319-332`
Two different implementations. The ChildProcess version uses SIGTERM→SIGKILL, the PID version uses polling. Both work, different contexts (owned vs adopted processes).

### 9. killall python3 hardcoded — FIXED
**Files:** `src/agent-manager.ts`
**Fix:** Replaced `killall python3` with `pkill -f 'agent.py'`. Targets the specific script, not all python3 processes.

### 10. Test suite expects researcher template — FIXED
**File:** `tests/test-suite.sh:105`
Test asserts `researcher` template exists, but install script only created worker, coder, quick.
**Fix:** Added researcher and creative templates to `scripts/install.sh`.

### 11. Bare exception handlers — FIXED
**File:** `src/agent-manager.ts`
**Fix:** Added error logging to cleanup catch blocks in listAgents and reconcileAgents. Remaining bare catches are intentional best-effort cleanup (umount, rmdir, unlink).

### 12. agent.py monolithic (598 lines) — OPEN
**File:** `agent/agent.py`
Handles IRC, skill discovery, tool execution, memory, config reload in one file. Functional but could benefit from splitting.

### 13. Unused writePool function — FIXED
**File:** `src/network.ts:198`
Left over after switching to `atomicWritePool`. Removed.

## Low

### 14. Hardcoded network interface fallback
**File:** `src/network.ts:56` — defaults to `"eno2"` if route parsing fails.

### 15. Predictable mount point names
**File:** `src/agent-manager.ts:94` — uses `Date.now()` instead of crypto random.

### 16. No Firecracker binary hash verification
**File:** `scripts/install.sh:115-124` — downloads binary without SHA256 check.

### 17. Ollama response size unbounded
**File:** `agent/agent.py:338` — `resp.read()` with no size limit.

### 18. IRC message splitting at 400 chars — FIXED
**File:** `agent/agent.py:266`
**Fix:** Reduced to 380 chars to stay within IRC's 512-byte line limit.

### 19. Thread safety on _last_response_time — FIXED
**File:** `agent/agent.py:485`
**Fix:** Added `_cooldown_lock` (threading.Lock) around cooldown check-and-set.

## Summary

| Status | Count |
|---|---|
| Fixed | 13 |
| False alarm | 1 |
| Open (medium) | 2 |
| Open (low) | 4 |