Uh oh!
There was an error while loading. Please reload this page.
feat(integrations): support chatRoomFilter and chat rule types - #433
Conversation
The latest updates on your projects. Learn more about Vercel for GitHub.
|
e0c5230 to
f0ef330Compareb604c58 to
c2e85adCompare
umair-ably
left a comment
There was a problem hiding this comment.
just a couple things, nothing major
| } | ||
| // Update target if it's an HTTP rule and target-url is provided | ||
| if (existingRule.ruleType === "http" && flags["target-url"]) { |
There was a problem hiding this comment.
does this need updating to work with but http/before-publish? that also takes a target-url which can't be updated with this guard
| `${formatLabel("Invocation Mode")} ${integration.invocationMode}`, | ||
| ); | ||
| } | ||
There was a problem hiding this comment.
more of nit... we seem to guard and log the chatRoomFilter in get.ts line 69... any reason not to have it here too?
| source: { type: "chat.message" }, | ||
| target: { | ||
| apiKey: "hive-key", | ||
| modelUrl: null, |
There was a problem hiding this comment.
should we test whether we're actually sending this or not? this just mocks the response?
c2e85ad to
3b29706CompareWalkthroughThis PR adds full support for Ably Chat integration rule types in the Changes
Review Notes
|
There was a problem hiding this comment.
Review Summary
Overall this is solid work — the investigation against the real API is clearly thorough, the data-table approach for chat rule types is a clean design, and the e2e coverage is impressive. A few things worth fixing before merge:
1. --chat-room-filter can silently end up in channel-rule payloads
File:src/commands/integrations/create.ts
The assignment happens unconditionally before the chatRuleConfig branch:
if(flags["chat-room-filter"]){integrationData.chatRoomFilter=flags["chat-room-filter"];// set here}if(chatRuleConfig){// then branched here
...
}else{// channel rules — chatRoomFilter is already on integrationData}So a user running:
ably integrations create --rule-type http --source-type channel.message \
--chat-room-filter "room:.*" --target-url "https://example.com"
…will get the chatRoomFilter silently included in the API payload. The API returns a 422, but the user sees a confusing upstream error rather than a helpful CLI message.
The existing source-type/rule-type cross-validation doesn't catch this because it checks for chat.message source mismatch — not for --chat-room-filter appearing on a channel rule.
Fix: Add a guard after the cross-type validation:
if(!chatRuleConfig&&flags["chat-room-filter"]){fail(`--chat-room-filter is only valid for chat rule types (e.g. --rule-type "http/before-publish")`);}2. --failed-action and --too-many-requests-action lack options constraints
File:src/commands/integrations/create.ts (~lines where these flags are defined)
Both flags accept free-form strings, but the Control API only accepts a limited set of values. Without options: [...], a typo like --failed-action REJCT makes a round-trip to the API and returns a 422 instead of being caught at parse time by oclif.
Based on the test cases that verify "IGNORE" and "DROP", the valid values appear to be:
--failed-action:["REJECT", "IGNORE"](possibly others — check the API schema)--too-many-requests-action:["RETRY", "DROP"](possibly others)
If the full set of valid values is known, add options: [...] to both flag definitions.
3. --target-secret-access-key in shell history (informational, not a blocker)
File:src/commands/integrations/create.ts
AWS secret access keys passed as CLI flags end up in ~/.bash_history, ~/.zsh_history, and /proc/<pid>/cmdline during execution. This is a common pattern in CLIs (the aws CLI does this too via --secret-access-key), so it's not a blocker, but it's worth a note in the flag description like: "(Caution: appears in shell history — prefer setting via environment variables for production use)".
4. MockRule.source.channelFilter non-optional in test fixture (cosmetic)
File:test/fixtures/control-api.ts:63
MockRule.source is typed as { channelFilter: string; type: string } (channelFilter non-optional), but chat-sourced rules don't have channelFilter. Tests work around this with source: { channelFilter: "", type: "room.message" }. This doesn't cause any failures, but aligning the fixture with the updated Rule interface (channelFilter?: string) would make the intent clearer.
Issues 1 and 2 are the ones worth addressing before merge — both result in confusing user-facing errors for inputs that the CLI could catch itself. The rest are informational.
Rules created from Ably Chat rooms populate chatRoomFilter (top-level on the rule, alongside source) instead of source.channelFilter. Surface it in both human-readable and JSON output so `integrations get`/`list` don't silently omit the filter for chat-room-sourced rules. DX-1546 Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Rules can be sourced from Ably Chat rooms (source-type chat.message), which requires setting chatRoomFilter rather than the channel-based source.channelFilter. `integrations create`/`update` had no way to set it, even though get/list already knew how to display it. Also corrects the example/test filter values to valid regex syntax (channelFilter/chatRoomFilter are regexps, not globs) — e.g. "room:.*" instead of "room:*". DX-1546
Extends the Control API e2e suite to create/get/list/delete a rule with source-type chat.message and --chat-room-filter, verifying chatRoomFilter round-trips through the real API end to end.
…ests channelFilter is a regexp, not a glob, so "chat:*" is invalid — the trailing * has nothing to repeat. Switch examples and test fixtures to "chat:.*" (and similarly for other filter values used in tests).
…sh rule types Ably Chat rules (hive/text-model-only, hive/dashboard, bodyguard/text-moderation, tisane/text-moderation, azure/text-moderation, aws/lambda/before-publish, and http/before-publish) use a structurally different schema from the classic Reactor rule types: invocationMode + beforePublishConfig instead of requestMode, and vendor-specific target shapes discovered by inspecting existing rules and probing the Control API directly. `integrations create` had no way to create any of these — only the classic channel-sourced rule types were supported. Along the way, found and fixed two bugs that blocked this entirely: chatRoomFilter and source.channelFilter were sent unconditionally (even as empty strings), which the API rejects outright for chat.message-sourced and before-publish rules. modelUrl was sent as an explicit null when unset, which the API also rejects — now omitted instead. DX-1546 Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
… test Extends the Control API e2e suite with a full create/get/delete lifecycle for every chat rule type (http/before-publish, hive/text-model-only, hive/dashboard, bodyguard/text-moderation, tisane/text-moderation, azure/text-moderation, aws/lambda/before-publish) against the real API. Also fixes the existing chat-room-sourced rule test, which used the "http" rule type — the real API rejects chatRoomFilter/chat.message on that schema, so this test would have failed had it ever actually run (it was silently skipped locally for lack of E2E_ABLY_ACCESS_TOKEN). Running the new tests against the real API surfaced two more things these tests now account for: the azure/text-moderation target.endpoint is validated via a live DNS lookup at creation time, so a placeholder domain 422s; and rule IDs can start with "-", which oclif misparses as an unknown flag unless a "--" separator precedes the positional arg. DX-1546 Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…rule config update.ts and delete.ts printed "Request Mode: undefined" for chat rule types (they use invocationMode instead of requestMode) and never showed invocationMode at all — the same display bug already fixed in create.ts/ get.ts/list.ts was missed in these two commands. parseThresholds silently accepted "key=" and "key= " as a threshold of 0 instead of rejecting them, and discarded everything after a second "=". create.ts had no validation that a chat rule type is paired with --source-type chat.message (or vice versa), so a mismatched combination only surfaced as an opaque Control API 4xx instead of a clear CLI error. Replaces the CHAT_RULE_TYPES/AFTER_PUBLISH_CHAT_RULE_TYPES sets plus the per-rule-type switch statement with a single declarative config table (invocationMode, required target flags, target builder per rule type), removes the duplicate local IntegrationData/BeforePublishConfig types in favour of importing RuleData from control-api.ts, and extracts a shared buildModerationTarget helper for the three moderation-vendor rule types. DX-1546 Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…rules The rule-type guard in `integrations update` only matched the classic "http" rule type, so chat-sourced `http/before-publish` rules (which also target a webhook URL) couldn't have their target URL updated.
integrations get already surfaces chatRoomFilter; the delete confirmation prompt was missing it, so a chat-sourced rule's filter was invisible when confirming a destructive action.
--chat-room-filter was applied to integrationData unconditionally, so combining it with a channel-sourced rule type (e.g. "http") sent chatRoomFilter in a payload the Control API rejects for that source, surfacing a confusing 422 instead of a clear CLI error.
The hive/text-model-only and tisane/text-moderation tests only asserted against the mocked response, not the outgoing request body, so they didn't actually verify the fix that omits modelUrl when --target-model-url isn't provided. Also add a positive case covering --target-model-url being sent when it is provided.
d511faf to
0bf37b5CompareUh oh!
There was an error while loading. Please reload this page.
Summary
chatRoomFilter(top-level on the rule, alongsidesource) rather thansource.channelFilter.ably integrations get/listpreviously had no way to display this, so chat-room-sourced rules looked like they had no filter configured.integrations get/listnow show a "Chat Room Filter" line when set, and includechatRoomFilterin--jsonoutput.integrations create/updategained a--chat-room-filterflag andchat.messageas a--source-typeoption — but on their own these only covered the classic Reactor-style rule types (http,amqp, etc.), which turned out not to be the actual rule types Ably Chat rules use.invocationMode+beforePublishConfiginstead ofrequestMode, and a source object that rejectschannelFilterentirely.integrations createhad no way to create any of these rule types at all. This PR adds full support for all seven:http/before-publish,hive/text-model-only,hive/dashboard,bodyguard/text-moderation,tisane/text-moderation,azure/text-moderation, andaws/lambda/before-publish. Each gets its own typed target flags (--target-api-key,--target-endpoint,--target-function-name,--threshold key=value, etc.), andinvocationMode/beforePublishConfigare set automatically per rule type (onlyhive/dashboardrunsAFTER_PUBLISH; everything else isBEFORE_PUBLISHwith a configurable retry/failure policy).chatRoomFilterandsource.channelFilterwere being sent unconditionally (even as empty strings), which the Control API rejects outright forchat.message-sourced and before-publish rules — now both are only included when actually set. AmodelUrl: nulldefault was also rejected by the schema (it only accepts a string or omission) — now omitted when not provided.Rule/RuleDatainterfaces incontrol-api.tsupdated to match:invocationMode,beforePublishConfigadded,requestMode/source.channelFiltermade optional.integrations get/listalso surfaceinvocationModeand no longer print a blank "Request Mode" line for rule types that don't have one.channelFilter/chatRoomFilterexample and test values to valid regex syntax (e.g.chat:.*,room:.*) instead of glob-stylechat:*— both fields are regexps evaluated by the Control API, not globs.Test plan
pnpm prepare(build + manifest)pnpm exec eslint .— 0 errorspnpm test:unit— full suite passes (2553 tests)pnpm test:tty— passesintegrations create/get/list/updatecovering all seven chat rule types, required-flag validation per type,beforePublishConfigdefaults/overrides, and thehive/dashboardAFTER_PUBLISH/no-beforePublishConfigexceptiontest/e2e/integrations/integrations-e2e.test.ts) extended with a full create/get/delete lifecycle for every chat rule type, plus a fix to the existing chat-room-sourced test which used the wrong (http) rule type. Actually run against the real Control API (withE2E_ABLY_ACCESS_TOKEN/E2E_ABLY_API_KEY) this time — all 10 tests pass, including app create/teardownnode bin/run.js integrations create ...against a real app, then cleaned up the test rulesNotes from live verification
azure/text-moderation'starget.endpointis validated via a live DNS lookup at rule-creation time (422 onENOTFOUND), so the e2e test uses a real resolvable hostname rather than a placeholder.-(e.g.-MYkHg), which oclif's arg parser misreads as an unknown flag rather than a positional argument forintegrations get/delete. Worked around it in the new e2e tests with a--separator; flagging as a follow-up since it'd affect real users with an unlucky rule ID.DX-1546
Manual testing
Run against a local build of this branch (
pnpm preparethennode bin/run.js ...) — the globally installedablyCLI will be an older published version without this support.Chat Room Filter: room:.*andInvocation Mode: BEFORE_PUBLISH.channel.messagerules show neither:get: