feat(alerts): two-tier integrations — global credentials + per-rule routing#85
feat(alerts): two-tier integrations — global credentials + per-rule routing#85AbianS wants to merge 3 commits into
Conversation
…outing Introduce alert_integrations (global provider credentials) and routing_override JSONB on alert_rule_channels so one Slack bot token or SMTP config can serve multiple projects/channels without duplication. Non-destructive migration extracts credentials from notification_channels, backfills routing_override per provider type, renames channel_id→integration_id in alert_rule_channels and alert_history, then drops notification_channels. API routes renamed from /api/alert-channels to /api/integrations; dispatcher send() updated to accept (integration, routing, payload); 422 validation enforced at rule-create time.
- Add validate_routing_override(provider_type, credentials, routing): Slack bot_token requires non-empty channel; Email requires non-empty recipients with '@'; Webhook requires URL in credentials or routing, rejects non-http/https routing URL - Call validate in create_rule, update_rule, and test_channel handlers (test_channel now accepts optional TestIntegrationBody with routing_override) - Rename AlertRuleResponse.channel_ids → integration_ids - Dedup integration_id before INSERT in create_rule and update_rule (ECH-4) - Spec: specLoopIteration 3, SCL-2 changelog entry - Deferred: append D-15 (header injection), D-16 (credential exposure), D-17 (SMTP recipient cap), D-18 (retry counter)
WalkthroughThis PR refactors the alert notification system from a single ChangesAlert Two-Tier Integrations Architecture
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
Greptile SummaryThis PR introduces a two-tier integration model: global
Confidence Score: 3/5Not safe to merge without addressing the silent Email dispatch failure on the legacy channel_ids path. The core two-tier model is well-designed and migrations look correct, but validate_routing_override is only called for the new channels request field. Any caller using channel_ids with an Email integration stores an empty recipients list and receives a success response without any email being sent. apps/server/src/routes/alerts.rs (create_rule and update_rule handlers need validation on the legacy channel_ids path) and apps/server/src/services/notification/email.rs (send_to_recipients always returns success). Important Files Changed
Sequence DiagramsequenceDiagram
participant Client
participant Route as alerts.rs
participant Service as AlertService
participant DB
participant Dispatcher as Notifier
Note over Client,Dispatcher: New channels field path
Client->>Route: "{channels: [{integration_id, routing_override}]}"
Route->>DB: get_channel(integration_id)
DB-->>Route: AlertIntegration
Route->>Route: validate_routing_override()
Route->>Service: create_rule(body)
Service->>DB: INSERT alert_rule_channels (routing_override)
Note over Client,Dispatcher: Legacy channel_ids path
Client->>Route: "{channel_ids: [1]}"
Route->>Route: validation skipped
Route->>Service: create_rule(body)
Service->>DB: "INSERT alert_rule_channels (routing_override={})"
Note over Client,Dispatcher: Alert firing
Service->>DB: SELECT integration_id, routing_override WHERE is_enabled
DB-->>Service: Vec AlertRuleChannel
loop for each rule_channel
Service->>DB: get_channel(integration_id)
DB-->>Service: AlertIntegration
Service->>Dispatcher: send(integration, routing_override, payload)
Dispatcher-->>Service: NotificationResult
Service->>DB: UPDATE alert_integrations and alert_history
end
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| // Verify project exists | ||
| let _ = ProjectService::get_by_id(pool.get_ref(), project_id).await?; | ||
|
|
||
| let rule = AlertService::create_rule(pool.get_ref(), project_id, body.into_inner()).await?; | ||
| let channel_ids = AlertService::get_rule_channels(pool.get_ref(), rule.id).await?; | ||
| let body = body.into_inner(); | ||
|
|
||
| // Validate routing_override for each channel before linking | ||
| for ch in &body.channels { | ||
| let integration = AlertService::get_channel(pool.get_ref(), ch.integration_id).await?; | ||
| validate_routing_override( | ||
| integration.provider_type, | ||
| &integration.credentials, | ||
| &ch.routing_override, | ||
| )?; | ||
| } | ||
|
|
||
| let rule = AlertService::create_rule(pool.get_ref(), project_id, body).await?; |
There was a problem hiding this comment.
channel_ids path bypasses routing validation — Email dispatches silently with no recipients
In both create_rule and update_rule, routing validation only runs over body.channels. When a caller submits the backward-compat field channel_ids: [1] (with channels absent or empty), the validation loop is skipped entirely. The service maps each legacy ID to routing_override: {}, which for an Email integration deserializes to EmailRoutingOverride { recipients: [] }. send_to_recipients with an empty slice loops zero times and returns NotificationResult::success(None) — the alert appears dispatched but no email is ever sent.
|
|
||
| NotificationResult::success(None) | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
sent_any is computed but never checked — function always returns success
send_to_recipients tracks sent_any across the per-recipient loop but unconditionally returns NotificationResult::success(None) regardless of its value. A total SMTP failure still returns success, preventing history records from being marked failed and suppressing retry logic.
![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
🧹 Nitpick comments (5)
apps/server/tests/unit/notification_test.rs (1)
188-199: 💤 Low valueDuplicate of
test_slack_bot_token_validate_config_valid.
test_slack_bot_token_validate_config_channel_is_routing_concernexercises the same path astest_slack_bot_token_validate_config_valid(line 162): abot_tokenconfig with nochannelfield is expected to validate Ok. Only the token literal differs. Consider folding this into the existing valid test (or adding a meaningfully distinct case, e.g., an explicit"channel": ""or extra unknown field, to assert thatchannelis genuinely ignored rather than just absent).🤖 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/server/tests/unit/notification_test.rs` around lines 188 - 199, The test test_slack_bot_token_validate_config_channel_is_routing_concern duplicates test_slack_bot_token_validate_config_valid; remove the duplicate or change it to exercise a distinct behavior: either delete test_slack_bot_token_validate_config_channel_is_routing_concern, or modify it to include an explicit `"channel": ""` (or another extra unknown field) in the config JSON and assert dispatcher.validate_config(&config).is_ok() to prove channel is ignored; locate the test by the function name and the use of create_dispatcher(ChannelType::Slack) and validate_config to make the change.apps/server/migrations/sqlite/20260521131726_alert_integrations.up.sql (1)
74-87: 💤 Low valueMinor cosmetic difference from Postgres migration in Slack routing override backfill.
The SQLite version uses
json_patch('{}', json_object('channel', ..., 'username', ..., 'icon_emoji', ...))which will include explicitnullvalues (e.g.{"channel":null,"username":null,"icon_emoji":null}) when the source config fields are null, whereas the Postgres version usesjsonb_strip_nulls(jsonb_build_object(...))to produce{}in the same scenario.Impact: This is a cosmetic difference only. The Rust routing override structs use
#[serde(default)]on all fields, so deserializing{"channel":null}and{}both produceNonefor thechannelfield. The behavior is functionally identical.🔧 Optional: Match Postgres behavior by filtering nulls
If you want exact JSON parity between databases, you could wrap the json_object result in a custom null-filtering expression, but SQLite doesn't have a built-in equivalent to
jsonb_strip_nulls. Since this doesn't affect functionality, it's safe to leave as-is.🤖 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/server/migrations/sqlite/20260521131726_alert_integrations.up.sql` around lines 74 - 87, Replace the Slack branch that currently returns json_patch('{}', json_object(...)) with an expression that builds the json_object into a temporary value and then removes keys whose values are null so the result matches Postgres' jsonb_strip_nulls; specifically, in the CASE branch that checks nc.channel_type = 'slack' and json_extract(nc.config, '$.method') = 'bot_token', build obj := json_object('channel', json_extract(nc.config, '$.channel'), 'username', json_extract(nc.config, '$.username'), 'icon_emoji', json_extract(nc.config, '$.icon_emoji')) and then apply json_remove(obj, '$.channel') if json_extract(obj, '$.channel') IS NULL (and similarly for '$.username' and '$.icon_emoji') so null-valued keys are removed rather than serialized as null.apps/server/openapi.json (2)
2886-2892: ⚡ Quick win
TestIntegrationBody.routing_overridelacks a type definition.The
routing_overrideproperty has no type specified (just an empty object{}). This should include"type": "object"or"type": ["object", "null"]for proper schema validation and client generation.Suggested fix
"TestIntegrationBody": { "type": "object", "description": "Request body for test endpoint (routing_override is optional)", "properties": { - "routing_override": {} + "routing_override": { + "type": ["object", "null"] + } } },🤖 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/server/openapi.json` around lines 2886 - 2892, The TestIntegrationBody schema defines routing_override with an empty object; update the property so it has an explicit type (e.g., add "type": "object" or "type": ["object", "null"] if nullable) to enable proper validation and client generation; locate TestIntegrationBody -> properties -> routing_override in the OpenAPI JSON and add the chosen type and any needed "additionalProperties" or "properties" constraints as appropriate.
22-22: 💤 Low valueOperation IDs still reference "channel" terminology.
The paths have been renamed to
/api/integrationsbut theoperationIdvalues still use the old naming (list_channels,create_channel,get_channel, etc.). This inconsistency may confuse SDK generators and API consumers.Also applies to: 59-59, 115-115, 171-171, 220-220, 288-288
🤖 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/server/openapi.json` at line 22, OperationId values still use "channel" names (e.g., "list_channels", "create_channel", "get_channel", "update_channel", "delete_channel") while the paths were renamed to /api/integrations; update each operationId to match the new resource naming (e.g., "list_integrations", "create_integration", "get_integration", "update_integration", "delete_integration") so SDK generators and consumers are consistent—locate the operationId fields in the OpenAPI JSON (the entries currently set to "list_channels", "create_channel", "get_channel", etc.) and replace them with the corresponding "integration" variants.apps/server/src/routes/alerts.rs (1)
157-166: 💤 Low valueOpenAPI tags still reference "Alert Channels" but paths use
/api/integrations.The
tagattribute in utoipa path macros still says"Alert Channels"while the actual paths have been renamed to/api/integrations. This will create inconsistent grouping in generated API documentation.Also applies to: 174-183, 195-205, 217-228, 242-252, 264-274
🤖 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/server/src/routes/alerts.rs` around lines 157 - 166, The utoipa::path attribute tags still read "Alert Channels" while the route paths are under "/api/integrations"; update the tag string in each utoipa::path attribute that targets "/api/integrations" to a consistent value like "Integrations" (e.g., change the tag in the utoipa::path macros attached to the GET/POST/PUT/DELETE handlers for the integrations endpoints in alerts.rs so all occurrences that currently say "Alert Channels" instead say "Integrations").
🤖 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/server/openapi.json`:
- Around line 91-100: The OpenAPI contract contains an incorrect 422 response
for POST /api/integrations (create_channel); remove the "422 Invalid routing
override" response block from apps/server/openapi.json for that operation (or
change it to the correct 400 mapping) so the schema matches the create_channel
utoipa annotation and the AppError::Validation -> 400 mapping; ensure the
operation only references the existing 400 ErrorResponse and that no
routing_override validation response remains.
In `@apps/server/src/services/notification/slack.rs`:
- Around line 138-167: format_bot_message currently inserts an empty string for
"channel" when both routing.channel and creds.channel are None, which allows a
request to be sent to Slack and elicit a channel_not_found error; update the
dispatch to fail fast: in send_bot_token (and the similar dispatcher used around
lines 289-324) check for a resolved channel by computing the same precedence
used in format_bot_message (routing.channel.or(creds.channel)) and return a
clear error (rather than sending) when that result is None or empty; keep
format_bot_message as a pure formatter but remove the unwrap_or_default() usage
or make it return Result so the caller can enforce the presence of a channel and
surface a descriptive error to the caller before any HTTP request is made.
In `@apps/server/tests/unit/notification_test.rs`:
- Around line 220-228: The test test_email_validate_config_no_smtp_host_fails is
flaky because it doesn't ensure SMTP_HOST is unset or hold SMTP_ENV_LOCK for the
test duration; change the test to acquire the SMTP_ENV_LOCK for the whole test
and explicitly remove SMTP_HOST from the environment before calling
dispatcher.validate_config so the environment state is deterministic. Also
update SmtpHostGuard so it actually owns the MutexGuard (store a field like
_lock: std::sync::MutexGuard<'static, ()> in the struct returned by
SmtpHostGuard::set and SmtpHostGuard::unset) so the lock is held while the guard
exists, and adjust callers (including test_email_validate_config_valid and
test_email_validate_config_recipients_are_routing_concern) to use the guard's
lifetime to serialize access to the SMTP_HOST env var.
---
Nitpick comments:
In `@apps/server/migrations/sqlite/20260521131726_alert_integrations.up.sql`:
- Around line 74-87: Replace the Slack branch that currently returns
json_patch('{}', json_object(...)) with an expression that builds the
json_object into a temporary value and then removes keys whose values are null
so the result matches Postgres' jsonb_strip_nulls; specifically, in the CASE
branch that checks nc.channel_type = 'slack' and json_extract(nc.config,
'$.method') = 'bot_token', build obj := json_object('channel',
json_extract(nc.config, '$.channel'), 'username', json_extract(nc.config,
'$.username'), 'icon_emoji', json_extract(nc.config, '$.icon_emoji')) and then
apply json_remove(obj, '$.channel') if json_extract(obj, '$.channel') IS NULL
(and similarly for '$.username' and '$.icon_emoji') so null-valued keys are
removed rather than serialized as null.
In `@apps/server/openapi.json`:
- Around line 2886-2892: The TestIntegrationBody schema defines routing_override
with an empty object; update the property so it has an explicit type (e.g., add
"type": "object" or "type": ["object", "null"] if nullable) to enable proper
validation and client generation; locate TestIntegrationBody -> properties ->
routing_override in the OpenAPI JSON and add the chosen type and any needed
"additionalProperties" or "properties" constraints as appropriate.
- Line 22: OperationId values still use "channel" names (e.g., "list_channels",
"create_channel", "get_channel", "update_channel", "delete_channel") while the
paths were renamed to /api/integrations; update each operationId to match the
new resource naming (e.g., "list_integrations", "create_integration",
"get_integration", "update_integration", "delete_integration") so SDK generators
and consumers are consistent—locate the operationId fields in the OpenAPI JSON
(the entries currently set to "list_channels", "create_channel", "get_channel",
etc.) and replace them with the corresponding "integration" variants.
In `@apps/server/src/routes/alerts.rs`:
- Around line 157-166: The utoipa::path attribute tags still read "Alert
Channels" while the route paths are under "/api/integrations"; update the tag
string in each utoipa::path attribute that targets "/api/integrations" to a
consistent value like "Integrations" (e.g., change the tag in the utoipa::path
macros attached to the GET/POST/PUT/DELETE handlers for the integrations
endpoints in alerts.rs so all occurrences that currently say "Alert Channels"
instead say "Integrations").
In `@apps/server/tests/unit/notification_test.rs`:
- Around line 188-199: The test
test_slack_bot_token_validate_config_channel_is_routing_concern duplicates
test_slack_bot_token_validate_config_valid; remove the duplicate or change it to
exercise a distinct behavior: either delete
test_slack_bot_token_validate_config_channel_is_routing_concern, or modify it to
include an explicit `"channel": ""` (or another extra unknown field) in the
config JSON and assert dispatcher.validate_config(&config).is_ok() to prove
channel is ignored; locate the test by the function name and the use of
create_dispatcher(ChannelType::Slack) and validate_config to make the change.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 5cbb459e-4b47-4e7a-ab68-1e03d294406e
📒 Files selected for processing (18)
_bmad-output/implementation-artifacts/deferred-work.md_bmad-output/implementation-artifacts/investigations/alert-two-tier-config-investigation.md_bmad-output/implementation-artifacts/spec-alert-two-tier-integrations.mdapps/server/migrations/postgres/20260521131726_alert_integrations.down.sqlapps/server/migrations/postgres/20260521131726_alert_integrations.up.sqlapps/server/migrations/sqlite/20260521131726_alert_integrations.down.sqlapps/server/migrations/sqlite/20260521131726_alert_integrations.up.sqlapps/server/openapi.jsonapps/server/src/models/alert.rsapps/server/src/models/mod.rsapps/server/src/routes/alerts.rsapps/server/src/services/alert.rsapps/server/src/services/notification/email.rsapps/server/src/services/notification/mod.rsapps/server/src/services/notification/slack.rsapps/server/src/services/notification/webhook.rsapps/server/tests/integration/alerts_api_test.rsapps/server/tests/unit/notification_test.rs
| "422": { | ||
| "description": "Invalid routing override", | ||
| "content": { | ||
| "application/json": { | ||
| "schema": { | ||
| "$ref": "#/components/schemas/ErrorResponse" | ||
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if create_channel handler in routes/alerts.rs calls validate_routing_override
rg -n -A 20 'pub async fn create_channel' apps/server/src/routes/alerts.rs | head -40Repository: AbianS/rustrak
Length of output: 965
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== 1) Find which /api/integrations operation includes 422 Invalid routing override =="
rg -n --context=3 '"422"\s*:\s*\{' apps/server/openapi.json | head -n 40
rg -n --context=3 '"/api/integrations"' apps/server/openapi.json | head -n 80
echo
echo "== 2) Find the Rust route handler(s) for /api/integrations =="
rg -n --context=3 '(/api/integrations|\"/api/integrations|path = "/api/integrations' apps/server/src/routes apps/server/src -S || true
echo
echo "== 3) Find all routing override validation call sites =="
rg -n --context=2 'validate_routing_override' apps/server/src || true
rg -n --context=2 'Invalid routing override' apps/server/src || trueRepository: AbianS/rustrak
Length of output: 8798
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== A) Show surrounding OpenAPI context for each 422 Invalid routing override occurrence =="
# Print ~40 lines around each match, including the likely surrounding operation block.
rg -n '"422"\s*:\s*\{\s*' apps/server/openapi.json | rg -n '"description"\s*:\s*"Invalid routing override"' -n >/dev/null 2>&1 || true
# Directly fetch context blocks around the description text.
rg -n --context=18 '"description"\s*:\s*"Invalid routing override"' apps/server/openapi.json
echo
echo "== B) Locate any Rust route definitions containing /api/integrations =="
rg -n '\/api\/integrations' apps/server/src/routes apps/server/src -S || true
echo
echo "== C) Specifically search for POST /api/integrations in Rust (UTOIPA attributes or route macros) =="
rg -n 'post,\s*\n\s*path\s*=\s*"/api/integrations|path\s*=\s*"/api/integrations".*post|path\s*=\s*"/api/integrations"' apps/server/src/routes apps/server/src -S || true
echo
echo "== D) Search for error mapping/response code 422 in openapi near integrations section =="
# Find the integrations path and scan for 422 status blocks within its operation range.
start=$(rg -n '"\/api\/integrations"' apps/server/openapi.json | head -n1 | cut -d: -f1)
echo "First /api/integrations occurrence at line: $start"
sed -n "${start},$((start+260))p" apps/server/openapi.json | rg -n '"422"|' || true
echo
echo "== E) Search for 'Invalid routing override' string in Rust (response text / error variants) =="
rg -n 'Invalid routing override' apps/server/src -S || trueRepository: AbianS/rustrak
Length of output: 4025
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
import json,sys
p="apps/server/openapi.json"
with open(p,"r",encoding="utf-8") as f:
data=json.load(f)
paths=data.get("paths",{})
def show(path):
ops=paths.get(path)
if not ops:
print(f"{path}: <missing>")
return
print(f"\n{path}:")
for method in ["get","post","put","patch","delete","options","head"]:
if method in ops:
resp=ops[method].get("responses",{})
has_422="422" in resp
desc=None
if has_422:
desc=resp["422"].get("description")
print(f" {method.upper()}: responses has 422? {has_422} desc={desc}")
show("/api/integrations")
show("/api/integrations/{id}")
# Also find any operation that uses the "Invalid routing override" description.
needle="Invalid routing override"
hits=[]
for path,ops in paths.items():
for method,op in ops.items():
for code,r in (op.get("responses") or {}).items():
if isinstance(r,dict) and r.get("description")==needle:
hits.append((path,method,code))
print("\nOccurrences of description='Invalid routing override':")
for t in sorted(hits):
print(f" {t[1].upper()} {t[0]} -> {t[2]}")
PY
echo
echo "== Rust: find handlers mentioning /api/integrations =="
rg -n '"/api/integrations' apps/server/src -S || true
rg -n 'path\s*=\s*"/api/integrations' apps/server/src -S || true
rg -n '\/api\/integrations' apps/server/src -S || true
echo
echo "== Rust: find any openapi utoipa path attribute for /api/integrations =="
rg -n 'utoipa::path\(|responses\(|responses\(\s*' apps/server/src/routes -S || trueRepository: AbianS/rustrak
Length of output: 6030
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== 1) Inspect utoipa::path blocks for /api/integrations in alerts.rs =="
sed -n '130,230p' apps/server/src/routes/alerts.rs
echo
echo "== 2) Inspect the router wiring for /api/integrations scope =="
sed -n '600,720p' apps/server/src/routes/alerts.rs
echo
echo "== 3) Locate AlertService::create_channel implementation and inspect for routing override validation =="
rg -n "create_channel\\(" apps/server/src | head -n 50
# Print likely implementation file(s)
files=$(rg -l "fn create_channel" apps/server/src || true)
echo "Files containing fn create_channel: "
echo "$files"
for f in $files; do
echo
echo "---- $f (context) ----"
# show ~80 lines around the function
line=$(rg -n "fn create_channel" "$f" | head -n 1 | cut -d: -f1)
start=$((line-40)); end=$((line+120))
sed -n "${start},${end}p" "$f"
done
echo
echo "== 4) Search for the 422 mapping or error text for 'Invalid routing override' in Rust =="
rg -n "Invalid routing override" apps/server/src || true
rg -n "UNPROCESSABLE_ENTITY|StatusCode::UNPROCESSABLE_ENTITY|unprocessable" apps/server/src || true
echo
echo "== 5) Search for routing_override usage during create_channel =="
rg -n "routing_override" apps/server/src/routes apps/server/src/services apps/server/src/models || trueRepository: AbianS/rustrak
Length of output: 28143
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== 1) Find where 422 / UNPROCESSABLE_ENTITY is produced =="
rg -n 'UNPROCESSABLE_ENTITY|StatusCode::UNPROCESSABLE_ENTITY|unprocessable' apps/server/src -S || true
rg -n 'AppError::Validation|Validation\(' apps/server/src -S || true
rg -n 'Invalid routing override' apps/server/src -S || true
echo
echo "== 2) Locate AppError->HTTP mapping (error handler / responder) =="
rg -n 'impl .*ResponseError|status_code\(|into_response|map_err|match .*AppError' apps/server/src -S || true
echo
echo "== 3) Show any error mapping code around Validation -> status =="
rg -n 'Validation.*(UNPROCESSABLE|422|StatusCode::UNPROCESSABLE)|UNPROCESSABLE.*Validation|422.*Validation' apps/server/src -S || trueRepository: AbianS/rustrak
Length of output: 12866
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '1,120p' apps/server/src/error.rs
echo
sed -n '50,170p' apps/server/src/routes/alerts.rsRepository: AbianS/rustrak
Length of output: 7379
Remove/fix the 422 Invalid routing override response for POST /api/integrations in apps/server/openapi.json.
POST /api/integrations (create_channel) doesn’t validate any routing_override (it only validates provider credentials), and routing-override validation returns AppError::Validation, which maps to 400. The utoipa annotation for create_channel also omits 422, so the OpenAPI contract is currently out of sync.
🤖 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/server/openapi.json` around lines 91 - 100, The OpenAPI contract
contains an incorrect 422 response for POST /api/integrations (create_channel);
remove the "422 Invalid routing override" response block from
apps/server/openapi.json for that operation (or change it to the correct 400
mapping) so the schema matches the create_channel utoipa annotation and the
AppError::Validation -> 400 mapping; ensure the operation only references the
existing 400 ErrorResponse and that no routing_override validation response
remains.
| fn format_bot_message( | ||
| creds: &SlackBotTokenConfig, | ||
| routing: &SlackRoutingOverride, | ||
| payload: &AlertPayload, | ||
| ) -> serde_json::Value { | ||
| let mut body = Self::format_webhook_message(payload); | ||
| body["channel"] = json!(cfg.channel); | ||
| if let Some(ref username) = cfg.username { | ||
| body["username"] = json!(username); | ||
|
|
||
| // Channel: routing wins over credentials | ||
| let channel = routing | ||
| .channel | ||
| .as_deref() | ||
| .or(creds.channel.as_deref()) | ||
| .unwrap_or_default(); | ||
| body["channel"] = json!(channel); | ||
|
|
||
| let username = routing.username.as_deref().or(creds.username.as_deref()); | ||
| if let Some(u) = username { | ||
| body["username"] = json!(u); | ||
| } | ||
| if let Some(ref icon_emoji) = cfg.icon_emoji { | ||
| body["icon_emoji"] = json!(icon_emoji); | ||
|
|
||
| let icon_emoji = routing | ||
| .icon_emoji | ||
| .as_deref() | ||
| .or(creds.icon_emoji.as_deref()); | ||
| if let Some(e) = icon_emoji { | ||
| body["icon_emoji"] = json!(e); | ||
| } | ||
|
|
||
| body | ||
| } |
There was a problem hiding this comment.
Bot-token dispatch silently sends with empty channel when neither routing nor credentials provide one.
At Line 146-151, when both routing.channel and creds.channel are None, unwrap_or_default() produces "" and the request body becomes "channel": "". Combined with validate_bot_token_config no longer requiring a channel in credentials (Line 364-372) and validate_config for routing being best-effort (Line 310-316 does unwrap_or(...) and swallows parse failures), the dispatcher can reach Slack with an empty channel and only get an opaque channel_not_found from the API.
The validate_routing_override route-layer check guards rule creation, but the POST /api/integrations/{id}/test path may pass an empty routing body, and any malformed routing JSON here falls back to all-None. Suggest failing fast at dispatch time with a clear error before hitting Slack.
🛡️ Proposed defensive check in send_bot_token
async fn send_bot_token(
&self,
cfg: &SlackBotTokenConfig,
routing: &SlackRoutingOverride,
payload: &AlertPayload,
) -> NotificationResult {
+ // bot_token requires a channel from routing or credentials.
+ if routing.channel.as_deref().or(cfg.channel.as_deref()).unwrap_or("").is_empty() {
+ return NotificationResult::failure(
+ "Slack bot_token requires a channel in routing_override or credentials".to_string(),
+ None,
+ );
+ }
let body = Self::format_bot_message(cfg, routing, payload);Also applies to: 289-324
🤖 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/server/src/services/notification/slack.rs` around lines 138 - 167,
format_bot_message currently inserts an empty string for "channel" when both
routing.channel and creds.channel are None, which allows a request to be sent to
Slack and elicit a channel_not_found error; update the dispatch to fail fast: in
send_bot_token (and the similar dispatcher used around lines 289-324) check for
a resolved channel by computing the same precedence used in format_bot_message
(routing.channel.or(creds.channel)) and return a clear error (rather than
sending) when that result is None or empty; keep format_bot_message as a pure
formatter but remove the unwrap_or_default() usage or make it return Result so
the caller can enforce the presence of a channel and surface a descriptive error
to the caller before any HTTP request is made.
| #[test] | ||
| fn test_email_validate_config_empty_recipients() { | ||
| let _guard = SmtpHostGuard::set("smtp.example.com"); | ||
|
|
||
| fn test_email_validate_config_no_smtp_host_fails() { | ||
| // No global SMTP and no integration SMTP should fail validation | ||
| let dispatcher = create_dispatcher(ChannelType::Email); | ||
| let config = json!({ | ||
| "recipients": [] | ||
| }); | ||
| let config = json!({}); | ||
|
|
||
| let result = dispatcher.validate_config(&config); | ||
| assert!(result.is_err()); | ||
| } |
There was a problem hiding this comment.
Flaky test: depends on SMTP_HOST being unset but doesn't enforce it.
test_email_validate_config_no_smtp_host_fails asserts that validate_config errors when no global SMTP is configured, but it neither acquires SMTP_ENV_LOCK nor removes SMTP_HOST from the environment. Cargo runs unit tests in parallel, and the sibling tests test_email_validate_config_valid (line 210) and test_email_validate_config_recipients_are_routing_concern (line 233) both call SmtpHostGuard::set("smtp.example.com"). Worse, SmtpHostGuard::set only holds SMTP_ENV_LOCK for the duration of set() itself (the _lock is dropped before the guard is returned), so the mutex doesn't actually serialize the env-var window across tests. If this test runs while another has SMTP_HOST set, validate_config will see the host and return Ok, and assert!(result.is_err()) will fail nondeterministically.
Two issues are entangled; consider fixing both together:
- Make this test explicitly clear
SMTP_HOST(and hold the lock for the test's lifetime). - Have
SmtpHostGuardactually own theMutexGuardso the lock spans the whole test.
🔒 Suggested fix sketch
// Change SmtpHostGuard to own the lock guard:
struct SmtpHostGuard {
previous: Option<String>,
_lock: std::sync::MutexGuard<'static, ()>,
}
impl SmtpHostGuard {
fn set(value: &str) -> Self {
let lock = SMTP_ENV_LOCK.lock().expect("SMTP env lock poisoned");
let previous = std::env::var("SMTP_HOST").ok();
std::env::set_var("SMTP_HOST", value);
Self { previous, _lock: lock }
}
fn unset() -> Self {
let lock = SMTP_ENV_LOCK.lock().expect("SMTP env lock poisoned");
let previous = std::env::var("SMTP_HOST").ok();
std::env::remove_var("SMTP_HOST");
Self { previous, _lock: lock }
}
} #[test]
fn test_email_validate_config_no_smtp_host_fails() {
+ let _guard = SmtpHostGuard::unset();
// No global SMTP and no integration SMTP should fail validation
let dispatcher = create_dispatcher(ChannelType::Email);
let config = json!({});🤖 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/server/tests/unit/notification_test.rs` around lines 220 - 228, The test
test_email_validate_config_no_smtp_host_fails is flaky because it doesn't ensure
SMTP_HOST is unset or hold SMTP_ENV_LOCK for the test duration; change the test
to acquire the SMTP_ENV_LOCK for the whole test and explicitly remove SMTP_HOST
from the environment before calling dispatcher.validate_config so the
environment state is deterministic. Also update SmtpHostGuard so it actually
owns the MutexGuard (store a field like _lock: std::sync::MutexGuard<'static,
()> in the struct returned by SmtpHostGuard::set and SmtpHostGuard::unset) so
the lock is held while the guard exists, and adjust callers (including
test_email_validate_config_valid and
test_email_validate_config_recipients_are_routing_concern) to use the guard's
lifetime to serialize access to the SMTP_HOST env var.
1 participant
Summary
alert_integrationstable (global credentials only) replacingnotification_channels; addsrouting_override JSONBtoalert_rule_channelsjunction for per-rule routing/api/alert-channels→/api/integrationsvalidate_routing_overrideenforcing provider-specific routing rules (Slack bot_token requires channel, Email requires recipients with @, Webhook requires a URL somewhere)POST /api/integrations/{id}/testnow accepts optional{ routing_override }body and validates before dispatchalert_history.channel_idrenamed tointegration_id, FKs preservedAlertRuleResponse.channel_ids→integration_ids; deduplicates channels before INSERTTest plan
cargo build— zero compile errorscargo test— all tests pass (149 unit tests, 95 integration, 71 ignored)cargo clippy -- -D warnings— zero warningssqlx migrate runon a DB with existingnotification_channelsdata completes without errorGET /api/integrations/{id}for Slack bot_token returns"token": "xoxb-****"POST /api/projects/{id}/alert-ruleswith Slack bot_token +routing_override:{channel:"#fe"}returns 201POST /api/projects/{id}/alert-ruleswith Webhook and no URL in credentials or routing returns 422POST /api/integrations/{id}/testwith optional routing_override body works end-to-endis_enabled: false) does not receive dispatched alertsSummary by CodeRabbit
Release Notes
New Features
API Changes
/api/alert-channelsto/api/integrationswith updated request/response schemas.