From e1b5b8b30a72df6da43231e7aa2e0319d5284308 Mon Sep 17 00:00:00 2001 From: CI Date: Sat, 26 Sep 2026 10:04:03 +0100 Subject: [PATCH] fix(triage-bot): stop a cleanup run that exits early from wedging the daemon On 2026-09-26 the PR #5216 cleanup backgrounded its pre-commit run, ended on "I'll push and reply once it finishes", and exited 0 with its fixes uncommitted. The daemon took exit 0 as success and cleared BOT_CLEANUP; the next sync_repo() then failed at `git checkout main` because the leftover edits would be overwritten - and since every flow starts with sync_repo(), every flow after it failed the same way. - claude_env() now sets CLAUDE_CODE_DISABLE_BACKGROUND_TASKS=1 for every claude invocation, which removes run_in_background from the Bash tool (checked against Claude Code 2.1.283). A -p session cannot come back for a background result. - sync_repo() stashes anything left uncommitted (aborting an unfinished merge first) before checking out main, so leftovers are kept but can no longer block the checkout. - process_bot_cleanup_pr() checks the clone after a clean exit and marks the PR BOT_FAILED, with the reason, when work was left uncommitted or unpushed. - pr-cleanup SKILL.md: run the quality gate in the foreground, and drop the false claim that an unpushed commit is discarded on the next run. Co-Authored-By: Claude Opus 5.5 (1M context) --- .claude/skills/pr-cleanup/SKILL.md | 6 +- tools/test_triage_daemon.py | 179 ++++++++++++++++++++++++++--- tools/triage_daemon.py | 83 +++++++++++-- 3 files changed, 243 insertions(+), 25 deletions(-) diff --git a/.claude/skills/pr-cleanup/SKILL.md b/.claude/skills/pr-cleanup/SKILL.md index d87ad0379..ca5ea4774 100644 --- a/.claude/skills/pr-cleanup/SKILL.md +++ b/.claude/skills/pr-cleanup/SKILL.md @@ -71,7 +71,11 @@ tools/triage_test.sh /test.log Use the test module the changed area maps to in `TEST_REGISTRY` (`apps/predbat/unit_test.py`), or `./run_all --quick` (from `coverage/`, same as above) if there's no clean single-module mapping. If either still fails, go to step 7 and report what's still broken — do not push a change that doesn't pass its own quality gate, -even if step 2's merge commit already exists locally: an unpushed local commit is discarded automatically the next time this runs. +even if step 2's merge commit already exists locally. + +Run both in the foreground and wait for them to finish; the pre-commit run plus the quick suite takes a few minutes, well inside a single command's timeout. This is a non-interactive run: once you write your final message the session is over, and nothing comes back later to read a result or finish the job. Never end on "still running, I'll push when it finishes". PR #5216's cleanup did exactly that on 2026-09-26: its fixes were never committed, and the leftover edits blocked every later bot run until they were cleared by hand. + +The daemon checks the clone after you exit. Uncommitted changes or unpushed commits mark the run failed (`BOT_FAILED`), and they are stashed rather than discarded. A run that stops at a failed quality gate is therefore reported as a failure even after posting its summary comment, which is correct, because the cleanup did not complete. ## 7. Commit, push, and reply diff --git a/tools/test_triage_daemon.py b/tools/test_triage_daemon.py index 3b0da5544..bad78a9a9 100644 --- a/tools/test_triage_daemon.py +++ b/tools/test_triage_daemon.py @@ -838,6 +838,107 @@ def test_checks_out_main_before_resetting(self, mock_run): self.assertIn(checkout_call, calls) self.assertLess(calls.index(checkout_call), calls.index(reset_call)) + @patch("triage_daemon.subprocess.run") + def test_stashes_leftover_changes_before_checking_out_main(self, mock_run): + """The 2026-09-26 wedge: a PR #5216 cleanup exited 0 with its edits uncommitted, and + `git checkout main` refused to overwrite them - so every later flow failed at the + same step. The leftovers are stashed first, which unblocks the checkout and keeps the + work recoverable rather than letting reset --hard destroy it.""" + triage_daemon.sync_repo() + calls = [call.args[0] for call in mock_run.call_args_list] + stash_calls = [c for c in calls if c[3:5] == ["stash", "push"]] + checkout_call = ["git", "-C", str(triage_daemon.CLONE_DIR), "checkout", "main"] + self.assertEqual(len(stash_calls), 1) + self.assertIn("triage-daemon", stash_calls[0][stash_calls[0].index("-m") + 1]) + self.assertLess(calls.index(stash_calls[0]), calls.index(checkout_call)) + + +class SetAsideLeftoversTests(DaemonPathsTestCase): + """set_aside_leftovers() - what sync_repo() does with a tree the previous flow left dirty.""" + + @patch("builtins.print") + @patch("triage_daemon.subprocess.run") + def test_reports_what_it_stashed(self, mock_run, mock_print): + """A stash that saved something is named in the daemon's own output, so the operator + knows there is work to recover and where it went.""" + mock_run.return_value = MagicMock(returncode=0, stdout="Saved working directory and index state On fix/x: triage-daemon\n") + triage_daemon.set_aside_leftovers() + printed = " ".join(str(call.args[0]) for call in mock_print.call_args_list) + self.assertIn("git stash list", printed) + + @patch("builtins.print") + @patch("triage_daemon.subprocess.run") + def test_is_silent_on_a_clean_tree(self, mock_run, mock_print): + """The normal case - nothing left behind - must not print a warning every flow.""" + mock_run.return_value = MagicMock(returncode=0, stdout="No local changes to save\n") + triage_daemon.set_aside_leftovers() + mock_print.assert_not_called() + + @patch("builtins.print") + @patch("triage_daemon.subprocess.run") + def test_aborts_an_unfinished_merge_first(self, mock_run, mock_print): + """`git stash` refuses a tree with unmerged paths, and `git checkout` refuses one + mid-merge, so a run that stopped inside a conflicted merge would wedge the clone the + same way. The merge is aborted first; it can be redone from origin/main at any time.""" + git_dir = triage_daemon.CLONE_DIR / ".git" + git_dir.mkdir(parents=True, exist_ok=True) + (git_dir / "MERGE_HEAD").write_text("abc123\n") + mock_run.return_value = MagicMock(returncode=0, stdout="No local changes to save\n") + triage_daemon.set_aside_leftovers() + calls = [call.args[0] for call in mock_run.call_args_list] + abort_call = ["git", "-C", str(triage_daemon.CLONE_DIR), "merge", "--abort"] + self.assertIn(abort_call, calls) + self.assertLess(calls.index(abort_call), next(i for i, c in enumerate(calls) if c[3:5] == ["stash", "push"])) + + @patch("triage_daemon.subprocess.run") + def test_no_merge_abort_without_a_merge_in_progress(self, mock_run): + """Aborting only ever runs when there is a merge to abort.""" + mock_run.return_value = MagicMock(returncode=0, stdout="No local changes to save\n") + triage_daemon.set_aside_leftovers() + calls = [call.args[0] for call in mock_run.call_args_list] + self.assertFalse(any(c[3:5] == ["merge", "--abort"] for c in calls)) + + +class UnfinishedCleanupReasonTests(unittest.TestCase): + """unfinished_cleanup_reason() - the check that a cleanup which exited 0 actually finished.""" + + @staticmethod + def _git(status="", ahead="0\n", ahead_returncode=0): + """Fake subprocess.run answering `git status` and `git rev-list --count` as given.""" + + def run(cmd, **kwargs): + """Answer one git call.""" + if "status" in cmd: + return MagicMock(returncode=0, stdout=status) + return MagicMock(returncode=ahead_returncode, stdout=ahead) + + return run + + def test_a_clean_pushed_branch_is_finished(self): + """Nothing uncommitted, nothing unpushed: the run did what it said.""" + with patch("triage_daemon.subprocess.run", side_effect=self._git()): + self.assertEqual(triage_daemon.unfinished_cleanup_reason(), "") + + def test_uncommitted_changes_are_unfinished(self): + """PR #5216 on 2026-09-26: the run backgrounded its quality gate, said it would push + once it finished, and exited 0 - with two files edited and never committed.""" + with patch("triage_daemon.subprocess.run", side_effect=self._git(status=" M apps/predbat/givtcp.py\n M apps/predbat/tests/test_givtcp_component.py\n")): + reason = triage_daemon.unfinished_cleanup_reason() + self.assertIn("2 file(s)", reason) + self.assertIn("uncommitted", reason) + + def test_unpushed_commits_are_unfinished(self): + """The same run's local merge of origin/main was never pushed either.""" + with patch("triage_daemon.subprocess.run", side_effect=self._git(ahead="1\n")): + reason = triage_daemon.unfinished_cleanup_reason() + self.assertIn("1 local commit(s)", reason) + + def test_an_unknowable_upstream_is_unfinished(self): + """No upstream (detached HEAD, or the PR branch was never checked out) means the + push cannot be confirmed - which is not the same as confirming it happened.""" + with patch("triage_daemon.subprocess.run", side_effect=self._git(ahead="", ahead_returncode=128)): + self.assertNotEqual(triage_daemon.unfinished_cleanup_reason(), "") + class OllamaContextWindowTests(unittest.TestCase): """Claude Code does not recognise the Ollama model names and assumes a 200k window, @@ -878,9 +979,8 @@ def test_an_operator_override_wins(self): self.assertEqual(env["CLAUDE_CODE_MAX_CONTEXT_TOKENS"], "250000") def test_no_window_is_set_without_an_ollama_model(self): - """The default Claude route inherits the daemon's environment untouched - claude_env() - returns None there, so there is nothing to set a window on.""" - self.assertIsNone(triage_daemon.claude_env()) + """The default Claude route inherits the daemon's environment, so no window is set on it.""" + self.assertNotIn("CLAUDE_CODE_MAX_CONTEXT_TOKENS", triage_daemon.claude_env()) def test_the_review_only_route_gets_it_too(self): """--ollama_review is how the daemon is actually run, so the window has to follow that @@ -1403,10 +1503,31 @@ def setUp(self): patcher.start() self.addCleanup(patcher.stop) - def test_none_by_default(self): - """With no --ollama, env=None so subprocess.run() inherits the daemon's own - environment unchanged - no ANTHROPIC_* overrides pointing at Ollama.""" - self.assertIsNone(triage_daemon.claude_env()) + def test_inherits_the_daemon_environment_by_default(self): + """With no --ollama the subprocess gets the daemon's own environment - no ANTHROPIC_* + overrides pointing at Ollama - plus only the background-task switch.""" + self.assertEqual(triage_daemon.claude_env(), {**os.environ, "CLAUDE_CODE_DISABLE_BACKGROUND_TASKS": "1"}) + + def test_background_tasks_are_disabled_on_every_route(self): + """A `claude -p` run is over when its final message is written, so a command it + backgrounded is killed with it and nothing ever reads the result. PR #5216's cleanup + backgrounded its pre-commit run, said it would push once that finished, and exited 0 + with its fixes uncommitted. The switch removes run_in_background from the Bash tool + (verified against Claude Code 2.1.283), on the Claude and the Ollama routes alike.""" + for model_name, review_only in ((None, False), ("OLLAMA_MODEL", False), ("OLLAMA_REVIEW_MODEL", True)): + with self.subTest(model=model_name): + if model_name: + with patch.object(triage_daemon, model_name, "glm-5.3-flash:cloud"): + env = triage_daemon.claude_env(review_only=review_only) + else: + env = triage_daemon.claude_env() + self.assertEqual(env["CLAUDE_CODE_DISABLE_BACKGROUND_TASKS"], "1") + + def test_an_operator_cannot_re_enable_background_tasks(self): + """Unlike the context window, this is a correctness guard, not a tuning knob: a value + exported in the daemon's shell must not switch backgrounding back on.""" + with patch.dict("os.environ", {"CLAUDE_CODE_DISABLE_BACKGROUND_TASKS": "0"}): + self.assertEqual(triage_daemon.claude_env()["CLAUDE_CODE_DISABLE_BACKGROUND_TASKS"], "1") def test_adds_the_documented_overrides_when_configured(self): """--ollama sets exactly the three env vars Ollama's integration guide documents.""" @@ -1433,7 +1554,8 @@ def test_review_only_env_ignored_by_default(self): """claude_env() with no argument - the form create_pr() uses - ignores --ollama_review.""" with patch.object(triage_daemon, "OLLAMA_REVIEW_MODEL", "glm-5.3-flash:cloud"): env = triage_daemon.claude_env() - self.assertIsNone(env) + self.assertNotIn("glm-5.3-flash:cloud", env.values()) + self.assertEqual(env.get("ANTHROPIC_BASE_URL"), os.environ.get("ANTHROPIC_BASE_URL")) class ClaudeBudgetArgsTests(unittest.TestCase): @@ -1532,7 +1654,7 @@ def test_no_model_flag_or_env_override_by_default(self, mock_run): triage_daemon.triage(4720) cmd = mock_run.call_args[0][0] self.assertNotIn("--model", cmd) - self.assertIsNone(mock_run.call_args.kwargs["env"]) + self.assertEqual(mock_run.call_args.kwargs["env"].get("ANTHROPIC_BASE_URL"), os.environ.get("ANTHROPIC_BASE_URL")) @patch("triage_daemon.subprocess.run") def test_adds_the_ollama_model_flag_and_env_when_configured(self, mock_run): @@ -1627,7 +1749,7 @@ def test_ollama_review_model_does_not_apply_to_pr_creation(self, mock_run): triage_daemon.create_pr(4720) cmd = mock_run.call_args[0][0] self.assertNotIn("--model", cmd) - self.assertIsNone(mock_run.call_args.kwargs["env"]) + self.assertEqual(mock_run.call_args.kwargs["env"].get("ANTHROPIC_BASE_URL"), os.environ.get("ANTHROPIC_BASE_URL")) @patch("triage_daemon.subprocess.run") def test_budget_cap_still_applies_under_ollama_review(self, mock_run): @@ -2177,6 +2299,18 @@ def test_comments_then_swaps_labels(self, mock_run): ) +class MarkPrCleanupFailedReasonTests(unittest.TestCase): + """mark_pr_cleanup_failed() with a reason - the run exited 0, so its log looks finished.""" + + @patch("triage_daemon.subprocess.run") + def test_the_reason_reaches_the_comment(self, mock_run): + """ "See the logs" is poor advice when the log reads like a finished run.""" + triage_daemon.mark_pr_cleanup_failed(5216, "Left 2 file(s) uncommitted.") + body = mock_run.call_args_list[0].args[0][mock_run.call_args_list[0].args[0].index("--body") + 1] + self.assertTrue(body.startswith("Automated")) + self.assertIn("Left 2 file(s) uncommitted.", body) + + class CleanupPrTests(DaemonPathsTestCase): """Tests for cleanup_pr(), new - runs /pr-cleanup under the write-capable cleanup permission set.""" @@ -2235,7 +2369,7 @@ def test_ollama_review_model_does_not_apply_to_cleanup(self, mock_run): triage_daemon.cleanup_pr(4742) cmd = mock_run.call_args[0][0] self.assertNotIn("glm-5.3-flash:cloud", cmd) - self.assertIsNone(mock_run.call_args.kwargs["env"], "cleanup must inherit the daemon environment, not the Ollama overrides") + self.assertEqual(mock_run.call_args.kwargs["env"].get("ANTHROPIC_BASE_URL"), os.environ.get("ANTHROPIC_BASE_URL"), "cleanup must inherit the daemon environment, not the Ollama overrides") class PushGuardHookTests(DaemonPathsTestCase): @@ -2375,10 +2509,12 @@ class ForkHeadSkipTests(unittest.TestCase): def setUp(self): """Patch every collaborator process_bot_cleanup_pr() calls.""" self.patches = {} - for name in ["sync_repo", "reset_scratch", "cleanup_pr", "remove_pr_cleanup_label", "mark_pr_cleanup_failed", "mark_pr_cleanup_unsupported"]: + for name in ["sync_repo", "reset_scratch", "cleanup_pr", "unfinished_cleanup_reason", "remove_pr_cleanup_label", "mark_pr_cleanup_failed", "mark_pr_cleanup_unsupported"]: patcher = patch.object(triage_daemon, name) self.patches[name] = patcher.start() self.addCleanup(patcher.stop) + # A finished run by default - the tests that want an unfinished one say so + self.patches["unfinished_cleanup_reason"].return_value = "" def test_a_fork_head_pr_is_never_checked_out_or_cleaned(self): """Nothing should run against a PR whose fixes could not be pushed back anyway.""" @@ -2400,10 +2536,12 @@ class ProcessBotCleanupPrTests(unittest.TestCase): def setUp(self): """Patch every collaborator process_bot_cleanup_pr() calls.""" self.patches = {} - for name in ["sync_repo", "reset_scratch", "cleanup_pr", "remove_pr_cleanup_label", "mark_pr_cleanup_failed", "mark_pr_cleanup_unsupported"]: + for name in ["sync_repo", "reset_scratch", "cleanup_pr", "unfinished_cleanup_reason", "remove_pr_cleanup_label", "mark_pr_cleanup_failed", "mark_pr_cleanup_unsupported"]: patcher = patch.object(triage_daemon, name) self.patches[name] = patcher.start() self.addCleanup(patcher.stop) + # A finished run by default - the tests that want an unfinished one say so + self.patches["unfinished_cleanup_reason"].return_value = "" def test_cleans_up_then_removes_the_label(self): """A successful cleanup syncs, cleans up, then removes BOT_CLEANUP.""" @@ -2421,6 +2559,21 @@ def test_failed_cleanup_marks_failed_instead_of_removing_the_label(self): self.patches["mark_pr_cleanup_failed"].assert_called_once_with(4742) self.patches["remove_pr_cleanup_label"].assert_not_called() + def test_a_run_that_exits_cleanly_but_leaves_work_behind_is_a_failure(self): + """PR #5216 on 2026-09-26: the run exited 0 with its fixes uncommitted and its merge + unpushed, and BOT_CLEANUP was removed as though it had finished - leaving the PR with no + push, no reply and nothing marking it for a retry.""" + self.patches["unfinished_cleanup_reason"].return_value = "The run exited cleanly but left uncommitted changes." + triage_daemon.process_bot_cleanup_pr({"number": 5216, "title": "fix(givtcp)"}) + self.patches["mark_pr_cleanup_failed"].assert_called_once_with(5216, "The run exited cleanly but left uncommitted changes.") + self.patches["remove_pr_cleanup_label"].assert_not_called() + + def test_a_failed_invocation_is_not_checked_for_leftovers_too(self): + """A non-zero exit is already a failure; checking the tree as well would mark it twice.""" + self.patches["cleanup_pr"].side_effect = subprocess.CalledProcessError(1, ["claude"]) + triage_daemon.process_bot_cleanup_pr({"number": 4742, "title": "Add confirmed findings"}) + self.patches["unfinished_cleanup_reason"].assert_not_called() + def test_waits_while_the_pr_still_carries_bot_review(self): """Cleanup acts on the review's findings, so with both labels set the review goes first. Nothing runs and BOT_CLEANUP stays, so a later poll picks the PR up again diff --git a/tools/triage_daemon.py b/tools/triage_daemon.py index 5b6c966ec..7aa929815 100644 --- a/tools/triage_daemon.py +++ b/tools/triage_daemon.py @@ -831,14 +831,37 @@ def refresh_gitnexus_index(): print(f"[triage] gitnexus: analyze failed ({result.returncode}) - flows will use the previous index", flush=True) +def set_aside_leftovers(): + """Stash whatever the previous flow left uncommitted in the clone, so main can be checked out. + + `git checkout main` refuses to overwrite local changes to a file that differs between the + two branches, and sync_repo() runs before every flow - so one run that exits with its edits + uncommitted wedges the daemon for good. PR #5216's cleanup did exactly that on 2026-09-26 + and every later flow failed at the checkout. Stashing, rather than `checkout --force`, keeps + the work recoverable: the daemon has no way to tell a half-finished fix from noise. + + An unfinished merge is aborted first, because `git stash` refuses a tree with unmerged paths + and `git checkout` refuses one mid-merge. The merge itself is always origin/main into a PR + branch, so nothing is lost that cannot be redone. + """ + if (CLONE_DIR / ".git" / "MERGE_HEAD").exists(): + subprocess.run(["git", "-C", str(CLONE_DIR), "merge", "--abort"], check=False) + label = f"triage-daemon: left behind by the previous flow, set aside {time.strftime('%Y-%m-%d %H:%M:%S')}" + result = subprocess.run(["git", "-C", str(CLONE_DIR), "stash", "push", "-m", label], capture_output=True, text=True, check=True) + if "Saved working directory" in result.stdout: + print(f"[triage] warning: the previous flow left uncommitted changes in the clone - stashed as '{label}' (see `git stash list` in {CLONE_DIR})", flush=True) + + def sync_repo(): """Sync the clone to origin/main, always returning to main first. A crashed BOT_PR run can leave the clone checked out on a fix/*|feat/* branch; without an explicit checkout, reset --hard would reset that branch instead of - main, leaving the clone stuck off main for every subsequent operation. + main, leaving the clone stuck off main for every subsequent operation. Anything + such a run left uncommitted is stashed first - see set_aside_leftovers(). """ subprocess.run(["git", "-C", str(CLONE_DIR), "fetch", "origin", "main"], check=True) + set_aside_leftovers() subprocess.run(["git", "-C", str(CLONE_DIR), "checkout", "main"], check=True) subprocess.run(["git", "-C", str(CLONE_DIR), "reset", "--hard", "origin/main"], check=True) # Drop untracked leftovers from the previous run's investigation. Not -x: @@ -878,16 +901,24 @@ def claude_model_args(review_only=False): def claude_env(review_only=False): - """Return the subprocess environment for a 'claude' invocation: None (inherit - the daemon's own environment unchanged) unless this invocation is using an - Ollama model, in which case add the Anthropic-compatible overrides Ollama's - Claude Code integration documents, so the CLI talks to the local Ollama server - instead of Anthropic's API, plus the model's real context window where we know it. + """Return the subprocess environment for a 'claude' invocation: the daemon's own + environment with background tasks switched off, plus - when this invocation is using + an Ollama model - the Anthropic-compatible overrides Ollama's Claude Code integration + documents, so the CLI talks to the local Ollama server instead of Anthropic's API, + and the model's real context window where we know it. """ + env = os.environ.copy() + # A `claude -p` run is over the moment it writes its final message, and a command it put + # in the background dies with it - nothing ever comes back to read the result. PR #5216's + # cleanup (2026-09-26) backgrounded its pre-commit run, ended on "I'll push and reply once + # it finishes", and exited 0 with its fixes uncommitted, which then wedged the clone for + # every later flow. This removes run_in_background from the Bash tool outright (checked + # against Claude Code 2.1.283). Assigned rather than setdefault: it is a correctness guard, + # not a tuning knob, so an exported value must not turn backgrounding back on. + env["CLAUDE_CODE_DISABLE_BACKGROUND_TASKS"] = "1" model = effective_ollama_model(review_only) if not model: - return None - env = os.environ.copy() + return env env["ANTHROPIC_BASE_URL"] = OLLAMA_BASE_URL env["ANTHROPIC_AUTH_TOKEN"] = "ollama" env["ANTHROPIC_API_KEY"] = "" @@ -1449,10 +1480,12 @@ def mark_pr_cleanup_unsupported(pr_number): ) -def mark_pr_cleanup_failed(pr_number): +def mark_pr_cleanup_failed(pr_number, reason=""): """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(). """ + detail = f" {reason}" if reason else "" subprocess.run( [ "gh", @@ -1462,7 +1495,7 @@ def mark_pr_cleanup_failed(pr_number): "--repo", REPO, "--body", - "Automated cleanup failed to complete for this PR - see the triage bot's logs for details. " "Not retrying automatically; remove `BOT_FAILED` and re-add `BOT_CLEANUP` to try again.", + 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.", ], check=True, ) @@ -1653,6 +1686,28 @@ def cleanup_pr(pr_number): print(f"[cleanup-pr] PR #{pr_number}: exited {result.returncode}", flush=True) +def unfinished_cleanup_reason(): + """Return why a cleanup run that exited 0 did not actually finish, or "" if it did. + + Every path through /pr-cleanup ends with the clone clean and level with its upstream: + fixes committed and pushed, or nothing to push. Exit status cannot tell those apart from a + run that stopped early - PR #5216's cleanup exited 0 on 2026-09-26 with two files edited and + a merge of origin/main unpushed, and BOT_CLEANUP was cleared as though it had finished. The + clone's state is the evidence, the same way process_bot_review_pr() counts comments. + """ + status = subprocess.run(["git", "-C", str(CLONE_DIR), "status", "--porcelain", "--untracked-files=no"], capture_output=True, text=True, check=False) + changed = [line for line in status.stdout.splitlines() if line.strip()] + if changed: + return f"The run exited cleanly but left {len(changed)} file(s) with uncommitted changes, so its fixes were never pushed. They are stashed in the bot's clone before the next flow runs." + ahead = subprocess.run(["git", "-C", str(CLONE_DIR), "rev-list", "--count", "@{upstream}..HEAD"], capture_output=True, text=True, check=False) + if ahead.returncode != 0: + return "The run exited cleanly, but its branch has no upstream to compare against, so there is no evidence that anything was pushed." + count = int(ahead.stdout.strip() or 0) + if count: + return f"The run exited cleanly but left {count} local commit(s) that were never pushed." + return "" + + def process_new_issue(issue, state): """Triage one new issue. Returns False when the caller should stop for this poll cycle. @@ -1698,7 +1753,8 @@ def process_new_issue(issue, state): def process_bot_cleanup_pr(pr): """Run the BOT_CLEANUP flow for one PR: address review feedback and CI failures, then remove the trigger label. A failed run swaps to BOT_FAILED instead, with an - explanatory comment. + explanatory comment - and so does one that exits 0 having left its work uncommitted + or unpushed (see unfinished_cleanup_reason()). A PR still carrying BOT_REVIEW is left alone, label and all: the cleanup acts on the review's findings, so the review goes first and a later poll picks this PR up again. @@ -1720,6 +1776,11 @@ def process_bot_cleanup_pr(pr): print(f"[cleanup-pr] PR #{pr_number}: cleanup failed: {exc}", flush=True) mark_pr_cleanup_failed(pr_number) return + reason = unfinished_cleanup_reason() + if reason: + print(f"[cleanup-pr] PR #{pr_number}: exited cleanly but did not finish - {reason}", flush=True) + mark_pr_cleanup_failed(pr_number, reason) + return remove_pr_cleanup_label(pr_number)