From 87561a0f622880983f160bade00ea272a3ff4199 Mon Sep 17 00:00:00 2001 From: Logan Rosen Date: Sat, 29 Aug 2026 15:36:38 -0400 Subject: [PATCH 1/3] Clear not_landed needinfos after landing Track the exact Phabricator revisions behind each not_landed needinfo and clear only the matching BugBot-created flags after those revisions publish or the bug closes. Retire completed tracking records and cover landing, resolution, legacy, and unrelated-flag safety cases. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- bugbot/rules/not_landed.py | 246 ++++++++++++++++++++++++++++++++- tests/rules/test_not_landed.py | 246 +++++++++++++++++++++++++++++++++ 2 files changed, 489 insertions(+), 3 deletions(-) create mode 100644 tests/rules/test_not_landed.py diff --git a/bugbot/rules/not_landed.py b/bugbot/rules/not_landed.py index 834e17edc..eb046fc68 100644 --- a/bugbot/rules/not_landed.py +++ b/bugbot/rules/not_landed.py @@ -5,6 +5,7 @@ import base64 import random import re +from typing import Any from libmozdata import utils as lmdutils from libmozdata.bugzilla import Bugzilla, BugzillaUser @@ -14,10 +15,17 @@ PhabricatorRevisionNotFoundException, ) -from bugbot import utils +from bugbot import db, utils from bugbot.bzcleaner import BzCleaner PHAB_URL_PAT = re.compile(r"https://phabricator\.services\.mozilla\.com/D([0-9]+)") +NOT_LANDED_COMMENT_PREFIXES = ( + "There is an r+ patch which didn't land and no activity in this bug for", + "There are some r+ patches which didn't land and no activity in this bug for", +) +NEEDINFO_CLEANUP_MARKER = "needinfo-cleanup" +NEEDINFO_TRACKING_PREFIX = "needinfo-revisions:" +CLOSED_STATUSES = {"RESOLVED", "VERIFIED", "CLOSED"} class NotLanded(BzCleaner): @@ -27,6 +35,8 @@ 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_cleanup_bugids: set[str] = set() + 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 +53,224 @@ def get_extra_for_needinfo_template(self): self.extra_ni.update(self.get_extra_for_template()) return self.extra_ni + @staticmethod + def get_pending_needinfos(changes: list[Any]) -> dict[str, set[int]]: + pending: dict[str, set[int]] = {} + for change in changes: + extra = change.extra.extra if change.extra else "" + bugid = str(change.bugid) + if extra == NEEDINFO_CLEANUP_MARKER: + pending.pop(bugid, None) + elif extra.startswith(NEEDINFO_TRACKING_PREFIX): + revision_ids = extra.removeprefix(NEEDINFO_TRACKING_PREFIX) + pending.setdefault(bugid, set()).update( + int(revision_id) + for revision_id in revision_ids.split(",") + if revision_id + ) + elif extra: + pending.setdefault(bugid, set()) + return pending + + def get_pending_needinfo_tracking(self) -> dict[str, set[int]]: + changes = list( + db.BugChange.get(name=self.name()).order_by(db.BugChange.id.asc()) + ) + return self.get_pending_needinfos(changes) + + def get_needinfo_cleanup_bugs(self, bugids: list[str]) -> dict[str, dict[str, Any]]: + if not bugids: + return {} + + data: dict[str, dict[str, Any]] = {} + + def bug_handler(bug, data): + data.setdefault(str(bug["id"]), {}).update(bug) + + def comment_handler(bug, bugid, data): + data.setdefault(str(bugid), {})["comments"] = bug["comments"] + + Bugzilla( + bugids=bugids, + include_fields=["id", "status", "flags"], + bughandler=bug_handler, + bugdata=data, + commenthandler=comment_handler, + commentdata=data, + comment_include_fields=["creator", "creation_time", "text"], + ).get_data().wait() + + return { + bugid: bug + for bugid, bug in data.items() + if "comments" in bug and "flags" in bug + } + + @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 comment["text"].startswith(NOT_LANDED_COMMENT_PREFIXES) + } + 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 + ] + + 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 mark_needinfo_revisions(self, revisions_by_bug: dict[str, set[int]]) -> None: + if getattr(self, "dryrun", True): + 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 mark_needinfo_tracking_complete(self, bugids: set[str]) -> None: + if getattr(self, "dryrun", True): + return + for bugid in bugids: + db.BugChange.add(self.name(), bugid, extra=NEEDINFO_CLEANUP_MARKER) + + def schedule_needinfo_cleanup(self) -> None: + revision_ids_by_bug = self.get_pending_needinfo_tracking() + bugs = self.get_needinfo_cleanup_bugs(list(revision_ids_by_bug)) + unavailable_bugids = set(revision_ids_by_bug) - set(bugs) + needinfos_by_bug = { + bugid: needinfos + for bugid, bug in bugs.items() + if (needinfos := self.get_not_landed_needinfos(bug)) + } + self.mark_needinfo_tracking_complete( + unavailable_bugids | (set(bugs) - set(needinfos_by_bug)) + ) + legacy_needinfos = { + bugid: needinfos + for bugid, needinfos in needinfos_by_bug.items() + if not revision_ids_by_bug[bugid] + } + legacy_revision_ids = self.get_legacy_revision_ids(legacy_needinfos) + revision_ids_by_bug.update(legacy_revision_ids) + self.mark_needinfo_revisions(legacy_revision_ids) + clear_bugids = { + bugid + for bugid in needinfos_by_bug + if bugs[bugid]["status"] in CLOSED_STATUSES + } + open_revisions = { + bugid: revision_ids_by_bug[bugid] + for bugid in needinfos_by_bug + if bugid not in clear_bugids + } + clear_bugids |= self.get_landed_bug_ids(open_revisions) + + self.needinfo_cleanup_bugids = clear_bugids + self.autofix_changes.update( + { + bugid: { + "flags": [ + {"id": flag["id"], "status": "X"} + for flag in needinfos_by_bug[bugid] + ] + } + for bugid in clear_bugids + } + ) + + 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() + } + ) + extra.update( + {bugid: NEEDINFO_CLEANUP_MARKER for bugid in self.needinfo_cleanup_bugids} + ) + return extra + def columns(self): return ["id", "summary", "assignee"] @@ -151,6 +379,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 +455,7 @@ def has_blocking_dependencies(attachment): "author": None, "count": 0, "has_blocking_dependencies": False, + "revision_ids": set(), } for bugid in bugids } @@ -265,6 +499,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"]} @@ -361,6 +596,7 @@ def get_bz_params(self, date): return params def get_bugs(self, date="today", bug_ids=[]): + self.schedule_needinfo_cleanup() bugs = super(NotLanded, self).get_bugs(date=date, bug_ids=bug_ids) bugs = self.filter_bugs(bugs) bugs_patch = self.get_patch_data(bugs) @@ -391,14 +627,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/tests/rules/test_not_landed.py b/tests/rules/test_not_landed.py new file mode 100644 index 000000000..2bcd7767f --- /dev/null +++ b/tests/rules/test_not_landed.py @@ -0,0 +1,246 @@ +# 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 bugbot import utils +from bugbot.rules.not_landed import ( + NEEDINFO_CLEANUP_MARKER, + NEEDINFO_TRACKING_PREFIX, + NotLanded, +) + +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(): + return { + "creator": BOT, + "creation_time": REQUEST_TIME, + "text": "There is an r+ patch which didn't land and no activity in this bug for 1 week.", + } + + +def _rule(monkeypatch): + monkeypatch.setattr(utils, "get_login_info", lambda: {"phab_api_key": "test-key"}) + rule = NotLanded() + rule.dryrun = True + return rule + + +def test_pending_needinfos_follow_cleanup_markers(): + changes = [ + _change(1, "first@example.com"), + _change(2, f"{NEEDINFO_TRACKING_PREFIX}20,21"), + _change(1, NEEDINFO_CLEANUP_MARKER), + _change(1, f"{NEEDINFO_TRACKING_PREFIX}10"), + _change(2, NEEDINFO_CLEANUP_MARKER), + _change(3, ""), + ] + + assert NotLanded.get_pending_needinfos(changes) == {"1": {10}} + + +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 NotLanded.get_not_landed_needinfos(bug) == [owned] + + +def test_schedule_cleanup_for_resolved_bug_clears_only_owned_flags(monkeypatch): + rule = _rule(monkeypatch) + owned = _flag(1) + unrelated = _flag(2, creation_date="2026-08-15T12:10:35Z") + monkeypatch.setattr( + rule, + "get_needinfo_cleanup_bugs", + lambda bugids: { + "123": { + "status": "RESOLVED", + "comments": [_not_landed_comment()], + "flags": [owned, unrelated], + } + }, + ) + monkeypatch.setattr(rule, "get_pending_needinfo_tracking", lambda: {"123": {123}}) + monkeypatch.setattr(rule, "get_landed_bug_ids", lambda bugs: set()) + + rule.schedule_needinfo_cleanup() + + assert rule.autofix_changes == { + "123": {"flags": [{"id": owned["id"], "status": "X"}]} + } + assert rule.get_db_extra()["123"] == NEEDINFO_CLEANUP_MARKER + + +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} + + +def test_landed_patch_after_needinfo_is_cleaned_up(monkeypatch): + rule = _rule(monkeypatch) + rule.phab = SimpleNamespace( + load_revision=lambda rev_id: { + "fields": { + "status": {"value": "published"}, + } + } + ) + + assert rule.get_landed_bug_ids({"123": {123, 124}}) == {"123"} + + +def test_schedule_cleanup_for_landed_patch(monkeypatch): + rule = _rule(monkeypatch) + owned = _flag(1) + monkeypatch.setattr(rule, "get_pending_needinfo_tracking", lambda: {"123": {123}}) + monkeypatch.setattr( + rule, + "get_needinfo_cleanup_bugs", + lambda bugids: { + "123": { + "status": "NEW", + "comments": [_not_landed_comment()], + "flags": [owned], + } + }, + ) + rule.phab = SimpleNamespace( + load_revision=lambda rev_id: {"fields": {"status": {"value": "published"}}} + ) + + rule.schedule_needinfo_cleanup() + + assert rule.autofix_changes == { + "123": {"flags": [{"id": owned["id"], "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_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_manually_cleared_needinfo_retires_tracking(monkeypatch): + rule = _rule(monkeypatch) + retired = set() + monkeypatch.setattr(rule, "get_pending_needinfo_tracking", lambda: {"123": {123}}) + monkeypatch.setattr( + rule, + "get_needinfo_cleanup_bugs", + lambda bugids: { + "123": { + "status": "NEW", + "comments": [_not_landed_comment()], + "flags": [], + } + }, + ) + monkeypatch.setattr( + rule, "mark_needinfo_tracking_complete", lambda bugids: retired.update(bugids) + ) + monkeypatch.setattr(rule, "get_landed_bug_ids", lambda bugs: set()) + + rule.schedule_needinfo_cleanup() + + assert retired == {"123"} + + +def test_unavailable_bug_retires_tracking(monkeypatch): + rule = _rule(monkeypatch) + retired = set() + monkeypatch.setattr(rule, "get_pending_needinfo_tracking", lambda: {"123": {123}}) + monkeypatch.setattr(rule, "get_needinfo_cleanup_bugs", lambda bugids: {}) + monkeypatch.setattr( + rule, "mark_needinfo_tracking_complete", lambda bugids: retired.update(bugids) + ) + monkeypatch.setattr(rule, "get_landed_bug_ids", lambda bugs: set()) + + rule.schedule_needinfo_cleanup() + + assert retired == {"123"} From 2e68427d8ecbdf9ae63fd258a5b804deff4e2e40 Mon Sep 17 00:00:00 2001 From: Logan Rosen Date: Sun, 30 Aug 2026 16:50:57 -0400 Subject: [PATCH 2/3] Move not-landed cleanup to separate rule Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- bugbot/rules/not_landed.py | 221 +------------------- bugbot/rules/not_landed_cleanup.py | 254 +++++++++++++++++++++++ scripts/cron_run_weekdays.sh | 3 + templates/not_landed_cleanup.html | 15 ++ tests/rules/test_not_landed.py | 213 +------------------- tests/rules/test_not_landed_cleanup.py | 269 +++++++++++++++++++++++++ 6 files changed, 546 insertions(+), 429 deletions(-) create mode 100644 bugbot/rules/not_landed_cleanup.py create mode 100644 templates/not_landed_cleanup.html create mode 100644 tests/rules/test_not_landed_cleanup.py diff --git a/bugbot/rules/not_landed.py b/bugbot/rules/not_landed.py index eb046fc68..b41f6cb1e 100644 --- a/bugbot/rules/not_landed.py +++ b/bugbot/rules/not_landed.py @@ -5,7 +5,6 @@ import base64 import random import re -from typing import Any from libmozdata import utils as lmdutils from libmozdata.bugzilla import Bugzilla, BugzillaUser @@ -15,17 +14,12 @@ PhabricatorRevisionNotFoundException, ) -from bugbot import db, utils +from bugbot import utils from bugbot.bzcleaner import BzCleaner PHAB_URL_PAT = re.compile(r"https://phabricator\.services\.mozilla\.com/D([0-9]+)") -NOT_LANDED_COMMENT_PREFIXES = ( - "There is an r+ patch which didn't land and no activity in this bug for", - "There are some r+ patches which didn't land and no activity in this bug for", -) -NEEDINFO_CLEANUP_MARKER = "needinfo-cleanup" NEEDINFO_TRACKING_PREFIX = "needinfo-revisions:" -CLOSED_STATUSES = {"RESOLVED", "VERIFIED", "CLOSED"} +NOT_LANDED_COMMENT_MARKER = "which didn't land and no activity in this bug for" class NotLanded(BzCleaner): @@ -35,7 +29,6 @@ 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_cleanup_bugids: set[str] = set() self.needinfo_revision_ids: dict[str, set[int]] = {} def description(self): @@ -53,210 +46,6 @@ def get_extra_for_needinfo_template(self): self.extra_ni.update(self.get_extra_for_template()) return self.extra_ni - @staticmethod - def get_pending_needinfos(changes: list[Any]) -> dict[str, set[int]]: - pending: dict[str, set[int]] = {} - for change in changes: - extra = change.extra.extra if change.extra else "" - bugid = str(change.bugid) - if extra == NEEDINFO_CLEANUP_MARKER: - pending.pop(bugid, None) - elif extra.startswith(NEEDINFO_TRACKING_PREFIX): - revision_ids = extra.removeprefix(NEEDINFO_TRACKING_PREFIX) - pending.setdefault(bugid, set()).update( - int(revision_id) - for revision_id in revision_ids.split(",") - if revision_id - ) - elif extra: - pending.setdefault(bugid, set()) - return pending - - def get_pending_needinfo_tracking(self) -> dict[str, set[int]]: - changes = list( - db.BugChange.get(name=self.name()).order_by(db.BugChange.id.asc()) - ) - return self.get_pending_needinfos(changes) - - def get_needinfo_cleanup_bugs(self, bugids: list[str]) -> dict[str, dict[str, Any]]: - if not bugids: - return {} - - data: dict[str, dict[str, Any]] = {} - - def bug_handler(bug, data): - data.setdefault(str(bug["id"]), {}).update(bug) - - def comment_handler(bug, bugid, data): - data.setdefault(str(bugid), {})["comments"] = bug["comments"] - - Bugzilla( - bugids=bugids, - include_fields=["id", "status", "flags"], - bughandler=bug_handler, - bugdata=data, - commenthandler=comment_handler, - commentdata=data, - comment_include_fields=["creator", "creation_time", "text"], - ).get_data().wait() - - return { - bugid: bug - for bugid, bug in data.items() - if "comments" in bug and "flags" in bug - } - - @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 comment["text"].startswith(NOT_LANDED_COMMENT_PREFIXES) - } - 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 - ] - - 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 mark_needinfo_revisions(self, revisions_by_bug: dict[str, set[int]]) -> None: - if getattr(self, "dryrun", True): - 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 mark_needinfo_tracking_complete(self, bugids: set[str]) -> None: - if getattr(self, "dryrun", True): - return - for bugid in bugids: - db.BugChange.add(self.name(), bugid, extra=NEEDINFO_CLEANUP_MARKER) - - def schedule_needinfo_cleanup(self) -> None: - revision_ids_by_bug = self.get_pending_needinfo_tracking() - bugs = self.get_needinfo_cleanup_bugs(list(revision_ids_by_bug)) - unavailable_bugids = set(revision_ids_by_bug) - set(bugs) - needinfos_by_bug = { - bugid: needinfos - for bugid, bug in bugs.items() - if (needinfos := self.get_not_landed_needinfos(bug)) - } - self.mark_needinfo_tracking_complete( - unavailable_bugids | (set(bugs) - set(needinfos_by_bug)) - ) - legacy_needinfos = { - bugid: needinfos - for bugid, needinfos in needinfos_by_bug.items() - if not revision_ids_by_bug[bugid] - } - legacy_revision_ids = self.get_legacy_revision_ids(legacy_needinfos) - revision_ids_by_bug.update(legacy_revision_ids) - self.mark_needinfo_revisions(legacy_revision_ids) - clear_bugids = { - bugid - for bugid in needinfos_by_bug - if bugs[bugid]["status"] in CLOSED_STATUSES - } - open_revisions = { - bugid: revision_ids_by_bug[bugid] - for bugid in needinfos_by_bug - if bugid not in clear_bugids - } - clear_bugids |= self.get_landed_bug_ids(open_revisions) - - self.needinfo_cleanup_bugids = clear_bugids - self.autofix_changes.update( - { - bugid: { - "flags": [ - {"id": flag["id"], "status": "X"} - for flag in needinfos_by_bug[bugid] - ] - } - for bugid in clear_bugids - } - ) - def get_db_extra(self): extra = dict(super().get_db_extra()) extra.update( @@ -266,9 +55,6 @@ def get_db_extra(self): for bugid, revision_ids in self.needinfo_revision_ids.items() } ) - extra.update( - {bugid: NEEDINFO_CLEANUP_MARKER for bugid in self.needinfo_cleanup_bugids} - ) return extra def columns(self): @@ -584,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 ", @@ -596,7 +382,6 @@ def get_bz_params(self, date): return params def get_bugs(self, date="today", bug_ids=[]): - self.schedule_needinfo_cleanup() bugs = super(NotLanded, self).get_bugs(date=date, bug_ids=bug_ids) bugs = self.filter_bugs(bugs) bugs_patch = self.get_patch_data(bugs) diff --git a/bugbot/rules/not_landed_cleanup.py b/bugbot/rules/not_landed_cleanup.py new file mode 100644 index 000000000..b04a7dbb8 --- /dev/null +++ b/bugbot/rules/not_landed_cleanup.py @@ -0,0 +1,254 @@ +# 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 index 2bcd7767f..a509065da 100644 --- a/tests/rules/test_not_landed.py +++ b/tests/rules/test_not_landed.py @@ -3,108 +3,14 @@ # You can obtain one at http://mozilla.org/MPL/2.0/. import base64 -from types import SimpleNamespace from bugbot import utils -from bugbot.rules.not_landed import ( - NEEDINFO_CLEANUP_MARKER, - NEEDINFO_TRACKING_PREFIX, - NotLanded, -) - -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(): - return { - "creator": BOT, - "creation_time": REQUEST_TIME, - "text": "There is an r+ patch which didn't land and no activity in this bug for 1 week.", - } +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"}) - rule = NotLanded() - rule.dryrun = True - return rule - - -def test_pending_needinfos_follow_cleanup_markers(): - changes = [ - _change(1, "first@example.com"), - _change(2, f"{NEEDINFO_TRACKING_PREFIX}20,21"), - _change(1, NEEDINFO_CLEANUP_MARKER), - _change(1, f"{NEEDINFO_TRACKING_PREFIX}10"), - _change(2, NEEDINFO_CLEANUP_MARKER), - _change(3, ""), - ] - - assert NotLanded.get_pending_needinfos(changes) == {"1": {10}} - - -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 NotLanded.get_not_landed_needinfos(bug) == [owned] - - -def test_schedule_cleanup_for_resolved_bug_clears_only_owned_flags(monkeypatch): - rule = _rule(monkeypatch) - owned = _flag(1) - unrelated = _flag(2, creation_date="2026-08-15T12:10:35Z") - monkeypatch.setattr( - rule, - "get_needinfo_cleanup_bugs", - lambda bugids: { - "123": { - "status": "RESOLVED", - "comments": [_not_landed_comment()], - "flags": [owned, unrelated], - } - }, - ) - monkeypatch.setattr(rule, "get_pending_needinfo_tracking", lambda: {"123": {123}}) - monkeypatch.setattr(rule, "get_landed_bug_ids", lambda bugs: set()) - - rule.schedule_needinfo_cleanup() - - assert rule.autofix_changes == { - "123": {"flags": [{"id": owned["id"], "status": "X"}]} - } - assert rule.get_db_extra()["123"] == NEEDINFO_CLEANUP_MARKER + return NotLanded() def test_new_needinfo_tracks_exact_revision_ids(monkeypatch): @@ -129,118 +35,3 @@ def test_unlanded_attachment_records_revision_id(monkeypatch): rule.handle_attachment(attachment, result) assert result["revision_ids"] == {123} - - -def test_landed_patch_after_needinfo_is_cleaned_up(monkeypatch): - rule = _rule(monkeypatch) - rule.phab = SimpleNamespace( - load_revision=lambda rev_id: { - "fields": { - "status": {"value": "published"}, - } - } - ) - - assert rule.get_landed_bug_ids({"123": {123, 124}}) == {"123"} - - -def test_schedule_cleanup_for_landed_patch(monkeypatch): - rule = _rule(monkeypatch) - owned = _flag(1) - monkeypatch.setattr(rule, "get_pending_needinfo_tracking", lambda: {"123": {123}}) - monkeypatch.setattr( - rule, - "get_needinfo_cleanup_bugs", - lambda bugids: { - "123": { - "status": "NEW", - "comments": [_not_landed_comment()], - "flags": [owned], - } - }, - ) - rule.phab = SimpleNamespace( - load_revision=lambda rev_id: {"fields": {"status": {"value": "published"}}} - ) - - rule.schedule_needinfo_cleanup() - - assert rule.autofix_changes == { - "123": {"flags": [{"id": owned["id"], "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_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_manually_cleared_needinfo_retires_tracking(monkeypatch): - rule = _rule(monkeypatch) - retired = set() - monkeypatch.setattr(rule, "get_pending_needinfo_tracking", lambda: {"123": {123}}) - monkeypatch.setattr( - rule, - "get_needinfo_cleanup_bugs", - lambda bugids: { - "123": { - "status": "NEW", - "comments": [_not_landed_comment()], - "flags": [], - } - }, - ) - monkeypatch.setattr( - rule, "mark_needinfo_tracking_complete", lambda bugids: retired.update(bugids) - ) - monkeypatch.setattr(rule, "get_landed_bug_ids", lambda bugs: set()) - - rule.schedule_needinfo_cleanup() - - assert retired == {"123"} - - -def test_unavailable_bug_retires_tracking(monkeypatch): - rule = _rule(monkeypatch) - retired = set() - monkeypatch.setattr(rule, "get_pending_needinfo_tracking", lambda: {"123": {123}}) - monkeypatch.setattr(rule, "get_needinfo_cleanup_bugs", lambda bugids: {}) - monkeypatch.setattr( - rule, "mark_needinfo_tracking_complete", lambda bugids: retired.update(bugids) - ) - monkeypatch.setattr(rule, "get_landed_bug_ids", lambda bugs: set()) - - rule.schedule_needinfo_cleanup() - - assert retired == {"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..666794bc3 --- /dev/null +++ b/tests/rules/test_not_landed_cleanup.py @@ -0,0 +1,269 @@ +# 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") From 530f7a5fd47ba5e4ccc33d122d8d3bf7c9dadf3f Mon Sep 17 00:00:00 2001 From: Logan Rosen Date: Thu, 3 Sep 2026 23:33:54 -0400 Subject: [PATCH 3/3] Apply Ruff formatting Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- bugbot/rules/not_landed_cleanup.py | 14 +++++--------- tests/rules/test_not_landed_cleanup.py | 21 +++++---------------- 2 files changed, 10 insertions(+), 25 deletions(-) diff --git a/bugbot/rules/not_landed_cleanup.py b/bugbot/rules/not_landed_cleanup.py index b04a7dbb8..6c919f5e3 100644 --- a/bugbot/rules/not_landed_cleanup.py +++ b/bugbot/rules/not_landed_cleanup.py @@ -97,24 +97,20 @@ def get_revision_tracking( revision_ids = tracked[bugid] = set() revision_ids.update( int(revision_id) - for revision_id in extra.removeprefix( - NEEDINFO_TRACKING_PREFIX - ).split(",") + 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]: + 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]: + 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: diff --git a/tests/rules/test_not_landed_cleanup.py b/tests/rules/test_not_landed_cleanup.py index 666794bc3..1065c6917 100644 --- a/tests/rules/test_not_landed_cleanup.py +++ b/tests/rules/test_not_landed_cleanup.py @@ -91,9 +91,7 @@ def test_revision_tracking_distinguishes_legacy_and_empty_results(): _change(3, NEEDINFO_TRACKING_PREFIX), ] - assert NotLandedCleanup.get_revision_tracking( - changes, {"1", "2", "3"} - ) == { + assert NotLandedCleanup.get_revision_tracking(changes, {"1", "2", "3"}) == { "1": None, "2": {20, 21, 22}, "3": set(), @@ -139,9 +137,7 @@ def test_resolved_bug_clears_only_owned_flags(monkeypatch): 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_tracked_revision_ids", lambda bugids: {"123": {123}}) monkeypatch.setattr(rule, "get_landed_bug_ids", lambda revisions: set()) assert rule.get_bugs() == bugs @@ -154,18 +150,14 @@ 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}} - ) + 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"}]} - } + assert rule.autofix_changes == {"123": {"flags": [{"id": 123, "status": "X"}]}} def test_all_relevant_patches_must_land(monkeypatch): @@ -183,10 +175,7 @@ def test_all_relevant_patches_must_land(monkeypatch): 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) - } + bugs = {str(bugid): _bug(str(bugid), status="RESOLVED") for bugid in range(1, 52)} _set_bugs(monkeypatch, bugs) monkeypatch.setattr( rule,