mirror of
https://github.com/n8n-io/n8n.git
synced 2026-05-28 23:37:00 +02:00
fix(core): Return plan-tool validator errors as tool results instead of throwing (#30592)
Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This commit is contained in:
parent
47ffd1cc07
commit
db69aa6509
|
|
@ -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);
|
||||
|
|
|
|||
|
|
@ -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);
|
||||
});
|
||||
});
|
||||
|
|
|
|||
|
|
@ -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<typeof plannedTaskSchema>) => ({
|
||||
|
|
|
|||
Loading…
Reference in New Issue
Block a user