Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion package.json
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
{
"name": "@tableau/mcp-server",
"description": "Helping agents see and understand data.",
"version": "2.25.4",
"version": "2.25.5",
"repository": {
"type": "git",
"url": "git+https://github.com/tableau/tableau-mcp.git"
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,7 @@ function field(columnRef: string, role: string, datatype: string, type: string):
return {
column_ref: columnRef,
role,
datasource: 'Sample Superstore',
datasource: columnRef.match(/^\[([^\]]+)\]\./)?.[1] ?? 'Sample Superstore',
columnName: columnRef,
columnInstanceName: columnRef,
derivation: 'None',
Expand Down Expand Up @@ -250,19 +250,22 @@ describe('buildAndApplyWorksheetTool — mapping construction characterization',
expect(captured.metadata).toHaveProperty('Dim1');
});

it('derives the datasource name from the <datasource caption=...> when present', async () => {
it('CHARACTERIZATION REBASELINE: rewrites with the refs common datasource, not workbook caption', async () => {
// NEW INVARIANT (seam-1 packet B / #219): the rewrite datasource comes from the
// explicit bind or, for no-manifest passthrough, the single common datasource in
// taskSpec.fields. Workbook caption is no longer allowed to repoint the refs.
const extra = makeExtra();
const captured = captureCall();

await getResult({ session: SESSION, taskSpec: TASK_SPEC_BASE }, extra);

expect(captured.datasource).toBe('Sample - Superstore');
expect(captured.datasource).toBe('DS');
});

it('CHARACTERIZATION: without a caption, uses the first datasource name that is not "Parameters"', async () => {
// CHARACTERIZATION: current behavior — caption wins; otherwise the first
// non-"Parameters" datasource name is used (the literal "Parameters" datasource
// is always skipped).
it('CHARACTERIZATION REBASELINE: without a caption, still rewrites with the refs common datasource', async () => {
// NEW INVARIANT (seam-1 packet B / #219): fallback to the workbook's first
// non-Parameters datasource is gone; no-manifest passthrough must infer exactly
// one datasource from taskSpec.fields and fail closed if refs are mixed.
const workbook =
'<workbook><datasources>' +
'<datasource name="Parameters"/>' +
Expand All @@ -273,7 +276,7 @@ describe('buildAndApplyWorksheetTool — mapping construction characterization',

await getResult({ session: SESSION, taskSpec: TASK_SPEC_BASE }, extra);

expect(captured.datasource).toBe('Real DS');
expect(captured.datasource).toBe('DS');
});

it('applies the extracted worksheet through the mocked session boundary on success', async () => {
Expand Down
112 changes: 108 additions & 4 deletions src/tools/desktop/coordination/buildAndApplyWorksheet.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,14 @@ const WORKBOOK_XML = `<?xml version="1.0"?>
</datasources>
</workbook>`;

const TWO_DATASOURCE_WORKBOOK_XML = `<?xml version="1.0"?>
<workbook>
<datasources>
<datasource name="DS_A" caption="First Caption"/>
<datasource name="DS_B"/>
</datasources>
</workbook>`;

const TEMPLATE_XML =
'<workbook><worksheets><worksheet name="TEMPLATE"><table/></worksheet></worksheets></workbook>';

Expand Down Expand Up @@ -67,6 +75,41 @@ function makeExtra(): TableauDesktopRequestHandlerExtra {
return extra;
}

function twoDatasourceFields(): any[] {
return [
{
column_ref: '[DS_A].[none:Region:nk]',
role: 'dimension',
datasource: 'DS_A',
columnName: '[Region]',
columnInstanceName: '[none:Region:nk]',
derivation: 'None' as any,
type: 'nominal',
datatype: 'string',
},
{
column_ref: '[DS_B].[none:Region:nk]',
role: 'dimension',
datasource: 'DS_B',
columnName: '[Region]',
columnInstanceName: '[none:Region:nk]',
derivation: 'None' as any,
type: 'nominal',
datatype: 'string',
},
{
column_ref: '[DS_B].[sum:Sales:qk]',
role: 'measure',
datasource: 'DS_B',
columnName: '[Sales]',
columnInstanceName: '[sum:Sales:qk]',
derivation: 'Sum' as any,
type: 'quantitative',
datatype: 'integer',
},
];
}

const TASK_SPEC_BASE = {
worksheetName: 'Sheet1',
worksheetFile: '/cache/worksheet.xml',
Expand Down Expand Up @@ -193,23 +236,84 @@ describe('buildAndApplyWorksheetTool', () => {
expect(result.isError).toBe(true);
});

it('should call rewriteFieldReferences with template, fieldMapping, datasource name, and namespacing options', async () => {
it('should call rewriteFieldReferences with template, fieldMapping, resolved datasource, and namespacing options', async () => {
await getResult({ session: SESSION, taskSpec: TASK_SPEC_BASE });

// CONVERGENCE: build-and-apply now calls the shared core (rewriteFieldReferences)
// directly instead of the deleted replaceFieldReferences wrapper, so the call
// gains a 5th arg: the per-apply options object wiring calc namespacing ON with a
// caller-minted nonce. The first four args (template, mapping, datasource,
// metadata) are unchanged.
// caller-minted nonce. Seam-1 packet B changes the datasource arg to the resolved
// bind datasource instead of the workbook caption.
expect(rewriteFieldReferences).toHaveBeenCalledWith(
TEMPLATE_XML,
expect.any(Object),
expect.stringMatching(/Sample/),
'DS',
expect.any(Object),
{ namespaceCalcs: true, applyNonce: expect.any(String) },
);
});

it('uses the explicit bind datasource for manifest-backed rewrites', async () => {
const extra = makeExtra();
vi.mocked(readFileSync).mockReturnValue(TWO_DATASOURCE_WORKBOOK_XML as any);
vi.mocked(listAvailableFields).mockReturnValue(twoDatasourceFields() as any);
vi.mocked(getTemplateColumnRequirements).mockReturnValue([
{ name: 'Region', role: 'dimension', datatype: 'string', type: 'nominal' },
{ name: 'Sales', role: 'measure', datatype: 'integer', type: 'quantitative' },
]);

const result = await getResult(
{
session: SESSION,
taskSpec: {
...TASK_SPEC_BASE,
fields: ['[DS_B].[none:Region:nk]', '[DS_B].[sum:Sales:qk]'],
},
},
extra,
);

expect(result.isError).toBeFalsy();
expect(rewriteFieldReferences).toHaveBeenCalledWith(
TEMPLATE_XML,
expect.any(Object),
'DS_B',
expect.any(Object),
{ namespaceCalcs: true, applyNonce: expect.any(String) },
);
});

it('blocks no-manifest passthrough when provided refs span datasources', async () => {
const extra = makeExtra();
vi.mocked(readFileSync).mockReturnValue(TWO_DATASOURCE_WORKBOOK_XML as any);
vi.mocked(readTemplate).mockReturnValue(TEMPLATE_XML);
vi.mocked(listAvailableFields).mockReturnValue(twoDatasourceFields() as any);
vi.mocked(getTemplateColumnRequirements).mockReturnValue([
{ name: 'Region', role: 'dimension', datatype: 'string', type: 'nominal' },
{ name: 'Sales', role: 'measure', datatype: 'integer', type: 'quantitative' },
]);

const result = await getResult(
{
session: SESSION,
taskSpec: {
...TASK_SPEC_BASE,
template: 'loose-template-without-manifest',
fields: ['[DS_A].[none:Region:nk]', '[DS_B].[sum:Sales:qk]'],
},
},
extra,
);

expect(result.isError).toBe(true);
invariant(result.content[0].type === 'text');
expect(result.content[0].text).toContain('BLOCKED: mixed-datasource field references');
expect(result.content[0].text).toContain('DS_A');
expect(result.content[0].text).toContain('DS_B');
expect(rewriteFieldReferences).not.toHaveBeenCalled();
expect(loadWorksheetXml).not.toHaveBeenCalled();
});

it('should return error when extracted worksheet element is missing from template', async () => {
const extra = makeExtra();
vi.mocked(rewriteFieldReferences).mockReturnValue('<workbook>no worksheet here</workbook>');
Expand Down
67 changes: 41 additions & 26 deletions src/tools/desktop/coordination/buildAndApplyWorksheet.ts
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ import {
} from '../../../desktop/binder/explicit-bind.js';
import { checkSidecar } from '../../../desktop/commands/workbook/cacheFingerprint.js';
import { loadWorksheetXml } from '../../../desktop/commands/workbook/loadWorksheetXml.js';
import { parseDatasourceQualifiedColumnRef } from '../../../desktop/metadata/field-resolver.js';
import { listAvailableFields } from '../../../desktop/metadata/index.js';
import {
checkRouteGateForScratchEntry,
Expand Down Expand Up @@ -60,6 +61,31 @@ function escapeXml(text: string): string {
.replace(/'/g, '&apos;');
}

function inferSingleDatasourceFromColumnRefs(
refs: string[],
): { ok: true; datasource: string | null } | { ok: false; message: string } {
const refsByDatasource = new Map<string, string[]>();
for (const ref of refs) {
const datasource = parseDatasourceQualifiedColumnRef(ref.trim())?.datasource;
if (!datasource) continue;
refsByDatasource.set(datasource, [...(refsByDatasource.get(datasource) ?? []), ref]);
}
if (refsByDatasource.size <= 1) {
return { ok: true, datasource: [...refsByDatasource.keys()][0] ?? null };
}
const breakdown = [...refsByDatasource.entries()]
.map(([datasource, dsRefs]) => `${datasource} (${dsRefs.join(', ')})`)
.join('; ');
return {
ok: false,
message:
'BLOCKED: mixed-datasource field references — cannot build worksheet\n\n' +
`taskSpec.fields resolve to multiple datasources — ${breakdown}. The no-manifest passthrough ` +
'path substitutes a single {{DATASOURCE}} and would silently repoint fields to the wrong datasource.\n\n' +
'FIX: Provide refs from one datasource, or use a manifest-backed template/data-model relationship that binds within one datasource.',
};
}

const paramsSchema = {
session: z.string().optional().describe('Session ID; optional if pinned or unique.'),
taskSpec: z
Expand Down Expand Up @@ -143,23 +169,9 @@ export const getBuildAndApplyWorksheetTool = (

const workbookXml = readFileSync(workbookFile, 'utf-8');

// Determine datasource name from workbook
let datasourceName = 'Unknown';
const captionMatch = workbookXml.match(/<datasource[^>]+caption=["']([^"']+)["']/);
if (captionMatch) {
datasourceName = captionMatch[1];
} else {
const allMatches = workbookXml.matchAll(/<datasource[^>]+name=["']([^"']+)["']/g);
for (const match of allMatches) {
if (match[1] !== 'Parameters') {
datasourceName = match[1];
break;
}
}
}

// Get available fields for role detection
const availableFields = listAvailableFields(workbookXml);
const schemaSummary = schemaSummaryFromAvailableFields(availableFields);

// Fields dropped here (no role match, or beyond the template's slot count)
// used to vanish silently (pinned by X1). Collect a non-breaking warning
Expand Down Expand Up @@ -230,16 +242,11 @@ export const getBuildAndApplyWorksheetTool = (
// Manifest enforcement (P0 W-23447710): slot derivations/keys come from the
// manifest, never the caller's positional refs. Blockers stop the apply —
// stricter than the old behavior, which left sample fields in unmapped slots.
const explicitBind = bindExplicitTemplate(
template,
fields,
schemaSummaryFromAvailableFields(availableFields),
{
title: worksheetName,
datasource: datasourceName,
passthroughFieldMapping,
},
);
const explicitBind = bindExplicitTemplate(template, fields, schemaSummary, {
title: worksheetName,
datasource: schemaSummary.datasource,
passthroughFieldMapping,
});

if (!explicitBind.ok) {
return new ArgsValidationError(
Expand All @@ -249,6 +256,14 @@ export const getBuildAndApplyWorksheetTool = (

warnings.push(...explicitBind.warnings);
const fieldMapping = explicitBind.fieldMapping;
let rewriteDatasource = explicitBind.datasource;
if (explicitBind.passthrough) {
const inferred = inferSingleDatasourceFromColumnRefs(fields);
if (!inferred.ok) {
return new ArgsValidationError(inferred.message).toErr();
}
rewriteDatasource = inferred.datasource ?? explicitBind.datasource;
}
const fieldMetadata =
Object.keys(explicitBind.fieldMetadata).length > 0
? explicitBind.fieldMetadata
Expand All @@ -269,7 +284,7 @@ export const getBuildAndApplyWorksheetTool = (
templateXml = rewriteFieldReferences(
templateXml,
fieldMapping,
datasourceName,
rewriteDatasource,
fieldMetadata,
{ namespaceCalcs: true, applyNonce },
);
Expand Down
Loading
Loading