Merge PR #28758 — keep #28147 versions of overlapping triage files; review_gate feature applied in next commit

.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

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()