ci: externalize PR review timeout settings to repository variables - #8460
Conversation
2a8c2cf to
8886245
Compare
|
Please do not rebase or force-push to an active PR as it invalidates existing review comments. Note for future reference, the bots always squash all changes into a single commit automatically as part of the integration. 中文请勿对活跃的 PR 执行 rebase 或 force-push,因为这会使已有的评审评论失效。另外,供日后参考:作为集成流程的一部分,机器人始终会自动将所有改动压缩(squash)为单个提交。 |
8886245 to
32f44f1
Compare
Read review timeout settings from two GitHub Actions repository
variables instead of hardcoding them, so tuning no longer requires
a code change:
QWEN_REVIEW_JOB_TIMEOUT_MINUTES (default: 360)
- review-pr job-level hard cap (was hardcoded 300)
QWEN_REVIEW_MAX_TIMEOUT_MINUTES (default: 300)
- per-review max timeout: validation ceiling, large PR auto-scale,
and fallback comment message (was hardcoded 240 in 3 places)
Constraint: QWEN_REVIEW_JOB_TIMEOUT_MINUTES must stay above
QWEN_REVIEW_MAX_TIMEOUT_MINUTES so retry + comment posting never
hit the job-level cap.
32f44f1 to
56cab25
Compare
|
Thanks for the PR! Template: substantively complete — under different headings than the template's ( Problem: exists, and it's operational. The review job cap and max timeout are actively tuned — repo variables Direction: aligned. This file already reads Size: not applicable — single workflow file, +19/-13, no core paths. Approach: the two variables chosen are the coupled top of the hierarchy (job cap and per-review max budget), so the scope is coherent. Two things I'll be looking at closely in code review: (1) neither expression carries a fallback — Risk: no elevated risk signals — the path is not in the revert-correlated high-risk set, and Moving on to code review. 🔍 中文说明感谢贡献! 模板:实质完整——标题与模板不同( 问题:真实存在,且是运维性质的。review job cap 与 max timeout 一直在被调优——repo 变量 方向:对齐。本文件已经通过 规模:不适用——单个 workflow 文件,+19/-13,未触及核心路径。 方案:所选两个变量是层级结构中耦合的顶部(job cap 与 per-review max budget),范围自洽。代码审查会重点看两点:(1)两处表达式都没有 fallback——变量未设置时 风险:无升级风险信号——该路径不在与 revert 相关的高风险集合中,且 进入代码审查 🔍 — Qwen Code · qwen3.8-max Reviewed at |
Code reviewMy independent proposal for this goal: externalize the job cap and the per-review max to repository variables, but keep an in-file fallback at every read site so the workflow degrades to today's behavior if a variable ever disappears — 1. No fallbacks — the pipeline becomes hostage to out-of-repo config.
Both variables exist today (=360, =300, created ahead of this PR), so it works as merged — but the failure mode is silent, surfaces only when someone later touches settings, and lands on the pipeline that reviews every PR in this repo. It also contradicts the description's own implementation note ( 2. Description/diff divergence. The description still documents the earlier 7-variable iteration: the defaults table (300/180/240/…), "safe fallbacks matching the current values", and a Reviewer Test Plan exercising Nothing else blocking: the 180 default, size threshold 300, resolve-pr's 120, the dispatch input, and the two 5-minute metadata jobs are untouched; the rewritten constraint comment captures the hierarchy correctly; Testing evidence (the PR's own CI, via API — this unattended run executes no PR code)
At review time 中文说明代码审查我对这个目标的独立方案与 PR 方向一致:把 job cap 与 per-review max 外部化到 repository variables,但每个读取点保留文件内 fallback( 1. 无 fallback——流水线变成 repo 外配置的人质。
2. 描述与 diff 不一致。 描述仍在写早先的 7 变量版本:默认值表(300/180/240/…)、"safe fallbacks matching the current values"、以及测试 无其他阻塞项:180 默认值、300 行规模阈值、resolve-pr 的 120、dispatch 输入、两个 5 分钟元数据 job 均未改动;重写后的约束注释正确表达了层级关系; 测试证据见上方 CI 表(finalize job 会在 CI 结束后更新):审查时 — Qwen Code · qwen3.8-max Reviewed at |
|
Confidence: 2/5 — right direction and scope, but the missing fallbacks make a settings change a single point of failure for the review pipeline, and the description still documents the previous iteration. Stepping back: I want to approve this. Externalizing the two coupled top-of-hierarchy values is the right move — the repo variables already exist at 360/300, nothing in the repo reads them yet, and the rescoped diff is small and focused. But it removes the one guarantee a hardcoded constant provides: always evaluating. As written, renaming or cleaning up either variable silently switches off the pipeline that reviews every PR in this repo — Verdict: request changes. 中文说明Confidence: 2/5——方向与范围都对,但缺失的 fallback 让一次设置变更就能成为 review 流水线的单点故障,且描述仍在写上一个版本。 退一步看:我是想批这个 PR 的。把层级顶部两个耦合的值外部化是正确方向——repo 变量已存在(360/300),repo 内尚无读取者,缩减后的 diff 小而聚焦。但它去掉了硬编码常量唯一的保证:永远能求值。按现状,改名或清理任一变量都会静默关掉审查本 repo 每个 PR 的流水线—— 结论:request changes。 — Qwen Code · qwen3.8-max Reviewed at |
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Requesting changes on the two findings above 🙏
- Add in-file fallbacks at the variable reads —
fromJSON(vars.QWEN_REVIEW_JOB_TIMEOUT_MINUTES || '360')andvars.QWEN_REVIEW_MAX_TIMEOUT_MINUTES || '300'in all three places — so a missing repo variable degrades to today's behavior instead of bricking the review pipeline (fromJSON('')fails every run at startup; an empty max timeout un-caps explicit--timeoutand zeroes the large-PR budget). - Sync the description to the rescoped diff — it still documents the 7-variable version with safe fallbacks, and the effective values move (job cap 300 → 360, max timeout 240 → 300).
@yiliang114 both are quick; happy to re-review right after.
Code Coverage Summary
CLI Package - Full Text ReportCore Package - Full Text ReportFor detailed HTML reports, please see the 'coverage-reports-22.x-ubuntu-latest' artifact from the main CI run. |
…les (QwenLM#8486) QwenLM#8460 moved the review timeouts into the QWEN_REVIEW_JOB_TIMEOUT_MINUTES and QWEN_REVIEW_MAX_TIMEOUT_MINUTES repository variables but left the workflow-text assertions in scripts/tests/qwen-resolve-workflow.test.js pinned to the old hardcoded 240/300 values, so the workspace test suite fails (Release Quality Checks and the PR Test job).
… review timeouts (#8431) The timeout externalization in #8460 replaced the hardcoded 300/240 values in qwen-code-pr-review.yml with the QWEN_REVIEW_JOB_TIMEOUT_MINUTES and QWEN_REVIEW_MAX_TIMEOUT_MINUTES repository variables but left scripts/tests/qwen-resolve-workflow.test.js asserting the old literals, so the full-profile Test job fails on any branch carrying that change. Update the three affected assertions to pin the externalized shape.
|
Released in v0.21.5. |
* fix(cli): preserve review startup version in footers * fix(cli): keep review startup version dynamic in bundle * fix(cli): reset review version after managed update * test(cli): use indexed env access * fix(cli): harden review footer strip and version stamping (QwenLM#8431) * fix(tests): sync qwen-resolve-workflow expectations with externalized review timeouts (QwenLM#8431) The timeout externalization in QwenLM#8460 replaced the hardcoded 300/240 values in qwen-code-pr-review.yml with the QWEN_REVIEW_JOB_TIMEOUT_MINUTES and QWEN_REVIEW_MAX_TIMEOUT_MINUTES repository variables but left scripts/tests/qwen-resolve-workflow.test.js asserting the old literals, so the full-profile Test job fails on any branch carrying that change. Update the three affected assertions to pin the externalized shape. * fix(cli): harden the review footer strip per review feedback (QwenLM#8431) The strip regex kept a 2^(N-1) partition ambiguity for same-line footer runs (measured 5.3 s at n=20) and missed footers truncated before their closing `_`; forged footers also survived on the body channel through `bodyCriticals`, and the values interpolated into the footer were not shape-validated. Guard the repeated group so an iteration cannot span another footer's start, make the final `_` optional, strip body Criticals per entry, refuse footer-forging model ids and non-version stamps, refuse non-object comment entries, pin the CLI-glue test suite against an ambient startup stamp, and cross-assert the LGTM filter regex against the footer builder. * fix(cli): strip review footers before ledger carryover * fix(cli): align ledger footer regression expectation --------- Co-authored-by: Shaojin Wen <shaojin.wensj@alibaba-inc.com> Co-authored-by: qwen-code-ci-bot <qwen-code-ci@service.alibaba.com> Co-authored-by: qwen-code-dev-bot <qwen-code-dev@service.alibaba.com>
…ed variables (QwenLM#8460 follow-up) (QwenLM#8485) * test(ci): align resolve-workflow timeout expectations with externalized variables QwenLM#8460 moved the review timeout numbers into repository variables (QWEN_REVIEW_JOB_TIMEOUT_MINUTES / QWEN_REVIEW_MAX_TIMEOUT_MINUTES) but left qwen-resolve-workflow.test.js expecting the old hardcoded 300/240 values — main's Test lane has been red since. The five expectations now pin the variable-driven forms the workflow actually carries. * test(ci): pin resolve-workflow timeout enforcement, tiering, and fallback machinery * test(ci): pin the timeout-advice branch CONDITION, not just both bodies With both branch bodies asserted as substrings of the same step, any comparison flip (-ge/-gt/-le) keeps both strings present and ships the wrong recovery advice on every timeout — a below-max run told it already used the maximum, an at-cap run told to retry with a timeout that cannot work. Raised in QwenLM#8388's review round (R4-21). * test(ci): anchor resolve-workflow arm slices at their if starts Address review feedback on QwenLM#8485: search for each sliced arm's else from the arm's own if start so an unrelated earlier if/else in the step cannot invert the slice (or vacuously satisfy .not.toContain on an empty slice). Also slice the small-PR tier arm instead of comparing first-occurrence indexes, unifying the technique and proving the assignments sit in opposite arms. * test(ci): keep the max-timeout text out of the below-max arm slice * test(ci): pin fallback arm bodies, comment wiring, and tiering order * review: line-anchor the at-max arm's closing fi A bare indexOf('fi') stops at the first word CONTAINING the letters — 'specified', 'notification' — so a harmless wording change inside the at-max arm would silently truncate the slice and hand the arm's not-toContain a vacuous pass. The end anchor is now the line-anchored /\n\s*fi\b/, with its own found-assertion. --------- Co-authored-by: Qwen Autofix <autofix@qwen-code.dev> Co-authored-by: qwen-code-dev-bot <qwen-code-dev@service.alibaba.com> Co-authored-by: qwen-code-dev-bot <qwen-code-dev-bot@users.noreply.github.com> Co-authored-by: verify <verify@local>
TLDR
The review workflow (
qwen-code-pr-review.yml) hardcodes timeout values in 7 places. Externalize them all to GitHub Actions repository variables (vars.*) with safe fallbacks matching the current values, so timeout tuning no longer requires a code change.What this changes
All hardcoded timeout values are now read from repository variables first, falling back to the current defaults:
QWEN_REVIEW_JOB_TIMEOUT_MINUTESreview-prjob-level hard capQWEN_REVIEW_DEFAULT_TIMEOUT_MINUTESQWEN_REVIEW_LARGE_PR_TIMEOUT_MINUTESQWEN_REVIEW_LARGE_PR_THRESHOLDQWEN_REVIEW_MAX_TIMEOUT_MINUTES--timeoutQWEN_RESOLVE_TIMEOUT_MINUTESresolve-prjob-level hard capBefore: adjusting any timeout required editing the workflow YAML and opening a PR.
After: maintainers set the variable in repo settings (Settings → Secrets and variables → Actions → Variables) and the next run picks it up — no code change, no PR.
Constraint
The timeout hierarchy must be preserved when tuning:
If the job cap drops below the max timeout, the retry budget plus comment posting can exceed the job-level cap and get killed mid-review.
Implementation note
Job-level
timeout-minutesuses(vars.X && fromJSON(vars.X)) || Nbecause actionlint treatsvars.*values as strings —fromJSONcasts to a number so the type check passes. Bash-embedded expressions use plainvars.X || Nsince bash arithmetic handles the cast natively.Reviewer Test Plan
actionlintpasses (runnode scripts/lint.js --actionlint).python3 -c "import yaml; yaml.safe_load(open(.github/workflows/qwen-code-pr-review.yml))".workflow_dispatch— behavior should be identical to before (all fallbacks match the old hardcoded values).QWEN_REVIEW_DEFAULT_TIMEOUT_MINUTES=120in repo variables, trigger another review — the default timeout should now be 120 minutes.Known Limitations
pull_request_target, so they can read repo variables normally — no limitation there.workflow_dispatchinputtimeout_minutesstill overrides the variable, preserving the existing manual override path.ack-review-requestandauthorizeare intentionally left hardcoded — they are fast metadata jobs, not tunable review budgets.