# Code Review: `feat/container-execution`

**Branch:** `feat/container-execution`
**Commits reviewed:**
- `e68cc48` feat: implement containerized repository-based execution model
- `f68eb0c` fix: comprehensive addressing of container execution review feedback

---

## Overview

Replaces `ClaudeRunner`/`GeminiRunner` with a `ContainerRunner`, adds `RepositoryURL` to the task model, and ships ADR-006. The direction is sound. After two commits, several issues from the initial implementation were addressed, but blocking bugs remain.

---

## Issues Fixed in Round 2

- ✅ Unconditional `defer os.RemoveAll` — replaced with `success`/`isBlocked` defer
- ✅ `--session-id` invalid flag — changed to `--resume`
- ✅ Instructions file dead code — now passed via file path to `-p`
- ✅ Resume/BLOCKED handling — `e.SandboxDir` check added
- ✅ API keys via `-e` — moved to `--env-file`
- ✅ Hardcoded image name — now configurable via `cfg.ClaudeImage`/`cfg.GeminiImage`
- ✅ `ClaudeRunner`/`GeminiRunner` deleted — no more dead code
- ✅ Tests added — `container_test.go` exists

---

## Blocking Bugs

### 1. `--env-file` uses the container path, not the host path

**File:** `internal/executor/container.go` — `buildDockerArgs`

```go
"--env-file", "/workspace/.claudomator-env",
```

`--env-file` is read by the Docker CLI on the **host**, before the container starts. `/workspace/.claudomator-env` is the in-container path. The correct value is the host-side path: `filepath.Join(workspace, ".claudomator-env")`. As written, this will fail in any real deployment unless `/workspace` on the host happens to exist and match the temp dir path.

### 2. The test validates the wrong behavior

**File:** `internal/executor/container_test.go` — `TestContainerRunner_BuildDockerArgs`

The test asserts `"--env-file", "/workspace/.claudomator-env"`, locking in the bug above rather than catching it.

### 3. `--resume execID` is passed on every run, including fresh executions

**File:** `internal/executor/container.go` — `buildInnerCmd`

```go
func (r *ContainerRunner) buildInnerCmd(t *task.Task, execID string) []string {
    return []string{"claude", "-p", "...", "--resume", execID, ...}
```

For a fresh execution, `execID` is a new UUID with no corresponding Claude session. `--resume <nonexistent-session>` will cause Claude to error out immediately. This flag should only be passed when `e.ResumeSessionID != ""`, using the original session ID.

### 4. `-p` passes a file path as the prompt, not the instructions text

**File:** `internal/executor/container.go` — `buildInnerCmd`

```go
"-p", "/workspace/.claudomator-instructions.txt",
```

Claude's `-p` flag takes **instructions text**, not a file path. The container agent would receive the literal string `/workspace/.claudomator-instructions.txt` as its task. Every task would run with the wrong instructions. The instructions content must be passed as text, or a mechanism like a `CLAUDE.md` in the workspace root must be used instead.

### 5. `streamErr` is silently discarded

**File:** `internal/executor/container.go` — `Run`

```go
if waitErr == nil && streamErr == nil {
    // push
    success = true
}
if waitErr != nil {
    return fmt.Errorf("container execution failed: %w", waitErr)
}
return nil  // streamErr is never returned
```

If Claude exits 0 but the stream contained `is_error:true`, a rate-limit rejection, or a permission denial, `streamErr` is non-nil but `waitErr` is nil. The function returns `nil` and the task is marked COMPLETED with a bad result. `ClaudeRunner.execOnce` returned `streamErr` explicitly; that logic was not ported.

---

## Non-Blocking Issues

### 6. 882 lines of executor tests deleted with no replacement coverage

`claude_test.go` covered: BLOCKED sandbox preservation, session ID propagation across resume cycles, goroutine leak detection, rate limit retry, autocommit-then-push, build failure blocking autocommit, and stale sandbox recovery. The new `container_test.go` has 65 lines testing only argument construction. None of the behavioral or integration coverage was ported.

### 7. `RepositoryURL` duplicated across two structs

Both `Task.RepositoryURL` and `AgentConfig.RepositoryURL` exist. The runner reads `t.RepositoryURL`, silently ignoring `t.Agent.RepositoryURL`. YAML task files would naturally put it under `agent:`, where it is ignored.

### 8. `success` never set for tasks with nothing to push

If the container agent runs and makes no commits (valid for read-only tasks), `waitErr == nil && streamErr == nil`, but `success` is only set after a successful `git push`. If the push returns non-zero for any reason (including "nothing to push" edge cases), `success` stays false and the workspace is incorrectly preserved.

### 9. `app.js` indentation regression

The event listener registrations lost their indentation relative to the `DOMContentLoaded` block (lines 2631–2632 in the diff). Appears to be functionally inside the block still, but is a consistency issue.

### 10. ADR-006 claims "Supersedes ADR-005" but ADR-005 was not updated

ADR-005 should have a "Superseded by ADR-006" note added to its Status section.

---

## Verdict

**Not mergeable.** Issues 1–5 are all functional failures that would cause every container-based task to fail in production:

- Issue 1: `--env-file` path → Docker fails to start the container
- Issues 3 & 4: wrong `--resume` and wrong `-p` → every fresh task errors immediately
- Issue 5: `streamErr` discarded → rate limits and task errors reported as success

Fix these before merging. Issue 6 (test deletion) should also be addressed — the behavioral coverage that was deleted is exactly what's needed to catch issues 3–5 in CI.