From bce9fd266ab324e62f19dd25fbe070f415288761 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jos=C3=A9=20Almeida?= <57680069+zeval@users.noreply.github.com> Date: Tue, 5 May 2026 19:14:57 +0100 Subject: [PATCH] feat: multi-question support for ask_user_question_kandev (#828) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * feat(backend): multi-question support for ask_user_question_kandev Reshape the MCP tool from a single prompt+options pair to a questions[] array (1-4 entries, single-choice each, custom text fallback). The clarification HTTP handler gates the response until every question has an answer, fans out one chat message per question (sharing pending_id, with question_index/question_total in metadata), and returns a JSON map keyed by question id back to the agent. The orchestrator resume prompt now formats multi-line Q1/A1, Q2/A2 summaries when the bundle has more than one question. Mock-agent gets a clarification-multi scenario and ask_user_question_test.go covers the new schema + map response. No back-compat shim — local-first tool, single-PR breaking change. * feat(web): refresh clarification overlay with multi-question + modern UI Each per-question chat message gets its own card with a "Question N of M" progress chip, a ✓ Answered badge once committed, and shadcn-aligned option buttons with kbd hints. A new useClarificationGroup hook coordinates the per-question commits (decision A) and posts every answer in a single batch once the user finishes the bundle; X rejects the whole group. The single-question case keeps the 1-9 / Esc keyboard shortcuts. Playwright e2e adds 6 multi-question scenarios (rendering, full happy path, partial state, mixed option/custom, skip-rejects-bundle, sibling-state) on top of the existing single-question regression suite. * fix(web): clarification carousel UX with stepper navigation + race fix Replaces the vertically stacked card layout with a horizontal stepper: numbered circle dots up top show step state (active/answered/pending), the single active question card sits in the middle, and Back / Next / Submit buttons sit at the bottom. Picking an option auto-advances to the next step (single-question case auto-submits, no explicit Submit button). The stepper steps are clickable so users can revise prior answers. Keyboard shortcuts: 1-9 for option pick, ArrowLeft/Right for navigation, Esc for skip. Also fixes a race that surfaced as a 400 BadRequest on the /api/v1/clarification/:id/respond endpoint when the user clicked an option before all N messages of a multi-question bundle had streamed in: findPendingClarificationGroup now gates on metadata.question_total and returns an empty array until the full bundle has arrived. Playwright suite expanded from 6 to 12 multi-question scenarios: stepper rendering, auto-advance, Back, stepper jump, Submit gating, happy path, mixed custom-text, revise-via-jump, skip-from-mid-step, number-key shortcuts, Esc skip, first-step Back disabled. * fix(clarification): address CodeRabbit/Greptile/Claude review comments Backend: - Single source of truth for question validation: rename/export NormalizeAndValidateQuestions and call it from both the HTTP and the WebSocket-side MCP handlers, so duplicate question ids and bad option counts can't slip through either path. - httpRespond now validates that every answer references a real question id from the bundle and that no id is answered twice (regression tests added). - questionsFromMessages reorders by metadata.question_index so the reconstructed bundle preserves the original question order. - adapters.CreateClarificationRequestMessages rolls back already-created messages when a later write in the bundle fails. - extractQuestionAnswers always returns a JSON map (even on rejection, with an envelope { rejected, reject_reason }) and simplifyAnswer preserves both selected_option and custom_text when both are present. - MCP tool description: rewrite the response example to match the actual output shape; add minItems / maxItems to the questions and options arrays in the JSON Schema. - Test helper: drop the local contains() shim, use strings.Contains. Frontend: - useMemo([props]) was a no-op (props is always a fresh ref); split into [message, messages] so it actually memoizes. - Drop the dead "expired" SubmitState branch — the backend never returns 410 from this endpoint. - Remove the asymmetric option-list dimming: the agent now receives both selected_option and custom_text together, so a half-grey state was misleading. - New tests: pending-clarification.test.ts covers findPendingClarificationGroup including the question_total gate, and use-clarification-group.test.ts covers the hook's record/submit/skip flows and the override-merge path used by the single-question case. * fix(web): treat clarification 409 as success so duplicate submit doesn't stick the overlay * fix(clarification): unblock agent on persistence failure + nil-guard rejected branch - Add Store.CancelRequest(pendingID) and call it from both the WebSocket MCP handler and the HTTP create-request handler when message creation fails. Without it, the agent's WaitForResponse parked for the full 2-hour MCP timeout while no clarification cards ever reached the user. Both handlers now return an explicit error response on failure. - applyAnswersToMessages now guards h.repo == nil in the rejected branch (parity with sibling expectedQuestionIDs). - extractQuestionAnswers stubs for unanswered questions in a rejected bundle now carry { answered: false, rejected: true } so agents reading per-question payloads see the rejection without inspecting the envelope. - Hook test: added 409-as-ok regression coverage for both submit and skip paths so the previous fix can't drift. - Store test: covers CancelRequest unblocking a parked WaitForResponse. * fix: drop unused e2e helper + use channel sync in CancelRequest test * refactor(web): drop dead 'message' prop from clarification overlay Both call sites pass 'messages' (the bundle group), so the legacy single-message prop was unused. Tightening the prop type also lets the useMemo depend on a single value. * fix(web): ignore modifier-keyed events in clarification carousel shortcuts Bare 1..9 / ArrowLeft / ArrowRight / Esc still drive the carousel, but Cmd/Ctrl+1..9 (browser tab switch) and Alt+ArrowLeft (browser back) now pass through as the user expects. --- apps/backend/cmd/kandev/adapters.go | 111 ++-- apps/backend/cmd/mock-agent/scenarios.go | 86 ++- .../internal/clarification/canceller.go | 34 +- .../internal/clarification/canceller_test.go | 55 +- .../internal/clarification/handlers.go | 447 ++++++++++--- .../internal/clarification/handlers_test.go | 244 +++++++- apps/backend/internal/clarification/store.go | 17 + .../internal/clarification/store_test.go | 48 +- apps/backend/internal/clarification/types.go | 24 +- .../backend/internal/mcp/handlers/handlers.go | 50 +- .../mcp/server/ask_user_question_test.go | 246 ++++++++ apps/backend/internal/mcp/server/handlers.go | 260 +++++--- apps/backend/internal/mcp/server/server.go | 130 ++-- .../event_handlers_clarification.go | 13 + .../executor/executor_mocks_test.go | 6 + .../task/handlers/process_handlers_test.go | 6 + .../internal/task/repository/interface.go | 2 + .../task/repository/sqlite/message.go | 60 +- .../internal/task/service/service_messages.go | 21 +- .../quick-chat/quick-chat-content.tsx | 11 +- .../task/chat/clarification-input-overlay.tsx | 586 +++++++++++------- .../task/chat/clarification-overlay-parts.tsx | 257 ++++++++ apps/web/components/task/task-chat-panel.tsx | 3 +- apps/web/e2e/pages/session-page.ts | 61 ++ apps/web/e2e/tests/chat/clarification.spec.ts | 366 +++++++++-- .../session/use-clarification-group.test.ts | 187 ++++++ .../session/use-clarification-group.ts | 153 +++++ apps/web/hooks/use-processed-messages.ts | 10 +- apps/web/lib/types/http-agents.ts | 7 + .../lib/utils/pending-clarification.test.ts | 72 ++- apps/web/lib/utils/pending-clarification.ts | 29 + 31 files changed, 2995 insertions(+), 607 deletions(-) create mode 100644 apps/backend/internal/mcp/server/ask_user_question_test.go create mode 100644 apps/web/components/task/chat/clarification-overlay-parts.tsx create mode 100644 apps/web/hooks/domains/session/use-clarification-group.test.ts create mode 100644 apps/web/hooks/domains/session/use-clarification-group.ts diff --git a/apps/backend/cmd/kandev/adapters.go b/apps/backend/cmd/kandev/adapters.go index 272db2b370..f71f6c8af6 100644 --- a/apps/backend/cmd/kandev/adapters.go +++ b/apps/backend/cmd/kandev/adapters.go @@ -622,51 +622,86 @@ func (a *messageCreatorAdapter) UpdatePermissionMessage(ctx context.Context, ses return a.svc.UpdatePermissionMessage(ctx, sessionID, pendingID, status) } -// CreateClarificationRequestMessage creates a message for a clarification request. -// This allows clarification requests to appear in the chat as messages. -func (a *messageCreatorAdapter) CreateClarificationRequestMessage(ctx context.Context, taskID, sessionID, pendingID string, question clarification.Question, clarificationContext string) (string, error) { - // Convert question options to interface{} for metadata storage - options := make([]interface{}, len(question.Options)) - for j, opt := range question.Options { - options[j] = map[string]interface{}{ - "option_id": opt.ID, - "label": opt.Label, - "description": opt.Description, +// CreateClarificationRequestMessages emits one chat message per question in a +// clarification request bundle, all sharing the given pending_id. The frontend +// renders them as a stacked group; only the last message sets RequestsInput=true +// so the chat scrolls to the bottom of the group when the bundle arrives. +// Returns the created message IDs in the same order as the input questions. +// +// If any individual CreateMessage call fails, every previously created message +// in the bundle is deleted so we don't leave a half-rendered group dangling in +// the chat. Best-effort: if cleanup itself fails the caller still receives the +// original error and the orphan messages stay (logged at warn-level). +func (a *messageCreatorAdapter) CreateClarificationRequestMessages(ctx context.Context, taskID, sessionID, pendingID string, questions []clarification.Question, clarificationContext string) ([]string, error) { + ids := make([]string, 0, len(questions)) + total := len(questions) + for i, question := range questions { + options := make([]interface{}, len(question.Options)) + for j, opt := range question.Options { + options[j] = map[string]interface{}{ + "option_id": opt.ID, + "label": opt.Label, + "description": opt.Description, + } } - } - questionData := map[string]interface{}{ - "id": question.ID, - "title": question.Title, - "prompt": question.Prompt, - "options": options, - } + questionData := map[string]interface{}{ + "id": question.ID, + "title": question.Title, + "prompt": question.Prompt, + "options": options, + } - metadata := map[string]interface{}{ - "pending_id": pendingID, - "question": questionData, - "context": clarificationContext, - "status": "pending", - } + metadata := map[string]interface{}{ + "pending_id": pendingID, + "question_id": question.ID, + "question": questionData, + "question_index": i, + "question_total": total, + "context": clarificationContext, + "status": "pending", + } - msg, err := a.svc.CreateMessage(ctx, &taskservice.CreateMessageRequest{ - TaskSessionID: sessionID, - TaskID: taskID, - Content: question.Prompt, - AuthorType: "agent", - Type: "clarification_request", - Metadata: metadata, - RequestsInput: true, // This marks the session as waiting for input - }) - if err != nil { - return "", err + // Only the last message marks the session as waiting for input so the + // chat scrolls to the end of the group when the bundle arrives. + requestsInput := i == total-1 + + msg, err := a.svc.CreateMessage(ctx, &taskservice.CreateMessageRequest{ + TaskSessionID: sessionID, + TaskID: taskID, + Content: question.Prompt, + AuthorType: "agent", + Type: "clarification_request", + Metadata: metadata, + RequestsInput: requestsInput, + }) + if err != nil { + a.rollbackPartialBundle(ctx, ids, pendingID) + return nil, err + } + ids = append(ids, msg.ID) } - return msg.ID, nil + return ids, nil } -// UpdateClarificationMessage updates a clarification message's status and response -func (a *messageCreatorAdapter) UpdateClarificationMessage(ctx context.Context, sessionID, pendingID, status string, answer *clarification.Answer) error { - return a.svc.UpdateClarificationMessage(ctx, sessionID, pendingID, status, answer) +// rollbackPartialBundle removes the messages already created when later writes +// in a multi-question bundle fail. Best-effort — failures are logged but do +// not bubble up because the caller already has a more useful error. +func (a *messageCreatorAdapter) rollbackPartialBundle(ctx context.Context, ids []string, pendingID string) { + for _, id := range ids { + if err := a.svc.DeleteMessage(ctx, id); err != nil { + a.logger.Warn("failed to roll back partial clarification bundle message", + zap.String("pending_id", pendingID), + zap.String("message_id", id), + zap.Error(err)) + } + } +} + +// UpdateClarificationMessage updates a clarification message's status and answer +// for a specific (pending_id, question_id) pair within the session. +func (a *messageCreatorAdapter) UpdateClarificationMessage(ctx context.Context, sessionID, pendingID, questionID, status string, answer *clarification.Answer) error { + return a.svc.UpdateClarificationMessageForQuestion(ctx, sessionID, pendingID, questionID, status, answer) } // CreateAgentMessageStreaming creates a new agent message with a pre-generated ID. diff --git a/apps/backend/cmd/mock-agent/scenarios.go b/apps/backend/cmd/mock-agent/scenarios.go index 067a4ec784..e6dea59122 100644 --- a/apps/backend/cmd/mock-agent/scenarios.go +++ b/apps/backend/cmd/mock-agent/scenarios.go @@ -28,6 +28,7 @@ var scenarioRegistry = map[string]func(e *emitter){ "untracked-file-setup": scenarioUntrackedFileSetup, "untracked-file-modify": scenarioUntrackedFileModify, "clarification": scenarioClarification, + "clarification-multi": scenarioClarificationMulti, "clarification-timeout": scenarioClarificationTimeout, "multi-permission": scenarioMultiPermission, "review-cumulative-setup": scenarioReviewCumulativeSetup, @@ -420,18 +421,75 @@ func scenarioUntrackedFileModify(e *emitter) { e.text("untracked-file-modify complete: untracked_test.txt now has MODIFIED_CONTENT") } -// clarificationQuestionArgs returns the MCP arguments for the clarification question. +// MCP argument keys used by the clarification scenarios. Pulled out so goconst +// stays happy when the same string would otherwise repeat across both helpers. +const ( + clarificationOptionsKey = "options" + clarificationLabelKey = "label" + clarificationDescKey = "description" + clarificationPromptKey = "prompt" + clarificationIDKey = "id" +) + +func mockOption(label, description string) map[string]any { + return map[string]any{clarificationLabelKey: label, clarificationDescKey: description} +} + +// clarificationQuestionArgs returns the MCP arguments for a single-question +// clarification call. Used by the simplest e2e scenario where a one-question +// bundle exercises the same code path as multi-question. func clarificationQuestionArgs() map[string]any { return map[string]any{ - "prompt": "Which database should we use for this project?", - "options": []map[string]any{ - {"label": "PostgreSQL", "description": "Relational database with strong consistency"}, - {"label": "MongoDB", "description": "Document database for flexible schemas"}, - {"label": "SQLite", "description": "Embedded database for simplicity"}, + "questions": []map[string]any{ + { + clarificationIDKey: "db", + clarificationPromptKey: "Which database should we use for this project?", + clarificationOptionsKey: []map[string]any{ + mockOption("PostgreSQL", "Relational database with strong consistency"), + mockOption("MongoDB", "Document database for flexible schemas"), + mockOption("SQLite", "Embedded database for simplicity"), + }, + }, }, } } +// clarificationMultiQuestionArgs returns the MCP arguments for a 3-question +// clarification bundle that exercises the multi-question UI flow. +func clarificationMultiQuestionArgs() map[string]any { + return map[string]any{ + "questions": []map[string]any{ + { + clarificationIDKey: "db", + clarificationPromptKey: "Which database should we use?", + clarificationOptionsKey: []map[string]any{ + mockOption("PostgreSQL", "Relational database with strong consistency"), + mockOption("MongoDB", "Document database for flexible schemas"), + mockOption("SQLite", "Embedded database for simplicity"), + }, + }, + { + clarificationIDKey: "language", + clarificationPromptKey: "Which language should we use?", + clarificationOptionsKey: []map[string]any{ + mockOption("Go", "Strong typing, fast compile"), + mockOption("TypeScript", "Familiar to the team"), + mockOption("Rust", "Best for systems work"), + }, + }, + { + clarificationIDKey: "deploy", + clarificationPromptKey: "How should we deploy?", + clarificationOptionsKey: []map[string]any{ + mockOption("Docker", "Containerized deploy"), + mockOption("Bare metal", "Run directly on hosts"), + }, + }, + }, + "context": "Picking the foundational stack — answer all three so we can move forward.", + } +} + // scenarioClarification: happy path — ask a question via MCP and wait for the answer. func scenarioClarification(e *emitter) { fixedDelay(100) @@ -447,6 +505,22 @@ func scenarioClarification(e *emitter) { e.text(fmt.Sprintf("You answered: %s", result)) } +// scenarioClarificationMulti: stress the multi-question path — 3 questions in +// one bundle, all required, single MCP call. +func scenarioClarificationMulti(e *emitter) { + fixedDelay(100) + e.text("Let me ask you a few questions about the project setup.") + + result, err := callMCPTool("kandev", "ask_user_question_kandev", clarificationMultiQuestionArgs()) + if err != nil { + e.text(fmt.Sprintf("Questions failed: %s", err)) + return + } + + fixedDelay(50) + e.text(fmt.Sprintf("You answered: %s", result)) +} + // scenarioClarificationTimeout: ask a question with a short timeout, then continue. func scenarioClarificationTimeout(e *emitter) { fixedDelay(100) diff --git a/apps/backend/internal/clarification/canceller.go b/apps/backend/internal/clarification/canceller.go index dc6c628360..ecf5199ddf 100644 --- a/apps/backend/internal/clarification/canceller.go +++ b/apps/backend/internal/clarification/canceller.go @@ -49,28 +49,28 @@ func (c *Canceller) CancelSessionAndNotify(ctx context.Context, sessionID string } for _, id := range pendingIDs { - msg, err := c.repo.FindMessageByPendingID(ctx, id) - if err != nil || msg == nil { - c.logger.Debug("message not found for cancelled clarification", + msgs, err := c.repo.FindMessagesByPendingID(ctx, id) + if err != nil || len(msgs) == 0 { + c.logger.Debug("messages not found for cancelled clarification", zap.String("pending_id", id), zap.Error(err)) continue } - if msg.Metadata == nil { - msg.Metadata = map[string]any{} + for _, msg := range msgs { + if msg.Metadata == nil { + msg.Metadata = map[string]any{} + } + msg.Metadata["agent_disconnected"] = true + msg.Metadata["status"] = string(StatusExpired) + if err := c.repo.UpdateMessage(ctx, msg); err != nil { + c.logger.Warn("failed to update message with expired status", + zap.String("pending_id", id), + zap.String("message_id", msg.ID), + zap.Error(err)) + continue + } + c.publishMessageUpdated(ctx, msg) } - 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 expired status", - zap.String("pending_id", id), - zap.String("message_id", msg.ID), - zap.Error(err)) - continue - } - - // Publish message.updated event so the frontend picks up the metadata change - c.publishMessageUpdated(ctx, msg) } return len(pendingIDs) diff --git a/apps/backend/internal/clarification/canceller_test.go b/apps/backend/internal/clarification/canceller_test.go index 774fbb8470..81ec97fa17 100644 --- a/apps/backend/internal/clarification/canceller_test.go +++ b/apps/backend/internal/clarification/canceller_test.go @@ -12,7 +12,7 @@ import ( ) type stubMessageStore struct { - messages map[string]*taskmodels.Message + messages map[string][]*taskmodels.Message updated []*taskmodels.Message } @@ -21,11 +21,19 @@ func (s *stubMessageStore) GetTaskSession(context.Context, string) (*taskmodels. } func (s *stubMessageStore) FindMessageByPendingID(_ context.Context, pendingID string) (*taskmodels.Message, error) { - m, ok := s.messages[pendingID] - if !ok { + msgs, ok := s.messages[pendingID] + if !ok || len(msgs) == 0 { return nil, errors.New("not found") } - return m, nil + return msgs[0], nil +} + +func (s *stubMessageStore) FindMessagesByPendingID(_ context.Context, pendingID string) ([]*taskmodels.Message, error) { + msgs, ok := s.messages[pendingID] + if !ok { + return nil, nil + } + return msgs, nil } func (s *stubMessageStore) UpdateMessage(_ context.Context, m *taskmodels.Message) error { @@ -42,7 +50,7 @@ func (s *stubEventBus) Publish(_ context.Context, _ string, ev *bus.Event) error return nil } -func newTestCanceller(t *testing.T, msgs map[string]*taskmodels.Message) (*Canceller, *stubMessageStore, *stubEventBus) { +func newTestCanceller(t *testing.T, msgs map[string][]*taskmodels.Message) (*Canceller, *stubMessageStore, *stubEventBus) { t.Helper() store := NewStore(time.Minute) repo := &stubMessageStore{messages: msgs} @@ -56,17 +64,17 @@ func newTestCanceller(t *testing.T, msgs map[string]*taskmodels.Message) (*Cance // 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{} + msgs := map[string][]*taskmodels.Message{} c, repo, _ := newTestCanceller(t, msgs) pendingID := c.store.CreateRequest(&Request{SessionID: "s1"}) - msgs[pendingID] = &taskmodels.Message{ + msgs[pendingID] = []*taskmodels.Message{{ ID: "m1", TaskSessionID: "s1", Metadata: map[string]any{ "status": "pending", }, - } + }} cancelled := c.CancelSessionAndNotify(context.Background(), "s1") if cancelled != 1 { @@ -88,7 +96,7 @@ func TestCanceller_MarksStatusExpiredOnDisconnect(t *testing.T) { // 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{}) + 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) @@ -101,15 +109,15 @@ func TestCanceller_NoMessagesToUpdate(t *testing.T) { // 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{} + msgs := map[string][]*taskmodels.Message{} c, _, eventBus := newTestCanceller(t, msgs) pendingID := c.store.CreateRequest(&Request{SessionID: "s1"}) - msgs[pendingID] = &taskmodels.Message{ + msgs[pendingID] = []*taskmodels.Message{{ ID: "m1", TaskSessionID: "s1", Metadata: map[string]any{"status": "pending"}, - } + }} c.CancelSessionAndNotify(context.Background(), "s1") @@ -117,3 +125,26 @@ func TestCanceller_PublishesMessageUpdatedEvent(t *testing.T) { t.Fatalf("expected 1 event, got %d", len(eventBus.events)) } } + +// TestCanceller_MultiQuestion_MarksAllMessagesExpired confirms a multi-question +// bundle has every message marked expired (and emits one update event each) +// so the frontend overlay closes for the whole stack on agent timeout. +func TestCanceller_MultiQuestion_MarksAllMessagesExpired(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", "question_id": "q1"}}, + {ID: "m2", TaskSessionID: "s1", Metadata: map[string]any{"status": "pending", "question_id": "q2"}}, + {ID: "m3", TaskSessionID: "s1", Metadata: map[string]any{"status": "pending", "question_id": "q3"}}, + } + + cancelled := c.CancelSessionAndNotify(context.Background(), "s1") + if cancelled != 1 { + t.Fatalf("expected 1 cancelled bundle, got %d", cancelled) + } + if len(eventBus.events) != 3 { + t.Fatalf("expected 3 message.updated events, got %d", len(eventBus.events)) + } +} diff --git a/apps/backend/internal/clarification/handlers.go b/apps/backend/internal/clarification/handlers.go index 3b4c7413da..a2784926f9 100644 --- a/apps/backend/internal/clarification/handlers.go +++ b/apps/backend/internal/clarification/handlers.go @@ -6,6 +6,8 @@ import ( "errors" "fmt" "net/http" + "sort" + "strings" "github.com/gin-gonic/gin" "github.com/kandev/kandev/internal/common/logger" @@ -16,10 +18,19 @@ import ( "go.uber.org/zap" ) +// Metadata key constants used when constructing event payloads and reading +// per-message clarification metadata. Pulled out so goconst stays happy and +// renames stay safe. +const ( + metaQuestionKey = "question" + metaQuestionIDKey = "question_id" +) + // messageStore is the minimal task repository interface required by clarification handlers. type messageStore interface { GetTaskSession(ctx context.Context, id string) (*taskmodels.TaskSession, error) FindMessageByPendingID(ctx context.Context, pendingID string) (*taskmodels.Message, error) + FindMessagesByPendingID(ctx context.Context, pendingID string) ([]*taskmodels.Message, error) UpdateMessage(ctx context.Context, message *taskmodels.Message) error } @@ -30,10 +41,16 @@ type Broadcaster interface { // MessageCreator interface for creating messages in the database type MessageCreator interface { - // CreateClarificationRequestMessage creates a message for a clarification request - CreateClarificationRequestMessage(ctx context.Context, taskID, sessionID, pendingID string, question Question, clarificationContext string) (string, error) - // UpdateClarificationMessage updates a clarification message's status and response - UpdateClarificationMessage(ctx context.Context, sessionID, pendingID, status string, answer *Answer) error + // CreateClarificationRequestMessages creates one chat message per question in + // a multi-question clarification request, all sharing the given pending_id. + // Only the last message returned should set RequestsInput=true so the chat + // scrolls to the bottom of the group. Returns the created message IDs in the + // same order as the input questions. + CreateClarificationRequestMessages(ctx context.Context, taskID, sessionID, pendingID string, questions []Question, clarificationContext string) ([]string, error) + // UpdateClarificationMessage updates the per-question clarification message's + // status (and stores the matching answer if any) for a (pending_id, question_id) + // pair within the session. + UpdateClarificationMessage(ctx context.Context, sessionID, pendingID, questionID, status string, answer *Answer) error } // EventBus interface for publishing events. @@ -74,11 +91,13 @@ func RegisterRoutes(router *gin.Engine, store *Store, hub Broadcaster, messageCr } // CreateRequestBody is the request body for creating a clarification request. +// A single request may bundle 1..N questions; the bundle is gated on the user +// answering every question (or rejecting the bundle as a whole). type CreateRequestBody struct { - SessionID string `json:"session_id" binding:"required"` - TaskID string `json:"task_id"` - Question Question `json:"question" binding:"required"` - Context string `json:"context"` + SessionID string `json:"session_id" binding:"required"` + TaskID string `json:"task_id"` + Questions []Question `json:"questions" binding:"required,min=1,dive"` + Context string `json:"context"` } // CreateRequestResponse is the response for creating a clarification request. @@ -93,15 +112,9 @@ func (h *Handlers) httpCreateRequest(c *gin.Context) { return } - // Validate question has ID, generate if missing - if body.Question.ID == "" { - body.Question.ID = "q1" - } - // Validate options have IDs - for j := range body.Question.Options { - if body.Question.Options[j].ID == "" { - body.Question.Options[j].ID = generateOptionID(0, j) - } + if errMsg := NormalizeAndValidateQuestions(body.Questions); errMsg != "" { + c.JSON(http.StatusBadRequest, gin.H{"error": errMsg}) + return } // Look up the task ID for this session @@ -121,35 +134,87 @@ func (h *Handlers) httpCreateRequest(c *gin.Context) { req := &Request{ SessionID: sessionID, TaskID: taskID, - Question: body.Question, + Questions: body.Questions, Context: body.Context, } pendingID := h.store.CreateRequest(req) - // Create a message in the database for the clarification request. - // This triggers the session.message.added WebSocket event which the frontend listens to. + // Create one message per question in the database; all share the same + // pending_id and are rendered as a stacked group on the frontend. The + // session.message.added WebSocket event fires per message. On failure we + // also cancel the in-store pending entry so any blocking WaitForResponse + // caller unblocks immediately rather than waiting for the MCP timeout. if h.messageCreator != nil { - _, err := h.messageCreator.CreateClarificationRequestMessage( + _, err := h.messageCreator.CreateClarificationRequestMessages( c.Request.Context(), taskID, sessionID, pendingID, - body.Question, + body.Questions, body.Context, ) if err != nil { - h.logger.Error("failed to create clarification request message", + h.logger.Error("failed to create clarification request messages", zap.String("pending_id", pendingID), zap.String("session_id", sessionID), zap.Error(err)) - // Don't fail the request - the clarification is still created in the store + h.store.CancelRequest(pendingID) + c.JSON(http.StatusInternalServerError, gin.H{ + "error": "failed to create clarification messages: " + err.Error(), + }) + return } } c.JSON(http.StatusOK, CreateRequestResponse{PendingID: pendingID}) } +// NormalizeAndValidateQuestions is the single source of truth for clarification +// bundle validation. It mutates `questions` to assign missing IDs (q1, q2, ...) +// and option IDs, and enforces: +// - 1..4 questions per bundle +// - unique question IDs (rejects duplicates) +// - non-empty prompt +// - 2..6 options per question +// +// Both the HTTP handler (httpCreateRequest) and the WebSocket-side MCP handler +// (handleAskUserQuestion) call this so validation never drifts between paths. +// Returns "" on success or an error message describing the first failure. +func NormalizeAndValidateQuestions(questions []Question) string { + if len(questions) == 0 { + return "questions must contain at least 1 question" + } + if len(questions) > 4 { + return "questions must contain at most 4 questions" + } + seen := map[string]bool{} + for i := range questions { + if questions[i].ID == "" { + questions[i].ID = fmt.Sprintf("q%d", i+1) + } + if seen[questions[i].ID] { + return fmt.Sprintf("duplicate question id %q", questions[i].ID) + } + seen[questions[i].ID] = true + if questions[i].Prompt == "" { + return fmt.Sprintf("question %d is missing required 'prompt'", i+1) + } + if len(questions[i].Options) < 2 { + return fmt.Sprintf("question %d must have at least 2 options", i+1) + } + if len(questions[i].Options) > 6 { + return fmt.Sprintf("question %d must have at most 6 options", i+1) + } + for j := range questions[i].Options { + if questions[i].Options[j].ID == "" { + questions[i].Options[j].ID = generateOptionID(i, j) + } + } + } + return "" +} + func (h *Handlers) httpGetRequest(c *gin.Context) { pendingID := c.Param("id") @@ -174,8 +239,12 @@ func (h *Handlers) httpWaitForResponse(c *gin.Context) { } // RespondBody is the request body for responding to a clarification request. +// The frontend posts every answer at once when the user finishes the bundle +// (decision A: per-question commit collected in the hook, batched on the wire). +// Answers must contain exactly one entry per question in the original request, +// or be empty when Rejected=true. type RespondBody struct { - Answers []Answer `json:"answers"` // Keep as array for frontend compatibility, but only first is used + Answers []Answer `json:"answers"` Rejected bool `json:"rejected"` RejectReason string `json:"reject_reason"` } @@ -189,16 +258,21 @@ func (h *Handlers) httpRespond(c *gin.Context) { return } - // Extract the single answer (first one if provided) - var answer *Answer - if len(body.Answers) > 0 { - answer = &body.Answers[0] + // Gate: when not rejecting, the user must have answered every question and + // each answer must reference a question id from the original bundle (no + // duplicates, no fabricated ids). We compare against the in-store request + // first; if the entry is gone (agent already moved on), fall back to the + // persisted messages so we still validate even after the in-memory cleanup. + if !body.Rejected { + if errMsg := h.validateRespondAnswers(c.Request.Context(), pendingID, body.Answers); errMsg != "" { + c.JSON(http.StatusBadRequest, gin.H{"error": errMsg}) + return + } } - // Build the response object resp := &Response{ PendingID: pendingID, - Answer: answer, + Answers: body.Answers, Rejected: body.Rejected, RejectReason: body.RejectReason, } @@ -209,11 +283,13 @@ func (h *Handlers) httpRespond(c *gin.Context) { err := h.store.Respond(pendingID, resp) if err == nil { - // Primary path succeeded — agent will receive the answer directly. - h.updateClarificationMessage(c, pendingID, body.Rejected, answer) - h.publishPrimaryAnsweredEvent(c, pendingID, answer, body.Rejected, body.RejectReason) + // Primary path succeeded — agent will receive the answers directly. + h.applyAnswersToMessages(c, pendingID, body.Rejected, body.Answers) + h.publishPrimaryAnsweredEvent(c, pendingID, body.Answers, body.Rejected, body.RejectReason) h.logger.Info("clarification answered via primary path (same turn)", - zap.String("pending_id", pendingID)) + zap.String("pending_id", pendingID), + zap.Int("answers", len(body.Answers)), + zap.Bool("rejected", body.Rejected)) c.JSON(http.StatusOK, gin.H{"success": true}) return } @@ -246,32 +322,145 @@ func (h *Handlers) httpRespond(c *gin.Context) { zap.String("pending_id", pendingID), zap.String("error", err.Error())) - h.updateClarificationMessage(c, pendingID, body.Rejected, answer) - h.respondViaEventFallback(c, pendingID, answer, body.Rejected, body.RejectReason) + h.applyAnswersToMessages(c, pendingID, body.Rejected, body.Answers) + h.respondViaEventFallback(c, pendingID, body.Answers, body.Rejected, body.RejectReason) c.JSON(http.StatusOK, gin.H{"success": true}) } -// updateClarificationMessage updates the message in the database with status and answer. -func (h *Handlers) updateClarificationMessage(c *gin.Context, pendingID string, rejected bool, answer *Answer) { +// validateRespondAnswers enforces the all-required gate **and** the question-id +// invariant: every answer must target a real question in the bundle, every +// question must have an answer, and no question id may be answered twice. +// Returns "" on success or an error message describing the first failure. +// +// The expected question ids come from the in-store request; if the in-memory +// entry has already been cleaned up (agent timed out before user responded), +// the persisted messages serve as fallback so the late-respond path is +// validated the same way as the primary path. +func (h *Handlers) validateRespondAnswers(ctx context.Context, pendingID string, answers []Answer) string { + expected := h.expectedQuestionIDs(ctx, pendingID) + if len(expected) == 0 { + // Couldn't determine the expected set — fall back to permissive (the + // primary-path Respond will still error sensibly if the bundle is gone). + return "" + } + expectedSet := make(map[string]bool, len(expected)) + for _, id := range expected { + expectedSet[id] = true + } + + if len(answers) != len(expected) { + return fmt.Sprintf("expected %d answers, got %d", len(expected), len(answers)) + } + + seen := make(map[string]bool, len(answers)) + for i, a := range answers { + if a.QuestionID == "" { + return fmt.Sprintf("answer %d is missing question_id", i+1) + } + if !expectedSet[a.QuestionID] { + return fmt.Sprintf("answer %d references unknown question id %q", i+1, a.QuestionID) + } + if seen[a.QuestionID] { + return fmt.Sprintf("answer %d duplicates question id %q", i+1, a.QuestionID) + } + seen[a.QuestionID] = true + } + return "" +} + +// expectedQuestionIDs returns the ordered question ids the user is expected to +// answer for the given pending bundle. Falls back to the persisted messages if +// the in-store request has been cleaned up. +func (h *Handlers) expectedQuestionIDs(ctx context.Context, pendingID string) []string { + if req, ok := h.store.GetRequest(pendingID); ok && req != nil { + ids := make([]string, 0, len(req.Questions)) + for _, q := range req.Questions { + ids = append(ids, q.ID) + } + return ids + } + if h.repo == nil { + return nil + } + msgs, err := h.repo.FindMessagesByPendingID(ctx, pendingID) + if err != nil || len(msgs) == 0 { + return nil + } + ids := make([]string, 0, len(msgs)) + for _, m := range msgs { + if id := stringFromMetadata(m.Metadata, metaQuestionIDKey); id != "" { + ids = append(ids, id) + } + } + return ids +} + +// applyAnswersToMessages flips per-question status (answered/rejected) on every +// message that belongs to the bundle. When rejected, every question is marked +// rejected; when answered, each answer updates the matching question's row. +func (h *Handlers) applyAnswersToMessages(c *gin.Context, pendingID string, rejected bool, answers []Answer) { if h.messageCreator == nil { return } sessionID := h.lookupSessionForPending(c, pendingID) - status := "answered" + if rejected { - status = "rejected" + // Mark every question in the bundle as rejected. Guard h.repo for + // parity with the sibling expectedQuestionIDs path; production wires + // both, but unit tests sometimes pass a nil repo. + if h.repo == nil { + return + } + msgs, err := h.repo.FindMessagesByPendingID(c.Request.Context(), pendingID) + if err != nil || len(msgs) == 0 { + h.logger.Debug("rejected clarification: no messages to update", + zap.String("pending_id", pendingID), + zap.Error(err)) + return + } + for _, msg := range msgs { + questionID := stringFromMetadata(msg.Metadata, "question_id") + if questionID == "" { + continue + } + if err := h.messageCreator.UpdateClarificationMessage(c.Request.Context(), sessionID, pendingID, questionID, "rejected", nil); err != nil { + h.logger.Warn("failed to mark clarification question rejected", + zap.String("pending_id", pendingID), + zap.String("question_id", questionID), + zap.Error(err)) + } + } + return } - if err := h.messageCreator.UpdateClarificationMessage(c.Request.Context(), sessionID, pendingID, status, answer); err != nil { - h.logger.Warn("failed to update clarification message", - zap.String("pending_id", pendingID), - zap.Error(err)) + + for i := range answers { + ans := answers[i] + if ans.QuestionID == "" { + continue + } + if err := h.messageCreator.UpdateClarificationMessage(c.Request.Context(), sessionID, pendingID, ans.QuestionID, "answered", &ans); err != nil { + h.logger.Warn("failed to update clarification question", + zap.String("pending_id", pendingID), + zap.String("question_id", ans.QuestionID), + zap.Error(err)) + } } } +func stringFromMetadata(meta map[string]any, key string) string { + if meta == nil { + return "" + } + if v, ok := meta[key].(string); ok { + return v + } + return "" +} + // respondViaEventFallback publishes a ClarificationAnswered event for the orchestrator // to resume the agent with a new turn. Used when the agent timed out. -func (h *Handlers) respondViaEventFallback(c *gin.Context, pendingID string, answer *Answer, rejected bool, rejectReason string) { +func (h *Handlers) respondViaEventFallback(c *gin.Context, pendingID string, answers []Answer, rejected bool, rejectReason string) { if h.eventBus == nil { return } @@ -291,13 +480,13 @@ func (h *Handlers) respondViaEventFallback(c *gin.Context, pendingID string, ans return } - answerText := buildAnswerText(answer, rejected, rejectReason) + answerText := buildAnswerSummary(clarificationCtx.Questions, answers, rejected, rejectReason) eventData := map[string]any{ "session_id": clarificationCtx.SessionID, "task_id": clarificationCtx.TaskID, "pending_id": pendingID, - "question": clarificationCtx.Question, + metaQuestionKey: clarificationCtx.QuestionSummary, "answer_text": answerText, "rejected": rejected, "reject_reason": rejectReason, @@ -334,7 +523,7 @@ func (h *Handlers) lookupSessionForPending(c *gin.Context, pendingID string) str return msg.TaskSessionID } -func (h *Handlers) publishPrimaryAnsweredEvent(c *gin.Context, pendingID string, answer *Answer, rejected bool, rejectReason string) { +func (h *Handlers) publishPrimaryAnsweredEvent(c *gin.Context, pendingID string, answers []Answer, rejected bool, rejectReason string) { if h.eventBus == nil { return } @@ -353,12 +542,12 @@ func (h *Handlers) publishPrimaryAnsweredEvent(c *gin.Context, pendingID string, return } - answerText := buildAnswerText(answer, rejected, rejectReason) + answerText := buildAnswerSummary(clarificationCtx.Questions, answers, rejected, rejectReason) eventData := map[string]any{ "session_id": clarificationCtx.SessionID, "task_id": clarificationCtx.TaskID, "pending_id": pendingID, - "question": clarificationCtx.Question, + metaQuestionKey: clarificationCtx.QuestionSummary, "answer_text": answerText, "rejected": rejected, "reject_reason": rejectReason, @@ -376,9 +565,10 @@ func (h *Handlers) publishPrimaryAnsweredEvent(c *gin.Context, pendingID string, } type clarificationEventContext struct { - SessionID string - TaskID string - Question string + SessionID string + TaskID string + Questions []Question // Source-of-truth questions used to label answers; falls back to a single synthetic Question when only metadata is available. + QuestionSummary string // Pre-formatted multi-line "Q1: ...\nQ2: ..." used by the orchestrator resume prompt. } func (h *Handlers) resolveClarificationEventContext(ctx context.Context, pendingID string) (clarificationEventContext, error) { @@ -387,57 +577,166 @@ func (h *Handlers) resolveClarificationEventContext(ctx context.Context, pending if req, ok := h.store.GetRequest(pendingID); ok && req != nil { out.SessionID = req.SessionID out.TaskID = req.TaskID - out.Question = req.Question.Prompt + out.Questions = req.Questions + out.QuestionSummary = formatQuestionSummary(req.Questions) + if out.SessionID != "" && out.TaskID != "" && len(out.Questions) > 0 { + return out, nil + } } - if out.SessionID != "" && out.TaskID != "" && out.Question != "" { - return out, nil - } if h.repo == nil { return out, fmt.Errorf("message repository unavailable") } - msg, err := h.repo.FindMessageByPendingID(ctx, pendingID) + msgs, err := h.repo.FindMessagesByPendingID(ctx, pendingID) if err != nil { return out, err } + if len(msgs) == 0 { + return out, fmt.Errorf("no messages for pending_id %s", pendingID) + } + if out.SessionID == "" { - out.SessionID = msg.TaskSessionID + out.SessionID = msgs[0].TaskSessionID } if out.TaskID == "" { - out.TaskID = msg.TaskID + out.TaskID = msgs[0].TaskID } - if out.Question == "" && msg.Metadata != nil { - if qData, ok := msg.Metadata["question"].(map[string]interface{}); ok { - if q, ok := qData["prompt"].(string); ok { - out.Question = q - } - } + if len(out.Questions) == 0 { + out.Questions = questionsFromMessages(msgs) + out.QuestionSummary = formatQuestionSummary(out.Questions) } return out, nil } -// buildAnswerText constructs a human-readable answer text from the response. -func buildAnswerText(answer *Answer, rejected bool, rejectReason string) string { +// questionsFromMessages reconstructs a Question slice from persisted clarification +// messages, ordered by metadata.question_index so the rebuilt summary matches +// the bundle the agent originally sent. Used as a fallback when the in-store +// request has been cleaned up but the persisted metadata still carries the +// original question text. +func questionsFromMessages(msgs []*taskmodels.Message) []Question { + sorted := make([]*taskmodels.Message, len(msgs)) + copy(sorted, msgs) + sort.SliceStable(sorted, func(i, j int) bool { + return questionIndexFromMetadata(sorted[i].Metadata) < questionIndexFromMetadata(sorted[j].Metadata) + }) + out := make([]Question, 0, len(sorted)) + for _, m := range sorted { + out = append(out, questionFromMessageMetadata(m.Metadata)) + } + return out +} + +func questionIndexFromMetadata(meta map[string]any) int { + if meta == nil { + return 0 + } + switch v := meta["question_index"].(type) { + case int: + return v + case int64: + return int(v) + case float64: + return int(v) + } + return 0 +} + +func questionFromMessageMetadata(meta map[string]any) Question { + q := Question{ID: stringFromMetadata(meta, metaQuestionIDKey)} + qData, ok := meta[metaQuestionKey].(map[string]any) + if !ok { + return q + } + if v, ok := qData["prompt"].(string); ok { + q.Prompt = v + } + if v, ok := qData["title"].(string); ok { + q.Title = v + } + if q.ID == "" { + if v, ok := qData["id"].(string); ok { + q.ID = v + } + } + return q +} + +func formatQuestionSummary(questions []Question) string { + if len(questions) == 0 { + return "" + } + if len(questions) == 1 { + return questions[0].Prompt + } + parts := make([]string, 0, len(questions)) + for i, q := range questions { + parts = append(parts, fmt.Sprintf("Q%d: %s", i+1, q.Prompt)) + } + return strings.Join(parts, "\n") +} + +// buildAnswerSummary constructs a human-readable summary of the user's response +// across every question in the bundle. Used in the orchestrator resume prompt +// and for chat history rendering. +func buildAnswerSummary(questions []Question, answers []Answer, rejected bool, rejectReason string) string { if rejected { if rejectReason != "" { return fmt.Sprintf("User declined to answer. Reason: %s", rejectReason) } return "User declined to answer." } - if answer == nil { + if len(answers) == 0 { return "User provided no specific answer." } - if answer.CustomText != "" { - return fmt.Sprintf("User answered: %s", answer.CustomText) + if len(questions) <= 1 && len(answers) == 1 { + return formatSingleAnswer(answers[0]) } - if len(answer.SelectedOptions) > 0 { - return fmt.Sprintf("User selected: %v", answer.SelectedOptions) + + answersByID := make(map[string]Answer, len(answers)) + for _, a := range answers { + answersByID[a.QuestionID] = a + } + + parts := make([]string, 0, len(answers)) + for i, q := range questions { + ans, ok := answersByID[q.ID] + if !ok { + continue + } + parts = append(parts, fmt.Sprintf("A%d: %s", i+1, formatAnswerBody(ans))) + } + if len(parts) == 0 { + // No matches by id — fall back to positional formatting so we still + // surface the answers rather than silently dropping them. + for i, a := range answers { + parts = append(parts, fmt.Sprintf("A%d: %s", i+1, formatAnswerBody(a))) + } + } + return strings.Join(parts, "\n") +} + +func formatSingleAnswer(a Answer) string { + if a.CustomText != "" { + return fmt.Sprintf("User answered: %s", a.CustomText) + } + if len(a.SelectedOptions) > 0 { + return fmt.Sprintf("User selected: %v", a.SelectedOptions) } return "User provided no specific answer." } +func formatAnswerBody(a Answer) string { + if a.CustomText != "" { + return a.CustomText + } + if len(a.SelectedOptions) > 0 { + return fmt.Sprintf("%v", a.SelectedOptions) + } + return "(no answer)" +} + func generateOptionID(questionIndex, optionIndex int) string { return fmt.Sprintf("q%d_opt%d", questionIndex+1, optionIndex+1) } diff --git a/apps/backend/internal/clarification/handlers_test.go b/apps/backend/internal/clarification/handlers_test.go index 37301f2cbb..7dee9fb5d9 100644 --- a/apps/backend/internal/clarification/handlers_test.go +++ b/apps/backend/internal/clarification/handlers_test.go @@ -6,6 +6,7 @@ import ( "encoding/json" "net/http" "net/http/httptest" + "strings" "testing" "time" @@ -17,28 +18,36 @@ import ( type stubMessageCreator struct { updates []struct { - pendingID string - status string + pendingID string + questionID string + status string } + created [][]Question } -func (s *stubMessageCreator) CreateClarificationRequestMessage( - _ context.Context, _, _, _ string, _ Question, _ string, -) (string, error) { - return "msg-id", nil +func (s *stubMessageCreator) CreateClarificationRequestMessages( + _ context.Context, _, _, _ string, questions []Question, _ string, +) ([]string, error) { + s.created = append(s.created, questions) + ids := make([]string, len(questions)) + for i := range questions { + ids[i] = "msg-id" + } + return ids, nil } func (s *stubMessageCreator) UpdateClarificationMessage( - _ context.Context, _, pendingID, status string, _ *Answer, + _ context.Context, _, pendingID, questionID, status string, _ *Answer, ) error { s.updates = append(s.updates, struct { - pendingID string - status string - }{pendingID, status}) + pendingID string + questionID string + status string + }{pendingID, questionID, status}) return nil } -func setupTestHandler(t *testing.T, msgs map[string]*taskmodels.Message) (*Handlers, *stubMessageStore, *stubEventBus, *stubMessageCreator) { +func setupTestHandler(t *testing.T, msgs map[string][]*taskmodels.Message) (*Handlers, *stubMessageStore, *stubEventBus, *stubMessageCreator) { t.Helper() gin.SetMode(gin.TestMode) store := NewStore(time.Minute) @@ -56,17 +65,19 @@ func setupTestHandler(t *testing.T, msgs map[string]*taskmodels.Message) (*Handl // 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": { + 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"}, + "pending_id": "pending-123", + "question_id": "q1", + "question": map[string]interface{}{"id": "q1", "prompt": "orig question"}, }, - }, + }}, } h, _, eventBus, messageCreator := setupTestHandler(t, msgs) @@ -99,17 +110,19 @@ func TestHttpRespond_RejectedAfterTimeout_NoNewTurn(t *testing.T) { // 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": { + 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"}, + "pending_id": "pending-456", + "question_id": "q1", + "question": map[string]interface{}{"id": "q1", "prompt": "orig question"}, }, - }, + }}, } h, _, eventBus, _ := setupTestHandler(t, msgs) @@ -136,6 +149,199 @@ func TestHttpRespond_AnsweredAfterTimeout_PublishesEvent(t *testing.T) { } } +// TestHttpRespond_DuplicateQuestionID_Rejected400 covers the dedupe gate: +// a payload that names the same question id twice should be rejected even +// when the cardinality matches the bundle size, otherwise the agent could +// receive a phantom answer for the question that was actually skipped. +func TestHttpRespond_DuplicateQuestionID_Rejected400(t *testing.T) { + h, _, _, _ := setupTestHandler(t, map[string][]*taskmodels.Message{}) + pendingID := h.store.CreateRequest(&Request{ + SessionID: "s1", + TaskID: "t1", + Questions: []Question{ + {ID: "q1", Prompt: "First?"}, + {ID: "q2", Prompt: "Second?"}, + }, + }) + body := RespondBody{ + Answers: []Answer{ + {QuestionID: "q1", SelectedOptions: []string{"opt1"}}, + {QuestionID: "q1", SelectedOptions: []string{"opt2"}}, + }, + } + rec := runRespond(t, h, pendingID, body) + if rec.Code != http.StatusBadRequest { + t.Fatalf("expected 400 for duplicate question_id, got %d (body=%s)", rec.Code, rec.Body.String()) + } +} + +// TestHttpRespond_UnknownQuestionID_Rejected400 ensures that fabricated ids +// are rejected even with the right cardinality. +func TestHttpRespond_UnknownQuestionID_Rejected400(t *testing.T) { + h, _, _, _ := setupTestHandler(t, map[string][]*taskmodels.Message{}) + pendingID := h.store.CreateRequest(&Request{ + SessionID: "s1", + TaskID: "t1", + Questions: []Question{ + {ID: "q1", Prompt: "First?"}, + }, + }) + body := RespondBody{ + Answers: []Answer{{QuestionID: "qZZZ", SelectedOptions: []string{"opt1"}}}, + } + rec := runRespond(t, h, pendingID, body) + if rec.Code != http.StatusBadRequest { + t.Fatalf("expected 400 for unknown question_id, got %d (body=%s)", rec.Code, rec.Body.String()) + } +} + +// TestHttpRespond_PartialAnswers_Rejected400 confirms that the handler +// refuses a respond payload that does not contain one answer per question +// in the original bundle. All-required gating is enforced here. +func TestHttpRespond_PartialAnswers_Rejected400(t *testing.T) { + h, _, _, _ := setupTestHandler(t, map[string][]*taskmodels.Message{}) + + pendingID := h.store.CreateRequest(&Request{ + SessionID: "s1", + TaskID: "t1", + Questions: []Question{ + {ID: "q1", Prompt: "First?"}, + {ID: "q2", Prompt: "Second?"}, + {ID: "q3", Prompt: "Third?"}, + }, + }) + + // Only one answer for a 3-question bundle. + body := RespondBody{ + Answers: []Answer{{ + QuestionID: "q1", + SelectedOptions: []string{"opt1"}, + }}, + } + rec := runRespond(t, h, pendingID, body) + if rec.Code != http.StatusBadRequest { + t.Fatalf("expected 400, got %d (body=%s)", rec.Code, rec.Body.String()) + } +} + +// TestHttpRespond_AllAnswers_PrimaryPath_Success verifies that when every +// question is answered the primary path delivers the full response and +// updates each message exactly once. +func TestHttpRespond_AllAnswers_PrimaryPath_Success(t *testing.T) { + h, _, _, msgCreator := setupTestHandler(t, map[string][]*taskmodels.Message{}) + + pendingID := h.store.CreateRequest(&Request{ + SessionID: "s1", + TaskID: "t1", + Questions: []Question{ + {ID: "q1", Prompt: "First?"}, + {ID: "q2", Prompt: "Second?"}, + }, + }) + + // Drain the response channel so Respond does not block indefinitely. + go func() { + _, _ = h.store.WaitForResponse(context.Background(), pendingID) + }() + + body := RespondBody{ + Answers: []Answer{ + {QuestionID: "q1", SelectedOptions: []string{"opt1"}}, + {QuestionID: "q2", CustomText: "free-form"}, + }, + } + rec := runRespond(t, h, pendingID, body) + if rec.Code != http.StatusOK { + t.Fatalf("expected 200, got %d (body=%s)", rec.Code, rec.Body.String()) + } + + if len(msgCreator.updates) != 2 { + t.Fatalf("expected 2 message updates (one per question), got %d: %+v", + len(msgCreator.updates), msgCreator.updates) + } + for _, u := range msgCreator.updates { + if u.status != "answered" { + t.Errorf("expected status=answered, got %q", u.status) + } + } +} + +// TestValidateAndNormalizeQuestions_AssignsDefaults checks that question and +// option IDs are auto-generated when omitted, in deterministic q1/q1_opt1 form. +func TestValidateAndNormalizeQuestions_AssignsDefaults(t *testing.T) { + qs := []Question{ + {Prompt: "First?", Options: []Option{{Label: "A", Description: "a"}, {Label: "B", Description: "b"}}}, + {Prompt: "Second?", Options: []Option{{Label: "X", Description: "x"}, {Label: "Y", Description: "y"}}}, + } + if err := NormalizeAndValidateQuestions(qs); err != "" { + t.Fatalf("unexpected validation error: %s", err) + } + if qs[0].ID != "q1" || qs[1].ID != "q2" { + t.Errorf("expected q1/q2, got %q/%q", qs[0].ID, qs[1].ID) + } + if qs[0].Options[0].ID != "q1_opt1" || qs[1].Options[1].ID != "q2_opt2" { + t.Errorf("unexpected option IDs: %+v / %+v", qs[0].Options, qs[1].Options) + } +} + +// TestValidateAndNormalizeQuestions_RejectsInvalid covers the edge cases that +// guard against malformed payloads (no questions, too many, bad option counts). +func TestValidateAndNormalizeQuestions_RejectsInvalid(t *testing.T) { + cases := []struct { + name string + input []Question + }{ + {"no questions", nil}, + {"too many", []Question{{}, {}, {}, {}, {}}}, + {"missing prompt", []Question{{Options: []Option{{Label: "A", Description: "a"}, {Label: "B", Description: "b"}}}}}, + {"single option", []Question{{Prompt: "?", Options: []Option{{Label: "A", Description: "a"}}}}}, + {"too many options", []Question{{Prompt: "?", Options: []Option{ + {Label: "1", Description: "1"}, {Label: "2", Description: "2"}, {Label: "3", Description: "3"}, + {Label: "4", Description: "4"}, {Label: "5", Description: "5"}, {Label: "6", Description: "6"}, + {Label: "7", Description: "7"}, + }}}}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + if msg := NormalizeAndValidateQuestions(tc.input); msg == "" { + t.Fatalf("expected validation error, got nil for %+v", tc.input) + } + }) + } +} + +// TestBuildAnswerSummary_SingleQuestion preserves the original "User selected:" +// text shape so existing prompts in the orchestrator stay readable. +func TestBuildAnswerSummary_SingleQuestion(t *testing.T) { + got := buildAnswerSummary( + []Question{{ID: "q1", Prompt: "Which?"}}, + []Answer{{QuestionID: "q1", SelectedOptions: []string{"opt1"}}}, + false, "", + ) + if got != "User selected: [opt1]" { + t.Errorf("unexpected single-q summary: %q", got) + } +} + +// TestBuildAnswerSummary_MultiQuestion produces an A1/A2 layout so the +// orchestrator resume prompt clearly maps each answer to its question. +func TestBuildAnswerSummary_MultiQuestion(t *testing.T) { + got := buildAnswerSummary( + []Question{ + {ID: "q1", Prompt: "First?"}, + {ID: "q2", Prompt: "Second?"}, + }, + []Answer{ + {QuestionID: "q1", SelectedOptions: []string{"opt1"}}, + {QuestionID: "q2", CustomText: "free"}, + }, + false, "", + ) + if got == "" || !strings.Contains(got, "A1:") || !strings.Contains(got, "A2:") { + t.Errorf("expected multi-line summary with A1/A2, got %q", got) + } +} + func runRespond(t *testing.T, h *Handlers, pendingID string, body RespondBody) *httptest.ResponseRecorder { t.Helper() payload, err := json.Marshal(body) diff --git a/apps/backend/internal/clarification/store.go b/apps/backend/internal/clarification/store.go index 07f2dbf0d4..9f309c2024 100644 --- a/apps/backend/internal/clarification/store.go +++ b/apps/backend/internal/clarification/store.go @@ -132,6 +132,23 @@ func (s *Store) Respond(pendingID string, resp *Response) error { } } +// CancelRequest cancels a single pending clarification by id, unblocking any +// WaitForResponse caller that is currently parked on it. Returns true if the +// entry existed (and was removed), false otherwise. Used by callers that need +// to surface a creation-side failure immediately rather than wait for the +// 2-hour MCP timeout. +func (s *Store) CancelRequest(pendingID string) bool { + s.mu.Lock() + defer s.mu.Unlock() + pending, ok := s.pending[pendingID] + if !ok { + return false + } + close(pending.CancelCh) + delete(s.pending, pendingID) + return true +} + // CancelSession cancels all pending clarification requests for a given session. // It closes the CancelCh to unblock any WaitForResponse callers and removes entries. // Returns the list of cancelled pending IDs. diff --git a/apps/backend/internal/clarification/store_test.go b/apps/backend/internal/clarification/store_test.go index b421da5aa1..0e7ed1f8d1 100644 --- a/apps/backend/internal/clarification/store_test.go +++ b/apps/backend/internal/clarification/store_test.go @@ -22,7 +22,7 @@ func TestNewStore_CustomTimeout(t *testing.T) { func TestCreateRequest_GeneratesID(t *testing.T) { s := NewStore(time.Minute) - req := &Request{SessionID: "s1", Question: Question{Prompt: "test?"}} + req := &Request{SessionID: "s1", Questions: []Question{{Prompt: "test?"}}} id := s.CreateRequest(req) @@ -47,7 +47,7 @@ func TestCreateRequest_PreservesExistingID(t *testing.T) { func TestGetRequest_Found(t *testing.T) { s := NewStore(time.Minute) - id := s.CreateRequest(&Request{SessionID: "s1", Question: Question{Prompt: "test?"}}) + id := s.CreateRequest(&Request{SessionID: "s1", Questions: []Question{{Prompt: "test?"}}}) req, ok := s.GetRequest(id) @@ -76,14 +76,14 @@ func TestWaitForResponse_Success(t *testing.T) { // Respond in a goroutine go func() { time.Sleep(10 * time.Millisecond) - _ = s.Respond(id, &Response{Answer: &Answer{CustomText: "hello"}}) + _ = s.Respond(id, &Response{Answers: []Answer{{CustomText: "hello"}}}) }() resp, err := s.WaitForResponse(context.Background(), id) if err != nil { t.Fatalf("unexpected error: %v", err) } - if resp.Answer == nil || resp.Answer.CustomText != "hello" { + if len(resp.Answers) != 1 || resp.Answers[0].CustomText != "hello" { t.Errorf("unexpected response: %+v", resp) } @@ -155,7 +155,7 @@ func TestRespond_Success(t *testing.T) { s := NewStore(time.Minute) id := s.CreateRequest(&Request{SessionID: "s1"}) - err := s.Respond(id, &Response{Answer: &Answer{CustomText: "yes"}}) + err := s.Respond(id, &Response{Answers: []Answer{{CustomText: "yes"}}}) if err != nil { t.Fatalf("unexpected error: %v", err) } @@ -209,6 +209,44 @@ func TestCancelSession_CancelsMatchingRequests(t *testing.T) { } } +// TestCancelRequest unblocks WaitForResponse for a single pending entry. +// Used by the create-message-failure recovery path so the agent doesn't +// have to wait for the full MCP timeout when the bundle could not be +// persisted. +// +// Synchronisation: the goroutine signals it has started before invoking +// WaitForResponse so we don't rely on a time.Sleep. CancelRequest may run +// either before or after the goroutine reads from the pending map, and both +// paths return an error from WaitForResponse — that's the contract under test. +func TestCancelRequest(t *testing.T) { + s := NewStore(time.Minute) + id := s.CreateRequest(&Request{SessionID: "s1"}) + + started := make(chan struct{}) + done := make(chan error, 1) + go func() { + close(started) + _, err := s.WaitForResponse(context.Background(), id) + done <- err + }() + <-started + + if !s.CancelRequest(id) { + t.Fatalf("CancelRequest returned false for known id") + } + if s.CancelRequest(id) { + t.Errorf("CancelRequest should return false the second time") + } + select { + case err := <-done: + if err == nil { + t.Errorf("expected error from cancelled WaitForResponse") + } + case <-time.After(time.Second): + t.Fatal("WaitForResponse did not return after CancelRequest") + } +} + func TestCancelSession_NoMatch(t *testing.T) { s := NewStore(time.Minute) s.CreateRequest(&Request{SessionID: "s1"}) diff --git a/apps/backend/internal/clarification/types.go b/apps/backend/internal/clarification/types.go index c65049046d..436d2e2eb3 100644 --- a/apps/backend/internal/clarification/types.go +++ b/apps/backend/internal/clarification/types.go @@ -18,30 +18,34 @@ type Question struct { ID string `json:"id"` Title string `json:"title"` // Short label (max 12 chars) Prompt string `json:"prompt"` // Full question text - Options []Option `json:"options"` // 2-4 options + Options []Option `json:"options"` // 2-6 options } -// Request represents a clarification request from an agent. +// Request represents a clarification request from an agent. A request bundles +// one or more questions; the agent stays blocked until every question has been +// answered (or the bundle is rejected as a whole). type Request struct { - PendingID string `json:"pending_id"` - SessionID string `json:"session_id"` - TaskID string `json:"task_id"` - Question Question `json:"question"` // Single question - Context string `json:"context,omitempty"` // Optional context for the question - CreatedAt time.Time `json:"created_at"` + PendingID string `json:"pending_id"` + SessionID string `json:"session_id"` + TaskID string `json:"task_id"` + Questions []Question `json:"questions"` // 1-N questions, all required + Context string `json:"context,omitempty"` // Optional shared context for all questions + CreatedAt time.Time `json:"created_at"` } // Answer represents the user's answer to a single question. type Answer struct { QuestionID string `json:"question_id"` - SelectedOptions []string `json:"selected_options,omitempty"` // Option IDs + SelectedOptions []string `json:"selected_options,omitempty"` // Option IDs (single-choice ⇒ at most one) CustomText string `json:"custom_text,omitempty"` // Free-text input } // Response represents the user's response to a clarification request. +// On success, Answers has exactly one entry per question in the request. +// On rejection, Answers may be nil and Rejected/RejectReason describe the skip. type Response struct { PendingID string `json:"pending_id"` - Answer *Answer `json:"answer,omitempty"` + Answers []Answer `json:"answers,omitempty"` Rejected bool `json:"rejected,omitempty"` RejectReason string `json:"reject_reason,omitempty"` // If rejected RespondedAt time.Time `json:"responded_at"` diff --git a/apps/backend/internal/mcp/handlers/handlers.go b/apps/backend/internal/mcp/handlers/handlers.go index a9e4b61710..509e62ddad 100644 --- a/apps/backend/internal/mcp/handlers/handlers.go +++ b/apps/backend/internal/mcp/handlers/handlers.go @@ -33,12 +33,13 @@ import ( type ClarificationService interface { CreateRequest(req *clarification.Request) string WaitForResponse(ctx context.Context, pendingID string) (*clarification.Response, error) + CancelRequest(pendingID string) bool CancelSession(sessionID string) []string } // MessageCreator creates messages for clarification requests. type MessageCreator interface { - CreateClarificationRequestMessage(ctx context.Context, taskID, sessionID, pendingID string, question clarification.Question, clarificationContext string) (string, error) + CreateClarificationRequestMessages(ctx context.Context, taskID, sessionID, pendingID string, questions []clarification.Question, clarificationContext string) ([]string, error) } // SessionRepository interface for updating session state. @@ -940,10 +941,10 @@ func (h *Handlers) publishQueueStatusEvent(ctx context.Context, sessionID string // the event-based fallback in the orchestrator handles resuming with a new turn. func (h *Handlers) handleAskUserQuestion(ctx context.Context, msg *ws.Message) (*ws.Message, error) { var req struct { - SessionID string `json:"session_id"` - TaskID string `json:"task_id"` - Question clarification.Question `json:"question"` - Context string `json:"context"` + SessionID string `json:"session_id"` + TaskID string `json:"task_id"` + Questions []clarification.Question `json:"questions"` + Context string `json:"context"` } if err := json.Unmarshal(msg.Payload, &req); err != nil { return ws.NewError(msg.ID, msg.Action, ws.ErrorCodeBadRequest, "Invalid payload: "+err.Error(), nil) @@ -951,19 +952,11 @@ func (h *Handlers) handleAskUserQuestion(ctx context.Context, msg *ws.Message) ( if req.SessionID == "" { return ws.NewError(msg.ID, msg.Action, ws.ErrorCodeValidation, "session_id is required", nil) } - if req.Question.Prompt == "" { - return ws.NewError(msg.ID, msg.Action, ws.ErrorCodeValidation, "question.prompt is required", nil) - } - - // Generate question ID if missing - if req.Question.ID == "" { - req.Question.ID = "q1" - } - // Generate option IDs if missing - for i := range req.Question.Options { - if req.Question.Options[i].ID == "" { - req.Question.Options[i].ID = generateOptionID(0, i) - } + // Single source of truth — same validator the HTTP handler uses, so + // duplicate IDs / bad option counts / empty prompts can't slip through + // either path. + if errMsg := clarification.NormalizeAndValidateQuestions(req.Questions); errMsg != "" { + return ws.NewError(msg.ID, msg.Action, ws.ErrorCodeValidation, errMsg, nil) } // Look up task ID from session if not provided @@ -983,20 +976,26 @@ func (h *Handlers) handleAskUserQuestion(ctx context.Context, msg *ws.Message) ( clarificationReq := &clarification.Request{ SessionID: req.SessionID, TaskID: taskID, - Question: req.Question, + Questions: req.Questions, Context: req.Context, } pendingID := h.clarificationSvc.CreateRequest(clarificationReq) - // Create the message in the database (triggers WS event to frontend) + // Create one chat message per question (triggers WS events to frontend). + // If the create fails, the in-store pending entry must be cancelled too — + // otherwise the agent's WaitForResponse would block for the full 2-hour + // timeout while the user never sees clarification cards. if h.messageCreator != nil { - if _, err := h.messageCreator.CreateClarificationRequestMessage( - ctx, taskID, req.SessionID, pendingID, req.Question, req.Context, + if _, err := h.messageCreator.CreateClarificationRequestMessages( + ctx, taskID, req.SessionID, pendingID, req.Questions, req.Context, ); err != nil { - h.logger.Error("failed to create clarification request message", + h.logger.Error("failed to create clarification request messages", zap.String("pending_id", pendingID), zap.String("session_id", req.SessionID), zap.Error(err)) + h.clarificationSvc.CancelRequest(pendingID) + return ws.NewError(msg.ID, msg.Action, ws.ErrorCodeInternalError, + "failed to create clarification messages: "+err.Error(), nil) } } @@ -1097,11 +1096,6 @@ func (h *Handlers) setSessionWaitingForInput(ctx context.Context, taskID, sessio } } -// generateOptionID generates an option ID for a question. -func generateOptionID(questionIndex, optionIndex int) string { - return fmt.Sprintf("q%d_opt%d", questionIndex+1, optionIndex+1) -} - // handleCreateTaskPlan creates a new task plan. func (h *Handlers) handleCreateTaskPlan(ctx context.Context, msg *ws.Message) (*ws.Message, error) { var req struct { diff --git a/apps/backend/internal/mcp/server/ask_user_question_test.go b/apps/backend/internal/mcp/server/ask_user_question_test.go new file mode 100644 index 0000000000..b3deddbfd7 --- /dev/null +++ b/apps/backend/internal/mcp/server/ask_user_question_test.go @@ -0,0 +1,246 @@ +package mcp + +import ( + "encoding/json" + "testing" + + ws "github.com/kandev/kandev/pkg/websocket" + mcplib "github.com/mark3labs/mcp-go/mcp" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// TestAskUserQuestion_ToolSchema_RequiresQuestionsArray asserts the tool now +// exposes a "questions" array (not legacy "prompt"/"options" top-level fields). +func TestAskUserQuestion_ToolSchema_RequiresQuestionsArray(t *testing.T) { + backend := &testBackend{} + s := newTaskModeServer(t, backend, "task-current") + + toolsMap := s.mcpServer.ListTools() + tool, ok := toolsMap["ask_user_question_kandev"] + require.True(t, ok, "ask_user_question tool not registered") + + schema, err := json.Marshal(tool.Tool.InputSchema) + require.NoError(t, err) + + var parsed map[string]interface{} + require.NoError(t, json.Unmarshal(schema, &parsed)) + + props, ok := parsed["properties"].(map[string]interface{}) + require.True(t, ok, "schema should have properties") + assert.Contains(t, props, "questions", "schema must expose 'questions'") + assert.Contains(t, props, "context") + assert.NotContains(t, props, "prompt", "legacy 'prompt' must not be top-level anymore") + assert.NotContains(t, props, "options", "legacy 'options' must not be top-level anymore") + + required, _ := parsed["required"].([]interface{}) + requiredSet := make(map[string]bool) + for _, r := range required { + requiredSet[r.(string)] = true + } + assert.True(t, requiredSet["questions"], "questions should be required") +} + +// TestAskUserQuestion_RejectsLegacyPromptShape ensures payloads using the old +// flat shape return a validation error rather than silently dropping the call. +func TestAskUserQuestion_RejectsLegacyPromptShape(t *testing.T) { + backend := &testBackend{} + s := newTaskModeServer(t, backend, "task-current") + + result := callTool(t, s, "ask_user_question_kandev", map[string]interface{}{ + "prompt": "Which database?", + "options": []map[string]interface{}{ + {"label": "Postgres", "description": "Relational"}, + {"label": "Mongo", "description": "Document"}, + }, + }) + assert.True(t, result.IsError, "legacy flat shape must surface a validation error") +} + +// TestAskUserQuestion_SingleQuestion_PayloadShape exercises the simplest +// happy path: one question with two options. +func TestAskUserQuestion_SingleQuestion_PayloadShape(t *testing.T) { + backend := &testBackend{ + response: map[string]interface{}{ + "answers": []interface{}{ + map[string]interface{}{ + "question_id": "q1", + "selected_options": []interface{}{"q1_opt1"}, + }, + }, + }, + } + s := newTaskModeServer(t, backend, "task-current") + + result := callTool(t, s, "ask_user_question_kandev", map[string]interface{}{ + "questions": []map[string]interface{}{ + { + "id": "q1", + "prompt": "Which database?", + "options": []map[string]interface{}{ + {"label": "Postgres", "description": "Relational"}, + {"label": "Mongo", "description": "Document"}, + }, + }, + }, + }) + require.False(t, result.IsError, "single-question call should succeed") + + assert.Equal(t, ws.ActionMCPAskUserQuestion, backend.lastAction) + payload, ok := backend.lastPayload.(map[string]interface{}) + require.True(t, ok) + questions, ok := payload["questions"].([]map[string]interface{}) + require.True(t, ok, "questions should be normalized to []map") + require.Len(t, questions, 1) + assert.Equal(t, "q1", questions[0]["id"]) + + // Result text should be a JSON map keyed by question id. + require.NotEmpty(t, result.Content) + textBlock, ok := result.Content[0].(mcplib.TextContent) + require.True(t, ok) + var parsed map[string]interface{} + require.NoError(t, json.Unmarshal([]byte(textBlock.Text), &parsed)) + q1 := parsed["q1"].(map[string]interface{}) + assert.Equal(t, "q1_opt1", q1["selected_option"]) +} + +// TestAskUserQuestion_MultiQuestion_BuildsMapResponse covers the full multi-q +// path: agent receives a JSON map keyed by every question id. +func TestAskUserQuestion_MultiQuestion_BuildsMapResponse(t *testing.T) { + backend := &testBackend{ + response: map[string]interface{}{ + "answers": []interface{}{ + map[string]interface{}{ + "question_id": "db", + "selected_options": []interface{}{"db_opt1"}, + }, + map[string]interface{}{ + "question_id": "migration", + "custom_text": "migrate all but flag rows older than 2 years", + }, + map[string]interface{}{ + "question_id": "approach", + "selected_options": []interface{}{"approach_opt2"}, + }, + }, + }, + } + s := newTaskModeServer(t, backend, "task-current") + + result := callTool(t, s, "ask_user_question_kandev", map[string]interface{}{ + "questions": []map[string]interface{}{ + { + "id": "db", + "prompt": "Which database?", + "options": []map[string]interface{}{ + {"label": "Postgres", "description": "Relational"}, + {"label": "Mongo", "description": "Document"}, + }, + }, + { + "id": "migration", + "prompt": "How to migrate?", + "options": []map[string]interface{}{ + {"label": "Migrate all", "description": "Keep everything"}, + {"label": "Fresh start", "description": "Drop existing"}, + }, + }, + { + "id": "approach", + "prompt": "Iterative or atomic?", + "options": []map[string]interface{}{ + {"label": "Iterative", "description": "Step by step"}, + {"label": "Atomic", "description": "One big change"}, + }, + }, + }, + }) + require.False(t, result.IsError) + + require.NotEmpty(t, result.Content) + textBlock, ok := result.Content[0].(mcplib.TextContent) + require.True(t, ok) + var parsed map[string]interface{} + require.NoError(t, json.Unmarshal([]byte(textBlock.Text), &parsed)) + + require.Len(t, parsed, 3) + assert.Equal(t, "db_opt1", parsed["db"].(map[string]interface{})["selected_option"]) + assert.Equal(t, "migrate all but flag rows older than 2 years", parsed["migration"].(map[string]interface{})["custom_text"]) + assert.Equal(t, "approach_opt2", parsed["approach"].(map[string]interface{})["selected_option"]) +} + +// TestAskUserQuestion_RejectsTooManyQuestions caps the bundle at 4 questions — +// past that the agent should batch the work differently. +func TestAskUserQuestion_RejectsTooManyQuestions(t *testing.T) { + backend := &testBackend{} + s := newTaskModeServer(t, backend, "task-current") + + makeQ := func(id string) map[string]interface{} { + return map[string]interface{}{ + "id": id, + "prompt": "anything?", + "options": []map[string]interface{}{ + {"label": "yes", "description": "y"}, + {"label": "no", "description": "n"}, + }, + } + } + + result := callTool(t, s, "ask_user_question_kandev", map[string]interface{}{ + "questions": []map[string]interface{}{ + makeQ("q1"), makeQ("q2"), makeQ("q3"), makeQ("q4"), makeQ("q5"), + }, + }) + assert.True(t, result.IsError, "more than 4 questions must be rejected") +} + +// TestAskUserQuestion_RejectsTooFewOptions guards against degenerate payloads +// (a question with a single option is just an alert, not a real question). +func TestAskUserQuestion_RejectsTooFewOptions(t *testing.T) { + backend := &testBackend{} + s := newTaskModeServer(t, backend, "task-current") + + result := callTool(t, s, "ask_user_question_kandev", map[string]interface{}{ + "questions": []map[string]interface{}{ + { + "id": "q1", + "prompt": "?", + "options": []map[string]interface{}{ + {"label": "only one", "description": "lonely"}, + }, + }, + }, + }) + assert.True(t, result.IsError, "single-option question must be rejected") +} + +// TestAskUserQuestion_RejectionPath returns a friendly text result when the +// user skipped the bundle. +func TestAskUserQuestion_RejectionPath(t *testing.T) { + backend := &testBackend{ + response: map[string]interface{}{ + "rejected": true, + "reject_reason": "User skipped", + }, + } + s := newTaskModeServer(t, backend, "task-current") + + result := callTool(t, s, "ask_user_question_kandev", map[string]interface{}{ + "questions": []map[string]interface{}{ + { + "id": "q1", + "prompt": "Which?", + "options": []map[string]interface{}{ + {"label": "Yes", "description": "y"}, + {"label": "No", "description": "n"}, + }, + }, + }, + }) + require.False(t, result.IsError) + + textBlock, ok := result.Content[0].(mcplib.TextContent) + require.True(t, ok) + assert.Contains(t, textBlock.Text, "rejected") + assert.Contains(t, textBlock.Text, "User skipped") +} diff --git a/apps/backend/internal/mcp/server/handlers.go b/apps/backend/internal/mcp/server/handlers.go index a7d2267540..379d6e9fb5 100644 --- a/apps/backend/internal/mcp/server/handlers.go +++ b/apps/backend/internal/mcp/server/handlers.go @@ -11,10 +11,21 @@ import ( "go.uber.org/zap" ) -// promptArg is the name of the MCP tool argument that carries the user-facing -// prompt text. Repeated across tool handlers; pulled out here so goconst stays -// happy and renames stay safe. -const promptArg = "prompt" +// Argument-name constants used across the ask_user_question_kandev handler. +// Pulled out so goconst stays happy and renames stay safe. +const ( + promptArg = "prompt" + questionsArg = "questions" + optionsArg = "options" + idArg = "id" + titleArg = "title" + labelArg = "label" + descriptionArg = "description" + optionIDFieldName = "option_id" + questionIDFieldKey = "question_id" + answeredFieldKey = "answered" + rejectedFieldKey = "rejected" +) func (s *Server) listWorkspacesHandler() server.ToolHandlerFunc { return func(ctx context.Context, req mcp.CallToolRequest) (*mcp.CallToolResult, error) { @@ -291,26 +302,15 @@ func copyOptionalMessageTypesArg(payload map[string]interface{}, req mcp.CallToo func (s *Server) askUserQuestionHandler() server.ToolHandlerFunc { return func(ctx context.Context, req mcp.CallToolRequest) (*mcp.CallToolResult, error) { - prompt, err := req.RequireString(promptArg) - if err != nil { - return mcp.NewToolResultError("prompt is required"), nil - } - - options, errResult := parseQuestionOptions(req) + questions, errResult := parseQuestions(req) if errResult != nil { return errResult, nil } questionCtx := req.GetString("context", "") - question := map[string]interface{}{ - "id": "q1", - "title": "Question", - promptArg: prompt, - "options": options, - } payload := map[string]interface{}{ "session_id": s.sessionID, - "question": question, + questionsArg: questions, "context": questionCtx, } @@ -327,90 +327,208 @@ func (s *Server) askUserQuestionHandler() server.ToolHandlerFunc { return mcp.NewToolResultError(err.Error()), nil } - return extractQuestionAnswer(result), nil + return extractQuestionAnswers(result, questions), nil } } -// parseQuestionOptions extracts and validates the "options" argument from the request. -// Returns (options, nil) on success or (nil, *mcp.CallToolResult) on validation failure. -func parseQuestionOptions(req mcp.CallToolRequest) ([]map[string]interface{}, *mcp.CallToolResult) { - args := req.GetArguments() - optionsRaw, ok := args["options"] - if !ok { - return nil, mcp.NewToolResultError("options is required") - } - - optionsJSON, err := json.Marshal(optionsRaw) - if err != nil { - return nil, mcp.NewToolResultError(fmt.Sprintf("failed to parse options: %v", err)) - } - - var options []map[string]interface{} - if err := json.Unmarshal(optionsJSON, &options); err != nil { - return nil, mcp.NewToolResultError("options must be an array of objects with 'label' and 'description' fields. Example: [{\"label\": \"Option A\", \"description\": \"Description of option A\"}]") - } - - if len(options) < 2 { - return nil, mcp.NewToolResultError("options must contain at least 2 choices") - } - if len(options) > 6 { - return nil, mcp.NewToolResultError("options must contain at most 6 choices") - } - - if errResult := validateAndNormalizeOptions(options); errResult != nil { +// parseQuestions extracts and validates the "questions" array from the request. +// Returns the normalized question payloads (with auto-assigned ids) on success +// or a tool-result error describing the first validation failure. +func parseQuestions(req mcp.CallToolRequest) ([]map[string]interface{}, *mcp.CallToolResult) { + questions, errResult := decodeQuestionsArg(req) + if errResult != nil { return nil, errResult } + seenIDs := map[string]bool{} + for i, q := range questions { + if errResult := normalizeAndValidateQuestion(q, i, seenIDs); errResult != nil { + return nil, errResult + } + questions[i] = q + } + return questions, nil +} + +// decodeQuestionsArg unmarshals the raw "questions" argument and enforces +// the bundle-size invariants (1..4 questions). +func decodeQuestionsArg(req mcp.CallToolRequest) ([]map[string]interface{}, *mcp.CallToolResult) { + args := req.GetArguments() + questionsRaw, ok := args[questionsArg] + if !ok { + return nil, mcp.NewToolResultError("questions is required (array of 1-4 question objects)") + } + questionsJSON, err := json.Marshal(questionsRaw) + if err != nil { + return nil, mcp.NewToolResultError(fmt.Sprintf("failed to parse questions: %v", err)) + } + var questions []map[string]interface{} + if err := json.Unmarshal(questionsJSON, &questions); err != nil { + return nil, mcp.NewToolResultError(`questions must be an array of objects with "prompt" and "options". Example: [{"prompt": "...", "options": [{"label": "A", "description": "..."}, {"label": "B", "description": "..."}]}]`) + } + if len(questions) < 1 { + return nil, mcp.NewToolResultError("questions must contain at least 1 question") + } + if len(questions) > 4 { + return nil, mcp.NewToolResultError("questions must contain at most 4 questions") + } + return questions, nil +} + +// normalizeAndValidateQuestion mutates a question payload in place: assigns a +// default id, parses options, and reports the first validation failure. +func normalizeAndValidateQuestion(q map[string]interface{}, index int, seenIDs map[string]bool) *mcp.CallToolResult { + prompt, hasPrompt := q[promptArg].(string) + if !hasPrompt || prompt == "" { + return mcp.NewToolResultError(fmt.Sprintf("question %d is missing required 'prompt' field", index+1)) + } + id, _ := q[idArg].(string) + if id == "" { + id = fmt.Sprintf("q%d", index+1) + q[idArg] = id + } + if seenIDs[id] { + return mcp.NewToolResultError(fmt.Sprintf("question %d has duplicate id %q", index+1, id)) + } + seenIDs[id] = true + + options, errResult := decodeOptionsForQuestion(q, index) + if errResult != nil { + return errResult + } + if errResult := validateAndNormalizeOptions(options, index+1); errResult != nil { + return errResult + } + q[optionsArg] = options + return nil +} + +func decodeOptionsForQuestion(q map[string]interface{}, index int) ([]map[string]interface{}, *mcp.CallToolResult) { + optionsRaw, hasOptions := q[optionsArg] + if !hasOptions { + return nil, mcp.NewToolResultError(fmt.Sprintf("question %d is missing required 'options' field", index+1)) + } + optionsJSON, err := json.Marshal(optionsRaw) + if err != nil { + return nil, mcp.NewToolResultError(fmt.Sprintf("question %d: failed to parse options: %v", index+1, err)) + } + var options []map[string]interface{} + if err := json.Unmarshal(optionsJSON, &options); err != nil { + return nil, mcp.NewToolResultError(fmt.Sprintf("question %d: options must be an array of objects with 'label' and 'description' fields", index+1)) + } + if len(options) < 2 { + return nil, mcp.NewToolResultError(fmt.Sprintf("question %d must have at least 2 options", index+1)) + } + if len(options) > 6 { + return nil, mcp.NewToolResultError(fmt.Sprintf("question %d must have at most 6 options", index+1)) + } return options, nil } // validateAndNormalizeOptions checks each option for required fields and assigns a default option_id. -func validateAndNormalizeOptions(options []map[string]interface{}) *mcp.CallToolResult { +func validateAndNormalizeOptions(options []map[string]interface{}, questionNum int) *mcp.CallToolResult { for i, opt := range options { - label, hasLabel := opt["label"].(string) + label, hasLabel := opt[labelArg].(string) if !hasLabel || label == "" { - return mcp.NewToolResultError(fmt.Sprintf("option %d is missing required 'label' field (1-5 words describing the choice)", i+1)) + return mcp.NewToolResultError(fmt.Sprintf("question %d option %d is missing required 'label' field (1-5 words describing the choice)", questionNum, i+1)) } - description, hasDesc := opt["description"].(string) + description, hasDesc := opt[descriptionArg].(string) if !hasDesc || description == "" { - return mcp.NewToolResultError(fmt.Sprintf("option %d is missing required 'description' field (explanation of what this option means)", i+1)) + return mcp.NewToolResultError(fmt.Sprintf("question %d option %d is missing required 'description' field (explanation of what this option means)", questionNum, i+1)) } // Generate option_id if not provided - if _, hasID := opt["option_id"].(string); !hasID { - opt["option_id"] = fmt.Sprintf("opt_%d", i+1) + if _, hasID := opt[optionIDFieldName].(string); !hasID { + opt[optionIDFieldName] = fmt.Sprintf("q%d_opt%d", questionNum, i+1) } } return nil } -// extractQuestionAnswer converts the backend question response into an MCP tool result. -func extractQuestionAnswer(result map[string]interface{}) *mcp.CallToolResult { - if answer, ok := result["answer"]; ok { - if res := extractAnswerMap(answer); res != nil { - return res +// extractQuestionAnswers converts the backend response into a JSON tool result +// keyed by question id. The shape is consistent across happy / rejected paths +// so the agent can always parse the response as a JSON object — rejected +// bundles emit an envelope { "rejected": true, "reject_reason": "..." } as +// well as per-question stub entries. +func extractQuestionAnswers(result map[string]interface{}, questions []map[string]interface{}) *mcp.CallToolResult { + rejected, _ := result["rejected"].(bool) + rejectReason, _ := result["reject_reason"].(string) + + out := make(map[string]interface{}, len(questions)+2) + if rejected { + out[rejectedFieldKey] = true + if rejectReason != "" { + out["reject_reason"] = rejectReason } } - if rejected, ok := result["rejected"].(bool); ok && rejected { - reason, _ := result["reject_reason"].(string) - return mcp.NewToolResultText(fmt.Sprintf("User rejected the question: %s", reason)) + + answers, _ := result["answers"].([]interface{}) + answersByID := make(map[string]map[string]interface{}, len(answers)) + for _, raw := range answers { + ans, ok := raw.(map[string]interface{}) + if !ok { + continue + } + qid, _ := ans[questionIDFieldKey].(string) + if qid == "" { + continue + } + answersByID[qid] = simplifyAnswer(ans, rejected) } - data, _ := json.MarshalIndent(result, "", " ") + + for _, q := range questions { + qid, _ := q[idArg].(string) + if qid == "" { + continue + } + if entry, ok := answersByID[qid]; ok { + out[qid] = entry + continue + } + stub := map[string]interface{}{answeredFieldKey: false} + if rejected { + stub[rejectedFieldKey] = true + } + out[qid] = stub + } + + if len(out) == 0 { + // Nothing matched by id — surface the raw payload so the agent can still inspect it. + data, _ := json.MarshalIndent(result, "", " ") + return mcp.NewToolResultText(string(data)) + } + + data, _ := json.MarshalIndent(out, "", " ") return mcp.NewToolResultText(string(data)) } -// extractAnswerMap inspects an answer map for selected options or custom text. -func extractAnswerMap(answer interface{}) *mcp.CallToolResult { - answerMap, ok := answer.(map[string]interface{}) - if !ok { - return nil +// simplifyAnswer normalizes the answer map. Single-choice per question, but +// the user can also type a custom text alongside an option pick — we preserve +// both fields so the agent gets the full context. When the bundle was +// rejected we annotate the entry to make the partial-answer scenario explicit. +// +// Output shape (one or more keys may be set): +// +// {"selected_option": ""} +// {"custom_text": ""} +// {"selected_option": "", "custom_text": ""} +// {"answered": false} +func simplifyAnswer(ans map[string]interface{}, rejected bool) map[string]interface{} { + out := map[string]interface{}{} + if selected, ok := ans["selected_options"].([]interface{}); ok && len(selected) > 0 { + if first, ok := selected[0].(string); ok && first != "" { + out["selected_option"] = first + } } - if selectedOptions, ok := answerMap["selected_options"].([]interface{}); ok && len(selectedOptions) > 0 { - return mcp.NewToolResultText(fmt.Sprintf("User selected: %v", selectedOptions[0])) + if customText, ok := ans["custom_text"].(string); ok && customText != "" { + out["custom_text"] = customText } - if customText, ok := answerMap["custom_text"].(string); ok && customText != "" { - return mcp.NewToolResultText(fmt.Sprintf("User answered: %s", customText)) + if rejected && len(out) == 0 { + return map[string]interface{}{answeredFieldKey: false, rejectedFieldKey: true} } - return nil + if len(out) == 0 { + return map[string]interface{}{answeredFieldKey: false} + } + return out } // notifyClarificationTimeout sends a fire-and-forget notification to the backend diff --git a/apps/backend/internal/mcp/server/server.go b/apps/backend/internal/mcp/server/server.go index b15bdddbb7..2cd7e607e2 100644 --- a/apps/backend/internal/mcp/server/server.go +++ b/apps/backend/internal/mcp/server/server.go @@ -483,54 +483,70 @@ IMPORTANT: func (s *Server) registerInteractionTools() { s.mcpServer.AddTool( mcp.NewTool("ask_user_question_kandev", - mcp.WithDescription(`Ask the user a clarifying question with multiple choice options. + mcp.WithDescription(`Ask the user one or more clarifying questions in a single tool call. -Use this tool when you need user input to proceed. The user will see the prompt and can select one of the options or provide a custom text response. +Use this tool when you need user input to proceed. Bundle related questions +together in one call so the user answers them all in one back-and-forth instead +of sequential round-trips. Each question is rendered as its own card; the user +selects an option or provides a custom text response per question, and the +agent receives a map keyed by question id once every question has been answered. -IMPORTANT: Each option must be a concrete, actionable choice - NOT meta-text like "Answer questions below". - -Options format - array of objects with: -- "label": Short text (1-5 words) shown as the clickable option -- "description": Brief explanation of what this option means +IMPORTANT: +- Provide 1 to 4 questions per call. +- Each question must have 2 to 6 concrete, actionable options. +- Each option must have a short "label" (1-5 words) and a "description" + explaining what selecting it means. NEVER use meta-text like "Answer below". +- Only call this tool when you genuinely need information you cannot infer. Example usage: { - "prompt": "Which database should I use for this project?", - "options": [ - {"label": "PostgreSQL", "description": "Relational database, good for complex queries"}, - {"label": "MongoDB", "description": "Document database, flexible schema"}, - {"label": "SQLite", "description": "Embedded database, simple setup"} + "questions": [ + { + "id": "db", + "prompt": "Which database should I use for this project?", + "options": [ + {"label": "PostgreSQL", "description": "Relational, good for complex queries"}, + {"label": "MongoDB", "description": "Document database, flexible schema"}, + {"label": "SQLite", "description": "Embedded, simple setup"} + ] + }, + { + "id": "migration", + "prompt": "How should I handle the existing user data during migration?", + "options": [ + {"label": "Migrate all", "description": "Keep all existing records"}, + {"label": "Archive old", "description": "Archive records older than 1 year"}, + {"label": "Fresh start", "description": "Delete existing data and start fresh"} + ] + } ], - "context": "The project requires storing user profiles and relationships between entities." + "context": "Backend redesign — picking the persistence layer and migration policy together." } -Another example: +The response is a JSON object keyed by each question id. Each entry may include +"selected_option" (the option_id the user picked), "custom_text" (the user's +free-form answer; can co-exist with a selected option), or "answered": false +when the user did not respond to that question. When the user skipped the entire +bundle, the envelope also carries "rejected": true and an optional +"reject_reason". Example success response: { - "prompt": "How should I handle the existing user data during migration?", - "options": [ - {"label": "Migrate all", "description": "Keep all existing records"}, - {"label": "Archive old", "description": "Archive records older than 1 year"}, - {"label": "Fresh start", "description": "Delete existing data and start fresh"} - ] + "db": {"selected_option": "q1_opt1"}, + "migration": {"custom_text": "Migrate all but flag rows older than 2 years"} +} +Example rejection: +{ + "rejected": true, + "reject_reason": "User skipped", + "db": {"answered": false, "rejected": true}, + "migration": {"answered": false, "rejected": true} }`), - mcp.WithString("prompt", mcp.Required(), mcp.Description("The question to ask the user. Be specific and clear.")), - mcp.WithArray("options", mcp.Required(), mcp.Description(`Array of option objects. Each option must have: "label" (1-5 words, the clickable choice) and "description" (explanation of the option). Provide 2-4 concrete, actionable options.`), - mcp.Items(map[string]any{ - "type": "object", - "properties": map[string]any{ - "label": map[string]any{ - "type": "string", - "description": "Short text (1-5 words) shown as the clickable option", - }, - "description": map[string]any{ - "type": "string", - "description": "Brief explanation of what this option means", - }, - }, - "required": []string{"label", "description"}, - }), + mcp.WithArray(questionsArg, mcp.Required(), + mcp.Description(`Array of 1-4 question objects. Each question must have a "prompt" (the question text) and an "options" array (2-6 entries with label + description). Optional fields: "id" (stable identifier in the response map; auto-generated if omitted), "title" (≤12 chars short label).`), + mcp.MinItems(1), + mcp.MaxItems(4), + mcp.Items(buildQuestionSchemaItem()), ), - mcp.WithString("context", mcp.Description("Optional background information to help the user understand why you're asking this question.")), + mcp.WithString("context", mcp.Description("Optional shared background information to help the user understand why you're asking these questions.")), ), s.wrapHandler("ask_user_question_kandev", s.askUserQuestionHandler()), ) @@ -570,3 +586,43 @@ func (s *Server) registerPlanTools() { s.wrapHandler("delete_task_plan_kandev", s.deleteTaskPlanHandler()), ) } + +// buildQuestionSchemaItem describes the shape of a single question object in +// the ask_user_question_kandev tool schema. Hoisted out of registerInteractionTools +// to keep the registration body short and to deduplicate the JSON-schema +// keyword strings (linter goconst rules). +func buildQuestionSchemaItem() map[string]any { + const typeKey = "type" + const propsKey = "properties" + const reqKey = "required" + const objType = "object" + const stringType = "string" + + str := func(desc string) map[string]any { + return map[string]any{typeKey: stringType, descriptionArg: desc} + } + + return map[string]any{ + typeKey: objType, + propsKey: map[string]any{ + idArg: str("Stable identifier used as the key in the response map. Auto-assigned (q1, q2, ...) if omitted."), + titleArg: str("Optional short label (≤12 chars) shown above the prompt."), + promptArg: str("The question text shown to the user."), + optionsArg: map[string]any{ + typeKey: "array", + descriptionArg: "2-6 concrete, actionable choices.", + "minItems": 2, + "maxItems": 6, + "items": map[string]any{ + typeKey: objType, + propsKey: map[string]any{ + labelArg: str("Short text (1-5 words) shown as the clickable option."), + descriptionArg: str("Brief explanation of what this option means."), + }, + reqKey: []string{labelArg, descriptionArg}, + }, + }, + }, + reqKey: []string{promptArg, optionsArg}, + } +} diff --git a/apps/backend/internal/orchestrator/event_handlers_clarification.go b/apps/backend/internal/orchestrator/event_handlers_clarification.go index 54e69b7514..3cb81ccb79 100644 --- a/apps/backend/internal/orchestrator/event_handlers_clarification.go +++ b/apps/backend/internal/orchestrator/event_handlers_clarification.go @@ -295,15 +295,28 @@ func (s *Service) cancelAllClarificationWatchdogs() { } // buildClarificationPrompt constructs the resume prompt from a clarification answer. +// Handles both single- and multi-question bundles: when data.Question contains +// newlines it is treated as a pre-formatted multi-line summary and embedded +// as-is rather than quoted. func buildClarificationPrompt(data clarificationAnsweredData) string { + multiQuestion := strings.Contains(data.Question, "\n") + if data.Rejected { reason := data.RejectReason if reason == "" { reason = "No reason provided" } + if multiQuestion { + return fmt.Sprintf("The user declined to answer your questions:\n%s\nReason: %s\nPlease continue without this information.", + data.Question, reason) + } return fmt.Sprintf("The user declined to answer your question: %q\nReason: %s\nPlease continue without this information.", data.Question, reason) } + if multiQuestion { + return fmt.Sprintf("You previously asked the user:\n%s\n\n%s\nPlease continue with this information.", + data.Question, data.AnswerText) + } return fmt.Sprintf("You previously asked the user: %q\n%s\nPlease continue with this information.", data.Question, data.AnswerText) } diff --git a/apps/backend/internal/orchestrator/executor/executor_mocks_test.go b/apps/backend/internal/orchestrator/executor/executor_mocks_test.go index 5286dcc84f..3ffa3f42dd 100644 --- a/apps/backend/internal/orchestrator/executor/executor_mocks_test.go +++ b/apps/backend/internal/orchestrator/executor/executor_mocks_test.go @@ -421,6 +421,12 @@ func (m *mockRepository) GetMessageByPendingID(ctx context.Context, sessionID, p func (m *mockRepository) FindMessageByPendingID(ctx context.Context, pendingID string) (*models.Message, error) { return nil, nil } +func (m *mockRepository) FindMessagesByPendingID(ctx context.Context, pendingID string) ([]*models.Message, error) { + return nil, nil +} +func (m *mockRepository) FindMessageByPendingIDAndQuestion(ctx context.Context, sessionID, pendingID, questionID string) (*models.Message, error) { + return nil, nil +} func (m *mockRepository) UpdateMessage(ctx context.Context, message *models.Message) error { return nil } diff --git a/apps/backend/internal/task/handlers/process_handlers_test.go b/apps/backend/internal/task/handlers/process_handlers_test.go index 3ff59ddf33..20d694d268 100644 --- a/apps/backend/internal/task/handlers/process_handlers_test.go +++ b/apps/backend/internal/task/handlers/process_handlers_test.go @@ -139,6 +139,12 @@ func (m *mockRepository) GetMessageByPendingID(ctx context.Context, sessionID, p func (m *mockRepository) FindMessageByPendingID(ctx context.Context, pendingID string) (*models.Message, error) { return nil, nil } +func (m *mockRepository) FindMessagesByPendingID(ctx context.Context, pendingID string) ([]*models.Message, error) { + return nil, nil +} +func (m *mockRepository) FindMessageByPendingIDAndQuestion(ctx context.Context, sessionID, pendingID, questionID string) (*models.Message, error) { + return nil, nil +} func (m *mockRepository) UpdateMessage(ctx context.Context, message *models.Message) error { return nil } diff --git a/apps/backend/internal/task/repository/interface.go b/apps/backend/internal/task/repository/interface.go index 1585ca3e2e..7d8ea057bd 100644 --- a/apps/backend/internal/task/repository/interface.go +++ b/apps/backend/internal/task/repository/interface.go @@ -66,6 +66,8 @@ type MessageRepository interface { GetMessageByToolCallID(ctx context.Context, sessionID, toolCallID string) (*models.Message, error) GetMessageByPendingID(ctx context.Context, sessionID, pendingID string) (*models.Message, error) FindMessageByPendingID(ctx context.Context, pendingID string) (*models.Message, error) + FindMessagesByPendingID(ctx context.Context, pendingID string) ([]*models.Message, error) + FindMessageByPendingIDAndQuestion(ctx context.Context, sessionID, pendingID, questionID string) (*models.Message, error) UpdateMessage(ctx context.Context, message *models.Message) error ListMessages(ctx context.Context, sessionID string) ([]*models.Message, error) ListMessagesPaginated(ctx context.Context, sessionID string, opts models.ListMessagesOptions) ([]*models.Message, bool, error) diff --git a/apps/backend/internal/task/repository/sqlite/message.go b/apps/backend/internal/task/repository/sqlite/message.go index 77db8a51db..e832a33a2b 100644 --- a/apps/backend/internal/task/repository/sqlite/message.go +++ b/apps/backend/internal/task/repository/sqlite/message.go @@ -312,8 +312,10 @@ func (r *Repository) GetMessageByPendingID(ctx context.Context, sessionID, pendi return r.getMessageByMetadataField(ctx, sessionID, "pending_id", pendingID, "") } -// FindMessageByPendingID finds a message by pending_id alone (without requiring session ID). +// FindMessageByPendingID finds the most-recent message by pending_id alone. // This is useful when we only have the pending ID (e.g., from expired clarification responses). +// For multi-question clarification requests, prefer FindMessagesByPendingID to retrieve +// every related message in one shot. func (r *Repository) FindMessageByPendingID(ctx context.Context, pendingID string) (*models.Message, error) { message := &models.Message{} var requestsInput int @@ -338,6 +340,62 @@ func (r *Repository) FindMessageByPendingID(ctx context.Context, pendingID strin return message, nil } +// FindMessagesByPendingID returns every message that carries the given pending_id +// in its metadata, ordered by creation time (oldest first). Multi-question +// clarification requests emit one message per question, all sharing the same +// pending_id; this lookup lets the canceller / status-update path touch all of +// them without N round-trips. +func (r *Repository) FindMessagesByPendingID(ctx context.Context, pendingID string) ([]*models.Message, error) { + drv := r.ro.DriverName() + query := fmt.Sprintf(` + SELECT id, task_session_id, task_id, turn_id, author_type, author_id, content, requests_input, type, metadata, created_at + FROM task_session_messages WHERE %s = ? + ORDER BY created_at ASC + `, dialect.JSONExtract(drv, "metadata", "pending_id")) + rows, err := r.ro.QueryContext(ctx, r.ro.Rebind(query), pendingID) + if err != nil { + return nil, err + } + defer func() { _ = rows.Close() }() + result, _, err := scanMessageRows(rows, 0) + return result, err +} + +// FindMessageByPendingIDAndQuestion finds the message for a specific (pending_id, +// question_id) pair within a session. Used to flip per-question status (answered / +// rejected) on multi-question clarification bundles. The persistence layer stores +// the question id at metadata.question_id (flat, alongside metadata.question) so +// the JSON path lookup works on both SQLite and Postgres. +func (r *Repository) FindMessageByPendingIDAndQuestion(ctx context.Context, sessionID, pendingID, questionID string) (*models.Message, error) { + drv := r.ro.DriverName() + query := fmt.Sprintf(` + SELECT id, task_session_id, task_id, turn_id, author_type, author_id, content, requests_input, type, metadata, created_at + FROM task_session_messages + WHERE task_session_id = ? AND %s = ? AND %s = ? + ORDER BY created_at ASC LIMIT 1 + `, + dialect.JSONExtract(drv, "metadata", "pending_id"), + dialect.JSONExtract(drv, "metadata", "question_id"), + ) + message := &models.Message{} + var requestsInput int + var messageType string + var metadataJSON string + err := r.ro.QueryRowContext(ctx, r.ro.Rebind(query), sessionID, pendingID, questionID).Scan( + &message.ID, &message.TaskSessionID, &message.TaskID, &message.TurnID, &message.AuthorType, &message.AuthorID, + &message.Content, &requestsInput, &messageType, &metadataJSON, &message.CreatedAt, + ) + if err != nil { + return nil, err + } + message.RequestsInput = requestsInput == 1 + message.Type = models.MessageType(messageType) + if metadataJSON != "" { + _ = json.Unmarshal([]byte(metadataJSON), &message.Metadata) + } + return message, nil +} + // CompletePendingToolCallsForTurn marks all non-terminal tool call messages for a turn as "complete". // This is a safety net to ensure no tool calls remain stuck in a non-terminal state (pending, // running, in_progress, etc.) after a turn completes. diff --git a/apps/backend/internal/task/service/service_messages.go b/apps/backend/internal/task/service/service_messages.go index 8be14b9867..d126857a14 100644 --- a/apps/backend/internal/task/service/service_messages.go +++ b/apps/backend/internal/task/service/service_messages.go @@ -584,19 +584,19 @@ func (s *Service) UpdatePermissionMessage(ctx context.Context, sessionID, pendin return nil } -// UpdateClarificationMessage updates a clarification request message's status and response. -// It includes retry logic to handle race conditions. -// The answers parameter should be a slice of answer objects with question_id, selected_options, and custom_text. -func (s *Service) UpdateClarificationMessage(ctx context.Context, sessionID, pendingID, status string, answers interface{}) error { +// UpdateClarificationMessageForQuestion updates a single clarification message +// (identified by pending_id + question_id) with the new status and the answer +// payload. Used by both single- and multi-question clarification bundles since +// each question lives in its own chat message. +func (s *Service) UpdateClarificationMessageForQuestion(ctx context.Context, sessionID, pendingID, questionID, status string, answer interface{}) error { const maxRetries = 5 const retryDelay = 100 * time.Millisecond var message *models.Message var err error - // Retry loop to handle race condition for attempt := 0; attempt < maxRetries; attempt++ { - message, err = s.messages.GetMessageByPendingID(ctx, sessionID, pendingID) + message, err = s.messages.FindMessageByPendingIDAndQuestion(ctx, sessionID, pendingID, questionID) if err == nil { break } @@ -609,6 +609,7 @@ func (s *Service) UpdateClarificationMessage(ctx context.Context, sessionID, pen s.logger.Debug("clarification message not found, retrying", zap.String("session_id", sessionID), zap.String("pending_id", pendingID), + zap.String("question_id", questionID), zap.Int("attempt", attempt+1), zap.Int("max_retries", maxRetries)) time.Sleep(retryDelay) @@ -619,6 +620,7 @@ func (s *Service) UpdateClarificationMessage(ctx context.Context, sessionID, pen s.logger.Warn("clarification message not found for update after retries", zap.String("session_id", sessionID), zap.String("pending_id", pendingID), + zap.String("question_id", questionID), zap.Int("retries", maxRetries), zap.Error(err)) return err @@ -628,24 +630,25 @@ func (s *Service) UpdateClarificationMessage(ctx context.Context, sessionID, pen message.Metadata = make(map[string]interface{}) } message.Metadata["status"] = status - if answers != nil { - message.Metadata["response"] = answers + if answer != nil { + message.Metadata["response"] = answer } if err := s.messages.UpdateMessage(ctx, message); err != nil { s.logger.Error("failed to update clarification message", zap.String("message_id", message.ID), zap.String("pending_id", pendingID), + zap.String("question_id", questionID), zap.Error(err)) return err } - // Publish message.updated event s.publishMessageEvent(ctx, events.MessageUpdated, message) s.logger.Info("clarification message updated", zap.String("message_id", message.ID), zap.String("pending_id", pendingID), + zap.String("question_id", questionID), zap.String("status", status)) return nil diff --git a/apps/web/components/quick-chat/quick-chat-content.tsx b/apps/web/components/quick-chat/quick-chat-content.tsx index ec2cca3234..db8850680d 100644 --- a/apps/web/components/quick-chat/quick-chat-content.tsx +++ b/apps/web/components/quick-chat/quick-chat-content.tsx @@ -74,7 +74,14 @@ export const QuickChatContent = memo(function QuickChatContent({ handleCancelQueue, handleQueueEditComplete, } = state; - const { taskId, pendingClarification, isQueued, queuedMessage, updateQueueContent } = panelState; + const { + taskId, + pendingClarification, + pendingClarificationGroup, + isQueued, + queuedMessage, + updateQueueContent, + } = panelState; useEffect(() => { const timer = setTimeout(() => chatInputRef.current?.focusInput(), 50); @@ -111,7 +118,7 @@ export const QuickChatContent = memo(function QuickChatContent({ {pendingClarification && (
diff --git a/apps/web/components/task/chat/clarification-input-overlay.tsx b/apps/web/components/task/chat/clarification-input-overlay.tsx index 49ea2d2837..f286bfd450 100644 --- a/apps/web/components/task/chat/clarification-input-overlay.tsx +++ b/apps/web/components/task/chat/clarification-input-overlay.tsx @@ -1,10 +1,8 @@ "use client"; -import { useCallback, useState } from "react"; +import { useCallback, useEffect, useMemo, useRef, useState } from "react"; import { IconX, IconMessageQuestion, IconInfoCircle } from "@tabler/icons-react"; import ReactMarkdown from "react-markdown"; -import { cn } from "@/lib/utils"; -import { getBackendConfig } from "@/lib/config"; import { markdownComponents, remarkPlugins } from "@/components/shared/markdown-components"; import type { Message, @@ -12,262 +10,392 @@ import type { ClarificationAnswer, ClarificationQuestion, } from "@/lib/types/http"; - -const RESULT_EXPIRED = "expired"; +import { useClarificationGroup } from "@/hooks/domains/session/use-clarification-group"; +import { + ClarificationCarouselNav, + ClarificationCustomInput, + ClarificationOptions, + ClarificationStepper, +} from "./clarification-overlay-parts"; type ClarificationInputOverlayProps = { - message: Message; + messages: readonly Message[] | null | undefined; onResolved: () => void; }; -/** - * Post a response to the clarification endpoint. Returns true on success. - */ -async function postClarificationResponse( - pendingId: string, - body: Record, -): Promise<"ok" | "expired" | "error"> { - const { apiBaseUrl } = getBackendConfig(); - const response = await fetch(`${apiBaseUrl}/api/v1/clarification/${pendingId}/respond`, { - method: "POST", - headers: { "Content-Type": "application/json" }, - body: JSON.stringify(body), - }); - if (response.ok) return "ok"; - if (response.status === 410) { - const data = await response.json().catch(() => ({})); - console.warn("Clarification expired:", data); - return RESULT_EXPIRED; - } - console.error("Clarification request failed:", response.status, response.statusText); - return "error"; -} - -function handleClarificationResult(result: "ok" | "expired" | "error", onResolved: () => void) { - if (result === "ok" || result === RESULT_EXPIRED) onResolved(); -} - -type ClarificationOptionsProps = { +type SingleQuestionMeta = { + message: Message; + metadata: ClarificationRequestMetadata; question: ClarificationQuestion; - isSubmitting: boolean; - onSelectOption: (optionId: string) => void; + questionId: string; }; -function ClarificationOptions({ - question, - isSubmitting, - onSelectOption, -}: ClarificationOptionsProps) { - return ( -
- {question.options.map((option) => ( - - ))} -
- ); +function readSingleQuestionMeta(message: Message | null | undefined): SingleQuestionMeta | null { + if (!message) return null; + const metadata = message.metadata as ClarificationRequestMetadata | undefined; + if (!metadata?.question) return null; + const questionId = metadata.question_id ?? metadata.question.id; + if (!questionId) return null; + return { message, metadata, question: metadata.question, questionId }; } -type ClarificationCustomInputProps = { - customText: string; +function resolveQuestionMessages(messages: readonly Message[] | null | undefined): Message[] { + if (messages && messages.length > 0) return [...messages]; + return []; +} + +function sortMessagesByQuestionIndex(messages: Message[]): Message[] { + return messages.slice().sort((a, b) => { + const ai = (a.metadata as ClarificationRequestMetadata | undefined)?.question_index ?? 0; + const bi = (b.metadata as ClarificationRequestMetadata | undefined)?.question_index ?? 0; + return ai - bi; + }); +} + +type CardProps = { + meta: SingleQuestionMeta; + index: number; + total: number; + selectedOption: string | null; + customCommittedText: string | null; + customDraft: string; isSubmitting: boolean; - onChange: (text: string) => void; - onSubmit: () => void; + showAgentDisconnected: boolean; + onSelectOption: (optionId: string) => void; + onCustomDraftChange: (text: string) => void; + onSubmitCustom: (text: string) => void; }; -function ClarificationCustomInput({ - customText, - isSubmitting, - onChange, - onSubmit, -}: ClarificationCustomInputProps) { +function ClarificationCard(props: CardProps) { + const { + meta, + index, + total, + selectedOption, + customCommittedText, + customDraft, + isSubmitting, + showAgentDisconnected, + onSelectOption, + onCustomDraftChange, + onSubmitCustom, + } = props; + const { question, metadata } = meta; return ( -
- • - onChange(e.target.value)} - disabled={isSubmitting} - data-testid="clarification-input" - className="flex-1 text-sm bg-transparent placeholder:text-muted-foreground focus:outline-none" - onKeyDown={(e) => { - if (e.key === "Enter" && !e.shiftKey && customText.trim()) { - e.preventDefault(); - onSubmit(); - } - }} +
+
+ + {total > 1 && ( + + Question {index + 1} of {total} + + )} + {metadata.question.title && ( + + {total > 1 ? "· " : ""} + {metadata.question.title} + + )} +
+
+ + {question.prompt} + +
+ + {showAgentDisconnected && ( +
+ + The agent has moved on. Your response will be sent as a new message. +
+ )} +
); } -type UseClarificationHandlersParams = { - metadata: ClarificationRequestMetadata | undefined; - isSubmitting: boolean; - setIsSubmitting: React.Dispatch>; - customText: string; - onResolved: () => void; -}; - -function useClarificationHandlers({ - metadata, - isSubmitting, - setIsSubmitting, - customText, - onResolved, -}: UseClarificationHandlersParams) { - const handleSubmitOption = useCallback( - async (optionId: string) => { - if (!metadata?.pending_id || isSubmitting) return; - setIsSubmitting(true); - try { - const answer: ClarificationAnswer = { - question_id: metadata.question.id, - selected_options: [optionId], - }; - const result = await postClarificationResponse(metadata.pending_id, { - answers: [answer], - rejected: false, - }); - handleClarificationResult(result, onResolved); - } catch (error) { - console.error("Failed to submit clarification response:", error); - } finally { - setIsSubmitting(false); - } - }, - [metadata, isSubmitting, onResolved, setIsSubmitting], - ); - - const handleSubmitCustom = useCallback(async () => { - if (!metadata?.pending_id || isSubmitting || !customText.trim()) return; - setIsSubmitting(true); - try { - const answer: ClarificationAnswer = { - question_id: metadata.question.id, - selected_options: [], - custom_text: customText.trim(), - }; - const result = await postClarificationResponse(metadata.pending_id, { - answers: [answer], - rejected: false, - }); - handleClarificationResult(result, onResolved); - } catch (error) { - console.error("Failed to submit clarification response:", error); - } finally { - setIsSubmitting(false); +function useResolveCallback( + submitState: ReturnType["submitState"], + onResolved: () => void, +) { + const last = useRef(submitState); + useEffect(() => { + if (last.current !== submitState && submitState === "ok") { + onResolved(); } - }, [metadata, isSubmitting, customText, onResolved, setIsSubmitting]); - - const handleSkip = useCallback(async () => { - if (!metadata?.pending_id || isSubmitting) return; - setIsSubmitting(true); - try { - const result = await postClarificationResponse(metadata.pending_id, { - answers: [], - rejected: true, - reject_reason: "User skipped", - }); - handleClarificationResult(result, onResolved); - } catch (error) { - console.error("Failed to skip clarification:", error); - } finally { - setIsSubmitting(false); - } - }, [metadata, isSubmitting, onResolved, setIsSubmitting]); - - return { handleSubmitOption, handleSubmitCustom, handleSkip }; + last.current = submitState; + }, [submitState, onResolved]); } -/** - * Inline clarification UI - simple numbered text options like Conductor. - */ -export function ClarificationInputOverlay({ message, onResolved }: ClarificationInputOverlayProps) { - const metadata = message.metadata as ClarificationRequestMetadata | undefined; - const [customText, setCustomText] = useState(""); - const [isSubmitting, setIsSubmitting] = useState(false); - const { handleSubmitOption, handleSubmitCustom, handleSkip } = useClarificationHandlers({ - metadata, - isSubmitting, - setIsSubmitting, - customText, - onResolved, +type CarouselShortcutArgs = { + meta: SingleQuestionMeta; + activeIndex: number; + total: number; + canSubmit: boolean; + onPick: (index: number) => void; + onPrev: () => void; + onNext: () => void; + onSkip: () => void; + onSubmit: () => void; +}; + +// shouldIgnoreShortcut filters out events that the overlay must not handle: +// keystrokes inside an input/textarea (the user is typing) and any modifier +// combo (so we don't hijack browser shortcuts like Cmd/Ctrl+1..9 for tab +// switching or Alt+ArrowLeft for back-navigation). +function shouldIgnoreShortcut(e: KeyboardEvent): boolean { + if ( + e.target instanceof HTMLElement && + (e.target.tagName === "INPUT" || e.target.tagName === "TEXTAREA") + ) { + return true; + } + return e.metaKey || e.ctrlKey || e.altKey || e.shiftKey; +} + +function CarouselKeyboardShortcuts(args: CarouselShortcutArgs) { + const optionsCount = args.meta.question.options.length; + const isLast = args.activeIndex === args.total - 1; + const { canSubmit, onPick, onPrev, onNext, onSkip, onSubmit } = args; + useEffect(() => { + const onKey = (e: KeyboardEvent) => { + if (shouldIgnoreShortcut(e)) return; + if (e.key === "Escape") { + e.preventDefault(); + onSkip(); + return; + } + if (e.key === "ArrowLeft") { + e.preventDefault(); + onPrev(); + return; + } + if (e.key === "ArrowRight") { + e.preventDefault(); + if (isLast && canSubmit) onSubmit(); + else onNext(); + return; + } + const num = Number.parseInt(e.key, 10); + if (Number.isFinite(num) && num >= 1 && num <= optionsCount) { + e.preventDefault(); + onPick(num - 1); + } + }; + window.addEventListener("keydown", onKey); + return () => window.removeEventListener("keydown", onKey); + }, [optionsCount, isLast, canSubmit, onPick, onPrev, onNext, onSkip, onSubmit]); + return null; +} + +type CarouselBodyProps = { + sortedMessages: Message[]; + group: ReturnType; + activeIndex: number; + setActiveIndex: (idx: number) => void; + customDrafts: Record; + setCustomDrafts: React.Dispatch>>; +}; + +function ClarificationCarouselBody({ + sortedMessages, + group, + activeIndex, + setActiveIndex, + customDrafts, + setCustomDrafts, +}: CarouselBodyProps) { + const total = sortedMessages.length; + const activeMessage = sortedMessages[Math.min(activeIndex, total - 1)] ?? null; + const meta = activeMessage ? readSingleQuestionMeta(activeMessage) : null; + const showAgentDisconnectedAtTop = sortedMessages.some( + (m) => (m.metadata as ClarificationRequestMetadata | undefined)?.agent_disconnected === true, + ); + const isSubmitting = group.submitState === "submitting"; + + const isSingleQuestion = total === 1; + + const allAnswered = sortedMessages.every((m) => { + const id = readSingleQuestionMeta(m)?.questionId; + return id ? Boolean(group.answers[id]) : false; }); - if (!metadata?.question) return null; + const handleSubmit = useCallback(() => { + if (allAnswered) void group.submitCollected(); + }, [allAnswered, group]); - const question = metadata.question; + if (!meta) return null; + + const stored = group.answers[meta.questionId]; + const selectedOption = + stored?.selected_options && stored.selected_options.length > 0 + ? stored.selected_options[0] + : null; + const customCommittedText = stored?.custom_text ?? null; + const draft = customDrafts[meta.questionId] ?? ""; + + // Records the new answer, then either auto-submits (single-question case + // takes the override path so the freshly recorded answer is included even + // though setState is async) or auto-advances to the next step. + const commitAnswer = (answer: ClarificationAnswer) => { + group.recordAnswer(meta.questionId, answer); + if (isSingleQuestion) { + void group.submitCollected({ [meta.questionId]: answer }); + return; + } + if (activeIndex < total - 1) { + setActiveIndex(activeIndex + 1); + } + }; + + const onSelectOption = (optionId: string) => { + commitAnswer({ + question_id: meta.questionId, + selected_options: [optionId], + }); + }; + const onSubmitCustom = (text: string) => { + if (!text.trim()) return; + commitAnswer({ + question_id: meta.questionId, + selected_options: [], + custom_text: text.trim(), + }); + }; return ( -
- + <> + + setCustomDrafts((prev) => ({ ...prev, [meta.questionId]: value })) + } + onSubmitCustom={onSubmitCustom} + /> + {!isSingleQuestion && ( + setActiveIndex(Math.max(0, activeIndex - 1))} + onNext={() => setActiveIndex(Math.min(total - 1, activeIndex + 1))} + onSubmit={handleSubmit} + canSubmit={allAnswered} + /> + )} + onSelectOption(meta.question.options[idx].option_id)} + onPrev={() => setActiveIndex(Math.max(0, activeIndex - 1))} + onNext={() => setActiveIndex(Math.min(total - 1, activeIndex + 1))} + onSkip={() => void group.skipAll("User skipped")} + onSubmit={handleSubmit} + /> + + ); +} -
-
- -
- - {question.prompt} - -
+export function ClarificationInputOverlay({ + messages, + onResolved, +}: ClarificationInputOverlayProps) { + const sortedMessages = useMemo( + () => sortMessagesByQuestionIndex(resolveQuestionMessages(messages)), + [messages], + ); + const group = useClarificationGroup(sortedMessages); + const [customDrafts, setCustomDrafts] = useState>({}); + const [rawActiveIndex, setActiveIndex] = useState(0); + // Clamp the active index to the current bundle size so late-arriving + // messages or shrunk bundles never put us out of range. + const total = sortedMessages.length; + const activeIndex = total === 0 ? 0 : Math.min(rawActiveIndex, total - 1); + + useResolveCallback(group.submitState, onResolved); + + if (sortedMessages.length === 0) return null; + const isSubmitting = group.submitState === "submitting"; + + const isAnsweredAt = (index: number) => { + const m = sortedMessages[index]; + if (!m) return false; + const id = readSingleQuestionMeta(m)?.questionId; + return id ? Boolean(group.answers[id]) : false; + }; + + return ( +
+
+
+ {total > 1 && ( + + )} + {total > 1 && ( + + {group.answeredCount} of {group.total} answered + + )}
- - {metadata.agent_disconnected && ( -
- - The agent has moved on. Your response will be sent as a new message. -
- )} - +
+
); } + +export type { ClarificationAnswer }; diff --git a/apps/web/components/task/chat/clarification-overlay-parts.tsx b/apps/web/components/task/chat/clarification-overlay-parts.tsx new file mode 100644 index 0000000000..761fa890dd --- /dev/null +++ b/apps/web/components/task/chat/clarification-overlay-parts.tsx @@ -0,0 +1,257 @@ +"use client"; + +import { IconCheck, IconCornerDownLeft, IconArrowLeft, IconArrowRight } from "@tabler/icons-react"; +import { cn } from "@/lib/utils"; +import type { ClarificationOption } from "@/lib/types/http"; + +export function stepClassName(active: boolean, answered: boolean): string { + if (active) { + return "bg-blue-500 text-white border-blue-500 shadow-[0_0_0_3px_rgba(59,130,246,0.18)]"; + } + if (answered) { + return "bg-blue-500/20 text-blue-600 border-blue-500/40 dark:text-blue-300"; + } + return "bg-muted text-muted-foreground border-border hover:bg-muted/70"; +} + +type StepperProps = { + total: number; + activeIndex: number; + isAnswered: (index: number) => boolean; + onJump: (index: number) => void; + isSubmitting: boolean; +}; + +export function ClarificationStepper({ + total, + activeIndex, + isAnswered, + onJump, + isSubmitting, +}: StepperProps) { + return ( +
+ {Array.from({ length: total }).map((_, i) => { + const answered = isAnswered(i); + const active = i === activeIndex; + return ( +
+ + {i < total - 1 && ( + + ); + })} +
+ ); +} + +type OptionListProps = { + options: ClarificationOption[]; + selectedOption: string | null; + isSubmitting: boolean; + onSelectOption: (optionId: string) => void; +}; + +// The agent receives both the selected option id and the custom text when +// both are present, so we never visually grey out one when the other is +// active — that previously created an asymmetric dimming bug where typing +// a custom answer dimmed the options but selecting an option did not dim +// the custom input. +export function ClarificationOptions({ + options, + selectedOption, + isSubmitting, + onSelectOption, +}: OptionListProps) { + return ( +
+ {options.map((option, idx) => { + const isSelected = selectedOption === option.option_id; + return ( + + ); + })} +
+ ); +} + +type CustomInputProps = { + draft: string; + isSubmitting: boolean; + committedText: string | null; + onChange: (text: string) => void; + onSubmit: (text: string) => void; +}; + +export function ClarificationCustomInput({ + draft, + isSubmitting, + committedText, + onChange, + onSubmit, +}: CustomInputProps) { + return ( +
+ ↳ + onChange(e.target.value)} + disabled={isSubmitting} + data-testid="clarification-input" + className="flex-1 text-sm bg-transparent placeholder:text-muted-foreground/60 focus:outline-none" + onKeyDown={(e) => { + if (e.key === "Enter" && !e.shiftKey && draft.trim()) { + e.preventDefault(); + onSubmit(draft.trim()); + } + }} + /> + +
+ ); +} + +type CarouselNavProps = { + activeIndex: number; + total: number; + isSubmitting: boolean; + onPrev: () => void; + onNext: () => void; + onSubmit: () => void; + canSubmit: boolean; +}; + +export function ClarificationCarouselNav({ + activeIndex, + total, + isSubmitting, + onPrev, + onNext, + onSubmit, + canSubmit, +}: CarouselNavProps) { + const isFirst = activeIndex === 0; + const isLast = activeIndex === total - 1; + return ( +
+ + {isLast ? ( + + ) : ( + + )} +
+ ); +} diff --git a/apps/web/components/task/task-chat-panel.tsx b/apps/web/components/task/task-chat-panel.tsx index 1b1f63bbe1..93bef70158 100644 --- a/apps/web/components/task/task-chat-panel.tsx +++ b/apps/web/components/task/task-chat-panel.tsx @@ -191,6 +191,7 @@ export const TaskChatPanel = memo(function TaskChatPanel({ agentMessageCount, cancelQueue, pendingClarification, + pendingClarificationGroup, isQueued, queuedMessage, updateQueueContent, @@ -262,7 +263,7 @@ export const TaskChatPanel = memo(function TaskChatPanel({ {pendingClarification && !isArchived && (
diff --git a/apps/web/e2e/pages/session-page.ts b/apps/web/e2e/pages/session-page.ts index 022e8b6edb..80b9f866a6 100644 --- a/apps/web/e2e/pages/session-page.ts +++ b/apps/web/e2e/pages/session-page.ts @@ -265,6 +265,67 @@ export class SessionPage { return this.clarificationOverlay().getByTestId("clarification-option-description"); } + /** All question cards rendered for the active clarification bundle. */ + clarificationQuestionCards(): Locator { + return this.clarificationOverlay().getByTestId("clarification-question-card"); + } + + /** A single question card by its question id (matches metadata.question_id). */ + clarificationQuestionCardById(questionId: string): Locator { + return this.clarificationOverlay().locator( + `[data-testid="clarification-question-card"][data-question-id="${questionId}"]`, + ); + } + + /** Group-wide progress chip "N of M answered" — only shown for bundles >1. */ + clarificationGroupProgress(): Locator { + return this.clarificationOverlay().getByTestId("clarification-group-progress"); + } + + /** Per-question "Question N of M" progress chip. */ + clarificationProgressChips(): Locator { + return this.clarificationOverlay().getByTestId("clarification-progress-chip"); + } + + /** Custom text input within a specific question card. */ + clarificationInputForQuestion(questionId: string): Locator { + return this.clarificationQuestionCardById(questionId).getByTestId("clarification-input"); + } + + /** Option button (by visible label text) inside a specific question card. */ + clarificationOptionForQuestion(questionId: string, text: string): Locator { + return this.clarificationQuestionCardById(questionId) + .getByTestId("clarification-option") + .filter({ hasText: text }); + } + + /** All step buttons in the horizontal stepper. */ + clarificationSteps(): Locator { + return this.clarificationOverlay().getByTestId("clarification-step"); + } + + /** A single step in the stepper, by its 0-based index. */ + clarificationStep(index: number): Locator { + return this.clarificationOverlay().locator( + `[data-testid="clarification-step"][data-step-index="${index}"]`, + ); + } + + /** Back button inside the carousel nav. */ + clarificationPrev(): Locator { + return this.clarificationOverlay().getByTestId("clarification-prev"); + } + + /** Next button inside the carousel nav. */ + clarificationNext(): Locator { + return this.clarificationOverlay().getByTestId("clarification-next"); + } + + /** Final "Submit answers" button (rendered only on the last step). */ + clarificationSubmit(): Locator { + return this.clarificationOverlay().getByTestId("clarification-submit"); + } + /** All visible "Approve / Deny" rows for pending permission requests. */ permissionActionRows(): Locator { return this.chat.getByTestId("permission-action-row"); diff --git a/apps/web/e2e/tests/chat/clarification.spec.ts b/apps/web/e2e/tests/chat/clarification.spec.ts index f62114e23a..ad7be0dd4d 100644 --- a/apps/web/e2e/tests/chat/clarification.spec.ts +++ b/apps/web/e2e/tests/chat/clarification.spec.ts @@ -50,20 +50,14 @@ test.describe("Clarification flow", () => { "clarification", ); - // Wait for clarification overlay to appear (agent calls ask_user_question MCP tool) await expect(session.clarificationOverlay()).toBeVisible({ timeout: 30_000 }); - - // Verify the question text appears await expect(session.clarificationOverlay()).toContainText("Which database"); - // Click the PostgreSQL option + // Single-question bundles still expose option click → instant resolve. await session.clarificationOption("PostgreSQL").click(); - // Agent receives the answer and completes its turn await expect(session.idleInput()).toBeVisible({ timeout: 30_000 }); - - // Verify the answer was reflected in chat - await expect(session.chat).toContainText(/You answered|User selected/); + await expect(session.chat).toContainText(/You answered|selected_option/); }); test("skip clarification", async ({ testPage, apiClient, seedData }) => { @@ -75,13 +69,8 @@ test.describe("Clarification flow", () => { "clarification", ); - // Wait for clarification overlay await expect(session.clarificationOverlay()).toBeVisible({ timeout: 30_000 }); - - // Click skip button await session.clarificationSkip().click(); - - // Agent should complete its turn await expect(session.idleInput()).toBeVisible({ timeout: 30_000 }); }); @@ -98,23 +87,11 @@ test.describe("Clarification flow", () => { "clarification-timeout", ); - // Wait for clarification overlay to appear await expect(session.clarificationOverlay()).toBeVisible({ timeout: 30_000 }); - - // Wait for agent to time out (5s) and complete its turn. await expect(session.chat).toContainText("timed out", { timeout: 30_000 }); - - // 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(); - - // 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 }); }); @@ -132,16 +109,10 @@ test.describe("Clarification flow", () => { ); 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) { @@ -153,7 +124,6 @@ test.describe("Clarification flow", () => { test("plan mode + clarification does not leave pointer-events stuck on body", async ({ testPage, }) => { - // Navigate to kanban board and open the task create dialog const kanban = new KanbanPage(testPage); await kanban.goto(); @@ -161,42 +131,346 @@ test.describe("Clarification flow", () => { const dialog = testPage.getByTestId("create-task-dialog"); await expect(dialog).toBeVisible(); - // Fill title await testPage.getByTestId("task-title-input").fill("Plan Mode Clarification PE"); - // Fill description with clarification scenario so the agent starts and - // calls the ask_user_question MCP tool. const descriptionInput = dialog.getByRole("textbox", { name: "Write a prompt for the agent...", }); await descriptionInput.click(); await descriptionInput.fill("/e2e:clarification"); - // With a description present, the footer shows a split button with dropdown. - // Open the chevron dropdown and click "Start task in plan mode". await testPage.getByTestId("submit-start-agent-chevron").click(); await testPage.getByTestId("submit-plan-mode").click(); - // Wait for navigation to session page await expect(testPage).toHaveURL(/\/t\//, { timeout: 15_000 }); const session = new SessionPage(testPage); await session.waitForLoad(); - // Wait for clarification overlay to appear (agent calls ask_user_question MCP tool) await expect(session.clarificationOverlay()).toBeVisible({ timeout: 30_000 }); - // CRITICAL ASSERTION: body must not have pointer-events: none stuck on it. - // Radix Dialog sets pointer-events: none on body when modal. If the task - // create dialog unmounts mid-close (onOpenChange(false) then router.push), - // Radix never finishes cleanup, leaving the page unclickable. const pointerEvents = await testPage.evaluate(() => document.body.style.pointerEvents); expect(pointerEvents).not.toBe("none"); - // Verify the UI is actually interactive by clicking a clarification option await session.clarificationOption("PostgreSQL").click(); - - // Agent receives the answer and completes its turn (plan mode uses different placeholder) await expect(session.planModeInput()).toBeVisible({ timeout: 30_000 }); }); }); + +// Multi-question carousel UX. Each scenario uses the mock-agent's +// `clarification-multi` scenario which sends 3 questions in a single MCP call. +test.describe("Multi-question clarification carousel", () => { + test.describe.configure({ retries: 1 }); + + test("renders stepper with 3 steps and shows the first question", async ({ + testPage, + apiClient, + seedData, + }) => { + const session = await seedClarificationTask( + testPage, + apiClient, + seedData, + "Multi-q stepper", + "clarification-multi", + ); + + await expect(session.clarificationOverlay()).toBeVisible({ timeout: 30_000 }); + + // 3 stepper buttons rendered; the first is active and unanswered. + await expect(session.clarificationSteps()).toHaveCount(3); + await expect(session.clarificationStep(0)).toHaveAttribute("data-active", "true"); + await expect(session.clarificationStep(0)).toHaveAttribute("data-answered", "false"); + await expect(session.clarificationStep(1)).toHaveAttribute("data-active", "false"); + + // Group progress + per-question chip both rendered. + await expect(session.clarificationGroupProgress()).toContainText("0 of 3 answered"); + await expect(session.clarificationOverlay()).toContainText("Question 1 of 3"); + + // Only one card is visible at a time (carousel UX, not stacked). + await expect(session.clarificationQuestionCards()).toHaveCount(1); + await expect(session.clarificationOverlay()).toContainText("Which database"); + }); + + test("answering option auto-advances to next step and marks step as answered", async ({ + testPage, + apiClient, + seedData, + }) => { + const session = await seedClarificationTask( + testPage, + apiClient, + seedData, + "Multi-q advance", + "clarification-multi", + ); + + await expect(session.clarificationOverlay()).toBeVisible({ timeout: 30_000 }); + + // Pick an option on step 1; auto-advances to step 2. + await session.clarificationOption("PostgreSQL").click(); + await expect(session.clarificationStep(1)).toHaveAttribute("data-active", "true"); + await expect(session.clarificationStep(0)).toHaveAttribute("data-answered", "true"); + await expect(session.clarificationGroupProgress()).toContainText("1 of 3 answered"); + await expect(session.clarificationOverlay()).toContainText("Question 2 of 3"); + await expect(session.clarificationOverlay()).toContainText("Which language"); + }); + + test("Back button restores the previous question and the prior selection", async ({ + testPage, + apiClient, + seedData, + }) => { + const session = await seedClarificationTask( + testPage, + apiClient, + seedData, + "Multi-q back", + "clarification-multi", + ); + + await expect(session.clarificationOverlay()).toBeVisible({ timeout: 30_000 }); + + await session.clarificationOption("PostgreSQL").click(); + await expect(session.clarificationStep(1)).toHaveAttribute("data-active", "true"); + + await session.clarificationPrev().click(); + await expect(session.clarificationStep(0)).toHaveAttribute("data-active", "true"); + + // Previous answer is still selected. + const selectedOption = session + .clarificationQuestionCardById("db") + .locator('[data-testid="clarification-option"][data-selected="true"]'); + await expect(selectedOption).toContainText("PostgreSQL"); + }); + + test("clicking a step in the stepper jumps directly to that question", async ({ + testPage, + apiClient, + seedData, + }) => { + const session = await seedClarificationTask( + testPage, + apiClient, + seedData, + "Multi-q jump", + "clarification-multi", + ); + + await expect(session.clarificationOverlay()).toBeVisible({ timeout: 30_000 }); + + await session.clarificationStep(2).click(); + await expect(session.clarificationStep(2)).toHaveAttribute("data-active", "true"); + await expect(session.clarificationOverlay()).toContainText("Question 3 of 3"); + await expect(session.clarificationOverlay()).toContainText("How should we deploy"); + }); + + test("Submit button disabled until every question is answered", async ({ + testPage, + apiClient, + seedData, + }) => { + const session = await seedClarificationTask( + testPage, + apiClient, + seedData, + "Multi-q submit gating", + "clarification-multi", + ); + + await expect(session.clarificationOverlay()).toBeVisible({ timeout: 30_000 }); + + // Jump to last step without answering. + await session.clarificationStep(2).click(); + const submit = session.clarificationSubmit(); + await expect(submit).toBeVisible(); + await expect(submit).toBeDisabled(); + }); + + test("happy path: answer all 3 then Submit unblocks the agent", async ({ + testPage, + apiClient, + seedData, + }) => { + const session = await seedClarificationTask( + testPage, + apiClient, + seedData, + "Multi-q happy path", + "clarification-multi", + ); + + await expect(session.clarificationOverlay()).toBeVisible({ timeout: 30_000 }); + + await session.clarificationOption("PostgreSQL").click(); + await session.clarificationOption("Go").click(); + await session.clarificationOption("Docker").click(); + + // We're on the last step; Submit is enabled. + const submit = session.clarificationSubmit(); + await expect(submit).toBeEnabled(); + await submit.click(); + + await expect(session.clarificationOverlay()).not.toBeVisible({ timeout: 30_000 }); + await expect(session.idleInput()).toBeVisible({ timeout: 30_000 }); + await expect(session.chat).toContainText("selected_option"); + }); + + test("mix custom text + option selections round-trips", async ({ + testPage, + apiClient, + seedData, + }) => { + const session = await seedClarificationTask( + testPage, + apiClient, + seedData, + "Multi-q mixed", + "clarification-multi", + ); + + await expect(session.clarificationOverlay()).toBeVisible({ timeout: 30_000 }); + + // Q1: option click → advances. + await session.clarificationOption("SQLite").click(); + + // Q2: custom text via Enter → advances. + const langInput = session.clarificationInputForQuestion("language"); + await langInput.click(); + await langInput.fill("Elixir"); + await langInput.press("Enter"); + await expect(session.clarificationGroupProgress()).toContainText("2 of 3 answered"); + + // Q3: option click; submit batch. + await session.clarificationOption("Bare metal").click(); + await session.clarificationSubmit().click(); + + await expect(session.clarificationOverlay()).not.toBeVisible({ timeout: 30_000 }); + await expect(session.idleInput()).toBeVisible({ timeout: 30_000 }); + await expect(session.chat).toContainText("Elixir"); + }); + + test("revising an answer via stepper jump updates the response", async ({ + testPage, + apiClient, + seedData, + }) => { + const session = await seedClarificationTask( + testPage, + apiClient, + seedData, + "Multi-q revise", + "clarification-multi", + ); + + await expect(session.clarificationOverlay()).toBeVisible({ timeout: 30_000 }); + + await session.clarificationOption("PostgreSQL").click(); + await session.clarificationOption("Go").click(); + await session.clarificationOption("Docker").click(); + + // Jump back to Q1 and pick a different option. + await session.clarificationStep(0).click(); + await session.clarificationOption("MongoDB").click(); + // Auto-advance brings us to Q2; jump to last step to submit. + await session.clarificationStep(2).click(); + + await session.clarificationSubmit().click(); + await expect(session.clarificationOverlay()).not.toBeVisible({ timeout: 30_000 }); + await expect(session.chat).toContainText("MongoDB"); + }); + + test("skip rejects the entire bundle from any step", async ({ + testPage, + apiClient, + seedData, + }) => { + const session = await seedClarificationTask( + testPage, + apiClient, + seedData, + "Multi-q skip mid", + "clarification-multi", + ); + + await expect(session.clarificationOverlay()).toBeVisible({ timeout: 30_000 }); + + // Answer the first one then skip from step 2. + await session.clarificationOption("PostgreSQL").click(); + await session.clarificationSkip().click(); + + await expect(session.clarificationOverlay()).not.toBeVisible({ timeout: 30_000 }); + await expect(session.idleInput()).toBeVisible({ timeout: 30_000 }); + await expect(session.chat).toContainText("rejected"); + }); + + test("number key shortcuts pick options on the active step", async ({ + testPage, + apiClient, + seedData, + }) => { + const session = await seedClarificationTask( + testPage, + apiClient, + seedData, + "Multi-q kbd", + "clarification-multi", + ); + + await expect(session.clarificationOverlay()).toBeVisible({ timeout: 30_000 }); + + // Press "1" → first option of the first question, auto-advance. + await testPage.keyboard.press("1"); + await expect(session.clarificationStep(1)).toHaveAttribute("data-active", "true"); + await expect(session.clarificationGroupProgress()).toContainText("1 of 3 answered"); + + // Press "2" → second option of Q2. + await testPage.keyboard.press("2"); + await expect(session.clarificationStep(2)).toHaveAttribute("data-active", "true"); + await expect(session.clarificationGroupProgress()).toContainText("2 of 3 answered"); + + // Press "1" → first option of Q3 (last step, no advance). + await testPage.keyboard.press("1"); + await expect(session.clarificationGroupProgress()).toContainText("3 of 3 answered"); + + // ArrowRight on the last step with all answered → submits. + await testPage.keyboard.press("ArrowRight"); + await expect(session.clarificationOverlay()).not.toBeVisible({ timeout: 30_000 }); + await expect(session.idleInput()).toBeVisible({ timeout: 30_000 }); + }); + + test("Esc skips the entire bundle from anywhere in the carousel", async ({ + testPage, + apiClient, + seedData, + }) => { + const session = await seedClarificationTask( + testPage, + apiClient, + seedData, + "Multi-q esc", + "clarification-multi", + ); + + await expect(session.clarificationOverlay()).toBeVisible({ timeout: 30_000 }); + + await session.clarificationStep(1).click(); + await testPage.keyboard.press("Escape"); + + await expect(session.clarificationOverlay()).not.toBeVisible({ timeout: 30_000 }); + await expect(session.chat).toContainText("rejected"); + }); + + test("Back button is disabled on the first step", async ({ testPage, apiClient, seedData }) => { + const session = await seedClarificationTask( + testPage, + apiClient, + seedData, + "Multi-q back disabled", + "clarification-multi", + ); + + await expect(session.clarificationOverlay()).toBeVisible({ timeout: 30_000 }); + await expect(session.clarificationPrev()).toBeDisabled(); + }); +}); diff --git a/apps/web/hooks/domains/session/use-clarification-group.test.ts b/apps/web/hooks/domains/session/use-clarification-group.test.ts new file mode 100644 index 0000000000..47d5123c30 --- /dev/null +++ b/apps/web/hooks/domains/session/use-clarification-group.test.ts @@ -0,0 +1,187 @@ +import { describe, it, expect, vi, beforeEach } from "vitest"; +import { act, renderHook } from "@testing-library/react"; +import type { Message } from "@/lib/types/http"; + +vi.mock("@/lib/config", () => ({ + getBackendConfig: () => ({ apiBaseUrl: "https://api.test" }), +})); + +import { useClarificationGroup } from "./use-clarification-group"; + +function clarMessage(opts: { + id: string; + pendingId: string; + questionId: string; + index: number; + total: number; +}): Message { + return { + id: opts.id, + session_id: "s1", + task_id: "t1", + author_type: "agent", + content: "Q", + type: "clarification_request", + created_at: "2026-05-04T00:00:00Z", + metadata: { + pending_id: opts.pendingId, + question_id: opts.questionId, + question_index: opts.index, + question_total: opts.total, + status: "pending", + question: { id: opts.questionId, prompt: "Q?" }, + }, + }; +} + +const fetchMock = vi.fn(); + +function setupFetchMock() { + fetchMock.mockReset(); + fetchMock.mockResolvedValue(new Response(null, { status: 200 })); + globalThis.fetch = fetchMock as unknown as typeof globalThis.fetch; +} + +describe("useClarificationGroup — derived state", () => { + beforeEach(setupFetchMock); + + it("derives total + pendingId from the message bundle", () => { + const msgs = [ + clarMessage({ id: "m1", pendingId: "p1", questionId: "q1", index: 0, total: 2 }), + clarMessage({ id: "m2", pendingId: "p1", questionId: "q2", index: 1, total: 2 }), + ]; + const { result } = renderHook(() => useClarificationGroup(msgs)); + expect(result.current.pendingId).toBe("p1"); + expect(result.current.total).toBe(2); + expect(result.current.answeredCount).toBe(0); + }); + + it("returns null pendingId when there are no messages", () => { + const { result } = renderHook(() => useClarificationGroup([])); + expect(result.current.pendingId).toBeNull(); + expect(result.current.total).toBe(0); + }); + + it("recordAnswer updates local state without posting", async () => { + const msgs = [ + clarMessage({ id: "m1", pendingId: "p1", questionId: "q1", index: 0, total: 2 }), + clarMessage({ id: "m2", pendingId: "p1", questionId: "q2", index: 1, total: 2 }), + ]; + const { result } = renderHook(() => useClarificationGroup(msgs)); + + await act(async () => { + result.current.recordAnswer("q1", { question_id: "q1", selected_options: ["o1"] }); + }); + + expect(result.current.answers["q1"]?.selected_options).toEqual(["o1"]); + expect(result.current.answeredCount).toBe(1); + expect(fetchMock).not.toHaveBeenCalled(); + }); +}); + +describe("useClarificationGroup — submit + skip", () => { + beforeEach(setupFetchMock); + + it("submitCollected POSTs the batch only when every question has an answer", async () => { + const msgs = [ + clarMessage({ id: "m1", pendingId: "p1", questionId: "q1", index: 0, total: 2 }), + clarMessage({ id: "m2", pendingId: "p1", questionId: "q2", index: 1, total: 2 }), + ]; + const { result } = renderHook(() => useClarificationGroup(msgs)); + + await act(async () => { + result.current.recordAnswer("q1", { question_id: "q1", selected_options: ["o1"] }); + }); + await act(async () => { + await result.current.submitCollected(); + }); + expect(fetchMock).not.toHaveBeenCalled(); + + await act(async () => { + result.current.recordAnswer("q2", { question_id: "q2", custom_text: "free" }); + }); + await act(async () => { + await result.current.submitCollected(); + }); + expect(fetchMock).toHaveBeenCalledTimes(1); + const [url, init] = fetchMock.mock.calls[0]; + expect(String(url)).toBe("https://api.test/api/v1/clarification/p1/respond"); + expect(JSON.parse(String(init.body))).toEqual({ + answers: [ + { question_id: "q1", selected_options: ["o1"] }, + { question_id: "q2", custom_text: "free" }, + ], + rejected: false, + }); + expect(result.current.submitState).toBe("ok"); + }); + + it("submitCollected with override merges the freshly recorded answer", async () => { + const msgs = [clarMessage({ id: "m1", pendingId: "p1", questionId: "q1", index: 0, total: 1 })]; + const { result } = renderHook(() => useClarificationGroup(msgs)); + + await act(async () => { + await result.current.submitCollected({ + q1: { question_id: "q1", selected_options: ["o1"] }, + }); + }); + expect(fetchMock).toHaveBeenCalledTimes(1); + expect(result.current.submitState).toBe("ok"); + }); + + it("skipAll POSTs rejected=true with the supplied reason", async () => { + const msgs = [clarMessage({ id: "m1", pendingId: "p1", questionId: "q1", index: 0, total: 1 })]; + const { result } = renderHook(() => useClarificationGroup(msgs)); + + await act(async () => { + await result.current.skipAll("Too vague"); + }); + expect(fetchMock).toHaveBeenCalledTimes(1); + const [, init] = fetchMock.mock.calls[0]; + expect(JSON.parse(String(init.body))).toEqual({ + rejected: true, + reject_reason: "Too vague", + }); + expect(result.current.submitState).toBe("ok"); + }); + + it("submitState transitions to 'error' when fetch returns non-OK", async () => { + fetchMock.mockResolvedValueOnce(new Response("nope", { status: 400 })); + const msgs = [clarMessage({ id: "m1", pendingId: "p1", questionId: "q1", index: 0, total: 1 })]; + const { result } = renderHook(() => useClarificationGroup(msgs)); + + await act(async () => { + await result.current.submitCollected({ + q1: { question_id: "q1", selected_options: ["o1"] }, + }); + }); + expect(result.current.submitState).toBe("error"); + }); + + // 409 Conflict means a duplicate submit landed after the first one already + // resolved the bundle on the backend. Treat it as success so the overlay + // closes instead of getting stuck in an unrecoverable error state. + it("submitState transitions to 'ok' on 409 (duplicate submit)", async () => { + fetchMock.mockResolvedValueOnce(new Response("conflict", { status: 409 })); + const msgs = [clarMessage({ id: "m1", pendingId: "p1", questionId: "q1", index: 0, total: 1 })]; + const { result } = renderHook(() => useClarificationGroup(msgs)); + + await act(async () => { + await result.current.submitCollected({ + q1: { question_id: "q1", selected_options: ["o1"] }, + }); + }); + expect(result.current.submitState).toBe("ok"); + }); + + it("skipAll transitions to 'ok' on 409 too", async () => { + fetchMock.mockResolvedValueOnce(new Response("conflict", { status: 409 })); + const msgs = [clarMessage({ id: "m1", pendingId: "p1", questionId: "q1", index: 0, total: 1 })]; + const { result } = renderHook(() => useClarificationGroup(msgs)); + + await act(async () => { + await result.current.skipAll(); + }); + expect(result.current.submitState).toBe("ok"); + }); +}); diff --git a/apps/web/hooks/domains/session/use-clarification-group.ts b/apps/web/hooks/domains/session/use-clarification-group.ts new file mode 100644 index 0000000000..2752dc29cc --- /dev/null +++ b/apps/web/hooks/domains/session/use-clarification-group.ts @@ -0,0 +1,153 @@ +"use client"; + +import { useCallback, useEffect, useMemo, useRef, useState } from "react"; +import type { ClarificationAnswer, ClarificationRequestMetadata, Message } from "@/lib/types/http"; +import { getBackendConfig } from "@/lib/config"; + +type SubmitState = "idle" | "submitting" | "ok" | "error"; + +export type ClarificationGroupApi = { + pendingId: string | null; + total: number; + answeredCount: number; + answers: Record; + submitState: SubmitState; + recordAnswer: (questionId: string, answer: ClarificationAnswer) => void; + // Submits every recorded answer in a single batch. An optional `override` + // map is merged into the current answers right before the POST so callers + // can safely auto-submit immediately after recording an answer (the React + // state update is async, so the hook's stored map may not include the + // freshly recorded answer yet). + submitCollected: (override?: Record) => Promise; + skipAll: (reason?: string) => Promise; +}; + +function questionIdsFromMessages(messages: readonly Message[]): string[] { + return messages + .slice() + .sort((a, b) => { + const ai = (a.metadata as ClarificationRequestMetadata | undefined)?.question_index ?? 0; + const bi = (b.metadata as ClarificationRequestMetadata | undefined)?.question_index ?? 0; + return ai - bi; + }) + .map((m) => { + const meta = m.metadata as ClarificationRequestMetadata | undefined; + return meta?.question_id ?? meta?.question?.id ?? ""; + }) + .filter(Boolean); +} + +async function postClarificationBatch( + pendingId: string, + answers: ClarificationAnswer[], +): Promise { + const { apiBaseUrl } = getBackendConfig(); + const res = await fetch(`${apiBaseUrl}/api/v1/clarification/${pendingId}/respond`, { + method: "POST", + headers: { "Content-Type": "application/json" }, + body: JSON.stringify({ answers, rejected: false }), + }); + if (res.ok) return "ok"; + // 409 Conflict means a duplicate submit (the user clicked Submit twice in + // quick succession). Treat it as success — the first submit already won + // and resolved the bundle on the backend, so the overlay should close. + if (res.status === 409) return "ok"; + console.error("Clarification submit failed:", res.status, res.statusText); + return "error"; +} + +async function postClarificationSkip(pendingId: string, reason: string): Promise { + const { apiBaseUrl } = getBackendConfig(); + const res = await fetch(`${apiBaseUrl}/api/v1/clarification/${pendingId}/respond`, { + method: "POST", + headers: { "Content-Type": "application/json" }, + body: JSON.stringify({ rejected: true, reject_reason: reason }), + }); + if (res.ok) return "ok"; + if (res.status === 409) return "ok"; + console.error("Clarification skip failed:", res.status, res.statusText); + return "error"; +} + +// useClarificationGroup tracks the per-question answers for a multi-question +// clarification bundle. The carousel UI owns navigation; this hook just stores +// the local answer state and exposes: +// - recordAnswer: write a single question's answer to local state +// - submitCollected: POST every recorded answer in one batch (called from +// the explicit "Submit answers" button on the last step) +// - skipAll: reject the entire bundle. +// Decision A is preserved (per-question commit, batched on the wire) but the +// final submit is no longer implicit — the user clicks "Submit answers" or +// presses ArrowRight on the last step. +export function useClarificationGroup( + messages: readonly Message[] | null | undefined, +): ClarificationGroupApi { + const [answers, setAnswers] = useState>({}); + const answersRef = useRef(answers); + useEffect(() => { + answersRef.current = answers; + }, [answers]); + const [submitState, setSubmitState] = useState("idle"); + + const pendingId = useMemo(() => { + if (!messages || messages.length === 0) return null; + const meta = messages[0].metadata as ClarificationRequestMetadata | undefined; + return meta?.pending_id ?? null; + }, [messages]); + + const questionIds = useMemo( + () => (messages ? questionIdsFromMessages(messages) : []), + [messages], + ); + const total = questionIds.length; + const answeredCount = Object.keys(answers).filter((id) => questionIds.includes(id)).length; + + const recordAnswer = useCallback((questionId: string, answer: ClarificationAnswer) => { + setAnswers((prev) => ({ ...prev, [questionId]: answer })); + }, []); + + const submitCollected = useCallback( + async (override?: Record) => { + if (!pendingId) return; + const current = { ...answersRef.current, ...(override ?? {}) }; + const haveAll = questionIds.every((id) => Boolean(current[id])); + if (!haveAll) return; + setSubmitState("submitting"); + const ordered = questionIds + .map((id) => current[id]) + .filter((a): a is ClarificationAnswer => Boolean(a)); + try { + setSubmitState(await postClarificationBatch(pendingId, ordered)); + } catch (err) { + console.error("Clarification submit threw:", err); + setSubmitState("error"); + } + }, + [pendingId, questionIds], + ); + + const skipAll = useCallback( + async (reason?: string) => { + if (!pendingId) return; + setSubmitState("submitting"); + try { + setSubmitState(await postClarificationSkip(pendingId, reason ?? "User skipped")); + } catch (err) { + console.error("Clarification skip threw:", err); + setSubmitState("error"); + } + }, + [pendingId], + ); + + return { + pendingId, + total, + answeredCount, + answers, + submitState, + recordAnswer, + submitCollected, + skipAll, + }; +} diff --git a/apps/web/hooks/use-processed-messages.ts b/apps/web/hooks/use-processed-messages.ts index 8d75f6e19a..22b5b36cce 100644 --- a/apps/web/hooks/use-processed-messages.ts +++ b/apps/web/hooks/use-processed-messages.ts @@ -1,7 +1,10 @@ import { useMemo } from "react"; import type { Message, ClarificationRequestMetadata, MessageType } from "@/lib/types/http"; import type { ToolCallMetadata } from "@/components/task/chat/types"; -import { findPendingClarification } from "@/lib/utils/pending-clarification"; +import { + findPendingClarification, + findPendingClarificationGroup, +} from "@/lib/utils/pending-clarification"; const ACTIVITY_MESSAGE_TYPES: Set = new Set([ "thinking", @@ -247,6 +250,10 @@ export function useProcessedMessages( [childrenByParentToolCallId], ); const pendingClarification = useMemo(() => findPendingClarification(messages), [messages]); + const pendingClarificationGroup = useMemo( + () => findPendingClarificationGroup(messages), + [messages], + ); const visibleMessages = useMemo( () => filterVisibleMessages(messages, toolCallIds, subagentChildIds), @@ -323,5 +330,6 @@ export function useProcessedMessages( todoItems, agentMessageCount, pendingClarification, + pendingClarificationGroup, }; } diff --git a/apps/web/lib/types/http-agents.ts b/apps/web/lib/types/http-agents.ts index 3e3e6b8217..c6950832c5 100644 --- a/apps/web/lib/types/http-agents.ts +++ b/apps/web/lib/types/http-agents.ts @@ -230,11 +230,18 @@ export type ClarificationQuestion = { options: ClarificationOption[]; }; +// Each per-question chat message carries its own metadata. For multi-question +// bundles, every message in the group shares the same pending_id and includes +// question_index/question_total so the renderer can show "Question 2 of 3" +// progress chips. export type ClarificationRequestMetadata = { pending_id: string; session_id: string; task_id?: string; question: ClarificationQuestion; + question_id?: string; + question_index?: number; + question_total?: number; context?: string; status?: "pending" | "answered" | "rejected" | "expired"; response?: ClarificationAnswer; diff --git a/apps/web/lib/utils/pending-clarification.test.ts b/apps/web/lib/utils/pending-clarification.test.ts index 15568a63cf..95b180cfd5 100644 --- a/apps/web/lib/utils/pending-clarification.test.ts +++ b/apps/web/lib/utils/pending-clarification.test.ts @@ -1,6 +1,6 @@ import { describe, expect, it } from "vitest"; import type { Message } from "@/lib/types/http"; -import { hasPendingClarification } from "./pending-clarification"; +import { findPendingClarificationGroup, hasPendingClarification } from "./pending-clarification"; function message(overrides: Partial): Message { return { @@ -62,3 +62,73 @@ describe("hasPendingClarification", () => { ).toBe(false); }); }); + +describe("findPendingClarificationGroup", () => { + it("returns empty array when there are no messages", () => { + expect(findPendingClarificationGroup([])).toEqual([]); + expect(findPendingClarificationGroup(null)).toEqual([]); + expect(findPendingClarificationGroup(undefined)).toEqual([]); + }); + + it("returns empty array when there is no pending clarification", () => { + expect( + findPendingClarificationGroup([ + message({ type: "clarification_request", metadata: { status: "answered" } }), + ]), + ).toEqual([]); + }); + + it("returns the single message when no pending_id is set", () => { + const msg = message({ type: "clarification_request" }); + expect(findPendingClarificationGroup([msg])).toEqual([msg]); + }); + + it("returns every message that shares the latest pending_id", () => { + const a = message({ + id: "a", + type: "clarification_request", + metadata: { pending_id: "p1", question_total: 3, question_index: 0, status: "pending" }, + }); + const b = message({ + id: "b", + type: "clarification_request", + metadata: { pending_id: "p1", question_total: 3, question_index: 1, status: "pending" }, + }); + const c = message({ + id: "c", + type: "clarification_request", + metadata: { pending_id: "p1", question_total: 3, question_index: 2, status: "pending" }, + }); + const noise = message({ id: "n", type: "message" }); + expect(findPendingClarificationGroup([noise, a, b, c]).map((m) => m.id)).toEqual([ + "a", + "b", + "c", + ]); + }); + + it("gates on question_total — returns [] when not all messages have arrived", () => { + // Backend declared 3 questions but only 1 has streamed in; the overlay + // should stay hidden until the remaining 2 land. + const a = message({ + id: "a", + type: "clarification_request", + metadata: { pending_id: "p1", question_total: 3, question_index: 0, status: "pending" }, + }); + expect(findPendingClarificationGroup([a])).toEqual([]); + }); + + it("ignores messages from a different pending bundle", () => { + const old = message({ + id: "old", + type: "clarification_request", + metadata: { pending_id: "p0", status: "answered" }, + }); + const a = message({ + id: "a", + type: "clarification_request", + metadata: { pending_id: "p1", question_total: 1, status: "pending" }, + }); + expect(findPendingClarificationGroup([old, a]).map((m) => m.id)).toEqual(["a"]); + }); +}); diff --git a/apps/web/lib/utils/pending-clarification.ts b/apps/web/lib/utils/pending-clarification.ts index f8932d6e69..129af13fad 100644 --- a/apps/web/lib/utils/pending-clarification.ts +++ b/apps/web/lib/utils/pending-clarification.ts @@ -14,6 +14,35 @@ export function findPendingClarification(messages?: readonly Message[] | null): return null; } +// findPendingClarificationGroup returns every clarification_request message +// that shares the latest pending message's pending_id, ordered by chat position. +// Multi-question bundles emit one message per question; the chat panel uses this +// list to render every pending question card together. +// +// Gates on `question_total` from metadata: returns an empty array until the +// number of messages received equals the expected bundle size. This prevents +// a user from acting on a partially-arrived bundle (clicking an option before +// the rest of the N messages have been streamed in via the WS), which would +// otherwise trigger a 400 from the backend's all-required gate. +export function findPendingClarificationGroup(messages?: readonly Message[] | null): Message[] { + if (!messages) return []; + const last = findPendingClarification(messages); + if (!last) return []; + const meta = last.metadata as ClarificationRequestMetadata | undefined; + const pendingID = meta?.pending_id; + if (!pendingID) return [last]; + const group = messages.filter((m) => { + if (m.type !== "clarification_request") return false; + const mMeta = m.metadata as ClarificationRequestMetadata | undefined; + return mMeta?.pending_id === pendingID; + }); + const expectedTotal = meta?.question_total; + if (typeof expectedTotal === "number" && expectedTotal > 0 && group.length < expectedTotal) { + return []; + } + return group; +} + export function hasPendingClarification(messages?: readonly Message[] | null): boolean { return findPendingClarification(messages) !== null; }