Skip to content

Report honest removed flag from oauth.clients.remove - #1603

Merged
RhysSullivan merged 1 commit into
mainfrom
oauth-clients-remove-honest
Aug 16, 2026
Merged

Report honest removed flag from oauth.clients.remove#1603
RhysSullivan merged 1 commit into
mainfrom
oauth-clients-remove-honest

Conversation

@RhysSullivan

Copy link
Copy Markdown
Collaborator

Follow-up to #1600, found by using it: while cleaning up 8 unused OAuth clients, one of the removals reported { removed: true } and deleted nothing. Only the arithmetic gave it away — 20 clients minus 8 removals left 13, not 12.

oauth.removeClient is idempotent by design at the storage layer (deleteMany on a missing row is a no-op, which is correct for a delete), but the tool mapped that silence to unconditional success. So a typo'd slug, an already-deleted client, and the wrong owner were all indistinguishable from a real deletion. I confirmed the extreme case against production: removing definitely-not-a-real-client also returned { removed: true }.

It bites hardest because clients are keyed by both owner and slug, so the same slug can exist separately under org and user. A sweep over a list of slugs with one hardcoded owner deletes only the matching half and reports every call as success — which is exactly what happened: a microsoft-graph OAuth app stayed registered under org after everything it authorized had been deleted.

The tool now checks the caller-visible client set first and returns removed: false when nothing matched that (owner, slug) pair. The service-level removeClient is unchanged and stays idempotent — the honesty belongs at the agent boundary, matching integrations.remove from #1600.

Verification

  • New test in oauth-remove-client.test.ts covers the exact shape that hid the bug: the same slug registered under both owners, removed one owner at a time, plus a never-existed slug and a repeat removal.
  • Confirmed the test is not vacuous — reverting the fix fails it with expected { removed: true } to deeply equal { removed: false }.
  • format:check, lint, repo-wide typecheck clean; packages/core/sdk 599/599.

Not in this PR

oauth.clients.list returns owner and slug as separate fields with no single unambiguous identifier, which is what let two microsoft-graph rows read as one entry when scanning the output. An owner-qualified address field would prevent that class of misread, but it changes an output schema, so it seemed worth deciding separately.

The tool mapped an idempotent storage no-op to removed: true, so a typo'd slug, an already-deleted client and the wrong owner all read as a real deletion. Clients are keyed by (owner, slug), so a sweep under one hardcoded owner silently skipped the other scope's copies. The tool now checks the visible client set first; the service-level removeClient stays idempotent.
@github-actions

github-actions Bot commented Aug 16, 2026

Copy link
Copy Markdown
Contributor

Cloudflare preview

Torn down — the PR is closed.

@cloudflare-workers-and-pages

Copy link
Copy Markdown

Deploying with  Cloudflare Workers  Cloudflare Workers

The latest updates on your project. Learn more about integrating Git with Workers.

Status Name Latest Commit Preview URL Updated (UTC)
✅ Deployment successful!
View logs
executor-marketing 5dec04e Commit Preview URL

Branch Preview URL
Aug 16 2026, 12:31 PM

@cloudflare-workers-and-pages

Copy link
Copy Markdown

Deploying with  Cloudflare Workers  Cloudflare Workers

The latest updates on your project. Learn more about integrating Git with Workers.

Status Name Latest Commit Updated (UTC)
✅ Deployment successful!
View logs
executor-cloud 5dec04e Aug 16 2026, 12:32 PM

@pkg-pr-new

pkg-pr-new Bot commented Aug 16, 2026

Copy link
Copy Markdown

Open in StackBlitz

@executor-js/cli

npm i https://pkg.pr.new/@executor-js/cli@1603

@executor-js/config

npm i https://pkg.pr.new/@executor-js/config@1603

@executor-js/execution

npm i https://pkg.pr.new/@executor-js/execution@1603

@executor-js/sdk

npm i https://pkg.pr.new/@executor-js/sdk@1603

@executor-js/codemode-core

npm i https://pkg.pr.new/@executor-js/codemode-core@1603

@executor-js/runtime-quickjs

npm i https://pkg.pr.new/@executor-js/runtime-quickjs@1603

@executor-js/plugin-file-secrets

npm i https://pkg.pr.new/@executor-js/plugin-file-secrets@1603

@executor-js/plugin-graphql

npm i https://pkg.pr.new/@executor-js/plugin-graphql@1603

@executor-js/plugin-keychain

npm i https://pkg.pr.new/@executor-js/plugin-keychain@1603

@executor-js/plugin-mcp

npm i https://pkg.pr.new/@executor-js/plugin-mcp@1603

@executor-js/plugin-onepassword

npm i https://pkg.pr.new/@executor-js/plugin-onepassword@1603

@executor-js/plugin-openapi

npm i https://pkg.pr.new/@executor-js/plugin-openapi@1603

executor

npm i https://pkg.pr.new/executor@1603

commit: 5dec04e

@RhysSullivan
RhysSullivan marked this pull request as ready for review August 16, 2026 12:43
@RhysSullivan
RhysSullivan merged commit 624e85f into main Aug 16, 2026
21 checks passed
@RhysSullivan
RhysSullivan deleted the oauth-clients-remove-honest branch August 16, 2026 12:43
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