fix(#465): validate a dashboard-variable tab's Run as a variable option query

CHANGELOG.md

@@ -109,6 +109,38 @@ auto-generated per-PR notes; this file is the curated, human-readable history.
   no row appears, disappears or moves when the work surface changes.
 
 ### Fixed
+- **A Dashboard variable's Run action validates its option SQL again** (#465,
+  follow-up to #457). #457 moved option SQL into a `dashboard-variable` main-editor
+  tab and deleted the drawer's **Test** action along with it, but nothing replaced
+  the check it performed — the ordinary Run action executed the SQL raw and
+  reported a plain success even when it returned the wrong number or type of
+  columns. The break surfaced later and further away: only when the Dashboard-wide
+  option batch failed, as one merged `UNION ALL` column list that cannot say which
+  variable's branch was at fault.
+
+  Run on a `dashboard-variable` tab is now validated as a variable option query
+  end to end. Every locally-detectable problem — blank or comment-only SQL, a
+  non-`SELECT`, more than one statement, a `FORMAT` or `INTO OUTFILE` clause, a
+  `{name:Type}` parameter, an optional `/*[ … ]*/` block, an unterminated string
+  or comment, the reserved batch-tag column name — is reported with no request
+  sent, before authentication. (A multi-statement query is rejected by name
+  rather than silently handed to the ordinary script runner, which has no concept
+  of this contract.) A query that passes runs through the same bounded,
+  read-only single-branch probe (row cap, `max_result_bytes`, `readonly: 2`)
+  the option batch's own branches run under, so Run can never accept SQL the
+  batch would reject, nor pull an unbounded result of its own. Its response is
+  then checked against the same shape the batch requires — exactly two
+  non-nullable `String`-compatible columns (`String`, `LowCardinality(String)`,
+  `FixedString(N)`) — and a shape failure is never recorded as a successful
+  run (no history entry, no detached-result source); a genuinely valid run
+  keeps both, exactly as it did before this fix. Ordinary query-tab Run
+  behaviour is unchanged.
+
+  The Explain button/view-switch now say so instead of silently doing the
+  wrong thing on this tab: EXPLAIN has no meaning for a two-column option-SQL
+  contract, and forwarding it into the new validated Run path would have run
+  an ordinary probe with no indication Explain was ignored.
+
 - **Opening a saved query resolves it before it navigates** (#443, #429). Handing
   `openSavedQuery` an id that names nothing used to switch to the Query surface
   and push a history entry first, then discover the query was missing — opening
@@ -216,7 +248,8 @@ auto-generated per-PR notes; this file is the curated, human-readable history.
   option-backed control unavailable, one banner, no automatic fall-back to N
   separate queries), because `UNION ALL` reports one merged column list for every
   branch and so cannot say which one is at fault. Narrowing it down means running
-  a single variable's SQL on its own, which its own editor tab does (#457).
+  a single variable's SQL on its own, which its own editor tab's Run action does
+  (#457/#465).
 
   Cascading option queries are rejected outright, and `Array`/`Tuple`/`Map`/
   `Nested` variables are marked as having no inferred control — they keep their
@@ -334,8 +367,8 @@ auto-generated per-PR notes; this file is the curated, human-readable history.
   narrow drawer duplicated that machinery, shrank the working area and ran its own
   editor lifecycle. The drawer, its editor seam, its textarea fallback and its
   drawer-local **Test** action are all gone — running a variable's SQL is now the
-  ordinary Run action. Re-hosting Test's two-`String`-column check on that Run is
-  tracked in #465.
+  ordinary Run action. Re-hosting Test's two-`String`-column check on that Run
+  shipped as #465, below.
 
 - **One File menu for the whole application** (#452). Query, Dashboard Edit,
   Dashboard View, the empty-Dashboard placeholder and the Dashboard

docs/ADR-0003-dashboard-viewing.md

@@ -772,6 +772,91 @@ branches were added — touch drag is a stated non-goal, and `draggable` stays
 unconditional on Library and History rows so the shipped editor drop is unchanged
 there.
 
+## Addendum (#465, 2026-07-27): Test's shape check is re-hosted on Run, not re-invented
+
+The #457 addendum above ("Losing Test is accepted...") deferred re-hosting the
+two-`String`-column check on the ordinary Run action. Until this addendum, Run on
+a `dashboard-variable` tab executed the SQL raw: a wrong column count or type
+reported a plain success, and the mistake surfaced only later, as an unattributed
+batch-level failure.
+
+- **The preflight lives in `WorkbenchSession`, ahead of every execution choke
+  point — not inside `run()` alone.** `runEntry()` dispatches a `dashboard-variable`
+  tab straight to `run()`, before `splitStatements` decides between `run()` and
+  `runScript()`. Doing the check only inside `run()` would leave a multi-statement
+  variable query free to reach `runScript()` unvalidated — the ordinary script
+  runner has no concept of this contract at all, so `optionSqlDiagnostics`' own
+  statement-count diagnostic is what a multi-statement input must fail with,
+  not a script grid. `run()` itself runs the check before its own blank-SQL
+  no-op and before the `{name:Type}` unfilled-variable gate, so blank/comment-only
+  option SQL is reported explicitly rather than silently doing nothing the way an
+  ordinary tab's blank Run does.
+
+- **Execution reuses the batch's own bounded, read-only probe, not a new
+  transport.** `compileOptionProbe` — restored alongside `isOptionColumnType`
+  and `validateOptionColumns`, the exact three helpers the #457 addendum named
+  as Test's own — embeds the SQL through the same `nestBounded` subquery and
+  per-branch `LIMIT` the batch compiler uses, so Run can never accept SQL the
+  combined batch would reject. It drops the branch tag `compileVariableOptionBatch`
+  adds, which is what makes the two-column rule checkable at all: a `UNION ALL`
+  reports one merged column list for every branch, but a lone probe's response
+  describes only that query's own columns.
+
+  A pre-ship review caught the transport CAPS as a separate, real mismatch: an
+  earlier revision passed `state.resultRowLimit` (the user's ordinary display
+  cap) and no `params` at all, rather than `VARIABLE_OPTION_CAP + 1` and
+  `{readonly: 2, max_result_bytes: VARIABLE_OPTION_BYTE_CAP}` — the exact bound
+  and safeguards `dashboard-viewer-session.ts`'s `runOptionBatch` sends, and the
+  removed Test path (`app.runOptionQuery`, e75bfc6) sent before it. A display
+  cap lower than the batch's own bound could cut the client off before hitting
+  the failure the full batch would hit later, and no `max_result_bytes` at all
+  left Run able to pull an unbounded response of unusually large String values
+  — precisely what "cannot pass SQL the batch would reject" is supposed to
+  rule out. Fixed to match both reference points exactly.
+
+- **A shape failure borrows the ordinary run's own success gate, rather than a
+  parallel one.** `runVariableSql` (a new function beside `run()`/`runScript()`,
+  not a branch inside `run()` — a typed FORMAT clause or a `{name:Type}`
+  parameter can never reach here, `optionSqlDiagnostics` rejects both locally,
+  and a variable document never carries a panel, so none of `run()`'s KPI/
+  FORMAT machinery applies) sets `result.error` to the validation diagnostic
+  before any success bookkeeping runs, so a shape-invalid response is never
+  mistaken for a successful one. It shares `run()`/`runScript()`'s private
+  run-state (`runT0`/`runQueryId`/`runTick`/`abortController`), so `cancel()`
+  and a transport error behave identically to an ordinary tab's Run — a
+  genuine transport error or cancellation is never overwritten by a shape
+  verdict about a response that
+  never fully arrived.
+
+- **Only a shape-invalid run loses History/Expand — a genuinely valid one
+  keeps them.** This PR's first pushed revision excluded a
+  variable tab's Run from History and detached-result `source` capture
+  UNCONDITIONALLY, reasoning by analogy to `saveVariableTab`'s exclusion from
+  History/Library/favourites/Panels — but Save's exclusion is about never
+  creating a `SavedQueryV2` or Library entry, not about a tab's own execution
+  history, which an ordinary UNSAVED query tab still records. A second
+  pre-ship review finding: #465 only requires that a shape failure not be
+  MISTAKEN for success; it never asks for a valid run's existing affordances to
+  be removed. Corrected to match `run()` exactly on the success path
+  (`result.source` when `result.rows.length > 0`, `hooks.recordHistory`
+  unconditionally) and to still skip only the two fields that are provably
+  inert for this document kind: bound-parameter recording (option SQL can
+  never carry one) and `lastSuccessfulResultColumns` (a variable tab has no
+  Spec for dynamic-source completion to read it from).
+
+- **EXPLAIN is refused where it is invoked, not silently swallowed where it
+  would land.** `run()` dispatches to `runVariableSql` unconditionally for a
+  variable tab, before `opts.explain`/`opts.explainView` are ever read — so
+  those flags are simply never consulted there. Left at that, the Explain
+  button and its production callers (`app.ts`'s `explainQuery`/
+  `setExplainView`, both gated only on `editorMode === 'sql'`, true for a
+  variable tab) would have called `workbench.run({explain: true})` and gotten
+  an ordinary validation probe back with no sign Explain was ignored — a
+  review finding caught before this shipped. `explainVariableBlocked()`
+  mirrors the existing `explainMultiBlocked()` toast-on-click pattern (the
+  Explain button itself stays visible, same as the multi-statement case) and
+  is checked first, since option SQL is always one statement.
+
 ## Alternatives considered
 
 - **Durable detached snapshots:** rejected because they silently diverge from

src/application/export-service.ts

@@ -55,6 +55,7 @@ import { prepareExportSql, isSchemaMutatingSql } from '../core/format.js';
 import { formatFileMeta, exportFilename, scriptExportName } from '../core/export.js';
 import { findExceptionFrame } from '../core/stream.js';
 import type { QueryTab } from '../state.js';
+import { variableDoc } from '../state.js';
 import type { ResultSort } from '../core/sort.js';
 import type { ChCtx, exportQuery, runQuery, killQuery } from '../net/ch-client.js';
 import type { WorkbenchParameterSession } from './workbench-parameter-session.js';
@@ -261,6 +262,17 @@ export function createExportService(deps: ExportServiceDeps): ExportService {
   function exportEntry(): Promise<void> | undefined {
     if (deps.activeTab().editorMode !== 'sql') return undefined;
     if (deps.state.exporting.value) return undefined;
+    // #465 review: Export's whole feature is a full, UNCAPPED stream of a
+    // query's raw SQL — exactly what a Dashboard variable's option SQL must
+    // never get (it's validated and run only through the bounded probe,
+    // compileOptionProbe, with its own row/byte caps). Blocked outright, same
+    // precedent as Explain's own `explainVariableBlocked` (app.ts): the button
+    // stays visible and this toasts on click rather than silently exporting
+    // unvalidated, uncapped option SQL past optionSqlDiagnostics entirely.
+    if (variableDoc(deps.activeTab()) !== null) {
+      deps.hooks.toast('Export isn’t available for a Dashboard variable’s option SQL.');
+      return undefined;
+    }
     const waveMs = deps.wallNow(); // one wall clock for this export wave (gate + args)
     if (deps.params.varGateBlocked(waveMs)) return undefined; // don't export with unfilled variables (#134)
     const input = deps.activeTab().sqlDraft;

src/core/variable-options.ts

@@ -8,11 +8,12 @@
 // `UNION ALL` request per refresh, so N configured variables still cost one round
 // trip.
 //
-// Three separable jobs, all pure:
+// Four separable jobs, all pure:
 //
 //   optionSqlDiagnostics   what can be rejected WITHOUT a server, per variable
 //   compileVariableOptionBatch   the deterministic one-request compiler
 //   readVariableOptionBatch      positional response reader + partitioner
+//   compileOptionProbe/validateOptionColumns   the single-variable Run path (#465)
 //
 // The division of labour between local and server-side rejection is the issue's
 // own: a problem the app can see in the SQL text is a per-variable diagnostic
@@ -22,7 +23,11 @@
 // batch-level failure. Narrowing a batch-level failure to the branch at fault
 // means running that one variable's query on its own, where the response
 // metadata describes that query alone — which is what a variable's own
-// main-editor tab and the ordinary Run action are for (#457).
+// main-editor tab and the ordinary Run action are for (#457/#465):
+// `compileOptionProbe` embeds the SQL exactly as a batch branch would (so Run
+// cannot pass what the batch would reject) but drops the branch tag, and
+// `validateOptionColumns` reads that probe's own, unmerged response metadata —
+// the one place the "exactly two String columns" rule is checkable at all.
 //
 // Deliberately NOT here: cascading/dependent option queries. Option SQL may not
 // reference `{name:Type}` parameters at all in this issue, which is what keeps
@@ -34,6 +39,7 @@ import { detectSqlFormat, detectSqlOutfile, sqlString, stripTrailingTrivia } fro
 import { scanParamDeclarations } from './param-scan.js';
 import { analysisView } from './param-pipeline.js';
 import { hasOptionalBlocks } from './optional-blocks.js';
+import { parseClickHouseType, analyzeTypeModifiers } from './clickhouse-type.js';
 import { isCompoundParamType, multiSelectElementType } from './param-type.js';
 import type { DashboardVariable } from './dashboard-variables.types.js';
 import type {
@@ -301,19 +307,75 @@ export function compileVariableOptionBatch(
   return { sql, branches, rowLimit: branches.length * BRANCH_LIMIT + 1 };
 }
 
-// #457 removed `compileOptionProbe`, `isOptionColumnType` and
-// `validateOptionColumns`. All three existed for the per-variable drawer's Test
-// action — a single-variable probe whose response metadata describes the user's
-// own columns, which is the only place the "exactly two String columns" rule is
-// checkable (a combined `UNION ALL` reports one merged column list for every
-// branch). Deleting that drawer left them with no caller in `src/`. Re-hosting the
-// check on the variable tab's Run is deferred to #465, which names the commit they
-// can be recovered from.
+/**
+ * The query a `dashboard-variable` tab's Run action executes (#465): ONE
+ * variable's option SQL, embedded exactly as a batch branch embeds it but
+ * without the branch tag.
+ *
+ * Sharing `nestBounded` with the compiler is the point — Run must not pass SQL
+ * the batch would reject, and the nesting is the one transformation that can
+ * make an otherwise-valid query fail. Dropping the tag column keeps the response
+ * metadata describing the USER's own columns, which is what makes the "exactly
+ * two String columns" rule checkable here and nowhere else: in the combined batch
+ * `UNION ALL` reports one merged column list for every branch.
+ *
+ * Bounded like a branch, so Run cannot pull an unbounded result either.
+ */
+export const compileOptionProbe = (sql: string): string =>
+  `SELECT * FROM ${nestBounded(normalizeOptionSql(sql))}`;
 
 // ── The response reader ──────────────────────────────────────────────────────
 
 const cell = (value: unknown): string => (value == null ? '' : String(value));
 
+/**
+ * Is `type` acceptable for an option value/label column?
+ *
+ * `String` and the wrappers that are transparent to VALUE handling —
+ * `LowCardinality(String)`, `FixedString(N)` — all qualify: a low-cardinality
+ * dimension column is the single most common real source of option values, and
+ * rejecting it would reject the contract's own example. `Nullable(...)` does NOT:
+ * the streaming transport renders a null cell as ClickHouse's literal `ᴺᵁᴸᴸ`
+ * marker, which is meaningless as either a bound value or a label, so it is
+ * better refused with its type named than silently offered as an option.
+ */
+export function isOptionColumnType(type?: string | null): boolean {
+  const node = parseClickHouseType(String(type ?? '').trim());
+  if (!node) return false;
+  const mods = analyzeTypeModifiers(node);
+  if (mods.nullable) return false;
+  const base = mods.valueType.name;
+  return base === 'String' || base === 'FixedString';
+}
+
+/**
+ * Validate the response METADATA of a SINGLE variable's option query — the
+ * variable tab's Run path (#465), where the columns describe that one query
+ * rather than a compiled batch.
+ *
+ * This is where the contract's column rules are actually enforceable: exactly two
+ * columns, both String. In the combined batch the same rules cannot be checked
+ * from metadata at all, because `UNION ALL` reports one merged column list for
+ * every branch (names from the first branch, types promoted to a supertype) — so
+ * the batch relies on ClickHouse rejecting a branch whose arity disagrees, and
+ * Run is how a user finds out precisely which variable is wrong and why.
+ */
+export function validateOptionColumns(
+  columns: readonly { name: string; type: string }[],
+): VariableOptionDiagnostic | null {
+  if (columns.length !== 2) {
+    return diagnostic('variable-option-column-count',
+      `Option SQL must return exactly two columns (value, then label); this returns ${columns.length}.`);
+  }
+  const bad = columns.filter((column) => !isOptionColumnType(column.type));
+  if (bad.length) {
+    return diagnostic('variable-option-column-type',
+      'Both option columns must be String; '
+      + `this returns ${bad.map((column) => column.type).join(' and ')}.`);
+  }
+  return null;
+}
+
 /**
  * Turn one option-batch response into per-variable option lists.
  *