#33145 CI: silent merge check
| Reviewability | Reviews | Agreement | Size | |||||||
|---|---|---|---|---|---|---|---|---|---|---|
| #33145 CI: silent merge check | Waiting on author | 0 | Blocked | 412 | ||||||
|
|
|
|
| ||||||
Reviewability: ReadyStalePaused Agreement: StrongPositivePositive w/ caveatsNeutralMildDisputedBlockedCrickets Reviews: current code-review ACKs, then (+stale ACKs) and -NACKs; greener = more ACKs. Size: lines added or modified outside tests, then in tests; greener = smaller. | ||||||||||
Goal
Adds a periodic GitHub Actions workflow and helper scripts to check open pull requests for silent merge conflicts against master. The scheduled job sequentially merges candidate PR heads onto master, builds and runs tests, and posts the status via the GitHub Checks API.
Problem: PRs can conflict logically with newly merged code on master without triggering git merge conflicts, requiring manual testing and review to detect broken PRs before or after merge.
Reviewability
Paused · Waiting on author
Open objections from Sammie05, fjahr with no author reply for 11 days; review would repeat what has been asked.
Waiting on (from the thread read):
- author: address fjahr's review comments regarding the draft parameter and head_sha recording 2026-10-21
- author: address fjahr's objections regarding the ignored draft filter and head_sha race condition 2026-10-21
Agreement
Blocked
objection that needs a change, nobody has replied (fjahr)
Thread notes: Re-classified maflcko's lint request as a suggestion and added missing suggestions from 0xB10C and willcl-ark. Added fjahr's Approach ACK and their two open objections concerning draft PR filtering and potential race conditions on head SHA. / Added fjahr's Approach ACK review along with two open correctness objections regarding the draft parameter filter and head_sha race condition. Upgraded the older record claims to schema 2, splitting multi-harm objections and capturing suggestions.
Objections, suggestions, and questions:
| Reviewer | Type | Harm, request, or question | Status | Must be | Author replied | Quote |
|---|---|---|---|---|---|---|
| Sammie05 (none) | question, correctness | whether multiple labels applied simultaneously would cause the workflow condition to misbehave | open | answered | no | 2025-08-06: 'do we need to consider cases where multiple labels are applied at once? Would the condition still behave as expected?' |
| maflcko (member) | suggestion, interface | allow passing a vector of task names for flexibility | answered · author asked for elaboration and the label-based approach was later replaced | answered | yes | 2025-08-06: 'It would be good to allow the same somehow?' Status from: 2025-08-07: m3dwards: 'Can you elaborate a little more on what this would look like?' |
| maflcko (member) | suggestion, correctness | verify that the workflow does not re-run an ancient commit rather than an updated merge commit | answered · author verified with a test on an updated master branch | answered | yes | 2025-08-07: 'Other stuff to check would be that this works at all and doesn't just re-run an ancient commit' Status from: 2025-08-07: m3dwards: 'Just ran a test to specifically test for this by updating the default (master) branch and adding and removing the label and it correctly picked an updated merge commit' |
| maflcko (member) | objection, approach | adding 200+ lines of workflow code creates maintenance burden, spams PRs with label events, and misdirects failure notifications | fixed · fix pushed · fixed when author redesigned the PR to use a standalone looping check script | changed | yes | 2025-08-07: 'We are now trying to add 200+ lines of code to this repo which have to be maintained, but they cause label spam, and likely aren't sufficient to achieve the goal, because the ci-failed notification wi' Status from: 2026-05-06: maflcko: 'lgtm, thx!' |
| maflcko (member) | objection, approach | triggering checks via label events causes label spam on pull requests | fixed · fix pushed · redesigned away from label-based triggering to a periodic scheduled runner | changed | yes | 2025-08-07: 'they cause label spam' Status from: 2026-05-06: maflcko: 'lgtm, thx!' |
| maflcko (member) | objection, correctness | CI failure notifications are sent to whoever added the trigger label instead of the PR author | fixed · fix pushed · redesigned to write check runs directly to the PRs via the checks API | changed | yes | 2025-08-07: 'the ci-failed notification will go to the one adding the label and triggering the workflow' Status from: 2026-05-06: maflcko: 'lgtm, thx!' |
| 0xB10C (member) | suggestion, approach | consolidate the conditional check into .github/workflows/ci.yml instead of maintaining separate workflow files | answered · author replied that the check seemed verbose across jobs but would consider it | answered | yes | 2025-08-08: 'Have you considered doing something like the following in `.github/workflows/ci.yml` to avoid having to maintain separate files?' Status from: 2025-08-08: m3dwards: 'Could be a good shout, seems a bit of a verbose check if that has to be included in each job. I'll have a think about it.' |
| maflcko (member) | suggestion, approach | schedule the cron run at an odd minute to avoid peak runner demand at minute 0 | answered · author replied with a lighthearted link accepting the idea | answered | yes | 2026-05-06: 'Actually, an odd minute may be better to avoid the peak time around minute 0' Status from: 2026-05-20: m3dwards: 'https://www.upworthy.com/pick-a-random-number-between-100-you-probably-chose-37-and-there-s-a-big-reason-for-that/' |
| willcl-ark (member) | suggestion, approach | run a fixed build command directly on a bare runner host to avoid Docker overhead | answered · author preferred running functional tests via test_run_all.sh; maflcko noted either way is fine | answered | yes | 2026-05-21: 'why can this job not just do something like: check out bitcoin/bitcoin:master fetch PR head attempt the merge locally run a fixed build command' Status from: 2026-05-21: m3dwards: 'I'm open to changing it to this but this is more code than it currently is which is simply calling `./ci/test_run_all.sh`.' |
| willcl-ark (member) | objection, safety | running the full job with check write permissions allows untrusted PR code executed in CI to access authenticated GitHub write tokens | fixed · fix pushed · author restricted write token usage and pushed an updated workflow | changed | yes | 2026-05-21: 'I think running the entire job with this permission would let a PR do some annoying things on GH as they have an authenticated write token to play with in run_all.sh?' Status from: 2026-05-28: m3dwards: 'Pushed latest changes to show direction. Running a bunch of tests on my fork.' |
| willcl-ark (member) | suggestion, correctness | set timedelta to 5 hours 30 minutes or read from an environment variable | fixed · fix pushed · author acknowledged the copy-paste error and fixed it in the subsequent push | answered | yes | 2026-05-21: 'surely we'd want here timedelta(hours=5, minutes=30) or read from an env var' Status from: 2026-05-28: m3dwards: 'Pushed latest changes to show direction. Running a bunch of tests on my fork.' |
| willcl-ark (member) | objection, correctness | the mergeable property on the GitHub list pulls endpoint returns null, breaking PR mergeability filtering | fixed · fix pushed · author updated logic to fetch mergeability per PR individually | changed | yes | 2026-05-21: 'Even though these have a mergable field, I think it's bunk :( ... perhaps we need to iterate each pr individually' Status from: 2026-05-28: m3dwards: 'Pushed latest changes to show direction. Running a bunch of tests on my fork.' |
| maflcko (member) | suggestion, scope | include the lint task in the silent merge check | accepted · accepted by maflcko after author showed lint check dramatically slowed down execution | answered | yes | 2026-06-18: 'realized I was also running `--task 'lint'`. lint should be fast, so it could also be run here?' Status from: 2026-06-19: maflcko: 'Ok, fair to leave it for later.' |
| maflcko (member) | objection, maintenance | failures are tedious to find in long log outputs lacking line timestamps and separation | fixed · fix pushed · author added timestamps and collapsed failure logs into their own sections | changed | yes | 2026-06-19: 'it could be a bit tedious to find the failure, as it is hidden in the middle of a long log? Also, there are no timestamps on any of the lines at all?' Status from: 2026-07-01: m3dwards: 'Added timestamps to the log messages if they do not already have them... The logs are now collapsed into their own secti' |
| fjahr (member) | objection, correctness | the GitHub pulls endpoint lacks a draft query parameter, causing the draft PR filter to be ignored | open | changed | no | 2026-10-21: 'There is no `draft` parameter on this endpoint so this filter is ignored: https://docs.github.com/en/rest/pulls/pulls?apiVersion=2026-03-10 Would probably need a manual filter function.' |
| fjahr (member) | objection, correctness | fetching head again during the test run can cause the older recorded head_sha to be reported for newer commits if a force push occurs | open | changed | no | 2026-10-21: 'If there was a force push in between, then the old `head_sha` is reported but the result is actually for the newer head. I think the older `head_sha` doesn't actually need to be recorded.' |
Support:
- maflcko (member) (Concept ACK, 2025-09-04): concept ack, but I don't have the slightest idea if this is even possible (or how) with GHA [not substantive]
- fjahr (member) (Concept ACK, 2025-11-21): (no reason given) [not substantive]
- maflcko (member) (ACK, 2026-05-06): lgtm, thx! [not substantive]
- maflcko (member) (ACK, 2026-06-19) [read: .github]: lgtm. Seems fine to put the code in this repo and call it inside this CI. An alternative would be to have the code under DrahtBot and run it there.
- fjahr (member) (Approach ACK, 2026-10-21) [read: .github/silent-merge-test.py]: this looks like the right approach to me
Participants:
- Sammie05 (none, question, 1 statements 2025-08-06 to 2025-08-06): asked whether multiple labels applied at once would affect the initial label-based trigger
- maflcko (member, support, 25 statements 2025-08-06 to 2026-06-19): approved the redesign, suggested improvements on timing and log formatting, and agreed to defer lint checks
- 0xB10C (member, neutral, 1 statements 2025-08-08 to 2025-08-08): suggested consolidating conditional logic into ci.yml
- fjahr (member, objection, 4 statements 2025-11-21 to 2026-10-21): Approach ACK, but identified bugs with the draft PR filter and head_sha recording
- fanquake (member, question, 1 statements 2026-02-13 to 2026-02-13): asked for a status update on whether the issue was still being addressed
- willcl-ark (member, objection, 4 statements 2026-05-21 to 2026-05-21): raised security concern about checks write token permissions in PR runs and noted GitHub API mergeable attribute bug
- sedited (member, neutral, 1 statements 2026-08-27 to 2026-08-27): pinged reviewers for attention
Files
0 lines under test/bench/ci.
- .github/silent-merge-test.py +256/-0
- .github/workflows/silent-merge-check.yml +83/-0
- .github/silent-merge-report.py +73/-0
Card
The short text the ranking pass compares across PRs.
Adds a periodic GitHub Actions workflow and python scripts that loop over open PRs, merge them locally against current master, build and run tests, and post results via GitHub Checks. It solves the problem of silent logical merge conflicts that pass git merge checks but break builds or tests, reducing maintainer effort currently spent manually investigating PRs. Maintainers support the initiative and past permission and logging concerns were addressed, but the PR has been dormant without author activity for over two months.
Data
thread read through 2026-10-21 (59 statements), head ef35e6c84c
labels: Tests
Categories
Build and CI
P3 · new feature (ranking pass: Moved down from 22: maintainer-workflow convenience, no build or release correctness impact)
Adds a periodic CI workflow that automatically tests candidate PRs merged against master to catch logical conflicts. While helpful for maintainer workflow and avoiding broken master merges, it is a non-critical CI enhancement.
Membership: All changed files are in .github/ creating a new GitHub Actions workflow and CI helper scripts (.github/workflows/silent-merge-check.yml, .github/silent-merge-test.py, .github/silent-merge-report.py).
Factors: security/stability 1, bug 0, performance 0, user value 0, leverage 1
Test infrastructure
P3 · new feature
Automates test suite execution across open pull requests to detect silent merge conflicts before they reach master. The value is clear for preventing regressions, though reasonably deferrable as an auxiliary CI test runner.
Membership: Labeled 'Tests' by maintainers; implements automation to run the test suite against simulated merges of open PRs.
Factors: security/stability 1, bug 0, performance 0, user value 0, leverage 1