Files
kandev/apps/backend/internal/clarification/canceller_test.go
T
José Almeida bce9fd266a 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.
2026-05-05 19:14:57 +01:00

151 lines
5.0 KiB
Go

package clarification
import (
"context"
"errors"
"testing"
"time"
"github.com/kandev/kandev/internal/common/logger"
"github.com/kandev/kandev/internal/events/bus"
taskmodels "github.com/kandev/kandev/internal/task/models"
)
type stubMessageStore struct {
messages map[string][]*taskmodels.Message
updated []*taskmodels.Message
}
func (s *stubMessageStore) GetTaskSession(context.Context, string) (*taskmodels.TaskSession, error) {
return nil, errors.New("not implemented")
}
func (s *stubMessageStore) FindMessageByPendingID(_ context.Context, pendingID string) (*taskmodels.Message, error) {
msgs, ok := s.messages[pendingID]
if !ok || len(msgs) == 0 {
return nil, errors.New("not found")
}
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 {
s.updated = append(s.updated, m)
return nil
}
type stubEventBus struct {
events []*bus.Event
}
func (s *stubEventBus) Publish(_ context.Context, _ string, ev *bus.Event) error {
s.events = append(s.events, ev)
return nil
}
func newTestCanceller(t *testing.T, msgs map[string][]*taskmodels.Message) (*Canceller, *stubMessageStore, *stubEventBus) {
t.Helper()
store := NewStore(time.Minute)
repo := &stubMessageStore{messages: msgs}
eventBus := &stubEventBus{}
return NewCanceller(store, repo, eventBus, logger.Default()), repo, eventBus
}
// TestCanceller_MarksStatusExpiredOnDisconnect verifies that when the agent's
// turn ends with a pending clarification, the message is marked status=expired.
// This is what causes the frontend overlay to close — without it, the overlay
// stays interactive and a user clicking X triggers an unintended new turn via
// the event fallback path in the respond handler.
func TestCanceller_MarksStatusExpiredOnDisconnect(t *testing.T) {
msgs := map[string][]*taskmodels.Message{}
c, repo, _ := newTestCanceller(t, msgs)
pendingID := c.store.CreateRequest(&Request{SessionID: "s1"})
msgs[pendingID] = []*taskmodels.Message{{
ID: "m1",
TaskSessionID: "s1",
Metadata: map[string]any{
"status": "pending",
},
}}
cancelled := c.CancelSessionAndNotify(context.Background(), "s1")
if cancelled != 1 {
t.Fatalf("expected 1 cancelled, got %d", cancelled)
}
if len(repo.updated) != 1 {
t.Fatalf("expected 1 message update, got %d", len(repo.updated))
}
updated := repo.updated[0]
if got, _ := updated.Metadata["agent_disconnected"].(bool); !got {
t.Errorf("expected agent_disconnected=true, got %v", updated.Metadata["agent_disconnected"])
}
if got, _ := updated.Metadata["status"].(string); got != "expired" {
t.Errorf("expected status=expired, got %q", got)
}
}
// TestCanceller_NoMessagesToUpdate confirms that a cancel with no pending
// clarifications returns 0 and does not touch the repo.
func TestCanceller_NoMessagesToUpdate(t *testing.T) {
c, repo, _ := newTestCanceller(t, map[string][]*taskmodels.Message{})
if got := c.CancelSessionAndNotify(context.Background(), "nonexistent"); got != 0 {
t.Errorf("expected 0 cancelled, got %d", got)
}
if len(repo.updated) != 0 {
t.Errorf("expected no message updates, got %d", len(repo.updated))
}
}
// TestCanceller_PublishesMessageUpdatedEvent confirms the canceller publishes
// a message.updated event so the frontend refreshes the message in place.
func TestCanceller_PublishesMessageUpdatedEvent(t *testing.T) {
msgs := map[string][]*taskmodels.Message{}
c, _, eventBus := newTestCanceller(t, msgs)
pendingID := c.store.CreateRequest(&Request{SessionID: "s1"})
msgs[pendingID] = []*taskmodels.Message{{
ID: "m1",
TaskSessionID: "s1",
Metadata: map[string]any{"status": "pending"},
}}
c.CancelSessionAndNotify(context.Background(), "s1")
if len(eventBus.events) != 1 {
t.Fatalf("expected 1 event, got %d", len(eventBus.events))
}
}
// 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))
}
}