Answer from Google Drive, as the person asking - #97
Open
guidovizoso wants to merge 41 commits into
Open
Conversation
The knowledge lane needs answers that come back as the person asking. What was
in the tree did the opposite: `connectors.ts` configured Google Drive with a
service account and a domain-impersonation subject, and the worker synced
documents into a local pgvector index guarded by our own ACL rows. Every person
got the same answer, computed from what one credential could see, and revoking
somebody's access left a cached copy of their documents behind.
Fixing it is not a change to that code, it is the other design, so this takes it
out first rather than building the replacement beside it. Nothing imported any of
it outside its own tests, so nothing observable goes with it except two admin
screens that configured a sync that will not happen.
Gone: the in-memory knowledge repository and its ACL check, the knowledge agent,
the connector catalogue and admin service, the sync persistence and the worker's
connector runner, the `/api/admin/connectors` routes and the two admin screens in
front of them.
Kept on purpose, both recorded where a reader will meet them:
- `knowledge.yaml` is still parsed and still validated. It is part of the
deployment-package contract and shipped packages carry it, so a malformed one
should keep being refused. `knowledgeSources` is now read by nothing, and
says so, so the next reader does not take it for something live.
- Every table stays. Dropping `connector_instances`, `documents`, `chunks`,
`document_acls` and the rest is an irreversible migration with none of this
slice's purpose behind it, and the `chunks` embedding column may yet be
wanted. They are unused tables until somebody decides otherwise.
Two things worth knowing for the next change here. `createApp` lost a positional
parameter, so the placeholder runs in the agent and channel route tests each drop
one `undefined`; both tests assert through the store they pass, which is what
catches a slot landing in the wrong position, since consecutive `undefined`s
shift without a type error. And `routeTree.gen.ts` carries `@ts-nocheck`, so
`bun run typecheck` stayed green while the route tree still imported both deleted
screens — the build is what catches that, not the typechecker.
The worker is now a stub that reports idle. Left in place; removing a workspace
is a separate decision.
`needsCredential: boolean` said that a server needed a credential and never said
whose. That is the one thing about a connector worth being unambiguous about: a
reader who has to guess guesses the deployment's, and there was nothing in the
shape to guess against.
It is now `auth`, a discriminated union, and every entry states one:
- `none`, answers without a credential
- `deployment-bearer`, one token an administrator holds for everybody, which is
what all five existing vendors are
- `user-oauth`, the asker's own grant, where the deployment holds only an OAuth
client and each person consents for themselves
Google Drive is the first `user-oauth` entry: `drivemcp.googleapis.com/mcp/v1`,
which is the address Google publishes. It is also the first vendor here that
cannot be reached with a token somebody pastes, because Google issues no such
token — access is an authorization-code grant belonging to a person. That is the
property this connector is for rather than an obstacle to it.
The OAuth authorize, token and revoke addresses are pinned in the entry beside the
MCP host, under the same rule as the host: taken from the vendor's published
documentation, never from a caller. They are where this deployment sends somebody's
authorization code and receives the refresh token standing in for their access, so
they are a reviewed source contract too.
Scope is `drive.readonly` alone. Nothing in this slice writes, and a scope granted
by everybody who connects and used by nothing is a permission nobody remembers
agreeing to. `create_file` and `copy_file` are still listed as writes even though
that scope makes Google refuse them: the scope is what stops them, and the list is
what keeps a boundary written about writes covering them if the scope ever widens.
Drive is one host per Workspace product, so Gmail, Calendar, Chat and the rest are
each a further entry rather than a flag on this one. Adding Gmail stays a reviewed
decision about Gmail.
The Plugins page is sent the kind and not the endpoints. It needs to know what to
ask an administrator for; a URL this deployment sends an authorization code to is
not improved by also existing in every browser that opens that page. The token
field now keys off `deployment-bearer` rather than "needs a credential", so Drive
correctly stops asking for a token it could not use.
Nothing connects yet: adding Drive stores a server with no client behind it, and
calling its tools will refuse until the connect flow lands.
A tool that matched nothing produced an empty string, and an empty string is the worst thing to put in front of a model. It reads as "the tool had nothing to say" rather than "there is nothing there", and the model closes the gap from memory. For a connector whose whole job is answering from a live system, that is the failure mode: an answer with nothing behind it, delivered with the same confidence as one with a document under it. An empty result now says so in words, and says the part that matters — there is nothing here to answer from. Every vendor, not just Drive, because the hazard is in the shape of the answer rather than in who sent it. The shaping moved out of `callTool` into `resultText`, which is a decision rather than plumbing: it settles what a model is told when a vendor answers with nothing, with something enormous, or with a part we cannot render. Out on its own it can be asserted without a server to talk to, which is why it now has tests at all — this module had none, because everything in it needed a live vendor. Nothing is decided by trimming except whether the result is empty. A vendor that sent one newline has said nothing, and which shape of nothing arrived should not change what the model is told; but a blank part beside a real one is still a result and is passed through untouched. Behaviour otherwise unchanged, including the visible truncation and naming an unreadable part rather than dropping it. The exact-limit boundary now has a test, which the rewritten comparison would otherwise have been free to get wrong.
The table that makes a Bot answer as the asker rather than as the deployment. `mcp_servers.credential_id` holds what the DEPLOYMENT has. For a `user-oauth` vendor that is an OAuth client, which identifies us to Google and reaches nobody's documents by itself. What reaches somebody's documents is `mcp_user_credentials`, one row per person per server, and a call picks the row belonging to whoever asked. The key is the pair, not a surrogate id. "Which credential serves this server for this person" has to have exactly one answer: with an id and no unique constraint two rows for one pair are legal, and then the answer is whichever the query happened to order first — so somebody who reconnected could keep being served the grant they thought they had replaced. `credential_id` is a real foreign key, unlike `mcp_servers.credential_id`, which is `text` against a `uuid` primary key and so references nothing the database checks. The new table does not copy that. It also does not cascade: a revoked credential is kept for the trail, and deleting the row that says whose it was would take the trail with it. The person and the server both do cascade — a deleted user must not leave a row pointing at a secret held on their behalf, and a removed server must not leave rows nobody can reach to disconnect. `scope` stores what the vendor granted, not what we asked for. The two differ when somebody declines part of a consent screen, and a tool failing for want of a scope should be explainable rather than a mystery about a permission we assumed. Two new credential kinds, because three different things deserve three names. `mcp` is one token an administrator holds for everybody. `mcp_oauth_client` belongs to the deployment and is what you rotate when it leaks. `mcp_user_token` belongs to one person and reaches everything they can see. Filing all three under `mcp` would make "what does this deployment hold" unanswerable without reading every row's metadata, which is the question the vault exists to answer. Two things fell out of that. `CredentialKind` was a hand-written union duplicating the enum, with nothing keeping them in agreement; it is now derived from the enum. That widens the type, so the API's credential allowlist — `model`, `connector`, `mcp` — is now deliberately narrower than it, and says so: a user token exists only as the outcome of a consent somebody gave, and an administrator hand-posting one would be creating a credential attributed to a person who never agreed to it. Migration adds the enum values and the table. Nothing writes to it yet.
The selection that makes the whole knowledge lane mean anything: a call to a
`user-oauth` server goes out on the asker's own grant, and every branch that
cannot prove it has that grant refuses.
There is deliberately no fallback. A fallback is the one bug this design exists to
make impossible — answering out of whatever the deployment, or the last person to
connect, happened to be able to see. That failure is silent by construction: it
returns a confident answer assembled from documents the person asking cannot open,
and it looks exactly like a correct answer. So the refusals are what the new tests
are about, and each one asserts that nothing was decrypted rather than only that an
error was thrown.
Four ways it refuses, each with a sentence aimed at whoever can act on it:
- not connected: tells the person to connect in Settings
- actor unattributable: `identifyActor` answers `{ id: "" }` when it cannot say
who is asking, and an empty string must never match a row, so it is refused
before the query rather than trusted to miss
- credential revoked: tells them to connect again. The vault already refused a
revoked secret, but by throwing, which reaches a person as "that tool could not
be called" — indistinguishable from the vendor being down. A withdrawn grant is
not a fault
- no OAuth client registered: names the administrator's job, because the person
did their part and cannot fix this one
Nothing is cached. The refresh token is exchanged for an access token per call and
the access token is thrown away, so no stored copy of anybody's access exists for a
disconnect to have to find — revocation is complete by construction rather than by
cleanup. The cost is a round trip to the vendor's token endpoint on every call.
This also closes a hole that would have been ugly. `refreshTools` decrypted
`row.credentialId` and passed it as a bearer token. For a `user-oauth` server that
column holds the OAuth CLIENT, so listing tools would have sent the deployment's
client secret to the vendor as somebody's access token. Listing now goes through the
same selection as calling, so there is one answer to "what token does this server
get" and it cannot be a secret of the wrong kind. It takes the actor for that
reason: an administrator who has not connected gets a refusal recorded in
`lastError` and shown on the Plugins page, which is honest — until somebody
connects, this deployment does not know what that server offers.
The audit payload gains `reachedAs`. Two rows for the same tool and the same Bot can
legitimately have seen entirely different documents, and without it nothing in the
row says why.
`callVendor` and `exchangeRefreshToken` are injected, defaulting to the real ones.
Whose credential is chosen is this module's security property, and asserting it
otherwise needs a reachable vendor — which would leave the property most worth
testing as the one thing never tested.
Nothing writes `mcp_user_credentials` yet, so in practice every user-oauth call
refuses. The connect flow is next.
The half of the flow that leaves this deployment and comes back. An administrator registers the deployment's OAuth client once; each person then consents for themselves and their refresh token becomes the row the call path already looks for. The browser being in the middle is the whole difficulty. An authorization code arrives on a request that somebody else's server sent the person to, so nothing on it can be believed alone — not who is connecting, not which server they meant, not that they ever asked. Two things carry that across: a state this deployment signed, and a PKCE verifier proving the code being redeemed belongs to the request that started it. So `oauth.ts` is mostly refusal, and its tests are mostly refusal. A state that was tampered with, signed with another key, replayed after expiry, stripped of its signature, or malformed all read back as null — one answer, because a caller that has to tell those apart is a caller that can get one of them wrong. The reason it matters is specific: the alternative is attaching one person's Google account to another person's row. Signed under its own label, so a connect state can never be a run assertion wearing a different hat, and no other signed value this deployment hands out is a candidate. The callback is deliberately not behind `requireUser`. Whose connection this is comes from the state, never from whatever session the browser happens to be carrying — which is what stops a callback delivered to the wrong browser from writing to the wrong person. Every failure ends identically, back at Settings with nothing written: there is no useful distinction for the person between a forged state and an expired one, and naming which is which tells anybody probing the endpoint how far they got. Two request parameters are load bearing and easy to lose. Without `access_type=offline` Google returns no refresh token, so a connection would appear to work and stop about an hour later with nothing to renew it. Without `prompt=consent` a second connect returns no refresh token either, because the person already agreed once — which would turn reconnecting after a disconnect into a silent no-op. Both are asserted. So is the absence of a refresh token in the response being treated as failure rather than partial success, for the same reason: storing the access token instead produces a connection that looks like success. The redirect URI is built from configured `publicUrl` (`OPENBOT_PUBLIC_URL`, falling back to `BETTER_AUTH_URL`) rather than the incoming request. It has to match what was registered with the vendor character for character, and one assembled out of a Host header is one an attacker has a say in. A deployment without it refuses to start a flow and says so. The value is served to the Plugins page so an administrator can copy exactly what we will send. Registering a client and recording a connection both write the vault themselves rather than having the browser post `/api/admin/credentials` first, which is what the narrowed allowlist in the previous commit anticipated: the first of two calls can succeed and the second fail, leaving a secret nothing points at and nobody knows to revoke. Both replace-and-revoke rather than accumulate — a refresh token nothing points at is still a live grant at the vendor, and leaving it would give a person who reconnected two valid grants and only one of them visible to disconnect. Also found while here: `server/tsconfig.json` includes only `src`, so server tests are never typechecked. Widening the store's vault type broke two test files at the type level in total silence. The two are fixed and their stubs now throw rather than no-op, but the gap is real — including `tests` surfaces 34 pre-existing errors, which is its own piece of work and not this one. Nothing in the UI reaches any of this yet.
The shape of the feature made visible: an administrator registers the connector
once on the Plugins page, and every person connects their own account from
Preferences. Two screens because they are two different decisions by two different
people, and putting them together would suggest the administrator's grant is what a
Bot reads with.
On Plugins, a `user-oauth` vendor asks for a client id and secret rather than a
token, and says in as many words that nobody's documents are reachable with what is
typed there. Worth the sentence: "client secret" in a box on an admin page looks
exactly like the access token in the box above it and is a completely different
thing.
The redirect URI is shown beside those fields, served by the server rather than
assembled in the browser, selectable and monospaced. It has to be copied by hand
into somebody else's console and match character for character; getting it wrong
fails at the vendor with a message that never mentions OpenBot, which is an
afternoon lost to a missing slash. A deployment with no public URL says so there
instead of offering fields that cannot lead anywhere.
On Preferences, somebody sees the vendors an administrator has added, whether they
have connected, and when. Only their own — the endpoint answers for whoever is
asking, so no page can render somebody else's by mistake. Connecting is a full page
navigation, not a fetch: the consent screen is the vendor's own and has to be shown
to the person in their own browser. There is deliberately nothing here that could
complete it for them.
A failed callback comes back as `?connected=failed` and says nothing was saved,
which is true — the callback writes nothing on any failure path.
One thing worth knowing for the next route that does this. `validateSearch` returning
`{ connected: undefined }` rather than `{}` makes `search` a required prop on every
`Link to="/settings"` in the app: present-but-undefined is not the same as absent to
the router's types. The key is omitted instead.
Google Drive is now end to end — added, client registered, connected, and called as
the asker — subject to a real client and a real account, which is the part no test
here can stand in for.
`OPENBOT_PUBLIC_URL` was added with the connect flow and documented nowhere, which for a setting whose absence silently means "nobody can connect an account" is the wrong way round. Named in all three places this repository documents configuration: `.env.example`, `docs/configuration.md` and the table there. Says what it is for, that it defaults to `BETTER_AUTH_URL` so most deployments never set it, and what happens with neither — the Plugins page reports that a consent flow cannot be completed, rather than offering fields that lead nowhere.
The callback redirected to `/settings`, relative. It lands on the API, so that resolved against the API's own origin — which locally is port 3001, where no page is served. The consent would succeed, the grant would be stored correctly, and the person would arrive on a 404. Worth being precise about why this got through: nothing was wrong with the flow. Every test passed because every test was about the flow. The failure is entirely in the fact that the app and the API are two processes on two ports, which is a fact about running the thing rather than about any function in it. `OPENBOT_APP_URL` names where the app is, defaulting to the first `TRUSTED_ORIGINS` entry — already defined as where the app is served from — and then to the API's own public URL, which is right for a deployment serving both from one origin. Relative stays the answer in that last case, which is the only one where the setting can be absent and the deployment still work. `settingsUrlFor` so the three outcomes are one function with tests, rather than string concatenation at two call sites. Documented alongside `OPENBOT_PUBLIC_URL` in `.env.example` and `docs/configuration.md`, including the port numbers, since the reason the two settings are separate is not guessable from their names.
# Conflicts: # app/src/lib/connectors/queries.ts # app/src/routeTree.gen.ts # app/src/routes/_authed/admin/connectors.tsx # app/src/routes/_authed/admin/connectors/google-drive.tsx # app/src/routes/_authed/admin/plugins.tsx
# Conflicts: # server/tests/agent-routes.test.ts # server/tests/channel-routes.test.ts
# Conflicts: # .env.example # docs/configuration.md # server/drizzle/meta/0002_snapshot.json # server/drizzle/meta/_journal.json # server/src/app.ts # server/src/config.ts
Plugins was three tabs: a catalogue of what could be added, a second tab for what had been, and skills. Answering one question about one vendor — is Drive available, and what can it do — meant visiting two of them, and the third was a different kind of thing altogether. Now two lists. Connected says what is added and where it stands; Explore plugins says what else there is. Each row goes to that vendor's own page, because what a connector needs configured is not the same from one vendor to the next: a token for one, an OAuth client and a redirect URI for another, an instance hostname for a third, and then a grant per tool per Bot. The old screen tried to hold all of that in a list and grew a column per Bot, which is how a grant goes unread. The detail page switches on the `auth` discriminator the knowledge lane added, which is the first thing to make real use of it. A bearer vendor gets a token row; a user-oauth vendor gets the client, the redirect URI to copy, and a read-only row for your own connection that points at Preferences — because connecting is yours and not an administrator's act. ServiceNow gets both, being per-instance as well. Skills moved to `/admin/skills`. A skill is not a connector: it adds no capability at all, only asks a Bot to use what it already holds. Sitting in a list of vendors made it look like a third thing a Bot could reach. Grants moved into `ItemFooter`, where the layout skill puts a set. In `ItemActions` a chip per Bot fights the tool name for horizontal space, which is what started the sideways scrolling. The add-by-URL flow is gone from the screen. `POST /api/plugins/servers/custom` and `addCustomServer` stay: they are governed and tested, and removing an endpoint is a different change from restyling a screen. A custom server already in the database still lists under Connected, marked by its provenance. Two repairs while in here. `addServerWithToken` awaited nothing, so a failed OAuth client registration went to an unhandled rejection instead of the error banner; it awaits now and takes an `after` step. And the layout skill pointed twice at `admin/connectors.tsx` as its reference screen, which Slice 0 deleted — repointed at the two new files. The sidebar's "seven things" comment was also stale before this change and now says ten. No tests: this is UI, and the repo's frontend has almost none. Driven by hand instead — both lists, all three auth kinds, and add/refresh/remove end to end. The error path is real: adding Atlassian with no token lists no tools and shows the vendor's own refusal on the row, which is what that state should look like.
A bare glyph put a 15px icon straight against the row's text, so a list of six had no fixed left edge for the eye to run down and read as six paragraphs rather than a list. Each icon now sits in a 36px rounded square with a muted fill and a hairline border, which gives every row the same anchor whatever its icon. `RowMark` rather than a class at every call site, because the whole value is that the tiles are identical: eleven copies of `size-9 rounded-lg border bg-muted/60` is eleven chances for one to drift, and one tile a pixel out looks broken rather than varied. A deliberate deviation from the default row anatomy, which is `ItemMedia variant="icon"` and nothing else. Recorded in the component's own comment, because the layout skill asks for a reason when a screen departs from it — and because `components/ui/item.tsx` is a shadcn file with its own upstream, so a new variant does not belong there.
Two changes. The tiles lose their border and keep the muted fill, so a row's icon reads as one soft shape rather than a boxed-in glyph. The catalogue is Google Drive alone. Atlassian, Box, Slack, Salesforce and ServiceNow are gone: each was a reviewed source contract for a vendor nobody had connected, and a screen offering five untried connectors asserts more than this deployment can stand behind. They are in the history, and re-adding one is a review of that vendor rather than a revert. This is a security-relevant edit, not a cosmetic one — the catalogue is what makes a host admissible, so five hosts this deployment would have talked to it no longer will. Recorded in the file, along with why `deployment-bearer` stays in the union with no entry using it: a server added by URL has no catalogue entry, and that is the branch it falls into. WHAT THE TESTS LOST. ServiceNow was the only per-instance entry, and removing it took the anchored-pattern assertions with it — that a prefix, a suffix and a subdomain are each refused. `PATTERNS` is compiled from the catalogue by key, so a synthetic entry cannot reach a pattern and there is no way left to exercise the matching through the public API. The fail-closed half survives and is asserted; the test says out loud what it no longer covers, so whoever adds the next per-instance vendor restores the rest with it. The rest of the churn is fixtures moving to Drive: host admissibility, the frozen path, effect classification. Two assertions in the store suite needed more than a rename. Both inferred that the policy had not refused a call from the call failing at the network, which Drive cannot do — it is reached as the person asking, so it is refused earlier for want of a connection. They now assert `rule` is null, which is the property they were reaching for and states it directly rather than inferring it from an unreachable vendor. Lint warnings 26 to 20: the removed vendors carried the non-null assertions.
Two changes to the plugin detail screen. Enabling is now one switch instead of an "Add to deployment" button at the top of the page and a destructive "Remove" row at the bottom. Those were the same decision drawn twice, in two places, one of them looking far more dangerous than the other. A Switch is what the layout skill reserves for exactly this: binary, immediate, no save. The description states the consequence in the present tense in both directions, because switching it off deletes every grant on the vendor's tools and switching it back on does not bring them back. Everything below it is now gated on being enabled. A vendor that is off shows one row, which is all there is to decide about it; the client, the redirect URI and the tools appear once it is on. That also fixes an ordering trap in the old flow — an OAuth client is recorded against the server row, so it could not be registered before the row existed, and the page previously offered both at once. The tile is now for one list only: the connectors on `admin/plugins`. Every row there is another company and the tile carries that vendor's own mark, which is work no other row in the app needs. A detail page's rows are this deployment's own settings and a skill is an instruction we wrote — no third party to identify, so both go back to the standard `ItemMedia variant="icon"` and stay consistent with the other screens. Recorded in `RowMark` so the next reader knows the narrowness is deliberate rather than incomplete. The switch row itself takes no icon at all. It is this deployment's own control, not a thing to tell apart from other things. Not tested beyond typecheck, lint, build and a look at the screen: the frontend has no tests. The toggle's off path in particular has not been exercised — the vendor on this deployment was enabled by hand outside this session, and I left it alone rather than switching it off to watch what happens.
The layout asked for: an unheaded row for the switch, then Connection holding only what has an action, with the redirect URI as prose under the card. No icons anywhere on this screen. These rows are one deployment's own settings, and an icon per row earns its place only where it tells things apart — which is the connector list, where every row is another company. Here it was decoration with a column of its own. No heading over the switch. One decision, and a heading that repeats the row's own title tells a reader nothing the row does not. The redirect URI is prose, not a row. It has nothing to click, and the layout skill's read-only row kind works on a screen full of them but reads as a broken control when it sits among actionable ones. As text under the card it is what it actually is: an instruction, with the value to copy beneath it. "Your account" is gone. Connecting is the person's own act and lives in Preferences — there is no endpoint for an administrator to do it for somebody, so a row about it on an admin screen was reporting on something this screen has no part in. Its query went with it. Not tested beyond typecheck, lint, build and looking at it: the frontend has no tests. The off position of the switch is still unexercised — Drive is enabled on this deployment with a real client registered, and switching it off would delete that.
It was the page's own action, on the title's baseline, which is where the layout skill puts the one primary verb of a screen. Refreshing is not that: it asks the vendor what it offers now, which is about the tools list and nothing else on the page. Beside the heading it names what it acts on. Ghost rather than outline, because it is maintenance. The thing an administrator came to this page to do is enable the connector or grant a tool; re-asking the vendor is housekeeping and should not compete with either. The page header now carries no action at all, which is correct — enabling moved to a switch, and there is nothing else this screen does once.
Connecting was a section at the bottom of Preferences. It is now `/settings/connected-accounts`, with a page per service — because what a connector needs from a person is not fixed. Drive needs one consent and nothing else; a vendor that scopes access per workspace, or per folder, or asks which of several accounts to use, needs somewhere to ask. This is that somewhere, before there is anything to put in it. The list shows only what a person can actually decide: vendors reached as the person asking, and only ones an administrator has enabled. A shared-token vendor answers the same for everybody, so listing it would offer a choice nobody has; a vendor nobody has enabled has no OAuth client to consent against. The empty state says whose move it is — "nothing here" alone reads as though you failed to do something, when what is missing is an administrator enabling a connector. THE OFF POSITION IS NOT BUILT, AND SAYS SO. Withdrawing is three acts — revoke at the vendor, revoke the vault credential, delete the row — and none exist yet; that is K2. Switching off therefore refuses and names the workaround (revoke it in Google's own third-party access settings, which stops the reading immediately). A switch that moved and changed nothing would be worse than one that declines: it would report access had been withdrawn when it had not. Flagged when this was chosen and repeated here, because a stub that lies is the failure mode. Also moved the OAuth callback. It sent people back to `/settings`, which no longer mentions what they just did — landing somebody on a page with no trace of their own action is a silent failure of exactly the kind this flow keeps producing. It now returns to `/settings/connected-accounts`, and the `?connected=` notice moved with it. Only the failure gets a sentence; a success is already told by the row saying "Connected". `RowMark` now covers both connector lists rather than one, since the reason for it — these rows are other companies, and the tile carries the vendor's mark — applies equally here. Its comment says so instead of contradicting the code. Verified by typecheck, lint, build and clicking both screens. NOT verified: the connect flow itself, which I drove up to but not through, because completing it would attach a real Google grant to somebody's account.
A person registered a Google Drive client on the Plugins page, and connecting then said "Google Drive has no OAuth client registered yet". The admin page said "Registered". Both were reporting honestly: `mcp_servers.credential_id` held `b3250247-…` and no such row existed in `credentials`, so `oauthClientFor` read a pointer to nothing and returned null. This suite did it. `registerClient` repoints `mcp_servers.credential_id` — for `google-drive`, a real catalogue key — and the cleanup then deletes the credential it pointed at. Two other tests in the file set the same column to a bogus string and to null. On a throwaway database that is all harmless; on one somebody is using, that column is live configuration and the suite was overwriting an administrator's registration and then deleting what replaced it. Nothing caught it because nothing can: `mcp_servers.credential_id` is `text` against a `uuid` primary key, so there is no foreign key and the database will happily hold a pointer to a row that does not exist. The failure surfaced as a sentence blaming an administrator for not doing something they had done. The suite now snapshots the pointer in `beforeAll` and restores it in `afterAll`, before the deletes rather than after, since the deletes remove the row it is pointing at. Verified by running the whole suite and checking the column is as it was. The file already said server rows are "deployment configuration, so it belongs to the deployment rather than here", and then repointed its credential anyway. The comment was right and the code was not.
A switch was the wrong control twice over. Connecting is not a position, it is a departure — the next thing on screen is the vendor's own consent page — so it is now a button with an arrow that says so. And being connected is a fact about a grant living at the vendor, not a slider's other end, so it reads as a state with a menu hanging off it: a green dot, "Connected", and one named item. Naming the destructive act is the point. "Disconnect your Google Drive account" is a sentence somebody can decide about. Sliding a switch left is not, and it is far too easy to do by accident to something that cannot be undone by sliding it back. The menu needed `w-auto`. `DropdownMenuContent` defaults to `w-(--anchor-width)`, which is the width of its trigger — here a small "Connected" button — so the one item inside wrapped onto three lines, at exactly the moment a destructive action most needs to be legible. Disconnecting still is not built. The item says so and names the workaround rather than closing the menu and changing nothing; that remains the one outcome worse than not offering it. Flagged when the stub was chosen and true again now that the menu exists mainly to hold it. Verified both states in the browser, including the menu open, by inserting a connection row and then removing it. Counted the tables before and after and ran the full suite afterwards: one credential, the real registered client, still pointed at.
Two things, one of which was not a code problem at all. The redirect already pointed at `/settings/connected-accounts` and had for an hour; the running server predated the change. Restarted. Worth recording because the symptom — "it still goes to /settings" — is indistinguishable from the code being wrong, and I went looking at the code first. The destination is now better than the list. Success returns to the account page, which is where the flow started and the only page that can say something new: it reads Connected, with the scope the vendor granted beside it. No query parameter with it, because "it worked" is already told by the thing it is news about. Failure still goes to the list, which is the one screen that draws the notice — and the only honest destination when the state could not be read, since without a state there is no server id to return to and choosing one would be a guess about what somebody had been doing. The server id is escaped into the path. It comes off a signed state so it is ours, but a value reaching a URL unescaped is one traversal away from meaning something else, and there is a test for it. Also: a dot beside Connected on the list rows. Two states differing only by the word "not" ask somebody to read carefully to tell them apart, which is the wrong amount of effort for the only fact a row carries. Same green as the account page's control, so the list and the page agree at a glance. Decorative, since the text beside it already says which. One import removed that a hand edit to this file had orphaned. The edit itself — `mt-0` on PageRows, and the vendor documentation row taken out — is left alone.
…al rows THE ERROR THAT PROMPTED THIS. Refreshing Drive's tools produced "Streamable HTTP error: Error POSTing to endpoint:" followed by a complete, valid, successful-looking tool list. It reads as a parsing bug here. It is not. Verified against the live endpoint: Google's Drive MCP answers `tools/list` with **401 and the full tool list in the body** when the bearer token is not accepted. No auth header at all returns 200 and the same list. So the body says nothing about the failure and the status says everything — and this code threw the status away, keeping only `error.message`, which is where the transport puts the body. `vendorFailure` now leads with the status and names 401 and 403, because those are the two an operator can act on: a rejected credential means reconnect, a refusal after acceptance means access or an API that is not enabled. The body is dropped. By the time a vendor is refusing a credential its payload is not what anybody needs. THE SECOND HALF OF A FIX I ONLY HALF MADE. The earlier commit restored `mcp_servers.credential_id` and stopped there. Both integration suites still did `delete(mcpUserCredentials) where serverId = 'google-drive'` — every connection for that vendor, not the suite's — and on this machine that deleted a real person's Drive connection minutes after their consent had succeeded. The refresh token survived in the vault, orphaned, so the screen said "Not connected" while the grant still existed at Google. Both deletes are now keyed on the suite's own user ids, which carry a per-run random suffix. And both suites advertise `search_files` on a real catalogue key while only cleaning up when they created the server row, so the fixture kept reappearing on a real Plugins page as a tool the vendor appeared to offer. Both now remember whether the row was there first: the vendor genuinely advertises that name, so deleting by name regardless would take a real one. Proved rather than asserted: ran the full suite and counted afterwards — zero leftover tools, the connection intact, the client intact. Worth flagging separately: `mcp.call_succeeded` is written before the call is attempted, so it records "the policy allowed this", not that anything succeeded. The name is wrong and it misled me while reading this trail. Not changed here — it is an audit event type and renaming it is its own decision.
A 403 from a Workspace MCP server carries the answer in its body: which API is
not enabled, and the console URL to enable it. We were dropping the body and
printing a guess between two very different problems, which cost three rounds
of diagnosis on a connector that had been correct since the first attempt.
reasonFrom() parses both shapes Google refuses in — the REST
{error:{message}} and the MCP {result:{content,isError}} — and trims to 400
characters, because the same position may instead hold a full tool list. That
is not hypothetical: these servers answer tools/list with a complete, valid
list under a 401 or a 403, which is what made the original error read as a
parsing bug here rather than a refusal there.
docs/plugins/google-drive.md is the rest of that diagnosis, written down. The
part worth a document is that each Workspace product is two APIs — enabling
drive.googleapis.com does not enable drivemcp.googleapis.com — so "I enabled
it" and "it is enabled" are different claims and only the vendor's sentence
tells them apart. Also: which half is the administrator's and which is each
person's, that the redirect URI is the API's port and not the app's, and that
disconnecting is not built, with the vendor-side revocation to use until it is.
The catalogue line in architecture.md named five vendors we no longer ship.
Two faults, found while diagnosing a Bot that had no Drive access on a deployment where every screen said it was connected. FIRST: the tests deleted an administrator's grant. plugin_grants has the primary key (kind, ref, agent_id), and two suites cleaned up with delete().where(eq(ref, ref)) — two of the three columns, so the delete matched every Bot in the deployment rather than the suite's own. Harmless while ref pointed at a fixture server; I repointed these suites at google-drive and search_files, which are real, and the next run took the live grant with it. The Bot silently lost the one tool a question about a document needs, with an audit row showing the grant being made and nothing showing it removed. Both are now scoped by agent id. SECOND: mcp.call_succeeded was written before the credential was selected and before the network call, so it recorded a decision and was named after an outcome. Every way a per-person connector actually fails is downstream of that line — no connection for the asker, a refresh token the vendor stopped accepting, an API not enabled for the project — so each of them left a row asserting the call had succeeded, and no row saying otherwise. That is worse than a gap. A trail with a gap sends somebody to look; a trail that is confidently wrong gets used to rule the connector out. It did: the Admin page showed successful calls for a Bot that could not read anything. The row is now written after the attempt, and mcp.call_failed carries the vendor's own sentence — which for a 403 is the one naming the API to enable. The audit page already had a label for that event type and nothing had ever emitted it. A policy refusal is still written before the throw, since this deployment declining is the whole event and there is no attempt to wait for. An isError result counts as failed: a vendor correctly reporting that the tool failed has not completed the call.
Now that a row is written after the attempt rather than before it, the event type is the fastest answer to whose problem a failure is — and no rows at all is its own answer: the tool was never granted to that Bot, which is the third step people miss after enabling the connector and connecting an account.
…eview gate An isError result was recorded as "the tool reported an error" when the vendor had said something specific. Google refuses the Drive MCP server with "The caller does not have permission", and losing that cost a round of probing to recover something already in hand. Only on the failure branch, which is the point of the distinction: a successful result is somebody's file listing and has no business in a row an administrator reads, while an isError result is a message written for whoever operates the deployment. The docs get the diagnosis. "The caller does not have permission" is PERMISSION_DENIED about the PROJECT, not the credential, which is what makes it expensive: minting a token from the stored refresh token and asking Google's own tokeninfo endpoint returns 200 with the right aud, azp and scope, so every check available locally says the credential is correct — and it is. The gate is the Workspace Developer Preview Program, which is checked against the project and mentions neither itself nor enrolment in the refusal. Also recorded, not acted on: Google's guide lists Drive as needing drive.file alongside drive.readonly. Not added. drive.file is write-capable, and the read-only guarantee here is currently the scope rather than only the tool classification, so widening it is a decision about what this deployment may do to somebody's Drive rather than a fix to apply pre-emptively. Enrolment is checked first because a missing scope normally reads as "insufficient authentication scopes" instead.
Google's Drive MCP server is gated behind the Workspace Developer Preview Program and refuses an unenrolled project with "The caller does not have permission" — a statement about the project rather than the credential, so every local check reports a correct setup, correctly. Enrolment is a Workspace application with a stated turnaround of days and no certainty at the end. The REST API underneath has been GA since 2015. This adds an adapter for it and points the Drive entry there. Verified against a real account: refreshTools lists four tools, and both listing and search return files. WHAT MAKES IT REVERSIBLE. The adapter implements the interface ./mcp already exported — listTools and callTool, same shapes — rather than inventing one for itself. That direction is the design: MCP is the contract and the adapter conforms to it, so MCP has not become a special case of a shape invented for Drive. transport.ts resolves one per catalogue entry, and there are exactly two call sites in the system. Nothing else — not the OAuth flow, the per-person credential selection, the grants, the policy engine or the audit trail — knows which protocol is underneath. Going back is `transport: "mcp"` plus the host and path. Tool names are Google's MCP names character for character, so every grant an administrator has already made survives the swap in either direction. Diverging would have made switching transports mean re-granting every tool on every Bot. Also fixes a hazard this exposed. mcp_servers.url is written once, by copying the catalogue at add time, which makes it a cache nothing invalidates: changing the host left this deployment still calling the preview endpoint, and no screen could show why — the row looks as intentional as the day it was written. For an entry with a pinned host the catalogue now wins, because it is the reviewed source contract and a host it no longer names is one this deployment has decided not to talk to. The stored value still wins where it is the only truth: a custom server added by URL, and a per-instance vendor whose hostname is the customer's. The catalogue tests now assert the preview host is INADMISSIBLE, since moving the entry is also a decision to stop talking to the old address.
The connector works. The Bot called list_recent_files, was handed nine real files, and answered "I don't have the necessary permissions" — because its system prompt ended "When none is connected, say so plainly", and a flat instruction to deny outranks evidence the prompt never mentions. That instruction was honest when it was written (#58): the pgvector connector had been removed and nothing had replaced it, so there genuinely was no source and admitting it beat inventing a citation. With a Drive connector granted it inverted — the prompt now produced exactly the false statement it was added to prevent, in the other direction. The condition is now something the Bot can observe. A tool that returns files is a connected source and cites itself; no tool, or a tool reporting a problem, is the case for saying so. The anti-fabrication rule is kept and pointed at the gap it was actually for — never answering from memory as though it came from a source — with the symmetric rule beside it: never claim to lack access to something a tool has just returned. Worth noting for the trail: nothing in the plumbing was wrong here. The store, the transport, the credential selection and the runtime bridge all did their jobs, and the audit row said call_succeeded because it had. The whole failure was one sentence of English.
Setting Drive up meant four stops: enable it, register the client, get refused at "refresh tools", go to your own settings page and connect a personal Google account, come back, refresh. Two changes remove that. THE GATE WAS DOING NO WORK. refreshTools asked connectionTokenFor unconditionally, which for a user-oauth server refuses unless the person pressing the button has connected their own account. That is right when listing means asking a remote server, which will not answer unauthenticated. The Drive adapter's tool list is this file's own code and listTools ignores the connection entirely — so an administrator was sent away to mint a token that was passed to a function that discards it. The transport now declares whether listing needs a credential; MCP says yes, the adapter says no. Verified: refreshTools with the anonymous actor and no connection returns four tools. VERIFYING IS NOT SETUP, BUT IT NEEDS A HOME. "Is this configured" and "does it work" are different questions and the second had no answer on this page. A "Your account" row answers it in place, below the client and only once one exists, since a Connect button with no OAuth client behind it can only fail. It says what it is: a personal grant reaching that administrator's documents only, not deployment state, and not a required step. And the callback had to stop overriding where somebody came from, or the inline row would bounce them out anyway. ConnectState carries returnTo — a closed set of two NAMES, never a URL, narrowed on the way in and again on the way out. Carrying a destination through an OAuth flow is exactly how an open redirect gets built, with a real consent screen in front of it; a name cannot express another origin, so the worst a tampered state achieves is the wrong page of this app. Tested against https://evil.test, //evil.test, a traversal and a case variant. A failure still lands on the settings list: the admin route needs the server key in its path, and a state that could not be read has no key.
The tools list drew a chip for every Bot inside every tool row. At three Bots and eight tools that is twenty-four controls stacked in a list, wrapping onto second and third lines, with the tool's own name losing the fight for attention against the buttons underneath it. The thing being decided — does THIS Bot get THIS tool — was the least legible part of the row, and it got worse with every Bot added, without bound. The list row is now a link that says what a reader scanning it is asking: how widely the tool is exposed, whether it reads or changes things, and nothing else. The count is words rather than a fraction, because "0/3" reads as a score and needs decoding; the two ends worth recognising without reading are named, and only the middle gets a number. The tool screen is where the decision lives, one Bot per row with one switch, which is the same governance with nothing competing for it. Each switch is disabled only while its own write is in flight, so switching one Bot does not freeze the list. The descriptions say which side of the boundary a Bot is on: granted still means every call is checked and audited, and not granted means the tool is never offered to the model, so there is nothing for it to refuse. `$key_` opts the route out of nesting, so the connector page stays a page rather than becoming a layout with an outlet. Verified in the generated tree: the path is /admin/plugins/$key/tools/$tool with the admin layout as its parent. A missing tool distinguishes withdrawn from mistyped, because a vendor dropping a tool between refreshes reads very differently from a bad address.
Three conflicts, and two of them were the same hazard as last time. createPluginRoutes grew a parameter on both sides: main added a required canUseBot (#37), this branch an optional connect. Both are kept, required first, because that is the only order that compiles — and the comment now says why, since every argument after the first optional one typechecks in the wrong position and silently does nothing. That has bitten this merge four times. `POST /call` is the interesting one. This branch had deleted it: nothing in the repository calls it, and it passed actorEmail where callTool needs users.id, so it could not work for a per-person connector. Main had just hardened it with canUseBot. Deleting another engineer's security work as a side effect of a merge is not a thing to do quietly, so the route is kept and only the actor is fixed. Removing it can be its own change, which says so and can be argued with. The migration collided the same way as before: both branches wrote 0005. Regenerated as 0008 on top of main's chain, byte-identical in content — which turned out to mean drizzle already had its hash, since it hashes content and not filenames. The local database needed its ledger re-timestamped rather than recreated; recreating it destroyed a real OAuth client and connection once already, and no deployment will ever be in the state this one was. 1052 pass, typecheck, lint and build green.
The changelog's own rule is that a line belongs there when a deployment behaves differently afterwards, and this branch adds a connector, two screens and an audit event type while removing a connector that was syncing documents. The removal is the part worth spelling out, because the new Drive connector is not the old one renamed and an operator could reasonably assume it was. The old one answered as the deployment from a cached index; this one answers as the person asking and caches nothing. A deployment that was syncing stops syncing, and somebody has to enable the new one and connect their account.
guidovizoso
requested review from
MikeRyanDev,
davidmckayv and
tylerslaton
as code owners
August 21, 2026 19:30
davidmckayv
reviewed
Aug 21, 2026
davidmckayv
left a comment
davidmckayv
left a comment
Contributor
There was a problem hiding this comment.
Reviewed, and driven against a running deployment rather than read. The design holds up: the seam
between "which protocol" and "whose credential" is in the right place, and the no-fallback rule is
the part that makes this worth having. One finding I would want addressed before merge, and it is a
sharper version of a gap you already flag.
The central claim, verified
I granted search_files to a Bot with nobody connected and called it:
403 You have not connected your Google Drive account. Connect it in Settings and ask again.
That is the whole product argument working. The trail then said mcp.call_failed with
reachedAs naming the asker, not call_succeeded, so moving the audit write after the attempt is
doing real work here: a per-person connector fails at exactly the two lines you moved it past, and
before this the row asserted the opposite. Anonymous is refused at the route with 401, and the
empty-string guard in connectionTokenFor is a correct second line for it, since ANONYMOUS_ACTOR
really is { id: "" }.
The actor fix is right and I checked it rather than took it: the runtime path already passes
resolveRequestActor(request).id, so /api/plugins/call passing an email was the odd one out. This
makes the two agree rather than changing what actor.id means in a CEL rule, which is what I was
worried about.
Not caching the access token is the right trade. It costs a round trip per call and buys revocation
being complete by construction, which also means nothing here is per-process, so it behaves the same
on any replica.
The finding: offboarding leaves a live grant
Disconnect being unbuilt is in your known gaps, framed as a self-service gap. The administrator side
is worse and is not mentioned. I set up a person with a connected account and ran both offboarding
paths against the real database:
- Remove access from the People screen (the intended control): the
mcp_user_credentialsrow
survives and the refresh token incredentialsis not revoked. Their Google grant is entirely
intact. - Delete the user row: the join row cascades away, and the credential row survives, unrevoked and
now orphaned. Nothing references it, no screen can reach it, and no code path can ever revoke it.
They cannot exercise it while revoked, because the actor comes from a session they no longer get. But
"we removed their access" is not true of the thing that matters, which is the token sitting at Google.
For a feature whose entire premise is per-person access, the offboarding story is the one a customer
asks about first.
This is the same shape as #53 and as the key-rotation gap I left in #90, so it is a pattern rather
than a one-off: we create credentials in the vault and have no owner-lifecycle hook that retires
them. Minimum I would want here is that peopleStore.revoke and the user delete both revoke the
person's connector credentials, even without vendor-side revocation, so the vault stops holding a
usable secret for somebody who has been removed. Vendor revocation can follow with disconnect.
Smaller things
connectionTokenForreturnscredentialOwnerand every caller destructures onlytoken. The
audit getsreachedAscomputed separately, which is fine and better documented, so the return
field is now dead. Drop it or use it.mcp_servers.credential_idastextagainst auuidprimary key with no foreign key: you flag it
and I agree it should be a migration. It is the same class of bug as the dangling pointer you hit,
and the join table you added shows the right shape.- The four inert grants are correctly left rather than pruned silently, but they are only inert
because the adapter does not advertise them. Worth an issue so the swap back to MCP does not
quietly re-enablecreate_fileon a read-only connector. app/src/components/ui/item.tsxgainstext-pretty. That is a shadcn primitive with its own
upstream and the layout skill says not to edit them; a one-class change is defensible but it will
be lost on the next sync.- Updating
openbot-screen-layout/SKILL.mdbecause the screen it pointed at moved is exactly right
and I would not want that dropped in a rebase.
Not blocking, but worth saying
Two of K2's own asks are outstanding by your own account: disconnect, and the two-real-accounts
proof. The store-level isolation test with two users is good, but the claim K2 makes is that two
people with deliberately different Drive access get different answers, and only two real accounts
show that. I would not hold the PR for it, but I would not call K2 done either.
Screens are good. The admin page states the redirect URI verbatim with a warning that a single wrong
character fails at the vendor with a message that never mentions OpenBot, which is precisely the
detail that costs somebody an afternoon.
Review found the offboarding half of the disconnect gap, and it is the worse
half. Removing somebody from the People screen ended their sessions and added
them to the deny list, and left the refresh token they had granted this
deployment sitting in the vault, unrevoked. They could not exercise it, because
the actor comes from a session they no longer get — but "we removed their
access" was not true of the thing that matters, and for a per-person connector
that is the first question a customer asks.
Deleting the user row was worse: mcp_user_credentials.user_id cascades, so the
join row went and the credential survived, unrevoked, referenced by nothing,
reachable from no screen and by no code path.
peopleStore.revoke now retires them, through a seam rather than an import: it has
no business knowing what a connector is, and the list of things a person owns
will grow. Retirement looks the owner up in the VAULT by key_id rather than
through the join table, which is what makes the orphan reachable at all — both
cases are tested, including that the cascade shape is handled.
Ordered after the removal transaction on purpose. Stopping them getting in must
not be undoable by a later failure in the vault; if retirement throws, the
removal has stuck and the trail shows one without the other, which is the honest
record and is recoverable by removing them again.
Not vendor-side revocation. That needs the OAuth client and the revoke endpoint
and belongs with disconnect, so mcp.account_disconnected carries
vendorRevoked: false rather than implying otherwise.
Also from review:
- mcp_servers.credential_id is now uuid with a real foreign key, `restrict`.
The migration clears a pointer to a credential that does not exist BEFORE
adding the key, because a generator cannot know that — and without it the
deployment that most needs this migration is the one that cannot run it.
This immediately caught a test writing "a-deployment-credential" into that
column, which means it was asserting against a state no deployment could
reach.
- connectionTokenFor's dead `credentialOwner` return is gone, and the audit's
duplicate computation of the same fact is now one named function. Two
expressions for "whose credential" can disagree, and the only place that
would show is a row claiming a call ran as somebody it did not.
- The `text-pretty` class is out of the shadcn `item` primitive and into our
own stylesheet, keyed on the data-slot it already emits. The primitive has an
upstream, so a class inside it is one the next sync silently removes.
Contributor
Author
|
Addressed. Pushed as The blocking findingYou were right that the administrator side is worse, and right that it is a different finding from the self-service gap I flagged. Two details worth surfacing because they changed the shape of the fix: It looks the owner up in the vault, not through the join table. Your second path is the reason — It runs after the removal transaction, not inside it. Retiring is a vault write plus an audit row, and holding the removal open until that finishes would let an unrelated failure undo the deny-list row and the session deletion. If retirement throws, the removal has stuck and the trail shows one without the other — the honest record, and recoverable by removing them again. It is a seam rather than an import: that module has no business knowing what a connector is, and by your own account the list of things a person owns is going to grow. Smaller things
On K2Agreed, and I have stopped calling it done. The PR description says architecture-without-proof; disconnect and the two-real-accounts pass are the outstanding asks. One correction to something I would otherwise have let stand: while writing the retirement tests I checked whether reconnecting leaks credentials, since the fixture appeared to accumulate them. It does not — Gate: 1057 pass, 0 fail, typecheck, lint and build green, and the live connection and OAuth client survived the migration. |
CI caught this and a local run could not. Every test in the suite passed and the cleanup failed: `delete from credentials` was refused by the foreign key added in the previous commit, because `mcp_servers.credential_id` was still pointing at a credential the suite had created. The restore was guarded by `serverWasAlreadyConfigured`. On a database that already had the server — mine — the guard passed and the pointer was put back. On a fresh one it skipped, and nothing repointed the column before the delete. The guard was hiding a leak rather than avoiding one. Before the foreign key existed that delete SUCCEEDED and left the column addressing a row that no longer existed — the same dangling pointer that made a configured connector report having no OAuth client, and the reason the key is there. So CI failing here is the key working on the first run that could show it. Restoring unconditionally is right either way: `clientBefore` is null when there was no server to borrow from, which is what the column should then say. Verified against a scratch database migrated from empty, which is the condition that failed: 1057 pass, and the suite leaves no credentials, servers or users behind.
…e changelog The entry written before review covered the connector and the screens and stopped there, so the two things a reader most needs from the last commit were missing. Retirement on removal is the important one. It is a change in what an existing control does — "remove access" now also retires the credentials that person granted — and it is the sentence an operator asked about offboarding needs to be able to read. It also says what it does NOT do: the grant at the vendor outlives it until disconnect ships, and claiming otherwise would be worse than saying nothing. The foreign key gets an Upgrading note because it can require an administrator to act. A deployment holding a pointer to a deleted credential loses that pointer, which is the honest outcome — the connector reports having no credential instead of looking configured and failing at the vendor — but somebody has to register it again, and finding that out from a screen rather than from here is the wrong way round.
The entry described the shape of the feature and skipped the part that costs somebody an afternoon. "Registers one OAuth client" reads like a field to fill in; it means creating a client in Google Cloud with a redirect URI that has to match character for character, and a mismatch fails at Google with a message that never mentions OpenBot. Now said, and pointed at the doc, which the neighbouring entries already do for releasing and deployment. Leads with what it is for rather than which screens it added, because somebody reading a changelog is deciding whether to upgrade, not learning the navigation. Disconnect is named here too. It is in the PR and in the docs, and a reader who finds out from neither is the one who has already promised somebody a button. Also adds the shipped Knowledge Bot's instructions, which changed in this branch and are deployment-visible for anyone running the shipped tenant package: its old instructions told it to deny having a source, which was honest when it had none and became the opposite once a connector was granted.
guidovizoso
enabled auto-merge (squash)
August 21, 2026 20:33
This was referenced Aug 21, 2026
Two conflicts, both from main moving underneath it. `app.ts` gained an import of `ConnectorAdminService` on main from the module this branch deletes, so the import goes with it; `createIntelligenceClient` beside it is still used and stays. The changelog had both sides' entries and wanted both.
This was referenced Aug 21, 2026
davidmckayv
added a commit
that referenced
this pull request
Aug 21, 2026
Reverts #113, which was merged by accident, and says why rather than only undoing it. It added a knowledge search over `documents`, `chunks` and `document_acls`: our own copy of a customer's corpus, ranked here, with an ACL predicate of our own writing deciding who may see what. The code is careful and the ACL filter is in the right place. The design is the thing being taken back. A Bot answers from a live system by calling that system's own search, as the person asking. The vendor decides what they may see, because the vendor is the only thing that actually knows: an index here is a permission model we have to keep in step with theirs, and every gap between the two is an answer assembled from documents somebody cannot open. It is also a second copy of their data to secure, to keep current, and to remember to delete when somebody leaves. Retrieval has a place later, over the tool catalogue rather than over documents: choosing which of a hundred tools to call is a search problem, and one about our own metadata rather than a customer's files. That is a different thing wearing the same word. The write half of the index goes with #97, which removes the connector that filled it. The three tables and the `knowledge/` modules are read by nothing after this and should be dropped in a change that says so, rather than as a side effect of this one.
davidmckayv
pushed a commit
that referenced
this pull request
Aug 22, 2026
#118 asked for the knowledge/ modules and the three tables to be dropped in a change that says so. This is the half that can be done now. server/src/knowledge/acl.ts, repository.ts and types.ts have no importer outside their own two test files. InMemoryKnowledgeRepository holds documents, chunks and ACLs in a Map in the process, which is the shape #21 took back, and canRead(actor, entries[]) filters ACL rows already pulled into memory rather than in SQL. Neither is a starting point for anything #119 describes. The three tables stay for now. PR #97 has not merged, so connectors/sync-persistence.ts still imports documents, chunks, document_acls, connector_cursors and sync_runs and writes to all of them; dropping the tables breaks typecheck there. It is already orphaned at runtime, its only caller being its own integration test, so this is a typecheck dependency rather than a live one, but it is #97's to remove. Left alone deliberately: connector_instances, which server/src/connectors.ts writes to from production code; connector_cursors and sync_runs, which are connector-side and go with #97; and webhook_subscriptions, which is referenced nowhere at all and wants its own change. Docs that describe pgvector holding knowledge records stay accurate while the tables exist and belong in the commit that drops them.
Only CHANGELOG.md conflicted, in Upgrading and in Fixed, and both sides were entries added independently. Kept both. The knowledge modules main deleted in #124 are not referenced here, and this branch removes sync-persistence.ts and the worker connector runner, which was the only thing still importing the document tables. The migration chain needed no renumbering: main ends at 0007 and this adds 0008 and 0009.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Answers from Google Drive, as the person asking.
What this delivers
K1 — Answer from a live system. Done. Ask a Bot a question whose answer is in Drive and it
answers, with a link that opens the file. The connector is granted, policy-checked and audited like
any other MCP call, and an answer with nothing behind it says so rather than returning an empty
string a model would fill in from memory.
K2 — As me, not as the deployment. The architecture, not the proof. Per-person credentials in
the existing vault, per-user OAuth, and a tool call that runs on the asker's own grant with no
fallback of any kind. Two things K2 asks for are not here, and are called out below rather than
left for a reviewer to discover.
How a deployment uses it
An administrator enables the connector at
/admin/plugins/google-driveand registers one OAuthclient. Each person then connects their own Google account, and nobody can do it for them — there is
no endpoint for an administrator to connect on somebody's behalf. Read-only: the scope requested is
drive.readonly, so a write is refused by Google before this deployment has to. Nothing is cached —the refresh token is stored and an access token is minted per call, so revoking at Google takes
effect on the next one rather than when a cache expires.
See docs/plugins/google-drive.md, which is mostly the diagnosis of
getting it working against a real account.
Two decisions worth reviewing
We do not use Google's Drive MCP server. That was the original design and it is the better one on
paper — a vendor-maintained server needs no Drive-specific code here at all. It is gated behind the
Google Workspace Developer Preview Program, and refuses an unenrolled project with
The caller does not have permission, which describes the project rather than the credential, so every checkavailable locally reports a correct setup. It is correct. Enrolment is a Workspace application with a
stated turnaround of days and no certainty at the end.
So this reaches Drive over its ordinary REST API, GA since 2015, behind a transport seam. The adapter
implements the interface
mcp.tsalready exported rather than inventing one, so MCP is still thecontract and going back is
transport: "mcp"plus a host and path. Tool names are Google's MCP namescharacter for character, so every grant survives the swap in either direction. There are exactly two
transport call sites in the system; nothing else — not the OAuth flow, the credential selection, the
grants, the policy engine or the audit trail — knows which protocol is underneath.
credentialshas no user column, and this adds a join table rather than one. The board suggesteda column. The pair
(server_id, user_id)needs to be the key: with a surrogate id and no uniqueconstraint two rows for one pair are legal, and then somebody who reconnected keeps being served the
grant they thought they had replaced.
Known gaps
revocation page, rather than offering a control that would report access withdrawn when it had not
been. This is the main piece of K2 left.
tested at the store level with two users, including that a call with nobody's credential is
refused before the network — but that is not the same claim as two real Google accounts with
deliberately different access.
copy_file,create_file,download_file_content,get_file_permissions). They list nothing today, so they are inert, butthey would come back if the transport were swapped to MCP. Left rather than pruned silently.
mcp_servers.credential_idistextagainst auuidprimary key with no foreign key. Thislet a test leave a dangling pointer at a real OAuth client during development. A migration would
make that class of bug impossible; not done here.
POST /api/plugins/callhas no caller in this repository. It is what the browser posted towhen a Bot's tool loop ran client-side. Ask whether the person may act as the Bot they named #37 hardened it with
canUseBotafter that loop movedto the server, so this branch fixes its actor argument — it passed an email where a
users.idisneeded, so it could never have worked for a per-person connector — and leaves the question of
removing it to a change that says so.
Verification
bun run format,typecheck,lint,test(1054 pass, 0 fail) andbuildare green. The connectorwas driven against a real Google account: tool refresh, recent files and search all return real
results, and every result line carries a markdown link.