Conversation
…es NV-7410 Wrap all three provider delivery boundaries (thread.post, adapter.editMessage, handler.send) in chat-sdk.service.ts with .catch(toDeliveryError), converting any unhandled provider error into a BadGatewayException (502) that preserves the original error message instead of swallowing it as a generic 500. Add AgentDeliveryError to @novu/framework so developers can instanceof-check delivery failures in their agent handlers and react to them explicitly. Old framework versions automatically get better error messages from the 502 body without any code changes — the new typed error is additive. Made-with: Cursor
✅ Deploy Preview for dashboard-v2-novu-staging canceled.
|
Contributor
📝 WalkthroughWalkthroughAdds a new AgentDeliveryError type, special-cases HTTP 502 agent responses to surface provider messages, and centralizes delivery-failure mapping so adapter/handler promise rejections are converted to delivery errors (BadGatewayException / AgentDeliveryError). Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client
participant Service as ChatSdkService / Usecase
participant Adapter as Adapter/Handler
participant Framework as Framework (AgentDeliveryError)
Client->>Service: request send/post/edit
Service->>Adapter: call adapter/handler (returns Promise)
Adapter--x Service: reject/error
Service->>Service: .catch(toDeliveryError)
Service->>Framework: map/throw standardized delivery error (BadGatewayException / AgentDeliveryError)
Service--x Client: propagate delivery error
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
packages/framework/src/resources/agent/agent.context.ts (1)
221-230: Optional: passresponse.statusinstead of the literal502.Functionally equivalent today since the branch is gated on
response.status === 502, but it removes a magic number, decouples the error from the gate, and makes it trivial to extend the branch later (e.g., 504).♻️ Suggested tweak
- if (response.status === 502) { + if (response.status === 502) { let message = text; try { const parsed = JSON.parse(text) as { message?: string }; if (parsed.message) message = parsed.message; } catch { // use raw text if JSON parsing fails } - throw new AgentDeliveryError(502, message); + throw new AgentDeliveryError(response.status, message); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/framework/src/resources/agent/agent.context.ts` around lines 221 - 230, In the response handling branch that checks response.status === 502 inside agent.context.ts, replace the hard-coded literal 502 in the thrown AgentDeliveryError with the actual response.status (i.e., throw new AgentDeliveryError(response.status, message)) so the thrown error reflects the real HTTP status and removes the magic number; update the throw site that currently reads AgentDeliveryError(502, message) to use response.status instead.apps/api/src/app/agents/services/chat-sdk.service.ts (1)
196-228: Reaction adapters intentionally out of scope?
reactToMessageandremoveReactionalso reach upstream providers (Slack/Teams/WhatsApp) and can return 401/403/etc., but they aren't wrapped with.catch(toDeliveryError), so failures there will continue to surface as opaque 500s with anerrorId— the exact problem this PR sets out to solve. The PR description explicitly scopes the fix tothread.post(),adapter.editMessage(), andhandler.send(), so this may be deliberate; flagging in case it isn't.♻️ If you want symmetry
- await adapter.removeReaction(platformThreadId, platformMessageId, resolved); + await adapter.removeReaction(platformThreadId, platformMessageId, resolved).catch(toDeliveryError);- await adapter.addReaction(platformThreadId, platformMessageId, resolved); + await adapter.addReaction(platformThreadId, platformMessageId, resolved).catch(toDeliveryError);🤖 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 196 - 228, reactToMessage and removeReaction call adapter.addReaction/adapter.removeReaction (after resolveEmoji) but aren’t wrapping failures with toDeliveryError, so upstream auth/HTTP errors will surface as opaque 500s; update both methods (reactToMessage and removeReaction) to catch adapter errors and convert them via toDeliveryError (e.g. await adapter.addReaction(...).catch(toDeliveryError) or try/catch and throw toDeliveryError(error)) after resolveEmoji so any 401/403/etc. are normalized the same way as thread.post/editMessage/handler.send.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/framework/src/resources/agent/agent.errors.ts`:
- Around line 24-32: The package version for `@novu/framework` must be bumped to
publish the new public export AgentDeliveryError; open the framework
package.json and update the version field (e.g., from 2.10.1-alpha.2 to
2.11.0-alpha.0 or your project's equivalent minor-alpha scheme), run any
required lockfile install/update, and ensure the new version is committed so the
AgentDeliveryError export is included in the published package.
---
Nitpick comments:
In `@apps/api/src/app/agents/services/chat-sdk.service.ts`:
- Around line 196-228: reactToMessage and removeReaction call
adapter.addReaction/adapter.removeReaction (after resolveEmoji) but aren’t
wrapping failures with toDeliveryError, so upstream auth/HTTP errors will
surface as opaque 500s; update both methods (reactToMessage and removeReaction)
to catch adapter errors and convert them via toDeliveryError (e.g. await
adapter.addReaction(...).catch(toDeliveryError) or try/catch and throw
toDeliveryError(error)) after resolveEmoji so any 401/403/etc. are normalized
the same way as thread.post/editMessage/handler.send.
In `@packages/framework/src/resources/agent/agent.context.ts`:
- Around line 221-230: In the response handling branch that checks
response.status === 502 inside agent.context.ts, replace the hard-coded literal
502 in the thrown AgentDeliveryError with the actual response.status (i.e.,
throw new AgentDeliveryError(response.status, message)) so the thrown error
reflects the real HTTP status and removes the magic number; update the throw
site that currently reads AgentDeliveryError(502, message) to use
response.status instead.
🪄 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: 70f52a39-1c52-4f99-bf12-38bf9e0df0b6
📒 Files selected for processing (5)
apps/api/src/app/agents/services/chat-sdk.service.tspackages/framework/src/index.tspackages/framework/src/resources/agent/agent.context.tspackages/framework/src/resources/agent/agent.errors.tspackages/framework/src/resources/agent/index.ts
Comment on lines
+24
to
+32
| export class AgentDeliveryError extends Error { | ||
| readonly statusCode: number; | ||
|
|
||
| constructor(statusCode: number, message: string) { | ||
| super(message); | ||
| this.name = 'AgentDeliveryError'; | ||
| this.statusCode = statusCode; | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# 1) Check the framework's compile target(s) — ES2015+ means instanceof works natively.
fd -t f -i 'tsconfig*.json' packages/framework -x sh -c 'echo "=== {} ==="; cat {}'
# 2) Confirm version of `@novu/framework` (PR should bump it because of the new public export).
fd -t f '^package.json$' packages/framework --max-depth 2 -x sh -c 'echo "=== {} ==="; jq "{name,version}" {}'
# 3) Confirm AgentDeliveryError is not silently re-introducing a name clash anywhere else.
rg -nP '\bclass\s+AgentDeliveryError\b'Repository: novuhq/novu
Length of output: 1977
🏁 Script executed:
#!/bin/bash
# 1) Check if AgentDeliveryError is exported from the main `@novu/framework` entry point
echo "=== Checking main entry points for `@novu/framework` ==="
fd -t f '^index\.(ts|js)$' packages/framework/src --max-depth 1 -x sh -c 'echo "=== {} ==="; head -50 {}'
# 2) Search for AgentDeliveryError exports across the codebase
echo ""
echo "=== Searching for AgentDeliveryError exports ==="
rg -n 'export.*AgentDeliveryError|from.*agent\.errors'
# 3) Check if the version in package.json was changed in recent commits
echo ""
echo "=== Checking git history for version changes ==="
git log --oneline -20 -- packages/framework/package.json
# 4) Get the previous version to confirm if a bump is needed
echo ""
echo "=== Previous package versions ==="
git show HEAD~1:packages/framework/package.json 2>/dev/null | jq '.version' || echo "Could not retrieve previous version"Repository: novuhq/novu
Length of output: 1602
🏁 Script executed:
#!/bin/bash
# Check current git status and compare versions
echo "=== Current package.json version ==="
cat packages/framework/package.json | jq '.version'
echo ""
echo "=== Check if there are uncommitted changes to package.json ==="
git status --porcelain packages/framework/package.json
echo ""
echo "=== List recent commits to see if version was already bumped ==="
git log --oneline -30 | head -15
echo ""
echo "=== Check the actual diff if package.json was modified ==="
git diff HEAD -- packages/framework/package.json | head -20Repository: novuhq/novu
Length of output: 388
🏁 Script executed:
#!/bin/bash
# Double-check if AgentDeliveryError was actually new in this commit
echo "=== Search for AgentDeliveryError in git history ==="
git log --all --oneline --grep="AgentDeliveryError" | head -10
echo ""
echo "=== Check if agent.errors.ts exists in parent commit ==="
git show HEAD~1:packages/framework/src/resources/agent/agent.errors.ts 2>/dev/null | head -30 || echo "File did not exist in parent commit"
echo ""
echo "=== Verify this is truly a new export by checking the agent resource index ==="
git show HEAD~1:packages/framework/src/resources/agent/index.ts 2>/dev/null | grep -i "AgentDeliveryError" || echo "AgentDeliveryError not exported in parent commit"Repository: novuhq/novu
Length of output: 384
Bump @novu/framework version for new public export AgentDeliveryError.
AgentDeliveryError is a new public export from @novu/framework (added in this commit, not present in parent), so the package version must be incremented at minimum as a minor bump per semver. Current version remains 2.10.1-alpha.2 with no changes to package.json.
Update packages/framework/package.json to reflect the new export (e.g., 2.11.0-alpha.0 or equivalent per your versioning scheme).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/framework/src/resources/agent/agent.errors.ts` around lines 24 - 32,
The package version for `@novu/framework` must be bumped to publish the new public
export AgentDeliveryError; open the framework package.json and update the
version field (e.g., from 2.10.1-alpha.2 to 2.11.0-alpha.0 or your project's
equivalent minor-alpha scheme), run any required lockfile install/update, and
ensure the new version is committed so the AgentDeliveryError export is included
in the published package.
…dpoint fixes NV-7410 Same unguarded handler.send() pattern as the delivery path. Wrap it with .catch() so provider errors (invalid API key, 401, etc.) are returned as BadGatewayException (502) with the original message, making the dashboard toast actionable instead of showing a generic internal server error. Made-with: Cursor
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
apps/api/src/app/agents/usecases/send-agent-test-email/send-agent-test-email.usecase.ts (1)
88-93: Consider logging the provider error before re-throwing.The caught provider error is mapped to a
BadGatewayExceptionand propagated to the client, but it is never logged on the server. For a "send test email" path where developers debug provider misconfigurations, retaining the original error (with stack/cause) in server logs is valuable for support and observability — especially when the provider returns a non-Errorvalue and the message gets stringified.♻️ Optional: log + idiomatic try/catch
- await handler.send(mailOptions).catch((err) => { - throw new BadGatewayException({ - error: 'delivery_failed', - message: err instanceof Error ? err.message : String(err), - }); - }); + try { + await handler.send(mailOptions); + } catch (err) { + Logger.error( + { err, agentIdentifier: command.agentIdentifier, providerId: senderIntegration.providerId }, + 'Failed to deliver agent test email', + SendAgentTestEmail.name + ); + throw new BadGatewayException({ + error: 'delivery_failed', + message: err instanceof Error ? err.message : String(err), + }); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/api/src/app/agents/usecases/send-agent-test-email/send-agent-test-email.usecase.ts` around lines 88 - 93, Wrap the call to handler.send(mailOptions) in a try/catch inside send-agent-test-email.usecase (the block that currently .catchs and throws BadGatewayException), log the original provider error with the application logger (including err.stack or the full error object/cause when err is not an Error) before re-throwing the BadGatewayException; prefer an idiomatic try/catch around handler.send so you can logger.error('email provider error', { err, mailOptions }) and then throw new BadGatewayException({ error: 'delivery_failed', message: err instanceof Error ? err.message : String(err) }).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In
`@apps/api/src/app/agents/usecases/send-agent-test-email/send-agent-test-email.usecase.ts`:
- Around line 88-93: Wrap the call to handler.send(mailOptions) in a try/catch
inside send-agent-test-email.usecase (the block that currently .catchs and
throws BadGatewayException), log the original provider error with the
application logger (including err.stack or the full error object/cause when err
is not an Error) before re-throwing the BadGatewayException; prefer an idiomatic
try/catch around handler.send so you can logger.error('email provider error', {
err, mailOptions }) and then throw new BadGatewayException({ error:
'delivery_failed', message: err instanceof Error ? err.message : String(err) }).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 9d631cb2-79cf-4c01-81e8-c08cdaf941c0
📒 Files selected for processing (1)
apps/api/src/app/agents/usecases/send-agent-test-email/send-agent-test-email.usecase.ts
… delivery Extract nested error detail from common REST API error shapes (errors[0].message or body.message) so the surfaced message is actionable rather than just the HTTP status text e.g. "Unauthorized: The provided authorization grant is invalid" instead of just "Unauthorized". Made-with: Cursor
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
apps/api/src/app/agents/usecases/send-agent-test-email/send-agent-test-email.usecase.ts (1)
88-96: Optional: log the original error before rethrowing to preserve diagnostics.The catch discards the original stack/cause, so once it becomes a
BadGatewayExceptionSREs lose visibility into the underlying provider failure (status code, request id, etc.). Consider a structuredLogger.warn/debug(or passing the original ascause) before throwing — purely additive, no behavior change on the response.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/api/src/app/agents/usecases/send-agent-test-email/send-agent-test-email.usecase.ts` around lines 88 - 96, In the send-agent-test-email use case's handler.send(...) catch block, preserve diagnostics by logging the original error (including stack and provider response) before rethrowing and/or attach it as the cause of the BadGatewayException; specifically, call your logger (e.g., Logger.warn or Logger.debug) with the err, response body, and mailOptions, or include the original err as the cause when constructing BadGatewayException in the catch that currently wraps handler.send and throws the object with error: 'delivery_failed'.
🤖 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/usecases/send-agent-test-email/send-agent-test-email.usecase.ts`:
- Around line 88-96: The inline delivery-error mapping in
send-agent-test-email.usecase.ts duplicates logic from the file-local helper;
export the existing toDeliveryError function from chat-sdk.service.ts (where it
is defined) and import it into this usecase (import { toDeliveryError } from
'../../services/chat-sdk.service';), then replace the current .catch((err) => {
... }) on handler.send(mailOptions) with .catch(toDeliveryError) so the
centralized toDeliveryError function handles error normalization and throws the
BadGatewayException consistently.
---
Nitpick comments:
In
`@apps/api/src/app/agents/usecases/send-agent-test-email/send-agent-test-email.usecase.ts`:
- Around line 88-96: In the send-agent-test-email use case's handler.send(...)
catch block, preserve diagnostics by logging the original error (including stack
and provider response) before rethrowing and/or attach it as the cause of the
BadGatewayException; specifically, call your logger (e.g., Logger.warn or
Logger.debug) with the err, response body, and mailOptions, or include the
original err as the cause when constructing BadGatewayException in the catch
that currently wraps handler.send and throws the object with error:
'delivery_failed'.
🪄 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: 09f96bd6-c09e-4467-aff0-1f4c3936b86a
📒 Files selected for processing (2)
apps/api/src/app/agents/services/chat-sdk.service.tsapps/api/src/app/agents/usecases/send-agent-test-email/send-agent-test-email.usecase.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/api/src/app/agents/services/chat-sdk.service.ts
AgentDeliveryError is a new public export — version bump required. Made-with: Cursor
This reverts commit e2e5ede.
This was referenced Apr 27, 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.
Problem
When an outbound provider (SendGrid, Slack, Teams, WhatsApp, etc.) rejects a message delivery — e.g. 401 Unauthorized, 403 Forbidden — the error was not caught at the delivery boundary. NestJS's default exception filter swallowed it as a generic 500 with an opaque
errorId, making it completely unactionable for developers:Solution
API (
apps/api)Wrap all three provider delivery boundaries in
chat-sdk.service.tswith.catch(toDeliveryError):thread.post()inpostToConversation— Slack, Teams, WhatsApp, Email chat deliveryadapter.editMessage()ineditInConversation— same platforms, edit pathhandler.send()inbuildSendEmailCallback— direct email provider sendsAny error from these calls is now returned as a
BadGatewayException(502) with the original provider error message preserved:{ "error": "delivery_failed", "message": "Email provider returned: 403 Forbidden — sender identity not verified" }Pre-existing 4xx errors from our own validation code are unaffected.
Framework (
packages/framework)Add
AgentDeliveryError— a simple typed error class exported from@novu/framework._post()inagent.context.tsnow throws it on 502 responses instead of a genericError, so developers can branch on it:Backwards compatibility
Merging the API change alone already improves error messages for all framework versions — old SDK users still get a generic
Errorbut the message now contains the actual provider error text. TheAgentDeliveryErrorclass is purely additive.Made with Cursor
What changed
Provider delivery failures from outbound integrations (e.g., 401/403 from SendGrid, Slack, Teams, WhatsApp) are no longer swallowed as opaque 500s. The API maps provider errors to 502 Bad Gateway with a structured body ({ "error": "delivery_failed", "message": "..." }) and preserves provider detail, and the framework adds AgentDeliveryError so agent handlers can detect and handle delivery failures programmatically.
Affected areas
api: Wrapped provider delivery boundaries in agent chat and email flows so adapter/thread/post/edit/send calls are routed through a centralized toDeliveryError mapper that throws BadGatewayException (502) with an actionable message; same mapping added to the send-agent-test-email use case.
framework: Added AgentDeliveryError and exported it from @novu/framework; agent.context._post() now throws AgentDeliveryError for 502 responses and preserves provider text so agent code can instanceof-check and respond to delivery failures.
Key technical decisions
Testing
No new automated tests were added; changes are focused on error handling paths and should be validated via manual tests that trigger provider 4xx/5xx responses and by running existing unit/e2e suites to ensure no regressions.