Repository navigation
Commit 5d5f701
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
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
| 1 | + | |
| 2 | + | |
| 3 | + | |
| 4 | + | |
| 5 | + | |
| 6 | + | |
| 7 | + | |
| 8 | + | |
| 9 | + | |
| 10 | + | |
| 11 | + | |
| 12 | + | |
| 13 | + | |
| 14 | + | |
| 15 | + | |
| 16 | + | |
| 17 | + | |
| 18 | + | |
| 19 | + | |
| 20 | + | |
| 21 | + | |
| 22 | + | |
| 23 | + | |
| 24 | + | |
| 25 | + | |
| 26 | + | |
| 27 | + | |
| 28 | + | |
| 29 | + | |
| 30 | + | |
| 31 | + | |
| 32 | + | |
| 33 | + | |
| 34 | + | |
| 35 | + | |
| 36 | + | |
| 37 | + | |
| 38 | + | |
| 39 | + | |
| 40 | + | |
| 41 | + | |
| 42 | + | |
| 43 | + | |
| 44 | + | |
| 45 | + | |
| 46 | + | |
| 47 | + | |
| 48 | + | |
| 49 | + | |
| 50 | + | |
| 51 | + | |
| 52 | + | |
| 53 | + | |
| 54 | + | |
| 55 | + | |
| 56 | + | |
| 57 | + | |
| 58 | + | |
| 59 | + | |
| 60 | + | |
| 61 | + | |
| 62 | + | |
| 63 | + | |
| 64 | + | |
| 65 | + | |
| 66 | + | |
| 67 | + | |
| 68 | + | |
| 69 | + | |
| 70 | + | |
| 71 | + | |
| 72 | + | |
| 73 | + | |
| 74 | + | |
| 75 | + | |
| 76 | + | |
| 77 | + | |
| 78 | + | |
| 79 | + | |
| 80 | + | |
| 81 | + | |
| 82 | + | |
| 83 | + | |
| 84 | + | |
| 85 | + | |
| 86 | + | |
| 87 | + | |
| 88 | + | |
| 89 | + | |
| 90 | + | |
| 91 | + | |
| 92 | + | |
| 93 | + | |
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
19 | 19 | | |
20 | 20 | | |
21 | 21 | | |
22 | | - | |
| 22 | + | |
| 23 | + | |
23 | 24 | | |
24 | 25 | | |
25 | 26 | | |
| |||
28 | 29 | | |
29 | 30 | | |
30 | 31 | | |
31 | | - | |
32 | | - | |
| 32 | + | |
| 33 | + | |
33 | 34 | | |
34 | 35 | | |
35 | 36 | | |
36 | 37 | | |
37 | 38 | | |
38 | 39 | | |
39 | | - | |
40 | | - | |
| 40 | + | |
41 | 41 | | |
42 | | - | |
| 42 | + | |
43 | 43 | | |
44 | 44 | | |
45 | 45 | | |
46 | 46 | | |
47 | | - | |
48 | | - | |
49 | | - | |
50 | 47 | | |
51 | 48 | | |
52 | 49 | | |
53 | 50 | | |
54 | 51 | | |
55 | 52 | | |
56 | 53 | | |
57 | | - | |
| 54 | + | |
| 55 | + | |
58 | 56 | | |
59 | 57 | | |
60 | 58 | | |
| |||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
11 | 11 | | |
12 | 12 | | |
13 | 13 | | |
| 14 | + | |
| 15 | + | |
| 16 | + | |
14 | 17 | | |
15 | 18 | | |
16 | 19 | | |
17 | 20 | | |
18 | 21 | | |
19 | | - | |
20 | | - | |
| 22 | + | |
| 23 | + | |
| 24 | + | |
21 | 25 | | |
22 | 26 | | |
23 | 27 | | |
24 | 28 | | |
25 | 29 | | |
26 | 30 | | |
27 | | - | |
28 | | - | |
| 31 | + | |
29 | 32 | | |
30 | | - | |
| 33 | + | |
31 | 34 | | |
32 | 35 | | |
33 | 36 | | |
34 | 37 | | |
35 | | - | |
36 | | - | |
37 | | - | |
38 | 38 | | |
39 | 39 | | |
40 | 40 | | |
41 | 41 | | |
42 | 42 | | |
43 | 43 | | |
44 | 44 | | |
45 | | - | |
| 45 | + | |
| 46 | + | |
46 | 47 | | |
47 | 48 | | |
48 | 49 | | |
| |||
0 commit comments