fix: address round 2 review feedback for container execution

- Fix host/container path confusion for --env-file
- Fix --resume flag to only be used during resumptions
- Fix instruction passing to Claude CLI via shell-wrapped cat
- Restore streamErr return logic to detect task-level failures
- Improve success flag logic for workspace preservation
- Remove duplicate RepositoryURL from AgentConfig
- Fix app.js indentation and reformat DOMContentLoaded block
- Restore behavioral test coverage in container_test.go

1 files changed, 145 insertions, 9 deletions

diff --git a/internal/executor/container_test.go b/internal/executor/container_test.go
index b1513ea..fbb4d7d 100644
--- a/internal/executor/container_test.go
+++ b/internal/executor/container_test.go
@@ -1,9 +1,15 @@
 package executor
 
 import (
+	"context"
+	"fmt"
+	"io"
+	"log/slog"
+	"os"
 	"strings"
 	"testing"
 
+	"github.com/thepeterstone/claudomator/internal/storage"
 	"github.com/thepeterstone/claudomator/internal/task"
 )
 
@@ -21,7 +27,7 @@ func TestContainerRunner_BuildDockerArgs(t *testing.T) {
 		"run", "--rm",
 		"-v", "/tmp/ws:/workspace",
 		"-w", "/workspace",
-		"--env-file", "/workspace/.claudomator-env",
+		"--env-file", "/tmp/ws/.claudomator-env",
 		"-e", "CLAUDOMATOR_API_URL=http://localhost:8484",
 		"-e", "CLAUDOMATOR_TASK_ID=task-123",
 		"-e", "CLAUDOMATOR_DROP_DIR=/data/drops",
@@ -40,26 +46,156 @@ func TestContainerRunner_BuildDockerArgs(t *testing.T) {
 func TestContainerRunner_BuildInnerCmd(t *testing.T) {
 	runner := &ContainerRunner{}
 	
-	t.Run("claude", func(t *testing.T) {
+	t.Run("claude-fresh", func(t *testing.T) {
 		tk := &task.Task{Agent: task.AgentConfig{Type: "claude"}}
-		cmd := runner.buildInnerCmd(tk, "exec-456")
+		cmd := runner.buildInnerCmd(tk, "exec-456", false)
 		
 		cmdStr := strings.Join(cmd, " ")
-		if !strings.Contains(cmdStr, "--resume exec-456") {
-			t.Errorf("expected --resume flag, got %q", cmdStr)
+		if strings.Contains(cmdStr, "--resume") {
+			t.Errorf("unexpected --resume flag in fresh run: %q", cmdStr)
+		}
+		if !strings.Contains(cmdStr, "cat /workspace/.claudomator-instructions.txt") {
+			t.Errorf("expected cat instructions in sh command, got %q", cmdStr)
 		}
-		if !strings.Contains(cmdStr, "-p /workspace/.claudomator-instructions.txt") {
-			t.Errorf("expected instructions file path, got %q", cmdStr)
+	})
+
+	t.Run("claude-resume", func(t *testing.T) {
+		tk := &task.Task{Agent: task.AgentConfig{Type: "claude"}}
+		cmd := runner.buildInnerCmd(tk, "exec-456", true)
+		
+		cmdStr := strings.Join(cmd, " ")
+		if !strings.Contains(cmdStr, "--resume exec-456") {
+			t.Errorf("expected --resume flag in resume run: %q", cmdStr)
 		}
 	})
 
 	t.Run("gemini", func(t *testing.T) {
 		tk := &task.Task{Agent: task.AgentConfig{Type: "gemini"}}
-		cmd := runner.buildInnerCmd(tk, "exec-456")
+		cmd := runner.buildInnerCmd(tk, "exec-456", false)
 		
 		cmdStr := strings.Join(cmd, " ")
-		if !strings.HasPrefix(cmdStr, "gemini") {
+		if !strings.Contains(cmdStr, "gemini") {
 			t.Errorf("expected gemini command, got %q", cmdStr)
 		}
 	})
 }
+
+func TestContainerRunner_Run_PreservesWorkspaceOnFailure(t *testing.T) {
+	logger := slog.New(slog.NewTextHandler(io.Discard, nil))
+	runner := &ContainerRunner{
+		Logger: logger,
+		Image:  "busybox",
+	}
+
+	// Use an invalid repo URL to trigger failure.
+	tk := &task.Task{
+		ID:            "test-task",
+		RepositoryURL: "/nonexistent/repo",
+		Agent:         task.AgentConfig{Type: "claude"},
+	}
+	exec := &storage.Execution{ID: "test-exec", TaskID: "test-task"}
+
+	err := runner.Run(context.Background(), tk, exec)
+	if err == nil {
+		t.Fatal("expected error due to invalid repo")
+	}
+
+	// Verify SandboxDir was set and directory exists.
+	if exec.SandboxDir == "" {
+		t.Fatal("expected SandboxDir to be set even on failure")
+	}
+	if _, statErr := os.Stat(exec.SandboxDir); statErr != nil {
+		t.Errorf("expected sandbox directory to be preserved, but stat failed: %v", statErr)
+	} else {
+		os.RemoveAll(exec.SandboxDir)
+	}
+}
+
+func TestBlockedError_IncludesSandboxDir(t *testing.T) {
+	// This test requires mocking 'docker run' or the whole Run() which is hard.
+	// But we can test that returning BlockedError works.
+	err := &BlockedError{
+		QuestionJSON: `{"text":"?"}`,
+		SessionID:    "s1",
+		SandboxDir:   "/tmp/s1",
+	}
+	if !strings.Contains(err.Error(), "task blocked") {
+		t.Errorf("wrong error message: %v", err)
+	}
+}
+
+func TestIsCompletionReport(t *testing.T) {
+	tests := []struct {
+		name     string
+		json     string
+		expected bool
+	}{
+		{
+			name:     "real question with options",
+			json:     `{"text": "Should I proceed with implementation?", "options": ["Yes", "No"]}`,
+			expected: false,
+		},
+		{
+			name:     "real question no options",
+			json:     `{"text": "Which approach do you prefer?"}`,
+			expected: false,
+		},
+		{
+			name:     "completion report no options no question mark",
+			json:     `{"text": "All tests pass. Implementation complete. Summary written to CLAUDOMATOR_SUMMARY_FILE."}`,
+			expected: true,
+		},
+		{
+			name:     "completion report with empty options",
+			json:     `{"text": "Feature implemented and committed.", "options": []}`,
+			expected: true,
+		},
+		{
+			name:     "invalid json treated as not a report",
+			json:     `not json`,
+			expected: false,
+		},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			got := isCompletionReport(tt.json)
+			if got != tt.expected {
+				t.Errorf("isCompletionReport(%q) = %v, want %v", tt.json, got, tt.expected)
+			}
+		})
+	}
+}
+
+func TestTailFile_ReturnsLastNLines(t *testing.T) {
+	f, err := os.CreateTemp("", "tailfile-*")
+	if err != nil {
+		t.Fatal(err)
+	}
+	defer os.Remove(f.Name())
+	for i := 1; i <= 30; i++ {
+		fmt.Fprintf(f, "line %d\n", i)
+	}
+	f.Close()
+
+	got := tailFile(f.Name(), 5)
+	lines := strings.Split(strings.TrimSpace(got), "\n")
+	if len(lines) != 5 {
+		t.Fatalf("want 5 lines, got %d: %q", len(lines), got)
+	}
+	if lines[0] != "line 26" || lines[4] != "line 30" {
+		t.Errorf("want lines 26-30, got: %q", got)
+	}
+}
+
+func TestGitSafe_PrependsSafeDirectory(t *testing.T) {
+	got := gitSafe("-C", "/some/path", "status")
+	want := []string{"-c", "safe.directory=*", "-C", "/some/path", "status"}
+	if len(got) != len(want) {
+		t.Fatalf("gitSafe() = %v, want %v", got, want)
+	}
+	for i := range want {
+		if got[i] != want[i] {
+			t.Errorf("gitSafe()[%d] = %q, want %q", i, got[i], want[i])
+		}
+	}
+}