[erikdarlingdata]

Closes #2733.

The footgun

The custom-view 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 than the one the author wrote. "filter" for "filters" (or "Filters", case) silently dropped the filter and widened the query to the whole fleet; a dropped overlay/thresholds changed what the panel claims. An iterating agent — the MCP surface's whole point — stops at valid:true with nobody reading the emitted SQL. Every rejection case in the new tests was proven red against the pre-fix validator first: each one returned {valid:true} (or the misdirecting unknown source '') before the change.

What changed

Unknown keys are errors on the WRITE path (validate/create/update — web routes and MCP tools route through the same ValidateDefinition authority), at every level the composed surface owns:

level	known keys	plus
dashboard root	kind, panels, variables, range	targeted hint for cells without the notebook kind
notebook root	kind, cells, variables, range	targeted hint for panels
composed panel / panel cell	ComposeSpec.ComposedPanelKeys (the 13 keys TryParsePanel reads)	presentation extras title/span/hours (+ type on a cell)
filter object	dimension, op, value
overlay object	measure, aggregate, unit
variable object	name, dimension, default
range object	hours

The error names the offending key and its path, with a did-you-mean for a near miss (case-insensitive match or edit distance ≤ 2): panel 0: panel has unknown key 'filter' — did you mean 'filters'?

The key-universe mechanism. The sets live in ComposeSpec, beside the parser that reads them, and cannot drift silently in either direction: an every-key acceptance test (ValidateDefinition_AcceptsEveryDeclaredComposedKey_AtOnce) proves each declared key actually parses, and because the strict check runs before TryParsePanel on the write path, a key the parser learns but the set does not is rejected by the new feature's own first write-path test — a forgotten registration is loud, not silent.

The write/read split (per the issue's fix direction): strictness applies to the write path only. TryParsePanel itself stays lenient — pinned by TryParsePanel_StaysLenient_ForTheRunAndReadPaths — so a definition stored before the check keeps loading, rendering, and running unchanged, and meets the strict check when someone next edits it. The read/render path never calls ValidateDefinition.

The panel-nesting shape mismatch the issue flags is kept (unifying it would rewrite the run tool's wire shape and the stored-cell shape for no gain) and explicitly named everywhere it bites:

• a notebook panel cell {"type":"panel","panel":{...}} — the reported repro — now errors with what to unnest instead of unknown source '';
• a dashboard panel {"panel":{...}} gets the same targeted message in the v1 arm;
• a doubly-nested run spec ({"panel":{"panel":{...}}}) gets it on the run path too (safe: no stored panel can carry a panel key — one never parsed);
• run_custom_view_panel / validate_custom_view tool descriptions now state the flat-vs-nested rule.

Related error-quality fixes: a panel with no source at all reads panel is missing 'source' (not unknown source ''), and a panel naming neither mode key gets a near-miss suggestion (did you mean 'source' (found 'sorce')?).

Deliberately NOT strict: v1 read panels keep their open presentation vocabulary (rowsKey/xKey/format/emptyText/columns/… — the renderer's contract, spread verbatim from the editor's vizcfg); a server-side enumeration of it would be a second copy that decays, and a stray key there changes pixels, not semantics. The v1 keys with semantic weight were already individually validated (read, viz, span, series colors, and the path rejection).

Verification

• All rejection tests red-first against the pre-fix validator (12 cases validated {valid:true}, the nesting repro misdirected), green after, via a local net10.0 harness running the real assemblies (the Windows suites can't run on macOS — CI is the arbiter for the xUnit runs).
• Every shipped serializer stays inside the sets: editor.js composedPanelToDesc/readPanelToDesc/modelToDefinition, notebook.js modelToNotebookDefinition/cellToStored, all five seed notebook templates (mirrored in ValidateDefinition_AcceptsEverySeedNotebookTemplate), and the v1 dashboard templates.
• One existing test updated: ValidateDefinition_RejectsACellsDoc_SentWithoutTheNotebookKind now pins the better targeted message (add "kind":"notebook") instead of "panels must be an array".
• No compiler or catalog changes: ComposeCompiler.cs and MeasureCatalog.cs untouched.

🤖 Generated with Claude Code

https://claude.ai/code/session_01JgLaaxFenZFZs1SntKQDEg

[claude]

Reviewed the diff (CHANGELOG, DarlingComposeTests.cs, DarlingMcpCustomViewToolsTests.cs, ComposeSpec.cs, DarlingWebEndpoints.cs, DarlingMcpCustomViewTools.cs).

Parity: No Lite counterpart exists for custom views/compose/MCP tooling (grep for ComposeSpec/ValidateDefinition/CustomView under Lite/ returns nothing) — this feature is Darling-only, so there's no parity drift to flag.

Correctness: Traced the new key-universe mechanism end-to-end against the actual parsers rather than trusting the docstrings:

• ComposedPanelKeys (13 keys) matches exactly what TryParsePanel reads via Str(panel, ...) / panel[...] (source, measure, ratio, aggregate, unit, timeBucket, topN, viz, filters, groupBy, overlay, thresholds, annotations).
• ComposedFilterKeys {dimension, op, value} matches ParseFilters.
• ComposedOverlayKeys {measure, aggregate, unit} matches the overlay parse block.
• VariableKeys {name, dimension, default} and RangeKeys {hours} match ParseVariables/ParseRange.
• Cross-checked the frontend emitters (editor.js composedPanelToDesc, notebook.js cellToStored) against s_composedDashboardPanelExtraKeys {title, span, hours} and s_notebookPanelCellExtraKeys {type, title, hours} — the notebook path correctly drops span before serializing, matching the doc comment that notebook cells have no width, and every key the frontend actually emits is covered so legitimate saves won't be broken by the new strictness.
• The write/read split holds: TryParsePanel itself is untouched in leniency (pinned by TryParsePanel_StaysLenient_ForTheRunAndReadPaths), strictness lives only in ValidateDefinition/ValidateNotebookDefinition, and RunComposedPanelAsync only special-cases the one doubly-nested mis-shape without otherwise tightening the run path.

No bugs found. This is a solid, self-verifying change — the "every declared key actually parses" acceptance test plus the write-path-rejects-parser-only-keys ordering genuinely close the drift hole described in the PR body, and I confirmed that guarantee by reading the parsers rather than taking the test's word for it.

No missing-index DMV suggestions applicable — this PR doesn't touch T-SQL collectors.

[claude]

Minor/non-blocking: this "spec nests under 'panel'" wording is now maintained in two independent places — this hardcoded string (v1 arm, panel has neither read nor source) and ComposeSpec.s_panelKeyHints["panel"] (composed-panel arm, panel has a source). Both are correct today (verified against composedPanelToDesc/cellToStored in editor.js/notebook.js and covered by ValidateDefinition_NamesTheRunSpecNesting_OnAPanelCell/_OnADashboardPanel), but a future wording tweak to one is easy to miss in the other. Not asking for a change here, just flagging for awareness.

[erikdarlingdata]

Addressed in 26d7c94 — the wording now lives in one internal constant (ComposeSpec.RunSpecNestingHint) feeding both the composed arm's key hint and the v1 arm's message, so a future tweak cannot drift the two apart.

[claude]

Reviewed the diff against CONTRIBUTING.md's C# style guidance and the correctness/parity/security/perf checklist. This is a C#-only change to Darling's custom-view compose/notebook validator — no T-SQL, and no Lite counterpart exists for this feature (grepped for ComposeSpec/DarlingWebEndpoints — Darling-only), so there's no Lite/Darling parity drift to flag here.

Correctness verification I did by hand, since this is exactly the kind of change where the key-set/parser pairing is easy to get subtly wrong:

• Cross-checked every entry in ComposeSpec.ComposedPanelKeys / ComposedFilterKeys / ComposedOverlayKeys / VariableKeys / RangeKeys / s_vizObjectKeys against what TryParsePanel/ParseFilters/ParseVariables/ParseRange/ParseViz actually read — exact match, no key drift in either direction.
• Cross-checked s_dashboardRootKeys / s_notebookRootKeys / s_markdownCellKeys / s_composedDashboardPanelExtraKeys / s_notebookPanelCellExtraKeys against what the frontend serializers actually emit (editor.js's composedPanelToDesc/readPanelToDesc/modelToDefinition, notebook.js's cellToStored/modelToNotebookDefinition) — every emitted key has a home in the new strict sets, so legitimately-authored definitions won't be rejected.
• Hand-verified the edit-distance-≤2 near-miss suggestions for every [InlineData] typo in the new theory test (filter→filters, Filters→filters via case-insensitive match, threshold→thresholds, values→value, unt→unit, variabels→variables, defalut→default, texte→text, titel→title, sorce→source) — all land within the threshold and point at the right candidate given the length-diff short-circuit in EditDistance.
• Confirmed the write/read split is real: TryParsePanel itself never calls the new strict-key helpers, so a definition stored before this change keeps rendering/running (pinned by TryParsePanel_StaysLenient_ForTheRunAndReadPaths), and the doubly-nested-spec guard on the run path (panel["panel"] is JsonObject && panel["source"] is null) can't false-positive on a legitimately-stored panel, since no write-path-accepted panel has ever carried a panel key.
• Checked placement order in both ValidateDefinition and ValidateNotebookDefinition: root strict-key check runs before the shape checks (so cells without "kind":"notebook" gets the targeted hint instead of "panels must be an array"), and the shared-root (variables/range) strict-key check runs after those parsers succeed, so structural errors keep their existing messages.

No security or performance concerns — this only tightens validation of already-untrusted JSON that was going to be parsed anyway; the EditDistance computation is bounded (short key names, ≤~13 candidates, write-path-only, only runs on the failure path).

Left one non-blocking inline note about a small duplication in the "spec nests under 'panel'" hint text (maintained in two places for the two arms). Otherwise this looks solid — nice self-verifying test suite (red-first against the pre-fix validator).

[claude]

Reviewed the diff (ComposeSpec.cs / DarlingWebEndpoints.cs / DarlingMcpCustomViewTools.cs + tests). This is Darling-only C# work — no T-SQL touched, so the T-SQL style rules do not apply here.

Parity: Lite has no custom-view/compose/MCP surface at all (searched for custom_view / ComposeSpec under Lite/, no hits), so there is no Lite/Darling parity drift to flag for this change.

Correctness, verified by cross-checking the new key sets against the actual readers (not just the tests):

• ComposedPanelKeys (13 keys) matches exactly what TryParsePanel reads (source/measure/ratio/aggregate/unit/timeBucket/topN/viz/filters/groupBy/overlay/thresholds/annotations).
• ComposedFilterKeys, ComposedOverlayKeys, VariableKeys, RangeKeys, and the viz-object key all match their respective Parse* readers.
• The frontend emitters (editor.js composedPanelToDesc, notebook.js cellToStored) only ever write keys that are in the corresponding allow-sets (title/span/hours for dashboard panels; type/title/hours for notebook panel cells), so this change cannot reject anything the app itself writes.
• name/description are sibling request fields, never merged into the definition blob, so the new strict root-key check (kind/panels/variables/range) cannot break existing saves.
• The write/read split is real: TryParsePanel itself stays lenient (only the missing-source message text changed), and all the new strict checks live in ValidateDefinition/ValidateNotebookDefinition, the write-path-only authority.
• The doubly-nested-spec check in RunComposedPanelAsync only triggers when panel["panel"] is an object and panel["source"] is absent, so it cannot misfire on a legitimately stored panel (the write path never allows a panel key to be stored in the first place).

No correctness, security, or performance issues found. One very minor non-blocking note: NearestKnownKeys tie-break among candidates at equal edit distance depends on HashSet enumeration order, which only affects which key a did-you-mean message names in a tie, never the pass/fail outcome — not worth acting on.

[claude]

Reviewed the diff (CHANGELOG.md, ComposeSpec.cs, DarlingWebEndpoints.cs, DarlingMcpCustomViewTools.cs, and the two test files).

Parity: Custom Views (dashboards/notebooks/ComposeSpec) is a Darling-only concept — Lite has no equivalent surface, so there's no Lite/Darling drift to flag here.

Correctness: Traced every new "known key" set back to the parser code it's supposed to mirror, and they line up exactly:

• ComposedPanelKeys (13 keys) ↔ every key TryParsePanel reads (source, measure/ratio, aggregate, unit, timeBucket, topN, viz, filters, groupBy, overlay, thresholds, annotations)
• ComposedFilterKeys ↔ ParseFilters (dimension, op, value)
• ComposedOverlayKeys ↔ the overlay block in TryParsePanel (measure, aggregate, unit)
• VariableKeys ↔ ParseVariables (name, dimension, default)
• RangeKeys ↔ ParseRange (hours)
• s_vizObjectKeys ↔ ParseViz's object form (type)

Also checked the write/read split holds: TryParsePanel itself never calls the new UnknownKeyError/UnknownComposedPanelKeyError helpers, so previously-stored definitions with a stray key keep rendering/running — only ValidateDefinition/ValidateNotebookDefinition (create/update, web + MCP alike) are strict. Root-key checks correctly run after the "kind":"notebook" dispatch, and the shared variables/range strict checks run after their structural parse succeeds, so a genuine structural error keeps its existing message and the new check only ever fires on an actually-stray key. The doubly-nested run-path guard (panel["panel"] is JsonObject && panel["source"] is null) is sound — no write-path-accepted panel can carry a panel key, so it can't misfire on a legitimate stored view.

No bugs found. No missing-index-DMV suggestions applicable (none introduced). No security concerns — this only tightens JSON shape validation on an already-gated write surface (mcp role limited to config.custom_views), doesn't touch SQL construction, and doesn't add file/network/process/secrets handling.

Nice attention to detail on making the every-key acceptance test (ValidateDefinition_AcceptsEveryDeclaredComposedKey_AtOnce) a two-way guard against set/parser drift.