Repository navigation
ci: bind the Required Merge Gate to its own event's runs, fail drafts, keep both sides of renames - #345
ci: bind the Required Merge Gate to its own event's runs, fail drafts, keep both sides of renames#345kiwidream wants to merge 3 commits into
Conversation
|
@codex review |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
Codex Review: Didn't find any major issues. Nice work! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
|
@codex review |
|
Codex Review: Didn't find any major issues. Swish! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
|
@codex review |
|
Codex Review: Didn't find any major issues. Delightful! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
The gate read the Core Checks Gate and Full Validation Gate check runs by name across every check suite on the head commit. On #268 the newest Full Validation Gate after ready_for_review was the draft-era skipped one, so the gate failed although nothing had. The same selection lets the push runs of a branch head, which pass trivially for a docs-only push, pass an integration pull request from that branch before its own Full Validation has run. Each wait now reads a run of the workflow that reports the check: same head SHA, event, head branch and repository and, for a pull request, the same pull request. A workflow the event also started has to be read from a run created at most 60 s before this gate run, because GitHub stamps the runs of one event up to a second apart in either order, or at least 60 s after the previous gate run on that head, which is too late to be the previous event's. Core Checks does not run for ready_for_review, labeled or unlabeled, so on those events its newest run for the head is read. The newest matching run wins, and its check runs are read from its own check suite, so a re-run of it is seen. Only the gate job reads the API now, with actions: read and checks: read; the other jobs keep contents: read. The gate job was skipped for drafts, and a skipped required check counts as passing until the next gate run reports. Drafts now run the gate and fail it. Every changed-file step used git diff with rename detection, which names only the destination of a rename, so moving a source file into doc/ routed as docs-only and skipped source validation. All 31 sites in the three workflows now pass --no-renames. Refs #312 Refs #313 Refs #314 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The gate read Core Checks' newest run for the head on ready_for_review, labeled and unlabeled, because core-checks.yml declared no pull_request types and GitHub starts it only for opened, synchronize and reopened. That fallback had no time bound and ignored the base: on a head branch with two pull requests, one could pass on the other's docs-only runs, and after a retarget a label or ready event read Core Checks from the old base, so the mainnet publication check of a 1.0.0 target never ran. Core Checks now declares the gate's types, the gate binds it exactly as it binds Full Validation, and the fallback and the event action input it needed are gone. A contract test requires both workflows to run, unfiltered, for every event the gate runs for. Nothing tested that the newest run of an event wins, or that the jq event and head SHA filters are needed: the suite passed with min_by, max_by(.id) or either filter dropped. New tests cover a pull request opened with a label, whose cancelled opened runs fall inside the window, a push run started in the same second as the pull request's synchronize runs, and a run for the next commit. The modelled listing is now ordered newest first, as GitHub lists runs. Refs #312 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
test/lint/lint-files.py requires files with a shebang line to have mode 755. ci/checks/test_changed_files_contract.py was added with mode 644, so the lint job failed. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
@codex review |
f1071de to
6b14e82
Compare
|
Codex Review: Didn't find any major issues. Bravo. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
Summary
Fixes #312
Fixes #313
Fixes #314
(GitHub closes issues automatically only on merges into the default branch, so these stay open until
1.x.xreachesmain.)ci.ymlorcore-checks.yml) and match the gate's own run on head SHA, event, head branch and head repository, plus the PR number for pull requests. Check runs are then read from that run's check suite (/check-suites/{id}/check-runs), never from the commit-wide listing. The newest matching run wins, and a re-run stays in its suite, so it is seen.core-checks.ymlnow declares the gate'spull_requesttypes (opened,synchronize,reopened,ready_for_review,labeled,unlabeled).ci.ymlalready declared them. Both workflows therefore run for every gate event and are bound the same way, with no fallback. The gate job alone getsactions: readandchecks: read; the workflow-level token is nowcontents: readonly.if: ${{ always() }}). For a draft its step fails first with "Pull request #N is a draft: draft pull requests are not mergeable. Mark it ready for review to run the gate."ready_for_reviewstays in the triggers, so marking the PR ready starts a fresh gate run.git diff --name-onlychanged-file sites inrequired-merge-gate.yml,ci.ymlandcore-checks.ymlnow pass--no-renames. That includes the release-validator detection step incore-checks.yml; thebuild-smokejob is not touched.Where the brief's rule had to change, with evidence
The issue and brief proposed binding to runs "created at or after this gate run". Production data shows where that rule would leave the gate waiting forever, and it would then time out red:
SIBLING_RUN_SKEW_SECONDS). It also counts runs created at least 60 s after the previous gate run on the same head, which is too late to be the previous event's runs. A run created within 60 s after the previous gate run is treated as the previous event's, because the previous event's own runs can also be stamped after its gate run.core-checks.ymldeclared nopull_requesttypes, so it ran only foropened,synchronizeandreopened; on subtree: import libbitcoinpqc v0.3.2 signer stack scrub #268 at 17:46:13Z only the gate and Full Validation started. The first round read Core Checks' newest run for those events. An adversarial review showed that fallback had no time bound and ignored the base. With two PRs from one head branch, PR 7's gate passed on PR 8's green docs-only runs. After a retarget, a label or ready event read Core Checks from the old base, so the mainnet publication check for a1.0.0target never ran. Round 2 removes the fallback and gives Core Checks the gate's types instead. A contract test reads both workflow files and requires each one to run for every event the gate runs for: every gatepull_requesttype, the gate's branches forpull_request,pushandmerge_group, and nopaths,paths-ignoreorbranches-ignorefilter.I re-checked every claim in the three issues against the code and the API. All are confirmed with one exception: #312 says a maintainer had to re-run gate run 41101389289. That run shows
run_attempt: 1, so I could not confirm a re-run. The false red itself is confirmed: its log saysFull Validation Gate concluded skipped.Testing
A broken gate blocks every merge, so every test runs the gate's real
run:script, lifted out of the YAML, against a stubbedgh. The real changed-file steps run in a scratch git repository with git's default rename detection.ci/checks/test_merge_gate_poller.py(35 tests).FakeGhnow routes replies by endpoint and serves a model of GitHub built from the workflow-run fields the API returned for subtree: import libbitcoinpqc v0.3.2 signer stack scrub #268 and wallet: keep the SQLite lock when probing loaded wallet files #297. New tests:skipped, and the new run's gate job does not exist yet. The gate stays pending and passes once this event's check passes.1.x.xare green as docs-only, and GitHub ties them to the open PR. The PR's own runs are still running, so the gate stays pending.openedruns andlabeledruns seconds apart, all inside the window. The cancelledopenedruns are not read, including when the newer runs have the lower run IDs, and within one second the higher run ID wins.synchronizeruns in the same second, on the same commit and branch, and GitHub ties both to the PR. Only the event filter separates them.head_shafilter.ready_for_reviewis a trigger, and the token scopes are as above.ci/checks/test_changed_files_contract.py(new, 3 tests, run inclassify-changes). Every changed-file step of the three workflows runs forpull_request,push, the push of a new branch,merge_groupand thediff-treefallback, on a head that renamessrc/validation.cppandci/release/verify_release.pyintodoc/reference/. Both sides must be listed, and the real classifier must route the result assource. The release-validator step must reportchanged=true. A static scan of every workflow requires--no-renameson eachgit diff,git logorgit showthat names files.ci/checks/test_classifier_ci_contract.py: its fakeghalso serves the workflow-run listing.Fails on the base, passes with the fix. I replayed the new test files against the
21fee6ee8eworkflows:Full Validation Gate concluded skipped.for subtree: import libbitcoinpqc v0.3.2 signer stack scrub #268, rc=0 for the push-context fail-open, and rc=0 for a draft. Every pre-existing poller test and subtest still passes there.doc/reference/validation.mdanddoc/reference/verify_release.md, the release step reportedchanged=false, and the scan found 31 sites without--no-renames.previous + 1floor fails the "runs created just after the previous event" case.Mutation check of run selection (round 2). Each mutant below was applied to a copy of the workflows, and the poller suite was run against it. Every mutant fails at least one test:
max_by([created, .id])→min_by([created, .id])test_the_newest_run_of_this_event_wins(all 3 cases)max_by(.id)test_the_newest_run_of_this_event_wins("labeled created later, with lower IDs")max_by(created)test_the_newest_run_of_this_event_wins("same second").event == $eventtest_another_events_run_inside_the_window_is_not_read.head_sha == $shatest_a_run_for_another_commit_is_not_readcore-checks.yml's newtypes:test_every_event_the_gate_runs_for_starts_the_workflows_it_readsAcceptance commands, all passing:
for t in ci/checks/test_*.py; do python3 "$t"; doneon system Python 3.14.4 and on Python 3.12.15: all 8 files OK.actionlint1.7.12, with shellcheck, on all workflows: clean, as it was on the base.python3 test/lint/lint-files.py: passes afterf1071de905setci/checks/test_changed_files_contract.pyto mode 755 (it has a shebang and was added as 644, which failed CIlint). The otherci/checks/and.github/files this PR touches already had matching modes.Live: this PR changes
required-merge-gate.yml, so its own gate runs the new script.Opened as a draft, the gate failed: run 41927559959 reported
Pull request #345 is a draft: draft pull requests are not mergeable. Mark it ready for review to run the gate.On the hosted runner,classify-changesran all four contract suites, including the new changed-file contract, and they passed.Then I marked it ready. The round-1
ready_for_reviewgate (run 41927824888) was still polling more than 90 minutes later, so the new API reads worked live. A failure to read the runs listing fails closed within about 3 minutes. Its Full Validation run was still queued on the self-hosted runners when round 2 was pushed.Built locally.
Ran focused unit or functional tests for the changed area.
Ran lint or formatting checks relevant to this change.
Not run. Reason:
Target Branch
mainor a maintainer-requested release branch such as0.1.x. (1.x.x, for v1.0.1.)Risk / Review Notes
Notes:
Required Merge Gateon the shared head counts for both PRs.edited) still keeps the old base's gate result. I did not addeditedto the triggers: Full Validation and Core Checks do not run onedited, so a run bound to that event would wait for runs that never start. Every title or body edit would also cancel an in-progress gate through the concurrency group.concluded cancelled. Re-running the gate then reads the newer run.labeled,unlabeledorready_for_reviewevent now waits for a new Full Validation run and, with round 2, a new Core Checks run. Both workflows'cancel-in-progressconcurrency also cancels and restarts the runs already in progress for that PR. This costs runner time on every label change.Required Merge Gate. Right after the integration PR opens, the push gate's green check on the same SHA briefly satisfies it. This lasts until the PR's own gate run creates its check: it reports pending at first, then its verdict. The gate cannot close this window; only branch protection could.--no-renamesscan is line-based. Agit diffcommand split across backslash continuations is not matched by the scan. The behavioral test still runs every changed-file step that exists today.build-smokejob incore-checks.yml. This PR edits that file'son:block and changed-file steps, and the merge is clean.Docs / Process Impact
Choose exactly one:
ci/README.mdanddoc/development/public-branch-and-rulesets.mdsay nothing about drafts or about how the gate selects check runs, so they stay accurate. There is no release-notes fragment because node users see no change.🤖 Generated with Claude Code