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