mirror of
https://github.com/tiennm99/DocsGPT.git
synced 2026-10-13 02:14:23 +00:00
feat: better artefact in convo UI
This commit is contained in:
1 parent
e0649d25cf
commit
132a2de3bf
6 files changed
+183
-22
No files matched your search
@@ -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
|
||||
|
||||
@@ -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}
|
||||
|
||||
# ------------------------------------------------------------------
|
||||
|
||||
@@ -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(
|
||||
|
||||
@@ -129,11 +129,6 @@ const ConversationBubble = forwardRef<
|
||||
const editableQueryRef = useRef<HTMLDivElement>(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 && (
|
||||
<div className="my-2 ml-2 flex flex-wrap justify-start gap-2">
|
||||
{completedArtifactCalls.map((artifactCall, artifactIndex) => (
|
||||
{completedArtifacts.map((artifact, artifactIndex) => (
|
||||
<Button
|
||||
key={artifactCall.call_id ?? artifactIndex}
|
||||
key={artifact.id ?? `${artifact.callId}-${artifactIndex}`}
|
||||
type="button"
|
||||
onClick={() =>
|
||||
onOpenArtifact({
|
||||
id: artifactCall.artifact_id!,
|
||||
toolName: artifactCall.tool_name,
|
||||
id: artifact.id,
|
||||
toolName: artifact.toolName,
|
||||
})
|
||||
}
|
||||
className="h-auto rounded-full bg-purple-100 px-3 py-2 text-sm font-medium text-purple-700 hover:bg-purple-200 dark:bg-purple-900/30 dark:text-purple-300 dark:hover:bg-purple-900/50"
|
||||
@@ -488,10 +504,9 @@ const ConversationBubble = forwardRef<
|
||||
d="M2.458 12C3.732 7.943 7.523 5 12 5c4.478 0 8.268 2.943 9.542 7-1.274 4.057-5.064 7-9.542 7-4.477 0-8.268-2.943-9.542-7z"
|
||||
/>
|
||||
</svg>
|
||||
{(artifactCall.tool_name
|
||||
? formatToolName(artifactCall.tool_name)
|
||||
: 'Artifact') +
|
||||
(artifactCount > 1 ? ` (${artifactIndex + 1})` : '')}
|
||||
<span className="max-w-50 truncate" title={artifact.label}>
|
||||
{artifact.label}
|
||||
</span>
|
||||
</Button>
|
||||
))}
|
||||
</div>
|
||||
@@ -720,17 +735,17 @@ const ConversationBubble = forwardRef<
|
||||
) : (
|
||||
<>
|
||||
{onOpenArtifact &&
|
||||
completedArtifactCalls.map((artifactCall, artifactIndex) => (
|
||||
completedArtifacts.map((artifact, artifactIndex) => (
|
||||
<div
|
||||
key={artifactCall.call_id ?? artifactIndex}
|
||||
key={artifact.id ?? `${artifact.callId}-${artifactIndex}`}
|
||||
className="relative flex items-center justify-center"
|
||||
>
|
||||
<Button
|
||||
type="button"
|
||||
onClick={() =>
|
||||
onOpenArtifact({
|
||||
id: artifactCall.artifact_id!,
|
||||
toolName: artifactCall.tool_name,
|
||||
id: artifact.id,
|
||||
toolName: artifact.toolName,
|
||||
})
|
||||
}
|
||||
className="h-auto rounded-full bg-purple-100 px-3 py-2 text-sm font-medium text-purple-700 hover:bg-purple-200 dark:bg-purple-900/30 dark:text-purple-300 dark:hover:bg-purple-900/50"
|
||||
@@ -755,10 +770,12 @@ const ConversationBubble = forwardRef<
|
||||
d="M2.458 12C3.732 7.943 7.523 5 12 5c4.478 0 8.268 2.943 9.542 7-1.274 4.057-5.064 7-9.542 7-4.477 0-8.268-2.943-9.542-7z"
|
||||
/>
|
||||
</svg>
|
||||
{(artifactCall.tool_name
|
||||
? formatToolName(artifactCall.tool_name)
|
||||
: 'Artifact') +
|
||||
(artifactCount > 1 ? ` (${artifactIndex + 1})` : '')}
|
||||
<span
|
||||
className="max-w-50 truncate"
|
||||
title={artifact.label}
|
||||
>
|
||||
{artifact.label}
|
||||
</span>
|
||||
</Button>
|
||||
</div>
|
||||
))}
|
||||
|
||||
@@ -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;
|
||||
|
||||
@@ -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) == []
|
||||
Reference in new issue
Block a user