From 5dec04e84e851ee98ccfe089094f1929bec751f4 Mon Sep 17 00:00:00 2001 From: Rhys Sullivan <39114868+RhysSullivan@users.noreply.github.com> Date: Sun, 16 Aug 2026 05:29:30 -0700 Subject: [PATCH] Report honest removed flag from oauth.clients.remove 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. --- .changeset/oauth-clients-remove-honest.md | 11 +++ packages/core/sdk/src/core-tools.ts | 22 ++++-- .../core/sdk/src/oauth-remove-client.test.ts | 67 ++++++++++++++++++- 3 files changed, 94 insertions(+), 6 deletions(-) create mode 100644 .changeset/oauth-clients-remove-honest.md diff --git a/.changeset/oauth-clients-remove-honest.md b/.changeset/oauth-clients-remove-honest.md new file mode 100644 index 0000000000..88371592b0 --- /dev/null +++ b/.changeset/oauth-clients-remove-honest.md @@ -0,0 +1,11 @@ +--- +"executor": patch +--- + +**Fix: `oauth.clients.remove` reported success for clients it never removed** + +The tool returned `{ removed: true }` unconditionally. `oauth.removeClient` is idempotent by design at the storage layer — `deleteMany` on a missing row is a no-op, which is the right behaviour for a delete — but the tool mapped that silence to success, so a typo'd slug, an already-deleted client, and the wrong owner were all indistinguishable from a real deletion. + +This bites hardest because clients are keyed by BOTH owner and slug, so the same slug can exist separately under `org` and `user`. An agent sweeping a list of slugs under one hardcoded owner would delete only half of them and report every call as a success, leaving org-owned OAuth apps registered after everything they authorized was gone. + +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. diff --git a/packages/core/sdk/src/core-tools.ts b/packages/core/sdk/src/core-tools.ts index b01952747f..e6dc78ce77 100644 --- a/packages/core/sdk/src/core-tools.ts +++ b/packages/core/sdk/src/core-tools.ts @@ -819,7 +819,7 @@ export const coreToolsPlugin = definePlugin((options: CoreToolsPluginOptions = { tool({ name: "oauth.clients.remove", description: - "Remove an owner-scoped OAuth client by owner and slug. Existing connections are not cascaded.", + "Remove an owner-scoped OAuth client by owner and slug. `removed: false` means no client matched that owner and slug — clients are keyed by BOTH, so the same slug can exist separately under `org` and `user`. Existing connections are not cascaded.", inputSchema: OAuthRemoveClientInputStd, outputSchema: RemovedOutputStd, // Removing a client breaks token refresh for every connection that @@ -828,10 +828,22 @@ export const coreToolsPlugin = definePlugin((options: CoreToolsPluginOptions = { // `sources.bindings.remove`. annotations: { requiresApproval: true }, execute: (input: typeof OAuthRemoveClientInput.Type, { ctx }) => - Effect.map( - ctx.oauth.removeClient(input.owner as Owner, OAuthClientSlug.make(input.slug)), - () => ({ removed: true }), - ), + Effect.gen(function* () { + const owner = input.owner as Owner; + const slug = OAuthClientSlug.make(input.slug); + // `removeClient` is idempotent by design at the storage layer, so + // on its own it cannot distinguish a real deletion from a typo'd + // slug or the wrong owner — and a caller sweeping a list of slugs + // under one hardcoded owner would read every no-op as success. + // Checking the visible set first is what keeps `removed` honest. + const clients = yield* ctx.oauth.listClients(); + const matched = clients.some( + (client) => client.owner === owner && String(client.slug) === String(slug), + ); + if (!matched) return { removed: false }; + yield* ctx.oauth.removeClient(owner, slug); + return { removed: true }; + }), }), tool({ name: "oauth.probe", diff --git a/packages/core/sdk/src/oauth-remove-client.test.ts b/packages/core/sdk/src/oauth-remove-client.test.ts index a5c90af07c..7ce3ffccc4 100644 --- a/packages/core/sdk/src/oauth-remove-client.test.ts +++ b/packages/core/sdk/src/oauth-remove-client.test.ts @@ -5,7 +5,7 @@ import { join } from "node:path"; import { describe, expect, it } from "@effect/vitest"; import { Effect } from "effect"; -import { OAuthClientSlug } from "./ids"; +import { OAuthClientSlug, ToolAddress } from "./ids"; import { makeTestWorkspaceHarness, memoryCredentialsPlugin } from "./test-config"; // removeClient permanently deletes an owner-scoped oauth_client row, keyed by @@ -175,4 +175,69 @@ describe("oauth.removeClient", () => { }), ), ); + + // The `oauth.clients.remove` TOOL reports `removed` honestly on top of the + // idempotent service call above, so an agent sweeping a list of slugs cannot + // read a no-op as a deletion. + it.effect("the remove tool distinguishes a real deletion from a no-op", () => + Effect.scoped( + Effect.gen(function* () { + const { executor } = yield* makeTestWorkspaceHarness({ + plugins, + coreTools: {}, + }); + const remove = ToolAddress.make("executor.coreTools.oauth.clients.remove"); + + // The same slug registered under BOTH owners — the shape that made a + // hardcoded `owner: "user"` sweep silently skip the org copy. + for (const owner of ["org", "user"] as const) { + yield* executor.oauth.createClient({ + owner, + slug: ORG_CLIENT, + authorizationUrl: "https://acme.test/authorize", + tokenUrl: "https://acme.test/token", + grant: "authorization_code", + clientId: `${owner}-client-id`, + clientSecret: `${owner}-secret`, + }); + } + + // A slug that never existed is not a removal. + expect( + yield* executor.execute( + remove, + { owner: "user", slug: "never-existed" }, + { onElicitation: "accept-all" }, + ), + ).toEqual({ removed: false }); + + // Removing the user copy leaves the org copy, which still reports as a + // real removal of its own rather than as already-gone. + expect( + yield* executor.execute( + remove, + { owner: "user", slug: String(ORG_CLIENT) }, + { onElicitation: "accept-all" }, + ), + ).toEqual({ removed: true }); + expect( + yield* executor.execute( + remove, + { owner: "org", slug: String(ORG_CLIENT) }, + { onElicitation: "accept-all" }, + ), + ).toEqual({ removed: true }); + + // Both are gone, and a repeat of either is now a no-op. + expect(yield* executor.oauth.listClients()).toEqual([]); + expect( + yield* executor.execute( + remove, + { owner: "org", slug: String(ORG_CLIENT) }, + { onElicitation: "accept-all" }, + ), + ).toEqual({ removed: false }); + }), + ), + ); });