Repository navigation
Connect many apps through one Composio key; footer button becomes Connect apps - #323
Conversation
Settings > Connectors gets a Composio row. The user pastes one API key and picks apps; the host asks Composio's tool router for a session limited to those apps and mounts its MCP address with DSH's MCP client, so Bots get mcp__composio__* tools. Composio asks for each app's sign-in in the chat. The key is a secret kept with no Bot shell scope, sent only as the x-api-key header to Composio's own host (https, same-host MCP address, no redirects), never written to state.json and redacted from errors. The connector code is now a table of definitions shared with GitHub.
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b974d27b80
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| return { userId, apps } | ||
| }, | ||
| async mountConfig(token, options, saved) { | ||
| const { url } = await composioSession(token, { api: options.api, userId: saved.userId, apps: saved.apps }) |
There was a problem hiding this comment.
Whenever the host starts, the user retries, or the app list changes, this creates a fresh remote session, but unmount only disposes the local MCP fiber and neither stores nor deletes the returned session_id; the preceding connection check also creates another discarded session. Because Composio sessions persist and do not expire, routine use accumulates permanent orphan sessions and can eventually exhaust or clutter the account. Return and persist the session ID so it can be reused, or delete superseded validation/mount sessions.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in b346db6 (code in 58b221f; the rest is a merge of main, with the generated patches regenerated).
- The Composio session is now made once per connect or Choose-apps change and kept in
state.connectors.composio.session({ id, url, apps }, never the key). The key check and the mount share it, so connecting makes one session, not two. - Mount and Try again reuse it only while its address is still https on the API host, the apps match, and
GET /tool_router/session/{id}still finds it. Otherwise a new one is made and kept. - A superseded session is deleted with
DELETE /tool_router/session/{id}: the old one after Choose apps or a new key (using the old key), the new one if saving it failed, and the kept one on Disconnect. The delete is best effort. A failure or outage never fails the action, and a 404 counts as already gone. The id is validated and URL-encoded, and the key goes only in thex-api-keyheader withredirect: 'error'. - Also fixed a race I found: a secret change during connect started a mount before the connection was saved, which could create a stray session.
- Tests:
core/test/connectors.test.jscovers one session per connect, reuse on Try again, recreate when Composio no longer has it, delete on Choose apps (and no delete when the new list is refused), delete on Disconnect (also when the delete fails), and a new key. The state file and errors never contain the key.
…ccount menu The footer button now opens Connectors, so the shell's plugin panel needs another way in that does not require classic Agent mode.
Birfy
changed the title
|
Maintenance review at tip 321f6e4 (read-only; another agent owns this branch). Footer requirement is met: the bottom-left button is now Connect apps and calls |
…delete it when superseded The key check and every mount each made a session that was never kept or deleted. The session id and address are now kept in state (never the key), reused while Composio still has them, and deleted (best effort) on Disconnect, Choose apps and a new key.
Birfy
left a comment
Birfy
left a comment
There was a problem hiding this comment.
Post-merge review of head b346db6 (merged as be97ef3 at 18:45 Shanghai time; I read the diff and the merged core/src/host/connectors.js). Posted from the author's account, so this is not an independent PASS.
Verdict: no blocking findings. The Codex P2 (orphan Composio sessions) is fixed in this head: one session per connect or Choose-apps change, kept in state.connectors.composio.session, reused only when it is https on the API host, for the same apps, and still alive; superseded sessions are deleted (old key first); a session made while the connection was disconnected or re-keyed meanwhile is deleted again. Footer button reads Connect apps → openConnectors(), and the account menu has Plugins → selectPanel('plugins'); zh strings exist for all new copy.
Checked: key kept with scope [] (no Bot shell), sent only as the x-api-key header to the API host and to an MCP address of the same origin (https, redirect: 'error'); errors go through redactSecrets; connector-* endpoints are shared-blocked in authz.js; composio reserved so a capability cannot shadow it; composio: false hides the row; the key is not in state.json.
Follow-ups (not blockers, for Danyang to decide):
state.jsonnow holds the Composio session id and MCP URL (not the key). The PR text says the key is never stored, which is true, but the URL is a session address; keep it out of any Bot-readable file if Composio ever treats it as a bearer.- Every Bot gets
mcp__composio__*(and GitHub's tools the same way), including a Bot shared with a friend under a per-Bot grant: the friend then reaches the owner's connected apps through that Bot, limited only by DSH's normal tool approval. The PR lists per-Bot app grants as out of scope; it deserves its own issue because it touches the default-deny grant rule. - If Composio is down at start,
composioSessionAlivethrows and the connector shows an error (Try again) rather than reusing the stored session; acceptable, just noting it. - Not exercised by tests: clicking the footer button / Plugins row and a real Composio account (source-level tests only, as the PR said).
…r get them (ThinkFlowLab#346) Fixes ThinkFlowLab#335 ## What changed Connector tools (`mcp__github__*`, `mcp__composio__*`) were mounted for every Bot and Session, including a Bot shared with a friend. Now: - **Per-Bot switch, default off.** `bot.connectors = { github?, composio? }`; absent means off. Switches: Settings → Connectors → **Choose Bots** (a switch per local Bot; the row says "On for N of M Bots") and a **Connectors** card in each Bot's settings. zh strings added. - **Shared Bots never get them**, whatever the switch says. `sharedWith()` (reach.js) is the one definition: a grant, or a current room with a friend's Bot, another person, another owner's Bot or a room code. It is read on every call. A grant also clears the Bot's switches, and turning one on for a shared Bot is refused naming who it is shared with. - **Enforced in the tool path, not the UI**: a `ctx.tools.guard` refuses every connector call from a Bot Session (own chat, group Session, earlier part; sub-agents follow their parent), and `restrict({deny})` removes the tools from that Agent's list (on create, on `resolveAgent`, on a switch/grant change, on `tools/change` when a connector mounts later). The DSH Agent and plain DSH Sessions (owner-only) are untouched. - **Upgrade behaviour: every existing Bot starts off**, including ones that used the tools (connectors are new since ThinkFlowLab#323). New/duplicated Bots start off; **Disconnect** turns the connector off for every Bot. `bot-connectors` is `SHARED_BLOCKED`. - **Composio session address is no longer in `state.json`.** Only `{ id, apps }` is kept. Composio's reference lists `mcp.url` as required in the create and `GET /tool_router/session/{id}` answers (`x-api-key`), so the mount asks for the address by id + key, accepts only https on the API host, and keeps it in memory. Composio's docs show the address always paired with the key header and call the exported url+headers config a secret, but never say the address alone cannot authenticate, so it is not stored. An older record's `url` is rewritten away at start; the in-memory address is redacted from connector errors. Docs: core-host (rule, upgrade, session address), core-client, team, README en/zh. Changelog fragment: `changelog.d/335-connector-bots.security.md`. ## Verification at the tip Tip SHA: 8242518 Ran: `ARCH_DOCS_BASE=origin/main TZ=UTC npm run verify` (Node 24) → exit 0: core 889/889, social 230/230, tree 56/56, timeline 80/80, `.github/scripts` 86/86, interop 37/37, live-e2e 16/16; arch-docs OK; `ds-bot-patches check` OK; `build-client --check` OK. New: `connector-access.test.js` (10: default off, toggled-on local Bot gets only its connector, shared Bot via grant/room gets none even with the flag forced on, sub-agents, late-mounting connector, disconnect, duplicate), `connector-bots.test.js` (6), connector session tests (+4, 6 updated), notices test (+1), authz and install order rows. The new test files fail on current `main` (the module does not exist there). Not run: the `dev/bench.sh` smoke test (needs a DSH checkout at `../deepseek-harness`, not on this box); live Composio/GitHub calls (no key; the session-address flow is tested against a stand-in for the documented API); a visual check of the new rows in a browser. Reviewer notes: T3 expected (`reach.js`, `authz.js`, host tool path). `core/` and `patches/ds-bot/` are generated together; new `core` files are stored whole in `patches/ds-bot/files/`.
…ess (ThinkFlowLab#1081) The case read `state.json` once right after the mount. Measured on `main`, the mount and the save that drops ThinkFlowLab#323's legacy `session.url` are two start-up paths and the mount comes first: under a 400 ms delay on every `state.json` write, the read was dirty 20/20 times and the file settled 401-422 ms later, with no store refusal (`not saving the Bot team`, `has closed`, `open in another dsh`: 0/20). The save is not dropped; the read is early. On the same host without the delay the case is clean 20/20, which is why the CI runner was the one that lost the race. The case now waits for the observable state — the file no longer carrying the address — with a 3 s deadline, no fixed sleep. The three assertions are unchanged. On timeout the message carries what the file still says and the store's own read-only lines, so a genuinely dropped save stays distinguishable from an early read (the product-side question the issue raised). Every `state.json` write in the case is delayed 400 ms on purpose, so the window is deterministic instead of luck: with the wait removed the case reds 20/20, with it 20/20 green. Docs-not-needed: a test-only change; no product or architecture behaviour moves.
Fixes #311
Also folds in #322 (Danyang confirmed): the bottom-left sidebar footer button now reads Connect apps (连接应用) and opens Settings > Connectors instead of the shell's plugin panel. The plugin panel stays reachable from a new Plugins (插件) row in the account menu, so classic mode is not needed.
Settings > Connectors gets a Composio row: paste one Composio API key, pick the apps (Gmail, Google Calendar, Slack, Notion, Linear and more, or type any Composio app name), and every Bot gets the tools as
mcp__composio__*. Composio shows a sign-in link in the chat the first time a Bot needs an app. Choose apps changes the list later without the key.How: the host asks Composio's tool router for a session limited to the picked apps (workbench off) and mounts its MCP address with DSH's own MCP client, next to GitHub. The connector code is now a small table of definitions shared by GitHub and Composio.
Secrets:
COMPOSIO_API_KEYis kept with scope[](no Bot shell), sent only as thex-api-keyheader to Composio's own host (https, MCP address must be on the API host, no redirects), proved by creating the session before it is saved, never written tostate.json, redacted from errors.connector-appsis shared-blocked like the other connector writes.composiois a reserved capability name;composio: falsehides the row.Ran at tip:
ARCH_DOCS_BASE=origin/main TZ=UTC npm run verify(green, core 852 tests incl. new Composio tests for the request, key handling, mount, apps change, refusals, https and same-host checks). Not run: clicking the footer button and the Plugins row in a running app (source-level tests only), a real Composio key and account, smoke test, a visual check in the browser (only a server-side render of the row).Not in this PR: a per-app approval gate for write actions (tool approval stays DSH's normal one), per-Bot app grants, Composio in-app sign-in buttons.