feat(js): add discriminated AgentContext types per handler fixes NV-7509#10974
feat(js): add discriminated AgentContext types per handler fixes NV-7509#10974
Conversation
Introduce handler-specific context types (AgentMessageContext, AgentActionContext, AgentReactionContext, AgentResolveContext) so each handler callback receives only the fields guaranteed to be non-null. - AgentMessageContext: ctx.message is non-null, no action/reaction - AgentActionContext: ctx.action is non-null - AgentReactionContext: ctx.reaction is non-null - AgentResolveContext: minimal context, no message/action/reaction The original AgentContext is preserved as deprecated for backward compatibility. AgentHandlers now uses the narrowed types. Adds 4 new tests verifying type-safe handler dispatch. Co-authored-by: Adam Chmara <adam.chmara1@gmail.com>
✅ Deploy Preview for dashboard-v2-novu-staging canceled.
|
📝 WalkthroughWalkthroughReplaces a generic AgentContext with event-discriminated context types (Message/Action/Reaction/Resolve), updates barrels to re-export the new types, removes the ChangesAgent handler and types
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
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. Review rate limit: 6/8 reviews remaining, refill in 13 minutes and 17 seconds.Comment |
…est case Co-authored-by: Adam Chmara <adam.chmara1@gmail.com>
Co-authored-by: Adam Chmara <adam.chmara1@gmail.com>
Since agents are unreleased, no backward compat needed. AgentContext is now a union of the four discriminated types. AgentContextImpl drops the implements clause since it is internal. Co-authored-by: Adam Chmara <adam.chmara1@gmail.com>
Use `any` for the internal dispatch map instead of per-case type casts. The type safety boundary is at the public AgentHandlers interface. Co-authored-by: Adam Chmara <adam.chmara1@gmail.com>
Co-authored-by: Adam Chmara <adam.chmara1@gmail.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
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.context.ts (1)
250-253:⚠️ Potential issue | 🟠 MajorThe type assertion at line 317 bypasses structural type safety — add event-based validation and null-guards before handler dispatch.
The core concern is valid.
AgentContextImpldeclares nullablemessage,action, andreactionfields, but the discriminated handler types require these to be non-null:
AgentMessageContext.message: AgentMessage(non-nullable)AgentActionContext.action: AgentAction(non-nullable)AgentReactionContext.reaction: AgentReaction(non-nullable)In handler.ts, line 317 uses a type assertion that falsely claims all handlers accept
AgentContextImpl, bypassing TypeScript's type-safety. No null-checks exist between context creation (line 226) and handler invocation (line 325). If a malformed bridge request sendsevent='onMessage'withmessage: null, the handler will receive null and crash when accessingctx.message.text, despite the type signature promising non-null.Required fix: validate the event and bridge request data match before dispatch. For example, reject
onMessageevents whenmessageis null, or narrow the context type based on the event enum before invoking the handler.🤖 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 250 - 253, The handler dispatch currently asserts types and can pass nulls to handlers; update AgentContextImpl dispatch logic to validate event + payload before invoking handlers: inspect ctx.event and ensure required fields are non-null (for event === 'onMessage' confirm ctx.message is present and cast/narrow to AgentMessageContext, for 'onAction' confirm ctx.action, for 'onReaction' confirm ctx.reaction), reject or throw a clear error if the payload is missing/invalid, and only then call the corresponding handler (referencing AgentContextImpl, AgentMessageContext, AgentActionContext, AgentReactionContext and the dispatch site where handlers are invoked) so TypeScript narrowing and runtime null-guards guarantee non-null fields for each handler.
🧹 Nitpick comments (2)
packages/framework/src/resources/agent/agent.types.ts (1)
199-203: ⚡ Quick win
addReactionJSDoc example referencesctx.reactionwhich doesn't exist on non-reaction contexts.The example at lines 200–201 (
ctx.addReaction(ctx.reaction!.messageId, 'check_mark')) lives onAgentContextBase, so it appears in IDE tooling for all four context types. Consumers ofAgentMessageContext,AgentActionContext, andAgentResolveContextwill see an example that references a property that does not exist on their type.Consider replacing the example with a standalone
messageId:✏️ Proposed fix
- * `@example` - * ctx.addReaction(ctx.reaction!.messageId, 'check_mark'); - * await ctx.reply('Done!'); + * `@example` + * ctx.addReaction('<platform-message-id>', 'check_mark'); + * await ctx.reply('Done!');🤖 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 199 - 203, The JSDoc example for addReaction on AgentContextBase references ctx.reaction which isn't present on AgentMessageContext/AgentActionContext/AgentResolveContext; update the addReaction JSDoc (method addReaction in AgentContextBase) to use a standalone messageId variable (e.g., const messageId = '...'; ctx.addReaction(messageId, 'check_mark'); await ctx.reply('Done!')) so the example is valid for all context types and remove any usage of ctx.reaction from the example.packages/framework/src/resources/agent/agent.context.ts (1)
249-249: ⚡ Quick win
AgentContextImplhas noimplementsclause — type contract is unverified.
implements AgentContextwas correctly removed since TypeScript forbids implementing a union type. However,AgentContextImplstructurally satisfiesAgentContextBasebut has no compile-time enforcement of that contract. If a required method (trigger,addReaction,resolve, etc.) is accidentally removed or its signature changed, TypeScript won't catch it until the cast inhandler.tsis exercised.Exporting
AgentContextBasefromagent.types.tsand addingimplements AgentContextBaseto this class would restore the safety net:✏️ Proposed fix
In
agent.types.ts:-interface AgentContextBase { +export interface AgentContextBase {In
agent.context.ts:+import type { ..., AgentContextBase } from './agent.types'; -export class AgentContextImpl { +export class AgentContextImpl implements AgentContextBase {🤖 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` at line 249, Export the AgentContextBase type from agent.types.ts (if not already exported) and add an implements clause to the class declaration: change AgentContextImpl to implement AgentContextBase; then adjust any method signatures in AgentContextImpl (e.g., trigger, addReaction, resolve, etc.) to match the AgentContextBase interface so the compiler enforces the contract and surface mismatches during build.
🤖 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.types.ts`:
- Line 225: Add a JSDoc deprecation tag to the exported AgentContext union:
update the declaration for AgentContext (the exported type named "AgentContext")
to include a top-line JSDoc comment with "@deprecated" and a short migration
hint (e.g., use AgentMessageContext | AgentActionContext | AgentReactionContext
| AgentResolveContext directly), so consumers are warned that the previous
unified access pattern is deprecated and should narrow to the specific context
types.
---
Outside diff comments:
In `@packages/framework/src/resources/agent/agent.context.ts`:
- Around line 250-253: The handler dispatch currently asserts types and can pass
nulls to handlers; update AgentContextImpl dispatch logic to validate event +
payload before invoking handlers: inspect ctx.event and ensure required fields
are non-null (for event === 'onMessage' confirm ctx.message is present and
cast/narrow to AgentMessageContext, for 'onAction' confirm ctx.action, for
'onReaction' confirm ctx.reaction), reject or throw a clear error if the payload
is missing/invalid, and only then call the corresponding handler (referencing
AgentContextImpl, AgentMessageContext, AgentActionContext, AgentReactionContext
and the dispatch site where handlers are invoked) so TypeScript narrowing and
runtime null-guards guarantee non-null fields for each handler.
---
Nitpick comments:
In `@packages/framework/src/resources/agent/agent.context.ts`:
- Line 249: Export the AgentContextBase type from agent.types.ts (if not already
exported) and add an implements clause to the class declaration: change
AgentContextImpl to implement AgentContextBase; then adjust any method
signatures in AgentContextImpl (e.g., trigger, addReaction, resolve, etc.) to
match the AgentContextBase interface so the compiler enforces the contract and
surface mismatches during build.
In `@packages/framework/src/resources/agent/agent.types.ts`:
- Around line 199-203: The JSDoc example for addReaction on AgentContextBase
references ctx.reaction which isn't present on
AgentMessageContext/AgentActionContext/AgentResolveContext; update the
addReaction JSDoc (method addReaction in AgentContextBase) to use a standalone
messageId variable (e.g., const messageId = '...'; ctx.addReaction(messageId,
'check_mark'); await ctx.reply('Done!')) so the example is valid for all context
types and remove any usage of ctx.reaction from the example.
🪄 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: b56f2d45-1855-4658-9485-c769ec3979b6
📒 Files selected for processing (3)
packages/framework/src/handler.tspackages/framework/src/resources/agent/agent.context.tspackages/framework/src/resources/agent/agent.types.ts
✅ Files skipped from review due to trivial changes (1)
- packages/framework/src/handler.ts
2 participants
What changed
Introduces handler-specific discriminated context types for each agent handler callback, replacing the single generic
AgentContextthat all handlers previously shared.New types
onMessageAgentMessageContextctx.message,ctx.event === 'onMessage'onActionAgentActionContextctx.action,ctx.event === 'onAction'onReactionAgentReactionContextctx.reaction,ctx.event === 'onReaction'onResolveAgentResolveContextctx.event === 'onResolve'(minimal context)Before
After
Details
AgentContextBaseinterface (not exported) containing the common fields:conversation,subscriber,history,platform,platformContext,reply(),resolve(),metadata,trigger(),addReaction()AgentHandlersnow maps each handler to its specific context typeAgentContextis preserved with@deprecatedfor backward compatibilityhandler.tsdispatches to the correct handler with a type-safe switch instead of a generic mapFiles changed
packages/framework/src/resources/agent/agent.types.ts— new discriminated types + deprecatedAgentContextpackages/framework/src/resources/agent/index.ts— export new typespackages/framework/src/index.ts— export new types at package levelpackages/framework/src/handler.ts— type-safe handler dispatch via switchLinear Issue: NV-7509
What changed
Replaces the single generic AgentContext with four handler-specific discriminated context types (AgentMessageContext, AgentActionContext, AgentReactionContext, AgentResolveContext) built on a shared AgentContextBase, and updates AgentHandlers so each handler receives a narrowed context. Handler dispatch was made type-safe (switch on AgentEventEnum) to allow compile-time guarantees about available fields (e.g., message for onMessage), reducing runtime guards while preserving the original AgentContext as @deprecated for compatibility.
Affected areas
Key technical decisions
Testing
No new tests added; existing framework tests cover runtime behavior and the type changes were validated via typecheck in CI (PR author reports all tests pass and zero type errors). Package builds and new types are exported at package level.