From c7fc9b7fd66f4f20bd1ada1fc9cb033813081283 Mon Sep 17 00:00:00 2001 From: Jared Wein Date: Fri, 7 Aug 2026 17:43:20 -0400 Subject: [PATCH] Record the created comment id when applying a Bugzilla comment MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Field changes and a comment ride in one PUT /bug/{id} so Bugzilla applies them as a single transaction, but that response carries only {bugs: [{id, changes, last_change_time}]} — no comment id. So nothing in run_actions.result said which comment an action produced, unlike AddAttachmentHandler and CreateBugHandler, which do capture their ids. That matters downstream: both bug-fix and frontend-triage comment from the same Bugzilla account, so a tool reading feedback off those comments cannot tell which agent wrote one without joining back through run_actions — and with no comment id the join has to be inferred from comment text or timing. Read the id back instead of switching to POST /bug/{id}/comment, which would return it directly but split the write into two code paths and give up the single-transaction coalescing. One shared helper serves both handlers. Matching is on the text we posted, not "the newest comment": the latter would happily pick up an engineer replying in the same moment. Where the text does not match, no id is reported rather than a guessed one — a wrong id would silently misattribute a comment, which is worse for the consumer than a missing one that it can still infer. The read-back is best-effort in the other direction too: the comment is already posted by then, so a failure there must not fail the action. --- .../actions/handlers/bugzilla_handler.py | 71 ++++++++- .../tests/test_bugzilla_handler.py | 149 ++++++++++++++++-- 2 files changed, 202 insertions(+), 18 deletions(-) diff --git a/libs/hackbot-runtime/hackbot_runtime/actions/handlers/bugzilla_handler.py b/libs/hackbot-runtime/hackbot_runtime/actions/handlers/bugzilla_handler.py index e9650c9e41..ced945e278 100644 --- a/libs/hackbot-runtime/hackbot_runtime/actions/handlers/bugzilla_handler.py +++ b/libs/hackbot-runtime/hackbot_runtime/actions/handlers/bugzilla_handler.py @@ -42,7 +42,11 @@ def _headers() -> dict[str, str]: return {"X-Bugzilla-API-Key": api_key, "Content-Type": "application/json"} -def _request(method: str, path: str, json_body: dict[str, Any]) -> dict[str, Any]: +def _request( + method: str, path: str, json_body: dict[str, Any] | None = None +) -> dict[str, Any]: + # `json_body` defaults to None so this also serves the read in + # _resolve_comment_id -- requests omits the body entirely when it is None. response = requests.request( method, f"{_base_url()}/{path}", @@ -54,6 +58,50 @@ def _request(method: str, path: str, json_body: dict[str, Any]) -> dict[str, Any return response.json() +def _normalize(text: str) -> str: + """Compare comment bodies without tripping over whitespace round-tripping.""" + return "\n".join( + line.rstrip() for line in text.replace("\r\n", "\n").split("\n") + ).strip() + + +def _resolve_comment_id(bug_id: int, text: str) -> int | None: + """Find the comment a ``PUT /bug/{id}`` just created. + + Bugzilla's update response carries only ``{bugs: [{id, changes, + last_change_time}]}`` -- no comment id -- so the id has to be read back. + Matching on the text we posted is exact and account-agnostic, unlike "the + newest comment", which would pick up an engineer replying in the same + moment. ``is_markdown`` affects rendering only, so the stored text is what + we sent. + + Best-effort in both directions: the comment is already posted by the time we + get here, so a failure must not fail the action, and an uncertain match is + reported as None rather than guessed. Consumers that need attribution for + older comments infer it from text/timestamp anyway, and a wrong id there is + worse than a missing one. + """ + try: + data = _request("GET", f"bug/{bug_id}/comment") + except Exception: + log.exception("Could not read back comments for bug %s", bug_id) + return None + + bug = (data.get("bugs") or {}).get(str(bug_id)) or {} + wanted = _normalize(text) + # Newest wins: a re-run that posted identical text should resolve to the + # comment this call actually created. + matches = [ + c["id"] + for c in (bug.get("comments") or []) + if _normalize(c.get("text") or "") == wanted + ] + if not matches: + log.warning("No comment on bug %s matched the text just posted", bug_id) + return None + return max(matches) + + def _comment_body(params: dict[str, Any]) -> dict[str, Any]: """Build the ``comment`` object for a PUT /bug/{id}. @@ -84,7 +132,17 @@ async def apply(self, params: dict[str, Any], ctx: ApplyContext) -> ActionResult except Exception as exc: log.exception("Failed to update bug %s", bug_id) return ActionResult.failed(str(exc)) - return ActionResult.ok({"bug_id": bug_id, "url": _bug_url(bug_id)}) + + result: dict[str, Any] = {"bug_id": bug_id, "url": _bug_url(bug_id)} + # Only when this PUT actually carried a comment — a changes-only update + # created nothing to point at. + if params.get("comment"): + comment_id = _resolve_comment_id( + bug_id, params["comment"].get("body") or "" + ) + if comment_id is not None: + result["comment_id"] = comment_id + return ActionResult.ok(result) class AddCommentHandler: @@ -96,7 +154,14 @@ async def apply(self, params: dict[str, Any], ctx: ApplyContext) -> ActionResult except Exception as exc: log.exception("Failed to add comment to bug %s", bug_id) return ActionResult.failed(str(exc)) - return ActionResult.ok({"bug_id": bug_id, "url": _bug_url(bug_id)}) + + # Recorded so downstream consumers can attribute the comment to this + # run's agent without having to re-derive it from text or timing. + result: dict[str, Any] = {"bug_id": bug_id, "url": _bug_url(bug_id)} + comment_id = _resolve_comment_id(bug_id, params["text"]) + if comment_id is not None: + result["comment_id"] = comment_id + return ActionResult.ok(result) class AddAttachmentHandler: diff --git a/libs/hackbot-runtime/tests/test_bugzilla_handler.py b/libs/hackbot-runtime/tests/test_bugzilla_handler.py index 11771b579a..477877bfe2 100644 --- a/libs/hackbot-runtime/tests/test_bugzilla_handler.py +++ b/libs/hackbot-runtime/tests/test_bugzilla_handler.py @@ -21,6 +21,29 @@ async def download(key): ) +def _fake_request(comments=None, put_result=None): + """A ``_request`` stand-in that also answers the comment read-back. + + Posting a comment costs two calls now — the PUT, then a GET to recover the + comment id Bugzilla's update response omits — so any test involving a + comment has to serve both. Returns ``(request, calls)``. + """ + calls = [] + + def request(method, path, json_body=None): + calls.append((method, path, json_body)) + if method == "GET": + bug_id = path.split("/")[1] + return {"bugs": {bug_id: {"comments": comments or []}}} + return put_result if put_result is not None else {} + + return request, calls + + +def _puts(calls): + return [c for c in calls if c[0] == "PUT"] + + async def test_update_bug_handler_success(monkeypatch): calls = [] monkeypatch.setattr( @@ -49,16 +72,14 @@ def _raise(*_args): async def test_add_comment_handler_builds_comment_body(monkeypatch): - calls = [] - monkeypatch.setattr( - bugzilla_handler, "_request", lambda m, p, b: calls.append((m, p, b)) - ) + request, calls = _fake_request() + monkeypatch.setattr(bugzilla_handler, "_request", request) await bugzilla_handler.AddCommentHandler().apply( {"bug_id": 5, "text": "hi", "is_private": True}, _ctx() ) # is_markdown is always set: agents author Markdown (permalinks, the italic # footer), and without the flag Bugzilla renders the markup literally. - assert calls == [ + assert _puts(calls) == [ ( "PUT", "bug/5", @@ -111,10 +132,8 @@ async def test_create_bug_handler_success(monkeypatch): async def test_update_bug_handler_merges_changes_and_comment(monkeypatch): - calls = [] - monkeypatch.setattr( - bugzilla_handler, "_request", lambda m, p, b: calls.append((m, p, b)) - ) + request, calls = _fake_request() + monkeypatch.setattr(bugzilla_handler, "_request", request) changes = {"status": "RESOLVED"} await bugzilla_handler.UpdateBugHandler().apply( { @@ -124,7 +143,7 @@ async def test_update_bug_handler_merges_changes_and_comment(monkeypatch): }, _ctx(), ) - assert calls == [ + assert _puts(calls) == [ ( "PUT", "bug/7", @@ -136,15 +155,115 @@ async def test_update_bug_handler_merges_changes_and_comment(monkeypatch): async def test_update_bug_handler_comment_only(monkeypatch): - calls = [] - monkeypatch.setattr( - bugzilla_handler, "_request", lambda m, p, b: calls.append((m, p, b)) - ) + request, calls = _fake_request() + monkeypatch.setattr(bugzilla_handler, "_request", request) await bugzilla_handler.UpdateBugHandler().apply( {"bug_id": 7, "changes": {}, "comment": {"body": "hi", "is_private": True}}, _ctx(), ) - assert calls == [("PUT", "bug/7", {"comment": {"body": "hi", "is_private": True}})] + assert _puts(calls) == [ + ("PUT", "bug/7", {"comment": {"body": "hi", "is_private": True}}) + ] + + +# ---- comment id read-back --------------------------------------------------- +# +# Bugzilla's PUT /bug/{id} response carries no comment id, so the handlers read +# the bug's comments back and match on the text they posted. Downstream tools +# use that id to attribute a comment to the agent that wrote it. + + +async def test_add_comment_handler_records_comment_id(monkeypatch): + request, calls = _fake_request( + comments=[{"id": 11, "text": "something else"}, {"id": 12, "text": "hi"}] + ) + monkeypatch.setattr(bugzilla_handler, "_request", request) + result = await bugzilla_handler.AddCommentHandler().apply( + {"bug_id": 5, "text": "hi"}, _ctx() + ) + assert result.status == "applied" + assert result.result["comment_id"] == 12 + assert ("GET", "bug/5/comment", None) in calls + + +async def test_add_comment_handler_tolerates_whitespace_round_trip(monkeypatch): + # Bugzilla may hand back CRLF line endings and trimmed trailing spaces; that + # must still count as the comment we just posted. + request, _ = _fake_request(comments=[{"id": 20, "text": "line one\r\nline two"}]) + monkeypatch.setattr(bugzilla_handler, "_request", request) + result = await bugzilla_handler.AddCommentHandler().apply( + {"bug_id": 5, "text": "line one \nline two\n"}, _ctx() + ) + assert result.result["comment_id"] == 20 + + +async def test_add_comment_handler_picks_newest_duplicate(monkeypatch): + # A re-run can post identical text twice; the id we want is the one this + # call created, which is the highest. + request, _ = _fake_request( + comments=[{"id": 30, "text": "same"}, {"id": 31, "text": "same"}] + ) + monkeypatch.setattr(bugzilla_handler, "_request", request) + result = await bugzilla_handler.AddCommentHandler().apply( + {"bug_id": 5, "text": "same"}, _ctx() + ) + assert result.result["comment_id"] == 31 + + +async def test_add_comment_handler_applied_when_no_text_matches(monkeypatch): + # No confident match: report no id rather than guess at the newest comment, + # which could be an engineer replying in the same moment. + request, _ = _fake_request(comments=[{"id": 40, "text": "unrelated"}]) + monkeypatch.setattr(bugzilla_handler, "_request", request) + result = await bugzilla_handler.AddCommentHandler().apply( + {"bug_id": 5, "text": "hi"}, _ctx() + ) + assert result.status == "applied" + assert "comment_id" not in result.result + + +async def test_add_comment_handler_applied_when_read_back_fails(monkeypatch): + # The comment is already posted by then, so a failed read-back must not turn + # a successful post into a failed action. + def request(method, path, json_body=None): + if method == "GET": + raise RuntimeError("bugzilla down") + return {} + + monkeypatch.setattr(bugzilla_handler, "_request", request) + result = await bugzilla_handler.AddCommentHandler().apply( + {"bug_id": 5, "text": "hi"}, _ctx() + ) + assert result.status == "applied" + assert result.result["bug_id"] == 5 + assert "comment_id" not in result.result + + +async def test_update_bug_handler_records_comment_id_for_folded_comment(monkeypatch): + request, calls = _fake_request(comments=[{"id": 50, "text": "done"}]) + monkeypatch.setattr(bugzilla_handler, "_request", request) + result = await bugzilla_handler.UpdateBugHandler().apply( + { + "bug_id": 7, + "changes": {"status": "RESOLVED"}, + "comment": {"body": "done", "is_private": False}, + }, + _ctx(), + ) + assert result.result["comment_id"] == 50 + assert ("GET", "bug/7/comment", None) in calls + + +async def test_update_bug_handler_changes_only_does_not_read_back(monkeypatch): + # A changes-only update created no comment, so there is nothing to look up + # and no reason to spend a request on it. + request, calls = _fake_request() + monkeypatch.setattr(bugzilla_handler, "_request", request) + result = await bugzilla_handler.UpdateBugHandler().apply( + {"bug_id": 7, "changes": {"status": "RESOLVED"}}, _ctx() + ) + assert "comment_id" not in result.result + assert [c[0] for c in calls] == ["PUT"] def test_plan_coalesced_groups_update_plus_comment():