Merged
Conversation
Widens the hard-coded three-way Union[TestAlertModel, ModelAlertModel, SourceFreshnessAlertModel] to the common AlertModel base across alert group, message builder, and integration APIs, and widens PendingAlertSchema.data to BaseAlertDataSchema. Enables downstream packages to extend the alert hierarchy (e.g. pipeline alerts) without needing type: ignore workarounds. No behavioral changes — type-level only. mypy passes and unit tests are unaffected. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Removed by mistake in the previous commit. It's a class-level setting that still affects other Union/Optional fields on the schema, so keeping it preserves the pre-existing parsing behavior. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…tract - AlertModel.asset_type: new mandatory @Property (NotImplementedError on base) - AlertModel.concise_name: now raises NotImplementedError on base (was "Alert") - TestAlertModel.asset_type -> "test" - ModelAlertModel.asset_type -> "snapshot" | "model" (based on materialization) - ModelAlertModel.concise_name -> self.alias (was "dbt {type} alert - {alias}") - SourceFreshnessAlertModel.asset_type -> "source" - SourceFreshnessAlertModel.concise_name -> "{source_name}.{identifier}" (was "source freshness alert - {source_name}.{identifier}") - AlertMessageBuilder._get_run_alert_subtitle_blocks now consumes alert.asset_type / alert.concise_name instead of an isinstance chain, so downstream subclasses (e.g. pipeline alerts) work without edits here. - Widened _get_run_alert_subtitle_block's `type` param from Literal to str. - Added unit tests for asset_type/concise_name on every concrete subclass. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Contributor
|
👋 @NoyaArie |
📝 WalkthroughWalkthroughThe pull request generalizes alert handling by introducing abstract Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
elementary/monitor/alerts/alert_messages/builder.py (2)
100-110: Rename parametertypeto avoid shadowing Python builtin.The parameter
typeshadows Python's built-intype()function, flagged by Ruff (A002). While this works, it's a minor code quality issue that can cause confusion or unexpected behavior if the builtin is needed within this method.♻️ Suggested rename
def _get_run_alert_subtitle_block( self, - type: str, + asset_type: str, name: str, status: Optional[str] = None, detected_at_str: Optional[str] = None, suppression_interval: Optional[int] = None, env: Optional[str] = None, links: list[ReportLinkData] = [], orchestrator_info: Optional[OrchestratorInfo] = None, ) -> LinesBlock: summary = [] - summary.append((type.capitalize() + ":", name)) + summary.append((asset_type.capitalize() + ":", name))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@elementary/monitor/alerts/alert_messages/builder.py` around lines 100 - 110, The parameter name "type" in _get_run_alert_subtitle_block shadows Python's built-in; rename it (e.g., to alert_type) in the function signature and update every reference inside the function body and any callers (tests or other modules) to use the new name to avoid the Ruff A002 warning and potential confusion; ensure imports/annotations (if any) and docstrings are adjusted accordingly and run the test/lint suite to confirm no remaining references to "type" remain in this context.
501-507: Type annotation inconsistency formodel_errorsparameter.The
model_errorsparameter is typed asSequence[AlertModel], but it will only ever receiveModelAlertModelinstances from callers (e.g.,AlertsGroup.model_errorsisList[ModelAlertModel]). While this works at runtime, keeping it asSequence[ModelAlertModel]would be more precise and self-documenting.♻️ Suggested type refinement
def _get_sub_alert_groups_blocks( self, test_errors: Sequence[AlertModel], test_warnings: Sequence[AlertModel], test_failures: Sequence[AlertModel], - model_errors: Sequence[AlertModel], + model_errors: Sequence[ModelAlertModel], ) -> List[MessageBlock]:🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@elementary/monitor/alerts/alert_messages/builder.py` around lines 501 - 507, The _get_sub_alert_groups_blocks method currently types the model_errors parameter as Sequence[AlertModel]; change its annotation to Sequence[ModelAlertModel] to reflect the actual objects passed in and improve static typing; update any imports/typing in the file to reference ModelAlertModel and adjust downstream usages/signatures that depend on this parameter (e.g., callers or other helper functions) to ensure consistency with the new Sequence[ModelAlertModel] annotation.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@elementary/monitor/alerts/alert_messages/builder.py`:
- Around line 100-110: The parameter name "type" in
_get_run_alert_subtitle_block shadows Python's built-in; rename it (e.g., to
alert_type) in the function signature and update every reference inside the
function body and any callers (tests or other modules) to use the new name to
avoid the Ruff A002 warning and potential confusion; ensure imports/annotations
(if any) and docstrings are adjusted accordingly and run the test/lint suite to
confirm no remaining references to "type" remain in this context.
- Around line 501-507: The _get_sub_alert_groups_blocks method currently types
the model_errors parameter as Sequence[AlertModel]; change its annotation to
Sequence[ModelAlertModel] to reflect the actual objects passed in and improve
static typing; update any imports/typing in the file to reference
ModelAlertModel and adjust downstream usages/signatures that depend on this
parameter (e.g., callers or other helper functions) to ensure consistency with
the new Sequence[ModelAlertModel] annotation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 967c0987-72cf-4722-9719-eab1a4bba951
📒 Files selected for processing (15)
elementary/monitor/alerts/alert.pyelementary/monitor/alerts/alert_messages/builder.pyelementary/monitor/alerts/alerts_groups/alerts_group.pyelementary/monitor/alerts/alerts_groups/base_alerts_group.pyelementary/monitor/alerts/model_alert.pyelementary/monitor/alerts/source_freshness_alert.pyelementary/monitor/alerts/test_alert.pyelementary/monitor/api/alerts/alert_filters.pyelementary/monitor/data_monitoring/alerts/data_monitoring_alerts.pyelementary/monitor/data_monitoring/alerts/integrations/base_integration.pyelementary/monitor/data_monitoring/alerts/integrations/slack/slack.pyelementary/monitor/fetchers/alerts/schema/alert_data.pyelementary/monitor/fetchers/alerts/schema/pending_alerts.pytests/unit/monitor/alerts/__init__.pytests/unit/monitor/alerts/test_alert_models.py
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
MikaKerman
approved these changes
Apr 16, 2026
| else: | ||
| dbt_type = "model" | ||
| return f"dbt {dbt_type} alert - {self.alias}" | ||
| return self.alias |
Contributor
There was a problem hiding this comment.
Isn't it a change in behavior for existing alerts?
Contributor
Author
There was a problem hiding this comment.
so the existing concise_name is used only for test alert
| @property | ||
| def concise_name(self) -> str: | ||
| return f"source freshness alert - {self.source_name}.{self.identifier}" | ||
| return f"{self.source_name}.{self.identifier}" |
Contributor
There was a problem hiding this comment.
Isn't it a change in behavior for existing alerts?
Contributor
Author
There was a problem hiding this comment.
so the existing concise_name is used only for test alert
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
2 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.
Summary by CodeRabbit
Refactor
Tests