fix: stop masked values from landing on new nodes and edges

docs/mcp-server.md

@@ -104,7 +104,9 @@ before retrying a start whose response was lost.
 - On `publish_workflow`/`update_workflow_definition`/`apply_workflow_patch`, real secrets are
   **restored from the stored version** by node id (including masks nested in condition/case arrays)
   — the agent's `***` never overwrites them, and a
-  secret the agent invents on a new node is rejected (masked + noted).
+  secret the agent invents on a new node is rejected (masked + noted). A `***` with no stored
+  value behind it (for example a condition literal copied onto a new edge id) rejects the whole
+  operation, because it would otherwise be saved as the literal `***`.
 - Credentials carry no password field; secret global values arrive masked. Create/update accept
   secrets write-only.
 

src/NodePilot.Ai/Prompts/assistant-system.md

@@ -50,7 +50,10 @@ Rules:
    never rewrite or reconstruct it. Never invent a secret value and never set one — if
    a change would require a secret (a password, API key, webhook secret, connection
    string), leave it as `***` and tell the user in your reply to set it manually on the
-   node afterwards.
+   node afterwards. A `***` only stands for the value stored under the same node or edge
+   `id`. Never copy a `***` onto a new node or edge (a new `id`), for example when you
+   replace one edge with two: there is nothing to restore, so `***` would be saved
+   literally. Ask the user for the real value instead.
 4. Keep the definition structurally valid: unique ids, every edge `source`/`target`
    referencing a real node, the trigger preserved unless explicitly told otherwise.
 

src/NodePilot.Ai/WorkflowDefinitionMerge.cs

@@ -122,6 +122,8 @@ private static void MergeObject(JsonObject target, JsonObject? source, List<stri
             {
                 if (sourceChild is not null && !IsMask(sourceChild))
                     target[key] = sourceChild.DeepClone();
+                else if (sourceChild is null)
+                    NoteMaskWithoutOriginal(key, itemId, notes);
                 continue;
             }
 
@@ -166,6 +168,18 @@ private static void ReconcileNamedSecret(
         }
     }
 
+    // A mask with nothing to restore would be saved as the literal "***", for example a condition
+    // operand copied onto a new edge.
+    private static void NoteMaskWithoutOriginal(string key, string? itemId, List<string> notes)
+    {
+        if (WorkflowSecretKeys.SecretConfigKeys.Contains(key))
+        {
+            notes.Add($"Secret '{key}' bitte manuell am Node setzen — die KI darf keine Secrets vergeben.");
+            return;
+        }
+        notes.Add($"'{key}' von '{itemId ?? "?"}' enthält '***' ohne gespeicherten Originalwert — bitte den echten Wert im Designer setzen, sonst wird '***' wörtlich gespeichert.");
+    }
+
     // Null and the empty string count as "no secret set".
     private static bool HasValue(JsonNode? node)
         => node is not null && !(node is JsonValue value && value.TryGetValue(out string? text) && string.IsNullOrEmpty(text));
@@ -206,9 +220,9 @@ private static void MergeNestedArray(
                     MergeNestedArray(targetArray, sourceItem as JsonArray, notes, itemId, key);
                     break;
                 case JsonValue targetValue when targetValue.TryGetValue(out string? value)
-                                               && value == SecretMask
-                                               && sourceItem is not null:
-                    target[index] = sourceItem.DeepClone();
+                                               && value == SecretMask:
+                    if (sourceItem is not null) target[index] = sourceItem.DeepClone();
+                    else NoteMaskWithoutOriginal(key, itemId, notes);
                     break;
             }
         }

src/NodePilot.Mcp/Mapping/WorkflowDefinitionPatcher.cs

@@ -114,8 +114,9 @@ private static JsonArray MergeFullArray(JsonElement proposed, string arrayName,
         {
             if (item.ValueKind != JsonValueKind.Object || JsonNode.Parse(item.GetRawText()) is not JsonObject obj) continue;
             JsonObject? source = null;
-            if (IdOf(obj) is { } id) originalById.TryGetValue(id, out source);
-            MergeObject(obj, source, notes);
+            var id = IdOf(obj);
+            if (id is not null) originalById.TryGetValue(id, out source);
+            MergeObject(obj, source, notes, id);
             result.Add(obj);
         }
         return result;
@@ -137,7 +138,7 @@ private static void UpsertById(List<JsonObject> list, JsonObject proposed, List<
             ?? throw new ArgumentException("upsert item must have a non-empty string 'id'.");
         var existing = list.FirstOrDefault(x => IdOf(x) == id);
         var merged = (JsonObject)proposed.DeepClone();
-        MergeObject(merged, existing, notes);
+        MergeObject(merged, existing, notes, id);
         if (existing is null)
             list.Add(merged);
         else
@@ -146,7 +147,7 @@ private static void UpsertById(List<JsonObject> list, JsonObject proposed, List<
 
     // Backfill untouched fields from the original + protect secret keys (adapted from the API
     // merge).
-    private static void MergeObject(JsonObject target, JsonObject? source, List<string> notes)
+    private static void MergeObject(JsonObject target, JsonObject? source, List<string> notes, string? itemId)
     {
         if (source is not null)
             foreach (var (key, sourceVal) in source)
@@ -166,6 +167,8 @@ private static void MergeObject(JsonObject target, JsonObject? source, List<stri
             {
                 if (sourceChild is not null && !IsMask(sourceChild))
                     target[key] = sourceChild.DeepClone();
+                else if (sourceChild is null)
+                    RejectMaskWithoutOriginal(key, itemId, notes);
                 continue;
             }
 
@@ -177,12 +180,12 @@ private static void MergeObject(JsonObject target, JsonObject? source, List<stri
 
             if (targetVal is JsonObject targetObj)
             {
-                MergeObject(targetObj, sourceChild as JsonObject, notes);
+                MergeObject(targetObj, sourceChild as JsonObject, notes, itemId);
                 continue;
             }
 
             if (targetVal is JsonArray targetArray)
-                MergeNestedArray(targetArray, sourceChild as JsonArray, notes);
+                MergeNestedArray(targetArray, sourceChild as JsonArray, notes, itemId, key);
         }
     }
 
@@ -207,6 +210,20 @@ private static void ReconcileNamedSecret(
         }
     }
 
+    // A mask with nothing to restore would be saved as the literal "***", for example a condition
+    // operand copied onto a new edge, so the whole operation is refused. A named secret keeps the
+    // mask with a note, as when the agent invents a secret value.
+    private static void RejectMaskWithoutOriginal(string key, string? itemId, List<string> notes)
+    {
+        if (WorkflowSecretKeys.SecretConfigKeys.Contains(key))
+        {
+            notes.Add($"Secret '{key}' must be set manually — the agent may not assign secret values.");
+            return;
+        }
+        throw new ArgumentException(
+            $"'{key}' on '{itemId ?? "?"}' is '***' but has no stored value to restore. Masked values cannot be copied onto new nodes or edges; send the real value instead. Nothing was saved.");
+    }
+
     // Null and the empty string count as "no secret set".
     private static bool HasValue(JsonNode? node)
         => node is not null && !(node is JsonValue value && value.TryGetValue(out string? text) && string.IsNullOrEmpty(text));
@@ -217,7 +234,8 @@ private static bool IsMask(JsonNode? node)
     // Nested condition/case arrays have no stable item identity. If a redacted array still
     // contains a mask, retain the whole original array so a reorder cannot attach a secret to the
     // wrong case. Otherwise recurse by index to keep named-secret protection for nested objects.
-    private static void MergeNestedArray(JsonArray target, JsonArray? source, List<string> notes)
+    private static void MergeNestedArray(
+        JsonArray target, JsonArray? source, List<string> notes, string? itemId, string key)
     {
         if (source is not null && ContainsMask(target))
         {
@@ -234,15 +252,15 @@ private static void MergeNestedArray(JsonArray target, JsonArray? source, List<s
             switch (targetItem)
             {
                 case JsonObject targetObject:
-                    MergeObject(targetObject, sourceItem as JsonObject, notes);
+                    MergeObject(targetObject, sourceItem as JsonObject, notes, itemId);
                     break;
                 case JsonArray targetArray:
-                    MergeNestedArray(targetArray, sourceItem as JsonArray, notes);
+                    MergeNestedArray(targetArray, sourceItem as JsonArray, notes, itemId, key);
                     break;
                 case JsonValue targetValue when targetValue.TryGetValue(out string? value)
-                                               && value == SecretMask
-                                               && sourceItem is not null:
-                    target[index] = sourceItem.DeepClone();
+                                               && value == SecretMask:
+                    if (sourceItem is not null) target[index] = sourceItem.DeepClone();
+                    else RejectMaskWithoutOriginal(key, itemId, notes);
                     break;
             }
         }

src/NodePilot.Mcp/Tools/WorkflowEditTools.cs

@@ -147,7 +147,9 @@ public async Task<object> CreateWorkflow(
         RequireFullDefinition(definition);
         // No original to MergeFull against an empty def masks any secrets the agent set on new
         // nodes.
-        var merged = WorkflowDefinitionPatcher.MergeFull(EmptyDefinition, definition);
+        WorkflowDefinitionPatcher.PatchResult merged;
+        try { merged = WorkflowDefinitionPatcher.MergeFull(EmptyDefinition, definition); }
+        catch (ArgumentException ex) { throw new McpException(ex.Message); }
         var mergedElement = JsonSerializer.SerializeToElement(merged.Definition);
         var validation = WorkflowDefinitionStructuralValidator.Validate(mergedElement);
         if (!validation.IsValid)
@@ -295,7 +297,9 @@ private async Task<object> SaveFullAsync(
         RequireFullDefinition(definition);
         var wf = await ResolveAsync(idOrName, ct);
         var current = ParseDef(wf.DefinitionJson);
-        var merged = WorkflowDefinitionPatcher.MergeFull(current, definition);   // restore secrets from current
+        WorkflowDefinitionPatcher.PatchResult merged;
+        try { merged = WorkflowDefinitionPatcher.MergeFull(current, definition); }   // restore secrets from current
+        catch (ArgumentException ex) { throw new McpException(ex.Message); }
         var mergedElement = JsonSerializer.SerializeToElement(merged.Definition);
         var validation = WorkflowDefinitionStructuralValidator.Validate(mergedElement);
         if (!validation.IsValid)

src/nodepilot-docs-ui/content/de/mcp-server.md

@@ -81,7 +81,9 @@ Transport ist **stdio** (Streamable HTTP ist als spätere Option vorgesehen). Wi
 - **Secrets:** Workflow-Definitionen werden vor Tool-Output redigiert (`secret`/`apiKey`/`password`/
   `authToken`/`bearer`/`connectionString` → `***`). Bei `publish`/`update`/`apply_workflow_patch`
   werden echte Secrets per Node-ID aus der gespeicherten Version wiederhergestellt — das `***` des
-  Agenten überschreibt nie einen echten Wert. Credentials/Globals geben Secrets nie aus.
+  Agenten überschreibt nie einen echten Wert. Ein `***` auf einer neuen Node- oder Edge-ID hat kein
+  Original, deshalb wird der Aufruf abgelehnt und nichts gespeichert. Credentials/Globals geben
+  Secrets nie aus.
 - **Annotations:** Read-Tools `readOnly`, gated Tools `destructive`, enable/disable/lock
   `idempotent`. `execute_workflow` ist **nicht idempotent**: ein erneuter Aufruf startet
   eine weitere Ausführung. Einen unklar beantworteten Start zuerst über die Ausführungen prüfen.

src/nodepilot-docs-ui/content/en/mcp-server.md

@@ -78,7 +78,8 @@ The transport is **stdio** (streamable HTTP is planned as a later option). Windo
 - **Secrets:** workflow definitions are redacted before tool output (`secret`/`apiKey`/`password`/
   `authToken`/`bearer`/`connectionString` → `***`). On `publish`/`update`/`apply_workflow_patch`, real
   secrets are restored by node ID from the stored version — the agent's `***` never overwrites a real
-  value. Credentials/globals never emit secrets.
+  value. A `***` on a new node or edge ID has nothing to restore, so the call is rejected and nothing
+  is saved. Credentials/globals never emit secrets.
 - **Annotations:** read tools are `readOnly`, gated tools `destructive`, and enable/disable/lock
   are `idempotent`. `execute_workflow` is **not idempotent**: calling it again starts another run.
   Check executions before retrying a start whose response was lost.

tests/NodePilot.Ai.Tests/WorkflowDefinitionMergeTests.cs

@@ -419,4 +419,59 @@ public void Merge_MaskedCaseWithOmittedKeys_AddsNoNote()
             .GetValue<string>().Should().Be("prod");
         result.Notes.Should().BeEmpty();
     }
+
+    // The AI replaced edge-1 by two new edges and copied its masked literal operand onto both.
+    private const string EdgeSplitOriginal = """
+        { "nodes": [], "edges": [{ "id": "edge-1", "source": "a", "target": "b", "data": {
+            "conditionExpression": { "type": "comparison", "left": { "kind": "variable", "value": "status" },
+                "op": "==", "right": { "kind": "literal", "value": "ready" } } } }] }
+        """;
+
+    [Fact]
+    public void Merge_MaskedLiteralCopiedOntoNewEdge_KeepsMaskAndNotes()
+    {
+        var proposed = Parse("""
+            { "nodes": [], "edges": [
+              { "id": "edge-2", "source": "a", "target": "j", "data": { "conditionExpression": {
+                  "type": "comparison", "left": { "kind": "variable", "value": "status" },
+                  "op": "==", "right": { "kind": "literal", "value": "***" } } } },
+              { "id": "edge-3", "source": "a", "target": "k", "data": { "conditionExpression": {
+                  "type": "group", "op": "AND", "children": [{ "type": "comparison",
+                      "left": { "kind": "variable", "value": "status" },
+                      "op": "!=", "right": { "kind": "literal", "value": "***" } }] } } }
+            ] }
+            """);
+
+        var result = WorkflowDefinitionMerge.Merge(Parse(EdgeSplitOriginal), proposed);
+
+        result.Notes.Should().HaveCount(2);
+        result.Notes[0].Should().Contain("'value'").And.Contain("'edge-2'").And.Contain("***");
+        result.Notes[1].Should().Contain("'value'").And.Contain("'edge-3'");
+    }
+
+    [Fact]
+    public void Merge_MaskedValueInNewArrayItem_Notes()
+    {
+        var proposed = Parse("""
+            { "nodes": [{ "id": "new-node", "type": "activity", "data": { "activityType": "runScript",
+                "config": { "arguments": ["-Force", "***"] } } }], "edges": [] }
+            """);
+
+        var result = WorkflowDefinitionMerge.Merge(Parse("""{ "nodes": [], "edges": [] }"""), proposed);
+
+        result.Notes.Should().ContainSingle().Which.Should().Contain("'arguments'").And.Contain("'new-node'");
+    }
+
+    [Fact]
+    public void Merge_MaskedNamedSecretOnNewNode_NotesManualSetting()
+    {
+        var proposed = Parse("""
+            { "nodes": [{ "id": "new-node", "type": "activity", "data": { "activityType": "restApi",
+                "config": { "password": "***" } } }], "edges": [] }
+            """);
+
+        var result = WorkflowDefinitionMerge.Merge(Parse("""{ "nodes": [], "edges": [] }"""), proposed);
+
+        result.Notes.Should().ContainSingle().Which.Should().Contain("Secret 'password'");
+    }
 }

tests/NodePilot.Mcp.Tests/Mapping/WorkflowDefinitionPatcherTests.cs

@@ -321,4 +321,66 @@ public void MergeFull_MaskedLiteralsInsideNestedArrays_RestoreOriginalConditions
         merged["edges"]![0]!["data"]!["conditionExpression"]!["children"]![0]!
             ["left"]!["value"]!.GetValue<string>().Should().Be("left-secret");
     }
+
+    private const string EdgeWithLiteral = """
+        {"nodes":[],"edges":[{"id":"edge-1","source":"a","target":"b","data":{"conditionExpression":{
+          "type":"comparison","left":{"kind":"variable","value":"status"},"op":"==",
+          "right":{"kind":"literal","value":"ready"}}}}]}
+        """;
+
+    [Fact]
+    public void MergeFull_MaskedLiteralCopiedOntoNewEdge_Throws()
+    {
+        // edge-1 replaced by edge-2, which carries the mask the agent saw on edge-1.
+        var proposed = E("""
+        {"nodes":[],"edges":[{"id":"edge-2","source":"a","target":"j","data":{"conditionExpression":{
+          "type":"comparison","left":{"kind":"variable","value":"status"},"op":"==",
+          "right":{"kind":"literal","value":"***"}}}}]}
+        """);
+
+        var act = () => WorkflowDefinitionPatcher.MergeFull(E(EdgeWithLiteral), proposed);
+
+        act.Should().Throw<ArgumentException>().WithMessage("*'value' on 'edge-2'*no stored value*");
+    }
+
+    [Fact]
+    public void Apply_UpsertNewEdgeWithMaskedGroupOperand_Throws()
+    {
+        var ops = WorkflowDefinitionPatcher.ParseOps(E("""
+        [{"op":"upsertEdge","edge":{"id":"edge-2","source":"a","target":"j","data":{"conditionExpression":{
+          "type":"group","op":"AND","children":[{"type":"comparison",
+          "left":{"kind":"variable","value":"status"},"op":"!=","right":{"kind":"literal","value":"***"}}]}}}}]
+        """));
+
+        var act = () => WorkflowDefinitionPatcher.Apply(E(EdgeWithLiteral), ops);
+
+        act.Should().Throw<ArgumentException>().WithMessage("*'value' on 'edge-2'*");
+    }
+
+    [Fact]
+    public void Apply_UpsertNewNodeWithMaskedArrayItem_Throws()
+    {
+        var ops = WorkflowDefinitionPatcher.ParseOps(E("""
+        [{"op":"upsertNode","node":{"id":"new-node","type":"activity","data":{"activityType":"runScript",
+          "config":{"arguments":["-Force","***"]}}}}]
+        """));
+
+        var act = () => WorkflowDefinitionPatcher.Apply(E("""{"nodes":[],"edges":[]}"""), ops);
+
+        act.Should().Throw<ArgumentException>().WithMessage("*'arguments' on 'new-node'*");
+    }
+
+    [Fact]
+    public void Apply_UpsertNewNodeWithMaskedNamedSecret_KeepsMaskWithNote()
+    {
+        var ops = WorkflowDefinitionPatcher.ParseOps(E("""
+        [{"op":"upsertNode","node":{"id":"new-node","type":"activity","data":{"activityType":"restApi",
+          "config":{"password":"***"}}}}]
+        """));
+
+        var result = WorkflowDefinitionPatcher.Apply(E("""{"nodes":[],"edges":[]}"""), ops);
+
+        result.Definition["nodes"]![0]!["data"]!["config"]!["password"]!.GetValue<string>().Should().Be("***");
+        result.Notes.Should().ContainSingle().Which.Should().Contain("Secret 'password'");
+    }
 }