Extract time validation and network port magic numbers to constants (#14545)

Author: Copilot

pkg/constants/constants.go

@@ -274,6 +274,28 @@ const MaxExpressionLineLength LineLength = 120
 // ExpressionBreakThreshold is the threshold for breaking long lines at logical points.
 const ExpressionBreakThreshold LineLength = 100
 
+// Network port constants
+//
+// These constants define standard network port values used throughout the codebase
+// for MCP servers, gateway services, and validation ranges.
+
+const (
+	// DefaultMCPGatewayPort is the default port for the MCP gateway HTTP service
+	DefaultMCPGatewayPort = 80
+
+	// DefaultMCPServerPort is the default port for MCP servers (safe-inputs server)
+	DefaultMCPServerPort = 3000
+
+	// DefaultMCPInspectorPort is the default port for the MCP inspector (safe-outputs server)
+	DefaultMCPInspectorPort = 3001
+
+	// MinNetworkPort is the minimum valid network port number
+	MinNetworkPort = 1
+
+	// MaxNetworkPort is the maximum valid network port number
+	MaxNetworkPort = 65535
+)
+
 // DefaultMCPRegistryURL is the default MCP registry URL.
 const DefaultMCPRegistryURL URL = "https://api.mcp.github.com/v0.1"
 

pkg/workflow/mcp_gateway_constants.go

@@ -2,37 +2,35 @@
 //
 // # MCP Gateway Constants
 //
-// This file defines default values and constants used by the MCP gateway
-// throughout the workflow compilation process. These constants ensure
-// consistent configuration across different components.
+// This file provides access to MCP gateway configuration constants.
 //
 // Gateway default values:
-//   - Port: 80 (HTTP standard port)
+//   - Port: 80 (HTTP standard port) - defined in pkg/constants
 //
 // The MCP gateway port is used when:
 //   - No custom port is specified in sandbox.mcp.port
 //   - Building gateway configuration in mcp_gateway_config.go
 //   - Generating gateway startup commands in mcp_setup_generator.go
 //
 // Historical note:
-// This constant was originally used with the awmg gateway binary.
-// The binary has been removed but the constant is retained for
-// backwards compatibility with existing workflow configurations.
+// This constant was originally defined locally but has been moved to pkg/constants
+// for centralization with other network port constants.
 //
 // Related files:
 //   - mcp_gateway_config.go: Uses DefaultMCPGatewayPort for configuration
 //   - mcp_setup_generator.go: Uses port for gateway startup
-//   - constants/constants.go: Other MCP-related constants (versions, containers)
+//   - constants/constants.go: Defines all MCP-related constants (versions, containers, ports)
 //
 // Related constants in pkg/constants:
+//   - DefaultMCPGatewayPort: Gateway port (80)
 //   - DefaultMCPGatewayVersion: Gateway container version
 //   - DefaultMCPGatewayContainer: Gateway container image
 //   - DefaultGitHubMCPServerVersion: GitHub MCP server version
 package workflow
 
-const (
-	// DefaultMCPGatewayPort is the default port for the MCP gateway
-	// This constant is kept for backwards compatibility with existing configurations
-	// even though the awmg gateway binary has been removed.
-	DefaultMCPGatewayPort = 80
-)
+import "github.com/github/gh-aw/pkg/constants"
+
+// DefaultMCPGatewayPort is the default port for the MCP gateway
+// This is now an alias to the constant defined in pkg/constants
+// for backwards compatibility with existing code.
+const DefaultMCPGatewayPort = constants.DefaultMCPGatewayPort

pkg/workflow/mcp_setup_generator.go

@@ -259,7 +259,7 @@ func (c *Compiler) generateMCPSetup(yaml *strings.Builder, tools map[string]any,
 		yaml.WriteString("          # Generate a secure random API key (360 bits of entropy, 40+ chars)\n")
 		yaml.WriteString("          API_KEY=\"\"\n")
 		yaml.WriteString("          API_KEY=$(openssl rand -base64 45 | tr -d '/+=')\n")
-		yaml.WriteString("          PORT=3001\n")
+		fmt.Fprintf(yaml, "          PORT=%d\n", constants.DefaultMCPInspectorPort)
 		yaml.WriteString("          \n")
 		yaml.WriteString("          # Register API key as secret to mask it from logs\n")
 		yaml.WriteString("          echo \"::add-mask::${API_KEY}\"\n")
@@ -382,7 +382,7 @@ func (c *Compiler) generateMCPSetup(yaml *strings.Builder, tools map[string]any,
 		yaml.WriteString("          # Generate a secure random API key (360 bits of entropy, 40+ chars)\n")
 		yaml.WriteString("          API_KEY=\"\"\n")
 		yaml.WriteString("          API_KEY=$(openssl rand -base64 45 | tr -d '/+=')\n")
-		yaml.WriteString("          PORT=3000\n")
+		fmt.Fprintf(yaml, "          PORT=%d\n", constants.DefaultMCPServerPort)
 		yaml.WriteString("          \n")
 		yaml.WriteString("          # Register API key as secret to mask it from logs\n")
 		yaml.WriteString("          echo \"::add-mask::${API_KEY}\"\n")

pkg/workflow/sandbox_validation.go

@@ -155,7 +155,7 @@ func validateSandboxConfig(workflowData *WorkflowData) error {
 
 	// Validate MCP gateway port if configured
 	if sandboxConfig.MCP != nil && sandboxConfig.MCP.Port != 0 {
-		if err := validateIntRange(sandboxConfig.MCP.Port, 1, 65535, "sandbox.mcp.port"); err != nil {
+		if err := validateIntRange(sandboxConfig.MCP.Port, constants.MinNetworkPort, constants.MaxNetworkPort, "sandbox.mcp.port"); err != nil {
 			return err
 		}
 		sandboxValidationLog.Printf("Validated MCP gateway port: %d", sandboxConfig.MCP.Port)

pkg/workflow/time_delta.go

@@ -139,20 +139,20 @@ func parseTimeDeltaWithMinutes(deltaStr string, allowMinutes bool) (*TimeDelta,
 	}
 
 	// Validate reasonable limits
-	if delta.Months > 12 {
-		return nil, fmt.Errorf("time delta too large: %d months exceeds maximum of 12 months", delta.Months)
+	if delta.Months > MaxTimeDeltaMonths {
+		return nil, fmt.Errorf("time delta too large: %d months exceeds maximum of %d months", delta.Months, MaxTimeDeltaMonths)
 	}
-	if delta.Weeks > 52 {
-		return nil, fmt.Errorf("time delta too large: %d weeks exceeds maximum of 52 weeks", delta.Weeks)
+	if delta.Weeks > MaxTimeDeltaWeeks {
+		return nil, fmt.Errorf("time delta too large: %d weeks exceeds maximum of %d weeks", delta.Weeks, MaxTimeDeltaWeeks)
 	}
-	if delta.Days > 365 {
-		return nil, fmt.Errorf("time delta too large: %d days exceeds maximum of 365 days", delta.Days)
+	if delta.Days > MaxTimeDeltaDays {
+		return nil, fmt.Errorf("time delta too large: %d days exceeds maximum of %d days", delta.Days, MaxTimeDeltaDays)
 	}
-	if delta.Hours > 8760 { // 365 * 24
-		return nil, fmt.Errorf("time delta too large: %d hours exceeds maximum of 8760 hours", delta.Hours)
+	if delta.Hours > MaxTimeDeltaHours {
+		return nil, fmt.Errorf("time delta too large: %d hours exceeds maximum of %d hours", delta.Hours, MaxTimeDeltaHours)
 	}
-	if delta.Minutes > 525600 { // 365 * 24 * 60
-		return nil, fmt.Errorf("time delta too large: %d minutes exceeds maximum of 525600 minutes", delta.Minutes)
+	if delta.Minutes > MaxTimeDeltaMinutes {
+		return nil, fmt.Errorf("time delta too large: %d minutes exceeds maximum of %d minutes", delta.Minutes, MaxTimeDeltaMinutes)
 	}
 
 	timeDeltaLog.Printf("Parsed time delta successfully: %s", delta.String())

pkg/workflow/time_delta_constants.go

@@ -0,0 +1,32 @@
+package workflow
+
+// Time delta validation limits
+//
+// Policy: Maximum stop-after time is 1 year to prevent scheduling too far in the future.
+// These constants define the maximum allowed values for each time unit when parsing
+// time deltas in workflow schedules. The limits ensure workflows don't schedule actions
+// unreasonably far into the future, which could indicate configuration errors or create
+// operational challenges.
+//
+// All limits are equivalent to approximately 1 year:
+//   - 12 months = 1 year (exact)
+//   - 52 weeks = 364 days ≈ 1 year
+//   - 365 days = 1 year (non-leap year)
+//   - 8760 hours = 365 days * 24 hours
+//   - 525600 minutes = 365 days * 24 hours * 60 minutes
+const (
+	// MaxTimeDeltaMonths is the maximum allowed months in a time delta (1 year)
+	MaxTimeDeltaMonths = 12
+
+	// MaxTimeDeltaWeeks is the maximum allowed weeks in a time delta (approximately 1 year)
+	MaxTimeDeltaWeeks = 52
+
+	// MaxTimeDeltaDays is the maximum allowed days in a time delta (1 year, non-leap)
+	MaxTimeDeltaDays = 365
+
+	// MaxTimeDeltaHours is the maximum allowed hours in a time delta (365 days * 24 hours)
+	MaxTimeDeltaHours = 8760
+
+	// MaxTimeDeltaMinutes is the maximum allowed minutes in a time delta (365 days * 24 hours * 60 minutes)
+	MaxTimeDeltaMinutes = 525600
+)