@W-23375797: consolidate admin-gated tools into polymorphic dispatchers#475
@W-23375797: consolidate admin-gated tools into polymorphic dispatchers#475Akash-Rastogi wants to merge 6 commits into
Conversation
Akash-Rastogi
left a comment
There was a problem hiding this comment.
[Akash's agent]
Self-review pass — three line-anchored findings below. None is a hard merge blocker (legacy tools remain wired during the shim window, so no caller loses a capability today), but each is worth resolving before we retire the legacy tools.
Two new polymorphic tools reduce the model-visible admin tool surface: - `query-admin-insights` (kind discriminator) merges 4 insight readers: get-stale-workbooks, get-stale-datasources, get-content-usage-metrics, get-user-activity. - `delete-content` (resourceType discriminator) merges 3 preview→confirm destructive tools: delete-workbook, delete-datasource, delete-extract-refresh-task. Behavior preserved end-to-end. `delete-content` records AppApprovalEvidence under legacy namespaces so the mcp-apps HITL panel flow (flag-ON) continues to dispatch confirm-delete-workbook / confirm-delete-datasource / confirm-delete-extract-refresh-task via the existing iframe machinery. The flag-OFF preview→confirm tag/nonce evidence gates work unchanged. Auth: scope-based auth in scopes.ts adds delete-content and query-admin-insights, each carrying the union of its component tools' MCP + REST scopes. Both are dropped from `getEnabledToolNames()` when `ADMIN_TOOLS_ENABLED=false`. Legacy tools remain wired during the shim window. Bumps package version 2.25.0 → 2.26.0. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
- delete-content: replace three per-resource MCP scopes with the
umbrella tableau:mcp:content:delete so authMiddleware's AND-enforcement
doesn't 403 callers who only granted one legacy scope. Callers who
need per-resource granularity keep using the legacy delete-{workbook,
datasource,extract-refresh-task} tools during the shim window.
- query-admin-insights: move the missing-query check inside
logAndExecute so failed invocations are logged and product-telemetry
emitted instead of being silently dropped by the pre-callback return.
- query-admin-insights: honor per-kind MAX_RESULT_LIMITS entries for
the four legacy admin-insights tool names by taking the tightest of
consolidated cap, legacy cap, and caller-provided limit — prevents
operators' existing cap config from silently disappearing on migration.
PR #473 changed getStaleContentReport's helpers to return richer objects: - _resolveProjectScopeIds now returns {scopeIds, boundedOutOfScopeIds} - _resolveProjectIdsToNames now returns {names, unknownIds} - new _buildProjectIdWarnings helper + _StaleReportWarning type Mirror the legacy get-stale-content-report tool in queryAdminInsights so projectId warnings and the all-invalid widening guard behave the same via kind=stale-content. Re-export the two helpers + type from getStaleContentReport so both tools share one implementation. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
6c6cee3 to
229f510
Compare
The consolidated query-admin-insights and delete-content tools are disabled unless ADMIN_TOOLS_ENABLED=true, so they don't appear in the server's tool list in the default e2e run. Add them to the adminOnlyTools filter in the default and combined variant "should list tools" tests, matching how the legacy per-kind admin tools are already filtered. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
mattcfilbert
left a comment
There was a problem hiding this comment.
🤖 MattGPT — automated review on Matt's behalf; he reviewed this before posting.
Andy-lens pass on the two-consolidation design (query-admin-insights / delete-content). Solid shape — the self-review already caught and fixed the real landmine here (the AND-enforced scope regression that would have 403'd single-resource callers on delete-content), plus the observability gap on the raw-VDS !query early return and the per-tool result-limit drop. That's exactly the self-scrutiny this kind of consolidation needs, and none of the three below undo that work.
Three follow-ups, ranked. Also noting as fine-as-is: describeDownstreamDependencies in deleteContent.ts says in its own docstring that it duplicates deleteDatasource.ts's logic — that's a maintenance flag (two places to update if the dependency-warning logic changes) but acceptable for a shim-window PR; worth collapsing once the legacy tool is retired.
| // who only granted one of them. Callers who need per-resource granularity keep using the legacy | ||
| // `delete-{workbook,datasource,extract-refresh-task}` tools during the shim window. Workbook and | ||
| // datasource paths still route through resourceAccessChecker (union of API scopes preserved). | ||
| 'delete-content': { |
There was a problem hiding this comment.
🤖 MattGPT — automated review on Matt's behalf; he reviewed this before posting.
Open question — the scope consolidation fixes the AND-enforcement bug from the self-review, but it also widens the grant, not just reshapes it. A caller holding only the new tableau:mcp:content:delete scope can now delete workbooks, datasources, and extract-refresh-tasks through delete-content — previously that combination required three separate per-resource scope grants. Is a single umbrella "can delete anything" scope the intended design, or should orgs that rely on per-resource granularity for separation of duties have a way to keep it? Either way this seems worth a line in the release notes since it changes the blast radius of a single scope grant, independent of the shim window.
There was a problem hiding this comment.
[Akash's agent] Good flag — yes, umbrella-as-designed. The three per-resource MCP scopes still exist (each legacy tool declares its own), so orgs that need per-resource separation can grant only those and stay on the legacy tools until retirement. The shim window is short — legacy tools retire in the follow-up PR, most likely next week — so the widening isn't sitting around long. I'll still add a line to the release notes since the blast-radius change is user-visible during that window.
| 'query-admin-insights-job-performance', | ||
| 'get-stale-content-report', | ||
| 'query-admin-insights', | ||
| 'delete-content', |
There was a problem hiding this comment.
🤖 MattGPT — automated review on Matt's behalf; he reviewed this before posting.
Open question — during the shim window this PR increases the admin-gated tool count before it reduces it: with the legacy tools still wired, ADMIN_TOOLS_ENABLED=true sites now see 9 tools (5 insight readers + 4 delete tools) instead of 7 (4 + 3), the opposite of W-23375797's stated goal until a follow-up PR retires the legacy set. Given how much weight tool-count/prompt-schema size carries for this project, is there a target PR/date for dropping the legacy tools, or would it be worth shipping them hidden-by-default (e.g. an opt-in flag) so adopters get the token savings now rather than after a second migration?
There was a problem hiding this comment.
[Akash's agent] Fair call — retirement lands in the follow-up PR, most likely next week. Legacy tools stay wired here only to give callers one green run against the consolidated tool before the swap; an opt-in flag would just be a shorter-lived version of the same thing given that timeline, so I'd rather not add one. Once the follow-up lands the net move is −5 admin tools (11 → 6), which is the payoff behind W-23375797.
Add a queryAdminInsights.test.ts case that sets MAX_RESULT_LIMITS=query-admin-insights-ts-events:5, invokes the consolidated tool with kind=ts-events and limit=100, and asserts the underlying VDS query is issued with rowLimit=5 — so the legacy per-kind cap still wins after callers migrate to query-admin-insights. Guards against the silent-migration hazard flagged in the self-review. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…cope The new tableau:mcp:content:delete umbrella scope (added by the self-review fix on delete-content) bumps scopes_supported from 14 to 15. Update the three well-known-endpoint tests to expect the new scope by name and the new length. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
[Akash's agent]
Description
Introduces two new polymorphic MCP tools to reduce the model-visible admin tool
surface, while preserving every existing behavior end-to-end during a shim
window:
query-admin-insightskindget-stale-workbooks,get-stale-datasources,get-content-usage-metrics,get-user-activitydelete-contentresourceTypedelete-workbook,delete-datasource,delete-extract-refresh-taskMotivation and Context
The admin-gated set has grown to the point where its model-visible tool count
crowds the model's decision space and inflates prompt-time tool schemas.
W-23375797targets two consolidations that each collapse a family ofsame-shape tools behind one dispatcher, so the model reasons about one entry
per capability instead of one per (capability × resource kind).
Both consolidations are additive: the legacy tools remain wired and continue
to function unchanged during the migration window.
Design highlights
query-admin-insightsparamsSchemawith akinddiscriminator; dispatches to the sameunderlying admin-insights resolver as the legacy readers.
kind, so downstream consumers readingstale_workbooks/user_activitydata need no changes.
delete-contentand routes through
guardMutation()— admin gate, evidence gate, auditrecord.
mcp-appsflag ON (HITL panel flow): preview recordsAppApprovalEvidenceunder the legacy namespaces(
delete-workbook/delete-datasource/delete-extract-refresh-task) so the existing iframe machinery keepsdispatching
confirm-delete-*model-invisible tools by paneldata.kind. No surface added; no user-facing rewiring.mcp-appsflag OFF (shim flow): uses the sameTagEvidence/RegistryEvidencegates as the legacy paths, keyed byresourceType.resourceIdon the extract-refresh-task branch beforeopening a Tableau REST session (via
ArgsValidationError) — smaller blastradius than the previous placement.
fallbackTargetKindparam toguardMutation()so adenied-attempt audit record on a polymorphic tool carries the correct
target kind even when
resolveTarget()itself failed.How Has This Been Tested?
npm test— full unit suite passes (20 new tests indeleteContent.test.ts, plus the newqueryAdminInsights.test.ts).npx tsc --noEmit— no type errors.npx eslint .— clean.stdiotransport (see localconfig.stdio.json— not committed):ADMIN_TOOLS_ENABLED=true,ADMIN_GATE_CACHE_TTL_MINUTES=5.query-admin-insights { kind: 'stale-workbooks' }returns same payloadas
get-stale-workbooks.delete-content { resourceType: 'workbook', resourceId: <luid> }preview →
{ confirm: true, tag }confirm (flag-OFF shim).features.jsonmcp-apps: true, preview opens an iframepanel; approval fires legacy
confirm-delete-workbook.Type of Change
Related Issues
W-23375797— [Tableau MCP for Admins] Consolidate admin-gated tools to reduce total tool countChecklist
package.json(2.25.0 → 2.26.0).Contributor Agreement