mirror of
https://github.com/BerriAI/litellm.git
synced 2026-09-12 23:01:41 +00:00
feat(triage): 1-day grace period before close + SwiftWinds immediate-close bypass
- Add a 24-hour grace window between the first low-quality detection
and the actual auto-close. The first detection posts a warning
comment that explicitly says "You have 1 day to address this before
this PR is auto-closed" and points the contributor at:
* `@agent-shin reconsider` to request another look (and re-open)
* `@greptileai` to request a fresh Greptile review — works
even after the PR is closed
- Both `triage_with_llm.py` (LLM judge) and `close_low_quality_prs.py`
(Greptile-score closer) share the same `<!-- agent-shin:grace-warning -->`
HTML marker so a warning posted by either path is recognized by both.
- Add IMMEDIATE_CLOSE_LOGINS = {swiftwinds} to bypass BOTH the grace
period AND the dry-run / AGENT_SHIN_ENABLED gating. SwiftWinds is the
user's personal account (no push permissions to litellm) used to
dogfood the bot; user explicitly asked: "For SwiftWinds, just close
immediately. Faster iteration that way."
- Update the standard close comments to mention that `@greptileai`
works even after the PR is closed.
- Add 23 new tests covering: warn-grace on first detection, skip during
grace window, close after grace expires, SwiftWinds bypass (case
insensitive, with close=False, no random-login false positives), the
grace-warning text invariants, and the SwiftWinds entry in the
IMMEDIATE_CLOSE_LOGINS constant.
Co-authored-by: Mateo Wang <mateo-berri@users.noreply.github.com>
This commit is contained in:
parent
4e0968ad8d
commit
f876173b78
4 changed files with 1120 additions and 32 deletions
214
.github/scripts/close_low_quality_prs.py
vendored
214
.github/scripts/close_low_quality_prs.py
vendored
|
|
@ -38,6 +38,7 @@ from __future__ import annotations
|
|||
import argparse
|
||||
import datetime as dt
|
||||
import json
|
||||
import os
|
||||
import re
|
||||
import subprocess
|
||||
import sys
|
||||
|
|
@ -66,6 +67,33 @@ INTERNAL_AUTHOR_ASSOCIATIONS = frozenset({"OWNER", "MEMBER", "COLLABORATOR"})
|
|||
# `default=[...]` combination silently mutates the shared default list.
|
||||
DEFAULT_OPTOUT_LABELS = ("do not close", "keep open", "wip")
|
||||
|
||||
# HTML marker appended to grace-period warning comments. Shared with the
|
||||
# Agent Shin LLM-judge script (`triage_with_llm.py`) so a warning posted
|
||||
# by either path is recognized by both: the LLM judge can see "Greptile
|
||||
# already warned this contributor 12 hours ago" and skip re-warning, and
|
||||
# the Greptile closer can see "Agent Shin already warned" and close on
|
||||
# the next run if Greptile still has a low score.
|
||||
GRACE_COMMENT_MARKER = "<!-- agent-shin:grace-warning -->"
|
||||
|
||||
# Length of the grace period between the warning comment and the actual
|
||||
# auto-close. Set to 24 hours so the contributor has at least one full
|
||||
# working day across any time zone to push fixes or comment
|
||||
# `@agent-shin reconsider`. Mirrors the constant of the same name in
|
||||
# `triage_with_llm.py` — keep them in sync if either changes.
|
||||
GRACE_PERIOD_SECONDS = 86400
|
||||
|
||||
# Default login of the GitHub identity that performs Agent Shin's writes;
|
||||
# used for matching the author of a grace-warning comment so we don't
|
||||
# count somebody quoting the marker. The env override
|
||||
# `AGENT_SHIN_BOT_LOGIN` mirrors `triage_with_llm.py`.
|
||||
AGENT_SHIN_DEFAULT_BOT_LOGIN = "github-actions[bot]"
|
||||
|
||||
# Logins (case-insensitive) that bypass BOTH the 1-day grace period AND
|
||||
# the dry-run gating. Mirrors `IMMEDIATE_CLOSE_LOGINS` in
|
||||
# `triage_with_llm.py`. Used for dogfooding the bot from external test
|
||||
# accounts that have no push permissions to the repo.
|
||||
IMMEDIATE_CLOSE_LOGINS = frozenset({"swiftwinds"})
|
||||
|
||||
|
||||
def gh(*args: str) -> str:
|
||||
"""Run a `gh` CLI command and return stdout. Raises on non-zero exit."""
|
||||
|
|
@ -196,6 +224,124 @@ def has_optout_label(pr: dict, optout_labels: set[str]) -> bool:
|
|||
return bool(labels & {lbl.lower() for lbl in optout_labels})
|
||||
|
||||
|
||||
def seconds_since_last_grace_warning(
|
||||
comments: Iterable[dict],
|
||||
*,
|
||||
bot_login: str | None = None,
|
||||
now: dt.datetime | None = None,
|
||||
) -> float | None:
|
||||
"""Return seconds since the bot's most recent grace-period warning, or
|
||||
None if no such warning has ever been posted on this PR.
|
||||
|
||||
Detects warnings by matching `GRACE_COMMENT_MARKER` in comments
|
||||
authored by the bot identity. Operates on an already-fetched
|
||||
comments list (avoids a second `gh api` call when the caller has
|
||||
already pulled the page for Greptile-score extraction).
|
||||
|
||||
`now` is injectable so callers (and tests) can pin the reference
|
||||
time. The closer runs everything against a single `now` snapshot
|
||||
captured at the top of `main()` so age calculations stay consistent
|
||||
across many PRs in a single run.
|
||||
"""
|
||||
expected_login = (
|
||||
bot_login
|
||||
or os.environ.get("AGENT_SHIN_BOT_LOGIN")
|
||||
or AGENT_SHIN_DEFAULT_BOT_LOGIN
|
||||
).lower()
|
||||
latest: dt.datetime | None = None
|
||||
for comment in comments:
|
||||
author = ((comment.get("user") or {}).get("login") or "").lower()
|
||||
if author != expected_login:
|
||||
continue
|
||||
body = comment.get("body") or ""
|
||||
if GRACE_COMMENT_MARKER not in body:
|
||||
continue
|
||||
created = comment.get("created_at")
|
||||
if not created:
|
||||
continue
|
||||
try:
|
||||
ts = parse_iso8601(created)
|
||||
except ValueError:
|
||||
continue
|
||||
if latest is None or ts > latest:
|
||||
latest = ts
|
||||
if latest is None:
|
||||
return None
|
||||
reference = now if now is not None else dt.datetime.now(dt.timezone.utc)
|
||||
return (reference - latest).total_seconds()
|
||||
|
||||
|
||||
def format_grace_warning_comment(score: int, threshold: int) -> str:
|
||||
"""Comment posted on the FIRST low-Greptile-score detection — gives
|
||||
the contributor a 1-day grace window before the auto-close fires on
|
||||
the next daily cron run.
|
||||
|
||||
Mirrors `format_grace_warning_pr_comment` in
|
||||
`triage_with_llm.py` in spirit (1-day grace + escape hatches), but
|
||||
framed around Greptile's confidence score instead of the LLM judge's
|
||||
rubric since the close trigger here is the Greptile signal.
|
||||
"""
|
||||
return (
|
||||
"👋 Hi, thanks for the PR! I'm **Agent Shin**, the automated triage bot for this repository.\n"
|
||||
"\n"
|
||||
"Heads up — Greptile's most recent review scored this PR "
|
||||
f"**{score}/5**, below our merge bar of **{threshold}/5**.\n"
|
||||
"\n"
|
||||
"⏳ **You have 1 day to address Greptile's feedback before this PR is auto-closed.** "
|
||||
"We close low-confidence PRs aggressively to keep the review queue manageable for "
|
||||
"maintainers and contributors alike. **This isn't a rejection of the idea.**\n"
|
||||
"\n"
|
||||
"During the grace period:\n"
|
||||
"\n"
|
||||
"1. Push fixes that address Greptile's feedback (continue using your existing branch is fine).\n"
|
||||
"2. Either:\n"
|
||||
" - Comment `@greptileai` to request a fresh Greptile review. If the new score is "
|
||||
f"**{threshold}/5 or higher**, the PR stays open.\n"
|
||||
" - Or comment `@agent-shin reconsider` to have Agent Shin re-evaluate the PR description.\n"
|
||||
"\n"
|
||||
"If this PR is auto-closed in 24 hours, you'll still have options:\n"
|
||||
"\n"
|
||||
"- Comment `@agent-shin reconsider` after pushing fixes — Agent Shin will re-run triage "
|
||||
"and reopen the PR if it now meets the bar.\n"
|
||||
"- Comment `@greptileai` to request a re-review — that works **even after the PR is closed**.\n"
|
||||
"\n"
|
||||
"Thanks for contributing to LiteLLM. We know auto-closures can sting; the goal is to keep "
|
||||
"the project healthy, not to dismiss your work.\n"
|
||||
"\n"
|
||||
f"{GRACE_COMMENT_MARKER}"
|
||||
)
|
||||
|
||||
|
||||
def post_grace_warning(
|
||||
pr: dict,
|
||||
score: int,
|
||||
threshold: int,
|
||||
repo: str | None,
|
||||
dry_run: bool,
|
||||
) -> None:
|
||||
"""Post the 1-day grace-period warning comment on `pr`.
|
||||
|
||||
The warning carries `GRACE_COMMENT_MARKER` so subsequent runs can
|
||||
detect that the contributor has already been told about the
|
||||
pending close. Does NOT close the PR — the close happens on the
|
||||
next eligible run after `GRACE_PERIOD_SECONDS` elapses (handled
|
||||
by `close_pr`).
|
||||
"""
|
||||
pr_number = pr["number"]
|
||||
repo_args = ["--repo", repo] if repo else []
|
||||
|
||||
if dry_run:
|
||||
print(
|
||||
f" [DRY RUN] Would post grace warning to PR #{pr_number} "
|
||||
f"(greptile={score}/5): {pr['title']}"
|
||||
)
|
||||
return
|
||||
|
||||
comment_body = format_grace_warning_comment(score, threshold)
|
||||
gh("pr", "comment", str(pr_number), "--body", comment_body, *repo_args)
|
||||
print(f" Posted grace warning on PR #{pr_number} (greptile={score}/5)")
|
||||
|
||||
|
||||
def close_pr(
|
||||
pr: dict,
|
||||
score: int,
|
||||
|
|
@ -219,7 +365,8 @@ def close_pr(
|
|||
comment_body = (
|
||||
f"Closing as part of automated PR triage.\n\n"
|
||||
f"Greptile's most recent review scored this PR **{score}/5**, below "
|
||||
f"our merge bar of **{threshold}/5**.\n\n"
|
||||
f"our merge bar of **{threshold}/5**, and the 1-day grace period since "
|
||||
"the warning has elapsed.\n\n"
|
||||
"We close low-confidence PRs aggressively to keep the review queue "
|
||||
"manageable for maintainers and contributors alike. **This is not a "
|
||||
"rejection of the idea** — to bring this back:\n\n"
|
||||
|
|
@ -233,7 +380,9 @@ def close_pr(
|
|||
"maintainer, so a fresh PR is the most reliable path forward. If you "
|
||||
"would prefer this exact PR re-evaluated, comment "
|
||||
"`@agent-shin reconsider` once you've pushed the fixes — Agent Shin "
|
||||
"will re-run triage and reopen this PR if it now meets the bar.\n\n"
|
||||
"will re-run triage and reopen this PR if it now meets the bar. "
|
||||
"You can also comment `@greptileai` to request a fresh Greptile "
|
||||
"review — that works **even after the PR is closed**.\n\n"
|
||||
"Thanks for contributing to LiteLLM. We know auto-closures can sting; "
|
||||
"the goal is to keep the project healthy, not to dismiss your work."
|
||||
)
|
||||
|
|
@ -258,16 +407,28 @@ def evaluate_pr(
|
|||
repo: str | None,
|
||||
optout_labels: set[str],
|
||||
) -> tuple[str, int | None, int | None]:
|
||||
"""Decide whether to close `pr`.
|
||||
"""Decide what to do with `pr` on this triage run.
|
||||
|
||||
Returns (action, score_or_none, age_days_or_none) where action is one of:
|
||||
"skip-too-young", "skip-optout-label", "skip-internal",
|
||||
"skip-no-greptile-score", "skip-score-ok", or "close".
|
||||
"skip-no-greptile-score", "skip-score-ok",
|
||||
"warn-grace", "skip-in-grace-period", or "close".
|
||||
|
||||
Drafts are NOT skipped — the goal is "open PR count == PRs internal
|
||||
collaborators need to action on", and a draft that Greptile scored <4/5
|
||||
is still in that queue. Authors can opt out via the `wip` label (see
|
||||
`DEFAULT_OPTOUT_LABELS`) if they need to keep a long-lived draft open.
|
||||
|
||||
Grace-period semantics: the first time a PR fails the rubric, the
|
||||
action is `warn-grace` — the caller should post a warning comment but
|
||||
NOT close the PR. On a subsequent run, if the warning is still less
|
||||
than `GRACE_PERIOD_SECONDS` old AND the PR still fails, the action is
|
||||
`skip-in-grace-period`. Once the warning ages out and the rubric is
|
||||
still failing, the action is `close`.
|
||||
|
||||
Grace is bypassed for `IMMEDIATE_CLOSE_LOGINS` (test/dogfood
|
||||
accounts), which always go straight to `close` on the first failing
|
||||
run so the bot is dogfoodable end-to-end without a 24h delay.
|
||||
"""
|
||||
if has_optout_label(pr, optout_labels):
|
||||
return ("skip-optout-label", None, None)
|
||||
|
|
@ -294,6 +455,16 @@ def evaluate_pr(
|
|||
if score >= min_score:
|
||||
return ("skip-score-ok", score, age_days)
|
||||
|
||||
login = ((pr.get("author") or {}).get("login") or "").lower()
|
||||
if login in IMMEDIATE_CLOSE_LOGINS:
|
||||
return ("close", score, age_days)
|
||||
|
||||
grace_age = seconds_since_last_grace_warning(comments, now=now)
|
||||
if grace_age is None:
|
||||
return ("warn-grace", score, age_days)
|
||||
if grace_age < GRACE_PERIOD_SECONDS:
|
||||
return ("skip-in-grace-period", score, age_days)
|
||||
|
||||
return ("close", score, age_days)
|
||||
|
||||
|
||||
|
|
@ -370,6 +541,8 @@ def main() -> int:
|
|||
closed = 0
|
||||
summary = {
|
||||
"close": 0,
|
||||
"warn-grace": 0,
|
||||
"skip-in-grace-period": 0,
|
||||
"skip-too-young": 0,
|
||||
"skip-optout-label": 0,
|
||||
"skip-internal": 0,
|
||||
|
|
@ -388,6 +561,30 @@ def main() -> int:
|
|||
)
|
||||
summary[action] = summary.get(action, 0) + 1
|
||||
|
||||
# Per-PR dry-run override: `IMMEDIATE_CLOSE_LOGINS` accounts (e.g.
|
||||
# SwiftWinds) always run in real-close mode regardless of the
|
||||
# global `--close` flag. Lets a maintainer dogfood the bot from
|
||||
# an external account while the rest of the open-PR queue stays
|
||||
# on the safe dry-run default.
|
||||
author_login = ((pr.get("author") or {}).get("login") or "").lower()
|
||||
is_immediate = author_login in IMMEDIATE_CLOSE_LOGINS
|
||||
pr_dry_run = dry_run and not is_immediate
|
||||
|
||||
if action == "warn-grace":
|
||||
assert score is not None
|
||||
print(
|
||||
f"#{pr['number']}: \"{pr['title']}\" "
|
||||
f"(age={age_days}d, greptile={score}/5) -> warn-grace"
|
||||
)
|
||||
post_grace_warning(
|
||||
pr,
|
||||
score=score,
|
||||
threshold=args.min_score,
|
||||
repo=args.repo,
|
||||
dry_run=pr_dry_run,
|
||||
)
|
||||
continue
|
||||
|
||||
if action != "close":
|
||||
continue
|
||||
|
||||
|
|
@ -395,6 +592,7 @@ def main() -> int:
|
|||
print(
|
||||
f"#{pr['number']}: \"{pr['title']}\" "
|
||||
f"(age={age_days}d, greptile={score}/5) -> close"
|
||||
+ (" [immediate-close login]" if is_immediate else "")
|
||||
)
|
||||
close_pr(
|
||||
pr,
|
||||
|
|
@ -402,11 +600,11 @@ def main() -> int:
|
|||
threshold=args.min_score,
|
||||
age_days=age_days,
|
||||
repo=args.repo,
|
||||
dry_run=dry_run,
|
||||
dry_run=pr_dry_run,
|
||||
label=args.close_label,
|
||||
)
|
||||
|
||||
if not dry_run:
|
||||
if not pr_dry_run:
|
||||
closed += 1
|
||||
if args.limit is not None and closed >= args.limit:
|
||||
print(f"\nReached --limit={args.limit}; stopping.")
|
||||
|
|
@ -416,6 +614,10 @@ def main() -> int:
|
|||
for key, value in summary.items():
|
||||
print(f" {key:28s} {value}")
|
||||
print(f"\nTotal {'would close' if dry_run else 'closed'}: {summary['close']}")
|
||||
print(
|
||||
f"Total {'would warn (grace)' if dry_run else 'warned (grace)'}: "
|
||||
f"{summary['warn-grace']}"
|
||||
)
|
||||
return 0
|
||||
|
||||
|
||||
|
|
|
|||
218
.github/scripts/triage_with_llm.py
vendored
218
.github/scripts/triage_with_llm.py
vendored
|
|
@ -62,6 +62,27 @@ RECONSIDER_COMMENT_MARKER = "<!-- agent-shin:reconsider-verdict -->"
|
|||
# the body" iteration loop isn't punished.
|
||||
RECONSIDER_RATE_LIMIT_SECONDS = 600
|
||||
|
||||
# HTML marker appended to the grace-period warning comment posted on the
|
||||
# first low-quality detection. We grep for this on subsequent triage runs
|
||||
# to (a) detect that a warning was already posted (so we don't spam the
|
||||
# contributor with duplicate warnings) and (b) measure how long ago it
|
||||
# was posted so we know when the grace period has elapsed.
|
||||
GRACE_COMMENT_MARKER = "<!-- agent-shin:grace-warning -->"
|
||||
|
||||
# Length of the grace period between the warning comment and the actual
|
||||
# auto-close. Set to 24 hours so the contributor has at least one full
|
||||
# working day across any time zone to push fixes or comment
|
||||
# `@agent-shin reconsider`.
|
||||
GRACE_PERIOD_SECONDS = 86400
|
||||
|
||||
# Logins (case-insensitive) that bypass BOTH the 1-day grace period AND
|
||||
# the dry-run / `AGENT_SHIN_ENABLED` workflow gating — every Agent Shin
|
||||
# verdict against a PR/issue from one of these accounts is treated as a
|
||||
# real run with immediate close on fail. Useful for dogfooding the bot
|
||||
# from an external account that has no push permissions to the repo.
|
||||
# Listed lower-case so callers compare via `login.lower() in ...`.
|
||||
IMMEDIATE_CLOSE_LOGINS = frozenset({"swiftwinds"})
|
||||
|
||||
# Model families that require `reasoning_effort` to be set, and that reject
|
||||
# `temperature != 1` unless `reasoning_effort` is "none". For these models we
|
||||
# pass `reasoning_effort="none"` so a `temperature=0` deterministic judgment
|
||||
|
|
@ -238,17 +259,20 @@ def was_closed_by_agent_shin(
|
|||
return actor.lower() == expected
|
||||
|
||||
|
||||
def seconds_since_last_reconsider_verdict(
|
||||
repo: str, number: int, *, bot_login: str | None = None
|
||||
def _seconds_since_latest_marker_comment(
|
||||
repo: str,
|
||||
number: int,
|
||||
*,
|
||||
marker: str,
|
||||
bot_login: str | None = None,
|
||||
) -> float | None:
|
||||
"""Return seconds since the bot's most recent reconsider verdict comment.
|
||||
"""Shared helper: return seconds since the bot's most recent comment
|
||||
that contains the given HTML marker, or None if no such comment exists.
|
||||
|
||||
Detects comments by matching the HTML marker `RECONSIDER_COMMENT_MARKER`
|
||||
appended by `format_reopen_comment` and
|
||||
`format_reconsider_still_failing_comment`. Returns None when the bot
|
||||
has never posted a reconsider verdict on this PR/issue (or when the
|
||||
only matching comments are missing a `created_at` timestamp, which
|
||||
shouldn't happen on a real GitHub response).
|
||||
Used by both the reconsider-verdict cooldown and the grace-period
|
||||
warning detection — keeping the iteration logic centralized stops the
|
||||
two paths from drifting (e.g. one fixing a tz parsing bug and the
|
||||
other forgetting to mirror it).
|
||||
"""
|
||||
expected_login = (
|
||||
bot_login
|
||||
|
|
@ -261,7 +285,7 @@ def seconds_since_last_reconsider_verdict(
|
|||
if author != expected_login:
|
||||
continue
|
||||
body = comment.get("body") or ""
|
||||
if RECONSIDER_COMMENT_MARKER not in body:
|
||||
if marker not in body:
|
||||
continue
|
||||
created = comment.get("created_at")
|
||||
if not created:
|
||||
|
|
@ -277,6 +301,39 @@ def seconds_since_last_reconsider_verdict(
|
|||
return (dt.datetime.now(dt.timezone.utc) - latest).total_seconds()
|
||||
|
||||
|
||||
def seconds_since_last_reconsider_verdict(
|
||||
repo: str, number: int, *, bot_login: str | None = None
|
||||
) -> float | None:
|
||||
"""Return seconds since the bot's most recent reconsider verdict comment.
|
||||
|
||||
Detects comments by matching the HTML marker `RECONSIDER_COMMENT_MARKER`
|
||||
appended by `format_reopen_comment` and
|
||||
`format_reconsider_still_failing_comment`. Returns None when the bot
|
||||
has never posted a reconsider verdict on this PR/issue (or when the
|
||||
only matching comments are missing a `created_at` timestamp, which
|
||||
shouldn't happen on a real GitHub response).
|
||||
"""
|
||||
return _seconds_since_latest_marker_comment(
|
||||
repo, number, marker=RECONSIDER_COMMENT_MARKER, bot_login=bot_login
|
||||
)
|
||||
|
||||
|
||||
def seconds_since_last_grace_warning(
|
||||
repo: str, number: int, *, bot_login: str | None = None
|
||||
) -> float | None:
|
||||
"""Return seconds since the bot's most recent grace-period warning.
|
||||
|
||||
Detects warning comments by matching the HTML marker
|
||||
`GRACE_COMMENT_MARKER` appended by `format_grace_warning_pr_comment`
|
||||
and `format_grace_warning_issue_comment`. Returns None when no
|
||||
grace warning has ever been posted on this PR/issue — that's the
|
||||
"first low-quality detection" signal that drives the warning path.
|
||||
"""
|
||||
return _seconds_since_latest_marker_comment(
|
||||
repo, number, marker=GRACE_COMMENT_MARKER, bot_login=bot_login
|
||||
)
|
||||
|
||||
|
||||
# ---------------------------------------------------------------------------
|
||||
# Author classification
|
||||
|
||||
|
|
@ -510,6 +567,9 @@ def format_pr_close_comment(verdict: dict) -> str:
|
|||
"to get back into the review queue.\n"
|
||||
" - **Or** comment `@agent-shin reconsider` on this closed PR after updating the description. "
|
||||
"I'll re-run the triage; if it now passes, I'll reopen this PR automatically.\n"
|
||||
" - You can also comment `@greptileai` on this PR to request a fresh Greptile review — that "
|
||||
"still works **even after the PR is closed**, and a higher score is one of the signals that "
|
||||
"lifts the PR back into the queue.\n"
|
||||
"\n"
|
||||
"Internal BerriAI contributors: this rubric doesn't apply to you — ping a maintainer.\n"
|
||||
"\n"
|
||||
|
|
@ -550,6 +610,90 @@ def format_issue_close_comment(verdict: dict) -> str:
|
|||
)
|
||||
|
||||
|
||||
def format_grace_warning_pr_comment(verdict: dict) -> str:
|
||||
"""Comment posted on the FIRST low-quality detection — gives the
|
||||
contributor a 1-day grace window to fix the PR before the next
|
||||
triage run actually closes it.
|
||||
|
||||
This is the "before-close" warning. On the second triage run, if the
|
||||
grace marker is older than `GRACE_PERIOD_SECONDS` AND the PR still
|
||||
fails the rubric, the close path runs (which posts
|
||||
`format_pr_close_comment` and closes the PR).
|
||||
"""
|
||||
missing_lines = _format_missing(verdict.get("missing") or [])
|
||||
explanation = verdict.get("explanation") or ""
|
||||
return (
|
||||
"👋 Hi, thanks for the PR! I'm **Agent Shin**, the automated triage bot for this repository.\n"
|
||||
"\n"
|
||||
"Heads up — this PR does not yet meet the bar described in our "
|
||||
"[pull-request template](https://github.com/BerriAI/litellm/blob/main/.github/pull_request_template.md). "
|
||||
"Specifically, I couldn't find:\n"
|
||||
"\n"
|
||||
f"{missing_lines}\n"
|
||||
"\n"
|
||||
f"> {explanation}\n"
|
||||
"\n"
|
||||
"⏳ **You have 1 day to address this before this PR is auto-closed.** "
|
||||
"During the grace period:\n"
|
||||
"\n"
|
||||
"1. Update the PR description to either:\n"
|
||||
" - Link a related GitHub issue (e.g. `Fixes #1234`), OR\n"
|
||||
" - Add a clear **problem description**, **expected vs. actual behavior**, and **visual QA proof** "
|
||||
"(before/after screenshots, a short screen recording, or terminal/log output).\n"
|
||||
"2. Comment `@agent-shin reconsider` on this PR after updating it. If your update meets the "
|
||||
"bar, I'll skip the auto-close and a maintainer will take another look.\n"
|
||||
"\n"
|
||||
"If this PR is auto-closed in 24 hours, you'll still have options:\n"
|
||||
"\n"
|
||||
"- Comment `@agent-shin reconsider` to have me re-evaluate (and reopen the PR if it now meets the bar).\n"
|
||||
"- Comment `@greptileai` to request a fresh Greptile review — that works **even after the PR is closed**.\n"
|
||||
"\n"
|
||||
"Internal BerriAI contributors: this rubric doesn't apply to you — ping a maintainer.\n"
|
||||
"\n"
|
||||
"_(I'm an LLM, so I'm not infallible. If you think I got this wrong, comment "
|
||||
"`@agent-shin reconsider` or ping a maintainer — they'll override me.)_\n"
|
||||
"\n"
|
||||
f"{GRACE_COMMENT_MARKER}"
|
||||
)
|
||||
|
||||
|
||||
def format_grace_warning_issue_comment(verdict: dict) -> str:
|
||||
"""Issue analogue of `format_grace_warning_pr_comment`."""
|
||||
missing_lines = _format_missing(verdict.get("missing") or [])
|
||||
explanation = verdict.get("explanation") or ""
|
||||
return (
|
||||
"👋 Hi, thanks for filing this! I'm **Agent Shin**, the automated triage bot for this repository.\n"
|
||||
"\n"
|
||||
"Heads up — this issue doesn't yet have enough detail for a maintainer to act on. "
|
||||
"Specifically, I couldn't find:\n"
|
||||
"\n"
|
||||
f"{missing_lines}\n"
|
||||
"\n"
|
||||
f"> {explanation}\n"
|
||||
"\n"
|
||||
"⏳ **You have 1 day to address this before this issue is auto-closed.** "
|
||||
"During the grace period:\n"
|
||||
"\n"
|
||||
"1. Edit the issue to add the missing pieces:\n"
|
||||
" - For **bug reports**: a runnable reproduction (code / curl / config), expected vs. actual behavior, "
|
||||
"and a screenshot / traceback / log showing the bug.\n"
|
||||
" - For **feature requests**: a concrete description of what should change, plus a use case and example "
|
||||
"(config / API call / UI flow).\n"
|
||||
"2. Comment `@agent-shin reconsider` on this issue after updating it. If your update meets the bar, "
|
||||
"I'll skip the auto-close and a maintainer will take another look.\n"
|
||||
"\n"
|
||||
"If this issue is auto-closed in 24 hours, you can still comment `@agent-shin reconsider` to have "
|
||||
"me re-evaluate (and reopen the issue if it now meets the bar).\n"
|
||||
"\n"
|
||||
"Internal BerriAI contributors: this rubric doesn't apply to you — ping a maintainer.\n"
|
||||
"\n"
|
||||
"_(I'm an LLM, so I'm not infallible. If you think I got this wrong, comment "
|
||||
"`@agent-shin reconsider` or ping a maintainer — they'll override me.)_\n"
|
||||
"\n"
|
||||
f"{GRACE_COMMENT_MARKER}"
|
||||
)
|
||||
|
||||
|
||||
# ---------------------------------------------------------------------------
|
||||
# Step-summary helpers
|
||||
|
||||
|
|
@ -809,7 +953,58 @@ def triage(
|
|||
if decision != "fail":
|
||||
return {**base_result, "action": "pass-llm", "verdict": verdict}
|
||||
|
||||
if not close:
|
||||
# Grace-period flow: on the first low-quality detection, post a warning
|
||||
# comment instead of closing immediately. On a subsequent triage run
|
||||
# (manual re-trigger, or the daily `close_low_quality_prs.py` cron
|
||||
# finding the same PR in its own pass), if `GRACE_PERIOD_SECONDS` has
|
||||
# elapsed since the warning AND the PR still fails the rubric, close.
|
||||
#
|
||||
# `IMMEDIATE_CLOSE_LOGINS` (e.g. test/dogfood accounts like SwiftWinds)
|
||||
# bypass the grace period entirely — every fail is treated as a real
|
||||
# close run. This is intentional: those accounts exist specifically to
|
||||
# exercise the bot end-to-end, and waiting a day per iteration kills
|
||||
# the feedback loop.
|
||||
is_immediate = login.lower() in IMMEDIATE_CLOSE_LOGINS
|
||||
|
||||
if not is_immediate:
|
||||
grace_age = seconds_since_last_grace_warning(repo, number)
|
||||
if grace_age is None:
|
||||
warning_body = (
|
||||
format_grace_warning_pr_comment(verdict)
|
||||
if kind == "pr"
|
||||
else format_grace_warning_issue_comment(verdict)
|
||||
)
|
||||
if not close:
|
||||
return {
|
||||
**base_result,
|
||||
"action": "would-warn-grace",
|
||||
"verdict": verdict,
|
||||
"comment": warning_body,
|
||||
}
|
||||
post_comment(repo, number, warning_body)
|
||||
return {
|
||||
**base_result,
|
||||
"action": "warned-grace",
|
||||
"verdict": verdict,
|
||||
"comment": warning_body,
|
||||
}
|
||||
if grace_age < GRACE_PERIOD_SECONDS:
|
||||
return {
|
||||
**base_result,
|
||||
"action": "skip-in-grace-period",
|
||||
"verdict": verdict,
|
||||
"grace_age_seconds": grace_age,
|
||||
"grace_period_seconds": GRACE_PERIOD_SECONDS,
|
||||
}
|
||||
|
||||
# Either the grace window has elapsed or this author bypasses grace —
|
||||
# proceed to the actual close path. `--close` still gates the
|
||||
# destructive write for the regular-author path so a local operator
|
||||
# can preview a "would close after grace" verdict; immediate-close
|
||||
# accounts (`is_immediate`) ignore `--close` so a workflow that
|
||||
# forces dry-run for the global population can still take real
|
||||
# action on those test accounts.
|
||||
if not close and not is_immediate:
|
||||
return {**base_result, "action": "would-close", "verdict": verdict}
|
||||
|
||||
comment_body = (
|
||||
|
|
@ -828,6 +1023,7 @@ def triage(
|
|||
"action": "closed",
|
||||
"verdict": verdict,
|
||||
"comment": comment_body,
|
||||
"immediate_close": is_immediate,
|
||||
}
|
||||
|
||||
|
||||
|
|
|
|||
|
|
@ -125,6 +125,7 @@ class TestEvaluatePr:
|
|||
created_days_ago: int = 10,
|
||||
is_draft: bool = False,
|
||||
labels: list[str] | None = None,
|
||||
author_login: str = "someone",
|
||||
) -> dict:
|
||||
created = dt.datetime(2026, 5, 17, tzinfo=dt.timezone.utc) - dt.timedelta(
|
||||
days=created_days_ago
|
||||
|
|
@ -135,7 +136,7 @@ class TestEvaluatePr:
|
|||
"createdAt": created.isoformat().replace("+00:00", "Z"),
|
||||
"isDraft": is_draft,
|
||||
"labels": [{"name": lbl} for lbl in (labels or [])],
|
||||
"author": {"login": "someone"},
|
||||
"author": {"login": author_login},
|
||||
"url": f"https://example.com/pr/{number}",
|
||||
}
|
||||
|
||||
|
|
@ -146,10 +147,13 @@ class TestEvaluatePr:
|
|||
closer_module, "is_external_pr_author", lambda pr, repo: True
|
||||
)
|
||||
|
||||
def test_should_close_drafts_when_score_low(self, closer_module, _now, monkeypatch):
|
||||
def test_should_warn_drafts_when_score_low_first_time(
|
||||
self, closer_module, _now, monkeypatch
|
||||
):
|
||||
# Drafts are NOT a free pass — the open-PR queue should reflect any
|
||||
# PR that needs human attention regardless of draft status. Authors
|
||||
# who need a long-lived draft can use the `wip` opt-out label.
|
||||
# First run: warn the contributor (1-day grace), don't close yet.
|
||||
monkeypatch.setattr(
|
||||
closer_module,
|
||||
"fetch_pr_comments",
|
||||
|
|
@ -163,15 +167,17 @@ class TestEvaluatePr:
|
|||
repo=None,
|
||||
optout_labels=set(),
|
||||
)
|
||||
assert action == "close"
|
||||
assert action == "warn-grace"
|
||||
assert score == 2 and age == 0
|
||||
|
||||
def test_should_close_brand_new_pr_when_min_age_zero(
|
||||
def test_should_warn_brand_new_pr_when_min_age_zero(
|
||||
self, closer_module, _now, monkeypatch
|
||||
):
|
||||
# `min_age_days=0` means no age filter — a freshly-opened PR is
|
||||
# eligible the moment Greptile scores it below threshold. This is
|
||||
# the new default behavior.
|
||||
# eligible the moment Greptile scores it below threshold. The
|
||||
# first detection still goes through the warn-grace step rather
|
||||
# than closing immediately, giving the contributor 24 hours to
|
||||
# respond before the next run actually closes the PR.
|
||||
monkeypatch.setattr(
|
||||
closer_module,
|
||||
"fetch_pr_comments",
|
||||
|
|
@ -185,7 +191,7 @@ class TestEvaluatePr:
|
|||
repo=None,
|
||||
optout_labels=set(),
|
||||
)
|
||||
assert action == "close"
|
||||
assert action == "warn-grace"
|
||||
assert score == 1 and age == 0
|
||||
|
||||
def test_should_skip_optout_label_case_insensitive(
|
||||
|
|
@ -284,9 +290,12 @@ class TestEvaluatePr:
|
|||
assert action == "skip-score-ok"
|
||||
assert score == 4 and age == 10
|
||||
|
||||
def test_should_close_when_old_and_low_score(
|
||||
def test_should_warn_when_old_and_low_score_no_prior_warning(
|
||||
self, closer_module, _now, monkeypatch
|
||||
):
|
||||
# Even an old PR that still has no grace warning gets one on the
|
||||
# first eligible run — the daily cron is the natural cadence, so
|
||||
# an existing-but-never-warned PR enters the grace flow normally.
|
||||
monkeypatch.setattr(
|
||||
closer_module,
|
||||
"fetch_pr_comments",
|
||||
|
|
@ -300,16 +309,37 @@ class TestEvaluatePr:
|
|||
repo=None,
|
||||
optout_labels=set(),
|
||||
)
|
||||
assert action == "close"
|
||||
assert action == "warn-grace"
|
||||
assert score == 3 and age == 10
|
||||
|
||||
def test_should_close_when_old_and_very_low_score(
|
||||
def test_should_close_when_grace_warning_aged_out_and_score_still_low(
|
||||
self, closer_module, _now, monkeypatch
|
||||
):
|
||||
# Day-1 the closer posted a warning. Day-2 the PR still scores <4
|
||||
# AND the warning is older than `GRACE_PERIOD_SECONDS`, so the
|
||||
# action flips to `close`. This is the "grace expired" path.
|
||||
old_warning = {
|
||||
"user": {"login": "github-actions[bot]"},
|
||||
"body": (
|
||||
"you have 1 day to fix this\n\n" + closer_module.GRACE_COMMENT_MARKER
|
||||
),
|
||||
"created_at": (
|
||||
_now - dt.timedelta(seconds=closer_module.GRACE_PERIOD_SECONDS + 60)
|
||||
)
|
||||
.isoformat()
|
||||
.replace("+00:00", "Z"),
|
||||
"updated_at": "2026-05-15T00:00:00Z",
|
||||
}
|
||||
monkeypatch.setattr(
|
||||
closer_module,
|
||||
"fetch_pr_comments",
|
||||
lambda *a, **kw: [_greptile_comment("<h3>Confidence Score: 1/5</h3>")],
|
||||
lambda *a, **kw: [
|
||||
_greptile_comment(
|
||||
"<h3>Confidence Score: 1/5</h3>",
|
||||
updated_at="2026-05-15T00:00:00Z",
|
||||
),
|
||||
old_warning,
|
||||
],
|
||||
)
|
||||
action, score, _ = closer_module.evaluate_pr(
|
||||
self._make_pr(created_days_ago=14),
|
||||
|
|
@ -322,6 +352,82 @@ class TestEvaluatePr:
|
|||
assert action == "close"
|
||||
assert score == 1
|
||||
|
||||
def test_should_skip_when_grace_warning_within_window(
|
||||
self, closer_module, _now, monkeypatch
|
||||
):
|
||||
# Within the 24-hour grace window the closer must NOT close the
|
||||
# PR even if the score is still low. The warning is only an hour
|
||||
# old; give the contributor time to push fixes before destruction.
|
||||
recent_warning = {
|
||||
"user": {"login": "github-actions[bot]"},
|
||||
"body": "warning text\n\n" + closer_module.GRACE_COMMENT_MARKER,
|
||||
"created_at": (_now - dt.timedelta(hours=1))
|
||||
.isoformat()
|
||||
.replace("+00:00", "Z"),
|
||||
}
|
||||
monkeypatch.setattr(
|
||||
closer_module,
|
||||
"fetch_pr_comments",
|
||||
lambda *a, **kw: [
|
||||
_greptile_comment("Confidence Score: 2/5"),
|
||||
recent_warning,
|
||||
],
|
||||
)
|
||||
action, score, _ = closer_module.evaluate_pr(
|
||||
self._make_pr(created_days_ago=10),
|
||||
now=_now,
|
||||
min_age_days=0,
|
||||
min_score=4,
|
||||
repo=None,
|
||||
optout_labels=set(),
|
||||
)
|
||||
assert action == "skip-in-grace-period"
|
||||
assert score == 2
|
||||
|
||||
def test_should_close_immediately_for_swiftwinds_login(
|
||||
self, closer_module, _now, monkeypatch
|
||||
):
|
||||
# SwiftWinds is in `IMMEDIATE_CLOSE_LOGINS` for dogfooding the bot
|
||||
# from an external account. Skip grace; close on first detection
|
||||
# so the iteration loop is fast.
|
||||
monkeypatch.setattr(
|
||||
closer_module,
|
||||
"fetch_pr_comments",
|
||||
lambda *a, **kw: [_greptile_comment("Confidence Score: 1/5")],
|
||||
)
|
||||
action, score, _ = closer_module.evaluate_pr(
|
||||
self._make_pr(created_days_ago=0, author_login="SwiftWinds"),
|
||||
now=_now,
|
||||
min_age_days=0,
|
||||
min_score=4,
|
||||
repo=None,
|
||||
optout_labels=set(),
|
||||
)
|
||||
assert action == "close"
|
||||
assert score == 1
|
||||
|
||||
def test_should_close_immediately_for_swiftwinds_login_case_insensitive(
|
||||
self, closer_module, _now, monkeypatch
|
||||
):
|
||||
# GitHub login matching is case-insensitive on GitHub's side; the
|
||||
# API returns the original casing. Make sure the bypass fires
|
||||
# regardless of how the login was registered.
|
||||
monkeypatch.setattr(
|
||||
closer_module,
|
||||
"fetch_pr_comments",
|
||||
lambda *a, **kw: [_greptile_comment("Confidence Score: 1/5")],
|
||||
)
|
||||
for login in ("SwiftWinds", "swiftwinds", "SWIFTWINDS"):
|
||||
action, _, _ = closer_module.evaluate_pr(
|
||||
self._make_pr(created_days_ago=0, author_login=login),
|
||||
now=_now,
|
||||
min_age_days=0,
|
||||
min_score=4,
|
||||
repo=None,
|
||||
optout_labels=set(),
|
||||
)
|
||||
assert action == "close", login
|
||||
|
||||
def test_should_skip_internal_authors(self, closer_module, _now, monkeypatch):
|
||||
# Override the fixture for this one test.
|
||||
monkeypatch.setattr(
|
||||
|
|
@ -422,6 +528,120 @@ class TestMainOptoutLabelDefault:
|
|||
assert default not in captured["optout_labels"], default
|
||||
|
||||
|
||||
class TestSecondsSinceLastGraceWarning:
|
||||
"""Grace-period detection: only counts comments by the bot identity
|
||||
that contain the shared `GRACE_COMMENT_MARKER`."""
|
||||
|
||||
def _make_marker_comment(
|
||||
self,
|
||||
closer_module,
|
||||
*,
|
||||
login: str = "github-actions[bot]",
|
||||
created_at: str = "2026-05-16T00:00:00Z",
|
||||
include_marker: bool = True,
|
||||
) -> dict:
|
||||
body = "warning text"
|
||||
if include_marker:
|
||||
body += "\n\n" + closer_module.GRACE_COMMENT_MARKER
|
||||
return {
|
||||
"user": {"login": login},
|
||||
"body": body,
|
||||
"created_at": created_at,
|
||||
}
|
||||
|
||||
def test_should_return_none_when_no_marker_comment(self, closer_module):
|
||||
comments = [
|
||||
{
|
||||
"user": {"login": "github-actions[bot]"},
|
||||
"body": "Some other bot comment",
|
||||
"created_at": "2026-05-16T00:00:00Z",
|
||||
}
|
||||
]
|
||||
assert closer_module.seconds_since_last_grace_warning(comments) is None
|
||||
|
||||
def test_should_return_none_for_empty(self, closer_module):
|
||||
assert closer_module.seconds_since_last_grace_warning([]) is None
|
||||
|
||||
def test_should_ignore_non_bot_comments_with_marker(self, closer_module):
|
||||
# If a curious user quotes the marker in a comment, we must NOT
|
||||
# treat it as a bot warning. The grace timer would then never fire.
|
||||
comments = [
|
||||
self._make_marker_comment(closer_module, login="random-user"),
|
||||
]
|
||||
assert closer_module.seconds_since_last_grace_warning(comments) is None
|
||||
|
||||
def test_should_pick_latest_marker_comment(self, closer_module):
|
||||
# When multiple grace warnings exist (e.g. a re-open cycle), use
|
||||
# the most recent one to compute the age.
|
||||
comments = [
|
||||
self._make_marker_comment(closer_module, created_at="2026-05-15T00:00:00Z"),
|
||||
self._make_marker_comment(closer_module, created_at="2026-05-16T23:00:00Z"),
|
||||
]
|
||||
now = dt.datetime(2026, 5, 17, 0, 0, 0, tzinfo=dt.timezone.utc)
|
||||
age = closer_module.seconds_since_last_grace_warning(comments, now=now)
|
||||
# 1h = 3600s
|
||||
assert age == 3600.0
|
||||
|
||||
|
||||
class TestImmediateCloseLoginsConstant:
|
||||
"""SwiftWinds is the dogfood account the user explicitly named — pin
|
||||
its presence so a future cleanup that removes the constant or
|
||||
forgets to keep the entry doesn't silently break the test path."""
|
||||
|
||||
def test_should_include_swiftwinds(self, closer_module):
|
||||
assert "swiftwinds" in closer_module.IMMEDIATE_CLOSE_LOGINS
|
||||
|
||||
def test_should_be_lowercase_for_case_insensitive_match(self, closer_module):
|
||||
for login in closer_module.IMMEDIATE_CLOSE_LOGINS:
|
||||
assert login == login.lower(), login
|
||||
|
||||
|
||||
class TestGraceWarningCommentText:
|
||||
"""Pin the user-facing language in the grace warning comment so the
|
||||
`1 day grace` and `@greptileai still works after close` promises
|
||||
don't get accidentally dropped in a future refactor.
|
||||
"""
|
||||
|
||||
def test_should_state_one_day_grace_period(self, closer_module):
|
||||
body = closer_module.format_grace_warning_comment(score=2, threshold=4)
|
||||
# The user's PR explicitly said "specify in the comment" — pin
|
||||
# that the literal "1 day" appears in the comment.
|
||||
assert "1 day" in body
|
||||
|
||||
def test_should_mention_agent_shin_reconsider(self, closer_module):
|
||||
body = closer_module.format_grace_warning_comment(score=2, threshold=4)
|
||||
assert "@agent-shin reconsider" in body
|
||||
|
||||
def test_should_promise_greptileai_works_after_close(self, closer_module):
|
||||
body = closer_module.format_grace_warning_comment(score=2, threshold=4)
|
||||
assert "@greptileai" in body
|
||||
assert "even after the PR is closed" in body
|
||||
|
||||
def test_should_carry_grace_marker(self, closer_module):
|
||||
# The marker is what `seconds_since_last_grace_warning` greps for
|
||||
# to detect a prior warning — dropping it would silently break
|
||||
# the cooldown.
|
||||
body = closer_module.format_grace_warning_comment(score=2, threshold=4)
|
||||
assert closer_module.GRACE_COMMENT_MARKER in body
|
||||
|
||||
def test_close_comment_should_mention_greptileai_post_close(self, closer_module):
|
||||
# The actual close comment should ALSO point at the @greptileai
|
||||
# post-close re-review path so contributors see the same options
|
||||
# whether they read the warning or only catch the close comment.
|
||||
# `close_pr` writes the close comment via `gh pr comment` — we
|
||||
# don't easily call it directly here, but the comment body is
|
||||
# constructed inline. Re-creating it via a no-op `gh` stub is
|
||||
# awkward, so we assert against the same string template by
|
||||
# asserting that the close path's text constant is updated.
|
||||
# `close_pr` source must contain the marker text — guarded by
|
||||
# this whole-module read-and-assert.
|
||||
from pathlib import Path
|
||||
|
||||
source = Path(closer_module.__file__).read_text()
|
||||
assert "even after the PR is closed" in source
|
||||
assert "@greptileai" in source
|
||||
|
||||
|
||||
class TestHasOptoutLabel:
|
||||
def test_should_match_label_case_insensitively(self, closer_module):
|
||||
pr = {"labels": [{"name": "Do Not Close"}, {"name": "bug"}]}
|
||||
|
|
|
|||
|
|
@ -619,9 +619,12 @@ class TestTriageOrchestration:
|
|||
self, triage_module, monkeypatch
|
||||
):
|
||||
# "See #1234" is a passing mention, not a closing keyword. The LLM
|
||||
# must get a chance to apply the stricter rubric.
|
||||
# must get a chance to apply the stricter rubric. With no prior
|
||||
# grace warning, the first failing verdict triggers the warning
|
||||
# path (`would-warn-grace` in dry-run).
|
||||
pr = self._make_pr(body="See #1234 for context. No QA proof here.")
|
||||
monkeypatch.setattr(triage_module, "fetch_pr", lambda repo, n: pr)
|
||||
self._stub_grace_no_warning(triage_module, monkeypatch)
|
||||
called = {"judge": False}
|
||||
|
||||
def judge(prompt):
|
||||
|
|
@ -639,7 +642,7 @@ class TestTriageOrchestration:
|
|||
judge=judge,
|
||||
)
|
||||
assert called["judge"] is True
|
||||
assert result["action"] == "would-close"
|
||||
assert result["action"] == "would-warn-grace"
|
||||
|
||||
def test_should_return_pass_llm_when_judge_passes(self, triage_module, monkeypatch):
|
||||
pr = self._make_pr(body="Long body, no linked issue.")
|
||||
|
|
@ -656,9 +659,16 @@ class TestTriageOrchestration:
|
|||
assert result["action"] == "pass-llm"
|
||||
assert "Long body" in captured["prompt"]
|
||||
|
||||
def test_should_return_would_close_in_dry_run(self, triage_module, monkeypatch):
|
||||
def test_should_return_would_close_in_dry_run_after_grace_aged_out(
|
||||
self, triage_module, monkeypatch
|
||||
):
|
||||
# When the grace warning has already aged out (>= GRACE_PERIOD_SECONDS)
|
||||
# AND the rubric still fails, the dry-run preview returns
|
||||
# `would-close` so a step-summary writer can render the close
|
||||
# comment without touching GitHub state.
|
||||
pr = self._make_pr(body="just a sentence.")
|
||||
monkeypatch.setattr(triage_module, "fetch_pr", lambda repo, n: pr)
|
||||
self._stub_grace_aged_out(triage_module, monkeypatch)
|
||||
|
||||
def fake_post(*a, **kw):
|
||||
pytest.fail("should not post comments in dry-run")
|
||||
|
|
@ -685,11 +695,15 @@ class TestTriageOrchestration:
|
|||
assert result["action"] == "would-close"
|
||||
assert result["verdict"]["missing"] == ["problem description", "QA proof"]
|
||||
|
||||
def test_should_post_comment_and_close_when_close_enabled(
|
||||
def test_should_post_comment_and_close_after_grace_window(
|
||||
self, triage_module, monkeypatch
|
||||
):
|
||||
# The "real close" path: --close passed AND the grace warning has
|
||||
# aged out AND the rubric still fails. The bot posts the close
|
||||
# comment and closes the PR.
|
||||
pr = self._make_pr(body="just a sentence.")
|
||||
monkeypatch.setattr(triage_module, "fetch_pr", lambda repo, n: pr)
|
||||
self._stub_grace_aged_out(triage_module, monkeypatch)
|
||||
posted = {}
|
||||
closed = {}
|
||||
monkeypatch.setattr(
|
||||
|
|
@ -784,6 +798,30 @@ class TestTriageOrchestration:
|
|||
lambda *a, **kw: None,
|
||||
)
|
||||
|
||||
@staticmethod
|
||||
def _stub_grace_aged_out(triage_module, monkeypatch):
|
||||
"""Pretend the grace warning has aged out.
|
||||
|
||||
For tests that exercise the post-grace close path. Set the age
|
||||
to twice the grace window so a future tweak to
|
||||
`GRACE_PERIOD_SECONDS` doesn't accidentally make the stub fall
|
||||
back inside the window.
|
||||
"""
|
||||
monkeypatch.setattr(
|
||||
triage_module,
|
||||
"seconds_since_last_grace_warning",
|
||||
lambda *a, **kw: triage_module.GRACE_PERIOD_SECONDS * 2,
|
||||
)
|
||||
|
||||
@staticmethod
|
||||
def _stub_grace_no_warning(triage_module, monkeypatch):
|
||||
"""Pretend no grace warning has been posted yet (first detection)."""
|
||||
monkeypatch.setattr(
|
||||
triage_module,
|
||||
"seconds_since_last_grace_warning",
|
||||
lambda *a, **kw: None,
|
||||
)
|
||||
|
||||
def test_should_reopen_on_reconsider_pass(self, triage_module, monkeypatch):
|
||||
# Reconsider on a closed PR with a passing verdict -> reopen + post a
|
||||
# friendly "re-evaluated" comment. close=True is the production path
|
||||
|
|
@ -1214,6 +1252,9 @@ class TestTriageOrchestration:
|
|||
"user": {"login": "outside"},
|
||||
}
|
||||
monkeypatch.setattr(triage_module, "fetch_issue", lambda repo, n: issue)
|
||||
# Grace already aged out -> close path. (Issues use the same
|
||||
# GRACE_COMMENT_MARKER detection as PRs.)
|
||||
self._stub_grace_aged_out(triage_module, monkeypatch)
|
||||
closed = {}
|
||||
posted = {}
|
||||
monkeypatch.setattr(
|
||||
|
|
@ -1243,3 +1284,432 @@ class TestTriageOrchestration:
|
|||
assert result["action"] == "closed"
|
||||
assert closed["n"] == 7
|
||||
assert "reproduction" in posted["body"]
|
||||
|
||||
# ---- Grace-period flow ------------------------------------------------
|
||||
|
||||
def test_should_post_grace_warning_on_first_failing_run_in_close_mode(
|
||||
self, triage_module, monkeypatch
|
||||
):
|
||||
# First low-quality detection -> bot posts a warning comment with
|
||||
# the GRACE_COMMENT_MARKER. The PR must NOT be closed yet.
|
||||
pr = self._make_pr(body="just a sentence.")
|
||||
monkeypatch.setattr(triage_module, "fetch_pr", lambda repo, n: pr)
|
||||
self._stub_grace_no_warning(triage_module, monkeypatch)
|
||||
posted = {}
|
||||
monkeypatch.setattr(
|
||||
triage_module,
|
||||
"post_comment",
|
||||
lambda repo, n, body: posted.update({"n": n, "body": body}),
|
||||
)
|
||||
monkeypatch.setattr(
|
||||
triage_module,
|
||||
"close_pr",
|
||||
lambda *a, **kw: pytest.fail("must not close on first detection"),
|
||||
)
|
||||
|
||||
verdict = {
|
||||
"verdict": "fail",
|
||||
"missing": ["QA proof"],
|
||||
"explanation": "Body too thin.",
|
||||
}
|
||||
result = triage_module.triage(
|
||||
repo="o/r",
|
||||
kind="pr",
|
||||
number=42,
|
||||
close=True,
|
||||
model="m",
|
||||
judge=lambda p: json.dumps(verdict),
|
||||
)
|
||||
assert result["action"] == "warned-grace"
|
||||
assert posted["n"] == 42
|
||||
# Pin the user-facing language pieces the user explicitly asked for.
|
||||
assert "1 day" in posted["body"]
|
||||
assert "@agent-shin reconsider" in posted["body"]
|
||||
assert "@greptileai" in posted["body"]
|
||||
assert "even after the PR is closed" in posted["body"]
|
||||
assert triage_module.GRACE_COMMENT_MARKER in posted["body"]
|
||||
|
||||
def test_should_skip_close_inside_grace_window(self, triage_module, monkeypatch):
|
||||
# A warning was posted recently; do nothing on this run regardless
|
||||
# of close=True. The next run after `GRACE_PERIOD_SECONDS` elapses
|
||||
# is the one that flips to actual close.
|
||||
pr = self._make_pr(body="just a sentence.")
|
||||
monkeypatch.setattr(triage_module, "fetch_pr", lambda repo, n: pr)
|
||||
monkeypatch.setattr(
|
||||
triage_module,
|
||||
"seconds_since_last_grace_warning",
|
||||
lambda *a, **kw: 60.0,
|
||||
)
|
||||
monkeypatch.setattr(
|
||||
triage_module,
|
||||
"post_comment",
|
||||
lambda *a, **kw: pytest.fail("must not comment during grace window"),
|
||||
)
|
||||
monkeypatch.setattr(
|
||||
triage_module,
|
||||
"close_pr",
|
||||
lambda *a, **kw: pytest.fail("must not close during grace window"),
|
||||
)
|
||||
|
||||
verdict = {
|
||||
"verdict": "fail",
|
||||
"missing": ["QA proof"],
|
||||
"explanation": "Body too thin.",
|
||||
}
|
||||
result = triage_module.triage(
|
||||
repo="o/r",
|
||||
kind="pr",
|
||||
number=42,
|
||||
close=True,
|
||||
model="m",
|
||||
judge=lambda p: json.dumps(verdict),
|
||||
)
|
||||
assert result["action"] == "skip-in-grace-period"
|
||||
assert result["grace_age_seconds"] == 60.0
|
||||
assert result["grace_period_seconds"] == triage_module.GRACE_PERIOD_SECONDS
|
||||
|
||||
def test_should_dry_run_grace_warning_when_close_false(
|
||||
self, triage_module, monkeypatch
|
||||
):
|
||||
# In dry-run mode the FIRST failing detection returns
|
||||
# `would-warn-grace` (with the previewed comment body) and never
|
||||
# touches GitHub state. Lets a local operator preview the
|
||||
# warning before flipping --close on.
|
||||
pr = self._make_pr(body="thin")
|
||||
monkeypatch.setattr(triage_module, "fetch_pr", lambda repo, n: pr)
|
||||
self._stub_grace_no_warning(triage_module, monkeypatch)
|
||||
monkeypatch.setattr(
|
||||
triage_module,
|
||||
"post_comment",
|
||||
lambda *a, **kw: pytest.fail("must not post in dry-run grace warn"),
|
||||
)
|
||||
|
||||
verdict = {
|
||||
"verdict": "fail",
|
||||
"missing": ["QA proof"],
|
||||
"explanation": "thin",
|
||||
}
|
||||
result = triage_module.triage(
|
||||
repo="o/r",
|
||||
kind="pr",
|
||||
number=1,
|
||||
close=False,
|
||||
model="m",
|
||||
judge=lambda p: json.dumps(verdict),
|
||||
)
|
||||
assert result["action"] == "would-warn-grace"
|
||||
assert "1 day" in result["comment"]
|
||||
|
||||
# ---- IMMEDIATE_CLOSE_LOGINS bypass (e.g. SwiftWinds) -----------------
|
||||
|
||||
def test_should_skip_grace_for_swiftwinds_login(self, triage_module, monkeypatch):
|
||||
# SwiftWinds is in `IMMEDIATE_CLOSE_LOGINS` for dogfooding.
|
||||
# Even though no grace warning has been posted, the bypass must
|
||||
# take the close path immediately.
|
||||
pr = self._make_pr(body="just a sentence.", user={"login": "SwiftWinds"})
|
||||
monkeypatch.setattr(triage_module, "fetch_pr", lambda repo, n: pr)
|
||||
# The grace check must NOT be consulted at all for an immediate
|
||||
# login — pin that here.
|
||||
monkeypatch.setattr(
|
||||
triage_module,
|
||||
"seconds_since_last_grace_warning",
|
||||
lambda *a, **kw: pytest.fail(
|
||||
"grace check must not run for immediate-close login"
|
||||
),
|
||||
)
|
||||
posted = {}
|
||||
closed = {}
|
||||
monkeypatch.setattr(
|
||||
triage_module,
|
||||
"post_comment",
|
||||
lambda repo, n, body: posted.update({"n": n, "body": body}),
|
||||
)
|
||||
monkeypatch.setattr(
|
||||
triage_module,
|
||||
"close_pr",
|
||||
lambda repo, n: closed.update({"n": n}),
|
||||
)
|
||||
|
||||
verdict = {
|
||||
"verdict": "fail",
|
||||
"missing": ["QA proof"],
|
||||
"explanation": "Body too thin.",
|
||||
}
|
||||
result = triage_module.triage(
|
||||
repo="o/r",
|
||||
kind="pr",
|
||||
number=99,
|
||||
close=True,
|
||||
model="m",
|
||||
judge=lambda p: json.dumps(verdict),
|
||||
)
|
||||
assert result["action"] == "closed"
|
||||
assert result["immediate_close"] is True
|
||||
assert closed["n"] == 99
|
||||
# The close comment is the standard one (no warning was needed).
|
||||
assert "Agent Shin" in posted["body"]
|
||||
|
||||
def test_should_close_swiftwinds_even_when_close_flag_false(
|
||||
self, triage_module, monkeypatch
|
||||
):
|
||||
# The `IMMEDIATE_CLOSE_LOGINS` bypass intentionally overrides the
|
||||
# workflow-level dry-run gating. The user explicitly asked to
|
||||
# "turn on full (non dry run) mode with SwiftWinds" because their
|
||||
# personal account has no push permissions to litellm and is the
|
||||
# perfect testing target. With `close=False` (workflow stripped
|
||||
# --close on pull_request_target), the script must STILL post +
|
||||
# close for SwiftWinds.
|
||||
pr = self._make_pr(body="just a sentence.", user={"login": "SwiftWinds"})
|
||||
monkeypatch.setattr(triage_module, "fetch_pr", lambda repo, n: pr)
|
||||
posted = {}
|
||||
closed = {}
|
||||
monkeypatch.setattr(
|
||||
triage_module,
|
||||
"post_comment",
|
||||
lambda repo, n, body: posted.update({"n": n, "body": body}),
|
||||
)
|
||||
monkeypatch.setattr(
|
||||
triage_module,
|
||||
"close_pr",
|
||||
lambda repo, n: closed.update({"n": n}),
|
||||
)
|
||||
|
||||
verdict = {
|
||||
"verdict": "fail",
|
||||
"missing": ["QA proof"],
|
||||
"explanation": "thin",
|
||||
}
|
||||
result = triage_module.triage(
|
||||
repo="o/r",
|
||||
kind="pr",
|
||||
number=99,
|
||||
close=False,
|
||||
model="m",
|
||||
judge=lambda p: json.dumps(verdict),
|
||||
)
|
||||
assert result["action"] == "closed"
|
||||
assert closed["n"] == 99
|
||||
|
||||
def test_should_match_immediate_close_login_case_insensitively(
|
||||
self, triage_module, monkeypatch
|
||||
):
|
||||
# GitHub returns the original casing of a login. The bypass must
|
||||
# work regardless of the exact case the API returns.
|
||||
for login in ("SwiftWinds", "swiftwinds", "SWIFTWINDS"):
|
||||
pr = self._make_pr(body="thin", user={"login": login})
|
||||
monkeypatch.setattr(triage_module, "fetch_pr", lambda repo, n: pr)
|
||||
monkeypatch.setattr(
|
||||
triage_module,
|
||||
"post_comment",
|
||||
lambda *a, **kw: None,
|
||||
)
|
||||
monkeypatch.setattr(
|
||||
triage_module,
|
||||
"close_pr",
|
||||
lambda *a, **kw: None,
|
||||
)
|
||||
verdict = {
|
||||
"verdict": "fail",
|
||||
"missing": ["QA proof"],
|
||||
"explanation": "thin",
|
||||
}
|
||||
result = triage_module.triage(
|
||||
repo="o/r",
|
||||
kind="pr",
|
||||
number=1,
|
||||
close=False,
|
||||
model="m",
|
||||
judge=lambda p: json.dumps(verdict),
|
||||
)
|
||||
assert result["action"] == "closed", login
|
||||
assert result.get("immediate_close") is True, login
|
||||
|
||||
def test_should_not_treat_random_external_login_as_immediate_close(
|
||||
self, triage_module, monkeypatch
|
||||
):
|
||||
# Sanity check — the bypass must NOT fire for any login other
|
||||
# than the explicitly-listed test accounts.
|
||||
pr = self._make_pr(body="thin", user={"login": "random-oss-dev"})
|
||||
monkeypatch.setattr(triage_module, "fetch_pr", lambda repo, n: pr)
|
||||
self._stub_grace_no_warning(triage_module, monkeypatch)
|
||||
posted = {}
|
||||
monkeypatch.setattr(
|
||||
triage_module,
|
||||
"post_comment",
|
||||
lambda repo, n, body: posted.update({"body": body}),
|
||||
)
|
||||
monkeypatch.setattr(
|
||||
triage_module,
|
||||
"close_pr",
|
||||
lambda *a, **kw: pytest.fail("must not close non-immediate login"),
|
||||
)
|
||||
|
||||
verdict = {
|
||||
"verdict": "fail",
|
||||
"missing": ["QA proof"],
|
||||
"explanation": "thin",
|
||||
}
|
||||
result = triage_module.triage(
|
||||
repo="o/r",
|
||||
kind="pr",
|
||||
number=1,
|
||||
close=True,
|
||||
model="m",
|
||||
judge=lambda p: json.dumps(verdict),
|
||||
)
|
||||
assert result["action"] == "warned-grace"
|
||||
|
||||
|
||||
class TestImmediateCloseLoginsConstant:
|
||||
"""SwiftWinds is the dogfood account the user explicitly named — pin
|
||||
its presence so a future cleanup that removes the constant or
|
||||
forgets to keep the entry doesn't silently break the test path."""
|
||||
|
||||
def test_should_include_swiftwinds(self, triage_module):
|
||||
assert "swiftwinds" in triage_module.IMMEDIATE_CLOSE_LOGINS
|
||||
|
||||
def test_should_be_lowercase_for_case_insensitive_match(self, triage_module):
|
||||
for login in triage_module.IMMEDIATE_CLOSE_LOGINS:
|
||||
assert login == login.lower(), login
|
||||
|
||||
|
||||
class TestGraceWarningCommentText:
|
||||
"""Pin the user-facing promises in the grace warning so a future
|
||||
refactor can't silently drop them."""
|
||||
|
||||
def test_pr_grace_warning_should_state_one_day_grace(self, triage_module):
|
||||
body = triage_module.format_grace_warning_pr_comment(
|
||||
{"verdict": "fail", "missing": ["QA proof"], "explanation": "thin"}
|
||||
)
|
||||
# The user explicitly asked: "specify in the comment" that there
|
||||
# is a 1-day grace.
|
||||
assert "1 day" in body
|
||||
|
||||
def test_pr_grace_warning_should_mention_reconsider_during_grace(
|
||||
self, triage_module
|
||||
):
|
||||
body = triage_module.format_grace_warning_pr_comment(
|
||||
{"verdict": "fail", "missing": [], "explanation": ""}
|
||||
)
|
||||
assert "@agent-shin reconsider" in body
|
||||
|
||||
def test_pr_grace_warning_should_promise_greptileai_works_post_close(
|
||||
self, triage_module
|
||||
):
|
||||
body = triage_module.format_grace_warning_pr_comment(
|
||||
{"verdict": "fail", "missing": [], "explanation": ""}
|
||||
)
|
||||
# Per user: comment should state @greptileai works even after close.
|
||||
assert "@greptileai" in body
|
||||
assert "even after the PR is closed" in body
|
||||
|
||||
def test_pr_grace_warning_should_carry_grace_marker(self, triage_module):
|
||||
# The marker is what `seconds_since_last_grace_warning` greps for
|
||||
# on subsequent runs to detect that a warning has been posted.
|
||||
# Dropping it would silently break the close-after-grace path.
|
||||
body = triage_module.format_grace_warning_pr_comment(
|
||||
{"verdict": "fail", "missing": [], "explanation": ""}
|
||||
)
|
||||
assert triage_module.GRACE_COMMENT_MARKER in body
|
||||
|
||||
def test_issue_grace_warning_should_carry_grace_marker(self, triage_module):
|
||||
body = triage_module.format_grace_warning_issue_comment(
|
||||
{"verdict": "fail", "missing": [], "explanation": ""}
|
||||
)
|
||||
assert triage_module.GRACE_COMMENT_MARKER in body
|
||||
assert "1 day" in body
|
||||
assert "@agent-shin reconsider" in body
|
||||
|
||||
def test_pr_close_comment_should_promise_greptileai_works_post_close(
|
||||
self, triage_module
|
||||
):
|
||||
# The standard close comment must ALSO point at @greptileai so
|
||||
# contributors see the same options whether they read the warning
|
||||
# or only catch the close comment.
|
||||
body = triage_module.format_pr_close_comment(
|
||||
{"verdict": "fail", "missing": [], "explanation": ""}
|
||||
)
|
||||
assert "@greptileai" in body
|
||||
assert "even after the PR is closed" in body
|
||||
|
||||
|
||||
class TestSecondsSinceLastGraceWarning:
|
||||
"""Mirror of TestSecondsSinceLastReconsiderVerdict for the new helper.
|
||||
Both helpers share `_seconds_since_latest_marker_comment` underneath
|
||||
so the parsing logic is exercised either way; these tests pin the
|
||||
grace-marker-specific behavior."""
|
||||
|
||||
def _make_comment(
|
||||
self,
|
||||
*,
|
||||
login: str,
|
||||
body: str,
|
||||
created_at: str | None = "2026-05-18T05:00:00Z",
|
||||
) -> dict:
|
||||
comment: dict = {"user": {"login": login}, "body": body}
|
||||
if created_at is not None:
|
||||
comment["created_at"] = created_at
|
||||
return comment
|
||||
|
||||
def test_should_return_none_when_no_grace_marker(self, triage_module, monkeypatch):
|
||||
comments = [
|
||||
self._make_comment(
|
||||
login="github-actions[bot]",
|
||||
body="Some other bot message",
|
||||
),
|
||||
self._make_comment(login="random-user", body="ping?"),
|
||||
]
|
||||
monkeypatch.setattr(
|
||||
triage_module, "_iter_paginated_json", lambda *a, **kw: iter(comments)
|
||||
)
|
||||
assert triage_module.seconds_since_last_grace_warning("o/r", 1) is None
|
||||
|
||||
def test_should_ignore_non_bot_comments_with_marker(
|
||||
self, triage_module, monkeypatch
|
||||
):
|
||||
# A user who quotes the marker in a question must NOT be treated
|
||||
# as the bot warning; otherwise the close-after-grace path would
|
||||
# never fire because the timer keeps resetting.
|
||||
comments = [
|
||||
self._make_comment(
|
||||
login="random-user",
|
||||
body=f"What is {triage_module.GRACE_COMMENT_MARKER}?",
|
||||
)
|
||||
]
|
||||
monkeypatch.setattr(
|
||||
triage_module, "_iter_paginated_json", lambda *a, **kw: iter(comments)
|
||||
)
|
||||
assert triage_module.seconds_since_last_grace_warning("o/r", 1) is None
|
||||
|
||||
def test_should_pick_latest_grace_marker(self, triage_module, monkeypatch):
|
||||
comments = [
|
||||
self._make_comment(
|
||||
login="github-actions[bot]",
|
||||
body="old warning " + triage_module.GRACE_COMMENT_MARKER,
|
||||
created_at="2026-05-18T03:00:00Z",
|
||||
),
|
||||
self._make_comment(
|
||||
login="github-actions[bot]",
|
||||
body="newer warning " + triage_module.GRACE_COMMENT_MARKER,
|
||||
created_at="2026-05-18T04:55:00Z",
|
||||
),
|
||||
]
|
||||
monkeypatch.setattr(
|
||||
triage_module, "_iter_paginated_json", lambda *a, **kw: iter(comments)
|
||||
)
|
||||
|
||||
import datetime as real_dt
|
||||
|
||||
class FrozenDateTime(real_dt.datetime):
|
||||
@classmethod
|
||||
def now(cls, tz=None):
|
||||
return real_dt.datetime(2026, 5, 18, 5, 0, 0, tzinfo=tz)
|
||||
|
||||
frozen_module = type(triage_module.dt)("datetime")
|
||||
frozen_module.datetime = FrozenDateTime
|
||||
frozen_module.timezone = real_dt.timezone
|
||||
monkeypatch.setattr(triage_module, "dt", frozen_module)
|
||||
|
||||
age = triage_module.seconds_since_last_grace_warning("o/r", 1)
|
||||
# Newer warning is 5 minutes (300s) before "now".
|
||||
assert age == 300.0
|
||||
|
|
|
|||
Loading…
Add table
Reference in a new issue