Merge pull request #41 from ArnasDon/fix/automations-validate-step-config-on-activate

fix(automations): reject activation of invalid step/trigger config

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<string, unknown> }[])
+      : 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')

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<string, unknown> }[],
+      ),
+    ]
+    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')

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) {
+        // 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")

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<string, unknown>
+  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<string, unknown>
+
+  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
+}