Carlos Florêncio
@@ -20,6 +20,7 @@ require (
|
||||
github.com/moby/moby/client v0.5.0
|
||||
github.com/nats-io/nats.go v1.31.0
|
||||
github.com/pkg/sftp v1.13.10
|
||||
github.com/santhosh-tekuri/jsonschema/v6 v6.0.2
|
||||
github.com/spf13/viper v1.16.0
|
||||
github.com/stretchr/testify v1.11.1
|
||||
github.com/superfly/sprites-go v0.0.0-20260206213632-8176adff485b
|
||||
|
||||
@@ -301,6 +301,8 @@ github.com/robfig/cron/v3 v3.0.1/go.mod h1:eQICP3HwyT7UooqI/z+Ov+PtYAWygg1TEWWzG
|
||||
github.com/rogpeppe/go-internal v1.3.0/go.mod h1:M8bDsm7K2OlrFYOpmOWEs/qY81heoFRclV5y23lUDJ4=
|
||||
github.com/rogpeppe/go-internal v1.14.1 h1:UQB4HGPB6osV0SQTLymcB4TgvyWu6ZyliaW0tI/otEQ=
|
||||
github.com/rogpeppe/go-internal v1.14.1/go.mod h1:MaRKkUm5W0goXpeCfT7UZI6fk/L7L7so1lCWt35ZSgc=
|
||||
github.com/santhosh-tekuri/jsonschema/v6 v6.0.2 h1:KRzFb2m7YtdldCEkzs6KqmJw4nqEVZGK7IN2kJkjTuQ=
|
||||
github.com/santhosh-tekuri/jsonschema/v6 v6.0.2/go.mod h1:JXeL+ps8p7/KNMjDQk3TCwPpBy0wYklyWTfbkIzdIFU=
|
||||
github.com/spf13/afero v1.9.5 h1:stMpOSZFs//0Lv29HduCmli3GUfpFoF3Y1Q/aXj/wVM=
|
||||
github.com/spf13/afero v1.9.5/go.mod h1:UBogFpq8E9Hx+xc5CNTTEpTnuHVmXDwZcZcE1eb/UhQ=
|
||||
github.com/spf13/cast v1.7.1 h1:cuNEagBQEHWN1FnbGEjCXL2szYEXqfJPbP2HNUaca9Y=
|
||||
|
||||
@@ -152,7 +152,7 @@ func (s *Server) createTaskHandler() server.ToolHandlerFunc {
|
||||
"workflow_step_id": workflowStepID,
|
||||
"workspace_mode": req.GetString("workspace_mode", ""),
|
||||
"title": title,
|
||||
"description": req.GetString("description", ""),
|
||||
"description": req.GetString("prompt", ""),
|
||||
"agent_profile_id": req.GetString("agent_profile_id", ""),
|
||||
"executor_profile_id": req.GetString("executor_profile_id", ""),
|
||||
"source_task_id": s.taskID,
|
||||
|
||||
@@ -39,6 +39,16 @@ func TestCreateTask_ToolSchema_HasParentID(t *testing.T) {
|
||||
assert.Contains(t, props, "workflow_id")
|
||||
assert.Contains(t, props, "workflow_step_id")
|
||||
assert.Contains(t, props, "workspace_mode")
|
||||
assert.Contains(t, props, "prompt")
|
||||
assert.NotContains(t, props, "description", "legacy alias must not increase the advertised schema")
|
||||
assert.Contains(t, tool.Tool.Description, "'prompt' is the sub-agent's initial prompt")
|
||||
assert.NotContains(t, tool.Tool.Description, "'description' is the sub-agent's initial prompt")
|
||||
promptProp, ok := props["prompt"].(map[string]interface{})
|
||||
require.True(t, ok, "prompt schema should be an object")
|
||||
promptDesc, ok := promptProp["description"].(string)
|
||||
require.True(t, ok, "prompt should have a description")
|
||||
assert.Contains(t, promptDesc, "For auto-started subtasks")
|
||||
assert.NotContains(t, promptDesc, "REQUIRED")
|
||||
assert.Contains(t, tool.Tool.Description, "explicit agent_profile_id always wins")
|
||||
assert.Contains(t, tool.Tool.Description, "current_task")
|
||||
assert.Contains(t, tool.Tool.Description, "workspace_default")
|
||||
@@ -86,6 +96,76 @@ func TestCreateTask_ToolSchema_HasParentID(t *testing.T) {
|
||||
assert.False(t, requiredSet["workflow_id"], "workflow_id should not be required")
|
||||
}
|
||||
|
||||
func TestCreateTask_PromptCanonical(t *testing.T) {
|
||||
backend := &testBackend{
|
||||
response: map[string]interface{}{"id": "subtask-1", "parent_id": "task-current"},
|
||||
}
|
||||
s := newTaskModeServer(t, backend, "task-current")
|
||||
|
||||
result := callTool(t, s, "create_task_kandev", map[string]interface{}{
|
||||
"title": "Review lane",
|
||||
"parent_id": "self",
|
||||
"prompt": "Review the authentication changes in detail.",
|
||||
})
|
||||
|
||||
assert.False(t, result.IsError)
|
||||
payload, ok := backend.lastPayload.(map[string]interface{})
|
||||
require.True(t, ok)
|
||||
assert.Equal(t, "Review the authentication changes in detail.", payload["description"])
|
||||
assert.NotContains(t, payload, "prompt")
|
||||
}
|
||||
|
||||
func TestCreateTask_DescriptionCompatibility(t *testing.T) {
|
||||
backend := &testBackend{
|
||||
response: map[string]interface{}{"id": "subtask-1", "parent_id": "task-current"},
|
||||
}
|
||||
s := newTaskModeServer(t, backend, "task-current")
|
||||
|
||||
result := callTool(t, s, "create_task_kandev", map[string]interface{}{
|
||||
"title": "Review lane",
|
||||
"parent_id": "self",
|
||||
"description": "Review the authentication changes in detail.",
|
||||
})
|
||||
|
||||
assert.False(t, result.IsError)
|
||||
payload, ok := backend.lastPayload.(map[string]interface{})
|
||||
require.True(t, ok)
|
||||
assert.Equal(t, "Review the authentication changes in detail.", payload["description"])
|
||||
assert.NotContains(t, payload, "prompt")
|
||||
}
|
||||
|
||||
func TestCreateTask_RejectsConflictingContext(t *testing.T) {
|
||||
backend := &testBackend{}
|
||||
s := newTaskModeServer(t, backend, "task-current")
|
||||
|
||||
result := callTool(t, s, "create_task_kandev", map[string]interface{}{
|
||||
"title": "Review lane",
|
||||
"description": "Short label",
|
||||
"prompt": "Detailed review instructions",
|
||||
})
|
||||
|
||||
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, "description")
|
||||
assert.Contains(t, content.Text, "prompt")
|
||||
}
|
||||
|
||||
func TestCreateTask_RejectsUnknownArguments(t *testing.T) {
|
||||
backend := &testBackend{}
|
||||
s := newTaskModeServer(t, backend, "task-current")
|
||||
|
||||
result := callTool(t, s, "create_task_kandev", map[string]interface{}{
|
||||
"title": "Review lane",
|
||||
"instructions": "Detailed review instructions",
|
||||
})
|
||||
|
||||
assert.True(t, result.IsError)
|
||||
assert.Empty(t, backend.lastAction)
|
||||
}
|
||||
|
||||
func TestCreateTask_SelfResolvesToTaskID(t *testing.T) {
|
||||
backend := &testBackend{
|
||||
response: map[string]interface{}{"id": "subtask-1", "parent_id": "task-current"},
|
||||
@@ -528,12 +608,10 @@ func TestMessageTask_ForwardsToBackend(t *testing.T) {
|
||||
s := newTaskModeServer(t, backend, "task-current")
|
||||
|
||||
result := callTool(t, s, "message_task_kandev", map[string]interface{}{
|
||||
"task_id": "task-target",
|
||||
"session_id": "sess-target",
|
||||
"prompt": "follow up",
|
||||
"delivery_mode": "interrupt",
|
||||
"sender_task_id": "spoofed-task",
|
||||
"sender_session_id": "spoofed-session",
|
||||
"task_id": "task-target",
|
||||
"session_id": "sess-target",
|
||||
"prompt": "follow up",
|
||||
"delivery_mode": "interrupt",
|
||||
})
|
||||
|
||||
assert.False(t, result.IsError)
|
||||
@@ -638,12 +716,7 @@ func TestStopTask_ForwardsTrustedSenderToBackend(t *testing.T) {
|
||||
s := newTaskModeServer(t, backend, "task-current")
|
||||
|
||||
result := callTool(t, s, "stop_task_kandev", map[string]interface{}{
|
||||
"task_id": "task-target",
|
||||
"sender_task_id": "spoofed-task",
|
||||
"sender_session_id": "spoofed-session",
|
||||
"session_id": "spoofed-target-session",
|
||||
"reason": "unsafe caller-controlled reason",
|
||||
"force": true,
|
||||
"task_id": "task-target",
|
||||
})
|
||||
|
||||
assert.False(t, result.IsError)
|
||||
|
||||
@@ -93,12 +93,14 @@ type Server struct {
|
||||
httpServer *server.StreamableHTTPServer
|
||||
logger *logger.Logger
|
||||
mcpLogger *zap.Logger // optional file logger for MCP debug traces
|
||||
mu sync.Mutex
|
||||
mu sync.RWMutex
|
||||
running bool
|
||||
attachmentMu sync.RWMutex
|
||||
attachmentAttempt streams.MCPAttachmentAttempt
|
||||
attachmentAttempts map[string]streams.MCPAttachmentAttempt
|
||||
attachmentReporter func(streams.MCPAttachmentEvidence)
|
||||
validatorMu sync.RWMutex
|
||||
toolValidators map[string]toolArgumentValidator
|
||||
}
|
||||
|
||||
// New creates a new MCP server for agentctl.
|
||||
@@ -363,7 +365,14 @@ func (s *Server) wrapHandler(toolName string, handler server.ToolHandlerFunc) se
|
||||
zap.Any("args", args))
|
||||
}
|
||||
|
||||
result, err := handler(ctx, req)
|
||||
validatedReq, validationErr := s.validateToolArguments(toolName, req)
|
||||
var result *mcp.CallToolResult
|
||||
var err error
|
||||
if validationErr != nil {
|
||||
result = mcp.NewToolResultError(validationErr.Error())
|
||||
} else {
|
||||
result, err = handler(ctx, validatedReq)
|
||||
}
|
||||
duration := time.Since(start)
|
||||
|
||||
switch {
|
||||
@@ -510,6 +519,7 @@ func (s *Server) registerTools() {
|
||||
zap.String("mode", s.mode),
|
||||
zap.Int("count", count),
|
||||
zap.Bool("disable_ask_question", s.disableAskQuestion))
|
||||
s.rebuildToolArgumentValidators()
|
||||
}
|
||||
|
||||
func (s *Server) registerKanbanTools() {
|
||||
@@ -701,8 +711,8 @@ IMPORTANT:
|
||||
- Different repo (you passed repository_url / repository_id / local_path): subtask defaults to that repo's default_branch
|
||||
- Pass base_branch explicitly to override either default. Use list_repositories_kandev to see each repo's default_branch.
|
||||
- Top-level tasks need a repository via repository_url, repository_id, or local_path
|
||||
- 'description' is the sub-agent's initial prompt — be specific and detailed
|
||||
- start_agent defaults to true and is what you want in nearly every case — the new task auto-launches an agent that immediately works on the description. Pass start_agent=false ONLY for an explicit placeholder (e.g. queuing work the user will start later, or creating a tracking task with no immediate work), and still pass agent_profile_id unless it can be inherited. When in doubt, leave it true.
|
||||
- 'prompt' is the sub-agent's initial prompt — be specific and detailed
|
||||
- start_agent defaults to true and is what you want in nearly every case — the new task auto-launches an agent that immediately works on the prompt. Pass start_agent=false ONLY for an explicit placeholder (e.g. queuing work the user will start later, or creating a tracking task with no immediate work), and still pass agent_profile_id unless it can be inherited. When in doubt, leave it true.
|
||||
- Kanban subtasks cannot have their own subtasks (max nesting depth is 1). To break work down further, create a sibling under the same parent. (Office task trees are exempt.)`
|
||||
parentDesc := "Parent task ID for subtasks. Use 'self' to create a subtask of your current task (RECOMMENDED for plan phases, delegated work). Omit only for unrelated top-level tasks."
|
||||
agentProfileDesc := "Agent profile ID to use. Explicit agent_profile_id always wins. When omitted, current_task inherits the current/source or parent profile before workflow/workspace defaults; workspace_default skips those task profiles, then uses workflow profiles before the target workspace default. start_agent=false still needs a resolvable profile for later manual start."
|
||||
@@ -716,8 +726,8 @@ IMPORTANT:
|
||||
- An explicit agent_profile_id always wins. When omitted, the saved user policy applies: current_task inherits a parent profile before workflow and target-workspace defaults; workspace_default skips the parent profile, honors workflow profiles first, then uses the target workspace default. External mode has no current/source task.
|
||||
- Executor and executor-profile inheritance from a parent is unchanged by either saved agent-profile policy.
|
||||
- Every created task must have a resolvable agent profile. start_agent=false still records the profile for a later manual start.
|
||||
- 'description' is the agent's initial prompt — be specific and detailed
|
||||
- start_agent defaults to true and is what you want in nearly every case — the new task auto-launches an agent that immediately works on the description. Pass start_agent=false ONLY for an explicit placeholder (e.g. queuing work the user will start later), and still pass agent_profile_id unless a default exists. When in doubt, leave it true.
|
||||
- 'prompt' is the agent's initial prompt — be specific and detailed
|
||||
- start_agent defaults to true and is what you want in nearly every case — the new task auto-launches an agent that immediately works on the prompt. Pass start_agent=false ONLY for an explicit placeholder (e.g. queuing work the user will start later), and still pass agent_profile_id unless a default exists. When in doubt, leave it true.
|
||||
- Use parent_id only when delegating to a known existing task by its ID`
|
||||
parentDesc = "Optional parent task ID. Omit for top-level tasks; provide an existing task ID only to create a subtask of that task."
|
||||
agentProfileDesc = "Agent profile ID to use. Explicit agent_profile_id always wins. When omitted, current_task inherits a parent profile before workflow/workspace defaults; workspace_default skips the parent profile, then uses workflow profiles before the target workspace default. External mode has no current/source task. start_agent=false still needs a resolvable profile for later manual start."
|
||||
@@ -732,10 +742,10 @@ IMPORTANT:
|
||||
mcp.WithString("workflow_step_id", mcp.Description("The workflow step ID (optional, auto-resolved if omitted; for subtasks, pass only with an explicit workflow_id)")),
|
||||
mcp.WithString("workspace_mode", mcp.Description("Subtask materialized-workspace mode: inherit_parent reuses the parent's worktree/materialized workspace (default for subtasks); new_workspace launches the subtask in its own workspace/worktree.")),
|
||||
mcp.WithString("title", mcp.Required(), mcp.Description("The task title")),
|
||||
mcp.WithString("description", mcp.Description("The initial prompt for the sub-agent. This is the ONLY context the agent receives when it starts — treat it as the agent's first user message. REQUIRED for subtasks: without a description the sub-agent starts with no context and cannot do useful work. Be specific and detailed.")),
|
||||
mcp.WithString("prompt", mcp.Description("The initial prompt for the sub-agent. This is the ONLY context the agent receives when it starts — treat it as the agent's first user message. For auto-started subtasks, provide a specific and detailed prompt; omitting it starts the sub-agent without task-specific context.")),
|
||||
mcp.WithString("agent_profile_id", mcp.Description(agentProfileDesc)),
|
||||
mcp.WithString("executor_profile_id", mcp.Description("Executor profile ID to use (determines the runtime environment: local, worktree, docker, etc.). For subtasks, inherited from the parent session. For top-level tasks, ask the user which executor profile they want if not already known.")),
|
||||
mcp.WithBoolean("start_agent", mcp.Description("Whether to auto-start an agent on the created task. Default: true — leave it true unless you specifically want a placeholder task with no agent running. Setting false leaves the task waiting for the user to click 'Start agent' in the UI; the description is preserved but no work happens automatically.")),
|
||||
mcp.WithBoolean("start_agent", mcp.Description("Whether to auto-start an agent on the created task. Default: true — leave it true unless you specifically want a placeholder task with no agent running. Setting false leaves the task waiting for the user to click 'Start agent' in the UI; the prompt is preserved but no work happens automatically.")),
|
||||
mcp.WithString("repository_id", mcp.Description("Repository ID. Required for top-level tasks unless local_path or repository_url is provided. For subtasks: optional — supply only when the subtask should target a different repo than the parent.")),
|
||||
mcp.WithString("local_path", mcp.Description("Local repository folder path (e.g. '/Users/me/projects/myrepo'). Will create/find the repository automatically. Preferred for local worktree flow. For subtasks: supply only when the subtask should target a different repo than the parent.")),
|
||||
mcp.WithString("repository_url", mcp.Description("GitHub repository URL (e.g. 'https://github.com/owner/repo'). The repository will be cloned automatically on first use. For subtasks: supply only when the subtask should target a different repo than the parent.")),
|
||||
|
||||
@@ -0,0 +1,171 @@
|
||||
package mcp
|
||||
|
||||
import (
|
||||
"encoding/json"
|
||||
"errors"
|
||||
"fmt"
|
||||
"net/url"
|
||||
"strings"
|
||||
|
||||
"github.com/mark3labs/mcp-go/mcp"
|
||||
jsonschema "github.com/santhosh-tekuri/jsonschema/v6"
|
||||
"go.uber.org/zap"
|
||||
)
|
||||
|
||||
type toolArgumentValidator struct {
|
||||
schema *jsonschema.Schema
|
||||
err error
|
||||
}
|
||||
|
||||
func (s *Server) rebuildToolArgumentValidators() {
|
||||
tools := s.mcpServer.ListTools()
|
||||
validators := make(map[string]toolArgumentValidator, len(tools))
|
||||
|
||||
for name, serverTool := range tools {
|
||||
schema, err := compileToolArgumentSchema(name, serverTool.Tool)
|
||||
validators[name] = toolArgumentValidator{schema: schema, err: err}
|
||||
if err != nil {
|
||||
s.logger.Error("failed to compile MCP tool argument schema",
|
||||
zap.String("tool", name),
|
||||
zap.Error(err))
|
||||
}
|
||||
}
|
||||
|
||||
s.validatorMu.Lock()
|
||||
s.toolValidators = validators
|
||||
s.validatorMu.Unlock()
|
||||
}
|
||||
|
||||
func compileToolArgumentSchema(toolName string, tool mcp.Tool) (*jsonschema.Schema, error) {
|
||||
rawSchema := tool.RawInputSchema
|
||||
if rawSchema == nil {
|
||||
var err error
|
||||
rawSchema, err = json.Marshal(tool.InputSchema)
|
||||
if err != nil {
|
||||
return nil, fmt.Errorf("marshal schema: %w", err)
|
||||
}
|
||||
}
|
||||
|
||||
var schemaDoc map[string]any
|
||||
if err := json.Unmarshal(rawSchema, &schemaDoc); err != nil {
|
||||
return nil, fmt.Errorf("decode schema: %w", err)
|
||||
}
|
||||
schemaDoc["additionalProperties"] = false
|
||||
|
||||
compiler := jsonschema.NewCompiler()
|
||||
compiler.DefaultDraft(jsonschema.Draft7)
|
||||
resourceURL := "https://kandev.local/mcp/schemas/" + url.PathEscape(toolName)
|
||||
if err := compiler.AddResource(resourceURL, schemaDoc); err != nil {
|
||||
return nil, fmt.Errorf("add schema resource: %w", err)
|
||||
}
|
||||
schema, err := compiler.Compile(resourceURL)
|
||||
if err != nil {
|
||||
return nil, fmt.Errorf("compile schema: %w", err)
|
||||
}
|
||||
return schema, nil
|
||||
}
|
||||
|
||||
func (s *Server) validateToolArguments(toolName string, req mcp.CallToolRequest) (mcp.CallToolRequest, error) {
|
||||
arguments := req.GetRawArguments()
|
||||
if arguments == nil {
|
||||
arguments = map[string]any{}
|
||||
}
|
||||
arguments, err := normalizeToolArguments(toolName, arguments)
|
||||
if err != nil {
|
||||
return req, err
|
||||
}
|
||||
req.Params.Arguments = arguments
|
||||
|
||||
// SetMode replaces the MCP tool set before rebuilding its validators. Use
|
||||
// the server read lock as a barrier so validation cannot observe that
|
||||
// intermediate state or pair a newly selected handler with old validators.
|
||||
s.mu.RLock()
|
||||
defer s.mu.RUnlock()
|
||||
|
||||
s.validatorMu.RLock()
|
||||
validator, ok := s.toolValidators[toolName]
|
||||
s.validatorMu.RUnlock()
|
||||
if !ok {
|
||||
return req, fmt.Errorf("invalid arguments for %s: schema validator is unavailable", toolName)
|
||||
}
|
||||
if validator.err != nil || validator.schema == nil {
|
||||
return req, fmt.Errorf("invalid arguments for %s: registered schema is invalid", toolName)
|
||||
}
|
||||
if err := validator.schema.Validate(arguments); err != nil {
|
||||
return req, sanitizedToolArgumentError(toolName, err)
|
||||
}
|
||||
return req, nil
|
||||
}
|
||||
|
||||
func sanitizedToolArgumentError(toolName string, err error) error {
|
||||
var validationErr *jsonschema.ValidationError
|
||||
if !errors.As(err, &validationErr) {
|
||||
return fmt.Errorf("invalid arguments for %s: validation failed", toolName)
|
||||
}
|
||||
|
||||
failure, keyword := firstKeywordFailure(validationErr)
|
||||
if failure == nil {
|
||||
failure = validationErr
|
||||
keyword = "schema"
|
||||
}
|
||||
return fmt.Errorf("invalid arguments for %s: validation failed at %s (keyword: %s)",
|
||||
toolName, validationInstancePath(failure.InstanceLocation), keyword)
|
||||
}
|
||||
|
||||
func firstKeywordFailure(err *jsonschema.ValidationError) (*jsonschema.ValidationError, string) {
|
||||
if err == nil {
|
||||
return nil, ""
|
||||
}
|
||||
if err.ErrorKind != nil {
|
||||
keywordPath := err.ErrorKind.KeywordPath()
|
||||
if len(keywordPath) > 0 && keywordPath[0] != "" {
|
||||
return err, keywordPath[0]
|
||||
}
|
||||
}
|
||||
for _, cause := range err.Causes {
|
||||
if failure, keyword := firstKeywordFailure(cause); failure != nil {
|
||||
return failure, keyword
|
||||
}
|
||||
}
|
||||
return nil, ""
|
||||
}
|
||||
|
||||
func validationInstancePath(tokens []string) string {
|
||||
if len(tokens) == 0 {
|
||||
return "$"
|
||||
}
|
||||
|
||||
var path strings.Builder
|
||||
for _, token := range tokens {
|
||||
path.WriteByte('/')
|
||||
token = strings.ReplaceAll(token, "~", "~0")
|
||||
path.WriteString(strings.ReplaceAll(token, "/", "~1"))
|
||||
}
|
||||
return path.String()
|
||||
}
|
||||
|
||||
func normalizeToolArguments(toolName string, arguments any) (any, error) {
|
||||
if toolName != "create_task_kandev" {
|
||||
return arguments, nil
|
||||
}
|
||||
args, ok := arguments.(map[string]any)
|
||||
if !ok {
|
||||
return arguments, nil
|
||||
}
|
||||
_, hasPrompt := args["prompt"]
|
||||
description, hasDescription := args["description"]
|
||||
if hasPrompt && hasDescription {
|
||||
return nil, fmt.Errorf("invalid arguments for %s: provide either prompt or description, not both", toolName)
|
||||
}
|
||||
if !hasDescription {
|
||||
return args, nil
|
||||
}
|
||||
|
||||
normalized := make(map[string]any, len(args))
|
||||
for key, value := range args {
|
||||
normalized[key] = value
|
||||
}
|
||||
normalized["prompt"] = description
|
||||
delete(normalized, "description")
|
||||
return normalized, nil
|
||||
}
|
||||
@@ -0,0 +1,292 @@
|
||||
package mcp
|
||||
|
||||
import (
|
||||
"encoding/json"
|
||||
"runtime"
|
||||
"testing"
|
||||
"time"
|
||||
|
||||
ws "github.com/kandev/kandev/pkg/websocket"
|
||||
"github.com/mark3labs/mcp-go/mcp"
|
||||
"github.com/stretchr/testify/assert"
|
||||
"github.com/stretchr/testify/require"
|
||||
)
|
||||
|
||||
func TestToolArgumentValidationRejectsUnknownTopLevelArgument(t *testing.T) {
|
||||
backend := &testBackend{
|
||||
response: map[string]interface{}{"workspaces": []interface{}{}, "total": 0},
|
||||
}
|
||||
s := newTaskModeServer(t, backend, "task-current")
|
||||
|
||||
result := callTool(t, s, "list_workspaces_kandev", map[string]interface{}{
|
||||
"unexpected": "discarded today",
|
||||
})
|
||||
|
||||
assert.True(t, result.IsError)
|
||||
assert.Empty(t, backend.lastAction)
|
||||
assert.NotEqual(t, ws.ActionMCPListWorkspaces, backend.lastAction)
|
||||
require.NotEmpty(t, result.Content)
|
||||
content, ok := result.Content[0].(mcp.TextContent)
|
||||
require.True(t, ok)
|
||||
assert.Equal(t,
|
||||
"invalid arguments for list_workspaces_kandev: validation failed at $ (keyword: additionalProperties)",
|
||||
content.Text)
|
||||
}
|
||||
|
||||
func TestToolArgumentValidation(t *testing.T) {
|
||||
t.Run("accepts an empty object for a parameterless tool", func(t *testing.T) {
|
||||
backend := &testBackend{
|
||||
response: map[string]interface{}{"workspaces": []interface{}{}, "total": 0},
|
||||
}
|
||||
s := newTaskModeServer(t, backend, "task-current")
|
||||
|
||||
result := callTool(t, s, "list_workspaces_kandev", map[string]interface{}{})
|
||||
|
||||
assert.False(t, result.IsError)
|
||||
assert.Equal(t, ws.ActionMCPListWorkspaces, backend.lastAction)
|
||||
})
|
||||
|
||||
t.Run("accepts omitted arguments for a parameterless tool", func(t *testing.T) {
|
||||
backend := &testBackend{
|
||||
response: map[string]interface{}{"workspaces": []interface{}{}, "total": 0},
|
||||
}
|
||||
s := newTaskModeServer(t, backend, "task-current")
|
||||
|
||||
result := callTool(t, s, "list_workspaces_kandev", nil)
|
||||
|
||||
assert.False(t, result.IsError)
|
||||
assert.Equal(t, ws.ActionMCPListWorkspaces, backend.lastAction)
|
||||
})
|
||||
|
||||
t.Run("rejects a missing required argument", func(t *testing.T) {
|
||||
backend := &testBackend{}
|
||||
s := newTaskModeServer(t, backend, "task-current")
|
||||
|
||||
result := callTool(t, s, "list_workflows_kandev", map[string]interface{}{})
|
||||
|
||||
assert.True(t, result.IsError)
|
||||
assert.Empty(t, backend.lastAction)
|
||||
})
|
||||
|
||||
t.Run("rejects the wrong declared type", func(t *testing.T) {
|
||||
backend := &testBackend{}
|
||||
s := newTaskModeServer(t, backend, "task-current")
|
||||
|
||||
result := callTool(t, s, "create_task_kandev", map[string]interface{}{
|
||||
"title": "Typed arguments",
|
||||
"start_agent": "false",
|
||||
})
|
||||
|
||||
assert.True(t, result.IsError)
|
||||
assert.Empty(t, backend.lastAction)
|
||||
})
|
||||
|
||||
t.Run("rejects a declared enum violation", func(t *testing.T) {
|
||||
backend := &testBackend{}
|
||||
s := newTaskModeServer(t, backend, "task-current")
|
||||
|
||||
result := callTool(t, s, "message_task_kandev", map[string]interface{}{
|
||||
"task_id": "task-target",
|
||||
"prompt": "Status?",
|
||||
"delivery_mode": "later",
|
||||
})
|
||||
|
||||
assert.True(t, result.IsError)
|
||||
assert.Empty(t, backend.lastAction)
|
||||
})
|
||||
|
||||
t.Run("keeps an intentionally open nested map", func(t *testing.T) {
|
||||
backend := &testBackend{response: map[string]interface{}{"id": "profile-1"}}
|
||||
s := newTestServer(t, backend)
|
||||
|
||||
result := callTool(t, s, "create_executor_profile_kandev", map[string]interface{}{
|
||||
"executor_id": "exec-local",
|
||||
"name": "Custom",
|
||||
"config": map[string]interface{}{
|
||||
"provider_specific_key": "allowed",
|
||||
},
|
||||
})
|
||||
|
||||
assert.False(t, result.IsError)
|
||||
assert.Equal(t, ws.ActionMCPCreateExecutorProfile, backend.lastAction)
|
||||
})
|
||||
}
|
||||
|
||||
func TestToolArgumentValidationDoesNotExposeRejectedValues(t *testing.T) {
|
||||
const (
|
||||
secret = "api-key-super-secret-123"
|
||||
toolName = "secret_pattern_tool"
|
||||
)
|
||||
backend := &testBackend{}
|
||||
s := newTaskModeServer(t, backend, "task-current")
|
||||
s.mcpServer.AddTool(
|
||||
mcp.NewToolWithRawSchema(
|
||||
toolName,
|
||||
"Validates a secret without exposing it.",
|
||||
json.RawMessage(`{
|
||||
"type": "object",
|
||||
"properties": {
|
||||
"token": {"type": "string", "pattern": "^safe$"}
|
||||
},
|
||||
"required": ["token"]
|
||||
}`),
|
||||
),
|
||||
s.wrapHandler(toolName, s.listWorkspacesHandler()),
|
||||
)
|
||||
s.rebuildToolArgumentValidators()
|
||||
|
||||
result := callTool(t, s, toolName, map[string]interface{}{
|
||||
"token": secret,
|
||||
})
|
||||
|
||||
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, "/token")
|
||||
assert.Contains(t, content.Text, "pattern")
|
||||
assert.NotContains(t, content.Text, secret)
|
||||
}
|
||||
|
||||
func TestAllRegisteredToolSchemasCompile(t *testing.T) {
|
||||
for _, mode := range []string{ModeTask, ModeConfig, ModeExternal, ModeOffice} {
|
||||
t.Run(mode, func(t *testing.T) {
|
||||
log := newTestLogger(t)
|
||||
s := New(&testBackend{}, "session-1", "task-1", 10005, log, "", true, mode)
|
||||
tools := s.mcpServer.ListTools()
|
||||
|
||||
s.validatorMu.RLock()
|
||||
defer s.validatorMu.RUnlock()
|
||||
require.Len(t, s.toolValidators, len(tools))
|
||||
for name := range tools {
|
||||
validator, ok := s.toolValidators[name]
|
||||
require.True(t, ok, "missing validator for %s", name)
|
||||
assert.NoError(t, validator.err, "schema for %s must compile", name)
|
||||
assert.NotNil(t, validator.schema, "schema for %s must compile", name)
|
||||
}
|
||||
})
|
||||
}
|
||||
}
|
||||
|
||||
func TestSetModeRebuildsToolValidators(t *testing.T) {
|
||||
backend := &testBackend{response: map[string]interface{}{"id": "workflow-1"}}
|
||||
s := newTaskModeServer(t, backend, "task-current")
|
||||
|
||||
s.SetMode(ModeConfig)
|
||||
result := callTool(t, s, "create_workflow_kandev", map[string]interface{}{
|
||||
"workspace_id": "workspace-1",
|
||||
"name": "Validated workflow",
|
||||
})
|
||||
|
||||
assert.False(t, result.IsError)
|
||||
assert.Equal(t, ws.ActionMCPCreateWorkflow, backend.lastAction)
|
||||
}
|
||||
|
||||
func TestToolValidationCoordinatesWithModeChange(t *testing.T) {
|
||||
s := newTaskModeServer(t, &testBackend{}, "task-current")
|
||||
req := mcp.CallToolRequest{}
|
||||
req.Method = "tools/call"
|
||||
req.Params.Name = "list_workspaces_kandev"
|
||||
req.Params.Arguments = map[string]interface{}{}
|
||||
|
||||
s.validatorMu.Lock()
|
||||
validatorLocked := true
|
||||
t.Cleanup(func() {
|
||||
if validatorLocked {
|
||||
s.validatorMu.Unlock()
|
||||
}
|
||||
})
|
||||
|
||||
validationDone := make(chan error, 1)
|
||||
go func() {
|
||||
_, err := s.validateToolArguments("list_workspaces_kandev", req)
|
||||
validationDone <- err
|
||||
}()
|
||||
|
||||
waitForCondition(t, "validation to acquire the mode read lock", func() bool {
|
||||
if s.mu.TryLock() {
|
||||
s.mu.Unlock()
|
||||
return false
|
||||
}
|
||||
return true
|
||||
})
|
||||
|
||||
modeDone := make(chan struct{})
|
||||
go func() {
|
||||
s.SetMode(ModeConfig)
|
||||
close(modeDone)
|
||||
}()
|
||||
waitForCondition(t, "mode change to wait for the write lock", func() bool {
|
||||
if s.mu.TryRLock() {
|
||||
s.mu.RUnlock()
|
||||
return false
|
||||
}
|
||||
return true
|
||||
})
|
||||
|
||||
select {
|
||||
case <-modeDone:
|
||||
t.Fatal("mode change completed while validation held the read lock")
|
||||
default:
|
||||
}
|
||||
s.validatorMu.Unlock()
|
||||
validatorLocked = false
|
||||
|
||||
select {
|
||||
case err := <-validationDone:
|
||||
assert.NoError(t, err)
|
||||
case <-time.After(5 * time.Second):
|
||||
t.Fatal("timed out waiting for validation to complete")
|
||||
}
|
||||
select {
|
||||
case <-modeDone:
|
||||
case <-time.After(5 * time.Second):
|
||||
t.Fatal("timed out waiting for mode change to complete")
|
||||
}
|
||||
assert.Equal(t, ModeConfig, s.mode)
|
||||
}
|
||||
|
||||
func TestNormalizeCreateTaskArguments(t *testing.T) {
|
||||
t.Run("keeps canonical prompt unchanged", func(t *testing.T) {
|
||||
arguments := map[string]any{
|
||||
"title": "Review lane",
|
||||
"prompt": "Detailed review instructions",
|
||||
}
|
||||
|
||||
normalized, err := normalizeToolArguments("create_task_kandev", arguments)
|
||||
|
||||
require.NoError(t, err)
|
||||
assert.Equal(t, arguments, normalized)
|
||||
assert.Contains(t, arguments, "prompt")
|
||||
assert.NotContains(t, arguments, "description")
|
||||
})
|
||||
|
||||
t.Run("copies the legacy description alias without mutating the request", func(t *testing.T) {
|
||||
arguments := map[string]any{
|
||||
"title": "Review lane",
|
||||
"description": "Detailed review instructions",
|
||||
}
|
||||
|
||||
normalized, err := normalizeToolArguments("create_task_kandev", arguments)
|
||||
|
||||
require.NoError(t, err)
|
||||
normalizedArguments, ok := normalized.(map[string]any)
|
||||
require.True(t, ok)
|
||||
assert.Equal(t, "Detailed review instructions", normalizedArguments["prompt"])
|
||||
assert.NotContains(t, normalizedArguments, "description")
|
||||
assert.Equal(t, "Detailed review instructions", arguments["description"])
|
||||
assert.NotContains(t, arguments, "prompt")
|
||||
})
|
||||
}
|
||||
|
||||
func waitForCondition(t *testing.T, description string, condition func() bool) {
|
||||
t.Helper()
|
||||
deadline := time.Now().Add(5 * time.Second)
|
||||
for !condition() {
|
||||
if time.Now().After(deadline) {
|
||||
t.Fatalf("timed out waiting for %s", description)
|
||||
}
|
||||
runtime.Gosched()
|
||||
}
|
||||
}
|
||||
@@ -0,0 +1,32 @@
|
||||
# ADR-2026-08-01-validate-mcp-tool-arguments: Validate MCP Tool Arguments at the Shared Server Boundary
|
||||
|
||||
**Status:** accepted
|
||||
**Date:** 2026-08-01
|
||||
**Area:** backend, protocol
|
||||
|
||||
## Context
|
||||
|
||||
Kandev registers JSON Schema for every MCP tool, but `mcp-go` v0.43.2 dispatches `tools/call` directly to the handler without validating arguments against that schema. Handlers commonly read known keys with permissive getters, so unknown keys and mistyped optional values can disappear while the tool reports success. Per-handler checks would leave current tools inconsistent and allow every newly registered tool to repeat the same failure mode.
|
||||
|
||||
## Decision
|
||||
|
||||
All Kandev MCP tool calls are validated in `apps/backend/internal/mcp/server` before their handlers or backend actions run.
|
||||
|
||||
- The currently registered tool schema is the source of truth for required fields, value types, arrays, objects, enums, and other declared constraints.
|
||||
- Validation treats the root argument object as closed even when the advertised schema omits `additionalProperties: false`; unknown top-level arguments fail without increasing every tool schema's context-token cost.
|
||||
- Nested objects retain their declared schema behavior. Open maps such as executor configuration and MCP server configuration remain open unless their own schema closes them.
|
||||
- Schemas are compiled once for the active tool set and rebuilt whenever `SetMode` replaces that set. A schema that cannot compile fails closed for calls to that tool and is covered by a registration test.
|
||||
- Explicit compatibility normalization may run before validation for an established caller mismatch. `create_task_kandev` advertises `prompt` for agent instructions and accepts the former `description` name as an unadvertised alias; supplying both is an error. The handler maps `prompt` to the backend's existing `description` field. Such exceptions require focused tests and do not weaken unknown-argument rejection for other keys.
|
||||
- Argument failures return MCP tool error results and do not invoke the tool handler or dispatch a backend action.
|
||||
|
||||
## Consequences
|
||||
|
||||
Invalid calls fail visibly and consistently across task, configuration, external, and Office MCP modes. Future tools inherit the validation boundary automatically when they use the standard registration and `wrapHandler` path. Kandev adds a JSON Schema validation dependency and must keep every registered schema compilable. Closing only the root preserves dynamic nested configuration maps while preventing accidental top-level parameter loss. Using `prompt` for agent instructions aligns create-task with the other agent-starting MCP tools without advertising both names or increasing the tool schema's parameter count; `description` remains appropriate for descriptive metadata in other contracts.
|
||||
|
||||
## Alternatives Considered
|
||||
|
||||
- **Validate only `create_task_kandev`.** Rejected because the same permissive dispatch behavior applies to every handler and every future tool.
|
||||
- **Add checks independently inside each handler.** Rejected because duplicated allowlists and type checks drift from registered schemas and are easy to omit on new tools.
|
||||
- **Only advertise `additionalProperties: false`.** Rejected because the current MCP server does not enforce advertised schemas, and repeating metadata across all tools increases agent context without guaranteeing correctness.
|
||||
- **Close every nested object recursively.** Rejected because several tool inputs intentionally carry arbitrary configuration maps.
|
||||
- **Upgrade `mcp-go` as the repair.** Rejected because the current library path does not provide the required dispatch validation, and a broad protocol dependency upgrade is unnecessary for this invariant.
|
||||
@@ -94,3 +94,4 @@ Read individual ADRs for full context. Create new ones via `/record decision` or
|
||||
| 2026-07-30-runtime-task-state-before-running-event | [Publish Task State Before Running Session State](2026-07-30-runtime-task-state-before-running-event.md) | accepted | backend, frontend, protocol, workflow | 2026-07-30 |
|
||||
| 2026-07-31-isolate-manual-pr-review-content | [Isolate Manual PR Review Content](2026-07-31-isolate-manual-pr-review-content.md) | accepted | infra, workflow, security | 2026-07-31 |
|
||||
| 2026-07-31-system-service-user-continuity | [Preserve System Service Identity Across Reinstallation](2026-07-31-system-service-user-continuity.md) | accepted | backend, cli, security, operations | 2026-07-31 |
|
||||
| 2026-08-01-validate-mcp-tool-arguments | [Validate MCP Tool Arguments at the Shared Server Boundary](2026-08-01-validate-mcp-tool-arguments.md) | accepted | backend, protocol | 2026-08-01 |
|
||||
|
||||
@@ -0,0 +1,89 @@
|
||||
---
|
||||
spec: docs/specs/integrations/mcp-tool-argument-validation.md
|
||||
created: 2026-08-01
|
||||
status: implemented
|
||||
---
|
||||
|
||||
# Implementation Plan: MCP Tool Argument Validation
|
||||
|
||||
## Overview
|
||||
|
||||
Compile every registered Kandev MCP input schema into a server-owned validator, close only its root argument object, and run validation in the shared `wrapHandler` boundary before any tool-specific code. Converge `create_task_kandev` on the advertised `prompt` name while retaining unadvertised `description` compatibility, and document the fail-closed caller behavior. This covers all current modes and automatically protects tools registered in the future through the standard server path.
|
||||
|
||||
## Confirmed root cause
|
||||
|
||||
`mcp-go` v0.43.2 unmarshals `tools/call` and invokes the selected handler without validating `CallToolRequest.Params.Arguments` against the tool's registered JSON Schema. Kandev's handlers then use permissive getters such as `GetString`, which return defaults for absent or mistyped values and never inspect unknown keys. The issue #2123 reproduction therefore returned success after discarding the detailed `prompt` supplied to `create_task_kandev`.
|
||||
|
||||
All 65 built-in tool registrations use `Server.wrapHandler`, which is the shared insertion point across task, configuration, external, and Office modes. Tool sets can be replaced by `SetMode`, so validator compilation must run after every registration pass rather than only in the constructor.
|
||||
|
||||
## Backend
|
||||
|
||||
### Shared schema compilation and validation
|
||||
|
||||
- Add `github.com/santhosh-tekuri/jsonschema/v6` v6.0.2 as a direct backend dependency and use its compiled-schema API rather than implementing a partial JSON Schema interpreter.
|
||||
- Add `apps/backend/internal/mcp/server/tool_argument_validation.go` to marshal either `Tool.RawInputSchema` or `Tool.InputSchema`, inject `additionalProperties: false` only at the root object, compile one validator per active tool, and retain a fail-closed compile error for any invalid schema.
|
||||
- Extend `Server` with the active compiled validator set. Rebuild it at the end of `registerTools`, which covers initial construction and `SetMode` replacement.
|
||||
- Normalize the create-task legacy alias when needed and validate arguments in `wrapHandler` before invoking the wrapped handler. Treat omitted arguments as an empty object, return a sanitized MCP tool error on failure, and preserve existing call-duration/error logging.
|
||||
- Keep nested object schemas unchanged so open configuration maps continue accepting arbitrary keys.
|
||||
|
||||
### Create-task parameter convergence
|
||||
|
||||
- Advertise `prompt` as the single create-task field for text delivered to the started agent.
|
||||
- Add a small pre-validation normalization path for `create_task_kandev`: shallow-copy unadvertised legacy `description` to `prompt` and remove `description` before schema validation. Avoid copying ordinary calls.
|
||||
- Reject calls that supply both keys before validation. Do not advertise both names.
|
||||
- Forward canonical `prompt` through the existing backend `description` payload without changing stored models or WebSocket actions.
|
||||
|
||||
## Public documentation
|
||||
|
||||
- Update `docs/public/automation-and-mcp.md` with the cross-cutting rule that Kandev validates required fields, types, constraints, and unknown top-level fields before a tool runs.
|
||||
- Document `prompt` as the canonical create-task input and explain legacy `description` migration outside the token-bearing tool schema.
|
||||
|
||||
## Tests
|
||||
|
||||
- **What:** every built-in tool schema in every MCP mode compiles after root closure.
|
||||
- **File:** `apps/backend/internal/mcp/server/tool_argument_validation_test.go`
|
||||
- **How:** construct task, config, external, and Office servers, enumerate their active tools, and assert each has a compiled validator without an error.
|
||||
- **What:** unknown, missing-required, wrong-type, enum/constraint, and parameterless-tool calls fail before handler invocation, while valid calls and intentionally open nested maps pass.
|
||||
- **File:** `apps/backend/internal/mcp/server/tool_argument_validation_test.go`
|
||||
- **How:** register focused test tools through the same wrapper, capture handler invocation, and exercise representative typed and raw schemas.
|
||||
- **What:** `SetMode` rebuilds validators for the replacement tool set.
|
||||
- **File:** `apps/backend/internal/mcp/server/tool_argument_validation_test.go`
|
||||
- **How:** change a server mode and validate calls against a tool schema available only in the replacement mode.
|
||||
- **What:** `create_task_kandev` advertises canonical `prompt`, accepts hidden legacy `description`, rejects both, and rejects every other unknown key without backend dispatch.
|
||||
- **File:** `apps/backend/internal/mcp/server/handlers_test.go`
|
||||
- **How:** focused handler-through-wrapper calls with `testBackend` payload capture.
|
||||
|
||||
No browser E2E is needed because this changes the MCP protocol boundary without changing UI behavior.
|
||||
|
||||
## Implementation Waves And Parallel Candidates
|
||||
|
||||
Execution is sequential in the primary conversation because the tasks share the MCP wrapper and validation contract.
|
||||
|
||||
- [x] [Task 01: Enforce registered schemas at the MCP boundary](task-01-enforce-registered-schemas.md) — done
|
||||
- [x] [Task 02: Converge create-task on prompt](task-02-create-task-prompt-compatibility.md) — done
|
||||
- [x] [Task 03: Document fail-closed MCP arguments](task-03-document-mcp-validation.md) — done
|
||||
|
||||
## Validation commands
|
||||
|
||||
- `make -C apps/backend test`
|
||||
- `make -C apps/backend lint`
|
||||
- `make -C apps/backend build`
|
||||
- `node --test scripts/validate-public-docs.test.mjs`
|
||||
- `node scripts/validate-public-docs.mjs`
|
||||
|
||||
## Risks and boundaries
|
||||
|
||||
- Existing schemas have historically been descriptive rather than enforced. The all-schema compilation test and focused valid-call cases must expose accidental mismatches before rollout.
|
||||
- Only the root argument object is closed. Recursively closing nested objects would break intentional arbitrary-key configuration inputs.
|
||||
- Validator error text must be sanitized and stable enough to guide callers without echoing sensitive values.
|
||||
- `registerTools` is the validator rebuild boundary; future dynamic registration outside that path must deliberately rebuild or use a new shared registration helper.
|
||||
|
||||
## Validation results
|
||||
|
||||
- `make -C apps/backend test` — passed across all backend packages.
|
||||
- `make -C apps/backend lint` — passed with no issues.
|
||||
- `make -C apps/backend build` — passed for the backend and local/remote helper binaries; Darwin helpers emitted the expected local no-codesigner warnings.
|
||||
- Focused MCP server tests — 167 passed under the race detector; the deterministic mode-change test passed 100 race-enabled repetitions.
|
||||
- CI-style changed-file lint against `bb525f1894803d150fb1bfb6a98f116a38ef3d3b` — no issues.
|
||||
- `node --test scripts/validate-public-docs.test.mjs` — 58 passed.
|
||||
- `node scripts/validate-public-docs.mjs` — validated 41 published pages.
|
||||
@@ -0,0 +1,58 @@
|
||||
---
|
||||
id: "01-enforce-registered-schemas"
|
||||
title: "Enforce registered schemas at the MCP boundary"
|
||||
status: done
|
||||
wave: 1
|
||||
depends_on: []
|
||||
plan: "plan.md"
|
||||
spec: "../../specs/integrations/mcp-tool-argument-validation.md"
|
||||
---
|
||||
|
||||
# Task 01: Enforce registered schemas at the MCP boundary
|
||||
|
||||
## Acceptance
|
||||
|
||||
- Every active built-in MCP tool has a compiled validator derived from its registered schema, with unknown root arguments rejected and intentional nested maps preserved.
|
||||
- Missing required fields, wrong types, declared constraint violations, and unknown top-level keys return sanitized MCP tool errors before handler invocation.
|
||||
- Initial registration and `SetMode` replacement both install the matching validator set; omitted arguments work only for schemas that accept an empty object.
|
||||
|
||||
## Verification
|
||||
|
||||
Follow strict TDD, then run:
|
||||
|
||||
```bash
|
||||
make -C apps/backend test
|
||||
```
|
||||
|
||||
## Files likely touched
|
||||
|
||||
- `apps/backend/go.mod`
|
||||
- `apps/backend/go.sum`
|
||||
- `apps/backend/internal/mcp/server/server.go`
|
||||
- `apps/backend/internal/mcp/server/tool_argument_validation.go`
|
||||
- `apps/backend/internal/mcp/server/tool_argument_validation_test.go`
|
||||
|
||||
## Dependencies
|
||||
|
||||
None.
|
||||
|
||||
## Parallelism
|
||||
|
||||
`sequential` — this task owns the shared registration and dispatch boundary plus the dependency files.
|
||||
|
||||
## Inputs
|
||||
|
||||
- Spec scenarios except the create-task compatibility scenarios.
|
||||
- `ADR-2026-08-01-validate-mcp-tool-arguments`.
|
||||
- `Server.wrapHandler`, `registerTools`, `SetMode`, and the typed/raw schema fields on `mcp.Tool`.
|
||||
- The v6.0.2 compiler and `Schema.Validate` contract from `github.com/santhosh-tekuri/jsonschema/v6`.
|
||||
|
||||
## Risks
|
||||
|
||||
- Do not close nested arbitrary-key maps.
|
||||
- Do not return full argument values in validation errors or logs added by this task.
|
||||
- A schema compilation defect must fail closed and remain observable without changing constructor signatures.
|
||||
|
||||
## Output contract
|
||||
|
||||
Report red/green evidence, dependency and files changed, exact test results, schema mismatches discovered, residual risks, and task/plan status updates in the primary conversation.
|
||||
@@ -0,0 +1,54 @@
|
||||
---
|
||||
id: "02-create-task-prompt-compatibility"
|
||||
title: "Converge create-task on prompt"
|
||||
status: done
|
||||
wave: 2
|
||||
depends_on: ["01-enforce-registered-schemas"]
|
||||
plan: "plan.md"
|
||||
spec: "../../specs/integrations/mcp-tool-argument-validation.md"
|
||||
---
|
||||
|
||||
# Task 02: Converge create-task on prompt
|
||||
|
||||
## Acceptance
|
||||
|
||||
- `create_task_kandev` advertises `prompt` and forwards the text unchanged to the existing backend `description` payload.
|
||||
- Existing callers may continue sending unadvertised legacy `description`; it is normalized to `prompt` before validation.
|
||||
- Calls containing both `prompt` and `description`, or any other unknown key, return a tool error without backend dispatch.
|
||||
- The registered create-task schema and description advertise only canonical `prompt`, so compatibility does not add a second schema property.
|
||||
|
||||
## Verification
|
||||
|
||||
Follow strict TDD, then run:
|
||||
|
||||
```bash
|
||||
make -C apps/backend test
|
||||
```
|
||||
|
||||
## Files likely touched
|
||||
|
||||
- `apps/backend/internal/mcp/server/tool_argument_validation.go`
|
||||
- `apps/backend/internal/mcp/server/handlers_test.go`
|
||||
|
||||
## Dependencies
|
||||
|
||||
- Task 01 provides the normalization/validation boundary and compiled create-task schema.
|
||||
|
||||
## Parallelism
|
||||
|
||||
`sequential` — this extends the shared validation path established by Task 01.
|
||||
|
||||
## Inputs
|
||||
|
||||
- The create-task scenarios in the spec.
|
||||
- Issue #2123 reproduction: `description` contained a short label while unknown `prompt` contained the real instructions.
|
||||
- Existing `createTaskHandler` forwarding to backend `description`.
|
||||
|
||||
## Risks
|
||||
|
||||
- Shallow-copy only when the legacy alias must be normalized so the original request cannot be mutated across hooks or logs.
|
||||
- Do not advertise the legacy alias or weaken rejection for any other unknown key.
|
||||
|
||||
## Output contract
|
||||
|
||||
Report red/green evidence, files changed, exact test result, blockers or residual risks, and task/plan status updates in the primary conversation.
|
||||
@@ -0,0 +1,52 @@
|
||||
---
|
||||
id: "03-document-mcp-validation"
|
||||
title: "Document fail-closed MCP arguments"
|
||||
status: done
|
||||
wave: 3
|
||||
depends_on: ["01-enforce-registered-schemas", "02-create-task-prompt-compatibility"]
|
||||
plan: "plan.md"
|
||||
spec: "../../specs/integrations/mcp-tool-argument-validation.md"
|
||||
---
|
||||
|
||||
# Task 03: Document fail-closed MCP arguments
|
||||
|
||||
## Acceptance
|
||||
|
||||
- Public MCP documentation states that required fields, types, constraints, and unknown top-level fields are validated before tool execution.
|
||||
- Public documentation identifies `prompt` as canonical and explains the legacy `description` migration without adding the compatibility alias or repeated text to tool schemas.
|
||||
- Public-doc validation passes without navigation changes.
|
||||
|
||||
## Verification
|
||||
|
||||
Run separately from the repository root:
|
||||
|
||||
```bash
|
||||
node --test scripts/validate-public-docs.test.mjs
|
||||
node scripts/validate-public-docs.mjs
|
||||
```
|
||||
|
||||
## Files likely touched
|
||||
|
||||
- `docs/public/automation-and-mcp.md`
|
||||
|
||||
## Dependencies
|
||||
|
||||
- Tasks 01 and 02 establish the final shipped behavior.
|
||||
|
||||
## Parallelism
|
||||
|
||||
`sequential` — public guidance must match the completed protocol boundary.
|
||||
|
||||
## Inputs
|
||||
|
||||
- Spec API surface and failure modes.
|
||||
- ADR decision and consequences.
|
||||
- Existing MCP mode and troubleshooting guidance in `docs/public/automation-and-mcp.md`.
|
||||
|
||||
## Risks
|
||||
|
||||
- Keep the guidance concise and do not expand every tool description.
|
||||
|
||||
## Output contract
|
||||
|
||||
Report the file changed, both validation results, remaining documentation risks, and task/plan status updates in the primary conversation.
|
||||
@@ -18,6 +18,8 @@ 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.
|
||||
|
||||
## Workflow events and human gates
|
||||
|
||||
Regular workflow entry actions can enable plan mode, reset agent context, or auto-start an agent; auto-start can use the step prompt or a stored prompt override. Turn-start and turn-complete events can move the task, while turn-complete and step-exit actions can disable plan mode. There is no regular standalone **stop agent** or **send prompt** workflow action. Approval/review steps and steps without automatic start remain the supported human gates. Inspect events on both the source and destination step before enabling a move or automatic start; otherwise two steps can form a loop.
|
||||
@@ -145,6 +147,8 @@ Names ending in `_kandev` are the canonical MCP protocol tool names. Some agent
|
||||
|
||||
Task tools use normal client discovery. When `step_complete_kandev` is required but is not already visible, the agent should search the active tool catalog for its canonical name. Kandev does not request eager loading through client-specific metadata.
|
||||
|
||||
`create_task_kandev` advertises `prompt` for instructions delivered to a newly started agent. Older callers may still send `description` when `prompt` is absent, but sending both is an error; the compatibility name is intentionally omitted from the advertised schema.
|
||||
|
||||
Task mode currently registers these tool groups:
|
||||
|
||||
| Group | Available operations |
|
||||
|
||||
@@ -90,7 +90,7 @@ Detaching changes task hierarchy only. An inherited workspace remains shared wit
|
||||
|
||||
Call `create_task_kandev` with `parent_id: "self"`. `workspace_mode` defaults to `inherit_parent`; set it to `new_workspace` for isolated materialization.
|
||||
|
||||
- `start_agent` defaults to `true`, so a description is required unless it is set to `false`.
|
||||
- `start_agent` defaults to `true`. Supply a detailed `prompt` when it is true; if omitted, Kandev still starts the agent without task-specific instructions. Set it to `false` for a placeholder task.
|
||||
- The tool inherits the parent workspace, workflow, profile, executor, repositories, and base branches unless overridden.
|
||||
- Inherited repository attachments deliberately do not copy an explicit checkout branch.
|
||||
- An explicit same-repository child uses the inherited base branch. An explicit cross-repository child defaults to that repository's default branch unless `base_branch` is supplied.
|
||||
|
||||
@@ -93,6 +93,7 @@ Per-workspace credentials and triage triggers for external services.
|
||||
| [azure-devops-integration](azure-devops-integration/spec.md) | shipped |
|
||||
| [slack](integrations/slack.md) | shipped |
|
||||
| [external-mcp](integrations/external-mcp.md) | draft |
|
||||
| [mcp-tool-argument-validation](integrations/mcp-tool-argument-validation.md) | shipped |
|
||||
| [github-authentication](integrations/github-authentication.md) | draft |
|
||||
| [gitlab-integration](gitlab-integration/spec.md) | shipped |
|
||||
| [jira-status-filter](jira-status-filter/spec.md) | shipped |
|
||||
|
||||
@@ -0,0 +1,59 @@
|
||||
---
|
||||
status: shipped
|
||||
created: 2026-08-01
|
||||
owner: kandev
|
||||
---
|
||||
|
||||
# MCP Tool Argument Validation
|
||||
|
||||
Decision: [ADR-2026-08-01-validate-mcp-tool-arguments](../../decisions/2026-08-01-validate-mcp-tool-arguments.md)
|
||||
|
||||
## 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.
|
||||
|
||||
## 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.
|
||||
- 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.
|
||||
|
||||
## API surface
|
||||
|
||||
The advertised MCP `tools/list` schemas do not gain repeated unknown-argument metadata or the legacy `create_task_kandev.description` compatibility alias. The server applies these rules when handling `tools/call`:
|
||||
|
||||
1. Normalize an explicitly supported compatibility alias, if any.
|
||||
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.
|
||||
|
||||
## 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.
|
||||
- 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 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.
|
||||
- **GIVEN** a tool with an intentionally open nested configuration map, **WHEN** a caller supplies an arbitrary key inside that map, **THEN** validation accepts the nested key.
|
||||
- **GIVEN** the server changes MCP mode, **WHEN** a caller invokes a tool in the replacement set, **THEN** validation uses that tool's replacement schema.
|
||||
- **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.
|
||||
|
||||
## Out of scope
|
||||
|
||||
- Advertising the legacy `description` compatibility alias alongside `prompt`.
|
||||
- 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.
|
||||
Reference in New Issue
Block a user