fix(mothership): end desktop tools at sign-out and settle held reports as final by waleedlatif1 · Pull Request #8673 · simstudioai/sim

waleedlatif1 · 2026-10-06T15:53:07Z

Summary

Follow-ups from the final review of #8666:

Sign-out ends desktop tools. clearUserData now calls a new stopAllDesktopTools() first, before any step that could fail. Every local file read, import and browser action still running in the tab is cancelled. Before, the server already rejected the result after sign-out, but the action itself kept running on the user's computer.
One lease key. Local file tools now lease the turn's stream id captured when the stream started, the same as browser actions. They used to read streamIdRef.current at tool start.
Page-exit reporter. reportClientToolCompletionOnPageExit treats a 409 as final, like reportClientToolCompletion, instead of throwing.
Wording. A stale browser observation now says it was not run this time and has no result, and that an observation changes nothing in the browser. It says not to retry it in this turn and to ask the user to keep this chat open in the Sim desktop app or ask again later. This stays true even if an earlier delivery already ran it. The result data is unchanged.
Module doc. It now notes that terminal calls take no lease. A terminal call returns once its operation does; for a run, that's when its wait window ends. The command it started keeps running in a terminal tab the user can see and control. Neither Stop nor sign-out kills that process: Stop only clears the agent's marks on the tab. Ending agent-started terminal processes would need a desktop-side API and is out of scope here.

Type of Change

Bug fix

Testing

stores/index.test.ts: signing out cancels running desktop tools even when the in-memory store reset fails.
desktop-tool-lifetimes.test.ts: stopAllDesktopTools cancels every turn, and the next tool gets a fresh lifetime.
completion.test.ts: a page-exit 409 resolves without retrying.
confirm/route.test.ts: a new test covers a not-started report on a call the desktop already claimed; it gets the final 409.
browser-tool-execution.test.ts: checks the new stale-observation wording.
check:test-patterns passes: the new tests check responses and outcomes, not mock calls.
No trace contract change: the confirm route's 409s keep recording tool_call_not_found. Regenerating trace-attribute-values-v1.ts from the committed Go contract produces no diff.
I reverted each source change on its own and watched its test go red.
There is no dedicated test for the lease key. I didn't find a client flow where streamIdRef.current differs from the id captured at stream start when a tool arrives, so this change makes the two tool kinds consistent rather than fixing a reproduced failure.
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
No new warnings introduced
I confirm that I have read and agree to the terms outlined in the Contributor License Agreement (CLA)

…s as final - Signing out cancels every desktop tool still running in the tab, so no local read or browser action outlives the session that started it. - Local file tools lease the turn captured at stream start, like browser actions. - The confirm route records a held_by_desktop outcome for its 409s, and the page-exit reporter treats a 409 as final. - A stale browser observation says it never started and not to retry it in this turn.

vercel · 2026-10-06T15:53:10Z

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

1 Skipped Deployment

Project	Deployment	Actions	Updated
docs	Skipped		Oct 6, 2026 5:44pm UTC

waleedlatif1 · 2026-10-06T15:53:11Z

@greptile

waleedlatif1 · 2026-10-06T15:53:13Z

@cubic-dev-ai review this PR

cubic-dev-ai · 2026-10-06T15:53:17Z

@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

1 issue found across 12 files

Confidence score: 4/5

In browser-tool-execution.ts, a stale replay can hit the stale-event branch before the replay ledger is checked, so the model may be told an observation never started even though it already ran. Check the replay ledger before reporting the stale delivery.

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/tools/client/browser-tool-execution.ts">

<violation number="1" location="apps/sim/lib/mothership/tools/client/browser-tool-execution.ts:113">
P2: This message can falsely tell the model that an observation never started: a stale replay may follow an earlier execution, but the stale-event branch runs before the replay ledger is checked. Say the current delivery was not run and that a previous delivery may already have run it.</violation>
</file>

_{Reply with feedback, questions, or to request a fix.

Fix all with cubic | Turn on auto-fix | Re-trigger cubic}

cubic-dev-ai

No issues found across 12 files

Confidence score: 5/5

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

_{Turn on auto-fix | Re-trigger cubic}

greptile-apps · 2026-10-06T15:59:29Z

Confidence Score: 5/5

[Medium risk] Cleans up desktop tool sessions at sign-out and hardens tool claim rules.

The PR appears safe to merge, though the confirmation trace outcome should be corrected to preserve monitoring accuracy.

Summary

The PR cancels leased desktop tools at sign-out, uses the captured turn ID for local-file leases, treats page-exit 409 responses as final, and clarifies stale browser-observation messaging. The subsequent changes also remove the distinct held-by-desktop trace outcome, conflating held calls with missing calls in telemetry.

_{Reviews (3) · Last reviewed commit: "revert(mothership): keep the confirm tra..."}

…otes - A stale browser observation says it was not run this time, which stays true when an earlier delivery already ran it. - The lifetimes module says what a terminal call leaves running, and that sign-out ends leased tools only. - Tests check responses and recorded outcomes instead of mock calls.

waleedlatif1 · 2026-10-06T16:05:46Z

@greptile

waleedlatif1 · 2026-10-06T16:05:48Z

@cubic-dev-ai review this PR

cubic-dev-ai · 2026-10-06T16:05:56Z

@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

No issues found across 12 files

Confidence score: 5/5

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

_{Turn on auto-fix | Re-trigger cubic}

Drop the held_by_desktop outcome so this change needs no trace contract update: the 409 paths record tool_call_not_found as before. Keep a route test for a not-started report on a claimed call.

waleedlatif1 · 2026-10-06T17:44:55Z

@greptile

waleedlatif1 · 2026-10-06T17:44:56Z

@cubic-dev-ai review this PR

cubic-dev-ai · 2026-10-06T17:45:03Z

@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

No issues found across 10 files

Confidence score: 5/5

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

_{Turn on auto-fix | Re-trigger cubic}

…not run" once Follow-ups from the #8673 review: the lease docs now name sign-out (`stopAllDesktopTools`) alongside the user's Stop as what cancels a desktop tool, and the stale-observation message no longer says it was not run twice.

…dropping it (#8674) * fix(mothership): keep a message the server never admitted instead of dropping it Two sends were lost without an error: - A send whose POST got no response (offline, Wi-Fi drop, waking a laptop) reconnected to the stream it would have opened. That stream does not exist, so the 404 read as "finished", the turn finalized as a success, and the refetched transcript no longer held the message. A queued follow-up was lost the same way, since it had already left the queue. - A send refused with 409 because another turn held the chat (started in another tab, or one this surface lost track of) reconnected to that turn under the new message's bubble, then vanished when it finished. Both now hand the message back under its id. An unreachable send is held in the queue, so it is not redispatched into the same failure, and goes out when the browser is back online (or when the user sends it). A send that found the chat busy waits in the queue behind that turn, which the chat shows as running, and goes out when it ends. Reusing the id keeps a retry deduplicated if the server did admit the first attempt. * fix(mothership): keep held and busy-refused sends exactly once across remounts - A first message held offline on the new-chat page sat under that mount's queue key, which dies with the mount, so a reload or remount before the network returned stranded it. Held sends on a chatless surface now carry the surface they belong to, and the next chatless mount of that surface adopts them. - Held sends are released for every chat when the browser comes back online, and on mount when it already is, so a send held in a chat the user is not viewing (or one whose `online` event fired with no surface mounted) still goes out. - A send refused because the chat is busy is handed back only after the chat's running turn has been read, so the queue cannot redispatch it before that turn ends. A busy refusal that does not name the running turn no longer reads as a deduplicated send, which reconnected to a stream that never existed and lost the message. * fix(mothership): send released held messages through the queue's own drain rules Releasing held sends on mount kicked the queue dispatcher directly, which skips the drain's guards. After a reload the chat history is not loaded yet, so a follow-up queued behind a still-running turn went out at once and was refused as busy. The release now only clears the hold; the drain effect, which waits for the history and for the running turn to end, sends a released head, and now also re-runs when the head's hold clears. * fix(mothership): don't hold a send whose network returned while it was failing An `online` event can fire while the failing POST is still pending, so the release ran before the message was held and the message then waited for a release that had already happened. A send now notes whether the browser came back online while it was in flight, and if so goes back to the queue unheld, for the drain to send under its usual rules. * chore(mothership): name sign-out in desktop tool lease docs and say "not run" once Follow-ups from the #8673 review: the lease docs now name sign-out (`stopAllDesktopTools`) alongside the user's Stop as what cancels a desktop tool, and the stale-observation message no longer says it was not run twice. * fix(mothership): restore a withdrawn queued send even after the user switched chats A queued send had already left the queue when its POST failed, and a dispatch whose epoch changed meanwhile (the user switched chats) skipped restoring it, so the message was lost. A withdrawn send was never admitted, so it now goes back to its own chat's queue regardless of the epoch. * fix(mothership): don't recreate a deleted chat's queue from a late restore A withdrawn send restored after its dispatch outlived a chat switch could land after the user deleted that chat, recreating a queue (and a message) for a conversation that no longer exists. Clearing a chat's queue now leaves a session tombstone that restores respect; a new enqueue for that key lifts it.

vercel Bot temporarily deployed to Preview October 6, 2026 15:53 Inactive

cubic-dev-ai Bot reviewed Oct 6, 2026

View reviewed changes

Comment thread apps/sim/app/workspace/[workspaceId]/home/hooks/desktop-tool-lifetimes.ts

Comment thread apps/sim/lib/mothership/tools/client/browser-tool-execution.ts Outdated

cubic-dev-ai Bot reviewed Oct 6, 2026

View reviewed changes

greptile-apps Bot reviewed Oct 6, 2026

View reviewed changes

Comment thread apps/sim/stores/index.ts

Comment thread apps/sim/app/api/copilot/confirm/route.test.ts Outdated

vercel Bot temporarily deployed to Preview October 6, 2026 16:05 Inactive

cubic-dev-ai Bot reviewed Oct 6, 2026

View reviewed changes

revert(mothership): keep the confirm trace outcome vocabulary unchanged

a37e9ea

Drop the held_by_desktop outcome so this change needs no trace contract update: the 409 paths record tool_call_not_found as before. Keep a route test for a not-started report on a claimed call.

vercel Bot temporarily deployed to Preview October 6, 2026 17:44 Inactive

cubic-dev-ai Bot reviewed Oct 6, 2026

View reviewed changes

waleedlatif1 merged commit 94cba3b into staging Oct 6, 2026
33 checks passed

waleedlatif1 deleted the fix/desktop-tool-signout branch October 6, 2026 21:19

waleedlatif1 mentioned this pull request Oct 6, 2026

improvement(desktop): Electron E2E for desktop tool lifetimes against a live local Sim #8697

Merged

6 tasks done

Conversation

waleedlatif1 commented Oct 6, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Type of Change

Testing

Checklist

Uh oh!

vercel Bot commented Oct 6, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

waleedlatif1 commented Oct 6, 2026

Uh oh!

waleedlatif1 commented Oct 6, 2026

Uh oh!

cubic-dev-ai Bot commented Oct 6, 2026

Uh oh!

cubic-dev-ai Bot left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

cubic-dev-ai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

greptile-apps Bot commented Oct 6, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Confidence Score: 5/5

Summary

Uh oh!

Uh oh!

Uh oh!

waleedlatif1 commented Oct 6, 2026

Uh oh!

waleedlatif1 commented Oct 6, 2026

Uh oh!

cubic-dev-ai Bot commented Oct 6, 2026

Uh oh!

cubic-dev-ai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

waleedlatif1 commented Oct 6, 2026

Uh oh!

waleedlatif1 commented Oct 6, 2026

Uh oh!

cubic-dev-ai Bot commented Oct 6, 2026

Uh oh!

cubic-dev-ai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

This branch was previously deployed

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

waleedlatif1 commented Oct 6, 2026 •

edited

Loading

vercel Bot commented Oct 6, 2026 •

edited

Loading

cubic-dev-ai Bot left a comment •

edited

Loading

greptile-apps Bot commented Oct 6, 2026 •

edited

Loading