diff --git a/package-lock.json b/package-lock.json index da00d3c82..a873f3b91 100644 --- a/package-lock.json +++ b/package-lock.json @@ -1,12 +1,12 @@ { "name": "@tableau/mcp-server", - "version": "2.63.8", + "version": "2.63.9", "lockfileVersion": 3, "requires": true, "packages": { "": { "name": "@tableau/mcp-server", - "version": "2.63.8", + "version": "2.63.9", "license": "Apache-2.0", "dependencies": { "@modelcontextprotocol/ext-apps": "^1.7.2", diff --git a/package.json b/package.json index c6f3ee182..de935ddb7 100644 --- a/package.json +++ b/package.json @@ -1,7 +1,7 @@ { "name": "@tableau/mcp-server", "description": "Helping agents see and understand data.", - "version": "2.63.8", + "version": "2.63.9", "repository": { "type": "git", "url": "git+https://github.com/tableau/tableau-mcp.git" diff --git a/src/desktop/metadata/dashboards.test.ts b/src/desktop/metadata/dashboards.test.ts index 0ce4a6736..46f7c62ad 100644 --- a/src/desktop/metadata/dashboards.test.ts +++ b/src/desktop/metadata/dashboards.test.ts @@ -24,6 +24,16 @@ const WORKBOOK_WITH_USER_NAMESPACE = ` `; +// A story serializes as a `` sharing the container with a +// real dashboard; the dashboard resolvers must ignore the story. +const WORKBOOK_WITH_STORYBOARD = ` + + + + + +`; + describe('extractDashboardXml', () => { it('finds and extracts an existing dashboard', () => { const xml = extractDashboardXml(WORKBOOK_WITH_USER_NAMESPACE, 'Overview'); @@ -36,6 +46,12 @@ describe('extractDashboardXml', () => { expect(extractDashboardXml(WORKBOOK_WITH_USER_NAMESPACE, 'Does Not Exist')).toBeNull(); }); + it('returns null for a storyboard — a story is not a dashboard', () => { + expect(extractDashboardXml(WORKBOOK_WITH_STORYBOARD, 'QBR Story')).toBeNull(); + // The real dashboard sharing the container still extracts. + expect(extractDashboardXml(WORKBOOK_WITH_STORYBOARD, 'Overview')).toContain('name="Overview"'); + }); + // Same live-bug shape as extractSheetXml (sheets.test.ts): an untouched get-dashboard-xml -> // apply-dashboard round-trip must pass the same well-formed-xml preflight apply-dashboard runs. it('carries the xmlns:user declaration from the workbook root onto the extracted dashboard', () => { @@ -159,6 +175,19 @@ describe('listDashboardRefs / resolveDashboardRef', () => { it('returns null when neither an id nor a name matches', () => { expect(resolveDashboardRef(WORKBOOK, 'No Such Dashboard')).toBeNull(); }); + + it('excludes storyboards: a story is neither listed nor resolvable as a dashboard', () => { + expect(listDashboardRefs(WORKBOOK_WITH_STORYBOARD)).toEqual([ + { id: '{DB-0001}', name: 'Overview' }, + ]); + expect(resolveDashboardRef(WORKBOOK_WITH_STORYBOARD, 'QBR Story')).toBeNull(); + expect(resolveDashboardRef(WORKBOOK_WITH_STORYBOARD, '{ST-0001}')).toBeNull(); + // The real dashboard sharing the container still resolves. + expect(resolveDashboardRef(WORKBOOK_WITH_STORYBOARD, 'Overview')).toEqual({ + id: '{DB-0001}', + name: 'Overview', + }); + }); }); describe('deleteDashboard', () => { diff --git a/src/desktop/metadata/dashboards.ts b/src/desktop/metadata/dashboards.ts index 2ce629e34..0c4d7c870 100644 --- a/src/desktop/metadata/dashboards.ts +++ b/src/desktop/metadata/dashboards.ts @@ -180,12 +180,19 @@ export function dashboardFragmentSimpleId(dashboardFragmentXml: string): string return dashboard?.['simple-id']?.['@_uuid']?.trim() || null; } +// A story serializes as a `` inside the same `` container as +// ordinary dashboards, so dashboard enumeration/resolution must skip it or a story leaks in as a dashboard. +function isStoryboard(dashboard: ParsedDashboard): boolean { + return dashboard['@_type'] === 'storyboard'; +} + // `id` is each dashboard's own `` — the same value the External Client API returns // as the dashboard id. export function listDashboardRefs(workbookXml: string): Array<{ id: string; name: string }> { const workbook = parseXML(workbookXml); const dashboards = normalizeArray(workbook.workbook?.dashboards?.dashboard); return dashboards.flatMap((db) => { + if (isStoryboard(db)) return []; const id = db['simple-id']?.['@_uuid']?.trim(); const name = db['@_name']; return id && name ? [{ id, name }] : []; @@ -201,7 +208,9 @@ export function resolveDashboardRef( ref: string, ): { id?: string; name: string } | null { const workbook = parseXML(workbookXml); - const dashboards = normalizeArray(workbook.workbook?.dashboards?.dashboard); + const dashboards = normalizeArray(workbook.workbook?.dashboards?.dashboard).filter( + (db) => !isStoryboard(db), + ); const trimmed = ref.trim(); const matched = dashboards.find((db) => db['simple-id']?.['@_uuid']?.trim() === trimmed) ?? @@ -216,7 +225,7 @@ export function resolveDashboardRef( export function extractDashboardXml(workbookXml: string, dashboardName: string): string | null { const workbook = parseXML(workbookXml); const dashboard = findDashboard(workbook, dashboardName); - if (!dashboard) { + if (!dashboard || isStoryboard(dashboard)) { return null; } carryNamespaceDeclarations(workbook.workbook, dashboard); diff --git a/src/desktop/wrappers/loadDashboardXml.test.ts b/src/desktop/wrappers/loadDashboardXml.test.ts index 7c51bf944..05524ea26 100644 --- a/src/desktop/wrappers/loadDashboardXml.test.ts +++ b/src/desktop/wrappers/loadDashboardXml.test.ts @@ -357,6 +357,28 @@ describe('loadDashboardXml (External Client API transport)', () => { expect(calls.find((c) => c.kind === 'apply')).toBeUndefined(); }); + it('names storyboardName (not dashboard_name) when the caller name disagrees with the XML', async () => { + const executor = makeExecutorMock({}); + + const result = await loadDashboardXml({ + dashboardName: 'Wrong Name', + xml: "", + executor, + signal: mockSignal, + focus: NO_FOCUS, + kind: 'storyboard', + requireExistingSheet: true, + }); + + expect(result.isErr()).toBe(true); + if (result.isErr()) { + invariant(result.error.type === 'load-dashboard-xml-error'); + invariant(result.error.error.type === 'name-mismatch'); + expect(result.error.error.message).toContain('storyboardName'); + expect(result.error.error.message).not.toContain('dashboard_name'); + } + }); + it('goes straight to the whole-workbook apply for an absent dashboard when requireExistingSheet is off', async () => { const { executor, calls } = absentDashboardExecutor(['Some Other DB']); diff --git a/src/desktop/wrappers/loadDashboardXml.ts b/src/desktop/wrappers/loadDashboardXml.ts index 18c4bb2eb..8bd5c74aa 100644 --- a/src/desktop/wrappers/loadDashboardXml.ts +++ b/src/desktop/wrappers/loadDashboardXml.ts @@ -65,8 +65,12 @@ type LoadDashboardHelperResult = Result< function resolveCanonicalDashboardName( dashboardName: string, xml: string, + kind: LoadDashboardKind, ): Result> { const callerRef = dashboardName.trim(); + // A storyboard is a `` element too, so the fragment tag and read-cached-xml selector + // stay `dashboard`; only the artifact noun and the tool's name param differ by kind. + const paramName = kind === 'storyboard' ? 'storyboardName' : 'dashboard_name'; let xmlName = ''; let xmlId = ''; let isWorkbookDocument = false; @@ -86,7 +90,7 @@ function resolveCanonicalDashboardName( return Err({ type: 'name-mismatch', message: isWorkbookDocument - ? 'Applying a dashboard needs a single fragment, but the cached file holds ' + + ? `Applying a ${kind} needs a single fragment, but the cached file holds ` + `a whole document. FIX: read-cached-xml with dashboard="${callerRef}" to pull just ` + 'that element, write-cached-xml with the same selector to splice your edit back, then apply ' + 'that file.' @@ -100,8 +104,8 @@ function resolveCanonicalDashboardName( return Err({ type: 'name-mismatch', message: - `dashboard_name "${dashboardName}" does not match the in the XML ("${xmlName}")` + - `${xmlId ? ` or its id ("${xmlId}")` : ''}. FIX: Retry with dashboard_name set to the XML's ` + + `${paramName} "${dashboardName}" does not match the in the XML ("${xmlName}")` + + `${xmlId ? ` or its id ("${xmlId}")` : ''}. FIX: Retry with ${paramName} set to the XML's ` + `name "${xmlName}"${xmlId ? ` or id "${xmlId}"` : ''} — or update the attribute ` + `in the XML to "${dashboardName}" if the caller name is intended.`, }); @@ -176,7 +180,7 @@ export async function loadDashboardXml({ // Require the caller's dashboard_name to agree with the XML root name before apply, then // thread the validated canonical name through the load. - const canonicalNameResult = resolveCanonicalDashboardName(dashboardName, xml); + const canonicalNameResult = resolveCanonicalDashboardName(dashboardName, xml, kind); if (canonicalNameResult.isErr()) { log({ level: 'error', diff --git a/src/server.desktop.test.ts b/src/server.desktop.test.ts index 6c92c020d..ade89f7d7 100644 --- a/src/server.desktop.test.ts +++ b/src/server.desktop.test.ts @@ -228,10 +228,15 @@ async function serializeDesktopToolSurface(tool: DesktopTool): Promise 40_081 (budget 39_087 -> 40_099, keeping the 18-char slack), full // surface 55_638 -> 56_650 (budget 55_656 -> 56_668). -const DYNAMIC_AUTHORING_SURFACE_EXPECTED = 40_081; -const DYNAMIC_AUTHORING_SURFACE_BUDGET = 40_099; +// Re-pinned 2026-08-19: get-storyboard-xml aligned to its get-xml siblings (get-dashboard-xml/ +// get-worksheet-xml) — storyboardName is now required and the deprecated storyboard alias param is +// dropped; net -51 bytes despite the longer "existing storyboard" description. It is in +// DYNAMIC_AUTHORING_TOOL_PROFILE, so both surfaces shrink: dynamic authoring 40_081 -> 40_030 +// (budget 40_099 -> 40_048), full surface 56_650 -> 56_599 (budget 56_668 -> 56_617), 18-char slack kept. +const DYNAMIC_AUTHORING_SURFACE_EXPECTED = 40_030; +const DYNAMIC_AUTHORING_SURFACE_BUDGET = 40_048; const DYNAMIC_AUTHORING_PRODUCT_CEILING = 46_000; -const FULL_TOOL_SURFACE_BUDGET = 56_668; +const FULL_TOOL_SURFACE_BUDGET = 56_617; describe('desktop tools/list serialized surface', () => { it('keeps the served dynamic authoring profile under the tool-search auto-deferral threshold budget', async () => { diff --git a/src/tools/desktop/api/getStoryboardXml.test.ts b/src/tools/desktop/api/getStoryboardXml.test.ts index ca8f01150..2a443e28c 100644 --- a/src/tools/desktop/api/getStoryboardXml.test.ts +++ b/src/tools/desktop/api/getStoryboardXml.test.ts @@ -68,36 +68,6 @@ describe('getStoryboardXmlTool', () => { expect(resultObj.storyboardXml).toContain('type="storyboard"'); }); - it('accepts the deprecated storyboard alias key', async () => { - const result = await getToolResult({ storyboard: 'QBR Story', mode: 'inline' }); - - expect(result.isError).toBe(false); - invariant(result.content[0].type === 'text'); - const resultObj = z - .object({ storyboardXml: z.string() }) - .parse(JSON.parse(result.content[0].text)); - expect(resultObj.storyboardXml).toContain('name="QBR Story"'); - }); - - it('errors when both storyboardName and its alias are absent', async () => { - const result = await getToolResult({}); - - expect(result.isError).toBe(true); - invariant(result.content[0].type === 'text'); - expect(result.content[0].text).toContain( - 'storyboardName is required (storyboard is a deprecated alias).', - ); - }); - - it('errors when storyboardName and its alias disagree', async () => { - const result = await getToolResult({ storyboardName: 'QBR Story', storyboard: 'Other' }); - - expect(result.isError).toBe(true); - invariant(result.content[0].type === 'text'); - expect(result.content[0].text).toContain('storyboardName ("QBR Story")'); - expect(result.content[0].text).toContain('Pass one of them.'); - }); - it('errors when the document route returns no subtree', async () => { const result = await getToolResult({ storyboardName: 'QBR Story', emptyDocument: true }); @@ -210,8 +180,7 @@ function makeExecutor({ async function getToolResult(opts: { session?: string; - storyboardName?: string; - storyboard?: string; + storyboardName: string; mode?: 'file' | 'inline'; capBytes?: number; bigDocument?: string; @@ -220,14 +189,7 @@ async function getToolResult(opts: { emptyDocument?: boolean; customSignal?: AbortSignal; }): Promise { - const { - session = '12345', - storyboardName, - storyboard, - mode = 'file', - capBytes, - customSignal, - } = opts; + const { session = '12345', storyboardName, mode = 'file', capBytes, customSignal } = opts; const { executor } = makeExecutor(opts); const tool = getStoryboardXmlTool(new DesktopMcpServer()); const callback = await Provider.from(tool.callback); @@ -240,7 +202,7 @@ async function getToolResult(opts: { ...(customSignal && { signal: customSignal }), ...(capBytes !== undefined && { config: { ...base.config, inlineXmlMaxBytes: capBytes } }), }; - return await callback({ session, storyboardName, storyboard, mode }, extra); + return await callback({ session, storyboardName, mode }, extra); } async function getToolCalls(opts: { storyboardName: string; customSignal?: AbortSignal }): Promise<{ @@ -261,7 +223,6 @@ async function getToolCalls(opts: { storyboardName: string; customSignal?: Abort { session: '12345', storyboardName: opts.storyboardName, - storyboard: undefined, mode: 'inline', }, extra, diff --git a/src/tools/desktop/api/getStoryboardXml.ts b/src/tools/desktop/api/getStoryboardXml.ts index 796bd36d4..8dace3d87 100644 --- a/src/tools/desktop/api/getStoryboardXml.ts +++ b/src/tools/desktop/api/getStoryboardXml.ts @@ -5,20 +5,13 @@ import { parseXML } from '../../../desktop/metadata/parser.js'; import { runExternalApiReadTool } from '../../../desktop/wrappers/readHarness.js'; import { UnknownError } from '../../../errors/mcpToolError.js'; import { DesktopMcpServer } from '../../../server.desktop.js'; -import { - artifactNameParam, - deprecatedArtifactAliasParam, - resolveArtifactNameArg, - sessionParam, - xmlModeParam, -} from '../params.js'; +import { artifactNameParam, sessionParam, xmlModeParam } from '../params.js'; import { DesktopTool } from '../tool.js'; import { finishXmlRead, XmlReadFileResult } from './xmlReadResult.js'; const paramsSchema = { session: sessionParam(), - storyboardName: artifactNameParam('storyboard').optional(), - storyboard: deprecatedArtifactAliasParam('storyboard'), + storyboardName: artifactNameParam('storyboard'), mode: xmlModeParam(), }; const title = 'Get Storyboard Document'; @@ -33,7 +26,7 @@ export const getStoryboardXmlTool = ( server, name: 'get-storyboard-xml', title, - description: 'Return one storyboard document subtree.', + description: 'Get structure for an existing storyboard.', paramsSchema, annotations: { readOnlyHint: false, // Writes to a cache file @@ -41,19 +34,11 @@ export const getStoryboardXmlTool = ( idempotentHint: false, openWorldHint: false, }, - callback: async ( - { session, storyboardName, storyboard, mode }, - extra, - ): Promise => { + callback: async ({ session, storyboardName, mode }, extra): Promise => { return await getStoryboardXml.logAndExecute({ extra, - args: { session, storyboardName, storyboard, mode }, + args: { session, storyboardName, mode }, callback: async () => { - const nameResult = resolveArtifactNameArg('storyboard', storyboardName, storyboard); - if (nameResult.isErr()) { - return nameResult; - } - const resolvedStoryboardName = nameResult.value; return await runExternalApiReadTool({ session, extra, @@ -71,7 +56,7 @@ export const getStoryboardXmlTool = ( const storyboardResult = resolveItemByNameOrId( 'Storyboard', - resolvedStoryboardName, + storyboardName, listResult.value.storyboards ?? [], ); if (storyboardResult.isErr()) { @@ -91,7 +76,7 @@ export const getStoryboardXmlTool = ( const storyboardXml = documentResult.value.xml; if (!parseXML(storyboardXml).dashboard) { return new UnknownError( - `No storyboard document subtree found for "${resolvedStoryboardName}".`, + `No storyboard document subtree found for "${storyboardName}".`, ).toErr(); } diff --git a/src/tools/desktop/api/listStoryboards.test.ts b/src/tools/desktop/api/listStoryboards.test.ts new file mode 100644 index 000000000..42ba7007d --- /dev/null +++ b/src/tools/desktop/api/listStoryboards.test.ts @@ -0,0 +1,124 @@ +import { CallToolResult } from '@modelcontextprotocol/sdk/types.js'; +import { Err, Ok } from 'ts-results-es'; +import { z } from 'zod'; + +import { DesktopCommandExecutionError } from '../../../errors/mcpToolError.js'; +import { DesktopMcpServer } from '../../../server.desktop.js'; +import invariant from '../../../utils/invariant.js'; +import { Provider } from '../../../utils/provider.js'; +import { TableauDesktopToolContext } from '../toolContext.js'; +import { getMockRequestHandlerExtra } from '../toolContext.mock.js'; +import { getListStoryboardsTool } from './listStoryboards.js'; + +const routeMissing = { + type: 'command-failed' as const, + error: { + code: 'not-found', + message: 'No route matches GET /v0/workbook/storyboards', + recoverable: false, + }, +}; + +describe('listStoryboardsTool', () => { + const resultSchema = z.object({ + storyboards: z.array(z.object({ id: z.string().optional(), name: z.string() })), + }); + + beforeEach(() => { + vi.clearAllMocks(); + }); + + it('should create a tool instance with correct properties', () => { + const tool = getListStoryboardsTool(new DesktopMcpServer()); + expect(tool.name).toBe('list-storyboards'); + expect(tool.description).toContain('stable id'); + expect(tool.paramsSchema).toMatchObject({ session: expect.any(Object) }); + expect(tool.annotations).toMatchObject({ readOnlyHint: true, openWorldHint: false }); + }); + + it('lists storyboards', async () => { + const listStoryboards = vi.fn().mockResolvedValue( + Ok({ + storyboards: [ + { id: 'story-1', name: 'QBR Story' }, + { id: 'story-2', name: 'Board Deck' }, + ], + }), + ); + + const result = await getToolResult({ listStoryboards }); + + expect(result.isError).toBe(false); + invariant(result.content[0].type === 'text'); + const resultObj = resultSchema.parse(JSON.parse(result.content[0].text)); + expect(resultObj.storyboards).toEqual([ + { id: 'story-1', name: 'QBR Story' }, + { id: 'story-2', name: 'Board Deck' }, + ]); + }); + + it('projects a missing storyboards field to an empty list', async () => { + const listStoryboards = vi.fn().mockResolvedValue(Ok({})); + + const result = await getToolResult({ listStoryboards }); + + expect(result.isError).toBe(false); + invariant(result.content[0].type === 'text'); + const resultObj = resultSchema.parse(JSON.parse(result.content[0].text)); + expect(resultObj.storyboards).toEqual([]); + }); + + it('maps a command-execution failure to DesktopCommandExecutionError', async () => { + const error = { type: 'command-timed-out' as const, error: 'Timeout' }; + const listStoryboards = vi.fn().mockResolvedValue(Err(error)); + + const result = await getToolResult({ listStoryboards }); + + expect(result.isError).toBe(true); + invariant(result.content[0].type === 'text'); + expect(result.content[0].text).toBe(new DesktopCommandExecutionError(error).message); + }); + + it('reports an honest too-new endpoint error when the storyboard list route is absent', async () => { + const listStoryboards = vi.fn().mockResolvedValue(Err(routeMissing)); + + const result = await getToolResult({ listStoryboards }); + + expect(result.isError).toBe(true); + invariant(result.content[0].type === 'text'); + expect(result.content[0].text).toContain('does not serve the storyboard list endpoint'); + expect(result.content[0].text).toContain('Do not retry'); + }); + + it('passes the abort signal to listStoryboards', async () => { + const listStoryboards = vi + .fn() + .mockResolvedValue(Ok({ storyboards: [{ id: 'story-1', name: 'QBR Story' }] })); + const customSignal = new AbortController().signal; + + await getToolResult({ listStoryboards, customSignal }); + + expect(listStoryboards).toHaveBeenCalledWith(customSignal); + }); +}); + +// list-storyboards calls executor.listStoryboards directly through the read harness (no wrapper +// module to spy on), so drive it by injecting a fake executor via getExecutor. +async function getToolResult({ + listStoryboards, + customSignal, +}: { + listStoryboards: ReturnType; + customSignal?: AbortSignal; +}): Promise { + const tool = getListStoryboardsTool(new DesktopMcpServer()); + const callback = await Provider.from(tool.callback); + const extra = { + ...getMockRequestHandlerExtra(), + getExecutor: vi.fn().mockResolvedValue({ + listStoryboards, + }) as unknown as TableauDesktopToolContext['getExecutor'], + ...(customSignal && { signal: customSignal }), + }; + return await callback({ session: '12345' }, extra); +} diff --git a/src/tools/desktop/local/cache/readCachedXml.test.ts b/src/tools/desktop/local/cache/readCachedXml.test.ts index 8c878bc03..e5bccdc56 100644 --- a/src/tools/desktop/local/cache/readCachedXml.test.ts +++ b/src/tools/desktop/local/cache/readCachedXml.test.ts @@ -112,7 +112,10 @@ describe('readCachedXmlTool', () => { "[Sales]
" + "[Profit]
" + '' + - "" + + '' + + "" + + "" + + '' + ''; beforeEach(() => { @@ -158,6 +161,16 @@ describe('readCachedXmlTool', () => { expect(result.content[0].text).not.toContain('[Profit]'); }); + it('slices a storyboard via the dashboard selector (a story is a element)', async () => { + const result = await getResult(CACHED_FILE, { dashboard: 'QBR Story' }); + + invariant(result.content[0].type === 'text'); + expect(result.content[0].text).toContain("type='storyboard'"); + expect(result.content[0].text).toContain(""); + // Only the story, not the sibling real dashboard. + expect(result.content[0].text).not.toContain(""); + }); + it('returns a byte range slice', async () => { const result = await getResult(CACHED_FILE, { startByte: 0, endByte: 10 }); diff --git a/src/tools/desktop/local/cache/writeCachedXml.test.ts b/src/tools/desktop/local/cache/writeCachedXml.test.ts index 6d94c6003..691df2e6e 100644 --- a/src/tools/desktop/local/cache/writeCachedXml.test.ts +++ b/src/tools/desktop/local/cache/writeCachedXml.test.ts @@ -296,6 +296,22 @@ describe('writeCachedXmlTool', () => { expect(written).toContain(""); }); + it('splices a storyboard via the dashboard selector (a story is a element)', async () => { + vi.mocked(readFileSync).mockReturnValue( + "", + ); + const result = await getResult( + CACHED_FILE, + "", + { dashboard: 'QBR Story' }, + ); + + expect(result.isError).toBeFalsy(); + const written = vi.mocked(writeFileSync).mock.calls[0][1] as string; + expect(written).toContain(""); + expect(written).not.toContain(""); + }); + it('splices when an entity-escaped fragment name matches a plain-text selector', async () => { vi.mocked(readFileSync).mockReturnValue( '' +