-
Notifications
You must be signed in to change notification settings - Fork 348
Support listing and deleting recorded agent actions #6571
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Changes from all commits
202320c
c582040
8a5bef1
adf58b9
962b684
0f6dd27
c99c465
de9fab7
83547bc
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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 "<br>".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.""" | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The docstrings here targets the agent, so it should provide what should help the agent only. In this case, no need to mention the return part, and the returned value should be something that confirms the what happened instead of the removed action which could be a bit confusing. |
||
| return recorder.remove_action(action_id) | ||
|
|
||
|
|
||
| TOOLS = tools_in(__name__) | ||
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
| @@ -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 ``<domain>.<verb>`` (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/<action_index>/<name>``: uploaded via the runtime | ||||||
| ``attachments/<action_sequence>/<name>``: uploaded via the runtime | ||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It should be action id, we do not want to use sequence. |
||||||
| 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 | ||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Also, sequential IDs would be easy for the agent to guess and might remove things by accident. We could use UUIDs, which would be harder to guess. |
||||||
| 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() | ||||||
| ] | ||||||
|
Comment on lines
+128
to
+130
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I would return MD table, with ID, Action (based on the tool name), and reasoning. |
||||||
|
|
||||||
| def remove_action(self, action_id: str) -> dict: | ||||||
| """Remove and return an action.""" | ||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The docstrings are not clear here.
Suggested change
|
||||||
| 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} | ||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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']})." | ||
|
Comment on lines
+67
to
+68
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We should file a follow-up issue to centralize the confirmation message. |
||
|
|
||
|
|
||
| def record_message( | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What if we call it
actions_manageror something related instead ofrecorded_actions? WDYT?