fix(pr-code-quality-reviewer): increase timeout-minutes from 15 → 20#43514
Conversation
…ent sub-agent cascade timeout The grumpy-coder sub-agent (SDK idle-timeout ~870s/14.5min) was running up to the 15-minute step wall, leaving no buffer for Docker container cleanup. This caused 'Execute GitHub Copilot CLI' to time out and the workflow to fail. Increase timeout-minutes from 15 → 20 and update the in-prompt reference and the contract test assertion accordingly. Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR increases the PR Code Quality Reviewer workflow timeout from 15 to 20 minutes to prevent the grumpy-coder sub-agent run from being terminated by the GitHub Actions wall-clock timeout during teardown.
Changes:
- Bump
timeout-minutesfrom 15 → 20 in the workflow frontmatter and update the inline “Respect time” guideline accordingly. - Recompile the generated
.lock.ymlto propagate the new timeout into the executable workflow (includingGH_AW_TIMEOUT_MINUTES). - Add a contract test assertion to prevent the timeout from silently regressing.
Show a summary per file
| File | Description |
|---|---|
.github/workflows/pr-code-quality-reviewer.md |
Raises workflow timeout to 20 minutes and updates the prompt guideline to match. |
.github/workflows/pr-code-quality-reviewer.lock.yml |
Recompiled lock file reflecting the 20-minute step timeout and corresponding env values. |
pkg/cli/pr_code_quality_reviewer_workflow_contract_test.go |
Adds an assertion to catch timeout regressions in the source workflow file. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 3/3 changed files
- Comments generated: 1
- Review effort level: Low
| // The timeout must be >= 20 minutes to give grumpy-coder sub-agent (SDK idle-timeout ~14.5 min) | ||
| // adequate headroom before the step-level timeout fires. | ||
| assert.Contains(t, text, "timeout-minutes: 20", "Workflow timeout should be at least 20 minutes to prevent sub-agent timeout cascade") |
|
✅ PR Code Quality Reviewer completed the code quality review. |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR #43514 does not have the 'implementation' label and has only 3 new lines of code in business logic directories (threshold: 100). |
|
✅ Test Quality Sentinel completed test quality analysis. |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
There was a problem hiding this comment.
Review: fix(pr-code-quality-reviewer): increase timeout-minutes from 15 → 20
The fix is well-reasoned and all three files are consistent:
- Source (
pr-code-quality-reviewer.md):timeout-minutesand the prompt guideline both updated. - Lock file: All three occurrences updated (
timeout-minutesstep field, step-envGH_AW_TIMEOUT_MINUTES, and failure-script env). - Contract test: Adds a regression guard so a future silent downgrade is caught immediately.
Non-blocking observation: The test comment says "The timeout must be ≥ 20 minutes" but assert.Contains(t, text, "timeout-minutes: 20", ...) does an exact-substring match — it would pass for timeout-minutes: 200 and fail for timeout-minutes: 25. The comment/error message implies >= semantics but the assertion enforces substring match only. If the intent is an exact pin, the comment should say exactly 20 to avoid confusion. Cosmetic issue only — the timeout value of 20 min is correct.
Approved. ✅
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · 16.1 AIC · ⌖ 5.97 AIC · ⊞ 4.9K
🧪 Test Quality Sentinel Report✅ Test Quality Score: 100/100 — Excellent
📊 Metrics (1 test)
Verdict
Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
network:
allowed:
- defaults
- "awmgmcpg"See Network Configuration for more information.
|
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /diagnosing-bugs and /tdd — approving with one minor suggestion on the test assertion.
📋 Key Themes & Highlights
Positive Highlights
- ✅ Root cause correctly identified and clearly documented in the PR description (SDK idle-timeout ~870s + container teardown headroom)
- ✅ Surgical change — only three files touched, each for a clear reason
- ✅ Regression test added via contract test, preventing silent rollback
- ✅ Lock file correctly regenerated from source (consistent metadata hashes)
- ✅ Inline agent guidance updated to match the new timeout value
One Minor Note
The new contract test comment says "must be >= 20 minutes" but the assert.Contains checks for the exact string timeout-minutes: 20. If the timeout is ever bumped again, the test will fail with a message that contradicts its own intent. See the inline comment for two resolution options.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · 31 AIC · ⌖ 5.44 AIC · ⊞ 6.6K
Comment /matt to run again
| assert.Contains(t, text, "## agent: `grumpy-coder`", "Workflow should define the grumpy-coder sub-agent") | ||
| assert.Contains(t, text, "model: claude-haiku-4.5", "Sub-agent should pin a supported Haiku model") | ||
| assert.NotContains(t, text, "model: inherited", "Sub-agent should not inherit an unsupported tier-specific model") | ||
| // The timeout must be >= 20 minutes to give grumpy-coder sub-agent (SDK idle-timeout ~14.5 min) |
There was a problem hiding this comment.
[/tdd] The comment says "must be >= 20 minutes" but the assertion checks for the exact string timeout-minutes: 20 — if the timeout is later bumped to 25+, the test fails with a message that contradicts its own stated intent.
💡 Suggested improvement
Option 1 — parse and assert >= 20 (matches the comment's intent):
re := regexp.MustCompile(`timeout-minutes: (\d+)`)
m := re.FindStringSubmatch(text)
require.NotNil(t, m, "timeout-minutes not found")
timeoutMinutes, _ := strconv.Atoi(m[1])
assert.GreaterOrEqual(t, timeoutMinutes, 20, "Workflow timeout should be at least 20 minutes to prevent sub-agent timeout cascade")Option 2 — align the comment to the assertion's exact semantics:
// Timeout is pinned to exactly 20 minutes; update both source and this test together if it changes.
assert.Contains(t, text, "timeout-minutes: 20", ...)Option 1 is more resilient if the timeout grows further.
@copilot please address this.
There was a problem hiding this comment.
Non-blocking observation
The timeout bump from 15 → 20 is correctly applied across all three surfaces (.md frontmatter, lock.yml step timeout-minutes, and the two GH_AW_TIMEOUT_MINUTES env vars). Lock file hashes are coherent with the source change.
One non-blocking concern
Test comment vs. assertion intent mismatch (pkg/cli/pr_code_quality_reviewer_workflow_contract_test.go:31): the comment says >= 20 minutes but assert.Contains enforces an exact string match on "timeout-minutes: 20". The fix direction is correct; only the comment or assertion message needs to be aligned so the next timeout bump isn't surprising. See inline comment.
🔎 Code quality review by PR Code Quality Reviewer · 87 AIC · ⌖ 5.43 AIC · ⊞ 5.4K
Comment /review to run again
Comments that could not be inline-anchored
pkg/cli/pr_code_quality_reviewer_workflow_contract_test.go:31
Test asserts equality, not the lower bound the comment promises: assert.Contains(t, text, "timeout-minutes: 20", ...) checks that the string "timeout-minutes: 20" appears verbatim — meaning this test will fail if the timeout is ever raised to 25 or 30, forcing an unnecessary coordinated update.
<details>
<summary>💡 Suggested fix</summary>
Either weaken the assertion to match the >= 20 intent expressed in the comment, or rename the assertion message to reflect it is an exact equalit…
pkg/cli/pr_code_quality_reviewer_workflow_contract_test.go:31
False-positive risk: assert.Contains(t, text, "timeout-minutes: 20") is a substring match — it would silently pass if the value were "timeout-minutes: 200" or "timeout-minutes: 2000", since "20" is a prefix of both.
<details>
<summary>💡 Suggested fix</summary>
Use word-boundary matching or parse the integer to avoid prefix collisions:
// Safe: exact line match
assert.Contains(t, text, "timeout-minutes: 20\n", "Workflow timeout must be exactly 20 minutes")
// Or: lower-bou…
</details>
<details><summary>pkg/cli/pr_code_quality_reviewer_workflow_contract_test.go:23</summary>
**Test validates the source `.md` but not the compiled artifact that Actions actually runs**: the contract test reads `pr-code-quality-reviewer.md` and asserts `timeout-minutes: 20` in the frontmatter, but the file GitHub Actions executes is `pr-code-quality-reviewer.lock.yml`. If the lock file is stale (e.g., someone edits `.md` without running `gh aw compile`), this test passes and the regression lands in production.
<details>
<summary>💡 Suggested fix</summary>
Add a parallel assertion tha…
</details>|
@copilot please run the
|
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Done. Fixed the misleading comment in the contract test — changed ">= 20 minutes" to "exactly 20 minutes" to accurately describe what the |
🤖 PR Triage — Run §28741077056
Rationale: One-line timeout bump (15→20 min) with 3× APPROVED reviews. Tiny targeted fix preventing wall-clock kills on grumpy-coder. CI-clean, low risk — auto-merge candidate.
|
|
@copilot please run the
|
The
grumpy-codersub-agent has an SDK idle-timeout of ~870s (14.5 min). Withtimeout-minutes: 15, only ~30s remained for Docker container teardown — not enough, causing theExecute GitHub Copilot CLIstep to be killed by GitHub Actions' wall-clock timer.The Jul 3 failure (§28661680929) was a separate
SyntaxError: await is only valid in async functionsinparse_mcp_gateway_log.cjscaused by a reviewed PR introducingawaitinto a non-async function; that bug is already fixed inmain.Changes
.github/workflows/pr-code-quality-reviewer.md—timeout-minutes: 15 → 20; update inline prompt guideline to reference the new limit.github/workflows/pr-code-quality-reviewer.lock.yml— recompiled from sourcepkg/cli/pr_code_quality_reviewer_workflow_contract_test.go— asserttimeout-minutes: 20to prevent silent regression