# 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
- `ad48791` fix: address round 2 review feedback for container execution

---

## Overview

Replaces `ClaudeRunner`/`GeminiRunner` with a `ContainerRunner`, adds `RepositoryURL` to the task model, and ships ADR-006. The direction is sound. Three rounds of iteration have fixed most of the original issues, but four blocking bugs remain.

---

## Fixed Across All Rounds

- ✅ Unconditional `defer os.RemoveAll` — replaced with `success`/`isBlocked` defer
- ✅ `--session-id` invalid flag — changed to `--resume`
- ✅ `--resume` on fresh runs — `isResume bool` parameter added to `buildInnerCmd`
- ✅ `-p` passes file path literally — now uses `sh -c "claude -p \"$(cat ...)\""`
- ✅ `streamErr` silently discarded — now returned
- ✅ API keys via `-e` — moved to `--env-file` with host-side path
- ✅ Hardcoded image name — configurable via `cfg.ClaudeImage`/`cfg.GeminiImage`
- ✅ `ClaudeRunner`/`GeminiRunner` orphaned — deleted
- ✅ `RepositoryURL` not checked in `AgentConfig` — fallback added
- ✅ `app.js` indentation regression — fixed
- ✅ Test coverage expanded — `isCompletionReport`, `tailFile`, `gitSafe`, workspace preservation tests added

---

## Blocking Bugs

### 1. Push failure is silently swallowed — task marked COMPLETED with lost commits

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

```go
if waitErr == nil && streamErr == nil {
    success = true // set BEFORE push
    if out, err := exec.CommandContext(..., "git", "-C", workspace, "push", "origin", "HEAD").CombinedOutput(); err != nil {
        r.Logger.Warn("git push failed or no changes", ...)
        // error not returned
    }
}
```

`success = true` before the push means the workspace is cleaned up whether the push succeeds or not. Push errors are only logged. If the agent commits changes and the push fails (auth, non-fast-forward, network), the task is marked COMPLETED, the workspace is deleted, and the commits are gone. ADR-006 explicitly states: *"If the remote is missing or the push fails, the task is marked FAILED and the host-side workspace is preserved for inspection."* This is the opposite.

### 2. `--resume` is passed with the wrong session ID

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

```go
innerCmd := r.buildInnerCmd(t, e.ID, isResume)
// ...
claudeArgs = append(claudeArgs, "--resume", execID) // execID = e.ID
```

`e.ID` is the *current* execution's UUID. `--resume` requires the *previous* Claude session ID, stored in `e.ResumeSessionID`. Passing the wrong ID causes Claude to error with "No conversation found". Should be `e.ResumeSessionID`.

### 3. `BlockedError.SessionID` is set to the execution UUID, not a Claude session ID

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

```go
return &BlockedError{
    QuestionJSON: questionJSON,
    SessionID:    e.ID, // For container runner, we use exec ID as session ID
```

The pool stores `BlockedError.SessionID` as the session to `--resume` when the user answers. Using `e.ID` means the resume invocation will fail — Claude has no session with that UUID. The actual Claude session ID must come from the stream output or an agent-written file. `ClaudeRunner` handled this via `e.SessionID` which was set before the run and populated into the stream's session context.

### 4. `sh -c` quoting breaks on instructions with shell metacharacters

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

```go
claudeArgs := []string{"claude", "-p", "\"$(" + promptCmd + ")\""}
return []string{"sh", "-c", strings.Join(claudeArgs, " ")}
```

Produces: `claude -p "$(cat /workspace/.claudomator-instructions.txt)" ...`

If the instructions file contains `"`, `` ` ``, `$VAR`, or `\`, the shell expansion breaks or executes unintended commands. Task instructions routinely contain code snippets with all of these. A safer pattern uses a shell variable to capture and isolate the expansion:

```sh
sh -c 'INST=$(cat /workspace/.claudomator-instructions.txt); claude -p "$INST" ...'
```

The single-quoted outer string prevents the host shell from interpreting the inner `$INST`.

---

## Non-Blocking Issues

### 5. `image` variable resolved twice

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

`image` is resolved (ContainerImage → r.Image → default) at the top of `Run`, then the identical three-way resolution runs again after `buildInnerCmd` is called. The first value is immediately overwritten — dead code.

### 6. `TODO` comment is stale and misplaced

```go
// TODO: Support Resume/BLOCKED by re-attaching to preserved workspace.
```

Resume workspace reuse is already implemented in step 1 (`e.SandboxDir` check). The BLOCKED path is handled after `cmd.Wait()`. The comment is inaccurate; the actual unresolved issue is the session ID problem (bug #3 above).

### 7. Test coverage still missing for the most critical paths

Round 3 restored `isCompletionReport`, `tailFile`, and `gitSafe` tests. Still missing: goroutine leak detection, rate-limit retry behavior, and session ID propagation across a BLOCKED → resume cycle. These are the tests most likely to catch bugs #2 and #3 in CI.

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

ADR-005 should add a "Superseded by ADR-006" line to its Status section.

---

## Verdict

**Not mergeable.** Bugs 1–4 are all functional failures:

- Bug 1: silently discarded push failures → lost commits, false COMPLETED status
- Bugs 2 & 3: wrong session IDs → every resume fails with "No conversation found"
- Bug 4: shell quoting → any task with code in its instructions silently misbehaves

Bug 1 is a regression introduced in round 3 (previously push failures correctly failed the task). Bugs 2–3 have been present since the first commit and were not caught by the new tests because no test exercises the BLOCKED → resume flow end-to-end.