-
Notifications
You must be signed in to change notification settings - Fork 28
Fix timeout-minutes schema documentation: 20 minutes, not 15 #4030
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR corrects documentation in the schema to match the actual implementation: the default timeout for agentic workflows is 20 minutes, not 15 minutes. The default has always been 20 minutes via DefaultAgenticWorkflowTimeoutMinutes constant, but the schema documentation incorrectly stated 15 minutes.
Key changes:
- Updated schema documentation to correctly reflect the 20-minute default
- Added comprehensive test coverage to validate the default timeout behavior and prevent future documentation drift
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| pkg/parser/schemas/main_workflow_schema.json | Updated timeout documentation from "15 minutes" to "20 minutes" for both timeout-minutes and deprecated timeout_minutes fields |
| pkg/workflow/compiler_timeout_default_test.go | Added new test file with three test functions: validates default timeout applied when omitted, validates explicit timeouts respected, and validates schema documentation matches the constant value |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Helper function to convert int to string | ||
| func intToString(n int) string { | ||
| if n == 0 { | ||
| return "0" | ||
| } | ||
| if n < 0 { | ||
| return "-" + intToString(-n) | ||
| } | ||
| result := "" | ||
| for n > 0 { | ||
| result = string(rune('0'+n%10)) + result | ||
| n /= 10 | ||
| } | ||
| return result | ||
| } |
Copilot
AI
Nov 15, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This custom intToString helper function is unnecessary. Go's standard library provides strconv.Itoa() for integer-to-string conversion, which is already used elsewhere in the codebase (e.g., pkg/cli/logs.go, pkg/cli/mcp_server.go).
Recommended fix: Remove this function and use strconv.Itoa() on line 89 instead.
| expectedTimeoutStr := "timeout-minutes: " + string(rune(tt.expectedTimeout+'0')) | ||
| if tt.expectedTimeout >= 10 { | ||
| expectedTimeoutStr = "timeout-minutes: " + intToString(tt.expectedTimeout) | ||
| } |
Copilot
AI
Nov 15, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The logic for building expectedTimeoutStr is flawed. Line 87 attempts to convert an integer to a string using string(rune(tt.expectedTimeout+'0')), which works incorrectly for multi-digit numbers. For example, with expectedTimeout=20, this would compute string(rune(68)) = "D", producing "timeout-minutes: D".
While the code on lines 88-90 fixes this for values >= 10 by calling intToString, the initial assignment on line 87 is incorrect and unnecessary.
Recommended fix: Use Go's standard library strconv.Itoa() instead:
import (
"strconv"
// ... other imports
)
// In the test:
expectedTimeoutStr := "timeout-minutes: " + strconv.Itoa(tt.expectedTimeout)This eliminates the need for the custom intToString helper function (lines 121-135) and the conditional check on line 88.
The schema documentation incorrectly stated that
timeout-minutesdefaults to 15 minutes for agentic workflows. The actual implementation has always used 20 minutes viaDefaultAgenticWorkflowTimeoutMinutesconstant.Changes
Schema documentation (
pkg/parser/schemas/main_workflow_schema.json)timeout-minutesfield: "15 minutes" → "20 minutes"timeout_minutesfield: "15 minutes" → "20 minutes"Test coverage (
pkg/workflow/compiler_timeout_default_test.go)timeout-minutesomittedExample
Workflow without explicit timeout:
Generates step with correct default:
The 20-minute default is applied consistently across all engines (Copilot, Claude, Codex) and has been since initial implementation. No code changes required—only documentation correction.
Original prompt
💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.