Repository navigation
[fix](ci) Preserve early review failure notifications and causes - #69469
Merged
hello-stephen merged 1 commit intoNov 2, 2026
Merged
hello-stephen merged 1 commit into
hello-stephen merged 1 commit into
Conversation
### 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
requested review from
hello-stephen,
morningman and
yiguolei
as code owners
October 22, 2026 12:27
Contributor
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
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.
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 both changed files at a5d111bab9544dfedd385a20c2419665a3bdcaea and their upstream/downstream workflow paths. No actionable defects were verified; there are no new inline findings or existing blocking comments to carry forward.
Critical checkpoint conclusions:
- Goal and evidence: the notification now supplies an explicit repository before checkout is available, and both reporters identify pending-status/checkout failures or cancellation. The seven new tests exercise the actual shell and env bindings; inspected statically, not executed here. Repository selection matches the GitHub CLI contract.
- Scope and clarity: changes are limited to two step IDs, corresponding outcome bindings, failure classification, repository selection, and focused tests.
- Concurrency: no new parallel execution or shared mutable state; existing per-PR serialization is unchanged.
- Lifecycle: the diagnostics helper is installed outside the PR checkout before input resolution and pending-status; required PR outputs exist before the newly handled failures. No new lifetime issue found.
- Configuration: no configuration item added.
- Compatibility: no function, storage, wire-format, or rolling-upgrade change.
- Thrift/protobuf: no generated definitions changed.
- Parallel paths: both the PR notification and terminal failure step use the same new early-step/context classification; terminal exit remains nonzero even when commenting fails.
- Conditional checks: failure/cancelled are distinguished from intentionally skipped pending-status and skipped/unstarted context. The context fallback explains its purpose. Outcome values match the Actions context contract.
- Test coverage: new cases cover pre-checkout pending failure, checkout failure with pending success/skipped, early cancellation, skipped/unstarted context, actual context failure/cancellation, preserved auth/review reasons, and notification failure. Existing credential recovery tests cover the preserved auth-marker branches.
- Test results: no expected-result artifacts changed; assertions and fake-gh exit handling are consistent with the intended behavior. Tests were not run under this review's static-only contract.
- Observability: named initiating steps and the workflow-run link improve diagnosis; existing API diagnostics remain available.
- Transactions and persistence: no database transaction, EditLog, replay, or failover behavior affected.
- Writes and atomicity: the existing failure-comment operation gains explicit repository selection; database writes and crash consistency are unaffected.
- FE/BE variables: no cross-process variables or send paths changed.
- Performance: bounded shell comparisons and small isolated tests add no meaningful runtime overhead.
- Other issues and core invariants: no additional verified bug found after checking skipped, failed, cancelled, successful, superseded, auth/usage, and notification-failure paths. Database visibility/version/delete-bitmap and BE memory-tracking invariants are outside this CI-only change.
User focus: no additional focus was supplied. Coverage limits: this was a static review; no builds, tests, or hosted Actions failure/cancellation runs were executed. Both changed files and related reporting paths were covered, with no unresolved static candidate or uncovered source scope.
Review converged after 2 round(s).
7 of 11 tasks
Contributor
Author
|
skip buildall |
hello-stephen
pushed a commit
that referenced
this pull request
Nov 2, 2026
…69510) Related PR: #69469 Problem Summary: A single transient GitHub response currently aborts the review pipeline. [Input resolution for #69322](https://github.com/apache/doris/actions/runs/39794013905/job/119876748768) stopped on a GET 503 despite `Retry-After: 2`; [aggregate resolution for #69438](https://github.com/apache/doris/actions/runs/39760934956/job/119764632659) stopped on a paginated GET 504 after emitting partial output. The shared helper now retries recognized body-free REST GETs on 502/503/504, for at most three attempts. Each attempt has a 30-second limit; attempts and waits share a 90-second budget or the caller's shorter timeout. It respects Retry-After, records every failed attempt, discards failed pagination output, and returns only a successful attempt's complete output. Exhaustion still fails. Writes, GraphQL, unknown argument forms, other HTTP errors, transport/timeouts and JSON parsing errors are not retried. Input resolution can also fail before publishing its PR-number output. Failure notices now fall back to the authorized trigger's numeric PR identity, explicitly select the repository, and name the input-resolution failure in both the notice and terminal error. This fallback does not permit status publication for an unresolved head/base. This change can merge independently. It includes the same explicit `--repo` argument as #69469; a local combination with #69469 and #69504 merged cleanly and passed the macOS suite.
hello-stephen
approved these changes
Nov 2, 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
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 commentfails withfatal: 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.Pass
--repoexplicitly 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)
git diff --check; actionlint passes with only its pre-existing unsupportedconcurrency.queuediagnostic filtered. Hosted Actions failure-path validation remains pending.Check List (For Reviewer who merge this PR)