Skip to content

feat(desktop): handle blocking dialogs safely - #901

Merged
myu404 merged 4 commits into
tableau:feature/desktopfrom
myu404:codex/handle-desktop-dialogs
Sep 11, 2026
Merged

feat(desktop): handle blocking dialogs safely#901
myu404 merged 4 commits into
tableau:feature/desktopfrom
myu404:codex/handle-desktop-dialogs

Conversation

@myu404

@myu404 myu404 commented Sep 10, 2026

Copy link
Copy Markdown

Decision

Desktop agents inspect the exact current dialog and invoke at most one exact returned action; when they cannot establish a safe action, they hand control to the user.

Description

  • Add get-active-dialogs and invoke-dialog-action tools over the Desktop External Client API 0.2.13 dialog routes.
  • Relay visible dialog context, exact labels and semantic actions, clean empty states, and indeterminate-action outcomes without guessing or blindly retrying.
  • Add conservative timeout and recovery guidance, generated contract coverage, and registration tests.
  • Keep the dynamic Desktop profile at 67 tools after incorporating the latest base-branch tools.
  • Bump the package version to 2.68.6. This change introduces no breaking changes.

Known Limitation

Dialog inspection and action invocation cover Tableau-owned application dialogs. Dialogs handed off to OS platform-native controls, such as file choosers or print dialogs, are outside the External Client API and require user interaction or separate OS automation.

Motivation and Context

Tableau Desktop operations can surface application-owned dialogs that block subsequent agent interaction. Agents need structured evidence about the active dialog and a guarded way to invoke one explicit available action, with a human fallback when safe automation is not possible.

Type of Change

  • Bug fix
  • New feature
  • Breaking change
  • Documentation update
  • Other (please describe):

How Has This Been Tested?

  • scripts/agent-check

Related Issues

Checklist

  • I have updated the version in the package.json file by using npm run version. For example,
    use npm run version:patch for a patch version bump.
  • I have made any necessary changes to the documentation
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes
  • I have documented any breaking changes in the PR description. For example, renaming a config
    environment variable or changing its default value.

Contributor Agreement

By submitting this pull request, I confirm that:

@mattcfilbert mattcfilbert left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Andy-lens review (HEAD f686a76)

Strong PR, Michael. The safety core is right: the "indeterminate outcome" handling that treats a 5xx / lost-socket / timeout after the POST as maybe-fired and forbids a second click, while keeping the pre-dispatch api-disabled 503 definitive, is exactly the conservative call this tool needs. The 0.2.12 version gate (with the 0.2.11-hides / 0.2.12-exposes test), the log redaction of dialog identity and label, and the contract test pinning the real 0.2.12 fixture are all credit-worthy. Test coverage is thorough.

One thing to fix before merge, and two questions.

Please (before merge)

  • Version collision with #875. This PR and #875 both bump to 2.68.1, and the base (feature/desktop) is at 2.68.0. Two PRs making the identical x → x+1 edit merge clean with no conflict, and CI has no version-increment guard — so whichever lands second ships silently under a duplicate version, two code states under one number. Also, #884#889 currently sit at 2.67.6 / 2.67.7, below the base. Rebase the stack so each PR lands with a distinct, ascending version.

Questions (your call)

  • Does a 401 from invokeDialogAction guarantee the action never dispatched? invoke-dialog-action is destructive and non-idempotent, but it runs through withRescan('command', …), which re-POSTs once after a 401 with a refreshed credential (externalApiToolExecutor.test.ts confirms two POSTs on a stale token). That's safe only if a 401 is always a pre-dispatch auth rejection — i.e. the click never fired. If the producer can ever return 401 after dispatching, the rescan would invoke the action twice and break the "at most one action" rule this PR is built on. This is really a question for the monolith side (#65450) — can we confirm 401 is always pre-dispatch here?
  • Reuse dialogActionSchema for the tool param? invokeDialogAction.ts defines its own actionSchema (z.enum + .refine) rather than reusing dialogActionSchema (the discriminated union in types.ts). Reusing it would drop the action.label as string cast and the manual re-normalization in the callback, and keep one source of truth if a third action kind ever appears. Was the flat shape deliberate (e.g. cleaner JSON Schema for the client)?

nit

  • The @deprecated ExternalApiRead alias and the runExternalApiReadTool wrapper are source-compat retainers. Keeping them to avoid churning every read tool is a fair call; worth a follow-up to rename the call sites and drop both.

Fine as-is

  • invoke-dialog-action naming get-active-dialogs in its description is safe: both share the 0.2.12 floor so the reference can't dangle, and emittedToolReferences / corpusIntegrity guard against a rename rotting it.

Posted by MattGPT on Matt's behalf.

@myu404

myu404 commented Sep 10, 2026

Copy link
Copy Markdown
Author

🤖 Posted by MichaelGPT

Re @mattcfilbert's comment:

  1. This PR is now at 2.68.2, assuming feat(desktop): add individual datasource tools #875 lands first; I’ll rebase onto the updated feature/desktop after feat(desktop): add individual datasource tools #875 merges so the published version sequence stays explicit.
  2. I switched the tool parameter to the shared dialogActionSchema while retaining exact outbound normalization, so compatibility fields accepted by the shared schema are not forwarded; regression coverage pins the prior accepted and rejected action shapes.
  3. Agreed that the deprecated aliases are follow-up cleanup; I left them unchanged here to avoid unrelated call-site churn.

@mattcfilbert mattcfilbert left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@myu404
myu404 merged commit 20adccf into tableau:feature/desktop Sep 11, 2026
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants