Conversation
✅ Deploy Preview for dashboard-v2-novu-staging canceled.
|
Contributor
📝 WalkthroughWalkthroughThis PR consolidates agent-related types and enums by migrating local definitions to the Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 3✅ 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 |
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: 1
🧹 Nitpick comments (1)
apps/api/src/app/agents/dtos/agent-reply-payload.dto.ts (1)
24-35: Consider enforcing strict discriminator exclusivity forSignalDto.Current validation enforces required fields per
type, but it still accepts mixed-shape payloads. If you want strict discriminated-union behavior, reject incompatible fields for each branch.Optional hardening diff
validate(signal: SignalDto): boolean { if (!signal?.type) return false; if (signal.type === 'metadata') { - return typeof signal.key === 'string' && signal.key.length > 0 && signal.value !== undefined; + return ( + typeof signal.key === 'string' && + signal.key.trim().length > 0 && + signal.value !== undefined && + signal.workflowId === undefined + ); } if (signal.type === 'trigger') { - return typeof signal.workflowId === 'string' && signal.workflowId.length > 0; + return ( + typeof signal.workflowId === 'string' && + signal.workflowId.trim().length > 0 && + signal.key === undefined && + signal.value === undefined + ); } return false; }Also applies to: 166-166
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/api/src/app/agents/dtos/agent-reply-payload.dto.ts` around lines 24 - 35, The validate method for SignalDto currently checks required fields for each signal type but allows extra incompatible fields; update validate(signal: SignalDto) to enforce strict discriminator exclusivity by rejecting signals that contain fields not allowed for their type (e.g., when signal.type === 'metadata' ensure workflowId is undefined or absent and when signal.type === 'trigger' ensure key/value are undefined or absent), keep the existing checks for required fields (key/value present for 'metadata', workflowId present for 'trigger') and return false if any incompatible fields are present.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.source:
- Line 1: The .source submodule pointer was advanced to commit
de44134b129e41f24dc931e96347a303f4ea5260 which does not exist on the enterprise
repo's next branch; ensure the enterprise PR novuhq/packages-enterprise#445 (or
that specific commit) is merged into the packages-enterprise/next branch before
merging this PR, or else update the .source pointer back to the current
packages-enterprise/next commit; locate the .source entry in the repo, either
merge/rebase the enterprise branch so the SHA is present or reset the submodule
pointer to the matching next-branch commit to resolve the blocking dependency.
---
Nitpick comments:
In `@apps/api/src/app/agents/dtos/agent-reply-payload.dto.ts`:
- Around line 24-35: The validate method for SignalDto currently checks required
fields for each signal type but allows extra incompatible fields; update
validate(signal: SignalDto) to enforce strict discriminator exclusivity by
rejecting signals that contain fields not allowed for their type (e.g., when
signal.type === 'metadata' ensure workflowId is undefined or absent and when
signal.type === 'trigger' ensure key/value are undefined or absent), keep the
existing checks for required fields (key/value present for 'metadata',
workflowId present for 'trigger') and return false if any incompatible fields
are present.
🪄 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: 185399af-28e6-4d29-8cac-1ec4ad33b3e8
📒 Files selected for processing (9)
.sourceapps/api/src/app/agents/dtos/agent-event.enum.tsapps/api/src/app/agents/dtos/agent-reply-payload.dto.tsapps/api/src/app/agents/services/agent-inbound-handler.service.tsapps/api/src/app/agents/services/bridge-executor.service.tsapps/api/src/app/agents/usecases/handle-agent-reply/handle-agent-reply.command.tspackages/framework/src/index.tspackages/framework/src/resources/agent/index.tspackages/shared/src/consts/providers/conversational-providers.ts
| @@ -1 +1 @@ | |||
| Subproject commit b7ead722c175bc6d14d5186b58b8cae86df829d6 | |||
| Subproject commit de44134b129e41f24dc931e96347a303f4ea5260 | |||
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
ENTERPRISE_REPO="novuhq/packages-enterprise"
ENTERPRISE_PR="445"
PINNED_SHA="de44134b129e41f24dc931e96347a303f4ea5260"
echo "1) Check enterprise PR state/base branch"
gh pr view "$ENTERPRISE_PR" --repo "$ENTERPRISE_REPO" \
--json number,state,mergedAt,baseRefName,headRefName,mergeCommit,url \
| jq .
echo "2) Check pinned SHA exists in remote"
git ls-remote "https://github.com/${ENTERPRISE_REPO}.git" "$PINNED_SHA"
echo "3) Check pinned SHA is ancestor of 'next'"
tmp_dir="$(mktemp -d)"
trap 'rm -rf "$tmp_dir"' EXIT
git -C "$tmp_dir" init -q
git -C "$tmp_dir" remote add origin "https://github.com/${ENTERPRISE_REPO}.git"
git -C "$tmp_dir" fetch --depth=300 origin next "$PINNED_SHA"
NEXT_COMMIT="$(git -C "$tmp_dir" rev-parse FETCH_HEAD~1 2>/dev/null || true)"
# Re-fetch to ensure local refs are explicit
git -C "$tmp_dir" fetch --depth=300 origin next:refs/remotes/origin/next "$PINNED_SHA:$PINNED_SHA" >/dev/null 2>&1 || true
if git -C "$tmp_dir" merge-base --is-ancestor "$PINNED_SHA" refs/remotes/origin/next; then
echo "OK: pinned SHA is contained in origin/next"
else
echo "FAIL: pinned SHA is NOT contained in origin/next"
exit 1
fiRepository: novuhq/novu
Length of output: 212
🏁 Script executed:
rg "packages-enterprise" --type md --type txt --type jsonRepository: novuhq/novu
Length of output: 37
🏁 Script executed:
# Check git configuration or any CI files that reference the enterprise repo
fd -type f \( -name "*.yml" -o -name "*.yaml" -o -name "*.json" -o -name "*.md" \) | xargs rg -l "enterprise" | head -20Repository: novuhq/novu
Length of output: 1206
🏁 Script executed:
# Check if .gitmodules or similar config exists
find . -maxdepth 2 -name ".gitmodules" -o -name ".sourcemap" -o -name ".source*"Repository: novuhq/novu
Length of output: 78
🏁 Script executed:
cat .gitmodulesRepository: novuhq/novu
Length of output: 148
🏁 Script executed:
cat .sourceRepository: novuhq/novu
Length of output: 83
🏁 Script executed:
# Check the git history of .source to understand this change
git log --oneline -10 .source 2>/dev/null || echo "No git history available"Repository: novuhq/novu
Length of output: 935
🏁 Script executed:
# The .source is a git submodule pointing to packages-enterprise
# Try to verify the SHA using git commands directly (no GitHub API)
PINNED_SHA="de44134b129e41f24dc931e96347a303f4ea5260"
ENTERPRISE_REPO_URL="git@github.com:novuhq/packages-enterprise.git"
echo "Attempting to verify SHA exists in enterprise repo..."
# Create a temporary directory for checking
tmp_dir=$(mktemp -d)
trap 'rm -rf "$tmp_dir"' EXIT
# Try SSH first (standard for this repo)
if git -C "$tmp_dir" clone --bare --depth=500 "$ENTERPRISE_REPO_URL" enterprise.git 2>&1 | head -20; then
echo "--- Checking if SHA exists in cloned repo ---"
git -C "$tmp_dir/enterprise.git" cat-file -t "$PINNED_SHA" 2>&1 || echo "SHA not found in shallow clone"
echo "--- Checking branches ---"
git -C "$tmp_dir/enterprise.git" branch -r | head -10
else
echo "Clone failed, trying HTTPS fallback..."
HTTPS_URL="https://github.com/novuhq/packages-enterprise.git"
if git -C "$tmp_dir" clone --bare --depth=500 "$HTTPS_URL" enterprise.git 2>&1 | head -20; then
echo "--- Checking if SHA exists ---"
git -C "$tmp_dir/enterprise.git" cat-file -t "$PINNED_SHA" 2>&1 || echo "SHA not found"
fi
fiRepository: novuhq/novu
Length of output: 672
🏁 Script executed:
# Also check if we can use ls-remote to verify the SHA exists
git ls-remote --heads --tags "https://github.com/novuhq/packages-enterprise.git" | grep -E "(next|de44134b)" || echo "Could not verify via ls-remote"Repository: novuhq/novu
Length of output: 213
🏁 Script executed:
cd .source && git log --oneline next | grep de44134b || echo "SHA not in next"Repository: novuhq/novu
Length of output: 70
Subproject SHA must be merged into packages-enterprise/next before this PR can merge.
Line 1 updates the .source submodule pointer to de44134b129e41f24dc931e96347a303f4ea5260. This commit is not present in the next branch of the enterprise repository, but your PR explicitly states that novuhq/packages-enterprise#445 must merge first. This is a blocking dependency: ensure that commit is merged into next before proceeding with this merge.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.source at line 1, The .source submodule pointer was advanced to commit
de44134b129e41f24dc931e96347a303f4ea5260 which does not exist on the enterprise
repo's next branch; ensure the enterprise PR novuhq/packages-enterprise#445 (or
that specific commit) is merged into the packages-enterprise/next branch before
merging this PR, or else update the .source pointer back to the current
packages-enterprise/next commit; locate the .source entry in the repo, either
merge/rebase the enterprise branch so the SHA is present or reset the submodule
pointer to the matching next-branch commit to resolve the blocking dependency.
…rce of truth Eliminate duplicate type definitions across packages/framework and apps/api. The framework package now owns all agent protocol types (AgentBridgeRequest, AgentMessage, AgentConversation, Signal, etc.) and the API imports from it instead of maintaining local copies. Adds discriminated union validation for SignalDto and exports all protocol types from the framework's public API. Made-with: Cursor
ChmaraX
force-pushed
the
refactor/consolidate-agent-protocol-types
branch
from
70f6bed to
a9ecde0
Compare
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 (2)
apps/api/src/app/agents/services/bridge-executor.service.ts (2)
186-203:⚠️ Potential issue | 🟡 MinorInsert the required blank line before these
returns.The new/updated returns in these methods don't follow the repo rule.
As per coding guidelines "Include a blank line before every
returnstatement".Also applies to: 232-240, 244-245, 248-257, 261-266, 270-278
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/api/src/app/agents/services/bridge-executor.service.ts` around lines 186 - 203, Insert a blank line immediately before the return statement that constructs and returns the event object (the one returning an object with keys version, timestamp, deliveryId, event, agentId, replyUrl, conversationId, integrationIdentifier, message mapped via mapMessage, conversation via mapConversation, subscriber via mapSubscriber, history via mapHistory, platform, platformContext, action, reaction via mapReaction) so it follows the repo rule to have a blank line before every return; apply the same fix to the other return statements in this file that build similar response objects (the other methods that call mapMessage/mapConversation/mapSubscriber/mapHistory/mapReaction and return platform/action/reaction payloads) to ensure each return is preceded by a blank line.
269-278:⚠️ Potential issue | 🟠 MajorValidate
signalDatabefore forwarding to framework.
mapHistoryforwards persistedsignalDatadirectly without validation. The DTO validator (IsValidSignal) only protects inbound payloads; legacy database rows or other code paths can store malformed signals without validation. The database schema allowsMixedtypes, providing no enforcement. Additionally, the codebase storesresolve-type signals that are not part of the framework'sSignalunion (which defines onlymetadataandtrigger). Revalidate or normalizesignalDatahere to ensure outbound history payloads match the expected contract.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/api/src/app/agents/services/bridge-executor.service.ts` around lines 269 - 278, The mapHistory function currently forwards persisted signalData directly; update mapHistory to revalidate and normalize activity.signalData before assigning it to AgentHistoryEntry.signalData so outbound history matches the framework Signal union. Implement or call a validator/normalizer (e.g., isValidSignal or normalizeSignal) inside mapHistory: if signalData matches the framework Signal shape (only metadata or trigger) include it, if it's a legacy "resolve" or contains extra keys then transform it into an allowed Signal form (or drop/return undefined) and ensure any invalid shapes are omitted; reference the mapHistory function, AgentHistoryEntry.signalData, the existing IsValidSignal DTO/validator, and the framework Signal union when locating where to add this check.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@apps/api/src/app/agents/services/bridge-executor.service.ts`:
- Around line 186-203: Insert a blank line immediately before the return
statement that constructs and returns the event object (the one returning an
object with keys version, timestamp, deliveryId, event, agentId, replyUrl,
conversationId, integrationIdentifier, message mapped via mapMessage,
conversation via mapConversation, subscriber via mapSubscriber, history via
mapHistory, platform, platformContext, action, reaction via mapReaction) so it
follows the repo rule to have a blank line before every return; apply the same
fix to the other return statements in this file that build similar response
objects (the other methods that call
mapMessage/mapConversation/mapSubscriber/mapHistory/mapReaction and return
platform/action/reaction payloads) to ensure each return is preceded by a blank
line.
- Around line 269-278: The mapHistory function currently forwards persisted
signalData directly; update mapHistory to revalidate and normalize
activity.signalData before assigning it to AgentHistoryEntry.signalData so
outbound history matches the framework Signal union. Implement or call a
validator/normalizer (e.g., isValidSignal or normalizeSignal) inside mapHistory:
if signalData matches the framework Signal shape (only metadata or trigger)
include it, if it's a legacy "resolve" or contains extra keys then transform it
into an allowed Signal form (or drop/return undefined) and ensure any invalid
shapes are omitted; reference the mapHistory function,
AgentHistoryEntry.signalData, the existing IsValidSignal DTO/validator, and the
framework Signal union when locating where to add this check.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 8dda9e14-7aeb-4c9c-89a7-a5526cddea17
📒 Files selected for processing (8)
apps/api/src/app/agents/dtos/agent-event.enum.tsapps/api/src/app/agents/dtos/agent-reply-payload.dto.tsapps/api/src/app/agents/services/agent-inbound-handler.service.tsapps/api/src/app/agents/services/bridge-executor.service.tsapps/api/src/app/agents/usecases/handle-agent-reply/handle-agent-reply.command.tspackages/framework/src/index.tspackages/framework/src/resources/agent/index.tspackages/shared/src/consts/providers/conversational-providers.ts
✅ Files skipped from review due to trivial changes (2)
- packages/framework/src/index.ts
- packages/framework/src/resources/agent/index.ts
🚧 Files skipped from review as they are similar to previous changes (5)
- apps/api/src/app/agents/usecases/handle-agent-reply/handle-agent-reply.command.ts
- packages/shared/src/consts/providers/conversational-providers.ts
- apps/api/src/app/agents/dtos/agent-event.enum.ts
- apps/api/src/app/agents/services/agent-inbound-handler.service.ts
- apps/api/src/app/agents/dtos/agent-reply-payload.dto.ts
This was referenced Apr 17, 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.
Summary
BridgeMessage,BridgeConversation,BridgeSubscriber,BridgeHistoryEntry,BridgeAction,BridgePlatformContext, and the localAgentBridgeRequestinterface in the API were structurally identical to types already defined inpackages/framework. Replaced them all with imports from@novu/framework.AgentBridgeRequest,AgentMessage,AgentConversation,AgentSubscriber,AgentHistoryEntry,AgentAction,AgentReaction,AgentPlatformContext,Signal,MetadataSignal,TriggerSignal,ReplyContent, andAgentEventEnumare now part of the public API of@novu/framework.IsValidSignalcustom validator ensuresmetadatasignals havekey+valueandtriggersignals haveworkflowId, enforced at the DTO layer.PopulateConversationParticipantsservice.Architecture
graph TD subgraph "Source of Truth" FW["packages/framework<br/>Agent protocol types"] end subgraph "Cross-cutting" SH["packages/shared<br/>Feature flags, permissions, enums"] end subgraph "Persistence" DAL["libs/dal<br/>Entities + DB enums"] end subgraph "API Layer" API["apps/api<br/>DTOs, commands, services"] end FW -->|"protocol types<br/>(import)"| API SH -->|"flags, permissions"| API DAL -->|"entities"| API FW -->|"SDK types"| USER["User Code"] SH -->|"enums"| DASH["apps/dashboard"]Enterprise PR
https://github.com/novuhq/packages-enterprise/pull/445 — merge first
Test plan
pnpm buildpasses forpackages/frameworkandapps/apikey)Made with Cursor