Add generic SMS executor for flow-based SMS sending#2128
Merged
ThaminduDilshan merged 1 commit intoasgardeo:mainfrom Apr 6, 2026
Merged
Add generic SMS executor for flow-based SMS sending#2128ThaminduDilshan merged 1 commit intoasgardeo:mainfrom
ThaminduDilshan merged 1 commit intoasgardeo:mainfrom
Conversation
|
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:
📝 WalkthroughWalkthroughAdds SMS sending support: new Changes
Sequence Diagram(s)sequenceDiagram
participant SM as ServiceManager
participant NI as Notification.Init
participant SS as SMSSenderService
participant EI as Executor.Init
SM->>NI: Initialize(mux, jwtService)
NI->>SS: newSMSSenderService(senderMgtService)
NI-->>SM: (notificationMgr, otpService, smsSenderService, exporter, err)
SM->>EI: Initialize(..., smsSenderService, ...)
EI->>EI: Register SMSExecutor via newSMSExecutor(flowFactory, smsSenderService)
EI-->>SM: initialized
sequenceDiagram
participant Flow as FlowExecutor
participant SMSEx as SMSExecutor
participant Mgr as SenderManager
participant Prov as MessageClientProvider
participant Client as MessageClient
Flow->>SMSEx: Execute(ctx, mode=send)
alt missing recipient
SMSEx-->>Flow: ExecFailure (recipient required)
else recipient present
SMSEx->>Mgr: GetSender(senderID)
alt sender not found or wrong type
SMSEx-->>Flow: ExecFailure / Error
else sender valid
SMSEx->>Prov: GetMessageClient(sender)
alt provider error
SMSEx-->>Flow: Error
else client acquired
SMSEx->>Client: SendSMS({To, Body})
alt client returns client-error
SMSEx-->>Flow: ExecFailure (service error message)
else server/network error
SMSEx-->>Flow: Error ("SMS send failed")
else success
SMSEx-->>Flow: ExecComplete (smsSent=true)
end
end
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #2128 +/- ##
==========================================
- Coverage 89.85% 89.84% -0.01%
==========================================
Files 898 900 +2
Lines 58937 59050 +113
==========================================
+ Hits 52955 53055 +100
- Misses 4416 4428 +12
- Partials 1566 1567 +1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
backend/internal/notification/sms_sender_service.go (1)
64-66: Consider preserving the original error for observability.When
client.SendSMSfails, the original error is discarded. While returning a genericErrorInternalServerErroris appropriate for client-facing responses, the underlying error details are lost for debugging and observability.Consider logging the error before returning, or wrapping it for internal observability:
🔧 Optional improvement to preserve error context
- if err := client.SendSMS(common.SMSData{To: recipient, Body: body}); err != nil { - return &ErrorInternalServerError - } + if err := client.SendSMS(common.SMSData{To: recipient, Body: body}); err != nil { + log.GetLogger().Error("Failed to send SMS", log.Error(err), log.String("recipient", recipient)) + return &ErrorInternalServerError + }Note: Per repository conventions, server errors are typically logged at the handler layer. However, since the executor may not have visibility into this specific failure detail, adding a debug/error log here could aid troubleshooting.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/internal/notification/sms_sender_service.go` around lines 64 - 66, The call to client.SendSMS discards the original error and returns ErrorInternalServerError, losing observability; update the failure path in sms_sender_service.go (the block that calls client.SendSMS with common.SMSData{To: recipient, Body: body}) to preserve the underlying error by either logging it with the service/component logger at error/debug level or wrapping it (e.g., fmt.Errorf("send sms: %w", err)) before returning the external ErrorInternalServerError, ensuring the original err from client.SendSMS is not lost.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@backend/cmd/server/servicemanager.go`:
- Around line 204-206: Add user-facing documentation for the new SMSExecutor
flow executor: create or update a guide under docs/content/guides/ describing
SMSExecutor and its purpose, list required node configuration properties
senderId and messageBody with examples of expected formats, explain how
mobileNumber is resolved (check UserInputs first, then RuntimeData fallback),
and document failure semantics (no-op when SMS service is not configured and
return ExecFailure on client errors). Reference the executor by name
(SMSExecutor) and note that it is registered via
execRegistry/executor.Initialize so flow authors can add it to their node
configurations. Ensure the doc clearly states configuration keys and runtime
behavior for flow authors.
In `@backend/internal/flow/executor/sms_executor.go`:
- Around line 39-57: Add documentation for the new SMSExecutor utility flow node
introduced by newSMSExecutor/ExecutorNameSMSExecutor: describe required node
properties senderId and messageBody, explain the mobileNumber lookup order
(UserInputs first, then RuntimeData using userAttributeMobileNumber), and
document runtime behavior including the smsSent output and the no-op behavior
when smsSenderService is nil (no SMS provider configured). Update the
flow-executor guide under docs/content/guides/ (or add a dedicated page) to
include usage examples, expected node schema, and the failure/no-op semantics so
this user-facing change is fully documented before merging.
- Around line 21-29: The SMS executor currently treats many notification
failures as server errors and returns a generic message; update the failure
handling in sms_executor.go (within the SMSExecutor Execute/send path) to
classify by ServiceError.Type: if the error is a client-side notification error
(i.e., ServiceError.Type indicates a client error as defined in the notification
package / MNS-* range), treat it as a client failure and return the original
user-friendly ServiceError.Reason (preserve the client reason) instead of the
generic server message; adjust the same logic in the other failure branches
mentioned (the blocks around the current checks that only match specific MNS-*
values) so all notification-defined client errors (up to MNS-1015 and similar)
are detected via ServiceError.Type and handled as client errors.
---
Nitpick comments:
In `@backend/internal/notification/sms_sender_service.go`:
- Around line 64-66: The call to client.SendSMS discards the original error and
returns ErrorInternalServerError, losing observability; update the failure path
in sms_sender_service.go (the block that calls client.SendSMS with
common.SMSData{To: recipient, Body: body}) to preserve the underlying error by
either logging it with the service/component logger at error/debug level or
wrapping it (e.g., fmt.Errorf("send sms: %w", err)) before returning the
external ErrorInternalServerError, ensuring the original err from client.SendSMS
is not lost.
🪄 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: 0e524f2f-972b-4f08-bc7c-9db6176f2fbd
⛔ Files ignored due to path filters (1)
backend/tests/mocks/notification/notificationmock/SMSSenderServiceInterface_mock.gois excluded by!**/*_mock.go
📒 Files selected for processing (11)
backend/cmd/server/servicemanager.gobackend/internal/flow/common/constants.gobackend/internal/flow/executor/constants.gobackend/internal/flow/executor/init.gobackend/internal/flow/executor/sms_executor.gobackend/internal/flow/executor/sms_executor_test.gobackend/internal/notification/SMSSenderServiceInterface_mock_test.gobackend/internal/notification/init.gobackend/internal/notification/init_test.gobackend/internal/notification/sms_sender_service.gobackend/internal/notification/sms_sender_service_test.go
Dilusha-Madushan
force-pushed
the
feature/generic-sms-executor
branch
from
801c02b to
bc03c19
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
backend/internal/flow/executor/sms_executor.go (1)
32-121:⚠️ Potential issue | 🟠 Major🔴 Documentation Required: This PR introduces a user-facing change (e.g., new/modified API endpoint, configuration option, authentication flow, or SDK behavior) but does not include corresponding documentation updates under
docs/. Please update the relevant documentation before merging.**Please document the new
SMSExecutorflow node behavior: required node properties (senderId,messageBody), recipient resolution order (UserInputs.mobileNumberthenRuntimeData.mobileNumber), and runtime outputs/failure semantics (smsSent, no-op when SMS service is unavailable, andExecFailureon client errors).
Suggested location:docs/content/guides/(existing flow-executor guide or a dedicated SMS executor page).As per coding guidelines:
If ANY of the above are detected and the PR does NOT include corresponding updates under docs/, flag this as a major issue.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/internal/flow/executor/sms_executor.go` around lines 32 - 121, The PR adds a new SMSExecutor flow node (types smsExecutor and method executeSend) but lacks docs; add a documentation page or update the existing flow-executor guide under docs/content/guides/ describing: required node properties propertyKeySMSSenderID (senderId) and propertyKeySMSMessage (messageBody); recipient resolution order implemented by resolveRecipientMobile (UserInputs.mobileNumber then RuntimeData.mobileNumber); runtime outputs and semantics including common.DataSMSSent values (true/false), no-op behavior when smsSenderService is nil, and failure semantics (ExecFailure when recipient missing or when service returns a client error, and propagated error for other service errors). Include examples of node JSON config, expected runtime outputs, and troubleshooting notes.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@backend/internal/flow/executor/sms_executor.go`:
- Around line 32-121: The PR adds a new SMSExecutor flow node (types smsExecutor
and method executeSend) but lacks docs; add a documentation page or update the
existing flow-executor guide under docs/content/guides/ describing: required
node properties propertyKeySMSSenderID (senderId) and propertyKeySMSMessage
(messageBody); recipient resolution order implemented by resolveRecipientMobile
(UserInputs.mobileNumber then RuntimeData.mobileNumber); runtime outputs and
semantics including common.DataSMSSent values (true/false), no-op behavior when
smsSenderService is nil, and failure semantics (ExecFailure when recipient
missing or when service returns a client error, and propagated error for other
service errors). Include examples of node JSON config, expected runtime outputs,
and troubleshooting notes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 0e8115bd-7d37-41bb-89a4-2c1ffa480fa7
⛔ Files ignored due to path filters (1)
backend/tests/mocks/notification/notificationmock/SMSSenderServiceInterface_mock.gois excluded by!**/*_mock.go
📒 Files selected for processing (11)
backend/cmd/server/servicemanager.gobackend/internal/flow/common/constants.gobackend/internal/flow/executor/constants.gobackend/internal/flow/executor/init.gobackend/internal/flow/executor/sms_executor.gobackend/internal/flow/executor/sms_executor_test.gobackend/internal/notification/SMSSenderServiceInterface_mock_test.gobackend/internal/notification/init.gobackend/internal/notification/init_test.gobackend/internal/notification/sms_sender_service.gobackend/internal/notification/sms_sender_service_test.go
✅ Files skipped from review due to trivial changes (4)
- backend/internal/notification/init_test.go
- backend/internal/flow/executor/constants.go
- backend/internal/notification/SMSSenderServiceInterface_mock_test.go
- backend/internal/flow/executor/sms_executor_test.go
🚧 Files skipped from review as they are similar to previous changes (3)
- backend/internal/flow/executor/init.go
- backend/internal/notification/sms_sender_service.go
- backend/internal/notification/init.go
Dilusha-Madushan
force-pushed
the
feature/generic-sms-executor
branch
from
bc03c19 to
6eb023d
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
backend/internal/flow/executor/sms_executor.go (1)
40-57:⚠️ Potential issue | 🟠 Major🔴 Documentation Required: This PR introduces a user-facing change (e.g., new/modified API endpoint, configuration option, authentication flow, or SDK behavior) but does not include corresponding documentation updates under
docs/. Please update the relevant documentation before merging.Please document the new
SMSExecutorutility node, its requiredsenderIdandmessageBodyproperties, themobileNumberresolution order, and thesmsSent/ no-op behavior. Updating the flow-executor guide underdocs/content/guides/or adding a dedicated SMS executor page there would cover the missing docs.As per coding guidelines:
If ANY of the above are detected and the PR does NOT include corresponding updates under docs/, flag this as a major issue.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/internal/flow/executor/sms_executor.go` around lines 40 - 57, Add documentation for the new SMSExecutor utility node introduced by newSMSExecutor/smsExecutor: create or update a guide under docs/content/guides/ (or a dedicated SMS executor page) that documents the required senderId and messageBody properties, the mobileNumber resolution order (how userAttributeMobileNumber is resolved), the runtime behavior including smsSent event vs no-op when smsSenderService is nil, and example flow snippets showing configuration and expected outputs; ensure the docs reference the ExecutorNameSMSExecutor and the inputs/outputs used by the executor.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@backend/internal/flow/executor/constants.go`:
- Around line 92-97: The constants file only defines propertyKeySMSSenderID and
a package-level smsDefaultMessage, so flows cannot supply message content; add a
new constant propertyKeySMSMessageBody (e.g., propertyKeySMSMessageBody =
"messageBody") to backend/internal/flow/executor/constants.go and remove/avoid
relying on the hardcoded smsDefaultMessage for real messages; then update the
executor to resolve propertyKeySMSMessageBody in
backend/internal/flow/executor/sms_executor.go before calling SendSMS (fall back
to smsDefaultMessage only if the resolved property is empty) so flow authors can
provide per-message content.
In `@backend/internal/flow/executor/sms_executor.go`:
- Around line 103-107: The handler for service errors uses svcErr.Error which
can expose a terse/internal value; update the branch that detects
serviceerror.ClientErrorType to set execResp.FailureReason to the client-facing
text by using svcErr.ErrorDescription.DefaultValue with a safe fallback (e.g.,
if ErrorDescription or DefaultValue is empty, fall back to svcErr.Error or a
generic message) in the function handling the response (where svcErr is checked
and execResp is populated).
In `@backend/internal/notification/sms_sender_service.go`:
- Around line 47-65: SendSMS currently calls client.SendSMS without passing ctx
and collapses all provider errors into ErrorInternalServerError; update the call
chain so the context is propagated and provider errors can be classified: change
the usage of GetMessageClient/SendSMS so SendSMS receives the incoming ctx
(i.e., call client.SendSMS(ctx, common.SMSData{...})), update any message client
implementations/signatures as needed to accept context, and replace the blanket
return of &ErrorInternalServerError with error mapping that converts provider
validation or client-returned client errors into the repository's
ClientErrorType/ExecFailure responses while keeping true unexpected failures as
ErrorInternalServerError. Ensure you modify SendSMS, the call to client.SendSMS,
and any concrete client implementations returned by GetMessageClient to accept
and honor context.
---
Duplicate comments:
In `@backend/internal/flow/executor/sms_executor.go`:
- Around line 40-57: Add documentation for the new SMSExecutor utility node
introduced by newSMSExecutor/smsExecutor: create or update a guide under
docs/content/guides/ (or a dedicated SMS executor page) that documents the
required senderId and messageBody properties, the mobileNumber resolution order
(how userAttributeMobileNumber is resolved), the runtime behavior including
smsSent event vs no-op when smsSenderService is nil, and example flow snippets
showing configuration and expected outputs; ensure the docs reference the
ExecutorNameSMSExecutor and the inputs/outputs used by the executor.
🪄 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: 68ccc3ec-b587-4203-a304-400c67f74baf
⛔ Files ignored due to path filters (1)
backend/tests/mocks/notification/notificationmock/SMSSenderServiceInterface_mock.gois excluded by!**/*_mock.go
📒 Files selected for processing (11)
backend/cmd/server/servicemanager.gobackend/internal/flow/common/constants.gobackend/internal/flow/executor/constants.gobackend/internal/flow/executor/init.gobackend/internal/flow/executor/sms_executor.gobackend/internal/flow/executor/sms_executor_test.gobackend/internal/notification/SMSSenderServiceInterface_mock_test.gobackend/internal/notification/init.gobackend/internal/notification/init_test.gobackend/internal/notification/sms_sender_service.gobackend/internal/notification/sms_sender_service_test.go
🚧 Files skipped from review as they are similar to previous changes (4)
- backend/internal/flow/executor/init.go
- backend/internal/notification/init.go
- backend/internal/notification/init_test.go
- backend/internal/flow/executor/sms_executor_test.go
Dilusha-Madushan
force-pushed
the
feature/generic-sms-executor
branch
from
6eb023d to
b3e51b5
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
backend/internal/flow/executor/sms_executor.go (1)
60-117:⚠️ Potential issue | 🟠 Major🔴 Documentation Required: This PR introduces a user-facing change (e.g., new/modified API endpoint, configuration option, authentication flow, or SDK behavior) but does not include corresponding documentation updates under
docs/. Please update the relevant documentation before merging.**Please document the new
SMSExecutorflow node, thesenderIdconfiguration,mobileNumberresolution (UserInputsfirst, thenRuntimeData), thesmsSentoutput, the no-op behavior when no SMS sender service is configured, and the current fixed message-body limitation until#1928.docs/content/guides/is the most likely place for this.As per coding guidelines:
If ANY of the above are detected and the PR does NOT include corresponding updates under docs/, flag this as a major issue.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/internal/flow/executor/sms_executor.go` around lines 60 - 117, Add documentation for the new SMSExecutor flow node: describe smsExecutor/SMSExecutor behavior, the senderId node property (propertyKeySMSSenderID) configuration, how mobileNumber resolution works (resolveRecipientMobile: UserInputs first then RuntimeData), the smsSent output flag (common.DataSMSSent), that the executor is a no-op when smsSenderService is nil, and note the current fixed message body (smsDefaultMessage) limitation with a pointer to issue `#1928`; place this content under docs/content/guides/ and include examples for configuring senderId, user inputs vs runtime data for mobileNumber, and expected outputs.
🧹 Nitpick comments (2)
backend/internal/notification/sms_sender_service_test.go (1)
86-88: Match the resolved sender/request instead ofmock.Anything.These expectations only prove the calls happened. They won't catch regressions where
smsSenderServicepasses the wrong sender intoGetMessageClientor builds the wrong outbound SMS. Usingmock.MatchedByhere would make the suite validate the actual contract.Also applies to: 130-132
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/internal/notification/sms_sender_service_test.go` around lines 86 - 88, Replace the broad mock.Anything matchers with predicate matchers so the test validates the actual sender and outbound SMS payload: change the mm.EXPECT().SendSMS(mock.Anything) to mm.EXPECT().SendSMS(mock.MatchedBy(func(req /* type of outbound SMS request */) bool { return /* check fields (From/To/Body/other) match expected values */ })) and change suite.mockClientProvider.EXPECT().GetMessageClient(mock.Anything) to suite.mockClientProvider.EXPECT().GetMessageClient(mock.MatchedBy(func(sender /* sender type */) bool { return /* check sender ID/credentials/fields equal the resolved sender used by smsSenderService */ })). Apply the same replacement pattern for the second occurrence around the 130-132 block so both SendSMS and GetMessageClient expectations validate the actual objects instead of using mock.Anything.backend/internal/flow/executor/sms_executor.go (1)
32-33: Align the comments with the current implementation.These comments still say the executor reads the message body from node properties, but the code always sends
smsDefaultMessage. Please describe the current temporary behavior here so the file does not advertise a contract it does not implement.📝 Suggested comment update
-// smsExecutor sends an SMS message using the configured sender and message body from node properties. +// smsExecutor sends an SMS message using the configured sender and a temporary default message body. // When smsSenderService is nil, it completes as a no-op with smsSent=false. @@ -// executeSend resolves the recipient, sender ID, and message body from node properties, then sends the SMS. +// executeSend resolves the recipient and sender ID, then sends the SMS using the temporary default message body. // If the SMS sender service is not configured, it completes without sending (no-op).Based on learnings:
smsDefaultMessageis intentionally hardcoded for the initial SMSExecutor PR, and template-basedmessageBodysupport is deferred to issue#1928.Also applies to: 70-71
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/internal/flow/executor/sms_executor.go` around lines 32 - 33, Update the top-of-file comments to reflect the current behavior: state that smsExecutor always sends the hardcoded smsDefaultMessage (not a message read from node properties) and that when smsSenderService is nil it is a no-op returning smsSent=false; mention that template-based messageBody support is intentionally deferred to issue `#1928`. Reference smsExecutor, smsSenderService, and smsDefaultMessage so reviewers can locate the implementation to keep comments aligned with the code.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@backend/internal/flow/executor/sms_executor.go`:
- Around line 60-117: Add documentation for the new SMSExecutor flow node:
describe smsExecutor/SMSExecutor behavior, the senderId node property
(propertyKeySMSSenderID) configuration, how mobileNumber resolution works
(resolveRecipientMobile: UserInputs first then RuntimeData), the smsSent output
flag (common.DataSMSSent), that the executor is a no-op when smsSenderService is
nil, and note the current fixed message body (smsDefaultMessage) limitation with
a pointer to issue `#1928`; place this content under docs/content/guides/ and
include examples for configuring senderId, user inputs vs runtime data for
mobileNumber, and expected outputs.
---
Nitpick comments:
In `@backend/internal/flow/executor/sms_executor.go`:
- Around line 32-33: Update the top-of-file comments to reflect the current
behavior: state that smsExecutor always sends the hardcoded smsDefaultMessage
(not a message read from node properties) and that when smsSenderService is nil
it is a no-op returning smsSent=false; mention that template-based messageBody
support is intentionally deferred to issue `#1928`. Reference smsExecutor,
smsSenderService, and smsDefaultMessage so reviewers can locate the
implementation to keep comments aligned with the code.
In `@backend/internal/notification/sms_sender_service_test.go`:
- Around line 86-88: Replace the broad mock.Anything matchers with predicate
matchers so the test validates the actual sender and outbound SMS payload:
change the mm.EXPECT().SendSMS(mock.Anything) to
mm.EXPECT().SendSMS(mock.MatchedBy(func(req /* type of outbound SMS request */)
bool { return /* check fields (From/To/Body/other) match expected values */ }))
and change suite.mockClientProvider.EXPECT().GetMessageClient(mock.Anything) to
suite.mockClientProvider.EXPECT().GetMessageClient(mock.MatchedBy(func(sender /*
sender type */) bool { return /* check sender ID/credentials/fields equal the
resolved sender used by smsSenderService */ })). Apply the same replacement
pattern for the second occurrence around the 130-132 block so both SendSMS and
GetMessageClient expectations validate the actual objects instead of using
mock.Anything.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: d6a26436-016b-4ffb-8738-16d05abd4a13
⛔ Files ignored due to path filters (1)
backend/tests/mocks/notification/notificationmock/SMSSenderServiceInterface_mock.gois excluded by!**/*_mock.go
📒 Files selected for processing (11)
backend/cmd/server/servicemanager.gobackend/internal/flow/common/constants.gobackend/internal/flow/executor/constants.gobackend/internal/flow/executor/init.gobackend/internal/flow/executor/sms_executor.gobackend/internal/flow/executor/sms_executor_test.gobackend/internal/notification/SMSSenderServiceInterface_mock_test.gobackend/internal/notification/init.gobackend/internal/notification/init_test.gobackend/internal/notification/sms_sender_service.gobackend/internal/notification/sms_sender_service_test.go
🚧 Files skipped from review as they are similar to previous changes (6)
- backend/internal/flow/executor/init.go
- backend/internal/flow/common/constants.go
- backend/internal/notification/init_test.go
- backend/internal/notification/sms_sender_service.go
- backend/internal/flow/executor/sms_executor_test.go
- backend/internal/notification/init.go
Dilusha-Madushan
force-pushed
the
feature/generic-sms-executor
branch
from
b3e51b5 to
c9970b8
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
backend/internal/flow/executor/init.go (1)
47-47:⚠️ Potential issue | 🟠 MajorMissing docs update for newly registered
SMSExecutorflow capability.🔴 Documentation Required: This PR introduces a user-facing change (e.g., new/modified API endpoint, configuration option, authentication flow, or SDK behavior) but does not include corresponding documentation updates under
docs/. Please update the relevant documentation before merging.Please document the new
SMSExecutorutility node behavior (requiredsenderId, recipient resolution order,smsSentoutput/no-op semantics) indocs/content/guides/(or the existing flow-executor guide) before merge.As per coding guidelines: "If ANY of the above are detected and the PR does NOT include corresponding updates under
docs/, flag this as a major issue."Also applies to: 101-101
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/internal/flow/executor/init.go` at line 47, The PR added a new flow capability SMSExecutor but did not update docs; add documentation under docs/content/guides/ (or update the existing flow-executor guide) describing the SMSExecutor utility node: required configuration field senderId, how recipients are resolved (explicit recipient param -> flow context lookup -> default/group fallback), the semantics of the smsSent output (true on send, no-op behavior when SMS service disabled or rate-limited, and error conditions), and any auth/configuration steps for notification.SMSSenderServiceInterface; ensure examples and schema snippets for the node inputs/outputs are included so users can configure senderId and interpret smsSent.
🧹 Nitpick comments (1)
backend/internal/notification/sms_sender_service_test.go (1)
75-77: Prefer Twilio property-key constants in test fixtures.Using
common.TwilioPropKeyAccountSID,common.TwilioPropKeyAuthToken, andcommon.TwilioPropKeySenderIDhere will keep tests aligned with production constants and avoid drift.♻️ Suggested refactor
- createTestProperty("account_sid", "AC00112233445566778899aabbccddeeff", true), - createTestProperty("auth_token", "test-token", true), - createTestProperty("sender_id", "+15551234567", false), + createTestProperty(common.TwilioPropKeyAccountSID, "AC00112233445566778899aabbccddeeff", true), + createTestProperty(common.TwilioPropKeyAuthToken, "test-token", true), + createTestProperty(common.TwilioPropKeySenderID, "+15551234567", false),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/internal/notification/sms_sender_service_test.go` around lines 75 - 77, Replace hard-coded Twilio property key strings in the test fixtures with the production constants: use common.TwilioPropKeyAccountSID, common.TwilioPropKeyAuthToken, and common.TwilioPropKeySenderID when calling createTestProperty in sms_sender_service_test.go so the test uses the same keys as production and avoids drift.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@backend/internal/flow/executor/sms_executor.go`:
- Around line 70-71: The function comment for executeSend is inaccurate: it
claims the message body is resolved from node properties but the implementation
sends the smsDefaultMessage; update the comment on executeSend to state that it
resolves recipient and sender ID from node properties but uses smsDefaultMessage
as the message body, and retain the note that if the SMS sender service is not
configured the function completes as a no-op; reference executeSend and
smsDefaultMessage when editing the comment.
---
Duplicate comments:
In `@backend/internal/flow/executor/init.go`:
- Line 47: The PR added a new flow capability SMSExecutor but did not update
docs; add documentation under docs/content/guides/ (or update the existing
flow-executor guide) describing the SMSExecutor utility node: required
configuration field senderId, how recipients are resolved (explicit recipient
param -> flow context lookup -> default/group fallback), the semantics of the
smsSent output (true on send, no-op behavior when SMS service disabled or
rate-limited, and error conditions), and any auth/configuration steps for
notification.SMSSenderServiceInterface; ensure examples and schema snippets for
the node inputs/outputs are included so users can configure senderId and
interpret smsSent.
---
Nitpick comments:
In `@backend/internal/notification/sms_sender_service_test.go`:
- Around line 75-77: Replace hard-coded Twilio property key strings in the test
fixtures with the production constants: use common.TwilioPropKeyAccountSID,
common.TwilioPropKeyAuthToken, and common.TwilioPropKeySenderID when calling
createTestProperty in sms_sender_service_test.go so the test uses the same keys
as production and avoids drift.
🪄 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: 7829e374-a35a-4c43-baef-70a8e5035301
⛔ Files ignored due to path filters (1)
backend/tests/mocks/notification/notificationmock/SMSSenderServiceInterface_mock.gois excluded by!**/*_mock.go
📒 Files selected for processing (11)
backend/cmd/server/servicemanager.gobackend/internal/flow/common/constants.gobackend/internal/flow/executor/constants.gobackend/internal/flow/executor/init.gobackend/internal/flow/executor/sms_executor.gobackend/internal/flow/executor/sms_executor_test.gobackend/internal/notification/SMSSenderServiceInterface_mock_test.gobackend/internal/notification/init.gobackend/internal/notification/init_test.gobackend/internal/notification/sms_sender_service.gobackend/internal/notification/sms_sender_service_test.go
✅ Files skipped from review due to trivial changes (2)
- backend/internal/flow/common/constants.go
- backend/internal/notification/init_test.go
🚧 Files skipped from review as they are similar to previous changes (4)
- backend/cmd/server/servicemanager.go
- backend/internal/notification/init.go
- backend/internal/notification/sms_sender_service.go
- backend/internal/flow/executor/sms_executor_test.go
Dilusha-Madushan
force-pushed
the
feature/generic-sms-executor
branch
from
c9970b8 to
47e8d33
Compare
Dilusha-Madushan
force-pushed
the
feature/generic-sms-executor
branch
from
47e8d33 to
ed0bd85
Compare
Dilusha-Madushan
force-pushed
the
feature/generic-sms-executor
branch
2 times, most recently
from
d30fffa to
a8a92f5
Compare
Dilusha-Madushan
force-pushed
the
feature/generic-sms-executor
branch
from
a8a92f5 to
3295abe
Compare
Signed-off-by: Dilusha-Madushan <dilushamadushan9912@gmail.com>
Dilusha-Madushan
force-pushed
the
feature/generic-sms-executor
branch
from
3295abe to
4699cbd
Compare
ThaminduDilshan
approved these changes
Apr 6, 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
3 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.
Purpose
Adds a new generic SMSExecutor utility executor that enables flow designers to send arbitrary SMS messages at any point within a flow. This is distinct from the existing SMSOTPAuthExecutor, which is tightly coupled to OTP-based authentication. The new executor enables use cases such as sending a welcome SMS after registration, delivery notifications, or any other flow-triggered SMS communication.
Approach
SMSSenderService — a new service layer (SMSSenderServiceInterface / smsSenderService) introduced in the notification package. It abstracts SMS delivery by looking up the configured sender by ID, resolving the appropriate message client (Twilio, Vonage, custom webhook, etc.), and dispatching the message. This service is wired through notification.Initialize and injected into the executor at startup.
SMSExecutor — a new utility-type flow executor registered as SMSExecutor. It operates in send mode and reads two required properties from the node configuration:
senderId — the notification sender to use
messageBody — the SMS content to deliver
The recipient (mobileNumber) is resolved from UserInputs first, falling back to RuntimeData, allowing flexibility in where the phone number is collected within the flow. If no SMS sender service is configured, the executor completes gracefully as a no-op (smsSent=false) rather than failing the flow.
Error handling follows the existing executor pattern:
Client errors (invalid sender, bad recipient) → ExecFailure with a failure reason, allowing the flow to handle it gracefully
Server errors → propagated as a Go error to halt the flow
Related Issues
Summary by CodeRabbit