Skip to content

Commit 1f36c04

Browse files
CIclaude
andcommitted
Triage bot: queue cleanup after review on PRs it opens
When the BOT_PR flow opens a PR it now labels it BOT_REVIEW and BOT_CLEANUP in a single edit, so /pr-cleanup acts on the review's findings without anyone relabelling the PR by hand. The cleanup flow now holds any PR that still carries BOT_REVIEW. It leaves BOT_CLEANUP in place so a later poll picks it up once the review has cleared its label. The main loop already runs the review queue before the cleanup queue, so both usually finish in the same cycle; the hold makes the order a guarantee rather than a matter of loop position. BOT_CLEANUP is only added to a PR the flow's own create_pr() just opened. A PR found already open on entry may be someone's own branch, and BOT_CLEANUP commits and pushes to it, so that path keeps adding BOT_REVIEW alone. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
1 parent 130596b commit 1f36c04

2 files changed

Lines changed: 78 additions & 11 deletions

File tree

‎tools/test_triage_daemon.py‎

Lines changed: 54 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -345,6 +345,24 @@ def test_mark_pr_opened_flags_the_pr_for_review(self, mock_run):
345345
calls = [call.args[0] for call in mock_run.call_args_list]
346346
self.assertIn(["gh", "pr", "edit", "4742", "--repo", "springfall2008/batpred", "--add-label", "BOT_REVIEW"], calls)
347347

348+
@patch("triage_daemon.subprocess.run")
349+
def test_mark_pr_opened_with_cleanup_flags_review_and_cleanup_together(self, mock_run):
350+
"""A PR this run just opened gets BOT_REVIEW and BOT_CLEANUP in one edit, so the
351+
review's findings are acted on without anyone relabelling it by hand."""
352+
mock_run.return_value = MagicMock(stdout=json.dumps([{"number": 4742}]))
353+
triage_daemon.mark_pr_opened(4720, cleanup=True)
354+
calls = [call.args[0] for call in mock_run.call_args_list]
355+
self.assertIn(["gh", "pr", "edit", "4742", "--repo", "springfall2008/batpred", "--add-label", "BOT_REVIEW", "--add-label", "BOT_CLEANUP"], calls)
356+
357+
@patch("triage_daemon.subprocess.run")
358+
def test_mark_pr_opened_without_cleanup_never_adds_bot_cleanup(self, mock_run):
359+
"""The default path is also taken for a PR found already open on entry, which may be
360+
a person's own branch - that must never gain a label that pushes commits to it."""
361+
mock_run.return_value = MagicMock(stdout=json.dumps([{"number": 4742}]))
362+
triage_daemon.mark_pr_opened(4720)
363+
calls = [call.args[0] for call in mock_run.call_args_list]
364+
self.assertFalse(any("BOT_CLEANUP" in call for call in calls))
365+
348366
@patch("triage_daemon.subprocess.run")
349367
def test_mark_pr_opened_skips_flagging_when_no_pr_found(self, mock_run):
350368
"""Defensive path: an empty PR search (e.g. a race with the PR being closed
@@ -387,6 +405,15 @@ def test_adds_bot_review_to_the_pr_scoped_to_repo(self, mock_run):
387405
args = mock_run.call_args[0][0]
388406
self.assertEqual(args, ["gh", "pr", "edit", "4742", "--repo", "springfall2008/batpred", "--add-label", "BOT_REVIEW"])
389407

408+
@patch("triage_daemon.subprocess.run")
409+
def test_cleanup_adds_bot_cleanup_in_the_same_edit(self, mock_run):
410+
"""Both labels land in one gh call, so no poll cycle can see BOT_CLEANUP without
411+
BOT_REVIEW and run the cleanup before the review it is meant to act on."""
412+
triage_daemon.flag_pr_for_review(4742, cleanup=True)
413+
mock_run.assert_called_once()
414+
args = mock_run.call_args[0][0]
415+
self.assertEqual(args, ["gh", "pr", "edit", "4742", "--repo", "springfall2008/batpred", "--add-label", "BOT_REVIEW", "--add-label", "BOT_CLEANUP"])
416+
390417

391418
class PermissionModelTests(unittest.TestCase):
392419
"""Regression tests for the ALLOWED_TOOLS/DISALLOWED_TOOLS delta between the
@@ -1663,11 +1690,12 @@ def test_skips_triage_when_already_triaged(self):
16631690
self.patches["create_pr"].assert_called_once_with(4720)
16641691

16651692
def test_marks_opened_when_a_pr_exists_afterwards(self):
1666-
"""If a PR references the issue after create_pr() runs, swap to BOT_PR_OPENED."""
1693+
"""If a PR references the issue after create_pr() runs, swap to BOT_PR_OPENED - and
1694+
since this run opened it, queue the cleanup that acts on its review as well."""
16671695
self.patches["has_existing_pr"].side_effect = [False, True]
16681696
self.patches["ensure_triaged"].return_value = True
16691697
triage_daemon.process_bot_pr_issue({"number": 4720, "labels": [], "title": "Solis TOU bit refused"})
1670-
self.patches["mark_pr_opened"].assert_called_once_with(4720)
1698+
self.patches["mark_pr_opened"].assert_called_once_with(4720, cleanup=True)
16711699
self.patches["mark_pr_failed"].assert_not_called()
16721700

16731701
def test_marks_failed_when_no_pr_exists_afterwards(self):
@@ -1937,6 +1965,15 @@ def test_queries_open_prs_labelled_bot_cleanup(self, mock_run):
19371965
self.assertEqual(args[args.index("--label") + 1], "BOT_CLEANUP")
19381966
self.assertIn("--limit", args)
19391967

1968+
@patch("triage_daemon.subprocess.run")
1969+
def test_the_query_asks_for_labels(self, mock_run):
1970+
"""process_bot_cleanup_pr() holds a PR that still carries BOT_REVIEW, which it can
1971+
only see if the labels are fetched."""
1972+
mock_run.return_value = MagicMock(stdout="[]")
1973+
triage_daemon.fetch_bot_cleanup_prs()
1974+
cmd = mock_run.call_args[0][0]
1975+
self.assertIn("labels", cmd[cmd.index("--json") + 1].split(","))
1976+
19401977

19411978
class RemovePrReviewLabelTests(unittest.TestCase):
19421979
"""Tests for remove_pr_review_label(), new - the BOT_REVIEW-on-PR flow."""
@@ -2372,6 +2409,21 @@ def test_failed_cleanup_marks_failed_instead_of_removing_the_label(self):
23722409
self.patches["mark_pr_cleanup_failed"].assert_called_once_with(4742)
23732410
self.patches["remove_pr_cleanup_label"].assert_not_called()
23742411

2412+
def test_waits_while_the_pr_still_carries_bot_review(self):
2413+
"""Cleanup acts on the review's findings, so with both labels set the review goes
2414+
first. Nothing runs and BOT_CLEANUP stays, so a later poll picks the PR up again
2415+
once the review has cleared BOT_REVIEW."""
2416+
triage_daemon.process_bot_cleanup_pr({"number": 4742, "title": "Add confirmed findings", "labels": [{"name": "BOT_CLEANUP"}, {"name": "BOT_REVIEW"}]})
2417+
self.patches["sync_repo"].assert_not_called()
2418+
self.patches["cleanup_pr"].assert_not_called()
2419+
self.patches["remove_pr_cleanup_label"].assert_not_called()
2420+
self.patches["mark_pr_cleanup_failed"].assert_not_called()
2421+
2422+
def test_runs_once_the_review_label_has_gone(self):
2423+
"""The hold is keyed on BOT_REVIEW alone - other labels do not block a cleanup."""
2424+
triage_daemon.process_bot_cleanup_pr({"number": 4742, "title": "Add confirmed findings", "labels": [{"name": "BOT_CLEANUP"}, {"name": "bug"}]})
2425+
self.patches["cleanup_pr"].assert_called_once_with(4742)
2426+
23752427
@patch("builtins.print")
23762428
def test_prints_the_title_and_link_before_doing_anything(self, mock_print):
23772429
"""Prints the PR's title and an openable GitHub link as soon as work starts."""

‎tools/triage_daemon.py‎

Lines changed: 24 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -655,26 +655,33 @@ def is_actionable(issue_number):
655655
return bool(label_names & {"bug", "enhancement"})
656656

657657

658-
def flag_pr_for_review(pr_number):
659-
"""Add BOT_REVIEW to a PR, so the next poll cycle runs /code-review against it.
660-
Idempotent - adding a label the PR already carries is a no-op, not an error.
658+
def flag_pr_for_review(pr_number, cleanup=False):
659+
"""Add BOT_REVIEW to a PR, so the next poll cycle runs /code-review against it - and
660+
with cleanup, BOT_CLEANUP too, so /pr-cleanup then acts on what the review found.
661+
Both go in one edit: a cycle that saw BOT_CLEANUP alone would run the cleanup before
662+
the review existed. Idempotent - adding a label the PR already carries is a no-op.
661663
"""
662-
subprocess.run(["gh", "pr", "edit", str(pr_number), "--repo", REPO, "--add-label", "BOT_REVIEW"], check=True)
664+
labels = ["--add-label", "BOT_REVIEW"] + (["--add-label", "BOT_CLEANUP"] if cleanup else [])
665+
subprocess.run(["gh", "pr", "edit", str(pr_number), "--repo", REPO] + labels, check=True)
663666

664667

665-
def mark_pr_opened(issue_number):
668+
def mark_pr_opened(issue_number, cleanup=False):
666669
"""Swap BOT_PR for BOT_PR_OPENED once the draft PR has been confirmed open, and
667670
flag the PR itself with BOT_REVIEW so a code review runs against it automatically -
668671
/issue-pr's own quality gate (step 4 of its SKILL.md) is pre-commit and a targeted
669672
test, not an LLM review of the diff.
673+
674+
cleanup also queues BOT_CLEANUP. Only process_bot_pr_issue() passes it, and only for a
675+
PR its own create_pr() just opened: a PR found already open on entry may be someone's
676+
own branch, and BOT_CLEANUP is the label that commits and pushes to it.
670677
"""
671678
subprocess.run(
672679
["gh", "issue", "edit", str(issue_number), "--repo", REPO, "--remove-label", "BOT_PR", "--add-label", "BOT_PR_OPENED"],
673680
check=True,
674681
)
675682
pr_number = find_pr_number_for_issue(issue_number)
676683
if pr_number is not None:
677-
flag_pr_for_review(pr_number)
684+
flag_pr_for_review(pr_number, cleanup=cleanup)
678685

679686

680687
def mark_triage_failed(issue_number, attempts):
@@ -1293,7 +1300,7 @@ def process_bot_pr_issue(issue):
12931300
return
12941301
create_pr(issue_number)
12951302
if has_existing_pr(issue_number):
1296-
mark_pr_opened(issue_number)
1303+
mark_pr_opened(issue_number, cleanup=True)
12971304
else:
12981305
mark_pr_failed(issue_number)
12991306

@@ -1333,9 +1340,9 @@ def pr_head_is_fork(pr):
13331340

13341341

13351342
def fetch_bot_cleanup_prs():
1336-
"""Return open PRs currently labelled BOT_CLEANUP, each with its title."""
1343+
"""Return open PRs currently labelled BOT_CLEANUP, each with its title and labels."""
13371344
result = subprocess.run(
1338-
["gh", "pr", "list", "--repo", REPO, "--state", "open", "--label", "BOT_CLEANUP", "--json", "number,title,headRepositoryOwner", "--limit", "100"],
1345+
["gh", "pr", "list", "--repo", REPO, "--state", "open", "--label", "BOT_CLEANUP", "--json", "number,title,headRepositoryOwner,labels", "--limit", "100"],
13391346
capture_output=True,
13401347
text=True,
13411348
check=True,
@@ -1686,9 +1693,15 @@ def process_bot_cleanup_pr(pr):
16861693
"""Run the BOT_CLEANUP flow for one PR: address review feedback and CI failures,
16871694
then remove the trigger label. A failed run swaps to BOT_FAILED instead, with an
16881695
explanatory comment.
1696+
1697+
A PR still carrying BOT_REVIEW is left alone, label and all: the cleanup acts on the
1698+
review's findings, so the review goes first and a later poll picks this PR up again.
16891699
"""
16901700
pr_number = pr["number"]
16911701
print(f'[cleanup-pr] PR #{pr_number}: "{pr["title"]}" - {pr_url(pr_number)}', flush=True)
1702+
if "BOT_REVIEW" in {label["name"] for label in pr.get("labels", [])}:
1703+
print(f"[cleanup-pr] PR #{pr_number}: BOT_REVIEW still pending - waiting for the review before cleaning up", flush=True)
1704+
return
16921705
if pr_head_is_fork(pr):
16931706
print(f"[cleanup-pr] PR #{pr_number}: head branch is in a fork - not writable with this credential, skipping", flush=True)
16941707
mark_pr_cleanup_unsupported(pr_number)
@@ -1754,6 +1767,8 @@ def main():
17541767
process_bot_pr_issue(issue)
17551768
for issue in fetch_bot_review_issues():
17561769
process_bot_review_issue(issue)
1770+
# Reviews before cleanups, so a PR carrying both is reviewed and cleaned up in
1771+
# the same cycle; process_bot_cleanup_pr() holds any PR whose review is pending.
17571772
for pr in fetch_bot_review_prs():
17581773
process_bot_review_pr(pr)
17591774
for pr in fetch_bot_cleanup_prs():

0 commit comments

Comments
 (0)