From aca5d5f48aef55809bd0983937156dd9f7baf241 Mon Sep 17 00:00:00 2001 From: can1357 Date: Fri, 19 Jun 2026 03:14:43 +0200 Subject: [PATCH] feat(python): implemented pull request vouching and gated review system - Introduced a vouching mechanism to manage PR authorization via a tracked user list and discussion-based management workflows. - Added automated PR gatekeeping workflows to close contributions from unvouched users and require specific labels for review. - Refactored PR event handling to support label-based review deferral and enforce authorization checks for labelers. - Added comprehensive test coverage for vouch-gate logic, including label activation and unauthorized access scenarios. --- .github/VOUCHED.td | 107 ++++++++++++ .github/workflows/vouch-manage.yml | 53 ++++++ .github/workflows/vouch-pr.yml | 51 ++++++ python/robomp/.env.example | 20 +++ python/robomp/docker-compose.yml | 10 ++ python/robomp/src/config.py | 12 ++ python/robomp/src/github_events.py | 59 +++++-- python/robomp/src/server.py | 3 + python/robomp/tests/test_github_events.py | 194 ++++++++++++++++++++++ 9 files changed, 491 insertions(+), 18 deletions(-) create mode 100644 .github/VOUCHED.td create mode 100644 .github/workflows/vouch-manage.yml create mode 100644 .github/workflows/vouch-pr.yml diff --git a/.github/VOUCHED.td b/.github/VOUCHED.td new file mode 100644 index 000000000..772bd36cd --- /dev/null +++ b/.github/VOUCHED.td @@ -0,0 +1,107 @@ +# The list of vouched (or actively denounced) users for this repository. +# +# Only vouched users can open PRs here; unvouched/denounced PRs are +# auto-closed by .github/workflows/vouch-pr.yml (mitchellh/vouch check-pr). +# Issues are intentionally NOT gated here — robomp triages those. +# +# Write-access collaborators and bots are auto-allowed and need no entry. +# A denounced user ("-handle") is always blocked, even if also listed. +# +# Syntax: +# - One handle per line (without @), sorted alphabetically. +# - Optional platform prefix: `platform:username` (default platform: github). +# - Denounce by prefixing with minus: `-username` / `-platform:username`. +# - Optional free-text reason after a space following the handle. +# +# Maintainers manage this list by commenting `!vouch` / `!denounce [user]` +# on a discussion (see .github/workflows/vouch-manage.yml). +# +# Seed (2026-06-19): authors with >=2 merged PRs in the prior 6 months. +# Audit found 0 denounce-worthy actors; all reverts were maintainer +# technical rollbacks, not abuse. See the PR/commit history for provenance. + +a-glapinski +ak4153 +apoc +AsafMah +azais-corentin +basedcorp99 +BayLee4 +bjin +cagedbird043 +cexll +chan1103 +chuaaron +ckumar1 +daandden +daaximus +danzaio +DarkPhilosophy +DeprecatedLuke +djdembeck +dmarsh-gusto +elikoga +enieuwy +ForeverYoungPp +GratefulDave +H4vC +HabibPro1999 +handlecusion +haosenwang1018 +hezhiyang2000 +infernix +inprealpha +itertea +jchristman +jiwangyihao +kamafozilov +KamijoToma +Kukkerem +ldx +lederniermagicien +loftiskg +lyc-aon +makoMakoGo +masonc15 +maximhar +maxvisionai +metaphorics +MikeeI +Mokto +mouyase +muness +nnk97 +ogrodev +oldschoola +Parsifa1 +phanthh +pidevxplay +qfrtt +ravshansbox +rburketaylor +RensTillmann +riverpilot +romanalexander +RzNmKX +scarthread +segmentationf4u1t +shoucandanghehe +shyndman +sit +slact +smileynet +sundbp +superhedge22 +tc97222 +tdiant +tsagi2045 +turbomolli +usr-bin-roygbiv +vmcall +VoidChecksum +voiys +watzon +WodenJay +wolfiesch +zakhar-kogan +zamorakpds diff --git a/.github/workflows/vouch-manage.yml b/.github/workflows/vouch-manage.yml new file mode 100644 index 000000000..706fbda0c --- /dev/null +++ b/.github/workflows/vouch-manage.yml @@ -0,0 +1,53 @@ +name: Vouch (manage) + +# Let maintainers vouch/denounce/unvouch by commenting on a Discussion: +# !vouch vouch the discussion author +# !vouch @user [reason] vouch a specific user +# !denounce [@user] [reason] +# !unvouch [@user] +# Only collaborators with admin/maintain/write are honored (triage EXCLUDED; +# upstream's default `roles` includes triage, which we override below). +# The action commits the VOUCHED.td change back to the default branch. + +on: + discussion_comment: + types: [created] + +# Serialize writes to VOUCHED.td so concurrent vouches don't clobber. +concurrency: + group: vouch-manage + cancel-in-progress: false + +# The job carries its own identity via the App token below, so the +# workflow itself needs no permissions. +permissions: {} + +jobs: + manage: + runs-on: ubuntu-latest + steps: + # A GitHub App identity is required only if the default branch is + # protected (the stock GITHUB_TOKEN cannot bypass branch protection). + # No branch protection? Delete this step, drop the checkout `token:`, + # set `permissions: { contents: write, discussions: write }`, and use + # `GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }}` below. + - uses: actions/create-github-app-token@v2 + id: app-token + with: + app-id: ${{ secrets.VOUCH_ID }} + private-key: ${{ secrets.VOUCH_PRIVATE_KEY }} + + - uses: actions/checkout@v4 + with: + token: ${{ steps.app-token.outputs.token }} + + - uses: mitchellh/vouch/action/manage-by-discussion@v1 + with: + discussion-number: ${{ github.event.discussion.number }} + comment-node-id: ${{ github.event.comment.node_id }} + vouch-keyword: "!vouch" + denounce-keyword: "!denounce" + unvouch-keyword: "!unvouch" + roles: admin,maintain,write + env: + GITHUB_TOKEN: ${{ steps.app-token.outputs.token }} diff --git a/.github/workflows/vouch-pr.yml b/.github/workflows/vouch-pr.yml new file mode 100644 index 000000000..6afa86cdc --- /dev/null +++ b/.github/workflows/vouch-pr.yml @@ -0,0 +1,51 @@ +name: Vouch (PR gate) + +# Auto-close PRs from unvouched or denounced users. Issues are left alone +# (robomp triages those). Runs under `pull_request_target` so the token can +# act on fork PRs; this job does NO checkout and runs NO PR code — it only +# reads .github/VOUCHED.td from the base repo and calls the GitHub API. + +on: + pull_request_target: + types: [opened, reopened, ready_for_review] + +permissions: + contents: read # read VOUCHED.td from the base branch + pull-requests: write # close + comment + issues: write # add the `vouched` label (labels use the Issues API) + +concurrency: + group: vouch-pr-${{ github.event.pull_request.number }} + cancel-in-progress: true + +jobs: + check: + runs-on: ubuntu-latest + steps: + - id: vouch + uses: mitchellh/vouch/action/check-pr@v1 + with: + pr-number: ${{ github.event.pull_request.number }} + auto-close: true + require-vouch: true # block unvouched, not only denounced + # vouched-file: .github/VOUCHED.td (default) + env: + GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }} + + # Survivors of the gate (vouched, or auto-allowed collaborators/bots) get + # a FRESH `vouched` label on every (re)open / ready-for-review. robomp + # reviews ONLY on that label event (ROBOMP_PR_REVIEW_TRIGGER=vouched_label), + # so review is always triggered by a just-validated PR, never a stale label. + - name: Label vouched PRs for robomp review + if: ${{ steps.vouch.outputs.status == 'vouched' || steps.vouch.outputs.status == 'allowed' }} + env: + GH_TOKEN: ${{ secrets.GITHUB_TOKEN }} + REPO: ${{ github.repository }} + PR: ${{ github.event.pull_request.number }} + run: | + gh label create vouched --repo "$REPO" --color 2da44e --description "Passed the vouch gate" --force + # remove+add so a fresh `labeled` event fires even when the label + # persisted across close/reopen (re-adding an existing label emits no + # event). The check above just re-validated, so trust is never stale. + gh pr edit "$PR" --repo "$REPO" --remove-label vouched || true + gh pr edit "$PR" --repo "$REPO" --add-label vouched diff --git a/python/robomp/.env.example b/python/robomp/.env.example index 235181162..2cf2ff218 100644 --- a/python/robomp/.env.example +++ b/python/robomp/.env.example @@ -110,6 +110,26 @@ ROBOMP_TASK_TIMEOUT_HARD_GRACE_SECONDS=60 ROBOMP_REQUEST_TIMEOUT_SECONDS=120 +# ============================================================================= +# --- PR review / vouch gate --- +# ============================================================================= +# robomp reviews incoming contributor PRs. When PRs are gated by the vouch +# GitHub Action (.github/workflows/vouch-pr.yml), set the trigger to +# `vouched_label` so robomp reviews ONLY the PRs the gate let through (the +# workflow labels survivors with ROBOMP_VOUCH_REVIEW_LABEL) instead of racing +# the gate on PR open. Set ROBOMP_PR_REVIEW_ENABLED=false to disable robomp PR +# review entirely. +ROBOMP_PR_REVIEW_ENABLED=true +# open | vouched_label +ROBOMP_PR_REVIEW_TRIGGER=open +ROBOMP_VOUCH_REVIEW_LABEL=vouched +# In vouched_label mode only `labeled` events from this actor trigger review, +# so a manual triage/maintainer label cannot bypass the gate. Default matches +# the stock GITHUB_TOKEN actor; set to your App's bot login if the vouch +# workflow labels via a GitHub App token. +ROBOMP_VOUCH_REVIEW_LABELER=github-actions[bot] + + # ============================================================================= # --- Per-submitter rate limiting --- # ============================================================================= diff --git a/python/robomp/docker-compose.yml b/python/robomp/docker-compose.yml index 4a06e418d..3d8aed813 100644 --- a/python/robomp/docker-compose.yml +++ b/python/robomp/docker-compose.yml @@ -44,6 +44,16 @@ services: ROBOMP_MAINTAINER_LOGINS: ${ROBOMP_MAINTAINER_LOGINS:-} ROBOMP_REVIEWER_BOTS: ${ROBOMP_REVIEWER_BOTS:-} + # --- PR review / vouch gate --- + # When PRs are gated by the vouch GitHub Action, set the trigger to + # `vouched_label` so robomp reviews ONLY PRs that survive the gate + # (the workflow labels survivors `ROBOMP_VOUCH_REVIEW_LABEL`), instead + # of racing the gate on PR open. `vouched_label` keeps review ENABLED. + ROBOMP_PR_REVIEW_ENABLED: ${ROBOMP_PR_REVIEW_ENABLED:-true} + ROBOMP_PR_REVIEW_TRIGGER: ${ROBOMP_PR_REVIEW_TRIGGER:-open} + ROBOMP_VOUCH_REVIEW_LABEL: ${ROBOMP_VOUCH_REVIEW_LABEL:-vouched} + ROBOMP_VOUCH_REVIEW_LABELER: ${ROBOMP_VOUCH_REVIEW_LABELER:-github-actions[bot]} + # --- model selection --- ROBOMP_MODEL: ${ROBOMP_MODEL:-anthropic/claude-sonnet-4-6} ROBOMP_PROVIDER: ${ROBOMP_PROVIDER:-} diff --git a/python/robomp/src/config.py b/python/robomp/src/config.py index 2c8f1494e..1498e86af 100644 --- a/python/robomp/src/config.py +++ b/python/robomp/src/config.py @@ -38,6 +38,18 @@ class Settings(BaseSettings): git_author_email: str = Field(..., alias="ROBOMP_GIT_AUTHOR_EMAIL") repo_allowlist_raw: str = Field("", alias="ROBOMP_REPO_ALLOWLIST") pr_review_enabled: bool = Field(True, alias="ROBOMP_PR_REVIEW_ENABLED") + # PR review trigger. "open" (default) reviews incoming PRs on + # opened/reopened/ready_for_review. "vouched_label" DEFERS review until the + # vouch GitHub Action labels the PR `vouch_review_label`, so robomp reviews + # only PRs that survive the vouch gate. `pr_review_enabled` remains the + # master switch (False disables review under either trigger). + pr_review_trigger: Literal["open", "vouched_label"] = Field("open", alias="ROBOMP_PR_REVIEW_TRIGGER") + vouch_review_label: str = Field("vouched", alias="ROBOMP_VOUCH_REVIEW_LABEL") + # In vouched_label mode, only `labeled` events from this actor trigger a + # review, so a manual label by a triage/maintainer cannot bypass the gate. + # Default is the actor for the stock GITHUB_TOKEN; set to your App's bot + # login (e.g. "vouch-bot[bot]") if the vouch workflow labels via an App. + vouch_review_labeler: str = Field("github-actions[bot]", alias="ROBOMP_VOUCH_REVIEW_LABELER") # gh-proxy. Set BOTH to route GitHub through the proxy; leave both empty # to keep PAT-on-orchestrator behavior. Mixing the two (PAT + proxy) is diff --git a/python/robomp/src/github_events.py b/python/robomp/src/github_events.py index 1e0c08ceb..75007fff2 100644 --- a/python/robomp/src/github_events.py +++ b/python/robomp/src/github_events.py @@ -140,6 +140,23 @@ def is_implementation_authorizer( return False +def _pr_review_pr(pr: Mapping[str, Any], repo: str, action: str, bot_login: str) -> RouteDecision: + """Build a `review_pr` decision for an incoming PR, or the matching skip.""" + if str(pr.get("state") or "open") != "open": + return RouteDecision("skip", None, repo, None, "PR not open") + if bool(pr.get("draft")): + return RouteDecision("skip", None, repo, None, "draft PR") + if _is_bot_account(pr.get("user") or {}, bot_login): + return RouteDecision("skip", None, repo, None, "bot-authored PR") + number = pr.get("number") + if not isinstance(number, int): + return RouteDecision("skip", None, repo, None, "PR missing number") + login, assoc = _submitter_info(pr) + return RouteDecision( + "queue", "review_pr", repo, issue_key(repo, number), f"pull_request.{action}", submitter=login, association=assoc + ) + + def route( event_type: str, payload: Mapping[str, Any], @@ -150,6 +167,9 @@ def route( reviewer_bots: frozenset[str] = frozenset(), resolve_issue_from_pr: PrIssueResolver = None, pr_review_enabled: bool = True, + pr_review_trigger: str = "open", + vouch_review_label: str = "vouched", + vouch_review_labeler: str = "github-actions[bot]", ) -> RouteDecision: """Decide whether and how to handle a webhook event. @@ -281,24 +301,27 @@ def route( if not pr_review_enabled: return RouteDecision("skip", None, repo, None, "PR review disabled") pr = payload.get("pull_request") or {} - if bool(pr.get("draft")): - return RouteDecision("skip", None, repo, None, "draft PR") - pr_user = pr.get("user") or {} - if _is_bot_account(pr_user, bot_login): - return RouteDecision("skip", None, repo, None, "bot-authored PR") - number = pr.get("number") - if not isinstance(number, int): - return RouteDecision("skip", None, repo, None, "PR missing number") - login, assoc = _submitter_info(pr) - return RouteDecision( - "queue", - "review_pr", - repo, - issue_key(repo, number), - f"pull_request.{action}", - submitter=login, - association=assoc, - ) + if pr_review_trigger == "vouched_label": + # Defer to the vouch gate. robomp reviews ONLY on the `labeled` + # event the workflow emits AFTER a fresh check (it re-applies the + # vouch label on every opened/reopened/ready_for_review). Never + # trust a persisted label here: a since-denounced author must not + # slip through on reopen. + return RouteDecision("skip", None, repo, None, "deferred to vouch label") + return _pr_review_pr(pr, repo, action, bot_login) + + if event_type == "pull_request" and action == "labeled" and pr_review_trigger == "vouched_label": + if not pr_review_enabled: + return RouteDecision("skip", None, repo, None, "PR review disabled") + label = payload.get("label") + label_name = str(label.get("name") or "") if isinstance(label, Mapping) else "" + if label_name.lower() != vouch_review_label.lower(): + return RouteDecision("skip", None, repo, None, f"label {label_name!r} not vouch label") + sender = payload.get("sender") + labeler = str(sender.get("login") or "") if isinstance(sender, Mapping) else "" + if labeler.lower() != vouch_review_labeler.lower(): + return RouteDecision("skip", None, repo, None, f"vouch label not from trusted labeler ({labeler!r})") + return _pr_review_pr(payload.get("pull_request") or {}, repo, action, bot_login) if event_type == "pull_request_review_comment" and action == "created": comment = payload.get("comment") or {} diff --git a/python/robomp/src/server.py b/python/robomp/src/server.py index 5d64700a6..0d1978679 100644 --- a/python/robomp/src/server.py +++ b/python/robomp/src/server.py @@ -341,6 +341,9 @@ def create_app(settings: Settings | None = None) -> FastAPI: maintainers=cfg.maintainer_logins, reviewer_bots=cfg.reviewer_bots, pr_review_enabled=cfg.pr_review_enabled, + pr_review_trigger=cfg.pr_review_trigger, + vouch_review_label=cfg.vouch_review_label, + vouch_review_labeler=cfg.vouch_review_labeler, resolve_issue_from_pr=_resolve, ) diff --git a/python/robomp/tests/test_github_events.py b/python/robomp/tests/test_github_events.py index 6acffbb09..67d51f2dc 100644 --- a/python/robomp/tests/test_github_events.py +++ b/python/robomp/tests/test_github_events.py @@ -857,3 +857,197 @@ def test_route_non_directive_comment_carries_no_pragmas() -> None: ) assert decision.directive is False assert decision.directive_pragmas == () + + +# ---------- vouched-label deferred PR review ---------- + + +def test_route_vouched_label_defers_pr_open() -> None: + decision = route( + "pull_request", + { + "action": "opened", + "pull_request": {"number": 9, "draft": False, "user": {"login": "alice", "type": "User"}}, + "repository": {"full_name": "octo/widget"}, + }, + allowlist=ALLOWLIST, + bot_login=BOT, + pr_review_trigger="vouched_label", + ) + assert not decision.should_queue + assert decision.reason == "deferred to vouch label" + + +def test_route_vouched_label_reviews_on_label() -> None: + decision = route( + "pull_request", + { + "action": "labeled", + "label": {"name": "vouched"}, + "pull_request": { + "number": 9, + "draft": False, + "user": {"login": "alice", "type": "User"}, + "author_association": "CONTRIBUTOR", + }, + "repository": {"full_name": "octo/widget"}, + "sender": {"login": "github-actions[bot]"}, + }, + allowlist=ALLOWLIST, + bot_login=BOT, + pr_review_trigger="vouched_label", + ) + assert decision.should_queue + assert decision.task == "review_pr" + assert decision.issue_key == "octo/widget#9" + assert decision.submitter == "alice" + + +def test_route_vouched_label_ignores_other_labels() -> None: + decision = route( + "pull_request", + { + "action": "labeled", + "label": {"name": "bug"}, + "pull_request": {"number": 9, "user": {"login": "alice"}}, + "repository": {"full_name": "octo/widget"}, + }, + allowlist=ALLOWLIST, + bot_login=BOT, + pr_review_trigger="vouched_label", + ) + assert not decision.should_queue + + +def test_route_vouched_label_ready_for_review_defers_even_with_label() -> None: + # Persisted labels are NOT trusted; the workflow re-applies the label (a + # fresh `labeled` event) after re-validating, so ready_for_review defers. + decision = route( + "pull_request", + { + "action": "ready_for_review", + "pull_request": { + "number": 9, + "draft": False, + "user": {"login": "alice", "type": "User"}, + "labels": [{"name": "vouched"}], + }, + "repository": {"full_name": "octo/widget"}, + }, + allowlist=ALLOWLIST, + bot_login=BOT, + pr_review_trigger="vouched_label", + ) + assert not decision.should_queue + assert decision.reason == "deferred to vouch label" + + +def test_route_vouched_label_labeled_skips_draft() -> None: + decision = route( + "pull_request", + { + "action": "labeled", + "label": {"name": "vouched"}, + "pull_request": {"number": 9, "draft": True, "user": {"login": "alice", "type": "User"}}, + "repository": {"full_name": "octo/widget"}, + "sender": {"login": "github-actions[bot]"}, + }, + allowlist=ALLOWLIST, + bot_login=BOT, + pr_review_trigger="vouched_label", + ) + assert not decision.should_queue + assert decision.reason == "draft PR" + + +def test_route_default_trigger_ignores_labeled() -> None: + # Backward-compat: the default "open" trigger does not route `labeled`. + decision = route( + "pull_request", + { + "action": "labeled", + "label": {"name": "vouched"}, + "pull_request": {"number": 9, "user": {"login": "alice"}}, + "repository": {"full_name": "octo/widget"}, + }, + allowlist=ALLOWLIST, + bot_login=BOT, + ) + assert not decision.should_queue + + +def test_route_vouched_label_reopened_defers_even_with_label() -> None: + # A since-denounced author must not slip through on reopen via a stale + # label; reopened always defers to a fresh gate check + re-label. + decision = route( + "pull_request", + { + "action": "reopened", + "pull_request": { + "number": 9, + "draft": False, + "user": {"login": "alice", "type": "User"}, + "labels": [{"name": "vouched"}], + }, + "repository": {"full_name": "octo/widget"}, + }, + allowlist=ALLOWLIST, + bot_login=BOT, + pr_review_trigger="vouched_label", + ) + assert not decision.should_queue + assert decision.reason == "deferred to vouch label" + + +def test_route_vouched_label_reopened_without_label_defers() -> None: + decision = route( + "pull_request", + { + "action": "reopened", + "pull_request": {"number": 9, "draft": False, "user": {"login": "alice", "type": "User"}}, + "repository": {"full_name": "octo/widget"}, + }, + allowlist=ALLOWLIST, + bot_login=BOT, + pr_review_trigger="vouched_label", + ) + assert not decision.should_queue + assert decision.reason == "deferred to vouch label" + + +def test_route_vouched_label_rejects_manual_labeler() -> None: + # A triage/maintainer hand-adding the label must NOT trigger review. + decision = route( + "pull_request", + { + "action": "labeled", + "label": {"name": "vouched"}, + "pull_request": {"number": 9, "draft": False, "user": {"login": "alice", "type": "User"}}, + "repository": {"full_name": "octo/widget"}, + "sender": {"login": "evilmaintainer"}, + }, + allowlist=ALLOWLIST, + bot_login=BOT, + pr_review_trigger="vouched_label", + ) + assert not decision.should_queue + assert "trusted labeler" in decision.reason + + +def test_route_vouched_label_skips_closed_pr() -> None: + # `labeled` can fire on a closed PR; never review one. + decision = route( + "pull_request", + { + "action": "labeled", + "label": {"name": "vouched"}, + "pull_request": {"number": 9, "state": "closed", "user": {"login": "alice", "type": "User"}}, + "repository": {"full_name": "octo/widget"}, + "sender": {"login": "github-actions[bot]"}, + }, + allowlist=ALLOWLIST, + bot_login=BOT, + pr_review_trigger="vouched_label", + ) + assert not decision.should_queue + assert decision.reason == "PR not open"