From e2d67ac5f1e128236b24293146b0982e1350ceb6 Mon Sep 17 00:00:00 2001 From: Patrick Green Date: Tue, 20 Oct 2026 17:16:42 -0600 Subject: [PATCH 01/21] Pack Data/ and Resources/ in package_twbx.py package_twbx.py wrote only the .twb and Packages/ into the .twbx, so a data app whose .twb references an embedded datasource file (for example Data/Datasources/Sample - Superstore.xls) packaged and published cleanly but had no data at runtime. Also pack Data/ and Resources/ at the archive root when the workspace has them. The .twb is still written first, OS cruft is still skipped, the Packages/ and .trex checks are unchanged, and nothing else at the workspace root is packed. SKILL.md's Package section says so. --- .../tableau-data-app-authoring/SKILL.md | 8 +++- .../scripts/package_twbx.py | 42 ++++++++++++------- .../scripts/tests/test_package_twbx.py | 38 +++++++++++++++++ 3 files changed, 71 insertions(+), 17 deletions(-) diff --git a/src/plugin/skills/tableau-data-app-authoring/SKILL.md b/src/plugin/skills/tableau-data-app-authoring/SKILL.md index 97429f7..e064ce9 100644 --- a/src/plugin/skills/tableau-data-app-authoring/SKILL.md +++ b/src/plugin/skills/tableau-data-app-authoring/SKILL.md @@ -328,6 +328,11 @@ hand: python3 "$SKILL_DIR/scripts/package_twbx.py" "" # the finalized workspace dir ``` +It also packs `Data/` and `Resources/` at the archive root when the workspace +has them, so a `.twb` that references an embedded datasource file by relative +path (e.g. `Data/Datasources/.xls`) keeps its data. Nothing else at the +workspace root is packed. + It writes `.twbx` next to the workspace dir (pass a second arg to override), overwriting any existing file, and prints the path for Publish. @@ -394,7 +399,8 @@ Uses the MCP publish tools (not available to Slack clients). ## Non-negotiable limits - Don't zip the `.twbx` freehand. Use `scripts/package_twbx.py` — it keeps - `.twb` + `Packages/` at the archive root, never nested under `/`. + `.twb`, `Packages/`, and any `Data/`/`Resources/` at the archive root, never + nested under `/`. - Don't apply a postUnzip plan freehand. Use `scripts/apply_plan.py` — edits before renames, renames deepest-first, verified; see its header comment for the full contract. diff --git a/src/plugin/skills/tableau-data-app-authoring/scripts/package_twbx.py b/src/plugin/skills/tableau-data-app-authoring/scripts/package_twbx.py index 6b850d7..a3941e1 100644 --- a/src/plugin/skills/tableau-data-app-authoring/scripts/package_twbx.py +++ b/src/plugin/skills/tableau-data-app-authoring/scripts/package_twbx.py @@ -4,11 +4,14 @@ / .twb Packages//extensions/*.trex, content/... + Data/... (optional: embedded datasource files the .twb references) + Resources/... (optional) -The archive holds the `.twb` (written first) and `Packages/` at its ROOT, never -nested under `/` — nesting fails to open on publish with +The archive holds the `.twb` (written first), `Packages/`, and any `Data/` and +`Resources/` at its ROOT, never nested under `/` — nesting fails to open on publish with `PackageValidationException: Package directory contains no extension .trex -files under extensions/`. OS cruft (`.DS_Store`, `__MACOSX`) is skipped. +files under extensions/`. Without `Data/`, a `.twb` that references an embedded +file by relative path publishes fine but has no data at runtime. OS cruft (`.DS_Store`, `__MACOSX`) is skipped. Hard-fails unless the workspace root has exactly one `.twb`, a `Packages/` dir, and at least one `.trex` under `Packages/*/extensions/`. @@ -24,6 +27,9 @@ import sys import zipfile +# Workspace-root dirs copied into the archive root, in order. Packages/ is +# required (checked in main); the rest are packed only when present. +PACKED_DIRS = ('Packages', 'Data', 'Resources') def die(message): print(f'✗ {message}', file=sys.stderr) @@ -60,22 +66,26 @@ def main(): output = os.path.abspath(argv[2]) if len(argv) == 3 else os.path.join( os.path.dirname(workspace), os.path.basename(workspace) + '.twbx') + dirs_to_pack = [name for name in PACKED_DIRS if os.path.isdir(os.path.join(workspace, name))] + # Paths are workspace-relative so nothing lands under an / prefix. with zipfile.ZipFile(output, 'w', zipfile.ZIP_DEFLATED) as archive: archive.write(os.path.join(workspace, twb), twb) - for root, dirs, files in os.walk(packages): - dirs.sort() - rel_root = os.path.relpath(root, workspace).replace(os.sep, '/') - if is_cruft(rel_root): - dirs[:] = [] - continue - archive.write(root, rel_root) - for name in sorted(files): - rel = f'{rel_root}/{name}' - if not is_cruft(rel): - archive.write(os.path.join(root, name), rel) - - print(f'✓ Packaged {twb} + Packages/ into {output}', file=sys.stderr) + for top in dirs_to_pack: + for root, dirs, files in os.walk(os.path.join(workspace, top)): + dirs.sort() + rel_root = os.path.relpath(root, workspace).replace(os.sep, '/') + if is_cruft(rel_root): + dirs[:] = [] + continue + archive.write(root, rel_root) + for name in sorted(files): + rel = f'{rel_root}/{name}' + if not is_cruft(rel): + archive.write(os.path.join(root, name), rel) + + packed = ', '.join(f'{name}/' for name in dirs_to_pack) + print(f'✓ Packaged {twb} + {packed} into {output}', file=sys.stderr) print(output) diff --git a/src/plugin/skills/tableau-data-app-authoring/scripts/tests/test_package_twbx.py b/src/plugin/skills/tableau-data-app-authoring/scripts/tests/test_package_twbx.py index 5ab9e6e..79bb007 100644 --- a/src/plugin/skills/tableau-data-app-authoring/scripts/tests/test_package_twbx.py +++ b/src/plugin/skills/tableau-data-app-authoring/scripts/tests/test_package_twbx.py @@ -69,6 +69,44 @@ def test_excludes_ds_store_and_macosx(tmp_path): assert f'{PACKAGE}/content/src/app.js' in entries +def test_packs_data_and_resources_at_root(tmp_path): + workspace = make_workspace(tmp_path) + datasources = workspace / 'Data' / 'Datasources' + datasources.mkdir(parents=True) + (datasources / 'Sample.xls').write_bytes(b'\xd0\xcf\x11\xe0') + (datasources / '.DS_Store').write_text('x') + agents = workspace / 'Resources' / '.agents' + agents.mkdir(parents=True) + (agents / 'notes.md').write_text('# notes') + + result = run_package(str(workspace)) + + assert result.returncode == 0, result.stderr + entries = names(result.stdout.strip()) + assert entries[0] == 'Sales Demo.twb' + assert 'Data/Datasources/Sample.xls' in entries + assert 'Resources/.agents/notes.md' in entries + assert f'{PACKAGE}/extensions/data-app.trex' in entries + assert not any(entry.startswith('Sales Demo/') for entry in entries) + assert not any('.DS_Store' in entry for entry in entries) + with zipfile.ZipFile(result.stdout.strip()) as archive: + assert archive.read('Data/Datasources/Sample.xls') == b'\xd0\xcf\x11\xe0' + assert '✓ Packaged Sales Demo.twb + Packages/, Data/, Resources/' in result.stderr + + +def test_skips_other_workspace_dirs(tmp_path): + workspace = make_workspace(tmp_path) + (workspace / 'node_modules' / 'pkg').mkdir(parents=True) + (workspace / 'node_modules' / 'pkg' / 'index.js').write_text('//') + + result = run_package(str(workspace)) + + assert result.returncode == 0, result.stderr + entries = names(result.stdout.strip()) + assert not any(entry.startswith('node_modules') for entry in entries) + assert not any(entry.startswith(('Data', 'Resources')) for entry in entries) + + def test_explicit_output_overwrites_existing_file(tmp_path): workspace = make_workspace(tmp_path) output = tmp_path / 'out' / 'custom.twbx' From 7e92e226f2b7421899f11ed87bcbdc8bae218d1d Mon Sep 17 00:00:00 2001 From: Patrick Green Date: Thu, 22 Oct 2026 16:21:49 -0600 Subject: [PATCH 02/21] Narrow Resources/, skip hidden entries, check Data/ refs Address review feedback on package_twbx.py: - Pack only Resources/.agents/ (the agent-docs folder that travels with the workbook) instead of all of Resources/. - Skip any file or dir whose name starts with "." below the packed dirs, and list each one on stderr with the reason. SKILL.md says to relay those lines to the user. - Fail when the .twb references a Data/ file (connection filename or dbname) that is missing from the workspace. - Follow symlinked dirs, with a guard against symlink loops. --- .../tableau-data-app-authoring/SKILL.md | 13 ++- .../scripts/package_twbx.py | 78 +++++++++++++--- .../scripts/tests/test_package_twbx.py | 91 +++++++++++++++++-- 3 files changed, 157 insertions(+), 25 deletions(-) diff --git a/src/plugin/skills/tableau-data-app-authoring/SKILL.md b/src/plugin/skills/tableau-data-app-authoring/SKILL.md index e064ce9..9962ec2 100644 --- a/src/plugin/skills/tableau-data-app-authoring/SKILL.md +++ b/src/plugin/skills/tableau-data-app-authoring/SKILL.md @@ -328,10 +328,13 @@ hand: python3 "$SKILL_DIR/scripts/package_twbx.py" "" # the finalized workspace dir ``` -It also packs `Data/` and `Resources/` at the archive root when the workspace -has them, so a `.twb` that references an embedded datasource file by relative -path (e.g. `Data/Datasources/.xls`) keeps its data. Nothing else at the -workspace root is packed. +It also packs `Data/` and `Resources/.agents/` at the archive root when the +workspace has them, so a `.twb` that references an embedded datasource file by +relative path (e.g. `Data/Datasources/.xls`) keeps its data. It fails if +a `Data/` file the `.twb` references is missing. Nothing else at the workspace +root or under `Resources/` is packed, and below those dirs any file or folder +whose name starts with `.` is skipped, along with everything inside it. Each +one is listed on stderr with the reason; relay those lines to the user. It writes `.twbx` next to the workspace dir (pass a second arg to override), overwriting any existing file, and prints the path for Publish. @@ -399,7 +402,7 @@ Uses the MCP publish tools (not available to Slack clients). ## Non-negotiable limits - Don't zip the `.twbx` freehand. Use `scripts/package_twbx.py` — it keeps - `.twb`, `Packages/`, and any `Data/`/`Resources/` at the archive root, never + `.twb`, `Packages/`, and any `Data/`/`Resources/.agents/` at the archive root, never nested under `/`. - Don't apply a postUnzip plan freehand. Use `scripts/apply_plan.py` — edits before renames, renames deepest-first, verified; see its header comment for diff --git a/src/plugin/skills/tableau-data-app-authoring/scripts/package_twbx.py b/src/plugin/skills/tableau-data-app-authoring/scripts/package_twbx.py index a3941e1..9c28459 100644 --- a/src/plugin/skills/tableau-data-app-authoring/scripts/package_twbx.py +++ b/src/plugin/skills/tableau-data-app-authoring/scripts/package_twbx.py @@ -4,17 +4,22 @@ / .twb Packages//extensions/*.trex, content/... - Data/... (optional: embedded datasource files the .twb references) - Resources/... (optional) + Data/... (optional: embedded datasource files the .twb references) + Resources/.agents/... (optional: agent docs that travel with the workbook) The archive holds the `.twb` (written first), `Packages/`, and any `Data/` and -`Resources/` at its ROOT, never nested under `/` — nesting fails to open on publish with +`Resources/.agents/` at its ROOT, never nested under `/` — nesting fails to open on publish with `PackageValidationException: Package directory contains no extension .trex files under extensions/`. Without `Data/`, a `.twb` that references an embedded -file by relative path publishes fine but has no data at runtime. OS cruft (`.DS_Store`, `__MACOSX`) is skipped. +file by relative path publishes fine but has no data at runtime. Nothing else +under `Resources/` is packed. OS cruft (`.DS_Store`, `__MACOSX`) and any +file or dir whose name starts with `.` below those dirs are skipped; hidden +ones are listed on stderr as ` skipped ()`. Symlinked files and +dirs are followed. Hard-fails unless the workspace root has exactly one `.twb`, a `Packages/` dir, -and at least one `.trex` under `Packages/*/extensions/`. +at least one `.trex` under `Packages/*/extensions/`, and every `Data/` file the +`.twb` references. Output defaults to `.twbx` next to the workspace dir; an existing file is overwritten. Prints the output path on stdout. @@ -25,11 +30,18 @@ import glob import os import sys +import xml.etree.ElementTree as ET import zipfile -# Workspace-root dirs copied into the archive root, in order. Packages/ is +# Workspace-relative dirs copied into the archive root, in order. Packages/ is # required (checked in main); the rest are packed only when present. -PACKED_DIRS = ('Packages', 'Data', 'Resources') +PACKED_DIRS = ('Packages', 'Data', 'Resources/.agents') + +HIDDEN_REASON = "(hidden: names starting with '.' are not packed)" + +# Connection attributes that hold a workbook-relative file path. +FILE_ATTRS = ('filename', 'dbname') + def die(message): print(f'✗ {message}', file=sys.stderr) @@ -40,6 +52,26 @@ def is_cruft(rel_path): return any(part.endswith('.DS_Store') or part.startswith('__MACOSX') for part in rel_path.split('/')) +def is_hidden(rel_path, top): + # Hidden below the packed dir only, so Resources/.agents itself still ships. + below = rel_path[len(top):].strip('/') + return any(part.startswith('.') for part in below.split('/') if part) + + +def missing_data_files(workspace, twb): + try: + root = ET.parse(os.path.join(workspace, twb)).getroot() + except ET.ParseError as error: + die(f'Could not parse {twb}: {error}') + missing = set() + for element in root.iter('connection'): + for attr in FILE_ATTRS: + path = element.get(attr, '').replace('\\', '/') + if path.startswith('Data/') and not os.path.isfile(os.path.join(workspace, path)): + missing.add(path) + return sorted(missing) + + def main(): argv = sys.argv if len(argv) < 2 or len(argv) > 3: @@ -63,28 +95,46 @@ def main(): die('No .trex under Packages/*/extensions/ — publish would fail with ' '"Package directory contains no extension .trex files under extensions/".') + missing = missing_data_files(workspace, twb) + if missing: + die(f'{twb} references files missing from the workspace (no data at runtime): ' + + ', '.join(missing)) + output = os.path.abspath(argv[2]) if len(argv) == 3 else os.path.join( os.path.dirname(workspace), os.path.basename(workspace) + '.twbx') - dirs_to_pack = [name for name in PACKED_DIRS if os.path.isdir(os.path.join(workspace, name))] + dirs_to_pack = [top for top in PACKED_DIRS if os.path.isdir(os.path.join(workspace, top))] # Paths are workspace-relative so nothing lands under an / prefix. with zipfile.ZipFile(output, 'w', zipfile.ZIP_DEFLATED) as archive: archive.write(os.path.join(workspace, twb), twb) + visited = set() for top in dirs_to_pack: - for root, dirs, files in os.walk(os.path.join(workspace, top)): + for root, dirs, files in os.walk(os.path.join(workspace, top), followlinks=True): dirs.sort() rel_root = os.path.relpath(root, workspace).replace(os.sep, '/') - if is_cruft(rel_root): + real_root = os.path.realpath(root) + if is_cruft(rel_root) or real_root in visited: dirs[:] = [] continue + visited.add(real_root) archive.write(root, rel_root) + # Prune hidden dirs here so each is reported once, not per file inside. + for name in list(dirs): + rel = f'{rel_root}/{name}' + if not is_cruft(rel) and is_hidden(rel, top): + print(f' skipped {rel}/ {HIDDEN_REASON}', file=sys.stderr) + dirs.remove(name) for name in sorted(files): rel = f'{rel_root}/{name}' - if not is_cruft(rel): - archive.write(os.path.join(root, name), rel) - - packed = ', '.join(f'{name}/' for name in dirs_to_pack) + if is_cruft(rel): + continue + if is_hidden(rel, top): + print(f' skipped {rel} {HIDDEN_REASON}', file=sys.stderr) + continue + archive.write(os.path.join(root, name), rel) + + packed = ', '.join(f'{top}/' for top in dirs_to_pack) print(f'✓ Packaged {twb} + {packed} into {output}', file=sys.stderr) print(output) diff --git a/src/plugin/skills/tableau-data-app-authoring/scripts/tests/test_package_twbx.py b/src/plugin/skills/tableau-data-app-authoring/scripts/tests/test_package_twbx.py index 79bb007..cc5d751 100644 --- a/src/plugin/skills/tableau-data-app-authoring/scripts/tests/test_package_twbx.py +++ b/src/plugin/skills/tableau-data-app-authoring/scripts/tests/test_package_twbx.py @@ -10,6 +10,9 @@ SCRIPT = os.path.join(os.path.dirname(os.path.dirname(os.path.abspath(__file__))), 'package_twbx.py') PACKAGE = 'Packages/com.tableau.mcp.sales-demo' +TWB_WITH_XLS = ("" + "" + "") def run_package(*args): @@ -69,15 +72,13 @@ def test_excludes_ds_store_and_macosx(tmp_path): assert f'{PACKAGE}/content/src/app.js' in entries -def test_packs_data_and_resources_at_root(tmp_path): +def test_packs_data_at_root(tmp_path): workspace = make_workspace(tmp_path) datasources = workspace / 'Data' / 'Datasources' datasources.mkdir(parents=True) (datasources / 'Sample.xls').write_bytes(b'\xd0\xcf\x11\xe0') (datasources / '.DS_Store').write_text('x') - agents = workspace / 'Resources' / '.agents' - agents.mkdir(parents=True) - (agents / 'notes.md').write_text('# notes') + (workspace / 'Sales Demo.twb').write_text(TWB_WITH_XLS) result = run_package(str(workspace)) @@ -85,13 +86,91 @@ def test_packs_data_and_resources_at_root(tmp_path): entries = names(result.stdout.strip()) assert entries[0] == 'Sales Demo.twb' assert 'Data/Datasources/Sample.xls' in entries - assert 'Resources/.agents/notes.md' in entries assert f'{PACKAGE}/extensions/data-app.trex' in entries assert not any(entry.startswith('Sales Demo/') for entry in entries) assert not any('.DS_Store' in entry for entry in entries) with zipfile.ZipFile(result.stdout.strip()) as archive: assert archive.read('Data/Datasources/Sample.xls') == b'\xd0\xcf\x11\xe0' - assert '✓ Packaged Sales Demo.twb + Packages/, Data/, Resources/' in result.stderr + assert '✓ Packaged Sales Demo.twb + Packages/, Data/ into' in result.stderr + + +def test_fails_when_twb_references_missing_data_file(tmp_path): + workspace = make_workspace(tmp_path) + (workspace / 'Sales Demo.twb').write_text(TWB_WITH_XLS) + + result = run_package(str(workspace)) + + assert result.returncode == 1 + assert '✗ Sales Demo.twb references files missing from the workspace' in result.stderr + assert 'Data/Datasources/Sample.xls' in result.stderr + assert not (tmp_path / 'Sales Demo.twbx').exists() + + +def test_packs_only_agents_under_resources(tmp_path): + workspace = make_workspace(tmp_path) + agents = workspace / 'Resources' / '.agents' + (agents / 'history').mkdir(parents=True) + (agents / 'README.md').write_text('# agents') + (agents / 'history' / 'spec.md').write_text('# old spec') + (workspace / 'Resources' / 'scratch.md').write_text('scratch') + + result = run_package(str(workspace)) + + assert result.returncode == 0, result.stderr + entries = names(result.stdout.strip()) + assert 'Resources/.agents/README.md' in entries + assert 'Resources/.agents/history/spec.md' in entries + assert 'Resources/scratch.md' not in entries + assert '✓ Packaged Sales Demo.twb + Packages/, Resources/.agents/ into' in result.stderr + + +def test_skips_hidden_entries_below_packed_dirs(tmp_path): + workspace = make_workspace(tmp_path) + content = workspace / PACKAGE / 'content' + (content / '.git').mkdir() + (content / '.git' / 'config').write_text('x') + (content / '.env').write_text('SECRET=x') + (content / '.DS_Store').write_text('x') + (content / '.config' / 'gh').mkdir(parents=True) + (content / '.config' / 'gh' / 'hosts.yml').write_text('token: x') + data = workspace / 'Data' + data.mkdir() + (data / '.env').write_text('SECRET=x') + agents = workspace / 'Resources' / '.agents' + agents.mkdir(parents=True) + (agents / 'README.md').write_text('# agents') + (agents / '.env').write_text('SECRET=x') + + result = run_package(str(workspace)) + + assert result.returncode == 0, result.stderr + entries = names(result.stdout.strip()) + assert 'Resources/.agents/README.md' in entries + assert not any(part.startswith('.') for entry in entries + for part in entry.removeprefix('Resources/.agents/').split('/')) + assert f" skipped {PACKAGE}/content/.config/ (hidden: names starting with '.' are not packed)\n" in result.stderr + assert f" skipped {PACKAGE}/content/.env (hidden: " in result.stderr + assert 'skipped Resources/.agents/.env' in result.stderr + assert 'gh/hosts.yml' not in result.stderr + assert '.DS_Store' not in result.stderr + + +def test_follows_symlinked_data_dir(tmp_path): + workspace = make_workspace(tmp_path) + shared = tmp_path / 'shared' + shared.mkdir() + (shared / 'Sample.xls').write_bytes(b'\xd0\xcf\x11\xe0') + (workspace / 'Data').mkdir() + (workspace / 'Data' / 'Datasources').symlink_to(shared, target_is_directory=True) + (workspace / 'Data' / 'Datasources' / 'loop').symlink_to(workspace / 'Data', target_is_directory=True) + (workspace / 'Sales Demo.twb').write_text(TWB_WITH_XLS) + + result = run_package(str(workspace)) + + assert result.returncode == 0, result.stderr + entries = names(result.stdout.strip()) + assert 'Data/Datasources/Sample.xls' in entries + assert not any('/loop/' in entry for entry in entries) def test_skips_other_workspace_dirs(tmp_path): From cd5ca8f2f9c8502a962310d742a9c6716a5c4d41 Mon Sep 17 00:00:00 2001 From: Patrick Green Date: Thu, 22 Oct 2026 17:43:47 -0600 Subject: [PATCH 03/21] Check directory+filename Data/ refs in package_twbx.py CSV, JSON, and shapefile connections split the path across two attributes (directory='Data' filename='x.csv'), so the missing-file check only saw 'x.csv' and let a workbook with no embedded data package cleanly. Join directory and filename before checking. Excel and Hyper keep the whole path in one attribute and are unchanged. --- .../scripts/package_twbx.py | 7 ++++++- .../scripts/tests/test_package_twbx.py | 21 +++++++++++++++++++ 2 files changed, 27 insertions(+), 1 deletion(-) diff --git a/src/plugin/skills/tableau-data-app-authoring/scripts/package_twbx.py b/src/plugin/skills/tableau-data-app-authoring/scripts/package_twbx.py index 9c28459..5f17f56 100644 --- a/src/plugin/skills/tableau-data-app-authoring/scripts/package_twbx.py +++ b/src/plugin/skills/tableau-data-app-authoring/scripts/package_twbx.py @@ -39,7 +39,9 @@ HIDDEN_REASON = "(hidden: names starting with '.' are not packed)" -# Connection attributes that hold a workbook-relative file path. +# Connection attributes that hold a workbook-relative file path. Excel and Hyper +# put the whole path in one (filename='Data/x.xlsx'); CSV, JSON, and shapefile +# split it (directory='Data' filename='x.csv'). FILE_ATTRS = ('filename', 'dbname') @@ -65,8 +67,11 @@ def missing_data_files(workspace, twb): die(f'Could not parse {twb}: {error}') missing = set() for element in root.iter('connection'): + directory = element.get('directory', '').replace('\\', '/').rstrip('/') for attr in FILE_ATTRS: path = element.get(attr, '').replace('\\', '/') + if attr == 'filename' and directory and path: + path = f'{directory}/{path}' if path.startswith('Data/') and not os.path.isfile(os.path.join(workspace, path)): missing.add(path) return sorted(missing) diff --git a/src/plugin/skills/tableau-data-app-authoring/scripts/tests/test_package_twbx.py b/src/plugin/skills/tableau-data-app-authoring/scripts/tests/test_package_twbx.py index cc5d751..8f43319 100644 --- a/src/plugin/skills/tableau-data-app-authoring/scripts/tests/test_package_twbx.py +++ b/src/plugin/skills/tableau-data-app-authoring/scripts/tests/test_package_twbx.py @@ -106,6 +106,27 @@ def test_fails_when_twb_references_missing_data_file(tmp_path): assert not (tmp_path / 'Sales Demo.twbx').exists() +def test_checks_directory_plus_filename_connections(tmp_path): + # CSV, JSON, and shapefile connections split the path: directory='Data' filename='x.csv'. + workspace = make_workspace(tmp_path) + (workspace / 'Sales Demo.twb').write_text( + "" + "" + "") + + result = run_package(str(workspace)) + + assert result.returncode == 1 + assert 'Data/orders.csv' in result.stderr + + (workspace / 'Data').mkdir() + (workspace / 'Data' / 'orders.csv').write_text('a,b\n1,2\n') + result = run_package(str(workspace)) + + assert result.returncode == 0, result.stderr + assert 'Data/orders.csv' in names(result.stdout.strip()) + + def test_packs_only_agents_under_resources(tmp_path): workspace = make_workspace(tmp_path) agents = workspace / 'Resources' / '.agents' From a233bdbda1372938c9c74add80f7356f95362892 Mon Sep 17 00:00:00 2001 From: Patrick Green Date: Fri, 23 Oct 2026 13:09:24 -0600 Subject: [PATCH 04/21] @W-24630680: Wire embedded file datasources; skip empty Data/ when packaging The scaffold-data-app template now ships Data/.gitkeep and Resources/.agents/README.md, so the plugin no longer owns that layout. - package_twbx.py: pack a dir only when it holds a packed file, so the template's Data/ (only .gitkeep) stays out of the .twbx; skip .gitkeep silently. - wire_datasource.py: add "type": "file" descriptors for Excel (excel-direct) and CSV (textscan) files under Data/, wired as federated datasources alongside published ones in a single run. - SKILL.md: describe the template's Data/ and Resources/.agents/, the embedded file wiring steps, and the packaging rules. --- .../tableau-data-app-authoring/SKILL.md | 86 +++++-- .../scripts/package_twbx.py | 82 ++++--- .../scripts/tests/test_package_twbx.py | 63 +++++ .../scripts/tests/test_wire_datasource.py | 225 ++++++++++++++++++ .../scripts/wire_datasource.py | 176 ++++++++++++-- 5 files changed, 572 insertions(+), 60 deletions(-) diff --git a/src/plugin/skills/tableau-data-app-authoring/SKILL.md b/src/plugin/skills/tableau-data-app-authoring/SKILL.md index 9962ec2..93680d3 100644 --- a/src/plugin/skills/tableau-data-app-authoring/SKILL.md +++ b/src/plugin/skills/tableau-data-app-authoring/SKILL.md @@ -1,6 +1,6 @@ --- name: tableau-data-app-authoring -description: Build and publish a Tableau data app, a custom viz-extension web app that queries a published datasource live. Scaffolds the app with the scaffold-data-app MCP tool, writes its query and visualization from the user's criteria or vibe, then packages it as a .twbx and publishes it. Requires scaffold-data-app, which may be unavailable depending on the user's site role, site settings, or Tableau Server version; don't invoke this skill unless it is available. Use to create, build, vibe-code, or publish a data app. Not for native workbooks or dashboards built from marks, Show Me, or the chart catalog (tableau-workbook-authoring), or for opening existing content, including published data apps (tableau-content-viewer). +description: Build and publish a Tableau data app, a custom viz-extension web app that queries a published datasource live, or an Excel or CSV file embedded in the workbook. Scaffolds the app with the scaffold-data-app MCP tool, writes its query and visualization from the user's criteria or vibe, then packages it as a .twbx and publishes it. Requires scaffold-data-app, which may be unavailable depending on the user's site role, site settings, or Tableau Server version; don't invoke this skill unless it is available. Use to create, build, vibe-code, or publish a data app. Not for native workbooks or dashboards built from marks, Show Me, or the chart catalog (tableau-workbook-authoring), or for opening existing content, including published data apps (tableau-content-viewer). --- # Tableau Data App Authoring @@ -160,8 +160,13 @@ At the end of this stage you have a finalized workspace directory: content/src/app.js ← the authoring surface content/src/styles.css content/src/tableau.extensions.1.latest.min.js + Data/.gitkeep ← placeholder; embedded datasource files go here + Resources/.agents/README.md ← which agent docs travel with the workbook ``` +Keep the `Resources/.agents/README.md` as shipped and follow it for what else +to put in that folder. + **Sheet name:** the worksheet is named after the app's display name. It's a plain string with no hash/join-key involved, so it's freely hand-editable at any point before packaging — edit all 3 matching locations in the `.twb` to @@ -181,10 +186,15 @@ user only wants to publish the starter to prove packaging. Only add, remove, or swap datasources the user asked for; if you think a different or extra datasource would help, ask first. -The app queries a datasource that already exists on the server (a `sqlproxy` -connection, resolved live by VDS). Its identity can come straight from the -user, or be reused from a datasource already wired into some other existing -workbook — see Reuse an existing workbook's datasource below. +The app queries either kind of datasource, in any mix: + +- **Published:** a datasource that already exists on the server (a `sqlproxy` + connection, resolved live by VDS). Its identity can come straight from the + user, or be reused from a datasource already wired into some other existing + workbook (see Reuse an existing workbook's datasource below). +- **Embedded file:** an Excel sheet or CSV file the user supplies, copied into + the workspace's `Data/` folder and packed into the `.twbx` (see Embedded + file datasource below). Do **not** hand-edit the XML — the anchors must be filled atomically with a matching join key across multiple coordinated locations, or the app silently @@ -263,6 +273,39 @@ Trust that failure over patching the XML by hand. `datatype` maps to the column `type` (`real`/`integer` → quantitative, `date`/`datetime` → ordinal, else nominal); `role: "measure"` gets a `Sum` aggregation, `dimension` a `Count`. +**Embedded file datasource:** use this when the user hands you an Excel +(`.xls`, `.xlsx`, `.xlsm`) or CSV file instead of naming a published +datasource. The same script wires it; the descriptor just has `"type": "file"`. + +1. **Copy the file into the workspace** under `Data/`, conventionally + `Data/Datasources/`. The script fails if the file isn't there, or if + its path is outside `Data/`. +2. **Write a descriptor.** `file` is the path relative to the `.twb`. For + Excel, `table` is the sheet to query (sheet `Orders` → `"table": "Orders"`); + CSV has no `table`. Field names must match the sheet's or file's column + headers exactly. No server, site, or `repositoryId`: + + ```bash + cat > "$WORK/descriptor.json" <<'DS_JSON' + { + "type": "file", + "caption": "Sample - Superstore", + "file": "Data/Datasources/Sample - Superstore.xls", + "table": "Orders", + "fields": [ + { "name": "Sales", "datatype": "real", "role": "measure" }, + { "name": "Region", "datatype": "string", "role": "dimension" } + ] + } + DS_JSON + ``` + + To mix it with published datasources, put every entry in one + `datasources` array (primary first) as above. +3. **Run the wiring script** the same way as for a published datasource. It + generates a `federated.` join key unless you supply `connectionName` + (which must then start with `federated.`). + **Reuse an existing workbook's datasource:** point the Published path above at a datasource already wired into some *other* already-published workbook, instead of a fresh published DS. Only applies when that datasource actually @@ -274,8 +317,10 @@ have produced). if ambiguous), then call `get-workbook({ workbookId })` and read `upstreamDatasources[]` — ask which entry if it lists more than one. 2. Check the matched entry's `datasourceType`: - - **`"embedded"`** — it can't be reused via this path; ask the user for a - published datasource instead. + - **`"embedded"`**: its data lives inside that workbook, so it can't be + reused by reference. Ask the user for either a published datasource or + the source Excel/CSV file, and wire the file as an Embedded file + datasource above. - **`"published"`** — still run the rest of Published datasource step 1 (`list-datasources` filtered by this LUID, for `contentUrl`/site/server — `get-workbook` doesn't carry those — plus @@ -328,13 +373,17 @@ hand: python3 "$SKILL_DIR/scripts/package_twbx.py" "" # the finalized workspace dir ``` -It also packs `Data/` and `Resources/.agents/` at the archive root when the -workspace has them, so a `.twb` that references an embedded datasource file by -relative path (e.g. `Data/Datasources/.xls`) keeps its data. It fails if -a `Data/` file the `.twb` references is missing. Nothing else at the workspace -root or under `Resources/` is packed, and below those dirs any file or folder -whose name starts with `.` is skipped, along with everything inside it. Each -one is listed on stderr with the reason; relay those lines to the user. +It also packs `Data/` and `Resources/.agents/` at the archive root, so a +`.twb` that references an embedded datasource file by relative path (e.g. +`Data/Datasources/.xls`) keeps its data. A folder is packed only when it +holds at least one file to pack: the template's `Data/`, with nothing but its +`.gitkeep`, is left out of the `.twbx` (expected when no file is embedded). +It fails if a `Data/` file the `.twb` references is missing. Nothing else at +the workspace root or under `Resources/` is packed. Below those folders, +`.gitkeep` placeholders and OS cruft (`.DS_Store`, `__MACOSX`) are skipped +silently, and any other file or folder whose name starts with `.` is skipped, +along with everything inside it, and listed on stderr as ` skipped +()`; relay those lines to the user. It writes `.twbx` next to the workspace dir (pass a second arg to override), overwriting any existing file, and prints the path for Publish. @@ -386,7 +435,8 @@ Uses the MCP publish tools (not available to Slack clients). source (the normal case after `wire_datasource.py`, or when reusing one), with its published name from `list-datasources` as `` (not the descriptor's caption, which can differ). Repeat it once per wired data - source. Omit it when no data source was wired. Do no extra permission lookups. Relay its stdout to the user + source. Omit it for embedded file datasources and when no data source was + wired. Do no extra permission lookups. Relay its stdout to the user verbatim. If `publish-workbook` returns `status: "invalid"` (or an error), surface the @@ -403,7 +453,11 @@ Uses the MCP publish tools (not available to Slack clients). - Don't zip the `.twbx` freehand. Use `scripts/package_twbx.py` — it keeps `.twb`, `Packages/`, and any `Data/`/`Resources/.agents/` at the archive root, never - nested under `/`. + nested under `/`, and leaves out a `Data/` that holds only its + `.gitkeep`. +- Don't reference an embedded file from outside `Data/` or leave it out of the + workspace. `wire_datasource.py` and `package_twbx.py` both refuse, and a + `.twbx` without the file publishes but has no data at runtime. - Don't apply a postUnzip plan freehand. Use `scripts/apply_plan.py` — edits before renames, renames deepest-first, verified; see its header comment for the full contract. diff --git a/src/plugin/skills/tableau-data-app-authoring/scripts/package_twbx.py b/src/plugin/skills/tableau-data-app-authoring/scripts/package_twbx.py index 5f17f56..41732f4 100644 --- a/src/plugin/skills/tableau-data-app-authoring/scripts/package_twbx.py +++ b/src/plugin/skills/tableau-data-app-authoring/scripts/package_twbx.py @@ -7,15 +7,20 @@ Data/... (optional: embedded datasource files the .twb references) Resources/.agents/... (optional: agent docs that travel with the workbook) +The scaffold-data-app template ships `Data/` holding only a `.gitkeep` +placeholder and `Resources/.agents/README.md`. + The archive holds the `.twb` (written first), `Packages/`, and any `Data/` and `Resources/.agents/` at its ROOT, never nested under `/` — nesting fails to open on publish with `PackageValidationException: Package directory contains no extension .trex files under extensions/`. Without `Data/`, a `.twb` that references an embedded file by relative path publishes fine but has no data at runtime. Nothing else -under `Resources/` is packed. OS cruft (`.DS_Store`, `__MACOSX`) and any -file or dir whose name starts with `.` below those dirs are skipped; hidden -ones are listed on stderr as ` skipped ()`. Symlinked files and -dirs are followed. +under `Resources/` is packed. A dir is packed only when it holds at least one +packed file, so a `Data/` with nothing but its `.gitkeep` is left out. OS cruft +(`.DS_Store`, `__MACOSX`) and `.gitkeep` placeholders are skipped silently; any +other file or dir whose name starts with `.` below those dirs is skipped and +listed on stderr as ` skipped ()`. Symlinked files and dirs are +followed. Hard-fails unless the workspace root has exactly one `.twb`, a `Packages/` dir, at least one `.trex` under `Packages/*/extensions/`, and every `Data/` file the @@ -39,6 +44,9 @@ HIDDEN_REASON = "(hidden: names starting with '.' are not packed)" +# Keeps an otherwise-empty dir (the template's Data/) in git; never packed or reported. +PLACEHOLDER = '.gitkeep' + # Connection attributes that hold a workbook-relative file path. Excel and Hyper # put the whole path in one (filename='Data/x.xlsx'); CSV, JSON, and shapefile # split it (directory='Data' filename='x.csv'). @@ -60,6 +68,42 @@ def is_hidden(rel_path, top): return any(part.startswith('.') for part in below.split('/') if part) +# Walk one packed dir and return its (path, archive name) entries in walk order, +# reporting hidden skips on stderr. Dirs are kept only on the way to a packed +# file, so a dir left with no files (an empty Data/) isn't packed at all. +def collect_entries(workspace, top): + entries = [] # (path, archive name, is_dir) + visited = set() + for root, dirs, files in os.walk(os.path.join(workspace, top), followlinks=True): + dirs.sort() + rel_root = os.path.relpath(root, workspace).replace(os.sep, '/') + real_root = os.path.realpath(root) + if is_cruft(rel_root) or real_root in visited: + dirs[:] = [] + continue + visited.add(real_root) + entries.append((root, rel_root, True)) + # Prune hidden dirs here so each is reported once, not per file inside. + for name in list(dirs): + rel = f'{rel_root}/{name}' + if not is_cruft(rel) and is_hidden(rel, top): + print(f' skipped {rel}/ {HIDDEN_REASON}', file=sys.stderr) + dirs.remove(name) + for name in sorted(files): + rel = f'{rel_root}/{name}' + if is_cruft(rel) or name == PLACEHOLDER: + continue + if is_hidden(rel, top): + print(f' skipped {rel} {HIDDEN_REASON}', file=sys.stderr) + continue + entries.append((os.path.join(root, name), rel, False)) + + # Every dir that is an ancestor of a packed file. + needed_dirs = {rel.rsplit('/', i)[0] for _, rel, is_dir in entries if not is_dir + for i in range(1, rel.count('/') + 1)} + return [(path, rel) for path, rel, is_dir in entries if not is_dir or rel in needed_dirs] + + def missing_data_files(workspace, twb): try: root = ET.parse(os.path.join(workspace, twb)).getroot() @@ -108,36 +152,16 @@ def main(): output = os.path.abspath(argv[2]) if len(argv) == 3 else os.path.join( os.path.dirname(workspace), os.path.basename(workspace) + '.twbx') - dirs_to_pack = [top for top in PACKED_DIRS if os.path.isdir(os.path.join(workspace, top))] + entries_by_top = {top: collect_entries(workspace, top) + for top in PACKED_DIRS if os.path.isdir(os.path.join(workspace, top))} + dirs_to_pack = [top for top, entries in entries_by_top.items() if entries] # Paths are workspace-relative so nothing lands under an / prefix. with zipfile.ZipFile(output, 'w', zipfile.ZIP_DEFLATED) as archive: archive.write(os.path.join(workspace, twb), twb) - visited = set() for top in dirs_to_pack: - for root, dirs, files in os.walk(os.path.join(workspace, top), followlinks=True): - dirs.sort() - rel_root = os.path.relpath(root, workspace).replace(os.sep, '/') - real_root = os.path.realpath(root) - if is_cruft(rel_root) or real_root in visited: - dirs[:] = [] - continue - visited.add(real_root) - archive.write(root, rel_root) - # Prune hidden dirs here so each is reported once, not per file inside. - for name in list(dirs): - rel = f'{rel_root}/{name}' - if not is_cruft(rel) and is_hidden(rel, top): - print(f' skipped {rel}/ {HIDDEN_REASON}', file=sys.stderr) - dirs.remove(name) - for name in sorted(files): - rel = f'{rel_root}/{name}' - if is_cruft(rel): - continue - if is_hidden(rel, top): - print(f' skipped {rel} {HIDDEN_REASON}', file=sys.stderr) - continue - archive.write(os.path.join(root, name), rel) + for path, rel in entries_by_top[top]: + archive.write(path, rel) packed = ', '.join(f'{top}/' for top in dirs_to_pack) print(f'✓ Packaged {twb} + {packed} into {output}', file=sys.stderr) diff --git a/src/plugin/skills/tableau-data-app-authoring/scripts/tests/test_package_twbx.py b/src/plugin/skills/tableau-data-app-authoring/scripts/tests/test_package_twbx.py index 8f43319..7ded003 100644 --- a/src/plugin/skills/tableau-data-app-authoring/scripts/tests/test_package_twbx.py +++ b/src/plugin/skills/tableau-data-app-authoring/scripts/tests/test_package_twbx.py @@ -33,6 +33,14 @@ def make_workspace(tmp_path, with_trex=True): return workspace +def add_template_dirs(workspace): + # The scaffold-data-app template ships Data/ with only a placeholder, plus the agent README. + (workspace / 'Data').mkdir() + (workspace / 'Data' / '.gitkeep').write_text('') + (workspace / 'Resources' / '.agents').mkdir(parents=True) + (workspace / 'Resources' / '.agents' / 'README.md').write_text('# Resources/.agents/') + + def names(twbx_path): with zipfile.ZipFile(twbx_path) as archive: return archive.namelist() @@ -94,6 +102,57 @@ def test_packs_data_at_root(tmp_path): assert '✓ Packaged Sales Demo.twb + Packages/, Data/ into' in result.stderr +def test_template_data_dir_with_only_placeholder_is_not_packed(tmp_path): + workspace = make_workspace(tmp_path) + add_template_dirs(workspace) + + result = run_package(str(workspace)) + + assert result.returncode == 0, result.stderr + entries = names(result.stdout.strip()) + assert not any(entry.startswith('Data') for entry in entries) + assert 'Resources/.agents/README.md' in entries + assert 'gitkeep' not in result.stderr + assert '✓ Packaged Sales Demo.twb + Packages/, Resources/.agents/ into' in result.stderr + + +def test_placeholder_is_skipped_silently_next_to_data_files(tmp_path): + workspace = make_workspace(tmp_path) + add_template_dirs(workspace) + (workspace / 'Data' / 'Datasources').mkdir() + (workspace / 'Data' / 'Datasources' / 'Sample.xls').write_bytes(b'\xd0\xcf\x11\xe0') + (workspace / 'Sales Demo.twb').write_text(TWB_WITH_XLS) + + result = run_package(str(workspace)) + + assert result.returncode == 0, result.stderr + entries = names(result.stdout.strip()) + assert 'Data/Datasources/Sample.xls' in entries + assert not any('gitkeep' in entry for entry in entries) + assert 'gitkeep' not in result.stderr + assert '✓ Packaged Sales Demo.twb + Packages/, Data/, Resources/.agents/ into' in result.stderr + + +def test_dirs_without_packed_files_are_left_out(tmp_path): + workspace = make_workspace(tmp_path) + data = workspace / 'Data' + (data / 'Datasources').mkdir(parents=True) + (data / 'Datasources' / 'Sample.xls').write_bytes(b'\xd0\xcf\x11\xe0') + (data / 'Extracts').mkdir() + (data / 'Scratch').mkdir() + (data / 'Scratch' / '.gitkeep').write_text('') + (workspace / 'Sales Demo.twb').write_text(TWB_WITH_XLS) + + result = run_package(str(workspace)) + + assert result.returncode == 0, result.stderr + entries = names(result.stdout.strip()) + assert 'Data/' in entries + assert 'Data/Datasources/' in entries + assert 'Data/Datasources/Sample.xls' in entries + assert not any(entry.startswith(('Data/Extracts', 'Data/Scratch')) for entry in entries) + + def test_fails_when_twb_references_missing_data_file(tmp_path): workspace = make_workspace(tmp_path) (workspace / 'Sales Demo.twb').write_text(TWB_WITH_XLS) @@ -174,6 +233,10 @@ def test_skips_hidden_entries_below_packed_dirs(tmp_path): assert 'skipped Resources/.agents/.env' in result.stderr assert 'gh/hosts.yml' not in result.stderr assert '.DS_Store' not in result.stderr + # Data/ held only a hidden file, so it's reported but Data/ itself isn't packed. + assert 'skipped Data/.env' in result.stderr + assert not any(entry.startswith('Data') for entry in entries) + assert '✓ Packaged Sales Demo.twb + Packages/, Resources/.agents/ into' in result.stderr def test_follows_symlinked_data_dir(tmp_path): diff --git a/src/plugin/skills/tableau-data-app-authoring/scripts/tests/test_wire_datasource.py b/src/plugin/skills/tableau-data-app-authoring/scripts/tests/test_wire_datasource.py index eedab16..68c5766 100644 --- a/src/plugin/skills/tableau-data-app-authoring/scripts/tests/test_wire_datasource.py +++ b/src/plugin/skills/tableau-data-app-authoring/scripts/tests/test_wire_datasource.py @@ -382,3 +382,228 @@ def test_default_site_omits_site_path_and_attribute(tmp_path): wired = open(twb_path).read() assert "" in wired assert 'site=' not in wired + + +# --- embedded file datasources ------------------------------------------------ + +XLS_PATH = 'Data/Datasources/Sample - Superstore.xls' + + +def add_data_file(tmp_path, rel=XLS_PATH, content=b'\xd0\xcf\x11\xe0'): + path = tmp_path / rel + path.parent.mkdir(parents=True, exist_ok=True) + path.write_bytes(content) + + +def excel_descriptor(**overrides): + ds = { + "type": "file", + "caption": "Sample - Superstore", + "file": XLS_PATH, + "table": "Orders", + "connectionName": "federated.abc123", + "fields": [ + {"name": "Sales", "datatype": "real", "role": "measure"}, + {"name": "Region", "datatype": "string", "role": "dimension"}, + {"name": "Order Date", "datatype": "date", "role": "dimension"}, + ], + } + ds.update(overrides) + return ds + + +def csv_descriptor(**overrides): + ds = { + "type": "file", + "caption": "Orders CSV", + "file": "Data/orders.csv", + "connectionName": "federated.csv1", + "fields": [ + {"name": "Profit", "datatype": "real", "role": "measure"}, + {"name": "Ship Date", "datatype": "date", "role": "dimension"}, + ], + } + ds.update(overrides) + return ds + + +def test_excel_file_wires_federated_datasource_with_matching_join_keys(tmp_path): + add_data_file(tmp_path) + twb_path = write_twb(tmp_path) + result = run_wire(twb_path, write_descriptor(tmp_path, excel_descriptor())) + + assert result.returncode == 0, result.stderr + assert result.stdout.strip() == twb_path + assert "✓ Wired datasource 'Sample - Superstore' (federated.abc123) with 3 field(s)" in result.stderr + + wired = open(twb_path).read() + root, view = view_of(wired) + root_ds = root.find('./datasources/datasource') + assert root_ds.get('name') == 'federated.abc123' + assert root_ds.get('caption') == 'Sample - Superstore' + assert root_ds.find('./connection').get('class') == 'federated' + named = root_ds.find('./connection/named-connections/named-connection') + assert named.get('name') == 'excel-direct.abc123' + file_connection = named.find('./connection') + assert file_connection.get('class') == 'excel-direct' + assert file_connection.get('filename') == XLS_PATH + relation = root_ds.find('./connection/relation') + assert relation.attrib == {'connection': 'excel-direct.abc123', 'name': 'Orders', 'table': '[Orders$]', 'type': 'table'} + records = {r.findtext('remote-name'): r for r in root_ds.iter('metadata-record')} + assert records['Sales'].findtext('remote-type') == '5' + assert records['Region'].findtext('remote-type') == '130' + assert records['Order Date'].findtext('remote-type') == '7' + assert {r.findtext('parent-name') for r in records.values()} == {'[Orders]'} + assert 'sqlproxy' not in wired + + # The view half is the same as for a published datasource. + assert [d.get('name') for d in view.findall('./datasources/datasource')] == ['federated.abc123'] + assert [d.get('datasource') for d in view.findall('./datasource-dependencies')] == ['federated.abc123'] + assert "" in wired + assert "" in wired + + +def test_csv_file_splits_directory_and_filename(tmp_path): + add_data_file(tmp_path, 'Data/orders.csv', b'Profit,Ship Date\n1.5,2026-01-31\n') + twb_path = write_twb(tmp_path) + result = run_wire(twb_path, write_descriptor(tmp_path, csv_descriptor())) + + assert result.returncode == 0, result.stderr + root, _ = view_of(open(twb_path).read()) + root_ds = root.find('./datasources/datasource') + named = root_ds.find('./connection/named-connections/named-connection') + assert named.get('name') == 'textscan.csv1' + assert named.find('./connection').attrib == { + 'class': 'textscan', 'directory': 'Data', 'filename': 'orders.csv', 'password': '', 'server': ''} + relation = root_ds.find('./connection/relation') + assert relation.get('name') == 'orders.csv' + assert relation.get('table') == '[orders#csv]' + records = {r.findtext('remote-name'): r for r in root_ds.iter('metadata-record')} + assert records['Profit'].findtext('remote-type') == '5' + assert records['Ship Date'].findtext('remote-type') == '133' + assert {r.findtext('parent-name') for r in records.values()} == {'[orders.csv]'} + + +def test_published_and_file_datasources_wire_in_one_run(tmp_path): + add_data_file(tmp_path) + twb_path = write_twb(tmp_path) + descriptor = {"datasources": [descriptor_happy(), excel_descriptor()]} + result = run_wire(twb_path, write_descriptor(tmp_path, descriptor)) + + assert result.returncode == 0, result.stderr + wired = open(twb_path).read() + root, view = view_of(wired) + assert [d.get('name') for d in root.findall('./datasources/datasource')] == ['sqlproxy.abc123', 'federated.abc123'] + assert [d.get('name') for d in view.findall('./datasources/datasource')] == ['sqlproxy.abc123', 'federated.abc123'] + # The published primary alone carries datasource-dependencies. + assert [d.get('datasource') for d in view.findall('./datasource-dependencies')] == ['sqlproxy.abc123'] + + +def test_generated_file_connection_name_shares_its_hash_with_the_named_connection(tmp_path): + add_data_file(tmp_path) + twb_path = write_twb(tmp_path) + result = run_wire(twb_path, write_descriptor(tmp_path, excel_descriptor(connectionName=None))) + + assert result.returncode == 0, result.stderr + root, _ = view_of(open(twb_path).read()) + root_ds = root.find('./datasources/datasource') + name = root_ds.get('name') + assert re.fullmatch(r'federated\.[a-z0-9]{22}', name) + expected = 'excel-direct.' + name.split('.', 1)[1] + assert root_ds.find('./connection/named-connections/named-connection').get('name') == expected + assert root_ds.find('./connection/relation').get('connection') == expected + + +def test_file_missing_from_the_workspace_is_rejected(tmp_path): + twb_path = write_twb(tmp_path) + result = run_wire(twb_path, write_descriptor(tmp_path, excel_descriptor())) + + assert result.returncode == 1 + assert result.stderr.strip() == ( + f"✗ Datasource 'Sample - Superstore' file \"{XLS_PATH}\" is not in the workspace ({tmp_path}). " + 'Copy it there first, at that path relative to the .twb.' + ) + assert open(twb_path).read() == TWB_TEMPLATE + + +def test_file_outside_data_folder_is_rejected(tmp_path): + twb_path = write_twb(tmp_path) + for bad in ('orders.csv', 'Data/../orders.csv', '/abs/Data/orders.csv', 'Resources/orders.csv'): + result = run_wire(twb_path, write_descriptor(tmp_path, csv_descriptor(file=bad))) + assert result.returncode == 1 + assert '"file" must be a path inside the workspace Data/ folder' in result.stderr + assert open(twb_path).read() == TWB_TEMPLATE + + +def test_unsupported_file_type_is_rejected(tmp_path): + twb_path = write_twb(tmp_path) + result = run_wire(twb_path, write_descriptor(tmp_path, csv_descriptor(file='Data/orders.json'))) + + assert result.returncode == 1 + assert result.stderr.strip() == '✗ "file" must be an Excel (.xls, .xlsx, .xlsm) or CSV (.csv) file (got "Data/orders.json").' + + +def test_excel_requires_a_table_and_csv_rejects_one(tmp_path): + twb_path = write_twb(tmp_path) + descriptor = excel_descriptor() + del descriptor['table'] + result = run_wire(twb_path, write_descriptor(tmp_path, descriptor)) + assert result.returncode == 1 + assert result.stderr.strip() == '✗ Excel datasources need "table": the name of the sheet to query.' + + result = run_wire(twb_path, write_descriptor(tmp_path, csv_descriptor(table='orders'))) + assert result.returncode == 1 + assert result.stderr.strip() == '✗ "table" applies only to Excel files; a CSV file is its own table.' + + +def test_file_connection_name_must_start_with_federated_prefix(tmp_path): + add_data_file(tmp_path) + twb_path = write_twb(tmp_path) + result = run_wire(twb_path, write_descriptor(tmp_path, excel_descriptor(connectionName='sqlproxy.abc'))) + + assert result.returncode == 1 + assert result.stderr.strip() == ( + '✗ connectionName for a file datasource must start with "federated." (got "sqlproxy.abc").') + + +def test_unknown_type_is_rejected(tmp_path): + twb_path = write_twb(tmp_path) + result = run_wire(twb_path, write_descriptor(tmp_path, excel_descriptor(type='extract'))) + + assert result.returncode == 1 + assert result.stderr.strip() == '✗ Descriptor "type" must be "published" or "file" (got "extract").' + + +def test_rewiring_a_file_wired_twb_is_rejected(tmp_path): + add_data_file(tmp_path) + twb_path = write_twb(tmp_path) + desc_path = write_descriptor(tmp_path, excel_descriptor()) + assert run_wire(twb_path, desc_path).returncode == 0 + + result = run_wire(twb_path, desc_path) + + assert result.returncode == 1 + assert '✗ This .twb is already wired.' in result.stderr + + +def test_wired_file_twb_packages_with_its_data(tmp_path): + # wire_datasource.py and package_twbx.py agree on where the file lives. + import zipfile + workspace = tmp_path / 'Sales Demo' + ext_dir = workspace / 'Packages' / 'com.tableau.mcp.sales-demo' / 'extensions' + ext_dir.mkdir(parents=True) + (ext_dir / 'data-app.trex').write_text('') + add_data_file(workspace) + add_data_file(workspace, 'Data/orders.csv', b'Profit,Ship Date\n') + twb_path = write_twb(workspace, filename='Sales Demo.twb') + descriptor = {"datasources": [excel_descriptor(), csv_descriptor()]} + assert run_wire(twb_path, write_descriptor(tmp_path, descriptor)).returncode == 0 + + package = os.path.join(os.path.dirname(SCRIPT), 'package_twbx.py') + result = subprocess.run([sys.executable, package, str(workspace)], capture_output=True, text=True) + + assert result.returncode == 0, result.stderr + with zipfile.ZipFile(result.stdout.strip()) as archive: + entries = archive.namelist() + assert XLS_PATH in entries + assert 'Data/orders.csv' in entries diff --git a/src/plugin/skills/tableau-data-app-authoring/scripts/wire_datasource.py b/src/plugin/skills/tableau-data-app-authoring/scripts/wire_datasource.py index 8ff49bb..3793762 100644 --- a/src/plugin/skills/tableau-data-app-authoring/scripts/wire_datasource.py +++ b/src/plugin/skills/tableau-data-app-authoring/scripts/wire_datasource.py @@ -1,12 +1,17 @@ #!/usr/bin/env python3 """ -Wire one or more published datasources into a scaffolded data-app `.twb`. +Wire one or more datasources into a scaffolded data-app `.twb`: published ones +and files embedded in the workspace's `Data/` folder, in any mix. `scaffold-data-app` emits a workbook with TWO empty `` anchors (workbook root + worksheet ``). Until both are filled the extension's `getAllDataSourcesAsync()` finds nothing and renders "no data source found in -the workbook." This fills both with N published `sqlproxy` (Data Server) -datasources, keeping each `sqlproxy.` join key byte-identical everywhere. +the workbook." This fills both in one run with N datasources: + - published: a `sqlproxy` (Data Server) connection, joined by `sqlproxy.`. + - file: a `federated` datasource over one Excel sheet (`excel-direct`) or CSV + file (`textscan`) at a workspace-relative `Data/...` path, joined by + `federated.`; its named connection is `.` (same hash). +Each join key stays byte-identical everywhere it appears. A script, not freehand XML, because the wiring spans coordinated locations that must agree exactly (root datasource `name`, root `relation connection`, view @@ -36,8 +41,18 @@ { "name": "Region", "datatype": "string", "role": "dimension" } ] } -or any number of them, primary first, wired in a single run: +or an embedded file (the file must already be in the workspace): + { + "type": "file", + "caption": "Sample - Superstore", + "file": "Data/Datasources/Sample - Superstore.xls", // relative to the .twb; .xls, .xlsx, .xlsm, or .csv + "table": "Orders", // Excel only: the sheet to query + "connectionName": "federated.", // optional, generated if omitted + "fields": [ ...as above... ] + } +or any number of either, primary first, wired in a single run: { "datasources": [ { ...as above... }, { ...as above... } ] } +`type` defaults to "published". """ import json @@ -50,6 +65,15 @@ # The exact empty anchor emitted by the scaffold template. Matched literally. EMPTY_ANCHOR = '' +# Embedded file extension -> connection class. +FILE_CLASSES = {'.xls': 'excel-direct', '.xlsx': 'excel-direct', '.xlsm': 'excel-direct', '.csv': 'textscan'} + +# local-type -> the remote-type code Tableau records for each file class. +FILE_REMOTE_TYPES = { + 'excel-direct': {'string': 130, 'integer': 20, 'real': 5, 'date': 7, 'datetime': 7, 'boolean': 11}, + 'textscan': {'string': 129, 'integer': 20, 'real': 5, 'date': 133, 'datetime': 135, 'boolean': 11}, +} + def die(message): print(f'✗ {message}', file=sys.stderr) @@ -106,15 +130,27 @@ def derive_field(field, ordinal, error_prefix=''): } -def generate_connection_name(): +def generate_connection_name(prefix): token = lambda: ''.join(random.choices(string.ascii_lowercase + string.digits, k=11)) - return f'sqlproxy.{token()}{token()}' + return f'{prefix}.{token()}{token()}' + + +def parse_fields(descriptor, error_prefix): + fields_in = descriptor.get('fields') if isinstance(descriptor.get('fields'), list) else [] + if len(fields_in) == 0: + die(f'{error_prefix}Descriptor "fields" must list at least one field the app will query.') + return [derive_field(f, i, error_prefix) for i, f in enumerate(fields_in)] # Validate one datasource descriptor. `error_prefix` names the datasource in errors when wiring several. def parse_datasource(descriptor, error_prefix=''): if not isinstance(descriptor, dict): die(f'{error_prefix}Descriptor must be a JSON object.') + kind = descriptor.get('type', 'published') + if kind == 'file': + return parse_file_datasource(descriptor, error_prefix) + if kind != 'published': + die(f'{error_prefix}Descriptor "type" must be "published" or "file" (got {json.dumps(kind)}).') for key in ('caption', 'repositoryId', 'site', 'server'): value = descriptor.get(key) # site may be "" (the Default site); everything else must be non-empty. @@ -123,22 +159,59 @@ def parse_datasource(descriptor, error_prefix=''): channel = descriptor.get('channel') or 'https' port = descriptor.get('port') if descriptor.get('port') is not None else (443 if channel == 'https' else 80) - fields_in = descriptor.get('fields') if isinstance(descriptor.get('fields'), list) else [] - if len(fields_in) == 0: - die(f'{error_prefix}Descriptor "fields" must list at least one field the app will query.') + fields = parse_fields(descriptor, error_prefix) connection_name = descriptor.get('connectionName') if connection_name and not connection_name.startswith('sqlproxy.'): die(f'{error_prefix}connectionName must start with "sqlproxy." (got "{connection_name}").') return { + 'type': 'published', 'caption': descriptor['caption'], 'repositoryId': descriptor['repositoryId'], 'site': descriptor['site'], 'server': descriptor['server'], 'channel': channel, 'port': port, - 'fields': [derive_field(f, i, error_prefix) for i, f in enumerate(fields_in)], + 'fields': fields, + 'connectionName': connection_name, + } + + +def parse_file_datasource(descriptor, error_prefix): + for key in ('caption', 'file'): + value = descriptor.get(key) + if not isinstance(value, str) or not value: + die(f'{error_prefix}Descriptor is missing required string "{key}".') + file_path = descriptor['file'].replace('\\', '/') + parts = file_path.split('/') + if len(parts) < 2 or parts[0] != 'Data' or any(part in ('', '.', '..') for part in parts): + die(f'{error_prefix}"file" must be a path inside the workspace Data/ folder, ' + f'e.g. "Data/Datasources/Sales.xlsx" (got "{descriptor["file"]}").') + file_class = FILE_CLASSES.get(os.path.splitext(file_path)[1].lower()) + if not file_class: + die(f'{error_prefix}"file" must be an Excel (.xls, .xlsx, .xlsm) or CSV (.csv) file (got "{file_path}").') + + table = descriptor.get('table') + if file_class == 'excel-direct': + if not isinstance(table, str) or not table: + die(f'{error_prefix}Excel datasources need "table": the name of the sheet to query.') + elif table is not None: + die(f'{error_prefix}"table" applies only to Excel files; a CSV file is its own table.') + + fields = parse_fields(descriptor, error_prefix) + + connection_name = descriptor.get('connectionName') + if connection_name and not connection_name.startswith('federated.'): + die(f'{error_prefix}connectionName for a file datasource must start with "federated." (got "{connection_name}").') + + return { + 'type': 'file', + 'caption': descriptor['caption'], + 'file': file_path, + 'class': file_class, + 'table': table, + 'fields': fields, 'connectionName': connection_name, } @@ -158,6 +231,8 @@ def parse_descriptor(descriptor): seen_repository_ids = set() for i, ds in enumerate(datasources): + if ds['type'] != 'published': + continue if ds['repositoryId'] in seen_repository_ids: die(f'datasources[{i}]: repositoryId "{ds["repositoryId"]}" is listed more than once.') seen_repository_ids.add(ds['repositoryId']) @@ -177,11 +252,18 @@ def parse_descriptor(descriptor): used = set(explicit_names) for ds in datasources: if not ds['connectionName']: - name = generate_connection_name() + prefix = 'sqlproxy' if ds['type'] == 'published' else 'federated' + name = generate_connection_name(prefix) while name in used: - name = generate_connection_name() + name = generate_connection_name(prefix) ds['connectionName'] = name used.add(name) + # The connection the root relation points at: itself for sqlproxy, the + # named file connection (same hash) for federated. + if ds['type'] == 'published': + ds['relationConnection'] = ds['connectionName'] + else: + ds['relationConnection'] = f"{ds['class']}.{ds['connectionName'].split('.', 1)[1]}" return datasources @@ -193,6 +275,58 @@ def repository_location_xml(repository_id, site): def root_datasource_xml(ds): + return published_datasource_xml(ds) if ds['type'] == 'published' else file_datasource_xml(ds) + + +# Tableau bracket-quotes table names, doubling any `]` inside. +def bracket(name): + return '[' + name.replace(']', ']]') + ']' + + +def file_datasource_xml(ds): + if ds['class'] == 'excel-direct': + # A sheet named Orders is the table [Orders$]. + relation_name, table = ds['table'], bracket(f"{ds['table']}$") + connection = (f"") + else: + # CSV splits the path, and orders.csv is the table [orders#csv]. + directory, filename = ds['file'].rsplit('/', 1) + stem, extension = filename.rsplit('.', 1) + relation_name, table = filename, bracket(f'{stem}#{extension}') + connection = (f"") + remote_types = FILE_REMOTE_TYPES[ds['class']] + metadata_records = '\n'.join( + f""" + {esc(f['name'])} + {remote_types.get(f['datatype'], remote_types['string'])} + {esc(f['localName'])} + {esc(bracket(relation_name))} + {esc(f['name'])} + {f['ordinal']} + {esc(f['datatype'])} + {f['aggregation']} + true + """ + for f in ds['fields'] + ) + return f""" + + + + {connection} + + + + +{metadata_records} + + + """ + + +def published_datasource_xml(ds): metadata_records = '\n'.join( f""" {esc(f['name'])} @@ -254,8 +388,12 @@ def verify_wiring(wired, datasources): root_ds = root_datasources.get(name) if root_ds is None: die(f'Root is missing {name} — wiring incomplete.') - if root_ds.find(f".//relation[@connection='{name}']") is None: - die(f'Root datasource {name} has no relation connection to itself — wiring incomplete.') + relation_connection = ds['relationConnection'] + if root_ds.find(f".//relation[@connection='{relation_connection}']") is None: + target = 'itself' if relation_connection == name else relation_connection + die(f'Root datasource {name} has no relation connection to {target} — wiring incomplete.') + if ds['type'] == 'file' and root_ds.find(f".//named-connection[@name='{relation_connection}']") is None: + die(f'Root datasource {name} has no named connection {relation_connection} — wiring incomplete.') # The server only connects datasources listed on the host sheet's view. host_view = next( @@ -290,6 +428,14 @@ def main(): datasources = parse_descriptor(descriptor) primary = datasources[0] + # An embedded file the .twb points at must already sit in the workspace, or + # the app packages and publishes but has no data at runtime. + workspace = os.path.dirname(twb_path) + for ds in datasources: + if ds['type'] == 'file' and not os.path.isfile(os.path.join(workspace, ds['file'])): + die(f"Datasource '{ds['caption']}' file \"{ds['file']}\" is not in the workspace ({workspace}). " + 'Copy it there first, at that path relative to the .twb.') + # --- build the XML blocks ---------------------------------------------- root_datasources = '\n' + '\n'.join(root_datasource_xml(ds) for ds in datasources) + '\n ' @@ -318,7 +464,7 @@ def main(): head = content[:split_idx] tail = content[split_idx:] - if "class='sqlproxy'" in head: + if "class='sqlproxy'" in head or "class='federated'" in head: die('This .twb is already wired. To change its datasources, re-scaffold, copy content/src/app.js ' 'into the new workspace, and wire every datasource in one run.') From a5f54436ee64b557aa4769130d838044afa51de8 Mon Sep 17 00:00:00 2001 From: Patrick Green Date: Fri, 23 Oct 2026 13:23:31 -0600 Subject: [PATCH 05/21] @W-24630680: Harden packaging and wiring found in review and E2E testing - package_twbx.py: cut only symlink cycles, so an alias no longer hides the real path; check .twb references against the packed names (catches case mismatches macOS hides); fail cleanly on dangling or unreadable files; write to a temp file so a failure never truncates an existing .twbx; never pack the output into itself. - wire_datasource.py: double ] in field names; reject connectionName hashes that aren't letters and digits. - References: cover embedded Excel/CSV files (snapshot, local introspection). --- .../tableau-data-app-authoring/SKILL.md | 5 +- .../references/build-data-app.md | 12 +++- .../references/design-data-app.md | 6 +- .../scripts/package_twbx.py | 70 +++++++++++++------ .../scripts/tests/test_package_twbx.py | 58 +++++++++++++++ .../scripts/tests/test_wire_datasource.py | 26 +++++++ .../scripts/wire_datasource.py | 18 +++-- 7 files changed, 164 insertions(+), 31 deletions(-) diff --git a/src/plugin/skills/tableau-data-app-authoring/SKILL.md b/src/plugin/skills/tableau-data-app-authoring/SKILL.md index 93680d3..20b7f85 100644 --- a/src/plugin/skills/tableau-data-app-authoring/SKILL.md +++ b/src/plugin/skills/tableau-data-app-authoring/SKILL.md @@ -378,7 +378,10 @@ It also packs `Data/` and `Resources/.agents/` at the archive root, so a `Data/Datasources/.xls`) keeps its data. A folder is packed only when it holds at least one file to pack: the template's `Data/`, with nothing but its `.gitkeep`, is left out of the `.twbx` (expected when no file is embedded). -It fails if a `Data/` file the `.twb` references is missing. Nothing else at +It fails if a `Data/` file the `.twb` references isn't packed under exactly +that path (missing, hidden, or different letter case, which macOS hides but the +server doesn't), or if a file can't be read (e.g. a dangling symlink); an +existing `.twbx` is only replaced once the new one is complete. Nothing else at the workspace root or under `Resources/` is packed. Below those folders, `.gitkeep` placeholders and OS cruft (`.DS_Store`, `__MACOSX`) are skipped silently, and any other file or folder whose name starts with `.` is skipped, diff --git a/src/plugin/skills/tableau-data-app-authoring/references/build-data-app.md b/src/plugin/skills/tableau-data-app-authoring/references/build-data-app.md index 595c6a3..3600400 100644 --- a/src/plugin/skills/tableau-data-app-authoring/references/build-data-app.md +++ b/src/plugin/skills/tableau-data-app-authoring/references/build-data-app.md @@ -10,7 +10,8 @@ **The data app queries its datasource live via the Tableau Extensions API — there is NO local data snapshot.** The shipped app calls `readMetadataAsync()` / `queryAsync()` at view time against the -published datasource, so it always reflects current data. Two consequences: +wired datasource. A published datasource always reflects current data; an embedded Excel/CSV +file is packed into the `.twbx` and is current only as of packaging. Two consequences: 1. **The app reaches datasources through the workbook, and queries them directly.** It is a viz extension hosted on a worksheet, so it uses @@ -28,7 +29,8 @@ published datasource, so it always reflects current data. Two consequences: 2. **You cannot run the live query yourself.** A live query only executes inside the Tableau host, so you cannot see real rows until the app is published and opened in Tableau. While authoring, introspect the datasource with `get-datasource-metadata` / `query-datasource` to design and - sanity-check the query; do the **visual** review in Tableau **after** publishing. + sanity-check the query (for an embedded file, read its headers and a few rows locally instead); + do the **visual** review in Tableau **after** publishing. ## Workflow @@ -40,7 +42,9 @@ published datasource, so it always reflects current data. Two consequences: 2. **Identify the datasource(s).** Find the target published datasource and its LUID with `list-datasources` (ask the user which one if ambiguous). You can wire any number if the app genuinely needs them — all in one 'Wire a datasource in' run, primary first. Each LUID drives - both the `.twb` wiring and your introspection queries. In `app.js`, find each datasource by + both the `.twb` wiring and your introspection queries. If the user supplies an Excel or CSV + file instead, copy it into `Data/` and wire it as an embedded file (no LUID); the SKILL's + 'Embedded file datasource' covers it. In `app.js`, find each datasource by `name` (its caption) in `getAllDataSourcesAsync()`, not by position. Query each separately and combine results in `app.js` — Tableau data blending doesn't apply, so don't add blend XML. 3. **Introspect the datasource before writing `app.js`.** @@ -49,6 +53,8 @@ published datasource, so it always reflects current data. Two consequences: - `query-datasource({ datasourceLuid, query, limit })` → preview real VDS `{ data: [...] }` rows so you can sanity-check the exact query the app will run before you commit to a chart. Match columns by field caption/name, not by position. + - Embedded file: there is no LUID until it's published, so read the sheet's or CSV's header row + and a few rows from the file in `Data/`. The headers are the field names to wire and query. 4. **Author `content/src/app.js`** — see Author `app.js` below. 5. **Package** the workspace into a flat `.twbx` with `scripts/package_twbx.py` per the SKILL's 'Package into a .twbx' stage. diff --git a/src/plugin/skills/tableau-data-app-authoring/references/design-data-app.md b/src/plugin/skills/tableau-data-app-authoring/references/design-data-app.md index 1c4f9be..d57d5ec 100644 --- a/src/plugin/skills/tableau-data-app-authoring/references/design-data-app.md +++ b/src/plugin/skills/tableau-data-app-authoring/references/design-data-app.md @@ -8,7 +8,8 @@ ## What this is The design layer for a Tableau **data app** — a custom HTML/JS/CSS web app, bundled as a viz -(worksheet) extension, that queries a published datasource **live**. +(worksheet) extension, that queries a published datasource **live** (or an Excel/CSV file embedded +in the workbook, queried the same way). **You render everything yourself.** There is no Marks card, no Show Me, no Analytics pane, no drag-to-Rows. When this guide says "put the measure on position" or "grey the non-focal marks," that @@ -99,7 +100,8 @@ size change is the just-noticeable difference). - **Rough-only for area/size.** Doubling a value doubles a bubble's *area* but radius grows as √, so viewers over-read big bubbles. - **Show provenance and uncertainty.** Because the app queries **live**, surface the datasource name - and an "as of " note in the app chrome; where relevant, show a reference band / + and an "as of " note in the app chrome (for an embedded file, which is a snapshot, + say what the data covers instead); where relevant, show a reference band / confidence range and sample size. A number with no target/prior/benchmark is meaningless. - **Don't imply causation** from a scatter + trend line without the caveat. diff --git a/src/plugin/skills/tableau-data-app-authoring/scripts/package_twbx.py b/src/plugin/skills/tableau-data-app-authoring/scripts/package_twbx.py index 41732f4..3ba02c6 100644 --- a/src/plugin/skills/tableau-data-app-authoring/scripts/package_twbx.py +++ b/src/plugin/skills/tableau-data-app-authoring/scripts/package_twbx.py @@ -24,7 +24,9 @@ Hard-fails unless the workspace root has exactly one `.twb`, a `Packages/` dir, at least one `.trex` under `Packages/*/extensions/`, and every `Data/` file the -`.twb` references. +`.twb` references packed under that exact (case-sensitive) path. Also fails on a +dangling symlink or unreadable file; the output is only replaced once the new +archive is complete. Output defaults to `.twbx` next to the workspace dir; an existing file is overwritten. Prints the output path on stdout. @@ -35,6 +37,7 @@ import glob import os import sys +import tempfile import xml.etree.ElementTree as ET import zipfile @@ -68,20 +71,28 @@ def is_hidden(rel_path, top): return any(part.startswith('.') for part in below.split('/') if part) +# True when a symlinked dir points back at one of its own ancestors. Only cycles +# are cut: a second link to the same dir (Data/current -> v2) is packed under +# both names, since the .twb may reference either. +def is_cycle(workspace, rel_root): + parts = rel_root.split('/') + real_root = os.path.realpath(os.path.join(workspace, rel_root)) + return any(os.path.realpath(os.path.join(workspace, *parts[:i])) == real_root + for i in range(1, len(parts))) + + # Walk one packed dir and return its (path, archive name) entries in walk order, # reporting hidden skips on stderr. Dirs are kept only on the way to a packed -# file, so a dir left with no files (an empty Data/) isn't packed at all. -def collect_entries(workspace, top): +# file, so a dir left with no files (an empty Data/) isn't packed at all. `skip` +# is the output path, so repacking into the workspace never packs the old archive. +def collect_entries(workspace, top, skip): entries = [] # (path, archive name, is_dir) - visited = set() for root, dirs, files in os.walk(os.path.join(workspace, top), followlinks=True): dirs.sort() rel_root = os.path.relpath(root, workspace).replace(os.sep, '/') - real_root = os.path.realpath(root) - if is_cruft(rel_root) or real_root in visited: + if is_cruft(rel_root) or is_cycle(workspace, rel_root): dirs[:] = [] continue - visited.add(real_root) entries.append((root, rel_root, True)) # Prune hidden dirs here so each is reported once, not per file inside. for name in list(dirs): @@ -96,7 +107,12 @@ def collect_entries(workspace, top): if is_hidden(rel, top): print(f' skipped {rel} {HIDDEN_REASON}', file=sys.stderr) continue - entries.append((os.path.join(root, name), rel, False)) + path = os.path.join(root, name) + if os.path.realpath(path) == skip: + continue + if not os.path.isfile(path) or not os.access(path, os.R_OK): + die(f'Cannot read {rel} (a dangling symlink or unreadable file); fix or remove it.') + entries.append((path, rel, False)) # Every dir that is an ancestor of a packed file. needed_dirs = {rel.rsplit('/', i)[0] for _, rel, is_dir in entries if not is_dir @@ -104,7 +120,10 @@ def collect_entries(workspace, top): return [(path, rel) for path, rel, is_dir in entries if not is_dir or rel in needed_dirs] -def missing_data_files(workspace, twb): +# Data/ files the .twb references that aren't packed under exactly that name. The +# names are compared as packed, not checked on disk, so a reference whose case +# differs from the file (fine on macOS, missing on the server) still fails. +def missing_data_files(workspace, twb, packed): try: root = ET.parse(os.path.join(workspace, twb)).getroot() except ET.ParseError as error: @@ -116,7 +135,7 @@ def missing_data_files(workspace, twb): path = element.get(attr, '').replace('\\', '/') if attr == 'filename' and directory and path: path = f'{directory}/{path}' - if path.startswith('Data/') and not os.path.isfile(os.path.join(workspace, path)): + if path.startswith('Data/') and path not in packed: missing.add(path) return sorted(missing) @@ -144,24 +163,33 @@ def main(): die('No .trex under Packages/*/extensions/ — publish would fail with ' '"Package directory contains no extension .trex files under extensions/".') - missing = missing_data_files(workspace, twb) - if missing: - die(f'{twb} references files missing from the workspace (no data at runtime): ' - + ', '.join(missing)) - output = os.path.abspath(argv[2]) if len(argv) == 3 else os.path.join( os.path.dirname(workspace), os.path.basename(workspace) + '.twbx') - entries_by_top = {top: collect_entries(workspace, top) + entries_by_top = {top: collect_entries(workspace, top, os.path.realpath(output)) for top in PACKED_DIRS if os.path.isdir(os.path.join(workspace, top))} dirs_to_pack = [top for top, entries in entries_by_top.items() if entries] + packed = {rel for entries in entries_by_top.values() for _, rel in entries} + missing = missing_data_files(workspace, twb, packed) + if missing: + die(f'{twb} references files missing from the workspace (no data at runtime): ' + + ', '.join(missing)) + # Paths are workspace-relative so nothing lands under an / prefix. - with zipfile.ZipFile(output, 'w', zipfile.ZIP_DEFLATED) as archive: - archive.write(os.path.join(workspace, twb), twb) - for top in dirs_to_pack: - for path, rel in entries_by_top[top]: - archive.write(path, rel) + # Written to a temp file first, so a failure never leaves a truncated .twbx. + fd, partial = tempfile.mkstemp(suffix='.twbx', dir=os.path.dirname(output)) + os.close(fd) + try: + with zipfile.ZipFile(partial, 'w', zipfile.ZIP_DEFLATED) as archive: + archive.write(os.path.join(workspace, twb), twb) + for top in dirs_to_pack: + for path, rel in entries_by_top[top]: + archive.write(path, rel) + os.replace(partial, output) + except BaseException: + os.remove(partial) + raise packed = ', '.join(f'{top}/' for top in dirs_to_pack) print(f'✓ Packaged {twb} + {packed} into {output}', file=sys.stderr) diff --git a/src/plugin/skills/tableau-data-app-authoring/scripts/tests/test_package_twbx.py b/src/plugin/skills/tableau-data-app-authoring/scripts/tests/test_package_twbx.py index 7ded003..ec5965a 100644 --- a/src/plugin/skills/tableau-data-app-authoring/scripts/tests/test_package_twbx.py +++ b/src/plugin/skills/tableau-data-app-authoring/scripts/tests/test_package_twbx.py @@ -325,3 +325,61 @@ def test_fails_without_trex(tmp_path): assert '✗ No .trex under Packages/*/extensions/' in result.stderr assert 'Package directory contains no extension .trex files under extensions/' in result.stderr assert not (tmp_path / 'Sales Demo.twbx').exists() + + +def test_symlinked_alias_packs_the_real_path_too(tmp_path): + workspace = make_workspace(tmp_path) + (workspace / 'Data' / 'v2').mkdir(parents=True) + (workspace / 'Data' / 'v2' / 'x.csv').write_text('a\n1\n') + (workspace / 'Data' / 'current').symlink_to(workspace / 'Data' / 'v2', target_is_directory=True) + (workspace / 'Sales Demo.twb').write_text( + "") + + result = run_package(str(workspace)) + + assert result.returncode == 0, result.stderr + entries = names(result.stdout.strip()) + assert 'Data/v2/x.csv' in entries + assert 'Data/current/x.csv' in entries + + +def test_reference_case_must_match_the_packed_file(tmp_path): + workspace = make_workspace(tmp_path) + (workspace / 'Data' / 'v2').mkdir(parents=True) + (workspace / 'Data' / 'v2' / 'x.csv').write_text('a\n1\n') + (workspace / 'Sales Demo.twb').write_text( + "") + + result = run_package(str(workspace)) + + assert result.returncode == 1 + assert 'Data/V2/X.csv' in result.stderr + + +def test_dangling_symlink_fails_cleanly_and_keeps_existing_output(tmp_path): + workspace = make_workspace(tmp_path) + (workspace / 'Data').mkdir() + (workspace / 'Data' / 'gone.csv').symlink_to(tmp_path / 'nonexistent.csv') + output = tmp_path / 'Sales Demo.twbx' + output.write_bytes(b'previous good archive') + + result = run_package(str(workspace)) + + assert result.returncode == 1 + assert 'Traceback' not in result.stderr + assert '✗ Cannot read Data/gone.csv' in result.stderr + assert output.read_bytes() == b'previous good archive' + assert sorted(p.name for p in tmp_path.iterdir() if p.suffix == '.twbx') == ['Sales Demo.twbx'] + + +def test_output_inside_workspace_is_not_packed_on_repack(tmp_path): + workspace = make_workspace(tmp_path) + (workspace / 'Data').mkdir() + (workspace / 'Data' / 'x.csv').write_text('a\n1\n') + output = workspace / 'Data' / 'out.twbx' + + run_package(str(workspace), str(output)) + result = run_package(str(workspace), str(output)) + + assert result.returncode == 0, result.stderr + assert 'Data/out.twbx' not in names(str(output)) diff --git a/src/plugin/skills/tableau-data-app-authoring/scripts/tests/test_wire_datasource.py b/src/plugin/skills/tableau-data-app-authoring/scripts/tests/test_wire_datasource.py index 68c5766..a2563a9 100644 --- a/src/plugin/skills/tableau-data-app-authoring/scripts/tests/test_wire_datasource.py +++ b/src/plugin/skills/tableau-data-app-authoring/scripts/tests/test_wire_datasource.py @@ -607,3 +607,29 @@ def test_wired_file_twb_packages_with_its_data(tmp_path): entries = archive.namelist() assert XLS_PATH in entries assert 'Data/orders.csv' in entries + + +def test_brackets_in_field_names_are_doubled(tmp_path): + add_data_file(tmp_path) + twb = write_twb(tmp_path) + fields = [{"name": "Sales [USD]", "datatype": "real", "role": "measure"}, + {"name": "Region", "datatype": "string", "role": "dimension"}] + + result = run_wire(twb, write_descriptor(tmp_path, excel_descriptor(fields=fields))) + + assert result.returncode == 0, result.stderr + wired = open(twb).read() + assert '[Sales [USD]]]' in wired + assert "name='[Sales [USD]]]'" in wired + assert "name='[sum:Sales [USD]]:qk]'" in wired + + +def test_quote_in_connection_name_fails_cleanly(tmp_path): + add_data_file(tmp_path) + twb = write_twb(tmp_path) + + result = run_wire(twb, write_descriptor(tmp_path, excel_descriptor(connectionName="federated.ab'c"))) + + assert result.returncode == 1 + assert result.stderr.strip() == ( + '✗ connectionName must be letters and digits after the prefix (got "federated.ab\'c").') diff --git a/src/plugin/skills/tableau-data-app-authoring/scripts/wire_datasource.py b/src/plugin/skills/tableau-data-app-authoring/scripts/wire_datasource.py index 3793762..5190eeb 100644 --- a/src/plugin/skills/tableau-data-app-authoring/scripts/wire_datasource.py +++ b/src/plugin/skills/tableau-data-app-authoring/scripts/wire_datasource.py @@ -58,6 +58,7 @@ import json import os import random +import re import string import sys import xml.etree.ElementTree as ET @@ -123,13 +124,19 @@ def derive_field(field, ordinal, error_prefix=''): 'aggregation': aggregation, # role attribute: 0 = dimension, 1 = measure 'roleAttr': 1 if is_measure else 0, - 'localName': f'[{name}]', + 'localName': bracket(name), # column-instance derivation + name token: [sum:Profit:qk] / [none:Region:nk] 'derivation': 'Sum' if is_measure else 'None', - 'instanceName': f'[sum:{name}:qk]' if is_measure else f'[none:{name}:nk]', + 'instanceName': bracket(f'sum:{name}:qk' if is_measure else f'none:{name}:nk'), } +# The hash after the prefix is letters and digits, as Tableau generates it. +def check_connection_hash(connection_name, error_prefix): + if connection_name and not re.fullmatch(r'[a-z]+\.[A-Za-z0-9]+', connection_name): + die(f'{error_prefix}connectionName must be letters and digits after the prefix (got "{connection_name}").') + + def generate_connection_name(prefix): token = lambda: ''.join(random.choices(string.ascii_lowercase + string.digits, k=11)) return f'{prefix}.{token()}{token()}' @@ -164,6 +171,7 @@ def parse_datasource(descriptor, error_prefix=''): connection_name = descriptor.get('connectionName') if connection_name and not connection_name.startswith('sqlproxy.'): die(f'{error_prefix}connectionName must start with "sqlproxy." (got "{connection_name}").') + check_connection_hash(connection_name, error_prefix) return { 'type': 'published', @@ -204,6 +212,7 @@ def parse_file_datasource(descriptor, error_prefix): connection_name = descriptor.get('connectionName') if connection_name and not connection_name.startswith('federated.'): die(f'{error_prefix}connectionName for a file datasource must start with "federated." (got "{connection_name}").') + check_connection_hash(connection_name, error_prefix) return { 'type': 'file', @@ -389,10 +398,11 @@ def verify_wiring(wired, datasources): if root_ds is None: die(f'Root is missing {name} — wiring incomplete.') relation_connection = ds['relationConnection'] - if root_ds.find(f".//relation[@connection='{relation_connection}']") is None: + if not any(r.get('connection') == relation_connection for r in root_ds.iter('relation')): target = 'itself' if relation_connection == name else relation_connection die(f'Root datasource {name} has no relation connection to {target} — wiring incomplete.') - if ds['type'] == 'file' and root_ds.find(f".//named-connection[@name='{relation_connection}']") is None: + if ds['type'] == 'file' and not any( + c.get('name') == relation_connection for c in root_ds.iter('named-connection')): die(f'Root datasource {name} has no named connection {relation_connection} — wiring incomplete.') # The server only connects datasources listed on the host sheet's view. From 9d01bac3cdf7ae4910fa04f3cf0d75c542973bae Mon Sep 17 00:00:00 2001 From: Patrick Green Date: Fri, 23 Oct 2026 13:33:34 -0600 Subject: [PATCH 06/21] @W-24630680: Never follow symlinks when packaging or wiring A followed link could pull any file on the machine (an SSH key, a PAT file) into a .twbx that gets published. package_twbx.py now skips and reports every symlink below the packed dirs (and a linked Data/ or Resources/.agents/), and fails if the .twb or Packages/ is one; a .twb reference to a skipped link fails as missing. wire_datasource.py rejects a symlinked data file up front. --- .../tableau-data-app-authoring/SKILL.md | 14 +-- .../scripts/package_twbx.py | 70 ++++++++------ .../scripts/tests/test_package_twbx.py | 96 +++++++++++++------ .../scripts/tests/test_wire_datasource.py | 14 +++ .../scripts/wire_datasource.py | 4 + 5 files changed, 138 insertions(+), 60 deletions(-) diff --git a/src/plugin/skills/tableau-data-app-authoring/SKILL.md b/src/plugin/skills/tableau-data-app-authoring/SKILL.md index 20b7f85..fb3cc7f 100644 --- a/src/plugin/skills/tableau-data-app-authoring/SKILL.md +++ b/src/plugin/skills/tableau-data-app-authoring/SKILL.md @@ -379,14 +379,16 @@ It also packs `Data/` and `Resources/.agents/` at the archive root, so a holds at least one file to pack: the template's `Data/`, with nothing but its `.gitkeep`, is left out of the `.twbx` (expected when no file is embedded). It fails if a `Data/` file the `.twb` references isn't packed under exactly -that path (missing, hidden, or different letter case, which macOS hides but the -server doesn't), or if a file can't be read (e.g. a dangling symlink); an -existing `.twbx` is only replaced once the new one is complete. Nothing else at +that path (missing, hidden, a symlink, or different letter case, which macOS +hides but the server doesn't), or if a file can't be read; an existing `.twbx` +is only replaced once the new one is complete. Nothing else at the workspace root or under `Resources/` is packed. Below those folders, `.gitkeep` placeholders and OS cruft (`.DS_Store`, `__MACOSX`) are skipped -silently, and any other file or folder whose name starts with `.` is skipped, -along with everything inside it, and listed on stderr as ` skipped -()`; relay those lines to the user. +silently. Any other file or folder whose name starts with `.`, and every +symlink (never followed, since a link could pull any file on the machine into a +published workbook), is skipped along with everything inside it and listed on +stderr as ` skipped ()`; relay those lines to the user. Copy +embedded files into `Data/`; never link them. It writes `.twbx` next to the workspace dir (pass a second arg to override), overwriting any existing file, and prints the path for Publish. diff --git a/src/plugin/skills/tableau-data-app-authoring/scripts/package_twbx.py b/src/plugin/skills/tableau-data-app-authoring/scripts/package_twbx.py index 3ba02c6..fe71aeb 100644 --- a/src/plugin/skills/tableau-data-app-authoring/scripts/package_twbx.py +++ b/src/plugin/skills/tableau-data-app-authoring/scripts/package_twbx.py @@ -19,14 +19,15 @@ packed file, so a `Data/` with nothing but its `.gitkeep` is left out. OS cruft (`.DS_Store`, `__MACOSX`) and `.gitkeep` placeholders are skipped silently; any other file or dir whose name starts with `.` below those dirs is skipped and -listed on stderr as ` skipped ()`. Symlinked files and dirs are -followed. +listed on stderr as ` skipped ()`. Symlinks are never followed: +each one is skipped and listed the same way, since a link could pull any file +on the machine (an SSH key, a token) into an archive that gets published. Hard-fails unless the workspace root has exactly one `.twb`, a `Packages/` dir, at least one `.trex` under `Packages/*/extensions/`, and every `Data/` file the -`.twb` references packed under that exact (case-sensitive) path. Also fails on a -dangling symlink or unreadable file; the output is only replaced once the new -archive is complete. +`.twb` references packed under that exact (case-sensitive) path. Also fails if +the `.twb` or `Packages/` is a symlink, or on an unreadable file; the output is +only replaced once the new archive is complete. Output defaults to `.twbx` next to the workspace dir; an existing file is overwritten. Prints the output path on stdout. @@ -46,6 +47,7 @@ PACKED_DIRS = ('Packages', 'Data', 'Resources/.agents') HIDDEN_REASON = "(hidden: names starting with '.' are not packed)" +SYMLINK_REASON = '(symlink: links are not followed; copy the file in instead)' # Keeps an otherwise-empty dir (the template's Data/) in git; never packed or reported. PLACEHOLDER = '.gitkeep' @@ -71,14 +73,13 @@ def is_hidden(rel_path, top): return any(part.startswith('.') for part in below.split('/') if part) -# True when a symlinked dir points back at one of its own ancestors. Only cycles -# are cut: a second link to the same dir (Data/current -> v2) is packed under -# both names, since the .twb may reference either. -def is_cycle(workspace, rel_root): - parts = rel_root.split('/') - real_root = os.path.realpath(os.path.join(workspace, rel_root)) - return any(os.path.realpath(os.path.join(workspace, *parts[:i])) == real_root - for i in range(1, len(parts))) +# The first symlinked component of a workspace-relative path, or None. +def linked_part(workspace, rel_path): + parts = rel_path.split('/') + for i in range(1, len(parts) + 1): + if os.path.islink(os.path.join(workspace, *parts[:i])): + return '/'.join(parts[:i]) + return None # Walk one packed dir and return its (path, archive name) entries in walk order, @@ -87,31 +88,35 @@ def is_cycle(workspace, rel_root): # is the output path, so repacking into the workspace never packs the old archive. def collect_entries(workspace, top, skip): entries = [] # (path, archive name, is_dir) - for root, dirs, files in os.walk(os.path.join(workspace, top), followlinks=True): + for root, dirs, files in os.walk(os.path.join(workspace, top)): dirs.sort() rel_root = os.path.relpath(root, workspace).replace(os.sep, '/') - if is_cruft(rel_root) or is_cycle(workspace, rel_root): + if is_cruft(rel_root): dirs[:] = [] continue entries.append((root, rel_root, True)) - # Prune hidden dirs here so each is reported once, not per file inside. + # Prune hidden and linked dirs here so each is reported once, not per file inside. for name in list(dirs): rel = f'{rel_root}/{name}' - if not is_cruft(rel) and is_hidden(rel, top): - print(f' skipped {rel}/ {HIDDEN_REASON}', file=sys.stderr) + if is_cruft(rel): + continue + reason = HIDDEN_REASON if is_hidden(rel, top) else ( + SYMLINK_REASON if os.path.islink(os.path.join(root, name)) else None) + if reason: + print(f' skipped {rel}/ {reason}', file=sys.stderr) dirs.remove(name) for name in sorted(files): rel = f'{rel_root}/{name}' - if is_cruft(rel) or name == PLACEHOLDER: - continue - if is_hidden(rel, top): - print(f' skipped {rel} {HIDDEN_REASON}', file=sys.stderr) - continue path = os.path.join(root, name) - if os.path.realpath(path) == skip: + if is_cruft(rel) or name == PLACEHOLDER or os.path.realpath(path) == skip: + continue + reason = HIDDEN_REASON if is_hidden(rel, top) else ( + SYMLINK_REASON if os.path.islink(path) else None) + if reason: + print(f' skipped {rel} {reason}', file=sys.stderr) continue if not os.path.isfile(path) or not os.access(path, os.R_OK): - die(f'Cannot read {rel} (a dangling symlink or unreadable file); fix or remove it.') + die(f'Cannot read {rel} (not a regular file you can read); fix or remove it.') entries.append((path, rel, False)) # Every dir that is an ancestor of a packed file. @@ -155,6 +160,10 @@ def main(): die(f'Expected exactly one .twb at the workspace root, found {len(twbs)}: {workspace}') twb = twbs[0] + for required in (twb, 'Packages'): + if os.path.islink(os.path.join(workspace, required)): + die(f'{required} is a symlink; links are not followed, so copy the real one in.') + packages = os.path.join(workspace, 'Packages') if not os.path.isdir(packages): die(f'Missing Packages/ directory in {workspace}') @@ -166,8 +175,15 @@ def main(): output = os.path.abspath(argv[2]) if len(argv) == 3 else os.path.join( os.path.dirname(workspace), os.path.basename(workspace) + '.twbx') - entries_by_top = {top: collect_entries(workspace, top, os.path.realpath(output)) - for top in PACKED_DIRS if os.path.isdir(os.path.join(workspace, top))} + entries_by_top = {} + for top in PACKED_DIRS: + if not os.path.isdir(os.path.join(workspace, top)): + continue + link = linked_part(workspace, top) + if link: + print(f' skipped {link}/ {SYMLINK_REASON}', file=sys.stderr) + continue + entries_by_top[top] = collect_entries(workspace, top, os.path.realpath(output)) dirs_to_pack = [top for top, entries in entries_by_top.items() if entries] packed = {rel for entries in entries_by_top.values() for _, rel in entries} diff --git a/src/plugin/skills/tableau-data-app-authoring/scripts/tests/test_package_twbx.py b/src/plugin/skills/tableau-data-app-authoring/scripts/tests/test_package_twbx.py index ec5965a..c35d06a 100644 --- a/src/plugin/skills/tableau-data-app-authoring/scripts/tests/test_package_twbx.py +++ b/src/plugin/skills/tableau-data-app-authoring/scripts/tests/test_package_twbx.py @@ -239,22 +239,75 @@ def test_skips_hidden_entries_below_packed_dirs(tmp_path): assert '✓ Packaged Sales Demo.twb + Packages/, Resources/.agents/ into' in result.stderr -def test_follows_symlinked_data_dir(tmp_path): +def test_symlinks_are_skipped_and_reported_never_followed(tmp_path): workspace = make_workspace(tmp_path) + secret = tmp_path / 'id_rsa' + secret.write_text('PRIVATE KEY') shared = tmp_path / 'shared' shared.mkdir() (shared / 'Sample.xls').write_bytes(b'\xd0\xcf\x11\xe0') - (workspace / 'Data').mkdir() - (workspace / 'Data' / 'Datasources').symlink_to(shared, target_is_directory=True) - (workspace / 'Data' / 'Datasources' / 'loop').symlink_to(workspace / 'Data', target_is_directory=True) - (workspace / 'Sales Demo.twb').write_text(TWB_WITH_XLS) + data = workspace / 'Data' + data.mkdir() + (data / 'real.csv').write_text('a\n1\n') + (data / 'orders.csv').symlink_to(secret) + (data / 'Datasources').symlink_to(shared, target_is_directory=True) + (data / 'gone.csv').symlink_to(tmp_path / 'nonexistent.csv') + (workspace / PACKAGE / 'content' / 'src' / 'lib.js').symlink_to(secret) result = run_package(str(workspace)) assert result.returncode == 0, result.stderr entries = names(result.stdout.strip()) - assert 'Data/Datasources/Sample.xls' in entries - assert not any('/loop/' in entry for entry in entries) + assert 'Data/real.csv' in entries + assert not any(e in entries for e in ('Data/orders.csv', 'Data/gone.csv', f'{PACKAGE}/content/src/lib.js')) + assert not any(e.startswith('Data/Datasources') for e in entries) + with zipfile.ZipFile(result.stdout.strip()) as archive: + assert not any(b'PRIVATE KEY' in archive.read(e) for e in entries if not e.endswith('/')) + for rel in ('Data/orders.csv', 'Data/Datasources/', 'Data/gone.csv', f'{PACKAGE}/content/src/lib.js'): + assert f' skipped {rel} (symlink: ' in result.stderr + + +def test_twb_referencing_a_symlinked_file_fails_as_missing(tmp_path): + workspace = make_workspace(tmp_path) + shared = tmp_path / 'Sample.xls' + shared.write_bytes(b'\xd0\xcf\x11\xe0') + (workspace / 'Data' / 'Datasources').mkdir(parents=True) + (workspace / 'Data' / 'Datasources' / 'Sample.xls').symlink_to(shared) + (workspace / 'Sales Demo.twb').write_text(TWB_WITH_XLS) + + result = run_package(str(workspace)) + + assert result.returncode == 1 + assert 'skipped Data/Datasources/Sample.xls (symlink: ' in result.stderr + assert 'references files missing from the workspace' in result.stderr + + +def test_symlinked_packed_dir_is_skipped(tmp_path): + workspace = make_workspace(tmp_path) + shared = tmp_path / 'shared' + shared.mkdir() + (shared / 'x.csv').write_text('a\n1\n') + (workspace / 'Data').symlink_to(shared, target_is_directory=True) + + result = run_package(str(workspace)) + + assert result.returncode == 0, result.stderr + assert not any(e.startswith('Data') for e in names(result.stdout.strip())) + assert ' skipped Data/ (symlink: ' in result.stderr + + +def test_symlinked_twb_or_packages_fails(tmp_path): + workspace = make_workspace(tmp_path) + real = tmp_path / 'real.twb' + real.write_text('') + (workspace / 'Sales Demo.twb').unlink() + (workspace / 'Sales Demo.twb').symlink_to(real) + + result = run_package(str(workspace)) + + assert result.returncode == 1 + assert '✗ Sales Demo.twb is a symlink' in result.stderr + def test_skips_other_workspace_dirs(tmp_path): @@ -327,22 +380,6 @@ def test_fails_without_trex(tmp_path): assert not (tmp_path / 'Sales Demo.twbx').exists() -def test_symlinked_alias_packs_the_real_path_too(tmp_path): - workspace = make_workspace(tmp_path) - (workspace / 'Data' / 'v2').mkdir(parents=True) - (workspace / 'Data' / 'v2' / 'x.csv').write_text('a\n1\n') - (workspace / 'Data' / 'current').symlink_to(workspace / 'Data' / 'v2', target_is_directory=True) - (workspace / 'Sales Demo.twb').write_text( - "") - - result = run_package(str(workspace)) - - assert result.returncode == 0, result.stderr - entries = names(result.stdout.strip()) - assert 'Data/v2/x.csv' in entries - assert 'Data/current/x.csv' in entries - - def test_reference_case_must_match_the_packed_file(tmp_path): workspace = make_workspace(tmp_path) (workspace / 'Data' / 'v2').mkdir(parents=True) @@ -356,18 +393,23 @@ def test_reference_case_must_match_the_packed_file(tmp_path): assert 'Data/V2/X.csv' in result.stderr -def test_dangling_symlink_fails_cleanly_and_keeps_existing_output(tmp_path): +def test_unreadable_file_fails_cleanly_and_keeps_existing_output(tmp_path): workspace = make_workspace(tmp_path) (workspace / 'Data').mkdir() - (workspace / 'Data' / 'gone.csv').symlink_to(tmp_path / 'nonexistent.csv') + locked = workspace / 'Data' / 'locked.csv' + locked.write_text('a\n1\n') + locked.chmod(0) output = tmp_path / 'Sales Demo.twbx' output.write_bytes(b'previous good archive') - result = run_package(str(workspace)) + try: + result = run_package(str(workspace)) + finally: + locked.chmod(0o644) assert result.returncode == 1 assert 'Traceback' not in result.stderr - assert '✗ Cannot read Data/gone.csv' in result.stderr + assert '✗ Cannot read Data/locked.csv' in result.stderr assert output.read_bytes() == b'previous good archive' assert sorted(p.name for p in tmp_path.iterdir() if p.suffix == '.twbx') == ['Sales Demo.twbx'] diff --git a/src/plugin/skills/tableau-data-app-authoring/scripts/tests/test_wire_datasource.py b/src/plugin/skills/tableau-data-app-authoring/scripts/tests/test_wire_datasource.py index a2563a9..4e7751b 100644 --- a/src/plugin/skills/tableau-data-app-authoring/scripts/tests/test_wire_datasource.py +++ b/src/plugin/skills/tableau-data-app-authoring/scripts/tests/test_wire_datasource.py @@ -633,3 +633,17 @@ def test_quote_in_connection_name_fails_cleanly(tmp_path): assert result.returncode == 1 assert result.stderr.strip() == ( '✗ connectionName must be letters and digits after the prefix (got "federated.ab\'c").') + + +def test_symlinked_file_is_rejected(tmp_path): + real = tmp_path / 'elsewhere.xls' + real.write_bytes(b'\xd0\xcf\x11\xe0') + (tmp_path / 'Data' / 'Datasources').mkdir(parents=True) + (tmp_path / XLS_PATH).symlink_to(real) + twb = write_twb(tmp_path) + + result = run_wire(twb, write_descriptor(tmp_path, excel_descriptor())) + + assert result.returncode == 1 + assert 'is a symlink, which packaging won\'t follow' in result.stderr + assert open(twb).read() == TWB_TEMPLATE diff --git a/src/plugin/skills/tableau-data-app-authoring/scripts/wire_datasource.py b/src/plugin/skills/tableau-data-app-authoring/scripts/wire_datasource.py index 5190eeb..4e2857e 100644 --- a/src/plugin/skills/tableau-data-app-authoring/scripts/wire_datasource.py +++ b/src/plugin/skills/tableau-data-app-authoring/scripts/wire_datasource.py @@ -445,6 +445,10 @@ def main(): if ds['type'] == 'file' and not os.path.isfile(os.path.join(workspace, ds['file'])): die(f"Datasource '{ds['caption']}' file \"{ds['file']}\" is not in the workspace ({workspace}). " 'Copy it there first, at that path relative to the .twb.') + parts = ds['file'].split('/') if ds['type'] == 'file' else [] + if any(os.path.islink(os.path.join(workspace, *parts[:i])) for i in range(1, len(parts) + 1)): + die(f"Datasource '{ds['caption']}' file \"{ds['file']}\" is a symlink, which packaging won't follow. " + 'Copy the real file there instead.') # --- build the XML blocks ---------------------------------------------- From 1ef2655ebdb781d3a29366686d4fbd3f847af3eb Mon Sep 17 00:00:00 2001 From: Patrick Green Date: Fri, 23 Oct 2026 14:14:01 -0600 Subject: [PATCH 07/21] @W-24630680: Pack only referenced Data/ files; reject template symlinks Packaging now packs only the Data/ files the .twb references and reports any other file there as skipped. A symlink pointing at the output is reported like any other link. apply_plan.py refuses a template containing symlinks, since safe_join only checks path text. Removes em dashes added on this branch. --- .../tableau-data-app-authoring/SKILL.md | 3 +- .../scripts/apply_plan.py | 8 +++ .../scripts/package_twbx.py | 40 +++++++++----- .../scripts/tests/test_apply_plan.py | 19 +++++++ .../scripts/tests/test_package_twbx.py | 55 +++++++++++++++++++ .../scripts/wire_datasource.py | 4 +- 6 files changed, 112 insertions(+), 17 deletions(-) diff --git a/src/plugin/skills/tableau-data-app-authoring/SKILL.md b/src/plugin/skills/tableau-data-app-authoring/SKILL.md index fb3cc7f..b7ed981 100644 --- a/src/plugin/skills/tableau-data-app-authoring/SKILL.md +++ b/src/plugin/skills/tableau-data-app-authoring/SKILL.md @@ -378,7 +378,8 @@ It also packs `Data/` and `Resources/.agents/` at the archive root, so a `Data/Datasources/.xls`) keeps its data. A folder is packed only when it holds at least one file to pack: the template's `Data/`, with nothing but its `.gitkeep`, is left out of the `.twbx` (expected when no file is embedded). -It fails if a `Data/` file the `.twb` references isn't packed under exactly +Only the `Data/` files the `.twb` references are packed; any other file there +is skipped and listed on stderr (see below). It fails if a `Data/` file the `.twb` references isn't packed under exactly that path (missing, hidden, a symlink, or different letter case, which macOS hides but the server doesn't), or if a file can't be read; an existing `.twbx` is only replaced once the new one is complete. Nothing else at diff --git a/src/plugin/skills/tableau-data-app-authoring/scripts/apply_plan.py b/src/plugin/skills/tableau-data-app-authoring/scripts/apply_plan.py index 2b1796b..24df9a0 100644 --- a/src/plugin/skills/tableau-data-app-authoring/scripts/apply_plan.py +++ b/src/plugin/skills/tableau-data-app-authoring/scripts/apply_plan.py @@ -46,6 +46,14 @@ def main(): if len(edits) == 0 and len(renames) == 0: die('Plan has no edits or renames — did you pass the full postUnzip object?') + # A link in the unzipped template would let an edit or rename reach outside + # the unzip dir (safe_join only checks the path text), so refuse any. + links = sorted(os.path.relpath(os.path.join(root, name), unzip_dir) + for root, dirs, files in os.walk(unzip_dir) for name in dirs + files + if os.path.islink(os.path.join(root, name))) + if links: + die('Template contains symlinks, which are not allowed: ' + ', '.join(links)) + def safe_join(rel): abs_path = os.path.abspath(os.path.join(unzip_dir, rel)) if abs_path != unzip_dir and not abs_path.startswith(unzip_dir + '/'): diff --git a/src/plugin/skills/tableau-data-app-authoring/scripts/package_twbx.py b/src/plugin/skills/tableau-data-app-authoring/scripts/package_twbx.py index fe71aeb..d3560c8 100644 --- a/src/plugin/skills/tableau-data-app-authoring/scripts/package_twbx.py +++ b/src/plugin/skills/tableau-data-app-authoring/scripts/package_twbx.py @@ -11,12 +11,14 @@ placeholder and `Resources/.agents/README.md`. The archive holds the `.twb` (written first), `Packages/`, and any `Data/` and -`Resources/.agents/` at its ROOT, never nested under `/` — nesting fails to open on publish with +`Resources/.agents/` at its ROOT, never nested under `/`: nesting fails to open on publish with `PackageValidationException: Package directory contains no extension .trex files under extensions/`. Without `Data/`, a `.twb` that references an embedded file by relative path publishes fine but has no data at runtime. Nothing else under `Resources/` is packed. A dir is packed only when it holds at least one -packed file, so a `Data/` with nothing but its `.gitkeep` is left out. OS cruft +packed file, so a `Data/` with nothing but its `.gitkeep` is left out. Under +`Data/`, only the files the `.twb` references are packed; any other file there is +skipped and listed on stderr like the skips below. OS cruft (`.DS_Store`, `__MACOSX`) and `.gitkeep` placeholders are skipped silently; any other file or dir whose name starts with `.` below those dirs is skipped and listed on stderr as ` skipped ()`. Symlinks are never followed: @@ -48,6 +50,7 @@ HIDDEN_REASON = "(hidden: names starting with '.' are not packed)" SYMLINK_REASON = '(symlink: links are not followed; copy the file in instead)' +UNREFERENCED_REASON = '(not referenced by the .twb: only the Data/ files it uses are packed)' # Keeps an otherwise-empty dir (the template's Data/) in git; never packed or reported. PLACEHOLDER = '.gitkeep' @@ -86,7 +89,8 @@ def linked_part(workspace, rel_path): # reporting hidden skips on stderr. Dirs are kept only on the way to a packed # file, so a dir left with no files (an empty Data/) isn't packed at all. `skip` # is the output path, so repacking into the workspace never packs the old archive. -def collect_entries(workspace, top, skip): +# If `allowed` is given, only files whose archive name is in it are packed. +def collect_entries(workspace, top, skip, allowed=None): entries = [] # (path, archive name, is_dir) for root, dirs, files in os.walk(os.path.join(workspace, top)): dirs.sort() @@ -108,13 +112,19 @@ def collect_entries(workspace, top, skip): for name in sorted(files): rel = f'{rel_root}/{name}' path = os.path.join(root, name) - if is_cruft(rel) or name == PLACEHOLDER or os.path.realpath(path) == skip: + if is_cruft(rel) or name == PLACEHOLDER: continue reason = HIDDEN_REASON if is_hidden(rel, top) else ( SYMLINK_REASON if os.path.islink(path) else None) if reason: print(f' skipped {rel} {reason}', file=sys.stderr) continue + # After the link check, so a link to the output is reported like any other. + if os.path.realpath(path) == skip: + continue + if allowed is not None and rel not in allowed: + print(f' skipped {rel} {UNREFERENCED_REASON}', file=sys.stderr) + continue if not os.path.isfile(path) or not os.access(path, os.R_OK): die(f'Cannot read {rel} (not a regular file you can read); fix or remove it.') entries.append((path, rel, False)) @@ -125,24 +135,24 @@ def collect_entries(workspace, top, skip): return [(path, rel) for path, rel, is_dir in entries if not is_dir or rel in needed_dirs] -# Data/ files the .twb references that aren't packed under exactly that name. The -# names are compared as packed, not checked on disk, so a reference whose case -# differs from the file (fine on macOS, missing on the server) still fails. -def missing_data_files(workspace, twb, packed): +# Data/ files the .twb references, as archive names. They're compared with the +# names as packed, not checked on disk, so a reference whose case differs from +# the file (fine on macOS, missing on the server) is reported missing. +def referenced_data_files(workspace, twb): try: root = ET.parse(os.path.join(workspace, twb)).getroot() except ET.ParseError as error: die(f'Could not parse {twb}: {error}') - missing = set() + referenced = set() for element in root.iter('connection'): directory = element.get('directory', '').replace('\\', '/').rstrip('/') for attr in FILE_ATTRS: path = element.get(attr, '').replace('\\', '/') if attr == 'filename' and directory and path: path = f'{directory}/{path}' - if path.startswith('Data/') and path not in packed: - missing.add(path) - return sorted(missing) + if path.startswith('Data/'): + referenced.add(path) + return referenced def main(): @@ -175,6 +185,7 @@ def main(): output = os.path.abspath(argv[2]) if len(argv) == 3 else os.path.join( os.path.dirname(workspace), os.path.basename(workspace) + '.twbx') + referenced = referenced_data_files(workspace, twb) entries_by_top = {} for top in PACKED_DIRS: if not os.path.isdir(os.path.join(workspace, top)): @@ -183,11 +194,12 @@ def main(): if link: print(f' skipped {link}/ {SYMLINK_REASON}', file=sys.stderr) continue - entries_by_top[top] = collect_entries(workspace, top, os.path.realpath(output)) + entries_by_top[top] = collect_entries(workspace, top, os.path.realpath(output), + referenced if top == 'Data' else None) dirs_to_pack = [top for top, entries in entries_by_top.items() if entries] packed = {rel for entries in entries_by_top.values() for _, rel in entries} - missing = missing_data_files(workspace, twb, packed) + missing = sorted(referenced - packed) if missing: die(f'{twb} references files missing from the workspace (no data at runtime): ' + ', '.join(missing)) diff --git a/src/plugin/skills/tableau-data-app-authoring/scripts/tests/test_apply_plan.py b/src/plugin/skills/tableau-data-app-authoring/scripts/tests/test_apply_plan.py index 1c3d1ba..57391a8 100644 --- a/src/plugin/skills/tableau-data-app-authoring/scripts/tests/test_apply_plan.py +++ b/src/plugin/skills/tableau-data-app-authoring/scripts/tests/test_apply_plan.py @@ -126,6 +126,25 @@ def test_rename_escaping_unzip_dir_is_rejected(tmp_path): assert 'Plan path escapes the unzip directory: ../evil' in result.stderr +def test_symlink_in_template_is_rejected_before_any_edit(tmp_path): + make_scaffold(tmp_path) + outside = tmp_path.parent / f'{tmp_path.name}-outside' + outside.mkdir() + (outside / 'f.txt').write_text('TODO App Name') + (tmp_path / 'Data App Name' / 'lnk').symlink_to(outside, target_is_directory=True) + plan = happy_plan() + plan['edits'].append({"file": "Data App Name/lnk/f.txt", + "replacements": [{"find": "TODO App Name", "replace": "PWNED"}]}) + plan_path = write_plan(tmp_path, plan) + + result = run_apply_plan(str(tmp_path), plan_path) + + assert result.returncode == 1 + assert '✗ Template contains symlinks, which are not allowed: Data App Name/lnk' in result.stderr + assert (outside / 'f.txt').read_text() == 'TODO App Name' + assert 'TODO App Name' in (tmp_path / 'Data App Name' / 'Data App Name.twb').read_text() + + def test_rename_of_nonexistent_source_hard_fails(tmp_path): plan_path = write_plan(tmp_path, { "edits": [], "renames": [{"from": "does-not-exist", "to": "renamed"}] diff --git a/src/plugin/skills/tableau-data-app-authoring/scripts/tests/test_package_twbx.py b/src/plugin/skills/tableau-data-app-authoring/scripts/tests/test_package_twbx.py index c35d06a..f4825b0 100644 --- a/src/plugin/skills/tableau-data-app-authoring/scripts/tests/test_package_twbx.py +++ b/src/plugin/skills/tableau-data-app-authoring/scripts/tests/test_package_twbx.py @@ -15,6 +15,12 @@ "") +def twb_referencing(*paths): + connections = ''.join(f"" for p in paths) + return f'{connections}' + + def run_package(*args): return subprocess.run([sys.executable, SCRIPT, *args], capture_output=True, text=True) @@ -165,6 +171,38 @@ def test_fails_when_twb_references_missing_data_file(tmp_path): assert not (tmp_path / 'Sales Demo.twbx').exists() +def test_packs_only_data_files_the_twb_references(tmp_path): + workspace = make_workspace(tmp_path) + datasources = workspace / 'Data' / 'Datasources' + datasources.mkdir(parents=True) + (datasources / 'Sample.xls').write_bytes(b'\xd0\xcf\x11\xe0') + (datasources / 'credentials.json').write_text('{"token": "x"}') + (workspace / 'Data' / 'Scratch').mkdir() + (workspace / 'Data' / 'Scratch' / 'notes.txt').write_text('x') + (workspace / 'Sales Demo.twb').write_text(TWB_WITH_XLS) + + result = run_package(str(workspace)) + + assert result.returncode == 0, result.stderr + entries = names(result.stdout.strip()) + assert 'Data/Datasources/Sample.xls' in entries + assert not any(e.startswith('Data/Scratch') or 'credentials' in e for e in entries) + for rel in ('Data/Datasources/credentials.json', 'Data/Scratch/notes.txt'): + assert f' skipped {rel} (not referenced by the .twb: ' in result.stderr + + +def test_data_dir_is_left_out_when_the_twb_references_nothing(tmp_path): + workspace = make_workspace(tmp_path) + (workspace / 'Data').mkdir() + (workspace / 'Data' / 'orders.csv').write_text('a\n1\n') + + result = run_package(str(workspace)) + + assert result.returncode == 0, result.stderr + assert not any(e.startswith('Data') for e in names(result.stdout.strip())) + assert ' skipped Data/orders.csv (not referenced by the .twb: ' in result.stderr + + def test_checks_directory_plus_filename_connections(tmp_path): # CSV, JSON, and shapefile connections split the path: directory='Data' filename='x.csv'. workspace = make_workspace(tmp_path) @@ -253,6 +291,7 @@ def test_symlinks_are_skipped_and_reported_never_followed(tmp_path): (data / 'Datasources').symlink_to(shared, target_is_directory=True) (data / 'gone.csv').symlink_to(tmp_path / 'nonexistent.csv') (workspace / PACKAGE / 'content' / 'src' / 'lib.js').symlink_to(secret) + (workspace / 'Sales Demo.twb').write_text(twb_referencing('Data/real.csv')) result = run_package(str(workspace)) @@ -399,6 +438,7 @@ def test_unreadable_file_fails_cleanly_and_keeps_existing_output(tmp_path): locked = workspace / 'Data' / 'locked.csv' locked.write_text('a\n1\n') locked.chmod(0) + (workspace / 'Sales Demo.twb').write_text(twb_referencing('Data/locked.csv')) output = tmp_path / 'Sales Demo.twbx' output.write_bytes(b'previous good archive') @@ -414,6 +454,21 @@ def test_unreadable_file_fails_cleanly_and_keeps_existing_output(tmp_path): assert sorted(p.name for p in tmp_path.iterdir() if p.suffix == '.twbx') == ['Sales Demo.twbx'] +def test_symlink_to_the_output_is_reported(tmp_path): + workspace = make_workspace(tmp_path) + (workspace / 'Data').mkdir() + (workspace / 'Data' / 'x.csv').write_text('a\n1\n') + output = tmp_path / 'Sales Demo.twbx' + (workspace / 'Data' / 'prev.twbx').symlink_to(output) + + run_package(str(workspace)) + result = run_package(str(workspace)) + + assert result.returncode == 0, result.stderr + assert ' skipped Data/prev.twbx (symlink: ' in result.stderr + assert 'Data/prev.twbx' not in names(str(output)) + + def test_output_inside_workspace_is_not_packed_on_repack(tmp_path): workspace = make_workspace(tmp_path) (workspace / 'Data').mkdir() diff --git a/src/plugin/skills/tableau-data-app-authoring/scripts/wire_datasource.py b/src/plugin/skills/tableau-data-app-authoring/scripts/wire_datasource.py index 4e2857e..e6f1dd4 100644 --- a/src/plugin/skills/tableau-data-app-authoring/scripts/wire_datasource.py +++ b/src/plugin/skills/tableau-data-app-authoring/scripts/wire_datasource.py @@ -400,10 +400,10 @@ def verify_wiring(wired, datasources): relation_connection = ds['relationConnection'] if not any(r.get('connection') == relation_connection for r in root_ds.iter('relation')): target = 'itself' if relation_connection == name else relation_connection - die(f'Root datasource {name} has no relation connection to {target} — wiring incomplete.') + die(f'Root datasource {name} has no relation connection to {target}, so wiring is incomplete.') if ds['type'] == 'file' and not any( c.get('name') == relation_connection for c in root_ds.iter('named-connection')): - die(f'Root datasource {name} has no named connection {relation_connection} — wiring incomplete.') + die(f'Root datasource {name} has no named connection {relation_connection}, so wiring is incomplete.') # The server only connects datasources listed on the host sheet's view. host_view = next( From d5a19bfbdd21221be50ec17e21de48eed99a676d Mon Sep 17 00:00:00 2001 From: Patrick Green Date: Fri, 23 Oct 2026 14:19:58 -0600 Subject: [PATCH 08/21] @W-24630680: Note how Tableau names CSV columns when wiring embedded files --- src/plugin/skills/tableau-data-app-authoring/SKILL.md | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/src/plugin/skills/tableau-data-app-authoring/SKILL.md b/src/plugin/skills/tableau-data-app-authoring/SKILL.md index b7ed981..c440ea0 100644 --- a/src/plugin/skills/tableau-data-app-authoring/SKILL.md +++ b/src/plugin/skills/tableau-data-app-authoring/SKILL.md @@ -282,8 +282,10 @@ datasource. The same script wires it; the descriptor just has `"type": "file"`. its path is outside `Data/`. 2. **Write a descriptor.** `file` is the path relative to the `.twb`. For Excel, `table` is the sheet to query (sheet `Orders` → `"table": "Orders"`); - CSV has no `table`. Field names must match the sheet's or file's column - headers exactly. No server, site, or `repositoryId`: + CSV has no `table`. Field names must match the column names Tableau gives + the file: usually the header row, but for CSV Tableau changes `[`/`]` to + `(`/`)`, names the columns of a file with no header row F1, F2…, and reads + decimal commas (`100,5`) as text. No server, site, or `repositoryId`: ```bash cat > "$WORK/descriptor.json" <<'DS_JSON' From 0e5d2f4d9f6e10749b8ad122c150c77810cd56f8 Mon Sep 17 00:00:00 2001 From: Patrick Green Date: Fri, 23 Oct 2026 15:09:33 -0600 Subject: [PATCH 09/21] @W-24630680: Drop the local header-reading hint for embedded files --- .../tableau-data-app-authoring/references/build-data-app.md | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/src/plugin/skills/tableau-data-app-authoring/references/build-data-app.md b/src/plugin/skills/tableau-data-app-authoring/references/build-data-app.md index 3600400..c39d7e1 100644 --- a/src/plugin/skills/tableau-data-app-authoring/references/build-data-app.md +++ b/src/plugin/skills/tableau-data-app-authoring/references/build-data-app.md @@ -29,8 +29,7 @@ file is packed into the `.twbx` and is current only as of packaging. Two consequ 2. **You cannot run the live query yourself.** A live query only executes inside the Tableau host, so you cannot see real rows until the app is published and opened in Tableau. While authoring, introspect the datasource with `get-datasource-metadata` / `query-datasource` to design and - sanity-check the query (for an embedded file, read its headers and a few rows locally instead); - do the **visual** review in Tableau **after** publishing. + sanity-check the query; do the **visual** review in Tableau **after** publishing. ## Workflow @@ -53,8 +52,6 @@ file is packed into the `.twbx` and is current only as of packaging. Two consequ - `query-datasource({ datasourceLuid, query, limit })` → preview real VDS `{ data: [...] }` rows so you can sanity-check the exact query the app will run before you commit to a chart. Match columns by field caption/name, not by position. - - Embedded file: there is no LUID until it's published, so read the sheet's or CSV's header row - and a few rows from the file in `Data/`. The headers are the field names to wire and query. 4. **Author `content/src/app.js`** — see Author `app.js` below. 5. **Package** the workspace into a flat `.twbx` with `scripts/package_twbx.py` per the SKILL's 'Package into a .twbx' stage. From aef57f6a6bd80058da051ef1f474d4f08856f4a6 Mon Sep 17 00:00:00 2001 From: Patrick Green Date: Fri, 23 Oct 2026 15:16:15 -0600 Subject: [PATCH 10/21] @W-24630680: Drop the embedded-file provenance hint --- .../tableau-data-app-authoring/references/design-data-app.md | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/plugin/skills/tableau-data-app-authoring/references/design-data-app.md b/src/plugin/skills/tableau-data-app-authoring/references/design-data-app.md index d57d5ec..6306c89 100644 --- a/src/plugin/skills/tableau-data-app-authoring/references/design-data-app.md +++ b/src/plugin/skills/tableau-data-app-authoring/references/design-data-app.md @@ -100,8 +100,7 @@ size change is the just-noticeable difference). - **Rough-only for area/size.** Doubling a value doubles a bubble's *area* but radius grows as √, so viewers over-read big bubbles. - **Show provenance and uncertainty.** Because the app queries **live**, surface the datasource name - and an "as of " note in the app chrome (for an embedded file, which is a snapshot, - say what the data covers instead); where relevant, show a reference band / + and an "as of " note in the app chrome; where relevant, show a reference band / confidence range and sample size. A number with no target/prior/benchmark is meaningless. - **Don't imply causation** from a scatter + trend line without the caveat. From 409e13610e46bb24b429d811d76dde52ac6a5087 Mon Sep 17 00:00:00 2001 From: Patrick Green Date: Fri, 23 Oct 2026 15:44:42 -0600 Subject: [PATCH 11/21] @W-24630680: Slim the embedded-file docs, drop file connectionName and wiring's symlink check --- .../tableau-data-app-authoring/SKILL.md | 34 ++------- .../references/build-data-app.md | 2 +- .../scripts/tests/test_wire_datasource.py | 75 ++++--------------- .../scripts/wire_datasource.py | 20 +---- 4 files changed, 21 insertions(+), 110 deletions(-) diff --git a/src/plugin/skills/tableau-data-app-authoring/SKILL.md b/src/plugin/skills/tableau-data-app-authoring/SKILL.md index c440ea0..aac2fb1 100644 --- a/src/plugin/skills/tableau-data-app-authoring/SKILL.md +++ b/src/plugin/skills/tableau-data-app-authoring/SKILL.md @@ -164,9 +164,6 @@ At the end of this stage you have a finalized workspace directory: Resources/.agents/README.md ← which agent docs travel with the workbook ``` -Keep the `Resources/.agents/README.md` as shipped and follow it for what else -to put in that folder. - **Sheet name:** the worksheet is named after the app's display name. It's a plain string with no hash/join-key involved, so it's freely hand-editable at any point before packaging — edit all 3 matching locations in the `.twb` to @@ -304,9 +301,7 @@ datasource. The same script wires it; the descriptor just has `"type": "file"`. To mix it with published datasources, put every entry in one `datasources` array (primary first) as above. -3. **Run the wiring script** the same way as for a published datasource. It - generates a `federated.` join key unless you supply `connectionName` - (which must then start with `federated.`). +3. **Run the wiring script** the same way as for a published datasource. **Reuse an existing workbook's datasource:** point the Published path above at a datasource already wired into some *other* already-published workbook, @@ -375,23 +370,10 @@ hand: python3 "$SKILL_DIR/scripts/package_twbx.py" "" # the finalized workspace dir ``` -It also packs `Data/` and `Resources/.agents/` at the archive root, so a -`.twb` that references an embedded datasource file by relative path (e.g. -`Data/Datasources/.xls`) keeps its data. A folder is packed only when it -holds at least one file to pack: the template's `Data/`, with nothing but its -`.gitkeep`, is left out of the `.twbx` (expected when no file is embedded). -Only the `Data/` files the `.twb` references are packed; any other file there -is skipped and listed on stderr (see below). It fails if a `Data/` file the `.twb` references isn't packed under exactly -that path (missing, hidden, a symlink, or different letter case, which macOS -hides but the server doesn't), or if a file can't be read; an existing `.twbx` -is only replaced once the new one is complete. Nothing else at -the workspace root or under `Resources/` is packed. Below those folders, -`.gitkeep` placeholders and OS cruft (`.DS_Store`, `__MACOSX`) are skipped -silently. Any other file or folder whose name starts with `.`, and every -symlink (never followed, since a link could pull any file on the machine into a -published workbook), is skipped along with everything inside it and listed on -stderr as ` skipped ()`; relay those lines to the user. Copy -embedded files into `Data/`; never link them. +It also packs `Data/` and `Resources/.agents/` at the archive root. Under +`Data/`, only the files the `.twb` references are packed. Anything it skips is +listed on stderr as ` skipped ()`; relay those lines to the user. +Copy embedded files into `Data/`; never link them. It writes `.twbx` next to the workspace dir (pass a second arg to override), overwriting any existing file, and prints the path for Publish. @@ -461,11 +443,7 @@ Uses the MCP publish tools (not available to Slack clients). - Don't zip the `.twbx` freehand. Use `scripts/package_twbx.py` — it keeps `.twb`, `Packages/`, and any `Data/`/`Resources/.agents/` at the archive root, never - nested under `/`, and leaves out a `Data/` that holds only its - `.gitkeep`. -- Don't reference an embedded file from outside `Data/` or leave it out of the - workspace. `wire_datasource.py` and `package_twbx.py` both refuse, and a - `.twbx` without the file publishes but has no data at runtime. + nested under `/`. - Don't apply a postUnzip plan freehand. Use `scripts/apply_plan.py` — edits before renames, renames deepest-first, verified; see its header comment for the full contract. diff --git a/src/plugin/skills/tableau-data-app-authoring/references/build-data-app.md b/src/plugin/skills/tableau-data-app-authoring/references/build-data-app.md index c39d7e1..d25e403 100644 --- a/src/plugin/skills/tableau-data-app-authoring/references/build-data-app.md +++ b/src/plugin/skills/tableau-data-app-authoring/references/build-data-app.md @@ -42,7 +42,7 @@ file is packed into the `.twbx` and is current only as of packaging. Two consequ `list-datasources` (ask the user which one if ambiguous). You can wire any number if the app genuinely needs them — all in one 'Wire a datasource in' run, primary first. Each LUID drives both the `.twb` wiring and your introspection queries. If the user supplies an Excel or CSV - file instead, copy it into `Data/` and wire it as an embedded file (no LUID); the SKILL's + file instead, copy it into `Data/` and wire it as an embedded file; the SKILL's 'Embedded file datasource' covers it. In `app.js`, find each datasource by `name` (its caption) in `getAllDataSourcesAsync()`, not by position. Query each separately and combine results in `app.js` — Tableau data blending doesn't apply, so don't add blend XML. diff --git a/src/plugin/skills/tableau-data-app-authoring/scripts/tests/test_wire_datasource.py b/src/plugin/skills/tableau-data-app-authoring/scripts/tests/test_wire_datasource.py index 4e7751b..211e230 100644 --- a/src/plugin/skills/tableau-data-app-authoring/scripts/tests/test_wire_datasource.py +++ b/src/plugin/skills/tableau-data-app-authoring/scripts/tests/test_wire_datasource.py @@ -401,7 +401,6 @@ def excel_descriptor(**overrides): "caption": "Sample - Superstore", "file": XLS_PATH, "table": "Orders", - "connectionName": "federated.abc123", "fields": [ {"name": "Sales", "datatype": "real", "role": "measure"}, {"name": "Region", "datatype": "string", "role": "dimension"}, @@ -417,7 +416,6 @@ def csv_descriptor(**overrides): "type": "file", "caption": "Orders CSV", "file": "Data/orders.csv", - "connectionName": "federated.csv1", "fields": [ {"name": "Profit", "datatype": "real", "role": "measure"}, {"name": "Ship Date", "datatype": "date", "role": "dimension"}, @@ -434,21 +432,23 @@ def test_excel_file_wires_federated_datasource_with_matching_join_keys(tmp_path) assert result.returncode == 0, result.stderr assert result.stdout.strip() == twb_path - assert "✓ Wired datasource 'Sample - Superstore' (federated.abc123) with 3 field(s)" in result.stderr - wired = open(twb_path).read() root, view = view_of(wired) root_ds = root.find('./datasources/datasource') - assert root_ds.get('name') == 'federated.abc123' + name = root_ds.get('name') + assert re.fullmatch(r'federated\.[a-z0-9]{22}', name) + assert f"✓ Wired datasource 'Sample - Superstore' ({name}) with 3 field(s)" in result.stderr + # The named connection shares the generated hash. + key = 'excel-direct.' + name.split('.', 1)[1] assert root_ds.get('caption') == 'Sample - Superstore' assert root_ds.find('./connection').get('class') == 'federated' named = root_ds.find('./connection/named-connections/named-connection') - assert named.get('name') == 'excel-direct.abc123' + assert named.get('name') == key file_connection = named.find('./connection') assert file_connection.get('class') == 'excel-direct' assert file_connection.get('filename') == XLS_PATH relation = root_ds.find('./connection/relation') - assert relation.attrib == {'connection': 'excel-direct.abc123', 'name': 'Orders', 'table': '[Orders$]', 'type': 'table'} + assert relation.attrib == {'connection': key, 'name': 'Orders', 'table': '[Orders$]', 'type': 'table'} records = {r.findtext('remote-name'): r for r in root_ds.iter('metadata-record')} assert records['Sales'].findtext('remote-type') == '5' assert records['Region'].findtext('remote-type') == '130' @@ -457,8 +457,8 @@ def test_excel_file_wires_federated_datasource_with_matching_join_keys(tmp_path) assert 'sqlproxy' not in wired # The view half is the same as for a published datasource. - assert [d.get('name') for d in view.findall('./datasources/datasource')] == ['federated.abc123'] - assert [d.get('datasource') for d in view.findall('./datasource-dependencies')] == ['federated.abc123'] + assert [d.get('name') for d in view.findall('./datasources/datasource')] == [name] + assert [d.get('datasource') for d in view.findall('./datasource-dependencies')] == [name] assert "" in wired assert "" in wired @@ -472,7 +472,7 @@ def test_csv_file_splits_directory_and_filename(tmp_path): root, _ = view_of(open(twb_path).read()) root_ds = root.find('./datasources/datasource') named = root_ds.find('./connection/named-connections/named-connection') - assert named.get('name') == 'textscan.csv1' + assert named.get('name') == 'textscan.' + root_ds.get('name').split('.', 1)[1] assert named.find('./connection').attrib == { 'class': 'textscan', 'directory': 'Data', 'filename': 'orders.csv', 'password': '', 'server': ''} relation = root_ds.find('./connection/relation') @@ -493,27 +493,13 @@ def test_published_and_file_datasources_wire_in_one_run(tmp_path): assert result.returncode == 0, result.stderr wired = open(twb_path).read() root, view = view_of(wired) - assert [d.get('name') for d in root.findall('./datasources/datasource')] == ['sqlproxy.abc123', 'federated.abc123'] - assert [d.get('name') for d in view.findall('./datasources/datasource')] == ['sqlproxy.abc123', 'federated.abc123'] + names = [d.get('name') for d in root.findall('./datasources/datasource')] + assert names[0] == 'sqlproxy.abc123' and names[1].startswith('federated.') + assert [d.get('name') for d in view.findall('./datasources/datasource')] == names # The published primary alone carries datasource-dependencies. assert [d.get('datasource') for d in view.findall('./datasource-dependencies')] == ['sqlproxy.abc123'] -def test_generated_file_connection_name_shares_its_hash_with_the_named_connection(tmp_path): - add_data_file(tmp_path) - twb_path = write_twb(tmp_path) - result = run_wire(twb_path, write_descriptor(tmp_path, excel_descriptor(connectionName=None))) - - assert result.returncode == 0, result.stderr - root, _ = view_of(open(twb_path).read()) - root_ds = root.find('./datasources/datasource') - name = root_ds.get('name') - assert re.fullmatch(r'federated\.[a-z0-9]{22}', name) - expected = 'excel-direct.' + name.split('.', 1)[1] - assert root_ds.find('./connection/named-connections/named-connection').get('name') == expected - assert root_ds.find('./connection/relation').get('connection') == expected - - def test_file_missing_from_the_workspace_is_rejected(tmp_path): twb_path = write_twb(tmp_path) result = run_wire(twb_path, write_descriptor(tmp_path, excel_descriptor())) @@ -556,16 +542,6 @@ def test_excel_requires_a_table_and_csv_rejects_one(tmp_path): assert result.stderr.strip() == '✗ "table" applies only to Excel files; a CSV file is its own table.' -def test_file_connection_name_must_start_with_federated_prefix(tmp_path): - add_data_file(tmp_path) - twb_path = write_twb(tmp_path) - result = run_wire(twb_path, write_descriptor(tmp_path, excel_descriptor(connectionName='sqlproxy.abc'))) - - assert result.returncode == 1 - assert result.stderr.strip() == ( - '✗ connectionName for a file datasource must start with "federated." (got "sqlproxy.abc").') - - def test_unknown_type_is_rejected(tmp_path): twb_path = write_twb(tmp_path) result = run_wire(twb_path, write_descriptor(tmp_path, excel_descriptor(type='extract'))) @@ -622,28 +598,3 @@ def test_brackets_in_field_names_are_doubled(tmp_path): assert '[Sales [USD]]]' in wired assert "name='[Sales [USD]]]'" in wired assert "name='[sum:Sales [USD]]:qk]'" in wired - - -def test_quote_in_connection_name_fails_cleanly(tmp_path): - add_data_file(tmp_path) - twb = write_twb(tmp_path) - - result = run_wire(twb, write_descriptor(tmp_path, excel_descriptor(connectionName="federated.ab'c"))) - - assert result.returncode == 1 - assert result.stderr.strip() == ( - '✗ connectionName must be letters and digits after the prefix (got "federated.ab\'c").') - - -def test_symlinked_file_is_rejected(tmp_path): - real = tmp_path / 'elsewhere.xls' - real.write_bytes(b'\xd0\xcf\x11\xe0') - (tmp_path / 'Data' / 'Datasources').mkdir(parents=True) - (tmp_path / XLS_PATH).symlink_to(real) - twb = write_twb(tmp_path) - - result = run_wire(twb, write_descriptor(tmp_path, excel_descriptor())) - - assert result.returncode == 1 - assert 'is a symlink, which packaging won\'t follow' in result.stderr - assert open(twb).read() == TWB_TEMPLATE diff --git a/src/plugin/skills/tableau-data-app-authoring/scripts/wire_datasource.py b/src/plugin/skills/tableau-data-app-authoring/scripts/wire_datasource.py index e6f1dd4..eaeb98b 100644 --- a/src/plugin/skills/tableau-data-app-authoring/scripts/wire_datasource.py +++ b/src/plugin/skills/tableau-data-app-authoring/scripts/wire_datasource.py @@ -47,7 +47,6 @@ "caption": "Sample - Superstore", "file": "Data/Datasources/Sample - Superstore.xls", // relative to the .twb; .xls, .xlsx, .xlsm, or .csv "table": "Orders", // Excel only: the sheet to query - "connectionName": "federated.", // optional, generated if omitted "fields": [ ...as above... ] } or any number of either, primary first, wired in a single run: @@ -58,7 +57,6 @@ import json import os import random -import re import string import sys import xml.etree.ElementTree as ET @@ -131,12 +129,6 @@ def derive_field(field, ordinal, error_prefix=''): } -# The hash after the prefix is letters and digits, as Tableau generates it. -def check_connection_hash(connection_name, error_prefix): - if connection_name and not re.fullmatch(r'[a-z]+\.[A-Za-z0-9]+', connection_name): - die(f'{error_prefix}connectionName must be letters and digits after the prefix (got "{connection_name}").') - - def generate_connection_name(prefix): token = lambda: ''.join(random.choices(string.ascii_lowercase + string.digits, k=11)) return f'{prefix}.{token()}{token()}' @@ -171,7 +163,6 @@ def parse_datasource(descriptor, error_prefix=''): connection_name = descriptor.get('connectionName') if connection_name and not connection_name.startswith('sqlproxy.'): die(f'{error_prefix}connectionName must start with "sqlproxy." (got "{connection_name}").') - check_connection_hash(connection_name, error_prefix) return { 'type': 'published', @@ -209,11 +200,6 @@ def parse_file_datasource(descriptor, error_prefix): fields = parse_fields(descriptor, error_prefix) - connection_name = descriptor.get('connectionName') - if connection_name and not connection_name.startswith('federated.'): - die(f'{error_prefix}connectionName for a file datasource must start with "federated." (got "{connection_name}").') - check_connection_hash(connection_name, error_prefix) - return { 'type': 'file', 'caption': descriptor['caption'], @@ -221,7 +207,7 @@ def parse_file_datasource(descriptor, error_prefix): 'class': file_class, 'table': table, 'fields': fields, - 'connectionName': connection_name, + 'connectionName': None, # always generated } @@ -445,10 +431,6 @@ def main(): if ds['type'] == 'file' and not os.path.isfile(os.path.join(workspace, ds['file'])): die(f"Datasource '{ds['caption']}' file \"{ds['file']}\" is not in the workspace ({workspace}). " 'Copy it there first, at that path relative to the .twb.') - parts = ds['file'].split('/') if ds['type'] == 'file' else [] - if any(os.path.islink(os.path.join(workspace, *parts[:i])) for i in range(1, len(parts) + 1)): - die(f"Datasource '{ds['caption']}' file \"{ds['file']}\" is a symlink, which packaging won't follow. " - 'Copy the real file there instead.') # --- build the XML blocks ---------------------------------------------- From 8d156f8ede4cb123f8279e07039f9ca3a3f7e6fc Mon Sep 17 00:00:00 2001 From: Patrick Green Date: Fri, 23 Oct 2026 16:02:00 -0600 Subject: [PATCH 12/21] @W-24630680: Name embedded file formats only in the SKILL's embedded-file section --- src/plugin/skills/tableau-data-app-authoring/SKILL.md | 6 +++--- .../references/build-data-app.md | 8 ++++---- .../references/design-data-app.md | 3 +-- 3 files changed, 8 insertions(+), 9 deletions(-) diff --git a/src/plugin/skills/tableau-data-app-authoring/SKILL.md b/src/plugin/skills/tableau-data-app-authoring/SKILL.md index aac2fb1..5cd3429 100644 --- a/src/plugin/skills/tableau-data-app-authoring/SKILL.md +++ b/src/plugin/skills/tableau-data-app-authoring/SKILL.md @@ -1,6 +1,6 @@ --- name: tableau-data-app-authoring -description: Build and publish a Tableau data app, a custom viz-extension web app that queries a published datasource live, or an Excel or CSV file embedded in the workbook. Scaffolds the app with the scaffold-data-app MCP tool, writes its query and visualization from the user's criteria or vibe, then packages it as a .twbx and publishes it. Requires scaffold-data-app, which may be unavailable depending on the user's site role, site settings, or Tableau Server version; don't invoke this skill unless it is available. Use to create, build, vibe-code, or publish a data app. Not for native workbooks or dashboards built from marks, Show Me, or the chart catalog (tableau-workbook-authoring), or for opening existing content, including published data apps (tableau-content-viewer). +description: Build and publish a Tableau data app, a custom viz-extension web app that queries a published datasource live, or a data file embedded in the workbook. Scaffolds the app with the scaffold-data-app MCP tool, writes its query and visualization from the user's criteria or vibe, then packages it as a .twbx and publishes it. Requires scaffold-data-app, which may be unavailable depending on the user's site role, site settings, or Tableau Server version; don't invoke this skill unless it is available. Use to create, build, vibe-code, or publish a data app. Not for native workbooks or dashboards built from marks, Show Me, or the chart catalog (tableau-workbook-authoring), or for opening existing content, including published data apps (tableau-content-viewer). --- # Tableau Data App Authoring @@ -189,7 +189,7 @@ The app queries either kind of datasource, in any mix: connection, resolved live by VDS). Its identity can come straight from the user, or be reused from a datasource already wired into some other existing workbook (see Reuse an existing workbook's datasource below). -- **Embedded file:** an Excel sheet or CSV file the user supplies, copied into +- **Embedded file:** a data file the user supplies, copied into the workspace's `Data/` folder and packed into the `.twbx` (see Embedded file datasource below). @@ -316,7 +316,7 @@ have produced). 2. Check the matched entry's `datasourceType`: - **`"embedded"`**: its data lives inside that workbook, so it can't be reused by reference. Ask the user for either a published datasource or - the source Excel/CSV file, and wire the file as an Embedded file + the source data file, and wire the file as an Embedded file datasource above. - **`"published"`** — still run the rest of Published datasource step 1 (`list-datasources` filtered by this LUID, for `contentUrl`/site/server diff --git a/src/plugin/skills/tableau-data-app-authoring/references/build-data-app.md b/src/plugin/skills/tableau-data-app-authoring/references/build-data-app.md index d25e403..6e8a7ed 100644 --- a/src/plugin/skills/tableau-data-app-authoring/references/build-data-app.md +++ b/src/plugin/skills/tableau-data-app-authoring/references/build-data-app.md @@ -10,8 +10,8 @@ **The data app queries its datasource live via the Tableau Extensions API — there is NO local data snapshot.** The shipped app calls `readMetadataAsync()` / `queryAsync()` at view time against the -wired datasource. A published datasource always reflects current data; an embedded Excel/CSV -file is packed into the `.twbx` and is current only as of packaging. Two consequences: +wired datasource. A published datasource always reflects current data; an embedded file is +packed into the `.twbx` and is current only as of packaging. Two consequences: 1. **The app reaches datasources through the workbook, and queries them directly.** It is a viz extension hosted on a worksheet, so it uses @@ -41,8 +41,8 @@ file is packed into the `.twbx` and is current only as of packaging. Two consequ 2. **Identify the datasource(s).** Find the target published datasource and its LUID with `list-datasources` (ask the user which one if ambiguous). You can wire any number if the app genuinely needs them — all in one 'Wire a datasource in' run, primary first. Each LUID drives - both the `.twb` wiring and your introspection queries. If the user supplies an Excel or CSV - file instead, copy it into `Data/` and wire it as an embedded file; the SKILL's + both the `.twb` wiring and your introspection queries. If the user supplies a data file + instead, copy it into `Data/` and wire it as an embedded file; the SKILL's 'Embedded file datasource' covers it. In `app.js`, find each datasource by `name` (its caption) in `getAllDataSourcesAsync()`, not by position. Query each separately and combine results in `app.js` — Tableau data blending doesn't apply, so don't add blend XML. diff --git a/src/plugin/skills/tableau-data-app-authoring/references/design-data-app.md b/src/plugin/skills/tableau-data-app-authoring/references/design-data-app.md index 6306c89..1c4f9be 100644 --- a/src/plugin/skills/tableau-data-app-authoring/references/design-data-app.md +++ b/src/plugin/skills/tableau-data-app-authoring/references/design-data-app.md @@ -8,8 +8,7 @@ ## What this is The design layer for a Tableau **data app** — a custom HTML/JS/CSS web app, bundled as a viz -(worksheet) extension, that queries a published datasource **live** (or an Excel/CSV file embedded -in the workbook, queried the same way). +(worksheet) extension, that queries a published datasource **live**. **You render everything yourself.** There is no Marks card, no Show Me, no Analytics pane, no drag-to-Rows. When this guide says "put the measure on position" or "grey the non-focal marks," that From 468a336ad63b484bb3995e57f214357d413caf14 Mon Sep 17 00:00:00 2001 From: Patrick Green Date: Fri, 23 Oct 2026 16:38:16 -0600 Subject: [PATCH 13/21] @W-24630680: Shorten package_twbx.py's docstring and clarify the live-query note --- .../references/build-data-app.md | 4 +-- .../scripts/package_twbx.py | 33 ++++++------------- 2 files changed, 12 insertions(+), 25 deletions(-) diff --git a/src/plugin/skills/tableau-data-app-authoring/references/build-data-app.md b/src/plugin/skills/tableau-data-app-authoring/references/build-data-app.md index 6e8a7ed..764ad33 100644 --- a/src/plugin/skills/tableau-data-app-authoring/references/build-data-app.md +++ b/src/plugin/skills/tableau-data-app-authoring/references/build-data-app.md @@ -8,8 +8,8 @@ ## Live-query model (read this first) -**The data app queries its datasource live via the Tableau Extensions API — there is NO local data -snapshot.** The shipped app calls `readMetadataAsync()` / `queryAsync()` at view time against the +**The data app queries its datasource live via the Tableau Extensions API: no data is baked into +the app code.** The shipped app calls `readMetadataAsync()` / `queryAsync()` at view time against the wired datasource. A published datasource always reflects current data; an embedded file is packed into the `.twbx` and is current only as of packaging. Two consequences: diff --git a/src/plugin/skills/tableau-data-app-authoring/scripts/package_twbx.py b/src/plugin/skills/tableau-data-app-authoring/scripts/package_twbx.py index d3560c8..54c4495 100644 --- a/src/plugin/skills/tableau-data-app-authoring/scripts/package_twbx.py +++ b/src/plugin/skills/tableau-data-app-authoring/scripts/package_twbx.py @@ -4,32 +4,19 @@ / .twb Packages//extensions/*.trex, content/... - Data/... (optional: embedded datasource files the .twb references) - Resources/.agents/... (optional: agent docs that travel with the workbook) - -The scaffold-data-app template ships `Data/` holding only a `.gitkeep` -placeholder and `Resources/.agents/README.md`. - -The archive holds the `.twb` (written first), `Packages/`, and any `Data/` and -`Resources/.agents/` at its ROOT, never nested under `/`: nesting fails to open on publish with -`PackageValidationException: Package directory contains no extension .trex -files under extensions/`. Without `Data/`, a `.twb` that references an embedded -file by relative path publishes fine but has no data at runtime. Nothing else -under `Resources/` is packed. A dir is packed only when it holds at least one -packed file, so a `Data/` with nothing but its `.gitkeep` is left out. Under -`Data/`, only the files the `.twb` references are packed; any other file there is -skipped and listed on stderr like the skips below. OS cruft -(`.DS_Store`, `__MACOSX`) and `.gitkeep` placeholders are skipped silently; any -other file or dir whose name starts with `.` below those dirs is skipped and -listed on stderr as ` skipped ()`. Symlinks are never followed: -each one is skipped and listed the same way, since a link could pull any file -on the machine (an SSH key, a token) into an archive that gets published. + Data/... (optional: files the .twb references) + Resources/.agents/... (optional: agent docs) + +The archive holds the `.twb` (written first), `Packages/`, `Data/`, and +`Resources/.agents/` at its ROOT, never nested under `/`: nesting fails +to open on publish with `PackageValidationException: Package directory contains +no extension .trex files under extensions/`. Hidden files, symlinks, and `Data/` +files the `.twb` doesn't reference are skipped and listed on stderr as +` skipped ()`; OS cruft and `.gitkeep` are skipped silently. Hard-fails unless the workspace root has exactly one `.twb`, a `Packages/` dir, at least one `.trex` under `Packages/*/extensions/`, and every `Data/` file the -`.twb` references packed under that exact (case-sensitive) path. Also fails if -the `.twb` or `Packages/` is a symlink, or on an unreadable file; the output is -only replaced once the new archive is complete. +`.twb` references. Output defaults to `.twbx` next to the workspace dir; an existing file is overwritten. Prints the output path on stdout. From 8a2ac75afe6965b3c17775870a915fbb7a7b42d9 Mon Sep 17 00:00:00 2001 From: Patrick Green Date: Fri, 23 Oct 2026 16:51:58 -0600 Subject: [PATCH 14/21] @W-24630680: Name things clearly and drop comments that restate the code --- .../scripts/apply_plan.py | 3 +- .../scripts/package_twbx.py | 51 ++++++------------- .../scripts/tests/test_package_twbx.py | 3 -- .../scripts/tests/test_wire_datasource.py | 6 --- .../scripts/wire_datasource.py | 34 ++++--------- 5 files changed, 26 insertions(+), 71 deletions(-) diff --git a/src/plugin/skills/tableau-data-app-authoring/scripts/apply_plan.py b/src/plugin/skills/tableau-data-app-authoring/scripts/apply_plan.py index 24df9a0..5bc04b2 100644 --- a/src/plugin/skills/tableau-data-app-authoring/scripts/apply_plan.py +++ b/src/plugin/skills/tableau-data-app-authoring/scripts/apply_plan.py @@ -46,8 +46,7 @@ def main(): if len(edits) == 0 and len(renames) == 0: die('Plan has no edits or renames — did you pass the full postUnzip object?') - # A link in the unzipped template would let an edit or rename reach outside - # the unzip dir (safe_join only checks the path text), so refuse any. + # safe_join checks only the path text, so a link could escape the unzip dir. links = sorted(os.path.relpath(os.path.join(root, name), unzip_dir) for root, dirs, files in os.walk(unzip_dir) for name in dirs + files if os.path.islink(os.path.join(root, name))) diff --git a/src/plugin/skills/tableau-data-app-authoring/scripts/package_twbx.py b/src/plugin/skills/tableau-data-app-authoring/scripts/package_twbx.py index 54c4495..0013488 100644 --- a/src/plugin/skills/tableau-data-app-authoring/scripts/package_twbx.py +++ b/src/plugin/skills/tableau-data-app-authoring/scripts/package_twbx.py @@ -10,9 +10,7 @@ The archive holds the `.twb` (written first), `Packages/`, `Data/`, and `Resources/.agents/` at its ROOT, never nested under `/`: nesting fails to open on publish with `PackageValidationException: Package directory contains -no extension .trex files under extensions/`. Hidden files, symlinks, and `Data/` -files the `.twb` doesn't reference are skipped and listed on stderr as -` skipped ()`; OS cruft and `.gitkeep` are skipped silently. +no extension .trex files under extensions/`. Hard-fails unless the workspace root has exactly one `.twb`, a `Packages/` dir, at least one `.trex` under `Packages/*/extensions/`, and every `Data/` file the @@ -31,21 +29,15 @@ import xml.etree.ElementTree as ET import zipfile -# Workspace-relative dirs copied into the archive root, in order. Packages/ is -# required (checked in main); the rest are packed only when present. PACKED_DIRS = ('Packages', 'Data', 'Resources/.agents') HIDDEN_REASON = "(hidden: names starting with '.' are not packed)" SYMLINK_REASON = '(symlink: links are not followed; copy the file in instead)' UNREFERENCED_REASON = '(not referenced by the .twb: only the Data/ files it uses are packed)' -# Keeps an otherwise-empty dir (the template's Data/) in git; never packed or reported. -PLACEHOLDER = '.gitkeep' +TEMPLATE_PLACEHOLDER = '.gitkeep' -# Connection attributes that hold a workbook-relative file path. Excel and Hyper -# put the whole path in one (filename='Data/x.xlsx'); CSV, JSON, and shapefile -# split it (directory='Data' filename='x.csv'). -FILE_ATTRS = ('filename', 'dbname') +FILE_PATH_ATTRS = ('filename', 'dbname') def die(message): @@ -57,14 +49,12 @@ def is_cruft(rel_path): return any(part.endswith('.DS_Store') or part.startswith('__MACOSX') for part in rel_path.split('/')) -def is_hidden(rel_path, top): - # Hidden below the packed dir only, so Resources/.agents itself still ships. +def is_hidden_below(top, rel_path): below = rel_path[len(top):].strip('/') return any(part.startswith('.') for part in below.split('/') if part) -# The first symlinked component of a workspace-relative path, or None. -def linked_part(workspace, rel_path): +def first_symlink(workspace, rel_path): parts = rel_path.split('/') for i in range(1, len(parts) + 1): if os.path.islink(os.path.join(workspace, *parts[:i])): @@ -72,12 +62,7 @@ def linked_part(workspace, rel_path): return None -# Walk one packed dir and return its (path, archive name) entries in walk order, -# reporting hidden skips on stderr. Dirs are kept only on the way to a packed -# file, so a dir left with no files (an empty Data/) isn't packed at all. `skip` -# is the output path, so repacking into the workspace never packs the old archive. -# If `allowed` is given, only files whose archive name is in it are packed. -def collect_entries(workspace, top, skip, allowed=None): +def collect_entries(workspace, top, output, allowed=None): entries = [] # (path, archive name, is_dir) for root, dirs, files in os.walk(os.path.join(workspace, top)): dirs.sort() @@ -86,12 +71,11 @@ def collect_entries(workspace, top, skip, allowed=None): dirs[:] = [] continue entries.append((root, rel_root, True)) - # Prune hidden and linked dirs here so each is reported once, not per file inside. for name in list(dirs): rel = f'{rel_root}/{name}' if is_cruft(rel): continue - reason = HIDDEN_REASON if is_hidden(rel, top) else ( + reason = HIDDEN_REASON if is_hidden_below(top, rel) else ( SYMLINK_REASON if os.path.islink(os.path.join(root, name)) else None) if reason: print(f' skipped {rel}/ {reason}', file=sys.stderr) @@ -99,15 +83,14 @@ def collect_entries(workspace, top, skip, allowed=None): for name in sorted(files): rel = f'{rel_root}/{name}' path = os.path.join(root, name) - if is_cruft(rel) or name == PLACEHOLDER: + if is_cruft(rel) or name == TEMPLATE_PLACEHOLDER: continue - reason = HIDDEN_REASON if is_hidden(rel, top) else ( + reason = HIDDEN_REASON if is_hidden_below(top, rel) else ( SYMLINK_REASON if os.path.islink(path) else None) if reason: print(f' skipped {rel} {reason}', file=sys.stderr) continue - # After the link check, so a link to the output is reported like any other. - if os.path.realpath(path) == skip: + if os.path.realpath(path) == output: continue if allowed is not None and rel not in allowed: print(f' skipped {rel} {UNREFERENCED_REASON}', file=sys.stderr) @@ -116,15 +99,11 @@ def collect_entries(workspace, top, skip, allowed=None): die(f'Cannot read {rel} (not a regular file you can read); fix or remove it.') entries.append((path, rel, False)) - # Every dir that is an ancestor of a packed file. - needed_dirs = {rel.rsplit('/', i)[0] for _, rel, is_dir in entries if not is_dir + ancestor_dirs = {rel.rsplit('/', i)[0] for _, rel, is_dir in entries if not is_dir for i in range(1, rel.count('/') + 1)} - return [(path, rel) for path, rel, is_dir in entries if not is_dir or rel in needed_dirs] + return [(path, rel) for path, rel, is_dir in entries if not is_dir or rel in ancestor_dirs] -# Data/ files the .twb references, as archive names. They're compared with the -# names as packed, not checked on disk, so a reference whose case differs from -# the file (fine on macOS, missing on the server) is reported missing. def referenced_data_files(workspace, twb): try: root = ET.parse(os.path.join(workspace, twb)).getroot() @@ -133,7 +112,7 @@ def referenced_data_files(workspace, twb): referenced = set() for element in root.iter('connection'): directory = element.get('directory', '').replace('\\', '/').rstrip('/') - for attr in FILE_ATTRS: + for attr in FILE_PATH_ATTRS: path = element.get(attr, '').replace('\\', '/') if attr == 'filename' and directory and path: path = f'{directory}/{path}' @@ -177,7 +156,7 @@ def main(): for top in PACKED_DIRS: if not os.path.isdir(os.path.join(workspace, top)): continue - link = linked_part(workspace, top) + link = first_symlink(workspace, top) if link: print(f' skipped {link}/ {SYMLINK_REASON}', file=sys.stderr) continue @@ -186,13 +165,13 @@ def main(): dirs_to_pack = [top for top, entries in entries_by_top.items() if entries] packed = {rel for entries in entries_by_top.values() for _, rel in entries} + # By name, not on disk: a case mismatch passes on macOS but fails on the server. missing = sorted(referenced - packed) if missing: die(f'{twb} references files missing from the workspace (no data at runtime): ' + ', '.join(missing)) # Paths are workspace-relative so nothing lands under an / prefix. - # Written to a temp file first, so a failure never leaves a truncated .twbx. fd, partial = tempfile.mkstemp(suffix='.twbx', dir=os.path.dirname(output)) os.close(fd) try: diff --git a/src/plugin/skills/tableau-data-app-authoring/scripts/tests/test_package_twbx.py b/src/plugin/skills/tableau-data-app-authoring/scripts/tests/test_package_twbx.py index f4825b0..34a754f 100644 --- a/src/plugin/skills/tableau-data-app-authoring/scripts/tests/test_package_twbx.py +++ b/src/plugin/skills/tableau-data-app-authoring/scripts/tests/test_package_twbx.py @@ -40,7 +40,6 @@ def make_workspace(tmp_path, with_trex=True): def add_template_dirs(workspace): - # The scaffold-data-app template ships Data/ with only a placeholder, plus the agent README. (workspace / 'Data').mkdir() (workspace / 'Data' / '.gitkeep').write_text('') (workspace / 'Resources' / '.agents').mkdir(parents=True) @@ -204,7 +203,6 @@ def test_data_dir_is_left_out_when_the_twb_references_nothing(tmp_path): def test_checks_directory_plus_filename_connections(tmp_path): - # CSV, JSON, and shapefile connections split the path: directory='Data' filename='x.csv'. workspace = make_workspace(tmp_path) (workspace / 'Sales Demo.twb').write_text( "" @@ -271,7 +269,6 @@ def test_skips_hidden_entries_below_packed_dirs(tmp_path): assert 'skipped Resources/.agents/.env' in result.stderr assert 'gh/hosts.yml' not in result.stderr assert '.DS_Store' not in result.stderr - # Data/ held only a hidden file, so it's reported but Data/ itself isn't packed. assert 'skipped Data/.env' in result.stderr assert not any(entry.startswith('Data') for entry in entries) assert '✓ Packaged Sales Demo.twb + Packages/, Resources/.agents/ into' in result.stderr diff --git a/src/plugin/skills/tableau-data-app-authoring/scripts/tests/test_wire_datasource.py b/src/plugin/skills/tableau-data-app-authoring/scripts/tests/test_wire_datasource.py index 211e230..7b600ca 100644 --- a/src/plugin/skills/tableau-data-app-authoring/scripts/tests/test_wire_datasource.py +++ b/src/plugin/skills/tableau-data-app-authoring/scripts/tests/test_wire_datasource.py @@ -384,8 +384,6 @@ def test_default_site_omits_site_path_and_attribute(tmp_path): assert 'site=' not in wired -# --- embedded file datasources ------------------------------------------------ - XLS_PATH = 'Data/Datasources/Sample - Superstore.xls' @@ -438,7 +436,6 @@ def test_excel_file_wires_federated_datasource_with_matching_join_keys(tmp_path) name = root_ds.get('name') assert re.fullmatch(r'federated\.[a-z0-9]{22}', name) assert f"✓ Wired datasource 'Sample - Superstore' ({name}) with 3 field(s)" in result.stderr - # The named connection shares the generated hash. key = 'excel-direct.' + name.split('.', 1)[1] assert root_ds.get('caption') == 'Sample - Superstore' assert root_ds.find('./connection').get('class') == 'federated' @@ -456,7 +453,6 @@ def test_excel_file_wires_federated_datasource_with_matching_join_keys(tmp_path) assert {r.findtext('parent-name') for r in records.values()} == {'[Orders]'} assert 'sqlproxy' not in wired - # The view half is the same as for a published datasource. assert [d.get('name') for d in view.findall('./datasources/datasource')] == [name] assert [d.get('datasource') for d in view.findall('./datasource-dependencies')] == [name] assert "" in wired @@ -496,7 +492,6 @@ def test_published_and_file_datasources_wire_in_one_run(tmp_path): names = [d.get('name') for d in root.findall('./datasources/datasource')] assert names[0] == 'sqlproxy.abc123' and names[1].startswith('federated.') assert [d.get('name') for d in view.findall('./datasources/datasource')] == names - # The published primary alone carries datasource-dependencies. assert [d.get('datasource') for d in view.findall('./datasource-dependencies')] == ['sqlproxy.abc123'] @@ -563,7 +558,6 @@ def test_rewiring_a_file_wired_twb_is_rejected(tmp_path): def test_wired_file_twb_packages_with_its_data(tmp_path): - # wire_datasource.py and package_twbx.py agree on where the file lives. import zipfile workspace = tmp_path / 'Sales Demo' ext_dir = workspace / 'Packages' / 'com.tableau.mcp.sales-demo' / 'extensions' diff --git a/src/plugin/skills/tableau-data-app-authoring/scripts/wire_datasource.py b/src/plugin/skills/tableau-data-app-authoring/scripts/wire_datasource.py index eaeb98b..9fab7bd 100644 --- a/src/plugin/skills/tableau-data-app-authoring/scripts/wire_datasource.py +++ b/src/plugin/skills/tableau-data-app-authoring/scripts/wire_datasource.py @@ -1,17 +1,13 @@ #!/usr/bin/env python3 """ -Wire one or more datasources into a scaffolded data-app `.twb`: published ones -and files embedded in the workspace's `Data/` folder, in any mix. +Wire one or more datasources (published, or files in the workspace's `Data/`) +into a scaffolded data-app `.twb`. `scaffold-data-app` emits a workbook with TWO empty `` anchors (workbook root + worksheet ``). Until both are filled the extension's `getAllDataSourcesAsync()` finds nothing and renders "no data source found in -the workbook." This fills both in one run with N datasources: - - published: a `sqlproxy` (Data Server) connection, joined by `sqlproxy.`. - - file: a `federated` datasource over one Excel sheet (`excel-direct`) or CSV - file (`textscan`) at a workspace-relative `Data/...` path, joined by - `federated.`; its named connection is `.` (same hash). -Each join key stays byte-identical everywhere it appears. +the workbook." This fills both with N datasources, keeping each +join key (`sqlproxy.` or `federated.`) byte-identical everywhere. A script, not freehand XML, because the wiring spans coordinated locations that must agree exactly (root datasource `name`, root `relation connection`, view @@ -45,13 +41,12 @@ { "type": "file", "caption": "Sample - Superstore", - "file": "Data/Datasources/Sample - Superstore.xls", // relative to the .twb; .xls, .xlsx, .xlsm, or .csv - "table": "Orders", // Excel only: the sheet to query + "file": "Data/Datasources/Sample - Superstore.xls", // .xls, .xlsx, .xlsm, or .csv + "table": "Orders", // Excel only: the sheet "fields": [ ...as above... ] } or any number of either, primary first, wired in a single run: { "datasources": [ { ...as above... }, { ...as above... } ] } -`type` defaults to "published". """ import json @@ -64,11 +59,9 @@ # The exact empty anchor emitted by the scaffold template. Matched literally. EMPTY_ANCHOR = '' -# Embedded file extension -> connection class. -FILE_CLASSES = {'.xls': 'excel-direct', '.xlsx': 'excel-direct', '.xlsm': 'excel-direct', '.csv': 'textscan'} +CONNECTION_CLASS_BY_EXTENSION = {'.xls': 'excel-direct', '.xlsx': 'excel-direct', '.xlsm': 'excel-direct', '.csv': 'textscan'} -# local-type -> the remote-type code Tableau records for each file class. -FILE_REMOTE_TYPES = { +REMOTE_TYPE_CODES = { 'excel-direct': {'string': 130, 'integer': 20, 'real': 5, 'date': 7, 'datetime': 7, 'boolean': 11}, 'textscan': {'string': 129, 'integer': 20, 'real': 5, 'date': 133, 'datetime': 135, 'boolean': 11}, } @@ -187,7 +180,7 @@ def parse_file_datasource(descriptor, error_prefix): if len(parts) < 2 or parts[0] != 'Data' or any(part in ('', '.', '..') for part in parts): die(f'{error_prefix}"file" must be a path inside the workspace Data/ folder, ' f'e.g. "Data/Datasources/Sales.xlsx" (got "{descriptor["file"]}").') - file_class = FILE_CLASSES.get(os.path.splitext(file_path)[1].lower()) + file_class = CONNECTION_CLASS_BY_EXTENSION.get(os.path.splitext(file_path)[1].lower()) if not file_class: die(f'{error_prefix}"file" must be an Excel (.xls, .xlsx, .xlsm) or CSV (.csv) file (got "{file_path}").') @@ -253,8 +246,6 @@ def parse_descriptor(descriptor): name = generate_connection_name(prefix) ds['connectionName'] = name used.add(name) - # The connection the root relation points at: itself for sqlproxy, the - # named file connection (same hash) for federated. if ds['type'] == 'published': ds['relationConnection'] = ds['connectionName'] else: @@ -273,25 +264,22 @@ def root_datasource_xml(ds): return published_datasource_xml(ds) if ds['type'] == 'published' else file_datasource_xml(ds) -# Tableau bracket-quotes table names, doubling any `]` inside. def bracket(name): return '[' + name.replace(']', ']]') + ']' def file_datasource_xml(ds): if ds['class'] == 'excel-direct': - # A sheet named Orders is the table [Orders$]. relation_name, table = ds['table'], bracket(f"{ds['table']}$") connection = (f"") else: - # CSV splits the path, and orders.csv is the table [orders#csv]. directory, filename = ds['file'].rsplit('/', 1) stem, extension = filename.rsplit('.', 1) relation_name, table = filename, bracket(f'{stem}#{extension}') connection = (f"") - remote_types = FILE_REMOTE_TYPES[ds['class']] + remote_types = REMOTE_TYPE_CODES[ds['class']] metadata_records = '\n'.join( f""" {esc(f['name'])} @@ -424,8 +412,6 @@ def main(): datasources = parse_descriptor(descriptor) primary = datasources[0] - # An embedded file the .twb points at must already sit in the workspace, or - # the app packages and publishes but has no data at runtime. workspace = os.path.dirname(twb_path) for ds in datasources: if ds['type'] == 'file' and not os.path.isfile(os.path.join(workspace, ds['file'])): From e06f9fff98ff5c848840f9151d2809de31c7e890 Mon Sep 17 00:00:00 2001 From: Patrick Green Date: Fri, 23 Oct 2026 16:56:04 -0600 Subject: [PATCH 15/21] @W-24630680: Drop the template symlink check from apply_plan.py (out of scope) --- .../scripts/apply_plan.py | 7 ------- .../scripts/tests/test_apply_plan.py | 19 ------------------- 2 files changed, 26 deletions(-) diff --git a/src/plugin/skills/tableau-data-app-authoring/scripts/apply_plan.py b/src/plugin/skills/tableau-data-app-authoring/scripts/apply_plan.py index 5bc04b2..2b1796b 100644 --- a/src/plugin/skills/tableau-data-app-authoring/scripts/apply_plan.py +++ b/src/plugin/skills/tableau-data-app-authoring/scripts/apply_plan.py @@ -46,13 +46,6 @@ def main(): if len(edits) == 0 and len(renames) == 0: die('Plan has no edits or renames — did you pass the full postUnzip object?') - # safe_join checks only the path text, so a link could escape the unzip dir. - links = sorted(os.path.relpath(os.path.join(root, name), unzip_dir) - for root, dirs, files in os.walk(unzip_dir) for name in dirs + files - if os.path.islink(os.path.join(root, name))) - if links: - die('Template contains symlinks, which are not allowed: ' + ', '.join(links)) - def safe_join(rel): abs_path = os.path.abspath(os.path.join(unzip_dir, rel)) if abs_path != unzip_dir and not abs_path.startswith(unzip_dir + '/'): diff --git a/src/plugin/skills/tableau-data-app-authoring/scripts/tests/test_apply_plan.py b/src/plugin/skills/tableau-data-app-authoring/scripts/tests/test_apply_plan.py index 57391a8..1c3d1ba 100644 --- a/src/plugin/skills/tableau-data-app-authoring/scripts/tests/test_apply_plan.py +++ b/src/plugin/skills/tableau-data-app-authoring/scripts/tests/test_apply_plan.py @@ -126,25 +126,6 @@ def test_rename_escaping_unzip_dir_is_rejected(tmp_path): assert 'Plan path escapes the unzip directory: ../evil' in result.stderr -def test_symlink_in_template_is_rejected_before_any_edit(tmp_path): - make_scaffold(tmp_path) - outside = tmp_path.parent / f'{tmp_path.name}-outside' - outside.mkdir() - (outside / 'f.txt').write_text('TODO App Name') - (tmp_path / 'Data App Name' / 'lnk').symlink_to(outside, target_is_directory=True) - plan = happy_plan() - plan['edits'].append({"file": "Data App Name/lnk/f.txt", - "replacements": [{"find": "TODO App Name", "replace": "PWNED"}]}) - plan_path = write_plan(tmp_path, plan) - - result = run_apply_plan(str(tmp_path), plan_path) - - assert result.returncode == 1 - assert '✗ Template contains symlinks, which are not allowed: Data App Name/lnk' in result.stderr - assert (outside / 'f.txt').read_text() == 'TODO App Name' - assert 'TODO App Name' in (tmp_path / 'Data App Name' / 'Data App Name.twb').read_text() - - def test_rename_of_nonexistent_source_hard_fails(tmp_path): plan_path = write_plan(tmp_path, { "edits": [], "renames": [{"from": "does-not-exist", "to": "renamed"}] From 897d8f2942d22b467d0154910d382064a8efbcfa Mon Sep 17 00:00:00 2001 From: Patrick Green Date: Fri, 23 Oct 2026 17:00:09 -0600 Subject: [PATCH 16/21] @W-24630680: Skip hidden files, symlinks, and unreferenced Data/ files silently --- .../tableau-data-app-authoring/SKILL.md | 5 +-- .../scripts/package_twbx.py | 45 +++---------------- .../scripts/tests/test_package_twbx.py | 32 +------------ 3 files changed, 9 insertions(+), 73 deletions(-) diff --git a/src/plugin/skills/tableau-data-app-authoring/SKILL.md b/src/plugin/skills/tableau-data-app-authoring/SKILL.md index 5cd3429..6871218 100644 --- a/src/plugin/skills/tableau-data-app-authoring/SKILL.md +++ b/src/plugin/skills/tableau-data-app-authoring/SKILL.md @@ -371,9 +371,8 @@ python3 "$SKILL_DIR/scripts/package_twbx.py" "" # the finalized work ``` It also packs `Data/` and `Resources/.agents/` at the archive root. Under -`Data/`, only the files the `.twb` references are packed. Anything it skips is -listed on stderr as ` skipped ()`; relay those lines to the user. -Copy embedded files into `Data/`; never link them. +`Data/`, only the files the `.twb` references are packed. Hidden files and +symlinks are never packed, so copy embedded files into `Data/`; never link them. It writes `.twbx` next to the workspace dir (pass a second arg to override), overwriting any existing file, and prints the path for Publish. diff --git a/src/plugin/skills/tableau-data-app-authoring/scripts/package_twbx.py b/src/plugin/skills/tableau-data-app-authoring/scripts/package_twbx.py index 0013488..67ca85c 100644 --- a/src/plugin/skills/tableau-data-app-authoring/scripts/package_twbx.py +++ b/src/plugin/skills/tableau-data-app-authoring/scripts/package_twbx.py @@ -31,12 +31,6 @@ PACKED_DIRS = ('Packages', 'Data', 'Resources/.agents') -HIDDEN_REASON = "(hidden: names starting with '.' are not packed)" -SYMLINK_REASON = '(symlink: links are not followed; copy the file in instead)' -UNREFERENCED_REASON = '(not referenced by the .twb: only the Data/ files it uses are packed)' - -TEMPLATE_PLACEHOLDER = '.gitkeep' - FILE_PATH_ATTRS = ('filename', 'dbname') @@ -49,17 +43,9 @@ def is_cruft(rel_path): return any(part.endswith('.DS_Store') or part.startswith('__MACOSX') for part in rel_path.split('/')) -def is_hidden_below(top, rel_path): - below = rel_path[len(top):].strip('/') - return any(part.startswith('.') for part in below.split('/') if part) - - -def first_symlink(workspace, rel_path): +def crosses_symlink(workspace, rel_path): parts = rel_path.split('/') - for i in range(1, len(parts) + 1): - if os.path.islink(os.path.join(workspace, *parts[:i])): - return '/'.join(parts[:i]) - return None + return any(os.path.islink(os.path.join(workspace, *parts[:i])) for i in range(1, len(parts) + 1)) def collect_entries(workspace, top, output, allowed=None): @@ -71,29 +57,12 @@ def collect_entries(workspace, top, output, allowed=None): dirs[:] = [] continue entries.append((root, rel_root, True)) - for name in list(dirs): - rel = f'{rel_root}/{name}' - if is_cruft(rel): - continue - reason = HIDDEN_REASON if is_hidden_below(top, rel) else ( - SYMLINK_REASON if os.path.islink(os.path.join(root, name)) else None) - if reason: - print(f' skipped {rel}/ {reason}', file=sys.stderr) - dirs.remove(name) + dirs[:] = [name for name in dirs if not name.startswith('.')] for name in sorted(files): rel = f'{rel_root}/{name}' path = os.path.join(root, name) - if is_cruft(rel) or name == TEMPLATE_PLACEHOLDER: - continue - reason = HIDDEN_REASON if is_hidden_below(top, rel) else ( - SYMLINK_REASON if os.path.islink(path) else None) - if reason: - print(f' skipped {rel} {reason}', file=sys.stderr) - continue - if os.path.realpath(path) == output: - continue - if allowed is not None and rel not in allowed: - print(f' skipped {rel} {UNREFERENCED_REASON}', file=sys.stderr) + if (name.startswith('.') or is_cruft(rel) or os.path.islink(path) + or os.path.realpath(path) == output or (allowed is not None and rel not in allowed)): continue if not os.path.isfile(path) or not os.access(path, os.R_OK): die(f'Cannot read {rel} (not a regular file you can read); fix or remove it.') @@ -156,9 +125,7 @@ def main(): for top in PACKED_DIRS: if not os.path.isdir(os.path.join(workspace, top)): continue - link = first_symlink(workspace, top) - if link: - print(f' skipped {link}/ {SYMLINK_REASON}', file=sys.stderr) + if crosses_symlink(workspace, top): continue entries_by_top[top] = collect_entries(workspace, top, os.path.realpath(output), referenced if top == 'Data' else None) diff --git a/src/plugin/skills/tableau-data-app-authoring/scripts/tests/test_package_twbx.py b/src/plugin/skills/tableau-data-app-authoring/scripts/tests/test_package_twbx.py index 34a754f..3c738cf 100644 --- a/src/plugin/skills/tableau-data-app-authoring/scripts/tests/test_package_twbx.py +++ b/src/plugin/skills/tableau-data-app-authoring/scripts/tests/test_package_twbx.py @@ -117,7 +117,6 @@ def test_template_data_dir_with_only_placeholder_is_not_packed(tmp_path): entries = names(result.stdout.strip()) assert not any(entry.startswith('Data') for entry in entries) assert 'Resources/.agents/README.md' in entries - assert 'gitkeep' not in result.stderr assert '✓ Packaged Sales Demo.twb + Packages/, Resources/.agents/ into' in result.stderr @@ -134,7 +133,6 @@ def test_placeholder_is_skipped_silently_next_to_data_files(tmp_path): entries = names(result.stdout.strip()) assert 'Data/Datasources/Sample.xls' in entries assert not any('gitkeep' in entry for entry in entries) - assert 'gitkeep' not in result.stderr assert '✓ Packaged Sales Demo.twb + Packages/, Data/, Resources/.agents/ into' in result.stderr @@ -186,8 +184,6 @@ def test_packs_only_data_files_the_twb_references(tmp_path): entries = names(result.stdout.strip()) assert 'Data/Datasources/Sample.xls' in entries assert not any(e.startswith('Data/Scratch') or 'credentials' in e for e in entries) - for rel in ('Data/Datasources/credentials.json', 'Data/Scratch/notes.txt'): - assert f' skipped {rel} (not referenced by the .twb: ' in result.stderr def test_data_dir_is_left_out_when_the_twb_references_nothing(tmp_path): @@ -199,7 +195,6 @@ def test_data_dir_is_left_out_when_the_twb_references_nothing(tmp_path): assert result.returncode == 0, result.stderr assert not any(e.startswith('Data') for e in names(result.stdout.strip())) - assert ' skipped Data/orders.csv (not referenced by the .twb: ' in result.stderr def test_checks_directory_plus_filename_connections(tmp_path): @@ -264,17 +259,11 @@ def test_skips_hidden_entries_below_packed_dirs(tmp_path): assert 'Resources/.agents/README.md' in entries assert not any(part.startswith('.') for entry in entries for part in entry.removeprefix('Resources/.agents/').split('/')) - assert f" skipped {PACKAGE}/content/.config/ (hidden: names starting with '.' are not packed)\n" in result.stderr - assert f" skipped {PACKAGE}/content/.env (hidden: " in result.stderr - assert 'skipped Resources/.agents/.env' in result.stderr - assert 'gh/hosts.yml' not in result.stderr - assert '.DS_Store' not in result.stderr - assert 'skipped Data/.env' in result.stderr assert not any(entry.startswith('Data') for entry in entries) assert '✓ Packaged Sales Demo.twb + Packages/, Resources/.agents/ into' in result.stderr -def test_symlinks_are_skipped_and_reported_never_followed(tmp_path): +def test_symlinks_are_skipped_never_followed(tmp_path): workspace = make_workspace(tmp_path) secret = tmp_path / 'id_rsa' secret.write_text('PRIVATE KEY') @@ -299,8 +288,6 @@ def test_symlinks_are_skipped_and_reported_never_followed(tmp_path): assert not any(e.startswith('Data/Datasources') for e in entries) with zipfile.ZipFile(result.stdout.strip()) as archive: assert not any(b'PRIVATE KEY' in archive.read(e) for e in entries if not e.endswith('/')) - for rel in ('Data/orders.csv', 'Data/Datasources/', 'Data/gone.csv', f'{PACKAGE}/content/src/lib.js'): - assert f' skipped {rel} (symlink: ' in result.stderr def test_twb_referencing_a_symlinked_file_fails_as_missing(tmp_path): @@ -314,7 +301,6 @@ def test_twb_referencing_a_symlinked_file_fails_as_missing(tmp_path): result = run_package(str(workspace)) assert result.returncode == 1 - assert 'skipped Data/Datasources/Sample.xls (symlink: ' in result.stderr assert 'references files missing from the workspace' in result.stderr @@ -329,7 +315,6 @@ def test_symlinked_packed_dir_is_skipped(tmp_path): assert result.returncode == 0, result.stderr assert not any(e.startswith('Data') for e in names(result.stdout.strip())) - assert ' skipped Data/ (symlink: ' in result.stderr def test_symlinked_twb_or_packages_fails(tmp_path): @@ -451,21 +436,6 @@ def test_unreadable_file_fails_cleanly_and_keeps_existing_output(tmp_path): assert sorted(p.name for p in tmp_path.iterdir() if p.suffix == '.twbx') == ['Sales Demo.twbx'] -def test_symlink_to_the_output_is_reported(tmp_path): - workspace = make_workspace(tmp_path) - (workspace / 'Data').mkdir() - (workspace / 'Data' / 'x.csv').write_text('a\n1\n') - output = tmp_path / 'Sales Demo.twbx' - (workspace / 'Data' / 'prev.twbx').symlink_to(output) - - run_package(str(workspace)) - result = run_package(str(workspace)) - - assert result.returncode == 0, result.stderr - assert ' skipped Data/prev.twbx (symlink: ' in result.stderr - assert 'Data/prev.twbx' not in names(str(output)) - - def test_output_inside_workspace_is_not_packed_on_repack(tmp_path): workspace = make_workspace(tmp_path) (workspace / 'Data').mkdir() From 26fdcef78058753377fac47645f94a89a74e318c Mon Sep 17 00:00:00 2001 From: Patrick Green Date: Fri, 23 Oct 2026 17:00:46 -0600 Subject: [PATCH 17/21] @W-24630680: Realign a continuation line in package_twbx.py --- .../skills/tableau-data-app-authoring/scripts/package_twbx.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/plugin/skills/tableau-data-app-authoring/scripts/package_twbx.py b/src/plugin/skills/tableau-data-app-authoring/scripts/package_twbx.py index 67ca85c..01f9b67 100644 --- a/src/plugin/skills/tableau-data-app-authoring/scripts/package_twbx.py +++ b/src/plugin/skills/tableau-data-app-authoring/scripts/package_twbx.py @@ -69,7 +69,7 @@ def collect_entries(workspace, top, output, allowed=None): entries.append((path, rel, False)) ancestor_dirs = {rel.rsplit('/', i)[0] for _, rel, is_dir in entries if not is_dir - for i in range(1, rel.count('/') + 1)} + for i in range(1, rel.count('/') + 1)} return [(path, rel) for path, rel, is_dir in entries if not is_dir or rel in ancestor_dirs] From 5ee91eac21cbfe8d96c3aeaf7bc5bac9dc409f78 Mon Sep 17 00:00:00 2001 From: Patrick Green Date: Fri, 23 Oct 2026 17:19:34 -0600 Subject: [PATCH 18/21] @W-24630680: Drop out-of-scope packager hardening, field-name bracket doubling, and overlapping tests --- .../tableau-data-app-authoring/SKILL.md | 4 +- .../scripts/package_twbx.py | 46 ++--- .../scripts/tests/test_package_twbx.py | 163 ------------------ .../scripts/tests/test_wire_datasource.py | 15 -- .../scripts/wire_datasource.py | 4 +- 5 files changed, 14 insertions(+), 218 deletions(-) diff --git a/src/plugin/skills/tableau-data-app-authoring/SKILL.md b/src/plugin/skills/tableau-data-app-authoring/SKILL.md index 6871218..398f8a3 100644 --- a/src/plugin/skills/tableau-data-app-authoring/SKILL.md +++ b/src/plugin/skills/tableau-data-app-authoring/SKILL.md @@ -371,8 +371,8 @@ python3 "$SKILL_DIR/scripts/package_twbx.py" "" # the finalized work ``` It also packs `Data/` and `Resources/.agents/` at the archive root. Under -`Data/`, only the files the `.twb` references are packed. Hidden files and -symlinks are never packed, so copy embedded files into `Data/`; never link them. +`Data/`, only the files the `.twb` references are packed. Hidden files are +never packed. It writes `.twbx` next to the workspace dir (pass a second arg to override), overwriting any existing file, and prints the path for Publish. diff --git a/src/plugin/skills/tableau-data-app-authoring/scripts/package_twbx.py b/src/plugin/skills/tableau-data-app-authoring/scripts/package_twbx.py index 01f9b67..baa1f29 100644 --- a/src/plugin/skills/tableau-data-app-authoring/scripts/package_twbx.py +++ b/src/plugin/skills/tableau-data-app-authoring/scripts/package_twbx.py @@ -25,7 +25,6 @@ import glob import os import sys -import tempfile import xml.etree.ElementTree as ET import zipfile @@ -43,12 +42,7 @@ def is_cruft(rel_path): return any(part.endswith('.DS_Store') or part.startswith('__MACOSX') for part in rel_path.split('/')) -def crosses_symlink(workspace, rel_path): - parts = rel_path.split('/') - return any(os.path.islink(os.path.join(workspace, *parts[:i])) for i in range(1, len(parts) + 1)) - - -def collect_entries(workspace, top, output, allowed=None): +def collect_entries(workspace, top, allowed=None): entries = [] # (path, archive name, is_dir) for root, dirs, files in os.walk(os.path.join(workspace, top)): dirs.sort() @@ -60,13 +54,9 @@ def collect_entries(workspace, top, output, allowed=None): dirs[:] = [name for name in dirs if not name.startswith('.')] for name in sorted(files): rel = f'{rel_root}/{name}' - path = os.path.join(root, name) - if (name.startswith('.') or is_cruft(rel) or os.path.islink(path) - or os.path.realpath(path) == output or (allowed is not None and rel not in allowed)): + if name.startswith('.') or is_cruft(rel) or (allowed is not None and rel not in allowed): continue - if not os.path.isfile(path) or not os.access(path, os.R_OK): - die(f'Cannot read {rel} (not a regular file you can read); fix or remove it.') - entries.append((path, rel, False)) + entries.append((os.path.join(root, name), rel, False)) ancestor_dirs = {rel.rsplit('/', i)[0] for _, rel, is_dir in entries if not is_dir for i in range(1, rel.count('/') + 1)} @@ -105,10 +95,6 @@ def main(): die(f'Expected exactly one .twb at the workspace root, found {len(twbs)}: {workspace}') twb = twbs[0] - for required in (twb, 'Packages'): - if os.path.islink(os.path.join(workspace, required)): - die(f'{required} is a symlink; links are not followed, so copy the real one in.') - packages = os.path.join(workspace, 'Packages') if not os.path.isdir(packages): die(f'Missing Packages/ directory in {workspace}') @@ -123,34 +109,22 @@ def main(): referenced = referenced_data_files(workspace, twb) entries_by_top = {} for top in PACKED_DIRS: - if not os.path.isdir(os.path.join(workspace, top)): - continue - if crosses_symlink(workspace, top): - continue - entries_by_top[top] = collect_entries(workspace, top, os.path.realpath(output), - referenced if top == 'Data' else None) + if os.path.isdir(os.path.join(workspace, top)): + entries_by_top[top] = collect_entries(workspace, top, referenced if top == 'Data' else None) dirs_to_pack = [top for top, entries in entries_by_top.items() if entries] packed = {rel for entries in entries_by_top.values() for _, rel in entries} - # By name, not on disk: a case mismatch passes on macOS but fails on the server. missing = sorted(referenced - packed) if missing: die(f'{twb} references files missing from the workspace (no data at runtime): ' + ', '.join(missing)) # Paths are workspace-relative so nothing lands under an / prefix. - fd, partial = tempfile.mkstemp(suffix='.twbx', dir=os.path.dirname(output)) - os.close(fd) - try: - with zipfile.ZipFile(partial, 'w', zipfile.ZIP_DEFLATED) as archive: - archive.write(os.path.join(workspace, twb), twb) - for top in dirs_to_pack: - for path, rel in entries_by_top[top]: - archive.write(path, rel) - os.replace(partial, output) - except BaseException: - os.remove(partial) - raise + with zipfile.ZipFile(output, 'w', zipfile.ZIP_DEFLATED) as archive: + archive.write(os.path.join(workspace, twb), twb) + for top in dirs_to_pack: + for path, rel in entries_by_top[top]: + archive.write(path, rel) packed = ', '.join(f'{top}/' for top in dirs_to_pack) print(f'✓ Packaged {twb} + {packed} into {output}', file=sys.stderr) diff --git a/src/plugin/skills/tableau-data-app-authoring/scripts/tests/test_package_twbx.py b/src/plugin/skills/tableau-data-app-authoring/scripts/tests/test_package_twbx.py index 3c738cf..bec08be 100644 --- a/src/plugin/skills/tableau-data-app-authoring/scripts/tests/test_package_twbx.py +++ b/src/plugin/skills/tableau-data-app-authoring/scripts/tests/test_package_twbx.py @@ -120,42 +120,6 @@ def test_template_data_dir_with_only_placeholder_is_not_packed(tmp_path): assert '✓ Packaged Sales Demo.twb + Packages/, Resources/.agents/ into' in result.stderr -def test_placeholder_is_skipped_silently_next_to_data_files(tmp_path): - workspace = make_workspace(tmp_path) - add_template_dirs(workspace) - (workspace / 'Data' / 'Datasources').mkdir() - (workspace / 'Data' / 'Datasources' / 'Sample.xls').write_bytes(b'\xd0\xcf\x11\xe0') - (workspace / 'Sales Demo.twb').write_text(TWB_WITH_XLS) - - result = run_package(str(workspace)) - - assert result.returncode == 0, result.stderr - entries = names(result.stdout.strip()) - assert 'Data/Datasources/Sample.xls' in entries - assert not any('gitkeep' in entry for entry in entries) - assert '✓ Packaged Sales Demo.twb + Packages/, Data/, Resources/.agents/ into' in result.stderr - - -def test_dirs_without_packed_files_are_left_out(tmp_path): - workspace = make_workspace(tmp_path) - data = workspace / 'Data' - (data / 'Datasources').mkdir(parents=True) - (data / 'Datasources' / 'Sample.xls').write_bytes(b'\xd0\xcf\x11\xe0') - (data / 'Extracts').mkdir() - (data / 'Scratch').mkdir() - (data / 'Scratch' / '.gitkeep').write_text('') - (workspace / 'Sales Demo.twb').write_text(TWB_WITH_XLS) - - result = run_package(str(workspace)) - - assert result.returncode == 0, result.stderr - entries = names(result.stdout.strip()) - assert 'Data/' in entries - assert 'Data/Datasources/' in entries - assert 'Data/Datasources/Sample.xls' in entries - assert not any(entry.startswith(('Data/Extracts', 'Data/Scratch')) for entry in entries) - - def test_fails_when_twb_references_missing_data_file(tmp_path): workspace = make_workspace(tmp_path) (workspace / 'Sales Demo.twb').write_text(TWB_WITH_XLS) @@ -186,17 +150,6 @@ def test_packs_only_data_files_the_twb_references(tmp_path): assert not any(e.startswith('Data/Scratch') or 'credentials' in e for e in entries) -def test_data_dir_is_left_out_when_the_twb_references_nothing(tmp_path): - workspace = make_workspace(tmp_path) - (workspace / 'Data').mkdir() - (workspace / 'Data' / 'orders.csv').write_text('a\n1\n') - - result = run_package(str(workspace)) - - assert result.returncode == 0, result.stderr - assert not any(e.startswith('Data') for e in names(result.stdout.strip())) - - def test_checks_directory_plus_filename_connections(tmp_path): workspace = make_workspace(tmp_path) (workspace / 'Sales Demo.twb').write_text( @@ -263,74 +216,6 @@ def test_skips_hidden_entries_below_packed_dirs(tmp_path): assert '✓ Packaged Sales Demo.twb + Packages/, Resources/.agents/ into' in result.stderr -def test_symlinks_are_skipped_never_followed(tmp_path): - workspace = make_workspace(tmp_path) - secret = tmp_path / 'id_rsa' - secret.write_text('PRIVATE KEY') - shared = tmp_path / 'shared' - shared.mkdir() - (shared / 'Sample.xls').write_bytes(b'\xd0\xcf\x11\xe0') - data = workspace / 'Data' - data.mkdir() - (data / 'real.csv').write_text('a\n1\n') - (data / 'orders.csv').symlink_to(secret) - (data / 'Datasources').symlink_to(shared, target_is_directory=True) - (data / 'gone.csv').symlink_to(tmp_path / 'nonexistent.csv') - (workspace / PACKAGE / 'content' / 'src' / 'lib.js').symlink_to(secret) - (workspace / 'Sales Demo.twb').write_text(twb_referencing('Data/real.csv')) - - result = run_package(str(workspace)) - - assert result.returncode == 0, result.stderr - entries = names(result.stdout.strip()) - assert 'Data/real.csv' in entries - assert not any(e in entries for e in ('Data/orders.csv', 'Data/gone.csv', f'{PACKAGE}/content/src/lib.js')) - assert not any(e.startswith('Data/Datasources') for e in entries) - with zipfile.ZipFile(result.stdout.strip()) as archive: - assert not any(b'PRIVATE KEY' in archive.read(e) for e in entries if not e.endswith('/')) - - -def test_twb_referencing_a_symlinked_file_fails_as_missing(tmp_path): - workspace = make_workspace(tmp_path) - shared = tmp_path / 'Sample.xls' - shared.write_bytes(b'\xd0\xcf\x11\xe0') - (workspace / 'Data' / 'Datasources').mkdir(parents=True) - (workspace / 'Data' / 'Datasources' / 'Sample.xls').symlink_to(shared) - (workspace / 'Sales Demo.twb').write_text(TWB_WITH_XLS) - - result = run_package(str(workspace)) - - assert result.returncode == 1 - assert 'references files missing from the workspace' in result.stderr - - -def test_symlinked_packed_dir_is_skipped(tmp_path): - workspace = make_workspace(tmp_path) - shared = tmp_path / 'shared' - shared.mkdir() - (shared / 'x.csv').write_text('a\n1\n') - (workspace / 'Data').symlink_to(shared, target_is_directory=True) - - result = run_package(str(workspace)) - - assert result.returncode == 0, result.stderr - assert not any(e.startswith('Data') for e in names(result.stdout.strip())) - - -def test_symlinked_twb_or_packages_fails(tmp_path): - workspace = make_workspace(tmp_path) - real = tmp_path / 'real.twb' - real.write_text('') - (workspace / 'Sales Demo.twb').unlink() - (workspace / 'Sales Demo.twb').symlink_to(real) - - result = run_package(str(workspace)) - - assert result.returncode == 1 - assert '✗ Sales Demo.twb is a symlink' in result.stderr - - - def test_skips_other_workspace_dirs(tmp_path): workspace = make_workspace(tmp_path) (workspace / 'node_modules' / 'pkg').mkdir(parents=True) @@ -399,51 +284,3 @@ def test_fails_without_trex(tmp_path): assert '✗ No .trex under Packages/*/extensions/' in result.stderr assert 'Package directory contains no extension .trex files under extensions/' in result.stderr assert not (tmp_path / 'Sales Demo.twbx').exists() - - -def test_reference_case_must_match_the_packed_file(tmp_path): - workspace = make_workspace(tmp_path) - (workspace / 'Data' / 'v2').mkdir(parents=True) - (workspace / 'Data' / 'v2' / 'x.csv').write_text('a\n1\n') - (workspace / 'Sales Demo.twb').write_text( - "") - - result = run_package(str(workspace)) - - assert result.returncode == 1 - assert 'Data/V2/X.csv' in result.stderr - - -def test_unreadable_file_fails_cleanly_and_keeps_existing_output(tmp_path): - workspace = make_workspace(tmp_path) - (workspace / 'Data').mkdir() - locked = workspace / 'Data' / 'locked.csv' - locked.write_text('a\n1\n') - locked.chmod(0) - (workspace / 'Sales Demo.twb').write_text(twb_referencing('Data/locked.csv')) - output = tmp_path / 'Sales Demo.twbx' - output.write_bytes(b'previous good archive') - - try: - result = run_package(str(workspace)) - finally: - locked.chmod(0o644) - - assert result.returncode == 1 - assert 'Traceback' not in result.stderr - assert '✗ Cannot read Data/locked.csv' in result.stderr - assert output.read_bytes() == b'previous good archive' - assert sorted(p.name for p in tmp_path.iterdir() if p.suffix == '.twbx') == ['Sales Demo.twbx'] - - -def test_output_inside_workspace_is_not_packed_on_repack(tmp_path): - workspace = make_workspace(tmp_path) - (workspace / 'Data').mkdir() - (workspace / 'Data' / 'x.csv').write_text('a\n1\n') - output = workspace / 'Data' / 'out.twbx' - - run_package(str(workspace), str(output)) - result = run_package(str(workspace), str(output)) - - assert result.returncode == 0, result.stderr - assert 'Data/out.twbx' not in names(str(output)) diff --git a/src/plugin/skills/tableau-data-app-authoring/scripts/tests/test_wire_datasource.py b/src/plugin/skills/tableau-data-app-authoring/scripts/tests/test_wire_datasource.py index 7b600ca..7edbdd8 100644 --- a/src/plugin/skills/tableau-data-app-authoring/scripts/tests/test_wire_datasource.py +++ b/src/plugin/skills/tableau-data-app-authoring/scripts/tests/test_wire_datasource.py @@ -577,18 +577,3 @@ def test_wired_file_twb_packages_with_its_data(tmp_path): entries = archive.namelist() assert XLS_PATH in entries assert 'Data/orders.csv' in entries - - -def test_brackets_in_field_names_are_doubled(tmp_path): - add_data_file(tmp_path) - twb = write_twb(tmp_path) - fields = [{"name": "Sales [USD]", "datatype": "real", "role": "measure"}, - {"name": "Region", "datatype": "string", "role": "dimension"}] - - result = run_wire(twb, write_descriptor(tmp_path, excel_descriptor(fields=fields))) - - assert result.returncode == 0, result.stderr - wired = open(twb).read() - assert '[Sales [USD]]]' in wired - assert "name='[Sales [USD]]]'" in wired - assert "name='[sum:Sales [USD]]:qk]'" in wired diff --git a/src/plugin/skills/tableau-data-app-authoring/scripts/wire_datasource.py b/src/plugin/skills/tableau-data-app-authoring/scripts/wire_datasource.py index 9fab7bd..89a9e6e 100644 --- a/src/plugin/skills/tableau-data-app-authoring/scripts/wire_datasource.py +++ b/src/plugin/skills/tableau-data-app-authoring/scripts/wire_datasource.py @@ -115,10 +115,10 @@ def derive_field(field, ordinal, error_prefix=''): 'aggregation': aggregation, # role attribute: 0 = dimension, 1 = measure 'roleAttr': 1 if is_measure else 0, - 'localName': bracket(name), + 'localName': f'[{name}]', # column-instance derivation + name token: [sum:Profit:qk] / [none:Region:nk] 'derivation': 'Sum' if is_measure else 'None', - 'instanceName': bracket(f'sum:{name}:qk' if is_measure else f'none:{name}:nk'), + 'instanceName': f'[sum:{name}:qk]' if is_measure else f'[none:{name}:nk]', } From 0466a19318f59d4eca0d1117250e40348c31db12 Mon Sep 17 00:00:00 2001 From: Patrick Green Date: Fri, 23 Oct 2026 17:30:25 -0600 Subject: [PATCH 19/21] @W-24630680: Simplify package_twbx.py's walk and summary --- .../scripts/package_twbx.py | 24 +++++++------------ 1 file changed, 8 insertions(+), 16 deletions(-) diff --git a/src/plugin/skills/tableau-data-app-authoring/scripts/package_twbx.py b/src/plugin/skills/tableau-data-app-authoring/scripts/package_twbx.py index baa1f29..a26daa9 100644 --- a/src/plugin/skills/tableau-data-app-authoring/scripts/package_twbx.py +++ b/src/plugin/skills/tableau-data-app-authoring/scripts/package_twbx.py @@ -38,23 +38,15 @@ def die(message): sys.exit(1) -def is_cruft(rel_path): - return any(part.endswith('.DS_Store') or part.startswith('__MACOSX') for part in rel_path.split('/')) - - def collect_entries(workspace, top, allowed=None): entries = [] # (path, archive name, is_dir) for root, dirs, files in os.walk(os.path.join(workspace, top)): - dirs.sort() + dirs[:] = sorted(name for name in dirs if not name.startswith(('.', '__MACOSX'))) rel_root = os.path.relpath(root, workspace).replace(os.sep, '/') - if is_cruft(rel_root): - dirs[:] = [] - continue entries.append((root, rel_root, True)) - dirs[:] = [name for name in dirs if not name.startswith('.')] for name in sorted(files): rel = f'{rel_root}/{name}' - if name.startswith('.') or is_cruft(rel) or (allowed is not None and rel not in allowed): + if name.startswith('.') or (allowed is not None and rel not in allowed): continue entries.append((os.path.join(root, name), rel, False)) @@ -110,8 +102,9 @@ def main(): entries_by_top = {} for top in PACKED_DIRS: if os.path.isdir(os.path.join(workspace, top)): - entries_by_top[top] = collect_entries(workspace, top, referenced if top == 'Data' else None) - dirs_to_pack = [top for top, entries in entries_by_top.items() if entries] + entries = collect_entries(workspace, top, referenced if top == 'Data' else None) + if entries: + entries_by_top[top] = entries packed = {rel for entries in entries_by_top.values() for _, rel in entries} missing = sorted(referenced - packed) @@ -122,12 +115,11 @@ def main(): # Paths are workspace-relative so nothing lands under an / prefix. with zipfile.ZipFile(output, 'w', zipfile.ZIP_DEFLATED) as archive: archive.write(os.path.join(workspace, twb), twb) - for top in dirs_to_pack: - for path, rel in entries_by_top[top]: + for entries in entries_by_top.values(): + for path, rel in entries: archive.write(path, rel) - packed = ', '.join(f'{top}/' for top in dirs_to_pack) - print(f'✓ Packaged {twb} + {packed} into {output}', file=sys.stderr) + print(f"✓ Packaged {twb} + {', '.join(top + '/' for top in entries_by_top)} into {output}", file=sys.stderr) print(output) From 2d2cfefbf44d8060d0ad503e9c93575e2cd1ddea Mon Sep 17 00:00:00 2001 From: Patrick Green Date: Fri, 23 Oct 2026 17:38:37 -0600 Subject: [PATCH 20/21] @W-24630680: Use the published remote-type rule for file datasources too --- .../scripts/tests/test_wire_datasource.py | 6 +++--- .../tableau-data-app-authoring/scripts/wire_datasource.py | 8 +------- 2 files changed, 4 insertions(+), 10 deletions(-) diff --git a/src/plugin/skills/tableau-data-app-authoring/scripts/tests/test_wire_datasource.py b/src/plugin/skills/tableau-data-app-authoring/scripts/tests/test_wire_datasource.py index 7edbdd8..06cf936 100644 --- a/src/plugin/skills/tableau-data-app-authoring/scripts/tests/test_wire_datasource.py +++ b/src/plugin/skills/tableau-data-app-authoring/scripts/tests/test_wire_datasource.py @@ -448,8 +448,8 @@ def test_excel_file_wires_federated_datasource_with_matching_join_keys(tmp_path) assert relation.attrib == {'connection': key, 'name': 'Orders', 'table': '[Orders$]', 'type': 'table'} records = {r.findtext('remote-name'): r for r in root_ds.iter('metadata-record')} assert records['Sales'].findtext('remote-type') == '5' - assert records['Region'].findtext('remote-type') == '130' - assert records['Order Date'].findtext('remote-type') == '7' + assert records['Region'].findtext('remote-type') == '129' + assert records['Order Date'].findtext('remote-type') == '129' assert {r.findtext('parent-name') for r in records.values()} == {'[Orders]'} assert 'sqlproxy' not in wired @@ -476,7 +476,7 @@ def test_csv_file_splits_directory_and_filename(tmp_path): assert relation.get('table') == '[orders#csv]' records = {r.findtext('remote-name'): r for r in root_ds.iter('metadata-record')} assert records['Profit'].findtext('remote-type') == '5' - assert records['Ship Date'].findtext('remote-type') == '133' + assert records['Ship Date'].findtext('remote-type') == '129' assert {r.findtext('parent-name') for r in records.values()} == {'[orders.csv]'} diff --git a/src/plugin/skills/tableau-data-app-authoring/scripts/wire_datasource.py b/src/plugin/skills/tableau-data-app-authoring/scripts/wire_datasource.py index 89a9e6e..f336ac1 100644 --- a/src/plugin/skills/tableau-data-app-authoring/scripts/wire_datasource.py +++ b/src/plugin/skills/tableau-data-app-authoring/scripts/wire_datasource.py @@ -61,11 +61,6 @@ CONNECTION_CLASS_BY_EXTENSION = {'.xls': 'excel-direct', '.xlsx': 'excel-direct', '.xlsm': 'excel-direct', '.csv': 'textscan'} -REMOTE_TYPE_CODES = { - 'excel-direct': {'string': 130, 'integer': 20, 'real': 5, 'date': 7, 'datetime': 7, 'boolean': 11}, - 'textscan': {'string': 129, 'integer': 20, 'real': 5, 'date': 133, 'datetime': 135, 'boolean': 11}, -} - def die(message): print(f'✗ {message}', file=sys.stderr) @@ -279,11 +274,10 @@ def file_datasource_xml(ds): relation_name, table = filename, bracket(f'{stem}#{extension}') connection = (f"") - remote_types = REMOTE_TYPE_CODES[ds['class']] metadata_records = '\n'.join( f""" {esc(f['name'])} - {remote_types.get(f['datatype'], remote_types['string'])} + {5 if f['type'] == 'quantitative' else 129} {esc(f['localName'])} {esc(bracket(relation_name))} {esc(f['name'])} From 2401b458ba25f7ee19c19bf8db7283558ba5ea20 Mon Sep 17 00:00:00 2001 From: Patrick Green Date: Fri, 23 Oct 2026 18:18:09 -0600 Subject: [PATCH 21/21] @W-24630680: Move file formats into classes in file_formats.py; extract collect_packed_dirs Each embedded format is a class listing its extensions, so adding one means one class. The skill docs no longer name formats, and package_twbx.py's collect/verify step is its own function. --- .../tableau-data-app-authoring/SKILL.md | 16 +++--- .../scripts/file_formats.py | 45 +++++++++++++++++ .../scripts/package_twbx.py | 31 +++++++----- .../scripts/tests/test_wire_datasource.py | 2 +- .../scripts/wire_datasource.py | 50 +++++++------------ 5 files changed, 89 insertions(+), 55 deletions(-) create mode 100644 src/plugin/skills/tableau-data-app-authoring/scripts/file_formats.py diff --git a/src/plugin/skills/tableau-data-app-authoring/SKILL.md b/src/plugin/skills/tableau-data-app-authoring/SKILL.md index 398f8a3..70159b6 100644 --- a/src/plugin/skills/tableau-data-app-authoring/SKILL.md +++ b/src/plugin/skills/tableau-data-app-authoring/SKILL.md @@ -270,19 +270,17 @@ Trust that failure over patching the XML by hand. `datatype` maps to the column `type` (`real`/`integer` → quantitative, `date`/`datetime` → ordinal, else nominal); `role: "measure"` gets a `Sum` aggregation, `dimension` a `Count`. -**Embedded file datasource:** use this when the user hands you an Excel -(`.xls`, `.xlsx`, `.xlsm`) or CSV file instead of naming a published -datasource. The same script wires it; the descriptor just has `"type": "file"`. +**Embedded file datasource:** use this when the user hands you a data file +instead of naming a published datasource. The same script wires it (and rejects +file types it can't); the descriptor just has `"type": "file"`. 1. **Copy the file into the workspace** under `Data/`, conventionally `Data/Datasources/`. The script fails if the file isn't there, or if its path is outside `Data/`. -2. **Write a descriptor.** `file` is the path relative to the `.twb`. For - Excel, `table` is the sheet to query (sheet `Orders` → `"table": "Orders"`); - CSV has no `table`. Field names must match the column names Tableau gives - the file: usually the header row, but for CSV Tableau changes `[`/`]` to - `(`/`)`, names the columns of a file with no header row F1, F2…, and reads - decimal commas (`100,5`) as text. No server, site, or `repositoryId`: +2. **Write a descriptor.** `file` is the path relative to the `.twb`. `table` + picks the table in a file that holds several (an Excel sheet); the script + says when it's needed. Field names must match the column names Tableau reads + from the file, usually its header row. No server, site, or `repositoryId`: ```bash cat > "$WORK/descriptor.json" <<'DS_JSON' diff --git a/src/plugin/skills/tableau-data-app-authoring/scripts/file_formats.py b/src/plugin/skills/tableau-data-app-authoring/scripts/file_formats.py new file mode 100644 index 0000000..1ef9ab0 --- /dev/null +++ b/src/plugin/skills/tableau-data-app-authoring/scripts/file_formats.py @@ -0,0 +1,45 @@ +""" +File formats `wire_datasource.py` can embed. To add one, write a class like +these and list it in `FILE_FORMATS`. +""" + + +def bracket(name): + return '[' + name.replace(']', ']]') + ']' + + +class ExcelFile: + extensions = ('.xls', '.xlsx', '.xlsm') + connection_class = 'excel-direct' + + def __init__(self, path, table): + if not isinstance(table, str) or not table: + raise ValueError('Excel datasources need "table": the name of the sheet to query.') + self.path = path + self.relation_name = table + self.relation_table = bracket(f'{table}$') + + def connection_attrs(self): + return {'cleaning': 'no', 'compat': 'no', 'dataRefreshTime': '', 'filename': self.path, + 'interpretationMode': '0', 'password': '', 'server': '', 'validate': 'no'} + + +class TextFile: + extensions = ('.csv',) + connection_class = 'textscan' + + def __init__(self, path, table): + if table is not None: + raise ValueError('"table" applies only to Excel files; a CSV file is its own table.') + self.directory, self.filename = path.rsplit('/', 1) + stem, extension = self.filename.rsplit('.', 1) + self.relation_name = self.filename + self.relation_table = bracket(f'{stem}#{extension}') + + def connection_attrs(self): + return {'directory': self.directory, 'filename': self.filename, 'password': '', 'server': ''} + + +FILE_FORMATS = (ExcelFile, TextFile) + +FORMAT_BY_EXTENSION = {extension: file_format for file_format in FILE_FORMATS for extension in file_format.extensions} diff --git a/src/plugin/skills/tableau-data-app-authoring/scripts/package_twbx.py b/src/plugin/skills/tableau-data-app-authoring/scripts/package_twbx.py index a26daa9..6aa35e0 100644 --- a/src/plugin/skills/tableau-data-app-authoring/scripts/package_twbx.py +++ b/src/plugin/skills/tableau-data-app-authoring/scripts/package_twbx.py @@ -72,6 +72,23 @@ def referenced_data_files(workspace, twb): return referenced +def collect_packed_dirs(workspace, twb): + referenced = referenced_data_files(workspace, twb) + entries_by_top = {} + for top in PACKED_DIRS: + if os.path.isdir(os.path.join(workspace, top)): + entries = collect_entries(workspace, top, referenced if top == 'Data' else None) + if entries: + entries_by_top[top] = entries + + packed = {rel for entries in entries_by_top.values() for _, rel in entries} + missing = sorted(referenced - packed) + if missing: + die(f'{twb} references files missing from the workspace (no data at runtime): ' + + ', '.join(missing)) + return entries_by_top + + def main(): argv = sys.argv if len(argv) < 2 or len(argv) > 3: @@ -98,19 +115,7 @@ def main(): output = os.path.abspath(argv[2]) if len(argv) == 3 else os.path.join( os.path.dirname(workspace), os.path.basename(workspace) + '.twbx') - referenced = referenced_data_files(workspace, twb) - entries_by_top = {} - for top in PACKED_DIRS: - if os.path.isdir(os.path.join(workspace, top)): - entries = collect_entries(workspace, top, referenced if top == 'Data' else None) - if entries: - entries_by_top[top] = entries - - packed = {rel for entries in entries_by_top.values() for _, rel in entries} - missing = sorted(referenced - packed) - if missing: - die(f'{twb} references files missing from the workspace (no data at runtime): ' - + ', '.join(missing)) + entries_by_top = collect_packed_dirs(workspace, twb) # Paths are workspace-relative so nothing lands under an / prefix. with zipfile.ZipFile(output, 'w', zipfile.ZIP_DEFLATED) as archive: diff --git a/src/plugin/skills/tableau-data-app-authoring/scripts/tests/test_wire_datasource.py b/src/plugin/skills/tableau-data-app-authoring/scripts/tests/test_wire_datasource.py index 06cf936..d0c3ba5 100644 --- a/src/plugin/skills/tableau-data-app-authoring/scripts/tests/test_wire_datasource.py +++ b/src/plugin/skills/tableau-data-app-authoring/scripts/tests/test_wire_datasource.py @@ -521,7 +521,7 @@ def test_unsupported_file_type_is_rejected(tmp_path): result = run_wire(twb_path, write_descriptor(tmp_path, csv_descriptor(file='Data/orders.json'))) assert result.returncode == 1 - assert result.stderr.strip() == '✗ "file" must be an Excel (.xls, .xlsx, .xlsm) or CSV (.csv) file (got "Data/orders.json").' + assert result.stderr.strip() == '✗ "file" must end in .xls, .xlsx, .xlsm, .csv (got "Data/orders.json").' def test_excel_requires_a_table_and_csv_rejects_one(tmp_path): diff --git a/src/plugin/skills/tableau-data-app-authoring/scripts/wire_datasource.py b/src/plugin/skills/tableau-data-app-authoring/scripts/wire_datasource.py index f336ac1..86e83ce 100644 --- a/src/plugin/skills/tableau-data-app-authoring/scripts/wire_datasource.py +++ b/src/plugin/skills/tableau-data-app-authoring/scripts/wire_datasource.py @@ -41,7 +41,7 @@ { "type": "file", "caption": "Sample - Superstore", - "file": "Data/Datasources/Sample - Superstore.xls", // .xls, .xlsx, .xlsm, or .csv + "file": "Data/Datasources/Sample - Superstore.xls", // Excel or CSV "table": "Orders", // Excel only: the sheet "fields": [ ...as above... ] } @@ -56,12 +56,11 @@ import sys import xml.etree.ElementTree as ET +from file_formats import FORMAT_BY_EXTENSION, bracket + # The exact empty anchor emitted by the scaffold template. Matched literally. EMPTY_ANCHOR = '' -CONNECTION_CLASS_BY_EXTENSION = {'.xls': 'excel-direct', '.xlsx': 'excel-direct', '.xlsm': 'excel-direct', '.csv': 'textscan'} - - def die(message): print(f'✗ {message}', file=sys.stderr) sys.exit(1) @@ -175,16 +174,13 @@ def parse_file_datasource(descriptor, error_prefix): if len(parts) < 2 or parts[0] != 'Data' or any(part in ('', '.', '..') for part in parts): die(f'{error_prefix}"file" must be a path inside the workspace Data/ folder, ' f'e.g. "Data/Datasources/Sales.xlsx" (got "{descriptor["file"]}").') - file_class = CONNECTION_CLASS_BY_EXTENSION.get(os.path.splitext(file_path)[1].lower()) - if not file_class: - die(f'{error_prefix}"file" must be an Excel (.xls, .xlsx, .xlsm) or CSV (.csv) file (got "{file_path}").') - - table = descriptor.get('table') - if file_class == 'excel-direct': - if not isinstance(table, str) or not table: - die(f'{error_prefix}Excel datasources need "table": the name of the sheet to query.') - elif table is not None: - die(f'{error_prefix}"table" applies only to Excel files; a CSV file is its own table.') + file_format = FORMAT_BY_EXTENSION.get(os.path.splitext(file_path)[1].lower()) + if not file_format: + die(f'{error_prefix}"file" must end in {", ".join(FORMAT_BY_EXTENSION)} (got "{file_path}").') + try: + source = file_format(file_path, descriptor.get('table')) + except ValueError as error: + die(f'{error_prefix}{error}') fields = parse_fields(descriptor, error_prefix) @@ -192,8 +188,7 @@ def parse_file_datasource(descriptor, error_prefix): 'type': 'file', 'caption': descriptor['caption'], 'file': file_path, - 'class': file_class, - 'table': table, + 'source': source, 'fields': fields, 'connectionName': None, # always generated } @@ -244,7 +239,7 @@ def parse_descriptor(descriptor): if ds['type'] == 'published': ds['relationConnection'] = ds['connectionName'] else: - ds['relationConnection'] = f"{ds['class']}.{ds['connectionName'].split('.', 1)[1]}" + ds['relationConnection'] = f"{ds['source'].connection_class}.{ds['connectionName'].split('.', 1)[1]}" return datasources @@ -259,27 +254,18 @@ def root_datasource_xml(ds): return published_datasource_xml(ds) if ds['type'] == 'published' else file_datasource_xml(ds) -def bracket(name): - return '[' + name.replace(']', ']]') + ']' +def file_connection_xml(source): + attrs = ' '.join(f"{name}='{esc(value)}'" for name, value in source.connection_attrs().items()) + return f"" def file_datasource_xml(ds): - if ds['class'] == 'excel-direct': - relation_name, table = ds['table'], bracket(f"{ds['table']}$") - connection = (f"") - else: - directory, filename = ds['file'].rsplit('/', 1) - stem, extension = filename.rsplit('.', 1) - relation_name, table = filename, bracket(f'{stem}#{extension}') - connection = (f"") metadata_records = '\n'.join( f""" {esc(f['name'])} {5 if f['type'] == 'quantitative' else 129} {esc(f['localName'])} - {esc(bracket(relation_name))} + {esc(bracket(ds['source'].relation_name))} {esc(f['name'])} {f['ordinal']} {esc(f['datatype'])} @@ -292,10 +278,10 @@ def file_datasource_xml(ds): - {connection} + {file_connection_xml(ds['source'])} - + {metadata_records}