From ff1d1dea5d3d2556caa89e6b5c548d92b5222728 Mon Sep 17 00:00:00 2001 From: Nicky Leach Date: Fri, 6 Nov 2026 09:30:24 -0800 Subject: [PATCH] fix(tools): reject non-tool connections before any write (#19492) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Thinking Path > - Paperclip is the open source app people use to manage AI agents for work > - Apps and tools connect to Paperclip through tool connections > - `POST /api/companies/:companyId/tools/connections` creates a tool connection through `createConnection` > - The request schema accepts a channel purpose and the chat and voice transports, but `createConnection` cannot store them > - The database then rejects the row, so the caller gets a 500 and a stray tool application stays behind > - This pull request rejects those requests with a 422 before any write, and stores `config` as `transportConfig` when the caller leaves `transportConfig` out > - The benefit is a clear error, no leftover rows, and consistent connection rows ## Linked Issues or Issue Description No issue exists. The new test typecheck in #18739 found this problem. #18739 listed it as a possible product problem and did not change it. **What happened?** 1. `createConnection` never writes `input.connectionPurpose`, so every row gets the database default `tool`. 2. The schema allows `transport: "chat_sdk"` and `transport: "voice"`. The database check `tool_connections_channel_transport_check` does not allow these transports on a `tool` row. 3. No error mapping exists for a check violation, so the route returns 500. 4. When the request has no `applicationId`, the service inserts a `tool_applications` row first, outside the connection transaction. That row stays after the failure. 5. The schema gave `transportConfig` a default of `{}`. The service stores `input.transportConfig ?? config`, so it stored `{}` instead of a copy of `config`. **Expected behavior** The route returns 422 for a purpose or transport that it cannot create, and writes no rows. When the request leaves `transportConfig` out, the row stores `config` there. **Steps to reproduce** 1. Send `POST /api/companies/:companyId/tools/connections` with `{ "name": "x", "transport": "chat_sdk" }`. 2. See the 500 response. 3. See the new `tool_applications` row for the company. ## What Changed - `server/src/services/tool-access.ts`: `createConnection` rejects a purpose other than `tool`, and the `chat_sdk` and `voice` transports, with 422. The check runs before any lookup or write. Channel connections still come from their own services. - `packages/shared/src/validators/tool-access.ts`: `transportConfig` is optional and has no default. The update schema already removed defaults, so it does not change. - `server/src/__tests__/tool-access-service.test.ts`: added tests for the three rejected inputs (no rows written), and for a request that sends only `config`. ## Verification - `pnpm exec vitest run server/src/__tests__/tool-access-service.test.ts server/src/__tests__/generic-mcp-connection.test.ts server/src/__tests__/tool-connection-health-work.test.ts server/src/__tests__/openapi-routes.test.ts server/src/__tests__/tool-access-routes.test.ts packages/shared/src/validators`: 27 files, 995 tests pass. - `cd server && pnpm typecheck`: passes. This includes the source layers and the three test-tree shards. - Not run locally: the full `pnpm test:run`, the root `pnpm build`, and the UI typecheck. Required CI covers them. ## Risks - Low risk. A client that sends a channel purpose or a chat or voice transport to this route now gets 422 instead of 500. The request failed before this change too. - A client that sends only `config` now gets the same value in `transportConfig`. Every reader checks `config` first, so this changes no behavior. - The OpenAPI output for this body no longer shows a default for `transportConfig`. ## Model Used - Claude Opus 5.5 (`claude-opus-5-5`) in Claude Code, with tool use (shell, file edits) and extended thinking. ## Checklist - [x] I have included a thinking path that traces from project context to this change - [x] I have specified the model used (with version and capability details) - [x] I have checked ROADMAP.md and confirmed this PR does not duplicate planned core work - [x] I have searched GitHub for duplicate or related PRs and linked them above - [x] I have either (a) linked existing issues with `Fixes: #` / `Closes #` / `Refs #` OR (b) described the issue in-PR following the relevant issue template - [x] I have not referenced internal/instance-local Paperclip issues or links (only public GitHub `#NNN` / `github.com/paperclipai/paperclip` URLs) - [x] My branch name describes the change (e.g. `docs/...`, `fix/...`) and contains no internal Paperclip ticket id or instance-derived details - [x] I have run tests locally and they pass - [x] I have added or updated tests where applicable - [x] I have updated relevant documentation to reflect my changes - [x] I have considered and documented any risks above - [ ] All Paperclip CI gates are green - [ ] Greptile is 5/5 with no open P2s, recommendations, or follow-ups - [ ] I will address all Greptile and reviewer comments before requesting merge 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: Claude Opus 5.5 --- packages/shared/src/validators/tool-access.ts | 3 +- .../src/__tests__/tool-access-service.test.ts | 37 +++++++++++++++++++ server/src/services/tool-access.ts | 7 ++++ 3 files changed, 46 insertions(+), 1 deletion(-) diff --git a/packages/shared/src/validators/tool-access.ts b/packages/shared/src/validators/tool-access.ts index 914af74e7ab..f55163c22eb 100644 --- a/packages/shared/src/validators/tool-access.ts +++ b/packages/shared/src/validators/tool-access.ts @@ -191,7 +191,8 @@ export const createToolConnectionSchema = z.object({ status: toolConnectionStatusSchema.optional(), connectionKind: toolConnectionKindSchema.default("managed"), config: toolTransportConfigSchema.optional(), - transportConfig: toolTransportConfigSchema.default({}), + // No default: when the caller omits it, the service stores `config` here. + transportConfig: toolTransportConfigSchema.optional(), credentialRefs: z.array(mcpConnectionCredentialRefSchema).optional(), credentialSecretRefs: z.array(toolCredentialSecretRefSchema).default([]), enabled: z.boolean().optional(), diff --git a/server/src/__tests__/tool-access-service.test.ts b/server/src/__tests__/tool-access-service.test.ts index 186b20e3042..69525cf08e0 100644 --- a/server/src/__tests__/tool-access-service.test.ts +++ b/server/src/__tests__/tool-access-service.test.ts @@ -68,6 +68,7 @@ import { GOOGLE_WORKSPACE_DEFAULT_SELECTION, getAvailableConnectionMethod, getConnectableAppDefinition, + createToolConnectionSchema, type CreateToolConnection, type GoogleWorkspaceConnectorProfileId, } from "@paperclipai/shared"; @@ -3363,6 +3364,42 @@ describeEmbeddedPostgres("tool access service", () => { .where(eq(companySecrets.companyId, company.id))).toEqual([]); }); + it.each([ + { name: "channel purpose", input: { connectionPurpose: "channel" as const, transport: "local_stdio" as const } }, + { name: "chat transport", input: { transport: "chat_sdk" as const } }, + { name: "voice transport", input: { transport: "voice" as const } }, + ])("rejects a $name before it writes any row", async ({ input }) => { + const company = await createCompany(db); + const service = createTestToolAccessService(db); + + await expect(service.createConnection(company.id, connectionInput({ + name: "Not a tool", + config: { templateId: "paperclip.echo-calculator-time" }, + ...input, + }))).rejects.toMatchObject({ status: 422 }); + + expect(await db.select().from(toolConnections) + .where(eq(toolConnections.companyId, company.id))).toEqual([]); + expect(await db.select().from(toolApplications) + .where(eq(toolApplications.companyId, company.id))).toEqual([]); + }); + + it("stores config as transportConfig when the request leaves transportConfig out", async () => { + const company = await createCompany(db); + const service = createTestToolAccessService(db); + const body = createToolConnectionSchema.parse({ + name: "Local echo config only", + transport: "local_stdio", + config: { templateId: "paperclip.echo-calculator-time" }, + }); + + const connection = await service.createConnection(company.id, body); + + const [stored] = await db.select().from(toolConnections).where(eq(toolConnections.id, connection.id)); + expect(stored?.config).toMatchObject({ templateId: "paperclip.echo-calculator-time" }); + expect(stored?.transportConfig).toEqual(stored?.config); + }); + it("registers an approved local stdio template and exposes its runtime slot", async () => { const company = await createCompany(db); const service = createTestToolAccessService(db); diff --git a/server/src/services/tool-access.ts b/server/src/services/tool-access.ts index 7441bcee493..ae1682df28c 100644 --- a/server/src/services/tool-access.ts +++ b/server/src/services/tool-access.ts @@ -4530,6 +4530,13 @@ export function toolAccessService( let applicationNamespace = input.applicationName ?? input.name; const transport = input.transport; if (!transport) throw badRequest("Tool connection transport is required"); + // This path writes only tool connections. Channel setup has its own + // services. Reject other purposes here, before any row is written, so the + // database purpose/transport check never turns them into a 500. + if (input.connectionPurpose !== undefined && input.connectionPurpose !== "tool") + throw unprocessable("Only tool connections can be created here"); + if (transport === "chat_sdk" || transport === "voice") + throw unprocessable("Chat and voice connections cannot be created here"); const config = normalizeGoogleSheetsConnectionConfig( input.config ?? input.transportConfig ?? {}, );