Conversation
…7384] Handlers can now return MessageContent directly instead of calling ctx.reply() explicitly. The framework captures the return value and sends it as a reply before flushing. Returning void/undefined keeps the existing behaviour. Returning a value when ctx.reply() was already called sends a second reply (two-reply model). Made-with: Cursor
…gent handlers [NV-7384] Tests for onAction, onReaction (including early-return-as-silence for reaction removes), onResolve, and the two-reply model (explicit ctx.reply + return value sends two messages). Made-with: Cursor
✅ Deploy Preview for dashboard-v2-novu-staging canceled.
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAgent handlers may now return Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
packages/framework/src/resources/agent/agent.types.ts (1)
197-202: Document the new return-value-as-reply contract onAgentHandlers.Since
packages/frameworkis the developer-facing SDK andAgentHandlersis part of the public API, the new "return MessageContent → automaticctx.reply()" behavior should be documented in JSDoc on the interface (or per-handler). This is also a subtle behavior change for existing users: with the oldPromise<void>signature, a handler that incidentally returned a non-void value (e.g.,return await someHelper(ctx)) was a no-op; with this PR, any non-null return is now sent as a reply, which can cause duplicate posts or runtime serialization errors if the returned value isn't a validMessageContent. A JSDoc note (and a mention in the changelog/migration notes) would help users update existing handlers safely.📝 Proposed JSDoc
+/** + * Agent event handlers. + * + * Each handler may either: + * - call `ctx.reply(...)` explicitly, and/or + * - return a `MessageContent` value (or a Promise resolving to one), which the + * framework will automatically send as a reply before flushing queued signals. + * + * Returning `void`/`undefined` produces no reply. If a handler both calls + * `ctx.reply()` and returns a `MessageContent`, two replies are sent in order. + */ export interface AgentHandlers { onMessage: (ctx: AgentContext) => Awaitable<MessageContent | void>; onReaction?: (ctx: AgentContext) => Awaitable<MessageContent | void>; onAction?: (ctx: AgentContext) => Awaitable<MessageContent | void>; onResolve?: (ctx: AgentContext) => Awaitable<MessageContent | void>; }As per coding guidelines: "
packages/frameworkdefines the code-first workflow SDK and serves as the interface between user-defined workflows and Novu's engine; write clear, minimal abstractions as changes affect the developer-facing API".🤖 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 197 - 202, Update the public API docs for AgentHandlers to document the new return-as-reply contract: add JSDoc to the AgentHandlers interface (or each handler signature: onMessage, onReaction, onAction, onResolve) stating that returning a non-null MessageContent from a handler will automatically call ctx.reply() with that value, and warn that incidental non-void returns (e.g., returning a helper result) will now be sent as replies and must be valid MessageContent to avoid duplicate posts or serialization errors; reference AgentContext and ctx.reply() in the note and suggest returning void or explicitly calling ctx.reply() to preserve previous behavior.packages/framework/src/resources/agent/agent.test.ts (1)
1189-1196: Negative assertion relies on a fixed 50 ms timer — flaky risk.This negative test depends on a hard-coded 50 ms sleep to "prove" no reply was sent. On a slow/contended CI runner the background
runAgentHandlermay not have finished by the time the assertion runs, which would cause a false pass (test green, but a regression that does send a reply might still slip through). The other negative-style tests in this file await a deterministic signal first (vi.waitForon a captured ctx, fetch call count, etc.).Consider awaiting handler completion deterministically — e.g., a spy on
onReactionso you canvi.waitForon its invocation, or wait forctx.flush()to settle — and only then assert that no/replycall exists infetchMock.mock.calls.♻️ Sketch of a more deterministic wait
- it('should not send a reply when onReaction returns nothing (reaction removed)', async () => { + it('should not send a reply when onReaction returns nothing (reaction removed)', async () => { + const onReactionSpy = vi.fn((ctx: any) => { + if (!ctx.reaction?.added) return; + + return 'thumbs up noted'; + }); const testBot = agent('test-bot', { onMessage: async (ctx) => { await ctx.reply('noop'); }, - onReaction: (ctx) => { - if (!ctx.reaction?.added) return; - - return 'thumbs up noted'; - }, + onReaction: onReactionSpy, }); ... await handler.createHandler()(); - await new Promise((r) => setTimeout(r, 50)); + await vi.waitFor(() => expect(onReactionSpy).toHaveBeenCalled()); const replyCall = fetchMock.mock.calls.find( (call: any[]) => call[0] === 'https://api.novu.co/v1/agents/test-bot/reply' ); expect(replyCall).toBeUndefined(); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/framework/src/resources/agent/agent.test.ts` around lines 1189 - 1196, The test uses a fixed 50ms sleep after await handler.createHandler()(); replace that flaky wait with a deterministic signal that the background runAgentHandler has settled: add a spy/spyOn for the agent's onReaction (or otherwise await ctx.flush()) and then use vi.waitFor to wait for that spy or ctx.flush() to complete before asserting against fetchMock.mock.calls; specifically, ensure you wait for the runAgentHandler completion (via onReaction spy or ctx.flush()) and only then check that no call to 'https://api.novu.co/v1/agents/test-bot/reply' exists in fetchMock.mock.calls.
🤖 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.test.ts`:
- Around line 1189-1196: The test uses a fixed 50ms sleep after await
handler.createHandler()(); replace that flaky wait with a deterministic signal
that the background runAgentHandler has settled: add a spy/spyOn for the agent's
onReaction (or otherwise await ctx.flush()) and then use vi.waitFor to wait for
that spy or ctx.flush() to complete before asserting against
fetchMock.mock.calls; specifically, ensure you wait for the runAgentHandler
completion (via onReaction spy or ctx.flush()) and only then check that no call
to 'https://api.novu.co/v1/agents/test-bot/reply' exists in
fetchMock.mock.calls.
In `@packages/framework/src/resources/agent/agent.types.ts`:
- Around line 197-202: Update the public API docs for AgentHandlers to document
the new return-as-reply contract: add JSDoc to the AgentHandlers interface (or
each handler signature: onMessage, onReaction, onAction, onResolve) stating that
returning a non-null MessageContent from a handler will automatically call
ctx.reply() with that value, and warn that incidental non-void returns (e.g.,
returning a helper result) will now be sent as replies and must be valid
MessageContent to avoid duplicate posts or serialization errors; reference
AgentContext and ctx.reply() in the note and suggest returning void or
explicitly calling ctx.reply() to preserve previous behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 5f4f7bbd-61fe-4082-a373-d364a1b74ef5
📒 Files selected for processing (3)
packages/framework/src/handler.tspackages/framework/src/resources/agent/agent.test.tspackages/framework/src/resources/agent/agent.types.ts
…ly [NV-7384] Replace await ctx.reply() + return with direct return in the init template so new users see the idiomatic pattern from the start. Made-with: Cursor
commit: |
1 participant
What
Allows agent handlers to return
MessageContentdirectly instead of always callingctx.reply()explicitly.Works for all four handlers:
onMessage,onAction,onReaction,onResolve.Returning
void/undefinedkeeps existing behaviour — no reply sent via the return path.If
ctx.reply()is called inside the handler and a value is returned, both are sent (two-reply model — useful for "Thinking…" + final answer patterns).Why
Reduces boilerplate for the common case where a handler just replies once. Return syntax is especially natural in
onReactionwhere early returns model "do nothing on reaction removes".How
AgentHandlersreturn types changed fromPromise<void>→Awaitable<MessageContent | void>inagent.types.tsrunAgentHandlerinhandler.tscaptures the return value and callsctx.reply(result)when non-null, beforectx.flush()sequenceDiagram participant Bridge participant runAgentHandler participant handler as User Handler participant ctx Bridge->>runAgentHandler: event fires runAgentHandler->>handler: await handler(ctx) handler-->>runAgentHandler: result (MessageContent | void) alt result != null runAgentHandler->>ctx: ctx.reply(result) end runAgentHandler->>ctx: ctx.flush()Test coverage
onMessagereturn → replyonActionreturn → replyonReactionreturn → reply (added=true)onReactionearly return → silence (added=false)onResolvereturn → replyctx.reply()+ return value → two repliesMade with Cursor
What changed
Agent handlers (onMessage, onAction, onReaction, onResolve) may now return MessageContent (or a Promise resolving to it) directly; returned non-null values are automatically sent as replies. Handlers that return void/undefined preserve previous behavior. If a handler both calls ctx.reply() and returns content, both replies are sent (two-reply model). This simplifies handler code and lets authors use a return-value-as-reply pattern instead of always calling ctx.reply().
Affected areas
Key technical decisions
Testing
New and updated unit tests verify return-value-as-reply for all handler types, the reaction-removed (no-reply) case, and the two-reply scenario; existing tests remain passing.