From 79a5858b5fab10483b13b8b7a26ea9b9863d7bc6 Mon Sep 17 00:00:00 2001 From: ayoubdiourin7 Date: Thu, 6 Aug 2026 13:42:07 +0200 Subject: [PATCH 1/3] Structure Phabricator webhook comments as XML --- .../bug_fix/prompts/follow-up.md | 4 +- .../hackbot-api/app/phabricator_webhook.py | 104 ++++++--- services/hackbot-api/tests/test_webhooks.py | 199 ++++++++++++++++-- 3 files changed, 258 insertions(+), 49 deletions(-) diff --git a/agents/bug-fix/hackbot_agents/bug_fix/prompts/follow-up.md b/agents/bug-fix/hackbot_agents/bug_fix/prompts/follow-up.md index 978b82f7de..82a36b8cf0 100644 --- a/agents/bug-fix/hackbot_agents/bug_fix/prompts/follow-up.md +++ b/agents/bug-fix/hackbot_agents/bug_fix/prompts/follow-up.md @@ -6,7 +6,9 @@ Respond only to the comments quoted below. Ignore any earlier mentions of you el {comment} -A quoted comment rarely stands alone: an inline one only makes sense next to the code it sits on. Before acting, use the `phabricator` tools to read D{revision_id} and its thread, and to locate the comment context so you can understand it. Your tree is at the revision's latest diff, so a comment on an older `diff_id` may point at code that has since changed. +The block contains one service-generated `` element for each triggering comment. Its `comment_id` and `type` attributes match the names returned by the Phabricator tools; inline comments also have a `diff_id`. + +A quoted comment rarely stands alone: an inline one only makes sense next to the code it sits on. Before acting, use the `phabricator` tools to read D{revision_id} and its thread, and locate the relevant comment context so you can understand it. For an inline comment, use the supplied `diff_id` to inspect its diff. Your tree is at the revision's latest diff, so a comment on an older `diff_id` may point at code that has since changed. Then address each quoted comment by taking the matching path: diff --git a/services/hackbot-api/app/phabricator_webhook.py b/services/hackbot-api/app/phabricator_webhook.py index b17b436e57..2d5b631eca 100644 --- a/services/hackbot-api/app/phabricator_webhook.py +++ b/services/hackbot-api/app/phabricator_webhook.py @@ -10,7 +10,8 @@ import logging from dataclasses import dataclass -from typing import TYPE_CHECKING +from typing import TYPE_CHECKING, Literal +from xml.sax.saxutils import escape if TYPE_CHECKING: from phabricator_client import PhabricatorClient @@ -28,6 +29,9 @@ class HackbotMention: comment: str author_phid: str + comment_id: int + comment_type: Literal["comment", "inline"] + diff_id: int | None = None def triggering_transaction_phids(payload: dict) -> list[str]: @@ -39,6 +43,46 @@ def triggering_transaction_phids(payload: dict) -> list[str]: ] +def _build_hackbot_mention( + transaction: dict, comment: dict, author_phid: str +) -> HackbotMention: + """Build the minimal agent context for a matching comment transaction.""" + transaction_type = transaction["type"] + if transaction_type == "inline": + fields = transaction["fields"] + diff = fields["diff"] + diff_id = diff["id"] + else: + diff_id = None + + return HackbotMention( + comment=comment["content"]["raw"], + author_phid=author_phid, + comment_id=comment["id"], + comment_type="inline" if transaction_type == "inline" else "comment", + diff_id=diff_id, + ) + + +def _get_hackbot_mention( + transaction: dict, *, bot_phid: str, token: str +) -> HackbotMention | None: + """Return the matching comment and its anchor context from one transaction.""" + transaction_type = transaction.get("type") + if transaction_type not in _COMMENT_TYPES: + return None + + author_phid = transaction.get("authorPHID") + if not author_phid or (bot_phid and author_phid == bot_phid): + return None + + for comment in transaction.get("comments") or []: + comment_text = comment["content"]["raw"] + if token in comment_text: + return _build_hackbot_mention(transaction, comment, author_phid) + return None + + def find_hackbot_mentions( transactions: list[dict], triggering_phids: set[str], @@ -58,35 +102,39 @@ def find_hackbot_mentions( for transaction in transactions: if transaction.get("phid") not in triggering_phids: continue - if transaction.get("type") not in _COMMENT_TYPES: - continue - author_phid = transaction.get("authorPHID") - if not author_phid: - continue - if bot_phid and author_phid == bot_phid: - continue - for comment in transaction.get("comments") or []: - comment_text = (comment.get("content") or {}).get("raw") or "" - if token in comment_text: - matches.append( - HackbotMention( - comment=comment_text, - author_phid=author_phid, - ) - ) - break + mention = _get_hackbot_mention(transaction, bot_phid=bot_phid, token=token) + if mention is not None: + matches.append(mention) return matches -def _join_comments(comments: list[str]) -> str: +def _format_comment(mention: HackbotMention) -> str: + """Render one triggering comment as a service-generated XML element. + + For example, an inline comment is rendered as:: + + + @hackbot fix this + + """ + attributes = [ + f'comment_id="{mention.comment_id}"', + f'type="{mention.comment_type}"', + ] + if mention.comment_type == "inline": + attributes.append(f'diff_id="{mention.diff_id}"') + body = "\n".join(f" {line}" for line in escape(mention.comment).splitlines()) + return f" \n{body}\n " + + +def _join_comments(mentions: list[HackbotMention]) -> str: """Combine one or more triggering comments into the agent's ``comment`` input. - A lone comment is passed through unchanged; multiple are numbered so the - agent can tell them apart and address each. + Each comment is an XML element with the same identifiers used by the + Phabricator tools. The agent prompt supplies the enclosing ```` + element. """ - if len(comments) == 1: - return comments[0] - return "\n\n".join(f"[comment {i}]\n{c}" for i, c in enumerate(comments, 1)) + return "\n\n".join(_format_comment(mention) for mention in mentions) async def resolve_revision( @@ -135,10 +183,10 @@ async def detect_mention_and_revision( bot_phid=webhook.bot_phid, token=webhook.mention_token, ) - comments: list[str] = [] + authorized_mentions: list[HackbotMention] = [] for mention in mentions: if await authorizer.is_authorized(mention.author_phid): - comments.append(mention.comment) + authorized_mentions.append(mention) else: log.warning( "Ignoring %s mention from non-editbugs user %s on %s", @@ -146,7 +194,7 @@ async def detect_mention_and_revision( mention.author_phid, object_phid, ) - if not comments: + if not authorized_mentions: log.warning( "No actionable %s mention found in triggering transactions %s on %s", webhook.mention_token, @@ -154,7 +202,7 @@ async def detect_mention_and_revision( object_phid, ) return None - comment = _join_comments(comments) + comment = _join_comments(authorized_mentions) revision_id, bug_id = await resolve_revision(client, object_phid) if revision_id is None: diff --git a/services/hackbot-api/tests/test_webhooks.py b/services/hackbot-api/tests/test_webhooks.py index bc408f0a78..7118adc408 100644 --- a/services/hackbot-api/tests/test_webhooks.py +++ b/services/hackbot-api/tests/test_webhooks.py @@ -63,12 +63,21 @@ def test_signature_unconfigured_secret(monkeypatch): # --- mention detection / loop prevention --- -def _comment_txn(phid: str, author: str, raw: str, txn_type: str = "comment") -> dict: +def _comment_txn( + phid: str, + author: str, + raw: str, + txn_type: str = "comment", + *, + comment_id: int = 1, + fields: dict | None = None, +) -> dict: return { "phid": phid, "type": txn_type, "authorPHID": author, - "comments": [{"content": {"raw": raw}}], + "comments": [{"id": comment_id, "content": {"raw": raw}}], + "fields": fields or {}, } @@ -76,7 +85,7 @@ def test_find_mention_matches(): txns = [_comment_txn("PHID-XACT-1", "PHID-USER-a", "hey @hackbot please fix")] assert find_hackbot_mentions( txns, {"PHID-XACT-1"}, bot_phid="PHID-USER-bot", token="@hackbot" - ) == [HackbotMention("hey @hackbot please fix", "PHID-USER-a")] + ) == [HackbotMention("hey @hackbot please fix", "PHID-USER-a", 1, "comment")] def test_find_mention_no_token(): @@ -122,20 +131,59 @@ def test_find_mention_ignores_non_comment_type(): def test_find_mention_matches_inline_comment(): txns = [ - _comment_txn("PHID-XACT-1", "PHID-USER-a", "@hackbot here", txn_type="inline") + _comment_txn( + "PHID-XACT-1", + "PHID-USER-a", + "@hackbot here", + txn_type="inline", + fields={ + "diff": {"id": 456}, + "path": "browser/foo.cpp", + "line": 42, + }, + ) ] assert find_hackbot_mentions( txns, {"PHID-XACT-1"}, bot_phid="PHID-USER-bot", token="@hackbot" - ) == [HackbotMention("@hackbot here", "PHID-USER-a")] + ) == [ + HackbotMention( + "@hackbot here", + "PHID-USER-a", + 1, + "inline", + diff_id=456, + ) + ] def test_find_mention_collects_all_inline_matches(): # A review with several inline @hackbot comments (each its own transaction) # yields all of them, in order; comments without the token are skipped. txns = [ - _comment_txn("PHID-XACT-1", "PHID-USER-a", "@hackbot fix this", "inline"), - _comment_txn("PHID-XACT-2", "PHID-USER-a", "no mention here", "inline"), - _comment_txn("PHID-XACT-3", "PHID-USER-a", "@hackbot and this too", "inline"), + _comment_txn( + "PHID-XACT-1", + "PHID-USER-a", + "@hackbot fix this", + "inline", + comment_id=1, + fields={"diff": {"id": 1}, "path": "a.cpp", "line": 10}, + ), + _comment_txn( + "PHID-XACT-2", + "PHID-USER-a", + "no mention here", + "inline", + comment_id=2, + fields={"diff": {"id": 2}, "path": "b.cpp", "line": 20}, + ), + _comment_txn( + "PHID-XACT-3", + "PHID-USER-a", + "@hackbot and this too", + "inline", + comment_id=3, + fields={"diff": {"id": 3}, "path": "c.cpp", "line": 30}, + ), ] assert find_hackbot_mentions( txns, @@ -143,8 +191,20 @@ def test_find_mention_collects_all_inline_matches(): bot_phid="PHID-USER-bot", token="@hackbot", ) == [ - HackbotMention("@hackbot fix this", "PHID-USER-a"), - HackbotMention("@hackbot and this too", "PHID-USER-a"), + HackbotMention( + "@hackbot fix this", + "PHID-USER-a", + 1, + "inline", + diff_id=1, + ), + HackbotMention( + "@hackbot and this too", + "PHID-USER-a", + 3, + "inline", + diff_id=3, + ), ] @@ -156,23 +216,75 @@ def test_find_mention_one_per_transaction_ignores_comment_versions(): "type": "inline", "authorPHID": "PHID-USER-a", "comments": [ - {"content": {"raw": "@hackbot v1"}}, - {"content": {"raw": "@hackbot v2 edited"}}, + {"id": 456, "content": {"raw": "@hackbot v1"}}, + {"id": 456, "content": {"raw": "@hackbot v2 edited"}}, ], + "fields": {"diff": {"id": 456}, "path": "browser/foo.cpp", "line": 42}, } assert find_hackbot_mentions( [txn], {"PHID-XACT-1"}, bot_phid="PHID-USER-bot", token="@hackbot" - ) == [HackbotMention("@hackbot v1", "PHID-USER-a")] + ) == [ + HackbotMention( + "@hackbot v1", + "PHID-USER-a", + 456, + "inline", + diff_id=456, + ) + ] + + +def test_join_comments_renders_normal_comment_as_xml(): + mention = HackbotMention("only one", "PHID-USER-a", 123, "comment") + assert _join_comments([mention]) == ( + ' \n only one\n ' + ) -def test_join_comments_single_passthrough(): - assert _join_comments(["only one"]) == "only one" +def test_join_comments_renders_inline_comment_as_xml(): + mention = HackbotMention( + "fix this", + "PHID-USER-a", + 456, + "inline", + diff_id=456, + ) + assert _join_comments([mention]) == ( + ' \n' + " fix this\n" + " " + ) + +def test_join_comments_renders_mixed_comments_in_order(): + joined = _join_comments( + [ + HackbotMention("first", "PHID-USER-a", 1, "comment"), + HackbotMention( + "second", + "PHID-USER-a", + 2, + "inline", + diff_id=456, + ), + ] + ) + assert joined == ( + ' \n first\n \n\n' + ' \n' + " second\n" + " " + ) -def test_join_comments_numbers_multiple(): - joined = _join_comments(["first", "second"]) - assert "[comment 1]\nfirst" in joined - assert "[comment 2]\nsecond" in joined + +def test_join_comments_escapes_comment_body(): + assert _join_comments( + [HackbotMention("@hackbot & explain", "PHID-USER-a", 1, "comment")] + ) == ( + ' \n' + " @hackbot <fix> & explain\n" + " " + ) # --- revision resolution --- @@ -254,7 +366,54 @@ async def test_detect_mention_accepts_editbugs_member(monkeypatch): ["PHID-XACT-1"], authorizer=PhabricatorAuthorizer(client, AUTHORIZED_GROUP_PHID), ) - assert result == ("@hackbot please fix", 42, 12345) + assert result == ( + ' \n' + " @hackbot please fix\n" + " ", + 42, + 12345, + ) + client.search_transactions.assert_awaited_once_with("PHID-DREV-x") + + +async def test_detect_mention_enriches_inline_anchor(monkeypatch): + client = _FakeClient( + {"id": 42, "fields": {"bugzilla.bug-id": "12345"}}, + members={"PHID-USER-authorized"}, + ) + transactions = [ + _comment_txn( + "PHID-XACT-1", + "PHID-USER-authorized", + "@hackbot please fix", + "inline", + fields={ + "diff": {"id": 456}, + "path": "browser/foo.cpp", + "line": 42, + }, + ) + ] + monkeypatch.setattr( + client, + "search_transactions", + AsyncMock(return_value=transactions), + ) + + result = await detect_mention_and_revision( + client, + settings.webhook, + "PHID-DREV-x", + ["PHID-XACT-1"], + authorizer=PhabricatorAuthorizer(client, AUTHORIZED_GROUP_PHID), + ) + assert result == ( + ' \n' + " @hackbot please fix\n" + " ", + 42, + 12345, + ) # --- payload parsing --- From c8b12feb2783169197a5b7ca33591cd958f2f5fc Mon Sep 17 00:00:00 2001 From: ayoubdiourin7 Date: Thu, 6 Aug 2026 23:43:24 +0200 Subject: [PATCH 2/3] Simplified comment extraction and removed unnecessary helper functions --- .../bug_fix/prompts/follow-up.md | 4 +- .../hackbot-api/app/phabricator_webhook.py | 86 +++++++------------ services/hackbot-api/tests/test_webhooks.py | 56 ++++++------ 3 files changed, 59 insertions(+), 87 deletions(-) diff --git a/agents/bug-fix/hackbot_agents/bug_fix/prompts/follow-up.md b/agents/bug-fix/hackbot_agents/bug_fix/prompts/follow-up.md index 82a36b8cf0..978b82f7de 100644 --- a/agents/bug-fix/hackbot_agents/bug_fix/prompts/follow-up.md +++ b/agents/bug-fix/hackbot_agents/bug_fix/prompts/follow-up.md @@ -6,9 +6,7 @@ Respond only to the comments quoted below. Ignore any earlier mentions of you el {comment} -The block contains one service-generated `` element for each triggering comment. Its `comment_id` and `type` attributes match the names returned by the Phabricator tools; inline comments also have a `diff_id`. - -A quoted comment rarely stands alone: an inline one only makes sense next to the code it sits on. Before acting, use the `phabricator` tools to read D{revision_id} and its thread, and locate the relevant comment context so you can understand it. For an inline comment, use the supplied `diff_id` to inspect its diff. Your tree is at the revision's latest diff, so a comment on an older `diff_id` may point at code that has since changed. +A quoted comment rarely stands alone: an inline one only makes sense next to the code it sits on. Before acting, use the `phabricator` tools to read D{revision_id} and its thread, and to locate the comment context so you can understand it. Your tree is at the revision's latest diff, so a comment on an older `diff_id` may point at code that has since changed. Then address each quoted comment by taking the matching path: diff --git a/services/hackbot-api/app/phabricator_webhook.py b/services/hackbot-api/app/phabricator_webhook.py index 2d5b631eca..2d0cca0999 100644 --- a/services/hackbot-api/app/phabricator_webhook.py +++ b/services/hackbot-api/app/phabricator_webhook.py @@ -30,7 +30,7 @@ class HackbotMention: comment: str author_phid: str comment_id: int - comment_type: Literal["comment", "inline"] + comment_type: Literal["regular", "inline"] diff_id: int | None = None @@ -43,46 +43,6 @@ def triggering_transaction_phids(payload: dict) -> list[str]: ] -def _build_hackbot_mention( - transaction: dict, comment: dict, author_phid: str -) -> HackbotMention: - """Build the minimal agent context for a matching comment transaction.""" - transaction_type = transaction["type"] - if transaction_type == "inline": - fields = transaction["fields"] - diff = fields["diff"] - diff_id = diff["id"] - else: - diff_id = None - - return HackbotMention( - comment=comment["content"]["raw"], - author_phid=author_phid, - comment_id=comment["id"], - comment_type="inline" if transaction_type == "inline" else "comment", - diff_id=diff_id, - ) - - -def _get_hackbot_mention( - transaction: dict, *, bot_phid: str, token: str -) -> HackbotMention | None: - """Return the matching comment and its anchor context from one transaction.""" - transaction_type = transaction.get("type") - if transaction_type not in _COMMENT_TYPES: - return None - - author_phid = transaction.get("authorPHID") - if not author_phid or (bot_phid and author_phid == bot_phid): - return None - - for comment in transaction.get("comments") or []: - comment_text = comment["content"]["raw"] - if token in comment_text: - return _build_hackbot_mention(transaction, comment, author_phid) - return None - - def find_hackbot_mentions( transactions: list[dict], triggering_phids: set[str], @@ -102,9 +62,35 @@ def find_hackbot_mentions( for transaction in transactions: if transaction.get("phid") not in triggering_phids: continue - mention = _get_hackbot_mention(transaction, bot_phid=bot_phid, token=token) - if mention is not None: - matches.append(mention) + if transaction.get("type") not in _COMMENT_TYPES: + continue + + author_phid = transaction.get("authorPHID") + if not author_phid or (bot_phid and author_phid == bot_phid): + continue + + for comment in transaction.get("comments") or []: + comment_text = comment["content"]["raw"] + if token not in comment_text: + continue + + diff_id = ( + transaction["fields"]["diff"]["id"] + if transaction["type"] == "inline" + else None + ) + matches.append( + HackbotMention( + comment=comment_text, + author_phid=author_phid, + comment_id=comment["id"], + comment_type=( + "inline" if transaction["type"] == "inline" else "regular" + ), + diff_id=diff_id, + ) + ) + break return matches @@ -127,16 +113,6 @@ def _format_comment(mention: HackbotMention) -> str: return f" \n{body}\n " -def _join_comments(mentions: list[HackbotMention]) -> str: - """Combine one or more triggering comments into the agent's ``comment`` input. - - Each comment is an XML element with the same identifiers used by the - Phabricator tools. The agent prompt supplies the enclosing ```` - element. - """ - return "\n\n".join(_format_comment(mention) for mention in mentions) - - async def resolve_revision( client: PhabricatorClient, revision_phid: str ) -> tuple[int | None, int | None]: @@ -202,7 +178,7 @@ async def detect_mention_and_revision( object_phid, ) return None - comment = _join_comments(authorized_mentions) + comment = "\n\n".join(_format_comment(mention) for mention in authorized_mentions) revision_id, bug_id = await resolve_revision(client, object_phid) if revision_id is None: diff --git a/services/hackbot-api/tests/test_webhooks.py b/services/hackbot-api/tests/test_webhooks.py index 7118adc408..b62d098fac 100644 --- a/services/hackbot-api/tests/test_webhooks.py +++ b/services/hackbot-api/tests/test_webhooks.py @@ -20,7 +20,7 @@ ) from app.phabricator_webhook import ( HackbotMention, - _join_comments, + _format_comment, detect_mention_and_revision, find_hackbot_mentions, resolve_revision, @@ -85,7 +85,7 @@ def test_find_mention_matches(): txns = [_comment_txn("PHID-XACT-1", "PHID-USER-a", "hey @hackbot please fix")] assert find_hackbot_mentions( txns, {"PHID-XACT-1"}, bot_phid="PHID-USER-bot", token="@hackbot" - ) == [HackbotMention("hey @hackbot please fix", "PHID-USER-a", 1, "comment")] + ) == [HackbotMention("hey @hackbot please fix", "PHID-USER-a", 1, "regular")] def test_find_mention_no_token(): @@ -234,14 +234,14 @@ def test_find_mention_one_per_transaction_ignores_comment_versions(): ] -def test_join_comments_renders_normal_comment_as_xml(): - mention = HackbotMention("only one", "PHID-USER-a", 123, "comment") - assert _join_comments([mention]) == ( - ' \n only one\n ' +def test_format_comment_renders_regular_comment_as_xml(): + mention = HackbotMention("only one", "PHID-USER-a", 123, "regular") + assert _format_comment(mention) == ( + ' \n only one\n ' ) -def test_join_comments_renders_inline_comment_as_xml(): +def test_format_comment_renders_inline_comment_as_xml(): mention = HackbotMention( "fix this", "PHID-USER-a", @@ -249,39 +249,37 @@ def test_join_comments_renders_inline_comment_as_xml(): "inline", diff_id=456, ) - assert _join_comments([mention]) == ( + assert _format_comment(mention) == ( ' \n' " fix this\n" " " ) -def test_join_comments_renders_mixed_comments_in_order(): - joined = _join_comments( - [ - HackbotMention("first", "PHID-USER-a", 1, "comment"), - HackbotMention( - "second", - "PHID-USER-a", - 2, - "inline", - diff_id=456, - ), - ] - ) - assert joined == ( - ' \n first\n \n\n' +def test_format_comments_renders_mixed_comments_in_order(): + mentions = [ + HackbotMention("first", "PHID-USER-a", 1, "regular"), + HackbotMention( + "second", + "PHID-USER-a", + 2, + "inline", + diff_id=456, + ), + ] + formatted = "\n\n".join(_format_comment(mention) for mention in mentions) + assert formatted == ( + ' \n first\n \n\n' ' \n' " second\n" " " ) -def test_join_comments_escapes_comment_body(): - assert _join_comments( - [HackbotMention("@hackbot & explain", "PHID-USER-a", 1, "comment")] - ) == ( - ' \n' +def test_format_comment_escapes_comment_body(): + mention = HackbotMention("@hackbot & explain", "PHID-USER-a", 1, "regular") + assert _format_comment(mention) == ( + ' \n' " @hackbot <fix> & explain\n" " " ) @@ -367,7 +365,7 @@ async def test_detect_mention_accepts_editbugs_member(monkeypatch): authorizer=PhabricatorAuthorizer(client, AUTHORIZED_GROUP_PHID), ) assert result == ( - ' \n' + ' \n' " @hackbot please fix\n" " ", 42, From bea1a18258a76f1673d819d1dde65ae76a8be81d Mon Sep 17 00:00:00 2001 From: ayoubdiourin7 Date: Fri, 7 Aug 2026 16:48:59 +0200 Subject: [PATCH 3/3] Remove diff ID from Phabricator comment context --- .../hackbot-api/app/phabricator_webhook.py | 11 +------ services/hackbot-api/tests/test_webhooks.py | 32 +++---------------- 2 files changed, 5 insertions(+), 38 deletions(-) diff --git a/services/hackbot-api/app/phabricator_webhook.py b/services/hackbot-api/app/phabricator_webhook.py index 2d0cca0999..02979a604b 100644 --- a/services/hackbot-api/app/phabricator_webhook.py +++ b/services/hackbot-api/app/phabricator_webhook.py @@ -31,7 +31,6 @@ class HackbotMention: author_phid: str comment_id: int comment_type: Literal["regular", "inline"] - diff_id: int | None = None def triggering_transaction_phids(payload: dict) -> list[str]: @@ -74,11 +73,6 @@ def find_hackbot_mentions( if token not in comment_text: continue - diff_id = ( - transaction["fields"]["diff"]["id"] - if transaction["type"] == "inline" - else None - ) matches.append( HackbotMention( comment=comment_text, @@ -87,7 +81,6 @@ def find_hackbot_mentions( comment_type=( "inline" if transaction["type"] == "inline" else "regular" ), - diff_id=diff_id, ) ) break @@ -99,7 +92,7 @@ def _format_comment(mention: HackbotMention) -> str: For example, an inline comment is rendered as:: - + @hackbot fix this """ @@ -107,8 +100,6 @@ def _format_comment(mention: HackbotMention) -> str: f'comment_id="{mention.comment_id}"', f'type="{mention.comment_type}"', ] - if mention.comment_type == "inline": - attributes.append(f'diff_id="{mention.diff_id}"') body = "\n".join(f" {line}" for line in escape(mention.comment).splitlines()) return f" \n{body}\n " diff --git a/services/hackbot-api/tests/test_webhooks.py b/services/hackbot-api/tests/test_webhooks.py index b62d098fac..90074f3b84 100644 --- a/services/hackbot-api/tests/test_webhooks.py +++ b/services/hackbot-api/tests/test_webhooks.py @@ -70,14 +70,12 @@ def _comment_txn( txn_type: str = "comment", *, comment_id: int = 1, - fields: dict | None = None, ) -> dict: return { "phid": phid, "type": txn_type, "authorPHID": author, "comments": [{"id": comment_id, "content": {"raw": raw}}], - "fields": fields or {}, } @@ -136,11 +134,6 @@ def test_find_mention_matches_inline_comment(): "PHID-USER-a", "@hackbot here", txn_type="inline", - fields={ - "diff": {"id": 456}, - "path": "browser/foo.cpp", - "line": 42, - }, ) ] assert find_hackbot_mentions( @@ -151,7 +144,6 @@ def test_find_mention_matches_inline_comment(): "PHID-USER-a", 1, "inline", - diff_id=456, ) ] @@ -166,7 +158,6 @@ def test_find_mention_collects_all_inline_matches(): "@hackbot fix this", "inline", comment_id=1, - fields={"diff": {"id": 1}, "path": "a.cpp", "line": 10}, ), _comment_txn( "PHID-XACT-2", @@ -174,7 +165,6 @@ def test_find_mention_collects_all_inline_matches(): "no mention here", "inline", comment_id=2, - fields={"diff": {"id": 2}, "path": "b.cpp", "line": 20}, ), _comment_txn( "PHID-XACT-3", @@ -182,7 +172,6 @@ def test_find_mention_collects_all_inline_matches(): "@hackbot and this too", "inline", comment_id=3, - fields={"diff": {"id": 3}, "path": "c.cpp", "line": 30}, ), ] assert find_hackbot_mentions( @@ -196,14 +185,12 @@ def test_find_mention_collects_all_inline_matches(): "PHID-USER-a", 1, "inline", - diff_id=1, ), HackbotMention( "@hackbot and this too", "PHID-USER-a", 3, "inline", - diff_id=3, ), ] @@ -219,7 +206,6 @@ def test_find_mention_one_per_transaction_ignores_comment_versions(): {"id": 456, "content": {"raw": "@hackbot v1"}}, {"id": 456, "content": {"raw": "@hackbot v2 edited"}}, ], - "fields": {"diff": {"id": 456}, "path": "browser/foo.cpp", "line": 42}, } assert find_hackbot_mentions( [txn], {"PHID-XACT-1"}, bot_phid="PHID-USER-bot", token="@hackbot" @@ -229,7 +215,6 @@ def test_find_mention_one_per_transaction_ignores_comment_versions(): "PHID-USER-a", 456, "inline", - diff_id=456, ) ] @@ -247,12 +232,9 @@ def test_format_comment_renders_inline_comment_as_xml(): "PHID-USER-a", 456, "inline", - diff_id=456, ) assert _format_comment(mention) == ( - ' \n' - " fix this\n" - " " + ' \n fix this\n ' ) @@ -264,13 +246,12 @@ def test_format_comments_renders_mixed_comments_in_order(): "PHID-USER-a", 2, "inline", - diff_id=456, ), ] formatted = "\n\n".join(_format_comment(mention) for mention in mentions) assert formatted == ( ' \n first\n \n\n' - ' \n' + ' \n' " second\n" " " ) @@ -374,7 +355,7 @@ async def test_detect_mention_accepts_editbugs_member(monkeypatch): client.search_transactions.assert_awaited_once_with("PHID-DREV-x") -async def test_detect_mention_enriches_inline_anchor(monkeypatch): +async def test_detect_mention_formats_inline_comment(monkeypatch): client = _FakeClient( {"id": 42, "fields": {"bugzilla.bug-id": "12345"}}, members={"PHID-USER-authorized"}, @@ -385,11 +366,6 @@ async def test_detect_mention_enriches_inline_anchor(monkeypatch): "PHID-USER-authorized", "@hackbot please fix", "inline", - fields={ - "diff": {"id": 456}, - "path": "browser/foo.cpp", - "line": 42, - }, ) ] monkeypatch.setattr( @@ -406,7 +382,7 @@ async def test_detect_mention_enriches_inline_anchor(monkeypatch): authorizer=PhabricatorAuthorizer(client, AUTHORIZED_GROUP_PHID), ) assert result == ( - ' \n' + ' \n' " @hackbot please fix\n" " ", 42,