feat: multi-question support for ask_user_question_kandev (#828)

* 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.
This commit is contained in:
José Almeida
2026-05-05 19:14:57 +01:00
committed by GitHub
parent c64e835612
commit bce9fd266a
31 changed files with 2995 additions and 607 deletions
+73 -38
View File
@@ -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.
+80 -6
View File
@@ -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)
@@ -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)
@@ -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))
}
}
+373 -74
View File
@@ -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)
}
@@ -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)
@@ -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.
@@ -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"})
+14 -10
View File
@@ -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"`
+22 -28
View File
@@ -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 {
@@ -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")
}
+189 -71
View File
@@ -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": "<id>"}
// {"custom_text": "<text>"}
// {"selected_option": "<id>", "custom_text": "<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
+93 -37
View File
@@ -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},
}
}
@@ -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)
}
@@ -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
}
@@ -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
}
@@ -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)
@@ -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.
@@ -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
@@ -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 && (
<div className="flex-shrink-0 border-t border-sky-400/30 bg-card px-1">
<ClarificationInputOverlay
message={pendingClarification}
messages={pendingClarificationGroup}
onResolved={handleClarificationResolved}
/>
</div>
@@ -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<string, unknown>,
): 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 (
<div className="space-y-0.5 mb-1.5 ml-6">
{question.options.map((option) => (
<button
key={option.option_id}
type="button"
onClick={() => onSelectOption(option.option_id)}
disabled={isSubmitting}
data-testid="clarification-option"
className={cn(
"flex items-start gap-2 w-full text-left text-xs rounded px-1.5 py-1 -ml-1.5 transition-colors",
"hover:bg-blue-500/15 hover:text-blue-600 dark:hover:text-blue-400",
isSubmitting ? "opacity-50 cursor-not-allowed" : "text-foreground/80 cursor-pointer",
)}
>
<span className="text-muted-foreground flex-shrink-0 leading-5">•</span>
<span className="flex-1 min-w-0">
<span data-testid="clarification-option-label" className="block leading-5">
{option.label}
</span>
{option.description && (
<span
data-testid="clarification-option-description"
className="block text-muted-foreground/70 mt-0.5 leading-snug"
>
{option.description}
</span>
)}
</span>
</button>
))}
</div>
);
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 (
<div className="flex items-center gap-2 ml-6">
<span className="text-muted-foreground flex-shrink-0">•</span>
<input
type="text"
placeholder="Type something..."
value={customText}
onChange={(e) => 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();
}
}}
<div
data-testid="clarification-question-card"
data-question-id={meta.questionId}
data-question-index={String(index)}
className="px-4 pt-3 pb-4"
>
<div className="flex items-center gap-2 mb-2 text-xs text-muted-foreground">
<IconMessageQuestion className="h-4 w-4 text-blue-500" />
{total > 1 && (
<span data-testid="clarification-progress-chip">
Question {index + 1} of {total}
</span>
)}
{metadata.question.title && (
<span className="text-muted-foreground/70">
{total > 1 ? "· " : ""}
{metadata.question.title}
</span>
)}
</div>
<div className="markdown-body max-w-none text-sm font-medium [&>*:first-child]:mt-0 [&>*:last-child]:mb-0 mb-3">
<ReactMarkdown remarkPlugins={remarkPlugins} components={markdownComponents}>
{question.prompt}
</ReactMarkdown>
</div>
<ClarificationOptions
options={question.options}
selectedOption={selectedOption}
isSubmitting={isSubmitting}
onSelectOption={onSelectOption}
/>
{showAgentDisconnected && (
<div
data-testid="clarification-deferred-notice"
className="mt-2 text-xs text-amber-500/90 flex items-center gap-1.5"
>
<IconInfoCircle className="h-3.5 w-3.5 flex-shrink-0" />
The agent has moved on. Your response will be sent as a new message.
</div>
)}
<ClarificationCustomInput
draft={customDraft}
isSubmitting={isSubmitting}
committedText={customCommittedText}
onChange={onCustomDraftChange}
onSubmit={onSubmitCustom}
/>
</div>
);
}
type UseClarificationHandlersParams = {
metadata: ClarificationRequestMetadata | undefined;
isSubmitting: boolean;
setIsSubmitting: React.Dispatch<React.SetStateAction<boolean>>;
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<typeof useClarificationGroup>["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<typeof useClarificationGroup>;
activeIndex: number;
setActiveIndex: (idx: number) => void;
customDrafts: Record<string, string>;
setCustomDrafts: React.Dispatch<React.SetStateAction<Record<string, string>>>;
};
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 (
<div className="relative px-3 py-2" data-testid="clarification-overlay">
<button
type="button"
onClick={handleSkip}
disabled={isSubmitting}
className="absolute top-2 right-3 text-muted-foreground hover:text-foreground z-10 cursor-pointer"
data-testid="clarification-skip"
>
<IconX className="h-4 w-4" />
</button>
<>
<ClarificationCard
meta={meta}
index={activeIndex}
total={total}
selectedOption={selectedOption}
customCommittedText={customCommittedText}
customDraft={draft}
isSubmitting={isSubmitting}
showAgentDisconnected={activeIndex === 0 && showAgentDisconnectedAtTop}
onSelectOption={onSelectOption}
onCustomDraftChange={(value) =>
setCustomDrafts((prev) => ({ ...prev, [meta.questionId]: value }))
}
onSubmitCustom={onSubmitCustom}
/>
{!isSingleQuestion && (
<ClarificationCarouselNav
activeIndex={activeIndex}
total={total}
isSubmitting={isSubmitting}
onPrev={() => setActiveIndex(Math.max(0, activeIndex - 1))}
onNext={() => setActiveIndex(Math.min(total - 1, activeIndex + 1))}
onSubmit={handleSubmit}
canSubmit={allAnswered}
/>
)}
<CarouselKeyboardShortcuts
meta={meta}
activeIndex={activeIndex}
total={total}
canSubmit={allAnswered}
onPick={(idx) => 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}
/>
</>
);
}
<div className="pr-6">
<div className="flex items-start gap-2 mb-1">
<IconMessageQuestion className="h-4 w-4 text-blue-500 flex-shrink-0 mt-0.5" />
<div className="markdown-body max-w-none text-sm [&>*:first-child]:mt-0 [&>*:last-child]:mb-0">
<ReactMarkdown remarkPlugins={remarkPlugins} components={markdownComponents}>
{question.prompt}
</ReactMarkdown>
</div>
export function ClarificationInputOverlay({
messages,
onResolved,
}: ClarificationInputOverlayProps) {
const sortedMessages = useMemo(
() => sortMessagesByQuestionIndex(resolveQuestionMessages(messages)),
[messages],
);
const group = useClarificationGroup(sortedMessages);
const [customDrafts, setCustomDrafts] = useState<Record<string, string>>({});
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 (
<div className="relative" data-testid="clarification-overlay">
<div className="flex items-center justify-between gap-3 px-4 pt-3 pb-2">
<div className="flex items-center gap-3">
{total > 1 && (
<ClarificationStepper
total={total}
activeIndex={activeIndex}
isAnswered={isAnsweredAt}
onJump={setActiveIndex}
isSubmitting={isSubmitting}
/>
)}
{total > 1 && (
<span
data-testid="clarification-group-progress"
className="text-xs text-muted-foreground"
>
{group.answeredCount} of {group.total} answered
</span>
)}
</div>
<ClarificationOptions
question={question}
isSubmitting={isSubmitting}
onSelectOption={handleSubmitOption}
/>
{metadata.agent_disconnected && (
<div
data-testid="clarification-deferred-notice"
className="ml-6 mb-1 text-xs text-amber-500/80 flex items-center gap-1.5"
>
<IconInfoCircle className="h-3.5 w-3.5 flex-shrink-0" />
The agent has moved on. Your response will be sent as a new message.
</div>
)}
<ClarificationCustomInput
customText={customText}
isSubmitting={isSubmitting}
onChange={setCustomText}
onSubmit={handleSubmitCustom}
/>
<button
type="button"
onClick={() => void group.skipAll("User skipped")}
disabled={isSubmitting}
className="text-muted-foreground hover:text-foreground cursor-pointer disabled:opacity-50"
data-testid="clarification-skip"
aria-label="Skip all questions"
>
<IconX className="h-4 w-4" />
</button>
</div>
<ClarificationCarouselBody
sortedMessages={sortedMessages}
group={group}
activeIndex={activeIndex}
setActiveIndex={setActiveIndex}
customDrafts={customDrafts}
setCustomDrafts={setCustomDrafts}
/>
</div>
);
}
export type { ClarificationAnswer };
@@ -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 (
<div
className="flex items-center gap-1.5 select-none"
role="tablist"
data-testid="clarification-stepper"
>
{Array.from({ length: total }).map((_, i) => {
const answered = isAnswered(i);
const active = i === activeIndex;
return (
<div key={i} className="flex items-center">
<button
type="button"
role="tab"
aria-selected={active}
aria-label={`Question ${i + 1} of ${total}${answered ? " (answered)" : ""}`}
onClick={() => onJump(i)}
disabled={isSubmitting}
data-testid="clarification-step"
data-step-index={String(i)}
data-active={active ? "true" : "false"}
data-answered={answered ? "true" : "false"}
className={cn(
"h-6 w-6 rounded-full text-[11px] font-semibold flex items-center justify-center transition-colors border cursor-pointer",
stepClassName(active, answered),
isSubmitting ? "opacity-60 cursor-not-allowed" : "",
)}
>
{answered && !active ? <IconCheck className="h-3 w-3" /> : i + 1}
</button>
{i < total - 1 && (
<div
aria-hidden="true"
className={cn("h-px w-5 mx-0.5", isAnswered(i) ? "bg-blue-500/50" : "bg-border")}
/>
)}
</div>
);
})}
</div>
);
}
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 (
<div className="space-y-1.5">
{options.map((option, idx) => {
const isSelected = selectedOption === option.option_id;
return (
<button
key={option.option_id}
type="button"
onClick={() => onSelectOption(option.option_id)}
disabled={isSubmitting}
data-testid="clarification-option"
data-selected={isSelected ? "true" : "false"}
className={cn(
"group flex items-start gap-3 w-full text-left text-sm rounded-lg px-3 py-2 transition-colors border",
isSelected
? "bg-blue-500/15 border-blue-500/50 text-foreground"
: "border-border hover:bg-muted/40 hover:border-border/80 text-foreground/90",
isSubmitting ? "opacity-60 cursor-not-allowed" : "cursor-pointer",
)}
>
<kbd
aria-hidden="true"
className="select-none font-mono text-[10px] px-1.5 py-0.5 rounded border border-border bg-muted text-muted-foreground leading-none mt-0.5"
>
{idx + 1}
</kbd>
<span className="flex-1 min-w-0">
<span
data-testid="clarification-option-label"
className="block leading-5 font-medium"
>
{option.label}
</span>
{option.description && (
<span
data-testid="clarification-option-description"
className="block text-muted-foreground/80 mt-0.5 text-xs leading-snug"
>
{option.description}
</span>
)}
</span>
{isSelected && <IconCheck className="h-3.5 w-3.5 text-blue-500 mt-1 flex-shrink-0" />}
</button>
);
})}
</div>
);
}
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 (
<div className="mt-2.5 flex items-center gap-2 px-3 py-2 rounded-lg border border-dashed border-border/70 bg-muted/30">
<span className="text-muted-foreground text-xs">↳</span>
<input
type="text"
placeholder={
committedText !== null ? "Press Enter to update your answer…" : "Or type a custom answer…"
}
value={draft}
onChange={(e) => 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());
}
}}
/>
<kbd
aria-hidden="true"
className="select-none flex items-center gap-1 font-mono text-[10px] px-1.5 py-0.5 rounded border border-border bg-background text-muted-foreground"
>
<IconCornerDownLeft className="h-2.5 w-2.5" />
Enter
</kbd>
</div>
);
}
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 (
<div className="flex items-center justify-between gap-2 px-4 pb-3">
<button
type="button"
onClick={onPrev}
disabled={isFirst || isSubmitting}
data-testid="clarification-prev"
className={cn(
"inline-flex items-center gap-1 text-xs px-2 py-1 rounded border",
isFirst
? "border-transparent text-muted-foreground/40 cursor-not-allowed"
: "border-border text-foreground/80 hover:bg-muted/50 cursor-pointer",
)}
>
<IconArrowLeft className="h-3 w-3" />
Back
</button>
{isLast ? (
<button
type="button"
onClick={onSubmit}
disabled={!canSubmit || isSubmitting}
data-testid="clarification-submit"
className={cn(
"inline-flex items-center gap-1 text-xs px-3 py-1 rounded font-medium",
canSubmit && !isSubmitting
? "bg-blue-500 text-white hover:bg-blue-500/90 cursor-pointer"
: "bg-muted text-muted-foreground cursor-not-allowed",
)}
>
{isSubmitting ? "Submitting…" : "Submit answers"}
<IconCheck className="h-3 w-3" />
</button>
) : (
<button
type="button"
onClick={onNext}
disabled={isSubmitting}
data-testid="clarification-next"
className="inline-flex items-center gap-1 text-xs px-2 py-1 rounded border border-border text-foreground/80 hover:bg-muted/50 cursor-pointer"
>
Next
<IconArrowRight className="h-3 w-3" />
</button>
)}
</div>
);
}
+2 -1
View File
@@ -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 && (
<div className="flex-shrink-0 border-t border-sky-400/30 bg-card px-1">
<ClarificationInputOverlay
message={pendingClarification}
messages={pendingClarificationGroup}
onResolved={handleClarificationResolved}
/>
</div>
+61
View File
@@ -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");
+320 -46
View File
@@ -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();
});
});
@@ -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");
});
});
@@ -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<string, ClarificationAnswer>;
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<string, ClarificationAnswer>) => Promise<void>;
skipAll: (reason?: string) => Promise<void>;
};
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<SubmitState> {
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<SubmitState> {
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<Record<string, ClarificationAnswer>>({});
const answersRef = useRef(answers);
useEffect(() => {
answersRef.current = answers;
}, [answers]);
const [submitState, setSubmitState] = useState<SubmitState>("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<string, ClarificationAnswer>) => {
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,
};
}
+9 -1
View File
@@ -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<MessageType> = 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,
};
}
+7
View File
@@ -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;
@@ -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>): 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"]);
});
});
@@ -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;
}