From 28ba9f7c6b839964d6b52896adb288048201d2bf Mon Sep 17 00:00:00 2001 From: Cursor Agent Date: Mon, 18 May 2026 05:54:47 +0000 Subject: [PATCH] fix(triage): honor --close in --reconsider mode + gate reopen on bot-close provenance MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Address two related concerns raised in PR review on the reconsider flow: 1. **Dry-run support for --reconsider** (P1, greptile-apps): The previous --reconsider branch unconditionally called post_comment + reopen_* regardless of --close. The docstring claimed 'close is forced True implicitly', but the workflow's only kill switch was AGENT_SHIN_ENABLED — invoking the script directly without --close was still destructive, the opposite of the conventional dry-run expectation. triage() now honors close=False in reconsider mode: a passing verdict returns action='would-reopen' (with the comment body it WOULD post in result['comment']) and a failing verdict returns action='would-leave-closed-still-failing'. The reconsider workflow now appends --close iff AGENT_SHIN_ENABLED == 'true', mirroring the pattern used by close_low_quality_prs.yml. 2. **Provenance gate for reopen** (P2, both greptile-apps and veria-ai): The reconsider flow could be used to silently override a maintainer's close decision — an external author edits the closed PR to include a closing keyword, comments '@agent-shin reconsider', and the bot reopens it. There was no check that Agent Shin was the actor that originally closed it. triage() now requires was_auto_closed_by_agent_shin() to be true before any reopen path can fire. The check looks for a bot-authored comment (login ends with '[bot]') containing the auto-close marker 'I'm **Agent Shin**'. Filtering by bot author makes the marker unspoofable: a contributor pasting the phrase into a manual comment cannot satisfy the check. When the provenance gate fails, triage returns action='skip-not-bot-closed' without burning LLM tokens or posting anything. Unit tests cover both behaviors plus the failure modes of was_auto_closed_by_agent_shin (no comments, non-bot author with marker, bot comment without marker, marker found anywhere in the comment list). Co-authored-by: Mateo Wang --- .github/scripts/triage_with_llm.py | 118 +++++++- .github/workflows/triage_reconsider.yml | 18 +- .../test_github_triage_with_llm.py | 282 +++++++++++++++++- 3 files changed, 399 insertions(+), 19 deletions(-) diff --git a/.github/scripts/triage_with_llm.py b/.github/scripts/triage_with_llm.py index 9cf7f04d32f..52d17f769ab 100644 --- a/.github/scripts/triage_with_llm.py +++ b/.github/scripts/triage_with_llm.py @@ -42,6 +42,14 @@ DEFAULT_MODEL = "gpt-5.4-mini" INTERNAL_ASSOCIATIONS = frozenset({"OWNER", "MEMBER", "COLLABORATOR"}) +# Marker phrase Agent Shin always includes in its auto-close comments +# (see `format_pr_close_comment` / `format_issue_close_comment`). The +# provenance check for reconsider uses this string + a bot-author filter +# to confirm a PR/issue was actually auto-closed by Agent Shin before +# letting a reconsider trigger reopen it. Keep the marker in sync with +# the literal text in those formatter functions. +AGENT_SHIN_AUTO_CLOSE_MARKER = "I'm **Agent Shin**" + # Model families that require `reasoning_effort` to be set, and that reject # `temperature != 1` unless `reasoning_effort` is "none". For these models we # pass `reasoning_effort="none"` so a `temperature=0` deterministic judgment @@ -160,6 +168,64 @@ def reopen_issue(repo: str, number: int) -> None: ) +def fetch_issue_comments(repo: str, number: int) -> list[dict]: + """Fetch all issue-style comments on a PR/issue (paginated). + + `gh api --paginate` returns one JSON array per page; iterate them and + flatten. Returns [] on error (the reconsider path treats "no comments + found" as "no proof Agent Shin closed this", which fails-safe). + """ + try: + raw = gh( + "api", + "--paginate", + f"repos/{repo}/issues/{number}/comments?per_page=100", + ) + except subprocess.CalledProcessError: + return [] + comments: list[dict] = [] + for line in raw.strip().splitlines(): + line = line.strip() + if not line: + continue + try: + parsed = json.loads(line) + except json.JSONDecodeError: + continue + if isinstance(parsed, list): + comments.extend(parsed) + else: + comments.append(parsed) + return comments + + +def was_auto_closed_by_agent_shin(repo: str, number: int) -> bool: + """Return True iff this PR/issue carries an Agent Shin auto-close comment. + + Provenance check for the `@agent-shin reconsider` flow. We require: + + 1. A comment authored by a bot account (login ends with "[bot]") — + the auto-close workflow uses GH_TOKEN which posts as + `github-actions[bot]`. Filtering by bot author makes it impossible + for a contributor to spoof an Agent Shin close by pasting the + marker text into a manual comment. + 2. The comment body contains the Agent Shin auto-close marker + (`AGENT_SHIN_AUTO_CLOSE_MARKER`). + + Without this check, a maintainer who closes a PR as a duplicate or + out-of-scope could be silently overridden by the original author + commenting `@agent-shin reconsider` and polishing the description. + """ + for comment in fetch_issue_comments(repo, number): + login = ((comment.get("user") or {}).get("login") or "").lower() + if not login.endswith("[bot]"): + continue + body = comment.get("body") or "" + if AGENT_SHIN_AUTO_CLOSE_MARKER in body: + return True + return False + + # --------------------------------------------------------------------------- # Author classification @@ -514,10 +580,20 @@ def triage( fail-but-no-comment is replaced with a "still failing" comment + leave closed; a pass triggers `reopen_pr`/`reopen_issue` plus a reopen comment. Reconsider mode is intended for the `@agent-shin reconsider` comment - trigger. `close` is forced True implicitly when `reconsider` is set - because the bot has already decided this is a real (non-dry-run) - invocation; it's the caller's responsibility to gate on - AGENT_SHIN_ENABLED before calling reconsider mode. + trigger. + + `close` still controls whether destructive side effects fire. When + `close=False` and `reconsider=True`, the function previews the + decision (`would-reopen`, `would-leave-closed-still-failing`, or + `skip-not-bot-closed`) without posting comments or reopening — this + mirrors the regular `would-close` dry-run behavior so the reconsider + workflow can be exercised safely with `AGENT_SHIN_ENABLED != "true"`. + + Provenance: when `reconsider=True`, we additionally check that the + PR/issue was actually auto-closed by Agent Shin (via the bot-authored + auto-close marker comment). If not, we refuse to reopen so a + maintainer-closed PR cannot be silently overridden by the author + polishing the description and commenting `@agent-shin reconsider`. """ fetcher = {"pr": fetch_pr, "issue": fetch_issue}[kind] item = fetcher(repo, number) @@ -551,6 +627,15 @@ def triage( if is_internal_contributor(item): return {**base_result, "action": "skip-internal-author"} + # Provenance gate for reconsider: only reopen items Agent Shin auto-closed. + # Done up-front so a maintainer-closed PR short-circuits before we burn + # LLM tokens or touch comments. The check requires a bot-authored + # comment containing the auto-close marker (see + # `was_auto_closed_by_agent_shin`), so a contributor cannot spoof it by + # quoting the close template themselves. + if reconsider and not was_auto_closed_by_agent_shin(repo, number): + return {**base_result, "action": "skip-not-bot-closed"} + if kind == "pr": prompt = build_pr_prompt(title=title, body=body) # Short-circuit: if body very clearly links a related issue, just pass. @@ -565,8 +650,14 @@ def triage( }, } if reconsider: - # Pass-on-reconsider -> reopen the PR with a friendly comment. reopen_body = format_reopen_comment(kind) + if not close: + return { + **base, + "action": "would-reopen", + "comment": reopen_body, + } + # Pass-on-reconsider -> reopen the PR with a friendly comment. post_comment(repo, number, reopen_body) reopen_pr(repo, number) return { @@ -606,9 +697,17 @@ def triage( if reconsider: # Reconsider: pass -> reopen + post reopen comment; # fail -> leave closed + post a "still failing" comment so the - # contributor can iterate again. + # contributor can iterate again. When `close=False` we preview + # the action instead of actually posting/reopening. if decision != "fail": reopen_body = format_reopen_comment(kind) + if not close: + return { + **base_result, + "action": "would-reopen", + "verdict": verdict, + "comment": reopen_body, + } post_comment(repo, number, reopen_body) if kind == "pr": reopen_pr(repo, number) @@ -621,6 +720,13 @@ def triage( "comment": reopen_body, } still_failing = format_reconsider_still_failing_comment(kind, verdict) + if not close: + return { + **base_result, + "action": "would-leave-closed-still-failing", + "verdict": verdict, + "comment": still_failing, + } post_comment(repo, number, still_failing) return { **base_result, diff --git a/.github/workflows/triage_reconsider.yml b/.github/workflows/triage_reconsider.yml index f048ac5d139..69ba73d224d 100644 --- a/.github/workflows/triage_reconsider.yml +++ b/.github/workflows/triage_reconsider.yml @@ -107,20 +107,22 @@ jobs: else ARGS=(--repo "${{ github.repository }}" --issue "${NUMBER}" --reconsider) fi - # Reconsider IS the destructive path here (it can post comments - # and reopen) — there's no separate `--close` flag because the - # script's reconsider mode handles both pass (reopen) and fail - # (still-failing comment) outcomes itself. + # Reconsider is the destructive path here (it can post comments + # and reopen). The triage script honors `--close` even in + # reconsider mode — without it the script previews actions + # (`would-reopen` / `would-leave-closed-still-failing`) without + # writing to GitHub. Append `--close` only when AGENT_SHIN_ENABLED + # is the literal string "true"; everything else (unset, "false", + # "True", "yes", "1", typos) stays in dry-run. # # Use the positive `= "true"` gate (instead of `!= "true" -> exit`) # so the workflow guardrails in # tests/test_litellm/test_github_triage_workflows.py see the - # canonical fail-safe enable pattern. Unknown values like "True", - # "yes", "1", or typos will fall through to the dry-run else - # branch, which is the safe default. + # canonical fail-safe enable pattern. if [ "${AGENT_SHIN_ENABLED:-false}" = "true" ]; then + ARGS+=(--close) echo "::notice::Agent Shin reconsider ENABLED — running real triage." - python3 .github/scripts/triage_with_llm.py "${ARGS[@]}" else echo "::notice::AGENT_SHIN_ENABLED is not 'true' -> reconsider stays in dry-run (no comment, no reopen)." fi + python3 .github/scripts/triage_with_llm.py "${ARGS[@]}" diff --git a/tests/test_litellm/test_github_triage_with_llm.py b/tests/test_litellm/test_github_triage_with_llm.py index bd9682159b5..078612e89f9 100644 --- a/tests/test_litellm/test_github_triage_with_llm.py +++ b/tests/test_litellm/test_github_triage_with_llm.py @@ -298,6 +298,87 @@ class TestMainModelDefault: assert captured["model"] == "gpt-4o-mini" +class TestWasAutoClosedByAgentShin: + """Provenance check that gates reconsider's reopen path.""" + + def test_should_return_true_when_bot_comment_has_marker( + self, triage_module, monkeypatch + ): + comments = [ + { + "user": {"login": "github-actions[bot]"}, + "body": ( + "👋 Hi, thanks for the PR! I'm **Agent Shin**, the automated " + "triage bot for this repository.\n\nThis PR is being **auto-closed**..." + ), + } + ] + monkeypatch.setattr( + triage_module, "fetch_issue_comments", lambda repo, n: comments + ) + assert triage_module.was_auto_closed_by_agent_shin("o/r", 1) is True + + def test_should_return_false_when_no_comments(self, triage_module, monkeypatch): + monkeypatch.setattr(triage_module, "fetch_issue_comments", lambda repo, n: []) + assert triage_module.was_auto_closed_by_agent_shin("o/r", 1) is False + + def test_should_ignore_non_bot_author_with_marker(self, triage_module, monkeypatch): + # A contributor pasting the marker into a manual comment must NOT + # be treated as proof Agent Shin closed the PR. Only bot accounts + # (login ends with "[bot]") count — they can't be spoofed. + comments = [ + { + "user": {"login": "outside-dev"}, + "body": "I'm **Agent Shin**, just kidding — please reconsider this.", + } + ] + monkeypatch.setattr( + triage_module, "fetch_issue_comments", lambda repo, n: comments + ) + assert triage_module.was_auto_closed_by_agent_shin("o/r", 1) is False + + def test_should_ignore_bot_comment_without_marker(self, triage_module, monkeypatch): + # Other bots (codecov, cla-assistant, etc.) post on every PR; their + # presence must not satisfy the provenance check. + comments = [ + { + "user": {"login": "codecov[bot]"}, + "body": "## Codecov Report ...", + }, + { + "user": {"login": "greptile-apps[bot]"}, + "body": "Confidence Score: 2/5", + }, + ] + monkeypatch.setattr( + triage_module, "fetch_issue_comments", lambda repo, n: comments + ) + assert triage_module.was_auto_closed_by_agent_shin("o/r", 1) is False + + def test_should_find_marker_in_any_bot_comment(self, triage_module, monkeypatch): + # The auto-close comment may not be the most recent (e.g. the + # contributor commented after Agent Shin closed it). Any matching + # bot comment counts. + comments = [ + { + "user": {"login": "codecov[bot]"}, + "body": "## Codecov Report", + }, + { + "user": {"login": "github-actions[bot]"}, + "body": "I'm **Agent Shin**, the automated triage bot ...", + }, + { + "user": {"login": "outside-dev"}, + "body": "Replying after auto-close ...", + }, + ] + monkeypatch.setattr( + triage_module, "fetch_issue_comments", lambda repo, n: comments + ) + assert triage_module.was_auto_closed_by_agent_shin("o/r", 1) is True + + class TestCallLlmJudge: """call_llm_judge sets gpt-5 specific kwargs correctly.""" @@ -604,6 +685,11 @@ class TestTriageOrchestration: state="closed", body="Updated body with QA proof + screenshots." ) monkeypatch.setattr(triage_module, "fetch_pr", lambda repo, n: pr) + # Provenance: the PR was auto-closed by Agent Shin (a bot-authored + # auto-close comment exists), so reconsider is allowed to reopen. + monkeypatch.setattr( + triage_module, "was_auto_closed_by_agent_shin", lambda repo, n: True + ) posted = {} reopened = {} monkeypatch.setattr( @@ -627,7 +713,7 @@ class TestTriageOrchestration: repo="o/r", kind="pr", number=42, - close=False, + close=True, model="m", judge=lambda p: json.dumps( {"verdict": "pass", "missing": [], "explanation": "ok now"} @@ -644,6 +730,9 @@ class TestTriageOrchestration: ): pr = self._make_pr(state="closed", body="still empty") monkeypatch.setattr(triage_module, "fetch_pr", lambda repo, n: pr) + monkeypatch.setattr( + triage_module, "was_auto_closed_by_agent_shin", lambda repo, n: True + ) posted = {} monkeypatch.setattr( triage_module, @@ -671,7 +760,7 @@ class TestTriageOrchestration: repo="o/r", kind="pr", number=42, - close=False, + close=True, model="m", judge=lambda p: json.dumps(verdict), reconsider=True, @@ -688,6 +777,9 @@ class TestTriageOrchestration: # path should reopen the PR without calling the LLM. pr = self._make_pr(state="closed", body="Fixes #1234\n\nAddresses the bug.") monkeypatch.setattr(triage_module, "fetch_pr", lambda repo, n: pr) + monkeypatch.setattr( + triage_module, "was_auto_closed_by_agent_shin", lambda repo, n: True + ) posted = {} reopened = {} monkeypatch.setattr( @@ -705,7 +797,7 @@ class TestTriageOrchestration: repo="o/r", kind="pr", number=55, - close=False, + close=True, model="m", judge=lambda p: pytest.fail("LLM must not run when linked-issue matches"), reconsider=True, @@ -729,17 +821,194 @@ class TestTriageOrchestration: "reopen_pr", lambda *a, **kw: pytest.fail("must not reopen for internal author"), ) + # Internal check must fire *before* provenance, so the provenance + # helper should never be invoked for an internal author. + monkeypatch.setattr( + triage_module, + "was_auto_closed_by_agent_shin", + lambda *a, **kw: pytest.fail("must not check provenance for internal"), + ) result = triage_module.triage( repo="o/r", kind="pr", number=1, - close=False, + close=True, model="m", judge=lambda p: pytest.fail("LLM must not run for internal author"), reconsider=True, ) assert result["action"] == "skip-internal-author" + def test_should_skip_reconsider_when_not_bot_closed( + self, triage_module, monkeypatch + ): + # A maintainer-closed PR (no Agent Shin auto-close comment) must + # never be reopened by `@agent-shin reconsider`, regardless of how + # good the LLM verdict would be. Otherwise the original author + # could polish the description and silently override a maintainer's + # "closed as duplicate / out of scope" decision. + pr = self._make_pr(state="closed", body="Fixes #1234") + monkeypatch.setattr(triage_module, "fetch_pr", lambda repo, n: pr) + monkeypatch.setattr( + triage_module, "was_auto_closed_by_agent_shin", lambda repo, n: False + ) + monkeypatch.setattr( + triage_module, + "post_comment", + lambda *a, **kw: pytest.fail("must not comment when not bot-closed"), + ) + monkeypatch.setattr( + triage_module, + "reopen_pr", + lambda *a, **kw: pytest.fail("must not reopen when not bot-closed"), + ) + result = triage_module.triage( + repo="o/r", + kind="pr", + number=1, + close=True, + model="m", + judge=lambda p: pytest.fail("LLM must not run when not bot-closed"), + reconsider=True, + ) + assert result["action"] == "skip-not-bot-closed" + + def test_should_skip_reconsider_issue_when_not_bot_closed( + self, triage_module, monkeypatch + ): + # Same provenance gate for issues: only Agent Shin auto-closed + # issues are eligible for reopen-on-reconsider. + issue = { + "number": 7, + "title": "Bug", + "body": "Repro: curl ...", + "state": "closed", + "author_association": "NONE", + "user": {"login": "outside"}, + } + monkeypatch.setattr(triage_module, "fetch_issue", lambda repo, n: issue) + monkeypatch.setattr( + triage_module, "was_auto_closed_by_agent_shin", lambda repo, n: False + ) + monkeypatch.setattr( + triage_module, + "reopen_issue", + lambda *a, **kw: pytest.fail("must not reopen maintainer-closed issue"), + ) + result = triage_module.triage( + repo="o/r", + kind="issue", + number=7, + close=True, + model="m", + judge=lambda p: pytest.fail("LLM must not run for non-bot-closed issue"), + reconsider=True, + ) + assert result["action"] == "skip-not-bot-closed" + + def test_should_preview_reopen_in_reconsider_dry_run( + self, triage_module, monkeypatch + ): + # When `close=False` and `reconsider=True`, a passing verdict must + # produce a `would-reopen` preview WITHOUT posting a comment or + # reopening — same dry-run pattern as `would-close` in regular mode. + pr = self._make_pr(state="closed", body="Now with screenshots + repro.") + monkeypatch.setattr(triage_module, "fetch_pr", lambda repo, n: pr) + monkeypatch.setattr( + triage_module, "was_auto_closed_by_agent_shin", lambda repo, n: True + ) + monkeypatch.setattr( + triage_module, + "post_comment", + lambda *a, **kw: pytest.fail("dry-run must not post"), + ) + monkeypatch.setattr( + triage_module, + "reopen_pr", + lambda *a, **kw: pytest.fail("dry-run must not reopen"), + ) + result = triage_module.triage( + repo="o/r", + kind="pr", + number=42, + close=False, + model="m", + judge=lambda p: json.dumps( + {"verdict": "pass", "missing": [], "explanation": "ok now"} + ), + reconsider=True, + ) + assert result["action"] == "would-reopen" + # The preview should include the comment body the bot WOULD post + # (useful for $GITHUB_STEP_SUMMARY). + assert "reopened" in result["comment"].lower() + + def test_should_preview_reopen_in_reconsider_dry_run_linked_issue( + self, triage_module, monkeypatch + ): + # The linked-issue short-circuit also has to honor dry-run in + # reconsider mode — no LLM call AND no destructive side effects. + pr = self._make_pr(state="closed", body="Fixes #1234\n\nDetails.") + monkeypatch.setattr(triage_module, "fetch_pr", lambda repo, n: pr) + monkeypatch.setattr( + triage_module, "was_auto_closed_by_agent_shin", lambda repo, n: True + ) + monkeypatch.setattr( + triage_module, + "post_comment", + lambda *a, **kw: pytest.fail("dry-run must not post"), + ) + monkeypatch.setattr( + triage_module, + "reopen_pr", + lambda *a, **kw: pytest.fail("dry-run must not reopen"), + ) + result = triage_module.triage( + repo="o/r", + kind="pr", + number=55, + close=False, + model="m", + judge=lambda p: pytest.fail("LLM must not run when linked-issue matches"), + reconsider=True, + ) + assert result["action"] == "would-reopen" + assert "reopened" in result["comment"].lower() + + def test_should_preview_still_failing_in_reconsider_dry_run( + self, triage_module, monkeypatch + ): + # When `close=False` and the verdict is fail, the dry-run preview + # must say `would-leave-closed-still-failing` and not post the + # "still failing" comment. + pr = self._make_pr(state="closed", body="still thin") + monkeypatch.setattr(triage_module, "fetch_pr", lambda repo, n: pr) + monkeypatch.setattr( + triage_module, "was_auto_closed_by_agent_shin", lambda repo, n: True + ) + monkeypatch.setattr( + triage_module, + "post_comment", + lambda *a, **kw: pytest.fail("dry-run must not post"), + ) + + verdict = { + "verdict": "fail", + "missing": ["QA proof"], + "explanation": "Still no QA proof.", + } + result = triage_module.triage( + repo="o/r", + kind="pr", + number=42, + close=False, + model="m", + judge=lambda p: json.dumps(verdict), + reconsider=True, + ) + assert result["action"] == "would-leave-closed-still-failing" + assert "QA proof" in result["comment"] + def test_should_reopen_issue_on_reconsider_pass(self, triage_module, monkeypatch): issue = { "number": 7, @@ -750,6 +1019,9 @@ class TestTriageOrchestration: "user": {"login": "outside"}, } monkeypatch.setattr(triage_module, "fetch_issue", lambda repo, n: issue) + monkeypatch.setattr( + triage_module, "was_auto_closed_by_agent_shin", lambda repo, n: True + ) posted = {} reopened = {} monkeypatch.setattr( @@ -767,7 +1039,7 @@ class TestTriageOrchestration: repo="o/r", kind="issue", number=7, - close=False, + close=True, model="m", judge=lambda p: json.dumps( {"verdict": "pass", "missing": [], "explanation": "now reproducible"}