Add SMS channel for crisis alerts#33
Conversation
![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: 4c28d9a8f4
ℹ️ 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".
| sms_result = publish_strategy_plugin_sms_alerts( | ||
| signals, | ||
| sms_settings=settings, | ||
| translator=translator, | ||
| strategy_label=settings.strategy_profile, |
There was a problem hiding this comment.
Publishing email and SMS alerts in a single call path means an exception from publish_strategy_plugin_sms_alerts aborts publish_strategy_plugin_alerts after email may already have been sent, and run_strategy_cycle then falls back to zero counts with strategy_plugin_alert_email_error. In environments where SMS credentials are missing or the SMS provider errors, this produces incorrect email telemetry and mislabels the failure channel, which can hide real deliveries and mislead incident response.
Useful? React with 👍 / 👎.
Pigbibi
force-pushed
the
add-crisis-alert-sms-channel
branch
from
4c28d9a to
434daf2
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 434daf2831
ℹ️ 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".
| sms_result = publish_strategy_plugin_sms_alerts( | ||
| signals, | ||
| sms_settings=settings, | ||
| translator=translator, | ||
| strategy_label=settings.strategy_profile, | ||
| context_label=build_strategy_plugin_alert_context_label(settings), | ||
| alert_store=build_strategy_plugin_sms_alert_store(settings, env_reader=env_reader), | ||
| log_message=log_message, | ||
| ) |
There was a problem hiding this comment.
Call failures from publish_strategy_plugin_sms_alerts currently abort publish_strategy_plugin_alerts after email may already have been sent, so run_strategy_cycle falls back to the zeroed alert fields and loses successful email delivery counts. This creates incorrect telemetry whenever SMS credentials/provider calls fail (for example, missing CRISIS_ALERT_SMS_AUTH_TOKEN), making alert-channel incidents harder to diagnose and masking real email sends.
Useful? React with 👍 / 👎.
1 participant
Summary
Tests