diff --git a/apps/backend/internal/clarification/canceller.go b/apps/backend/internal/clarification/canceller.go index 7bca1a1e62..dc6c628360 100644 --- a/apps/backend/internal/clarification/canceller.go +++ b/apps/backend/internal/clarification/canceller.go @@ -34,8 +34,14 @@ func NewCanceller(store *Store, repo messageStore, eventBus EventBus, log *logge // CancelSessionAndNotify cancels all pending clarifications for a session, // unblocking WaitForResponse callers, and marks the database messages -// with agent_disconnected=true so the frontend shows a deferred notice. +// as expired (with agent_disconnected=true for context) so the frontend +// closes the interactive overlay and renders a "Timed out" history entry. // Returns the number of cancelled clarifications. +// +// Setting status=expired also prevents a UX bug where clicking the overlay's +// X button after the agent moved on would trigger a new turn via the +// respond handler's event fallback path (rejected responses were being +// forwarded to the orchestrator as "User declined to answer"). func (c *Canceller) CancelSessionAndNotify(ctx context.Context, sessionID string) int { pendingIDs := c.store.CancelSession(sessionID) if len(pendingIDs) == 0 { @@ -54,8 +60,9 @@ func (c *Canceller) CancelSessionAndNotify(ctx context.Context, sessionID string msg.Metadata = map[string]any{} } msg.Metadata["agent_disconnected"] = true + msg.Metadata["status"] = "expired" if err := c.repo.UpdateMessage(ctx, msg); err != nil { - c.logger.Warn("failed to update message with agent_disconnected", + c.logger.Warn("failed to update message with expired status", zap.String("pending_id", id), zap.String("message_id", msg.ID), zap.Error(err)) diff --git a/apps/backend/internal/clarification/canceller_test.go b/apps/backend/internal/clarification/canceller_test.go new file mode 100644 index 0000000000..774fbb8470 --- /dev/null +++ b/apps/backend/internal/clarification/canceller_test.go @@ -0,0 +1,119 @@ +package clarification + +import ( + "context" + "errors" + "testing" + "time" + + "github.com/kandev/kandev/internal/common/logger" + "github.com/kandev/kandev/internal/events/bus" + taskmodels "github.com/kandev/kandev/internal/task/models" +) + +type stubMessageStore struct { + messages map[string]*taskmodels.Message + updated []*taskmodels.Message +} + +func (s *stubMessageStore) GetTaskSession(context.Context, string) (*taskmodels.TaskSession, error) { + return nil, errors.New("not implemented") +} + +func (s *stubMessageStore) FindMessageByPendingID(_ context.Context, pendingID string) (*taskmodels.Message, error) { + m, ok := s.messages[pendingID] + if !ok { + return nil, errors.New("not found") + } + return m, nil +} + +func (s *stubMessageStore) UpdateMessage(_ context.Context, m *taskmodels.Message) error { + s.updated = append(s.updated, m) + return nil +} + +type stubEventBus struct { + events []*bus.Event +} + +func (s *stubEventBus) Publish(_ context.Context, _ string, ev *bus.Event) error { + s.events = append(s.events, ev) + return nil +} + +func newTestCanceller(t *testing.T, msgs map[string]*taskmodels.Message) (*Canceller, *stubMessageStore, *stubEventBus) { + t.Helper() + store := NewStore(time.Minute) + repo := &stubMessageStore{messages: msgs} + eventBus := &stubEventBus{} + return NewCanceller(store, repo, eventBus, logger.Default()), repo, eventBus +} + +// TestCanceller_MarksStatusExpiredOnDisconnect verifies that when the agent's +// turn ends with a pending clarification, the message is marked status=expired. +// This is what causes the frontend overlay to close — without it, the overlay +// stays interactive and a user clicking X triggers an unintended new turn via +// the event fallback path in the respond handler. +func TestCanceller_MarksStatusExpiredOnDisconnect(t *testing.T) { + msgs := map[string]*taskmodels.Message{} + c, repo, _ := newTestCanceller(t, msgs) + + pendingID := c.store.CreateRequest(&Request{SessionID: "s1"}) + msgs[pendingID] = &taskmodels.Message{ + ID: "m1", + TaskSessionID: "s1", + Metadata: map[string]any{ + "status": "pending", + }, + } + + cancelled := c.CancelSessionAndNotify(context.Background(), "s1") + if cancelled != 1 { + t.Fatalf("expected 1 cancelled, got %d", cancelled) + } + + if len(repo.updated) != 1 { + t.Fatalf("expected 1 message update, got %d", len(repo.updated)) + } + updated := repo.updated[0] + if got, _ := updated.Metadata["agent_disconnected"].(bool); !got { + t.Errorf("expected agent_disconnected=true, got %v", updated.Metadata["agent_disconnected"]) + } + if got, _ := updated.Metadata["status"].(string); got != "expired" { + t.Errorf("expected status=expired, got %q", got) + } +} + +// TestCanceller_NoMessagesToUpdate confirms that a cancel with no pending +// clarifications returns 0 and does not touch the repo. +func TestCanceller_NoMessagesToUpdate(t *testing.T) { + c, repo, _ := newTestCanceller(t, map[string]*taskmodels.Message{}) + + if got := c.CancelSessionAndNotify(context.Background(), "nonexistent"); got != 0 { + t.Errorf("expected 0 cancelled, got %d", got) + } + if len(repo.updated) != 0 { + t.Errorf("expected no message updates, got %d", len(repo.updated)) + } +} + +// TestCanceller_PublishesMessageUpdatedEvent confirms the canceller publishes +// a message.updated event so the frontend refreshes the message in place. +func TestCanceller_PublishesMessageUpdatedEvent(t *testing.T) { + msgs := map[string]*taskmodels.Message{} + c, _, eventBus := newTestCanceller(t, msgs) + + pendingID := c.store.CreateRequest(&Request{SessionID: "s1"}) + msgs[pendingID] = &taskmodels.Message{ + ID: "m1", + TaskSessionID: "s1", + Metadata: map[string]any{"status": "pending"}, + } + + c.CancelSessionAndNotify(context.Background(), "s1") + + if len(eventBus.events) != 1 { + t.Fatalf("expected 1 event, got %d", len(eventBus.events)) + } +} diff --git a/apps/backend/internal/clarification/handlers.go b/apps/backend/internal/clarification/handlers.go index ee49f7ddc9..3b4c7413da 100644 --- a/apps/backend/internal/clarification/handlers.go +++ b/apps/backend/internal/clarification/handlers.go @@ -227,8 +227,21 @@ func (h *Handlers) httpRespond(c *gin.Context) { } // Fallback path: entry not found (agent timed out, entry was cleaned up). - // Look up the original request context from the database message and - // publish an event so the orchestrator resumes the agent with a new turn. + // If the user rejected (clicked X to dismiss), they're discarding a stale + // overlay — not continuing the conversation. Treat as a no-op so we don't + // surprise them by resuming the agent with "User declined to answer". + // The message status is already "expired" (set by the canceller), so the + // chat history will keep rendering the "Timed out" entry. + if body.Rejected { + h.logger.Info("clarification rejected after agent moved on; no-op", + zap.String("pending_id", pendingID)) + c.JSON(http.StatusOK, gin.H{"success": true}) + return + } + + // User is providing an affirmative answer after the agent moved on. Update + // the clarification record and publish an event so the orchestrator resumes + // the agent with a new turn containing the answer. h.logger.Info("clarification entry not found, using event fallback", zap.String("pending_id", pendingID), zap.String("error", err.Error())) diff --git a/apps/backend/internal/clarification/handlers_test.go b/apps/backend/internal/clarification/handlers_test.go new file mode 100644 index 0000000000..37301f2cbb --- /dev/null +++ b/apps/backend/internal/clarification/handlers_test.go @@ -0,0 +1,153 @@ +package clarification + +import ( + "bytes" + "context" + "encoding/json" + "net/http" + "net/http/httptest" + "testing" + "time" + + "github.com/gin-gonic/gin" + "github.com/kandev/kandev/internal/common/logger" + "github.com/kandev/kandev/internal/events" + taskmodels "github.com/kandev/kandev/internal/task/models" +) + +type stubMessageCreator struct { + updates []struct { + pendingID string + status string + } +} + +func (s *stubMessageCreator) CreateClarificationRequestMessage( + _ context.Context, _, _, _ string, _ Question, _ string, +) (string, error) { + return "msg-id", nil +} + +func (s *stubMessageCreator) UpdateClarificationMessage( + _ context.Context, _, pendingID, status string, _ *Answer, +) error { + s.updates = append(s.updates, struct { + pendingID string + status string + }{pendingID, status}) + return nil +} + +func setupTestHandler(t *testing.T, msgs map[string]*taskmodels.Message) (*Handlers, *stubMessageStore, *stubEventBus, *stubMessageCreator) { + t.Helper() + gin.SetMode(gin.TestMode) + store := NewStore(time.Minute) + repo := &stubMessageStore{messages: msgs} + eventBus := &stubEventBus{} + messageCreator := &stubMessageCreator{} + h := NewHandlers(store, nil, messageCreator, repo, eventBus, logger.Default()) + return h, repo, eventBus, messageCreator +} + +// TestHttpRespond_RejectedAfterTimeout_NoNewTurn verifies that when the user +// clicks X on an overlay after the agent already moved on (fallback path), +// the handler does NOT publish a ClarificationAnswered event. The user is +// just dismissing a stale overlay; resuming the agent with "User declined +// to answer" is surprising and wastes a turn. +func TestHttpRespond_RejectedAfterTimeout_NoNewTurn(t *testing.T) { + // Message exists in DB (agent already moved on; canceller ran). + msgs := map[string]*taskmodels.Message{ + "pending-123": { + ID: "m1", + TaskID: "t1", + TaskSessionID: "s1", + Metadata: map[string]any{ + "status": "expired", + "agent_disconnected": true, + "question": map[string]interface{}{"prompt": "orig question"}, + }, + }, + } + h, _, eventBus, messageCreator := setupTestHandler(t, msgs) + + body := RespondBody{ + Rejected: true, + RejectReason: "User skipped", + } + rec := runRespond(t, h, "pending-123", body) + + if rec.Code != http.StatusOK { + t.Fatalf("expected 200, got %d", rec.Code) + } + for _, ev := range eventBus.events { + if ev.Type == events.ClarificationAnswered { + t.Errorf("expected no %s event; got events: %v", events.ClarificationAnswered, eventBus.events) + } + } + + // The message is already "expired" (set by the canceller). The no-op path + // must NOT re-update the status — otherwise a stale X click would overwrite + // "expired" with "rejected" and the history entry would look wrong. + if len(messageCreator.updates) != 0 { + t.Errorf("expected no message updates in rejected no-op path, got %d: %+v", + len(messageCreator.updates), messageCreator.updates) + } +} + +// TestHttpRespond_AnsweredAfterTimeout_PublishesEvent confirms that an +// affirmative answer (option selected or custom text) still goes through +// the fallback path and publishes the event so the orchestrator resumes +// the agent — the user chose to continue, so a new turn is expected. +func TestHttpRespond_AnsweredAfterTimeout_PublishesEvent(t *testing.T) { + msgs := map[string]*taskmodels.Message{ + "pending-456": { + ID: "m2", + TaskID: "t1", + TaskSessionID: "s1", + Metadata: map[string]any{ + "status": "expired", + "agent_disconnected": true, + "question": map[string]interface{}{"prompt": "orig question"}, + }, + }, + } + h, _, eventBus, _ := setupTestHandler(t, msgs) + + body := RespondBody{ + Answers: []Answer{{ + QuestionID: "q1", + SelectedOptions: []string{"opt1"}, + }}, + } + rec := runRespond(t, h, "pending-456", body) + + if rec.Code != http.StatusOK { + t.Fatalf("expected 200, got %d", rec.Code) + } + var found bool + for _, ev := range eventBus.events { + if ev.Type == events.ClarificationAnswered { + found = true + break + } + } + if !found { + t.Errorf("expected %s event to be published", events.ClarificationAnswered) + } +} + +func runRespond(t *testing.T, h *Handlers, pendingID string, body RespondBody) *httptest.ResponseRecorder { + t.Helper() + payload, err := json.Marshal(body) + if err != nil { + t.Fatalf("marshal: %v", err) + } + req := httptest.NewRequest(http.MethodPost, "/api/v1/clarification/"+pendingID+"/respond", bytes.NewReader(payload)) + req.Header.Set("Content-Type", "application/json") + rec := httptest.NewRecorder() + c, _ := gin.CreateTestContext(rec) + c.Request = req + c.Params = gin.Params{gin.Param{Key: "id", Value: pendingID}} + h.httpRespond(c) + return rec +} diff --git a/apps/web/components/task/chat/clarification-input-overlay.tsx b/apps/web/components/task/chat/clarification-input-overlay.tsx index 9c942a208f..49ea2d2837 100644 --- a/apps/web/components/task/chat/clarification-input-overlay.tsx +++ b/apps/web/components/task/chat/clarification-input-overlay.tsx @@ -68,16 +68,25 @@ function ClarificationOptions({ disabled={isSubmitting} data-testid="clarification-option" className={cn( - "flex items-start gap-2 w-full text-left text-xs rounded px-1.5 py-0.5 -ml-1.5 transition-colors", + "flex items-start gap-2 w-full text-left text-xs rounded px-1.5 py-1 -ml-1.5 transition-colors", "hover:bg-blue-500/15 hover:text-blue-600 dark:hover:text-blue-400", isSubmitting ? "opacity-50 cursor-not-allowed" : "text-foreground/80 cursor-pointer", )} > - • - {option.label} - {option.description && ( - — {option.description} - )} + • + + + {option.label} + + {option.description && ( + + {option.description} + + )} + ))} diff --git a/apps/web/components/task/chat/messages/clarification-request-message.tsx b/apps/web/components/task/chat/messages/clarification-request-message.tsx index 1df71cf6c7..165c3a0ba9 100644 --- a/apps/web/components/task/chat/messages/clarification-request-message.tsx +++ b/apps/web/components/task/chat/messages/clarification-request-message.tsx @@ -98,7 +98,10 @@ export function ClarificationRequestMessage({ comment }: ClarificationRequestMes )} {isExpired && ( -
+
{getStatusIndicator()} Timed out (agent moved on)
diff --git a/apps/web/e2e/pages/session-page.ts b/apps/web/e2e/pages/session-page.ts index be96f65baf..fb98efcaab 100644 --- a/apps/web/e2e/pages/session-page.ts +++ b/apps/web/e2e/pages/session-page.ts @@ -227,6 +227,21 @@ export class SessionPage { return this.page.getByTestId("clarification-deferred-notice"); } + /** Expired notice rendered in chat history when the agent timed out waiting. */ + clarificationExpiredNotice(): Locator { + return this.page.getByTestId("clarification-expired-notice"); + } + + /** Label span inside a clarification option. */ + clarificationOptionLabels(): Locator { + return this.clarificationOverlay().getByTestId("clarification-option-label"); + } + + /** Description span inside a clarification option (hidden when option has none). */ + clarificationOptionDescriptions(): Locator { + return this.clarificationOverlay().getByTestId("clarification-option-description"); + } + /** Reset context button in the chat input toolbar. */ resetContextButton(): Locator { return this.page.getByTestId("reset-context-button"); diff --git a/apps/web/e2e/tests/chat/clarification.spec.ts b/apps/web/e2e/tests/chat/clarification.spec.ts index 68e46af7ac..f62114e23a 100644 --- a/apps/web/e2e/tests/chat/clarification.spec.ts +++ b/apps/web/e2e/tests/chat/clarification.spec.ts @@ -85,7 +85,11 @@ test.describe("Clarification flow", () => { await expect(session.idleInput()).toBeVisible({ timeout: 30_000 }); }); - test("timeout and reconciliation", async ({ testPage, apiClient, seedData }) => { + test("timeout closes overlay and renders expired entry in history", async ({ + testPage, + apiClient, + seedData, + }) => { const session = await seedClarificationTask( testPage, apiClient, @@ -98,15 +102,52 @@ test.describe("Clarification flow", () => { await expect(session.clarificationOverlay()).toBeVisible({ timeout: 30_000 }); // Wait for agent to time out (5s) and complete its turn. - // The "timed out" text appears in chat and the deferred notice should show. await expect(session.chat).toContainText("timed out", { timeout: 30_000 }); - await expect(session.clarificationDeferredNotice()).toBeVisible({ timeout: 10_000 }); - // User responds after agent has moved on — this triggers the event fallback path - await session.clarificationOption("PostgreSQL").click(); + // Overlay should auto-close once the canceller marks status=expired. The + // deferred "your response will be sent as a new message" notice must NOT + // appear — we're not keeping a stale interactive prompt around. + await expect(session.clarificationOverlay()).not.toBeVisible({ timeout: 10_000 }); + await expect(session.clarificationDeferredNotice()).not.toBeVisible(); - // Orchestrator resumes agent with new turn via ClarificationAnswered event - await expect(session.idleInput()).toBeVisible({ timeout: 30_000 }); + // Chat history should show the question as expired (orange X + label). + await expect(session.clarificationExpiredNotice()).toBeVisible(); + + // Chat input returns to the default idle placeholder — not the clarification + // one. Confirms no new turn was triggered by the timeout flow. + await expect(session.idleInput()).toBeVisible({ timeout: 10_000 }); + }); + + test("options render label and description on separate rows", async ({ + testPage, + apiClient, + seedData, + }) => { + const session = await seedClarificationTask( + testPage, + apiClient, + seedData, + "Clarification Layout", + "clarification", + ); + + await expect(session.clarificationOverlay()).toBeVisible({ timeout: 30_000 }); + + // The mock scenario uses three options, each with a label and description. + const labels = session.clarificationOptionLabels(); + const descriptions = session.clarificationOptionDescriptions(); + await expect(labels).toHaveCount(3); + await expect(descriptions).toHaveCount(3); + + // Label and description must be stacked vertically (description's top + // edge sits below the label's bottom edge). Regression guard for the + // old layout that rendered them side-by-side on a single row. + const labelBox = await labels.first().boundingBox(); + const descriptionBox = await descriptions.first().boundingBox(); + if (!labelBox || !descriptionBox) { + throw new Error("expected both label and description to have bounding boxes"); + } + expect(descriptionBox.y).toBeGreaterThanOrEqual(labelBox.y + labelBox.height - 1); }); test("plan mode + clarification does not leave pointer-events stuck on body", async ({