fix(ci): relay fork review hygiene (#5063)
Signed-off-by: Pat Sukprasert <pattara.sk127@gmail.com>
This commit is contained in:
@@ -21,6 +21,7 @@ REVIEW_LABEL = "waiting-for-review"
|
||||
WAITING_DAYS = 7
|
||||
CANONICAL_REPO = "omnigent-ai/omnigent"
|
||||
MAX_CLOSURES_PER_RUN = 30
|
||||
REVIEW_EVENTS = {"pull_request_review", "pull_request_review_comment"}
|
||||
|
||||
|
||||
def label_names(item: dict[str, Any]) -> list[str]:
|
||||
@@ -114,6 +115,16 @@ class GitHubAPI:
|
||||
pull, _ = self.request("GET", f"/repos/{self.repo}/pulls/{pull_number}")
|
||||
return pull
|
||||
|
||||
def get_review(self, pull_number: int, review_id: int) -> dict[str, Any]:
|
||||
review, _ = self.request(
|
||||
"GET", f"/repos/{self.repo}/pulls/{pull_number}/reviews/{review_id}"
|
||||
)
|
||||
return review
|
||||
|
||||
def get_review_comment(self, comment_id: int) -> dict[str, Any]:
|
||||
comment, _ = self.request("GET", f"/repos/{self.repo}/pulls/comments/{comment_id}")
|
||||
return comment
|
||||
|
||||
def remove_label(self, issue_number: int, label: str) -> bool:
|
||||
quoted = urllib.parse.quote(label, safe="")
|
||||
try:
|
||||
@@ -355,10 +366,14 @@ def apply_waiting_on_maintainer_activity(
|
||||
pull_number = payload["pull_request"]["number"]
|
||||
review = payload.get("review") or {}
|
||||
actor = (review.get("user") or {}).get("login")
|
||||
review_state = (review.get("state") or "").lower()
|
||||
# An approval asks nothing of the author; it means the PR is ready.
|
||||
if (review.get("state") or "").lower() == "approved":
|
||||
if review_state == "approved":
|
||||
print(f"#{pull_number}: approving review, leaving the label alone.")
|
||||
return False
|
||||
if review_state not in {"commented", "changes_requested"}:
|
||||
print(f"#{pull_number}: {review_state or 'unknown'} review, leaving the label alone.")
|
||||
return False
|
||||
if is_slash_command(review.get("body")):
|
||||
return False
|
||||
reason = "a maintainer reviewed"
|
||||
@@ -473,6 +488,42 @@ def close_stale_waiting_prs(api: GitHubAPI, now: datetime | None = None) -> int:
|
||||
return closed
|
||||
|
||||
|
||||
def relay_integer(record: dict[str, Any], field: str) -> int:
|
||||
value = record.get(field)
|
||||
if isinstance(value, bool) or not isinstance(value, int) or value <= 0:
|
||||
raise ValueError(f"Relay field {field!r} must be a positive integer")
|
||||
return value
|
||||
|
||||
|
||||
def hydrate_relay_event(
|
||||
record: dict[str, Any], api: GitHubAPI, repo: str, expected_event: str
|
||||
) -> tuple[str, dict[str, Any]]:
|
||||
event_name = record.get("event_name")
|
||||
if event_name not in REVIEW_EVENTS:
|
||||
raise ValueError(f"Unsupported relayed event: {event_name!r}")
|
||||
if event_name != expected_event:
|
||||
raise ValueError(
|
||||
f"Relayed event {event_name!r} does not match workflow event {expected_event!r}"
|
||||
)
|
||||
|
||||
pull_number = relay_integer(record, "pull_number")
|
||||
activity_id = relay_integer(record, "activity_id")
|
||||
pull = api.get_pull(pull_number)
|
||||
base_repo = ((pull.get("base") or {}).get("repo") or {}).get("full_name") or ""
|
||||
if base_repo.lower() != repo.lower():
|
||||
raise ValueError(f"Relayed PR #{pull_number} targets {base_repo!r}, not {repo!r}")
|
||||
|
||||
if event_name == "pull_request_review":
|
||||
review = api.get_review(pull_number, activity_id)
|
||||
return event_name, {"pull_request": pull, "review": review}
|
||||
|
||||
comment = api.get_review_comment(activity_id)
|
||||
expected_url = f"https://api.github.com/repos/{repo}/pulls/{pull_number}"
|
||||
if comment.get("pull_request_url") != expected_url:
|
||||
raise ValueError(f"Review comment {activity_id} does not belong to PR #{pull_number}")
|
||||
return event_name, {"pull_request": pull, "comment": comment}
|
||||
|
||||
|
||||
def run(
|
||||
event_name: str,
|
||||
payload: dict[str, Any],
|
||||
@@ -495,8 +546,7 @@ def run(
|
||||
apply_waiting_on_maintainer_activity(event_name, payload, api)
|
||||
|
||||
|
||||
def load_event_payload() -> dict[str, Any]:
|
||||
path = os.environ.get("GITHUB_EVENT_PATH")
|
||||
def load_json(path: str | None) -> dict[str, Any]:
|
||||
if not path:
|
||||
return {}
|
||||
with open(path, encoding="utf-8") as handle:
|
||||
@@ -509,8 +559,15 @@ def main() -> int:
|
||||
if not token:
|
||||
print("GITHUB_TOKEN is required", file=sys.stderr)
|
||||
return 1
|
||||
api = GitHubAPI(token, repo)
|
||||
relay_path = os.environ.get("WAITING_ON_AUTHOR_RELAY_PATH")
|
||||
if relay_path:
|
||||
expected_event = os.environ.get("WAITING_ON_AUTHOR_RELAY_EVENT", "")
|
||||
event_name, payload = hydrate_relay_event(load_json(relay_path), api, repo, expected_event)
|
||||
else:
|
||||
event_name = os.environ.get("GITHUB_EVENT_NAME", "")
|
||||
run(event_name, load_event_payload(), GitHubAPI(token, repo), repo)
|
||||
payload = load_json(os.environ.get("GITHUB_EVENT_PATH"))
|
||||
run(event_name, payload, api, repo)
|
||||
return 0
|
||||
|
||||
|
||||
|
||||
@@ -63,6 +63,8 @@ class FakeAPI:
|
||||
issue_comments: dict[int, list[dict[str, Any]]] | None = None,
|
||||
review_comments: dict[int, list[dict[str, Any]]] | None = None,
|
||||
reviews: dict[int, list[dict[str, Any]]] | None = None,
|
||||
review_by_id: dict[tuple[int, int], dict[str, Any]] | None = None,
|
||||
review_comment_by_id: dict[int, dict[str, Any]] | None = None,
|
||||
commits: dict[int, list[dict[str, Any]]] | None = None,
|
||||
writers: list[str] | None = None,
|
||||
):
|
||||
@@ -73,6 +75,8 @@ class FakeAPI:
|
||||
self.issue_comments = issue_comments or {}
|
||||
self.review_comments = review_comments or {}
|
||||
self.reviews = reviews or {}
|
||||
self.review_by_id = review_by_id or {}
|
||||
self.review_comment_by_id = review_comment_by_id or {}
|
||||
self.commits = commits or {}
|
||||
self.removed: list[tuple[int, str]] = []
|
||||
self.closed: list[int] = []
|
||||
@@ -83,6 +87,12 @@ class FakeAPI:
|
||||
def get_pull(self, pull_number: int) -> dict[str, Any]:
|
||||
return self.pull | {"number": pull_number}
|
||||
|
||||
def get_review(self, pull_number: int, review_id: int) -> dict[str, Any]:
|
||||
return self.review_by_id[(pull_number, review_id)]
|
||||
|
||||
def get_review_comment(self, comment_id: int) -> dict[str, Any]:
|
||||
return self.review_comment_by_id[comment_id]
|
||||
|
||||
def remove_label(self, issue_number: int, label: str) -> bool:
|
||||
self.removed.append((issue_number, label))
|
||||
return True
|
||||
@@ -442,6 +452,21 @@ class AutoWaitingOnAuthorTest(unittest.TestCase):
|
||||
)
|
||||
self.assertEqual(api.added, [])
|
||||
|
||||
def test_dismissed_review_leaves_the_label_alone(self) -> None:
|
||||
api = self.dispatch(
|
||||
"pull_request_review",
|
||||
{
|
||||
"pull_request": {"number": 12},
|
||||
"review": {
|
||||
"user": {"login": "maintainer1"},
|
||||
"state": "dismissed",
|
||||
"body": "stale feedback",
|
||||
},
|
||||
},
|
||||
pull=pr(labels=[]),
|
||||
)
|
||||
self.assertEqual(api.added, [])
|
||||
|
||||
def test_commenting_review_applies_the_label(self) -> None:
|
||||
api = self.dispatch(
|
||||
"pull_request_review",
|
||||
@@ -483,6 +508,99 @@ class AutoWaitingOnAuthorTest(unittest.TestCase):
|
||||
)
|
||||
self.assertEqual(api.added, [(12, waiting_on_author.LABEL)])
|
||||
|
||||
def test_relayed_review_rehydrates_trusted_api_data(self) -> None:
|
||||
pull = pr(labels=[]) | {"base": {"repo": {"full_name": waiting_on_author.CANONICAL_REPO}}}
|
||||
api = FakeAPI(
|
||||
pull=pull,
|
||||
review_by_id={
|
||||
(12, 41): {
|
||||
"id": 41,
|
||||
"user": {"login": "maintainer1"},
|
||||
"state": "changes_requested",
|
||||
"body": "please fix",
|
||||
}
|
||||
},
|
||||
)
|
||||
event, payload = waiting_on_author.hydrate_relay_event(
|
||||
{"event_name": "pull_request_review", "pull_number": 12, "activity_id": 41},
|
||||
api,
|
||||
waiting_on_author.CANONICAL_REPO,
|
||||
"pull_request_review",
|
||||
)
|
||||
|
||||
waiting_on_author.run(event, payload, api, waiting_on_author.CANONICAL_REPO)
|
||||
|
||||
self.assertEqual(api.added, [(12, waiting_on_author.LABEL)])
|
||||
|
||||
def test_relayed_review_comment_rehydrates_author_reply(self) -> None:
|
||||
pull = pr(author="alice") | {
|
||||
"base": {"repo": {"full_name": waiting_on_author.CANONICAL_REPO}}
|
||||
}
|
||||
api = FakeAPI(
|
||||
pull=pull,
|
||||
review_comment_by_id={
|
||||
73: {
|
||||
"id": 73,
|
||||
"user": {"login": "alice"},
|
||||
"body": "fixed",
|
||||
"pull_request_url": (
|
||||
"https://api.github.com/repos/omnigent-ai/omnigent/pulls/12"
|
||||
),
|
||||
}
|
||||
},
|
||||
)
|
||||
event, payload = waiting_on_author.hydrate_relay_event(
|
||||
{
|
||||
"event_name": "pull_request_review_comment",
|
||||
"pull_number": 12,
|
||||
"activity_id": 73,
|
||||
},
|
||||
api,
|
||||
waiting_on_author.CANONICAL_REPO,
|
||||
"pull_request_review_comment",
|
||||
)
|
||||
|
||||
waiting_on_author.run(event, payload, api, waiting_on_author.CANONICAL_REPO)
|
||||
|
||||
self.assertEqual(api.removed, [(12, waiting_on_author.LABEL)])
|
||||
|
||||
def test_relayed_review_comment_must_match_pull(self) -> None:
|
||||
pull = pr() | {"base": {"repo": {"full_name": waiting_on_author.CANONICAL_REPO}}}
|
||||
api = FakeAPI(
|
||||
pull=pull,
|
||||
review_comment_by_id={
|
||||
73: {
|
||||
"id": 73,
|
||||
"pull_request_url": (
|
||||
"https://api.github.com/repos/omnigent-ai/omnigent/pulls/99"
|
||||
),
|
||||
}
|
||||
},
|
||||
)
|
||||
|
||||
with self.assertRaisesRegex(ValueError, "does not belong"):
|
||||
waiting_on_author.hydrate_relay_event(
|
||||
{
|
||||
"event_name": "pull_request_review_comment",
|
||||
"pull_number": 12,
|
||||
"activity_id": 73,
|
||||
},
|
||||
api,
|
||||
waiting_on_author.CANONICAL_REPO,
|
||||
"pull_request_review_comment",
|
||||
)
|
||||
|
||||
def test_relayed_event_must_match_workflow_event(self) -> None:
|
||||
api = FakeAPI()
|
||||
|
||||
with self.assertRaisesRegex(ValueError, "does not match workflow event"):
|
||||
waiting_on_author.hydrate_relay_event(
|
||||
{"event_name": "pull_request_review", "pull_number": 12, "activity_id": 41},
|
||||
api,
|
||||
waiting_on_author.CANONICAL_REPO,
|
||||
"pull_request_review_comment",
|
||||
)
|
||||
|
||||
def test_applying_clears_waiting_for_review(self) -> None:
|
||||
api = self.dispatch(
|
||||
"issue_comment",
|
||||
|
||||
@@ -0,0 +1,78 @@
|
||||
name: Waiting on Author Review Run
|
||||
|
||||
# Privileged half of the fork-review relay. The artifact contains numeric IDs
|
||||
# only; the script re-fetches the PR and review/comment from GitHub before using
|
||||
# actor identity, review state, or comment content. No PR code is checked out.
|
||||
|
||||
on:
|
||||
workflow_run:
|
||||
workflows: [Waiting on Author Review]
|
||||
types: [completed]
|
||||
|
||||
permissions:
|
||||
contents: read
|
||||
|
||||
concurrency:
|
||||
group: waiting-on-author-review-run-${{ github.event.workflow_run.head_sha }}
|
||||
cancel-in-progress: false
|
||||
|
||||
jobs:
|
||||
hygiene:
|
||||
if: >-
|
||||
github.repository == 'omnigent-ai/omnigent'
|
||||
&& github.event.workflow_run.conclusion == 'success'
|
||||
runs-on: ubuntu-latest
|
||||
timeout-minutes: 10
|
||||
permissions:
|
||||
actions: read
|
||||
contents: read
|
||||
issues: write
|
||||
pull-requests: write
|
||||
steps:
|
||||
- name: Download recorded review event IDs
|
||||
id: download
|
||||
uses: actions/github-script@3a2844b7e9c422d3c10d287c895573f7108da1b3 # v9.0.0
|
||||
with:
|
||||
script: |
|
||||
const fs = require('fs');
|
||||
const { owner, repo } = context.repo;
|
||||
const arts = await github.rest.actions.listWorkflowRunArtifacts({
|
||||
owner, repo, run_id: context.payload.workflow_run.id,
|
||||
});
|
||||
const art = arts.data.artifacts.find(
|
||||
a => a.name === 'waiting-on-author-review-event'
|
||||
);
|
||||
if (!art) {
|
||||
core.info('No fork-review artifact; the direct review job handled this event.');
|
||||
core.setOutput('found', 'false');
|
||||
return;
|
||||
}
|
||||
const dl = await github.rest.actions.downloadArtifact({
|
||||
owner, repo, artifact_id: art.id, archive_format: 'zip',
|
||||
});
|
||||
fs.writeFileSync(
|
||||
`${process.env.RUNNER_TEMP}/waiting-on-author-review.zip`,
|
||||
Buffer.from(dl.data)
|
||||
);
|
||||
core.setOutput('found', 'true');
|
||||
|
||||
- name: Check out trusted .github
|
||||
if: steps.download.outputs.found == 'true'
|
||||
uses: actions/checkout@9c091bb21b7c1c1d1991bb908d89e4e9dddfe3e0 # v7.0.0
|
||||
with:
|
||||
ref: ${{ github.event.repository.default_branch }}
|
||||
sparse-checkout: .github
|
||||
persist-credentials: false
|
||||
|
||||
- name: Apply relayed waiting-on-author state
|
||||
if: steps.download.outputs.found == 'true'
|
||||
env:
|
||||
GITHUB_TOKEN: ${{ github.token }}
|
||||
RELAY_ZIP: ${{ runner.temp }}/waiting-on-author-review.zip
|
||||
RELAY_DIR: ${{ runner.temp }}/waiting-on-author-review
|
||||
WAITING_ON_AUTHOR_RELAY_EVENT: ${{ github.event.workflow_run.event }}
|
||||
WAITING_ON_AUTHOR_RELAY_PATH: ${{ runner.temp }}/waiting-on-author-review/event.json
|
||||
run: |
|
||||
mkdir -p "$RELAY_DIR"
|
||||
unzip -q "$RELAY_ZIP" -d "$RELAY_DIR"
|
||||
python3 .github/scripts/waiting_on_author.py
|
||||
@@ -0,0 +1,71 @@
|
||||
name: Waiting on Author Review
|
||||
|
||||
# Review events on fork PRs receive a read-only token. Same-repo reviews run the
|
||||
# hygiene script directly; fork reviews record only GitHub-provided numeric IDs
|
||||
# for the privileged workflow_run consumer. No PR code is checked out.
|
||||
|
||||
on:
|
||||
pull_request_review_comment:
|
||||
types: [created]
|
||||
pull_request_review:
|
||||
types: [submitted]
|
||||
|
||||
permissions:
|
||||
contents: read
|
||||
|
||||
concurrency:
|
||||
group: waiting-on-author-review-${{ github.event.pull_request.number }}
|
||||
cancel-in-progress: false
|
||||
|
||||
jobs:
|
||||
hygiene:
|
||||
if: >-
|
||||
github.repository == 'omnigent-ai/omnigent'
|
||||
&& github.event.pull_request.head.repo.full_name == github.repository
|
||||
runs-on: ubuntu-latest
|
||||
timeout-minutes: 10
|
||||
permissions:
|
||||
contents: read
|
||||
issues: write
|
||||
pull-requests: write
|
||||
steps:
|
||||
- name: Check out .github
|
||||
uses: actions/checkout@9c091bb21b7c1c1d1991bb908d89e4e9dddfe3e0 # v7.0.0
|
||||
with:
|
||||
ref: ${{ github.event.repository.default_branch }}
|
||||
sparse-checkout: .github
|
||||
persist-credentials: false
|
||||
- name: Update waiting-on-author state
|
||||
env:
|
||||
GITHUB_TOKEN: ${{ github.token }}
|
||||
run: python3 .github/scripts/waiting_on_author.py
|
||||
|
||||
record:
|
||||
if: >-
|
||||
github.repository == 'omnigent-ai/omnigent'
|
||||
&& github.event.pull_request.head.repo.full_name != github.repository
|
||||
runs-on: ubuntu-latest
|
||||
timeout-minutes: 5
|
||||
permissions:
|
||||
contents: read
|
||||
steps:
|
||||
- name: Record review event IDs
|
||||
env:
|
||||
EVENT_NAME: ${{ github.event_name }}
|
||||
PULL_NUMBER: ${{ github.event.pull_request.number }}
|
||||
ACTIVITY_ID: ${{ github.event.review.id || github.event.comment.id }}
|
||||
run: |
|
||||
mkdir -p relay
|
||||
jq -n \
|
||||
--arg event_name "$EVENT_NAME" \
|
||||
--argjson pull_number "$PULL_NUMBER" \
|
||||
--argjson activity_id "$ACTIVITY_ID" \
|
||||
'{event_name: $event_name, pull_number: $pull_number, activity_id: $activity_id}' \
|
||||
> relay/event.json
|
||||
- name: Upload review event IDs
|
||||
uses: actions/upload-artifact@043fb46d1a93c77aae656e7c1c64a875d1fc6a0a # v7.0.1
|
||||
with:
|
||||
name: waiting-on-author-review-event
|
||||
path: relay/event.json
|
||||
retention-days: 1
|
||||
if-no-files-found: error
|
||||
@@ -9,6 +9,8 @@ on:
|
||||
- .github/scripts/waiting_on_author.py
|
||||
- .github/scripts/waiting_on_author_test.py
|
||||
- .github/workflows/waiting-on-author.yml
|
||||
- .github/workflows/waiting-on-author-review.yml
|
||||
- .github/workflows/waiting-on-author-review-run.yml
|
||||
- .github/workflows/waiting-on-author-test.yml
|
||||
workflow_dispatch:
|
||||
|
||||
|
||||
@@ -6,16 +6,14 @@ name: Waiting on Author Hygiene
|
||||
# once a review is submitted). The two labels are mutually exclusive. PRs left
|
||||
# waiting on the author for 7 days are closed. The workflow runs from trusted
|
||||
# default-branch code and never checks out PR-authored files.
|
||||
# Review events use the read-only -> workflow_run relay in
|
||||
# waiting-on-author-review.yml and waiting-on-author-review-run.yml.
|
||||
|
||||
on:
|
||||
pull_request_target:
|
||||
types: [synchronize, labeled]
|
||||
issue_comment:
|
||||
types: [created]
|
||||
pull_request_review_comment:
|
||||
types: [created]
|
||||
pull_request_review:
|
||||
types: [submitted]
|
||||
schedule:
|
||||
- cron: "0 */12 * * *"
|
||||
workflow_dispatch:
|
||||
|
||||
Reference in New Issue
Block a user