Conversation
✅ Deploy Preview for dashboard-v2-novu-staging canceled.
|
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds conversational agents: new agents controllers/endpoints and guard, Chat SDK integration with cached Chat instances and webhook handling, conversation & conversation-activity DAL (entities/schemas/repositories), multiple agent services (credential resolution, subscriber mapping, inbound handling, bridge dispatch), DTOs/use-case for replies, and raw-body capture for agent routes. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Client
participant Express
participant Controller as AgentsWebhookController
participant ChatSdk as ChatSdkService
participant Adapter as PlatformAdapter
participant Inbound as AgentInboundHandler
participant ConversationSvc as AgentConversationService
participant BridgeExec as BridgeExecutorService
participant BridgeEndpoint
Client->>Express: POST /agents/:agentId/webhook/:integrationId
Express->>Controller: handleWebhook(agentId,integrationId,req,res)
Controller->>ChatSdk: handleWebhook(...)
ChatSdk->>ChatSdk: resolve credentials / getOrCreate Chat instance
ChatSdk->>Adapter: invoke platform webhook handler (toWebRequest)
Adapter->>Inbound: emit event (mention/message)
Inbound->>ConversationSvc: createOrGetConversation(...)
ConversationSvc-->>Inbound: conversation
Inbound->>ConversationSvc: persistInboundMessage(...)
Inbound->>BridgeExec: execute(event, config, conversation, ...)
BridgeExec->>BridgeEndpoint: POST signed payload (with retries)
BridgeEndpoint-->>BridgeExec: response
ChatSdk-->>Controller: sendWebResponse
Controller-->>Client: HTTP response
sequenceDiagram
autonumber
participant Client
participant Express
participant Controller as AgentsWebhookController
participant UseCase as HandleAgentReply
participant ConvRepo as ConversationRepository
participant ChatSdk as ChatSdkService
participant BridgeExec as BridgeExecutorService
Client->>Express: POST /agents/:agentId/reply
Express->>Controller: reply(agentId,payload)
Controller->>UseCase: execute(command)
UseCase->>ConvRepo: findById(conversationId)
ConvRepo-->>UseCase: conversation
alt update
UseCase->>ChatSdk: postToConversation(...)
ChatSdk-->>UseCase: OK
UseCase->>ConvRepo: createAgentActivity / touchActivity
UseCase-->>Express: { status: "update_sent" }
else reply
UseCase->>ChatSdk: postToConversation(...)
UseCase->>ConvRepo: createAgentActivity / touchActivity
else resolve
UseCase->>ConvRepo: updateStatus(RESOLVED)
UseCase->>ConvRepo: createSignalActivity(resolve)
UseCase->>BridgeExec: execute(ON_RESOLVE,...)
end
UseCase-->>Express: response
Express-->>Client: HTTP response
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
commit: |
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
ChmaraX
changed the title
Introduce DAL layer for the conversations module — entities, schemas, and repositories for Conversation and ConversationActivity collections. Also adds an unscoped findByIdForWebhook helper to AgentRepository. Made-with: Cursor
…ebhook plumbing Wire up Phase 3 of the Agent API — multi-platform Chat SDK integration with real DB credential resolution and feature flag gating. - ChatSdkService: manages LRU-cached Chat instances per agent/integration, delegates platform webhooks via Express↔Web API conversion - AgentCredentialService: resolves agent → integration → credentials chain with decryption and ChannelConnection lookup - AgentConversationEnabledGuard: gates all agent/conversation routes behind IS_AGENT_CONVERSATION_ENABLED feature flag (LaunchDarkly) - Webhook controller at POST /agents/:agentId/webhook/:integrationIdentifier - Provider-to-platform mapping (Slack, Teams, WhatsApp) - ConversationRepository: fix _organizationId scoping on all query methods Enterprise: novuhq/packages-enterprise#442 Made-with: Cursor
Made-with: Cursor
The Chat SDK and adapter packages are ESM-only (no CJS exports). Since NestJS compiles to CJS, static imports fail at runtime with "No exports main defined". Use dynamic import() via new Function wrapper to prevent SWC from rewriting to require(). Also removes premature AgentSubscriberResolver reference. Made-with: Cursor
…ng types Made-with: Cursor
- Add PLATFORM_USER to ConversationActivitySenderTypeEnum — unlinked platform users now correctly show as platform_user, not subscriber - createUserActivity accepts senderType param instead of hardcoding SUBSCRIBER - AgentInboundHandler maps participantType to activity senderType - Fix replyUrl missing /v1/ prefix in bridge payload - Use conversation._agentId (MongoDB ObjectId) for postToConversation instead of agentIdentifier (string) which caused CastError Made-with: Cursor
![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: 9
🧹 Nitpick comments (16)
apps/api/src/app/agents/guards/agent-conversation-enabled.guard.ts (1)
13-18: Empty-string fallback for missing user context may cause inconsistent flag evaluation.When
request.useris undefined (e.g., unauthenticated routes), the guard passes empty strings fororganization._idandenvironment._id. This could cause LaunchDarkly to evaluate the flag against a fallback context rather than returningfalseas expected. Consider explicitly returningfalseor throwing early when user context is missing.🛡️ Suggested fix
async canActivate(context: ExecutionContext): Promise<boolean> { const request = context.switchToHttp().getRequest(); const user: UserSessionData | undefined = request.user; + if (!user?.organizationId || !user?.environmentId) { + throw new NotFoundException(); + } + const isEnabled = await this.featureFlagsService.getFlag({ key: FeatureFlagsKeysEnum.IS_CONVERSATIONAL_AGENTS_ENABLED, defaultValue: false, - organization: { _id: user?.organizationId ?? '' }, - environment: { _id: user?.environmentId ?? '' }, + organization: { _id: user.organizationId }, + environment: { _id: user.environmentId }, });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/api/src/app/agents/guards/agent-conversation-enabled.guard.ts` around lines 13 - 18, The guard currently calls featureFlagsService.getFlag with empty-string fallbacks when request.user is missing; instead, update the AgentConversationEnabledGuard (the code around getFlag / FeatureFlagsKeysEnum.IS_CONVERSATIONAL_AGENTS_ENABLED) to check for a valid request.user and required user.organizationId and user.environmentId up front and return false (or throw early per your guard semantics) if they are absent, so you do not invoke getFlag with empty context values; ensure the early-return replaces the current invocation of this.featureFlagsService.getFlag when user or IDs are missing.apps/api/src/app/agents/e2e/agents.e2e.ts (1)
11-13: Consider resetting the environment variable after the test suite.Setting
process.env.IS_CONVERSATIONAL_AGENTS_ENABLEDinbefore()without a correspondingafter()cleanup means the variable persists for any subsequent test suites run in the same process. This is likely fine for isolated e2e runs but could cause unexpected behavior in shared test processes.Optional cleanup hook
+ after(() => { + delete process.env.IS_CONVERSATIONAL_AGENTS_ENABLED; + }); + beforeEach(async () => {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/api/src/app/agents/e2e/agents.e2e.ts` around lines 11 - 13, The test suite sets process.env.IS_CONVERSATIONAL_AGENTS_ENABLED in the existing before() hook but doesn't restore it; capture the original value (e.g., const _orig = process.env.IS_CONVERSATIONAL_AGENTS_ENABLED) before setting it in before(), then add a matching after() hook that restores the environment variable (set it back to _orig or delete it if undefined) to ensure process.env.IS_CONVERSATIONAL_AGENTS_ENABLED is cleaned up after the agents.e2e test run.libs/dal/src/repositories/conversation/conversation.repository.ts (1)
81-92: Consider storinglastActivityAtasDateinstead of ISO string.Line 86 uses
new Date().toISOString()which stores a string. For optimal MongoDB sorting and range queries on this field, storing a nativeDateobject is more efficient and allows proper indexing.Proposed fix
{ $set: { - lastActivityAt: new Date().toISOString(), + lastActivityAt: new Date(), lastMessagePreview: messagePreview.slice(0, 200), }, $inc: { messageCount: 1 }, }Also update
ConversationEntity.lastActivityAtandcreatedAttypes fromstringtoDateif they should be native dates.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/dal/src/repositories/conversation/conversation.repository.ts` around lines 81 - 92, touchActivity currently sets lastActivityAt to an ISO string (new Date().toISOString()); change this to store a native Date object (new Date()) and update the update call in touchActivity to set lastActivityAt: new Date(). Also update the ConversationEntity type definitions for lastActivityAt (and createdAt if applicable) from string to Date so the repository and any mappers expect native Date objects; ensure any serialization/deserialization code that reads/writes ConversationEntity handles Date objects appropriately (e.g., when returning DTOs or persisting to MongoDB).apps/api/src/app/agents/services/chat-sdk.service.ts (1)
162-197: Non-null assertions on credentials may produce unclear runtime errors.The
!assertions (e.g.,credentials.apiKey!,credentials.clientId!) assume upstream validation. If a credential field is missing, the error will surface inside the adapter library rather than here with a clear message.Consider adding explicit checks or let
AgentCredentialService.resolve()validate required fields per platform.Example validation
case AgentPlatformEnum.SLACK: { if (!connectionAccessToken || !credentials.apiKey) { throw new BadRequestException('Slack integration requires botToken and signingSecret'); } const { createSlackAdapter } = await esmImport('@chat-adapter/slack'); return { slack: createSlackAdapter({ botToken: connectionAccessToken, signingSecret: credentials.apiKey, }), }; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/api/src/app/agents/services/chat-sdk.service.ts` around lines 162 - 197, The switch branches in chat-sdk.service.ts (cases for AgentPlatformEnum.SLACK, TEAMS, WHATSAPP) use non-null assertions like connectionAccessToken! and credentials.clientId! which defer missing-credential errors into adapter libraries; replace these with explicit validation checks for required fields (e.g., ensure connectionAccessToken and credentials.apiKey for SLACK; credentials.clientId/secretKey/tenantId for TEAMS; credentials.token/secretKey/apiToken/phoneNumberIdentification for WHATSAPP) and throw BadRequestException with clear messages when a required field is missing (or delegate to AgentCredentialService.resolve() to perform per-platform validation) before calling createSlackAdapter/createTeamsAdapter/createWhatsAppAdapter.apps/api/src/app/agents/services/agent-inbound-handler.service.ts (1)
78-87: Typing indicator may timeout before bridge response.
thread.startTyping()is called but there's no mechanism to refresh it if the bridge call takes longer than the platform's typing timeout (typically 5-10 seconds). Consider:
- Periodic refresh in a background task, or
- Calling
startTyping()closer to the bridge call, or- Accepting this limitation for the initial implementation.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/api/src/app/agents/services/agent-inbound-handler.service.ts` around lines 78 - 87, The thread.startTyping() call can expire if the bridge/remote call takes longer, so implement a typing keep-alive: when you call thread.startTyping() (in agent-inbound-handler.service near the code using serializedThread and conversationService.updateChannelThread), start a repeating timer (e.g., setInterval) that calls thread.startTyping() periodically (before the platform timeout) and re-serializes the thread (thread.toJSON()) then calls this.conversationService.updateChannelThread(...) to persist the refreshed indicator; clear the interval and stop typing once the bridge response completes or errors. Ensure the timer is cleared in both success and error paths to avoid leaks.apps/api/src/app/agents/services/agent-conversation.service.ts (6)
153-155: Missing blank line beforereturnstatement.As per coding guidelines, include a blank line before every
returnstatement.Suggested fix
async getHistory(environmentId: string, conversationId: string, limit = 20): Promise<ConversationActivityEntity[]> { + return this.activityRepository.findByConversation(environmentId, conversationId, limit); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/api/src/app/agents/services/agent-conversation.service.ts` around lines 153 - 155, The getHistory method in agent-conversation.service.ts is missing a blank line before its return; update the async function getHistory(environmentId: string, conversationId: string, limit = 20) to insert a single blank line immediately before the return statement that calls this.activityRepository.findByConversation so it conforms to the coding guideline requiring a blank line before every return.
65-68: Missing blank line beforereturnstatement.As per coding guidelines, include a blank line before every
returnstatement.Suggested fix
await this.ensureParticipant(existing, params); + return existing;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/api/src/app/agents/services/agent-conversation.service.ts` around lines 65 - 68, Add a blank line immediately before the `return existing;` statement in the method that calls `await this.ensureParticipant(existing, params);` so the `return` is separated by a single empty line from the preceding call; update the method in AgentConversationService (the function containing `ensureParticipant` and `existing`) to follow the project's rule of a blank line before every `return`.
153-155: Consider addingorganizationIdparameter for consistency.Other methods in this service (
createOrGetConversation,persistInboundMessage,updateChannelThread) all accept and passorganizationId. ThegetHistorymethod only usesenvironmentId, which may be sufficient for scoping, but includingorganizationIdwould maintain consistency with the enforcement pattern used elsewhere.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/api/src/app/agents/services/agent-conversation.service.ts` around lines 153 - 155, getHistory currently lacks an organizationId parameter which breaks consistency with other methods (createOrGetConversation, persistInboundMessage, updateChannelThread) that enforce org scoping; update the getHistory method signature to accept organizationId (e.g., getHistory(organizationId: string, environmentId: string, conversationId: string, limit = 20)), pass organizationId into the activityRepository.findByConversation call (and adjust that repository method signature if needed), and update all callers to provide organizationId so org-level access control remains consistent.
85-85: Add defensive check forfirstMessageText.If
firstMessageTextis empty or undefined at runtime (despite the type), calling.slice()could fail or produce unexpected results. Consider adding a fallback:- title: params.firstMessageText.slice(0, 200), + title: (params.firstMessageText || '').slice(0, 200),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/api/src/app/agents/services/agent-conversation.service.ts` at line 85, The title assignment uses params.firstMessageText.slice(0, 200) without guarding against undefined or non-string values; update the code in AgentConversationService where title is set (the object with title: params.firstMessageText.slice(...)) to first coerce/validate params.firstMessageText (e.g., typeof === 'string' and length > 0) and fall back to a safe default ('' or a placeholder like 'New conversation') before slicing so slice is only called on a string.
100-101: Missing blank line before earlyreturn.As per coding guidelines, include a blank line before every
returnstatement.Suggested fix
); if (alreadyPresent) return; +Note: Since this is an early return on the same line as the condition, you could also refactor to:
- if (alreadyPresent) return; + if (alreadyPresent) { + return; + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/api/src/app/agents/services/agent-conversation.service.ts` around lines 100 - 101, The early-return line "if (alreadyPresent) return;" violates the blank-line-before-return rule; in the method inside AgentConversationService (the spot checking the alreadyPresent variable), insert a blank line before the return or rewrite the check as a multi-line conditional with braces (e.g., if (alreadyPresent) { return; }) and ensure there is a blank line immediately above the return so the function follows the coding guideline.
48-68: In-memory mutation of returned entity may cause stale data.The
existingentity is mutated in-memory on line 60 (existing.status = ConversationStatusEnum.ACTIVE) after updating the database. If the caller or any downstream code holds a reference to this entity, they'll see the updated status, but other fields may be stale (e.g.,updatedAtwon't reflect the database update). Consider re-fetching after the update or returning a fresh object.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/api/src/app/agents/services/agent-conversation.service.ts` around lines 48 - 68, The code mutates the in-memory existing ConversationEntity (variable existing) after calling conversationRepository.updateStatus inside createOrGetConversation, which can leave other fields (e.g., updatedAt) stale; change the flow so you do not mutate existing directly—either re-fetch the conversation from the DB after calling conversationRepository.updateStatus (call the repository's fetch/find method for the updated _id) and use that fresh object when calling ensureParticipant and returning, or have updateStatus return the updated entity and use that return value instead of assigning to existing.status; ensure ensureParticipant receives the fresh entity and return the fresh object to callers.libs/dal/src/repositories/conversation-activity/conversation-activity.repository.ts (5)
27-31: Missing blank line beforereturnstatement.As per coding guidelines, include a blank line before every
returnstatement.Suggested fix
limit = 20 ): Promise<ConversationActivityEntity[]> { + return this.find({ _environmentId: environmentId, _conversationId: conversationId }, '*', {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/dal/src/repositories/conversation-activity/conversation-activity.repository.ts` around lines 27 - 31, In the method that returns Promise<ConversationActivityEntity[]> (the function whose body currently returns this.find({ _environmentId: environmentId, _conversationId: conversationId }, '*', { sort: { createdAt: -1 }, limit, })), add a single blank line immediately before the return statement to comply with the coding guideline requiring a blank line before every return; no other changes needed.
47-62: Missing blank line beforereturnstatement.As per coding guidelines, include a blank line before every
returnstatement.Suggested fix
}): Promise<ConversationActivityEntity> { + return this.create({🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/dal/src/repositories/conversation-activity/conversation-activity.repository.ts` around lines 47 - 62, Add a blank line immediately before the return statement that calls this.create(...) in the method that constructs a ConversationActivityEntity (the block populating _conversationId, type: ConversationActivityTypeEnum.MESSAGE, platform, _integrationId, etc.); ensure there is one empty line separating the preceding code and the return to comply with the project's rule that every return statement is preceded by a blank line.
77-91: Missing blank line beforereturnstatement.As per coding guidelines, include a blank line before every
returnstatement.Suggested fix
}): Promise<ConversationActivityEntity> { + return this.create({🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/dal/src/repositories/conversation-activity/conversation-activity.repository.ts` around lines 77 - 91, Add a blank line immediately before the return statement in the function that returns Promise<ConversationActivityEntity> (the block that calls this.create) to follow the coding guideline; locate the method returning Promise<ConversationActivityEntity> that constructs the object with fields like identifier, _conversationId, type (ConversationActivityTypeEnum), senderType (ConversationActivitySenderTypeEnum), senderId, content, and then calls this.create, and insert one empty line directly above the return this.create(...) line.
105-119: Missing blank line beforereturnstatement.As per coding guidelines, include a blank line before every
returnstatement.Suggested fix
}): Promise<ConversationActivityEntity> { + return this.create({🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/dal/src/repositories/conversation-activity/conversation-activity.repository.ts` around lines 105 - 119, The method returning Promise<ConversationActivityEntity> is missing a blank line before the return; insert a single blank line immediately before the "return this.create({...})" statement in the function that builds the SIGNAL activity (references: ConversationActivityTypeEnum.SIGNAL, ConversationActivitySenderTypeEnum.SYSTEM, senderId set from params.agentId, and the this.create(...) call) so the file follows the coding guideline of having a blank line before every return.
23-32: Consider adding_organizationIdfor stricter multi-tenant isolation.The current implementation uses
_environmentIdwhich satisfies the constraint that requires either_environmentIdor_organizationId. However, passing both parameters would provide additional isolation boundaries. IforganizationIdis available from the caller, including it in the filter strengthens tenant isolation:{ _environmentId: environmentId, _organizationId: organizationId, _conversationId: conversationId }. This is an optional enhancement rather than a requirement.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/dal/src/repositories/conversation-activity/conversation-activity.repository.ts` around lines 23 - 32, The findByConversation method should optionally include _organizationId to tighten multi-tenant isolation: update the findByConversation signature to accept an optional organizationId parameter and, when provided, add _organizationId to the query filter alongside _environmentId and _conversationId before calling this.find; reference the ConversationActivityEntity return type and the fields _environmentId, _organizationId, and _conversationId so callers can pass organizationId when available.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/api/src/app/agents/dtos/agent-reply-payload.dto.ts`:
- Around line 22-27: SignalDto only validates type and lacks required key/value
fields for metadata signals, so create separate DTOs (e.g., MetadataSignalDto
with type: 'metadata', `@IsString`() key, and value typed/validated as unknown or
a safe persisted type, and TriggerSignalDto for other signal types) or implement
a discriminated-union DTO pattern, then change AgentReplyPayloadDto.signals to
accept an array of MetadataSignalDto | TriggerSignalDto; ensure the SIGNAL_TYPES
enum validation remains and the metadata branch matches the shape expected by
the handler (handle-agent-reply use case) that expects { type: 'metadata'; key:
string; value: unknown } so malformed payloads are rejected by validation.
In `@apps/api/src/app/agents/services/agent-subscriber-resolver.service.ts`:
- Around line 22-24: AgentSubscriberResolver.resolve currently ignores
integrationIdentifier causing cross-installation collisions for workspace-scoped
identities (Slack/Teams); update the lookup logic in
AgentSubscriberResolver.resolve (and the similar code around lines 34-40) to
include integrationIdentifier when building the query/filters (use
ResolveSubscriberParams.integrationIdentifier) so subscriber lookups are scoped
by the originating integration/workspace; ensure any use of
PLATFORM_ENDPOINT_CONFIG.identityKeys or platform-specific identity lookup
includes integrationIdentifier as an additional equality filter and adjust
DB/service query calls accordingly.
In `@apps/api/src/app/agents/services/bridge-executor.service.ts`:
- Around line 106-117: The resolveBridgeUrl path performs a cross-tenant lookup
because it reads EnvironmentRepository by bare _id; update resolveBridgeUrl (and
the other environment lookup later in the same function) to include the tenant
scope by filtering with the incoming config values (use config.organizationId
and/or config.environmentId) when querying EnvironmentRepository (e.g., change
the repository call to include {_id: envId, _organizationId:
config.organizationId} or {_id: envId, _environmentId: config.environmentId}) so
the bridge configuration lookup is always scoped to the tenant; ensure you use
the same scoped filter for both occurrences referenced in resolveBridgeUrl and
the later lookup in execute.
- Around line 82-96: AgentBridgeRequest lacks a stable delivery identifier, so
BridgeExecutorService.fireWithRetries() may POST identical events multiple times
without idempotency; add a deliveryId (or idempotencyKey) field to
AgentBridgeRequest and ensure BridgeExecutorService.fireWithRetries() generates
that key once from the triggering activity/message (or reuses message.id when
present) and includes it in both the request body and any outgoing idempotency
headers for every retry (also apply the same change in the related code paths
referenced around the 127-145 region). Ensure the deliveryId is stable across
retries, is sufficiently unique (e.g., UUIDv5 or deterministic hash of
activity), and is passed through all downstream callers that construct or
forward AgentBridgeRequest.
In `@apps/api/src/app/agents/services/chat-sdk.service.ts`:
- Around line 141-152: The Redis URL construction in the Chat creation block
(see Chat, createRedisState, instanceKey) doesn't handle TLS or auth; update the
URL builder to use the scheme rediss:// when process.env.REDIS_TLS_ENABLED is
truthy and include process.env.REDIS_PASSWORD in the authority component (e.g.,
:password@host:port) only when present (ensure empty/undefined password is
omitted), then pass that constructed url into createRedisState's url option so
secured Redis instances can connect.
In
`@apps/api/src/app/agents/usecases/handle-agent-reply/handle-agent-reply.usecase.ts`:
- Around line 91-117: The current Promise.all call can cause partial failures
leaving a persisted activity when postToConversation fails; change the flow in
handle-agent-reply to perform chatSdkService.postToConversation first and only
on success call activityRepository.createAgentActivity and
conversationRepository.touchActivity, and if you need stronger guarantees
implement either a retry/enqueue for postToConversation failures or a
compensation/rollback that deletes the created activity (use
activityRepository.delete by identifier) when postToConversation fails after
persistence; reference functions: chatSdkService.postToConversation,
activityRepository.createAgentActivity, conversationRepository.touchActivity to
locate and reorder/add retry/rollback logic.
In `@apps/api/src/app/agents/utils/express-to-web-request.ts`:
- Around line 25-35: The toWebRequest conversion currently always
JSON.stringify(req.body) when rawBody is missing which rewrites
string/form-encoded payloads and breaks signatures; update the logic in
toWebRequest (express-to-web-request) so you only JSON.stringify when req.body
is a plain object (e.g., typeof req.body === 'object' and not a
Buffer/Uint8Array/URLSearchParams), otherwise preserve req.body as-is
(string/Buffer/URLSearchParams) and do not overwrite content-type headers; if
rawBody is required for your webhook routes and none of these safe types are
present, throw a clear error asking for rawBody. Ensure you reference rawBody,
req.body, headers and the toWebRequest function when making this change.
In `@libs/dal/src/repositories/agent/agent.repository.ts`:
- Around line 15-24: The method findByIdForWebhook currently calls
this.MongooseModel.findById(...) which bypasses the EnforceEnvOrOrgIds
protections; either refactor so the lookup happens after
AgentCredentialService.resolve() establishes env/org context (move the lookup
out of findByIdForWebhook and use the inherited this.findOne({ _id: agentId })
or this.find(...) so the EnforceEnvOrOrgIds checks apply) or, if this exception
is unavoidable, replace the direct call with a clear, documented
security-exception comment and an explicit guard/assert that the caller (e.g.,
AgentCredentialService.resolve) has validated the webhook/feature-flag and
necessary permissions before invoking findByIdForWebhook; reference
findByIdForWebhook and this.MongooseModel in your change.
In `@libs/dal/src/repositories/conversation/conversation.schema.ts`:
- Around line 74-80: The conversation schema is missing the lastActivityAt field
that bridge-executor.service.ts expects; update the Conversation mongoose schema
(in conversation.schema.ts near messageCount/lastMessagePreview) to add a
lastActivityAt property with type Schema.Types.Date (or Date), set an
appropriate default (e.g., Date.now or null) and consider adding an index if
recency queries are used so persisted documents retain and expose recency for
conversation.lastActivityAt reads.
---
Nitpick comments:
In `@apps/api/src/app/agents/e2e/agents.e2e.ts`:
- Around line 11-13: The test suite sets
process.env.IS_CONVERSATIONAL_AGENTS_ENABLED in the existing before() hook but
doesn't restore it; capture the original value (e.g., const _orig =
process.env.IS_CONVERSATIONAL_AGENTS_ENABLED) before setting it in before(),
then add a matching after() hook that restores the environment variable (set it
back to _orig or delete it if undefined) to ensure
process.env.IS_CONVERSATIONAL_AGENTS_ENABLED is cleaned up after the agents.e2e
test run.
In `@apps/api/src/app/agents/guards/agent-conversation-enabled.guard.ts`:
- Around line 13-18: The guard currently calls featureFlagsService.getFlag with
empty-string fallbacks when request.user is missing; instead, update the
AgentConversationEnabledGuard (the code around getFlag /
FeatureFlagsKeysEnum.IS_CONVERSATIONAL_AGENTS_ENABLED) to check for a valid
request.user and required user.organizationId and user.environmentId up front
and return false (or throw early per your guard semantics) if they are absent,
so you do not invoke getFlag with empty context values; ensure the early-return
replaces the current invocation of this.featureFlagsService.getFlag when user or
IDs are missing.
In `@apps/api/src/app/agents/services/agent-conversation.service.ts`:
- Around line 153-155: The getHistory method in agent-conversation.service.ts is
missing a blank line before its return; update the async function
getHistory(environmentId: string, conversationId: string, limit = 20) to insert
a single blank line immediately before the return statement that calls
this.activityRepository.findByConversation so it conforms to the coding
guideline requiring a blank line before every return.
- Around line 65-68: Add a blank line immediately before the `return existing;`
statement in the method that calls `await this.ensureParticipant(existing,
params);` so the `return` is separated by a single empty line from the preceding
call; update the method in AgentConversationService (the function containing
`ensureParticipant` and `existing`) to follow the project's rule of a blank line
before every `return`.
- Around line 153-155: getHistory currently lacks an organizationId parameter
which breaks consistency with other methods (createOrGetConversation,
persistInboundMessage, updateChannelThread) that enforce org scoping; update the
getHistory method signature to accept organizationId (e.g.,
getHistory(organizationId: string, environmentId: string, conversationId:
string, limit = 20)), pass organizationId into the
activityRepository.findByConversation call (and adjust that repository method
signature if needed), and update all callers to provide organizationId so
org-level access control remains consistent.
- Line 85: The title assignment uses params.firstMessageText.slice(0, 200)
without guarding against undefined or non-string values; update the code in
AgentConversationService where title is set (the object with title:
params.firstMessageText.slice(...)) to first coerce/validate
params.firstMessageText (e.g., typeof === 'string' and length > 0) and fall back
to a safe default ('' or a placeholder like 'New conversation') before slicing
so slice is only called on a string.
- Around line 100-101: The early-return line "if (alreadyPresent) return;"
violates the blank-line-before-return rule; in the method inside
AgentConversationService (the spot checking the alreadyPresent variable), insert
a blank line before the return or rewrite the check as a multi-line conditional
with braces (e.g., if (alreadyPresent) { return; }) and ensure there is a blank
line immediately above the return so the function follows the coding guideline.
- Around line 48-68: The code mutates the in-memory existing ConversationEntity
(variable existing) after calling conversationRepository.updateStatus inside
createOrGetConversation, which can leave other fields (e.g., updatedAt) stale;
change the flow so you do not mutate existing directly—either re-fetch the
conversation from the DB after calling conversationRepository.updateStatus (call
the repository's fetch/find method for the updated _id) and use that fresh
object when calling ensureParticipant and returning, or have updateStatus return
the updated entity and use that return value instead of assigning to
existing.status; ensure ensureParticipant receives the fresh entity and return
the fresh object to callers.
In `@apps/api/src/app/agents/services/agent-inbound-handler.service.ts`:
- Around line 78-87: The thread.startTyping() call can expire if the
bridge/remote call takes longer, so implement a typing keep-alive: when you call
thread.startTyping() (in agent-inbound-handler.service near the code using
serializedThread and conversationService.updateChannelThread), start a repeating
timer (e.g., setInterval) that calls thread.startTyping() periodically (before
the platform timeout) and re-serializes the thread (thread.toJSON()) then calls
this.conversationService.updateChannelThread(...) to persist the refreshed
indicator; clear the interval and stop typing once the bridge response completes
or errors. Ensure the timer is cleared in both success and error paths to avoid
leaks.
In `@apps/api/src/app/agents/services/chat-sdk.service.ts`:
- Around line 162-197: The switch branches in chat-sdk.service.ts (cases for
AgentPlatformEnum.SLACK, TEAMS, WHATSAPP) use non-null assertions like
connectionAccessToken! and credentials.clientId! which defer missing-credential
errors into adapter libraries; replace these with explicit validation checks for
required fields (e.g., ensure connectionAccessToken and credentials.apiKey for
SLACK; credentials.clientId/secretKey/tenantId for TEAMS;
credentials.token/secretKey/apiToken/phoneNumberIdentification for WHATSAPP) and
throw BadRequestException with clear messages when a required field is missing
(or delegate to AgentCredentialService.resolve() to perform per-platform
validation) before calling
createSlackAdapter/createTeamsAdapter/createWhatsAppAdapter.
In
`@libs/dal/src/repositories/conversation-activity/conversation-activity.repository.ts`:
- Around line 27-31: In the method that returns
Promise<ConversationActivityEntity[]> (the function whose body currently returns
this.find({ _environmentId: environmentId, _conversationId: conversationId },
'*', { sort: { createdAt: -1 }, limit, })), add a single blank line immediately
before the return statement to comply with the coding guideline requiring a
blank line before every return; no other changes needed.
- Around line 47-62: Add a blank line immediately before the return statement
that calls this.create(...) in the method that constructs a
ConversationActivityEntity (the block populating _conversationId, type:
ConversationActivityTypeEnum.MESSAGE, platform, _integrationId, etc.); ensure
there is one empty line separating the preceding code and the return to comply
with the project's rule that every return statement is preceded by a blank line.
- Around line 77-91: Add a blank line immediately before the return statement in
the function that returns Promise<ConversationActivityEntity> (the block that
calls this.create) to follow the coding guideline; locate the method returning
Promise<ConversationActivityEntity> that constructs the object with fields like
identifier, _conversationId, type (ConversationActivityTypeEnum), senderType
(ConversationActivitySenderTypeEnum), senderId, content, and then calls
this.create, and insert one empty line directly above the return
this.create(...) line.
- Around line 105-119: The method returning Promise<ConversationActivityEntity>
is missing a blank line before the return; insert a single blank line
immediately before the "return this.create({...})" statement in the function
that builds the SIGNAL activity (references:
ConversationActivityTypeEnum.SIGNAL, ConversationActivitySenderTypeEnum.SYSTEM,
senderId set from params.agentId, and the this.create(...) call) so the file
follows the coding guideline of having a blank line before every return.
- Around line 23-32: The findByConversation method should optionally include
_organizationId to tighten multi-tenant isolation: update the findByConversation
signature to accept an optional organizationId parameter and, when provided, add
_organizationId to the query filter alongside _environmentId and _conversationId
before calling this.find; reference the ConversationActivityEntity return type
and the fields _environmentId, _organizationId, and _conversationId so callers
can pass organizationId when available.
In `@libs/dal/src/repositories/conversation/conversation.repository.ts`:
- Around line 81-92: touchActivity currently sets lastActivityAt to an ISO
string (new Date().toISOString()); change this to store a native Date object
(new Date()) and update the update call in touchActivity to set lastActivityAt:
new Date(). Also update the ConversationEntity type definitions for
lastActivityAt (and createdAt if applicable) from string to Date so the
repository and any mappers expect native Date objects; ensure any
serialization/deserialization code that reads/writes ConversationEntity handles
Date objects appropriately (e.g., when returning DTOs or persisting to MongoDB).
🪄 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: 9f05cbb0-560b-4d34-b80a-346fe5c26b80
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (35)
apps/api/package.jsonapps/api/src/app.module.tsapps/api/src/app/agents/agents-webhook.controller.tsapps/api/src/app/agents/agents.controller.tsapps/api/src/app/agents/agents.module.tsapps/api/src/app/agents/dtos/agent-event.enum.tsapps/api/src/app/agents/dtos/agent-platform.enum.tsapps/api/src/app/agents/dtos/agent-reply-payload.dto.tsapps/api/src/app/agents/e2e/agents.e2e.tsapps/api/src/app/agents/guards/agent-conversation-enabled.guard.tsapps/api/src/app/agents/services/agent-conversation.service.tsapps/api/src/app/agents/services/agent-credential.service.tsapps/api/src/app/agents/services/agent-inbound-handler.service.tsapps/api/src/app/agents/services/agent-subscriber-resolver.service.tsapps/api/src/app/agents/services/bridge-executor.service.tsapps/api/src/app/agents/services/chat-sdk.service.tsapps/api/src/app/agents/usecases/handle-agent-reply/handle-agent-reply.command.tsapps/api/src/app/agents/usecases/handle-agent-reply/handle-agent-reply.usecase.tsapps/api/src/app/agents/usecases/index.tsapps/api/src/app/agents/utils/express-to-web-request.tsapps/api/src/app/agents/utils/platform-endpoint-config.tsapps/api/src/app/agents/utils/provider-to-platform.tsapps/api/src/bootstrap.tsenterprise/packages/api/package.jsonlibs/dal/src/index.tslibs/dal/src/repositories/agent/agent.repository.tslibs/dal/src/repositories/channel-endpoint/channel-endpoint.repository.tslibs/dal/src/repositories/conversation-activity/conversation-activity.entity.tslibs/dal/src/repositories/conversation-activity/conversation-activity.repository.tslibs/dal/src/repositories/conversation-activity/conversation-activity.schema.tslibs/dal/src/repositories/conversation-activity/index.tslibs/dal/src/repositories/conversation/conversation.entity.tslibs/dal/src/repositories/conversation/conversation.repository.tslibs/dal/src/repositories/conversation/conversation.schema.tslibs/dal/src/repositories/conversation/index.ts
Comment on lines
+22
to
+27
| export class SignalDto { | ||
| @ApiProperty({ enum: SIGNAL_TYPES }) | ||
| @IsString() | ||
| @IsIn(SIGNAL_TYPES) | ||
| type: (typeof SIGNAL_TYPES)[number]; | ||
| } |
Contributor
There was a problem hiding this comment.
SignalDto is missing key and value fields for metadata signals.
The use-case at handle-agent-reply.usecase.ts:144 expects metadata signals to have { type: 'metadata'; key: string; value: unknown }, but this DTO only validates type. Incoming metadata signals won't have key/value validated, allowing malformed payloads to reach the use-case.
Consider using a discriminated union or separate DTOs per signal type:
Proposed fix
+export class MetadataSignalDto {
+ `@ApiProperty`({ enum: ['metadata'] })
+ `@IsString`()
+ `@IsIn`(['metadata'])
+ type: 'metadata';
+
+ `@ApiProperty`()
+ `@IsString`()
+ `@IsNotEmpty`()
+ key: string;
+
+ `@ApiProperty`()
+ value: unknown;
+}
+
+export class TriggerSignalDto {
+ `@ApiProperty`({ enum: ['trigger'] })
+ `@IsString`()
+ `@IsIn`(['trigger'])
+ type: 'trigger';
+
+ // Add trigger-specific fields as needed
+}
-export class SignalDto {
- `@ApiProperty`({ enum: SIGNAL_TYPES })
- `@IsString`()
- `@IsIn`(SIGNAL_TYPES)
- type: (typeof SIGNAL_TYPES)[number];
-}Then update AgentReplyPayloadDto.signals to accept an array of MetadataSignalDto | TriggerSignalDto.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/api/src/app/agents/dtos/agent-reply-payload.dto.ts` around lines 22 -
27, SignalDto only validates type and lacks required key/value fields for
metadata signals, so create separate DTOs (e.g., MetadataSignalDto with type:
'metadata', `@IsString`() key, and value typed/validated as unknown or a safe
persisted type, and TriggerSignalDto for other signal types) or implement a
discriminated-union DTO pattern, then change AgentReplyPayloadDto.signals to
accept an array of MetadataSignalDto | TriggerSignalDto; ensure the SIGNAL_TYPES
enum validation remains and the metadata branch matches the shape expected by
the handler (handle-agent-reply use case) that expects { type: 'metadata'; key:
string; value: unknown } so malformed payloads are rejected by validation.
Comment on lines
+82
to
+96
| export interface AgentBridgeRequest { | ||
| version: 1; | ||
| timestamp: string; | ||
| event: AgentEventEnum; | ||
| agentId: string; | ||
| replyUrl: string; | ||
| conversationId: string; | ||
| integrationIdentifier: string; | ||
| message: BridgeMessage | null; | ||
| conversation: BridgeConversation; | ||
| subscriber: BridgeSubscriber | null; | ||
| history: BridgeHistoryEntry[]; | ||
| platform: string; | ||
| platformContext: BridgePlatformContext; | ||
| } |
Contributor
There was a problem hiding this comment.
Bridge retries can replay customer-side mutations.
BridgeExecutorService.fireWithRetries() can POST the same event three times, but neither AgentBridgeRequest nor the headers carry a stable delivery identifier. If attempt 1 succeeds server-side and the connection drops before the response arrives, attempt 2 is indistinguishable from a brand-new event and can rerun downstream workflows or resolve logic. Add a stable deliveryId/idempotency key derived from the triggering activity or message and reuse it across every retry.
Also applies to: 127-145
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/api/src/app/agents/services/bridge-executor.service.ts` around lines 82
- 96, AgentBridgeRequest lacks a stable delivery identifier, so
BridgeExecutorService.fireWithRetries() may POST identical events multiple times
without idempotency; add a deliveryId (or idempotencyKey) field to
AgentBridgeRequest and ensure BridgeExecutorService.fireWithRetries() generates
that key once from the triggering activity/message (or reuses message.id when
present) and includes it in both the request body and any outgoing idempotency
headers for every retry (also apply the same change in the related code paths
referenced around the 127-145 region). Ensure the deliveryId is stable across
retries, is sufficiently unique (e.g., UUIDv5 or deterministic hash of
activity), and is passed through all downstream callers that construct or
forward AgentBridgeRequest.
Comment on lines
+91
to
+117
| await Promise.all([ | ||
| this.chatSdkService.postToConversation( | ||
| conversation._agentId, | ||
| command.integrationIdentifier, | ||
| channel.platform, | ||
| channel.serializedThread!, | ||
| text | ||
| ), | ||
| this.activityRepository.createAgentActivity({ | ||
| identifier: `act-${shortId(8)}`, | ||
| conversationId: conversation._id, | ||
| platform: channel.platform, | ||
| integrationId: channel._integrationId, | ||
| platformThreadId: channel.platformThreadId, | ||
| agentId: command.agentIdentifier, | ||
| content: text, | ||
| type, | ||
| environmentId: command.environmentId, | ||
| organizationId: command.organizationId, | ||
| }), | ||
| this.conversationRepository.touchActivity( | ||
| command.environmentId, | ||
| command.organizationId, | ||
| conversation._id, | ||
| text | ||
| ), | ||
| ]); |
Contributor
There was a problem hiding this comment.
Partial failure in Promise.all may leave inconsistent state.
If postToConversation fails (e.g., network error to Slack), the activity record and touchActivity update may still succeed, leading to a persisted activity for a message that was never delivered. Consider:
- Sequential execution with rollback on failure, or
- Accepting eventual consistency and adding a retry/reconciliation mechanism, or
- Using a transaction if the operations support it.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@apps/api/src/app/agents/usecases/handle-agent-reply/handle-agent-reply.usecase.ts`
around lines 91 - 117, The current Promise.all call can cause partial failures
leaving a persisted activity when postToConversation fails; change the flow in
handle-agent-reply to perform chatSdkService.postToConversation first and only
on success call activityRepository.createAgentActivity and
conversationRepository.touchActivity, and if you need stronger guarantees
implement either a retry/enqueue for postToConversation failures or a
compensation/rollback that deletes the created activity (use
activityRepository.delete by identifier) when postToConversation fails after
persistence; reference functions: chatSdkService.postToConversation,
activityRepository.createAgentActivity, conversationRepository.touchActivity to
locate and reorder/add retry/rollback logic.
Comment on lines
+25
to
+35
| let body: BodyInit | undefined; | ||
| if (req.method !== 'GET' && req.method !== 'HEAD') { | ||
| const rawBody = (req as ExpressRequest & { rawBody?: Buffer }).rawBody; | ||
| if (rawBody) { | ||
| body = rawBody; | ||
| } else if (req.body !== undefined) { | ||
| body = JSON.stringify(req.body); | ||
| if (!headers.has('content-type')) { | ||
| headers.set('content-type', 'application/json'); | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
Don't re-serialize webhook bodies as JSON by default.
When toWebRequest() falls back to JSON.stringify(req.body), it rewrites string or form-encoded payloads and changes the signed bytes. That breaks signature verification and can make Slack-style webhooks unreadable; either require rawBody on these routes or only JSON-encode plain objects.
🛠️ Minimal fix
if (req.method !== 'GET' && req.method !== 'HEAD') {
const rawBody = (req as ExpressRequest & { rawBody?: Buffer }).rawBody;
if (rawBody) {
body = rawBody;
+ } else if (Buffer.isBuffer(req.body) || typeof req.body === 'string') {
+ body = req.body;
} else if (req.body !== undefined) {
+ const contentType = headers.get('content-type') || '';
+ if (contentType && !contentType.includes('application/json')) {
+ throw new Error(`rawBody is required for ${contentType} webhook requests`);
+ }
body = JSON.stringify(req.body);
if (!headers.has('content-type')) {
headers.set('content-type', 'application/json');
}
}
}📝 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
| let body: BodyInit | undefined; | |
| if (req.method !== 'GET' && req.method !== 'HEAD') { | |
| const rawBody = (req as ExpressRequest & { rawBody?: Buffer }).rawBody; | |
| if (rawBody) { | |
| body = rawBody; | |
| } else if (req.body !== undefined) { | |
| body = JSON.stringify(req.body); | |
| if (!headers.has('content-type')) { | |
| headers.set('content-type', 'application/json'); | |
| } | |
| } | |
| let body: BodyInit | undefined; | |
| if (req.method !== 'GET' && req.method !== 'HEAD') { | |
| const rawBody = (req as ExpressRequest & { rawBody?: Buffer }).rawBody; | |
| if (rawBody) { | |
| body = rawBody; | |
| } else if (Buffer.isBuffer(req.body) || typeof req.body === 'string') { | |
| body = req.body; | |
| } else if (req.body !== undefined) { | |
| const contentType = headers.get('content-type') || ''; | |
| if (contentType && !contentType.includes('application/json')) { | |
| throw new Error(`rawBody is required for ${contentType} webhook requests`); | |
| } | |
| body = JSON.stringify(req.body); | |
| if (!headers.has('content-type')) { | |
| headers.set('content-type', 'application/json'); | |
| } | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/api/src/app/agents/utils/express-to-web-request.ts` around lines 25 -
35, The toWebRequest conversion currently always JSON.stringify(req.body) when
rawBody is missing which rewrites string/form-encoded payloads and breaks
signatures; update the logic in toWebRequest (express-to-web-request) so you
only JSON.stringify when req.body is a plain object (e.g., typeof req.body ===
'object' and not a Buffer/Uint8Array/URLSearchParams), otherwise preserve
req.body as-is (string/Buffer/URLSearchParams) and do not overwrite content-type
headers; if rawBody is required for your webhook routes and none of these safe
types are present, throw a clear error asking for rawBody. Ensure you reference
rawBody, req.body, headers and the toWebRequest function when making this
change.
- Scope environment query by _organizationId in BridgeExecutorService.resolveBridgeUrl to prevent cross-tenant bridge URL lookups - Add Redis TLS (rediss://) and password auth support in Chat SDK instance creation for production Redis environments - Add lastActivityAt to conversation Mongoose schema (entity had it, schema was missing) - Scope subscriber resolver by integrationIdentifier to prevent cross-workspace subscriber collision when multiple Slack workspaces share an environment Made-with: Cursor
…ries Stable identifier derived from conversationId + messageId (or event type for lifecycle events). Same value on every retry attempt so the customer's serve() handler can deduplicate if needed. Made-with: Cursor
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
♻️ Duplicate comments (1)
apps/api/src/app/agents/services/bridge-executor.service.ts (1)
82-96:⚠️ Potential issue | 🔴 CriticalAdd a stable delivery ID for bridge retries.
fireWithRetries()can POST the same event three times, but the request still has no stable idempotency key. If attempt 1 succeeds server-side and the response is lost, later attempts are indistinguishable from a new event and can rerun customer workflows. Add adeliveryIdtoAgentBridgeRequestand reuse it in both the body and an idempotency header on every retry.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/api/src/app/agents/services/bridge-executor.service.ts` around lines 82 - 96, Add a stable UUID deliveryId to the AgentBridgeRequest interface and ensure fireWithRetries reuses it across retries: update the AgentBridgeRequest type (add deliveryId: string) and when constructing requests in the fireWithRetries POST flow include that deliveryId in the JSON body and send it as an idempotency header (e.g., "Idempotency-Key" or "X-Delivery-Id") on every attempt; generate the deliveryId once before the first attempt (if missing) so subsequent retries use the same value and the server can deduplicate.
🧹 Nitpick comments (3)
apps/api/src/app/agents/services/chat-sdk.service.ts (3)
158-201: Non-null assertions on credentials may cause unclear runtime errors.The
!assertions at lines 170-171, 180-182, and 191-194 bypass null checks. If credentials are misconfigured or missing required fields, the runtime error from the adapter will be less actionable than an explicit validation error.Consider validating required credentials before passing them to adapters.
♻️ Example validation for Slack case
case AgentPlatformEnum.SLACK: { const { createSlackAdapter } = await esmImport('@chat-adapter/slack'); + if (!connectionAccessToken || !credentials.apiKey) { + throw new BadRequestException( + `Slack adapter requires botToken (connectionAccessToken) and signingSecret (apiKey)` + ); + } return { slack: createSlackAdapter({ - botToken: connectionAccessToken!, - signingSecret: credentials.apiKey!, + botToken: connectionAccessToken, + signingSecret: credentials.apiKey, }), }; }Apply similar validation for Teams and WhatsApp cases.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/api/src/app/agents/services/chat-sdk.service.ts` around lines 158 - 201, In buildAdapters, avoid non-null assertions on credentials (e.g., connectionAccessToken!, credentials.apiKey!, credentials.clientId!, credentials.secretKey!, credentials.tenantId!, credentials.token!, credentials.apiKey!, credentials.phoneNumberIdentification!) by explicitly validating required fields per platform before calling createSlackAdapter/createTeamsAdapter/createWhatsAppAdapter; for each AgentPlatformEnum case check the specific credential keys, throw a BadRequestException with a clear message listing the missing fields if any, and only then pass the validated values to the adapter factory functions.
72-82: Collect keys before deleting to avoid iterator invalidation.Iterating over
this.instances.keys()while callingdelete()can cause undefined behavior or skip entries depending on the LRU cache's iterator implementation.♻️ Proposed fix
evict(agentId: string, integrationIdentifier?: string) { if (integrationIdentifier) { this.instances.delete(`${agentId}:${integrationIdentifier}`); } else { - for (const key of this.instances.keys()) { - if (key.startsWith(`${agentId}:`)) { - this.instances.delete(key); - } - } + const keysToDelete = [...this.instances.keys()].filter((key) => key.startsWith(`${agentId}:`)); + for (const key of keysToDelete) { + this.instances.delete(key); + } } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/api/src/app/agents/services/chat-sdk.service.ts` around lines 72 - 82, The evict method currently iterates this.instances.keys() and deletes entries during iteration which can invalidate the iterator; modify evict (the evict(agentId: string, integrationIdentifier?: string) method) to first collect matching keys into a temporary array (e.g., const keys = [...this.instances.keys()] or filter to matching keys) and then loop over that array to call this.instances.delete(key), and keep the existing fast-path when integrationIdentifier is provided to delete the single `${agentId}:${integrationIdentifier}` key.
97-112: Useconfig.platforminstead of theplatformparameter.The
platformparameter is a loosestringtype whileconfig.platformis the resolvedAgentPlatformEnum. Since the integration and its platform are uniquely identified byagentIdandintegrationIdentifier(which are already resolved viaagentCredentialService.resolve), the platform is already known and should come from the resolved config rather than accepting it as a separate parameter. Usingconfig.platformensures type consistency and eliminates redundancy.♻️ Proposed fix
async postToConversation( agentId: string, integrationIdentifier: string, - platform: string, serializedThread: Record<string, unknown>, message: string ): Promise<void> { const config = await this.agentCredentialService.resolve(agentId, integrationIdentifier); const instanceKey = `${agentId}:${integrationIdentifier}`; const chat = await this.getOrCreate(instanceKey, agentId, config.platform, config); const { ThreadImpl } = await esmImport('chat'); - const adapter = chat.getAdapter(platform); + const adapter = chat.getAdapter(config.platform); const thread = ThreadImpl.fromJSON(serializedThread, adapter); await thread.post(message); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/api/src/app/agents/services/chat-sdk.service.ts` around lines 97 - 112, In postToConversation, stop using the loose platform parameter and instead use the resolved enum from config.platform: after calling agentCredentialService.resolve(agentId, integrationIdentifier) and creating/getting the chat instance via getOrCreate, pass config.platform into chat.getAdapter and any other platform-dependent calls (e.g., chat.getAdapter(platform) → chat.getAdapter(config.platform)); update ThreadImpl.fromJSON usage if it relies on the adapter. Also remove or ignore the platform parameter from postToConversation signature/usage to avoid redundancy and ensure type consistency.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/api/src/app/agents/services/bridge-executor.service.ts`:
- Around line 110-124: The bridge preparation steps (resolveBridgeUrl,
getDecryptedSecretKey.execute, buildPayload, buildNovuSignatureHeader) can throw
and currently are not caught; wrap the entire block that resolves the URL,
decrypts the secret, builds the payload and signature, and calls fireWithRetries
in a local try/catch inside execute(), and on error log a clear best-effort
message (including agentIdentifier and error) and return/skip dispatch so bridge
misconfiguration doesn't reject the main execute flow; keep fireWithRetries call
detached as before but only invoked if the try block succeeds.
- Around line 122-123: The error log currently prints the full bridgeUrl (in the
fireWithRetries catch), which may leak credentials or tenant data; update the
catch handler that calls this.logger.error (referencing fireWithRetries,
bridgeUrl, agentIdentifier, MAX_RETRIES) to log a redacted representation
instead: parse bridgeUrl with the URL constructor and log only the origin/host
(e.g., url.origin or url.hostname + port) or a masked string that removes
userinfo and query params, and include that redacted value in the message rather
than the full bridgeUrl while preserving the existing error and context.
- Around line 115-117: Update the GetDecryptedSecretKey use case so its
repository query is tenant-scoped: change the EnvironmentRepository lookup in
GetDecryptedSecretKey.execute
(libs/application-generic/src/usecases/get-decrypted-secret-key/get-decrypted-secret-key.usecase.ts)
to include the organizationId (and/or environmentId) in the filter instead of
querying by _id only (e.g., filter by { _id: command.environmentId,
_organizationId: command.organizationId } or the equivalent keys used across the
codebase). Adjust the repository method signature or types if needed so the
enforcement constraint requires _organizationId/_environmentId in filters, and
ensure the use case uses the organizationId passed in via
GetDecryptedSecretKeyCommand.create.
- Around line 138-145: The outbound fetch in BridgeExecutorService (the call
that awaits fetch(url, { method: 'POST', headers: { 'Content-Type':
'application/json', 'x-novu-signature': signatureHeader }, body })) needs a
per-attempt timeout: wrap each fetch attempt in an AbortController, start a
timer (e.g., setTimeout) that calls controller.abort() after the per-attempt
deadline, pass controller.signal into fetch, and clear the timer after fetch
completes; ensure abort errors are caught and re-thrown or converted into the
same retryable error path so the retry loop advances on timeouts. Also ensure
the controller/timer are cleaned up on both success and error to avoid leaks.
In `@apps/api/src/app/agents/services/chat-sdk.service.ts`:
- Around line 114-128: The getOrCreate method can create duplicate Chat
instances under concurrency; fix it by introducing a pending-creation map keyed
by instanceKey to store in-flight Promise<Chat>, check pending map after
instances.get() and before calling createChatInstance, and if absent add the
createChatInstance() Promise to pending; await that promise, then on success
call registerEventHandlers and set the resolved chat into instances and remove
the pending entry (ensure pending is cleared on error too). Update getOrCreate
to return existing instance if found, otherwise return the pending promise if
present, and only create/register/set when no pending exists so
createChatInstance, registerEventHandlers, instances.set, and pending map
operations are synchronized.
In `@libs/dal/src/repositories/conversation/conversation.schema.ts`:
- Around line 99-100: The existing conversationSchema.index calls do not include
all fields used by repository filters: update the index used by
findByPlatformThread to include _environmentId, _organizationId and
channels.platformThreadId (so the index matches the query predicate in
findByPlatformThread), and update the index used by findActiveByParticipant to
include _environmentId, _organizationId, participants.id, participants.type and
status (to match the $elemMatch on participants used in
findActiveByParticipant); modify the conversationSchema.index(...) entries
accordingly so the index keys mirror the exact filter fields referenced by those
methods.
---
Duplicate comments:
In `@apps/api/src/app/agents/services/bridge-executor.service.ts`:
- Around line 82-96: Add a stable UUID deliveryId to the AgentBridgeRequest
interface and ensure fireWithRetries reuses it across retries: update the
AgentBridgeRequest type (add deliveryId: string) and when constructing requests
in the fireWithRetries POST flow include that deliveryId in the JSON body and
send it as an idempotency header (e.g., "Idempotency-Key" or "X-Delivery-Id") on
every attempt; generate the deliveryId once before the first attempt (if
missing) so subsequent retries use the same value and the server can
deduplicate.
---
Nitpick comments:
In `@apps/api/src/app/agents/services/chat-sdk.service.ts`:
- Around line 158-201: In buildAdapters, avoid non-null assertions on
credentials (e.g., connectionAccessToken!, credentials.apiKey!,
credentials.clientId!, credentials.secretKey!, credentials.tenantId!,
credentials.token!, credentials.apiKey!, credentials.phoneNumberIdentification!)
by explicitly validating required fields per platform before calling
createSlackAdapter/createTeamsAdapter/createWhatsAppAdapter; for each
AgentPlatformEnum case check the specific credential keys, throw a
BadRequestException with a clear message listing the missing fields if any, and
only then pass the validated values to the adapter factory functions.
- Around line 72-82: The evict method currently iterates this.instances.keys()
and deletes entries during iteration which can invalidate the iterator; modify
evict (the evict(agentId: string, integrationIdentifier?: string) method) to
first collect matching keys into a temporary array (e.g., const keys =
[...this.instances.keys()] or filter to matching keys) and then loop over that
array to call this.instances.delete(key), and keep the existing fast-path when
integrationIdentifier is provided to delete the single
`${agentId}:${integrationIdentifier}` key.
- Around line 97-112: In postToConversation, stop using the loose platform
parameter and instead use the resolved enum from config.platform: after calling
agentCredentialService.resolve(agentId, integrationIdentifier) and
creating/getting the chat instance via getOrCreate, pass config.platform into
chat.getAdapter and any other platform-dependent calls (e.g.,
chat.getAdapter(platform) → chat.getAdapter(config.platform)); update
ThreadImpl.fromJSON usage if it relies on the adapter. Also remove or ignore the
platform parameter from postToConversation signature/usage to avoid redundancy
and ensure type consistency.
🪄 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: a89688cc-a6ed-4804-95e2-cf993203f30f
📒 Files selected for processing (5)
apps/api/src/app/agents/services/agent-subscriber-resolver.service.tsapps/api/src/app/agents/services/bridge-executor.service.tsapps/api/src/app/agents/services/chat-sdk.service.tslibs/dal/src/repositories/channel-endpoint/channel-endpoint.repository.tslibs/dal/src/repositories/conversation/conversation.schema.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- libs/dal/src/repositories/channel-endpoint/channel-endpoint.repository.ts
- apps/api/src/app/agents/services/agent-subscriber-resolver.service.ts
Comment on lines
+115
to
+117
| const secretKey = await this.getDecryptedSecretKey.execute( | ||
| GetDecryptedSecretKeyCommand.create({ environmentId: config.environmentId, organizationId: config.organizationId }) | ||
| ); |
Contributor
There was a problem hiding this comment.
Secret-key lookup is still not tenant-scoped.
This call now passes organizationId, but GetDecryptedSecretKey.execute() still queries EnvironmentRepository by _id only in libs/application-generic/src/usecases/get-decrypted-secret-key/get-decrypted-secret-key.usecase.ts:10-18. That leaves this path able to read the wrong tenant's signing secret if an incorrect environment ID reaches it. The use case needs the same _organizationId scoping you added for resolveBridgeUrl().
Based on learnings, all query methods must include _environmentId or _organizationId in their filter to satisfy the enforcement type constraint.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/api/src/app/agents/services/bridge-executor.service.ts` around lines 115
- 117, Update the GetDecryptedSecretKey use case so its repository query is
tenant-scoped: change the EnvironmentRepository lookup in
GetDecryptedSecretKey.execute
(libs/application-generic/src/usecases/get-decrypted-secret-key/get-decrypted-secret-key.usecase.ts)
to include the organizationId (and/or environmentId) in the filter instead of
querying by _id only (e.g., filter by { _id: command.environmentId,
_organizationId: command.organizationId } or the equivalent keys used across the
codebase). Adjust the repository method signature or types if needed so the
enforcement constraint requires _organizationId/_environmentId in filters, and
ensure the use case uses the organizationId passed in via
GetDecryptedSecretKeyCommand.create.
Comment on lines
+138
to
+145
| const response = await fetch(url, { | ||
| method: 'POST', | ||
| headers: { | ||
| 'Content-Type': 'application/json', | ||
| 'x-novu-signature': signatureHeader, | ||
| }, | ||
| body, | ||
| }); |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n apps/api/src/app/agents/services/bridge-executor.service.ts | sed -n '120,160p'Repository: novuhq/novu
Length of output: 1812
🏁 Script executed:
head -50 apps/api/src/app/agents/services/bridge-executor.service.tsRepository: novuhq/novu
Length of output: 1297
🏁 Script executed:
grep -n "MAX_RETRIES\|RETRY_BASE_DELAY\|REQUEST_TIMEOUT\|AbortController\|setTimeout" apps/api/src/app/agents/services/bridge-executor.service.tsRepository: novuhq/novu
Length of output: 464
🏁 Script executed:
rg -n "fetch\(" apps/api --type ts --type tsx -B 2 -A 5 | head -100Repository: novuhq/novu
Length of output: 82
🏁 Script executed:
grep -r "AbortController\|signal.*controller" apps/api --include="*.ts" --include="*.tsx" | head -20Repository: novuhq/novu
Length of output: 37
🏁 Script executed:
rg -n "fetch\(" apps/api --type ts -B 2 -A 5 | head -150Repository: novuhq/novu
Length of output: 1796
🏁 Script executed:
rg "AbortController" apps/api --type tsRepository: novuhq/novu
Length of output: 37
🏁 Script executed:
cat -n apps/api/src/app/agents/services/bridge-executor.service.ts | sed -n '127,165p'Repository: novuhq/novu
Length of output: 1553
Add per-attempt timeout to the outbound fetch call.
The retry loop has no timeout deadline on individual fetch attempts. If a connection hangs, the entire loop blocks indefinitely and retries never advance. Each attempt needs its own timeout, with timeout errors treated as retryable.
🛠️ Minimal fix
+const REQUEST_TIMEOUT_MS = 5000;
+
for (let attempt = 0; attempt <= MAX_RETRIES; attempt++) {
try {
- const response = await fetch(url, {
- method: 'POST',
- headers: {
- 'Content-Type': 'application/json',
- 'x-novu-signature': signatureHeader,
- },
- body,
- });
+ const controller = new AbortController();
+ const timeout = setTimeout(() => controller.abort(), REQUEST_TIMEOUT_MS);
+
+ try {
+ const response = await fetch(url, {
+ method: 'POST',
+ headers: {
+ 'Content-Type': 'application/json',
+ 'x-novu-signature': signatureHeader,
+ },
+ body,
+ signal: controller.signal,
+ });
+ } finally {
+ clearTimeout(timeout);
+ }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/api/src/app/agents/services/bridge-executor.service.ts` around lines 138
- 145, The outbound fetch in BridgeExecutorService (the call that awaits
fetch(url, { method: 'POST', headers: { 'Content-Type': 'application/json',
'x-novu-signature': signatureHeader }, body })) needs a per-attempt timeout:
wrap each fetch attempt in an AbortController, start a timer (e.g., setTimeout)
that calls controller.abort() after the per-attempt deadline, pass
controller.signal into fetch, and clear the timer after fetch completes; ensure
abort errors are caught and re-thrown or converted into the same retryable error
path so the retry loop advances on timeouts. Also ensure the controller/timer
are cleaned up on both success and error to avoid leaks.
Comment on lines
+99
to
+100
| conversationSchema.index({ _environmentId: 1, 'channels.platformThreadId': 1 }); | ||
| conversationSchema.index({ _environmentId: 1, 'participants.id': 1, status: 1 }); |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify query predicates in repository methods vs current schema indexes.
rg -n -C4 "findByPlatformThread|findActiveByParticipant|\\$elemMatch|channels\\.platformThreadId|participants" libs/dal/src/repositories/conversation/conversation.repository.ts
rg -n -C3 "conversationSchema\\.index" libs/dal/src/repositories/conversation/conversation.schema.tsRepository: novuhq/novu
Length of output: 1713
Align indexes with repository filters (_organizationId and participant type).
The indexes at lines 99–100 are missing fields that appear in both query filter predicates:
findByPlatformThreadfilters on_environmentId + _organizationId + channels.platformThreadId(line 26–27), but the index at line 99 omits_organizationIdfindActiveByParticipantuses$elemMatchwith bothidandtype(line 48), plus_organizationIdandstatus, but the index at line 100 omits both_organizationIdandparticipants.type
Update indexes to include all filter fields for full query optimization:
Suggested index replacements
-conversationSchema.index({ _environmentId: 1, 'channels.platformThreadId': 1 });
-conversationSchema.index({ _environmentId: 1, 'participants.id': 1, status: 1 });
+conversationSchema.index({ _environmentId: 1, _organizationId: 1, 'channels.platformThreadId': 1 });
+conversationSchema.index({
+ _environmentId: 1,
+ _organizationId: 1,
+ 'participants.id': 1,
+ 'participants.type': 1,
+ status: 1,
+});📝 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
| conversationSchema.index({ _environmentId: 1, 'channels.platformThreadId': 1 }); | |
| conversationSchema.index({ _environmentId: 1, 'participants.id': 1, status: 1 }); | |
| conversationSchema.index({ _environmentId: 1, _organizationId: 1, 'channels.platformThreadId': 1 }); | |
| conversationSchema.index({ | |
| _environmentId: 1, | |
| _organizationId: 1, | |
| 'participants.id': 1, | |
| 'participants.type': 1, | |
| status: 1, | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@libs/dal/src/repositories/conversation/conversation.schema.ts` around lines
99 - 100, The existing conversationSchema.index calls do not include all fields
used by repository filters: update the index used by findByPlatformThread to
include _environmentId, _organizationId and channels.platformThreadId (so the
index matches the query predicate in findByPlatformThread), and update the index
used by findActiveByParticipant to include _environmentId, _organizationId,
participants.id, participants.type and status (to match the $elemMatch on
participants used in findActiveByParticipant); modify the
conversationSchema.index(...) entries accordingly so the index keys mirror the
exact filter fields referenced by those methods.
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (4)
apps/api/src/app/agents/services/bridge-executor.service.ts (4)
111-123:⚠️ Potential issue | 🟠 MajorMake bridge preparation failures best-effort as well.
resolveBridgeUrl, secret-key decryption, payload/signature building can throw before the detached retry.catch, soexecute()can still reject the caller flow. Wrap preparation + dispatch setup in a localtry/catchand skip bridge dispatch on failure.🛠️ Minimal fix
async execute(params: BridgeExecutorParams): Promise<void> { const { config, event } = params; const agentIdentifier = config.agentIdentifier; - const bridgeUrl = await this.resolveBridgeUrl(config.environmentId, config.organizationId, agentIdentifier, event); - if (!bridgeUrl) { - return; - } - - const secretKey = await this.getDecryptedSecretKey.execute( - GetDecryptedSecretKeyCommand.create({ environmentId: config.environmentId, organizationId: config.organizationId }) - ); - - const payload = this.buildPayload(params); - const signatureHeader = buildNovuSignatureHeader(secretKey, payload); - - this.fireWithRetries(bridgeUrl, payload, signatureHeader, agentIdentifier).catch((err) => { - this.logger.error(err, `[agent:${agentIdentifier}] Bridge call to ${bridgeUrl} failed after ${MAX_RETRIES + 1} attempts`); - }); + try { + const bridgeUrl = await this.resolveBridgeUrl( + config.environmentId, + config.organizationId, + agentIdentifier, + event + ); + if (!bridgeUrl) { + return; + } + + const secretKey = await this.getDecryptedSecretKey.execute( + GetDecryptedSecretKeyCommand.create({ + environmentId: config.environmentId, + organizationId: config.organizationId, + }) + ); + + const payload = this.buildPayload(params); + const signatureHeader = buildNovuSignatureHeader(secretKey, payload); + + this.fireWithRetries(bridgeUrl, payload, signatureHeader, agentIdentifier).catch((err) => { + this.logger.error(err, `[agent:${agentIdentifier}] Bridge call failed after ${MAX_RETRIES + 1} attempts`); + }); + } catch (err) { + this.logger.warn(err, `[agent:${agentIdentifier}] Bridge dispatch skipped due to setup error`); + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/api/src/app/agents/services/bridge-executor.service.ts` around lines 111 - 123, The preparation steps before dispatch (resolveBridgeUrl, getDecryptedSecretKey.execute, buildPayload, buildNovuSignatureHeader) can throw and currently run outside the detached retry, so wrap the entire preparation and dispatch setup in a local try/catch inside execute(); if any of resolveBridgeUrl, the secret-key decryption, buildPayload, or buildNovuSignatureHeader throws, catch the error, log or noop, and return early (skip calling fireWithRetries) so failures are best-effort and do not reject the caller flow; keep the existing detached .catch on fireWithRetries for retries/errors during the actual HTTP dispatch.
123-125:⚠️ Potential issue | 🟠 MajorRedact bridge URL in failure logs.
Logging the full customer-configured URL can leak credentials/query data into logs. Log only origin/host (or masked URL) in the error message.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/api/src/app/agents/services/bridge-executor.service.ts` around lines 123 - 125, The failure log in the catch handler for this.fireWithRetries leaks the full bridgeUrl; update the catch block in bridge-executor.service.ts so logger.error does not include the raw bridgeUrl—compute a safe value (e.g. new URL(bridgeUrl).origin or a masked version that strips/query/credentials) and use that maskedOrigin when calling this.logger.error for the agentIdentifier/bridge failure message; keep MAX_RETRIES and the existing error object in the log but replace bridgeUrl with the maskedOrigin to avoid leaking sensitive data.
137-146:⚠️ Potential issue | 🟠 MajorAdd a per-attempt timeout to each outbound fetch retry.
Without a timeout, a hung socket can stall the retry loop indefinitely. Use
AbortController+ timer per attempt and clear it infinally.#!/bin/bash # Verify whether per-attempt timeout primitives are present around bridge fetch. rg -n -C3 "fetch\(|AbortController|signal:|REQUEST_TIMEOUT|setTimeout\(" apps/api/src/app/agents/services/bridge-executor.service.ts🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/api/src/app/agents/services/bridge-executor.service.ts` around lines 137 - 146, The retry loop around the outbound fetch (inside the for loop that uses MAX_RETRIES and calls fetch(url, { method: 'POST', headers: { 'Content-Type': 'application/json', 'x-novu-signature': signatureHeader }, body })) needs a per-attempt AbortController and timer: for each attempt create a new AbortController, pass controller.signal to fetch, start a timeout using REQUEST_TIMEOUT that calls controller.abort(), and ensure you clear the timeout (clearTimeout) in the finally block so the timer is cancelled if the request finishes or errors; reference the existing fetch call, MAX_RETRIES loop, signatureHeader, url and body to locate where to add the controller/signal and timer.
116-118:⚠️ Potential issue | 🟠 MajorSecret-key lookup still needs tenant scoping in the called use case.
This call passes
organizationId, but the providedGetDecryptedSecretKey.executesnippet still queries environment by_idonly. That leaves a cross-tenant lookup path for signing secrets.Based on learnings, all query methods must include
_environmentIdor_organizationIdin their filter to satisfy the enforcement type constraint.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/api/src/app/agents/services/bridge-executor.service.ts` around lines 116 - 118, The GetDecryptedSecretKey use case is performing an environment lookup by _id only, creating a cross-tenant secret lookup; update the GetDecryptedSecretKey.execute implementation (and any underlying repository method like secretKeyRepository.findOne / findById) to include tenant scoping from the command (use _environmentId or _organizationId in the query filter) and ensure the GetDecryptedSecretKeyCommand.create payload (as used in bridge-executor.service where getDecryptedSecretKey.execute is called) carries the environmentId and/or organizationId; in short, change the DB query in GetDecryptedSecretKey.execute to filter by {_id: <id>, _environmentId: command.environmentId} (or include _organizationId when required) so secrets are tenant-scoped.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@apps/api/src/app/agents/services/bridge-executor.service.ts`:
- Around line 111-123: The preparation steps before dispatch (resolveBridgeUrl,
getDecryptedSecretKey.execute, buildPayload, buildNovuSignatureHeader) can throw
and currently run outside the detached retry, so wrap the entire preparation and
dispatch setup in a local try/catch inside execute(); if any of
resolveBridgeUrl, the secret-key decryption, buildPayload, or
buildNovuSignatureHeader throws, catch the error, log or noop, and return early
(skip calling fireWithRetries) so failures are best-effort and do not reject the
caller flow; keep the existing detached .catch on fireWithRetries for
retries/errors during the actual HTTP dispatch.
- Around line 123-125: The failure log in the catch handler for
this.fireWithRetries leaks the full bridgeUrl; update the catch block in
bridge-executor.service.ts so logger.error does not include the raw
bridgeUrl—compute a safe value (e.g. new URL(bridgeUrl).origin or a masked
version that strips/query/credentials) and use that maskedOrigin when calling
this.logger.error for the agentIdentifier/bridge failure message; keep
MAX_RETRIES and the existing error object in the log but replace bridgeUrl with
the maskedOrigin to avoid leaking sensitive data.
- Around line 137-146: The retry loop around the outbound fetch (inside the for
loop that uses MAX_RETRIES and calls fetch(url, { method: 'POST', headers: {
'Content-Type': 'application/json', 'x-novu-signature': signatureHeader }, body
})) needs a per-attempt AbortController and timer: for each attempt create a new
AbortController, pass controller.signal to fetch, start a timeout using
REQUEST_TIMEOUT that calls controller.abort(), and ensure you clear the timeout
(clearTimeout) in the finally block so the timer is cancelled if the request
finishes or errors; reference the existing fetch call, MAX_RETRIES loop,
signatureHeader, url and body to locate where to add the controller/signal and
timer.
- Around line 116-118: The GetDecryptedSecretKey use case is performing an
environment lookup by _id only, creating a cross-tenant secret lookup; update
the GetDecryptedSecretKey.execute implementation (and any underlying repository
method like secretKeyRepository.findOne / findById) to include tenant scoping
from the command (use _environmentId or _organizationId in the query filter) and
ensure the GetDecryptedSecretKeyCommand.create payload (as used in
bridge-executor.service where getDecryptedSecretKey.execute is called) carries
the environmentId and/or organizationId; in short, change the DB query in
GetDecryptedSecretKey.execute to filter by {_id: <id>, _environmentId:
command.environmentId} (or include _organizationId when required) so secrets are
tenant-scoped.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 1373694e-fdcd-44c9-b16b-c6ea902fd764
📒 Files selected for processing (1)
apps/api/src/app/agents/services/bridge-executor.service.ts
- Guard: skip FF check for unauthenticated webhook routes, enforce for authenticated routes only — no more empty-string fallback to LaunchDarkly - Bridge: wrap entire execute() in try/catch so setup failures (URL resolution, secret key, signing) never break the inbound flow - SignalDto: add key/value/workflowId/to/payload fields for proper validation of metadata and trigger signal payloads at the DTO layer - toWebRequest: handle Buffer and string bodies directly before JSON.stringify fallback to preserve original bytes for HMAC signature verification - ChatSdkService: fix getOrCreate race condition with pendingCreations map — concurrent calls for the same instanceKey await the same promise instead of creating duplicate Chat instances that leak Redis connections Made-with: Cursor
Comment on lines
+3
to
+13
| import { | ||
| IsArray, | ||
| IsDefined, | ||
| IsIn, | ||
| IsNotEmpty, | ||
| IsObject, | ||
| IsOptional, | ||
| IsString, | ||
| MaxLength, | ||
| ValidateNested, | ||
| } from 'class-validator'; |
Made-with: Cursor
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
apps/api/src/app/agents/dtos/agent-reply-payload.dto.ts (1)
32-61:⚠️ Potential issue | 🟠 MajorEnforce signal-type-specific required fields in
SignalDto.On Lines 38-60,
metadataandtriggerpayload fields are all optional, so invalid signals can pass validation and fail later in the use case. Add conditional requirements (or split into discriminated DTOs) sometadatarequireskey/value, andtriggerrequires its contract fields.Proposed fix
import { IsArray, IsDefined, IsIn, IsNotEmpty, IsObject, IsOptional, IsString, MaxLength, + ValidateIf, ValidateNested, } from 'class-validator'; @@ class SignalDto { @@ `@ApiPropertyOptional`() - `@IsOptional`() + `@ValidateIf`((o: SignalDto) => o.type === 'metadata') + `@IsDefined`() `@IsString`() key?: string; `@ApiPropertyOptional`() - `@IsOptional`() + `@ValidateIf`((o: SignalDto) => o.type === 'metadata') + `@IsDefined`() value?: unknown; `@ApiPropertyOptional`() - `@IsOptional`() + `@ValidateIf`((o: SignalDto) => o.type === 'trigger') + `@IsDefined`() `@IsString`() workflowId?: string; }#!/bin/bash # Verify required signal fields expected by handler/usecase contract. rg -n -C3 --iglob '*handle-agent-reply*.ts' "metadata|trigger|key|value|workflowId|to|payload"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/api/src/app/agents/dtos/agent-reply-payload.dto.ts` around lines 32 - 61, SignalDto currently allows all signal fields optional so invalid "metadata" or "trigger" signals slip through; update validation so required fields are enforced per signal type: either split into discriminated DTOs (e.g., MetadataSignalDto, TriggerSignalDto) and use a union based on SIGNAL_TYPES, or add conditional validators on SignalDto (use `@ValidateIf` and `@IsDefined/`@IsString/@IsObject) to require key and value when type === 'metadata' and require the trigger contract fields when type === 'trigger'. Modify SignalDto (and any consumers of SIGNAL_TYPES) to validate workflowId/to/payload only when their signal type requires them and ensure the validator decorators reference the exact property names (key, value, workflowId, to, payload) so invalid signals are rejected at validation time.
🧹 Nitpick comments (4)
apps/api/src/app/agents/guards/agent-conversation-enabled.guard.ts (1)
13-14: Add a blank line before the earlyreturn.This branch misses the required blank line before
return.As per coding guidelines
**/*.{ts,tsx,js,jsx}: Include a blank line before everyreturnstatement.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/api/src/app/agents/guards/agent-conversation-enabled.guard.ts` around lines 13 - 14, In AgentConversationEnabledGuard's canActivate (in agent-conversation-enabled.guard.ts) the early-return branch "if (!user?.organizationId || !user?.environmentId) { return true; }" violates the rule requiring a blank line before return; insert a blank line immediately before the return statement so the block reads with a separating empty line before "return true;" to comply with the project's return-spacing guideline.apps/api/src/app/agents/services/bridge-executor.service.ts (1)
211-213: Consider adding timestamp to deliveryId for non-message events.For events without a message (e.g.,
ON_START,ON_RESOLVE), thedeliveryIdis${conversation._id}:${event}. If the same event type fires multiple times for the same conversation (e.g., due to a race condition or retry at a higher layer), the downstream system cannot distinguish between distinct invocations.If this is intentional for deduplication, this is fine. Otherwise, consider appending a timestamp or nonce:
const deliveryId = message?.id ? `${conversation._id}:${message.id}` - : `${conversation._id}:${event}`; + : `${conversation._id}:${event}:${Date.now()}`;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/api/src/app/agents/services/bridge-executor.service.ts` around lines 211 - 213, The current deliveryId computation (variable deliveryId using message?.id) uses `${conversation._id}:${event}` for non-message events which can collide if the same event fires multiple times; update the branch that handles when message?.id is falsy (the `${conversation._id}:${event}` case) to append a high-resolution timestamp or a short nonce (e.g., Date.now() or crypto-random value) so deliveryId becomes `${conversation._id}:${event}:${timestampOrNonce}` to uniquely identify distinct invocations while keeping the message-id path unchanged.apps/api/src/app/agents/services/chat-sdk.service.ts (2)
178-221: Non-null assertions on credentials could produce unclear runtime errors.Multiple fields are accessed with
!assertions (e.g.,credentials.apiKey!,connectionAccessToken!). IfAgentCredentialService.resolve()returns incomplete data, the error won't clearly indicate which credential field is missing.Consider validating required fields and throwing a descriptive error, or ensure
AgentCredentialService.resolve()guarantees these fields exist.💡 Example validation for Slack adapter
case AgentPlatformEnum.SLACK: { + if (!connectionAccessToken || !credentials.apiKey) { + throw new BadRequestException( + `Slack adapter requires botToken and signingSecret for agent ${config.agentId}` + ); + } const { createSlackAdapter } = await esmImport('@chat-adapter/slack'); return { slack: createSlackAdapter({ - botToken: connectionAccessToken!, - signingSecret: credentials.apiKey!, + botToken: connectionAccessToken, + signingSecret: credentials.apiKey, }), }; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/api/src/app/agents/services/chat-sdk.service.ts` around lines 178 - 221, The buildAdapters method uses non-null assertions on credentials and connectionAccessToken which can produce unclear runtime errors; update buildAdapters (and callers like AgentCredentialService.resolve) to validate required fields per platform before calling createSlackAdapter/createTeamsAdapter/createWhatsAppAdapter: check presence of connectionAccessToken for SLACK and required credential keys for each AgentPlatformEnum branch, and throw a descriptive BadRequestException (e.g., "Missing credential: apiKey for SLACK") listing the exact missing field(s) so errors point to the precise credential instead of failing with a cryptic runtime null/undefined error.
73-83: Consider collecting keys before deletion for clearer intent and defensive coding.While lru-cache v11.2.4 safely handles deletion during iteration over
keys()(defenses were added in v7.5), the current pattern is unnecessarily defensive. Collecting the keys first makes the intent explicit and aligns with the pattern already used elsewhere in the file (e.g., line 86 with[...this.instances.entries()]).♻️ Suggested refactor
evict(agentId: string, integrationIdentifier?: string) { if (integrationIdentifier) { this.instances.delete(`${agentId}:${integrationIdentifier}`); } else { const keysToDelete = [...this.instances.keys()].filter((key) => key.startsWith(`${agentId}:`) ); for (const key of keysToDelete) { this.instances.delete(key); } } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/api/src/app/agents/services/chat-sdk.service.ts` around lines 73 - 83, The evict method currently deletes entries while iterating this.instances.keys(); change it to first collect the matching keys into an array (e.g., const keys = [...this.instances.keys()].filter(k => k.startsWith(`${agentId}:`))) and then iterate that array to delete each key, so the method (evict) explicitly expresses intent and matches the existing pattern used elsewhere (e.g., [...this.instances.entries()]) for safer, clearer deletion.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/api/src/app/agents/services/chat-sdk.service.ts`:
- Around line 98-113: In postToConversation, the platform passed to getOrCreate
(config.platform) can differ from the method parameter platform used for
chat.getAdapter, causing adapter lookup failures; fix by making the platform
source consistent: either remove the platform parameter and always use
config.platform (update postToConversation signature and callers) or validate
early that platform === config.platform and throw/log if mismatched; locate this
in postToConversation, referencing agentCredentialService.resolve, instanceKey,
getOrCreate, chat.getAdapter, and ThreadImpl.fromJSON to apply the chosen fix.
---
Duplicate comments:
In `@apps/api/src/app/agents/dtos/agent-reply-payload.dto.ts`:
- Around line 32-61: SignalDto currently allows all signal fields optional so
invalid "metadata" or "trigger" signals slip through; update validation so
required fields are enforced per signal type: either split into discriminated
DTOs (e.g., MetadataSignalDto, TriggerSignalDto) and use a union based on
SIGNAL_TYPES, or add conditional validators on SignalDto (use `@ValidateIf` and
`@IsDefined/`@IsString/@IsObject) to require key and value when type ===
'metadata' and require the trigger contract fields when type === 'trigger'.
Modify SignalDto (and any consumers of SIGNAL_TYPES) to validate
workflowId/to/payload only when their signal type requires them and ensure the
validator decorators reference the exact property names (key, value, workflowId,
to, payload) so invalid signals are rejected at validation time.
---
Nitpick comments:
In `@apps/api/src/app/agents/guards/agent-conversation-enabled.guard.ts`:
- Around line 13-14: In AgentConversationEnabledGuard's canActivate (in
agent-conversation-enabled.guard.ts) the early-return branch "if
(!user?.organizationId || !user?.environmentId) { return true; }" violates the
rule requiring a blank line before return; insert a blank line immediately
before the return statement so the block reads with a separating empty line
before "return true;" to comply with the project's return-spacing guideline.
In `@apps/api/src/app/agents/services/bridge-executor.service.ts`:
- Around line 211-213: The current deliveryId computation (variable deliveryId
using message?.id) uses `${conversation._id}:${event}` for non-message events
which can collide if the same event fires multiple times; update the branch that
handles when message?.id is falsy (the `${conversation._id}:${event}` case) to
append a high-resolution timestamp or a short nonce (e.g., Date.now() or
crypto-random value) so deliveryId becomes
`${conversation._id}:${event}:${timestampOrNonce}` to uniquely identify distinct
invocations while keeping the message-id path unchanged.
In `@apps/api/src/app/agents/services/chat-sdk.service.ts`:
- Around line 178-221: The buildAdapters method uses non-null assertions on
credentials and connectionAccessToken which can produce unclear runtime errors;
update buildAdapters (and callers like AgentCredentialService.resolve) to
validate required fields per platform before calling
createSlackAdapter/createTeamsAdapter/createWhatsAppAdapter: check presence of
connectionAccessToken for SLACK and required credential keys for each
AgentPlatformEnum branch, and throw a descriptive BadRequestException (e.g.,
"Missing credential: apiKey for SLACK") listing the exact missing field(s) so
errors point to the precise credential instead of failing with a cryptic runtime
null/undefined error.
- Around line 73-83: The evict method currently deletes entries while iterating
this.instances.keys(); change it to first collect the matching keys into an
array (e.g., const keys = [...this.instances.keys()].filter(k =>
k.startsWith(`${agentId}:`))) and then iterate that array to delete each key, so
the method (evict) explicitly expresses intent and matches the existing pattern
used elsewhere (e.g., [...this.instances.entries()]) for safer, clearer
deletion.
🪄 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: 514bf7cf-4488-43eb-b5d5-e991aca82390
📒 Files selected for processing (5)
apps/api/src/app/agents/dtos/agent-reply-payload.dto.tsapps/api/src/app/agents/guards/agent-conversation-enabled.guard.tsapps/api/src/app/agents/services/bridge-executor.service.tsapps/api/src/app/agents/services/chat-sdk.service.tsapps/api/src/app/agents/utils/express-to-web-request.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/api/src/app/agents/utils/express-to-web-request.ts
Comment on lines
+98
to
+113
| async postToConversation( | ||
| agentId: string, | ||
| integrationIdentifier: string, | ||
| platform: string, | ||
| serializedThread: Record<string, unknown>, | ||
| message: string | ||
| ): Promise<void> { | ||
| const config = await this.agentCredentialService.resolve(agentId, integrationIdentifier); | ||
| const instanceKey = `${agentId}:${integrationIdentifier}`; | ||
| const chat = await this.getOrCreate(instanceKey, agentId, config.platform, config); | ||
|
|
||
| const { ThreadImpl } = await esmImport('chat'); | ||
| const adapter = chat.getAdapter(platform); | ||
| const thread = ThreadImpl.fromJSON(serializedThread, adapter); | ||
| await thread.post(message); | ||
| } |
Contributor
There was a problem hiding this comment.
Potential bug: platform parameter may not match config.platform.
The method uses config.platform for getOrCreate (line 107) but uses the platform parameter for getAdapter (line 110). If these differ, the adapter lookup will fail or behave unexpectedly.
Consider either:
- Removing the
platformparameter and usingconfig.platformconsistently, or - Validating that
platform === config.platform
🐛 Proposed fix (option 1: use config.platform consistently)
async postToConversation(
agentId: string,
integrationIdentifier: string,
- platform: string,
serializedThread: Record<string, unknown>,
message: string
): Promise<void> {
const config = await this.agentCredentialService.resolve(agentId, integrationIdentifier);
const instanceKey = `${agentId}:${integrationIdentifier}`;
const chat = await this.getOrCreate(instanceKey, agentId, config.platform, config);
const { ThreadImpl } = await esmImport('chat');
- const adapter = chat.getAdapter(platform);
+ const adapter = chat.getAdapter(config.platform);
const thread = ThreadImpl.fromJSON(serializedThread, adapter);
await thread.post(message);
}📝 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
| async postToConversation( | |
| agentId: string, | |
| integrationIdentifier: string, | |
| platform: string, | |
| serializedThread: Record<string, unknown>, | |
| message: string | |
| ): Promise<void> { | |
| const config = await this.agentCredentialService.resolve(agentId, integrationIdentifier); | |
| const instanceKey = `${agentId}:${integrationIdentifier}`; | |
| const chat = await this.getOrCreate(instanceKey, agentId, config.platform, config); | |
| const { ThreadImpl } = await esmImport('chat'); | |
| const adapter = chat.getAdapter(platform); | |
| const thread = ThreadImpl.fromJSON(serializedThread, adapter); | |
| await thread.post(message); | |
| } | |
| async postToConversation( | |
| agentId: string, | |
| integrationIdentifier: string, | |
| serializedThread: Record<string, unknown>, | |
| message: string | |
| ): Promise<void> { | |
| const config = await this.agentCredentialService.resolve(agentId, integrationIdentifier); | |
| const instanceKey = `${agentId}:${integrationIdentifier}`; | |
| const chat = await this.getOrCreate(instanceKey, agentId, config.platform, config); | |
| const { ThreadImpl } = await esmImport('chat'); | |
| const adapter = chat.getAdapter(config.platform); | |
| const thread = ThreadImpl.fromJSON(serializedThread, adapter); | |
| await thread.post(message); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/api/src/app/agents/services/chat-sdk.service.ts` around lines 98 - 113,
In postToConversation, the platform passed to getOrCreate (config.platform) can
differ from the method parameter platform used for chat.getAdapter, causing
adapter lookup failures; fix by making the platform source consistent: either
remove the platform parameter and always use config.platform (update
postToConversation signature and callers) or validate early that platform ===
config.platform and throw/log if mismatched; locate this in postToConversation,
referencing agentCredentialService.resolve, instanceKey, getOrCreate,
chat.getAdapter, and ThreadImpl.fromJSON to apply the chosen fix.
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
apps/api/src/app/agents/dtos/agent-reply-payload.dto.ts (1)
32-46:⚠️ Potential issue | 🟠 Major
SignalDtostill accepts malformed metadata signals.For
type: 'metadata', Line 41 (key) and Line 45 (value) can be omitted, so invalid payloads pass DTO validation.Proposed fix
import { IsArray, IsDefined, IsIn, IsNotEmpty, IsObject, IsOptional, IsString, MaxLength, + ValidateIf, ValidateNested, } from 'class-validator'; export class SignalDto { `@ApiProperty`({ enum: SIGNAL_TYPES }) `@IsString`() `@IsIn`(SIGNAL_TYPES) type: (typeof SIGNAL_TYPES)[number]; `@ApiPropertyOptional`() - `@IsOptional`() + `@ValidateIf`((o: SignalDto) => o.type === 'metadata') + `@IsDefined`() `@IsString`() + `@IsNotEmpty`() key?: string; `@ApiPropertyOptional`() - `@IsOptional`() + `@ValidateIf`((o: SignalDto) => o.type === 'metadata') + `@IsDefined`() value?: unknown;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/api/src/app/agents/dtos/agent-reply-payload.dto.ts` around lines 32 - 46, SignalDto currently allows missing key/value for type === 'metadata'; update the key and value decorators to require them only when type is 'metadata' by replacing `@IsOptional`() with conditional validators: add `@ValidateIf`(o => o.type === 'metadata') above both key and value, add `@IsDefined`() (and `@IsString`() on key) so they are required when type is 'metadata', and keep ApiPropertyOptional for docs if needed; reference the SignalDto class and the key/value property decorators to locate the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@apps/api/src/app/agents/dtos/agent-reply-payload.dto.ts`:
- Around line 32-46: SignalDto currently allows missing key/value for type ===
'metadata'; update the key and value decorators to require them only when type
is 'metadata' by replacing `@IsOptional`() with conditional validators: add
`@ValidateIf`(o => o.type === 'metadata') above both key and value, add
`@IsDefined`() (and `@IsString`() on key) so they are required when type is
'metadata', and keep ApiPropertyOptional for docs if needed; reference the
SignalDto class and the key/value property decorators to locate the change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 83a8830b-fa15-4902-8957-65122bec0156
📒 Files selected for processing (1)
apps/api/src/app/agents/dtos/agent-reply-payload.dto.ts
This was referenced Apr 14, 2026
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
1 participant
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.
Summary
End-to-end Conversational Agents feature: webhook ingestion, credential resolution, conversation persistence, subscriber mapping, bridge event dispatch, and agent reply delivery — plus the DAL entities and dashboard UI to manage it all.
What changed
agent:integrationpair. Supports Slack, Teams, and WhatsApp adapters with Redis-backed state. Handles graceful shutdown and eviction.POSTto the customer's bridge URL with the full conversation context (message, history, subscriber, platform context). Includes exponential-backoff retries (up to 3 attempts).onResolvebridge callback), and trigger signals.POST /agents/:agentId/webhook/:integrationIdentifier— derives platform from integration'sproviderId.POST /agents/:agentId/reply— API-key accessible, validates and dispatches reply commands.IS_CONVERSATIONAL_AGENTS_ENABLED(LaunchDarkly) viaAgentConversationEnabledGuard. Routes return 404 when disabled and are hidden from Swagger.Architecture
Inbound Flow (Platform → Novu → Bridge)
sequenceDiagram participant Platform as Slack / Teams / WhatsApp participant Webhook as Webhook Controller participant ChatSDK as ChatSdkService (LRU) participant Creds as AgentCredentialService participant Inbound as AgentInboundHandler participant SubRes as SubscriberResolver participant ConvSvc as ConversationService participant Bridge as BridgeExecutorService participant Customer as Customer Bridge URL Platform->>Webhook: POST /agents/:id/webhook/:integration Webhook->>Creds: resolve(agentId, integrationId) Creds-->>Webhook: ResolvedPlatformConfig Webhook->>ChatSDK: handleWebhook(config, req, res) ChatSDK->>ChatSDK: getOrCreate(LRU cache) Note over ChatSDK: Chat SDK fires event ChatSDK->>Inbound: onNewMention / onSubscribedMessage Inbound->>SubRes: resolve(platformUserId) SubRes-->>Inbound: subscriberId | null Inbound->>ConvSvc: createOrGetConversation() Inbound->>ConvSvc: persistInboundMessage() Inbound->>ConvSvc: updateChannelThread(serialized) Inbound->>Bridge: execute(event, context) Bridge->>Bridge: resolveBridgeUrl(env) Bridge->>Bridge: sign payload (HMAC) Bridge->>Customer: POST bridge?action=agent-event Customer-->>Bridge: 200 OKOutbound Flow (Bridge → Novu → Platform)
sequenceDiagram participant Customer as Customer Bridge participant Reply as Reply Controller participant Handler as HandleAgentReply participant ConvRepo as ConversationRepository participant ChatSDK as ChatSdkService participant Platform as Slack / Teams / WhatsApp participant Activity as ActivityRepository participant Bridge as BridgeExecutorService Customer->>Reply: POST /agents/:id/reply (API Key auth) Reply->>Handler: execute(command) Handler->>ConvRepo: findOne(conversationId) ConvRepo-->>Handler: conversation + serializedThread alt reply or update Handler->>ChatSDK: postToConversation(serializedThread, text) ChatSDK->>Platform: deliver message Handler->>Activity: persist activity Handler->>ConvRepo: touchActivity() end alt metadata signals Handler->>ConvRepo: updateMetadata(merged) Handler->>Activity: persist signal activity end alt resolve signal Handler->>ConvRepo: updateStatus(RESOLVED) Handler->>Activity: persist resolve activity Handler->>Bridge: execute(onResolve callback) Bridge->>Customer: POST bridge?event=onResolve endCredential Resolution Chain
flowchart LR A[agentId + integrationIdentifier] --> B[AgentRepository] B --> C{Feature Flag<br/>enabled?} C -- No --> D[404] C -- Yes --> E[IntegrationRepository] E --> F[AgentIntegrationRepository] F --> G[decryptCredentials] G --> H[ChannelConnectionRepository] H --> I[ResolvedPlatformConfig] style C fill:#fbbf24,stroke:#92400e style D fill:#ef4444,stroke:#991b1b,color:#fff style I fill:#22c55e,stroke:#166534,color:#fffChat SDK Instance Lifecycle
flowchart TD A[Webhook arrives] --> B{LRU cache<br/>has instance?} B -- Yes --> C[Return cached Chat] B -- No --> D[Create Chat + adapters] D --> E[Register event handlers<br/>onNewMention / onSubscribedMessage] E --> F[Store in LRU cache] F --> C C --> G[Process webhook] H[TTL expired / evicted] --> I[chat.shutdown] J[Integration updated] --> K[chatSdkService.evict] K --> I style B fill:#fbbf24,stroke:#92400e style I fill:#ef4444,stroke:#991b1b,color:#fffAgent Events
onStartonMessageonActiononResolveEnterprise
Depends on novuhq/packages-enterprise#442 — same feature flag guard applied to the EE
ConversationsController, plus_organizationIdfixes on all conversation use case queries.Test plan
/agents/*and/conversations/*routes return 404 whenIS_CONVERSATIONAL_AGENTS_ENABLEDis not setonResolvebridge callbackNote
High Risk
Adds new unauthenticated webhook ingestion and outbound message delivery paths, plus new Mongo DAL schemas for conversations/activities; failures could impact data integrity and introduce security/availability risks if misconfigured.
Overview
Implements an end-to-end Conversational Agents pipeline in the API, including a new inbound webhook endpoint (
POST /agents/:agentId/webhook/:integrationIdentifier) that routes Slack/Teams/WhatsApp events through a Chat SDK integration and dispatches signed bridge callbacks.Adds a new authenticated reply endpoint (
POST /agents/:agentId/reply) that lets the external bridge send agentreply/updatemessages, metadata signals, and resolve actions, persisting them as conversation activity and posting back to the originating platform.Introduces new DAL models/repositories for
ConversationandConversationActivity(plus helper repository methods likeAgentRepository.findByIdForWebhookandChannelEndpointRepository.findByPlatformIdentity) and updates bootstrap body parsing to preserve raw request bodies for signature verification on agent webhook routes.Wires enterprise
ConversationsModulewhen present, gates agent routes behindIS_CONVERSATIONAL_AGENTS_ENABLED(404 when disabled, hidden from Swagger), and adds requiredchat/adapter + Redis state dependencies.Reviewed by Cursor Bugbot for commit 4495c3d. Configure here.
What changed
Implemented end-to-end Conversational Agents: webhook ingestion, credential resolution, subscriber mapping, conversation/activity persistence, bridge dispatch with HMAC-signed deliveries and retries, and outbound reply delivery. Added a multi-platform Chat SDK (Slack, Teams, WhatsApp) with Redis-backed state and LRU-cached per-agent instances, plus a dashboard UI and enterprise wiring. The change enables tenant-scoped agent event routing to external bridge endpoints and reliable agent replies to platform threads while preserving raw request bodies for signature verification and enforcing feature-flag gating.
Affected areas
Key technical decisions
Testing
E2E tests updated to enable the conversational agents flag; test plan and e2e coverage include webhook ingestion, reply flow, cache/eviction behavior, bridge retry/backoff logic, metadata limits, and enterprise gating. Manual review is recommended for high-risk areas (unauthenticated webhooks, new Mongo schemas, and data integrity).