Say which of a connector's granted tools it no longer offers - #187
Conversation
9e590b4 to
0866a4e
Compare
|
Merge note. This and #189 both add a section directly under No other file is shared: this one is |
davidmckayv
left a comment
There was a problem hiding this comment.
Drove it rather than read it.
Granted google-drive/delete_file to a Bot — a tool the connector does not advertise — and loaded the connector page. The new section appears:
Held but not offered
This vendor no longer lists these, so no Bot is told about them and no model can call one. The grant is still recorded, and the tool would be offered again if the vendor started listing it. Revoke from the Bot's own page if that is not what you want.
delete_file— Not listed by Google Drive as of the last refresh. 1 of 5 Bots
Before this, that grant was invisible on the one screen an administrator reads to answer what a Bot may do. A page that shows part of the answer as though it were all of it is the failure worth fixing here.
Refusing to prune is the right call and worth the comment it got: refreshTools cannot tell "the vendor withdrew a tool" from "the vendor returned an empty list", so pruning there would let one bad response revoke every grant and report the refresh healthy.
One thing to tidy, not blocking: the grant lookup and audit write sit inside refreshTools' try, so a failed audit write marks a successful refresh as failed and zeroes the tool count. Elsewhere the repo catches audit-write failures rather than letting them fail the operation. Worth a follow-up.
Checks: stacked with #189, #191 and #114 on current main; typecheck, lint and format clean, 1372 tests pass, 0 fail.
Closes CopilotKit#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.
0866a4e to
9125618
Compare
What this changes
Closes #106.
A grant names
serverId/toolName, andlistForAgentreads it against the tool list, so a grant on atool the vendor has stopped advertising reaches no model. As #106 says, that is a property of the
transport rather than of the grant — and
google-driveis one line from proving it. The entry's owncomment:
Nothing said it was happening.
listServersaskedgrantsForabout the refs of the tools it had justlisted, which can only ever return a subset of those — so a grant on a withdrawn tool appeared
nowhere at all, on the one screen an administrator reads to answer what a Bot may do.
Reported, not pruned
You asked for this to be decided deliberately. Pruning is the wrong answer, and the reason is stronger
than "don't delete somebody's decision":
refreshToolsis the only place the prune could go, and it isnot a safe place to decide from.
The tool list is replaced by a bare
deleteand then aninsert, so a failure between them alreadyleaves a server with no tools. Worse, the bad case is not a failure at all — a vendor answering
successfully with an empty list deletes every tool row, sets
lastErrortonull, and stampstoolsRefreshedAt. So with pruning, one empty answer from Google silently revokes every Drive grant inthe deployment and the trail records a healthy refresh. That converts a visible, inert discrepancy into
an invisible, destructive one.
So, three parts:
listServersreports them. A newmcpGrantsForServersasks for every MCP grant belonging tothese servers, matched on the server half in the query (
split_part, not aLIKEprefix — a customserver's id is text a person chooses, and a
%in one would widen the match). Advertised refs staygrantedToon the tool; the rest becomewithdrawn.own section rather than rows in Tools, because these are not tools the vendor listed and folding
them in would make the count above wrong. Not drawn at all when empty, which is the normal case.
worry rather than only displaying it: the discrepancy enters the trail when it arises, so flipping
transportback tomcpis preceded by a record that those grants were sitting there, instead ofthe only record of the gap being its absence.
configuration.changedagainst the server, naming therefs and the Bots — nothing was denied and the refresh succeeded, so it did not seem worth minting an
event type. Say the word if you would rather it were its own so it is filterable.
An explicit "drop these" action for an administrator is now cheap, and deliberately left undone:
revoking should stay something somebody decided.
Where it runs
audit insert. Nothing held.
administrators refreshing at once leave two rows saying the same true thing, which is what the trail
is for.
Boundary and audit
grants ∩ advertised tools; this reports the difference rather than changing it. There is a testasserting exactly that, because "reporting a grant" turning into "honouring one" is the way this
change could go wrong.
configuration.changedwithchange: "grants_not_advertised". It isnot a refusal — nothing was denied — which is why it is not one.
Changelog
Added under
Unreleased: a connector says which of its granted tools it no longer offers, the grantsare untouched, and nothing changes for a connector whose grants all match its tool list.
Proof
Two tests in
server/tests/plugin-store.integration.test.ts. A tool is advertised, granted, thenwithdrawn the way a refresh withdraws one, and then:
toolsand present inwithdrawn, with the holding Bot named;listForAgentstill does not offer it — the assertion that fails if this had quietly become agrant;
withdrawnempty, because a field only ever exercisednon-empty is one whose empty shape nobody has checked, and a screen keys off it being empty.
What I have not run. These are integration tests and I still have no local PostgreSQL — Docker
Desktop is installed on this machine but its engine will not start and WSL2 integration is off, so CI
is where they first execute. I also have not driven the new section in a browser; it is an
Itemlistin the same shape as the Tools section directly above it, but I have not seen it draw. Say so if you
want either before it lands and I will get a stack up rather than have you find out from the screen.
What is not covered
advertises exactly four names,
create_fileandcopy_fileare inwriteTools, the swap is onefield — but I have no deployment holding those four grant rows, so I have not seen them.
refreshToolsis still not one transaction. I found that while arguing against pruning and haveleft it alone: it is a real gap, it is not this issue, and folding it in would bury the decision you
asked for under a rewrite of that function. Worth its own issue if you want one.
its grants (the switch on this page says so), so there is no stray to report.
different screen, and I did not invent one.