From 4d085f3d57ebf25685a125912792a193eba32f7f Mon Sep 17 00:00:00 2001 From: Cursor Agent Date: Sat, 25 Jul 2026 11:08:02 +0000 Subject: [PATCH] fix(sql-editor): fail closed when @set captures a partial result MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Column/table @set previously wrote truncated page-0 / capped rows into variables with no error, so later ${{…}} expansion could silently miss rows. Refuse incomplete or over-cap captures; scalar @set is unchanged. Co-authored-by: huy.phan9 --- .../src/frontend/lib/sql-variables.test.ts | 49 +++++++++++++++ apps/web/src/frontend/lib/sql-variables.ts | 59 ++++++++++++++++++- 2 files changed, 105 insertions(+), 3 deletions(-) diff --git a/apps/web/src/frontend/lib/sql-variables.test.ts b/apps/web/src/frontend/lib/sql-variables.test.ts index e26d72f..aa55304 100644 --- a/apps/web/src/frontend/lib/sql-variables.test.ts +++ b/apps/web/src/frontend/lib/sql-variables.test.ts @@ -17,6 +17,7 @@ import { substituteVariables, type SqlVariable, SQL_VARIABLE_LIST_MAX, + SQL_VARIABLE_TABLE_MAX, } from './sql-variables'; function scalar(name: string, value: unknown, extra?: Partial): SqlVariable { @@ -220,6 +221,54 @@ SELECT 3 AS a, 4 AS b;`; }); }); + it('applySetDirectives refuses incomplete column/table captures (paged results)', () => { + const page = { + columns: ['id'], + rows: [[1], [2]], + hasNext: true, + truncated: true, + }; + // Scalar only needs the first cell — still ok on a partial page. + expect(applySetDirectives([{ mode: 'scalar', name: 'x' }], page)).toEqual({ + ok: true, + updates: [{ name: 'x', kind: 'scalar', value: 1 }], + }); + expect( + applySetDirectives([{ mode: 'column', name: 'ids', column: 'id' }], page) + ).toMatchObject({ + ok: false, + error: expect.stringMatching(/incomplete/i), + }); + expect(applySetDirectives([{ mode: 'table', name: 't' }], page)).toMatchObject({ + ok: false, + error: expect.stringMatching(/incomplete/i), + }); + }); + + it('applySetDirectives refuses silent list/table cap truncation', () => { + const listRows = Array.from({ length: SQL_VARIABLE_LIST_MAX + 1 }, (_, i) => [i]); + expect( + applySetDirectives([{ mode: 'column', name: 'ids', column: 'id' }], { + columns: ['id'], + rows: listRows, + }) + ).toMatchObject({ + ok: false, + error: expect.stringMatching(/max 500/i), + }); + + const tableRows = Array.from({ length: SQL_VARIABLE_TABLE_MAX + 1 }, (_, i) => [i]); + expect( + applySetDirectives([{ mode: 'table', name: 't' }], { + columns: ['id'], + rows: tableRows, + }) + ).toMatchObject({ + ok: false, + error: expect.stringMatching(/max 500/i), + }); + }); + it('resolveVariablesForConnection merges scalar/list overrides', () => { const vars: SqlVariable[] = [ scalar('x', 1, { overrides: { c1: { value: 99 }, c2: { value: 2 } } }), diff --git a/apps/web/src/frontend/lib/sql-variables.ts b/apps/web/src/frontend/lib/sql-variables.ts index 8258d7b..5dcbf5d 100644 --- a/apps/web/src/frontend/lib/sql-variables.ts +++ b/apps/web/src/frontend/lib/sql-variables.ts @@ -518,15 +518,44 @@ export type ApplySetResult = | { ok: true; updates: SetUpdate[] } | { ok: false; error: string }; +/** True when the result grid is not the full statement output (page/cap). */ +function resultIsIncomplete(result: { + truncated?: boolean; + hasNext?: boolean; +}): boolean { + return Boolean(result.hasNext || result.truncated); +} + +/** Count non-null cells in a result column (no list-cap early exit). */ +function countNonNullColumnValues(rows: unknown[][], colIndex: number): number { + let n = 0; + for (const row of rows) { + const cell = row[colIndex]; + if (cell !== null && cell !== undefined) n += 1; + } + return n; +} + /** * Build variable upserts from @set directives + a successful statement result. + * + * Scalar capture uses only the first cell, so a paged/truncated grid is fine. + * Column/table capture must see the full result — fail closed when the grid is + * incomplete or would silently exceed the list/table caps (otherwise later + * `${{…}}` expansion runs against a partial variable with no error). */ export function applySetDirectives( directives: SetDirective[], - result: { columns: string[]; rows: unknown[][] } + result: { + columns: string[]; + rows: unknown[][]; + truncated?: boolean; + hasNext?: boolean; + } ): ApplySetResult { if (directives.length === 0) return { ok: true, updates: [] }; const updates: SetUpdate[] = []; + const incomplete = resultIsIncomplete(result); for (const d of directives) { if (d.mode === 'scalar') { @@ -544,6 +573,12 @@ export function applySetDirectives( continue; } if (d.mode === 'column') { + if (incomplete) { + return { + ok: false, + error: `@set ${d.name}: result is incomplete (more rows available) — increase Max rows or narrow the query before capturing`, + }; + } const idx = columnIndex(result.columns, d.column); if (idx < 0) { return { @@ -551,6 +586,13 @@ export function applySetDirectives( error: `@set ${d.name}: column "${d.column}" not in result`, }; } + const nonNullCount = countNonNullColumnValues(result.rows, idx); + if (nonNullCount > SQL_VARIABLE_LIST_MAX) { + return { + ok: false, + error: `@set ${d.name}: column "${d.column}" has ${nonNullCount} values (max ${SQL_VARIABLE_LIST_MAX}) — narrow the query before capturing`, + }; + } const values = columnToListValues(result.rows, idx); if (values.length === 0) { return { @@ -565,12 +607,23 @@ export function applySetDirectives( if (result.columns.length === 0) { return { ok: false, error: `@set ${d.name}: result has no columns` }; } - const rows = result.rows.slice(0, SQL_VARIABLE_TABLE_MAX); + if (incomplete) { + return { + ok: false, + error: `@set ${d.name}: result is incomplete (more rows available) — increase Max rows or narrow the query before capturing`, + }; + } + if (result.rows.length > SQL_VARIABLE_TABLE_MAX) { + return { + ok: false, + error: `@set ${d.name}: result has ${result.rows.length} rows (max ${SQL_VARIABLE_TABLE_MAX}) — narrow the query before capturing`, + }; + } updates.push({ name: d.name, kind: 'table', columns: [...result.columns], - rows, + rows: result.rows.map((r) => [...r]), }); }