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 --- internal/executor/container.go | 47 ++++++++++++++++++++---------------------- 1 file changed, 22 insertions(+), 25 deletions(-) (limited to 'internal/executor/container.go') 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 { -- cgit v1.2.3