# Security Review Report

**Branch**: feat/134-system-prompt-support -> main
**Date**: 2026-04-17
**Scope**: Incremental review of 7 commits since ef16f93b

## Issues in Your Changes (BLOCKING)

### MEDIUM

**Path traversal in GeminiBasePromptCache.cleanupTaskFile** - `src/implementations/gemini-adapter.ts:63-68`
**Confidence**: 82%
- Problem: `cleanupTaskFile(taskId)` constructs a file path from the raw `taskId` string via `path.join(this.#cacheDir, \`${taskId}.md\`)` and calls `unlinkSync()` on it. If a malformed `taskId` containing path separators (e.g., `../../etc/important`) reaches this method, it could delete files outside the intended cache directory.
- Mitigation factors: (1) The `taskId` is generated by `createTask()` using `task-${crypto.randomUUID()}` (safe format), (2) the caller `EventDrivenWorkerPool.cleanupWorkerState` only passes `taskId` from an in-memory `Worker` object that went through spawn, (3) the `AgentAdapter.cleanup()` interface accepts `string` not `TaskId` branded type, which means the type system does not enforce the format. The path that reaches `cleanupTaskFile` in `base-agent-adapter.ts:194` uses `safeId = taskId ?? crypto.randomUUID().substring(0, 8)` for the *write* side but cleanup uses the original `taskId`.
- Impact: Low practical risk given the UUID-based generation, but the defensive pattern used in `orchestration-manager.ts:136-140` (path validation via `startsWith(expectedDir + path.sep)`) is notably absent here.
- Fix: Add a path containment check before `unlinkSync`, mirroring the existing `isWithinStateDir` pattern:
```typescript
cleanupTaskFile(taskId: string): void {
  const filePath = path.resolve(path.join(this.#cacheDir, `${taskId}.md`));
  const resolvedCacheDir = path.resolve(this.#cacheDir);
  if (!filePath.startsWith(resolvedCacheDir + path.sep)) return; // path traversal guard
  try {
    unlinkSync(filePath);
  } catch {
    // File may not exist
  }
}
```

## Issues in Code You Touched (Should Fix)

_No issues found._

## Pre-existing Issues (Not Blocking)

_No CRITICAL pre-existing security issues found in reviewed files._

## Suggestions (Lower Confidence)

- **System prompt passed as CLI arg is visible in process list** - `src/implementations/claude-adapter.ts:48`, `src/implementations/codex-adapter.ts:41` (Confidence: 65%) -- Both Claude (`--append-system-prompt`) and Codex (`-c developer_instructions=...`) pass the full system prompt text as a command-line argument. On multi-user systems, other users can see CLI args via `ps aux`. The system prompt may contain sensitive instructions. Gemini avoids this by writing to a file with 0o600 permissions and passing via env var. This is a design tradeoff, not a bug -- the Claude and Codex CLIs do not offer a file-based system prompt mechanism.

- **No size limit enforcement on combined Gemini system prompt file on disk** - `src/implementations/gemini-adapter.ts:47-58` (Confidence: 62%) -- The `MAX_COMBINED_PROMPT_BYTES` (64KB) guard prevents large combined prompts, but the base cache file (`gemini-base.md`) itself is read without size validation in `#ensureCacheLoaded`. A corrupt or maliciously large cache file could cause memory pressure. The cache is written by the user via `beat agents refresh-base-prompt`, so this requires local user action.

- **Codex developer_instructions injection** - `src/implementations/codex-adapter.ts:41` (Confidence: 60%) -- The system prompt is passed as `-c developer_instructions=<text>`. If the Codex CLI uses shell-level parsing of the `-c` flag value, special characters in the system prompt (backticks, `$()`) could be interpreted. This depends entirely on how the Codex CLI parses the `-c` argument internally. Node.js `spawn()` does not invoke a shell by default, so the direct spawn is safe. No action needed unless `shell: true` is ever added.

## Summary

| Category | CRITICAL | HIGH | MEDIUM | LOW |
|----------|----------|------|--------|-----|
| Blocking | 0 | 0 | 1 | 0 |
| Should Fix | 0 | 0 | 0 | 0 |
| Pre-existing | 0 | 0 | 0 | 0 |

**Security Score**: 8/10
**Recommendation**: APPROVED_WITH_CONDITIONS

The incremental changes demonstrate strong security awareness: file permissions are correctly set (0o700 for directories, 0o600 for files), the `safeId` fallback with `crypto.randomUUID()` prevents path collisions, the Gemini cache has size guards and staleness checks, the orchestratorId validation regex in `base-agent-adapter.ts` prevents log injection, and the worker pool cleanup was correctly migrated from a hardcoded path to adapter-owned cleanup. The one remaining concern (path traversal in `cleanupTaskFile`) is low risk due to UUID-based task IDs but should get the same defensive check already used elsewhere in the codebase.

### Pitfall Check (PF-006)
Verified PF-006 (paired-interface drift) against this diff. The new `cleanup(taskId: string)` method was added to both the `AgentAdapter` interface (`src/core/agents.ts:296`) and all implementations (`BaseAgentAdapter`, `ProcessSpawnerAdapter`, `GeminiAdapter`). No drift detected -- the resolution from PF-006 was correctly applied.
