test(e2e): de-flake review-watch, port-forward, and pr-list specs (#1330)

apps/backend/cmd/kandev/e2e_reset.go

@@ -11,10 +11,14 @@ import (
 
 	"github.com/kandev/kandev/internal/automation"
 	"github.com/kandev/kandev/internal/common/logger"
+	"github.com/kandev/kandev/internal/github"
 	sqliterepo "github.com/kandev/kandev/internal/task/repository/sqlite"
 	taskservice "github.com/kandev/kandev/internal/task/service"
 )
 
+// errKey is the JSON field used for error responses from the E2E endpoints.
+const errKey = "error"
+
 // registerE2EResetRoutes registers the E2E test-only endpoints.
 // The endpoints are available when KANDEV_MOCK_AGENT is "true" or "only" (dev/E2E modes).
 func registerE2EResetRoutes(
@@ -22,6 +26,7 @@ func registerE2EResetRoutes(
 	repo *sqliterepo.Repository,
 	taskSvc *taskservice.Service,
 	automationSvc *automation.Service,
+	githubSvc *github.Service,
 	log *logger.Logger,
 ) {
 	mockMode := os.Getenv("KANDEV_MOCK_AGENT")
@@ -30,7 +35,7 @@ func registerE2EResetRoutes(
 	}
 
 	api := router.Group("/api/v1/e2e")
-	api.DELETE("/reset/:workspaceId", handleE2EReset(repo, taskSvc, automationSvc, log))
+	api.DELETE("/reset/:workspaceId", handleE2EReset(repo, taskSvc, automationSvc, githubSvc, log))
 	// Hidden-workflow factory: lets E2E tests cover the system-only
 	// workflow path (e.g. improve-kandev) without depending on the real
 	// bootstrap endpoint, which clones from GitHub and shells out to gh.
@@ -43,6 +48,7 @@ func handleE2EReset(
 	repo *sqliterepo.Repository,
 	taskSvc *taskservice.Service,
 	automationSvc *automation.Service,
+	githubSvc *github.Service,
 	log *logger.Logger,
 ) gin.HandlerFunc {
 	return func(c *gin.Context) {
@@ -87,6 +93,26 @@ func handleE2EReset(
 			log.Warn("e2e reset: agent settings reset failed", zap.Error(err))
 		}
 
+		// Wipe GitHub review watches (and their dedup rows + owned tasks) so
+		// review-watch specs stay isolated. seedData/backend are worker-scoped,
+		// so a watch an earlier test created stays enabled; the global review
+		// poller polls every enabled watch and would create a duplicate task
+		// for any PR a later test adds that the stale watch also matches.
+		// Done before task deletion so the poller can't recreate tasks mid-reset.
+		var deletedWatches int
+		if githubSvc != nil {
+			n, err := githubSvc.DeleteReviewWatchesByWorkspace(ctx, workspaceID)
+			if err != nil {
+				// Abort like every other cleanup below: leaving stale watches
+				// behind reintroduces the cross-test poller pollution this
+				// endpoint exists to prevent.
+				log.Error("e2e reset: review watch cleanup failed", zap.Error(err))
+				c.JSON(http.StatusInternalServerError, gin.H{errKey: err.Error()})
+				return
+			}
+			deletedWatches = n
+		}
+
 		// Route through the task service (rather than a raw SQL DELETE) so
 		// each delete spawns the async cleanup goroutine that stops the
 		// agentctl instance and releases its port. Without this, instances
@@ -96,7 +122,7 @@ func handleE2EReset(
 		tasks, total, err := repo.ListTasksByWorkspace(ctx, workspaceID, "", "", "", 1, resetPageSize, true, true, false, false)
 		if err != nil {
 			log.Error("e2e reset: failed to list tasks", zap.Error(err))
-			c.JSON(http.StatusInternalServerError, gin.H{"error": err.Error()})
+			c.JSON(http.StatusInternalServerError, gin.H{errKey: err.Error()})
 			return
 		}
 		if total > resetPageSize {
@@ -116,7 +142,7 @@ func handleE2EReset(
 				// would create orphan rows visible to subsequent tests.
 				log.Error("e2e reset: failed to delete task",
 					zap.String("task_id", t.ID), zap.Error(err))
-				c.JSON(http.StatusInternalServerError, gin.H{"error": err.Error()})
+				c.JSON(http.StatusInternalServerError, gin.H{errKey: err.Error()})
 				return
 			}
 			deletedTasks++
@@ -125,21 +151,22 @@ func handleE2EReset(
 		deletedWorkflows, err := repo.DeleteWorkflowsByWorkspace(ctx, workspaceID, keepWorkflowIDs)
 		if err != nil {
 			log.Error("e2e reset: failed to delete workflows", zap.Error(err))
-			c.JSON(http.StatusInternalServerError, gin.H{"error": err.Error()})
+			c.JSON(http.StatusInternalServerError, gin.H{errKey: err.Error()})
 			return
 		}
 
 		deletedAutomations, autoErr := deleteAutomationsForReset(ctx, automationSvc, workspaceID)
 		if autoErr != nil {
 			log.Error("e2e reset: failed to delete automations", zap.Error(autoErr))
-			c.JSON(http.StatusInternalServerError, gin.H{"error": autoErr.Error()})
+			c.JSON(http.StatusInternalServerError, gin.H{errKey: autoErr.Error()})
 			return
 		}
 
 		c.JSON(http.StatusOK, gin.H{
-			"deleted_tasks":       deletedTasks,
-			"deleted_workflows":   deletedWorkflows,
-			"deleted_automations": deletedAutomations,
+			"deleted_tasks":          deletedTasks,
+			"deleted_workflows":      deletedWorkflows,
+			"deleted_automations":    deletedAutomations,
+			"deleted_review_watches": deletedWatches,
 		})
 	}
 }
@@ -164,7 +191,7 @@ func handleE2ECreateHiddenWorkflow(taskSvc *taskservice.Service, log *logger.Log
 	return func(c *gin.Context) {
 		var body e2eHiddenWorkflowRequest
 		if err := c.ShouldBindJSON(&body); err != nil || body.WorkspaceID == "" || body.Name == "" {
-			c.JSON(http.StatusBadRequest, gin.H{"error": "workspace_id and name are required"})
+			c.JSON(http.StatusBadRequest, gin.H{errKey: "workspace_id and name are required"})
 			return
 		}
 		workflow, err := taskSvc.CreateWorkflow(c.Request.Context(), &taskservice.CreateWorkflowRequest{
@@ -174,7 +201,7 @@ func handleE2ECreateHiddenWorkflow(taskSvc *taskservice.Service, log *logger.Log
 		})
 		if err != nil {
 			log.Error("e2e: failed to create hidden workflow", zap.Error(err))
-			c.JSON(http.StatusInternalServerError, gin.H{"error": err.Error()})
+			c.JSON(http.StatusInternalServerError, gin.H{errKey: err.Error()})
 			return
 		}
 		c.JSON(http.StatusCreated, gin.H{

apps/backend/cmd/kandev/helpers.go

@@ -801,7 +801,7 @@ func registerSecondaryRoutes(
 	if p.services.Automation != nil {
 		automationSvc = p.services.Automation.Service
 	}
-	registerE2EResetRoutes(p.router, p.taskRepo, p.taskSvc, automationSvc, p.log)
+	registerE2EResetRoutes(p.router, p.taskRepo, p.taskSvc, automationSvc, p.services.GitHub, p.log)
 
 	if officetestharness.Enabled() {
 		var officeAgentSvc *officeagents.AgentService

apps/backend/internal/github/service_cleanup_policy_test.go

@@ -694,6 +694,94 @@ func TestDeleteReviewWatch_CascadesDedupRows(t *testing.T) {
 	}
 }
 
+func TestDeleteReviewWatchesByWorkspace(t *testing.T) {
+	_, svc, _, store := setupPollerTest(t)
+	ctx := context.Background()
+
+	// Wire a deleter so the watch-delete task-reaping branch runs: the
+	// workspace sweep must reap each cleared watch's owned task and leave the
+	// survivor's task untouched.
+	rec := &recordingTaskDeleter{}
+	svc.SetTaskDeleter(rec)
+
+	mk := func(ws string) *ReviewWatch {
+		w := &ReviewWatch{WorkspaceID: ws, Enabled: true}
+		if err := store.CreateReviewWatch(ctx, w); err != nil {
+			t.Fatalf("CreateReviewWatch: %v", err)
+		}
+		rpt := &ReviewPRTask{
+			ReviewWatchID: w.ID,
+			RepoOwner:     "acme",
+			RepoName:      "widget",
+			PRNumber:      1,
+			TaskID:        "task-" + w.ID,
+		}
+		if err := store.CreateReviewPRTask(ctx, rpt); err != nil {
+			t.Fatalf("CreateReviewPRTask: %v", err)
+		}
+		return w
+	}
+	// Two watches in ws-1 (to be cleared) and one in ws-2 that must survive.
+	w1 := mk("ws-1")
+	w2 := mk("ws-1")
+	wOther := mk("ws-2")
+
+	deleted, err := svc.DeleteReviewWatchesByWorkspace(ctx, "ws-1")
+	if err != nil {
+		t.Fatalf("DeleteReviewWatchesByWorkspace: %v", err)
+	}
+	if deleted != 2 {
+		t.Fatalf("deleted = %d, want 2", deleted)
+	}
+
+	ws1, err := store.ListReviewWatches(ctx, "ws-1")
+	if err != nil {
+		t.Fatalf("ListReviewWatches ws-1: %v", err)
+	}
+	if len(ws1) != 0 {
+		t.Fatalf("ws-1 watches remain: %d", len(ws1))
+	}
+	ws2, err := store.ListReviewWatches(ctx, "ws-2")
+	if err != nil {
+		t.Fatalf("ListReviewWatches ws-2: %v", err)
+	}
+	if len(ws2) != 1 {
+		t.Fatalf("ws-2 watches = %d, want 1 (unaffected)", len(ws2))
+	}
+
+	// Dedup rows for the deleted watches are gone; the survivor's remain.
+	for _, w := range []*ReviewWatch{w1, w2} {
+		rows, err := store.ListReviewPRTasksByWatch(ctx, w.ID)
+		if err != nil {
+			t.Fatalf("ListReviewPRTasksByWatch: %v", err)
+		}
+		if len(rows) != 0 {
+			t.Fatalf("dedup rows leaked for deleted watch %s: %d", w.ID, len(rows))
+		}
+	}
+	survivor, err := store.ListReviewPRTasksByWatch(ctx, wOther.ID)
+	if err != nil {
+		t.Fatalf("ListReviewPRTasksByWatch survivor: %v", err)
+	}
+	if len(survivor) != 1 {
+		t.Fatalf("survivor dedup rows = %d, want 1", len(survivor))
+	}
+
+	// Only the cleared workspace's tasks are reaped; the survivor's is not.
+	reaped := map[string]bool{}
+	for _, id := range rec.calls {
+		reaped[id] = true
+	}
+	for _, w := range []*ReviewWatch{w1, w2} {
+		if !reaped["task-"+w.ID] {
+			t.Fatalf("task for cleared watch %s was not reaped; calls=%v", w.ID, rec.calls)
+		}
+	}
+	if reaped["task-"+wOther.ID] {
+		t.Fatalf("survivor task was reaped; calls=%v", rec.calls)
+	}
+}
+
 func TestNormalizeCleanupPolicy(t *testing.T) {
 	if got := NormalizeCleanupPolicy(""); got != CleanupPolicyAuto {
 		t.Fatalf("empty → %q, want auto", got)

apps/backend/internal/github/service_reviews.go

@@ -184,6 +184,30 @@ func (s *Service) DeleteReviewWatch(ctx context.Context, id string) error {
 	return s.store.DeleteReviewWatch(ctx, id)
 }
 
+// DeleteReviewWatchesByWorkspace deletes every review watch in a workspace —
+// its dedup rows (transactionally, in the store) and any tasks it owned. The
+// E2E reset endpoint calls this to keep worker-scoped specs isolated: review
+// watches are not otherwise cleared between tests, so a stale enabled watch
+// would let the global review poller create duplicate tasks for PRs a later
+// test adds that the stale watch also matches. Best-effort per watch — a
+// single failure logs Warn and the sweep continues. Returns the count deleted.
+func (s *Service) DeleteReviewWatchesByWorkspace(ctx context.Context, workspaceID string) (int, error) {
+	watches, err := s.store.ListReviewWatches(ctx, workspaceID)
+	if err != nil {
+		return 0, fmt.Errorf("list review watches: %w", err)
+	}
+	deleted := 0
+	for _, w := range watches {
+		if err := s.DeleteReviewWatch(ctx, w.ID); err != nil {
+			s.logger.Warn("failed to delete review watch during workspace reset",
+				zap.String("watch_id", w.ID), zap.Error(err))
+			continue
+		}
+		deleted++
+	}
+	return deleted, nil
+}
+
 // CheckReviewWatch checks for new PRs needing review and returns ones not yet tracked.
 // If watch.Repos is empty, all repos are queried. Otherwise, each repo is queried individually.
 func (s *Service) CheckReviewWatch(ctx context.Context, watch *ReviewWatch) ([]*PR, error) {

apps/web/e2e/tests/chat/quick-chat.spec.ts

@@ -97,8 +97,11 @@ test.describe("Quick Chat", () => {
 
     const dialog = await openQuickChatWithAgent(testPage);
 
-    // Type initial text.
+    // Type initial text. Re-gate on the editor being editable: eager init can
+    // flip it back to contenteditable=false (agent briefly RUNNING) after the
+    // open helper's initial check, and fill() requires an editable element.
     const editor = dialog.locator(".tiptap.ProseMirror");
+    await expect(editor).toHaveAttribute("contenteditable", "true", { timeout: 30_000 });
     await editor.click();
     await editor.fill("fix the bug");
 

apps/web/e2e/tests/github/pr-list-task-indicator.spec.ts

@@ -95,8 +95,16 @@ test.describe("GitHub PR list task indicator", () => {
 
     await testPage.goto("/github");
 
+    // Gate on the PR list finishing its first-load chain (client GitHub-status
+    // fetch -> AuthenticatedLayout mount -> PR search) before asserting on the
+    // per-row indicators. /github doesn't SSR-hydrate github status, so the list
+    // only renders after that serial chain resolves. Waiting for the three rows
+    // here lets every indicator assertion below run against an already-rendered
+    // list with the default timeout instead of racing the load.
+    await expect(testPage.getByTestId("pr-row")).toHaveCount(3, { timeout: 15_000 });
+
     const singleIndicator = testPage.getByTestId("pr-row-task-indicator-single");
-    await expect(singleIndicator).toBeVisible({ timeout: 15_000 });
+    await expect(singleIndicator).toBeVisible();
     await expect(singleIndicator).toContainText("Linked task title");
 
     const emptyIndicator = testPage.getByTestId("pr-row-task-indicator-empty");

apps/web/e2e/tests/session/dockview-repository-scripts.spec.ts

@@ -28,7 +28,10 @@ async function seedTaskWithSession(
 
   const session = new SessionPage(testPage);
   await session.waitForLoad();
-  await expect(session.idleInput()).toBeVisible({ timeout: 30_000 });
+  // The auto-started mock agent can finish its turn before the client's WS
+  // subscription registers, so a raw idleInput() visibility wait loses that
+  // race. waitForChatIdle reloads once to re-derive state from SSR.
+  await session.waitForChatIdle();
 
   return session;
 }

apps/web/e2e/tests/session/port-forward-dialog.spec.ts

@@ -38,7 +38,11 @@ async function seedRemoteSession(
 
   const session = new SessionPage(testPage);
   await session.waitForLoad();
-  await expect(session.idleInput()).toBeVisible({ timeout: 30_000 });
+  // waitForChatIdle (not a raw idleInput wait) rides out the WS-subscribe race:
+  // the auto-started mock agent can complete before the client's WS subscription
+  // registers, leaving isAgentBusy=true and the idle input never rendering. The
+  // helper reloads once to re-derive state from SSR. The plain wait flaked here.
+  await session.waitForChatIdle({ timeout: 30_000 });
 
   // Reset workspace default executor so other tests aren't affected
   await apiClient.updateWorkspace(seedData.workspaceId, {
@@ -75,7 +79,9 @@ async function seedLocalSession(
 
   const session = new SessionPage(testPage);
   await session.waitForLoad();
-  await expect(session.idleInput()).toBeVisible({ timeout: 30_000 });
+  // See seedRemoteSession: waitForChatIdle handles the WS-subscribe race that
+  // makes a raw idleInput wait flake when the auto-started agent finishes early.
+  await session.waitForChatIdle({ timeout: 30_000 });
 
   return { session, sessionId: task.session_id };
 }