From db69aa65090e59d97f3fcf5306ce2d35b8c3e733 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jos=C3=A9=20Braulio=20Gonz=C3=A1lez=20Valido?= Date: Tue, 19 May 2026 08:53:55 +0100 Subject: [PATCH] fix(core): Return plan-tool validator errors as tool results instead of throwing (#30592) Co-authored-by: Claude Opus 4.7 (1M context) --- .../src/planned-tasks/planned-task-service.ts | 26 ++++++--- .../orchestration/__tests__/plan.tool.test.ts | 53 +++++++++++++++++++ .../src/tools/orchestration/plan.tool.ts | 35 +++++++++--- 3 files changed, 100 insertions(+), 14 deletions(-) diff --git a/packages/@n8n/instance-ai/src/planned-tasks/planned-task-service.ts b/packages/@n8n/instance-ai/src/planned-tasks/planned-task-service.ts index dce1c597a13..6a567d64a0f 100644 --- a/packages/@n8n/instance-ai/src/planned-tasks/planned-task-service.ts +++ b/packages/@n8n/instance-ai/src/planned-tasks/planned-task-service.ts @@ -8,13 +8,27 @@ import type { PlannedTaskService, } from '../types'; +/** + * Thrown when a submitted task graph fails structural validation (duplicate + * IDs, unknown deps, missing checkpoint deps, dependency cycles, etc.). + * Callers — notably `plan.tool.ts` — should catch this specifically and surface + * the message back to the LLM so it can retry with a corrected graph. Storage, + * abort, or programming errors are NOT this class and must propagate. + */ +export class PlanValidationError extends Error { + constructor(message: string) { + super(message); + this.name = 'PlanValidationError'; + } +} + function hasDuplicateIds(tasks: PlannedTask[]): boolean { return new Set(tasks.map((task) => task.id)).size !== tasks.length; } function validateDependencies(tasks: PlannedTask[]): void { if (hasDuplicateIds(tasks)) { - throw new Error('Plan contains duplicate task IDs'); + throw new PlanValidationError('Plan contains duplicate task IDs'); } const knownIds = new Set(tasks.map((task) => task.id)); @@ -22,15 +36,15 @@ function validateDependencies(tasks: PlannedTask[]): void { for (const task of tasks) { for (const depId of task.deps) { if (!knownIds.has(depId)) { - throw new Error(`Task "${task.id}" depends on unknown task "${depId}"`); + throw new PlanValidationError(`Task "${task.id}" depends on unknown task "${depId}"`); } } if (task.kind === 'delegate' && (!task.tools || task.tools.length === 0)) { - throw new Error(`Delegate task "${task.id}" must include at least one tool`); + throw new PlanValidationError(`Delegate task "${task.id}" must include at least one tool`); } if (task.kind === 'checkpoint') { if (task.deps.length === 0) { - throw new Error( + throw new PlanValidationError( `Checkpoint task "${task.id}" must depend on at least one build-workflow task`, ); } @@ -38,7 +52,7 @@ function validateDependencies(tasks: PlannedTask[]): void { (depId) => byId.get(depId)?.kind === 'build-workflow', ); if (!dependsOnBuildWorkflow) { - throw new Error( + throw new PlanValidationError( `Checkpoint task "${task.id}" must depend on at least one build-workflow task`, ); } @@ -51,7 +65,7 @@ function validateDependencies(tasks: PlannedTask[]): void { const visit = (taskId: string) => { if (visited.has(taskId)) return; if (visiting.has(taskId)) { - throw new Error(`Plan contains a dependency cycle involving "${taskId}"`); + throw new PlanValidationError(`Plan contains a dependency cycle involving "${taskId}"`); } visiting.add(taskId); diff --git a/packages/@n8n/instance-ai/src/tools/orchestration/__tests__/plan.tool.test.ts b/packages/@n8n/instance-ai/src/tools/orchestration/__tests__/plan.tool.test.ts index fcb1c51ecc7..1219984afac 100644 --- a/packages/@n8n/instance-ai/src/tools/orchestration/__tests__/plan.tool.test.ts +++ b/packages/@n8n/instance-ai/src/tools/orchestration/__tests__/plan.tool.test.ts @@ -1,3 +1,4 @@ +import { PlanValidationError } from '../../../planned-tasks/planned-task-service'; import type { OrchestrationContext, PlannedTaskService, TaskStorage } from '../../../types'; // Mock heavy Mastra dependencies to avoid ESM issues in Jest @@ -349,3 +350,55 @@ describe('createPlanTool — replan-only guard', () => { expect(context.plannedTaskService!.createPlan).not.toHaveBeenCalled(); }); }); + +describe('createPlanTool — createPlan validation failures', () => { + it('returns a PlanValidationError as a tool result instead of throwing', async () => { + const validatorError = new PlanValidationError( + 'Checkpoint task "chk-1" must depend on at least one build-workflow task', + ); + const context = createMockContext({ + currentUserMessage: 'replan after failure', + plannedTaskService: makePlannedTaskService({ + createPlan: jest.fn().mockRejectedValue(validatorError), + }), + }); + const tool = createPlanTool(context) as unknown as Executable; + const suspend = jest.fn(); + + const out = await tool.execute( + { + tasks: validTasks(), + skipPlannerDiscovery: true, + reason: 'bypass for test', + }, + { agent: { suspend } }, + ); + + expect(out.taskCount).toBe(0); + expect(out.result).toContain(validatorError.message); + expect(out.result).toContain('Revise the task graph and call this tool again'); + expect(suspend).not.toHaveBeenCalled(); + expect(context.logger.warn).toHaveBeenCalledWith( + 'plan tool: createPlan rejected by validator', + expect.objectContaining({ threadId: 'test-thread', error: validatorError.message }), + ); + }); + + it('propagates non-validation errors (storage, abort, bugs)', async () => { + const storageError = new Error('connection refused'); + const context = createMockContext({ + currentUserMessage: 'replan', + plannedTaskService: makePlannedTaskService({ + createPlan: jest.fn().mockRejectedValue(storageError), + }), + }); + const tool = createPlanTool(context) as unknown as Executable; + + await expect( + tool.execute( + { tasks: validTasks(), skipPlannerDiscovery: true, reason: 'bypass' }, + { agent: { suspend: jest.fn() } }, + ), + ).rejects.toBe(storageError); + }); +}); diff --git a/packages/@n8n/instance-ai/src/tools/orchestration/plan.tool.ts b/packages/@n8n/instance-ai/src/tools/orchestration/plan.tool.ts index a2337d0e6d4..ec05a0692e4 100644 --- a/packages/@n8n/instance-ai/src/tools/orchestration/plan.tool.ts +++ b/packages/@n8n/instance-ai/src/tools/orchestration/plan.tool.ts @@ -3,6 +3,7 @@ import { taskListSchema } from '@n8n/api-types'; import { nanoid } from 'nanoid'; import { z } from 'zod'; +import { PlanValidationError } from '../../planned-tasks/planned-task-service'; import type { OrchestrationContext, PlannedTask } from '../../types'; const plannedTaskSchema = z.object({ @@ -164,14 +165,32 @@ export function createPlanTool(context: OrchestrationContext) { // First call — persist plan, show to user, suspend for approval if (isFirstCall) { - await context.plannedTaskService.createPlan( - context.threadId, - input.tasks as PlannedTask[], - { - planRunId: context.runId, - messageGroupId: context.messageGroupId, - }, - ); + try { + await context.plannedTaskService.createPlan( + context.threadId, + input.tasks as PlannedTask[], + { + planRunId: context.runId, + messageGroupId: context.messageGroupId, + }, + ); + } catch (error) { + // Surface only validator rejections back to the LLM as a tool result + // so it can re-call with a corrected graph. Storage failures, abort + // signals, and bugs propagate. + if (!(error instanceof PlanValidationError)) { + throw error; + } + context.logger.warn('plan tool: createPlan rejected by validator', { + threadId: context.threadId, + taskCount: input.tasks.length, + error: error.message, + }); + return { + result: `Error: ${error.message}. Revise the task graph and call this tool again.`, + taskCount: 0, + }; + } // Emit tasks-update so the checklist appears in the chat immediately const taskItems = input.tasks.map((t: z.infer) => ({