fix(framework): unify text and markdown making markdown the default fixes NV-7392#10880
fix(framework): unify text and markdown making markdown the default fixes NV-7392#10880
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
…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
… 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
…ixes NV-7392
Remove the separate `text` content type from the agent reply system.
Plain strings passed to `ctx.reply()` now serialize as `{ markdown }`
instead of `{ text }`, routing them through the chat SDK's platform
conversion layer (mrkdwn for Slack, HTML for Teams, etc.) rather than
raw passthrough.
Breaking change: `ReplyContent.text` is removed. Any agent code relying
on the `text` wire field should update to `markdown`.
Made-with: Cursor
📝 WalkthroughWalkthroughThis PR removes the Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
…e fixes NV-7392 Made-with: Cursor
…erge next fixes NV-7392 Made-with: Cursor
✅ Deploy Preview for dashboard-v2-novu-staging canceled.
|
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/framework/src/resources/agent/agent.types.ts (1)
96-103: Breaking removal ofReplyContent.texton a public package export.Removing
textfromReplyContentis a breaking change to a@novu/frameworkexported type and will causeTS2353/TS2322for any consumer that referenced it (object literals, intermediate variables typed asReplyContent). The PR description notes this is intentional while agents are pre-release, which is fine — just confirm:
- The package version is bumped according to semver (major bump, or pre-release tag) and the change is called out in the changelog/migration notes.
- There are no remaining internal/external consumers reading
ReplyContent.text.A softer alternative — if you'd like to preserve the
@deprecatedstep expected by the package guideline — is to keep the field as@deprecated text?: stringfor one release before deletion. Otherwise, an explicit "BREAKING CHANGE" note in the release entry is sufficient.As per coding guidelines: "Deprecate symbols with
@deprecatedJSDoc before removing them from packages" and "Treat all exported symbols in packages as public API; follow semver conventions with breaking changes requiring major bumps".#!/bin/bash # Find any remaining references to ReplyContent.text in the monorepo (typed access patterns). rg -nP --type=ts -C2 '\bReplyContent\b' echo "---" # Look for direct .text property access on values typed as ReplyContent / agent reply content rg -nP --type=ts -C2 '(reply|content)\.text\b' apps packages 2>/dev/null | rg -v '\.test\.|\.e2e\.|message\.text|params\.text' | head -200 echo "---" # Confirm framework package version handling fd -t f 'package.json' packages/framework -x cat {}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/framework/src/resources/agent/agent.types.ts` around lines 96 - 103, Restore a nullable deprecated text property on the exported ReplyContent type to avoid breaking consumers: add " /** `@deprecated` text will be removed in next major release */ text?: string" to the ReplyContent interface (keep markdown/card/files as-is), then run the repository search suggested in the comment to confirm there are no remaining internal consumers reading text (check for ReplyContent references and .text access), and finally ensure the package version/changelog is updated for a breaking change if you intend to remove text immediately — otherwise bump a minor and remove text only after one release with a clear deprecation note in the changelog.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/framework/src/resources/agent/agent.types.ts`:
- Around line 96-103: Restore a nullable deprecated text property on the
exported ReplyContent type to avoid breaking consumers: add " /** `@deprecated`
text will be removed in next major release */ text?: string" to the ReplyContent
interface (keep markdown/card/files as-is), then run the repository search
suggested in the comment to confirm there are no remaining internal consumers
reading text (check for ReplyContent references and .text access), and finally
ensure the package version/changelog is updated for a breaking change if you
intend to remove text immediately — otherwise bump a minor and remove text only
after one release with a clear deprecation note in the changelog.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: c77df1f3-341b-4a2c-b942-9b671e93b756
📒 Files selected for processing (7)
apps/api/src/app/agents/dtos/agent-reply-payload.dto.tsapps/api/src/app/agents/e2e/agent-reply.e2e.tsapps/api/src/app/agents/services/chat-sdk.service.tsapps/api/src/app/agents/usecases/handle-agent-reply/handle-agent-reply.usecase.tspackages/framework/src/resources/agent/agent.context.tspackages/framework/src/resources/agent/agent.test.tspackages/framework/src/resources/agent/agent.types.ts
💤 Files with no reviewable changes (1)
- apps/api/src/app/agents/usecases/handle-agent-reply/handle-agent-reply.usecase.ts
Summary
textcontent type from the agent reply system — plain strings passed toctx.reply()now serialize as{ markdown }instead of{ text }ReplyContent.textfield removed from the wire format;ReplyContentDto.textremoved from the API DTOChatSdkServicepost/edit branches simplified: non-card content always goes throughthread.post({ markdown }), which routes through the chat SDK's platform conversion layer (Slack mrkdwn, Teams HTML, etc.) rather than raw passthroughWhy
string→{ text }was a raw passthrough to the platform — asterisks stayed as asterisks.string→{ markdown }lets thechatSDK handle platform-specific formatting conversion, which is the correct behavior. Since markdown is a superset of plain text, there is no regression for callers passing plain strings.Breaking change
ReplyContent.textis removed. This is a backwards-incompatible change intentionally made while the agents feature is pre-release.Test plan
pnpm --filter @novu/framework test— 289 tests pass, no type errorsMade with Cursor
What changed
Unifies the agent reply content model by removing the
textfield and makingmarkdownthe default. Plain strings passed toctx.reply()now serialize as{ markdown }instead of{ text }, allowing the chat SDK to handle platform-specific formatting conversion (e.g., Slack mrkdwn, Teams HTML) rather than treating text as a separate passthrough format.Affected areas
textfield fromReplyContentDtoDTO;ChatSdkServicenow always routes non-card content throughthread.post({ markdown })instead of branching on text versus markdown; text fallback extraction logic updated to only consider markdown.ReplyContentinterface no longer includes optionaltextfield; agent context serializes plain string input as{ markdown }instead of{ text }.Key technical decisions
ReplyContent.textremoved from the wire format and API contract (intentional while agents are pre-release).Testing
Existing unit tests (289 total) pass without modification. E2E tests updated to use
markdownfield instead oftextin request payloads and assertions across all reply and edit scenarios.