Merge pull request 'fix: surface skipped tracker cleanup on merge read-back failure (#98)' (#99) from fix/issue-98-merge-cleanup-status into master

mcp_server.py

@@ -1098,16 +1098,30 @@ def gitea_merge_pr(
             payload["MergeMessageField"] = message
         api_request("POST", merge_url, auth, payload)
         # Best-effort read-back of the merge commit SHA (redacted on error).
+        merged = None
         try:
             merged = api_request(
                 "GET", f"{repo_api_url(h, o, r)}/pulls/{pr_number}", auth
             )
             result["merge_commit"] = (merged or {}).get("merged_commit_sha")
-
-            cleanup = cleanup_in_progress_for_pr(merged or {}, remote, host, org, repo)
-            result["cleanup_status"] = cleanup.get("cleanup_status")
         except Exception:
             result["merge_commit"] = None
+
+        # Tracker cleanup (#98): never blocks the merge, and always surfaces an
+        # explicit cleanup_status once the merge mutation has happened. Cleanup
+        # needs the PR title/body/branch, which only the read-back payload
+        # carries here — so a failed read-back is an explicit skip, not a
+        # silent one.
+        if merged is None:
+            result["cleanup_status"] = "skipped (merge read-back failed)"
+        else:
+            try:
+                cleanup = cleanup_in_progress_for_pr(merged, remote, host, org, repo)
+                result["cleanup_status"] = cleanup.get("cleanup_status")
+            except Exception as cleanup_exc:  # noqa: BLE001 — redact before surfacing
+                result["cleanup_status"] = (
+                    f"skipped (cleanup error: {_redact(str(cleanup_exc))})"
+                )
     except Exception as exc:  # noqa: BLE001 — redact before surfacing
         reasons.append(f"merge failed: {_redact(str(exc))}")
         return result

tests/test_mcp_server.py

@@ -370,6 +370,55 @@ class TestMergePR(unittest.TestCase):
                 expected_changed_files=["b.py", "a.py"], remote="prgs")
         self.assertTrue(r["performed"])
 
+    # -- read-back / cleanup surfacing (#98) -----------------------------------
+
+    @patch("mcp_server.api_request")
+    @patch("mcp_server.get_auth_header", return_value=FAKE_AUTH)
+    def test_readback_failure_reports_skipped_cleanup(self, _auth, mock_api):
+        """Merge OK + read-back GET failure => explicit cleanup skip, not silence."""
+        mock_api.side_effect = [
+            {"login": "merger-bot"}, self._pr("author-bot"),
+            {},  # merge POST
+            RuntimeError("HTTP 502: Gitea upstream unavailable"),  # read-back fails
+        ]
+        env = {"GITEA_PROFILE_NAME": "gitea-merger",
+               "GITEA_ALLOWED_OPERATIONS": "read,merge"}
+        with patch.dict(os.environ, env, clear=True):
+            r = gitea_merge_pr(pr_number=8, confirmation=self._confirm(8),
+                               remote="prgs")
+        # The merge itself is still reported performed/successful.
+        self.assertTrue(r["performed"])
+        self.assertEqual(r["merge_result"], "PR #8 merged via 'merge'.")
+        self.assertIsNone(r["merge_commit"])
+        # The skip is explicit, not silent.
+        self.assertEqual(r["cleanup_status"], "skipped (merge read-back failed)")
+        # No tracker-cleanup API traffic after the failed read-back:
+        # user, PR (eligibility), merge POST, read-back — and nothing more.
+        self.assertEqual(mock_api.call_count, 4)
+        for c in mock_api.call_args_list:
+            self.assertNotEqual(c.args[0], "DELETE")
+
+    @patch("mcp_server.cleanup_in_progress_for_pr",
+           side_effect=RuntimeError("boom token secret-xyz"))
+    @patch("mcp_server.api_request")
+    @patch("mcp_server.get_auth_header", return_value=FAKE_AUTH)
+    def test_cleanup_exception_surfaced_and_redacted(self, _auth, mock_api, _cleanup):
+        """Unexpected cleanup exception => merge still succeeds; error surfaced redacted."""
+        mock_api.side_effect = [
+            {"login": "merger-bot"}, self._pr("author-bot"),
+            {},  # merge POST
+            {"merged_commit_sha": "c9"},  # read-back OK
+        ]
+        env = {"GITEA_PROFILE_NAME": "gitea-merger",
+               "GITEA_ALLOWED_OPERATIONS": "read,merge"}
+        with patch.dict(os.environ, env, clear=True):
+            r = gitea_merge_pr(pr_number=8, confirmation=self._confirm(8),
+                               remote="prgs")
+        self.assertTrue(r["performed"])
+        self.assertEqual(r["merge_commit"], "c9")
+        self.assertTrue(r["cleanup_status"].startswith("skipped (cleanup error:"))
+        self.assertNotIn("secret-xyz", r["cleanup_status"])
+
     # -- confirmation ---------------------------------------------------------
 
     @patch("mcp_server.api_request")