Remove a connector's grants when the connector is removed

[zopeVaibhav]

What this changes

Removing a connector now removes its tool grants.

removeServer revoked the server's credential, every person's stored token and every brokered
account, then deleted the mcp_servers row — and left every plugin_grants row naming that server
standing. The rows were inert while the app was gone and invisible while they were inert: the only
surface that reports a grant nothing advertises builds it per server row, and there was no row. Add
the app back and the id is the same (composio-<slug>), the action names are the same, and every
grant resolves again, with nobody having granted anything.

The delete of the grants and the delete of the server row are now one transaction, so a failure
between them cannot leave a server whose grants have gone. The removal's audit row carries
releasedGrants and the bots that held them, because until now the trail recorded the removal and
said nothing about what it took away. 0041_drop_orphaned_mcp_grants.sql drops the rows earlier
removals left, scoped to kind = 'mcp' and to refs whose server half names no mcp_servers row, so
skill and bot grants and every live grant are untouched.

Where it runs

• New state that outlives a request? None. One delete statement more inside an existing method.
• What happens on the second replica? Two administrators removing the same app concurrently both
  run the same delete; the second removes nothing and the row is gone either way. The grants delete
  and the server delete are in one transaction, so no replica can observe the server gone with its
  grants still standing.
• Anything serialised? The pair is a transaction. The delete is keyed on the same
  split_part(ref, '/', 1) the store already uses to find a server's grants, not on a LIKE prefix,
  because a custom server id is text an administrator chooses and % in one would widen the match.
• Anything fanned out to a browser? No.
• New listener, port, or schedule? No.

Boundary and audit

• Every acting call still goes through the gateway: resolve, decide, audit, then act.
• New refusals and new failures each write a row. No new refusal; the existing
  configuration.changed removal row now names what it released.
• Nothing new is trusted from the client. The server id comes from the path as before, and the
  refs deleted are derived from it.

Changelog

• CHANGELOG.md, under Unreleased.

Proof

server/tests/mcp-grant-removal.integration.test.ts is new and asserts both halves: a removal takes
the app's grants and names them on the trail while leaving another connector's grant alone, and a
re-added app a Bot was never granted again refuses the call.

Run against a migrated openbot_test:

$ bun test server/tests/mcp-grant-removal.integration.test.ts
 2 pass
 0 fail

The same file on main in a detached worktree, which is the control that says the test is about this
change rather than about the fixtures:

$ bun test server/tests/mcp-grant-removal.integration.test.ts
(fail) removing an app takes its grants with it, and adding it back grants nothing
(fail) a re-added app a Bot was never granted again refuses the call
 0 pass
 2 fail

The migration was run against a database holding one orphaned mcp grant, one live mcp grant and
one skill grant: it deleted the orphan and left the other two.

Also green: plugin-store.integration.test.ts, skill-uninstall-grants.integration.test.ts,
plugin-routes.test.ts, composio-connections.test.ts and migration-journal.test.ts (290 tests),
plus typecheck, lint and format:check.

No surface changed, so there is nothing to screenshot.

Closes #571.

[Hotragn]

Reviewing this alongside #574 since they are the same seam. The diagnosis is right, the transaction is the right shape, and the migration is a faithful analogue of 0040_drop_orphaned_skill_grants — same NOT EXISTS against the owning table, with split_part(ref, '/', 1) standing in for the whole ref because an mcp ref is <serverId>/<tool>. Having the precedent one migration back is worth more than any argument either of us could make about it.

I checked the thing I would want checked before a DELETE migration ships: whether a legitimate kind = 'mcp' grant can name a server with no mcp_servers row, in which case the migration would take live grants. It cannot name one that works — a call resolves the server row — so anything the migration deletes is inert, and deleting it is right. Good.

But that check turned up the part I think is missing.

Removal was one door. The grant route is the other, and it is still open

plugins/routes.ts:2273-2277, inside enablementRefusal:

if (kind === "mcp") {
  return actor.isAdmin
    ? null
    : "An administrator decides which Bots may reach a tool.";
}

That is the whole check for an mcp grant. store.grant (store.ts:4389) is then a bare upsert with no lookup. So POST /grants with {"kind":"mcp","ref":"composio-slack/SLACK_SEND_MESSAGE","agentId":"..."} stores a grant for an app this deployment has not added, and every consequence in your changelog entry follows from there:

• Nothing shows it, for exactly the reason you give — the surface that reports a grant nothing advertises is built per server row, and there is no row.
• Add the app later and the id is the same, the action names are the same, and the grant resolves.
• Nobody granted anything for that app, and the trail has no row saying they did.

So after this lands, the migration has cleaned history and the transaction has closed the removal path, and an administrator can still mint a fresh orphan in one request. It is admin-only, so this is a consistency hole rather than a privilege one — but the changelog line reads as though the class of bug is gone, and it is one door narrower.

And the codebase already states the principle your mcp branch is missing, four lines below it. The bot branch does the lookup, on this reasoning (routes.ts:2297-2300):

A grant that could never do anything is refused rather than stored, from both ends.

An mcp grant naming no server is precisely a grant that could never do anything. The asymmetry looks like the bot kind having been written later and more carefully rather than a decision about mcp.

Two notes if you take it:

• Grant only. intent === "revoke" must stay unchecked, for the reason the docblock on that parameter already gives: applying the check to a revoke means the orphans this very migration exists to clean could never have been deleted through the UI. Your migration is the one-off; the route still needs to let an administrator remove a dead row by hand.
• It is a real question whether this belongs in this PR. It is a different door to the same room, and the argument for one PR is that the invariant — no mcp grant names a server that does not exist — is what you are actually establishing, and it is not established while one writer can break it. The argument for a follow-up is that this diff is already a transaction plus a migration. I would lean toward here, since the migration is what makes the invariant true for the first time and it is cheapest to hold while it is true. Your call, and I am happy to send it either way.

One small thing

The changelog entry has no blank line after it, so ### Removing a connector takes its grants with it runs straight into ### A vendor that broke no longer reads as a refusal…:

+behind. Grants for other connectors, and skill grants, are untouched.
 ### A vendor that broke no longer reads as a refusal to a Bot running its own loop

Every other entry in that file has one, and most markdown renderers need it before a heading.

Otherwise: recording releasedGrants and the bots that held them on the removal row is the right call and the part I would have asked for if it were not there — "we removed the app" and "we removed the app and took these six actions off these two Bots" are different facts, and only the second one answers the question somebody has six months later.