From 2432228d688de60b5397aebc232f0282bdaf7eb5 Mon Sep 17 00:00:00 2001 From: claude Date: Sat, 22 Aug 2026 01:30:07 +0000 Subject: [PATCH] feat(webhook): gate on .pr-review.json:enabled, drop labels --- pilot/webhook_server.py | 60 ++++++++++-------------------- tests/pilot/test_webhook_server.py | 41 +++++++++++++------- 2 files changed, 47 insertions(+), 54 deletions(-) diff --git a/pilot/webhook_server.py b/pilot/webhook_server.py index 4853c41..0d371ab 100644 --- a/pilot/webhook_server.py +++ b/pilot/webhook_server.py @@ -2,13 +2,15 @@ """pragent pilot — central webhook receiver. A stdlib-only HTTP server that Gitea posts user-webhook events to. It gates on -the `AI-REVIEW` PR label, then runs the same review core (`ai_review.review_pr`) -the CI-step pilot uses, posting findings back as `pragent-bot`. +the PR's base ref having `.pr-review.json` with `"enabled": true`, then runs +the same review core (`ai_review.review_pr`) the CI-step pilot uses, posting +findings back as `pragent-bot`. Per-owner setup: one Gitea **user-level webhook** per repo-owner fires for every -repo that owner has; this service filters to labeled PRs. (Gitea 1.26.1 system +repo that owner has; this service filters to opted-in PRs. (Gitea 1.26.1 system webhooks are broken — see pilot/README-webhook.md.) Onboarding a repo = add the -bot as a Write collaborator + create the label + label a PR. +bot as a Write collaborator + commit a `.pr-review.json` with `"enabled": true` +on the base ref. Stdlib only — no pip install, runs on python:3-slim with the scripts mounted. @@ -53,15 +55,13 @@ except ImportError: feedback_harvest = None # Pull-request webhook `action` values. We fire on EVERY pull_request action -# except `closed` (no point reviewing a closed/merged PR) — the AI-REVIEW label -# gate + sha dedupe downstream make broadening safe: a same-sha re-fire (title -# edit, assignee, milestone, label toggle of another label…) is skipped by -# `review_pr`'s dedupe, and an `unlabeled` event that removed AI-REVIEW fails -# the label gate (payload `labels` reflect current state). Gitea emits -# GitHub-style `action` names (`labeled`, `synchronize`) even though the -# `X-Gitea-Event-Type` header uses `label_updated` / `synchronized`. +# except `closed` (no point reviewing a closed/merged PR) — the +# `.pr-review.json:enabled` gate + sha dedupe downstream make broadening safe: +# a same-sha re-fire (title edit, assignee, milestone, label toggle…) is +# skipped by `review_pr`'s dedupe. Gitea emits GitHub-style `action` names +# (`labeled`, `synchronize`) even though the `X-Gitea-Event-Type` header uses +# `label_updated` / `synchronized`. SKIP_ACTIONS = {"closed"} -AI_REVIEW_LABEL = "AI-REVIEW" GITEA_API = os.environ.get("GITEA_API", "http://gitea-http.gitea.svc.cluster.local:3000") BOT_TOKEN = os.environ.get("PRAGENT_BOT_TOKEN", "") @@ -87,30 +87,13 @@ _review_slots = threading.Semaphore(MAX_CONCURRENT) # Reviews currently accepted or running, keyed (repo, index, sha). The # sha-marker dedupe in `review_pr` reads Gitea *before* posting, so two # deliveries for the same commit in flight together both see "not yet reviewed" -# and both post — the classic check-then-act race, and label-toggling is exactly -# the kind of thing that fires two deliveries a second apart. This set closes -# the window inside one process. +# and both post — the classic check-then-act race. Common triggers are Gitea +# retries after a slow 202 response and bursty re-fires from a rapid title / +# assign / label toggle. This set closes the window inside one process. _inflight: set[tuple[str, str, str]] = set() _inflight_lock = threading.Lock() -def _labels_have(labels, name: str) -> bool: - """True if the Gitea PR `labels` list (dicts with `name`, or bare strings) - contains `name`.""" - if not isinstance(labels, list): - return False - for lab in labels: - if isinstance(lab, dict) and lab.get("name") == name: - return True - if isinstance(lab, str) and lab == name: - return True - return False - - -def _labels_have_ai_review(labels) -> bool: - return _labels_have(labels, AI_REVIEW_LABEL) - - def is_repo_enabled(api: str, repo: str, ref: str, token: str) -> bool: """True iff `.pr-review.json` on `ref` has `"enabled": true`. @@ -162,10 +145,6 @@ def _handle_pull_request(payload: dict) -> tuple[int, str]: if not repo: return 400, "no repository.full_name" - labels = pr.get("labels") - if not _labels_have_ai_review(labels): - return 200, f"ignore (no {AI_REVIEW_LABEL} label) action={action}" - index = pr.get("number") if index is None: return 400, "no pull_request.number" @@ -176,6 +155,9 @@ def _handle_pull_request(payload: dict) -> tuple[int, str]: base_ref = (pr.get("base") or {}).get("ref", "") or "" + if not is_repo_enabled(GITEA_API, repo, base_ref or "", BOT_TOKEN): + return 200, f"skip (repo not opted in) action={action}" + if not BOT_TOKEN: return 500, "PRAGENT_BOT_TOKEN not set" @@ -304,11 +286,9 @@ class Handler(BaseHTTPRequestHandler): self._send(200, f"ignore event={event}") return - pr0 = payload.get("pull_request") or {} + repo_full = (payload.get("repository") or {}).get("full_name") print( - f"pragent-webhook: pull_request action={payload.get('action')} " - f"repo={(payload.get('repository') or {}).get('full_name')} " - f"ai_review={_labels_have_ai_review(pr0.get('labels'))}", + f"pragent-webhook: pull_request action={payload.get('action')} repo={repo_full}", flush=True, ) status, msg = _handle_pull_request(payload) diff --git a/tests/pilot/test_webhook_server.py b/tests/pilot/test_webhook_server.py index fead2b2..3228ed3 100644 --- a/tests/pilot/test_webhook_server.py +++ b/tests/pilot/test_webhook_server.py @@ -16,7 +16,6 @@ def _payload(**over): "number": 7, "title": "t", "body": "b", - "labels": [{"name": "AI-REVIEW"}], "head": {"sha": "a" * 40}, "base": {"ref": "main"}, } @@ -26,16 +25,11 @@ def _payload(**over): return p -# --------------------------------------------------------------------------- -# label gating -# --------------------------------------------------------------------------- - - -def test_labels_have_matches_dicts_and_strings(): - assert ws._labels_have([{"name": "AI-REVIEW"}], "AI-REVIEW") - assert ws._labels_have(["AI-REVIEW"], "AI-REVIEW") - assert not ws._labels_have([{"name": "other"}], "AI-REVIEW") - assert not ws._labels_have(None, "AI-REVIEW") +def _enable_repo(monkeypatch, enabled: bool = True): + """Patch `is_repo_enabled` to the given bool for handler tests.""" + monkeypatch.setattr( + "webhook_server.is_repo_enabled", lambda *a, **kw: enabled + ) # --------------------------------------------------------------------------- @@ -76,6 +70,7 @@ def test_claim_is_thread_safe(): def test_duplicate_delivery_for_same_sha_is_not_reviewed_twice(monkeypatch): started = [] monkeypatch.setattr(ws, "BOT_TOKEN", "tok") + _enable_repo(monkeypatch) class FakeThread: def __init__(self, target, args, daemon): @@ -98,6 +93,7 @@ def test_duplicate_delivery_for_same_sha_is_not_reviewed_twice(monkeypatch): def test_base_ref_is_passed_to_the_review_thread(monkeypatch): started = [] monkeypatch.setattr(ws, "BOT_TOKEN", "tok") + _enable_repo(monkeypatch) class FakeThread: def __init__(self, target, args, daemon): @@ -115,16 +111,33 @@ def test_base_ref_is_passed_to_the_review_thread(monkeypatch): def test_closed_action_is_ignored(monkeypatch): monkeypatch.setattr(ws, "BOT_TOKEN", "tok") + _enable_repo(monkeypatch) status, msg = ws._handle_pull_request(_payload(action="closed")) assert status == 200 assert "ignore" in msg -def test_missing_label_is_ignored(monkeypatch): +def test_handle_pull_request_skips_when_repo_not_enabled(monkeypatch): + _enable_repo(monkeypatch, enabled=False) + started = [] monkeypatch.setattr(ws, "BOT_TOKEN", "tok") - status, msg = ws._handle_pull_request(_payload(pr={"labels": [{"name": "wip"}]})) + + class FakeThread: + def __init__(self, target, args, daemon): + self.args = args + + def start(self): + started.append(self.args) + + monkeypatch.setattr(ws.threading, "Thread", FakeThread) + ws._release(("o/r", "7", "a" * 40)) + + status, msg = ws._handle_pull_request(_payload()) assert status == 200 - assert "AI-REVIEW" in msg + assert "skip" in msg and "repo not opted in" in msg + assert "opened" in msg + assert started == [] + ws._release(("o/r", "7", "a" * 40)) # ---------------------------------------------------------------------------