diff --git a/pkg/workflow/mcp_scripts_parser.go b/pkg/workflow/mcp_scripts_parser.go index d18e2294bb1..8df2e8af135 100644 --- a/pkg/workflow/mcp_scripts_parser.go +++ b/pkg/workflow/mcp_scripts_parser.go @@ -2,7 +2,9 @@ package workflow import ( "encoding/json" - "fmt" + "strconv" + "strings" + "time" "github.com/github/gh-aw/pkg/logger" "github.com/github/gh-aw/pkg/typeutil" @@ -10,6 +12,26 @@ import ( var mcpScriptsLog = logger.New("workflow:mcp_scripts") +// parseTimeoutString converts a string timeout value to seconds. +// It accepts plain integers ("120"), Go duration strings ("6m", "1h", "30s", "2h30m"), +// and trims surrounding whitespace. Returns (seconds, true) on success, +// or (0, false) if the value cannot be parsed. +func parseTimeoutString(s string) (int, bool) { + s = strings.TrimSpace(s) + if s == "" { + return 0, false + } + // Plain integer – e.g. "120" + if n, err := strconv.Atoi(s); err == nil { + return n, true + } + // Go duration – e.g. "6m", "1h", "30s", "2h30m" + if d, err := time.ParseDuration(s); err == nil { + return int(d.Seconds()), true + } + return 0, false +} + // MCPScriptsConfig holds the configuration for mcp-scripts custom tools type MCPScriptsConfig struct { Mode string // Transport mode: "http" (default) or "stdio" @@ -179,8 +201,11 @@ func parseMCPScriptsMap(mcpScriptsMap map[string]any) (*MCPScriptsConfig, bool) case float64: toolConfig.Timeout = int(t) case string: - // Try to parse string as integer - _, _ = fmt.Sscanf(t, "%d", &toolConfig.Timeout) + if n, ok := parseTimeoutString(t); ok { + toolConfig.Timeout = n + } else { + mcpScriptsLog.Printf("Warning: invalid timeout value %q for tool %q, keeping default timeout (60s)", t, toolName) + } } } @@ -350,8 +375,11 @@ func (c *Compiler) mergeMCPScripts(main *MCPScriptsConfig, importedConfigs []str case float64: toolConfig.Timeout = int(t) case string: - // Try to parse string as integer - _, _ = fmt.Sscanf(t, "%d", &toolConfig.Timeout) + if n, ok := parseTimeoutString(t); ok { + toolConfig.Timeout = n + } else { + mcpScriptsLog.Printf("Warning: invalid timeout value %q for tool %q, keeping default timeout (60s)", t, toolName) + } } } diff --git a/pkg/workflow/mcp_scripts_timeout_test.go b/pkg/workflow/mcp_scripts_timeout_test.go index f2fbe4e7b9e..08ad8ad0e09 100644 --- a/pkg/workflow/mcp_scripts_timeout_test.go +++ b/pkg/workflow/mcp_scripts_timeout_test.go @@ -98,6 +98,118 @@ func TestMCPScriptsTimeoutParsing(t *testing.T) { toolName: "go-tool", expectedTimeout: 90, }, + { + name: "timeout as valid numeric string", + frontmatter: map[string]any{ + "mcp-scripts": map[string]any{ + "string-tool": map[string]any{ + "description": "String timeout tool", + "script": "return 'ok';", + "timeout": "120", + }, + }, + }, + toolName: "string-tool", + expectedTimeout: 120, + }, + { + name: "invalid string timeout falls back to default", + frontmatter: map[string]any{ + "mcp-scripts": map[string]any{ + "bad-timeout-tool": map[string]any{ + "description": "Bad timeout tool", + "script": "return 'ok';", + "timeout": "not-a-number", + }, + }, + }, + toolName: "bad-timeout-tool", + expectedTimeout: 60, // Default timeout + }, + { + name: "duration minutes string timeout is parsed", + frontmatter: map[string]any{ + "mcp-scripts": map[string]any{ + "duration-tool": map[string]any{ + "description": "Duration string timeout tool", + "script": "return 'ok';", + "timeout": "5m", + }, + }, + }, + toolName: "duration-tool", + expectedTimeout: 300, // 5 * 60 + }, + { + name: "duration hours string timeout is parsed", + frontmatter: map[string]any{ + "mcp-scripts": map[string]any{ + "hour-tool": map[string]any{ + "description": "Hour timeout tool", + "script": "return 'ok';", + "timeout": "1h", + }, + }, + }, + toolName: "hour-tool", + expectedTimeout: 3600, // 1 * 3600 + }, + { + name: "duration seconds string timeout is parsed", + frontmatter: map[string]any{ + "mcp-scripts": map[string]any{ + "sec-tool": map[string]any{ + "description": "Seconds timeout tool", + "script": "return 'ok';", + "timeout": "30s", + }, + }, + }, + toolName: "sec-tool", + expectedTimeout: 30, + }, + { + name: "compound duration string timeout is parsed", + frontmatter: map[string]any{ + "mcp-scripts": map[string]any{ + "compound-tool": map[string]any{ + "description": "Compound duration timeout tool", + "script": "return 'ok';", + "timeout": "1h30m", + }, + }, + }, + toolName: "compound-tool", + expectedTimeout: 5400, // 90 * 60 + }, + { + name: "empty string timeout falls back to default", + frontmatter: map[string]any{ + "mcp-scripts": map[string]any{ + "empty-timeout-tool": map[string]any{ + "description": "Empty string timeout tool", + "script": "return 'ok';", + "timeout": "", + }, + }, + }, + toolName: "empty-timeout-tool", + expectedTimeout: 60, // Default timeout + }, + { + name: "string timeout with whitespace is parsed", + frontmatter: map[string]any{ + "mcp-scripts": map[string]any{ + "ws-tool": map[string]any{ + "description": "Whitespace timeout tool", + "script": "return 'ok';", + "timeout": " 120 ", + }, + }, + }, + toolName: "ws-tool", + expectedTimeout: 120, + }, } for _, tt := range tests { @@ -244,3 +356,100 @@ func TestMCPScriptsDefaultTimeoutWhenMerging(t *testing.T) { t.Errorf("Expected default timeout 60, got %d", merged.Tools["imported-tool"].Timeout) } } + +// TestMCPScriptsMergeStringTimeout tests that string timeouts are parsed correctly when merging +func TestMCPScriptsMergeStringTimeout(t *testing.T) { + compiler := &Compiler{} + + main := &MCPScriptsConfig{ + Tools: make(map[string]*MCPScriptToolConfig), + } + + // Imported config with valid numeric string timeout + validImportedJSON := `{ + "valid-string-timeout": { + "description": "Tool with numeric string timeout", + "script": "return 'ok';", + "timeout": "120" + } + }` + + merged := compiler.mergeMCPScripts(main, []string{validImportedJSON}) + + // Verify valid numeric string timeout is parsed correctly + if merged.Tools["valid-string-timeout"].Timeout != 120 { + t.Errorf("Expected timeout 120, got %d", merged.Tools["valid-string-timeout"].Timeout) + } + + // Imported config with invalid string timeout + invalidImportedJSON := `{ + "invalid-string-timeout": { + "description": "Tool with invalid string timeout", + "script": "return 'ok';", + "timeout": "not-a-number" + } + }` + + merged2 := compiler.mergeMCPScripts(main, []string{invalidImportedJSON}) + + // Verify invalid string timeout falls back to default (60s) + if merged2.Tools["invalid-string-timeout"].Timeout != 60 { + t.Errorf("Expected default timeout 60, got %d", merged2.Tools["invalid-string-timeout"].Timeout) + } + + // Imported config with duration-style string timeout ("5m"). + // time.ParseDuration now converts this to 300s. + durationImportedJSON := `{ + "duration-string-timeout": { + "description": "Tool with duration-style string timeout", + "script": "return 'ok';", + "timeout": "5m" + } + }` + + merged3 := compiler.mergeMCPScripts(main, []string{durationImportedJSON}) + + // Verify "5m" is correctly parsed as 300 seconds + if merged3.Tools["duration-string-timeout"].Timeout != 300 { + t.Errorf("Expected timeout 300 for \"5m\", got %d", merged3.Tools["duration-string-timeout"].Timeout) + } +} + +// TestParseTimeoutString is a unit test for the parseTimeoutString helper. +func TestParseTimeoutString(t *testing.T) { + tests := []struct { + input string + wantSecs int + wantOk bool + }{ + // Plain integers + {"120", 120, true}, + {"0", 0, true}, + {" 120 ", 120, true}, // leading/trailing whitespace + + // Go duration strings + {"30s", 30, true}, + {"6m", 360, true}, + {"1h", 3600, true}, + {"1h30m", 5400, true}, + {"2h30m10s", 9010, true}, + + // Invalid + {"", 0, false}, + {" ", 0, false}, + {"not-a-number", 0, false}, + {"5 m", 0, false}, // space inside duration is not valid + } + + for _, tt := range tests { + t.Run(tt.input, func(t *testing.T) { + gotSecs, gotOk := parseTimeoutString(tt.input) + if gotOk != tt.wantOk { + t.Errorf("parseTimeoutString(%q) ok = %v, want %v", tt.input, gotOk, tt.wantOk) + } + if gotOk && gotSecs != tt.wantSecs { + t.Errorf("parseTimeoutString(%q) = %d, want %d", tt.input, gotSecs, tt.wantSecs) + } + }) + } +}