Five out-of-subject findings from PR #770's tier-3 review (credential-revoking tests, single-user boot gate, OAuth client rotation, shared-use atomicity, untyped server/tests)

This bundles five independent defects, so split them before starting. Each has a file and line: server/tests/plugin-credential-rotation.integration.test.ts:96, server/src/config.ts:577 (reachOf), server/src/plugins/store.ts:2486-2641, server/src/plugins/shared-use-store.ts:250 and :637-663, and the server/tests type errors. Done means each fix has a test that fails before and passes after, and the typecheck covers server/tests. Ask a maintainer which item to take first, since several touch security-sensitive code.

Five load-bearing findings surfaced by the tier-3 code review of #770 (80 reviewers, ~450 raw
findings). All five are real and all five are out of that PR's subject — it is about adding
Arcade as a second broker and its per-action OAuth lifecycle, and none of these are. They are filed
here rather than absorbed into #770, which would have conflated two subjects in one diff.

Each is recorded with the evidence that established it, so none needs rediscovering.

1. Two integration tests permanently revoke real OAuth client credentials

server/tests/plugin-credential-rotation.integration.test.ts:96 and
server/tests/plugin-user-credential.integration.test.ts:188.

Both declare const serverId = "google-drive" — a fixed literal, not suite-scoped — then UPDATE credentials SET revoked_at = now() for every live mcp_oauth_client row matching it. Neither
restores revoked_at: the rotation suite's afterAll only nulls the credential_id pointer and
deletes rows the run created.

The comment sitting on the rotation suite's revocation reads:

This run's own client key, so a deployment's real Drive registration is never touched.

That is the opposite of what the code does. The TEST_DATABASE_URL guards keep the blast radius off
a dev database (they refuse the openbot database by name), so this is not a live incident — the
defect is that the source asserts a scoping it does not have, which is what makes it survive review.

Verified: both literals read directly; grep revokedAt over the rotation suite shows the set at
:99 and no restore anywhere.

2. reachOf classifies a scheme-less public address as private

server/src/config.ts:577.

new URL("example.com:3010") parses — protocol example.com:, hostname "" — so it never reaches
the "unparseable counts as public" guard its own docblock promises. The empty hostname then falls
through to the !bare.includes(".") branch and returns "private".

Consequence: OPENBOT_SINGLE_USER with a public OPENBOT_PUBLIC_URL / OPENBOT_APP_URL /
TRUSTED_ORIGINS written without a scheme warns instead of refusing boot, leaving no-sign-in
admin access on a routable address.

Verified (bun 1.4.2): new URL("example.com:3010").hostname === ""; "example.com" throws (so
it is correctly public); "http://example.com" resolves normally. Two reviewers found this
independently.

3. A transient fault rotates away the live OAuth client

server/src/plugins/store.ts:2625-2641 and :2486-2498.

storedOAuthClient / heldOAuthClient use a bare catch { return null }, so a DB or decrypt
failure is indistinguishable from "no client registered". ensureOAuthClient then re-registers —
rotating away the live client and invalidating every existing consent.

4. Shared-use approval writes are not atomic, and one 500s from a request body

server/src/plugins/shared-use-store.ts:250 and :637-663.

• decide() flips request status in one statement and writes the approval in another. A failure
  between them leaves a permanently approved request with no approval, unretryable — the claim
  requires pending.
• setApproval inserts approval members without de-duplicating, so a repeated (kind, value)
  violates the members primary key and 500s. Reachable from a request body (asSharedUseApproval
  never dedupes) and from exposureOf, which emits a group twice when a Bot is both published to
  and assigned to that group.

5. server/tests is outside tsc, and it is hiding real type errors — FIXED, and the figure below was wrong

Corrected 2026-10-14. The text below said four errors were proven. That was the size of the
sample taken at the time, not the size of the problem. When server/tests was actually brought into
a typed program it reported 277 errors — roughly seventy times the number quoted.

The gap itself is now closed. bun run typecheck:tests type-checks server/tests against a
frozen baseline (server/tests/typecheck-baseline.txt) and fails on any error not already recorded.
The baseline is 264 today: lower than when it was frozen, because some were repaired rather than
merely recorded. It has already earned its place, catching breaks that appeared only when
independently-correct branches were merged and that every individual author had missed.

Status of the instances named below: mcp-grant-removal.integration.test.ts:25 (TS2739) is
fixed; :35, :61, server-side-tools.integration.test.ts:47 and four plugin-routes.test.ts
entries remain open and are now gated. The remaining items are recorded as debt rather than hidden —
the baseline file is explicit that every line is a real defect, that deleting one by fixing it is
always correct, and that adding one is not.

The original text follows, unaltered.

---

Not a style point — four were proven by building a scratch tsconfig that pulls the files into the
typed program:

• mcp-grant-removal.integration.test.ts:25 — the ConnectedAppBroker literal is missing
  connectionFields, connectWithFields, revokeAccount (TS2739).
• :35 — ensureAuthConfig: async () => undefined violates Promise<AuthConfigOutcome> (TS2322),
  silently forcing store.ts:3488's non-"standing" branch so the config-reuse path is never
  exercised.
• :61 — the credentials stub is not assignable to CredentialSecretReader & CredentialStore.
• server-side-tools.integration.test.ts:47 — the credentials seam omits required
  create/updateSecret/revoke.

And plugin-routes.test.ts:50 documents a Partial<PluginStore> + fixed-tuple arrangement whose
stated purpose is "a compile error rather than a green test" — unenforced, because
server/tsconfig.json excludes the file. Its own serverRecord() omitting three ServerRecord
fields is the proof.

This is the same gap that hid a missing required option during #770's merge with main: nine of
main's tests died at runtime in toolkitOfFor because a plugin store was built without
brokerProvider, and typecheck stayed green throughout (fixed in 75614fb2).

Related

• #785 — server/tests connection leak (55 files open a pool and never close it).