Reject unknown keys in custom-view validation, naming the stray and the near miss (#2733)

CHANGELOG.md

@@ -8,6 +8,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 ## [Unreleased]
 
 ### Added
+- **Custom-view validation rejects unknown keys instead of silently ignoring them** ([#2733]) - the validator positive-read the keys it knew and defaulted every optional one on absence, so a typo'd key validated `{valid:true}` and stored a syntactically-valid DIFFERENT panel: `"filter"` for `"filters"` silently dropped the filter and widened the query to the whole fleet, `"Filters"` (case) did the same, and an iterating agent - the MCP surface's whole point - stops at valid:true with nobody reading the emitted SQL. The write path (validate/create/update, web and MCP alike) now rejects any key outside the known set at every level it owns - definition root, composed panel, filters, overlay, notebook cell, variables, range - naming the stray key with a did-you-mean for a near miss. The shape mismatch that invited the worst mis-shape is named too: run_custom_view_panel's spec nests under "panel" while a stored panel/cell is FLAT, and nesting where flat belongs used to fail as the misdirecting "unknown source ''" - it now says what to unnest, on the write path and the run path both. Strictness is the write path's alone: a definition stored before the check keeps loading, rendering, and running unchanged, and meets the strict check when next edited. v1 read panels keep their open presentation vocabulary (the renderer's contract, where a stray key changes pixels, not semantics) - their semantically-loaded keys were already individually validated.
 - **`get_query_store_duration_trend` stops ranking the raw Query Store slab per call** ([#2736]) - the trend's read did #1841's dedup - a `ROW_NUMBER()` over `query_store_stats` at full interval-identity grain - per call, forever, chunk-excluded only by a fixed `-1 day / +30 days` collection_time slab, so its cost was the slab's, not the window's: on a store carrying ~6.9M Query Store rows per 6 hours it exceeded the mcp role's 15s statement_timeout backstop at EVERY window width, including a 2-hour window anchored at now - the one trend read that could not run on exactly the store big enough to need it. The read now routes through `query_store_stats_corrected_hourly` ([#1849]), which already materializes this series' ingredients deduped at interval grain: the materialized window portion becomes a rollup scan and the raw rank runs only over the unmaterialized tail (normally the refresh policy's hour or two), bounded to that tail - `GREATEST(window start, boundary) - 1 hour` to `window end + 1 day` in place of the slab. ComposeSourceRouter-style gating on availability AND materialized coverage ([#1665]/[#1759]), and the routed SQL is built once and shared by the MCP tool and the desktop viewer so the two apps cannot drift about the same hour; a store without a materialized rollup (plain PostgreSQL, or nothing refreshed yet) keeps the original read byte for byte, slab included, because that is where its generosity is affordable. The payload labels the routing instead of presenting two estimators as one series: rollup-served points are collection-hour buckets carrying #1849's hour-straddle residual while tail points stay at interval starts, and a window head below the rollup's materialized floor is disclosed as missing-not-zero with `--backfill-rollups` named as the remedy - a degraded answer must label itself rather than time out. Verified by running the shipped SQL against a live TimescaleDB 2.28.1/PG18 rig: routing, boundary partition, per-bucket dedup arithmetic, the database-filter variant, and the old read side by side on the same fixture.
 - **Query Store gets total-CPU and total-duration compose measures** ([#2732]) - the compose catalog's Query Store roster could rank by executions, a per-interval peak, or a per-execution average - never by what a query group actually CONSUMED, so "what burned the most CPU over this window" was not expressible against the box's headline dataset (a per-execution average ranks a query that ran once at 30s over one that burned an hour of CPU at 200ms a call). `qs_total_cpu_us` / `qs_total_duration_us` fill the hole as a new weighted-sum measure shape (`MeasureRatioMode.WeightedSum`): the existing Weighted ratio's numerator with no denominator - `SUM(avg * execution_count)` on the raw route, since avg times executions IS each interval's total, and the corrected rollups' pre-multiplied `cpu_us_weighted_sum` / `duration_us_weighted_sum` columns read directly on the CAGG routes. Pure catalog/expression plumbing - no migration, no collector change, no backfill - and time-series, ranked (top query_hash by total CPU), and scalar modes all work for free.
 - **Alert deliveries carry a linked triage page** ([#2710]) - option 2 of the issue's sketch: a purpose-built, read-only `GET /api/triage` endpoint in the Darling web API assembles a per-firing triage page ON READ - the matching `config_alert_log` row(s), the recent collection log, and alert-type-relevant collected data (deadlock/blocking detail for those alerts, CPU trend + top queries for High CPU, the PostgreSQL reads for the PG alert family, a summary fallback for unmapped metrics) - every section running through the SAME `/api/read/{tool}` dispatch the dashboard already serves, so there is zero SQL/projection drift and nothing is provisioned per alert (no `config.custom_views` row, nothing to GC; a link older than retention degrades to honest empties, and a missing server/metric/history row degrades to notes on the page, never a 500). The SPA renders it at `#/triage?server=...&metric=...&at=...&dedup=...` with derive.js-inferred field configs, and the Alert History page links each row to it. All four webhook channels carry the link when the new `web.publicBaseUrl` (darling.json, file-authoritative - deployment plumbing, not a store knob) is set: Slack gets a link button, Teams a `potentialAction` OpenUri, PagerDuty the Events v2 `links` array plus a `custom_details` Triage row, and the generic template a `{{triage_url}}` token. Unset (the default, and always on Lite, which serves no web page) every payload renders byte-identical to before - a link can never be the reason an alert fails to deliver.
@@ -3069,5 +3070,6 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 [#2710]: https://github.com/erikdarlingdata/PerformanceMonitor/issues/2710
 [#2711]: https://github.com/erikdarlingdata/PerformanceMonitor/issues/2711
 [#2719]: https://github.com/erikdarlingdata/PerformanceMonitor/issues/2719
+[#2733]: https://github.com/erikdarlingdata/PerformanceMonitor/issues/2733
 [#2736]: https://github.com/erikdarlingdata/PerformanceMonitor/issues/2736
 [#2732]: https://github.com/erikdarlingdata/PerformanceMonitor/issues/2732

Darling/Darling.Tests/DarlingComposeTests.cs

@@ -1785,12 +1785,163 @@ public void ValidateDefinition_StillAcceptsADashboard_AfterTheKindDispatch()
     [Fact]
     public void ValidateDefinition_RejectsACellsDoc_SentWithoutTheNotebookKind()
     {
-        /* Without "kind":"notebook", a cells-only doc is just a dashboard missing its panels — rejected, so a
-           notebook can never be silently mistaken for (or stored as) an empty dashboard. */
+        /* Without "kind":"notebook", a cells-only doc dispatches down the dashboard arm — still rejected (a
+           notebook can never be silently stored as an empty dashboard), and since #2733 the strict root-key
+           check names the actual mis-shape (add the kind) instead of "panels must be an array". */
         var result = DarlingWebEndpoints.ValidateDefinition(
             "{\"cells\":[{\"type\":\"markdown\",\"text\":\"x\"}]}");
         Assert.False(result.IsValid);
-        Assert.Contains("panels", result.Error!, StringComparison.OrdinalIgnoreCase);
+        Assert.Contains("'cells'", result.Error!, StringComparison.Ordinal);
+        Assert.Contains("\"kind\":\"notebook\"", result.Error!, StringComparison.Ordinal);
+    }
+
+    /* ─────────────────────────── #2733: unknown keys are write-path errors ─────────────────────────── */
+
+    /* The footgun class: TryParsePanel positive-reads known keys and defaults every optional one on absence,
+       so before #2733 each of these validated {valid:true} and stored a syntactically-valid DIFFERENT panel —
+       a typo'd 'filter' silently dropping the filter and widening the query to the whole fleet. Every case
+       here was proven red against the pre-fix validator (they all validated clean, or failed with the
+       misdirecting "unknown source ''"). */
+    [Theory]
+    /* panel level: the reported footgun — a typo'd/mis-cased optional key that silently defaults. */
+    [InlineData(
+        "{\"panels\":[{\"source\":\"wait_stats\",\"measure\":\"wait_time_ms\",\"aggregate\":\"sum\",\"timeBucket\":\"hour\",\"viz\":\"line\",\"filter\":[{\"dimension\":\"wait_type\",\"op\":\"eq\",\"value\":\"CXPACKET\"}]}]}",
+        "panel 0: panel has unknown key 'filter' — did you mean 'filters'?")]
+    [InlineData(
+        "{\"panels\":[{\"source\":\"wait_stats\",\"measure\":\"wait_time_ms\",\"aggregate\":\"sum\",\"timeBucket\":\"hour\",\"viz\":\"line\",\"Filters\":[{\"dimension\":\"wait_type\",\"op\":\"eq\",\"value\":\"x\"}]}]}",
+        "panel 0: panel has unknown key 'Filters' — did you mean 'filters'?")]
+    [InlineData(
+        "{\"panels\":[{\"source\":\"wait_stats\",\"measure\":\"wait_time_ms\",\"aggregate\":\"sum\",\"timeBucket\":\"hour\",\"viz\":\"line\",\"threshold\":[90]}]}",
+        "did you mean 'thresholds'?")]
+    /* filter level. */
+    [InlineData(
+        "{\"panels\":[{\"source\":\"wait_stats\",\"measure\":\"wait_time_ms\",\"aggregate\":\"sum\",\"timeBucket\":\"hour\",\"viz\":\"line\",\"filters\":[{\"dimension\":\"wait_type\",\"op\":\"eq\",\"value\":\"x\",\"values\":\"y\"}]}]}",
+        "filter 0 has unknown key 'values' — did you mean 'value'?")]
+    /* overlay level. */
+    [InlineData(
+        "{\"panels\":[{\"source\":\"query_stats\",\"measure\":\"query_worker_us\",\"aggregate\":\"sum\",\"timeBucket\":\"hour\",\"viz\":\"line\",\"overlay\":{\"measure\":\"query_avg_elapsed_us\",\"unt\":\"us\"}}]}",
+        "overlay has unknown key 'unt' — did you mean 'unit'?")]
+    /* view root, variables, range. */
+    [InlineData(
+        "{\"variabels\":[{\"name\":\"db\",\"dimension\":\"database_name\"}],\"panels\":[{\"source\":\"wait_stats\",\"measure\":\"wait_time_ms\",\"aggregate\":\"sum\",\"timeBucket\":\"hour\",\"viz\":\"line\"}]}",
+        "definition has unknown key 'variabels' — did you mean 'variables'?")]
+    [InlineData(
+        "{\"variables\":[{\"name\":\"db\",\"dimension\":\"database_name\",\"defalut\":\"x\"}],\"panels\":[{\"source\":\"wait_stats\",\"measure\":\"wait_time_ms\",\"aggregate\":\"sum\",\"timeBucket\":\"hour\",\"viz\":\"line\"}]}",
+        "variable 0 has unknown key 'defalut' — did you mean 'default'?")]
+    [InlineData(
+        "{\"range\":{\"hours\":24,\"days\":3},\"panels\":[{\"source\":\"wait_stats\",\"measure\":\"wait_time_ms\",\"aggregate\":\"sum\",\"timeBucket\":\"hour\",\"viz\":\"line\"}]}",
+        "range has unknown key 'days'.")]
+    /* notebook root + cells. */
+    [InlineData(
+        "{\"kind\":\"notebook\",\"panels\":[],\"cells\":[{\"type\":\"markdown\",\"text\":\"x\"}]}",
+        "notebook has unknown key 'panels' — a notebook carries 'cells', not 'panels'.")]
+    [InlineData(
+        "{\"kind\":\"notebook\",\"cells\":[{\"type\":\"markdown\",\"text\":\"x\",\"texte\":\"y\"}]}",
+        "cell 0 (markdown) has unknown key 'texte' — did you mean 'text'?")]
+    [InlineData(
+        "{\"kind\":\"notebook\",\"cells\":[{\"type\":\"panel\",\"titel\":\"t\",\"source\":\"wait_stats\",\"measure\":\"wait_time_ms\",\"aggregate\":\"sum\",\"timeBucket\":\"hour\",\"viz\":\"line\"}]}",
+        "cell 0: panel has unknown key 'titel' — did you mean 'title'?")]
+    public void ValidateDefinition_RejectsUnknownKeys_NamingKeyPathAndNearMiss(string json, string expectedError)
+    {
+        var result = DarlingWebEndpoints.ValidateDefinition(json);
+        Assert.False(result.IsValid, "a stray key must not validate {valid:true} — that is the #2733 footgun");
+        Assert.Contains(expectedError, result.Error!, StringComparison.Ordinal);
+    }
+
+    [Fact]
+    public void ValidateDefinition_NamesTheRunSpecNesting_OnAPanelCell()
+    {
+        /* The reported repro verbatim: run_custom_view_panel's spec nests under 'panel' while a notebook
+           panel cell is FLAT, so nesting is the natural guess — and it used to fail with the misdirecting
+           "unknown source ''". The strict check now names the wrapper and says what to do. */
+        var result = DarlingWebEndpoints.ValidateDefinition(
+            "{\"kind\":\"notebook\",\"cells\":[{\"type\":\"panel\",\"panel\":{\"source\":\"wait_stats\",\"measure\":\"wait_time_ms\",\"aggregate\":\"sum\",\"timeBucket\":\"hour\",\"viz\":\"line\"}}]}");
+        Assert.False(result.IsValid);
+        Assert.Contains("cell 0: panel has unknown key 'panel'", result.Error!, StringComparison.Ordinal);
+        Assert.Contains("run_custom_view_panel", result.Error!, StringComparison.Ordinal);
+        Assert.DoesNotContain("unknown source", result.Error!, StringComparison.OrdinalIgnoreCase);
+    }
+
+    [Fact]
+    public void ValidateDefinition_NamesTheRunSpecNesting_OnADashboardPanel()
+    {
+        /* The same mis-shape on the dashboard arm: {"panel":{...}} names neither 'read' nor 'source', so it
+           lands in the v1 arm — which now recognizes the wrapper instead of shrugging "missing 'read' or
+           'source'". */
+        var result = DarlingWebEndpoints.ValidateDefinition(
+            "{\"panels\":[{\"panel\":{\"source\":\"wait_stats\",\"measure\":\"wait_time_ms\",\"aggregate\":\"sum\",\"viz\":\"table\"}}]}");
+        Assert.False(result.IsValid);
+        Assert.Contains("nests its spec under 'panel'", result.Error!, StringComparison.Ordinal);
+        Assert.Contains("run_custom_view_panel", result.Error!, StringComparison.Ordinal);
+    }
+
+    [Fact]
+    public void ValidateDefinition_SuggestsTheModeKey_OnANearMissTypo()
+    {
+        /* 'sorce' names neither mode key, so the panel lands in the v1 arm; the near-miss scan points at the
+           typo instead of leaving the author to diff their JSON against the docs. */
+        var result = DarlingWebEndpoints.ValidateDefinition(
+            "{\"panels\":[{\"sorce\":\"wait_stats\",\"measure\":\"wait_time_ms\",\"aggregate\":\"sum\",\"viz\":\"table\"}]}");
+        Assert.False(result.IsValid);
+        Assert.Contains("did you mean 'source' (found 'sorce')?", result.Error!, StringComparison.Ordinal);
+    }
+
+    [Fact]
+    public void ValidateDefinition_AcceptsEveryDeclaredComposedKey_AtOnce()
+    {
+        /* The sync guard for the key universe: one panel carrying EVERY key in ComposeSpec.ComposedPanelKeys
+           plus every presentation extra (title/span/hours) validates clean, so a key declared in the set but
+           not actually read by the parser cannot hide. The converse cannot drift silently either: the strict
+           check runs BEFORE TryParsePanel on the write path, so a future key the parser learns but the set
+           does not is rejected here — the new feature's own first write-path test goes red. */
+        var ok = DarlingWebEndpoints.ValidateDefinition(
+            "{\"kind\":\"dashboard\",\"range\":{\"hours\":24},\"variables\":[{\"name\":\"w\",\"dimension\":\"wait_type\",\"default\":\"CXPACKET\"}]," +
+            "\"panels\":[" +
+            "{\"source\":\"wait_stats\",\"measure\":\"wait_time_ms\",\"aggregate\":\"sum\",\"unit\":\"ms\",\"timeBucket\":\"hour\",\"viz\":\"line\"," +
+            "\"filters\":[{\"dimension\":\"wait_type\",\"op\":\"eq\",\"value\":\"$w\"}],\"groupBy\":[],\"thresholds\":[100],\"annotations\":[\"deadlocks\"]," +
+            "\"title\":\"T\",\"span\":2,\"hours\":48}," +
+            "{\"source\":\"query_stats\",\"measure\":\"query_worker_us\",\"aggregate\":\"sum\",\"topN\":25,\"groupBy\":[\"query_hash\"],\"viz\":\"scatter\"," +
+            "\"overlay\":{\"measure\":\"query_executions\",\"aggregate\":\"sum\",\"unit\":\"count\"},\"title\":\"S\",\"span\":1}," +
+            "{\"source\":\"wait_stats\",\"ratio\":\"signal_wait_pct\",\"timeBucket\":\"hour\",\"viz\":{\"type\":\"line\"}}]}");
+        Assert.True(ok.IsValid, ok.Error);
+    }
+
+    [Fact]
+    public void ValidateDefinition_LeavesTheV1ReadVocabularyOpen()
+    {
+        /* v1 read panels deliberately keep their open presentation vocabulary (rowsKey/xKey/format/emptyText/
+           columns/... — the renderer's contract, spread from the editor's vizcfg verbatim); enumerating it
+           server-side would be a second copy that decays. The v1 keys with semantic weight are each validated
+           individually, and raw 'path' stays rejected. */
+        var ok = DarlingWebEndpoints.ValidateDefinition(
+            "{\"panels\":[{\"title\":\"CPU\",\"read\":\"get_cpu_utilization\",\"params\":{\"hours\":24},\"viz\":\"line\",\"span\":2," +
+            "\"rowsKey\":\"samples\",\"xKey\":\"sample_time\",\"format\":\"pct\",\"unit\":\"%\",\"emptyText\":\"none\"," +
+            "\"series\":[{\"key\":\"total_cpu\",\"label\":\"Total %\"}]}]}");
+        Assert.True(ok.IsValid, ok.Error);
+    }
+
+    [Fact]
+    public void TryParsePanel_StaysLenient_ForTheRunAndReadPaths()
+    {
+        /* The write/read split (#2733): strictness lives in ValidateDefinition (validate/create/update), NOT
+           in the parser — a definition stored before the strictness existed must keep loading, rendering, and
+           running. This pin keeps a future "tidy-up" from moving the key check into TryParsePanel and
+           breaking every stored view carrying a legacy stray. */
+        var (plan, error) = ComposeSpec.TryParsePanel(
+            PanelJson("{\"source\":\"wait_stats\",\"measure\":\"wait_time_ms\",\"aggregate\":\"sum\",\"timeBucket\":\"hour\",\"viz\":\"line\",\"legacyStray\":1}"),
+            Array.Empty<string>());
+        Assert.True(error is null, error);
+        Assert.NotNull(plan);
+    }
+
+    [Fact]
+    public void TryParsePanel_SaysMissingSource_NotUnknownEmptySource()
+    {
+        /* The misdirecting half of the #2733 report: a mis-shaped panel has no 'source' at all, and "unknown
+           source ''" sent the author hunting the catalog instead of the shape. */
+        var reason = RejectReason("{\"measure\":\"wait_time_ms\",\"aggregate\":\"sum\",\"viz\":\"table\"}");
+        Assert.Contains("missing 'source'", reason, StringComparison.Ordinal);
+        Assert.DoesNotContain("unknown source", reason, StringComparison.OrdinalIgnoreCase);
     }
 
     /* ─────────────────────────── D7: list-summary kind (badge/route) ─────────────────────────── */