From 6e36053856060b444b5ff4810ba3d3b7a83dae5e Mon Sep 17 00:00:00 2001 From: Ed Zynda Date: Mon, 4 May 2026 17:06:11 +0300 Subject: [PATCH] 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. --- internal/config/config.go | 11 +++++++++ internal/config/config_test.go | 44 +++++++++++++++++++++++++++++++++ pkg/kit/kit.go | 7 ++++++ pkg/kit/mcp_tasks.go | 16 ++++++++++++ pkg/kit/mcp_tasks_test.go | 45 ++++++++++++++++++++++++++++++++++ 5 files changed, 123 insertions(+) diff --git a/internal/config/config.go b/internal/config/config.go index 020a4707..33428d41 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -345,6 +345,17 @@ func (c *Config) Validate() error { 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() switch transport { case "stdio": diff --git a/internal/config/config_test.go b/internal/config/config_test.go index d5e91c78..095acfb6 100644 --- a/internal/config/config_test.go +++ b/internal/config/config_test.go @@ -672,3 +672,47 @@ func TestMCPServerConfig_TasksMode_DefaultEmpty(t *testing.T) { 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) + } + }) +} diff --git a/pkg/kit/kit.go b/pkg/kit/kit.go index 3b7e754e..25e6484f 100644 --- a/pkg/kit/kit.go +++ b/pkg/kit/kit.go @@ -1842,6 +1842,13 @@ func (m *Kit) Subagent(ctx context.Context, cfg SubagentConfig) (*SubagentResult Streaming: true, 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) if err != nil { return &SubagentResult{Elapsed: time.Since(start)}, fmt.Errorf("failed to create subagent: %w", err) diff --git a/pkg/kit/mcp_tasks.go b/pkg/kit/mcp_tasks.go index e57f76b6..e341f023 100644 --- a/pkg/kit/mcp_tasks.go +++ b/pkg/kit/mcp_tasks.go @@ -218,3 +218,19 @@ func mcpTaskFromInternal(t tools.MCPTaskInfo) MCPTask { 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 +} diff --git a/pkg/kit/mcp_tasks_test.go b/pkg/kit/mcp_tasks_test.go index 50589188..9887bebc 100644 --- a/pkg/kit/mcp_tasks_test.go +++ b/pkg/kit/mcp_tasks_test.go @@ -118,3 +118,48 @@ func TestKitMCPTasksWithoutAgentReturnsError(t *testing.T) { 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) +}