feat(desktop): add individual datasource tools - #875
Conversation
tableaukyler
left a comment
There was a problem hiding this comment.
Reviewed the three new Desktop datasource tools (get-datasource-info, get-datasource-xml, apply-datasource) plus the new cachePath.ts TOCTOU-safe cache read. The TOCTOU work itself is solid, and datasourceResult.ts's field allowlist correctly keeps metadata credential-safe — but that discipline doesn't extend to the raw XML path, which is the one 🔴 below.
🔴 Raw datasource XML (passwords/oauth tokens) goes out unredacted, in both modes
finishXmlRead (src/tools/desktop/api/xmlReadResult.ts) never redacts the xml it returns inline or writes to the cache file — only the prose summary (artifactSummary.ts) is scrubbed. The PR's own test proves it: getDatasourceXml.test.ts builds a <connection password="must-not-appear"/> datasource, and in file mode asserts readFileSync(body.file, 'utf-8')).toBe(xml) — the password lands on disk verbatim — while only checking the message doesn't contain it. Inline mode (datasourceXml: xml a few lines above) has the same gap, putting the raw credential straight into the tool response / conversation. Worksheet/dashboard/workbook reuse the same unredacted path safely because their XML doesn't carry connection attributes; this PR is what first routes credential-bearing XML through it.
🟠 The new TOCTOU-safe cache read only protects apply-datasource
runApplyPreamble's secureContainedCacheRead defaults to false; only applyDatasource.ts passes true. applyWorksheet.ts/applyWorkbook.ts/applyDashboard.ts/applyStoryboard.ts still go through the legacy existsSync/readFileSync branch with zero containment check — the exact class of risk cachePath.ts was built to close. Not a regression (pre-existing behavior), but worth a follow-up to backfill the other four rather than leaving datasource as the only hardened tool.
🟠 New routes skip step 4 of the External Client API recipe
workbookDatasource/workbookDatasourceDocument (gated minApiVersion: '0.2.10') complete steps 1-3 of externalApi/README.md's recipe cleanly, but externalApiContract.test.ts's route list and the __fixtures__/externalClientApi-openapi.json fixture (still pinned to spec 0.2.9) were never updated, so these two routes have no contract coverage against the real API spec.
🟡 Two Template Method duplications (both ≥80 on the design-pattern rubric, inline below):
xmlReadResult.ts's datasource cap-message forksbuildInlineCapFileMessage's skeleton instead of parameterizing it.applyPreamble.ts's new secure branch duplicates the legacy branch's 5-step skeleton (including the literalNotFoundErrorternary) instead of factoring one skeleton with a pluggable read step.
Everything else came back clean: tool registration, comment discipline, blast-radius on checkSidecar/ArtifactKind/attr() regex/sendDocumentApply, and no regression of prior traversal-guard or credential-safety fixes. (One bug candidate — a malformed % crashing canonicalDatasourceSegment — didn't hold up under verification: the datasource id is always API-supplied and pre-encoded, never raw user text, so it's not reachable from ordinary input.)
No docs action — tableau-mcp isn't in the mapped docs-repo areas.
🤖 Posted by KylerGPT — an AI reviewer trained on Kyler's review history. Kyler reviewed and approved this before posting.
mattcfilbert
left a comment
There was a problem hiding this comment.
MattGPT review of 7fd73e87, using the Andy lens.
Michael, the datasource tools fit the existing read/apply flow, and the version gates and wire contracts match the native API. The earlier cache-read and contract-test fixes are in place.
No P0/P1 findings. Approving with one non-blocking P2 on fallback redaction of connection strings.
Verified the full unit suite (7,108 tests), typecheck, changed-file lint, and the merged native route/export contract. No live Desktop 0.2.10 round-trip or connector-specific credential audit was run.
Decision
RULE WE NOW KEEP: Desktop datasource tools resolve through workbook inventory, expose credential-redacted datasource XML, and apply cached XML only through contained reads.
Description
get-datasource-info,get-datasource-xml, andapply-datasourcefor External Client API 0.2.10 and newer.datasource-not-foundproblem code in the External Client API contract fixture.Motivation and Context
The individual datasource routes let an agent inspect or update one workbook-local datasource without reading or replacing the whole workbook document. Credential redaction keeps connection secrets out of MCP responses and cache files, while contained reads prevent cached apply paths from escaping the Desktop cache.
Type of Change
How Has This Been Tested?
npx --no-install vitest run src/desktop/cachePath.test.ts src/desktop/externalApi/datasourceRoutes.test.ts src/desktop/externalApi/externalApiToolExecutor.test.ts src/desktop/limits/artifactSummary.test.ts src/desktop/wrappers/cacheFingerprint.test.ts src/desktop/wrappers/applyDatasourceXml.test.ts src/server.desktop.test.ts src/tools/desktop/api/applyDatasource.test.ts src/tools/desktop/api/applyPreamble.test.ts src/tools/desktop/api/datasourceResult.test.ts src/tools/desktop/api/getDatasourceInfo.test.ts src/tools/desktop/api/getDatasourceXml.test.ts src/tools/desktop/api/listWorkbookDatasources.test.ts src/tools/desktop/api/resolveDatasourceRef.test.ts --maxWorkers=1npx vitest run src/desktop/externalApi/externalApiContract.test.ts src/desktop/limits/datasourceCredentialRedaction.test.ts src/desktop/limits/inlineXmlCap.test.ts src/tools/desktop/api/getDatasourceXml.test.ts src/tools/desktop/api/applyPreamble.test.ts src/tools/desktop/api/applyDatasource.test.ts src/tools/desktop/api/applyWorkbook.test.ts src/tools/desktop/api/applyWorksheet.test.ts src/tools/desktop/api/applyDashboard.test.ts src/tools/desktop/api/applyStoryboard.test.tsnpx vitest run src/desktop/externalApi/externalApiToolExecutor.test.tsnpx vitest run src/desktop/externalApi/externalApiHttpAsyncDispatch.test.tsnpx vitest run --config ./vitest.config.tsRelated Issues
Relates to #146.
Checklist
npm run version. For example,use
npm run version:patchfor a patch version bump.environment variable or changing its default value.
Contributor Agreement
By submitting this pull request, I confirm that:
its Contribution Checklist.