Skip to content
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
83 changes: 80 additions & 3 deletions tools/test_triage_daemon.py
Original file line number Diff line number Diff line change
Expand Up @@ -352,7 +352,7 @@ def test_mark_pr_opened_flags_the_pr_for_review(self, mock_run):
def test_mark_pr_opened_with_cleanup_flags_review_and_cleanup_together(self, mock_run):
"""A PR this run just opened gets BOT_REVIEW and BOT_CLEANUP in one edit, so the
review's findings are acted on without anyone relabelling it by hand."""
mock_run.return_value = MagicMock(stdout=json.dumps([{"number": 4742}]))
mock_run.side_effect = self._bot_pr_gh()
triage_daemon.mark_pr_opened(4720, cleanup=True)
calls = [call.args[0] for call in mock_run.call_args_list]
self.assertIn(["gh", "pr", "edit", "4742", "--repo", "springfall2008/batpred", "--add-label", "BOT_REVIEW", "--add-label", "BOT_CLEANUP"], calls)
Expand All @@ -376,6 +376,82 @@ def test_mark_pr_opened_skips_flagging_when_no_pr_found(self, mock_run):
calls = [call.args[0] for call in mock_run.call_args_list]
self.assertFalse(any(call[:3] == ["gh", "pr", "edit"] for call in calls))

@staticmethod
def _bot_pr_gh(requester_stdout="latest-requester\n", requester_rc=0):
"""A subprocess.run stand-in: the PR search finds #4742, the events lookup prints
requester_stdout, and every other gh call succeeds."""

def fake_run(cmd, *args, **kwargs):
"""Answer one gh call by its leading words."""
if cmd[:3] == ["gh", "pr", "list"]:
return MagicMock(stdout=json.dumps([{"number": 4742}]), returncode=0)
if cmd[:2] == ["gh", "api"]:
return MagicMock(stdout=requester_stdout, returncode=requester_rc, stderr="")
return MagicMock(stdout="", returncode=0, stderr="")

return fake_run

@patch("triage_daemon.subprocess.run")
def test_mark_pr_opened_by_bot_labels_and_assigns_the_requester(self, mock_run):
"""A PR the bot just opened gets BOT_PR_OPENED in the same edit as its review labels, and
is assigned to whoever added BOT_PR, since its author is always the bot's credential."""
mock_run.side_effect = self._bot_pr_gh()
triage_daemon.mark_pr_opened(4720, cleanup=True, opened_by_bot=True)
calls = [call.args[0] for call in mock_run.call_args_list]
self.assertIn(["gh", "pr", "edit", "4742", "--repo", "springfall2008/batpred", "--add-label", "BOT_REVIEW", "--add-label", "BOT_CLEANUP", "--add-label", "BOT_PR_OPENED"], calls)
self.assertIn(["gh", "pr", "edit", "4742", "--repo", "springfall2008/batpred", "--add-assignee", "latest-requester"], calls)

@patch("triage_daemon.subprocess.run")
def test_mark_pr_opened_not_by_bot_does_not_claim_the_pr(self, mock_run):
"""A PR found already open may be a person's own - it is neither labelled as the bot's
nor reassigned."""
mock_run.side_effect = self._bot_pr_gh()
triage_daemon.mark_pr_opened(4720)
calls = [call.args[0] for call in mock_run.call_args_list]
self.assertFalse(any(call[:3] == ["gh", "pr", "edit"] and ("BOT_PR_OPENED" in call or "--add-assignee" in call) for call in calls))
self.assertFalse(any(call[:2] == ["gh", "api"] for call in calls))

@patch("triage_daemon.subprocess.run")
def test_assign_bot_pr_requester_skips_when_lookup_fails(self, mock_run):
"""A failed events lookup skips the assignee and does not raise."""
mock_run.side_effect = self._bot_pr_gh(requester_stdout="", requester_rc=1)
triage_daemon.assign_bot_pr_requester(4742, 4720)
calls = [call.args[0] for call in mock_run.call_args_list]
self.assertFalse(any("--add-assignee" in call for call in calls))

@patch("triage_daemon.subprocess.run")
def test_assign_bot_pr_requester_swallows_a_hung_gh(self, mock_run):
"""The PR is already open and labelled, so a gh call that times out is logged, not raised."""
mock_run.side_effect = subprocess.TimeoutExpired(cmd="gh", timeout=120)
triage_daemon.assign_bot_pr_requester(4742, 4720) # must not raise

@patch("triage_daemon.subprocess.run")
def test_assign_bot_pr_requester_failed_edit_does_not_raise(self, mock_run):
"""An unassignable login makes the edit fail; that is logged and the flow carries on."""
fake = self._bot_pr_gh()

def failing_edit(cmd, *args, **kwargs):
"""Fail the assignee edit, answer everything else as normal."""
if "--add-assignee" in cmd:
return MagicMock(stdout="", returncode=1, stderr="not a collaborator")
return fake(cmd, *args, **kwargs)

mock_run.side_effect = failing_edit
triage_daemon.assign_bot_pr_requester(4742, 4720) # must not raise
calls = [call.args[0] for call in mock_run.call_args_list]
self.assertTrue(any("--add-assignee" in call for call in calls))

@patch("triage_daemon.subprocess.run")
def test_find_bot_pr_requester_takes_the_latest_label_event(self, mock_run):
"""BOT_PR re-added after a failure has several labeled events, one line each across
pages; the most recent one is the person who asked this time. Null actors are
filtered in the query, so 'null' is never taken for a login."""
mock_run.side_effect = self._bot_pr_gh(requester_stdout="earlier-requester\n\nlatest-requester\n")
self.assertEqual(triage_daemon.find_bot_pr_requester(4720), "latest-requester")
cmd = mock_run.call_args_list[0].args[0]
self.assertEqual(cmd[:4], ["gh", "api", "--paginate", "repos/springfall2008/batpred/issues/4720/events"])
self.assertIn(".actor != null", cmd[-1])

@patch("triage_daemon.subprocess.run")
def test_mark_pr_failed_swaps_labels(self, mock_run):
"""Removes BOT_PR and adds BOT_PR_FAILED, scoped to the configured repo."""
Expand Down Expand Up @@ -1847,11 +1923,12 @@ def test_skips_triage_when_already_triaged(self):

def test_marks_opened_when_a_pr_exists_afterwards(self):
"""If a PR references the issue after create_pr() runs, swap to BOT_PR_OPENED - and
since this run opened it, queue the cleanup that acts on its review as well."""
since this run opened it, queue the cleanup that acts on its review as well, and claim
the PR as the bot's for its requester."""
self.patches["has_existing_pr"].side_effect = [False, True]
self.patches["ensure_triaged"].return_value = True
triage_daemon.process_bot_pr_issue({"number": 4720, "labels": [], "title": "Solis TOU bit refused"})
self.patches["mark_pr_opened"].assert_called_once_with(4720, cleanup=True)
self.patches["mark_pr_opened"].assert_called_once_with(4720, cleanup=True, opened_by_bot=True)
self.patches["mark_pr_failed"].assert_not_called()

def test_marks_failed_when_no_pr_exists_afterwards(self):
Expand Down
62 changes: 57 additions & 5 deletions tools/triage_daemon.py
Original file line number Diff line number Diff line change
Expand Up @@ -126,6 +126,9 @@
from pathlib import Path

REPO = "springfall2008/batpred"
# Bounds the best-effort gh calls that link a bot PR to its requester, so a stalled call
# can't hang the single-threaded poll loop.
GH_LINK_TIMEOUT_SECONDS = 120
BASE_DIR = Path.home() / "predbat-triage-bot"
CLONE_DIR = BASE_DIR / "batpred"
SCRATCH_DIR = BASE_DIR / "scratch"
Expand Down Expand Up @@ -695,17 +698,20 @@ def is_actionable(issue_number):
return bool(label_names & {"bug", "enhancement"})


def flag_pr_for_review(pr_number, cleanup=False):
def flag_pr_for_review(pr_number, cleanup=False, opened_by_bot=False):
"""Add BOT_REVIEW to a PR, so the next poll cycle runs /pr-review against it - and
with cleanup, BOT_CLEANUP too, so /pr-cleanup then acts on what the review found.
Both go in one edit: a cycle that saw BOT_CLEANUP alone would run the cleanup before
the review existed. Idempotent - adding a label the PR already carries is a no-op.

opened_by_bot adds BOT_PR_OPENED to the PR as well (not just the issue), so PRs the
BOT_PR flow opened can be listed by label - their author is always the bot's credential.
"""
labels = ["--add-label", "BOT_REVIEW"] + (["--add-label", "BOT_CLEANUP"] if cleanup else [])
labels = ["--add-label", "BOT_REVIEW"] + (["--add-label", "BOT_CLEANUP"] if cleanup else []) + (["--add-label", "BOT_PR_OPENED"] if opened_by_bot else [])
subprocess.run(["gh", "pr", "edit", str(pr_number), "--repo", REPO] + labels, check=True)


def mark_pr_opened(issue_number, cleanup=False):
def mark_pr_opened(issue_number, cleanup=False, opened_by_bot=False):
"""Swap BOT_PR for BOT_PR_OPENED once the draft PR has been confirmed open, and
flag the PR itself with BOT_REVIEW so a code review runs against it automatically -
/issue-pr's own quality gate (step 4 of its SKILL.md) is pre-commit and a targeted
Expand All @@ -714,14 +720,60 @@ def mark_pr_opened(issue_number, cleanup=False):
cleanup also queues BOT_CLEANUP. Only process_bot_pr_issue() passes it, and only for a
PR its own create_pr() just opened: a PR found already open on entry may be someone's
own branch, and BOT_CLEANUP is the label that commits and pushes to it.

opened_by_bot, passed under the same condition, labels the PR as the bot's and assigns
it to whoever added BOT_PR (assign_bot_pr_requester()).
"""
subprocess.run(
["gh", "issue", "edit", str(issue_number), "--repo", REPO, "--remove-label", "BOT_PR", "--add-label", "BOT_PR_OPENED"],
check=True,
)
pr_number = find_pr_number_for_issue(issue_number)
if pr_number is not None:
flag_pr_for_review(pr_number, cleanup=cleanup)
flag_pr_for_review(pr_number, cleanup=cleanup, opened_by_bot=opened_by_bot)
if opened_by_bot:
assign_bot_pr_requester(pr_number, issue_number)


def find_bot_pr_requester(issue_number):
"""Return the login of whoever last added BOT_PR to the issue, or None if it can't be found.

The PR is always authored by the bot's credential, so the issue's label event is the only
record of who asked for it. Read after mark_pr_opened() has swapped BOT_PR off: the
labeled event stays in the issue's history either way. Events come oldest first and
--paginate walks the pages in order, so the last line is the latest request.
"""
result = subprocess.run(
["gh", "api", "--paginate", f"repos/{REPO}/issues/{issue_number}/events", "--jq", '.[] | select(.event == "labeled" and .label.name == "BOT_PR" and .actor != null) | .actor.login'],
capture_output=True,
text=True,
check=False,
timeout=GH_LINK_TIMEOUT_SECONDS,
)
if result.returncode != 0:
return None
Comment thread
chalfontchubby marked this conversation as resolved.
logins = [line.strip() for line in (result.stdout or "").splitlines() if line.strip()]
return logins[-1] if logins else None


def assign_bot_pr_requester(pr_number, issue_number):
"""Assign a PR the BOT_PR flow just opened to whoever added BOT_PR to its issue.

The PR's author is always the bot's credential, so this is what makes it show up under
the requester's assigned PRs. Best effort: the PR is already open and labelled, so any
failure - no requester found, an unassignable login, gh erroring or hanging - is logged
and must not fail the flow.
"""
try:
requester = find_bot_pr_requester(issue_number)
if requester is None:
print(f"[pr] PR #{pr_number}: no BOT_PR requester found on issue #{issue_number}, not assigning", flush=True)
return
assign = subprocess.run(["gh", "pr", "edit", str(pr_number), "--repo", REPO, "--add-assignee", requester], capture_output=True, text=True, check=False, timeout=GH_LINK_TIMEOUT_SECONDS)
if assign.returncode != 0:
print(f"[pr] PR #{pr_number}: could not assign requester {requester}: {(assign.stderr or '').strip()}", flush=True)
except (OSError, subprocess.SubprocessError) as exc:
print(f"[pr] PR #{pr_number}: assigning the BOT_PR requester failed: {exc}", flush=True)


class FlowFailed(subprocess.CalledProcessError):
Expand Down Expand Up @@ -1503,7 +1555,7 @@ def process_bot_pr_issue(issue):
return
create_pr(issue_number)
if has_existing_pr(issue_number):
mark_pr_opened(issue_number, cleanup=True)
mark_pr_opened(issue_number, cleanup=True, opened_by_bot=True)
else:
mark_pr_failed(issue_number)

Expand Down
Loading