From 43a65aabc5888e7bc5543718347c3bb72a700f8f Mon Sep 17 00:00:00 2001 From: scotttong Date: Wed, 30 Sep 2026 20:03:31 -0700 Subject: [PATCH] fix(connections): default new connections to allowed for every action Agents get full permissions on a fresh connection so their tasks are not parked on approval cards. The connect flow lands on the Permissions tab, where any write or destructive action can be set to Ask first or Off. Reverts the C6a ask-first default and the server test assertions that encoded it; the Railway callback interstitial test is kept. Co-Authored-By: Claude Opus 5.5 --- packages/shared/src/app-definitions.test.ts | 4 +- packages/shared/src/app-definitions.ts | 29 ++--- packages/shared/src/index.ts | 1 - .../src/__tests__/tool-access-service.test.ts | 102 +++++------------- .../connections/connection-defaults.test.ts | 3 +- 5 files changed, 37 insertions(+), 102 deletions(-) diff --git a/packages/shared/src/app-definitions.test.ts b/packages/shared/src/app-definitions.test.ts index 35846d8ad33..c0ea0eb6c1d 100644 --- a/packages/shared/src/app-definitions.test.ts +++ b/packages/shared/src/app-definitions.test.ts @@ -609,12 +609,12 @@ describe("AppDefinition catalog", () => { APP_DEFINITIONS.find((app) => app.slug === "hugging-face")?.methods[0] ?.defaults?.scopesHint, ).toEqual(["read-mcp", "read-repos", "contribute-repos", "jobs"])); - it("defaults reads to allowed and state-changing actions to ask first", () => { + it("defaults every action, reads and writes, to allowed", () => { for (const app of APP_DEFINITIONS) for (const method of app.methods) expect(recommendedDefaultsForApp(app, method.key)).toEqual({ access: "all_agents", - askFirstRiskLevels: ["write", "destructive", "high", "critical"], + askFirstRiskLevels: [], }); }); it("defaults explicit read/write capability groups to their write-capable method", () => { diff --git a/packages/shared/src/app-definitions.ts b/packages/shared/src/app-definitions.ts index 9e7aad8702c..0d013b73ef8 100644 --- a/packages/shared/src/app-definitions.ts +++ b/packages/shared/src/app-definitions.ts @@ -252,33 +252,18 @@ export function resolveConnectionMethodServerUrl( } } -/** - * Risk levels a fresh connection gates behind human approval (PAP-659 C6a). - * - * The runtime gate and its approval card already exist; what was missing was a - * default with anything behind it. Reads stay Allowed so the constant read - * traffic an agent does is never interrupted; anything that changes the - * provider's state asks first, and one click on the Permissions tab turns that - * off per connection. - * - * Two risk vocabularies are in use — `read|write|destructive` from the tool - * classifier and `low|medium|high|critical` from curated catalogs — so both - * are named rather than assuming callers normalise them. - */ -export const DEFAULT_ASK_FIRST_RISK_LEVELS = [ - "write", - "destructive", - "high", - "critical", -] as const; - export function recommendedDefaultsForApp(app: AppDefinition, methodKey?: string | null): Record { // Keep the parameters in the public contract: callers resolve defaults for a - // concrete app/method even though the initial policy is uniform. + // concrete app/method even though the initial policy is uniform. This is + // an open default, not an approval bypass: connection finalization remains a + // configure-authorized, audited operation, and Ask first stays available as + // an operator-selected policy for any action after the connection is made. + // The connect flow lands on the Permissions tab so that choice is the very + // next screen (PAP-659: agents get full permissions unless someone narrows them). void app; void methodKey; return { access: "all_agents", - askFirstRiskLevels: [...DEFAULT_ASK_FIRST_RISK_LEVELS], + askFirstRiskLevels: [], }; } diff --git a/packages/shared/src/index.ts b/packages/shared/src/index.ts index 021a70791be..66a6a15f2ea 100644 --- a/packages/shared/src/index.ts +++ b/packages/shared/src/index.ts @@ -330,7 +330,6 @@ export { getRecommendedConnectionMethod, isConnectableAppSlug, isAppStoreVisibleSlug, - DEFAULT_ASK_FIRST_RISK_LEVELS, recommendedDefaultsForApp, resolveConnectionMethodServerUrl, } from "./app-definitions.js"; diff --git a/server/src/__tests__/tool-access-service.test.ts b/server/src/__tests__/tool-access-service.test.ts index a1c668aa46e..3fe3fcfc0fb 100644 --- a/server/src/__tests__/tool-access-service.test.ts +++ b/server/src/__tests__/tool-access-service.test.ts @@ -1099,7 +1099,7 @@ describeEmbeddedPostgres("tool access service", () => { await mintDb.$client.end({ timeout: 0 }).catch(() => undefined); await revocationDb.$client.end({ timeout: 0 }).catch(() => undefined); } - }, 20_000); + }, 15_000); it("denies token minting with an actionable error when the requesting agent has no install", async () => { const company = await createCompany(db); @@ -7207,7 +7207,7 @@ describeEmbeddedPostgres("tool access service", () => { if (deadline) clearTimeout(deadline); await callbackDb.$client.end({ timeout: 0 }).catch(() => undefined); } - }, 20_000); + }, 15_000); it("reports GitHub reauthorization for the viewer without borrowing another user's grant", async () => { const company = await createCompany(db); @@ -8048,7 +8048,7 @@ describeEmbeddedPostgres("tool access service", () => { } }); - it("activates Drive write actions behind ask-first defaults after a managed callback", async () => { + it("activates allowed Drive write actions without approval defaults after a managed callback", async () => { const company = await createCompany(db); const userId = `drive-write-member-${randomUUID()}`; await grantBoardUser(db, company.id, userId, [], "owner"); @@ -8148,9 +8148,8 @@ describeEmbeddedPostgres("tool access service", () => { const createEntry = callback.body.catalog.find( (entry: { toolName: string }) => entry.toolName === "create_file", ); - // The armed default: writes ask first, reads stay allowed. - const drivePolicies = JSON.stringify( - await db + await expect( + db .select() .from(toolPolicies) .where( @@ -8159,9 +8158,7 @@ describeEmbeddedPostgres("tool access service", () => { eq(toolPolicies.enabled, true), ), ), - ); - expect(drivePolicies).toContain(createEntry.id); - expect(drivePolicies).not.toContain(searchEntry.id); + ).resolves.toEqual([]); await expect( db .select() @@ -8276,20 +8273,12 @@ describeEmbeddedPostgres("tool access service", () => { ).resolves.toEqual([ expect.objectContaining({ targetType: "agent", targetId: agent.id }), ]); - // Reconnecting neither adds nor drops the ask-first defaults. - expect( - JSON.stringify( - await db - .select() - .from(toolPolicies) - .where( - and( - eq(toolPolicies.companyId, company.id), - eq(toolPolicies.enabled, true), - ), - ), - ), - ).toBe(drivePolicies); + await expect( + db + .select() + .from(toolPolicies) + .where(eq(toolPolicies.companyId, company.id)), + ).resolves.toEqual([]); } finally { driveDefinition.ownershipAvailability = previousOwnershipAvailability; } @@ -8466,7 +8455,6 @@ describeEmbeddedPostgres("tool access service", () => { targetId: company.id, }), ]); - // The armed default: exactly the active write actions ask first. await expect( db .select() @@ -8476,18 +8464,8 @@ describeEmbeddedPostgres("tool access service", () => { eq(toolPolicies.companyId, company.id), eq(toolPolicies.enabled, true), ), - ) - .then((rows) => - rows - .map((row) => [row.policyType, row.selectors.catalogEntryId]) - .sort(), ), - ).resolves.toEqual( - completed.catalog - .filter((entry) => entry.status === "active" && entry.riskLevel !== "read") - .map((entry) => ["require_approval", entry.id]) - .sort(), - ); + ).resolves.toEqual([]); await expect( db .select() @@ -8704,7 +8682,6 @@ describeEmbeddedPostgres("tool access service", () => { targetId: company.id, }), ]); - // The armed default: exactly the active write actions ask first. await expect( db .select() @@ -8714,18 +8691,8 @@ describeEmbeddedPostgres("tool access service", () => { eq(toolPolicies.companyId, company.id), eq(toolPolicies.enabled, true), ), - ) - .then((rows) => - rows - .map((row) => [row.policyType, row.selectors.catalogEntryId]) - .sort(), ), - ).resolves.toEqual( - completed.catalog - .filter((entry) => entry.status === "active" && entry.riskLevel !== "read") - .map((entry) => ["require_approval", entry.id]) - .sort(), - ); + ).resolves.toEqual([]); } finally { driveDefinition.ownershipAvailability = previousOwnershipAvailability; } @@ -8949,7 +8916,7 @@ describeEmbeddedPostgres("tool access service", () => { await callbackDb.$client.end({ timeout: 0 }).catch(() => undefined); await removalDb.$client.end({ timeout: 0 }).catch(() => undefined); } - }, 20_000); + }, 15_000); it("synchronizes shared OAuth credentials to the organization grant used by gateway calls", async () => { vi.stubEnv("PAPERCLIP_TOOL_OAUTH_SLACK_CLIENT_ID", "slack-client-id"); @@ -9528,9 +9495,8 @@ describeEmbeddedPostgres("tool access service", () => { const sendMessageEntry = completed.catalog.find( (entry) => entry.toolName === "send_message", )!; - // The armed default: writes ask first, reads stay allowed. - const callbackPolicies = JSON.stringify( - await db + await expect( + db .select() .from(toolPolicies) .where( @@ -9539,9 +9505,7 @@ describeEmbeddedPostgres("tool access service", () => { eq(toolPolicies.enabled, true), ), ), - ); - expect(callbackPolicies).toContain(sendMessageEntry.id); - expect(callbackPolicies).not.toContain(searchMessagesEntry.id); + ).resolves.toEqual([]); const callbackPolicy = toolAccessPolicyService(db); const decide = ( entry: (typeof completed.catalog)[number], @@ -9557,14 +9521,13 @@ describeEmbeddedPostgres("tool access service", () => { arguments: {}, }, }); - await expect(decide(searchMessagesEntry)).resolves.toMatchObject( - host === "task" - ? { decision: "deny" } - : { decision: "allow", reasonCode: "allow_profile" }, - ); - await expect(decide(sendMessageEntry)).resolves.toMatchObject( - host === "task" ? { decision: "deny" } : { decision: "require_approval" }, - ); + for (const entry of [searchMessagesEntry, sendMessageEntry]) { + await expect(decide(entry)).resolves.toMatchObject( + host === "task" + ? { decision: "deny" } + : { decision: "allow", reasonCode: "allow_profile" }, + ); + } if (host === "task") { await expect( db @@ -11200,23 +11163,12 @@ describeEmbeddedPostgres("tool access service", () => { expect(completed.actions.readOnly).toEqual([ expect.objectContaining({ toolName: "list_tables", riskLevel: "read" }), ]); - // The armed default: exactly the active write actions ask first. await expect( db .select() .from(toolPolicies) - .where(eq(toolPolicies.companyId, company.id)) - .then((rows) => - rows - .map((row) => [row.policyType, row.selectors.catalogEntryId]) - .sort(), - ), - ).resolves.toEqual( - completed.catalog - .filter((entry) => entry.status === "active" && entry.riskLevel !== "read") - .map((entry) => ["require_approval", entry.id]) - .sort(), - ); + .where(eq(toolPolicies.companyId, company.id)), + ).resolves.toEqual([]); const [connection] = await db .select() .from(toolConnections) diff --git a/ui/src/features/connections/connection-defaults.test.ts b/ui/src/features/connections/connection-defaults.test.ts index f54c460fdc4..587309e420c 100644 --- a/ui/src/features/connections/connection-defaults.test.ts +++ b/ui/src/features/connections/connection-defaults.test.ts @@ -1,5 +1,4 @@ import { describe, expect, it } from "vitest"; -import { DEFAULT_ASK_FIRST_RISK_LEVELS } from "@paperclipai/shared"; import { askFirstCatalogEntryIdsFor } from "./connection-defaults"; const action = (catalogEntryId: string, riskLevel: string) => @@ -15,7 +14,7 @@ describe("askFirstCatalogEntryIdsFor", () => { action("odd-1", "medium"), ], }, - suggestedDefaults: { askFirstRiskLevels: [...DEFAULT_ASK_FIRST_RISK_LEVELS] }, + suggestedDefaults: { askFirstRiskLevels: ["write", "destructive", "high", "critical"] }, }; it("gates the write and destructive actions the server named", () => {