Conversation
…) signature Move the agent SDK handler signatures from a destructured payload object to a positional shape that puts the main entity first and ctx second: - onMessage: (message, ctx) => ... - onAction: (action, ctx) => ... - onReaction: (reaction, ctx) => ... - onResolve: (ctx) => ... This is a framework DX change only — the bridge wire protocol (AgentBridgeRequest, AgentAction.actionId, etc.) is unchanged, so the API server and downstream consumers of the bridge payload are not affected. Inside onAction, fields are now read as action.actionId / action.value off the AgentAction argument. Co-authored-by: Dima Grossman <dima@grossman.io>
…dler signature Match the @novu/framework handler API change: - packages/novu init template (support-agent.tsx): onMessage / onAction now take positional (message, ctx) / (action, ctx) args; references to actionId / value are read off the action argument (action.actionId, action.value). - README-template.md: onMessage example switched to (message, ctx). - apps/api e2e demo (mock-agent-handler.ts): onMessage, onAction, onResolve handlers updated to positional args. Co-authored-by: Dima Grossman <dima@grossman.io>
✅ 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:
📝 WalkthroughWalkthroughThis PR refactors the agent handler callback API from wrapper objects with destructured parameters to positional arguments. Handler signatures change from ChangesAgent Handler API Refactor
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
Contributor
|
Hey there and thank you for opening this pull request! 👋 We require pull request titles to follow specific formatting rules and it looks like your proposed title needs to be adjusted. Your PR title is: Requirements:
Expected format: Details: PR title must end with 'fixes TICKET-ID' (e.g., 'fixes NOV-123') or include ticket ID in branch name |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
packages/novu/src/commands/init/templates/app-agent/ts/app/novu/agents/support-agent.tsx (3)
42-48:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winMissing blank line before return statement.
Per coding guidelines, include a blank line before every
returnstatement.📝 Proposed fix
ctx.metadata.set('lastMessage', text); return ( `**Got it.** You said: "${message.text}"\n\n` +Should be:
ctx.metadata.set('lastMessage', text); + return ( `**Got it.** You said: "${message.text}"\n\n` +As per coding guidelines: "Include a blank line before every
returnstatement".🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/novu/src/commands/init/templates/app-agent/ts/app/novu/agents/support-agent.tsx` around lines 42 - 48, Add a blank line immediately before the return statement that constructs the demo agent message in support-agent.tsx (the function that builds the string using message.text, ctx.history, and ctx.metadata.get('topic')), i.e., insert one empty line between the preceding code and the return (...) line to satisfy the "blank line before return" guideline.
19-29:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winMissing blank line before return statement.
Per coding guidelines, include a blank line before every
returnstatement.📝 Proposed fix
<Card title={`Hi${firstName ? `, ${firstName}` : ''}! I'm Support Agent`}> <CardText>How can I help you today?</CardText> <Actions> <Button id="topic-billing" label="Billing question" value="billing" /> <Button id="topic-technical" label="Technical issue" value="technical" /> <Button id="topic-other" label="Something else" value="other" /> </Actions> </Card> ); } if (text.includes('resolve') || text.includes('thanks')) {Should be:
<Card title={`Hi${firstName ? `, ${firstName}` : ''}! I'm Support Agent`}> <CardText>How can I help you today?</CardText> <Actions> <Button id="topic-billing" label="Billing question" value="billing" /> <Button id="topic-technical" label="Technical issue" value="technical" /> <Button id="topic-other" label="Something else" value="other" /> </Actions> </Card> ); } + if (text.includes('resolve') || text.includes('thanks')) {As per coding guidelines: "Include a blank line before every
returnstatement".🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/novu/src/commands/init/templates/app-agent/ts/app/novu/agents/support-agent.tsx` around lines 19 - 29, Add a blank line immediately before the return statement in the Support Agent component (the JSX return block in support-agent.tsx) so the function's logic and the return are separated per style guidelines; locate the component that renders <Card title={`Hi${firstName ? `, ${firstName}` : ''}! I'm Support Agent`}> and insert a single empty line just above the line starting with return to satisfy the "blank line before return" rule.
35-37:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winMissing blank line before return statement.
Per coding guidelines, include a blank line before every
returnstatement.📝 Proposed fix
ctx.resolve(`Resolved by user: ${text}`); // ctx.trigger('follow-up-survey', { to: ctx.subscriber }); return 'Glad I could help! Marking this resolved.'; }Should be:
ctx.resolve(`Resolved by user: ${text}`); // ctx.trigger('follow-up-survey', { to: ctx.subscriber }); + return 'Glad I could help! Marking this resolved.'; }As per coding guidelines: "Include a blank line before every
returnstatement".🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/novu/src/commands/init/templates/app-agent/ts/app/novu/agents/support-agent.tsx` around lines 35 - 37, Insert a blank line immediately before the return statement that returns 'Glad I could help! Marking this resolved.' in support-agent.tsx to comply with the coding guideline that every return must be preceded by a blank line; locate the return in the support-agent component/function (the line containing return 'Glad I could help! Marking this resolved.') and add one empty line above it.
🧹 Nitpick comments (2)
packages/framework/src/handler.ts (1)
327-327: 💤 Low valueNon-null assertion is safe but could add runtime guard for clarity.
ctx.message!assumesmessageis always present forON_MESSAGEevents, which is correct based on the bridge protocol. However, for defense in depth, consider adding a runtime check before the non-null assertion:if (!ctx.message) { throw new InvalidActionError(event, AgentEventEnum); } await replyIfPresent( await registeredAgent.handlers.onMessage(ctx.message, ctx as unknown as AgentMessageContext) );This pattern would apply to lines 333 (
ctx.action!) and 340 (ctx.reaction!) as well.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/framework/src/handler.ts` at line 327, Add runtime guards before using non-null assertions for message/action/reaction: check if ctx.message is present and if not throw InvalidActionError(event, AgentEventEnum), then call registeredAgent.handlers.onMessage with ctx.message and ctx as AgentMessageContext and pass its result into replyIfPresent; apply the same pattern for ctx.action (using the onAction handler and appropriate AgentActionContext) and ctx.reaction (using the onReaction handler and appropriate AgentReactionContext) to replace direct non-null assertions like ctx.message!, ctx.action!, ctx.reaction!.packages/framework/src/resources/agent/agent.test.ts (1)
122-124: ⚡ Quick winConsider using more specific types than
any.The test handler uses
anyfor both parameters. Since this is testing the framework's dispatch logic, using the actual types would provide better type safety:-const onMessageSpy = vi.fn(async (_message: any, ctx: any) => { +const onMessageSpy = vi.fn(async (_message: AgentMessage, ctx: AgentMessageContext) => { await ctx.reply('Echo: Hello bot!'); });This would catch type mismatches between the mock and actual signatures.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/framework/src/resources/agent/agent.test.ts` around lines 122 - 124, Replace the loose any types on the test spy with the real handler parameter types: change the onMessageSpy signature from vi.fn(async (_message: any, ctx: any) => ...) to use the framework's concrete types (e.g., (message: AgentMessage, ctx: AgentContext) or the actual names used by your codebase) and import those types at the top of the test file, so the test handler (onMessageSpy) matches the real handler signature and TypeScript will catch mismatches with methods like ctx.reply.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/framework/src/handler.ts`:
- Line 327: AgentContextImpl.event is typed as string causing unsafe
double-casts like the one around the onMessage call; change its type to the
specific event union or enum (e.g., AgentEventEnum or the literal union
'onMessage'|'onAction'|'onReaction'|'onResolve') so TypeScript can narrow the
context in the switch and you can remove the casts at the four call sites (the
line with registeredAgent.handlers.onMessage(ctx.message!, ctx as unknown as
AgentMessageContext) and the similar onAction/onReaction/onResolve calls);
update the AgentContextImpl declaration to use that enum/union and rebuild to
verify the double-casts are no longer required.
In `@packages/framework/src/resources/agent/agent.types.ts`:
- Around line 302-340: This change is a breaking API change to the exported
AgentHandlers interface (onMessage, onReaction, onAction, onResolve) — either
restore backward compatibility or mark the old signature as deprecated and
follow semver rules: add a backward-compatible export (e.g., AgentHandlersOld or
an overloaded AgentHandlers type that accepts the previous destructured
parameter shapes) and annotate it with `@deprecated` JSDoc pointing to the new
positional-parameter signatures, or if you intend to remove the old shape now,
update package release metadata to perform a major version bump and add a clear
changelog entry/upgrade note; ensure the AgentHandlers symbol is updated
consistently and that JSDoc deprecation comments reference the exact function
names (onMessage, onReaction, onAction, onResolve).
In
`@packages/novu/src/commands/init/templates/app-agent/ts/app/novu/agents/support-agent.tsx`:
- Around line 55-56: The return statement in support-agent.tsx (inside the
function that builds the reply—look for the logic that returns `Topic set to
**${action.value}**. Describe your issue and I'll help.`) lacks the required
blank line before it; add a single blank line immediately above that return so
it follows the coding guideline of having a blank line before every return
statement.
---
Outside diff comments:
In
`@packages/novu/src/commands/init/templates/app-agent/ts/app/novu/agents/support-agent.tsx`:
- Around line 42-48: Add a blank line immediately before the return statement
that constructs the demo agent message in support-agent.tsx (the function that
builds the string using message.text, ctx.history, and
ctx.metadata.get('topic')), i.e., insert one empty line between the preceding
code and the return (...) line to satisfy the "blank line before return"
guideline.
- Around line 19-29: Add a blank line immediately before the return statement in
the Support Agent component (the JSX return block in support-agent.tsx) so the
function's logic and the return are separated per style guidelines; locate the
component that renders <Card title={`Hi${firstName ? `, ${firstName}` : ''}! I'm
Support Agent`}> and insert a single empty line just above the line starting
with return to satisfy the "blank line before return" rule.
- Around line 35-37: Insert a blank line immediately before the return statement
that returns 'Glad I could help! Marking this resolved.' in support-agent.tsx to
comply with the coding guideline that every return must be preceded by a blank
line; locate the return in the support-agent component/function (the line
containing return 'Glad I could help! Marking this resolved.') and add one empty
line above it.
---
Nitpick comments:
In `@packages/framework/src/handler.ts`:
- Line 327: Add runtime guards before using non-null assertions for
message/action/reaction: check if ctx.message is present and if not throw
InvalidActionError(event, AgentEventEnum), then call
registeredAgent.handlers.onMessage with ctx.message and ctx as
AgentMessageContext and pass its result into replyIfPresent; apply the same
pattern for ctx.action (using the onAction handler and appropriate
AgentActionContext) and ctx.reaction (using the onReaction handler and
appropriate AgentReactionContext) to replace direct non-null assertions like
ctx.message!, ctx.action!, ctx.reaction!.
In `@packages/framework/src/resources/agent/agent.test.ts`:
- Around line 122-124: Replace the loose any types on the test spy with the real
handler parameter types: change the onMessageSpy signature from vi.fn(async
(_message: any, ctx: any) => ...) to use the framework's concrete types (e.g.,
(message: AgentMessage, ctx: AgentContext) or the actual names used by your
codebase) and import those types at the top of the test file, so the test
handler (onMessageSpy) matches the real handler signature and TypeScript will
catch mismatches with methods like ctx.reply.
🪄 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: 70feff5d-aed4-4b7c-b239-9266c999c249
📒 Files selected for processing (6)
apps/api/src/app/agents/e2e/mock-agent-handler.tspackages/framework/src/handler.tspackages/framework/src/resources/agent/agent.test.tspackages/framework/src/resources/agent/agent.types.tspackages/novu/src/commands/init/templates/app-agent/ts/README-template.mdpackages/novu/src/commands/init/templates/app-agent/ts/app/novu/agents/support-agent.tsx
…he wire)
Split the agent action type so handlers see action.id while the bridge
wire payload keeps its actionId field (no API contract change):
- AgentAction (handler-facing) now has { id, value, sourceMessageId }.
- AgentBridgeAction is the wire-facing shape with { actionId, value,
sourceMessageId }. AgentBridgeRequest.action uses this new type.
- AgentContextImpl remaps request.action.actionId -> action.id when
constructing the context, so ctx.action and the onAction argument both
expose .id.
- AgentBridgeAction is exported from the package barrel for consumers
(like the API server) that need the wire-facing type.
Co-authored-by: Dima Grossman <dima@grossman.io>
The framework's AgentAction is now the handler-facing shape (with .id). The API server constructs and forwards the bridge payload, so its typed references are switched to AgentBridgeAction (the wire-facing shape with .actionId). No runtime behavior or wire payload changes. Co-authored-by: Dima Grossman <dima@grossman.io>
Switch the init template (support-agent.tsx) and the mock agent demo (mock-agent-handler.ts) to read action.id / action.value off the handler argument, matching the new AgentAction handler-facing shape. Co-authored-by: Dima Grossman <dima@grossman.io>
cursor
Bot
changed the title
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/framework/src/resources/agent/agent.types.ts (1)
148-169:⚠️ Potential issue | 🟠 Major | ⚖️ Poor tradeoffRenaming
AgentAction.actionId→AgentAction.idis also a breaking change to a previously-public type.
AgentActionwas previously the wire-facing shape (withactionId) and is re-exported frompackages/framework/src/index.ts. After this PR, consumers who importedAgentActionand readaction.actionIdwill silently getundefinedat runtime (TS will catch it at compile time, but runtime users of plain JS won't). The newly introducedAgentBridgeActioncovers the wire shape, but external consumers relying on the oldAgentAction.actionIdneed a migration path.Consider one of:
- Keeping
actionIdas a@deprecatedfield onAgentActionfor one release and dual-populating it inAgentContextImpl, then removing in the next major.- Documenting the rename explicitly in the changelog / upgrade notes so SDK users know to switch to
action.id(and toAgentBridgeActionif they were consuming the bridge payload directly).As per coding guidelines: "Treat all exported symbols in packages as public API; follow semver conventions with breaking changes requiring major bumps" and "Deprecate symbols with
@deprecatedJSDoc before removing them from packages".🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/framework/src/resources/agent/agent.types.ts` around lines 148 - 169, AgentAction was changed to use .id which breaks JS consumers expecting .actionId; restore backward-compatibility by reintroducing an actionId field on AgentAction as a `@deprecated` alias and ensure AgentContextImpl (the code that constructs/populates AgentAction) fills both .id and .actionId (dual-populate) for this release, add a JSDoc `@deprecated` comment on actionId, and leave a TODO to remove actionId in the next major; alternatively, if you prefer not to preserve runtime compatibility here, document the breaking rename clearly in the changelog/upgrade notes and ensure AgentBridgeAction remains the wire shape.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@packages/framework/src/resources/agent/agent.types.ts`:
- Around line 148-169: AgentAction was changed to use .id which breaks JS
consumers expecting .actionId; restore backward-compatibility by reintroducing
an actionId field on AgentAction as a `@deprecated` alias and ensure
AgentContextImpl (the code that constructs/populates AgentAction) fills both .id
and .actionId (dual-populate) for this release, add a JSDoc `@deprecated` comment
on actionId, and leave a TODO to remove actionId in the next major;
alternatively, if you prefer not to preserve runtime compatibility here,
document the breaking rename clearly in the changelog/upgrade notes and ensure
AgentBridgeAction remains the wire shape.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: a6139693-8feb-4aba-9f7e-5d751e4b4860
📒 Files selected for processing (9)
apps/api/src/app/agents/e2e/mock-agent-handler.tsapps/api/src/app/agents/services/agent-inbound-handler.service.tsapps/api/src/app/agents/services/bridge-executor.service.tspackages/framework/src/index.tspackages/framework/src/resources/agent/agent.context.tspackages/framework/src/resources/agent/agent.test.tspackages/framework/src/resources/agent/agent.types.tspackages/framework/src/resources/agent/index.tspackages/novu/src/commands/init/templates/app-agent/ts/app/novu/agents/support-agent.tsx
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/novu/src/commands/init/templates/app-agent/ts/app/novu/agents/support-agent.tsx
- apps/api/src/app/agents/e2e/mock-agent-handler.ts
- packages/framework/src/resources/agent/agent.test.ts
…al-handler-signature-fc71 # Conflicts: # packages/framework/src/resources/agent/agent.test.ts # packages/framework/src/resources/agent/agent.types.ts # packages/novu/src/commands/init/templates/app-agent/ts/README-template.md Co-authored-by: Dima Grossman <dima@grossman.io>
…uble-casts Type AgentContextImpl.event as AgentEventEnum (the union of all four literal event names) instead of plain string. With the discriminator narrowed to the same literal union the per-event AgentXxxContext interfaces use, the dispatch in runAgentHandler() no longer needs the 'as unknown as AgentMessageContext' / Action / Reaction / Resolve double-cast — a single 'as' is sufficient. Per CodeRabbit review on PR #11060. Co-authored-by: Dima Grossman <dima@grossman.io>
…on.id)
Drop AgentBridgeAction now that the bridge JSON also uses { id, value,
sourceMessageId } for action payloads. AgentBridgeRequest.action points
at AgentAction directly, and AgentContextImpl no longer needs to remap
actionId -> id when constructing the context.
Test fixtures for the bridge request switch to { id: ... } on the
action payload to match the new wire shape.
Co-authored-by: Dima Grossman <dima@grossman.io>
Align the API bridge contract with the @novu/framework handler shape:
- chat-sdk.service.ts: forward the upstream chat-SDK event to the
inbound handler as { id, value, sourceMessageId } instead of
{ actionId, value, sourceMessageId }. The third-party event field
(event.actionId) is unchanged - only the bridge-facing field flips.
- agent-inbound-handler.service.ts: handleAction() now takes AgentAction
again; first-message marker and analytics read action.id. The
analytics property name keeps its existing 'actionId' label so
dashboards stay stable.
- bridge-executor.service.ts: BridgeExecutorParams.action is typed as
AgentAction; deliveryId template reads action.id.
Co-authored-by: Dima Grossman <dima@grossman.io>
cursor
Bot
changed the title
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/framework/src/resources/agent/agent.types.ts`:
- Around line 144-152: The current AgentAction type is being reused for both
wire transport (AgentBridgeRequest.action) and handler-facing APIs (onAction),
causing compatibility breaks when bridging versions; define a separate transport
type (e.g., AgentBridgeAction) that accepts both actionId and id (both
optional/string) for backward/forward compatibility, keep AgentAction as the
normalized handler-facing shape with a required id string, and update the
bridge-to-handler normalization logic (where AgentBridgeRequest.action is
converted before calling onAction) to map actionId -> id if present so handlers
always receive a single stable AgentAction; update references to
AgentBridgeRequest.action and any bridge parsing/adapter code to use
AgentBridgeAction and perform the normalization.
🪄 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: 3b8bf6f6-23a7-46cc-907a-32ef3c2d5fbe
📒 Files selected for processing (6)
apps/api/src/app/agents/services/agent-inbound-handler.service.tsapps/api/src/app/agents/services/bridge-executor.service.tsapps/api/src/app/agents/services/chat-sdk.service.tspackages/framework/src/resources/agent/agent.context.tspackages/framework/src/resources/agent/agent.test.tspackages/framework/src/resources/agent/agent.types.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/framework/src/resources/agent/agent.test.ts
Comment on lines
+144
to
+152
| /** | ||
| * Data carried by a button click or other interactive action. | ||
| * | ||
| * Used both on the bridge wire (`AgentBridgeRequest.action`) and as the | ||
| * handler-facing argument passed to `onAction(action, ctx)`. | ||
| */ | ||
| export interface AgentAction { | ||
| /** The `id` prop of the clicked `<Button>` or action element. */ | ||
| actionId: string; | ||
| id: string; |
Contributor
There was a problem hiding this comment.
Keep the bridge action shape separate from the handler-facing action.
Reusing AgentAction for both AgentBridgeRequest.action and onAction(action, ctx) removes the version-skew shim. With this shape, a new framework talking to an old API gets action.id === undefined, and an old framework talking to a new API loses action.actionId, so action handlers break during rollout.
💡 Compatibility-safe direction
+export interface AgentBridgeAction {
+ actionId: string;
+ value?: string;
+ sourceMessageId?: string;
+}
+
export interface AgentAction {
id: string;
value?: string;
sourceMessageId?: string;
} export interface AgentBridgeRequest {
- action: AgentAction | null;
+ action: AgentBridgeAction | null;
} // packages/framework/src/resources/agent/agent.context.ts
- this.action = request.action ?? null;
+ this.action = request.action
+ ? {
+ id: request.action.actionId,
+ value: request.action.value,
+ sourceMessageId: request.action.sourceMessageId,
+ }
+ : null;If you want a zero-downtime rollout, the bridge can temporarily accept/send both actionId and id, but the SDK should still normalize to a single handler-facing AgentAction.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/framework/src/resources/agent/agent.types.ts` around lines 144 -
152, The current AgentAction type is being reused for both wire transport
(AgentBridgeRequest.action) and handler-facing APIs (onAction), causing
compatibility breaks when bridging versions; define a separate transport type
(e.g., AgentBridgeAction) that accepts both actionId and id (both
optional/string) for backward/forward compatibility, keep AgentAction as the
normalized handler-facing shape with a required id string, and update the
bridge-to-handler normalization logic (where AgentBridgeRequest.action is
converted before calling onAction) to map actionId -> id if present so handlers
always receive a single stable AgentAction; update references to
AgentBridgeRequest.action and any bridge parsing/adapter code to use
AgentBridgeAction and perform the normalization.
Contributor
Author
There was a problem hiding this comment.
This feature has not yet been released, so it's ok
Contributor
There was a problem hiding this comment.
@scopsy Understood — since this is an unreleased feature, there's no live version-skew risk to protect against. The single AgentAction shape is fine for now. Just worth keeping the AgentBridgeAction separation in mind if/when a migration path to existing deployments is needed in the future. Thanks for the context!
🧠 Learnings used
Learnt from: ChmaraX
Repo: novuhq/novu PR: 11003
File: packages/framework/src/handler.ts:0-0
Timestamp: 2026-05-06T13:36:38.575Z
Learning: In novuhq/novu, Biome's noUselessUndefinedInitialization rule automatically strips '= undefined' initializers in TypeScript files. When fixing TS2454 'used before assigned' errors, prefer structural fixes over adding '= undefined' (which will be removed and can cause the commit to fail). Do not initialize variables with undefined. Instead, initialize with a concrete value at declaration when possible, or refactor to ensure the value is assigned before use (e.g., via a helper function or inline initialization that yields a valid value). Use guards or explicit control flow to guarantee assignment before first use. This guidance applies broadly to TypeScript files in the repository, including packages/framework/src/handler.ts.
Learnt from: scopsy
Repo: novuhq/novu PR: 11060
File: packages/novu/src/commands/init/templates/app-agent/ts/app/novu/agents/support-agent.tsx:55-56
Timestamp: 2026-05-11T05:04:35.896Z
Learning: In PR reviews for TypeScript/TSX code, do not suggest adding blank lines before `return` statements. Follow the repository’s existing formatting conventions and avoid raising review comments about this specific whitespace change (blank line insertion before `return`).
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What changed? Why was the change needed?
What changed
This PR refactors the
@novu/frameworkagent handler signatures from a destructured payload object pattern to positional parameters. Handlers now receive the primary entity (message, action, reaction) as the first parameter andctxas the second (e.g.,onMessage(message, ctx)instead ofonMessage({ message, ctx })). TheactionIdfield on agent actions is renamed toidthroughout. The bridge wire contract was updated to transmitaction.idinstead ofaction.actionIdto align handler and wire representations.Affected areas
AgentHandlersinterface and handler dispatch logic (runAgentHandler) to accept positional parameters; renamedactionIdtoidinAgentActiontype; updated JSDoc and tests to reflect new signatures.chat-sdk.service.ts,agent-inbound-handler.service.ts,bridge-executor.service.ts) to map incoming actions usingaction.idand updated analytics tracking accordingly; updated e2e mock handler signatures.support-agent.tsx) to use new positional handler signatures.Key technical decisions
action.idinstead ofaction.actionId.action.idconsistently with handler-facing representation, eliminating remapping at the framework layer.Testing
Handler signatures and dispatch logic were validated through existing unit tests in
agent.test.ts(46 lines updated). API service integration was tested via e2e mock handler. Local typechecks and builds passed (vitest: 27 files, 317 tests).The
@novu/frameworkagent SDK previously required every handler to destructure a single payload object:That is awkward — every handler must destructure, and the main entity (message, action, reaction) is buried alongside
ctx. This PR switches the handlers to a positional signature where the main entity is the first argument andctxis the second, and aligns the bridge wire payload to match (action.id/action.value):Wire alignment. The bridge JSON sent by the API server now also uses
idinstead ofactionId:{ "event": "onAction", "action": { "id": "topic-billing", "value": "billing", "sourceMessageId": "msg-card-001" }, ... }AgentActionis the single source of truth for both the wire (AgentBridgeRequest.action) and the handler-facing argument. The intermediateAgentBridgeActiontype and the remap inAgentContextImpl(introduced in an earlier commit on this PR) have been removed.Scope of changes:
AgentHandlersinterface, JSDoc, andAgentActiondefinition inpackages/framework/src/resources/agent/agent.types.tspackages/framework/src/handler.ts(runAgentHandler()), with tightenedAgentContextImpl.event: AgentEventEnumto drop theas unknown asdouble-cast (CodeRabbit suggestion)AgentContextImplpassesrequest.actionthrough unchanged (no remap)id)support-agent.tsx,README-template.md) and dev demo (mock-agent-handler.ts) readaction.id/action.valuechat-sdk.service.tsconstructs{ id: event.actionId, value, sourceMessageId }when forwarding upstream chat-SDK eventsagent-inbound-handler.service.tsandbridge-executor.service.tstakeAgentActionand readaction.idactionIdinagent-analytics.ts) is intentionally kept stable so existing dashboards keep working; it is now sourced fromaction.idScreenshots
N/A — SDK + bridge contract change.
Expand for optional sections
Related enterprise PR
N/A.
Special notes for your reviewer
@novu/framework(2.10.1-alpha.6) and the bridge protocol. Existing handler authors must update their callback signature and readaction.id/action.value. Since the agent feature is unreleased alpha, no deprecation wrappers were added.event.actionIdinchat-sdk.service.ts) is unrelated and remains untouched.actionIdis kept stable to avoid breaking downstream dashboards — sourced fromaction.id.npx vitest run --typecheckinpackages/framework(27 files / 317 tests passed, no type errors) andpnpm build(cleanattw+ tsup output, no circular deps). API-servertsc --noEmitshows no new errors in the touched agent files.