diff --git a/application/api/user/agents/portability.py b/application/api/user/agents/portability.py index 2bd2f563..00b5888d 100644 --- a/application/api/user/agents/portability.py +++ b/application/api/user/agents/portability.py @@ -888,6 +888,38 @@ def _tool_key(index: int) -> str: # --------------------------------------------------------------------------- +def _plan_workflow_removal(conn, user: str, target: dict) -> Optional[dict]: + """Describe what an explicit ``workflow: null`` would destroy, if anything. + + Returns a ``delete`` block sized with the current graph, or None when the + workflow survives — the target is new, published (``_apply_workflow`` + keeps a published agent's graph), has no workflow, or another agent still + references it. + """ + if target.get("action") != "update" or not target.get("agent_id"): + return None + if (target.get("status") or "") == "published": + return None + agent_row = AgentsRepository(conn).get(str(target["agent_id"]), user) or {} + existing_id = agent_row.get("workflow_id") + if not existing_id: + return None + row = WorkflowsRepository(conn).get(str(existing_id), user) + if row is None: + return None + if AgentsRepository(conn).count_by_workflow(str(existing_id), user) > 1: + return None + # Lazy import, same as _apply_workflow — keeps route modules decoupled at load. + from application.api.user.workflows.routes import get_workflow_graph_version + + version = get_workflow_graph_version(row) + return { + "action": "delete", + "nodes": len(WorkflowNodesRepository(conn).find_by_version(str(row["id"]), version)), + "edges": len(WorkflowEdgesRepository(conn).find_by_version(str(row["id"]), version)), + } + + def plan_import(conn, user: str, doc: dict) -> dict: """Resolve every reference without writing; returns a resolution report.""" spec = doc["spec"] @@ -978,18 +1010,25 @@ def plan_import(conn, user: str, doc: dict) -> dict: workflow_plan = None wf_spec = spec.get("workflow") - if spec.get("agent_type") == "workflow" and isinstance(wf_spec, dict): - action = "create" - if target.get("action") == "update" and target.get("agent_id"): - agent_row = AgentsRepository(conn).get(str(target["agent_id"]), user) - existing_wf = (agent_row or {}).get("workflow_id") - if existing_wf and WorkflowsRepository(conn).get(str(existing_wf), user): - action = "update" - workflow_plan = { - "nodes": len(wf_spec.get("nodes") or []), - "edges": len(wf_spec.get("edges") or []), - "action": action, - } + if spec.get("agent_type") == "workflow": + if isinstance(wf_spec, dict): + action = "create" + if target.get("action") == "update" and target.get("agent_id"): + agent_row = AgentsRepository(conn).get(str(target["agent_id"]), user) + existing_wf = (agent_row or {}).get("workflow_id") + if existing_wf and WorkflowsRepository(conn).get(str(existing_wf), user): + action = "update" + workflow_plan = { + "nodes": len(wf_spec.get("nodes") or []), + "edges": len(wf_spec.get("edges") or []), + "action": action, + } + elif "workflow" in spec: + # Explicit ``workflow: null`` (vs. an omitted key, which changes + # nothing). Apply clears the link and reaps the row when no other + # agent references it, destroying the graph, its run history and + # its artifacts — the dry-run has to say so before the user commits. + workflow_plan = _plan_workflow_removal(conn, user, target) return { "target": target, @@ -1029,7 +1068,19 @@ def _apply_sources(conn, user: str, spec: dict, resolution: dict, warnings: list mapping = {} resolved: list[str] = [] id_by_name: dict[str, Optional[str]] = {} - for src in spec.get("sources") or []: + entries = [src for src in spec.get("sources") or [] if isinstance(src, dict)] + # ``id_by_name`` (and a workflow node's source reference) keys on the name + # alone, but names aren't unique per user — ``find_by_name`` returns the + # oldest match, so duplicates collapse onto one source. Same hazard the + # custom-model display names carry below; surface it rather than silently + # rewiring a node to a different source. + names = [src.get("name") or "" for src in entries] + for dup in sorted({n for n in names if n and names.count(n) > 1}): + warnings.append( + f"Multiple sources share the name '{dup}'; references to it all " + "resolve to the oldest one" + ) + for src in entries: name = src.get("name") or "" id_by_name.setdefault(name, None) mapped = mapping.get(name) @@ -1338,7 +1389,9 @@ def _rewrite_node_refs( resolved_sources.append(ref) else: warnings.append(f"Workflow node source '{ref}' not resolved; removed") - cfg["sources"] = resolved_sources + # Distinct names can collapse onto one source (see ``_apply_sources``), so + # dedupe rather than storing the same id twice on the node. + cfg["sources"] = list(dict.fromkeys(resolved_sources)) raw_model = cfg.get("model_id") if raw_model: @@ -1536,6 +1589,12 @@ def apply_import(conn, user: str, doc: dict, resolution: Optional[dict] = None) "extra_source_ids": [], } ) + prior_workflow_id = None + if is_update: + prior_workflow_id = ( + agents_repo.get(str(target["agent_id"]), user) or {} + ).get("workflow_id") + published = (target.get("status") or "") == "published" workflow_result = _apply_workflow( conn, user, @@ -1546,21 +1605,31 @@ def apply_import(conn, user: str, doc: dict, resolution: Optional[dict] = None) model_ids_by_name, warnings, ) - if workflow_result is not _WORKFLOW_UNCHANGED: - if workflow_result is None and (target.get("status") or "") == "published": - warnings.append( - "File has no workflow; the published agent kept its existing one" + # A published workflow agent with no graph can't run — the create and + # update routes both reject that state, so import must not install it + # through the back door (e.g. a file that flips ``agent_type`` to + # workflow over a published classic agent). + if workflow_result is _WORKFLOW_UNCHANGED: + if published and not prior_workflow_id: + raise AgentImportError( + "A published workflow agent needs a workflow; this file has none" ) - else: - authoritative["workflow_id"] = workflow_result - if workflow_result is None and is_update: - # Explicit ``workflow: null`` on a draft: remember the old - # link so the row can be reaped after the update — there is - # no workflow-list API independent of agents, so an - # unlinked workflow would be unreachable forever. - prior = agents_repo.get(str(target["agent_id"]), user) or {} - if prior.get("workflow_id"): - orphaned_workflow_id = str(prior["workflow_id"]) + elif workflow_result is None and published: + if not prior_workflow_id: + raise AgentImportError( + "A published workflow agent needs a workflow; this file has none" + ) + warnings.append( + "File has no workflow; the published agent kept its existing one" + ) + else: + authoritative["workflow_id"] = workflow_result + if workflow_result is None and prior_workflow_id: + # Explicit ``workflow: null`` on a draft: remember the old + # link so the row can be reaped after the update — there is + # no workflow-list API independent of agents, so an + # unlinked workflow would be unreachable forever. + orphaned_workflow_id = str(prior_workflow_id) # Optional fields — applied only when present so a partial file doesn't wipe them. optional = { @@ -1583,6 +1652,12 @@ def apply_import(conn, user: str, doc: dict, resolution: Optional[dict] = None) # Nothing references it anymore; delete (cascades nodes/edges # and reaps run artifacts) rather than stranding the row. WorkflowsRepository(conn).delete(orphaned_workflow_id, user) + # Destructive and irreversible — the graph, its run history and + # its artifacts (rows and stored bytes) all go. Say so. + warnings.append( + "File has no workflow; the agent's workflow was deleted " + "along with its run history and artifacts" + ) return { "agent_id": str(target["agent_id"]), "action": "updated", diff --git a/frontend/src/agents/AgentCard.tsx b/frontend/src/agents/AgentCard.tsx index 1e5ae660..64ae3dc3 100644 --- a/frontend/src/agents/AgentCard.tsx +++ b/frontend/src/agents/AgentCard.tsx @@ -17,12 +17,14 @@ import Trash from '../assets/red-trash.svg'; import ThreeDots from '../assets/three-dots.svg'; import UnPin from '../assets/unpin.svg'; import { Avatar } from '../components/ui/avatar'; +import { Button } from '../components/ui/button'; import { DropdownMenu, DropdownMenuContent, DropdownMenuItem, DropdownMenuTrigger, } from '../components/ui/dropdown-menu'; +import { Modal } from '../components/ui/modal'; import ConfirmationModal from '../modals/ConfirmationModal'; import MoveToFolderModal from '../modals/MoveToFolderModal'; import { ActiveState } from '../models/misc'; @@ -67,6 +69,7 @@ export default function AgentCard({ useState('INACTIVE'); const [moveModalState, setMoveModalState] = useState('INACTIVE'); const [shareModalOpen, setShareModalOpen] = useState(false); + const [exportError, setExportError] = useState(null); const menuOptionsConfig: Record = { template: [ @@ -302,7 +305,12 @@ export default function AgentCard({ .json() .then((data) => data?.message) .catch(() => null); - throw new Error(message || t('agents.exportAgentFailed')); + // Server-side refusals (e.g. a workflow referencing too many + // resources) carry an actionable message; flag it so the catch can + // tell it apart from a transport error like "Failed to fetch". + const error = new Error(message || t('agents.exportAgentFailed')); + error.name = 'ExportRefused'; + throw error; } const yamlText = await response.text(); const blob = new Blob([yamlText], { type: 'application/x-yaml' }); @@ -316,8 +324,10 @@ export default function AgentCard({ URL.revokeObjectURL(url); } catch (error) { console.error('Error:', error); - alert( - error instanceof Error ? error.message : t('agents.exportAgentFailed'), + setExportError( + error instanceof Error && error.name === 'ExportRefused' + ? error.message + : t('agents.exportAgentFailed'), ); } }; @@ -470,6 +480,25 @@ export default function AgentCard({ cancelLabel="Cancel" variant="danger" /> + { + if (!open) setExportError(null); + }} + title={t('agents.exportAgentFailed')} + size="sm" + footer={ + + } + > +

{exportError}

+ { - if (acceptedFiles[0]) processFile(acceptedFiles[0]); - }, - multiple: false, - // Declared here (not via getInputProps) so drag-and-drop is filtered - // too, with drag-over rejection feedback; processFile's extension check - // stays as a backstop for odd MIME reports. - accept: { - 'application/x-yaml': ['.yaml', '.yml'], - 'text/yaml': ['.yaml', '.yml'], - }, - }); + const { getRootProps, getInputProps, isDragActive, isDragReject } = + useDropzone({ + onDrop: (acceptedFiles: File[], fileRejections: FileRejection[]) => { + // A rejected file never reaches acceptedFiles, so without this the + // drop is a silent no-op and any previously picked file stays staged. + if (fileRejections.length > 0) { + setFileName(''); + setYamlText(''); + setError(t('modals.importAgent.invalidFileType')); + return; + } + if (acceptedFiles[0]) processFile(acceptedFiles[0]); + }, + multiple: false, + // Declared here (not via getInputProps) so drag-and-drop is filtered + // too; processFile's extension check stays as a backstop for odd MIME + // reports. + accept: { + 'application/x-yaml': ['.yaml', '.yml'], + 'text/yaml': ['.yaml', '.yml'], + }, + }); const handleAnalyze = async () => { if (!yamlText) return; @@ -320,7 +329,11 @@ export default function ImportAgentModal({
@@ -346,18 +359,28 @@ export default function ImportAgentModal({ : t('modals.importAgent.willCreate')}
- {plan.workflow && ( -

- - {plan.workflow.action === 'update' - ? t('modals.importAgent.workflowUpdate', { - nodes: plan.workflow.nodes, - }) - : t('modals.importAgent.workflowCreate', { - nodes: plan.workflow.nodes, - })} -

- )} + {plan.workflow && + (plan.workflow.action === 'delete' ? ( + // Irreversible: the graph, its run history and its artifacts + // all go. Warn rather than confirming with a green check. +

+ + {t('modals.importAgent.workflowDelete', { + nodes: plan.workflow.nodes, + })} +

+ ) : ( +

+ + {plan.workflow.action === 'update' + ? t('modals.importAgent.workflowUpdate', { + nodes: plan.workflow.nodes, + }) + : t('modals.importAgent.workflowCreate', { + nodes: plan.workflow.nodes, + })} +

+ ))} {plan.sources.length > 0 && (
diff --git a/frontend/src/modals/ImportSpecModal.tsx b/frontend/src/modals/ImportSpecModal.tsx index 2f636087..a5bee57b 100644 --- a/frontend/src/modals/ImportSpecModal.tsx +++ b/frontend/src/modals/ImportSpecModal.tsx @@ -1,5 +1,5 @@ import { useState } from 'react'; -import { useDropzone } from 'react-dropzone'; +import { type FileRejection, useDropzone } from 'react-dropzone'; import { useTranslation } from 'react-i18next'; import { useSelector } from 'react-redux'; @@ -73,19 +73,28 @@ export default function ImportSpecModal({ setParsedResult(null); }; - const { getRootProps, getInputProps, isDragActive } = useDropzone({ - onDrop: (acceptedFiles: File[]) => { - if (acceptedFiles[0]) processFile(acceptedFiles[0]); - }, - multiple: false, - // Declared here (not via getInputProps) so drag-and-drop is filtered - // too; processFile's extension check stays as a backstop. - accept: { - 'application/json': ['.json'], - 'application/x-yaml': ['.yaml', '.yml'], - 'text/yaml': ['.yaml', '.yml'], - }, - }); + const { getRootProps, getInputProps, isDragActive, isDragReject } = + useDropzone({ + onDrop: (acceptedFiles: File[], fileRejections: FileRejection[]) => { + // A rejected file never reaches acceptedFiles, so without this the + // drop is a silent no-op and any previously picked file stays staged. + if (fileRejections.length > 0) { + setFile(null); + setParsedResult(null); + setError(t('modals.importSpec.invalidFileType')); + return; + } + if (acceptedFiles[0]) processFile(acceptedFiles[0]); + }, + multiple: false, + // Declared here (not via getInputProps) so drag-and-drop is filtered + // too; processFile's extension check stays as a backstop. + accept: { + 'application/json': ['.json'], + 'application/x-yaml': ['.yaml', '.yml'], + 'text/yaml': ['.yaml', '.yml'], + }, + }); const handleParse = async () => { if (!file) return; @@ -209,7 +218,11 @@ export default function ImportSpecModal({
diff --git a/tests/api/test_workflow_portability.py b/tests/api/test_workflow_portability.py index 138d8858..68875846 100644 --- a/tests/api/test_workflow_portability.py +++ b/tests/api/test_workflow_portability.py @@ -408,3 +408,128 @@ def test_null_workflow_keeps_row_still_referenced_by_another_agent(pg_conn): assert updated["workflow_id"] is None # Still referenced — must not be deleted. assert WorkflowsRepository(pg_conn).get(str(wf["id"]), user) is not None + + +def test_publish_guard_rejects_workflow_agent_with_no_graph(pg_conn): + """A file that makes a published agent a graph-less workflow agent is refused. + + The create and update routes both reject that state; import must not + install it through the back door. + """ + user = "u_wf_publish_guard" + agent = AgentsRepository(pg_conn).create( + user, "Probe Bot", "published", agent_type="classic", slug="probe-bot" + ) + doc = { + "apiVersion": "docsgpt.arc53.com/v1", + "kind": "Agent", + "metadata": {"id": str(agent["id"]), "slug": "probe-bot"}, + "spec": {"name": "Probe Bot", "agent_type": "workflow"}, + } + + # Key omitted entirely. + with pytest.raises(AgentImportError, match="published workflow agent needs a workflow"): + apply_import(pg_conn, user, doc) + + # Explicit null, with no prior workflow to fall back on. + doc["spec"]["workflow"] = None + with pytest.raises(AgentImportError, match="published workflow agent needs a workflow"): + apply_import(pg_conn, user, doc) + + unchanged = AgentsRepository(pg_conn).get(str(agent["id"]), user) + assert unchanged["agent_type"] == "classic" + assert unchanged["status"] == "published" + + +def test_plan_distinguishes_absent_workflow_from_explicit_null(pg_conn): + """Omitting the key is a no-op; an explicit null destroys the graph. + + The two used to produce an identical plan, so the modal showed nothing + either way while the outcomes differed completely. + """ + user = "u_wf_plan_null" + agent, wf = _seed_workflow_agent(pg_conn, user) + AgentsRepository(pg_conn).update(str(agent["id"]), user, {"status": "draft"}) + doc = parse_agent_yaml(agent_to_yaml(serialize_agent(pg_conn, agent, user))) + + absent = parse_agent_yaml(agent_to_yaml(serialize_agent(pg_conn, agent, user))) + del absent["spec"]["workflow"] + assert plan_import(pg_conn, user, absent)["workflow"] is None + + doc["spec"]["workflow"] = None + assert plan_import(pg_conn, user, doc)["workflow"] == { + "action": "delete", + "nodes": 3, + "edges": 2, + } + + +def test_plan_reports_no_removal_when_workflow_survives(pg_conn): + """No delete block when the row lives on — published, or still referenced.""" + user = "u_wf_plan_keep" + agent, wf = _seed_workflow_agent(pg_conn, user) + doc = parse_agent_yaml(agent_to_yaml(serialize_agent(pg_conn, agent, user))) + doc["spec"]["workflow"] = None + + # Published: _apply_workflow keeps the existing graph. + assert plan_import(pg_conn, user, doc)["workflow"] is None + + # Draft, but a sibling agent still references the row. + AgentsRepository(pg_conn).update(str(agent["id"]), user, {"status": "draft"}) + AgentsRepository(pg_conn).create( + user, "Sibling", "draft", agent_type="workflow", workflow_id=str(wf["id"]) + ) + assert plan_import(pg_conn, user, doc)["workflow"] is None + + +def test_null_workflow_deletion_is_warned_about(pg_conn): + """The destructive reap must not be silent.""" + user = "u_wf_null_warn" + agent, wf = _seed_workflow_agent(pg_conn, user) + doc = parse_agent_yaml(agent_to_yaml(serialize_agent(pg_conn, agent, user))) + doc["spec"]["workflow"] = None + AgentsRepository(pg_conn).update(str(agent["id"]), user, {"status": "draft"}) + + result = apply_import(pg_conn, user, doc) + assert WorkflowsRepository(pg_conn).get(str(wf["id"]), user) is None + assert any("was deleted" in w and "run history" in w for w in result["warnings"]) + + +def test_duplicate_source_names_warn_and_dedupe(pg_conn): + """Two sources sharing a name collapse onto the oldest — say so.""" + user = "u_wf_dup_src" + repo = SourcesRepository(pg_conn) + older = repo.create("KB", user_id=user, type="file") + newer = repo.create("KB", user_id=user, type="file") + agent, _wf = _seed_workflow_agent(pg_conn, user, source_id=str(older["id"])) + + # Point the node at both same-named sources. + version = 1 + nodes_repo = WorkflowNodesRepository(pg_conn) + node = [n for n in nodes_repo.find_by_version(str(agent["workflow_id"]), version) + if n["node_type"] == "agent"][0] + config = dict(node["config"]) + config["sources"] = [str(older["id"]), str(newer["id"])] + nodes_repo.bulk_create( + str(agent["workflow_id"]), + version, + [{ + "node_id": node["node_id"], + "node_type": node["node_type"], + "title": node.get("title"), + "position": node.get("position"), + "config": config, + }], + ) + + doc = parse_agent_yaml(agent_to_yaml(serialize_agent(pg_conn, agent, user))) + assert [s["name"] for s in doc["spec"]["sources"]] == ["KB", "KB"] + + result = apply_import(pg_conn, user, doc) + assert any("share the name 'KB'" in w for w in result["warnings"]) + + updated = AgentsRepository(pg_conn).get(str(agent["id"]), user) + graph_nodes, _ = _graph(pg_conn, WorkflowsRepository(pg_conn).get(str(updated["workflow_id"]), user)) + agent_node = [n for n in graph_nodes if n["node_type"] == "agent"][0] + # Collapsed onto the oldest match, and not stored twice. + assert agent_node["config"]["sources"] == [str(older["id"])]