[codex] Add 508 members newsletter sync#332
Conversation
ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (26)
✅ Files skipped from review due to trivial changes (3)
🚧 Files skipped from review as they are similar to previous changes (13)
📝 WalkthroughWalkthroughAdds Brevo and Keila newsletter provider clients, a NewsletterSyncProcessor with CRM-aware orchestration, settings and environment configuration, dashboard API trigger with periodic scheduling, worker job with email redaction, mailbox provisioning integration across Discord and tooling flows, admin dashboard action UI, and comprehensive unit test coverage. ChangesNewsletter Sync End-to-End Implementation
Sequence DiagramsequenceDiagram
participant AdminUI as Admin Dashboard
participant API as apps/api
participant Queue as Job Queue
participant Worker as apps/worker
participant Processor as NewsletterSyncProcessor
participant Brevo as Brevo API
participant Keila as Keila API
AdminUI->>API: POST /dashboard/api/sync/newsletters
API->>Queue: enqueue sync_508_members_newsletters_job
API->>Audit: record newsletter.508_members_sync event
Queue->>Worker: deliver job by name
Worker->>Processor: NewsletterSyncProcessor.sync_508_members()
Processor->>Brevo: add_contact_to_list / list lookup
Processor->>Keila: upsert_active_contact
Processor-->>Worker: result (emails masked)
Worker-->>API: audit/event already recorded at enqueue
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3c1183a9be
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| brevo_api_key: str | None = None | ||
| brevo_api_base_url: str = "https://api.brevo.com/v3" | ||
| brevo_api_timeout_seconds: float = 20.0 | ||
| brevo_newsletter_list_id: int | None = Field(default=None, ge=1) |
There was a problem hiding this comment.
When .env is copied from the updated .env.example, BREVO_NEWSLETTER_LIST_ID= is present but blank; pydantic-settings does not coerce an empty string to None for an int | None, so constructing SharedSettings fails before the optional Brevo sync can simply remain disabled. The existing optional numeric Docuseal setting has a mode="before" normalizer for this case; this new field needs the same treatment (or equivalent env_ignore_empty) so local/dev deployments with the documented blank value still start.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Pull request overview
Adds best-effort Brevo newsletter subscription during mailbox/account provisioning across the agent tools and Discord bot flows, with shared configuration and a shared Brevo API client.
Changes:
- Introduces a shared
BrevoClientplus settings/runtime-config wiring forBREVO_API_KEYandBREVO_NEWSLETTER_LIST_ID. - Subscribes both the 508 mailbox email and backup email to a Brevo list during
/create-mailboxand/create-user-accounts(non-blocking, warning surfaced). - Extends unit tests and docs/env examples to cover the new integration.
Reviewed changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
packages/shared/src/five08/clients/brevo.py |
Adds shared Brevo API wrapper for create-or-update contact + list subscription. |
packages/shared/src/five08/settings.py |
Exposes Brevo settings in shared BaseSettings. |
packages/shared/src/five08/agent/tools.py |
Adds Brevo runtime config + newsletter subscription in mailbox provisioning tool results. |
apps/discord_bot/src/five08/discord_bot/cogs/migadu.py |
Subscribes created mailbox + backup email to Brevo after Migadu provisioning; surfaces result in embeds/audit metadata. |
apps/discord_bot/src/five08/discord_bot/cogs/crm.py |
Subscribes mailbox + backup email to Brevo during user-account provisioning; surfaces result in messages/audit metadata. |
tests/unit/test_brevo_client.py |
Adds unit tests for BrevoClient request payload and error handling. |
tests/unit/test_shared_settings.py |
Extends shared settings/runtime config test coverage for Brevo fields. |
tests/unit/test_migadu_create_mailbox.py |
Asserts newsletter sync hook is invoked in mailbox command flow. |
tests/unit/test_crm_create_sso_user.py |
Asserts newsletter sync hook is invoked and success message includes newsletter status. |
tests/unit/test_agent_gateway.py |
Adds Brevo fake + assertions for agent tool subscription behavior and result metadata. |
ENVIRONMENT.md |
Documents optional Brevo newsletter sync env vars and behavior. |
apps/discord_bot/README.md |
Documents Brevo newsletter sync behavior for /create-mailbox. |
.env.example |
Adds Brevo env var placeholders. |
Comments suppressed due to low confidence (1)
apps/discord_bot/src/five08/discord_bot/cogs/crm.py:8632
exc.newsletter_erroris interpolated into a Discord message inside backticks without sanitization/truncation. If the underlying Brevo error contains backticks/control characters or is long (e.g., includes an HTTP body), this can break formatting or exceed Discord limits. Sanitize/truncate the newsletter error before buildingnewsletter_line.
self._audit_command_safe(
interaction=interaction,
action="crm.create_user_accounts",
result="error",
metadata={
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| brevo_api_key: str | None = None | ||
| brevo_api_base_url: str = "https://api.brevo.com/v3" | ||
| brevo_api_timeout_seconds: float = 20.0 | ||
| brevo_newsletter_list_id: int | None = Field(default=None, ge=1) |
| logger.warning( | ||
| "Failed to add %s to Brevo newsletter list: %s", | ||
| normalized_email, | ||
| exc, | ||
| ) | ||
| errors.append(f"{normalized_email}: {exc}") | ||
| return "; ".join(errors) if errors else None |
| except (BrevoAPIError, ValueError) as exc: | ||
| logger.warning( | ||
| "Failed to add %s to Brevo newsletter list: %s", | ||
| normalized_email, | ||
| exc, | ||
| ) | ||
| errors.append(f"{normalized_email}: {exc}") | ||
| return "; ".join(errors) if errors else None |
| assert runtime_config.brevo_api_key == "brevo-key" | ||
| assert runtime_config.brevo_newsletter_list_id == 4 | ||
|
|
||
|
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6780a019e4
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if client is None: | ||
| return "BREVO_API_KEY is not configured." |
There was a problem hiding this comment.
When BREVO_API_KEY is intentionally unset, the documented behavior is that newsletter sync is optional, but this return value is propagated as newsletter_error, so successful /create-mailbox and /create-user-accounts flows display “Brevo subscription failed: BREVO_API_KEY is not configured” and audit newsletter_subscribed=false. In deployments that do not configure Brevo, every mailbox/account creation now looks partially failed even though sync should simply be skipped unless the key is configured.
Useful? React with 👍 / 👎.
michaelmwu
changed the title
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e6dce19025
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| except EspoAPIError: | ||
| return None |
There was a problem hiding this comment.
When EspoCRM is configured but list_contacts raises, this returns None, and the caller treats that the same as “no blocked CRM contact” because _is_crm_blocked(None) is false. During a transient CRM outage or bad CRM credentials, the weekly newsletter sync will therefore subscribe every non-excluded Migadu mailbox/recovery email, including contacts that would otherwise be skipped as inactive, rejected, or blocked; the lookup error should skip the mailbox or abort the sync instead of failing open.
Useful? React with 👍 / 👎.
| newsletter_line = ( | ||
| f"\nNewsletter: subscription warning: `{exc.newsletter_error}`" | ||
| if exc.newsletter_error | ||
| else "\nNewsletter: added mailbox and backup email." | ||
| ) |
| message_lines.append( | ||
| "Newsletter: subscription warning: " | ||
| f"`{result.mailbox.newsletter_error}`" | ||
| ) |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 224a385aba
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if existing is not None and ( | ||
| bool(existing.get("emailBlacklisted")) | ||
| or bool(existing.get("smsBlacklisted")) | ||
| or str(existing.get("status") or "").strip().casefold() | ||
| in PROVIDER_SUPPRESSED_STATUSES | ||
| ): | ||
| return "skipped_provider_suppressed" |
There was a problem hiding this comment.
When a Brevo contact has unsubscribed from only the 508 members list, Brevo returns that list ID in listUnsubscribed while emailBlacklisted remains false. This suppression check does not inspect listUnsubscribed, so the code falls through and calls add_contact_to_list, which can re-add or repeatedly attempt to re-add contacts who explicitly opted out of this list, contrary to the new “does not re-add provider-suppressed contacts” behavior.
Useful? React with 👍 / 👎.
| def _response_body_excerpt(body: object) -> str: | ||
| """Return a bounded response-body excerpt for persisted/logged errors.""" | ||
| text = " ".join(str(body or "").split()) | ||
| if len(text) <= ERROR_BODY_MAX_LENGTH: | ||
| return text | ||
| return f"{text[:ERROR_BODY_MAX_LENGTH]}..." |
| def _response_body_excerpt(body: object) -> str: | ||
| """Return a bounded response-body excerpt for persisted/logged errors.""" | ||
| text = " ".join(str(body or "").split()) | ||
| if len(text) <= ERROR_BODY_MAX_LENGTH: | ||
| return text | ||
| return f"{text[:ERROR_BODY_MAX_LENGTH]}..." |
| def _response_body_excerpt(body: object) -> str: | ||
| """Return a bounded response-body excerpt for persisted/logged errors.""" | ||
| text = " ".join(str(body or "").split()) | ||
| if len(text) <= ERROR_BODY_MAX_LENGTH: | ||
| return text | ||
| return f"{text[:ERROR_BODY_MAX_LENGTH]}..." |
| if failed and isinstance(failures, list) and failures: | ||
| detail = "; ".join( | ||
| f"{item.get('email')}: {item.get('error')}" | ||
| for item in failures[:3] | ||
| if isinstance(item, dict) | ||
| ) | ||
| messages.append(f"{provider_name} failed for {failed} contact(s): {detail}") |
| "job_type": "sync_508_members_newsletters_job", | ||
| "reason": "dashboard", | ||
| "idempotency_key_pattern": "newsletter-sync:508-members:dashboard:<timestamp>", | ||
| }, |
| import base64 | ||
| import logging | ||
| import re | ||
| from datetime import datetime, timezone | ||
| from email import message_from_bytes |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a4252e5e38
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
|
||
|
|
||
| DOCUSEAL_COMPLETED_AT_UTC_FORMAT = "%Y-%m-%d %H:%M:%S" | ||
| EMAIL_PATTERN = re.compile(r"(?<![\w.+-])[\w.+-]+@[\w.-]+\.[A-Za-z]{2,}(?![\w.-])") |
There was a problem hiding this comment.
When a Brevo or Keila per-contact lookup fails at the request layer, requests can include the URL path in the exception (for example /contacts/jane%40example.com); the new worker-side masking only matches raw @ email strings, so these encoded mailbox addresses can still be persisted in newsletter job results via provider failures[].error. Fresh evidence since the earlier raw-email masking comment is that provider lookup URLs encode the contact email before the exception reaches this regex, so the current pattern will not redact it.
Useful? React with 👍 / 👎.
| normalized_email = email.strip().lower() | ||
| if not normalized_email or normalized_email.count("@") != 1: | ||
| raise ValueError("Brevo contact email must be a full email address.") |
| normalized_email = email.strip().lower() | ||
| if not normalized_email or normalized_email.count("@") != 1: | ||
| raise ValueError("Brevo contact email must be a full email address.") |
| normalized_email = email.strip().lower() | ||
| if not normalized_email or normalized_email.count("@") != 1: | ||
| raise ValueError("Keila contact email must be a full email address.") |
| normalized_email = email.strip().lower() | ||
| if not normalized_email or normalized_email.count("@") != 1: | ||
| raise ValueError("Keila contact email must be a full email address.") |
| def ensure_contact(self, contact: NewsletterContact) -> str: | ||
| """Ensure contact exists, returning added/updated/already/skipped.""" |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 19a831cec4
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| payload["data"] = { | ||
| **(existing_data if isinstance(existing_data, dict) else {}), | ||
| **(data or {}), |
There was a problem hiding this comment.
When an existing Keila contact already has data.audiences for other segments, this merge still lets the newsletter payload from KeilaNewsletterProvider overwrite that list with only ['508_members']; fresh evidence after the earlier data-preservation concern is that the current spread order preserves top-level keys but replaces the nested audience array. A member sync would silently drop the contact's other Keila audience tags, so union the existing and new audiences before PATCHing.
Useful? React with 👍 / 👎.
![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
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/api/src/five08/backend/api.py`:
- Around line 6243-6299: The current implementation sends the email via
send_onboarding_email_message before calling
_mark_dashboard_onboarding_email_sent to record the send, creating a race
condition where duplicate emails can be sent if the function is retried between
a successful SMTP send and the marker update. Refactor this by moving the marker
reservation or update to occur before the SMTP send operation. Check if the
marker already exists at the beginning of the logic after validation, and only
proceed with send_onboarding_email_message if the marker reservation succeeds.
This makes the operation idempotent so retries after a successful SMTP send will
be safely skipped.
- Around line 6372-6387: The audit metadata being passed to
_audit_dashboard_onboarding_email() contains raw email addresses in the
recipient_email and reply_to_email fields, which violates PII safety practices.
Either mask these email addresses using the existing mask_email() helper
function before including them in the metadata dictionary, or omit these fields
entirely and rely on the contact_id field for traceability. Choose the approach
that best balances debugging needs with privacy requirements.
In `@packages/shared/src/five08/settings.py`:
- Line 90: The newsletter_sync_excluded_mailboxes field in settings.py is
initialized with an empty string default, which means no mailboxes are excluded
by default. This is unsafe because downstream sync logic directly consumes this
value to determine which mailboxes to skip, potentially exposing system and
service mailbox addresses to external providers. Replace the empty string
default with a safe default value that includes common system or service
mailboxes that should be excluded from newsletter synchronization.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: c9c56d89-0c26-46a3-831a-222c33d779bb
📒 Files selected for processing (26)
.env.exampleENVIRONMENT.mdapps/admin_dashboard/src/main.tsxapps/api/src/five08/backend/api.pyapps/api/src/five08/backend/static/dashboard/.vite/manifest.jsonapps/api/src/five08/backend/static/dashboard/assets/index-DLefnMNV.jsapps/api/src/five08/backend/static/dashboard/index.htmlapps/worker/src/five08/worker/jobs.pydocs/configuration.mdpackages/shared/src/five08/agent/tools.pypackages/shared/src/five08/clients/brevo.pypackages/shared/src/five08/clients/contact_email.pypackages/shared/src/five08/clients/keila.pypackages/shared/src/five08/clients/migadu.pypackages/shared/src/five08/newsletter_sync.pypackages/shared/src/five08/redaction.pypackages/shared/src/five08/runtime_config.pypackages/shared/src/five08/settings.pytests/unit/test_agent_gateway.pytests/unit/test_backend_api.pytests/unit/test_brevo_client.pytests/unit/test_keila_client.pytests/unit/test_migadu_client.pytests/unit/test_newsletter_sync.pytests/unit/test_runtime_config.pytests/unit/test_worker_newsletter_sync.py
✅ Files skipped from review due to trivial changes (3)
- packages/shared/src/five08/clients/contact_email.py
- apps/api/src/five08/backend/static/dashboard/.vite/manifest.json
- ENVIRONMENT.md
🚧 Files skipped from review as they are similar to previous changes (13)
- apps/api/src/five08/backend/static/dashboard/index.html
- docs/configuration.md
- tests/unit/test_worker_newsletter_sync.py
- packages/shared/src/five08/clients/migadu.py
- apps/worker/src/five08/worker/jobs.py
- tests/unit/test_backend_api.py
- packages/shared/src/five08/runtime_config.py
- tests/unit/test_agent_gateway.py
- apps/admin_dashboard/src/main.tsx
- tests/unit/test_newsletter_sync.py
- tests/unit/test_runtime_config.py
- packages/shared/src/five08/agent/tools.py
- packages/shared/src/five08/newsletter_sync.py
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/api/src/five08/backend/api.py`:
- Around line 6243-6299: The current implementation sends the email via
send_onboarding_email_message before calling
_mark_dashboard_onboarding_email_sent to record the send, creating a race
condition where duplicate emails can be sent if the function is retried between
a successful SMTP send and the marker update. Refactor this by moving the marker
reservation or update to occur before the SMTP send operation. Check if the
marker already exists at the beginning of the logic after validation, and only
proceed with send_onboarding_email_message if the marker reservation succeeds.
This makes the operation idempotent so retries after a successful SMTP send will
be safely skipped.
- Around line 6372-6387: The audit metadata being passed to
_audit_dashboard_onboarding_email() contains raw email addresses in the
recipient_email and reply_to_email fields, which violates PII safety practices.
Either mask these email addresses using the existing mask_email() helper
function before including them in the metadata dictionary, or omit these fields
entirely and rely on the contact_id field for traceability. Choose the approach
that best balances debugging needs with privacy requirements.
In `@packages/shared/src/five08/settings.py`:
- Line 90: The newsletter_sync_excluded_mailboxes field in settings.py is
initialized with an empty string default, which means no mailboxes are excluded
by default. This is unsafe because downstream sync logic directly consumes this
value to determine which mailboxes to skip, potentially exposing system and
service mailbox addresses to external providers. Replace the empty string
default with a safe default value that includes common system or service
mailboxes that should be excluded from newsletter synchronization.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: c9c56d89-0c26-46a3-831a-222c33d779bb
📒 Files selected for processing (26)
.env.exampleENVIRONMENT.mdapps/admin_dashboard/src/main.tsxapps/api/src/five08/backend/api.pyapps/api/src/five08/backend/static/dashboard/.vite/manifest.jsonapps/api/src/five08/backend/static/dashboard/assets/index-DLefnMNV.jsapps/api/src/five08/backend/static/dashboard/index.htmlapps/worker/src/five08/worker/jobs.pydocs/configuration.mdpackages/shared/src/five08/agent/tools.pypackages/shared/src/five08/clients/brevo.pypackages/shared/src/five08/clients/contact_email.pypackages/shared/src/five08/clients/keila.pypackages/shared/src/five08/clients/migadu.pypackages/shared/src/five08/newsletter_sync.pypackages/shared/src/five08/redaction.pypackages/shared/src/five08/runtime_config.pypackages/shared/src/five08/settings.pytests/unit/test_agent_gateway.pytests/unit/test_backend_api.pytests/unit/test_brevo_client.pytests/unit/test_keila_client.pytests/unit/test_migadu_client.pytests/unit/test_newsletter_sync.pytests/unit/test_runtime_config.pytests/unit/test_worker_newsletter_sync.py
✅ Files skipped from review due to trivial changes (3)
- packages/shared/src/five08/clients/contact_email.py
- apps/api/src/five08/backend/static/dashboard/.vite/manifest.json
- ENVIRONMENT.md
🚧 Files skipped from review as they are similar to previous changes (13)
- apps/api/src/five08/backend/static/dashboard/index.html
- docs/configuration.md
- tests/unit/test_worker_newsletter_sync.py
- packages/shared/src/five08/clients/migadu.py
- apps/worker/src/five08/worker/jobs.py
- tests/unit/test_backend_api.py
- packages/shared/src/five08/runtime_config.py
- tests/unit/test_agent_gateway.py
- apps/admin_dashboard/src/main.tsx
- tests/unit/test_newsletter_sync.py
- tests/unit/test_runtime_config.py
- packages/shared/src/five08/agent/tools.py
- packages/shared/src/five08/newsletter_sync.py
🛑 Comments failed to post (3)
apps/api/src/five08/backend/api.py (2)
6243-6299:
⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftMake the SMTP send idempotent before calling the provider.
Line 6285 performs the non-idempotent email send before any durable “already sent” reservation. A double-click, retry, or crash between SMTP success and the marker update can send duplicate onboarding emails. Check/reserve the send marker before SMTP, or route this through an idempotent queued job/outbox so retries become no-ops.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/api/src/five08/backend/api.py` around lines 6243 - 6299, The current implementation sends the email via send_onboarding_email_message before calling _mark_dashboard_onboarding_email_sent to record the send, creating a race condition where duplicate emails can be sent if the function is retried between a successful SMTP send and the marker update. Refactor this by moving the marker reservation or update to occur before the SMTP send operation. Check if the marker already exists at the beginning of the logic after validation, and only proceed with send_onboarding_email_message if the marker reservation succeeds. This makes the operation idempotent so retries after a successful SMTP send will be safely skipped.
6372-6387:
⚠️ Potential issue | 🟠 Major | ⚡ Quick winDo not persist raw email addresses in audit metadata.
This success audit stores
recipient_emailandreply_to_emailverbatim. Use the existingmask_email()helper, or omit these fields and rely oncontact_idfor traceability.Proposed PII-safe audit metadata
metadata={ **metadata, - "candidate_name": candidate_name, - "recipient_email": recipient_email, - "reply_to_email": reply_to_email, - "sender_display_name": sender_display_name, - "signature_name": signature_name, + "recipient_email_masked": mask_email(recipient_email), + "reply_to_email_masked": mask_email(reply_to_email), "onboarding_status": onboarding_status, "marker_status": marker_status, "marker_error": marker_error, },📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.await _audit_dashboard_onboarding_email( session, result=AuditResult.SUCCESS, contact_id=contact_id.strip(), metadata={ **metadata, "recipient_email_masked": mask_email(recipient_email), "reply_to_email_masked": mask_email(reply_to_email), "onboarding_status": onboarding_status, "marker_status": marker_status, "marker_error": marker_error, }, )🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/api/src/five08/backend/api.py` around lines 6372 - 6387, The audit metadata being passed to _audit_dashboard_onboarding_email() contains raw email addresses in the recipient_email and reply_to_email fields, which violates PII safety practices. Either mask these email addresses using the existing mask_email() helper function before including them in the metadata dictionary, or omit these fields entirely and rely on the contact_id field for traceability. Choose the approach that best balances debugging needs with privacy requirements.packages/shared/src/five08/settings.py (1)
90-90:
⚠️ Potential issue | 🟠 Major | ⚡ Quick winUse a safe default for excluded newsletter mailboxes.
Line 90 sets
newsletter_sync_excluded_mailboxesto empty, and downstream sync logic consumes this value directly to decide which mailboxes to skip. That can send system/service mailbox addresses to external providers when exclusions are not explicitly configured.🔧 Proposed fix
- newsletter_sync_excluded_mailboxes: str = "" + newsletter_sync_excluded_mailboxes: str = "system,service-account"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.newsletter_sync_excluded_mailboxes: str = "system,service-account"🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/shared/src/five08/settings.py` at line 90, The newsletter_sync_excluded_mailboxes field in settings.py is initialized with an empty string default, which means no mailboxes are excluded by default. This is unsafe because downstream sync logic directly consumes this value to determine which mailboxes to skip, potentially exposing system and service mailbox addresses to external providers. Replace the empty string default with a safe default value that includes common system or service mailboxes that should be excluded from newsletter synchronization.
Summary
/create-mailbox,/create-user-accounts, and agent account provisioning on direct best-effort newsletter sync for mailbox plus backup emailBREVO_508_MEMBERS_NEWSLETTER_LIST_NAMEwith production overrideBREVO_508_MEMBERS_NEWSLETTER_LIST_ID=4audiences: ["508_members"]for segmentingValidation
uv run pytest tests/ -quv run pytest tests/unit/test_brevo_client.py tests/unit/test_keila_client.py tests/unit/test_newsletter_sync.py tests/unit/test_migadu_create_mailbox.py tests/unit/test_crm_create_sso_user.py tests/unit/test_shared_settings.py tests/unit/test_agent_gateway.py tests/unit/test_backend_api.py./scripts/lint.sh./scripts/mypy.shFrontend verification note
bun run lintpasses through./scripts/lint.shbun run typecheckis blocked in this workspace becauseapps/admin_dashboard/node_moduleshas TypeScript 5.9.3 and is missing localtypescript/@types/bun, while the lockfile expects TypeScript 6.0.3 and@types/bunbun run testis blocked because localvitestis missing fromnode_modulesNotes
BREVO_508_MEMBERS_NEWSLETTER_LIST_ID=4; non-prod can omit it and rely on list-name lookup or leave Brevo disabled.508_membersaudience so Keila segments can target it.Summary by CodeRabbit