test(api-service): e2e the agent ↔ Slack outbound contract via emulate.dev fixes NV-7630#11091
Merged
Merged
Conversation
…ev (NV-7630) Add the test infrastructure to exercise the outbound Slack contract end-to-end through @chat-adapter/slack and @slack/web-api against an in-process Slack emulator (https://emulate.dev/slack), so contract drift gets caught by CI instead of shipping silently. Pieces: - emulate@^0.5.0 dev dependency on apps/api - helpers/slack-emulator.ts: starts an emulator on a free port and patches @slack/web-api WebClient (constructor wrap + prototype apiCall override) so every Slack call is rerouted via process.env.SLACK_API_URL — the only knob that works since @chat-adapter/slack@4.25.0 calls new WebClient(token) with no options - helpers/bridge-server.ts: in-process @novu/framework/express bridge so the inbound webhook hits a real bridge HTTP roundtrip into a configurable onMessage handler - helpers/bridge-executor-test-stub.ts: sinon stub that performs the real bridge call via safeOutboundJsonRequest (which honors NOVU_SAFE_OUTBOUND_TEST_ALLOW_IPS), bypassing only the redundant resolvePublicAddresses pre-flight - agent-slack-roundtrip.e2e.ts: Scenario A — inbound app_mention triggers ctx.reply('pong'), assert message + thread_ts in the emulator and the matching ConversationActivity row Co-authored-by: Dima Grossman <dima@grossman.io>
Wire the suite end-to-end and unblock the contract test: - WebClient patch: `@slack/web-api` exports `WebClient` as a configurable getter, so plain assignment throws `Cannot set property … which has only a getter`. Switch to `Object.defineProperty` and add a prototype `apiCall` interceptor as a safety net for instances constructed before the patch lands. - Don't reset the emulator between tests: `store.reset()` re-runs `seed()` which assigns fresh random user/channel IDs, invalidating the IDs the suite-level `before()` cached. - Disable `acknowledgeOnReceived` on the test agent: the inbound handler awaits `thread.startTyping(...)` → `assistant.threads.setStatus`, which the emulator returns 404 for. `@slack/web-api` retries that for ~30 minutes, deadlocking the handler. - Poll for the bridge stub to fire before draining; the chat SDK dispatches `processMessage` as a fire-and-forget task after returning 200 to the webhook. Co-authored-by: Dima Grossman <dima@grossman.io>
✅ Deploy Preview for dashboard-v2-novu-staging canceled.
|
Contributor
📝 WalkthroughWalkthroughAdds in-process Slack emulator, an in-process Novu bridge server, a bridge-executor test stub that issues real HTTP POSTs, package dependency for the emulator, and a comprehensive e2e test suite validating Slack inbound events → bridge → emulator roundtrips and persistence. ChangesSlack Agent E2E Testing Infrastructure
Sequence DiagramsequenceDiagram
participant TestSuite as Test Suite
participant Emulator as Slack Emulator
participant BridgeServer as Bridge Server
participant ExecutorStub as BridgeExecutor Stub
participant DB as Database
TestSuite->>Emulator: startSlackEmulator(), seed workspace
TestSuite->>BridgeServer: startBridgeServer(agent handlers, secretKey)
TestSuite->>ExecutorStub: stubBridgeExecutorWithRealHttp()
TestSuite->>BridgeServer: POST signed app_mention webhook
BridgeServer->>ExecutorStub: BridgeExecutorService.execute invoked
ExecutorStub->>BridgeServer: HTTP POST to bridge URL (real outbound)
BridgeServer->>Emulator: agent handler posts message via Slack API
Emulator->>Emulator: store message/thread
Emulator->>TestSuite: recorded calls / API responses
TestSuite->>DB: assert conversation and activity persisted
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labelsnovu/api-service 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/api/src/app/agents/e2e/agent-slack-roundtrip.e2e.ts`:
- Around line 141-152: The before() hook sets
process.env.IS_CONVERSATIONAL_AGENTS_ENABLED without restoring it; update the
test suite to capture the original value (e.g., const prev =
process.env.IS_CONVERSATIONAL_AGENTS_ENABLED) before setting it in before(), and
restore that original value in after() (alongside calling stopSlackEmulator()).
Modify the existing before() and after() that call startSlackEmulator() and
stopSlackEmulator() to save and restore the env var so other tests do not
inherit the mutated flag.
- Around line 286-294: The test currently passes threadTs: ts to
buildSlackAppMention which forces a thread_ts in the fixture and prevents
exercising the non-threaded code path; remove the threadTs: ts argument from the
call in agent-slack-roundtrip.e2e.ts and update buildSlackAppMention to accept
threadTs as optional (make its parameter optional and only set the thread_ts
field on the returned payload when threadTs is provided) so the fixture can be
created without thread_ts and the adapter fallback logic (deriving thread_ts
from ts) is tested.
In `@apps/api/src/app/agents/e2e/helpers/bridge-server.ts`:
- Around line 28-68: The helper currently uses getPort() then app.listen(port,
...) which has a race; change startBridgeServer to bind to port 0 directly
(remove getPort usage) and after listen resolve the server and read the actual
assigned port from server.address() (cast to AddressInfo) and use that value for
the returned port and url; keep using app.listen('0.0.0.0' or appropriate host)
but ensure url uses the resolved port, and ensure the returned close() uses the
same server.close promise to shut down the server.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: a5a2836a-82ff-4cad-98e5-ded9403bd097
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (5)
apps/api/package.jsonapps/api/src/app/agents/e2e/agent-slack-roundtrip.e2e.tsapps/api/src/app/agents/e2e/helpers/bridge-executor-test-stub.tsapps/api/src/app/agents/e2e/helpers/bridge-server.tsapps/api/src/app/agents/e2e/helpers/slack-emulator.ts
Comment on lines
+141
to
+152
| before(async () => { | ||
| process.env.IS_CONVERSATIONAL_AGENTS_ENABLED = 'true'; | ||
| const emulator = await startSlackEmulator(); | ||
| emulatorUrl = emulator.url; | ||
|
|
||
| channel = await findEmulatorChannel(emulatorUrl, 'incidents'); | ||
| user = await findEmulatorUser(emulatorUrl, 'e2e@novu.test'); | ||
| }); | ||
|
|
||
| after(async () => { | ||
| await stopSlackEmulator(); | ||
| }); |
Contributor
There was a problem hiding this comment.
Restore IS_CONVERSATIONAL_AGENTS_ENABLED after the suite.
This suite mutates a process-global flag in before() and never puts it back. Because Mocha runs other e2e files in the same process, later suites can accidentally inherit conversational agents being forced on.
♻️ Proposed fix
describe('Agent Slack Roundtrip - emulate.dev `#novu-v2`', () => {
+ const previousConversationalAgentsEnabled = process.env.IS_CONVERSATIONAL_AGENTS_ENABLED;
let ctx: AgentTestContext;
@@
after(async () => {
+ if (previousConversationalAgentsEnabled === undefined) {
+ delete process.env.IS_CONVERSATIONAL_AGENTS_ENABLED;
+ } else {
+ process.env.IS_CONVERSATIONAL_AGENTS_ENABLED = previousConversationalAgentsEnabled;
+ }
await stopSlackEmulator();
});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| before(async () => { | |
| process.env.IS_CONVERSATIONAL_AGENTS_ENABLED = 'true'; | |
| const emulator = await startSlackEmulator(); | |
| emulatorUrl = emulator.url; | |
| channel = await findEmulatorChannel(emulatorUrl, 'incidents'); | |
| user = await findEmulatorUser(emulatorUrl, 'e2e@novu.test'); | |
| }); | |
| after(async () => { | |
| await stopSlackEmulator(); | |
| }); | |
| describe('Agent Slack Roundtrip - emulate.dev `#novu-v2`', () => { | |
| const previousConversationalAgentsEnabled = process.env.IS_CONVERSATIONAL_AGENTS_ENABLED; | |
| let ctx: AgentTestContext; | |
| before(async () => { | |
| process.env.IS_CONVERSATIONAL_AGENTS_ENABLED = 'true'; | |
| const emulator = await startSlackEmulator(); | |
| emulatorUrl = emulator.url; | |
| channel = await findEmulatorChannel(emulatorUrl, 'incidents'); | |
| user = await findEmulatorUser(emulatorUrl, 'e2e@novu.test'); | |
| }); | |
| after(async () => { | |
| if (previousConversationalAgentsEnabled === undefined) { | |
| delete process.env.IS_CONVERSATIONAL_AGENTS_ENABLED; | |
| } else { | |
| process.env.IS_CONVERSATIONAL_AGENTS_ENABLED = previousConversationalAgentsEnabled; | |
| } | |
| await stopSlackEmulator(); | |
| }); | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/api/src/app/agents/e2e/agent-slack-roundtrip.e2e.ts` around lines 141 -
152, The before() hook sets process.env.IS_CONVERSATIONAL_AGENTS_ENABLED without
restoring it; update the test suite to capture the original value (e.g., const
prev = process.env.IS_CONVERSATIONAL_AGENTS_ENABLED) before setting it in
before(), and restore that original value in after() (alongside calling
stopSlackEmulator()). Modify the existing before() and after() that call
startSlackEmulator() and stopSlackEmulator() to save and restore the env var so
other tests do not inherit the mutated flag.
Comment on lines
+286
to
+294
| const ts = `${Math.floor(Date.now() / 1000)}.000200`; | ||
| const body = JSON.stringify( | ||
| buildSlackAppMention({ | ||
| userId: user.id, | ||
| channel: channel.id, | ||
| threadTs: ts, | ||
| text: '<@UBOT> hello', | ||
| eventTs: ts, | ||
| }) |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
fd -i 'slack.ts' apps/api/src/app/agents/e2e/helpers/providers -x sed -n '1,220p' {}
rg -n -C3 'buildSlackAppMention|threadTs|thread_ts|eventTs' apps/api/src/app/agents/e2e/helpers/providersRepository: novuhq/novu
Length of output: 4919
🏁 Script executed:
# First, let's look at the test file around lines 286-294
fd -i 'agent-slack-roundtrip.e2e.ts' apps/api/src -x cat -n {} | sed -n '270,310p'Repository: novuhq/novu
Length of output: 1921
🏁 Script executed:
# Check the full signature and other test calls to buildSlackAppMention
rg -n 'buildSlackAppMention' apps/api/src/app/agents/e2e/agent-slack-roundtrip.e2e.ts -A 6Repository: novuhq/novu
Length of output: 663
🏁 Script executed:
# Look at the context around line 217 to understand the first test
sed -n '200,230p' apps/api/src/app/agents/e2e/agent-slack-roundtrip.e2e.tsRepository: novuhq/novu
Length of output: 1152
🏁 Script executed:
# Check if buildSlackAppMention is called elsewhere to see if threadTs should be optional
rg -n 'buildSlackAppMention|buildSlackSubscribedMessage' apps/api/src/app/agents/e2e --context 2Repository: novuhq/novu
Length of output: 3772
🏁 Script executed:
# Check if threadTs is ever omitted in calls to buildSlackAppMention or if it should be optional
rg -B2 -A5 'buildSlackAppMention\s*\(' apps/api/src/app/agents/e2eRepository: novuhq/novu
Length of output: 3505
🏁 Script executed:
# Check what the actual behavior should be - look for any handling of undefined threadTs in Slack adapter code
rg -n 'thread_ts|threadTs' apps/api/src/app/agents --context 3 -t ts | head -100Repository: novuhq/novu
Length of output: 9478
🏁 Script executed:
# Let's look more carefully at the adapter code to understand what behavior should be tested
rg -B5 -A10 'rawThreadTs.*thread_ts' apps/api/src/app/agents/services/agent-inbound-handler.service.tsRepository: novuhq/novu
Length of output: 697
🏁 Script executed:
# Check the full buildSlackAppMention implementation to see if threadTs could be made optional
sed -n '14,38p' apps/api/src/app/agents/e2e/helpers/providers/slack.tsRepository: novuhq/novu
Length of output: 631
Remove threadTs: ts from the test fixture to properly test the non-threaded scenario.
The test claims to verify behavior when an inbound Slack message has no thread_ts, but it currently passes threadTs: ts to buildSlackAppMention(), which sets thread_ts in the fixture. This defeats the test's purpose—it only exercises the existing-thread code path and never covers the adapter's fallback logic that derives thread_ts from the message ts when no thread exists.
To fix this, remove threadTs: ts from the fixture call. Note: This will also require making threadTs optional in buildSlackAppMention() (currently it's required) and conditionally setting thread_ts only when provided.
♻️ Proposed fix
buildSlackAppMention({
userId: user.id,
channel: channel.id,
- threadTs: ts,
text: '<@UBOT> hello',
eventTs: ts,
})🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/api/src/app/agents/e2e/agent-slack-roundtrip.e2e.ts` around lines 286 -
294, The test currently passes threadTs: ts to buildSlackAppMention which forces
a thread_ts in the fixture and prevents exercising the non-threaded code path;
remove the threadTs: ts argument from the call in agent-slack-roundtrip.e2e.ts
and update buildSlackAppMention to accept threadTs as optional (make its
parameter optional and only set the thread_ts field on the returned payload when
threadTs is provided) so the fixture can be created without thread_ts and the
adapter fallback logic (deriving thread_ts from ts) is tested.
Comment on lines
+28
to
+68
| import getPort from 'get-port'; | ||
|
|
||
| type AgentHandlers = Parameters<typeof agent>[1]; | ||
|
|
||
| export interface BridgeServerHandle { | ||
| url: string; | ||
| port: number; | ||
| close: () => Promise<void>; | ||
| } | ||
|
|
||
| export async function startBridgeServer(opts: { | ||
| agentId: string; | ||
| handlers: AgentHandlers; | ||
| secretKey: string; | ||
| }): Promise<BridgeServerHandle> { | ||
| const port = await getPort(); | ||
| const app = express(); | ||
|
|
||
| app.use(express.json({ limit: '10mb' })); | ||
|
|
||
| const novuAgent = agent(opts.agentId, opts.handlers); | ||
|
|
||
| app.use( | ||
| '/api/novu', | ||
| serve({ | ||
| agents: [novuAgent], | ||
| client: new Client({ | ||
| secretKey: opts.secretKey, | ||
| strictAuthentication: false, | ||
| }), | ||
| }) | ||
| ); | ||
|
|
||
| const server: http.Server = await new Promise((resolve, reject) => { | ||
| const s = app.listen(port, '0.0.0.0', () => resolve(s)); | ||
| s.once('error', reject); | ||
| }); | ||
|
|
||
| return { | ||
| port, | ||
| url: `http://0.0.0.0:${port}/api/novu`, |
Contributor
There was a problem hiding this comment.
Bind to port 0 instead of preselecting one.
getPort() followed by app.listen(port, ...) still has a race window. Under parallel e2e shards another process can grab that port before this server binds, which turns this helper into an intermittent startup flake.
♻️ Proposed fix
import http from 'node:http';
import { agent, Client, serve } from '@novu/framework/express';
import express from 'express';
-import getPort from 'get-port';
@@
}): Promise<BridgeServerHandle> {
- const port = await getPort();
const app = express();
@@
const server: http.Server = await new Promise((resolve, reject) => {
- const s = app.listen(port, '0.0.0.0', () => resolve(s));
+ const s = app.listen(0, '0.0.0.0', () => resolve(s));
s.once('error', reject);
});
+ const address = server.address();
+ if (!address || typeof address === 'string') {
+ throw new Error('Bridge server did not expose a numeric port');
+ }
+ const port = address.port;
return {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/api/src/app/agents/e2e/helpers/bridge-server.ts` around lines 28 - 68,
The helper currently uses getPort() then app.listen(port, ...) which has a race;
change startBridgeServer to bind to port 0 directly (remove getPort usage) and
after listen resolve the server and read the actual assigned port from
server.address() (cast to AddressInfo) and use that value for the returned port
and url; keep using app.listen('0.0.0.0' or appropriate host) but ensure url
uses the resolved port, and ensure the returned close() uses the same
server.close promise to shut down the server.
…NV-7630)
Three new outbound contract assertions building on the Scenario A scaffolding:
- **Card → Block Kit fidelity**: emulate@0.5's chat.postMessage drops the
blocks field when storing messages, so we can't read blocks back via
conversations.history. Patch WebClient.prototype.apiCall to record every
Slack request and assert the wire payload directly — that's exactly what
the production adapter sends to Slack. Verifies button action_ids and
styles survive Card → cardToBlockKit serialization.
- **reactions.add on resolve**: ctx.resolve fires reactionOnResolve against
the conversation's firstPlatformMessageId (the inbound user message),
which lives only in the webhook payload — never chat.postMessage'd into
the emulator — so reactions.add resolves to message_not_found
server-side. We assert via the recorded WebClient call instead and
cross-check the conversation status moved to 'resolved' in Mongo.
- **Edit via /reply edit path**: full roundtrip exercising chat.update via
the public /v1/agents/:agentId/reply endpoint with edit: { messageId,
content }. Asserts the emulator-stored message text changed in place.
clearRecordedCalls() runs in beforeEach so each test sees only its own
WebClient calls, and the recorded-calls store lives next to the emulator
helpers since both are part of the same test scaffolding.
Co-authored-by: Dima Grossman <dima@grossman.io>
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (2)
apps/api/src/app/agents/e2e/agent-slack-roundtrip.e2e.ts (2)
130-152:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRestore
IS_CONVERSATIONAL_AGENTS_ENABLEDafter the suite.This suite mutates a process-global flag in
before()and never puts it back, so later e2e suites in the same Mocha process can inherit conversational agents being forced on.♻️ Proposed fix
describe('Agent Slack Roundtrip - emulate.dev `#novu-v2`', () => { + const previousConversationalAgentsEnabled = process.env.IS_CONVERSATIONAL_AGENTS_ENABLED; let ctx: AgentTestContext; let bridge: BridgeServerHandle | undefined; let bridgeStub: BridgeExecutorStubHandle; let emulatorUrl: string; @@ after(async () => { + if (previousConversationalAgentsEnabled === undefined) { + delete process.env.IS_CONVERSATIONAL_AGENTS_ENABLED; + } else { + process.env.IS_CONVERSATIONAL_AGENTS_ENABLED = previousConversationalAgentsEnabled; + } await stopSlackEmulator(); });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/api/src/app/agents/e2e/agent-slack-roundtrip.e2e.ts` around lines 130 - 152, The test suite sets process.env.IS_CONVERSATIONAL_AGENTS_ENABLED in before() but never restores it; update the suite to save the original value (e.g., const prevIsConv = process.env.IS_CONVERSATIONAL_AGENTS_ENABLED) at the start of before(), then in after() restore it (if prevIsConv is undefined delete process.env.IS_CONVERSATIONAL_AGENTS_ENABLED else set it back to prevIsConv) so the global environment is not leaked between tests; modify the before() and after() blocks around startSlackEmulator()/stopSlackEmulator() and reference the process.env.IS_CONVERSATIONAL_AGENTS_ENABLED symbol when saving/restoring.
281-299:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDrop
threadTsfrom the "non-threaded" fixture.This test says it covers an inbound message with no
thread_ts, butthreadTs: tsmakes the payload threaded up front, so the adapter's fallback path never gets exercised.♻️ Proposed fix
const body = JSON.stringify( buildSlackAppMention({ userId: user.id, channel: channel.id, - threadTs: ts, text: '<@UBOT> hello', eventTs: ts, }) );You'll also need to make
threadTsoptional inapps/api/src/app/agents/e2e/helpers/providers/slack.tsand only includethread_tswhen it is provided.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/api/src/app/agents/e2e/agent-slack-roundtrip.e2e.ts` around lines 281 - 299, The test "serializes top-level (non-threaded) replies into channel history" wrongly sets threadTs: ts so the non-threaded code path is never exercised; remove the threadTs property from the fixture in agent-slack-roundtrip.e2e.ts (i.e., stop passing threadTs when building the inbound payload) and update the helper buildSlackAppMention / provider code in apps/api/src/app/agents/e2e/helpers/providers/slack.ts to make the threadTs parameter optional and only add the thread_ts field to the generated payload when threadTs is defined, ensuring callers can omit it to simulate non-threaded messages.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In `@apps/api/src/app/agents/e2e/agent-slack-roundtrip.e2e.ts`:
- Around line 130-152: The test suite sets
process.env.IS_CONVERSATIONAL_AGENTS_ENABLED in before() but never restores it;
update the suite to save the original value (e.g., const prevIsConv =
process.env.IS_CONVERSATIONAL_AGENTS_ENABLED) at the start of before(), then in
after() restore it (if prevIsConv is undefined delete
process.env.IS_CONVERSATIONAL_AGENTS_ENABLED else set it back to prevIsConv) so
the global environment is not leaked between tests; modify the before() and
after() blocks around startSlackEmulator()/stopSlackEmulator() and reference the
process.env.IS_CONVERSATIONAL_AGENTS_ENABLED symbol when saving/restoring.
- Around line 281-299: The test "serializes top-level (non-threaded) replies
into channel history" wrongly sets threadTs: ts so the non-threaded code path is
never exercised; remove the threadTs property from the fixture in
agent-slack-roundtrip.e2e.ts (i.e., stop passing threadTs when building the
inbound payload) and update the helper buildSlackAppMention / provider code in
apps/api/src/app/agents/e2e/helpers/providers/slack.ts to make the threadTs
parameter optional and only add the thread_ts field to the generated payload
when threadTs is defined, ensuring callers can omit it to simulate non-threaded
messages.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 34fdf6bb-ee7d-4009-af3d-c0bf70cd3e1e
📒 Files selected for processing (3)
apps/api/src/app/agents/e2e/agent-slack-roundtrip.e2e.tsapps/api/src/app/agents/e2e/helpers/bridge-executor-test-stub.tsapps/api/src/app/agents/e2e/helpers/slack-emulator.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- apps/api/src/app/agents/e2e/helpers/slack-emulator.ts
- apps/api/src/app/agents/e2e/helpers/bridge-executor-test-stub.ts
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.
Why
Today's agent e2e tests at
apps/api/src/app/agents/e2e/cover inbound webhook parsing well, but every outbound Slack call (chat.postMessage,chat.update,reactions.add, threading, Block Kit serialization) is stubbed via sinon onChatSdkService. The real@chat-adapter/slackadapter never makes an HTTP call in CI, so a contract drift in the adapter,@slack/web-api, or ourCard→ Block Kit serialization can land without a regression test catching it.What
Stand up an in-process Slack Web API mock (emulate.dev/slack) and an in-process bridge SDK server, then assert against the messages the production code path actually delivered to Slack.
Five scenarios, all green locally:
app_mention→ bridgeonMessage→ctx.reply('pong')→ assert message +thread_tsin the emulator and the matchingConversationActivityrow.conversations.history.confirm/cancelbuttonaction_ids and styles survive Card →cardToBlockKit→ wire serialization.emulate@0.5dropsblockswhen storing, so we patchWebClient.prototype.apiCallto record every Slack request and assert the wire payload directly.reactions.addon resolve:ctx.resolvefiresreactionOnResolveagainst the conversation'sfirstPlatformMessageId, which lives only in the webhook payload (neverchat.postMessage'd into the emulator), soreactions.addresolves tomessage_not_foundserver-side. We assert via the recorded WebClient call instead and cross-check the conversation moved toRESOLVEDin Mongo./reply: full roundtrip exercisingchat.updateviaPOST /v1/agents/:id/replywithedit: { messageId, content }. Asserts the emulator-stored message text changed in place.How it wires together
sequenceDiagram participant Test participant API as Novu API participant Bridge as In-process bridge<br/>(@novu/framework/express) participant Adapter as @chat-adapter/slack participant Emulator as emulate.dev/slack Test->>API: POST /v1/agents/:id/webhook (signed app_mention) API->>Adapter: handleWebhook Adapter-->>API: 200 OK (fire-and-forget) Adapter->>Adapter: parseSlackMessage → mention handler API->>Bridge: POST /api/novu (HMAC, real HTTP) Bridge->>Bridge: onMessage → ctx.reply(...) Bridge->>API: POST /v1/agents/:id/reply (ApiKey) API->>Adapter: postToConversation / addReaction / editMessage Adapter->>Emulator: chat.postMessage / reactions.add / chat.update<br/>(patched WebClient) Test->>Emulator: conversations.replies / recorded calls → assertKey implementation details
emulate@^0.5.0as a dev dependency onapps/apionly.helpers/slack-emulator.tsboots the emulator on a free port (viaget-port) and patches@slack/web-api'sWebClientso every Slack call is rerouted viaprocess.env.SLACK_API_URL. The plan's "env-var path works out of the box" claim assumed a newer@chat-adapter/slackthat threadsapiUrlthrough; the version we ship (4.25.0) callsnew WebClient(botToken)with no options, so we patch the constructor (viaObject.definePropertybecause the export is a getter) AND theapiCallprototype method as a safety net. The sameapiCallpatch records every call into a per-test buffer (getRecordedCalls/clearRecordedCalls) so tests can assert against the wire payload — necessary because emulate@0.5 only persists a subset of fields perchat.postMessage(noblocks, etc.).helpers/bridge-server.tsspins up a real@novu/framework/expressserve()handler bound to0.0.0.0:port(already inNOVU_SAFE_OUTBOUND_TEST_ALLOW_IPS). Test passes the agent handler in.helpers/bridge-executor-test-stub.tsstubsBridgeExecutorService.executewith a real-HTTP implementation that exercises the productionresolveBridgeUrl/buildPayload/ signature path via reflection, but skips the redundantresolvePublicAddressespre-flight (which doesn't honorNOVU_SAFE_OUTBOUND_TEST_ALLOW_IPS).agent-slack-roundtrip.e2e.tsdisablesacknowledgeOnReceivedon the test agent because the inbound handler awaitsassistant.threads.setStatus(the typing indicator), which the emulator returns 404 for and@slack/web-apiretries for ~30 minutes — long enough to deadlock the handler.Production code
Zero changes. The test stub bypasses the redundant SSRF pre-flight only at test time; the actual
safeOutboundJsonRequestcall (which is the load-bearing SSRF check) still runs untouched.Risks
@chat-adapter/slack's adapter still pulls in@slack/socket-modetransitively. Verified that the agent flow uses the HTTP Events API only (chat-sdk.service.ts:handleWebhook) and never opens a socket-mode connection.emulate@0.5is early; some Block Kit elements may serialize differently than Slack's real backend. We work around the missingblocksstorage by asserting on recorded WebClient calls.Testing
pnpm exec mocha … src/**/agent-slack-roundtrip.e2e.ts— 5 passingagents.e2e.ts,agent-webhook.e2e.ts,agent-reply.e2e.ts)agent-webhook.e2e.ts(should process inbound again after reactivation) reproduces onnextwithout these changes — unrelated.What changed
Added end-to-end test infrastructure in apps/api to validate outbound Slack Web API calls from agent bridge code against an in-process Slack emulator (emulate.dev/slack). The tests patch @slack/web-api so production code paths send real chat calls to the emulator, enabling verification of threading, message edits, reactions, and Card → Block Kit fidelity without stubbing the adapter layer.
Affected areas
api: New e2e test infra and tests under apps/api including a Slack emulator helper, an in-process bridge server, a BridgeExecutorTest stub that issues real HTTP bridge requests, and the agent-slack-roundtrip.e2e.ts test suite exercising inbound app_mention → bridge → reply flows and asserting emulator state and DB persistence.
Key technical decisions
Testing
New e2e tests (mocha) exercise multiple scenarios: threaded and non-threaded mentions, Card → Block Kit serialization checks, reaction-on-resolve behavior, and reply-edit flows; tests assert emulator-received API calls and ConversationActivity DB records. No production code changes; test-only helpers are included as dev/test code.