diff --git a/apps/backend/AGENTS.md b/apps/backend/AGENTS.md index 778b3b73c6..df22b445b8 100644 --- a/apps/backend/AGENTS.md +++ b/apps/backend/AGENTS.md @@ -258,6 +258,8 @@ Every long-running goroutine must have a single owner with explicit start and st You may still list the column in the `CREATE TABLE` so fresh DBs get it inline, but the migration is the source of truth for evolution and must stand alone. New columns also need: the struct field in `models/`, the DTO field + `ToAPI` in `pkg/api/v1/`, and every `CreateX`/`UpdateX`/bulk write in the repo that should set it. +Built-in prompt content refreshes are seed-data migrations, not schema migrations. Match only known historical content hashes after applying the same normalization as the embedded prompt loader, require `created_at == updated_at` to preserve user edits, and use a conditional update over the original row values to avoid racing concurrent edits. Keep these refreshes with prompt seeding rather than `runMigrations()`. + ## Code-quality limits Enforced by `apps/backend/.golangci.yml` (errors on new code only): diff --git a/apps/backend/config/prompts/changes-walkthrough.md b/apps/backend/config/prompts/changes-walkthrough.md index c66cfd23aa..78498fa903 100644 --- a/apps/backend/config/prompts/changes-walkthrough.md +++ b/apps/backend/config/prompts/changes-walkthrough.md @@ -1,5 +1,10 @@ Please create an agent-authored walkthrough of the current changes using `show_walkthrough_kandev`. +Required tool shape: +- `steps` is an ordered array. +- Every step requires `file` (repository-relative path), `line` (positive 1-based integer), and `text` (markdown explanation). +- Optional step fields are `repo`, `title`, and `line_end`. + Walkthrough requirements: - Inspect the changed files yourself instead of relying on UI-provided paths or diff context. - If this is a PR task or PR review task, compare the PR head against the PR base branch. diff --git a/apps/backend/config/prompts/kandev-context.md b/apps/backend/config/prompts/kandev-context.md index b18361ed1f..1084c9fb18 100644 --- a/apps/backend/config/prompts/kandev-context.md +++ b/apps/backend/config/prompts/kandev-context.md @@ -11,6 +11,9 @@ Available tools: - 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. - delete_task_plan_kandev: Delete a task plan. Required params: task_id. +- show_walkthrough_kandev: Show and store a code walkthrough for the current task. Required param: `steps` (ordered array; every step requires `file`, `line`, and `text`). Optional: task_id, title; each step may include repo, title, and line_end. +- get_walkthrough_kandev: Retrieve the stored walkthrough for a task. Optional: task_id (defaults to the current task). +- delete_walkthrough_kandev: Delete the stored walkthrough for a task. Optional: task_id (defaults to the current task). - list_workspaces_kandev: List all workspaces. - list_workflows_kandev: List workflows in a workspace. Required params: workspace_id. - list_tasks_kandev: List tasks in a workflow. Required params: workflow_id. diff --git a/apps/backend/internal/mcp/server/sysprompt_sync_test.go b/apps/backend/internal/mcp/server/sysprompt_sync_test.go index e7f2392c53..4a9a61f7fc 100644 --- a/apps/backend/internal/mcp/server/sysprompt_sync_test.go +++ b/apps/backend/internal/mcp/server/sysprompt_sync_test.go @@ -8,6 +8,7 @@ import ( "strings" "testing" + promptcfg "github.com/kandev/kandev/config/prompts" "github.com/kandev/kandev/internal/sysprompt" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" @@ -425,3 +426,71 @@ func TestAskUserQuestionDocs_MatchSchema(t *testing.T) { } } } + +func extractWalkthroughStepRequiredFields(t *testing.T, s *Server) []string { + t.Helper() + + tool, ok := s.mcpServer.ListTools()["show_walkthrough_kandev"] + require.True(t, ok, "show_walkthrough_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)) + properties, ok := parsed["properties"].(map[string]any) + require.True(t, ok, "walkthrough schema must expose properties") + topLevelRequired, ok := parsed["required"].([]any) + require.True(t, ok, "walkthrough schema must declare top-level required fields") + require.Contains(t, topLevelRequired, "steps") + steps, ok := properties["steps"].(map[string]any) + require.True(t, ok, "walkthrough schema must expose steps") + require.Equal(t, "array", steps["type"]) + items, ok := steps["items"].(map[string]any) + require.True(t, ok, "walkthrough steps must declare an item schema") + requiredRaw, ok := items["required"].([]any) + require.True(t, ok, "walkthrough step schema must declare required fields") + + required := make([]string, 0, len(requiredRaw)) + for _, value := range requiredRaw { + field, ok := value.(string) + require.True(t, ok, "walkthrough required field must be a string") + required = append(required, field) + } + sort.Strings(required) + return required +} + +func TestWalkthroughDocs_MatchSchema(t *testing.T) { + log := newTestLogger(t) + backend := NewChannelBackendClient(log) + t.Cleanup(backend.Close) + + s := New(backend, "test-session", "test-task", 10005, log, "", false, ModeTask) + require.NotNil(t, s) + requiredFields := extractWalkthroughStepRequiredFields(t, s) + + context := sysprompt.KandevContext() + for _, toolName := range []string{ + "show_walkthrough_kandev", + "get_walkthrough_kandev", + "delete_walkthrough_kandev", + } { + assert.Contains(t, context, toolName, + "KandevContext must advertise task-mode walkthrough tool %q", toolName) + } + + for name, prompt := range map[string]string{ + "KandevContext": context, + "ChangesWalkthrough": promptcfg.Get("changes-walkthrough"), + } { + assert.Contains(t, prompt, "`steps`", + "%s must identify the walkthrough steps parameter", name) + assert.Contains(t, prompt, "ordered array", + "%s must say walkthrough steps preserve order", name) + for _, field := range requiredFields { + assert.Contains(t, prompt, fmt.Sprintf("`%s`", field), + "%s must identify required walkthrough step field %q", name, field) + } + } +} diff --git a/apps/backend/internal/mcp/server/tool_argument_validation.go b/apps/backend/internal/mcp/server/tool_argument_validation.go index 99db00d007..160f1fe5e0 100644 --- a/apps/backend/internal/mcp/server/tool_argument_validation.go +++ b/apps/backend/internal/mcp/server/tool_argument_validation.go @@ -5,10 +5,13 @@ import ( "errors" "fmt" "net/url" + "sort" + "strconv" "strings" "github.com/mark3labs/mcp-go/mcp" jsonschema "github.com/santhosh-tekuri/jsonschema/v6" + "github.com/santhosh-tekuri/jsonschema/v6/kind" "go.uber.org/zap" ) @@ -108,8 +111,22 @@ func sanitizedToolArgumentError(toolName string, err error) error { failure = validationErr keyword = "schema" } - return fmt.Errorf("invalid arguments for %s: validation failed at %s (keyword: %s)", - toolName, validationInstancePath(failure.InstanceLocation), keyword) + return fmt.Errorf("invalid arguments for %s: validation failed at %s (keyword: %s%s)", + toolName, validationInstancePath(failure.InstanceLocation), keyword, missingRequiredProperties(failure)) +} + +func missingRequiredProperties(err *jsonschema.ValidationError) string { + required, ok := err.ErrorKind.(*kind.Required) + if !ok || len(required.Missing) == 0 { + return "" + } + + missing := make([]string, len(required.Missing)) + for i, property := range required.Missing { + missing[i] = strconv.Quote(property) + } + sort.Strings(missing) + return "; missing: " + strings.Join(missing, ", ") } func firstKeywordFailure(err *jsonschema.ValidationError) (*jsonschema.ValidationError, string) { diff --git a/apps/backend/internal/mcp/server/tool_argument_validation_test.go b/apps/backend/internal/mcp/server/tool_argument_validation_test.go index 8c683d6338..5d54f4b6a1 100644 --- a/apps/backend/internal/mcp/server/tool_argument_validation_test.go +++ b/apps/backend/internal/mcp/server/tool_argument_validation_test.go @@ -33,6 +33,81 @@ func TestToolArgumentValidationRejectsUnknownTopLevelArgument(t *testing.T) { content.Text) } +func TestToolArgumentValidationNamesMissingWalkthroughStepProperty(t *testing.T) { + const rejectedValue = "private sibling value" + backend := &testBackend{} + s := newTaskModeServer(t, backend, "task-current") + + result := callTool(t, s, "show_walkthrough_kandev", map[string]interface{}{ + "steps": []interface{}{ + map[string]interface{}{ + "file": "example.go", + "line": 1, + "title": rejectedValue, + }, + }, + }) + + assert.True(t, result.IsError) + assert.Empty(t, backend.lastAction) + require.NotEmpty(t, result.Content) + content, ok := result.Content[0].(mcp.TextContent) + require.True(t, ok) + assert.Contains(t, content.Text, "/steps/0") + assert.Contains(t, content.Text, "keyword: required") + assert.Contains(t, content.Text, `missing: "text"`) + assert.NotContains(t, content.Text, rejectedValue) +} + +func TestToolArgumentValidationNamesEveryMissingWalkthroughStepProperty(t *testing.T) { + backend := &testBackend{} + s := newTaskModeServer(t, backend, "task-current") + + result := callTool(t, s, "show_walkthrough_kandev", map[string]interface{}{ + "steps": []interface{}{ + map[string]interface{}{"line": 1}, + }, + }) + + assert.True(t, result.IsError) + assert.Empty(t, backend.lastAction) + require.NotEmpty(t, result.Content) + content, ok := result.Content[0].(mcp.TextContent) + require.True(t, ok) + assert.Contains(t, content.Text, `missing: "file", "text"`) +} + +func TestToolArgumentValidationSortsMissingProperties(t *testing.T) { + const toolName = "unordered_required_properties_tool" + backend := &testBackend{} + s := newTaskModeServer(t, backend, "task-current") + s.mcpServer.AddTool( + mcp.NewToolWithRawSchema( + toolName, + "Validates deterministic missing-property diagnostics.", + json.RawMessage(`{ + "type": "object", + "properties": { + "alpha": {"type": "string"}, + "zeta": {"type": "string"} + }, + "required": ["zeta", "alpha"] + }`), + ), + s.wrapHandler(toolName, s.listWorkspacesHandler()), + ) + s.rebuildToolArgumentValidators() + + result := callTool(t, s, toolName, map[string]interface{}{}) + + assert.True(t, result.IsError) + assert.Empty(t, backend.lastAction) + require.NotEmpty(t, result.Content) + content, ok := result.Content[0].(mcp.TextContent) + require.True(t, ok) + assert.Contains(t, content.Text, `missing: "alpha", "zeta"`) +} + func TestToolArgumentValidation(t *testing.T) { t.Run("accepts an empty object for a parameterless tool", func(t *testing.T) { backend := &testBackend{ diff --git a/apps/backend/internal/prompts/store/sqlite.go b/apps/backend/internal/prompts/store/sqlite.go index 6c2d8078fb..17d2f43c60 100644 --- a/apps/backend/internal/prompts/store/sqlite.go +++ b/apps/backend/internal/prompts/store/sqlite.go @@ -2,6 +2,8 @@ package store import ( "context" + "crypto/sha256" + "database/sql" "errors" "fmt" "strings" @@ -14,6 +16,13 @@ import ( "github.com/kandev/kandev/internal/prompts/models" ) +const ( + builtinChangesWalkthroughPromptID = "builtin-changes-walkthrough" + // Historical prompt hashes use the same TrimSpace normalization as promptcfg.Get. + changesWalkthroughV1SHA256 = "23a82694ef3b6d0220da2879c1c351cf5ee4926c2bc54a52fa4f7d5182bcb111" + changesWalkthroughV2SHA256 = "7a28dc81df4bff75b4fb8d66d6b9118febe5daf7f4b570e3b8ef8c74ac3e3146" +) + type sqliteRepository struct { db *sqlx.DB // writer ro *sqlx.DB // reader @@ -181,10 +190,58 @@ func (r *sqliteRepository) seedBuiltinPrompts() error { if err != nil { return fmt.Errorf("failed to upsert built-in prompt %s: %w", prompt.ID, err) } + if prompt.ID == builtinChangesWalkthroughPromptID { + if err := r.refreshLegacyChangesWalkthroughPrompt(prompt); err != nil { + return err + } + } } return nil } +// refreshLegacyChangesWalkthroughPrompt updates exact, untouched revisions +// shipped before the required step shape was documented. User edits change +// updated_at, and the conditional update prevents racing a concurrent edit. +func (r *sqliteRepository) refreshLegacyChangesWalkthroughPrompt(current *models.Prompt) error { + var storedContent string + var builtinInt int + var createdAt, updatedAt time.Time + err := r.db.QueryRow(r.db.Rebind(` + SELECT content, builtin, created_at, updated_at + FROM custom_prompts + WHERE id = ? + `), current.ID).Scan(&storedContent, &builtinInt, &createdAt, &updatedAt) + if errors.Is(err, sql.ErrNoRows) { + return nil + } + if err != nil { + return fmt.Errorf("read stored changes walkthrough prompt: %w", err) + } + if builtinInt != 1 || !createdAt.Equal(updatedAt) || !isLegacyChangesWalkthroughPrompt(storedContent) { + return nil + } + + _, err = r.db.Exec(r.db.Rebind(` + UPDATE custom_prompts + SET content = ? + WHERE id = ? AND builtin = 1 AND content = ? AND created_at = ? AND updated_at = ? + `), current.Content, current.ID, storedContent, createdAt, updatedAt) + if err != nil { + return fmt.Errorf("refresh stored changes walkthrough prompt: %w", err) + } + return nil +} + +func isLegacyChangesWalkthroughPrompt(content string) bool { + hash := fmt.Sprintf("%x", sha256.Sum256([]byte(content))) + switch hash { + case changesWalkthroughV1SHA256, changesWalkthroughV2SHA256: + return true + default: + return false + } +} + // getBuiltinPrompts returns the predefined built-in prompts loaded from embedded markdown files. func (r *sqliteRepository) getBuiltinPrompts() []*models.Prompt { now := time.Now().UTC() @@ -193,6 +250,6 @@ func (r *sqliteRepository) getBuiltinPrompts() []*models.Prompt { {ID: "builtin-open-pr", Name: "open-pr", Builtin: true, CreatedAt: now, UpdatedAt: now, Content: promptcfg.Get("open-pr")}, {ID: "builtin-merge-base", Name: "merge-base", Builtin: true, CreatedAt: now, UpdatedAt: now, Content: promptcfg.Get("merge-base")}, {ID: "builtin-ci-auto-fix", Name: "ci-auto-fix", Builtin: true, CreatedAt: now, UpdatedAt: now, Content: promptcfg.Get("ci-auto-fix")}, - {ID: "builtin-changes-walkthrough", Name: "changes-walkthrough", Builtin: true, CreatedAt: now, UpdatedAt: now, Content: promptcfg.Get("changes-walkthrough")}, + {ID: builtinChangesWalkthroughPromptID, Name: "changes-walkthrough", Builtin: true, CreatedAt: now, UpdatedAt: now, Content: promptcfg.Get("changes-walkthrough")}, } } diff --git a/apps/backend/internal/prompts/store/sqlite_test.go b/apps/backend/internal/prompts/store/sqlite_test.go index 25cb47db34..3d5463abd3 100644 --- a/apps/backend/internal/prompts/store/sqlite_test.go +++ b/apps/backend/internal/prompts/store/sqlite_test.go @@ -2,12 +2,14 @@ package store import ( "context" + "os" "path/filepath" "strings" "testing" "time" "github.com/jmoiron/sqlx" + promptcfg "github.com/kandev/kandev/config/prompts" "github.com/kandev/kandev/internal/db" "github.com/kandev/kandev/internal/prompts/models" ) @@ -35,6 +37,52 @@ func createTestRepo(t *testing.T) (*sqliteRepository, func()) { return repo, cleanup } +func createUnseededPromptDB(t *testing.T) *sqlx.DB { + t.Helper() + dbConn, err := db.OpenSQLite(filepath.Join(t.TempDir(), "test.db")) + if err != nil { + t.Fatalf("failed to open sqlite db: %v", err) + } + sqlxDB := sqlx.NewDb(dbConn, "sqlite3") + t.Cleanup(func() { + if err := sqlxDB.Close(); err != nil { + t.Errorf("failed to close sqlite db: %v", err) + } + }) + if _, err := sqlxDB.Exec(` + CREATE TABLE custom_prompts ( + id TEXT PRIMARY KEY, + name TEXT NOT NULL UNIQUE, + content TEXT NOT NULL, + builtin INTEGER NOT NULL DEFAULT 0, + created_at TIMESTAMP NOT NULL, + updated_at TIMESTAMP NOT NULL + ); + `); err != nil { + t.Fatalf("create schema: %v", err) + } + return sqlxDB +} + +func seedStoredWalkthroughPrompt(t *testing.T, sqlxDB *sqlx.DB, content string, createdAt, updatedAt time.Time) { + t.Helper() + if _, err := sqlxDB.Exec( + `INSERT INTO custom_prompts (id, name, content, builtin, created_at, updated_at) VALUES (?, ?, ?, 1, ?, ?)`, + "builtin-changes-walkthrough", "changes-walkthrough", content, createdAt, updatedAt, + ); err != nil { + t.Fatalf("seed stored walkthrough prompt: %v", err) + } +} + +func readPromptFixture(t *testing.T, name string) string { + t.Helper() + content, err := os.ReadFile(filepath.Join("testdata", name)) + if err != nil { + t.Fatalf("read prompt fixture: %v", err) + } + return string(content) +} + func TestSQLiteRepository_CRUD(t *testing.T) { repo, cleanup := createTestRepo(t) defer cleanup() @@ -222,3 +270,67 @@ func TestSQLiteRepository_BuiltinSeedIgnoresUserPromptNameConflict(t *testing.T) t.Fatalf("expected user prompt to remain canonical, got %+v", got) } } + +func TestSQLiteRepository_RefreshesUnmodifiedLegacyChangesWalkthroughPrompt(t *testing.T) { + for _, fixture := range []string{ + "changes-walkthrough-v1.md", + "changes-walkthrough-v2.md", + } { + t.Run(fixture, func(t *testing.T) { + sqlxDB := createUnseededPromptDB(t) + legacyContent := strings.TrimSpace(readPromptFixture(t, fixture)) + now := time.Now().UTC() + seedStoredWalkthroughPrompt(t, sqlxDB, legacyContent, now, now) + + repo, err := newSQLiteRepositoryWithDB(sqlxDB, sqlxDB) + if err != nil { + t.Fatalf("initialize repository: %v", err) + } + got, err := repo.GetPromptByName(context.Background(), "changes-walkthrough") + if err != nil { + t.Fatalf("get refreshed prompt: %v", err) + } + if got.Content != promptcfg.Get("changes-walkthrough") { + t.Fatalf("stored legacy prompt was not refreshed") + } + }) + } +} + +func TestSQLiteRepository_PreservesEditedLegacyChangesWalkthroughPrompt(t *testing.T) { + sqlxDB := createUnseededPromptDB(t) + legacyContent := readPromptFixture(t, "changes-walkthrough-v2.md") + createdAt := time.Now().UTC().Add(-time.Hour) + seedStoredWalkthroughPrompt(t, sqlxDB, legacyContent, createdAt, createdAt.Add(time.Minute)) + + repo, err := newSQLiteRepositoryWithDB(sqlxDB, sqlxDB) + if err != nil { + t.Fatalf("initialize repository: %v", err) + } + got, err := repo.GetPromptByName(context.Background(), "changes-walkthrough") + if err != nil { + t.Fatalf("get edited prompt: %v", err) + } + if got.Content != legacyContent { + t.Fatalf("user-edited built-in prompt was overwritten") + } +} + +func TestSQLiteRepository_PreservesUntouchedUnrecognizedChangesWalkthroughPrompt(t *testing.T) { + sqlxDB := createUnseededPromptDB(t) + customContent := "this is not a recognized legacy revision" + now := time.Now().UTC() + seedStoredWalkthroughPrompt(t, sqlxDB, customContent, now, now) + + repo, err := newSQLiteRepositoryWithDB(sqlxDB, sqlxDB) + if err != nil { + t.Fatalf("initialize repository: %v", err) + } + got, err := repo.GetPromptByName(context.Background(), "changes-walkthrough") + if err != nil { + t.Fatalf("get unrecognized prompt: %v", err) + } + if got.Content != customContent { + t.Fatalf("unrecognized untouched content was overwritten") + } +} diff --git a/apps/backend/internal/prompts/store/testdata/changes-walkthrough-v1.md b/apps/backend/internal/prompts/store/testdata/changes-walkthrough-v1.md new file mode 100644 index 0000000000..cba51bb9e9 --- /dev/null +++ b/apps/backend/internal/prompts/store/testdata/changes-walkthrough-v1.md @@ -0,0 +1,12 @@ +Please create an agent-authored walkthrough of the current changes using `show_walkthrough_kandev`. + +Walkthrough requirements: +- Use only files listed below or files you verify exist in this task's local worktree/review diff. +- For PR-only files, do not assume the PR head is checked out locally; anchor to the review diff when available, and avoid editor-only/current-worktree claims. +- Anchor steps to changed lines or changed line ranges whenever possible. +- Use `line_end` whenever a logical explanation spans multiple lines; prefer one range step over adjacent single-line steps. +- Keep each step concise and direct. Do not include a `Justification:` preamble. +- If a good local/review anchor is unavailable, omit that step instead of referencing a remote-only path. + +Available changed files: +{{changed_files}} diff --git a/apps/backend/internal/prompts/store/testdata/changes-walkthrough-v2.md b/apps/backend/internal/prompts/store/testdata/changes-walkthrough-v2.md new file mode 100644 index 0000000000..c66cfd23aa --- /dev/null +++ b/apps/backend/internal/prompts/store/testdata/changes-walkthrough-v2.md @@ -0,0 +1,14 @@ +Please create an agent-authored walkthrough of the current changes using `show_walkthrough_kandev`. + +Walkthrough requirements: +- Inspect the changed files yourself instead of relying on UI-provided paths or diff context. +- If this is a PR task or PR review task, compare the PR head against the PR base branch. +- Otherwise, compare against the task/repository base using the local task state (`git status`, `git diff`, `git diff --cached`, and cumulative diff context when available). +- For PR-only files, do not assume the PR head is checked out locally; anchor to the review diff when available, and avoid editor-only/current-worktree claims. +- The first walkthrough step must contextualize the whole change before explaining individual hunks. +- Anchor the first walkthrough step to the most representative changed line or changed line range available. +- Include an `ELI5:` line in the first walkthrough step text that explains the change in the simplest terms possible. +- Anchor steps to changed lines or changed line ranges whenever possible. +- Use `line_end` whenever a logical explanation spans multiple lines; prefer one range step over adjacent single-line steps. +- Keep each step concise and direct. Do not include a `Justification:` preamble. +- If a good local/review anchor is unavailable, omit that step instead of referencing a remote-only path. diff --git a/docs/plans/mcp-walkthrough-call-recovery/plan.md b/docs/plans/mcp-walkthrough-call-recovery/plan.md new file mode 100644 index 0000000000..ce5cd19d7f --- /dev/null +++ b/docs/plans/mcp-walkthrough-call-recovery/plan.md @@ -0,0 +1,175 @@ +--- +spec: docs/specs/integrations/mcp-tool-argument-validation.md +created: 2026-08-03 +status: done +--- + +# Fix Plan: Recover Invalid Walkthrough MCP Calls + +## Overview + +Make shared MCP validation errors actionable without weakening their redaction, +then put the load-bearing walkthrough call shape in both task context and the +built-in walkthrough request. Bind those instructions to the registered schema +with focused tests and update the public MCP reference. + +## Confirmed root cause + +`sanitizedToolArgumentError` reduces a JSON Schema `required` failure to its +instance path and keyword. A walkthrough step missing `text` therefore returns +`validation failed at /steps/0 (keyword: required)` and never reaches the +walkthrough service's more specific `step 1 text is required` validation. The +shared validator landed on 2026-08-01 and now intercepts this malformed call +before the tool-specific handler. + +The registered `show_walkthrough_kandev` schema correctly requires `file`, +`line`, and `text`, but the injected task context omits all walkthrough tools +and the built-in `changes-walkthrough` prompt never states the argument shape. +Clients or models that do not preserve nested schema detail therefore receive +weak initial guidance followed by an error that does not identify the missing +field. + +Smallest reproduction: call `show_walkthrough_kandev` with +`steps: [{"file":"example.go","line":1,"title":"Explanation"}]`. The +advertised schema contains `required: ["file", "line", "text"]`, but the +returned error does not contain `text` and no backend action runs. + +## Backend + +### Actionable required-property errors + +- `apps/backend/internal/mcp/server/tool_argument_validation.go` — recognize + the validator library's `kind.Required` error and append its schema-defined + missing property names to the sanitized result. Preserve the current path and + keyword for compatibility and retain the existing generic formatting for all + other constraints. +- Never format a generic validator message or rejected value. Only the missing + names declared by the registered schema may enter the error. +- Keep fail-closed validation, root closure, open nested maps, mode rebuilds, + handler suppression, and create-task compatibility normalization unchanged. + +### Existing-install prompt refresh + +- `apps/backend/internal/prompts/store/sqlite.go` — refresh only the exact + loader-normalized stored content of the two historical `changes-walkthrough` + built-in revisions that predate the required step shape and have never been + saved by a user. +- Preserve unrecognized content, edited built-ins, and user-owned name + conflicts. Keep the conditional write race-safe so an edit concurrent with + startup cannot be overwritten. + +## Agent prompt contract + +- `apps/backend/config/prompts/kandev-context.md` — list + `show_walkthrough_kandev`, `get_walkthrough_kandev`, and + `delete_walkthrough_kandev`. State that `show` requires an ordered `steps` + array and every step requires `file`, `line`, and `text`; keep optional + top-level and step fields concise. +- `apps/backend/config/prompts/changes-walkthrough.md` — repeat the exact + required step shape next to the existing generation requirements. Do not add + task-specific paths, diff contents, or a copied sample that an agent could + submit literally. + +## Public documentation + +Update the MCP validation contract in +`docs/public/automation-and-mcp.md` to state that `required` failures name +missing schema properties while never echoing argument values. The existing +walkthrough guide already documents that each step needs text, a file, and a +positive line, so it needs no behavior change. + +Primary content type: reference. + +## Tests + +- **What:** A nested missing-required-property failure names `text`, retains + `/steps/0` and `required`, does not expose sibling values, and dispatches no + backend action. + **File:** + `apps/backend/internal/mcp/server/tool_argument_validation_test.go`. + **How:** Call the real registered `show_walkthrough_kandev` tool through + `wrapHandler` with the minimal malformed step. Write this regression first + and confirm it fails because the current result omits `text`. +- **What:** Walkthrough instructions stay aligned with the live tool schema. + **File:** `apps/backend/internal/mcp/server/sysprompt_sync_test.go`. + **How:** Extract the `steps` item required fields from the registered tool, + assert the first-turn task context names all three walkthrough tools, and + assert both task context and embedded `changes-walkthrough` content mention + `steps` plus every required item field. +- **What:** Public documentation remains structurally valid. + **File:** `docs/public/automation-and-mcp.md`. + **How:** Run both public-doc validation scripts. +- **What:** Existing installations receive the corrected walkthrough request + without losing prompt customizations. + **File:** `apps/backend/internal/prompts/store/sqlite_test.go` plus historical + fixtures under `apps/backend/internal/prompts/store/testdata/`. + **How:** Initialize repositories over each shipped legacy revision in the + exact trimmed form stored by `promptcfg.Get` and require refresh, then prove + unrecognized content, a saved built-in, and a user-owned name conflict remain + unchanged. + +No browser E2E is needed: this changes agent-facing MCP diagnostics and hidden +prompt instructions, not rendered UI behavior. + +## Verification Results + +Completed on 2026-08-03 with strict red-green coverage: + +- Focused RED tests failed because required-property diagnostics omitted + `text`, task context omitted the walkthrough tools, and neither prompt surface + documented the step shape. +- The same focused tests passed after implementation, including coverage for + multiple missing properties and submitted-value redaction. +- `cd apps/backend && go test ./internal/mcp/server ./internal/sysprompt` — + passed. +- `node --test scripts/validate-public-docs.test.mjs` — passed, 58 tests. +- `node scripts/validate-public-docs.mjs` — passed, 41 published pages. +- `git diff --check` — passed. + +PR review remediation also passed: + +- Focused RED tests reproduced stale stored prompts and declaration-ordered + missing-property output. +- Focused GREEN tests passed for both shipped legacy prompt revisions, edited + prompt preservation, canonical property sorting, the top-level `steps` + contract, and immediate test cleanup. +- `cd apps/backend && go test ./internal/prompts/store ./internal/mcp/server ./internal/sysprompt` — passed. +- `cd apps/backend && golangci-lint run ./... --new-from-rev="0ca00c2aca4430a5e4f06874ba960743de1acf9a" --timeout=5m` — passed with 0 issues. + +Follow-up review remediation also passed: + +- Focused RED coverage seeded both historical revisions in the exact trimmed + form produced by `promptcfg.Get`; both remained stale with the fixture-file + hashes. +- Focused GREEN coverage passed after hashing the loader-normalized historical + contents. Equal-timestamp unrecognized content and edited content remain + unchanged. +- `cd apps/backend && go test ./internal/prompts/store ./internal/mcp/server ./internal/sysprompt` — passed. +- `cd apps/backend && golangci-lint run ./... --new-from-rev="0ca00c2aca4430a5e4f06874ba960743de1acf9a" --timeout=5m` — passed with 0 issues. +- `node --test scripts/validate-public-docs.test.mjs` — passed, 58 tests. +- `node scripts/validate-public-docs.mjs` — passed, 41 published pages. +- `git diff --check` — passed. + +No browser E2E was run because the change affects MCP diagnostics and embedded +agent prompts, not rendered UI behavior. + +## Implementation Wave + +- [x] [Task 01: Make walkthrough call recovery actionable](task-01-actionable-walkthrough-call-recovery.md) — done. + +Execution stays sequential in the primary conversation. No subagents are +authorized. + +## Risks and out of scope + +- Calling a generic localized validator formatter could expose rejected values; + inspect only the typed `required` error's missing schema names. +- Keep existing non-`required` error text stable to avoid broadening this fix. +- Do not alter the walkthrough schema, persistence schema, unrelated + persistence behavior, rendering, MCP transport, authorization, or retry + behavior. +- Task context receives only the walkthrough contract needed for reliable calls; + this work does not mirror all registered tool schemas into that prompt. +- Exact offending ACP frames were not available, so this repair hardens both + documented contributors without claiming whether one client dropped `text` + or one model omitted it. diff --git a/docs/plans/mcp-walkthrough-call-recovery/task-01-actionable-walkthrough-call-recovery.md b/docs/plans/mcp-walkthrough-call-recovery/task-01-actionable-walkthrough-call-recovery.md new file mode 100644 index 0000000000..c3b505db74 --- /dev/null +++ b/docs/plans/mcp-walkthrough-call-recovery/task-01-actionable-walkthrough-call-recovery.md @@ -0,0 +1,127 @@ +--- +id: "mcp-walkthrough-call-recovery-01" +title: "Make walkthrough call recovery actionable" +status: done +wave: 1 +depends_on: [] +plan: "plan.md" +spec: "../../specs/integrations/mcp-tool-argument-validation.md" +--- + +# Task 01: Make Walkthrough Call Recovery Actionable + +## Acceptance + +1. A `show_walkthrough_kandev` call whose step omits `text` fails before + backend dispatch and names `/steps/0`, the `required` constraint, and the + missing `text` schema property without exposing any submitted value. +2. First-turn task context names show/get/delete walkthrough tools, and both it + and the built-in walkthrough request state that every `steps` item requires + `file`, `line`, and `text`, with tests derived from the registered schema. +3. Public MCP reference documents missing-property diagnostics; all focused Go + and public-doc checks pass. +4. Existing installations refresh untouched historical walkthrough prompts in + their loader-normalized stored form to the corrected embedded content + without overwriting unrecognized content, user edits, or user-owned prompt + conflicts. + +## Verification + +Follow strict TDD, then run: + +```bash +cd apps/backend +go test ./internal/prompts/store ./internal/mcp/server ./internal/sysprompt +cd ../.. +node --test scripts/validate-public-docs.test.mjs +node scripts/validate-public-docs.mjs +git diff --check +``` + +## Files likely touched + +- `apps/backend/internal/mcp/server/tool_argument_validation.go` +- `apps/backend/internal/mcp/server/tool_argument_validation_test.go` +- `apps/backend/internal/mcp/server/sysprompt_sync_test.go` +- `apps/backend/internal/prompts/store/sqlite.go` +- `apps/backend/internal/prompts/store/sqlite_test.go` +- `apps/backend/internal/prompts/store/testdata/` +- `apps/backend/config/prompts/kandev-context.md` +- `apps/backend/config/prompts/changes-walkthrough.md` +- `docs/public/automation-and-mcp.md` +- `docs/specs/integrations/mcp-tool-argument-validation.md` +- `docs/specs/INDEX.md` +- `docs/plans/mcp-walkthrough-call-recovery/plan.md` +- `docs/plans/mcp-walkthrough-call-recovery/task-01-actionable-walkthrough-call-recovery.md` + +## Dependencies + +None. + +## Parallelism + +`sequential` — validator behavior, schema-derived prompt coverage, and contract +documentation form one regression repair. + +## Inputs + +- [MCP Tool Argument Validation spec](../../specs/integrations/mcp-tool-argument-validation.md) +- [Fix plan](plan.md) +- `docs/decisions/2026-08-01-validate-mcp-tool-arguments.md` +- `sanitizedToolArgumentError`, `firstKeywordFailure`, and + `buildWalkthroughStepSchemaItem` +- `TestAskUserQuestionDocs_MatchSchema` as schema-to-prompt consistency pattern +- Built-in `changes-walkthrough` prompt and task-mode Kandev context + +## Risks + +- Do not expose rejected argument values through a generic validator message. +- Preserve error formatting for every non-`required` validation keyword. +- Do not change nested-map openness or add client-specific retries. + +## Output contract + +Report red/green test evidence, exact error shape, prompt and public-doc files, +all command results, residual risks, and synchronized task/plan/spec statuses. + +## Results + +Completed on 2026-08-03. + +- RED: focused server tests failed on the old behavior because the validation + error did not name missing `text`, task context did not advertise the + walkthrough tools, and the embedded prompts did not state the required item + shape. +- GREEN: required failures now retain the object path and keyword while adding + quoted schema property names, for example + `validation failed at /steps/0 (keyword: required; missing: "text")`. + Submitted argument values remain absent and invalid calls still do not reach + backend dispatch. +- Task context now lists show/get/delete walkthrough tools. It and the built-in + walkthrough request both describe `steps` as an ordered array whose items + require `file`, `line`, and `text`; a schema-derived regression test guards + this contract. +- Public MCP reference now documents the actionable, redacted missing-property + behavior. +- `cd apps/backend && go test ./internal/mcp/server ./internal/sysprompt` — + passed. +- `node --test scripts/validate-public-docs.test.mjs` — passed, 58 tests. +- `node scripts/validate-public-docs.mjs` — passed, 41 published pages. +- `git diff --check` — passed. +- Browser E2E was intentionally not run because no rendered UI behavior + changed. +- PR review remediation refreshes the exact loader-normalized stored content of + the two untouched historical prompt revisions, preserves unrecognized + content, edited built-ins, and user-owned conflicts, sorts missing schema + properties deterministically, asserts top-level `steps` requiredness, and + registers backend cleanup with `t.Cleanup`. +- Remediation RED tests reproduced both stale stored revisions and unsorted + output; the focused GREEN regressions passed afterward. +- `cd apps/backend && go test ./internal/prompts/store ./internal/mcp/server ./internal/sysprompt` — passed. +- `cd apps/backend && golangci-lint run ./... --new-from-rev="0ca00c2aca4430a5e4f06874ba960743de1acf9a" --timeout=5m` — passed with 0 issues. +- Follow-up review RED coverage proved the fixture-file hashes did not recognize + historical content in the exact trimmed form stored by `promptcfg.Get`. +- Follow-up GREEN coverage uses loader-normalized historical hashes and proves + equal-timestamp unrecognized content remains unchanged. +- The affected Go packages, changed-code lint, 58 public-doc tests, 41-page + public-doc validation, and `git diff --check` all passed again. diff --git a/docs/public/automation-and-mcp.md b/docs/public/automation-and-mcp.md index b34efba70e..fb8acba11a 100644 --- a/docs/public/automation-and-mcp.md +++ b/docs/public/automation-and-mcp.md @@ -18,7 +18,7 @@ Kandev has several mechanisms that can act without repeated manual setup. Their Use workflow events for predictable transitions on existing work. Use a workspace automation when an external signal must create new work. MCP is a tool interface, not a scheduler. -Across Kandev's task, configuration, external, and Office MCP modes, each tool call is validated against that mode's live `tools/list` schema before its handler runs. Missing required fields, wrong types, declared constraint violations, and unknown top-level fields return a tool error without performing the requested action. Nested configuration maps still accept arbitrary keys when their schema defines them as open. +Across Kandev's task, configuration, external, and Office MCP modes, each tool call is validated against that mode's live `tools/list` schema before its handler runs. Missing required fields, wrong types, declared constraint violations, and unknown top-level fields return a tool error without performing the requested action. A missing-field error names each absent schema property, but never echoes submitted argument values. Nested configuration maps still accept arbitrary keys when their schema defines them as open. ## Quick path diff --git a/docs/specs/integrations/mcp-tool-argument-validation.md b/docs/specs/integrations/mcp-tool-argument-validation.md index e03f09d983..28da76e734 100644 --- a/docs/specs/integrations/mcp-tool-argument-validation.md +++ b/docs/specs/integrations/mcp-tool-argument-validation.md @@ -10,17 +10,20 @@ Decision: [ADR-2026-08-01-validate-mcp-tool-arguments](../../decisions/2026-08-0 ## Why -Agents and external MCP clients need a failed tool call to be distinguishable from a successful operation. Silently ignoring an incorrect argument can create tasks, start agents, or mutate configuration without the caller's intended data while still reporting success. +Agents and external MCP clients need a failed tool call to be distinguishable from a successful operation and specific enough to repair safely. Silently ignoring an incorrect argument can create tasks, start agents, or mutate configuration without the caller's intended data, while a generic constraint failure can leave an agent repeating the same malformed call. ## What - Every Kandev MCP tool validates its arguments against the schema registered for its current MCP mode before running its handler. - Missing required arguments, values of the wrong declared type, violated declared constraints, and unknown top-level arguments return a tool error and cause no handler or backend side effect. +- A missing-required-property error names the missing schema property or properties as well as the invalid object location and `required` constraint, without returning argument values. - Parameterless tools accept omitted arguments or an empty object and reject any supplied field. - Validation follows mode changes: after the server changes its registered tool set, calls use the replacement tools' schemas. - Nested configuration objects remain open when their schema intentionally permits arbitrary keys. - `create_task_kandev` advertises `prompt` for the text delivered to a newly started agent. It accepts the former `description` name as an unadvertised compatibility alias, without adding a second field or explanation to the tool schema. A call containing both names fails rather than choosing one silently. - The backend task action continues receiving the text in its existing `description` field; this naming convergence changes only the MCP boundary. +- Task-mode agent context lists the walkthrough tools and states that every `show_walkthrough_kandev.steps` item requires `file`, `line`, and `text`. The built-in `changes-walkthrough` request repeats that load-bearing call shape so callers do not depend on a client rendering nested JSON Schema correctly. +- On startup, a stored built-in `changes-walkthrough` prompt that exactly matches the loader-normalized content of an untouched shipped revision is refreshed to the current embedded prompt. User-edited built-ins, unrecognized stored content, and user-owned prompts remain unchanged. ## API surface @@ -30,18 +33,20 @@ The advertised MCP `tools/list` schemas do not gain repeated unknown-argument me 2. Validate the resulting arguments against the currently registered tool schema with the root object closed to unknown fields. 3. Invoke the handler only when validation succeeds. -Validation failures are returned as MCP tool error results. They identify the invalid argument location and violated constraint without returning sensitive argument values. +Validation failures are returned as MCP tool error results. They identify the invalid argument location and violated constraint without returning sensitive argument values. For the `required` constraint, the result also identifies each missing property by its schema-defined name. ## Failure modes - If tool arguments fail validation, Kandev returns an error and does not invoke the handler. - If a registered tool schema cannot compile, calls to that tool fail closed and Kandev logs the schema defect; test coverage prevents shipping an uncompilable built-in schema. - If both `prompt` and compatibility alias `description` are supplied to `create_task_kandev`, Kandev returns an error and creates no task. +- Missing-property diagnostics expose only names declared by the registered schema, never sibling values or rejected argument contents. +- Prompt refresh recognizes only exact stored content whose hash matches a loader-normalized historical built-in revision and that has never been saved by a user; unknown or edited content is left intact. - Validation errors do not echo complete prompts, credentials, configuration values, or other argument contents. ## Scenarios -- **GIVEN** any registered Kandev MCP tool, **WHEN** a caller omits one of its schema-required arguments, **THEN** the call returns a tool error and its handler is not invoked. +- **GIVEN** any registered Kandev MCP tool, **WHEN** a caller omits one or more schema-required properties, **THEN** the call returns a tool error naming the invalid object location, the `required` constraint, and every missing schema property, and its handler is not invoked. - **GIVEN** any registered Kandev MCP tool, **WHEN** a caller supplies a value with the wrong schema type, **THEN** the call returns a tool error and its handler is not invoked. - **GIVEN** any registered Kandev MCP tool, **WHEN** a caller supplies an unknown top-level argument, **THEN** the call returns a tool error naming its location and its handler is not invoked. - **GIVEN** a parameterless tool, **WHEN** a caller omits arguments or supplies `{}`, **THEN** the handler runs normally; **WHEN** it supplies any field, **THEN** the call fails. @@ -50,6 +55,8 @@ Validation failures are returned as MCP tool error results. They identify the in - **GIVEN** a caller supplies advertised `prompt` to `create_task_kandev`, **WHEN** validation runs, **THEN** the prompt is forwarded unchanged through the existing backend `description` field. - **GIVEN** a caller supplies only legacy `description` to `create_task_kandev`, **WHEN** validation runs, **THEN** the value is accepted as `prompt` and forwarded unchanged. - **GIVEN** a caller supplies both `prompt` and `description` to `create_task_kandev`, **WHEN** validation runs, **THEN** the call returns an error and creates no task. +- **GIVEN** a task-mode agent receives Kandev's first-turn context or the built-in `changes-walkthrough` request, **WHEN** it prepares `show_walkthrough_kandev`, **THEN** the instructions identify `steps` as an ordered array whose items require `file`, `line`, and `text`; the first-turn context also names the show, get, and delete walkthrough tools. +- **GIVEN** an existing installation with an untouched historical built-in `changes-walkthrough` prompt stored through the embedded loader, **WHEN** Kandev starts with a newer embedded prompt, **THEN** the stored built-in is refreshed; unrecognized content, a user-edited built-in, or a user-owned prompt is preserved. ## Out of scope @@ -57,3 +64,5 @@ Validation failures are returned as MCP tool error results. They identify the in - Changing the semantic business rules enforced inside individual handlers and services. - Closing arbitrary-key nested configuration maps that are intentionally open. - Changing MCP transport, authorization, tool availability by mode, or backend action payloads. +- Adding automatic tool-call retries or agent-client-specific schema rewriting. +- Duplicating every registered tool schema in the first-turn task context.