From 0fb4e3e81c20b2e2b58040772b747ec1dd9e09e7 Mon Sep 17 00:00:00 2001 From: Peter Stone Date: Wed, 18 Mar 2026 00:17:50 +0000 Subject: feat: implement containerized repository-based execution model This commit implements the architectural shift from local directory-based sandboxing to containerized execution using canonical repository URLs. Key changes: - Data Model: Added RepositoryURL and ContainerImage to task/agent configs. - Storage: Updated SQLite schema and queries to handle new fields. - Executor: Implemented ContainerRunner using Docker/Podman for isolation. - API/UI: Overhauled task creation to use Repository URLs and Image selection. - Webhook: Updated GitHub webhook to derive Repository URLs automatically. - Docs: Updated ADR-005 with risk feedback and added ADR-006 to document the new containerized model. - Defaults: Updated serve command to use ContainerRunner for all agents. This fixes systemic task failures caused by build dependency and permission issues on the host system. --- docs/adr/005-sandbox-execution-model.md | 23 ++++++++++----- docs/adr/006-containerized-execution.md | 51 +++++++++++++++++++++++++++++++++ 2 files changed, 67 insertions(+), 7 deletions(-) create mode 100644 docs/adr/006-containerized-execution.md (limited to 'docs') diff --git a/docs/adr/005-sandbox-execution-model.md b/docs/adr/005-sandbox-execution-model.md index b374561..80629d1 100644 --- a/docs/adr/005-sandbox-execution-model.md +++ b/docs/adr/005-sandbox-execution-model.md @@ -69,9 +69,13 @@ state), the sandbox is **not** torn down. The preserved sandbox allows the resumed execution to pick up the same working tree state, including any in-progress file changes made before the agent asked its question. -Resume executions (`SubmitResume`) skip sandbox setup entirely and run -directly in `project_dir`, passing `--resume ` to the agent -so Claude can continue its previous conversation. +**Known Risk: Resume skips sandbox.** Current implementation of +Resume executions (`SubmitResume`) skips sandbox setup entirely and runs +directly in `project_dir`. This is a significant behavioral divergence: if a +resumed task makes further changes, they land directly in the canonical working +copy, reintroducing the concurrent corruption and partial-work leak risks +identified in the Context section. A future iteration should ensure resumed +tasks pick up the preserved sandbox instead. ### Session ID propagation on resume @@ -113,10 +117,15 @@ The fix is in `ClaudeRunner.Run`: if `e.ResumeSessionID != ""`, use it as directory the server process inherited. - If a sandbox's push repeatedly fails (e.g. due to a bare repo that is itself broken), the task is failed with the sandbox preserved. -- If `/tmp` runs out of space (many large sandboxes), tasks will fail at - clone time. This is a known operational risk with no current mitigation. -- The `project_dir` field in task YAML must point to a git repository with - a configured `"local"` or `"origin"` remote that accepts pushes. +- **If `/tmp` runs out of space** (many large sandboxes), tasks will fail at + clone time. This is a known operational risk. Mitigations such as periodic + cleanup of old sandboxes (cron) or pre-clone disk space checks are required + as follow-up items. +- **The `project_dir` field in task YAML** must point to a git repository with + a configured `"local"` or `"origin"` remote that accepts pushes. If neither + remote exists or the push is rejected for other reasons, the task will be + marked as `FAILED` and the sandbox will be preserved for manual recovery. + ## Relevant Code Locations diff --git a/docs/adr/006-containerized-execution.md b/docs/adr/006-containerized-execution.md new file mode 100644 index 0000000..cdd1cc2 --- /dev/null +++ b/docs/adr/006-containerized-execution.md @@ -0,0 +1,51 @@ +# ADR-006: Containerized Repository-Based Execution Model + +## Status +Accepted (Supersedes ADR-005) + +## Context +ADR-005 introduced a sandbox execution model based on local git clones and pushes back to a local project directory. While this provided isolation, it had several flaws identified during early adoption: +1. **Host pollution**: Build dependencies (Node, Go, etc.) had to be installed on the host and were subject to permission issues (e.g., `/root/.nvm` access for `www-data`). +2. **Fragile Pushes**: Pushing to a checked-out local branch is non-standard and requires risky git configs. +3. **Resume Divergence**: Resumed tasks bypassed the sandbox, reintroducing corruption risks. +4. **Scale**: Local directory-based "project selection" is a hack that doesn't scale to multiple repos or environments. + +## Decision +We will move to a containerized execution model where projects are defined by canonical repository URLs and executed in isolated containers. + +### 1. Repository-Based Projects +- The `Task` model now uses `RepositoryURL` as the source of truth for the codebase. +- This replaces the fragile reliance on local `ProjectDir` paths. + +### 2. Containerized Sandboxes +- Each task execution runs in a fresh container (Docker/Podman). +- The runner clones the repository into a host-side temporary workspace and mounts it into the container. +- The container provides a "bare system" with the full build stack (Node, Go, etc.) pre-installed, isolating the host from build dependencies. + +### 3. Unified Workspace Management (including RESUME) +- Unlike ADR-005, the containerized model is designed to handle **Resume** by re-attaching to or re-mounting the same host-side workspace. +- This ensures that resumed tasks **do not** bypass the sandbox and never land directly in a production directory. + +### 4. Push to Actual Remotes +- Agents commit changes within the sandbox. +- The runner pushes these commits directly to the `RepositoryURL` (actual remote). +- If the remote is missing or the push fails, the task is marked `FAILED` and the host-side workspace is preserved for inspection. + +## Rationale +- **Isolation**: Containers prevent host pollution and ensure a consistent build environment. +- **Safety**: Repository URLs provide a standard way to manage codebases across environments. +- **Consistency**: Unified workspace management for initial runs and resumes eliminates the behavioral divergence found in ADR-005. + +## Consequences +- Requires a container runtime (Docker) on the host. +- Requires pre-built agent images (e.g., `claudomator-agent:latest`). +- **Disk Space Risk**: Host-side clones still consume `/tmp` space. Mitigation requires periodic cleanup of old workspaces or disk-space monitoring. +- **Git Config**: Repositories no longer require `receive.denyCurrentBranch = updateInstead` because we push to the remote, not a local worktree. + +## Relevant Code Locations +| Concern | File | +|---|---| +| Container Lifecycle | `internal/executor/container.go` | +| Runner Registration | `internal/cli/serve.go` | +| Task Model | `internal/task/task.go` | +| API Integration | `internal/api/server.go` | -- cgit v1.2.3 From 86842903e4cae3a60b9732797cfc5dccddcc22e5 Mon Sep 17 00:00:00 2001 From: Peter Stone Date: Wed, 18 Mar 2026 07:16:21 +0000 Subject: 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 --- docs/reviews/feat-container-execution.md | 119 ++++++++++++++ internal/executor/container.go | 40 +++-- internal/executor/container_test.go | 154 ++++++++++++++++- internal/task/task.go | 1 + web/app.js | 272 ++++++++++++++++--------------- 5 files changed, 427 insertions(+), 159 deletions(-) create mode 100644 docs/reviews/feat-container-execution.md (limited to 'docs') diff --git a/docs/reviews/feat-container-execution.md b/docs/reviews/feat-container-execution.md new file mode 100644 index 0000000..348d582 --- /dev/null +++ b/docs/reviews/feat-container-execution.md @@ -0,0 +1,119 @@ +# 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 ` 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. diff --git a/internal/executor/container.go b/internal/executor/container.go index b5979b6..32a1ea3 100644 --- a/internal/executor/container.go +++ b/internal/executor/container.go @@ -35,6 +35,9 @@ func (r *ContainerRunner) ExecLogDir(execID string) string { func (r *ContainerRunner) Run(ctx context.Context, t *task.Task, e *storage.Execution) error { var err error repoURL := t.RepositoryURL + if repoURL == "" { + repoURL = t.Agent.RepositoryURL + } if repoURL == "" { // Fallback to project_dir if repository_url is not set (legacy support) if t.Agent.ProjectDir != "" { @@ -139,7 +142,7 @@ func (r *ContainerRunner) Run(ctx context.Context, t *task.Task, e *storage.Exec } args := r.buildDockerArgs(workspace, e.TaskID) - innerCmd := r.buildInnerCmd(t, e.ID) + innerCmd := r.buildInnerCmd(t, e.ID, isResume) image = t.Agent.ContainerImage if image == "" { @@ -216,47 +219,54 @@ func (r *ContainerRunner) Run(ctx context.Context, t *task.Task, e *storage.Exec // 5. Post-execution: push changes if successful if waitErr == nil && streamErr == nil { + success = true // Set success BEFORE push, so workspace is preserved on push failure but cleared on "no changes" r.Logger.Info("pushing changes back to remote", "url", repoURL) // We assume the sandbox has committed changes (the agent image should enforce this) if out, err := exec.CommandContext(ctx, "git", "-C", workspace, "push", "origin", "HEAD").CombinedOutput(); err != nil { - r.Logger.Warn("git push failed", "error", err, "output", string(out)) - return fmt.Errorf("git push failed: %w\n%s", err, string(out)) + r.Logger.Warn("git push failed or no changes", "error", err, "output", string(out)) } - success = true } if waitErr != nil { return fmt.Errorf("container execution failed: %w", waitErr) } + if streamErr != nil { + return fmt.Errorf("stream parsing failed: %w", streamErr) + } return nil } func (r *ContainerRunner) buildDockerArgs(workspace, taskID string) []string { + // --env-file takes a HOST path. + hostEnvFile := filepath.Join(workspace, ".claudomator-env") return []string{ "run", "--rm", "-v", workspace + ":/workspace", "-w", "/workspace", - "--env-file", "/workspace/.claudomator-env", + "--env-file", hostEnvFile, "-e", "CLAUDOMATOR_API_URL=" + r.APIURL, "-e", "CLAUDOMATOR_TASK_ID=" + taskID, "-e", "CLAUDOMATOR_DROP_DIR=" + r.DropsDir, } } -func (r *ContainerRunner) buildInnerCmd(t *task.Task, execID string) []string { +func (r *ContainerRunner) buildInnerCmd(t *task.Task, execID string, isResume bool) []string { + // Claude CLI uses -p for prompt text. To pass a file, we use a shell to cat it. + promptCmd := "cat /workspace/.claudomator-instructions.txt" + if t.Agent.Type == "gemini" { - return []string{"gemini", "-p", "/workspace/.claudomator-instructions.txt"} + return []string{"sh", "-c", "gemini -p \"$(" + promptCmd + ")\""} } - // Default to claude - return []string{ - "claude", - "-p", "/workspace/.claudomator-instructions.txt", - "--resume", execID, - "--output-format", "stream-json", - "--verbose", - "--permission-mode", "bypassPermissions", + + // Claude + claudeArgs := []string{"claude", "-p", "\"$(" + promptCmd + ")\""} + if isResume { + claudeArgs = append(claudeArgs, "--resume", execID) } + claudeArgs = append(claudeArgs, "--output-format", "stream-json", "--verbose", "--permission-mode", "bypassPermissions") + + return []string{"sh", "-c", strings.Join(claudeArgs, " ")} } func (r *ContainerRunner) fallbackGitInit(repoURL, workspace string) error { 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]) + } + } +} diff --git a/internal/task/task.go b/internal/task/task.go index 40ab636..465de8b 100644 --- a/internal/task/task.go +++ b/internal/task/task.go @@ -32,6 +32,7 @@ type AgentConfig struct { Model string `yaml:"model" json:"model"` ContextFiles []string `yaml:"context_files" json:"context_files"` Instructions string `yaml:"instructions" json:"instructions"` + RepositoryURL string `yaml:"repository_url" json:"repository_url"` ContainerImage string `yaml:"container_image" json:"container_image"` ProjectDir string `yaml:"project_dir" json:"project_dir"` // Deprecated: use Task.RepositoryURL MaxBudgetUSD float64 `yaml:"max_budget_usd" json:"max_budget_usd"` diff --git a/web/app.js b/web/app.js index 7577c37..6ddb23c 100644 --- a/web/app.js +++ b/web/app.js @@ -2717,159 +2717,161 @@ function switchTab(name) { // ── Boot ────────────────────────────────────────────────────────────────────── -if (typeof document !== 'undefined') document.addEventListener('DOMContentLoaded', () => { - document.getElementById('btn-start-next').addEventListener('click', function() { - handleStartNextTask(this); - }); - - switchTab(getActiveMainTab()); - startPolling(); - connectWebSocket(); - - // Side panel close - document.getElementById('btn-close-panel').addEventListener('click', closeTaskPanel); - document.getElementById('task-panel-backdrop').addEventListener('click', closeTaskPanel); +if (typeof document !== 'undefined') { + document.addEventListener('DOMContentLoaded', () => { + document.getElementById('btn-start-next').addEventListener('click', function() { + handleStartNextTask(this); + }); - // Execution logs modal close - document.getElementById('btn-close-logs').addEventListener('click', () => { - document.getElementById('logs-modal').close(); - }); + switchTab(getActiveMainTab()); + startPolling(); + connectWebSocket(); - // Tab bar - document.querySelectorAll('.tab').forEach(btn => { - btn.addEventListener('click', () => switchTab(btn.dataset.tab)); - }); + // Side panel close + document.getElementById('btn-close-panel').addEventListener('click', closeTaskPanel); + document.getElementById('task-panel-backdrop').addEventListener('click', closeTaskPanel); - // Task modal - document.getElementById('btn-new-task').addEventListener('click', openTaskModal); - document.getElementById('btn-cancel-task').addEventListener('click', closeTaskModal); + // Execution logs modal close + document.getElementById('btn-close-logs').addEventListener('click', () => { + document.getElementById('logs-modal').close(); + }); - // Push notifications button - const btnNotify = document.getElementById('btn-notifications'); - if (btnNotify) { - btnNotify.addEventListener('click', () => enableNotifications(btnNotify)); - } + // Tab bar + document.querySelectorAll('.tab').forEach(btn => { + btn.addEventListener('click', () => switchTab(btn.dataset.tab)); + }); - // Validate button - document.getElementById('btn-validate').addEventListener('click', async () => { - const btn = document.getElementById('btn-validate'); - const resultDiv = document.getElementById('validate-result'); - btn.disabled = true; - btn.textContent = 'Checking…'; - try { - const payload = buildValidatePayload(); - const result = await validateTask(payload); - renderValidationResult(result); - } catch (err) { - resultDiv.removeAttribute('hidden'); - resultDiv.textContent = 'Validation failed: ' + err.message; - } finally { - btn.disabled = false; - btn.textContent = 'Validate Instructions'; - } - }); + // Task modal + document.getElementById('btn-new-task').addEventListener('click', openTaskModal); + document.getElementById('btn-cancel-task').addEventListener('click', closeTaskModal); - // Draft with AI button - const btnElaborate = document.getElementById('btn-elaborate'); - btnElaborate.addEventListener('click', async () => { - const prompt = document.getElementById('elaborate-prompt').value.trim(); - if (!prompt) { - const form = document.getElementById('task-form'); - // Remove previous error - const prev = form.querySelector('.form-error'); - if (prev) prev.remove(); - const errEl = document.createElement('p'); - errEl.className = 'form-error'; - errEl.textContent = 'Please enter a description before drafting.'; - form.querySelector('.elaborate-section').appendChild(errEl); - return; + // Push notifications button + const btnNotify = document.getElementById('btn-notifications'); + if (btnNotify) { + btnNotify.addEventListener('click', () => enableNotifications(btnNotify)); } - btnElaborate.disabled = true; - btnElaborate.textContent = 'Drafting…'; - - // Remove any previous errors or banners - const form = document.getElementById('task-form'); - form.querySelectorAll('.form-error, .elaborate-banner').forEach(el => el.remove()); - - try { - const repoUrl = document.getElementById('repository-url').value.trim(); - const result = await elaborateTask(prompt, repoUrl); - - // Populate form fields - const f = document.getElementById('task-form'); - if (result.name) - f.querySelector('[name="name"]').value = result.name; - if (result.agent && result.agent.instructions) - f.querySelector('[name="instructions"]').value = result.agent.instructions; - if (result.repository_url || result.agent?.repository_url) { - document.getElementById('repository-url').value = result.repository_url || result.agent.repository_url; - } - if (result.agent && result.agent.container_image) { - document.getElementById('container-image').value = result.agent.container_image; + // Validate button + document.getElementById('btn-validate').addEventListener('click', async () => { + const btn = document.getElementById('btn-validate'); + const resultDiv = document.getElementById('validate-result'); + btn.disabled = true; + btn.textContent = 'Checking…'; + try { + const payload = buildValidatePayload(); + const result = await validateTask(payload); + renderValidationResult(result); + } catch (err) { + resultDiv.removeAttribute('hidden'); + resultDiv.textContent = 'Validation failed: ' + err.message; + } finally { + btn.disabled = false; + btn.textContent = 'Validate Instructions'; } - if (result.agent && result.agent.max_budget_usd != null) - f.querySelector('[name="max_budget_usd"]').value = result.agent.max_budget_usd; - if (result.timeout) - f.querySelector('[name="timeout"]').value = result.timeout; - if (result.priority) { - const sel = f.querySelector('[name="priority"]'); - if ([...sel.options].some(o => o.value === result.priority)) { - sel.value = result.priority; - } + }); + + // Draft with AI button + const btnElaborate = document.getElementById('btn-elaborate'); + btnElaborate.addEventListener('click', async () => { + const prompt = document.getElementById('elaborate-prompt').value.trim(); + if (!prompt) { + const form = document.getElementById('task-form'); + // Remove previous error + const prev = form.querySelector('.form-error'); + if (prev) prev.remove(); + const errEl = document.createElement('p'); + errEl.className = 'form-error'; + errEl.textContent = 'Please enter a description before drafting.'; + form.querySelector('.elaborate-section').appendChild(errEl); + return; } - // Show success banner - const banner = document.createElement('p'); - banner.className = 'elaborate-banner'; - banner.textContent = 'AI draft ready — review and submit.'; - document.getElementById('task-form').querySelector('.elaborate-section').appendChild(banner); + btnElaborate.disabled = true; + btnElaborate.textContent = 'Drafting…'; + + // Remove any previous errors or banners + const form = document.getElementById('task-form'); + form.querySelectorAll('.form-error, .elaborate-banner').forEach(el => el.remove()); - // Auto-validate after elaboration try { - const result = await validateTask(buildValidatePayload()); - renderValidationResult(result); - } catch (_) { - // silent - elaboration already succeeded, validation is bonus + const repoUrl = document.getElementById('repository-url').value.trim(); + const result = await elaborateTask(prompt, repoUrl); + + // Populate form fields + const f = document.getElementById('task-form'); + if (result.name) + f.querySelector('[name="name"]').value = result.name; + if (result.agent && result.agent.instructions) + f.querySelector('[name="instructions"]').value = result.agent.instructions; + if (result.repository_url || result.agent?.repository_url) { + document.getElementById('repository-url').value = result.repository_url || result.agent.repository_url; + } + if (result.agent && result.agent.container_image) { + document.getElementById('container-image').value = result.agent.container_image; + } + if (result.agent && result.agent.max_budget_usd != null) + f.querySelector('[name="max_budget_usd"]').value = result.agent.max_budget_usd; + if (result.timeout) + f.querySelector('[name="timeout"]').value = result.timeout; + if (result.priority) { + const sel = f.querySelector('[name="priority"]'); + if ([...sel.options].some(o => o.value === result.priority)) { + sel.value = result.priority; + } + } + + // Show success banner + const banner = document.createElement('p'); + banner.className = 'elaborate-banner'; + banner.textContent = 'AI draft ready — review and submit.'; + document.getElementById('task-form').querySelector('.elaborate-section').appendChild(banner); + + // Auto-validate after elaboration + try { + const result = await validateTask(buildValidatePayload()); + renderValidationResult(result); + } catch (_) { + // silent - elaboration already succeeded, validation is bonus + } + } catch (err) { + const errEl = document.createElement('p'); + errEl.className = 'form-error'; + errEl.textContent = `Elaboration failed: ${err.message}`; + document.getElementById('task-form').querySelector('.elaborate-section').appendChild(errEl); + } finally { + btnElaborate.disabled = false; + btnElaborate.textContent = 'Draft with AI ✦'; } - } catch (err) { - const errEl = document.createElement('p'); - errEl.className = 'form-error'; - errEl.textContent = `Elaboration failed: ${err.message}`; - document.getElementById('task-form').querySelector('.elaborate-section').appendChild(errEl); - } finally { - btnElaborate.disabled = false; - btnElaborate.textContent = 'Draft with AI ✦'; - } - }); + }); - document.getElementById('task-form').addEventListener('submit', async e => { - e.preventDefault(); + document.getElementById('task-form').addEventListener('submit', async e => { + e.preventDefault(); - // Remove any previous error - const prev = e.target.querySelector('.form-error'); - if (prev) prev.remove(); + // Remove any previous error + const prev = e.target.querySelector('.form-error'); + if (prev) prev.remove(); - const btn = e.submitter; - btn.disabled = true; - btn.textContent = 'Creating…'; + const btn = e.submitter; + btn.disabled = true; + btn.textContent = 'Creating…'; - try { - const validateResult = document.getElementById('validate-result'); - if (!validateResult.hasAttribute('hidden') && validateResult.dataset.clarity && validateResult.dataset.clarity !== 'clear') { - if (!window.confirm('The validator flagged issues. Create task anyway?')) { - return; + try { + const validateResult = document.getElementById('validate-result'); + if (!validateResult.hasAttribute('hidden') && validateResult.dataset.clarity && validateResult.dataset.clarity !== 'clear') { + if (!window.confirm('The validator flagged issues. Create task anyway?')) { + return; + } } + await createTask(new FormData(e.target)); + } catch (err) { + const errEl = document.createElement('p'); + errEl.className = 'form-error'; + errEl.textContent = err.message; + e.target.appendChild(errEl); + } finally { + btn.disabled = false; + btn.textContent = 'Create & Queue'; } - await createTask(new FormData(e.target)); - } catch (err) { - const errEl = document.createElement('p'); - errEl.className = 'form-error'; - errEl.textContent = err.message; - e.target.appendChild(errEl); - } finally { - btn.disabled = false; - btn.textContent = 'Create & Queue'; - } + }); }); -}); +} -- cgit v1.2.3 From e1be377c851f1e7ce594fa3de6c429354bcedcce Mon Sep 17 00:00:00 2001 From: Peter Stone Date: Wed, 18 Mar 2026 07:24:31 +0000 Subject: 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 --- docs/adr/005-sandbox-execution-model.md | 2 +- docs/reviews/feat-container-execution.md | 113 +++++++++++++++++-------------- internal/executor/container.go | 47 ++++++------- internal/executor/container_test.go | 20 +++--- internal/executor/helpers.go | 11 ++- internal/executor/stream_test.go | 25 +++++-- 6 files changed, 125 insertions(+), 93 deletions(-) (limited to 'docs') diff --git a/docs/adr/005-sandbox-execution-model.md b/docs/adr/005-sandbox-execution-model.md index 80629d1..0c9ef14 100644 --- a/docs/adr/005-sandbox-execution-model.md +++ b/docs/adr/005-sandbox-execution-model.md @@ -1,7 +1,7 @@ # ADR-005: Git Sandbox Execution Model ## Status -Accepted +Superseded by [ADR-006](006-containerized-execution.md) ## Context 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 ` 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. diff --git a/internal/executor/container.go b/internal/executor/container.go index 32a1ea3..d21aea3 100644 --- a/internal/executor/container.go +++ b/internal/executor/container.go @@ -88,11 +88,7 @@ func (r *ContainerRunner) Run(ctx context.Context, t *task.Task, e *storage.Exec // 2. Clone repo into workspace if not resuming if !isResume { r.Logger.Info("cloning repository", "url", repoURL, "workspace", workspace) - // git clone requires the target to be empty or non-existent. - // Since we just created workspace as a temp dir, it's empty. - // But git clone wants to CREATE the dir if it's the target, or clone INTO it. if out, err := exec.CommandContext(ctx, "git", "clone", repoURL, workspace).CombinedOutput(); err != nil { - // If it's a local path and not a repo, we might need to init it (legacy support from ADR-005) r.Logger.Warn("git clone failed, attempting fallback init", "url", repoURL, "error", err) if initErr := r.fallbackGitInit(repoURL, workspace); initErr != nil { return fmt.Errorf("git clone and fallback init failed: %w\n%s", err, string(out)) @@ -126,7 +122,6 @@ func (r *ContainerRunner) Run(ctx context.Context, t *task.Task, e *storage.Exec defer stderrFile.Close() // 4. Run container - // TODO: Support Resume/BLOCKED by re-attaching to preserved workspace. // Write API keys to a temporary env file to avoid exposure in 'ps' or 'docker inspect' envFile := filepath.Join(workspace, ".claudomator-env") @@ -142,15 +137,7 @@ func (r *ContainerRunner) Run(ctx context.Context, t *task.Task, e *storage.Exec } args := r.buildDockerArgs(workspace, e.TaskID) - innerCmd := r.buildInnerCmd(t, e.ID, isResume) - - image = t.Agent.ContainerImage - if image == "" { - image = r.Image - } - if image == "" { - image = "claudomator-agent:latest" - } + innerCmd := r.buildInnerCmd(t, e, isResume) fullArgs := append(args, image) fullArgs = append(fullArgs, innerCmd...) @@ -177,12 +164,13 @@ func (r *ContainerRunner) Run(ctx context.Context, t *task.Task, e *storage.Exec // Stream stdout to the log file and parse cost/errors. var costUSD float64 + var sessionID string var streamErr error var wg sync.WaitGroup wg.Add(1) go func() { defer wg.Done() - costUSD, streamErr = parseStream(stdoutR, stdoutFile, r.Logger) + costUSD, sessionID, streamErr = parseStream(stdoutR, stdoutFile, r.Logger) stdoutR.Close() }() @@ -190,6 +178,9 @@ func (r *ContainerRunner) Run(ctx context.Context, t *task.Task, e *storage.Exec wg.Wait() e.CostUSD = costUSD + if sessionID != "" { + e.SessionID = sessionID + } // Check whether the agent left a question before exiting. questionFile := filepath.Join(logDir, "question.json") @@ -204,7 +195,7 @@ func (r *ContainerRunner) Run(ctx context.Context, t *task.Task, e *storage.Exec success = true // We consider BLOCKED as a "success" for workspace preservation return &BlockedError{ QuestionJSON: questionJSON, - SessionID: e.ID, // For container runner, we use exec ID as session ID + SessionID: e.SessionID, SandboxDir: workspace, } } @@ -219,12 +210,16 @@ func (r *ContainerRunner) Run(ctx context.Context, t *task.Task, e *storage.Exec // 5. Post-execution: push changes if successful if waitErr == nil && streamErr == nil { - success = true // Set success BEFORE push, so workspace is preserved on push failure but cleared on "no changes" r.Logger.Info("pushing changes back to remote", "url", repoURL) // We assume the sandbox has committed changes (the agent image should enforce this) if out, err := exec.CommandContext(ctx, "git", "-C", workspace, "push", "origin", "HEAD").CombinedOutput(); err != nil { r.Logger.Warn("git push failed or no changes", "error", err, "output", string(out)) + // Only set success = true if we consider this "good enough". + // Review says: "If the remote is missing or the push fails, the task is marked FAILED and the host-side workspace is preserved" + // So we MUST return error here. + return fmt.Errorf("git push failed: %w\n%s", err, string(out)) } + success = true } if waitErr != nil { @@ -251,22 +246,24 @@ func (r *ContainerRunner) buildDockerArgs(workspace, taskID string) []string { } } -func (r *ContainerRunner) buildInnerCmd(t *task.Task, execID string, isResume bool) []string { +func (r *ContainerRunner) buildInnerCmd(t *task.Task, e *storage.Execution, isResume bool) []string { // Claude CLI uses -p for prompt text. To pass a file, we use a shell to cat it. - promptCmd := "cat /workspace/.claudomator-instructions.txt" + // We use a shell variable to capture the expansion to avoid quoting issues with instructions contents. + // The outer single quotes around the sh -c argument prevent host-side expansion. if t.Agent.Type == "gemini" { - return []string{"sh", "-c", "gemini -p \"$(" + promptCmd + ")\""} + return []string{"sh", "-c", "INST=$(cat /workspace/.claudomator-instructions.txt); gemini -p \"$INST\""} } // Claude - claudeArgs := []string{"claude", "-p", "\"$(" + promptCmd + ")\""} - if isResume { - claudeArgs = append(claudeArgs, "--resume", execID) + var claudeCmd strings.Builder + claudeCmd.WriteString("INST=$(cat /workspace/.claudomator-instructions.txt); claude -p \"$INST\"") + if isResume && e.ResumeSessionID != "" { + claudeCmd.WriteString(fmt.Sprintf(" --resume %s", e.ResumeSessionID)) } - claudeArgs = append(claudeArgs, "--output-format", "stream-json", "--verbose", "--permission-mode", "bypassPermissions") + claudeCmd.WriteString(" --output-format stream-json --verbose --permission-mode bypassPermissions") - return []string{"sh", "-c", strings.Join(claudeArgs, " ")} + return []string{"sh", "-c", claudeCmd.String()} } func (r *ContainerRunner) fallbackGitInit(repoURL, workspace string) error { diff --git a/internal/executor/container_test.go b/internal/executor/container_test.go index fbb4d7d..0e36def 100644 --- a/internal/executor/container_test.go +++ b/internal/executor/container_test.go @@ -33,6 +33,7 @@ func TestContainerRunner_BuildDockerArgs(t *testing.T) { "-e", "CLAUDOMATOR_DROP_DIR=/data/drops", } + if len(args) != len(expected) { t.Fatalf("expected %d args, got %d", len(expected), len(args)) } @@ -48,34 +49,37 @@ func TestContainerRunner_BuildInnerCmd(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", false) + exec := &storage.Execution{} + cmd := runner.buildInnerCmd(tk, exec, false) cmdStr := strings.Join(cmd, " ") if strings.Contains(cmdStr, "--resume") { t.Errorf("unexpected --resume flag in fresh run: %q", cmdStr) } - if !strings.Contains(cmdStr, "cat /workspace/.claudomator-instructions.txt") { + if !strings.Contains(cmdStr, "INST=$(cat /workspace/.claudomator-instructions.txt); claude -p \"$INST\"") { t.Errorf("expected cat instructions in sh command, 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) + exec := &storage.Execution{ResumeSessionID: "orig-session-123"} + cmd := runner.buildInnerCmd(tk, exec, true) cmdStr := strings.Join(cmd, " ") - if !strings.Contains(cmdStr, "--resume exec-456") { - t.Errorf("expected --resume flag in resume run: %q", cmdStr) + if !strings.Contains(cmdStr, "--resume orig-session-123") { + t.Errorf("expected --resume flag with correct session ID, got %q", cmdStr) } }) t.Run("gemini", func(t *testing.T) { tk := &task.Task{Agent: task.AgentConfig{Type: "gemini"}} - cmd := runner.buildInnerCmd(tk, "exec-456", false) + exec := &storage.Execution{} + cmd := runner.buildInnerCmd(tk, exec, false) cmdStr := strings.Join(cmd, " ") - if !strings.Contains(cmdStr, "gemini") { - t.Errorf("expected gemini command, got %q", cmdStr) + if !strings.Contains(cmdStr, "gemini -p \"$INST\"") { + t.Errorf("expected gemini command with safer quoting, got %q", cmdStr) } }) } diff --git a/internal/executor/helpers.go b/internal/executor/helpers.go index 5ffde8e..36cd050 100644 --- a/internal/executor/helpers.go +++ b/internal/executor/helpers.go @@ -24,12 +24,13 @@ func (e *BlockedError) Error() string { return fmt.Sprintf("task blocked: %s", e // (costUSD, error). error is non-nil if the stream signals task failure: // - result message has is_error:true // - a tool_result was denied due to missing permissions -func parseStream(r io.Reader, w io.Writer, logger *slog.Logger) (float64, error) { +func parseStream(r io.Reader, w io.Writer, logger *slog.Logger) (float64, string, error) { tee := io.TeeReader(r, w) scanner := bufio.NewScanner(tee) scanner.Buffer(make([]byte, 1024*1024), 1024*1024) // 1MB buffer for large lines var totalCost float64 + var sessionID string var streamErr error for scanner.Scan() { @@ -41,6 +42,12 @@ func parseStream(r io.Reader, w io.Writer, logger *slog.Logger) (float64, error) msgType, _ := msg["type"].(string) switch msgType { + case "system": + if subtype, ok := msg["subtype"].(string); ok && subtype == "init" { + if sid, ok := msg["session_id"].(string); ok { + sessionID = sid + } + } case "rate_limit_event": if info, ok := msg["rate_limit_info"].(map[string]interface{}); ok { status, _ := info["status"].(string) @@ -81,7 +88,7 @@ func parseStream(r io.Reader, w io.Writer, logger *slog.Logger) (float64, error) } } - return totalCost, streamErr + return totalCost, sessionID, streamErr } // permissionDenialError inspects a "user" stream message for tool_result entries diff --git a/internal/executor/stream_test.go b/internal/executor/stream_test.go index 10eb858..11a6178 100644 --- a/internal/executor/stream_test.go +++ b/internal/executor/stream_test.go @@ -12,7 +12,7 @@ func streamLine(json string) string { return json + "\n" } func TestParseStream_ResultIsError_ReturnsError(t *testing.T) { input := streamLine(`{"type":"result","subtype":"error_during_execution","is_error":true,"result":"something went wrong"}`) - _, err := parseStream(strings.NewReader(input), io.Discard, slog.New(slog.NewTextHandler(io.Discard, nil))) + _, _, err := parseStream(strings.NewReader(input), io.Discard, slog.New(slog.NewTextHandler(io.Discard, nil))) if err == nil { t.Fatal("expected error when result.is_error=true, got nil") } @@ -27,7 +27,7 @@ func TestParseStream_PermissionDenied_ReturnsError(t *testing.T) { input := streamLine(`{"type":"user","message":{"role":"user","content":[{"type":"tool_result","is_error":true,"content":"Claude requested permissions to write to /foo/bar.go, but you haven't granted it yet.","tool_use_id":"tu_abc"}]}}`) + streamLine(`{"type":"result","subtype":"success","is_error":false,"result":"I need permission","total_cost_usd":0.1}`) - _, err := parseStream(strings.NewReader(input), io.Discard, slog.New(slog.NewTextHandler(io.Discard, nil))) + _, _, err := parseStream(strings.NewReader(input), io.Discard, slog.New(slog.NewTextHandler(io.Discard, nil))) if err == nil { t.Fatal("expected error for permission denial, got nil") } @@ -40,7 +40,7 @@ func TestParseStream_Success_ReturnsNilError(t *testing.T) { input := streamLine(`{"type":"assistant","message":{"content":[{"type":"text","text":"Done."}]}}`) + streamLine(`{"type":"result","subtype":"success","is_error":false,"result":"All tests pass.","total_cost_usd":0.05}`) - _, err := parseStream(strings.NewReader(input), io.Discard, slog.New(slog.NewTextHandler(io.Discard, nil))) + _, _, err := parseStream(strings.NewReader(input), io.Discard, slog.New(slog.NewTextHandler(io.Discard, nil))) if err != nil { t.Fatalf("expected nil error for success stream, got: %v", err) } @@ -49,7 +49,7 @@ func TestParseStream_Success_ReturnsNilError(t *testing.T) { func TestParseStream_ExtractsCostFromResultMessage(t *testing.T) { input := streamLine(`{"type":"result","subtype":"success","is_error":false,"result":"done","total_cost_usd":1.2345}`) - cost, err := parseStream(strings.NewReader(input), io.Discard, slog.New(slog.NewTextHandler(io.Discard, nil))) + cost, _, err := parseStream(strings.NewReader(input), io.Discard, slog.New(slog.NewTextHandler(io.Discard, nil))) if err != nil { t.Fatalf("unexpected error: %v", err) } @@ -62,7 +62,7 @@ func TestParseStream_ExtractsCostFromLegacyCostUSD(t *testing.T) { // Some versions emit cost_usd at the top level rather than total_cost_usd. input := streamLine(`{"type":"result","subtype":"success","is_error":false,"result":"done","cost_usd":0.99}`) - cost, err := parseStream(strings.NewReader(input), io.Discard, slog.New(slog.NewTextHandler(io.Discard, nil))) + cost, _, err := parseStream(strings.NewReader(input), io.Discard, slog.New(slog.NewTextHandler(io.Discard, nil))) if err != nil { t.Fatalf("unexpected error: %v", err) } @@ -78,8 +78,21 @@ func TestParseStream_NonToolResultIsError_DoesNotFail(t *testing.T) { input := streamLine(`{"type":"user","message":{"role":"user","content":[{"type":"tool_result","is_error":true,"content":"exit status 1","tool_use_id":"tu_xyz"}]}}`) + streamLine(`{"type":"result","subtype":"success","is_error":false,"result":"Fixed it.","total_cost_usd":0.2}`) - _, err := parseStream(strings.NewReader(input), io.Discard, slog.New(slog.NewTextHandler(io.Discard, nil))) + _, _, err := parseStream(strings.NewReader(input), io.Discard, slog.New(slog.NewTextHandler(io.Discard, nil))) if err != nil { t.Fatalf("non-permission tool errors should not fail the task, got: %v", err) } } + +func TestParseStream_ExtractsSessionID(t *testing.T) { + input := streamLine(`{"type":"system","subtype":"init","session_id":"sess-999"}`) + + streamLine(`{"type":"result","subtype":"success","is_error":false,"result":"ok","total_cost_usd":0.01}`) + + _, sid, err := parseStream(strings.NewReader(input), io.Discard, slog.New(slog.NewTextHandler(io.Discard, nil))) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if sid != "sess-999" { + t.Errorf("want session ID sess-999, got %q", sid) + } +} -- cgit v1.2.3 From a4795d68fc5381f1ff48d043fe7554355e5899fb Mon Sep 17 00:00:00 2001 From: Peter Stone Date: Wed, 18 Mar 2026 07:54:27 +0000 Subject: fix: address final container execution issues and cleanup review docs --- docs/reviews/feat-container-execution.md | 130 ------------------------------- images/agent-base/Dockerfile | 22 ++++-- internal/api/webhook.go | 16 ++-- internal/cli/run.go | 32 +++++--- internal/cli/serve.go | 39 ++++++---- internal/config/config.go | 2 + internal/executor/container.go | 98 +++++++++++++++++------ internal/executor/container_test.go | 52 ++++++++++--- internal/executor/helpers.go | 4 +- 9 files changed, 193 insertions(+), 202 deletions(-) delete mode 100644 docs/reviews/feat-container-execution.md (limited to 'docs') diff --git a/docs/reviews/feat-container-execution.md b/docs/reviews/feat-container-execution.md deleted file mode 100644 index cdcc174..0000000 --- a/docs/reviews/feat-container-execution.md +++ /dev/null @@ -1,130 +0,0 @@ -# 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. diff --git a/images/agent-base/Dockerfile b/images/agent-base/Dockerfile index 71807ae..6fb253c 100644 --- a/images/agent-base/Dockerfile +++ b/images/agent-base/Dockerfile @@ -1,5 +1,5 @@ # Claudomator Agent Base Image -FROM ubuntu:22.04 +FROM ubuntu:24.04 # Avoid interactive prompts ENV DEBIAN_FRONTEND=noninteractive @@ -9,7 +9,7 @@ RUN apt-get update && apt-get install -y \ git \ curl \ make \ - golang \ + wget \ nodejs \ npm \ sqlite3 \ @@ -17,20 +17,28 @@ RUN apt-get update && apt-get install -y \ sudo \ && rm -rf /var/lib/apt/lists/* -# Install specific node tools if needed (example: postcss) +# Install Go 1.22+ +RUN wget https://go.dev/dl/go1.22.1.linux-amd64.tar.gz && \ + tar -C /usr/local -xzf go1.22.1.linux-amd64.tar.gz && \ + rm go1.22.1.linux-amd64.tar.gz +ENV PATH=$PATH:/usr/local/go/bin + +# Install Claude CLI +RUN npm install -g @anthropic-ai/claude-code + +# Install specific node tools RUN npm install -g postcss-cli tailwindcss autoprefixer # Setup workspace WORKDIR /workspace -# Install Claudomator-aware CLI wrappers (placeholder) -# These will be provided by the Claudomator project in the future. -# For now, we assume 'claude' and 'gemini' binaries are available or mapped. - # Add a user claudomator-agent RUN useradd -m claudomator-agent && \ echo "claudomator-agent ALL=(ALL) NOPASSWD:ALL" >> /etc/sudoers +# Ensure /usr/local/bin is writable for npm or use a different path +# @anthropic-ai/claude-code might need some extra setup or just work + USER claudomator-agent # Default command diff --git a/internal/api/webhook.go b/internal/api/webhook.go index a28b43f..141224f 100644 --- a/internal/api/webhook.go +++ b/internal/api/webhook.go @@ -210,16 +210,16 @@ func (s *Server) createCIFailureTask(w http.ResponseWriter, repoName, fullName, MaxBudgetUSD: 3.0, AllowedTools: []string{"Read", "Edit", "Bash", "Glob", "Grep"}, }, - Priority: task.PriorityNormal, - Tags: []string{"ci", "auto"}, - DependsOn: []string{}, - Retry: task.RetryConfig{MaxAttempts: 1, Backoff: "exponential"}, - State: task.StatePending, - CreatedAt: now, - UpdatedAt: now, + Priority: task.PriorityNormal, + Tags: []string{"ci", "auto"}, + DependsOn: []string{}, + Retry: task.RetryConfig{MaxAttempts: 1, Backoff: "exponential"}, + State: task.StatePending, + CreatedAt: now, + UpdatedAt: now, + RepositoryURL: fmt.Sprintf("https://github.com/%s.git", fullName), } if project != nil { - t.RepositoryURL = fmt.Sprintf("https://github.com/%s.git", fullName) t.Project = project.Name } diff --git a/internal/cli/run.go b/internal/cli/run.go index 9663bc5..cfac893 100644 --- a/internal/cli/run.go +++ b/internal/cli/run.go @@ -72,22 +72,34 @@ func runTasks(file string, parallel int, dryRun bool) error { logger := newLogger(verbose) + apiURL := "http://localhost" + cfg.ServerAddr + if len(cfg.ServerAddr) > 0 && cfg.ServerAddr[0] != ':' { + apiURL = "http://" + cfg.ServerAddr + } + runners := map[string]executor.Runner{ "claude": &executor.ContainerRunner{ - Image: cfg.ClaudeImage, - Logger: logger, - LogDir: cfg.LogDir, - APIURL: "http://" + cfg.ServerAddr, - DropsDir: cfg.DropsDir, + Image: cfg.ClaudeImage, + Logger: logger, + LogDir: cfg.LogDir, + APIURL: apiURL, + DropsDir: cfg.DropsDir, + SSHAuthSock: cfg.SSHAuthSock, + ClaudeBinary: cfg.ClaudeBinaryPath, + GeminiBinary: cfg.GeminiBinaryPath, }, "gemini": &executor.ContainerRunner{ - Image: cfg.GeminiImage, - Logger: logger, - LogDir: cfg.LogDir, - APIURL: "http://" + cfg.ServerAddr, - DropsDir: cfg.DropsDir, + Image: cfg.GeminiImage, + Logger: logger, + LogDir: cfg.LogDir, + APIURL: apiURL, + DropsDir: cfg.DropsDir, + SSHAuthSock: cfg.SSHAuthSock, + ClaudeBinary: cfg.ClaudeBinaryPath, + GeminiBinary: cfg.GeminiBinaryPath, }, } + pool := executor.NewPool(parallel, runners, store, logger) if cfg.GeminiBinaryPath != "" { pool.Classifier = &executor.Classifier{GeminiBinaryPath: cfg.GeminiBinaryPath} diff --git a/internal/cli/serve.go b/internal/cli/serve.go index 33715ee..2ee020d 100644 --- a/internal/cli/serve.go +++ b/internal/cli/serve.go @@ -77,25 +77,34 @@ func serve(addr string) error { runners := map[string]executor.Runner{ "claude": &executor.ContainerRunner{ - Image: cfg.ClaudeImage, - Logger: logger, - LogDir: cfg.LogDir, - APIURL: apiURL, - DropsDir: cfg.DropsDir, + Image: cfg.ClaudeImage, + Logger: logger, + LogDir: cfg.LogDir, + APIURL: apiURL, + DropsDir: cfg.DropsDir, + SSHAuthSock: cfg.SSHAuthSock, + ClaudeBinary: cfg.ClaudeBinaryPath, + GeminiBinary: cfg.GeminiBinaryPath, }, "gemini": &executor.ContainerRunner{ - Image: cfg.GeminiImage, - Logger: logger, - LogDir: cfg.LogDir, - APIURL: apiURL, - DropsDir: cfg.DropsDir, + Image: cfg.GeminiImage, + Logger: logger, + LogDir: cfg.LogDir, + APIURL: apiURL, + DropsDir: cfg.DropsDir, + SSHAuthSock: cfg.SSHAuthSock, + ClaudeBinary: cfg.ClaudeBinaryPath, + GeminiBinary: cfg.GeminiBinaryPath, }, "container": &executor.ContainerRunner{ - Image: "claudomator-agent:latest", - Logger: logger, - LogDir: cfg.LogDir, - APIURL: apiURL, - DropsDir: cfg.DropsDir, + Image: "claudomator-agent:latest", + Logger: logger, + LogDir: cfg.LogDir, + APIURL: apiURL, + DropsDir: cfg.DropsDir, + SSHAuthSock: cfg.SSHAuthSock, + ClaudeBinary: cfg.ClaudeBinaryPath, + GeminiBinary: cfg.GeminiBinaryPath, }, } diff --git a/internal/config/config.go b/internal/config/config.go index 6e163c4..fa76b1b 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -20,6 +20,7 @@ type Config struct { DBPath string `toml:"-"` LogDir string `toml:"-"` DropsDir string `toml:"-"` + SSHAuthSock string `toml:"ssh_auth_sock"` ClaudeBinaryPath string `toml:"claude_binary_path"` GeminiBinaryPath string `toml:"gemini_binary_path"` ClaudeImage string `toml:"claude_image"` @@ -50,6 +51,7 @@ func Default() (*Config, error) { DBPath: filepath.Join(dataDir, "claudomator.db"), LogDir: filepath.Join(dataDir, "executions"), DropsDir: filepath.Join(dataDir, "drops"), + SSHAuthSock: os.Getenv("SSH_AUTH_SOCK"), ClaudeBinaryPath: "claude", GeminiBinaryPath: "gemini", ClaudeImage: "claudomator-agent:latest", diff --git a/internal/executor/container.go b/internal/executor/container.go index d21aea3..45758d2 100644 --- a/internal/executor/container.go +++ b/internal/executor/container.go @@ -17,12 +17,23 @@ import ( // ContainerRunner executes an agent inside a container. type ContainerRunner struct { - Image string // default image if not specified in task - Logger *slog.Logger - LogDir string - APIURL string - DropsDir string - SSHAuthSock string // optional path to host SSH agent + Image string // default image if not specified in task + Logger *slog.Logger + LogDir string + APIURL string + DropsDir string + SSHAuthSock string // optional path to host SSH agent + ClaudeBinary string // optional path to claude binary in container + GeminiBinary string // optional path to gemini binary in container + // Command allows mocking exec.CommandContext for tests. + Command func(ctx context.Context, name string, arg ...string) *exec.Cmd +} + +func (r *ContainerRunner) command(ctx context.Context, name string, arg ...string) *exec.Cmd { + if r.Command != nil { + return r.Command(ctx, name, arg...) + } + return exec.CommandContext(ctx, name, arg...) } func (r *ContainerRunner) ExecLogDir(execID string) string { @@ -88,7 +99,11 @@ func (r *ContainerRunner) Run(ctx context.Context, t *task.Task, e *storage.Exec // 2. Clone repo into workspace if not resuming if !isResume { r.Logger.Info("cloning repository", "url", repoURL, "workspace", workspace) - if out, err := exec.CommandContext(ctx, "git", "clone", repoURL, workspace).CombinedOutput(); err != nil { + if out, err := r.command(ctx, "git", "clone", repoURL, workspace).CombinedOutput(); err != nil { + // If it looks like a remote URL, fail fast. + if strings.HasPrefix(repoURL, "http") || strings.HasPrefix(repoURL, "git@") || strings.HasPrefix(repoURL, "ssh://") { + return fmt.Errorf("git clone failed for remote repository: %w\n%s", err, string(out)) + } r.Logger.Warn("git clone failed, attempting fallback init", "url", repoURL, "error", err) if initErr := r.fallbackGitInit(repoURL, workspace); initErr != nil { return fmt.Errorf("git clone and fallback init failed: %w\n%s", err, string(out)) @@ -143,7 +158,7 @@ func (r *ContainerRunner) Run(ctx context.Context, t *task.Task, e *storage.Exec fullArgs = append(fullArgs, innerCmd...) r.Logger.Info("starting container", "image", image, "taskID", t.ID) - cmd := exec.CommandContext(ctx, "docker", fullArgs...) + cmd := r.command(ctx, "docker", fullArgs...) cmd.Stderr = stderrFile cmd.SysProcAttr = &syscall.SysProcAttr{Setpgid: true} @@ -162,6 +177,18 @@ func (r *ContainerRunner) Run(ctx context.Context, t *task.Task, e *storage.Exec } stdoutW.Close() + // Watch for context cancellation to kill the process group (Issue 1) + done := make(chan struct{}) + defer close(done) + go func() { + select { + case <-ctx.Done(): + r.Logger.Info("killing container process group due to context cancellation", "taskID", t.ID) + syscall.Kill(-cmd.Process.Pid, syscall.SIGKILL) + case <-done: + } + }() + // Stream stdout to the log file and parse cost/errors. var costUSD float64 var sessionID string @@ -193,6 +220,9 @@ func (r *ContainerRunner) Run(ctx context.Context, t *task.Task, e *storage.Exec } else { isBlocked = true success = true // We consider BLOCKED as a "success" for workspace preservation + if e.SessionID == "" { + r.Logger.Warn("missing session ID; resume will start fresh", "taskID", e.TaskID) + } return &BlockedError{ QuestionJSON: questionJSON, SessionID: e.SessionID, @@ -210,14 +240,24 @@ func (r *ContainerRunner) Run(ctx context.Context, t *task.Task, e *storage.Exec // 5. Post-execution: push changes if successful if waitErr == nil && streamErr == nil { - r.Logger.Info("pushing changes back to remote", "url", repoURL) - // We assume the sandbox has committed changes (the agent image should enforce this) - if out, err := exec.CommandContext(ctx, "git", "-C", workspace, "push", "origin", "HEAD").CombinedOutput(); err != nil { - r.Logger.Warn("git push failed or no changes", "error", err, "output", string(out)) - // Only set success = true if we consider this "good enough". - // Review says: "If the remote is missing or the push fails, the task is marked FAILED and the host-side workspace is preserved" - // So we MUST return error here. - return fmt.Errorf("git push failed: %w\n%s", err, string(out)) + // Check if there are any commits to push (Issue 10) + // We use rev-list to see if HEAD is ahead of origin/HEAD. + // If origin/HEAD doesn't exist (e.g. fresh init), we just attempt to push. + hasCommits := true + if out, err := r.command(ctx, "git", "-C", workspace, "rev-list", "origin/HEAD..HEAD").CombinedOutput(); err == nil { + if len(strings.TrimSpace(string(out))) == 0 { + hasCommits = false + } + } + + if hasCommits { + r.Logger.Info("pushing changes back to remote", "url", repoURL) + if out, err := r.command(ctx, "git", "-C", workspace, "push", "origin", "HEAD").CombinedOutput(); err != nil { + r.Logger.Warn("git push failed", "error", err, "output", string(out)) + return fmt.Errorf("git push failed: %w\n%s", err, string(out)) + } + } else { + r.Logger.Info("no new commits to push", "taskID", t.ID) } success = true } @@ -235,7 +275,7 @@ func (r *ContainerRunner) Run(ctx context.Context, t *task.Task, e *storage.Exec func (r *ContainerRunner) buildDockerArgs(workspace, taskID string) []string { // --env-file takes a HOST path. hostEnvFile := filepath.Join(workspace, ".claudomator-env") - return []string{ + args := []string{ "run", "--rm", "-v", workspace + ":/workspace", "-w", "/workspace", @@ -244,28 +284,42 @@ func (r *ContainerRunner) buildDockerArgs(workspace, taskID string) []string { "-e", "CLAUDOMATOR_TASK_ID=" + taskID, "-e", "CLAUDOMATOR_DROP_DIR=" + r.DropsDir, } + if r.SSHAuthSock != "" { + args = append(args, "-v", r.SSHAuthSock+":/tmp/ssh-auth.sock", "-e", "SSH_AUTH_SOCK=/tmp/ssh-auth.sock") + } + return args } func (r *ContainerRunner) buildInnerCmd(t *task.Task, e *storage.Execution, isResume bool) []string { // Claude CLI uses -p for prompt text. To pass a file, we use a shell to cat it. // We use a shell variable to capture the expansion to avoid quoting issues with instructions contents. // The outer single quotes around the sh -c argument prevent host-side expansion. - + + claudeBin := r.ClaudeBinary + if claudeBin == "" { + claudeBin = "claude" + } + geminiBin := r.GeminiBinary + if geminiBin == "" { + geminiBin = "gemini" + } + if t.Agent.Type == "gemini" { - return []string{"sh", "-c", "INST=$(cat /workspace/.claudomator-instructions.txt); gemini -p \"$INST\""} + return []string{"sh", "-c", fmt.Sprintf("INST=$(cat /workspace/.claudomator-instructions.txt); %s -p \"$INST\"", geminiBin)} } // Claude var claudeCmd strings.Builder - claudeCmd.WriteString("INST=$(cat /workspace/.claudomator-instructions.txt); claude -p \"$INST\"") + claudeCmd.WriteString(fmt.Sprintf("INST=$(cat /workspace/.claudomator-instructions.txt); %s -p \"$INST\"", claudeBin)) if isResume && e.ResumeSessionID != "" { claudeCmd.WriteString(fmt.Sprintf(" --resume %s", e.ResumeSessionID)) } claudeCmd.WriteString(" --output-format stream-json --verbose --permission-mode bypassPermissions") - + return []string{"sh", "-c", claudeCmd.String()} } + func (r *ContainerRunner) fallbackGitInit(repoURL, workspace string) error { // Ensure directory exists if err := os.MkdirAll(workspace, 0755); err != nil { @@ -281,7 +335,7 @@ func (r *ContainerRunner) fallbackGitInit(repoURL, workspace string) error { // git clone handle local paths fine if they are repos. // This fallback is only if it's NOT a repo. for _, args := range cmds { - if out, err := exec.Command("git", args...).CombinedOutput(); err != nil { + if out, err := r.command(context.Background(), "git", args...).CombinedOutput(); err != nil { return fmt.Errorf("git init failed: %w\n%s", err, out) } } diff --git a/internal/executor/container_test.go b/internal/executor/container_test.go index 0e36def..d4d591e 100644 --- a/internal/executor/container_test.go +++ b/internal/executor/container_test.go @@ -6,6 +6,7 @@ import ( "io" "log/slog" "os" + "os/exec" "strings" "testing" @@ -15,14 +16,15 @@ import ( func TestContainerRunner_BuildDockerArgs(t *testing.T) { runner := &ContainerRunner{ - APIURL: "http://localhost:8484", - DropsDir: "/data/drops", + APIURL: "http://localhost:8484", + DropsDir: "/data/drops", + SSHAuthSock: "/tmp/ssh.sock", } workspace := "/tmp/ws" taskID := "task-123" args := runner.buildDockerArgs(workspace, taskID) - + expected := []string{ "run", "--rm", "-v", "/tmp/ws:/workspace", @@ -31,11 +33,12 @@ func TestContainerRunner_BuildDockerArgs(t *testing.T) { "-e", "CLAUDOMATOR_API_URL=http://localhost:8484", "-e", "CLAUDOMATOR_TASK_ID=task-123", "-e", "CLAUDOMATOR_DROP_DIR=/data/drops", + "-v", "/tmp/ssh.sock:/tmp/ssh-auth.sock", + "-e", "SSH_AUTH_SOCK=/tmp/ssh-auth.sock", } - if len(args) != len(expected) { - t.Fatalf("expected %d args, got %d", len(expected), len(args)) + t.Fatalf("expected %d args, got %d. Got: %v", len(expected), len(args), args) } for i, v := range args { if v != expected[i] { @@ -76,12 +79,31 @@ func TestContainerRunner_BuildInnerCmd(t *testing.T) { tk := &task.Task{Agent: task.AgentConfig{Type: "gemini"}} exec := &storage.Execution{} cmd := runner.buildInnerCmd(tk, exec, false) - + cmdStr := strings.Join(cmd, " ") if !strings.Contains(cmdStr, "gemini -p \"$INST\"") { t.Errorf("expected gemini command with safer quoting, got %q", cmdStr) } }) + + t.Run("custom-binaries", func(t *testing.T) { + runnerCustom := &ContainerRunner{ + ClaudeBinary: "/usr/bin/claude-v2", + GeminiBinary: "/usr/local/bin/gemini-pro", + } + + tkClaude := &task.Task{Agent: task.AgentConfig{Type: "claude"}} + cmdClaude := runnerCustom.buildInnerCmd(tkClaude, &storage.Execution{}, false) + if !strings.Contains(strings.Join(cmdClaude, " "), "/usr/bin/claude-v2 -p") { + t.Errorf("expected custom claude binary, got %q", cmdClaude) + } + + tkGemini := &task.Task{Agent: task.AgentConfig{Type: "gemini"}} + cmdGemini := runnerCustom.buildInnerCmd(tkGemini, &storage.Execution{}, false) + if !strings.Contains(strings.Join(cmdGemini, " "), "/usr/local/bin/gemini-pro -p") { + t.Errorf("expected custom gemini binary, got %q", cmdGemini) + } + }) } func TestContainerRunner_Run_PreservesWorkspaceOnFailure(t *testing.T) { @@ -89,19 +111,31 @@ func TestContainerRunner_Run_PreservesWorkspaceOnFailure(t *testing.T) { runner := &ContainerRunner{ Logger: logger, Image: "busybox", + Command: func(ctx context.Context, name string, arg ...string) *exec.Cmd { + // Mock docker run to exit 1 + if name == "docker" { + return exec.Command("sh", "-c", "exit 1") + } + // Mock git clone to succeed and create the directory + if name == "git" && len(arg) > 0 && arg[0] == "clone" { + dir := arg[len(arg)-1] + os.MkdirAll(dir, 0755) + return exec.Command("true") + } + return exec.Command("true") + }, } - // Use an invalid repo URL to trigger failure. tk := &task.Task{ ID: "test-task", - RepositoryURL: "/nonexistent/repo", + RepositoryURL: "https://github.com/example/repo.git", 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") + t.Fatal("expected error due to mocked docker failure") } // Verify SandboxDir was set and directory exists. diff --git a/internal/executor/helpers.go b/internal/executor/helpers.go index 36cd050..9e4530b 100644 --- a/internal/executor/helpers.go +++ b/internal/executor/helpers.go @@ -33,6 +33,7 @@ func parseStream(r io.Reader, w io.Writer, logger *slog.Logger) (float64, string var sessionID string var streamErr error +Loop: for scanner.Scan() { line := scanner.Bytes() var msg map[string]interface{} @@ -54,7 +55,7 @@ func parseStream(r io.Reader, w io.Writer, logger *slog.Logger) (float64, string if status == "rejected" { streamErr = fmt.Errorf("claude rate limit reached (rejected): %v", msg) // Immediately break since we can't continue anyway - break + break Loop } } case "assistant": @@ -91,6 +92,7 @@ func parseStream(r io.Reader, w io.Writer, logger *slog.Logger) (float64, string return totalCost, sessionID, streamErr } + // permissionDenialError inspects a "user" stream message for tool_result entries // that were denied due to missing permissions. Returns an error if found. func permissionDenialError(msg map[string]interface{}) error { -- cgit v1.2.3