mirror of
https://github.com/paperclipai/paperclip.git
synced 2026-11-09 16:02:36 +01:00
fix(tools): reject non-tool connections before any write (#19492)
## 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 <noreply@anthropic.com>
This commit is contained in:
1 parent
f5de197536
commit
ff1d1dea5d
3 files changed
+46
-1
No files matched your search
@@ -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(),
|
||||
|
||||
@@ -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);
|
||||
|
||||
@@ -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 ?? {},
|
||||
);
|
||||
|
||||
Reference in new issue
Block a user