diff --git a/ui/src/features/connections/ConnectionSetupFlow.tsx b/ui/src/features/connections/ConnectionSetupFlow.tsx index 7b8ce01ad43..6cb7270778c 100644 --- a/ui/src/features/connections/ConnectionSetupFlow.tsx +++ b/ui/src/features/connections/ConnectionSetupFlow.tsx @@ -74,6 +74,7 @@ import { resolveAuthorizationTarget } from "@/lib/authorizationUrl"; import { navigateTopLevel } from "@/lib/browserNavigation"; import { prepareOAuthNavigation, savePendingCloudHandoff } from "@/lib/oauthHandoff"; import { redactUrlSecrets } from "@/lib/redact-url-secrets"; +import { askFirstCatalogEntryIdsFor } from "./connection-defaults"; import { AppLogo } from "@/pages/apps/AppLogo"; import { appApplicationSourceSlug } from "@/pages/apps/app-definition-display"; import { UnverifiedServerBadge } from "@/pages/apps/UnverifiedServerBadge"; @@ -1404,7 +1405,12 @@ function StandardConnectionSetupFlow({ const guidance = genericConnectGuidance(code, error instanceof Error ? error.message : null); setLinkGuidance(guidance); setGenericOAuthPending(false); - if (guidance.focus === "credentials") setLinkAdvancedOpen(true); + if (guidance.focus === "credentials") { + // The probe is the answer to "does it need a key?", so the field + // appears now rather than being offered as a guess beforehand. + setLinkNeedsKey(true); + setLinkAdvancedOpen(true); + } return; } pushToast({ @@ -1681,16 +1687,10 @@ function StandardConnectionSetupFlow({ const enabledIds = Object.entries(enabledMap) .filter(([, on]) => on) .map(([id]) => id); - const askFirstRiskLevels = new Set( - Array.isArray(connected.suggestedDefaults.askFirstRiskLevels) - ? connected.suggestedDefaults.askFirstRiskLevels.filter( - (riskLevel): riskLevel is string => typeof riskLevel === "string", - ) - : [], + const askFirstIds = askFirstCatalogEntryIdsFor( + connected, + (catalogEntryId) => Boolean(enabledMap[catalogEntryId]), ); - const askFirstIds = connected.actions.canMakeChanges - .filter((action) => enabledMap[action.catalogEntryId] && askFirstRiskLevels.has(action.riskLevel)) - .map((action) => action.catalogEntryId); // The Access step asks one question about agent reach, so profile access // and installs are committed to the same target set instead of drifting // apart behind two separate wizard screens. @@ -2421,10 +2421,6 @@ function StandardConnectionSetupFlow({ link={linkUrl} name={linkName} needsKey={linkNeedsKey} - onNeedsKeyChange={(next) => { - setLinkNeedsKey(next); - if (!next) setLinkKey(""); - }} keyValue={linkKey} onKeyChange={setLinkKey} authMode={linkAuthMode} @@ -3057,7 +3053,6 @@ function LinkConnectStep({ link, name, needsKey, - onNeedsKeyChange, keyValue, onKeyChange, authMode, @@ -3081,7 +3076,6 @@ function LinkConnectStep({ link: string; name: string; needsKey: boolean; - onNeedsKeyChange: (next: boolean) => void; keyValue: string; onKeyChange: (next: string) => void; authMode: GenericMcpAuthMode; @@ -3117,7 +3111,10 @@ function LinkConnectStep({ oauthClientSecret, }; const canSubmit = canSubmitGenericConnect(draft); - const showSimpleKeyQuestion = authMode === "auto"; + // PAP-659 bucket H: the address alone is the whole default path. `needsKey` is + // now set by the server's probe after a credential challenge, never guessed at + // up front, so the field appears exactly when it is known to be required. + const showKeyField = (authMode === "auto" && needsKey) || authMode === "bearer"; const displayedLink = matchedEntry?.slug === "zapier" ? redactUrlSecrets(link) : link; const updateHeader = (id: string, patch: Partial) => { @@ -3152,30 +3149,7 @@ function LinkConnectStep({ ) : null}
- {showSimpleKeyQuestion && ( -
- -
- onNeedsKeyChange(false)} - /> - onNeedsKeyChange(true)} - /> -
-

- {needsKey - ? "Paste the key this app gave you." - : "Most servers just work from the address — pick Yes only if the server gave you a key, or if it asks you to sign in."} -

-
- )} - - {(showSimpleKeyQuestion && needsKey) || authMode === "bearer" ? ( + {showKeyField ? (
@@ -4338,6 +4312,7 @@ export function connectionDefaultSummarySentence(input: { */ export function ConnectionAccessDefaults({ companyId, + agents, sentence, notice, extra, @@ -4346,6 +4321,12 @@ export function ConnectionAccessDefaults({ ...accessProps }: Omit[0], "agents" | "agentsLoading" | "submitLabel" | "onBack" | "onContinue" | "pending" | "continuesToProvider" | "hideFooter" | "bare"> & { companyId: string; + /** + * Supplied by callers that already hold the agent list — design specimens and + * review stories, which must not reach the network. Omitted in the product, + * where the disclosure fetches its own. + */ + agents?: AgentMultiSelectOption[]; sentence: string; /** Shown when the resolved default genuinely cannot apply. Never a step. */ notice?: string[]; @@ -4387,7 +4368,9 @@ export function ConnectionAccessDefaults({
{extra} - {}} onContinue={() => {}} /> + {agents + ? {}} onContinue={() => {}} /> + : {}} onContinue={() => {}} />}
diff --git a/ui/src/features/connections/connection-defaults.test.ts b/ui/src/features/connections/connection-defaults.test.ts new file mode 100644 index 00000000000..f54c460fdc4 --- /dev/null +++ b/ui/src/features/connections/connection-defaults.test.ts @@ -0,0 +1,50 @@ +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) => + ({ catalogEntryId, riskLevel }) as never; + +describe("askFirstCatalogEntryIdsFor", () => { + const result = { + actions: { + readOnly: [action("read-1", "read")], + canMakeChanges: [ + action("write-1", "write"), + action("destroy-1", "destructive"), + action("odd-1", "medium"), + ], + }, + suggestedDefaults: { askFirstRiskLevels: [...DEFAULT_ASK_FIRST_RISK_LEVELS] }, + }; + + it("gates the write and destructive actions the server named", () => { + expect(askFirstCatalogEntryIdsFor(result, () => true)).toEqual(["write-1", "destroy-1"]); + }); + + it("leaves a risk level the policy did not name alone", () => { + // `medium` is not in the default set; gating it would be the classifier + // making policy instead of the policy function. + expect(askFirstCatalogEntryIdsFor(result, () => true)).not.toContain("odd-1"); + }); + + it("never gates an action that is switched off", () => { + expect(askFirstCatalogEntryIdsFor(result, (id) => id !== "write-1")).toEqual(["destroy-1"]); + }); + + it("gates nothing when the server sends an open policy", () => { + expect( + askFirstCatalogEntryIdsFor({ ...result, suggestedDefaults: { askFirstRiskLevels: [] } }, () => true), + ).toEqual([]); + }); + + it("gates nothing rather than throwing when the field is missing or malformed", () => { + expect(askFirstCatalogEntryIdsFor({ ...result, suggestedDefaults: {} }, () => true)).toEqual([]); + expect( + askFirstCatalogEntryIdsFor({ ...result, suggestedDefaults: undefined as never }, () => true), + ).toEqual([]); + expect( + askFirstCatalogEntryIdsFor({ ...result, suggestedDefaults: { askFirstRiskLevels: "write" } }, () => true), + ).toEqual([]); + }); +}); diff --git a/ui/src/features/connections/connection-defaults.ts b/ui/src/features/connections/connection-defaults.ts new file mode 100644 index 00000000000..85840c17d7f --- /dev/null +++ b/ui/src/features/connections/connection-defaults.ts @@ -0,0 +1,30 @@ +import type { ConnectToolAppResult } from "@paperclipai/shared"; + +/** + * Which of a fresh connection's actions start behind a human approval (PAP-659 + * C6a/C7). + * + * The runtime gate and its approval card already existed; what was missing was + * a default with anything behind it. This lives outside the wizard because two + * different setup paths commit connections — the catalog flow and the + * remote-MCP gateway flow — and a gate that is armed on only one of them is a + * gate you cannot reason about. The risk levels come from the server's + * `suggestedDefaults` rather than a constant here, so retuning the policy stays + * a one-line change in `recommendedDefaultsForApp`. + */ +export function askFirstCatalogEntryIdsFor( + result: Pick, + isEnabled: (catalogEntryId: string) => boolean, +): string[] { + const riskLevels = new Set( + Array.isArray(result.suggestedDefaults?.askFirstRiskLevels) + ? result.suggestedDefaults.askFirstRiskLevels.filter( + (riskLevel): riskLevel is string => typeof riskLevel === "string", + ) + : [], + ); + if (riskLevels.size === 0) return []; + return result.actions.canMakeChanges + .filter((action) => isEnabled(action.catalogEntryId) && riskLevels.has(action.riskLevel)) + .map((action) => action.catalogEntryId); +} diff --git a/ui/src/features/connections/remote-mcp/RemoteMcpConnectionSetup.tsx b/ui/src/features/connections/remote-mcp/RemoteMcpConnectionSetup.tsx index d68e05bd2c9..25cdd205333 100644 --- a/ui/src/features/connections/remote-mcp/RemoteMcpConnectionSetup.tsx +++ b/ui/src/features/connections/remote-mcp/RemoteMcpConnectionSetup.tsx @@ -8,11 +8,22 @@ import { Input } from "@/components/ui/input"; import { Label } from "@/components/ui/label"; import { Tooltip, TooltipContent, TooltipTrigger } from "@/components/ui/tooltip"; import { RemoteMcpManagement } from "./RemoteMcpManagement"; -import { AccessStepContent, StepHeader } from "../ConnectionSetupFlow"; +import { + AccessStepContent, + ConnectionAccessDefaults, + connectionDefaultSummarySentence, + StepHeader, +} from "../ConnectionSetupFlow"; import type { RemoteMcpProvider } from "./providers"; import type { RemoteMcpSetupActions, RemoteMcpSetupState } from "./types"; -const steps = ["access", "connect"] as const; +/** + * PAP-659 C0: the connect path is one screen. `access` is still a real screen, + * but only as management after the connection exists — the way in states the + * resolved default and puts its controls in the Advanced disclosure, the same + * as every catalog connector. + */ +const steps = ["connect"] as const; const selectClass = "h-9 w-full rounded-md border border-input bg-background px-3 text-sm focus-visible:outline-none focus-visible:ring-2 focus-visible:ring-ring disabled:opacity-50"; function FieldHelp({ label, children }: { label: string; children: ReactNode }) { @@ -25,7 +36,8 @@ function FieldHelp({ label, children }: { label: string; children: ReactNode }) /** Controlled presentation shared by provider setup, configuration imports and review stories. * Authentication, persistence and calls belong to the controller, never these views. */ -export function RemoteMcpConnectionSetup({ provider, state: s, actions: a, agents, connectionId, fixedGrantKind, lockedAgentId, host = "page", authorizationUrl, upstreamServiceName }: { +export function RemoteMcpConnectionSetup({ provider, state: s, actions: a, agents, companyId, connectionId, fixedGrantKind, lockedAgentId, host = "page", authorizationUrl, upstreamServiceName }: { + companyId: string; upstreamServiceName?: string; host?: "page" | "dialog"; lockedAgentId?: string; @@ -53,6 +65,48 @@ export function RemoteMcpConnectionSetup({ provider, state: s, actions: a, agent ; const footer = (children: ReactNode) => {children}; + /** + * The stated default and the one Advanced disclosure (PAP-659 C0). + * + * Step 1 of this work learned the lesson the hard way on Gmail: adding an + * access disclosure beside a connector's existing "Advanced authentication" + * panel leaves two of them on one screen, which is worse than the step it + * replaced. So the provider's authentication settings are passed in here as + * `extra` and share a single panel with the access controls. + */ + // Deliberately not derived from `s.auth`. A gateway URL can carry a personal + // token even when the method declares `auth: "none"`, so the shared-vs-mine + // credential choice has to stay offered; deriving "none" here would silently + // remove it and pin every Zapier connection to the organization. + const authKind = "oauth"; + const defaults = (extra: ReactNode) => { if (grantKind !== "agent") change({ grantKind }); }} + installChoice={s.allAgents ? "all" : "specific"} + setInstallChoice={(choice) => change({ allAgents: choice === "all" })} + installAgentIds={new Set(s.agentIds)} + setInstallAgentIds={(ids) => change({ agentIds: [...ids] })} + lockedAgentId={lockedAgentId} + preserveAgentAccess={s.setupComplete} + />; + const error = s.connectStatus === "invalid_url" ? { title: "Enter a valid MCP URL", body: "Paste the complete server URL, including https:// or http://. A dashboard page is not an MCP endpoint." } : s.connectStatus === "oauth_failed" ? { title: `${provider.name} couldn’t connect`, body: "Authorization did not complete. Your saved connection is still here, so you can try again." } : s.connectStatus === "rejected" ? { title: "Credentials were rejected", body: `Check or replace the credentials from ${provider.name}, then reconnect. Your agent access and tool choices are preserved.` } @@ -62,8 +116,8 @@ export function RemoteMcpConnectionSetup({ provider, state: s, actions: a, agent return
= 0 && !s.setupComplete ? `Step ${currentStep + 1} of 2` : s.step === "draft" ? `Your ${provider.name} setup is ready to resume.` : s.step === "permissions" ? `Connected${s.identity ? ` as ${s.identity}` : ""} · ${s.tools.length} actions available` : `Manage this ${provider.name} connection.`} - step={currentStep >= 0 && !s.setupComplete ? "key" : "gallery"} activeIndex={currentStep} labels={["Access", "Connect"]} onCancel={busy || s.step === "management" || s.step === "permissions" || s.step === "draft" ? undefined : a.saveExit} /> + subtitle={currentStep >= 0 && !s.setupComplete ? `Paperclip will use ${provider.name} on your behalf.` : s.step === "draft" ? `Your ${provider.name} setup is ready to resume.` : s.step === "permissions" ? `Connected${s.identity ? ` as ${s.identity}` : ""} · ${s.tools.length} actions available` : `Manage this ${provider.name} connection.`} + step={currentStep >= 0 && !s.setupComplete ? "key" : "gallery"} activeIndex={currentStep} labels={steps.map(() => "Connect")} onCancel={busy || s.step === "management" || s.step === "permissions" || s.step === "draft" ? undefined : a.saveExit} />
{upstreamServiceName && {provider.name} is an external service that handles the connection and requests to {upstreamServiceName}. After connecting, the agent will verify the app and guide you through any additional authorization.} {s.notice &&

{s.notice}

} @@ -104,11 +158,10 @@ export function RemoteMcpConnectionSetup({ provider, state: s, actions: a, agent change({ url: event.target.value })} />

{provider.urlHelp}

-
{ if (event.currentTarget.open !== s.advanced) change({ advanced: event.currentTarget.open }); }}> - Advanced authentication -
+ {defaults(
+

Authentication

{provider.authHelp}

-
change({ auth: event.target.value as RemoteMcpSetupState["auth"] })}> {provider.supportsBrowserAuth && }
{s.auth === "bearer" &&
Paste the token only, without the word Bearer. It is kept in the connection’s credentials.
change({ token: event.target.value })} />
} @@ -121,11 +174,10 @@ export function RemoteMcpConnectionSetup({ provider, state: s, actions: a, agent
)}
} -
- +
)} {busy &&

Connecting and discovering tools…

} - {footer(<>)} + {footer(<>{s.setupComplete ? : })} } } diff --git a/ui/src/features/connections/remote-mcp/RemoteMcpDesignExample.tsx b/ui/src/features/connections/remote-mcp/RemoteMcpDesignExample.tsx index fe822d852c9..d5e4fef3ca9 100644 --- a/ui/src/features/connections/remote-mcp/RemoteMcpDesignExample.tsx +++ b/ui/src/features/connections/remote-mcp/RemoteMcpDesignExample.tsx @@ -8,7 +8,7 @@ function ConnectExample({ providerId }: { providerId: RemoteMcpProviderId }) { const provider = remoteMcpProviders[providerId]; const [state, setState] = useState({ step: "access", grantKind: "organization", setupComplete: false, url: provider.defaultUrl, - auth: provider.supportsBrowserAuth ? "auto" : "none", token: "", headers: [], advanced: false, connectStatus: "idle", + auth: provider.supportsBrowserAuth ? "auto" : "none", token: "", headers: [], connectStatus: "idle", connected: false, identity: null, allAgents: true, agentIds: [], permissions: {}, tools: [], notice: null, refreshing: false, }); @@ -17,7 +17,7 @@ function ConnectExample({ providerId }: { providerId: RemoteMcpProviderId }) { const actions: RemoteMcpSetupActions = { edit, navigate: (step) => edit({ step }), connect: explain, cancelConnect: explain, openProvider: explain, saveExit: explain, resumeDraft: explain, finish: explain, refresh: explain, reconnect: explain, disconnect: explain, }; - return ; + return ; } /** Design-guide specimen; full state matrix is maintained in each provider's Storybook group. */ diff --git a/ui/src/features/connections/remote-mcp/RemoteMcpProductionSetup.tsx b/ui/src/features/connections/remote-mcp/RemoteMcpProductionSetup.tsx index 42966566a76..d9245a98395 100644 --- a/ui/src/features/connections/remote-mcp/RemoteMcpProductionSetup.tsx +++ b/ui/src/features/connections/remote-mcp/RemoteMcpProductionSetup.tsx @@ -1,6 +1,7 @@ import { useEffect, useRef, useState } from "react"; import { useQuery, useQueryClient } from "@tanstack/react-query"; import { REMOTE_MCP_CONNECTOR_METHODS, type ToolConnection } from "@paperclipai/shared"; +import { askFirstCatalogEntryIdsFor } from "../connection-defaults"; import { RemoteMcpAccountChoice } from "./RemoteMcpAccountChoice"; import { readConnectionIntentOAuthOutcome, type ConnectionSetupFlowProps } from "../ConnectionSetupFlow"; import { agentsApi } from "@/api/agents"; @@ -45,11 +46,14 @@ export function RemoteMcpProductionSetup({ providerId, connection, host = "page" const busy = useRef(false); const authorizationUrl = useRef(undefined); const [state, setState] = useState(() => ({ - step: connection ? "connect" : "access", grantKind: connection ? connection.credentialPolicy === "per_user" ? "user" : "organization" : requestedAgentId ? "user" : "organization", + // PAP-659 C0: there is no Access step on the way in. The resolved default + // is stated on the connect screen and changed in its Advanced disclosure; + // `access` survives only as a management screen reached after connecting. + step: "connect", grantKind: connection ? connection.credentialPolicy === "per_user" ? "user" : "organization" : requestedAgentId ? "user" : "organization", setupComplete: Boolean(connection && connection.status !== "draft"), url: typeof connection?.config?.url === "string" ? connection.config?.url : provider.defaultUrl, auth: connection?.config?.mcpAuthMode === "bearer" ? "bearer" : connection?.authKind === "api_key" ? "headers" : provider.supportsBrowserAuth ? "auto" : "none", - token: "", headers: [], advanced: false, connectStatus: oauthOutcome === "denied" ? "cancelled" : oauthOutcome === "failed" ? "oauth_failed" : "idle", connected: false, + token: "", headers: [], connectStatus: oauthOutcome === "denied" ? "cancelled" : oauthOutcome === "failed" ? "oauth_failed" : "idle", connected: false, identity: null, allAgents: true, agentIds: [], permissions: {}, tools: [], notice: connection?.authKind === "api_key" ? "Saved credentials are retained when these fields are left blank. Enter a replacement only to change them." : null, refreshing: false, ...(!connection ? readAccessDraft(accessDraftKey) : {}), ...(requestedAgentId ? { allAgents: false, agentIds: [requestedAgentId] } : {}), @@ -149,9 +153,14 @@ export function RemoteMcpProductionSetup({ providerId, connection, host = "page" popup.current = null; // The server retains existing permissions when reconnecting. Only a fresh setup enables everything. { + const enabled = new Set(result.catalog.filter((tool) => tool.status === "active").map((tool) => tool.id)); await toolsApi.finishApp(selectedCompanyId, result.connectionId, { - enabledCatalogEntryIds: result.catalog.filter((tool) => tool.status === "active").map((tool) => tool.id), - askFirstCatalogEntryIds: [], access: requestedAgentId ? { agentIds: [requestedAgentId] } : state.allAgents ? "all_agents" : { agentIds: state.agentIds }, + enabledCatalogEntryIds: [...enabled], + // PAP-659 C6a/C7: this path used to send an empty list, so the four + // gateway connectors were the one place the armed write gate did not + // apply. The policy now comes from the same helper as the catalog flow. + askFirstCatalogEntryIds: askFirstCatalogEntryIdsFor(result, (id) => enabled.has(id)), + access: requestedAgentId ? { agentIds: [requestedAgentId] } : state.allAgents ? "all_agents" : { agentIds: state.agentIds }, ...(requestedAgentId ? { preserveExistingAccess: true } : {}), }); } @@ -187,5 +196,5 @@ export function RemoteMcpProductionSetup({ providerId, connection, host = "page" void onUseExisting(id).catch((error) => { setChoiceError(error instanceof Error ? error.message : "Could not use this connection."); setChoicePending(null); }); }} />; if (connection && !installs.data) return

{installs.isError ? "Could not load saved access. Retry before changing this connection." : "Loading saved access…"}

{installs.isError && }
; - return ; + return ; } diff --git a/ui/src/features/connections/remote-mcp/types.ts b/ui/src/features/connections/remote-mcp/types.ts index 5d94314c670..ebcea379102 100644 --- a/ui/src/features/connections/remote-mcp/types.ts +++ b/ui/src/features/connections/remote-mcp/types.ts @@ -15,7 +15,6 @@ export interface RemoteMcpSetupState { auth: "auto" | "bearer" | "headers" | "none"; token: string; headers: { id: string; name: string; value: string }[]; - advanced: boolean; connectStatus: ConnectStatus; connected: boolean; identity: string | null; diff --git a/ui/src/pages/apps/AppsConnect.test.tsx b/ui/src/pages/apps/AppsConnect.test.tsx index 20c878137f1..d09f0de5d2d 100644 --- a/ui/src/pages/apps/AppsConnect.test.tsx +++ b/ui/src/pages/apps/AppsConnect.test.tsx @@ -370,22 +370,37 @@ describe("AppsConnect — Connect with a link (M4 frame)", () => { const onComplete = vi.fn(); const popup = vi.spyOn(window, "open").mockReturnValue(null); const connection = { id: "conn-inline", status: "draft", credentialPolicy: "per_user", authKind: "api_key" }; - connectAppMock.mockResolvedValue({ connectionId: connection.id, connection, catalog: [{ id: "tool-1", status: "active" }] }); + connectAppMock.mockResolvedValue({ + connectionId: connection.id, connection, + catalog: [{ id: "tool-1", status: "active" }, { id: "tool-2", status: "active" }], + actions: { + readOnly: [{ catalogEntryId: "tool-1", riskLevel: "read" }], + canMakeChanges: [{ catalogEntryId: "tool-2", riskLevel: "write" }], + }, + suggestedDefaults: { askFirstRiskLevels: ["write", "destructive", "high", "critical"] }, + }); await render(undefined, false, ); - expect(container.textContent).toContain("This task grants access only to Ada"); + // PAP-659 C0: the gateway connectors reach the endpoint field immediately. + // The task-scoped reach is stated rather than confirmed on its own step. + const connectLabel = `Connect ${provider[0].toUpperCase()}${provider.slice(1)}`; + expect(container.textContent).toContain("available to the agent that asked for it"); expect(radioContaining("Any agent")).toBeUndefined(); - await passAccessStep(); expect(container.textContent).toContain("MCP server URL"); expect(container.textContent).not.toContain("Add your key"); + expect(container.textContent).not.toContain("Step 1 of 2"); const url = container.querySelector('input[type="password"]')!; expect(url.value).toBe(provider === "composio" ? "https://connect.composio.dev/mcp" : ""); - expect(buttonByText("Connect")?.disabled).toBe(provider !== "composio"); + expect(buttonByText(connectLabel)?.disabled).toBe(provider !== "composio"); await act(async () => setInputValue(url, "not a url")); - await act(async () => buttonByText("Connect")!.click()); + await act(async () => buttonByText(connectLabel)!.click()); expect(container.textContent).toContain("Enter a valid MCP URL"); expect(connectAppMock).not.toHaveBeenCalled(); + await act(async () => setInputValue(url, "https://provider.example/mcp")); + // The sign-in method shares the one Advanced disclosure with the access + // controls, and Radix unmounts collapsed content — so it has to be opened. + expect(container.querySelector("select")).toBeNull(); + await act(async () => buttonByText("Change")!.click()); await act(async () => { - setInputValue(url, "https://provider.example/mcp"); const auth = container.querySelector("select")!; auth.value = "bearer"; auth.dispatchEvent(new Event("change", { bubbles: true })); @@ -395,7 +410,9 @@ describe("AppsConnect — Connect with a link (M4 frame)", () => { await act(async () => buttonByText("Try again")!.click()); await vi.waitFor(() => expect(onComplete).toHaveBeenCalledWith({ connectionId: connection.id })); expect(connectAppMock).toHaveBeenCalledWith("company-1", expect.objectContaining({ galleryKey: provider, link: "https://provider.example/mcp", grantKind: "user", authMode: "bearer", credentialValues: { "credentials.authorization": "fixture-token" } })); - expect(finishAppMock).toHaveBeenCalledWith("company-1", connection.id, { enabledCatalogEntryIds: ["tool-1"], askFirstCatalogEntryIds: [], access: { agentIds: ["agent-1"] }, preserveExistingAccess: true }); + // PAP-659 C6a/C7: this path used to send an empty ask-first list, so the + // gateway connectors were the one place the armed write gate did not apply. + expect(finishAppMock).toHaveBeenCalledWith("company-1", connection.id, { enabledCatalogEntryIds: ["tool-1", "tool-2"], askFirstCatalogEntryIds: ["tool-2"], access: { agentIds: ["agent-1"] }, preserveExistingAccess: true }); expect(putConnectionInstallsMock).not.toHaveBeenCalled(); expect(mockNavigate).not.toHaveBeenCalled(); expect(popup).not.toHaveBeenCalled(); @@ -409,9 +426,9 @@ describe("AppsConnect — Connect with a link (M4 frame)", () => { const connection = { id: "conn-inline-oauth", status: "draft", credentialPolicy: "per_user", authKind: "oauth" }; connectAppMock.mockResolvedValue({ connectionId: connection.id, connection, catalog: [], auth: { kind: "oauth" } }); const root = await render(undefined, false, ); - await passAccessStep(); + const connectLabel = `Connect ${provider[0].toUpperCase()}${provider.slice(1)}`; await act(async () => setInputValue(container.querySelector('input[type="password"]')!, "https://provider.example/mcp")); - await act(async () => buttonByText("Connect")!.click()); + await act(async () => buttonByText(connectLabel)!.click()); await vi.waitFor(() => expect(startOAuthMock).toHaveBeenCalledWith(connection.id, { asCurrentUser: true, interactionId: "intent-inline" })); expect(popup.location.assign).toHaveBeenCalled(); popup.closed = true; @@ -442,7 +459,6 @@ describe("AppsConnect — Connect with a link (M4 frame)", () => { getConnectionMock.mockResolvedValue(connection); const content = ; const root = await render(undefined, false, content); - await passAccessStep(); await act(async () => setInputValue(container.querySelector('input[type="password"]')!, "https://provider.example/mcp?token=fixture-secret")); await act(async () => buttonByText("Save & exit")!.click()); await vi.waitFor(() => expect(onCancel).toHaveBeenCalled()); @@ -456,7 +472,8 @@ describe("AppsConnect — Connect with a link (M4 frame)", () => { await render(undefined, false, content); await vi.waitFor(() => expect(container.textContent).toContain("MCP server URL")); expect(getConnectionMock).toHaveBeenCalledWith(connection.id); - expect(container.textContent).toContain("Step 2 of 2"); + // Resuming lands on the same single screen it was saved from, not "step 2". + expect(container.textContent).not.toContain("Step 2 of 2"); }); it("inline aggregator reuses an eligible account without changing its access", async () => { @@ -520,19 +537,22 @@ describe("AppsConnect — Connect with a link (M4 frame)", () => { }); it.each([ - ["shared", "Any human in the organization", false], - ["per_user", "Just me", true], - ])("retains the %s identity when returning to Access after an OAuth return", async (credentialPolicy, identityLabel, asCurrentUser) => { + ["shared", "Connects for everyone in your organization", "Any human in the organization", false], + ["per_user", "Connects as you", "Just me", true], + ])("retains the %s identity across an OAuth return without a step to walk back through", async (credentialPolicy, statedDefault, identityLabel, asCurrentUser) => { const draft = { id: "conn-oauth-draft", status: "draft", authKind: "oauth", credentialPolicy, config: { sourceTemplateKey: "composio", connectionMethodKey: "mcp", url: "https://connect.composio.dev/mcp" } }; mockSearch.value = `source=composio&resume=${draft.id}&oauth=denied`; getConnectionMock.mockResolvedValue(draft); connectAppMock.mockResolvedValue({ connectionId: draft.id, connection: draft, catalog: [], auth: { kind: "oauth" } }); await render(); - await vi.waitFor(() => expect(buttonByText("Back")).toBeTruthy()); - await act(async () => buttonByText("Back")!.click()); + // PAP-659 C0: the retained identity is stated on the one screen instead of + // being re-confirmed on an Access step the operator has to navigate back to. + await vi.waitFor(() => expect(container.textContent).toContain(statedDefault)); + // It is also still editable, one disclosure away. Radix unmounts collapsed + // content, so the controls have to be opened to be asserted at all. + await act(async () => buttonByText("Change")!.click()); expect(container.textContent).toContain(identityLabel); expect(container.querySelector('[role="radiogroup"][aria-label="Which humans can use this credential?"]')).toBeNull(); - await act(async () => buttonByText("Continue")!.click()); await act(async () => buttonByText("Try again")!.click()); await vi.waitFor(() => expect(startOAuthMock).toHaveBeenCalledWith(draft.id, { asCurrentUser })); expect(connectAppMock).toHaveBeenCalledWith("company-1", expect.objectContaining({ resumeConnectionId: draft.id, grantKind: asCurrentUser ? "user" : "organization" })); @@ -595,18 +615,20 @@ describe("AppsConnect — Connect with a link (M4 frame)", () => { expect(container.querySelector('input[aria-label="MCP server URL"]')?.value).toBe("https://edited.example.test/mcp"); }); - it("an unrecognized URL routes to a minimal frame with the URL and key choice", async () => { + it("an unrecognized URL routes to a minimal frame that asks nothing but the address", async () => { await render(); await gotoLinkFrame(container, "https://www.example.com/actions"); expect(container.textContent).toContain("Connect your own MCP server"); expect(container.textContent).toContain("https://www.example.com/actions"); - expect(container.textContent).toContain("Does it need a key?"); - expect(Array.from(container.querySelectorAll("label")).find( - (label) => label.textContent === "Does it need a key?", - )?.classList.contains("mr-2")).toBe(true); - expect(buttonByText("No")).toBeTruthy(); - expect(buttonByText("Yes")).toBeTruthy(); + // PAP-659 bucket H: the server's probe answers "does it need a key?", so the + // operator is no longer asked to guess at it before anything has been tried. + expect(container.textContent).not.toContain("Does it need a key?"); + expect(buttonByText("No")).toBeUndefined(); + expect(buttonByText("Yes")).toBeUndefined(); + expect( + Array.from(container.querySelectorAll("input")).some((i) => i.type === "password"), + ).toBe(false); expect(container.querySelector('input[placeholder="My app"]')).toBeNull(); }); @@ -2801,11 +2823,19 @@ describe("AppsConnect — Connect with a link (M4 frame)", () => { expect(input.credentialValues).toBeUndefined(); }); - it("choosing Yes reveals one masked key field without extra reassurance copy", async () => { + it("reveals one masked key field only after the probe says a credential is needed", async () => { + // PAP-659 bucket H. The first press asks the server; the server is what + // knows. Only its credential challenge puts a key field on screen, and the + // second press carries the key. + connectAppMock.mockRejectedValueOnce( + new ApiError("This app needs you to sign in.", 502, { + error: "This app needs you to sign in.", + details: { code: "oauth_challenge" }, + }), + ); await render(); await gotoLinkFrame(container, "https://www.example.com/actions"); - // No key field while No is selected. expect( Array.from(container.querySelectorAll("input")).some( (i) => i.type === "password", @@ -2813,10 +2843,12 @@ describe("AppsConnect — Connect with a link (M4 frame)", () => { ).toBe(false); await act(async () => { - buttonByText("Yes")?.dispatchEvent(new MouseEvent("click", { bubbles: true })); + buttonByText("Check link")?.dispatchEvent(new MouseEvent("click", { bubbles: true })); }); await flushReact(); + await flushReact(); + expect(container.textContent).toContain("This server wants a credential"); const passwordInputs = Array.from( container.querySelectorAll("input"), ).filter((i) => i.type === "password"); @@ -2831,8 +2863,8 @@ describe("AppsConnect — Connect with a link (M4 frame)", () => { }); await flushReact(); - expect(connectAppMock).toHaveBeenCalledTimes(1); - const [, input] = connectAppMock.mock.calls[0]; + expect(connectAppMock).toHaveBeenCalledTimes(2); + const [, input] = connectAppMock.mock.calls[1]; expect(input.credentialValues).toEqual({ "credentials.authorization": "secret-key" }); }); @@ -2916,22 +2948,27 @@ describe("AppsConnect — Connect with a link (M4 frame)", () => { }); await render(); - expect(container.textContent).toContain("Step 1 of 2"); + // PAP-659 C0: no step counter and no Access step — the endpoint field is + // the first and only thing on the way in, with the default stated above it. + expect(container.textContent).not.toContain("Step 1 of 2"); + expect(container.textContent).not.toContain("Step 2 of 2"); expect(container.textContent).toContain("Connect Zapier"); - expect(container.textContent).toContain("Which humans can use this credential?"); - expect(container.textContent).toContain("Which agents can use this connection?"); + expect(container.textContent).toContain("Connects for everyone in your organization, available to all agents."); expect(container.querySelector('[data-remote-mcp-provider="zapier"]')).toBeTruthy(); expect(container.textContent).not.toContain("Pick the app you want your agents to use."); - expect(container.querySelector('input[placeholder="Paste the full URL from Zapier"]')).toBeNull(); + expect(container.textContent).toContain("MCP server URL"); + // The two questions the deleted step asked are still answerable, together, + // one disclosure away. + expect(container.textContent).not.toContain("Which humans can use this credential?"); + await act(async () => buttonByText("Change")!.click()); + expect(container.textContent).toContain("Which humans can use this credential?"); + expect(container.textContent).toContain("Which agents can use this connection?"); await act(async () => { radioContaining("Just me")?.dispatchEvent(new MouseEvent("click", { bubbles: true })); }); await flushReact(); - await passAccessStep(); - - expect(container.textContent).toContain("Step 2 of 2"); - expect(container.textContent).toContain("MCP server URL"); + expect(container.textContent).toContain("Connects as you, available to all agents."); const linkInput = container.querySelector( 'input[placeholder="Paste the full URL from Zapier"]', @@ -2943,7 +2980,7 @@ describe("AppsConnect — Connect with a link (M4 frame)", () => { await flushReact(); await act(async () => { - buttonByText("Connect")?.dispatchEvent(new MouseEvent("click", { bubbles: true })); + buttonByText("Connect Zapier")?.dispatchEvent(new MouseEvent("click", { bubbles: true })); }); await flushReact(); diff --git a/ui/storybook/fixtures/remoteMcpConnections.ts b/ui/storybook/fixtures/remoteMcpConnections.ts index e5869e76b3f..2f3cde27e21 100644 --- a/ui/storybook/fixtures/remoteMcpConnections.ts +++ b/ui/storybook/fixtures/remoteMcpConnections.ts @@ -86,7 +86,7 @@ export function initialReviewState(provider: RemoteMcpProviderId, scenario: Revi url: ["initial", "journey", "connect", "selected_agents"].includes(scenario) ? config.defaultUrl : scenario === "invalid_url" ? "not-a-server-url" : exampleUrl(provider), auth: scenario === "advanced" ? provider === "composio" ? "headers" : "bearer" : config.supportsBrowserAuth ? "auto" : "none", token: "", headers: scenario === "advanced" ? [{ id: "header-1", name: provider === "arcade" ? "Arcade-User-ID" : "X-Session-Key", value: "" }] : [], - advanced: scenario === "advanced", connectStatus: connectCases[scenario] ?? "idle", + connectStatus: connectCases[scenario] ?? "idle", connected: !access && !isConnect, identity: provider === "zapier" ? null : "reviewer@example.invalid", allAgents: scenario !== "selected_agents", agentIds: access && scenario !== "selected_agents" ? [] : ["researcher", "operator"], permissions: Object.fromEntries(tools.map((tool, index) => [tool.id, scenario !== "new_tools" || tool.id === newFixtureTool.id ? "allowed" : index === 1 ? "ask_first" : index === 2 ? "off" : "allowed"])), diff --git a/ui/storybook/prototypes/RemoteMcpConnectionReview.tsx b/ui/storybook/prototypes/RemoteMcpConnectionReview.tsx index abfc994349f..9454721bd4e 100644 --- a/ui/storybook/prototypes/RemoteMcpConnectionReview.tsx +++ b/ui/storybook/prototypes/RemoteMcpConnectionReview.tsx @@ -123,8 +123,8 @@ export function RemoteMcpConnectionReview({ provider, scenario = "journey", inli
Design review · {remoteMcpProviders[provider].name} — Example accounts and tools. No real sign-in, calls, or credential storage. Use fake values only.
{inline ? Connect {remoteMcpProviders[provider].name} - - : } + + : }