From e954d33613e4ff5027667eb8f646615d9bbd499d Mon Sep 17 00:00:00 2001 From: AlexRV12 <71396855+AlexRV12@users.noreply.github.com> Date: Thu, 17 Sep 2026 10:29:37 +0200 Subject: [PATCH 01/22] feat(ai-chat): add list_workers and list_data_metrics global tools (#11143) * feat(ai-chat): add list_workers and list_data_metrics global tools Co-Authored-By: Claude Opus 5 (1M context) * fix(ai-evals): keep the generated eval workspace id valid for the backend Co-Authored-By: Claude Opus 5 (1M context) * refactor(ai-chat): trim the worker truncation note and added comments Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01Gi6UeLKJB9UaSWtjDCVaMq * fix(ai-chat): only hedge an empty worker list when workers can be hidden Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01Gi6UeLKJB9UaSWtjDCVaMq * fix(ai-chat): show the empty-worker message in the tool card Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01Gi6UeLKJB9UaSWtjDCVaMq * fix(ai-chat): stop advertising workers in the api catalog tool Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01Gi6UeLKJB9UaSWtjDCVaMq * test(ai-evals): move the workspace id guard beside its function Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01Gi6UeLKJB9UaSWtjDCVaMq * fix(ai-chat): keep the empty metric note true for every filter shape Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01Gi6UeLKJB9UaSWtjDCVaMq * fix(ai-chat): resolve the caller role before judging an empty worker list Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01Gi6UeLKJB9UaSWtjDCVaMq * fix(ai-evals): collapse doubled hyphens in the generated workspace id Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01Gi6UeLKJB9UaSWtjDCVaMq * docs(ai-chat): describe what the ducklake tools and the worker hedge do Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01Gi6UeLKJB9UaSWtjDCVaMq * fix(ai-chat): name the missing lake when a metric table filter has none Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01Gi6UeLKJB9UaSWtjDCVaMq * fix(ai-chat): strip the ducklake scheme from the table retry hint Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01Gi6UeLKJB9UaSWtjDCVaMq * test(ai-evals): add a global case for the declared measure path Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01Gi6UeLKJB9UaSWtjDCVaMq --------- Co-authored-by: Claude Opus 5 (1M context) --- ai_evals/adapters/frontend/backendPreview.ts | 13 +- ai_evals/adapters/frontend/mockBackend.ts | 25 ++++ .../adapters/frontend/vitestAdapter.test.ts | 12 ++ ai_evals/adapters/frontend/windmillBackend.ts | 13 +- .../adapters/frontend/workspaceId.test.ts | 21 ++++ ai_evals/adapters/frontend/workspaceId.ts | 22 ++++ ai_evals/cases/global.yaml | 44 +++++-- .../initial/ducklake_orders_metrics.json | 36 ++++++ .../copilot/chat/ducklakeTools.test.ts | 75 +++++++++++- .../components/copilot/chat/ducklakeTools.ts | 85 +++++++++++-- .../chat/global/apiCatalogTools.test.ts | 63 ++++++++-- .../copilot/chat/global/apiCatalogTools.ts | 6 +- .../copilot/chat/global/core.test.ts | 115 +++++++++++++++++- .../components/copilot/chat/global/core.ts | 62 +++++++++- 14 files changed, 527 insertions(+), 65 deletions(-) create mode 100644 ai_evals/adapters/frontend/workspaceId.test.ts create mode 100644 ai_evals/adapters/frontend/workspaceId.ts create mode 100644 ai_evals/fixtures/frontend/global/initial/ducklake_orders_metrics.json diff --git a/ai_evals/adapters/frontend/backendPreview.ts b/ai_evals/adapters/frontend/backendPreview.ts index 57e1cfdf2a..8c1837c808 100644 --- a/ai_evals/adapters/frontend/backendPreview.ts +++ b/ai_evals/adapters/frontend/backendPreview.ts @@ -1,5 +1,5 @@ -import { randomUUID } from 'node:crypto' import type { BackendValidationSettings } from '../../core/backendValidation' +import { buildWorkspaceId } from './workspaceId' interface CompletedJobResultMaybe { completed: boolean @@ -24,7 +24,6 @@ export interface CompletedPreviewJob { const tokenCache = new Map>() const sharedWorkspaceQueue = new Map>() const managedSharedWorkspacePrefixes = ['f/evals/'] -const DEFAULT_WORKSPACE_PREFIX = 'ai-evals' export class BackendPreviewClient { constructor(private readonly settings: BackendValidationSettings) {} @@ -441,16 +440,6 @@ async function withSharedWorkspaceLock(workspaceId: string, body: () => Promi } } -function buildWorkspaceId(caseId: string, attempt: number): string { - const caseSlug = caseId - .toLowerCase() - .replace(/[^a-z0-9-]+/g, '-') - .replace(/^-+|-+$/g, '') - .slice(0, 30) - const suffix = randomUUID().slice(0, 8) - return `${DEFAULT_WORKSPACE_PREFIX}-${caseSlug || 'case'}-a${attempt}-${suffix}` -} - function extractFolderName(path: string): string | null { if (!path.startsWith('f/')) { return null diff --git a/ai_evals/adapters/frontend/mockBackend.ts b/ai_evals/adapters/frontend/mockBackend.ts index 8c1ca431b9..fd66c1cbcc 100644 --- a/ai_evals/adapters/frontend/mockBackend.ts +++ b/ai_evals/adapters/frontend/mockBackend.ts @@ -11,6 +11,7 @@ import type { Script } from '../../../frontend/src/lib/gen' import type { + DataMetric, DataTableTables, DataTableTableSchema, EndpointTool, @@ -112,6 +113,9 @@ export interface BenchmarkWorkspaceRunnables { aiProviders?: BenchmarkWorkspaceAiProvider[] resources?: BenchmarkWorkspaceResource[] datatables?: BenchmarkDatatableSeed[] + /** DuckLake catalog names, as `list_ducklakes` reports them. */ + ducklakes?: string[] + dataMetrics?: DataMetric[] jobs?: BenchmarkWorkspaceJob[] } @@ -673,6 +677,27 @@ export function listBenchmarkDatatables(workspace: string): DataTableTables[] | })) } +// ============= DuckLake catalogs and declared metrics ============= + +/** Seeded DuckLake names, or `null` for a non-benchmark workspace. */ +export function listBenchmarkDucklakes(workspace: string): string[] | null { + const runnables = benchmarkWorkspaceRunnables.get(workspace) + return runnables ? (runnables.ducklakes ?? []) : null +} + +/** + * Seeded metric declarations, or `null` for a non-benchmark workspace. + * + * The `table` / `path_prefix` filters are ignored: which rows a filter selects is + * `canonical_table_path`'s business and is pinned by `ducklakeTools.test.ts`. + * Re-deriving it here would give the eval its own copy of that spec to drift from, + * and the case this serves measures whether the model reaches for the tool at all. + */ +export function listBenchmarkDataMetrics(workspace: string): DataMetric[] | null { + const runnables = benchmarkWorkspaceRunnables.get(workspace) + return runnables ? (runnables.dataMetrics ?? []) : null +} + export function getBenchmarkDatatableSchema(input: { workspace: string datatableName: string diff --git a/ai_evals/adapters/frontend/vitestAdapter.test.ts b/ai_evals/adapters/frontend/vitestAdapter.test.ts index f240d0ee36..bd6419da7c 100644 --- a/ai_evals/adapters/frontend/vitestAdapter.test.ts +++ b/ai_evals/adapters/frontend/vitestAdapter.test.ts @@ -76,7 +76,9 @@ vi.mock('$lib/gen', async () => { listBenchmarkPlainResources, listBenchmarkApps, listBenchmarkDatatables, + listBenchmarkDataMetrics, listBenchmarkDrafts, + listBenchmarkDucklakes, listBenchmarkFlows, listBenchmarkJobs, listBenchmarkScripts, @@ -341,6 +343,10 @@ vi.mock('$lib/gen', async () => { hasBenchmarkWorkspace(data.workspace) ? (listBenchmarkDatatables(data.workspace) ?? []) : actual.WorkspaceService.listDataTableTables(data), + listDucklakes: async (data: { workspace: string }) => + hasBenchmarkWorkspace(data.workspace) + ? (listBenchmarkDucklakes(data.workspace) ?? []) + : actual.WorkspaceService.listDucklakes(data), getDataTableTableSchema: async (data: { workspace: string datatableName: string @@ -356,6 +362,12 @@ vi.mock('$lib/gen', async () => { }) : actual.WorkspaceService.getDataTableTableSchema(data) }), + DataMetricService: wrapService(actual.DataMetricService, { + listDataMetrics: async (data: { workspace: string }) => + hasBenchmarkWorkspace(data.workspace) + ? { metrics: listBenchmarkDataMetrics(data.workspace) ?? [] } + : actual.DataMetricService.listDataMetrics(data) + }), ScheduleService: wrapService(actual.ScheduleService, { existsSchedule: async (data: { workspace: string; path: string }) => hasBenchmarkWorkspace(data.workspace) ? false : actual.ScheduleService.existsSchedule(data), diff --git a/ai_evals/adapters/frontend/windmillBackend.ts b/ai_evals/adapters/frontend/windmillBackend.ts index 2247d8e5d5..bf483aa1f4 100644 --- a/ai_evals/adapters/frontend/windmillBackend.ts +++ b/ai_evals/adapters/frontend/windmillBackend.ts @@ -1,9 +1,8 @@ -import { randomUUID } from "node:crypto"; import type { WindmillBackendSettings } from "../../core/windmillBackendSettings"; +import { buildWorkspaceId } from "./workspaceId"; const tokenCache = new Map>(); const sharedWorkspaceQueue = new Map>(); -const DEFAULT_WORKSPACE_PREFIX = "ai-evals"; export class WindmillBackendClient { constructor(private readonly settings: WindmillBackendSettings) {} @@ -179,16 +178,6 @@ async function withSharedWorkspaceLock( } } -function buildWorkspaceId(caseId: string, attempt: number): string { - const caseSlug = caseId - .toLowerCase() - .replace(/[^a-z0-9-]+/g, "-") - .replace(/^-+|-+$/g, "") - .slice(0, 30); - const suffix = randomUUID().slice(0, 8); - return `${DEFAULT_WORKSPACE_PREFIX}-${caseSlug || "case"}-a${attempt}-${suffix}`; -} - async function expectOk(response: Response, context: string): Promise { if (response.ok) { return; diff --git a/ai_evals/adapters/frontend/workspaceId.test.ts b/ai_evals/adapters/frontend/workspaceId.test.ts new file mode 100644 index 0000000000..3573a9e51a --- /dev/null +++ b/ai_evals/adapters/frontend/workspaceId.test.ts @@ -0,0 +1,21 @@ +import { describe, expect, it } from "bun:test"; +import { buildWorkspaceId } from "./workspaceId"; + +describe("buildWorkspaceId", () => { + // `workspace.proper_id` rejects `--`, which a case id can carry itself and + // which truncating a slug on a hyphen produces once the suffix adds its own. + // One id per shape: cut landing on a hyphen, cut landing mid-word, no cut, and + // a doubled hyphen no cut ever reaches. + it("stays within the id length cap and the proper_id format", () => { + for (const caseId of [ + "global-test6-secret-variable-draft", + "global-test23-datatable-query-select", + "short", + "global--test-foo", + ]) { + const id = buildWorkspaceId(caseId, 1); + expect(id.length).toBeLessThanOrEqual(50); + expect(id).toMatch(/^\w+(-\w+)*$/); + } + }); +}); diff --git a/ai_evals/adapters/frontend/workspaceId.ts b/ai_evals/adapters/frontend/workspaceId.ts new file mode 100644 index 0000000000..9545c06a7d --- /dev/null +++ b/ai_evals/adapters/frontend/workspaceId.ts @@ -0,0 +1,22 @@ +import { randomUUID } from "node:crypto"; + +const DEFAULT_WORKSPACE_PREFIX = "ai-evals"; + +// A workspace id must be at most 50 characters AND match `^\w+(-\w+)*$` +// (`workspace.proper_id`), so the case slug yields to the random suffix that +// makes the id unique, and no hyphen may end up doubled — neither one already in +// the case id nor one a truncation leaves for the suffix to follow. +const MAX_WORKSPACE_ID_LENGTH = 50; + +export function buildWorkspaceId(caseId: string, attempt: number): string { + const caseSlug = caseId + .toLowerCase() + .replace(/[^a-z0-9-]+/g, "-") + .replace(/-{2,}/g, "-") + .replace(/^-+|-+$/g, ""); + const suffix = `-a${attempt}-${randomUUID().slice(0, 8)}`; + const head = `${DEFAULT_WORKSPACE_PREFIX}-${caseSlug || "case"}`; + return `${head + .slice(0, MAX_WORKSPACE_ID_LENGTH - suffix.length) + .replace(/-+$/, "")}${suffix}`; +} diff --git a/ai_evals/cases/global.yaml b/ai_evals/cases/global.yaml index 951d316524..e8ec660f3b 100644 --- a/ai_evals/cases/global.yaml +++ b/ai_evals/cases/global.yaml @@ -1919,10 +1919,11 @@ - when the lookup fails, tells the user instead of inventing table names - does not write scripts or resources to answer a read-only question -# --- API catalog (search_api_endpoints / call_api_get / call_api_endpoint) --- -# The harness serves the catalog and the executed calls itself (mock -# listMcpTools + benchmark fetch handlers in adapters/frontend), so these cases -# do not require an mcp-enabled eval backend. +# --- Dedicated tools preferred over the API catalog --- +# The harness serves worker/queue reads itself (benchmark fetch handlers in +# adapters/frontend), so these cases do not require an mcp-enabled eval backend. +# The stale `api-catalog` in the id below is kept so results stay comparable +# across benchmark runs. - id: global-test30-api-catalog-workers prompt: |- @@ -1934,23 +1935,42 @@ draftCountExactly: 0 toolExpect: requiredToolsUsed: - - search_api_endpoints - - call_api_get + - list_workers forbiddenToolsUsed: + - call_api_get - call_api_endpoint - write_script - deploy_workspace_item - toolCallArgs: - - tool: call_api_get - field: name - stringIncludesAnyOf: - - listWorkers # Read-only workspace inspection produces no draft; validate via tool use. skipJudge: true judgeChecklist: - - discovers the workers endpoint through the API catalog instead of guessing or fabricating + - reads worker state through list_workers instead of guessing or fabricating - reports worker status from the returned data +- id: global-test37-ducklake-declared-measure + prompt: |- + We track orders in the main ducklake. Write me a duckdb script that reports total + revenue by month. Keep it as a draft, don't deploy it. + initial: ai_evals/fixtures/frontend/global/initial/ducklake_orders_metrics.json + runtime: + maxTurns: 8 + validate: + draftCountExactly: 1 + toolExpect: + requiredToolsUsed: + - list_data_metrics + forbiddenToolsUsed: + - deploy_workspace_item + - delete_workspace_item + # The judge runs: the point is not that the tool was called but that the number it + # describes is the declared one. `revenue` excludes test rows, so an aggregate that + # reproduces it without the filter is plausible, runnable and wrong. + judgeChecklist: + - totals revenue with the declared sum over the amount column rather than an invented aggregate over a guessed column + - excludes test orders from the total, as the declared revenue measure does + - groups by month using the declared order_month expression over order_date + - does not introduce column names absent from the declarations + - id: global-test31-draft-test-run-not-deployed prompt: |- Update `f/evals/global/format_greeting` so the provided name is uppercased in the greeting, then run it with name "ada" to check it works. diff --git a/ai_evals/fixtures/frontend/global/initial/ducklake_orders_metrics.json b/ai_evals/fixtures/frontend/global/initial/ducklake_orders_metrics.json new file mode 100644 index 0000000000..fba88c53d3 --- /dev/null +++ b/ai_evals/fixtures/frontend/global/initial/ducklake_orders_metrics.json @@ -0,0 +1,36 @@ +{ + "workspace": { + "ducklakes": ["main"], + "dataMetrics": [ + { + "script_path": "f/analytics/orders_pipeline", + "table_path": "main/main.orders", + "kind": "measure", + "name": "revenue", + "expr": "sum(amount)", + "filter": "not is_test" + }, + { + "script_path": "f/analytics/orders_pipeline", + "table_path": "main/main.orders", + "kind": "measure", + "name": "order_count", + "expr": "count(*)" + }, + { + "script_path": "f/analytics/orders_pipeline", + "table_path": "main/main.orders", + "kind": "dimension", + "name": "order_month", + "expr": "date_trunc('month', order_date)" + }, + { + "script_path": "f/analytics/orders_pipeline", + "table_path": "main/main.orders", + "kind": "dimension", + "name": "region", + "expr": "region" + } + ] + } +} diff --git a/frontend/src/lib/components/copilot/chat/ducklakeTools.test.ts b/frontend/src/lib/components/copilot/chat/ducklakeTools.test.ts index fdd8c74558..d02d1b9497 100644 --- a/frontend/src/lib/components/copilot/chat/ducklakeTools.test.ts +++ b/frontend/src/lib/components/copilot/chat/ducklakeTools.test.ts @@ -1,6 +1,9 @@ import { beforeEach, describe, expect, it, vi } from 'vitest' -const { listMock } = vi.hoisted(() => ({ listMock: vi.fn() })) +const { listMock, listMetricsMock } = vi.hoisted(() => ({ + listMock: vi.fn(), + listMetricsMock: vi.fn() +})) vi.mock('./shared', () => ({ createToolDef: (_schema: unknown, name: string, description: string) => ({ @@ -10,7 +13,8 @@ vi.mock('./shared', () => ({ })) vi.mock('$lib/gen', () => ({ - WorkspaceService: { listDucklakes: listMock } + WorkspaceService: { listDucklakes: listMock }, + DataMetricService: { listDataMetrics: listMetricsMock } })) import { getDucklakeTools } from './ducklakeTools' @@ -31,7 +35,10 @@ function run(name: string, args: Record = {}) { }) } -beforeEach(() => listMock.mockReset()) +beforeEach(() => { + listMock.mockReset() + listMetricsMock.mockReset() +}) describe('list_ducklakes', () => { it('returns the configured catalog names', async () => { @@ -51,3 +58,65 @@ describe('list_ducklakes', () => { expect(result).toContain('still draft the pipeline scripts') }) }) + +describe('list_data_metrics', () => { + it('forwards the filters and returns the declarations', async () => { + listMetricsMock.mockResolvedValue({ + metrics: [ + { + script_path: 'f/analytics/rev', + table_path: 'main/main.orders', + kind: 'measure', + name: 'revenue', + expr: 'sum(amount)', + filter: 'not is_test' + } + ] + }) + const result = await run('list_data_metrics', { + table: 'ducklake://main/main.orders', + path_prefix: 'f/analytics', + limit: 50 + }) + expect(listMetricsMock).toHaveBeenCalledWith({ + workspace: 'test-workspace', + table: 'ducklake://main/main.orders', + pathPrefix: 'f/analytics', + perPage: 50 + }) + expect(JSON.parse(result).metrics[0]).toMatchObject({ name: 'revenue', expr: 'sum(amount)' }) + }) + + it('never reports an empty result as proof that nothing is declared', async () => { + listMetricsMock.mockResolvedValue({ metrics: [] }) + const result = await run('list_data_metrics', {}) + // Unreadable declarations are omitted, not flagged, so absence is unprovable. + expect(result).toContain('does not establish') + expect(result).toContain('cannot read') + }) + + // The server matches a lake-less name against nothing, so the readability hedge + // would confirm "nothing is declared" for a filter worth retrying. The scheme is + // optional on the way in, so the retry it names must not carry it back. + it.each(['orders', 'ducklake://orders'])( + 'blames the missing lake, not readability, for table %s', + async (table) => { + listMetricsMock.mockResolvedValue({ metrics: [] }) + const result = await run('list_data_metrics', { table }) + expect(result).toContain('`/orders`') + expect(result).not.toContain('cannot read') + } + ) + + it('warns that more declarations exist when the page is cut short', async () => { + listMetricsMock.mockResolvedValue({ + // A cursor only comes back on a full page, never with an empty one. + metrics: [{ table_path: 't', kind: 'measure', name: 'n', script_path: 's' }], + next_cursor: { table_path: 't', kind: 'measure', name: 'n', script_path: 's' } + }) + const result = await run('list_data_metrics', {}) + // Without this the model reads a partial page as "no such measure" and + // re-derives a number that disagrees with the declared one. + expect(result).toContain('rather than concluding a measure is undeclared') + }) +}) diff --git a/frontend/src/lib/components/copilot/chat/ducklakeTools.ts b/frontend/src/lib/components/copilot/chat/ducklakeTools.ts index 5594e4f3d0..dfadc58db9 100644 --- a/frontend/src/lib/components/copilot/chat/ducklakeTools.ts +++ b/frontend/src/lib/components/copilot/chat/ducklakeTools.ts @@ -1,17 +1,19 @@ import { z } from 'zod' -import { WorkspaceService } from '$lib/gen' +import { DataMetricService, WorkspaceService } from '$lib/gen' import { createToolDef, type Tool } from './shared' /** - * Workspace-scoped DuckLake readiness tool, the pipeline counterpart to - * `list_datatables` in `datatableTools.ts`. + * Workspace-scoped DuckLake tools, the pipeline counterpart to `list_datatables` + * in `datatableTools.ts`. * * A data pipeline materializes DuckLake tables and reads/writes S3 assets, which * only work once the workspace has object storage + a DuckLake catalog - * configured. This tool lets the chat detect that prerequisite (and warn with - * role-appropriate next steps) instead of silently producing a pipeline that - * cannot run. It is a plain read gated only by workspace membership, so it needs - * no app context and belongs in the global tool set. + * configured. `list_ducklakes` lets the chat detect that prerequisite (and warn + * with role-appropriate next steps) instead of silently producing a pipeline that + * cannot run. `list_data_metrics` reads the declarations recorded against those + * lake tables, not the tables themselves. Both are plain reads gated only by + * workspace membership, so they need no app context and belong in the global tool + * set. */ /** List the names of the DuckLake catalogs configured in the workspace. */ @@ -31,9 +33,78 @@ const listDucklakesToolDef = createToolDef( 'List the DuckLake catalogs configured in this workspace, by name. Call this before building or deploying a data pipeline that materializes DuckLake tables or reads/writes S3 assets: if it returns none, the workspace has no object storage + DuckLake configured and the pipeline cannot run until a workspace admin sets it up. Returns names only.' ) +const listDataMetricsSchema = z.object({ + table: z + .string() + .optional() + .describe( + 'Only declarations on this DuckLake table, as `/` or `/.
`, with or without the `ducklake://` scheme. A name with no lake matches nothing and comes back empty.' + ), + path_prefix: z + .string() + .optional() + .describe('Only declarations made by scripts under this path, e.g. `f/analytics`.'), + limit: z + .number() + .int() + .min(1) + .max(1000) + .optional() + .describe('Max number of declarations to return. Defaults to 200.') +}) +const listDataMetricsToolDef = createToolDef( + listDataMetricsSchema, + 'list_data_metrics', + 'List the measures and dimensions declared on DuckLake tables (from `// measure` / `// dimension` annotations in deployed scripts). Call this before writing any aggregate query over a DuckLake table: a declared measure is the canonical definition of that number, and reproducing it yourself silently disagrees with it (a `revenue` measure typically excludes refunds or test rows). Use each returned `expr` verbatim, and when a measure has a `filter` write it as `expr FILTER (WHERE filter)` so measures with different predicates share one GROUP BY. Only declarations whose producing script you can read are returned, so what comes back is never proof of what exists: if the number you need is not here you may still write your own aggregate, but say that you found no declared measure for it rather than implying none exists.' +) + +// The endpoint drops declarations whose producing script the caller cannot read +// (token scope + RLS on `script`), so an empty result means "none declared" or +// "none readable by you" and the tool cannot tell which. +const NO_DATA_METRICS_NOTE = + 'Nothing matched. That does not establish that nothing is declared: declarations whose producing script you cannot read are omitted from this list, not flagged. You may write your own aggregate, but tell the user you found no declared measure you can read rather than stating none is declared.' + +// Well under the endpoint's 1000 cap: a full page is pretty-printed into the +// chat context, and 1000 declarations would cost tens of thousands of tokens. +const DEFAULT_DATA_METRICS_LIMIT = 200 + /** The workspace DuckLake tools, for registration in global mode. */ export function getDucklakeTools(): Tool<{}>[] { return [ + { + def: listDataMetricsToolDef, + planModeSafe: true, + showDetails: true, + fn: async ({ args, workspace, toolId, toolCallbacks }) => { + const parsed = listDataMetricsSchema.parse(args) + toolCallbacks.setToolStatus(toolId, { content: 'Listing declared measures...' }) + const limit = parsed.limit ?? DEFAULT_DATA_METRICS_LIMIT + const { metrics, next_cursor } = await DataMetricService.listDataMetrics({ + workspace, + table: parsed.table, + pathPrefix: parsed.path_prefix, + perPage: limit + }) + // `canonical_table_path` expands a value only once it contains a `/`, so a bare + // table name matches nothing — a retryable filter, not a permissions outcome. + const bareTable = parsed.table?.replace('ducklake://', '') + const emptyNote = + bareTable && !bareTable.includes('/') + ? `Nothing matched \`${parsed.table}\`: a table filter must name its lake, as \`/${bareTable}\`. Re-call with the lake before concluding anything about what is declared.` + : NO_DATA_METRICS_NOTE + const note = next_cursor + ? `More declarations exist beyond the first ${limit}. Re-call with table/path_prefix to target what you are looking for, or with a higher limit (max 1000) for the rest of the list, rather than concluding a measure is undeclared.` + : metrics.length === 0 + ? emptyNote + : undefined + const result = JSON.stringify({ metrics, ...(note ? { note } : {}) }, null, 2) + toolCallbacks.setToolStatus(toolId, { + content: `Listed ${metrics.length} declared measure(s)/dimension(s)`, + result + }) + return result + } + }, { def: listDucklakesToolDef, planModeSafe: true, diff --git a/frontend/src/lib/components/copilot/chat/global/apiCatalogTools.test.ts b/frontend/src/lib/components/copilot/chat/global/apiCatalogTools.test.ts index a78f09071a..06109708de 100644 --- a/frontend/src/lib/components/copilot/chat/global/apiCatalogTools.test.ts +++ b/frontend/src/lib/components/copilot/chat/global/apiCatalogTools.test.ts @@ -31,6 +31,24 @@ const CATALOG = [ properties: { page: { type: 'integer' }, per_page: { type: 'integer' } } } }, + { + name: 'listDataMetrics', + description: 'List declared measures and dimensions', + instructions: '', + path: '/w/{workspace}/data_metrics/list', + method: 'GET' + }, + { + name: 'listQueue', + description: 'List queued jobs', + instructions: 'List the jobs waiting in the queue', + path: '/w/{workspace}/jobs/queue/list', + method: 'GET', + query_params_schema: { + type: 'object', + properties: { running: { type: 'boolean' }, per_page: { type: 'integer' } } + } + }, { name: 'getJobUpdates', description: 'Get job updates', @@ -177,11 +195,12 @@ beforeEach(() => { describe('search_api_endpoints', () => { it('matches on name/path tokens, plural-insensitively, and excludes covered endpoints', async () => { - const result = await run('search_api_endpoints', { query: 'worker' }) - expect(result.matches.map((m: any) => m.name)).toEqual(['listWorkers']) - expect(result.matches[0].endpoint).toBe('GET /workers/list') - expect(result.matches[0].params).toEqual(['page', 'per_page']) - expect(result.matches[0].instructions).toContain('ping status') + // Singular "job" matches the plural "jobs" path segment. + const result = await run('search_api_endpoints', { query: 'list queued job' }) + expect(result.matches[0].name).toBe('listQueue') + expect(result.matches[0].endpoint).toBe('GET /w/{workspace}/jobs/queue/list') + expect(result.matches[0].params).toEqual(['running', 'per_page']) + expect(result.matches[0].instructions).toContain('waiting in the queue') const flows = await run('search_api_endpoints', { query: 'create flow' }) expect(flows.matches.map((m: any) => m.name)).not.toContain('createFlow') @@ -191,8 +210,11 @@ describe('search_api_endpoints', () => { it('returns endpoint categories when nothing matches', async () => { const result = await run('search_api_endpoints', { query: 'kubernetes' }) expect(result.matches).toEqual([]) - expect(result.hint).toContain('workers') - expect(result.hint).toContain('jobs') + expect(result.hint).toContain('jobs_u') + // Categories are built from the uncovered endpoints only, so a covered one + // must not be advertised as somewhere to retry. + expect(result.hint).not.toContain('workers') + expect(result.hint).not.toContain('data_metrics') }) }) @@ -253,6 +275,21 @@ describe('call_api_get', () => { expect(search.matches.map((m: any) => m.name)).not.toContain('getScriptByPath') }) + it('refuses the worker and data-metric reads, pointing at their dedicated tools', async () => { + for (const [name, tool, query] of [ + ['listWorkers', 'list_workers', 'workers'], + ['listDataMetrics', 'list_data_metrics', 'data metrics'] + ]) { + const result = await run('call_api_get', { name }) + expect(result.success).toBe(false) + expect(result.error).toContain(tool) + + const search = await run('search_api_endpoints', { query }) + expect(search.matches.map((m: any) => m.name)).not.toContain(name) + expect(search.covered_by_dedicated_tools?.join(' ')).toContain(tool) + } + }) + it('refuses variable reads so variable values never reach the model', async () => { const result = await run('call_api_get', { name: 'getVariable' }) expect(result.success).toBe(false) @@ -273,12 +310,14 @@ describe('call_api_get', () => { const fetchMock = vi.fn().mockResolvedValue({ ok: true, headers: new Headers({ 'content-type': 'application/json' }), - json: async () => [{ worker: 'w1' }] + json: async () => [{ id: 'job-1' }] }) vi.stubGlobal('fetch', fetchMock) - const result = await run('call_api_get', { name: 'listWorkers', params: { page: 2 } }) - expect(fetchMock).toHaveBeenCalledWith('/api/workers/list?page=2', { method: 'GET' }) - expect(result).toEqual({ success: true, data: [{ worker: 'w1' }] }) + const result = await run('call_api_get', { name: 'listQueue', params: { running: true } }) + expect(fetchMock).toHaveBeenCalledWith('/api/w/test-ws/jobs/queue/list?running=true', { + method: 'GET' + }) + expect(result).toEqual({ success: true, data: [{ id: 'job-1' }] }) }) }) @@ -305,7 +344,7 @@ describe('call_api_endpoint', () => { }) it('redirects GET endpoints to call_api_get', async () => { - const result = await run('call_api_endpoint', { name: 'listWorkers' }) + const result = await run('call_api_endpoint', { name: 'listQueue' }) expect(result.error).toContain('call_api_get') }) }) diff --git a/frontend/src/lib/components/copilot/chat/global/apiCatalogTools.ts b/frontend/src/lib/components/copilot/chat/global/apiCatalogTools.ts index 308d222544..42c68c6ebf 100644 --- a/frontend/src/lib/components/copilot/chat/global/apiCatalogTools.ts +++ b/frontend/src/lib/components/copilot/chat/global/apiCatalogTools.ts @@ -58,6 +58,8 @@ const COVERED_ENDPOINTS: Record = { searchDocs: 'search_docs', readDocsPage: 'read_docs_page', listJobs: 'list_runs', + listWorkers: 'list_workers', + listDataMetrics: 'list_data_metrics', getJob: 'get_run', getJobLogs: 'get_run', runScriptPreviewAndWaitResult: 'test_run_script' @@ -231,7 +233,7 @@ const searchApiEndpointsSchema = z.object({ query: z .string() .describe( - 'Keywords matched against endpoint names, paths, and descriptions (e.g. "workers", "queue", "run flow"). Jobs are called "runs" in the UI.' + 'Keywords matched against endpoint names, paths, and descriptions (e.g. "queue", "run flow", "audit log"). Jobs are called "runs" in the UI.' ) }) @@ -264,7 +266,7 @@ export const apiCatalogTools: Tool<{}>[] = [ def: createToolDef( searchApiEndpointsSchema, 'search_api_endpoints', - 'Search the Windmill REST API endpoint catalog for operations no dedicated tool covers (workers, queue state, job details, running deployed items, deletions, ...). Returns endpoint names to pass to call_api_get or call_api_endpoint.' + 'Search the Windmill REST API endpoint catalog for operations no dedicated tool covers (queue state, job details, running deployed items, deletions, ...). Returns endpoint names to pass to call_api_get or call_api_endpoint.' ), planModeSafe: true, fn: async ({ args, workspace, toolId, toolCallbacks }) => { diff --git a/frontend/src/lib/components/copilot/chat/global/core.test.ts b/frontend/src/lib/components/copilot/chat/global/core.test.ts index fe0477a544..822b36135c 100644 --- a/frontend/src/lib/components/copilot/chat/global/core.test.ts +++ b/frontend/src/lib/components/copilot/chat/global/core.test.ts @@ -256,6 +256,9 @@ vi.mock('$lib/gen', async () => { createVariable: vi.fn(async () => 'created'), updateVariable: vi.fn(async () => 'updated') }), + WorkerService: wrapService(actual.WorkerService, { + listWorkers: vi.fn(async () => []) + }), FolderService: wrapService(actual.FolderService, { createFolder: vi.fn(async () => 'created') }), @@ -264,6 +267,17 @@ vi.mock('$lib/gen', async () => { const user = whoamiByWorkspace.get(workspace) if (!user) throw new Error(`not a member of ${workspace}`) return user + }), + // `refreshSuperadmin` cancels the previous in-flight call, so this stands in for + // the CancelablePromise the real client returns. + globalWhoami: vi.fn(() => { + const pending: any = Promise.resolve({ + email: 'devops@windmill.dev', + super_admin: false, + devops: true + }) + pending.cancel = () => {} + return pending }) }), DraftService: wrapService(actual.DraftService, { @@ -389,9 +403,10 @@ import { ScheduleService, ScriptService, UserService, - VariableService + VariableService, + WorkerService } from '$lib/gen' -import { superadmin, userStore, usersWorkspaceStore } from '$lib/stores' +import { devopsRole, superadmin, userStore, usersWorkspaceStore } from '$lib/stores' import { processSecretArgs } from '$lib/components/secretArgUtils' import { clearWorkspaceRoleCache } from '$lib/user' import { get } from 'svelte/store' @@ -738,6 +753,102 @@ describe('global AI tools', () => { ) }) + it('lists workers with the diagnostic fields only', async () => { + vi.mocked(WorkerService.listWorkers).mockResolvedValueOnce([ + { + worker: 'wk-1', + worker_instance: 'host-1', + worker_group: 'gpu', + custom_tags: ['gpu'], + last_ping: 3, + jobs_executed: 12, + started_at: '2024-01-01T00:00:00Z', + ip: '10.0.0.1', + wm_version: 'v1', + memory: 123, + occupancy_rate: 0.5 + } + ]) + + const result = await callGlobalTool('list_workers', {}) + + expect(JSON.parse(result).workers).toEqual([ + { + worker: 'wk-1', + worker_group: 'gpu', + custom_tags: ['gpu'], + last_ping: 3, + jobs_executed: 12 + } + ]) + // Page telemetry must stay out of the model's context. + expect(result).not.toContain('occupancy_rate') + expect(result).not.toContain('10.0.0.1') + }) + + it('says so when the worker page is cut short', async () => { + vi.mocked(WorkerService.listWorkers).mockResolvedValueOnce( + Array(100).fill({ + worker: 'wk', + worker_group: 'default', + custom_tags: [], + last_ping: 1, + jobs_executed: 0 + }) as any + ) + + const result = await callGlobalTool('list_workers', {}) + + // A full page is indistinguishable from the whole fleet, and the model reasons + // about tag coverage from this list. + expect(JSON.parse(result).note).toContain('Only the first 100 workers') + }) + + describe('list_workers with nothing to show', () => { + afterEach(() => { + superadmin.set(undefined) + devopsRole.set(undefined) + }) + + it('never reports an empty list as an absence to a caller workers can be hidden from', async () => { + superadmin.set(false) + devopsRole.set(false) + vi.mocked(WorkerService.listWorkers).mockResolvedValueOnce([]) + + const result = await callGlobalTool('list_workers', {}) + + // An instance hiding workers from a non-devops caller answers with an empty + // list, so absence is unprovable here. + expect(result).toContain('does NOT establish that no workers are running') + expect(result).toContain('devops role') + expect(result).not.toContain('"workers"') + }) + + it('reports an empty list as an absence to a devops caller', async () => { + devopsRole.set('devops@windmill.dev') + vi.mocked(WorkerService.listWorkers).mockResolvedValueOnce([]) + + const result = await callGlobalTool('list_workers', {}) + + // Nothing is hidden from this caller, so hedging would withhold the answer a + // stuck queue is waiting on. + expect(result).toContain('No workers are connected') + expect(result).not.toContain('does NOT establish') + }) + + it('resolves the role before deciding, rather than reading unloaded stores as no role', async () => { + // Both stores start undefined; without the refresh a devops caller whose whoami + // has not landed yet is hedged at instead of answered. + expect(get(superadmin)).toBeUndefined() + expect(get(devopsRole)).toBeUndefined() + vi.mocked(WorkerService.listWorkers).mockResolvedValueOnce([]) + + const result = await callGlobalTool('list_workers', {}) + + expect(result).toContain('No workers are connected') + }) + }) + it('returns args, result and logs of a run in one call', async () => { const result = await callGlobalTool('get_run', { id: 'job-123' }) diff --git a/frontend/src/lib/components/copilot/chat/global/core.ts b/frontend/src/lib/components/copilot/chat/global/core.ts index 1511bf24a6..f1dcc0ef0a 100644 --- a/frontend/src/lib/components/copilot/chat/global/core.ts +++ b/frontend/src/lib/components/copilot/chat/global/core.ts @@ -17,7 +17,8 @@ import { ScriptService, SqsTriggerService, VariableService, - WebsocketTriggerService + WebsocketTriggerService, + WorkerService } from '$lib/gen' import { createTwoFilesPatch } from 'diff' import { deepEqual } from 'fast-equals' @@ -179,6 +180,7 @@ import { workspaceStore } from '$lib/stores' import { getWorkspaceRole, type RoleLookup } from '$lib/user' +import { refreshSuperadmin } from '$lib/refreshUser' import { get } from 'svelte/store' import { canonicalDraftSideValue, @@ -724,6 +726,17 @@ const listRunsSchema = z.object({ .describe('Max number of runs to return, most recent first. Defaults to 30.') }) +// `GET /workers/list` hides workers from a caller without the devops role by +// returning an empty list, not an error, when HIDE_WORKERS_FOR_NON_ADMINS is set. +// The flag is not visible here, so absence is only provable for a devops or +// superadmin caller; every other caller gets the hedge even where nothing is hidden. +const NO_WORKERS_VISIBLE_MESSAGE = + 'No workers came back. This does NOT establish that no workers are running: an instance can hide workers from callers without the devops role, and it does so by returning an empty list rather than an error. ' + + 'Tell the user you cannot see any workers and that worker visibility may be restricted for your account, and suggest they check the Workers page themselves. Never state that no workers are online or that the instance has none.' +const NO_WORKERS_CONNECTED_MESSAGE = + 'No workers are connected to this instance (none pinged in the last 5 minutes). Queued runs will stay queued until a worker starts.' +const WORKER_PAGE_SIZE = 100 + const deleteWorkspaceItemSchema = z.object({ type: itemTypeSchema, path: z.string().describe('Workspace path of the item to delete.'), @@ -1360,7 +1373,7 @@ ${pipelineBullet} ? ' By default it preselects the items this chat modified; pass items (":" entries) to control the selection' : ' Pass items (":" entries naming the items you changed) so the review is scoped to them — omitting items preselects every pending change in the workspace' }, or mode ("draft" or "fork") to force which comparison is shown. Prefer offering this review page over calling deploy_workspace_item directly when several items changed. -- For a Windmill operation no other tool covers (workers, queue state, a run's args, ...), use search_api_endpoints to find a REST endpoint, then call_api_get for reads or call_api_endpoint for mutations (the user is asked to confirm those). Always prefer a dedicated tool when one exists; endpoints for authoring or deleting scripts, flows, apps, schedules, resources, or variables are not available through the API catalog tools — use the draft tools and delete_workspace_item instead. +- For a Windmill operation no other tool covers (queue state, a run's args, ...), use search_api_endpoints to find a REST endpoint, then call_api_get for reads or call_api_endpoint for mutations (the user is asked to confirm those). Always prefer a dedicated tool when one exists; endpoints for authoring or deleting scripts, flows, apps, schedules, resources, or variables are not available through the API catalog tools — use the draft tools and delete_workspace_item instead. - Default to test_run_script, test_run_flow, or test_run_step for any run request, an existing script included; they prefer drafts and need no deployment. Use run_script or run_flow only when the user names the deployed version ("the deployed X", "in production", "for real") — a bare "run X" is not that. For those two, read the item with read_workspace_item version: "deployed" first so the arguments match the deployed schema. test_run_script, test_run_flow, test_run_step, run_script and run_flow all show the user an argument form prefilled with what you sent, so fill in every argument you can infer rather than asking for it in chat. test_run_step's form is the step's own inputs, not the flow's. - When a required decision is ambiguous, use askUserQuestion with two to ten clear proposed answer strings instead of guessing. The user can also type a custom answer when none of the proposed answers fit. Set multiSelect: true only when the answers can genuinely co-apply and the user may pick several (not mutually exclusive). - When the user asks you to remember a lasting preference, always/never do something, or change/stop a behavior going forward, call update_user_instructions to persist it. It edits only the USER INSTRUCTIONS block (not WORKSPACE INSTRUCTIONS). Keep each instruction concise; do not use it for one-off requests scoped to the current task. @@ -3742,6 +3755,49 @@ export const globalTools: Tool<{}>[] = [ return result } }, + { + def: createToolDef( + z.object({}), + 'list_workers', + 'List the workers connected to this Windmill instance (those that pinged in the last 5 minutes), with their worker group, custom tags, seconds since their last ping, and jobs executed. Pair with list_runs to diagnose a stuck queue: runs queued on a tag no listed worker picks up will never start. Three blind spots to report rather than reason past: an empty result states whether no worker is connected or whether workers may be hidden from you, so relay the one it gives instead of picking; a missing custom_tags can mean tags are hidden from you, not unset; and only the 100 most recently pinging workers are listed, so on a bigger instance a tag none of them carries may still be served.' + ), + planModeSafe: true, + showDetails: true, + fn: async ({ toolId, toolCallbacks }) => { + toolCallbacks.setToolStatus(toolId, { content: 'Listing workers...' }) + const pings = await WorkerService.listWorkers({ perPage: WORKER_PAGE_SIZE }) + if (pings.length === 0) { + // Both role stores resolve asynchronously, and an unloaded one must not read as + // an absent role — that hedges the answer this branch exists to give plainly. + // No-ops once they hold a value. + await refreshSuperadmin() + const hiddenFromCaller = !get(superadmin) && !get(devopsRole) + const message = hiddenFromCaller ? NO_WORKERS_VISIBLE_MESSAGE : NO_WORKERS_CONNECTED_MESSAGE + toolCallbacks.setToolStatus(toolId, { + content: hiddenFromCaller ? 'No workers visible' : 'No workers connected', + result: message + }) + return message + } + const workers = pings.map((w) => ({ + worker: w.worker, + worker_group: w.worker_group, + custom_tags: w.custom_tags, + last_ping: w.last_ping, + jobs_executed: w.jobs_executed + })) + const note = + workers.length === WORKER_PAGE_SIZE + ? `Only the first ${WORKER_PAGE_SIZE} workers are listed; more may be connected.` + : undefined + const result = JSON.stringify({ workers, ...(note ? { note } : {}) }, null, 2) + toolCallbacks.setToolStatus(toolId, { + content: `Listed ${workers.length} worker(s)`, + result + }) + return result + } + }, { def: createToolDef( getRunSchema, @@ -4263,7 +4319,7 @@ export const globalTools: Tool<{}>[] = [ }, // Workspace-scoped datatable tools (unrestricted: no whitelist, no creation policy) ...getDatatableTools(), - // Workspace DuckLake readiness (storage prerequisite check for pipelines) + // Workspace DuckLake: pipeline storage prerequisite, and declared measures ...getDucklakeTools(), // Read-only tools over files the user attached to the conversation ...fileTools, From 381d4470ef699ea82283742132e56556b95d2bd2 Mon Sep 17 00:00:00 2001 From: hugocasa Date: Thu, 17 Sep 2026 11:40:00 +0200 Subject: [PATCH 02/22] fix: disable a schedule whose cron has no run left instead of panicking (#11195) Co-authored-by: Claude Opus 5 --- backend/windmill-common/src/utils.rs | 43 ++++++++++++++++--- backend/windmill-queue/src/schedule.rs | 5 +-- backend/windmill-queue/tests/schedule_push.rs | 41 ++++++++++++++++++ 3 files changed, 79 insertions(+), 10 deletions(-) diff --git a/backend/windmill-common/src/utils.rs b/backend/windmill-common/src/utils.rs index 4b93bb89ff..fd0bd634ea 100644 --- a/backend/windmill-common/src/utils.rs +++ b/backend/windmill-common/src/utils.rs @@ -976,18 +976,33 @@ fn six_fields_hint(schedule_str: &str, version: Option<&str>, seconds_required: } impl ScheduleType { + /// `NotFound` means the expression has no run left (an expired year, an impossible + /// date), and schedule pushes disable the schedule on it. Every other error must stay + /// transient: croner fails across a DST jump longer than an hour (Antarctica/Troll) + /// and succeeds again once the jump has passed. pub fn find_next( &self, starting_from: &chrono::DateTime, - ) -> chrono::DateTime { + ) -> Result> { + let no_run_left = || { + Error::NotFound(format!( + "cron: the schedule has no run left after {}", + starting_from.format("%Y-%m-%d %H:%M:%S %Z") + )) + }; match self { ScheduleType::Croner(croner_schedule) => croner_schedule .find_next_occurrence(starting_from, false) - .expect("cron: a schedule should have a next event"), - ScheduleType::Cron(schedule) => schedule - .after(starting_from) - .next() - .expect("cron: a schedule should have a next event"), + .map_err(|e| match e { + croner::errors::CronError::TimeSearchLimitExceeded => no_run_left(), + e => Error::internal_err(format!( + "cron: could not compute the run after {}: {e}", + starting_from.format("%Y-%m-%d %H:%M:%S %Z") + )), + }), + ScheduleType::Cron(schedule) => { + schedule.after(starting_from).next().ok_or_else(no_run_left) + } } } @@ -1709,6 +1724,22 @@ mod tests { assert!(!err.contains("6 fields"), "{err}"); } + #[test] + fn find_next_reports_only_a_cron_with_no_run_left_as_not_found() { + use chrono::TimeZone; + let troll: chrono_tz::Tz = "Antarctica/Troll".parse().unwrap(); + // Troll's clocks jump from 01:00 to 03:00 on the last Sunday of March. + let before_jump = troll.with_ymd_and_hms(2027, 3, 28, 0, 30, 0).unwrap(); + + let expired = ScheduleType::from_str("0 0 9 1 1 * 2026", Some("v1"), true).unwrap(); + let err = expired.find_next(&before_jump).unwrap_err(); + assert!(matches!(err, Error::NotFound(_)), "{err}"); + + let across_jump = ScheduleType::from_str("0 30 1 * * *", Some("v2"), true).unwrap(); + let err = across_jump.find_next(&before_jump).unwrap_err(); + assert!(!matches!(err, Error::NotFound(_)), "{err}"); + } + /// A worker that restarts must land on the exact same name to reclaim its `worker_ping` /// row, while still never colliding with the other workers of its own process. The /// suffix must also stay a single `-` segment, which is what the interactive shell tag diff --git a/backend/windmill-queue/src/schedule.rs b/backend/windmill-queue/src/schedule.rs index fa495c9cd2..5d5cf005fc 100644 --- a/backend/windmill-queue/src/schedule.rs +++ b/backend/windmill-queue/src/schedule.rs @@ -166,13 +166,10 @@ pub async fn push_scheduled_job<'c>( } }; - let next = sched.find_next(&starting_from); - // println!("next event ({:?}): {}", tz, next); - // println!("next event(UTC): {}", next.with_timezone(&chrono::Utc)); + let next = sched.find_next(&starting_from)?; // Scheduled events must be stored in the database in UTC let next = next.with_timezone(&chrono::Utc); - // panic!("next: {}", next); let already_exists: bool = sqlx::query_scalar!( // Query plan: // - use of the `ix_v2_job_root_by_path` index; hence the `parent_job IS NULL` clause. diff --git a/backend/windmill-queue/tests/schedule_push.rs b/backend/windmill-queue/tests/schedule_push.rs index d58d9a5a46..333d211765 100644 --- a/backend/windmill-queue/tests/schedule_push.rs +++ b/backend/windmill-queue/tests/schedule_push.rs @@ -921,6 +921,47 @@ mod schedule_push { Ok(()) } + // ----------------------------------------------------------------------- + // try_schedule_next_job: a cron with no run left disables the schedule + // ----------------------------------------------------------------------- + + #[sqlx::test(migrations = "../migrations", fixtures("base", "schedule_push"))] + async fn test_cron_with_no_run_left_disables_schedule( + db: Pool, + ) -> anyhow::Result<()> { + sqlx::query( + "INSERT INTO schedule (workspace_id, path, edited_by, edited_at, schedule, timezone, enabled, script_path, is_flow, email, extra_perms, ws_error_handler_muted, no_flow_overlap, permissioned_as, cron_version) + VALUES ('test-workspace', 'f/system/test_schedule', 'test-user', now(), '0 0 9 1 1 * 2020', 'UTC', true, 'f/system/test_script', false, 'test@windmill.dev', '{}', false, false, 'u/test-user', 'v1')" + ) + .execute(&db) + .await?; + + let schedule = make_schedule(|s| { + s.schedule = "0 0 9 1 1 * 2020".to_string(); + s.cron_version = Some("v1".to_string()); + }); + let job = make_completed_job(&schedule); + + let tx = db.begin().await?; + let (tx, err) = + try_schedule_next_job(&db, tx, &job, &schedule, &schedule.script_path).await; + assert!(err.is_none(), "completion must go through, got: {err:?}"); + tx.commit().await?; + + assert_eq!(count_queued_jobs(&db).await, 0); + let (enabled, error): (bool, Option) = sqlx::query_as( + "SELECT enabled, error FROM schedule WHERE workspace_id = 'test-workspace' AND path = 'f/system/test_schedule'", + ) + .fetch_one(&db) + .await?; + assert!(!enabled, "schedule with no run left must be disabled"); + assert!( + error.as_deref().is_some_and(|e| e.contains("no run left")), + "error should say why, got: {error:?}" + ); + Ok(()) + } + // ----------------------------------------------------------------------- // try_schedule_next_job: disabled schedule leaves no side effects // ----------------------------------------------------------------------- From 4eab995cf7cf091a5e4640da4cb77e0921bb7fdf Mon Sep 17 00:00:00 2001 From: Guilhem Date: Thu, 17 Sep 2026 11:41:49 +0200 Subject: [PATCH 03/22] feat: tell test flow conversations from deployed ones and rename a chat (#11179) * feat: mark test flow conversations apart from deployed ones and allow renaming a chat Co-Authored-By: Claude Fable 5.1 * fix: keep the conversation kind across refreshes and reject NUL titles Co-Authored-By: Claude Fable 5.1 * fix: ignore conversation lists for a kind no longer selected Co-Authored-By: Claude Opus 5 (1M context) * fix: start a fresh conversation listing when the kind changes on a later page Co-Authored-By: Claude Opus 5 (1M context) * fix: refuse sending into a conversation of the other kind Co-Authored-By: Claude Opus 5 (1M context) * fix: refuse cross-kind conversation continuations on the server Co-Authored-By: Claude Opus 5 (1M context) * fix: keep the conversation filter unavailable while an answer runs Co-Authored-By: Claude Opus 5 (1M context) --------- Co-authored-by: Claude Fable 5.1 --- ...1cfe028a8b2a32bd790840cef65c452a8c31.json} | 10 +- ...0cc0aef2e7bde732899976eddac36a2da7658.json | 24 ++ ...9b6b2cc43b51f2056c677d58f39c31fb26cb.json} | 13 +- ...45427131bf8008eba6920021d6a449791534.json} | 10 +- ...6155738_flow_conversation_is_test.down.sql | 1 + ...916155738_flow_conversation_is_test.up.sql | 26 ++ backend/summarized_schema.txt | 2 +- backend/tests/v2_job_delete_orphans.rs | 1 + .../src/lib.rs | 75 ++++- backend/windmill-api-jobs/src/execution.rs | 3 + backend/windmill-api/openapi.yaml | 48 ++- backend/windmill-api/src/jobs.rs | 2 + .../windmill-common/src/flow_conversations.rs | 38 ++- chat-sdk/README.md | 7 +- chat-sdk/src/api.ts | 22 +- chat-sdk/src/assistant-ui.ts | 3 +- chat-sdk/src/chat.ts | 40 ++- chat-sdk/src/history.ts | 7 + chat-sdk/src/index.ts | 1 + chat-sdk/src/react.ts | 2 + chat-sdk/src/types.ts | 17 +- chat-sdk/src/utils.ts | 6 + chat-sdk/test/chat.test.ts | 127 ++++++++ .../lib/components/FlowPreviewContent.svelte | 2 +- .../components/flows/content/FlowInput.svelte | 2 +- .../flows/conversations/FlowChat.svelte | 19 +- .../conversations/FlowChatInterface.svelte | 25 +- .../FlowConversationsSidebar.svelte | 279 +++++++++++++++--- 28 files changed, 727 insertions(+), 85 deletions(-) rename backend/.sqlx/{query-6f32c1feed096ff706ae359ad6a3ca33b3f82ca38289dfa4a69aa95041027d57.json => query-48c8522a4fed219c5011f4ba63c81cfe028a8b2a32bd790840cef65c452a8c31.json} (77%) create mode 100644 backend/.sqlx/query-5b9c9eb64051f291fed4be9bc0b0cc0aef2e7bde732899976eddac36a2da7658.json rename backend/.sqlx/{query-c1e3ed3ecc3bcb98f60ba8196d33fee4a74f61b061e5025ecb75882208b3ba8f.json => query-6d259b8cce5da5fecefe4ce322789b6b2cc43b51f2056c677d58f39c31fb26cb.json} (71%) rename backend/.sqlx/{query-c383cc023714b361d10c10e8fef1fc148ab1da942951ee9ffdddaecee76a6be9.json => query-dd84f9dfb238d18cb74f9e43228345427131bf8008eba6920021d6a449791534.json} (76%) create mode 100644 backend/migrations/20260916155738_flow_conversation_is_test.down.sql create mode 100644 backend/migrations/20260916155738_flow_conversation_is_test.up.sql diff --git a/backend/.sqlx/query-6f32c1feed096ff706ae359ad6a3ca33b3f82ca38289dfa4a69aa95041027d57.json b/backend/.sqlx/query-48c8522a4fed219c5011f4ba63c81cfe028a8b2a32bd790840cef65c452a8c31.json similarity index 77% rename from backend/.sqlx/query-6f32c1feed096ff706ae359ad6a3ca33b3f82ca38289dfa4a69aa95041027d57.json rename to backend/.sqlx/query-48c8522a4fed219c5011f4ba63c81cfe028a8b2a32bd790840cef65c452a8c31.json index 0e92e3aa99..1b50ef4134 100644 --- a/backend/.sqlx/query-6f32c1feed096ff706ae359ad6a3ca33b3f82ca38289dfa4a69aa95041027d57.json +++ b/backend/.sqlx/query-48c8522a4fed219c5011f4ba63c81cfe028a8b2a32bd790840cef65c452a8c31.json @@ -1,6 +1,6 @@ { "db_name": "PostgreSQL", - "query": "SELECT id, workspace_id, flow_path, title, created_at, updated_at, created_by\n FROM flow_conversation\n WHERE id = $1 AND workspace_id = $2\n FOR UPDATE", + "query": "SELECT id, workspace_id, flow_path, title, created_at, updated_at, created_by, is_test\n FROM flow_conversation\n WHERE id = $1 AND workspace_id = $2", "describe": { "columns": [ { @@ -37,6 +37,11 @@ "ordinal": 6, "name": "created_by", "type_info": "Varchar" + }, + { + "ordinal": 7, + "name": "is_test", + "type_info": "Bool" } ], "parameters": { @@ -52,8 +57,9 @@ true, false, false, + false, false ] }, - "hash": "6f32c1feed096ff706ae359ad6a3ca33b3f82ca38289dfa4a69aa95041027d57" + "hash": "48c8522a4fed219c5011f4ba63c81cfe028a8b2a32bd790840cef65c452a8c31" } diff --git a/backend/.sqlx/query-5b9c9eb64051f291fed4be9bc0b0cc0aef2e7bde732899976eddac36a2da7658.json b/backend/.sqlx/query-5b9c9eb64051f291fed4be9bc0b0cc0aef2e7bde732899976eddac36a2da7658.json new file mode 100644 index 0000000000..8864b75b79 --- /dev/null +++ b/backend/.sqlx/query-5b9c9eb64051f291fed4be9bc0b0cc0aef2e7bde732899976eddac36a2da7658.json @@ -0,0 +1,24 @@ +{ + "db_name": "PostgreSQL", + "query": "UPDATE flow_conversation SET title = $1, updated_at = updated_at\n WHERE id = $2 AND workspace_id = $3\n RETURNING id", + "describe": { + "columns": [ + { + "ordinal": 0, + "name": "id", + "type_info": "Uuid" + } + ], + "parameters": { + "Left": [ + "Varchar", + "Uuid", + "Text" + ] + }, + "nullable": [ + false + ] + }, + "hash": "5b9c9eb64051f291fed4be9bc0b0cc0aef2e7bde732899976eddac36a2da7658" +} diff --git a/backend/.sqlx/query-c1e3ed3ecc3bcb98f60ba8196d33fee4a74f61b061e5025ecb75882208b3ba8f.json b/backend/.sqlx/query-6d259b8cce5da5fecefe4ce322789b6b2cc43b51f2056c677d58f39c31fb26cb.json similarity index 71% rename from backend/.sqlx/query-c1e3ed3ecc3bcb98f60ba8196d33fee4a74f61b061e5025ecb75882208b3ba8f.json rename to backend/.sqlx/query-6d259b8cce5da5fecefe4ce322789b6b2cc43b51f2056c677d58f39c31fb26cb.json index 50ba9d2897..b666243d40 100644 --- a/backend/.sqlx/query-c1e3ed3ecc3bcb98f60ba8196d33fee4a74f61b061e5025ecb75882208b3ba8f.json +++ b/backend/.sqlx/query-6d259b8cce5da5fecefe4ce322789b6b2cc43b51f2056c677d58f39c31fb26cb.json @@ -1,6 +1,6 @@ { "db_name": "PostgreSQL", - "query": "INSERT INTO flow_conversation (id, workspace_id, flow_path, created_by, title)\n VALUES ($1, $2, $3, $4, $5)\n ON CONFLICT (id) DO NOTHING\n RETURNING id, workspace_id, flow_path, title, created_at, updated_at, created_by", + "query": "INSERT INTO flow_conversation (id, workspace_id, flow_path, created_by, title, is_test)\n VALUES ($1, $2, $3, $4, $5, $6)\n ON CONFLICT (id) DO NOTHING\n RETURNING id, workspace_id, flow_path, title, created_at, updated_at, created_by, is_test", "describe": { "columns": [ { @@ -37,6 +37,11 @@ "ordinal": 6, "name": "created_by", "type_info": "Varchar" + }, + { + "ordinal": 7, + "name": "is_test", + "type_info": "Bool" } ], "parameters": { @@ -45,7 +50,8 @@ "Varchar", "Varchar", "Varchar", - "Varchar" + "Varchar", + "Bool" ] }, "nullable": [ @@ -55,8 +61,9 @@ true, false, false, + false, false ] }, - "hash": "c1e3ed3ecc3bcb98f60ba8196d33fee4a74f61b061e5025ecb75882208b3ba8f" + "hash": "6d259b8cce5da5fecefe4ce322789b6b2cc43b51f2056c677d58f39c31fb26cb" } diff --git a/backend/.sqlx/query-c383cc023714b361d10c10e8fef1fc148ab1da942951ee9ffdddaecee76a6be9.json b/backend/.sqlx/query-dd84f9dfb238d18cb74f9e43228345427131bf8008eba6920021d6a449791534.json similarity index 76% rename from backend/.sqlx/query-c383cc023714b361d10c10e8fef1fc148ab1da942951ee9ffdddaecee76a6be9.json rename to backend/.sqlx/query-dd84f9dfb238d18cb74f9e43228345427131bf8008eba6920021d6a449791534.json index 56a3642faa..a73c736a1d 100644 --- a/backend/.sqlx/query-c383cc023714b361d10c10e8fef1fc148ab1da942951ee9ffdddaecee76a6be9.json +++ b/backend/.sqlx/query-dd84f9dfb238d18cb74f9e43228345427131bf8008eba6920021d6a449791534.json @@ -1,6 +1,6 @@ { "db_name": "PostgreSQL", - "query": "SELECT id, workspace_id, flow_path, title, created_at, updated_at, created_by\n FROM flow_conversation\n WHERE id = $1 AND workspace_id = $2", + "query": "SELECT id, workspace_id, flow_path, title, created_at, updated_at, created_by, is_test\n FROM flow_conversation\n WHERE id = $1 AND workspace_id = $2\n FOR UPDATE", "describe": { "columns": [ { @@ -37,6 +37,11 @@ "ordinal": 6, "name": "created_by", "type_info": "Varchar" + }, + { + "ordinal": 7, + "name": "is_test", + "type_info": "Bool" } ], "parameters": { @@ -52,8 +57,9 @@ true, false, false, + false, false ] }, - "hash": "c383cc023714b361d10c10e8fef1fc148ab1da942951ee9ffdddaecee76a6be9" + "hash": "dd84f9dfb238d18cb74f9e43228345427131bf8008eba6920021d6a449791534" } diff --git a/backend/migrations/20260916155738_flow_conversation_is_test.down.sql b/backend/migrations/20260916155738_flow_conversation_is_test.down.sql new file mode 100644 index 0000000000..aa186105cf --- /dev/null +++ b/backend/migrations/20260916155738_flow_conversation_is_test.down.sql @@ -0,0 +1 @@ +ALTER TABLE flow_conversation DROP COLUMN is_test; diff --git a/backend/migrations/20260916155738_flow_conversation_is_test.up.sql b/backend/migrations/20260916155738_flow_conversation_is_test.up.sql new file mode 100644 index 0000000000..970375a84d --- /dev/null +++ b/backend/migrations/20260916155738_flow_conversation_is_test.up.sql @@ -0,0 +1,26 @@ +-- A chat run from the flow editor's test panel is stored exactly like one from the +-- deployed flow, so the two were indistinguishable once written. Marking them lets the +-- lists tell a trial apart from a real conversation. +ALTER TABLE flow_conversation ADD COLUMN is_test BOOLEAN NOT NULL DEFAULT false; + +-- Existing rows: a conversation whose messages came from a flowpreview run was a test. +-- Derived once here because the job is purged on retention, after which the origin of an +-- old conversation is unknowable. +-- +-- Walked to the root job rather than matched directly: an existing message row never holds +-- the flow job itself. The rows point at the step that produced them — the AI agent's job +-- for an answer, the tool's own job for a tool call — whose kind is never 'flowpreview'. +-- +-- `root_job` first, matching `get_root_job_id` (windmill-worker/src/common.rs): only it +-- reaches the top of the run. `flow_innermost_root_job` stops at the closest flow scope by +-- design, so an agent inside a subflow would land on that subflow's 'flow' row and the +-- conversation would read as deployed. +UPDATE flow_conversation c +SET is_test = true +WHERE EXISTS ( + SELECT 1 FROM flow_conversation_message m + JOIN v2_job j ON j.id = m.job_id + JOIN v2_job root + ON root.id = coalesce(j.root_job, j.flow_innermost_root_job, j.parent_job, j.id) + WHERE m.conversation_id = c.id AND root.kind = 'flowpreview' +); diff --git a/backend/summarized_schema.txt b/backend/summarized_schema.txt index 1e27ee1ce4..cab1d7fe68 100644 --- a/backend/summarized_schema.txt +++ b/backend/summarized_schema.txt @@ -99,7 +99,7 @@ email_trigger: path(char), local_part(char), workspaced_local_part(bool), script favorite: usr(char), workspace_id(char), path(char), favorite_kind(favorite_kind) flow: workspace_id(char), path(char), summary(text), description(text), value(jsonb), edited_by(char), edited_at(ts), archived(bool), schema(json), extra_perms(jsonb), dependency_job(uuid), draft_only(bool), tag(char), ws_error_handler_muted(bool), dedicated_worker(bool), timeout(int), visible_to_runner_only(bool), concurrency_key(char), versions(bigint[]), on_behalf_of(varchar), on_behalf_of_email(text), lock_error_logs(text), labels(text[]) FK: (workspace_id) -> workspace(id) -flow_conversation: id(uuid), workspace_id(char), flow_path(char), title(char), created_at(ts), updated_at(ts), created_by(char) +flow_conversation: id(uuid), workspace_id(char), flow_path(char), title(char), created_at(ts), updated_at(ts), created_by(char), is_test(bool) FK: (workspace_id) -> workspace(id) flow_conversation_message: id(uuid), conversation_id(uuid), message_type(message_type), content(text), job_id(uuid), created_at(ts), created_seq(int8), step_name(char), success(bool) FK: (conversation_id) -> flow_conversation(id) | (job_id) -> v2_job(id) diff --git a/backend/tests/v2_job_delete_orphans.rs b/backend/tests/v2_job_delete_orphans.rs index ded760b8ff..b45fa4201f 100644 --- a/backend/tests/v2_job_delete_orphans.rs +++ b/backend/tests/v2_job_delete_orphans.rs @@ -258,6 +258,7 @@ async fn test_new_turns_wait_for_conversation_cleanup_and_recreate( "test-user", "hi again", conv_id, + false, ) .await?; windmill_common::flow_conversations::add_message_to_conversation_tx( diff --git a/backend/windmill-api-flow-conversations/src/lib.rs b/backend/windmill-api-flow-conversations/src/lib.rs index e85af5b83b..80eebfeda9 100644 --- a/backend/windmill-api-flow-conversations/src/lib.rs +++ b/backend/windmill-api-flow-conversations/src/lib.rs @@ -1,6 +1,6 @@ use axum::{ extract::{Path, Query}, - routing::{delete, get}, + routing::{delete, get, post}, Extension, Json, Router, }; use chrono::{DateTime, Utc}; @@ -15,13 +15,14 @@ use windmill_common::{ db::{UserDB, DB}, error::{JsonResult, Result}, flow_conversations::MessageType, - utils::{not_found_if_none, paginate, Pagination}, + utils::{not_found_if_none, paginate, truncate_with_ellipsis, Pagination}, }; pub fn workspaced_service() -> Router { Router::new() .route("/list", get(list_conversations)) .route("/delete/{conversation_id}", delete(delete_conversation)) + .route("/update/{conversation_id}", post(update_conversation)) .route("/{conversation_id}/messages", get(list_messages)) } @@ -38,9 +39,22 @@ pub struct FlowConversationMessage { pub success: bool, } +/// Which conversations a listing holds. A test chat was started from the editor's test +/// panel; a deployed one from the flow itself. +#[derive(Deserialize, Default, Clone, Copy)] +#[serde(rename_all = "lowercase")] +pub enum ConversationKind { + Test, + /// The default: a deployed flow's chat should not surface someone's trial runs. + #[default] + Deployed, + All, +} + #[derive(Deserialize)] pub struct ListConversationsQuery { pub flow_path: Option, + pub kind: Option, } #[derive(Deserialize)] @@ -67,6 +81,7 @@ async fn list_conversations( "created_at", "updated_at", "created_by", + "is_test", ]) .and_where_eq("workspace_id", "?".bind(&w_id)); @@ -74,6 +89,16 @@ async fn list_conversations( sqlb.and_where_eq("flow_path", "?".bind(flow_path)); } + match query.kind.unwrap_or_default() { + ConversationKind::Test => { + sqlb.and_where_eq("is_test", "true"); + } + ConversationKind::Deployed => { + sqlb.and_where_eq("is_test", "false"); + } + ConversationKind::All => {} + } + sqlb.order_by("updated_at", true) .limit(per_page as i64) .offset(offset as i64); @@ -101,7 +126,7 @@ async fn delete_conversation( // Verify the conversation exists and belongs to the user let conversation = sqlx::query_as!( FlowConversation, - "SELECT id, workspace_id, flow_path, title, created_at, updated_at, created_by + "SELECT id, workspace_id, flow_path, title, created_at, updated_at, created_by, is_test FROM flow_conversation WHERE id = $1 AND workspace_id = $2", conversation_id, @@ -148,6 +173,50 @@ async fn delete_conversation( Ok(format!("Conversation {} deleted", conversation_id)) } +#[derive(Deserialize)] +pub struct UpdateConversation { + pub title: String, +} + +async fn update_conversation( + authed: ApiAuthed, + Extension(user_db): Extension, + Path((w_id, conversation_id)): Path<(String, Uuid)>, + Json(update): Json, +) -> Result { + // Postgres refuses a NUL in a text column, so it must not reach the query as a 500. + if update.title.contains('\0') { + return Err(windmill_common::error::Error::BadRequest( + "title cannot contain a NUL character".to_string(), + )); + } + // The column is VARCHAR(255) and the helper appends an ellipsis to what it cuts, so the + // bound it takes is three short of the column's. A longer title would otherwise reach + // Postgres as a 22001 and come back a 500. + let title = truncate_with_ellipsis(update.title.trim(), 252); + + let mut tx = user_db.clone().begin(&authed).await?; + + // `updated_at` is kept: the list is ordered by it, and a rename must not move the + // chat to the top the way a new turn does. + let updated = sqlx::query_scalar!( + "UPDATE flow_conversation SET title = $1, updated_at = updated_at + WHERE id = $2 AND workspace_id = $3 + RETURNING id", + title, + conversation_id, + &w_id + ) + .fetch_optional(&mut *tx) + .await?; + + not_found_if_none(updated, "Conversation", conversation_id.to_string())?; + + tx.commit().await?; + + Ok(format!("Conversation {} updated", conversation_id)) +} + async fn list_messages( authed: ApiAuthed, Extension(user_db): Extension, diff --git a/backend/windmill-api-jobs/src/execution.rs b/backend/windmill-api-jobs/src/execution.rs index c15068583d..11fb883429 100644 --- a/backend/windmill-api-jobs/src/execution.rs +++ b/backend/windmill-api-jobs/src/execution.rs @@ -669,6 +669,7 @@ pub async fn handle_chat_conversation_messages( run_query: &RunJobQuery, user_message_raw: Option<&Box>, job_id: Uuid, + is_test: bool, ) -> error::Result<()> { // Names the query parameter rather than the field: it is not a flow argument, and // supplying it as one is the first thing tried on reading `memory_id is required`. @@ -701,6 +702,7 @@ pub async fn handle_chat_conversation_messages( &authed.username, &user_message, memory_id, + is_test, ) .await?; @@ -836,6 +838,7 @@ pub async fn run_flow<'c>( &run_query, args.args.get("user_message"), uuid, + false, ) .await?; } diff --git a/backend/windmill-api/openapi.yaml b/backend/windmill-api/openapi.yaml index 2cb8a0e831..650331e41c 100644 --- a/backend/windmill-api/openapi.yaml +++ b/backend/windmill-api/openapi.yaml @@ -12464,6 +12464,15 @@ paths: in: query schema: type: string + - name: kind + description: which conversations to list - the flow editor's test chats, the deployed flow's own (the default), or both + in: query + schema: + type: string + enum: + - test + - deployed + - all responses: "200": description: flow conversations list @@ -12474,6 +12483,40 @@ paths: items: $ref: "#/components/schemas/FlowConversation" + /w/{workspace}/flow_conversations/update/{conversation_id}: + post: + summary: rename flow conversation + operationId: updateFlowConversation + tags: + - flow_conversations + parameters: + - $ref: "#/components/parameters/WorkspaceId" + - name: conversation_id + description: conversation id + in: path + required: true + schema: + type: string + format: uuid + requestBody: + required: true + content: + application/json: + schema: + type: object + required: [title] + properties: + title: + type: string + description: the chat's name + responses: + "200": + description: flow conversation updated + content: + text/plain: + schema: + type: string + /w/{workspace}/flow_conversations/delete/{conversation_id}: delete: summary: delete flow conversation @@ -28301,7 +28344,7 @@ components: FlowConversation: type: object required: - [id, workspace_id, flow_path, created_at, updated_at, created_by] + [id, workspace_id, flow_path, created_at, updated_at, created_by, is_test] properties: id: type: string @@ -28328,6 +28371,9 @@ components: created_by: type: string description: Username who created the conversation + is_test: + type: boolean + description: Started from the flow editor's test panel rather than a deployed run FlowConversationMessage: type: object diff --git a/backend/windmill-api/src/jobs.rs b/backend/windmill-api/src/jobs.rs index bcc2fed94f..e31b12a53f 100644 --- a/backend/windmill-api/src/jobs.rs +++ b/backend/windmill-api/src/jobs.rs @@ -9554,6 +9554,8 @@ async fn run_preview_flow_job( &run_query, user_message.as_ref(), uuid, + // Run from the editor's test panel: a trial, not a real conversation. + true, ) .await?; } diff --git a/backend/windmill-common/src/flow_conversations.rs b/backend/windmill-common/src/flow_conversations.rs index b62f768bbc..673fc430a4 100644 --- a/backend/windmill-common/src/flow_conversations.rs +++ b/backend/windmill-common/src/flow_conversations.rs @@ -26,8 +26,12 @@ pub struct FlowConversation { pub created_at: DateTime, pub updated_at: DateTime, pub created_by: String, + /// Started from the flow editor's test panel rather than a deployed run. + pub is_test: bool, } +/// `is_test` is written on insert. An existing conversation of the other kind refuses the +/// turn, so preview and deployed runs never share one. pub async fn get_or_create_conversation_with_id( tx: &mut sqlx::Transaction<'_, sqlx::Postgres>, w_id: &str, @@ -35,9 +39,10 @@ pub async fn get_or_create_conversation_with_id( username: &str, title: &str, conversation_id: Uuid, + is_test: bool, ) -> Result { if let Some(existing) = lock_conversation(tx, w_id, conversation_id).await? { - return Ok(existing); + return same_kind(existing, is_test); } // Truncate title to 25 characters max @@ -47,15 +52,16 @@ pub async fn get_or_create_conversation_with_id( // wins, the others wait on it, do nothing, and read the row it created. let created = sqlx::query_as!( FlowConversation, - "INSERT INTO flow_conversation (id, workspace_id, flow_path, created_by, title) - VALUES ($1, $2, $3, $4, $5) + "INSERT INTO flow_conversation (id, workspace_id, flow_path, created_by, title, is_test) + VALUES ($1, $2, $3, $4, $5, $6) ON CONFLICT (id) DO NOTHING - RETURNING id, workspace_id, flow_path, title, created_at, updated_at, created_by", + RETURNING id, workspace_id, flow_path, title, created_at, updated_at, created_by, is_test", conversation_id, w_id, flow_path, username, - title + title, + is_test ) .fetch_optional(&mut **tx) .await?; @@ -63,13 +69,29 @@ pub async fn get_or_create_conversation_with_id( return Ok(conversation); } - lock_conversation(tx, w_id, conversation_id) + // The concurrent first turn that won the insert may have been of the other kind. + let existing = lock_conversation(tx, w_id, conversation_id) .await? .ok_or_else(|| { crate::error::Error::BadRequest(format!( "conversation {conversation_id} belongs to another workspace" )) - }) + })?; + same_kind(existing, is_test) +} + +/// `memory_id` is the caller's to choose, so a preview run could name a deployed +/// conversation and the reverse. A conversation's kind is fixed at creation and nothing +/// would show the mixing afterwards, so the turn is refused before it starts. +fn same_kind(existing: FlowConversation, is_test: bool) -> Result { + if existing.is_test == is_test { + return Ok(existing); + } + Err(crate::error::Error::BadRequest(if existing.is_test { + "this conversation was started from the flow editor's test panel; start a new conversation to run the deployed flow".to_string() + } else { + "this conversation belongs to the deployed flow; start a new conversation to test from the flow editor".to_string() + })) } /// Locked, so a turn orders against retention collecting the conversation @@ -83,7 +105,7 @@ async fn lock_conversation( ) -> Result> { Ok(sqlx::query_as!( FlowConversation, - "SELECT id, workspace_id, flow_path, title, created_at, updated_at, created_by + "SELECT id, workspace_id, flow_path, title, created_at, updated_at, created_by, is_test FROM flow_conversation WHERE id = $1 AND workspace_id = $2 FOR UPDATE", diff --git a/chat-sdk/README.md b/chat-sdk/README.md index a233425d14..ba76d1d507 100644 --- a/chat-sdk/README.md +++ b/chat-sdk/README.md @@ -225,8 +225,11 @@ answer, an `assistant` message with `success: false`. `status: 'error'` (with `e set) means the turn could not run or be followed at all, such as a refused request. Methods: `sendMessage(text, { inputs? })`, `stop()`, `newConversation()`, -`selectConversation(id)`, `loadConversations({ page?, perPage? })`, -`deleteConversation(id)`, `loadOlderMessages()`, `destroy()`. Switching conversations +`selectConversation(id)`, `loadConversations({ page?, perPage?, kind? })`, +`deleteConversation(id)`, `renameConversation(id, title)`, `loadOlderMessages()`, +`destroy()`. `kind` lists the flow editor's test chats (`'test'`), the deployed flow's +own (`'deployed'`, the server's default) or both (`'all'`); each `Conversation` carries +`isTest`. A rename keeps the conversation's place in the list. Switching conversations stops following the current answer; the flow keeps running and, with server history, its answer is there when you come back. diff --git a/chat-sdk/src/api.ts b/chat-sdk/src/api.ts index afc50ff0a1..4bba733196 100644 --- a/chat-sdk/src/api.ts +++ b/chat-sdk/src/api.ts @@ -28,8 +28,16 @@ export interface FlowConversation { created_at: string updated_at: string created_by: string + /** Started from the flow editor's test panel rather than a deployed run. */ + is_test: boolean } +/** + * Which conversations a listing holds: the flow editor's test chats, the deployed flow's + * own (the server's default), or both. + */ +export type ConversationKind = 'test' | 'deployed' | 'all' + export interface FlowConversationMessage { id: string conversation_id: string @@ -167,15 +175,25 @@ export class WindmillChatApi { async listConversations( flowPath: string, - options: { page?: number; perPage?: number; signal?: AbortSignal } = {} + options: { page?: number; perPage?: number; kind?: ConversationKind; signal?: AbortSignal } = {} ): Promise { + const extra: Record = { flow_path: flowPath } + if (options.kind !== undefined) extra.kind = options.kind const res = await this.#request('flow_conversations/list', { - query: pagination(options, { flow_path: flowPath }), + query: pagination(options, extra), signal: options.signal }) return (await res.json()) as FlowConversation[] } + /** Sets a conversation's title. Its place in the list is kept: only a turn moves one. */ + async renameConversation(conversationId: string, title: string): Promise { + await this.#request(`flow_conversations/update/${encodeURIComponent(conversationId)}`, { + method: 'POST', + body: { title } + }) + } + /** * Without `afterSeq`: one page counted from the newest message, returned oldest first. * With `afterSeq`: the messages created after that cursor, oldest first. diff --git a/chat-sdk/src/assistant-ui.ts b/chat-sdk/src/assistant-ui.ts index ef22f28531..36133c2272 100644 --- a/chat-sdk/src/assistant-ui.ts +++ b/chat-sdk/src/assistant-ui.ts @@ -53,7 +53,8 @@ export function useWindmillRuntime(options: WindmillRuntimeOptions): AssistantRu threads: chat.conversations.map((c) => ({ status: 'regular' as const, id: c.id, title: c.title })), onSwitchToNewThread: () => chat.newConversation(), onSwitchToThread: (id) => chat.selectConversation(id), - onDelete: (id) => chat.deleteConversation(id) + onDelete: (id) => chat.deleteConversation(id), + onRename: (id, title) => chat.renameConversation(id, title) } } : undefined diff --git a/chat-sdk/src/chat.ts b/chat-sdk/src/chat.ts index dda8f10d05..8055ea2234 100644 --- a/chat-sdk/src/chat.ts +++ b/chat-sdk/src/chat.ts @@ -1,6 +1,7 @@ import { WindmillApiError, WindmillChatApi, + type ConversationKind, type FlowConversation, type FlowConversationMessage } from './api' @@ -18,6 +19,7 @@ import type { } from './types' import { conversationTitle, + truncateTitle, errorResultMessage, extractChatAnswer, isAbortError, @@ -59,6 +61,8 @@ class ChatImpl implements Chat { #state: ChatState #turn: Turn | undefined #page = 1 + /** The kind the caller last listed, so the refresh after a new turn lists the same rows. */ + #conversationKind: ConversationKind | undefined #persistTimer: ReturnType | undefined constructor(options: ChatOptions) { @@ -229,15 +233,21 @@ class ChatImpl implements Chat { } loadConversations = async ( - options: { page?: number; perPage?: number } = {} + options: { page?: number; perPage?: number; kind?: ConversationKind } = {} ): Promise => { const page = options.page ?? 1 + // A different kind is a different listing: its first rows replace the held ones, on + // whichever page they were asked for. + const kindChanged = 'kind' in options && options.kind !== this.#conversationKind + if ('kind' in options) this.#conversationKind = options.kind + const kind = this.#conversationKind let conversations: Conversation[] if (this.#state.history === 'server') { try { const rows = await this.#api.listConversations(this.#config.flowPath, { page, - perPage: options.perPage ?? this.#config.pageSize + perPage: options.perPage ?? this.#config.pageSize, + kind }) conversations = rows.map(fromConversation) } catch (e) { @@ -247,10 +257,13 @@ class ChatImpl implements Chat { } else { conversations = this.#state.history === 'local' ? this.#local.listConversations() : [] } + // Another kind was asked for while this list was on its way: its rows are not the + // listing any more, whichever response lands last. + if (kind !== this.#conversationKind) return conversations const known = new Set(this.#state.conversations.map((c) => c.id)) this.#set({ conversations: - page === 1 + page === 1 || kindChanged ? conversations : [...this.#state.conversations, ...conversations.filter((c) => !known.has(c.id))] }) @@ -273,6 +286,24 @@ class ChatImpl implements Chat { this.#set({ conversations: this.#state.conversations.filter((c) => c.id !== conversationId) }) } + renameConversation = async (conversationId: string, title: string): Promise => { + // Cut here as the server cuts, so the title shown is the one stored. + const trimmed = truncateTitle(title.trim()) + if (!trimmed) return + if (this.#state.history === 'server') { + await this.#api.renameConversation(conversationId, trimmed) + } else if (this.#state.history === 'local') { + this.#local.renameConversation(conversationId, trimmed) + } + // Patched in place: the server keeps `updated_at` on a rename, so the list order the + // next load returns is the one shown now. + this.#set({ + conversations: this.#state.conversations.map((c) => + c.id === conversationId ? { ...c, title: trimmed } : c + ) + }) + } + loadOlderMessages = async (): Promise => { const conversationId = this.#state.conversationId if ( @@ -734,7 +765,8 @@ function fromConversation(row: FlowConversation): Conversation { id: row.id, title: row.title ?? undefined, createdAt: row.created_at, - updatedAt: row.updated_at + updatedAt: row.updated_at, + isTest: row.is_test } } diff --git a/chat-sdk/src/history.ts b/chat-sdk/src/history.ts index 485dd056cf..b2eb124e8f 100644 --- a/chat-sdk/src/history.ts +++ b/chat-sdk/src/history.ts @@ -4,6 +4,8 @@ export interface LocalHistory { listConversations(): Conversation[] getMessages(conversationId: string): ChatMessage[] upsertConversation(conversation: Conversation): void + /** Changes a stored conversation's title in place; unlike `upsertConversation`, its position is kept. */ + renameConversation(conversationId: string, title: string): void saveMessages(conversationId: string, messages: ChatMessage[]): void deleteConversation(conversationId: string): void } @@ -55,6 +57,11 @@ export function createLocalHistory(storage: StorageLike | undefined, key: string } write(s) }, + renameConversation(id, title) { + const s = read() + s.conversations = s.conversations.map((c) => (c.id === id ? { ...c, title } : c)) + write(s) + }, saveMessages(id, messages) { const s = read() s.messages[id] = messages.map((m) => ({ ...m, pending: false })) diff --git a/chat-sdk/src/index.ts b/chat-sdk/src/index.ts index 353c814154..1c18a5924b 100644 --- a/chat-sdk/src/index.ts +++ b/chat-sdk/src/index.ts @@ -5,6 +5,7 @@ export { WindmillApiError, readServerSentEvents, type WindmillChatApiOptions, + type ConversationKind, type FlowConversation, type FlowConversationMessage, type JobUpdateEvent, diff --git a/chat-sdk/src/react.ts b/chat-sdk/src/react.ts index ecba159cb3..33193842f2 100644 --- a/chat-sdk/src/react.ts +++ b/chat-sdk/src/react.ts @@ -11,6 +11,7 @@ export type UseWindmillChat = ChatState & | 'selectConversation' | 'loadConversations' | 'deleteConversation' + | 'renameConversation' | 'loadOlderMessages' > & { chat: Chat } @@ -65,6 +66,7 @@ export function useWindmillChat(options: ChatOptions): UseWindmillChat { selectConversation: chat.selectConversation, loadConversations: chat.loadConversations, deleteConversation: chat.deleteConversation, + renameConversation: chat.renameConversation, loadOlderMessages: chat.loadOlderMessages }), [state, chat] diff --git a/chat-sdk/src/types.ts b/chat-sdk/src/types.ts index 92b8dfb89b..5e0d673bd9 100644 --- a/chat-sdk/src/types.ts +++ b/chat-sdk/src/types.ts @@ -52,6 +52,11 @@ export interface Conversation { title: string | undefined createdAt: string updatedAt: string + /** + * Started from the flow editor's test panel rather than a deployed run. Known once the + * server has listed the conversation; unset for one only this client has seen. + */ + isTest?: boolean } export interface ChatState { @@ -128,8 +133,18 @@ export interface Chat { stop(): Promise newConversation(): void selectConversation(conversationId: string): Promise - loadConversations(options?: { page?: number; perPage?: number }): Promise + /** + * `kind` narrows server history to the flow editor's test chats, the deployed flow's + * own (the server's default), or both. Local history has no test chats and ignores it. + */ + loadConversations(options?: { + page?: number + perPage?: number + kind?: 'test' | 'deployed' | 'all' + }): Promise deleteConversation(conversationId: string): Promise + /** Sets a conversation's title. The list keeps its order: only a turn moves a conversation. */ + renameConversation(conversationId: string, title: string): Promise loadOlderMessages(): Promise /** Stops background work (stream, polling) and writes local history out. The chat stays usable. */ destroy(): void diff --git a/chat-sdk/src/utils.ts b/chat-sdk/src/utils.ts index fe7e5d8960..7973914e93 100644 --- a/chat-sdk/src/utils.ts +++ b/chat-sdk/src/utils.ts @@ -69,6 +69,12 @@ export function conversationTitle(firstMessage: string): string { return chars.length > 25 ? `${chars.slice(0, 25).join('')}...` : firstMessage } +/** The server's bound on a typed title: 252 characters plus an ellipsis fits its 255-char column. */ +export function truncateTitle(title: string): string { + const chars = Array.from(title) + return chars.length > 252 ? `${chars.slice(0, 252).join('')}...` : title +} + export function sleep(ms: number, signal?: AbortSignal): Promise { return new Promise((resolve, reject) => { if (signal?.aborted) return reject(abortError()) diff --git a/chat-sdk/test/chat.test.ts b/chat-sdk/test/chat.test.ts index 1a5be8375d..89c4565942 100644 --- a/chat-sdk/test/chat.test.ts +++ b/chat-sdk/test/chat.test.ts @@ -414,6 +414,112 @@ describe('createChat with server history', () => { expect(chat.getState().conversations.map((c) => c.id)).toEqual(['c1', 'c2']) }) + test('lists one kind of conversation and carries which kind each one is', async () => { + const row = (id: string, is_test: boolean) => ({ + id, + workspace_id: 'ws', + flow_path: FLOW, + title: id, + created_at: '2026-01-01T00:00:00Z', + updated_at: '2026-01-01T00:00:00Z', + created_by: 'admin', + is_test + }) + const { fetch, calls } = fetchMock((c) => + c.url.pathname === '/api/w/ws/flow_conversations/list' + ? json(c.url.searchParams.get('kind') === 'test' ? [row('t1', true)] : [row('d1', false)]) + : undefined + ) + const chat = createChat(options({}, fetch)) + await chat.loadConversations() + expect(calls[0].url.searchParams.has('kind')).toBe(false) + expect(chat.getState().conversations.map((c) => [c.id, c.isTest])).toEqual([['d1', false]]) + await chat.loadConversations({ kind: 'test' }) + expect(calls[1].url.searchParams.get('kind')).toBe('test') + expect(chat.getState().conversations.map((c) => [c.id, c.isTest])).toEqual([['t1', true]]) + }) + + test('a list for a kind no longer asked for does not replace the newer one', async () => { + const row = (id: string, is_test: boolean) => ({ + id, + workspace_id: 'ws', + flow_path: FLOW, + title: id, + created_at: '2026-01-01T00:00:00Z', + updated_at: '2026-01-01T00:00:00Z', + created_by: 'admin', + is_test + }) + const { fetch } = fetchMock((c) => { + if (c.url.pathname !== '/api/w/ws/flow_conversations/list') return undefined + if (c.url.searchParams.get('kind') === 'test') { + return new Promise((r) => setTimeout(() => r(json([row('t1', true)])), 50)) + } + return json([row('d1', false)]) + }) + const chat = createChat(options({}, fetch)) + const slow = chat.loadConversations({ kind: 'test' }) + await chat.loadConversations({ kind: 'deployed' }) + await slow + expect(chat.getState().conversations.map((c) => c.id)).toEqual(['d1']) + // Another kind asked for on a later page starts its own listing rather than appending. + await chat.loadConversations({ page: 2, kind: 'test' }) + expect(chat.getState().conversations.map((c) => c.id)).toEqual(['t1']) + }) + + test('the refresh after a new turn lists the kind last asked for', async () => { + const { fetch, calls } = fetchMock( + run, + (c) => + c.url.pathname === streamPath + ? sse([{ type: 'update', completed: true, only_result: { output: 'Hello', messages: [] } }]) + : undefined, + (c) => + c.method === 'GET' && c.url.pathname.endsWith('/messages') + ? json([messageRow(11, 'user', 'hi'), messageRow(12, 'assistant', 'Hello', { job_id: 'agent-job' })]) + : undefined, + (c) => (c.url.pathname === '/api/w/ws/flow_conversations/list' ? json([]) : undefined) + ) + const chat = createChat(options({}, fetch)) + await chat.loadConversations({ kind: 'test' }) + await chat.sendMessage('hi') + const lists = calls.filter((c) => c.url.pathname === '/api/w/ws/flow_conversations/list') + expect(lists.length).toBeGreaterThan(1) + expect(lists.every((c) => c.url.searchParams.get('kind') === 'test')).toBe(true) + }) + + test('renaming a conversation keeps its place in the list', async () => { + const row = (id: string) => ({ + id, + workspace_id: 'ws', + flow_path: FLOW, + title: id, + created_at: '2026-01-01T00:00:00Z', + updated_at: '2026-01-01T00:00:00Z', + created_by: 'admin', + is_test: false + }) + const { fetch, calls } = fetchMock( + (c) => (c.url.pathname === '/api/w/ws/flow_conversations/list' ? json([row('c1'), row('c2')]) : undefined), + (c) => + c.method === 'POST' && c.url.pathname === '/api/w/ws/flow_conversations/update/c2' + ? text('Conversation c2 updated') + : undefined + ) + const chat = createChat(options({}, fetch)) + await chat.loadConversations() + await chat.renameConversation('c2', ' Budget review ') + expect(calls[1].body).toEqual({ title: 'Budget review' }) + expect(chat.getState().conversations.map((c) => [c.id, c.title])).toEqual([ + ['c1', 'c1'], + ['c2', 'Budget review'] + ]) + // Cut as the server cuts, so what is shown is what is stored. + await chat.renameConversation('c2', 'x'.repeat(300)) + expect(chat.getState().conversations[1].title).toBe('x'.repeat(252) + '...') + expect(calls[2].body).toEqual({ title: 'x'.repeat(252) + '...' }) + }) + test('a turn started right after stop() is not touched by the stop sync', async () => { let jobs = 0 const { fetch } = fetchMock( @@ -726,6 +832,27 @@ describe('createChat with server history', () => { expect((await again.loadConversations()).map((c) => c.id)).toEqual([newer, older]) }) + test('renaming a local conversation persists the title without reordering history', async () => { + const storage = memoryStorage() + const { fetch, calls } = fetchMock(run, (c) => + c.url.pathname === streamPath ? sse([{ type: 'update', completed: true, only_result: 'ok' }]) : undefined + ) + const chat = createChat(options({ token: 'tok', storage }, fetch)) + await chat.sendMessage('older') + const older = chat.getState().conversationId! + chat.newConversation() + await chat.sendMessage('newer') + const newer = chat.getState().conversationId! + const before = calls.length + await chat.renameConversation(older, 'Renamed') + expect(calls.length).toBe(before) + const again = createChat(options({ token: 'tok', storage }, fetch)) + expect((await again.loadConversations()).map((c) => [c.id, c.title])).toEqual([ + [newer, 'newer'], + [older, 'Renamed'] + ]) + }) + test('destroying the chat mid-turn leaves it idle', async () => { const { fetch } = fetchMock(run, (c) => c.url.pathname === streamPath diff --git a/frontend/src/lib/components/FlowPreviewContent.svelte b/frontend/src/lib/components/FlowPreviewContent.svelte index cd2460b1b0..32b46cb2be 100644 --- a/frontend/src/lib/components/FlowPreviewContent.svelte +++ b/frontend/src/lib/components/FlowPreviewContent.svelte @@ -470,7 +470,7 @@ ) return jobId ?? '' }} - hideSidebar={true} + conversationKind="test" path={$pathStore} inputSchema={flowStore.val.schema} flowModules={flowStore.val.value?.modules} diff --git a/frontend/src/lib/components/flows/content/FlowInput.svelte b/frontend/src/lib/components/flows/content/FlowInput.svelte index eb78dbdc89..145ecce9ea 100644 --- a/frontend/src/lib/components/flows/content/FlowInput.svelte +++ b/frontend/src/lib/components/flows/content/FlowInput.svelte @@ -849,7 +849,7 @@ diff --git a/frontend/src/lib/components/flows/conversations/FlowChat.svelte b/frontend/src/lib/components/flows/conversations/FlowChat.svelte index 1025ddaa15..24c41a2ca9 100644 --- a/frontend/src/lib/components/flows/conversations/FlowChat.svelte +++ b/frontend/src/lib/components/flows/conversations/FlowChat.svelte @@ -28,6 +28,13 @@ /** The flow's description, shown under the empty transcript's prompt. */ description?: string wideLayout?: boolean + /** + * What this surface's own runs are: the editor runs previews and lists its test + * chats, the flow page runs the deployed flow and lists only its users' chats. + * The sidebar offers the kind filter everywhere but on the deployed flow, whose + * users have no test chats to look at. + */ + conversationKind?: 'test' | 'deployed' } let { @@ -38,7 +45,8 @@ inputSchema = undefined, flowModules = undefined, description = undefined, - wideLayout = false + wideLayout = false, + conversationKind = 'deployed' }: Props = $props() const flowEditorContext = getContext('FlowEditorContext') @@ -97,7 +105,13 @@
{#if chat && chatState} {#if !hideSidebar} - + {/if} @@ -111,6 +125,7 @@ {workspace} {description} {wideLayout} + {conversationKind} /> {/key} {/if} diff --git a/frontend/src/lib/components/flows/conversations/FlowChatInterface.svelte b/frontend/src/lib/components/flows/conversations/FlowChatInterface.svelte index f6aa0670c4..72982b828e 100644 --- a/frontend/src/lib/components/flows/conversations/FlowChatInterface.svelte +++ b/frontend/src/lib/components/flows/conversations/FlowChatInterface.svelte @@ -32,6 +32,8 @@ /** The flow's description, shown under the empty transcript's prompt. */ description?: string wideLayout?: boolean + /** What this surface's runs create: previews in the editor, deployed runs on the flow page. */ + conversationKind?: 'test' | 'deployed' } let { @@ -42,7 +44,8 @@ path, workspace = undefined, description = undefined, - wideLayout = false + wideLayout = false, + conversationKind = 'deployed' }: Props = $props() // Derive helperScript for dynamic inputs from schema @@ -150,10 +153,22 @@ { additionalInputs: () => (additionalInputsSchema ? { ...runInputs } : undefined), workspace: () => workspace, - sendDisabled: () => deploymentInProgress || !!modelGap + sendDisabled: () => deploymentInProgress || !!modelGap || !!wrongKindReason } ) setChatViewHost(chatHost) + + // A chat of the other kind can be read from here but not added to: the server refuses a + // preview run into a deployed conversation and the reverse, so the composer says why first. + const wrongKindReason = $derived.by(() => { + const { conversationId, conversations } = chatHost.state + const open = conversations.find((c) => c.id === conversationId) + if (open?.isTest === undefined || open.isTest === (conversationKind === 'test')) + return undefined + return open.isTest + ? 'This chat was run from the flow editor. Start a new chat to continue here.' + : 'This chat belongs to the deployed flow. Start a new chat to test.' + }) onDestroy(() => chatHost.dispose()) // What the Configure-inputs modal asks for: every flow input the composer does not @@ -287,8 +302,10 @@ {emptyHint} footerSettings={modalSchema || showModelButton ? footerSettings : undefined} placeholder="Send a message to run the flow" - disabled={deploymentInProgress || !!modelGap} - disabledMessage={deploymentInProgress ? 'Deployment in progress' : (modelGap ?? '')} + disabled={deploymentInProgress || !!modelGap || !!wrongKindReason} + disabledMessage={deploymentInProgress + ? 'Deployment in progress' + : (modelGap ?? wrongKindReason ?? '')} loadPastChat={() => {}} deletePastChat={() => {}} saveAndClear={() => {}} diff --git a/frontend/src/lib/components/flows/conversations/FlowConversationsSidebar.svelte b/frontend/src/lib/components/flows/conversations/FlowConversationsSidebar.svelte index 02c2581db1..49efbbc1fa 100644 --- a/frontend/src/lib/components/flows/conversations/FlowConversationsSidebar.svelte +++ b/frontend/src/lib/components/flows/conversations/FlowConversationsSidebar.svelte @@ -1,20 +1,46 @@ + +{#if on && !s3StorageConfigured} +

+ Without S3 storage on the workspace, memory is kept in the database, up to 100KB per memory. +

+{/if} +{#if legacyMessages} + +
+ + An earlier version of the editor saved these previous messages inside the memory setting, + and this agent still sends them. + + {#if historyOnStep} +
+ +
+ {/if} +
+ +{/if} +{#if legacyMemoryId} + +
+ + {historyOnStep + ? 'Fixed memory id generated when this flow was saved.' + : 'Fixed memory id saved with this agent.'} + Every run shares it unless the caller passes one. + +
+ {#if historyOnStep} + + {/if} + +
+
+ +{/if} +{#if legacyEquivalent} + +
+ + An earlier version of the editor saved this setting. It works the same as {on + ? 'On' + : 'Off'}. + +
+ +
+
+ +{/if} diff --git a/frontend/src/lib/components/flows/content/AgentResourceBar.svelte b/frontend/src/lib/components/flows/content/AgentResourceBar.svelte index fafe69ebb4..2e760bb88c 100644 --- a/frontend/src/lib/components/flows/content/AgentResourceBar.svelte +++ b/frontend/src/lib/components/flows/content/AgentResourceBar.svelte @@ -53,7 +53,9 @@ moduleId, opWorkspace = undefined, flowPath = '', - fromAgentEditor = false + fromAgentEditor = false, + chatInputEnabled = false, + linkedMemory = $bindable() }: { agent: string | undefined inputTransforms: Record @@ -71,6 +73,9 @@ // backend supports it, but only a flow can author it, and a second editor over a second draft // is the wrong way in. fromAgentEditor?: boolean + chatInputEnabled?: boolean + // The linked agent's memory once its config has loaded, for the step's history inputs. + linkedMemory?: { memory: unknown } | undefined } = $props() let ws = $derived(opWorkspace ?? $workspaceStore) @@ -211,6 +216,9 @@ let linkedInfo = $derived( loadedInfo?.ws === ws && loadedInfo?.path === agent ? loadedInfo : undefined ) + $effect(() => { + linkedMemory = linkedInfo ? { memory: linkedInfo.config?.memory } : undefined + }) let inheritedTools = $derived(linkedInfo?.tools ?? []) let brainParams = $derived(summarizeAgentBrain(linkedInfo?.config)) let providerPath = $derived(linkedInfo?.providerPath) @@ -312,6 +320,20 @@ // saved without a complete one fails on every linked run. Block saving when the provider is // computed/connected (only a static value can be captured into the resource) or when the static // value is incomplete (a fresh step defaults to empty resource/model, which is still static). + // A saved agent never carries a memory id, so saving would drop the id this step's runs still fall + // back to and leave them without memory. The author picks what replaces it first. In chat mode the + // conversation id always won, so there the id was never read. + let legacyMemorySaveError = $derived.by(() => { + const memory = inputTransforms?.memory as + | { type?: string; value?: { kind?: string; context_length?: number; memory_id?: string } } + | undefined + const value = memory?.type === 'static' ? memory.value : undefined + if (chatInputEnabled || value?.kind !== 'auto' || !value.memory_id || !value.context_length) { + return undefined + } + return "This step still uses a fixed memory id from an earlier version. In Managed memory, choose Keep as memory id or Use the run's memory id, then save it as an agent." + }) + let providerSaveError = $derived.by(() => { const t = inputTransforms?.provider as | { type?: string; value?: { resource?: string; model?: string } } @@ -343,8 +365,8 @@ // the success toast that would otherwise bury the explanation. async function persist(path: string, description?: string): Promise { const dropped = nonStaticBrainKeys(inputTransforms) - if (providerSaveError) { - throw new Error(providerSaveError) + if (providerSaveError ?? legacyMemorySaveError) { + throw new Error(providerSaveError ?? legacyMemorySaveError) } if (dropped.length > 0) { sendUserToast( @@ -355,6 +377,12 @@ // Tool inputs are saved verbatim: the agent carries its tools' default bindings (static, AI or // flow expressions) as authored. Host flows override per-step via tool_inputs, never here. const value = inputTransformsToAgentConfig(inputTransforms, tools) + // An id an older editor baked into this step names the flow's memory. The agent is shared by + // every step linking it, and each of those takes its memory id from its own run. + if (value.memory && typeof value.memory === 'object' && 'memory_id' in value.memory) { + const { memory_id: _, ...memory } = value.memory as Record + value.memory = memory + } // The editor stays live during the requests below, so remember what linking would discard: // every brain transform and the tools. Comparing the saved config instead would miss a // non-static brain edit, which the resource cannot hold yet linking still strips. @@ -691,9 +719,9 @@ size="sm" /> - {#if providerSaveError} + {#if providerSaveError ?? legacyMemorySaveError}

- {providerSaveError} + {providerSaveError ?? legacyMemorySaveError}

{/if}
@@ -701,7 +729,10 @@ + {/if} +
+ {#if editable && row.id !== ADMIN_ROLE} + removeRole(row.id)} /> + {/if} + + + + {/each} + + + {#if editable && unusedRoles.length > 0} +
+ +
+ Name + Login + + + + + {#if loading} + + Loading… + + {:else if roles.length === 0} + + + No data table role yet. Every job connects as + admin. + + + {/if} + {#each roles as role (role.id)} + + + {#if renaming?.id === role.id} +
+ + + (renaming = undefined)} /> +
+ {:else} +
+ {role.name} +
+ {/if} + + +
+ + run( + () => + SettingService.updateInstanceDatatableRole({ + id: role.id, + requestBody: { enabled: e.detail } + }), + e.detail ? `Enabled ${role.name}` : `Disabled ${role.name}` + )} + /> + {#if !role.enabled} + Cannot log in + {/if} +
+ + + remove(role)} /> + + + {/each} + + +
+ + +
+ + + + + {/if} + diff --git a/frontend/src/lib/components/workspaceSettings/DataTableSettings.svelte b/frontend/src/lib/components/workspaceSettings/DataTableSettings.svelte index bb97e434f6..867cc58d87 100644 --- a/frontend/src/lib/components/workspaceSettings/DataTableSettings.svelte +++ b/frontend/src/lib/components/workspaceSettings/DataTableSettings.svelte @@ -12,6 +12,10 @@ resource_type: 'postgresql' | 'instance' resource_path?: string | undefined } + /** Set on a fork's entry: it names the workspace whose data table governs this one, and + * owns no database of its own. Read-only here — only forking writes it, and the server + * carries it across a save rather than taking it from this form. */ + reference?: { workspace_id: string; datatable: string } }[] } @@ -24,7 +28,10 @@ s.dataTables.push({ id: randomUUID(), name, - ...rest + ...rest, + // A pointer entry owns no database. The row renders read-only in that case, so this + // placeholder is never shown or sent. + database: rest.database ?? { resource_type: 'instance' } }) } } @@ -38,6 +45,12 @@ const database = dataTable.database if (dataTable.name in s.datatables) throw 'Settings contain duplicate dataTable name: ' + dataTable.name + // A pointer owns no database, so it has nothing to validate and nothing to send: the + // server keeps the stored reference whatever this payload says. + if (dataTable.reference) { + s.datatables[dataTable.name] = {} + continue + } if (!database.resource_path) throw 'No resource selected for ' + dataTable.name if (database.resource_type === 'instance' && database.resource_path === 'windmill') throw dataTable.name + ' database cannot be called "windmill"' @@ -79,6 +92,7 @@ type GetSettingsResponse, type TestDataTableConnectionResponse } from '$lib/gen' + // `superadmin` gates the commented-out roles section at the bottom; restore it there. import { workspaceStore } from '$lib/stores' import { createAsyncConfirmationModal } from '../common/confirmationModal/asyncConfirmationModal.svelte' import ConfirmationModal from '../common/confirmationModal/ConfirmationModal.svelte' @@ -87,6 +101,10 @@ import { Popover } from '../meltComponents' import ExploreAssetButton from '../ExploreAssetButton.svelte' import DataTableMigrationsButton from './DataTableMigrationsButton.svelte' + // Both components are complete and reviewed; their call sites in this file are commented + // out until the ACL editor lands. Uncomment these with them. + // import DataTablePermissionsButton from './DataTablePermissionsButton.svelte' + // import DataTableRolesSection from './DataTableRolesSection.svelte' import { deepEqual } from 'fast-equals' import { clone } from '$lib/utils' import SettingsFooter from './SettingsFooter.svelte' @@ -208,12 +226,28 @@ const deleted_datatables = dataTableSettings.dataTables .filter((d) => !tempIds.has(d.id)) .map((d) => d.name) - await WorkspaceService.editDataTableConfig({ + const result = await WorkspaceService.editDataTableConfig({ workspace: $workspaceStore!, requestBody: { settings, renames, deleted_datatables } }) dataTableSettings = clone(tempSettings) - sendUserToast('Data table settings saved successfully') + // A delete can leave another workspace's data table governed by nothing. Swallowing + // that is what made it silent for the person who caused it. + const stranded = result?.stranded_references ?? [] + if (stranded.length > 0) { + sendUserToast( + `These data tables were governed by one you deleted and no longer resolve: ${stranded + .map((s) => `${s.workspace_id}/${s.datatable}`) + .join(', ')}. Their databases still exist; a superadmin can point them at another ` + + `workspace's data table.`, + 'warning', + [], + undefined, + 20000 + ) + } else { + sendUserToast('Data table settings saved successfully') + } } catch (e) { sendUserToast(e, true) console.error('Error saving data table settings', e) @@ -350,62 +384,85 @@ {#each tempSettings.dataTables as dataTable, dataTableIndex (dataTable.id)} - + {#if dataTable.reference} + {dataTable.name} + {:else} + + {/if} -
-
- {#if dataTable.database.resource_type === 'instance'} - - Use Windmill's PostgreSQL instance - - {/if} - dataTable.database.resource_type, + (resource_type) => { + dataTable.database = { + resource_type, + resource_path: + resource_type === 'instance' ? defaultInstanceDbName() : undefined + } } } - } - id="database-type-select" - class="w-28" - /> -
-
- {#if dataTable.database.resource_type !== 'instance'} - - {:else} - - {/if} +
+
+ {#if dataTable.database.resource_type !== 'instance'} + + {:else} + + {/if} +
- + {/if} @@ -415,6 +472,23 @@ datatable={dataTable.name} disabled={!!dirtyMap[dataTable.name]} /> + + Role + Privileges + On + + + + + {#each grantRows as grant (grantKey(grant))} + {@const revokeScope = revokeScopeOf(grant)} + {@const revocable = revocablePrivileges(grant, target)} + {@const blocked = blockingSources(grant, revocable)} + {@const uncovered = uncoveredCreators(grant, info.roles)} + + {grant.grantee} + {grant.privileges.join(', ')} + + {grantScopeLabel(grant)} + {#if blocked.length > 0} + + from {blocked.join(', ')} + + {/if} + {#if uncovered.length > 0} + + · not for what {uncovered.join(', ')} + {uncovered.length === 1 ? 'creates' : 'create'} + + {/if} + + + + {#if info.editable && revokeScope && revocable.length > 0 && blocked.length === 0 && info.roles.includes(grant.grantee) && grant.grantee !== ADMIN_ROLE} + + + {/if} + + +{/if} + + (pending = undefined)} +> +
+ {#if pendingCoversObjects} + + The same privileges on several objects read as one row, and are revoked together. + + {/if} + {#each pending?.warnings ?? [] as warning (warning)} + {warning} + {/each} + + Runs against {datatable} in a single transaction: + + 
{(pending?.statements ?? []).join(';\n')};
+
+ diff --git a/frontend/src/lib/components/datatableAcl/PgGrantBuilder.svelte b/frontend/src/lib/components/datatableAcl/PgGrantBuilder.svelte new file mode 100644 index 0000000000..5e5119abb6 --- /dev/null +++ b/frontend/src/lib/components/datatableAcl/PgGrantBuilder.svelte @@ -0,0 +1,95 @@ + + +
+
+ GRANT + ({ value: p, label: p }))} + placeholder="privileges" + {disabled} + size="sm" + class="min-w-48" + /> + ON + ({ value: r, label: r }))} + placeholder="role" + {disabled} + size="sm" + class="w-40" + /> + +
+ {#if statement} + 
{statement}
+ {/if} +
diff --git a/frontend/src/lib/components/datatableAcl/aclScopes.test.ts b/frontend/src/lib/components/datatableAcl/aclScopes.test.ts new file mode 100644 index 0000000000..7c01836e89 --- /dev/null +++ b/frontend/src/lib/components/datatableAcl/aclScopes.test.ts @@ -0,0 +1,178 @@ +import { describe, expect, it } from 'vitest' +import type { AclGrant } from '$lib/gen' +import { + blockingSources, + grantKey, + groupGrants, + revocablePrivileges, + revokeScopeOf, + uncoveredCreators +} from './aclScopes' + +const table = (name: string) => ({ name, kind: 'TABLE' }) +const by = (role: string, privileges: string[], reachable = true) => ({ + role, + privileges, + reachable +}) +const byAdmin = (grant: Omit): AclGrant => ({ + ...grant, + sources: [by('admin', grant.privileges)] +}) + +describe('grantKey', () => { + it('tells apart a table and a function of the same name', () => { + const row = (object: { name: string; kind: string; args?: string }) => ({ + grantee: 'analytics', + privileges: ['SELECT'], + objects: [object], + sources: [by('admin', ['SELECT'])] + }) + expect(grantKey(row(table('orders')))).not.toBe( + grantKey(row({ name: 'orders', kind: 'FUNCTION', args: '' })) + ) + }) +}) + +describe('groupGrants', () => { + // A row's revoke names every object in it, so a row must only hold what one revoke may take. + it('folds the same privileges on objects of one kind, and nothing else', () => { + const grants: AclGrant[] = [ + { grantee: 'analytics', privileges: ['SELECT'], object: table('orders') }, + { grantee: 'analytics', privileges: ['SELECT'], object: table('salaries') }, + { grantee: 'operator', privileges: ['SELECT'], object: table('orders') }, + { grantee: 'analytics', privileges: ['INSERT', 'SELECT'], object: table('events') }, + { grantee: 'analytics', privileges: ['SELECT'], object: { name: 's', kind: 'SEQUENCE' } }, + { grantee: 'analytics', privileges: ['SELECT'], future: 'TABLES' }, + { grantee: 'analytics', privileges: ['USAGE'] } + ].map(byAdmin) + const rows = groupGrants(grants) + expect(rows.map((r) => [r.grantee, r.privileges, r.objects, r.future])).toEqual([ + ['analytics', ['SELECT'], [table('orders'), table('salaries')], undefined], + ['operator', ['SELECT'], [table('orders')], undefined], + ['analytics', ['INSERT', 'SELECT'], [table('events')], undefined], + ['analytics', ['SELECT'], [{ name: 's', kind: 'SEQUENCE' }], undefined], + ['analytics', ['SELECT'], [], 'TABLES'], + ['analytics', ['USAGE'], [], undefined] + ]) + }) + + // A revoke takes the row back from every source, so the row must name them all, with what each + // gave. + it('keeps every source of the grants it folds', () => { + const grants: AclGrant[] = [ + { + grantee: 'analytics', + privileges: ['SELECT'], + object: table('orders'), + sources: [by('admin', ['SELECT'])] + }, + { + grantee: 'analytics', + privileges: ['SELECT'], + object: table('salaries'), + sources: [by('admin', ['SELECT']), by('operator', ['SELECT'])] + } + ] + expect(groupGrants(grants)[0].sources).toEqual([ + by('admin', ['SELECT']), + by('operator', ['SELECT']) + ]) + }) +}) + +describe('revoke of a row', () => { + const row = (future?: string) => ({ + grantee: 'analytics', + privileges: ['SELECT'], + objects: [], + future, + sources: [by('admin', ['SELECT'])] + }) + + it('takes back only what the editor may revoke on the database', () => { + const database = { ...row(), privileges: ['CONNECT', 'CREATE'] } + expect(revocablePrivileges(database, { kind: 'database' })).toEqual(['CREATE']) + expect(revocablePrivileges(database, { kind: 'schema', schema: 'public' })).toEqual([ + 'CONNECT', + 'CREATE' + ]) + // Set database-wide, so not one a schema's revoke could take back either. + const schemasLater = { ...row('SCHEMAS'), privileges: ['CREATE'] } + expect(revocablePrivileges(schemasLater, { kind: 'database' })).toEqual([]) + }) + + it('maps default privileges to their scope, and refuses the ones it has none for', () => { + expect(revokeScopeOf(row())).toBe('target') + expect(revokeScopeOf(row('TABLES'))).toBe('future_tables') + expect(revokeScopeOf(row('TYPES'))).toBeUndefined() + expect(revokeScopeOf({ ...row(), objects: [{ name: 'mood', kind: 'TYPE' }] })).toBeUndefined() + }) + + // Postgres takes a grant back only through its source: offering the revoke would promise what + // the plan then refuses. But only the sources of what is revoked count: the catalog's CONNECT + // on the database comes from its owner, out of reach, and must not hold back a CREATE the + // editor granted. + it('is held back only by a source out of reach for what it takes', () => { + const database = { + ...row(), + privileges: ['CONNECT', 'CREATE'], + sources: [by('postgres', ['CONNECT'], false), by('admin', ['CREATE'])] + } + const revocable = revocablePrivileges(database, { kind: 'database' }) + expect(blockingSources(database, revocable)).toEqual([]) + expect(blockingSources(database, ['CONNECT'])).toEqual(['postgres']) + const partly = { + ...row('TABLES'), + sources: [by('admin', ['SELECT']), by('postgres', ['SELECT'], false)] + } + expect(blockingSources(partly, ['SELECT'])).toEqual(['postgres']) + }) + + // Whether a grant can be taken back depends on its object, so a row folding several objects is + // only revocable if each of its grants is. + it('is held back by a source out of reach on any of the objects it folds', () => { + const grants: AclGrant[] = [ + { + grantee: 'analytics', + privileges: ['SELECT'], + object: table('orders'), + sources: [by('admin', ['SELECT'])] + }, + { + grantee: 'analytics', + privileges: ['SELECT'], + object: table('salaries'), + sources: [by('admin', ['SELECT'], false)] + } + ] + const [folded] = groupGrants(grants) + expect(folded.objects).toHaveLength(2) + expect(blockingSources(folded, ['SELECT'])).toEqual(['admin']) + // Folding reads the grants, never rewrites them. + expect(grants[0].sources[0].reachable).toBe(true) + }) +}) + +describe('uncoveredCreators', () => { + // A default privilege binds only the creating roles it was granted for: a role added since is + // left out until the grant is made again. + it('names the roles a created-later row leaves out', () => { + const future = { + grantee: 'analytics', + privileges: ['SELECT'], + objects: [], + future: 'TABLES', + sources: [by('admin', ['SELECT']), by('analytics', ['SELECT'])] + } + expect(uncoveredCreators(future, ['admin', 'analytics', 'late'])).toEqual(['late']) + expect(uncoveredCreators({ ...future, future: undefined }, ['late'])).toEqual([]) + // Set by a role outside the catalog, it was never meant to cover the catalog's roles. + expect( + uncoveredCreators({ ...future, sources: [by('postgres', ['SELECT'], false)] }, [ + 'admin', + 'late' + ]) + ).toEqual([]) + }) +}) diff --git a/frontend/src/lib/components/datatableAcl/aclScopes.ts b/frontend/src/lib/components/datatableAcl/aclScopes.ts new file mode 100644 index 0000000000..d5b8ad9d20 --- /dev/null +++ b/frontend/src/lib/components/datatableAcl/aclScopes.ts @@ -0,0 +1,211 @@ +import type { AclGrant, AclSource, AclTarget } from '$lib/gen' + +/** The role a data table connects as without roles — `custom_instance_user` in Postgres. */ +export const ADMIN_ROLE = 'admin' + +/** Privileges Postgres accepts per kind of object. Mirrors the whitelist the backend validates + * against — a privilege missing here just cannot be built. */ +/** `CREATE` on a database is the right to create schemas in it, and the only database privilege + * handed out here: `CONNECT` is managed with the instance's role catalog. */ +export const DATABASE_PRIVILEGES = ['CREATE'] +export const SCHEMA_PRIVILEGES = ['USAGE', 'CREATE'] +export const TABLE_PRIVILEGES = [ + 'SELECT', + 'INSERT', + 'UPDATE', + 'DELETE', + 'TRUNCATE', + 'REFERENCES', + 'TRIGGER' +] +/** Postgres 17 and later only, so it is offered from what the server reports. */ +export const MAINTAIN_PRIVILEGE = 'MAINTAIN' +export const SEQUENCE_PRIVILEGES = ['USAGE', 'SELECT', 'UPDATE'] +export const FUNCTION_PRIVILEGES = ['EXECUTE'] + +export type AclScope = + | 'target' + | 'all_tables' + | 'all_sequences' + | 'all_functions' + | 'future_tables' + | 'future_sequences' + | 'future_functions' + +export type AclTargetKind = AclTarget['kind'] + +/** The scopes a target can grant on, in the order the builder offers them. */ +export function scopesOf(kind: AclTargetKind): { value: AclScope; label: string }[] { + if (kind === 'database') return [{ value: 'target', label: 'the database itself' }] + if (kind === 'table') return [{ value: 'target', label: 'this table' }] + return [ + { value: 'target', label: 'the schema itself' }, + { value: 'all_tables', label: 'all tables in it' }, + { value: 'all_sequences', label: 'all sequences in it' }, + { value: 'all_functions', label: 'all functions in it' }, + { value: 'future_tables', label: 'tables created later' }, + { value: 'future_sequences', label: 'sequences created later' }, + { value: 'future_functions', label: 'functions created later' } + ] +} + +export function privilegesOf( + scope: AclScope, + kind: AclTargetKind, + supportsMaintain = false +): string[] { + const tablePrivileges = supportsMaintain + ? [...TABLE_PRIVILEGES, MAINTAIN_PRIVILEGE] + : TABLE_PRIVILEGES + switch (scope) { + case 'target': + if (kind === 'database') return DATABASE_PRIVILEGES + return kind === 'schema' ? SCHEMA_PRIVILEGES : tablePrivileges + case 'all_tables': + case 'future_tables': + return tablePrivileges + case 'all_sequences': + case 'future_sequences': + return SEQUENCE_PRIVILEGES + case 'all_functions': + case 'future_functions': + return FUNCTION_PRIVILEGES + } +} + +/** What a statement built at this scope reads as, for the builder's own preview. */ +export function scopeSql(scope: AclScope, target: AclTarget, dbname?: string): string { + if (target.kind === 'database') return `DATABASE ${dbname ?? ''}`.trim() + const schema = target.schema + switch (scope) { + case 'target': + return target.kind === 'schema' ? `SCHEMA ${schema}` : `TABLE ${schema}.${target.table}` + case 'all_tables': + return `ALL TABLES IN SCHEMA ${schema}` + case 'all_sequences': + return `ALL SEQUENCES IN SCHEMA ${schema}` + case 'all_functions': + return `ALL FUNCTIONS IN SCHEMA ${schema}` + case 'future_tables': + return `TABLES (default privileges in ${schema})` + case 'future_sequences': + return `SEQUENCES (default privileges in ${schema})` + case 'future_functions': + return `FUNCTIONS (default privileges in ${schema})` + } +} + +/** One row of the grants table: the same privileges on several objects read as one line, since + * granting them per object is what `ON ALL TABLES` does. */ +export type GroupedGrant = { + grantee: string + privileges: string[] + objects: NonNullable[] + future?: string + /** Every role the row's grants come from, each once, with what it gave. */ + sources: AclSource[] +} + +export function groupGrants(grants: AclGrant[]): GroupedGrant[] { + const rows: GroupedGrant[] = [] + for (const grant of grants) { + const existing = grant.object + ? rows.find( + (r) => + r.grantee === grant.grantee && + r.future === grant.future && + r.objects[0]?.kind === grant.object?.kind && + r.privileges.join() === grant.privileges.join() + ) + : undefined + if (existing) { + existing.objects.push(grant.object!) + for (const source of grant.sources) { + const known = existing.sources.find((s) => s.role === source.role) + // Whether a role's grant can be taken back depends on the object it is on, so a row + // holds a source as reachable only if it is on every object the row folds. + if (known) { + known.reachable &&= source.reachable + known.privileges = [...new Set([...known.privileges, ...source.privileges])].sort() + } else { + existing.sources.push({ ...source, privileges: [...source.privileges] }) + } + } + } else { + rows.push({ + grantee: grant.grantee, + privileges: grant.privileges, + objects: grant.object ? [grant.object] : [], + future: grant.future, + sources: grant.sources.map((s) => ({ ...s, privileges: [...s.privileges] })) + }) + } + } + return rows +} + +/** The roles that gave some of `privileges` and that this data table's connection cannot act for. + * Only they can take those grants back, so a revoke of `privileges` is not offered. */ +export function blockingSources(grant: GroupedGrant, privileges: string[]): string[] { + return grant.sources + .filter((s) => !s.reachable && s.privileges.some((p) => privileges.includes(p))) + .map((s) => s.role) +} + +/** Which of `roles` a "created later" row granted for some of them does not cover. A default + * privilege binds only the creating roles it was granted for, so what the others create stays out + * of it. A row none of `roles` set — the instance's own, say — was never meant to cover them, and + * names none. */ +export function uncoveredCreators(grant: GroupedGrant, roles: string[]): string[] { + if (!grant.future || !grant.sources.some((s) => roles.includes(s.role))) return [] + return roles.filter((r) => !grant.sources.some((s) => s.role === r)) +} + +/** A row's identity. Two rows may share a grantee and an object name — a table `orders` and a + * function `orders()` — so the kind and the privileges are part of it too. */ +export function grantKey(grant: GroupedGrant): string { + return [ + grant.grantee, + grant.future ?? '', + grant.privileges.join(','), + ...grant.objects.map((o) => `${o.kind}:${o.name}(${o.args ?? ''})`) + ].join('|') +} + +/** The scope a revoke of this row takes, or `undefined` when the builder cannot express it — + * Postgres also records privileges on types, present and default, which nothing here grants and + * the API has no scope for. */ +export function revokeScopeOf(grant: GroupedGrant): AclScope | undefined { + if (!grant.future) return grant.objects.some((o) => o.kind === 'TYPE') ? undefined : 'target' + const scope = `future_${grant.future.toLowerCase()}` + return (['future_tables', 'future_sequences', 'future_functions'] as const).find( + (s) => s === scope + ) +} + +/** The privileges of a row a revoke may take back. On the database that is `CREATE` alone: + * `CONNECT` belongs to the role catalog, which would grant it again, and `TEMPORARY` is not one + * the editor hands out — a row holding only those has nothing to revoke here. A database's rows + * "created later" are default privileges set database-wide, which nothing here revokes. */ +export function revocablePrivileges(grant: GroupedGrant, target: AclTarget): string[] { + if (target.kind === 'database' && grant.objects.length === 0) { + if (grant.future) return [] + return grant.privileges.filter((p) => DATABASE_PRIVILEGES.includes(p)) + } + return grant.privileges +} + +/** How a row reads back: what it covers, in one phrase. */ +export function grantScopeLabel(grant: GroupedGrant): string { + if (grant.future) return `${grant.future.toLowerCase()} created later` + if (grant.objects.length === 1) { + const object = grant.objects[0] + // A routine's arguments are part of what it is, so two of the same name would otherwise + // read as one row twice. + const args = object.args !== undefined ? `(${object.args})` : '' + return `${object.kind.toLowerCase()} ${object.name}${args}` + } + if (grant.objects.length > 1) + return `${grant.objects.length} ${grant.objects[0].kind.toLowerCase()}s` + return 'itself' +} diff --git a/frontend/src/lib/components/workspaceSettings/DataTablePermissionsButton.svelte b/frontend/src/lib/components/workspaceSettings/DataTablePermissionsButton.svelte index 17ba0edd09..c5c4171150 100644 --- a/frontend/src/lib/components/workspaceSettings/DataTablePermissionsButton.svelte +++ b/frontend/src/lib/components/workspaceSettings/DataTablePermissionsButton.svelte @@ -11,10 +11,14 @@ GroupService, UserService, WorkspaceService, + type AclTarget, + type DatatableAclInfo, type DatatablePermissions, type InstanceDatatableRole } from '$lib/gen' import { sendUserToast } from '$lib/toast' + import AclTargetPicker from '../datatableAcl/AclTargetPicker.svelte' + import PgAclEditor from '../datatableAcl/PgAclEditor.svelte' const ADMIN_ROLE = 'admin' @@ -49,11 +53,38 @@ const availableRoles: InstanceDatatableRole[] = $derived(info?.available_roles ?? []) const unusedRoles = $derived(availableRoles.filter((r) => !rows.some((row) => row.id === r.id))) + let aclSchema = $state(undefined) + let aclTable = $state(undefined) + const aclTarget: AclTarget = $derived( + aclSchema + ? aclTable + ? { kind: 'table', schema: aclSchema, table: aclTable } + : { kind: 'schema', schema: aclSchema } + : { kind: 'database' } + ) + let aclSchemas = $state([]) + let aclSchemasLoaded = $state(false) + let aclTables = $state([]) + + // The editor's read of a database lists its schemas, and of a schema its tables — which is what + // the picker offers, so the picker reads nothing of its own. A read for a target since left + // behind is dropped. + function onAclLoaded(target: AclTarget, loaded: DatatableAclInfo) { + if (JSON.stringify(target) !== JSON.stringify(aclTarget)) return + if (target.kind === 'database') { + aclSchemas = loaded.children + aclSchemasLoaded = true + } else if (target.kind === 'schema') aclTables = loaded.children + } + async function load() { loading = true loadError = undefined try { - const res = await WorkspaceService.getDatatablePermissions({ workspace, datatableName: datatable }) + const res = await WorkspaceService.getDatatablePermissions({ + workspace, + datatableName: datatable + }) info = res permissioned = res.permissioned defaultRole = res.default_role @@ -124,6 +155,11 @@ } export function open() { + aclSchema = undefined + aclTable = undefined + aclSchemas = [] + aclSchemasLoaded = false + aclTables = [] drawer?.openDrawer() load() } @@ -143,8 +179,11 @@ drawer?.closeDrawer()} - tooltip="A data table role is a Postgres login. A job that names one connects as it, and Postgres decides what it may touch — grant privileges with SQL. Roles are defined for the whole instance; here you say who may use each one on this data table." + tooltip="A data table role is a Postgres login. A job that names one connects as it, and Postgres decides what it may touch — grant it privileges under Access. Roles are defined for the whole instance; here you say who may use each one on this data table." > + {#snippet titleExtra()} + Beta + {/snippet} {#if loading}

Loading…

{:else if loadError} @@ -173,7 +212,7 @@ These data tables point at the same database with their own entry, so what you set here does not reach them:
    - {#each info.ungoverned_reachers as reacher} + {#each info.ungoverned_reachers as reacher (`${reacher.workspace_id}/${reacher.datatable}`)}
  • {reacher.workspace_id} / {reacher.datatable}
    • {/each}
@@ -194,7 +233,7 @@ {#if availableRoles.length === 0} Only admin can be used until a superadmin adds a data table - role in the data table settings page. + role, from Instance roles at the top of the data tables settings page. {/if} @@ -271,6 +310,34 @@ {/if} + + {#if info?.supported} +
+
+ Access + + What each role may do in Postgres, on the database, a schema or a table. Every + change shows the SQL it runs before running it. + +
+ aclSchema, + (s) => { + aclSchema = s + aclTables = [] + } + } + bind:table={aclTable} + /> + {#key JSON.stringify(aclTarget)} + + {/key} +
+ {/if} {/if} diff --git a/frontend/src/lib/components/workspaceSettings/DataTableRolesSection.svelte b/frontend/src/lib/components/workspaceSettings/DataTableRolesSection.svelte index e46b578597..d100b1681a 100644 --- a/frontend/src/lib/components/workspaceSettings/DataTableRolesSection.svelte +++ b/frontend/src/lib/components/workspaceSettings/DataTableRolesSection.svelte @@ -3,7 +3,6 @@ import CloseButton from '../common/CloseButton.svelte' import TextInput from '../text_input/TextInput.svelte' import Toggle from '../Toggle.svelte' - import Tooltip from '../Tooltip.svelte' import ConfirmationModal from '../common/confirmationModal/ConfirmationModal.svelte' import { createAsyncConfirmationModal } from '../common/confirmationModal/asyncConfirmationModal.svelte' import Cell from '../table/Cell.svelte' @@ -83,16 +82,6 @@
-
-

Instance roles

- - A data table role is a real Postgres login on this instance, shared by every instance - database. A job that names one connects as it, and Postgres decides what it may touch — grant - it privileges with SQL. Which people may use a role on a given data table is set per data - table, in its roles drawer. - -
- {#if loadError} {loadError} {:else} diff --git a/frontend/src/lib/components/workspaceSettings/DataTableSettings.svelte b/frontend/src/lib/components/workspaceSettings/DataTableSettings.svelte index 867cc58d87..53b4819689 100644 --- a/frontend/src/lib/components/workspaceSettings/DataTableSettings.svelte +++ b/frontend/src/lib/components/workspaceSettings/DataTableSettings.svelte @@ -92,8 +92,7 @@ type GetSettingsResponse, type TestDataTableConnectionResponse } from '$lib/gen' - // `superadmin` gates the commented-out roles section at the bottom; restore it there. - import { workspaceStore } from '$lib/stores' + import { enterpriseLicense, superadmin, workspaceStore } from '$lib/stores' import { createAsyncConfirmationModal } from '../common/confirmationModal/asyncConfirmationModal.svelte' import ConfirmationModal from '../common/confirmationModal/ConfirmationModal.svelte' import { resource } from 'runed' @@ -101,10 +100,8 @@ import { Popover } from '../meltComponents' import ExploreAssetButton from '../ExploreAssetButton.svelte' import DataTableMigrationsButton from './DataTableMigrationsButton.svelte' - // Both components are complete and reviewed; their call sites in this file are commented - // out until the ACL editor lands. Uncomment these with them. - // import DataTablePermissionsButton from './DataTablePermissionsButton.svelte' - // import DataTableRolesSection from './DataTableRolesSection.svelte' + import DataTablePermissionsButton from './DataTablePermissionsButton.svelte' + import InstanceRolesButton from './InstanceRolesButton.svelte' import { deepEqual } from 'fast-equals' import { clone } from '$lib/utils' import SettingsFooter from './SettingsFooter.svelte' @@ -318,7 +315,13 @@ title="Data tables" description="Relational storage the whole workspace shares under one name. Scripts, flows and apps address it as datatable://main instead of picking a PostgreSQL resource, so nobody needs access to the credentials to query it, and you can point that name at another database without touching a line of code. Browse and edit tables, and version schema changes as migrations, from here." link="https://www.windmill.dev/docs/core_concepts/persistent_storage/data_tables" -/> +> + {#snippet actions()} + {#if $superadmin && $enterpriseLicense && !isCloudHosted()} + + {/if} + {/snippet} + {#if isCloudHosted()} @@ -472,15 +475,6 @@ datatable={dataTable.name} disabled={!!dirtyMap[dataTable.name]} /> -
-{/if} ---> - + import { Badge, Button, Drawer, DrawerContent } from '../common' + import { Users } from 'lucide-svelte' + import DataTableRolesSection from './DataTableRolesSection.svelte' + + let drawer: Drawer | undefined = $state(undefined) + + + + + + drawer?.closeDrawer()} + tooltip="A data table role is a real Postgres login on this instance, shared by every instance database. A job that names one connects as it, and Postgres decides what it may touch. Which people may use a role on a given data table, and what it may do there, is set per data table, in its roles drawer." + > + {#snippet titleExtra()} + Beta + {/snippet} + + + From f0f1f7c186f82ba7787d640f13bf8ca7e90ba1f9 Mon Sep 17 00:00:00 2001 From: Diego Imbert Date: Wed, 16 Sep 2026 16:39:14 +0200 Subject: [PATCH 15/22] fix: take every pooled connection before the ACL apply locks Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01BRoYE5ZeAVvrDYdfhDAYXb --- .../src/datatable_acl.rs | 70 ++++++++++++------- 1 file changed, 44 insertions(+), 26 deletions(-) diff --git a/backend/windmill-api-workspaces/src/datatable_acl.rs b/backend/windmill-api-workspaces/src/datatable_acl.rs index 39fa8fac5d..84bc949dd5 100644 --- a/backend/windmill-api-workspaces/src/datatable_acl.rs +++ b/backend/windmill-api-workspaces/src/datatable_acl.rs @@ -37,7 +37,8 @@ use windmill_common::datatable_roles::{ }; use windmill_common::error::{pg_error_message, Error, JsonResult, Result}; use windmill_common::workspaces::{ - get_datatable_resource_from_db_unchecked, resolve_governing_datatable, GoverningDatatable, + get_datatable_resource_from_db_unchecked, resolve_governing_datatable, DataTable, + GoverningDatatable, }; use windmill_common::{PgDatabase, DB}; @@ -1307,6 +1308,21 @@ async fn authorize_acl_change( Ok(governing) } +/// Whether the governing entry an apply was authorized on is still the one in the settings, read +/// under the lock: a save in between could have pointed it at another database or changed its roles. +fn entry_unchanged(governing: &GoverningDatatable, entry_now: Option) -> bool { + let Some(Ok(now)) = entry_now.map(serde_json::from_value::) else { + return false; + }; + match ( + serde_json::to_value(&now), + serde_json::to_value(&governing.datatable), + ) { + (Ok(now), Ok(authorized)) => now == authorized, + _ => false, + } +} + /// Plan one change against the catalog and the database as they are now. async fn build_plan( client: &tokio_postgres::Client, @@ -1502,33 +1518,11 @@ async fn apply_datatable_acl( .to_string(), ) })?; - // Refuses without taking a lock; everything is checked again once they are held. + // Everything that needs the pool happens before the locks: once `tx` holds them, a second pool + // connection could wait forever on a pool that concurrent applies, queued on the same locks, + // have exhausted. let governing = authorize_acl_change(&db, &authed, &w_id, &datatable_name).await?; - - // Held until the change is committed: a role renamed or dropped meanwhile would change what - // the plan names, and a settings save could move the entry onto another database. Taken in the - // same order as the permissions save, so the two cannot deadlock. - let mut tx = db.begin().await?; - lock_role_catalog(&mut tx).await?; - sqlx::query!( - "SELECT 1 AS one FROM workspace_settings WHERE workspace_id = $1 FOR UPDATE", - &governing.workspace_id - ) - .fetch_optional(&mut *tx) - .await?; - let governing = authorize_acl_change(&db, &authed, &w_id, &datatable_name).await?; - let catalog = read_role_catalog_tx(&mut tx).await?; - let (mut client, mut notices, dbname) = connect_as_admin_unchecked(&db, &governing).await?; - let plan = build_plan(&client, &dbname, &catalog, &req.target, &req.change).await?; - if &plan.statements != confirmed { - return Err(Error::BadRequest( - "The data table or its roles changed since this was planned, so it would no longer \ - run what was confirmed. Plan it again." - .to_string(), - )); - } - // Postgres only lets a role pass on a privilege it holds with grant option, and an instance // database provisioned before data table roles holds none. Best-effort: a grant this fails to // enable is refused below rather than skipped. @@ -1537,6 +1531,30 @@ async fn apply_datatable_acl( tracing::warn!("Could not refresh grant options on '{dbname}': {e}"); } + // Held until the change is committed: a role renamed or dropped meanwhile would change what + // the plan names, and a settings save could move the entry onto another database. Taken in the + // same order as the permissions save, so the two cannot deadlock. + let mut tx = db.begin().await?; + lock_role_catalog(&mut tx).await?; + let entry_now = sqlx::query_scalar::<_, Option>( + "SELECT datatable->'datatables'->$2 FROM workspace_settings WHERE workspace_id = $1 FOR UPDATE", + ) + .bind(&governing.workspace_id) + .bind(&governing.name) + .fetch_optional(&mut *tx) + .await? + .flatten(); + let catalog = read_role_catalog_tx(&mut tx).await?; + + let plan = build_plan(&client, &dbname, &catalog, &req.target, &req.change).await?; + if !entry_unchanged(&governing, entry_now) || &plan.statements != confirmed { + return Err(Error::BadRequest( + "The data table or its roles changed since this was planned, so it would no longer \ + run what was confirmed. Plan it again." + .to_string(), + )); + } + // One transaction: a half-applied ownership transfer leaves one schema's objects owned by two // different roles. let pg_tx = client.transaction().await.map_err(|e| { From 45dd4cb271877ba963f809739ee9a34c7a55898c Mon Sep 17 00:00:00 2001 From: Diego Imbert Date: Wed, 16 Sep 2026 16:53:13 +0200 Subject: [PATCH 16/22] fix: refresh grant options only after the ACL apply validates its plan Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01BRoYE5ZeAVvrDYdfhDAYXb --- .../windmill-api-workspaces/src/datatable_acl.rs | 16 +++++++++------- 1 file changed, 9 insertions(+), 7 deletions(-) diff --git a/backend/windmill-api-workspaces/src/datatable_acl.rs b/backend/windmill-api-workspaces/src/datatable_acl.rs index 84bc949dd5..396a456e4e 100644 --- a/backend/windmill-api-workspaces/src/datatable_acl.rs +++ b/backend/windmill-api-workspaces/src/datatable_acl.rs @@ -1523,13 +1523,6 @@ async fn apply_datatable_acl( // have exhausted. let governing = authorize_acl_change(&db, &authed, &w_id, &datatable_name).await?; let (mut client, mut notices, dbname) = connect_as_admin_unchecked(&db, &governing).await?; - // Postgres only lets a role pass on a privilege it holds with grant option, and an instance - // database provisioned before data table roles holds none. Best-effort: a grant this fails to - // enable is refused below rather than skipped. - if let Err(e) = windmill_common::ensure_instance_db_grant_options_unchecked(&db, &dbname).await - { - tracing::warn!("Could not refresh grant options on '{dbname}': {e}"); - } // Held until the change is committed: a role renamed or dropped meanwhile would change what // the plan names, and a settings save could move the entry onto another database. Taken in the @@ -1555,6 +1548,15 @@ async fn apply_datatable_acl( )); } + // A role passes on only privileges it holds with grant option, which a database provisioned + // before data table roles lacks; a grant this fails to enable is refused below. After the plan + // check, since the planner reads these grants and would refuse what it just accepted; it + // connects as the server's own Postgres user, so it takes nothing from the pool. + if let Err(e) = windmill_common::ensure_instance_db_grant_options_unchecked(&db, &dbname).await + { + tracing::warn!("Could not refresh grant options on '{dbname}': {e}"); + } + // One transaction: a half-applied ownership transfer leaves one schema's objects owned by two // different roles. let pg_tx = client.transaction().await.map_err(|e| { From 9b9d16452499eadcb379ba94e18ea9663a4ba401 Mon Sep 17 00:00:00 2001 From: Diego Imbert Date: Wed, 16 Sep 2026 17:31:06 +0200 Subject: [PATCH 17/22] fix: add only missing grant options before an ACL apply, never default privileges Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01BRoYE5ZeAVvrDYdfhDAYXb --- .../src/datatable_acl.rs | 63 ++++++++++++++++--- 1 file changed, 54 insertions(+), 9 deletions(-) diff --git a/backend/windmill-api-workspaces/src/datatable_acl.rs b/backend/windmill-api-workspaces/src/datatable_acl.rs index 396a456e4e..6aa2f936b0 100644 --- a/backend/windmill-api-workspaces/src/datatable_acl.rs +++ b/backend/windmill-api-workspaces/src/datatable_acl.rs @@ -1308,6 +1308,59 @@ async fn authorize_acl_change( Ok(governing) } +/// A role passes on only privileges it holds with grant option, and an instance database +/// provisioned before data table roles gave `custom_instance_user` none. Adds that option to its +/// database and `public` privileges, and nothing else: default privileges are left alone, since a +/// schema's change of owner is planned against them. Best-effort, as a grant it fails to enable is +/// refused when it runs. +async fn ensure_grant_options(client: &tokio_postgres::Client, db: &DB, dbname: &str) { + let held = client + .query_one( + "SELECT has_database_privilege(current_database(), 'CONNECT WITH GRANT OPTION') + AND has_database_privilege(current_database(), 'CREATE WITH GRANT OPTION') + AND (to_regnamespace('public') IS NULL + OR (has_schema_privilege('public', 'USAGE WITH GRANT OPTION') + AND has_schema_privilege('public', 'CREATE WITH GRANT OPTION')))", + &[], + ) + .await + .is_ok_and(|row| row.get::<_, bool>(0)); + if held { + return; + } + if let Err(e) = grant_options_as_server(db, dbname).await { + tracing::warn!("Could not enable grant options on '{dbname}': {e}"); + } +} + +/// Only the database's owner, the server's own Postgres user, can hand out an option it holds. +async fn grant_options_as_server(db: &DB, dbname: &str) -> Result<()> { + let server = PgDatabase::parse_uri(&windmill_common::get_database_url().await?.as_str().await)?; + let creds = PgDatabase { dbname: dbname.to_string(), ..server }; + let (client, connection) = creds.connect(Some(db)).await?; + let join_handle = tokio::spawn(async move { connection.await }); + let role = quote_ident(CUSTOM_INSTANCE_USER); + let result = client + .batch_execute(&format!( + "GRANT CONNECT, CREATE ON DATABASE {} TO {role} WITH GRANT OPTION; + DO $$ BEGIN + IF to_regnamespace('public') IS NOT NULL THEN + GRANT USAGE, CREATE ON SCHEMA public TO {role} WITH GRANT OPTION; + END IF; + END $$;", + quote_ident(dbname) + )) + .await; + drop(client); + windmill_common::shutdown_pg_connection(join_handle).await?; + result.map_err(|e| { + Error::internal_err(format!( + "Failed to grant options on '{dbname}': {}", + pg_error_message(&e) + )) + }) +} + /// Whether the governing entry an apply was authorized on is still the one in the settings, read /// under the lock: a save in between could have pointed it at another database or changed its roles. fn entry_unchanged(governing: &GoverningDatatable, entry_now: Option) -> bool { @@ -1523,6 +1576,7 @@ async fn apply_datatable_acl( // have exhausted. let governing = authorize_acl_change(&db, &authed, &w_id, &datatable_name).await?; let (mut client, mut notices, dbname) = connect_as_admin_unchecked(&db, &governing).await?; + ensure_grant_options(&client, &db, &dbname).await; // Held until the change is committed: a role renamed or dropped meanwhile would change what // the plan names, and a settings save could move the entry onto another database. Taken in the @@ -1548,15 +1602,6 @@ async fn apply_datatable_acl( )); } - // A role passes on only privileges it holds with grant option, which a database provisioned - // before data table roles lacks; a grant this fails to enable is refused below. After the plan - // check, since the planner reads these grants and would refuse what it just accepted; it - // connects as the server's own Postgres user, so it takes nothing from the pool. - if let Err(e) = windmill_common::ensure_instance_db_grant_options_unchecked(&db, &dbname).await - { - tracing::warn!("Could not refresh grant options on '{dbname}': {e}"); - } - // One transaction: a half-applied ownership transfer leaves one schema's objects owned by two // different roles. let pg_tx = client.transaction().await.map_err(|e| { From d985f5d0ffb274e4b6d52e9a399ea37fa95ae3a5 Mon Sep 17 00:00:00 2001 From: Diego Imbert Date: Wed, 16 Sep 2026 22:25:51 +0200 Subject: [PATCH 18/22] fix: run one data table ACL apply at a time per server before it connects Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01BRoYE5ZeAVvrDYdfhDAYXb --- backend/windmill-api-workspaces/src/datatable_acl.rs | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/backend/windmill-api-workspaces/src/datatable_acl.rs b/backend/windmill-api-workspaces/src/datatable_acl.rs index 6aa2f936b0..8ec966d475 100644 --- a/backend/windmill-api-workspaces/src/datatable_acl.rs +++ b/backend/windmill-api-workspaces/src/datatable_acl.rs @@ -1308,6 +1308,8 @@ async fn authorize_acl_change( Ok(governing) } +static APPLY_SLOT: tokio::sync::Semaphore = tokio::sync::Semaphore::const_new(1); + /// A role passes on only privileges it holds with grant option, and an instance database /// provisioned before data table roles gave `custom_instance_user` none. Adds that option to its /// database and `public` privileges, and nothing else: default privileges are left alone, since a @@ -1575,6 +1577,13 @@ async fn apply_datatable_acl( // connection could wait forever on a pool that concurrent applies, queued on the same locks, // have exhausted. let governing = authorize_acl_change(&db, &authed, &w_id, &datatable_name).await?; + // Applies queue on an instance-wide lock while each holds a direct connection to the instance's + // Postgres; unbounded, the queue alone could exhaust its connection limit. One at a time per + // server, and the ones waiting hold no connection at all. + let _slot = APPLY_SLOT + .acquire() + .await + .map_err(|e| Error::internal_err(format!("ACL apply slot closed: {e}")))?; let (mut client, mut notices, dbname) = connect_as_admin_unchecked(&db, &governing).await?; ensure_grant_options(&client, &db, &dbname).await; From 0f99cce43cf12da3ef03c4be3b4a777df8e1edf7 Mon Sep 17 00:00:00 2001 From: Diego Imbert Date: Wed, 16 Sep 2026 23:55:43 +0200 Subject: [PATCH 19/22] fix: hold the ACL connection to the database that was authorized Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01BRoYE5ZeAVvrDYdfhDAYXb --- .../windmill-api-workspaces/src/datatable_acl.rs | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/backend/windmill-api-workspaces/src/datatable_acl.rs b/backend/windmill-api-workspaces/src/datatable_acl.rs index 8ec966d475..ce63555267 100644 --- a/backend/windmill-api-workspaces/src/datatable_acl.rs +++ b/backend/windmill-api-workspaces/src/datatable_acl.rs @@ -328,6 +328,22 @@ async fn connect_as_admin_unchecked( .await?; let pg: PgDatabase = serde_json::from_value(resource) .map_err(|e| Error::internal_err(format!("Failed to parse database credentials: {e}")))?; + // Resolving reads the settings again, and a save since `governing` was authorized can point + // the entry elsewhere and back. An instance entry's database is its `resource_path`, so the + // connection is held to the database that was authorized, and a later check of the entry + // cannot pass while this talks to another one. + if governing + .datatable + .database + .as_ref() + .map(|d| d.resource_path.as_str()) + != Some(&pg.dbname) + { + return Err(Error::BadRequest(format!( + "Data table '{}' was pointed at another database while this ran; try again", + governing.name + ))); + } let dbname = pg.dbname.clone(); let (client, mut connection) = pg.connect(Some(db)).await?; // Unbounded: the driver must never wait on the receiver, which only drains once the statement From de7be97124d0d1d3ef4bb81d077bc35c476c3873 Mon Sep 17 00:00:00 2001 From: Diego Imbert Date: Thu, 17 Sep 2026 00:09:29 +0200 Subject: [PATCH 20/22] fix: build the ACL connection from the authorized data table entry Resolving the settings again could land on a resource with the same database name on another server, which the later entry checks never see. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01BRoYE5ZeAVvrDYdfhDAYXb --- .../src/datatable_acl.rs | 34 +++++++------------ 1 file changed, 12 insertions(+), 22 deletions(-) diff --git a/backend/windmill-api-workspaces/src/datatable_acl.rs b/backend/windmill-api-workspaces/src/datatable_acl.rs index ce63555267..1dac05beb0 100644 --- a/backend/windmill-api-workspaces/src/datatable_acl.rs +++ b/backend/windmill-api-workspaces/src/datatable_acl.rs @@ -36,10 +36,7 @@ use windmill_common::datatable_roles::{ ADMIN_DATATABLE_ROLE, CUSTOM_INSTANCE_USER, }; use windmill_common::error::{pg_error_message, Error, JsonResult, Result}; -use windmill_common::workspaces::{ - get_datatable_resource_from_db_unchecked, resolve_governing_datatable, DataTable, - GoverningDatatable, -}; +use windmill_common::workspaces::{resolve_governing_datatable, DataTable, GoverningDatatable}; use windmill_common::{PgDatabase, DB}; use crate::datatable_permissions::{ensure_governs_datatable, ensure_reaches_datatable}; @@ -323,27 +320,20 @@ async fn connect_as_admin_unchecked( mpsc::UnboundedReceiver, String, )> { - let resource = - get_datatable_resource_from_db_unchecked(db, &governing.workspace_id, &governing.name) - .await?; - let pg: PgDatabase = serde_json::from_value(resource) - .map_err(|e| Error::internal_err(format!("Failed to parse database credentials: {e}")))?; - // Resolving reads the settings again, and a save since `governing` was authorized can point - // the entry elsewhere and back. An instance entry's database is its `resource_path`, so the - // connection is held to the database that was authorized, and a later check of the entry - // cannot pass while this talks to another one. - if governing + ensure_instance(governing)?; + // Built from the authorized entry, never by resolving the settings again: a save in between + // could point the entry at a resource on another server and back, and this connection would + // then alter a database the later checks of the entry never see. + let mut pg = PgDatabase::parse_uri(&windmill_common::get_database_url().await?.as_str().await)?; + pg.dbname = governing .datatable .database .as_ref() - .map(|d| d.resource_path.as_str()) - != Some(&pg.dbname) - { - return Err(Error::BadRequest(format!( - "Data table '{}' was pointed at another database while this ran; try again", - governing.name - ))); - } + .expect("a governing entry owns a database") + .resource_path + .clone(); + pg.user = Some(CUSTOM_INSTANCE_USER.to_string()); + pg.password = Some(windmill_common::utils::get_custom_pg_instance_password(db).await?); let dbname = pg.dbname.clone(); let (client, mut connection) = pg.connect(Some(db)).await?; // Unbounded: the driver must never wait on the receiver, which only drains once the statement From 908b98ef780e1e1b7b94a84c85c552cae3353171 Mon Sep 17 00:00:00 2001 From: Diego Imbert Date: Thu, 17 Sep 2026 09:00:04 +0200 Subject: [PATCH 21/22] fix: check ACL read reach against the entry it connects from Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01BRoYE5ZeAVvrDYdfhDAYXb --- backend/ee-repo-ref.txt | 2 +- .../src/datatable_acl.rs | 4 ++-- .../src/datatable_permissions.rs | 12 ++++++++++++ .../src/datatable_permissions_oss.rs | 18 ++++++++++++++++-- 4 files changed, 31 insertions(+), 5 deletions(-) diff --git a/backend/ee-repo-ref.txt b/backend/ee-repo-ref.txt index b878c8de1f..8cab265586 100644 --- a/backend/ee-repo-ref.txt +++ b/backend/ee-repo-ref.txt @@ -1 +1 @@ -6f26308c67acf9fcc45773b373aa30a2593b665c +0edd40979cf36bfba59323f3f6a0811ae1369cf5 diff --git a/backend/windmill-api-workspaces/src/datatable_acl.rs b/backend/windmill-api-workspaces/src/datatable_acl.rs index 1dac05beb0..6bd14316fc 100644 --- a/backend/windmill-api-workspaces/src/datatable_acl.rs +++ b/backend/windmill-api-workspaces/src/datatable_acl.rs @@ -39,7 +39,7 @@ use windmill_common::error::{pg_error_message, Error, JsonResult, Result}; use windmill_common::workspaces::{resolve_governing_datatable, DataTable, GoverningDatatable}; use windmill_common::{PgDatabase, DB}; -use crate::datatable_permissions::{ensure_governs_datatable, ensure_reaches_datatable}; +use crate::datatable_permissions::{ensure_governs_datatable, ensure_reaches_governing_datatable}; pub(crate) fn routes() -> Router { Router::new() @@ -1018,8 +1018,8 @@ async fn get_datatable_acl( ) -> JsonResult { crate::datatable_acl_oss::ensure_datatable_acl_available()?; let target: AclTarget = query.try_into()?; - ensure_reaches_datatable(&db, &w_id, &datatable_name, &authed).await?; let governing = resolve_governing_datatable(&db, &w_id, &datatable_name).await?; + ensure_reaches_governing_datatable(&db, &w_id, &datatable_name, &governing, &authed).await?; ensure_instance(&governing)?; let editable = ensure_governs_datatable(&db, &authed, &w_id, &governing) .await diff --git a/backend/windmill-api-workspaces/src/datatable_permissions.rs b/backend/windmill-api-workspaces/src/datatable_permissions.rs index 5cb1f3c1c3..69e04e328d 100644 --- a/backend/windmill-api-workspaces/src/datatable_permissions.rs +++ b/backend/windmill-api-workspaces/src/datatable_permissions.rs @@ -65,3 +65,15 @@ pub(crate) async fn ensure_reaches_datatable( ) -> Result<()> { roles::ensure_reaches_datatable(db, w_id, datatable_name, authed).await } + +/// [`ensure_reaches_datatable`] against an entry already resolved, for a caller that goes on to +/// connect from that same entry. +pub(crate) async fn ensure_reaches_governing_datatable( + db: &DB, + w_id: &str, + datatable_name: &str, + governing: &GoverningDatatable, + authed: &ApiAuthed, +) -> Result<()> { + roles::ensure_reaches_governing_datatable(db, w_id, datatable_name, governing, authed).await +} diff --git a/backend/windmill-api-workspaces/src/datatable_permissions_oss.rs b/backend/windmill-api-workspaces/src/datatable_permissions_oss.rs index f4d8c6a7ad..c7c7314885 100644 --- a/backend/windmill-api-workspaces/src/datatable_permissions_oss.rs +++ b/backend/windmill-api-workspaces/src/datatable_permissions_oss.rs @@ -12,8 +12,8 @@ #[cfg(all(feature = "private", feature = "enterprise"))] pub(crate) use crate::datatable_permissions_ee::{ - ensure_governs_datatable, ensure_reaches_datatable, get_datatable_permissions, - list_usable_datatable_roles, set_datatable_permissions, + ensure_governs_datatable, ensure_reaches_datatable, ensure_reaches_governing_datatable, + get_datatable_permissions, list_usable_datatable_roles, set_datatable_permissions, }; #[cfg(not(all(feature = "private", feature = "enterprise")))] @@ -56,6 +56,20 @@ mod ce { } } + pub(crate) async fn ensure_reaches_governing_datatable( + _db: &DB, + _w_id: &str, + _datatable_name: &str, + governing: &GoverningDatatable, + _authed: &ApiAuthed, + ) -> Result<()> { + if governing.datatable.permissions.is_none() { + Ok(()) + } else { + Err(unavailable()) + } + } + // The routes stay registered so the API has one shape; each answers after authentication, // before anything is read. From 64f95bcb7b97d4041bbae38aaa214a342940f0fe Mon Sep 17 00:00:00 2001 From: "windmill-internal-app[bot]" Date: Fri, 18 Sep 2026 11:54:29 +0000 Subject: [PATCH 22/22] chore: update ee-repo-ref to 7e338e4dabf91689bfd7fb0333c6534040b17b59 This commit updates the EE repository reference after PR #787 was merged in windmill-ee-private. Previous ee-repo-ref: 0edd40979cf36bfba59323f3f6a0811ae1369cf5 New ee-repo-ref: 7e338e4dabf91689bfd7fb0333c6534040b17b59 Automated by sync-ee-ref workflow. --- backend/ee-repo-ref.txt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/backend/ee-repo-ref.txt b/backend/ee-repo-ref.txt index 8cab265586..cc50737a1d 100644 --- a/backend/ee-repo-ref.txt +++ b/backend/ee-repo-ref.txt @@ -1 +1 @@ -0edd40979cf36bfba59323f3f6a0811ae1369cf5 +7e338e4dabf91689bfd7fb0333c6534040b17b59