Skip to content

fix(mothership): refuse desktop claims for unapproved or stopped calls - #8643

Merged
waleedlatif1 merged 7 commits into
stagingfrom
fix/desktop-tool-authorize-stop
Oct 6, 2026
Merged

waleedlatif1 merged 7 commits into
stagingfrom
fix/desktop-tool-authorize-stop

Conversation

@waleedlatif1

@waleedlatif1 waleedlatif1 commented Oct 5, 2026 •

Copy link
Copy Markdown
Collaborator

Summary

  • /api/desktop/tool/authorize no longer hands a call to the desktop app when it should not run:
    • a call held for the user's approval is refused (403) until they allow it. A declined call stays refused. Whether a call is gated depends on the turn (flag, arguments, allow lists), so pre-persist records it on the row (permission_requested_at, nullable, expand-only migration 0395).
    • once the run's tool admission has closed (Stop, a newer turn, or the run's end), every desktop call is refused (410), including non-claiming local reads.
  • One claim primitive. claimSimToolExecution becomes claimToolExecution, which takes either a Sim owner token (the existing leased claim, unchanged) or a desktop claim owner. Both lock the run row and check admission the same way, so a desktop claim serializes with Stop. There is no second copy of that logic.
  • Stop settles the stopped runs' open desktop calls in the transaction that closes admission. An unclaimed call is cancelled as never started (notStarted); a claimed one as outcome unknown (outcomeUnknown, doNotRetry).
    • Candidate rows are picked with the shared TS classifier (lib/mothership/tools/desktop-tools.ts, now the single "is this a desktop tool" module), not a SQL copy of it.
    • Each result is sealed exactly as /api/copilot/confirm seals a client result (the shared sealClientToolSettlement), so a waiter still listening restores what Stop did.
  • /api/copilot/confirm acknowledges a result for an already-settled call with the stored outcome (200), instead of 404 (native) or 500 (other client tools). That covers a retried delivery and a late result after Stop or a server-side settlement. Nothing is written or published again, and the renderer stops retrying.
  • No desktop release is needed: the desktop app only checks response.ok on authorize, and an import refusal maps as before.

A terminal command already running when Stop lands keeps running on the desktop. There is no cancel channel for it in the current IPC, so that is left to the desktop executor work.

Type of Change

  • Bug fix

Testing

  • desktop-tool-authorization.integration.ts (real Postgres + Redis, production pre-persist, authorize, tool-permission, Stop and confirm paths): 11 tests. On staging they fail with 200 instead of 403/410, claims succeed after Stop, the waiter never wakes, a late result revives a stopped call, and duplicate results get 404/500. The Stop wake-up test runs with sealed provenance: with an unsealed Stop result it restores only a generic "Tool cancelled".
  • Reverted each guard on its own and watched its test go red: the claim permission predicate, the claim admission check, the route admission check, the Stop settlement (incl. local-folder reads), Stop sealing, the settled-call acknowledgement, the lost-write re-read, and the pre-persist gate flag.
  • bun run lint, bun run type-check, bun run check:audits, bun run check:migrations, bun run test, bun run test:integration (1438 passed), bun run docs-manifest:check, block-registry check: all pass. drizzle-kit generate produces no new migration.

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)

@waleedlatif1

Copy link
Copy Markdown
Collaborator Author

@greptile

@waleedlatif1

Copy link
Copy Markdown
Collaborator Author

@cubic-dev-ai review this PR

@vercel

vercel Bot commented Oct 5, 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 6, 2026 2:11am UTC

Request Review

@cubic-dev-ai

cubic-dev-ai Bot commented Oct 5, 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 15 files

Re-trigger cubic

Comment thread apps/sim/lib/mothership/async-runs/repository.ts
Comment thread apps/sim/app/api/desktop/tool/authorize/route.ts Outdated
Comment thread apps/sim/app/api/desktop/tool/authorize/route.ts Outdated
Comment thread apps/sim/lib/mothership/tools/client/desktop-tool-authorization.integration.ts Outdated
Comment thread apps/sim/lib/mothership/async-runs/desktop-tools.ts Outdated
Comment thread apps/sim/lib/mothership/tools/client/desktop-tool-authorization.integration.ts Outdated
@greptile-apps

greptile-apps Bot commented Oct 5, 2026 •

Copy link
Copy Markdown
Contributor

RetriggerConfidence Score: 5/5

[High risk] Adds database schema column and refactors tool execution claim logic.

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

Summary

The PR prevents the desktop app from claiming unapproved or stopped tool calls and settles open desktop calls when Stop closes admission.

  • Records when a tool call requires permission and shares admission checks between Sim and desktop claims.
  • Seals Stop settlements so waiting turns can recover them, and acknowledges late or retried results with the stored outcome.
Diagram
%%{init: {'theme': 'neutral'}}%%
flowchart LR
  A[Tool call pre-persisted] --> B{Permission required?}
  B -->|Yes| C[Wait for approval]
  B -->|No| D[Desktop authorization]
  C -->|Allowed| D
  C -->|Not allowed| E[Refuse claim]
  D --> F{Admission open?}
  F -->|No| G[Refuse claim]
  F -->|Yes| H[Claim and execute]
  H --> I[Confirm result]
  G --> J[Stop settles open desktop calls]
  J --> K[Acknowledge late result with stored outcome]
Loading

Reviews (6) · Last reviewed commit: "test(mothership): assert authorize outco..."

Comment thread apps/sim/app/api/desktop/tool/authorize/route.ts Outdated
Comment thread apps/sim/app/api/copilot/confirm/route.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 5, 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 15 files

Fix all with cubic | 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 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 16 files

Confidence score: 5/5

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

Re-trigger cubic

The desktop authorize route claimed any pending call, so a gated terminal
run that was still awaiting approval, or a call on a run the user had
stopped, could be claimed and executed. The claim now locks the run row and
refuses once tool admission has closed (Stop, a newer turn, or the run's
end), and refuses a call held for the user's decision until they allow it.
Whether a call is gated depends on the turn, so pre-persist records it on
the row (permission_requested_at).

Stop now settles the stopped runs' open desktop calls in the transaction
that closes admission: unclaimed calls as never started, claimed calls as
outcome unknown. A result for an already-settled call (a retry, or one that
lost to Stop) is acknowledged with the stored outcome instead of 404/500.
…0 after it

Stop also settles delivered desktop calls and reads of granted local
folders. Authorize checks admission before the call's status, so a call
Stop already settled answers 410 rather than 404.
…r, sealed Stop results

The desktop claim is now an option of the run-locked tool execution claim
(claimSimToolExecution becomes claimToolExecution) instead of a second copy
of the admission check. Stop picks the open desktop calls with the shared TS
classifier, which moves to lib/mothership/tools/desktop-tools.ts, instead of
a SQL restatement of it, and seals each result the way the confirm route
does, so a waiter restores what Stop did rather than failing to unseal it.
@waleedlatif1
waleedlatif1 force-pushed the fix/desktop-tool-authorize-stop branch from 8e796d1 to 2c923ff Compare October 6, 2026 01:52
@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 25 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/async-runs/repository.ts
Comment thread apps/sim/lib/mothership/async-runs/repository.ts
Comment thread apps/sim/lib/mothership/tools/desktop-tools.ts
…arker

Calls gated before permission_requested_at existed carry no marker, so a
recorded decision that does not allow the call now disqualifies it too.
@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.

Comment thread apps/sim/app/api/desktop/tool/authorize/route.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 25 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

@waleedlatif1
waleedlatif1 merged commit 46cd710 into staging Oct 6, 2026
33 checks passed
@waleedlatif1
waleedlatif1 deleted the fix/desktop-tool-authorize-stop branch October 6, 2026 17:31

This branch was previously deployed

1 inactive deployment
Preview — 7554e45f 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