From f91424db14e0c29d7ebd9c32cbd8bbf13891d31f Mon Sep 17 00:00:00 2001 From: Michele Dolfi Date: Thu, 8 Oct 2026 10:08:51 +0200 Subject: [PATCH] feat(ci): skip the AI review after maintainer activity and add /ai commands - A push runs the AI review only while no maintainer (OWNER, MEMBER, or COLLABORATOR, not a bot and not the PR author) has commented on or reviewed the PR. The triage still runs on every push. - Maintainers can comment `/ai review` (or `/ai`) for a new full review of the current commit, and `/ai triage` to run the triage again. The bot confirms a command with a reaction. Commands from other users are ignored. - A new `context` job resolves the PR and the mode from the pull_request_target or issue_comment event, so the other jobs no longer read the event payload. Other comments get their own concurrency group and do not cancel a running triage. Signed-off-by: Michele Dolfi --- .github/CI_LABELS.md | 14 ++++ .github/scripts/ai_pr_event.py | 106 +++++++++++++++++++++++++++++ .github/scripts/ai_pr_review.py | 33 ++++++++- .github/workflows/ai-pr-triage.yml | 90 ++++++++++++++++++++---- tests/test_ai_pr_event.py | 89 ++++++++++++++++++++++++ 5 files changed, 316 insertions(+), 16 deletions(-) create mode 100644 .github/scripts/ai_pr_event.py create mode 100644 tests/test_ai_pr_event.py diff --git a/.github/CI_LABELS.md b/.github/CI_LABELS.md index 616de18d57..d1c482048c 100644 --- a/.github/CI_LABELS.md +++ b/.github/CI_LABELS.md @@ -71,6 +71,20 @@ three more. The list is in `.github/scripts/pr_topic_labels.py`. The workflow adds topic labels only on the first triage of a PR, never creates a label, and never removes one, so maintainers can correct them. +### `/ai` commands + +After a maintainer (`OWNER`, `MEMBER`, or `COLLABORATOR`) comments on or +reviews a PR, a push runs only the triage, not the AI review. Maintainers can +ask for more with a PR comment that starts with one of these commands: + +- `/ai review` (or `/ai`): a new, full AI review of the current commit. It + also runs for a commit that was already reviewed and for a PR rated as a + duplicate. The size limits still apply. +- `/ai triage`: run the triage again. + +The bot adds a 👀 reaction to confirm the command. Commands from other users +are ignored. + The comment also summarizes changed reference data in `tests/data/**/groundtruth/`. It separates formatting-only and coordinate-only changes from text, table, and structure changes. diff --git a/.github/scripts/ai_pr_event.py b/.github/scripts/ai_pr_event.py new file mode 100644 index 0000000000..4bc1552715 --- /dev/null +++ b/.github/scripts/ai_pr_event.py @@ -0,0 +1,106 @@ +# SPDX-FileCopyrightText: The Docling Contributors +# SPDX-License-Identifier: MIT + +"""Resolve which PR the AI triage handles and in which mode. + +The workflow starts on ``pull_request_target`` (each push) and on +``issue_comment`` (``/ai`` commands from maintainers). This module turns the +event payload into a small, validated decision, so the jobs never read +untrusted comment text themselves. +""" + +from __future__ import annotations + +import argparse +import json +import os +import re +import sys +from dataclasses import dataclass +from pathlib import Path +from typing import Any + +MAINTAINER_ASSOCIATIONS = frozenset({"OWNER", "MEMBER", "COLLABORATOR"}) +COMMAND = re.compile(r"^/ai(?:\s+(?Preview|triage))?\s*$") + + +@dataclass(slots=True) +class Decision: + pr: int + run: bool + force_review: bool + reason: str + + +def parse_command(body: str) -> str | None: + """Return 'review' or 'triage' for an /ai command on the first line.""" + lines = body.strip().splitlines() + match = COMMAND.match(lines[0].strip()) if lines else None + if match is None: + return None + return match.group("name") or "review" + + +def resolve(event_name: str, event: dict[str, Any]) -> Decision: + if event_name == "pull_request_target": + pull = event["pull_request"] + number = int(pull["number"]) + if pull.get("draft"): + return Decision(number, False, False, "draft PR") + if pull["user"]["type"] == "Bot": + return Decision(number, False, False, "PR from a bot") + return Decision(number, True, False, "push") + + if event_name == "issue_comment": + issue = event["issue"] + number = int(issue["number"]) + comment = event["comment"] + if "pull_request" not in issue or issue.get("state") != "open": + return Decision(number, False, False, "not an open PR") + if comment.get("author_association") not in MAINTAINER_ASSOCIATIONS: + return Decision(number, False, False, "comment is not from a maintainer") + command = parse_command(comment.get("body") or "") + if command is None: + return Decision(number, False, False, "no /ai command") + return Decision(number, True, command == "review", f"/ai {command}") + + raise ValueError(f"unsupported event: {event_name}") + + +def is_maintainer_activity(item: dict[str, Any], pr_author: str) -> bool: + """True for a comment or review that a maintainer wrote by hand.""" + user = item.get("user") or {} + body = (item.get("body") or "").strip() + return ( + item.get("author_association") in MAINTAINER_ASSOCIATIONS + and user.get("type") != "Bot" + and user.get("login") != pr_author + and parse_command(body) is None + ) + + +def main(argv: list[str] | None = None) -> int: + parser = argparse.ArgumentParser(description=__doc__) + parser.add_argument("--event-name", required=True) + parser.add_argument("--event-path", type=Path, required=True) + args = parser.parse_args(argv) + + event = json.loads(args.event_path.read_text("utf-8")) + decision = resolve(args.event_name, event) + print( + f"PR #{decision.pr}: run={decision.run}," + f" force_review={decision.force_review} ({decision.reason})" + ) + github_output = os.environ.get("GITHUB_OUTPUT") + if github_output: + with Path(github_output).open("a", encoding="utf-8") as handle: + handle.write(f"pr={decision.pr}\n") + handle.write(f"run={'true' if decision.run else 'false'}\n") + handle.write( + f"force_review={'true' if decision.force_review else 'false'}\n" + ) + return 0 + + +if __name__ == "__main__": + sys.exit(main()) diff --git a/.github/scripts/ai_pr_review.py b/.github/scripts/ai_pr_review.py index cb7d6ea8d6..a72d8a009d 100644 --- a/.github/scripts/ai_pr_review.py +++ b/.github/scripts/ai_pr_review.py @@ -25,6 +25,7 @@ from pathlib import Path, PurePosixPath from typing import Any +from ai_pr_event import is_maintainer_activity from ai_pr_triage import ( DIFF_EXCLUDED_PATHS, TriageContext, @@ -276,11 +277,34 @@ def previous_review_text(repo: str, pr_number: int) -> str: return "\n".join(parts)[:MAX_PREVIOUS_REVIEW_CHARS] -def prepare(repo: str, context_dir: Path, git_dir: Path) -> bool: +def maintainer_is_involved(repo: str, pr_number: int, pr_author: str) -> bool: + """True when a maintainer already commented on or reviewed the PR.""" + sources = ( + f"repos/{repo}/issues/{pr_number}/comments?per_page=100", + f"repos/{repo}/pulls/{pr_number}/reviews?per_page=100", + f"repos/{repo}/pulls/{pr_number}/comments?per_page=100", + ) + return any( + is_maintainer_activity(item, pr_author) + for source in sources + for item in gh_api(source, paginate=True) + ) + + +def prepare(repo: str, context_dir: Path, git_dir: Path, force: bool) -> bool: + """Write the review inputs. Return False when no review should run. + + Without ``force`` (a push), the review is skipped after a maintainer got + involved and for a commit that the AI already reviewed. ``force`` (the + ``/ai review`` command) always runs a full review of the current commit. + """ context = TriageContext.from_dict( json.loads((context_dir / "context.json").read_text("utf-8")) ) pr = context.pr + if not force and maintainer_is_involved(repo, pr.number, pr.author): + print("A maintainer is involved. Use the /ai review command for a review.") + return False files = pr_files(repo, pr.number) if not is_reviewable(files): print("The PR is too large or has no source changes. No AI review.") @@ -291,7 +315,7 @@ def prepare(repo: str, context_dir: Path, git_dir: Path) -> bool: anchors = {item["filename"]: right_side_lines(item.get("patch")) for item in files} (review_dir / "anchors.json").write_text(json.dumps(anchors), encoding="utf-8") - reviewed = last_reviewed_sha(repo, pr.number) + reviewed = None if force else last_reviewed_sha(repo, pr.number) if reviewed == pr.head_sha: print("The AI already reviewed this commit.") return False @@ -391,6 +415,9 @@ def main(argv: list[str] | None = None) -> int: prep.add_argument("--repo", required=True) prep.add_argument("--context-dir", type=Path, required=True) prep.add_argument("--git-dir", type=Path, default=Path.cwd()) + prep.add_argument( + "--force", action="store_true", help="Full review (the /ai review command)." + ) gate = sub.add_parser("gate") gate.add_argument("--triage-result", type=Path, required=True) @@ -408,7 +435,7 @@ def main(argv: list[str] | None = None) -> int: args = parser.parse_args(argv) if args.command == "prepare": - run_review = prepare(args.repo, args.context_dir, args.git_dir) + run_review = prepare(args.repo, args.context_dir, args.git_dir, args.force) github_output = os.environ.get("GITHUB_OUTPUT") if github_output: with Path(github_output).open("a", encoding="utf-8") as handle: diff --git a/.github/workflows/ai-pr-triage.yml b/.github/workflows/ai-pr-triage.yml index 2396c69088..2174453343 100644 --- a/.github/workflows/ai-pr-triage.yml +++ b/.github/workflows/ai-pr-triage.yml @@ -14,20 +14,79 @@ name: "AI PR Triage" # The workflow is a review aid, not a gate. Every step uses continue-on-error, # and long steps have a timeout below the job timeout, so a problem shows as a # warning annotation and never as a failed check on the PR. +# +# Maintainers can comment `/ai review` (or `/ai`) for a new full review and +# `/ai triage` to run the triage again. After a maintainer comments on or +# reviews a PR, pushes run only the triage, not the review. on: pull_request_target: types: [opened, reopened, synchronize, ready_for_review] + issue_comment: + types: [created] concurrency: - group: ${{ github.workflow }}-${{ github.event.pull_request.number }} + # Other comments must not cancel a running triage, so they get their own group. + group: >- + ${{ github.workflow }}-${{ github.event.pull_request.number || github.event.issue.number }}-${{ + (github.event_name == 'pull_request_target' || + (startsWith(github.event.comment.body, '/ai') && + contains(fromJSON('["OWNER", "MEMBER", "COLLABORATOR"]'), github.event.comment.author_association))) + && 'pr' || github.run_id }} cancel-in-progress: true permissions: {} jobs: + context: + # Cheap filter first: most comments are not /ai commands from maintainers. + # ai_pr_event.py validates the event again. + if: >- + github.event_name == 'pull_request_target' || + (github.event.issue.pull_request && + startsWith(github.event.comment.body, '/ai') && + contains(fromJSON('["OWNER", "MEMBER", "COLLABORATOR"]'), github.event.comment.author_association)) + runs-on: ubuntu-latest + timeout-minutes: 5 + continue-on-error: true + permissions: + contents: read + issues: write + outputs: + pr: ${{ steps.resolve.outputs.pr }} + run: ${{ steps.resolve.outputs.run }} + force_review: ${{ steps.resolve.outputs.force_review }} + steps: + - uses: actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd # v6.0.2 + continue-on-error: true + with: + ref: ${{ github.sha }} + fetch-depth: 1 + persist-credentials: false + sparse-checkout: .github/scripts + + - name: Resolve the PR and the mode + id: resolve + continue-on-error: true + env: + EVENT_NAME: ${{ github.event_name }} + run: | + python3 .github/scripts/ai_pr_event.py \ + --event-name "${EVENT_NAME}" --event-path "${GITHUB_EVENT_PATH}" + + - name: Confirm the command + if: github.event_name == 'issue_comment' && steps.resolve.outputs.run == 'true' + continue-on-error: true + env: + GH_TOKEN: ${{ secrets.GITHUB_TOKEN }} + REPO: ${{ github.repository }} + COMMENT_ID: ${{ github.event.comment.id }} + run: | + gh api --method POST "repos/${REPO}/issues/comments/${COMMENT_ID}/reactions" \ + -f content=eyes > /dev/null + reset-labels: # A new push makes the previous verdict stale. - if: github.event.action == 'synchronize' + if: github.event_name == 'pull_request_target' && github.event.action == 'synchronize' runs-on: ubuntu-latest timeout-minutes: 5 continue-on-error: true @@ -46,9 +105,8 @@ jobs: done prepare: - if: >- - github.event.pull_request.draft == false && - github.event.pull_request.user.type != 'Bot' + needs: context + if: needs.context.outputs.run == 'true' runs-on: ubuntu-latest timeout-minutes: 15 continue-on-error: true @@ -75,7 +133,7 @@ jobs: env: GH_TOKEN: ${{ secrets.GITHUB_TOKEN }} REPO: ${{ github.repository }} - PR_NUMBER: ${{ github.event.pull_request.number }} + PR_NUMBER: ${{ needs.context.outputs.pr }} run: | python3 .github/scripts/ai_pr_triage.py prepare \ --repo "${REPO}" --pr "${PR_NUMBER}" \ @@ -89,10 +147,11 @@ jobs: env: GH_TOKEN: ${{ secrets.GITHUB_TOKEN }} REPO: ${{ github.repository }} + FORCE: ${{ needs.context.outputs.force_review == 'true' && '--force' || '' }} run: | python3 .github/scripts/ai_pr_review.py prepare \ --repo "${REPO}" \ - --context-dir "${RUNNER_TEMP}/ai-triage" --git-dir "${GITHUB_WORKSPACE}" + --context-dir "${RUNNER_TEMP}/ai-triage" --git-dir "${GITHUB_WORKSPACE}" ${FORCE} - uses: actions/upload-artifact@043fb46d1a93c77aae656e7c1c64a875d1fc6a0a # v7.0.1 continue-on-error: true @@ -166,7 +225,7 @@ jobs: retention-days: 7 publish: - needs: [prepare, analyze] + needs: [context, prepare, analyze] # run_llm is set only when the prepare step succeeded. if: always() && needs.prepare.outputs.run_llm != '' runs-on: ubuntu-latest @@ -202,7 +261,7 @@ jobs: env: GH_TOKEN: ${{ secrets.GITHUB_TOKEN }} REPO: ${{ github.repository }} - PR_NUMBER: ${{ github.event.pull_request.number }} + PR_NUMBER: ${{ needs.context.outputs.pr }} run: | python3 .github/scripts/ai_pr_triage.py publish \ --repo "${REPO}" --pr "${PR_NUMBER}" \ @@ -211,7 +270,7 @@ jobs: review: # Runs after the triage, so it can skip PRs rated as duplicates. - needs: [prepare, analyze] + needs: [context, prepare, analyze] if: >- always() && needs.prepare.result == 'success' && @@ -245,8 +304,13 @@ jobs: - name: Skip duplicates continue-on-error: true id: gate + env: + FORCE_REVIEW: ${{ needs.context.outputs.force_review }} run: | - if python3 .github/scripts/ai_pr_review.py gate \ + if [ "${FORCE_REVIEW}" = "true" ]; then + echo "A maintainer asked for this review with /ai review." + echo "run=true" >> "${GITHUB_OUTPUT}" + elif python3 .github/scripts/ai_pr_review.py gate \ --triage-result "${RUNNER_TEMP}/ai-triage-result/result.json"; then echo "run=true" >> "${GITHUB_OUTPUT}" else @@ -295,7 +359,7 @@ jobs: retention-days: 7 publish-review: - needs: [prepare, review] + needs: [context, prepare, review] if: always() && needs.review.result == 'success' runs-on: ubuntu-latest timeout-minutes: 10 @@ -332,7 +396,7 @@ jobs: env: GH_TOKEN: ${{ secrets.GITHUB_TOKEN }} REPO: ${{ github.repository }} - PR_NUMBER: ${{ github.event.pull_request.number }} + PR_NUMBER: ${{ needs.context.outputs.pr }} run: | python3 .github/scripts/ai_pr_review.py publish \ --repo "${REPO}" --pr "${PR_NUMBER}" \ diff --git a/tests/test_ai_pr_event.py b/tests/test_ai_pr_event.py new file mode 100644 index 0000000000..755dce5f11 --- /dev/null +++ b/tests/test_ai_pr_event.py @@ -0,0 +1,89 @@ +# SPDX-FileCopyrightText: The Docling Contributors +# SPDX-License-Identifier: MIT + +from __future__ import annotations + +import importlib.util +import sys +from pathlib import Path +from typing import Any + +import pytest + +SPEC = importlib.util.spec_from_file_location( + "ai_pr_event", + Path(__file__).resolve().parents[1] / ".github/scripts/ai_pr_event.py", +) +assert SPEC is not None and SPEC.loader is not None +event = importlib.util.module_from_spec(SPEC) +sys.modules[SPEC.name] = event +SPEC.loader.exec_module(event) + + +def comment_event(body: str, association: str = "MEMBER", **issue: Any): + issue_data = {"number": 7, "state": "open", "pull_request": {}} + issue_data.update(issue) + return { + "issue": issue_data, + "comment": {"body": body, "author_association": association}, + } + + +@pytest.mark.parametrize( + ("body", "expected"), + [ + ("/ai", "review"), + ("/ai review", "review"), + (" /ai triage \nplease", "triage"), + ("/ai please review", None), + ("/aireview", None), + ("please run /ai review", None), + ("", None), + ], +) +def test_only_exact_commands_on_the_first_line_count(body, expected) -> None: + assert event.parse_command(body) == expected + + +def test_push_events_skip_drafts_and_bots() -> None: + def push(draft: bool, user_type: str) -> object: + pull = {"number": 3, "draft": draft, "user": {"type": user_type}} + return event.resolve("pull_request_target", {"pull_request": pull}) + + assert push(False, "User") == event.Decision(3, True, False, "push") + assert not push(True, "User").run + assert not push(False, "Bot").run + + +def test_commands_need_a_maintainer_and_an_open_pr() -> None: + review = event.resolve("issue_comment", comment_event("/ai review")) + assert (review.pr, review.run, review.force_review) == (7, True, True) + + triage = event.resolve("issue_comment", comment_event("/ai triage", "OWNER")) + assert (triage.run, triage.force_review) == (True, False) + + assert not event.resolve("issue_comment", comment_event("/ai", "CONTRIBUTOR")).run + assert not event.resolve("issue_comment", comment_event("/ai", state="closed")).run + issue_only = comment_event("/ai") + del issue_only["issue"]["pull_request"] + assert not event.resolve("issue_comment", issue_only).run + + +def test_maintainer_activity_ignores_bots_the_author_and_commands() -> None: + def item(login: str, association: str, body: str = "Looks off.", kind="User"): + return { + "user": {"login": login, "type": kind}, + "author_association": association, + "body": body, + } + + assert event.is_maintainer_activity(item("maint", "MEMBER"), "contrib") + assert not event.is_maintainer_activity(item("contrib", "CONTRIBUTOR"), "contrib") + assert not event.is_maintainer_activity( + item("github-actions[bot]", "NONE", kind="Bot"), "contrib" + ) + # A maintainer's own PR: their replies are not a review by someone else. + assert not event.is_maintainer_activity(item("maint", "MEMBER"), "maint") + assert not event.is_maintainer_activity( + item("maint", "MEMBER", body="/ai review"), "contrib" + )