PagerDuty proxy parity (#1945)

[erikdarlingdata]

Follow-through on #1943: pagerduty_proxy threads the sibling channels' exact shape end to end - IAlertSettings, the send call (was proxyAddress: null unconditionally), both test buttons, both Settings windows with a proxy row, Lite's plain settings.json pref, Darling's non-secret column tier (V43 migration, both viewer column lists, upsert $19, round-trip, managed-roles + provisioning grants), and a V43 viewer probe sentinel. The version-gate and migration-pin tests caught exactly their two designed spots and were updated with the arm. Lite 1952/1952, Darling 4040/4050 (10 usual gated skips) on a live rig, all apps 0 warnings.

🤖 Generated with Claude Code

[claude]

Bug: this grid's Grid.RowDefinitions still only declares 3 rows, but content now uses Grid.Row="3" (this StackPanel).

The PagerDuty Grid above (line ~636) declares only 3 <RowDefinition Height="Auto"/> entries (indices 0-2). This hunk adds a new row-2 (Proxy Address) and moves the test-button StackPanel to row 3 — but no 4th RowDefinition was added, so row 3 has no allocated height. The button/status text will render collapsed or overlapping the Proxy Address field instead of appearing below it.

Looks like the row that should have been added here landed in the wrong grid instead: the Teams grid earlier in this file (~line 554) picked up an extra, unused RowDefinition with mismatched indentation (20 spaces vs. its siblings' 24) — that block only uses rows 0-2, so the addition there is harmless clutter, but it's the edit that belongs in this PagerDuty grid.

Lite/Windows/SettingsWindow.xaml's equivalent PagerDuty grid correctly has 4 RowDefinitions — this is a Lite/Darling parity gap from what looks like a misapplied patch.

Fix: add a 4th <RowDefinition Height="Auto"/> to this grid's Grid.RowDefinitions, and drop the stray one from the Teams grid.

[claude]

Review summary

Reviewed the PagerDuty proxy parity work end to end (settings model, migration, viewer probe, grants, both Settings windows, both apps' test-button wiring).

Correctness / Lite-Darling parity: Found one real bug — left an inline comment on Darling/PerformanceMonitor.Darling.Viewer/SettingsWindow.xaml. The PagerDuty Grid still declares only 3 RowDefinitions but its content now uses Grid.Row="3" for the test-button row, so that row has no allocated height and will render collapsed/overlapping. The RowDefinition that should have gone into the PagerDuty grid instead landed as a stray, harmless extra row in the Teams grid ~90 lines earlier (visible from its mismatched indentation). Lite's equivalent grid is correct (4 RowDefinitions) — this is purely a Darling-side layout regression from what looks like a misapplied patch.

Everything else checks out:

• IAlertSettings.PagerDutyProxyAddress is threaded consistently through both apps: AppAlertSettings/App.xaml.cs (Lite, plain settings.json pref, matching the sibling non-secret proxy fields) and DarlingAlertSettings/DarlingConfig/StoreConfigProvider (Darling).
• WebhookAlertService.PostWebhookAsync/GetHttpClient proxy plumbing is pre-existing (shared with Teams/Slack/generic) and reused as-is — no new attack surface.
• Darling V43 migration, NotificationColumns/NotificationUpsertSql ($19), both viewer projections (full + non-secret), NotificationRow, DarlingManagedRoles, and provision-roles.sql are all updated together and stay in sync — pagerduty_proxy is correctly non-secret (viewer SELECT grant) like the sibling *_proxy columns.
• MapProbedSchemaVersion's new V43 arm is newest-first as required, with the version-gate and migration-pin tests updated to match (Scripts[^1].Version == 43, StorageVersion.SchemaVersion == 43, extra true arg in the full-probe test).
• Deprecated Dashboard stub classes and the four test double classes got the new interface member added, so nothing breaks the build.
• The unrelated-looking CHANGELOG.md additions are just filling in previously-dangling [#nnn] reference-link definitions that were already used in older entries — not a functional change.

No SQL injection, secrets-handling, or missing-index concerns found (raw T-SQL/PL-pgSQL surface in this PR is limited to the additive ALTER TABLE ... ADD COLUMN IF NOT EXISTS in V43, which follows the existing style).

[claude]

This 4th RowDefinition was added to the Teams grid by this PR, but the Teams grid only uses rows 0-2 (Webhook URL, Proxy Address, test button) — it's unused here. This looks like a misplaced edit for the row that the PagerDuty grid actually needs further down (see the comment around line 639) since it now uses Grid.Row="3".

Suggested change

	<RowDefinition Height="Auto"/>
	<RowDefinition Height="Auto"/>
	</Grid.RowDefinitions>

[claude]

This StackPanel now uses Grid.Row="3", but the enclosing Grid.RowDefinitions above (around line 636-639) still only declares 3 rows (0-2). WPF clamps an out-of-range Grid.Row to the last defined row, so this StackPanel (Send Test Notification button + status text) will render on top of row 2 — the new Proxy Address textbox — instead of getting its own row.

It looks like the 4th RowDefinition this grid needs was accidentally added to the Teams grid instead (flagged separately at line 554), which doesn't need it. Compare with Lite's equivalent PagerDuty grid (Lite/Windows/SettingsWindow.xaml), which correctly has 4 RowDefinitions for the same 4 rows.

[claude]

UpdatePagerDutyControlStates() (further down, around line 1161-1167) never toggles PagerDutyProxyAddressBox.IsEnabled. Every sibling channel does this — UpdateTeamsControlStates/UpdateSlackControlStates/UpdateGenericControlStates all set their *ProxyAddressBox.IsEnabled = enabled; — and Darling's own UpdatePagerDutyControlStates (added in this PR) does set PagerDutyProxyAddressBox.IsEnabled = enabled;. As written, Lite's PagerDuty proxy textbox stays enabled/editable even when "Enable PagerDuty notifications" is unchecked — a within-app sibling-channel inconsistency and a Lite/Darling parity drift.

Suggested fix in UpdatePagerDutyControlStates():

Suggested change

	PagerDutyProxyAddressBox.Text = App.PagerDutyProxyAddress;
	PagerDutyProxyAddressBox.Text = App.PagerDutyProxyAddress;

(and add PagerDutyProxyAddressBox.IsEnabled = enabled; next to PagerDutyEuRegionCheckBox.IsEnabled = enabled; in UpdatePagerDutyControlStates())

[claude]

Review: PagerDuty proxy parity (#1945)

Solid, narrowly-scoped follow-through on #1943 — threads pagerduty_proxy through IAlertSettings, the actual send call (fixing the proxyAddress: null hardcode), both Settings windows, Lite's plain pref, and Darling's V43 migration + viewer non-secret column tier (upsert $19, managed-roles grant, provision-roles.sql grant, and the V43 probe sentinel). Column counts, upsert parameter ordinals, and reader offsets all line up correctly end to end, and the schema-probe/migration-pin tests were updated consistently with the rest of that ladder.

Found two concrete bugs, both left inline:

1. Darling Settings window — UI overlap bug. In SettingsWindow.xaml, the PagerDuty Grid.RowDefinitions still only declares 3 rows, but the new Proxy Address row pushed the test-button StackPanel to Grid.Row="3". WPF clamps an out-of-range Grid.Row to the last defined row, so the Send Test Notification button/status text will render on top of the Proxy Address textbox. The 4th RowDefinition this grid needed appears to have been added to the Teams grid instead (which doesn't need it) — looks like a misplaced edit. Lite's equivalent grid has the correct 4 RowDefinitions.

2. Lite/Darling parity drift + within-app inconsistency. Lite's UpdatePagerDutyControlStates() never sets PagerDutyProxyAddressBox.IsEnabled, unlike every sibling channel (UpdateTeamsControlStates/UpdateSlackControlStates/UpdateGenericControlStates all toggle their proxy box) and unlike Darling's own UpdatePagerDutyControlStates in this same PR. The proxy field stays enabled/editable in Lite even when PagerDuty notifications are disabled.

No security concerns beyond what's already inherent to the shared PostWebhookAsync/GetHttpClient proxy plumbing (unchanged here, and already exercised by Teams/Slack/Generic). No missing-index or other DMV-folklore suggestions, per repo convention.

[claude]

Review: PagerDuty proxy parity (#1945)

Went through the full diff column-by-column across both apps. This is a clean, mechanical follow-through — I could not find a correctness, Lite/Darling parity, or security issue.

What I verified:

• IAlertSettings.PagerDutyProxyAddress is implemented on every concrete/adapter/test-fake class that implements the interface (Lite's AppAlertSettings, Darling's DarlingAlertSettings, both SettingsWindow.xaml.cs test doubles, both deprecated Dashboard adapters, DarlingAnalysisPipelineTests, PagerDutyWebhookTests, WebhookCooldownSeedTests) — no build-breaking gap.
• WebhookAlertService.TrySendPagerDutyAlertAsync now passes _settings.PagerDutyProxyAddress into PostWebhookAsync instead of the old hardcoded proxyAddress: null, matching the Teams/Slack/Generic call sites exactly (GetHttpClient(proxyAddress) handles null/empty the same way for all four).
• Darling's V43 migration (pagerduty_proxy text NOT NULL DEFAULT ''), StorageVersion.SchemaVersion = 43, and the migration-pin/version-gate tests all move together.
• Column ordinals check out end-to-end: StoreConfigProvider SELECT list → reader indices (18), ViewerDataService.Notification.cs full-projection reader (18) and no-secret-projection reader (11), and the $19 upsert parameter — all consistent with the appended-last column.
• DarlingManagedRoles.ViewerRestrictedConfigTables and provision-roles.sql's hand-written GRANT SELECT both gained pagerduty_proxy as non-secret — and I confirmed ProvisionRolesAclDriftTests set-equality-checks exactly these two lists, so a mismatch here would fail loudly rather than silently reintroducing the Darling: close the BYO provision-roles.sql viewer column-ACL drift #1639-style BYO-role drift this repo already got bitten by once.
• MapProbedSchemaVersion's new hasPagerDutyProxy arm is newest-first and gates the viewer connect at V43, so a viewer build never issues the new pagerduty_proxy-naming SELECT/UPSERT against a pre-V43 store.
• Lite's pagerduty_proxy_address settings.json round-trip (load, save, enable/disable control state) mirrors the teams_proxy_address/generic_proxy_address pattern exactly — plain pref, not routed through Credential Manager, correctly so since it's not a secret.
• XAML: both Settings windows (Lite and Darling) got the same new grid row + row-index bump for the button StackPanel below it, and UpdatePagerDutyControlStates enables/disables the new box in Darling (Lite's equivalent already does the same).
• CHANGELOG's new reference-link definitions at the bottom (#110, #111, #138, ... #1940) look unrelated to this PR's stated purpose — appear to be pre-existing broken/missing reference links in the file getting fixed incidentally. Worth a sanity check that this wasn't an unintended merge artifact, but it's harmless either way (pure doc-link fixes).

Minor, non-blocking nits (not worth blocking on):

• ViewerDataService.Notification.cs: row.PagerDutyProxy ?? "" at the $19 parameter is redundant — PagerDutyProxy is a non-nullable string defaulted to "", and the sibling proxy parameters (TeamsProxy, SlackProxy, GenericProxy) don't null-coalesce. Harmless, just a small inconsistency with the established pattern in the same file.
• NotificationRow.ValueEquals compares PagerDutyProxy == other.PagerDutyProxy while every other string field in that method uses string.Equals(..., StringComparison.Ordinal). Functionally identical (C# == on string is ordinal), just not visually consistent with its neighbors.
• WebhookAlertService.SendTestPagerDutyAsync's new proxyAddress parameter is appended at the end as = null after branding, whereas SendTestTeamsAsync/SendTestSlackAsync take proxyAddress as a required parameter positioned right after the URL. Not a bug (both call sites already pass it), just a shape inconsistency with siblings.

No missing-index or other DMV-folklore suggestions, per repo convention. Nice, thorough parity work.