fix(automation): audit and fix the rules feature — required fields, trigger variables, filters

apps/bot/src/features/automation/system/eventBridge.ts

@@ -211,7 +211,7 @@ export function registerActionEventListeners(client: Client): void {
 
   // Message created
   client.on("messageCreate", (message) => {
-    if (!message.guildId || message.author.bot) return;
+    if (!message.guildId || message.author?.bot) return;
     if (getRulesForEvent(message.guildId, "messageCreated").length === 0) return;
     const ctx = buildMessageContext("messageCreated", message);
     processEvent(client, ctx).catch((e) =>
@@ -229,24 +229,35 @@ export function registerActionEventListeners(client: Client): void {
     );
   });
 
-  // Reaction added/removed
-  client.on("messageReactionAdd", (reaction, user) => {
-    if (!reaction.message.guildId || user.bot) return;
-    if (getRulesForEvent(reaction.message.guildId, "reactionAdded").length === 0) return;
-    const ctx = buildReactionContext("reactionAdded", reaction, user);
-    processEvent(client, ctx).catch((e) =>
-      handleError("reactionAdded", e),
-    );
-  });
+  // Reaction added/removed.
+  //
+  // Partials are enabled on the client, so these fire for messages the bot has
+  // never cached — which is the normal case for a pinned rules message. Both
+  // the reaction and the user must be resolved before the context is built, or
+  // every reaction automation on an older message stays silently broken.
+  const onReaction = (
+    eventType: "reactionAdded" | "reactionRemoved",
+  ) => async (
+    reaction: MessageReaction | PartialMessageReaction,
+    user: User | PartialUser,
+  ) => {
+    try {
+      if (user.bot) return;
+      if (reaction.partial) await reaction.fetch();
+      if (user.partial) user = await user.fetch();
+      const guildId = reaction.message.guildId;
+      if (!guildId) return;
+      if (getRulesForEvent(guildId, eventType).length === 0) return;
+      await processEvent(client, buildReactionContext(eventType, reaction, user));
+    } catch (e) {
+      // A deleted message cannot be fetched back; that is not an error worth
+      // surfacing, but anything else is.
+      handleError(eventType, e);
+    }
+  };
 
-  client.on("messageReactionRemove", (reaction, user) => {
-    if (!reaction.message.guildId || user.bot) return;
-    if (getRulesForEvent(reaction.message.guildId, "reactionRemoved").length === 0) return;
-    const ctx = buildReactionContext("reactionRemoved", reaction, user);
-    processEvent(client, ctx).catch((e) =>
-      handleError("reactionRemoved", e),
-    );
-  });
+  client.on("messageReactionAdd", onReaction("reactionAdded"));
+  client.on("messageReactionRemove", onReaction("reactionRemoved"));
 
   // Role added/removed + nickname/timeout/boost (all via guildMemberUpdate)
   client.on("guildMemberUpdate", (oldMember, newMember) => {
@@ -379,6 +390,10 @@ export function registerActionEventListeners(client: Client): void {
       channelId: thread.id,
       channelName: thread.name,
       channelMention: `<#${thread.id}>`,
+      // The thread IS the context channel, so a channel filter would never
+      // match anything a user could pick. Carry the parent so
+      // "only threads under #support" is expressible.
+      parentChannelId: thread.parentId ?? undefined,
       memberCount: thread.guild.memberCount,
       timestamp: new Date().toISOString(),
       extra: {

apps/bot/src/features/automation/system/executor.ts

@@ -46,32 +46,60 @@ function checkRateLimit(guildId: string, eventType: string): boolean {
 
 // --- Condition matching ---
 
+/**
+ * Trigger filters FAIL CLOSED: a configured filter the event context cannot
+ * answer means the rule does not fire.
+ *
+ * These guards used to read `conditions.excludeRoleIds?.length &&
+ * context.member`, which silently *skipped* an unanswerable filter. The rule
+ * then fired on exactly the users and channels it was configured to exclude —
+ * "exclude @Staff" on a Member Banned rule announced every staff ban, because
+ * a ban context carries no member — while the dashboard counted the filter as
+ * active. An exclusion that cannot be evaluated is not a permission to
+ * proceed; it is a reason to stop.
+ *
+ * The editor is event-aware (EVENT_CONDITION_SUPPORT) so new rules cannot be
+ * built with a filter their trigger can never evaluate.
+ */
 function matchesConditions(
   conditions: ActionConditions,
   context: EventContext,
 ): boolean {
-  // Include filters: if specified, context must match
-  if (conditions.channelIds?.length && context.channelId) {
-    if (!conditions.channelIds.includes(context.channelId)) return false;
+  // Channel filters test the parent too. On threadCreated the context channel
+  // is the brand-new thread, whose id the user cannot possibly have picked in
+  // the editor; the parent is the channel they actually chose.
+  const channelIds = [context.channelId, context.parentChannelId].filter(
+    (id): id is string => !!id,
+  );
+
+  // Include filters: the context must carry the datum AND match it.
+  if (conditions.channelIds?.length) {
+    if (channelIds.length === 0) return false;
+    if (!channelIds.some((id) => conditions.channelIds!.includes(id))) return false;
   }
-  if (conditions.userIds?.length && context.userId) {
+  if (conditions.userIds?.length) {
+    if (!context.userId) return false;
     if (!conditions.userIds.includes(context.userId)) return false;
   }
-  if (conditions.roleIds?.length && context.member) {
+  if (conditions.roleIds?.length) {
+    if (!context.member) return false;
     const hasMatchingRole = conditions.roleIds.some((id) =>
       context.member!.roles.cache.has(id),
     );
     if (!hasMatchingRole) return false;
   }
 
-  // Exclude filters: if matched, skip
-  if (conditions.excludeChannelIds?.length && context.channelId) {
-    if (conditions.excludeChannelIds.includes(context.channelId)) return false;
+  // Exclude filters: an unanswerable exclusion cannot be cleared, so it blocks.
+  if (conditions.excludeChannelIds?.length) {
+    if (channelIds.length === 0) return false;
+    if (channelIds.some((id) => conditions.excludeChannelIds!.includes(id))) return false;
   }
-  if (conditions.excludeUserIds?.length && context.userId) {
+  if (conditions.excludeUserIds?.length) {
+    if (!context.userId) return false;
     if (conditions.excludeUserIds.includes(context.userId)) return false;
   }
-  if (conditions.excludeRoleIds?.length && context.member) {
+  if (conditions.excludeRoleIds?.length) {
+    if (!context.member) return false;
     const hasExcludedRole = conditions.excludeRoleIds.some((id) =>
       context.member!.roles.cache.has(id),
     );
@@ -111,9 +139,21 @@ function evaluateCondition(
   condition: StepConditionConfig,
   context: EventContext,
 ): boolean {
-  const actual = getContextValue(condition.field, context);
   const expected = condition.value;
 
+  // Role operators are answered entirely by the member's roles — they never
+  // consume `condition.field`. Checking the field first meant the canonical
+  // "if member has @Verified" branch always took the else path, because the
+  // field defaults to channelId and memberJoin never populates one.
+  if (condition.operator === "hasRole") {
+    return context.member?.roles.cache.has(expected) ?? false;
+  }
+  if (condition.operator === "notHasRole") {
+    if (!context.member) return false;
+    return !context.member.roles.cache.has(expected);
+  }
+
+  const actual = getContextValue(condition.field, context);
   if (actual === undefined || actual === null) return false;
 
   const actualStr = String(actual);
@@ -137,10 +177,6 @@ function evaluateCondition(
       return Number(actual) > Number(expected);
     case "lessThan":
       return Number(actual) < Number(expected);
-    case "hasRole":
-      return context.member?.roles.cache.has(expected) ?? false;
-    case "notHasRole":
-      return !(context.member?.roles.cache.has(expected) ?? true);
     case "inList":
       return expected
         .split(",")
@@ -188,12 +224,12 @@ async function executeSteps(
         try {
           const executor = getExecutor(step.action.type as ActionType);
           if (!executor) {
-            logger.warn(
-              `Unknown action type: ${step.action.type} in rule "${rule.name}"`,
-            );
-          } else {
-            await executor(client, context, step.action);
+            // Must not fall through to the success log below: a rule
+            // referencing a removed action type would report a clean 100%
+            // success rate while doing nothing at all.
+            throw new Error(`Unknown action type: ${step.action.type}`);
           }
+          await executor(client, context, step.action);
           logExecution(rule, step.action.type, true, null).catch(() => {});
         } catch (error) {
           const err =