fix: close clarification overlay when agent moves on (#608)

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))

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))
+	}
+}

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()))

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
+}

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",
           )}
         >
-          <span className="text-muted-foreground flex-shrink-0">•</span>
-          <span>{option.label}</span>
-          {option.description && (
-            <span className="text-muted-foreground/60">— {option.description}</span>
-          )}
+          <span className="text-muted-foreground flex-shrink-0 leading-5">•</span>
+          <span className="flex-1 min-w-0">
+            <span data-testid="clarification-option-label" className="block leading-5">
+              {option.label}
+            </span>
+            {option.description && (
+              <span
+                data-testid="clarification-option-description"
+                className="block text-muted-foreground/70 mt-0.5 leading-snug"
+              >
+                {option.description}
+              </span>
+            )}
+          </span>
         </button>
       ))}
     </div>

apps/web/components/task/chat/messages/clarification-request-message.tsx

@@ -98,7 +98,10 @@ export function ClarificationRequestMessage({ comment }: ClarificationRequestMes
             </div>
           )}
           {isExpired && (
-            <div className="mt-1 ml-3 flex items-center gap-1.5 text-xs text-orange-500/80">
+            <div
+              data-testid="clarification-expired-notice"
+              className="mt-1 ml-3 flex items-center gap-1.5 text-xs text-orange-500/80"
+            >
               {getStatusIndicator()}
               Timed out (agent moved on)
             </div>

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");

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 ({