diff --git a/packages/@n8n/instance-ai/src/tools/__tests__/workflows.tool.test.ts b/packages/@n8n/instance-ai/src/tools/__tests__/workflows.tool.test.ts index 5c9c37354c7..03a6061e283 100644 --- a/packages/@n8n/instance-ai/src/tools/__tests__/workflows.tool.test.ts +++ b/packages/@n8n/instance-ai/src/tools/__tests__/workflows.tool.test.ts @@ -3473,6 +3473,78 @@ describe('workflows tool', () => { expect(suspend.mock.calls[0][0]).toMatchObject({ setupRequests: [sheetsRequest] }); }); + describe('nodes the build left out of setup', () => { + // The build changed only the Sheets node; Slack is pre-existing and unconfigured. + function createScopedContext() { + const context = createGrantAwareContext(); + context.runId = 'run-1'; + (context as { workflowBuildContext?: unknown }).workflowBuildContext = { + threadId: 't1', + runId: 'run-1', + taskId: 'task-1', + workItemId: 'wi-1', + workflowTaskService: { + getLatestBuildOutcomeForWorkflow: vi + .fn() + .mockResolvedValue({ runId: 'run-1', changedNodeNames: ['Log to Sheet'] }), + }, + }; + return context; + } + + beforeEach(() => { + (analyzeWorkflow as Mock).mockResolvedValue([slackRequest, sheetsRequest]); + (applyNodeChanges as Mock).mockResolvedValue({ applied: [], failed: [] }); + (buildCompletedReport as Mock).mockReturnValue([]); + }); + + it('does not record them as skipped when the user skips the card', async () => { + const context = createScopedContext(); + + const tool = createWorkflowsTool(context); + const result = await executeTool(tool, { action: 'setup', workflowId: 'wf1' }, { + resumeData: { approved: false }, + } as never); + + expect(context.grantSessionToolApproval).not.toHaveBeenCalledWith( + 'workflows:setup-skip:cred:slackApi', + ); + expect(result).toMatchObject({ skippedByUser: [{ nodeName: 'Log to Sheet' }] }); + }); + + it('does not report them as still needing setup after apply', async () => { + const context = createScopedContext(); + + const tool = createWorkflowsTool(context); + const result = await executeTool(tool, { action: 'setup', workflowId: 'wf1' }, { + resumeData: { approved: true, action: 'apply' }, + } as never); + + expect(result).toMatchObject({ + partial: true, + nodesStillNeedingSetup: [{ nodeName: 'Log to Sheet' }], + }); + }); + + it('keeps them out of the panel a trigger test rebuilds', async () => { + const context = createScopedContext(); + (context.executionService.run as Mock).mockResolvedValue({ status: 'success' }); + const suspend = vi.fn(); + + const tool = createWorkflowsTool(context); + await executeTool(tool, { action: 'setup', workflowId: 'wf1' }, { + suspend, + resumeData: { + approved: true, + action: 'test-trigger', + testTriggerNode: 'Log to Sheet', + }, + } as never); + + expect(suspend.mock.calls[0][0]).toMatchObject({ setupRequests: [sheetsRequest] }); + }); + }); + it('revalidates the credential test destination after a trigger test', async () => { const fixture = templatedSetupFixture({ testUrl: 'https://status.example.net/me' }); (analyzeWorkflow as Mock).mockResolvedValue([fixture.request]); diff --git a/packages/@n8n/instance-ai/src/tools/workflows.tool.ts b/packages/@n8n/instance-ai/src/tools/workflows.tool.ts index cf8685b8070..eeb305787f7 100644 --- a/packages/@n8n/instance-ai/src/tools/workflows.tool.ts +++ b/packages/@n8n/instance-ai/src/tools/workflows.tool.ts @@ -1189,11 +1189,11 @@ async function handleSetupTestTrigger( appliedCredentialIds: appliedCredentialIdList(resumeData.credentials, preTestApply.applied), }, ); - // Re-derived from scratch, so it has to be partitioned again: this is the second path that - // builds the panel, and without it a trigger test mid-session puts back the cards state 1 - // left out. + // Re-derived from scratch, so it has to be scoped and partitioned again: this is the second + // path that builds the panel, and without it a trigger test mid-session puts back the cards + // state 1 left out. The destination check below still sees every node. const { pending: refreshedPending } = partitionSkippedSetupRequests( - refreshedRequests, + await filterToSetupScope(context, input, refreshedRequests), input.workflowId, getSkippedSetupSubjects(context), ); @@ -1319,12 +1319,17 @@ async function handleSetupApply( // Filter by needsAction to distinguish "render this card" from // "this still requires user intervention". Settled requests are kept so // a just-applied credential whose test failed stays reportable below — - // a bound credential is settled for routing even when its test fails. - const remainingRequests = await analyzeWorkflow(context, input.workflowId, undefined, { - includeSettled: true, - ...preferNewCredentialOptions(input, resumeData.credentials), - appliedCredentialIds: appliedCredentialIdList(resumeData.credentials, applyResult.applied), - }); + // a bound credential is settled for routing even when its test fails. Scoped like the + // card: a node it left out is neither still pending nor skipped by the user. + const remainingRequests = await filterToSetupScope( + context, + input, + await analyzeWorkflow(context, input.workflowId, undefined, { + includeSettled: true, + ...preferNewCredentialOptions(input, resumeData.credentials), + appliedCredentialIds: appliedCredentialIdList(resumeData.credentials, applyResult.applied), + }), + ); const completedNodes = buildCompletedReport( resumeData.credentials, resumeData.nodeParameters, @@ -1444,6 +1449,23 @@ async function resolveSetupScopeNodeNames( } } +/** + * Narrow requests to the build-bound setup scope. Every path that builds or reports the setup + * card goes through here, so a node the card left out cannot come back through a trigger test, + * the apply report, or a skip the user never made. + */ +async function filterToSetupScope( + context: InstanceAiContext, + input: Extract, + requests: SetupRequest[], +): Promise { + if (input.includeAllNodes) return requests; + const scopeNodeNames = await resolveSetupScopeNodeNames(context, input.workflowId); + return scopeNodeNames + ? requests.filter((request) => scopeNodeNames.includes(request.node.name)) + : requests; +} + /** * Coverage disclosure for the workflow's latest verification, or undefined when * it was fully verified or no claim exists. Absent evidence never blocks: a @@ -1689,12 +1711,7 @@ async function handleSetup( // Setup after a build covers only the nodes that build changed — // pre-existing, unrelated nodes must not surface in the setup card. - const scopeNodeNames = input.includeAllNodes - ? undefined - : await resolveSetupScopeNodeNames(context, input.workflowId); - const scopedRequests = scopeNodeNames - ? allSetupRequests.filter((request) => scopeNodeNames.includes(request.node.name)) - : allSetupRequests; + const scopedRequests = await filterToSetupScope(context, input, allSetupRequests); // Two reasons a card stays out, applied in order: this build never touched the node, or // the user declined it. Partitioning the scoped list keeps them apart in the report — @@ -1774,11 +1791,9 @@ async function handleSetup( // the user declined, and pre-existing nodes this build never touched. Both can hold at // once, so neither branch may swallow the other. Named "out of scope" rather than // "skipped" because in this file a skip is specifically a user decision. - const outOfScopeNodeNames = scopeNodeNames - ? allSetupRequests - .map((request) => request.node.name) - .filter((name) => !scopeNodeNames.includes(name)) - : []; + const outOfScopeNodeNames = allSetupRequests + .filter((request) => !scopedRequests.includes(request)) + .map((request) => request.node.name); const outOfScopeReason = outOfScopeNodeNames.length > 0 ? `Pre-existing node(s) ${outOfScopeNodeNames @@ -1837,10 +1852,11 @@ async function handleSetup( } // Re-analyze rather than remembering what was suspended: the closure state doesn't // survive a resume in another process, and a skip that silently fails to persist is - // the whole bug. Everything still needing setup is what the user just dismissed. - const dismissed = (await analyzeWorkflow(context, input.workflowId)).filter( - (request) => request.needsAction, - ); + // the whole bug. Everything in scope still needing setup is what the user just + // dismissed; a node the card left out was never offered, so it is not a skip. + const dismissed = ( + await filterToSetupScope(context, input, await analyzeWorkflow(context, input.workflowId)) + ).filter((request) => request.needsAction); await rememberSkippedSetup(context, dismissed, input.workflowId); return { success: true, diff --git a/packages/@n8n/instance-ai/src/tools/workflows/__tests__/build-workflow.tool.test.ts b/packages/@n8n/instance-ai/src/tools/workflows/__tests__/build-workflow.tool.test.ts index 9ccdc150981..1b5e7b8b3ed 100644 --- a/packages/@n8n/instance-ai/src/tools/workflows/__tests__/build-workflow.tool.test.ts +++ b/packages/@n8n/instance-ai/src/tools/workflows/__tests__/build-workflow.tool.test.ts @@ -1612,6 +1612,90 @@ describe('createBuildWorkflowTool', () => { }); }); + describe('setup scope across rebuilds', () => { + const pendingWebhook = { + node: { + id: 'webhook-1', + name: 'Webhook', + type: 'n8n-nodes-base.webhook', + typeVersion: 2, + parameters: {}, + position: [0, 0], + }, + parameterIssues: { path: ['Missing webhook path'] }, + isTrigger: true, + needsAction: true, + } as SetupRequest; + + // The first build creates the workflow, so every node is in scope. The rebuild + // leaves the pending node untouched. + async function rebuildUnchanged({ + workItemId, + nextWorkItemId = workItemId, + nextRunId, + failInBetween = false, + }: { + workItemId?: string; + nextWorkItemId?: string; + nextRunId?: string; + failInBetween?: boolean; + } = {}) { + vi.mocked(analyzeWorkflow).mockResolvedValue([pendingWebhook]); + const { context, filePath } = makeContext({ source: 'workflow source' }); + const tool = createBuildWorkflowTool(context); + await executeTool(tool, { filePath, workItemId, name: 'Webhook flow' }); + vi.mocked(context.workflowService.getAsWorkflowJSON).mockResolvedValue({ + ...structuredClone(generatedWorkflow), + name: 'Webhook flow', + }); + if (failInBetween) { + vi.mocked(compileWorkflowSource).mockResolvedValueOnce({ + success: false, + reason: 'workflow_json_invalid', + editable: true, + errors: ['Workflow JSON must include name, nodes, and connections.'], + summary: 'Workflow JSON source is missing required workflow fields.', + }); + expect(await executeTool(tool, { filePath })).toMatchObject({ + success: false, + }); + } + if (nextRunId) context.runId = nextRunId; + return await executeTool(tool, { filePath, workItemId: nextWorkItemId }); + } + + it.each([false, true])( + 'keeps pending setup for a node an earlier build in the same run created (failed rebuild in between: %s)', + async (failInBetween) => { + expect(await rebuildUnchanged({ failInBetween })).toMatchObject({ + success: true, + setupRequirement: { status: 'required' }, + }); + }, + ); + + it.each([ + ['a later run of the same work item', 'required', { workItemId: 'wi-1', nextRunId: 'run-2' }], + [ + 'another work item in the same run', + 'required', + { workItemId: 'wi-1', nextWorkItemId: 'wi-2' }, + ], + [ + 'another work item in a later run', + 'not_required', + { workItemId: 'wi-1', nextWorkItemId: 'wi-2', nextRunId: 'run-2' }, + ], + // Without a work item id, the file path stands in for it on every build. + ['a later run without a work item id', 'not_required', { nextRunId: 'run-2' }], + ])('rebuild in %s: setup %s', async (_label, status, options) => { + expect(await rebuildUnchanged(options)).toMatchObject({ + success: true, + setupRequirement: { status }, + }); + }); + }); + it('updates the workflow bound to the source file and remembers the binding', async () => { const { context, filePath, trackTelemetry } = makeContext({ source: 'workflow source' }); const tool = createBuildWorkflowTool(context); diff --git a/packages/@n8n/instance-ai/src/tools/workflows/build-workflow.tool.ts b/packages/@n8n/instance-ai/src/tools/workflows/build-workflow.tool.ts index 043aff7c68a..e3aecf3d41f 100644 --- a/packages/@n8n/instance-ai/src/tools/workflows/build-workflow.tool.ts +++ b/packages/@n8n/instance-ai/src/tools/workflows/build-workflow.tool.ts @@ -1483,13 +1483,28 @@ export function createBuildWorkflowTool(context: InstanceAiContext) { (t): t is { nodeName: string; nodeType: string } => Boolean(t.nodeName) && Boolean(t.nodeType), ); - // Setup routing is scoped to nodes this build actually changed: - // pre-existing nodes the build merely round-tripped must not route - // the user into setup for an unrelated edit. Undefined = unscoped - // (new workflow, or prior state unreadable). - const changedNodeNames = savedWorkflowSnapshot + // Setup routing is scoped to nodes this work item's builds actually + // changed: pre-existing nodes the build merely round-tripped must not + // route the user into setup for an unrelated edit. Earlier builds of the + // same work item (or the same run) stay in scope, so a fix that leaves a + // new node alone keeps its pending setup. Undefined = unscoped (workflow + // created by this work item, or prior state unreadable). + const runId = buildContext?.runId ?? context.runId; + // The file path is the fallback work item id. It stays the same across + // work items, so only the run can match then. + const scopeWorkItemId = resolvedWorkItemId === filePath ? undefined : resolvedWorkItemId; + const priorScope = binding.setupScope; + const sameWork = + (scopeWorkItemId !== undefined && priorScope?.workItemId === scopeWorkItemId) || + (runId !== undefined && priorScope?.runId === runId); + const buildChanges = savedWorkflowSnapshot ? computeChangedNodeNames(json, savedWorkflowSnapshot) : undefined; + const changedNodeNames = !sameWork + ? buildChanges + : priorScope?.nodeNames && buildChanges + ? [...new Set([...priorScope.nodeNames, ...buildChanges])] + : undefined; const isInSetupScope = (nodeName: string | undefined) => changedNodeNames === undefined || (nodeName !== undefined && changedNodeNames.includes(nodeName)); @@ -1555,7 +1570,6 @@ export function createBuildWorkflowTool(context: InstanceAiContext) { scriptedGateCount: waitGateScripts?.length ?? 0, savedWorkflowId: saved.id, }); - const runId = buildContext?.runId ?? context.runId; const workflowName = json.name || 'workflow'; const summary = `${operation === 'update' ? 'Updated' : 'Created'} ${isSupportingWorkflow ? 'supporting ' : ''}workflow "${workflowName}" (${saved.id}).`; binding = await saveWorkflowSourceFileBinding(context, { @@ -1565,6 +1579,7 @@ export function createBuildWorkflowTool(context: InstanceAiContext) { ...(saved.checksum ? { workflowChecksum: saved.checksum } : {}), sourceHash, setupPending: undefined, + setupScope: { workItemId: scopeWorkItemId, runId, nodeNames: changedNodeNames }, ...(setupPreferences ? { setupPreferences: { diff --git a/packages/@n8n/instance-ai/src/tools/workflows/workflow-file-bindings.ts b/packages/@n8n/instance-ai/src/tools/workflows/workflow-file-bindings.ts index 426a309c173..23eae56a2f6 100644 --- a/packages/@n8n/instance-ai/src/tools/workflows/workflow-file-bindings.ts +++ b/packages/@n8n/instance-ai/src/tools/workflows/workflow-file-bindings.ts @@ -18,6 +18,14 @@ const workflowSourceFileBindingSchema = z.object({ /** False when the source was read while parameter values were hidden. */ parameterValuesIncluded: z.boolean().optional(), setupPending: z.boolean().optional(), + /** Setup scope of the builds so far in this work item or run. `nodeNames` undefined = every node. */ + setupScope: z + .object({ + workItemId: z.string().optional(), + runId: z.string().optional(), + nodeNames: z.array(z.string()).optional(), + }) + .optional(), setupPreferences: z .object({ runId: z.string().optional(), diff --git a/packages/@n8n/instance-ai/src/workflow-loop/workflow-loop-state.ts b/packages/@n8n/instance-ai/src/workflow-loop/workflow-loop-state.ts index aad59edef81..71e88a55637 100644 --- a/packages/@n8n/instance-ai/src/workflow-loop/workflow-loop-state.ts +++ b/packages/@n8n/instance-ai/src/workflow-loop/workflow-loop-state.ts @@ -430,11 +430,12 @@ export const workflowBuildOutcomeSchema = z.object({ /** Whether any node parameters contain unresolved placeholder values. */ hasUnresolvedPlaceholders: z.boolean().optional(), /** - * Nodes this build added or modified relative to the previously saved - * workflow. Setup routing and `workflows(action="setup")` requests are - * scoped to these nodes so editing one node never routes pre-existing, - * unrelated nodes into setup. Absent when the build created the workflow - * or the prior state was unreadable — every node is then in scope. + * Nodes that this build and earlier builds of the same work item or run added + * or modified, each relative to the workflow saved before it. Setup routing and + * `workflows(action="setup")` requests are scoped to these nodes so editing one + * node never routes pre-existing, unrelated nodes into setup. Absent when one of + * those builds created the workflow or the prior state was unreadable — every + * node is then in scope. */ changedNodeNames: z.array(z.string()).optional(), /**