From 570fc1234c727a856e3a6c0254752f6f7aeeaf87 Mon Sep 17 00:00:00 2001 From: Arnas Donauskas Date: Fri, 17 Apr 2026 21:50:13 +0300 Subject: [PATCH] fix(automations): reject activation of invalid step/trigger config MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Activating an automation with obviously-broken config (add_tag step with empty tag_id, send_message with empty text, keyword_match trigger with no keywords, etc.) used to succeed — and then every dispatch produced a cryptic failed log row that users often missed. - New validate.ts centralises the checks (mirrors engine.ts's runtime guards one layer earlier so failures surface at save time). Returns { path, message } issues so the UI can point users at the exact problem. - POST /api/automations rejects with 400 when is_active === true and the merged config fails validation. - PATCH /api/automations/[id] computes the post-patch "effective" state (merging body fields over the existing row, loading current steps if body.steps isn't provided) and applies the same gate. Drafts (is_active=false) remain free to be incomplete so users can still save progress mid-build. - Builder surfaces the first issue in a toast with path as the description, so users don't need DevTools to find what to fix. Co-Authored-By: Claude Opus 4.7 (1M context) --- src/app/api/automations/[id]/route.ts | 36 +++- src/app/api/automations/route.ts | 23 +++ .../automations/automation-builder.tsx | 13 +- src/lib/automations/validate.ts | 178 ++++++++++++++++++ 4 files changed, 247 insertions(+), 3 deletions(-) create mode 100644 src/lib/automations/validate.ts diff --git a/src/app/api/automations/[id]/route.ts b/src/app/api/automations/[id]/route.ts index 8e905fa..5ca0ab9 100644 --- a/src/app/api/automations/[id]/route.ts +++ b/src/app/api/automations/[id]/route.ts @@ -6,6 +6,10 @@ import { replaceSteps, type BuilderStepInput, } from '@/lib/automations/steps-tree' +import { + validateStepsForActivation, + validateTriggerForActivation, +} from '@/lib/automations/validate' async function requireUser() { const supabase = await createClient() @@ -51,10 +55,11 @@ export async function PATCH( const admin = supabaseAdmin() - // Ownership check before we touch anything. + // Ownership check before we touch anything. Load the fields we need + // to compute the post-patch "effective" state for validation. const { data: existing } = await admin .from('automations') - .select('id, user_id') + .select('id, user_id, is_active, trigger_type, trigger_config') .eq('id', id) .maybeSingle() if (!existing || existing.user_id !== user.id) { @@ -72,6 +77,33 @@ export async function PATCH( if (k in body) update[k] = body[k] } + // If this PATCH leaves the automation active (either explicitly + // activating it OR editing an already-active one), validate the + // merged configuration first. Activation is the natural gate — drafts + // are still allowed to be incomplete. + const willBeActive = + typeof update.is_active === 'boolean' ? update.is_active : existing.is_active + if (willBeActive) { + const mergedTriggerType = (update.trigger_type ?? existing.trigger_type) as string + const mergedTriggerConfig = update.trigger_config ?? existing.trigger_config + const mergedSteps = Array.isArray(body.steps) + ? (body.steps as { step_type: string; step_config: Record }[]) + : await loadStepsTree(id) + const issues = [ + ...validateTriggerForActivation(mergedTriggerType, mergedTriggerConfig), + ...validateStepsForActivation(mergedSteps), + ] + if (issues.length > 0) { + return NextResponse.json( + { + error: 'Cannot keep automation active with invalid configuration', + issues, + }, + { status: 400 }, + ) + } + } + if (Object.keys(update).length > 0) { const { error: updErr } = await admin .from('automations') diff --git a/src/app/api/automations/route.ts b/src/app/api/automations/route.ts index 63e914a..d3125cc 100644 --- a/src/app/api/automations/route.ts +++ b/src/app/api/automations/route.ts @@ -3,6 +3,10 @@ import { createClient } from '@/lib/supabase/server' import { supabaseAdmin } from '@/lib/automations/admin-client' import { getTemplate } from '@/lib/automations/templates' import { insertSteps, type BuilderStepInput } from '@/lib/automations/steps-tree' +import { + validateStepsForActivation, + validateTriggerForActivation, +} from '@/lib/automations/validate' export async function GET() { const supabase = await createClient() @@ -55,6 +59,25 @@ export async function POST(request: Request) { ) } + // Block activation of a clearly broken automation up-front instead of + // letting every trigger silently produce a failed log row. Drafts + // (is_active=false) are allowed to be incomplete so users can save + // progress mid-build. + if (is_active) { + const issues = [ + ...validateTriggerForActivation(effectiveTriggerType, effectiveTriggerConfig ?? {}), + ...validateStepsForActivation( + (effectiveSteps ?? []) as unknown as { step_type: string; step_config: Record }[], + ), + ] + if (issues.length > 0) { + return NextResponse.json( + { error: 'Cannot activate automation with invalid configuration', issues }, + { status: 400 }, + ) + } + } + const admin = supabaseAdmin() const { data: automation, error: insertErr } = await admin .from('automations') diff --git a/src/components/automations/automation-builder.tsx b/src/components/automations/automation-builder.tsx index 6deaa67..b9b7245 100644 --- a/src/components/automations/automation-builder.tsx +++ b/src/components/automations/automation-builder.tsx @@ -221,7 +221,18 @@ export function AutomationBuilder({ initial }: { initial: BuilderInitial }) { const body = await res.json().catch(() => ({})) if (!res.ok) { - toast.error(body?.error ?? "Save failed") + // If the server blocked activation with validation issues, + // surface the first concrete problem so the user can fix it + // without opening DevTools for the full array. + const firstIssue: { path?: string; message?: string } | undefined = + body?.issues?.[0] + if (firstIssue?.message) { + toast.error(firstIssue.message, { + description: firstIssue.path ? `at ${firstIssue.path}` : undefined, + }) + } else { + toast.error(body?.error ?? "Save failed") + } return } toast.success(isEditing ? "Automation saved" : "Automation created") diff --git a/src/lib/automations/validate.ts b/src/lib/automations/validate.ts new file mode 100644 index 0000000..707d685 --- /dev/null +++ b/src/lib/automations/validate.ts @@ -0,0 +1,178 @@ +import type { AutomationTriggerType } from '@/types' + +// ------------------------------------------------------------ +// Pre-flight config validation for automations about to be activated. +// +// Activating a broken automation (e.g. an add_tag step with tag_id="") +// used to succeed silently — every trigger then produced a failed log +// row with a cryptic "add_tag needs contact + tag_id" message, and +// users often didn't notice until reviewing logs. This module lets +// the API refuse activation with a useful 400 response instead. +// +// The rules here mirror the runtime checks in engine.ts's runStep; +// they're the same invariants, enforced one step earlier so failures +// surface at save time. +// ------------------------------------------------------------ + +export interface ValidationIssue { + /** Dot-path for the UI to highlight; stable enough to build a table. */ + path: string + message: string +} + +interface StepLike { + step_type: string + step_config: Record + branches?: { yes?: StepLike[]; no?: StepLike[] } +} + +export function validateStepsForActivation(steps: StepLike[]): ValidationIssue[] { + const issues: ValidationIssue[] = [] + if (!Array.isArray(steps) || steps.length === 0) { + issues.push({ + path: 'steps', + message: 'active automations need at least one step', + }) + return issues + } + walk(steps, '', issues) + return issues +} + +function walk(steps: StepLike[], prefix: string, issues: ValidationIssue[]): void { + steps.forEach((s, i) => { + const path = `${prefix}steps[${i}]` + validateOne(s, path, issues) + if (s.step_type === 'condition' && s.branches) { + if (s.branches.yes) walk(s.branches.yes, `${path}.yes.`, issues) + if (s.branches.no) walk(s.branches.no, `${path}.no.`, issues) + } + }) +} + +function validateOne(step: StepLike, path: string, issues: ValidationIssue[]): void { + const c = step.step_config ?? {} + switch (step.step_type) { + case 'send_message': + if (!nonEmpty(c.text)) { + issues.push({ path: `${path}.text`, message: 'message text is required' }) + } + break + case 'send_template': + if (!nonEmpty(c.template_name)) { + issues.push({ path: `${path}.template_name`, message: 'template name is required' }) + } + break + case 'add_tag': + case 'remove_tag': + if (!nonEmpty(c.tag_id)) { + issues.push({ path: `${path}.tag_id`, message: 'tag is required' }) + } + break + case 'assign_conversation': + if (c.mode === 'specific' && !nonEmpty(c.agent_id)) { + issues.push({ + path: `${path}.agent_id`, + message: 'agent is required when mode is "specific"', + }) + } + break + case 'update_contact_field': + if (!nonEmpty(c.field)) { + issues.push({ path: `${path}.field`, message: 'field name is required' }) + } + if (c.value === undefined || c.value === null || c.value === '') { + issues.push({ path: `${path}.value`, message: 'field value is required' }) + } + break + case 'create_deal': + if (!nonEmpty(c.pipeline_id)) { + issues.push({ path: `${path}.pipeline_id`, message: 'pipeline is required' }) + } + if (!nonEmpty(c.stage_id)) { + issues.push({ path: `${path}.stage_id`, message: 'stage is required' }) + } + if (!nonEmpty(c.title)) { + issues.push({ path: `${path}.title`, message: 'title is required' }) + } + break + case 'wait': + if (typeof c.amount !== 'number' || !Number.isFinite(c.amount) || c.amount <= 0) { + issues.push({ path: `${path}.amount`, message: 'wait amount must be greater than 0' }) + } + if (!['minutes', 'hours', 'days'].includes(String(c.unit))) { + issues.push({ + path: `${path}.unit`, + message: 'wait unit must be minutes, hours, or days', + }) + } + break + case 'condition': + if (!nonEmpty(c.subject)) { + issues.push({ path: `${path}.subject`, message: 'condition subject is required' }) + } + if (!nonEmpty(c.operand)) { + issues.push({ path: `${path}.operand`, message: 'condition operand is required' }) + } + break + case 'send_webhook': + if (!nonEmpty(c.url)) { + issues.push({ path: `${path}.url`, message: 'webhook URL is required' }) + break + } + try { + const u = new URL(String(c.url)) + if (u.protocol !== 'http:' && u.protocol !== 'https:') { + issues.push({ + path: `${path}.url`, + message: 'webhook URL must use http or https', + }) + } + } catch { + issues.push({ path: `${path}.url`, message: 'webhook URL is not a valid URL' }) + } + break + case 'close_conversation': + // No config required. + break + default: + issues.push({ path, message: `unknown step type: ${step.step_type}` }) + } +} + +export function validateTriggerForActivation( + triggerType: AutomationTriggerType | string, + triggerConfig: unknown, +): ValidationIssue[] { + const issues: ValidationIssue[] = [] + const cfg = (triggerConfig ?? {}) as Record + + if (triggerType === 'keyword_match') { + const k = cfg.keywords + if (!Array.isArray(k) || k.length === 0) { + issues.push({ path: 'trigger.keywords', message: 'at least one keyword is required' }) + } else if (k.some((v) => typeof v !== 'string' || v.trim() === '')) { + issues.push({ path: 'trigger.keywords', message: 'keywords cannot be empty strings' }) + } + if (cfg.match_type !== 'exact' && cfg.match_type !== 'contains') { + issues.push({ + path: 'trigger.match_type', + message: 'match type must be "exact" or "contains"', + }) + } + } else if (triggerType === 'time_based') { + if (!nonEmpty(cfg.schedule)) { + issues.push({ path: 'trigger.schedule', message: 'schedule is required' }) + } + } else if (triggerType === 'tag_added') { + if (!nonEmpty(cfg.tag_id)) { + issues.push({ path: 'trigger.tag_id', message: 'tag is required' }) + } + } + + return issues +} + +function nonEmpty(v: unknown): boolean { + return typeof v === 'string' && v.trim().length > 0 +}