diff --git a/.github/workflows/review_gate.yml b/.github/workflows/review_gate.yml new file mode 100644 index 00000000000..73215f359e4 --- /dev/null +++ b/.github/workflows/review_gate.yml @@ -0,0 +1,128 @@ +name: Agent Shin — review gate + +# Keeps the `ready for review` label in sync with whether an external PR +# currently clears BOTH the LLM rubric AND Greptile's confidence score. +# +# pass -> add `ready for review` + a "passed / all clear" comment +# regress -> remove the label + a "what's missing" comment (PR stays open) +# fail, <24h old -> a one-time "what's missing" notice (grace window) +# fail, >24h old -> close + a comment (reopen via `@agent-shin reconsider`) +# +# DRY-RUN BY DEFAULT. Every side effect (label add/remove, comment, close) is +# gated behind `--close`, which is only added when the repo variable +# `AGENT_SHIN_ENABLED == "true"`. Until then runs only write the verdict to the +# workflow step summary. +# +# Manual single PR: gh workflow run "Agent Shin — review gate" -f pr_number=NNN +# Manual dry-run: gh workflow run "Agent Shin — review gate" -f close=false +# +# We use `pull_request_target` so the workflow can read repo secrets and run +# against fork PRs. Fork code is never checked out — only PR metadata is read +# via `gh api`. + +on: + pull_request_target: + types: [opened, reopened, synchronize, ready_for_review] + schedule: + # Daily at 09:30 UTC — re-reconciles labels as Greptile re-reviews land. + - cron: "30 9 * * *" + workflow_dispatch: + inputs: + pr_number: + description: "Single PR to reconcile (omit to sweep all open PRs)." + required: false + close: + description: "If AGENT_SHIN_ENABLED=true, actually act (false = dry run)." + required: false + default: "false" + type: choice + options: + - "true" + - "false" + grace_days: + description: "Hours/24 a failing, un-tagged PR may stay open before close." + required: false + default: "1" + min_greptile_score: + description: "Greptile score below which a PR counts as not passing (1-5)." + required: false + default: "4" + +permissions: + contents: read + issues: write + pull-requests: write + +jobs: + review-gate: + if: github.repository == 'BerriAI/litellm' + runs-on: ubuntu-latest + steps: + - name: Checkout triage script + uses: actions/checkout@08eba0b27e820071cde6df949e0beb9ba4906955 # v4.3.0 + with: + sparse-checkout: .github/scripts + persist-credentials: false + + - name: Set up Python + uses: actions/setup-python@a26af69be951a213d495a4c3e4e4022e16d87065 # v5.6.0 + with: + python-version: "3.12" + + - name: Install LLM client + run: pip install --no-cache-dir "openai>=1.40.0" + + - name: Run review gate + env: + GH_TOKEN: ${{ secrets.GITHUB_TOKEN }} + # Mirror the triage workflow: only expose the LLM key when the bot is + # enabled or a collaborator triggers it manually, so an external user + # can't force paid LLM calls by churning a fork PR while the bot is + # still in dry-run. + OPENAI_API_KEY: ${{ (vars.AGENT_SHIN_ENABLED == 'true' || github.event_name == 'workflow_dispatch') && secrets.OPENAI_API_KEY || '' }} + OPENAI_BASE_URL: ${{ vars.OPENAI_BASE_URL }} + TRIAGE_MODEL: ${{ vars.TRIAGE_MODEL }} + AGENT_SHIN_ENABLED: ${{ vars.AGENT_SHIN_ENABLED }} + CLOSE_FLAG: ${{ github.event.inputs.close || 'false' }} + GRACE_DAYS: ${{ github.event.inputs.grace_days || '1' }} + MIN_GREPTILE_SCORE: ${{ github.event.inputs.min_greptile_score || '4' }} + EVENT_PR: ${{ github.event.pull_request.number }} + INPUT_PR: ${{ github.event.inputs.pr_number }} + run: | + set -euo pipefail + COMMON=(--review-gate --grace-days "${GRACE_DAYS}" --min-greptile-score "${MIN_GREPTILE_SCORE}") + + # Fail-safe gating, identical philosophy to the Greptile closer: + # - AGENT_SHIN_ENABLED must be the EXACT string "true" to act at all. + # - A manual dispatch can still preview with close=false. + # - Automatic triggers (PR events, schedule) act once enabled — that + # is the whole point of the gate (re-tag / un-tag automatically). + DO_CLOSE="false" + if [ "${AGENT_SHIN_ENABLED:-false}" != "true" ]; then + echo "::notice::AGENT_SHIN_ENABLED is not 'true' -> dry-run (no labels/comments/closes)." + elif [ "${GITHUB_EVENT_NAME:-}" = "workflow_dispatch" ] && [ "${CLOSE_FLAG:-false}" = "true" ]; then + DO_CLOSE="true" + echo "::notice::Manual run -> acting for real." + elif [ "${GITHUB_EVENT_NAME:-}" != "workflow_dispatch" ]; then + DO_CLOSE="true" + echo "::notice::Enabled automatic trigger (${GITHUB_EVENT_NAME:-}) -> acting for real." + else + echo "::notice::Manual dispatch with close=false -> dry-run." + fi + if [ "${DO_CLOSE}" = "true" ]; then + COMMON+=(--close) + fi + + # Single PR (PR event or explicit input) vs. sweep over all open PRs. + TARGET_PR="${EVENT_PR:-${INPUT_PR:-}}" + if [ -n "${TARGET_PR}" ]; then + python3 .github/scripts/triage_with_llm.py --repo "${{ github.repository }}" --pr "${TARGET_PR}" "${COMMON[@]}" + else + echo "::notice::Sweeping all open PRs." + mapfile -t NUMBERS < <(gh pr list --repo "${{ github.repository }}" --state open --limit 1000 --json number --jq '.[].number') + for n in "${NUMBERS[@]}"; do + echo "::group::PR #${n}" + python3 .github/scripts/triage_with_llm.py --repo "${{ github.repository }}" --pr "${n}" "${COMMON[@]}" || echo "::warning::review gate errored on #${n}" + echo "::endgroup::" + done + fi diff --git a/tests/test_litellm/test_github_review_gate.py b/tests/test_litellm/test_github_review_gate.py new file mode 100644 index 00000000000..635aec17c97 --- /dev/null +++ b/tests/test_litellm/test_github_review_gate.py @@ -0,0 +1,385 @@ +"""Unit tests for the `ready for review` label lifecycle (Agent Shin review gate). + +Exercises `triage_with_llm.review_gate`, the state machine that keeps the +`ready for review` label in sync with whether a PR clears both the LLM rubric +and Greptile's confidence score: + + * pass (untagged) -> add label + "ready for review" comment + * pass (untagged, recovered) -> add label + "all clear again" comment + * pass (already tagged) -> noop + * regress (tagged) -> remove label + "what's missing" comment, stays open + * fail (untagged, within 24h)-> one-time "what's missing" notice + * fail (untagged, >24h) -> close + comment + * dry run (close=False) -> would-* previews, no side effects +""" + +from __future__ import annotations + +import datetime as dt +import importlib.util +import sys +from pathlib import Path + +import pytest + +SCRIPT_PATH = ( + Path(__file__).resolve().parents[2] / ".github" / "scripts" / "triage_with_llm.py" +) + +NOW = dt.datetime(2026, 5, 24, 12, 0, 0, tzinfo=dt.timezone.utc) +JUST_NOW = "2026-05-24T11:00:00Z" # 1h old -> within 24h grace +TWO_DAYS_AGO = "2026-05-22T11:00:00Z" # >24h old -> past grace + + +@pytest.fixture(scope="module") +def triage_module(): + spec = importlib.util.spec_from_file_location("triage_with_llm", SCRIPT_PATH) + assert spec and spec.loader + module = importlib.util.module_from_spec(spec) + sys.modules["triage_with_llm"] = module + spec.loader.exec_module(module) + return module + + +class _Recorder: + """Captures every gh mutation review_gate could fire, and fails loudly + on the ones a given scenario forbids.""" + + def __init__(self, triage_module, monkeypatch): + self.comments: list[str] = [] + self.added: list[str] = [] + self.removed: list[str] = [] + self.closed: list[int] = [] + monkeypatch.setattr( + triage_module, + "post_comment", + lambda repo, n, body: self.comments.append(body), + ) + monkeypatch.setattr( + triage_module, + "add_label", + lambda repo, n, label: self.added.append(label), + ) + monkeypatch.setattr( + triage_module, + "remove_label", + lambda repo, n, label: self.removed.append(label), + ) + monkeypatch.setattr( + triage_module, + "close_pr", + lambda repo, n: self.closed.append(n), + ) + + +def _make_pr(**overrides): + base = { + "number": 7, + "title": "feat: do a thing", + "body": "some body without a linked issue or QA proof", + "state": "open", + "author_association": "NONE", + "user": {"login": "outside-dev"}, + "labels": [], + "created_at": JUST_NOW, + } + base.update(overrides) + return base + + +def _pass(prompt): + return '{"verdict": "pass", "missing": [], "explanation": "looks good"}' + + +def _fail(prompt): + return ( + '{"verdict": "fail", "missing": ["QA proof", "expected vs. actual"],' + ' "explanation": "thin description"}' + ) + + +def _gate(triage_module, **kwargs): + """Call review_gate with safe defaults for the injectable hooks.""" + params = dict( + repo="o/r", + number=7, + close=True, + model="m", + judge=_pass, + greptile_score=None, + comments=[], + now=NOW, + ) + params.update(kwargs) + return triage_module.review_gate(**params) + + +class TestReviewGatePass: + def test_pass_untagged_adds_label_and_ready_comment( + self, triage_module, monkeypatch + ): + monkeypatch.setattr(triage_module, "fetch_pr", lambda repo, n: _make_pr()) + rec = _Recorder(triage_module, monkeypatch) + + result = _gate(triage_module, judge=_pass, greptile_score=5) + + assert result["action"] == "labeled-ready" + assert rec.added == [triage_module.READY_FOR_REVIEW_LABEL] + assert rec.removed == [] and rec.closed == [] + assert len(rec.comments) == 1 + assert "ready for review" in rec.comments[0].lower() + assert triage_module.READY_MARKER in rec.comments[0] + assert "5/5" in rec.comments[0] + + def test_pass_already_tagged_is_noop(self, triage_module, monkeypatch): + pr = _make_pr(labels=[{"name": "ready for review"}]) + monkeypatch.setattr(triage_module, "fetch_pr", lambda repo, n: pr) + rec = _Recorder(triage_module, monkeypatch) + + result = _gate(triage_module, judge=_pass, greptile_score=5) + + assert result["action"] == "noop-passing" + assert rec.added == [] and rec.removed == [] and rec.comments == [] + + def test_pass_after_prior_regression_uses_all_clear_wording( + self, triage_module, monkeypatch + ): + # A regression marker in history -> this is a recovery, not a first pass. + monkeypatch.setattr(triage_module, "fetch_pr", lambda repo, n: _make_pr()) + rec = _Recorder(triage_module, monkeypatch) + prior = [{"user": {"login": "x"}, "body": triage_module.REGRESSED_MARKER}] + + result = _gate(triage_module, judge=_pass, greptile_score=5, comments=prior) + + assert result["action"] == "labeled-ready" + assert "all clear" in rec.comments[0].lower() + + def test_linked_issue_passes_without_calling_judge( + self, triage_module, monkeypatch + ): + pr = _make_pr(body="Fixes #4321\n\nbody") + monkeypatch.setattr(triage_module, "fetch_pr", lambda repo, n: pr) + rec = _Recorder(triage_module, monkeypatch) + + result = _gate( + triage_module, + judge=lambda p: pytest.fail("LLM must not be called for linked issue"), + greptile_score=5, + ) + assert result["action"] == "labeled-ready" + assert rec.added == [triage_module.READY_FOR_REVIEW_LABEL] + + +class TestReviewGateRegression: + def test_regression_removes_label_and_keeps_pr_open( + self, triage_module, monkeypatch + ): + pr = _make_pr(labels=[{"name": "ready for review"}]) + monkeypatch.setattr(triage_module, "fetch_pr", lambda repo, n: pr) + rec = _Recorder(triage_module, monkeypatch) + + result = _gate(triage_module, judge=_fail, greptile_score=5) + + assert result["action"] == "label-removed-regressed" + assert rec.removed == [triage_module.READY_FOR_REVIEW_LABEL] + assert rec.closed == [] # regression NEVER closes the PR + assert triage_module.REGRESSED_MARKER in rec.comments[0] + assert "QA proof" in rec.comments[0] + + def test_greptile_drop_alone_triggers_regression(self, triage_module, monkeypatch): + # Rubric still passes, but Greptile fell to 2/5 -> not passing. + pr = _make_pr(labels=[{"name": "ready for review"}]) + monkeypatch.setattr(triage_module, "fetch_pr", lambda repo, n: pr) + rec = _Recorder(triage_module, monkeypatch) + + result = _gate(triage_module, judge=_pass, greptile_score=2) + + assert result["action"] == "label-removed-regressed" + assert rec.removed == [triage_module.READY_FOR_REVIEW_LABEL] + assert "2/5" in rec.comments[0] + + def test_greptile_score_read_from_comments_when_not_injected( + self, triage_module, monkeypatch + ): + pr = _make_pr(labels=[{"name": "ready for review"}]) + monkeypatch.setattr(triage_module, "fetch_pr", lambda repo, n: pr) + rec = _Recorder(triage_module, monkeypatch) + greptile = [ + { + "user": {"login": "greptile-apps[bot]"}, + "body": "Confidence Score: 2/5", + "created_at": "2026-05-24T10:00:00Z", + } + ] + + result = _gate( + triage_module, + judge=_pass, + greptile_score=triage_module._UNSET, + comments=greptile, + ) + assert result["action"] == "label-removed-regressed" + assert "2/5" in rec.comments[0] + + +class TestReviewGateGraceAndClose: + def test_within_grace_posts_one_time_notice(self, triage_module, monkeypatch): + monkeypatch.setattr( + triage_module, "fetch_pr", lambda repo, n: _make_pr(created_at=JUST_NOW) + ) + rec = _Recorder(triage_module, monkeypatch) + + result = _gate(triage_module, judge=_fail, greptile_score=None) + + assert result["action"] == "within-grace-notified" + assert rec.closed == [] and rec.added == [] and rec.removed == [] + assert triage_module.WITHIN_GRACE_MARKER in rec.comments[0] + assert "QA proof" in rec.comments[0] + + def test_within_grace_does_not_double_notify(self, triage_module, monkeypatch): + monkeypatch.setattr( + triage_module, "fetch_pr", lambda repo, n: _make_pr(created_at=JUST_NOW) + ) + rec = _Recorder(triage_module, monkeypatch) + prior = [{"user": {"login": "b"}, "body": triage_module.WITHIN_GRACE_MARKER}] + + result = _gate(triage_module, judge=_fail, greptile_score=None, comments=prior) + + assert result["action"] == "within-grace-already-notified" + assert rec.comments == [] + + def test_past_grace_closes_with_comment(self, triage_module, monkeypatch): + monkeypatch.setattr( + triage_module, + "fetch_pr", + lambda repo, n: _make_pr(created_at=TWO_DAYS_AGO), + ) + rec = _Recorder(triage_module, monkeypatch) + + result = _gate(triage_module, judge=_fail, greptile_score=None) + + assert result["action"] == "closed" + assert rec.closed == [7] + assert len(rec.comments) == 1 + # The close comment must carry the reconsider provenance marker. + assert triage_module.AGENT_SHIN_AUTO_CLOSE_MARKER in rec.comments[0] + + +class TestReviewGateDryRun: + @pytest.mark.parametrize( + "scenario,labels,judge,score,created,expected", + [ + ("pass", [], _pass, 5, JUST_NOW, "would-label-ready"), + ( + "regress", + [{"name": "ready for review"}], + _fail, + 5, + JUST_NOW, + "would-remove-label", + ), + ("within-grace", [], _fail, None, JUST_NOW, "would-notify-within-grace"), + ("past-grace", [], _fail, None, TWO_DAYS_AGO, "would-close"), + ], + ) + def test_dry_run_previews_without_side_effects( + self, + triage_module, + monkeypatch, + scenario, + labels, + judge, + score, + created, + expected, + ): + pr = _make_pr(labels=labels, created_at=created) + monkeypatch.setattr(triage_module, "fetch_pr", lambda repo, n: pr) + rec = _Recorder(triage_module, monkeypatch) + + result = _gate(triage_module, close=False, judge=judge, greptile_score=score) + + assert result["action"] == expected + # Dry run touches nothing. + assert rec.added == [] and rec.removed == [] and rec.closed == [] + assert rec.comments == [] + assert "comment" in result # preview body still surfaced + + +class TestReviewGateGuards: + def test_skips_internal_author(self, triage_module, monkeypatch): + pr = _make_pr(author_association="MEMBER", user={"login": "krrish"}) + monkeypatch.setattr(triage_module, "fetch_pr", lambda repo, n: pr) + result = _gate( + triage_module, judge=lambda p: pytest.fail("no LLM for internal") + ) + assert result["action"] == "skip-internal-author" + + def test_skips_closed_pr(self, triage_module, monkeypatch): + pr = _make_pr(state="closed") + monkeypatch.setattr(triage_module, "fetch_pr", lambda repo, n: pr) + result = _gate(triage_module, judge=lambda p: pytest.fail("no LLM for closed")) + assert result["action"] == "skip-not-open" + + def test_llm_error_is_non_destructive(self, triage_module, monkeypatch): + monkeypatch.setattr(triage_module, "fetch_pr", lambda repo, n: _make_pr()) + rec = _Recorder(triage_module, monkeypatch) + + def boom(prompt): + raise RuntimeError("api down") + + result = _gate(triage_module, judge=boom, greptile_score=None) + + assert result["action"] == "skip-llm-error" + assert rec.closed == [] and rec.added == [] and rec.removed == [] + + def test_full_recovery_cycle(self, triage_module, monkeypatch): + """pass -> regress -> recover, threading labels/comments like GitHub would.""" + state = {"labels": [], "comments": []} + + def fake_fetch(repo, n): + return _make_pr(labels=list(state["labels"]), created_at=JUST_NOW) + + monkeypatch.setattr(triage_module, "fetch_pr", fake_fetch) + monkeypatch.setattr( + triage_module, + "post_comment", + lambda repo, n, body: state["comments"].append( + {"user": {"login": "agent-shin[bot]"}, "body": body} + ), + ) + monkeypatch.setattr( + triage_module, + "add_label", + lambda repo, n, label: state["labels"].append({"name": label}), + ) + monkeypatch.setattr( + triage_module, + "remove_label", + lambda repo, n, label: state["labels"].clear(), + ) + monkeypatch.setattr( + triage_module, "close_pr", lambda repo, n: pytest.fail("must not close") + ) + + # 1) passes -> tagged + r1 = _gate( + triage_module, judge=_pass, greptile_score=5, comments=state["comments"] + ) + assert r1["action"] == "labeled-ready" + assert any(lbl["name"] == "ready for review" for lbl in state["labels"]) + + # 2) regresses -> tag removed, comment posted, PR still open + r2 = _gate( + triage_module, judge=_fail, greptile_score=2, comments=state["comments"] + ) + assert r2["action"] == "label-removed-regressed" + assert state["labels"] == [] + + # 3) fixed again -> "all clear" + tag back + r3 = _gate( + triage_module, judge=_pass, greptile_score=5, comments=state["comments"] + ) + assert r3["action"] == "labeled-ready" + assert any(lbl["name"] == "ready for review" for lbl in state["labels"]) + assert "all clear" in state["comments"][-1]["body"].lower()