diff --git a/libs/hackbot-runtime/hackbot_runtime/actions/__init__.py b/libs/hackbot-runtime/hackbot_runtime/actions/__init__.py index 9f9a04f787..6ae0891ec8 100644 --- a/libs/hackbot-runtime/hackbot_runtime/actions/__init__.py +++ b/libs/hackbot-runtime/hackbot_runtime/actions/__init__.py @@ -7,7 +7,14 @@ claude-sdk adapter is ``hackbot_runtime.actions.claude_sdk.actions_server_for``. """ -from hackbot_runtime.actions import bugzilla, phabricator, slack, testrail, try_server +from hackbot_runtime.actions import ( + bugzilla, + phabricator, + recorded_actions, + slack, + testrail, + try_server, +) from hackbot_runtime.actions.recorder import ActionHook, ActionsRecorder ACTIONS_SERVER_NAME = "actions" @@ -18,6 +25,7 @@ "ActionsRecorder", "bugzilla", "phabricator", + "recorded_actions", "slack", "testrail", "try_server", diff --git a/libs/hackbot-runtime/hackbot_runtime/actions/bugzilla.py b/libs/hackbot-runtime/hackbot_runtime/actions/bugzilla.py index 0cd5d4d2e9..98c1b2532b 100644 --- a/libs/hackbot-runtime/hackbot_runtime/actions/bugzilla.py +++ b/libs/hackbot-runtime/hackbot_runtime/actions/bugzilla.py @@ -26,8 +26,8 @@ ) -def _confirm(recorder: ActionsRecorder, action_type: str) -> str: - return f"Recorded {action_type} (#{len(recorder.actions) - 1})." +def _confirm(action: dict) -> str: + return f"Recorded {action['type']} (ID: {action['action_id']})." @tool @@ -61,12 +61,12 @@ async def update_bug( Recorded into the run summary for human review — does not modify Bugzilla. """ - recorder.record( + action = recorder.record( "bugzilla.update_bug", {"bug_id": bug_id, "changes": changes}, reasoning=reasoning, ) - return _confirm(recorder, "bugzilla.update_bug") + return _confirm(action) @tool @@ -91,12 +91,12 @@ async def add_comment( summary for human review — does not post to Bugzilla. """ text_with_footer = text.rstrip() + "\n\n---\n\n" + _COMMENT_FOOTER - recorder.record( + action = recorder.record( "bugzilla.add_comment", {"bug_id": bug_id, "text": text_with_footer, "is_private": is_private}, reasoning=reasoning, ) - return _confirm(recorder, "bugzilla.add_comment") + return _confirm(action) @tool @@ -181,13 +181,13 @@ async def add_attachment( if comment: params["comment"] = comment - recorder.record( + action = recorder.record( "bugzilla.add_attachment", params, reasoning=reasoning, attachments={"file": Path(file_path)}, ) - return _confirm(recorder, "bugzilla.add_attachment") + return _confirm(action) @tool @@ -231,8 +231,8 @@ async def create_bug( for k, v in (extra or {}).items(): body.setdefault(k, v) - recorder.record("bugzilla.create_bug", body, reasoning=reasoning) - return _confirm(recorder, "bugzilla.create_bug") + action = recorder.record("bugzilla.create_bug", body, reasoning=reasoning) + return _confirm(action) TOOLS = tools_in(__name__) diff --git a/libs/hackbot-runtime/hackbot_runtime/actions/claude_sdk.py b/libs/hackbot-runtime/hackbot_runtime/actions/claude_sdk.py index 35ea94eb63..226df45ca5 100644 --- a/libs/hackbot-runtime/hackbot_runtime/actions/claude_sdk.py +++ b/libs/hackbot-runtime/hackbot_runtime/actions/claude_sdk.py @@ -15,6 +15,7 @@ from hackbot_runtime.actions import ACTIONS_SERVER_NAME from hackbot_runtime.actions import bugzilla as _bugzilla from hackbot_runtime.actions import phabricator as _phabricator +from hackbot_runtime.actions import recorded_actions as _recorded_actions from hackbot_runtime.actions import slack as _slack from hackbot_runtime.actions import testrail as _testrail from hackbot_runtime.actions import try_server as _try_server @@ -37,7 +38,8 @@ def actions_server_for( if recorder is None: recorder = ActionsRecorder(artifacts_dir=fallback_artifacts_dir) tools = ( - _bugzilla.TOOLS + _recorded_actions.TOOLS + + _bugzilla.TOOLS + _phabricator.TOOLS + _testrail.TOOLS + _slack.TOOLS diff --git a/libs/hackbot-runtime/hackbot_runtime/actions/phabricator.py b/libs/hackbot-runtime/hackbot_runtime/actions/phabricator.py index 88773e51c0..5c70d80ed4 100644 --- a/libs/hackbot-runtime/hackbot_runtime/actions/phabricator.py +++ b/libs/hackbot-runtime/hackbot_runtime/actions/phabricator.py @@ -32,8 +32,8 @@ ) -def _confirm(recorder: ActionsRecorder, action_type: str) -> str: - return f"Recorded {action_type} (#{len(recorder.actions) - 1})." +def _confirm(action: dict) -> str: + return f"Recorded {action['type']} (ID: {action['action_id']})." def _validate_summary(summary: str | None) -> None: @@ -100,13 +100,13 @@ async def submit_patch( bug comment). """ _validate_summary(summary) - recorder.record( + action = recorder.record( "phabricator.submit_patch", {"bug_id": bug_id, "title": title, "summary": summary}, reasoning=reasoning, ref=ref, ) - return _confirm(recorder, "phabricator.submit_patch") + return _confirm(action) @tool @@ -141,12 +141,12 @@ async def update_patch( Only the diff changes: the revision keeps its title, summary, and bug association exactly as they are. """ - recorder.record( + action = recorder.record( "phabricator.update_patch", {"revision_id": revision_id}, reasoning=reasoning, ) - return _confirm(recorder, "phabricator.update_patch") + return _confirm(action) @tool @@ -167,12 +167,12 @@ async def add_comment( changes, use ``submit_patch`` instead. Recorded into the run summary for human review; nothing is posted to Phabricator during the run. """ - recorder.record( + action = recorder.record( "phabricator.add_comment", {"revision_id": revision_id, "text": text}, reasoning=reasoning, ) - return _confirm(recorder, "phabricator.add_comment") + return _confirm(action) TOOLS = tools_in(__name__) diff --git a/libs/hackbot-runtime/hackbot_runtime/actions/recorded_actions.py b/libs/hackbot-runtime/hackbot_runtime/actions/recorded_actions.py new file mode 100644 index 0000000000..b5960277de --- /dev/null +++ b/libs/hackbot-runtime/hackbot_runtime/actions/recorded_actions.py @@ -0,0 +1,52 @@ +"""Agent-facing tools for inspecting and retracting recorded actions.""" + +from __future__ import annotations + +from typing import Annotated + +from agent_tools.registry import tool, tools_in +from pydantic import Field + +from hackbot_runtime.actions.recorder import ActionsRecorder + + +def _table_cell(value: str | None) -> str: + """Format one value for a Markdown table cell.""" + if value is None: + return "" + return "
".join(value.splitlines()).replace("|", r"\|") + + +@tool +async def list_actions(recorder: ActionsRecorder) -> str: + """List the actions currently proposed by this agent run. + + Returns a Markdown table with each action's ID, type, and reasoning. + """ + actions = recorder.list_actions() + if not actions: + return "No recorded actions." + + rows = ["| ID | Action | Reasoning |", "| --- | --- | --- |"] + rows.extend( + f"| {_table_cell(action['action_id'])} " + f"| {_table_cell(action['type'])} " + f"| {_table_cell(action.get('reasoning'))} |" + for action in actions + ) + return "\n".join(rows) + + +@tool +async def remove_action( + recorder: ActionsRecorder, + action_id: Annotated[ + str, + Field(description="ID of the action to remove."), + ], +) -> dict: + """Remove and return a recorded action.""" + return recorder.remove_action(action_id) + + +TOOLS = tools_in(__name__) diff --git a/libs/hackbot-runtime/hackbot_runtime/actions/recorder.py b/libs/hackbot-runtime/hackbot_runtime/actions/recorder.py index dffca6e641..f26d9b007c 100644 --- a/libs/hackbot-runtime/hackbot_runtime/actions/recorder.py +++ b/libs/hackbot-runtime/hackbot_runtime/actions/recorder.py @@ -1,6 +1,10 @@ +import copy +import uuid from collections.abc import Callable, Mapping, Sequence from pathlib import Path +from agent_tools.registry import ToolError + from hackbot_runtime.artifacts import publish_file from hackbot_runtime.uploader import SignedPolicyUploader @@ -37,7 +41,8 @@ def __init__( artifacts_dir: Path | None = None, hooks: Mapping[str, Sequence[ActionHook]] = {}, ) -> None: - self._actions: list[dict] = [] + self._actions: dict[str, dict] = {} + self._next_action_sequence = 0 self._uploader = uploader self._artifacts_dir = artifacts_dir self._hooks = { @@ -62,17 +67,18 @@ def record( attachments: dict[str, Path] | None = None, ref: str | None = None, ) -> dict: - """Record an intended action. + """Record an action and return a detached copy with its ID. ``action_type`` uses ``.`` (e.g. ``bugzilla.update_bug``, ``phabricator.create_revision``). ``params`` is action-specific data the apply step will need. ``attachments`` maps a logical name to a local file path; each file is preserved under the stable key - ``attachments//``: uploaded via the runtime + ``attachments//``: uploaded via the runtime uploader when one is configured, otherwise copied into the local artifacts directory (so it is retrievable from compose/direct runs). - The recorded action references it by that key; the original local - path is not persisted (it disappears with the container). + The sequence is never reused, even after action removal. The recorded + action references it by that key; the original local path is not + persisted (it disappears with the container). ``ref`` optionally labels this action so a *later* action in the same run can reference its apply-time result (e.g. a Bugzilla comment's @@ -88,7 +94,9 @@ def record( recording leaves nothing behind: the action the hooks see carries no ``attachments`` key yet. """ - idx = len(self._actions) + sequence = self._next_action_sequence + self._next_action_sequence += 1 + action_id = f"action-{uuid.uuid4().hex}" action: dict = { "type": action_type, "params": params, @@ -106,15 +114,36 @@ def record( key = publish_file( self._uploader, self._artifacts_dir, - f"attachments/{idx}/{name}", + f"attachments/{sequence}/{name}", path, ) recorded_attachments.append({"name": name, "uploaded_key": key}) action["attachments"] = recorded_attachments - self._actions.append(action) - return action + self._actions[action_id] = action + return _detach(action_id, action) + + def list_actions(self) -> list[dict]: + """Return complete copies of the current actions with stable in-run IDs.""" + return [ + _detach(action_id, action) for action_id, action in self._actions.items() + ] + + def remove_action(self, action_id: str) -> dict: + """Remove and return an action.""" + action = self._actions.get(action_id) + if action is None: + raise ToolError(f"No recorded action with ID {action_id!r}.") + + removed = _detach(action_id, action) + del self._actions[action_id] + return removed @property def actions(self) -> list[dict]: - return list(self._actions) + return list(self._actions.values()) + + +def _detach(action_id: str, action: dict) -> dict: + """Return a detached copy of an action with its ID.""" + return {**copy.deepcopy(action), "action_id": action_id} diff --git a/libs/hackbot-runtime/hackbot_runtime/actions/slack.py b/libs/hackbot-runtime/hackbot_runtime/actions/slack.py index 591d6ae851..4fccbee380 100644 --- a/libs/hackbot-runtime/hackbot_runtime/actions/slack.py +++ b/libs/hackbot-runtime/hackbot_runtime/actions/slack.py @@ -64,8 +64,8 @@ async def post_message( Recorded into the run summary for human review -- does not post to Slack. """ - recorder.record(ACTION_TYPE, _params(channel, text), reasoning=reasoning) - return f"Recorded {ACTION_TYPE} (#{len(recorder.actions) - 1})." + action = recorder.record(ACTION_TYPE, _params(channel, text), reasoning=reasoning) + return f"Recorded {ACTION_TYPE} (ID: {action['action_id']})." def record_message( diff --git a/libs/hackbot-runtime/hackbot_runtime/actions/testrail.py b/libs/hackbot-runtime/hackbot_runtime/actions/testrail.py index d009dd2974..7edcaf4a69 100644 --- a/libs/hackbot-runtime/hackbot_runtime/actions/testrail.py +++ b/libs/hackbot-runtime/hackbot_runtime/actions/testrail.py @@ -87,8 +87,8 @@ def feature_must_not_be_blank(cls, value: str) -> str: return value -def _confirm(recorder: ActionsRecorder, action_type: str) -> str: - return f"Recorded {action_type} (#{len(recorder.actions) - 1})." +def _confirm(action: dict) -> str: + return f"Recorded {action['type']} (ID: {action['action_id']})." def _validated_params(feature: str, generated_test_cases: list[Any]) -> dict[str, Any]: @@ -128,8 +128,8 @@ async def submit_test_plan( Nothing is sent to TestRail during the agent run. """ params = _validated_params(feature, generated_test_cases) - recorder.record(ACTION_TYPE, params) - return _confirm(recorder, ACTION_TYPE) + action = recorder.record(ACTION_TYPE, params) + return _confirm(action) def record_test_plan( diff --git a/libs/hackbot-runtime/hackbot_runtime/actions/try_server.py b/libs/hackbot-runtime/hackbot_runtime/actions/try_server.py index 9b38e53496..5723e6248d 100644 --- a/libs/hackbot-runtime/hackbot_runtime/actions/try_server.py +++ b/libs/hackbot-runtime/hackbot_runtime/actions/try_server.py @@ -150,8 +150,8 @@ async def push( if tests: params["test_paths"] = validate_test_paths(tests) - recorder.record(TRY_PUSH_ACTION_TYPE, params, reasoning=reasoning, ref=ref) - return f"Recorded {TRY_PUSH_ACTION_TYPE} (#{len(recorder.actions) - 1})." + action = recorder.record(TRY_PUSH_ACTION_TYPE, params, reasoning=reasoning, ref=ref) + return f"Recorded {TRY_PUSH_ACTION_TYPE} (ID: {action['action_id']})." TOOLS = tools_in(__name__) diff --git a/libs/hackbot-runtime/tests/test_claude_sdk.py b/libs/hackbot-runtime/tests/test_claude_sdk.py index 2c59444588..07cecf7b6f 100644 --- a/libs/hackbot-runtime/tests/test_claude_sdk.py +++ b/libs/hackbot-runtime/tests/test_claude_sdk.py @@ -1,11 +1,18 @@ """Tests for the actions MCP server (built via agent-tools' adapter).""" +import json + import mcp.server.lowlevel.server as low from hackbot_runtime.actions import ActionsRecorder -from hackbot_runtime.actions.claude_sdk import actions_server_for +from hackbot_runtime.actions.claude_sdk import ( + actions_server_for, + actions_to_tool_names, +) from mcp.types import CallToolRequest, CallToolRequestParams, ListToolsRequest _ALL = [ + "recorded_actions.list_actions", + "recorded_actions.remove_action", "bugzilla.update_bug", "bugzilla.add_comment", "bugzilla.add_attachment", @@ -50,6 +57,8 @@ async def test_lists_expected_tools_without_recorder(): srv = _server(ActionsRecorder()) tools = await _list(srv) assert {t.name for t in tools} == { + "recorded_actions_list_actions", + "recorded_actions_remove_action", "bugzilla_update_bug", "bugzilla_add_comment", "bugzilla_add_attachment", @@ -115,3 +124,61 @@ async def test_actions_server_exposes_selected_testrail_tool(): ) tools = await _list(config["instance"]) assert {t.name for t in tools} == {"testrail_submit_test_plan"} + + +async def test_recorded_actions_tools_list_and_remove_complete_action(): + recorder = ActionsRecorder() + action_id = recorder.record( + "bugzilla.update_bug", + {"bug_id": 7, "changes": {"severity": "S2"}}, + reasoning="rule X", + )["action_id"] + srv = _server(recorder) + + listed_result = await _call(srv, "recorded_actions_list_actions", {}) + assert listed_result.content[0].text.splitlines() == [ + "| ID | Action | Reasoning |", + "| --- | --- | --- |", + f"| {action_id} | bugzilla.update_bug | rule X |", + ] + + removed_result = await _call( + srv, "recorded_actions_remove_action", {"action_id": action_id} + ) + removed = json.loads(removed_result.content[0].text) + assert removed == { + "type": "bugzilla.update_bug", + "params": {"bug_id": 7, "changes": {"severity": "S2"}}, + "reasoning": "rule X", + "action_id": action_id, + } + assert recorder.actions == [] + + +async def test_recorded_actions_remove_unknown_id_surfaces_is_error(): + srv = _server(ActionsRecorder()) + + result = await _call( + srv, "recorded_actions_remove_action", {"action_id": "action-404"} + ) + + assert result.isError is True + assert "No recorded action" in result.content[0].text + + +def test_actions_to_tool_names_maps_exactly_the_selected_tools(): + assert actions_to_tool_names( + [ + "recorded_actions.list_actions", + "recorded_actions.remove_action", + "bugzilla.update_bug", + ] + ) == [ + "mcp__actions__recorded_actions_list_actions", + "mcp__actions__recorded_actions_remove_action", + "mcp__actions__bugzilla_update_bug", + ] + + assert actions_to_tool_names(["bugzilla.update_bug"]) == [ + "mcp__actions__bugzilla_update_bug" + ] diff --git a/libs/hackbot-runtime/tests/test_recorder.py b/libs/hackbot-runtime/tests/test_recorder.py index 933662673b..b72dc68024 100644 --- a/libs/hackbot-runtime/tests/test_recorder.py +++ b/libs/hackbot-runtime/tests/test_recorder.py @@ -3,6 +3,7 @@ from pathlib import Path import pytest +from agent_tools.registry import ToolError from hackbot_runtime.actions import ActionsRecorder @@ -21,6 +22,7 @@ def test_record_basic_shape(): {"bug_id": 1, "changes": {"severity": "S2"}}, reasoning="rule X", ) + assert returned.pop("action_id").startswith("action-") assert returned == rec.actions[0] assert rec.actions == [ { @@ -108,7 +110,7 @@ def second(action): assert calls == ["first", "second"] assert returned["params"] == {"bug_id": 1, "priority": "P1", "seen": "P1"} - assert rec.actions[0] == returned + assert rec.actions[0]["params"] == returned["params"] def test_hooks_only_run_for_their_action_type(): @@ -217,3 +219,87 @@ def test_constructor_hooks_are_copied(): rec.record("bugzilla.update_bug", {"bug_id": 1}) assert len(rec.actions) == 1 + + +def test_list_actions_returns_stable_ids_and_complete_detached_payloads(): + rec = ActionsRecorder() + patch = rec.record( + "phabricator.submit_patch", + {"bug_id": 1, "title": "Fix"}, + reasoning="verified fix", + ref="patch", + ) + comment = rec.record( + "bugzilla.add_comment", + {"bug_id": 1, "text": "See {{actions.patch.url}}"}, + reasoning="announce the patch", + ) + + listed = rec.list_actions() + + assert [action["action_id"] for action in listed] == [ + patch["action_id"], + comment["action_id"], + ] + assert listed[0] == { + "action_id": patch["action_id"], + "type": "phabricator.submit_patch", + "params": {"bug_id": 1, "title": "Fix"}, + "reasoning": "verified fix", + "ref": "patch", + } + assert "action_id" not in rec.actions[0] + + listed[0]["params"]["title"] = "mutated copy" + assert rec.actions[0]["params"]["title"] == "Fix" + + +def test_remove_action_deletes_only_the_requested_action(): + rec = ActionsRecorder() + first = rec.record("bugzilla.update_bug", {"bug_id": 1}, reasoning="first") + second = rec.record("bugzilla.add_comment", {"bug_id": 1}, reasoning="second") + + removed = rec.remove_action(first["action_id"]) + + assert removed["action_id"] == first["action_id"] + assert removed["reasoning"] == "first" + assert rec.list_actions()[0]["action_id"] == second["action_id"] + assert [action["type"] for action in rec.actions] == ["bugzilla.add_comment"] + + +def test_remove_action_rejects_unknown_or_already_removed_id(): + rec = ActionsRecorder() + action_id = rec.record("bugzilla.update_bug", {"bug_id": 1})["action_id"] + rec.remove_action(action_id) + + with pytest.raises(ToolError, match="No recorded action"): + rec.remove_action(action_id) + + +def test_removed_action_id_and_attachment_key_are_not_reused(tmp_path): + first = tmp_path / "first.txt" + second = tmp_path / "second.txt" + first.write_text("first") + second.write_text("second") + rec = ActionsRecorder(artifacts_dir=tmp_path / "artifacts") + + removed_id = rec.record( + "bugzilla.add_attachment", {"bug_id": 1}, attachments={"file": first} + )["action_id"] + rec.remove_action(removed_id) + kept_id = rec.record( + "bugzilla.add_attachment", {"bug_id": 1}, attachments={"file": second} + )["action_id"] + + assert kept_id != removed_id + assert rec.list_actions()[0]["action_id"] == kept_id + + assert rec.actions[0]["attachments"] == [ + {"name": "file", "uploaded_key": "attachments/1/file"} + ] + assert (tmp_path / "artifacts" / "attachments" / "0" / "file").read_text() == ( + "first" + ) + assert (tmp_path / "artifacts" / "attachments" / "1" / "file").read_text() == ( + "second" + ) diff --git a/libs/hackbot-runtime/tests/test_runtime.py b/libs/hackbot-runtime/tests/test_runtime.py index 3469283b42..4a638a79a0 100644 --- a/libs/hackbot-runtime/tests/test_runtime.py +++ b/libs/hackbot-runtime/tests/test_runtime.py @@ -56,6 +56,35 @@ def test_summary_written_for_exception(tmp_path): assert "boom" in summary["error"] +def test_removed_action_is_absent_from_summary(tmp_path): + ctx = _ctx(tmp_path) + inaccurate = ctx.actions.record( + "bugzilla.update_bug", + {"bug_id": 1, "changes": {"severity": "S2"}}, + reasoning="inaccurate", + ) + ctx.actions.record( + "bugzilla.add_comment", + {"bug_id": 1, "text": "Corrected assessment"}, + reasoning="corrected", + ) + ctx.actions.remove_action(inaccurate["action_id"]) + + code = _finish(ctx, HackbotAgentResult(num_turns=1)) + + assert code == 0 + summary = json.loads( + (tmp_path / "artifacts" / "local-test" / "summary.json").read_text() + ) + assert summary["actions"] == [ + { + "type": "bugzilla.add_comment", + "params": {"bug_id": 1, "text": "Corrected assessment"}, + "reasoning": "corrected", + } + ] + + def test_non_result_return_is_contract_error(tmp_path): ctx = _ctx(tmp_path) # A bare dict (or None) is no longer accepted — only a HackbotAgentResult. diff --git a/libs/hackbot-runtime/tests/test_slack_actions.py b/libs/hackbot-runtime/tests/test_slack_actions.py index d80cd64762..b925754c23 100644 --- a/libs/hackbot-runtime/tests/test_slack_actions.py +++ b/libs/hackbot-runtime/tests/test_slack_actions.py @@ -14,7 +14,9 @@ async def test_post_message_records_action(): text=" a test regressed ", reasoning="sheriffs decide on the backout", ) - assert "slack.post_message (#0)" in confirmation + assert confirmation == ( + f"Recorded slack.post_message (ID: {rec.list_actions()[0]['action_id']})." + ) assert rec.actions == [ { "type": "slack.post_message", @@ -39,7 +41,7 @@ def test_record_message_supports_a_ref_for_later_reference(): action = slack.record_message(rec, "sheriffs", "backout recommended", ref="notice") assert action["ref"] == "notice" assert action["params"]["channel"] == "sheriffs" - assert rec.actions == [action] + assert rec.actions[0]["ref"] == "notice" def test_tools_are_exposed_under_the_slack_namespace(): diff --git a/libs/hackbot-runtime/tests/test_testrail_action.py b/libs/hackbot-runtime/tests/test_testrail_action.py index 75fce89617..4eecba5419 100644 --- a/libs/hackbot-runtime/tests/test_testrail_action.py +++ b/libs/hackbot-runtime/tests/test_testrail_action.py @@ -27,7 +27,10 @@ async def test_submit_test_plan_tool_records_deferred_action(): recorder, feature="Feature", generated_test_cases=_cases() ) - assert message == "Recorded testrail.submit_test_plan (#0)." + assert message == ( + f"Recorded testrail.submit_test_plan " + f"(ID: {recorder.list_actions()[0]['action_id']})." + ) assert recorder.actions[0]["type"] == ACTION_TYPE assert recorder.actions[0]["params"] == { "feature": "Feature",