[pull] main from czlonkowski:main - #100
Merged
Merged
Conversation
* fix: preserve and author n8n canvas groups (nodeGroups) n8n 2.28 added canvas groups, stored on the workflow as `nodeGroups` and validated by n8n on every write — including writes unrelated to grouping. Our update payload dropped the field, so n8n backfilled the stored groups and validated them against the graph we submitted. Any grouped workflow then failed with HTTP 400 as soon as a diff removed a grouped node or changed a group's connectivity, taking partial updates, full updates, autofix, rollback and version restore with it. Groups are now carried through writes and reconciled with the finished graph once, at the end of a diff: members a diff deleted are pruned, a group left empty is removed, and a group n8n refuses is ungrouped so the edit still lands. Nodes and connections are never altered to save a frame. Topology stays n8n's call. Its rejection names the offending group, so the server adjudicates and we drop precisely that group — no local copy of its grouping rules to drift from the running instance. Groups the caller authored in the same request are never silently discarded; n8n's message is surfaced instead. Support is discovered from the write itself rather than a version probe, since /rest/settings does not expose a version on every instance: an instance that rejects the field gets the workflow without it, one that rejects group descriptions (n8n 2.32+) gets the groups without them, and both warn. Also adds the setNodeGroups diff operation, `nodeGroups` on create and full update, groups in structure/filtered/active reads and template deploys, non-blocking group checks in validate_workflow, and two fixes found along the way: `updateNode` no longer accepts an `id` rewrite (it orphaned group membership and pinned data), and compareVersions now notices a group-only change. Conceived by Romuald Członkowski - www.aiadvisors.pl/en Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix: address review findings on canvas-group support Copilot: - classifyGroupError bailed out when the sent nodeGroups array was empty, so an explicit `nodeGroups: []` (ungroup everything) against a pre-2.28 instance threw instead of degrading to omitting the field. The classifier is only consulted once the field was sent, and [] is a sent field. - setNodeGroups reported "node is in two groups" when the same node appeared twice in ONE group's member list. A duplicate within a group describes the same member set, so it is deduped; only a claim by a different group is a conflict. Codex: - sendWorkflowWrite warned about an unsupported nodeGroups field only on the write that discovered it. The client outlives a request, so every later write silently dropped groups — including ones the caller had just authored, reported as plain success. Both capability limits now warn on every affected write, from shared constants so the paths cannot drift. - patchNodeField could rewrite node.id by dot path, bypassing the immutability guard added to updateNode and orphaning canvas groups and pinned data. Only the node's own id is refused; a nested id such as parameters.assignments.assignments[0].id is ordinary data. - Group warnings raised while rolling back never reached the caller, because that path throws before the success response is built. They now ride the augmented error details, so "restored to prior state" no longer hides that a group was dropped to get there. Also folds in a simplification pass: toWorkflowNodeGroup() is shared by the create/update path and the diff operation so they cannot drift on trimming or id generation, nodeGroupsField() replaces the repeated spread-conditionals in the read handlers, and a docblock that had drifted onto the wrong function is back where it belongs. Conceived by Romuald Członkowski - www.aiadvisors.pl/en Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix: close canvas-group defects found in review Critical: a payload with `nodeIds: []` alongside a populated `nodeNames` ungrouped the entire workflow while reporting success. validateSetNodeGroups picked the member list by non-emptiness, resolveGroupMembers by mere presence, so the group resolved to zero members, finalization dropped it as empty, and — setNodeGroups being a full replacement — every other group went with it. Both keys with one empty is a common LLM shape. The resolver now uses the validator's predicate, and applySetNodeGroups throws instead of skipping when the two disagree, so an unvalidated group can never reach the workflow. The additionalProperties branch of classifyGroupError was not scoped to nodeGroups: any unknown-property 400 (a stray settings key, a field a future n8n stops accepting) latched `groupSupport.groups = false` on a client that lives for the process. Every later write then omitted the field, n8n backfilled the stored groups and revalidated them, and the exact 400 this feature exists to prevent came back — on an instance that supports groups fine. The complaint must now implicate nodeGroups. On a genuinely pre-2.28 instance the only way to reach that branch is an explicitly authored grouping, and failing that loudly is the documented contract. A group name containing a quote (`Say "hi"`) truncated the name capture, so the offending group matched nothing and control fell through to ungroup-everything — destroying every other group to recover from one. n8n's connectivity message carries the group id, which is now preferred for matching; an identified group we cannot match surfaces the error instead of guessing. The authored-group contract held in the write ladder but not in repair, so a mistyped nodeId in a group the caller had just authored was silently pruned (or the group dropped) and reported as success. Repair now takes the authored set and errors for those, while inherited groups keep degrading as before. Also: the last resort drops only inherited groups rather than all of them; malformed group entries are reported instead of vanishing; a payload with no `nodes` is left alone rather than treated as "no nodes exist"; a null `nodeGroups` from a GET is not forwarded; one malformed row no longer fails a filtered read; and version restore and template deploy carry the warning channel so group adjustments made during those writes reach the caller. Conceived by Romuald Członkowski - www.aiadvisors.pl/en Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix: only forward a usable string as a canvas-group description Groups read back from n8n are untyped at runtime, so sanitizeGroupsForApi could forward a number or object as `description` and earn a 400 the degradation ladder cannot attribute to descriptions. Non-strings and blanks are now dropped. Conceived by Romuald Członkowski - www.aiadvisors.pl/en Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix: validate the setNodeGroups payload instead of trusting it This operation's payload is unvalidated at the tool boundary (z.any() in the operations schema), so malformed input reached code that assumed strings: a non-string member threw a bare "trim is not a function" from normalizeNodeName, and a non-string id did the same inside toWorkflowNodeGroup. Both now return an actionable message. Description is validated here too — type and n8n's 155-character cap — so the diff operation agrees with the create/update tools rather than letting n8n answer with a 400 that the degradation ladder then has to interpret. Conceived by Romuald Członkowski - www.aiadvisors.pl/en Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix: restore n8n_workflow_versions rollback without an instance context The handler resolved its API client only when an InstanceContext was supplied: const client = context ? getN8nApiClient(context) : null; That skipped the environment-variable fallback inside getN8nApiClient which every other tool reaches through ensureApiConfigured. On a plain N8N_API_URL setup — the common single-instance case — `client` was therefore always null, and since `rollback` is the only mode that needs the API, it answered "n8n API not configured. Cannot perform rollback without API access." for every workflow while `list` and `get` kept working off the local version store. Multi-tenant isolation is unaffected: it is enforced inside getN8nApiClient (which refuses process credentials when ENABLE_MULTI_TENANT is set) and by the tenant-scope check at the top of this handler, not by the ternary. Found by the canvas-group live test, which could not exercise rollback across a node deletion. Unrelated to canvas groups, folded into this PR at the author's request. Conceived by Romuald Członkowski - www.aiadvisors.pl/en Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix: close four defects from the second review pass Template deploy collected canvas-group warnings into a variable nothing read, so a template whose frames n8n could not store reported an unqualified "deployed successfully". It was the only one of the five write entry points breaking the warning contract; tsc cannot flag a write-only local. dropRejectedGroup matched "id only, else name only" rather than id first and name second. The id is scraped from a parenthetical in n8n's message, so any message shaped `Node group "X" (2 nodes) ...` produced an id that matched nothing and suppressed a perfectly good name match — failing the whole write for an inherited group, which is the class of failure this feature removes. Group names are unique, so the fallback cannot select the wrong group. The setNodeGroups member guard used find(), which returns the element: a member that IS undefined satisfied `badMember === undefined` and skipped the very guard written to catch it, leaving `undefined.trim()` to throw. Unreachable over JSON but reachable in-process. findIndex fixes it. The filtered read lost its element-shape guard in an earlier simplification pass, so one malformed group row would fail the whole read. Also softens the changelog's degradation claim: it holds when n8n identifies the offending property, which its request validator reports, and a rejection naming nothing is surfaced rather than guessed at. Conceived by Romuald Członkowski - www.aiadvisors.pl/en Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix: make the unsupported-field rung work against real n8n responses The scoping added earlier required the string "nodeGroups" to appear in n8n's 400 before treating it as a schema rejection. Captured from a live instance, n8n's actual bodies are: unknown top-level property {"message":"request/body must NOT have additional properties"} unknown key inside a group {"message":"request/body/nodeGroups/0 must NOT have additional properties"} Its public-API serializer keeps only `message`, and AJV's text names the offending property only for a nested path. So the description rung worked — a group-level rejection carries `nodeGroups/0`, confirmed live — but the field rung could never fire: a pre-2.28 instance would hard-fail every write carrying stored groups, which is the failure this feature exists to remove. Blaming the field on a pathless message would reintroduce the poisoning risk that scoping was added to prevent, so neither guess is used. A pathless rejection is now a hypothesis: retry without nodeGroups, and record the instance as lacking the field only if that retry succeeds. If it fails too, something else in the body was wrong, n8n's original error is surfaced and the capability memo is left alone. A nested path still identifies the group schema directly, and a path naming another field is ignored outright. The classifier fixtures asserted shapes n8n never sends — a property name baked into the message text, or a fabricated details.errors[].path — so they passed while encoding the same wrong model as the code. They are now the bodies captured from the live instance. Also documents why the diff engine's finalization deliberately does not apply the authored-group contract: inside one batch the only way an authored group loses a member is that the batch removed it, which is an instruction rather than a mistake, and a group naming a node that never existed is already rejected by validateSetNodeGroups. Pinned with a test. Conceived by Romuald Członkowski - www.aiadvisors.pl/en Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * refactor: drop the now-unused sentGroups parameter from classifyGroupError The branch that consumed it — preferring a description strip when a pathless rejection could plausibly have been caused by one — was removed once the real n8n error shapes were captured. Classification is decided entirely by the message now. Conceived by Romuald Członkowski - www.aiadvisors.pl/en Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix: make rollback warnings the same shape as success-path warnings The rollback-on-error path put a string[] into details.warnings, while the success path returns WorkflowDiffValidationError objects via mergeWarnings. A client reading details.warnings had to branch on the outcome to know which it was getting. Rollback warnings are now objects with operation: -1, matching the batch-level convention used everywhere else. Also corrects the setNodeGroups docblock: it claimed exactly one of nodeNames/nodeIds may be present, but the engine deliberately accepts both when one is empty — that shape is common in generated payloads, and rejecting it was the critical bug fixed in c440093. Both populated is still ambiguous and rejected. Conceived by Romuald Członkowski - www.aiadvisors.pl/en Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
See Commits and Changes for more details.
Created by
pull[bot] (v2.0.0-alpha.4)
Can you help keep this open source service alive? 💖 Please sponsor : )