Repository navigation
Commit f1961c3
feat(arcade): Arcade as a second connected-app provider (PR 1) (#770)
* docs(arcade): say which object a `provider_id` is on, and record the live run
All of this was measured in the first live run against the Arcade API on this
branch, from this deployment's own code.
`provider_id` names three different things, and nothing in the tree said so: on
a tool listing it is the provider TYPE (`google`), on a user-connection row the
provider INSTANCE id (`google-openbot`, `cpk-poc`), and on an auth-provider
config the two sit side by side, `id` being the instance and `provider_id` the
type. A connection row carries no vendor-type field at all — its `provider_type`
is `"oauth2"`, the auth scheme — so the instance id is the only link to the
vendor. The `authProviderOf` docblock claimed the member answered with a
configuration; it answers with the type, which is what made the comparison fixed
in 28b13e4a look right.
The operator page gains that distinction with the verbatim config and
connection-row shapes, and notes that it bit this deployment once: a connection
that existed read as not connected.
Moved out of "unproven" on that page, all now measured:
- sibling revocation — one `google` provider serves `Gmail` and `GoogleCalendar`,
so disconnecting one app can end another, in production;
- Google's per-action isolation — 0 of 30 scope-carrying `Gmail` actions
authorized before one real consent to `Gmail.GetEmail`, 11 of 30 after, and
nothing beyond the granted scopes;
- the wrong-user confirm shape — a generic `400 coordinator_error "Bad request"`
naming no mismatch, so a wrong-account return presents as the vendor-unavailable
refusal rather than its own sentence.
Added as unproven: Google's "Use this client for an AI-powered agent" checkbox,
a self-declaration that puts the client under a separate admin-policy surface and
which Google advises keeping apart from human-workflow clients. OpenBot's
brokered calls are agent-initiated, so a deployer should choose deliberately; it
was left unticked today to keep a policy variable out of the measurement.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* test(arcade): correct the live suite against what the first real run measured
The Arcade live suite ran against the live API for the first time: 9 passed, 3
failed, and not one failure was the vendor's. One was a product defect in the
connection filter (fixed in 28b13e4a); the other two, and a duration failure,
were defects in this suite. All three are corrected from the measurement rather
than from the assumption that preceded it.
TIMEOUTS, because bun's per-test default is 5000ms. `next_uri is refused for
every value` makes five sequential vendor round-trips and died at 5001.03ms
having measured none of them. Every case that dials Arcade now carries an
explicit timeout, following the pattern the consent case already used: 60s for
a case, five times that for the five-call `next_uri` case, the poll's own bound
for `auth.status`, and the same on the `afterAll` cleanup, which also dials.
GATE 4 ASSERTED THE WRONG SPELLING, and that is how it hid the product defect.
It filtered connections by the provider TYPE, found nothing, and concluded "the
filter does not take this provider id". The filter is fine; `provider_id` means
two different things: the TYPE on a tool listing (`google`), the INSTANCE on a
connection row (`google-openbot`, `cpk-poc`, `arcade-google`), with the
auth-provider config the only bridge between them. It now prints the consented
connection's own `provider_id` verbatim — the line whose absence made the run
ambiguous — resolves the instances through `admin.authProviders.list()`,
asserts the connection is found by instance, and measures the case handling
against the instance, which is the value a filter is actually given.
GATE 2 MEASURED THAT ITS ASSUMPTION IS FALSE, so it now records that instead.
Arcade answers a wrong-user `confirm_user` with a generic 400
`coordinator_error` / "Bad request", carrying nothing that identifies a
mismatch, so the `{ mismatch: true }` contract is unreachable through the live
API today; detecting it would mean reading every bad request as a mismatch. The
product keeps that branch — it costs nothing and starts working if the vendor
ever distinguishes the case — and the case asserts the measured reality: the
vendor refuses, `confirmReturn` throws, and the return leg refuses with
`vendor-unavailable`. A UX degradation, not a hole: the leg stays bound by the
`auth_id` comparison regardless. Both other outcomes are still printed.
Suite only; no product code is touched. The live suite was NOT re-run — it needs
a person at a browser. Verified that it still type-checks against the server's
config, still skips every case with no key, and that `test:live-arcade` still
exits non-zero without one.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* test(memory): name the broker the defaults fixtures already use
`main`'s memory-defaults suite built a plugin store without `brokerProvider`.
The option is required and deliberately has no fallback, because the store
dispatches its toolkit read on it, so all nine of those tests died in
`toolkitOfFor` the moment this branch and `main` met. The fixtures are
`composio://` rows with `provenance: "composio"`, so they now say so.
`server/tests` sits outside `tsc`, which is why a missing required option
surfaced as a runtime `TypeError` rather than a type error.
This belonged in the merge commit and missed it: the `git add` that should
have staged it named a path that does not exist, which makes git stage
nothing, and the error was suppressed. The merge commit's verification line
describes a tree that had this fix applied but uncommitted.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* test(arcade): give back the pools these files open
`bun test` runs every file in one process, a pool is never handed back, and
nothing reaps an idle connection — so a file costs two connections from its
first query until the run ends. PostgreSQL's 100, less the superuser reserve,
is therefore reached by WRITING A FILE rather than by any file misbehaving,
and these fifteen are the files that reached it.
Measured, because the prose this branch shipped says otherwise. The sharded
runner's docblock states the slots go "during the import phase"; constructing
a pool opens nothing at all, the first query opens the pool's full `max`, and
twenty pools sat flat at forty connections for as long as they were watched.
Exhaustion is a function of how many files have RUN, which is why a sharded
sweep never sees it and `test:ci` does.
Half this suite already knew. Forty-three files on `main` close their pool in
their own `afterAll`; fifty-five do not, and the fifteen added here were
fifteen more. Closing them puts this branch back at `main`'s own number, which
is the number CI passes at today. Nothing about the harness changes: no
workflow edit, no shared pool, no new convention — these files now do what
`identity-store.integration.test.ts` and forty-two others already do.
The close goes last in each file's `afterAll`, after that file's cleanup,
because hooks run first-registered-first and a pool closed before the deletes
takes the deletes with it.
Verified against a freshly migrated test database: `format:check` 0, `lint` 0,
`typecheck` 0, `bun test app/tests` 1246 pass / 0 fail, `bun run
test:server:sharded` 5027 pass / 1 fail / 19 skip, and `bun run test:ci` — the
whole-repo single-process run CI performs — peaked at 84 connections against a
ceiling of 97, with zero `too many clients`, where it had reported 114 such
failures before.
The one remaining failure is not this change: `arcade-journey`'s "two
separately-shared apps" fails identically with and without it, and is the
merge-gate item of that name. Connection exhaustion had been hiding it.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* test(arcade): ask the trail for its order instead of assuming one
`eventsOn` read the audit trail with no `ORDER BY` and the assertion below it
takes `.at(-1)`, meaning "the disconnect this test just caused". An unordered
`SELECT` does not answer that question. PostgreSQL may return such a read in
any order, and what it returns in practice is roughly heap order — which
agrees with time order right up until it doesn't.
It didn't on CI. The last row back was the PERSONAL disconnect from the
describe above, so the Shared app's entry was read as a person ending their
own account:
- owner: "deployment", reason: "admin"
+ owner: "arcade-journey-…", reason: "self", vendorRevocationRequested: true
THE PRODUCT WAS RIGHT THE WHOLE TIME. Both entries are written, correctly and
48ms apart — the personal one at `…42.954`, the deployment one at `…43.002`.
Only the reading of them was wrong, and it reported the product for it: an
audit misattribution on a Shared account, which is the kind of defect somebody
would rightly stop a release over.
Reproduced away from the suite, by making heap order disagree with time order
— insert the newer row first, then the older. Unordered, `.at(-1)` is the
older row; ordered by `createdAt`, it is the newer one. That is the CI failure
exactly.
`createdAt` is the only key that can carry this, because `id` is a random UUID
and sorts arbitrarily. The two disconnects arrive on separate requests and so
in separate transactions, which is what makes their `now()` values distinct.
This was invisible until today for a reason worth recording: the whole-suite
single-process run is where the two describes' rows meet, and that run has
been drowning in `too many clients` since before this branch existed.
Verified: `format:check` 0, `lint` 0, `typecheck` 0, `bun test app/tests` 1246
pass / 0 fail, `bun run test:server:sharded` **5028 pass / 0 fail / 19 skip**.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* fix(arcade): read every listing to its end, and tell an absent total from a zero
`@arcadeai/arcadejs`'s `OffsetPage` constructor does `this.total_count =
body.total_count || 0`, so an ABSENT `total_count` is indistinguishable from a
reported zero. Two defects fall out of that one coercion, both measured against
a stub HTTP engine with the real SDK building real pages:
- `connectionsFor` used the SDK's own auto-pagination
(`for await (… of client.admin.userConnections.list(…))`), and
`nextPageRequestOptions()` returns `null` the moment the total is falsy. With
250 connections served 100 to a page and no `total_count`, ONE request went
out: `revoke` deleted 100, ANSWERED `true`, and left 150 connections live at
the vendor — a person told the disconnect succeeded while it had not. That is
the outcome `revoke`'s own comment forbids. `isConnected` and `accountName`
read off page one the same way.
- `listApps` and the transport's `listTools` kept the LAST page's `total_count`,
which the coercion turned into `0` whenever the short final page omitted the
field. An engine truncating at 250 of 300 committed a 250-action listing as
complete, with no error.
Fixed at the root, not per call site:
- `reportedTotalOf` reads the total off the RAW parsed body (`AbstractPage.body`)
and falls back to the page property only when there is no body, so "the vendor
did not say" can never read as "the vendor said zero".
- `highestTotal` keeps the HIGHEST total any page reported, never the last, so a
short final page that drops the field cannot erase page one's claim.
- `pagedItems` reads one listing to its end: pages until a SHORT page, never
touches the SDK's auto-pagination, and the incompleteness throw is the last
thing it yields, so a caller folding items as they arrive still cannot see a
short listing finish as a whole one. A partial or unverifiable listing now
THROWS on every path — neither a clean success nor an empty "nothing there".
- `createArcadeClient`'s `actions.listTools` normalizes `total_count` at the
seam, OMITTING the field when the vendor omitted it. That is the only place it
can be done: `./arcade` must not import this file (cycle), so it cannot reach
`reportedTotalOf` itself.
Same-pattern audit over both files, every instance fixed:
- `listApps` (`arcade-adapter.ts`) — now through `pagedItems`.
- `authProviderInstances` — now through `pagedItems`.
- `connectionsFor` — was the SDK's auto-pagination; now explicitly paged.
- `grantsFor` — was a single `await` with no loop, and answers one `GrantState`
PER ACTION, so a first page silently dropped every action past it
(`GithubApi` publishes hundreds). Now paged.
- `arcade.ts listTools` — highest-total tracking.
- `authProviderOf` — LEFT UNPAGED, deliberately, and the docblock now records
why: it answers one fact for the WHOLE toolkit, so a further page can only
repeat the first, and paging it would be paid on every `isConnected`,
`accountName` and `revoke`.
Covering test: `server/tests/arcade-pagination-engine.test.ts` serves real JSON
over a real socket through `ARCADE_BASE_URL` and lets the SDK build its own
`PagePromise`/`OffsetPage`. The existing adapter suite stubs resource PROTOTYPES
and hands the adapter a bare async generator, which is exactly what hides this
class — a test in that style cannot go red here. Against the unfixed source this
file fails 5 of 7 (revoke spent 100 deletes of 250 and returned `true`); against
the fixed source it passes 7 of 7.
Symbols added, all module-private to `server/src/plugins/arcade-adapter.ts`,
exported by nothing, so they have no call sites outside it: `reportedTotalOf`,
`highestTotal`, type `Listing`, `pagedItems`, `CONNECTION_PAGE_SIZE`,
`CONNECTION_MAX_PAGES`. No symbol was renamed or removed and no exported
signature changed.
Call sites enumerated for what did change behaviour:
- `actions.listTools` return VALUE shape (a plain `{items, total_count?}` rather
than the SDK's `OffsetPage`; the declared `ArcadeActions["listTools"]` type is
unchanged). One consumer: `server/src/plugins/arcade.ts:179`
(`installed.listTools`), installed from `server/src/index.ts:797`. Test doubles
implementing `ArcadeActions` already answer plain objects and are unaffected.
- `broker.listApps()` — `server/src/plugins/logos.ts:20`,
`server/src/plugins/routes.ts:957`, `server/src/plugins/routes.ts:1066`.
- `broker.grantsFor(...)` — `server/src/plugins/routes.ts:2183`.
- `broker.isConnected(...)` — `server/src/plugins/store.ts:6887`.
- `broker.revoke(...)` — `server/src/plugins/store.ts:4076`,
`server/src/plugins/store.ts:7969`, `server/src/plugins/store.ts:8198`.
- `broker.accountName(...)` — `server/src/plugins/store.ts:5847`.
All of these could already receive a throw from this path — `authProviderOf` and
`authProviderInstances` throw on a malformed listing today, and `connectionsFor`'s
docblock already records that it propagates onto the Disconnect path — so no
caller needed new handling.
Three fixtures were taught the page shape the adapter now consumes, because they
stubbed `userConnections.list` with a bare async generator that has no `items`:
`arcade-adapter-connections.test.ts`, `arcade-adapter.test.ts`,
`arcade-journey.integration.test.ts`. The suite's structural blindness in general
is not addressed here; it is somebody else's finding.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01PsS7GKpBgupvAKampCVKAK
* fix(db): refuse to migrate a database that cannot reach every migration
`PgDialect.migrate` decides what to run with one comparison — the journal's
`when` against the newest `created_at` in `drizzle.__drizzle_migrations` — and
says nothing about a migration stamped below that mark which it has never run.
Merging `main` into this branch renumbered the arcade work to `0057` and gave
main's `0053_identity_links` a `when` of 1791547580808, older than the
1791774497487 a pre-merge database had already recorded for the branch's own
`0054_spicy_wallflower`.
Reproduced against a throwaway database: the pre-merge folder applies, the
post-merge folder dies on `CREATE TABLE "pending_authorizations"`, and after the
obvious hand repair (drop that table, run again) the migrator prints
`{"type":"migrations-applied","status":"ok"}` while `identity_links` and
`identity_link_challenges` were never created. A fresh database is unaffected,
which is why CI — always migrating from empty — cannot see it.
The migrations themselves are untouched: `0057_arcade_provider.sql` is byte
identical to a fresh generate and the snapshot chain is intact, so renumbering
again would be a second defect. What changes is that the migrator now looks
before it runs. `migrationsOutOfReach` names every migration stamped at or below
the database's high-water mark whose hash is not among the rows already applied —
those are unreachable, not pending — and `applyMigrations` throws
`MigrationsOutOfReachError` with the names and two ways out instead of migrating.
Nothing is applied, so the hard failure and the silent skip that follows its hand
repair are both replaced by one refusal that exits non-zero.
It refuses rather than repairs on purpose: the migrations are out of reach
because some other journal already ran here, and nothing in this process knows
what that journal did to the schema. An operator recreating a disposable database
or restamping a durable one is a decision, not a side effect of a deploy.
Validated for false positives against two real databases: `openbot_test` (58 rows,
fully migrated with this folder) and the development `openbot` (53 rows, at 0052)
both report nothing out of reach, and every applied hash is accounted for by this
folder — confirming the sha256-of-the-whole-file recipe matches what drizzle has
been writing for the whole history.
Every migrator invocation now runs the guarded script, since the unguarded CLI
was reachable from the three environments that actually hold such a database:
`db:migrate`, the compose `migrate` service that `scripts/start.sh` runs, and CI.
`drizzle-kit generate` and `drizzle-kit check` are unchanged.
Audit: `server/drizzle` is the only migration folder and `server/drizzle.config.ts`
the only config in the repository, and after this change `server/scripts/migrate.ts`
is the only thing that applies them. `scripts/start.sh` keeps its two-table canary
after a successful migrate, which is the symptom-level version of this check and
now sits behind a cause-level one.
Call sites of every symbol added or changed:
- `readMigrationFolder`, `migrationsOutOfReach`, `outOfReachRefusal`,
`FolderMigration`, `AppliedMigration` (new, server/src/db/migration-reach.ts):
server/src/db/apply-migrations.ts, server/tests/migration-reach.test.ts.
- `applyMigrations`, `MigrationsOutOfReachError` (new,
server/src/db/apply-migrations.ts): server/scripts/migrate.ts,
server/tests/migration-reach.integration.test.ts.
- `migrate`/`drizzle` imports removed from server/scripts/migrate.ts: no other
module imported them from there.
- `db:migrate` keeps its name and its behaviour on a healthy database, so its
three mentions stand: README.md:430, docs/development.md:77,
scripts/start.sh:277.
- `bunx drizzle-kit migrate` had exactly two invocation sites, both replaced:
.github/workflows/ci.yml and docker-compose.yml.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01PsS7GKpBgupvAKampCVKAK
* fix(arcade): cancel the authorization the path names, not whatever the actor holds
`DELETE /servers/:id/authorize` resolved `:id` through `brokeredAccountFor` and
then threw the resolved id away: `authorizationAttemptFor` is keyed on the actor
alone, so a cancel issued from app B deleted an in-flight attempt on app A and
answered `{cancelled: true}` as though it had done what was asked. That is the
mint/claim/complete correlation this PR holds from the minting end, broken from
the cancelling end.
The route now compares the attempt's `server_id` against the raw path parameter
— the same value `POST /servers/:id/authorize` writes when it claims the row —
and answers `{cancelled: false}` when they differ, the same non-error as there
being no attempt at all. The deletion still matches on `(actor_id, created_at)`,
so the existing race guard is untouched.
No symbol was added, renamed, resignatured or removed: `authorizationAttemptFor`
and `discardAuthorizationAttempt` keep their signatures, so every call site of
both (routes.ts:2681 and :2828; store.ts's own definitions; the mint's
definite-failure discard at routes.ts:2628; the mocks in
server/tests/arcade-authorize-cancel.test.ts and server/tests/arcade-verify.test.ts;
server/tests/arcade-attempt-store.integration.test.ts) is unaffected.
Audited every other route in routes.ts: no other authorization-lifecycle route
resolves an identifier and then acts on an actor-keyed lookup. `/arcade/verify`
takes no path identifier and binds on `(actor, auth_id)` deliberately.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01PsS7GKpBgupvAKampCVKAK
* fix(broker): a key of nothing is no key, decided where "configured" is decided
`activeProvider` tested the two API keys with `!== undefined`, so
`ARCADE_API_KEY=` with nothing after it, or a key that is all whitespace,
counted as a CONFIGURED provider. `selectProviderWiring` then built a vendor
client from `""` — the failure surfaced at the vendor, on somebody's first
connect, instead of at the setting that caused it.
Only `server/src/config.ts`'s `optional()` trim prevented this in the deployed
path, and nothing stated or tested that dependency. `activeProvider` takes
config-shaped VALUES rather than an environment, so every other caller (a test,
an embedder, a future loader) reached it untrimmed. What "configured" means for
a credential is this function's own question, so it is now answered there: a key
is present only if it is defined and not blank after trimming.
RED-GREEN: the new cases failed against the unfixed function —
`activeProvider({ arcadeApiKey: "" })` returned "arcade", and
`selectProviderWiring({ arcadeApiKey: "" }, …)` built an Arcade client
(factory called with a zero-length key) — and pass after.
Same-pattern audit of the whole of broker.ts: no other credential, key or
identifier is tested for presence in a way that admits a blank. `brokerProvider`
is a two-member union validated in config, not a free string; `brokerReturnUrl`
already refuses a whitespace-only address with `returnUrl.trim() === ""`;
`isFieldScheme` and `flagOf` test vendor-sent values against closed sets, not
presence. `provider-wiring.ts`'s `c.arcadeApiKey !== undefined` narrowing guards
are now unreachable with a blank key and are left as type narrowing.
No symbol added, renamed, resignatured or removed: `activeProvider` keeps its
name and signature, so all three call sites (server/src/index.ts:884,
server/src/plugins/provider-wiring.ts:44, server/tests/provider-selection.test.ts)
are unchanged. The startup refusal's sentence now reads "is not set to a key",
so that an operator who typed `ARCADE_API_KEY=` is not told the variable is
missing; docs/plugins/arcade.md quotes that sentence verbatim and was updated
with it.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01PsS7GKpBgupvAKampCVKAK
* fix(broker): refuse an activation count that is not a count
`assertActivatable` tested `otherProviderRows <= 0` and returned. `NaN <= 0`
is false, so a NaN did not return — it fell through to the refusal sentence
and reached a person as the literal words "NaN brokered connections". A
negative or fractional count did the same. Whatever produced such a value was
not counting rows, and not knowing how many rows the other provider holds is
not the same as knowing it holds none, so the guard now refuses on any count
that is not a whole non-negative number of rows, and says the count could not
be taken instead of repeating the sentinel back.
The zero case is now `=== 0`, so the only path that returns is a real zero.
The second half of the finding — that the call site's `ne(provider, wired)`
skips NULL-provider rows — does not hold: `brokered_connections.provider` is
NOT NULL in the schema and in migration 0052, and a NULL insert is refused by
the database, so there is no row the inequality can hide. Rather than add an
`IS NULL` arm that can never match, a test in arcade-provider-check pins the
NOT NULL, so dropping it fails loudly next to the reason the count needs it.
Call sites of `assertActivatable` (signature and name unchanged): the startup
activation check in server/src/index.ts:812, and server/tests/provider-selection.test.ts.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01PsS7GKpBgupvAKampCVKAK
* fix(plugins): refuse a provider configuration this deployment cannot decide
`selectProviderWiring` returned null wiring — no broker, no transport seam,
no word to anybody — for both keys set with `OPENBOT_BROKER_PROVIDER` unset.
Nobody wants that state on purpose: the deployment comes up holding two
vendor keys and brokering nothing, so every brokered feature is dead and the
symptom surfaces much later and far away. It is now refused at boot, in the
shape of the refusal already beside it, naming the two keys and the variable
and never a value.
Same-pattern audit of the whole file: the two vendor branches each carried
`&& c.<vendor>ApiKey !== undefined`, a narrowing that doubled as a silent
exit to the same null wiring. The narrowing is kept as `requiredKey`, which
throws instead of falling through; it cannot fire today, because
`activeProvider` selects only from a key it has seen, but if the two ever
disagree the deployment says so rather than unwiring itself. The one
remaining null wiring is returned from the branch that means it — no keys at
all — and is no longer reachable by falling through.
Symbols: `requiredKey` added, module-private, called twice, both in this
file. `selectProviderWiring` keeps its name, signature and return type;
its three call sites (`server/src/index.ts:792`, and
`server/tests/provider-selection.test.ts`) are unchanged in shape.
`activeProvider` is untouched: it is a query, and the refusal belongs to
the wiring.
`docs/plugins/arcade.md` said "with both set and the variable unset, neither
is active" and listed two refusals; it now states the third and quotes its
sentence.
Not changed: `provider-wiring.ts:61` building a client from an empty-string
key, which is a separate finding owned elsewhere. The `!== undefined`
semantics are preserved exactly here.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01PsS7GKpBgupvAKampCVKAK
* test(arcade): search a logged value for a credential instead of flattening it
The D31 gate exists to prove that a bearer URL and an access token reach no
model, log line, error body, audit row or redirect. It read each captured
console argument through `String(arg)`, which renders an object as
"[object Object]" — so a token logged INSIDE an object passed it, and the
status reply the case feeds through carries one in `context: { token }`. The
gate could not see the very shape it constructs.
Every value is now WALKED by `harvest`: array and set members, map keys and
values, own property names and their values, an error's message, name, stack
and cause, and the rendering of anything with a `toString` of its own. The
audit and connection rows go through it too, since `JSON.stringify` is blind
to a Map, a Set and an Error's message. The answer is checked WHOLE — every
header and the body — rather than `Location` alone, in the D31 case and in the
vendor-unavailable case whose error message carries the token.
Watched against a planted leak: `console.log("...", { context: answer })` in
`settledStatus` passed the old gate and fails the new one, naming the token.
No real leak: the route is clean on all four surfaces.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01PsS7GKpBgupvAKampCVKAK
* test(plugins): spell the vendor column as production does and prove both directions of the cross-provider guard
The two store provider fixtures modelled a deployment that cannot exist. Both
wrote `vendor: "arcade"` / `vendor: "composio"` — the lower-case provider key,
which is what the PROVENANCE column holds. Production mints the vendor column
from `BROKERED_PROFILE[provider].vendor`, which is `"Arcade"` / `"Composio"`.
Both files now take that word from `BROKERED_PROFILE` rather than spelling a
second, wrong one; nothing on these paths reads the column, so this buys
fidelity rather than coverage, and it is recorded as such in the report.
The guards themselves were each asserted in one direction only:
* `store-toolkit-dispatch.test.ts` proved an `arcade://` row answers nothing
under a composio store, and nothing at all about a `composio://` row under
an arcade store. The dispatch is a per-provider table, so one entry proved
is one entry proved.
* `store-provider-required.test.ts` proved a composio store does not read an
arcade connection filed under the same app name, and not the mirror.
A test for each missing direction is added.
No symbol added, renamed, resignatured or removed: both files gain a value
import of `BROKERED_PROFILE` from `../src/plugins/access` (already exported and
already imported by `store-scheme-sites.test.ts`), and two `test()` cases. No
call site anywhere changes.
Break-the-guard evidence (source reverted after each):
* `toolkitOfFor` made to try both parsers — the new dispatch mirror fails
with `Received: { toolkit: "gmail", accounts: [] }` where null is required.
* the `provider` predicate dropped from `accountsOn`'s connection read — the
new provider-required mirror fails with the foreign `other-*` account
appearing in the slice.
The `@ts-expect-error` pin at `store-provider-required.test.ts:59` was
re-verified after the edits: with `brokerProvider` made optional in
`PluginStoreOptions`, `tsc` reports `tests/store-provider-required.test.ts(59,1):
error TS2578: Unused '@ts-expect-error' directive.` — it still errors for the
intended reason, and the probe was reverted.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01PsS7GKpBgupvAKampCVKAK
* fix(arcade): name an authorization attempt by an instant, not by a microsecond
`pending_authorizations.created_at` is a `timestamptz`, which PostgreSQL keeps to the
microsecond; the `Date` the driver hands a caller back carries milliseconds. A row stored
at `…13.407577` reads back as `…13.407Z`, and `recordAuthorizationMint` and
`discardAuthorizationAttempt` matched it with an exact `=` against that `Date`, which
matched zero rows. `claimAuthorizationAttempt` works around this by writing
`date_trunc('milliseconds', now())`, but the COLUMN DEFAULT is `defaultNow()` at full
precision — what drizzle's inferred insert type invites any other writer to take — so a
row written that way could never be matched, no `auth_id` was ever written, and per the
table's own docblock "an attempt holding null can never complete": a silently dead
authorization.
The same two call sites also raised `TypeError: value.toISOString is not a function` from
inside `PgTimestamp.mapToDriverValue` when handed a `createdAt` that had been through
JSON — typed `Date`, carrying a string.
Both now go through `attemptNamed`, which truncates the COLUMN in the comparison as well
as the value, so a row is reachable however it was written, and through `attemptInstant`,
which accepts an ISO string and refuses anything naming no instant with a message that
says which field was wrong instead of a driver `TypeError`.
Fixed in the store rather than in the schema deliberately: `.default(sql\`date_trunc(...)\`)`
would change generated DDL and need a migration on a branch whose migration state is
delicate, and it would still only hold for rows written AFTER it — the matcher holds for
every row, whatever wrote it. Truncating cannot match the wrong row: `actor_id` is the
table's primary key, so there is at most one attempt per actor and `created_at` is the
staleness guard on that one row, not a selector among several; a value naming an older
attempt is still refused, which is what stops a late discard deleting the replacement.
Signature change: `recordAuthorizationMint` and `discardAuthorizationAttempt` widen
`createdAt` from `Date` to `Date | string`. Both are called only from
`server/src/plugins/routes.ts` (lines 2628, 2647, 2683), all three passing a `Date`, and
from `server/tests/arcade-attempt-store.integration.test.ts` and
`server/tests/arcade-completion-transaction.integration.test.ts`. A widened input breaks
no caller. New non-exported helpers `attemptInstant` and `attemptNamed` have no other call
sites.
Audit: the only timestamp column in `server/src/db/schema/plugins.ts` used as an equality
key anywhere is `pendingAuthorizations.createdAt`; every other one (`updatedAt`,
`connectedAt`, `verifiedAt`, `toolsRefreshedAt`, `publishedAt`, `grantedAt`,
`completedAt`) is written, ordered by or range-compared, never matched exactly. No DDL
change, so no migration.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01PsS7GKpBgupvAKampCVKAK
* fix(arcade): read a string with no text in it as no text at all
`textOf` claimed to answer "is this text" and answered yes for `""`. Every
caller that chains it with `??` then ends on the empty field, because `??`
falls through on null and undefined only: a connection row carrying
`provider_user_info: { name: "", login: "maxim" }` yielded NO account name at
all, and the Connected-accounts screen drew a blank where the identity goes on
a connection that is genuinely connected.
The emptiness belongs in `textOf`, not in a `||` at each call site — one of
which would be forgotten. Whitespace-only is the same blank to whoever reads
the screen, so it answers the same. The value itself is returned untouched:
this says whether there is text, it does not reformat the vendor's.
No symbol was added, renamed, resignatured or removed. `textOf` is module-local
(never exported) and has ten call sites, all in this file; behaviour changes at
three of them and is unchanged at the other seven:
- CHANGED L692 `accountName`: `textOf(info?.name) ?? textOf(info?.login)` now
reaches the login. The defect.
- CHANGED L787 `authorizations.status`: an empty `status` reads `"unknown"`
rather than `""`. routes.ts compares it to "completed"/"failed"/"pending",
so the refusal it produces is the same; the value is now honest.
- CHANGED L788 `authorizations.status`: an empty `user_id` reads `null` rather
than `""`. The route's `settled.userId !== attempt.vendorUserId` mismatch
verdict is unchanged either way.
- unchanged L49/L50 `normalizeArcadeError`: `name`/`message` become undefined
instead of `""`. `arcade.ts`'s `arcadeFailure` then answers null for such a
thrown value, and `callTool` returns the same "could not run" sentence,
because `"" !== "tool_authorization_required"` on both sides.
- unchanged L329/L330 `authProviderInstances`: already guarded by `instance &&`,
and `wanted` can never be empty (`authProviderOf` only adds truthy providers).
- unchanged L417 `connectionsFor`: `instances` holds no empty id, so an empty
`provider_id` matched nothing before and is skipped now.
- unchanged L460 `grantStateOf` action, L469 secret key: both already `!x`.
- unchanged L504/L505 token status/status: compared to literals, and `""` is
falsy exactly where `undefined` was.
Same-pattern audit of the whole file: `listApps` had the one other presence
test that admitted an empty value — `typeof toolkit.name !== "string"` let a
toolkit named `""` raise a blank, unconnectable row whose slug is what a URL is
built from. It now skips, through the same `textOf`. The description beside it
keeps its blank default, which is a label and not an identity. `authorize`'s
`url` and id checks already spelled this rule inline and are left alone; scope
arrays are vendor data echoed verbatim and are not a presence test.
Red-green: with the fix reverted, the three new assertions fail —
accountName answered `null` for `{ name: "", login: "maxim" }`, status answered
`""`, and listApps answered `["Github", "", " "]`.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01PsS7GKpBgupvAKampCVKAK
* fix(arcade): read a listing's items off the body Arcade sent, not the page the SDK built
`OffsetPage`'s constructor does `this.items = body.items || []`, so every reader of the SDK's PAGE
OBJECT sees an array whatever the vendor sent. Three guards read items off the page and so could
never fire for the case they were written for:
- `pagedItems` — its `!Array.isArray(items)` refusal was unreachable, which made the `malformed:`
sentence of all four of its callers (`listApps`, `grantsFor`, `authProviderInstances`,
`connectionsFor`) dead text; a listing answering `{"error":"nope"}` resolved as an empty one.
- `authProviderOf` — the one unpaged listing, same read, same dead sentence.
- `createArcadeClient.listTools` — handed the transport the manufactured `[]`, so the transport's
own guard (`arcade.ts`) was dead too and a refresh against a malformed listing would have cleared
an app's recorded actions to none.
C3 already taught `reportedTotalOf` to read `total_count` off the raw parsed body for the same
reason (`body.total_count || 0` hides an absent total). That read is extracted here as `sentBody`
and reused for items as `sentItemsOf`, keeping C3's fallback: an answer with no `body` is read
directly, which is what `admin.authProviders.list` (a plain `APIPromise`, not a page) and every
hand-built stub return.
Audit of the same pattern: `OffsetPage` manufactures exactly `items`, `total_count` and `offset`;
no other class in the SDK manufactures defaults. `total_count` was already handled; `offset` is
never read off an answer in either file (both track their own). Everything else is read off row
objects, which are plain parsed JSON.
RED-GREEN: `server/tests/arcade-malformed-listing-engine.test.ts` serves `{"error":"nope"}` over a
real socket through `ARCADE_BASE_URL` and lets the SDK build its own pages — 6 of its 7 tests
failed before this change ("Received promise that resolved"), all 7 pass after.
`arcade-pagination-engine.test.ts` stays green. The existing prototype-stubbing adapter tests are
structurally blind to this class, which is why a test in that style is not used.
Call sites (nothing renamed, removed or resignatured):
- `sentBody` (new, private): called by `reportedTotalOf`, `sentItemsOf`.
- `sentItemsOf` (new, private): called by `pagedItems`, `authProviderOf`, `listTools`.
- `reportedTotalOf`: signature and behaviour unchanged; callers `pagedItems`, `listTools`.
- `createArcadeClient`'s `listTools` return type unchanged, so `ArcadeActions` is untouched.
Not changed: a JSON body of literal `null` still escapes as the vendor's own TypeError from inside
`OffsetPage`'s constructor; catching it would mean wrapping the page call and swallowing genuine
`APIError`s the authorize path depends on.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01PsS7GKpBgupvAKampCVKAK
* fix(arcade): read a connection's own connection_status before calling it connected
A user-connection row carries `connection_status` — the field is in the key set
this file quotes verbatim at `connectionsFor`, and the SDK declares it
(`connection_status?: string`) — and NOTHING read it. The adapter took a row's
mere EXISTENCE as the answer, so `isConnected` answered `true` and `accountName`
answered a name for a connection Arcade was reporting as expired, revoked at the
provider, pending or failed. Nothing presented as wrong until a tool call failed
later, far from the screen that said Connected, with no way back but guessing.
The seam's own word for `isConnected` is "currently attached to this app at the
vendor", and a finished connection is not that.
THE DESIGN, AND WHICH WAY EACH UNKNOWN FALLS:
- A row is LIVE when its `connection_status` is `"active"` — the one value
measured on a usable connection at the live API, and the spelling this tree's
own engine fixture already carried. An ALLOWLIST of the live spelling and not a
denylist of dead ones: the SDK types the field as a bare string with no union,
so the dead vocabulary cannot be enumerated from here, and a denylist missing
one spelling would go on reading that spelling as connected — the defect itself.
- A row with NO status, or an empty one, still reads as LIVE. The field is
optional on the wire, so a pessimistic reading would answer "not connected" for
every real connection against any engine that omits it: `isConnected` false,
`accountName` null, `revoke` false WITHOUT SPENDING A DELETE. That is exactly
the live defect 28b13e4a fixed, re-entered one field along, and it is the worse
direction — a live connection reading as absent strands somebody with no path
back, where a live-but-unrecognised status at worst sends them round a consent
they already gave. `grantStateOf` already draws this line: not knowing is not
the same as not connected.
- `revoke` ASKS ON EVERY ROW, live or dead, which is why the liveness test is not
a filter inside `connectionsFor`. A dead row is still a row at the vendor;
skipping it would leave it standing while `revoke` answered `false` ("there was
nothing to withdraw"), and a later reconnect would be made beside debris nobody
can see. The two kinds of caller need opposite things from the same row, so the
listing stays whole and only the POSITIVE reads ask the question.
- Folded and trimmed, Amendment 16's client-side fold, as every other comparison
in this file is.
THE GRANTS VIEW NEEDS NOTHING HERE: `grantsFor` answers out of the per-person
tool listing's own `requirements.authorization` (`token_status`, `status`) rather
than out of connection rows, and `grantStateOf` already reads both negatives.
SAME-PATTERN AUDIT OF THE WHOLE FILE — one other vendor field is named in a
docblock and never read, and it is DELIBERATELY left unread: an auth-provider
config's `status` (`"status":"active"` in the verbatim block at
`authProviderInstances`). That listing is the ATTRIBUTION BRIDGE — it says which
vendor a connection row belongs to — not a liveness claim about anybody's
connection. Dropping inactive instances from it would make rows under them
UNATTRIBUTABLE, which does not merely read as "not connected": it also stops
`revoke` from deleting them, so an operator disabling a provider configuration
would silently make everybody's Disconnect a no-op. Whether a person's connection
still works is the connection row's own `connection_status`, and that is now
read. Everything else the file touches already reads its negative:
`grantStateOf`'s secret `met`, `token_status`/`status`, the authorize answer's
`url`/`id`, and `requirements.met`, whose non-reading is argued in place.
RED-GREEN, against `arcade-pagination-engine.test.ts`'s real-socket engine rather
than the prototype stubs, which cannot see this class at all. Red, verbatim:
(fail) ... isConnected and accountName refuse a row whose connection_status
is not live
Expected: false / Received: true
(fail) ... a live row beside a dead one answers for the live one
Expected: "person 1" / Received: "person 0"
Four cases now stand: a dead row is not a connection; a live row beside a dead
one is the one answered for; a row with no status at all still connects; and
`revoke` deletes both and says it asked.
CALL SITES of everything added or resignatured, all of them module-private:
- `connectionIsLive` (new): `broker.isConnected`, `broker.accountName`. Nothing
else, deliberately — `revoke` above.
- `LIVE_CONNECTION_STATUS` (new): `connectionIsLive` only.
- `connectionsFor`: unchanged signature and unchanged answer.
- test helper `connectionRow` gained an optional second argument (one call site,
in its own file), `EngineOptions` an optional `connectionStatuses`, and
`statusOf` is new beside them. No exported symbol changed.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01PsS7GKpBgupvAKampCVKAK
* fix(db): hold pending_authorizations to the unions it declares
`pending_authorizations` declared two TypeScript unions — `holder` as
"person" | "deployment" and `outcome` as the four-name roster — and the
migration that created it, `0057_arcade_provider.sql`, added no CHECK for
either, in the same file that drops and re-adds its sibling's
`brokered_connections_provider_check`. `$type<>` is erased at runtime, so
the database accepted 'llama' as a holder and 'banana' as an outcome and
drizzle handed both back typed as the declared unions: a `switch` over
`outcome` the compiler believes exhaustive was not.
The same table also permitted exactly the two holder/`user_id`
combinations its sibling forbids — a person attempt with a null `user_id`
and a deployment attempt carrying one — because `user_id` is nullable, so
`InferInsertModel` makes it optional and TypeScript raises nothing.
`brokered_connections_holder_check` exists precisely to make those
impossible; the new table now carries the same pair, under the same names.
Two CHECKs, modelled on the sibling and on
`mcp_servers_account_mode_check` for the nullable column:
pending_authorizations_holder_check
pending_authorizations_outcome_check
Migration `0058_pending_authorization_checks.sql`, generated with
`drizzle-kit generate`. `0057`, `_journal.json`'s existing entries and
every existing snapshot are untouched; the journal gains only its append,
and `0058_snapshot.prevId == 0057_snapshot.id`. `drizzle-kit check` passes.
Applied cleanly to a fresh database and to the already-migrated
`openbot_test`. No row can violate either CHECK: the table is created by
0057 on this unmerged branch, both local databases hold zero rows, and its
only writer, `claimAuthorizationAttempt`, already writes `user_id` only
for a `person` holder and `outcome` only from the roster — the CHECKs are
the backstop for a hand-written statement or a cast, not a correction.
No symbol was added, renamed, resignatured or removed. `pendingAuthorizations`
keeps its name, its columns and its inferred types; the `pgTable` call simply
gains the extra-config argument the sibling already passes, so no call site
changes. `bun run typecheck` is green.
Whole-of-plugins.ts audit for the same pattern: the other three `$type<>`
unions are already held — `mcp_servers.account_mode` by
`mcp_servers_account_mode_check`, `brokered_connections.provider` by
`brokered_connections_provider_check`, and `brokered_connections.holder` by
`brokered_connections_holder_check`, whose two branches pin it to the pair.
These two were the only unheld ones. Deliberately left alone:
`mcp_servers.provenance`, which is plain `text` with no declared union and
so makes the compiler no promise, and the many `$type<>` unions in the
other schema files, which are a different finding and a different table.
Tests: server/tests/arcade-attempt-constraints.test.ts, run red against a
database holding 0057's shape (every refusal read "accepted") and green
after 0058.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01PsS7GKpBgupvAKampCVKAK
* fix(broker): name the active vendor in a refused return address, and only to its operator
`brokerReturnUrl` spelled `Composio` as a literal in both refusals and took no
provider, so it could say nothing else. The deployments that can actually reach
the authorize route's return-address check are Arcade ones — `routes.ts` gates
that route on `broker.authorizations`, which only Arcade declares — so the one
audience for this sentence was told its failure involved a vendor it does not
use. The same sentence handed that reader `OPENBOT_APP_URL`, an environment
variable only an operator can set, on two `requireUser` routes.
The refusal is now written twice, the division `BrokerUnconfiguredError`
already draws between a `person` and an `operator`: `message` — what the route
answers with — names no vendor and no variable and says an administrator can
correct the address, and `operatorMessage` names the ACTIVE provider (from
`BROKERED_PROFILE`) and the variable, and is logged rather than answered. Had
the operator's half simply been dropped, a misconfigured address would have
become invisible to the only reader who can fix it.
CALL SITES OF `brokerReturnUrl`, ENUMERATED. Two in `src`, both in
`server/src/plugins/routes.ts`, both `requireUser` and both now passing
`activeProvider` and logging the operator's half through `logReturnUrlRefusal`:
- `POST /servers/:id/connect` (the brokered branch, mint of a connect link)
- `POST /servers/:id/authorize` (the per-action authorization mint)
Seven in `server/tests/composio-broker-seam.test.ts`, updated. No other caller
exists in `server/`, `app/`, `worker/` or any script; `BrokerReturnUrlError`
was constructed nowhere but inside `brokerReturnUrl` itself.
SAME-PATTERN AUDIT OF `broker.ts`. The only other message-bearing strings in
the file are `BrokerUnconfiguredError` (already provider-keyed and
reader-split) and `assertActivatable`, which names the vendor it is actually
refusing, keyed on its `candidate` argument, in a boot-time refusal no person
can reach. Neither is this defect. Comments that said "Composio" where they
meant "the vendor" were corrected where they sat on this code.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01PsS7GKpBgupvAKampCVKAK
* fix(app): look a notice code up in a map that holds only what was put in it
A crafted `?authorized=` on this deployment's own origin broke the Connected
accounts screen. `AUTHORIZATION_NOTICES` was an object literal, so the lookup
walked `Object.prototype`: `?authorized=toString` answered with a function,
`constructor` with another, `__proto__` with an object. None of them is
nullish, so the `?? null` that exists to mean "a code this build does not
know draws nothing" never caught them, and `Record<string, string>` told the
compiler the case could not arise. The page then handed React whatever came
back — an empty red alert for a function, and for `__proto__` "Objects are
not valid as a React child", which took the whole screen down for whoever
opened the link.
Filtering three names would leave the declared type a lie. These maps are now
`ReadonlyMap`s instead: `.get()` answers with what was put in the map and
nothing inherited, and it returns `string | undefined`, so the `?? null` and
`authorizationNoticeFor`'s `string | null` are true statements rather than
claims the compiler takes on trust.
The same shape, keyed by a string from outside this deployment, is fixed
wherever it appears:
- `AUTHORIZATION_NOTICES` and `MARKS` (connected-accounts/index.tsx), keyed by
a search parameter and by a catalogue key.
- `FUNCTION_FOR` (gallery/activity.tsx) and its server mirror
`ACTIVITY_FUNCTION_FOR` (components/headless.ts), keyed by a model's
`report` argument; a borrowed member passed the `functionName ?` guard and
was handed to the grant check as the data function the component reads.
- `EXPORTABLE` (plugins/google-drive-rest.ts), keyed by the mime type of a
file somebody else uploaded and named; a borrowed member made an unreadable
file look like an exportable editor format, skipping the refusal and sending
a function to Google as the export format.
- `FIXES` (intelligence-entitlement-check.ts), keyed by Intelligence's own
error code; a borrowed member won the `||` and left `fix` a function where
the advice sentence belongs.
Red first in every case, against unfixed code, then green.
Symbols: no symbol is added, removed or renamed. Four module-private constants
and two exported ones change type from an object literal to `ReadonlyMap`.
Call sites, all updated: `AUTHORIZATION_NOTICES` — `authorizationNoticeFor`
in the same file and `app/tests/connected-accounts-authorized-notice.test.tsx`
(the only importer; `Object.keys`/`Object.values` there become `.keys()`/
`.values()`); `MARKS` — `markFor`, same file, no other reader; `FUNCTION_FOR`
— `ActivityReportCard` and the gallery entry's `reads`, same file;
`ACTIVITY_FUNCTION_FOR` — one `reads`, same file; `EXPORTABLE` — one read in
`read_file_content`; `FIXES` — one read in `entitlementWarning`.
Deliberately left alone: `AUTH_ROUTE_EVENTS` in server/src/app.ts, which is
the identical pattern on a caller-controlled request path and is being handled
outside this change; and the maps keyed by this deployment's own closed sets
(`DECISIONS`, `DISCOVERY_REASONS`, `PROVIDER_NAMES`, `RUN_LABEL`,
`TRANSPORT_LABEL`, `ORDER`, `browser-activity`'s `labels`, already guarded by
`Object.hasOwn`). The settings page's `?connected=`/`?authorized=` surviving a
reload, noted by a reviewer on index.tsx:209, is untouched.
The one import reordered in connected-accounts/index.tsx is the formatter's
doing: the file did not pass `biome check` before it.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01PsS7GKpBgupvAKampCVKAK
* fix(arcade): answer a mint this deployment could not record, and free the actor
`recordAuthorizationMint` ran unguarded after a SUCCESSFUL vendor mint. A
transient local fault on that one UPDATE threw past the handler into a bodyless
500 and left the claim standing with a null `auth_id` — a row that can never
complete, because the return leg matches on `auth_id`, and that still holds the
actor until its fifteen-minute TTL. One database hiccup turned a successful
authorization into a person locked out of retrying, told nothing, with a live
flow at Arcade that nothing here recorded.
The write is now guarded. The dead attempt is discarded — matched on
`(actor_id, created_at)`, so it can never delete a newer one, and in its own
`try`, so a cleanup that fails too cannot restore the bodyless 500 — the person
is answered a sentence that says the authorization started at the vendor, that
nothing was connected, and what to press, and is NOT sent to the consent screen
behind a flow that could never be completed here. The fact the mint happened is
carried on an `arcade-mint-not-recorded` console line holding the `ar_...` the
vendor answered, so an operator can end that flow; the redirect url, which is
the bearer capability, is not logged, and the error goes through
`reasonWithoutStatement` as `oauth-connection-not-recorded` does.
Discarding here is not the uncertain-mint branch turned around: that row is left
standing because it may record a live flow, while this one provably records
nothing — the id that would let it complete is the value that failed to write.
Same-pattern audit of the whole of routes.ts found one other local write after
an irreversible external effect with no guard: `forgetRule` after
`removeServer` on `DELETE /servers/:id`. By then every account is withdrawn at
the vendor and every row is gone, and a second Remove finds no app, so a fault
retiring the leftover ask-before-write rule answered a completed removal with a
bodyless 500. It is guarded, logged as `shared-rule-not-forgotten`, and still
answered `ok`. Every other vendor-then-local sequence on this file (connect,
confirm, recheck, disconnect, account-mode, the verify and oauth callback legs)
is already inside a catch that authors an answer; those are untouched.
Red-green: both branches were run against the unfixed route first. The mint one
failed on `JSON Parse error: Unexpected identifier "Internal"` (Hono's bodyless
500) and on the attempt row surviving with `authId: null`; the removal one
failed with `Expected: 200 / Received: 500`.
No symbol added, renamed, resignatured or removed in `src`. The only signature
change is the test-local `appAs` helper in
server/tests/arcade-authorize-route.test.ts, which gains a fourth optional
`override: Partial<PluginStore>` argument. All 23 call sites are in that file:
the 20 that existed pass three arguments or fewer and are unchanged, and the 3
new ones (lines 478, 500, 519) are the tests added here.
Deliberately NOT changed: D7 (`recordAuthorizationMint`'s `(actor_id,
created_at)` equality match) and D3 (`discardAuthorizationAttempt` throwing
inside the mint catch at the uncertain/definite branch above) — both are other
agents' findings, and the defect in that catch is left exactly as it was.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01PsS7GKpBgupvAKampCVKAK
* fix(plugins): remove a server at the broker that minted it, not the configured one
`removeServer` read three different answers to "which vendor is this row's" in
one operation: the app slug off the ROW's provenance (through `accessFor`, which
parses the url with the parser belonging to that column), the connection rows off
the CONFIGURED `brokerProvider`, and the revocation off the ACTIVE broker. For a
row provenanced at the other broker — a deployment that switched broker, a
restore, a hand edit — the three disagreed: a non-null FOREIGN slug was handed to
`revoke` and `deleteAuthConfig` at the wrong vendor, while the deletes beneath it
matched no row at all. The removal then reported the connector gone, with every
local consent row still standing and the right vendor never asked.
The row's provenance now decides WHAT is cleared, and the configured provider
decides only WHETHER a vendor is asked. The local half always runs — "removing an
app leaves nobody holding brokered access to it" is about this deployment's own
gate, whichever broker minted the row — so the connection rows of that row's own
provider are read, deleted and audited. The vendor half (`resolveSiblings`,
`revoke`, `deleteAuthConfig`) is skipped where the row's broker is not the
configured one, because there is no client for it here; `vendorRevocationRequested`
then says `false`, which is what that field exists to say and the same answer a
deployment with no broker at all gets.
The same-pattern audit of `store.ts` found one more, in `connectionTokenFor`: the
`grantIsLocal` waiver — the ACTIVE broker's declaration that it checks per action
at the vendor — was read about rows minted at the OTHER broker. On an Arcade
deployment a left-over Composio app was therefore callable with no connection row
of any kind, exempted by a per-action check at a vendor that has never heard of
it. The waiver is now read only about that broker's own rows; a foreign row falls
through to the local gate, which refuses. Every other site that meets both —
`accountRefFor`, `requireServer`, `serverAddress`, `accountsOn`,
`setAccountModeColumns`, `deploymentConnectionFor`, `listServers`,
`addBrokeredApp` — either already guards with `provenance === brokerProvider` or
reads the active provider throughout and never consults the column, and is
unchanged.
No symbol added, renamed, resignatured or removed: `rowProvider` and `rowBroker`
are locals inside `removeServer`, and `brokeredProvenance` is an existing export
of `./access` newly imported here (its other callers are unaffected).
Not changed: `retireConnectionsFor`'s missing `resolveSiblings` (a separate
finding, owned elsewhere), which this change does not touch.
Tests: both defects were watched to fail first — the removal asking Composio to
delete an auth config named `Gmail-…` and leaving two `arcade` rows standing, and
the foreign-provenance call resolving instead of refusing.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01PsS7GKpBgupvAKampCVKAK
* fix(plugins): name the broker this deployment actually runs
Sixteen sentences in `routes.ts` and three in `store.ts`
spelled the literal "Composio" into prose a reader acts on. On an Arcade
deployment every one of them named a vendor this deployment holds no key
for and never dials, and sent the reader to check a key that does not
exist.
Each now reads the ACTIVE provider's name off `BROKERED_PROFILE`, which
is where a provider's vocabulary is already written down once:
- `routes.ts` gains one `activeVendor`, derived from `activeProvider` —
the same binding the brokered reads parse rows with, so the vendor a
sentence names and the vendor a route reached cannot come apart. It
feeds the remove tail, the enable tail, the six key-app connect
refusals, the unreadable-scheme refusal, the no-app-URL refusal, the
consent tail, and the confirm, re-check and disconnect tails — plus
the two `console.error` notes those two tails write.
- `store.ts` reads `BROKERED_PROFILE[brokerProvider].vendor` inline, as
`addBrokeredApp` already does, in the brokered-credential invariant,
the brokered-name collision an administrator meets on Add by URL, and
the retry a person is told to make after a re-check that could not be
made.
Deliberately unchanged: the `Decides<>` roster literals in both files
(type-only and erased, never read by anybody), and
`probeBrokeredConnection`'s own sentence, which says the only connection
check this deployment knows how to make is Composio's — that names the
implementation gap and is true on every deployment.
Call sites: `activeVendor` is a new local constant inside
`createPluginRoutes`, read only by that function's own handlers; nothing
is exported, renamed, removed or re-signatured, so there are no call
sites outside the two files.
Red-green: `server/tests/routes-vendor-vocabulary.test.ts` drives all
fourteen route refusals on both providers — fourteen Arcade cases failed
against the unfixed routes ("Composio would not say whether this account
is connected…"), all 42 pass now, including the assertion that the two
deployments' sentences differ in nothing but the vendor's name.
`server/tests/store-vendor-vocabulary.test.ts` does the same for the
three store sentences, each watched red first.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01PsS7GKpBgupvAKampCVKAK
* fix(plugins): never read an undecided provider as Composio
`activeProvider` answers null for three different reasons and `?? "composio"`
flattened all three into "Composio is active". The dangerous one is both keys
set with OPENBOT_BROKER_PROVIDER unset — a state docs/plugins/arcade.md calls
legal and the one a migrating deployment passes through. In it the plugin store
and the shared-use store were keyed to provider='composio', so every Arcade row
was filtered out of existence: live Arcade connections read as absent and the
accounts behind them could not be revoked. The plugins routes made the same
default a second time (`composio?.provider ?? "composio"`), so with no broker
wired every `arcade://` row was parsed with Composio's parser — the account-mode
switch refused it as "not reached through a broker", the directory called
enabled apps not enabled, connect fell past its brokered branch, and the
operator was told to set COMPOSIO_API_KEY.
The three causes are now separate answers:
- no key set, or both keys with nothing named: legal, no broker is wired, and
the slice the stores read is named by the brokered_connections rows the
deployment actually holds. Asking the rows cannot be wrong about them.
- rows at BOTH vendors while nothing is active: refused at startup, which is
assertActivatable's refusal seen from the other side.
- a named provider whose key is unset: still refused at startup, unchanged.
With no rows of either vendor the name labels an empty set and no row can
appear, because writing one needs a live broker; the comment on storedProvider
says so. The ambiguous case also warns once at startup that nothing is active.
Symbols:
- broker.ts, new: `ProviderDecision`, `providerDecision`. Called from
index.ts, plugins/provider-wiring.ts, tests/provider-undecided.test.ts.
- broker.ts: `activeProvider` kept, same signature, now implemented over
`providerDecision`; no longer called in src (its callers now need the cause),
still covered by tests/provider-selection.test.ts.
- provider-wiring.ts, new: `storedProvider`. Called from index.ts and
tests/provider-undecided.test.ts.
- store.ts: `createPluginStore` now returns its `brokerProvider`, so
routes.ts reads the one name the deployment decided instead of deciding a
second time. Read in plugins/routes.ts; every stand-in PluginStore handed to
createPluginRoutes now carries it (11 test files), and the routes refuse at
construction rather than dispatching on undefined.
Red-green: tests/provider-undecided.test.ts. Against the unfixed ro…1 parent 2fbeb1b commit f1961c3
248 files changed
Lines changed: 75631 additions & 1096 deletions
File tree
- .github/workflows
- agent-pydantic-ai
- app
- src
- components
- gallery
- plugins
- lib
- auth
- copilot
- plugins
- routes
- _authed
- _app
- channel
- admin/plugins
- settings/connected-accounts
- tests
- docs
- plugins
- scripts
- server
- drizzle
- meta
- scripts
- src
- components
- db
- schema
- plugins
- composio-client
- plugin-store
- route-groups
- tests
- fixtures
- support
- tests
Some content is hidden
Large Commits have some content hidden by default. Use the searchbox below for content that may be hidden.
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
35 | 35 | | |
36 | 36 | | |
37 | 37 | | |
| 38 | + | |
| 39 | + | |
| 40 | + | |
| 41 | + | |
| 42 | + | |
| 43 | + | |
| 44 | + | |
| 45 | + | |
| 46 | + | |
| 47 | + | |
38 | 48 | | |
39 | 49 | | |
40 | 50 | | |
| |||
349 | 359 | | |
350 | 360 | | |
351 | 361 | | |
352 | | - | |
353 | | - | |
354 | | - | |
| 362 | + | |
| 363 | + | |
| 364 | + | |
| 365 | + | |
| 366 | + | |
| 367 | + | |
355 | 368 | | |
356 | 369 | | |
357 | 370 | | |
| |||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
1 | 1 | | |
| 2 | + | |
| 3 | + | |
| 4 | + | |
| 5 | + | |
| 6 | + | |
2 | 7 | | |
3 | 8 | | |
4 | 9 | | |
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
5 | 5 | | |
6 | 6 | | |
7 | 7 | | |
| 8 | + | |
8 | 9 | | |
9 | 10 | | |
10 | 11 | | |
| |||
33 | 34 | | |
34 | 35 | | |
35 | 36 | | |
36 | | - | |
37 | | - | |
38 | | - | |
39 | | - | |
40 | | - | |
| 37 | + | |
| 38 | + | |
| 39 | + | |
| 40 | + | |
| 41 | + | |
| 42 | + | |
| 43 | + | |
| 44 | + | |
| 45 | + | |
| 46 | + | |
| 47 | + | |
| 48 | + | |
| 49 | + | |
41 | 50 | | |
42 | 51 | | |
43 | 52 | | |
| |||
61 | 70 | | |
62 | 71 | | |
63 | 72 | | |
64 | | - | |
| 73 | + | |
65 | 74 | | |
66 | 75 | | |
67 | 76 | | |
| |||
284 | 293 | | |
285 | 294 | | |
286 | 295 | | |
287 | | - | |
| 296 | + | |
| 297 | + | |
| 298 | + | |
288 | 299 | | |
289 | 300 | | |
290 | 301 | | |
| |||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
| 1 | + | |
| 2 | + | |
| 3 | + | |
| 4 | + | |
| 5 | + | |
| 6 | + | |
| 7 | + | |
| 8 | + | |
| 9 | + | |
| 10 | + | |
| 11 | + | |
| 12 | + | |
| 13 | + | |
| 14 | + | |
| 15 | + | |
| 16 | + | |
| 17 | + | |
| 18 | + | |
| 19 | + | |
| 20 | + | |
| 21 | + | |
| 22 | + | |
| 23 | + | |
| 24 | + | |
| 25 | + | |
| 26 | + | |
| 27 | + | |
| 28 | + | |
| 29 | + | |
| 30 | + | |
| 31 | + | |
| 32 | + | |
| 33 | + | |
| 34 | + | |
| 35 | + | |
| 36 | + | |
| 37 | + | |
| 38 | + | |
| 39 | + | |
| 40 | + | |
| 41 | + | |
| 42 | + | |
| 43 | + | |
| 44 | + | |
| 45 | + | |
| 46 | + | |
| 47 | + | |
| 48 | + | |
| 49 | + | |
| 50 | + | |
| 51 | + | |
| 52 | + | |
| 53 | + | |
| 54 | + | |
| 55 | + | |
| 56 | + | |
| 57 | + | |
| 58 | + | |
| 59 | + | |
| 60 | + | |
| 61 | + | |
| 62 | + | |
| 63 | + | |
| 64 | + | |
| 65 | + | |
| 66 | + | |
| 67 | + | |
| 68 | + | |
| 69 | + | |
| 70 | + | |
| 71 | + | |
| 72 | + | |
| 73 | + | |
| 74 | + | |
| 75 | + | |
| 76 | + | |
| 77 | + | |
| 78 | + | |
| 79 | + | |
| 80 | + | |
| 81 | + | |
| 82 | + | |
| 83 | + | |
| 84 | + | |
| 85 | + | |
| 86 | + | |
| 87 | + | |
| 88 | + | |
| 89 | + | |
| 90 | + | |
| 91 | + | |
| 92 | + | |
| 93 | + | |
| 94 | + | |
| 95 | + | |
| 96 | + | |
| 97 | + | |
| 98 | + | |
| 99 | + | |
| 100 | + | |
| 101 | + | |
| 102 | + | |
| 103 | + | |
| 104 | + | |
| 105 | + | |
| 106 | + | |
| 107 | + | |
| 108 | + | |
| 109 | + | |
| 110 | + | |
| 111 | + | |
| 112 | + | |
| 113 | + | |
| 114 | + | |
| 115 | + | |
| 116 | + | |
| 117 | + | |
| 118 | + | |
| 119 | + | |
| 120 | + | |
| 121 | + | |
| 122 | + | |
| 123 | + | |
| 124 | + | |
| 125 | + | |
| 126 | + | |
| 127 | + | |
| 128 | + | |
| 129 | + | |
| 130 | + | |
| 131 | + | |
| 132 | + | |
| 133 | + | |
| 134 | + | |
| 135 | + | |
| 136 | + | |
| 137 | + | |
| 138 | + | |
| 139 | + | |
| 140 | + | |
| 141 | + | |
| 142 | + | |
| 143 | + | |
| 144 | + | |
| 145 | + | |
| 146 | + | |
| 147 | + | |
| 148 | + | |
| 149 | + | |
| 150 | + | |
| 151 | + | |
| 152 | + | |
| 153 | + | |
| 154 | + | |
| 155 | + | |
| 156 | + | |
| 157 | + | |
| 158 | + | |
| 159 | + | |
| 160 | + | |
| 161 | + | |
| 162 | + | |
| 163 | + | |
| 164 | + | |
| 165 | + | |
| 166 | + | |
| 167 | + | |
| 168 | + | |
| 169 | + | |
| 170 | + | |
| 171 | + | |
| 172 | + | |
| 173 | + | |
| 174 | + | |
| 175 | + | |
| 176 | + | |
| 177 | + | |
| 178 | + | |
| 179 | + | |
| 180 | + | |
| 181 | + | |
| 182 | + | |
| 183 | + | |
| 184 | + | |
| 185 | + | |
| 186 | + | |
| 187 | + | |
| 188 | + | |
| 189 | + | |
| 190 | + | |
| 191 | + | |
| 192 | + | |
| 193 | + | |
| 194 | + | |
| 195 | + | |
| 196 | + | |
| 197 | + | |
| 198 | + | |
| 199 | + | |
| 200 | + | |
| 201 | + | |
| 202 | + | |
| 203 | + | |
| 204 | + | |
| 205 | + | |
| 206 | + | |
| 207 | + | |
| 208 | + | |
| 209 | + | |
| 210 | + | |
| 211 | + | |
| 212 | + | |
| 213 | + | |
| 214 | + | |
| 215 | + | |
| 216 | + | |
| 217 | + | |
| 218 | + | |
| 219 | + | |
| 220 | + | |
| 221 | + | |
| 222 | + | |
| 223 | + | |
| 224 | + | |
| 225 | + | |
| 226 | + | |
| 227 | + | |
| 228 | + | |
| 229 | + | |
| 230 | + | |
| 231 | + | |
| 232 | + | |
| 233 | + | |
| 234 | + | |
| 235 | + | |
| 236 | + | |
| 237 | + | |
| 238 | + | |
| 239 | + | |
| 240 | + | |
| 241 | + | |
| 242 | + | |
| 243 | + | |
| 244 | + | |
| 245 | + | |
| 246 | + | |
| 247 | + | |
| 248 | + | |
| 249 | + | |
| 250 | + | |
| 251 | + | |
0 commit comments