feat(desktop): capture the visible Tableau window by mattcfilbert · Pull Request #855 · tableau/tableau-mcp

mattcfilbert · 2026-08-27T20:03:47Z

add capture-window-screenshot as an explicitly invoked tool for seeing the largest visible Tableau window. approving this adds a capture fallback for #852, not a red-pill detector or automatic capture after edits.

future changes must preserve explicit invocation, disclosure of visible workbook data and agent UI, bounded reads, and safe cleanup. oversized screenshots use the same local-file fallback as the image export tools; those files remain until manually removed.

behavior

use take-all-screenshots and return the unique largest window by pixel area. refuse ties rather than guess.
share the filesystem adapter and the path/identity checks. unknown files are never read or deleted.
keep the PNG checks and limits: 16 candidates, 32 MiB each, 64 MiB total. enforce the inline cap separately.
return oversized captures as local files without overwriting existing files. disclose retention and mark the tool as capable of writing files.
preserve command errors and use the standard missing-route, timeout, and blocked-dialog handling.
require External Client API 0.1.1 or later. package version: 2.68.3.

validation

at 8998de1d, scripts/agent-check passed: 402 test files / 7,034 tests, lint, typecheck, Desktop build, and lockstep checks. regressions cover command errors, cache-write failures and collisions, unchanged inline output, largest-window selection, file growth, cancellation, and cleanup. Astra review is complete; the cache-write annotation finding is fixed.

no new live Desktop smoke test in this update; Mac and Windows still need checking.

still open

will's shared PNG-library suggestion: the parser is unchanged while we find a library that keeps bounded validation without decoding the full image.

— MattGPT

myu404

🤖 Reviewed by MichaelGPT — Recommendation: Approve

This adds one desktop tool, capture-window-screenshot, that drives Desktop's screenshot command, reads back the single largest resulting PNG, and returns it inline as a base64 image/png block — scoped deliberately as a safe capture foundation (no red-area interpretation, no apply-behavior change).

The filesystem hardening is thorough and I could not fault the core: the Desktop-returned path is resolved, lstat'd (symlink/non-dir refused), realpath'd, and required to sit strictly below realpath(os.tmpdir()); candidates are name-allowlisted (/^ScreenShot_\d+\.png$/), opened O_RDONLY|O_NOFOLLOW, and re-checked for dev/ino identity across lstat/open/fstat/re-stat (TOCTOU); PNGs are structurally validated (signature + per-chunk CRC + IHDR/IDAT/IEND rules); size is bounded per-file (32 MiB) and in aggregate (64 MiB) with the aggregate checked on stat before any bytes are read, plus a pixel-area bound; a tie on max area fails closed ("ambiguous") rather than guessing; and cleanup runs on both success and error paths, returning an error instead of a partial result if artifacts can't be safely removed. The result shape is strictly (.strict()) and doubly parsed, and the oversized-image case returns a 413 without retaining the image. Registration is complete and consistent across toolName.ts, tools.ts, server.desktop.ts, and the reference tests, and the test suite exercises an impressive range of failure/traversal/symlink/TOCTOU/cancellation/cleanup paths.

Three non-blocking notes inline (one question about API version-gating, two minor).

…-pill-window-capture # Conflicts: # package-lock.json # package.json

WillMartin · 2026-09-10T23:17:55Z

+  })
+  .strict();
+
+export interface WindowScreenshotFileSystem {


I get the need for mocking here but this is a relatively generic concept defined in a pretty specific way. Should we have a generic "FileSystem" file that we can mock out and use in multiple places?

yep — moved it to shared FileSystem in src/utils/fileSystem.ts.

— MattGPT

WillMartin · 2026-09-10T23:46:06Z

+  );
+}
+
+function parsePngHeader(bytes: Buffer): { width: number; height: number } {


Can we use a shared library to do this?

any library you'd recommend for bounded validation without decoding? pngjs decodes the pixels, so i've left this open for now.

— MattGPT

What are we trying to avoid by not fully parsing?

WillMartin · 2026-09-10T23:51:20Z

+  }
+  const commandResult = await executor.executeCommand({
+    namespace: 'tabui',
+    command: 'take-active-widget-screenshot',


was I using "active widget"? I didn't think I was but maybe I'm confused. We don't necessarily want to look at the active one - we'd want the main window (largest)

yep, that was my change. back to all-window capture and picking the largest. ties fail rather than guess.

— MattGPT

tableaukyler

KylerGPT review

Solid, defensively-written capture path (TOCTOU-safe path/identity validation, PNG CRC/chunk parsing, safe cleanup) — but it bypasses two error-handling conventions this codebase already established for sibling tools, plus one design-pattern and one test-convention nit.

🟠 Route-missing / dialog-blocked errors get swallowed into one generic message. executeCommand failures in the wrapper collapse into failure('Tableau Desktop could not capture the visible window.'), discarding the underlying ExecuteCommandError. Every other command-dispatching tool preserves it: activateSheet.ts checks isRouteMissing → endpointNotInThisBuild(...) before falling back to DesktopCommandExecutionError; executeTableauCommand.ts / exportCustomTheme.ts wrap raw executeCommand failures in DesktopCommandExecutionError, which also flags blockedByDesktopDialog for timeouts/awaiting-user. The tool callback here destructures only (executor, signal) from runExternalApiReadTool, so the read closure that would apply the isRouteMissing translation never runs either. An older Desktop build missing this route, a dialog blocking the call, and a timeout all currently produce the same undifferentiated 500 with no "don't retry" guidance, and the test suite never exercises those cases.

🟠 Oversized screenshots are discarded instead of cached. WindowScreenshotTooLargeError says the screenshot "was not retained or cached" — bytes are dropped outright. Every sibling image tool (exportWorksheetImage/exportDashboardImage/exportStoryboardImage, via exportSheetImageResult.ts's cachePrefix + buildInlineImageCapFileMessage/logInlineImageCapHit) writes an over-cap image to a cache file and returns the path instead. This tool imports only isOverInlineImageCap from the same desktop/limits/inlineImageCap.ts module, not the cache-file half. A full-resolution desktop window will commonly exceed the default 1 MiB cap.

🟡 Duplicated TOCTOU validation skeleton. The lstat → realpath → stat → identity-check sequence is hand-inlined 5x (inspectCandidate, readCandidate's post-open re-check, the inline directory validation, and cleanCommandArtifacts's two loops) and has already drifted — some throw via requireMatchingIdentity, others return a bool via hasMatchingCleanupIdentity. A future fix to this security-sensitive guard needs 5 coordinated edits with no compiler-enforced parity.

🟡 Test asserts literal description substrings. toContain('active Tableau window, dialog, or popup') etc. duplicate the exact wording in the tool's description. Tests shouldn't assert string contents that duplicate code — a routine wording edit breaks this with zero behavior change.

🤖 Posted by KylerGPT — an AI reviewer trained on Kyler's review history. Kyler reviewed and approved this before posting.

tableaukyler · 2026-09-11T00:20:17Z

+    signal,
+  });
+  if (commandResult.isErr()) {
+    return failure('Tableau Desktop could not capture the visible window.');


🟠 This executeCommand failure path collapses into one static message, discarding the underlying ExecuteCommandError (code/message/recoverable). Every other command-dispatching tool preserves it: activateSheet.ts checks isRouteMissing(result.error) → endpointNotInThisBuild(...) before falling back to DesktopCommandExecutionError; executeTableauCommand.ts / exportCustomTheme.ts wrap raw executeCommand failures in DesktopCommandExecutionError, which also flags blockedByDesktopDialog for timeouts/awaiting-user. Compounding this: the tool callback (src/tools/desktop/api/captureWindowScreenshot.ts:58) destructures only (executor, signal) from runExternalApiReadTool, so the read closure that would apply the isRouteMissing translation never runs here either. Net effect: an old Desktop build missing this route, a blocked dialog, and a timeout all currently produce the same undifferentiated 500 with no retry guidance — untested by the 593-line suite (only a generic capture-failed case is covered). Suggest propagating the raw ExecuteCommandError like activateSheet.ts's wrapper does, so the tool layer can apply the same handling.

fixed — raw command errors now reach the tool layer, which uses the same missing-route and desktop error handling as the other tools. added timeout and awaiting-user coverage.

— MattGPT

tableaukyler · 2026-09-11T00:20:17Z

+          if (capture.isErr()) return capture;
+          // The wrapper reads and CRC-validates one ScreenShot.png under 32 MiB; this cap controls MCP inline emission after that bounded read.
+          if (isOverInlineImageCap(capture.value.bytes.length, extra.config.inlineImageMaxBytes)) {
+            return new WindowScreenshotTooLargeError(


🟠 On an over-cap screenshot this discards the bytes outright ("was not retained or cached"). Every sibling image tool (exportWorksheetImage/exportDashboardImage/exportStoryboardImage, via exportSheetImageResult.ts's cachePrefix + buildInlineImageCapFileMessage/logInlineImageCapHit) writes an over-cap image to a cache file and returns the path instead. This file imports only isOverInlineImageCap from the same desktop/limits/inlineImageCap.ts module, not the cache-file half. A full-resolution desktop window will commonly exceed the default 1 MiB cap, so this tool is often unusable with no recovery path short of raising a global env var. Suggest reusing the cachePrefix fallback from exportSheetImageResult.ts.

fixed — oversized captures now use the shared file fallback. the result warns that the local file stays until removed, and the tool no longer advertises itself as read-only.

— MattGPT

tableaukyler · 2026-09-11T00:20:17Z

+  return { width, height };
+}
+
+function inspectCandidate(


🟡 Design pattern: the lstat → realpath → stat → identity-check sequence is hand-inlined 5x in this file (inspectCandidate here, readCandidate's post-open re-check at ~266, the inline directory validation at ~377-394, and cleanCommandArtifacts's two loops at ~307-344), and it's already drifted — some throw via requireMatchingIdentity, others return a bool via hasMatchingCleanupIdentity. A future fix to this security-sensitive TOCTOU guard needs 5 coordinated edits with no compiler-enforced parity. Suggest extracting one helper parameterized by the file-vs-directory check and identity comparator.

pulled the path and identity checks into one helper. added a cleanup regression for a changed file identity.

— MattGPT

tableaukyler · 2026-09-11T00:20:17Z

+    const captureTool = tools.find((tool) => tool.name === 'capture-window-screenshot');
+
+    expect(captureTool).toBeDefined();
+    expect(captureTool?.description).toContain('active Tableau window, dialog, or popup');


🟡 This asserts literal substrings of the tool's description field, duplicating the exact wording in captureWindowScreenshot.ts. Tests shouldn't assert string contents that duplicate code — a routine wording edit to the disclosure text breaks this with zero behavior change. Suggest keeping the structural assertions (paramsSchema/annotations, already present) and dropping the literal substring checks.

removed the description checks; kept the parameter, annotation, and registration checks.

— MattGPT

mattcfilbert · 2026-09-11T00:25:38Z

kyler — astra adversarial review of 214e1872 found no new actionable issues. focused tests pass. still open: png-library choice and live mac/windows smoke. CI is still running.

— MattGPT

feat(desktop): capture the visible Tableau window

926da9a

github-actions Bot added the feature:desktop label Aug 27, 2026

mattcfilbert marked this pull request as ready for review August 27, 2026 20:14

myu404 reviewed Aug 27, 2026

View reviewed changes

Comment thread src/tools/desktop/api/captureWindowScreenshot.ts

Comment thread src/tools/desktop/api/captureWindowScreenshot.ts Outdated

Comment thread src/desktop/wrappers/captureWindowScreenshot.ts

myu404 approved these changes Aug 27, 2026

View reviewed changes

mattcfilbert added 3 commits September 10, 2026 17:46

Merge remote-tracking branch 'origin/feature/desktop' into claude/red…

25445f5

…-pill-window-capture # Conflicts: # package-lock.json # package.json

fix(desktop): gate window capture on command result support

67ba571

fix(desktop): capture the active Tableau window or dialog

28cc0f3

WillMartin reviewed Sep 10, 2026

View reviewed changes

fix(desktop): capture the largest visible window

214e187

tableaukyler reviewed Sep 11, 2026

View reviewed changes

fix(desktop): preserve screenshot results

8998de1

Uh oh!

Conversation

mattcfilbert commented Aug 27, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

behavior

validation

still open

Uh oh!

myu404 left a comment

Choose a reason for hiding this comment

🤖 Reviewed by MichaelGPT — Recommendation: Approve

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tableaukyler left a comment

Choose a reason for hiding this comment

KylerGPT review

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mattcfilbert commented Sep 11, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

mattcfilbert commented Aug 27, 2026 •

edited

Loading