Repository navigation
Conversation
### What problem does this PR solve? Issue Number: None Related PR: apache#68032, apache#68622 Problem Summary: Aggregate review status overwrites unscoped manual force passes. Record authorized explicit acceptance for a PR and its current head, honor it across automated reruns, and prevent reuse for new heads or other PRs. Make the serialized aggregate job the only final-status writer. Shared heads still require every open PR to qualify; previous final verdicts remain until the next successful aggregation. Leave unused heads unchanged after PR closure. ### Release note None ### Check List (For Author) - Test: 55 related unit tests, 8 mocked workflow scenarios, actionlint v1.7.7 with shellcheck disabled, and git diff --check passed. No live status writes. - Behavior changed: Yes, add PR/head manual acceptance; final status changes only on aggregate verdict publication. - Does this need documentation: Yes, usage and rollout notes included.
shuke987
requested review from
hello-stephen,
morningman and
yiguolei
as code owners
October 10, 2026 07:33
Contributor
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
### 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.
Static review of all 10 changed files at b6fb4df found no new actionable issues. No existing inline findings were supplied to reconfirm. All three independent reviewers returned NO_NEW_VALUABLE_FINDINGS; the initial risks were resolved with evidence.
The reviewed flow is: created PR comment -> live author permission and open PR/head validation -> manual PR/head source -> serialized aggregation of all open PR contexts on that head -> final code-review status.
Applicable critical checkpoints:
- Goal and proof: the manual source overrides automated results for the accepted PR/head, survives base movement, and leaves other PRs on the same head independently gated. Validator, resolver and extracted-workflow tests cover these behaviors; their assertions were reviewed statically.
- Scope and reuse: the change is focused on manual acceptance and final-status ownership. It reuses the existing diagnostics helper, source-status protocol and reusable aggregator.
- Concurrency: source producers may overlap, while final writes use the same head-specific aggregate concurrency group. Queued aggregation reads current durable sources. Each source producer explicitly invokes aggregation after publication. Retaining the previous final verdict during reruns or aggregation failures is documented behavior.
- Lifecycle: acceptance is tied to the explicit SHA and PR number. A racing push cannot redirect it to the new head. Current/previous-head, close/reopen, base movement, comment creation/edit/delete and shared-head paths were traced. No static initialization or object-lifetime change applies.
- Configuration: no Doris configuration item is added; permission and PR state are read for each command.
- Compatibility and rollout: automated/local/skip source formats remain supported. Ignoring legacy unscoped force-pass records is intentional and documented. Trusted scripts and workflows must roll out together; old in-flight workflow behavior and live acceptance verification are documented limits. No symbols or storage formats change.
- Parallel paths: manual, local, skip, automated and PR lifecycle paths retain explicit aggregate callers. Acceptance does not depend on status events generated by GITHUB_TOKEN. Caller permissions and trusted checkouts were inspected.
- Conditions and errors: the parser requires a complete command and current full SHA, a matching open PR, a human creator matching the sender, and live write/admin permission. Validation/read/write failures stop before acceptance outputs; successful source publication precedes downstream aggregation. The permission endpoint contract was checked against GitHub's API documentation.
- Test coverage: positive and negative fixtures cover malformed/denied/stale commands, wrong or closed PRs, read/write failures, precedence, reruns, base movement, new/shared/closed heads, diagnostics wiring and final-writer ownership. Workflow conditions, outputs, needs and permissions were checked directly because the shell fixtures do not execute the Actions scheduler.
- Test-result accuracy: expected fixture outcomes agree with the inspected control flow; no generated regression result files change. Python syntax parsing succeeded for all six changed Python files.
- Observability: manual sources link to the authorizing comment and identify author/PR; rejection errors explain the failed check. API failures retain diagnostic artifacts, and aggregate statuses link to the workflow run.
- Persistence and data writes: latest status ID wins per exact source context. The source write precedes success outputs; final publication is serialized and respects publish=false when no open PR uses the head. There are no Doris transaction, EditLog or failover changes.
- Performance: the implementation reuses existing paginated reads and status scans. No material new hot path, redundant API loop or retry behavior was found.
- Other risks: PR/head isolation, aggregate scheduling, publication ordering and the suspected release-workflow rollout path were independently rechecked and dismissed with evidence. Current GitHub event documentation confirms the default-branch context used for PR lifecycle runs.
Thrift/protobuf field IDs, FE-BE variables, database visibility/version/delete-bitmap invariants, BE memory/null handling and C++ static initialization are not affected. No additional user focus was provided.
Verification was static only: no builds or tests were run, and hosted Actions acceptance was not exercised. The fake-gh tests do not establish live scheduler/API behavior; that remains the documented rollout verification. There are no unresolved code candidates or uncovered changed paths within this review scope.
Review converged after 1 round(s).
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: None
Related PR: #68032, #68622
Problem Summary:
A manual
code-review: successstatus with descriptionforce passis overwritten by subsequent aggregation because it is not a scoped review source. Preserving that unscoped success would also approve other PRs sharing the same commit.Add an explicit PR comment command:
The trusted workflow verifies the human comment creator's write/admin permission, an open matching PR, and the exact current head. It records a separate
code-review/source/manual/pr-<number>/head-<sha>status linked to the authorizing comment. Aggregation honors that acceptance across automated reruns and base movement, without applying it to a new head or another PR.The manual-pass job uses the trusted HTTP diagnostics helper for permission checks, PR reads, and source publication; failures retain diagnostic artifacts and do not emit acceptance.
Only the serialized aggregate job now publishes the final
code-reviewcontext. Remove unconditional pending writes from the runner and local/skip/aggregate paths so they cannot erase manual acceptance. When no open PR uses a head, leave its final status unchanged.Scope and behavior: GitHub's final commit status is still shared by identical heads. Every open PR on that SHA must independently qualify: accepting A cannot approve B, and B can keep their shared check pending/failed. This PR does not introduce independently green/red merge gates for identical heads or change branch protection. During a rerun, the previous aggregate verdict remains visible until a new aggregate verdict is published; source statuses still report pending. If aggregation fails, the previous final verdict remains until a successful rerun.
Rollout: Merge the workflow and trusted scripts together. Existing unscoped force-pass records are not automatically migrated; a maintainer must issue the explicit command on the intended PR. Comment edits/deletion do not revoke an accepted source. Allow runs using older workflow definitions to finish before final rollout verification. See
.github/scripts/manual-code-review-pass.md.Release note
None
Check List (For Author)
Test
18a2cc5e0e48c699c204a5d574791a49a5a22ec8: 276 tests OK, 257 passed and 19 platform skips.concurrency.queuediagnostic;git diff --check.Behavior changed:
Does this need documentation?
Check List (For Reviewer who merge this PR)