Have Knowledge admit no source is connected, instead of citing one - #58
Merged
davidmckayv merged 2 commits intoAug 20, 2026
Merged
Conversation
Hotragn
requested review from
MikeRyanDev,
davidmckayv,
guidovizoso and
tylerslaton
as code owners
August 20, 2026 20:27
davidmckayv
force-pushed
the
have-knowledge-admit-no-source
branch
from
August 20, 2026 22:36
93cf23e to
7b81387
Compare
Contributor
|
Rebased onto main and pushed one commit on top: the wording now keeps the instruction to cite alongside the admission that there is nothing to cite. Your diagnosis was right and the Both halves, and it still matches the connectors-page rule you cited: say plainly what isn't there. On the local tooling: nothing to apologise for, and stashing to reproduce on a clean |
davidmckayv
approved these changes
Aug 20, 2026
davidmckayv
left a comment
davidmckayv
left a comment
Contributor
There was a problem hiding this comment.
Approved. Narrow, correct, and the doc mirror is kept in sync.
davidmckayv
approved these changes
Aug 20, 2026
bryangarcia831
pushed a commit
to bryangarcia831/openbot
that referenced
this pull request
Aug 21, 2026
…opilotKit#58) * Have Knowledge admit no source is connected, instead of citing one * Keep the instruction to cite, alongside the admission that there is nothing to cite --------- Co-authored-by: David McKay <davidmckayv@users.noreply.github.com>
davidmckayv
added a commit
that referenced
this pull request
Aug 22, 2026
* Take out the Drive connector that answered as the deployment
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.
* Make a server say whose credential reaches it, and add Google Drive
`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.
* Say when a tool returned nothing, instead of returning nothing
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.
* Give one person one credential per server
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.
* Call a user-oauth server as the person asking, or not at all
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.
* Let a person connect their own account, and believe only what we signed
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.
* Give the two halves of a connector the two screens they belong on
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="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/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.
* Document the one setting a connected account needs
`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.
* Send somebody back to the app after consenting, not to the API
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.
* Give every plugin its own page, and two lists instead of three tabs
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.
* Give each row's icon a tile to sit in
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.
* Leave one connector in the catalogue, and drop the tile's border
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.
* Make enabling a connector a switch, and keep the tile for third parties
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.
* Separate the plugin page's sections, and drop its icons
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.
* Move Refresh tools onto the Tools heading
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.
* Give connected accounts its own screen, and a page per service
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.
* Stop the credential test from eating the deployment's OAuth client
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.
* Connect with a button that admits it leaves, disconnect from a menu
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.
* Return from consent to the account, not to a list
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.
* Say which status a vendor refused with, and stop the suites eating real 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.
* Keep a 403's reason, and write down what it took to connect Drive
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.
* Stop the tests deleting real grants, and stop the trail claiming success
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.
* Point the Drive troubleshooting at the audit trail
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.
* Record the vendor's sentence on a failed tool result, and name the preview 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.
* Reach Drive over its GA REST API, behind a swappable transport
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.
* Stop Knowledge denying access to files a tool just handed it
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.
* Let an administrator set Drive up without leaving the admin screen
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.
* Give each tool its own screen, with a switch per Bot
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.
* wip: pre-merge checkpoint
* Say what this changes for somebody running OpenBot
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.
* Retire a person's connector credentials when they are removed
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.
* Restore the client pointer whether or not the suite borrowed a server
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.
* Add the offboarding change and the credential-pointer migration to the 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.
* Say what the Drive connector needs before it works, and link the doc
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.
---------
Co-authored-by: David McKay <davidmckayv@users.noreply.github.com>
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.
What this changes
The Knowledge coworker's
role_descriptioninexamples/fintech/agents.yamlalready says it should "cite sources when available" — correctly hedged, since not every deployment has a knowledge source connected. Itssystem_prompt, the text actually sent to the model, disagreed: "Answer from authorized company knowledge and cite every source," with no hedge at all. The same unconditional wording is repeated in theagents.yamlexample insidedocs/configuration.md.Out of the box,
TENANT_PACKAGE_DIRdefaults to this fintech example, so a fresh install ships a Knowledge coworker instructed to always cite a source, whether or not one is connected — a citation invented under instruction reads the same as a real one.This changes the wording in both places to match the hedge already present in the same block, matching a pattern the connectors page already uses on purpose:
app/src/routes/_authed/admin/connectors.tsxshows "No setup screen yet" for an unconfigured OneDrive connector rather than a broken or blank control, specifically so the product says plainly what isn't there instead of implying it. The Knowledge prompt should hold itself to the same rule.This is a narrow fix to the example config and its matching doc block. It does not touch the underlying gap that a Knowledge coworker has no retrieval behind it in this repo yet — I'm filing that separately as an issue, since it's a bigger, cross-cutting change that should be proposed before it's built.
Where it runs
Boundary and audit
Proof
This is a two-line text change to a YAML string and its doc mirror; no runtime code, imports, or tests reference the exact string (
grep -rn "cite every source"matched only these two files before the change).I could not get a clean local run of
format:check/lint/typecheckin this dev environment:bunx biome format .andbunx biome lint .both crash (access violation) on this machine, andbun run typecheckreportsCannot find module 'yaml'/'zod'/etc. from several files. I confirmed both failures are pre-existing and unrelated to this change by stashing it and reproducing the identical errors on a cleanmaincheckout with zero edits — looks like a package-linking issue local to this Windows setup, not something this PR introduces.bun test tests/fintech-package.test.ts, which exercises this example package, doesn't assert the exact prompt string either way and isn't affected. Happy to have CI be the real signal here.