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 1/4] 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 2/4] 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 3/4] 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 @@