Skip to content

fix(mothership): leave desktop tool calls to the desktop app in other clients - #8562

Merged
waleedlatif1 merged 2 commits into
stagingfrom
fix/mothership-browser-tool-single-executor
Oct 2, 2026
Merged

waleedlatif1 merged 2 commits into
stagingfrom
fix/mothership-browser-tool-single-executor

Conversation

@waleedlatif1

@waleedlatif1 waleedlatif1 commented Oct 2, 2026 •

Copy link
Copy Markdown
Collaborator

Summary

  • A Chat open in the desktop app and in a web tab at the same time could lose a browser or terminal tool's real result: the agent got an error instead
  • Every client tailing a chat starts each client-executed call it sees. A web tab has no desktop bridge, so for a desktop tool it fails at once and posts an error to /api/copilot/confirm. The confirm route takes a pre-claim error while the call is still pending, so the web tab's error usually lands before the desktop claims the call (the desktop restores its browser scope first). The desktop's claim then 404s, and its retried reports 404 too
  • Fix: the stream tool-event handler starts a desktop-only tool (agent browser, terminal, local files) only in the desktop app. Any other client leaves the call pending for the desktop, which claims it atomically through /api/desktop/tool/authorize as before
  • isDesktopExecutedToolCall now names that set in client-executed-tools.ts, and isClientExecutedToolCall is built from it, so the gate and the client-executed set can't drift apart
  • Workflow tools are unchanged: they still start in any client, and their claim settles who runs them

Design notes

  • Rejected: a server-side owner check on confirm. Native claims record a constant owner (desktop-browser/desktop-terminal), not which client holds them, and the desktop renderer's pre-claim errors (stale event, closed session, replay-guard limits) need the pre-claim path. Telling the web tab and the desktop apart would mean trusting a client header, or adding a per-client claim token across Electron main, the renderer and the server, which old desktop builds don't send
  • Rejected: changing the 404 on a late confirm for an already-finished native call to a 200 or 409. With this fix that 404 no longer happens in this scenario. A native confirm carries no executor identity, so a 200 would tell a client its result was accepted when it was thrown away

Behavior changes

  • Web only (no desktop app): no change. The server offers browser, terminal and local-file tools only when the request reports desktop capabilities, so a web-only turn never gets these calls
  • Desktop and a web tab open together: the desktop's result reaches the agent. The web tab shows the call as running until the result streams in
  • Desktop app (any shell version): no change. The renderer is served by Sim, and every shell injects the bridge the gate checks
  • If the desktop app quits for good while a web tab stays open, and the agent then issues a desktop tool, the call is no longer failed right away with a misleading "no active browser session" error from the web tab. It now ends on the worker's deferred-call timeout, the same as when no web tab is open. A desktop that exits mid-action still reports through its page-exit path, as before
  • Not covered here: two desktop windows on the same chat both try the call. The losing window's rejected claim is still reported as an error. This predates this PR and is unchanged by it. Fixing it needs a per-window claim identity

Type of Change

  • Bug fix

Testing

  • New lib/mothership/tools/client/desktop-tool-executor.integration.ts runs against real Postgres and Redis. A web tab runs the production stream handler and client executors. The desktop claims through the real authorize route and reports through the real confirm route. The server-side waiter shows what the agent receives. Covers browser_find and terminal
    • Red on origin/staging: the desktop's claim gets 404 and the agent receives the web tab's error
    • Green with the fix: the claim returns 200, the confirm returns 200, the agent receives success, and the row is completed
  • handle-tool-event.test.ts: the existing desktop routing tests now set the desktop bridge explicitly
  • Reverting the gate turns the integration test red again
  • bun run lint, bun run type-check (apps/sim), bun run check:audits, docs-manifest:check, block-registry check, root bun run test

Checklist

  • Code follows project style guidelines
  • Self-reviewed my changes
  • Tests added/updated and passing (new tests pass the test-audit authoring gate)
  • No new warnings introduced
  • I confirm that I have read and agree to the terms outlined in the Contributor License Agreement (CLA)

@vercel

vercel Bot commented Oct 2, 2026 •

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

1 Skipped Deployment
Project Deployment Actions Updated
docs Skipped Skipped Oct 2, 2026 8:09am UTC

Request Review

@waleedlatif1

Copy link
Copy Markdown
Collaborator Author

@greptile

@waleedlatif1

Copy link
Copy Markdown
Collaborator Author

@cubic-dev-ai review this PR

@cubic-dev-ai

cubic-dev-ai Bot commented Oct 2, 2026

Copy link
Copy Markdown
Contributor

@cubic-dev-ai review this PR

@waleedlatif1 I have started the AI code review. It will take a few minutes to complete.

@greptile-apps

greptile-apps Bot commented Oct 2, 2026 •

Copy link
Copy Markdown
Contributor

RetriggerConfidence Score: 5/5

[Medium risk] Refactors which tool calls run on desktop versus web clients.

The PR appears safe to merge, though the omitted web-tab routing coverage is worth restoring.

Summary

The PR prevents web tabs tailing a chat from executing desktop-only tool calls, leaving those calls for the desktop app to claim and complete.

  • Separates desktop-executed tools from workflow tools in the client-executed classification.
  • Adds a database- and Redis-backed regression test for browser and terminal results.
  • Removes the prior mock-call test, leaving workflow and local-file web-tab routing without equivalent coverage.
Diagram
sequenceDiagram
  participant Web as Web tab
  participant Desktop as Desktop app
  participant Server as Tool-call server
  participant Agent as Agent waiter
  Server-->>Web: Desktop-only call frame
  Server-->>Desktop: Desktop-only call frame
  Web->>Web: Leave call pending
  Desktop->>Server: Claim call
  Server-->>Desktop: Authorized
  Desktop->>Server: Confirm native result
  Server-->>Agent: Deliver result
Loading

Reviews (2) · Last reviewed commit: "test(mothership): drop the mock-call rou..."

Comment thread apps/sim/app/workspace/[workspaceId]/home/hooks/stream/handle-tool-event.test.ts Outdated

@cubic-dev-ai cubic-dev-ai Bot 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.

No issues found across 4 files

Confidence score: 5/5

  • Automated review surfaced no issues in the provided summaries.
  • No files require special attention.

Re-trigger cubic

@cubic-dev-ai cubic-dev-ai Bot 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.

No issues found across 4 files

Confidence score: 5/5

  • Automated review surfaced no issues in the provided summaries.
  • No files require special attention.

Re-trigger cubic

@waleedlatif1

Copy link
Copy Markdown
Collaborator Author

@greptile

@waleedlatif1

Copy link
Copy Markdown
Collaborator Author

@cubic-dev-ai review this PR

@cubic-dev-ai

cubic-dev-ai Bot commented Oct 2, 2026

Copy link
Copy Markdown
Contributor

@cubic-dev-ai review this PR

@waleedlatif1 I have started the AI code review. It will take a few minutes to complete.

@cubic-dev-ai cubic-dev-ai Bot 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.

No issues found across 4 files

Confidence score: 5/5

  • Automated review surfaced no issues in the provided summaries.
  • No files require special attention.

Re-trigger cubic

@greptile-apps

greptile-apps Bot commented Oct 2, 2026

Copy link
Copy Markdown
Contributor

Comments Outside Diff

These findings could not be posted inline.

  • P2 Web-tab routing coverage removed apps/sim/app/workspace/[workspaceId]/home/hooks/stream/handle-tool-event.test.ts:70 ▶

    The deleted test was the only one checking that workflows still start in a web tab while local-file calls stay pending. The remaining unit tests all use a desktop bridge, and the new integration test covers only browser and terminal calls. This leaves both omitted paths open to regressions without a failing test. Please restore coverage through observable outcomes rather than mock-call assertions.

    Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

@waleedlatif1

Copy link
Copy Markdown
Collaborator Author

Re the outside-diff note on web-tab routing coverage: leaving this as is, deliberately.

  • Local files: they take the same branch as browser and terminal. The gate is isDesktopApp() || !isDesktopExecutedToolCall(name, args), and isClientExecutedToolCall is now built from isDesktopExecutedToolCall, so the set of calls a client starts and the set it leaves to the desktop can't drift apart. The integration suite exercises that branch end to end for browser and terminal. Adding import_local_files needs a DOM window in the web-tab realm, because the native-file executor registers a pagehide listener. In this Node-environment suite, a global window would also flip the server modules that read it to tell server from browser (env resolution). I tried it, and the harness that keeps the two realms apart was more machinery than the coverage is worth.
  • Workflow tools: they are outside isDesktopExecutedToolCall by construction, so the gate never applies to them. If a regression ever stopped a web tab from starting one, the server's workflow pickup race (raceWorkflowToolClientPickup) would still run it after the grace period. The removed unit test was the only cheap guard, and it asserted mock calls, which the repo's testing rules prohibit.

@waleedlatif1
waleedlatif1 merged commit cef594c into staging Oct 2, 2026
33 of 34 checks passed
@waleedlatif1
waleedlatif1 deleted the fix/mothership-browser-tool-single-executor branch October 2, 2026 17:26

This branch was previously deployed

1 inactive deployment
Preview — 842a2f3c Deployed Oct 2, 2026 by vercel[bot]
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.

1 participant