Remove a skill's grants when it is uninstalled, so a new skill under that name starts on no Bot

[zopeVaibhav]

What this changes

Fixes #557.

Uninstalling a skill deleted its row in skills and left its rows in plugin_grants, whose ref is plain text with no foreign key. listForAgent resolves skill grants by slug, and any signed-in person may write a slug nobody holds, so the next skill written under that name was offered on every Bot the old one had been granted to. That included Bots its author does not own and Bots the deployment shares with everybody. The grant route refuses exactly that when asked directly (server/src/plugins/routes.ts:780), and the leftover row got there without asking.

uninstallSkill now deletes the skill's grants and the skill in one transaction. Migration 0040_drop_orphaned_skill_grants deletes the skill grants earlier uninstalls already left behind, because a deployment that has uninstalled a granted skill is exposed until they are gone. A grant naming no skill was never listed or offered anywhere, so removing those rows changes nothing a person can see.

The fix sits at removal rather than at read time on purpose. Filtering listForAgent by ownership would also hide skills an administrator put on a Bot the skill's author does not own, which the grant route allows (server/src/plugins/routes.ts:765). Both delete statements name kind = 'skill', so mcp and bot grants in the same table are untouched. The skill_uninstalled audit row is unchanged. The tenant package already refuses to grant a skill it did not write (server/src/tenant-package.ts:914) and needed nothing.

Where it runs

• New state that outlives a request? None. One more delete inside an existing store method, and a one-time migration.
• What happens on the second replica? The same thing as on the first. The rows are in Postgres, the transaction commits once, and the migration runs through the same migrate step as every other.
• Anything serialised? The two deletes share a transaction, so a failure cannot leave the skill without its grants or the grants without the skill. One window is not closed. POST /api/plugins/grants checks that the skill exists and then inserts without a lock, so a grant committed in the moment after an uninstall commits could still leave a row. Only the skill's owner or an administrator may grant it, so nobody else can arrange that. Closing it means locking the skill row in the generic grant path, which I kept out of this change.
• Anything fanned out to a browser? No.
• New listener, port, or schedule? No.

Boundary and audit

• No acting call is added or changed.
• No new refusal or failure. Uninstalling writes the same configuration.changed row it did before. The grants it now removes are not recorded one by one, in the same way that deleting a Bot cascades its grants away.
• Nothing new is trusted from the client.

Changelog

• A section under Unreleased in CHANGELOG.md.

Proof

server/tests/skill-uninstall-grants.integration.test.ts drives createPluginRoutes as non-administrators against a migrated test database. It covers a person's skill reused by another person, a deployment skill on a shared Bot reused by a person, and a check that uninstalling removes that skill's grants and leaves another skill's grant on the same Bot.

On main at a96d88c the file fails 3 of 3, and the failures are the bug itself. Alice's Bot is offered "instructions": "Bob's words.", the shared Bot is offered "instructions": "Alice's words.", and all three grants are still in plugin_grants. In the same run, each direct grant attempt is refused with 403, so the ownership rule was working and was bypassed.

On this branch:

• The new file passes 3 of 3. Run with skill-ownership.integration.test.ts and skill-tools.integration.test.ts, the three files pass 29 of 29.
• The migration was run against seeded rows inside a transaction that was then rolled back. There were 4 rows before and it reported DELETE 1: only the skill grant naming a missing skill went, and the live skill grant and the mcp and bot rows with the same ref stayed.
• All migrations apply to a fresh database (migrations applied successfully, 38 rows in __drizzle_migrations).
• bun run typecheck exits 0, bun run lint checks 747 files with no fixes, and bun run format:check checks 732 files with no fixes.
• Full bun test with TEST_DATABASE_URL set: 3750 pass on this branch against 3747 on main, exactly the three new tests. Both runs show the same 8 fails and 6 errors, and every one is a Cannot find module in a directory that sits outside the bun workspaces (agent-bot, agent-langgraph, agent-mastra, desktop) and had no install of its own in the fresh worktrees. None of them involve this change.

[davidmckayv]

Landed in #563 — your fix with the migration renumbered 0037→0040 (0037-0039 are the composio migrations already on main). GitHub wouldn't let me push the renumber to this branch, so I folded it in there, credited to you. Thanks!

[davidmckayv]

Superseded by #563 — merged. Thanks @zopeVaibhav.