From 25cf4c9d4d6f3c18ee7565bf8e6172896fff00c3 Mon Sep 17 00:00:00 2001 From: Peter Stone Date: Sat, 4 Apr 2026 21:59:58 +0000 Subject: fix: prevent SHIPPABLE stories and wrong READY state on failed tasks Three related bugs fixed: 1. maybeUnblockParent: guard against promoting QUEUED leaf tasks (no subtasks) to READY. The vacuously-true 'all subtasks done' check was advancing tasks that stalled in QUEUED (due to a prior SQLite lock error) to READY on server restart via RecoverStaleBlocked, despite having only failed executions and no commits. 2. checkStoryCompletion: require COMPLETED (not just READY) for all top-level tasks before advancing a story to SHIPPABLE. READY means the checker agent is still pending or the task awaits human review; a story with READY tasks is not ready to ship. 3. handleAcceptTask: call CheckStoryCompletion after a task is accepted so stories with parent tasks (whose subtasks are all done and then the parent is manually accepted) can auto-advance to SHIPPABLE. Co-Authored-By: Claude Sonnet 4.6 --- internal/api/server.go | 3 ++ internal/executor/executor.go | 15 +++++++- internal/executor/executor_test.go | 77 +++++++++++++++++++++++++++++++++++--- 3 files changed, 88 insertions(+), 7 deletions(-) diff --git a/internal/api/server.go b/internal/api/server.go index 604f354..e7756d1 100644 --- a/internal/api/server.go +++ b/internal/api/server.go @@ -697,6 +697,9 @@ func (s *Server) handleAcceptTask(w http.ResponseWriter, r *http.Request) { writeJSON(w, http.StatusInternalServerError, map[string]string{"error": err.Error()}) return } + if t.StoryID != "" { + go s.pool.CheckStoryCompletion(r.Context(), t.StoryID) + } writeJSON(w, http.StatusOK, map[string]string{"message": "task accepted", "task_id": id}) } diff --git a/internal/executor/executor.go b/internal/executor/executor.go index 8257f31..384a323 100644 --- a/internal/executor/executor.go +++ b/internal/executor/executor.go @@ -543,6 +543,12 @@ func (p *Pool) handleRunResult(ctx context.Context, t *task.Task, exec *storage. // a terminal success state and transitions the story to SHIPPABLE if so. // Subtasks are intentionally excluded — a parent task reaching READY/COMPLETED // already accounts for its subtasks. +// CheckStoryCompletion is the exported entry point for story completion checks +// called from outside the package (e.g. the API accept handler). +func (p *Pool) CheckStoryCompletion(ctx context.Context, storyID string) { + p.checkStoryCompletion(ctx, storyID) +} + func (p *Pool) checkStoryCompletion(ctx context.Context, storyID string) { story, err := p.store.GetStory(storyID) if err != nil { @@ -566,8 +572,8 @@ func (p *Pool) checkStoryCompletion(ctx context.Context, storyID string) { continue // subtasks are covered by their parent } topLevelCount++ - if t.State != task.StateCompleted && t.State != task.StateReady { - return // not all top-level tasks done + if t.State != task.StateCompleted { + return // not all top-level tasks done; READY alone is not sufficient (checker may be pending) } } if topLevelCount == 0 { @@ -1251,6 +1257,11 @@ func (p *Pool) maybeUnblockParent(parentID string) { p.logger.Error("maybeUnblockParent: list subtasks", "parentID", parentID, "error", err) return } + // A task with no subtasks was never blocked by subtask delegation — don't promote it. + // This prevents incorrectly promoting leaf tasks that are stuck in QUEUED to READY. + if len(subtasks) == 0 { + return + } for _, sub := range subtasks { if sub.State != task.StateCompleted { return diff --git a/internal/executor/executor_test.go b/internal/executor/executor_test.go index 94ba65d..cb8205d 100644 --- a/internal/executor/executor_test.go +++ b/internal/executor/executor_test.go @@ -783,13 +783,80 @@ func TestPool_RecoverStaleBlocked_PromotesQueuedParentWithAllSubtasksDone(t *tes t.Errorf("parent state: want READY, got %s", got.State) } - // Story should have been advanced to SHIPPABLE. + // Story should still be IN_PROGRESS — READY tasks don't satisfy the completion check; + // the task must be accepted (READY → COMPLETED) before the story advances to SHIPPABLE. s, err := store.GetStory(story.ID) if err != nil { t.Fatalf("GetStory: %v", err) } - if s.Status != task.StoryShippable { - t.Errorf("story status: want SHIPPABLE, got %s", s.Status) + if s.Status != task.StoryInProgress { + t.Errorf("story status: want IN_PROGRESS, got %s", s.Status) + } +} + +// TestPool_RecoverStaleBlocked_DoesNotPromoteQueuedLeafTask verifies that a top-level +// QUEUED task with NO subtasks is not promoted to READY by RecoverStaleBlocked. +// This guards against the bug where a task that failed to start (stuck in QUEUED due +// to a DB error) was incorrectly promoted to READY because the "all subtasks done" +// check is vacuously true when there are no subtasks. +func TestPool_RecoverStaleBlocked_DoesNotPromoteQueuedLeafTask(t *testing.T) { + store := testStore(t) + logger := slog.New(slog.NewTextHandler(os.Stderr, &slog.HandlerOptions{Level: slog.LevelError})) + pool := NewPool(2, map[string]Runner{"claude": &mockRunner{}}, store, logger) + + // A top-level task stuck in QUEUED with no subtasks (e.g. DB lock prevented RUNNING transition). + leaf := makeTask("queued-leaf-no-subtasks") + leaf.State = task.StateQueued + store.CreateTask(leaf) + + pool.RecoverStaleBlocked() + + got, err := store.GetTask(leaf.ID) + if err != nil { + t.Fatalf("GetTask: %v", err) + } + if got.State != task.StateQueued { + t.Errorf("leaf task state: want QUEUED (unchanged), got %s", got.State) + } +} + +// TestPool_CheckStoryCompletion_ReadyTasksNotSufficient verifies that READY tasks +// alone do not advance a story to SHIPPABLE — tasks must be COMPLETED. +func TestPool_CheckStoryCompletion_ReadyTasksNotSufficient(t *testing.T) { + store := testStore(t) + logger := slog.New(slog.NewTextHandler(os.Stderr, &slog.HandlerOptions{Level: slog.LevelError})) + pool := NewPool(2, map[string]Runner{"claude": &mockRunner{}}, store, logger) + + now := time.Now().UTC() + story := &task.Story{ + ID: "story-ready-only", + Name: "Ready Only Story", + Status: task.StoryInProgress, + CreatedAt: now, + UpdatedAt: now, + } + store.CreateStory(story) + + // One task driven to READY (checker pending), one COMPLETED. + tk1 := makeTask("ro-task-1") + tk1.StoryID = story.ID + store.CreateTask(tk1) + for _, s := range []task.State{task.StateQueued, task.StateRunning, task.StateReady} { + store.UpdateTaskState(tk1.ID, s) + } + + tk2 := makeTask("ro-task-2") + tk2.StoryID = story.ID + store.CreateTask(tk2) + for _, s := range []task.State{task.StateQueued, task.StateRunning, task.StateReady, task.StateCompleted} { + store.UpdateTaskState(tk2.ID, s) + } + + pool.checkStoryCompletion(context.Background(), story.ID) + + got, _ := store.GetStory(story.ID) + if got.Status != task.StoryInProgress { + t.Errorf("story status: want IN_PROGRESS (tk1 still READY/checker pending), got %s", got.Status) } } @@ -1652,7 +1719,7 @@ func TestPool_CheckStoryCompletion_AllComplete(t *testing.T) { t.Fatalf("CreateStory: %v", err) } - // Create two top-level story tasks and drive them through valid transitions to READY. + // Create two top-level story tasks and drive them through valid transitions to COMPLETED. for i, id := range []string{"sctask-1", "sctask-2"} { tk := makeTask(id) tk.StoryID = "story-comp-1" @@ -1660,7 +1727,7 @@ func TestPool_CheckStoryCompletion_AllComplete(t *testing.T) { if err := store.CreateTask(tk); err != nil { t.Fatalf("CreateTask %d: %v", i, err) } - for _, s := range []task.State{task.StateQueued, task.StateRunning, task.StateReady} { + for _, s := range []task.State{task.StateQueued, task.StateRunning, task.StateReady, task.StateCompleted} { if err := store.UpdateTaskState(id, s); err != nil { t.Fatalf("UpdateTaskState %s → %s: %v", id, s, err) } -- cgit v1.2.3