diff --git a/.cspell/custom-dictionary-workspace.txt b/.cspell/custom-dictionary-workspace.txt index 9dce572c6..e2c6ff0f0 100644 --- a/.cspell/custom-dictionary-workspace.txt +++ b/.cspell/custom-dictionary-workspace.txt @@ -170,6 +170,7 @@ dtype dwindow eauth echargingpile +ECONNRESET ecosmart Eddi eddis @@ -195,6 +196,7 @@ energythroughput enho Enlighten enlm +ENOTFOUND enph enphase enphaseenergy @@ -204,6 +206,7 @@ epod epvtoday errno ESSAPI +ETIMEDOUT etoday etok euids diff --git a/tools/test_triage_daemon.py b/tools/test_triage_daemon.py index 8c1726a49..619f4757f 100644 --- a/tools/test_triage_daemon.py +++ b/tools/test_triage_daemon.py @@ -18,6 +18,9 @@ import triage_daemon +# Captured before any test patches it, for the one test of the real function. +REAL_DAEMON_VERSION = triage_daemon.daemon_version + # What `gh pr list --json number` prints once the flush has opened its PR. The flush reads # this to decide whether the queue may be consumed, so the tests need both shapes. @@ -1060,7 +1063,7 @@ def test_the_last_attempt_marks_the_issue_and_moves_past_it(self): state = {"last_processed": 5003, "triage_attempts": {"5004": triage_daemon.TRIAGE_MAX_ATTEMPTS - 1}} self.assertTrue(triage_daemon.process_new_issue(self.issue, state)) self.assertEqual(state["last_processed"], 5004) - self.patches["mark_triage_failed"].assert_called_once_with(5004, triage_daemon.TRIAGE_MAX_ATTEMPTS) + self.patches["mark_triage_failed"].assert_called_once_with(5004, triage_daemon.TRIAGE_MAX_ATTEMPTS, self.patches["triage"].side_effect) self.assertNotIn("5004", state["triage_attempts"]) def test_a_failing_mark_does_not_abort_the_cycle(self): @@ -2296,7 +2299,7 @@ def test_failed_follow_up_marks_failed_instead_of_removing_the_label(self): self.patches["is_already_triaged"].return_value = True self.patches["triage_followup"].side_effect = subprocess.CalledProcessError(1, ["claude"]) triage_daemon.process_bot_review_issue({"number": 3100, "labels": [{"name": "BOT_TRIAGED"}], "title": "Old ticket needing review"}) - self.patches["mark_review_failed"].assert_called_once_with(3100) + self.patches["mark_review_failed"].assert_called_once_with(3100, exc=self.patches["triage_followup"].side_effect) self.patches["remove_review_label"].assert_not_called() def test_failed_first_pass_triage_marks_failed_instead_of_removing_the_label(self): @@ -2305,7 +2308,7 @@ def test_failed_first_pass_triage_marks_failed_instead_of_removing_the_label(sel self.patches["find_triage_comment"].return_value = False self.patches["triage"].side_effect = subprocess.CalledProcessError(1, ["claude"]) triage_daemon.process_bot_review_issue({"number": 3100, "labels": [], "title": "Old ticket needing review"}) - self.patches["mark_review_failed"].assert_called_once_with(3100) + self.patches["mark_review_failed"].assert_called_once_with(3100, exc=self.patches["triage"].side_effect) self.patches["remove_review_label"].assert_not_called() @patch("builtins.print") @@ -2806,7 +2809,7 @@ def test_failed_cleanup_marks_failed_instead_of_removing_the_label(self): """A cleanup_pr() failure swaps to BOT_FAILED rather than retrying next poll.""" self.patches["cleanup_pr"].side_effect = subprocess.CalledProcessError(1, ["claude"]) triage_daemon.process_bot_cleanup_pr({"number": 4742, "title": "Add confirmed findings"}) - self.patches["mark_pr_cleanup_failed"].assert_called_once_with(4742) + self.patches["mark_pr_cleanup_failed"].assert_called_once_with(4742, exc=self.patches["cleanup_pr"].side_effect) self.patches["remove_pr_cleanup_label"].assert_not_called() def test_a_run_that_exits_cleanly_but_leaves_work_behind_is_a_failure(self): @@ -3304,5 +3307,191 @@ def test_the_slice_is_taken_from_the_front_of_the_queue(self): self.assertEqual(archived, ["1-a.md", "2-b.md"]) +class FailureDetailTests(DaemonPathsTestCase): + """What a BOT_FAILED comment says about why, where and which log - and what it must not say.""" + + def setUp(self): + """Keep the daemon's git lookup out of the mocked subprocess calls the tests inspect.""" + super().setUp() + patcher = patch.object(triage_daemon, "daemon_version", return_value="") + patcher.start() + self.addCleanup(patcher.stop) + + def _flow_failure(self, text="", status=1): + """Run triage() against a mocked claude that exits non-zero, then leave `text` in its log. + + Going through the real function means the exception carries the real traceback and log + path, so the origin and the log name are what production would report. + """ + with patch("triage_daemon.subprocess.run", return_value=MagicMock(returncode=status)): + try: + triage_daemon.triage(4960) + except triage_daemon.FlowFailed as exc: + # Not assertRaises: it strips the traceback from the exception it hands back, + # and the origin is read from exactly that. + with exc.log_path.open("a") as handle: + handle.write(text) + return exc + self.fail("triage() did not raise FlowFailed") + + def test_each_log_signature_is_named(self): + """Every classified cause reaches the comment, so a maintainer need not open the log.""" + cases = { + "Error: Reached max turns (100)": "ran out of turns", + "Exceeded USD budget (10)": "spend limit", + "Credit balance is too low": "out of credit", + "API Error: 401 invalid": "could not authenticate", + "API Error: 429 rate_limit_error": "rate limited", + "overloaded_error": "overloaded", + "Error: read ECONNRESET": "network connection", + } + for line, expected in cases.items(): + with self.subTest(line=line): + exc = self._flow_failure(f"working\n{line}\n") + self.assertIn(expected, triage_daemon.failure_cause(exc)) + + def test_an_unrecognised_failure_falls_back_to_the_exit_status(self): + """A log that says nothing useful still yields an honest, specific sentence.""" + exc = self._flow_failure("something odd\n", status=7) + self.assertEqual(triage_daemon.failure_cause(exc), "The Claude run exited with status 7 and its log shows no recognised cause") + + def test_only_the_last_attempt_in_an_appended_log_counts(self): + """The log is appended to across retries; an earlier attempt's cause must not be blamed.""" + self._flow_failure("Error: Reached max turns (100)\n") + exc = self._flow_failure("API Error: 429 rate_limit_error\n") + self.assertIn("rate limited", triage_daemon.failure_cause(exc)) + + def test_the_agent_quoting_a_signature_mid_run_is_not_a_cause(self): + """An issue about max turns makes the agent write those words; only the end of the run is read.""" + exc = self._flow_failure("The reporter saw Reached max turns\n" + "ordinary output\n" * 600) + self.assertNotIn("turns", triage_daemon.failure_cause(exc)) + + def test_a_missing_log_does_not_raise(self): + """The log may have been pruned; the comment still gets the exit status.""" + exc = triage_daemon.FlowFailed(1, ["claude"], Path("/nonexistent/issue-1.log")) + self.assertIn("exited with status 1", triage_daemon.failure_cause(exc)) + + def test_a_killed_run_names_the_signal(self): + """A negative status is a signal, typically the OOM killer.""" + exc = triage_daemon.FlowFailed(-9, ["claude"], Path("/nonexistent/x.log")) + self.assertIn("SIGKILL", triage_daemon.failure_cause(exc)) + + def test_a_gh_failure_names_the_subcommand_but_not_the_arguments(self): + """A comment body can be an argument of a gh call and must never be echoed back.""" + exc = subprocess.CalledProcessError(1, ["gh", "issue", "comment", "4960", "--repo", "r", "--body", "SECRET-TOKEN"]) + cause = triage_daemon.failure_cause(exc) + self.assertEqual(cause, "`gh issue comment` exited with status 1") + self.assertNotIn("SECRET-TOKEN", cause) + + def test_the_origin_is_the_raise_site_of_the_run(self): + """The line in triage() that raised, not the test, subprocess or unittest frames.""" + exc = self._flow_failure() + self.assertRegex(triage_daemon.failure_origin(exc), r"^triage_daemon\.py:\d+ in triage\(\)") + + def test_a_check_true_failure_reports_the_daemon_line_not_subprocess_py(self): + """check=True raises inside the subprocess module; the useful frame is the call to it.""" + with patch("triage_daemon.subprocess.run", side_effect=subprocess.CalledProcessError(1, ["gh", "pr", "view"])): + try: + triage_daemon.pr_review_activity_count(4742) + except subprocess.CalledProcessError as exc: + self.assertRegex(triage_daemon.failure_origin(exc), r"^triage_daemon\.py:\d+ in pr_review_activity_count\(\)") + return + self.fail("pr_review_activity_count() did not raise") + + def test_the_origin_is_stamped_with_the_daemon_version_when_known(self): + """A line number only means something against the right file.""" + exc = self._flow_failure() + with patch.object(triage_daemon, "daemon_version", return_value="abc1234"): + self.assertIn("(daemon abc1234)", triage_daemon.failure_origin(exc)) + with patch.object(triage_daemon, "daemon_version", return_value=""): + self.assertNotIn("(daemon", triage_daemon.failure_origin(exc)) + + def test_a_clean_exit_failure_names_the_function_that_spotted_it(self): + """A run that exited 0 and posted nothing has no traceback, so the comment names the + process_bot_review_pr() check that noticed instead.""" + for name in ("sync_repo", "reset_scratch", "review_pr"): + patcher = patch.object(triage_daemon, name) + patcher.start() + self.addCleanup(patcher.stop) + patcher = patch.object(triage_daemon, "pr_review_activity_count", return_value=3) + patcher.start() + self.addCleanup(patcher.stop) + with patch("triage_daemon.subprocess.run") as mock_run: + triage_daemon.process_bot_review_pr({"number": 4742, "title": "Add confirmed findings"}) + cmd = mock_run.call_args_list[0].args[0] + body = cmd[cmd.index("--body") + 1] + self.assertIn("**Reason:** The run exited cleanly but posted nothing", body) + self.assertRegex(body, r"\*\*Failed at:\*\* `triage_daemon\.py:\d+ in process_bot_review_pr\(\)") + + def test_the_detail_names_the_log_file_but_never_quotes_it(self): + """A log can hold a reporter's attachment or a token, so only its name is given.""" + exc = self._flow_failure("SECRET-TOKEN\nError: Reached max turns (100)\n") + detail = triage_daemon.failure_detail(exc=exc) + self.assertIn("`issue-4960.log`", detail) + self.assertNotIn("SECRET-TOKEN", detail) + self.assertNotIn(self.tmp_dir.name, detail) + + def test_daemon_version_is_the_short_commit_or_empty(self): + """Outside a git checkout, or with no git binary, the version is simply left off.""" + self.addCleanup(REAL_DAEMON_VERSION.cache_clear) + for run_result, expected in ( + (dict(return_value=MagicMock(returncode=0, stdout="abc1234\n")), "abc1234"), + (dict(return_value=MagicMock(returncode=128, stdout="")), ""), + (dict(side_effect=FileNotFoundError), ""), + ): + REAL_DAEMON_VERSION.cache_clear() + with patch("triage_daemon.subprocess.run", **run_result): + self.assertEqual(REAL_DAEMON_VERSION(), expected) + + def test_reason_and_cause_are_both_given(self): + """A caller's own explanation is kept alongside what the exception shows.""" + exc = subprocess.CalledProcessError(1, ["gh", "pr", "view", "4742"]) + detail = triage_daemon.failure_detail("Unable to sample PR review activity.", exc) + self.assertIn("Unable to sample PR review activity; `gh pr view` exited with status 1.", detail) + + def test_no_detail_when_nothing_is_known(self): + """Nothing to say adds nothing, rather than an empty Reason.""" + with patch.object(triage_daemon, "failure_origin", return_value=""): + self.assertEqual(triage_daemon.failure_detail(), "") + + def test_the_four_comments_carry_the_detail(self): + """Issue triage, issue follow-up, PR review and PR cleanup comments all say why, where and which log.""" + exc = self._flow_failure("Error: Reached max turns (100)\n") + calls = { + "mark_triage_failed": lambda: triage_daemon.mark_triage_failed(4960, 3, exc), + "mark_review_failed": lambda: triage_daemon.mark_review_failed(4960, exc=exc), + "mark_pr_review_failed": lambda: triage_daemon.mark_pr_review_failed(4742, exc=exc), + "mark_pr_cleanup_failed": lambda: triage_daemon.mark_pr_cleanup_failed(4742, exc=exc), + } + for name, call in calls.items(): + with self.subTest(name), patch("triage_daemon.subprocess.run") as mock_run: + call() + cmd = mock_run.call_args_list[0].args[0] + body = cmd[cmd.index("--body") + 1] + self.assertIn("ran out of turns", body) + self.assertIn("**Failed at:** `triage_daemon.py:", body) + self.assertIn("`issue-4960.log`", body) + self.assertNotIn("see the triage bot's logs", body) + + +class FlowFailedRaisedTests(DaemonPathsTestCase): + """The claude-run functions raise FlowFailed so the comment can classify the run's log.""" + + @patch("triage_daemon.subprocess.run") + def test_each_flow_raises_with_its_log(self, mock_run): + """triage, follow-up, PR review and cleanup all carry the path they wrote to.""" + mock_run.return_value = MagicMock(returncode=2) + for run, name in ( + (triage_daemon.triage, "issue-4960.log"), + (triage_daemon.triage_followup, "issue-4960-followup.log"), + (triage_daemon.review_pr, "pr-4960-review.log"), + (triage_daemon.cleanup_pr, "pr-4960-cleanup.log"), + ): + with self.subTest(name=name), self.assertRaises(triage_daemon.FlowFailed) as raised: + run(4960) + self.assertEqual(raised.exception.log_path.name, name) + self.assertEqual(raised.exception.returncode, 2) + + if __name__ == "__main__": unittest.main() diff --git a/tools/triage_daemon.py b/tools/triage_daemon.py index 2a8b59546..5f5ba64d4 100644 --- a/tools/triage_daemon.py +++ b/tools/triage_daemon.py @@ -114,11 +114,15 @@ """ import argparse +import functools import json import os +import re import shutil +import signal import subprocess import time +import traceback from pathlib import Path REPO = "springfall2008/batpred" @@ -715,7 +719,139 @@ def mark_pr_opened(issue_number, cleanup=False): flag_pr_for_review(pr_number, cleanup=cleanup) -def mark_triage_failed(issue_number, attempts): +class FlowFailed(subprocess.CalledProcessError): + """A claude run that exited non-zero, carrying the log its output went to. + + A CalledProcessError on its own says only that something exited non-zero. Keeping the log + path lets failure_cause() look inside the run for why, and lets the comment name the file + a maintainer on the bot's host should open. + """ + + def __init__(self, returncode, cmd, log_path): + """Record the log the failed run wrote to alongside the usual exit status and command.""" + super().__init__(returncode, cmd) + self.log_path = log_path + + +# What a failed claude run says in its own log, most specific first. Matched only against the +# end of the last run's output (see failure_log_cause), because the agent quotes the issue it is +# reading and an issue about "Reached max turns" must not classify its own triage. These are the +# strings Claude Code prints at the point it gives up; an unrecognised failure falls back to the +# exit status, so a wrong or stale entry costs a less specific comment, not a wrong label. +FAILURE_LOG_SIGNATURES = ( + (re.compile(r"Reached max turns", re.IGNORECASE), "Claude ran out of turns before finishing (--max-turns)"), + (re.compile(r"Exceeded USD budget|budget .{0,40}exceeded", re.IGNORECASE), "Claude hit the spend limit set for this run (--max-budget-usd)"), + (re.compile(r"credit balance is too low|usage limit reached|usage limit exceeded", re.IGNORECASE), "The bot's Claude account is out of credit or at its usage limit"), + (re.compile(r"Invalid API key|authentication_error|OAuth token has expired|API Error: 401|Please run /login", re.IGNORECASE), "Claude could not authenticate - the bot's login has expired or been revoked"), + (re.compile(r"API Error: 429|rate_limit_error", re.IGNORECASE), "Claude was rate limited by the API"), + (re.compile(r"API Error: 529|overloaded_error", re.IGNORECASE), "The Claude API reported it was overloaded"), + (re.compile(r"ECONNRESET|ETIMEDOUT|ENOTFOUND|EAI_AGAIN|Connection error|fetch failed", re.IGNORECASE), "Claude lost its network connection to the API"), +) +FAILURE_LOG_SECTION_CHARS = 4000 +FAILURE_LOG_READ_BYTES = 262144 +FAILURE_RUN_START = re.compile(r"^==== .* started .* ====$", re.MULTILINE) + + +def failure_log_cause(log_path): + """Name why the last run in a flow's log gave up, or "" if the log says nothing recognisable. + + The log is appended to across retries, so only the text after the final "started" marker + belongs to this failure, and only its last few thousand characters are searched. + """ + try: + with open(log_path, "rb") as handle: + handle.seek(0, os.SEEK_END) + handle.seek(max(0, handle.tell() - FAILURE_LOG_READ_BYTES)) + text = handle.read().decode("utf-8", errors="replace") + except OSError: + return "" + starts = list(FAILURE_RUN_START.finditer(text)) + if starts: + text = text[starts[-1].end() :] + text = text[-FAILURE_LOG_SECTION_CHARS:] + for pattern, reason in FAILURE_LOG_SIGNATURES: + if pattern.search(text): + return reason + return "" + + +def failure_cause(exc): + """Say in a sentence what failed, from the exit status, the command and the run's log. + + Commands are named by their leading words only: the arguments of a `gh ... --body` call can + be a whole comment, and these sentences are posted publicly. + """ + status = exc.returncode + command = exc.cmd if isinstance(exc.cmd, (list, tuple)) else str(exc.cmd).split() + program = Path(str(command[0])).name if command else "a command" + if status < 0: + try: + name = signal.Signals(-status).name + except ValueError: + name = f"signal {-status}" + return f"`{program}` was killed by {name}, which usually means the bot's host ran out of memory or the process was stopped by hand" + if program in ("gh", "git"): + words = [str(word) for word in command[1:] if not str(word).startswith("-")][:2] + return f"`{' '.join([program] + words)}` exited with status {status}" + cause = failure_log_cause(exc.log_path) if isinstance(exc, FlowFailed) else "" + if cause: + return f"{cause} (exit status {status})" + return f"The Claude run exited with status {status} and its log shows no recognised cause" + + +@functools.lru_cache(maxsize=1) +def daemon_version(): + """Short commit of the checkout the daemon is running from, so a line number can be read + against the right file, or "" when it is not in a git checkout.""" + try: + result = subprocess.run(["git", "-C", str(Path(__file__).resolve().parent), "rev-parse", "--short", "HEAD"], capture_output=True, text=True, check=False) + except OSError: + return "" + return result.stdout.strip() if result.returncode == 0 else "" + + +def failure_origin(exc=None): + """Return "file:line in function()" for where in this file a failure arose. + + With an exception, the innermost frame in this file: a subprocess.run(check=True) fails + inside the subprocess module, but the line that matters is the call in the daemon. Without + one - a run that exited 0 and left its work undone - the place that spotted the problem, + which is the caller of the mark_*_failed() function that called failure_detail(). + """ + this_file = Path(__file__).resolve() + if exc is not None and exc.__traceback__ is not None: + frames = [frame for frame in traceback.extract_tb(exc.__traceback__) if Path(frame.filename).resolve() == this_file] + else: + frames = [frame for frame in traceback.extract_stack() if Path(frame.filename).resolve() == this_file] + while frames and (frames[-1].name in ("failure_origin", "failure_detail") or frames[-1].name.startswith("mark_")): + frames.pop() + if not frames: + return "" + frame = frames[-1] + version = daemon_version() + return f"{this_file.name}:{frame.lineno} in {frame.name}(){f' (daemon {version})' if version else ''}" + + +def failure_detail(reason="", exc=None): + """Build the part of a BOT_FAILED comment that says why, where and which log. + + `reason` is a specific explanation a caller already has; `exc` adds what the exception + itself shows. Never includes log text - only a classification of it - because a log can + hold a reporter's attachment, a token or the bot's paths. + """ + causes = [text for text in (reason, failure_cause(exc) if exc is not None else "") if text] + parts = [] + if causes: + parts.append("**Reason:** " + "; ".join(text.rstrip(".") for text in causes) + ".") + origin = failure_origin(exc) + if origin: + parts.append(f"**Failed at:** `{origin}`.") + if isinstance(exc, FlowFailed): + parts.append(f"**Log:** `{Path(exc.log_path).name}` in the bot's logs directory.") + return (" " + " ".join(parts)) if parts else "" + + +def mark_triage_failed(issue_number, attempts, exc=None): """Note on an issue that triage never completed, label it, and let the queue move on. The watermark advances past the issue afterwards, so nothing re-triages it by itself. @@ -734,7 +870,7 @@ def mark_triage_failed(issue_number, attempts): "--repo", REPO, "--body", - f"Automated triage did not complete for this issue after {attempts} attempts - see the triage bot's logs for details. " + f"Automated triage did not complete for this issue after {attempts} attempts.{failure_detail(exc=exc)} " "It has been skipped so that it does not hold up triage of later issues. Remove `BOT_FAILED` and add `BOT_REVIEW` " "to have the bot look at it again - nothing removes `BOT_FAILED` on its own, so leaving it on would outlive a later successful run.", ], @@ -1251,7 +1387,7 @@ def triage(issue_number): result = subprocess.run(cmd, cwd=str(CLONE_DIR), stdout=log_handle, stderr=subprocess.STDOUT, env=claude_env(review_only=True)) log_handle.write(f"==== issue #{issue_number} exited {result.returncode} ====\n") if result.returncode != 0: - raise subprocess.CalledProcessError(result.returncode, cmd) + raise FlowFailed(result.returncode, cmd, log_path) print(f"[triage] issue #{issue_number}: exited {result.returncode}", flush=True) @@ -1293,7 +1429,7 @@ def triage_followup(issue_number): result = subprocess.run(cmd, cwd=str(CLONE_DIR), stdout=log_handle, stderr=subprocess.STDOUT, env=claude_env(review_only=True)) log_handle.write(f"==== issue #{issue_number} follow-up exited {result.returncode} ====\n") if result.returncode != 0: - raise subprocess.CalledProcessError(result.returncode, cmd) + raise FlowFailed(result.returncode, cmd, log_path) print(f"[review] issue #{issue_number}: follow-up exited {result.returncode}", flush=True) @@ -1621,7 +1757,7 @@ def remove_review_label(issue_number): subprocess.run(["gh", "issue", "edit", str(issue_number), "--repo", REPO, "--remove-label", "BOT_REVIEW"], check=True) -def mark_review_failed(issue_number): +def mark_review_failed(issue_number, exc=None): """Post a note and swap BOT_REVIEW for BOT_FAILED, so a failing triage isn't retried (and re-billed) every poll cycle. Remove BOT_FAILED and re-add BOT_REVIEW to retry once the underlying issue is fixed. @@ -1635,7 +1771,7 @@ def mark_review_failed(issue_number): "--repo", REPO, "--body", - "Automated triage failed to complete for this issue - see the triage bot's logs for details. " "Not retrying automatically; remove `BOT_FAILED` and re-add `BOT_REVIEW` to try again.", + f"Automated triage failed to complete for this issue.{failure_detail(exc=exc)} " "Not retrying automatically; remove `BOT_FAILED` and re-add `BOT_REVIEW` to try again.", ], check=True, ) @@ -1650,13 +1786,15 @@ def remove_pr_review_label(pr_number): subprocess.run(["gh", "pr", "edit", str(pr_number), "--repo", REPO, "--remove-label", "BOT_REVIEW"], check=True) -def mark_pr_review_failed(pr_number, reason=""): +def mark_pr_review_failed(pr_number, reason="", exc=None): """Post a note and swap BOT_REVIEW for BOT_FAILED on a PR, so a failing review isn't retried every poll cycle. Remove BOT_FAILED and re-add BOT_REVIEW to retry. `reason` - names the specific failure when there is one - "see the logs" is poor advice for the - run that exits 0 having posted nothing, because its log reads like a finished review. + names the specific failure when there is one, and `exc` is the exception behind it, if + any: the comment says why, where in the daemon it arose and which log to open. "See the + logs" alone is poor advice for the run that exits 0 having posted nothing, because its + log reads like a finished review. """ - detail = f" {reason}" if reason else "" + detail = failure_detail(reason, exc) subprocess.run( [ "gh", @@ -1666,7 +1804,7 @@ def mark_pr_review_failed(pr_number, reason=""): "--repo", REPO, "--body", - f"Automated review failed to complete for this PR - see the triage bot's logs for details.{detail} " "Not retrying automatically; remove `BOT_FAILED` and re-add `BOT_REVIEW` to try again.", + f"Automated review failed to complete for this PR.{detail} " "Not retrying automatically; remove `BOT_FAILED` and re-add `BOT_REVIEW` to try again.", ], check=True, ) @@ -1709,12 +1847,12 @@ def mark_pr_cleanup_unsupported(pr_number): ) -def mark_pr_cleanup_failed(pr_number, reason=""): +def mark_pr_cleanup_failed(pr_number, reason="", exc=None): """Post a note and swap BOT_CLEANUP for BOT_FAILED on a PR, so a failing cleanup isn't retried every poll cycle. Remove BOT_FAILED and re-add BOT_CLEANUP to retry. - `reason` names the failure when there is one, as for mark_pr_review_failed(). + `reason` and `exc` name the failure when there is one, as for mark_pr_review_failed(). """ - detail = f" {reason}" if reason else "" + detail = failure_detail(reason, exc) subprocess.run( [ "gh", @@ -1724,7 +1862,7 @@ def mark_pr_cleanup_failed(pr_number, reason=""): "--repo", REPO, "--body", - f"Automated cleanup failed to complete for this PR - see the triage bot's logs for details.{detail} " "Not retrying automatically; remove `BOT_FAILED` and re-add `BOT_CLEANUP` to try again.", + f"Automated cleanup failed to complete for this PR.{detail} " "Not retrying automatically; remove `BOT_FAILED` and re-add `BOT_CLEANUP` to try again.", ], check=True, ) @@ -1767,7 +1905,7 @@ def process_bot_review_issue(issue): action(issue_number) except subprocess.CalledProcessError as exc: print(f"[review] issue #{issue_number}: {action_name} failed: {exc}", flush=True) - mark_review_failed(issue_number) + mark_review_failed(issue_number, exc=exc) return remove_review_label(issue_number) @@ -1809,7 +1947,7 @@ def review_pr(pr_number): result = subprocess.run(cmd, cwd=str(CLONE_DIR), stdout=log_handle, stderr=subprocess.STDOUT, env=claude_env(review_only=True)) log_handle.write(f"==== PR #{pr_number} review exited {result.returncode} ====\n") if result.returncode != 0: - raise subprocess.CalledProcessError(result.returncode, cmd) + raise FlowFailed(result.returncode, cmd, log_path) print(f"[review-pr] PR #{pr_number}: exited {result.returncode}", flush=True) @@ -1845,21 +1983,21 @@ def process_bot_review_pr(pr): before = pr_review_activity_count(pr_number) except subprocess.CalledProcessError as exc: print(f"[review-pr] PR #{pr_number}: failed to sample activity count before review: {exc}", flush=True) - mark_pr_review_failed(pr_number, "Unable to sample PR review activity before running the review, so the result could not be verified.") + mark_pr_review_failed(pr_number, "Unable to sample PR review activity before running the review, so the result could not be verified.", exc) return try: review_pr(pr_number) except subprocess.CalledProcessError as exc: print(f"[review-pr] PR #{pr_number}: review failed: {exc}", flush=True) - mark_pr_review_failed(pr_number) + mark_pr_review_failed(pr_number, exc=exc) return try: after = pr_review_activity_count(pr_number) except subprocess.CalledProcessError as exc: print(f"[review-pr] PR #{pr_number}: failed to sample activity count after review: {exc}", flush=True) - mark_pr_review_failed(pr_number, "The review run finished, but the activity count check failed, so it could not be verified that anything was posted.") + mark_pr_review_failed(pr_number, "The review run finished, but the activity count check failed, so it could not be verified that anything was posted.", exc) return if after <= before: @@ -1911,7 +2049,7 @@ def cleanup_pr(pr_number): result = subprocess.run(cmd, cwd=str(CLONE_DIR), stdout=log_handle, stderr=subprocess.STDOUT, env=claude_env()) log_handle.write(f"==== PR #{pr_number} cleanup exited {result.returncode} ====\n") if result.returncode != 0: - raise subprocess.CalledProcessError(result.returncode, cmd) + raise FlowFailed(result.returncode, cmd, log_path) print(f"[cleanup-pr] PR #{pr_number}: exited {result.returncode}", flush=True) @@ -1967,7 +2105,7 @@ def process_new_issue(issue, state): return False print(f"[triage] issue #{number}: failed {count} times - marking it and moving on", flush=True) try: - mark_triage_failed(number, count) + mark_triage_failed(number, count, exc) except subprocess.CalledProcessError as mark_exc: # Advancing matters more than the label. Letting this escape would abort the rest # of the poll cycle and leave the watermark behind - the precise failure this @@ -2003,7 +2141,7 @@ def process_bot_cleanup_pr(pr): cleanup_pr(pr_number) except subprocess.CalledProcessError as exc: print(f"[cleanup-pr] PR #{pr_number}: cleanup failed: {exc}", flush=True) - mark_pr_cleanup_failed(pr_number) + mark_pr_cleanup_failed(pr_number, exc=exc) return reason = unfinished_cleanup_reason() if reason: