From 1d0d4908478640dd52e6deecc6bad7defed95096 Mon Sep 17 00:00:00 2001 From: Cursor Agent Date: Mon, 25 May 2026 01:58:18 +0000 Subject: [PATCH] agent_shin: fix bug bundle (gated LLM key, author-filtered marker dedup, dedup gh/grace helpers) Co-authored-by: Yassin Kortam --- .github/scripts/agent_shin_shared.py | 66 ++++++++++++++ .github/scripts/close_low_quality_prs.py | 71 ++++----------- .github/scripts/triage_with_llm.py | 87 +++++++++---------- .github/workflows/triage_issue_with_llm.yml | 8 +- .github/workflows/triage_pr_with_llm.yml | 8 +- tests/test_litellm/test_github_review_gate.py | 16 +++- 6 files changed, 155 insertions(+), 101 deletions(-) diff --git a/.github/scripts/agent_shin_shared.py b/.github/scripts/agent_shin_shared.py index 6b763a2e20b..492aaed438c 100644 --- a/.github/scripts/agent_shin_shared.py +++ b/.github/scripts/agent_shin_shared.py @@ -27,7 +27,9 @@ enforced it. from __future__ import annotations import datetime as dt +import os import re +import subprocess from typing import Iterable GREPTILE_BOT_LOGINS = frozenset({"greptile-apps", "greptile-apps[bot]"}) @@ -77,3 +79,67 @@ def extract_greptile_score(comments: Iterable[dict]) -> tuple[int, dict] | None: def parse_iso8601(value: str) -> dt.datetime: """Parse a GitHub ISO-8601 timestamp into a timezone-aware datetime.""" return dt.datetime.fromisoformat(value.replace("Z", "+00:00")) + + +def gh(*args: str) -> str: + """Run a `gh` CLI command and return stdout. Raises on non-zero exit. + + Shared by both Agent Shin entrypoints so a future change here + (timeout handling, logging, retry on transient failures) only needs + to be made once. + """ + result = subprocess.run( + ["gh", *args], + capture_output=True, + text=True, + check=True, + ) + return result.stdout + + +def seconds_since_latest_marker_comment( + comments: Iterable[dict], + *, + marker: str, + bot_login: str | None = None, + now: dt.datetime | None = None, +) -> float | None: + """Return seconds since the bot's most recent comment containing ``marker``. + + Filters comments by author so a contributor who quotes the HTML + marker (e.g. via GitHub's "Quote reply" feature, which preserves + HTML comments in the raw markdown of the quoted text) is not + mistaken for a bot warning — that would silently reset cooldown + timers and suppress legitimate notifications. + + ``bot_login`` defaults to the `AGENT_SHIN_BOT_LOGIN` env override or + ``AGENT_SHIN_DEFAULT_BOT_LOGIN`` so callers normally don't need to + pass it. ``now`` is injectable for tests / callers (like the daily + sweep) that want every age calculation pinned to one snapshot. + """ + 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 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() diff --git a/.github/scripts/close_low_quality_prs.py b/.github/scripts/close_low_quality_prs.py index 5e9832ccd85..59d9797585f 100644 --- a/.github/scripts/close_low_quality_prs.py +++ b/.github/scripts/close_low_quality_prs.py @@ -49,14 +49,15 @@ from typing import Iterable sys.path.insert(0, os.path.dirname(os.path.abspath(__file__))) from agent_shin_shared import ( # noqa: E402 -- sys.path adjusted above - AGENT_SHIN_DEFAULT_BOT_LOGIN, GRACE_COMMENT_MARKER, GRACE_PERIOD_SECONDS, GREPTILE_BOT_LOGINS, IMMEDIATE_CLOSE_LOGINS, SCORE_PATTERN, extract_greptile_score, + gh, parse_iso8601, + seconds_since_latest_marker_comment, ) # `GREPTILE_BOT_LOGINS` and `SCORE_PATTERN` (Greptile's GitHub App login @@ -82,24 +83,13 @@ DEFAULT_OPTOUT_LABELS = ("do not close", "keep open", "wip") # imported from `agent_shin_shared` so the Agent Shin LLM judge and # this daily Greptile sweep agree on the same marker and duration. # -# `AGENT_SHIN_DEFAULT_BOT_LOGIN` (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` works the same here) and # `IMMEDIATE_CLOSE_LOGINS` (case-insensitive logins that bypass BOTH # the grace period AND the dry-run gating; useful for dogfooding the -# bot from external test accounts) are imported too. - - -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 +# bot from external test accounts) is imported too. Author-matching +# against the bot login (`AGENT_SHIN_DEFAULT_BOT_LOGIN` plus the +# `AGENT_SHIN_BOT_LOGIN` env override) now happens inside +# `seconds_since_latest_marker_comment` itself, so this script no +# longer needs to reach for the constant directly. def fetch_open_prs(repo: str | None) -> list[dict]: @@ -196,42 +186,19 @@ def seconds_since_last_grace_warning( """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. + Thin wrapper over + `agent_shin_shared.seconds_since_latest_marker_comment` — the + centralized helper handles the bot-author filter, marker match, + timestamp parsing, and `now` injection. Keeping this wrapper + preserves the closer's "already-fetched comments + injectable now" + interface so callers (and tests) don't need to change. """ - 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() + return seconds_since_latest_marker_comment( + comments, + marker=GRACE_COMMENT_MARKER, + bot_login=bot_login, + now=now, + ) def format_grace_warning_comment(score: int, threshold: int) -> str: diff --git a/.github/scripts/triage_with_llm.py b/.github/scripts/triage_with_llm.py index a47b5c782a1..edcc6371d46 100644 --- a/.github/scripts/triage_with_llm.py +++ b/.github/scripts/triage_with_llm.py @@ -55,7 +55,9 @@ from agent_shin_shared import ( # noqa: E402 -- sys.path adjusted above IMMEDIATE_CLOSE_LOGINS, SCORE_PATTERN, extract_greptile_score, + gh, parse_iso8601, + seconds_since_latest_marker_comment, ) DEFAULT_MODEL = "gpt-5.4-mini" @@ -152,17 +154,9 @@ 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 +# +# `gh` is imported from `agent_shin_shared` so a future change (timeout, +# logging, retry) only needs to be made once. def fetch_pr(repo: str, number: int) -> dict: @@ -339,39 +333,22 @@ def _seconds_since_latest_marker_comment( marker: str, bot_login: str | None = None, ) -> float | None: - """Shared helper: return seconds since the bot's most recent comment - that contains the given HTML marker, or None if no such comment exists. + """Return seconds since the bot's most recent comment with ``marker``. - 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). + Fetches comments via `_iter_paginated_json` and delegates the + iteration / author-filter / timestamp logic to + `agent_shin_shared.seconds_since_latest_marker_comment` so the daily + Greptile sweep and the LLM judge use one source of truth for the + "bot already posted X" detection. The wall-clock `now` is resolved + against this module's `dt` so tests that freeze time via + `monkeypatch.setattr(triage_module, "dt", ...)` still apply. """ - expected_login = ( - bot_login - or os.environ.get("AGENT_SHIN_BOT_LOGIN") - or AGENT_SHIN_DEFAULT_BOT_LOGIN - ).lower() - latest: dt.datetime | None = None - for comment in _iter_paginated_json(f"repos/{repo}/issues/{number}/comments"): - author = ((comment.get("user") or {}).get("login") or "").lower() - if author != expected_login: - continue - body = comment.get("body") or "" - if marker not in body: - continue - created = comment.get("created_at") - if not created: - continue - try: - ts = dt.datetime.fromisoformat(created.replace("Z", "+00:00")) - except ValueError: - continue - if latest is None or ts > latest: - latest = ts - if latest is None: - return None - return (dt.datetime.now(dt.timezone.utc) - latest).total_seconds() + return seconds_since_latest_marker_comment( + _iter_paginated_json(f"repos/{repo}/issues/{number}/comments"), + marker=marker, + bot_login=bot_login, + now=dt.datetime.now(dt.timezone.utc), + ) def seconds_since_last_reconsider_verdict( @@ -861,8 +838,30 @@ def _combine_missing( 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 _has_marker( + comments: Iterable[dict], marker: str, *, bot_login: str | None = None +) -> bool: + """Return True iff the bot itself posted a comment containing ``marker``. + + Filters by author so a contributor who quotes the marker (e.g. via + GitHub's "Quote reply" feature, which preserves HTML comments in + raw markdown) is not mistaken for a bot action — that would + silently suppress notifications or change which "recovered" wording + is selected. Matches the author-filter pattern used by the sibling + `_seconds_since_latest_marker_comment` helper. + """ + expected_login = ( + bot_login + or os.environ.get("AGENT_SHIN_BOT_LOGIN") + or AGENT_SHIN_DEFAULT_BOT_LOGIN + ).lower() + for comment in comments: + author = ((comment.get("user") or {}).get("login") or "").lower() + if author != expected_login: + continue + if marker in (comment.get("body") or ""): + return True + return False def format_ready_for_review_comment(verdict: dict, greptile_score: int | None) -> str: diff --git a/.github/workflows/triage_issue_with_llm.yml b/.github/workflows/triage_issue_with_llm.yml index ff0497f9893..5161165beef 100644 --- a/.github/workflows/triage_issue_with_llm.yml +++ b/.github/workflows/triage_issue_with_llm.yml @@ -49,7 +49,13 @@ jobs: - name: Run Agent Shin env: GH_TOKEN: ${{ secrets.GITHUB_TOKEN }} - OPENAI_API_KEY: ${{ secrets.OPENAI_API_KEY }} + # 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 issues while the bot is still in dry-run. + # The Python script calls the LLM whenever this var is set + # (regardless of `--close`); stripping `--close` doesn't suppress + # the API call, only the destructive side effects. + 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 }} diff --git a/.github/workflows/triage_pr_with_llm.yml b/.github/workflows/triage_pr_with_llm.yml index eac7e6a56b3..4aee1b14305 100644 --- a/.github/workflows/triage_pr_with_llm.yml +++ b/.github/workflows/triage_pr_with_llm.yml @@ -60,7 +60,13 @@ jobs: - name: Run Agent Shin env: GH_TOKEN: ${{ secrets.GITHUB_TOKEN }} - OPENAI_API_KEY: ${{ secrets.OPENAI_API_KEY }} + # 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. + # The Python script calls the LLM whenever this var is set + # (regardless of `--close`); stripping `--close` doesn't suppress + # the API call, only the destructive side effects. + 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 }} diff --git a/tests/test_litellm/test_github_review_gate.py b/tests/test_litellm/test_github_review_gate.py index 635aec17c97..27f6c8c0db2 100644 --- a/tests/test_litellm/test_github_review_gate.py +++ b/tests/test_litellm/test_github_review_gate.py @@ -147,7 +147,12 @@ class TestReviewGatePass: # 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}] + prior = [ + { + "user": {"login": "github-actions[bot]"}, + "body": triage_module.REGRESSED_MARKER, + } + ] result = _gate(triage_module, judge=_pass, greptile_score=5, comments=prior) @@ -241,7 +246,12 @@ class TestReviewGateGraceAndClose: 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}] + prior = [ + { + "user": {"login": "github-actions[bot]"}, + "body": triage_module.WITHIN_GRACE_MARKER, + } + ] result = _gate(triage_module, judge=_fail, greptile_score=None, comments=prior) @@ -345,7 +355,7 @@ class TestReviewGateGuards: triage_module, "post_comment", lambda repo, n, body: state["comments"].append( - {"user": {"login": "agent-shin[bot]"}, "body": body} + {"user": {"login": "github-actions[bot]"}, "body": body} ), ) monkeypatch.setattr(