bce9fd266a
* 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.
247 lines
8.1 KiB
Go
247 lines
8.1 KiB
Go
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")
|
|
}
|