mirror of
https://github.com/n8n-io/n8n.git
synced 2026-08-30 18:01:23 +08:00
fix(ai-builder): Treat bound stored credentials as settled in setup analysis (#35340)
This commit is contained in:
committed by
GitHub
parent
05a809867e
commit
5decab21ac
@@ -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', () => {
|
||||
|
||||
@@ -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,
|
||||
|
||||
+66
-2
@@ -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);
|
||||
});
|
||||
|
||||
@@ -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<string, { status: 'success' | 'error' | 'listening'; error?: string }>,
|
||||
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<SetupRequest[]> {
|
||||
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,
|
||||
|
||||
Reference in New Issue
Block a user