Enforce explicit job and step timeouts on Visual Regression Checker workflow (#56980)

Author: Copilot

.github/workflows/visual-regression-checker.lock.yml

@@ -35,7 +35,11 @@ network:
     - playwright
     - local
     - node
+jobs:
+  agent:
+    timeout-minutes: 15
 safe-outputs:
+  timeout-minutes: 10
   add-comment:
     max: 1
 timeout-minutes: 15
@@ -54,10 +58,12 @@ steps:
 
   - name: Install dependencies
     working-directory: ./docs
+    timeout-minutes: 5
     run: npm ci
 
   - name: Build documentation
     working-directory: ./docs
+    timeout-minutes: 5
     run: npm run build
 
   - name: Start docs server
@@ -70,6 +76,7 @@ steps:
 
   - name: Wait for server readiness
     # runner-guard:ignore RGS-012 -- loopback-only port/readiness checks for the docs server started in this job; no external network or secrets are involved.
+    timeout-minutes: 2
     run: |
       MAX_WAIT=90
       WAITED=0

.github/workflows/visual-regression-checker.md

@@ -5,13 +5,18 @@ package cli
 import (
 	"os"
 	"path/filepath"
+	"regexp"
 	"testing"
 
 	"github.com/github/gh-aw/pkg/gitutil"
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 )
 
+// explicitMainAgentModelPattern matches a top-level, provider-qualified model
+// declaration (for example "model: openai/gpt-5.4") in the workflow frontmatter.
+var explicitMainAgentModelPattern = regexp.MustCompile(`(?m)^model: \S+/\S+$`)
+
 func TestPRCodeQualityReviewerWorkflowSubAgentModelContract(t *testing.T) {
 	t.Parallel()
 	repoRoot, err := gitutil.FindGitRoot()
@@ -24,7 +29,7 @@ func TestPRCodeQualityReviewerWorkflowSubAgentModelContract(t *testing.T) {
 	require.NoError(t, err, "Should read pr-code-quality-reviewer workflow")
 
 	text := string(content)
-	assert.Contains(t, text, "model: copilot/gpt-5.4", "Main agent should use an explicit Copilot model")
+	assert.Regexp(t, explicitMainAgentModelPattern, text, "Main agent should use an explicit provider-qualified model")
 	assert.Contains(t, text, "## agent: `grumpy-coder`", "Workflow should define the grumpy-coder sub-agent")
 	assert.Contains(t, text, "model: small", "Sub-agent should use the portable small alias")
 	assert.NotContains(t, text, "model: inherited", "Sub-agent should not inherit an unsupported tier-specific model")

pkg/cli/pr_code_quality_reviewer_workflow_contract_test.go

@@ -4,6 +4,7 @@ import (
 	"errors"
 	"fmt"
 	"maps"
+	"math"
 	"strconv"
 
 	"github.com/github/gh-aw/pkg/importinpututil"
@@ -117,34 +118,13 @@ func MapToStep(stepMap map[string]any) (*WorkflowStep, error) {
 		step.With = with
 	}
 	if env, ok := stepMap["env"].(map[string]any); ok {
-		// Convert map[string]any to map[string]string
-		step.Env = make(map[string]string)
-		for k, v := range env {
-			if strVal, ok := v.(string); ok {
-				step.Env[k] = strVal
-			} else if v != nil {
-				// Arrays and maps are serialized as JSON so that shell consumers
-				// (e.g. jq --argjson) receive valid JSON. This handles both the
-				// []any / map[string]any case returned by encoding/json and the
-				// typed-slice case (e.g. []string) returned by goccy/go-yaml.
-				step.Env[k] = marshalEnvValue(v)
-			}
-		}
+		step.Env = parseStepEnv(env)
 	}
 	if continueOnError, ok := stepMap["continue-on-error"]; ok {
-		switch value := continueOnError.(type) {
-		case bool:
-			templatableValue := TemplatableBool(strconv.FormatBool(value))
-			step.ContinueOnError = &templatableValue
-		case string:
-			if value == "true" || value == "false" || isExpression(value) {
-				templatableValue := TemplatableBool(value)
-				step.ContinueOnError = &templatableValue
-			}
-		}
+		step.ContinueOnError = parseStepContinueOnError(continueOnError)
 	}
-	if timeoutMinutes, ok := stepMap["timeout-minutes"].(int); ok {
-		step.TimeoutMinutes = timeoutMinutes
+	if timeoutMinutesVal, ok := stepMap["timeout-minutes"]; ok {
+		step.TimeoutMinutes = parseStepTimeoutMinutes(timeoutMinutesVal)
 	}
 
 	stepType := "unknown"
@@ -157,6 +137,67 @@ func MapToStep(stepMap map[string]any) (*WorkflowStep, error) {
 	return step, nil
 }
 
+func parseStepEnv(env map[string]any) map[string]string {
+	result := make(map[string]string)
+	for k, v := range env {
+		if strVal, ok := v.(string); ok {
+			result[k] = strVal
+		} else if v != nil {
+			result[k] = marshalEnvValue(v)
+		}
+	}
+	return result
+}
+
+func parseStepContinueOnError(val any) *TemplatableBool {
+	switch value := val.(type) {
+	case bool:
+		templatableValue := TemplatableBool(strconv.FormatBool(value))
+		return &templatableValue
+	case string:
+		if value == "true" || value == "false" || isExpression(value) {
+			templatableValue := TemplatableBool(value)
+			return &templatableValue
+		}
+	}
+	return nil
+}
+
+// parseStepTimeoutMinutes converts a YAML `timeout-minutes` value into a positive
+// number of minutes. Values that are not positive integers within the platform int
+// range are ignored (returning 0, which omits the field from the rendered step).
+func parseStepTimeoutMinutes(val any) int {
+	switch v := val.(type) {
+	case int:
+		if v > 0 {
+			return v
+		}
+	case int64:
+		if v > 0 && v <= int64(math.MaxInt) {
+			return int(v)
+		}
+	case uint64:
+		if v > 0 && v <= uint64(math.MaxInt) {
+			return int(v)
+		}
+	case float64:
+		// float64 loses integer precision near MaxInt on 64-bit platforms, so treat
+		// values at or above the rounded float boundary as out of range. Only
+		// integral values are accepted so fractional timeouts are not truncated.
+		if math.IsNaN(v) || math.IsInf(v, 0) || v != math.Trunc(v) {
+			return 0
+		}
+		if v >= 1 && v < float64(math.MaxInt) {
+			return int(v)
+		}
+	case string:
+		if n, err := strconv.Atoi(v); err == nil && n > 0 {
+			return n
+		}
+	}
+	return 0
+}
+
 // Clone creates a deep copy of the WorkflowStep
 func (s *WorkflowStep) Clone() *WorkflowStep {
 	clone := &WorkflowStep{

pkg/workflow/step_types.go

@@ -3,6 +3,7 @@
 package workflow
 
 import (
+	"math"
 	"testing"
 
 	"github.com/stretchr/testify/assert"
@@ -663,6 +664,46 @@ func TestSliceToSteps_RoundTrip(t *testing.T) {
 	}
 }
 
+func TestMapToStep_TimeoutMinutesNumericTypes(t *testing.T) {
+	tests := []struct {
+		name string
+		val  any
+		want int
+	}{
+		{"int", 5, 5},
+		{"int64", int64(10), 10},
+		{"uint64", uint64(15), 15},
+		{"float64", float64(20), 20},
+		{"string", "25", 25},
+		{"negative int", -5, 0},
+		{"zero int", 0, 0},
+		{"negative int64", int64(-10), 0},
+		{"zero uint64", uint64(0), 0},
+		{"negative float64", float64(-20), 0},
+		{"fractional float64", 1.9, 0},
+		{"out of range float64", math.MaxFloat64, 0},
+		{"NaN float64", math.NaN(), 0},
+		{"negative string", "-25", 0},
+		{"zero string", "0", 0},
+		{"non-numeric string", "abc", 0},
+		{"bool", true, 0},
+		{"nil", nil, 0},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			stepMap := map[string]any{
+				"name":            "Test",
+				"run":             "echo test",
+				"timeout-minutes": tt.val,
+			}
+			step, err := MapToStep(stepMap)
+			require.NoError(t, err)
+			assert.Equal(t, tt.want, step.TimeoutMinutes)
+		})
+	}
+}
+
 func TestMapToStep_InvalidTypes(t *testing.T) {
 	tests := []struct {
 		name        string