[pull] main from UsefulSoftwareCo:main

.changeset/core-tools-integrations-remove.md

@@ -0,0 +1,9 @@
+---
+"executor": patch
+---
+
+**Add `integrations.remove` to the core tools so an agent can drop a catalog integration**
+
+`integrations.list` advertises `canRemove` per integration, but nothing on the agent surface could act on it: removal existed only on the HTTP API and the web console, so an agent that could add an integration could never take one back out. Cleaning up a catalog meant clicking through the UI once per integration.
+
+The core-tools plugin now contributes `integrations.remove`, taking the `slug` reported by `integrations.list` and cascading to every connection under the integration and the tools those produced. It is approval-gated, being strictly more destructive than `connections.remove`. The `removed` flag is honest rather than always-true: `false` means no catalog row matched, so an already-absent slug and a built-in namespace like `executor` are distinguishable from a real removal, and an integration pinned with `canRemove: false` is refused with `IntegrationRemovalNotAllowedError` instead of silently surviving.

.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

@@ -51,6 +51,8 @@ const IntegrationsListOutput = Schema.Struct({
   integrations: Schema.Array(IntegrationOutput),
 });
 
+const IntegrationRemoveInput = Schema.Struct({ slug: Schema.String });
+
 const DetectInput = Schema.Struct({ url: Schema.String });
 const DetectOutput = Schema.Struct({
   results: Schema.Array(
@@ -331,6 +333,7 @@ const OAuthCancelInput = Schema.Struct({
 
 // Standard-schema versions for the tool() builder.
 const IntegrationsListOutputStd = schemaToStandard(IntegrationsListOutput);
+const IntegrationRemoveInputStd = schemaToStandard(IntegrationRemoveInput);
 const DetectInputStd = schemaToStandard(DetectInput);
 const DetectOutputStd = schemaToStandard(DetectOutput);
 const ConnectionsListInputStd = schemaToStandard(ConnectionsListInput);
@@ -559,6 +562,30 @@ export const coreToolsPlugin = definePlugin((options: CoreToolsPluginOptions = {
               })),
             })),
         }),
+        tool({
+          name: "integrations.remove",
+          description:
+            "Remove an integration from the workspace catalog by slug, dropping every connection under it and every tool those produced. `removed: false` means no catalog row matched: the slug was already gone, or it names a built-in namespace that is not catalog-backed. Integrations whose `canRemove` is false are refused.",
+          inputSchema: IntegrationRemoveInputStd,
+          outputSchema: RemovedOutputStd,
+          // Strictly more destructive than `connections.remove`, which is
+          // already approval-gated: this cascades to every connection under the
+          // integration and takes the catalog row with it, so re-adding means
+          // re-importing the definition, not just reconnecting an account.
+          annotations: { requiresApproval: true },
+          execute: (input: typeof IntegrationRemoveInput.Type, { ctx }) =>
+            Effect.gen(function* () {
+              const slug = IntegrationSlug.make(input.slug);
+              // `core.integrations.get` reads catalog ROWS only, so a built-in
+              // static namespace reports absent here. Checking first is what
+              // keeps `removed` honest — the underlying remove is a silent
+              // no-op for a slug it can't find.
+              const existing = yield* ctx.core.integrations.get(slug);
+              if (existing === null) return { removed: false };
+              yield* ctx.core.integrations.remove(slug);
+              return { removed: true };
+            }),
+        }),
         tool({
           name: "connections.list",
           description:
@@ -792,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
@@ -801,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/executor.test.ts

@@ -38,6 +38,7 @@ const memoryProvider = (): CredentialProvider => {
 };
 
 const INTEG = IntegrationSlug.make("demo");
+const PINNED = IntegrationSlug.make("demo-pinned");
 const TEMPLATE = AuthTemplateSlug.make("apiKey");
 const CONN = ConnectionName.make("main");
 
@@ -94,6 +95,15 @@ const demoPlugin = definePlugin(() => ({
         description: "Demo",
         config: {},
       }),
+    /** A catalog row the host pins in place (`canRemove: false`), the shape
+     *  `integrations.remove` has to refuse rather than drop. */
+    seedPinned: () =>
+      ctx.core.integrations.register({
+        slug: PINNED,
+        description: "Demo (pinned)",
+        config: {},
+        canRemove: false,
+      }),
     storagePut: (owner: "org" | "user", key: string, value: string) =>
       ctx.storage.put(owner, key, value),
     storageList: () => ctx.storage.list(),
@@ -347,6 +357,66 @@ describe("createExecutor", () => {
     }),
   );
 
+  it.effect("removes catalog integrations through the built-in Executor tools", () =>
+    Effect.gen(function* () {
+      const executor = yield* makeTestExecutor({
+        plugins: [demoPlugin] as const,
+        coreTools: {},
+      });
+      yield* executor.demo.seed();
+      yield* executor.demo.seedPinned();
+      yield* executor.execute(
+        ToolAddress.make("executor.coreTools.connections.create"),
+        {
+          owner: "org",
+          name: String(CONN),
+          integration: String(INTEG),
+          template: String(TEMPLATE),
+          from: { provider: "memory", id: "secret-token" },
+        },
+        { onElicitation: "accept-all" },
+      );
+
+      const remove = ToolAddress.make("executor.coreTools.integrations.remove");
+
+      // Removing the integration cascades to the connections under it.
+      const removed = yield* executor.execute(
+        remove,
+        { slug: String(INTEG) },
+        { onElicitation: "accept-all" },
+      );
+      expect(removed).toEqual({ removed: true });
+      const listed = yield* executor.integrations.list();
+      expect(listed.map((integration) => String(integration.slug))).not.toContain(String(INTEG));
+      expect(yield* executor.connections.list()).toHaveLength(0);
+
+      // An already-absent slug and a built-in namespace both report honestly
+      // instead of claiming a removal that never happened.
+      expect(
+        yield* executor.execute(remove, { slug: String(INTEG) }, { onElicitation: "accept-all" }),
+      ).toEqual({ removed: false });
+      expect(
+        yield* executor.execute(remove, { slug: "executor" }, { onElicitation: "accept-all" }),
+      ).toEqual({ removed: false });
+
+      // A pinned integration is refused, and survives the attempt.
+      const refused = yield* Effect.result(
+        executor.execute(remove, { slug: String(PINNED) }, { onElicitation: "accept-all" }),
+      );
+      expect(Result.isFailure(refused)).toBe(true);
+      if (!Result.isFailure(refused)) return;
+      expect(Predicate.isTagged(refused.failure, "ToolInvocationError")).toBe(true);
+      expect(
+        Predicate.isTagged(
+          (refused.failure as { readonly cause?: unknown }).cause,
+          "IntegrationRemovalNotAllowedError",
+        ),
+      ).toBe(true);
+      const afterRefusal = yield* executor.integrations.list();
+      expect(afterRefusal.map((integration) => String(integration.slug))).toContain(String(PINNED));
+    }),
+  );
+
   it.effect("surfaces failed tool sync diagnostics through connection tools", () =>
     Effect.gen(function* () {
       const executor = yield* makeTestExecutor({

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