Skip to content

Commit 5d5f701

Browse files
zaerlclaude
andauthored
Keep the required checks green when a pull request's description is edited (#676)
## Why Since #660, editing a pull request's title or description turns its six required checks (`unit`, `Journeys` and `Packaged smoke` on macOS and Windows) into "Expected — Waiting for status to be reported", even though they already passed on that commit. The merge box shows them as pending until someone pushes or re-runs. Seen on #675: the runs at 07:59 passed, and the edits at 08:18 and 08:45 started runs 37904200109/37904200065 and 37906918098/37906918097 with every job skipped. ## What changes **Root cause:** #660 made `unit-tests.yml` and `e2e.yml` listen for `edited`, so that a base move would re-run their `changes` job. For any other edit, `changes` is skipped and every job after it is skipped too. GitHub treats the newest run of a workflow on a commit as that commit's checks. A skipped matrix job is reported with its name unexpanded (`unit (${{ matrix.os }})`), so the required `unit (macos-latest)` and the rest disappear from the newest run. **Fix:** both workflows stop listening for `edited`, so a title or description edit starts nothing in them and the earlier results stand. A new `.github/workflows/base-change.yml` handles a base move instead. It finds the latest Unit tests and E2E runs for the PR's head and re-runs them with `gh run rerun`; a run that is still in progress is cancelled first and re-run once it stops. A re-run is a new attempt of the same run rather than a newer run, so it replaces those checks directly. The `changes` job reads the PR's file list from the API at run time, so the re-run sees the new base: a Markdown-only stacked PR moved onto trunk is evaluated as code again, which is the guarantee #660 added. Deliberately not in it: re-running anything for a fork PR. A fork's token is read-only, so the workflow posts a warning instead (see Risks). ## How to test this Platforms: none. This is CI only, and it runs on GitHub's Linux runners. The workflows only take effect once this is on trunk: `edited` runs use the workflow files from the PR's merge commit, so a PR opened after this merges, or one that is pushed to after it merges, picks them up. **Starting state:** this branch merged to trunk. You need a pull request against trunk whose head was pushed after the merge, with all six required checks passed. 1. Edit that PR's description and save. Then edit its title. Expected: no new run of **Unit tests** or **E2E** appears in the Actions tab. **Re-run on base change** shows a run in which the `rerun` and `fork` jobs are both skipped. In the merge box, all six required checks are still ✓, not "Expected — Waiting for status to be reported". 2. Make a stacked pair: PR A (any code change) against trunk, and PR B with one Markdown-only commit on top of A's branch, opened against A's branch. Wait for B's checks. Expected: B passes the six checks through the stub jobs ("Only Markdown changed"). 3. On PR B, choose **Edit** next to the title and change the base from A's branch to `trunk`. Expected: a **Re-run on base change** run whose `rerun` job prints `re-running run <id>` for `unit-tests.yml` and `e2e.yml`. Unit tests and E2E each show a second attempt of their earlier run. In it, `changes` reports `code=true` (B now carries A's code), and the real macOS and Windows jobs run in place of the stubs. 4. Optional: repeat step 3 while B's Unit tests run is still in progress (push a Markdown commit to B, then retarget it straight away). Expected: the in-progress run is cancelled and then re-run, and the job log shows both. **What must not have happened:** - The six required checks going to "Expected" after step 1. This is the bug: on trunk today, step 1 reproduces it. - The stub passes surviving step 3. That would mean B could merge A's code without the suites having run on it. - A push made right after a retarget having its new run cancelled. The job compares the PR's live head with the event's head immediately before each re-run, and stops if they differ. **Cannot be tested here:** none of this runs before merge (see Platforms), and there is no unit test for workflow files. In particular, it is unverified that `GITHUB_TOKEN` with `actions: write` may cancel and re-run a `pull_request` run in this repository. Steps 3 and 4 are the check for that, and the first stacked retarget after merge should be watched. The workflows pass `actionlint` (with shellcheck), and the `gh run list` lookup was run against #675's real runs. ## Risks and limitations - **A re-run tests the old merge commit.** A re-run keeps the original event, so the suites check out the head merged into the *old* base. Only the Markdown-only decision uses the new base. That is what #660's guarantee needs, and it matches every PR run once its base branch has moved on. #660's `edited` runs did check out a fresh merge with the new base. - **Fork PRs:** a fork PR whose base moves is not re-run. A `::warning::` asks for a manual re-run or a push. `pull_request_target` would have a write token, but re-running a fork's run with it might bypass the approval that a first-time contributor's run waits for, so I did not use it. It is rare: a fork PR based on an upstream branch other than trunk. - **Runs older than 30 days:** GitHub refuses to re-run them. The job fails with an error that says to push, or to close and reopen the PR. That check is not required, so it does not block the merge on its own. - **PRs already open when this merges:** where the newest run on the head is one of the old skipped `edited` runs (as on #675 now), re-running it replays that skip. A push, or re-running the earlier passing run from the Actions tab, clears it once. - Review: 6 [fix here] · 1 [follow-up]. Five fixed; the remaining [fix here] (no live run yet) is covered by How to test steps 3–4. The follow-up is the open-PR transition above, noted rather than coded. ## Related Follow-up to #660. Seen on #675. --- <details> <summary>Design decisions and alternatives considered</summary> - **Keep `edited`, but give the matrix jobs expanded names when skipped:** not possible. GitHub does not expand a matrix for a job that is skipped at the job level, which is why #660 added stub jobs in the first place. - **Keep `edited` and run the stubs/real jobs again on every edit:** every title edit would cost two Linux jobs at best and the full macOS/Windows suites at worst, and it would cancel or duplicate runs. - **`workflow_dispatch` from the edit workflow:** this would start a fresh run on the head with the new base. But it is a different event, so the `pull_request`-only logic in `changes` would need a second branch, and the checks would come from a run nobody pushed. A re-run reuses everything that already exists. - **Concurrency:** base moves share the group `base-change-<PR>`, so a second move cancels the first and re-runs whatever the first left behind. Any other edit gets a group per run (`-<run_id>`), so it cannot cancel a base move. The concurrency group is evaluated before the job's `if:`, so without this a description edit made during a base move would have cancelled it. </details> <details> <summary>Review outcome</summary> 6 [fix here] · 1 [follow-up]: 5 fixed, 1 covered by How to test, 1 follow-up recorded in Risks. `npm run lint` clean; unit suite 2096 pass, 0 fail, 2 skipped (local, macOS). `actionlint` clean on all workflows. - **Review:** completed. Reviewer: a separate agent context, read-only. Head `da2dbfa`, base `7ee3ead`. Findings: - 🟡 A description edit could cancel a base-move run, because the concurrency group is evaluated before `if:`. Fixed. - 🟡 Re-running the old head's run after a push would cancel the new head's run through the shared concurrency group. Fixed. - 🟡 The 30-day re-run limit failed silently and aborted before the second workflow. Fixed. - 🔵 The fork regression was unannounced. Fixed with a warning job. - 🔵 [follow-up] PRs open across the merge. Recorded in Risks. - 🔵 The GitHub semantics are unverified in practice. Covered by How to test steps 3–4. - **Since review:** `da2dbfa → 667bd95` re-reviewed by the same separate context. #1–#4 resolved. One new 🔵: a push during the cancel-and-wait loop could still be cancelled. Fixed in `667bd95 → 8b5ea25`, which moves the head check to just before each re-run. I checked that last change myself; it was not reviewed again. - **CodeRabbit:** not requested yet. </details> <details> <summary>Implementation notes</summary> - The run lookup is `gh run list --workflow <file> --event pull_request --commit <head sha> --branch <head ref> --limit 1`. `--commit` matches the run's head SHA, not the merge commit's, and `--event` leaves out trunk push runs. - The job needs `actions: write` (cancel, re-run) and `pull-requests: read` (the head check). The workflow's default permissions are `{}`. Every value from the event reaches the script through `env:`, never interpolated into it. - `if ! gh run rerun` keeps a failure for one workflow from skipping the other under `bash -e`. The job then exits non-zero. - The `changes` jobs lose their `if:`, and the concurrency groups lose their `-edit` suffix, since neither workflow sees `edited` any more. </details> Screenshots: nothing on screen changed. 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
1 parent c4b1bfa commit 5d5f701

3 files changed

Lines changed: 111 additions & 19 deletions

File tree

‎.github/workflows/base-change.yml‎

Lines changed: 93 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,93 @@
1+
# Re-runs the unit and E2E suites when a pull request's base moves.
2+
#
3+
# Moving the base changes a PR's diff without a new commit: a Markdown-only PR stacked on a
4+
# code PR and moved onto trunk now carries that PR's code, so its stub passes from
5+
# unit-tests.yml and e2e.yml no longer hold. Those workflows do not listen for `edited`
6+
# themselves. GitHub reports the newest run of a workflow on a commit as that commit's checks,
7+
# and a run started by a title or description edit, with nothing to do, skipped every job and
8+
# left the six required checks "Expected" in place of the passes before it (seen on #675).
9+
#
10+
# A re-run is an attempt of the same run, not a newer one, so a title or description edit now
11+
# starts nothing in those workflows and a base move replaces their results with fresh ones.
12+
# Their `changes` job reads the PR's file list from the API at run time, so the re-run sees
13+
# the new base. Everything else in a re-run comes from the original event, including the
14+
# merge commit the suites check out: the code is tested merged into the old base, which is
15+
# what every PR run already does once its base branch moves on.
16+
#
17+
# Never checks out or runs the PR's code.
18+
19+
name: Re-run on base change
20+
21+
on:
22+
pull_request:
23+
types: [edited]
24+
25+
permissions: {}
26+
27+
# A second base move cancels this one; the run it starts re-runs whatever this one left
28+
# behind. Any other edit gets a group of its own, so it cannot cancel a base move.
29+
concurrency:
30+
group: base-change-${{ github.event.pull_request.number }}${{ !github.event.changes.base && format('-{0}', github.run_id) || '' }}
31+
cancel-in-progress: true
32+
33+
jobs:
34+
rerun:
35+
if: github.event.changes.base && github.event.pull_request.head.repo.full_name == github.repository
36+
runs-on: ubuntu-latest
37+
timeout-minutes: 15
38+
permissions:
39+
actions: write
40+
pull-requests: read
41+
steps:
42+
# The latest run for the head, not the latest successful one: that is the run GitHub
43+
# reports. One still running may have read the old file list already, so it is
44+
# cancelled and re-run rather than left to finish. No run means none is left to re-run,
45+
# which on a live PR means one is about to start and will read the new base itself.
46+
- env:
47+
GH_TOKEN: ${{ github.token }}
48+
GH_REPO: ${{ github.repository }}
49+
PR: ${{ github.event.pull_request.number }}
50+
SHA: ${{ github.event.pull_request.head.sha }}
51+
BRANCH: ${{ github.event.pull_request.head.ref }}
52+
run: |
53+
failed=0
54+
for workflow in unit-tests.yml e2e.yml; do
55+
run=$(gh run list --workflow "$workflow" --event pull_request \
56+
--commit "$SHA" --branch "$BRANCH" --limit 1 \
57+
--json databaseId,status --jq '.[] | "\(.databaseId) \(.status)"')
58+
if [ -z "$run" ]; then
59+
echo "$workflow: no run for $SHA."
60+
continue
61+
fi
62+
read -r id status <<< "$run"
63+
if [ "$status" != completed ]; then
64+
# It may finish between the list and the cancel; the wait covers both.
65+
gh run cancel "$id" || true
66+
until [ "$(gh run view "$id" --json status --jq .status)" = completed ]; do
67+
sleep 10
68+
done
69+
fi
70+
# A push since the base moved has a run of its own, which reads the new base. Re-running
71+
# the old head's run would join that run's concurrency group and cancel it. Checked
72+
# here, after the wait, because a push can land during it.
73+
head=$(gh api "repos/$GH_REPO/pulls/$PR" --jq .head.sha)
74+
if [ "$head" != "$SHA" ]; then
75+
echo "The head moved to $head since the base changed; its own runs cover the new base."
76+
exit "$failed"
77+
fi
78+
echo "$workflow: re-running run $id."
79+
# GitHub refuses to re-run a run older than 30 days. Nothing required fails here, so
80+
# say plainly what clears it.
81+
if ! gh run rerun "$id"; then
82+
echo "::error::Could not re-run $workflow run $id, so its checks still describe the old base. Push a commit, or close and reopen the pull request, to start a fresh run."
83+
failed=1
84+
fi
85+
done
86+
exit "$failed"
87+
88+
# A fork's token is read-only, so it cannot re-run anything.
89+
fork:
90+
if: github.event.changes.base && github.event.pull_request.head.repo.full_name != github.repository
91+
runs-on: ubuntu-latest
92+
steps:
93+
- run: echo "::warning::This pull request comes from a fork, so its unit and E2E checks were not re-run for the new base. A maintainer can re-run them from the Actions tab, or the author can push a commit."

‎.github/workflows/e2e.yml‎

Lines changed: 8 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -19,7 +19,8 @@
1919
#
2020
# A pull request that changes only Markdown files skips both jobs. Their checks are
2121
# required, so the `*-skipped` jobs report the same names as passed on a cheap Linux
22-
# runner; unit-tests.yml explains why it is done this way and not with `paths-ignore`.
22+
# runner; unit-tests.yml explains why it is done this way and not with `paths-ignore`,
23+
# and how base-change.yml re-runs both workflows when a PR's base moves.
2324

2425
name: E2E
2526

@@ -28,33 +29,30 @@ on:
2829
# `ready_for_review` is not in the default set. Without it, a PR opened as a draft
2930
# and later marked ready never runs this workflow at all — the draft guard below
3031
# would skip the only run it ever gets.
31-
# `edited` re-runs `changes` when the PR's base moves (see the `changes` job).
32-
types: [opened, synchronize, reopened, ready_for_review, edited]
32+
# Not `edited`: see unit-tests.yml.
33+
types: [opened, synchronize, reopened, ready_for_review]
3334
push:
3435
branches: [trunk]
3536

3637
permissions:
3738
contents: read
3839

39-
# A new push supersedes the previous run on the same ref. A title or description edit
40-
# gets a group of its own, so it cannot cancel a run that is still testing the code.
40+
# A new push supersedes the previous run on the same ref.
4141
concurrency:
42-
group: e2e-${{ github.ref }}${{ github.event.action == 'edited' && !github.event.changes.base && '-edit' || '' }}
42+
group: e2e-${{ github.ref }}
4343
cancel-in-progress: true
4444

4545
jobs:
4646
changes:
47-
# Moving the PR's base changes its diff: a Markdown-only PR moved onto trunk from a
48-
# stacked branch now carries that branch's code. Any other edit changes nothing here.
49-
if: github.event.action != 'edited' || github.event.changes.base
5047
runs-on: ubuntu-latest
5148
permissions:
5249
pull-requests: read
5350
outputs:
5451
code: ${{ steps.files.outputs.code }}
5552
steps:
5653
# A push to trunk always runs both jobs. On a pull request, any file that is
57-
# not Markdown (including the old name of a renamed file) counts as code.
54+
# not Markdown (including the old name of a renamed file) counts as code. The
55+
# list is read live, so a re-run after the base moves sees the new diff.
5856
- id: files
5957
env:
6058
GH_TOKEN: ${{ github.token }}

‎.github/workflows/unit-tests.yml‎

Lines changed: 10 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -11,38 +11,39 @@
1111
# workflow skipped by `paths-ignore` never reports them, so the PR could not merge. Instead the
1212
# `changes` job decides, and `unit-skipped` reports the same check names as passed on a cheap
1313
# Linux runner. e2e.yml does the same.
14+
#
15+
# Moving a PR's base changes its file list without a new commit, so base-change.yml re-runs the
16+
# latest run of this workflow for the PR's head when that happens.
1417

1518
name: Unit tests
1619

1720
on:
1821
pull_request:
19-
# `edited` re-runs `changes` when the PR's base moves (see the `changes` job).
20-
types: [opened, synchronize, reopened, edited]
22+
# Not `edited`: GitHub reports the newest run on a commit as its checks, and an edit run
23+
# that skips its jobs replaces the passes before it. base-change.yml handles a base move.
24+
types: [opened, synchronize, reopened]
2125
push:
2226
branches: [trunk]
2327

2428
permissions:
2529
contents: read
2630

27-
# A new push supersedes the previous run on the same ref. A title or description edit
28-
# gets a group of its own, so it cannot cancel a run that is still testing the code.
31+
# A new push supersedes the previous run on the same ref.
2932
concurrency:
30-
group: unit-tests-${{ github.ref }}${{ github.event.action == 'edited' && !github.event.changes.base && '-edit' || '' }}
33+
group: unit-tests-${{ github.ref }}
3134
cancel-in-progress: true
3235

3336
jobs:
3437
changes:
35-
# Moving the PR's base changes its diff: a Markdown-only PR moved onto trunk from a
36-
# stacked branch now carries that branch's code. Any other edit changes nothing here.
37-
if: github.event.action != 'edited' || github.event.changes.base
3838
runs-on: ubuntu-latest
3939
permissions:
4040
pull-requests: read
4141
outputs:
4242
code: ${{ steps.files.outputs.code }}
4343
steps:
4444
# A push to trunk always runs the suite. On a pull request, any file that is not Markdown
45-
# (including the old name of a renamed file) counts as code.
45+
# (including the old name of a renamed file) counts as code. The list is read live, not
46+
# from the event, so a re-run after the base moves sees the new diff.
4647
- id: files
4748
env:
4849
GH_TOKEN: ${{ github.token }}

0 commit comments

Comments
 (0)