diff --git a/libs/hackbot-runtime/hackbot_runtime/actions/__init__.py b/libs/hackbot-runtime/hackbot_runtime/actions/__init__.py index 409476d90f..b9f0979c0d 100644 --- a/libs/hackbot-runtime/hackbot_runtime/actions/__init__.py +++ b/libs/hackbot-runtime/hackbot_runtime/actions/__init__.py @@ -7,7 +7,13 @@ claude-sdk adapter is ``hackbot_runtime.actions.claude_sdk.actions_server_for``. """ -from hackbot_runtime.actions import bugzilla, phabricator, slack, testrail +from hackbot_runtime.actions import ( + bugzilla, + phabricator, + recorded_actions, + slack, + testrail, +) from hackbot_runtime.actions.recorder import ActionHook, ActionsRecorder ACTIONS_SERVER_NAME = "actions" @@ -18,6 +24,7 @@ "ActionsRecorder", "bugzilla", "phabricator", + "recorded_actions", "slack", "testrail", ] diff --git a/libs/hackbot-runtime/hackbot_runtime/actions/bugzilla.py b/libs/hackbot-runtime/hackbot_runtime/actions/bugzilla.py index 0cd5d4d2e9..f9a76defda 100644 --- a/libs/hackbot-runtime/hackbot_runtime/actions/bugzilla.py +++ b/libs/hackbot-runtime/hackbot_runtime/actions/bugzilla.py @@ -27,7 +27,7 @@ def _confirm(recorder: ActionsRecorder, action_type: str) -> str: - return f"Recorded {action_type} (#{len(recorder.actions) - 1})." + return f"Recorded {action_type} as {recorder.last_action_id}." @tool diff --git a/libs/hackbot-runtime/hackbot_runtime/actions/claude_sdk.py b/libs/hackbot-runtime/hackbot_runtime/actions/claude_sdk.py index 1166042e4f..8a21848bb9 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.recorder import ActionsRecorder @@ -35,7 +36,13 @@ def actions_server_for( """ if recorder is None: recorder = ActionsRecorder(artifacts_dir=fallback_artifacts_dir) - tools = _bugzilla.TOOLS + _phabricator.TOOLS + _testrail.TOOLS + _slack.TOOLS + tools = ( + _recorded_actions.TOOLS + + _bugzilla.TOOLS + + _phabricator.TOOLS + + _testrail.TOOLS + + _slack.TOOLS + ) if types is not None: wanted = set(types) tools = [t for t in tools if t.dotted in wanted] diff --git a/libs/hackbot-runtime/hackbot_runtime/actions/phabricator.py b/libs/hackbot-runtime/hackbot_runtime/actions/phabricator.py index 098822398c..cc5d494ed4 100644 --- a/libs/hackbot-runtime/hackbot_runtime/actions/phabricator.py +++ b/libs/hackbot-runtime/hackbot_runtime/actions/phabricator.py @@ -27,7 +27,7 @@ def _confirm(recorder: ActionsRecorder, action_type: str) -> str: - return f"Recorded {action_type} (#{len(recorder.actions) - 1})." + return f"Recorded {action_type} as {recorder.last_action_id}." @tool 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..e3ae280734 --- /dev/null +++ b/libs/hackbot-runtime/hackbot_runtime/actions/recorded_actions.py @@ -0,0 +1,48 @@ +"""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 + + +@tool +async def list_actions(recorder: ActionsRecorder) -> list[dict]: + """List every action currently proposed by this agent run. + + Returns each action's stable in-run ID and its complete recorded payload, + including parameters, reasoning, references, and attachment metadata. Use + this when earlier action details are no longer present in your context or + before deciding whether a proposal needs to be retracted. + """ + return recorder.list_actions() + + +@tool +async def remove_action( + recorder: ActionsRecorder, + action_id: Annotated[ + str, + Field( + description=( + "Exact stable ID returned when the action was recorded or by " + "list_actions (for example, action-2)." + ) + ), + ], +) -> dict: + """Retract one proposed action from this agent run. + + The removed action will not appear in the final run summary and cannot be + applied. This operation accepts exactly one action ID and has no cascade or + force mode. + """ + removed = recorder.remove_action(action_id) + return {"removed": removed, "remaining_count": recorder.action_count} + + +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..7108fc154d 100644 --- a/libs/hackbot-runtime/hackbot_runtime/actions/recorder.py +++ b/libs/hackbot-runtime/hackbot_runtime/actions/recorder.py @@ -1,6 +1,9 @@ +import copy 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 +40,12 @@ def __init__( artifacts_dir: Path | None = None, hooks: Mapping[str, Sequence[ActionHook]] = {}, ) -> None: - self._actions: list[dict] = [] + # IDs are deliberately separate from the serialized action payload. They + # are handles for managing proposals while the agent is running, not a + # new field in the summary/API contract. + self._actions: dict[str, dict] = {} + self._next_action_sequence = 0 + self._last_action_id: str | None = None self._uploader = uploader self._artifacts_dir = artifacts_dir self._hooks = { @@ -68,11 +76,12 @@ def record( ``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 +97,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-{sequence}" action: dict = { "type": action_type, "params": params, @@ -106,15 +117,49 @@ 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) + self._actions[action_id] = action + self._last_action_id = action_id return action + def list_actions(self) -> list[dict]: + """Return complete copies of the current actions with stable in-run IDs.""" + return [ + {**copy.deepcopy(action), "action_id": action_id} + for action_id, action in self._actions.items() + ] + + def remove_action(self, action_id: str) -> dict: + """Remove one action. + + The returned payload includes the stable ID and is detached from recorder + state. Removing an action only changes the proposals that will be written + to ``summary.json``; an attachment already uploaded for it may remain as + an unreferenced artifact until normal storage cleanup. + """ + action = self._actions.get(action_id) + if action is None: + raise ToolError(f"No recorded action with ID {action_id!r}.") + + removed = {**copy.deepcopy(action), "action_id": action_id} + del self._actions[action_id] + return removed + + @property + def last_action_id(self) -> str: + if self._last_action_id is None: + raise RuntimeError("No action has been recorded.") + return self._last_action_id + + @property + def action_count(self) -> int: + return len(self._actions) + @property def actions(self) -> list[dict]: - return list(self._actions) + return list(self._actions.values()) diff --git a/libs/hackbot-runtime/hackbot_runtime/actions/slack.py b/libs/hackbot-runtime/hackbot_runtime/actions/slack.py index 591d6ae851..6abc152052 100644 --- a/libs/hackbot-runtime/hackbot_runtime/actions/slack.py +++ b/libs/hackbot-runtime/hackbot_runtime/actions/slack.py @@ -65,7 +65,7 @@ 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})." + return f"Recorded {ACTION_TYPE} as {recorder.last_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..d63014e247 100644 --- a/libs/hackbot-runtime/hackbot_runtime/actions/testrail.py +++ b/libs/hackbot-runtime/hackbot_runtime/actions/testrail.py @@ -88,7 +88,7 @@ def feature_must_not_be_blank(cls, value: str) -> str: def _confirm(recorder: ActionsRecorder, action_type: str) -> str: - return f"Recorded {action_type} (#{len(recorder.actions) - 1})." + return f"Recorded {action_type} as {recorder.last_action_id}." def _validated_params(feature: str, generated_test_cases: list[Any]) -> dict[str, Any]: diff --git a/libs/hackbot-runtime/tests/test_claude_sdk.py b/libs/hackbot-runtime/tests/test_claude_sdk.py index 2c59444588..9dc836aa30 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() + recorder.record( + "bugzilla.update_bug", + {"bug_id": 7, "changes": {"severity": "S2"}}, + reasoning="rule X", + ) + srv = _server(recorder) + + listed_result = await _call(srv, "recorded_actions_list_actions", {}) + listed = json.loads(listed_result.content[0].text) + assert listed == [ + { + "type": "bugzilla.update_bug", + "params": {"bug_id": 7, "changes": {"severity": "S2"}}, + "reasoning": "rule X", + "action_id": "action-0", + } + ] + + removed_result = await _call( + srv, "recorded_actions_remove_action", {"action_id": "action-0"} + ) + removed = json.loads(removed_result.content[0].text) + assert removed["removed"] == listed[0] + assert removed["remaining_count"] == 0 + 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..3edd2a10d7 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 @@ -217,3 +218,79 @@ 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() + rec.record( + "phabricator.submit_patch", + {"bug_id": 1, "title": "Fix"}, + reasoning="verified fix", + ref="patch", + ) + 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] == ["action-0", "action-1"] + assert listed[0] == { + "action_id": "action-0", + "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() + rec.record("bugzilla.update_bug", {"bug_id": 1}, reasoning="first") + rec.record("bugzilla.add_comment", {"bug_id": 1}, reasoning="second") + + removed = rec.remove_action("action-0") + + assert removed["action_id"] == "action-0" + assert removed["reasoning"] == "first" + assert rec.action_count == 1 + assert rec.list_actions()[0]["action_id"] == "action-1" + assert [action["type"] for action in rec.actions] == ["bugzilla.add_comment"] + + +def test_remove_action_rejects_unknown_or_already_removed_id(): + rec = ActionsRecorder() + rec.record("bugzilla.update_bug", {"bug_id": 1}) + rec.remove_action("action-0") + + with pytest.raises(ToolError, match="No recorded action"): + rec.remove_action("action-0") + + +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") + + rec.record("bugzilla.add_attachment", {"bug_id": 1}, attachments={"file": first}) + rec.remove_action("action-0") + rec.record("bugzilla.add_attachment", {"bug_id": 1}, attachments={"file": second}) + + assert rec.list_actions()[0]["action_id"] == "action-1" + 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..f7e1483096 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) + 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("action-0") + + 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..6dd41cefd7 100644 --- a/libs/hackbot-runtime/tests/test_slack_actions.py +++ b/libs/hackbot-runtime/tests/test_slack_actions.py @@ -14,7 +14,7 @@ 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 == "Recorded slack.post_message as action-0." assert rec.actions == [ { "type": "slack.post_message", diff --git a/libs/hackbot-runtime/tests/test_testrail_action.py b/libs/hackbot-runtime/tests/test_testrail_action.py index 75fce89617..7c5a966a90 100644 --- a/libs/hackbot-runtime/tests/test_testrail_action.py +++ b/libs/hackbot-runtime/tests/test_testrail_action.py @@ -27,7 +27,7 @@ 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 == "Recorded testrail.submit_test_plan as action-0." assert recorder.actions[0]["type"] == ACTION_TYPE assert recorder.actions[0]["params"] == { "feature": "Feature",