[erikdarlingdata]

Part of #3285. Fixes #3334 (Fixes only auto-closes on dev->main, so close it manually on dev-merge).

The last correctness item before the client nightly: the resolve-on-delete grant gap flagged in #3305/#3333.

The gap

CustomAlertEvaluator.WriteTeardownResolutionAsync writes the "<name> Resolved" recovery row to config.config_alert_log, but only admin/owner may INSERT there. On the two least-privilege delete paths -- the mcp delete_custom_alert_rule tool and the viewer DELETE /api/alerts -- the INSERT was permission-denied, failure-isolated (swallowed), and the row silently dropped, leaving a deleted firing rule showing "open" in history forever. #3305's tests missed it because they connect as the owner.

Fix — a scoped SECURITY DEFINER function (Erik's greenlit approach, over a blanket INSERT grant)

DarlingManagedRoles provisioning gains, via a shared builder BuildCustomAlertResolveFunctionSql:

CREATE OR REPLACE FUNCTION config.record_custom_alert_resolution(
   p_server_id integer, p_server_name text, p_metric_name text, p_detail_text text)
RETURNS void
LANGUAGE sql
SECURITY DEFINER
SET search_path = config, pg_catalog
AS $fn$
   INSERT INTO config_alert_log
      (alert_time, server_id, server_name, metric_name, current_value, threshold_value,
       alert_sent, notification_type, send_error, muted, detail_text, context_json)
   VALUES
      ((now() AT TIME ZONE 'UTC'), p_server_id, p_server_name, p_metric_name, 0, 0,
       false, 'none', NULL, false, p_detail_text, NULL);
$fn$;
REVOKE ALL ON FUNCTION config.record_custom_alert_resolution(integer, text, text, text) FROM PUBLIC;
GRANT EXECUTE ON FUNCTION config.record_custom_alert_resolution(integer, text, text, text) TO viewer, mcp;

Definer-safety (first SECURITY DEFINER function in the codebase, so each property is deliberate):

• owner: created by the provisioning connection (the store owner), which holds the config_alert_log INSERT the body runs with; CREATE OR REPLACE by the owner keeps ownership.
• pinned search_path config, pg_catalog: no caller search_path can redirect the unqualified config_alert_log or now().
• parameters only, no dynamic SQL: a static, fully-parameterized INSERT.
• thin + shape-locked: writes ONE row and HARDCODES the no-channel resolution shape (alert_sent false, notification_type 'none', current/threshold 0, unmuted, no send_error/context) -- byte-identical to what RecordAlertAsync writes for a BuildResolutionRecord. A grantee can page nothing and forge no fire; only server_id/name and the already-sanitized title/detail vary.
• REVOKE ALL FROM PUBLIC before the narrow GRANT EXECUTE TO viewer, mcp (a fresh function is PUBLIC-executable by default).
• provisioning, not a migration: CREATE OR REPLACE is idempotent + owner-run every start with no probeable schema footprint, so no SchemaVersion bump (which would drag in the probe rung + ladder fixture + version pins).

Routing: WriteTeardownResolutionAsync (the delete AND sweep-reconcile teardown writer) now calls PgAlertHistoryStore.RecordCustomAlertResolutionAsync → SELECT config.record_custom_alert_resolution(...) on the caller's pool (runs as owner via the definer). Normal fire/resolve history writes (the owner service path, DeliverFireAsync/DeliverResolveAsync) stay on RecordAlertAsync, unchanged.

Provisioning-pin test updates

Added BuildProvisioningSql_CreatesResolveDefinerFunction_ScopedAndGrantedToViewerAndMcp to DarlingManagedRolesTests: pins the CREATE, SECURITY DEFINER, the pinned search_path, the PUBLIC-revoke-before-EXECUTE-grant order, the locked resolution shape, and the ABSENCE of any blanket config_alert_log grant. The existing pins are unaffected (all substring Assert.Contains/DoesNotContain; the new section is additive).

The proof test #3305 lacked (gated live, run green on a local PG 18.4 + TimescaleDB rig)

CustomAlertResolveGrantLiveTests connects as a disposable low-privilege role (viewer/mcp-shaped: SELECT + narrow custom_alert_rules write + EXECUTE on the function, and no direct config_alert_log INSERT):

• delete path: a direct config_alert_log INSERT by the role is asserted denied (42501); then ResolveAndDeleteRuleAsync on the low-priv pool writes the <name> Resolved row (asserted present + a proper no-channel resolution shape) and deletes the rule. A present row can only have come through the definer function.
• sweep-reconcile path: the reconcile teardown with a low-priv history pool writes the resolution row the same way.

CustomAlertTeardownLiveTests now provisions the function in its fixture (the teardown path depends on this provisioning artifact; those tests migrate but do not provision). All 9 custom-alert live tests pass on a fresh DB.

Out of scope (flagged, not changed)

The BYO Darling/tools/provision-roles.sql (bring-your-own-Postgres, viewer-only, no mcp) is a separate hand-maintained surface with its own drift guard. A BYO viewer's DELETE /api/alerts would still hit the gap. It was not in the stated scope (which named DarlingManagedRoles), and adding a SECURITY DEFINER function to the shipped BYO script is a security-surface decision for the operator -- worth a follow-up decision.

Build & full-suite results (post-rebase onto current dev)

• Build 0 warnings / 0 errors.
• Darling.Tests: 8750 total, 0 errors, 4 failed, 350 skipped (live-gated). Three are the known worktree/platform baseline (FleetIdentifierScrubTests x2 "scanned 0 files", NpgsqlRootCertificateValidationTests x1 TLS). The fourth, IncidentAttachmentDeliveryTests.TheFourNonEmailChannels_PostIdenticalBytes_WithAndWithoutIncidentAttachments, is a test-ISOLATION issue in dev's newly-merged [BUG] AttachmentXml is per-alert but the cards are per-incident: per-event mode emails N cards that all attach the FIRST graph in the window #3330 class: it passes in isolation and fails only in the full parallel run, and has no code path to this change (it exercises delivery-channel bytes; this touches config_alert_log resolution provisioning). It affects any branch off current dev -- flagging for a dev-side look, not caused here.
• Lite.Tests: 3713 total, 0 errors, 4 failed -- all the known AuroraOnlySqlIsGatedTests worktree source-scan baseline; no Lite files touched.

No MCP tool added, so the tool-inventory pins do not apply.

🤖 Generated with Claude Code

https://claude.ai/code/session_01WejwpgWF5Xfm3fAmoEbFz4

[claude]

Reviewed the diff (CustomAlertEvaluator.cs, DarlingManagedRoles.cs, PgAlertHistoryStore.cs + the three test files). No blocking issues found.

Parity: Custom alert rules are Darling-only (no custom_alert_rules/CustomAlertEvaluator under Lite/), so there's no Lite counterpart to keep in sync here — correctly out of scope.

Security definer function: Checked the definer-safety properties claimed in the PR description against the actual SQL in BuildCustomAlertResolveFunctionSql — owner-created, pinned SET search_path = config, pg_catalog, no dynamic SQL, REVOKE ALL FROM PUBLIC precedes the narrow GRANT EXECUTE, and the write is fully parameterized ($1-$4) with the delivery-shape columns hardcoded. All hold up. I also checked whether the Custom Views v2 compose surface (which also touches the viewer/mcp roles) could be abused to invoke this function with attacker-chosen parameters — it can't: the compose compiler only ever emits SQL built from a hardcoded C# catalog (MeasureCatalog), never caller-supplied function names or text, so the GRANT EXECUTE is reachable only through the intended RecordCustomAlertResolutionAsync code path.

Correctness: Verified WriteTeardownResolutionAsync's new call routes title/message into the same metric_name/detail_text columns the old BuildResolutionRecord path wrote them into, so the on-disk shape is unchanged. The removed MetricNameFor(ruleId) computation was dead in this path even before this PR (never read by BuildResolutionRecord), so no behavior loss there. Confirmed ResolveAndDeleteRuleAsync and the sweep-reconcile path both run the history write on the caller's own pool, matching the low-privilege delete paths the PR targets.

Minor/non-blocking nit: RecordCustomAlertResolutionAsync's metricName parameter (and the SQL function's p_metric_name) actually receives the alert title (e.g. "<rule> Resolved"), not the true metric key ("Custom:<rule_id>") — this mirrors the pre-existing AlertResolution.MetricName/BuildResolutionRecord mapping rather than being a new defect, but the parameter name in the new code is a bit misleading for a reader who doesn't already know that history. Not blocking.

Tests (CustomAlertResolveGrantLiveTests, the DarlingManagedRolesTests addition, CustomAlertTeardownLiveTests fixture update) look solid — the gated live tests specifically assert a direct low-priv INSERT is denied (42501) before proving the resolve row lands via the function, which is exactly the proof #3305's tests were missing.