extend to email address list#1363
Conversation
Code Review by Qodo
1. Recipients not validated empty
|
| var recipients = args?.ToAddresses?.Where(x => !string.IsNullOrWhiteSpace(x)) ?? []; | ||
| var body = args?.Content; | ||
| var subject = args?.Subject; | ||
| var isNeedAttachments = args?.IsNeedAttachemnts ?? false; |
There was a problem hiding this comment.
1. Recipients not validated empty 📘 Rule violation ☼ Reliability
HandleEmailSenderFn.Execute defaults recipients to an empty collection when args/ToAddresses is null and then proceeds without validating that at least one recipient exists, so invalid boundary inputs aren’t rejected early. This can mask client/LLM input errors (e.g., missing or mis-shaped to_address_list) and lead to unpredictable downstream behavior or a late SMTP send failure instead of a clear validation error.
Agent Prompt
## Issue description
`HandleEmailSenderFn.Execute` currently accepts missing/empty boundary inputs by defaulting `recipients` to an empty collection and continuing execution, which can result in attempting to send an email with no recipients and failing late (SMTP/Mime creation) with a generic failure. Update the handler to validate required inputs at the boundary (at minimum: recipients; optionally: subject and body) and fail fast with a clear, predictable validation error/result returned to the caller, per compliance requirements.
## Issue Context
- `recipients` is computed with a null-coalescing default (e.g., `args?.ToAddresses ?? []`), so null/missing `to_address_list` is silently treated as “no recipients”.
- The PR changed input from `to_address` to `to_address_list`; the implementation filters null/whitespace and adds recipients in a loop, but never checks that any were actually added before calling the SMTP send.
- When the LLM omits or mis-shapes `to_address_list`, this becomes a runtime send attempt failure rather than a clear validation error.
## Fix Focus Areas
- src/Plugins/BotSharp.Plugin.EmailHandler/Functions/HandleEmailSenderFn.cs[31-62]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| message.Content = response; | ||
|
|
||
| _logger.LogWarning($"Email successfully send over to {recipient}. Email Subject: {subject} [{response}]"); | ||
| _logger.LogWarning($"Email successfully send over to {string.Join(",", recipients)}. Email Subject: {subject} [{response}]"); |
There was a problem hiding this comment.
2. Pii in interpolated log 📘 Rule violation ⛨ Security
The success log uses string interpolation and includes recipient email addresses and subject, which can leak sensitive/PII into operational logs and is not structured for queryable context. It also logs a success path at Warning level, which is not an appropriate severity.
Agent Prompt
## Issue description
A success log message is written via string interpolation and includes recipient email addresses and the email subject. This can expose PII/sensitive data and is not structured logging; additionally, `LogWarning` is used for a successful operation.
## Issue Context
The code logs `string.Join(",", recipients)` and `subject` in a warning-level message.
## Fix Focus Areas
- src/Plugins/BotSharp.Plugin.EmailHandler/Functions/HandleEmailSenderFn.cs[61-61]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
PR Summary by Qodo
Support sending email to multiple recipients
✨ Enhancement⚙️ Configuration changes🕐 10-20 MinutesWalkthroughs
Description
Diagram
graph TD A["FunctionArgs JSON"] --> B["LlmContextIn (to_address_list)"] --> C["HandleEmailSenderFn"] --> D["MimeMessage (To: many)"] --> E{{"SMTP server"}}High-Level Assessment
The following are alternative approaches to this PR:
1. Backward-compatible input (accept both keys)
to_addressto_address_listas canonical2. Polymorphic `to_address` (string or array)
Recommendation: Keeping an explicit
to_address_listis a good, strongly-typed contract. Consider short-term backward compatibility by also accepting the legacyto_addressfield (and mapping it into the list) to prevent breaking existing function callers; then deprecate and remove after clients migrate.File Changes
Enhancement (2)
Other (1)