mirror of
https://github.com/mark3labs/kit.git
synced 2026-06-14 03:30:26 +00:00
fix(mcp): validate tasksMode and inherit task options in Subagent (#21)
Address two review findings on the MCP Tasks PR. - Config.Validate() now rejects unknown tasksMode values with a clear error naming the server and bad value. Without this a typo (e.g. "alwasy") was silently downgraded to "auto" by the runtime parser. - Kit.Subagent() now propagates the parent's six MCP task options (mode map, timeout, TTL, poll interval, max poll interval, progress callback) onto the child via a new inheritMCPTaskOptions helper. Without this, child subagents always saw default polling and no progress feedback regardless of parent configuration. The propagation logic lives in a helper so the test exercises the real code path instead of duplicating it; future task fields only need to be added in one place.
This commit is contained in:
@@ -345,6 +345,17 @@ func (c *Config) Validate() error {
|
|||||||
return fmt.Errorf("server %s: allowedTools and excludedTools are mutually exclusive", serverName)
|
return fmt.Errorf("server %s: allowedTools and excludedTools are mutually exclusive", serverName)
|
||||||
}
|
}
|
||||||
|
|
||||||
|
// Reject unknown tasksMode values up front so a typo (e.g. "alwasy")
|
||||||
|
// fails loud here instead of being silently downgraded to "auto" by
|
||||||
|
// the runtime parser. Comparison is case-insensitive to match
|
||||||
|
// tools.ParseTaskMode.
|
||||||
|
switch strings.ToLower(strings.TrimSpace(serverConfig.TasksMode)) {
|
||||||
|
case "", "auto", "never", "always":
|
||||||
|
// ok
|
||||||
|
default:
|
||||||
|
return fmt.Errorf("server %s: invalid tasksMode %q (expected one of: auto, never, always)", serverName, serverConfig.TasksMode)
|
||||||
|
}
|
||||||
|
|
||||||
transport := serverConfig.GetTransportType()
|
transport := serverConfig.GetTransportType()
|
||||||
switch transport {
|
switch transport {
|
||||||
case "stdio":
|
case "stdio":
|
||||||
|
|||||||
@@ -672,3 +672,47 @@ func TestMCPServerConfig_TasksMode_DefaultEmpty(t *testing.T) {
|
|||||||
t.Errorf("expected default TasksMode to be empty, got %q", cfg.TasksMode)
|
t.Errorf("expected default TasksMode to be empty, got %q", cfg.TasksMode)
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
|
func TestConfig_Validate_TasksMode(t *testing.T) {
|
||||||
|
t.Run("empty is valid", func(t *testing.T) {
|
||||||
|
cfg := &Config{
|
||||||
|
MCPServers: map[string]MCPServerConfig{
|
||||||
|
"a": {Type: "remote", URL: "https://x.example"},
|
||||||
|
},
|
||||||
|
}
|
||||||
|
if err := cfg.Validate(); err != nil {
|
||||||
|
t.Errorf("empty TasksMode should validate, got %v", err)
|
||||||
|
}
|
||||||
|
})
|
||||||
|
|
||||||
|
t.Run("known values are valid", func(t *testing.T) {
|
||||||
|
for _, mode := range []string{"auto", "never", "always", "AUTO", " always "} {
|
||||||
|
cfg := &Config{
|
||||||
|
MCPServers: map[string]MCPServerConfig{
|
||||||
|
"a": {Type: "remote", URL: "https://x.example", TasksMode: mode},
|
||||||
|
},
|
||||||
|
}
|
||||||
|
if err := cfg.Validate(); err != nil {
|
||||||
|
t.Errorf("TasksMode=%q should validate, got %v", mode, err)
|
||||||
|
}
|
||||||
|
}
|
||||||
|
})
|
||||||
|
|
||||||
|
t.Run("typo is rejected with a clear error", func(t *testing.T) {
|
||||||
|
cfg := &Config{
|
||||||
|
MCPServers: map[string]MCPServerConfig{
|
||||||
|
"buildbot": {Type: "remote", URL: "https://x.example", TasksMode: "alwasy"},
|
||||||
|
},
|
||||||
|
}
|
||||||
|
err := cfg.Validate()
|
||||||
|
if err == nil {
|
||||||
|
t.Fatal("expected validation error for invalid TasksMode")
|
||||||
|
}
|
||||||
|
// Error must mention the server name AND the bad value so the
|
||||||
|
// user knows where to look.
|
||||||
|
msg := err.Error()
|
||||||
|
if !strings.Contains(msg, "buildbot") || !strings.Contains(msg, `"alwasy"`) {
|
||||||
|
t.Errorf("error %q should mention both server name and bad value", msg)
|
||||||
|
}
|
||||||
|
})
|
||||||
|
}
|
||||||
|
|||||||
@@ -1842,6 +1842,13 @@ func (m *Kit) Subagent(ctx context.Context, cfg SubagentConfig) (*SubagentResult
|
|||||||
Streaming: true,
|
Streaming: true,
|
||||||
MCPConfig: m.mcpConfig,
|
MCPConfig: m.mcpConfig,
|
||||||
}
|
}
|
||||||
|
// Propagate the parent's MCP task configuration so a child subagent
|
||||||
|
// invoking long-running MCP tools observes the same per-server modes,
|
||||||
|
// timeouts, and progress callback as the parent. Without this, child
|
||||||
|
// agents would silently fall back to MCPTaskModeAuto with default
|
||||||
|
// polling and no progress feedback even when the parent had configured
|
||||||
|
// custom values.
|
||||||
|
inheritMCPTaskOptions(childOpts, m.opts)
|
||||||
child, err := New(ctx, childOpts)
|
child, err := New(ctx, childOpts)
|
||||||
if err != nil {
|
if err != nil {
|
||||||
return &SubagentResult{Elapsed: time.Since(start)}, fmt.Errorf("failed to create subagent: %w", err)
|
return &SubagentResult{Elapsed: time.Since(start)}, fmt.Errorf("failed to create subagent: %w", err)
|
||||||
|
|||||||
@@ -218,3 +218,19 @@ func mcpTaskFromInternal(t tools.MCPTaskInfo) MCPTask {
|
|||||||
PollInterval: t.PollInterval,
|
PollInterval: t.PollInterval,
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
|
// inheritMCPTaskOptions copies every MCP task-related field from parent
|
||||||
|
// onto child. Used by Kit.Subagent so child instances observe the same
|
||||||
|
// per-server modes, timeouts, and progress callback as their parent.
|
||||||
|
// A nil parent is a no-op so callers don't have to guard at the call site.
|
||||||
|
func inheritMCPTaskOptions(child, parent *Options) {
|
||||||
|
if child == nil || parent == nil {
|
||||||
|
return
|
||||||
|
}
|
||||||
|
child.MCPTaskMode = parent.MCPTaskMode
|
||||||
|
child.MCPTaskTimeout = parent.MCPTaskTimeout
|
||||||
|
child.MCPTaskTTL = parent.MCPTaskTTL
|
||||||
|
child.MCPTaskPollInterval = parent.MCPTaskPollInterval
|
||||||
|
child.MCPTaskMaxPollInterval = parent.MCPTaskMaxPollInterval
|
||||||
|
child.MCPTaskProgress = parent.MCPTaskProgress
|
||||||
|
}
|
||||||
|
|||||||
@@ -118,3 +118,48 @@ func TestKitMCPTasksWithoutAgentReturnsError(t *testing.T) {
|
|||||||
t.Error("CancelMCPTask on nil Kit should error")
|
t.Error("CancelMCPTask on nil Kit should error")
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
|
func TestSubagentPropagatesMCPTaskOptions(t *testing.T) {
|
||||||
|
// Exercises the helper Kit.Subagent uses to copy MCP task options
|
||||||
|
// onto child Options. Calling the real helper (rather than
|
||||||
|
// duplicating its body in the test) means any new field added to
|
||||||
|
// the propagation list is picked up automatically by the
|
||||||
|
// equivalence assertion below.
|
||||||
|
parent := &Options{
|
||||||
|
MCPTaskMode: map[string]MCPTaskMode{
|
||||||
|
"build": MCPTaskModeAlways,
|
||||||
|
"chat": MCPTaskModeNever,
|
||||||
|
},
|
||||||
|
MCPTaskTimeout: 30 * time.Minute,
|
||||||
|
MCPTaskTTL: 45 * time.Minute,
|
||||||
|
MCPTaskPollInterval: 750 * time.Millisecond,
|
||||||
|
MCPTaskMaxPollInterval: 4 * time.Second,
|
||||||
|
MCPTaskProgress: func(MCPTaskProgress) {},
|
||||||
|
}
|
||||||
|
|
||||||
|
child := &Options{}
|
||||||
|
inheritMCPTaskOptions(child, parent)
|
||||||
|
|
||||||
|
if child.MCPTaskMode["build"] != MCPTaskModeAlways || child.MCPTaskMode["chat"] != MCPTaskModeNever {
|
||||||
|
t.Errorf("MCPTaskMode not propagated: got %+v", child.MCPTaskMode)
|
||||||
|
}
|
||||||
|
if child.MCPTaskTimeout != 30*time.Minute {
|
||||||
|
t.Errorf("MCPTaskTimeout = %v, want 30m", child.MCPTaskTimeout)
|
||||||
|
}
|
||||||
|
if child.MCPTaskTTL != 45*time.Minute {
|
||||||
|
t.Errorf("MCPTaskTTL = %v, want 45m", child.MCPTaskTTL)
|
||||||
|
}
|
||||||
|
if child.MCPTaskPollInterval != 750*time.Millisecond {
|
||||||
|
t.Errorf("MCPTaskPollInterval = %v, want 750ms", child.MCPTaskPollInterval)
|
||||||
|
}
|
||||||
|
if child.MCPTaskMaxPollInterval != 4*time.Second {
|
||||||
|
t.Errorf("MCPTaskMaxPollInterval = %v, want 4s", child.MCPTaskMaxPollInterval)
|
||||||
|
}
|
||||||
|
if child.MCPTaskProgress == nil {
|
||||||
|
t.Error("MCPTaskProgress not propagated")
|
||||||
|
}
|
||||||
|
|
||||||
|
// Nil parent is a no-op rather than a panic.
|
||||||
|
inheritMCPTaskOptions(&Options{}, nil)
|
||||||
|
inheritMCPTaskOptions(nil, parent)
|
||||||
|
}
|
||||||
|
|||||||
Reference in New Issue
Block a user