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 6ca5ef36d8f [fix](ci) Accept bold priorities in existing review
comments (#69377)
6ca5ef36d8f is described below
commit 6ca5ef36d8f968e6e56392f586453a7ecbc6b250
Author: shuke <[email protected]>
AuthorDate: Wed Oct 21 19:36:32 2026 +0800
[fix](ci) Accept bold priorities in existing review comments (#69377)
### What problem does this PR solve?
Issue Number: None
Related PR: #67649
When an automated review reconfirms an existing blocker whose heading
starts with `**[P1] ...**` or `**[P1]** ...`, `submit_review.py` rejects
the historical comment before posting the final review. This caused [run
39532829872](https://github.com/apache/doris/actions/runs/39532829872)
and [run
39535990055](https://github.com/apache/doris/actions/runs/39535990055)
to finish without delivering a review.
Normalize the leading bold priority markup only when counting explicitly
reconfirmed existing comments. New findings retain their strict format.
Existing P0/P1 findings still request changes without duplicate inline
comments, and existing P2/P3 findings remain invalid as blockers.
### Release note
None
### Check List (For Author)
- Test:
- [x] Unit Test: `test_submit_review.py` — 40 passed;
`test_run_review_with_resume.py` — 78 passed, 3 Linux-only tests skipped
on macOS; `test_resolve_code_review_status.py` — 20 passed.
- [x] Manual test: the new bold-comment cases fail against the original
implementation. `git diff --check` passes.
- Coverage includes P0/P1 counts, exact readback, repeated submission
without reposting, rejection of P2/P3 and malformed or reply comments,
and strict validation of new findings. GitHub API calls are mocked.
- Behavior changed:
- [x] Yes. Existing review comments with bold priority headings can be
reconfirmed and delivered.
- 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
---
.github/scripts/submit_review.py | 5 +++-
.github/scripts/test_submit_review.py | 48 ++++++++++++++++++++++++++++++++++-
2 files changed, 51 insertions(+), 2 deletions(-)
diff --git a/.github/scripts/submit_review.py b/.github/scripts/submit_review.py
index 036d5387c71..dc48b1b8ed3 100644
--- a/.github/scripts/submit_review.py
+++ b/.github/scripts/submit_review.py
@@ -179,7 +179,10 @@ def review_payload(submission, run, root, remaining):
comment = comments.get(comment_id)
if comment is None or comment.get("in_reply_to_id") is not None:
raise ValueError(f"Existing blocking finding {comment_id} is
missing or a reply")
- level = priority(comment["body"])
+ # Older reviews may bold the whole heading or only the priority.
+ # Normalize existing comments only; new findings keep their strict
format.
+ body = re.sub(r"^\*\*(\[P[0-3]\])(?:\*\*)?", r"\1",
comment["body"], count=1)
+ level = priority(body)
if level > 1:
raise ValueError(f"Existing finding {comment_id} is not P0/P1")
counts[level] += 1
diff --git a/.github/scripts/test_submit_review.py
b/.github/scripts/test_submit_review.py
index 42735555b29..2d1bdc4fb27 100644
--- a/.github/scripts/test_submit_review.py
+++ b/.github/scripts/test_submit_review.py
@@ -300,11 +300,57 @@ class FinalReviewTest(unittest.TestCase):
self.assertEqual([], self.posts[0]["comments"])
self.assertIn("#discussion_r101", self.posts[0]["body"])
+ def test_existing_bold_blockers_are_counted_without_duplicate_inline(self):
+ formats = ("**[P{level}] Previously reported blocker**",
+ "**[P{level}]** Previously reported blocker")
+ for level, template in itertools.product((0, 1), formats):
+ body = template.format(level=level) + "\n\nThe issue still
applies."
+ with self.subTest(body=body), tempfile.TemporaryDirectory() as
tmp, \
+ mock.patch.object(self, "context", Path(tmp)):
+ self.reviews.clear()
+ self.posts.clear()
+ self.existing = [{"id": 101, "body": body}]
+ submission = self.submission(None)
+ submission["existing_blocking_comment_ids"] = [101]
+ result = self.submit(submission)
+ self.assertEqual((int(level == 0), int(level == 1), "failure"),
+ (result["p0"], result["p1"], result["state"]))
+ self.assertEqual(result, self.submit(submission))
+ self.assertEqual(result, self.verify())
+ self.assertEqual(1, len(self.posts))
+ self.assertEqual("REQUEST_CHANGES", self.posts[0]["event"])
+ self.assertEqual([], self.posts[0]["comments"])
+ self.assertIn("#discussion_r101", self.posts[0]["body"])
+
+ def test_bold_non_blocking_comments_cannot_be_declared_as_blockers(self):
+ submission = self.submission(None)
+ submission["existing_blocking_comment_ids"] = [101]
+ for level, template in itertools.product(
+ (2, 3), ("**[P{level}] Minor**", "**[P{level}]** Minor")):
+ body = template.format(level=level)
+ with self.subTest(body=body), self.assertRaisesRegex(ValueError,
"not P0/P1"):
+ self.existing = [{"id": 101, "body": body}]
+ self.submit(submission)
+ self.assertEqual([], self.posts)
+ self.assertFalse((self.context / submitter.SUBMISSION_FILE).exists())
+
+ def test_new_findings_still_require_plain_priority(self):
+ for body in ("**[P1] New finding**", "**[P1]** New finding"):
+ submission = self.submission()
+ submission["comments"][0]["body"] = body
+ with self.subTest(body=body), self.assertRaisesRegex(ValueError,
"must start with"):
+ self.submit(submission)
+ self.api.assert_not_called()
+
def test_invalid_existing_blocker_cannot_silently_pass(self):
submission = self.submission(None)
submission["existing_blocking_comment_ids"] = [101]
for comments in ([], [{"id": 101, "body": "[P2] Minor"}],
- [{"id": 101, "body": "[P1] Reply", "in_reply_to_id":
1}]):
+ [{"id": 101, "body": "**[P4] Unknown priority**"}],
+ [{"id": 101, "body": "**[P1]**"}],
+ [{"id": 101, "body": "Text mentioning **[P1]**
later"}],
+ [{"id": 101, "body": "[P1] Reply", "in_reply_to_id":
1}],
+ [{"id": 101, "body": "**[P1] Reply**",
"in_reply_to_id": 1}]):
with self.subTest(comments=comments),
self.assertRaises(ValueError):
self.existing = comments
self.submit(submission)
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
