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.

2 files changed, 84 insertions, 47 deletions

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)
 	}
 }
-
diff --git a/internal/api/taskops.go b/internal/api/taskops.go
index 2b133ac..a2668f3 100644
--- a/internal/api/taskops.go
+++ b/internal/api/taskops.go
@@ -36,7 +36,9 @@ type badRequestError struct{ msg string }
 
 func (e *badRequestError) Error() string { return e.msg }
 
-func badRequestf(format string, a ...any) error { return &badRequestError{msg: fmt.Sprintf(format, a...)} }
+func badRequestf(format string, a ...any) error {
+	return &badRequestError{msg: fmt.Sprintf(format, a...)}
+}
 
 // submitTaskSpec describes a task a chatbot wants created and immediately run.
 type submitTaskSpec struct {
@@ -127,6 +129,16 @@ func (s *Server) acceptTask(ctx context.Context, id string, actor event.Actor) e
 	if err != nil {
 		return errTaskNotFound
 	}
+	// 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 isn't actually
+	// done yet.
+	if t.State == task.StateBlocked {
+		return conflictf("task cannot be accepted while BLOCKED (awaiting subtasks or a question)")
+	}
 	if !task.ValidTransition(t.State, task.StateCompleted) {
 		return conflictf("task cannot be accepted from state %s", t.State)
 	}