From 2e7fdc6f9c3dfde7e558682e3272500ca6ce4be4 Mon Sep 17 00:00:00 2001 From: Cursor Agent Date: Sun, 17 May 2026 09:24:59 +0000 Subject: [PATCH 01/17] feat(triage): auto-close stale PRs with Greptile score <4/5 Adds .github/scripts/close_low_quality_prs.py and a daily workflow that closes PRs which: - are open for at least 7 days, and - carry a most-recent greptile-apps review with Confidence Score <4/5, - and are not drafts or opt-out-labeled ('do not close', 'wip', etc.). Each closure posts an explanatory comment telling the contributor how to bring the PR back (rebase, re-request greptile, reopen at 4+/5). The 4/5 bar is already documented in the PR template (.github/pull_request_template.md), so this just enforces it. Tested with a dry run against the live BerriAI/litellm backlog of 1000 open PRs: 100 candidates identified, 598 PRs pass the bar (4+/5), 186 are too young, 97 are drafts, 19 lack any Greptile review and are left alone. Workflow defaults to closing 25 PRs/run as a safety net and supports workflow_dispatch with overrides (close=false for a dry run, custom min_age_days/min_score/limit). 18 unit tests cover score extraction (HTML/markdown/plain text, login variants, multi-review picks latest) and per-PR evaluation (drafts, opt-out labels, age, missing/passing/failing scores). Co-authored-by: Mateo Wang --- .github/scripts/close_low_quality_prs.py | 348 ++++++++++++++++++ .github/workflows/close_low_quality_prs.yml | 80 ++++ .../test_github_close_low_quality_prs.py | 277 ++++++++++++++ 3 files changed, 705 insertions(+) create mode 100644 .github/scripts/close_low_quality_prs.py create mode 100644 .github/workflows/close_low_quality_prs.yml create mode 100644 tests/test_litellm/test_github_close_low_quality_prs.py diff --git a/.github/scripts/close_low_quality_prs.py b/.github/scripts/close_low_quality_prs.py new file mode 100644 index 00000000000..0a0ff980201 --- /dev/null +++ b/.github/scripts/close_low_quality_prs.py @@ -0,0 +1,348 @@ +#!/usr/bin/env python3 +""" +Auto-close stale, low-quality pull requests. + +Closes open PRs that satisfy ALL of the following: + 1. Are at least N days old (default: 7) since creation. + 2. Have a Greptile (`greptile-apps`) review comment whose latest + "Confidence Score: X/5" is below the configured threshold (default: 4). + 3. Are not drafts. + 4. Do not carry an opt-out label (default: "do not close"). + +For each match, the script posts an explanatory comment and closes the PR. +Contributors are invited to rebase and request a new Greptile review; if +Greptile then scores 4/5 or higher the PR can be reopened by anyone with +push access. + +Requires the `gh` CLI to be authenticated. + +Usage examples: + # Dry run (default) - prints what would be closed + python3 close_low_quality_prs.py + + # Actually close matching PRs + python3 close_low_quality_prs.py --close + + # Tweak thresholds + python3 close_low_quality_prs.py --min-age-days 7 --min-score 4 --close +""" + +from __future__ import annotations + +import argparse +import datetime as dt +import json +import re +import subprocess +import sys +from typing import Any, Iterable + +# Greptile's GitHub App appears as `greptile-apps[bot]` in REST API comments +# and `greptile-apps` in `gh pr view --json` output. Accept either form. +GREPTILE_BOT_LOGINS = frozenset({"greptile-apps", "greptile-apps[bot]"}) + +# Matches lines like: +#

Confidence Score: 3/5

+# **Confidence Score: 4/5** +# Confidence Score: 5 / 5 +SCORE_PATTERN = re.compile( + r"confidence\s*score\s*[:\-]?\s*(\d+)\s*/\s*5", + re.IGNORECASE, +) + + +def gh(*args: str) -> str: + """Run a `gh` CLI command and return stdout. Raises on non-zero exit.""" + result = subprocess.run( + ["gh", *args], + capture_output=True, + text=True, + check=True, + ) + return result.stdout + + +def gh_json(*args: str) -> Any: + """Run a `gh` CLI command that emits JSON and return the parsed value.""" + return json.loads(gh(*args)) + + +def fetch_open_prs(repo: str | None) -> list[dict]: + """Fetch all open PRs (number, createdAt, isDraft, labels, author).""" + repo_args = ["--repo", repo] if repo else [] + fields = "number,title,createdAt,isDraft,labels,author,url" + raw = gh( + "pr", + "list", + "--state", + "open", + "--limit", + "1000", + "--json", + fields, + *repo_args, + ) + return json.loads(raw) + + +def fetch_pr_comments(pr_number: int, repo: str | None) -> list[dict]: + """Fetch issue-level comments on a PR (where Greptile posts its summary).""" + endpoint = ( + f"repos/{repo}/issues/{pr_number}/comments?per_page=100" + if repo + else f"repos/{{owner}}/{{repo}}/issues/{pr_number}/comments?per_page=100" + ) + raw = gh("api", "--paginate", endpoint) + comments: list[dict] = [] + for line in raw.strip().splitlines(): + line = line.strip() + if not line: + continue + parsed = json.loads(line) + if isinstance(parsed, list): + comments.extend(parsed) + else: + comments.append(parsed) + return comments + + +def extract_greptile_score(comments: Iterable[dict]) -> tuple[int, dict] | None: + """Return (score, comment) for the most recent Greptile-authored comment + that contains a "Confidence Score: X/5". Returns None if no such comment. + + "Most recent" is determined by the comment's `updated_at` (falling back to + `created_at`), so re-reviews override earlier passes. + """ + candidates: list[tuple[str, int, dict]] = [] + for comment in comments: + user = (comment.get("user") or {}).get("login", "") + if user not in GREPTILE_BOT_LOGINS: + continue + body = comment.get("body") or "" + match = SCORE_PATTERN.search(body) + if not match: + continue + score = int(match.group(1)) + timestamp = comment.get("updated_at") or comment.get("created_at") or "" + candidates.append((timestamp, score, comment)) + + if not candidates: + return None + + candidates.sort(key=lambda triple: triple[0]) + _, score, comment = candidates[-1] + return score, comment + + +def parse_iso8601(value: str) -> dt.datetime: + """Parse a GitHub ISO-8601 timestamp into a timezone-aware datetime.""" + return dt.datetime.fromisoformat(value.replace("Z", "+00:00")) + + +def has_optout_label(pr: dict, optout_labels: set[str]) -> bool: + labels = {label.get("name", "").lower() for label in pr.get("labels", [])} + return bool(labels & {lbl.lower() for lbl in optout_labels}) + + +def close_pr( + pr: dict, + score: int, + threshold: int, + age_days: int, + repo: str | None, + dry_run: bool, + label: str | None, +) -> None: + """Post the explanatory comment and close the PR.""" + pr_number = pr["number"] + repo_args = ["--repo", repo] if repo else [] + + if dry_run: + print( + f" [DRY RUN] Would close PR #{pr_number} " + f"(age={age_days}d, greptile={score}/5): {pr['title']}" + ) + return + + comment_body = ( + f"Closing as part of automated PR triage.\n\n" + f"This PR has been open for **{age_days} day(s)** and Greptile's most " + f"recent review scored it **{score}/5**, below our merge bar of " + f"**{threshold}/5**.\n\n" + "We close low-confidence PRs aggressively to keep the review queue " + "manageable for maintainers and contributors alike. **This is not a " + "rejection of the idea** — to bring this back:\n\n" + "1. Rebase on the latest `main` and address the points Greptile raised.\n" + f"2. Re-request a review from `@greptileai` once you've pushed the fixes.\n" + f"3. If Greptile returns a score of **{threshold}/5 or higher**, reopen " + "this PR (or open a new one) — a maintainer will take another look.\n\n" + "Thanks for contributing to LiteLLM. We know auto-closures can sting; " + "the goal is to keep the project healthy, not to dismiss your work." + ) + gh("pr", "comment", str(pr_number), "--body", comment_body, *repo_args) + + if label: + try: + gh("pr", "edit", str(pr_number), "--add-label", label, *repo_args) + except subprocess.CalledProcessError as exc: + stderr = (exc.stderr or "").strip() + print(f" warn: failed to add label '{label}' to #{pr_number}: {stderr}") + + gh("pr", "close", str(pr_number), *repo_args) + print(f" Closed PR #{pr_number} (greptile={score}/5, age={age_days}d)") + + +def evaluate_pr( + pr: dict, + now: dt.datetime, + min_age_days: int, + min_score: int, + repo: str | None, + optout_labels: set[str], +) -> tuple[str, int | None, int | None]: + """Decide whether to close `pr`. + + Returns (action, score_or_none, age_days_or_none) where action is one of: + "skip-draft", "skip-too-young", "skip-optout-label", + "skip-no-greptile-score", "skip-score-ok", or "close". + """ + if pr.get("isDraft"): + return ("skip-draft", None, None) + + if has_optout_label(pr, optout_labels): + return ("skip-optout-label", None, None) + + created = parse_iso8601(pr["createdAt"]) + age_days = (now - created).days + if age_days < min_age_days: + return ("skip-too-young", None, age_days) + + comments = fetch_pr_comments(pr["number"], repo) + extraction = extract_greptile_score(comments) + if extraction is None: + return ("skip-no-greptile-score", None, age_days) + + score, _ = extraction + if score >= min_score: + return ("skip-score-ok", score, age_days) + + return ("close", score, age_days) + + +def main() -> int: + parser = argparse.ArgumentParser(description=__doc__) + parser.add_argument( + "--repo", + type=str, + default=None, + help="Repository (owner/repo). Auto-detected if omitted.", + ) + parser.add_argument( + "--min-age-days", + type=int, + default=7, + help="Minimum age (in days) before a PR is eligible (default: 7).", + ) + parser.add_argument( + "--min-score", + type=int, + default=4, + choices=range(1, 6), + help="Greptile score below which a PR is closed (default: 4 -> closes <4/5).", + ) + parser.add_argument( + "--optout-label", + action="append", + default=["do not close", "keep open", "wip"], + help=( + "Label(s) that exempt a PR from auto-close. " + "Repeat to add more. Case-insensitive." + ), + ) + parser.add_argument( + "--close-label", + type=str, + default=None, + help=( + "Optional label to add to PRs that get auto-closed " + "(e.g. 'auto-closed-low-quality'). Must already exist on the repo." + ), + ) + parser.add_argument( + "--close", + action="store_true", + help="Actually close matching PRs (default is dry-run).", + ) + parser.add_argument( + "--limit", + type=int, + default=None, + help="Maximum number of PRs to close in one run (safety net).", + ) + args = parser.parse_args() + + dry_run = not args.close + if dry_run: + print("=== DRY RUN MODE (pass --close to actually close PRs) ===\n") + + print("Fetching open PRs...") + prs = fetch_open_prs(args.repo) + print(f"Found {len(prs)} open PRs.\n") + + now = dt.datetime.now(dt.timezone.utc) + optout_labels = set(args.optout_label) + + closed = 0 + summary = { + "close": 0, + "skip-draft": 0, + "skip-too-young": 0, + "skip-optout-label": 0, + "skip-no-greptile-score": 0, + "skip-score-ok": 0, + } + + for pr in sorted(prs, key=lambda p: p["createdAt"]): + action, score, age_days = evaluate_pr( + pr, + now, + args.min_age_days, + args.min_score, + args.repo, + optout_labels, + ) + summary[action] = summary.get(action, 0) + 1 + + if action != "close": + continue + + assert score is not None and age_days is not None + print( + f"#{pr['number']}: \"{pr['title']}\" " + f"(age={age_days}d, greptile={score}/5) -> close" + ) + close_pr( + pr, + score=score, + threshold=args.min_score, + age_days=age_days, + repo=args.repo, + dry_run=dry_run, + label=args.close_label, + ) + + if not dry_run: + closed += 1 + if args.limit is not None and closed >= args.limit: + print(f"\nReached --limit={args.limit}; stopping.") + break + + print("\n=== Summary ===") + for key, value in summary.items(): + print(f" {key:28s} {value}") + print(f"\nTotal {'would close' if dry_run else 'closed'}: {summary['close']}") + return 0 + + +if __name__ == "__main__": + sys.exit(main()) diff --git a/.github/workflows/close_low_quality_prs.yml b/.github/workflows/close_low_quality_prs.yml new file mode 100644 index 00000000000..7768e89c0f8 --- /dev/null +++ b/.github/workflows/close_low_quality_prs.yml @@ -0,0 +1,80 @@ +name: Close Low-Quality Stale PRs + +# Auto-close PRs that have been open for a week or more and that Greptile has +# reviewed with a confidence score below 4/5. Closures are explained in a +# comment so contributors know how to bring the PR back (rebase, request a new +# Greptile review, reopen on a 4+/5). +# +# Manual one-off run: +# gh workflow run "Close Low-Quality Stale PRs" -f close=true +# +# Dry-run preview (no PRs are touched): +# gh workflow run "Close Low-Quality Stale PRs" -f close=false + +on: + schedule: + # Daily at 09:00 UTC. Pairs well with the stale-issue workflow at midnight. + - cron: "0 9 * * *" + workflow_dispatch: + inputs: + close: + description: "Actually close matching PRs (false = dry run)." + required: false + default: "true" + type: choice + options: + - "true" + - "false" + min_age_days: + description: "Minimum PR age in days before it is eligible." + required: false + default: "7" + min_score: + description: "Greptile score below which a PR is closed (1-5)." + required: false + default: "4" + limit: + description: "Maximum number of PRs to close in a single run." + required: false + default: "25" + +permissions: + contents: read + pull-requests: write + issues: write + +jobs: + close-low-quality-prs: + 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: Run low-quality PR closer + env: + GH_TOKEN: ${{ secrets.GITHUB_TOKEN }} + CLOSE_FLAG: ${{ github.event.inputs.close || 'true' }} + MIN_AGE_DAYS: ${{ github.event.inputs.min_age_days || '7' }} + MIN_SCORE: ${{ github.event.inputs.min_score || '4' }} + LIMIT: ${{ github.event.inputs.limit || '25' }} + run: | + set -euo pipefail + ARGS=( + --repo "${{ github.repository }}" + --min-age-days "${MIN_AGE_DAYS}" + --min-score "${MIN_SCORE}" + --limit "${LIMIT}" + ) + if [ "${CLOSE_FLAG}" = "true" ]; then + ARGS+=(--close) + fi + python3 .github/scripts/close_low_quality_prs.py "${ARGS[@]}" diff --git a/tests/test_litellm/test_github_close_low_quality_prs.py b/tests/test_litellm/test_github_close_low_quality_prs.py new file mode 100644 index 00000000000..2b3119fc920 --- /dev/null +++ b/tests/test_litellm/test_github_close_low_quality_prs.py @@ -0,0 +1,277 @@ +"""Unit tests for `.github/scripts/close_low_quality_prs.py`. + +These exercise the pure logic (score extraction and per-PR evaluation) without +hitting GitHub. Network/CLI calls are stubbed via monkeypatch. +""" + +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" + / "close_low_quality_prs.py" +) + + +@pytest.fixture(scope="module") +def closer_module(): + """Load the script as a module via its file path (it lives outside the package).""" + spec = importlib.util.spec_from_file_location("close_low_quality_prs", SCRIPT_PATH) + assert spec and spec.loader, f"Could not load spec for {SCRIPT_PATH}" + module = importlib.util.module_from_spec(spec) + sys.modules["close_low_quality_prs"] = module + spec.loader.exec_module(module) + return module + + +def _greptile_comment( + body: str, + updated_at: str = "2026-05-10T00:00:00Z", + login: str = "greptile-apps[bot]", +) -> dict: + return { + "user": {"login": login}, + "body": body, + "created_at": updated_at, + "updated_at": updated_at, + } + + +class TestExtractGreptileScore: + def test_should_extract_score_from_html_header(self, closer_module): + comments = [ + _greptile_comment("

Confidence Score: 3/5

\nSome body text.") + ] + result = closer_module.extract_greptile_score(comments) + assert result is not None + score, _ = result + assert score == 3 + + def test_should_accept_both_greptile_login_variants(self, closer_module): + # REST API form ("greptile-apps[bot]") and GraphQL form ("greptile-apps") + for login in ("greptile-apps", "greptile-apps[bot]"): + comments = [ + _greptile_comment("

Confidence Score: 2/5

", login=login) + ] + result = closer_module.extract_greptile_score(comments) + assert result is not None, f"failed to detect score for login={login}" + score, _ = result + assert score == 2 + + def test_should_extract_score_from_plain_text(self, closer_module): + comments = [_greptile_comment("Confidence Score: 5/5 — looks good!")] + result = closer_module.extract_greptile_score(comments) + assert result is not None + score, _ = result + assert score == 5 + + def test_should_tolerate_whitespace_and_case(self, closer_module): + comments = [_greptile_comment("**confidence score : 2 / 5**")] + result = closer_module.extract_greptile_score(comments) + assert result is not None + score, _ = result + assert score == 2 + + def test_should_pick_most_recent_comment_when_rereview_happens(self, closer_module): + comments = [ + _greptile_comment( + "Confidence Score: 2/5", updated_at="2026-05-01T00:00:00Z" + ), + _greptile_comment( + "Confidence Score: 5/5", updated_at="2026-05-12T00:00:00Z" + ), + ] + result = closer_module.extract_greptile_score(comments) + assert result is not None + score, _ = result + assert score == 5 + + def test_should_ignore_non_greptile_authors(self, closer_module): + comments = [ + { + "user": {"login": "some-human"}, + "body": "Confidence Score: 1/5", + "created_at": "2026-05-12T00:00:00Z", + "updated_at": "2026-05-12T00:00:00Z", + } + ] + assert closer_module.extract_greptile_score(comments) is None + + def test_should_return_none_when_no_score_present(self, closer_module): + comments = [_greptile_comment("Greptile summary without a score.")] + assert closer_module.extract_greptile_score(comments) is None + + def test_should_return_none_for_empty_comments(self, closer_module): + assert closer_module.extract_greptile_score([]) is None + + +class TestEvaluatePr: + @pytest.fixture(autouse=True) + def _now(self): + return dt.datetime(2026, 5, 17, tzinfo=dt.timezone.utc) + + def _make_pr( + self, + *, + number: int = 1, + created_days_ago: int = 10, + is_draft: bool = False, + labels: list[str] | None = None, + ) -> dict: + created = dt.datetime(2026, 5, 17, tzinfo=dt.timezone.utc) - dt.timedelta( + days=created_days_ago + ) + return { + "number": number, + "title": f"PR #{number}", + "createdAt": created.isoformat().replace("+00:00", "Z"), + "isDraft": is_draft, + "labels": [{"name": lbl} for lbl in (labels or [])], + "author": {"login": "someone"}, + "url": f"https://example.com/pr/{number}", + } + + def test_should_skip_drafts(self, closer_module, _now, monkeypatch): + monkeypatch.setattr( + closer_module, + "fetch_pr_comments", + lambda *a, **kw: pytest.fail("should not fetch comments for drafts"), + ) + action, score, age = closer_module.evaluate_pr( + self._make_pr(is_draft=True), + now=_now, + min_age_days=7, + min_score=4, + repo=None, + optout_labels=set(), + ) + assert action == "skip-draft" + assert score is None and age is None + + def test_should_skip_optout_label_case_insensitive( + self, closer_module, _now, monkeypatch + ): + monkeypatch.setattr( + closer_module, + "fetch_pr_comments", + lambda *a, **kw: pytest.fail("should not fetch comments for opt-outs"), + ) + action, _, _ = closer_module.evaluate_pr( + self._make_pr(labels=["WIP"]), + now=_now, + min_age_days=7, + min_score=4, + repo=None, + optout_labels={"wip"}, + ) + assert action == "skip-optout-label" + + def test_should_skip_too_young(self, closer_module, _now, monkeypatch): + monkeypatch.setattr( + closer_module, + "fetch_pr_comments", + lambda *a, **kw: pytest.fail("should not fetch comments for young PRs"), + ) + action, _, age = closer_module.evaluate_pr( + self._make_pr(created_days_ago=2), + now=_now, + min_age_days=7, + min_score=4, + repo=None, + optout_labels=set(), + ) + assert action == "skip-too-young" + assert age == 2 + + def test_should_skip_when_greptile_has_not_reviewed( + self, closer_module, _now, monkeypatch + ): + monkeypatch.setattr(closer_module, "fetch_pr_comments", lambda *a, **kw: []) + action, score, age = closer_module.evaluate_pr( + self._make_pr(created_days_ago=10), + now=_now, + min_age_days=7, + min_score=4, + repo=None, + optout_labels=set(), + ) + assert action == "skip-no-greptile-score" + assert score is None and age == 10 + + def test_should_skip_when_score_meets_threshold( + self, closer_module, _now, monkeypatch + ): + monkeypatch.setattr( + closer_module, + "fetch_pr_comments", + lambda *a, **kw: [_greptile_comment("Confidence Score: 4/5")], + ) + action, score, age = closer_module.evaluate_pr( + self._make_pr(created_days_ago=10), + now=_now, + min_age_days=7, + min_score=4, + repo=None, + optout_labels=set(), + ) + assert action == "skip-score-ok" + assert score == 4 and age == 10 + + def test_should_close_when_old_and_low_score( + self, closer_module, _now, monkeypatch + ): + monkeypatch.setattr( + closer_module, + "fetch_pr_comments", + lambda *a, **kw: [_greptile_comment("Confidence Score: 3/5")], + ) + action, score, age = closer_module.evaluate_pr( + self._make_pr(created_days_ago=10), + now=_now, + min_age_days=7, + min_score=4, + repo=None, + optout_labels=set(), + ) + assert action == "close" + assert score == 3 and age == 10 + + def test_should_close_when_old_and_very_low_score( + self, closer_module, _now, monkeypatch + ): + monkeypatch.setattr( + closer_module, + "fetch_pr_comments", + lambda *a, **kw: [_greptile_comment("

Confidence Score: 1/5

")], + ) + action, score, _ = closer_module.evaluate_pr( + self._make_pr(created_days_ago=14), + now=_now, + min_age_days=7, + min_score=4, + repo=None, + optout_labels=set(), + ) + assert action == "close" + assert score == 1 + + +class TestHasOptoutLabel: + def test_should_match_label_case_insensitively(self, closer_module): + pr = {"labels": [{"name": "Do Not Close"}, {"name": "bug"}]} + assert closer_module.has_optout_label(pr, {"do not close"}) is True + + def test_should_return_false_when_no_match(self, closer_module): + pr = {"labels": [{"name": "bug"}, {"name": "enhancement"}]} + assert closer_module.has_optout_label(pr, {"wip", "keep open"}) is False + + def test_should_handle_missing_labels(self, closer_module): + assert closer_module.has_optout_label({}, {"wip"}) is False From ac18f1407e412bf0f2e673da5aa093b95d1592c7 Mon Sep 17 00:00:00 2001 From: Cursor Agent Date: Sun, 17 May 2026 16:25:07 +0000 Subject: [PATCH 02/17] docs(templates): require expected/actual + QA proof for external contributions PR template: - Make the rubric explicit at the top: link an issue, OR provide a clear problem description + expected vs. actual + visual QA proof. - Add dedicated sections for each piece so the bot has a deterministic shape to read. - Keep the existing 'Linear ticket' section for internal contributors (they're exempt from the auto-triage rubric). Bug report template: - Split 'What happened?' into 'Actual behavior' + 'Expected behavior'. - Make logs/screenshot a required textarea. - Warning banner at the top tells external contributors that incomplete reports will be auto-closed (with re-evaluation on reopen). Feature request template: - Require a concrete use case + example in the motivation field, not just a one-liner pitch. - Same auto-triage warning banner. Co-authored-by: Mateo Wang --- .github/ISSUE_TEMPLATE/bug_report.yml | 51 +++++++++++++------ .github/ISSUE_TEMPLATE/feature_request.yml | 28 ++++++++--- .github/pull_request_template.md | 58 ++++++++++++++++++---- 3 files changed, 105 insertions(+), 32 deletions(-) diff --git a/.github/ISSUE_TEMPLATE/bug_report.yml b/.github/ISSUE_TEMPLATE/bug_report.yml index bbe4b76775d..1938aab5007 100644 --- a/.github/ISSUE_TEMPLATE/bug_report.yml +++ b/.github/ISSUE_TEMPLATE/bug_report.yml @@ -6,8 +6,11 @@ body: - type: markdown attributes: value: | - Thanks for taking the time to fill out this bug report! - + Thanks for taking the time to file a bug report! + + > ⚠️ **Auto-triage notice for external contributors:** + > Bug reports without **clear reproduction steps, expected vs. actual behavior, and a screenshot or terminal/log output** are auto-closed by our LLM triage bot with an explanation of what was missing. You can fill in the missing details and reopen at any time — the bot will re-evaluate. Internal BerriAI contributors are exempt. + **💡 Tip:** See our [Troubleshooting Guide](https://docs.litellm.ai/docs/troubleshoot) for what information to include. - type: checkboxes id: duplicate-check @@ -20,21 +23,33 @@ body: - type: textarea id: what-happened attributes: - label: What happened? - description: Also tell us, what did you expect to happen? - placeholder: Tell us what you see! - value: "A bug happened!" + label: What happened? (Actual behavior) + description: A clear description of what is happening today, with the bug. + placeholder: e.g. "Calling completion() with model=gpt-4o-mini returns an empty string." + validations: + required: true + - type: textarea + id: expected-behavior + attributes: + label: What did you expect to happen? (Expected behavior) + description: A clear description of what you expected to happen. **Required.** + placeholder: e.g. "I expected completion() to return the model's response text." validations: required: true - type: textarea id: steps-to-reproduce attributes: - label: Steps to Reproduce - description: Please provide detailed steps to reproduce this bug(A curl/python code to reproduce the bug) + label: Steps to reproduce + description: | + Provide a minimal reproduction. Include a runnable Python snippet or a + `curl` command, your config.yaml if relevant, and the exact LiteLLM + version + Python version. Reports without a runnable reproduction are + auto-closed. placeholder: | - 1. config.yaml file/ .env file/ etc. - 2. Run the following code... - 3. Observe the error... + 1. Create `config.yaml` with: ... + 2. Start the proxy with: `litellm --config config.yaml --port 4000` + 3. Run this Python / curl: ... + 4. Observe: ... value: | 1. 2. @@ -44,9 +59,15 @@ body: - type: textarea id: logs attributes: - label: Relevant log output - description: Please copy and paste any relevant log output. This will be automatically formatted into code, so no need for backticks. + label: Relevant log output / screenshot + description: | + **Required.** Paste the full traceback, stderr, proxy logs, or attach a + screenshot showing the bug. For UI bugs a screenshot or screen + recording is mandatory. Without proof of the bug, the issue is + auto-closed. render: shell + validations: + required: true - type: dropdown id: component attributes: @@ -63,14 +84,14 @@ body: - type: input id: version attributes: - label: What LiteLLM version are you on ? + label: What LiteLLM version are you on ? placeholder: v1.53.1 validations: required: true - type: input id: contact attributes: - label: Twitter / LinkedIn details + label: Twitter / LinkedIn details description: We announce new features on Twitter + LinkedIn. If this issue leads to an announcement, and you'd like a mention, we'll gladly shout you out! placeholder: ex. @krrish_dh / https://www.linkedin.com/in/krish-d/ validations: diff --git a/.github/ISSUE_TEMPLATE/feature_request.yml b/.github/ISSUE_TEMPLATE/feature_request.yml index 4cc42901897..9844032a98b 100644 --- a/.github/ISSUE_TEMPLATE/feature_request.yml +++ b/.github/ISSUE_TEMPLATE/feature_request.yml @@ -1,4 +1,4 @@ -name: 🚀 Feature Request +name: 🚀 Feature Request description: Submit a proposal/request for a new LiteLLM feature. title: "[Feature]: " labels: ["enhancement"] @@ -6,7 +6,10 @@ body: - type: markdown attributes: value: | - Thanks for making LiteLLM better! + Thanks for making LiteLLM better! + + > ⚠️ **Auto-triage notice for external contributors:** + > Feature requests need (1) a clear description of the proposed feature, (2) the motivation / use case with a concrete example, and (3) what success looks like. Vague requests are auto-closed by our LLM triage bot with an explanation. Fill in the missing details and reopen at any time — the bot will re-evaluate. Internal BerriAI contributors are exempt. - type: checkboxes id: duplicate-check attributes: @@ -18,16 +21,25 @@ body: - type: textarea id: the-feature attributes: - label: The Feature - description: A clear and concise description of the feature proposal - placeholder: Tell us what you want! + label: The feature + description: A clear and concise description of the feature proposal. What should LiteLLM do that it doesn't today? + placeholder: e.g. "Support per-team max_input_tokens overrides on the proxy." validations: required: true - type: textarea id: motivation attributes: - label: Motivation, pitch - description: Please outline the motivation for the proposal. Is your feature request related to a specific problem? e.g., "I'm working on X and would like Y to be possible". If this is related to another GitHub issue, please link here too. + label: Motivation, pitch, and concrete example + description: | + **Required.** Why is this needed? Include a concrete use case — what + you're trying to accomplish, what's blocked today, and what success + would look like (ideally with an example config / API call / UI flow). + If this is related to another GitHub issue, link it here too. + placeholder: | + I'm running a multi-tenant proxy where team A processes long docs and + team B only does short chats. Today I have to spin up two proxies. + With this feature I could set max_input_tokens per team and route in one + proxy. Example config: ... validations: required: true - type: dropdown @@ -56,7 +68,7 @@ body: - type: input id: contact attributes: - label: Twitter / LinkedIn details + label: Twitter / LinkedIn details description: We announce new features on Twitter + LinkedIn. When this is announced, and you'd like a mention, we'll gladly shout you out! placeholder: ex. @krrish_dh / https://www.linkedin.com/in/krish-d/ validations: diff --git a/.github/pull_request_template.md b/.github/pull_request_template.md index f9ce9e5dcb8..b6feb076779 100644 --- a/.github/pull_request_template.md +++ b/.github/pull_request_template.md @@ -1,10 +1,57 @@ + + ## Relevant issues - + ## Linear ticket - + + +## Problem description + + + +## Expected vs. actual behavior + + + +## QA proof + + ## Pre-Submission checklist @@ -36,13 +83,6 @@ If you're seeing a delay in your PR being merged, ping the LiteLLM Team on [Slac - [ ] **Merge / cherry-pick CI run** Links: -## Screenshots / Proof of Fix - - - ## Type From 7b4a09353ebfd6d4c4d4c9326b69d0a8aa485d6a Mon Sep 17 00:00:00 2001 From: Cursor Agent Date: Sun, 17 May 2026 16:25:23 +0000 Subject: [PATCH 03/17] feat(triage): Agent Shin LLM-as-judge for external PRs and issues MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Adds a new triage flow that evaluates external pull requests and issues against the project's contribution rubric and, when configured to do so, auto-closes non-conforming ones with an explanatory comment. Contributors can update + reopen to be re-evaluated. Scope: - Internal BerriAI contributors (author_association OWNER/MEMBER/COLLABORATOR) and bot accounts are skipped entirely. - 'Fixes #1234' / 'Resolves https://github.com/.../issues/N' in the PR body short-circuits to PASS without burning LLM tokens. - LLM judge returns structured JSON (verdict, missing[], explanation); parser tolerates markdown fences and embedded JSON. - LLM errors NEVER close PRs/issues — failure surfaces as 'skip-llm-error'. Safety: - pull_request_target / issues triggers are FORCED dry-run in the workflow; only manual workflow_dispatch with close=true (and AGENT_SHIN_ENABLED=true) takes destructive action. - Default mode writes verdicts to GITHUB_STEP_SUMMARY only — no public comments until the team flips the AGENT_SHIN_ENABLED repo variable. - LLM uses an OpenAI-compatible endpoint (model and base URL configurable via repo variables; key via OPENAI_API_KEY secret). Files: - .github/scripts/triage_with_llm.py - judge orchestrator + CLI - .github/workflows/triage_pr_with_llm.yml - .github/workflows/triage_issue_with_llm.yml - tests/test_litellm/test_github_triage_with_llm.py - 33 unit tests End-to-end validated against four real PRs (#28117 internal collaborator, #28108 bot, #28129 'Fixes #28128', #28116 no linked issue) and issue #28132 with a stubbed LLM judge: each path produces the expected action. Co-authored-by: Mateo Wang --- .github/scripts/triage_with_llm.py | 567 ++++++++++++++++++ .github/workflows/triage_issue_with_llm.yml | 75 +++ .github/workflows/triage_pr_with_llm.yml | 89 +++ .../test_github_triage_with_llm.py | 357 +++++++++++ 4 files changed, 1088 insertions(+) create mode 100644 .github/scripts/triage_with_llm.py create mode 100644 .github/workflows/triage_issue_with_llm.yml create mode 100644 .github/workflows/triage_pr_with_llm.yml create mode 100644 tests/test_litellm/test_github_triage_with_llm.py diff --git a/.github/scripts/triage_with_llm.py b/.github/scripts/triage_with_llm.py new file mode 100644 index 00000000000..f8ca780ee4c --- /dev/null +++ b/.github/scripts/triage_with_llm.py @@ -0,0 +1,567 @@ +#!/usr/bin/env python3 +""" +Agent Shin — LLM-as-judge triage for external OSS pull requests and issues. + +Evaluates a single PR or issue against the contribution rubric and, when the +LLM judge marks it as failing, posts an explanatory comment + closes the +PR/issue. Re-triggers on `reopened` so contributors can iterate back in by +filling in the missing pieces and reopening. + +Internal BerriAI contributors (`author_association` in {OWNER, MEMBER, +COLLABORATOR}) and bot accounts are skipped entirely. + +Usage: + triage_with_llm.py --repo owner/repo --pr 1234 + triage_with_llm.py --repo owner/repo --issue 5678 + triage_with_llm.py --repo owner/repo --pr 1234 --close # actually close + triage_with_llm.py --repo owner/repo --pr 1234 --print-prompt # show prompt + +Defaults are SAFE: without `--close` the script writes a verdict to stdout (and, +when running in GitHub Actions, to $GITHUB_STEP_SUMMARY) but takes no GitHub +write actions. + +Environment: + GH_TOKEN / GITHUB_TOKEN - for `gh` CLI auth (auto-set in Actions) + OPENAI_API_KEY - required when --close is passed + OPENAI_BASE_URL - optional (route to any OpenAI-compatible API) + TRIAGE_MODEL - optional model override (default: gpt-4o-mini) +""" + +from __future__ import annotations + +import argparse +import json +import os +import re +import subprocess +import sys +import textwrap +from typing import Any + +DEFAULT_MODEL = "gpt-4o-mini" + +INTERNAL_ASSOCIATIONS = frozenset({"OWNER", "MEMBER", "COLLABORATOR"}) + +# Regexes for picking off "obvious passes" without burning LLM tokens. +LINKED_ISSUE_PATTERN = re.compile( + r"\b(?:fixes|fix|closes|close|resolves|resolve|refs|ref|see|addresses)\s+" + r"(?:#\d+|https?://github\.com/[\w.-]+/[\w.-]+/issues/\d+)", + re.IGNORECASE, +) +HTML_COMMENT_PATTERN = re.compile(r"", re.DOTALL) + + +# --------------------------------------------------------------------------- +# gh helpers + + +def gh(*args: str) -> str: + """Run a `gh` CLI command and return stdout. Raises on non-zero exit.""" + result = subprocess.run( + ["gh", *args], + capture_output=True, + text=True, + check=True, + ) + return result.stdout + + +def fetch_pr(repo: str, number: int) -> dict: + """Return the full GitHub REST representation of a PR.""" + return json.loads(gh("api", f"repos/{repo}/pulls/{number}")) + + +def fetch_issue(repo: str, number: int) -> dict: + """Return the full GitHub REST representation of an issue.""" + return json.loads(gh("api", f"repos/{repo}/issues/{number}")) + + +def post_comment(repo: str, number: int, body: str) -> None: + """Post an issue-style comment (works for both issues and PRs).""" + gh( + "api", + f"repos/{repo}/issues/{number}/comments", + "-X", + "POST", + "-f", + f"body={body}", + ) + + +def close_pr(repo: str, number: int) -> None: + """Close a pull request (state=closed).""" + gh( + "api", + f"repos/{repo}/pulls/{number}", + "-X", + "PATCH", + "-f", + "state=closed", + ) + + +def close_issue(repo: str, number: int, *, not_planned: bool = True) -> None: + """Close an issue, marking state_reason=not_planned by default.""" + args = [ + "api", + f"repos/{repo}/issues/{number}", + "-X", + "PATCH", + "-f", + "state=closed", + ] + if not_planned: + args.extend(["-f", "state_reason=not_planned"]) + gh(*args) + + +# --------------------------------------------------------------------------- +# Author classification + + +def is_internal_contributor(item: dict) -> bool: + """Return True if the PR/issue author should be exempted from triage.""" + association = (item.get("author_association") or "").upper() + if association in INTERNAL_ASSOCIATIONS: + return True + login = ((item.get("user") or {}).get("login") or "").lower() + if login.endswith("[bot]") or login in {"dependabot", "github-actions"}: + return True + return False + + +# --------------------------------------------------------------------------- +# Prompt construction + + +def strip_html_comments(text: str) -> str: + """Remove HTML comments — template placeholder text shouldn't fool the judge.""" + return HTML_COMMENT_PATTERN.sub("", text or "") + + +def has_linked_issue(text: str) -> bool: + """Heuristic: does this body link to an open issue (Fixes #123 etc.)?""" + return bool(LINKED_ISSUE_PATTERN.search(strip_html_comments(text or ""))) + + +def build_pr_prompt(*, title: str, body: str) -> str: + cleaned_body = strip_html_comments(body or "").strip() or "(empty)" + return textwrap.dedent( + f""" + You are "Agent Shin", the OSS triage bot for the LiteLLM open-source + repository (BerriAI/litellm). Decide whether this external pull request + meets the project's contribution standards. + + The PR PASSES triage if it satisfies AT LEAST ONE of: + + (A) It links to a related GitHub issue. Acceptable forms: + "Fixes #1234", "Closes #1234", "Resolves #1234", + "Refs https://github.com/BerriAI/litellm/issues/1234". A bare + issue number without a closing keyword counts only if it's + clearly the related issue (not a passing mention). + + (B) The PR body contains ALL of: + - A clear problem description (what bug or missing feature this + addresses, beyond the title). + - Expected vs. actual behavior (or, for features, "what's + possible now vs. with this PR"). + - Visual QA proof: before/after screenshots, a screen recording, + terminal output, log output, or test output demonstrating the + fix or feature works end-to-end. Saying "I tested it" is NOT + proof. + + Bias toward PASS when the PR has structure and context — only FAIL when + the body is empty, copy-paste filler from the template, or genuinely + missing both a linked issue AND the core elements of (B). + + Respond with a single JSON object, no prose: + + {{ + "verdict": "pass" | "fail", + "linked_issue": boolean, + "has_problem_description": boolean, + "has_expected_vs_actual": boolean, + "has_qa_proof": boolean, + "missing": ["plain-english strings naming what is missing"], + "explanation": "1-2 sentence reasoning for the team to skim" + }} + + --- + PR title: {title} + + PR body: + --- + {cleaned_body} + --- + """ + ).strip() + + +def build_issue_prompt(*, title: str, body: str) -> str: + cleaned_body = strip_html_comments(body or "").strip() or "(empty)" + return textwrap.dedent( + f""" + You are "Agent Shin", the OSS triage bot for the LiteLLM open-source + repository (BerriAI/litellm). Decide whether this GitHub issue meets + the project's reporting standards. + + For a BUG REPORT the issue PASSES triage when it contains ALL of: + - A clear reproduction (steps, runnable code snippet, curl command, + or example config the maintainer can paste into their machine). + - Screenshot, terminal output, traceback, or log output as proof of + the bug. + - Expected vs. actual behavior. + + For a FEATURE REQUEST the issue PASSES triage when it contains ALL of: + - A clear description of the proposed feature (what should LiteLLM do + that it does not today). + - Motivation / use case with a concrete example (config, API call, + UI flow, or scenario showing what's blocked today). + + Bias toward PASS when the issue has structure and context — only FAIL + when the body is empty, copy-paste template placeholder text, or a + one-line "X is broken" with no detail. Asking clarifying questions is + OK content; mark such issues PASS. + + Respond with a single JSON object, no prose: + + {{ + "verdict": "pass" | "fail", + "kind": "bug" | "feature" | "other", + "has_repro": boolean, + "has_proof": boolean, + "has_expected_vs_actual": boolean, + "has_motivation_example": boolean, + "missing": ["plain-english strings naming what is missing"], + "explanation": "1-2 sentence reasoning for the team to skim" + }} + + --- + Issue title: {title} + + Issue body: + --- + {cleaned_body} + --- + """ + ).strip() + + +# --------------------------------------------------------------------------- +# LLM call + verdict parsing + + +def call_llm_judge( + prompt: str, *, model: str, api_key: str, base_url: str | None +) -> str: + """Call an OpenAI-compatible chat completions endpoint. Returns raw text.""" + # Import inside the function so unit tests that monkey-patch this never + # need the openai package installed. + from openai import OpenAI + + client = ( + OpenAI(api_key=api_key, base_url=base_url) + if base_url + else OpenAI(api_key=api_key) + ) + response = client.chat.completions.create( + model=model, + messages=[{"role": "user", "content": prompt}], + temperature=0, + response_format={"type": "json_object"}, + ) + return response.choices[0].message.content or "" + + +def parse_verdict(raw: str) -> dict: + """Parse the LLM's JSON response. Tolerates ```json fences and stray text.""" + if not raw: + raise ValueError("empty LLM response") + text = raw.strip() + if text.startswith("```"): + text = re.sub(r"^```(?:json)?\s*", "", text) + text = re.sub(r"\s*```$", "", text) + try: + return json.loads(text) + except json.JSONDecodeError: + match = re.search(r"\{.*\}", text, re.DOTALL) + if not match: + raise ValueError(f"could not extract JSON from LLM response: {raw[:200]}") + return json.loads(match.group(0)) + + +# --------------------------------------------------------------------------- +# Comment composition + + +def _format_missing(missing: list[str]) -> str: + if not missing: + return "- (see explanation below)" + return "\n".join(f"- {m}" for m in missing) + + +def format_pr_close_comment(verdict: dict) -> str: + missing_lines = _format_missing(verdict.get("missing") or []) + explanation = verdict.get("explanation") or "" + return ( + "👋 Hi, thanks for the PR! I'm **Agent Shin**, the automated triage bot for this repository.\n" + "\n" + "This PR is being **auto-closed** because it does not yet meet the bar described in our " + "[pull-request template](https://github.com/BerriAI/litellm/blob/main/.github/pull_request_template.md). " + "Specifically, I couldn't find:\n" + "\n" + f"{missing_lines}\n" + "\n" + f"> {explanation}\n" + "\n" + "**This isn't a rejection of the idea.** To bring this PR back:\n" + "\n" + "1. Update the PR description to either:\n" + " - Link a related GitHub issue (e.g. `Fixes #1234`), OR\n" + " - Add a clear **problem description**, **expected vs. actual behavior**, and **visual QA proof** " + "(before/after screenshots, a short screen recording, or terminal/log output).\n" + "2. **Reopen** the PR (or open a fresh one) — I'll re-evaluate automatically.\n" + "\n" + "Internal BerriAI contributors: this rubric doesn't apply to you — ping a maintainer.\n" + "\n" + "_(I'm an LLM, so I'm not infallible. If you think I got this wrong, reopen and ping a maintainer — " + "they'll override me.)_" + ) + + +def format_issue_close_comment(verdict: dict) -> str: + missing_lines = _format_missing(verdict.get("missing") or []) + explanation = verdict.get("explanation") or "" + return ( + "👋 Hi, thanks for filing this! I'm **Agent Shin**, the automated triage bot for this repository.\n" + "\n" + "This issue is being **auto-closed** because it doesn't yet have enough detail for a maintainer to act on. " + "Specifically, I couldn't find:\n" + "\n" + f"{missing_lines}\n" + "\n" + f"> {explanation}\n" + "\n" + "**This isn't a \"won't fix\".** To bring this issue back:\n" + "\n" + "1. Edit the issue to add the missing pieces:\n" + " - For **bug reports**: a runnable reproduction (code / curl / config), expected vs. actual behavior, " + "and a screenshot / traceback / log showing the bug.\n" + " - For **feature requests**: a concrete description of what should change, plus a use case and example " + "(config / API call / UI flow).\n" + "2. **Reopen** the issue — I'll re-evaluate automatically.\n" + "\n" + "Internal BerriAI contributors: this rubric doesn't apply to you — ping a maintainer.\n" + "\n" + "_(I'm an LLM, so I'm not infallible. If you think I got this wrong, reopen and ping a maintainer — " + "they'll override me.)_" + ) + + +# --------------------------------------------------------------------------- +# Step-summary helpers + + +def write_step_summary(content: str) -> None: + """When running inside GitHub Actions, append to the step summary file.""" + path = os.environ.get("GITHUB_STEP_SUMMARY") + if not path: + return + try: + with open(path, "a", encoding="utf-8") as handle: + handle.write(content) + if not content.endswith("\n"): + handle.write("\n") + except OSError as exc: + print(f"warn: failed to write step summary: {exc}", file=sys.stderr) + + +# --------------------------------------------------------------------------- +# Core orchestration + + +def triage( + *, + repo: str, + kind: str, + number: int, + close: bool, + model: str, + judge: Any = None, + print_prompt: bool = False, +) -> dict: + """Triage a single PR or issue. Returns a result dict for logging/tests. + + `judge` is an optional callable `(prompt) -> str` for tests / dry-run with + a stub. In production, leave it None and the script uses `call_llm_judge`. + """ + fetcher = {"pr": fetch_pr, "issue": fetch_issue}[kind] + item = fetcher(repo, number) + + title = item.get("title") or "" + body = item.get("body") or "" + login = (item.get("user") or {}).get("login") or "" + association = item.get("author_association") or "" + state = item.get("state") or "" + + base_result = { + "kind": kind, + "number": number, + "title": title, + "author": login, + "author_association": association, + "state": state, + } + + if state != "open": + return {**base_result, "action": "skip-not-open"} + + if is_internal_contributor(item): + return {**base_result, "action": "skip-internal-author"} + + if kind == "pr": + prompt = build_pr_prompt(title=title, body=body) + # Short-circuit: if body very clearly links a related issue, just pass. + if has_linked_issue(body): + return { + **base_result, + "action": "pass-linked-issue", + "verdict": { + "verdict": "pass", + "linked_issue": True, + "explanation": "Linked-issue regex matched; LLM was not called.", + }, + } + else: + prompt = build_issue_prompt(title=title, body=body) + + if print_prompt: + return {**base_result, "action": "print-prompt", "prompt": prompt} + + if judge is None: + api_key = os.environ.get("OPENAI_API_KEY") + if not api_key: + # No key configured — never take a destructive action. Report skip. + return { + **base_result, + "action": "skip-no-llm-key", + "prompt_preview": prompt[:200], + } + base_url = os.environ.get("OPENAI_BASE_URL") or None + + def judge(p: str) -> str: + return call_llm_judge(p, model=model, api_key=api_key, base_url=base_url) + + try: + raw = judge(prompt) + verdict = parse_verdict(raw) + except Exception as exc: # noqa: BLE001 - judge errors must never close PRs + return {**base_result, "action": "skip-llm-error", "error": str(exc)} + + decision = (verdict.get("verdict") or "").lower() + if decision != "fail": + return {**base_result, "action": "pass-llm", "verdict": verdict} + + if not close: + return {**base_result, "action": "would-close", "verdict": verdict} + + comment_body = ( + format_pr_close_comment(verdict) + if kind == "pr" + else format_issue_close_comment(verdict) + ) + post_comment(repo, number, comment_body) + if kind == "pr": + close_pr(repo, number) + else: + close_issue(repo, number) + + return { + **base_result, + "action": "closed", + "verdict": verdict, + "comment": comment_body, + } + + +# --------------------------------------------------------------------------- +# CLI + + +def render_summary(result: dict) -> str: + """Render a human-readable summary block (used for stdout + step summary).""" + lines = ["## Agent Shin verdict", ""] + lines.append( + f"- **{result['kind'].upper()} #{result['number']}**: {result.get('title', '')}" + ) + lines.append( + f"- **Author**: `{result.get('author', '')}` ({result.get('author_association', '')})" + ) + lines.append(f"- **State**: {result.get('state', '')}") + lines.append(f"- **Action**: `{result['action']}`") + verdict = result.get("verdict") + if verdict: + lines.append("") + lines.append("```json") + lines.append(json.dumps(verdict, indent=2)) + lines.append("```") + error = result.get("error") + if error: + lines.append("") + lines.append(f"_LLM error: {error}_") + comment = result.get("comment") + if comment: + lines.append("") + lines.append("### Would post comment:") + lines.append("") + lines.append("> " + comment.replace("\n", "\n> ")) + return "\n".join(lines) + + +def main() -> int: + parser = argparse.ArgumentParser(description=__doc__) + parser.add_argument("--repo", required=True, help="Repository (owner/repo).") + target = parser.add_mutually_exclusive_group(required=True) + target.add_argument("--pr", type=int, help="Pull request number to triage.") + target.add_argument("--issue", type=int, help="Issue number to triage.") + parser.add_argument( + "--close", + action="store_true", + help="Actually post comment + close on fail (default: dry run).", + ) + parser.add_argument( + "--model", + default=os.environ.get("TRIAGE_MODEL", DEFAULT_MODEL), + help=f"OpenAI-compatible model name (default: {DEFAULT_MODEL}).", + ) + parser.add_argument( + "--print-prompt", + action="store_true", + help="Print the prompt that would be sent to the judge and exit.", + ) + args = parser.parse_args() + + kind = "pr" if args.pr is not None else "issue" + number = args.pr if args.pr is not None else args.issue + + result = triage( + repo=args.repo, + kind=kind, + number=number, + close=args.close, + model=args.model, + print_prompt=args.print_prompt, + ) + + if result.get("action") == "print-prompt": + print(result["prompt"]) + return 0 + + summary = render_summary(result) + print(summary) + write_step_summary(summary + "\n") + return 0 + + +if __name__ == "__main__": + sys.exit(main()) diff --git a/.github/workflows/triage_issue_with_llm.yml b/.github/workflows/triage_issue_with_llm.yml new file mode 100644 index 00000000000..c9ddad5d3ea --- /dev/null +++ b/.github/workflows/triage_issue_with_llm.yml @@ -0,0 +1,75 @@ +name: Agent Shin — Issue triage + +# LLM-as-judge triage for external GitHub issues. +# +# DRY-RUN BY DEFAULT. See .github/workflows/triage_pr_with_llm.yml for the +# enablement procedure — same repo variable (`AGENT_SHIN_ENABLED=true`) +# unlocks the PR and issue triage flows together. + +on: + issues: + types: [opened, reopened] + workflow_dispatch: + inputs: + issue_number: + description: "Issue number to triage manually." + required: true + close: + description: "If true and AGENT_SHIN_ENABLED=true, actually close on fail." + required: false + default: "false" + type: choice + options: + - "true" + - "false" + +permissions: + contents: read + issues: write + +jobs: + triage: + 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 Agent Shin + env: + GH_TOKEN: ${{ secrets.GITHUB_TOKEN }} + OPENAI_API_KEY: ${{ secrets.OPENAI_API_KEY }} + OPENAI_BASE_URL: ${{ vars.OPENAI_BASE_URL }} + TRIAGE_MODEL: ${{ vars.TRIAGE_MODEL }} + AGENT_SHIN_ENABLED: ${{ vars.AGENT_SHIN_ENABLED }} + DISPATCH_CLOSE: ${{ github.event.inputs.close }} + ISSUE_NUMBER: ${{ github.event.issue.number || github.event.inputs.issue_number }} + run: | + set -euo pipefail + ARGS=(--repo "${{ github.repository }}" --issue "${ISSUE_NUMBER}") + if [ "${AGENT_SHIN_ENABLED:-false}" = "true" ] && [ "${DISPATCH_CLOSE:-false}" != "false" ]; then + ARGS+=(--close) + echo "::notice::Agent Shin is ENABLED and running in close-on-fail mode." + elif [ "${AGENT_SHIN_ENABLED:-false}" = "true" ]; then + echo "::notice::Agent Shin is ENABLED but this trigger is dry-run." + else + echo "::notice::Agent Shin is in DRY-RUN mode (AGENT_SHIN_ENABLED is not 'true'). No comments will be posted; no issues will be closed." + fi + # Automatic `issues` events stay dry-run regardless until the team + # explicitly invokes workflow_dispatch with close=true. + if [ "${GITHUB_EVENT_NAME:-}" = "issues" ]; then + ARGS=("${ARGS[@]/--close/}") + echo "::notice::issues trigger -> forcing dry-run." + fi + python3 .github/scripts/triage_with_llm.py "${ARGS[@]}" diff --git a/.github/workflows/triage_pr_with_llm.yml b/.github/workflows/triage_pr_with_llm.yml new file mode 100644 index 00000000000..f9b6d7fed47 --- /dev/null +++ b/.github/workflows/triage_pr_with_llm.yml @@ -0,0 +1,89 @@ +name: Agent Shin — PR triage + +# LLM-as-judge triage for external pull requests. +# +# DRY-RUN BY DEFAULT. Closures and public comments are gated on the repo +# variable `AGENT_SHIN_ENABLED` being set to the string `"true"`. Until then, +# every run only writes its verdict to the workflow step summary so the team +# can QA the judge's decisions before flipping it on. +# +# To enable for real: +# 1. Add a repo secret `OPENAI_API_KEY` (or compatible). +# 2. Set repo variable `AGENT_SHIN_ENABLED` to `true` +# (Settings > Secrets and variables > Actions > Variables). +# +# We use `pull_request_target` so the workflow has access to repo secrets +# and runs against PRs from forks. We never check out fork code — only read +# PR metadata via `gh api`, so this is safe. + +on: + pull_request_target: + types: [opened, reopened] + workflow_dispatch: + inputs: + pr_number: + description: "PR number to triage manually." + required: true + close: + description: "If true and AGENT_SHIN_ENABLED=true, actually close on fail." + required: false + default: "false" + type: choice + options: + - "true" + - "false" + +permissions: + contents: read + issues: write + pull-requests: write + +jobs: + triage: + 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 Agent Shin + env: + GH_TOKEN: ${{ secrets.GITHUB_TOKEN }} + OPENAI_API_KEY: ${{ secrets.OPENAI_API_KEY }} + OPENAI_BASE_URL: ${{ vars.OPENAI_BASE_URL }} + TRIAGE_MODEL: ${{ vars.TRIAGE_MODEL }} + AGENT_SHIN_ENABLED: ${{ vars.AGENT_SHIN_ENABLED }} + DISPATCH_CLOSE: ${{ github.event.inputs.close }} + PR_NUMBER: ${{ github.event.pull_request.number || github.event.inputs.pr_number }} + run: | + set -euo pipefail + ARGS=(--repo "${{ github.repository }}" --pr "${PR_NUMBER}") + if [ "${AGENT_SHIN_ENABLED:-false}" = "true" ] && [ "${DISPATCH_CLOSE:-false}" != "false" ]; then + ARGS+=(--close) + echo "::notice::Agent Shin is ENABLED and running in close-on-fail mode." + elif [ "${AGENT_SHIN_ENABLED:-false}" = "true" ]; then + echo "::notice::Agent Shin is ENABLED but this trigger is dry-run (workflow_dispatch close=false or scheduled event)." + else + echo "::notice::Agent Shin is in DRY-RUN mode (AGENT_SHIN_ENABLED is not 'true'). No comments will be posted; no PRs will be closed." + fi + # On the scheduled/automatic pull_request_target trigger we default to + # dry-run regardless, so the team can review verdicts in the step + # summary before any contributor sees a comment. Only the manual + # workflow_dispatch path (with close=true) closes PRs. + if [ "${GITHUB_EVENT_NAME:-}" = "pull_request_target" ]; then + # strip any --close added above + ARGS=("${ARGS[@]/--close/}") + echo "::notice::pull_request_target trigger -> forcing dry-run." + fi + python3 .github/scripts/triage_with_llm.py "${ARGS[@]}" diff --git a/tests/test_litellm/test_github_triage_with_llm.py b/tests/test_litellm/test_github_triage_with_llm.py new file mode 100644 index 00000000000..de72cc8d7c9 --- /dev/null +++ b/tests/test_litellm/test_github_triage_with_llm.py @@ -0,0 +1,357 @@ +"""Unit tests for `.github/scripts/triage_with_llm.py` (Agent Shin).""" + +from __future__ import annotations + +import importlib.util +import json +import sys +from pathlib import Path + +import pytest + +SCRIPT_PATH = ( + Path(__file__).resolve().parents[2] / ".github" / "scripts" / "triage_with_llm.py" +) + + +@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 TestIsInternalContributor: + @pytest.mark.parametrize("association", ["OWNER", "MEMBER", "COLLABORATOR"]) + def test_should_mark_org_associations_as_internal(self, triage_module, association): + item = { + "author_association": association, + "user": {"login": "krrishdholakia"}, + } + assert triage_module.is_internal_contributor(item) is True + + @pytest.mark.parametrize( + "association", + ["CONTRIBUTOR", "FIRST_TIME_CONTRIBUTOR", "FIRST_TIMER", "NONE", ""], + ) + def test_should_mark_outside_associations_as_external( + self, triage_module, association + ): + item = { + "author_association": association, + "user": {"login": "random-oss-dev"}, + } + assert triage_module.is_internal_contributor(item) is False + + @pytest.mark.parametrize( + "login", + ["dependabot[bot]", "greptile-apps[bot]", "dependabot", "github-actions"], + ) + def test_should_skip_bot_accounts_regardless_of_association( + self, triage_module, login + ): + item = {"author_association": "NONE", "user": {"login": login}} + assert triage_module.is_internal_contributor(item) is True + + +class TestHasLinkedIssue: + @pytest.mark.parametrize( + "body", + [ + "Fixes #1234", + "closes #1", + "Resolves #99", + "fix #42 — this addresses the regression", + "Refs https://github.com/BerriAI/litellm/issues/27000", + ], + ) + def test_should_detect_common_link_phrases(self, triage_module, body): + assert triage_module.has_linked_issue(body) is True + + @pytest.mark.parametrize( + "body", + [ + "", + "Some change", + "See #1234", # "see" is allowed per regex but we want documented coverage + ], + ) + def test_should_handle_empty_and_unrelated_bodies(self, triage_module, body): + # "See #1234" is intentionally accepted as a related-issue reference. + # Just make sure empty/unrelated bodies don't crash. + triage_module.has_linked_issue(body) + + def test_should_not_detect_when_only_html_comment_template(self, triage_module): + body = "" + assert triage_module.has_linked_issue(body) is False + + +class TestStripHtmlComments: + def test_should_remove_single_line_comments(self, triage_module): + text = "before after" + assert "placeholder" not in triage_module.strip_html_comments(text) + + def test_should_remove_multiline_comments(self, triage_module): + text = "kept\n\nkept2" + cleaned = triage_module.strip_html_comments(text) + assert "Fixes #1" not in cleaned + assert "kept" in cleaned and "kept2" in cleaned + + def test_should_handle_none(self, triage_module): + assert triage_module.strip_html_comments(None) == "" + + +class TestParseVerdict: + def test_should_parse_plain_json(self, triage_module): + raw = '{"verdict": "pass", "missing": []}' + assert triage_module.parse_verdict(raw)["verdict"] == "pass" + + def test_should_strip_markdown_fence(self, triage_module): + raw = '```json\n{"verdict": "fail", "missing": ["foo"]}\n```' + result = triage_module.parse_verdict(raw) + assert result["verdict"] == "fail" + assert result["missing"] == ["foo"] + + def test_should_extract_embedded_json_from_prose(self, triage_module): + raw = 'Here you go: {"verdict": "pass", "missing": []}\nThanks.' + assert triage_module.parse_verdict(raw)["verdict"] == "pass" + + def test_should_raise_for_unparseable_text(self, triage_module): + with pytest.raises(ValueError): + triage_module.parse_verdict("not even close to json") + + def test_should_raise_for_empty(self, triage_module): + with pytest.raises(ValueError): + triage_module.parse_verdict("") + + +class TestBuildPrompts: + def test_should_include_pr_title_and_body(self, triage_module): + prompt = triage_module.build_pr_prompt( + title="Add foo", body=" Real body" + ) + assert "Add foo" in prompt + assert "Real body" in prompt + assert "comment" not in prompt # HTML comments are stripped + + def test_should_show_empty_marker_for_empty_pr_body(self, triage_module): + prompt = triage_module.build_pr_prompt(title="t", body="") + assert "(empty)" in prompt + + def test_should_include_issue_title_and_body(self, triage_module): + prompt = triage_module.build_issue_prompt(title="Bug", body="repro here") + assert "Bug" in prompt + assert "repro here" in prompt + + +class TestTriageOrchestration: + """End-to-end-ish tests that mock both gh fetchers and the LLM.""" + + def _make_pr(self, **overrides): + base = { + "number": 1, + "title": "PR title", + "body": "PR body", + "state": "open", + "author_association": "NONE", + "user": {"login": "outside-dev"}, + } + base.update(overrides) + return base + + def test_should_skip_internal_author(self, triage_module, monkeypatch): + pr = self._make_pr( + author_association="MEMBER", user={"login": "krrishdholakia"} + ) + monkeypatch.setattr(triage_module, "fetch_pr", lambda repo, n: pr) + + def boom(*a, **kw): + pytest.fail("LLM should not be called for internal authors") + + result = triage_module.triage( + repo="o/r", + kind="pr", + number=1, + close=True, + model="m", + judge=boom, + ) + assert result["action"] == "skip-internal-author" + + def test_should_skip_closed_pr(self, triage_module, monkeypatch): + pr = self._make_pr(state="closed") + monkeypatch.setattr(triage_module, "fetch_pr", lambda repo, n: pr) + result = triage_module.triage( + repo="o/r", + kind="pr", + number=1, + close=True, + model="m", + judge=lambda p: pytest.fail("should not run on closed PRs"), + ) + assert result["action"] == "skip-not-open" + + def test_should_short_circuit_on_linked_issue(self, triage_module, monkeypatch): + pr = self._make_pr(body="Fixes #1234\n\nFoo bar") + monkeypatch.setattr(triage_module, "fetch_pr", lambda repo, n: pr) + result = triage_module.triage( + repo="o/r", + kind="pr", + number=1, + close=True, + model="m", + judge=lambda p: pytest.fail("LLM should not be called"), + ) + assert result["action"] == "pass-linked-issue" + assert result["verdict"]["verdict"] == "pass" + + def test_should_return_pass_llm_when_judge_passes(self, triage_module, monkeypatch): + pr = self._make_pr(body="Long body, no linked issue.") + monkeypatch.setattr(triage_module, "fetch_pr", lambda repo, n: pr) + captured = {} + + def judge(prompt): + captured["prompt"] = prompt + return json.dumps({"verdict": "pass", "missing": [], "explanation": "ok"}) + + result = triage_module.triage( + repo="o/r", kind="pr", number=1, close=True, model="m", judge=judge + ) + assert result["action"] == "pass-llm" + assert "Long body" in captured["prompt"] + + def test_should_return_would_close_in_dry_run(self, triage_module, monkeypatch): + pr = self._make_pr(body="just a sentence.") + monkeypatch.setattr(triage_module, "fetch_pr", lambda repo, n: pr) + + def fake_post(*a, **kw): + pytest.fail("should not post comments in dry-run") + + def fake_close(*a, **kw): + pytest.fail("should not close in dry-run") + + monkeypatch.setattr(triage_module, "post_comment", fake_post) + monkeypatch.setattr(triage_module, "close_pr", fake_close) + + verdict = { + "verdict": "fail", + "missing": ["problem description", "QA proof"], + "explanation": "Body is one sentence.", + } + result = triage_module.triage( + repo="o/r", + kind="pr", + number=1, + close=False, + model="m", + judge=lambda p: json.dumps(verdict), + ) + assert result["action"] == "would-close" + assert result["verdict"]["missing"] == ["problem description", "QA proof"] + + def test_should_post_comment_and_close_when_close_enabled( + self, triage_module, monkeypatch + ): + pr = self._make_pr(body="just a sentence.") + monkeypatch.setattr(triage_module, "fetch_pr", lambda repo, n: pr) + posted = {} + closed = {} + monkeypatch.setattr( + triage_module, + "post_comment", + lambda repo, n, body: posted.update({"repo": repo, "n": n, "body": body}), + ) + monkeypatch.setattr( + triage_module, + "close_pr", + lambda repo, n: closed.update({"repo": repo, "n": n}), + ) + + verdict = { + "verdict": "fail", + "missing": ["QA proof"], + "explanation": "Body too thin.", + } + result = triage_module.triage( + repo="o/r", + kind="pr", + number=42, + close=True, + model="m", + judge=lambda p: json.dumps(verdict), + ) + assert result["action"] == "closed" + assert posted["n"] == 42 and closed["n"] == 42 + assert "Agent Shin" in posted["body"] + assert "QA proof" in posted["body"] + + def test_should_skip_on_llm_error_in_close_mode(self, triage_module, monkeypatch): + pr = self._make_pr(body="something.") + monkeypatch.setattr(triage_module, "fetch_pr", lambda repo, n: pr) + monkeypatch.setattr( + triage_module, + "post_comment", + lambda *a, **kw: pytest.fail("must not comment on LLM error"), + ) + monkeypatch.setattr( + triage_module, + "close_pr", + lambda *a, **kw: pytest.fail("must not close on LLM error"), + ) + + def broken_judge(prompt): + raise RuntimeError("upstream 500") + + result = triage_module.triage( + repo="o/r", + kind="pr", + number=1, + close=True, + model="m", + judge=broken_judge, + ) + assert result["action"] == "skip-llm-error" + assert "upstream 500" in result["error"] + + def test_should_triage_issues_kind(self, triage_module, monkeypatch): + issue = { + "number": 7, + "title": "Bug: X is broken", + "body": "no detail", + "state": "open", + "author_association": "NONE", + "user": {"login": "outside"}, + } + monkeypatch.setattr(triage_module, "fetch_issue", lambda repo, n: issue) + closed = {} + posted = {} + monkeypatch.setattr( + triage_module, + "post_comment", + lambda repo, n, body: posted.update(body=body), + ) + monkeypatch.setattr( + triage_module, "close_issue", lambda repo, n: closed.update(n=n) + ) + + verdict = { + "verdict": "fail", + "kind": "bug", + "has_repro": False, + "missing": ["reproduction", "expected vs. actual"], + "explanation": "No repro provided.", + } + result = triage_module.triage( + repo="o/r", + kind="issue", + number=7, + close=True, + model="m", + judge=lambda p: json.dumps(verdict), + ) + assert result["action"] == "closed" + assert closed["n"] == 7 + assert "reproduction" in posted["body"] From 483042ef84a15a5f46779ba6111bc7d39f768d84 Mon Sep 17 00:00:00 2001 From: Cursor Agent Date: Sun, 17 May 2026 16:25:32 +0000 Subject: [PATCH 04/17] feat(triage): scope Greptile auto-closer to external contributors + dry-run by default - close_low_quality_prs.py now filters by GitHub author_association via the REST API: PRs from OWNER / MEMBER / COLLABORATOR (and bot accounts) are skipped with a new 'skip-internal' summary bucket. - close_low_quality_prs.yml now defaults workflow_dispatch close=false, and ignores 'close=true' unless the new repo variable AGENT_SHIN_ENABLED is set to 'true'. Scheduled runs are dry-run only until the team flips that switch. - Updated unit tests: one new test asserting internal authors are skipped, and an autouse fixture treats unspecified test PRs as external so the rest of the suite still exercises the close path. Co-authored-by: Mateo Wang --- .github/scripts/close_low_quality_prs.py | 44 ++++++++++++++++++- .github/workflows/close_low_quality_prs.yml | 15 +++++-- .../test_github_close_low_quality_prs.py | 28 ++++++++++++ 3 files changed, 83 insertions(+), 4 deletions(-) diff --git a/.github/scripts/close_low_quality_prs.py b/.github/scripts/close_low_quality_prs.py index 0a0ff980201..6338ad380ce 100644 --- a/.github/scripts/close_low_quality_prs.py +++ b/.github/scripts/close_low_quality_prs.py @@ -50,6 +50,10 @@ SCORE_PATTERN = re.compile( re.IGNORECASE, ) +# `author_association` values for internal BerriAI contributors who should be +# exempt from auto-triage. +INTERNAL_AUTHOR_ASSOCIATIONS = frozenset({"OWNER", "MEMBER", "COLLABORATOR"}) + def gh(*args: str) -> str: """Run a `gh` CLI command and return stdout. Raises on non-zero exit.""" @@ -85,6 +89,38 @@ def fetch_open_prs(repo: str | None) -> list[dict]: return json.loads(raw) +def fetch_pr_author_association(pr_number: int, repo: str | None) -> str: + """Return the GitHub `author_association` for a PR, uppercase. + + Values: OWNER, MEMBER, COLLABORATOR, CONTRIBUTOR, FIRST_TIME_CONTRIBUTOR, + FIRST_TIMER, MANNEQUIN, NONE. Returns "" on lookup failure. + """ + endpoint = ( + f"repos/{repo}/pulls/{pr_number}" + if repo + else f"repos/{{owner}}/{{repo}}/pulls/{pr_number}" + ) + try: + data = json.loads(gh("api", endpoint)) + except subprocess.CalledProcessError: + return "" + return (data.get("author_association") or "").upper() + + +def is_external_pr_author(pr: dict, repo: str | None) -> bool: + """Return True if the PR author is an external OSS contributor. + + Internal = `OWNER` / `MEMBER` / `COLLABORATOR` association, or a bot login. + """ + login = ((pr.get("author") or {}).get("login") or "").lower() + if login.endswith("[bot]") or login in {"dependabot", "github-actions"}: + return False + association = fetch_pr_author_association(pr["number"], repo) + if association in INTERNAL_AUTHOR_ASSOCIATIONS: + return False + return True + + def fetch_pr_comments(pr_number: int, repo: str | None) -> list[dict]: """Fetch issue-level comments on a PR (where Greptile posts its summary).""" endpoint = ( @@ -203,7 +239,7 @@ def evaluate_pr( """Decide whether to close `pr`. Returns (action, score_or_none, age_days_or_none) where action is one of: - "skip-draft", "skip-too-young", "skip-optout-label", + "skip-draft", "skip-too-young", "skip-optout-label", "skip-internal", "skip-no-greptile-score", "skip-score-ok", or "close". """ if pr.get("isDraft"): @@ -217,6 +253,11 @@ def evaluate_pr( if age_days < min_age_days: return ("skip-too-young", None, age_days) + # Only auto-close external OSS contributors. Internal contributors + # (BerriAI org members) handle their own backlog. + if not is_external_pr_author(pr, repo): + return ("skip-internal", None, age_days) + comments = fetch_pr_comments(pr["number"], repo) extraction = extract_greptile_score(comments) if extraction is None: @@ -298,6 +339,7 @@ def main() -> int: "skip-draft": 0, "skip-too-young": 0, "skip-optout-label": 0, + "skip-internal": 0, "skip-no-greptile-score": 0, "skip-score-ok": 0, } diff --git a/.github/workflows/close_low_quality_prs.yml b/.github/workflows/close_low_quality_prs.yml index 7768e89c0f8..4271adca184 100644 --- a/.github/workflows/close_low_quality_prs.yml +++ b/.github/workflows/close_low_quality_prs.yml @@ -20,7 +20,7 @@ on: close: description: "Actually close matching PRs (false = dry run)." required: false - default: "true" + default: "false" type: choice options: - "true" @@ -62,7 +62,11 @@ jobs: - name: Run low-quality PR closer env: GH_TOKEN: ${{ secrets.GITHUB_TOKEN }} - CLOSE_FLAG: ${{ github.event.inputs.close || 'true' }} + # Default to dry-run for scheduled triggers as well. The repo + # variable AGENT_SHIN_ENABLED must be "true" before scheduled runs + # actually close PRs, AND workflow_dispatch must opt-in via close=true. + CLOSE_FLAG: ${{ github.event.inputs.close || 'false' }} + AGENT_SHIN_ENABLED: ${{ vars.AGENT_SHIN_ENABLED }} MIN_AGE_DAYS: ${{ github.event.inputs.min_age_days || '7' }} MIN_SCORE: ${{ github.event.inputs.min_score || '4' }} LIMIT: ${{ github.event.inputs.limit || '25' }} @@ -74,7 +78,12 @@ jobs: --min-score "${MIN_SCORE}" --limit "${LIMIT}" ) - if [ "${CLOSE_FLAG}" = "true" ]; then + if [ "${AGENT_SHIN_ENABLED:-false}" != "true" ]; then + echo "::notice::AGENT_SHIN_ENABLED is not 'true' -> forcing dry-run regardless of close input." + elif [ "${CLOSE_FLAG}" = "true" ]; then ARGS+=(--close) + echo "::notice::Running in close-on-fail mode." + else + echo "::notice::AGENT_SHIN_ENABLED is true but close=false -> dry-run." fi python3 .github/scripts/close_low_quality_prs.py "${ARGS[@]}" diff --git a/tests/test_litellm/test_github_close_low_quality_prs.py b/tests/test_litellm/test_github_close_low_quality_prs.py index 2b3119fc920..0d63562ec0f 100644 --- a/tests/test_litellm/test_github_close_low_quality_prs.py +++ b/tests/test_litellm/test_github_close_low_quality_prs.py @@ -139,6 +139,13 @@ class TestEvaluatePr: "url": f"https://example.com/pr/{number}", } + @pytest.fixture(autouse=True) + def _external_author(self, closer_module, monkeypatch): + """Treat every test PR as external unless overridden.""" + monkeypatch.setattr( + closer_module, "is_external_pr_author", lambda pr, repo: True + ) + def test_should_skip_drafts(self, closer_module, _now, monkeypatch): monkeypatch.setattr( closer_module, @@ -263,6 +270,27 @@ class TestEvaluatePr: assert action == "close" assert score == 1 + def test_should_skip_internal_authors(self, closer_module, _now, monkeypatch): + # Override the fixture for this one test. + monkeypatch.setattr( + closer_module, "is_external_pr_author", lambda pr, repo: False + ) + monkeypatch.setattr( + closer_module, + "fetch_pr_comments", + lambda *a, **kw: pytest.fail("should not fetch comments for internal"), + ) + action, score, _ = closer_module.evaluate_pr( + self._make_pr(created_days_ago=14), + now=_now, + min_age_days=7, + min_score=4, + repo=None, + optout_labels=set(), + ) + assert action == "skip-internal" + assert score is None + class TestHasOptoutLabel: def test_should_match_label_case_insensitively(self, closer_module): From 4961bf4eaf67581554b29f9560f786acfd738279 Mon Sep 17 00:00:00 2001 From: Cursor Agent Date: Sun, 17 May 2026 16:37:04 +0000 Subject: [PATCH 05/17] fix(workflows): scheduled cron closes PRs; safe --close strip in triage Co-authored-by: Yassin Kortam --- .github/workflows/close_low_quality_prs.yml | 9 +++++---- .github/workflows/triage_issue_with_llm.yml | 10 +++++++++- .github/workflows/triage_pr_with_llm.yml | 12 ++++++++++-- 3 files changed, 24 insertions(+), 7 deletions(-) diff --git a/.github/workflows/close_low_quality_prs.yml b/.github/workflows/close_low_quality_prs.yml index 4271adca184..57f74399cc1 100644 --- a/.github/workflows/close_low_quality_prs.yml +++ b/.github/workflows/close_low_quality_prs.yml @@ -62,9 +62,10 @@ jobs: - name: Run low-quality PR closer env: GH_TOKEN: ${{ secrets.GITHUB_TOKEN }} - # Default to dry-run for scheduled triggers as well. The repo - # variable AGENT_SHIN_ENABLED must be "true" before scheduled runs - # actually close PRs, AND workflow_dispatch must opt-in via close=true. + # Scheduled runs honor AGENT_SHIN_ENABLED directly: when the repo + # variable is "true", the cron actually closes PRs. workflow_dispatch + # must additionally opt-in via close=true so manual previews stay + # dry-run by default. CLOSE_FLAG: ${{ github.event.inputs.close || 'false' }} AGENT_SHIN_ENABLED: ${{ vars.AGENT_SHIN_ENABLED }} MIN_AGE_DAYS: ${{ github.event.inputs.min_age_days || '7' }} @@ -80,7 +81,7 @@ jobs: ) if [ "${AGENT_SHIN_ENABLED:-false}" != "true" ]; then echo "::notice::AGENT_SHIN_ENABLED is not 'true' -> forcing dry-run regardless of close input." - elif [ "${CLOSE_FLAG}" = "true" ]; then + elif [ "${GITHUB_EVENT_NAME:-}" = "schedule" ] || [ "${CLOSE_FLAG}" = "true" ]; then ARGS+=(--close) echo "::notice::Running in close-on-fail mode." else diff --git a/.github/workflows/triage_issue_with_llm.yml b/.github/workflows/triage_issue_with_llm.yml index c9ddad5d3ea..1aab2cfd533 100644 --- a/.github/workflows/triage_issue_with_llm.yml +++ b/.github/workflows/triage_issue_with_llm.yml @@ -69,7 +69,15 @@ jobs: # Automatic `issues` events stay dry-run regardless until the team # explicitly invokes workflow_dispatch with close=true. if [ "${GITHUB_EVENT_NAME:-}" = "issues" ]; then - ARGS=("${ARGS[@]/--close/}") + # filter out --close rather than substituting to "" (which would + # leave an empty positional arg that argparse rejects) + FILTERED=() + for arg in "${ARGS[@]}"; do + if [ "${arg}" != "--close" ]; then + FILTERED+=("${arg}") + fi + done + ARGS=("${FILTERED[@]}") echo "::notice::issues trigger -> forcing dry-run." fi python3 .github/scripts/triage_with_llm.py "${ARGS[@]}" diff --git a/.github/workflows/triage_pr_with_llm.yml b/.github/workflows/triage_pr_with_llm.yml index f9b6d7fed47..58d2aaee66c 100644 --- a/.github/workflows/triage_pr_with_llm.yml +++ b/.github/workflows/triage_pr_with_llm.yml @@ -82,8 +82,16 @@ jobs: # summary before any contributor sees a comment. Only the manual # workflow_dispatch path (with close=true) closes PRs. if [ "${GITHUB_EVENT_NAME:-}" = "pull_request_target" ]; then - # strip any --close added above - ARGS=("${ARGS[@]/--close/}") + # strip any --close added above (filter out, don't substitute + # to empty string — that would leave a stray "" positional arg + # that argparse rejects) + FILTERED=() + for arg in "${ARGS[@]}"; do + if [ "${arg}" != "--close" ]; then + FILTERED+=("${arg}") + fi + done + ARGS=("${FILTERED[@]}") echo "::notice::pull_request_target trigger -> forcing dry-run." fi python3 .github/scripts/triage_with_llm.py "${ARGS[@]}" From 401433374d7488bc4c1ce953a85763b9829eb05e Mon Sep 17 00:00:00 2001 From: Cursor Agent Date: Sun, 17 May 2026 16:51:26 +0000 Subject: [PATCH 06/17] fix(triage): scheduled cron stays dry-run; dedent prompts before interpolation - close_low_quality_prs.yml: only workflow_dispatch with close=true (and AGENT_SHIN_ENABLED=true) actually closes PRs. Scheduled runs are always dry-run, matching the safety invariant documented for triage_pr/issue. - triage_with_llm.py: textwrap.dedent on an f-string with multi-line interpolated bodies fails because the body's 2nd+ lines start at column 0, making the common-indent zero. Dedent the static template first, then .format() the title/body in. Co-authored-by: Yassin Kortam --- .github/scripts/triage_with_llm.py | 16 ++++++++++++---- .github/workflows/close_low_quality_prs.yml | 12 ++++++------ 2 files changed, 18 insertions(+), 10 deletions(-) diff --git a/.github/scripts/triage_with_llm.py b/.github/scripts/triage_with_llm.py index f8ca780ee4c..3062770dbe9 100644 --- a/.github/scripts/triage_with_llm.py +++ b/.github/scripts/triage_with_llm.py @@ -146,8 +146,11 @@ def has_linked_issue(text: str) -> bool: def build_pr_prompt(*, title: str, body: str) -> str: cleaned_body = strip_html_comments(body or "").strip() or "(empty)" - return textwrap.dedent( - f""" + # Dedent the static template *before* interpolating dynamic fields so that + # multi-line bodies (whose 2nd+ lines start at column 0) don't defeat the + # common-indent computation in textwrap.dedent. + template = textwrap.dedent( + """ You are "Agent Shin", the OSS triage bot for the LiteLLM open-source repository (BerriAI/litellm). Decide whether this external pull request meets the project's contribution standards. @@ -195,12 +198,16 @@ def build_pr_prompt(*, title: str, body: str) -> str: --- """ ).strip() + return template.format(title=title, cleaned_body=cleaned_body) def build_issue_prompt(*, title: str, body: str) -> str: cleaned_body = strip_html_comments(body or "").strip() or "(empty)" - return textwrap.dedent( - f""" + # Dedent the static template *before* interpolating dynamic fields so that + # multi-line bodies (whose 2nd+ lines start at column 0) don't defeat the + # common-indent computation in textwrap.dedent. + template = textwrap.dedent( + """ You are "Agent Shin", the OSS triage bot for the LiteLLM open-source repository (BerriAI/litellm). Decide whether this GitHub issue meets the project's reporting standards. @@ -245,6 +252,7 @@ def build_issue_prompt(*, title: str, body: str) -> str: --- """ ).strip() + return template.format(title=title, cleaned_body=cleaned_body) # --------------------------------------------------------------------------- diff --git a/.github/workflows/close_low_quality_prs.yml b/.github/workflows/close_low_quality_prs.yml index 57f74399cc1..d834cea0c82 100644 --- a/.github/workflows/close_low_quality_prs.yml +++ b/.github/workflows/close_low_quality_prs.yml @@ -62,10 +62,10 @@ jobs: - name: Run low-quality PR closer env: GH_TOKEN: ${{ secrets.GITHUB_TOKEN }} - # Scheduled runs honor AGENT_SHIN_ENABLED directly: when the repo - # variable is "true", the cron actually closes PRs. workflow_dispatch - # must additionally opt-in via close=true so manual previews stay - # dry-run by default. + # Scheduled runs are ALWAYS dry-run, even when AGENT_SHIN_ENABLED is + # "true", so the team can QA the closer's verdicts in step summaries + # before any contributor sees a PR closed. Real closures only happen + # on manual workflow_dispatch with close=true (and the variable set). CLOSE_FLAG: ${{ github.event.inputs.close || 'false' }} AGENT_SHIN_ENABLED: ${{ vars.AGENT_SHIN_ENABLED }} MIN_AGE_DAYS: ${{ github.event.inputs.min_age_days || '7' }} @@ -81,10 +81,10 @@ jobs: ) if [ "${AGENT_SHIN_ENABLED:-false}" != "true" ]; then echo "::notice::AGENT_SHIN_ENABLED is not 'true' -> forcing dry-run regardless of close input." - elif [ "${GITHUB_EVENT_NAME:-}" = "schedule" ] || [ "${CLOSE_FLAG}" = "true" ]; then + elif [ "${GITHUB_EVENT_NAME:-}" = "workflow_dispatch" ] && [ "${CLOSE_FLAG}" = "true" ]; then ARGS+=(--close) echo "::notice::Running in close-on-fail mode." else - echo "::notice::AGENT_SHIN_ENABLED is true but close=false -> dry-run." + echo "::notice::AGENT_SHIN_ENABLED is true but this trigger is dry-run (scheduled event or close=false)." fi python3 .github/scripts/close_low_quality_prs.py "${ARGS[@]}" From edddf0c1796d7955f7ce3bfdaa788196a5c08d86 Mon Sep 17 00:00:00 2001 From: Cursor Agent Date: Sun, 17 May 2026 17:04:07 +0000 Subject: [PATCH 07/17] Fix bugs in auto-close PR triage scripts - close_low_quality_prs.py: Treat author_association API lookup failures as internal (fail-safe) so transient errors don't cause internal contributors' PRs to be auto-closed. - triage_with_llm.py: Update summary heading from 'Would post comment:' to 'Posted comment:' since this branch only runs after the comment has already been posted. Co-authored-by: Yassin Kortam --- .github/scripts/close_low_quality_prs.py | 5 ++++- .github/scripts/triage_with_llm.py | 2 +- 2 files changed, 5 insertions(+), 2 deletions(-) diff --git a/.github/scripts/close_low_quality_prs.py b/.github/scripts/close_low_quality_prs.py index 6338ad380ce..95335aec167 100644 --- a/.github/scripts/close_low_quality_prs.py +++ b/.github/scripts/close_low_quality_prs.py @@ -116,7 +116,10 @@ def is_external_pr_author(pr: dict, repo: str | None) -> bool: if login.endswith("[bot]") or login in {"dependabot", "github-actions"}: return False association = fetch_pr_author_association(pr["number"], repo) - if association in INTERNAL_AUTHOR_ASSOCIATIONS: + # Fail-safe: if the API lookup failed (empty string), treat the author as + # internal so we don't auto-close their PR. Auto-close is destructive, so + # an unknown association should never make a PR eligible for closing. + if not association or association in INTERNAL_AUTHOR_ASSOCIATIONS: return False return True diff --git a/.github/scripts/triage_with_llm.py b/.github/scripts/triage_with_llm.py index 3062770dbe9..1c6c87aeb0b 100644 --- a/.github/scripts/triage_with_llm.py +++ b/.github/scripts/triage_with_llm.py @@ -520,7 +520,7 @@ def render_summary(result: dict) -> str: comment = result.get("comment") if comment: lines.append("") - lines.append("### Would post comment:") + lines.append("### Posted comment:") lines.append("") lines.append("> " + comment.replace("\n", "\n> ")) return "\n".join(lines) From 09da8a7bf9e61b5e90f73bbe698b0d442ec98a69 Mon Sep 17 00:00:00 2001 From: Cursor Agent Date: Sun, 17 May 2026 19:01:35 +0000 Subject: [PATCH 08/17] feat(triage): default Agent Shin to gpt-5.4-mini with reasoning_effort=none - Bump DEFAULT_MODEL from gpt-4o-mini to gpt-5.4-mini (more modern; 4M total context window per OpenAI catalog, JSON-schema response format, function calling all supported). - For gpt-5.x family models, pass reasoning_effort="none" via extra_body. gpt-5.x rejects temperature != 1 unless reasoning_effort is explicitly "none"; setting it lets us keep temperature=0 for deterministic JSON rubric judgments. extra_body works across openai SDK versions regardless of whether they natively type the kwarg. - For non-gpt5 overrides (TRIAGE_MODEL=gpt-4o-mini etc.), reasoning_effort is not sent. - 4 new unit tests cover: gpt-5.4-mini -> reasoning_effort=none, capitalized/dated gpt-5 variants -> reasoning_effort=none, gpt-4o-mini -> no extra_body, base_url passthrough. Co-authored-by: Mateo Wang --- .github/scripts/triage_with_llm.py | 30 +++++-- .../test_github_triage_with_llm.py | 88 +++++++++++++++++++ 2 files changed, 110 insertions(+), 8 deletions(-) diff --git a/.github/scripts/triage_with_llm.py b/.github/scripts/triage_with_llm.py index 1c6c87aeb0b..24c33f3e78b 100644 --- a/.github/scripts/triage_with_llm.py +++ b/.github/scripts/triage_with_llm.py @@ -24,7 +24,7 @@ Environment: GH_TOKEN / GITHUB_TOKEN - for `gh` CLI auth (auto-set in Actions) OPENAI_API_KEY - required when --close is passed OPENAI_BASE_URL - optional (route to any OpenAI-compatible API) - TRIAGE_MODEL - optional model override (default: gpt-4o-mini) + TRIAGE_MODEL - optional model override (default: gpt-5.4-mini) """ from __future__ import annotations @@ -38,10 +38,17 @@ import sys import textwrap from typing import Any -DEFAULT_MODEL = "gpt-4o-mini" +DEFAULT_MODEL = "gpt-5.4-mini" INTERNAL_ASSOCIATIONS = frozenset({"OWNER", "MEMBER", "COLLABORATOR"}) +# Model families that require `reasoning_effort` to be set, and that reject +# `temperature != 1` unless `reasoning_effort` is "none". For these models we +# pass `reasoning_effort="none"` so a `temperature=0` deterministic judgment +# is still accepted. See litellm/llms/openai/chat/gpt_5_transformation.py for +# the full set of constraints LiteLLM applies to these models. +GPT5_FAMILY_PREFIX = "gpt-5" + # Regexes for picking off "obvious passes" without burning LLM tokens. LINKED_ISSUE_PATTERN = re.compile( r"\b(?:fixes|fix|closes|close|resolves|resolve|refs|ref|see|addresses)\s+" @@ -272,12 +279,19 @@ def call_llm_judge( if base_url else OpenAI(api_key=api_key) ) - response = client.chat.completions.create( - model=model, - messages=[{"role": "user", "content": prompt}], - temperature=0, - response_format={"type": "json_object"}, - ) + kwargs: dict[str, Any] = { + "model": model, + "messages": [{"role": "user", "content": prompt}], + "temperature": 0, + "response_format": {"type": "json_object"}, + } + # gpt-5.x reasoning models reject `temperature != 1` unless + # `reasoning_effort` is explicitly "none". Set it via `extra_body` so this + # works across openai SDK versions regardless of whether the SDK natively + # types `reasoning_effort` as a top-level chat-completions param yet. + if model.lower().startswith(GPT5_FAMILY_PREFIX): + kwargs["extra_body"] = {"reasoning_effort": "none"} + response = client.chat.completions.create(**kwargs) return response.choices[0].message.content or "" diff --git a/tests/test_litellm/test_github_triage_with_llm.py b/tests/test_litellm/test_github_triage_with_llm.py index de72cc8d7c9..57518251ea8 100644 --- a/tests/test_litellm/test_github_triage_with_llm.py +++ b/tests/test_litellm/test_github_triage_with_llm.py @@ -147,6 +147,94 @@ class TestBuildPrompts: assert "repro here" in prompt +class TestCallLlmJudge: + """call_llm_judge sets gpt-5 specific kwargs correctly.""" + + def _stub_openai(self, monkeypatch, captured: dict): + """Install a fake `openai.OpenAI` client into sys.modules. + + The fake client records the kwargs passed to chat.completions.create + and returns a minimal response object whose .choices[0].message.content + is "ok". + """ + import types + + class FakeMessage: + content = '{"verdict": "pass"}' + + class FakeChoice: + message = FakeMessage() + + class FakeResponse: + choices = [FakeChoice()] + + class FakeCompletions: + def create(self, **kwargs): + captured.update(kwargs) + return FakeResponse() + + class FakeChat: + completions = FakeCompletions() + + class FakeClient: + def __init__(self, api_key, base_url=None): + captured["__client_kwargs__"] = { + "api_key": api_key, + "base_url": base_url, + } + self.chat = FakeChat() + + fake_module = types.ModuleType("openai") + fake_module.OpenAI = FakeClient + monkeypatch.setitem(sys.modules, "openai", fake_module) + + def test_should_set_reasoning_effort_none_for_gpt5_family( + self, triage_module, monkeypatch + ): + captured: dict = {} + self._stub_openai(monkeypatch, captured) + triage_module.call_llm_judge( + "prompt", model="gpt-5.4-mini", api_key="sk-test", base_url=None + ) + assert captured["model"] == "gpt-5.4-mini" + assert captured["temperature"] == 0 + assert captured["extra_body"] == {"reasoning_effort": "none"} + + def test_should_set_reasoning_effort_for_capitalized_or_dated_gpt5( + self, triage_module, monkeypatch + ): + for model in ("GPT-5.4-mini", "gpt-5.4-mini-2026-03-17", "gpt-5"): + captured: dict = {} + self._stub_openai(monkeypatch, captured) + triage_module.call_llm_judge( + "prompt", model=model, api_key="sk-test", base_url=None + ) + assert captured["extra_body"] == {"reasoning_effort": "none"}, model + + def test_should_omit_reasoning_effort_for_non_gpt5( + self, triage_module, monkeypatch + ): + captured: dict = {} + self._stub_openai(monkeypatch, captured) + triage_module.call_llm_judge( + "prompt", model="gpt-4o-mini", api_key="sk-test", base_url=None + ) + assert "extra_body" not in captured + + def test_should_pass_base_url_when_provided(self, triage_module, monkeypatch): + captured: dict = {} + self._stub_openai(monkeypatch, captured) + triage_module.call_llm_judge( + "p", + model="gpt-5.4-mini", + api_key="sk-test", + base_url="https://proxy.example.com/v1", + ) + assert ( + captured["__client_kwargs__"]["base_url"] == "https://proxy.example.com/v1" + ) + + class TestTriageOrchestration: """End-to-end-ish tests that mock both gh fetchers and the LLM.""" From c501f74a2303f390353c8ee71e62090aa3cbe2f3 Mon Sep 17 00:00:00 2001 From: Cursor Agent Date: Sun, 17 May 2026 21:19:39 +0000 Subject: [PATCH 09/17] =?UTF-8?q?fix(triage):=20bugbot=20=E2=80=94=20drop?= =?UTF-8?q?=20dead=20gh=5Fjson=20and=20fix=20--optout-label=20append-with-?= =?UTF-8?q?default?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Removed the unused gh_json helper (bugbot low-severity dead code). - Replaced argparse `action="append", default=[...]` with default=None + DEFAULT_OPTOUT_LABELS fallback. The mutable-default + append combo silently APPENDS to the canonical defaults instead of replacing them, so --optout-label could not actually scope the opt-out list. - Added tests covering both the canonical default and the flag-replaces-defaults behavior. Co-authored-by: Mateo Wang --- .github/scripts/close_low_quality_prs.py | 24 +++--- .../test_github_close_low_quality_prs.py | 78 +++++++++++++++++++ 2 files changed, 92 insertions(+), 10 deletions(-) diff --git a/.github/scripts/close_low_quality_prs.py b/.github/scripts/close_low_quality_prs.py index 95335aec167..43141a38a61 100644 --- a/.github/scripts/close_low_quality_prs.py +++ b/.github/scripts/close_low_quality_prs.py @@ -35,7 +35,7 @@ import json import re import subprocess import sys -from typing import Any, Iterable +from typing import Iterable # Greptile's GitHub App appears as `greptile-apps[bot]` in REST API comments # and `greptile-apps` in `gh pr view --json` output. Accept either form. @@ -54,6 +54,12 @@ SCORE_PATTERN = re.compile( # exempt from auto-triage. INTERNAL_AUTHOR_ASSOCIATIONS = frozenset({"OWNER", "MEMBER", "COLLABORATOR"}) +# Default labels that exempt a PR from auto-close. Defined at module scope (not +# as a mutable argparse default) so that `--optout-label foo` REPLACES the +# defaults instead of appending to them — the argparse `action="append"` + +# `default=[...]` combination silently mutates the shared default list. +DEFAULT_OPTOUT_LABELS = ("do not close", "keep open", "wip") + def gh(*args: str) -> str: """Run a `gh` CLI command and return stdout. Raises on non-zero exit.""" @@ -66,11 +72,6 @@ def gh(*args: str) -> str: return result.stdout -def gh_json(*args: str) -> Any: - """Run a `gh` CLI command that emits JSON and return the parsed value.""" - return json.loads(gh(*args)) - - def fetch_open_prs(repo: str | None) -> list[dict]: """Fetch all open PRs (number, createdAt, isDraft, labels, author).""" repo_args = ["--repo", repo] if repo else [] @@ -297,10 +298,13 @@ def main() -> int: parser.add_argument( "--optout-label", action="append", - default=["do not close", "keep open", "wip"], + default=None, help=( - "Label(s) that exempt a PR from auto-close. " - "Repeat to add more. Case-insensitive." + "Label(s) that exempt a PR from auto-close. Repeat to add more. " + "Case-insensitive. When omitted, defaults to " + f"{list(DEFAULT_OPTOUT_LABELS)!r}; passing this flag REPLACES the " + "defaults (argparse `append` with a mutable default would append " + "instead, which we explicitly avoid)." ), ) parser.add_argument( @@ -334,7 +338,7 @@ def main() -> int: print(f"Found {len(prs)} open PRs.\n") now = dt.datetime.now(dt.timezone.utc) - optout_labels = set(args.optout_label) + optout_labels = set(args.optout_label or DEFAULT_OPTOUT_LABELS) closed = 0 summary = { diff --git a/tests/test_litellm/test_github_close_low_quality_prs.py b/tests/test_litellm/test_github_close_low_quality_prs.py index 0d63562ec0f..6d1ad50012e 100644 --- a/tests/test_litellm/test_github_close_low_quality_prs.py +++ b/tests/test_litellm/test_github_close_low_quality_prs.py @@ -292,6 +292,84 @@ class TestEvaluatePr: assert score is None +class TestMainOptoutLabelDefault: + """`--optout-label` must REPLACE the canonical defaults, not append.""" + + def _patch_no_op(self, closer_module, monkeypatch): + monkeypatch.setattr(closer_module, "fetch_open_prs", lambda repo: []) + # `optout_labels` is captured indirectly via evaluate_pr; sniff the + # set passed in by stubbing evaluate_pr. + captured: dict = {} + + def fake_evaluate(pr, now, min_age_days, min_score, repo, optout_labels): + captured["optout_labels"] = set(optout_labels) + return ("skip-draft", None, None) + + monkeypatch.setattr(closer_module, "evaluate_pr", fake_evaluate) + return captured + + def test_should_use_canonical_defaults_when_flag_omitted( + self, closer_module, monkeypatch + ): + captured = self._patch_no_op(closer_module, monkeypatch) + # No PRs -> capture won't fire; instead inject one synthetic PR via + # fetch_open_prs so evaluate_pr is invoked at least once. + monkeypatch.setattr( + closer_module, + "fetch_open_prs", + lambda repo: [ + { + "number": 1, + "title": "p", + "createdAt": "2026-05-10T00:00:00Z", + "isDraft": True, + "labels": [], + "author": {"login": "x"}, + } + ], + ) + monkeypatch.setattr(sys, "argv", ["close_low_quality_prs.py"]) + rc = closer_module.main() + assert rc == 0 + assert captured["optout_labels"] == set(closer_module.DEFAULT_OPTOUT_LABELS) + + def test_should_replace_defaults_when_flag_provided( + self, closer_module, monkeypatch + ): + captured = self._patch_no_op(closer_module, monkeypatch) + monkeypatch.setattr( + closer_module, + "fetch_open_prs", + lambda repo: [ + { + "number": 1, + "title": "p", + "createdAt": "2026-05-10T00:00:00Z", + "isDraft": True, + "labels": [], + "author": {"login": "x"}, + } + ], + ) + monkeypatch.setattr( + sys, + "argv", + [ + "close_low_quality_prs.py", + "--optout-label", + "hold", + "--optout-label", + "needs-discussion", + ], + ) + rc = closer_module.main() + assert rc == 0 + # Crucially, none of the canonical defaults leak in. + assert captured["optout_labels"] == {"hold", "needs-discussion"} + for default in closer_module.DEFAULT_OPTOUT_LABELS: + assert default not in captured["optout_labels"], default + + class TestHasOptoutLabel: def test_should_match_label_case_insensitively(self, closer_module): pr = {"labels": [{"name": "Do Not Close"}, {"name": "bug"}]} From 1ac5beea7211388dba5fcf492e95580fd3934cb3 Mon Sep 17 00:00:00 2001 From: Cursor Agent Date: Sun, 17 May 2026 21:19:51 +0000 Subject: [PATCH 10/17] =?UTF-8?q?fix(triage):=20bugbot=20=E2=80=94=20tight?= =?UTF-8?q?en=20linked-issue=20regex,=20fail-safe=20author=5Fassociation,?= =?UTF-8?q?=20fix=20empty=20TRIAGE=5FMODEL?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three independent bugbot findings against triage_with_llm.py: 1. LINKED_ISSUE_PATTERN included weak keywords (`see`, `ref`, `addresses`) so casual mentions like "See #1234 for context" were short-circuited to pass-linked-issue without ever calling the LLM — contradicting the prompt's own "a bare issue number without a closing keyword counts only if it's clearly the related issue (not a passing mention)" rubric. Limit the regex to GitHub's documented PR-closing keywords (fixes/fix/fixed/closes/close/closed/resolves/resolve/resolved). 2. is_internal_contributor() treated an empty/missing author_association as external (eligible for the destructive close path), while the sibling is_external_pr_author() in close_low_quality_prs.py fail-safes the same case as internal. Align the two so a partial/unknown GitHub response can never make a PR eligible for auto-close. 3. argparse `default=os.environ.get("TRIAGE_MODEL", DEFAULT_MODEL)` returns the empty string when GitHub Actions exposes an unset repo variable as an empty-string env var (the optional vars.TRIAGE_MODEL case in the workflow). Use `os.environ.get(...) or DEFAULT_MODEL` so empty -> default, matching the existing OPENAI_BASE_URL pattern. Tests: - Casual mentions now must fall through to the LLM (parametrized); added an orchestration test ensuring "See #1234" reaches the judge. - Empty/missing author_association now fails safe (parametrized). - Empty TRIAGE_MODEL env var falls back to DEFAULT_MODEL; explicit TRIAGE_MODEL is still honored. Co-authored-by: Mateo Wang --- .github/scripts/triage_with_llm.py | 30 ++++- .../test_github_triage_with_llm.py | 107 ++++++++++++++++-- 2 files changed, 124 insertions(+), 13 deletions(-) diff --git a/.github/scripts/triage_with_llm.py b/.github/scripts/triage_with_llm.py index 24c33f3e78b..1b0e862be2f 100644 --- a/.github/scripts/triage_with_llm.py +++ b/.github/scripts/triage_with_llm.py @@ -50,8 +50,15 @@ INTERNAL_ASSOCIATIONS = frozenset({"OWNER", "MEMBER", "COLLABORATOR"}) GPT5_FAMILY_PREFIX = "gpt-5" # Regexes for picking off "obvious passes" without burning LLM tokens. +# +# Keep this list to GitHub's documented PR-closing keywords only +# (https://docs.github.com/issues/tracking-your-work-with-issues/linking-a-pull-request-to-an-issue). +# Casual mentions like "see #1234" or "ref #1234" are intentionally NOT +# auto-passed — they should fall through to the LLM judge, which has the +# stricter rubric "a bare issue number without a closing keyword counts only +# if it's clearly the related issue (not a passing mention)". LINKED_ISSUE_PATTERN = re.compile( - r"\b(?:fixes|fix|closes|close|resolves|resolve|refs|ref|see|addresses)\s+" + r"\b(?:fixes|fix|fixed|closes|close|closed|resolves|resolve|resolved)\s+" r"(?:#\d+|https?://github\.com/[\w.-]+/[\w.-]+/issues/\d+)", re.IGNORECASE, ) @@ -127,13 +134,20 @@ def close_issue(repo: str, number: int, *, not_planned: bool = True) -> None: def is_internal_contributor(item: dict) -> bool: - """Return True if the PR/issue author should be exempted from triage.""" - association = (item.get("author_association") or "").upper() - if association in INTERNAL_ASSOCIATIONS: - return True + """Return True if the PR/issue author should be exempted from triage. + + Fail-safe: if `author_association` is missing or empty (which should never + happen on a successful GitHub REST response but is possible on schema + changes or partial responses), treat the author as INTERNAL so the + destructive close path never fires on an unknown contributor. This matches + the sibling `is_external_pr_author` in `close_low_quality_prs.py`. + """ login = ((item.get("user") or {}).get("login") or "").lower() if login.endswith("[bot]") or login in {"dependabot", "github-actions"}: return True + association = (item.get("author_association") or "").upper() + if not association or association in INTERNAL_ASSOCIATIONS: + return True return False @@ -553,7 +567,11 @@ def main() -> int: ) parser.add_argument( "--model", - default=os.environ.get("TRIAGE_MODEL", DEFAULT_MODEL), + # `os.environ.get("TRIAGE_MODEL", DEFAULT_MODEL)` would return "" when + # GitHub Actions exposes an unset repo variable as an empty-string env + # var, silently bypassing DEFAULT_MODEL and causing every call to fail + # as `skip-llm-error`. The `or` guard collapses empty -> default. + default=os.environ.get("TRIAGE_MODEL") or DEFAULT_MODEL, help=f"OpenAI-compatible model name (default: {DEFAULT_MODEL}).", ) parser.add_argument( diff --git a/tests/test_litellm/test_github_triage_with_llm.py b/tests/test_litellm/test_github_triage_with_llm.py index 57518251ea8..668185526a6 100644 --- a/tests/test_litellm/test_github_triage_with_llm.py +++ b/tests/test_litellm/test_github_triage_with_llm.py @@ -35,7 +35,7 @@ class TestIsInternalContributor: @pytest.mark.parametrize( "association", - ["CONTRIBUTOR", "FIRST_TIME_CONTRIBUTOR", "FIRST_TIMER", "NONE", ""], + ["CONTRIBUTOR", "FIRST_TIME_CONTRIBUTOR", "FIRST_TIMER", "NONE"], ) def test_should_mark_outside_associations_as_external( self, triage_module, association @@ -46,6 +46,20 @@ class TestIsInternalContributor: } assert triage_module.is_internal_contributor(item) is False + @pytest.mark.parametrize( + "item", + [ + {"author_association": "", "user": {"login": "random-oss-dev"}}, + {"user": {"login": "random-oss-dev"}}, # association field absent + ], + ) + def test_should_fail_safe_when_author_association_is_missing( + self, triage_module, item + ): + # Fail-safe: an empty/missing association must never make a PR + # eligible for the destructive close path. Treat as internal (skip). + assert triage_module.is_internal_contributor(item) is True + @pytest.mark.parametrize( "login", ["dependabot[bot]", "greptile-apps[bot]", "dependabot", "github-actions"], @@ -65,7 +79,8 @@ class TestHasLinkedIssue: "closes #1", "Resolves #99", "fix #42 — this addresses the regression", - "Refs https://github.com/BerriAI/litellm/issues/27000", + "Closes https://github.com/BerriAI/litellm/issues/27000", + "Resolved https://github.com/BerriAI/litellm/issues/27001", ], ) def test_should_detect_common_link_phrases(self, triage_module, body): @@ -76,13 +91,17 @@ class TestHasLinkedIssue: [ "", "Some change", - "See #1234", # "see" is allowed per regex but we want documented coverage + # Casual mentions must NOT auto-pass — they should fall through to + # the LLM judge so the stricter "not a passing mention" rule fires. + "See #1234", + "see #1234 for context", + "ref #1234", + "Refs https://github.com/BerriAI/litellm/issues/27000", + "this addresses #1234", ], ) - def test_should_handle_empty_and_unrelated_bodies(self, triage_module, body): - # "See #1234" is intentionally accepted as a related-issue reference. - # Just make sure empty/unrelated bodies don't crash. - triage_module.has_linked_issue(body) + def test_should_not_auto_pass_casual_mentions(self, triage_module, body): + assert triage_module.has_linked_issue(body) is False def test_should_not_detect_when_only_html_comment_template(self, triage_module): body = "" @@ -147,6 +166,54 @@ class TestBuildPrompts: assert "repro here" in prompt +class TestMainModelDefault: + """`--model` falls back to DEFAULT_MODEL even when TRIAGE_MODEL is empty.""" + + def _stub_triage(self, triage_module, monkeypatch): + captured: dict = {} + + def fake_triage(**kwargs): + captured.update(kwargs) + return { + "kind": kwargs["kind"], + "number": kwargs["number"], + "title": "", + "author": "x", + "author_association": "NONE", + "state": "open", + "action": "skip-no-llm-key", + } + + monkeypatch.setattr(triage_module, "triage", fake_triage) + return captured + + def test_should_fall_back_to_default_when_triage_model_env_empty( + self, triage_module, monkeypatch + ): + captured = self._stub_triage(triage_module, monkeypatch) + monkeypatch.setenv("TRIAGE_MODEL", "") + monkeypatch.setattr( + sys, + "argv", + ["triage_with_llm.py", "--repo", "o/r", "--pr", "1"], + ) + rc = triage_module.main() + assert rc == 0 + assert captured["model"] == triage_module.DEFAULT_MODEL + + def test_should_respect_explicit_triage_model_env(self, triage_module, monkeypatch): + captured = self._stub_triage(triage_module, monkeypatch) + monkeypatch.setenv("TRIAGE_MODEL", "gpt-4o-mini") + monkeypatch.setattr( + sys, + "argv", + ["triage_with_llm.py", "--repo", "o/r", "--pr", "1"], + ) + rc = triage_module.main() + assert rc == 0 + assert captured["model"] == "gpt-4o-mini" + + class TestCallLlmJudge: """call_llm_judge sets gpt-5 specific kwargs correctly.""" @@ -296,6 +363,32 @@ class TestTriageOrchestration: assert result["action"] == "pass-linked-issue" assert result["verdict"]["verdict"] == "pass" + def test_should_not_short_circuit_on_casual_mention( + self, triage_module, monkeypatch + ): + # "See #1234" is a passing mention, not a closing keyword. The LLM + # must get a chance to apply the stricter rubric. + pr = self._make_pr(body="See #1234 for context. No QA proof here.") + monkeypatch.setattr(triage_module, "fetch_pr", lambda repo, n: pr) + called = {"judge": False} + + def judge(prompt): + called["judge"] = True + return json.dumps( + {"verdict": "fail", "missing": ["QA proof"], "explanation": "thin."} + ) + + result = triage_module.triage( + repo="o/r", + kind="pr", + number=1, + close=False, + model="m", + judge=judge, + ) + assert called["judge"] is True + assert result["action"] == "would-close" + def test_should_return_pass_llm_when_judge_passes(self, triage_module, monkeypatch): pr = self._make_pr(body="Long body, no linked issue.") monkeypatch.setattr(triage_module, "fetch_pr", lambda repo, n: pr) From 638dba9d1474d41657d2b84002c8deef0ec5baad Mon Sep 17 00:00:00 2001 From: Cursor Agent Date: Sun, 17 May 2026 21:55:52 +0000 Subject: [PATCH 11/17] =?UTF-8?q?fix(workflows):=20bugbot=20=E2=80=94=20ga?= =?UTF-8?q?te=20Agent=20Shin=20--close=20on=20'=3D=20true'=20not=20'!=3D?= =?UTF-8?q?=20false'?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The PR and issue Agent Shin workflows gated the destructive --close flag with [ "${DISPATCH_CLOSE:-false}" != "false" ]. That pattern treats anything other than the literal string "false" as enabling closure — "True", "yes", "1", typos, accidental whitespace, etc. The workflow_dispatch input UI is a 'true'/'false' choice dropdown so the form is constrained, but the API (`gh workflow run -f close=...`) accepts any string, and a CI cron / external invoker passing a non-canonical truthy value would have silently enabled real contributor PR closures. Mirror the sibling Greptile closer's [ "${CLOSE_FLAG}" = "true" ] pattern: only the EXACT string "true" enables --close; every other value (including the unset/empty default) resolves to dry-run. This is the fail-safe philosophy applied everywhere else in this PR. Added tests/test_litellm/test_github_triage_workflows.py with two parametrized invariants: 1. The destructive gate uses '= "true"' for its env-var comparison (either bare '${ENV}' or '${ENV:-false}' form accepted), and never the fail-open '!= "false"' pattern. 2. Every destructive gate is also gated on AGENT_SHIN_ENABLED being "true" — either by entering the close branch on '=' or by bailing out early on '!=' — so flipping the repo variable off is a true kill switch regardless of per-run inputs. Manually verified the test fails on the buggy '!= "false"' pattern and passes on the fix, so it would have caught the regression at PR time. Co-authored-by: Mateo Wang --- .github/workflows/triage_issue_with_llm.yml | 11 +- .github/workflows/triage_pr_with_llm.yml | 11 +- .../test_github_triage_workflows.py | 135 ++++++++++++++++++ 3 files changed, 153 insertions(+), 4 deletions(-) create mode 100644 tests/test_litellm/test_github_triage_workflows.py diff --git a/.github/workflows/triage_issue_with_llm.yml b/.github/workflows/triage_issue_with_llm.yml index 1aab2cfd533..ff0497f9893 100644 --- a/.github/workflows/triage_issue_with_llm.yml +++ b/.github/workflows/triage_issue_with_llm.yml @@ -58,11 +58,18 @@ jobs: run: | set -euo pipefail ARGS=(--repo "${{ github.repository }}" --issue "${ISSUE_NUMBER}") - if [ "${AGENT_SHIN_ENABLED:-false}" = "true" ] && [ "${DISPATCH_CLOSE:-false}" != "false" ]; then + # Fail-safe gating: only the EXACT string "true" enables the + # destructive --close path. The workflow_dispatch input is a + # `choice` dropdown of "true"/"false" so the UI is constrained, + # but the API (`gh workflow run -f close=...`) accepts any + # string, and a `!= "false"` check would treat "True", "yes", + # "1", "TRUE", typos, and accidental whitespace as enabling + # closure. Mirror the Greptile closer's `= "true"` pattern. + if [ "${AGENT_SHIN_ENABLED:-false}" = "true" ] && [ "${DISPATCH_CLOSE:-false}" = "true" ]; then ARGS+=(--close) echo "::notice::Agent Shin is ENABLED and running in close-on-fail mode." elif [ "${AGENT_SHIN_ENABLED:-false}" = "true" ]; then - echo "::notice::Agent Shin is ENABLED but this trigger is dry-run." + echo "::notice::Agent Shin is ENABLED but this trigger is dry-run (workflow_dispatch close != 'true')." else echo "::notice::Agent Shin is in DRY-RUN mode (AGENT_SHIN_ENABLED is not 'true'). No comments will be posted; no issues will be closed." fi diff --git a/.github/workflows/triage_pr_with_llm.yml b/.github/workflows/triage_pr_with_llm.yml index 58d2aaee66c..eac7e6a56b3 100644 --- a/.github/workflows/triage_pr_with_llm.yml +++ b/.github/workflows/triage_pr_with_llm.yml @@ -69,11 +69,18 @@ jobs: run: | set -euo pipefail ARGS=(--repo "${{ github.repository }}" --pr "${PR_NUMBER}") - if [ "${AGENT_SHIN_ENABLED:-false}" = "true" ] && [ "${DISPATCH_CLOSE:-false}" != "false" ]; then + # Fail-safe gating: only the EXACT string "true" enables the + # destructive --close path. The workflow_dispatch input is a + # `choice` dropdown of "true"/"false" so the UI is constrained, + # but the API (`gh workflow run -f close=...`) accepts any + # string, and a `!= "false"` check would treat "True", "yes", + # "1", "TRUE", typos, and accidental whitespace as enabling + # closure. Mirror the Greptile closer's `= "true"` pattern. + if [ "${AGENT_SHIN_ENABLED:-false}" = "true" ] && [ "${DISPATCH_CLOSE:-false}" = "true" ]; then ARGS+=(--close) echo "::notice::Agent Shin is ENABLED and running in close-on-fail mode." elif [ "${AGENT_SHIN_ENABLED:-false}" = "true" ]; then - echo "::notice::Agent Shin is ENABLED but this trigger is dry-run (workflow_dispatch close=false or scheduled event)." + echo "::notice::Agent Shin is ENABLED but this trigger is dry-run (workflow_dispatch close != 'true' or scheduled event)." else echo "::notice::Agent Shin is in DRY-RUN mode (AGENT_SHIN_ENABLED is not 'true'). No comments will be posted; no PRs will be closed." fi diff --git a/tests/test_litellm/test_github_triage_workflows.py b/tests/test_litellm/test_github_triage_workflows.py new file mode 100644 index 00000000000..438b92b6add --- /dev/null +++ b/tests/test_litellm/test_github_triage_workflows.py @@ -0,0 +1,135 @@ +"""Static guardrails for the Agent Shin + Greptile workflow YAML files. + +These workflows can post comments and close PRs/issues on +BerriAI/litellm, so the gating logic that decides "is this a real +close-on-fail run?" must fail-safe on any unexpected input. The risk +is mostly maintenance: someone edits the bash gate, drops a quote, +inverts a comparison, or uses `!= "false"` (which treats "True", +"yes", "1", and typos as enabling closure) and the regression isn't +caught until a real OSS contributor's PR gets auto-closed. + +The tests below pin two invariants across every workflow that gates a +destructive `--close`: + + 1. The gate uses the fail-safe `= "true"` comparison — not `!= "false"`, + not `!= ""`. Only the literal string "true" should ever enable + closure. + 2. The gate also requires `AGENT_SHIN_ENABLED = "true"` (or the + scheduled-job equivalent) — disabling the variable must always + force dry-run. + +Static parsing of the YAML + bash text is the right level of test here: +the gating logic lives in a `run:` block, not in a Python module we can +import, and end-to-end testing a GitHub Actions workflow from CI is +infeasible. A YAML-level guardrail is exactly what would have caught +the original `!= "false"` regression at PR time. +""" + +from __future__ import annotations + +from pathlib import Path + +import pytest +import yaml + +REPO_ROOT = Path(__file__).resolve().parents[2] +WORKFLOWS_DIR = REPO_ROOT / ".github" / "workflows" + +# Map of workflow file -> the env var name that drives the destructive +# gate inside that workflow's `run:` block. Keeping this table explicit +# (rather than scraping every workflow file) means a new workflow file +# that bypasses the dry-run gating doesn't silently slip past this test. +DESTRUCTIVE_GATE_ENV: dict[str, str] = { + "triage_pr_with_llm.yml": "DISPATCH_CLOSE", + "triage_issue_with_llm.yml": "DISPATCH_CLOSE", + "close_low_quality_prs.yml": "CLOSE_FLAG", +} + + +def _load_workflow(name: str) -> dict: + return yaml.safe_load((WORKFLOWS_DIR / name).read_text()) + + +def _all_run_blocks(workflow: dict) -> list[str]: + """Return every `run:` step's command text, joined.""" + commands: list[str] = [] + jobs = workflow.get("jobs") or {} + for job in jobs.values(): + for step in job.get("steps", []) or []: + if not isinstance(step, dict): + continue + run = step.get("run") + if isinstance(run, str): + commands.append(run) + return commands + + +@pytest.mark.parametrize("workflow_file,env_var", sorted(DESTRUCTIVE_GATE_ENV.items())) +def test_should_use_failsafe_equals_true_comparison( + workflow_file: str, env_var: str +) -> None: + """The destructive `--close` gate must use `= "true"` (fail-safe), not + `!= "false"` (which would treat "True", "yes", "1", or any typo as + enabling closure). + + Both bare `${ENV_VAR}` and `${ENV_VAR:-false}` (with a default) are + accepted forms — what matters is the comparison operator. The + Greptile closer relies on an outer `AGENT_SHIN_ENABLED` gate so it + can use the bare form; the Agent Shin workflows include `:-false` + for defense in depth. Either is fine. + """ + workflow = _load_workflow(workflow_file) + text = "\n".join(_all_run_blocks(workflow)) + assert env_var in text, ( + f"{workflow_file} no longer references {env_var}; was the " + "gating env var renamed without updating this test?" + ) + accepted_patterns = ( + f'"${{{env_var}}}" = "true"', + f'"${{{env_var}:-false}}" = "true"', + ) + assert any(p in text for p in accepted_patterns), ( + f"{workflow_file} must gate the destructive --close flag on the " + f'EXACT string "true" (one of: {accepted_patterns!r}). Mirror ' + 'the Greptile closer pattern; do NOT use `!= "false"` which ' + 'fail-opens on unknown values like "True", "yes", "1", or typos.' + ) + forbidden_patterns = ( + f'"${{{env_var}}}" != "false"', + f'"${{{env_var}:-false}}" != "false"', + f'"${{{env_var}:-true}}" != "false"', + ) + for forbidden in forbidden_patterns: + assert forbidden not in text, ( + f"{workflow_file} uses the fail-open pattern {forbidden!r}. " + 'Switch to `= "true"` so unknown values stay dry-run.' + ) + + +@pytest.mark.parametrize("workflow_file", sorted(DESTRUCTIVE_GATE_ENV)) +def test_should_require_agent_shin_enabled_for_close(workflow_file: str) -> None: + """Every destructive gate must also gate on the global enablement + variable, so flipping `AGENT_SHIN_ENABLED` off is a kill switch + regardless of any per-run input. + + Two patterns are equally fine: + - Positive: `[ "${AGENT_SHIN_ENABLED:-false}" = "true" ]` to enter + the close branch (Agent Shin workflows). + - Negative: `[ "${AGENT_SHIN_ENABLED:-false}" != "true" ]` then + bail out / force dry-run (Greptile closer). + + What matters is that the comparison value is the literal "true"; + `!= "false"` or `= "1"` etc. would not be a true kill switch. + """ + workflow = _load_workflow(workflow_file) + text = "\n".join(_all_run_blocks(workflow)) + accepted_patterns = ( + '"${AGENT_SHIN_ENABLED:-false}" = "true"', + '"${AGENT_SHIN_ENABLED:-false}" != "true"', + ) + assert any(p in text for p in accepted_patterns), ( + f"{workflow_file} must gate destructive actions on " + '`AGENT_SHIN_ENABLED = "true"` (or the inverted `!= "true"` ' + "guard that forces dry-run). Without this, an unset repo " + "variable would not be treated as a kill switch." + ) From ff57d5b5466ea8d52621a57fdc4631a469168c6b Mon Sep 17 00:00:00 2001 From: Cursor Agent Date: Mon, 18 May 2026 00:15:42 +0000 Subject: [PATCH 12/17] feat(triage): close any PR (incl. drafts, any age); add @agent-shin reconsider flow MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Follow-up to PR #28117. Three behavior changes + one new workflow, addressing the team's concerns on the original review: 1) Apply auto-close to ALL open PRs, not just those over a week old. - close_low_quality_prs.py: --min-age-days default flipped from 7 to 0. The flag is preserved as an opt-in safety net for one-off backfill runs that want to spare very-young PRs, but the daily scheduled sweep now closes external-author PRs as soon as Greptile scores them <4/5. - close_low_quality_prs.yml: workflow_dispatch input default also flipped to 0; doc comments updated. 2) Apply auto-close to draft PRs too. - close_low_quality_prs.py: removed the skip-draft branch in evaluate_pr. Drafts are NOT a free pass — the team's intent is 'open PR count == PRs internal collaborators need to action on', so a draft Greptile scored 2/5 still belongs in the closed bucket. Authors who genuinely need a long-lived draft can attach the 'wip' opt-out label, which is unchanged. - The 'skip-draft' action is gone; the 'wip' label still skips. 3) Address the 'OSS contributors cannot reopen a bot-closed PR' wrinkle. GitHub does NOT let an external (non-write-access) contributor reopen a PR that was closed by a bot or maintainer (long-standing limitation). The original PR's close-comments told contributors to 'Reopen the PR — I'll re-evaluate automatically', which is broken for the very audience this triage targets. Two changes: a) Reword every close-comment (Greptile sweep + Agent Shin PR close + Agent Shin issue close + PR template) to recommend: - Open a new PR with the updated branch (primary path). - Or comment '@agent-shin reconsider' on the closed PR for a re-evaluation that, on pass, reopens the PR via the bot's GH_TOKEN write access. b) Add the @agent-shin reconsider workflow: - .github/workflows/triage_reconsider.yml: new 'issue_comment'-triggered workflow. Authorizes only the PR/issue author or an internal collaborator (OWNER/MEMBER/COLLABORATOR), gated via a step output so unauthorized commenters never reach the destructive steps. Globally gated on AGENT_SHIN_ENABLED='true' (positive form, matching the test_github_triage_workflows guardrail patterns). - triage_with_llm.py: --reconsider mode. On a closed PR/issue, re-runs the LLM judge (or linked-issue regex short-circuit) and: - on pass: reopens via reopen_pr/reopen_issue + posts a 'Re-evaluated and reopened' comment. - on fail: leaves closed and posts a 'still missing X' comment so the contributor can iterate again. Reconsider-on-open is a no-op ('skip-not-closed'). Internal-author + bot-account skips still take priority over reconsider. 4) Greptile-on-closed-PRs question: the team asked whether Greptile can re-review a closed PR. Greptile's docs don't address this and we shouldn't promise behavior we can't verify, so the new close-comment wording does NOT instruct contributors to 're-request greptile on the closed PR'. Instead it points them at the new-PR path (which Greptile definitely reviews) or the @agent-shin reconsider trigger (which re-runs the LiteLLM-side rubric judge, not Greptile). Tests: 93 passing (was 59). - test_github_close_low_quality_prs.py: replaced 'skip drafts' test with 'closes drafts when score is low' + 'closes brand-new PR when min_age=0' + 'no skip when min_age=0'. The 'skip too young' assertion is preserved as opt-in. - test_github_triage_with_llm.py: 6 new TestTriageOrchestration cases for reconsider mode (skip-not-closed on open, reopen on pass, still-failing comment on fail, linked-issue short-circuit reopen, skip internal author in reconsider, reopen-issue on pass) + a new TestCloseCommentText class that pins the user-facing 'open a new PR' + '@agent-shin reconsider' wording. - test_github_triage_workflows.py: added triage_reconsider.yml to the destructive-gate guardrail table; AGENT_SHIN_ENABLED is its own destructive gate (no separate per-run flag needed). Co-authored-by: Mateo Wang --- .github/pull_request_template.md | 25 +- .github/scripts/close_low_quality_prs.py | 78 ++++-- .github/scripts/triage_with_llm.py | 164 ++++++++++++- .github/workflows/close_low_quality_prs.yml | 22 +- .github/workflows/triage_reconsider.yml | 126 ++++++++++ .../test_github_close_low_quality_prs.py | 68 ++++- .../test_github_triage_with_llm.py | 232 ++++++++++++++++++ .../test_github_triage_workflows.py | 4 + 8 files changed, 658 insertions(+), 61 deletions(-) create mode 100644 .github/workflows/triage_reconsider.yml diff --git a/.github/pull_request_template.md b/.github/pull_request_template.md index b6feb076779..a9c41d4b231 100644 --- a/.github/pull_request_template.md +++ b/.github/pull_request_template.md @@ -1,10 +1,10 @@ `). Inside the window the triage returns `skip-rate-limited` and the LLM never runs. Workflow update: - `triage_reconsider.yml` now passes `--close` only when `AGENT_SHIN_ENABLED=true`, matching the pattern of `triage_pr_with_llm.yml`. The script runs in both states so the verdict still appears in the step summary for QA. Tests: - Add 5 reconsider safety tests: dry-run for pass / fail / linked-issue short-circuit, bot-closed-guard refusal on maintainer close, rate-limit refusal inside the cooldown window, and cooldown-elapsed acceptance. - Add unit tests for `was_closed_by_agent_shin` (bot / maintainer / missing actor / env-override) and `seconds_since_last_reconsider_verdict` (no marker / multiple markers / non-bot comment with marker / bot comment without marker). - Pin the `` marker in both reopen and still-failing comments — dropping it would silently break the cooldown. Existing reconsider tests updated to pass `close=True` (the production path now) + stub the new guards via `_stub_reconsider_guards`. 112 tests pass (was 93). Co-authored-by: Mateo Wang --- .github/scripts/triage_with_llm.py | 189 +++++++- .github/workflows/triage_reconsider.yml | 24 +- .../test_github_triage_with_llm.py | 436 +++++++++++++++++- 3 files changed, 628 insertions(+), 21 deletions(-) diff --git a/.github/scripts/triage_with_llm.py b/.github/scripts/triage_with_llm.py index 9cf7f04d32f..94052c32e7b 100644 --- a/.github/scripts/triage_with_llm.py +++ b/.github/scripts/triage_with_llm.py @@ -30,6 +30,7 @@ Environment: from __future__ import annotations import argparse +import datetime as dt import json import os import re @@ -42,6 +43,25 @@ DEFAULT_MODEL = "gpt-5.4-mini" INTERNAL_ASSOCIATIONS = frozenset({"OWNER", "MEMBER", "COLLABORATOR"}) +# Login of the account that performs Agent Shin's GitHub writes. When the +# workflow uses `secrets.GITHUB_TOKEN` (our default), the closure / reopen +# event's `actor.login` is `github-actions[bot]`. The env override exists +# for local debugging and for repos that wire Agent Shin to a PAT. +AGENT_SHIN_DEFAULT_BOT_LOGIN = "github-actions[bot]" + +# HTML marker appended to every reconsider verdict comment. We grep for this +# on subsequent reconsider triggers to enforce a short cooldown so that +# repeated `@agent-shin reconsider` comments don't burn CI/LLM budget. +# Using a unique HTML comment keeps the marker invisible to humans while +# being trivially greppable from a comments-list API response. +RECONSIDER_COMMENT_MARKER = "" + +# Minimum gap between two reconsider verdicts on the same PR/issue. Set to +# 10 minutes — long enough that a contributor can't trivially spam the +# trigger, short enough that a genuine "I just pushed a fix and reupdated +# the body" iteration loop isn't punished. +RECONSIDER_RATE_LIMIT_SECONDS = 600 + # Model families that require `reasoning_effort` to be set, and that reject # `temperature != 1` unless `reasoning_effort` is "none". For these models we # pass `reasoning_effort="none"` so a `temperature=0` deterministic judgment @@ -160,6 +180,103 @@ def reopen_issue(repo: str, number: int) -> None: ) +def _iter_paginated_json(*api_args: str) -> Any: + """Yield JSON objects from `gh api --paginate ... -q '.[]'`. + + `gh api --paginate` on a JSON-array endpoint concatenates pages into + one stream; `-q '.[]'` flattens that stream into newline-delimited + objects (jq-style). This keeps memory bounded for chatty endpoints + like issue events/comments on long-lived PRs. + """ + raw = gh("api", "--paginate", *api_args, "-q", ".[]") + for line in raw.splitlines(): + line = line.strip() + if not line: + continue + try: + yield json.loads(line) + except json.JSONDecodeError: + # A malformed line should not blow up the whole guard. Skip and + # carry on — at worst the guard fail-closes (returns False / + # None) and the caller treats it as "unknown". + continue + + +def fetch_last_close_actor(repo: str, number: int) -> str | None: + """Return the login of the actor who most recently closed this PR/issue. + + Returns None if no `closed` event is found (unusual for a closed item, + but possible if the events API returns nothing — in which case the + bot-closed guard should fail-closed, i.e. refuse to reopen). + """ + last: str | None = None + for event in _iter_paginated_json(f"repos/{repo}/issues/{number}/events"): + if event.get("event") == "closed": + last = (event.get("actor") or {}).get("login") + return last + + +def was_closed_by_agent_shin( + repo: str, number: int, *, bot_login: str | None = None +) -> bool: + """Return True iff the PR/issue was most-recently closed by Agent Shin. + + This is the guard that stops `@agent-shin reconsider` from being used + to override a maintainer's closure for non-rubric reasons (security, + duplicate, design rejection, etc.). The check is intentionally + fail-closed: any uncertainty about who closed the item must be + treated as "not the bot" so the destructive reopen path stays gated. + """ + expected = ( + bot_login + or os.environ.get("AGENT_SHIN_BOT_LOGIN") + or AGENT_SHIN_DEFAULT_BOT_LOGIN + ).lower() + actor = fetch_last_close_actor(repo, number) + if not actor: + return False + return actor.lower() == expected + + +def seconds_since_last_reconsider_verdict( + repo: str, number: int, *, bot_login: str | None = None +) -> float | None: + """Return seconds since the bot's most recent reconsider verdict comment. + + Detects comments by matching the HTML marker `RECONSIDER_COMMENT_MARKER` + appended by `format_reopen_comment` and + `format_reconsider_still_failing_comment`. Returns None when the bot + has never posted a reconsider verdict on this PR/issue (or when the + only matching comments are missing a `created_at` timestamp, which + shouldn't happen on a real GitHub response). + """ + expected_login = ( + bot_login + or os.environ.get("AGENT_SHIN_BOT_LOGIN") + or AGENT_SHIN_DEFAULT_BOT_LOGIN + ).lower() + latest: dt.datetime | None = None + for comment in _iter_paginated_json(f"repos/{repo}/issues/{number}/comments"): + author = ((comment.get("user") or {}).get("login") or "").lower() + if author != expected_login: + continue + body = comment.get("body") or "" + if RECONSIDER_COMMENT_MARKER not in body: + continue + created = comment.get("created_at") + if not created: + continue + try: + ts = dt.datetime.fromisoformat(created.replace("Z", "+00:00")) + except ValueError: + continue + if latest is None or ts > latest: + latest = ts + if latest is None: + return None + return (dt.datetime.now(dt.timezone.utc) - latest).total_seconds() + + # --------------------------------------------------------------------------- # Author classification @@ -458,6 +575,9 @@ def write_step_summary(content: str) -> None: def format_reopen_comment(kind: str) -> str: """Comment posted when Agent Shin reopens after a successful reconsider.""" noun = "PR" if kind == "pr" else "issue" + # The trailing HTML marker is used by `seconds_since_last_reconsider_verdict` + # to enforce a cooldown between repeated `@agent-shin reconsider` triggers. + # Keep the marker on its own line so it doesn't disturb the rendered text. return ( f"♻️ **Re-evaluated and reopened.** Thanks for updating the {noun}!\n" "\n" @@ -467,7 +587,9 @@ def format_reopen_comment(kind: str) -> str: "\n" "_(If a maintainer ends up closing this for non-rubric reasons, that " "decision stands; comment `@agent-shin reconsider` again only if you " - "have substantively new information.)_" + "have substantively new information.)_\n" + "\n" + f"{RECONSIDER_COMMENT_MARKER}" ) @@ -476,6 +598,8 @@ def format_reconsider_still_failing_comment(kind: str, verdict: dict) -> str: missing_lines = _format_missing(verdict.get("missing") or []) explanation = verdict.get("explanation") or "" noun = "PR" if kind == "pr" else "issue" + # The trailing HTML marker is used by `seconds_since_last_reconsider_verdict` + # to enforce a cooldown between repeated `@agent-shin reconsider` triggers. return ( f"⏸️ **Re-evaluated; this {noun} still doesn't meet the rubric.**\n" "\n" @@ -490,7 +614,9 @@ def format_reconsider_still_failing_comment(kind: str, verdict: dict) -> str: "`@agent-shin reconsider` again, or ping a maintainer if you think " "I got this wrong.\n" "\n" - "_(I'm an LLM and I'm not infallible.)_" + "_(I'm an LLM and I'm not infallible.)_\n" + "\n" + f"{RECONSIDER_COMMENT_MARKER}" ) @@ -514,10 +640,22 @@ def triage( fail-but-no-comment is replaced with a "still failing" comment + leave closed; a pass triggers `reopen_pr`/`reopen_issue` plus a reopen comment. Reconsider mode is intended for the `@agent-shin reconsider` comment - trigger. `close` is forced True implicitly when `reconsider` is set - because the bot has already decided this is a real (non-dry-run) - invocation; it's the caller's responsibility to gate on - AGENT_SHIN_ENABLED before calling reconsider mode. + trigger. Like regular triage, `close=False` keeps reconsider in dry-run + (returns `would-reopen` / `would-reconsider-still-failing` so a local + operator can preview without write side effects); the workflow only + passes `--close` when `AGENT_SHIN_ENABLED=true`. + + Reconsider mode adds two extra safety guards on top of the regular + triage skip-internal-author check: + + 1. **Bot-closed guard.** Only reopens if the most recent close was + performed by the bot identity (default `github-actions[bot]`). + This stops a contributor from using `@agent-shin reconsider` to + override a maintainer's close for non-rubric reasons. + 2. **Rate-limit guard.** If the bot has already posted a reconsider + verdict on this PR/issue within `RECONSIDER_RATE_LIMIT_SECONDS`, + skip — repeated triggers from the same contributor shouldn't burn + CI minutes or LLM budget. """ fetcher = {"pr": fetch_pr, "issue": fetch_issue}[kind] item = fetcher(repo, number) @@ -551,6 +689,20 @@ def triage( if is_internal_contributor(item): return {**base_result, "action": "skip-internal-author"} + # Reconsider-only guards — these run BEFORE the LLM call so a + # maintainer-closed PR / rate-limited trigger never spends LLM budget. + if reconsider: + if not was_closed_by_agent_shin(repo, number): + return {**base_result, "action": "skip-not-bot-closed"} + age = seconds_since_last_reconsider_verdict(repo, number) + if age is not None and age < RECONSIDER_RATE_LIMIT_SECONDS: + return { + **base_result, + "action": "skip-rate-limited", + "rate_limit_age_seconds": age, + "rate_limit_window_seconds": RECONSIDER_RATE_LIMIT_SECONDS, + } + if kind == "pr": prompt = build_pr_prompt(title=title, body=body) # Short-circuit: if body very clearly links a related issue, just pass. @@ -567,6 +719,12 @@ def triage( if reconsider: # Pass-on-reconsider -> reopen the PR with a friendly comment. reopen_body = format_reopen_comment(kind) + if not close: + return { + **base, + "action": "would-reopen", + "comment": reopen_body, + } post_comment(repo, number, reopen_body) reopen_pr(repo, number) return { @@ -607,8 +765,20 @@ def triage( # Reconsider: pass -> reopen + post reopen comment; # fail -> leave closed + post a "still failing" comment so the # contributor can iterate again. + # In dry-run (`close=False`) we return `would-*` actions instead + # of touching GitHub state, mirroring the regular triage flow's + # `would-close`. This lets a local operator preview the outcome + # of `python triage_with_llm.py --reconsider --pr N` without + # risking accidental comments or reopens. if decision != "fail": reopen_body = format_reopen_comment(kind) + if not close: + return { + **base_result, + "action": "would-reopen", + "verdict": verdict, + "comment": reopen_body, + } post_comment(repo, number, reopen_body) if kind == "pr": reopen_pr(repo, number) @@ -621,6 +791,13 @@ def triage( "comment": reopen_body, } still_failing = format_reconsider_still_failing_comment(kind, verdict) + if not close: + return { + **base_result, + "action": "would-reconsider-still-failing", + "verdict": verdict, + "comment": still_failing, + } post_comment(repo, number, still_failing) return { **base_result, diff --git a/.github/workflows/triage_reconsider.yml b/.github/workflows/triage_reconsider.yml index f048ac5d139..5b23a8d5697 100644 --- a/.github/workflows/triage_reconsider.yml +++ b/.github/workflows/triage_reconsider.yml @@ -107,20 +107,24 @@ jobs: else ARGS=(--repo "${{ github.repository }}" --issue "${NUMBER}" --reconsider) fi - # Reconsider IS the destructive path here (it can post comments - # and reopen) — there's no separate `--close` flag because the - # script's reconsider mode handles both pass (reopen) and fail - # (still-failing comment) outcomes itself. + # Reconsider's destructive actions (post comment + reopen) are + # gated on `--close`, mirroring the regular triage workflows. + # When AGENT_SHIN_ENABLED is not the EXACT string "true", we + # still run the script so its verdict + would-X action lands in + # the step summary for QA — but without `--close`, the script + # returns `would-reopen` / `would-reconsider-still-failing` + # instead of touching GitHub state. # - # Use the positive `= "true"` gate (instead of `!= "true" -> exit`) - # so the workflow guardrails in + # Use the positive `= "true"` gate (not `!= "true" -> exit`) so + # the workflow guardrails in # tests/test_litellm/test_github_triage_workflows.py see the - # canonical fail-safe enable pattern. Unknown values like "True", - # "yes", "1", or typos will fall through to the dry-run else + # canonical fail-safe enable pattern. Unknown values like + # "True", "yes", "1", or typos fall through to the dry-run # branch, which is the safe default. if [ "${AGENT_SHIN_ENABLED:-false}" = "true" ]; then - echo "::notice::Agent Shin reconsider ENABLED — running real triage." - python3 .github/scripts/triage_with_llm.py "${ARGS[@]}" + ARGS+=(--close) + echo "::notice::Agent Shin reconsider ENABLED — running real triage (close=true)." else echo "::notice::AGENT_SHIN_ENABLED is not 'true' -> reconsider stays in dry-run (no comment, no reopen)." fi + python3 .github/scripts/triage_with_llm.py "${ARGS[@]}" diff --git a/tests/test_litellm/test_github_triage_with_llm.py b/tests/test_litellm/test_github_triage_with_llm.py index bd9682159b5..a8ecd0fbea0 100644 --- a/tests/test_litellm/test_github_triage_with_llm.py +++ b/tests/test_litellm/test_github_triage_with_llm.py @@ -138,6 +138,22 @@ class TestCloseCommentText: # appear (they can't reopen a PR closed by a bot/maintainer). assert "Reopen the PR" not in body + def test_reopen_comment_should_carry_reconsider_marker(self, triage_module): + # The marker is what the rate-limit guard greps for to detect a + # prior reconsider verdict on the same PR. If the marker ever + # gets dropped from this comment, the cooldown silently breaks + # and a contributor can spam `@agent-shin reconsider` to burn + # LLM budget. + body = triage_module.format_reopen_comment("pr") + assert triage_module.RECONSIDER_COMMENT_MARKER in body + + def test_still_failing_comment_should_carry_reconsider_marker(self, triage_module): + body = triage_module.format_reconsider_still_failing_comment( + "pr", + {"verdict": "fail", "missing": ["QA proof"], "explanation": "thin"}, + ) + assert triage_module.RECONSIDER_COMMENT_MARKER in body + def test_pr_close_comment_should_not_promise_automatic_reopen_on_open( self, triage_module ): @@ -158,6 +174,158 @@ class TestCloseCommentText: assert "Reopen the issue" not in body +class TestWasClosedByAgentShin: + """Bot-closed guard: only the bot's own closures are reopen candidates.""" + + def test_should_return_true_when_last_close_actor_is_bot( + self, triage_module, monkeypatch + ): + monkeypatch.setattr( + triage_module, + "fetch_last_close_actor", + lambda repo, n: "github-actions[bot]", + ) + assert triage_module.was_closed_by_agent_shin("o/r", 1) is True + + def test_should_return_false_when_last_close_actor_is_maintainer( + self, triage_module, monkeypatch + ): + # A maintainer closed it (e.g. duplicate, security, design). The + # bot must refuse to reopen on @agent-shin reconsider. + monkeypatch.setattr( + triage_module, + "fetch_last_close_actor", + lambda repo, n: "krrishdholakia", + ) + assert triage_module.was_closed_by_agent_shin("o/r", 1) is False + + def test_should_fail_closed_when_no_close_event(self, triage_module, monkeypatch): + # If the events API returns nothing (network blip, repo permission + # quirk), the guard must fail-closed: refuse to reopen rather than + # assume the bot did it. + monkeypatch.setattr( + triage_module, "fetch_last_close_actor", lambda repo, n: None + ) + assert triage_module.was_closed_by_agent_shin("o/r", 1) is False + + def test_should_respect_bot_login_override_via_env( + self, triage_module, monkeypatch + ): + # Operators wiring Agent Shin to a PAT (instead of GITHUB_TOKEN) + # can override the expected bot login via env. The guard must + # respect the override so non-default deployments still work. + monkeypatch.setenv("AGENT_SHIN_BOT_LOGIN", "my-bot") + monkeypatch.setattr( + triage_module, "fetch_last_close_actor", lambda repo, n: "my-bot" + ) + assert triage_module.was_closed_by_agent_shin("o/r", 1) is True + # Default "github-actions[bot]" should NOT match when env is set. + monkeypatch.setattr( + triage_module, + "fetch_last_close_actor", + lambda repo, n: "github-actions[bot]", + ) + assert triage_module.was_closed_by_agent_shin("o/r", 1) is False + + +class TestSecondsSinceLastReconsiderVerdict: + """Rate-limit guard: detects the bot's own reconsider verdict marker.""" + + def _make_comment( + self, *, login: str, body: str, created_at: str | None = "2026-05-18T05:00:00Z" + ) -> dict: + comment: dict = {"user": {"login": login}, "body": body} + if created_at is not None: + comment["created_at"] = created_at + return comment + + def test_should_return_none_when_no_bot_reconsider_comments( + self, triage_module, monkeypatch + ): + # An issue with chatter from other users but no bot reconsider + # verdict must not be rate-limited. + comments = [ + self._make_comment(login="outside-dev", body="ping?"), + self._make_comment( + login="github-actions[bot]", body="some other bot message" + ), + ] + monkeypatch.setattr( + triage_module, "_iter_paginated_json", lambda *a, **kw: iter(comments) + ) + assert triage_module.seconds_since_last_reconsider_verdict("o/r", 1) is None + + def test_should_pick_latest_bot_reconsider_marker(self, triage_module, monkeypatch): + # When multiple reconsider verdicts exist, return the AGE of the + # most recent one. Using a frozen reference helps pin the math. + comments = [ + self._make_comment( + login="github-actions[bot]", + body="old verdict " + triage_module.RECONSIDER_COMMENT_MARKER, + created_at="2026-05-18T04:00:00Z", + ), + self._make_comment( + login="github-actions[bot]", + body="newer verdict " + triage_module.RECONSIDER_COMMENT_MARKER, + created_at="2026-05-18T04:55:00Z", + ), + ] + monkeypatch.setattr( + triage_module, "_iter_paginated_json", lambda *a, **kw: iter(comments) + ) + + # Freeze "now" via a tiny shim on the module's `dt` import. + import datetime as real_dt + + class FrozenDateTime(real_dt.datetime): + @classmethod + def now(cls, tz=None): + return real_dt.datetime(2026, 5, 18, 5, 0, 0, tzinfo=tz) + + frozen_module = type(triage_module.dt)("datetime") + frozen_module.datetime = FrozenDateTime + frozen_module.timezone = real_dt.timezone + monkeypatch.setattr(triage_module, "dt", frozen_module) + + age = triage_module.seconds_since_last_reconsider_verdict("o/r", 1) + # newer verdict is 5 minutes (300 seconds) before "now" + assert age == 300.0 + + def test_should_ignore_non_bot_comments_with_marker( + self, triage_module, monkeypatch + ): + # A user comment that happens to quote the marker (e.g. in + # a "what does this hidden marker do?" question) must NOT count. + # The rate-limit guard only trusts comments authored by the bot. + comments = [ + self._make_comment( + login="curious-user", + body=f"Saw this marker: {triage_module.RECONSIDER_COMMENT_MARKER}", + ), + ] + monkeypatch.setattr( + triage_module, "_iter_paginated_json", lambda *a, **kw: iter(comments) + ) + assert triage_module.seconds_since_last_reconsider_verdict("o/r", 1) is None + + def test_should_ignore_bot_comments_without_marker( + self, triage_module, monkeypatch + ): + # The bot posts other things too (Agent Shin close comments, + # CI status, etc.) — only the reconsider-verdict marker should + # arm the cooldown. + comments = [ + self._make_comment( + login="github-actions[bot]", + body="Agent Shin closed this PR (no marker)", + ), + ] + monkeypatch.setattr( + triage_module, "_iter_paginated_json", lambda *a, **kw: iter(comments) + ) + assert triage_module.seconds_since_last_reconsider_verdict("o/r", 1) is None + + class TestParseVerdict: def test_should_parse_plain_json(self, triage_module): raw = '{"verdict": "pass", "missing": []}' @@ -597,13 +765,34 @@ class TestTriageOrchestration: ) assert result["action"] == "skip-not-closed" + @staticmethod + def _stub_reconsider_guards(triage_module, monkeypatch): + """Default reconsider-guard stubs: pretend bot closed + no cooldown. + + The new safety guards (`was_closed_by_agent_shin`, + `seconds_since_last_reconsider_verdict`) hit the GitHub API in + production. Tests that exercise the reconsider happy path stub + them to "yes the bot closed it, no recent reconsider comment" + so the test stays focused on its actual assertion. + """ + monkeypatch.setattr( + triage_module, "was_closed_by_agent_shin", lambda *a, **kw: True + ) + monkeypatch.setattr( + triage_module, + "seconds_since_last_reconsider_verdict", + lambda *a, **kw: None, + ) + def test_should_reopen_on_reconsider_pass(self, triage_module, monkeypatch): # Reconsider on a closed PR with a passing verdict -> reopen + post a - # friendly "re-evaluated" comment. + # friendly "re-evaluated" comment. close=True is the production path + # (the workflow only adds --close when AGENT_SHIN_ENABLED=true). pr = self._make_pr( state="closed", body="Updated body with QA proof + screenshots." ) monkeypatch.setattr(triage_module, "fetch_pr", lambda repo, n: pr) + self._stub_reconsider_guards(triage_module, monkeypatch) posted = {} reopened = {} monkeypatch.setattr( @@ -627,7 +816,7 @@ class TestTriageOrchestration: repo="o/r", kind="pr", number=42, - close=False, + close=True, model="m", judge=lambda p: json.dumps( {"verdict": "pass", "missing": [], "explanation": "ok now"} @@ -639,11 +828,52 @@ class TestTriageOrchestration: assert posted["n"] == 42 assert "reopened" in posted["body"].lower() + def test_should_dry_run_reconsider_pass_when_close_false( + self, triage_module, monkeypatch + ): + # Reconsider must honor `close=False` (dry-run) just like the + # regular triage flow. A local invocation of + # `python triage_with_llm.py --reconsider --pr N` (no --close) + # must NOT post a comment or reopen the PR — it should return + # `would-reopen` so the operator can preview the outcome. + pr = self._make_pr( + state="closed", body="Updated body with QA proof + screenshots." + ) + monkeypatch.setattr(triage_module, "fetch_pr", lambda repo, n: pr) + self._stub_reconsider_guards(triage_module, monkeypatch) + monkeypatch.setattr( + triage_module, + "post_comment", + lambda *a, **kw: pytest.fail("must not post comment in dry-run reconsider"), + ) + monkeypatch.setattr( + triage_module, + "reopen_pr", + lambda *a, **kw: pytest.fail("must not reopen PR in dry-run reconsider"), + ) + + result = triage_module.triage( + repo="o/r", + kind="pr", + number=42, + close=False, + model="m", + judge=lambda p: json.dumps( + {"verdict": "pass", "missing": [], "explanation": "ok now"} + ), + reconsider=True, + ) + assert result["action"] == "would-reopen" + # The previewed comment body is still returned so a step-summary + # writer can render exactly what would have been posted. + assert "reopened" in result["comment"].lower() + def test_should_post_still_failing_on_reconsider_fail( self, triage_module, monkeypatch ): pr = self._make_pr(state="closed", body="still empty") monkeypatch.setattr(triage_module, "fetch_pr", lambda repo, n: pr) + self._stub_reconsider_guards(triage_module, monkeypatch) posted = {} monkeypatch.setattr( triage_module, @@ -671,7 +901,7 @@ class TestTriageOrchestration: repo="o/r", kind="pr", number=42, - close=False, + close=True, model="m", judge=lambda p: json.dumps(verdict), reconsider=True, @@ -680,6 +910,39 @@ class TestTriageOrchestration: assert posted["n"] == 42 assert "QA proof" in posted["body"] + def test_should_dry_run_reconsider_fail_when_close_false( + self, triage_module, monkeypatch + ): + # Mirror dry-run behavior for the FAIL branch — `close=False` + # must NOT post the "still failing" comment. + pr = self._make_pr(state="closed", body="still empty") + monkeypatch.setattr(triage_module, "fetch_pr", lambda repo, n: pr) + self._stub_reconsider_guards(triage_module, monkeypatch) + monkeypatch.setattr( + triage_module, + "post_comment", + lambda *a, **kw: pytest.fail( + "must not post still-failing comment in dry-run" + ), + ) + + verdict = { + "verdict": "fail", + "missing": ["QA proof"], + "explanation": "Still no QA proof.", + } + result = triage_module.triage( + repo="o/r", + kind="pr", + number=42, + close=False, + model="m", + judge=lambda p: json.dumps(verdict), + reconsider=True, + ) + assert result["action"] == "would-reconsider-still-failing" + assert "QA proof" in result["comment"] + def test_should_reopen_on_reconsider_with_linked_issue_short_circuit( self, triage_module, monkeypatch ): @@ -688,6 +951,7 @@ class TestTriageOrchestration: # path should reopen the PR without calling the LLM. pr = self._make_pr(state="closed", body="Fixes #1234\n\nAddresses the bug.") monkeypatch.setattr(triage_module, "fetch_pr", lambda repo, n: pr) + self._stub_reconsider_guards(triage_module, monkeypatch) posted = {} reopened = {} monkeypatch.setattr( @@ -705,7 +969,7 @@ class TestTriageOrchestration: repo="o/r", kind="pr", number=55, - close=False, + close=True, model="m", judge=lambda p: pytest.fail("LLM must not run when linked-issue matches"), reconsider=True, @@ -714,6 +978,35 @@ class TestTriageOrchestration: assert reopened["n"] == 55 assert "reopened" in posted["body"].lower() + def test_should_dry_run_reconsider_with_linked_issue_when_close_false( + self, triage_module, monkeypatch + ): + # Linked-issue short-circuit must ALSO honor dry-run. + pr = self._make_pr(state="closed", body="Fixes #1234\n\nAddresses the bug.") + monkeypatch.setattr(triage_module, "fetch_pr", lambda repo, n: pr) + self._stub_reconsider_guards(triage_module, monkeypatch) + monkeypatch.setattr( + triage_module, + "post_comment", + lambda *a, **kw: pytest.fail("must not post in dry-run"), + ) + monkeypatch.setattr( + triage_module, + "reopen_pr", + lambda *a, **kw: pytest.fail("must not reopen in dry-run"), + ) + + result = triage_module.triage( + repo="o/r", + kind="pr", + number=55, + close=False, + model="m", + judge=lambda p: pytest.fail("LLM must not run when linked-issue matches"), + reconsider=True, + ) + assert result["action"] == "would-reopen" + def test_should_skip_internal_in_reconsider_mode(self, triage_module, monkeypatch): # Internal authors are exempt from triage in both regular and # reconsider mode — Agent Shin should never reopen one of their PRs @@ -740,6 +1033,138 @@ class TestTriageOrchestration: ) assert result["action"] == "skip-internal-author" + def test_should_skip_reconsider_when_not_bot_closed( + self, triage_module, monkeypatch + ): + # SECURITY: `@agent-shin reconsider` must NOT reopen a PR/issue + # that a MAINTAINER closed for non-rubric reasons (e.g. duplicate, + # design rejection, security report). Only PRs closed by the bot + # itself should ever be candidates for the reconsider reopen path. + pr = self._make_pr(state="closed", body="something.") + monkeypatch.setattr(triage_module, "fetch_pr", lambda repo, n: pr) + monkeypatch.setattr( + triage_module, "was_closed_by_agent_shin", lambda *a, **kw: False + ) + # Even though there's no rate-limit conflict, the bot-closed guard + # alone is sufficient to block. The LLM judge must never run on a + # maintainer-closed PR. + monkeypatch.setattr( + triage_module, + "seconds_since_last_reconsider_verdict", + lambda *a, **kw: None, + ) + monkeypatch.setattr( + triage_module, + "post_comment", + lambda *a, **kw: pytest.fail("must not comment on maintainer-closed PR"), + ) + monkeypatch.setattr( + triage_module, + "reopen_pr", + lambda *a, **kw: pytest.fail("must not reopen maintainer-closed PR"), + ) + + result = triage_module.triage( + repo="o/r", + kind="pr", + number=1, + close=True, + model="m", + judge=lambda p: pytest.fail("LLM must not run before bot-closed guard"), + reconsider=True, + ) + assert result["action"] == "skip-not-bot-closed" + + def test_should_rate_limit_repeated_reconsider_triggers( + self, triage_module, monkeypatch + ): + # COST CONTROL: each `@agent-shin reconsider` event burns CI + # minutes + an OpenAI API call. If the bot already posted a + # reconsider verdict within the cooldown window + # (RECONSIDER_RATE_LIMIT_SECONDS), refuse to run again. This + # bounds the damage from a contributor spamming the trigger. + pr = self._make_pr(state="closed", body="something with new edits.") + monkeypatch.setattr(triage_module, "fetch_pr", lambda repo, n: pr) + monkeypatch.setattr( + triage_module, "was_closed_by_agent_shin", lambda *a, **kw: True + ) + # Pretend the bot posted a reconsider verdict 1 second ago. + monkeypatch.setattr( + triage_module, + "seconds_since_last_reconsider_verdict", + lambda *a, **kw: 1.0, + ) + monkeypatch.setattr( + triage_module, + "post_comment", + lambda *a, **kw: pytest.fail("must not comment during cooldown"), + ) + monkeypatch.setattr( + triage_module, + "reopen_pr", + lambda *a, **kw: pytest.fail("must not reopen during cooldown"), + ) + + result = triage_module.triage( + repo="o/r", + kind="pr", + number=1, + close=True, + model="m", + judge=lambda p: pytest.fail("LLM must not run during cooldown"), + reconsider=True, + ) + assert result["action"] == "skip-rate-limited" + assert result["rate_limit_age_seconds"] == 1.0 + assert ( + result["rate_limit_window_seconds"] + == triage_module.RECONSIDER_RATE_LIMIT_SECONDS + ) + + def test_should_allow_reconsider_after_cooldown_window( + self, triage_module, monkeypatch + ): + # The cooldown is a window, not a one-shot lock — once + # RECONSIDER_RATE_LIMIT_SECONDS has elapsed since the last bot + # verdict, a fresh `@agent-shin reconsider` is allowed through. + pr = self._make_pr(state="closed", body="updated with screenshots now.") + monkeypatch.setattr(triage_module, "fetch_pr", lambda repo, n: pr) + monkeypatch.setattr( + triage_module, "was_closed_by_agent_shin", lambda *a, **kw: True + ) + # Last reconsider was 1 hour ago — well outside the 10-min window. + monkeypatch.setattr( + triage_module, + "seconds_since_last_reconsider_verdict", + lambda *a, **kw: 3600.0, + ) + posted = {} + reopened = {} + monkeypatch.setattr( + triage_module, + "post_comment", + lambda repo, n, body: posted.update({"n": n, "body": body}), + ) + monkeypatch.setattr( + triage_module, + "reopen_pr", + lambda repo, n: reopened.update({"n": n}), + ) + + result = triage_module.triage( + repo="o/r", + kind="pr", + number=1, + close=True, + model="m", + judge=lambda p: json.dumps( + {"verdict": "pass", "missing": [], "explanation": "ok"} + ), + reconsider=True, + ) + assert result["action"] == "reopened" + assert reopened["n"] == 1 + def test_should_reopen_issue_on_reconsider_pass(self, triage_module, monkeypatch): issue = { "number": 7, @@ -750,6 +1175,7 @@ class TestTriageOrchestration: "user": {"login": "outside"}, } monkeypatch.setattr(triage_module, "fetch_issue", lambda repo, n: issue) + self._stub_reconsider_guards(triage_module, monkeypatch) posted = {} reopened = {} monkeypatch.setattr( @@ -767,7 +1193,7 @@ class TestTriageOrchestration: repo="o/r", kind="issue", number=7, - close=False, + close=True, model="m", judge=lambda p: json.dumps( {"verdict": "pass", "missing": [], "explanation": "now reproducible"} From f876173b78f493bc661023cb928f66efb6d79e7b Mon Sep 17 00:00:00 2001 From: Cursor Agent Date: Tue, 19 May 2026 07:35:24 +0000 Subject: [PATCH 15/17] feat(triage): 1-day grace period before close + SwiftWinds immediate-close bypass MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Add a 24-hour grace window between the first low-quality detection and the actual auto-close. The first detection posts a warning comment that explicitly says "You have 1 day to address this before this PR is auto-closed" and points the contributor at: * `@agent-shin reconsider` to request another look (and re-open) * `@greptileai` to request a fresh Greptile review — works even after the PR is closed - Both `triage_with_llm.py` (LLM judge) and `close_low_quality_prs.py` (Greptile-score closer) share the same `` HTML marker so a warning posted by either path is recognized by both. - Add IMMEDIATE_CLOSE_LOGINS = {swiftwinds} to bypass BOTH the grace period AND the dry-run / AGENT_SHIN_ENABLED gating. SwiftWinds is the user's personal account (no push permissions to litellm) used to dogfood the bot; user explicitly asked: "For SwiftWinds, just close immediately. Faster iteration that way." - Update the standard close comments to mention that `@greptileai` works even after the PR is closed. - Add 23 new tests covering: warn-grace on first detection, skip during grace window, close after grace expires, SwiftWinds bypass (case insensitive, with close=False, no random-login false positives), the grace-warning text invariants, and the SwiftWinds entry in the IMMEDIATE_CLOSE_LOGINS constant. Co-authored-by: Mateo Wang --- .github/scripts/close_low_quality_prs.py | 214 +++++++- .github/scripts/triage_with_llm.py | 218 +++++++- .../test_github_close_low_quality_prs.py | 242 ++++++++- .../test_github_triage_with_llm.py | 478 +++++++++++++++++- 4 files changed, 1120 insertions(+), 32 deletions(-) diff --git a/.github/scripts/close_low_quality_prs.py b/.github/scripts/close_low_quality_prs.py index d7363f88328..85b281e959d 100644 --- a/.github/scripts/close_low_quality_prs.py +++ b/.github/scripts/close_low_quality_prs.py @@ -38,6 +38,7 @@ from __future__ import annotations import argparse import datetime as dt import json +import os import re import subprocess import sys @@ -66,6 +67,33 @@ INTERNAL_AUTHOR_ASSOCIATIONS = frozenset({"OWNER", "MEMBER", "COLLABORATOR"}) # `default=[...]` combination silently mutates the shared default list. DEFAULT_OPTOUT_LABELS = ("do not close", "keep open", "wip") +# HTML marker appended to grace-period warning comments. Shared with the +# Agent Shin LLM-judge script (`triage_with_llm.py`) so a warning posted +# by either path is recognized by both: the LLM judge can see "Greptile +# already warned this contributor 12 hours ago" and skip re-warning, and +# the Greptile closer can see "Agent Shin already warned" and close on +# the next run if Greptile still has a low score. +GRACE_COMMENT_MARKER = "" + +# Length of the grace period between the warning comment and the actual +# auto-close. Set to 24 hours so the contributor has at least one full +# working day across any time zone to push fixes or comment +# `@agent-shin reconsider`. Mirrors the constant of the same name in +# `triage_with_llm.py` — keep them in sync if either changes. +GRACE_PERIOD_SECONDS = 86400 + +# Default login of the GitHub identity that performs Agent Shin's writes; +# used for matching the author of a grace-warning comment so we don't +# count somebody quoting the marker. The env override +# `AGENT_SHIN_BOT_LOGIN` mirrors `triage_with_llm.py`. +AGENT_SHIN_DEFAULT_BOT_LOGIN = "github-actions[bot]" + +# Logins (case-insensitive) that bypass BOTH the 1-day grace period AND +# the dry-run gating. Mirrors `IMMEDIATE_CLOSE_LOGINS` in +# `triage_with_llm.py`. Used for dogfooding the bot from external test +# accounts that have no push permissions to the repo. +IMMEDIATE_CLOSE_LOGINS = frozenset({"swiftwinds"}) + def gh(*args: str) -> str: """Run a `gh` CLI command and return stdout. Raises on non-zero exit.""" @@ -196,6 +224,124 @@ def has_optout_label(pr: dict, optout_labels: set[str]) -> bool: return bool(labels & {lbl.lower() for lbl in optout_labels}) +def seconds_since_last_grace_warning( + comments: Iterable[dict], + *, + bot_login: str | None = None, + now: dt.datetime | None = None, +) -> float | None: + """Return seconds since the bot's most recent grace-period warning, or + None if no such warning has ever been posted on this PR. + + Detects warnings by matching `GRACE_COMMENT_MARKER` in comments + authored by the bot identity. Operates on an already-fetched + comments list (avoids a second `gh api` call when the caller has + already pulled the page for Greptile-score extraction). + + `now` is injectable so callers (and tests) can pin the reference + time. The closer runs everything against a single `now` snapshot + captured at the top of `main()` so age calculations stay consistent + across many PRs in a single run. + """ + expected_login = ( + bot_login + or os.environ.get("AGENT_SHIN_BOT_LOGIN") + or AGENT_SHIN_DEFAULT_BOT_LOGIN + ).lower() + latest: dt.datetime | None = None + for comment in comments: + author = ((comment.get("user") or {}).get("login") or "").lower() + if author != expected_login: + continue + body = comment.get("body") or "" + if GRACE_COMMENT_MARKER not in body: + continue + created = comment.get("created_at") + if not created: + continue + try: + ts = parse_iso8601(created) + except ValueError: + continue + if latest is None or ts > latest: + latest = ts + if latest is None: + return None + reference = now if now is not None else dt.datetime.now(dt.timezone.utc) + return (reference - latest).total_seconds() + + +def format_grace_warning_comment(score: int, threshold: int) -> str: + """Comment posted on the FIRST low-Greptile-score detection — gives + the contributor a 1-day grace window before the auto-close fires on + the next daily cron run. + + Mirrors `format_grace_warning_pr_comment` in + `triage_with_llm.py` in spirit (1-day grace + escape hatches), but + framed around Greptile's confidence score instead of the LLM judge's + rubric since the close trigger here is the Greptile signal. + """ + return ( + "👋 Hi, thanks for the PR! I'm **Agent Shin**, the automated triage bot for this repository.\n" + "\n" + "Heads up — Greptile's most recent review scored this PR " + f"**{score}/5**, below our merge bar of **{threshold}/5**.\n" + "\n" + "⏳ **You have 1 day to address Greptile's feedback before this PR is auto-closed.** " + "We close low-confidence PRs aggressively to keep the review queue manageable for " + "maintainers and contributors alike. **This isn't a rejection of the idea.**\n" + "\n" + "During the grace period:\n" + "\n" + "1. Push fixes that address Greptile's feedback (continue using your existing branch is fine).\n" + "2. Either:\n" + " - Comment `@greptileai` to request a fresh Greptile review. If the new score is " + f"**{threshold}/5 or higher**, the PR stays open.\n" + " - Or comment `@agent-shin reconsider` to have Agent Shin re-evaluate the PR description.\n" + "\n" + "If this PR is auto-closed in 24 hours, you'll still have options:\n" + "\n" + "- Comment `@agent-shin reconsider` after pushing fixes — Agent Shin will re-run triage " + "and reopen the PR if it now meets the bar.\n" + "- Comment `@greptileai` to request a re-review — that works **even after the PR is closed**.\n" + "\n" + "Thanks for contributing to LiteLLM. We know auto-closures can sting; the goal is to keep " + "the project healthy, not to dismiss your work.\n" + "\n" + f"{GRACE_COMMENT_MARKER}" + ) + + +def post_grace_warning( + pr: dict, + score: int, + threshold: int, + repo: str | None, + dry_run: bool, +) -> None: + """Post the 1-day grace-period warning comment on `pr`. + + The warning carries `GRACE_COMMENT_MARKER` so subsequent runs can + detect that the contributor has already been told about the + pending close. Does NOT close the PR — the close happens on the + next eligible run after `GRACE_PERIOD_SECONDS` elapses (handled + by `close_pr`). + """ + pr_number = pr["number"] + repo_args = ["--repo", repo] if repo else [] + + if dry_run: + print( + f" [DRY RUN] Would post grace warning to PR #{pr_number} " + f"(greptile={score}/5): {pr['title']}" + ) + return + + comment_body = format_grace_warning_comment(score, threshold) + gh("pr", "comment", str(pr_number), "--body", comment_body, *repo_args) + print(f" Posted grace warning on PR #{pr_number} (greptile={score}/5)") + + def close_pr( pr: dict, score: int, @@ -219,7 +365,8 @@ def close_pr( comment_body = ( f"Closing as part of automated PR triage.\n\n" f"Greptile's most recent review scored this PR **{score}/5**, below " - f"our merge bar of **{threshold}/5**.\n\n" + f"our merge bar of **{threshold}/5**, and the 1-day grace period since " + "the warning has elapsed.\n\n" "We close low-confidence PRs aggressively to keep the review queue " "manageable for maintainers and contributors alike. **This is not a " "rejection of the idea** — to bring this back:\n\n" @@ -233,7 +380,9 @@ def close_pr( "maintainer, so a fresh PR is the most reliable path forward. If you " "would prefer this exact PR re-evaluated, comment " "`@agent-shin reconsider` once you've pushed the fixes — Agent Shin " - "will re-run triage and reopen this PR if it now meets the bar.\n\n" + "will re-run triage and reopen this PR if it now meets the bar. " + "You can also comment `@greptileai` to request a fresh Greptile " + "review — that works **even after the PR is closed**.\n\n" "Thanks for contributing to LiteLLM. We know auto-closures can sting; " "the goal is to keep the project healthy, not to dismiss your work." ) @@ -258,16 +407,28 @@ def evaluate_pr( repo: str | None, optout_labels: set[str], ) -> tuple[str, int | None, int | None]: - """Decide whether to close `pr`. + """Decide what to do with `pr` on this triage run. Returns (action, score_or_none, age_days_or_none) where action is one of: "skip-too-young", "skip-optout-label", "skip-internal", - "skip-no-greptile-score", "skip-score-ok", or "close". + "skip-no-greptile-score", "skip-score-ok", + "warn-grace", "skip-in-grace-period", or "close". Drafts are NOT skipped — the goal is "open PR count == PRs internal collaborators need to action on", and a draft that Greptile scored <4/5 is still in that queue. Authors can opt out via the `wip` label (see `DEFAULT_OPTOUT_LABELS`) if they need to keep a long-lived draft open. + + Grace-period semantics: the first time a PR fails the rubric, the + action is `warn-grace` — the caller should post a warning comment but + NOT close the PR. On a subsequent run, if the warning is still less + than `GRACE_PERIOD_SECONDS` old AND the PR still fails, the action is + `skip-in-grace-period`. Once the warning ages out and the rubric is + still failing, the action is `close`. + + Grace is bypassed for `IMMEDIATE_CLOSE_LOGINS` (test/dogfood + accounts), which always go straight to `close` on the first failing + run so the bot is dogfoodable end-to-end without a 24h delay. """ if has_optout_label(pr, optout_labels): return ("skip-optout-label", None, None) @@ -294,6 +455,16 @@ def evaluate_pr( if score >= min_score: return ("skip-score-ok", score, age_days) + login = ((pr.get("author") or {}).get("login") or "").lower() + if login in IMMEDIATE_CLOSE_LOGINS: + return ("close", score, age_days) + + grace_age = seconds_since_last_grace_warning(comments, now=now) + if grace_age is None: + return ("warn-grace", score, age_days) + if grace_age < GRACE_PERIOD_SECONDS: + return ("skip-in-grace-period", score, age_days) + return ("close", score, age_days) @@ -370,6 +541,8 @@ def main() -> int: closed = 0 summary = { "close": 0, + "warn-grace": 0, + "skip-in-grace-period": 0, "skip-too-young": 0, "skip-optout-label": 0, "skip-internal": 0, @@ -388,6 +561,30 @@ def main() -> int: ) summary[action] = summary.get(action, 0) + 1 + # Per-PR dry-run override: `IMMEDIATE_CLOSE_LOGINS` accounts (e.g. + # SwiftWinds) always run in real-close mode regardless of the + # global `--close` flag. Lets a maintainer dogfood the bot from + # an external account while the rest of the open-PR queue stays + # on the safe dry-run default. + author_login = ((pr.get("author") or {}).get("login") or "").lower() + is_immediate = author_login in IMMEDIATE_CLOSE_LOGINS + pr_dry_run = dry_run and not is_immediate + + if action == "warn-grace": + assert score is not None + print( + f"#{pr['number']}: \"{pr['title']}\" " + f"(age={age_days}d, greptile={score}/5) -> warn-grace" + ) + post_grace_warning( + pr, + score=score, + threshold=args.min_score, + repo=args.repo, + dry_run=pr_dry_run, + ) + continue + if action != "close": continue @@ -395,6 +592,7 @@ def main() -> int: print( f"#{pr['number']}: \"{pr['title']}\" " f"(age={age_days}d, greptile={score}/5) -> close" + + (" [immediate-close login]" if is_immediate else "") ) close_pr( pr, @@ -402,11 +600,11 @@ def main() -> int: threshold=args.min_score, age_days=age_days, repo=args.repo, - dry_run=dry_run, + dry_run=pr_dry_run, label=args.close_label, ) - if not dry_run: + if not pr_dry_run: closed += 1 if args.limit is not None and closed >= args.limit: print(f"\nReached --limit={args.limit}; stopping.") @@ -416,6 +614,10 @@ def main() -> int: for key, value in summary.items(): print(f" {key:28s} {value}") print(f"\nTotal {'would close' if dry_run else 'closed'}: {summary['close']}") + print( + f"Total {'would warn (grace)' if dry_run else 'warned (grace)'}: " + f"{summary['warn-grace']}" + ) return 0 diff --git a/.github/scripts/triage_with_llm.py b/.github/scripts/triage_with_llm.py index 94052c32e7b..2442c061b98 100644 --- a/.github/scripts/triage_with_llm.py +++ b/.github/scripts/triage_with_llm.py @@ -62,6 +62,27 @@ RECONSIDER_COMMENT_MARKER = "" # the body" iteration loop isn't punished. RECONSIDER_RATE_LIMIT_SECONDS = 600 +# HTML marker appended to the grace-period warning comment posted on the +# first low-quality detection. We grep for this on subsequent triage runs +# to (a) detect that a warning was already posted (so we don't spam the +# contributor with duplicate warnings) and (b) measure how long ago it +# was posted so we know when the grace period has elapsed. +GRACE_COMMENT_MARKER = "" + +# Length of the grace period between the warning comment and the actual +# auto-close. Set to 24 hours so the contributor has at least one full +# working day across any time zone to push fixes or comment +# `@agent-shin reconsider`. +GRACE_PERIOD_SECONDS = 86400 + +# Logins (case-insensitive) that bypass BOTH the 1-day grace period AND +# the dry-run / `AGENT_SHIN_ENABLED` workflow gating — every Agent Shin +# verdict against a PR/issue from one of these accounts is treated as a +# real run with immediate close on fail. Useful for dogfooding the bot +# from an external account that has no push permissions to the repo. +# Listed lower-case so callers compare via `login.lower() in ...`. +IMMEDIATE_CLOSE_LOGINS = frozenset({"swiftwinds"}) + # Model families that require `reasoning_effort` to be set, and that reject # `temperature != 1` unless `reasoning_effort` is "none". For these models we # pass `reasoning_effort="none"` so a `temperature=0` deterministic judgment @@ -238,17 +259,20 @@ def was_closed_by_agent_shin( return actor.lower() == expected -def seconds_since_last_reconsider_verdict( - repo: str, number: int, *, bot_login: str | None = None +def _seconds_since_latest_marker_comment( + repo: str, + number: int, + *, + marker: str, + bot_login: str | None = None, ) -> float | None: - """Return seconds since the bot's most recent reconsider verdict comment. + """Shared helper: return seconds since the bot's most recent comment + that contains the given HTML marker, or None if no such comment exists. - Detects comments by matching the HTML marker `RECONSIDER_COMMENT_MARKER` - appended by `format_reopen_comment` and - `format_reconsider_still_failing_comment`. Returns None when the bot - has never posted a reconsider verdict on this PR/issue (or when the - only matching comments are missing a `created_at` timestamp, which - shouldn't happen on a real GitHub response). + Used by both the reconsider-verdict cooldown and the grace-period + warning detection — keeping the iteration logic centralized stops the + two paths from drifting (e.g. one fixing a tz parsing bug and the + other forgetting to mirror it). """ expected_login = ( bot_login @@ -261,7 +285,7 @@ def seconds_since_last_reconsider_verdict( if author != expected_login: continue body = comment.get("body") or "" - if RECONSIDER_COMMENT_MARKER not in body: + if marker not in body: continue created = comment.get("created_at") if not created: @@ -277,6 +301,39 @@ def seconds_since_last_reconsider_verdict( return (dt.datetime.now(dt.timezone.utc) - latest).total_seconds() +def seconds_since_last_reconsider_verdict( + repo: str, number: int, *, bot_login: str | None = None +) -> float | None: + """Return seconds since the bot's most recent reconsider verdict comment. + + Detects comments by matching the HTML marker `RECONSIDER_COMMENT_MARKER` + appended by `format_reopen_comment` and + `format_reconsider_still_failing_comment`. Returns None when the bot + has never posted a reconsider verdict on this PR/issue (or when the + only matching comments are missing a `created_at` timestamp, which + shouldn't happen on a real GitHub response). + """ + return _seconds_since_latest_marker_comment( + repo, number, marker=RECONSIDER_COMMENT_MARKER, bot_login=bot_login + ) + + +def seconds_since_last_grace_warning( + repo: str, number: int, *, bot_login: str | None = None +) -> float | None: + """Return seconds since the bot's most recent grace-period warning. + + Detects warning comments by matching the HTML marker + `GRACE_COMMENT_MARKER` appended by `format_grace_warning_pr_comment` + and `format_grace_warning_issue_comment`. Returns None when no + grace warning has ever been posted on this PR/issue — that's the + "first low-quality detection" signal that drives the warning path. + """ + return _seconds_since_latest_marker_comment( + repo, number, marker=GRACE_COMMENT_MARKER, bot_login=bot_login + ) + + # --------------------------------------------------------------------------- # Author classification @@ -510,6 +567,9 @@ def format_pr_close_comment(verdict: dict) -> str: "to get back into the review queue.\n" " - **Or** comment `@agent-shin reconsider` on this closed PR after updating the description. " "I'll re-run the triage; if it now passes, I'll reopen this PR automatically.\n" + " - You can also comment `@greptileai` on this PR to request a fresh Greptile review — that " + "still works **even after the PR is closed**, and a higher score is one of the signals that " + "lifts the PR back into the queue.\n" "\n" "Internal BerriAI contributors: this rubric doesn't apply to you — ping a maintainer.\n" "\n" @@ -550,6 +610,90 @@ def format_issue_close_comment(verdict: dict) -> str: ) +def format_grace_warning_pr_comment(verdict: dict) -> str: + """Comment posted on the FIRST low-quality detection — gives the + contributor a 1-day grace window to fix the PR before the next + triage run actually closes it. + + This is the "before-close" warning. On the second triage run, if the + grace marker is older than `GRACE_PERIOD_SECONDS` AND the PR still + fails the rubric, the close path runs (which posts + `format_pr_close_comment` and closes the PR). + """ + missing_lines = _format_missing(verdict.get("missing") or []) + explanation = verdict.get("explanation") or "" + return ( + "👋 Hi, thanks for the PR! I'm **Agent Shin**, the automated triage bot for this repository.\n" + "\n" + "Heads up — this PR does not yet meet the bar described in our " + "[pull-request template](https://github.com/BerriAI/litellm/blob/main/.github/pull_request_template.md). " + "Specifically, I couldn't find:\n" + "\n" + f"{missing_lines}\n" + "\n" + f"> {explanation}\n" + "\n" + "⏳ **You have 1 day to address this before this PR is auto-closed.** " + "During the grace period:\n" + "\n" + "1. Update the PR description to either:\n" + " - Link a related GitHub issue (e.g. `Fixes #1234`), OR\n" + " - Add a clear **problem description**, **expected vs. actual behavior**, and **visual QA proof** " + "(before/after screenshots, a short screen recording, or terminal/log output).\n" + "2. Comment `@agent-shin reconsider` on this PR after updating it. If your update meets the " + "bar, I'll skip the auto-close and a maintainer will take another look.\n" + "\n" + "If this PR is auto-closed in 24 hours, you'll still have options:\n" + "\n" + "- Comment `@agent-shin reconsider` to have me re-evaluate (and reopen the PR if it now meets the bar).\n" + "- Comment `@greptileai` to request a fresh Greptile review — that works **even after the PR is closed**.\n" + "\n" + "Internal BerriAI contributors: this rubric doesn't apply to you — ping a maintainer.\n" + "\n" + "_(I'm an LLM, so I'm not infallible. If you think I got this wrong, comment " + "`@agent-shin reconsider` or ping a maintainer — they'll override me.)_\n" + "\n" + f"{GRACE_COMMENT_MARKER}" + ) + + +def format_grace_warning_issue_comment(verdict: dict) -> str: + """Issue analogue of `format_grace_warning_pr_comment`.""" + missing_lines = _format_missing(verdict.get("missing") or []) + explanation = verdict.get("explanation") or "" + return ( + "👋 Hi, thanks for filing this! I'm **Agent Shin**, the automated triage bot for this repository.\n" + "\n" + "Heads up — this issue doesn't yet have enough detail for a maintainer to act on. " + "Specifically, I couldn't find:\n" + "\n" + f"{missing_lines}\n" + "\n" + f"> {explanation}\n" + "\n" + "⏳ **You have 1 day to address this before this issue is auto-closed.** " + "During the grace period:\n" + "\n" + "1. Edit the issue to add the missing pieces:\n" + " - For **bug reports**: a runnable reproduction (code / curl / config), expected vs. actual behavior, " + "and a screenshot / traceback / log showing the bug.\n" + " - For **feature requests**: a concrete description of what should change, plus a use case and example " + "(config / API call / UI flow).\n" + "2. Comment `@agent-shin reconsider` on this issue after updating it. If your update meets the bar, " + "I'll skip the auto-close and a maintainer will take another look.\n" + "\n" + "If this issue is auto-closed in 24 hours, you can still comment `@agent-shin reconsider` to have " + "me re-evaluate (and reopen the issue if it now meets the bar).\n" + "\n" + "Internal BerriAI contributors: this rubric doesn't apply to you — ping a maintainer.\n" + "\n" + "_(I'm an LLM, so I'm not infallible. If you think I got this wrong, comment " + "`@agent-shin reconsider` or ping a maintainer — they'll override me.)_\n" + "\n" + f"{GRACE_COMMENT_MARKER}" + ) + + # --------------------------------------------------------------------------- # Step-summary helpers @@ -809,7 +953,58 @@ def triage( if decision != "fail": return {**base_result, "action": "pass-llm", "verdict": verdict} - if not close: + # Grace-period flow: on the first low-quality detection, post a warning + # comment instead of closing immediately. On a subsequent triage run + # (manual re-trigger, or the daily `close_low_quality_prs.py` cron + # finding the same PR in its own pass), if `GRACE_PERIOD_SECONDS` has + # elapsed since the warning AND the PR still fails the rubric, close. + # + # `IMMEDIATE_CLOSE_LOGINS` (e.g. test/dogfood accounts like SwiftWinds) + # bypass the grace period entirely — every fail is treated as a real + # close run. This is intentional: those accounts exist specifically to + # exercise the bot end-to-end, and waiting a day per iteration kills + # the feedback loop. + is_immediate = login.lower() in IMMEDIATE_CLOSE_LOGINS + + if not is_immediate: + grace_age = seconds_since_last_grace_warning(repo, number) + if grace_age is None: + warning_body = ( + format_grace_warning_pr_comment(verdict) + if kind == "pr" + else format_grace_warning_issue_comment(verdict) + ) + if not close: + return { + **base_result, + "action": "would-warn-grace", + "verdict": verdict, + "comment": warning_body, + } + post_comment(repo, number, warning_body) + return { + **base_result, + "action": "warned-grace", + "verdict": verdict, + "comment": warning_body, + } + if grace_age < GRACE_PERIOD_SECONDS: + return { + **base_result, + "action": "skip-in-grace-period", + "verdict": verdict, + "grace_age_seconds": grace_age, + "grace_period_seconds": GRACE_PERIOD_SECONDS, + } + + # Either the grace window has elapsed or this author bypasses grace — + # proceed to the actual close path. `--close` still gates the + # destructive write for the regular-author path so a local operator + # can preview a "would close after grace" verdict; immediate-close + # accounts (`is_immediate`) ignore `--close` so a workflow that + # forces dry-run for the global population can still take real + # action on those test accounts. + if not close and not is_immediate: return {**base_result, "action": "would-close", "verdict": verdict} comment_body = ( @@ -828,6 +1023,7 @@ def triage( "action": "closed", "verdict": verdict, "comment": comment_body, + "immediate_close": is_immediate, } diff --git a/tests/test_litellm/test_github_close_low_quality_prs.py b/tests/test_litellm/test_github_close_low_quality_prs.py index 141fb7f8c02..cda28036c75 100644 --- a/tests/test_litellm/test_github_close_low_quality_prs.py +++ b/tests/test_litellm/test_github_close_low_quality_prs.py @@ -125,6 +125,7 @@ class TestEvaluatePr: created_days_ago: int = 10, is_draft: bool = False, labels: list[str] | None = None, + author_login: str = "someone", ) -> dict: created = dt.datetime(2026, 5, 17, tzinfo=dt.timezone.utc) - dt.timedelta( days=created_days_ago @@ -135,7 +136,7 @@ class TestEvaluatePr: "createdAt": created.isoformat().replace("+00:00", "Z"), "isDraft": is_draft, "labels": [{"name": lbl} for lbl in (labels or [])], - "author": {"login": "someone"}, + "author": {"login": author_login}, "url": f"https://example.com/pr/{number}", } @@ -146,10 +147,13 @@ class TestEvaluatePr: closer_module, "is_external_pr_author", lambda pr, repo: True ) - def test_should_close_drafts_when_score_low(self, closer_module, _now, monkeypatch): + def test_should_warn_drafts_when_score_low_first_time( + self, closer_module, _now, monkeypatch + ): # Drafts are NOT a free pass — the open-PR queue should reflect any # PR that needs human attention regardless of draft status. Authors # who need a long-lived draft can use the `wip` opt-out label. + # First run: warn the contributor (1-day grace), don't close yet. monkeypatch.setattr( closer_module, "fetch_pr_comments", @@ -163,15 +167,17 @@ class TestEvaluatePr: repo=None, optout_labels=set(), ) - assert action == "close" + assert action == "warn-grace" assert score == 2 and age == 0 - def test_should_close_brand_new_pr_when_min_age_zero( + def test_should_warn_brand_new_pr_when_min_age_zero( self, closer_module, _now, monkeypatch ): # `min_age_days=0` means no age filter — a freshly-opened PR is - # eligible the moment Greptile scores it below threshold. This is - # the new default behavior. + # eligible the moment Greptile scores it below threshold. The + # first detection still goes through the warn-grace step rather + # than closing immediately, giving the contributor 24 hours to + # respond before the next run actually closes the PR. monkeypatch.setattr( closer_module, "fetch_pr_comments", @@ -185,7 +191,7 @@ class TestEvaluatePr: repo=None, optout_labels=set(), ) - assert action == "close" + assert action == "warn-grace" assert score == 1 and age == 0 def test_should_skip_optout_label_case_insensitive( @@ -284,9 +290,12 @@ class TestEvaluatePr: assert action == "skip-score-ok" assert score == 4 and age == 10 - def test_should_close_when_old_and_low_score( + def test_should_warn_when_old_and_low_score_no_prior_warning( self, closer_module, _now, monkeypatch ): + # Even an old PR that still has no grace warning gets one on the + # first eligible run — the daily cron is the natural cadence, so + # an existing-but-never-warned PR enters the grace flow normally. monkeypatch.setattr( closer_module, "fetch_pr_comments", @@ -300,16 +309,37 @@ class TestEvaluatePr: repo=None, optout_labels=set(), ) - assert action == "close" + assert action == "warn-grace" assert score == 3 and age == 10 - def test_should_close_when_old_and_very_low_score( + def test_should_close_when_grace_warning_aged_out_and_score_still_low( self, closer_module, _now, monkeypatch ): + # Day-1 the closer posted a warning. Day-2 the PR still scores <4 + # AND the warning is older than `GRACE_PERIOD_SECONDS`, so the + # action flips to `close`. This is the "grace expired" path. + old_warning = { + "user": {"login": "github-actions[bot]"}, + "body": ( + "you have 1 day to fix this\n\n" + closer_module.GRACE_COMMENT_MARKER + ), + "created_at": ( + _now - dt.timedelta(seconds=closer_module.GRACE_PERIOD_SECONDS + 60) + ) + .isoformat() + .replace("+00:00", "Z"), + "updated_at": "2026-05-15T00:00:00Z", + } monkeypatch.setattr( closer_module, "fetch_pr_comments", - lambda *a, **kw: [_greptile_comment("

Confidence Score: 1/5

")], + lambda *a, **kw: [ + _greptile_comment( + "

Confidence Score: 1/5

", + updated_at="2026-05-15T00:00:00Z", + ), + old_warning, + ], ) action, score, _ = closer_module.evaluate_pr( self._make_pr(created_days_ago=14), @@ -322,6 +352,82 @@ class TestEvaluatePr: assert action == "close" assert score == 1 + def test_should_skip_when_grace_warning_within_window( + self, closer_module, _now, monkeypatch + ): + # Within the 24-hour grace window the closer must NOT close the + # PR even if the score is still low. The warning is only an hour + # old; give the contributor time to push fixes before destruction. + recent_warning = { + "user": {"login": "github-actions[bot]"}, + "body": "warning text\n\n" + closer_module.GRACE_COMMENT_MARKER, + "created_at": (_now - dt.timedelta(hours=1)) + .isoformat() + .replace("+00:00", "Z"), + } + monkeypatch.setattr( + closer_module, + "fetch_pr_comments", + lambda *a, **kw: [ + _greptile_comment("Confidence Score: 2/5"), + recent_warning, + ], + ) + action, score, _ = closer_module.evaluate_pr( + self._make_pr(created_days_ago=10), + now=_now, + min_age_days=0, + min_score=4, + repo=None, + optout_labels=set(), + ) + assert action == "skip-in-grace-period" + assert score == 2 + + def test_should_close_immediately_for_swiftwinds_login( + self, closer_module, _now, monkeypatch + ): + # SwiftWinds is in `IMMEDIATE_CLOSE_LOGINS` for dogfooding the bot + # from an external account. Skip grace; close on first detection + # so the iteration loop is fast. + monkeypatch.setattr( + closer_module, + "fetch_pr_comments", + lambda *a, **kw: [_greptile_comment("Confidence Score: 1/5")], + ) + action, score, _ = closer_module.evaluate_pr( + self._make_pr(created_days_ago=0, author_login="SwiftWinds"), + now=_now, + min_age_days=0, + min_score=4, + repo=None, + optout_labels=set(), + ) + assert action == "close" + assert score == 1 + + def test_should_close_immediately_for_swiftwinds_login_case_insensitive( + self, closer_module, _now, monkeypatch + ): + # GitHub login matching is case-insensitive on GitHub's side; the + # API returns the original casing. Make sure the bypass fires + # regardless of how the login was registered. + monkeypatch.setattr( + closer_module, + "fetch_pr_comments", + lambda *a, **kw: [_greptile_comment("Confidence Score: 1/5")], + ) + for login in ("SwiftWinds", "swiftwinds", "SWIFTWINDS"): + action, _, _ = closer_module.evaluate_pr( + self._make_pr(created_days_ago=0, author_login=login), + now=_now, + min_age_days=0, + min_score=4, + repo=None, + optout_labels=set(), + ) + assert action == "close", login + def test_should_skip_internal_authors(self, closer_module, _now, monkeypatch): # Override the fixture for this one test. monkeypatch.setattr( @@ -422,6 +528,120 @@ class TestMainOptoutLabelDefault: assert default not in captured["optout_labels"], default +class TestSecondsSinceLastGraceWarning: + """Grace-period detection: only counts comments by the bot identity + that contain the shared `GRACE_COMMENT_MARKER`.""" + + def _make_marker_comment( + self, + closer_module, + *, + login: str = "github-actions[bot]", + created_at: str = "2026-05-16T00:00:00Z", + include_marker: bool = True, + ) -> dict: + body = "warning text" + if include_marker: + body += "\n\n" + closer_module.GRACE_COMMENT_MARKER + return { + "user": {"login": login}, + "body": body, + "created_at": created_at, + } + + def test_should_return_none_when_no_marker_comment(self, closer_module): + comments = [ + { + "user": {"login": "github-actions[bot]"}, + "body": "Some other bot comment", + "created_at": "2026-05-16T00:00:00Z", + } + ] + assert closer_module.seconds_since_last_grace_warning(comments) is None + + def test_should_return_none_for_empty(self, closer_module): + assert closer_module.seconds_since_last_grace_warning([]) is None + + def test_should_ignore_non_bot_comments_with_marker(self, closer_module): + # If a curious user quotes the marker in a comment, we must NOT + # treat it as a bot warning. The grace timer would then never fire. + comments = [ + self._make_marker_comment(closer_module, login="random-user"), + ] + assert closer_module.seconds_since_last_grace_warning(comments) is None + + def test_should_pick_latest_marker_comment(self, closer_module): + # When multiple grace warnings exist (e.g. a re-open cycle), use + # the most recent one to compute the age. + comments = [ + self._make_marker_comment(closer_module, created_at="2026-05-15T00:00:00Z"), + self._make_marker_comment(closer_module, created_at="2026-05-16T23:00:00Z"), + ] + now = dt.datetime(2026, 5, 17, 0, 0, 0, tzinfo=dt.timezone.utc) + age = closer_module.seconds_since_last_grace_warning(comments, now=now) + # 1h = 3600s + assert age == 3600.0 + + +class TestImmediateCloseLoginsConstant: + """SwiftWinds is the dogfood account the user explicitly named — pin + its presence so a future cleanup that removes the constant or + forgets to keep the entry doesn't silently break the test path.""" + + def test_should_include_swiftwinds(self, closer_module): + assert "swiftwinds" in closer_module.IMMEDIATE_CLOSE_LOGINS + + def test_should_be_lowercase_for_case_insensitive_match(self, closer_module): + for login in closer_module.IMMEDIATE_CLOSE_LOGINS: + assert login == login.lower(), login + + +class TestGraceWarningCommentText: + """Pin the user-facing language in the grace warning comment so the + `1 day grace` and `@greptileai still works after close` promises + don't get accidentally dropped in a future refactor. + """ + + def test_should_state_one_day_grace_period(self, closer_module): + body = closer_module.format_grace_warning_comment(score=2, threshold=4) + # The user's PR explicitly said "specify in the comment" — pin + # that the literal "1 day" appears in the comment. + assert "1 day" in body + + def test_should_mention_agent_shin_reconsider(self, closer_module): + body = closer_module.format_grace_warning_comment(score=2, threshold=4) + assert "@agent-shin reconsider" in body + + def test_should_promise_greptileai_works_after_close(self, closer_module): + body = closer_module.format_grace_warning_comment(score=2, threshold=4) + assert "@greptileai" in body + assert "even after the PR is closed" in body + + def test_should_carry_grace_marker(self, closer_module): + # The marker is what `seconds_since_last_grace_warning` greps for + # to detect a prior warning — dropping it would silently break + # the cooldown. + body = closer_module.format_grace_warning_comment(score=2, threshold=4) + assert closer_module.GRACE_COMMENT_MARKER in body + + def test_close_comment_should_mention_greptileai_post_close(self, closer_module): + # The actual close comment should ALSO point at the @greptileai + # post-close re-review path so contributors see the same options + # whether they read the warning or only catch the close comment. + # `close_pr` writes the close comment via `gh pr comment` — we + # don't easily call it directly here, but the comment body is + # constructed inline. Re-creating it via a no-op `gh` stub is + # awkward, so we assert against the same string template by + # asserting that the close path's text constant is updated. + # `close_pr` source must contain the marker text — guarded by + # this whole-module read-and-assert. + from pathlib import Path + + source = Path(closer_module.__file__).read_text() + assert "even after the PR is closed" in source + assert "@greptileai" in source + + class TestHasOptoutLabel: def test_should_match_label_case_insensitively(self, closer_module): pr = {"labels": [{"name": "Do Not Close"}, {"name": "bug"}]} diff --git a/tests/test_litellm/test_github_triage_with_llm.py b/tests/test_litellm/test_github_triage_with_llm.py index a8ecd0fbea0..020de81c391 100644 --- a/tests/test_litellm/test_github_triage_with_llm.py +++ b/tests/test_litellm/test_github_triage_with_llm.py @@ -619,9 +619,12 @@ class TestTriageOrchestration: self, triage_module, monkeypatch ): # "See #1234" is a passing mention, not a closing keyword. The LLM - # must get a chance to apply the stricter rubric. + # must get a chance to apply the stricter rubric. With no prior + # grace warning, the first failing verdict triggers the warning + # path (`would-warn-grace` in dry-run). pr = self._make_pr(body="See #1234 for context. No QA proof here.") monkeypatch.setattr(triage_module, "fetch_pr", lambda repo, n: pr) + self._stub_grace_no_warning(triage_module, monkeypatch) called = {"judge": False} def judge(prompt): @@ -639,7 +642,7 @@ class TestTriageOrchestration: judge=judge, ) assert called["judge"] is True - assert result["action"] == "would-close" + assert result["action"] == "would-warn-grace" def test_should_return_pass_llm_when_judge_passes(self, triage_module, monkeypatch): pr = self._make_pr(body="Long body, no linked issue.") @@ -656,9 +659,16 @@ class TestTriageOrchestration: assert result["action"] == "pass-llm" assert "Long body" in captured["prompt"] - def test_should_return_would_close_in_dry_run(self, triage_module, monkeypatch): + def test_should_return_would_close_in_dry_run_after_grace_aged_out( + self, triage_module, monkeypatch + ): + # When the grace warning has already aged out (>= GRACE_PERIOD_SECONDS) + # AND the rubric still fails, the dry-run preview returns + # `would-close` so a step-summary writer can render the close + # comment without touching GitHub state. pr = self._make_pr(body="just a sentence.") monkeypatch.setattr(triage_module, "fetch_pr", lambda repo, n: pr) + self._stub_grace_aged_out(triage_module, monkeypatch) def fake_post(*a, **kw): pytest.fail("should not post comments in dry-run") @@ -685,11 +695,15 @@ class TestTriageOrchestration: assert result["action"] == "would-close" assert result["verdict"]["missing"] == ["problem description", "QA proof"] - def test_should_post_comment_and_close_when_close_enabled( + def test_should_post_comment_and_close_after_grace_window( self, triage_module, monkeypatch ): + # The "real close" path: --close passed AND the grace warning has + # aged out AND the rubric still fails. The bot posts the close + # comment and closes the PR. pr = self._make_pr(body="just a sentence.") monkeypatch.setattr(triage_module, "fetch_pr", lambda repo, n: pr) + self._stub_grace_aged_out(triage_module, monkeypatch) posted = {} closed = {} monkeypatch.setattr( @@ -784,6 +798,30 @@ class TestTriageOrchestration: lambda *a, **kw: None, ) + @staticmethod + def _stub_grace_aged_out(triage_module, monkeypatch): + """Pretend the grace warning has aged out. + + For tests that exercise the post-grace close path. Set the age + to twice the grace window so a future tweak to + `GRACE_PERIOD_SECONDS` doesn't accidentally make the stub fall + back inside the window. + """ + monkeypatch.setattr( + triage_module, + "seconds_since_last_grace_warning", + lambda *a, **kw: triage_module.GRACE_PERIOD_SECONDS * 2, + ) + + @staticmethod + def _stub_grace_no_warning(triage_module, monkeypatch): + """Pretend no grace warning has been posted yet (first detection).""" + monkeypatch.setattr( + triage_module, + "seconds_since_last_grace_warning", + lambda *a, **kw: None, + ) + def test_should_reopen_on_reconsider_pass(self, triage_module, monkeypatch): # Reconsider on a closed PR with a passing verdict -> reopen + post a # friendly "re-evaluated" comment. close=True is the production path @@ -1214,6 +1252,9 @@ class TestTriageOrchestration: "user": {"login": "outside"}, } monkeypatch.setattr(triage_module, "fetch_issue", lambda repo, n: issue) + # Grace already aged out -> close path. (Issues use the same + # GRACE_COMMENT_MARKER detection as PRs.) + self._stub_grace_aged_out(triage_module, monkeypatch) closed = {} posted = {} monkeypatch.setattr( @@ -1243,3 +1284,432 @@ class TestTriageOrchestration: assert result["action"] == "closed" assert closed["n"] == 7 assert "reproduction" in posted["body"] + + # ---- Grace-period flow ------------------------------------------------ + + def test_should_post_grace_warning_on_first_failing_run_in_close_mode( + self, triage_module, monkeypatch + ): + # First low-quality detection -> bot posts a warning comment with + # the GRACE_COMMENT_MARKER. The PR must NOT be closed yet. + pr = self._make_pr(body="just a sentence.") + monkeypatch.setattr(triage_module, "fetch_pr", lambda repo, n: pr) + self._stub_grace_no_warning(triage_module, monkeypatch) + posted = {} + monkeypatch.setattr( + triage_module, + "post_comment", + lambda repo, n, body: posted.update({"n": n, "body": body}), + ) + monkeypatch.setattr( + triage_module, + "close_pr", + lambda *a, **kw: pytest.fail("must not close on first detection"), + ) + + verdict = { + "verdict": "fail", + "missing": ["QA proof"], + "explanation": "Body too thin.", + } + result = triage_module.triage( + repo="o/r", + kind="pr", + number=42, + close=True, + model="m", + judge=lambda p: json.dumps(verdict), + ) + assert result["action"] == "warned-grace" + assert posted["n"] == 42 + # Pin the user-facing language pieces the user explicitly asked for. + assert "1 day" in posted["body"] + assert "@agent-shin reconsider" in posted["body"] + assert "@greptileai" in posted["body"] + assert "even after the PR is closed" in posted["body"] + assert triage_module.GRACE_COMMENT_MARKER in posted["body"] + + def test_should_skip_close_inside_grace_window(self, triage_module, monkeypatch): + # A warning was posted recently; do nothing on this run regardless + # of close=True. The next run after `GRACE_PERIOD_SECONDS` elapses + # is the one that flips to actual close. + pr = self._make_pr(body="just a sentence.") + monkeypatch.setattr(triage_module, "fetch_pr", lambda repo, n: pr) + monkeypatch.setattr( + triage_module, + "seconds_since_last_grace_warning", + lambda *a, **kw: 60.0, + ) + monkeypatch.setattr( + triage_module, + "post_comment", + lambda *a, **kw: pytest.fail("must not comment during grace window"), + ) + monkeypatch.setattr( + triage_module, + "close_pr", + lambda *a, **kw: pytest.fail("must not close during grace window"), + ) + + verdict = { + "verdict": "fail", + "missing": ["QA proof"], + "explanation": "Body too thin.", + } + result = triage_module.triage( + repo="o/r", + kind="pr", + number=42, + close=True, + model="m", + judge=lambda p: json.dumps(verdict), + ) + assert result["action"] == "skip-in-grace-period" + assert result["grace_age_seconds"] == 60.0 + assert result["grace_period_seconds"] == triage_module.GRACE_PERIOD_SECONDS + + def test_should_dry_run_grace_warning_when_close_false( + self, triage_module, monkeypatch + ): + # In dry-run mode the FIRST failing detection returns + # `would-warn-grace` (with the previewed comment body) and never + # touches GitHub state. Lets a local operator preview the + # warning before flipping --close on. + pr = self._make_pr(body="thin") + monkeypatch.setattr(triage_module, "fetch_pr", lambda repo, n: pr) + self._stub_grace_no_warning(triage_module, monkeypatch) + monkeypatch.setattr( + triage_module, + "post_comment", + lambda *a, **kw: pytest.fail("must not post in dry-run grace warn"), + ) + + verdict = { + "verdict": "fail", + "missing": ["QA proof"], + "explanation": "thin", + } + result = triage_module.triage( + repo="o/r", + kind="pr", + number=1, + close=False, + model="m", + judge=lambda p: json.dumps(verdict), + ) + assert result["action"] == "would-warn-grace" + assert "1 day" in result["comment"] + + # ---- IMMEDIATE_CLOSE_LOGINS bypass (e.g. SwiftWinds) ----------------- + + def test_should_skip_grace_for_swiftwinds_login(self, triage_module, monkeypatch): + # SwiftWinds is in `IMMEDIATE_CLOSE_LOGINS` for dogfooding. + # Even though no grace warning has been posted, the bypass must + # take the close path immediately. + pr = self._make_pr(body="just a sentence.", user={"login": "SwiftWinds"}) + monkeypatch.setattr(triage_module, "fetch_pr", lambda repo, n: pr) + # The grace check must NOT be consulted at all for an immediate + # login — pin that here. + monkeypatch.setattr( + triage_module, + "seconds_since_last_grace_warning", + lambda *a, **kw: pytest.fail( + "grace check must not run for immediate-close login" + ), + ) + posted = {} + closed = {} + monkeypatch.setattr( + triage_module, + "post_comment", + lambda repo, n, body: posted.update({"n": n, "body": body}), + ) + monkeypatch.setattr( + triage_module, + "close_pr", + lambda repo, n: closed.update({"n": n}), + ) + + verdict = { + "verdict": "fail", + "missing": ["QA proof"], + "explanation": "Body too thin.", + } + result = triage_module.triage( + repo="o/r", + kind="pr", + number=99, + close=True, + model="m", + judge=lambda p: json.dumps(verdict), + ) + assert result["action"] == "closed" + assert result["immediate_close"] is True + assert closed["n"] == 99 + # The close comment is the standard one (no warning was needed). + assert "Agent Shin" in posted["body"] + + def test_should_close_swiftwinds_even_when_close_flag_false( + self, triage_module, monkeypatch + ): + # The `IMMEDIATE_CLOSE_LOGINS` bypass intentionally overrides the + # workflow-level dry-run gating. The user explicitly asked to + # "turn on full (non dry run) mode with SwiftWinds" because their + # personal account has no push permissions to litellm and is the + # perfect testing target. With `close=False` (workflow stripped + # --close on pull_request_target), the script must STILL post + + # close for SwiftWinds. + pr = self._make_pr(body="just a sentence.", user={"login": "SwiftWinds"}) + monkeypatch.setattr(triage_module, "fetch_pr", lambda repo, n: pr) + posted = {} + closed = {} + monkeypatch.setattr( + triage_module, + "post_comment", + lambda repo, n, body: posted.update({"n": n, "body": body}), + ) + monkeypatch.setattr( + triage_module, + "close_pr", + lambda repo, n: closed.update({"n": n}), + ) + + verdict = { + "verdict": "fail", + "missing": ["QA proof"], + "explanation": "thin", + } + result = triage_module.triage( + repo="o/r", + kind="pr", + number=99, + close=False, + model="m", + judge=lambda p: json.dumps(verdict), + ) + assert result["action"] == "closed" + assert closed["n"] == 99 + + def test_should_match_immediate_close_login_case_insensitively( + self, triage_module, monkeypatch + ): + # GitHub returns the original casing of a login. The bypass must + # work regardless of the exact case the API returns. + for login in ("SwiftWinds", "swiftwinds", "SWIFTWINDS"): + pr = self._make_pr(body="thin", user={"login": login}) + monkeypatch.setattr(triage_module, "fetch_pr", lambda repo, n: pr) + monkeypatch.setattr( + triage_module, + "post_comment", + lambda *a, **kw: None, + ) + monkeypatch.setattr( + triage_module, + "close_pr", + lambda *a, **kw: None, + ) + verdict = { + "verdict": "fail", + "missing": ["QA proof"], + "explanation": "thin", + } + result = triage_module.triage( + repo="o/r", + kind="pr", + number=1, + close=False, + model="m", + judge=lambda p: json.dumps(verdict), + ) + assert result["action"] == "closed", login + assert result.get("immediate_close") is True, login + + def test_should_not_treat_random_external_login_as_immediate_close( + self, triage_module, monkeypatch + ): + # Sanity check — the bypass must NOT fire for any login other + # than the explicitly-listed test accounts. + pr = self._make_pr(body="thin", user={"login": "random-oss-dev"}) + monkeypatch.setattr(triage_module, "fetch_pr", lambda repo, n: pr) + self._stub_grace_no_warning(triage_module, monkeypatch) + posted = {} + monkeypatch.setattr( + triage_module, + "post_comment", + lambda repo, n, body: posted.update({"body": body}), + ) + monkeypatch.setattr( + triage_module, + "close_pr", + lambda *a, **kw: pytest.fail("must not close non-immediate login"), + ) + + verdict = { + "verdict": "fail", + "missing": ["QA proof"], + "explanation": "thin", + } + result = triage_module.triage( + repo="o/r", + kind="pr", + number=1, + close=True, + model="m", + judge=lambda p: json.dumps(verdict), + ) + assert result["action"] == "warned-grace" + + +class TestImmediateCloseLoginsConstant: + """SwiftWinds is the dogfood account the user explicitly named — pin + its presence so a future cleanup that removes the constant or + forgets to keep the entry doesn't silently break the test path.""" + + def test_should_include_swiftwinds(self, triage_module): + assert "swiftwinds" in triage_module.IMMEDIATE_CLOSE_LOGINS + + def test_should_be_lowercase_for_case_insensitive_match(self, triage_module): + for login in triage_module.IMMEDIATE_CLOSE_LOGINS: + assert login == login.lower(), login + + +class TestGraceWarningCommentText: + """Pin the user-facing promises in the grace warning so a future + refactor can't silently drop them.""" + + def test_pr_grace_warning_should_state_one_day_grace(self, triage_module): + body = triage_module.format_grace_warning_pr_comment( + {"verdict": "fail", "missing": ["QA proof"], "explanation": "thin"} + ) + # The user explicitly asked: "specify in the comment" that there + # is a 1-day grace. + assert "1 day" in body + + def test_pr_grace_warning_should_mention_reconsider_during_grace( + self, triage_module + ): + body = triage_module.format_grace_warning_pr_comment( + {"verdict": "fail", "missing": [], "explanation": ""} + ) + assert "@agent-shin reconsider" in body + + def test_pr_grace_warning_should_promise_greptileai_works_post_close( + self, triage_module + ): + body = triage_module.format_grace_warning_pr_comment( + {"verdict": "fail", "missing": [], "explanation": ""} + ) + # Per user: comment should state @greptileai works even after close. + assert "@greptileai" in body + assert "even after the PR is closed" in body + + def test_pr_grace_warning_should_carry_grace_marker(self, triage_module): + # The marker is what `seconds_since_last_grace_warning` greps for + # on subsequent runs to detect that a warning has been posted. + # Dropping it would silently break the close-after-grace path. + body = triage_module.format_grace_warning_pr_comment( + {"verdict": "fail", "missing": [], "explanation": ""} + ) + assert triage_module.GRACE_COMMENT_MARKER in body + + def test_issue_grace_warning_should_carry_grace_marker(self, triage_module): + body = triage_module.format_grace_warning_issue_comment( + {"verdict": "fail", "missing": [], "explanation": ""} + ) + assert triage_module.GRACE_COMMENT_MARKER in body + assert "1 day" in body + assert "@agent-shin reconsider" in body + + def test_pr_close_comment_should_promise_greptileai_works_post_close( + self, triage_module + ): + # The standard close comment must ALSO point at @greptileai so + # contributors see the same options whether they read the warning + # or only catch the close comment. + body = triage_module.format_pr_close_comment( + {"verdict": "fail", "missing": [], "explanation": ""} + ) + assert "@greptileai" in body + assert "even after the PR is closed" in body + + +class TestSecondsSinceLastGraceWarning: + """Mirror of TestSecondsSinceLastReconsiderVerdict for the new helper. + Both helpers share `_seconds_since_latest_marker_comment` underneath + so the parsing logic is exercised either way; these tests pin the + grace-marker-specific behavior.""" + + def _make_comment( + self, + *, + login: str, + body: str, + created_at: str | None = "2026-05-18T05:00:00Z", + ) -> dict: + comment: dict = {"user": {"login": login}, "body": body} + if created_at is not None: + comment["created_at"] = created_at + return comment + + def test_should_return_none_when_no_grace_marker(self, triage_module, monkeypatch): + comments = [ + self._make_comment( + login="github-actions[bot]", + body="Some other bot message", + ), + self._make_comment(login="random-user", body="ping?"), + ] + monkeypatch.setattr( + triage_module, "_iter_paginated_json", lambda *a, **kw: iter(comments) + ) + assert triage_module.seconds_since_last_grace_warning("o/r", 1) is None + + def test_should_ignore_non_bot_comments_with_marker( + self, triage_module, monkeypatch + ): + # A user who quotes the marker in a question must NOT be treated + # as the bot warning; otherwise the close-after-grace path would + # never fire because the timer keeps resetting. + comments = [ + self._make_comment( + login="random-user", + body=f"What is {triage_module.GRACE_COMMENT_MARKER}?", + ) + ] + monkeypatch.setattr( + triage_module, "_iter_paginated_json", lambda *a, **kw: iter(comments) + ) + assert triage_module.seconds_since_last_grace_warning("o/r", 1) is None + + def test_should_pick_latest_grace_marker(self, triage_module, monkeypatch): + comments = [ + self._make_comment( + login="github-actions[bot]", + body="old warning " + triage_module.GRACE_COMMENT_MARKER, + created_at="2026-05-18T03:00:00Z", + ), + self._make_comment( + login="github-actions[bot]", + body="newer warning " + triage_module.GRACE_COMMENT_MARKER, + created_at="2026-05-18T04:55:00Z", + ), + ] + monkeypatch.setattr( + triage_module, "_iter_paginated_json", lambda *a, **kw: iter(comments) + ) + + import datetime as real_dt + + class FrozenDateTime(real_dt.datetime): + @classmethod + def now(cls, tz=None): + return real_dt.datetime(2026, 5, 18, 5, 0, 0, tzinfo=tz) + + frozen_module = type(triage_module.dt)("datetime") + frozen_module.datetime = FrozenDateTime + frozen_module.timezone = real_dt.timezone + monkeypatch.setattr(triage_module, "dt", frozen_module) + + age = triage_module.seconds_since_last_grace_warning("o/r", 1) + # Newer warning is 5 minutes (300s) before "now". + assert age == 300.0 From e0eeb735541c2548bc4d4c7bded9ab0f8de3bffe Mon Sep 17 00:00:00 2001 From: Cursor Agent Date: Tue, 19 May 2026 07:46:38 +0000 Subject: [PATCH 16/17] fix: skip grace-period text in close comment for IMMEDIATE_CLOSE_LOGINS For PRs from IMMEDIATE_CLOSE_LOGINS (e.g. swiftwinds), evaluate_pr returns 'close' immediately without ever posting a grace warning, so the close comment should not reference a 1-day grace period. Make close_pr take a grace_period_elapsed flag, default True, and pass False from the main loop when the close path was the immediate-close branch. Co-authored-by: Yassin Kortam --- .github/scripts/close_low_quality_prs.py | 14 ++++++++++++-- 1 file changed, 12 insertions(+), 2 deletions(-) diff --git a/.github/scripts/close_low_quality_prs.py b/.github/scripts/close_low_quality_prs.py index 85b281e959d..c3416e48a9b 100644 --- a/.github/scripts/close_low_quality_prs.py +++ b/.github/scripts/close_low_quality_prs.py @@ -350,6 +350,7 @@ def close_pr( repo: str | None, dry_run: bool, label: str | None, + grace_period_elapsed: bool = True, ) -> None: """Post the explanatory comment and close the PR.""" pr_number = pr["number"] @@ -362,11 +363,19 @@ def close_pr( ) return - comment_body = ( - f"Closing as part of automated PR triage.\n\n" + score_sentence = ( f"Greptile's most recent review scored this PR **{score}/5**, below " f"our merge bar of **{threshold}/5**, and the 1-day grace period since " "the warning has elapsed.\n\n" + if grace_period_elapsed + else ( + f"Greptile's most recent review scored this PR **{score}/5**, " + f"below our merge bar of **{threshold}/5**.\n\n" + ) + ) + comment_body = ( + f"Closing as part of automated PR triage.\n\n" + f"{score_sentence}" "We close low-confidence PRs aggressively to keep the review queue " "manageable for maintainers and contributors alike. **This is not a " "rejection of the idea** — to bring this back:\n\n" @@ -602,6 +611,7 @@ def main() -> int: repo=args.repo, dry_run=pr_dry_run, label=args.close_label, + grace_period_elapsed=not is_immediate, ) if not pr_dry_run: From b0fc8c224f6bfddafd377346b2e89451b47fa8d9 Mon Sep 17 00:00:00 2001 From: Cursor Agent Date: Tue, 19 May 2026 07:59:22 +0000 Subject: [PATCH 17/17] fix(close-low-quality-prs): report actual closes in dry-run summary IMMEDIATE_CLOSE_LOGINS PRs are closed even when the global --close flag is not set, but the summary used the global dry-run flag to choose between 'would close' and 'closed'. Split the count so operators can see both actual closures and dry-run would-be closures. Co-authored-by: Yassin Kortam --- .github/scripts/close_low_quality_prs.py | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/.github/scripts/close_low_quality_prs.py b/.github/scripts/close_low_quality_prs.py index c3416e48a9b..746807f4a58 100644 --- a/.github/scripts/close_low_quality_prs.py +++ b/.github/scripts/close_low_quality_prs.py @@ -623,7 +623,17 @@ def main() -> int: print("\n=== Summary ===") for key, value in summary.items(): print(f" {key:28s} {value}") - print(f"\nTotal {'would close' if dry_run else 'closed'}: {summary['close']}") + # `IMMEDIATE_CLOSE_LOGINS` PRs are closed even in global dry-run mode, so + # report actual closures alongside the dry-run "would close" count to avoid + # misleading operators into thinking no writes occurred. + would_close = summary["close"] - closed + if dry_run: + if closed: + print(f"\nTotal closed: {closed}; would close: {would_close}") + else: + print(f"\nTotal would close: {would_close}") + else: + print(f"\nTotal closed: {closed}") print( f"Total {'would warn (grace)' if dry_run else 'warned (grace)'}: " f"{summary['warn-grace']}"