From 9125618441aa179b59f9fa4def4a8638c1d1c007 Mon Sep 17 00:00:00 2001 From: hotragn Date: Sat, 22 Aug 2026 17:29:31 -0400 Subject: [PATCH] Say which of a connector's granted tools it no longer offers MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Closes #106. A grant names `serverId/toolName`, and `listForAgent` reads it against the tool list, so a grant on a tool the vendor has stopped advertising reaches no model. That is a property of what the vendor advertises today rather than of the grant. `google-drive` is one line from proving it: the entry's own comment says the REST transport can be swapped back to MCP and that "tool names match Google's MCP server exactly, so grants survive the swap in either direction", so a name that resolves to nothing today resolves again when it does. Nothing said it was happening. `listServers` asked for grants on the refs of the tools it had just listed, which can only ever return a subset of those — so a grant on a withdrawn tool appeared nowhere, on the one screen an administrator reads to answer what a Bot may do. Reported, not pruned, which is the decision the issue asked to be made deliberately. `refreshTools` is the only place a prune could go and it is the wrong place: the tool list is replaced by a `delete` and then an `insert`, so a failure between them already empties a server, and a vendor answering with an empty list is not a failure at all — every tool row goes, `lastError` is set to null and the refresh is stamped as healthy. Pruning there means one bad answer from a vendor silently revokes every grant on that connector and the trail says the refresh went fine. That turns a visible, inert discrepancy into an invisible, destructive one. So: `listServers` reports them, the connector's page draws them under "Held but not offered", and a refresh that leaves any behind writes a `configuration.changed` row naming the refs and the Bots. The row is the part that answers the transport swap rather than only displaying it — the discrepancy enters the trail when it arises, instead of the only record of the gap being its absence. An explicit "drop these" action for an administrator is now cheap and is left undone, because revoking should stay something somebody decided. Grants are untouched, so the run-time behaviour is unchanged. There is a test for that specifically: reporting a grant must not become honouring one. --- CHANGELOG.md | 18 ++- app/src/lib/plugins/queries.ts | 17 +++ app/src/routes/_authed/admin/plugins/$key.tsx | 40 ++++++ server/src/plugins/store.ts | 119 +++++++++++++++++- server/tests/plugin-store.integration.test.ts | 76 +++++++++++ 5 files changed, 266 insertions(+), 4 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 8d35f4a5..f9102ee9 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -77,6 +77,23 @@ Refusals are now on the audit trail as `agent.dial_refused`, with the address an refused run already told the person what happened; nothing told the deployment, and an agent that has quietly started redirecting somewhere it should not is worth being able to count. +### A connector says which of its granted tools it no longer offers + +A grant names `serverId/toolName`, and a Bot is told about a tool only when the grant and the vendor's +current tool list agree — so a grant on a tool the vendor has stopped listing reaches no model. That +is a property of what the vendor advertises today rather than of the grant, and nothing said it was +happening: the plugins page built its grant list from the advertised tools, so such a grant appeared +nowhere at all. The one screen an administrator reads to answer "what may this Bot do" was quietly +leaving some of the answer out. + +A connector's page now has a "Held but not offered" section listing them, with how many Bots hold +each, and a refresh that leaves any behind writes an audit row naming the refs and the Bots. The +grants themselves are untouched: a tool the vendor starts listing again is offered again, and revoking +stays a decision somebody makes rather than a side effect of a vendor's bad afternoon. + +Nothing changes for a connector whose grants all match its tool list, which is the normal case — the +section is not drawn and no row is written. + ## 0.0.4 ### A click citing a ref this deployment cannot resolve is refused @@ -102,7 +119,6 @@ the decision row is written, so an action somebody tried to take still appears o **A deployment may see refusals it did not see before.** That is the point: those are the actions that were being carried out without the boundary seeing what they touched. A Bot that meets one takes a fresh snapshot and continues. - ### A package ships its skills, so tool selection works on a clone Tool selection narrows a Bot's tools to the ones its matching skills declare, and a deployment starts diff --git a/app/src/lib/plugins/queries.ts b/app/src/lib/plugins/queries.ts index 27accab6..19dc6008 100644 --- a/app/src/lib/plugins/queries.ts +++ b/app/src/lib/plugins/queries.ts @@ -14,6 +14,21 @@ export type PluginTool = { grantedTo: string[]; }; +/** + * A grant on a tool this server does not currently advertise. + * + * Held and not offered: nothing reaches a model, because a Bot is told about a tool only when the + * grant and the tool list agree. That is a fact about what the vendor advertises today, not about the + * grant, so it is reported rather than quietly dropped — a connector that starts advertising the name + * again offers it again. + */ +export type WithdrawnGrant = { + /** `/`. What a grant names, and what an administrator revokes. */ + ref: string; + name: string; + grantedTo: string[]; +}; + export type PluginServer = { id: string; title: string; @@ -28,6 +43,8 @@ export type PluginServer = { lastError: string | null; addedBy: string | null; tools: PluginTool[]; + /** Empty for a healthy connector. See {@link WithdrawnGrant}. */ + withdrawn: WithdrawnGrant[]; }; export type PluginSkill = { diff --git a/app/src/routes/_authed/admin/plugins/$key.tsx b/app/src/routes/_authed/admin/plugins/$key.tsx index 9733d61c..2e824918 100644 --- a/app/src/routes/_authed/admin/plugins/$key.tsx +++ b/app/src/routes/_authed/admin/plugins/$key.tsx @@ -519,6 +519,46 @@ function RouteComponent() { ) : null} + {/* + * Only when there is something to say. An empty section here would teach a reader to scroll past + * a heading that is usually blank, which is the opposite of the point. + * + * Its own section rather than rows inside Tools, because these are not tools: they are not + * listed by the vendor, there is no page to open for one, and putting them in the same list + * would make the count above it wrong. + */} + {server && server.withdrawn.length > 0 ? ( + + + {server.withdrawn.map((held, index) => ( + + + + + {held.name} + + + Not listed by {title} + {server.toolsRefreshedAt ? ` as of the last refresh` : ""} + . + + + + + {grantSummary(held.grantedTo.length, bots.length)} + + + + {index !== server.withdrawn.length - 1 && } + + ))} + + + ) : null} + setDialog(open ? dialog : null)} open={dialog !== null} diff --git a/server/src/plugins/store.ts b/server/src/plugins/store.ts index 6096a1e3..745f736a 100644 --- a/server/src/plugins/store.ts +++ b/server/src/plugins/store.ts @@ -1,4 +1,4 @@ -import { and, asc, eq, inArray, isNull, or } from "drizzle-orm"; +import { and, asc, eq, inArray, isNull, or, sql } from "drizzle-orm"; import { type AuditStore, recordAuditEvent } from "../audit"; import { type ActionPolicy, @@ -57,6 +57,28 @@ export type ToolRecord = { grantedTo: string[]; }; +/** + * A grant naming a tool this server does not currently advertise. + * + * Held and not offered. `listForAgent` reads the grant against the tool list, so nothing reaches a + * model — but that is a property of what the vendor is advertising today rather than of the grant, and + * it changes the moment the vendor advertises the name again. Google's Drive entry says so in its own + * comment: the REST transport is one line from being swapped back to MCP, and "tool names match + * Google's MCP server exactly, so grants survive the swap in either direction". + * + * So it is reported rather than pruned. A grant is the record of a decision somebody made, and the + * refresh that would have deleted it is not a safe place to decide from: the tool list is replaced by + * a `delete` and then an `insert`, and a vendor answering with an empty list is a success, so one bad + * answer would revoke every grant on that server and stamp the refresh as healthy. + */ +export type WithdrawnGrant = { + /** `/`, exactly as the grant is stored. */ + ref: string; + /** The tool half, for a screen that already has the server. */ + name: string; + grantedTo: string[]; +}; + export type ServerRecord = { id: string; title: string; @@ -71,6 +93,13 @@ export type ServerRecord = { lastError: string | null; addedBy: string | null; tools: ToolRecord[]; + /** + * Grants on tools this server no longer advertises. + * + * Empty for a healthy connector. Non-empty is the discrepancy an administrator should be reading + * about, which is why it is here rather than inferred by a screen comparing two lists. + */ + withdrawn: WithdrawnGrant[]; }; export type SkillRecord = { @@ -334,6 +363,35 @@ export function createPluginStore(options: PluginStoreOptions) { return byRef; } + /** + * Every MCP grant belonging to these servers, whether or not the tool is still advertised. + * + * {@link grantsFor} asks about refs somebody already has, which is the wrong question when the + * point is to find the ones nothing else knows about: called with the advertised refs it can only + * ever return a subset of them, so a grant on a withdrawn tool is invisible by construction. + * + * Matched on the server half in the query rather than by reading every grant and splitting here. + * `split_part` rather than a `LIKE` prefix, because a server id is text a person can choose for a + * custom server and `%` in one would silently widen the match. + */ + async function mcpGrantsForServers(serverIds: string[]) { + if (serverIds.length === 0) return new Map(); + const rows = await database + .select({ ref: pluginGrants.ref, agentId: pluginGrants.agentId }) + .from(pluginGrants) + .where( + and( + eq(pluginGrants.kind, "mcp"), + inArray(sql`split_part(${pluginGrants.ref}, '/', 1)`, serverIds), + ), + ); + const byRef = new Map(); + for (const row of rows) { + byRef.set(row.ref, [...(byRef.get(row.ref) ?? []), row.agentId]); + } + return byRef; + } + /** The refs each of these skills declares, keyed by skill id. Skills with none are absent. */ async function toolsDeclaredBy(skillIds: string[]) { if (skillIds.length === 0) return new Map(); @@ -739,6 +797,41 @@ export function createPluginStore(options: PluginStoreOptions) { }) .where(eq(mcpServers.id, serverId)); + /* + * A grant left pointing at nothing goes in the trail, at the moment it starts pointing at + * nothing. + * + * Reporting it on a screen answers "what is true now", which somebody has to go and look at. + * This answers "when did it stop being offered, and what was holding it" — the question asked + * after a transport is swapped back and a name starts resolving again. Without the row, the + * only record of the gap is its absence. + * + * Not a refusal and not an error, so `configuration.changed` rather than a new event type: + * nothing was denied and the refresh succeeded. Written after the tool list is replaced, so + * what it names is what is actually left over. + */ + const advertised = new Set(tools.map((tool) => tool.name)); + const stranded = [...(await mcpGrantsForServers([serverId])).entries()] + .filter(([ref]) => !advertised.has(ref.slice(serverId.length + 1))) + .sort(([left], [right]) => left.localeCompare(right)); + + if (stranded.length > 0) { + await recordAuditEvent(auditStore, { + eventType: "configuration.changed", + targetType: "mcp_server", + targetId: serverId, + payload: { + actor: actorId, + change: "grants_not_advertised", + server: serverId, + // The refs, because that is what a grant is keyed on and what an administrator revokes. + refs: stranded.map(([ref]) => ref), + bots: [...new Set(stranded.flatMap(([, agents]) => agents))], + note: "Held by a Bot and not offered to any model, because this server no longer advertises the tool. Offered again if it starts.", + }, + }); + } + return { tools: tools.length }; } catch (error) { const message = @@ -775,8 +868,13 @@ export function createPluginStore(options: PluginStoreOptions) { ) .orderBy(asc(mcpTools.name)); - const grants = await grantsFor( - "mcp", + /* + * Every grant on these servers, not only the ones matching a tool that is still advertised. + * Asking about the advertised refs answers "who holds what is offered", which cannot report the + * grants that are the point here — see `mcpGrantsForServers`. + */ + const grants = await mcpGrantsForServers(rows.map((row) => row.id)); + const advertised = new Set( tools.map((tool) => `${tool.serverId}/${tool.name}`), ); @@ -808,6 +906,21 @@ export function createPluginStore(options: PluginStoreOptions) { grantedTo: grants.get(ref) ?? [], }; }), + /* + * Sorted by ref so the list is stable between reads, which matters because this is the one + * place a discrepancy is reported and a reader comparing two visits should see the same + * order. + */ + withdrawn: [...grants.entries()] + .filter( + ([ref]) => ref.startsWith(`${row.id}/`) && !advertised.has(ref), + ) + .sort(([left], [right]) => left.localeCompare(right)) + .map(([ref, grantedTo]) => ({ + ref, + name: ref.slice(row.id.length + 1), + grantedTo, + })), }; }); }, diff --git a/server/tests/plugin-store.integration.test.ts b/server/tests/plugin-store.integration.test.ts index add506a8..b76c4597 100644 --- a/server/tests/plugin-store.integration.test.ts +++ b/server/tests/plugin-store.integration.test.ts @@ -443,3 +443,79 @@ describe("the trail can be read by a second reader", () => { expect(row?.bot).toBeTruthy(); }); }); + +/** + * A grant outliving the tool it names. + * + * The runtime already handles it: `listForAgent` reads the grant against the tool list, so a tool the + * vendor has stopped advertising reaches no model. What was missing is that nothing said so — the + * plugins page derives its grant list from the advertised refs, so a grant on a withdrawn tool was + * invisible on the one screen an administrator reads to answer "what may this Bot do". + */ +describe("a grant on a tool the vendor no longer lists", () => { + const withdrawnName = `withdrawn_${suite}`; + const withdrawnRef = `${serverId}/${withdrawnName}`; + + afterAll(async () => { + await database + .delete(pluginGrants) + .where( + and( + eq(pluginGrants.ref, withdrawnRef), + eq(pluginGrants.agentId, holderId), + ), + ); + await database + .delete(mcpTools) + .where( + and(eq(mcpTools.serverId, serverId), eq(mcpTools.name, withdrawnName)), + ); + }); + + test("is reported as held and not offered, and still reaches no model", async () => { + // Advertised once, which is how a grant comes to exist against it. + await database + .insert(mcpTools) + .values({ + serverId, + name: withdrawnName, + description: "Listed by the vendor when the grant was made.", + }) + .onConflictDoNothing(); + await store.grant("mcp", withdrawnRef, holderId, "admin@openbot.local"); + + // Then withdrawn. A refresh replaces the tool list wholesale, so this is what one does to a name + // the vendor has stopped offering. + await database + .delete(mcpTools) + .where( + and(eq(mcpTools.serverId, serverId), eq(mcpTools.name, withdrawnName)), + ); + + const drive = (await store.listServers()).find( + (server) => server.id === serverId, + ); + + // Not a tool: it is not in the list the vendor gave, so it must not be counted as one. + expect(drive?.tools.map((tool) => tool.ref)).not.toContain(withdrawnRef); + // But it is reported, with who holds it, which is the whole point. + expect(drive?.withdrawn.map((held) => held.ref)).toContain(withdrawnRef); + const held = drive?.withdrawn.find((row) => row.ref === withdrawnRef); + expect(held?.name).toBe(withdrawnName); + expect(held?.grantedTo).toContain(holderId); + + // And the property that made it inert in the first place is unchanged. This is the assertion that + // would fail if reporting a grant had turned into honouring one. + const offered = await store.listForAgent(holderId); + expect(offered.tools.map((tool) => tool.ref)).not.toContain(withdrawnRef); + }); + + test("a healthy connector reports nothing withdrawn", async () => { + // The empty case, because a field that is only ever exercised non-empty is a field whose empty + // shape nobody has checked — and this one is read by a screen that hides itself when it is empty. + const drive = (await store.listServers()).find( + (server) => server.id === serverId, + ); + expect(drive?.withdrawn.map((row) => row.ref)).not.toContain(ref); + }); +});