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

Darling/Darling.Tests/DarlingMcpAlertToolsTests.cs

@@ -12,6 +12,7 @@
 using System.Linq;
 using System.Reflection;
 using System.Text.Json;
+using System.Text.Json.Nodes;
 using System.Threading;
 using System.Threading.Tasks;
 using Microsoft.Extensions.DependencyInjection;
@@ -219,6 +220,173 @@ public void AlertSettingsSelect_ColumnCount_MatchesTheOrdinalsRead()
         Assert.Equal(columns - 1, ordinals.Max());
     }
 
+    /// <summary>
+    /// The round-trip invariant, DERIVED rather than hand-pinned: the set of columns the reader SELECTs must
+    /// equal the set of columns <c>update_alert_settings</c> writes when it is handed <c>get_alert_settings</c>'
+    /// own payload. That is the round trip the tool's description tells a caller to perform — read the
+    /// settings, change one number, write them back — asserted as one set comparison rather than as a list of
+    /// keys somebody has to remember to extend.
+    ///
+    /// <para>#2417 found the drift running in BOTH directions at once, which is why the assertion is an
+    /// equality and not a subset. Six columns were read out of the store on every call and emitted to nobody
+    /// (the AG four and the connection two) — those are missing from the write set. And one key WAS emitted
+    /// that the writer refused (<c>blocking.wait_threshold_seconds</c>), so feeding a read payload back
+    /// failed with "Unknown field", naming a field the caller never chose to send — that shows up as a
+    /// non-null parse error instead of a column set. The third direction, a key emitted for a column nobody
+    /// reads, no hand-maintained list would have noticed at all.</para>
+    ///
+    /// <para>There are NO exemptions, deliberately. Every column on this row is a knob the Viewer's Settings
+    /// window already writes, so leaving one readable-but-not-writable just moves an operator to a hand-typed
+    /// UPDATE — and it would need an exemption entry, which is the same hand-maintained fact that let six
+    /// columns go missing in the first place. If a genuinely read-only column ever arrives, exempt it BY NAME
+    /// here with its reason; do not loosen the equality.</para>
+    /// </summary>
+    [Fact]
+    public void EveryColumnRead_IsEmittedByThePayload_AndAcceptedByTheWriter()
+    {
+        var (columns, error) = ParseAsPartialUpdate(SerializedSettingsPayload(SampleSettingsRow()));
+
+        /* Every key the payload emits is accepted. The parse stops at the FIRST rejection, so a non-null
+           error here names the exact key update_alert_settings would refuse from its own read. */
+        Assert.Null(error);
+
+        /* Each accepted key claimed its own column — two keys sharing one would make whichever lost the
+           parse order a silent no-op, with the caller told both were updated. */
+        Assert.Equal(columns.Count, columns.Distinct(StringComparer.Ordinal).Count());
+
+        Assert.Equal(
+            SelectedAlertSettingsColumns().OrderBy(c => c, StringComparer.Ordinal).ToArray(),
+            columns.OrderBy(c => c, StringComparer.Ordinal).ToArray());
+    }
+
+    /// <summary>
+    /// The write bounds for everything #2417 made writable, against <c>DarlingAlertSettings</c>' clamps —
+    /// the same parity <see cref="FileGrowthWriteBounds_MatchTheEngineClamps"/> holds for the file-growth
+    /// knobs, and for the same reason: a bound that differs lets the tool ACCEPT a value the engine then
+    /// silently rewrites on read, which presents as the setting not sticking. Nothing says no.
+    ///
+    /// <para>The two booleans are asserted to reach the engine UNCLAMPED. That is the case that would
+    /// otherwise go unnoticed — a range added to one of them later needs a matching gate here, and this is
+    /// what makes the day it appears loud.</para>
+    ///
+    /// <para>Zero is IN range on all four numerics because 0 is a shipped configuration on each: no
+    /// connection re-fire, no AG lag gate, no redo-queue gate (<c>ag_redo_queue_alert_kb</c> ships at 0
+    /// precisely because a healthy queue size is workload-specific), no second blocking gate. A floor of 1
+    /// would make the shipped row unwritable through the tool that reports it.</para>
+    /// </summary>
+    [Fact]
+    public void AgConnectionAndBlockingWaitWriteBounds_MatchTheEngineClamps()
+    {
+        var tools = ReadRepoFile(System.IO.Path.Combine(
+            "Darling", "PerformanceMonitor.Darling.Service", "Mcp", "DarlingMcpAlertTools.cs"));
+        var settings = ReadRepoFile(System.IO.Path.Combine(
+            "Darling", "PerformanceMonitor.Darling.Service", "DarlingAlertSettings.cs"));
+
+        /* engine */
+        Assert.Contains("NotifyConnectionDownAtStartup => _config.Alerts.NotifyConnectionDownAtStartup;", settings, StringComparison.Ordinal);
+        Assert.Contains("_config.Alerts.ConnectionRefireMinutes, 0, 1440", settings, StringComparison.Ordinal);
+        Assert.Contains("NotifyAgHealth => _config.Alerts.NotifyAgHealth;", settings, StringComparison.Ordinal);
+        Assert.Contains("_config.Alerts.AgLagAlertSeconds, 0, 86400", settings, StringComparison.Ordinal);
+        Assert.Contains("_config.Alerts.AgRedoQueueAlertKb, 0L, 1073741824L", settings, StringComparison.Ordinal);
+        Assert.Contains("_config.Alerts.AgDisconnectRefireMinutes, 0, 1440", settings, StringComparison.Ordinal);
+        Assert.Contains("Math.Max(0, _config.Alerts.BlockingWaitSecondsThreshold)", settings, StringComparison.Ordinal);
+
+        /* tool: the same numbers, and no gate at all on the two booleans */
+        Assert.Contains("AddBool(\"notify_connection_down_at_startup\"", tools, StringComparison.Ordinal);
+        Assert.Contains("\"connection_refire_minutes\", 0, 1440", tools, StringComparison.Ordinal);
+        Assert.Contains("AddBool(\"notify_ag_health\"", tools, StringComparison.Ordinal);
+        Assert.Contains("\"ag.lag_threshold_seconds\", 0, 86400", tools, StringComparison.Ordinal);
+        Assert.Contains("\"ag.redo_queue_threshold_kb\", 0L, 1073741824L", tools, StringComparison.Ordinal);
+        Assert.Contains("\"ag.disconnect_refire_minutes\", 0, 1440", tools, StringComparison.Ordinal);
+        Assert.Contains("\"blocking.wait_threshold_seconds\", 0, int.MaxValue", tools, StringComparison.Ordinal);
+    }
+
+    /// <summary>The reader's SELECT list, split from the SHIPPED constant the same way
+    /// <see cref="AlertSettingsSelect_ColumnCount_MatchesTheOrdinalsRead"/> counts it — so it cannot drift
+    /// from what the reader actually asks the store for.</summary>
+    private static IReadOnlyList<string> SelectedAlertSettingsColumns()
+    {
+        var sql = Reader.AlertSettingsSelectSql;
+        var select = sql[(sql.IndexOf("SELECT", StringComparison.Ordinal) + 6)..
+                          sql.IndexOf("FROM config_alert_settings", StringComparison.Ordinal)];
+        return select.Split(',', StringSplitOptions.RemoveEmptyEntries)
+            .Select(c => c.Trim())
+            .Where(c => c.Length > 0)
+            .ToList();
+    }
+
+    /// <summary>get_alert_settings' payload for one row, serialized through the SAME options the tool
+    /// serializes with and re-parsed. Runtime rather than source-parsing for the reason Lite's
+    /// <c>McpAlertSettingsKeyTests</c> gives: the C# identifier is not automatically the wire key, so only
+    /// serializing proves what a client receives — and therefore what it would hand back.</summary>
+    private static JsonObject SerializedSettingsPayload(Reader.AlertSettingsReadRow row)
+    {
+        var build = typeof(DarlingMcpAlertTools).GetMethod(
+            "BuildAlertSettingsPayload", BindingFlags.NonPublic | BindingFlags.Static)!;
+        var payload = build.Invoke(null, new object[] { row })!;
+        var json = JsonSerializer.Serialize(payload, payload.GetType(), McpHelpers.JsonOptions);
+        return (JsonObject)JsonNode.Parse(json)!;
+    }
+
+    /// <summary>Runs a body through the tool's REAL partial-update parser and reports the columns it would
+    /// write plus the first validation error, if any.</summary>
+    private static (IReadOnlyList<string> Columns, string? Error) ParseAsPartialUpdate(JsonObject body)
+    {
+        var build = typeof(DarlingMcpAlertTools).GetMethod(
+            "BuildAlertSettingsUpdate", BindingFlags.NonPublic | BindingFlags.Static)!;
+        var result = build.Invoke(null, new object[] { body })!;
+        var type = result.GetType();
+        var updates = (IEnumerable<(string Column, NpgsqlParameter Param)>)type.GetField("Item1")!.GetValue(result)!;
+        return (updates.Select(u => u.Column).ToList(), (string?)type.GetField("Item2")!.GetValue(result));
+    }
+
+    /// <summary>A plausible settings row whose every value sits INSIDE the writer's bounds, so the invariant
+    /// above fails on a missing or unaccepted KEY rather than on a value. Named arguments deliberately: a new
+    /// column makes this stop compiling until someone supplies it, which is the moment to decide whether the
+    /// payload and the writer learn about it too. (Ordinal safety of the SELECT itself is
+    /// <see cref="AlertSettingsSelect_ColumnCount_MatchesTheOrdinalsRead"/>'s job, not this one's.)</summary>
+    private static Reader.AlertSettingsReadRow SampleSettingsRow() => new(
+        Enabled: true,
+        CpuEnabled: true, CpuThresholdPercent: 80, CpuMode: "sql",
+        BlockingEnabled: true, BlockingCountThreshold: 5,
+        DeadlockEnabled: true, DeadlockCountThreshold: 3,
+        PoisonWaitEnabled: true, PoisonWaitThresholdMs: 1000,
+        LongRunningQueryEnabled: true, LongRunningQueryThresholdMinutes: 5,
+        TempDbSpaceEnabled: true, TempDbSpaceThresholdPercent: 80,
+        LowDiskEnabled: true, LowDiskThresholdPercent: 10, LowDiskThresholdGb: 20,
+        LongRunningJobEnabled: true, LongRunningJobMultiplier: 3,
+        FailedJobEnabled: true, FailedJobLookbackMinutes: 60,
+        CooldownMinutes: 15,
+        ExcludedDatabases: new[] { "tempdb" },
+        AnalysisEnabled: true, AnalysisIntervalMinutes: 60,
+        AnalysisNotificationsEnabled: true, AnalysisNotifySeverity: 1.0,
+        DeliveryMode: "Summary", PerEventMax: 10,
+        LongRunningQueryMaxResults: 25,
+        LongRunningQueryExcludeSpServerDiagnostics: true,
+        LongRunningQueryExcludeWaitFor: true,
+        LongRunningQueryExcludeBackups: true,
+        LongRunningQueryExcludeMiscWaits: true,
+        LongRunningQueryExcludeCdc: true,
+        NotifyConnectionChanges: true,
+        NotifyConnectionDownAtStartup: true,
+        ConnectionRefireMinutes: 30,
+        NotifyAgHealth: true,
+        AgLagAlertSeconds: 300,
+        AgRedoQueueAlertKb: 1048576L,
+        AgDisconnectRefireMinutes: 30,
+        BlockingWaitSecondsThreshold: 60,
+        PvsEnabled: true, PvsThresholdPercent: 40, PvsFloorGb: 1,
+        DatabaseStateEnabled: true,
+        SelfDiskFreeWarnPercent: 15,
+        CollectionStaleMinutes: 30,
+        CollectionFailureThreshold: 3,
+        DiskCriticalFreePercent: 5,
+        DiskCriticalFreeGb: 10,
+        AnalysisNotifyCooldownMinutes: 360,
+        StoreJobCadenceWarnPercent: 80,
+        FileGrowthEnabled: true, FileGrowthRiseMb: 1024, FileGrowthVolumePercent: 10,
+        FileGrowthLookbackMinutes: 60);
+
     private static string ReadRepoFile(
         string relative, [System.Runtime.CompilerServices.CallerFilePath] string thisFile = "")
     {