Replace a connector's tool list in one step, so a failed refresh leaves it alone

[kevin9327]

What this changes

server/src/plugins/store.ts:1927-1937 replaced a connector's tool list with two auto-committed
statements:

await database.delete(mcpTools).where(eq(mcpTools.serverId, serverId));
if (tools.length > 0) {
  await database.insert(mcpTools).values(tools.map(...));
}

Eighty lines below, the catch states the invariant this is supposed to hold
(store.ts:2016-2019):

The tools already held are left alone: a vendor being briefly unreachable is not a reason to
revoke what Bots are using.

That holds only for a failure before the delete — a refused token exchange, an unreachable host —
which is the case plugin-store.integration.test.ts:2194 covers. Once the delete has committed,
anything that stops the insert leaves the connector with no tools at all.

It does not take a crash. mcp_tools is keyed on (server_id, name)
(server/src/db/schema/plugins.ts:110) and tools.map(...) is not de-duplicated, so a server that
answers tools/list with the same name twice makes the single multi-row insert fail on a duplicate
key. A custom server an administrator added by URL, or a curated vendor having a bad day, is enough.
A pod killed mid-refresh, a dropped connection or a statement timeout do the same thing.

And it is not one replica. mcp_tools is shared state, so every replica loses that connector at
the same moment:

• listForAgent (store.ts:2444-2455) reads grants against the tool list, so every grant an
  administrator made stops being offered without ever being revoked.
• grantedToolGuidance (plugins/tools.ts:79-147) then tells the Bot outright that it holds none of
  that vendor's tools and to say so — so the deployment does not fail, it confidently answers wrong.
• Nothing repairs it. refreshTools has exactly three call sites — addServer (store.ts:1591),
  addCustomServer (store.ts:1748) and POST /api/plugins/servers/:id/refresh
  (plugins/routes.ts:320). There is no timer and no queued job, so the connector stays empty until
  somebody reads lastError on the Plugins page and presses Refresh by hand.

Wrapping the pair in the transaction the store already uses elsewhere (store.ts:914, :1117,
:1219) makes the replacement one step: the vendor's bad answer lands in lastError and the Bots go
on using what they were granted, which is what the comment always said.

I deliberately did not add a de-duplication pass. It would only mask the one trigger that needs
no infrastructure fault, and it would quietly accept a malformed tool list rather than recording it —
lastError naming the duplicate is more useful to an operator than a list silently repaired.

Where it runs

• New state that outlives a request? None.
• What happens on the second replica? This is the fix for the second replica. The rows are in
  Postgres and shared; the change is that a half-applied replacement can no longer be observed by
  any of them, because it can no longer commit.
• Anything serialised? The delete and the insert now commit or roll back together, in one
  transaction. Two concurrent refreshes of the same server still serialise the way they did — the
  later one's delete waits on the earlier one's row locks and then replaces the list wholesale,
  which is the same outcome as before and the same outcome as either one alone.
• Anything fanned out to a browser? No.
• New listener, port, or schedule? None.

Boundary and audit

• Every acting call still goes through the gateway: unchanged, nothing on the acting path.
• New refusals and new failures each write a row: the failure already writes lastError and the
  configuration.changed / withdrawn-grant rows below are untouched — they now run only when the
  replacement actually landed, which is the point.
• Nothing new is trusted from the client.

Changelog

• CHANGELOG.md, under Unreleased.

Proof

bun run format:check   -> Checked 487 files. No fixes applied.
bun run lint           -> Checked 490 files. No fixes applied.
bun run typecheck      -> app / server / worker all exit 0
bun test server/tests/plugin-routes.test.ts server/tests/plugin-connect-route.test.ts \
         server/tests/builtin-routines.test.ts
                       -> 49 pass, 0 fail

The two existing refreshTools cases in plugin-store.integration.test.ts (:2149, :2209) cover
the successful replacement and the pre-delete failure; CI runs both against a real PostgreSQL.

No new test, and I want to be straight about why rather than quietly ship without one. The suite
reaches this code through the vendor, and MCPMock keys its tools by name
(mcp-mock.js:20-23, this.tools.set(def.name, …)), so it cannot be made to advertise a duplicate.
The other triggers are a killed pod and a dropped connection. What would pin it is a fault-injected
database whose insert on mcpTools throws inside the real transaction — and I have no PostgreSQL
on this machine, so I could not run that wrapper before sending it, and I would rather not put an
unverified fixture in front of you. If you would like it, say so and I will add it; I would also be
glad to be told a seam I have missed.

[chatgpt-codex-connector]

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.

[davidmckayv]

Verified + CI verify green; template-appropriate correctness fix.

[davidmckayv]

CI verify green pre-rebase; CHANGELOG-only rebase; template-appropriate.