fix(mcp): correct ask_user_question_kandev schema docs and add consistency test (#1018)

2026-05-22 20:09:30 +01:00
parent 6ba50ba51e
commit 50fb93e04b
4 changed files with 141 additions and 2 deletions
@@ -44,7 +44,7 @@ TASK TOOLS:
 - update_task_state_kandev: Update task state. Required: task_id, state (TODO, CREATED, SCHEDULING, IN_PROGRESS, REVIEW, BLOCKED, WAITING_FOR_INPUT, COMPLETED, FAILED, CANCELLED).

 INTERACTION:
- ask_user_question_kandev: Ask the user a clarifying question with multiple-choice options. Required: prompt, options.
+- ask_user_question_kandev: Ask the user one or more clarifying questions in a single tool call. Required: questions (array of 1-4 question objects; each has prompt and options (2-6 {label, description})). Optional: context.

 EXAMPLE REQUESTS the user might ask:
 - "Create a new workflow called 'Feature Development'"
@@ -6,7 +6,7 @@ Kandev Session ID: {session_id}
 Use these IDs when calling tools that require task_id or session_id.

 Available tools:
- ask_user_question_kandev: Ask the user a clarifying question with multiple-choice options. Use this whenever you need user input before proceeding. Required params: prompt (string), options (array of {label, description}).
+- ask_user_question_kandev: Ask the user one or more clarifying questions in a single tool call. Use this whenever you need user input before proceeding. Required params: questions (array of 1-4 question objects; each object has prompt (string) and options (array of 2-6 {label, description})). Optional: context (string).
 - create_task_plan_kandev: Save an implementation plan for the current task. Required params: task_id, content (markdown). Optional: title.
 - get_task_plan_kandev: Retrieve the current plan for a task (includes any user edits). Required params: task_id.
 - update_task_plan_kandev: Update an existing plan. Required params: task_id, content (markdown). Optional: title.
@@ -1,6 +1,8 @@
 package mcp

 import (
+	"encoding/json"
+	"fmt"
 	"regexp"
 	"sort"
 	"strings"
@@ -218,3 +220,121 @@ func TestFindBareToolReferences_DistinguishesSuffixedFromBare(t *testing.T) {
 		})
 	}
 }
+
+// askUserQuestionSchemaFacts pulls the load-bearing structural facts from the
+// registered ask_user_question_kandev tool schema: the top-level array param
+// name, its min/max bounds, and the required sub-fields of each question
+// object. These are the facts the embedded prompt contexts must mirror.
+type askUserQuestionSchemaFacts struct {
+	paramName      string
+	minItems       int
+	maxItems       int
+	requiredFields []string
+}
+
+func extractAskUserQuestionFacts(t *testing.T, s *Server) askUserQuestionSchemaFacts {
+	t.Helper()
+
+	tool, ok := s.mcpServer.ListTools()["ask_user_question_kandev"]
+	require.True(t, ok, "ask_user_question_kandev not registered on this server")
+
+	raw, err := json.Marshal(tool.Tool.InputSchema)
+	require.NoError(t, err)
+
+	var parsed map[string]any
+	require.NoError(t, json.Unmarshal(raw, &parsed))
+
+	props, ok := parsed["properties"].(map[string]any)
+	require.True(t, ok, "schema must expose 'properties'")
+
+	// Collect all top-level array params and assert exactly one exists.
+	// Using a collect-then-assert pattern (rather than break-on-first) makes
+	// the selection deterministic even if Go's map iteration visits properties
+	// in a different order between runs or Go versions.
+	var arrayParams []string
+	paramSpecs := make(map[string]map[string]any)
+	for name, spec := range props {
+		m, ok := spec.(map[string]any)
+		if !ok {
+			continue
+		}
+		if m["type"] == "array" {
+			arrayParams = append(arrayParams, name)
+			paramSpecs[name] = m
+		}
+	}
+	sort.Strings(arrayParams)
+	require.Len(t, arrayParams, 1,
+		"ask_user_question schema must have exactly one top-level array parameter; got %v", arrayParams)
+
+	arrayParam := arrayParams[0]
+	arraySpec := paramSpecs[arrayParam]
+
+	minF, _ := arraySpec["minItems"].(float64)
+	maxF, _ := arraySpec["maxItems"].(float64)
+	require.NotZero(t, maxF,
+		"ask_user_question schema must declare maxItems on the %q array — if absent, bounds would silently become \"0-0\" and mask schema drift", arrayParam)
+
+	items, ok := arraySpec["items"].(map[string]any)
+	require.True(t, ok, "ask_user_question schema must declare an items schema on the %q array", arrayParam)
+	requiredRaw, ok := items["required"].([]any)
+	require.True(t, ok, "ask_user_question schema items must declare a required field list")
+	required := make([]string, 0, len(requiredRaw))
+	for _, r := range requiredRaw {
+		if field, ok := r.(string); ok {
+			required = append(required, field)
+		}
+	}
+	sort.Strings(required)
+
+	return askUserQuestionSchemaFacts{
+		paramName:      arrayParam,
+		minItems:       int(minF),
+		maxItems:       int(maxF),
+		requiredFields: required,
+	}
+}
+
+// TestAskUserQuestionDocs_MatchSchema binds the embedded prompt documentation
+// for ask_user_question_kandev to the actual MCP tool schema. If the schema
+// changes (param renamed, bounds adjusted, required sub-fields added or
+// removed) the prompt files must change with it — otherwise agents read stale
+// docs and send malformed payloads, which is exactly what cancelled task
+// 399f3f98 in v0.28.
+func TestAskUserQuestionDocs_MatchSchema(t *testing.T) {
+	log := newTestLogger(t)
+	backend := NewChannelBackendClient(log)
+	defer backend.Close()
+
+	taskServer := New(backend, "test-session", "test-task", 10005, log, "", false, ModeTask)
+	require.NotNil(t, taskServer)
+	configServer := New(backend, "test-session", "test-task", 10005, log, "", false, ModeConfig)
+	require.NotNil(t, configServer)
+
+	// Both modes register the same tool; assert that and pick one set of facts.
+	taskFacts := extractAskUserQuestionFacts(t, taskServer)
+	configFacts := extractAskUserQuestionFacts(t, configServer)
+	require.Equal(t, taskFacts, configFacts,
+		"ask_user_question_kandev schema differs between task and config modes — pick one source of truth")
+
+	facts := taskFacts
+	bounds := fmt.Sprintf("%d-%d", facts.minItems, facts.maxItems)
+
+	// Each prompt must mention the array param name, the bounds, and every
+	// required sub-field name. We assert phrases (not exact wording) so authors
+	// can rewrite the prose freely as long as the load-bearing facts survive.
+	cases := map[string]string{
+		"KandevContext": sysprompt.KandevContext(),
+		"ConfigContext": sysprompt.ConfigContext(),
+	}
+	for name, prompt := range cases {
+		assert.Contains(t, prompt, facts.paramName,
+			"%s must mention ask_user_question param name %q", name, facts.paramName)
+		assert.Contains(t, prompt, bounds,
+			"%s must mention the question-count bounds %q from the schema", name, bounds)
+		for _, field := range facts.requiredFields {
+			assert.Contains(t, prompt, field,
+				"%s must mention required question sub-field %q", name, field)
+		}
+	}
+}
@@ -253,6 +253,25 @@ func TestInterpolatePlaceholders_MultiplePlaceholders(t *testing.T) {
 	assert.Equal(t, "task-123 and task-123", result)
 }

+// --- ask_user_question schema documentation ---
+
+func TestContexts_DocumentCurrentAskUserQuestionSchema(t *testing.T) {
+	// Regression: the embedded prompt context used to document a legacy
+	// top-level `prompt` / `options` schema for ask_user_question_kandev.
+	// The real MCP tool requires a `questions` array of 1-4 question objects.
+	// Stale docs caused agents to send malformed payloads that landed in the
+	// approval layer as "0 questions" and were ultimately cancelled.
+	for name, ctx := range map[string]string{
+		"ConfigContext": ConfigContext(),
+		"KandevContext": KandevContext(),
+	} {
+		assert.Contains(t, ctx, "questions", "%s should mention the questions array param", name)
+		assert.Contains(t, ctx, "1-4 question objects", "%s should document the 1-4 question limit", name)
+		assert.NotContains(t, ctx, "Required params: prompt (string), options", "%s leaks the legacy ask_user_question schema", name)
+		assert.NotContains(t, ctx, "Required: prompt, options", "%s leaks the legacy ask_user_question schema", name)
+	}
+}
+
 // --- ConfigContext vs KandevContext distinction ---

 func TestConfigContext_DoesNotContainPlanTools(t *testing.T) {