mirror of
https://github.com/dograh-hq/dograh.git
synced 2026-06-22 08:38:13 +02:00
fix: disable duplicate trigger nodes in workflow builder (#402)
* fix: disable duplicate trigger nodes in workflow builder AddNodePanel: disable trigger buttons and show tooltip when a trigger already exists on the canvas, using bySpecName to identify trigger- category specs from the live node list. useWorkflowState: preflight in saveWorkflow rejects saves with multiple trigger nodes via a sonner toast before the network request is made. text_chat_session_service: include the original exception message in TextChatSessionExecutionError so the HTTP 500 detail surfaces the root cause without DB inspection. Closes #378 * style: format test_text_chat_session_service.py with ruff * chore: retrigger CI checks * fix(workflow): enforce node instance constraints --------- Co-authored-by: Abhishek Kumar <abhishek@a6k.me>
This commit is contained in:
parent
7c31dd3eec
commit
7d053320df
27 changed files with 591 additions and 91 deletions
23
api/tests/dto_fixtures/multiple_global_nodes.json
Normal file
23
api/tests/dto_fixtures/multiple_global_nodes.json
Normal file
|
|
@ -0,0 +1,23 @@
|
|||
{
|
||||
"nodes": [
|
||||
{
|
||||
"id": "s1",
|
||||
"type": "startCall",
|
||||
"position": {"x": 0, "y": 0},
|
||||
"data": {"name": "Start", "prompt": "Greet.", "is_start": true}
|
||||
},
|
||||
{
|
||||
"id": "g1",
|
||||
"type": "globalNode",
|
||||
"position": {"x": 0, "y": 200},
|
||||
"data": {"name": "Global A", "prompt": "Use a calm tone."}
|
||||
},
|
||||
{
|
||||
"id": "g2",
|
||||
"type": "globalNode",
|
||||
"position": {"x": 0, "y": 400},
|
||||
"data": {"name": "Global B", "prompt": "Keep answers short."}
|
||||
}
|
||||
],
|
||||
"edges": []
|
||||
}
|
||||
23
api/tests/dto_fixtures/multiple_trigger_nodes.json
Normal file
23
api/tests/dto_fixtures/multiple_trigger_nodes.json
Normal file
|
|
@ -0,0 +1,23 @@
|
|||
{
|
||||
"nodes": [
|
||||
{
|
||||
"id": "s1",
|
||||
"type": "startCall",
|
||||
"position": {"x": 0, "y": 0},
|
||||
"data": {"name": "Start", "prompt": "Greet.", "is_start": true}
|
||||
},
|
||||
{
|
||||
"id": "t1",
|
||||
"type": "trigger",
|
||||
"position": {"x": 0, "y": 200},
|
||||
"data": {"name": "Trigger A", "trigger_path": "trigger_a"}
|
||||
},
|
||||
{
|
||||
"id": "t2",
|
||||
"type": "trigger",
|
||||
"position": {"x": 0, "y": 400},
|
||||
"data": {"name": "Trigger B", "trigger_path": "trigger_b"}
|
||||
}
|
||||
],
|
||||
"edges": []
|
||||
}
|
||||
66
api/tests/test_mcp_create_workflow.py
Normal file
66
api/tests/test_mcp_create_workflow.py
Normal file
|
|
@ -0,0 +1,66 @@
|
|||
from unittest.mock import AsyncMock, MagicMock, patch
|
||||
|
||||
import pytest
|
||||
|
||||
from api.mcp_server.tools.create_workflow import create_workflow
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_create_workflow_rejects_duplicate_api_triggers():
|
||||
user = MagicMock()
|
||||
user.id = 1
|
||||
user.selected_organization_id = 1
|
||||
payload = {
|
||||
"nodes": [
|
||||
{
|
||||
"id": "start-1",
|
||||
"type": "startCall",
|
||||
"position": {"x": 0, "y": 0},
|
||||
"data": {"name": "Start", "prompt": "Greet."},
|
||||
},
|
||||
{
|
||||
"id": "trigger-1",
|
||||
"type": "trigger",
|
||||
"position": {"x": 0, "y": 200},
|
||||
"data": {"name": "Trigger A", "trigger_path": "support_west"},
|
||||
},
|
||||
{
|
||||
"id": "trigger-2",
|
||||
"type": "trigger",
|
||||
"position": {"x": 0, "y": 400},
|
||||
"data": {"name": "Trigger B", "trigger_path": "support_east"},
|
||||
},
|
||||
],
|
||||
"edges": [],
|
||||
}
|
||||
|
||||
with (
|
||||
patch(
|
||||
"api.mcp_server.tools.create_workflow.authenticate_mcp_request",
|
||||
AsyncMock(return_value=user),
|
||||
),
|
||||
patch(
|
||||
"api.mcp_server.tools.create_workflow.parse_code",
|
||||
AsyncMock(
|
||||
return_value={
|
||||
"ok": True,
|
||||
"workflowName": "duplicate-trigger-test",
|
||||
"workflow": payload,
|
||||
}
|
||||
),
|
||||
),
|
||||
patch(
|
||||
"api.mcp_server.tools.create_workflow.reconcile_positions",
|
||||
return_value=payload,
|
||||
),
|
||||
patch(
|
||||
"api.mcp_server.tools.create_workflow.db_client.create_workflow",
|
||||
AsyncMock(),
|
||||
) as create_mock,
|
||||
):
|
||||
result = await create_workflow(code="ignored")
|
||||
|
||||
assert result["created"] is False
|
||||
assert result["error_code"] == "graph_validation"
|
||||
assert "at most one API Trigger" in result["error"]
|
||||
create_mock.assert_not_awaited()
|
||||
|
|
@ -244,6 +244,58 @@ const only = wf.addTyped(endCall({ name: "only", prompt: "bye" }));
|
|||
update_mock.assert_not_awaited()
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_graph_validation_catches_duplicate_api_triggers(mock_backends):
|
||||
save_mock, update_mock = mock_backends
|
||||
payload = {
|
||||
"nodes": [
|
||||
{
|
||||
"id": "start-1",
|
||||
"type": "startCall",
|
||||
"position": {"x": 0, "y": 0},
|
||||
"data": {"name": "Start", "prompt": "Greet."},
|
||||
},
|
||||
{
|
||||
"id": "trigger-1",
|
||||
"type": "trigger",
|
||||
"position": {"x": 0, "y": 200},
|
||||
"data": {"name": "Trigger A", "trigger_path": "support_west"},
|
||||
},
|
||||
{
|
||||
"id": "trigger-2",
|
||||
"type": "trigger",
|
||||
"position": {"x": 0, "y": 400},
|
||||
"data": {"name": "Trigger B", "trigger_path": "support_east"},
|
||||
},
|
||||
],
|
||||
"edges": [],
|
||||
}
|
||||
|
||||
with (
|
||||
patch(
|
||||
"api.mcp_server.tools.save_workflow.parse_code",
|
||||
AsyncMock(
|
||||
return_value={
|
||||
"ok": True,
|
||||
"workflowName": _FakeWorkflowModel.name,
|
||||
"workflow": payload,
|
||||
}
|
||||
),
|
||||
),
|
||||
patch(
|
||||
"api.mcp_server.tools.save_workflow.reconcile_positions",
|
||||
return_value=payload,
|
||||
),
|
||||
):
|
||||
result = await save_workflow(workflow_id=1, code="ignored")
|
||||
|
||||
assert result["saved"] is False
|
||||
assert result["error_code"] == "graph_validation"
|
||||
assert "at most one API Trigger" in result["error"]
|
||||
save_mock.assert_not_awaited()
|
||||
update_mock.assert_not_awaited()
|
||||
|
||||
|
||||
# ─── Workflow not found / unauthorized ───────────────────────────────────
|
||||
|
||||
|
||||
|
|
|
|||
|
|
@ -414,4 +414,9 @@ def test_to_mcp_dict_retains_authoring_signal_startcall():
|
|||
]
|
||||
|
||||
# graph_constraints drops its null sub-fields.
|
||||
assert projected["graph_constraints"] == {"min_incoming": 0, "max_incoming": 0}
|
||||
assert projected["graph_constraints"] == {
|
||||
"min_incoming": 0,
|
||||
"max_incoming": 0,
|
||||
"min_instances": 1,
|
||||
"max_instances": 1,
|
||||
}
|
||||
|
|
|
|||
|
|
@ -42,15 +42,15 @@ async def test_whole_call_qa_tolerates_array_llm_response():
|
|||
"resolve_llm_config",
|
||||
new=AsyncMock(return_value=("openai", "gpt-4o", "sk-test", {})),
|
||||
),
|
||||
patch.object(qa_analysis, "create_llm_service_from_provider", return_value=object()),
|
||||
patch.object(
|
||||
qa_analysis, "create_llm_service_from_provider", return_value=object()
|
||||
),
|
||||
patch.object(
|
||||
qa_analysis,
|
||||
"_run_llm_inference",
|
||||
new=AsyncMock(return_value='["tag1", "tag2"]'),
|
||||
),
|
||||
patch.object(
|
||||
qa_analysis, "setup_langfuse_parent_context", return_value=None
|
||||
),
|
||||
patch.object(qa_analysis, "setup_langfuse_parent_context", return_value=None),
|
||||
patch.object(qa_analysis, "add_qa_span_to_trace", return_value=None),
|
||||
patch.object(qa_analysis.logger, "warning", warning_mock),
|
||||
):
|
||||
|
|
|
|||
|
|
@ -9,6 +9,7 @@ from api.services.workflow.text_chat_session_service import (
|
|||
TextChatTurnNotFoundError,
|
||||
_reload_text_chat_session,
|
||||
build_pending_text_chat_turn,
|
||||
execute_pending_text_chat_turn,
|
||||
truncate_text_chat_future_turns,
|
||||
validate_text_chat_turn_cursor,
|
||||
)
|
||||
|
|
@ -77,6 +78,36 @@ async def test_reload_text_chat_session_uses_run_id_to_resolve_organization(
|
|||
get_text_session.assert_awaited_once_with(123, organization_id=77)
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_execute_pending_turn_surfaces_original_exception_message(monkeypatch):
|
||||
session = WorkflowRunTextSessionModel(workflow_run_id=42)
|
||||
session.session_data = {
|
||||
"turns": [{"id": "turn-1", "status": "pending"}],
|
||||
"cursor_turn_id": "turn-1",
|
||||
}
|
||||
session.checkpoint = None
|
||||
|
||||
monkeypatch.setattr(
|
||||
text_chat_session_service,
|
||||
"execute_text_chat_pending_turn",
|
||||
AsyncMock(side_effect=RuntimeError("Workflow has 2 start nodes")),
|
||||
)
|
||||
monkeypatch.setattr(
|
||||
text_chat_session_service,
|
||||
"_mark_pending_turn_failed",
|
||||
AsyncMock(),
|
||||
)
|
||||
|
||||
with pytest.raises(
|
||||
TextChatSessionExecutionError, match="Workflow has 2 start nodes"
|
||||
):
|
||||
await execute_pending_text_chat_turn(
|
||||
workflow_id=1,
|
||||
run_id=42,
|
||||
text_session=session,
|
||||
)
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_reload_text_chat_session_raises_when_run_organization_is_missing(
|
||||
monkeypatch,
|
||||
|
|
|
|||
|
|
@ -47,3 +47,38 @@ def test_create_workflow_rejects_invalid_trigger_path_before_db_write():
|
|||
assert detail["errors"][0]["field"] == "data.trigger_path"
|
||||
assert "single URL path segment" in detail["errors"][0]["message"]
|
||||
assert mock_db.mock_calls == []
|
||||
|
||||
|
||||
def test_create_workflow_rejects_duplicate_api_triggers_before_db_write():
|
||||
app = _make_test_app()
|
||||
client = TestClient(app)
|
||||
|
||||
with patch("api.routes.workflow.db_client") as mock_db:
|
||||
response = client.post(
|
||||
"/workflow/create/definition",
|
||||
json={
|
||||
"name": "Support Agent",
|
||||
"workflow_definition": {
|
||||
"nodes": [
|
||||
{
|
||||
"id": "trigger-1",
|
||||
"type": "trigger",
|
||||
"data": {"trigger_path": "support_west"},
|
||||
},
|
||||
{
|
||||
"id": "trigger-2",
|
||||
"type": "trigger",
|
||||
"data": {"trigger_path": "support_east"},
|
||||
},
|
||||
],
|
||||
"edges": [],
|
||||
},
|
||||
},
|
||||
)
|
||||
|
||||
assert response.status_code == 422
|
||||
detail = response.json()["detail"]
|
||||
assert detail["is_valid"] is False
|
||||
assert detail["errors"][0]["kind"] == "workflow"
|
||||
assert "at most one API Trigger" in detail["errors"][0]["message"]
|
||||
assert mock_db.mock_calls == []
|
||||
|
|
|
|||
|
|
@ -72,14 +72,24 @@ _SCENARIOS = [
|
|||
(
|
||||
"no_start_node",
|
||||
["no_start_node"],
|
||||
["Workflow has no start node"],
|
||||
["Workflow must have at least one Start Call"],
|
||||
),
|
||||
# Two startCall nodes — surfaced separately from no_start_node so
|
||||
# the editor can show a count-specific message.
|
||||
(
|
||||
"multiple_start_nodes",
|
||||
["multiple_start_nodes:2"],
|
||||
["Workflow has 2 start nodes"],
|
||||
["Workflow can have at most one Start Call"],
|
||||
),
|
||||
(
|
||||
"multiple_trigger_nodes",
|
||||
["max_instances_1:trigger:2"],
|
||||
["Workflow can have at most one API Trigger"],
|
||||
),
|
||||
(
|
||||
"multiple_global_nodes",
|
||||
["max_instances_1:globalNode:2"],
|
||||
["Workflow can have at most one Global Node"],
|
||||
),
|
||||
]
|
||||
|
||||
|
|
@ -122,3 +132,35 @@ def test_workflow_graph_rejects_violations(name, expected_graph_messages):
|
|||
assert any(expected in m for m in actual_messages), (
|
||||
f"Expected substring {expected!r} not found in graph errors: {actual_messages}"
|
||||
)
|
||||
|
||||
|
||||
def test_workflow_graph_can_skip_duplicate_api_trigger_check_for_runtime():
|
||||
raw, _ = _load("multiple_trigger_nodes")
|
||||
dto = ReactFlowDTO.model_validate_json(raw)
|
||||
|
||||
WorkflowGraph(dto, skip_instance_constraints_for={"trigger"})
|
||||
|
||||
|
||||
def test_workflow_graph_start_semantics_come_from_node_type_not_legacy_flag():
|
||||
dto = ReactFlowDTO.model_validate(
|
||||
{
|
||||
"nodes": [
|
||||
{
|
||||
"id": "start-1",
|
||||
"type": "startCall",
|
||||
"position": {"x": 0, "y": 0},
|
||||
"data": {
|
||||
"name": "Start",
|
||||
"prompt": "Greet.",
|
||||
"is_start": False,
|
||||
},
|
||||
}
|
||||
],
|
||||
"edges": [],
|
||||
}
|
||||
)
|
||||
|
||||
graph = WorkflowGraph(dto)
|
||||
|
||||
assert graph.start_node_id == "start-1"
|
||||
assert graph.nodes["start-1"].is_start is True
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue