Give a Composio app the kind of connection it actually needs - #481
Conversation
4008ffe to
21218e2
Compare
mxmzb
changed the title
guidovizoso
left a comment
guidovizoso
left a comment
There was a problem hiding this comment.
Review of the Composio brokered-connection work — 13 findings across the transport, store, routes and the new settings surfaces, left inline.
Worth blocking on: the OAuth-callback log at routes.ts:1992 can put the encrypted refresh-token envelope into stdout; store.ts:5176 records any vendor error as "your key is wrong" and revokes the account it just created; and store.ts:2934/:3099 let auth_scheme diverge from the standing Composio auth config, which in one direction erases a recorded refused-key verdict on page load.
One thing I could not verify, flagging rather than asserting: composio-adapter.ts:3229 — deleteAuthConfig awaits vendor.authConfigs.delete(...) and discards the reply, while the whole of withdrawalDeclined (:1315) and revokeAccount (:4481) exists to argue that not throwing is not the same as having been done, for the connected-account delete. ComposioVendor.authConfigs.delete is typed Promise<unknown> (:2058), so the reply is unreachable by construction. If AuthConfigDeleteResponse carries a success field the way ConnectedAccountDeleteResponse does, this is the same asked-vs-done gap one level up, with removeServer deleting the app's row behind it. @composio/client is not installed in my checkout and no test stubs a reply body for that call.
Checked and sound, so it need not be re-walked: migrations 0036-0038 match the drizzle schema column-for-column with an intact snapshot/journal chain from 0035; db/query-failure.ts scrubs on every reachable path; credential values never reach an audit row (Object.keys(values).sort()), a route error, or a read-back, and the browser's copy is genuinely erased on settle; per-person authorization takes the subject from context.var.actor.id only on all four brokered routes, and requireAdmin is correctly returned inside all five admin handlers; prototype pollution is closed on both the form and the route (Object.create(null) plus a published-name whitelist, and status cannot be overwritten); no open redirect or CSRF on the callback; the audit.ts empty-filter sql\false`and strict-UUID-cursor fixes are both correct;connectionOf`'s classification precedence matches its tests, including the deliberate NO_AUTH-beats-everything rule.
… dials it, and document the setting before anything can use it The setting, the dependency and the operator's page, ahead of every commit that needs one of them. THE KEY IS A SETTING RATHER THAN AN ENVIRONMENT LOOKUP AT THE CALL SITE, for the reason every other credential here is: a deployment that has not set one has no broker at all, and that is a state the whole feature has to be able to answer about rather than discover at the first call. `composioApiKey` is what `index.ts` builds the client from, and what every "is Composio configured" answer on a screen is drawn from. AND THE SDK SHIPS IN THE IMAGE, which is not automatic. The Dockerfile prunes what the runtime does not import, and `@composio/core` is imported from exactly one file — so an image built before this would boot without it and fail at the first brokered call rather than at build time. `scripts/composio-smoke.ts` is named individually in the server tsconfig for the same reason: it ships, it is what `docs/plugins/composio.md` tells an operator to run, and the root `scripts/` directory is in no workspace. THE DOCUMENTATION COMES FIRST BECAUSE NOBODY CAN REACH THE FEATURE YET. What it describes is the setting and the shape of the thing, so that the commits behind it are read against a stated intent rather than inferred from a diff. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…brokered account a person holds Three migrations and the columns they add, before anything reads them. WHAT AN ACTION DOES AND WHICH REVISION IT IS. `mcp_tools` gains `effect`, `destructive` and `version`. The first two are what a vendor said about its own action, recorded at listing time rather than re-derived on every read from a name; the third is what Composio requires on an execute and rejects `latest` for, so an action with none recorded cannot be called at all. WHICH APP A PERSON HAS CONNECTED, AND HOW WELL THAT IS KNOWN. `composio_connections` is keyed on the person and the app, and carries `verified`, `verified_at` and `probe_action`. Those three are ONE fact in three columns and the schema says so at length: `verified` alone means several different things about somebody's key, and the action a check was spent on — null where none was — is what separates "the app published nothing safe to try" from "a check ran and did not come back clean". AND HOW THE APP ITSELF CONNECTS. `mcp_servers.auth_scheme` holds the vendor's own scheme literal for the authorization config this deployment created, which is what every later connection against it has to keep using. 0037 backfills it to `OAUTH2` for every existing brokered row, because managed OAuth was the only config this deployment ever made, and backfills the verification pair from `connected_at` for the consent rows that predate the columns — where the vendor's own yes at its own screen is the evidence and no call was ever spent. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ow is reached in one place
The transport, and the one answer every caller of it reads.
A BROKERED CALL RUNS IN THE ASKER'S OWN ACCOUNT, which is the whole point of the
connector: the deployment holds one key, Composio keeps people apart by an id we
send, and what a Bot sees is what that person can see and nothing more. The app
travels WITH the call rather than beside it, resolved from the url the row is
dialled at, so the app a person was gated against and the app the call runs in
are one fact by construction rather than two spellings agreeing.
AND THE ACTION IS CALLED AT THE VERSION IT WAS LISTED AT. Composio will not
execute without a specific one and rejects `latest`, so there is nothing to fall
back on — and a version invented here would be a call against some other
revision of the action, whose arguments and behaviour are not the ones that were
listed, classified and granted. It travels under a reserved key the transport
strips before anything reaches the vendor, and asserts that it did.
RESOLVING AND SUCCEEDING ARE DIFFERENT THINGS, AND THE VERDICT SAYS WHICH.
Composio answers a 200 carrying `successful: false`, throws for a transport
fault, and can answer a shape this deployment's package cannot parse. Read as
one, all three were "the vendor refused" — which is a verdict a caller then
writes down about somebody's credential. {@link ActionAnswer} separates the
vendor having ANSWERED from what it said, so only a call that can be shown to
have run may say anything about a key.
`accessFor` IS THE ONE ANSWER TO WHICH PROTOCOL, WHOSE CREDENTIAL, WHICH APP AND
WHOSE NAME THE TRAIL RECORDS. Three call sites used to derive their own from
whichever field was nearest, which was complete while every server had a frozen
catalogue entry or was somebody's MCP endpoint. A Composio app is neither, and
made all three wrong at once.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…hape in the one file allowed to know it Nine methods, and the adapter that satisfies them. A NARROW PROJECTION RATHER THAN THEIR CLIENT. Nine methods is a shape a test satisfies with an object literal, so every test about enablement, connection and revocation is a test about this deployment's logic and none of them reaches the network. The vendor's client would drag its constructor, its retries and its schemas into each of those, and the first thing every one of them would do is find a way not to dial. AND WHAT THE ADAPTER OWES IS SAYING WHAT WAS ACTUALLY DONE. Not throwing is not the same as having been done: a withdrawal whose reply says `success: false` deleted nothing, a soft-delete without `revoke_on_delete` leaves the grant live at Google while this deployment records a revocation, and a listing read to the end of one page is not the listing. Each of those is answered rather than assumed, and where the vendor publishes nothing to answer it with — the auth config delete carries no verdict at all — that is written down where the next reader will ask. A ROW THAT COULD NOT BE READ IS PARTITIONED, NOT THROWN ON. An unreadable account or config used to refuse everything behind it, permanently, because the next read is the same shape and nothing anybody can press changes it. Every caller now acts on what it can name and reports the rest — except the one that would act by CREATING, where finding nothing of ours in a listing we could not read is not the same as finding nothing. THE SMOKE SCRIPT IS AN OPERATOR'S DIAGNOSIS AND NOT A TEST. It is what `docs/plugins/composio.md` points at when the key is wrong, and it fails least usefully in exactly that case unless it is written to say so. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…r key can and cannot establish The store and the routes: everything between an administrator pressing Add and a Bot making a call as somebody. WHAT AN ENABLE RECORDS IS WHAT STANDS AT COMPOSIO. The authorization config is created first, so a failure leaves no row an administrator can see, grant and press Connect on with nothing at the vendor for any of it to attach to — and the scheme written on the row is the scheme that config was created AS, never what today's catalogue resolves. The config is reused whatever it holds, so a second press of Add can only record a word beside an object it did not move. A CONNECTION IS THE VENDOR'S ANSWER AND NOT A BROWSER ARRIVING BACK ON A PAGE. The return trip from a consent screen carries nothing signed, so the confirm goes and asks; a `false` from Composio deletes the row rather than leaving one that would pass every later gate for an account nobody has. AND A CHECK OF A TYPED KEY SAYS ONLY WHAT ITS ANSWER SUPPORTS. Composio accepts a key without ever trying it, so a real call is what separates "connected" from "works" — but the envelope that call comes back in carries no status and no error code, so a rate limit, a scope one action wants and a credential the vendor rejected are one answer. The four outcomes are named for what was observed, the account is never destroyed on a failure, and a name beside `verified: false` says a check was spent and did not come back clean rather than accusing anybody's key. A FAILED QUERY IS NOT QUOTABLE. drizzle puts the statement and every bound value in its own message, and along the credential path those values include the encrypted envelope. `db/query-failure.ts` is the one door every site that copies a caught message reads through — the store, the routes, and `credentials.ts`, where the first leak wrote one into an audit row that is append-only by trigger. EVERY BROKERED ROUTE TAKES ITS SUBJECT FROM THE SESSION AND NEVER FROM THE BODY, answers a broker failure in the broker's own words rather than as a bare 500, and returns its admin guard rather than evaluating it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…nistrator the one that enables the app and grants its actions The four surfaces, and the one row two of them share. BROWSING AND ENABLING. Composio's directory is the vendor's and is too long to read, so the picker searches it, debounces, and states the size of the decision before it is made: an app is not one tool, and "167 actions" is what says so. An app already here says Added rather than offering a second press. GRANTING PER ACTION, PER BOT. An app's actions arrive split by what a boundary would see, because the difference between a read and a write is the difference between a Bot that can look at somebody's mail and one that can send it. ONE PERSON'S ACCOUNT, DRAWN ONCE. The connector's admin page and that person's own connected-accounts page both draw it, and drew it twice — forty lines apiece, comment for comment — with the same two defects in both copies, each of which had to be found twice to be fixed once. What genuinely differs stays an argument: which Bots can read you as you, and where a consent flow returns to. What the row SAYS is one answer and lives in one place. AND WHAT IT SAYS TURNS ON THREE FIELDS READ TOGETHER. `verified` alone means several things; the action a check was spent on is what separates an app that published nothing safe to try from a check that ran and did not come back clean, and whether the app publishes something TODAY is the separate question the Re-check button is gated on. Asking the record that second question is what left a key nothing was ever spent on unable to ever have anything spent on it. A SCREEN WHOSE READ FAILED SAYS SO RATHER THAN DEFAULTING. Every fact a brokered row draws comes from the connections read, so a page that gated only on the plugins read drew "not connected" for accounts people hold — and withdrew the two controls that could have corrected it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
19795d8 to
8ab7451
Compare
… main added FOUR CONFLICTS, AND ONLY ONE OF THEM IS A TEXTUAL ONE. THE MIGRATION NUMBER COLLISION IS THE REAL MERGE. Both sides branched off 0035 and both wrote an 0036: main's `0036_routine_sweeps` and this branch's `0036_composio`. Two files at one index is not something either side can be right about, so this branch's three move up — `0037_composio`, `0038_composio_schemes`, `0039_composio_probe_action` — and main's keeps the number it already carries on main. THE ORDER THAT ACTUALLY DECIDES ANYTHING IS THE `when`, NOT THE FILE NAME, which is what `migration-journal.test.ts` exists to say. Every stamp on this branch is later than main's 0036, so the renumbering follows the order drizzle would have applied them in anyway rather than imposing a new one. AND A SNAPSHOT IS THE WHOLE SCHEMA AT ITS POINT, not the diff that produced it. The three snapshots on this branch were generated before `routine_sweeps` existed, so each one now carries that table too and the first re-points its `prevId` at main's 0036. A snapshot missing a table main added is not a cosmetic gap: the next `drizzle-kit generate` diffs against it and emits a `CREATE TABLE` for something the database already has. VERIFIED BY MIGRATING A DATABASE FROM ZERO. All forty apply cleanly in order, and both sides' objects are there at the end — `routine_sweeps` from main, and `composio_connections` with its verification columns and `mcp_servers.auth_scheme` from here. The other three conflicts are two import lists that both sides appended to, and main's `0036_snapshot.json`, which is main's file and is taken unchanged. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…r the merge The docblocks on `composio_connections.verified`, `verified_at` and `probe_action` name the migration that adds and backfills them, and the merge moved it: main took 0036 while this branch was open, so the schemes migration is `0038_composio_schemes.sql` rather than `0037_`. In a file whose docblocks are the specification for which rows a reader may trust, that pointer is the whole of what says a backfilled `verified: true` was earned by a consent screen rather than by a call — so a number sending the next reader to the wrong file is worth the one-line correction it costs. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
guidovizoso
left a comment
guidovizoso
left a comment
There was a problem hiding this comment.
Re-reviewed at 1b582321. All 16 findings from my previous pass are genuinely fixed — I checked each at the new head rather than reading the fix commits:
reasonWithoutStatement at both callback log sites (and two more); the answered/complained/unreachable split so a rate limit no longer revokes the account it just made; asksForArguments walking composed schemas; the only: "a row that is still there" guard closing the re-check resurrection; both halves of the auth_scheme write-once rule; find-first-partition-after on auth_config_details; the de-dup hoist; reachedAs branching on scheme; pending and error branches on all three account surfaces; the per-row adding set with mutation-level onError; the NO_AUTH filter; typeof body?.slug === "string"; and the docblocks now pointing at 0038.
The migration renumber is clean. Journal idx matches every tag prefix, the prevId chain links 0034→0039 unbroken, main's 0036_routine_sweeps snapshot is byte-identical so the merge did not rewrite it, and the cumulative structural diff 0036→0039 shows exactly the intended deltas and nothing else. migration-journal.test.ts 7/7, server typechecks, the Composio suites 393/393, the changed app components 86/86.
Four new findings from the fix pass, labelled inline by priority:
- 🔴 Fix before merge —
store.ts:5891(nomcp.account_connectedon the complained path) andadmin/plugins/$key.tsx:358(the connections-error branch blanks the whole admin page). - 🟡 Judgment call —
composio-adapter.ts:4005({ mode: null }slips the new partition). Cheap enough that I would take it now. - 🟢 Nice to have —
composio-adapter.ts:1192(the de-dup hoist is now first-copy-wins).
One thing the review surfaced that I am not passing on as a finding: store.ts:3204 gates the scheme write on configured !== "standing" && connections.length === 0, which looked like it contradicted its docblock. It does not — the block argues the conjunction deliberately ("SO BOTH CLAUSES STAND, AND EACH CLOSES WHAT THE OTHER CANNOT"), and the residual stuck-scheme case is repairable by the remove-and-re-add the same block names. Working as documented.
… row rather than a page
Four things Guido found on the fix pass, and the failure the suite was reporting
as one unnamed test.
- `connectBrokeredWithFields` filed `mcp.account_connected` below the outcome
branch, and every way out of that branch is a throw. A rate limit or one
refused scope therefore left a live account at Composio, a `composio_connections`
row and an `mcp.connection_verified` row saying the check did not come back
clean, with nothing saying the account had ever been connected — so the
`mcp.account_disconnected` row filed later had no counterpart to pair against.
The read of the previous row and the connect row both move above the branch,
where the account already exists whichever way the check went.
- The connector's admin page collapsed to one sentence whenever
`/api/plugins/connections` answered 500 — for every plugin, a deployment-token
server included — taking the tools, the grants, Refresh and the switch that
removes the server with it. The withholding is now per row and covers both rows
that read that endpoint: the brokered account and the `user-oauth` one, whose
Connect button was derived from the same defaulted `false`.
- `{ mode: null, fields: {…} }` is an object and not an array, so it passed the
mode partition's shape test and was never collected as unreadable. With the
wanted mode absent from the rest of the list the vendor-shape refusal was
skipped and an administrator was told the app had dropped the scheme, against a
listing that reads identically on the re-add.
- `readableConfigs` claimed an id for whichever copy the listing handed over
first, so an unreadable first copy left a readable second copy unrecorded and
`deleteAuthConfig` left a config standing that the other ordering would have
deleted. One slot per id now holds the best copy, in the first sighting's place.
And `plugin-store.integration.test.ts` issued its four ownership-guard reads
through one `Promise.all` against a two-connection pool, where Bun's postgres
client collided the five-parameter `composio_connections` read with the
one-parameter `agents` read on a prepared-statement name. The guard threw out of
the file's first hook, which took all 134 tests with it. They run one at a time.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
guidovizoso
left a comment
guidovizoso
left a comment
There was a problem hiding this comment.
Approving. All four findings from the last round are correctly fixed, and I verified each at 2c18f05b rather than reading the fix commit.
One cleanup to fold in before merge — the fix commit accidentally committed a compiled Python artifact:
A agent-langgraph-agui/src/__pycache__/main.cpython-313.pyc
It is a binary build output, and .gitignore currently has no Python patterns at all — no __pycache__, no *.pyc, no venv — so this is not a one-off slip and will keep recurring in agent-langgraph-agui and any other Python package here. Worth one commit doing both:
git rm --cached agent-langgraph-agui/src/__pycache__/main.cpython-313.pyc
printf '__pycache__/\n*.pyc\n' >> .gitignore
Not blocking the approval — it touches nothing that ships — but please do not merge it in.
The four fixes
store.ts — mcp.account_connected on the throwing path. Moved above the outcome branch with the existing read that reconnected depends on. The placement argument is the right one: the row asserts the ACCOUNT was made, which connectWithFields has already established, so filing it ahead of recordBrokeredConnection stays true even if that write fails.
composio-adapter.ts:1154 — readableConfigs. Rewritten from a Set to a slot map where a readable copy replaces an unreadable one in place. I traced all five paths: both orderings of a repeated id now answer configs=[ac_1], unreadable=[], and a second unreadable copy no longer counts one config twice. That is order-independent by construction rather than patched for the one ordering I happened to report.
composio-adapter.ts:4047 — { mode: null, fields: {…} }. Partitioned before the scheme comparison, with the hole-vs-unreadable distinction spelled out. Correct.
admin/plugins/$key.tsx — the blanked page. Withheld per row rather than per page, page-level early return gone, tools/grants/Refresh/Remove all intact. It also covers the user-oauth row, which was living off the same defaulted false — that one was mine to have caught in the first pass and I missed it.
All four carry tests.
Verification
4533 pass. Composio and plugin suites green — adapter, connections, access, connection-kinds, transport, plugin-routes (441), the integration suites (203), brokered-account-row (42). bun run typecheck clean across all four packages; biome lint --error-on-warnings and biome format both clean.
I also ran drizzle-kit migrate against a fresh empty database, so the renumbered 0037–0039 chain is exercised end to end rather than only read out of the journal. Applies cleanly.
The 8 remaining failures in a full bun test are pre-existing environment noise — missing modules in agent-bot, agent-mastra, agent-langgraph and desktop, sub-packages that need their own installs. None are touched by this PR.
For the record
Across the three rounds this PR closed 20 findings: the 16 from the first pass — including the refresh-token envelope reaching stdout, the probe that read any vendor error as a bad key and revoked the account it had just created, and the re-check that could resurrect a disconnected row — and the 4 from the fix pass. Nice work on the second round in particular: the readableConfigs rewrite and the per-row withholding both generalised past what was reported instead of spot-fixing the named case.
…ttern for it `agent-langgraph-agui/src/__pycache__/main.cpython-313.pyc` was committed by the previous change. It is a compiled artifact of a source file already in the tree, so it ships nothing and says nothing, and it is untracked here rather than deleted: the interpreter writes it again the moment that package is run. The slip is the symptom. `.gitignore` carried no Python pattern at all, and eleven of the agent packages in this repo are Python — so every one of them leaves `__pycache__` beside its source once somebody runs it, and any `git add` wide enough to catch a whole tree picks it up. The environments are the more expensive half of the same gap: `.venv` is hundreds of megabytes of platform-specific binaries that would be useless to anybody who cloned them, and both its spellings are in use across these packages depending on which tool made them. No tracked file matches any of the added patterns, so nothing that was in the repository leaves it with this. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Done in 336ce94 — thanks for catching it, that was my Untracked rather than deleted, since the interpreter writes it again the moment that package runs. And you were right that it is not a one-off: eleven of the agent packages here are Python, so Went slightly wider than the two lines, on the same reasoning: Both Checked that no tracked file matches any of the added patterns, so nothing that was in the repository leaves it with this. |
2 participants
Composio apps used to work only when Composio held its own credentials for them, which was true of 121 of the 1540 apps in the catalogue. This teaches the deployment that an app can need one of five different things, and gives four of the five a way through.
What an app can be
Every app now resolves to one connection kind, decided from what Composio publishes about it:
Enabling an app now creates whatever that kind needs, instead of always asking Composio for a credential of its own.
Connected, checked, and works are three different facts
composio_connectionsgainsverified,verified_atandprobe_action, andmcp_serversgainsauth_scheme.A vendor accepting a key is not the same as the key working — Composio does not grade what it is given. So after a key connection is made, one argument-less read-only action is spent against the account that was just created, and the row records whether that happened, when, and which action it spent. The four states the pair can hold are each distinct and each mean something different to a person reading the screen:
That last one is the state that matters most and the easiest to lose. A page load cannot erase it, a re-check that spends nothing cannot overwrite it, and an unreachable vendor is a fourth thing that is never recorded as a verdict about anybody's key.
The credential itself
What somebody types is never persisted by this deployment. It goes to Composio and nowhere else: no secret column, no vault row, no read-back path. The audit trail records the names of the fields that were filled in and never their values, and the browser erases its own copy once the request settles.
Review
This branch went through five review rounds with unbiased reviewers, three fix cycles, and a convergence audit. Every behavioural fix carries a test that was watched failing against the unfixed code before the fix landed.
Worth calling out for reviewers, because they were found here rather than in the original work:
Three structural levers were added after the same class of defect survived three rounds: adding a member to either connection-state vocabulary is now a compile error at every site that must decide, refusal assertions name which refusal they expect and fail if a sibling is produced, and three cross-layer rosters count against the real declarations rather than against test literals.
Migrations
0036_composio.sql,0037_composio_schemes.sql,0038_composio_probe_action.sql, renumbered off main's0035. Verified applying from an empty database. Neither is reversible and neither is idempotent, matching the repo's convention.Operator note: existing
openbotandopenbot_testdatabases are stuck on an older chain whose last timestamp is later than main's0035, sodrizzle-kit migratewill silently skip main's0029–0035on them. They need recreating from empty rather than migrating forward.Proven live
linear_mcpandhackernewsenable and connect through the real Composio endpoint;docusignrefuses with its own sentence. The offline boundary probe confirms the payloads this sends are accepted by@composio/core0.18.1 on every path it takes.Known follow-ups
Recorded rather than fixed here, with the reasoning in each case. Two are security-shaped and pre-existing, in the skills routes rather than this path: deleting a skill that does not exist answers success and writes an audit row, and the same endpoint distinguishes unknown from deployment-owned from another person's skill. Four other screens hold a credential in mutation state the way this one did before it was fixed.