From 6c2173a8bd4b6af56a41634937d986b9ac1388de Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Fri, 1 May 2026 21:40:28 +0000 Subject: [PATCH 1/2] Initial plan From b0eda38695fe8a796cd067f442fec9f61b9879e9 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Fri, 1 May 2026 21:56:09 +0000 Subject: [PATCH 2/2] feat: add engine.mcp.tool-timeout support for configurable MCP gateway tool timeout - Add MCPToolTimeoutMin (10s) and MCPToolTimeoutMax (600s) constants - Add tool-timeout property to engine.mcp JSON schema - Add MCPToolTimeout field to EngineConfig struct - Extract tool-timeout from engine.mcp frontmatter in ExtractEngineConfig - Add ToolTimeout field to MCPGatewayRuntimeConfig struct - Propagate ToolTimeout in buildMCPGatewayConfig - Emit toolTimeout in gateway stdin JSON config (mcp_renderer.go) - Add validateEngineMCPToolTimeout (bounds: 10s-600s) in engine_validation.go - Call validateEngineMCPToolTimeout in both compiler paths - Add tests for extraction, validation, config propagation, and rendering Closes # Agent-Logs-Url: https://github.com/github/gh-aw/sessions/9bdea675-7d5f-447b-a30b-14924a3b5ff2 Co-authored-by: lpcox <15877973+lpcox@users.noreply.github.com> --- pkg/constants/constants.go | 6 + pkg/parser/schemas/main_workflow_schema.json | 5 + .../compiler_orchestrator_workflow.go | 5 + pkg/workflow/compiler_string_api.go | 5 + pkg/workflow/engine.go | 8 ++ pkg/workflow/engine_config_test.go | 68 ++++++++++ pkg/workflow/engine_validation.go | 26 ++++ pkg/workflow/engine_validation_test.go | 120 ++++++++++++++++++ pkg/workflow/mcp_gateway_config.go | 5 +- pkg/workflow/mcp_gateway_config_test.go | 39 ++++++ pkg/workflow/mcp_renderer.go | 3 + pkg/workflow/mcp_renderer_test.go | 71 +++++++++++ pkg/workflow/tools_types.go | 1 + 13 files changed, 361 insertions(+), 1 deletion(-) diff --git a/pkg/constants/constants.go b/pkg/constants/constants.go index 50901183dd9..ce62ddd6741 100644 --- a/pkg/constants/constants.go +++ b/pkg/constants/constants.go @@ -225,6 +225,12 @@ const DefaultMCPStartupTimeout = 120 * time.Second // MCPSessionTimeoutMin is the minimum allowed value for engine.mcp.session-timeout (5 minutes). const MCPSessionTimeoutMin = 5 * time.Minute +// MCPToolTimeoutMin is the minimum allowed value for engine.mcp.tool-timeout (10 seconds). +const MCPToolTimeoutMin = 10 * time.Second + +// MCPToolTimeoutMax is the maximum allowed value for engine.mcp.tool-timeout (600 seconds). +const MCPToolTimeoutMax = 600 * time.Second + // DefaultActivationJobRunnerImage is the default runner image for activation and pre-activation jobs const DefaultActivationJobRunnerImage = "ubuntu-slim" diff --git a/pkg/parser/schemas/main_workflow_schema.json b/pkg/parser/schemas/main_workflow_schema.json index f0d83fd97fc..3aaa67b6b80 100644 --- a/pkg/parser/schemas/main_workflow_schema.json +++ b/pkg/parser/schemas/main_workflow_schema.json @@ -9902,6 +9902,11 @@ "type": "string", "description": "Session timeout for MCP gateway sessions as a Go duration string (e.g. \"30m\", \"4h\", \"24h\"). Must be at least 5m (no upper bound). Omitted or empty uses the effective gateway default (precedence: this field > MCP_GATEWAY_SESSION_TIMEOUT env var > built-in default 6h). Longer timeouts benefit multi-hour workflows such as large-scale migrations; shorter values free gateway resources sooner.", "examples": ["30m", "1h", "4h", "6h", "12h"] + }, + "tool-timeout": { + "type": "string", + "description": "Timeout for individual MCP tool calls as a Go duration string (e.g. \"30s\", \"2m\", \"10m\"). Must be between 10s and 600s inclusive. Omitted or empty uses the gateway built-in default (60s). Use a higher value for slow MCP backends such as full-text search over large indexes.", + "examples": ["30s", "2m", "5m", "10m"] } }, "additionalProperties": false diff --git a/pkg/workflow/compiler_orchestrator_workflow.go b/pkg/workflow/compiler_orchestrator_workflow.go index 7e9598f1737..8e3ab89a172 100644 --- a/pkg/workflow/compiler_orchestrator_workflow.go +++ b/pkg/workflow/compiler_orchestrator_workflow.go @@ -86,6 +86,11 @@ func (c *Compiler) ParseWorkflowFile(markdownPath string) (*WorkflowData, error) return nil, fmt.Errorf("%s: %w", cleanPath, err) } + // Validate optional engine.mcp.tool-timeout configuration. + if err := c.validateEngineMCPToolTimeout(workflowData); err != nil { + return nil, fmt.Errorf("%s: %w", cleanPath, err) + } + // Validate that inlined-imports is not used with agent file imports. // Agent files require runtime access and cannot be resolved without sources. if workflowData.InlinedImports && engineSetup.importsResult.AgentFile != "" { diff --git a/pkg/workflow/compiler_string_api.go b/pkg/workflow/compiler_string_api.go index df69f946db4..d90c13e4b21 100644 --- a/pkg/workflow/compiler_string_api.go +++ b/pkg/workflow/compiler_string_api.go @@ -140,6 +140,11 @@ func (c *Compiler) ParseWorkflowString(content string, virtualPath string) (*Wor return nil, fmt.Errorf("%s: %w", cleanPath, err) } + // Validate optional engine.mcp.tool-timeout configuration. + if err := c.validateEngineMCPToolTimeout(workflowData); err != nil { + return nil, fmt.Errorf("%s: %w", cleanPath, err) + } + // Validate GitHub tool configuration if err := validateGitHubToolConfig(workflowData.ParsedTools, workflowData.Name); err != nil { return nil, fmt.Errorf("%s: %w", cleanPath, err) diff --git a/pkg/workflow/engine.go b/pkg/workflow/engine.go index 0ebd28f32dd..3a8fbb49799 100644 --- a/pkg/workflow/engine.go +++ b/pkg/workflow/engine.go @@ -49,6 +49,7 @@ type EngineConfig struct { // MCP gateway configuration from engine.mcp sub-object MCPSessionTimeout string // session-timeout: Go duration string for MCP gateway sessions (e.g. "4h", "30m") + MCPToolTimeout string // tool-timeout: Go duration string for individual MCP tool calls (e.g. "2m", "30s") } // NetworkPermissions represents network access permissions for workflow execution @@ -331,6 +332,13 @@ func (c *Compiler) ExtractEngineConfig(frontmatter map[string]any) (string, *Eng engineLog.Printf("Extracted engine.mcp.session-timeout: %s", config.MCPSessionTimeout) } } + // Extract tool-timeout (kebab-case only; camelCase is not supported) + if ttVal, hasToolTimeout := mcpObj["tool-timeout"]; hasToolTimeout { + if ttStr, ok := ttVal.(string); ok && ttStr != "" { + config.MCPToolTimeout = ttStr + engineLog.Printf("Extracted engine.mcp.tool-timeout: %s", config.MCPToolTimeout) + } + } } } diff --git a/pkg/workflow/engine_config_test.go b/pkg/workflow/engine_config_test.go index d12630baa4b..6c8b8ea74c0 100644 --- a/pkg/workflow/engine_config_test.go +++ b/pkg/workflow/engine_config_test.go @@ -925,3 +925,71 @@ func TestEngineMCPSessionTimeoutExtraction(t *testing.T) { }) } } + +// TestEngineMCPToolTimeoutExtraction tests extraction of engine.mcp.tool-timeout. +func TestEngineMCPToolTimeoutExtraction(t *testing.T) { + compiler := NewCompiler() + + tests := []struct { + name string + frontmatter map[string]any + expectedTimeout string + }{ + { + name: "extracts tool-timeout from engine.mcp", + frontmatter: map[string]any{ + "engine": map[string]any{ + "id": "copilot", + "mcp": map[string]any{ + "tool-timeout": "2m", + }, + }, + }, + expectedTimeout: "2m", + }, + { + name: "no mcp section - empty tool timeout", + frontmatter: map[string]any{ + "engine": map[string]any{ + "id": "copilot", + }, + }, + expectedTimeout: "", + }, + { + name: "mcp section without tool-timeout - empty", + frontmatter: map[string]any{ + "engine": map[string]any{ + "id": "copilot", + "mcp": map[string]any{}, + }, + }, + expectedTimeout: "", + }, + { + name: "mcp section with both session-timeout and tool-timeout", + frontmatter: map[string]any{ + "engine": map[string]any{ + "id": "copilot", + "mcp": map[string]any{ + "session-timeout": "4h", + "tool-timeout": "5m", + }, + }, + }, + expectedTimeout: "5m", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + _, config := compiler.ExtractEngineConfig(tt.frontmatter) + if config == nil { + t.Fatal("Expected non-nil config") + } + if config.MCPToolTimeout != tt.expectedTimeout { + t.Errorf("MCPToolTimeout = %q, want %q", config.MCPToolTimeout, tt.expectedTimeout) + } + }) + } +} diff --git a/pkg/workflow/engine_validation.go b/pkg/workflow/engine_validation.go index e289649a333..5935ab5a295 100644 --- a/pkg/workflow/engine_validation.go +++ b/pkg/workflow/engine_validation.go @@ -131,6 +131,32 @@ func (c *Compiler) validateEngineMCPSessionTimeout(workflowData *WorkflowData) e return nil } +// validateEngineMCPToolTimeout validates optional engine.mcp.tool-timeout configuration. +// The value must be a valid Go duration string between 10s and 600s inclusive. +func (c *Compiler) validateEngineMCPToolTimeout(workflowData *WorkflowData) error { + if workflowData == nil || workflowData.EngineConfig == nil || workflowData.EngineConfig.MCPToolTimeout == "" { + return nil + } + + raw := workflowData.EngineConfig.MCPToolTimeout + + d, err := time.ParseDuration(raw) + if err != nil { + return fmt.Errorf("engine.mcp.tool-timeout: invalid duration %q. Must be a valid Go duration string (e.g. \"30s\", \"2m\", \"10m\").\n\nExamples:\n engine:\n mcp:\n tool-timeout: 2m\n\nSee: %s", raw, constants.DocsEnginesURL) + } + + if d < constants.MCPToolTimeoutMin { + return fmt.Errorf("engine.mcp.tool-timeout: %q is too short (minimum is 10s).\n\nExamples:\n tool-timeout: 30s\n tool-timeout: 2m\n\nSee: %s", raw, constants.DocsEnginesURL) + } + + if d > constants.MCPToolTimeoutMax { + return fmt.Errorf("engine.mcp.tool-timeout: %q exceeds the maximum allowed value (600s / 10m).\n\nExamples:\n tool-timeout: 2m\n tool-timeout: 10m\n\nSee: %s", raw, constants.DocsEnginesURL) + } + + engineValidationLog.Printf("engine.mcp.tool-timeout validated: %s (%s)", raw, d) + return nil +} + // validateEngineInlineDefinition validates an inline engine definition parsed from // engine.runtime + optional engine.provider in the workflow frontmatter. // Returns an error if: diff --git a/pkg/workflow/engine_validation_test.go b/pkg/workflow/engine_validation_test.go index 6743a59f3bb..13b10f146fb 100644 --- a/pkg/workflow/engine_validation_test.go +++ b/pkg/workflow/engine_validation_test.go @@ -505,3 +505,123 @@ func TestValidateEngineMCPSessionTimeout(t *testing.T) { }) } } + +// TestValidateEngineMCPToolTimeout tests the validateEngineMCPToolTimeout function. +func TestValidateEngineMCPToolTimeout(t *testing.T) { + tests := []struct { + name string + workflow *WorkflowData + expectError bool + errorSubstr string + }{ + { + name: "nil workflow data", + workflow: nil, + expectError: false, + }, + { + name: "nil engine config", + workflow: &WorkflowData{}, + expectError: false, + }, + { + name: "empty tool timeout - no error", + workflow: &WorkflowData{ + EngineConfig: &EngineConfig{ID: "copilot"}, + }, + expectError: false, + }, + { + name: "valid duration 2m", + workflow: &WorkflowData{ + EngineConfig: &EngineConfig{ID: "copilot", MCPToolTimeout: "2m"}, + }, + expectError: false, + }, + { + name: "valid duration 30s", + workflow: &WorkflowData{ + EngineConfig: &EngineConfig{ID: "copilot", MCPToolTimeout: "30s"}, + }, + expectError: false, + }, + { + name: "valid duration 10s (minimum)", + workflow: &WorkflowData{ + EngineConfig: &EngineConfig{ID: "copilot", MCPToolTimeout: "10s"}, + }, + expectError: false, + }, + { + name: "valid duration 600s (maximum)", + workflow: &WorkflowData{ + EngineConfig: &EngineConfig{ID: "copilot", MCPToolTimeout: "600s"}, + }, + expectError: false, + }, + { + name: "valid duration 10m (maximum)", + workflow: &WorkflowData{ + EngineConfig: &EngineConfig{ID: "copilot", MCPToolTimeout: "10m"}, + }, + expectError: false, + }, + { + name: "invalid duration string", + workflow: &WorkflowData{ + EngineConfig: &EngineConfig{ID: "copilot", MCPToolTimeout: "2hours"}, + }, + expectError: true, + errorSubstr: "invalid duration", + }, + { + name: "too short - 5s", + workflow: &WorkflowData{ + EngineConfig: &EngineConfig{ID: "copilot", MCPToolTimeout: "5s"}, + }, + expectError: true, + errorSubstr: "too short", + }, + { + name: "too long - 601s", + workflow: &WorkflowData{ + EngineConfig: &EngineConfig{ID: "copilot", MCPToolTimeout: "601s"}, + }, + expectError: true, + errorSubstr: "exceeds the maximum", + }, + { + name: "too long - 11m", + workflow: &WorkflowData{ + EngineConfig: &EngineConfig{ID: "copilot", MCPToolTimeout: "11m"}, + }, + expectError: true, + errorSubstr: "exceeds the maximum", + }, + { + name: "plain integer - not valid Go duration", + workflow: &WorkflowData{ + EngineConfig: &EngineConfig{ID: "copilot", MCPToolTimeout: "120"}, + }, + expectError: true, + errorSubstr: "invalid duration", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + compiler := NewCompiler() + err := compiler.validateEngineMCPToolTimeout(tt.workflow) + + if tt.expectError { + require.Error(t, err, "Expected validation error") + if tt.errorSubstr != "" { + assert.Contains(t, err.Error(), tt.errorSubstr, "Expected error substring mismatch") + } + return + } + + assert.NoError(t, err, "Expected tool-timeout validation to pass") + }) + } +} diff --git a/pkg/workflow/mcp_gateway_config.go b/pkg/workflow/mcp_gateway_config.go index 8ef74536506..69c271a7f82 100644 --- a/pkg/workflow/mcp_gateway_config.go +++ b/pkg/workflow/mcp_gateway_config.go @@ -136,9 +136,11 @@ func buildMCPGatewayConfig(workflowData *WorkflowData) *MCPGatewayRuntimeConfig // are written directly into the gateway config JSON. // // SessionTimeout comes from engine.mcp.session-timeout in frontmatter (via EngineConfig). - var sessionTimeout string + // ToolTimeout comes from engine.mcp.tool-timeout in frontmatter (via EngineConfig). + var sessionTimeout, toolTimeout string if workflowData.EngineConfig != nil { sessionTimeout = workflowData.EngineConfig.MCPSessionTimeout + toolTimeout = workflowData.EngineConfig.MCPToolTimeout } return &MCPGatewayRuntimeConfig{ Port: int(DefaultMCPGatewayPort), // Will be formatted as "${MCP_GATEWAY_PORT}" in renderer @@ -150,6 +152,7 @@ func buildMCPGatewayConfig(workflowData *WorkflowData) *MCPGatewayRuntimeConfig TrustedBots: workflowData.SandboxConfig.MCP.TrustedBots, // Additional trusted bot identities from frontmatter KeepaliveInterval: workflowData.SandboxConfig.MCP.KeepaliveInterval, // Keepalive interval from frontmatter (0=default, -1=disabled, >0=custom) SessionTimeout: sessionTimeout, // Session timeout from engine.mcp.session-timeout (empty = gateway default 6h) + ToolTimeout: toolTimeout, // Tool timeout from engine.mcp.tool-timeout (empty = gateway built-in default 60s) // OTLPEndpoint and OTLPHeaders are set from workflowData by injectOTLPConfig, which is // the fully resolved OTLP config (including imports). Using these fields ensures gateway // OTLP config honours observability defined in imported shared workflows. diff --git a/pkg/workflow/mcp_gateway_config_test.go b/pkg/workflow/mcp_gateway_config_test.go index 5aa150af8bd..bc299b460fe 100644 --- a/pkg/workflow/mcp_gateway_config_test.go +++ b/pkg/workflow/mcp_gateway_config_test.go @@ -401,6 +401,44 @@ func TestBuildMCPGatewayConfig(t *testing.T) { SessionTimeout: "", }, }, + { + name: "propagates tool-timeout from engine config", + workflowData: &WorkflowData{ + EngineConfig: &EngineConfig{ + ID: "copilot", + MCPToolTimeout: "2m", + }, + SandboxConfig: &SandboxConfig{}, + }, + expected: &MCPGatewayRuntimeConfig{ + Port: int(DefaultMCPGatewayPort), + Domain: "${MCP_GATEWAY_DOMAIN}", + APIKey: "${MCP_GATEWAY_API_KEY}", + PayloadDir: "${MCP_GATEWAY_PAYLOAD_DIR}", + PayloadSizeThreshold: constants.DefaultMCPGatewayPayloadSizeThreshold, + ToolTimeout: "2m", + }, + }, + { + name: "propagates both session-timeout and tool-timeout from engine config", + workflowData: &WorkflowData{ + EngineConfig: &EngineConfig{ + ID: "copilot", + MCPSessionTimeout: "4h", + MCPToolTimeout: "5m", + }, + SandboxConfig: &SandboxConfig{}, + }, + expected: &MCPGatewayRuntimeConfig{ + Port: int(DefaultMCPGatewayPort), + Domain: "${MCP_GATEWAY_DOMAIN}", + APIKey: "${MCP_GATEWAY_API_KEY}", + PayloadDir: "${MCP_GATEWAY_PAYLOAD_DIR}", + PayloadSizeThreshold: constants.DefaultMCPGatewayPayloadSizeThreshold, + SessionTimeout: "4h", + ToolTimeout: "5m", + }, + }, } for _, tt := range tests { @@ -419,6 +457,7 @@ func TestBuildMCPGatewayConfig(t *testing.T) { assert.Equal(t, tt.expected.TrustedBots, result.TrustedBots, "TrustedBots should match") assert.Equal(t, tt.expected.KeepaliveInterval, result.KeepaliveInterval, "KeepaliveInterval should match") assert.Equal(t, tt.expected.SessionTimeout, result.SessionTimeout, "SessionTimeout should match") + assert.Equal(t, tt.expected.ToolTimeout, result.ToolTimeout, "ToolTimeout should match") } }) } diff --git a/pkg/workflow/mcp_renderer.go b/pkg/workflow/mcp_renderer.go index 112cdc98ceb..0ac86c47cc8 100644 --- a/pkg/workflow/mcp_renderer.go +++ b/pkg/workflow/mcp_renderer.go @@ -191,6 +191,9 @@ func RenderJSONMCPConfig( if options.GatewayConfig.SessionTimeout != "" { fmt.Fprintf(&configBuilder, ",\n \"sessionTimeout\": %q", options.GatewayConfig.SessionTimeout) } + if options.GatewayConfig.ToolTimeout != "" { + fmt.Fprintf(&configBuilder, ",\n \"toolTimeout\": %q", options.GatewayConfig.ToolTimeout) + } // When OTLP tracing is configured, add the opentelemetry section directly to the // gateway config. The endpoint is passed via the OTEL_EXPORTER_OTLP_ENDPOINT env var // (injected by injectOTLPConfig) so that secrets are never interpolated directly into diff --git a/pkg/workflow/mcp_renderer_test.go b/pkg/workflow/mcp_renderer_test.go index 8e50ca8fa9d..49d3f736bc0 100644 --- a/pkg/workflow/mcp_renderer_test.go +++ b/pkg/workflow/mcp_renderer_test.go @@ -668,3 +668,74 @@ func TestRenderJSONMCPConfig_SessionTimeout(t *testing.T) { }) } } + +// TestRenderJSONMCPConfig_ToolTimeout verifies that toolTimeout is emitted +// in the gateway JSON section when set on the MCPGatewayRuntimeConfig. +func TestRenderJSONMCPConfig_ToolTimeout(t *testing.T) { + tests := []struct { + name string + toolTimeout string + wantField bool + }{ + { + name: "includes toolTimeout when set", + toolTimeout: "2m", + wantField: true, + }, + { + name: "omits toolTimeout when empty", + toolTimeout: "", + wantField: false, + }, + { + name: "includes toolTimeout 30s", + toolTimeout: "30s", + wantField: true, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + gatewayConfig := &MCPGatewayRuntimeConfig{ + Domain: "localhost", + APIKey: "test-api-key", + ToolTimeout: tt.toolTimeout, + } + + workflowData := &WorkflowData{ + Name: "test-workflow", + FrontmatterHash: "abc123", + } + + var output strings.Builder + err := RenderJSONMCPConfig( + &output, + map[string]any{}, + []string{}, + workflowData, + JSONMCPConfigOptions{ + ConfigPath: "/tmp/test/mcp-servers.json", + GatewayConfig: gatewayConfig, + Renderers: MCPToolRenderers{}, + }, + ) + + if err != nil { + t.Fatalf("RenderJSONMCPConfig returned error: %v", err) + } + + result := output.String() + hasField := strings.Contains(result, `"toolTimeout":`) + if hasField != tt.wantField { + t.Errorf("toolTimeout field presence = %v, want %v\noutput:\n%s", hasField, tt.wantField, result) + } + + if tt.wantField && tt.toolTimeout != "" { + expected := `"toolTimeout": "` + tt.toolTimeout + `"` + if !strings.Contains(result, expected) { + t.Errorf("expected %q in output\noutput:\n%s", expected, result) + } + } + }) + } +} diff --git a/pkg/workflow/tools_types.go b/pkg/workflow/tools_types.go index 0199deddc2d..000139bb3ae 100644 --- a/pkg/workflow/tools_types.go +++ b/pkg/workflow/tools_types.go @@ -442,6 +442,7 @@ type MCPGatewayRuntimeConfig struct { TrustedBots []string `yaml:"trusted-bots,omitempty"` // Additional bot identity strings to pass to the gateway, merged with its built-in list KeepaliveInterval int `yaml:"keepalive-interval,omitempty"` // Keepalive ping interval in seconds for HTTP MCP backends (0=default 1500s, -1=disabled, >0=custom) SessionTimeout string `yaml:"session-timeout,omitempty"` // Session timeout for MCP gateway sessions as a Go duration string (e.g. "4h", "30m"); empty = gateway default (precedence: stdin config > MCP_GATEWAY_SESSION_TIMEOUT env var > built-in default 6h) + ToolTimeout string `yaml:"tool-timeout,omitempty"` // Timeout for individual MCP tool calls as a Go duration string (e.g. "2m", "30s"); empty = gateway built-in default (60s) OTLPEndpoint string `yaml:"-"` // OTLP collector endpoint (derived from observability.otlp, not user-settable) OTLPHeaders string `yaml:"-"` // Raw OTLP HTTP headers string (derived from observability.otlp, not user-settable) }