From 54d320a61f8f9a0032d4d49f80248350fad3a39c Mon Sep 17 00:00:00 2001 From: Peter Stone Date: Thu, 9 Jul 2026 04:07:04 +0000 Subject: fix(api): reject accepting a BLOCKED task directly The nested-subtask-completion fix (ad00e18) necessarily added BLOCKED -> COMPLETED to task.ValidTransition so executor.Pool.maybeUnblockParent can complete a subtask whose own children all finished. That widened acceptTask's gate (shared by POST /api/tasks/{id}/accept and the chatbot MCP accept_task tool, which validates only via task.ValidTransition) to also permit accepting ANY BLOCKED task directly -- one still awaiting ask_user, or with genuinely incomplete subtasks -- bypassing the completion invariant this session's fix was built to protect. Caught by local subagent review of that fix. Not live in production: the deployed claudomator service is still on an earlier commit. --- internal/api/server_test.go | 117 +++++++++++++++++++++++++++----------------- 1 file changed, 71 insertions(+), 46 deletions(-) (limited to 'internal/api/server_test.go') diff --git a/internal/api/server_test.go b/internal/api/server_test.go index 460c669..d75df02 100644 --- a/internal/api/server_test.go +++ b/internal/api/server_test.go @@ -500,9 +500,9 @@ func TestListTasks_WithTasks(t *testing.T) { tk := &task.Task{ ID: fmt.Sprintf("lt-%d", i), Name: fmt.Sprintf("T%d", i), RepositoryURL: "https://github.com/user/repo", - Agent: task.AgentConfig{Type: "claude", Instructions: "x"}, Priority: task.PriorityNormal, + Agent: task.AgentConfig{Type: "claude", Instructions: "x"}, Priority: task.PriorityNormal, Retry: task.RetryConfig{MaxAttempts: 1, Backoff: "linear"}, - Tags: []string{}, DependsOn: []string{}, State: task.StatePending, + Tags: []string{}, DependsOn: []string{}, State: task.StatePending, } store.CreateTask(tk) } @@ -593,13 +593,13 @@ var stateWalkPaths = map[task.State][]task.State{ func createTaskWithState(t *testing.T, store *storage.DB, id string, state task.State) *task.Task { t.Helper() tk := &task.Task{ - ID: id, - Name: "test-task-" + id, + ID: id, + Name: "test-task-" + id, RepositoryURL: "https://github.com/user/repo", - Agent: task.AgentConfig{Type: "claude", Instructions: "do something"}, - Priority: task.PriorityNormal, - Retry: task.RetryConfig{MaxAttempts: 1, Backoff: "linear"}, - Tags: []string{}, DependsOn: []string{}, State: task.StatePending, + Agent: task.AgentConfig{Type: "claude", Instructions: "do something"}, + Priority: task.PriorityNormal, + Retry: task.RetryConfig{MaxAttempts: 1, Backoff: "linear"}, + Tags: []string{}, DependsOn: []string{}, State: task.StatePending, } if err := store.CreateTask(tk); err != nil { t.Fatalf("createTaskWithState: CreateTask: %v", err) @@ -720,6 +720,32 @@ func TestAcceptTask_NonReadyTask_Returns409(t *testing.T) { } } +// TestAcceptTask_BlockedTask_Returns409 proves a BLOCKED task can never be +// accepted directly via the human/chatbot-facing endpoint. BLOCKED -> COMPLETED +// is a valid task.ValidTransition (added so internal/executor.Pool's +// maybeUnblockParent can complete a subtask whose own children all finished), +// but that transition must only ever happen via that internal mechanism -- +// never via a direct accept call, which would bypass the subtask-completion +// invariant (or silently foreclose an open ask_user question) for a task +// that is not actually done. +func TestAcceptTask_BlockedTask_Returns409(t *testing.T) { + srv, store := testServer(t) + createTaskWithState(t, store, "accept-blocked", task.StateBlocked) + + req := httptest.NewRequest("POST", "/api/tasks/accept-blocked/accept", nil) + w := httptest.NewRecorder() + srv.Handler().ServeHTTP(w, req) + + if w.Code != http.StatusConflict { + t.Errorf("status: want 409, got %d; body: %s", w.Code, w.Body.String()) + } + + got, _ := store.GetTask("accept-blocked") + if got.State != task.StateBlocked { + t.Errorf("task state: want still BLOCKED, got %v", got.State) + } +} + func TestRejectTask_ReadyTask_Returns200(t *testing.T) { srv, store := testServer(t) createTaskWithState(t, store, "reject-ready", task.StateReady) @@ -972,15 +998,15 @@ func TestResumeTimedOut_Success_Returns202(t *testing.T) { func TestRunTask_ManualRunIgnoresRetryLimit(t *testing.T) { srv, store := testServer(t) tk := &task.Task{ - ID: "retry-limit-manual", - Name: "Retry Limit Task", + ID: "retry-limit-manual", + Name: "Retry Limit Task", RepositoryURL: "https://github.com/user/repo", - Agent: task.AgentConfig{Instructions: "do something"}, - Priority: task.PriorityNormal, - Retry: task.RetryConfig{MaxAttempts: 1, Backoff: "linear"}, - Tags: []string{}, - DependsOn: []string{}, - State: task.StateFailed, + Agent: task.AgentConfig{Instructions: "do something"}, + Priority: task.PriorityNormal, + Retry: task.RetryConfig{MaxAttempts: 1, Backoff: "linear"}, + Tags: []string{}, + DependsOn: []string{}, + State: task.StateFailed, } if err := store.CreateTask(tk); err != nil { t.Fatal(err) @@ -1010,15 +1036,15 @@ func TestRunTask_WithinRetryLimit_Returns202(t *testing.T) { srv, store := testServer(t) // Task with MaxAttempts: 3 — 1 execution used, 2 remaining. tk := &task.Task{ - ID: "retry-within-1", - Name: "Retry Within Task", + ID: "retry-within-1", + Name: "Retry Within Task", RepositoryURL: "https://github.com/user/repo", - Agent: task.AgentConfig{Instructions: "do something"}, - Priority: task.PriorityNormal, - Retry: task.RetryConfig{MaxAttempts: 3, Backoff: "linear"}, - Tags: []string{}, - DependsOn: []string{}, - State: task.StatePending, + Agent: task.AgentConfig{Instructions: "do something"}, + Priority: task.PriorityNormal, + Retry: task.RetryConfig{MaxAttempts: 3, Backoff: "linear"}, + Tags: []string{}, + DependsOn: []string{}, + State: task.StatePending, } if err := store.CreateTask(tk); err != nil { t.Fatal(err) @@ -1093,14 +1119,14 @@ func TestDeleteTask_RunningTaskRejected(t *testing.T) { // Create the task directly in RUNNING state to avoid going through state transitions. tk := &task.Task{ - ID: "running-task-del", - Name: "Running Task", + ID: "running-task-del", + Name: "Running Task", RepositoryURL: "https://github.com/user/repo", - Agent: task.AgentConfig{Instructions: "x", Model: "sonnet"}, - Priority: task.PriorityNormal, - Tags: []string{}, - DependsOn: []string{}, - State: task.StateRunning, + Agent: task.AgentConfig{Instructions: "x", Model: "sonnet"}, + Priority: task.PriorityNormal, + Tags: []string{}, + DependsOn: []string{}, + State: task.StateRunning, } if err := store.CreateTask(tk); err != nil { t.Fatal(err) @@ -1204,11 +1230,11 @@ func TestServer_CancelTask_Completed_Returns409(t *testing.T) { // mockQuestionStore implements questionStore for testing handleAnswerQuestion. type mockQuestionStore struct { - getTaskFn func(id string) (*task.Task, error) - getLatestExecutionFn func(taskID string) (*storage.Execution, error) - updateTaskQuestionFn func(taskID, questionJSON string) error - updateTaskStateFn func(id string, newState task.State) error - appendInteractionFn func(taskID string, interaction task.Interaction) error + getTaskFn func(id string) (*task.Task, error) + getLatestExecutionFn func(taskID string) (*storage.Execution, error) + updateTaskQuestionFn func(taskID, questionJSON string) error + updateTaskStateFn func(id string, newState task.State) error + appendInteractionFn func(taskID string, interaction task.Interaction) error } func (m *mockQuestionStore) GetTask(id string) (*task.Task, error) { @@ -1620,16 +1646,16 @@ func TestRunTask_AgentTimesOut_TaskSetToTimedOut(t *testing.T) { srv, store := testServerWithRunner(t, runner) tk := &task.Task{ - ID: "async-timeout-1", - Name: "timeout-test", + ID: "async-timeout-1", + Name: "timeout-test", RepositoryURL: "https://github.com/user/repo", - Agent: task.AgentConfig{Type: "claude", Instructions: "do something"}, - Priority: task.PriorityNormal, - Retry: task.RetryConfig{MaxAttempts: 1, Backoff: "linear"}, - Tags: []string{}, - DependsOn: []string{}, - State: task.StatePending, - Timeout: task.Duration{Duration: 50 * time.Millisecond}, + Agent: task.AgentConfig{Type: "claude", Instructions: "do something"}, + Priority: task.PriorityNormal, + Retry: task.RetryConfig{MaxAttempts: 1, Backoff: "linear"}, + Tags: []string{}, + DependsOn: []string{}, + State: task.StatePending, + Timeout: task.Duration{Duration: 50 * time.Millisecond}, } if err := store.CreateTask(tk); err != nil { t.Fatal(err) @@ -2126,4 +2152,3 @@ func TestHandleRunTask_CascadesRetryToFailedDeps(t *testing.T) { t.Errorf("task B: want QUEUED, got %s", b.State) } } - -- cgit v1.2.3