[CI] Improve comment-triggered authorization and retries (#50414)
Signed-off-by: khluu <khluu000@gmail.com> Co-authored-by: OpenAI Codex <noreply@openai.com>
This commit is contained in:
co-authored by
OpenAI Codex
parent
e04a30a77c
commit
48a077e4cf
@@ -12,6 +12,7 @@ from typing import Any
|
||||
|
||||
COMMAND_RUN_CI = "/ci run"
|
||||
COMMAND_RETRY_FAILED = "/ci retry"
|
||||
CI_AUTHORIZED_COMMENT_MARKER = "<!-- vllm-ci-authorized -->"
|
||||
READY_LABELS = {"ready", "ready-run-all-tests"}
|
||||
TRUSTED_PERMISSIONS = {"admin", "maintain", "write"}
|
||||
ACTIVE_BUILD_STATES = {
|
||||
@@ -175,6 +176,9 @@ class GitHubClient:
|
||||
def list_reviews(self, number: int) -> list[dict[str, Any]]:
|
||||
return self._paginate(self._repo_path(f"/pulls/{number}/reviews"))
|
||||
|
||||
def list_issue_comments(self, number: int) -> list[dict[str, Any]]:
|
||||
return self._paginate(self._repo_path(f"/issues/{number}/comments"))
|
||||
|
||||
def list_reactions(self, comment_id: int) -> list[dict[str, Any]]:
|
||||
return self._paginate(
|
||||
self._repo_path(f"/issues/comments/{comment_id}/reactions")
|
||||
@@ -484,12 +488,90 @@ def add_reaction_safely(
|
||||
print(f"Could not add {content} reaction: {error}", file=sys.stderr)
|
||||
|
||||
|
||||
def is_already_handled(github: GitHubClient, comment_id: int) -> bool:
|
||||
def command_comment_marker(comment_id: int) -> str:
|
||||
return f"<!-- vllm-ci-command:{comment_id} -->"
|
||||
|
||||
|
||||
def has_bot_comment_marker(
|
||||
github: GitHubClient,
|
||||
issue_number: int,
|
||||
marker: str,
|
||||
) -> bool:
|
||||
return any(
|
||||
marker in str(comment.get("body", ""))
|
||||
and (comment.get("user") or {}).get("login") == "github-actions[bot]"
|
||||
for comment in github.list_issue_comments(issue_number)
|
||||
)
|
||||
|
||||
|
||||
def is_already_handled(
|
||||
github: GitHubClient,
|
||||
issue_number: int,
|
||||
comment_id: int,
|
||||
) -> bool:
|
||||
terminal_reaction = any(
|
||||
reaction.get("content") in {"rocket", "-1"}
|
||||
and (reaction.get("user") or {}).get("login") == "github-actions[bot]"
|
||||
for reaction in github.list_reactions(comment_id)
|
||||
)
|
||||
if terminal_reaction:
|
||||
return True
|
||||
return has_bot_comment_marker(
|
||||
github,
|
||||
issue_number,
|
||||
command_comment_marker(comment_id),
|
||||
)
|
||||
|
||||
|
||||
def notify_authorized(
|
||||
event: Mapping[str, Any],
|
||||
github: GitHubClient,
|
||||
trusted_users_value: str = "",
|
||||
) -> None:
|
||||
pr = event["pull_request"]
|
||||
if pr["state"] != "open" or pr["draft"]:
|
||||
return
|
||||
|
||||
trusted_users = parse_trusted_users(trusted_users_value)
|
||||
author = pr["user"]["login"]
|
||||
author_permission = github.get_permission(author)
|
||||
if (
|
||||
is_trusted_permission(author_permission)
|
||||
or author.casefold() in trusted_users
|
||||
or has_bot_comment_marker(
|
||||
github,
|
||||
pr["number"],
|
||||
CI_AUTHORIZED_COMMENT_MARKER,
|
||||
)
|
||||
):
|
||||
return
|
||||
|
||||
if "label" in event:
|
||||
if (
|
||||
event.get("action") != "labeled"
|
||||
or event["label"]["name"] not in READY_LABELS
|
||||
or has_trusted_approval(github, pr["number"], trusted_users)
|
||||
):
|
||||
return
|
||||
elif "review" in event:
|
||||
if (
|
||||
event.get("action") != "submitted"
|
||||
or str(event["review"].get("state", "")).casefold() != "approved"
|
||||
or has_ready_label(pr)
|
||||
or not has_trusted_approval(github, pr["number"], trusted_users)
|
||||
):
|
||||
return
|
||||
else:
|
||||
return
|
||||
|
||||
github.add_comment(
|
||||
pr["number"],
|
||||
(
|
||||
f"✅ @{author}, CI is now available for this PR. Comment `/ci run` "
|
||||
"to run full CI or `/ci retry` to retry failed jobs.\n\n"
|
||||
f"{CI_AUTHORIZED_COMMENT_MARKER}"
|
||||
),
|
||||
)
|
||||
|
||||
|
||||
def handle_run_ci(
|
||||
@@ -553,8 +635,6 @@ def handle_retry_failed(
|
||||
metadata = build.get("meta_data") or {}
|
||||
if str(metadata.get("github-comment-id")) == str(comment_id):
|
||||
return f"CI was already requested by this comment: {build['web_url']}"
|
||||
if not build.get("finished_at") or is_active_build(build):
|
||||
return f"CI is still running for this commit: {build['web_url']}"
|
||||
|
||||
retried = buildkite.retry_failed_jobs(build["number"], RETRY_STATES)
|
||||
if retried["retried_jobs_count"] == 0:
|
||||
@@ -652,7 +732,7 @@ def run(
|
||||
comment_id = event["comment"]["id"]
|
||||
actor = event["comment"]["user"]["login"]
|
||||
|
||||
if is_already_handled(github, comment_id):
|
||||
if is_already_handled(github, issue_number, comment_id):
|
||||
print(f"Comment {comment_id} was already handled.")
|
||||
return
|
||||
add_reaction_safely(github, comment_id, "eyes")
|
||||
@@ -661,7 +741,11 @@ def run(
|
||||
pr = github.get_pr(issue_number)
|
||||
permission = github.get_permission(actor)
|
||||
if pr["state"] != "open":
|
||||
github.add_comment(issue_number, "CI commands require an open PR.")
|
||||
github.add_comment(
|
||||
issue_number,
|
||||
"❌ CI commands require an open PR.\n\n"
|
||||
f"{command_comment_marker(comment_id)}",
|
||||
)
|
||||
return
|
||||
|
||||
trusted_users = parse_trusted_users(trusted_users_value)
|
||||
@@ -685,8 +769,10 @@ def run(
|
||||
trusted_users=trusted_users,
|
||||
)
|
||||
if not allowed:
|
||||
add_reaction_safely(github, comment_id, "-1")
|
||||
github.add_comment(issue_number, f"@{actor}, {reason}")
|
||||
github.add_comment(
|
||||
issue_number,
|
||||
f"❌ @{actor}, {reason}\n\n{command_comment_marker(comment_id)}",
|
||||
)
|
||||
return
|
||||
|
||||
print(f"Authorized @{actor}: {reason}")
|
||||
@@ -707,7 +793,7 @@ def run(
|
||||
pr=pr,
|
||||
)
|
||||
add_reaction_safely(github, comment_id, "rocket")
|
||||
github.add_comment(issue_number, message)
|
||||
github.add_comment(issue_number, f"✅ {message}")
|
||||
except Exception:
|
||||
add_reaction_safely(github, comment_id, "confused")
|
||||
raise
|
||||
@@ -718,13 +804,37 @@ def main() -> None:
|
||||
with open(event_path, encoding="utf-8") as event_file:
|
||||
event = json.load(event_file)
|
||||
|
||||
if not parse_command(event["comment"]["body"]):
|
||||
return
|
||||
|
||||
github = GitHubClient(
|
||||
os.environ.get("GH_TOKEN", ""),
|
||||
os.environ["GITHUB_REPOSITORY"],
|
||||
)
|
||||
event_name = os.environ.get("GITHUB_EVENT_NAME", "issue_comment")
|
||||
if event_name == "pull_request_target":
|
||||
notify_authorized(
|
||||
event,
|
||||
github,
|
||||
os.environ.get("CI_TRUSTED_USERS", ""),
|
||||
)
|
||||
return
|
||||
if event_name == "workflow_run":
|
||||
pull_requests = event["workflow_run"].get("pull_requests") or []
|
||||
if not pull_requests:
|
||||
return
|
||||
pr = github.get_pr(int(pull_requests[0]["number"]))
|
||||
notify_authorized(
|
||||
{
|
||||
"action": "submitted",
|
||||
"pull_request": pr,
|
||||
"review": {"state": "approved"},
|
||||
},
|
||||
github,
|
||||
os.environ.get("CI_TRUSTED_USERS", ""),
|
||||
)
|
||||
return
|
||||
|
||||
if not parse_command(event["comment"]["body"]):
|
||||
return
|
||||
|
||||
buildkite = BuildkiteClient(
|
||||
os.environ.get("BUILDKITE_API_TOKEN", ""),
|
||||
os.environ.get("BUILDKITE_ORGANIZATION", "vllm"),
|
||||
|
||||
@@ -6,6 +6,7 @@ import unittest
|
||||
from typing import Any
|
||||
|
||||
from run_ci_command import (
|
||||
CI_AUTHORIZED_COMMENT_MARKER,
|
||||
COMMAND_RETRY_FAILED,
|
||||
COMMAND_RUN_CI,
|
||||
RETRY_STATES,
|
||||
@@ -15,6 +16,7 @@ from run_ci_command import (
|
||||
has_trusted_approval,
|
||||
is_active_build,
|
||||
is_build_for_pr,
|
||||
notify_authorized,
|
||||
parse_command,
|
||||
parse_trusted_users,
|
||||
run,
|
||||
@@ -63,8 +65,9 @@ class FakeGitHub:
|
||||
pr: dict[str, Any] | None = None,
|
||||
review_decision: str = "REVIEW_REQUIRED",
|
||||
reviews: list[dict[str, Any]] | None = None,
|
||||
comments: list[str] | None = None,
|
||||
) -> None:
|
||||
self.comments: list[str] = []
|
||||
self.comments = comments or []
|
||||
self.permission = permission
|
||||
self.permissions = permissions or {}
|
||||
self.pr = pr or make_pr()
|
||||
@@ -84,6 +87,15 @@ class FakeGitHub:
|
||||
def list_reviews(self, number: int) -> list[dict[str, Any]]:
|
||||
return self.reviews
|
||||
|
||||
def list_issue_comments(self, number: int) -> list[dict[str, Any]]:
|
||||
return [
|
||||
{
|
||||
"body": body,
|
||||
"user": {"login": "github-actions[bot]"},
|
||||
}
|
||||
for body in self.comments
|
||||
]
|
||||
|
||||
def list_reactions(self, comment_id: int) -> list[dict[str, Any]]:
|
||||
return []
|
||||
|
||||
@@ -303,7 +315,7 @@ class RunCiCommandTest(unittest.TestCase):
|
||||
},
|
||||
)
|
||||
|
||||
def test_ci_run_dispatches_build_with_current_pr_metadata(self) -> None:
|
||||
def test_write_reviewer_runs_ci_without_delegation(self) -> None:
|
||||
github = FakeGitHub()
|
||||
buildkite = FakeBuildkite([[], []])
|
||||
run(make_event(COMMAND_RUN_CI), github, buildkite)
|
||||
@@ -314,6 +326,7 @@ class RunCiCommandTest(unittest.TestCase):
|
||||
"PR #42 /ci run by @reviewer",
|
||||
)
|
||||
self.assertEqual(github.reactions, ["eyes", "rocket"])
|
||||
self.assertTrue(github.comments[0].startswith("✅ "))
|
||||
self.assertIn("Buildkite CI #123", github.comments[0])
|
||||
|
||||
def test_unapproved_authors_are_denied_without_buildkite(self) -> None:
|
||||
@@ -326,10 +339,171 @@ class RunCiCommandTest(unittest.TestCase):
|
||||
run(make_event(COMMAND_RUN_CI, "author"), github, buildkite)
|
||||
|
||||
self.assertEqual(buildkite.list_calls, [])
|
||||
self.assertEqual(github.reactions, ["eyes", "-1"])
|
||||
self.assertEqual(github.reactions, ["eyes"])
|
||||
self.assertTrue(github.comments[0].startswith("❌ "))
|
||||
self.assertIn("approve the PR", github.comments[0])
|
||||
|
||||
def test_ci_retry_uses_latest_current_sha_build(self) -> None:
|
||||
run(make_event(COMMAND_RUN_CI, "author"), github, buildkite)
|
||||
self.assertEqual(len(github.comments), 1)
|
||||
|
||||
def test_untrusted_approval_cannot_launch_ci(self) -> None:
|
||||
github = FakeGitHub(
|
||||
permission="read",
|
||||
pr=make_pr(),
|
||||
review_decision="APPROVED",
|
||||
reviews=[
|
||||
{
|
||||
"state": "APPROVED",
|
||||
"user": {"login": "untrusted-reviewer"},
|
||||
}
|
||||
],
|
||||
)
|
||||
buildkite = FakeBuildkite()
|
||||
|
||||
run(make_event(COMMAND_RUN_CI, "author"), github, buildkite)
|
||||
|
||||
self.assertEqual(buildkite.list_calls, [])
|
||||
self.assertEqual(github.reactions, ["eyes"])
|
||||
self.assertTrue(github.comments[0].startswith("❌ "))
|
||||
|
||||
def test_ready_label_notifies_author_once(self) -> None:
|
||||
pr = make_pr(labels=[{"name": "ready"}])
|
||||
event = {
|
||||
"action": "labeled",
|
||||
"label": {"name": "ready"},
|
||||
"pull_request": pr,
|
||||
}
|
||||
github = FakeGitHub(permission="read", pr=pr)
|
||||
|
||||
notify_authorized(event, github)
|
||||
notify_authorized(event, github)
|
||||
|
||||
self.assertEqual(len(github.comments), 1)
|
||||
self.assertTrue(github.comments[0].startswith("✅ @author"))
|
||||
self.assertIn("`/ci run`", github.comments[0])
|
||||
self.assertIn("`/ci retry`", github.comments[0])
|
||||
self.assertIn(CI_AUTHORIZED_COMMENT_MARKER, github.comments[0])
|
||||
|
||||
def test_ready_label_does_not_notify_after_trusted_approval(self) -> None:
|
||||
pr = make_pr(labels=[{"name": "ready"}])
|
||||
event = {
|
||||
"action": "labeled",
|
||||
"label": {"name": "ready"},
|
||||
"pull_request": pr,
|
||||
}
|
||||
github = FakeGitHub(
|
||||
permission="read",
|
||||
permissions={"reviewer": "write"},
|
||||
pr=pr,
|
||||
review_decision="APPROVED",
|
||||
reviews=[
|
||||
{
|
||||
"state": "APPROVED",
|
||||
"user": {"login": "reviewer"},
|
||||
}
|
||||
],
|
||||
)
|
||||
|
||||
notify_authorized(event, github)
|
||||
|
||||
self.assertEqual(github.comments, [])
|
||||
|
||||
def test_trusted_approval_notifies_author_once(self) -> None:
|
||||
event = {
|
||||
"action": "submitted",
|
||||
"pull_request": make_pr(),
|
||||
"review": {"state": "approved"},
|
||||
}
|
||||
github = FakeGitHub(
|
||||
permission="read",
|
||||
permissions={"reviewer": "write"},
|
||||
review_decision="APPROVED",
|
||||
reviews=[
|
||||
{
|
||||
"state": "APPROVED",
|
||||
"user": {"login": "reviewer"},
|
||||
}
|
||||
],
|
||||
)
|
||||
|
||||
notify_authorized(event, github)
|
||||
notify_authorized(event, github)
|
||||
|
||||
self.assertEqual(len(github.comments), 1)
|
||||
self.assertIn("@author", github.comments[0])
|
||||
|
||||
def test_approval_does_not_notify_when_ready_label_exists(self) -> None:
|
||||
pr = make_pr(labels=[{"name": "ready"}])
|
||||
event = {
|
||||
"action": "submitted",
|
||||
"pull_request": pr,
|
||||
"review": {"state": "approved"},
|
||||
}
|
||||
github = FakeGitHub(
|
||||
permission="read",
|
||||
permissions={"reviewer": "write"},
|
||||
pr=pr,
|
||||
review_decision="APPROVED",
|
||||
reviews=[
|
||||
{
|
||||
"state": "APPROVED",
|
||||
"user": {"login": "reviewer"},
|
||||
}
|
||||
],
|
||||
)
|
||||
|
||||
notify_authorized(event, github)
|
||||
|
||||
self.assertEqual(github.comments, [])
|
||||
|
||||
def test_untrusted_approval_does_not_notify_author(self) -> None:
|
||||
event = {
|
||||
"action": "submitted",
|
||||
"pull_request": make_pr(),
|
||||
"review": {"state": "approved"},
|
||||
}
|
||||
github = FakeGitHub(
|
||||
permission="read",
|
||||
review_decision="APPROVED",
|
||||
reviews=[
|
||||
{
|
||||
"state": "APPROVED",
|
||||
"user": {"login": "reviewer"},
|
||||
}
|
||||
],
|
||||
)
|
||||
|
||||
notify_authorized(event, github)
|
||||
|
||||
self.assertEqual(github.comments, [])
|
||||
|
||||
def test_notification_skips_authors_who_already_have_write(self) -> None:
|
||||
pr = make_pr(labels=[{"name": "ready"}])
|
||||
event = {
|
||||
"action": "labeled",
|
||||
"label": {"name": "ready"},
|
||||
"pull_request": pr,
|
||||
}
|
||||
github = FakeGitHub(permission="write", pr=pr)
|
||||
|
||||
notify_authorized(event, github)
|
||||
|
||||
self.assertEqual(github.comments, [])
|
||||
|
||||
def test_notification_skips_draft_prs(self) -> None:
|
||||
pr = make_pr(draft=True, labels=[{"name": "ready"}])
|
||||
event = {
|
||||
"action": "labeled",
|
||||
"label": {"name": "ready"},
|
||||
"pull_request": pr,
|
||||
}
|
||||
github = FakeGitHub(permission="read", pr=pr)
|
||||
|
||||
notify_authorized(event, github)
|
||||
|
||||
self.assertEqual(github.comments, [])
|
||||
|
||||
def test_ci_retry_retries_failed_jobs_while_build_is_running(self) -> None:
|
||||
github = FakeGitHub(
|
||||
permission="read",
|
||||
pr=make_pr(labels=[{"name": "ready"}]),
|
||||
@@ -339,10 +513,9 @@ class RunCiCommandTest(unittest.TestCase):
|
||||
[
|
||||
{
|
||||
"created_at": "2026-07-28T01:00:00Z",
|
||||
"finished_at": "2026-07-28T02:00:00Z",
|
||||
"number": 123,
|
||||
"pull_request": {"id": 42},
|
||||
"state": "failed",
|
||||
"state": "failing",
|
||||
"web_url": "https://buildkite.example/builds/123",
|
||||
}
|
||||
]
|
||||
|
||||
Reference in New Issue
Block a user