Skip to content

fix(mothership): fail unclaimed desktop calls fast and stop dropping them silently - #8652

Merged
waleedlatif1 merged 4 commits into
stagingfrom
fix/desktop-tool-fast-failure
Oct 6, 2026
Merged

waleedlatif1 merged 4 commits into
stagingfrom
fix/desktop-tool-fast-failure

Conversation

@waleedlatif1

@waleedlatif1 waleedlatif1 commented Oct 6, 2026 •

Copy link
Copy Markdown
Collaborator

Summary

Builds on #8643 (merged). Only the chat view showing a chat starts that chat's desktop calls, so a call issued while the user is on another chat or page was claimed by nobody. The turn then waited 90 to 160 s for a watchdog that called it "hung".

  • One desktop wait (request/tools/desktop-wait.ts). A desktop call the desktop claims on pickup that is still unclaimed after a 15 s grace is settled as never started, using the inverse of the claim (pending -> failed). Exactly one side wins: a late claim is refused, and a call claimed in time keeps waiting for its result.
    • The failure is sealed like any client completion, through one shared settleToolCallFailure that the watchdog uses too.
    • A result that lands as the grace runs out is returned, not discarded.
    • The model gets { error, notStarted: true }: never started, the chat isn't open in the desktop app, safe to retry.
  • Local reads (read_local_file, user-local VFS read/grep/glob) are covered too, for a desktop that advertises localReadClaims:
    • the preload exposes it, the renderer sends it with the turn's desktop capabilities, and pre-persist then stores those calls pending;
    • Electron main claims each one through authorize (claim: true), as it already does for imports; later reads of the same call ride on that claim.
    • Older desktops don't advertise the capability, and a recovered leg doesn't carry it (the strict recovery config is untouched). In both cases those calls keep today's running behaviour, which authorize still accepts.
  • The single "hung and was abandoned" result becomes two:
    • notStarted, for a desktop call nobody claimed;
    • outcomeUnknown + doNotRetry, for one that started and lost its result.
  • Terminal run budget. The server budget now follows the wait the desktop holds a run for (waitSeconds, up to 120 s), plus headroom; it was a fixed 60 + 30 s. resolveRunWaitMs moves into @sim/terminal-protocol, shared with the desktop.
  • A report that says a call never started can only settle an unclaimed call. A stale replay can no longer end a command the desktop is running.
  • Renderer:
    • a running browser action is cancelled only by the user's Stop. Recovering the stream when the window returns to view, or leaving the chat view, lets it finish and report its own result;
    • a terminal call delivered too late is reported as not started instead of being skipped.
  • One classifier. lib/mothership/tools/desktop-tools.ts is the only desktop-tool classifier; the authorize and confirm routes and the pickup path use it.
  • Typed claims. The Sim execution claim and the desktop claim share one run-admission lock but have their own outcome types, so Sim callers cannot receive the desktop-only awaiting_permission.
  • The worker needs no change: it passes tool result data to the model as opaque JSON, and success: false feeds its failure breaker as before.

Type of Change

  • Bug fix

Testing

  • desktop-tool-pickup.integration.ts (real Postgres + Redis, production pre-persist and dispatch, authorize and confirm routes), 7 tests:
    • an unclaimed browser, terminal or capable-desktop local read fails as not started at about 15 s; on staging it hangs for the whole test budget;
    • a late claim is refused;
    • a call claimed in time, including a claimed local read read again past the grace, still completes;
    • a stale not-started report can't end a running command;
    • an older desktop's local read works as before.
  • Unit and DOM tests:
    • desktop-wait.test.ts forces a result to land inside the grace/abort window;
    • executor.test.ts covers both abandoned shapes and the terminal budget;
    • terminal-tool-execution.test.ts;
    • use-chat.dom.test.tsx: recovery and unmount leave the action running, and Stop cancels it;
    • the desktop ipc.test.ts claim flag.
  • Reverted each of the 12 guards on its own and watched its test go red.
  • bun run lint, bun run type-check, bun run check:audits, bun run test, bun run test:integration, bun run docs-manifest:check, block-registry check: all pass.

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 6, 2026 •

Copy link
Copy Markdown

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

Project Deployment Actions Updated
docs Ready Ready Preview Oct 6, 2026 3:38am 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 6, 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.

1 issue found across 14 files

Confidence score: 3/5

  • In desktop-pickup.ts, calls arriving near the end of pickup grace can time out before receiving their full timeoutMs execution budget. Restart the wait with the full timeout after pickup grace.
Prompt for AI agents (unresolved issues)

Check if these issues are valid — if so, understand the root cause of each and fix them. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. If appropriate, use sub-agents to investigate and fix each issue separately.


<file name="apps/sim/lib/mothership/request/tools/desktop-pickup.ts">

<violation number="1" location="apps/sim/lib/mothership/request/tools/desktop-pickup.ts:99">
P2: A call claimed near the end of the pickup grace gets only the original deadline’s remainder to execute, not its full `timeoutMs` budget. Restart the wait with the full timeout so pickup grace does not consume the execution allowance.</violation>
</file>

Fix all with cubic | Re-trigger cubic

Comment thread apps/sim/lib/mothership/request/tools/executor.ts
Comment thread apps/sim/app/workspace/[workspaceId]/home/hooks/use-chat.ts Outdated
Comment thread apps/sim/lib/mothership/tools/client/terminal-tool-execution.ts
Comment thread apps/sim/lib/mothership/request/tools/desktop-pickup.ts Outdated
@greptile-apps

greptile-apps Bot commented Oct 6, 2026 •

Copy link
Copy Markdown
Contributor

RetriggerConfidence Score: 5/5

[High risk] Changes how desktop tool calls are claimed and waited for.

The PR appears safe to merge; no new actionable issue or outstanding previous finding was identified.

Summary

The PR adds a bounded pickup wait for desktop calls, distinguishes calls that never started from calls whose results were lost, and adds claim-aware local reads while retaining older-desktop behavior.

  • Desktop authorization and completion use claim state to prevent late pickup or stale “not started” reports from replacing a running call’s result.
  • The terminal run budget now follows the desktop’s bounded wait.
  • Renderer browser actions survive stream recovery and view changes while remaining cancellable by Stop.
Diagram
%%{init: {'theme': 'neutral'}}%%
flowchart TD
  A[Desktop tool call persisted pending] --> B{Desktop claims before grace ends?}
  B -->|Yes| C[Running: wait for desktop result]
  B -->|No| D[Pending-to-failed settlement]
  D --> E[Return notStarted]
  C --> F{Result received?}
  F -->|Yes| G[Return desktop result]
  F -->|No, budget exhausted| H[Return outcomeUnknown and doNotRetry]
Loading

Reviews (5) · Last reviewed commit: "fix(mothership): settle an unclaimed des..."

Comment thread apps/sim/app/workspace/[workspaceId]/home/hooks/use-chat.ts Outdated
Comment thread apps/sim/lib/mothership/request/tools/executor.test.ts
@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 6, 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 15 files

Confidence score: 5/5

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

Re-trigger cubic

Comment thread apps/sim/lib/mothership/tools/client/browser-tool-execution.test.ts Outdated
@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 6, 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 15 files

Confidence score: 5/5

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

Re-trigger cubic

…them silently

A desktop call reaches the user's machine only through the chat view showing
that chat, so one issued while the user is elsewhere was never claimed and
the turn waited out a 90-160 s watchdog that called it hung. One desktop wait
now fails a call still unclaimed after a 15 s pickup grace as never started,
with the inverse of the desktop's claim (pending -> failed) and a result
sealed like any client completion. A call claimed in time keeps waiting, and
a result that lands as the grace runs out is returned, not discarded.

Local reads (read_local_file, user-local VFS reads) join them for a desktop
that advertises localReadClaims: the desktop claims each read through
authorize, as it already does for imports. Older desktops keep today's
behaviour.

The single "hung and was abandoned" result becomes two: notStarted for an
unclaimed call, and outcomeUnknown/doNotRetry for one that started and lost
its result. Both are settled through one sealed failure helper. A terminal
run's server budget now follows the wait the desktop holds it for. A
not-started report can settle only an unclaimed call.

In the renderer, a running browser action is cancelled only by Stop:
recovering the stream or leaving the chat view lets it finish and report.
A terminal call delivered too late is reported as not started instead of
being skipped.
…or each claimant

The authorize and confirm routes classify desktop tools through
lib/mothership/tools/desktop-tools.ts instead of inline checks. The Sim
execution claim and the desktop claim share one run-admission lock but
return their own outcome types, so a Sim caller can no longer receive the
desktop-only awaiting_permission outcome. The terminal-status mapping moves
to lifecycle as getTerminalConfirmationStatus, since confirm uses it for
every client tool.
@waleedlatif1
waleedlatif1 force-pushed the fix/desktop-tool-fast-failure branch from 07cfc57 to d9b5bc2 Compare October 6, 2026 03:26
@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 6, 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.

All reported issues were addressed across 36 files

You've manually re-run cubic several times on this PR. Each manual re-review checks the full PR again and counts toward your usage quota. To preserve your usage limits, we recommend letting cubic automatically review new commits.

Fix all with cubic | Re-trigger cubic

Comment thread apps/sim/lib/mothership/request/tools/desktop-wait.ts Outdated
Comment thread apps/desktop/src/main/ipc.test.ts
Comment thread apps/sim/lib/mothership/async-runs/repository.ts
…ore the grace

A wait shorter than the pickup grace ended with no result and left the call
claimable; it now settles it as never started like any unclaimed call. The
desktop E2E fixture models claims per call, the way the server accepts a
local read's repeat claim and refuses a second import claim. The admission
probe follows the claim rename.
@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 6, 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 38 files

Confidence score: 5/5

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

You've manually re-run cubic several times on this PR. Each manual re-review checks the full PR again and counts toward your usage quota. To preserve your usage limits, we recommend letting cubic automatically review new commits.

Re-trigger cubic

This branch was successfully deployed

1 active deployment
Preview — 60a4d186 Deployed Oct 6, 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