diff --git a/.changeset/core-tools-integrations-remove.md b/.changeset/core-tools-integrations-remove.md new file mode 100644 index 0000000000..d4932f8ccb --- /dev/null +++ b/.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. 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 2b0ef306e9..e6dc78ce77 100644 --- a/packages/core/sdk/src/core-tools.ts +++ b/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", diff --git a/packages/core/sdk/src/executor.test.ts b/packages/core/sdk/src/executor.test.ts index f5d18cfc87..029b06d75d 100644 --- a/packages/core/sdk/src/executor.test.ts +++ b/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({ 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 }); + }), + ), + ); });