fix: address round 3 review feedback for container execution

- Fix push failure swallowing and ensure workspace preservation on push error
- Fix wrong session ID in --resume flag and BlockedError
- Implement safer shell quoting for instructions in buildInnerCmd
- Capture and propagate actual Claude session ID from stream init message
- Clean up redundant image resolution and stale TODOs
- Mark ADR-005 as Superseded
- Consolidate RepositoryURL to Task level (removed from AgentConfig)
- Add unit test for session ID extraction in parseStream

1 files changed, 62 insertions, 51 deletions

diff --git a/docs/reviews/feat-container-execution.md b/docs/reviews/feat-container-execution.md
index 348d582..cdcc174 100644
--- a/docs/reviews/feat-container-execution.md
+++ b/docs/reviews/feat-container-execution.md
@@ -4,116 +4,127 @@
 **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. After two commits, several issues from the initial implementation were addressed, but blocking bugs remain.
+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.
 
 ---
 
-## Issues Fixed in Round 2
+## Fixed Across All Rounds
 
 - ✅ 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
+- ✅ `--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. `--env-file` uses the container path, not the host path
+### 1. Push failure is silently swallowed — task marked COMPLETED with lost commits
 
-**File:** `internal/executor/container.go` — `buildDockerArgs`
+**File:** `internal/executor/container.go` — `Run`
 
 ```go
-"--env-file", "/workspace/.claudomator-env",
+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
+    }
+}
 ```
 
-`--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.
+`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
 
-### 2. The test validates the wrong behavior
+**File:** `internal/executor/container.go` — `Run`, `buildInnerCmd`
 
-**File:** `internal/executor/container_test.go` — `TestContainerRunner_BuildDockerArgs`
+```go
+innerCmd := r.buildInnerCmd(t, e.ID, isResume)
+// ...
+claudeArgs = append(claudeArgs, "--resume", execID) // execID = e.ID
+```
 
-The test asserts `"--env-file", "/workspace/.claudomator-env"`, locking in the bug above rather than catching it.
+`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. `--resume execID` is passed on every run, including fresh executions
+### 3. `BlockedError.SessionID` is set to the execution UUID, not a Claude session ID
 
-**File:** `internal/executor/container.go` — `buildInnerCmd`
+**File:** `internal/executor/container.go`
 
 ```go
-func (r *ContainerRunner) buildInnerCmd(t *task.Task, execID string) []string {
-    return []string{"claude", "-p", "...", "--resume", execID, ...}
+return &BlockedError{
+    QuestionJSON: questionJSON,
+    SessionID:    e.ID, // For container runner, we use exec ID as session ID
 ```
 
-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.
+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. `-p` passes a file path as the prompt, not the instructions text
+### 4. `sh -c` quoting breaks on instructions with shell metacharacters
 
 **File:** `internal/executor/container.go` — `buildInnerCmd`
 
 ```go
-"-p", "/workspace/.claudomator-instructions.txt",
+claudeArgs := []string{"claude", "-p", "\"$(" + promptCmd + ")\""}
+return []string{"sh", "-c", strings.Join(claudeArgs, " ")}
 ```
 
-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
+Produces: `claude -p "$(cat /workspace/.claudomator-instructions.txt)" ...`
 
-**File:** `internal/executor/container.go` — `Run`
+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:
 
-```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
+```sh
+sh -c 'INST=$(cat /workspace/.claudomator-instructions.txt); claude -p "$INST" ...'
 ```
 
-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.
+The single-quoted outer string prevents the host shell from interpreting the inner `$INST`.
 
 ---
 
 ## Non-Blocking Issues
 
-### 6. 882 lines of executor tests deleted with no replacement coverage
+### 5. `image` variable resolved twice
 
-`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.
+**File:** `internal/executor/container.go` — `Run`
 
-### 7. `RepositoryURL` duplicated across two structs
+`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.
 
-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.
+### 6. `TODO` comment is stale and misplaced
 
-### 8. `success` never set for tasks with nothing to push
+```go
+// TODO: Support Resume/BLOCKED by re-attaching to preserved workspace.
+```
 
-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.
+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).
 
-### 9. `app.js` indentation regression
+### 7. Test coverage still missing for the most critical paths
 
-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.
+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.
 
-### 10. ADR-006 claims "Supersedes ADR-005" but ADR-005 was not updated
+### 8. ADR-006 claims "Supersedes ADR-005" but ADR-005 Status was not updated
 
-ADR-005 should have a "Superseded by ADR-006" note added to its Status section.
+ADR-005 should add a "Superseded by ADR-006" line 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:
+**Not mergeable.** Bugs 1–4 are all functional failures:
 
-- 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
+- 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
 
-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.
+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.