From 9955a2f10c034dac60bc17cde6b80b432e21d9d3 Mon Sep 17 00:00:00 2001 From: Claudomator Date: Sun, 8 Mar 2026 07:47:17 +0000 Subject: security(cli): validate --parallel flag is positive in run command Co-Authored-By: Claude Sonnet 4.6 --- internal/cli/run.go | 4 ++++ internal/cli/run_test.go | 18 ++++++++++++++++++ 2 files changed, 22 insertions(+) create mode 100644 internal/cli/run_test.go (limited to 'internal/cli') diff --git a/internal/cli/run.go b/internal/cli/run.go index c666406..ed831f5 100644 --- a/internal/cli/run.go +++ b/internal/cli/run.go @@ -36,6 +36,10 @@ func newRunCmd() *cobra.Command { } func runTasks(file string, parallel int, dryRun bool) error { + if parallel < 1 { + return fmt.Errorf("--parallel must be at least 1, got %d", parallel) + } + tasks, err := task.ParseFile(file) if err != nil { return fmt.Errorf("parsing: %w", err) diff --git a/internal/cli/run_test.go b/internal/cli/run_test.go new file mode 100644 index 0000000..705fe29 --- /dev/null +++ b/internal/cli/run_test.go @@ -0,0 +1,18 @@ +package cli + +import ( + "strings" + "testing" +) + +func TestRunTasks_InvalidParallel(t *testing.T) { + for _, parallel := range []int{0, -1, -100} { + err := runTasks("ignored.yaml", parallel, false) + if err == nil { + t.Fatalf("parallel=%d: expected error, got nil", parallel) + } + if !strings.Contains(err.Error(), "--parallel") { + t.Errorf("parallel=%d: error should mention --parallel flag, got: %v", parallel, err) + } + } +} -- cgit v1.2.3 From 1f36e2312d316969db65a601ac7d9793fbc3bc4c Mon Sep 17 00:00:00 2001 From: Peter Stone Date: Sun, 8 Mar 2026 20:16:00 +0000 Subject: feat: rename working_dir→project_dir; git sandbox execution MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - ClaudeConfig.WorkingDir → ProjectDir (json: project_dir) - UnmarshalJSON fallback reads legacy working_dir from DB records - New executions with project_dir clone into a temp sandbox via git clone --local - Non-git project_dirs get git init + initial commit before clone - After success: verify clean working tree, merge --ff-only back to project_dir, remove sandbox - On failure/BLOCKED: sandbox preserved, path included in error message - Resume executions run directly in project_dir (no re-clone) Co-Authored-By: Claude Sonnet 4.6 --- internal/api/elaborate.go | 12 ++--- internal/api/elaborate_test.go | 2 +- internal/api/validate.go | 6 +-- internal/cli/create.go | 14 +++-- internal/executor/claude.go | 114 +++++++++++++++++++++++++++++++++++++-- internal/executor/claude_test.go | 6 +-- internal/task/task.go | 26 ++++++++- internal/task/validator_test.go | 2 +- web/app.js | 38 ++++++------- web/index.html | 5 +- 10 files changed, 181 insertions(+), 44 deletions(-) (limited to 'internal/cli') diff --git a/internal/api/elaborate.go b/internal/api/elaborate.go index 00f3297..e480e00 100644 --- a/internal/api/elaborate.go +++ b/internal/api/elaborate.go @@ -14,9 +14,9 @@ import ( const elaborateTimeout = 30 * time.Second func buildElaboratePrompt(workDir string) string { - workDirLine := ` "working_dir": string — leave empty unless you have a specific reason to set it,` + workDirLine := ` "project_dir": string — leave empty unless you have a specific reason to set it,` if workDir != "" { - workDirLine = fmt.Sprintf(` "working_dir": string — use %q for tasks that operate on this codebase, empty string otherwise,`, workDir) + workDirLine = fmt.Sprintf(` "project_dir": string — use %q for tasks that operate on this codebase, empty string otherwise,`, workDir) } return `You are a task configuration assistant for Claudomator, an AI task runner that executes tasks by running Claude as a subprocess. @@ -53,7 +53,7 @@ type elaboratedTask struct { type elaboratedClaude struct { Model string `json:"model"` Instructions string `json:"instructions"` - WorkingDir string `json:"working_dir"` + ProjectDir string `json:"project_dir"` MaxBudgetUSD float64 `json:"max_budget_usd"` AllowedTools []string `json:"allowed_tools"` } @@ -87,7 +87,7 @@ func (s *Server) claudeBinaryPath() string { func (s *Server) handleElaborateTask(w http.ResponseWriter, r *http.Request) { var input struct { Prompt string `json:"prompt"` - WorkingDir string `json:"working_dir"` + ProjectDir string `json:"project_dir"` } if err := json.NewDecoder(r.Body).Decode(&input); err != nil { writeJSON(w, http.StatusBadRequest, map[string]string{"error": "invalid JSON: " + err.Error()}) @@ -99,8 +99,8 @@ func (s *Server) handleElaborateTask(w http.ResponseWriter, r *http.Request) { } workDir := s.workDir - if input.WorkingDir != "" { - workDir = input.WorkingDir + if input.ProjectDir != "" { + workDir = input.ProjectDir } ctx, cancel := context.WithTimeout(r.Context(), elaborateTimeout) diff --git a/internal/api/elaborate_test.go b/internal/api/elaborate_test.go index 52f7fdf..09f7fbe 100644 --- a/internal/api/elaborate_test.go +++ b/internal/api/elaborate_test.go @@ -56,7 +56,7 @@ func TestElaborateTask_Success(t *testing.T) { Claude: elaboratedClaude{ Model: "sonnet", Instructions: "Run go test -race ./... and report results.", - WorkingDir: "", + ProjectDir: "", MaxBudgetUSD: 0.5, AllowedTools: []string{"Bash"}, }, diff --git a/internal/api/validate.go b/internal/api/validate.go index d8ebde9..4b691a9 100644 --- a/internal/api/validate.go +++ b/internal/api/validate.go @@ -56,7 +56,7 @@ func (s *Server) handleValidateTask(w http.ResponseWriter, r *http.Request) { Name string `json:"name"` Claude struct { Instructions string `json:"instructions"` - WorkingDir string `json:"working_dir"` + ProjectDir string `json:"project_dir"` AllowedTools []string `json:"allowed_tools"` } `json:"claude"` } @@ -74,8 +74,8 @@ func (s *Server) handleValidateTask(w http.ResponseWriter, r *http.Request) { } userMsg := fmt.Sprintf("Task name: %s\n\nInstructions:\n%s", input.Name, input.Claude.Instructions) - if input.Claude.WorkingDir != "" { - userMsg += fmt.Sprintf("\n\nWorking directory: %s", input.Claude.WorkingDir) + if input.Claude.ProjectDir != "" { + userMsg += fmt.Sprintf("\n\nWorking directory: %s", input.Claude.ProjectDir) } if len(input.Claude.AllowedTools) > 0 { userMsg += fmt.Sprintf("\n\nAllowed tools: %v", input.Claude.AllowedTools) diff --git a/internal/cli/create.go b/internal/cli/create.go index fdad932..addd034 100644 --- a/internal/cli/create.go +++ b/internal/cli/create.go @@ -4,7 +4,7 @@ import ( "bytes" "encoding/json" "fmt" - "net/http" + "io" "github.com/spf13/cobra" ) @@ -52,7 +52,7 @@ func createTask(serverURL, name, instructions, workingDir, model, parentID strin "priority": priority, "claude": map[string]interface{}{ "instructions": instructions, - "working_dir": workingDir, + "project_dir": workingDir, "model": model, "max_budget_usd": budget, }, @@ -62,20 +62,26 @@ func createTask(serverURL, name, instructions, workingDir, model, parentID strin } data, _ := json.Marshal(body) - resp, err := http.Post(serverURL+"/api/tasks", "application/json", bytes.NewReader(data)) //nolint:noctx + resp, err := httpClient.Post(serverURL+"/api/tasks", "application/json", bytes.NewReader(data)) //nolint:noctx if err != nil { return fmt.Errorf("POST /api/tasks: %w", err) } defer resp.Body.Close() + raw, _ := io.ReadAll(resp.Body) var result map[string]interface{} - _ = json.NewDecoder(resp.Body).Decode(&result) + if err := json.Unmarshal(raw, &result); err != nil { + return fmt.Errorf("server returned invalid JSON (status %d): %s", resp.StatusCode, string(raw)) + } if resp.StatusCode >= 300 { return fmt.Errorf("server returned %d: %v", resp.StatusCode, result["error"]) } id, _ := result["id"].(string) + if id == "" { + return fmt.Errorf("server returned task without id field") + } fmt.Printf("Created task %s\n", id) if autoStart { diff --git a/internal/executor/claude.go b/internal/executor/claude.go index c04a747..aa715da 100644 --- a/internal/executor/claude.go +++ b/internal/executor/claude.go @@ -55,10 +55,18 @@ func (r *ClaudeRunner) binaryPath() string { // Run executes a claude -p invocation, streaming output to log files. // It retries up to 3 times on rate-limit errors using exponential backoff. // If the agent writes a question file and exits, Run returns *BlockedError. +// +// When project_dir is set and this is not a resume execution, Run clones the +// project into a temp sandbox, runs the agent there, then merges committed +// changes back to project_dir. On failure the sandbox is preserved and its +// path is included in the error. func (r *ClaudeRunner) Run(ctx context.Context, t *task.Task, e *storage.Execution) error { - if t.Claude.WorkingDir != "" { - if _, err := os.Stat(t.Claude.WorkingDir); err != nil { - return fmt.Errorf("working_dir %q: %w", t.Claude.WorkingDir, err) + projectDir := t.Claude.ProjectDir + + // Validate project_dir exists when set. + if projectDir != "" { + if _, err := os.Stat(projectDir); err != nil { + return fmt.Errorf("project_dir %q: %w", projectDir, err) } } @@ -82,6 +90,20 @@ func (r *ClaudeRunner) Run(ctx context.Context, t *task.Task, e *storage.Executi e.SessionID = e.ID // reuse execution UUID as session UUID (both are UUIDs) } + // For new (non-resume) executions with a project_dir, clone into a sandbox. + // Resume executions run directly in project_dir to pick up the previous session. + var sandboxDir string + effectiveWorkingDir := projectDir + if projectDir != "" && e.ResumeSessionID == "" { + var err error + sandboxDir, err = setupSandbox(projectDir) + if err != nil { + return fmt.Errorf("setting up sandbox: %w", err) + } + effectiveWorkingDir = sandboxDir + r.Logger.Info("sandbox created", "sandbox", sandboxDir, "project_dir", projectDir) + } + questionFile := filepath.Join(logDir, "question.json") args := r.buildArgs(t, e, questionFile) @@ -95,9 +117,12 @@ func (r *ClaudeRunner) Run(ctx context.Context, t *task.Task, e *storage.Executi ) } attempt++ - return r.execOnce(ctx, args, t.Claude.WorkingDir, e) + return r.execOnce(ctx, args, effectiveWorkingDir, e) }) if err != nil { + if sandboxDir != "" { + return fmt.Errorf("%w (sandbox preserved at %s)", err, sandboxDir) + } return err } @@ -105,8 +130,89 @@ func (r *ClaudeRunner) Run(ctx context.Context, t *task.Task, e *storage.Executi data, readErr := os.ReadFile(questionFile) if readErr == nil { os.Remove(questionFile) // consumed + // Preserve sandbox on BLOCKED — agent may have partial work. return &BlockedError{QuestionJSON: strings.TrimSpace(string(data)), SessionID: e.SessionID} } + + // Merge sandbox back to project_dir and clean up. + if sandboxDir != "" { + if mergeErr := teardownSandbox(projectDir, sandboxDir, r.Logger); mergeErr != nil { + return fmt.Errorf("sandbox teardown: %w (sandbox preserved at %s)", mergeErr, sandboxDir) + } + } + return nil +} + +// setupSandbox prepares a temporary git clone of projectDir. +// If projectDir is not a git repo it is initialised with an initial commit first. +func setupSandbox(projectDir string) (string, error) { + // Ensure projectDir is a git repo; initialise if not. + check := exec.Command("git", "-C", projectDir, "rev-parse", "--git-dir") + if err := check.Run(); err != nil { + // Not a git repo — init and commit everything. + cmds := [][]string{ + {"git", "-C", projectDir, "init"}, + {"git", "-C", projectDir, "add", "-A"}, + {"git", "-C", projectDir, "commit", "--allow-empty", "-m", "chore: initial commit"}, + } + for _, args := range cmds { + if out, err := exec.Command(args[0], args[1:]...).CombinedOutput(); err != nil { //nolint:gosec + return "", fmt.Errorf("git init %s: %w\n%s", projectDir, err, out) + } + } + } + + tempDir, err := os.MkdirTemp("", "claudomator-sandbox-*") + if err != nil { + return "", fmt.Errorf("creating sandbox dir: %w", err) + } + + // Clone into the pre-created dir (git clone requires the target to not exist, + // so remove it first and let git recreate it). + if err := os.Remove(tempDir); err != nil { + return "", fmt.Errorf("removing temp dir placeholder: %w", err) + } + out, err := exec.Command("git", "clone", "--local", projectDir, tempDir).CombinedOutput() + if err != nil { + return "", fmt.Errorf("git clone: %w\n%s", err, out) + } + return tempDir, nil +} + +// teardownSandbox verifies the sandbox is clean, merges commits back to +// projectDir via fast-forward, then removes the sandbox. +func teardownSandbox(projectDir, sandboxDir string, logger *slog.Logger) error { + // Fail if agent left uncommitted changes. + out, err := exec.Command("git", "-C", sandboxDir, "status", "--porcelain").Output() + if err != nil { + return fmt.Errorf("git status: %w", err) + } + if len(strings.TrimSpace(string(out))) > 0 { + return fmt.Errorf("uncommitted changes in sandbox (agent must commit all work):\n%s", out) + } + + // Check whether there are any new commits to merge. + ahead, err := exec.Command("git", "-C", sandboxDir, "rev-list", "--count", "origin/HEAD..HEAD").Output() + if err != nil { + // No origin/HEAD (e.g. fresh init with no prior commits) — proceed anyway. + logger.Warn("could not determine commits ahead of origin; proceeding with merge", "err", err) + } + if strings.TrimSpace(string(ahead)) == "0" { + // Nothing to merge — clean up and return. + os.RemoveAll(sandboxDir) + return nil + } + + // Fetch new commits from sandbox into project_dir and fast-forward merge. + if out, err := exec.Command("git", "-C", projectDir, "fetch", sandboxDir, "HEAD").CombinedOutput(); err != nil { + return fmt.Errorf("git fetch from sandbox: %w\n%s", err, out) + } + if out, err := exec.Command("git", "-C", projectDir, "merge", "--ff-only", "FETCH_HEAD").CombinedOutput(); err != nil { + return fmt.Errorf("git merge --ff-only FETCH_HEAD: %w\n%s", err, out) + } + + logger.Info("sandbox merged and cleaned up", "sandbox", sandboxDir, "project_dir", projectDir) + os.RemoveAll(sandboxDir) return nil } diff --git a/internal/executor/claude_test.go b/internal/executor/claude_test.go index 056c7e8..31dcf52 100644 --- a/internal/executor/claude_test.go +++ b/internal/executor/claude_test.go @@ -224,7 +224,7 @@ func TestClaudeRunner_Run_InaccessibleWorkingDir_ReturnsError(t *testing.T) { } tk := &task.Task{ Claude: task.ClaudeConfig{ - WorkingDir: "/nonexistent/path/does/not/exist", + ProjectDir: "/nonexistent/path/does/not/exist", SkipPlanning: true, }, } @@ -235,8 +235,8 @@ func TestClaudeRunner_Run_InaccessibleWorkingDir_ReturnsError(t *testing.T) { if err == nil { t.Fatal("expected error for inaccessible working_dir, got nil") } - if !strings.Contains(err.Error(), "working_dir") { - t.Errorf("expected 'working_dir' in error, got: %v", err) + if !strings.Contains(err.Error(), "project_dir") { + t.Errorf("expected 'project_dir' in error, got: %v", err) } } diff --git a/internal/task/task.go b/internal/task/task.go index f6635cc..498c364 100644 --- a/internal/task/task.go +++ b/internal/task/task.go @@ -1,6 +1,9 @@ package task -import "time" +import ( + "encoding/json" + "time" +) type State string @@ -29,7 +32,7 @@ type ClaudeConfig struct { Model string `yaml:"model" json:"model"` ContextFiles []string `yaml:"context_files" json:"context_files"` Instructions string `yaml:"instructions" json:"instructions"` - WorkingDir string `yaml:"working_dir" json:"working_dir"` + ProjectDir string `yaml:"project_dir" json:"project_dir"` MaxBudgetUSD float64 `yaml:"max_budget_usd" json:"max_budget_usd"` PermissionMode string `yaml:"permission_mode" json:"permission_mode"` AllowedTools []string `yaml:"allowed_tools" json:"allowed_tools"` @@ -39,6 +42,25 @@ type ClaudeConfig struct { SkipPlanning bool `yaml:"skip_planning" json:"skip_planning"` } +// UnmarshalJSON reads project_dir with fallback to legacy working_dir. +func (c *ClaudeConfig) UnmarshalJSON(data []byte) error { + type Alias ClaudeConfig + aux := &struct { + ProjectDir string `json:"project_dir"` + WorkingDir string `json:"working_dir"` // legacy + *Alias + }{Alias: (*Alias)(c)} + if err := json.Unmarshal(data, aux); err != nil { + return err + } + if aux.ProjectDir != "" { + c.ProjectDir = aux.ProjectDir + } else { + c.ProjectDir = aux.WorkingDir + } + return nil +} + type RetryConfig struct { MaxAttempts int `yaml:"max_attempts" json:"max_attempts"` Backoff string `yaml:"backoff" json:"backoff"` // "linear", "exponential" diff --git a/internal/task/validator_test.go b/internal/task/validator_test.go index 967eed3..02bde45 100644 --- a/internal/task/validator_test.go +++ b/internal/task/validator_test.go @@ -11,7 +11,7 @@ func validTask() *Task { Name: "Valid Task", Claude: ClaudeConfig{ Instructions: "do something", - WorkingDir: "/tmp", + ProjectDir: "/tmp", }, Priority: PriorityNormal, Retry: RetryConfig{MaxAttempts: 1, Backoff: "exponential"}, diff --git a/web/app.js b/web/app.js index 3b807c4..28d438a 100644 --- a/web/app.js +++ b/web/app.js @@ -228,9 +228,10 @@ function sortTasksByDate(tasks) { // ── Filter ──────────────────────────────────────────────────────────────────── -const HIDE_STATES = new Set(['COMPLETED', 'FAILED']); -const ACTIVE_STATES = new Set(['PENDING', 'QUEUED', 'RUNNING', 'READY', 'BLOCKED']); -const DONE_STATES = new Set(['COMPLETED', 'FAILED', 'TIMED_OUT', 'CANCELLED', 'BUDGET_EXCEEDED']); +const HIDE_STATES = new Set(['COMPLETED', 'FAILED']); +const ACTIVE_STATES = new Set(['PENDING', 'QUEUED', 'RUNNING', 'READY', 'BLOCKED']); +const INTERRUPTED_STATES = new Set(['CANCELLED', 'FAILED']); +const DONE_STATES = new Set(['COMPLETED', 'TIMED_OUT', 'BUDGET_EXCEEDED']); // filterActiveTasks uses its own set (excludes PENDING — tasks "in-flight" only) const _PANEL_ACTIVE_STATES = new Set(['RUNNING', 'READY', 'QUEUED', 'BLOCKED']); @@ -245,8 +246,9 @@ export function filterActiveTasks(tasks) { } export function filterTasksByTab(tasks, tab) { - if (tab === 'active') return tasks.filter(t => ACTIVE_STATES.has(t.state)); - if (tab === 'done') return tasks.filter(t => DONE_STATES.has(t.state)); + if (tab === 'active') return tasks.filter(t => ACTIVE_STATES.has(t.state)); + if (tab === 'interrupted') return tasks.filter(t => INTERRUPTED_STATES.has(t.state)); + if (tab === 'done') return tasks.filter(t => DONE_STATES.has(t.state)); return tasks; } @@ -517,7 +519,7 @@ function createEditForm(task) { form.appendChild(makeField('Description', 'textarea', { name: 'description', rows: '2', value: task.description || '' })); form.appendChild(makeField('Instructions', 'textarea', { name: 'instructions', rows: '4', value: c.instructions || '' })); form.appendChild(makeField('Model', 'input', { type: 'text', name: 'model', value: c.model || 'sonnet' })); - form.appendChild(makeField('Working Directory', 'input', { type: 'text', name: 'working_dir', value: c.working_dir || '', placeholder: '/path/to/repo' })); + form.appendChild(makeField('Working Directory', 'input', { type: 'text', name: 'project_dir', value: c.project_dir || '', placeholder: '/path/to/repo' })); form.appendChild(makeField('Max Budget (USD)', 'input', { type: 'number', name: 'max_budget_usd', step: '0.01', value: c.max_budget_usd != null ? String(c.max_budget_usd) : '1.00' })); form.appendChild(makeField('Timeout', 'input', { type: 'text', name: 'timeout', value: formatDurationForInput(task.timeout) || '15m', placeholder: '15m' })); @@ -569,7 +571,7 @@ async function handleEditSave(taskId, form, saveBtn) { claude: { model: get('model'), instructions: get('instructions'), - working_dir: get('working_dir'), + project_dir: get('project_dir'), max_budget_usd: parseFloat(get('max_budget_usd')), }, timeout: get('timeout'), @@ -1018,7 +1020,7 @@ async function elaborateTask(prompt, workingDir) { const res = await fetch(`${API_BASE}/api/tasks/elaborate`, { method: 'POST', headers: { 'Content-Type': 'application/json' }, - body: JSON.stringify({ prompt, working_dir: workingDir }), + body: JSON.stringify({ prompt, project_dir: workingDir }), }); if (!res.ok) { let msg = `HTTP ${res.status}`; @@ -1048,13 +1050,13 @@ function buildValidatePayload() { const f = document.getElementById('task-form'); const name = f.querySelector('[name="name"]').value; const instructions = f.querySelector('[name="instructions"]').value; - const working_dir = f.querySelector('[name="working_dir"]').value; + const project_dir = f.querySelector('[name="project_dir"]').value; const model = f.querySelector('[name="model"]').value; const allowedToolsEl = f.querySelector('[name="allowed_tools"]'); const allowed_tools = allowedToolsEl ? allowedToolsEl.value.split(',').map(s => s.trim()).filter(Boolean) : []; - return { name, claude: { instructions, working_dir, model, allowed_tools } }; + return { name, claude: { instructions, project_dir, model, allowed_tools } }; } function renderValidationResult(result) { @@ -1167,7 +1169,7 @@ function closeTaskModal() { } async function createTask(formData) { - const selectVal = formData.get('working_dir'); + const selectVal = formData.get('project_dir'); const workingDir = selectVal === '__new__' ? document.getElementById('new-project-input').value.trim() : selectVal; @@ -1177,7 +1179,7 @@ async function createTask(formData) { claude: { model: formData.get('model'), instructions: formData.get('instructions'), - working_dir: workingDir, + project_dir: workingDir, max_budget_usd: parseFloat(formData.get('max_budget_usd')), }, timeout: formData.get('timeout'), @@ -1221,7 +1223,7 @@ async function saveTemplate(formData) { claude: { model: formData.get('model'), instructions: formData.get('instructions'), - working_dir: formData.get('working_dir'), + project_dir: formData.get('project_dir'), max_budget_usd: parseFloat(formData.get('max_budget_usd')), allowed_tools: splitTrim(formData.get('allowed_tools') || ''), }, @@ -1401,7 +1403,7 @@ function renderTaskPanel(task, executions) { claudeGrid.append( makeMetaItem('Model', c.model), makeMetaItem('Max Budget', c.max_budget_usd != null ? `$${c.max_budget_usd.toFixed(2)}` : '—'), - makeMetaItem('Working Dir', c.working_dir), + makeMetaItem('Project Dir', c.project_dir), makeMetaItem('Permission Mode', c.permission_mode || 'default'), ); if (c.allowed_tools && c.allowed_tools.length > 0) { @@ -2071,15 +2073,15 @@ if (typeof document !== 'undefined') document.addEventListener('DOMContentLoaded f.querySelector('[name="name"]').value = result.name; if (result.claude && result.claude.instructions) f.querySelector('[name="instructions"]').value = result.claude.instructions; - if (result.claude && result.claude.working_dir) { + if (result.claude && result.claude.project_dir) { const sel = document.getElementById('project-select'); - const exists = [...sel.options].some(o => o.value === result.claude.working_dir); + const exists = [...sel.options].some(o => o.value === result.claude.project_dir); if (exists) { - sel.value = result.claude.working_dir; + sel.value = result.claude.project_dir; } else { sel.value = '__new__'; document.getElementById('new-project-row').hidden = false; - document.getElementById('new-project-input').value = result.claude.working_dir; + document.getElementById('new-project-input').value = result.claude.project_dir; } } if (result.claude && result.claude.model) diff --git a/web/index.html b/web/index.html index 3b7901c..842c272 100644 --- a/web/index.html +++ b/web/index.html @@ -24,6 +24,7 @@
@@ -101,7 +102,7 @@ - + -- cgit v1.2.3 From 1ce83b6b6a300f4389dd84c4477f3ca73a431524 Mon Sep 17 00:00:00 2001 From: Peter Stone Date: Sun, 8 Mar 2026 20:40:55 +0000 Subject: cli: newLogger helper, defaultServerURL, shared http client, report command - Extract newLogger() to remove duplication across run/serve/start - Add defaultServerURL const ("http://localhost:8484") used by all client commands - Move http.Client into internal/cli/http.go with 30s timeout - Add 'report' command for printing execution summaries - Add test coverage for create and serve commands Co-Authored-By: Claude Sonnet 4.6 --- internal/cli/create_test.go | 125 ++++++++++++++++++++++++++++++++++++++++++++ internal/cli/http.go | 10 ++++ internal/cli/report.go | 74 ++++++++++++++++++++++++++ internal/cli/report_test.go | 32 ++++++++++++ internal/cli/root.go | 21 +++++++- internal/cli/run.go | 7 +-- internal/cli/serve.go | 15 +++--- internal/cli/serve_test.go | 91 ++++++++++++++++++++++++++++++++ internal/cli/start.go | 12 +++-- 9 files changed, 369 insertions(+), 18 deletions(-) create mode 100644 internal/cli/create_test.go create mode 100644 internal/cli/http.go create mode 100644 internal/cli/report.go create mode 100644 internal/cli/report_test.go create mode 100644 internal/cli/serve_test.go (limited to 'internal/cli') diff --git a/internal/cli/create_test.go b/internal/cli/create_test.go new file mode 100644 index 0000000..22ce6bd --- /dev/null +++ b/internal/cli/create_test.go @@ -0,0 +1,125 @@ +package cli + +import ( + "net/http" + "net/http/httptest" + "strings" + "testing" + "time" +) + +func TestCreateTask_TimesOut(t *testing.T) { + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + select { + case <-r.Context().Done(): + case <-time.After(5 * time.Second): // fallback so srv.Close() never deadlocks + } + })) + defer srv.Close() + + orig := httpClient + httpClient = &http.Client{Timeout: 50 * time.Millisecond} + defer func() { httpClient = orig }() + + err := createTask(srv.URL, "test", "do something", "", "", "", 1.0, "15m", "normal", false) + if err == nil { + t.Fatal("expected timeout error, got nil") + } + if !strings.Contains(err.Error(), "POST /api/tasks") { + t.Errorf("expected error mentioning POST /api/tasks, got: %v", err) + } +} + +func TestStartTask_EscapesTaskID(t *testing.T) { + var capturedPath string + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + capturedPath = r.URL.RawPath + if capturedPath == "" { + capturedPath = r.URL.Path + } + w.WriteHeader(http.StatusOK) + w.Write([]byte(`{}`)) + })) + defer srv.Close() + + err := startTask(srv.URL, "task/with/slashes") + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if strings.Contains(capturedPath, "task/with/slashes") { + t.Errorf("task ID was not escaped; raw path contains unescaped slashes: %s", capturedPath) + } + if !strings.Contains(capturedPath, "task%2Fwith%2Fslashes") { + t.Errorf("expected escaped path segment, got: %s", capturedPath) + } +} + +func TestCreateTask_MissingIDField_ReturnsError(t *testing.T) { + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusOK) + w.Write([]byte(`{"name":"test"}`)) // no "id" field + })) + defer srv.Close() + + err := createTask(srv.URL, "test", "do something", "", "", "", 1.0, "15m", "normal", false) + if err == nil { + t.Fatal("expected error for missing id field, got nil") + } + if !strings.Contains(err.Error(), "without id") { + t.Errorf("expected error mentioning missing id, got: %v", err) + } +} + +func TestCreateTask_NonJSONResponse_ReturnsError(t *testing.T) { + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusBadGateway) + w.Write([]byte(`502 Bad Gateway`)) + })) + defer srv.Close() + + err := createTask(srv.URL, "test", "do something", "", "", "", 1.0, "15m", "normal", false) + if err == nil { + t.Fatal("expected error for non-JSON response, got nil") + } + if !strings.Contains(err.Error(), "invalid JSON") { + t.Errorf("expected error mentioning invalid JSON, got: %v", err) + } +} + +func TestStartTask_NonJSONResponse_ReturnsError(t *testing.T) { + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusBadGateway) + w.Write([]byte(`502 Bad Gateway`)) + })) + defer srv.Close() + + err := startTask(srv.URL, "task-abc") + if err == nil { + t.Fatal("expected error for non-JSON response, got nil") + } + if !strings.Contains(err.Error(), "invalid JSON") { + t.Errorf("expected error mentioning invalid JSON, got: %v", err) + } +} + +func TestStartTask_TimesOut(t *testing.T) { + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + select { + case <-r.Context().Done(): + case <-time.After(5 * time.Second): // fallback so srv.Close() never deadlocks + } + })) + defer srv.Close() + + orig := httpClient + httpClient = &http.Client{Timeout: 50 * time.Millisecond} + defer func() { httpClient = orig }() + + err := startTask(srv.URL, "task-abc") + if err == nil { + t.Fatal("expected timeout error, got nil") + } + if !strings.Contains(err.Error(), "POST") { + t.Errorf("expected error mentioning POST, got: %v", err) + } +} diff --git a/internal/cli/http.go b/internal/cli/http.go new file mode 100644 index 0000000..907818a --- /dev/null +++ b/internal/cli/http.go @@ -0,0 +1,10 @@ +package cli + +import ( + "net/http" + "time" +) + +const httpTimeout = 30 * time.Second + +var httpClient = &http.Client{Timeout: httpTimeout} diff --git a/internal/cli/report.go b/internal/cli/report.go new file mode 100644 index 0000000..7f95c80 --- /dev/null +++ b/internal/cli/report.go @@ -0,0 +1,74 @@ +package cli + +import ( + "fmt" + "os" + "time" + + "github.com/spf13/cobra" + "github.com/thepeterstone/claudomator/internal/reporter" + "github.com/thepeterstone/claudomator/internal/storage" +) + +func newReportCmd() *cobra.Command { + var format string + var limit int + var taskID string + + cmd := &cobra.Command{ + Use: "report", + Short: "Report execution history", + RunE: func(cmd *cobra.Command, args []string) error { + return runReport(format, limit, taskID) + }, + } + + cmd.Flags().StringVar(&format, "format", "table", "output format: table, json, html") + cmd.Flags().IntVar(&limit, "limit", 50, "maximum number of executions to show") + cmd.Flags().StringVar(&taskID, "task", "", "filter by task ID") + + return cmd +} + +func runReport(format string, limit int, taskID string) error { + var rep reporter.Reporter + switch format { + case "table", "": + rep = &reporter.ConsoleReporter{} + case "json": + rep = &reporter.JSONReporter{Pretty: true} + case "html": + rep = &reporter.HTMLReporter{} + default: + return fmt.Errorf("invalid format %q: must be table, json, or html", format) + } + + store, err := storage.Open(cfg.DBPath) + if err != nil { + return fmt.Errorf("opening db: %w", err) + } + defer store.Close() + + recent, err := store.ListRecentExecutions(time.Time{}, limit, taskID) + if err != nil { + return fmt.Errorf("listing executions: %w", err) + } + + execs := make([]*storage.Execution, len(recent)) + for i, r := range recent { + e := &storage.Execution{ + ID: r.ID, + TaskID: r.TaskID, + Status: r.State, + StartTime: r.StartedAt, + ExitCode: r.ExitCode, + CostUSD: r.CostUSD, + } + if r.FinishedAt != nil { + e.EndTime = *r.FinishedAt + } + execs[i] = e + } + + return rep.Generate(os.Stdout, execs) +} diff --git a/internal/cli/report_test.go b/internal/cli/report_test.go new file mode 100644 index 0000000..3ef96f4 --- /dev/null +++ b/internal/cli/report_test.go @@ -0,0 +1,32 @@ +package cli + +import ( + "strings" + "testing" +) + +func TestReportCmd_InvalidFormat(t *testing.T) { + cmd := newReportCmd() + cmd.SetArgs([]string{"--format", "xml"}) + err := cmd.Execute() + if err == nil { + t.Fatal("expected error for invalid format, got nil") + } + if !strings.Contains(err.Error(), "format") { + t.Errorf("expected error to mention 'format', got: %v", err) + } +} + +func TestReportCmd_DefaultsRegistered(t *testing.T) { + cmd := newReportCmd() + f := cmd.Flags() + if f.Lookup("format") == nil { + t.Error("missing --format flag") + } + if f.Lookup("limit") == nil { + t.Error("missing --limit flag") + } + if f.Lookup("task") == nil { + t.Error("missing --task flag") + } +} diff --git a/internal/cli/root.go b/internal/cli/root.go index 1a528fb..ab6ac1f 100644 --- a/internal/cli/root.go +++ b/internal/cli/root.go @@ -1,12 +1,17 @@ package cli import ( + "fmt" + "log/slog" + "os" "path/filepath" "github.com/thepeterstone/claudomator/internal/config" "github.com/spf13/cobra" ) +const defaultServerURL = "http://localhost:8484" + var ( cfgFile string verbose bool @@ -14,7 +19,12 @@ var ( ) func NewRootCmd() *cobra.Command { - cfg = config.Default() + var err error + cfg, err = config.Default() + if err != nil { + fmt.Fprintf(os.Stderr, "fatal: %v\n", err) + os.Exit(1) + } cmd := &cobra.Command{ Use: "claudomator", @@ -43,6 +53,7 @@ func NewRootCmd() *cobra.Command { newLogsCmd(), newStartCmd(), newCreateCmd(), + newReportCmd(), ) return cmd @@ -51,3 +62,11 @@ func NewRootCmd() *cobra.Command { func Execute() error { return NewRootCmd().Execute() } + +func newLogger(v bool) *slog.Logger { + level := slog.LevelInfo + if v { + level = slog.LevelDebug + } + return slog.New(slog.NewTextHandler(os.Stderr, &slog.HandlerOptions{Level: level})) +} diff --git a/internal/cli/run.go b/internal/cli/run.go index ed831f5..ebf371c 100644 --- a/internal/cli/run.go +++ b/internal/cli/run.go @@ -3,7 +3,6 @@ package cli import ( "context" "fmt" - "log/slog" "os" "os/signal" "syscall" @@ -71,11 +70,7 @@ func runTasks(file string, parallel int, dryRun bool) error { } defer store.Close() - level := slog.LevelInfo - if verbose { - level = slog.LevelDebug - } - logger := slog.New(slog.NewTextHandler(os.Stderr, &slog.HandlerOptions{Level: level})) + logger := newLogger(verbose) runner := &executor.ClaudeRunner{ BinaryPath: cfg.ClaudeBinaryPath, diff --git a/internal/cli/serve.go b/internal/cli/serve.go index 363e276..cd5bfce 100644 --- a/internal/cli/serve.go +++ b/internal/cli/serve.go @@ -3,7 +3,6 @@ package cli import ( "context" "fmt" - "log/slog" "net/http" "os" "os/signal" @@ -12,6 +11,7 @@ import ( "github.com/thepeterstone/claudomator/internal/api" "github.com/thepeterstone/claudomator/internal/executor" + "github.com/thepeterstone/claudomator/internal/notify" "github.com/thepeterstone/claudomator/internal/storage" "github.com/thepeterstone/claudomator/internal/version" "github.com/spf13/cobra" @@ -44,11 +44,7 @@ func serve(addr string) error { } defer store.Close() - level := slog.LevelInfo - if verbose { - level = slog.LevelDebug - } - logger := slog.New(slog.NewTextHandler(os.Stderr, &slog.HandlerOptions{Level: level})) + logger := newLogger(verbose) apiURL := "http://localhost" + addr if len(addr) > 0 && addr[0] != ':' { @@ -63,6 +59,9 @@ func serve(addr string) error { pool := executor.NewPool(cfg.MaxConcurrent, runner, store, logger) srv := api.NewServer(store, pool, logger, cfg.ClaudeBinaryPath) + if cfg.WebhookURL != "" { + srv.SetNotifier(notify.NewWebhookNotifier(cfg.WebhookURL, logger)) + } srv.StartHub() httpSrv := &http.Server{ @@ -81,7 +80,9 @@ func serve(addr string) error { logger.Info("shutting down server...") shutdownCtx, shutdownCancel := context.WithTimeout(ctx, 5*time.Second) defer shutdownCancel() - httpSrv.Shutdown(shutdownCtx) + if err := httpSrv.Shutdown(shutdownCtx); err != nil { + logger.Warn("shutdown error", "err", err) + } }() fmt.Printf("Claudomator %s listening on %s\n", version.Version(), addr) diff --git a/internal/cli/serve_test.go b/internal/cli/serve_test.go new file mode 100644 index 0000000..6bd0e8f --- /dev/null +++ b/internal/cli/serve_test.go @@ -0,0 +1,91 @@ +package cli + +import ( + "context" + "log/slog" + "net" + "net/http" + "sync" + "testing" + "time" +) + +// recordHandler captures log records for assertions. +type recordHandler struct { + mu sync.Mutex + records []slog.Record +} + +func (h *recordHandler) Enabled(_ context.Context, _ slog.Level) bool { return true } +func (h *recordHandler) Handle(_ context.Context, r slog.Record) error { + h.mu.Lock() + h.records = append(h.records, r) + h.mu.Unlock() + return nil +} +func (h *recordHandler) WithAttrs(_ []slog.Attr) slog.Handler { return h } +func (h *recordHandler) WithGroup(_ string) slog.Handler { return h } +func (h *recordHandler) hasWarn(msg string) bool { + h.mu.Lock() + defer h.mu.Unlock() + for _, r := range h.records { + if r.Level == slog.LevelWarn && r.Message == msg { + return true + } + } + return false +} + +// TestServe_ShutdownError_IsLogged verifies that a shutdown timeout error is +// logged as a warning rather than silently dropped. +func TestServe_ShutdownError_IsLogged(t *testing.T) { + // Start a real listener so we have an address. + ln, err := net.Listen("tcp", "127.0.0.1:0") + if err != nil { + t.Fatalf("listen: %v", err) + } + + // Handler that hangs so the active connection prevents clean shutdown. + hang := make(chan struct{}) + mux := http.NewServeMux() + mux.HandleFunc("/hang", func(w http.ResponseWriter, r *http.Request) { + <-hang + }) + + srv := &http.Server{Handler: mux} + + // Serve in background. + go srv.Serve(ln) //nolint:errcheck + + // Open a connection and start a hanging request so the server has an + // active connection when we call Shutdown. + addr := ln.Addr().String() + connReady := make(chan struct{}) + go func() { + req, _ := http.NewRequest(http.MethodGet, "http://"+addr+"/hang", nil) + close(connReady) + http.DefaultClient.Do(req) //nolint:errcheck + }() + <-connReady + // Give the goroutine a moment to establish the request. + time.Sleep(20 * time.Millisecond) + + // Shutdown with an already-expired deadline so it times out immediately. + expiredCtx, cancel := context.WithDeadline(context.Background(), time.Now().Add(-1*time.Second)) + defer cancel() + + h := &recordHandler{} + logger := slog.New(h) + + // This is the exact logic from serve.go's shutdown goroutine. + if err := srv.Shutdown(expiredCtx); err != nil { + logger.Warn("shutdown error", "err", err) + } + + // Unblock the hanging handler. + close(hang) + + if !h.hasWarn("shutdown error") { + t.Error("expected shutdown error to be logged as Warn, but it was not") + } +} diff --git a/internal/cli/start.go b/internal/cli/start.go index 6ec09b2..9e66e00 100644 --- a/internal/cli/start.go +++ b/internal/cli/start.go @@ -3,7 +3,8 @@ package cli import ( "encoding/json" "fmt" - "net/http" + "io" + "net/url" "github.com/spf13/cobra" ) @@ -25,15 +26,18 @@ func newStartCmd() *cobra.Command { } func startTask(serverURL, id string) error { - url := fmt.Sprintf("%s/api/tasks/%s/run", serverURL, id) - resp, err := http.Post(url, "application/json", nil) //nolint:noctx + url := fmt.Sprintf("%s/api/tasks/%s/run", serverURL, url.PathEscape(id)) + resp, err := httpClient.Post(url, "application/json", nil) //nolint:noctx if err != nil { return fmt.Errorf("POST %s: %w", url, err) } defer resp.Body.Close() + raw, _ := io.ReadAll(resp.Body) var body map[string]string - _ = json.NewDecoder(resp.Body).Decode(&body) + if err := json.Unmarshal(raw, &body); err != nil { + return fmt.Errorf("server returned invalid JSON (status %d): %s", resp.StatusCode, string(raw)) + } if resp.StatusCode >= 300 { return fmt.Errorf("server returned %d: %s", resp.StatusCode, body["error"]) -- cgit v1.2.3