feat: support duration string timeouts (6m, 1h) in mcp_scripts_parser

pkg/workflow/mcp_scripts_parser.go

@@ -2,14 +2,36 @@ package workflow
 
 import (
 	"encoding/json"
-	"fmt"
+	"strconv"
+	"strings"
+	"time"
 
 	"github.com/github/gh-aw/pkg/logger"
 	"github.com/github/gh-aw/pkg/typeutil"
 )
 
 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)
+					}
 				}
 			}
 

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)
+			}
+		})
+	}
+}