From 761ce5f7b0222f51712ac3602fb14b130591b811 Mon Sep 17 00:00:00 2001 From: ayoubdiourin7 Date: Thu, 13 Aug 2026 12:46:01 +0200 Subject: [PATCH 1/3] Sanitize Phabricator summary headers --- .../actions/handlers/phabricator_handler.py | 18 +++++++++-- .../tests/test_phabricator_handler.py | 32 ++++++++++++++++++- 2 files changed, 47 insertions(+), 3 deletions(-) diff --git a/libs/hackbot-runtime/hackbot_runtime/actions/handlers/phabricator_handler.py b/libs/hackbot-runtime/hackbot_runtime/actions/handlers/phabricator_handler.py index bf8c88bfec..a40b5142d0 100644 --- a/libs/hackbot-runtime/hackbot_runtime/actions/handlers/phabricator_handler.py +++ b/libs/hackbot-runtime/hackbot_runtime/actions/handlers/phabricator_handler.py @@ -93,11 +93,25 @@ async def _repository_phid() -> str: # in the visible title only adds cleanup work when promoting to review. _WIP_PREFIX_RE = re.compile(r"^(?:WIP[: ]|WIP$)", re.IGNORECASE) +# Phabricator reparses summaries as commit messages. Test Plan aliases at the +# start of a line are interpreted as fields, making the summary ambiguous. +_PHABRICATOR_TEST_PLAN_HEADER_RE = re.compile( + r"^(?=(?:Test Plan|Testplan|Tested|Tests):)", + re.IGNORECASE | re.MULTILINE, +) + def _revision_title(title: str) -> str: return _WIP_PREFIX_RE.sub("", title).strip() +def _sanitize_summary(summary: str | None) -> str | None: + """Indent lines Phabricator would parse as Test Plan field headers.""" + if not summary: + return summary + return _PHABRICATOR_TEST_PLAN_HEADER_RE.sub(" ", summary) + + async def _revision_fields(revision_id: int) -> dict: """The current fields (title/summary/status) of an existing revision.""" result = await _conduit_request( @@ -152,7 +166,7 @@ def _arc_commit_message(title: str, summary: str | None, bug_id: Any, url: str) reconstructed commit reads identically to a moz-phab submission. Reviewers are always empty: hackbot never assigns them (draft submissions omit them). """ - body = summary or "" + body = _sanitize_summary(summary) or "" if body: body += "\n" body += f"\nDifferential Revision: {url}" @@ -219,7 +233,7 @@ class SubmitPatchHandler: async def apply(self, params: dict[str, Any], ctx: ApplyContext) -> ActionResult: bug_id = params["bug_id"] - summary = params.get("summary") + summary = _sanitize_summary(params.get("summary")) try: raw = await ctx.download_artifact(_DIFF_ARTIFACT_KEY) diff --git a/libs/hackbot-runtime/tests/test_phabricator_handler.py b/libs/hackbot-runtime/tests/test_phabricator_handler.py index 96986d7eac..b38589a952 100644 --- a/libs/hackbot-runtime/tests/test_phabricator_handler.py +++ b/libs/hackbot-runtime/tests/test_phabricator_handler.py @@ -86,6 +86,31 @@ def test_revision_title_strips_wip_prefix(): assert rt("WIP: Fix bug") == "Fix bug" +@pytest.mark.parametrize( + "header", + ["Tests", "tests", "Test Plan", "Testplan", "Tested"], +) +def test_sanitize_summary_indents_test_plan_headers(header): + summary = f"Explanation\n\n{header}: details" + assert phabricator_handler._sanitize_summary(summary) == ( + f"Explanation\n\n {header}: details" + ) + + +@pytest.mark.parametrize( + "summary", + [ + None, + "", + "Testing: details", + "Some Tests: details", + " Tests: already indented", + ], +) +def test_sanitize_summary_leaves_safe_text_unchanged(summary): + assert phabricator_handler._sanitize_summary(summary) == summary + + async def test_submit_patch_creates_planned_changes_revision(monkeypatch): fake, calls = _fake_conduit( { @@ -98,8 +123,12 @@ async def test_submit_patch_creates_planned_changes_revision(monkeypatch): phabricator_handler, "_repository_phid", AsyncMock(return_value="PHID-REPO-1") ) + summary = ( + "The migration picker now defaults to Documents.\n\n" + "Tests: `browser_file_migration.js` passes." + ) result = await phabricator_handler.SubmitPatchHandler().apply( - {"bug_id": 1, "title": "Fix", "summary": "s"}, + {"bug_id": 1, "title": "Fix", "summary": summary}, _ctx(), ) @@ -123,6 +152,7 @@ async def test_submit_patch_creates_planned_changes_revision(monkeypatch): assert transactions["plan-changes"] is True assert "reviewers.add" not in transactions assert transactions["bugzilla.bug-id"] == "1" + assert transactions["summary"] == summary.replace("\nTests:", "\n Tests:") async def test_submit_patch_sets_local_commits_property(monkeypatch): From 1e71336510b3bd266f70d4cfd2c56a1be3ce506e Mon Sep 17 00:00:00 2001 From: ayoubdiourin7 Date: Thu, 13 Aug 2026 17:49:29 +0200 Subject: [PATCH 2/3] Validate Phabricator summaries before recording --- .../actions/handlers/phabricator_handler.py | 18 +------- .../hackbot_runtime/actions/phabricator.py | 31 +++++++++++++- .../tests/test_phabricator_actions.py | 42 +++++++++++++++++++ .../tests/test_phabricator_handler.py | 33 +-------------- 4 files changed, 75 insertions(+), 49 deletions(-) diff --git a/libs/hackbot-runtime/hackbot_runtime/actions/handlers/phabricator_handler.py b/libs/hackbot-runtime/hackbot_runtime/actions/handlers/phabricator_handler.py index a40b5142d0..bf8c88bfec 100644 --- a/libs/hackbot-runtime/hackbot_runtime/actions/handlers/phabricator_handler.py +++ b/libs/hackbot-runtime/hackbot_runtime/actions/handlers/phabricator_handler.py @@ -93,25 +93,11 @@ async def _repository_phid() -> str: # in the visible title only adds cleanup work when promoting to review. _WIP_PREFIX_RE = re.compile(r"^(?:WIP[: ]|WIP$)", re.IGNORECASE) -# Phabricator reparses summaries as commit messages. Test Plan aliases at the -# start of a line are interpreted as fields, making the summary ambiguous. -_PHABRICATOR_TEST_PLAN_HEADER_RE = re.compile( - r"^(?=(?:Test Plan|Testplan|Tested|Tests):)", - re.IGNORECASE | re.MULTILINE, -) - def _revision_title(title: str) -> str: return _WIP_PREFIX_RE.sub("", title).strip() -def _sanitize_summary(summary: str | None) -> str | None: - """Indent lines Phabricator would parse as Test Plan field headers.""" - if not summary: - return summary - return _PHABRICATOR_TEST_PLAN_HEADER_RE.sub(" ", summary) - - async def _revision_fields(revision_id: int) -> dict: """The current fields (title/summary/status) of an existing revision.""" result = await _conduit_request( @@ -166,7 +152,7 @@ def _arc_commit_message(title: str, summary: str | None, bug_id: Any, url: str) reconstructed commit reads identically to a moz-phab submission. Reviewers are always empty: hackbot never assigns them (draft submissions omit them). """ - body = _sanitize_summary(summary) or "" + body = summary or "" if body: body += "\n" body += f"\nDifferential Revision: {url}" @@ -233,7 +219,7 @@ class SubmitPatchHandler: async def apply(self, params: dict[str, Any], ctx: ApplyContext) -> ActionResult: bug_id = params["bug_id"] - summary = _sanitize_summary(params.get("summary")) + summary = params.get("summary") try: raw = await ctx.download_artifact(_DIFF_ARTIFACT_KEY) diff --git a/libs/hackbot-runtime/hackbot_runtime/actions/phabricator.py b/libs/hackbot-runtime/hackbot_runtime/actions/phabricator.py index 098822398c..72a089d0f4 100644 --- a/libs/hackbot-runtime/hackbot_runtime/actions/phabricator.py +++ b/libs/hackbot-runtime/hackbot_runtime/actions/phabricator.py @@ -13,9 +13,10 @@ from __future__ import annotations +import re from typing import Annotated -from agent_tools.registry import tool, tools_in +from agent_tools.registry import ToolError, tool, tools_in from pydantic import Field from hackbot_runtime.actions.recorder import ActionsRecorder @@ -25,11 +26,29 @@ # in ``context.publish_changes`` — has to cover both types. PATCH_ACTION_TYPES = frozenset({"phabricator.submit_patch", "phabricator.update_patch"}) +_PHABRICATOR_TEST_PLAN_HEADER_RE = re.compile( + r"^(?:Test Plan|Testplan|Tested|Tests):", + re.IGNORECASE | re.MULTILINE, +) + def _confirm(recorder: ActionsRecorder, action_type: str) -> str: return f"Recorded {action_type} (#{len(recorder.actions) - 1})." +def _validate_summary(summary: str | None) -> None: + if not summary: + return + + match = _PHABRICATOR_TEST_PLAN_HEADER_RE.search(summary) + if match: + raise ToolError( + f'Invalid Phabricator summary: "{match.group()}" at the beginning of ' + "a line is interpreted as a Test Plan field. Add a leading space or " + "rephrase that line, then call submit_patch again." + ) + + @tool async def submit_patch( recorder: ActionsRecorder, @@ -48,7 +67,14 @@ async def submit_patch( ], summary: Annotated[ str | None, - Field(default=None, description="Revision summary/description."), + Field( + default=None, + description=( + "Revision summary/description. Do not start a line with a Test " + "Plan header or alias such as Tests:, Test Plan:, Testplan:, or " + "Tested:; indent or rephrase it." + ), + ), ] = None, ref: Annotated[ str | None, @@ -80,6 +106,7 @@ async def submit_patch( in the same run, written as `{{actions..url}}` (for example, inside a bug comment). """ + _validate_summary(summary) recorder.record( "phabricator.submit_patch", {"bug_id": bug_id, "title": title, "summary": summary}, diff --git a/libs/hackbot-runtime/tests/test_phabricator_actions.py b/libs/hackbot-runtime/tests/test_phabricator_actions.py index b8a947d24d..03ce75e9d4 100644 --- a/libs/hackbot-runtime/tests/test_phabricator_actions.py +++ b/libs/hackbot-runtime/tests/test_phabricator_actions.py @@ -1,6 +1,7 @@ """Tests for the phabricator recording tools (submit/update patch, comment).""" import pytest +from agent_tools.registry import ToolError from hackbot_runtime.actions import ActionsRecorder, phabricator @@ -19,6 +20,47 @@ async def test_submit_records_create_params_only(): assert "ref" not in action +@pytest.mark.parametrize( + "header", + ["Tests", "tests", "Test Plan", "Testplan", "Tested"], +) +async def test_submit_rejects_test_plan_headers(header): + rec = ActionsRecorder() + + with pytest.raises(ToolError) as exc: + await phabricator.submit_patch( + rec, + bug_id=1, + title="Fix", + reasoning="r", + summary=f"Explanation\n\n{header}: details", + ) + + assert header in str(exc.value) + assert "Add a leading space or rephrase that line" in str(exc.value) + assert rec.actions == [] + + +@pytest.mark.parametrize( + "summary", + [ + None, + "", + "Testing: details", + "Some Tests: details", + " Tests: already indented", + ], +) +async def test_submit_accepts_safe_summary(summary): + rec = ActionsRecorder() + + await phabricator.submit_patch( + rec, bug_id=1, title="Fix", reasoning="r", summary=summary + ) + + assert rec.actions[0]["params"]["summary"] == summary + + async def test_submit_requires_title(): rec = ActionsRecorder() with pytest.raises(TypeError): diff --git a/libs/hackbot-runtime/tests/test_phabricator_handler.py b/libs/hackbot-runtime/tests/test_phabricator_handler.py index b38589a952..de9a7ba79a 100644 --- a/libs/hackbot-runtime/tests/test_phabricator_handler.py +++ b/libs/hackbot-runtime/tests/test_phabricator_handler.py @@ -86,31 +86,6 @@ def test_revision_title_strips_wip_prefix(): assert rt("WIP: Fix bug") == "Fix bug" -@pytest.mark.parametrize( - "header", - ["Tests", "tests", "Test Plan", "Testplan", "Tested"], -) -def test_sanitize_summary_indents_test_plan_headers(header): - summary = f"Explanation\n\n{header}: details" - assert phabricator_handler._sanitize_summary(summary) == ( - f"Explanation\n\n {header}: details" - ) - - -@pytest.mark.parametrize( - "summary", - [ - None, - "", - "Testing: details", - "Some Tests: details", - " Tests: already indented", - ], -) -def test_sanitize_summary_leaves_safe_text_unchanged(summary): - assert phabricator_handler._sanitize_summary(summary) == summary - - async def test_submit_patch_creates_planned_changes_revision(monkeypatch): fake, calls = _fake_conduit( { @@ -123,12 +98,8 @@ async def test_submit_patch_creates_planned_changes_revision(monkeypatch): phabricator_handler, "_repository_phid", AsyncMock(return_value="PHID-REPO-1") ) - summary = ( - "The migration picker now defaults to Documents.\n\n" - "Tests: `browser_file_migration.js` passes." - ) result = await phabricator_handler.SubmitPatchHandler().apply( - {"bug_id": 1, "title": "Fix", "summary": summary}, + {"bug_id": 1, "title": "Fix", "summary": "s"}, _ctx(), ) @@ -152,7 +123,7 @@ async def test_submit_patch_creates_planned_changes_revision(monkeypatch): assert transactions["plan-changes"] is True assert "reviewers.add" not in transactions assert transactions["bugzilla.bug-id"] == "1" - assert transactions["summary"] == summary.replace("\nTests:", "\n Tests:") + assert transactions["summary"] == "s" async def test_submit_patch_sets_local_commits_property(monkeypatch): From 49ef4c66a701b64318019f74ca66f4edb3d79980 Mon Sep 17 00:00:00 2001 From: ayoubdiourin7 Date: Thu, 13 Aug 2026 19:25:51 +0200 Subject: [PATCH 3/3] Let the agent fix invalid Phabricator summaries --- .../hackbot_runtime/actions/phabricator.py | 13 +++---------- .../tests/test_phabricator_actions.py | 2 +- 2 files changed, 4 insertions(+), 11 deletions(-) diff --git a/libs/hackbot-runtime/hackbot_runtime/actions/phabricator.py b/libs/hackbot-runtime/hackbot_runtime/actions/phabricator.py index 72a089d0f4..88773e51c0 100644 --- a/libs/hackbot-runtime/hackbot_runtime/actions/phabricator.py +++ b/libs/hackbot-runtime/hackbot_runtime/actions/phabricator.py @@ -44,8 +44,8 @@ def _validate_summary(summary: str | None) -> None: if match: raise ToolError( f'Invalid Phabricator summary: "{match.group()}" at the beginning of ' - "a line is interpreted as a Test Plan field. Add a leading space or " - "rephrase that line, then call submit_patch again." + "a line is interpreted as a Test Plan field. Call submit_patch again " + "with that fixed." ) @@ -67,14 +67,7 @@ async def submit_patch( ], summary: Annotated[ str | None, - Field( - default=None, - description=( - "Revision summary/description. Do not start a line with a Test " - "Plan header or alias such as Tests:, Test Plan:, Testplan:, or " - "Tested:; indent or rephrase it." - ), - ), + Field(default=None, description="Revision summary/description."), ] = None, ref: Annotated[ str | None, diff --git a/libs/hackbot-runtime/tests/test_phabricator_actions.py b/libs/hackbot-runtime/tests/test_phabricator_actions.py index 03ce75e9d4..8364f340eb 100644 --- a/libs/hackbot-runtime/tests/test_phabricator_actions.py +++ b/libs/hackbot-runtime/tests/test_phabricator_actions.py @@ -37,7 +37,7 @@ async def test_submit_rejects_test_plan_headers(header): ) assert header in str(exc.value) - assert "Add a leading space or rephrase that line" in str(exc.value) + assert "Call submit_patch again with that fixed" in str(exc.value) assert rec.actions == []