fix: don't insert skipped test/model results into alerts_v2 when disable_skipped_*_alerts is on

[masukai]

Summary

Fixes #2385.

edr monitor populates alerts_v2 with every test result that is not pass / reused and every model result that is not success / reused, so skipped results are inserted. Its default status filter (fail / error / runtime error / warn) then drops them before sending, and dropped alerts are not marked skipped either, so these rows stay pending forever.

This change follows #2363 (which did the same for reused): when populating alerts_v2, skipped results are left out if the existing dbt package vars disable_skipped_test_alerts / disable_skipped_model_alerts are on. Both are true by default, and the macros read them with elementary.get_config_var, like the package does. Setting a var to false keeps the previous behavior for that kind of result (the row is inserted; with the default filter it will still stay pending, since nothing sends or marks it).

In #2385 I described two possible directions. I went with the smaller one here since it matches #2363 and the existing vars. If you would rather mark alerts removed by the status filter as skipped (the other option in the issue), I'm happy to rework this.

Rows that are already pending in existing installations are not touched.

Validation

The repro from #2385 (DuckDB, edr 0.26.1 installed from this branch, dbt-core 1.10.23, a local stand-in for the Slack webhook), with one failing model, a test and a model skipped because of it, and a control test that fails:

	alerts_v2 rows
master	model error -> sent, test fail -> sent, skipped test -> pending
this branch, default vars	model error -> sent, test fail -> sent (no skipped row)
this branch, disable_skipped_*_alerts: false	as master: the skipped test and the skipped model are inserted as pending

There are no existing tests for the population macros (#2363 had none either), so I did not add one; I can add a test if you tell me where you would like it.

Summary by CodeRabbit

• Bug Fixes
  • Skipped model runs are no longer included in alerts when skipped model alerts are disabled.
  • Skipped test results are no longer included in alerts when skipped test alerts are disabled.
  • When these settings are not enabled, skipped runs and results remain eligible for alerts.

[github-actions]

👋 @masukai
Thank you for raising your pull request.
Please make sure to add tests and document all user-facing changes.
You can do this by editing the docs files in this pull request.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

• Configuration used: defaults
• Review profile: CHILL
• Plan: Advanced
• Run ID: 09305c84-e3d4-447f-af7f-39e7fe9f3e03

📥 Commits

Reviewing files that changed from the base of the PR and between 9c13ec1 and 389ae65.

📒 Files selected for processing: 2

• elementary/monitor/dbt_project/macros/alerts/population/model_alerts.sql
• elementary/monitor/dbt_project/macros/alerts/population/test_alerts.sql

Included review availability: This review used your included allowance. Your plan provides up to 8 included reviews per hour; 7 remain after this review.

📝 Walkthrough

Model and test alert population queries now conditionally exclude skipped results when their respective configuration variables are enabled. Existing exclusions remain unchanged.

Changes: Skipped alert filtering

Layer / File(s)	Summary
Conditional status exclusion elementary/monitor/dbt_project/macros/alerts/population/model_alerts.sql, elementary/monitor/dbt_project/macros/alerts/population/test_alerts.sql	When disable_skipped_model_alerts or disable_skipped_test_alerts is enabled, the corresponding query excludes skipped results. Existing exclusions for success and reused model results, and pass and reused test results, remain in place.

Priority: ➖ Normal

Estimated code review effort: 2 (Simple) | ~8 minutes

Change: Bug fix · Severity of issue fixed: Medium

Suggested reviewers: erikallmer

Merge Risk: ⚪ Minimal · up to 389ae

No merge-blocking issue was identified.

Architecture risk: 🔵 Low · up to 389ae

The change affects 1 system.

Changed systems: elementary

Architecture concerns
No architecture-level concerns identified.

Review details

Systems and components

• observed — elementary (service) was modified; 2 changed files map to changed impact.

Before / after behavior

• observed — Modified behavior in elementary/monitor/dbt_project/macros/alerts/population/model_alerts.sql: The alert-status filter now conditionally adds skipped to the excluded statuses when disable_skipped_model_alerts is enabled; success and reused remain excluded in all cases.
• observed — Modified behavior in elementary/monitor/dbt_project/macros/alerts/population/test_alerts.sql: The failed-test filter now adds skipped to the excluded statuses when disable_skipped_test_alerts is enabled; pass and reused remain excluded unconditionally.

Pre-merge checks: 5

Check name	Status	Explanation
Description Check		Check skipped - CodeRabbit’s high-level summary is enabled.
Title check		The title clearly summarizes the main change: skipped test and model results are excluded from alerts when the corresponding configuration variables are enabled.
Linked Issues check		Issue #2385 accepts excluding skipped results during population when the existing disable variables are enabled. The test population macro now excludes skipped when `elementary.get_config_var('disab…
Out of Scope Changes check		The whole-PR diff contains only one targeted predicate change in each population macro. Both changes directly implement issue #2385 by preventing skipped test and model results from entering `alerts_v…
Docstring Coverage		No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 0…

Full details: Linked Issues check

Explanation

Issue #2385 accepts excluding skipped results during population when the existing disable variables are enabled. The test population macro now excludes skipped when elementary.get_config_var('disable_skipped_test_alerts') is true. The model population macro applies the equivalent disable_skipped_model_alerts condition. The existing pass/success and reused exclusions remain unchanged. The conditions preserve insertion when the variables are false. The reported DuckDB validation covers default and disabled-variable behavior. No issue requirement mandates a new automated test.

Full details: Out of Scope Changes check

Explanation

The whole-PR diff contains only one targeted predicate change in each population macro. Both changes directly implement issue #2385 by preventing skipped test and model results from entering alerts_v2 under the existing configuration variables. No unrelated production or test changes are present.

Full details: Docstring Coverage

Explanation

No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 0 files. (2 skipped: 2 unsupported.)

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create a new PR

• Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

_{Comment @coderabbitai help to get the list of available commands.}

[masukai]

I found a problem with this PR after opening it, so I'm marking it as a draft.

The docs (Filter alerts) say that skipped alerts are filtered out by default and that you receive them with edr monitor --filters statuses:skipped. That flow reads the skipped rows from alerts_v2. With this change and the default vars (disable_skipped_*_alerts: true), those rows are never inserted, so --filters statuses:skipped sends nothing.

I checked this with the repro from #2385 (a skipped test and a skipped model), running edr monitor --filters statuses:skipped with default vars:

	skipped test / model alerts
master	sent
this branch	not sent (no rows)

So this is a regression for that documented usage, unlike reused in #2363 where nothing documents receiving those alerts. The only way to keep receiving skipped alerts with this branch would be to also set the vars to false, which the docs call deprecated for OSS users.

I also want to correct what I wrote in #2385: marking alerts removed by the status filter as skipped (the second option there) would not be safe either, because the docs describe running edr several times with different filters, and an alert dropped by one run's filter has to stay pending for another run.

A direction that might work: let the status filter of the current run decide which statuses are inserted when populating alerts_v2, so the default run does not insert skipped rows (no leftover pending), while --filters statuses:skipped still inserts and sends them. I have not tried it yet. Would you prefer that, or is a pending row for results that no run asked for considered fine by design? I'll rework or close this PR depending on your answer.