From 5decab21ac29076e848fc45cee532622db76fc50 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ra=C3=BAl=20G=C3=B3mez=20Morales?= Date: Mon, 3 Aug 2026 15:44:37 +0200 Subject: [PATCH] fix(ai-builder): Treat bound stored credentials as settled in setup analysis (#35340) --- .../tools/__tests__/workflows.tool.test.ts | 45 ++++++++++++ .../instance-ai/src/tools/workflows.tool.ts | 8 ++- .../__tests__/setup-workflow.service.test.ts | 68 ++++++++++++++++++- .../tools/workflows/setup-workflow.service.ts | 41 ++++++++--- 4 files changed, 149 insertions(+), 13 deletions(-) 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 0a27a9e8545..67ef8958092 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 @@ -1279,6 +1279,51 @@ describe('workflows tool', () => { ['HTTP Request'], ); }); + + it('reports a just-applied credential whose test failed as a failed node', async () => { + // A bound credential is settled (needsAction=false) even when its test + // fails, so the apply path must re-analyze with includeSettled to keep + // the failure reportable instead of silently marking the node complete. + (analyzeWorkflow as Mock).mockResolvedValue([ + { + node: { name: 'Slack', type: 'n8n-nodes-base.slack' }, + credentialType: 'slackApi', + needsAction: false, + credentialTestResult: { success: false, message: 'Invalid token' }, + }, + ]); + (applyNodeChanges as Mock).mockResolvedValue({ applied: ['Slack'], failed: [] }); + (buildCompletedReport as Mock).mockReturnValue([ + { nodeName: 'Slack', credentialType: 'slackApi' }, + ]); + + const context = createMockContext(); + + const tool = createWorkflowsTool(context, 'full'); + const result = await executeTool(tool, { action: 'setup', workflowId: 'wf1' }, { + resumeData: { + approved: true, + action: 'apply', + credentials: { Slack: { slackApi: 'cred-1' } }, + }, + } as never); + + expect(analyzeWorkflow).toHaveBeenCalledWith(context, 'wf1', undefined, { + includeSettled: true, + }); + expect(result).toMatchObject({ + success: true, + completedNodes: [], + failedNodes: [ + { + nodeName: 'Slack', + error: 'Credential test failed for slackApi: Invalid token', + }, + ], + }); + // Settled requests never count as pending, so the apply is not partial. + expect(result).not.toHaveProperty('partial'); + }); }); describe('unpublish action', () => { diff --git a/packages/@n8n/instance-ai/src/tools/workflows.tool.ts b/packages/@n8n/instance-ai/src/tools/workflows.tool.ts index 152ef039220..58300568d95 100644 --- a/packages/@n8n/instance-ai/src/tools/workflows.tool.ts +++ b/packages/@n8n/instance-ai/src/tools/workflows.tool.ts @@ -654,8 +654,12 @@ async function handleSetupApply( // Re-analyze to determine if any nodes still need setup. // Filter by needsAction to distinguish "render this card" from - // "this still requires user intervention". - const remainingRequests = await analyzeWorkflow(context, input.workflowId); + // "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, + }); const pendingRequests = remainingRequests.filter((r) => r.needsAction); const completedNodes = buildCompletedReport( resumeData.credentials, diff --git a/packages/@n8n/instance-ai/src/tools/workflows/__tests__/setup-workflow.service.test.ts b/packages/@n8n/instance-ai/src/tools/workflows/__tests__/setup-workflow.service.test.ts index 28683334b8a..1ce9ae60d69 100644 --- a/packages/@n8n/instance-ai/src/tools/workflows/__tests__/setup-workflow.service.test.ts +++ b/packages/@n8n/instance-ai/src/tools/workflows/__tests__/setup-workflow.service.test.ts @@ -323,7 +323,7 @@ describe('buildSetupRequests', () => { expect(context.credentialService.test).not.toHaveBeenCalled(); }); - it('sets needsAction=true when credential test fails', async () => { + it('keeps needsAction=false when a bound stored credential fails its live test', async () => { (context.credentialService.list as Mock).mockResolvedValue([ { id: 'cred-1', name: 'My Slack', updatedAt: '2025-01-01T00:00:00.000Z' }, ]); @@ -337,6 +337,24 @@ describe('buildSetupRequests', () => { }); const result = await buildSetupRequests(context, node); + expect(result[0].needsAction).toBe(false); + expect(result[0].credentialTestResult).toEqual({ success: false, message: 'Invalid token' }); + }); + + it('sets needsAction=true when the bound credential id is not a stored credential', async () => { + (context.credentialService.list as Mock).mockResolvedValue([ + { id: 'cred-1', name: 'My Slack', updatedAt: '2025-01-01T00:00:00.000Z' }, + ]); + (context.credentialService.test as Mock).mockResolvedValue({ + success: false, + message: 'Credential not found', + }); + + const node = makeNode({ + credentials: { slackApi: { id: 'cred-gone', name: 'Imported Slack' } }, + }); + const result = await buildSetupRequests(context, node); + expect(result[0].needsAction).toBe(true); }); @@ -761,7 +779,7 @@ describe('analyzeWorkflow', () => { expect(result).toHaveLength(0); }); - it('keeps credential-only requests whose credential test fails', async () => { + it('drops credential-only requests whose bound stored credential fails its test', async () => { const node = makeNode({ credentials: { slackApi: { id: 'cred-1', name: 'My Slack' } }, }); @@ -780,6 +798,52 @@ describe('analyzeWorkflow', () => { const result = await analyzeWorkflow(context, 'wf-1'); + expect(result).toHaveLength(0); + }); + + it('keeps settled failing-test requests when includeSettled is set', async () => { + const node = makeNode({ + credentials: { slackApi: { id: 'cred-1', name: 'My Slack' } }, + }); + (context.workflowService.getAsWorkflowJSON as Mock).mockResolvedValue(makeWorkflowJSON([node])); + (context.nodeService.getDescription as Mock).mockResolvedValue({ + group: [], + credentials: [{ name: 'slackApi' }], + }); + (context.credentialService.list as Mock).mockResolvedValue([ + { id: 'cred-1', name: 'My Slack', updatedAt: '2025-01-01T00:00:00.000Z' }, + ]); + (context.credentialService.test as Mock).mockResolvedValue({ + success: false, + message: 'Invalid token', + }); + + const result = await analyzeWorkflow(context, 'wf-1', undefined, { includeSettled: true }); + + expect(result).toHaveLength(1); + expect(result[0].needsAction).toBe(false); + expect(result[0].credentialTestResult).toEqual({ success: false, message: 'Invalid token' }); + }); + + it('keeps credential-only requests whose bound credential id is not stored', async () => { + const node = makeNode({ + credentials: { slackApi: { id: 'cred-gone', name: 'Imported Slack' } }, + }); + (context.workflowService.getAsWorkflowJSON as Mock).mockResolvedValue(makeWorkflowJSON([node])); + (context.nodeService.getDescription as Mock).mockResolvedValue({ + group: [], + credentials: [{ name: 'slackApi' }], + }); + (context.credentialService.list as Mock).mockResolvedValue([ + { id: 'cred-1', name: 'My Slack', updatedAt: '2025-01-01T00:00:00.000Z' }, + ]); + (context.credentialService.test as Mock).mockResolvedValue({ + success: false, + message: 'Credential not found', + }); + + const result = await analyzeWorkflow(context, 'wf-1'); + expect(result).toHaveLength(1); expect(result[0].needsAction).toBe(true); }); diff --git a/packages/@n8n/instance-ai/src/tools/workflows/setup-workflow.service.ts b/packages/@n8n/instance-ai/src/tools/workflows/setup-workflow.service.ts index 8c1d4da1e3e..186e630a358 100644 --- a/packages/@n8n/instance-ai/src/tools/workflows/setup-workflow.service.ts +++ b/packages/@n8n/instance-ai/src/tools/workflows/setup-workflow.service.ts @@ -592,16 +592,27 @@ async function buildRequestForCredentialType( if (!credentialType && isTrigger && !isTestable && !hasParamIssues) return null; // Determine whether this request still needs user intervention. - // A credential request needs action if no credential is set or the test failed. + // A credential request needs action only when the slot leaves something to + // collect: nothing bound, or a bound id that doesn't resolve to a stored + // credential in scope (e.g. an imported workflow referencing another + // instance's credential). A resolvable bound credential is settled even if + // its live test fails — tests fail transiently and re-asking for an + // already-connected credential is redundant friction; the failure still + // rides along in credentialTestResult for display. // A parameter request needs action if issues remain. // A trigger-only request (no credential, no param issues) never blocks apply. let needsAction = false; if (credentialType) { const existingOnNode = node.credentials?.[credentialType]; - const hasValidCredential = - (typeof existingOnNode?.id === 'string' || isAiGatewayManagedCredential(existingOnNode)) && - (credentialTestResult === undefined || credentialTestResult.success); - needsAction = !hasValidCredential; + const boundId = + typeof existingOnNode?.id === 'string' && existingOnNode.id !== '' + ? existingOnNode.id + : undefined; + const isSettled = + isAiGatewayManagedCredential(existingOnNode) || + (boundId !== undefined && + existingCredentials.some((credential) => credential.id === boundId)); + needsAction = !isSettled; } if (hasParamIssues) { needsAction = true; @@ -1261,6 +1272,13 @@ export async function analyzeWorkflow( context: InstanceAiContext, workflowId: string, triggerResults?: Record, + options?: { + /** Keep settled requests (needsAction=false) in the result. For reporting + * consumers only (e.g. the apply path surfacing a just-applied credential + * whose test failed) — never for card rendering, where settled slots must + * stay hidden. */ + includeSettled?: boolean; + }, ): Promise { const workflowJson = await context.workflowService.getAsWorkflowJSON(workflowId); @@ -1285,11 +1303,16 @@ export async function analyzeWorkflow( req.isTrigger || (req.parameterIssues && Object.keys(req.parameterIssues).length > 0), ) - // Hide cards the user has nothing to do on: credentials already set and - // tested, no parameter issues, not a trigger awaiting testing. Trigger - // steps are always kept — triggers require user testing regardless of + // Hide cards the user has nothing to do on: credentials already set, + // no parameter issues, not a trigger awaiting testing. Trigger steps + // are always kept — triggers require user testing regardless of // credential state. - .filter((req) => !!req.needsAction || (req.isTrigger && !!req.isTestable)); + .filter( + (req) => + options?.includeSettled === true || + !!req.needsAction || + (req.isTrigger && !!req.isTestable), + ); sortByExecutionOrder( setupRequests,