[chore](ci) refactor code review workflow to run on PR events with comment-triggered dispatch#62369
[chore](ci) refactor code review workflow to run on PR events with comment-triggered dispatch#62369zclllyybb wants to merge 1 commit intoapache:masterfrom
Conversation
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
|
run buildall |
|
/review |
There was a problem hiding this comment.
Findings:
-
High:
.asf.yamlnow replaces the oldNeed_2_Approvalrequired check withcode-review, but branch protection in.asf.yamlstill requires onlyrequired_approving_review_count: 1. Since the deletedpr-approve-status.yml+tools/maintainers/check_review.pypath was the extra enforcement for two approvals, this PR weakens merge protection from two approvals to one. -
High: the new required
code-reviewstatus can only become green after a botAPPROVEDreview exists for the current head SHA, but the new runner itself relies ongh pr review --approve/--request-changesand the PR description explicitly notes thatAllow GitHub Actions to create and approve pull requestsstill needs to be enabled. Until that external repo setting is flipped,/reviewruns cannot produce the review object thatopencode-review.yml/refresh-required-checkexpect, so the requiredcode-reviewcheck stays pending or flips to failure and blocks merges.
Critical checkpoint conclusions:
- Goal of the task: The PR tries to split AI review into a required status-sync workflow plus a comment-triggered runner. The implementation does not safely accomplish that goal because it both weakens approval policy and introduces a required check that cannot reliably pass with the repository’s current settings. No in-repo test proves the full end-to-end branch-protection flow.
- Scope/minimality: The change is not fully focused because it mixes workflow refactoring with a policy change (
Need_2_Approvalremoval) and a new required status contract. - Concurrency: No meaningful shared-memory/threading risk here; the main risk is workflow/event ordering and cross-workflow state propagation through PR reviews and commit statuses.
- Lifecycle/config: This change adds a new required-status dependency on external repository configuration (
Allow GitHub Actions to create and approve pull requests) without making that prerequisite explicit in code or preserving current behavior until it is enabled. - Parallel/related paths: The previous approval-count path was removed, but no equivalent enforcement remains elsewhere in the repo.
- Conditional checks: The review-state checks are straightforward, but they assume the bot review object will always exist after
/review, which is false in the current repo configuration. - Test coverage: No repository-side test or simulation covers the critical branch-protection behaviors. Linked external PR trials do not prove the production repo configuration.
- Test results: No generated test artifacts were added; nothing in-tree demonstrates all new status outcomes are correct.
- Observability: Status descriptions are adequate for debugging, but they only report the missing bot review after the system is already wedged.
- Compatibility/data/transactions/FE-BE vars: Not applicable to this CI/workflow-only change.
- Performance: No significant performance concern identified.
- Other issues: The two blocking process regressions above should be resolved before merge.
Overall opinion: blocking issues found.
There was a problem hiding this comment.
Pull request overview
Refactors the automated code review setup so a code-review required check is maintained on PR events, while the actual AI review is dispatched via /review PR comments and executed in a reusable workflow.
Changes:
- Replace the old “Need_2_Approval” workflow with a
code-reviewstatus context required check. - Add a comment-driven dispatcher (
/review) that invokes a reusable review runner workflow. - Update docs/checklists to align with the new review workflow expectations.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
AGENTS.md |
Documentation updates for workflow/standards; adds a new commit hygiene rule (currently duplicated/overly absolute). |
.github/workflows/pr-approve-status.yml |
Removes legacy “Need_2_Approval” review enforcement workflow. |
.github/workflows/opencode-review.yml |
New PR-event workflow that syncs the required code-review commit status based on bot review state. |
.github/workflows/opencode-review-runner.yml |
New reusable workflow that runs OpenCode and posts an APPROVE / REQUEST_CHANGES review. |
.github/workflows/opencode-review-comment.yml |
New /review comment dispatcher that marks status pending, runs the review runner, then refreshes status. |
.claude/skills/code-review/SKILL.md |
Expands review checkpoints (notably around test results and concurrency reasoning). |
.asf.yaml |
Updates protected branch required status contexts to require code-review instead of Need_2_Approval. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…mment-triggered dispatch Split code review workflow into two parts: 1. Main workflow (opencode-review.yml) now triggers on PR events (opened/synchronize/reopened/ready_for_review) as a required check, but only runs actual review when called via workflow_call 2. New comment dispatcher (opencode-review-comment.yml) triggers on /review comments and invokes the main workflow with PR context This ensures the Code Review check is always
|
/review |
There was a problem hiding this comment.
Findings:
-
High:
.github/workflows/opencode-review-comment.ymlallows any PR commenter to trigger/review, then forwardssecrets: inheritinto the reusable runner. Other comment-triggered workflows in this repo explicitly restrict trusted actors (doris-robotor an allowlist), but this path has no equivalent authorization gate. That gives untrusted contributors an on-demand way to invoke a privileged workflow on their PRs. -
High:
.github/workflows/opencode-review-runner.ymlchecks out the untrusted PR head and then explicitly tells OpenCode to read in-repoAGENTS.mdandcode-reviewskill content from that checkout whileGH_TOKENand the persisted OpenCode credential are available. Because those instruction files are part of the PR diff, a malicious contributor can modify them to coerce the agent into exfiltrating credentials or rubber-stamping the review. -
High: the new required
code-reviewstatus can only turn green after a botAPPROVEDreview exists for the current head SHA, but the runner depends on the repository-levelAllow GitHub Actions to create and approve pull requestssetting to create that review. The PR description says that setup is still outstanding, so merging this before the repo setting is enabled can deadlock PRs behind a required check that cannot pass.
Critical checkpoint conclusions:
- Goal of the task: The PR tries to replace the old approval-status path with a split design: PR-event status sync plus comment-triggered AI review. In its current form it does not safely accomplish that goal because the dispatch is not authorization-gated, the runner trusts untrusted in-PR instructions while holding secrets, and the required-check path still depends on external repo configuration.
- Scope/minimality: Not fully focused. Besides workflow refactoring, it changes branch-protection behavior and introduces a new privileged comment-trigger path.
- Concurrency: No shared-memory/threading concern here. The main risk is event/workflow coordination and privilege boundaries between the
issue_commentdispatcher, the reusable runner, and the status-sync workflow. - Lifecycle/static initialization: Not applicable.
- Configuration items: No Doris runtime config is added, but the workflow now has a hidden hard dependency on repository settings and trusted instruction sources that is not enforced in code.
- Incompatible changes: No FE/BE or storage compatibility issue. CI behavior compatibility regresses because the required check can wedge PRs until external setup is completed.
- Parallel code paths: Yes. Other comment-triggered workflows in this repo gate trusted actors; this new
/reviewpath should follow the same restriction. - Special conditional checks: The
/reviewcondition is too broad and lacks commenter authorization/provenance checks. - Test coverage: There is no in-repo automation covering the critical authorization, secret-handling, and required-check behavior. Linked external trials do not prove the production repository configuration.
- Test results added/modified: None.
- Observability: Status descriptions are adequate once the flow runs, but they do not mitigate unauthorized dispatch or proactively surface missing repository capabilities.
- Transaction/persistence: Not applicable.
- Data write/atomicity: Not applicable.
- FE-BE variable propagation: Not applicable.
- Performance: No significant performance issue identified beyond the blocking correctness/security issues above.
- Other issues: None beyond the blockers listed above.
Overall opinion: blocking issues found.
| runs-on: ubuntu-latest | ||
| if: >- | ||
| github.event.issue.pull_request && | ||
| contains(github.event.comment.body, '/review') |
There was a problem hiding this comment.
This introduces a privileged issue_comment trigger with no authorization check beyond contains(comment.body, '/review'). Unlike the repo's other comment-driven workflows, there is no github.actor/author_association allowlist here before calling the reusable workflow with secrets: inherit.
That means any external contributor can request a secret-bearing review run on demand for their PR. Please gate this path to trusted actors before dispatching the runner.
| - PR Head SHA: PLACEHOLDER_HEAD_SHA | ||
| - PR Base SHA: PLACEHOLDER_BASE_SHA | ||
|
|
||
| Before reviewing any code, you MUST read and follow the code review skill in this repository. During review, you must strictly follow those instructions. |
There was a problem hiding this comment.
This runner checks out the untrusted PR head and then tells OpenCode to read AGENTS.md and the repository's code-review skill from that checkout while GH_TOKEN and the persisted OpenCode credential are in scope.
Because those instruction files are review inputs and live inside the PR, a malicious contributor can modify them to redirect the agent into exfiltrating credentials or auto-approving the PR. The review instructions need to come from trusted base-branch content, or the analysis must run without secrets/write tokens against the untrusted checkout.
make AI review to be official access control workflow.
Split code review workflow into two parts:
1. Main workflow (opencode-review.yml) now triggers on PR events (opened/synchronize/reopened/ready_for_review) as a required check, but only runs actual review when called via workflow_call
2. New comment dispatcher (opencode-review-comment.yml) triggers on /review comments and invokes the main workflow with PR context
and remove
Need 2 Approval.tested in zclllyybb#24 and zclllyybb#25
still need to enable
Allow GitHub Actions to create and approve pull requestsand set workflow permissions toRead and write permissions