Report honest removed flag from oauth.clients.remove

.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.

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",

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 });
+      }),
+    ),
+  );
 });