feat(core): Improve node grouping instructions for AI Assistant and MCP (no-changelog) - #37729
Draft
miguelsaddress wants to merge 18 commits into
Draft
feat(core): Improve node grouping instructions for AI Assistant and MCP (no-changelog)#37729miguelsaddress wants to merge 18 commits into
miguelsaddress wants to merge 18 commits into
Conversation
…CP (no-changelog)
Grouping guidance had no numbers ("larger workflows", "small workflows"), so
agents over-grouped: frames on trivial workflows, groups of one or two nodes,
titles naming the nodes inside instead of the outcome.
GROUPING_GUIDANCE now carries the judgement — 3 to 5 groups for a medium
workflow, at most 7 items at the canvas top level, a group as one business
outcome rather than a technical category, 2-4 word outcome-first titles, and a
description on every group that adds to the title instead of restating it.
NODE_GROUPS_REFERENCE keeps only the mechanics, and no longer calls the
description optional or asks for a single sentence, both of which undercut the
guidance.
Structural rules stay sourced from NODE_GROUPING_RULES and are not restated in
the guidance; a test walks that constant to keep the two copies from drifting.
Both surfaces already import both constants, so Instance AI and MCP pick this
up with no further change.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…builder skill (no-changelog)
The builder skill carried its own summary of the grouping guidance, and it had
drifted: no group counts, and a "one-sentence description" instruction that the
shared guidance now contradicts. The skill is read in full whenever it loads, so
that stale copy is what the agent acts on, while the updated guidance sat in a
knowledge-base file it had to choose to open.
The skill now injects the shared GROUPING_GUIDANCE at materialization time
through a {{GROUPING_GUIDANCE_PLACEHOLDER}} token, so there is one source of
truth and the criteria arrive in context without depending on an extra read.
What stays hand-written is one line of mechanics and a pointer to the node-groups
reference for the rules that make a group valid.
The reference itself is not injected: it is mechanical, it is only needed while
writing a .group(...) call, and it already ships inside the SDK language
reference the builder has to read anyway.
Tests cover the injected text, that no placeholder is left unresolved in any
materialized skill, and that the skill source carries no grouping criteria of its
own, so a hand-written copy cannot come back unnoticed.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…-changelog) An observed build skipped grouping on a workflow that clearly warranted three groups. Asked why, the agent said it had not decided against groups — it never reached the step, and then applied the criteria correctly on request. The criteria were fine; grouping was simply not part of the build. The shared guidance now opens by requiring an explicit decision while the code is written: declare the groups, or conclude the workflow does not warrant any. Deciding against groups stays a first-class answer, so this does not trade omission for grouping by rote. The AI Assistant builder skill gets the sequencing the guidance cannot carry: the Mandatory Process step that produces the SDK code now says to decide grouping there, since .group(...) lives in the source and cannot be added after the build, and Completion refuses to report a build as done before that decision. MCP has no equivalent process to hook into, which is why the obligation itself lives in the shared string. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…gelog) An observed build left 9 top-level items ungrouped and justified it as "linear and small". Its reasoning showed why: it read "purely linear" as a standalone exemption, treated 9 nodes as small on its own authority, believed groups were for parallel blocks, and never counted against the 7-item ceiling. Told about the ceiling, it agreed it applied and proposed the right three groups. The decision is now a count rather than a judgement about shape or size: count the trigger plus every node or existing group, and more than 7 means grouping is required. Linearity is explicitly not an exemption, and sequential stages are named as the normal case for grouping rather than the exception. The ceiling moves to a post-grouping check, keeping the tie-break toward fewer groups. Boundaries that come out at one or two nodes each are now called too fine, with the instruction to widen them — the same build rejected grouping because it only ever considered two-node stages. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…(no-changelog)
The grouping guidance was injected while rendering the materialized SKILL.md,
which left two holes.
`load_skill` serves `skill.instructions` straight from the source, so an agent
loading the builder skill through the tool received the literal
{{GROUPING_GUIDANCE_PLACEHOLDER}} and no guidance at all. Only the filesystem
copy of SKILL.md carried the real text.
`hashSkill` also hashes the raw instructions, so changing the shared constant
left `skillsHash` untouched. Caches and workspace manifests keyed on that hash
kept serving a previously rendered SKILL.md with the older guidance.
Substitution now happens as the skill source is loaded, through a
`transformInstructions` option on `loadRuntimeSkillSourceFromDirectory`. It runs
before the registry is built, so both delivery routes carry the same text and the
hash covers it.
The test that asserted the placeholder survives into the loaded skill was
asserting the bug. It now reads SKILL.md from disk to check the source keeps no
grouping criteria of its own, and two new tests cover the load_skill route and
the pre-hash substitution.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…hangelog) Two findings from real builds. A description that opens by restating its title reads as compliant but wastes the only words the user sees: a collapsed group clips the text, so "Validates the shipping address against the Google API and reduces it to a verdict" under the title "Validate via Google" shows the reader nothing new before it cuts off. The guidance now forbids opening with the title and says to lead with the detail that earns its place, giving the clipping as the reason. The 7-item ceiling was written as a hard requirement, which invites an agent to invent an invalid group or split a coherent stage to hit the number. Branches fanning straight out of a trigger cannot share a group, so some graphs cannot reach 7 at all. It is now a target: landing an item or two over is fine when the shape forbids better, and neither breaking the validity rules nor splitting a stage that reads as one objective is an acceptable way to reach it. The tie-break becomes fewer and larger groups. Deciding *whether* to group stays the hard count it was, so this does not reopen the "it is linear, so it needs no groups" excuse. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
A build described its agent group as "AI agent with Postgres chat memory drafts a reply, searching the docs vector store and opening a Linear ticket". The collapsed group clips the text, so the only words the reader saw were the node inventory plus a restatement of the title — everything already visible on the canvas. The lead-with-the-detail rule now forbids opening on what the group is built from, and carries that description as the counter-example. A fourth worked example shows the same group written from the input and the output instead. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…tay high (no-changelog) Both prompt strings are tightened: the naming and description sections become inline paragraphs and the reference leads with its MUST. Five findings from real builds then land on top, so the guidance still ends smaller than it started. A build wrote three groups and lost two to NODE_GROUP_DROPPED. Its stages ended in an IF that stopped one way and continued the other, which makes the IF a non-leaf with an edge out of its own group. Nothing told it where to cut, so the guidance now does: end the group before the branch node, or leave that node and its stop path outside. The same build read its two warnings as licence to publish the stage ungrouped. The reference now says a warning means the boundary was wrong, not that the stage stays ungrouped — redraw and build again, without re-emitting the same invalid group. Another build skipped the grouping decision entirely and rationalised it afterwards, exactly the failure the decision rule was added for. That rule had been folded into the paragraph that defines what a group is; it gets its own paragraph back. The 7-item target invited an agent to chase a number a branchy graph cannot reach. Staying above 7 is now allowed only when every remaining top-level item is a group already or cannot join one without breaking a validity rule, which is stricter than the target it replaces: it has to be checked item by item. Titles came out as step pairs — "Charge & ship", "Store & alert" — which also drags the description into restating them. Titles now name one outcome, with the fix shown rather than just banned. It is explicitly a preference: a two-verb title beats splitting a coherent stage or leaving it ungrouped, because canvas coverage matters more than a title. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Contributor
PR review overviewBased on ownership of the 12 changed files in this PR:
|
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Contributor
Instance AI Discovery Eval ✅Branch: Eval output |
…s a ban on grouping (no-changelog) A build skipped grouping and explained why: the SDK_UNSOLICITED_STICKY warning fired, it stripped the sticky note, and it then treated every cosmetic canvas construct as "do not add unless asked". Groups went with the stickies. The warning only ever mentioned stickies, but the generalisation is a fair reading: stickies and groups are the two cosmetic canvas constructs, and "unless the user explicitly asked" is a strong frame that arrives mid-build, in response to the agent's own code. That is far more salient than a paragraph in the skill. The message now says it applies to sticky notes only and that node groups are not optional decoration, so the grouping decision survives the warning. This is likely the root cause of the omissions seen earlier, where builds described skipping "the visual organisation step" rather than deciding against groups. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
…one (no-changelog) A build drew two groups, lost one to NODE_GROUP_DROPPED, described the loss as cosmetic and shipped with the top level above 7. Asked about it, it quoted both rules it had broken and said the skill was loaded and reloaded throughout the build. So this was not a delivery or knowledge gap: it read the rules and under-applied them. The grouping decision already fires reliably, because it is a step in the Mandatory Process. What did not fire was checking the result, which lived only in the guidance. Completion now demands that check: a dropped-group warning means redraw the boundary and build again, and staying above 7 top-level items means naming each remaining item and why it cannot join a group. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Both the reference and the builder skill said a dropped-group warning means the boundary was wrong. Five of the causes are not about the boundary at all: a duplicate group id or name, an empty group, a member that does not exist, and a node claimed by two groups. Faced with a duplicate name, an agent following that wording would go and redraw a boundary that was already correct. Both texts now say the warning names what was invalid, give the non-boundary causes as examples, and tell the agent to fix what the warning reports. A warning still never means the stage stays ungrouped, and the same invalid group must not be re-emitted. A test fails if the old wording returns. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
… (no-changelog) The previous commit flipped what the test asserts — the reference must not tell the agent to redraw the boundary — but left the title and comment saying the opposite. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
… branch (no-changelog) The branch rule offered two ways out and missed the best one. A branch node whose paths both leave the group is a leaf inside the selection, and both edges come from that one node, so it counts as a single exit and the group is valid. Only a branch node that keeps one path inside and sends the other out is rejected. That missing option matters: keeping the branch node inside leaves the fewest boxes on the canvas. An observed build ended its validation group before the majority check and put the check plus both alert nodes at the top level, where the check itself could have stayed inside. The three options are now listed, best first. The ceiling also becomes a shared constant, TOP_LEVEL_ITEM_CEILING in n8n-workflow, interpolated into the guidance next to the description cap so the number has one home. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…elog) Grouping rules now live in three places an agent can read: the guidance, the Mandatory Process step that writes the code, and a Completion checkpoint that asks it to count the top level and justify anything left over. Builds quote all three accurately when asked, group correctly when they act, and skip the count anyway. The step that produces something fires; the step that reviews what was produced does not. Dropped-group warnings do land, so the count is now delivered the same way. build-workflow reports the boxes a reader would see with every group collapsed, once past the ceiling, and names what is still ungrouped — which is the list the checkpoint asks the agent to produce. It runs after invalid groups are dropped, so it describes the canvas that was saved. Nothing is reported at or under the ceiling, so a correct build stays quiet. Sub- nodes ride with their parent and stickies belong to the user, so neither counts. The trigger counts as a box but is never listed, since no group may hold one. Each of those has a test: a false positive here teaches the agent to ignore the warning. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Bundle ReportChanges will increase total bundle size by 114.68kB (0.18%) ⬆️. This is within the configured threshold ✅ Detailed changes
Affected Assets, Files, and Routes:view changes for bundle: editor-ui-esmAssets Changed:
|
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
… (no-changelog) The shared rule said a group takes at most one incoming and one outgoing main connection. The validator counts nodes: it collects the members that take input from outside and fails when there is more than one, and likewise for output. So several connections may reach a single entry member — a Merge fed by three branches is a valid entry — and the old wording ruled that out. An agent reading it literally rejects groups the save path would accept. The guidance also gains the play for a stage whose work fans out. Such a stage cannot be one group while the branches start outside it, because each branch head takes input from outside; keeping the node they fan out from inside makes the fan-out internal. When the branches leave the trigger, which can never be a member, the stage needs a preparation step to serve as the entry, or the group starts where the branches reconverge. A build that hit this wall grouped nothing at all afterwards, including a post-merge stretch that was valid. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
…hangelog) A build called grouping a finishing touch and skipped it, echoing the word this doc used: groups were described as cosmetic and purely organisational three times. The reference now pairs no execution semantics with why groups still matter, so the two never travel apart. The boundary rules move into their own block, stating the one-member-per-side limit once and covering the exit side and nodes already in another group. A rejected group no longer reads as a closed stage: three builds took one rejection and left the stage ungrouped where a smaller slice was valid. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
0 issues found across 2 files (changes from recent commits).
Confidence score: 5/5
- Automated review surfaced no issues in the provided summaries.
- No files require special attention.
Shadow auto-approve: would auto-approve. Refines AI assistant/MCP node-grouping guidance into counting-based rules, injects the shared constant into the builder skill via a placeholder, and adds an informational top-level-item warning; extensive tests pin the prompt text, placeholder substitution, and warning behavior.
Re-trigger cubic
…(no-changelog) Every reminder that arrives after the save is ignored — the completion checkpoint and the build warning both. The one an agent has acted on is a source lint issue, at the same informational severity: it stripped a sticky note because SDK_UNSOLICITED_STICKY told it to. The difference is phase, not severity, so the box count moves to the lint the Mandatory Process makes it run before its first build. The count is exact at source level: node() and trigger() draw boxes, the sub-node builders do not, and a .group() is one box plus its members. No connection heuristics. The message says to group while building and to leave an existing layout alone, since the lint cannot tell authoring from editing. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
3 issues found across 2 files (changes from recent commits).
Confidence score: 3/5
- In
packages/@n8n/workflow-sdk/src/lint/sdk/workflow-sdk-lint.ts, omittingifElse,switchCase,merge, andsplitInBatchesfromboxBuilderslets oversized workflows avoid the grouping warning; include all main-node canvas types. - The lint analysis in
packages/@n8n/workflow-sdk/src/lint/sdk/workflow-sdk-lint.tscan warn about boxes thattoJSON()never serializes when source handles outnumber exported builders, creating unnecessary grouping guidance; count only handles reachable from the exported builder. - The
.group()analysis inpackages/@n8n/workflow-sdk/src/lint/sdk/workflow-sdk-lint.tstreats validGroupMember[]variables and spread arrays as ungrouped, causing false warnings for compact canvases; resolve member bindings before counting.
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="packages/@n8n/workflow-sdk/src/lint/sdk/workflow-sdk-lint.ts">
<violation number="1" location="packages/@n8n/workflow-sdk/src/lint/sdk/workflow-sdk-lint.ts:170">
P1: When a workflow uses `ifElse`, `switchCase`, `merge`, or `splitInBatches`, this set leaves those canvas nodes out of `boxBuilders`, so oversized workflows can pass without the grouping warning. Include all main-node composite builders in the counted set while continuing to exclude sub-nodes and sticky notes.</violation>
<violation number="2" location="packages/@n8n/workflow-sdk/src/lint/sdk/workflow-sdk-lint.ts:190">
P2: When source declares more than seven node handles but adds seven or fewer to the exported builder, this warning reports boxes that `toJSON()` never serializes and urges unnecessary grouping. Count handles reachable from the exported workflow builder instead of every factory call.</violation>
<violation number="3" location="packages/@n8n/workflow-sdk/src/lint/sdk/workflow-sdk-lint.ts:205">
P2: When `.group()` receives a valid `GroupMember[]` variable or spread array, this pass treats every member as ungrouped and can warn for a compact canvas. Resolve the member binding before counting, or avoid claiming those nodes are ungrouped when the list cannot be statically resolved.</violation>
</file>
Shadow auto-approve: would not auto-approve because issues were found.
Tip: Review your code locally with the cubic CLI to iterate faster.
Fix all with cubic | Re-trigger cubic
| /** Lint a prepared, parsed SDK AST (imports/TS already stripped). */ | ||
| /** Builders that put a box on the canvas. Sub-node builders (`languageModel`, | ||
| * `tool`, `memory`, …) and `sticky` ride with their parent, so they are absent. */ | ||
| const CANVAS_BOX_BUILDERS = new Set(['node', 'trigger']); |
Contributor
There was a problem hiding this comment.
P1: When a workflow uses ifElse, switchCase, merge, or splitInBatches, this set leaves those canvas nodes out of boxBuilders, so oversized workflows can pass without the grouping warning. Include all main-node composite builders in the counted set while continuing to exclude sub-nodes and sticky notes.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/@n8n/workflow-sdk/src/lint/sdk/workflow-sdk-lint.ts, line 170:
<comment>When a workflow uses `ifElse`, `switchCase`, `merge`, or `splitInBatches`, this set leaves those canvas nodes out of `boxBuilders`, so oversized workflows can pass without the grouping warning. Include all main-node composite builders in the counted set while continuing to exclude sub-nodes and sticky notes.</comment>
<file context>
@@ -164,6 +165,76 @@ function rangeContains(range: SourceRange, line: number, column: number): boolea
/** Lint a prepared, parsed SDK AST (imports/TS already stripped). */
+/** Builders that put a box on the canvas. Sub-node builders (`languageModel`,
+ * `tool`, `memory`, …) and `sticky` ride with their parent, so they are absent. */
+const CANVAS_BOX_BUILDERS = new Set(['node', 'trigger']);
+
+/**
</file context>
Suggested change
| const CANVAS_BOX_BUILDERS = new Set(['node', 'trigger']); | |
| const CANVAS_BOX_BUILDERS = new Set(['node', 'trigger', 'ifElse', 'switchCase', 'merge', 'splitInBatches']); |
|
|
||
| const { callee } = node; | ||
| if (callee.type === 'Identifier' && CANVAS_BOX_BUILDERS.has(callee.name)) { | ||
| boxBuilders++; |
Contributor
There was a problem hiding this comment.
P2: When source declares more than seven node handles but adds seven or fewer to the exported builder, this warning reports boxes that toJSON() never serializes and urges unnecessary grouping. Count handles reachable from the exported workflow builder instead of every factory call.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/@n8n/workflow-sdk/src/lint/sdk/workflow-sdk-lint.ts, line 190:
<comment>When source declares more than seven node handles but adds seven or fewer to the exported builder, this warning reports boxes that `toJSON()` never serializes and urges unnecessary grouping. Count handles reachable from the exported workflow builder instead of every factory call.</comment>
<file context>
@@ -164,6 +165,76 @@ function rangeContains(range: SourceRange, line: number, column: number): boolea
+
+ const { callee } = node;
+ if (callee.type === 'Identifier' && CANVAS_BOX_BUILDERS.has(callee.name)) {
+ boxBuilders++;
+ return;
+ }
</file context>
|
|
||
| groupCount++; | ||
| const members = node.arguments[1]; | ||
| if (members?.type !== 'ArrayExpression') { |
Contributor
There was a problem hiding this comment.
P2: When .group() receives a valid GroupMember[] variable or spread array, this pass treats every member as ungrouped and can warn for a compact canvas. Resolve the member binding before counting, or avoid claiming those nodes are ungrouped when the list cannot be statically resolved.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/@n8n/workflow-sdk/src/lint/sdk/workflow-sdk-lint.ts, line 205:
<comment>When `.group()` receives a valid `GroupMember[]` variable or spread array, this pass treats every member as ungrouped and can warn for a compact canvas. Resolve the member binding before counting, or avoid claiming those nodes are ungrouped when the list cannot be statically resolved.</comment>
<file context>
@@ -164,6 +165,76 @@ function rangeContains(range: SourceRange, line: number, column: number): boolea
+
+ groupCount++;
+ const members = node.arguments[1];
+ if (members?.type !== 'ArrayExpression') {
+ return;
+ }
</file context>
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 join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Summary
The AI Assistant and MCP clients group workflows poorly. They draw frames on trivial
workflows, put one or two nodes in a group, and write titles that name the nodes inside
instead of the outcome. The instructions live in two shared strings in
packages/@n8n/workflow-sdk/src/prompts/sdk-reference/sdk-language.ts:NODE_GROUPS_REFERENCEholds the mechanics and the validity rules, and
GROUPING_GUIDANCEholds the judgement.Both surfaces import both strings.
GROUPING_GUIDANCEhad no numbers, only "larger workflows" and "small workflows", which anagent resolves in its own favour. It now requires an explicit grouping decision while the
code is written, decides whether to group by counting top-level items rather than by shape
or size, says where to cut when a stage ends in a branch, and holds titles and descriptions
to one outcome each.
NODE_GROUPS_REFERENCEkeeps only the mechanics, no longer presents thedescription as optional, and states that a dropped-group warning means the boundary was
wrong rather than that the stage stays ungrouped.
The Instance AI builder skill kept its own summary of the guidance, which had already
drifted. It now injects the shared constant through a placeholder resolved when the skill
source is loaded, so there is one source of truth and it reaches both the materialized
SKILL.mdand theload_skilltool.How to test
For a manual check, build workflows of varying size with the AI Assistant and read the
nodeGroupsof each result. A trivial workflow should get no groups. A workflow with morethan 7 top-level items should get one group per stage, with outcome-first titles of 2 to 4
words and a description that adds to the title. Group output varies between runs, so run
each prompt at least three times. Test prompts and expected results follow in a comment.
Related Linear tickets, Github issues, and Community forum posts
https://linear.app/n8n/issue/ADO-5802
Review / Merge checklist
Backport to Beta,Backport to Stable, orBackport to v1(if the PR is an urgent fix that needs to be backported)🤖 PR Summary generated by AI