diff --git a/.github/ISSUE_TEMPLATE/bug_report.yml b/.github/ISSUE_TEMPLATE/bug_report.yml index bbe4b76775da..1938aab5007d 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 4cc429018977..9844032a98b0 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 f9ce9e5dcb8a..a9c41d4b231e 100644 --- a/.github/pull_request_template.md +++ b/.github/pull_request_template.md @@ -1,10 +1,66 @@ + + ## Relevant issues - + ## Linear ticket - + + +## Problem description + + + +## Expected vs. actual behavior + + + +## QA proof + + ## Pre-Submission checklist @@ -13,7 +69,7 @@ - [ ] I have Added testing in the [`tests/test_litellm/`](https://github.com/BerriAI/litellm/tree/main/tests/test_litellm) directory, **Adding at least 1 test is a hard requirement** - [see details](https://docs.litellm.ai/docs/extras/contributing_code) - [ ] My PR passes all unit tests on [`make test-unit`](https://docs.litellm.ai/docs/extras/contributing_code) - [ ] My PR's scope is as isolated as possible, it only solves 1 specific problem -- [ ] I have requested a Greptile review by commenting `@greptileai` and received a **Confidence Score of at least 4/5** before requesting a maintainer review +- [ ] I have received a Greptile **Confidence Score of at least 4/5** before requesting a maintainer review (Greptile reviews automatically on open; comment `@greptileai` to re-trigger after pushing fixes) ## Delays in PR merge? @@ -36,13 +92,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 diff --git a/.github/scripts/close_low_quality_prs.py b/.github/scripts/close_low_quality_prs.py new file mode 100644 index 000000000000..a71ad6fd950c --- /dev/null +++ b/.github/scripts/close_low_quality_prs.py @@ -0,0 +1,430 @@ +#!/usr/bin/env python3 +""" +Auto-close low-quality pull requests. + +Closes open PRs (including drafts, regardless of age) that satisfy ALL of: + 1. Have a Greptile (`greptile-apps`) review comment whose latest + "Confidence Score: X/5" is below the configured threshold (default: 4). + 2. Are authored by an external OSS contributor (internal BerriAI + contributors are exempt). + 3. Do not carry an opt-out label (default: "do not close"). + +`--min-age-days` is retained as an opt-in safety net for one-off backfill +runs (default: 0). The team's intent is that the count of open PRs equals +the count of PRs internal collaborators need to action on, so neither age +nor draft status acts as a free pass. + +For each match, the script posts an explanatory comment and closes the PR. +Because OSS contributors *cannot* reopen a PR closed by the bot/maintainer +(GitHub limitation), the close-comment instructs them to push their fixes +and **open a fresh PR**, or to comment `@agent-shin reconsider` on the +closed PR to have the LLM judge re-evaluate (and reopen on pass). + +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 + + # Restrict to PRs at least N days old (one-off backfill safety net) + 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 subprocess +import sys +from pathlib import Path + +# Share constants with the sibling Agent Shin script instead of duplicating +# them. `AGENT_SHIN_AUTO_CLOSE_MARKER` is the literal phrase the reconsider +# provenance check keys off, and `INTERNAL_ASSOCIATIONS` is the exempt-author +# set; drift between the two files would silently break reconsider for +# Greptile-closed PRs (marker) or let one script close a PR the other would +# skip (associations), with no test catching it. +_SCRIPTS_DIR = Path(__file__).resolve().parent +if str(_SCRIPTS_DIR) not in sys.path: + sys.path.insert(0, str(_SCRIPTS_DIR)) +# `extract_greptile_score`, `GREPTILE_BOT_LOGINS`, `SCORE_PATTERN`, and +# `parse_iso8601` now live in triage_with_llm so the daily sweep and the +# review gate read the Greptile score through one implementation (drift would +# silently let one path act on a PR the other would spare). +from triage_with_llm import ( # noqa: E402 + AGENT_SHIN_AUTO_CLOSE_MARKER, + GREPTILE_BOT_LOGINS, + INTERNAL_ASSOCIATIONS, + SCORE_PATTERN, + extract_greptile_score, + parse_iso8601, +) + +# Re-exported above for any caller that imports them from this module. +__all__ = ["GREPTILE_BOT_LOGINS", "SCORE_PATTERN", "extract_greptile_score"] + +# 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.""" + result = subprocess.run( + ["gh", *args], + capture_output=True, + text=True, + check=True, + ) + return result.stdout + + +# `gh pr list --limit` caps at 1000 (the CLI's documented hard ceiling). +# Surface a warning if we ever hit that cap so the silent truncation is +# visible in workflow logs instead of just being a missed close. +GH_PR_LIST_LIMIT = 1000 + + +def fetch_open_prs(repo: str | None) -> list[dict]: + """Fetch all open PRs (number, createdAt, isDraft, labels, author). + + Includes drafts: `gh pr list --state open` returns both ready-for-review + and draft PRs by default. This is the desired behavior β€” drafts are not + a free pass; the internal-collaborator open-PR queue should reflect every + PR that needs human attention regardless of draft status. + """ + repo_args = ["--repo", repo] if repo else [] + fields = "number,title,createdAt,isDraft,labels,author,url" + raw = gh( + "pr", + "list", + "--state", + "open", + "--limit", + str(GH_PR_LIST_LIMIT), + "--json", + fields, + *repo_args, + ) + prs = json.loads(raw) + if len(prs) >= GH_PR_LIST_LIMIT: + # `gh pr list --limit N` returns at most N rows even if more exist; + # log a GitHub Actions warning so the truncation isn't silent. + message = ( + f"fetch_open_prs hit the gh CLI cap ({GH_PR_LIST_LIMIT}); " + "the open-PR list is likely truncated. Switch to paginated " + "`gh api` calls if the repo regularly exceeds this cap." + ) + print(f"::warning::{message}", file=sys.stderr) + return prs + + +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) + # 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_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). + + Returns [] on API failure so a transient hiccup on any single PR doesn't + abort the whole daily sweep mid-loop. Matches the fail-safe pattern in + `fetch_pr_author_association`; downstream the empty list becomes a + `skip-no-greptile-score` action and the PR is re-evaluated on the next run. + """ + 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" + ) + try: + raw = gh("api", "--paginate", endpoint) + except subprocess.CalledProcessError: + return [] + comments: list[dict] = [] + for line in raw.strip().splitlines(): + line = line.strip() + if not line: + continue + try: + parsed = json.loads(line) + except json.JSONDecodeError: + return [] + if isinstance(parsed, list): + comments.extend(parsed) + else: + comments.append(parsed) + return comments + + +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"πŸ‘‹ Hi, thanks for the PR! {AGENT_SHIN_AUTO_CLOSE_MARKER}, the automated triage " + "bot for this repository. 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" + "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. Push the fixes that address Greptile's feedback (continue using " + "your existing branch is fine).\n" + "2. **Open a new PR** with the updated branch. Greptile will review " + "it again, and if it scores " + f"**{threshold}/5 or higher** a maintainer will take another look.\n\n" + "_Why open a new PR instead of reopening this one?_ GitHub does not " + "let external contributors reopen a PR that was closed by a bot or " + "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" + "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-too-young", "skip-optout-label", "skip-internal", + "skip-no-greptile-score", "skip-score-ok", 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. + """ + if has_optout_label(pr, optout_labels): + return ("skip-optout-label", None, None) + + created = parse_iso8601(pr["createdAt"]) + age_days = (now - created).days + # `min_age_days` defaults to 0 (close as soon as Greptile scores low). + # Set a positive value via --min-age-days for one-off backfill runs that + # want to skip very-young PRs. + if min_age_days > 0 and 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: + 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=0, + help=( + "Minimum age (in days) before a PR is eligible. Default 0 = " + "close as soon as Greptile flags it. Set a positive value for " + "one-off backfill runs that want to spare very-young PRs." + ), + ) + 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=None, + help=( + "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( + "--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). " + "Applied in dry-run too so `--limit N` previews exactly the " + "first N closures." + ), + ) + 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 or DEFAULT_OPTOUT_LABELS) + + closed = 0 + summary = { + "close": 0, + "skip-too-young": 0, + "skip-optout-label": 0, + "skip-internal": 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, + ) + + closed += 1 + if args.limit is not None and closed >= args.limit: + verb = "Would close" if dry_run else "Closed" + print(f"\nReached --limit={args.limit} ({verb} count); 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/scripts/triage_with_llm.py b/.github/scripts/triage_with_llm.py new file mode 100644 index 000000000000..433ef0c0dc57 --- /dev/null +++ b/.github/scripts/triage_with_llm.py @@ -0,0 +1,1352 @@ +#!/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-5.4-mini) +""" + +from __future__ import annotations + +import argparse +import datetime as dt +import json +import os +import re +import subprocess +import sys +import textwrap +import urllib.parse +from typing import Any, Iterable + +DEFAULT_MODEL = "gpt-5.4-mini" + +INTERNAL_ASSOCIATIONS = frozenset({"OWNER", "MEMBER", "COLLABORATOR"}) + +# --- Review-gate ("ready for review" label lifecycle) configuration ---------- +# The review gate keeps a single label in sync with whether a PR currently +# clears BOTH quality bars: the LLM rubric (clear problem + expected/actual + +# QA proof, or a linked issue) AND Greptile's most recent confidence score. +READY_FOR_REVIEW_LABEL = "ready for review" +DEFAULT_GRACE_DAYS = 1 # 24h before an un-passing, un-tagged PR is auto-closed +DEFAULT_MIN_GREPTILE_SCORE = 4 # Greptile < 4/5 counts as "not passing" + +# Hidden HTML-comment markers stamped into review-gate comments. They never +# render in the GitHub UI but let the gate detect its own prior actions so it +# (a) posts the within-grace "what's missing" notice at most once and (b) can +# tell a first-time pass ("ready for review") from a recovery after a +# regression ("all clear again"). They deliberately do NOT contain +# AGENT_SHIN_AUTO_CLOSE_MARKER, so review-gate chatter on an open PR never +# trips the reconsider provenance check (which keys off the close marker). +READY_MARKER = "" +REGRESSED_MARKER = "" +WITHIN_GRACE_MARKER = "" + +# 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. These +# live here (rather than in close_low_quality_prs.py) so both the daily sweep +# and the review gate read the score through one implementation β€” drift would +# silently let one path close/label a PR the other would spare. +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, +) + +# Marker phrase Agent Shin always includes in its auto-close comments +# (see `format_pr_close_comment` / `format_issue_close_comment`). The +# provenance check for reconsider matches this marker against a comment +# authored by the same bot login that performed the most recent `closed` +# event, so a contributor cannot reopen a PR/issue that a maintainer +# closed after a prior Agent Shin auto-close. Keep the marker in sync +# with the literal text in those formatter functions. +AGENT_SHIN_AUTO_CLOSE_MARKER = "I'm **Agent Shin**" + +# 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. +# +# 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|fixed|closes|close|closed|resolves|resolve|resolved)\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 reopen_pr(repo: str, number: int) -> None: + """Reopen a previously-closed pull request (state=open). + + Used by the `@agent-shin reconsider` comment-trigger flow: the bot has + write access via GH_TOKEN, so it can reopen on the contributor's behalf + even though GitHub doesn't let the OSS author do it themselves. + """ + gh( + "api", + f"repos/{repo}/pulls/{number}", + "-X", + "PATCH", + "-f", + "state=open", + ) + + +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) + + +def reopen_issue(repo: str, number: int) -> None: + """Reopen a previously-closed issue (state=open, state_reason=reopened).""" + gh( + "api", + f"repos/{repo}/issues/{number}", + "-X", + "PATCH", + "-f", + "state=open", + "-f", + "state_reason=reopened", + ) + + +def add_label(repo: str, number: int, label: str) -> None: + """Add a label to a PR/issue (GitHub creates the label if it's missing).""" + gh( + "api", + f"repos/{repo}/issues/{number}/labels", + "-X", + "POST", + "-f", + f"labels[]={label}", + ) + + +def remove_label(repo: str, number: int, label: str) -> None: + """Remove a label from a PR/issue. A missing label (404) is not an error.""" + encoded = urllib.parse.quote(label, safe="") + try: + gh( + "api", + f"repos/{repo}/issues/{number}/labels/{encoded}", + "-X", + "DELETE", + ) + except subprocess.CalledProcessError as exc: + stderr = (exc.stderr or "").lower() + if "404" in stderr or "not found" in stderr: + return + raise + + +def fetch_issue_comments(repo: str, number: int) -> list[dict]: + """Fetch all issue-style comments on a PR/issue (paginated). + + `gh api --paginate` returns one JSON array per page; iterate them and + flatten. Returns [] on error (the reconsider path treats "no comments + found" as "no proof Agent Shin closed this", which fails-safe). + """ + try: + raw = gh( + "api", + "--paginate", + f"repos/{repo}/issues/{number}/comments?per_page=100", + ) + except subprocess.CalledProcessError: + return [] + comments: list[dict] = [] + for line in raw.strip().splitlines(): + line = line.strip() + if not line: + continue + try: + parsed = json.loads(line) + except json.JSONDecodeError: + continue + if isinstance(parsed, list): + comments.extend(parsed) + else: + comments.append(parsed) + return comments + + +def fetch_issue_events(repo: str, number: int) -> list[dict]: + """Fetch all issue events for a PR/issue (paginated, ascending order). + + Used by the reconsider provenance check to identify the actor of the + most recent `closed` event. Returns [] on error so the reconsider + path fails safe (no proof of Agent Shin close -> refuse to reopen). + """ + try: + raw = gh( + "api", + "--paginate", + f"repos/{repo}/issues/{number}/events?per_page=100", + ) + except subprocess.CalledProcessError: + return [] + events: list[dict] = [] + for line in raw.strip().splitlines(): + line = line.strip() + if not line: + continue + try: + parsed = json.loads(line) + except json.JSONDecodeError: + continue + if isinstance(parsed, list): + events.extend(parsed) + else: + events.append(parsed) + return events + + +def was_auto_closed_by_agent_shin(repo: str, number: int) -> bool: + """Return True iff Agent Shin is responsible for the *current* closure. + + Provenance check for the `@agent-shin reconsider` flow. We require ALL of: + + 1. The most recent `closed` event on the PR/issue was performed by + a bot account (actor login ends with "[bot]"). Agent Shin's + auto-close workflow uses GH_TOKEN, which posts as + `github-actions[bot]`. Anchoring on the most recent close β€” not + just any historical close β€” prevents a contributor from + overriding a *later* maintainer-initiated closure (e.g. + duplicate, out-of-scope) by polishing the description and + commenting `@agent-shin reconsider`. + 2. A comment authored by the same bot login that performed the + close contains the Agent Shin auto-close marker + (`AGENT_SHIN_AUTO_CLOSE_MARKER`) AND was posted in the current + openβ†’closed cycle (after the most recent `reopened` event, if + any, and no later than the most recent `closed` event). This + anchors the marker to the close that's actually being + reconsidered: a stale-workflow closure that runs as + `github-actions[bot]` (because `actions/stale` uses + `secrets.GITHUB_TOKEN`, the same identity as Agent Shin) does + NOT post a marker in its own cycle, so the cycle-anchored check + refuses to override it even though a historical Agent Shin + marker comment still exists from an earlier cycle. + """ + events = fetch_issue_events(repo, number) + last_close_ts = "" + last_closer = "" + last_reopen_ts = "" + for event in events: + kind = (event.get("event") or "").lower() + ts = event.get("created_at") or "" + if kind == "closed": + last_close_ts = ts + last_closer = ((event.get("actor") or {}).get("login") or "").lower() + elif kind == "reopened": + last_reopen_ts = ts + if not last_closer or not last_closer.endswith("[bot]"): + return False + for comment in fetch_issue_comments(repo, number): + login = ((comment.get("user") or {}).get("login") or "").lower() + if login != last_closer: + continue + body = comment.get("body") or "" + if AGENT_SHIN_AUTO_CLOSE_MARKER not in body: + continue + comment_ts = comment.get("created_at") or "" + if last_reopen_ts and comment_ts <= last_reopen_ts: + continue + if last_close_ts and comment_ts > last_close_ts: + continue + return True + return False + + +# --------------------------------------------------------------------------- +# Author classification + + +def is_internal_contributor(item: dict) -> bool: + """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 + + +# --------------------------------------------------------------------------- +# Greptile score + age helpers (shared with close_low_quality_prs.py) + + +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")) + + +# --------------------------------------------------------------------------- +# 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)" + # 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. + + 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() + 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)" + # 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. + + 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() + return template.format(title=title, cleaned_body=cleaned_body) + + +# --------------------------------------------------------------------------- +# 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) + ) + 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 "" + + +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 ( + f"πŸ‘‹ Hi, thanks for the PR! {AGENT_SHIN_AUTO_CLOSE_MARKER}, 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. Either:\n" + " - **Open a new PR** with the same fixes β€” recommended path. GitHub does not let external " + "contributors reopen a PR that was closed by a bot/maintainer, so a fresh PR is the most reliable way " + "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" + "\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.)_" + ) + + +def format_issue_close_comment(verdict: dict) -> str: + missing_lines = _format_missing(verdict.get("missing") or []) + explanation = verdict.get("explanation") or "" + return ( + f"πŸ‘‹ Hi, thanks for filing this! {AGENT_SHIN_AUTO_CLOSE_MARKER}, 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. Comment `@agent-shin reconsider` on this issue once you've updated it. " + "I'll re-run triage and reopen the issue if it now meets the bar. " + "(GitHub doesn't always let the original reporter reopen a bot-closed issue, " + "so the comment-based reconsider is the reliable path.)\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.)_" + ) + + +# --------------------------------------------------------------------------- +# 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 format_reopen_comment(kind: str) -> str: + """Comment posted when Agent Shin reopens after a successful reconsider.""" + noun = "PR" if kind == "pr" else "issue" + return ( + f"♻️ **Re-evaluated and reopened.** Thanks for updating the {noun}!\n" + "\n" + "Agent Shin re-ran triage on the latest description and it now meets " + "the bar. A maintainer will take another look soon β€” please don't " + f"close this {noun} again unless asked to.\n" + "\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.)_" + ) + + +def format_reconsider_still_failing_comment(kind: str, verdict: dict) -> str: + """Comment posted when reconsider re-runs triage but the verdict is still fail.""" + missing_lines = _format_missing(verdict.get("missing") or []) + explanation = verdict.get("explanation") or "" + noun = "PR" if kind == "pr" else "issue" + return ( + f"⏸️ **Re-evaluated; this {noun} still doesn't meet the rubric.**\n" + "\n" + "Agent Shin re-ran triage on the current description but is still " + "missing:\n" + "\n" + f"{missing_lines}\n" + "\n" + f"> {explanation}\n" + "\n" + "Update the description with the missing pieces and comment " + "`@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.)_" + ) + + +# --------------------------------------------------------------------------- +# Review gate β€” "ready for review" label lifecycle + +_UNSET = object() + + +def _combine_missing( + verdict: dict, greptile_score: int | None, min_score: int +) -> list[str]: + """Merge the LLM rubric's `missing` list with a Greptile-score shortfall.""" + missing = list(verdict.get("missing") or []) + if greptile_score is not None and greptile_score < min_score: + missing.insert( + 0, + f"Greptile's most recent review scored this PR {greptile_score}/5 " + f"(below the {min_score}/5 bar)", + ) + return missing or ["(see explanation below)"] + + +def _has_marker(comments: Iterable[dict], marker: str) -> bool: + return any(marker in (comment.get("body") or "") for comment in comments) + + +def format_ready_for_review_comment(verdict: dict, greptile_score: int | None) -> str: + """Posted the first time a PR clears the bar (label added).""" + score_line = ( + f" Greptile scored it **{greptile_score}/5**." + if greptile_score is not None + else "" + ) + explanation = verdict.get("explanation") or "" + return ( + "βœ… **Triage passed β€” tagging `ready for review`.**\n" + "\n" + "Agent Shin checked this PR against the " + "[contribution rubric](https://github.com/BerriAI/litellm/blob/main/.github/pull_request_template.md) " + "and it clears the bar (a linked issue, or a clear problem description " + f"+ expected vs. actual + QA proof).{score_line}\n" + "\n" + f"> {explanation}\n" + "\n" + "A maintainer will take it from here. If a later re-check finds the PR " + f"has regressed (Greptile drops below {DEFAULT_MIN_GREPTILE_SCORE}/5, " + "the QA proof is removed, etc.) I'll pull the tag and comment with " + "what's missing β€” fix it and the tag comes back automatically.\n" + f"{READY_MARKER}" + ) + + +def format_all_clear_comment(verdict: dict, greptile_score: int | None) -> str: + """Posted when a PR recovers after a regression (label re-added).""" + score_line = ( + f" Greptile is back to **{greptile_score}/5**." + if greptile_score is not None + else "" + ) + explanation = verdict.get("explanation") or "" + return ( + "βœ… **All clear again β€” re-adding `ready for review`.**\n" + "\n" + "Thanks for addressing the earlier feedback. On re-check this PR meets " + f"the contribution bar once more.{score_line}\n" + "\n" + f"> {explanation}\n" + "\n" + "A maintainer will take another look.\n" + f"{READY_MARKER}" + ) + + +def format_regression_comment(missing: list[str], explanation: str) -> str: + """Posted when a previously-tagged PR regresses (label removed, PR stays open).""" + return ( + "⚠️ **Removing the `ready for review` tag.**\n" + "\n" + "On a re-check this PR no longer meets the contribution bar. What's " + "missing now:\n" + "\n" + f"{_format_missing(missing)}\n" + "\n" + f"> {explanation}\n" + "\n" + "The PR stays open β€” address the points above and Agent Shin will post " + 'an "all clear" comment and re-add the tag automatically.\n' + f"{REGRESSED_MARKER}" + ) + + +def format_within_grace_comment( + missing: list[str], explanation: str, grace_days: int +) -> str: + """Posted once while a failing PR is still inside its grace window.""" + window = "24 hours" if grace_days == 1 else f"{grace_days} days" + return ( + "πŸ‘‹ Hi, thanks for the PR! This is **Agent Shin**, the automated triage " + "bot. This PR doesn't meet the contribution bar yet:\n" + "\n" + f"{_format_missing(missing)}\n" + "\n" + f"> {explanation}\n" + "\n" + f"You have ~{window} from when this PR was opened to add the missing " + "pieces. Once it passes I'll tag it `ready for review`; otherwise I'll " + "auto-close it (you can always re-open the conversation with " + "`@agent-shin reconsider`).\n" + f"{WITHIN_GRACE_MARKER}" + ) + + +def review_gate( + *, + repo: str, + number: int, + close: bool, + model: str, + judge: Any = None, + greptile_score: Any = _UNSET, + comments: Any = _UNSET, + now: dt.datetime | None = None, + grace_days: int = DEFAULT_GRACE_DAYS, + min_greptile_score: int = DEFAULT_MIN_GREPTILE_SCORE, + label: str = READY_FOR_REVIEW_LABEL, +) -> dict: + """Reconcile the `ready for review` label with a PR's current quality. + + A PR is *passing* when it clears BOTH gates: the LLM rubric (linked issue, + or problem description + expected/actual + QA proof) AND Greptile's most + recent confidence score (>= ``min_greptile_score``; absence of a score is + not held against the PR). The gate then drives a small state machine, using + the label itself as the persisted state so comments fire only on + transitions (never on every scheduled run): + + passing, untagged -> add label + "ready for review" / "all clear" + passing, tagged -> noop-passing + not passing, tagged -> remove label + regression comment (stays open) + not passing, untagged, old -> close + comment (past the grace window) + not passing, untagged, new -> one-time "what's missing" notice (within grace) + + ``close`` gates every destructive side effect: with ``close=False`` the + function returns a ``would-*`` preview and touches nothing, mirroring the + dry-run contract of :func:`triage`. ``judge``/``greptile_score``/ + ``comments``/``now`` are injectable for tests; in production they are + resolved from the OpenAI judge, the PR's Greptile comment, the live comment + list, and the wall clock respectively. + """ + item = fetch_pr(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 "" + labels_now = {(lbl.get("name") or "") for lbl in (item.get("labels") or [])} + created_raw = item.get("created_at") or "" + + base_result = { + "kind": "pr", + "number": number, + "title": title, + "author": login, + "author_association": association, + "state": state, + "labeled": label in labels_now, + "review_gate": True, + } + + if state != "open": + return {**base_result, "action": "skip-not-open"} + + if is_internal_contributor(item): + return {**base_result, "action": "skip-internal-author"} + + # Resolve the comment list once β€” used for both the Greptile score and the + # marker-based dedup below. + if comments is _UNSET: + comments = fetch_issue_comments(repo, number) + + # --- rubric verdict: linked-issue short-circuit, else the LLM judge ------- + if has_linked_issue(body): + verdict = { + "verdict": "pass", + "linked_issue": True, + "missing": [], + "explanation": "Linked-issue regex matched; LLM was not called.", + } + rubric_pass = True + else: + prompt = build_pr_prompt(title=title, body=body) + if judge is None: + api_key = os.environ.get("OPENAI_API_KEY") + if not api_key: + return {**base_result, "action": "skip-no-llm-key"} + 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: + verdict = parse_verdict(judge(prompt)) + except Exception as exc: # noqa: BLE001 - judge errors must never act + return {**base_result, "action": "skip-llm-error", "error": str(exc)} + rubric_pass = (verdict.get("verdict") or "").lower() == "pass" + + # --- Greptile score ------------------------------------------------------- + if greptile_score is _UNSET: + extraction = extract_greptile_score(comments) + greptile_score = extraction[0] if extraction else None + greptile_ok = greptile_score is None or greptile_score >= min_greptile_score + passing = rubric_pass and greptile_ok + + # --- age ------------------------------------------------------------------ + age_days = None + if created_raw: + reference = now or dt.datetime.now(dt.timezone.utc) + age_days = (reference - parse_iso8601(created_raw)).days + + label_present = label in labels_now + explanation = verdict.get("explanation") or "" + base_result = { + **base_result, + "verdict": verdict, + "greptile_score": greptile_score, + "passing": passing, + "age_days": age_days, + } + + if passing: + if label_present: + return {**base_result, "action": "noop-passing"} + recovered = _has_marker(comments, REGRESSED_MARKER) + comment = ( + format_all_clear_comment(verdict, greptile_score) + if recovered + else format_ready_for_review_comment(verdict, greptile_score) + ) + if not close: + return {**base_result, "action": "would-label-ready", "comment": comment} + post_comment(repo, number, comment) + add_label(repo, number, label) + return {**base_result, "action": "labeled-ready", "comment": comment} + + missing = _combine_missing(verdict, greptile_score, min_greptile_score) + + if label_present: + comment = format_regression_comment(missing, explanation) + if not close: + return {**base_result, "action": "would-remove-label", "comment": comment} + remove_label(repo, number, label) + post_comment(repo, number, comment) + return {**base_result, "action": "label-removed-regressed", "comment": comment} + + # Not passing and not tagged: close if past the grace window, else notify once. + if age_days is not None and age_days >= grace_days: + comment = format_pr_close_comment({**verdict, "missing": missing}) + if not close: + return {**base_result, "action": "would-close", "comment": comment} + post_comment(repo, number, comment) + close_pr(repo, number) + return {**base_result, "action": "closed", "comment": comment} + + if _has_marker(comments, WITHIN_GRACE_MARKER): + return {**base_result, "action": "within-grace-already-notified"} + comment = format_within_grace_comment(missing, explanation, grace_days) + if not close: + return { + **base_result, + "action": "would-notify-within-grace", + "comment": comment, + } + post_comment(repo, number, comment) + return {**base_result, "action": "within-grace-notified", "comment": comment} + + +def triage( + *, + repo: str, + kind: str, + number: int, + close: bool, + model: str, + judge: Any = None, + print_prompt: bool = False, + reconsider: 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`. + + When `reconsider=True`, the closed-state guard is skipped and a + 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` still controls whether destructive side effects fire. When + `close=False` and `reconsider=True`, the function previews the + decision (`would-reopen`, `would-leave-closed-still-failing`, or + `skip-not-bot-closed`) without posting comments or reopening β€” this + mirrors the regular `would-close` dry-run behavior so the reconsider + workflow can be exercised safely with `AGENT_SHIN_ENABLED != "true"`. + + Provenance: when `reconsider=True`, we additionally check that the + PR/issue was actually auto-closed by Agent Shin (via the bot-authored + auto-close marker comment). If not, we refuse to reopen so a + maintainer-closed PR cannot be silently overridden by the author + polishing the description and commenting `@agent-shin reconsider`. + """ + 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, + "reconsider": reconsider, + } + + # Reconsider only makes sense on a closed PR/issue. A "reconsider on an + # open PR" is a no-op (the regular triage flow already evaluates open + # PRs); return a clear skip so the workflow can short-circuit. + if reconsider: + if state != "closed": + return {**base_result, "action": "skip-not-closed"} + else: + if state != "open": + return {**base_result, "action": "skip-not-open"} + + if is_internal_contributor(item): + return {**base_result, "action": "skip-internal-author"} + + # Provenance gate for reconsider: only reopen items Agent Shin auto-closed. + # Done up-front so a maintainer-closed PR short-circuits before we burn + # LLM tokens or touch comments. The check requires a bot-authored + # comment containing the auto-close marker (see + # `was_auto_closed_by_agent_shin`), so a contributor cannot spoof it by + # quoting the close template themselves. + if reconsider and not was_auto_closed_by_agent_shin(repo, number): + return {**base_result, "action": "skip-not-bot-closed"} + + 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): + base = { + **base_result, + "action": "pass-linked-issue", + "verdict": { + "verdict": "pass", + "linked_issue": True, + "explanation": "Linked-issue regex matched; LLM was not called.", + }, + } + if reconsider: + reopen_body = format_reopen_comment(kind) + if not close: + return { + **base, + "action": "would-reopen", + "comment": reopen_body, + } + # Reopen before posting so a failed reopen call doesn't + # leave a misleading "we reopened it" comment on a still- + # closed PR. + reopen_pr(repo, number) + post_comment(repo, number, reopen_body) + return { + **base, + "action": "reopened", + "comment": reopen_body, + } + return base + 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 reconsider: + # Reconsider: pass -> reopen + post reopen comment; + # fail -> leave closed + post a "still failing" comment so the + # contributor can iterate again. When `close=False` we preview + # the action instead of actually posting/reopening. + # Only an explicit "pass" verdict triggers a reopen β€” any other + # value (including missing/malformed verdicts) is fail-safe and + # leaves the PR/issue closed. + if decision == "pass": + reopen_body = format_reopen_comment(kind) + if not close: + return { + **base_result, + "action": "would-reopen", + "verdict": verdict, + "comment": reopen_body, + } + # Reopen before posting so a failed reopen call doesn't leave a + # misleading "reopened" comment on a still-closed item. + if kind == "pr": + reopen_pr(repo, number) + else: + reopen_issue(repo, number) + post_comment(repo, number, reopen_body) + return { + **base_result, + "action": "reopened", + "verdict": verdict, + "comment": reopen_body, + } + still_failing = format_reconsider_still_failing_comment(kind, verdict) + if not close: + return { + **base_result, + "action": "would-leave-closed-still-failing", + "verdict": verdict, + "comment": still_failing, + } + post_comment(repo, number, still_failing) + return { + **base_result, + "action": "reconsider-still-failing", + "verdict": verdict, + "comment": still_failing, + } + + 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', '')}") + if result.get("review_gate"): + score = result.get("greptile_score") + lines.append( + f"- **Greptile**: {score}/5" + if score is not None + else "- **Greptile**: (no score yet)" + ) + lines.append(f"- **`ready for review` label present**: {result.get('labeled')}") + if result.get("age_days") is not None: + lines.append(f"- **Age**: {result['age_days']}d") + 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("### Posted 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", + # `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( + "--print-prompt", + action="store_true", + help="Print the prompt that would be sent to the judge and exit.", + ) + parser.add_argument( + "--reconsider", + action="store_true", + help=( + "Re-run triage on a CLOSED PR/issue and reopen it on pass. " + "Used by the `@agent-shin reconsider` comment-trigger workflow. " + "Only invoke this from a workflow that has already gated on " + "AGENT_SHIN_ENABLED=true and verified the commenter is the " + "PR/issue author or an internal collaborator." + ), + ) + parser.add_argument( + "--review-gate", + action="store_true", + help=( + "Reconcile the `ready for review` label for an OPEN PR: tag on " + "pass, remove the tag + comment on regression, close after the " + "grace window if it never passed. PR-only." + ), + ) + parser.add_argument( + "--grace-days", + type=int, + default=DEFAULT_GRACE_DAYS, + help=( + "Review-gate only: hours/24 a failing, un-tagged PR may stay open " + f"before auto-close (default: {DEFAULT_GRACE_DAYS} = 24h)." + ), + ) + parser.add_argument( + "--min-greptile-score", + type=int, + default=DEFAULT_MIN_GREPTILE_SCORE, + choices=range(1, 6), + help=( + "Review-gate only: Greptile score below which a PR counts as not " + f"passing (default: {DEFAULT_MIN_GREPTILE_SCORE} -> <4/5 regresses)." + ), + ) + 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 + + if args.review_gate: + if kind != "pr": + parser.error("--review-gate applies to pull requests only (use --pr).") + result = review_gate( + repo=args.repo, + number=number, + close=args.close, + model=args.model, + grace_days=args.grace_days, + min_greptile_score=args.min_greptile_score, + ) + else: + result = triage( + repo=args.repo, + kind=kind, + number=number, + close=args.close, + model=args.model, + print_prompt=args.print_prompt, + reconsider=args.reconsider, + ) + + 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/close_low_quality_prs.yml b/.github/workflows/close_low_quality_prs.yml new file mode 100644 index 000000000000..2401be84000e --- /dev/null +++ b/.github/workflows/close_low_quality_prs.yml @@ -0,0 +1,92 @@ +name: Close Low-Quality PRs + +# Auto-close any open PR (including drafts, regardless of age) authored by an +# external OSS contributor that Greptile reviewed with a confidence score +# below 4/5. Closures are explained in a comment that tells the contributor +# to push fixes and open a fresh PR (since OSS authors cannot reopen a PR +# closed by a bot/maintainer) or comment `@agent-shin reconsider` to have +# Agent Shin re-evaluate. +# +# Manual one-off run: +# gh workflow run "Close Low-Quality PRs" -f close=true +# +# Dry-run preview (no PRs are touched): +# gh workflow run "Close Low-Quality 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: "false" + type: choice + options: + - "true" + - "false" + min_age_days: + description: "Minimum PR age in days (default 0 = no age filter)." + required: false + default: "0" + 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 }} + # 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 || '0' }} + 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 [ "${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:-}" = "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 this trigger is dry-run (scheduled event or close=false)." + fi + python3 .github/scripts/close_low_quality_prs.py "${ARGS[@]}" diff --git a/.github/workflows/review_gate.yml b/.github/workflows/review_gate.yml new file mode 100644 index 000000000000..73215f359e4a --- /dev/null +++ b/.github/workflows/review_gate.yml @@ -0,0 +1,128 @@ +name: Agent Shin β€” review gate + +# Keeps the `ready for review` label in sync with whether an external PR +# currently clears BOTH the LLM rubric AND Greptile's confidence score. +# +# pass -> add `ready for review` + a "passed / all clear" comment +# regress -> remove the label + a "what's missing" comment (PR stays open) +# fail, <24h old -> a one-time "what's missing" notice (grace window) +# fail, >24h old -> close + a comment (reopen via `@agent-shin reconsider`) +# +# DRY-RUN BY DEFAULT. Every side effect (label add/remove, comment, close) is +# gated behind `--close`, which is only added when the repo variable +# `AGENT_SHIN_ENABLED == "true"`. Until then runs only write the verdict to the +# workflow step summary. +# +# Manual single PR: gh workflow run "Agent Shin β€” review gate" -f pr_number=NNN +# Manual dry-run: gh workflow run "Agent Shin β€” review gate" -f close=false +# +# We use `pull_request_target` so the workflow can read repo secrets and run +# against fork PRs. Fork code is never checked out β€” only PR metadata is read +# via `gh api`. + +on: + pull_request_target: + types: [opened, reopened, synchronize, ready_for_review] + schedule: + # Daily at 09:30 UTC β€” re-reconciles labels as Greptile re-reviews land. + - cron: "30 9 * * *" + workflow_dispatch: + inputs: + pr_number: + description: "Single PR to reconcile (omit to sweep all open PRs)." + required: false + close: + description: "If AGENT_SHIN_ENABLED=true, actually act (false = dry run)." + required: false + default: "false" + type: choice + options: + - "true" + - "false" + grace_days: + description: "Hours/24 a failing, un-tagged PR may stay open before close." + required: false + default: "1" + min_greptile_score: + description: "Greptile score below which a PR counts as not passing (1-5)." + required: false + default: "4" + +permissions: + contents: read + issues: write + pull-requests: write + +jobs: + review-gate: + if: github.repository == 'BerriAI/litellm' + runs-on: ubuntu-latest + steps: + - name: Checkout triage script + uses: actions/checkout@08eba0b27e820071cde6df949e0beb9ba4906955 # v4.3.0 + with: + sparse-checkout: .github/scripts + persist-credentials: false + + - name: Set up Python + uses: actions/setup-python@a26af69be951a213d495a4c3e4e4022e16d87065 # v5.6.0 + with: + python-version: "3.12" + + - name: Install LLM client + run: pip install --no-cache-dir "openai>=1.40.0" + + - name: Run review gate + env: + GH_TOKEN: ${{ secrets.GITHUB_TOKEN }} + # Mirror the triage workflow: only expose the LLM key when the bot is + # enabled or a collaborator triggers it manually, so an external user + # can't force paid LLM calls by churning a fork PR while the bot is + # still in dry-run. + OPENAI_API_KEY: ${{ (vars.AGENT_SHIN_ENABLED == 'true' || github.event_name == 'workflow_dispatch') && secrets.OPENAI_API_KEY || '' }} + OPENAI_BASE_URL: ${{ vars.OPENAI_BASE_URL }} + TRIAGE_MODEL: ${{ vars.TRIAGE_MODEL }} + AGENT_SHIN_ENABLED: ${{ vars.AGENT_SHIN_ENABLED }} + CLOSE_FLAG: ${{ github.event.inputs.close || 'false' }} + GRACE_DAYS: ${{ github.event.inputs.grace_days || '1' }} + MIN_GREPTILE_SCORE: ${{ github.event.inputs.min_greptile_score || '4' }} + EVENT_PR: ${{ github.event.pull_request.number }} + INPUT_PR: ${{ github.event.inputs.pr_number }} + run: | + set -euo pipefail + COMMON=(--review-gate --grace-days "${GRACE_DAYS}" --min-greptile-score "${MIN_GREPTILE_SCORE}") + + # Fail-safe gating, identical philosophy to the Greptile closer: + # - AGENT_SHIN_ENABLED must be the EXACT string "true" to act at all. + # - A manual dispatch can still preview with close=false. + # - Automatic triggers (PR events, schedule) act once enabled β€” that + # is the whole point of the gate (re-tag / un-tag automatically). + DO_CLOSE="false" + if [ "${AGENT_SHIN_ENABLED:-false}" != "true" ]; then + echo "::notice::AGENT_SHIN_ENABLED is not 'true' -> dry-run (no labels/comments/closes)." + elif [ "${GITHUB_EVENT_NAME:-}" = "workflow_dispatch" ] && [ "${CLOSE_FLAG:-false}" = "true" ]; then + DO_CLOSE="true" + echo "::notice::Manual run -> acting for real." + elif [ "${GITHUB_EVENT_NAME:-}" != "workflow_dispatch" ]; then + DO_CLOSE="true" + echo "::notice::Enabled automatic trigger (${GITHUB_EVENT_NAME:-}) -> acting for real." + else + echo "::notice::Manual dispatch with close=false -> dry-run." + fi + if [ "${DO_CLOSE}" = "true" ]; then + COMMON+=(--close) + fi + + # Single PR (PR event or explicit input) vs. sweep over all open PRs. + TARGET_PR="${EVENT_PR:-${INPUT_PR:-}}" + if [ -n "${TARGET_PR}" ]; then + python3 .github/scripts/triage_with_llm.py --repo "${{ github.repository }}" --pr "${TARGET_PR}" "${COMMON[@]}" + else + echo "::notice::Sweeping all open PRs." + mapfile -t NUMBERS < <(gh pr list --repo "${{ github.repository }}" --state open --limit 1000 --json number --jq '.[].number') + for n in "${NUMBERS[@]}"; do + echo "::group::PR #${n}" + python3 .github/scripts/triage_with_llm.py --repo "${{ github.repository }}" --pr "${n}" "${COMMON[@]}" || echo "::warning::review gate errored on #${n}" + echo "::endgroup::" + done + fi diff --git a/.github/workflows/triage_issue_with_llm.yml b/.github/workflows/triage_issue_with_llm.yml new file mode 100644 index 000000000000..f85e95136117 --- /dev/null +++ b/.github/workflows/triage_issue_with_llm.yml @@ -0,0 +1,98 @@ +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 }} + # Only expose the LLM key when the workflow is actually allowed to + # take action (AGENT_SHIN_ENABLED=true) or when a collaborator runs + # it manually via workflow_dispatch (write access required to + # trigger). On the public issues path while the bot is not yet + # enabled, the key is intentionally absent so the script + # short-circuits with skip-no-llm-key β€” otherwise an external user + # could open/reopen issues with large bodies to force paid LLM + # calls. + OPENAI_API_KEY: ${{ (vars.AGENT_SHIN_ENABLED == 'true' || github.event_name == 'workflow_dispatch') && secrets.OPENAI_API_KEY || '' }} + OPENAI_BASE_URL: ${{ vars.OPENAI_BASE_URL }} + TRIAGE_MODEL: ${{ vars.TRIAGE_MODEL }} + AGENT_SHIN_ENABLED: ${{ vars.AGENT_SHIN_ENABLED }} + 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}") + # 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 != '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 + # Automatic `issues` events stay dry-run regardless until the team + # explicitly invokes workflow_dispatch with close=true. + if [ "${GITHUB_EVENT_NAME:-}" = "issues" ]; then + # 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 new file mode 100644 index 000000000000..fdf2dd354008 --- /dev/null +++ b/.github/workflows/triage_pr_with_llm.yml @@ -0,0 +1,111 @@ +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 }} + # Only expose the LLM key when the workflow is actually allowed to + # take action (AGENT_SHIN_ENABLED=true) or when a collaborator runs + # it manually via workflow_dispatch (write access required to + # trigger). On the public pull_request_target path while the bot is + # not yet enabled, the key is intentionally absent so the script + # short-circuits with skip-no-llm-key β€” otherwise an external user + # could open/reopen PRs with large bodies to force paid LLM calls. + OPENAI_API_KEY: ${{ (vars.AGENT_SHIN_ENABLED == 'true' || github.event_name == 'workflow_dispatch') && secrets.OPENAI_API_KEY || '' }} + OPENAI_BASE_URL: ${{ vars.OPENAI_BASE_URL }} + TRIAGE_MODEL: ${{ vars.TRIAGE_MODEL }} + AGENT_SHIN_ENABLED: ${{ vars.AGENT_SHIN_ENABLED }} + 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}") + # 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 != '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 + # 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 (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[@]}" diff --git a/.github/workflows/triage_reconsider.yml b/.github/workflows/triage_reconsider.yml new file mode 100644 index 000000000000..88ba37d00abf --- /dev/null +++ b/.github/workflows/triage_reconsider.yml @@ -0,0 +1,144 @@ +name: Agent Shin β€” reconsider + +# Comment-trigger workflow: when the PR/issue author (or an internal +# collaborator) comments `@agent-shin reconsider` on a CLOSED PR/issue, +# Agent Shin re-runs LLM-judge triage on the current title+body and: +# +# - on PASS: posts a "re-evaluated and reopened" comment + reopens. +# - on FAIL: posts a "still missing X" comment and leaves it closed, +# so the contributor can iterate again. +# +# This exists because GitHub does NOT let an external (non-write-access) +# OSS contributor reopen a PR/issue closed by a bot or maintainer. Without +# this comment trigger, a contributor whose PR Agent Shin auto-closed +# would have no path back into the review queue except opening a fresh PR +# (which loses the original PR's history). The bot, on the other hand, +# has write access via GH_TOKEN and can reopen on their behalf. +# +# DRY-RUN BY DEFAULT β€” gated on `vars.AGENT_SHIN_ENABLED == 'true'` just +# like the other Agent Shin workflows. The workflow also gates on the +# commenter being either the PR/issue author or an internal collaborator +# (OWNER/MEMBER/COLLABORATOR) so random commenters cannot DOS the LLM +# judge or force a reopen. + +on: + issue_comment: + types: [created] + +permissions: + contents: read + issues: write + pull-requests: write + +jobs: + reconsider: + # Reconsider only makes sense on a CLOSED PR/issue β€” its job is to + # re-evaluate and (on pass) reopen. Gating the job here means a + # stray `@agent-shin reconsider` on an open PR/issue exits before + # checkout/Python/pip ever runs, instead of spinning up the runner + # to be a no-op. The triage script itself still re-checks state + # (`skip-not-closed`) as defense in depth. + if: | + github.repository == 'BerriAI/litellm' + && contains(github.event.comment.body, '@agent-shin reconsider') + && github.event.issue.state == 'closed' + runs-on: ubuntu-latest + steps: + - name: Authorize commenter + # Only the PR/issue author OR an internal collaborator may trigger + # a reconsider. Outside random commenters could otherwise spam the + # phrase to burn LLM budget or, if a fail-open bug were ever + # introduced, force a reopen on someone else's behalf. + # + # We expose the authorization decision as a step output and gate + # every subsequent (potentially destructive) step on it. A `run:` + # step with `exit 0` would NOT stop the job β€” only `if:` gating + # on a known-true output is safe here. + id: auth + env: + COMMENTER: ${{ github.event.comment.user.login }} + AUTHOR: ${{ github.event.issue.user.login }} + ASSOCIATION: ${{ github.event.comment.author_association }} + run: | + set -euo pipefail + if [ "${COMMENTER}" = "${AUTHOR}" ]; then + echo "::notice::Authorized: commenter is the PR/issue author." + echo "authorized=true" >> "$GITHUB_OUTPUT" + exit 0 + fi + case "${ASSOCIATION}" in + OWNER|MEMBER|COLLABORATOR) + echo "::notice::Authorized: commenter is an internal collaborator (${ASSOCIATION})." + echo "authorized=true" >> "$GITHUB_OUTPUT" + ;; + *) + echo "::notice::Commenter '${COMMENTER}' (${ASSOCIATION}) is not authorized to trigger reconsider; skipping subsequent steps." + echo "authorized=false" >> "$GITHUB_OUTPUT" + ;; + esac + + - name: Checkout triage script + if: steps.auth.outputs.authorized == 'true' + uses: actions/checkout@08eba0b27e820071cde6df949e0beb9ba4906955 # v4.3.0 + with: + sparse-checkout: .github/scripts + persist-credentials: false + + - name: Set up Python + if: steps.auth.outputs.authorized == 'true' + uses: actions/setup-python@a26af69be951a213d495a4c3e4e4022e16d87065 # v5.6.0 + with: + python-version: "3.12" + + - name: Install LLM client + if: steps.auth.outputs.authorized == 'true' + run: pip install --no-cache-dir "openai>=1.40.0" + + - name: Run Agent Shin reconsider + if: steps.auth.outputs.authorized == 'true' + env: + GH_TOKEN: ${{ secrets.GITHUB_TOKEN }} + # Mirror the OPENAI_API_KEY gate used in triage_pr_with_llm.yml and + # triage_issue_with_llm.yml: only expose the LLM key when the bot + # has been opted in (AGENT_SHIN_ENABLED=true). Reconsider has no + # workflow_dispatch path, so AGENT_SHIN_ENABLED is the sole gate. + # Without this, an external OSS contributor whose PR was auto-closed + # could comment `@agent-shin reconsider` and trigger paid LLM calls + # before the team has set AGENT_SHIN_ENABLED β€” the authorization + # check alone is not enough, since the PR/issue author counts as + # "authorized" but is still an external contributor. + OPENAI_API_KEY: ${{ vars.AGENT_SHIN_ENABLED == 'true' && secrets.OPENAI_API_KEY || '' }} + OPENAI_BASE_URL: ${{ vars.OPENAI_BASE_URL }} + TRIAGE_MODEL: ${{ vars.TRIAGE_MODEL }} + AGENT_SHIN_ENABLED: ${{ vars.AGENT_SHIN_ENABLED }} + # `issue_comment` events fire for both issues and PR comments. + # `issue.pull_request` is set iff this is a PR comment, so we use + # its presence to decide whether to invoke `--pr N` or `--issue N`. + IS_PR: ${{ github.event.issue.pull_request != null }} + NUMBER: ${{ github.event.issue.number }} + run: | + set -euo pipefail + if [ "${IS_PR}" = "true" ]; then + ARGS=(--repo "${{ github.repository }}" --pr "${NUMBER}" --reconsider) + else + ARGS=(--repo "${{ github.repository }}" --issue "${NUMBER}" --reconsider) + fi + # Reconsider is the destructive path here (it can post comments + # and reopen). The triage script honors `--close` even in + # reconsider mode β€” without it the script previews actions + # (`would-reopen` / `would-leave-closed-still-failing`) without + # writing to GitHub. Append `--close` only when AGENT_SHIN_ENABLED + # is the literal string "true"; everything else (unset, "false", + # "True", "yes", "1", typos) stays in dry-run. + # + # Use the positive `= "true"` gate (instead of `!= "true" -> exit`) + # so the workflow guardrails in + # tests/test_litellm/test_github_triage_workflows.py see the + # canonical fail-safe enable pattern. + if [ "${AGENT_SHIN_ENABLED:-false}" = "true" ]; then + ARGS+=(--close) + echo "::notice::Agent Shin reconsider ENABLED β€” running real triage." + 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/litellm/proxy/_types.py b/litellm/proxy/_types.py index 004f33e630ab..9046d522280e 100644 --- a/litellm/proxy/_types.py +++ b/litellm/proxy/_types.py @@ -255,6 +255,7 @@ class KeyManagementRoutes(str, enum.Enum): # team spend-log viewing SPEND_LOGS = "/spend/logs" + SPEND_LOGS_V2 = "/spend/logs/v2" class LiteLLMRoutes(enum.Enum): @@ -548,6 +549,7 @@ class LiteLLMRoutes(enum.Enum): KeyManagementRoutes.TEAM_KEY_BULK_UPDATE.value, KeyManagementRoutes.TEAM_DAILY_ACTIVITY.value, KeyManagementRoutes.SPEND_LOGS.value, + KeyManagementRoutes.SPEND_LOGS_V2.value, KeyManagementRoutes.KEY_RESET_SPEND.value, KeyManagementRoutes.KEY_ALIASES.value, ] @@ -599,6 +601,7 @@ class LiteLLMRoutes(enum.Enum): "/spend/tags", "/spend/calculate", "/spend/logs", + "/spend/logs/v2", "/spend/logs/ui", "/spend/logs/session/ui", "/cost/estimate", diff --git a/litellm/proxy/management_endpoints/team_endpoints.py b/litellm/proxy/management_endpoints/team_endpoints.py index 43fdc9ae1cf2..0d34974fbef4 100644 --- a/litellm/proxy/management_endpoints/team_endpoints.py +++ b/litellm/proxy/management_endpoints/team_endpoints.py @@ -64,6 +64,7 @@ UserAPIKeyAuth, ) from litellm.proxy.auth.auth_checks import ( + _cache_team_object, allowed_route_check_inside_route, can_org_access_model, get_org_object, @@ -130,6 +131,33 @@ def _sanitize_for_log(value: Any) -> str: return text.replace("\r", "").replace("\n", "") +async def _refresh_cached_team( + team_row: Any, + user_api_key_cache: Any, + proxy_logging_obj: Any, +) -> None: + """ + Refresh the in-memory cached team object after a DB write. + + Every endpoint that mutates `litellm_teamtable` must call this so the + cached `LiteLLM_TeamTableCachedObj` used by `common_checks` stays in + sync. Without this, subsequent auth checks read a stale team and can + 403 on permissions the DB has already granted (or, symmetrically, + keep granting permissions the DB has already revoked). + + `team_row` is the Prisma row returned by `update`/`find_unique` on + `litellm_teamtable`. It is converted to `LiteLLM_TeamTableCachedObj` + via `model_dump()` to match the cache shape `_cache_team_object` + expects. + """ + await _cache_team_object( + team_id=team_row.team_id, + team_table=LiteLLM_TeamTableCachedObj(**team_row.model_dump()), + user_api_key_cache=user_api_key_cache, + proxy_logging_obj=proxy_logging_obj, + ) + + async def _verify_team_access( team_obj: LiteLLM_TeamTable, user_api_key_dict: UserAPIKeyAuth, @@ -1591,7 +1619,6 @@ async def update_team( # noqa: PLR0915 ``` """ try: - from litellm.proxy.auth.auth_checks import _cache_team_object from litellm.proxy.proxy_server import ( litellm_proxy_admin_name, llm_router, @@ -1861,7 +1888,13 @@ async def update_team( # noqa: PLR0915 await prisma_client.db.litellm_teamtable.update( where={"team_id": data.team_id}, data=updated_kv, - include={"litellm_model_table": True}, # type: ignore + # `object_permission` is included so `_refresh_cached_team` + # doesn't write a cached team with the relation nulled out β€” + # see team_model_add for the full rationale. + include={ + "litellm_model_table": True, + "object_permission": True, + }, # type: ignore ) ) @@ -1874,9 +1907,8 @@ async def update_team( # noqa: PLR0915 verbose_proxy_logger.info( "Successfully updated team - %s, info", team_row.team_id ) - await _cache_team_object( - team_id=team_row.team_id, - team_table=LiteLLM_TeamTableCachedObj(**team_row.model_dump()), + await _refresh_cached_team( + team_row=team_row, user_api_key_cache=user_api_key_cache, proxy_logging_obj=proxy_logging_obj, ) @@ -4569,7 +4601,11 @@ async def team_model_add( }' ``` """ - from litellm.proxy.proxy_server import prisma_client + from litellm.proxy.proxy_server import ( + prisma_client, + proxy_logging_obj, + user_api_key_cache, + ) if prisma_client is None: raise HTTPException(status_code=500, detail={"error": "No db connected"}) @@ -4603,9 +4639,21 @@ async def team_model_add( ) updated_models = add_new_models_to_team(team_obj=team_obj, new_models=data.models) - # Update team + # Update team. `include` mirrors the relations the auth path consumes + # off the cached team object so that `_refresh_cached_team` doesn't + # null them out β€” see object_permission_utils.validate_key_search_tools_against_team + # and the MCP/agent authz paths, which treat a missing object_permission + # as "no team-level restriction". updated_team = await prisma_client.db.litellm_teamtable.update( - where={"team_id": data.team_id}, data={"models": updated_models} + where={"team_id": data.team_id}, + data={"models": updated_models}, + include={"object_permission": True}, # type: ignore + ) + + await _refresh_cached_team( + team_row=updated_team, + user_api_key_cache=user_api_key_cache, + proxy_logging_obj=proxy_logging_obj, ) return updated_team @@ -4640,7 +4688,11 @@ async def team_model_delete( }' ``` """ - from litellm.proxy.proxy_server import prisma_client + from litellm.proxy.proxy_server import ( + prisma_client, + proxy_logging_obj, + user_api_key_cache, + ) if prisma_client is None: raise HTTPException(status_code=500, detail={"error": "No db connected"}) @@ -4679,9 +4731,17 @@ async def team_model_delete( # Remove specified models updated_models = [m for m in current_models if m not in data.models] - # Update team + # Update team. See team_model_add for the rationale on `include`. updated_team = await prisma_client.db.litellm_teamtable.update( - where={"team_id": data.team_id}, data={"models": updated_models} + where={"team_id": data.team_id}, + data={"models": updated_models}, + include={"object_permission": True}, # type: ignore + ) + + await _refresh_cached_team( + team_row=updated_team, + user_api_key_cache=user_api_key_cache, + proxy_logging_obj=proxy_logging_obj, ) return updated_team diff --git a/tests/test_litellm/proxy/auth/test_route_checks.py b/tests/test_litellm/proxy/auth/test_route_checks.py index 4acf42996e06..ad00c55a8382 100644 --- a/tests/test_litellm/proxy/auth/test_route_checks.py +++ b/tests/test_litellm/proxy/auth/test_route_checks.py @@ -268,6 +268,47 @@ def test_mcp_management_routes_classified_as_management_not_llm_api(route): assert RouteChecks.is_management_route(route=route) is True +def test_spend_logs_v2_classified_as_management_not_llm_api(): + """Paginated spend logs are a management/spend read route, not an LLM API.""" + + assert RouteChecks.is_llm_api_route(route="/spend/logs/v2") is False + assert RouteChecks.is_management_route(route="/spend/logs/v2") is True + + +def test_virtual_key_management_routes_allows_spend_logs_v2(): + """Management virtual keys should be allowed to call the v2 spend logs endpoint.""" + + valid_token = UserAPIKeyAuth( + user_id="test_user", + allowed_routes=["management_routes"], + ) + + result = RouteChecks.is_virtual_key_allowed_to_call_route( + route="/spend/logs/v2", + valid_token=valid_token, + ) + + assert result is True + + +def test_virtual_key_llm_api_routes_denies_spend_logs_v2(): + """AI API virtual keys should not gain spend-log access.""" + + valid_token = UserAPIKeyAuth( + user_id="test_user", + allowed_routes=["llm_api_routes"], + ) + + with pytest.raises(HTTPException) as exc_info: + RouteChecks.is_virtual_key_allowed_to_call_route( + route="/spend/logs/v2", + valid_token=valid_token, + ) + + assert exc_info.value.status_code == 403 + assert "Virtual key is not allowed to call this route" in str(exc_info.value.detail) + + @pytest.mark.parametrize( "route", [ @@ -1322,6 +1363,7 @@ def test_proxy_admin_viewer_can_access_audit_logs(route): "/cost/estimate", # Public spend logs / spend tracking routes that admin viewer should read "/spend/logs", + "/spend/logs/v2", "/spend/keys", "/spend/users", "/spend/tags", diff --git a/tests/test_litellm/proxy/management_endpoints/test_team_endpoints.py b/tests/test_litellm/proxy/management_endpoints/test_team_endpoints.py index 41a5b891ad39..13bb39c35c93 100644 --- a/tests/test_litellm/proxy/management_endpoints/test_team_endpoints.py +++ b/tests/test_litellm/proxy/management_endpoints/test_team_endpoints.py @@ -1540,6 +1540,137 @@ def test_add_new_models_to_team_with_existing_models(): assert updated_models.sort() == ["model1", "model2", "model3", "model4"].sort() +@pytest.mark.asyncio +@pytest.mark.parametrize( + "endpoint_name", + ["team_model_add", "team_model_delete"], +) +async def test_team_model_add_delete_refresh_team_cache(endpoint_name): + """ + Regression pin for LIT-3244 vector-store BYOK 403. + + `team_model_add` and `team_model_delete` mutate `team.models` in the + DB. Without a cache refresh, the in-memory `LiteLLM_TeamTableCachedObj` + used by `common_checks` stays stale and team members 403 on a model + the DB has just granted (or, symmetrically, keep using a model the DB + has just revoked). + + Pin: after the DB update, the endpoint must call `_cache_team_object` + with the updated team row so the cached team stays in sync. + """ + from unittest.mock import AsyncMock, MagicMock, Mock, patch + + from fastapi import Request + + from litellm.proxy._types import ( + LitellmUserRoles, + TeamModelAddRequest, + TeamModelDeleteRequest, + UserAPIKeyAuth, + ) + from litellm.proxy.management_endpoints.team_endpoints import ( + team_model_add, + team_model_delete, + ) + + mock_request = Mock(spec=Request) + mock_user_api_key_dict = UserAPIKeyAuth( + user_role=LitellmUserRoles.PROXY_ADMIN, user_id="test_user_id" + ) + + existing_team = MagicMock() + existing_team.model_dump.return_value = { + "team_id": "team-1234", + "models": ["bedrock-claude-sonnet-4", "openai/*"], + "object_permission_id": "op-1234", + "object_permission": { + "object_permission_id": "op-1234", + "search_tools": ["allowed-tool-A"], + }, + } + + updated_team = MagicMock() + updated_team.team_id = "team-1234" + updated_team.model_dump.return_value = { + "team_id": "team-1234", + "models": ["bedrock-claude-sonnet-4", "openai/*", "team-byok-1"], + # The Prisma update must come back with `object_permission` populated + # (via `include={"object_permission": True}`), otherwise the cache + # write below would null it out β€” see LIT-3244 follow-up. + "object_permission_id": "op-1234", + "object_permission": { + "object_permission_id": "op-1234", + "search_tools": ["allowed-tool-A"], + }, + } + + with ( + patch("litellm.proxy.proxy_server.prisma_client") as mock_prisma_client, + patch("litellm.proxy.proxy_server.user_api_key_cache") as mock_cache, + patch("litellm.proxy.proxy_server.proxy_logging_obj") as mock_logging, + patch( + "litellm.proxy.management_endpoints.team_endpoints._cache_team_object" + ) as mock_cache_team, + ): + mock_prisma_client.db.litellm_teamtable.find_unique = AsyncMock( + return_value=existing_team + ) + mock_prisma_client.db.litellm_teamtable.update = AsyncMock( + return_value=updated_team + ) + mock_cache_team.return_value = None + + if endpoint_name == "team_model_add": + await team_model_add( + data=TeamModelAddRequest(team_id="team-1234", models=["team-byok-1"]), + http_request=mock_request, + user_api_key_dict=mock_user_api_key_dict, + ) + else: + await team_model_delete( + data=TeamModelDeleteRequest(team_id="team-1234", models=["openai/*"]), + http_request=mock_request, + user_api_key_dict=mock_user_api_key_dict, + ) + + # The pin: cache refresh must run with the updated team row. + assert mock_cache_team.await_count == 1, ( + f"{endpoint_name} must call _cache_team_object exactly once " + f"after the DB update (LIT-3244 regression pin); " + f"got await_count={mock_cache_team.await_count}" + ) + call_kwargs = mock_cache_team.await_args.kwargs + assert call_kwargs["team_id"] == "team-1234" + # The cached object must be built from the *updated* row, not the + # pre-mutation `existing_team` β€” that's the whole point. Both rows + # share team_id, so the only assertion that actually pins this is + # against the field that differs between them: `models`. + assert call_kwargs["team_table"].team_id == "team-1234" + assert call_kwargs["team_table"].models == [ + "bedrock-claude-sonnet-4", + "openai/*", + "team-byok-1", + ] + # And the cached object MUST carry the `object_permission` relation + # (LIT-3244 follow-up). If the Prisma update were missing + # `include={"object_permission": True}`, the cached team would have + # object_permission=None, and downstream consumers like + # `validate_key_search_tools_against_team` would treat that as + # "no team-level restriction" and stop enforcing the team's + # search-tool allowlist on key issuance. + assert call_kwargs["team_table"].object_permission is not None + assert call_kwargs["team_table"].object_permission.search_tools == [ + "allowed-tool-A" + ] + # Pin the Prisma call shape too β€” the regression is in *what the + # update returns*, so the contract that the update asks for + # `object_permission` belongs in this test. + update_call_kwargs = ( + mock_prisma_client.db.litellm_teamtable.update.call_args.kwargs + ) + assert update_call_kwargs.get("include", {}).get("object_permission") is True + + @pytest.mark.asyncio async def test_update_team_team_member_budget_not_passed_to_db(): """ @@ -1568,7 +1699,9 @@ async def test_update_team_team_member_budget_not_passed_to_db(): patch("litellm.proxy.proxy_server.user_api_key_cache") as mock_cache, patch("litellm.proxy.proxy_server.proxy_logging_obj") as mock_logging, patch("litellm.proxy.proxy_server.litellm_proxy_admin_name", "admin"), - patch("litellm.proxy.auth.auth_checks._cache_team_object") as mock_cache_team, + patch( + "litellm.proxy.management_endpoints.team_endpoints._cache_team_object" + ) as mock_cache_team, patch( "litellm.proxy.management_endpoints.team_endpoints.TeamMemberBudgetHandler.upsert_team_member_budget_table" ) as mock_upsert_budget, @@ -1999,7 +2132,9 @@ async def test_update_team_with_team_member_budget_duration(): patch("litellm.proxy.proxy_server.user_api_key_cache") as mock_cache, patch("litellm.proxy.proxy_server.proxy_logging_obj") as mock_logging, patch("litellm.proxy.proxy_server.litellm_proxy_admin_name", "admin"), - patch("litellm.proxy.auth.auth_checks._cache_team_object") as mock_cache_team, + patch( + "litellm.proxy.management_endpoints.team_endpoints._cache_team_object" + ) as mock_cache_team, patch( "litellm.proxy.management_endpoints.team_endpoints.TeamMemberBudgetHandler.upsert_team_member_budget_table" ) as mock_upsert_budget, 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 000000000000..316ca696179b --- /dev/null +++ b/tests/test_litellm/test_github_close_low_quality_prs.py @@ -0,0 +1,541 @@ +"""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 TestFetchPrComments: + """`fetch_pr_comments` must fail-safe so a transient `gh api` error on + one PR doesn't abort the whole daily sweep mid-loop. Pinning the + empty-list return matches the fail-safe pattern in + `fetch_pr_author_association`. + """ + + def test_should_return_empty_list_when_gh_api_fails( + self, closer_module, monkeypatch + ): + import subprocess + + def _failing_gh(*args, **kwargs): + raise subprocess.CalledProcessError(1, ["gh", *args]) + + monkeypatch.setattr(closer_module, "gh", _failing_gh) + assert closer_module.fetch_pr_comments(123, repo="x/y") == [] + + def test_should_return_empty_list_when_paginated_output_is_malformed( + self, closer_module, monkeypatch + ): + monkeypatch.setattr(closer_module, "gh", lambda *a, **kw: "not json\n") + assert closer_module.fetch_pr_comments(123, repo="x/y") == [] + + +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}", + } + + @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_close_drafts_when_score_low(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. + monkeypatch.setattr( + closer_module, + "fetch_pr_comments", + lambda *a, **kw: [_greptile_comment("Confidence Score: 2/5")], + ) + action, score, age = closer_module.evaluate_pr( + self._make_pr(is_draft=True, created_days_ago=0), + now=_now, + min_age_days=0, + min_score=4, + repo=None, + optout_labels=set(), + ) + assert action == "close" + assert score == 2 and age == 0 + + def test_should_close_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. + monkeypatch.setattr( + closer_module, + "fetch_pr_comments", + lambda *a, **kw: [_greptile_comment("Confidence Score: 1/5")], + ) + action, score, age = closer_module.evaluate_pr( + self._make_pr(created_days_ago=0), + now=_now, + min_age_days=0, + min_score=4, + repo=None, + optout_labels=set(), + ) + assert action == "close" + assert score == 1 and age == 0 + + 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_when_min_age_set( + self, closer_module, _now, monkeypatch + ): + # The min-age-days flag is now opt-in (default 0). When a maintainer + # explicitly passes a positive value (e.g. for a backfill run that + # wants to spare brand-new PRs), the skip-too-young path still works. + 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_not_skip_when_min_age_is_zero( + self, closer_module, _now, monkeypatch + ): + # With the new default min_age_days=0, even a 0-day-old PR is + # evaluated. This test pins that behavior so future refactors don't + # silently restore an age filter. + monkeypatch.setattr( + closer_module, + "fetch_pr_comments", + lambda *a, **kw: [_greptile_comment("Confidence Score: 5/5")], + ) + action, score, age = closer_module.evaluate_pr( + self._make_pr(created_days_ago=0), + now=_now, + min_age_days=0, + min_score=4, + repo=None, + optout_labels=set(), + ) + assert action == "skip-score-ok" + assert score == 5 and age == 0 + + 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 + + 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 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-internal", 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 TestMainLimitFlag: + """`--limit N` must cap closures in both dry-run and real mode.""" + + def _patch_three_closeable_prs(self, closer_module, monkeypatch): + prs = [ + { + "number": i, + "title": f"p{i}", + "createdAt": "2026-05-10T00:00:00Z", + "isDraft": False, + "labels": [], + "author": {"login": f"ext{i}"}, + } + for i in (1, 2, 3) + ] + monkeypatch.setattr(closer_module, "fetch_open_prs", lambda repo: prs) + monkeypatch.setattr( + closer_module, + "evaluate_pr", + lambda pr, now, mad, ms, repo, ol: ("close", 2, 30), + ) + called: list[int] = [] + + def fake_close_pr(pr, **kwargs): + called.append(pr["number"]) + + monkeypatch.setattr(closer_module, "close_pr", fake_close_pr) + return called + + def test_should_stop_at_limit_in_dry_run(self, closer_module, monkeypatch): + called = self._patch_three_closeable_prs(closer_module, monkeypatch) + monkeypatch.setattr(sys, "argv", ["close_low_quality_prs.py", "--limit", "2"]) + rc = closer_module.main() + assert rc == 0 + assert len(called) == 2 + + def test_should_stop_at_limit_when_closing(self, closer_module, monkeypatch): + called = self._patch_three_closeable_prs(closer_module, monkeypatch) + monkeypatch.setattr( + sys, "argv", ["close_low_quality_prs.py", "--limit", "2", "--close"] + ) + rc = closer_module.main() + assert rc == 0 + assert len(called) == 2 + + +class TestFetchOpenPrsLimitWarning: + """`fetch_open_prs` must surface a warning when the gh CLI cap is hit.""" + + def test_should_warn_when_at_cap(self, closer_module, monkeypatch, capsys): + # Pretend `gh pr list --limit 1000` returned exactly 1000 PRs β€” + # this is the silent-truncation case the warning is meant to catch. + cap = closer_module.GH_PR_LIST_LIMIT + synthetic = [{"number": i} for i in range(cap)] + import json as _json + + monkeypatch.setattr( + closer_module, "gh", lambda *a, **kw: _json.dumps(synthetic) + ) + result = closer_module.fetch_open_prs(None) + assert len(result) == cap + captured = capsys.readouterr() + # GitHub Actions `::warning::` annotations go to stderr by + # convention; just check the marker appears somewhere visible. + combined = captured.out + captured.err + assert "::warning::" in combined + assert str(cap) in combined + + def test_should_not_warn_when_under_cap(self, closer_module, monkeypatch, capsys): + synthetic = [{"number": i} for i in range(5)] + import json as _json + + monkeypatch.setattr( + closer_module, "gh", lambda *a, **kw: _json.dumps(synthetic) + ) + result = closer_module.fetch_open_prs(None) + assert len(result) == 5 + captured = capsys.readouterr() + assert "::warning::" not in (captured.out + captured.err) + + +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 diff --git a/tests/test_litellm/test_github_review_gate.py b/tests/test_litellm/test_github_review_gate.py new file mode 100644 index 000000000000..635aec17c977 --- /dev/null +++ b/tests/test_litellm/test_github_review_gate.py @@ -0,0 +1,385 @@ +"""Unit tests for the `ready for review` label lifecycle (Agent Shin review gate). + +Exercises `triage_with_llm.review_gate`, the state machine that keeps the +`ready for review` label in sync with whether a PR clears both the LLM rubric +and Greptile's confidence score: + + * pass (untagged) -> add label + "ready for review" comment + * pass (untagged, recovered) -> add label + "all clear again" comment + * pass (already tagged) -> noop + * regress (tagged) -> remove label + "what's missing" comment, stays open + * fail (untagged, within 24h)-> one-time "what's missing" notice + * fail (untagged, >24h) -> close + comment + * dry run (close=False) -> would-* previews, no side effects +""" + +from __future__ import annotations + +import datetime as dt +import importlib.util +import sys +from pathlib import Path + +import pytest + +SCRIPT_PATH = ( + Path(__file__).resolve().parents[2] / ".github" / "scripts" / "triage_with_llm.py" +) + +NOW = dt.datetime(2026, 5, 24, 12, 0, 0, tzinfo=dt.timezone.utc) +JUST_NOW = "2026-05-24T11:00:00Z" # 1h old -> within 24h grace +TWO_DAYS_AGO = "2026-05-22T11:00:00Z" # >24h old -> past grace + + +@pytest.fixture(scope="module") +def triage_module(): + spec = importlib.util.spec_from_file_location("triage_with_llm", SCRIPT_PATH) + assert spec and spec.loader + module = importlib.util.module_from_spec(spec) + sys.modules["triage_with_llm"] = module + spec.loader.exec_module(module) + return module + + +class _Recorder: + """Captures every gh mutation review_gate could fire, and fails loudly + on the ones a given scenario forbids.""" + + def __init__(self, triage_module, monkeypatch): + self.comments: list[str] = [] + self.added: list[str] = [] + self.removed: list[str] = [] + self.closed: list[int] = [] + monkeypatch.setattr( + triage_module, + "post_comment", + lambda repo, n, body: self.comments.append(body), + ) + monkeypatch.setattr( + triage_module, + "add_label", + lambda repo, n, label: self.added.append(label), + ) + monkeypatch.setattr( + triage_module, + "remove_label", + lambda repo, n, label: self.removed.append(label), + ) + monkeypatch.setattr( + triage_module, + "close_pr", + lambda repo, n: self.closed.append(n), + ) + + +def _make_pr(**overrides): + base = { + "number": 7, + "title": "feat: do a thing", + "body": "some body without a linked issue or QA proof", + "state": "open", + "author_association": "NONE", + "user": {"login": "outside-dev"}, + "labels": [], + "created_at": JUST_NOW, + } + base.update(overrides) + return base + + +def _pass(prompt): + return '{"verdict": "pass", "missing": [], "explanation": "looks good"}' + + +def _fail(prompt): + return ( + '{"verdict": "fail", "missing": ["QA proof", "expected vs. actual"],' + ' "explanation": "thin description"}' + ) + + +def _gate(triage_module, **kwargs): + """Call review_gate with safe defaults for the injectable hooks.""" + params = dict( + repo="o/r", + number=7, + close=True, + model="m", + judge=_pass, + greptile_score=None, + comments=[], + now=NOW, + ) + params.update(kwargs) + return triage_module.review_gate(**params) + + +class TestReviewGatePass: + def test_pass_untagged_adds_label_and_ready_comment( + self, triage_module, monkeypatch + ): + monkeypatch.setattr(triage_module, "fetch_pr", lambda repo, n: _make_pr()) + rec = _Recorder(triage_module, monkeypatch) + + result = _gate(triage_module, judge=_pass, greptile_score=5) + + assert result["action"] == "labeled-ready" + assert rec.added == [triage_module.READY_FOR_REVIEW_LABEL] + assert rec.removed == [] and rec.closed == [] + assert len(rec.comments) == 1 + assert "ready for review" in rec.comments[0].lower() + assert triage_module.READY_MARKER in rec.comments[0] + assert "5/5" in rec.comments[0] + + def test_pass_already_tagged_is_noop(self, triage_module, monkeypatch): + pr = _make_pr(labels=[{"name": "ready for review"}]) + monkeypatch.setattr(triage_module, "fetch_pr", lambda repo, n: pr) + rec = _Recorder(triage_module, monkeypatch) + + result = _gate(triage_module, judge=_pass, greptile_score=5) + + assert result["action"] == "noop-passing" + assert rec.added == [] and rec.removed == [] and rec.comments == [] + + def test_pass_after_prior_regression_uses_all_clear_wording( + self, triage_module, monkeypatch + ): + # A regression marker in history -> this is a recovery, not a first pass. + monkeypatch.setattr(triage_module, "fetch_pr", lambda repo, n: _make_pr()) + rec = _Recorder(triage_module, monkeypatch) + prior = [{"user": {"login": "x"}, "body": triage_module.REGRESSED_MARKER}] + + result = _gate(triage_module, judge=_pass, greptile_score=5, comments=prior) + + assert result["action"] == "labeled-ready" + assert "all clear" in rec.comments[0].lower() + + def test_linked_issue_passes_without_calling_judge( + self, triage_module, monkeypatch + ): + pr = _make_pr(body="Fixes #4321\n\nbody") + monkeypatch.setattr(triage_module, "fetch_pr", lambda repo, n: pr) + rec = _Recorder(triage_module, monkeypatch) + + result = _gate( + triage_module, + judge=lambda p: pytest.fail("LLM must not be called for linked issue"), + greptile_score=5, + ) + assert result["action"] == "labeled-ready" + assert rec.added == [triage_module.READY_FOR_REVIEW_LABEL] + + +class TestReviewGateRegression: + def test_regression_removes_label_and_keeps_pr_open( + self, triage_module, monkeypatch + ): + pr = _make_pr(labels=[{"name": "ready for review"}]) + monkeypatch.setattr(triage_module, "fetch_pr", lambda repo, n: pr) + rec = _Recorder(triage_module, monkeypatch) + + result = _gate(triage_module, judge=_fail, greptile_score=5) + + assert result["action"] == "label-removed-regressed" + assert rec.removed == [triage_module.READY_FOR_REVIEW_LABEL] + assert rec.closed == [] # regression NEVER closes the PR + assert triage_module.REGRESSED_MARKER in rec.comments[0] + assert "QA proof" in rec.comments[0] + + def test_greptile_drop_alone_triggers_regression(self, triage_module, monkeypatch): + # Rubric still passes, but Greptile fell to 2/5 -> not passing. + pr = _make_pr(labels=[{"name": "ready for review"}]) + monkeypatch.setattr(triage_module, "fetch_pr", lambda repo, n: pr) + rec = _Recorder(triage_module, monkeypatch) + + result = _gate(triage_module, judge=_pass, greptile_score=2) + + assert result["action"] == "label-removed-regressed" + assert rec.removed == [triage_module.READY_FOR_REVIEW_LABEL] + assert "2/5" in rec.comments[0] + + def test_greptile_score_read_from_comments_when_not_injected( + self, triage_module, monkeypatch + ): + pr = _make_pr(labels=[{"name": "ready for review"}]) + monkeypatch.setattr(triage_module, "fetch_pr", lambda repo, n: pr) + rec = _Recorder(triage_module, monkeypatch) + greptile = [ + { + "user": {"login": "greptile-apps[bot]"}, + "body": "Confidence Score: 2/5", + "created_at": "2026-05-24T10:00:00Z", + } + ] + + result = _gate( + triage_module, + judge=_pass, + greptile_score=triage_module._UNSET, + comments=greptile, + ) + assert result["action"] == "label-removed-regressed" + assert "2/5" in rec.comments[0] + + +class TestReviewGateGraceAndClose: + def test_within_grace_posts_one_time_notice(self, triage_module, monkeypatch): + monkeypatch.setattr( + triage_module, "fetch_pr", lambda repo, n: _make_pr(created_at=JUST_NOW) + ) + rec = _Recorder(triage_module, monkeypatch) + + result = _gate(triage_module, judge=_fail, greptile_score=None) + + assert result["action"] == "within-grace-notified" + assert rec.closed == [] and rec.added == [] and rec.removed == [] + assert triage_module.WITHIN_GRACE_MARKER in rec.comments[0] + assert "QA proof" in rec.comments[0] + + def test_within_grace_does_not_double_notify(self, triage_module, monkeypatch): + monkeypatch.setattr( + triage_module, "fetch_pr", lambda repo, n: _make_pr(created_at=JUST_NOW) + ) + rec = _Recorder(triage_module, monkeypatch) + prior = [{"user": {"login": "b"}, "body": triage_module.WITHIN_GRACE_MARKER}] + + result = _gate(triage_module, judge=_fail, greptile_score=None, comments=prior) + + assert result["action"] == "within-grace-already-notified" + assert rec.comments == [] + + def test_past_grace_closes_with_comment(self, triage_module, monkeypatch): + monkeypatch.setattr( + triage_module, + "fetch_pr", + lambda repo, n: _make_pr(created_at=TWO_DAYS_AGO), + ) + rec = _Recorder(triage_module, monkeypatch) + + result = _gate(triage_module, judge=_fail, greptile_score=None) + + assert result["action"] == "closed" + assert rec.closed == [7] + assert len(rec.comments) == 1 + # The close comment must carry the reconsider provenance marker. + assert triage_module.AGENT_SHIN_AUTO_CLOSE_MARKER in rec.comments[0] + + +class TestReviewGateDryRun: + @pytest.mark.parametrize( + "scenario,labels,judge,score,created,expected", + [ + ("pass", [], _pass, 5, JUST_NOW, "would-label-ready"), + ( + "regress", + [{"name": "ready for review"}], + _fail, + 5, + JUST_NOW, + "would-remove-label", + ), + ("within-grace", [], _fail, None, JUST_NOW, "would-notify-within-grace"), + ("past-grace", [], _fail, None, TWO_DAYS_AGO, "would-close"), + ], + ) + def test_dry_run_previews_without_side_effects( + self, + triage_module, + monkeypatch, + scenario, + labels, + judge, + score, + created, + expected, + ): + pr = _make_pr(labels=labels, created_at=created) + monkeypatch.setattr(triage_module, "fetch_pr", lambda repo, n: pr) + rec = _Recorder(triage_module, monkeypatch) + + result = _gate(triage_module, close=False, judge=judge, greptile_score=score) + + assert result["action"] == expected + # Dry run touches nothing. + assert rec.added == [] and rec.removed == [] and rec.closed == [] + assert rec.comments == [] + assert "comment" in result # preview body still surfaced + + +class TestReviewGateGuards: + def test_skips_internal_author(self, triage_module, monkeypatch): + pr = _make_pr(author_association="MEMBER", user={"login": "krrish"}) + monkeypatch.setattr(triage_module, "fetch_pr", lambda repo, n: pr) + result = _gate( + triage_module, judge=lambda p: pytest.fail("no LLM for internal") + ) + assert result["action"] == "skip-internal-author" + + def test_skips_closed_pr(self, triage_module, monkeypatch): + pr = _make_pr(state="closed") + monkeypatch.setattr(triage_module, "fetch_pr", lambda repo, n: pr) + result = _gate(triage_module, judge=lambda p: pytest.fail("no LLM for closed")) + assert result["action"] == "skip-not-open" + + def test_llm_error_is_non_destructive(self, triage_module, monkeypatch): + monkeypatch.setattr(triage_module, "fetch_pr", lambda repo, n: _make_pr()) + rec = _Recorder(triage_module, monkeypatch) + + def boom(prompt): + raise RuntimeError("api down") + + result = _gate(triage_module, judge=boom, greptile_score=None) + + assert result["action"] == "skip-llm-error" + assert rec.closed == [] and rec.added == [] and rec.removed == [] + + def test_full_recovery_cycle(self, triage_module, monkeypatch): + """pass -> regress -> recover, threading labels/comments like GitHub would.""" + state = {"labels": [], "comments": []} + + def fake_fetch(repo, n): + return _make_pr(labels=list(state["labels"]), created_at=JUST_NOW) + + monkeypatch.setattr(triage_module, "fetch_pr", fake_fetch) + monkeypatch.setattr( + triage_module, + "post_comment", + lambda repo, n, body: state["comments"].append( + {"user": {"login": "agent-shin[bot]"}, "body": body} + ), + ) + monkeypatch.setattr( + triage_module, + "add_label", + lambda repo, n, label: state["labels"].append({"name": label}), + ) + monkeypatch.setattr( + triage_module, + "remove_label", + lambda repo, n, label: state["labels"].clear(), + ) + monkeypatch.setattr( + triage_module, "close_pr", lambda repo, n: pytest.fail("must not close") + ) + + # 1) passes -> tagged + r1 = _gate( + triage_module, judge=_pass, greptile_score=5, comments=state["comments"] + ) + assert r1["action"] == "labeled-ready" + assert any(lbl["name"] == "ready for review" for lbl in state["labels"]) + + # 2) regresses -> tag removed, comment posted, PR still open + r2 = _gate( + triage_module, judge=_fail, greptile_score=2, comments=state["comments"] + ) + assert r2["action"] == "label-removed-regressed" + assert state["labels"] == [] + + # 3) fixed again -> "all clear" + tag back + r3 = _gate( + triage_module, judge=_pass, greptile_score=5, comments=state["comments"] + ) + assert r3["action"] == "labeled-ready" + assert any(lbl["name"] == "ready for review" for lbl in state["labels"]) + assert "all clear" in state["comments"][-1]["body"].lower() 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 000000000000..1776880e1e9f --- /dev/null +++ b/tests/test_litellm/test_github_triage_with_llm.py @@ -0,0 +1,1379 @@ +"""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( + "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"], + ) + 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", + "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): + assert triage_module.has_linked_issue(body) is True + + @pytest.mark.parametrize( + "body", + [ + "", + "Some change", + # 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_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 = "" + 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 TestCloseCommentText: + """Pin the user-facing language in close comments so changes are intentional.""" + + def test_pr_close_comment_should_recommend_new_pr_primarily(self, triage_module): + body = triage_module.format_pr_close_comment( + {"verdict": "fail", "missing": ["QA proof"], "explanation": "thin"} + ) + # Primary path: open a new PR (because OSS authors can't reopen a + # bot-closed PR). Secondary path: `@agent-shin reconsider`. + assert "Open a new PR" in body + assert "@agent-shin reconsider" in body + # Old advice that no longer works for OSS contributors must NOT + # appear (they can't reopen a PR closed by a bot/maintainer). + assert "Reopen the PR" not in body + + def test_pr_close_comment_should_not_promise_automatic_reopen_on_open( + self, triage_module + ): + # The previous comment said "I'll re-evaluate automatically" β€” that + # only worked because the author could reopen, which they often + # can't. The new wording must point them at the comment trigger or + # a new PR instead. + body = triage_module.format_pr_close_comment( + {"verdict": "fail", "missing": [], "explanation": ""} + ) + assert "I'll re-evaluate automatically" not in body + + def test_issue_close_comment_should_use_reconsider_trigger(self, triage_module): + body = triage_module.format_issue_close_comment( + {"verdict": "fail", "missing": ["repro"], "explanation": "thin"} + ) + assert "@agent-shin reconsider" in body + assert "Reopen the issue" not in body + + +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 + + def test_should_not_crash_when_pr_body_contains_curly_braces(self, triage_module): + """User-supplied content with `{` / `}` must NOT be re-parsed by + `str.format()`. `format` only scans the template literal for + replacement fields; values being substituted in are inserted as + plain strings, so a body like `{"foo": "bar"}` or `{unmatched` + cannot blow up the script. Pinning this here so a future + "improvement" to the templating doesn't reintroduce a crash on + every PR that quotes JSON. + """ + for body in ( + 'Here is some JSON: {"foo": "bar", "n": 1}', + "Half a brace { left dangling, and a stray }", + "Format-spec-looking thing: {0}, {name:>10}, {!r}", + "Nested {a: {b: c}} braces", + ): + pr_prompt = triage_module.build_pr_prompt(title="t", body=body) + issue_prompt = triage_module.build_issue_prompt(title="t", body=body) + assert body in pr_prompt + assert body in issue_prompt + + def test_should_not_crash_when_pr_title_contains_curly_braces(self, triage_module): + title = "Fix bug in {0:>10} format-spec handling" + pr_prompt = triage_module.build_pr_prompt(title=title, body="x") + issue_prompt = triage_module.build_issue_prompt(title=title, body="x") + assert title in pr_prompt + assert title in issue_prompt + + def test_should_preserve_template_indentation_with_multiline_body( + self, triage_module + ): + """`textwrap.dedent` runs on the static template *before* user + content is interpolated, so a multi-line body (whose 2nd+ lines + start at column 0) cannot defeat the common-indent computation + and leave 8-space indentation on every template line. Pin the + dedented shape so the rendered prompt stays consistent for the + LLM judge. + """ + body = "first line\nsecond line at column 0\nthird line at column 0" + for builder in ( + triage_module.build_pr_prompt, + triage_module.build_issue_prompt, + ): + prompt = builder(title="t", body=body) + # Template lines should NOT carry the 8 leading spaces from + # the source-file indentation of the triple-quoted string. + assert " You are " not in prompt + assert 'You are "Agent Shin"' in prompt + assert body 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 TestWasAutoClosedByAgentShin: + """Provenance check that gates reconsider's reopen path.""" + + @staticmethod + def _install(monkeypatch, triage_module, events, comments): + monkeypatch.setattr(triage_module, "fetch_issue_events", lambda repo, n: events) + monkeypatch.setattr( + triage_module, "fetch_issue_comments", lambda repo, n: comments + ) + + def test_should_return_true_when_latest_close_was_bot_with_marker( + self, triage_module, monkeypatch + ): + events = [ + { + "event": "closed", + "actor": {"login": "github-actions[bot]"}, + "created_at": "2025-01-01T00:00:10Z", + } + ] + comments = [ + { + "user": {"login": "github-actions[bot]"}, + "created_at": "2025-01-01T00:00:05Z", + "body": ( + "πŸ‘‹ Hi, thanks for the PR! I'm **Agent Shin**, the automated " + "triage bot for this repository.\n\nThis PR is being **auto-closed**..." + ), + } + ] + self._install(monkeypatch, triage_module, events, comments) + assert triage_module.was_auto_closed_by_agent_shin("o/r", 1) is True + + def test_should_return_false_when_no_close_event(self, triage_module, monkeypatch): + self._install(monkeypatch, triage_module, [], []) + assert triage_module.was_auto_closed_by_agent_shin("o/r", 1) is False + + def test_should_ignore_non_bot_author_with_marker(self, triage_module, monkeypatch): + # A contributor pasting the marker into a manual comment must NOT + # be treated as proof Agent Shin closed the PR. Even if a bot did + # the most recent close, the marker comment must be authored by + # that same bot login β€” not by the human. + events = [ + { + "event": "closed", + "actor": {"login": "github-actions[bot]"}, + "created_at": "2025-01-01T00:00:10Z", + } + ] + comments = [ + { + "user": {"login": "outside-dev"}, + "created_at": "2025-01-01T00:00:05Z", + "body": "I'm **Agent Shin**, just kidding β€” please reconsider this.", + } + ] + self._install(monkeypatch, triage_module, events, comments) + assert triage_module.was_auto_closed_by_agent_shin("o/r", 1) is False + + def test_should_ignore_bot_comment_without_marker(self, triage_module, monkeypatch): + # Other bots (codecov, cla-assistant, etc.) post on every PR; their + # presence must not satisfy the provenance check. + events = [ + { + "event": "closed", + "actor": {"login": "github-actions[bot]"}, + "created_at": "2025-01-01T00:00:10Z", + } + ] + comments = [ + { + "user": {"login": "codecov[bot]"}, + "created_at": "2025-01-01T00:00:01Z", + "body": "## Codecov Report ...", + }, + { + "user": {"login": "greptile-apps[bot]"}, + "created_at": "2025-01-01T00:00:02Z", + "body": "Confidence Score: 2/5", + }, + ] + self._install(monkeypatch, triage_module, events, comments) + assert triage_module.was_auto_closed_by_agent_shin("o/r", 1) is False + + def test_should_anchor_on_most_recent_close_event(self, triage_module, monkeypatch): + # Agent Shin auto-closed first, contributor commented after; the + # bot-authored marker comment is anywhere in the timeline. + events = [ + { + "event": "labeled", + "actor": {"login": "krrishdholakia"}, + "created_at": "2025-01-01T00:00:01Z", + }, + { + "event": "closed", + "actor": {"login": "github-actions[bot]"}, + "created_at": "2025-01-01T00:00:10Z", + }, + ] + comments = [ + { + "user": {"login": "codecov[bot]"}, + "created_at": "2025-01-01T00:00:02Z", + "body": "## Codecov Report", + }, + { + "user": {"login": "github-actions[bot]"}, + "created_at": "2025-01-01T00:00:05Z", + "body": "I'm **Agent Shin**, the automated triage bot ...", + }, + { + "user": {"login": "outside-dev"}, + "created_at": "2025-01-01T00:00:15Z", + "body": "Replying after auto-close ...", + }, + ] + self._install(monkeypatch, triage_module, events, comments) + assert triage_module.was_auto_closed_by_agent_shin("o/r", 1) is True + + def test_should_refuse_when_maintainer_re_closed_after_agent_shin( + self, triage_module, monkeypatch + ): + # Agent Shin auto-closed, the PR was reopened, then a maintainer + # closed it again (e.g. as a duplicate). `@agent-shin reconsider` + # must NOT override the maintainer's later closure even though the + # historical Agent Shin marker comment still exists. + events = [ + { + "event": "closed", + "actor": {"login": "github-actions[bot]"}, + "created_at": "2025-01-01T00:00:10Z", + }, + { + "event": "reopened", + "actor": {"login": "github-actions[bot]"}, + "created_at": "2025-01-01T00:00:20Z", + }, + { + "event": "closed", + "actor": {"login": "krrishdholakia"}, + "created_at": "2025-01-01T00:00:30Z", + }, + ] + comments = [ + { + "user": {"login": "github-actions[bot]"}, + "created_at": "2025-01-01T00:00:05Z", + "body": "I'm **Agent Shin**, the automated triage bot ...", + } + ] + self._install(monkeypatch, triage_module, events, comments) + assert triage_module.was_auto_closed_by_agent_shin("o/r", 1) is False + + def test_should_refuse_when_unrelated_bot_re_closed_after_agent_shin( + self, triage_module, monkeypatch + ): + # Agent Shin closed, the PR was reopened, then a different bot + # (stale, etc.) closed it. The marker comment is from + # `github-actions[bot]` but the most recent closer is + # `stale[bot]`, so the logins don't match -> refuse to reopen. + events = [ + { + "event": "closed", + "actor": {"login": "github-actions[bot]"}, + "created_at": "2025-01-01T00:00:10Z", + }, + { + "event": "reopened", + "actor": {"login": "github-actions[bot]"}, + "created_at": "2025-01-01T00:00:20Z", + }, + { + "event": "closed", + "actor": {"login": "stale[bot]"}, + "created_at": "2025-01-01T00:00:30Z", + }, + ] + comments = [ + { + "user": {"login": "github-actions[bot]"}, + "created_at": "2025-01-01T00:00:05Z", + "body": "I'm **Agent Shin**, the automated triage bot ...", + } + ] + self._install(monkeypatch, triage_module, events, comments) + assert triage_module.was_auto_closed_by_agent_shin("o/r", 1) is False + + def test_should_refuse_when_stale_workflow_re_closed_after_agent_shin( + self, triage_module, monkeypatch + ): + # The repo's `actions/stale` workflow uses `secrets.GITHUB_TOKEN`, + # so its closes are attributed to `github-actions[bot]` β€” the same + # login as Agent Shin. A historical Agent Shin marker comment + # from an earlier close cycle must NOT satisfy the provenance check + # for a later stale-initiated close (which never posts that marker). + events = [ + { + "event": "closed", + "actor": {"login": "github-actions[bot]"}, + "created_at": "2025-01-01T00:00:10Z", + }, + { + "event": "reopened", + "actor": {"login": "github-actions[bot]"}, + "created_at": "2025-01-01T00:00:20Z", + }, + { + "event": "closed", + "actor": {"login": "github-actions[bot]"}, + "created_at": "2025-04-01T00:00:00Z", + }, + ] + comments = [ + { + "user": {"login": "github-actions[bot]"}, + "created_at": "2025-01-01T00:00:05Z", + "body": "I'm **Agent Shin**, the automated triage bot ...", + }, + { + "user": {"login": "github-actions[bot]"}, + "created_at": "2025-03-25T00:00:00Z", + "body": "This pull request has been automatically marked as stale...", + }, + ] + self._install(monkeypatch, triage_module, events, comments) + assert triage_module.was_auto_closed_by_agent_shin("o/r", 1) is False + + def test_should_accept_marker_from_current_cycle_after_reopen( + self, triage_module, monkeypatch + ): + # Two clean Agent Shin cycles: closed, reconsiderβ†’reopened, closed + # again with a new marker comment posted in the current cycle. + # The second-cycle marker is what proves provenance. + events = [ + { + "event": "closed", + "actor": {"login": "github-actions[bot]"}, + "created_at": "2025-01-01T00:00:10Z", + }, + { + "event": "reopened", + "actor": {"login": "github-actions[bot]"}, + "created_at": "2025-01-01T00:00:20Z", + }, + { + "event": "closed", + "actor": {"login": "github-actions[bot]"}, + "created_at": "2025-01-01T00:00:40Z", + }, + ] + comments = [ + { + "user": {"login": "github-actions[bot]"}, + "created_at": "2025-01-01T00:00:05Z", + "body": "I'm **Agent Shin**, the automated triage bot ...", + }, + { + "user": {"login": "github-actions[bot]"}, + "created_at": "2025-01-01T00:00:35Z", + "body": "I'm **Agent Shin** β€” still missing X ...", + }, + ] + self._install(monkeypatch, triage_module, events, comments) + assert triage_module.was_auto_closed_by_agent_shin("o/r", 1) is True + + +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.""" + + 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_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) + 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_skip_open_pr_in_reconsider_mode(self, triage_module, monkeypatch): + # Reconsider only makes sense on a CLOSED PR β€” running it on an open + # one is a no-op (the regular triage flow already evaluated it). + pr = self._make_pr(state="open") + monkeypatch.setattr(triage_module, "fetch_pr", lambda repo, n: pr) + result = triage_module.triage( + repo="o/r", + kind="pr", + number=1, + close=False, + model="m", + judge=lambda p: pytest.fail("should not run on open PR in reconsider"), + reconsider=True, + ) + assert result["action"] == "skip-not-closed" + + 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. + pr = self._make_pr( + state="closed", body="Updated body with QA proof + screenshots." + ) + monkeypatch.setattr(triage_module, "fetch_pr", lambda repo, n: pr) + # Provenance: the PR was auto-closed by Agent Shin (a bot-authored + # auto-close comment exists), so reconsider is allowed to reopen. + monkeypatch.setattr( + triage_module, "was_auto_closed_by_agent_shin", lambda repo, n: True + ) + 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}), + ) + # close_pr / close_issue MUST NOT fire in reconsider mode. + monkeypatch.setattr( + triage_module, + "close_pr", + lambda *a, **kw: pytest.fail("must not close on reconsider pass"), + ) + + result = triage_module.triage( + repo="o/r", + kind="pr", + number=42, + close=True, + model="m", + judge=lambda p: json.dumps( + {"verdict": "pass", "missing": [], "explanation": "ok now"} + ), + reconsider=True, + ) + assert result["action"] == "reopened" + assert reopened["n"] == 42 + assert posted["n"] == 42 + assert "reopened" in posted["body"].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) + monkeypatch.setattr( + triage_module, "was_auto_closed_by_agent_shin", lambda repo, n: True + ) + posted = {} + monkeypatch.setattr( + triage_module, + "post_comment", + lambda repo, n, body: posted.update({"n": n, "body": body}), + ) + # Neither reopen nor close should fire when reconsider verdict is fail. + monkeypatch.setattr( + triage_module, + "reopen_pr", + lambda *a, **kw: pytest.fail("must not reopen on fail"), + ) + monkeypatch.setattr( + triage_module, + "close_pr", + lambda *a, **kw: pytest.fail("must not close again on reconsider fail"), + ) + + verdict = { + "verdict": "fail", + "missing": ["QA proof"], + "explanation": "Still no QA proof.", + } + result = triage_module.triage( + repo="o/r", + kind="pr", + number=42, + close=True, + model="m", + judge=lambda p: json.dumps(verdict), + reconsider=True, + ) + assert result["action"] == "reconsider-still-failing" + assert posted["n"] == 42 + assert "QA proof" in posted["body"] + + def test_should_reopen_on_reconsider_with_linked_issue_short_circuit( + self, triage_module, monkeypatch + ): + # The linked-issue short-circuit also has to honor reconsider mode: + # if the contributor edited the body to add `Fixes #1234`, the regex + # 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) + monkeypatch.setattr( + triage_module, "was_auto_closed_by_agent_shin", lambda repo, n: True + ) + posted = {} + reopened = {} + monkeypatch.setattr( + triage_module, + "post_comment", + lambda repo, n, body: posted.update({"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=55, + close=True, + model="m", + judge=lambda p: pytest.fail("LLM must not run when linked-issue matches"), + reconsider=True, + ) + assert result["action"] == "reopened" + assert reopened["n"] == 55 + assert "reopened" in posted["body"].lower() + + 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 + # automatically, in case a maintainer closed it intentionally. + pr = self._make_pr( + state="closed", + author_association="MEMBER", + user={"login": "krrishdholakia"}, + ) + monkeypatch.setattr(triage_module, "fetch_pr", lambda repo, n: pr) + monkeypatch.setattr( + triage_module, + "reopen_pr", + lambda *a, **kw: pytest.fail("must not reopen for internal author"), + ) + # Internal check must fire *before* provenance, so the provenance + # helper should never be invoked for an internal author. + monkeypatch.setattr( + triage_module, + "was_auto_closed_by_agent_shin", + lambda *a, **kw: pytest.fail("must not check provenance for internal"), + ) + result = triage_module.triage( + repo="o/r", + kind="pr", + number=1, + close=True, + model="m", + judge=lambda p: pytest.fail("LLM must not run for internal author"), + reconsider=True, + ) + assert result["action"] == "skip-internal-author" + + def test_should_skip_reconsider_when_not_bot_closed( + self, triage_module, monkeypatch + ): + # A maintainer-closed PR (no Agent Shin auto-close comment) must + # never be reopened by `@agent-shin reconsider`, regardless of how + # good the LLM verdict would be. Otherwise the original author + # could polish the description and silently override a maintainer's + # "closed as duplicate / out of scope" decision. + pr = self._make_pr(state="closed", body="Fixes #1234") + monkeypatch.setattr(triage_module, "fetch_pr", lambda repo, n: pr) + monkeypatch.setattr( + triage_module, "was_auto_closed_by_agent_shin", lambda repo, n: False + ) + monkeypatch.setattr( + triage_module, + "post_comment", + lambda *a, **kw: pytest.fail("must not comment when not bot-closed"), + ) + monkeypatch.setattr( + triage_module, + "reopen_pr", + lambda *a, **kw: pytest.fail("must not reopen when not bot-closed"), + ) + result = triage_module.triage( + repo="o/r", + kind="pr", + number=1, + close=True, + model="m", + judge=lambda p: pytest.fail("LLM must not run when not bot-closed"), + reconsider=True, + ) + assert result["action"] == "skip-not-bot-closed" + + def test_should_skip_reconsider_issue_when_not_bot_closed( + self, triage_module, monkeypatch + ): + # Same provenance gate for issues: only Agent Shin auto-closed + # issues are eligible for reopen-on-reconsider. + issue = { + "number": 7, + "title": "Bug", + "body": "Repro: curl ...", + "state": "closed", + "author_association": "NONE", + "user": {"login": "outside"}, + } + monkeypatch.setattr(triage_module, "fetch_issue", lambda repo, n: issue) + monkeypatch.setattr( + triage_module, "was_auto_closed_by_agent_shin", lambda repo, n: False + ) + monkeypatch.setattr( + triage_module, + "reopen_issue", + lambda *a, **kw: pytest.fail("must not reopen maintainer-closed issue"), + ) + result = triage_module.triage( + repo="o/r", + kind="issue", + number=7, + close=True, + model="m", + judge=lambda p: pytest.fail("LLM must not run for non-bot-closed issue"), + reconsider=True, + ) + assert result["action"] == "skip-not-bot-closed" + + def test_should_preview_reopen_in_reconsider_dry_run( + self, triage_module, monkeypatch + ): + # When `close=False` and `reconsider=True`, a passing verdict must + # produce a `would-reopen` preview WITHOUT posting a comment or + # reopening β€” same dry-run pattern as `would-close` in regular mode. + pr = self._make_pr(state="closed", body="Now with screenshots + repro.") + monkeypatch.setattr(triage_module, "fetch_pr", lambda repo, n: pr) + monkeypatch.setattr( + triage_module, "was_auto_closed_by_agent_shin", lambda repo, n: True + ) + monkeypatch.setattr( + triage_module, + "post_comment", + lambda *a, **kw: pytest.fail("dry-run must not post"), + ) + monkeypatch.setattr( + triage_module, + "reopen_pr", + lambda *a, **kw: pytest.fail("dry-run must not reopen"), + ) + 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 preview should include the comment body the bot WOULD post + # (useful for $GITHUB_STEP_SUMMARY). + assert "reopened" in result["comment"].lower() + + def test_should_preview_reopen_in_reconsider_dry_run_linked_issue( + self, triage_module, monkeypatch + ): + # The linked-issue short-circuit also has to honor dry-run in + # reconsider mode β€” no LLM call AND no destructive side effects. + pr = self._make_pr(state="closed", body="Fixes #1234\n\nDetails.") + monkeypatch.setattr(triage_module, "fetch_pr", lambda repo, n: pr) + monkeypatch.setattr( + triage_module, "was_auto_closed_by_agent_shin", lambda repo, n: True + ) + monkeypatch.setattr( + triage_module, + "post_comment", + lambda *a, **kw: pytest.fail("dry-run must not post"), + ) + monkeypatch.setattr( + triage_module, + "reopen_pr", + lambda *a, **kw: pytest.fail("dry-run must not reopen"), + ) + 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" + assert "reopened" in result["comment"].lower() + + def test_should_preview_still_failing_in_reconsider_dry_run( + self, triage_module, monkeypatch + ): + # When `close=False` and the verdict is fail, the dry-run preview + # must say `would-leave-closed-still-failing` and not post the + # "still failing" comment. + pr = self._make_pr(state="closed", body="still thin") + monkeypatch.setattr(triage_module, "fetch_pr", lambda repo, n: pr) + monkeypatch.setattr( + triage_module, "was_auto_closed_by_agent_shin", lambda repo, n: True + ) + monkeypatch.setattr( + triage_module, + "post_comment", + lambda *a, **kw: pytest.fail("dry-run must not post"), + ) + + 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-leave-closed-still-failing" + assert "QA proof" in result["comment"] + + def test_should_reopen_issue_on_reconsider_pass(self, triage_module, monkeypatch): + issue = { + "number": 7, + "title": "Bug: now with repro", + "body": "## Repro\n```bash\ncurl ...\n```\n\nExpected X, got Y.", + "state": "closed", + "author_association": "NONE", + "user": {"login": "outside"}, + } + monkeypatch.setattr(triage_module, "fetch_issue", lambda repo, n: issue) + monkeypatch.setattr( + triage_module, "was_auto_closed_by_agent_shin", lambda repo, n: True + ) + posted = {} + reopened = {} + monkeypatch.setattr( + triage_module, + "post_comment", + lambda repo, n, body: posted.update({"body": body}), + ) + monkeypatch.setattr( + triage_module, + "reopen_issue", + lambda repo, n: reopened.update({"n": n}), + ) + + result = triage_module.triage( + repo="o/r", + kind="issue", + number=7, + close=True, + model="m", + judge=lambda p: json.dumps( + {"verdict": "pass", "missing": [], "explanation": "now reproducible"} + ), + reconsider=True, + ) + assert result["action"] == "reopened" + assert reopened["n"] == 7 + assert "reopened" in posted["body"].lower() + + def test_should_reopen_before_posting_on_reconsider_pass( + self, triage_module, monkeypatch + ): + # A failed reopen call must not leave a misleading "we reopened it" + # comment on a still-closed PR. Pin the order: reopen happens first; + # if reopen raises, post_comment must not run. + pr = self._make_pr(state="closed", body="Updated body with QA proof.") + monkeypatch.setattr(triage_module, "fetch_pr", lambda repo, n: pr) + monkeypatch.setattr( + triage_module, "was_auto_closed_by_agent_shin", lambda repo, n: True + ) + monkeypatch.setattr( + triage_module, + "post_comment", + lambda *a, **kw: pytest.fail("must not post comment if reopen fails"), + ) + + def boom(*a, **kw): + raise RuntimeError("reopen 422") + + monkeypatch.setattr(triage_module, "reopen_pr", boom) + with pytest.raises(RuntimeError): + triage_module.triage( + repo="o/r", + kind="pr", + number=42, + close=True, + model="m", + judge=lambda p: json.dumps( + {"verdict": "pass", "missing": [], "explanation": "ok"} + ), + reconsider=True, + ) + + def test_should_reopen_before_posting_on_reconsider_linked_issue( + self, triage_module, monkeypatch + ): + # Same ordering invariant for the linked-issue short-circuit. + pr = self._make_pr(state="closed", body="Fixes #1234") + monkeypatch.setattr(triage_module, "fetch_pr", lambda repo, n: pr) + monkeypatch.setattr( + triage_module, "was_auto_closed_by_agent_shin", lambda repo, n: True + ) + monkeypatch.setattr( + triage_module, + "post_comment", + lambda *a, **kw: pytest.fail("must not post comment if reopen fails"), + ) + + def boom(*a, **kw): + raise RuntimeError("reopen 422") + + monkeypatch.setattr(triage_module, "reopen_pr", boom) + with pytest.raises(RuntimeError): + triage_module.triage( + repo="o/r", + kind="pr", + number=55, + close=True, + model="m", + judge=lambda p: pytest.fail("LLM must not run for linked-issue path"), + reconsider=True, + ) + + def test_should_reopen_before_posting_on_reconsider_issue_pass( + self, triage_module, monkeypatch + ): + # Same ordering invariant for issues (reopen_issue, not reopen_pr). + issue = { + "number": 7, + "title": "Bug", + "body": "Repro: curl ...", + "state": "closed", + "author_association": "NONE", + "user": {"login": "outside"}, + } + monkeypatch.setattr(triage_module, "fetch_issue", lambda repo, n: issue) + monkeypatch.setattr( + triage_module, "was_auto_closed_by_agent_shin", lambda repo, n: True + ) + monkeypatch.setattr( + triage_module, + "post_comment", + lambda *a, **kw: pytest.fail("must not post comment if reopen fails"), + ) + + def boom(*a, **kw): + raise RuntimeError("reopen 422") + + monkeypatch.setattr(triage_module, "reopen_issue", boom) + with pytest.raises(RuntimeError): + triage_module.triage( + repo="o/r", + kind="issue", + number=7, + close=True, + model="m", + judge=lambda p: json.dumps( + {"verdict": "pass", "missing": [], "explanation": "ok"} + ), + reconsider=True, + ) + + 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"] 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 000000000000..0b01a9e3a92b --- /dev/null +++ b/tests/test_litellm/test_github_triage_workflows.py @@ -0,0 +1,140 @@ +"""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", + "review_gate.yml": "CLOSE_FLAG", + # The reconsider workflow has no per-run "really do it?" knob β€” its + # only kill switch is `AGENT_SHIN_ENABLED`, which already serves as + # both the destructive gate and the global enablement gate. + "triage_reconsider.yml": "AGENT_SHIN_ENABLED", +} + + +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." + ) diff --git a/ui/litellm-dashboard/src/components/provider_info_helpers.test.tsx b/ui/litellm-dashboard/src/components/provider_info_helpers.test.tsx index fa014de4e62b..ab22b0ef49ae 100644 --- a/ui/litellm-dashboard/src/components/provider_info_helpers.test.tsx +++ b/ui/litellm-dashboard/src/components/provider_info_helpers.test.tsx @@ -218,14 +218,83 @@ describe("provider_info_helpers", () => { expect(result).toEqual(["gpt-3.5-turbo", "gpt-4"]); }); - it("should return models when litellm_provider includes the provider string", () => { + it("should return models whose litellm_provider is a prefix-anchored variant of the provider", () => { const modelMap = { - "custom-openai-model": { litellm_provider: "custom_openai_endpoint" }, - "another-model": { litellm_provider: "openai" }, + "anthropic-text-model": { litellm_provider: "anthropic_text" }, + "claude-3-opus": { litellm_provider: "anthropic" }, + }; + const result = getProviderModels(Providers.Anthropic, modelMap); + expect(result).toContain("anthropic-text-model"); + expect(result).toContain("claude-3-opus"); + }); + + it("should not leak vertex_ai-anthropic_models into the Anthropic provider", () => { + const modelMap = { + "claude-3-opus": { litellm_provider: "anthropic" }, + "vertex_ai/claude-3-5-sonnet": { litellm_provider: "vertex_ai-anthropic_models" }, + "vertex_ai/claude-haiku-4-5": { litellm_provider: "vertex_ai-anthropic_models" }, + }; + const result = getProviderModels(Providers.Anthropic, modelMap); + expect(result).toEqual(["claude-3-opus"]); + expect(result).not.toContain("vertex_ai/claude-3-5-sonnet"); + expect(result).not.toContain("vertex_ai/claude-haiku-4-5"); + }); + + it("should not leak vertex_ai-openai_models into the OpenAI provider", () => { + const modelMap = { + "gpt-4": { litellm_provider: "openai" }, + "vertex_ai/openai-something": { litellm_provider: "vertex_ai-openai_models" }, }; const result = getProviderModels(Providers.OpenAI, modelMap); - expect(result).toContain("custom-openai-model"); - expect(result).toContain("another-model"); + expect(result).toEqual(["gpt-4"]); + expect(result).not.toContain("vertex_ai/openai-something"); + }); + + // Note on the next three tests: in production, AddModelForm passes the + // backend `provider` field (the provider_map *key*, e.g. "Vertex_AI", + // "Bedrock", "FireworksAI") into getProviderModels, not the Providers + // enum value. The `as Providers` cast in callers is misleading. We mirror + // the production shape here by passing the key directly. + it("should include all vertex_ai variants when called with 'Vertex_AI' provider key", () => { + const modelMap = { + "vertex_ai/gemini-pro": { litellm_provider: "vertex_ai" }, + "vertex_ai/claude-3-5-sonnet": { litellm_provider: "vertex_ai-anthropic_models" }, + "vertex_ai/text-bison": { litellm_provider: "vertex_ai-text-models" }, + "vertex_ai_beta/something": { litellm_provider: "vertex_ai_beta" }, + "anthropic-native": { litellm_provider: "anthropic" }, + }; + const result = getProviderModels("Vertex_AI" as Providers, modelMap); + expect(result).toContain("vertex_ai/gemini-pro"); + expect(result).toContain("vertex_ai/claude-3-5-sonnet"); + expect(result).toContain("vertex_ai/text-bison"); + expect(result).toContain("vertex_ai_beta/something"); + expect(result).not.toContain("anthropic-native"); + }); + + it("should include bedrock variants (converse, mantle) when called with 'Bedrock' provider key", () => { + const modelMap = { + "bedrock-base": { litellm_provider: "bedrock" }, + "bedrock-converse-model": { litellm_provider: "bedrock_converse" }, + "bedrock-mantle-model": { litellm_provider: "bedrock_mantle" }, + "openai-model": { litellm_provider: "openai" }, + }; + const result = getProviderModels("Bedrock" as Providers, modelMap); + expect(result).toContain("bedrock-base"); + expect(result).toContain("bedrock-converse-model"); + expect(result).toContain("bedrock-mantle-model"); + expect(result).not.toContain("openai-model"); + }); + + it("should include fireworks_ai-embedding-models when called with 'FireworksAI' provider key", () => { + const modelMap = { + "fireworks-base": { litellm_provider: "fireworks_ai" }, + "fireworks-embed": { litellm_provider: "fireworks_ai-embedding-models" }, + "openai-model": { litellm_provider: "openai" }, + }; + const result = getProviderModels("FireworksAI" as Providers, modelMap); + expect(result).toContain("fireworks-base"); + expect(result).toContain("fireworks-embed"); + expect(result).not.toContain("openai-model"); }); it("should filter out models with null values", () => { diff --git a/ui/litellm-dashboard/src/components/provider_info_helpers.tsx b/ui/litellm-dashboard/src/components/provider_info_helpers.tsx index 62c0633d1177..105951114cab 100644 --- a/ui/litellm-dashboard/src/components/provider_info_helpers.tsx +++ b/ui/litellm-dashboard/src/components/provider_info_helpers.tsx @@ -389,7 +389,9 @@ export const getProviderModels = (provider: Providers, modelMap: any): Array