diff --git a/libs/hackbot-runtime/hackbot_runtime/actions/phabricator.py b/libs/hackbot-runtime/hackbot_runtime/actions/phabricator.py index 098822398c..88773e51c0 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. Call submit_patch again " + "with that fixed." + ) + + @tool async def submit_patch( recorder: ActionsRecorder, @@ -80,6 +99,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..8364f340eb 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 "Call submit_patch again with that fixed" 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 1585dd2149..3d595bec75 100644 --- a/libs/hackbot-runtime/tests/test_phabricator_handler.py +++ b/libs/hackbot-runtime/tests/test_phabricator_handler.py @@ -123,6 +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"] == "s" async def test_submit_patch_sets_local_commits_property(monkeypatch):