mirror of
https://github.com/paperclipai/paperclip.git
synced 2026-10-26 10:26:26 +01:00
fix(apps): complete MCP connection follow-ups
Co-Authored-By: Paperclip <noreply@paperclip.ing>
This commit is contained in:
1 parent
ab5e821d6a
commit
faaa0ce428
9 files changed
+190
-30
No files matched your search
@@ -5840,18 +5840,38 @@ describeEmbeddedPostgres("tool access service", () => {
|
||||
const service = toolAccessService(db);
|
||||
mockToolsList([
|
||||
{ name: "list_zaps", description: "List", inputSchema: { type: "object", properties: {} }, annotations: { readOnlyHint: true } },
|
||||
{ name: "update_zap", description: "Update", inputSchema: { type: "object", properties: {} }, annotations: { readOnlyHint: false } },
|
||||
]);
|
||||
|
||||
const connect = await service.connectGalleryApp(company.id, {
|
||||
galleryKey: "zapier",
|
||||
name: "Zapier reconnect",
|
||||
credentialValues: { "credentials.authorization": "old-secret" },
|
||||
}, { actorType: "user", actorId: "board" });
|
||||
const connect = await withGalleryServerUrl("zapier", PUBLIC_MCP_FIXTURE_URL, () =>
|
||||
service.connectGalleryApp(company.id, {
|
||||
galleryKey: "zapier",
|
||||
name: "Zapier reconnect",
|
||||
credentialValues: { "credentials.authorization": "old-secret" },
|
||||
}, { actorType: "user", actorId: "board" }));
|
||||
|
||||
const before = await service.getConnection(connect.connectionId, company.id);
|
||||
const beforeRef = before.credentialSecretRefs.find((r) => r.configPath === "credentials.authorization")!;
|
||||
expect(beforeRef).toBeDefined();
|
||||
|
||||
const listEntry = connect.catalog.find((entry) => entry.toolName === "list_zaps")!;
|
||||
const updateEntry = connect.catalog.find((entry) => entry.toolName === "update_zap")!;
|
||||
const finished = await service.finishGalleryAppConnection(company.id, connect.connectionId, {
|
||||
enabledCatalogEntryIds: [listEntry.id, updateEntry.id],
|
||||
askFirstCatalogEntryIds: [updateEntry.id],
|
||||
access: "all_agents",
|
||||
}, { actorType: "user", actorId: "board" });
|
||||
await db.delete(toolProfileEntries).where(eq(toolProfileEntries.profileId, finished.profile.id));
|
||||
await db.update(toolCatalogEntries).set({
|
||||
status: "quarantined",
|
||||
quarantineReason: "pending_review",
|
||||
quarantinedAt: new Date(),
|
||||
}).where(eq(toolCatalogEntries.connectionId, connect.connectionId));
|
||||
await db.update(toolConnections).set({
|
||||
config: { ...before.config, quarantineNewEntries: true },
|
||||
transportConfig: { ...before.transportConfig, quarantineNewEntries: true },
|
||||
}).where(eq(toolConnections.id, connect.connectionId));
|
||||
|
||||
await expect(
|
||||
service.reconnectGalleryApp(connect.connectionId, company.id, { credentialValues: {} }, { actorType: "user", actorId: "board" }),
|
||||
).rejects.toMatchObject({ message: expect.stringContaining("Paste a new key") });
|
||||
@@ -5869,6 +5889,27 @@ describeEmbeddedPostgres("tool access service", () => {
|
||||
// Rotated in place: same secret, no duplicate ref created.
|
||||
expect(after.credentialSecretRefs).toHaveLength(before.credentialSecretRefs.length);
|
||||
expect(afterRef.secretId).toBe(beforeRef.secretId);
|
||||
expect(after.config).toMatchObject({ quarantineNewEntries: false });
|
||||
expect(after.transportConfig).toMatchObject({ quarantineNewEntries: false });
|
||||
|
||||
const catalogAfterReconnect = await db.select().from(toolCatalogEntries).where(
|
||||
eq(toolCatalogEntries.connectionId, connect.connectionId),
|
||||
);
|
||||
expect(catalogAfterReconnect).toEqual(expect.arrayContaining([
|
||||
expect.objectContaining({ id: listEntry.id, status: "active", quarantineReason: null }),
|
||||
expect.objectContaining({ id: updateEntry.id, status: "active", quarantineReason: null }),
|
||||
]));
|
||||
const profileEntriesAfterReconnect = await db.select().from(toolProfileEntries).where(
|
||||
eq(toolProfileEntries.profileId, finished.profile.id),
|
||||
);
|
||||
expect(profileEntriesAfterReconnect).toEqual(expect.arrayContaining([
|
||||
expect.objectContaining({ catalogEntryId: listEntry.id, effect: "include" }),
|
||||
expect.objectContaining({ catalogEntryId: updateEntry.id, effect: "include" }),
|
||||
]));
|
||||
await expect(db.select().from(toolPolicies).where(and(
|
||||
eq(toolPolicies.companyId, company.id),
|
||||
eq(toolPolicies.enabled, true),
|
||||
))).resolves.toHaveLength(0);
|
||||
});
|
||||
|
||||
it("stops and restarts local stdio runtime slots through the board service", async () => {
|
||||
|
||||
@@ -4129,7 +4129,11 @@ export function toolAccessService(db: Db, options: ToolAccessServiceOptions = {}
|
||||
}
|
||||
}
|
||||
|
||||
async function refreshCatalog(connectionId: string, actor?: ActorInfo): Promise<ToolCatalogRefreshResult> {
|
||||
async function refreshCatalog(
|
||||
connectionId: string,
|
||||
actor?: ActorInfo,
|
||||
refreshOptions: { enableAllByDefault?: boolean } = {},
|
||||
): Promise<ToolCatalogRefreshResult> {
|
||||
const connection = await getConnectionRow(connectionId);
|
||||
const now = new Date();
|
||||
let descriptors: McpToolDescriptor[];
|
||||
@@ -4161,7 +4165,8 @@ export function toolAccessService(db: Db, options: ToolAccessServiceOptions = {}
|
||||
const sourceTemplateKey = typeof asRecord(connection.config).sourceTemplateKey === "string"
|
||||
? String(asRecord(connection.config).sourceTemplateKey)
|
||||
: null;
|
||||
const quarantineOnRefresh = shouldQuarantineNewEntries(connection)
|
||||
const quarantineOnRefresh = !refreshOptions.enableAllByDefault
|
||||
&& shouldQuarantineNewEntries(connection)
|
||||
&& (connection.status === "active" || sourceTemplateKey === "posthog");
|
||||
const safeDefault = asRecord(connection.config).safeDefault === true;
|
||||
for (const descriptor of descriptors) {
|
||||
@@ -4239,9 +4244,17 @@ export function toolAccessService(db: Db, options: ToolAccessServiceOptions = {}
|
||||
}
|
||||
}
|
||||
|
||||
const normalizedConfig = refreshOptions.enableAllByDefault
|
||||
? { ...connection.config, quarantineNewEntries: false }
|
||||
: connection.config;
|
||||
const normalizedTransportConfig = refreshOptions.enableAllByDefault
|
||||
? { ...connection.transportConfig, quarantineNewEntries: false }
|
||||
: connection.transportConfig;
|
||||
const [updatedConnection] = await db
|
||||
.update(toolConnections)
|
||||
.set({
|
||||
config: normalizedConfig,
|
||||
transportConfig: normalizedTransportConfig,
|
||||
healthStatus: "ok",
|
||||
healthMessage: "Tool catalog refreshed.",
|
||||
healthCheckedAt: now,
|
||||
@@ -4264,15 +4277,25 @@ export function toolAccessService(db: Db, options: ToolAccessServiceOptions = {}
|
||||
const activeEntries = updatedEntries.filter((entry) => entry.status === "active");
|
||||
await enableCatalogEntriesByDefault({
|
||||
connection: updatedConnection,
|
||||
newCatalogEntryIds: activeEntries
|
||||
.filter((entry) => {
|
||||
const previous = existingByName.get(entry.toolName);
|
||||
return !previous || previous.status === "quarantined";
|
||||
})
|
||||
.map((entry) => entry.id),
|
||||
newCatalogEntryIds: refreshOptions.enableAllByDefault
|
||||
? activeEntries.map((entry) => entry.id)
|
||||
: activeEntries
|
||||
.filter((entry) => {
|
||||
const previous = existingByName.get(entry.toolName);
|
||||
return !previous || previous.status === "quarantined";
|
||||
})
|
||||
.map((entry) => entry.id),
|
||||
activeCatalogEntryIds: activeEntries.map((entry) => entry.id),
|
||||
actor,
|
||||
});
|
||||
if (refreshOptions.enableAllByDefault) {
|
||||
await upsertAskFirstPolicies({
|
||||
companyId: updatedConnection.companyId,
|
||||
connection: updatedConnection,
|
||||
askFirstEntries: [],
|
||||
actor,
|
||||
});
|
||||
}
|
||||
|
||||
await audit({
|
||||
companyId: connection.companyId,
|
||||
@@ -6631,7 +6654,7 @@ export function toolAccessService(db: Db, options: ToolAccessServiceOptions = {}
|
||||
}
|
||||
throw error;
|
||||
}
|
||||
const refresh = await refreshCatalog(connectionRow.id, actor);
|
||||
const refresh = await refreshCatalog(connectionRow.id, actor, { enableAllByDefault: true });
|
||||
const [application] = await db.select().from(toolApplications).where(eq(toolApplications.id, applicationRow.id));
|
||||
return {
|
||||
connectionId: refresh.connection.id,
|
||||
@@ -7072,7 +7095,9 @@ export function toolAccessService(db: Db, options: ToolAccessServiceOptions = {}
|
||||
.where(eq(toolConnections.id, connection.id))
|
||||
.returning();
|
||||
await syncCredentialBindings(updated);
|
||||
return checkConnectionHealth(updated.id, actor);
|
||||
const health = await checkConnectionHealth(updated.id, actor);
|
||||
const refresh = await refreshCatalog(updated.id, actor, { enableAllByDefault: true });
|
||||
return { ...health, connection: refresh.connection };
|
||||
}
|
||||
|
||||
async function startOAuth(
|
||||
@@ -7532,7 +7557,7 @@ export function toolAccessService(db: Db, options: ToolAccessServiceOptions = {}
|
||||
await syncCredentialBindings(connection);
|
||||
|
||||
await checkConnectionHealth(connection.id, input.actor);
|
||||
const refresh = await refreshCatalog(connection.id, input.actor);
|
||||
const refresh = await refreshCatalog(connection.id, input.actor, { enableAllByDefault: true });
|
||||
const [application] = await db.select().from(toolApplications).where(eq(toolApplications.id, connection.applicationId));
|
||||
return {
|
||||
connectionId: refresh.connection.id,
|
||||
|
||||
@@ -388,6 +388,16 @@ describe("AppDetail", () => {
|
||||
expect(container.textContent).not.toContain("Action permissions");
|
||||
});
|
||||
|
||||
it("explains that MCP actions can take a minute while Test loads", async () => {
|
||||
mockParams.tab = "test";
|
||||
listCatalogMock.mockImplementation(() => new Promise(() => undefined));
|
||||
|
||||
await renderAppDetail();
|
||||
|
||||
expect(container.textContent).toContain("Loading MCP actions, this may take a minute.");
|
||||
expect(container.querySelector(".animate-spin")).toBeTruthy();
|
||||
});
|
||||
|
||||
it("hides secret URL parameters in setup technical details", async () => {
|
||||
mockParams.tab = "setup";
|
||||
getConnectionMock.mockResolvedValue(
|
||||
@@ -700,6 +710,35 @@ describe("AppDetail", () => {
|
||||
]);
|
||||
});
|
||||
|
||||
it("removes an existing agent grant directly from Permissions", async () => {
|
||||
mockParams.tab = "permissions";
|
||||
listProfilesMock.mockResolvedValue({
|
||||
profiles: [{
|
||||
profileKey: "app:conn-1",
|
||||
entries: [
|
||||
{ effect: "include", catalogEntryId: "catalog-read" },
|
||||
{ effect: "include", catalogEntryId: "catalog-write" },
|
||||
],
|
||||
bindings: [{ targetType: "agent", targetId: "agent-1" }],
|
||||
}],
|
||||
});
|
||||
|
||||
await renderAppDetail();
|
||||
|
||||
const remove = container.querySelector<HTMLButtonElement>('button[aria-label="Remove Coder access"]');
|
||||
expect(remove).toBeTruthy();
|
||||
await act(async () => {
|
||||
remove!.dispatchEvent(new MouseEvent("click", { bubbles: true }));
|
||||
});
|
||||
await flushReact();
|
||||
|
||||
expect(finishAppMock).toHaveBeenCalledWith("company-1", "conn-1", {
|
||||
enabledCatalogEntryIds: ["catalog-read", "catalog-write"],
|
||||
askFirstCatalogEntryIds: ["catalog-write"],
|
||||
access: { agentIds: [] },
|
||||
});
|
||||
});
|
||||
|
||||
it("renders activity attribution with issue context and human resolver names", async () => {
|
||||
mockParams.tab = "activity";
|
||||
listConnectionActivityMock.mockResolvedValue({
|
||||
|
||||
@@ -520,7 +520,7 @@ export function AppDetail() {
|
||||
catalogQuery.isError
|
||||
? <ToolsLoadError onRetry={() => { void catalogQuery.refetch(); }} />
|
||||
: catalogQuery.isLoading
|
||||
? <ToolsLoading />
|
||||
? <ToolsLoading mcpActions />
|
||||
: <TestPanel connectionId={connectionId} appName={appName} active={active} quarantined={quarantined} />
|
||||
)}
|
||||
{activeTab === "activity" && (
|
||||
@@ -636,11 +636,11 @@ function AppDetailHeader({
|
||||
);
|
||||
}
|
||||
|
||||
function ToolsLoading() {
|
||||
function ToolsLoading({ mcpActions = false }: { mcpActions?: boolean }) {
|
||||
return (
|
||||
<div className="flex items-center gap-2 py-8 text-sm text-muted-foreground" role="status">
|
||||
<Loader2 className="h-4 w-4 animate-spin" />
|
||||
Loading tools…
|
||||
{mcpActions ? "Loading MCP actions, this may take a minute." : "Loading tools…"}
|
||||
</div>
|
||||
);
|
||||
}
|
||||
|
||||
@@ -205,7 +205,7 @@ describe("Browse store door (PAP-13254 door 1)", () => {
|
||||
it("shows the connected owner, edits existing connections, and offers a deliberate second account", async () => {
|
||||
listApplicationsMock.mockResolvedValue({
|
||||
applications: [
|
||||
{ id: "app-notion", status: "active", applicationKey: "app-gallery:notion:one", metadata: {} },
|
||||
{ id: "app-notion", name: "Legacy integration", status: "active", applicationKey: "legacy:notion", metadata: {} },
|
||||
],
|
||||
});
|
||||
listConnectionsMock.mockResolvedValue({
|
||||
@@ -216,6 +216,8 @@ describe("Browse store door (PAP-13254 door 1)", () => {
|
||||
name: "Notion",
|
||||
status: "active",
|
||||
createdByUserId: "user-1",
|
||||
config: { sourceTemplateKey: "notion" },
|
||||
transportConfig: {},
|
||||
},
|
||||
{ id: "conn-two", applicationId: "app-notion", status: "disabled" },
|
||||
{ id: "conn-draft", applicationId: "app-notion", status: "draft" },
|
||||
@@ -255,7 +257,7 @@ describe("Browse store door (PAP-13254 door 1)", () => {
|
||||
const addAnother = container.querySelector<HTMLButtonElement>(
|
||||
'button[aria-label="Add another Notion account"]',
|
||||
);
|
||||
expect(addAnother?.textContent).toContain("Add another account");
|
||||
expect(addAnother?.textContent).toContain("Add new");
|
||||
await act(async () => {
|
||||
addAnother?.dispatchEvent(new MouseEvent("click", { bubbles: true }));
|
||||
});
|
||||
|
||||
@@ -119,6 +119,10 @@ export function Browse() {
|
||||
}, [gallery, trimmed]);
|
||||
const connectionSummaryBySlug = useMemo(() => {
|
||||
const connections = connectionsQuery.data?.connections ?? [];
|
||||
const gallerySlugs = new Set(gallery.map((entry) => appDefinitionSlug(entry)));
|
||||
const gallerySlugByName = new Map(
|
||||
gallery.map((entry) => [appDefinitionName(entry).trim().toLowerCase(), appDefinitionSlug(entry)]),
|
||||
);
|
||||
const connectionsByApplicationId = new Map<string, typeof connections>();
|
||||
for (const connection of connections) {
|
||||
if (connection.status === "archived" || connection.status === "draft") continue;
|
||||
@@ -135,9 +139,17 @@ export function Browse() {
|
||||
}>();
|
||||
for (const application of applicationsQuery.data?.applications ?? []) {
|
||||
if (application.status === "archived") continue;
|
||||
const slug = appApplicationSourceSlug(application);
|
||||
if (!slug) continue;
|
||||
const appConnections = connectionsByApplicationId.get(application.id) ?? [];
|
||||
const configuredConnectionSlug = appConnections
|
||||
.map((connection) => connection.config?.sourceTemplateKey ?? connection.transportConfig?.sourceTemplateKey)
|
||||
.find((value): value is string => typeof value === "string" && gallerySlugs.has(value));
|
||||
const applicationSlug = appApplicationSourceSlug(application);
|
||||
const slug = applicationSlug && gallerySlugs.has(applicationSlug)
|
||||
? applicationSlug
|
||||
: configuredConnectionSlug
|
||||
?? gallerySlugByName.get(application.name.trim().toLowerCase())
|
||||
?? null;
|
||||
if (!slug) continue;
|
||||
const current = summaries.get(slug);
|
||||
summaries.set(slug, {
|
||||
applicationId: current?.applicationId ?? application.id,
|
||||
@@ -146,7 +158,7 @@ export function Browse() {
|
||||
});
|
||||
}
|
||||
return summaries;
|
||||
}, [applicationsQuery.data, connectionsQuery.data]);
|
||||
}, [applicationsQuery.data, connectionsQuery.data, gallery]);
|
||||
const userProfileById = useMemo(
|
||||
() => buildCompanyUserProfileMap(userDirectoryQuery.data?.users),
|
||||
[userDirectoryQuery.data],
|
||||
@@ -321,7 +333,7 @@ function AppTile({
|
||||
className="text-xs text-muted-foreground transition-colors hover:text-foreground"
|
||||
aria-label={`Add another ${appName} account`}
|
||||
>
|
||||
Add another account
|
||||
Add new
|
||||
</button>
|
||||
)}
|
||||
</div>
|
||||
@@ -365,7 +377,7 @@ function AppTile({
|
||||
className="text-xs text-muted-foreground transition-colors hover:text-foreground"
|
||||
aria-label={`Add another ${appName} account`}
|
||||
>
|
||||
Add another account
|
||||
Add new
|
||||
</button>
|
||||
)}
|
||||
</div>
|
||||
|
||||
@@ -1,7 +1,8 @@
|
||||
import { useEffect, useRef, useState } from "react";
|
||||
import { Loader2, PackageCheck, RefreshCw } from "lucide-react";
|
||||
import { Loader2, PackageCheck, RefreshCw, X } from "lucide-react";
|
||||
import type { Agent, ToolCatalogEntry } from "@paperclipai/shared";
|
||||
import { useSearchParams } from "@/lib/router";
|
||||
import { AgentIcon } from "@/components/AgentIconPicker";
|
||||
import { Button } from "@/components/ui/button";
|
||||
import { Checkbox } from "@/components/ui/checkbox";
|
||||
import { AgentMultiSelect } from "@/components/AgentMultiSelect";
|
||||
@@ -105,9 +106,11 @@ function AccessSection({
|
||||
const summary =
|
||||
access.mode === "all"
|
||||
? "Every agent can use it"
|
||||
: `${access.agentIds.size} ${access.agentIds.size === 1 ? "agent" : "agents"} can use it`;
|
||||
: access.agentIds.size === 0
|
||||
? "No agents can use it"
|
||||
: `${access.agentIds.size} ${access.agentIds.size === 1 ? "agent" : "agents"} can use it`;
|
||||
|
||||
const canSave = draft.mode === "all" || draft.agentIds.size > 0;
|
||||
const grantedAgents = liveAgents.filter((agent) => access.agentIds.has(agent.id));
|
||||
|
||||
return (
|
||||
<section>
|
||||
@@ -123,6 +126,30 @@ function AccessSection({
|
||||
)}
|
||||
</div>
|
||||
|
||||
{!editing && access.mode === "specific" && grantedAgents.length > 0 && (
|
||||
<div className="space-y-0.5 pt-3">
|
||||
{grantedAgents.map((agent) => (
|
||||
<div key={agent.id} className="flex items-center gap-2 px-1.5 py-1 text-sm">
|
||||
<AgentIcon icon={agent.icon ?? null} className="h-4 w-4 shrink-0 text-muted-foreground" />
|
||||
<span className="min-w-0 flex-1 truncate text-foreground">{agent.name}</span>
|
||||
<button
|
||||
type="button"
|
||||
aria-label={`Remove ${agent.name} access`}
|
||||
disabled={disabled}
|
||||
className="rounded-sm p-1 text-muted-foreground transition-colors hover:bg-accent hover:text-foreground disabled:cursor-not-allowed disabled:opacity-50"
|
||||
onClick={() => {
|
||||
const nextAgentIds = new Set(access.agentIds);
|
||||
nextAgentIds.delete(agent.id);
|
||||
onSave({ mode: "specific", agentIds: nextAgentIds });
|
||||
}}
|
||||
>
|
||||
<X className="h-3.5 w-3.5" />
|
||||
</button>
|
||||
</div>
|
||||
))}
|
||||
</div>
|
||||
)}
|
||||
|
||||
{editing && (
|
||||
<div className="space-y-3 pt-4">
|
||||
<label className="flex items-start gap-3">
|
||||
@@ -162,7 +189,7 @@ function AccessSection({
|
||||
<div className="flex items-center gap-2 pt-1">
|
||||
<Button
|
||||
size="sm"
|
||||
disabled={disabled || !canSave}
|
||||
disabled={disabled}
|
||||
onClick={() => {
|
||||
onSave(draft);
|
||||
setEditing(false);
|
||||
|
||||
@@ -219,6 +219,16 @@ afterEach(() => {
|
||||
});
|
||||
|
||||
describe("TestPanel", () => {
|
||||
it("pairs the loading skeleton with explicit MCP wait copy and animation", async () => {
|
||||
listTestAgentsMock.mockImplementation(() => new Promise(() => undefined));
|
||||
|
||||
await act(async () => renderPanel());
|
||||
|
||||
expect(container.textContent).toContain("Loading MCP actions, this may take a minute.");
|
||||
expect(container.querySelector(".animate-spin")).toBeTruthy();
|
||||
expect(container.querySelectorAll('[data-slot="skeleton"]')).not.toHaveLength(0);
|
||||
});
|
||||
|
||||
it("renders the Test-as header and grouped actions with access badges", async () => {
|
||||
await act(async () => renderPanel());
|
||||
await flushReact();
|
||||
|
||||
@@ -201,6 +201,10 @@ export function TestPanel({
|
||||
if (testAgentsQuery.isLoading) {
|
||||
return (
|
||||
<div className="space-y-4">
|
||||
<div className="flex items-center gap-2 text-sm text-muted-foreground" role="status">
|
||||
<Loader2 className="h-4 w-4 animate-spin" />
|
||||
Loading MCP actions, this may take a minute.
|
||||
</div>
|
||||
<Skeleton className="h-20 w-full" />
|
||||
<Skeleton className="h-12 w-full" />
|
||||
<Skeleton className="h-12 w-full" />
|
||||
|
||||
Reference in new issue
Block a user