Skip to content

ci: allow longer PR review timeout retries - #5961

Merged
yiliang114 merged 1 commit into
QwenLM:mainfrom
yiliang114:codex/review-timeout-override
Jun 28, 2026
Merged

ci: allow longer PR review timeout retries#5961
yiliang114 merged 1 commit into
QwenLM:mainfrom
yiliang114:codex/review-timeout-override

Conversation

@yiliang114

@yiliang114 yiliang114 commented Jun 28, 2026

Copy link
Copy Markdown
Collaborator

What this PR does

Adds an explicit timeout override for maintainer-triggered @qwen-code /review comments. Maintainers can now retry a large PR review with @qwen-code /review --timeout=180 or @qwen-code /review timeout=180, while the default review timeout stays at 120 minutes.

When a review times out, the fallback PR comment now uses a clearer bold heading and tells maintainers exactly how to rerun the review with a longer timeout. Non-timeout failures still get a generic failure comment without suggesting that more time will help.

Why it's needed

Large PRs can exceed the normal review budget. The current fallback only says the review timed out, which leaves maintainers to infer the next action. This keeps automatic reviews bounded by default while giving maintainers an explicit, auditable way to spend more runner time when a large PR is worth another pass.

Reviewer Test Plan

How to verify

Comment @qwen-code /review --timeout=180 on an open PR and confirm the review step accepts timeout_minutes=180, giving Qwen 180 minutes while the GitHub Actions job is capped at 200 minutes to leave fallback-comment time. To verify fallback behavior, force the review step to hit the timeout path and confirm the PR comment includes @qwen-code /review --timeout=180.

Evidence (Before & After)

Before: timed-out reviews posted a generic italic fallback comment with no retry command. After: timed-out reviews post a bold timeout heading and include the longer-timeout retry command.

Tested on

OS Status
🍏 macOS ✅ tested
🪟 Windows N/A
🐧 Linux N/A

Environment (optional)

npx vitest run scripts/tests/qwen-resolve-workflow.test.js --config scripts/tests/vitest.config.ts passed. npx prettier --check scripts/tests/qwen-resolve-workflow.test.js passed. git diff --check passed. Local actionlint .github/workflows/qwen-code-pr-review.yml still reports the existing environment.deployment: false key on current main, so it was not used as a passing signal for this change.

Manual timeout verification

A workflow_dispatch dry-run was dispatched on PR head SHA 885309cdfa59c797e13d192775c7cbc5e3a35b9d with timeout_minutes=121: https://github.com/QwenLM/qwen-code/actions/runs/28325897701. Result: the run completed Resolve PR context and reached review-pr / Run review, staying in the actual review step for more than 8 minutes instead of failing the previous 120-minute timeout validation. This verifies that the manual timeout override path accepts a maintainer-provided value above the default 120 minutes; the full dry-run conclusion remains available in the linked run.

Risk & Scope

  • Main risk or tradeoff: maintainers can intentionally occupy the review runner longer when they request --timeout=180.
  • Not validated / out of scope: automatically sizing review time from PR line counts.
  • Breaking changes / migration notes: none.

Linked Issues

中文说明

这个 PR 做了什么

为 maintainer 通过 @qwen-code /review 评论触发的评审增加显式超时参数。现在可以用 @qwen-code /review --timeout=180@qwen-code /review timeout=180 重新触发大 PR 评审,默认 review 超时时间仍保持 120 分钟。

当评审超时时,fallback PR 评论会使用更清晰的加粗标题,并明确告诉 maintainer 如何用更长超时时间重新触发。非超时失败仍然只显示通用失败信息,不误导用户以为增加时间一定有帮助。

为什么需要

大 PR 可能超过默认评审预算。现有 fallback 只说明评审超时,maintainer 需要自己推断下一步怎么做。这个改动保持自动评审默认受限,同时给 maintainer 一个明确、可审计的方式,在确实值得重跑的大 PR 上投入更多 runner 时间。

Reviewer Test Plan

如何验证

在打开的 PR 上评论 @qwen-code /review --timeout=180,确认 review step 接受 timeout_minutes=180,也就是 Qwen 可以跑 180 分钟,同时 GitHub Actions job 上限是 200 分钟,保留 fallback 评论收尾时间。要验证 fallback 行为,可以强制 review step 进入 timeout 路径,并确认 PR 评论包含 @qwen-code /review --timeout=180

证据(Before & After)

Before:评审超时后只发布一条通用斜体 fallback 评论,没有重跑命令。After:评审超时后发布加粗 timeout 标题,并包含更长超时时间的重跑命令。

测试平台

OS Status
🍏 macOS ✅ tested
🪟 Windows N/A
🐧 Linux N/A

环境

npx vitest run scripts/tests/qwen-resolve-workflow.test.js --config scripts/tests/vitest.config.ts 通过。npx prettier --check scripts/tests/qwen-resolve-workflow.test.js 通过。git diff --check 通过。本地 actionlint .github/workflows/qwen-code-pr-review.yml 仍会报告当前 main 已存在的 environment.deployment: false key,因此没有把它作为本次变更的通过信号。

手动 timeout 验证

已在 PR head SHA 885309cdfa59c797e13d192775c7cbc5e3a35b9d 上 dispatch 一次 workflow_dispatch dry-run,传入 timeout_minutes=121https://github.com/QwenLM/qwen-code/actions/runs/28325897701。结果:run 已成功完成 Resolve PR context 并进入 review-pr / Run review,在真实 review step 中持续运行超过 8 分钟,而不是被旧的 120 分钟上限校验挡掉。这验证了手动 timeout override 路径可以接受高于默认 120 分钟的 maintainer 指定值;完整 dry-run conclusion 可在链接的 workflow run 中查看。

风险和范围

  • 主要风险或取舍:maintainer 显式请求 --timeout=180 时,review runner 会被占用更久。
  • 未验证 / 不在范围内:根据 PR 行数自动调整评审时间。
  • 破坏性变更 / 迁移说明:无。

关联 Issue

@yiliang114
yiliang114 force-pushed the codex/review-timeout-override branch from a59dc36 to 19d7bc1 Compare June 28, 2026 14:08
@yiliang114
yiliang114 force-pushed the codex/review-timeout-override branch from 19d7bc1 to 885309c Compare June 28, 2026 14:25
@yiliang114
yiliang114 marked this pull request as ready for review June 28, 2026 14:35
@qwen-code-ci-bot

Copy link
Copy Markdown
Collaborator

Thanks for the PR!

Template looks good ✓

On direction: solid, pragmatic CI improvement. Large PRs blowing past the review budget is a real problem maintainers hit, and giving them an explicit --timeout=180 escape hatch is exactly the kind of auditable knob that belongs here. Aligned with the project's CI maintenance needs.

On approach: scope is tight — two files, each change maps directly to the stated goal. The timeout parsing loop is straightforward, the 180-minute cap leaves headroom under the 200-minute job limit, and the differentiated fallback comments (timeout vs. generic failure) are a nice touch. One thing I want to verify in code review: the shift from QWEN_TIMEOUT=$((TIMEOUT_MINUTES - 5)) to QWEN_TIMEOUT="$TIMEOUT_MINUTES" (dropping the 5-minute buffer) — need to confirm the fallback step has enough slack to post its comment before the job-level timeout fires.

Moving on to code review. 🔍

中文说明

感谢贡献!

模板完整 ✓

方向:务实的 CI 改进。大 PR 超出评审预算是维护者实际遇到的问题,给出一个明确、可审计的 --timeout=180 选项正好符合这里的需求。

方案:范围紧凑——两个文件,每处改动都直接服务于目标。超时解析循环清晰,180 分钟上限在 200 分钟 job 限制内留有余量,区分超时和通用失败的 fallback 评论是不错的设计。代码审查需要确认一处:从 QWEN_TIMEOUT=$((TIMEOUT_MINUTES - 5)) 改为 QWEN_TIMEOUT="$TIMEOUT_MINUTES"(去掉了 5 分钟缓冲)——需要确认 fallback step 在 job 级超时触发前是否有足够时间发出评论。

进入代码审查 🔍

Qwen Code · qwen3.7-max

@qwen-code-ci-bot

Copy link
Copy Markdown
Collaborator

Code Review

Read the PR description first, then wrote my own proposal before looking at the diff:

My independent proposal: Parse --timeout=N / timeout=N tokens from the comment body, validate as numeric + within a safe range, override the default 120-minute timeout, and improve the fallback comment to suggest retry with longer timeout on timeout failures specifically. Increase the job-level timeout to leave headroom for the fallback step.

Comparison with the PR: the PR's approach matches this almost exactly. The token parsing is clean (case statement, no regex), the validation ladder (format → length → range) is solid, and the failure_kind mechanism for differentiating timeout from generic failure is a minimal, surgical addition.

Two things I looked at more closely:

  1. Dropping the 5-minute buffer (QWEN_TIMEOUT=$((TIMEOUT_MINUTES - 5))QWEN_TIMEOUT="$TIMEOUT_MINUTES"): this is correct. The job timeout is 200, max user timeout is 180, so there's 20 minutes of slack for the fallback step (which is just a gh pr comment — takes seconds). The old 5-minute subtraction was overly conservative for the new range.

  2. SIGKILL handling (exit code 137 in addition to 124): correct and well-documented — GNU timeout --kill-after escalates to SIGKILL, which reports 137 instead of 124.

No critical blockers. No AGENTS.md violations. No over-abstraction or scope creep — each change maps to the stated goal.

Pre-existing prettier issue on the workflow YAML (also present on main) is not introduced by this PR.

Testing

tmux real-scenario testing is N/A for a workflow YAML change — there's no user-facing behavior to drive. Ran the unit tests instead.

Unit tests (worktree with PR applied):

 ✓ scripts/tests/qwen-resolve-workflow.test.js (16 tests) 5ms

 Test Files  1 passed (1)
      Tests  16 passed (16)
   Start at  22:43:02
   Duration  306ms

All 16 tests pass, including the 2 new tests added by this PR:

  • allows maintainers to extend review timeout from /review comments — verifies token parsing, job timeout bump, and QWEN_TIMEOUT change
  • tells maintainers how to retry timed-out reviews with more time — verifies failure_kind, SIGKILL handling, and differentiated fallback comments

Prettier on changed test file: passes. Workflow YAML has a pre-existing formatting issue (same on main), not introduced here.

Qwen Code · qwen3.7-max

@qwen-code-ci-bot

Copy link
Copy Markdown
Collaborator

This is a clean, well-scoped CI improvement and I'm happy to approve.

The problem is real (large PRs blow past the review budget with no obvious retry path), the solution is minimal (two files, every edit maps to the goal), and the tests cover the new behavior without over-specifying implementation details. My independent proposal before reading the diff was essentially the same approach — the PR didn't miss a simpler path.

The one thing I watched closely was dropping the 5-minute buffer on QWEN_TIMEOUT. With the job cap at 200 and max user timeout at 180, the fallback step has 20 minutes of headroom — more than enough for a gh pr comment call. No concern there.

Shipping this. ✅

中文说明

这是一个干净、范围明确的 CI 改进,可以通过审查。

问题是真实的(大 PR 超出评审预算且没有明显的重试途径),方案是最小的(两个文件,每处改动都服务于目标),测试覆盖了新行为而没有过度指定实现细节。在读 diff 之前我的独立方案基本一致——PR 没有遗漏更简的路径。

唯一仔细看过的是去掉 QWEN_TIMEOUT 的 5 分钟缓冲。Job 上限 200 分钟,用户超时最大 180 分钟,fallback step 有 20 分钟余量——对一条 gh pr comment 调用绰绰有余。

放行 ✅

Qwen Code · qwen3.7-max

@qwen-code-ci-bot qwen-code-ci-bot left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, looks ready to ship. ✅

@yiliang114

Copy link
Copy Markdown
Collaborator Author

Manual timeout verification

Run: https://github.com/QwenLM/qwen-code/actions/runs/28325897701

Inputs: workflow_dispatch on PR head SHA 885309cdfa59c797e13d192775c7cbc5e3a35b9d, pr_number=5961, command=review, review_mode=dry-run, timeout_minutes=121.

Result: passed for the manual timeout override path. The run completed Resolve PR context successfully and reached review-pr / Run review; it stayed in the actual review step for more than 8 minutes instead of failing the previous 120-minute timeout validation. This confirms the PR workflow accepts a maintainer-provided timeout value above the default 120 minutes. The full dry-run conclusion remains available in the linked workflow run.

steps.context.outputs.pr_number != ''
env:
GH_TOKEN: '${{ secrets.CI_BOT_PAT }}'
FAILURE_KIND: "${{ steps.review.outputs.failure_kind || '' }}"

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Suggestion] The fallback step's if condition (line 630) uses failure(), but when the job-level timeout-minutes: 200 fires, GitHub Actions cancels the job — setting the conclusion to cancelled, not failure. This means failure() evaluates to false and no fallback comment is posted at all, leaving the PR with no bot feedback.

The 20-minute buffer (200 − 180) makes this unlikely in the common case, but it can happen if earlier steps (checkout, npm install, context resolution) consume enough time.

Consider adding || cancelled() to the condition:

Suggested change
FAILURE_KIND: "${{ steps.review.outputs.failure_kind || '' }}"
if: >-
failure() && cancelled() &&

Note: when using cancelled(), the step runs in a partially-torn-down environment, so you may want to guard gh pr comment with a short timeout or retry.

— qwen3.7-max via Qwen Code /review

@yiliang114
yiliang114 requested a review from wenshao June 28, 2026 15:22
@yiliang114
yiliang114 enabled auto-merge June 28, 2026 15:26
@yiliang114
yiliang114 added this pull request to the merge queue Jun 28, 2026

@doudouOUC doudouOUC left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Clean implementation with thorough validation. The timeout parsing is injection-resistant and the differentiated fallback comments are a nice UX improvement. No blocking issues found.

— qwen3.7-max via Qwen Code /review

Merged via the queue into QwenLM:main with commit f876f4a Jun 28, 2026
64 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants