diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 6d3542905..5a01309cc 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -864,6 +864,31 @@ jobs: - name: Test the Composio ops helpers run: scripts/ci/run-scoped-suite.sh "composio ops helpers" openhuman,tinycortex,composio harness::composio::ops_helper_tests + # Issue #820 — which connected account an agent acts as. A third narrow + # filter for the same reason the two above are narrow, and the same reason + # they exist at all: these assert on the `composio_execute` REQUEST BODY — + # that an unpinned toolkit still carries no connection id, and a pinned one + # carries the operator's. Both are decidable only under `composio`, so + # without a lane they would be twenty-four's worth of silence again, and + # the negative half is the one protecting every existing single-account + # company from having its account resolution changed. + - name: Test which Composio account an execute acts as + run: scripts/ci/run-scoped-suite.sh "composio account choice" openhuman,tinycortex,composio harness::composio::live::live_tests + + # Issue #820 — a fourth narrow filter, and the first outside `harness::`. + # The console-plane half of the same decision: the grouping the choice is + # reported through, and the cleanup that drops a choice naming an account + # Composio no longer lists. Both are gated on `composio`, so before this + # step they were compiled by `Check (--all-features)` and run by nothing. + # + # `…::tests::gated_tests` and not `server::ops::composio`, which would + # sweep in `an_admin_is_unaffected` — the one that dials + # `api.tinyhumans.ai` for real once the feature is on (#801). The module + # exists to be nameable here: a gated ops test added later joins it and is + # run, rather than depending on somebody remembering this file. + - name: Test the Composio account choice on the console plane + run: scripts/ci/run-scoped-suite.sh "composio choice ops" openhuman,tinycortex,composio server::ops::composio::tests::gated_tests + # Issue #477. Before this step, `tinyplace` was COMPILED by CI and # EXECUTED by nothing. `Check (--all-features)` above builds every # `tinyplace`-gated line and deliberately runs none of them, and no other @@ -935,10 +960,18 @@ jobs: # `openhuman_core/mcp` enabled, so what recompiles here is this crate plus # its bin, not the vendored tree. # + # `composio` on the same grounds as `mcp`, and for one spec: + # `composio-account-choice.spec.ts` asserts WHICH connected account an + # agent acts as (issue #820), and `composio_execute` exists on a belt only + # under this feature. Without it the spec would skip — which is how the + # four specs #467 rescued came to sit unrun for months. Cheap for the same + # reason: `composio` adds no `openhuman_core/*` subfeature, so the + # vendored tree does not rebuild. + # # `--bin`, not `--all-targets`: the lane needs a binary, and the tests and # examples under this feature set are already covered above. - name: Build the gated host binary for the live-brain e2e lane - run: cargo build --locked --features openhuman,tinycortex,mcp --bin opencompany + run: cargo build --locked --features openhuman,tinycortex,mcp,composio --bin opencompany - name: Upload the gated host binary uses: actions/upload-artifact@043fb46d1a93c77aae656e7c1c64a875d1fc6a0a # v7.0.1 @@ -1229,9 +1262,17 @@ jobs: # what makes `playwright.config.ts` start the two fixtures and hand the # host their addresses. See `frontend/test/e2e/capabilities.ts` for why # this cannot be detected from the host instead. + # `PW_COMPOSIO=1` is the same kind of declaration as `PW_LIVE_BRAIN`: the + # binary downloaded above carries `--features composio`, so this run can + # stand up `test/e2e/composio-backend.mjs` and point the host at it + # (issue #820). Set here rather than in `e2e:live` so a developer running + # that script against a host without the feature still skips cleanly + # instead of failing on routes that answer `409 not in this build`. - name: Run the end-to-end suite against a live brain run: npm run e2e:live working-directory: frontend + env: + PW_COMPOSIO: "1" - name: Upload failure artifacts if: failure() diff --git a/docs/modules/server/authority.md b/docs/modules/server/authority.md index 3cc214a72..3bdedd42d 100644 --- a/docs/modules/server/authority.md +++ b/docs/modules/server/authority.md @@ -23,7 +23,7 @@ the world as, and which third-party accounts its agents act through: | Surface | Admin-scoped | |---|---| -| `composio` | `PUT …/composio/token`, `POST …/composio/authorize` | +| `composio` | `PUT …/composio/token`, `POST …/composio/authorize`, `DELETE …/composio/connections/{id}`, `PUT`/`DELETE …/composio/connections/{id}/default` | | `connections` (`oauth`) | `POST …/connections/{p}/start`, `POST …/connections/{p}/disconnect` | | `inference` | `PUT …/inference`, `DELETE …/inference` | | `smtp` | `PUT …/smtp`, `POST …/smtp/test` (the caller names the recipient) | diff --git a/docs/spec/runtime/credentials.md b/docs/spec/runtime/credentials.md index 8130551fa..e0a363242 100644 --- a/docs/spec/runtime/credentials.md +++ b/docs/spec/runtime/credentials.md @@ -136,6 +136,53 @@ that made `PUT …/composio/token` admin-only in issue #403. Both a set and a clear are journaled as `ToolAccessChanged`, told apart from each other, and attributed to whoever made the change. +## Which connected account (issue #820) + +The credential decides **whose** accounts a call can reach. It does not decide +**which** of them, and for a company holding two accounts for one toolkit — +`ops@` and `billing@` Gmail — those are different questions. + +Until #820 the second had no answer at all. `composio_execute` built its body as +`{tool, arguments}` and carried no connection id, so the account was resolved by +Composio for the entity, outside this codebase entirely. Two consequences worth +naming: "send from the billing account, not ops" was not sayable, and *which +Gmail did the agent send from* was unanswerable even after the fact. The only +lever was to disconnect the account you did not want. + +The choice is now a per-company, per-toolkit preference: + +- **Stored** as one JSON blob under `composio/defaults` + (`{"gmail": "ca_billing"}`), beside the credential it qualifies and read the + same way `inference/config` is. Not a secret — the ids are the same ones + `GET …/composio/connections` already hands the console, and are useless + without the bearer that scopes them — but company state, so it moves, backs up + and is deleted with the rest of the company's Composio state. +- **Resolved** into `TenantComposio` by the same `resolve` the credential goes + through, and folded into the roster fingerprint, so a change reaches the + agents on their next turn with no restart — exactly like a rotated token. +- **Sent** as `connectionId` on the execute body, which the platform backend + forwards to Composio as `connectedAccountId`. +- **Set** through `PUT …/composio/connections/{id}/default` (admin-only), which + validates the id against this company's own filtered connection list first and + refuses an account that is not usable. Cleared through the matching `DELETE`, + which deliberately makes **no** upstream call: clearing has to work when the + account is gone or the provider is unreachable, which is when a validating + clear would refuse. + +**Absent is the ordinary state, and it is not a degraded one.** A company that +has chosen nothing sends no connection id and gets Composio's own resolution, +byte-for-byte the behaviour that existed before — which is what keeps this +change invisible to every single-account company. Nothing invents a default from +the connection list: `list_connections_detailed`'s `(toolkit, id)` sort is a +stable render order for a read, never a choice, and a default the console +claimed but the harness did not honour would read as a guarantee. The console +says "Composio picks" rather than pointing at a row. + +Two pins are dropped automatically, because a pin to a connection that no longer +exists would be sent on the next execute and refused — turning the disconnect of +one account into a broken toolkit: when the console revokes an account, and when +`GET …/composio/connections` finds a chosen id that Composio no longer lists. + ## Not the inference key `inference/key` is a different thing and must stay a different slot. It holds diff --git a/frontend/playwright.config.ts b/frontend/playwright.config.ts index f4001cfd5..d2074ef3b 100644 --- a/frontend/playwright.config.ts +++ b/frontend/playwright.config.ts @@ -3,7 +3,13 @@ import { mkdirSync } from "node:fs"; import { dirname, resolve } from "node:path"; import { fileURLToPath } from "node:url"; -import { LIVE_BRAIN, MCP_FIXTURE_BIND, MOCK_BRAIN_BIND } from "./test/e2e/capabilities"; +import { + COMPOSIO, + COMPOSIO_FIXTURE_BIND, + LIVE_BRAIN, + MCP_FIXTURE_BIND, + MOCK_BRAIN_BIND, +} from "./test/e2e/capabilities"; // `package.json` is `"type": "module"`, so this file is ESM and `__dirname` // does not exist here — it type-checks against `@types/node` and then throws at @@ -98,10 +104,31 @@ const inferenceEnv: Record = managesFixtures ? { OPENCOMPANY_INFERENCE_KEY: "mock-brain", OPENCOMPANY_INFERENCE_URL: `http://${MOCK_BRAIN_BIND}/v1`, - PW_HOST_PASSTHROUGH: "OPENCOMPANY_INFERENCE_KEY OPENCOMPANY_INFERENCE_URL", } : {}; +/** Whether this run also brings up the Composio fixture backend (issue #820). */ +const managesComposio = managesHost && COMPOSIO; + +/** + * Where the host's Composio calls go, when this run is standing a fixture up. + * + * The same `PW_HOST_PASSTHROUGH` caveat applies as above and is the reason the + * two blocks are joined below rather than each setting the variable: `host.sh` + * copies an allowlist into an empty environment, so a second assignment here + * would quietly replace the first and the inference URL would never arrive. + */ +const composioEnv: Record = managesComposio + ? { OPENCOMPANY_COMPOSIO_BACKEND_URL: `http://${COMPOSIO_FIXTURE_BIND}` } + : {}; + +const passthrough = [...Object.keys(inferenceEnv), ...Object.keys(composioEnv)]; +const hostEnv: Record = { + ...inferenceEnv, + ...composioEnv, + ...(passthrough.length > 0 ? { PW_HOST_PASSTHROUGH: passthrough.join(" ") } : {}), +}; + /** * One `webServer` entry per fixture, ahead of the host. * @@ -111,7 +138,20 @@ const inferenceEnv: Record = managesFixtures * only — the host reads its inference URL at boot but does not dial it until a * turn runs, well after every server here is ready. */ -const fixtureServers = managesFixtures +const fixtureServers = [ + ...(managesComposio + ? [ + { + command: `node ./test/e2e/composio-backend.mjs --bind ${COMPOSIO_FIXTURE_BIND}`, + url: `http://${COMPOSIO_FIXTURE_BIND}/healthz`, + reuseExistingServer: !process.env.CI, + timeout: 30_000, + stdout: "pipe" as const, + stderr: "pipe" as const, + }, + ] + : []), + ...(managesFixtures ? [ { command: `node ./test/e2e/mock-brain.mjs --bind ${MOCK_BRAIN_BIND}`, @@ -129,8 +169,9 @@ const fixtureServers = managesFixtures stdout: "pipe" as const, stderr: "pipe" as const, }, - ] - : []; + ] + : []), +]; export default defineConfig({ testDir: "./test/e2e", @@ -163,7 +204,7 @@ export default defineConfig({ stderr: "pipe" as const, env: { PW_HOST_BIND: new URL(baseURL).host, - ...inferenceEnv, + ...hostEnv, }, }, ] diff --git a/frontend/src/api/composio.ts b/frontend/src/api/composio.ts index 29b980f4b..082f60bf7 100644 --- a/frontend/src/api/composio.ts +++ b/frontend/src/api/composio.ts @@ -164,6 +164,17 @@ export interface ComposioConnectedAccount { * indistinguishable at exactly the moment the operator has to pick one. */ account?: string; + /** + * Whether this is the account the company chose to act as for the toolkit + * (issue #820). False on every account until somebody chooses — nothing is + * defaulted implicitly, because a default the harness does not honour reads + * as a guarantee. + * + * Optional on the wire for the same reason {@link ComposioConnection.accounts} + * is: a host predating #820 answers without it, and absent must read as "no + * choice", not as "this one". + */ + isDefault?: boolean; } /** One toolkit's connected state, as returned by `GET …/composio/connections`. */ @@ -187,6 +198,15 @@ export interface ComposioConnection { * provider", not as "no accounts". */ accounts?: ComposioConnectedAccount[]; + /** + * The account the company chose for this toolkit (issue #820), or absent. + * + * **Absent is the ordinary state and means nothing is chosen** — Composio + * resolves the account itself, exactly as it did before a company could + * express a preference. The console must not fill this in from the account + * list: a default the harness does not honour reads as a guarantee. + */ + defaultConnectionId?: string; } /** The company's Composio status. */ @@ -245,6 +265,16 @@ export interface ComposioDisconnect { note: string; } +/** The `…/default` response: what the company now acts as, and a sentence. */ +export interface ComposioDefaultMutation { + /** The toolkit the change applied to. Empty on a clear. */ + toolkit: string; + /** The account now acting for that toolkit — absent after a clear. */ + connectionId?: string; + /** Plain-language confirmation, in the host's own words. */ + note: string; +} + /** * Revoke one connected account (issue #404). * @@ -267,3 +297,37 @@ export function disconnectComposioConnection( `${client.scopeFor(company)}/composio/connections/${encodeURIComponent(connectionId)}`, ); } + +/** + * Make `connectionId` the account this company's agents act as for its toolkit + * (issue #820). Admin-only; 404 when the id names no connection this company + * holds, or names one that is connected but not usable. + * + * The toolkit is not passed: it is a property of the connection, and asking the + * caller to repeat it would only let the two disagree. + */ +export function setComposioDefaultAccount( + client: OpenCompanyClient, + company: string | null, + connectionId: string, +): Promise { + return client.put( + `${client.scopeFor(company)}/composio/connections/${encodeURIComponent(connectionId)}/default`, + {}, + ); +} + +/** + * Stop naming an account for that connection's toolkit — Composio resolves it + * again, as it did before. Needs no live provider, so it still works when the + * account is gone or the backend is unreachable. + */ +export function clearComposioDefaultAccount( + client: OpenCompanyClient, + company: string | null, + connectionId: string, +): Promise { + return client.del( + `${client.scopeFor(company)}/composio/connections/${encodeURIComponent(connectionId)}/default`, + ); +} diff --git a/frontend/src/views/ConnectionsView.tsx b/frontend/src/views/ConnectionsView.tsx index 70e29f7c8..077d95ac9 100644 --- a/frontend/src/views/ConnectionsView.tsx +++ b/frontend/src/views/ConnectionsView.tsx @@ -21,6 +21,7 @@ import { buildGridProviders, disconnectRouteFor, type GridProvider } from "@/lib import { ProviderDetail, type ConnectionSubject } from "@/views/connections/ProviderDetail"; import { InferenceSection } from "@/views/connections/InferenceSection"; import { McpServersSection } from "@/views/connections/McpServersSection"; +import { AccountChoiceSection } from "@/views/connections/AccountChoiceSection"; import { CompanyCredentialCard } from "@/views/connections/CompanyCredentialCard"; import { ComposioSection } from "@/views/connections/ComposioSection"; import { ProvidersSection } from "@/views/connections/ProvidersSection"; @@ -95,6 +96,11 @@ export function ConnectionsView({ client, company }: Props) { // switch or unmount cannot leave one running. const pollTimers = useRef>({}); + // Bumped on every reconciled re-read, so the account-choice section re-reads + // with it: connecting a second Gmail is exactly when that section appears, + // and releasing one is exactly when it stops being a choice (issue #820). + const [connectionsGeneration, setConnectionsGeneration] = useState(0); + const refresh = useCallback(async () => { // Both reads, together: the page's status and the accounts behind it are one // answer to the operator, and refreshing them apart is how a tile ends up @@ -117,6 +123,13 @@ export function ConnectionsView({ client, company }: Props) { rows.filter((r) => r.accounts?.length).map((r) => [toolkitSlug(r.toolkit), r.accounts!]), ), ); + // Bumped here rather than on the success path: the account-choice section + // is downstream of the accounts read, which just landed, and it is a strict + // addition to a page whose status read is allowed to fail independently + // (issue #820). This used to be a `finally` on a `try`; #819 replaced the + // try/catch with the fault-isolated `Promise.all` above, and the bump has + // to survive the early return below. + setConnectionsGeneration((n) => n + 1); if (!list.ok) { // No connections surface on this host yet — show the catalog read-only. setLoad("unavailable"); @@ -565,6 +578,16 @@ export function ConnectionsView({ client, company }: Props) { onConnectSlug={(slug) => void connectSlug(slug)} /> + {/* Only renders for a provider this company holds two or more accounts + for — the one case where "which account do agents act as" is a + question the product can answer (issue #820). */} + + {/* A connection as an object you open rather than a row with a button (issue #404). The grid's half is Composio only: the native catalog is inert — its credential is written and read by nothing (#396) — and a diff --git a/frontend/src/views/connections/AccountChoiceSection.tsx b/frontend/src/views/connections/AccountChoiceSection.tsx new file mode 100644 index 000000000..7540447d1 --- /dev/null +++ b/frontend/src/views/connections/AccountChoiceSection.tsx @@ -0,0 +1,246 @@ +import { useCallback, useEffect, useRef, useState } from "react"; +import { CircleCheck, Loader2, Users } from "lucide-react"; +import { toast } from "sonner"; + +import type { OpenCompanyClient } from "@/api/client"; +import { + clearComposioDefaultAccount, + listComposioConnections, + setComposioDefaultAccount, + type ComposioConnectedAccount, + type ComposioConnection, +} from "@/api/composio"; +import { ApiError } from "@/api/types"; +import { Badge } from "@/components/ui/badge"; +import { Button } from "@/components/ui/button"; +import { Card, CardContent } from "@/components/ui/card"; +import { toolkitLabel } from "@/lib/composio-catalog"; +import { cn } from "@/lib/utils"; + +interface Props { + client: OpenCompanyClient; + company: string | null; + /** Whether this viewer may change what the company acts as (issue #403). */ + canManage: boolean; + /** + * Bumped by the page when a connection is made or released, so the list + * re-reads rather than describing accounts that have since changed. + */ + generation?: number; +} + +/** + * Which account the company acts as, for a provider it holds more than one of + * (issue #820). + * + * ## Why this section exists at all + * + * A company can hold two Composio accounts for one toolkit — `ops@` and + * `billing@` Gmail. Until #820 nothing in the product chose between them: + * `composio_execute` sent no connection id, so the account was resolved by + * Composio for the entity, outside this codebase entirely, and "send from the + * billing account, not ops" was not sayable. The only lever was to disconnect + * the account you did not want — a blunt instrument when both are wanted for + * different work. + * + * ## Why it only appears for two or more + * + * A single account is not a choice, and drawing a "default" control beside the + * only account there is would invite an operator to make a decision that + * changes nothing. One account per toolkit is the ordinary case, so for almost + * every company this section is simply not on the page. + * + * ## Why nothing is marked until somebody marks it + * + * There is no implicit default to report. The host sends + * `defaultConnectionId` only once a company has chosen, and this renders that + * absence as "Composio picks" rather than pointing at the first row — a default + * the console claimed and the harness did not honour would read as a + * guarantee, which is worse than saying nothing. Same reason #819 states the + * absence on the provider detail view. + */ +export function AccountChoiceSection({ client, company, canManage, generation = 0 }: Props) { + const [rows, setRows] = useState(null); + const [busy, setBusy] = useState(null); + // Only the latest read may paint: switching company re-issues this call, and + // a slow earlier response would otherwise show another company's accounts. + const requestGeneration = useRef(0); + // The company these rows describe. A mutation is started against the company + // that was on screen and must stay so — but `refresh` closes over that same + // company, and running it after the view has moved on would answer company + // B's page with company A's accounts. + const shownCompany = useRef(company); + + const refresh = useCallback(async () => { + const generation = ++requestGeneration.current; + try { + const list = await listComposioConnections(client, company); + if (generation !== requestGeneration.current) return; + setRows(list); + } catch { + // No Composio on this host, no credential, or the provider is + // unreachable. Not an error for this page — there is simply nothing to + // choose between, and the sections above already say why. + if (generation === requestGeneration.current) setRows([]); + } + }, [client, company]); + + useEffect(() => { + shownCompany.current = company; + setRows(null); + void refresh(); + }, [company, refresh, generation]); + + /** + * Re-read, unless the operator has moved to another company since the + * mutation was sent. The write itself still lands where it was aimed — it + * named a connection id that belongs to the company that was on screen — but + * the read that follows it must not paint that company's accounts over the + * one now being looked at. + */ + async function refreshIfStillShowing(startedFor: string | null) { + if (shownCompany.current === startedFor) await refresh(); + } + + async function choose(toolkit: string, account: ComposioConnectedAccount) { + const startedFor = company; + setBusy(account.id); + try { + const res = await setComposioDefaultAccount(client, company, account.id); + toast.success(res.note); + await refreshIfStillShowing(startedFor); + } catch (err) { + toast.error(err instanceof ApiError ? err.message : `Couldn't set the ${toolkit} account.`); + } finally { + setBusy(null); + } + } + + async function clear(toolkit: string, account: ComposioConnectedAccount) { + const startedFor = company; + setBusy(account.id); + try { + const res = await clearComposioDefaultAccount(client, company, account.id); + toast.success(res.note); + await refreshIfStillShowing(startedFor); + } catch (err) { + toast.error(err instanceof ApiError ? err.message : `Couldn't clear the ${toolkit} account.`); + } finally { + setBusy(null); + } + } + + // Only providers where the choice is real. `rows === null` is still loading; + // an empty result and a single-account result render nothing at all. + const multi = (rows ?? []).filter((row) => (row.accounts?.length ?? 0) > 1); + if (rows === null || multi.length === 0) return null; + + return ( +
+
+ +

+ Which account agents act as +

+
+

+ {canManage + ? "This company holds more than one account for these providers. Choose which one your agents act as — the choice applies to every agent, and takes effect on their next turn. The other accounts stay connected." + : "This company holds more than one account for these providers. Which one agents act as is an admin's choice."} +

+ + + + {multi.map((row) => ( +
+
+ {toolkitLabel(row.toolkit)} + {row.defaultConnectionId === undefined && ( + // The honest render of "nothing is chosen". Composio's own + // resolution is deterministic enough that nobody has reported + // surprise — but it is not a decision this product makes, and + // the page says which of the two situations the operator is in. + + Composio picks + + )} +
+
    + {(row.accounts ?? []).map((account) => ( +
  • +
    + + {/* No label is a real state for plenty of toolkits, and + inventing one from the slug would render as a fact + the operator cannot check. The id is at least + something they can match against Composio. */} + {account.account ?? account.id} + + + {account.account ? `${account.id} · ` : ""} + {account.status.toLowerCase()} + +
    + + {account.isDefault ? ( + <> + + agents act as this + + {canManage && ( + + )} + + ) : ( + canManage && ( + + ) + )} +
    • + ))} +
+
+ ))} + + +
+ ); +} diff --git a/frontend/src/views/connections/ProviderDetail.tsx b/frontend/src/views/connections/ProviderDetail.tsx index aa0a6ca78..8dbfc506d 100644 --- a/frontend/src/views/connections/ProviderDetail.tsx +++ b/frontend/src/views/connections/ProviderDetail.tsx @@ -103,12 +103,16 @@ const USAGE_RANGE = "30d"; * naively: * * 1. **Which account an agent uses** (Composio). OpenHuman marks the first of - * several as the default, and inheriting that was the plan. It is not true - * here: `composio_execute` posts `{tool, arguments}` and **no connection - * id** (`src/harness/composio.rs`, and `execute_tool` in the shared - * client), so nothing on this side selects an account — Composio resolves it - * for the entity. A "Default" chip would name a decision this product does - * not make. The panel says that instead. + * several as the default, and inheriting that was the plan. It was not true + * when this panel was written: `composio_execute` posted `{tool, arguments}` + * and **no connection id**, so nothing on this side selected an account and + * a "Default" chip would have named a decision the product did not make. + * Issue #820 makes the decision real — `ComposioExecuteTool` sends + * `connectionId` for a toolkit the company has chosen for — so the panel no + * longer says the choice is impossible. It still marks nothing: the choice + * is made in one place (`AccountChoiceSection`), and a second surface + * reading it back is how two surfaces come to disagree. Unchosen stays the + * ordinary state, and Composio resolves it exactly as before. * 2. **When it was connected.** Only Composio records it. The native * `oauth/{provider}` store keeps `{token, account}` and journals nothing on * connect, and MCP has no such concept — so for those the date is not @@ -149,33 +153,47 @@ export function ProviderDetail({ client, company, subject, canManage, busy, onCl : subject.kind === "composio" ? toolkitSlug(subject.provider.slug) : mcpProviderSlug(subject.server.name); - const [calls, setCalls] = useState(null); - const [usageLoad, setUsageLoad] = useState<"loading" | "ready" | "unavailable">("loading"); + // The read carries the key it was made for. The sheet changes subject without + // unmounting, and state set in an effect lands one render *after* the subject + // does — so a figure kept as a bare number would paint against the new + // provider for that frame, which is one provider's call count under another + // provider's name. Nothing is read back unless the key still matches. + const [loaded, setLoaded] = useState<{ + key: string; + load: "ready" | "unavailable"; + calls: number | null; + } | null>(null); useEffect(() => { if (usageKey === null) return; let alive = true; - setUsageLoad("loading"); - setCalls(null); client .usage(USAGE_RANGE, company) .then((usage) => { if (!alive) return; - setCalls(callsForProvider(usage.byProvider, usageKey)); - setUsageLoad("ready"); + setLoaded({ + key: usageKey, + load: "ready", + calls: callsForProvider(usage.byProvider, usageKey), + }); }) // A host without the usage route (older build) 404s. "Not recorded here" // is the honest render — not a zero, which claims the calls were counted // and there were none. .catch(() => { - if (alive) setUsageLoad("unavailable"); + if (alive) setLoaded({ key: usageKey, load: "unavailable", calls: null }); }); return () => { alive = false; }; }, [client, company, usageKey]); - const usage = { load: usageLoad, calls, key: usageKey }; + const current = loaded !== null && loaded.key === usageKey ? loaded : null; + const usage = { + load: current?.load ?? ("loading" as const), + calls: current?.calls ?? null, + key: usageKey, + }; return ( !next && onClose()}> @@ -301,13 +319,18 @@ function ComposioBody({ {live.length > 1 && ( + // #819 wrote this paragraph to say the choice did not exist — + // "`composio_execute` sends no connection id, so Composio resolves + // it. Disconnect the one you do not want an agent to use." #820 is + // what makes that false, so the two branches meeting is what forces + // this edit: the panel must not deny a control the same page offers.

Holding several accounts is fine — they are the company's, and every member works - through them. Which one an agent acts as is not set here:{" "} - composio_execute sends no connection id, so Composio - resolves it. Disconnect the one you do not want an agent to use. + through them. Which one an agent acts as is set under{" "} + Which account agents act as on the Connections + page; until one is chosen, Composio resolves it for the company as it always has.

)} diff --git a/frontend/test/e2e/capabilities.ts b/frontend/test/e2e/capabilities.ts index 809c205fb..d505a22f2 100644 --- a/frontend/test/e2e/capabilities.ts +++ b/frontend/test/e2e/capabilities.ts @@ -58,6 +58,36 @@ export const MOCK_BRAIN_BIND = process.env.PW_MOCK_BRAIN_BIND || "127.0.0.1:8099 /** Where `mcp-server.mjs` listens when this run starts it. */ export const MCP_FIXTURE_BIND = process.env.PW_MCP_FIXTURE_BIND || "127.0.0.1:8098"; +/** + * A host built with `--features composio`, pointed at + * [`composio-backend.mjs`](./composio-backend.mjs) rather than the platform. + * + * Set `PW_COMPOSIO=1` when both are true. The default-feature binary compiles + * none of the live Composio plane — every route on it answers `409 not in this + * build` — so a spec about *which connected account an agent acts as* has + * nothing to drive there. A declaration rather than a probe, for the same + * reason {@link LIVE_BRAIN} is one: the person who chose the feature set is the + * only one who knows. + */ +export const COMPOSIO = process.env.PW_COMPOSIO === "1"; + +/** Where `composio-backend.mjs` listens when this run starts it. */ +export const COMPOSIO_FIXTURE_BIND = process.env.PW_COMPOSIO_FIXTURE_BIND || "127.0.0.1:8097"; + +/** + * The fixture's base URL, for a spec that reads back what the host sent it. + * Defaulted only when this run started it — against a host you brought, the + * backend it dials is yours to name. + */ +export const COMPOSIO_FIXTURE_URL = + process.env.PW_COMPOSIO_FIXTURE_URL || + (COMPOSIO && MANAGES_HOST ? `http://${COMPOSIO_FIXTURE_BIND}` : undefined); + +/** The reason string a `COMPOSIO` skip carries, so no skip is ever bare. */ +export const COMPOSIO_REASON = + "needs a --features composio host pointed at test/e2e/composio-backend.mjs; " + + "set PW_COMPOSIO=1 to run (issue #820)."; + /** * The **URL** of an MCP server an agent may be told to call. * diff --git a/frontend/test/e2e/chat-to-card.spec.ts b/frontend/test/e2e/chat-to-card.spec.ts index ca7db0e7c..20304a866 100644 --- a/frontend/test/e2e/chat-to-card.spec.ts +++ b/frontend/test/e2e/chat-to-card.spec.ts @@ -54,7 +54,7 @@ test("any message on a desk thread can be added to the board", async ({ page }) const prompt = `ship the launch checklist ${Date.now()}`; await page.getByPlaceholder(/^Message /).fill(prompt); - await page.getByRole("button", { name: "Send" }).click(); + await page.getByRole("button", { name: "Send", exact: true }).click(); // The operator's own bubble is the one being turned into a card. const bubble = page.getByText(prompt, { exact: true }).first(); @@ -95,7 +95,7 @@ test("a card the orchestrator opens is chipped in chat, and survives a reload", // `SPAWNONE` is the scripted backend's cue to call `spawn_task` once. const prompt = `please track this SPAWNONE ${Date.now()}`; await page.getByPlaceholder(/^Message /).fill(prompt); - await page.getByRole("button", { name: "Send" }).click(); + await page.getByRole("button", { name: "Send", exact: true }).click(); // Live: the reply bubble says a card was opened. const chip = page.getByRole("link", { name: /Card opened/ }).last(); diff --git a/frontend/test/e2e/composio-account-choice.spec.ts b/frontend/test/e2e/composio-account-choice.spec.ts new file mode 100644 index 000000000..88cb50a1b --- /dev/null +++ b/frontend/test/e2e/composio-account-choice.spec.ts @@ -0,0 +1,328 @@ +import { randomUUID } from "node:crypto"; + +import { expect, test, type Page } from "@playwright/test"; + +import { + COMPOSIO, + COMPOSIO_FIXTURE_URL, + COMPOSIO_REASON, + LIVE_BRAIN, + LIVE_BRAIN_REASON, +} from "./capabilities"; + +/** + * Issue #820 — a company says which connected account its agents act as, and + * the agents act as it. + * + * A company can hold two Composio accounts for one toolkit: `ops@` and + * `billing@` Gmail. Before this, nothing in the product chose between them. + * `composio_execute` built its body as `{tool, arguments}` and sent no + * connection id, so the account was resolved by Composio for the entity, + * entirely outside this codebase — "send from the billing account, not ops" + * was not sayable, and "which Gmail did the agent send from" was unanswerable + * even after the fact. + * + * # Why this spec has two halves, and why both are needed + * + * The console half and the harness half fail independently, and each is + * uninteresting alone: + * + * * A stored preference nothing reads is the exact shape of #396 — a console + * control that writes a value no code path consults. So it is not enough to + * assert the button stored something. + * * A connection id sent on every execute regardless of what the operator + * chose would pass a wire assertion while ignoring the page. So it is not + * enough to assert the body carried an id. + * + * The claim worth pinning joins them: the id on the wire is *the one the + * operator clicked*, and no id is sent at all until they click. The second test + * asserts the negative on purpose — it is what protects every existing + * single-account company from having its account resolution changed by this + * feature. + * + * # What is a fixture here + * + * `composio-backend.mjs` stands in for the platform's + * `/agent-integrations/composio/*` routes, because holding two live Gmail + * connections per CI run is not a thing a test can arrange — and because the + * assertion is about the request body the host sends, which only the receiver + * can report. The host, its secret store, the roster rebuild, the tool belt and + * the agent turn are all real. Only the model's *choice* of tool is scripted + * (`__MOCK_TOOL_CALL__`), for the reason `mcp-agent.spec.ts` gives: a spec + * cannot assert on a model that is free to decline. + */ + +test.skip(!COMPOSIO || !COMPOSIO_FIXTURE_URL, COMPOSIO_REASON); + +/** The Composio bearer the console pastes. The fixture checks no auth. */ +const TOKEN = "e2e-composio-token"; + +/** + * Open Connections with the first-run tour out of the way, and with this + * company holding a Composio credential. + * + * The token is set through the API rather than by typing into the credential + * card: that card is `ComposioSection`'s own subject, and driving it here would + * make this spec fail for that surface's reasons. Everything this spec is + * actually about is clicked. + */ +async function openConnections(page: Page): Promise { + const set = await page.request.put("/api/v1/company/composio/token", { + data: { token: TOKEN }, + }); + expect(set.ok(), `setting the composio token failed: ${set.status()}`).toBeTruthy(); + + await page.goto("/#/settings/connections"); + const skip = page.getByRole("button", { name: "Skip for now" }); + await skip + .waitFor({ state: "visible", timeout: 10_000 }) + .then(() => skip.click()) + .catch(() => { + /* already dismissed in this context */ + }); + await expect(skip).toBeHidden({ timeout: 10_000 }); + await expect(page.getByRole("heading", { name: "Providers" })).toBeVisible({ timeout: 30_000 }); +} + +/** Every execute body the fixture has received, oldest first. */ +async function executes(page: Page): Promise<{ tool: string; connectionId?: string }[]> { + const seen = await page.request.get(`${COMPOSIO_FIXTURE_URL}/__executes`); + expect(seen.ok(), "the composio fixture did not answer /__executes").toBeTruthy(); + return seen.json(); +} + +/** + * Forget what the fixture saw, so one test cannot read another's calls — and + * restore its connection list, so one cannot remove another's accounts either. + */ +async function resetFixture(page: Page): Promise { + await page.request.post(`${COMPOSIO_FIXTURE_URL}/__reset`); +} + +/** Return the company to "nothing chosen", whatever a test left behind. */ +async function clearChoice(page: Page): Promise { + for (const id of ["ca_ops", "ca_billing"]) { + await page.request.delete(`/api/v1/company/composio/connections/${id}/default`); + } +} + +/** + * Put the company back as this file found it: no chosen accounts, and no + * Composio token. + * + * Not housekeeping for its own sake. The suite runs serially against one host + * with one data root, and this file sorts before `connections-*.spec.ts` — so a + * token left set would hand those specs a credentialled company with a live + * provider grid, and they would be asserting about a page this file configured. + * The `PW_COMPOSIO` lane is new; the specs it runs alongside are not. + */ +test.afterAll(async ({ playwright }, testInfo) => { + // `testInfo.project.use`, NOT the environment. `playwright.config.ts` DERIVES + // both of these — `baseURL` defaults when `PW_BASE_URL` is unset, and + // `storageState` defaults to `target/e2e/storage-state.json` whenever the run + // manages its own host — so reading the variables gets the unset case wrong + // in exactly the configuration CI uses. That is not hypothetical: this hook + // built an ANONYMOUS context on the first CI run, its writes were refused + // 401, the token stayed set, and `oauth-onboarding-resume.spec.ts` failed two + // files later on a Slack tile this file had connected. + const request = await playwright.request.newContext({ + baseURL: testInfo.project.use.baseURL, + storageState: testInfo.project.use.storageState as string | undefined, + }); + try { + for (const id of ["ca_ops", "ca_billing"]) { + await request.delete(`/api/v1/company/composio/connections/${id}/default`); + } + const cleared = await request.put("/api/v1/company/composio/token", { + data: { token: "" }, + }); + // Asserted, not fired and forgotten. A silently-refused cleanup is what + // broke the run above, and it broke it somewhere else — two files later, in + // a spec with no idea this one exists. A failure here names the right file. + expect( + cleared.ok(), + `clearing the composio token failed: ${cleared.status()} ${await cleared.text()}`, + ).toBeTruthy(); + } finally { + await request.dispose(); + } +}); + +test("an operator names the account, and the page says so", async ({ page }) => { + await clearChoice(page); + await openConnections(page); + + // The section exists at all only because this company holds two Gmail + // accounts. A single-account company never sees it — a "default" control + // beside the only account there is invites a decision that changes nothing. + const gmail = page.getByTestId("accounts-gmail"); + await expect(gmail).toBeVisible({ timeout: 30_000 }); + await expect(gmail).toContainText("ops@acme.test"); + await expect(gmail).toContainText("billing@acme.test"); + + // Nothing chosen yet, and the page says which of the two situations this is + // rather than pointing at a row it cannot back up (#819's argument). + await expect(gmail).toContainText("Composio picks"); + + await gmail + .getByTestId("account-ca_billing") + .getByRole("button", { name: "Act as this" }) + .click(); + + // The claim is the host's, re-read: the mark is drawn from `GET + // …/composio/connections`, so a passing assertion here means the choice was + // stored and reported, not merely painted locally. + await expect(gmail.getByTestId("account-ca_billing")).toContainText("agents act as this"); + await expect(gmail).not.toContainText("Composio picks"); + + await page.reload(); + await openConnections(page); + await expect( + page.getByTestId("accounts-gmail").getByTestId("account-ca_billing"), + ).toContainText("agents act as this", { timeout: 30_000 }); + + // Choosing for Gmail says nothing about any other provider. + const rows = await page.request.get("/api/v1/company/composio/connections"); + const body = (await rows.json()) as { + toolkit: string; + defaultConnectionId?: string; + }[]; + expect(body.find((r) => r.toolkit === "gmail")?.defaultConnectionId).toBe("ca_billing"); + // Asserted through the row, not through `find(...)?.` — an absent slack row + // would satisfy `toBeUndefined()` just as an unchosen one does, and then the + // isolation claim would hold vacuously. + const slack = body.find((r) => r.toolkit === "slack"); + expect(slack, "the fixture serves a slack toolkit; without it this proves nothing").toBeDefined(); + expect(slack?.defaultConnectionId).toBeUndefined(); +}); + +test.describe("the agent acts as the chosen account", () => { + test.skip(!LIVE_BRAIN, LIVE_BRAIN_REASON); + + /** + * Approve the parked `composio_execute`, which is the only way one ever + * reaches a provider. + * + * Not incidental plumbing: `composio_execute` is an `Execute`-level tool and + * the harness parks it — "sends to a counterparty, which cannot be taken + * back". So the send an operator authorises is the send that carries their + * choice of account, and this spec would be describing a path that does not + * exist if it bypassed the gate. + */ + async function approvePendingSend(page: Page): Promise { + await expect + .poll( + async () => { + const pending = await page.request.get("/api/v1/company/approvals"); + if (!pending.ok()) return 0; + return ((await pending.json()) as unknown[]).length; + }, + { timeout: 30_000 }, + ) + .toBeGreaterThan(0); + + const pending = (await (await page.request.get("/api/v1/company/approvals")).json()) as { + id: string; + tool?: string; + }[]; + for (const approval of pending) { + const decided = await page.request.post( + `/api/v1/company/approvals/${encodeURIComponent(approval.id)}`, + { data: { verdict: "approve" } }, + ); + expect( + decided.ok(), + `approving ${approval.id} failed: ${decided.status()} ${await decided.text()}`, + ).toBeTruthy(); + } + } + + /** + * Make the agent run one `composio_execute` and hand back what the fixture + * received for it. + * + * The POST is awaited explicitly before anything else happens, for the reason + * `mcp-agent.spec.ts` documents: a turn runs inside the request that started + * it, and the host drops the work when the client goes away. + */ + async function runOneExecute(page: Page): Promise<{ tool: string; connectionId?: string }[]> { + await resetFixture(page); + await page.goto("/#/conversation"); + const skip = page.getByRole("button", { name: "Skip for now" }); + await skip + .waitFor({ state: "visible", timeout: 5_000 }) + .then(() => skip.click()) + .catch(() => { + /* already dismissed */ + }); + await page + .getByRole("complementary") + .getByRole("button", { name: /Your company/ }) + .first() + .click(); + + // A SEND, deliberately. `composio_execute` is parked for approval on the + // strength of the slug — a fetch runs straight through — and the send is + // both the case #820 is written around ("send from billing@, not ops@") + // and the one that exercises the gate: park → operator approves → the + // agent re-issues → the pinned account reaches the wire. A fetch here + // would pass while quietly covering none of that. + const directive = `__MOCK_TOOL_CALL__ ${JSON.stringify({ + name: "composio_execute", + arguments: { + tool: "GMAIL_SEND_EMAIL", + arguments: { to: `${randomUUID()}@acme.test`, subject: "e2e", body: "e2e" }, + }, + })}`; + + const posted = page.waitForResponse( + (response) => response.url().endsWith("/chat") && response.request().method() === "POST", + { timeout: 90_000 }, + ); + await page.getByPlaceholder(/^Message /).fill(directive); + // `exact`. The composer's button is labelled exactly "Send"; the sidebar's + // thread preview takes its accessible name from the last message, so a + // loose match resolves to both as soon as any message mentions sending. + await page.getByRole("button", { name: "Send", exact: true }).click(); + expect((await posted).ok(), "the chat POST did not succeed").toBeTruthy(); + + await approvePendingSend(page); + + await expect + .poll(async () => (await executes(page)).length, { timeout: 30_000 }) + .toBeGreaterThan(0); + return executes(page); + } + + test("no id is sent until somebody chooses, and then it is theirs", async ({ page }) => { + await clearChoice(page); + await openConnections(page); + + // The untouched path first. Every company that holds one account per + // toolkit is on it, and this feature must not have moved them: the body is + // the one this tool has always sent, and Composio resolves the account. + const before = await runOneExecute(page); + expect(before[0].tool).toBe("GMAIL_SEND_EMAIL"); + expect( + before[0].connectionId, + `an unchosen toolkit must send no connection id: ${JSON.stringify(before[0])}`, + ).toBeUndefined(); + + // Now choose, through the page, exactly as an operator would. + await openConnections(page); + await page + .getByTestId("accounts-gmail") + .getByTestId("account-ca_billing") + .getByRole("button", { name: "Act as this" }) + .click(); + await expect( + page.getByTestId("accounts-gmail").getByTestId("account-ca_billing"), + ).toContainText("agents act as this"); + + // …and the next turn acts as it. This is the whole issue in one assertion: + // the choice made on the page reaches the request the harness sends, which + // the backend forwards to Composio as the connected account. + const after = await runOneExecute(page); + expect(after[0].connectionId, `the chosen account did not reach the wire`).toBe("ca_billing"); + }); +}); diff --git a/frontend/test/e2e/composio-backend.mjs b/frontend/test/e2e/composio-backend.mjs new file mode 100644 index 000000000..c1a3c6b2e --- /dev/null +++ b/frontend/test/e2e/composio-backend.mjs @@ -0,0 +1,181 @@ +#!/usr/bin/env node +// +// A stand-in for the platform's Composio routes, so the end-to-end suite can +// exercise a company that holds **two accounts for one toolkit** (issue #820). +// +// # Why a fixture and not the real backend +// +// The thing under test is which account an agent acts as, and the only way to +// have that question exist is to hold two connections for one toolkit. Against +// the real backend that would mean running two live Gmail OAuth handshakes per +// run, from CI, against an account nobody owns. It would also make the +// assertion unobservable: the point is the **request body** the host sends — +// whether it carries `connectionId` — and only the party receiving it can say. +// +// So this serves the four routes the console and the harness actually call, and +// records every execute body for the spec to read back: +// +// GET /agent-integrations/composio/toolkits → catalog (gmail, slack) +// GET /agent-integrations/composio/connections → two gmail accounts, one slack +// POST /agent-integrations/composio/authorize → a connect URL nobody opens +// POST /agent-integrations/composio/execute → records the body, succeeds +// DELETE /agent-integrations/composio/connections/{id} +// +// Plus two routes of its own, outside the backend's namespace so they cannot be +// mistaken for it: +// +// GET /healthz → readiness, for `playwright.config.ts`'s webServer wait +// GET /__executes → every execute body seen, oldest first +// POST /__reset → forget them, and restore the seed connection list +// +// # What it deliberately does not do +// +// It does not check the bearer. Tenant isolation is decided by which credential +// the host presents, and that is asserted where it is decidable — the host's own +// `isolation_tests` in `src/harness/composio.rs` drive a recording backend for +// exactly that. A fixture that also enforced auth would fail runs for reasons +// that have nothing to do with the spec driving it. +// +// No dependencies, so it starts as fast as the other fixtures. + +import { createServer } from "node:http"; + +const bindArg = process.argv.indexOf("--bind"); +const bind = bindArg > -1 ? process.argv[bindArg + 1] : "127.0.0.1:8097"; +const [host, port] = bind.split(":"); + +/** + * The connections this company starts each spec with. Two Gmail accounts is the + * whole point, so a spec that disconnects one has to be able to put it back — + * `DELETE …/connections/{id}` mutates the live list, and one process serves + * every spec in the file. + */ +const seedConnections = () => [ + { + id: "ca_ops", + toolkit: "gmail", + status: "ACTIVE", + createdAt: "2026-08-01T10:00:00Z", + accountEmail: "ops@acme.test", + }, + { + id: "ca_billing", + toolkit: "gmail", + status: "ACTIVE", + createdAt: "2026-08-02T10:00:00Z", + accountEmail: "billing@acme.test", + }, + { + id: "ca_slack", + toolkit: "slack", + status: "ACTIVE", + createdAt: "2026-08-03T10:00:00Z", + workspace: "Acme Workspace", + }, +]; + +/** The connections this company holds right now. */ +let connections = seedConnections(); + +/** Every `POST …/execute` body this process has seen, oldest first. */ +const executes = []; + +/** The backend's success envelope. Everything it answers is wrapped in it. */ +function ok(res, data) { + res.writeHead(200, { "content-type": "application/json" }); + res.end(JSON.stringify({ success: true, data })); +} + +function readBody(req) { + return new Promise((resolve) => { + let raw = ""; + req.on("data", (chunk) => (raw += chunk)); + req.on("end", () => { + try { + resolve(raw ? JSON.parse(raw) : {}); + } catch { + resolve({ __unparseable: raw }); + } + }); + }); +} + +const server = createServer(async (req, res) => { + const url = new URL(req.url, `http://${req.headers.host}`); + const path = url.pathname; + + if (path === "/healthz") return ok(res, { status: "ok" }); + + if (path === "/__executes") { + res.writeHead(200, { "content-type": "application/json" }); + return res.end(JSON.stringify(executes)); + } + + if (path === "/__reset" && req.method === "POST") { + executes.length = 0; + // The connection list is state this server changes too. Resetting only the + // execute log would leave a spec that disconnected an account deciding what + // every later spec in the process sees — a failure that surfaces in a spec + // that did not cause it, and only in the order the file happens to run. + connections = seedConnections(); + return ok(res, { reset: true }); + } + + if (path === "/agent-integrations/composio/toolkits") { + return ok(res, { + toolkits: ["gmail", "slack"], + catalog: [ + { + slug: "gmail", + name: "Gmail", + enabled: true, + description: "Send and read email.", + categories: ["email"], + }, + { + slug: "slack", + name: "Slack", + enabled: true, + description: "Post messages to channels.", + categories: ["communication"], + }, + ], + }); + } + + if (path === "/agent-integrations/composio/connections" && req.method === "GET") { + return ok(res, { connections }); + } + + if (path.startsWith("/agent-integrations/composio/connections/") && req.method === "DELETE") { + const id = decodeURIComponent(path.split("/").pop()); + const at = connections.findIndex((c) => c.id === id); + if (at > -1) connections.splice(at, 1); + return ok(res, { deleted: at > -1 }); + } + + if (path === "/agent-integrations/composio/authorize" && req.method === "POST") { + return ok(res, { connectUrl: "https://connect.composio.dev/e2e", connectionId: "ca_new" }); + } + + if (path === "/agent-integrations/composio/execute" && req.method === "POST") { + const body = await readBody(req); + executes.push(body); + // The shape the harness parses: a provider result plus the cost. The + // account it *would* have acted as is echoed back so a failure reads as + // "actedAs ca_ops, expected ca_billing" rather than as an empty diff. + return ok(res, { + data: { actedAs: body.connectionId ?? null, tool: body.tool }, + successful: true, + error: null, + costUsd: 0, + }); + } + + res.writeHead(404, { "content-type": "application/json" }); + res.end(JSON.stringify({ success: false, error: `no fixture route for ${req.method} ${path}` })); +}); + +server.listen(Number(port), host, () => { + console.error(`[composio fixture] listening on http://${bind}`); +}); diff --git a/frontend/test/e2e/mcp-agent.spec.ts b/frontend/test/e2e/mcp-agent.spec.ts index ec8f34a4d..6ff71eb04 100644 --- a/frontend/test/e2e/mcp-agent.spec.ts +++ b/frontend/test/e2e/mcp-agent.spec.ts @@ -124,7 +124,12 @@ test("an agent calls a tool on a registered MCP server and shows the result", as { timeout: 90_000 }, ); await page.getByPlaceholder(/^Message /).fill(directive); - await page.getByRole("button", { name: "Send" }).click(); + // `exact`, because the composer's button is labelled exactly "Send" while + // the sidebar's thread previews take their accessible names from message + // text — so a loose match resolves to two elements the moment any message + // in the transcript mentions sending, and dies on a strict-mode violation + // in a spec that has nothing to do with whatever wrote that message. + await page.getByRole("button", { name: "Send", exact: true }).click(); await expect(page.getByText(/^Couldn't send/)).toHaveCount(0); expect((await posted).ok(), "the chat POST did not succeed").toBeTruthy(); diff --git a/frontend/test/e2e/mock-brain.mjs b/frontend/test/e2e/mock-brain.mjs index 96877c802..be156365f 100644 --- a/frontend/test/e2e/mock-brain.mjs +++ b/frontend/test/e2e/mock-brain.mjs @@ -43,16 +43,26 @@ // `/embeddings` is served here too rather than left to 404 in the middle of a // memory write. // -// # The three arms +// # The arms, in the order they are tried // -// Everything this server does is decided by scanning the request's messages: +// Everything this server does is decided by scanning the request's messages. +// The order is load-bearing, not incidental: each of the first two arms exists +// because a later arm would otherwise consume a directive that was not meant +// for it. // -// 1. a message carrying `__MOCK_TOOL_CALL__ {"name":…,"arguments":{…}}` — +// 1. a **triage classification** (issue #678) — answer `chatter` and touch +// nothing else. It is handed the operator's raw message, so it carries any +// directive that message carried, and serving one here burns it. +// 2. the host's **re-issue instruction** as the last message (issue #820) — +// emit the named call with the arguments the instruction dictates. The +// directive that produced the parked call has already been served, so +// without this arm no approval-gated tool can run in this lane at all. +// 3. a message carrying `__MOCK_TOOL_CALL__ {"name":…,"arguments":{…}}` — // emit exactly that tool call, once. `mcp.spec.ts` uses it to make an // agent call a named MCP tool without a model that might decide not to. -// 2. a message carrying `SPAWNONE` — call `spawn_task` once, which is what +// 4. a message carrying `SPAWNONE` — call `spawn_task` once, which is what // `chat-to-card.spec.ts` needs an orchestrator to do. -// 3. anything else — a fixed line carrying the `__MOCK_LLM__` marker. +// 5. anything else — a fixed line carrying the `__MOCK_LLM__` marker. // // # Why the plain reply quotes nothing // @@ -123,6 +133,28 @@ const TOOL_CALL_DIRECTIVE = "__MOCK_TOOL_CALL__"; /** The cue that makes the orchestrator open exactly one board card. */ const SPAWN_DIRECTIVE = "SPAWNONE"; +/** + * The host's own re-issue instruction, sent to the agent when an operator + * approves a parked tool call (`src/harness/brain.rs`): + * + * Operator approved your `composio_execute` call. Re-issue it now with + * EXACTLY these arguments: {…}. Do not modify them. + * + * Honouring it is not a fourth directive — it is the same behaviour a real + * model has on that prompt, and without it **no approval-gated tool can ever + * run in this lane**. The directive arms fire once per identity, so on the + * re-issue turn the original `__MOCK_TOOL_CALL__` is already served and the + * mock would answer with prose; the operator's approval would then produce a + * cheerful reply and no call, which is exactly the failure #243 was about. Any + * spec about an `Execute`-level tool (`composio_execute`, `repo_publish`) + * needs this. + * + * The arguments are re-issued VERBATIM, as the instruction demands: the grant + * admits one call matching them exactly, so drift would simply re-park. + */ +const REISSUE_PATTERN = + /Operator approved your `([^`]+)` call\. Re-issue it now with EXACTLY these arguments: /; + /** * Width of every vector `/embeddings` returns. `HostedEmbeddings` compares this * against its declared dimensionality and errors on a mismatch rather than @@ -278,6 +310,28 @@ function findDirective(messages) { return null; } +/** + * The host's re-issue instruction in the last message, or null. + * + * Only the last message is considered. An instruction further back was already + * answered on the turn it arrived, and re-answering it would call the tool + * again every turn for the rest of the thread. + * + * @param {any[]} messages + * @returns {{name: string, arguments: any} | null} + */ +function findReissue(messages) { + const text = textOf(messages[messages.length - 1]); + const match = REISSUE_PATTERN.exec(text); + if (!match) return null; + const args = readJsonObject(text, match.index + match[0].length); + if (!args) { + process.stderr.write("[mock brain] re-issue instruction found but its arguments did not parse\n"); + return null; + } + return { name: match[1], arguments: args }; +} + /** * Directive identities already acted on, for the life of this process. * @@ -385,11 +439,45 @@ function chatCompletion(body) { // Answered `chatter` rather than refused, so the suite stays on the ungated // path it was written for: only an `answer` verdict narrows the delegation // claim. + // + // **First arm tried**, ahead of the re-issue arm below as well as the + // directive arms: everything after this point assumes an agent turn, and a + // classification is not one. It cannot currently reach the re-issue arm — + // `findReissue` requires the host's instruction to be the LAST message and a + // classification's last message is the operator's — but that is a property of + // one prompt, not a rule worth relying on. if (isTriageRequest(messages)) { process.stderr.write("[mock brain] triage classification (no directive consumed)\n"); return completion(model, { role: "assistant", content: "chatter" }, "stop"); } + // Ahead of the directive arms, and only when the instruction is the LAST + // thing said: the re-issue prompt is a fresh turn from the host, so anything + // older in the transcript — including the directive that produced the parked + // call — has already had its say. + const reissue = findReissue(messages); + if (reissue) { + process.stderr.write(`[mock brain] re-issuing approved call: ${reissue.name}\n`); + return completion( + model, + { + role: "assistant", + content: null, + tool_calls: [ + { + id: `mock-reissue-${messages.length}`, + type: "function", + function: { + name: reissue.name, + arguments: JSON.stringify(reissue.arguments), + }, + }, + ], + }, + "tool_calls", + ); + } + const directive = findDirective(messages); if ( diff --git a/frontend/test/e2e/wiring.spec.ts b/frontend/test/e2e/wiring.spec.ts index da2880c54..022ff2452 100644 --- a/frontend/test/e2e/wiring.spec.ts +++ b/frontend/test/e2e/wiring.spec.ts @@ -53,7 +53,12 @@ test("operator console renders a mocked backend reply end to end", async ({ // Send a unique prompt through the operator chat input. const prompt = `e2e wiring ping ${Date.now()}`; await page.getByPlaceholder(/^Message /).fill(prompt); - await page.getByRole("button", { name: "Send" }).click(); + // `exact`, because the composer's button is labelled exactly "Send" while the + // sidebar's thread previews take their accessible names from message text — so + // a loose match resolves to two elements the moment any message in the + // transcript mentions sending, and dies on a strict-mode violation in a spec + // that has nothing to do with whatever wrote that message. + await page.getByRole("button", { name: "Send", exact: true }).click(); // The mocked backend reply must render as a company bubble, and no send // error may appear. diff --git a/frontend/test/unit/account-choice-company-switch.test.ts b/frontend/test/unit/account-choice-company-switch.test.ts new file mode 100644 index 000000000..4c42b541c --- /dev/null +++ b/frontend/test/unit/account-choice-company-switch.test.ts @@ -0,0 +1,190 @@ +// @vitest-environment jsdom + +import { act, createElement } from "react"; +import { createRoot, type Root } from "react-dom/client"; +import { afterEach, beforeEach, describe, expect, it } from "vitest"; + +import type { OpenCompanyClient } from "@/api/client"; +import type { ComposioConnection } from "@/api/composio"; +import { AccountChoiceSection } from "@/views/connections/AccountChoiceSection"; + +/** + * Which account a company acts as, while the operator moves between companies + * (issue #820). + * + * A pure test cannot reach this. The bug is one of *ordering* between a + * mutation, the read that follows it, and a prop change that lands in between — + * three things that only exist once the component is mounted and rendering. It + * is the same exception `provider-detail-render` earns: what is under test is + * what the operator ends up looking at. + * + * The failure it pins: a choose/clear that resolves after the view has moved to + * another company re-reads through the closure it was created with, and paints + * the previous company's accounts onto the current company's page. Every id on + * screen then belongs to a company the operator is not looking at — and the + * next click sends one of them. + */ + +/** Two accounts under one toolkit — the only shape this section renders at all. */ +function twoAccounts(company: string): ComposioConnection[] { + return [ + { + toolkit: "gmail", + connected: true, + accounts: [ + { + id: `${company}-ops`, + status: "ACTIVE", + connected: true, + account: `ops@${company}.test`, + }, + { + id: `${company}-billing`, + status: "ACTIVE", + connected: true, + account: `billing@${company}.test`, + }, + ], + }, + ]; +} + +/** A promise this test resolves by hand, so a request can be left in flight. */ +function deferred() { + let settle!: (value: T) => void; + const promise = new Promise((resolve) => { + settle = resolve; + }); + return { promise, settle }; +} + +interface Host { + client: OpenCompanyClient; + /** Every company the connection list was read for, oldest first. */ + reads: string[]; + /** Release the pending `PUT …/default`. */ + finishChoose: () => void; +} + +/** + * A host that serves each company its own accounts and holds the choice write + * open until the test lets it go. + * + * The scope prefix is the real one (`/api/v1/companies/{id}` vs the unscoped + * path), so the company a call was aimed at is recoverable from the URL — which + * is the whole assertion. + */ +function host(): Host { + const reads: string[] = []; + const pending = deferred<{ toolkit: string; connectionId: string; note: string }>(); + const companyOf = (path: string) => path.match(/companies\/([^/]+)\//)?.[1] ?? ""; + const client = { + scopeFor: (company: string | null) => + company === null ? "/api/v1/company" : `/api/v1/companies/${company}`, + get: async (path: string) => { + const company = companyOf(path); + reads.push(company); + return twoAccounts(company); + }, + put: async (path: string) => { + const company = companyOf(path); + // Only the write under test is deferred; a second company's writes would + // deadlock the test rather than fail it. + if (company !== "alpha") return { toolkit: "gmail", connectionId: "", note: "done" }; + return pending.promise; + }, + } as unknown as OpenCompanyClient; + return { + client, + reads, + finishChoose: () => + pending.settle({ toolkit: "gmail", connectionId: "alpha-billing", note: "Acting as billing" }), + }; +} + +let container: HTMLDivElement; +let root: Root; + +async function show(client: OpenCompanyClient, company: string) { + await act(async () => { + root.render( + createElement(AccountChoiceSection, { client, company, canManage: true }), + ); + }); +} + +function text(): string { + return container.textContent ?? ""; +} + +/** The "Act as this" button on a named account row. */ +function actAs(id: string): HTMLButtonElement { + const row = container.querySelector(`[data-testid="account-${id}"]`); + expect(row, `no row for ${id}`).not.toBeNull(); + const button = row!.querySelector("button"); + expect(button, `no button on ${id}`).not.toBeNull(); + return button as HTMLButtonElement; +} + +beforeEach(() => { + (globalThis as unknown as { IS_REACT_ACT_ENVIRONMENT: boolean }).IS_REACT_ACT_ENVIRONMENT = true; + container = document.createElement("div"); + document.body.appendChild(container); + root = createRoot(container); +}); + +afterEach(() => { + act(() => root.unmount()); + container.remove(); +}); + +describe("choosing an account while the view changes company", () => { + it("does not answer the new company's page with the previous one's accounts", async () => { + const { client, reads, finishChoose } = host(); + await show(client, "alpha"); + expect(text()).toContain("ops@alpha.test"); + + // Send the choice, and leave it in flight. + await act(async () => { + actAs("alpha-billing").click(); + }); + + // The operator moves to another company before it lands. + await show(client, "beta"); + expect(text()).toContain("ops@beta.test"); + + // Now the write completes. The refresh it would have run is bound to + // `alpha` — the closure it was created in — so running it here is exactly + // the bug. + await act(async () => { + finishChoose(); + }); + + expect(text()).toContain("ops@beta.test"); + expect(text()).not.toContain("alpha"); + expect( + reads.filter((company) => company === "alpha"), + "the settled mutation must not re-read the company that is no longer shown", + ).toHaveLength(1); + }); + + it("still re-reads when the operator stayed put", async () => { + // The guard has to be a *company* check and not a blanket "never refresh + // after a mutation" — the mark the console draws comes from the host's own + // answer, so the ordinary path must still go back for it. + const { client, reads, finishChoose } = host(); + await show(client, "alpha"); + + await act(async () => { + actAs("alpha-billing").click(); + }); + await act(async () => { + finishChoose(); + }); + + expect( + reads.filter((company) => company === "alpha"), + "the initial read plus the one the choice triggered", + ).toHaveLength(2); + }); +}); diff --git a/frontend/test/unit/provider-detail-render.test.ts b/frontend/test/unit/provider-detail-render.test.ts index 69364b9ba..7f84a559d 100644 --- a/frontend/test/unit/provider-detail-render.test.ts +++ b/frontend/test/unit/provider-detail-render.test.ts @@ -1,6 +1,7 @@ // @vitest-environment jsdom import { act, createElement } from "react"; +import { flushSync } from "react-dom"; import { createRoot, type Root } from "react-dom/client"; import { afterEach, beforeEach, describe, expect, it } from "vitest"; @@ -155,12 +156,17 @@ describe("the provider detail view", () => { expect(text()).toContain("EXPIRED"); }); - it("marks no account as the one agents use, because nothing chooses one", async () => { + it("marks no account itself, and points at where the choice is made", async () => { // OpenHuman marks the first of several as the default and inheriting that - // was the plan. It is not true here: `composio_execute` posts - // `{tool, arguments}` and no connection id, so Composio resolves which - // account acts. A "Default" chip would name a decision this product does - // not make — the exact shape of invention the issue rules out. + // was the plan. #819 refused, correctly for its own moment: `composio_execute` + // posted `{tool, arguments}` and no connection id, so a "Default" chip would + // have named a decision the product did not make. + // + // #820 makes the decision real, which is what this assertion had to change + // for. The panel still marks nothing — the choice is one control on one + // surface, and a second place to read it back is how two surfaces come to + // disagree — but it no longer tells the operator the choice does not exist, + // because on this page it now does. await render( gmail([ account({ id: "conn-gmail-1", account: "ops@acme.test" }), @@ -169,7 +175,10 @@ describe("the provider detail view", () => { true, ); expect(text()).not.toContain("Default"); - expect(text()).toContain("composio_execute"); + expect(text()).toContain("Which account agents act as"); + // And specifically not the claim it replaced: an operator reading this + // panel must not be told to disconnect an account to control which one acts. + expect(text()).not.toContain("sends no connection id"); }); it("says a connection date is not recorded rather than leaving it blank", async () => { @@ -375,3 +384,97 @@ describe("the same panel, opened on a remote MCP server (#821)", () => { expect(text()).toContain("ceo, engineer"); }); }); + +/** A host whose usage answer is released by the test, one call at a time. */ +function gatedClient(byProvider: UsageDto["byProvider"]) { + const gates: Array<() => void> = []; + const client = { + usage: () => + new Promise((resolve) => { + gates.push(() => resolve({ ...EMPTY_USAGE, byProvider })); + }), + } as unknown as OpenCompanyClient; + return { client, gates }; +} + +/** The Usage section's own text, so a figure cannot be matched from elsewhere. */ +function usageText(): string { + return document.querySelector('[data-testid="connection-detail-usage"]')?.textContent ?? ""; +} + +/** A connected Composio provider by slug, with one account. */ +function connected(slug: string, name: string): GridProvider { + const rows = buildGridProviders( + [entry(slug, name)], + [], + { [slug]: { provider: slug, connected: true, via: ["composio"] } }, + OPEN, + false, + { [slug]: [account()] }, + ); + return rows.find((p) => p.slug === slug)!; +} + +describe("the same panel, changed from one subject to another", () => { + it("never shows one provider's call count under another provider's name", async () => { + // The sheet stays mounted and changes subject, so the usage figure is state + // that outlives the thing it describes. State reset in an effect lands one + // render *after* the new subject does — long enough for the browser to + // paint 4,242 Gmail calls against Slack's name, which is not a slow render + // but a wrong claim. + const { client, gates } = gatedClient([ + { provider: "gmail", calls: 4242 }, + { provider: "slack", calls: 7 }, + ]); + + await render(connected("gmail", "Gmail"), true, client); + await act(async () => gates[0]()); + expect(text()).toContain("4242"); + + // Change subject and read the DOM *before* effects run — the frame the + // operator sees. The second usage read is deliberately left in flight, so + // nothing can have answered for Slack yet. + // + // Deliberately outside `act`, which flushes effects before it returns and + // would hide the very frame under test. The environment flag is dropped for + // the duration only so React does not warn about the render it is being + // asked to do. + const env = globalThis as unknown as { IS_REACT_ACT_ENVIRONMENT: boolean }; + env.IS_REACT_ACT_ENVIRONMENT = false; + flushSync(() => { + root.render( + createElement(ProviderDetail, { + client, + company: null, + subject: { + kind: "composio", + provider: connected("slack", "Slack"), + noCredential: false, + onConnectAnother: () => {}, + onDisconnectAccount: () => {}, + }, + canManage: true, + busy: false, + onClose: () => {}, + }), + ); + }); + env.IS_REACT_ACT_ENVIRONMENT = true; + expect(text()).toContain("Slack"); + expect(text()).not.toContain("4242"); + expect(text()).toContain("Reading usage…"); + + // The frame under test is behind us, so the effect that render scheduled + // can be flushed — deliberately, rather than trusting it to have run by the + // scheduler's grace, since `gates[1]` does not exist until it has. + await act(async () => {}); + expect(gates, "the subject change must have issued a usage read of its own").toHaveLength(2); + + // And Slack's own answer, when it arrives, is Slack's. Read from the Usage + // section itself: a bare `7` would be satisfied by any digit anywhere on a + // panel that also carries account counts and timestamps. + await act(async () => gates[1]()); + expect(usageText()).toContain("7 calls in the last 30 days"); + expect(text()).not.toContain("4242"); + }); +}); diff --git a/scripts/ci/feature-lanes.txt b/scripts/ci/feature-lanes.txt index 6584f243d..bbdd791ef 100644 --- a/scripts/ci/feature-lanes.txt +++ b/scripts/ci/feature-lanes.txt @@ -60,7 +60,7 @@ acp | partial | acp | server::acp ha runner | partial | runner | runner mcp | partial | openhuman,mcp,telegram,media | harness::build::tests app::types media | partial | openhuman,mcp,telegram,media | harness::build::tests harness::toolbelt -composio | partial | openhuman,tinycortex,composio | harness::composio::isolation_tests harness::composio::ops_helper_tests +composio | partial | openhuman,tinycortex,composio | harness::composio::isolation_tests harness::composio::ops_helper_tests harness::composio::live::live_tests server::ops::composio::tests::gated_tests export | partial | export | store::export imap | partial | imap,smtp | server::ops::imap sidecar | partial | sidecar | brain::sidecar diff --git a/src/company/composio.rs b/src/company/composio.rs index e6bb8fd40..415614cc9 100644 --- a/src/company/composio.rs +++ b/src/company/composio.rs @@ -123,6 +123,143 @@ pub async fn token_configured(company: &CompanyId, secrets: &dyn SecretStore) -> .unwrap_or(false)) } +/// The [`SecretStore`] key holding this company's per-toolkit default +/// connections — a JSON object `{"gmail": "ca_123"}` written by the console +/// (issue #820). +/// +/// Stored the way `inference/config` is: one small JSON blob per company, +/// alongside the credential it qualifies, rather than a new port. It is a +/// *preference*, not a secret — the ids in it are already handed to the console +/// by `GET …/composio/connections`, and are useless without the bearer that +/// scopes them. It lives in the secret store because that is the one per-company +/// key/value plane this repo has, and because keeping it beside [`TOKEN_KEY`] +/// means a company's Composio state moves, backs up and is deleted as one thing. +pub const DEFAULTS_KEY: &str = "composio/defaults"; + +/// This company's chosen connection per toolkit: `gmail` → a Composio connection +/// id (issue #820). +/// +/// Absent for a toolkit means **no company has expressed an intent**, and the +/// execute path then sends no connection id at all, leaving the resolution to +/// Composio exactly as before. That absence is the ordinary case and is not a +/// degraded one — one account per toolkit needs no choice — so nothing here +/// invents a default from the connection list. A default that the product does +/// not actually make would be a claim the harness could not honour, which is the +/// failure #820 was filed about. +pub type ComposioDefaults = std::collections::BTreeMap; + +/// This company's stored per-toolkit defaults, or an empty map. +/// +/// A blob that will not parse is treated as *no defaults* rather than an error: +/// the only writer is [`set_default`] / [`clear_default`], so unparseable means +/// hand-edited or from a future shape, and the honest response on the agent path +/// is to fall back to Composio's own resolution rather than to withhold the +/// tools. It is logged, not swallowed silently. +pub async fn load_defaults( + company: &CompanyId, + secrets: &dyn SecretStore, +) -> Result { + let Some(SecretValue(raw)) = secrets.get(company, DEFAULTS_KEY).await? else { + return Ok(ComposioDefaults::new()); + }; + if raw.trim().is_empty() { + return Ok(ComposioDefaults::new()); + } + match serde_json::from_str::(&raw) { + Ok(defaults) => Ok(defaults + .into_iter() + .map(|(toolkit, id)| (toolkit.trim().to_ascii_lowercase(), id.trim().to_string())) + .filter(|(toolkit, id)| !toolkit.is_empty() && !id.is_empty()) + .collect()), + Err(err) => { + tracing::warn!( + company = %company, + error = %err, + "[composio] stored connection defaults did not parse; treating this company as \ + having expressed no preference" + ); + Ok(ComposioDefaults::new()) + } + } +} + +/// Pin `toolkit` to `connection_id`, replacing whatever it named before, and +/// return the resulting map. +/// +/// The caller is responsible for checking that the id names a connection this +/// company actually holds — see +/// [`set_default_connection`](crate::harness::composio::set_default_connection), +/// which is the only path the console reaches this through. Storing an id blind +/// would let a typo silently redirect every send for a toolkit to nothing. +pub async fn set_default( + company: &CompanyId, + secrets: &dyn SecretStore, + toolkit: &str, + connection_id: &str, +) -> Result { + let mut defaults = load_defaults(company, secrets).await?; + defaults.insert( + toolkit.trim().to_ascii_lowercase(), + connection_id.trim().to_string(), + ); + save_defaults(company, secrets, &defaults).await?; + Ok(defaults) +} + +/// Drop `toolkit`'s pin — back to letting Composio resolve the account — and +/// return the resulting map. +pub async fn clear_default( + company: &CompanyId, + secrets: &dyn SecretStore, + toolkit: &str, +) -> Result { + let mut defaults = load_defaults(company, secrets).await?; + defaults.remove(&toolkit.trim().to_ascii_lowercase()); + save_defaults(company, secrets, &defaults).await?; + Ok(defaults) +} + +/// Drop every pin naming `connection_id`, and report whether anything went. +/// +/// Called when an account is revoked: a pin to a connection that no longer +/// exists would be sent on the next execute and refused by Composio, turning a +/// disconnect of the *other* account into a broken toolkit. +pub async fn forget_connection( + company: &CompanyId, + secrets: &dyn SecretStore, + connection_id: &str, +) -> Result { + let connection_id = connection_id.trim(); + let mut defaults = load_defaults(company, secrets).await?; + let before = defaults.len(); + defaults.retain(|_, id| id != connection_id); + if defaults.len() == before { + return Ok(false); + } + save_defaults(company, secrets, &defaults).await?; + Ok(true) +} + +async fn save_defaults( + company: &CompanyId, + secrets: &dyn SecretStore, + defaults: &ComposioDefaults, +) -> Result<()> { + // An empty map is stored as an empty string rather than `{}`, matching how + // every other value here is cleared: `SecretStore` has no delete, and the + // loader already reads empty as "nothing pinned". + let raw = if defaults.is_empty() { + String::new() + } else { + serde_json::to_string(defaults).map_err(|err| { + crate::error::OpenCompanyError::Store(format!( + "could not serialize composio defaults: {err}" + )) + })? + }; + secrets.set(company, DEFAULTS_KEY, SecretValue(raw)).await +} + /// One provider in the catalog the console renders, carrying the backend's own /// display metadata rather than a bare slug (issue #600). /// @@ -232,4 +369,144 @@ mod tests { "https://staging-api.tinyhumans.ai" ); } + + #[derive(Default)] + struct MemSecrets { + map: std::sync::Mutex>, + } + + #[async_trait::async_trait] + impl SecretStore for MemSecrets { + async fn get(&self, _c: &CompanyId, key: &str) -> Result> { + Ok(self + .map + .lock() + .unwrap() + .get(key) + .map(|v| SecretValue(v.clone()))) + } + async fn set(&self, _c: &CompanyId, key: &str, value: SecretValue) -> Result<()> { + self.map.lock().unwrap().insert(key.to_string(), value.0); + Ok(()) + } + } + + #[tokio::test] + async fn a_company_with_no_stored_preference_pins_nothing() { + let company = CompanyId::new("acme"); + let secrets = MemSecrets::default(); + assert!(load_defaults(&company, &secrets).await.unwrap().is_empty()); + } + + #[tokio::test] + async fn a_pin_round_trips_and_is_replaced_rather_than_appended() { + let company = CompanyId::new("acme"); + let secrets = MemSecrets::default(); + + let after = set_default(&company, &secrets, "gmail", "ca_ops") + .await + .unwrap(); + assert_eq!(after.get("gmail").map(String::as_str), Some("ca_ops")); + assert_eq!( + load_defaults(&company, &secrets).await.unwrap(), + after, + "the stored blob is what the setter reported" + ); + + // A second toolkit is additive; naming gmail again replaces it. + set_default(&company, &secrets, "slack", "ca_workspace") + .await + .unwrap(); + let after = set_default(&company, &secrets, "gmail", "ca_billing") + .await + .unwrap(); + assert_eq!(after.get("gmail").map(String::as_str), Some("ca_billing")); + assert_eq!( + after.get("slack").map(String::as_str), + Some("ca_workspace"), + "pinning one toolkit must not disturb another" + ); + } + + #[tokio::test] + async fn toolkits_are_normalized_so_a_pin_is_found_by_the_slug_prefix() { + // `slug_toolkit` lowercases (`GMAIL_SEND_EMAIL` → `gmail`), so a pin + // stored under `GMail` would be invisible to the execute path. + let company = CompanyId::new("acme"); + let secrets = MemSecrets::default(); + set_default(&company, &secrets, " GMail ", " ca_ops ") + .await + .unwrap(); + assert_eq!( + load_defaults(&company, &secrets) + .await + .unwrap() + .get("gmail") + .map(String::as_str), + Some("ca_ops") + ); + } + + #[tokio::test] + async fn clearing_a_pin_returns_the_toolkit_to_composios_own_resolution() { + let company = CompanyId::new("acme"); + let secrets = MemSecrets::default(); + set_default(&company, &secrets, "gmail", "ca_ops") + .await + .unwrap(); + let after = clear_default(&company, &secrets, "gmail").await.unwrap(); + assert!(after.is_empty()); + assert!(load_defaults(&company, &secrets).await.unwrap().is_empty()); + // Clearing what was never pinned is not an error. + assert!( + clear_default(&company, &secrets, "gmail") + .await + .unwrap() + .is_empty() + ); + } + + #[tokio::test] + async fn revoking_the_pinned_account_drops_the_pin() { + // Otherwise the next execute sends an id Composio no longer knows, and + // disconnecting the *other* account breaks the toolkit. + let company = CompanyId::new("acme"); + let secrets = MemSecrets::default(); + set_default(&company, &secrets, "gmail", "ca_ops") + .await + .unwrap(); + set_default(&company, &secrets, "slack", "ca_workspace") + .await + .unwrap(); + + assert!( + !forget_connection(&company, &secrets, "ca_unrelated") + .await + .unwrap(), + "revoking an unpinned account changes nothing" + ); + assert!( + forget_connection(&company, &secrets, "ca_ops") + .await + .unwrap() + ); + + let left = load_defaults(&company, &secrets).await.unwrap(); + assert_eq!(left.get("slack").map(String::as_str), Some("ca_workspace")); + assert!(!left.contains_key("gmail")); + } + + #[tokio::test] + async fn an_unparseable_blob_reads_as_no_preference() { + let company = CompanyId::new("acme"); + let secrets = MemSecrets::default(); + secrets + .set(&company, DEFAULTS_KEY, SecretValue("not json".into())) + .await + .unwrap(); + assert!( + load_defaults(&company, &secrets).await.unwrap().is_empty(), + "a hand-edited blob must fall back to Composio's resolution, not withhold the tools" + ); + } } diff --git a/src/harness/composio.rs b/src/harness/composio.rs index 06a6866ac..a7c4edaf1 100644 --- a/src/harness/composio.rs +++ b/src/harness/composio.rs @@ -110,6 +110,16 @@ pub struct TenantComposio { /// backend's server-enforced allowlist (open mode); non-empty narrows /// strictly, client-side, before any network round-trip. pub toolkits: Vec, + /// Which connected account this company means, per toolkit (issue #820). + /// + /// Read from the company's own store by [`Self::resolve`], never from agent + /// input: the id decides which Gmail an agent sends as, so it must be a + /// company decision the same way the credential is. + /// + /// Empty — the ordinary case — means the company has expressed no intent and + /// `composio_execute` sends no connection id, leaving the account to + /// Composio's own resolution exactly as before. + defaults: crate::company::composio::ComposioDefaults, } impl TenantComposio { @@ -124,9 +134,30 @@ impl TenantComposio { backend_url: backend_url.into(), credential, toolkits, + defaults: Default::default(), } } + /// The same config with this company's per-toolkit connection pins attached + /// (issue #820). + /// + /// A builder rather than a fourth parameter on [`Self::new`]: every existing + /// call site means "no pins", and the honest way to say that is to not say + /// it. + pub fn with_defaults(mut self, defaults: crate::company::composio::ComposioDefaults) -> Self { + self.defaults = defaults; + self + } + + /// The connection id this company pinned for `toolkit`, if any. + /// + /// `toolkit` is matched as [`slug_toolkit`] produces it — lowercased — which + /// is what [`crate::company::composio::set_default`] normalizes to on the way + /// in. + pub fn default_connection(&self, toolkit: &str) -> Option<&str> { + self.defaults.get(toolkit).map(String::as_str) + } + /// Resolve a per-tenant Composio config, or `None` (fail closed) when no /// credential can be obtained at all. /// @@ -179,11 +210,27 @@ impl TenantComposio { }; match credential { Credential::None => None, - credential => Some(Self::new( - backend_url_or_default(backend_url_env, api_url_env), - credential, - toolkits, - )), + credential => { + // Which account the company means, per toolkit (issue #820). + // Read here rather than per call so it lands in the fingerprint + // below: changing the pin then rebuilds the roster on the next + // turn, the same way a rotated token does, and no tool holds a + // stale answer. A store hiccup on *this* read means "no + // preference" — degrading to Composio's own resolution is the + // behaviour that existed before the pin did, so it cannot + // reroute anything. + let defaults = crate::company::composio::load_defaults(company, secrets) + .await + .unwrap_or_default(); + Some( + Self::new( + backend_url_or_default(backend_url_env, api_url_env), + credential, + toolkits, + ) + .with_defaults(defaults), + ) + } } } @@ -231,6 +278,11 @@ impl TenantComposio { c.backend_url.hash(&mut hasher); c.credential.hash_identity(&mut hasher); c.toolkits.hash(&mut hasher); + // The pins are part of what the tools do, so a console change + // to one has to reach the agents the same cycle a token change + // does (issue #820). Safe to hash by value: a connection id is + // not a credential. + c.defaults.hash(&mut hasher); } } hasher.finish() @@ -342,6 +394,7 @@ impl std::fmt::Display for DisconnectError { pub use live::{ ComposioMetering, authorize_connect_url, composio_tools, delete_connection, list_catalog_toolkits, list_connection_states, list_connections_detailed, + set_default_connection, }; #[cfg(feature = "composio")] @@ -455,6 +508,109 @@ mod live { Ok((client, vec![token])) } + /// Run a Composio action **as a named connected account** (issue #820). + /// + /// The vendored [`ComposioClient::execute_tool`] builds its body as + /// `{tool, arguments}` and has no parameter for a connected account, so a + /// company that holds two Gmail accounts has no way to say which one an + /// agent sends from — the account is resolved by Composio for the entity, + /// outside this codebase entirely. The platform backend's + /// `POST /agent-integrations/composio/execute` *does* accept a + /// `connectionId` and forwards it to Composio as `connectedAccountId` + /// (`composioExecuteToolController`), so the only missing link was this + /// body field. + /// + /// This is deliberately a **thin shim, not a fork**: every step below is the + /// vendored client's own public helper, called in the vendored client's own + /// order, so the two paths cannot drift on argument normalization, egress + /// disclosure or provider-error rendering. It is reached **only** when the + /// company has pinned an account; an unpinned call still goes through + /// `execute_tool` verbatim, which is why the ordinary single-account + /// company's behaviour is untouched by this change. + /// + /// The one behaviour it does not reproduce is the client's private + /// single-shot post-OAuth retry, so it is re-stated here against the same + /// error string — see [`POST_OAUTH_AUTH_ERROR`]. Delete all of this the day + /// the vendored client's execute body takes a connection id. + async fn execute_pinned( + client: &ComposioClient, + tool: &str, + arguments: Option, + connection_id: &str, + ) -> Result { + use oh::security::egress::{EgressDescriptor, emit_external_transfer, enforce_egress}; + + // Egress spine: disclose (and, under LocalOnly, refuse) the transfer + // BEFORE the round-trip, exactly as `execute_tool` does. A pinned call + // ships the same arguments to the same third party; it must not be a way + // around the gate. + let egress = EgressDescriptor::composio(tool); + enforce_egress(&egress)?; + emit_external_transfer(egress); + + let arguments = + oh::integrations::composio::execute_prepare::prepare_execute_arguments(tool, arguments) + .map_err(anyhow::Error::msg)?; + let body = json!({ + "tool": tool, + "arguments": arguments, + "connectionId": connection_id, + }); + // The connection id is not a credential (it is the same id the console + // renders and `delete_connection` takes), so it may be traced — the + // arguments still may not. + tracing::debug!(tool = %tool, connection_id = %connection_id, "[composio] execute (pinned account)"); + + let post = async |body: &Value| { + client + .inner() + .post::( + "/agent-integrations/composio/execute", + body, + ) + .await + }; + + let mut resp = post(&body).await?; + if is_post_oauth_auth_error(&resp) { + tracing::debug!( + tool = %tool, + "[composio] pinned execute hit the post-OAuth readiness gap; retrying once" + ); + tokio::time::sleep(POST_OAUTH_RETRY_DELAY).await; + resp = post(&body).await?; + } + if !resp.successful + && let Some(ref err) = resp.error + { + resp.error = + Some(oh::integrations::composio::error_mapping::format_provider_error(tool, err)); + } + Ok(resp) + } + + /// Composio's gateway string for the window between a connection reporting + /// `ACTIVE` and its token being usable for actions. Matched + /// case-insensitively as a substring, mirroring the vendored client. + const POST_OAUTH_AUTH_ERROR: &str = "connection error, try to authenticate"; + + /// How long to wait before the single post-OAuth retry — the vendored + /// client's own delay. + const POST_OAUTH_RETRY_DELAY: std::time::Duration = std::time::Duration::from_secs(10); + + /// Whether a response is the post-OAuth readiness gap rather than a real + /// refusal. Only the payload-level `successful:false` shape is eligible; + /// transport errors have already propagated by this point. + fn is_post_oauth_auth_error( + resp: &oh::integrations::composio::types::ComposioExecuteResponse, + ) -> bool { + !resp.successful + && resp + .error + .as_deref() + .is_some_and(|err| err.to_ascii_lowercase().contains(POST_OAUTH_AUTH_ERROR)) + } + /// Serialize a successful response to JSON, redact the tenant token out of /// it, and bound it to a *body* budget before it reaches the agent. /// Text-only output — the structured value is dropped so a credential the @@ -650,6 +806,58 @@ mod live { } } + /// Pin the toolkit of `connection_id` to that account, so every + /// `composio_execute` for it acts as that account (issue #820). Backs the + /// console's `PUT …/composio/connections/{id}/default`. + /// + /// **The id is checked against this tenant's own filtered list first**, for + /// the same two reasons [`delete_connection`] checks it, and one more that + /// only applies here: an unchecked id would be stored, and a stored id that + /// names nothing is not an error the operator sees at write time — it is a + /// toolkit that stops working at the next agent turn, for a reason nothing + /// on screen explains. Failing the write is the only place the mistake is + /// still legible. + /// + /// Returns the toolkit that was pinned, which is the one the console needs + /// to re-render and never has to guess at. + pub async fn set_default_connection( + config: &TenantComposio, + company: &CompanyId, + secrets: &dyn SecretStore, + connection_id: &str, + ) -> std::result::Result { + let connection_id = connection_id.trim(); + if connection_id.is_empty() { + return Err(DisconnectError::NotFound( + "a connection id is required".to_string(), + )); + } + let known = list_connections_detailed(config) + .await + .map_err(DisconnectError::Upstream)?; + let Some(row) = known.iter().find(|row| row.id == connection_id) else { + return Err(DisconnectError::NotFound( + "no such connection for this company".to_string(), + )); + }; + // An account that is not usable is refused rather than stored: pinning + // an EXPIRED connection would route every send for the toolkit to an + // account that cannot send, which is worse than the unpinned behaviour + // it replaces. Re-authorize it first, then pin it. + if !row.connected { + return Err(DisconnectError::NotFound(format!( + "that account is `{}`, not connected — re-authorize it before making it the default", + row.status + ))); + } + let toolkit = row.toolkit.clone(); + tracing::debug!(connection_id = %connection_id, toolkit = %toolkit, "[composio] ops set_default_connection"); + crate::company::composio::set_default(company, secrets, &toolkit, connection_id) + .await + .map_err(|err| DisconnectError::Upstream(anyhow::anyhow!("{err}")))?; + Ok(toolkit) + } + /// The backend's live Composio toolkit catalog — every slug it will let /// this tenant connect. Backs the console's open-mode provider list /// (issue #397). @@ -1144,15 +1352,30 @@ mod live { ))); } let arguments = args.get("arguments").cloned(); + // Which account this company means for the toolkit, if it has said + // (issue #820). Resolved from the company's own config — never from + // `args` — because "send from billing@, not ops@" is a company + // decision, and an agent that could name a connection could name one + // the operator deliberately did not choose. + let pinned = self.config.default_connection(&toolkit).map(str::to_string); // tracing carries the slug/toolkit only — NEVER arguments or bodies. - tracing::debug!(tool = %tool, toolkit = %toolkit, "[composio] execute"); + tracing::debug!(tool = %tool, toolkit = %toolkit, pinned = ?pinned, "[composio] execute"); let (client, secrets) = match live_call(&self.config).await { Ok(live) => live, Err(err) => { return Ok(ToolResult::error(format!("composio_execute failed: {err}"))); } }; - match client.execute_tool(&tool, arguments).await { + // No pin — the ordinary case — is the untouched path: the same call + // this tool has always made, with no connection id, resolved by + // Composio for the entity. + let call = match pinned.as_deref() { + None => client.execute_tool(&tool, arguments).await, + Some(connection_id) => { + execute_pinned(&client, &tool, arguments, connection_id).await + } + }; + match call { Ok(resp) => { // Metered only on success — i.e. a call that actually // reached the connected account. `connections` in the read @@ -1259,6 +1482,172 @@ mod live { assert!(result.is_error, "the call should be refused"); assert!(meter.samples.lock().unwrap().is_empty()); } + + // ── which account the call acts as (issue #820) ────────────────── + // + // These assert on the **wire body**, not on a return value, because the + // whole of #820 is a field that was missing from it: a test that only + // checked the result would have passed before the change and after it. + + /// Every execute body a stub backend saw. + type Bodies = Arc>>; + + /// A backend that records each `POST …/composio/execute` body and + /// answers with a successful, empty result. + async fn spawn_execute_recorder() -> (String, Bodies) { + use axum::Router; + use axum::routing::post; + + let bodies: Bodies = Arc::new(Mutex::new(Vec::new())); + let seen = Arc::clone(&bodies); + let app = Router::new().route( + "/agent-integrations/composio/execute", + post(async move |axum::Json(body): axum::Json| { + seen.lock().unwrap().push(body); + axum::Json(json!({ + "success": true, + "data": { "data": {"ok": true}, "successful": true, "error": null } + })) + }), + ); + let listener = + tokio::net::TcpListener::bind(std::net::SocketAddr::from(([127, 0, 0, 1], 0))) + .await + .unwrap(); + let addr = listener.local_addr().unwrap(); + tokio::spawn(async move { + axum::serve(listener, app).await.unwrap(); + }); + (format!("http://{addr}"), bodies) + } + + /// An execute tool over `url`, admitting gmail + slack, carrying + /// `defaults` as the company's pins. + fn tool_over( + url: &str, + defaults: &[(&str, &str)], + ) -> (ComposioExecuteTool, Arc) { + let meter = Arc::new(RecordingMeter::default()); + let toolkits = vec!["gmail".to_string(), "slack".to_string()]; + let config = TenantComposio::new( + url.to_string(), + Credential::from_value("token"), + toolkits.clone(), + ) + .with_defaults( + defaults + .iter() + .map(|(t, id)| (t.to_string(), id.to_string())) + .collect(), + ); + ( + ComposioExecuteTool { + config: Arc::new(config), + toolkits: Arc::new(toolkits), + metering: ComposioMetering { + company: CompanyId::new("acme"), + agent: "ceo".to_string(), + meter: Some(Arc::clone(&meter) as Arc), + }, + }, + meter, + ) + } + + /// The ordinary company — one account per toolkit, nothing pinned — + /// must send exactly the body it sent before #820, with no connection + /// id at all. Sending one would change which account Composio resolves + /// for every existing company. + #[tokio::test] + async fn an_unpinned_call_names_no_connection() { + let (url, bodies) = spawn_execute_recorder().await; + let (tool, meter) = tool_over(&url, &[]); + + let result = tool + .execute(json!({"tool": "GMAIL_SEND_EMAIL", "arguments": {"to": "a@b.test"}})) + .await + .expect("execute returns a result"); + assert!(!result.is_error, "the call should succeed: {result:?}"); + + let bodies = bodies.lock().unwrap(); + assert_eq!(bodies.len(), 1); + assert_eq!(bodies[0]["tool"], json!("GMAIL_SEND_EMAIL")); + assert!( + bodies[0].get("connectionId").is_none(), + "an unpinned call must carry no connection id: {}", + bodies[0] + ); + assert_eq!(meter.samples.lock().unwrap().len(), 1, "still metered"); + } + + /// The point of the issue: a company that said "send as billing@" has + /// that carried to the backend, which forwards it to Composio as the + /// connected account. + #[tokio::test] + async fn a_pinned_toolkit_sends_its_connection_id() { + let (url, bodies) = spawn_execute_recorder().await; + let (tool, meter) = tool_over(&url, &[("gmail", "ca_billing")]); + + let result = tool + .execute(json!({"tool": "GMAIL_SEND_EMAIL", "arguments": {"to": "a@b.test"}})) + .await + .expect("execute returns a result"); + assert!(!result.is_error, "the call should succeed: {result:?}"); + + let bodies = bodies.lock().unwrap(); + assert_eq!(bodies.len(), 1); + assert_eq!(bodies[0]["connectionId"], json!("ca_billing")); + assert_eq!( + bodies[0]["arguments"]["to"], + json!("a@b.test"), + "the pinned path still normalizes and forwards the arguments" + ); + assert_eq!( + meter.samples.lock().unwrap().len(), + 1, + "a pinned call is metered like any other" + ); + } + + /// A pin is per toolkit, so one on gmail must not reach a slack call — + /// the toolkit is derived from the slug, the same prefix the allowlist + /// is enforced on. + #[tokio::test] + async fn a_pin_does_not_leak_across_toolkits() { + let (url, bodies) = spawn_execute_recorder().await; + let (tool, _) = tool_over(&url, &[("gmail", "ca_billing")]); + + tool.execute(json!({"tool": "SLACK_POST_MESSAGE", "arguments": {}})) + .await + .expect("execute returns a result"); + + let bodies = bodies.lock().unwrap(); + assert_eq!(bodies.len(), 1); + assert!( + bodies[0].get("connectionId").is_none(), + "slack was never pinned: {}", + bodies[0] + ); + } + + /// The allowlist is still enforced on the slug prefix before anything + /// is sent — a pin is not a way past it. + #[tokio::test] + async fn a_pin_does_not_widen_the_allowlist() { + let (url, bodies) = spawn_execute_recorder().await; + let (mut tool, _) = tool_over(&url, &[("notion", "ca_notion")]); + tool.toolkits = Arc::new(vec!["gmail".to_string()]); + + let result = tool + .execute(json!({"tool": "NOTION_CREATE_PAGE"})) + .await + .expect("execute returns a result"); + assert!(result.is_error, "notion is outside the allowlist"); + assert!( + bodies.lock().unwrap().is_empty(), + "nothing should have been sent" + ); + } } } @@ -1994,6 +2383,82 @@ mod ops_helper_tests { ); } + /// Issue #820: an account that is not usable cannot be the one agents act + /// as. `c2` is a real gmail connection of this company's, and `INITIATED` — + /// pinning it would route every gmail send to an account that cannot send, + /// which is worse than the unpinned behaviour it replaces. So the refusal is + /// a product decision, not a validation nicety, and it is asserted with the + /// store: a refusal that still wrote would be a broken toolkit with a + /// reassuring error message. + /// + /// The two blunter refusals share the test because they share the guard, and + /// the assertion that matters for all three is the same one — nothing + /// reached [`crate::company::composio::set_default`]. + #[tokio::test] + async fn pinning_an_account_that_cannot_send_is_refused_and_stores_nothing() { + use crate::company::composio::load_defaults; + use crate::ports::types::CompanyId; + use crate::store::FsSecretStore; + + let url = spawn_backend().await; + let dir = tempfile::Builder::new() + .prefix("oc-composio-pin-") + .tempdir() + .expect("tempdir"); + let secrets = FsSecretStore::new(dir.path()); + let company = CompanyId::new("acme"); + let cfg = config(&url, vec!["gmail".into(), "slack".into()]); + + let err = set_default_connection(&cfg, &company, &secrets, "c2") + .await + .expect_err("an account that is not connected cannot be pinned"); + // `NotFound` and not `Upstream`: the backend answered fine, and the + // console must render this as the operator's mistake with the fix in it + // ("re-authorize it"), not as a provider outage. + assert!( + matches!(err, DisconnectError::NotFound(_)), + "unexpected error: {err:?}" + ); + assert!( + err.to_string().contains("INITIATED") && err.to_string().contains("not connected"), + "the message names the status the operator has to fix: {err}" + ); + + // An id belonging to nobody, and an id belonging to this company under a + // toolkit its manifest does not grant — the same boundary + // `delete_connection` draws, so a pin cannot reach what no read shows. + for id in ["nope", "c4", " "] { + match set_default_connection(&cfg, &company, &secrets, id).await { + Err(DisconnectError::NotFound(_)) => {} + other => panic!("`{id}` must be refused as NotFound, got {other:?}"), + } + } + + assert!( + load_defaults(&company, &secrets) + .await + .expect("defaults read") + .is_empty(), + "a refused pin must not be stored — the whole point is that the next \ + agent turn is unchanged" + ); + + // The control: `c1` is the same toolkit, ACTIVE, and goes through. Without + // it a guard that refused everything would pass every assertion above. + let toolkit = set_default_connection(&cfg, &company, &secrets, "c1") + .await + .expect("an active account is pinnable"); + assert_eq!(toolkit, "gmail", "the pinned toolkit is reported back"); + assert_eq!( + load_defaults(&company, &secrets) + .await + .expect("defaults read") + .get("gmail") + .map(String::as_str), + Some("c1") + ); + } + /// The console's open-mode source (issue #397): the backend's real catalog, /// normalised. Connectable entries only, trimmed + lowercased, de-duplicated, /// sorted. diff --git a/src/server/ops/composio.rs b/src/server/ops/composio.rs index 9b0de8432..2f1019581 100644 --- a/src/server/ops/composio.rs +++ b/src/server/ops/composio.rs @@ -237,6 +237,10 @@ pub fn router() -> Router { "/composio/connections/{connection_id}", delete(disconnect), )) + .merge(scoped( + "/composio/connections/{connection_id}/default", + put(set_default).delete(clear_default), + )) } /// The company's Composio status as the console renders it. **Never** carries the @@ -371,6 +375,16 @@ struct ConnectionDto { /// one — the concrete reason `connected: bool` alone could not back a /// disconnect. accounts: Vec, + /// The account this company chose for the toolkit, when it has chosen one + /// (issue #820) — the id `composio_execute` sends as `connectionId`. + /// + /// **Omitted, not defaulted.** Absent means the company has expressed no + /// intent and Composio resolves the account itself; there is no implicit + /// default here to report, and inventing one (the oldest, the first in the + /// sort) would be a claim the console makes and the harness does not honour. + /// That absence is the honest state and stays the ordinary one. + #[serde(skip_serializing_if = "Option::is_none")] + default_connection_id: Option, } /// One connected account inside a [`ConnectionDto`] (issue #404). @@ -396,6 +410,10 @@ struct ConnectedAccountDto { /// guessed — see the row type's docs. #[serde(skip_serializing_if = "Option::is_none")] account: Option, + /// Whether this is the account the company chose for the toolkit (issue + /// #820). At most one account per toolkit carries it, and none does until + /// somebody says so. + is_default: bool, } /// Which tier a company's Composio credential comes from. @@ -650,7 +668,63 @@ async fn connections_impl(runtime: &CompanyRuntime) -> Result Result { + let live: std::collections::BTreeSet<&str> = rows.iter().map(|row| row.id.as_str()).collect(); + // Iterated over a snapshot, one write per stale toolkit, and `defaults` is + // reassigned from each write rather than mutated here: `clear_default` + // re-reads the stored blob and returns the whole map as it now stands, so + // the last iteration's return is the fully reduced map and the answer this + // returns cannot drift from what was actually persisted. With nothing stale + // the loop does not run and the map passed in is returned untouched — no + // write on the ordinary read. + let mut defaults = defaults; + for (toolkit, id) in defaults + .clone() + .into_iter() + .filter(|(_, id)| !live.contains(id.as_str())) + { + tracing::info!( + company = %runtime.id(), + toolkit = %toolkit, + connection_id = %id, + "[composio] the chosen account no longer exists at Composio; clearing the choice" + ); + defaults = crate::company::composio::clear_default( + runtime.id(), + runtime.secrets().as_ref(), + &toolkit, + ) + .await + .map_err(ApiError)?; + } + Ok(defaults) } /// Fold per-connection rows into the per-toolkit response shape. @@ -666,19 +740,23 @@ async fn connections_impl(runtime: &CompanyRuntime) -> Result, + defaults: &crate::company::composio::ComposioDefaults, ) -> Vec { let mut by_toolkit: std::collections::BTreeMap = std::collections::BTreeMap::new(); for row in rows { + let chosen = defaults.get(&row.toolkit).map(String::as_str); let entry = by_toolkit .entry(row.toolkit.clone()) .or_insert_with(|| ConnectionDto { toolkit: row.toolkit.clone(), connected: false, accounts: Vec::new(), + default_connection_id: chosen.map(str::to_string), }); entry.connected = entry.connected || row.connected; entry.accounts.push(ConnectedAccountDto { + is_default: chosen == Some(row.id.as_str()), id: row.id, status: row.status, connected: row.connected, @@ -753,6 +831,16 @@ async fn disconnect_impl( }) } })?; + // A pin naming the account just revoked would be sent on the next execute + // and refused — so disconnecting the account a company *did not* choose + // must not be what breaks the one it did (issue #820). + crate::company::composio::forget_connection( + runtime.id(), + runtime.secrets().as_ref(), + connection_id, + ) + .await + .map_err(ApiError)?; Ok(Json(DisconnectDto { note: "Disconnected at Composio. Agents lose these tools on their next turn.".to_string(), })) @@ -766,6 +854,126 @@ async fn disconnect_impl( Err(not_in_build()) } +/// `PUT …/composio/connections/{id}/default` — make that account the one this +/// company's agents act as for its toolkit (issue #820). +/// +/// **Admin-only** (issue #403), for the reason `authorize` and `disconnect` are: +/// "send from billing@, not ops@" is a decision about what the company does, not +/// a per-operator preference — every agent in the company acts through the one +/// answer. +/// +/// The account is named by **connection id**, not by toolkit-plus-id, because +/// the toolkit is already a property of the connection: asking the caller to +/// repeat it would invite the two to disagree, and the id alone is what the +/// console has in hand from `GET …/connections`. +async fn set_default( + company: AdminScopedCompany, + Path(ConnectionPath { connection_id }): Path, +) -> Result, ApiError> { + let dto = set_default_impl(company.runtime.as_ref(), &connection_id).await?; + journal( + &company, + "provider_default_account_set", + Some(dto.0.toolkit.clone()), + ) + .await?; + Ok(dto) +} + +/// `DELETE …/composio/connections/{id}/default` — stop naming an account for +/// that connection's toolkit, returning it to Composio's own resolution. +/// +/// Unlike [`set_default`] this makes **no upstream call** and validates nothing +/// against Composio: the whole point of clearing is to be able to undo a pin +/// when the account is gone or the provider is unreachable, which is exactly +/// when a validating clear would refuse. It removes any pin naming this id and +/// says so; a request for an id that was never pinned is a no-op, not an error. +async fn clear_default( + company: AdminScopedCompany, + Path(ConnectionPath { connection_id }): Path, +) -> Result, ApiError> { + let cleared = crate::company::composio::forget_connection( + company.runtime.id(), + company.runtime.secrets().as_ref(), + &connection_id, + ) + .await + .map_err(ApiError)?; + if cleared { + journal(&company, "provider_default_account_cleared", None).await?; + } + Ok(Json(DefaultDto { + toolkit: String::new(), + connection_id: None, + note: if cleared { + "Cleared. Composio picks the account for this provider again, as it did before." + .to_string() + } else { + "That account was not the default; nothing changed.".to_string() + }, + })) +} + +/// The `…/default` response: what is now pinned, and a sentence saying so. +#[derive(Debug, Serialize)] +#[serde(rename_all = "camelCase")] +struct DefaultDto { + /// The toolkit the change applied to. Empty on a clear, where the caller + /// named a connection rather than a toolkit and may be clearing a pin for + /// an account that no longer exists. + toolkit: String, + /// The account now acting for that toolkit — `None` after a clear. + #[serde(skip_serializing_if = "Option::is_none")] + connection_id: Option, + /// Plain-language confirmation, in the same words the console repeats. + note: String, +} + +#[cfg(feature = "composio")] +async fn set_default_impl( + runtime: &CompanyRuntime, + connection_id: &str, +) -> Result, ApiError> { + use crate::harness::composio::DisconnectError; + + let config = resolve_tenant(runtime).await?; + let toolkit = crate::harness::composio::set_default_connection( + &config, + runtime.id(), + runtime.secrets().as_ref(), + connection_id, + ) + .await + // Same split as `disconnect`: an id this company cannot see is a `404` + // about a call that never left the host, not a `502` about a provider that + // is up. + .map_err(|err| match err { + DisconnectError::NotFound(message) => { + ApiError(crate::error::OpenCompanyError::NotFound(message)) + } + DisconnectError::Upstream(err) => ApiError(crate::error::OpenCompanyError::TinyHumans { + code: "composio_set_default".to_string(), + message: err.to_string(), + }), + })?; + Ok(Json(DefaultDto { + note: format!( + "Agents act as this account for {toolkit} from their next turn. Other accounts stay \ + connected." + ), + toolkit, + connection_id: Some(connection_id.to_string()), + })) +} + +#[cfg(not(feature = "composio"))] +async fn set_default_impl( + _runtime: &CompanyRuntime, + _connection_id: &str, +) -> Result, ApiError> { + Err(not_in_build()) +} + /// A `409 Conflict` "Composio is not in this build" — the OAuth plane's off-state /// under a non-`composio` build. Mirrors the status route's `inBuild:false` /// semantics rather than pretending nothing is connected. @@ -1752,49 +1960,316 @@ mod tests { assert_eq!(body["code"], "conflict", "{body}"); } - /// Issue #404: the per-toolkit shape the tile grid reads is a fold over the - /// per-connection rows, and the fold must not lose an account or flip a - /// boolean. Pure — no live backend needed. + /// The ops tests that are decidable only in a build carrying `composio`. + /// + /// Gathered under one module so a CI lane can *name* them. A feature-gated + /// test's default fate in this repo is "compiled by `Check + /// (--all-features)`, executed by nothing" (issue #770), and the composio + /// lane has to select by filter rather than run this whole module: with the + /// feature on, `an_admin_is_unaffected` dials `api.tinyhumans.ai` for real + /// (issue #801). One filter on this module runs every gated test here and + /// none of that, and a gated test added later is picked up by joining the + /// module rather than by remembering to edit `ci.yml`. #[cfg(feature = "composio")] - #[test] - fn grouping_keeps_every_account_and_ors_their_connected_state() { - use crate::harness::composio::ComposioConnectionRow; + mod gated_tests { + use super::*; + use crate::server::ops::composio::{drop_dangling_defaults, group_by_toolkit}; + + /// Issue #404: the per-toolkit shape the tile grid reads is a fold over the + /// per-connection rows, and the fold must not lose an account or flip a + /// boolean. Pure — no live backend needed. + #[test] + fn grouping_keeps_every_account_and_ors_their_connected_state() { + use crate::harness::composio::ComposioConnectionRow; + + let row = |id: &str, toolkit: &str, connected: bool, account: Option<&str>| { + ComposioConnectionRow { + id: id.to_string(), + toolkit: toolkit.to_string(), + status: if connected { "ACTIVE" } else { "INITIATED" }.to_string(), + connected, + created_at: None, + account: account.map(str::to_string), + } + }; + + let out = group_by_toolkit( + vec![ + row("c1", "gmail", false, Some("a@acme.test")), + row("c2", "gmail", true, Some("b@acme.test")), + row("c3", "slack", false, None), + ], + &Default::default(), + ); + + assert_eq!(out.len(), 2, "one entry per toolkit"); + assert_eq!(out[0].toolkit, "gmail"); + assert!( + out[0].connected, + "a toolkit is connected when ANY of its accounts is — the second row \ + here, which a first-row-wins fold would have missed" + ); + assert_eq!( + out[0] + .accounts + .iter() + .map(|a| (a.id.as_str(), a.connected)) + .collect::>(), + vec![("c1", false), ("c2", true)], + "both accounts survive, in the order the rows arrived" + ); + assert_eq!(out[1].toolkit, "slack"); + assert!(!out[1].connected, "no active account, so not connected"); + assert_eq!(out[1].accounts.len(), 1); + + // Nothing pinned: nothing is marked, and no default is reported. This + // is the shape #819 asks the console to render honestly, and it stays + // the shape until somebody chooses. + assert!(out.iter().all(|dto| dto.default_connection_id.is_none())); + assert!( + out.iter() + .flat_map(|dto| dto.accounts.iter()) + .all(|account| !account.is_default), + "an unchosen account is never marked as the default" + ); + } - let row = |id: &str, toolkit: &str, connected: bool, account: Option<&str>| { - ComposioConnectionRow { + /// Issue #820: once a company has chosen, the choice is reported on the + /// toolkit **and** marked on the one account it names — the console needs + /// both to draw a list with one row marked and the rest offering to become + /// it. + #[test] + fn grouping_marks_the_chosen_account_and_only_that_one() { + use crate::harness::composio::ComposioConnectionRow; + + let row = |id: &str, toolkit: &str| ComposioConnectionRow { id: id.to_string(), toolkit: toolkit.to_string(), - status: if connected { "ACTIVE" } else { "INITIATED" }.to_string(), - connected, + status: "ACTIVE".to_string(), + connected: true, created_at: None, - account: account.map(str::to_string), + account: None, + }; + let defaults: crate::company::composio::ComposioDefaults = + [("gmail".to_string(), "c2".to_string())] + .into_iter() + .collect(); + + let out = group_by_toolkit( + vec![row("c1", "gmail"), row("c2", "gmail"), row("c3", "slack")], + &defaults, + ); + + assert_eq!(out[0].default_connection_id.as_deref(), Some("c2")); + assert_eq!( + out[0] + .accounts + .iter() + .map(|a| (a.id.as_str(), a.is_default)) + .collect::>(), + vec![("c1", false), ("c2", true)], + "exactly one account carries the mark" + ); + assert!( + out[1].default_connection_id.is_none(), + "a choice made for gmail says nothing about slack: {:?}", + out[1].default_connection_id + ); + } + + /// Issue #820: an account revoked **at Composio** — not through this console, + /// so nothing here saw the disconnect — leaves a choice naming a connection + /// that no longer exists. That choice is not merely stale: it is sent on the + /// next `composio_execute` and refused, so the toolkit stops working for + /// every agent for a reason nothing on screen explains. The read the console + /// polls repairs it. + /// + /// Driven through the handler's own helper rather than the route, because + /// the route needs a live Composio backend and the decision under test is + /// the one made *after* it answers. What it must not do is as load-bearing + /// as what it must: a live choice is untouched, and a toolkit whose chosen + /// account is gone falls back to "Composio picks" rather than being + /// re-pointed at a sibling account nobody chose. + #[tokio::test] + async fn the_connections_read_forgets_a_choice_composio_no_longer_lists() { + use crate::company::composio::{load_defaults, set_default}; + use crate::harness::composio::ComposioConnectionRow; + + let home_dir = home(); + let state = state_with_manifest(home_dir.path(), GRANTED).await; + let runtime = runtime_of(&state, "acme"); + let (id, secrets) = (runtime.id(), runtime.secrets()); + + // gmail names an account still live; slack names one revoked since. + for (toolkit, connection) in [("gmail", "c1"), ("slack", "c_revoked")] { + set_default(id, secrets.as_ref(), toolkit, connection) + .await + .unwrap(); } - }; - let out = super::group_by_toolkit(vec![ - row("c1", "gmail", false, Some("a@acme.test")), - row("c2", "gmail", true, Some("b@acme.test")), - row("c3", "slack", false, None), - ]); + let row = |id: &str, toolkit: &str| ComposioConnectionRow { + id: id.to_string(), + toolkit: toolkit.to_string(), + status: "ACTIVE".to_string(), + connected: true, + created_at: None, + account: None, + }; + // The company still holds a slack account — just not the chosen one. + let rows = vec![row("c1", "gmail"), row("c9", "slack")]; + + let left = drop_dangling_defaults( + runtime.as_ref(), + &rows, + load_defaults(id, secrets.as_ref()).await.unwrap(), + ) + .await + .expect("the cleanup completes"); + + assert_eq!( + left.get("gmail").map(String::as_str), + Some("c1"), + "a choice naming a live account is untouched" + ); + assert!( + !left.contains_key("slack"), + "the choice naming a revoked account is dropped: {left:?}" + ); + assert_eq!( + load_defaults(id, secrets.as_ref()).await.unwrap(), + left, + "the repair is stored, not merely reflected in this one response — \ + otherwise the next agent turn still sends the dead id" + ); + + let out = group_by_toolkit(rows, &left); + assert_eq!(out[1].toolkit, "slack"); + assert!( + out[1].default_connection_id.is_none() + && out[1].accounts.iter().all(|account| !account.is_default), + "with its choice gone slack is unchosen again — the surviving account \ + is not silently promoted into a decision nobody made: {:?}", + out[1] + ); + assert_eq!(out[0].default_connection_id.as_deref(), Some("c1")); + } + } + + /// The choice plane is wired on the same terms as the rest of the OAuth + /// plane: in the route table whatever the build, and a `409` — never a + /// `404` — when there is no usable client, since "no such connection" is a + /// claim about this company's accounts that a build without Composio cannot + /// make. + #[tokio::test] + async fn set_default_route_conflicts_without_build_or_token() { + let home_dir = home(); + let state = state_with_manifest(home_dir.path(), GRANTED).await; + + let (status, body, raw) = send( + &state, + "PUT", + "/api/v1/company/composio/connections/conn-1/default", + None, + ) + .await; + assert_eq!(status, StatusCode::CONFLICT, "{raw}"); + assert_eq!(body["code"], "conflict", "{body}"); + } + + /// Clearing is the deliberate exception: it takes no upstream call, so it + /// works in a build without Composio and — the case that matters — when the + /// provider is unreachable or the account is already gone. A clear that + /// needed the network would refuse exactly when it is most needed. + #[tokio::test] + async fn clearing_a_choice_needs_no_client_and_is_idempotent() { + let home_dir = home(); + let state = state_with_manifest(home_dir.path(), GRANTED).await; - assert_eq!(out.len(), 2, "one entry per toolkit"); - assert_eq!(out[0].toolkit, "gmail"); + let (status, body, raw) = send( + &state, + "DELETE", + "/api/v1/company/composio/connections/conn-1/default", + None, + ) + .await; + assert_eq!(status, StatusCode::OK, "{raw}"); assert!( - out[0].connected, - "a toolkit is connected when ANY of its accounts is — the second row \ - here, which a first-row-wins fold would have missed" + body["note"] + .as_str() + .unwrap_or_default() + .contains("nothing changed"), + "clearing what was never chosen says so rather than claiming a change: {body}" ); - assert_eq!( - out[0] - .accounts - .iter() - .map(|a| (a.id.as_str(), a.connected)) - .collect::>(), - vec![("c1", false), ("c2", true)], - "both accounts survive, in the order the rows arrived" + + // Now with something stored, the same call reports the real change and + // leaves nothing behind. + let runtime = state.registry().get(&CompanyId::new("acme")).unwrap(); + crate::company::composio::set_default( + runtime.id(), + runtime.secrets().as_ref(), + "gmail", + "conn-1", + ) + .await + .unwrap(); + + let (status, body, raw) = send( + &state, + "DELETE", + "/api/v1/company/composio/connections/conn-1/default", + None, + ) + .await; + assert_eq!(status, StatusCode::OK, "{raw}"); + assert!( + body["note"] + .as_str() + .unwrap_or_default() + .contains("Cleared"), + "{body}" + ); + assert!( + crate::company::composio::load_defaults(runtime.id(), runtime.secrets().as_ref()) + .await + .unwrap() + .is_empty() + ); + } + + /// Choosing the account a company acts as is an admin's decision, like + /// connecting and disconnecting one: every agent in the company acts + /// through the single answer, so it is not a per-operator preference. + #[tokio::test] + async fn a_member_cannot_choose_the_account_the_company_acts_as() { + let home_dir = home(); + let state = state_with_manifest(home_dir.path(), GRANTED).await; + let member = crate::server::test_support::seed_session( + &state, + "acme", + crate::ports::UserRole::Member, + ) + .await; + + for method in ["PUT", "DELETE"] { + let (status, body, raw) = send_as( + &state, + method, + "/api/v1/company/composio/connections/conn-1/default", + None, + Auth::Cookie(member.clone()), + ) + .await; + assert_eq!(status, StatusCode::FORBIDDEN, "{method}: {raw}"); + assert_eq!(body["code"], "forbidden", "{method}: {body}"); + } + + // And the refusal is real: nothing was stored by either attempt. + let runtime = state.registry().get(&CompanyId::new("acme")).unwrap(); + assert!( + crate::company::composio::load_defaults(runtime.id(), runtime.secrets().as_ref()) + .await + .unwrap() + .is_empty() ); - assert_eq!(out[1].toolkit, "slack"); - assert!(!out[1].connected, "no active account, so not connected"); - assert_eq!(out[1].accounts.len(), 1); } } diff --git a/src/server/ops/write_test.rs b/src/server/ops/write_test.rs index 36a2d4fb2..ed4c5d4bc 100644 --- a/src/server/ops/write_test.rs +++ b/src/server/ops/write_test.rs @@ -6661,6 +6661,20 @@ async fn a_member_cannot_change_what_the_company_reaches_the_world_as() { "/api/v1/company/composio/connections/conn-1", None, ), + // And choosing WHICH of two accounts every agent acts as (issue #820) — + // the same decision again, one step finer: it does not change what the + // company is connected to, only what it sends as, which is precisely + // the kind of company-wide answer this plane exists to hold. + ( + "PUT", + "/api/v1/company/composio/connections/conn-1/default", + None, + ), + ( + "DELETE", + "/api/v1/company/composio/connections/conn-1/default", + None, + ), // The model every agent thinks with, and the key it is billed against. ( "PUT",