mirror of
https://github.com/BerriAI/litellm.git
synced 2026-09-14 23:21:35 +00:00
fix(triage): bugbot — tighten linked-issue regex, fail-safe author_association, fix empty TRIAGE_MODEL
Three independent bugbot findings against triage_with_llm.py: 1. LINKED_ISSUE_PATTERN included weak keywords (`see`, `ref`, `addresses`) so casual mentions like "See #1234 for context" were short-circuited to pass-linked-issue without ever calling the LLM — contradicting the prompt's own "a bare issue number without a closing keyword counts only if it's clearly the related issue (not a passing mention)" rubric. Limit the regex to GitHub's documented PR-closing keywords (fixes/fix/fixed/closes/close/closed/resolves/resolve/resolved). 2. is_internal_contributor() treated an empty/missing author_association as external (eligible for the destructive close path), while the sibling is_external_pr_author() in close_low_quality_prs.py fail-safes the same case as internal. Align the two so a partial/unknown GitHub response can never make a PR eligible for auto-close. 3. argparse `default=os.environ.get("TRIAGE_MODEL", DEFAULT_MODEL)` returns the empty string when GitHub Actions exposes an unset repo variable as an empty-string env var (the optional vars.TRIAGE_MODEL case in the workflow). Use `os.environ.get(...) or DEFAULT_MODEL` so empty -> default, matching the existing OPENAI_BASE_URL pattern. Tests: - Casual mentions now must fall through to the LLM (parametrized); added an orchestration test ensuring "See #1234" reaches the judge. - Empty/missing author_association now fails safe (parametrized). - Empty TRIAGE_MODEL env var falls back to DEFAULT_MODEL; explicit TRIAGE_MODEL is still honored. Co-authored-by: Mateo Wang <mateo-berri@users.noreply.github.com>
This commit is contained in:
parent
c501f74a23
commit
1ac5beea72
2 changed files with 124 additions and 13 deletions
30
.github/scripts/triage_with_llm.py
vendored
30
.github/scripts/triage_with_llm.py
vendored
|
|
@ -50,8 +50,15 @@ INTERNAL_ASSOCIATIONS = frozenset({"OWNER", "MEMBER", "COLLABORATOR"})
|
|||
GPT5_FAMILY_PREFIX = "gpt-5"
|
||||
|
||||
# Regexes for picking off "obvious passes" without burning LLM tokens.
|
||||
#
|
||||
# Keep this list to GitHub's documented PR-closing keywords only
|
||||
# (https://docs.github.com/issues/tracking-your-work-with-issues/linking-a-pull-request-to-an-issue).
|
||||
# Casual mentions like "see #1234" or "ref #1234" are intentionally NOT
|
||||
# auto-passed — they should fall through to the LLM judge, which has the
|
||||
# stricter rubric "a bare issue number without a closing keyword counts only
|
||||
# if it's clearly the related issue (not a passing mention)".
|
||||
LINKED_ISSUE_PATTERN = re.compile(
|
||||
r"\b(?:fixes|fix|closes|close|resolves|resolve|refs|ref|see|addresses)\s+"
|
||||
r"\b(?:fixes|fix|fixed|closes|close|closed|resolves|resolve|resolved)\s+"
|
||||
r"(?:#\d+|https?://github\.com/[\w.-]+/[\w.-]+/issues/\d+)",
|
||||
re.IGNORECASE,
|
||||
)
|
||||
|
|
@ -127,13 +134,20 @@ def close_issue(repo: str, number: int, *, not_planned: bool = True) -> None:
|
|||
|
||||
|
||||
def is_internal_contributor(item: dict) -> bool:
|
||||
"""Return True if the PR/issue author should be exempted from triage."""
|
||||
association = (item.get("author_association") or "").upper()
|
||||
if association in INTERNAL_ASSOCIATIONS:
|
||||
return True
|
||||
"""Return True if the PR/issue author should be exempted from triage.
|
||||
|
||||
Fail-safe: if `author_association` is missing or empty (which should never
|
||||
happen on a successful GitHub REST response but is possible on schema
|
||||
changes or partial responses), treat the author as INTERNAL so the
|
||||
destructive close path never fires on an unknown contributor. This matches
|
||||
the sibling `is_external_pr_author` in `close_low_quality_prs.py`.
|
||||
"""
|
||||
login = ((item.get("user") or {}).get("login") or "").lower()
|
||||
if login.endswith("[bot]") or login in {"dependabot", "github-actions"}:
|
||||
return True
|
||||
association = (item.get("author_association") or "").upper()
|
||||
if not association or association in INTERNAL_ASSOCIATIONS:
|
||||
return True
|
||||
return False
|
||||
|
||||
|
||||
|
|
@ -553,7 +567,11 @@ def main() -> int:
|
|||
)
|
||||
parser.add_argument(
|
||||
"--model",
|
||||
default=os.environ.get("TRIAGE_MODEL", DEFAULT_MODEL),
|
||||
# `os.environ.get("TRIAGE_MODEL", DEFAULT_MODEL)` would return "" when
|
||||
# GitHub Actions exposes an unset repo variable as an empty-string env
|
||||
# var, silently bypassing DEFAULT_MODEL and causing every call to fail
|
||||
# as `skip-llm-error`. The `or` guard collapses empty -> default.
|
||||
default=os.environ.get("TRIAGE_MODEL") or DEFAULT_MODEL,
|
||||
help=f"OpenAI-compatible model name (default: {DEFAULT_MODEL}).",
|
||||
)
|
||||
parser.add_argument(
|
||||
|
|
|
|||
|
|
@ -35,7 +35,7 @@ class TestIsInternalContributor:
|
|||
|
||||
@pytest.mark.parametrize(
|
||||
"association",
|
||||
["CONTRIBUTOR", "FIRST_TIME_CONTRIBUTOR", "FIRST_TIMER", "NONE", ""],
|
||||
["CONTRIBUTOR", "FIRST_TIME_CONTRIBUTOR", "FIRST_TIMER", "NONE"],
|
||||
)
|
||||
def test_should_mark_outside_associations_as_external(
|
||||
self, triage_module, association
|
||||
|
|
@ -46,6 +46,20 @@ class TestIsInternalContributor:
|
|||
}
|
||||
assert triage_module.is_internal_contributor(item) is False
|
||||
|
||||
@pytest.mark.parametrize(
|
||||
"item",
|
||||
[
|
||||
{"author_association": "", "user": {"login": "random-oss-dev"}},
|
||||
{"user": {"login": "random-oss-dev"}}, # association field absent
|
||||
],
|
||||
)
|
||||
def test_should_fail_safe_when_author_association_is_missing(
|
||||
self, triage_module, item
|
||||
):
|
||||
# Fail-safe: an empty/missing association must never make a PR
|
||||
# eligible for the destructive close path. Treat as internal (skip).
|
||||
assert triage_module.is_internal_contributor(item) is True
|
||||
|
||||
@pytest.mark.parametrize(
|
||||
"login",
|
||||
["dependabot[bot]", "greptile-apps[bot]", "dependabot", "github-actions"],
|
||||
|
|
@ -65,7 +79,8 @@ class TestHasLinkedIssue:
|
|||
"closes #1",
|
||||
"Resolves #99",
|
||||
"fix #42 — this addresses the regression",
|
||||
"Refs https://github.com/BerriAI/litellm/issues/27000",
|
||||
"Closes https://github.com/BerriAI/litellm/issues/27000",
|
||||
"Resolved https://github.com/BerriAI/litellm/issues/27001",
|
||||
],
|
||||
)
|
||||
def test_should_detect_common_link_phrases(self, triage_module, body):
|
||||
|
|
@ -76,13 +91,17 @@ class TestHasLinkedIssue:
|
|||
[
|
||||
"",
|
||||
"Some change",
|
||||
"See #1234", # "see" is allowed per regex but we want documented coverage
|
||||
# Casual mentions must NOT auto-pass — they should fall through to
|
||||
# the LLM judge so the stricter "not a passing mention" rule fires.
|
||||
"See #1234",
|
||||
"see #1234 for context",
|
||||
"ref #1234",
|
||||
"Refs https://github.com/BerriAI/litellm/issues/27000",
|
||||
"this addresses #1234",
|
||||
],
|
||||
)
|
||||
def test_should_handle_empty_and_unrelated_bodies(self, triage_module, body):
|
||||
# "See #1234" is intentionally accepted as a related-issue reference.
|
||||
# Just make sure empty/unrelated bodies don't crash.
|
||||
triage_module.has_linked_issue(body)
|
||||
def test_should_not_auto_pass_casual_mentions(self, triage_module, body):
|
||||
assert triage_module.has_linked_issue(body) is False
|
||||
|
||||
def test_should_not_detect_when_only_html_comment_template(self, triage_module):
|
||||
body = "<!-- e.g. Fixes #1234 -->"
|
||||
|
|
@ -147,6 +166,54 @@ class TestBuildPrompts:
|
|||
assert "repro here" in prompt
|
||||
|
||||
|
||||
class TestMainModelDefault:
|
||||
"""`--model` falls back to DEFAULT_MODEL even when TRIAGE_MODEL is empty."""
|
||||
|
||||
def _stub_triage(self, triage_module, monkeypatch):
|
||||
captured: dict = {}
|
||||
|
||||
def fake_triage(**kwargs):
|
||||
captured.update(kwargs)
|
||||
return {
|
||||
"kind": kwargs["kind"],
|
||||
"number": kwargs["number"],
|
||||
"title": "",
|
||||
"author": "x",
|
||||
"author_association": "NONE",
|
||||
"state": "open",
|
||||
"action": "skip-no-llm-key",
|
||||
}
|
||||
|
||||
monkeypatch.setattr(triage_module, "triage", fake_triage)
|
||||
return captured
|
||||
|
||||
def test_should_fall_back_to_default_when_triage_model_env_empty(
|
||||
self, triage_module, monkeypatch
|
||||
):
|
||||
captured = self._stub_triage(triage_module, monkeypatch)
|
||||
monkeypatch.setenv("TRIAGE_MODEL", "")
|
||||
monkeypatch.setattr(
|
||||
sys,
|
||||
"argv",
|
||||
["triage_with_llm.py", "--repo", "o/r", "--pr", "1"],
|
||||
)
|
||||
rc = triage_module.main()
|
||||
assert rc == 0
|
||||
assert captured["model"] == triage_module.DEFAULT_MODEL
|
||||
|
||||
def test_should_respect_explicit_triage_model_env(self, triage_module, monkeypatch):
|
||||
captured = self._stub_triage(triage_module, monkeypatch)
|
||||
monkeypatch.setenv("TRIAGE_MODEL", "gpt-4o-mini")
|
||||
monkeypatch.setattr(
|
||||
sys,
|
||||
"argv",
|
||||
["triage_with_llm.py", "--repo", "o/r", "--pr", "1"],
|
||||
)
|
||||
rc = triage_module.main()
|
||||
assert rc == 0
|
||||
assert captured["model"] == "gpt-4o-mini"
|
||||
|
||||
|
||||
class TestCallLlmJudge:
|
||||
"""call_llm_judge sets gpt-5 specific kwargs correctly."""
|
||||
|
||||
|
|
@ -296,6 +363,32 @@ class TestTriageOrchestration:
|
|||
assert result["action"] == "pass-linked-issue"
|
||||
assert result["verdict"]["verdict"] == "pass"
|
||||
|
||||
def test_should_not_short_circuit_on_casual_mention(
|
||||
self, triage_module, monkeypatch
|
||||
):
|
||||
# "See #1234" is a passing mention, not a closing keyword. The LLM
|
||||
# must get a chance to apply the stricter rubric.
|
||||
pr = self._make_pr(body="See #1234 for context. No QA proof here.")
|
||||
monkeypatch.setattr(triage_module, "fetch_pr", lambda repo, n: pr)
|
||||
called = {"judge": False}
|
||||
|
||||
def judge(prompt):
|
||||
called["judge"] = True
|
||||
return json.dumps(
|
||||
{"verdict": "fail", "missing": ["QA proof"], "explanation": "thin."}
|
||||
)
|
||||
|
||||
result = triage_module.triage(
|
||||
repo="o/r",
|
||||
kind="pr",
|
||||
number=1,
|
||||
close=False,
|
||||
model="m",
|
||||
judge=judge,
|
||||
)
|
||||
assert called["judge"] is True
|
||||
assert result["action"] == "would-close"
|
||||
|
||||
def test_should_return_pass_llm_when_judge_passes(self, triage_module, monkeypatch):
|
||||
pr = self._make_pr(body="Long body, no linked issue.")
|
||||
monkeypatch.setattr(triage_module, "fetch_pr", lambda repo, n: pr)
|
||||
|
|
|
|||
Loading…
Add table
Reference in a new issue