diff --git a/application/agents/tool_executor.py b/application/agents/tool_executor.py index 18472612..09c3377a 100644 --- a/application/agents/tool_executor.py +++ b/application/agents/tool_executor.py @@ -944,6 +944,33 @@ class ToolExecutor: artifact_id = str(artifact_id).strip() if artifact_id is not None else "" if artifact_id: tool_call_data["artifact_id"] = artifact_id + + # A single call can produce several files (``run_code`` writing more + # than one). ``artifact_id`` names only the first, which left the rest + # with no way into the UI, so report the full set with display names. + get_artifacts = ( + getattr(tool, "get_artifacts", None) + if tool_data["name"] != "api_tool" + else None + ) + if callable(get_artifacts): + artifacts = [] + try: + # Normalize inside the guard: a tool returning an unexpected + # shape must not break the call it just completed. + artifacts = [ + {"id": str(a["id"]).strip(), "filename": a.get("filename")} + for a in (get_artifacts(action_name, **parameters) or []) + if isinstance(a, dict) and a.get("id") + ] + except Exception: + logger.exception( + "Failed to extract artifacts from tool %s for action %s", + tool_data["name"], + action_name, + ) + if artifacts: + tool_call_data["artifacts"] = artifacts result_full = bound_result_full(str(result)) tool_call_data["resolved_arguments"] = resolved_arguments tool_call_data["result_full"] = result_full diff --git a/application/agents/tools/artifact_generator.py b/application/agents/tools/artifact_generator.py index 29fcf9c5..f53afbb0 100644 --- a/application/agents/tools/artifact_generator.py +++ b/application/agents/tools/artifact_generator.py @@ -435,6 +435,7 @@ class ArtifactGeneratorTool(Tool): self.workflow_run_id: Optional[str] = self.config.get("workflow_run_id") self.message_id: Optional[str] = self.config.get("message_id") self._last_artifact_id: Optional[str] = None + self._last_filename: Optional[str] = None # ------------------------------------------------------------------ # Tool ABC @@ -536,12 +537,19 @@ class ArtifactGeneratorTool(Tool): """Return the produced artifact id so the UI artifact rail lights up.""" return self._last_artifact_id + def get_artifacts(self, action_name: str, **kwargs: Any) -> List[Dict[str, Any]]: + """Return the produced artifact with its filename, for UI labelling.""" + if not self._last_artifact_id: + return [] + return [{"id": self._last_artifact_id, "filename": self._last_filename}] + # ------------------------------------------------------------------ # Dispatch # ------------------------------------------------------------------ def execute_action(self, action_name: str, **kwargs: Any) -> Dict[str, Any]: """Dispatch a create/edit/rewrite action.""" self._last_artifact_id = None + self._last_filename = None if not self.user_id: return {"status": "error", "error": "artifact_generator requires a valid user_id."} if self.conversation_id is None and self.workflow_run_id is None: @@ -595,6 +603,7 @@ class ArtifactGeneratorTool(Tool): if ref is None: return {"status": "error", "error": "failed to persist artifact."} self._last_artifact_id = ref["artifact_id"] + self._last_filename = ref.get("filename") return {"status": "ok", **ref} def _edit(self, **kwargs: Any) -> Dict[str, Any]: @@ -661,6 +670,7 @@ class ArtifactGeneratorTool(Tool): if ref is None: return {"status": "error", "error": "failed to persist artifact version."} self._last_artifact_id = ref["artifact_id"] + self._last_filename = ref.get("filename") return {"status": "ok", **ref} # ------------------------------------------------------------------ diff --git a/application/agents/tools/code_executor.py b/application/agents/tools/code_executor.py index eb54adaf..02e9f521 100644 --- a/application/agents/tools/code_executor.py +++ b/application/agents/tools/code_executor.py @@ -73,6 +73,7 @@ class CodeExecutorTool(Tool): # Static, deployment-level approval gate (mirrors the action metadata flag). self._require_approval: bool = bool(self.config.get("require_approval", False)) self._last_artifact_id: Optional[str] = None + self._last_artifacts: List[Dict[str, Any]] = [] # ------------------------------------------------------------------ # Tool ABC @@ -181,6 +182,15 @@ class CodeExecutorTool(Tool): """Return the primary produced artifact id so the UI artifact rail lights up.""" return self._last_artifact_id + def get_artifacts(self, action_name: str, **kwargs: Any) -> List[Dict[str, Any]]: + """Return every artifact this call produced, with display names. + + A single ``run_code`` can write several files. Reporting only the first + left the others reachable by API but invisible in the UI, and gave the + one button the tool's name rather than the file's. + """ + return list(self._last_artifacts) + def preview_decision(self, action_name: str, params: dict) -> Tuple[bool, bool]: """Return ``(requires_approval, denylist_forced)`` for the approval gate; never denylist-forced here.""" if action_name != "run_code": @@ -195,6 +205,7 @@ class CodeExecutorTool(Tool): if action_name != "run_code": return {"status": "error", "error": f"unknown action: {action_name}"} self._last_artifact_id = None + self._last_artifacts = [] return self._run_code(**kwargs) def _run_code(self, **kwargs: Any) -> Dict[str, Any]: @@ -404,6 +415,11 @@ class CodeExecutorTool(Tool): ) if captured: self._last_artifact_id = captured[0]["artifact_id"] + self._last_artifacts = [ + {"id": a["artifact_id"], "filename": a.get("filename")} + for a in captured + if a.get("artifact_id") + ] return captured def _shape_payload( diff --git a/frontend/src/conversation/ConversationBubble.tsx b/frontend/src/conversation/ConversationBubble.tsx index 8f621dcd..df942b20 100644 --- a/frontend/src/conversation/ConversationBubble.tsx +++ b/frontend/src/conversation/ConversationBubble.tsx @@ -129,11 +129,6 @@ const ConversationBubble = forwardRef< const editableQueryRef = useRef(null); const [isQuestionCollapsed, setIsQuestionCollapsed] = useState(true); - const completedArtifactCalls = (toolCalls ?? []).filter( - (toolCall) => toolCall.artifact_id && toolCall.status === 'completed', - ); - const artifactCount = completedArtifactCalls.length; - const formatToolName = (toolName: string | undefined): string => { if (!toolName) return ''; // Display-name overrides for tools whose label differs from the formatted key. @@ -147,6 +142,27 @@ const ConversationBubble = forwardRef< .join(' '); }; + // One entry per artifact, not per tool call: a single call (``run_code``) + // can write several files, and only the first was reachable before. + const completedArtifacts = (toolCalls ?? []) + .filter((toolCall) => toolCall.status === 'completed') + .flatMap((toolCall) => { + const produced = toolCall.artifacts?.length + ? toolCall.artifacts + : toolCall.artifact_id + ? [{ id: toolCall.artifact_id, filename: undefined }] + : []; + return produced.map((artifact) => ({ + id: artifact.id, + // The file's own name is what the user recognises; the tool that made + // it ("Code Executor") tells them nothing about which file this is. + label: + artifact.filename || formatToolName(toolCall.tool_name) || 'Artifact', + toolName: toolCall.tool_name, + callId: toolCall.call_id, + })); + }); + useOutsideAlerter(editableQueryRef, () => setIsEditClicked(false), [], true); useEffect(() => { @@ -455,16 +471,16 @@ const ConversationBubble = forwardRef< agentId={agentId} /> )} - {!message && onOpenArtifact && completedArtifactCalls.length > 0 && ( + {!message && onOpenArtifact && completedArtifacts.length > 0 && (
- {completedArtifactCalls.map((artifactCall, artifactIndex) => ( + {completedArtifacts.map((artifact, artifactIndex) => ( ))}
@@ -720,17 +735,17 @@ const ConversationBubble = forwardRef< ) : ( <> {onOpenArtifact && - completedArtifactCalls.map((artifactCall, artifactIndex) => ( + completedArtifacts.map((artifact, artifactIndex) => (
))} diff --git a/frontend/src/conversation/types/index.ts b/frontend/src/conversation/types/index.ts index 2757e251..f468eef4 100644 --- a/frontend/src/conversation/types/index.ts +++ b/frontend/src/conversation/types/index.ts @@ -13,6 +13,10 @@ export type ToolCallsType = { | 'denied' | 'requires_client_execution'; artifact_id?: string; + // Every artifact this call produced, with display names. A single call can + // write several files (``run_code``), and ``artifact_id`` names only the + // first — the rest had no way into the UI without this. + artifacts?: { id: string; filename?: string | null }[]; // Remote-device tool calls carry the device id so the approval UI can // offer a "don't ask again" sticky-pattern action without a lookup. device_id?: string; diff --git a/tests/agents/test_tool_artifact_reporting.py b/tests/agents/test_tool_artifact_reporting.py new file mode 100644 index 00000000..e96a9af9 --- /dev/null +++ b/tests/agents/test_tool_artifact_reporting.py @@ -0,0 +1,87 @@ +"""A tool call must report every artifact it produced, with display names. + +``get_artifact_id`` returns one id, so a single ``run_code`` that wrote two +files surfaced only the first — the second existed in the database and was +downloadable by API, but had no button in the UI and no way to reach it. The +one button it did render was labelled with the tool's name ("Code Executor"), +which says nothing about which file it opens. +""" + +from __future__ import annotations + +import pytest + +from application.agents.tools.artifact_generator import ArtifactGeneratorTool +from application.agents.tools.code_executor import CodeExecutorTool + + +@pytest.mark.unit +class TestCodeExecutorArtifacts: + def test_reports_every_captured_file(self): + tool = CodeExecutorTool({}) + tool._last_artifacts = [ + {"id": "a1", "filename": "nda.pdf"}, + {"id": "a2", "filename": "saas.pdf"}, + ] + assert tool.get_artifacts("run_code") == [ + {"id": "a1", "filename": "nda.pdf"}, + {"id": "a2", "filename": "saas.pdf"}, + ] + + def test_primary_id_still_reported_for_back_compat(self): + tool = CodeExecutorTool({}) + tool._last_artifact_id = "a1" + assert tool.get_artifact_id("run_code") == "a1" + + def test_no_artifacts_is_empty_not_none(self): + assert CodeExecutorTool({}).get_artifacts("run_code") == [] + + def test_returned_list_is_a_copy(self): + """A caller mutating the result must not corrupt the tool's state.""" + tool = CodeExecutorTool({}) + tool._last_artifacts = [{"id": "a1", "filename": "nda.pdf"}] + tool.get_artifacts("run_code").clear() + assert tool._last_artifacts == [{"id": "a1", "filename": "nda.pdf"}] + + +@pytest.mark.unit +class TestArtifactGeneratorArtifacts: + def test_carries_the_rendered_filename(self): + tool = ArtifactGeneratorTool({}) + tool._last_artifact_id = "b1" + tool._last_filename = "Mock_SaaS_Agreement.pdf" + assert tool.get_artifacts("create_artifact") == [ + {"id": "b1", "filename": "Mock_SaaS_Agreement.pdf"} + ] + + def test_nothing_produced_is_empty(self): + tool = ArtifactGeneratorTool({}) + tool._last_artifact_id = None + assert tool.get_artifacts("create_artifact") == [] + + +@pytest.mark.unit +class TestExecutorRecordsArtifacts: + """The executor must tolerate a tool returning an unexpected shape.""" + + @staticmethod + def _extract(produced): + """Mirror the executor's normalization step.""" + return [ + {"id": str(a["id"]).strip(), "filename": a.get("filename")} + for a in (produced or []) + if isinstance(a, dict) and a.get("id") + ] + + def test_drops_entries_without_an_id(self): + assert self._extract([{"filename": "x.pdf"}, {"id": "a1"}]) == [ + {"id": "a1", "filename": None} + ] + + def test_ignores_non_dict_entries(self): + assert self._extract(["nope", 42, {"id": "a1", "filename": "x.pdf"}]) == [ + {"id": "a1", "filename": "x.pdf"} + ] + + def test_none_is_empty(self): + assert self._extract(None) == []