Skip to content

[fix](build) Preserve GitHub API failure diagnostics for code review - #69386

Merged
hello-stephen merged 1 commit into
apache:masterfrom
shuke987:codex/review-api-diagnostics
Oct 22, 2026
Merged

hello-stephen merged 1 commit into
apache:masterfrom
shuke987:codex/review-api-diagnostics

Conversation

@shuke987

Copy link
Copy Markdown
Contributor

What problem does this PR solve?

Issue Number: N/A

Problem Summary:

An empty or malformed JSON error response can make gh api fail with only unexpected 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

    • Unit Test: Python workflow/helper suite completed 256 tests, OK with 19 Linux-only tests/subtests skipped on macOS. The submission suite passed all 37 tests after the final formatting adjustment.
    • Manual test: Real gh 2.88.1 against a loopback HTTP server with fake credentials preserved normal 201 output and captured status/request ID for empty JSON 422, 502, and 503 errors. Four calls produced exactly four POST requests.
    • Workflow validation: actionlint v1.7.7 passed with only the existing unsupported concurrency.queue syntax ignored; optional ShellCheck/Pyflakes disabled. Workflow equivalence checks and git diff --check passed.

    Full suite command:

    RUN_GH_HTTP_TESTS=1 PYTHONDONTWRITEBYTECODE=1 python3 -m unittest discover -s .github/scripts -p 'test_*.py' -v
  • Behavior changed:

    • Yes. Adds trusted diagnostic bootstrap and failure artifacts; existing review decisions and API retry/posting policy remain unchanged.
  • Does this need documentation?

    • Yes. Included .github/scripts/review-github-diagnostics.md.

Check List (For Reviewer who merge this PR)

  • Confirm the release note
  • Confirm test cases
  • Confirm document
  • Add branch pick label

### 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.
@hello-stephen

Copy link
Copy Markdown
Contributor

Thank you for your contribution to Apache Doris.
Don't know what should be done next? See How to process your PR.

Please clearly describe your PR:

  1. What problem was fixed (it's best to include specific error reporting information). How it was fixed.
  2. Which behaviors were modified. What was the previous behavior, what is it now, why was it modified, and what possible impacts might there be.
  3. What features were added. Why was this function added?
  4. Which code was refactored and why was this part of the code refactored?
  5. Which functions were optimized and what is the difference before and after the optimization?

@shuke987

Copy link
Copy Markdown
Contributor Author

skip buildall

@hello-stephen
hello-stephen merged commit a2638f3 into apache:master Oct 22, 2026
38 checks passed
@github-actions

Copy link
Copy Markdown
Contributor

PR approved by anyone and no changes requested.

@github-actions github-actions Bot added the approved Indicates a PR has been approved by one committer. label Oct 22, 2026
@github-actions

Copy link
Copy Markdown
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
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
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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approved Indicates a PR has been approved by one committer. reviewed

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants