sadpandajoe commented on code in PR #45392: URL: https://github.com/apache/superset/pull/45392#discussion_r4318686163
########## superset-frontend/src/core/clientTools/sqlLab/index.ts: ########## @@ -0,0 +1,239 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +import type { chat, sqlLab as sqlLabApi } from '@apache-superset/core'; +import { navigation } from 'src/core/navigation'; +import { sqlLab } from 'src/core/sqlLab'; + +const emptySchema = { + type: 'object', + properties: {}, + additionalProperties: false, +}; +const editorIdSchema = { + type: 'object', + properties: { editor_id: { type: 'string', minLength: 1 } }, + required: ['editor_id'], + additionalProperties: false, +}; + +/** Validates inputs even when a consumer invokes a handler without schema validation. */ +function inputObject(input: unknown): Record<string, unknown> { + if (!input || typeof input !== 'object' || Array.isArray(input)) { + throw new Error('Expected an object input'); + } + return input as Record<string, unknown>; +} + +/** Reads a required string, allowing an empty SQL buffer. */ +function stringInput(input: Record<string, unknown>, key: string): string { + const value = input[key]; + if (typeof value !== 'string' || (key !== 'sql' && !value.trim())) { + throw new Error(`Expected a string for "${key}"`); + } + return value; +} + +/** Resolves the active tab at invocation time. */ +function currentTab(): sqlLabApi.Tab { + const tab = sqlLab.getCurrentTab(); + if (!tab) throw new Error('No active SQL editor tab'); + return tab; +} + +/** Resolves a target before APIs that silently ignore missing tab IDs are called. */ +function targetTab(input: unknown): sqlLabApi.Tab { + const editorId = stringInput(inputObject(input), 'editor_id'); + const tab = sqlLab.getTabs().find(item => item.id === editorId); + if (!tab) throw new Error(`SQL editor "${editorId}" not found`); + return tab; +} + +/** Serializes tab metadata without waiting for inactive editors to mount. */ +function tabMetadata(tab: sqlLabApi.Tab) { + return { + editor_id: tab.id, + title: tab.title, + databaseId: tab.databaseId ?? null, + catalog: tab.catalog, + schema: tab.schema, + }; +} + +/** SQL Lab tools backed exclusively by the existing frontend API. */ +const sqlLabTools: chat.ClientTool[] = [ + { + name: 'sqlLab__get_current_editor_id', + description: 'Get the ID of the active SQL Lab editor tab.', + inputSchema: emptySchema, + annotations: { readOnlyHint: true }, + handler: () => ({ success: true, editor_id: currentTab().id }), + }, + { + name: 'sqlLab__get_editor_tabs', + description: + 'List open SQL Lab tabs and their database, catalog, and schema.', + inputSchema: emptySchema, + annotations: { readOnlyHint: true }, + handler: () => ({ + success: true, + editors: sqlLab.getTabs().map(tabMetadata), + }), + }, + { + name: 'sqlLab__get_databases', + description: 'List database IDs and names already loaded in SQL Lab.', + inputSchema: emptySchema, + annotations: { readOnlyHint: true }, + handler: () => ({ + success: true, + databases: sqlLab.getDatabases().map(({ id, name }) => ({ id, name })), + }), + }, + { + name: 'sqlLab__get_editor_sql', + description: + 'Read SQL, selection, and metadata from the SQL Lab editor specified by editor_id.', + inputSchema: editorIdSchema, + annotations: { readOnlyHint: true }, + handler: async input => { + const tab = targetTab(input); + const editor = await tab.getEditor(); Review Comment: `get_editor_sql` hangs for a tab that was loaded earlier but is no longer the active one. `SqlEditor` mounts `EditorWrapper` only while `isActive`, so switching away from tab A unregisters its handle while A stays `loaded: true`. A later `get_editor_sql({ editor_id: A.id })` passes the new loaded check, then `getEditor()` waits for a handle that cannot register until A is activated again. The tool call never settles, so the agent gets no result or error. The test mock at `sqlLab/index.test.ts:121` resolves `getEditor` immediately, so it doesn't catch this. Should the handler return a "switch to this tab first" error for a loaded tab with no registered handle, or read the stored SQL without waiting on the editor? ########## superset-frontend/packages/superset-core/src/chat/index.ts: ########## @@ -151,6 +172,162 @@ export declare const onDidChangeDisplayMode: Event<DisplayMode>; */ export declare const onDidResizePanel: Event<{ width: number }>; -// TODO: client actions API — tool availability functions will be added here -// once the client_actions SIP is finalized. The chat namespace is the -// intended integration point between the two SIPs. +/** + * A client-side (frontend) tool the chat agent can call — see the "Client + * Tools" SIP. Unlike a backend/MCP-server tool, its handler runs in the + * browser and can read/mutate whatever is currently on screen (e.g. the + * Dashboard v2 canvas), so it works entirely off local state with no network + * round trip of its own. + * + * Contributed either by the host itself (built-in "core" tools) or by an + * extension, via {@link registerClientTool} called directly from its own + * module — same as {@link registerChat}/`commands.registerCommand`. + */ +export interface ClientTool { + /** + * Name WITHOUT your extension's own prefix, e.g. `dashboard__do_thing` — + * when {@link registerClientTool}/{@link registerClientTools} is called + * from your extension's own module (the normal case: `import { chat } + * from '@apache-superset/core'` inside your extension), the host + * automatically qualifies it with your extension id, so it's registered + * (and addressed in a tool call) as `<your-extension-id>.dashboard__do_thing`. + * This only applies to calls made through that per-extension binding — + * host-internal code (this codebase's own "core" tools) isn't + * extension-scoped, so it manages its own prefix explicitly instead (see + * {@link registerClientTool}'s own docs). + */ + name: string; + /** Describes what the tool does, so the LLM agent knows when to call it. */ + description: string; + /** JSON Schema for the tool's input, e.g. `{ type: 'object', properties: {} }`. */ + inputSchema: Record<string, unknown>; + /** Invoked with the model's tool-call arguments; return value is reported back as the tool result. */ + handler: (input: unknown) => Promise<unknown> | unknown; + /** + * Optional behavior hints for this tool. See {@link ClientToolAnnotations}. + * Not required, and nothing in this namespace enforces or acts on them + * today — the host doesn't yet gate execution (e.g. a confirmation prompt + * before a destructive call) on either hint. Setting them now means a + * future consumer that does can rely on tools already carrying this + * metadata, rather than that being a breaking change to `ClientTool` + * itself. + */ + annotations?: ClientToolAnnotations; +} + +/** + * Behavior hints for a {@link ClientTool}, named to match the backend MCP + * tools' own `readOnlyHint`/`destructiveHint` (`superset/mcp_service/*`'s + * `@tool(..., annotations=ToolAnnotations(readOnlyHint=..., destructiveHint=...))`, + * from the official `mcp.types.ToolAnnotations`) — so a tool that exists in + * both a client and a backend form can describe itself the same way in + * either one. Like their backend counterparts, these are hints an agent or + * host UI may use to decide things like whether to ask for confirmation + * before calling the tool — they are not enforced by this namespace itself. + */ +export interface ClientToolAnnotations { + /** + * True if the tool only reads state and never modifies anything visible + * on screen or persisted — e.g. reading the active dashboard's id. + * Defaults to `false` (i.e. assume a tool may write) when omitted, same + * as the backend's `ToolAnnotations.readOnlyHint`. + */ + readOnlyHint?: boolean; + /** + * True if calling the tool may destructively change state (data loss, + * irreversible UI changes) — only meaningful when {@link readOnlyHint} + * is falsy. Defaults to `true` (i.e. assume a non-read-only tool may be + * destructive) when omitted, same as the backend's + * `ToolAnnotations.destructiveHint`. + */ + destructiveHint?: boolean; +} + +/** + * Registers a single client-side tool the chat agent can call — mirrors + * `commands.registerCommand`: a direct, imperative call an extension makes + * from its own module (typically its `./index` entry), not a declarative + * `extension.json` pointer the host resolves for you. Called from your + * extension's own module, your extension id is automatically prepended to + * `tool.name` (see {@link ClientTool.name}'s own docs) — write just + * `dashboard__do_thing`, not `my-extension.dashboard__do_thing`. Registering + * a second tool under the same (already-qualified) name overwrites the + * first (logged), and disposing the returned Disposable unregisters it. + * + * @example + * ```typescript + * import { chat } from '@apache-superset/core'; + * + * chat.registerClientTool({ + * name: 'dashboard__do_thing', + * description: '...', + * inputSchema: { type: 'object', properties: {} }, + * handler: () => ({ success: true }), + * }); + * ``` + */ +export declare function registerClientTool(tool: ClientTool): Disposable; + +/** + * Registers a list of client-side tools in one call — equivalent to calling + * {@link registerClientTool} once per entry (including its automatic + * extension-id prefixing), but without writing that loop yourself. Disposing + * the returned Disposable unregisters every tool in the list. + * + * @example + * ```typescript + * import { chat } from '@apache-superset/core'; + * + * chat.registerClientTools(getMyTools(chat)); + * ``` + */ +export declare function registerClientTools(tools: ClientTool[]): Disposable; + +/** + * Returns every client-side tool currently available — the host's own + * built-in ("core") tools plus every loaded extension's + * {@link registerClientTool} calls. Call this once tools are needed (e.g. + * when starting a chat turn) rather than caching the result, since it can + * grow as extensions finish loading. + * + * Each entry keeps its `handler`, so this is what a tool-call dispatcher + * should look tools up from by name. + * + * @example + * ```typescript + * import { chat } from '@apache-superset/core'; + * + * const tools = chat.getTools(); // for dispatching a tool call by name + * ``` + */ +export declare function getTools(): ClientTool[]; + +/** + * Authoring convenience for building a list of tools to hand to + * {@link registerClientTools} in one call — `typeof import('.')` here means + * "the `chat` namespace itself", the same object an extension gets from + * `import { chat } from '@apache-superset/core'`, so implementing against + * this type is enough to get the parameter right without hand-writing + * `(chat: typeof chatApi) => ClientTool[]` again in every extension. Not + * required — {@link registerClientTools} takes a plain array and doesn't + * care how the caller assembled it — but grouping a surface's tools behind + * one factory function (as this codebase's own "core" tools do) keeps a + * growing list organized. + * + * @example + * ```typescript + * import type { chat } from '@apache-superset/core'; Review Comment: The `ClientToolsFactory` example uses `import type { chat }` but then calls `chat.registerClientTools(getMyTools(chat))` as a runtime value. Copying it into an extension fails to compile (a type-only import can't be used as a value). ```suggestion * import { chat } from '@apache-superset/core'; ``` ########## docs/developer_docs/extensions/extension-points/chat.md: ########## @@ -143,6 +143,58 @@ All methods are available on the `chat` namespace from `@apache-superset/core`: | `onDidUnregisterChat(listener)` | Subscribe to unregistration events. | | `onDidResizePanel(listener)` | Subscribe to panel resize events (panel mode only). Not all hosts provide a resizer — do not rely on this firing. Returns a `Disposable`. | +## Built-in SQL Lab Tools + +Use `chat.getTools()` to discover these actions alongside other registered client tools. + +Superset registers these tools at application startup, before loading extensions. +They remain discoverable on every page; invoking them outside SQL Lab returns +`{ success: false, message }`. Open SQL Lab before calling them. SQL Lab handlers +also return this error shape for invalid input, missing tabs, and errors raised +while handling a tool call. Query execution failures are reported separately. + +| Tool | Input | Result / behavior | +| --- | --- | --- | +| `core.sqlLab__get_current_editor_id` | `{}` | Active `editor_id` | +| `core.sqlLab__get_editor_tabs` | `{}` | `editors`: IDs, titles, database IDs, catalogs, schemas | +| `core.sqlLab__get_databases` | `{}` | `databases`: IDs and names already loaded in SQL Lab | +| `core.sqlLab__get_editor_sql` | `{ editor_id: string }` | Target `editor`: metadata, SQL, and selected text; fails if the tab state is not loaded | +| `core.sqlLab__update_editor_sql` | `{ sql: string }` | Replace active editor SQL; empty SQL clears the buffer | +| `core.sqlLab__create_editor_tab` | `{ title: string, databaseId: number, sql?: string, catalog?: string, schema?: string }` | Open a tab and return its metadata; omitted options use SQL Lab defaults | +| `core.sqlLab__switch_editor_tab` | `{ editor_id: string }` | Activate an existing tab | +| `core.sqlLab__close_editor_tab` | `{ editor_id: string }` | Close a tab and discard its editor buffer | +| `core.sqlLab__run_current_query` | `{}` | Trigger the existing SQL Lab Run action; return `editor_id` and client `query_id` | + +Successful results include `success: true`. SQL reads target the supplied +`editor_id`; SQL updates use the active editor, so switch tabs before updating +another editor. Listing tabs +reads metadata only, so it does not wait for inactive editors to mount. +Persisted tabs remain listed before they are visited; their `databaseId` is +`null` until their state is loaded. SQL reads and updates return an error if +their tab closes while waiting for its editor to mount. Creating a tab for a +different database clears omitted catalog and schema values; explicit values +are preserved, and tabs using the same database inherit omitted values. + +`get_editor_sql` requires an `editor_id` and reads that tab rather than the active +tab. If the tab's state has not loaded, `Tab.getEditor()` rejects, and the tool +returns `success: false` with a message to use +`switch_editor_tab` to load the target tab before retrying. + +`run_current_query` follows SQL Lab's existing selection, database, catalog, +schema, template parameter, and row limit behavior. It starts execution through +the SQL Lab API and returns a client query ID; it does not wait for results. +If a persisted tab is still loading after a switch, execution returns an error; +retry once the tab has loaded. +Its `success: true` confirms only that execution was initiated, not that the +query succeeded. Backend execution errors (including invalid SQL) are delivered +through SQL Lab's query lifecycle. Subscribe to `sqlLab.onDidQuerySuccess` and Review Comment: This tells callers to subscribe to `onDidQuerySuccess`/`onDidQueryFail` before starting execution, but those subscriptions are bound to whichever tab is active when they are created (`predicate` captures the active editor's immutable ID). A panel that subscribed while tab A was active and then calls `switch_editor_tab(B)` followed by `run_current_query` will never receive B's success or failure event, even when `clientId` matches the returned `query_id`. Could this say to (re)subscribe after switching to the target tab, so a panel-lifetime subscription isn't assumed to be enough? -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
