This is an automated email from the ASF dual-hosted git repository.
hello-stephen pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/doris.git
The following commit(s) were added to refs/heads/master by this push:
new 81eeced65c2 [fix](build) Serialize automated reviews for each PR
(#68699)
81eeced65c2 is described below
commit 81eeced65c2dc6be534d53815cc1006b9498a22a
Author: shuke <[email protected]>
AuthorDate: Tue Oct 13 19:23:50 2026 +0800
[fix](build) Serialize automated reviews for each PR (#68699)
Related PR: #68646, #68579
Problem Summary:
Repeated `/review` comments can start overlapping reviews of the same
PR. When one job posts first, the existing duplicate-submission guard
rejects the other job's final candidate, even if it contains different
findings. The outer completion check then replaces that rejection with
"No final review submission was declared".
Serialize the `code-review` job by PR number across comment and dispatch
triggers. Use `cancel-in-progress: false` and `queue: max` so running
and pending review requests are retained. Other PRs and the runtime
canary remain independent. The PR head and review context are resolved
after the queued job starts.
If the duplicate guard still rejects a candidate, save its reason and
complete proposed submission separately from the POST intent, propagate
that reason to the runner, and upload only that JSON as a 14-day
artifact. The candidate remains unsubmitted and cannot prove completion.
This is scoped to the workflow, submission helper and regression tests.
Existing duplicate protection, exclusive POST intent, invocation marker,
full GitHub readback and P0/P1 verdict logic are preserved.
---
.github/scripts/submit_review.py | 10 +++-
.github/scripts/test_submit_review.py | 85 ++++++++++++++++++++++++++++++++
.github/workflows/code-review-runner.yml | 23 ++++++++-
3 files changed, 115 insertions(+), 3 deletions(-)
diff --git a/.github/scripts/submit_review.py b/.github/scripts/submit_review.py
index 6db8997b3b9..358c05a95d4 100644
--- a/.github/scripts/submit_review.py
+++ b/.github/scripts/submit_review.py
@@ -29,6 +29,7 @@ from pathlib import Path
SUBMISSION_FILE = "review-submission.json"
+REJECTION_FILE = "review-submission-rejected.json"
RUN_FILE = "review-run.json"
RESULT_FILE = "review-result.json"
PRIORITY = re.compile(r"^\[P([0-3])\]\s+\S")
@@ -163,6 +164,9 @@ def verify_completion(context, run, remaining, *,
submit=False, submission=None)
path = context / SUBMISSION_FILE
if submission is None:
if not path.exists():
+ rejection = context / REJECTION_FILE
+ if rejection.exists():
+ raise ValueError(json.loads(rejection.read_text())["reason"])
raise ValueError("No final review submission was declared")
submission = json.loads(path.read_text())
submission = validate_submission(submission)
@@ -176,7 +180,11 @@ def verify_completion(context, run, remaining, *,
submit=False, submission=None)
if any(r.get("user", {}).get("login") == "github-actions[bot]"
and r.get("commit_id") == run["head_sha"]
and (r.get("submitted_at") or "") >= run["started_at"] for r in
reviews):
- raise ValueError("Another bot review was submitted during this
run; refusing duplicate submission")
+ reason = "Another bot review was submitted during this run;
refusing duplicate submission"
+ # This is a rejected candidate, not a POST intent or proof of
delivery.
+ # Keep it separate so completion still requires exact GitHub
readback.
+ write_json(context / REJECTION_FILE, {"reason": reason,
"submission": submission})
+ raise ValueError(reason)
if path.exists():
raise ValueError("Final submission was attempted but cannot be
verified; refusing to repost")
# Persist the complete intent before POST: if the response is lost, the
diff --git a/.github/scripts/test_submit_review.py
b/.github/scripts/test_submit_review.py
index 50dbfed228d..546bfecede6 100644
--- a/.github/scripts/test_submit_review.py
+++ b/.github/scripts/test_submit_review.py
@@ -20,6 +20,7 @@ import copy
import io
import itertools
import json
+import re
import subprocess
import tempfile
import threading
@@ -212,6 +213,70 @@ class FinalReviewTest(unittest.TestCase):
self.submit()
self.assertEqual(1, len(self.posts))
+ def test_rejected_submission_preserves_distinct_findings_and_reason(self):
+ self.reviews = [{"id": 98, "user": {"login": "github-actions[bot]"},
+ "commit_id": self.run["head_sha"],
+ "submitted_at": self.run["started_at"], "body": "A
different finding"}]
+ for level in (1, 2):
+ with self.subTest(priority=level):
+ submission = self.submission(level)
+ with self.assertRaisesRegex(ValueError, "Another bot review")
as error:
+ self.submit(submission)
+ rejected = json.loads((self.context /
submitter.REJECTION_FILE).read_text())
+ self.assertEqual(submission, rejected["submission"])
+ self.assertEqual(str(error.exception), rejected["reason"])
+ with self.assertRaisesRegex(ValueError, "Another bot review"):
+ self.verify()
+ self.assertEqual([], self.posts)
+ self.assertFalse((self.context / submitter.SUBMISSION_FILE).exists())
+
+ def test_review_before_start_does_not_block_next_run(self):
+ self.reviews = [{"id": 98, "user": {"login": "github-actions[bot]"},
+ "commit_id": self.run["head_sha"],
+ "submitted_at": "2026-09-28T23:59:59Z", "body":
"Earlier review"}]
+ self.assertEqual("failure", self.submit()["state"])
+ self.assertEqual(1, len(self.posts))
+ self.assertFalse((self.context / submitter.REJECTION_FILE).exists())
+
+ def test_verified_submission_takes_precedence_over_rejection(self):
+ submitter.write_json(self.context / submitter.REJECTION_FILE, {
+ "reason": "Earlier rejected candidate", "submission":
self.submission(2),
+ })
+ result = self.submit()
+ self.assertEqual(result, self.verify())
+ self.assertEqual(1, len(self.posts))
+
+ def test_runner_reports_rejection_instead_of_missing_submission(self):
+ (self.context / "codex_goal_prompt.txt").write_text("Review the PR")
+ args = SimpleNamespace(context_dir=self.context, cwd=self.context, **{
+ k: self.run[k] for k in ("repository", "pr_number", "head_sha",
"base_sha")
+ }, model="gpt-6-sol", effort="xhigh", budget_seconds=60)
+
+ def attempt(command, events, stderr, timeout, reaper=None):
+ run = json.loads((self.context / submitter.RUN_FILE).read_text())
+ self.reviews = [{"user": {"login": "github-actions[bot]"},
+ "commit_id": run["head_sha"], "submitted_at":
run["started_at"]}]
+ with self.assertRaisesRegex(ValueError, "Another bot review"):
+ submitter.verify_completion(self.context, run, self.remaining,
submit=True,
+ submission=self.submission())
+ # The model can finish normally after the helper rejects its
candidate.
+ events.write_text(json.dumps({"type": "thread.started",
"thread_id":
+ "0199a213-81c0-7800-8aa1-bbab2a035a53"}) + "\n" +
+ json.dumps({"type": "turn.completed"}) + "\n")
+ stderr.write_text("")
+ return 0
+
+ with mock.patch.object(runner, "run_attempt", side_effect=attempt), \
+ mock.patch.object(runner, "check_resume_target") as guard, \
+ redirect_stderr(io.StringIO()):
+ self.assertEqual(1, runner.run_review(args))
+ guard.assert_not_called()
+ error = runner.read_events(self.context /
"codex-events.jsonl")[-1]["error"]["message"]
+ self.assertIn("Another bot review", error)
+ self.assertNotIn("No final review submission", error)
+ self.assertEqual([], self.posts)
+ self.assertFalse((self.context / submitter.RESULT_FILE).exists())
+
def test_simultaneous_helper_invocations_post_only_once(self):
barrier = threading.Barrier(2, timeout=5)
original_open = Path.open
@@ -376,6 +441,26 @@ class FinalReviewTest(unittest.TestCase):
self.assertEqual([], self.posts)
+class ReviewWorkflowTest(unittest.TestCase):
+ def test_queue_is_per_pr_and_rejected_artifact_is_explicit(self):
+ workflow = (Path(__file__).resolve().parents[1] /
"workflows/code-review-runner.yml").read_text()
+ review_job = workflow.split(" code-review:\n", 1)[1].split("
aggregate-status:", 1)[0]
+ concurrency = re.search(r"^ concurrency:\n((?: .*\n)+)",
review_job, re.MULTILINE)
+ self.assertIsNotNone(concurrency)
+ self.assertIn("github.event.issue.number || inputs.pr_number ||
github.run_id", concurrency[1])
+ self.assertIn("cancel-in-progress: false", concurrency[1])
+ self.assertIn("queue: max", concurrency[1])
+ self.assertNotIn("sha", concurrency[1])
+ self.assertNotRegex(workflow, r"(?m)^concurrency:")
+ artifact = review_job.split(" - name: Preserve rejected final
review\n", 1)[1]
+ artifact = artifact.split(" - name:", 1)[0]
+ self.assertIn("always()", artifact)
+ self.assertIn("steps.review.outcome == 'failure'", artifact)
+ self.assertIn("path: ${{ env.REVIEW_CONTEXT_DIR }}/" +
submitter.REJECTION_FILE, artifact)
+ self.assertIn("include-hidden-files: true", artifact)
+ self.assertIn("if-no-files-found: ignore", artifact)
+
+
class GitHubAPITest(unittest.TestCase):
def test_read_all_pages_and_bound_timeout(self):
with mock.patch.object(submitter.subprocess, "run",
return_value=SimpleNamespace(
diff --git a/.github/workflows/code-review-runner.yml
b/.github/workflows/code-review-runner.yml
index fc227900abc..e5626ce624b 100644
--- a/.github/workflows/code-review-runner.yml
+++ b/.github/workflows/code-review-runner.yml
@@ -201,14 +201,20 @@ jobs:
code-review:
runs-on: ubuntu-24.04
+ # Serialize actual reviews of a PR, including comment and dispatch
triggers.
+ # Keep every pending request rather than replacing the previous queued job.
+ concurrency:
+ group: code-review-pr-${{ github.event.issue.number || inputs.pr_number
|| github.run_id }}
+ cancel-in-progress: false
+ queue: max
outputs:
base_sha: ${{ steps.review_inputs.outputs.base_sha }}
head_sha: ${{ steps.review_inputs.outputs.head_sha }}
manage_status: ${{ steps.review_inputs.outputs.manage_status }}
review_state: ${{ steps.review.outputs.review_state }}
pr_number: ${{ steps.review_inputs.outputs.pr_number }}
- # Pre-finalization steps can use 218 minutes and auth sync can use 8 more,
- # leaving 12 minutes for runner setup and post-job cleanup.
+ # Pre-finalization steps can use 220 minutes and auth sync can use 8 more,
+ # leaving 10 minutes for runner setup and post-job cleanup.
timeout-minutes: 238
if: >-
inputs.runtime_canary != true &&
@@ -973,6 +979,19 @@ jobs:
"review_state=\(.state)\np0=\(.p0)\np1=\(.p1)\nconverged=\(.converged)\nrounds=\(.rounds)"'
\
"$REVIEW_CONTEXT_DIR/review-result.json" >> "$GITHUB_OUTPUT"
+ - name: Preserve rejected final review
+ if: ${{ always() && steps.review_context.outcome == 'success' &&
steps.review.outcome == 'failure' }}
+ continue-on-error: true
+ timeout-minutes: 2
+ uses: actions/upload-artifact@v4
+ with:
+ name: rejected-code-review-${{ github.run_id }}-${{
github.run_attempt }}
+ path: ${{ env.REVIEW_CONTEXT_DIR }}/review-submission-rejected.json
+ # The context directory is hidden; include only the explicit JSON
above.
+ include-hidden-files: true
+ if-no-files-found: ignore
+ retention-days: 14
+
- name: Record invalid Codex auth
id: auth_invalid_record
if: ${{ always() && steps.auth.outcome == 'success' &&
steps.review.outputs.auth_invalid_reason == 'refresh_token_reused' }}
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
