mirror of
https://github.com/microsoft/agent-framework.git
synced 2026-06-16 21:04:09 +08:00
.NET: Fixing issue with invalid node Ids when visualizing dotnet workflows. (#4269)
* Fix Mermaid rendering errors in WorkflowVisualizer.ToMermaidString Fix two bugs in the Mermaid diagram output: 1. Use safe node aliases (node_0, node_1, ...) instead of raw executor IDs as Mermaid node identifiers. Raw IDs containing spaces, dots, or non-ASCII characters (e.g. Japanese) caused Mermaid parse errors. 2. Fix conditional edge arrow syntax from '.--> ' (invalid) to '.-> ' (valid Mermaid dotted arrow syntax). Fixes #1406 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Use recognizable sanitized IDs for Mermaid node identifiers\n\nReplace generic node_0/node_1 aliases with IDs derived from the original\nexecutor names. ASCII letters, digits, and underscores are preserved;\nother characters become underscores (collapsed, trimmed). Leading digits\nget an n_ prefix. Collisions are resolved with a numeric suffix.\n\nThis keeps node IDs readable in the Mermaid source while the display\nlabels continue to show the full original names." * Remove issue number references from test names and comments" * Address PR review feedback from Copilot\n\n- Add Throw.IfNull(id) guard to SanitizeMermaidNodeId\n- Add safety limit (10,000) to collision resolution loop\n- Restore missing edge assertions (middle1/middle2 --> end)\n- Fix comment to show actual sanitized ID (n_1_User_input)\n- Use stricter regex in Unicode test (must start with letter/underscore)" * Address second round of PR review feedback\n\n- Escape node display labels via EscapeMermaidLabel to handle quotes,\n brackets, and newlines in executor IDs\n- Fix XML doc on SanitizeMermaidNodeId to accurately describe that\n existing consecutive underscores in input are preserved\n- Restore specific edge assertion (mid --> end) in conditional edge test\n- Restore fan-in routing assertions (s1/s2 through intermediate node,\n no direct edges to t) in fan-in test" --------- Co-authored-by: alliscode <bentho@microsoft.com> Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
This commit is contained in:
committed by
GitHub
Unverified
parent
7d374f00bb
commit
db48f8ce09
@@ -153,18 +153,52 @@ public static class WorkflowVisualizer
|
||||
|
||||
private static void EmitWorkflowMermaid(Workflow workflow, List<string> lines, string indent, string? ns = null)
|
||||
{
|
||||
string MapId(string id) => ns != null ? $"{ns}/{id}" : id;
|
||||
// Build a mapping from raw IDs to Mermaid-safe node aliases that preserve
|
||||
// as much of the original ID as possible for readability.
|
||||
// Mermaid node IDs cannot contain spaces, dots, pipes, or most special characters.
|
||||
var aliasMap = new Dictionary<string, string>();
|
||||
var usedAliases = new HashSet<string>(StringComparer.Ordinal);
|
||||
|
||||
string GetSafeId(string id)
|
||||
{
|
||||
var key = ns != null ? $"{ns}/{id}" : id;
|
||||
if (!aliasMap.TryGetValue(key, out var alias))
|
||||
{
|
||||
alias = SanitizeMermaidNodeId(key);
|
||||
|
||||
// Handle collisions by appending a numeric suffix
|
||||
if (!usedAliases.Add(alias))
|
||||
{
|
||||
var i = 2;
|
||||
while (!usedAliases.Add($"{alias}_{i}"))
|
||||
{
|
||||
if (i >= 10_000)
|
||||
{
|
||||
throw new InvalidOperationException($"Unable to generate a unique Mermaid node ID for '{key}'.");
|
||||
}
|
||||
|
||||
i++;
|
||||
}
|
||||
|
||||
alias = $"{alias}_{i}";
|
||||
}
|
||||
|
||||
aliasMap[key] = alias;
|
||||
}
|
||||
|
||||
return alias;
|
||||
}
|
||||
|
||||
// Add start node
|
||||
var startExecutorId = workflow.StartExecutorId;
|
||||
lines.Add($"{indent}{MapId(startExecutorId)}[\"{startExecutorId} (Start)\"];");
|
||||
lines.Add($"{indent}{GetSafeId(startExecutorId)}[\"{EscapeMermaidLabel(startExecutorId)} (Start)\"];");
|
||||
|
||||
// Add other executor nodes
|
||||
foreach (var executorId in workflow.ExecutorBindings.Keys)
|
||||
{
|
||||
if (executorId != startExecutorId)
|
||||
{
|
||||
lines.Add($"{indent}{MapId(executorId)}[\"{executorId}\"];");
|
||||
lines.Add($"{indent}{GetSafeId(executorId)}[\"{EscapeMermaidLabel(executorId)}\"];");
|
||||
}
|
||||
}
|
||||
|
||||
@@ -175,7 +209,7 @@ public static class WorkflowVisualizer
|
||||
lines.Add("");
|
||||
foreach (var (nodeId, _, _) in fanInDescriptors)
|
||||
{
|
||||
lines.Add($"{indent}{MapId(nodeId)}((fan-in))");
|
||||
lines.Add($"{indent}{GetSafeId(nodeId)}((fan-in))");
|
||||
}
|
||||
}
|
||||
|
||||
@@ -184,9 +218,9 @@ public static class WorkflowVisualizer
|
||||
{
|
||||
foreach (var src in sources)
|
||||
{
|
||||
lines.Add($"{indent}{MapId(src)} --> {MapId(nodeId)};");
|
||||
lines.Add($"{indent}{GetSafeId(src)} --> {GetSafeId(nodeId)};");
|
||||
}
|
||||
lines.Add($"{indent}{MapId(nodeId)} --> {MapId(target)};");
|
||||
lines.Add($"{indent}{GetSafeId(nodeId)} --> {GetSafeId(target)};");
|
||||
}
|
||||
|
||||
// Emit normal edges
|
||||
@@ -197,17 +231,17 @@ public static class WorkflowVisualizer
|
||||
string effectiveLabel = label != null ? EscapeMermaidLabel(label) : "conditional";
|
||||
|
||||
// Conditional edge, with user label or default
|
||||
lines.Add($"{indent}{MapId(src)} -. {effectiveLabel} .--> {MapId(target)};");
|
||||
lines.Add($"{indent}{GetSafeId(src)} -. {effectiveLabel} .-> {GetSafeId(target)};");
|
||||
}
|
||||
else if (label != null)
|
||||
{
|
||||
// Regular edge with label
|
||||
lines.Add($"{indent}{MapId(src)} -->|{EscapeMermaidLabel(label)}| {MapId(target)};");
|
||||
lines.Add($"{indent}{GetSafeId(src)} -->|{EscapeMermaidLabel(label)}| {GetSafeId(target)};");
|
||||
}
|
||||
else
|
||||
{
|
||||
// Regular edge without label
|
||||
lines.Add($"{indent}{MapId(src)} --> {MapId(target)};");
|
||||
lines.Add($"{indent}{GetSafeId(src)} --> {GetSafeId(target)};");
|
||||
}
|
||||
}
|
||||
}
|
||||
@@ -301,6 +335,50 @@ public static class WorkflowVisualizer
|
||||
return false;
|
||||
}
|
||||
|
||||
/// <summary>
|
||||
/// Converts a raw node ID into a Mermaid-safe identifier that preserves as much
|
||||
/// of the original text as possible. ASCII letters, digits, and underscores are kept
|
||||
/// as-is (including existing consecutive underscores). All other characters (including
|
||||
/// non-ASCII letters) are replaced with underscores, with consecutive invalid characters
|
||||
/// collapsed into a single underscore. A leading digit gets a prefix.
|
||||
/// </summary>
|
||||
private static string SanitizeMermaidNodeId(string id)
|
||||
{
|
||||
Throw.IfNull(id);
|
||||
|
||||
var sb = new StringBuilder(id.Length);
|
||||
bool lastWasUnderscore = false;
|
||||
foreach (var ch in id)
|
||||
{
|
||||
bool isAsciiSafe = (ch >= 'a' && ch <= 'z') || (ch >= 'A' && ch <= 'Z') || (ch >= '0' && ch <= '9') || ch == '_';
|
||||
if (isAsciiSafe)
|
||||
{
|
||||
sb.Append(ch);
|
||||
lastWasUnderscore = ch == '_';
|
||||
}
|
||||
else if (!lastWasUnderscore)
|
||||
{
|
||||
sb.Append('_');
|
||||
lastWasUnderscore = true;
|
||||
}
|
||||
}
|
||||
|
||||
// Trim trailing underscore
|
||||
while (sb.Length > 0 && sb[sb.Length - 1] == '_')
|
||||
{
|
||||
sb.Length--;
|
||||
}
|
||||
|
||||
// Mermaid IDs must not start with a digit
|
||||
if (sb.Length > 0 && sb[0] >= '0' && sb[0] <= '9')
|
||||
{
|
||||
sb.Insert(0, "n_");
|
||||
}
|
||||
|
||||
// Guard against empty result (e.g. id was all special chars)
|
||||
return sb.Length == 0 ? "node" : sb.ToString();
|
||||
}
|
||||
|
||||
// Helper method to escape special characters in DOT labels
|
||||
private static string EscapeDotLabel(string label)
|
||||
{
|
||||
|
||||
@@ -292,11 +292,14 @@ public class WorkflowVisualizerTests
|
||||
|
||||
var mermaidContent = workflow.ToMermaidString();
|
||||
|
||||
// Conditional edge should be dotted with label
|
||||
mermaidContent.Should().Contain("start -. conditional .--> mid");
|
||||
// Non-conditional edge should be solid
|
||||
// Conditional edge should be dotted with label (using .-> not .-->)
|
||||
mermaidContent.Should().Contain("-. conditional .-> ");
|
||||
// Non-conditional edge should be a specific solid arrow
|
||||
mermaidContent.Should().Contain("mid --> end");
|
||||
mermaidContent.Should().NotContain("end -. conditional");
|
||||
// Display labels should be present
|
||||
mermaidContent.Should().Contain("\"start (Start)\"");
|
||||
mermaidContent.Should().Contain("\"mid\"");
|
||||
mermaidContent.Should().Contain("\"end\"");
|
||||
}
|
||||
|
||||
[Fact]
|
||||
@@ -320,7 +323,7 @@ public class WorkflowVisualizerTests
|
||||
var fanInLines = Array.FindAll(lines, line => line.Contains("((fan-in))"));
|
||||
fanInLines.Should().HaveCount(1);
|
||||
|
||||
// Extract the intermediate node id from the line
|
||||
// Extract the intermediate fan-in node id from the line
|
||||
var fanInLine = fanInLines[0].Trim();
|
||||
var fanInNodeId = fanInLine.Substring(0, fanInLine.IndexOf("((fan-in))", StringComparison.Ordinal)).Trim();
|
||||
fanInNodeId.Should().NotBeNullOrEmpty();
|
||||
@@ -333,6 +336,24 @@ public class WorkflowVisualizerTests
|
||||
// Ensure direct edges are not present
|
||||
mermaidContent.Should().NotContain("s1 --> t");
|
||||
mermaidContent.Should().NotContain("s2 --> t");
|
||||
|
||||
// Display labels should be present
|
||||
mermaidContent.Should().Contain("\"start (Start)\"");
|
||||
mermaidContent.Should().Contain("\"s1\"");
|
||||
mermaidContent.Should().Contain("\"s2\"");
|
||||
mermaidContent.Should().Contain("\"t\"");
|
||||
|
||||
// All node IDs should be safe aliases (ASCII-only identifiers)
|
||||
foreach (var line in mermaidContent.Split('\n'))
|
||||
{
|
||||
var trimmed = line.Trim();
|
||||
if (trimmed.Contains("[\"") || trimmed.Contains("(("))
|
||||
{
|
||||
var bracketIdx = trimmed.IndexOfAny(['[', '(']);
|
||||
var nodeId = trimmed.Substring(0, bracketIdx);
|
||||
nodeId.Should().MatchRegex("^[a-zA-Z_][a-zA-Z0-9_]*$");
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
[Fact]
|
||||
@@ -353,13 +374,14 @@ public class WorkflowVisualizerTests
|
||||
|
||||
var mermaidContent = workflow.ToMermaidString();
|
||||
|
||||
// Check all executors are present
|
||||
mermaidContent.Should().Contain("start[\"start (Start)\"]");
|
||||
mermaidContent.Should().Contain("middle1[\"middle1\"]");
|
||||
mermaidContent.Should().Contain("middle2[\"middle2\"]");
|
||||
mermaidContent.Should().Contain("end[\"end\"]");
|
||||
// Check display labels are present
|
||||
mermaidContent.Should().Contain("\"start (Start)\"");
|
||||
mermaidContent.Should().Contain("\"middle1\"");
|
||||
mermaidContent.Should().Contain("\"middle2\"");
|
||||
mermaidContent.Should().Contain("\"end\"");
|
||||
|
||||
// Check all edges are present
|
||||
// Check that sanitized IDs are used and all edges connect them
|
||||
mermaidContent.Should().Contain("start[\"start (Start)\"]");
|
||||
mermaidContent.Should().Contain("start --> middle1");
|
||||
mermaidContent.Should().Contain("start --> middle2");
|
||||
mermaidContent.Should().Contain("middle1 --> end");
|
||||
@@ -386,15 +408,19 @@ public class WorkflowVisualizerTests
|
||||
|
||||
var mermaidContent = workflow.ToMermaidString();
|
||||
|
||||
// Check conditional edge
|
||||
mermaidContent.Should().Contain("start -. conditional .--> a");
|
||||
|
||||
// Check fan-out edges
|
||||
mermaidContent.Should().Contain("a --> b");
|
||||
mermaidContent.Should().Contain("a --> c");
|
||||
// Check conditional edge uses correct syntax (.-> not .-->)
|
||||
mermaidContent.Should().Contain("-. conditional .->");
|
||||
mermaidContent.Should().NotContain(".-->");
|
||||
|
||||
// Check fan-in (should have intermediate node)
|
||||
mermaidContent.Should().Contain("((fan-in))");
|
||||
|
||||
// Display labels should be present
|
||||
mermaidContent.Should().Contain("\"start (Start)\"");
|
||||
mermaidContent.Should().Contain("\"a\"");
|
||||
mermaidContent.Should().Contain("\"b\"");
|
||||
mermaidContent.Should().Contain("\"c\"");
|
||||
mermaidContent.Should().Contain("\"end\"");
|
||||
}
|
||||
|
||||
[Fact]
|
||||
@@ -411,7 +437,7 @@ public class WorkflowVisualizerTests
|
||||
var mermaidContent = workflow.ToMermaidString();
|
||||
|
||||
// Should escape pipe character
|
||||
mermaidContent.Should().Contain("start -->|High | Low Priority| end");
|
||||
mermaidContent.Should().Contain("-->|High | Low Priority|");
|
||||
// Should not contain unescaped pipe that would break syntax
|
||||
mermaidContent.Should().NotContain("-->|High | Low");
|
||||
}
|
||||
@@ -453,4 +479,88 @@ public class WorkflowVisualizerTests
|
||||
// Should not contain literal newline in the label (but the overall output has newlines between statements)
|
||||
mermaidContent.Should().NotContain("Line 1\nLine 2");
|
||||
}
|
||||
|
||||
[Fact]
|
||||
public void Test_WorkflowViz_Mermaid_ConditionalEdge_ArrowSyntax()
|
||||
{
|
||||
// Conditional edges must use "-. label .->" (not ".-->") which is the correct
|
||||
// Mermaid syntax for dotted arrows with labels.
|
||||
var start = new MockExecutor("start");
|
||||
var mid = new MockExecutor("mid");
|
||||
|
||||
static bool Condition(string? msg) => msg == "foo";
|
||||
|
||||
var workflow = new WorkflowBuilder("start")
|
||||
.AddEdge<string>(start, mid, Condition)
|
||||
.Build();
|
||||
|
||||
var mermaidContent = workflow.ToMermaidString();
|
||||
|
||||
// The output should use ".->" not ".-->" for conditional (dotted) edges
|
||||
mermaidContent.Should().NotContain(".-->", because: "'.-->' is invalid Mermaid syntax for dotted arrows; should be '.->'");
|
||||
mermaidContent.Should().Contain("-. conditional .->", because: "'-. label .->' is the correct Mermaid syntax for dotted arrows with labels");
|
||||
}
|
||||
|
||||
[Fact]
|
||||
public void Test_WorkflowViz_Mermaid_IdentifiersWithSpaces()
|
||||
{
|
||||
// Identifiers with spaces must not be used directly as Mermaid node IDs
|
||||
// because spaces cause rendering errors.
|
||||
var executor1 = new MockExecutor("1. User input");
|
||||
var executor2 = new MockExecutor("2. Process data");
|
||||
|
||||
var workflow = new WorkflowBuilder("1. User input")
|
||||
.AddEdge(executor1, executor2)
|
||||
.Build();
|
||||
|
||||
var mermaidContent = workflow.ToMermaidString();
|
||||
|
||||
// Node definitions should use safe aliases as IDs (no spaces), with display names in quotes
|
||||
// Bad: '1. User input["1. User input (Start)"]' — spaces in ID break Mermaid
|
||||
// Good: 'n_1_User_input["1. User input (Start)"]' — alias ID is safe and sanitized
|
||||
|
||||
// Each node definition line (containing ["..."]) should have a space-free ID before the bracket
|
||||
foreach (var line in mermaidContent.Split('\n'))
|
||||
{
|
||||
var trimmed = line.Trim();
|
||||
if (trimmed.Contains("[\""))
|
||||
{
|
||||
var bracketIdx = trimmed.IndexOf('[');
|
||||
var nodeId = trimmed.Substring(0, bracketIdx);
|
||||
nodeId.Should().NotContain(" ", because: $"Mermaid node IDs must not contain spaces, but got '{nodeId}'");
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
[Fact]
|
||||
public void Test_WorkflowViz_Mermaid_IdentifiersWithUnicode()
|
||||
{
|
||||
// Non-ASCII characters (e.g. Japanese) in identifiers cause Mermaid rendering errors.
|
||||
var executor1 = new MockExecutor("ユーザー入力");
|
||||
var executor2 = new MockExecutor("データ処理");
|
||||
|
||||
var workflow = new WorkflowBuilder("ユーザー入力")
|
||||
.AddEdge(executor1, executor2)
|
||||
.Build();
|
||||
|
||||
var mermaidContent = workflow.ToMermaidString();
|
||||
|
||||
// The display labels should contain the original names
|
||||
mermaidContent.Should().Contain("ユーザー入力");
|
||||
mermaidContent.Should().Contain("データ処理");
|
||||
|
||||
// But node IDs (before the bracket) should be safe ASCII-only identifiers
|
||||
foreach (var line in mermaidContent.Split('\n'))
|
||||
{
|
||||
var trimmed = line.Trim();
|
||||
if (trimmed.Contains("[\""))
|
||||
{
|
||||
var bracketIdx = trimmed.IndexOf('[');
|
||||
var nodeId = trimmed.Substring(0, bracketIdx);
|
||||
// Node ID should start with a letter or underscore, followed by ASCII alphanumeric or underscores
|
||||
nodeId.Should().MatchRegex("^[a-zA-Z_][a-zA-Z0-9_]*$",
|
||||
because: $"Mermaid node IDs should be ASCII-safe, but got '{nodeId}'");
|
||||
}
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user