Repository navigation
[fix](build) Preserve GitHub API failure diagnostics for code review - #69386
Merged
hello-stephen merged 1 commit intoOct 22, 2026
Merged
Conversation
### What problem does this PR solve? Issue Number: N/A Problem Summary: A malformed GitHub error response can make gh exit with only "unexpected end of JSON input", losing the HTTP status and request ID. Capture redacted response diagnostics for review startup, status synchronization, aggregation, helper downloads, submission and head polling. Upload separate failure records even when checkout/review preparation never ran or the caller handles the API failure. Preserve normal stdout, existing request arguments, write deduplication and retry behavior. ### Release note None ### Check List (For Author) - Test: Python workflow/helper suite: 256 tests, OK with 19 Linux-only skips on macOS; real gh loopback tests included. Submission compatibility suite: 37 tests passed after final formatting adjustment. actionlint v1.7.7 passed excluding the existing unsupported concurrency.queue syntax, with optional ShellCheck/Pyflakes disabled. git diff --check passed. - Behavior changed: No review verdict, permission, retry or posting changes; adds trusted diagnostic bootstrap and failure logging/artifact collection. - Does this need documentation: Yes; included review-github-diagnostics.md.
shuke987
requested review from
hello-stephen,
morningman and
yiguolei
as code owners
October 21, 2026 08:28
Contributor
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
Contributor
Author
|
skip buildall |
hello-stephen
approved these changes
Oct 22, 2026
4 of 16 tasks
Contributor
|
PR approved by anyone and no changes requested. |
Contributor
|
PR approved by at least one committer and no changes requested. |
shuke987
added a commit
to shuke987/doris
that referenced
this pull request
Oct 22, 2026
### 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
5 of 9 tasks
shuke987
added a commit
to shuke987/doris
that referenced
this pull request
Oct 22, 2026
### What problem does this PR solve? Related PR: apache#69386 Problem Summary: When the pending-status API call fails before repository checkout, the failure notification runs outside a Git checkout. Without an explicit repository, `gh pr comment` fails with `fatal: not a git repository`; both the notification and terminal failure step also misidentify the skipped context preparation step as the cause. This occurred in [run 38864234303](https://github.com/apache/doris/actions/runs/38864234303/job/116853272967). Pass `--repo` explicitly and report failed/cancelled pending-status and checkout steps by name. Distinguish actual context failure/cancellation from skipped or unstarted context in both reporting paths, preserving the existing auth/usage-limit recovery messages and failing exit status. ### Release note None ### Check List (For Author) - Test: - [x] Unit Test: seven new tests execute the actual workflow shell and env bindings through the diagnostic helper with a fake gh, including a non-Git working directory, early failure/cancellation, skipped context, preserved auth/review reasons, and notification failure. - [x] Regression test: new tests fail against the old workflow and pass after the fix. Full script suite: 266 tests, 247 passed and 19 Linux process-supervision tests skipped on macOS; includes real gh against a local HTTP server with fake credentials. - [x] Manual test: `git diff --check`; actionlint passes with only its pre-existing unsupported `concurrency.queue` diagnostic filtered. Hosted Actions failure-path validation remains pending. - Behavior changed: - [x] Yes. Early failure notifications no longer depend on a checkout to select the repository, and skipped context is no longer reported as a context preparation failure. - Does this need documentation? - [x] No. ### Check List (For Reviewer who merge this PR) - [ ] Confirm the release note - [ ] Confirm test cases - [ ] Confirm document - [ ] Add branch pick label
5 of 9 tasks
shuke987
added a commit
to shuke987/doris
that referenced
this pull request
Oct 22, 2026
### What problem does this PR solve? Related PR: apache#68872, apache#69386, apache#69469 Problem Summary: Merge current master while preserving the serialized aggregate as the only final code-review status writer. Keep diagnostics for existing paths and add trusted helper installation and failure artifacts to the new manual-pass job. Retain the pending step name so early-failure reporting remains compatible. ### Release note None ### Check List (For Author) - Test: 276 script tests OK, 19 platform skips. Durable workflow tests cover authorization, failed reads/writes, PR/head scope, closed heads and single final writer. actionlint with only the existing queue diagnostic filtered; diff check. - Behavior changed: Manual-pass API failures now retain diagnostics; intended manual acceptance semantics remain unchanged. - Does this need documentation: No additional user-facing behavior beyond the existing manual-pass guide.
hello-stephen
pushed a commit
that referenced
this pull request
Oct 29, 2026
### 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: 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.
hello-stephen
pushed a commit
that referenced
this pull request
Nov 2, 2026
) ### What problem does this PR solve? Related PR: #69386 Problem Summary: When the pending-status API call fails before repository checkout, the failure notification runs outside a Git checkout. Without an explicit repository, `gh pr comment` fails with `fatal: not a git repository`; both the notification and terminal failure step also misidentify the skipped context preparation step as the cause. This occurred in [run 38864234303](https://github.com/apache/doris/actions/runs/38864234303/job/116853272967). Pass `--repo` explicitly and report failed/cancelled pending-status and checkout steps by name. Distinguish actual context failure/cancellation from skipped or unstarted context in both reporting paths, preserving the existing auth/usage-limit recovery messages and failing exit status.
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?
Issue Number: N/A
Problem Summary:
An empty or malformed JSON error response can make
gh apifail with onlyunexpected end of JSON input, hiding the HTTP status and GitHub request ID. This prevents distinguishing transient server failures from rejected requests in the review pipeline.Add a shared diagnostic helper for review startup, status synchronization and aggregation, helper downloads, submission, and head polling. Each failure records HTTP status, allowlisted response headers, bounded redacted response/output excerpts, exit code or timeout, and run context. Failure artifacts are uploaded even before PR checkout or when the caller handles the error. Load the helper from the immutable workflow commit before the first API request; distinguish aggregate artifacts by head SHA.
Outgoing request headers, bodies, and complete command arguments are excluded. Credential values and fields are redacted before truncation. Response excerpts depend on gh's debug format and are not complete wire captures.
Normal stdout, existing request arguments, permissions, review verdicts, submission deduplication, and retry behavior are preserved.
Release note
None
Check List (For Author)
Test
concurrency.queuesyntax ignored; optional ShellCheck/Pyflakes disabled. Workflow equivalence checks andgit diff --checkpassed.Full suite command:
RUN_GH_HTTP_TESTS=1 PYTHONDONTWRITEBYTECODE=1 python3 -m unittest discover -s .github/scripts -p 'test_*.py' -vBehavior changed:
Does this need documentation?
.github/scripts/review-github-diagnostics.md.Check List (For Reviewer who merge this PR)