fix(automation): audit and fix the rules feature — required fields, trigger variables, filters - #61
Merged
Merged
Conversation
Five parallel auditors over the automation surface (canvas editor, rules
list, actions API, packages/systems/actions, bot executor/eventBridge/
registry), each finding put through an adversarial refutation pass.
62 raised, 47 confirmed, 5 refuted, 10 unverified.
Covers the three reported problems with root causes and file:line:
(A) required action fields don't block save — validateAction emits
"warning" and validation.valid counts errors only; the server
gate checks only action.type
(B) per-trigger variables reach autocomplete but not the action
Variables tab, and step-mode actions lose both tab and preview
(C) trigger filters fail *open* when the event context lacks the
datum, so exclude filters silently don't exclude
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Missing required action fields were reported at level "warning", and validateWorkflow computes `valid` from errors only — so Save stayed enabled and a rule that can never execute was persisted. It then sat in the rules list looking healthy and silently never fired. Client: promote the required-field branch in validateAction to "error". Genuinely advisory graph issues (empty condition value, unconnected delay, unreachable step) stay warnings so the split keeps meaning something. Server: validateRuleBody checked only that action.type was known plus the webhook URL scheme. Add validateActionConfig, driven by the same ACTION_TYPE_FIELDS descriptors the dashboard renders its form from, so the two cannot drift — and apply it to action *steps* as well, since the bot runs the step graph in preference to the flat action list. The server test file mocked @fluxcore/systems/actions/constants with an empty ACTION_TYPE_FIELDS and two action types, which made the contract invisible and let a test named "validates sendWebhook requires HTTPS URL" pass while posting no webhook at all. Constants are pure data with no I/O — unmock them and give the webhook case a real payload. +21 tests. Full dashboard suite 894/894. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…in step mode
The action panel's Variables tab rendered every token in the system
(Object.keys(constants.templateVariables)) regardless of the selected
trigger, so it contradicted both the autocomplete beside it — which was
correctly scoped via buildAutomationVariables — and the editor's own
unknown-token validator. A memberJoin rule offered {message.content}
and {ban.reason}, which the bot renders as "Unknown".
Worse, the step-mode action editor had no Variables tab and no message
preview at all. Adding a single condition or delay converts the rule to
step mode, which silently stripped both from every action in it.
Extract ActionSettings (type picker + fields + preview) and ActionTabs
(the Settings/Variables shell, scoped to the trigger) and use them from
both ActionPanel and StepPanel, so the two editors cannot diverge again.
Also withdraws finding B3 from the audit doc: VARIABLE_FIELD_KEYS turns
out to already cover every template-bearing field — the three outside it
(webhook.url, webhook.headers, emoji) should not accept templates.
+4 tests (new NodeDetailPanel.test.tsx). Full dashboard suite 898/898.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Filters were guarded on the presence of their own datum (`conditions.excludeRoleIds?.length && context.member`), so a filter the event context could not answer was silently *skipped*. The rule then fired on exactly the users and channels it had been configured to exclude, while the dashboard counted the filter as active: "Exclude -> Roles: @Staff" on Member Banned announced every staff ban, because a ban context carries no member object at all. An exclusion that cannot be evaluated is not permission to proceed. Both directions now fail closed: a configured filter whose datum is missing stops the rule. That makes it important not to offer a filter a trigger can never satisfy, so add EVENT_CONDITION_SUPPORT — the per-event map of which of channelId / member / userId the event bridge actually populates — as the single source of truth for the editor to read next. +8 executor tests, +9 support-matrix tests. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Companion to the fail-closed change: now that an unanswerable filter
stops a rule from firing, the editor must not let one be built. It reads
EVENT_CONDITION_SUPPORT (served on /api/actions/constants) and renders
only the filter groups the selected trigger can satisfy — no channel
filters on Member Join, no role filters on Member Banned, no user filter
on Channel Created. With no trigger picked yet it offers everything,
since the user is about to pick one.
Rules can still carry a filter from before their trigger was changed, so
those are surfaced rather than hidden: a warning naming the stranded
filters plus a one-click "remove filters that cannot apply" that clears
only the unusable keys.
Also gives include and exclude distinct labels ("Only in these channels"
vs "Never in these channels"). The two groups previously shared the
identical strings "Channels"/"Roles"/"Users", which left a screen-reader
user unable to tell them apart.
+8 ConditionsEditor tests, +1 constants-endpoint test.
Dashboard 907/907, bot automation 32/32, systems 337/337.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…cales Covers the 8 keys added by the event-aware filter work (the distinct include/exclude group labels, the unsupported-filter warning and its clear action) plus 3 that an earlier a11y pass had left English-only (collapse, userIdInvalid, addUserId). 47 locales x 11 keys. Every file is a pure insertion (+12/-1: the 11 keys plus the trailing comma on the previous last key) — no reformatting, verified against the semi-compact and \u-escaped files that a JSON round-trip would otherwise rewrite wholesale. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`conditions.removeItem` is the aria-label on each filter chip's remove
button ("Remove {{label}}"). It had shipped English in 38 of 48 locales,
so screen-reader users in those languages heard an English control name
inside an otherwise translated panel.
Pre-existing, but in the exact block the trigger-filter work touches, so
it is fixed here rather than left as a known gap. The remaining
identical-to-English strings (es/gl "Roles", fr "Conditions"/"active")
are correct in those languages, not leaks.
Every file is a one-line change.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
processEvent cannot tell "returned because there was nothing to do" from "succeeded", and every executor returned silently when it could not act. The ActionLog therefore recorded success for actions that never ran: a moderator whose auto-role rule did nothing saw "214 executions, 100% success" and had nothing to debug with. Executors now throw, with the reason, so the caller's existing catch logs success:false: - missing required config (channelId, message, roleId, nickname, emoji…) - an unresolvable / non-text / invisible channel - a DM that Discord refused, a nickname change without permission - an invalid emoji, which previously only warned addRole/removeRole additionally resolve the member from the guild when the context has none, the way setNickname already did. This is what broke the single most common automation people build — "react to this message, get a role" — since a reaction context carries no member. Condition steps evaluate hasRole/notHasRole BEFORE reading condition.field: those operators never consume the field, so the canonical "if member has @verified" branch always took the else path because the field defaults to channelId and memberJoin has none. Step mode's unknown-action-type branch warned and then fell through to the success log; it now throws. sendWebhook: follow redirects by hand so every hop is re-checked against the private-address rules (a public host answering "302 -> http://169.254.169.254/" previously walked straight into cloud metadata), cap at 3 hops, cap the drained response body at 64 KiB instead of buffering whatever the remote sends, and treat a non-2xx as a failure. +27 tests. Bot suite 400 passing; the 5 guildMemberAdd failures are pre-existing on the merge-base (that suite hits a real Postgres). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ally fire Three separate reasons an automation could never run at all. **No Partials.** ExtendedClient enabled none, so discord.js silently DROPPED any gateway event whose primary structure was not already cached. The bot only caches messages seen since its last restart, so reactionAdded / reactionRemoved / messageDeleted never reached a handler for an older message — which breaks every reaction-role automation attached to a pinned rules message, with no error anywhere. (The standalone messageReactionAdd handler already fetched partials; that code had simply never been reachable.) The event bridge now resolves the partial reaction and user before building a context, and treats an unfetchable deleted message as a drop rather than an error. **threadCreated could not be scoped.** context.channelId is the brand-new thread, an id no user could have picked, so a channel filter never matched. EventContext carries parentChannelId now and channel filters test both. **Announcement/stage/forum channels were invisible.** Three components each hardcoded `c.type === 0 || c.type === 2`, so a rule could not be scoped to an announcement channel and Send Message could not target one. Replaced with one shared `channelTypes.ts` (isMessageableChannel + per-type icons), used by DiscordMultiSelect and ActionFields. +13 tests. Dashboard client 470/470, bot 409 passing (5 pre-existing guildMemberAdd failures), all typechecks clean. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Four ways the canvas discarded edits with no warning.
**Changing an action's type erased every filled field.** handleTypeChange
reset to `{ type }`, so composing a long welcome message under Send
Message and switching to Send DM lost it the instant the dropdown closed
— both types declare `message` — with no undo, and the draft autosave
immediately persisting the emptied action. carryOverActionFields now
keeps any value whose key the new type also declares and drops the rest.
**Escape inside a text field closed the whole editor.** The hotkey layer
explicitly let Escape through from INPUT/TEXTAREA/SELECT, and Escape
with nothing selected is the close binding. Escape now belongs to the
focused control (blur and stop); only Ctrl/Cmd+S still passes through.
**Multi-select delete removed the wrong actions.** React Flow sends one
`remove` change per node and each was applied by index in a loop, but
every removal splices the array — deleting nodes 0 and 2 removed actions
0 and 3. Added handleActionsRemove to drop the whole set in one pass.
**Closing discarded unsaved edits silently.** The autosave wrote a draft
for every session, but loadDraft is only consulted for NEW rules, so
editing an existing rule produced an unread draft while the edits were
simply lost. Drafts are now written only where they can be read back,
and every close path (back button, Escape, tab close) goes through a
dirty check — an in-app confirm plus a beforeunload guard.
Also gives the action-type picker an accessible name; `<Label>` carried
no htmlFor, so the control had none at all.
i18n: 8 new keys plus 5 pre-existing draft keys that had shipped
English-only, translated across all 47 locales.
+16 tests. Dashboard 501/501.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…lls in
EVENT_TYPE_VARIABLES handed every trigger the same GENERAL_VARIABLES
block, so a Member Join rule advertised {channel} / {channel.name} and a
Channel Created rule advertised {user.name} — tokens the corresponding
event context never populates. The dashboard preview rendered "#general"
and "@ada" while the bot posted "Unknown Channel" and "Unknown", and the
editor's unknown-token warning stayed silent because the token looked
legitimate.
The list is now built from what eventBridge actually sets: guild tokens
everywhere, user tokens only where there is an acting user, channel
tokens only where the event happened in a channel, and threadCreated
limited to {user}/{user.id} because threadCreate resolves only ownerId.
A new test pins the map against the context builders, including a
cross-check that it agrees with EVENT_CONDITION_SUPPORT — both describe
the same question (what does this event carry) and must not drift apart.
+9 tests. Systems 346/346, dashboard 928/928.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…h field is missing `webhook.headers` is typed Record<string,string> but rendered as a plain textarea that wrote the raw string into it. Typing the exact JSON the placeholder showed produced "Expected object, received string" on save — English-only, naming no field and no node — and nothing the user could type would ever save. The reverse was as bad: a rule that already had headers (created via the API) opened showing the literal text "[object Object]", and saving overwrote the real headers with it. Adds a `json` descriptor type. The raw text lives in local state so a half-typed object stays typeable; only a successful parse into an object of string values is committed upward, and anything else sets aria-invalid with a translated message. Clearing the field yields undefined, not "". Separately, required fields had only a decorative red asterisk — no aria-invalid, no message — so a screen-reader user had no way to tell why Save was disabled. ActionFields now marks an empty required field invalid and links a message to it. +7 tests, 2 new keys across all 48 locales. Dashboard 935/935. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The step graph is what the canvas saves as soon as a rule has any condition or delay, and executor.ts prefers it over the flat action list — so it is the path that actually runs for every branching workflow. None of it was covered. 12 tests: traversal follows `next` rather than array order, the graph wins over a legacy action list, both condition branches, a null branch terminating cleanly, the iteration cap containing a cycle, a dangling `next`, one failing action not stopping the rest, delay steps, a graph with no entry step, trigger filters short-circuiting the whole graph, and condition outcomes reaching the execution log. Verified these can fail: swapping then/else fails 3, shrinking the iteration cap fails 1, and disabling the graph branch fails 9. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…names **Rule names were unguarded on the API.** The name is rendered into Discord embeds, `/actions` autocomplete and audit logs, and the bot command guards it with RULE_NAME_REGEX — but the dashboard route checked only length, so the stated injection protection was bypassable by anything not going through Discord. The API cannot reuse that regex: it is ASCII-only, and the preset rule templates produce non-ASCII names in 47 of 48 locales, so it would reject the product's own onboarding path. Added isSafeRuleName, which keeps the same protection (no markdown, code fences, mention syntax, control / zero-width / bidi-override characters) while accepting any script. **Trigger conditions were stored verbatim** — any key, any value. A malformed id can never match, so the rule silently never fires, and unknown keys accumulate forever. Now validated against the six known keys and the snowflake format. **A duplicate rule name surfaced Prisma's P2002 unhandled** as a 500 with a generic client message. Now a 409 that names the problem. Also unmocks @fluxcore/systems/actions/constants in updateRuleErrors, for the same reason as actions.test.ts: the stub silently dropped the route's newest import and the tests then failed for a reason they were never written to check. +15 tests. Dashboard 949/949, systems green. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The only aggregate validation signal was a tooltip on a non-focusable div. Keyboard and screen-reader users could not read it, the disabled Save button gave no reason at all, and with five actions there was no way to tell WHICH one was incomplete without opening each in turn. - The issue count is now a real button opening a popover, and each issue is a control that selects and centres the node it belongs to. - Save carries aria-describedby naming how many problems block it. - Condition and delay nodes surface their validation state at all: only ActionNode and TriggerNode rendered `warning`, so an empty condition value or an unconnected delay was invisible on the canvas while the toolbar counted it. - That state is now carried by a data attribute and an icon with an accessible label rather than border colour alone — a condition node's brand colour already IS amber, so a "warning" border was indistinguishable from its normal styling, and colour-only status fails WCAG 1.4.1 either way. - The trigger node shows how many filters are active; they were invisible outside the open detail panel, so a rule scoped to two channels looked identical to an unscoped one. - The rules-list event filter resets when its option disappears (it kept filtering on a value with no visible label, leaving an empty list) and sorts by translated label rather than raw key. - Preset templates are reachable again: the gallery only rendered while the guild had zero rules, so a user with one rule could never see them. +9 tests. Dashboard 958/958. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ilter selects
- The rule list now flags a rule that cannot execute. An action missing a
required field never runs, and the list showed it as perfectly healthy
— "Never fired", no explanation. Rules predating the save-time guard,
and anything created through the API or /actions, can still be in this
state, so detection uses the same ACTION_TYPE_FIELDS contract the
editor and the API validate against.
- Step mode enforced no ceiling at all: the toolbar happily added a 6th
action, a 4th condition or an 11th step, and the server then rejected
the finished rule in raw untranslated English. The buttons now carry
the same limits the API does.
- The autosave no longer writes an empty draft. It fired 500ms after
mount with the untouched initial state, so the next "Create Rule" was
greeted by a "draft restored" banner for a draft containing nothing.
- DiscordMultiSelect renders a <Label> that was never associated with
its trigger, so the four include/exclude comboboxes in the filter
panel had no accessible name and were indistinguishable. It also
shipped three hardcoded English strings ("Search...", "N selected",
"Remove X") despite being used across the whole dashboard.
+3 tests. Dashboard 505/505.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Filtering a rule by member meant pasting a raw 17-20 digit snowflake, and saved filters rendered as those same digits — because the dashboard had no members endpoint anywhere to build a picker on. Adds GET /api/guilds/:guildId/members, backed by Discord's members/search (the bot already declares the GUILD_MEMBERS intent it needs) and cached on the same short TTL as the channel and role lookups. `?q=` searches by name; `?ids=` resolves stored ids back to names. Rate-limited under a new `discordRead` tier — it fans out to Discord on our bot token and is reachable per keystroke, so the client debounces at 250ms and the server caps it at 60/min per session. MemberMultiSelect replaces UserIdInput in the trigger-filter panel: search by name, chips showing display names, and a member who has since left the guild still shows their raw id so the filter stays editable rather than silently losing an entry. +4 tests. Dashboard 965/965, both typechecks clean. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Every trigger name, action name, their descriptions, and every action field label and placeholder were hardcoded English in packages/systems/src/actions/constants.ts and served raw by /api/actions/constants — so the entire automation builder was English-only in an app with 48 locales. The trigger picker, the rules list, the canvas nodes, the logs table and the overview activity feed all rendered "Member Join" and "Send Message" regardless of locale. The data stays where it is (the bot needs it for embed titles and /actions autocomplete); the client now resolves it through i18n on the way out and falls back to the API's English when a key is missing, so an untranslated locale degrades to today's behaviour rather than blanks. Adds `labels.ts` with a pure `makeAutomationLabels` (for useWorkflowNodes, which already threads its own `t`) and a `useAutomationLabels` hook for components. The English keys are generated from the canonical constants so they cannot drift: 19 triggers, 10 actions, 19 field entries. Field keys contain dots, which i18next reads as nesting — escaped to underscores, pinned by a test. +7 tests. Dashboard 972/972, typechecks clean. Translating these keys into the other 47 locales follows. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
First batch of the automation vocabulary: 19 trigger labels and their descriptions for af, ar, bg, bn, ca, cs, da, de, el, es, et, eu, fa, fi, fil, fr — 608 strings. Locales not yet covered fall back to the API's English via makeAutomationLabels, so the UI stays correct throughout. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Audits the automation / action-rules feature end to end and fixes what it found. Findings recorded in
docs/superpowers/specs/2026-07-27-automation-rules-audit.md— 62 raised by five parallel auditors, each put through an adversarial refutation pass; 47 confirmed, 5 refuted.The three reported problems
(A) Action nodes didn't force required fields before save.
validateActionreported a missing required field atlevel: "warning", andvalidateWorkflowcomputesvalidfrom errors only — so Save stayed enabled. The server didn't compensate: it checked only thataction.typewas known. "Send Message with no channel" saved green, appeared healthy in the list, and never did anything. Five of the six preset templates shipped in exactly that state.Now an error on the client, and enforced server-side off the same
ACTION_TYPE_FIELDSdescriptors the form renders from — including action steps, since the bot runs the step graph in preference to the flat list.(B) Per-trigger variables didn't reach the action editor. The autocomplete was wired correctly; the reference list beside it wasn't — it rendered all 27 tokens regardless of trigger, contradicting both the autocomplete and the editor's own unknown-token validator. Worse, step-mode actions had no Variables tab and no preview at all, so adding a single condition stripped both from every action in the rule. Both editors now share one implementation.
(C) Trigger filters weren't just inconvenient — they were unreliable. Filters were guarded on the presence of their own datum, so a filter the event context couldn't answer was silently skipped:
Exclude → @Staffon a Member Banned rule announced every staff ban, because a ban context carries no member. Filters now fail closed, the editor only offers filters the selected trigger can satisfy (EVENT_CONDITION_SUPPORT), and rules carrying a stranded filter say so with a one-click fix.What else the audit found
The runtime was reporting phantom successes. Every executor returned silently when it couldn't act, and
processEventcannot tell that from success — so the ActionLog recordedsuccess: truefor actions that never ran. A moderator whose auto-role rule did nothing saw "214 executions, 100% success". Executors now throw with a reason.addRole/removeRoleadditionally resolve the member from the guild when the context has none, which is what broke the single most common automation people build: react to this message, get a role.Three classes of trigger could never fire.
threadCreated's context channel is the new thread, so a channel filter could never match; the parent now travels with the context.type === 0 || type === 2, making announcement, stage and forum channels invisible everywhere.The editor lost work four ways: changing an action's type erased every filled field, Escape inside a text box tore down the whole editor, a multi-select delete removed the wrong actions (stale indices after each splice), and closing discarded unsaved edits while writing a draft that was never read back.
Security: the webhook SSRF check ran once against the original hostname, so a public host answering
302 → http://169.254.169.254/walked straight into cloud metadata. Redirects are now followed by hand and re-validated per hop, capped at 3, with the drained response body capped at 64 KiB. Rule names were unguarded on the API (the injection protection existed only on the bot command).Validation became findable. The only aggregate signal was a tooltip on a non-focusable div; it's now a popover whose entries select and centre the offending node, Save explains why it's disabled, and the rule list flags a saved rule that cannot execute.
New:
GET /api/guilds/:guildId/membersand a real member picker — filtering by member previously meant pasting a raw 17–20 digit snowflake, because the dashboard had no members endpoint at all.Verification
+152 tests. The v2 step-graph execution path went from zero coverage to 12 tests, mutation-checked (swapping then/else fails 3; shrinking the iteration cap fails 1; disabling the graph branch fails 9).
The 5
guildMemberAddfailures in the bot suite are pre-existing on the merge-base — that suite hits a real Postgres. Verified by running it against a clean checkout ofc609ab1.Behaviour changes worth reviewing
Known remaining work
The automation vocabulary is now translatable — it was structurally impossible before, being hardcoded English served raw by the API. 16 of 47 locales have trigger names so far; 5,784 strings remain across trigger/action names, field labels and a pre-existing UI backlog. Untranslated locales fall back to English cleanly, so nothing is broken in the meantime.
🤖 Generated with Claude Code