diff --git a/apps/backend/cmd/kandev/e2e_reset.go b/apps/backend/cmd/kandev/e2e_reset.go index 73430b5b9d..d49645bf77 100644 --- a/apps/backend/cmd/kandev/e2e_reset.go +++ b/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{ diff --git a/apps/backend/cmd/kandev/helpers.go b/apps/backend/cmd/kandev/helpers.go index 326f5eda6c..358f0f0316 100644 --- a/apps/backend/cmd/kandev/helpers.go +++ b/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 diff --git a/apps/backend/internal/github/service_cleanup_policy_test.go b/apps/backend/internal/github/service_cleanup_policy_test.go index 1f7e06cce5..5442aacc73 100644 --- a/apps/backend/internal/github/service_cleanup_policy_test.go +++ b/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) diff --git a/apps/backend/internal/github/service_reviews.go b/apps/backend/internal/github/service_reviews.go index 6a7cb98be7..17a2b4082a 100644 --- a/apps/backend/internal/github/service_reviews.go +++ b/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) { diff --git a/apps/web/e2e/tests/chat/quick-chat.spec.ts b/apps/web/e2e/tests/chat/quick-chat.spec.ts index 158723a27c..03ad507acf 100644 --- a/apps/web/e2e/tests/chat/quick-chat.spec.ts +++ b/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"); diff --git a/apps/web/e2e/tests/github/pr-list-task-indicator.spec.ts b/apps/web/e2e/tests/github/pr-list-task-indicator.spec.ts index 872d382e13..e907ad7133 100644 --- a/apps/web/e2e/tests/github/pr-list-task-indicator.spec.ts +++ b/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"); diff --git a/apps/web/e2e/tests/session/dockview-repository-scripts.spec.ts b/apps/web/e2e/tests/session/dockview-repository-scripts.spec.ts index b34cdddc93..2a3daaf01a 100644 --- a/apps/web/e2e/tests/session/dockview-repository-scripts.spec.ts +++ b/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; } diff --git a/apps/web/e2e/tests/session/port-forward-dialog.spec.ts b/apps/web/e2e/tests/session/port-forward-dialog.spec.ts index bfd7ebdbaf..cf71871b10 100644 --- a/apps/web/e2e/tests/session/port-forward-dialog.spec.ts +++ b/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 }; }