fix(ci): run CI when pull requests retarget main - #4186
Conversation
b2dc6b8 to
ac52efb
Compare
|
CI has now failed three times in the WorkHub Desktop E2E area: The first two runs completed with 87 tests passing, 1 skipped, and 3 failing. The latest run completed with 88 passing, 1 skipped, and 2 failing. The two This PR only changes Could a maintainer confirm whether these are current baseline or flaky E2E failures? |
Astro-Han
left a comment
There was a problem hiding this comment.
Thanks for the focused fix and for tracing the retarget gap back to a concrete PR timeline.
I reviewed exact head ac52efb00af15e1e52b569e02982ae143f3203bc and found no P0–P3 issues.
The problem is demonstrated: a pull request whose head was updated while targeting another branch could be retargeted to main without producing the required CI / test check. The implementation extends the existing CI workflow to the edited event, keeps base retargets on the established test authority, and isolates title/body-only edits under a uniquely named skipped job so they neither cancel nor satisfy the required check. It introduces no second workflow, required-check authority, or additional permission.
Approved.
The exact-head test check is currently red only in two WorkHub reconstruction E2E cases after all earlier workflow steps passed. Those failures appear unrelated to this two-file CI policy change, but repository policy still requires a green exact-head test before merge.
Review analysis was assisted by Codex and an independent @reviewer agent. Astro-Han verified the exact head, GitHub Actions event and concurrency semantics, the focused regression contract, CI evidence, and severity judgment, and owns this review.
中文对照
谢谢你提交这个边界清晰的修复,并通过真实 PR 时间线定位 retarget 缺口。
我审查了精确 head ac52efb00af15e1e52b569e02982ae143f3203bc,没有发现 P0–P3 问题。
问题已经得到实际证明:PR 的 head 在目标还是其他分支时发生更新,随后把 base 改为 main,不会产生 required CI / test。当前实现继续使用既有 CI workflow 和 test authority,通过 edited 覆盖 base retarget;title/body 编辑则进入唯一命名的 skipped job,不会取消或冒充 required check。没有新增第二套 workflow、required-check authority 或额外权限。
Approve。
当前 exact-head test 仅在两条 WorkHub reconstruction E2E 上失败,之前的 workflow 步骤均已通过,看起来与这个两文件的 CI policy 修复无关。但按照仓库规则,合并前仍必须恢复绿色的 exact-head test。
本次审查分析由 Codex 和独立的 @reviewer 子代理协助;Astro-Han 核验了精确 head、GitHub Actions 事件与并发语义、聚焦回归契约、CI 证据和问题分级,并对本次 Review 负责。
Generated-by: Codex
ac52efb to
c22b646
Compare
|
@Astro-Han I rebased this PR onto the current The retarget behavior remains unchanged. The regression coverage now follows the current When convenient, could you please take a quick look at the updated exact head? Thanks! 中文对照: 我已将这个 PR rebase 到当前 retarget 行为没有改变;回归测试已迁移到当前的 方便时麻烦快速复核一下更新后的 exact head,谢谢! |
Astro-Han
left a comment
There was a problem hiding this comment.
Reviewed current head c22b6465e1eac905080bf81041a8fe7cc3d0dd83 (OPEN). Technical GO — no P0–P2, two P3s below. Human attention requested before merging: this touches branch-protection required checks; a mistake here freezes merges repo-wide.
What it fixes
CI / test is a required status check configured in .asf.yaml. The pull_request trigger used only the default event types (opened, synchronize, reopened) — not edited. But retargeting a PR's base branch to main fires exactly the edited event. Result: retargeted PRs never ran CI, the required check never reported, and those PRs could never merge.
The fix adds edited to the trigger types, with three expressions separating "base changed" from "only title/body edited".
Why the design holds
edited also fires for title/body edits that change no code. Left alone, those would (1) cancel the real check run in progress and (2) overwrite the existing test conclusion with an empty run. All three are plugged:
if:— non-base-change edits skip outright, consuming no runners.name:— skipped jobs are renamedignored-edit. Required checks match by name, so a renamed job cannot impersonateCI / testor cover that SHA's existing conclusion.concurrencygroup — the empty runs' group includesrun_id, unique every time, so they never cancel the real in-flight check. Easiest of the three to miss; the author caught it.
P3s and open items
(Two minor P3s — see review thread for details.) What I could not judge: none of this was verified against a real repository — no live base-retarget or title-edit was ever run, so final proof can only come from real events; recommend walking one test PR through a retarget right after merging. Whether the .asf.yaml required-check configuration is itself correct can only be confirmed from the ASF infrastructure side.
Automated review notice: This comment was posted by an automated review agent operated by Astro-Han. It is not an independent human review and does not replace one.
简体中文
本条结论全部来自 @Opus-Qronos-AstroHan 的审查。我自己没有读这份 diff;我核的是当前 head 有没有漂移。当前 head 是 c22b646,未关闭。修的是改指分支后 CI 不跑导致合不了的问题,设计三处都堵住了。动的是分支保护必需检查,合并前请人类看一眼。
Astro-Han
left a comment
There was a problem hiding this comment.
A follow-up to my review above on the same head c22b6465e1eac905080bf81041a8fe7cc3d0dd83 — two points for the human merging this, neither changes the technical GO:
- Verify with a real test PR right after merging. Correctness here cannot be fully proven by reading code — no retarget-to-main and no title-only edit was ever run against a real repository. Please walk one test PR through a base-retarget immediately after this lands.
- Note the reduced protection surface. This change necessarily lifts the old policy-test constraint "the CI
testjob must not carry anifcondition". That lift is required, but from now on nothing in tests will stop someone from adding furtherifs to that job. Worth knowing for future maintenance.
Automated review notice: This comment was posted by an automated review agent operated by Astro-Han. It is not an independent human review and does not replace one.
简体中文
本条是上一条的补充,同样来自 @Opus-Qronos-AstroHan 的审查结论。合并后请拿测试 PR 实走一遍改指流程;另外这次解除了测试 job 不许带条件的旧约束,以后加条件不会再被测试拦住,维护者知道就好。
Summary
main.Fixes #4181
Verification
node --test --test-concurrency=1 scripts/ci-test-plan.test.mjsnpx biome check scripts/ci-test-plan.test.mjsgit diff --checkChecklist
Does this PR entail a change in behavior?
main