feat: support duration string timeouts (6m, 1h) in mcp_scripts_parser#28817
Merged
feat: support duration string timeouts (6m, 1h) in mcp_scripts_parser#28817
Conversation
4 tasks
…parser.go 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>
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>
Copilot
AI
changed the title
[WIP] Fix log warning when string timeout cannot be parsed in mcp_scripts_parser.go
fix: warn on unparseable string timeout in mcp_scripts_parser
Apr 27, 2026
Collaborator
|
@copilot add tests |
Contributor
There was a problem hiding this comment.
Pull request overview
Adjusts MCP scripts timeout parsing so string-typed timeout values no longer fail silently, and adds regression tests for the updated behavior.
Changes:
- Update
mcp_scripts_parser.goto parse string timeouts viastrconv.Atoiand log a warning when invalid (instead of discarding parse errors). - Add tests covering numeric-string and invalid-string timeout values for both direct parsing and merge behavior.
- Bump the pinned
ruby/setup-rubyentry in action pins data files.
Show a summary per file
| File | Description |
|---|---|
pkg/workflow/mcp_scripts_parser.go |
Switches from fmt.Sscanf to strconv.Atoi for string timeout parsing and emits warnings on invalid values. |
pkg/workflow/mcp_scripts_timeout_test.go |
Adds test cases for valid/invalid string timeouts and merge-path coverage. |
pkg/workflow/data/action_pins.json |
Updates the ruby/setup-ruby pinned version/sha entry. |
pkg/actionpins/data/action_pins.json |
Mirrors the same ruby/setup-ruby pins update in the embedded pins dataset. |
Copilot's findings
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comments suppressed due to low confidence (1)
pkg/workflow/mcp_scripts_parser.go:361
- Same parsing concern as above:
strconv.Atoiwill fail on numeric strings with surrounding whitespace (a behavior change vs the previousfmt.Sscanf). Consider trimming whitespace before parsing so valid values like " 120 " don't incorrectly fall back to the default with a warning.
if n, err := strconv.Atoi(t); err == nil {
toolConfig.Timeout = n
} else {
mcpScriptsLog.Printf("Warning: invalid timeout value %q for tool %q, keeping default timeout (60s)", t, toolName)
}
- Files reviewed: 4/4 changed files
- Comments generated: 3
Comment on lines
+183
to
+187
| if n, err := strconv.Atoi(t); err == nil { | ||
| toolConfig.Timeout = n | ||
| } else { | ||
| mcpScriptsLog.Printf("Warning: invalid timeout value %q for tool %q, keeping default timeout (60s)", t, toolName) | ||
| } |
There was a problem hiding this comment.
strconv.Atoi rejects leading/trailing whitespace (e.g. " 120" or "120\n"), whereas the previous fmt.Sscanf("%d") would successfully parse these as 120. If the intent is to accept numeric strings with incidental whitespace while still warning on non-numeric values, trim the string (e.g. strings.TrimSpace) before parsing (and consider treating an empty string as invalid).
This issue also appears on line 357 of the same file.
Comment on lines
+171
to
+174
| "ruby/setup-ruby@v1.306.0": { | ||
| "repo": "ruby/setup-ruby", | ||
| "version": "v1.305.0", | ||
| "sha": "0cb964fd540e0a24c900370abf38a33466142735" | ||
| "version": "v1.306.0", | ||
| "sha": "c4e5b1316158f92e3d49443a9d58b31d25ac0f8f" |
There was a problem hiding this comment.
This file updates the pinned ruby/setup-ruby version/sha (v1.305.0 → v1.306.0), but the PR title/description are about mcp_scripts_parser timeout parsing. If this pins update is intentional, please call it out in the PR description (or split it into a separate PR) so the behavioral surface area is clear to reviewers.
Comment on lines
+171
to
+174
| "ruby/setup-ruby@v1.306.0": { | ||
| "repo": "ruby/setup-ruby", | ||
| "version": "v1.305.0", | ||
| "sha": "0cb964fd540e0a24c900370abf38a33466142735" | ||
| "version": "v1.306.0", | ||
| "sha": "c4e5b1316158f92e3d49443a9d58b31d25ac0f8f" |
There was a problem hiding this comment.
Same as in pkg/workflow/data/action_pins.json: this changes the pinned ruby/setup-ruby entry (v1.305.0 → v1.306.0) but isn't mentioned in the PR description, which focuses on mcp-scripts timeouts. Please document this additional change or move it to a dedicated PR.
Contributor
🧪 Test Quality Sentinel ReportTest Quality Score: 80/100✅ Excellent test quality
Test Classification Details
Analysis NotesAll three new tests directly verify behavioral contracts:
Build tag: ✅
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
✅ Test Quality Sentinel: 80/100. Test quality is excellent — 0% of new tests are implementation tests (threshold: 30%). All 3 new tests verify behavioral contracts with both valid-input and error/edge-case coverage. No coding-guideline violations detected.
Closed
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>
Contributor
Author
Added in commit 1c5ec05. The new test cases cover:
All four cases are exercised for both the |
Collaborator
|
@copilot merge main, use time parser to extract 6m 1h types of values |
…ng-string-timeout Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
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>
Contributor
Author
Done in commit e85eefb. Merged main and replaced
Added Warning Firewall rules blocked me from connecting to one or more addresses (expand for details)I tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
Copilot
AI
changed the title
fix: warn on unparseable string timeout in mcp_scripts_parser
feat: support duration string timeouts (6m, 1h) in mcp_scripts_parser
Apr 28, 2026
Collaborator
|
@copilot merge main and recompile |
…ng-string-timeout Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Contributor
Author
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Two sites in
mcp_scripts_parser.gosilently discardedfmt.Sscanferrors when parsing string-typedtimeoutvalues, leaving users with no feedback when writingtimeout: "abc"— the value would silently stay at 60s. Additionally, duration-style strings like"5m"or"1h"were not supported at all.Changes
mcp_scripts_parser.go— Both occurrences (inparseMCPScriptsMapandmergeMCPScripts) replaced with a sharedparseTimeoutStringhelper:"strings"and"time"importsparseTimeoutString(s string) (int, bool)helper that:strconv.Atoifor plain integer strings ("120"→ 120s)time.ParseDurationfor Go duration strings ("6m"→ 360s,"1h"→ 3600s,"30s"→ 30s,"1h30m"→ 5400s)mcp_scripts_timeout_test.go— Added test cases for:timeout: "120"→ parsed to 120stimeout: " 120 "(whitespace) → parsed to 120stimeout: "30s"→ parsed to 30stimeout: "6m"→ parsed to 360stimeout: "1h"→ parsed to 3600stimeout: "1h30m"→ parsed to 5400stimeout: "not-a-number"→ falls back to 60s defaulttimeout: ""(empty string) → falls back to 60s defaultTestMCPScriptsMergeStringTimeoutcovering valid, invalid, and duration-style string timeouts through the merge pathTestParseTimeoutString— direct unit tests for theparseTimeoutStringhelper covering all code paths