diff --git a/bugbot/rules/not_landed.py b/bugbot/rules/not_landed.py index 834e17edc..b41f6cb1e 100644 --- a/bugbot/rules/not_landed.py +++ b/bugbot/rules/not_landed.py @@ -18,6 +18,8 @@ from bugbot.bzcleaner import BzCleaner PHAB_URL_PAT = re.compile(r"https://phabricator\.services\.mozilla\.com/D([0-9]+)") +NEEDINFO_TRACKING_PREFIX = "needinfo-revisions:" +NOT_LANDED_COMMENT_MARKER = "which didn't land and no activity in this bug for" class NotLanded(BzCleaner): @@ -27,6 +29,7 @@ def __init__(self): self.nyears = utils.get_config(self.name(), "number_of_years", 2) self.phab = PhabricatorAPI(utils.get_login_info()["phab_api_key"]) self.extra_ni = {} + self.needinfo_revision_ids: dict[str, set[int]] = {} def description(self): return "Open bugs with no activity for {} week(s) and a r+ patch which hasn't landed".format( @@ -43,6 +46,17 @@ def get_extra_for_needinfo_template(self): self.extra_ni.update(self.get_extra_for_template()) return self.extra_ni + def get_db_extra(self): + extra = dict(super().get_db_extra()) + extra.update( + { + bugid: NEEDINFO_TRACKING_PREFIX + + ",".join(str(revision_id) for revision_id in sorted(revision_ids)) + for bugid, revision_ids in self.needinfo_revision_ids.items() + } + ) + return extra + def columns(self): return ["id", "summary", "assignee"] @@ -151,6 +165,11 @@ def handle_attachment(self, attachment, res): res["phab"] = c if c is not None: + if c: + phab_url = base64.b64decode(attachment["data"]).decode("utf-8") + match = PHAB_URL_PAT.search(phab_url) + if match: + res.setdefault("revision_ids", set()).add(int(match.group(1))) attacher = attachment["creator"] if "author" in res: if attacher in res["author"]: @@ -222,6 +241,7 @@ def has_blocking_dependencies(attachment): "author": None, "count": 0, "has_blocking_dependencies": False, + "revision_ids": set(), } for bugid in bugids } @@ -265,6 +285,7 @@ def has_blocking_dependencies(attachment): data[bugid]["reviewers_phid"] = res["reviewers_phid"] data[bugid]["author"] = res["author"] data[bugid]["count"] = res["count"] + data[bugid]["revision_ids"] = res["revision_ids"] data = {bugid: v for bugid, v in data.items() if v["author"]} @@ -349,7 +370,7 @@ def get_bz_params(self, date): "n6": 1, "f6": "longdesc", "o6": "casesubstring", - "v6": "which didn't land and no activity in this bug for", + "v6": NOT_LANDED_COMMENT_MARKER, "f7": "status_whiteboard", "o7": "notsubstring", "v7": "[reminder-test ", @@ -391,14 +412,18 @@ def get_bugs(self, date="today", bug_ids=[]): if not assignee: continue - self.add_auto_ni(bugid, {"mail": assignee, "nickname": nickname}) + added_needinfo = self.add_auto_ni( + bugid, {"mail": assignee, "nickname": nickname} + ) common = all_reviewers & data["reviewers_phid"] if common: reviewer = random.choice(list(common)) - self.add_auto_ni( + added_needinfo |= self.add_auto_ni( bugid, {"mail": bz_reviewers[reviewer], "nickname": None} ) + if added_needinfo: + self.needinfo_revision_ids[bugid] = data["revision_ids"] return res diff --git a/bugbot/rules/not_landed_cleanup.py b/bugbot/rules/not_landed_cleanup.py new file mode 100644 index 000000000..6c919f5e3 --- /dev/null +++ b/bugbot/rules/not_landed_cleanup.py @@ -0,0 +1,250 @@ +# This Source Code Form is subject to the terms of the Mozilla Public +# License, v. 2.0. If a copy of the MPL was not distributed with this file, +# You can obtain one at http://mozilla.org/MPL/2.0/. + +import base64 +from typing import Any + +from libmozdata import utils as lmdutils +from libmozdata.bugzilla import Bugzilla +from libmozdata.phabricator import ( + PhabricatorAPI, + PhabricatorRevisionNotFoundException, +) + +from bugbot import db, utils +from bugbot.bzcleaner import BzCleaner +from bugbot.rules.not_landed import ( + NEEDINFO_TRACKING_PREFIX, + NOT_LANDED_COMMENT_MARKER, + PHAB_URL_PAT, +) + +NOT_LANDED_RULE = "not_landed" +CLOSED_STATUSES = {"RESOLVED", "VERIFIED", "CLOSED"} + + +class NotLandedCleanup(BzCleaner): + def __init__(self): + super().__init__() + self.phab = PhabricatorAPI(utils.get_login_info()["phab_api_key"]) + + def description(self): + return "Clear obsolete needinfos created by the not_landed rule" + + def filter_no_nag_keyword(self): + return False + + def has_last_comment_time(self): + return True + + def get_bz_params(self, date): + return { + "include_fields": ["flags", "status"], + "f1": "flagtypes.name", + "o1": "substring", + "v1": "needinfo?", + "f2": "setters.login_name", + "o2": "equals", + "v2": utils.get_config("common", "bot_bz_mail")[0], + "f3": "longdesc", + "o3": "casesubstring", + "v3": NOT_LANDED_COMMENT_MARKER, + } + + def handle_bug(self, bug, data): + data[str(bug["id"])] = { + "flags": bug["flags"], + "status": bug["status"], + } + return bug + + def commenthandler(self, bug, bugid, data): + data[str(bugid)]["comments"] = bug["comments"] + + @staticmethod + def get_not_landed_needinfos(bug: dict[str, Any]) -> list[dict[str, Any]]: + bot_accounts = utils.get_config("common", "bot_bz_mail") + comment_times = { + comment["creation_time"] + for comment in bug.get("comments", []) + if comment["creator"] in bot_accounts + and NOT_LANDED_COMMENT_MARKER in comment["text"] + } + return [ + flag + for flag in bug.get("flags", []) + if flag["name"] == "needinfo" + and flag["status"] == "?" + and flag["setter"] in bot_accounts + and flag["creation_date"] in comment_times + ] + + @staticmethod + def get_revision_tracking( + changes: list[Any], bugids: set[str] + ) -> dict[str, set[int] | None]: + tracked: dict[str, set[int] | None] = dict.fromkeys(bugids) + for change in changes: + bugid = str(change.bugid) + if bugid not in tracked: + continue + extra = change.extra.extra if change.extra else "" + if not extra.startswith(NEEDINFO_TRACKING_PREFIX): + continue + revision_ids = tracked[bugid] + if revision_ids is None: + revision_ids = tracked[bugid] = set() + revision_ids.update( + int(revision_id) + for revision_id in extra.removeprefix(NEEDINFO_TRACKING_PREFIX).split( + "," + ) + if revision_id + ) + return tracked + + def get_tracked_revision_ids(self, bugids: set[str]) -> dict[str, set[int] | None]: + changes = list(db.BugChange.get(name=NOT_LANDED_RULE)) + changes += list(db.BugChange.get(name=self.name())) + changes.sort(key=lambda change: change.id) + return self.get_revision_tracking(changes, bugids) + + def get_landed_bug_ids(self, revision_ids_by_bug: dict[str, set[int]]) -> set[str]: + landed = set() + for bugid, revision_ids in revision_ids_by_bug.items(): + if not revision_ids: + continue + all_published = True + for revision_id in revision_ids: + try: + revision = self.phab.load_revision(rev_id=revision_id) + except PhabricatorRevisionNotFoundException: + all_published = False + break + if revision["fields"]["status"].get("value") != "published": + all_published = False + break + if all_published: + landed.add(bugid) + return landed + + def get_phab_attachments( + self, bugids: list[str] + ) -> dict[str, list[dict[str, Any]]]: + attachment_ids: list[int] = [] + + def attachment_id_handler(attachments, bugid, data): + for attachment in attachments: + if ( + attachment["content_type"] == "text/x-phabricator-request" + and attachment["is_obsolete"] == 0 + ): + data.append(attachment["id"]) + + Bugzilla( + bugids=bugids, + attachmenthandler=attachment_id_handler, + attachmentdata=attachment_ids, + attachment_include_fields=["is_obsolete", "content_type", "id"], + ).get_data().wait() + + attachments_by_bug: dict[str, list[dict[str, Any]]] = {} + + def attachment_handler(attachments, data): + for attachment in attachments: + data.setdefault(str(attachment["bug_id"]), []).append(attachment) + + if attachment_ids: + Bugzilla( + attachmentids=attachment_ids, + attachmenthandler=attachment_handler, + attachmentdata=attachments_by_bug, + attachment_include_fields=["bug_id", "creation_time", "data"], + ).get_data().wait() + + return attachments_by_bug + + def get_legacy_revision_ids( + self, needinfos_by_bug: dict[str, list[dict[str, Any]]] + ) -> dict[str, set[int]]: + attachments_by_bug = self.get_phab_attachments(list(needinfos_by_bug)) + revisions_by_bug: dict[str, set[int]] = {} + for bugid, needinfos in needinfos_by_bug.items(): + requested_at = min( + lmdutils.get_timestamp(flag["creation_date"]) for flag in needinfos + ) + for attachment in attachments_by_bug.get(bugid, []): + if lmdutils.get_timestamp(attachment["creation_time"]) > requested_at: + continue + phab_url = base64.b64decode(attachment["data"]).decode("utf-8") + match = PHAB_URL_PAT.search(phab_url) + if match: + revisions_by_bug.setdefault(bugid, set()).add(int(match.group(1))) + return revisions_by_bug + + def record_revision_ids(self, revisions_by_bug: dict[str, set[int]]) -> None: + if getattr(self, "dryrun", True) or self.test_mode: + return + for bugid, revision_ids in revisions_by_bug.items(): + extra = NEEDINFO_TRACKING_PREFIX + ",".join( + str(revision_id) for revision_id in sorted(revision_ids) + ) + db.BugChange.add(self.name(), bugid, extra=extra) + + def get_bugs(self, date="today", bug_ids=[]): + bugs = super().get_bugs(date=date, bug_ids=bug_ids) + needinfos_by_bug = { + bugid: needinfos + for bugid, bug in bugs.items() + if (needinfos := self.get_not_landed_needinfos(bug)) + } + revision_ids_by_bug = self.get_tracked_revision_ids(set(needinfos_by_bug)) + + legacy_needinfos = { + bugid: needinfos + for bugid, needinfos in needinfos_by_bug.items() + if revision_ids_by_bug[bugid] is None + } + recovered_revision_ids = self.get_legacy_revision_ids(legacy_needinfos) + legacy_revision_ids = { + bugid: recovered_revision_ids.get(bugid, set()) + for bugid in legacy_needinfos + } + revision_ids_by_bug.update(legacy_revision_ids) + self.record_revision_ids(legacy_revision_ids) + + clear_bugids = { + bugid + for bugid in needinfos_by_bug + if bugs[bugid]["status"] in CLOSED_STATUSES + } + open_revisions = {} + for bugid in needinfos_by_bug: + if bugid in clear_bugids: + continue + revision_ids = revision_ids_by_bug[bugid] + assert revision_ids is not None + open_revisions[bugid] = revision_ids + clear_bugids |= self.get_landed_bug_ids(open_revisions) + clear_bugids = set(sorted(clear_bugids, key=int)[: self.normal_changes_max]) + + self.autofix_changes = { + bugid: { + "flags": [ + {"id": flag["id"], "status": "X"} + for flag in needinfos_by_bug[bugid] + ] + } + for bugid in clear_bugids + } + return {bugid: bugs[bugid] for bugid in clear_bugids} + + def get_email_data(self, date): + # Run the autofix pipeline without sending a summary email. + super().get_email_data(date) + return [] + + +if __name__ == "__main__": + NotLandedCleanup().run() diff --git a/scripts/cron_run_weekdays.sh b/scripts/cron_run_weekdays.sh index 2d21fc6e9..43e2e1ed5 100755 --- a/scripts/cron_run_weekdays.sh +++ b/scripts/cron_run_weekdays.sh @@ -58,6 +58,9 @@ python -m bugbot.rules.multi_nag --production # Pretty common python -m bugbot.rules.not_landed --production +# Clear not_landed needinfos after the tracked patches land or the bug closes +python -m bugbot.rules.not_landed_cleanup --production + # New workflow # https://docs.google.com/document/d/1EHuWa-uR-7Sq63X1ZiDN1mvJ9gQtWiqYrCifkySJyW0/edit# # https://docs.google.com/drawings/d/1oZA-AUvkOxGMNhZNofL8Wlfk6ol3o5ATQCV5DJJKbwM/edit diff --git a/templates/not_landed_cleanup.html b/templates/not_landed_cleanup.html new file mode 100644 index 000000000..66401ba14 --- /dev/null +++ b/templates/not_landed_cleanup.html @@ -0,0 +1,15 @@ +

Needinfos created by the not_landed rule that are no longer actionable.

+ + + + + + {% for bugid, summary in data %} + + + + + {% endfor %} +
BugSummary
+ {{ bugid }} + {{ summary | e }}
diff --git a/tests/rules/test_not_landed.py b/tests/rules/test_not_landed.py new file mode 100644 index 000000000..a509065da --- /dev/null +++ b/tests/rules/test_not_landed.py @@ -0,0 +1,37 @@ +# This Source Code Form is subject to the terms of the Mozilla Public +# License, v. 2.0. If a copy of the MPL was not distributed with this file, +# You can obtain one at http://mozilla.org/MPL/2.0/. + +import base64 + +from bugbot import utils +from bugbot.rules.not_landed import NEEDINFO_TRACKING_PREFIX, NotLanded + + +def _rule(monkeypatch): + monkeypatch.setattr(utils, "get_login_info", lambda: {"phab_api_key": "test-key"}) + return NotLanded() + + +def test_new_needinfo_tracks_exact_revision_ids(monkeypatch): + rule = _rule(monkeypatch) + rule.needinfo_revision_ids = {"123": {124, 123}} + + assert rule.get_db_extra()["123"] == f"{NEEDINFO_TRACKING_PREFIX}123,124" + + +def test_unlanded_attachment_records_revision_id(monkeypatch): + rule = _rule(monkeypatch) + monkeypatch.setattr(rule, "check_phab", lambda attachment, reviewers: True) + result = {"reviewers_phid": set()} + attachment = { + "content_type": "text/x-phabricator-request", + "creator": "author@example.com", + "data": base64.b64encode( + b"https://phabricator.services.mozilla.com/D123" + ).decode(), + } + + rule.handle_attachment(attachment, result) + + assert result["revision_ids"] == {123} diff --git a/tests/rules/test_not_landed_cleanup.py b/tests/rules/test_not_landed_cleanup.py new file mode 100644 index 000000000..1065c6917 --- /dev/null +++ b/tests/rules/test_not_landed_cleanup.py @@ -0,0 +1,258 @@ +# This Source Code Form is subject to the terms of the Mozilla Public +# License, v. 2.0. If a copy of the MPL was not distributed with this file, +# You can obtain one at http://mozilla.org/MPL/2.0/. + +import base64 +from types import SimpleNamespace + +from jinja2 import Environment, FileSystemLoader + +from bugbot import db, utils +from bugbot.bzcleaner import BzCleaner +from bugbot.rules.not_landed import ( + NEEDINFO_TRACKING_PREFIX, + NOT_LANDED_COMMENT_MARKER, +) +from bugbot.rules.not_landed_cleanup import NotLandedCleanup + +BOT = "release-mgmt-account-bot@mozilla.tld" +REQUEST_TIME = "2026-08-14T12:10:35Z" + + +def _change(bugid, extra): + return SimpleNamespace( + bugid=bugid, + extra=SimpleNamespace(extra=extra) if extra else None, + ) + + +def _flag(flag_id, setter=BOT, creation_date=REQUEST_TIME): + return { + "id": flag_id, + "name": "needinfo", + "status": "?", + "setter": setter, + "requestee": f"user-{flag_id}@example.com", + "creation_date": creation_date, + } + + +def _not_landed_comment( + text="There is an r+ patch which didn't land and no activity in this bug for 1 week.", +): + return { + "creator": BOT, + "creation_time": REQUEST_TIME, + "text": text, + } + + +def _bug(bugid, status="NEW", flags=None): + return { + "id": int(bugid), + "summary": f"Bug {bugid}", + "status": status, + "comments": [_not_landed_comment()], + "flags": flags if flags is not None else [_flag(int(bugid))], + } + + +def _rule(monkeypatch): + monkeypatch.setattr(utils, "get_login_info", lambda: {"phab_api_key": "test-key"}) + rule = NotLandedCleanup() + rule.dryrun = True + return rule + + +def _set_bugs(monkeypatch, bugs): + monkeypatch.setattr( + BzCleaner, + "get_bugs", + lambda self, date="today", bug_ids=[]: bugs, + ) + + +def test_query_finds_current_not_landed_needinfos(monkeypatch): + rule = _rule(monkeypatch) + + params = rule.get_bz_params("today") + + assert params["v1"] == "needinfo?" + assert params["v2"] == BOT + assert params["v3"] == NOT_LANDED_COMMENT_MARKER + assert {"flags", "status"} <= set(params["include_fields"]) + + +def test_revision_tracking_distinguishes_legacy_and_empty_results(): + changes = [ + _change(1, "first@example.com"), + _change(2, f"{NEEDINFO_TRACKING_PREFIX}20,21"), + _change(2, f"{NEEDINFO_TRACKING_PREFIX}22"), + _change(3, NEEDINFO_TRACKING_PREFIX), + ] + + assert NotLandedCleanup.get_revision_tracking(changes, {"1", "2", "3"}) == { + "1": None, + "2": {20, 21, 22}, + "3": set(), + } + + +def test_not_landed_needinfos_exclude_unrelated_flags(): + owned = _flag(1) + other_rule = _flag(2, creation_date="2026-08-15T12:10:35Z") + human = _flag(3, setter="human@example.com") + bug = { + "comments": [ + _not_landed_comment(), + { + "creator": BOT, + "creation_time": other_rule["creation_date"], + "text": "A different BugBot rule created this needinfo.", + }, + ], + "flags": [owned, other_rule, human], + } + + assert NotLandedCleanup.get_not_landed_needinfos(bug) == [owned] + + +def test_historical_not_landed_comment_is_recognized(): + owned = _flag(1) + bug = { + "comments": [ + _not_landed_comment( + "There's a r+ patch which didn't land and no activity in this bug for 1 week." + ) + ], + "flags": [owned], + } + + assert NotLandedCleanup.get_not_landed_needinfos(bug) == [owned] + + +def test_resolved_bug_clears_only_owned_flags(monkeypatch): + rule = _rule(monkeypatch) + owned = _flag(1) + unrelated = _flag(2, creation_date="2026-08-15T12:10:35Z") + bugs = {"123": _bug("123", status="RESOLVED", flags=[owned, unrelated])} + _set_bugs(monkeypatch, bugs) + monkeypatch.setattr(rule, "get_tracked_revision_ids", lambda bugids: {"123": {123}}) + monkeypatch.setattr(rule, "get_landed_bug_ids", lambda revisions: set()) + + assert rule.get_bugs() == bugs + assert rule.autofix_changes == { + "123": {"flags": [{"id": owned["id"], "status": "X"}]} + } + + +def test_open_bug_with_landed_patch_is_cleared(monkeypatch): + rule = _rule(monkeypatch) + bugs = {"123": _bug("123")} + _set_bugs(monkeypatch, bugs) + monkeypatch.setattr(rule, "get_tracked_revision_ids", lambda bugids: {"123": {123}}) + rule.phab = SimpleNamespace( + load_revision=lambda rev_id: {"fields": {"status": {"value": "published"}}} + ) + + rule.get_bugs() + + assert rule.autofix_changes == {"123": {"flags": [{"id": 123, "status": "X"}]}} + + +def test_all_relevant_patches_must_land(monkeypatch): + rule = _rule(monkeypatch) + rule.phab = SimpleNamespace( + load_revision=lambda rev_id: { + "fields": { + "status": {"value": "published" if rev_id == 123 else "accepted"}, + } + } + ) + + assert rule.get_landed_bug_ids({"123": {123, 124}}) == set() + + +def test_cleanup_is_capped_to_framework_limit(monkeypatch): + rule = _rule(monkeypatch) + bugs = {str(bugid): _bug(str(bugid), status="RESOLVED") for bugid in range(1, 52)} + _set_bugs(monkeypatch, bugs) + monkeypatch.setattr( + rule, + "get_tracked_revision_ids", + lambda bugids: {bugid: {int(bugid)} for bugid in bugids}, + ) + monkeypatch.setattr(rule, "get_landed_bug_ids", lambda revisions: set()) + + rule.get_bugs() + + assert len(rule.autofix_changes) == rule.normal_changes_max + assert "50" in rule.autofix_changes + assert "51" not in rule.autofix_changes + + +def test_legacy_tracking_ignores_patches_attached_after_needinfo(monkeypatch): + rule = _rule(monkeypatch) + before_needinfo = { + "creation_time": "2026-08-13T12:10:35Z", + "data": base64.b64encode( + b"https://phabricator.services.mozilla.com/D123" + ).decode(), + } + after_needinfo = { + "creation_time": "2026-08-15T12:10:35Z", + "data": base64.b64encode( + b"https://phabricator.services.mozilla.com/D124" + ).decode(), + } + monkeypatch.setattr( + rule, + "get_phab_attachments", + lambda bugids: {"123": [before_needinfo, after_needinfo]}, + ) + + assert rule.get_legacy_revision_ids({"123": [_flag(1)]}) == {"123": {123}} + + +def test_empty_legacy_result_is_recorded(monkeypatch): + rule = _rule(monkeypatch) + recorded = [] + rule.dryrun = False + rule.test_mode = False + monkeypatch.setattr( + db.BugChange, + "add", + lambda name, bugid, extra: recorded.append((name, bugid, extra)), + ) + + rule.record_revision_ids({"123": set()}) + + assert recorded == [("not_landed_cleanup", "123", NEEDINFO_TRACKING_PREFIX)] + + +def test_test_mode_does_not_record_legacy_results(monkeypatch): + rule = _rule(monkeypatch) + rule.dryrun = False + rule.test_mode = True + monkeypatch.setattr( + db.BugChange, + "add", + lambda name, bugid, extra: raise_error(), + ) + + rule.record_revision_ids({"123": {123}}) + + +def test_abort_template_escapes_summary(): + env = Environment(loader=FileSystemLoader("templates")) + rendered = env.get_template("not_landed_cleanup.html").render( + data=[("123", "")], + table_attrs="", + ) + + assert "<private>" in rendered + assert "" not in rendered + + +def raise_error(): + raise AssertionError("DB write should not happen")