Enforce explicit job and step timeouts on Visual Regression Checker workflow - #56980
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
…low and add bounds check for step timeout-minutes Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅
|
|
Warning Firewall blocked 4 domainsThe following domains were blocked by the firewall during workflow execution:
[!TIP] tools:
github:
mode: gh-proxySee GitHub Tools for more information on To allow these domains, add them to the network:
allowed:
- defaults
- "ab.chatgpt.com"
- "api.github.com"
- "chatgpt.com"
- "github.com"See Network Configuration for more information.
|
|
✅ Test Quality Sentinel completed test quality analysis. Test Quality Sentinel skipped because pre-fetch PR data was unavailable: unable to fetch test file diff
|
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. See the comment below for the result and any generated ADR draft. No ADR enforcement needed: PR does not have the 'implementation' label and has ≤100 new lines of code in business logic directories.
|
|
✅ PR Code Quality Reviewer completed the code quality review. Completed PR review for #56980 using local diff/comments; actionable findings will be surfaced in the final review if any exist.
|
Comment MemoryNote This comment is managed by comment memory.It stores persistent context for this thread in the code block at the top of this comment.
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Verdict
Non-blocking from the changed lines I reviewed.
Details
I checked the workflow timeout additions and the new timeout-minutes parsing path. The workflow changes do what the PR claims, and I did not find a changed-line bug that clearly warrants blocking merge.
One caveat: parseStepTimeoutMinutes now accepts more input shapes than the documented frontmatter contract elsewhere in the repo, but that inconsistency predates this review surface and is not a changed-line break by itself.
🔎 Code quality review by PR Code Quality Reviewer · pi · gpt54 · 19.3 AIC · ⌖ 7.28 AIC · ⊞ 7.2K
Comment /review to run again
There was a problem hiding this comment.
The PR is mostly lean; the only clear cut is collapsing the one-caller parsing helpers back into MapToStep to avoid indirection. net: -20 lines possible.
Warning
Firewall blocked 4 domains
The following domains were blocked by the firewall during workflow execution:
ab.chatgpt.comapi.github.comchatgpt.comgithub.com
[!TIP]
api.github.com is blocked because GitHub API access uses the built-in GitHub tools by default. Instead of adding api.github.com to network.allowed, use tools.github.mode: gh-proxy for direct pre-authenticated GitHub CLI access without requiring network access to api.github.com:
tools:
github:
mode: gh-proxySee GitHub Tools for more information on gh-proxy mode.
To allow these domains, add them to the network.allowed list in your workflow frontmatter:
network:
allowed:
- defaults
- "ab.chatgpt.com"
- "api.github.com"
- "chatgpt.com"
- "github.com"See Network Configuration for more information.
Generated by ✂️ Ponytail Reviewer for #56980 · codex · mai10 · 4.28 AIC · ⌖ 0.441 AIC · ⊞ 14K
Comment /ponytail to run again
| return step, nil | ||
| } | ||
|
|
||
| func parseStepEnv(env map[string]any) map[string]string { |
There was a problem hiding this comment.
pkg/workflow/step_types.go:140: yagni: three one-caller parsing helpers for env/continue-on-error/timeout. Inline the small switch logic in MapToStep and keep the mapping local.
There was a problem hiding this comment.
Good PR — the timeout enforcement is well-scoped and the parseStepTimeoutMinutes refactor correctly handles multiple numeric types with appropriate bounds checks. Two non-blocking consistency issues were flagged:
intcase missing> 0guard — theint64,uint64, andfloat64branches all reject non-positive values, but theintcase does not.stringcase accepts negative strings —strconv.Atoi("-5")succeeds, so a negative string timeout would pass through silently.
Neither is blocking for this PR's primary goal, but both should be addressed for correctness.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · copilot · sonnet46 · 27.3 AIC · ⌖ 10.7 AIC · ⊞ 6.2K
| case int: | ||
| return v | ||
| case int64: | ||
| if v > 0 && v <= int64(math.MaxInt) { |
There was a problem hiding this comment.
The int case does not guard against zero or negative values, unlike the int64, uint64, and float64 branches which all check v > 0. A zero or negative timeout would be silently accepted and may behave unexpectedly.
Suggested fix:
case int:
if v > 0 {
return v
}@copilot please address this.
| case float64: | ||
| if v > 0 && v <= float64(math.MaxInt) { | ||
| return int(v) | ||
| } |
There was a problem hiding this comment.
The string case parses via strconv.Atoi but does not reject negative values like "-5". strconv.Atoi("-5") returns -5, nil so a negative string timeout would be silently accepted.
Suggested fix:
case string:
if n, err := strconv.Atoi(v); err == nil && n > 0 {
return n
}@copilot please address this.
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /tdd and /codebase-design — requesting changes on two correctness issues in parseStepTimeoutMinutes and its test coverage.
📋 Key Themes & Highlights
Issues Found
- Missing lower-bound guard (
step_types.go): Theintandstringbranches ofparseStepTimeoutMinutesdon't reject negative values, while theint64andfloat64branches do. Atimeout-minutes: -5in a workflow silently produces a negative int. - Test coverage gap (
step_types_test.go): The new table test only covers valid positive inputs. There are no cases for negative values, zero, or unsupported types, which means the inconsistent guard logic wasn't caught by the tests. - safe-outputs timeout headroom (
.lock.yml): Reducing from 45m → 10m is sensible to stop runaway jobs, but 10m may be tight if many write intents are queued post-agent. Consider 15–20m.
Positive Highlights
- ✅ Extracting
parseStepEnv,parseStepContinueOnError, andparseStepTimeoutMinutesas standalone helpers is a clean design improvement — easier to test, easier to read. - ✅ The
math.MaxIntbounds check onuint64andint64correctly prevents integer overflow. - ✅ Step-level timeouts on
npm ci,npm run build, and the server-readiness wait are well-chosen values that match realistic worst-case durations. - ✅ Tests are structured cleanly with table-driven subtests and clear names.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · copilot · sonnet46 · 20 AIC · ⌖ 10.8 AIC · ⊞ 7.6K
Comment /matt to run again
| return &templatableValue | ||
| } | ||
| } | ||
| return nil |
There was a problem hiding this comment.
[/tdd] The int case in parseStepTimeoutMinutes lacks the > 0 lower-bound guard present in the int64 and float64 branches, so a negative value like timeout-minutes: -5 passes through silently.
💡 Suggested fix + missing test
case int:
if v > 0 {
return v
}The string branch via strconv.Atoi has the same gap — a "-5" string returns a negative int. Both should return 0 (treated as absent) or produce a validation error.
A regression test covering invalid inputs (negative ints, negative strings, zero) would prevent this from regressing.
@copilot please address this.
| } | ||
| step, err := MapToStep(stepMap) | ||
| require.NoError(t, err) | ||
| assert.Equal(t, tt.want, step.TimeoutMinutes) |
There was a problem hiding this comment.
[/tdd] The new test TestMapToStep_TimeoutMinutesNumericTypes only covers happy-path values; there are no cases for invalid inputs (negative numbers, zero, unsupported types like bool). The gaps in the guard logic above mean these aren't caught.
💡 Suggested additional test cases
{"negative int", -1, 0},
{"zero", 0, 0},
{"negative float", float64(-3), 0},
{"negative string", "-5", 0},
{"bool (unsupported)", true, 0},Having these in the table would have caught the missing lower-bound guard before the PR was submitted.
@copilot please address this.
| issues: write | ||
| pull-requests: write | ||
| timeout-minutes: 45 | ||
| timeout-minutes: 10 |
There was a problem hiding this comment.
[/codebase-design] The safe-outputs job timeout was reduced from 45m → 10m. The safe-outputs job is responsible for all GitHub write operations after the agent completes. If a complex run produces many write intents, 10m may be too tight, causing the job to time out before all writes are committed.
💡 Consider a slightly larger buffer
The agent job is capped at 15m, which is reasonable. But safe-outputs processes the agent's outputs after it finishes, and depending on how many write operations are queued, it may need more than 10m on a slow runner. A value of 15m (matching the agent job) or 20m would provide safer headroom without re-introducing the original 45m problem.
@copilot please address this.
There was a problem hiding this comment.
🟡 Changes recommended
Float timeout parsing can truncate fractional values and permit an out-of-range conversion.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Adds explicit runtime bounds to prevent prolonged Visual Regression Checker runs.
Changes:
- Adds agent, safe-output, and setup-step timeouts.
- Expands step-timeout parsing and tests.
- Regenerates the compiled workflow.
File summaries
| File | Description |
|---|---|
.github/workflows/visual-regression-checker.md |
Configures explicit timeouts. |
.github/workflows/visual-regression-checker.lock.yml |
Updates generated workflow. |
pkg/workflow/step_types.go |
Parses additional timeout numeric types. |
pkg/workflow/step_types_test.go |
Tests supported timeout representations. |
Review details
- Files reviewed: 4/4 changed files
- Comments generated: 1
- Review effort level: Balanced
💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
| case float64: | ||
| if v > 0 && v <= float64(math.MaxInt) { | ||
| return int(v) | ||
| } |
|
@copilot Please address the latest review feedback on this PR, then run the Priority items from the current reviews:
Failed checks to fix:
Please also refresh the branch if needed while addressing the review comments. Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
To allow these domains, add them to the network:
allowed:
- defaults
- "github.com"See Network Configuration for more information.
|
|
@gh-aw-bot Unfortunately I hit an unexpected error while processing your comment. I've automatically reported this to GitHub. You can ask me to try again later by mentioning me in a new comment. If you want to contact GitHub about this error, please mention the following identifier so they can better serve you: Sorry for the inconvenience! |
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Addressed in
On the failing |
…ontract test Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Fixed in The failing job was The test now asserts a top-level provider-qualified model via regexp Test passes locally, and |
|
🎉 This pull request is included in a new release. Release: |
The Visual Regression Checker workflow lacked explicit job and setup step timeouts, causing hanging runs to occupy CI runners for 1.6–2.0 hours before failing. Top-level
timeout-minutesonly bounded theagentic_executionstep, leaving theagentjob (default 60m),safe_outputsjob (default 45m), and pre-execution setup steps unconstrained.Workflow Timeouts
jobs.agent.timeout-minutes: 15andsafe-outputs.timeout-minutes: 10in.github/workflows/visual-regression-checker.md.timeout-minutesto setup steps (npm ci: 5m,npm run build: 5m,Wait for server readiness: 2m)..github/workflows/visual-regression-checker.lock.yml.Compiler Step Timeout Parsing
MapToStepinpkg/workflow/step_types.goto handleuint64,int64,float64, and numericstringtypes for step-leveltimeout-minutes(emitted during YAML unmarshaling) withmath.MaxIntbounds checks.jobs: agent: timeout-minutes: 15 safe-outputs: timeout-minutes: 10 timeout-minutes: 15 steps: - name: Install dependencies working-directory: ./docs timeout-minutes: 5 run: npm ciRun: https://github.com/github/gh-aw/actions/runs/33279163536
Warning
Firewall blocked 1 domain
The following domain was blocked by the firewall during workflow execution:
github.comTo allow these domains, add them to the
network.allowedlist in your workflow frontmatter:See Network Configuration for more information.