Repository navigation
[fix](ci) Allow review status sync after fork PR merges - #69457
Merged
hello-stephen merged 1 commit intoOct 29, 2026
Merged
Conversation
### What problem does this PR solve? Related PR: apache#69386 The diagnostics helper checkout passes the workflow SHA explicitly. On a merged fork PR's `pull_request_target: closed` event, that trusted SHA can equal `pull_request.merge_commit_sha`; checkout's fork guard rejects it before aggregate status resolution starts. This occurred in [run 39721514326](https://github.com/apache/doris/actions/runs/39721514326/job/119635802685) and [run 39727686843](https://github.com/apache/doris/actions/runs/39727686843/job/119655282682). Use the trusted default self-checkout for `pull_request_target` events and retain workflow-SHA pinning for other callers. Credentials remain non-persistent and unsafe checkout stays disabled. Closed events still recalculate status for any remaining open PRs sharing the head. Add event regression tests for both observed merge SHAs, other PR target events, comment/dispatch SHA fallback, and closed-PR aggregation. ### Release note None ### Check List (For Author) - Test: - [x] Unit Test: `RUN_GH_HTTP_TESTS=1 python3 -m unittest discover -s .github/scripts -p 'test_*.py'` — 264 tests, OK, 19 platform skips. The new event tests fail against the original checkout expression and pass with the fix. - [x] Manual test: local replay of the pinned checkout `11d5960a326750d5838078e36cf38b85af677262` input helper and fork guard, 25 assertions. Both original merge cases reject; the fix preserves the trusted event ref/SHA; explicit fork-head checkout still rejects. No real Git checkout or hosted Actions run was performed by this harness. - [x] `actionlint .github/workflows/code-review-aggregate-status.yml` and `git diff --check`. - Behavior changed: - [x] Yes. Merged fork PR events can load diagnostics and proceed to status resolution. - Does this need documentation? - [x] No. Internal workflow fix. ### Check List (For Reviewer who merge this PR) - [ ] Confirm the release note - [ ] Confirm test cases - [ ] Confirm document - [ ] Add branch pick label
shuke987
requested review from
hello-stephen,
morningman and
yiguolei
as code owners
October 22, 2026 09:06
Contributor
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
This was referenced Oct 22, 2026
Contributor
Author
|
/review |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Reviewed PR #69457 at 9180b939e1327cef6c807f8734173848082e8227. No verified new findings.
The change addresses the checkout failure shown in run 39721514326 and run 39727686843. With an empty ref on the workflow repository, the pinned checkout implementation selects the event ref and SHA and skips the explicit-input guard; it still fetches the event commit. This supports the fix by static inspection. Sources: checkout input resolution, default-checkout handling, commit refspec.
| Review checkpoint | Conclusion |
|---|---|
| Goal and evidence | The expression removes the explicit merged-fork SHA collision. New tests cover both reported SHAs, configured PR target events, other callers, fallback, and shared-head release. |
| Focus and reuse | One production expression, explanatory comments, and a focused test module; existing status resolver is reused. |
| Concurrency and locks | Per-head job serialization and isolated runners remain unchanged; no new shared variables, locks, or deadlock path. |
| Lifecycle | Diagnostics are installed in RUNNER_TEMP before use and survive the later resolver checkout; cleanup remains intact. No static initialization changes. |
| Configuration | No configuration items or dynamic settings added. |
| Compatibility | Helper CLI, status contexts, function symbols, and storage formats remain unchanged. |
| Thrift/protobuf | No gensrc changes; field-ID checks are inapplicable. |
| Parallel paths | All five aggregate callers inspected. PR target current/previous-head jobs use event checkout; comment/dispatch callers retain SHA pinning. The resolver checkout uses a named branch and avoids the reported SHA collision. |
| Conditional logic | Event selection and short circuit behavior match the intended cases; comments explain the condition. The test interpreter supports the exact expression subset in use. |
| Test coverage | Fixture/input extraction, imports/discovery, fork/same-repository cases, closed-event routing, resolver negative cases, and neighboring workflow tests inspected. Unit fixtures do not establish hosted action execution. |
| Test results | No generated expected-result files changed. Added assertions are consistent with the inspected resolver/action logic; no test outcomes independently reproduced. |
| Observability and errors | Diagnostics installation, failure propagation, and head/run-specific failure artifacts are preserved; no additional logging or metrics needed for this expression change. |
| Transactions and persistence | No Doris transactions, EditLog, replay, or failover behavior changed. |
| Writes and crash behavior | No database writes introduced. Existing GitHub status publication and its error handling remain unchanged. |
| FE/BE propagation | No FE/BE variables or cross-module interfaces changed. |
| Performance | Constant-work expression; shallow sparse checkout remains in place with no added API requests or production scans. |
| Other issues and core invariants | No substantiated defect after the edge-case recheck. Database visibility, version/delete-bitmap consistency, and MemTracker invariants are unaffected; unsupported test expressions fail explicitly. |
No additional user focus was supplied. No existing inline threads or previously reported blocking findings require carry-forward. Both changed files and their relevant caller/helper/resolver paths were covered; no unresolved candidates remain.
Coverage limit: static inspection only. No builds, tests, author replay harness, or hosted workflow were executed during this review.
Review converged after 2 round(s).
Contributor
Author
|
skip buildall |
hello-stephen
approved these changes
Oct 29, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What problem does this PR solve?
Related PR: #69386
The diagnostics helper checkout passes the workflow SHA explicitly. On a merged fork PR's
pull_request_target: closedevent, that trusted SHA can equalpull_request.merge_commit_sha; checkout's fork guard rejects it before aggregate status resolution starts. This occurred in run 39721514326 and run 39727686843.Use the trusted default self-checkout for
pull_request_targetevents and retain workflow-SHA pinning for other callers. Credentials remain non-persistent and unsafe checkout stays disabled. Closed events still recalculate status for any remaining open PRs sharing the head.Add event regression tests for both observed merge SHAs, other PR target events, comment/dispatch SHA fallback, and closed-PR aggregation.
Release note
None
Check List (For Author)
RUN_GH_HTTP_TESTS=1 python3 -m unittest discover -s .github/scripts -p 'test_*.py'— 264 tests, OK, 19 platform skips. The new event tests fail against the original checkout expression and pass with the fix.11d5960a326750d5838078e36cf38b85af677262input helper and fork guard, 25 assertions. Both original merge cases reject; the fix preserves the trusted event ref/SHA; explicit fork-head checkout still rejects. No real Git checkout or hosted Actions run was performed by this harness.actionlint .github/workflows/code-review-aggregate-status.ymlandgit diff --check.Check List (For Reviewer who merge this PR)