[codex] Add 508 members newsletter sync#332
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 newsletter sync configuration, shared provider clients, sync orchestration, dashboard/API scheduling, worker masking, provisioning integrations, and dashboard controls with unit coverage. ChangesNewsletter sync end-to-end implementation
Sequence Diagram(s)sequenceDiagram
participant AdminDashboard
participant BackendAPI
participant JobQueue
participant WorkerJobs
participant NewsletterSyncProcessor
AdminDashboard->>BackendAPI: POST /dashboard/api/sync/newsletters
BackendAPI->>JobQueue: enqueue sync_508_members_newsletters_job
BackendAPI->>BackendAPI: write newsletter.508_members_sync audit event
JobQueue->>WorkerJobs: dispatch by job name
WorkerJobs->>NewsletterSyncProcessor: sync_508_members_newsletters_job()
NewsletterSyncProcessor-->>WorkerJobs: masked sync result
Estimated code review effort🎯 5 (Critical) | ⏱️ ~90 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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 |
![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 👍 / 👎.
| 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.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| preview = await asyncio.to_thread( | ||
| NewsletterSyncProcessor(settings).sync_508_members, | ||
| dry_run=True, | ||
| ) |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 6687-6702: Dry-run responses in the dashboard flow are leaking
sensitive details because the exception path only redacts email-like text and
the success path returns the raw preview. Update the
`sync_508_members`/dashboard dry-run handling in `api.py` to return a generic
failure message instead of raw exception text, and apply recursive redaction to
the `preview` payload before including it in the `JSONResponse` (for strings,
lists, and dicts, including nested `email`/`mailbox` fields). Use a helper like
`_redact_newsletter_sync_preview` near the dashboard helpers and make the
dry-run branches call it before returning.
🪄 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: eb00d1b3-937d-48fb-933f-fb14a2fd0403
📒 Files selected for processing (13)
.env.exampleapps/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-BQi7fXYh.jsapps/api/src/five08/backend/static/dashboard/index.htmlpackages/shared/src/five08/newsletter_sync.pypackages/shared/src/five08/runtime_config.pypackages/shared/src/five08/settings.pytests/unit/test_backend_api.pytests/unit/test_newsletter_sync.pytests/unit/test_runtime_config.pytests/unit/test_shared_settings.py
✅ Files skipped from review due to trivial changes (2)
- apps/api/src/five08/backend/static/dashboard/.vite/manifest.json
- apps/api/src/five08/backend/static/dashboard/index.html
🚧 Files skipped from review as they are similar to previous changes (8)
- tests/unit/test_shared_settings.py
- apps/admin_dashboard/src/main.tsx
- packages/shared/src/five08/settings.py
- tests/unit/test_backend_api.py
- packages/shared/src/five08/runtime_config.py
- tests/unit/test_runtime_config.py
- packages/shared/src/five08/newsletter_sync.py
- tests/unit/test_newsletter_sync.py
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 209846bc2a
ℹ️ 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".
| mailbox["newsletter_error"] = self._add_emails_to_newsletter( | ||
| [mailbox_email, backup_email] | ||
| ) |
There was a problem hiding this comment.
When the standalone mail_write.create_mailbox agent tool is authorized, its manifest still requires only mailbox:create, but this new call now writes the mailbox and backup email into Brevo/Keila. In any scoped execution or confirmation flow that relies on the manifest-required scopes, a mailbox-only action can now perform external newsletter integration writes without the integration:manage scope that account_write.create_user_accounts already requires; either keep this tool mailbox-only or require the additional scope before subscribing.
Useful? React with 👍 / 👎.
| except Exception as exc: | ||
| text = " ".join(f"Newsletter sync failed: {exc}".split()).strip() | ||
| return f"{text[:197]}..." if len(text) > 200 else text |
| except Exception as exc: | ||
| error_warning = _truncate_discord_text( | ||
| f"Newsletter sync failed: {exc}", | ||
| limit=500, | ||
| ) | ||
| logger.warning("Newsletter sync warning: %s", error_warning, exc_info=True) | ||
| return error_warning |
| except Exception as exc: | ||
| error_warning = self._sanitize_error_message_for_discord( | ||
| f"Newsletter sync failed: {exc}", | ||
| max_length=500, | ||
| ) | ||
| logger.warning("Newsletter sync warning: %s", error_warning, exc_info=True) | ||
| return error_warning |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 24fe493bca
ℹ️ 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 not isinstance(data, dict): | ||
| raise MigaduAPIError("Migadu response payload must be a JSON object.") | ||
|
|
||
| raw_mailboxes = data.get("mailboxes", []) |
There was a problem hiding this comment.
When the periodic or dashboard newsletter sync hits a real Migadu account, the Mailboxes/INDEX response is the index payload itself as a JSON array of mailbox objects, but this branch rejects any top-level list and then looks for a non-documented mailboxes wrapper. That makes configured resync jobs fail before scanning any mailbox; accept the array response shape (or support both shapes) before raising here.
Useful? React with 👍 / 👎.
| def get_suppression( | ||
| self, contact: NewsletterContact | ||
| ) -> NewsletterSuppressionSignal | None: | ||
| list_id = self._list_id() | ||
| if list_id is None: | ||
| return None | ||
| existing = self.client.get_contact(contact.email) | ||
| return self._suppression_from_existing(contact, existing, list_id) | ||
|
|
||
| def _suppression_from_existing( | ||
| self, | ||
| contact: NewsletterContact, | ||
| existing: dict[str, Any] | None, | ||
| list_id: int, | ||
| ) -> NewsletterSuppressionSignal | None: | ||
| if existing is None: | ||
| return None | ||
| reasons: list[str] = [] | ||
| if bool(existing.get("emailBlacklisted")): | ||
| reasons.append("email_blacklisted") | ||
| status = str(existing.get("status") or "").strip().casefold() | ||
| if status in PROVIDER_SUPPRESSED_STATUSES: | ||
| reasons.append(f"status_{status}") | ||
| if _contains_list_id(existing.get("listUnsubscribed"), list_id): | ||
| reasons.append("list_unsubscribed") | ||
| if not reasons: | ||
| return None | ||
| return NewsletterSuppressionSignal( | ||
| email=contact.email, | ||
| source_provider=self.name, | ||
| reason=_joined_reason(reasons), | ||
| metadata={"list_id": list_id, "reasons": reasons}, | ||
| ) | ||
|
|
||
| def _sync_contact(self, contact: NewsletterContact, *, write: bool) -> str: | ||
| list_id = self._list_id() | ||
| if list_id is None: | ||
| return "skipped_list_missing" | ||
|
|
||
| existing = self.client.get_contact(contact.email) | ||
| if self._suppression_from_existing(contact, existing, list_id) is not None: |
| def get_suppression( | ||
| self, contact: NewsletterContact | ||
| ) -> NewsletterSuppressionSignal | None: | ||
| existing = self.client.get_contact_by_email(contact.email) | ||
| return self._suppression_from_existing(contact, existing) | ||
|
|
||
| def _suppression_from_existing( | ||
| self, | ||
| contact: NewsletterContact, | ||
| existing: dict[str, Any] | None, | ||
| ) -> NewsletterSuppressionSignal | None: | ||
| if existing is None: | ||
| return None | ||
| status = str(existing.get("status") or "").strip().casefold() | ||
| if status not in PROVIDER_SUPPRESSED_STATUSES: | ||
| return None | ||
| return NewsletterSuppressionSignal( | ||
| email=contact.email, | ||
| source_provider=self.name, | ||
| reason=f"status_{status}", | ||
| metadata={"status": status}, | ||
| ) | ||
|
|
||
| def _sync_contact(self, contact: NewsletterContact, *, write: bool) -> str: | ||
| existing = self.client.get_contact_by_email(contact.email) | ||
| if self._suppression_from_existing(contact, existing) is not None: |
| assert FakeBrevoClient.contact_lookup_emails == [] | ||
|
|
||
|
|
||
| def test_sync_508_members_avoids_duplicate_keila_contact_lookups() -> None: |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3c35dd6d00
ℹ️ 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 Exception as exc: | ||
| self._apply_provider_status( | ||
| result, | ||
| provider.name, | ||
| contact.email, | ||
| synced_key, | ||
| error=exc, | ||
| ) | ||
| continue |
There was a problem hiding this comment.
When a provider's get_suppression call fails for a contact, this records a provider failure and then continues with no suppression signal, so the caller can still write that same contact to any other configured provider in the later ensure_contact loop. During a Brevo/Keila outage or bad credentials, a contact that is opted out in the failing provider can therefore be added to another newsletter tool, bypassing the provider-independent suppression behavior; skip the contact or abort the sync when suppression lookup fails.
Useful? React with 👍 / 👎.
| result = sync_newsletter_contacts( | ||
| self.runtime_config, | ||
| emails, | ||
| source="agent_account_creation", |
There was a problem hiding this comment.
When newsletter sync is triggered through the agent tools, passing ToolRuntimeConfig here drops postgres_url, and sync_newsletter_contacts only loads newsletter_suppressions when that attribute is present. Agent mailbox/account provisioning therefore ignores active internal/manual suppression records and can re-subscribe an email that the Discord and worker paths would skip; include the DB setting in the runtime config or use the shared settings for this call.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
packages/shared/src/five08/newsletter_sync.py (1)
417-442: 🚀 Performance & Scalability | 🔵 Trivial | 🏗️ Heavy liftAvoid fetching provider contacts twice per sync.
get_suppression()fetches the existing provider contact, thenensure_contact()fetches it again for non-suppressed contacts. Carry the lookup result/sync plan through the provider API so weekly reconciliation and direct provisioning don’t double provider API traffic.Also applies to: 824-877
🤖 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/newsletter_sync.py` around lines 417 - 442, `get_suppression()` and `ensure_contact()` are both fetching the same provider contact, causing duplicate provider API calls during sync. Update the sync flow in `newsletter_sync.py` so the lookup result from `get_suppression()` is carried into the provider operation, and adjust the provider API used by `ensure_contact()` to accept/use that precomputed sync plan or contact state instead of refetching. Keep the change aligned with the `provider_suppressions`, `_record_provider_suppressions`, and `provider.ensure_contact` path so weekly reconciliation and direct provisioning share one provider lookup.
🤖 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/admin_dashboard/src/main.tsx`:
- Around line 1741-1754: The people load flow is using canUse("people:sync") to
decide whether to fetch newsletter suppressions, which incorrectly includes
dry-run-only users and triggers 403s. Update the suppression-read gating in the
main people loader and the related People card/render paths to use
can("people:sync") instead, while keeping canUse("people:sync") only for the
sync button or other dry-run-allowed actions. Apply the same split anywhere the
People load/refresh or suppression UI currently checks canUse("people:sync"),
including the affected people sync and suppression sections.
In `@apps/api/src/five08/backend/api.py`:
- Around line 6738-6739: The Newsletter sync dry-run exception handler is still
leaking sensitive details via logger.exception, so update the except block in
the dry-run flow to avoid logging the raw traceback/message. In the relevant
dry-run handler around the Newsletter sync path, replace
logger.exception("Newsletter sync dry run failed") with a generic failure log
that records only the exception type (for example via type(exc).__name__) and
keep the message free of provider/CRM payload details.
In `@packages/shared/src/five08/newsletter_sync.py`:
- Around line 403-410: The dry-run branch in newsletter_sync’s sync flow returns
before registry suppressions are loaded, so preview results can show contacts as
would_sync incorrectly. Update the dry-run path in the sync method around the
existing _preview_provider_contacts call so _load_registry_suppressions is
applied first, using the same suppression data and synced_key before returning
result. Keep the preview behavior intact, but ensure the contacts filtered by
_load_registry_suppressions match what the write path would later skip as
skipped_internal_suppression.
In `@tests/unit/test_newsletter_sync.py`:
- Around line 143-145: The fake contact-matching logic in
test_newsletter_sync.py is too permissive because it appends contacts even when
neither c508Email nor emailAddress is present. Update the matcher to mirror
production behavior by only adding contacts when there is a non-empty
intersection on email fields, using the existing contact filtering logic around
contact_values and matches so email-less CRM contacts are not returned.
---
Nitpick comments:
In `@packages/shared/src/five08/newsletter_sync.py`:
- Around line 417-442: `get_suppression()` and `ensure_contact()` are both
fetching the same provider contact, causing duplicate provider API calls during
sync. Update the sync flow in `newsletter_sync.py` so the lookup result from
`get_suppression()` is carried into the provider operation, and adjust the
provider API used by `ensure_contact()` to accept/use that precomputed sync plan
or contact state instead of refetching. Keep the change aligned with the
`provider_suppressions`, `_record_provider_suppressions`, and
`provider.ensure_contact` path so weekly reconciliation and direct provisioning
share one provider lookup.
🪄 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: 93e50824-f786-419d-8cde-c6b2dce6f7b5
📒 Files selected for processing (21)
apps/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-BoK8s4aw.cssapps/api/src/five08/backend/static/dashboard/assets/index-DE4wrrPS.cssapps/api/src/five08/backend/static/dashboard/assets/index-DQl5fG-a.jsapps/api/src/five08/backend/static/dashboard/index.htmlapps/discord_bot/src/five08/discord_bot/cogs/crm.pyapps/discord_bot/src/five08/discord_bot/cogs/migadu.pyapps/worker/src/five08/worker/migrations/versions/20260614_0100_create_newsletter_suppressions.pypackages/shared/src/five08/agent/tools.pypackages/shared/src/five08/clients/migadu.pypackages/shared/src/five08/newsletter_suppressions.pypackages/shared/src/five08/newsletter_sync.pytests/evals/discord-agent/fixtures/v1/mailbox_create_confirmation_001.jsontests/unit/test_agent_gateway.pytests/unit/test_backend_api.pytests/unit/test_crm_create_sso_user.pytests/unit/test_migadu_client.pytests/unit/test_migadu_create_mailbox.pytests/unit/test_newsletter_sync.py
💤 Files with no reviewable changes (1)
- apps/api/src/five08/backend/static/dashboard/assets/index-BoK8s4aw.css
✅ Files skipped from review due to trivial changes (2)
- apps/api/src/five08/backend/static/dashboard/index.html
- apps/api/src/five08/backend/static/dashboard/.vite/manifest.json
🚧 Files skipped from review as they are similar to previous changes (7)
- tests/unit/test_crm_create_sso_user.py
- tests/unit/test_migadu_create_mailbox.py
- apps/discord_bot/src/five08/discord_bot/cogs/migadu.py
- packages/shared/src/five08/clients/migadu.py
- tests/unit/test_agent_gateway.py
- apps/discord_bot/src/five08/discord_bot/cogs/crm.py
- packages/shared/src/five08/agent/tools.py
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: aa583fa53a
ℹ️ 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".
| synced_key, | ||
| ) | ||
| self._record_provider_suppressions(provider_suppressions, result) | ||
| if active_suppressions or provider_suppressions: |
There was a problem hiding this comment.
When a Brevo/Keila contact that was previously suppressed later resubscribes or is otherwise re-enabled at the provider, provider_suppressions becomes empty but the old active registry row is still loaded here and causes the contact to be skipped forever. The new registry only ever upserts rows with active = true and there is no path that deactivates provider-observed suppressions when the provider no longer reports them, so explicit resubscriptions remain blocked by stale local state.
Useful? React with 👍 / 👎.
| ) | ||
| keila_api_timeout_seconds: float = 20.0 | ||
| newsletter_sync_enabled: bool = True | ||
| newsletter_sync_interval_seconds: int = 604800 |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c12f5f0c33
ℹ️ 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 Exception as exc: | ||
| result["suppression_registry_warning"] = redact_email_addresses(str(exc)) | ||
| return {} |
There was a problem hiding this comment.
When postgres_url is configured but loading newsletter_suppressions fails (for example, a transient DB/query error or a missing table during deployment), this returns an empty map and the sync proceeds as if no manual/internal suppressions exist. That can re-subscribe addresses that were explicitly suppressed only in the registry; this should skip/abort the affected sync instead of treating the registry as empty.
Useful? React with 👍 / 👎.
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