From 2c9030786cca25e26ef4939a57e70782d2ab3442 Mon Sep 17 00:00:00 2001 From: hotragn Date: Wed, 16 Sep 2026 16:19:05 -0400 Subject: [PATCH] Record what each app answered when one of them refuses an offboarding MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Follow-up to #574, at zopeVaibhav's request on that thread. `retireConnectionsFor` asked the broker to withdraw every brokered account in one loop, then deleted every row, then wrote every audit row. `ComposioBroker.revoke` throws, so one app refusing ended the act before the delete and before the trail — and the apps already withdrawn at Composio kept their `composio_connections` row, kept their `(toolkit, user_id)` gate, and left nothing behind saying the account had ended. That was survivable while repeating the act did nothing. #574 made repeating it the documented recovery and its changelog line tells an administrator to take it, so the second pass asks again for those apps, Composio answers false because the accounts are already gone, and each writes `vendorRevocationRequested: false` about a withdrawal this deployment asked for and got. That field exists for exactly that distinction. Its own docblock: "a reader has to be able to tell an account this deployment acted on from one that outlives it somewhere else." After a partial refusal the rows said the wrong one, permanently, and no later act could correct them. So the answer is kept per app and the refusal is held rather than thrown. Every app is still asked, so a later one is not punished for an earlier one; the apps that answered lose their rows and are recorded with the answer they actually gave; and the first refusal is rethrown after the loop, so the act still fails and the administrator still gets a 500. The delete is now scoped to the apps that answered rather than to the person. Deleting by user id would take the rows of apps that were refused or never reached, and those are the rows the recovery needs — the criterion "an offboarding the vendor refuses leaves the connection standing" already states it. What changes is that the rule now applies to the app it is about rather than to every app in the same act. Co-Authored-By: Claude Opus 5 --- CHANGELOG.md | 11 +++ server/src/plugins/store.ts | 78 ++++++++++++++++---- server/tests/composio-connections.test.ts | 89 +++++++++++++++++++++++ 3 files changed, 165 insertions(+), 13 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index a1edb6a53..2a71ad850 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,6 +8,17 @@ Newest first. `Unreleased` is what is on `main` and not yet tagged. ## Unreleased +### An offboarding one app refuses still records the apps that answered + +Removing somebody withdraws each brokered account they connected. When the broker refused one of +those apps, the whole act stopped before anything was recorded: apps already withdrawn at Composio +kept their `composio_connections` row and left nothing on the trail saying the account had ended, +and the retry that #574 made the recovery then asked again, was told there was nothing to withdraw, +and wrote `vendorRevocationRequested: false` about a withdrawal this deployment had asked for and +got. Each app is now asked, recorded with the answer it actually gave and its row removed, and only +the apps that were refused are left standing for the retry. The act still fails and still answers +500, so a refusal is as loud as it was. + ### The New chat shortcut works on a Russian or Greek keyboard layout Settings lists New chat as Shift+N, and the app matched the character the keystroke wrote. A layout diff --git a/server/src/plugins/store.ts b/server/src/plugins/store.ts index b96d4d60e..ecdc6d161 100644 --- a/server/src/plugins/store.ts +++ b/server/src/plugins/store.ts @@ -6691,22 +6691,65 @@ export function createPluginStore(options: PluginStoreOptions) { * rather than the call. False where there is no broker at all: a deployment whose key has * since been unset can still offboard somebody, and it could not have been calling Composio * either way — but nothing was asked there and the row must not claim otherwise. + * + * ONE APP'S REFUSAL IS ONE APP'S REFUSAL, and until now it was everybody's. A throw out of + * `revoke` left this loop before the delete and before the trail, so three accounts already + * withdrawn at Composio kept their rows and got no row on the trail. That was survivable while + * repeating the act did nothing — and #574 made repeating it the documented recovery, so the + * second pass asks again for those three, Composio answers `false` because the accounts are + * gone, and each writes `vendorRevocationRequested: false` about a withdrawal this deployment + * asked for and got. That field exists to tell an account we acted on from one that outlives + * us somewhere else; those three rows say the wrong one. + * + * So the answer is kept per app and the refusal is held rather than thrown. Every app is still + * asked — a later one is not punished for an earlier one — and the first refusal is rethrown + * below, so the act still fails loudly and the administrator still gets a 500. */ - const vendorRevocationRequested = new Map(); + const withdrawn: { toolkit: string; requested: boolean }[] = []; + const refusals: unknown[] = []; for (const connection of brokered) { - vendorRevocationRequested.set( - connection.toolkit, - broker - ? await broker.revoke({ userId, toolkit: connection.toolkit }) - : false, - ); + try { + withdrawn.push({ + toolkit: connection.toolkit, + requested: broker + ? await broker.revoke({ userId, toolkit: connection.toolkit }) + : false, + }); + } catch (error) { + /* + * Held, and the row deliberately left standing. + * + * "An offboarding the vendor refuses leaves the connection standing" is the existing + * criterion and it is unchanged: the row is the only thing naming which app this person + * connected, repeating the act is the recovery, and repeating it is only possible while + * the row is there. What changes is that the rule now applies to the app it is about + * rather than to every app in the same act. + */ + refusals.push(error); + } } - await database - .delete(composioConnections) - .where(eq(composioConnections.userId, userId)); + /* + * Only the apps that answered, which is the other half of the same correction. + * + * Deleting by user id would take the rows of apps that were refused or never reached, and + * those are exactly the rows the recovery needs. Deleting none — what a throw used to do — + * leaves a row and an open `(toolkit, user_id)` gate for an account that is already gone at + * Composio, so the table claims a connection this person does not have. + */ + if (withdrawn.length > 0) { + await database.delete(composioConnections).where( + and( + eq(composioConnections.userId, userId), + inArray( + composioConnections.toolkit, + withdrawn.map((entry) => entry.toolkit), + ), + ), + ); + } - for (const connection of brokered) { + for (const connection of withdrawn) { retired += 1; await recordAuditEvent(auditStore, { eventType: "mcp.account_disconnected", @@ -6729,12 +6772,21 @@ export function createPluginStore(options: PluginStoreOptions) { * ask. The value of the field is exactly that a reader can tell those apart, so a * constant here would be worse than none. */ - vendorRevocationRequested: - vendorRevocationRequested.get(connection.toolkit) ?? false, + vendorRevocationRequested: connection.requested, }, }); } + /* + * Loud, after every app has been asked and every answer recorded. + * + * The first, because the route turns this into a 500 and one sentence is what reaches the + * administrator; the rest are the same act failing more than once, and the trail above already + * says which apps did not end. Thrown last rather than first so a refusal on one app cannot + * cost the record of another — which is the whole of this change. + */ + if (refusals.length > 0) throw refusals[0]; + return { retired }; }, diff --git a/server/tests/composio-connections.test.ts b/server/tests/composio-connections.test.ts index 61e31bca9..f2f541558 100644 --- a/server/tests/composio-connections.test.ts +++ b/server/tests/composio-connections.test.ts @@ -1304,6 +1304,95 @@ test("an offboarding carries the vendor's answer per app, in a fixed order", asy ]); }); +/** + * ONE APP'S REFUSAL IS ONE APP'S REFUSAL, AND THE APPS THAT ANSWERED ARE RECORDED. + * + * CRITERION. Two apps, the vendor refusing the second: the act still fails, the refused app keeps + * its row and gets no trail row, and the app that WAS withdrawn loses its row and is recorded with + * `vendorRevocationRequested: true`. + * + * REASON. A throw out of `revoke` used to leave the loop before the delete and before the trail, so + * an account already gone at Composio kept its row and its `(toolkit, user_id)` gate and left + * nothing on the trail saying it had ended. That was survivable while repeating the act did + * nothing — and #574 made repeating it the documented recovery, so the second pass asks again for + * that app, the vendor answers `false` because the account is already gone, and the row that + * finally lands says `vendorRevocationRequested: false` about a withdrawal this deployment asked + * for and got. `vendorRevocationRequested` exists to tell an account we acted on from one that + * outlives us somewhere else; that row says the wrong one, permanently, and no later act corrects it. + * + * THE REFUSED APP IS UNCHANGED, which is the criterion the single-app test above already states: + * the row is the only thing naming which app this person connected, and repeating the act is only + * possible while it is there. What is new is that the rule now applies to the app it is about + * rather than to every app in the same act. + */ +test("an offboarding one app refuses still records the app that answered", async () => { + await seedApp({ connect: false }); + await database + .insert(composioConnections) + .values({ toolkit, userId: askerId }); + await database + .insert(composioConnections) + .values({ toolkit: secondToolkit, userId: askerId }); + vendorRefuses = ({ toolkit: asked }) => asked === secondToolkit; + + await expect(store.retireConnectionsFor(askerId, admin)).rejects.toThrow( + /would not withdraw/i, + ); + + // Both were asked. A later app is not punished for an earlier one, and on the previous shape the + // throw left the loop, so an app after the refused one was never reached at all. + expect(asksMade()).toEqual([ + `revoke:${toolkit}/${askerId}`, + `revoke:${secondToolkit}/${askerId}`, + ]); + + // The withdrawn app's row is gone; the refused app's stands, so the recovery still has it. + expect(await connectedToolkitsFor(askerId)).toEqual([secondToolkit]); + + const disconnected = recordedOfType("mcp.account_disconnected"); + expect(disconnected).toHaveLength(1); + expect(disconnected[0]?.payload).toMatchObject({ + server: toolkit, + owner: askerId, + reason: "person_removed", + // The answer this app actually got, written while it was still known. + vendorRevocationRequested: true, + }); +}); + +/** + * AND THE REFUSAL IS STILL THROWN, WHATEVER ORDER IT CAME IN. + * + * CRITERION. The vendor refusing the FIRST app: the second is still asked and still recorded, and + * the act still fails with the refusal rather than reporting a success. + * + * REASON. Holding a refusal instead of throwing it is only correct if it is still thrown. The risk + * this pins is the opposite of the one above — that collecting refusals turns a failed offboarding + * into a reported one, which is the state `an offboarding the vendor refuses leaves the connection + * standing` exists to forbid. + */ +test("a refusal on the first app still fails the act and still asks the second", async () => { + await seedApp({ connect: false }); + await database + .insert(composioConnections) + .values({ toolkit, userId: askerId }); + await database + .insert(composioConnections) + .values({ toolkit: secondToolkit, userId: askerId }); + vendorRefuses = ({ toolkit: asked }) => asked === toolkit; + + await expect(store.retireConnectionsFor(askerId, admin)).rejects.toThrow( + /would not withdraw/i, + ); + + expect(asksMade()).toEqual([ + `revoke:${toolkit}/${askerId}`, + `revoke:${secondToolkit}/${askerId}`, + ]); + expect(await connectedToolkitsFor(askerId)).toEqual([toolkit]); + expect(recordedOfType("mcp.account_disconnected")).toHaveLength(1); +}); + /** * THE ANONYMOUS ACTOR OWNS NOTHING, and `notNull` does not exclude the empty string, so a row at * `(toolkit, "")` is legal. Retiring "nobody" must not be what deletes it — that would be an