From 65ff7ceac6a617dfcd1e90b6e78fb32e5412d5cc Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Mon, 27 Apr 2026 23:30:12 +0000 Subject: [PATCH 1/5] Initial plan From fb6d7d284aa6a4eba827d60e6e7d5edaa6991551 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Mon, 27 Apr 2026 23:43:01 +0000 Subject: [PATCH 2/5] fix: log warning when string timeout cannot be parsed in mcp_scripts_parser.go MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Replace silent fmt.Sscanf with strconv.Atoi and emit a warning log on failure at both occurrences (parseMCPScriptsMap and mergeMCPScripts). Add test cases: - timeout as valid numeric string "120" → parses to 120s - timeout as invalid string "not-a-number" → falls back to default 60s with warning - same coverage for the merge path (TestMCPScriptsMergeStringTimeout) Agent-Logs-Url: https://github.com/github/gh-aw/sessions/a450004a-8130-4ca5-ad13-435bb4ae95ba Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com> --- pkg/actionpins/data/action_pins.json | 6 +-- pkg/workflow/data/action_pins.json | 6 +-- pkg/workflow/mcp_scripts_parser.go | 16 ++++-- pkg/workflow/mcp_scripts_timeout_test.go | 69 ++++++++++++++++++++++++ 4 files changed, 86 insertions(+), 11 deletions(-) diff --git a/pkg/actionpins/data/action_pins.json b/pkg/actionpins/data/action_pins.json index d58af0d9de2..b3fc349a038 100644 --- a/pkg/actionpins/data/action_pins.json +++ b/pkg/actionpins/data/action_pins.json @@ -168,10 +168,10 @@ "version": "v2.2.0", "sha": "0c5077e51419868618aeaa5fe8019c62421857d6" }, - "ruby/setup-ruby@v1.305.0": { + "ruby/setup-ruby@v1.306.0": { "repo": "ruby/setup-ruby", - "version": "v1.305.0", - "sha": "0cb964fd540e0a24c900370abf38a33466142735" + "version": "v1.306.0", + "sha": "c4e5b1316158f92e3d49443a9d58b31d25ac0f8f" }, "super-linter/super-linter@v8.6.0": { "repo": "super-linter/super-linter", diff --git a/pkg/workflow/data/action_pins.json b/pkg/workflow/data/action_pins.json index d58af0d9de2..b3fc349a038 100644 --- a/pkg/workflow/data/action_pins.json +++ b/pkg/workflow/data/action_pins.json @@ -168,10 +168,10 @@ "version": "v2.2.0", "sha": "0c5077e51419868618aeaa5fe8019c62421857d6" }, - "ruby/setup-ruby@v1.305.0": { + "ruby/setup-ruby@v1.306.0": { "repo": "ruby/setup-ruby", - "version": "v1.305.0", - "sha": "0cb964fd540e0a24c900370abf38a33466142735" + "version": "v1.306.0", + "sha": "c4e5b1316158f92e3d49443a9d58b31d25ac0f8f" }, "super-linter/super-linter@v8.6.0": { "repo": "super-linter/super-linter", diff --git a/pkg/workflow/mcp_scripts_parser.go b/pkg/workflow/mcp_scripts_parser.go index f26e0fc29e4..b2947f64d8e 100644 --- a/pkg/workflow/mcp_scripts_parser.go +++ b/pkg/workflow/mcp_scripts_parser.go @@ -2,7 +2,7 @@ package workflow import ( "encoding/json" - "fmt" + "strconv" "github.com/github/gh-aw/pkg/logger" "github.com/github/gh-aw/pkg/typeutil" @@ -180,8 +180,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, err := strconv.Atoi(t); err == nil { + toolConfig.Timeout = n + } else { + mcpScriptsLog.Printf("Warning: invalid timeout value %q for tool %q, using default 60s", t, toolName) + } } } @@ -351,8 +354,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, err := strconv.Atoi(t); err == nil { + toolConfig.Timeout = n + } else { + mcpScriptsLog.Printf("Warning: invalid timeout value %q for tool %q, using default 60s", t, toolName) + } } } diff --git a/pkg/workflow/mcp_scripts_timeout_test.go b/pkg/workflow/mcp_scripts_timeout_test.go index f2fbe4e7b9e..4fd7f3e901b 100644 --- a/pkg/workflow/mcp_scripts_timeout_test.go +++ b/pkg/workflow/mcp_scripts_timeout_test.go @@ -98,6 +98,34 @@ 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 + }, } for _, tt := range tests { @@ -244,3 +272,44 @@ 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) + } +} From 21939c09d71e7598f442e140434d1304016f11a2 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Mon, 27 Apr 2026 23:44:28 +0000 Subject: [PATCH 3/5] fix: clarify warning message to 'keeping default timeout (60s)' Agent-Logs-Url: https://github.com/github/gh-aw/sessions/a450004a-8130-4ca5-ad13-435bb4ae95ba Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com> --- pkg/workflow/mcp_scripts_parser.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/pkg/workflow/mcp_scripts_parser.go b/pkg/workflow/mcp_scripts_parser.go index b2947f64d8e..b44e88a1637 100644 --- a/pkg/workflow/mcp_scripts_parser.go +++ b/pkg/workflow/mcp_scripts_parser.go @@ -183,7 +183,7 @@ func parseMCPScriptsMap(mcpScriptsMap map[string]any) (*MCPScriptsConfig, bool) if n, err := strconv.Atoi(t); err == nil { toolConfig.Timeout = n } else { - mcpScriptsLog.Printf("Warning: invalid timeout value %q for tool %q, using default 60s", t, toolName) + mcpScriptsLog.Printf("Warning: invalid timeout value %q for tool %q, keeping default timeout (60s)", t, toolName) } } } @@ -357,7 +357,7 @@ func (c *Compiler) mergeMCPScripts(main *MCPScriptsConfig, importedConfigs []str if n, err := strconv.Atoi(t); err == nil { toolConfig.Timeout = n } else { - mcpScriptsLog.Printf("Warning: invalid timeout value %q for tool %q, using default 60s", t, toolName) + mcpScriptsLog.Printf("Warning: invalid timeout value %q for tool %q, keeping default timeout (60s)", t, toolName) } } } From 1c5ec05fae8eda6c2f86359e463e43573bd8d76d Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Tue, 28 Apr 2026 01:02:55 +0000 Subject: [PATCH 4/5] test: add more string timeout edge case tests (5m, empty string) Agent-Logs-Url: https://github.com/github/gh-aw/sessions/327bfddb-6d0e-494d-a561-66a3b995e9f6 Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com> --- pkg/workflow/mcp_scripts_timeout_test.go | 48 ++++++++++++++++++++++++ 1 file changed, 48 insertions(+) diff --git a/pkg/workflow/mcp_scripts_timeout_test.go b/pkg/workflow/mcp_scripts_timeout_test.go index 4fd7f3e901b..6fc15f8078c 100644 --- a/pkg/workflow/mcp_scripts_timeout_test.go +++ b/pkg/workflow/mcp_scripts_timeout_test.go @@ -126,6 +126,36 @@ func TestMCPScriptsTimeoutParsing(t *testing.T) { toolName: "bad-timeout-tool", expectedTimeout: 60, // Default timeout }, + { + // "5m" looks like a duration string; strconv.Atoi rejects it entirely + // (old fmt.Sscanf would have silently parsed it as 5s, which is wrong) + name: "duration-style string timeout falls back to default", + 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: 60, // Default timeout + }, + { + 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 + }, } for _, tt := range tests { @@ -312,4 +342,22 @@ func TestMCPScriptsMergeStringTimeout(t *testing.T) { 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"). + // The old fmt.Sscanf code would have silently parsed this as 5s (wrong). + // The new strconv.Atoi code correctly rejects it and falls back to 60s. + durationImportedJSON := `{ + "duration-string-timeout": { + "description": "Tool with duration-style string timeout", + "script": "return 'ok';", + "timeout": "5m" + } + }` + + merged3 := compiler.mergeMCPScripts(main, []string{durationImportedJSON}) + + // Verify duration-style string timeout falls back to default (60s) + if merged3.Tools["duration-string-timeout"].Timeout != 60 { + t.Errorf("Expected default timeout 60 for duration-style string, got %d", merged3.Tools["duration-string-timeout"].Timeout) + } } From e85eefbd1c99516ee10d595beb77dcf8fc27d4ad Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Tue, 28 Apr 2026 01:22:11 +0000 Subject: [PATCH 5/5] feat: parse duration strings (6m, 1h) in mcp_scripts timeout; merge main Agent-Logs-Url: https://github.com/github/gh-aw/sessions/bc046142-acbe-4dae-a949-7ce00ed98fc5 Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com> --- .../docs/reference/frontmatter-full.md | 3 +- pkg/workflow/mcp_scripts_parser.go | 26 ++++- pkg/workflow/mcp_scripts_timeout_test.go | 110 ++++++++++++++++-- 3 files changed, 126 insertions(+), 13 deletions(-) diff --git a/docs/src/content/docs/reference/frontmatter-full.md b/docs/src/content/docs/reference/frontmatter-full.md index a308f718e79..7ba4d00e9c0 100644 --- a/docs/src/content/docs/reference/frontmatter-full.md +++ b/docs/src/content/docs/reference/frontmatter-full.md @@ -825,12 +825,11 @@ on: # Option 1: Single label name that must match the triggering label (e.g., # 'panel-review') - labels: "example-value" # Option 2: List of label names; the workflow fires when the triggering label # matches any entry. labels: [] - # Array items: Label name (e.g., 'panel-review', 'needs-triage') + # Array items: undefined # Environment name that requires manual approval before the workflow can run. Must # match a valid environment configured in the repository settings. diff --git a/pkg/workflow/mcp_scripts_parser.go b/pkg/workflow/mcp_scripts_parser.go index 7412b24a923..8df2e8af135 100644 --- a/pkg/workflow/mcp_scripts_parser.go +++ b/pkg/workflow/mcp_scripts_parser.go @@ -3,6 +3,8 @@ package workflow import ( "encoding/json" "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,7 +201,7 @@ func parseMCPScriptsMap(mcpScriptsMap map[string]any) (*MCPScriptsConfig, bool) case float64: toolConfig.Timeout = int(t) case string: - if n, err := strconv.Atoi(t); err == nil { + 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) @@ -353,7 +375,7 @@ func (c *Compiler) mergeMCPScripts(main *MCPScriptsConfig, importedConfigs []str case float64: toolConfig.Timeout = int(t) case string: - if n, err := strconv.Atoi(t); err == nil { + 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 6fc15f8078c..08ad8ad0e09 100644 --- a/pkg/workflow/mcp_scripts_timeout_test.go +++ b/pkg/workflow/mcp_scripts_timeout_test.go @@ -127,9 +127,7 @@ func TestMCPScriptsTimeoutParsing(t *testing.T) { expectedTimeout: 60, // Default timeout }, { - // "5m" looks like a duration string; strconv.Atoi rejects it entirely - // (old fmt.Sscanf would have silently parsed it as 5s, which is wrong) - name: "duration-style string timeout falls back to default", + name: "duration minutes string timeout is parsed", frontmatter: map[string]any{ "mcp-scripts": map[string]any{ "duration-tool": map[string]any{ @@ -140,7 +138,49 @@ func TestMCPScriptsTimeoutParsing(t *testing.T) { }, }, toolName: "duration-tool", - expectedTimeout: 60, // Default timeout + 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", @@ -156,6 +196,20 @@ func TestMCPScriptsTimeoutParsing(t *testing.T) { 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 { @@ -344,8 +398,7 @@ func TestMCPScriptsMergeStringTimeout(t *testing.T) { } // Imported config with duration-style string timeout ("5m"). - // The old fmt.Sscanf code would have silently parsed this as 5s (wrong). - // The new strconv.Atoi code correctly rejects it and falls back to 60s. + // time.ParseDuration now converts this to 300s. durationImportedJSON := `{ "duration-string-timeout": { "description": "Tool with duration-style string timeout", @@ -356,8 +409,47 @@ func TestMCPScriptsMergeStringTimeout(t *testing.T) { merged3 := compiler.mergeMCPScripts(main, []string{durationImportedJSON}) - // Verify duration-style string timeout falls back to default (60s) - if merged3.Tools["duration-string-timeout"].Timeout != 60 { - t.Errorf("Expected default timeout 60 for duration-style string, got %d", merged3.Tools["duration-string-timeout"].Timeout) + // 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) + } + }) } }