Make Darling's MCP alert payload and its writer agree (#2417)

[erikdarlingdata]

Closes #2417.

get_alert_settings and update_alert_settings are the read and write halves of one file, and they had drifted apart in both directions at once. Six columns AlertSettingsReadRow reads were emitted to nobody, and one key the payload did emit was refused by the writer.

The six are the V33 connection opt-ins and the V35/V37 Availability Group family. The reader was widened for them on purpose — its own comment says an MCP client could not otherwise see them at all — but the payload never followed, so every call pulled them out of the store and dropped them on the floor. The AG four are the ones that cost something, because those are the settings whose misconfiguration is least visible anywhere else: an agent asked why nothing alerted when a replica fell behind could not see the lag threshold, could not see whether AG notification was on at all, and had no way to tell "configured not to alert" apart from "failed to alert".

In the other direction the payload emitted blocking.wait_threshold_seconds and the blocking arm of the update switch did not take it, so handing a whole read payload back to the writer — the round trip the tool's own description tells the caller to perform — was rejected with Unknown field, naming a field the caller never chose to send.

What the fix decides

All six are writable, not merely readable. Every column on this row is a knob the Viewer's Settings window already writes, so a read-only knob just moves an operator to a hand-typed UPDATE — and read-only would have needed an exemption entry in the new test, which is the same hand-maintained fact that let the six go missing. Every bound mirrors DarlingAlertSettings' clamp exactly: Clamp(0, 1440) on both re-fires, Clamp(0, 86400) on the lag, Clamp(0L, 1073741824L) on the redo queue, Math.Max(0, ...) on the blocking wait. A bound that differs would let the tool accept a value the engine silently rewrites on read, which presents as the setting not sticking and nothing saying no. Zero stays in range on all four numerics — it is the shipped configuration meaning that gate is off, and the redo queue ships at zero precisely because a healthy queue size is workload-specific. The redo queue binds through a new AddLong rather than AddInt because it is the one bigint on the row and its ceiling only happens to fit in an int today.

The wire keys are the house's, not transliterations of the columns. AG becomes a group like every other alert family, with its master switch named ag.enabled for the same reason cpu/blocking/pvs/database_state spell theirs that way — inside this payload enabled always means "this alert family is on", and notify_ag_health IS that switch rather than a second opt-in behind one. Its thresholds take the <what>_threshold_<unit> spelling the neighbours already use (blocking.wait_threshold_seconds, poison_wait.threshold_ms, low_disk.threshold_gb) instead of the columns' older _alert_<unit> suffix, which appears nowhere on the wire.

The two connection settings stay top-level beside notify_connection_changes rather than becoming a connection group. The family's master switch shipped as a top-level key and Lite emits it there too, so a group could only ever have held two of the three — splitting one family across two levels of the document is worse for a reader than two extra top-level keys. Their spellings are the store's column names, which are also the keys Lite already reads out of settings.json, so Lite's payload can adopt them verbatim when it follows.

"notify_connection_changes": true,
"notify_connection_down_at_startup": true,
"connection_refire_minutes": 30,
...
"ag": { "enabled": true, "lag_threshold_seconds": 300, "redo_queue_threshold_kb": 1048576, "disconnect_refire_minutes": 30 },

The part worth keeping

The existing tests pin individual keys, which is exactly why six missing keys and one unroundtrippable key both survived. The new one is derived: it builds a settings row, runs it through the real BuildAlertSettingsPayload, serializes it through the same options the tool serializes with, and feeds the result to the real BuildAlertSettingsUpdate — then asserts the set of columns that comes out equals the set of columns AlertSettingsSelectSql reads.

One equality closes all three directions. A column read but not emitted is missing from the write set. A key emitted but refused shows up as a parse error naming itself. And a key emitted for a column nobody reads fails the direction no hand-maintained list would ever have noticed. There are no exemptions, deliberately — if a genuinely read-only column ever arrives it gets exempted by name with its reason, rather than the equality being loosened.

A second test holds the clamp parity for everything this PR made writable, in the shape FileGrowthWriteBounds_MatchTheEngineClamps already established, including asserting that the two booleans reach the engine unclamped so the day a range is added to one of them is loud.

Verification

Darling.Tests targets net10.0-windows and cannot run on macOS, so the invariant was executed against the real compiled assemblies through a throwaway net10.0 harness (assembly named Darling.Tests so the InternalsVisibleTo grants apply) running the new test's exact logic. Red/green:

built against	result
this branch	all three assertions pass
dev as-is	fails at Unknown field 'blocking.wait_threshold_seconds'
dev + only the blocking arm added	fails naming exactly the six: ag_disconnect_refire_minutes, ag_lag_alert_seconds, ag_redo_queue_alert_kb, connection_refire_minutes, notify_ag_health, notify_connection_down_at_startup

That third row is the one that matters — it shows the invariant catches each defect independently rather than one masking the other.

Every clamp literal the parity test asserts was grepped out of DarlingAlertSettings.cs and DarlingMcpAlertTools.cs before being written down. Service, Viewer, Darling.Tests and Lite.Tests all build clean.

Nothing about the SELECT or the by-ordinal read changed. Every column was already being read, which is what made this a payload defect rather than a schema one, so AlertSettingsSelect_ColumnCount_MatchesTheOrdinalsRead is untouched and still holds.

PerformanceMonitor.Common gains an InternalsVisibleTo for Darling.Tests so the new test serializes through McpHelpers.JsonOptions itself rather than a look-alike — a naming policy added there can never leave the invariant asserting keys no client receives. Lite.Tests and Dashboard.Tests were already on that list.

Lite followed in the same PR, because CI now requires it

The first CI run went red on Lite.Tests, and correctly. #2410 landed on dev between this branch's base and that run: GetAlertSettings_ReportsEveryGroupDarlingDoes_SpelledDarlingsWay parses Darling's BuildAlertSettingsPayload and requires Lite's payload to carry every group and key it finds. So the "Darling moves first, Lite follows separately" plan the issue describes is no longer available — the test named exactly the three additions (missing group 'notify_connection_down_at_startup'; missing group 'connection_refire_minutes'; missing group 'ag'). That test is doing what it was written to do, so Lite follows here rather than the test being weakened.

Six of the seven needed no new plumbing. App.NotifyConnectionDownAtStartup / App.ConnectionRefireMinutes have driven the connection-alert path since #1659, and App.NotifyAgHealth / App.AgLagAlertSeconds / App.AgRedoQueueAlertKb have driven all four AG conditions since #1726. Only the MCP surface was narrow — the same gap #2394 closed for the other nine groups.

The seventh is a real asymmetry, exempted by name rather than faked. Darling's ag_disconnect_refire_minutes is #1696 / store V37, and Lite has no AG disconnect re-fire at all — no static, no settings.json key, no edge state that could re-announce. Emitting a constant 0 would tell an agent it can tune something Lite cannot, which is the reasoning this file already applies to self_alerts, here at member granularity instead of group. LiteOmittedMembers holds the one entry, and a new test asserts both halves still hold — Darling still emits it (so the exemption cannot become dead weight hiding a Darling regression) and Lite still does not (so it cannot mask a key that has since arrived). The set is pinned exactly, so a second exemption has to be justified in that test the way a second Lite-only group has to be justified in the smtp assertion.

Lite.Tests cannot run on macOS either, so the parity test's parser was ported to Python and validated against the test's own self-checks before being trusted — the 15–40 group count, cpu/analysis/self_alerts present, smtp absent, and cpu's members exactly enabled/threshold_percent/mode. Then:

run against	result
dev's Lite payload	reproduces CI's failure message byte for byte
this branch	passes
this branch, exemption removed	fails on ag.disconnect_refire_minutes and nothing else

That last row is what shows the exemption is both load-bearing and minimal. Darling's own round-trip invariant re-ran green after the merge.

Deferred

#2426 — Lite has no AG disconnect re-fire, so a replica disconnected for a week is announced once where Darling can re-announce it. That is #1696's Lite half: a static, a clamp on load and save, a Settings box, and real edge state. It is a feature, not a payload key, and does not belong in a payload-symmetry change. Filed with the mechanism, the four steps to close it, and the note that ConnectionRefireMinutes and the AG re-fire are the same #1674 shape and may want one shared helper.

The CHANGELOG entry is also deliberately not here — see the comment below. [3.5.1] is being cut in #2395, so adding the section here would put this PR in conflict with a release PR over a file it otherwise does not touch. The bullet is written out ready to paste, and the safe order is #2395 first.

🤖 Generated with Claude Code

[claude]

Reviewed the diff against DarlingAlertSettings.cs, DarlingAlertReader.cs, and the Postgres migrations (PgMigrations.cs) rather than just the PR body's claims. Summary: this checks out.

Ordinal safety — walked all 58 columns in AlertSettingsSelectSql against the 58 positional reader.Get* calls in GetAlertSettingsAsync (unchanged by this PR) and against BuildAlertSettingsPayload's field list: every column read is emitted, by name not position, so the new fields can't silently shift anything.

Clamp parity — cross-checked every new/newly-writable bound against DarlingAlertSettings.cs:

• connection_refire_minutes AddInt(0, 1440) ↔ Math.Clamp(_config.Alerts.ConnectionRefireMinutes, 0, 1440) ✓
• ag.lag_threshold_seconds AddInt(0, 86400) ↔ Math.Clamp(AgLagAlertSeconds, 0, 86400) ✓
• ag.redo_queue_threshold_kb AddLong(0L, 1073741824L) ↔ Math.Clamp(AgRedoQueueAlertKb, 0L, 1073741824L) ✓ — and the column really is bigint in PgMigrations.cs, so the new AddLong path (vs. AddInt) is the right call, not overengineering.
• ag.disconnect_refire_minutes AddInt(0, 1440) ↔ Math.Clamp(AgDisconnectRefireMinutes, 0, 1440) ✓
• blocking.wait_threshold_seconds AddInt(0, int.MaxValue) ↔ Math.Max(0, BlockingWaitSecondsThreshold) ✓ (this is the previously-unroundtrippable key from Darling's MCP alert payload drops six settings it reads, and emits one key its writer rejects #2417's second bug — confirmed the writer arm was genuinely missing on dev, not just untested)
• Booleans (notify_connection_down_at_startup, ag.enabled) correctly reach the engine unclamped, matching the pass-through properties.

Injection/security — column names in the UPDATE ... SET are all switch-case string literals, never caller input; values are bound NpgsqlParameters throughout, including the new AddLong path. No new attack surface.

Lite/Darling parity — Lite's McpAlertTools.cs has no update_alert_settings (read-only by design, per its own tool description), so there's no writer-side counterpart to drift here. The read-side gap (Lite's get_alert_settings still omitting these 7 keys) is explicitly called out in the PR's own "Deferred" section rather than silently left — consistent with #2417 noting Darling moves first. Confirmed App.NotifyAgHealth/AgRedoQueueAlertKb etc. already exist in Lite/App.xaml.cs, so the follow-up PR should be a straight payload addition, not new plumbing.

Test approach — the new derived round-trip test (build row → real BuildAlertSettingsPayload → real McpHelpers.JsonOptions serialization → real BuildAlertSettingsUpdate → assert column-set equality) is a materially stronger regression guard than the pre-existing pinned-key tests, and the InternalsVisibleTo addition for Darling.Tests is scoped to exactly what's needed (McpHelpers.JsonOptions).

No correctness, security, or parity issues found. One non-blocking observation: CHANGELOG.md's [3.5.0] entry doesn't yet mention this fix, but that may be intentionally left for release time — not raising as a blocker.

[erikdarlingdata]

CHANGELOG: deliberately not in this PR, and why

[3.5.1] does not exist on dev — it is being cut in #2395, which is populated and dated today. Adding the section here to hold one bullet would put this PR in conflict with a release PR over a file it otherwise does not touch, which is a bad trade for a one-line entry. So the entry is not in the diff; it is here, ready to paste, and the safe order is #2395 merges first, then this bullet lands (either folded into #2395 before it merges, or as a one-line follow-up).

It belongs under ## [3.5.1] → ### Added, directly after the #2391 file-growth entry — same shape of defect (a knob wired through the store and the engine and exposed nowhere), same precedent for the exact-clamp-parity rule.

- **Darling's MCP alert payload and its writer agree again** ([#2417]) - `AlertSettingsReadRow` reads six columns `get_alert_settings` emitted to nobody (`notify_ag_health`, `ag_lag_alert_seconds`, `ag_redo_queue_alert_kb`, `ag_disconnect_refire_minutes`, `notify_connection_down_at_startup`, `connection_refire_minutes`), so they were pulled out of the store on every call and dropped on the floor - the reader had been widened for them deliberately and the payload never followed. The Availability Group four are the ones that cost something, because they are the settings whose misconfiguration is least visible anywhere else: an agent asked why nothing alerted when a replica fell behind could not see the lag threshold, could not see whether AG notification was on at all, and had no way to tell "configured not to alert" apart from "failed to alert". In the other direction `get_alert_settings` emitted `blocking.wait_threshold_seconds` and `update_alert_settings` rejected it, so handing a whole read payload back to the writer - the round trip the tool's own description tells the caller to perform - failed with `Unknown field`, naming a field the caller never chose to send. All seven are now readable AND writable: the AG family as an `ag` group whose master switch is `enabled` like every other family's and whose thresholds take the house `<what>_threshold_<unit>` spelling rather than the columns' older `_alert_<unit>` suffix, the two connection knobs top-level beside `notify_connection_changes` where their master switch already lives. Write bounds match `DarlingAlertSettings`' clamps exactly on the #2391 precedent, with zero staying valid on every numeric because zero is the shipped "this gate is off" value on all four. The guard is DERIVED rather than hand-pinned - the payload is run through the real writer and the resulting column set asserted EQUAL to the columns the reader SELECTs - which catches a column read but not emitted, a key emitted but not accepted, and a key emitted for a column nobody reads, none of which the previous per-key pins could do. Lite has the same six settings in `App` and no MCP surface for them either; that follows separately, since Darling had to move first.

Plus the link-reference line at the foot of the file, in the block #2395 already adds to:

[#2417]: https://github.com/erikdarlingdata/PerformanceMonitor/issues/2417

[claude]

Reviewed. This is a tightly-scoped, well-verified fix — I traced the full round trip (reader SELECT → AlertSettingsReadRow → BuildAlertSettingsPayload → BuildAlertSettingsUpdate → engine clamps in DarlingAlertSettings.cs) and everything lines up:

• All six previously-orphaned columns (notify_connection_down_at_startup, connection_refire_minutes, and the four AG fields) are now both emitted and accepted, with write bounds matching DarlingAlertSettings' clamps exactly (0–1440, 0–86400, 0L–1073741824L, unclamped booleans).
• blocking.wait_threshold_seconds is now accepted by the writer, closing the other direction of drift.
• The new AddLong helper is warranted — ag_redo_queue_alert_kb is genuinely the one bigint on the row, and typing it separately from AddInt avoids silently rejecting values above int.MaxValue if the ceiling is ever raised.
• Lite/Darling parity: verified Lite already has the backing statics (App.NotifyConnectionDownAtStartup, App.ConnectionRefireMinutes, App.NotifyAgHealth, App.AgLagAlertSeconds, App.AgRedoQueueAlertKb) wired since Connection alerts never fire for a server that is already down, and never re-fire during a standing outage #1659/Lite gets the Availability Group alerts, off a shared policy (#1696) #1726, so Lite's GetAlertSettings update is pure surface, no new alerting behavior. The one deliberate asymmetry — Lite omitting ag.disconnect_refire_minutes because it has no re-fire mechanism at all — is exempted by name and pinned by a test asserting both that Darling still emits it and Lite still doesn't, so the exemption can't silently go stale. Confirmed Dashboard's MCP tools are under deprecated/ and out of scope.
• Both get_alert_settings tool descriptions (Darling and Lite) were updated in parallel to mention AG health and connection loss — no drift there.
• No T-SQL in this diff (pure C#/Postgres-store), so the T-SQL style rules don't apply; standard C# conventions (naming, nullable handling) are followed.

Didn't find correctness, security, or parity issues to flag. The derived round-trip test (EveryColumnRead_IsEmittedByThePayload_AndAcceptedByTheWriter) is a good structural guard against this class of bug recurring — nice that it's an equality rather than a subset check.