diff --git a/doc/plugins/PLUGIN_SPEC.md b/doc/plugins/PLUGIN_SPEC.md index 5b88b533ec1..165c2e53ce4 100644 --- a/doc/plugins/PLUGIN_SPEC.md +++ b/doc/plugins/PLUGIN_SPEC.md @@ -642,11 +642,16 @@ Serves a first-class repository provider contributed by a trusted extension package. Requires the `repository.providers.register` capability and a matching `repositoryProviders[]` manifest declaration. -Methods: `repositoryProviderBeginInstallation`, +Required methods: `repositoryProviderBeginInstallation`, `repositoryProviderCompleteInstallation`, `repositoryProviderDiscover`, -`repositoryProviderRefreshMetadata`, `repositoryProviderSync`, -`repositoryProviderDisconnect`, and -`repositoryProviderResolveCloneCredential`. +`repositoryProviderRefreshMetadata`, `repositoryProviderSync`, and +`repositoryProviderResolveCloneCredential`. Registration is refused when the +worker implements only some of them. + +Optional method: `repositoryProviderDisconnect`. Implement it to revoke an +installation on the provider side. A provider with nothing to revoke may omit +it — the host then skips the call and still tears the connection down locally, +so an operator can always disconnect. Connector identity is `providerKey` plus a normalized `host`, so the same provider key can serve github.com and an enterprise install without either diff --git a/server/src/__tests__/repository-provider-plugin-bridge.test.ts b/server/src/__tests__/repository-provider-plugin-bridge.test.ts index 107ab54ac09..daabb09470e 100644 --- a/server/src/__tests__/repository-provider-plugin-bridge.test.ts +++ b/server/src/__tests__/repository-provider-plugin-bridge.test.ts @@ -1,9 +1,13 @@ import { randomUUID } from "node:crypto"; -import { describe } from "vitest"; +import { describe, expect, it } from "vitest"; import type { RepositoryConnection } from "@paperclipai/shared"; import type { HostToWorkerMethodName } from "@paperclipai/plugin-sdk"; +import { JsonRpcCallError, PLUGIN_RPC_ERROR_CODES } from "@paperclipai/plugin-sdk"; import { FakeRepositoryProvider } from "../services/repository-providers/index.js"; -import { createPluginRepositoryProviderConnector } from "../services/repository-providers/plugin-connector.js"; +import { + createPluginRepositoryProviderConnector, + REQUIRED_REPOSITORY_PROVIDER_METHODS, +} from "../services/repository-providers/plugin-connector.js"; import { runProviderConformance, type ProviderConformanceWorld, @@ -157,3 +161,90 @@ function bridgedWorld(): ProviderConformanceWorld { describe("plugin-bridged repository provider conformance", () => { runProviderConformance(bridgedWorld); }); + +/** + * Disconnect is the one provider hook a plugin may leave out — a provider with + * nothing to revoke on its side has no work to do. The host must still tear the + * connection down locally. Dispatching the RPC anyway makes the worker answer + * `METHOD_NOT_IMPLEMENTED`, which would strand the connection in `error` and + * leave an operator unable to disconnect at all. + */ +describe("plugin-bridged optional disconnect", () => { + const connection = connectionFromSnapshot({ + id: randomUUID(), + companyId: randomUUID(), + provider: PROVIDER, + host: HOST, + installationId: "install-1", + accountId: "account-1", + accountName: "acme", + syncCursor: null, + providerMetadata: null, + }); + + function connectorWith(worker: { supportedMethods?: string[]; call: () => Promise }) { + const calls: string[] = []; + const connector = createPluginRepositoryProviderConnector({ + pluginId: randomUUID(), + pluginKey: "acme.github-enterprise", + declaration: { providerKey: PROVIDER, displayName: "GitHub Enterprise", host: HOST }, + workerManager: { + isRunning: () => true, + getWorker: ((() => + worker.supportedMethods ? { supportedMethods: worker.supportedMethods } : undefined) as never), + call: (async (_pluginId: string, method: HostToWorkerMethodName) => { + calls.push(method); + return await worker.call(); + }) as never, + }, + }); + return { connector, calls }; + } + + it("does not dispatch when the worker does not advertise the hook", async () => { + const { connector, calls } = connectorWith({ + supportedMethods: [...REQUIRED_REPOSITORY_PROVIDER_METHODS], + call: async () => { + throw new Error("must not dispatch"); + }, + }); + + await expect(connector.disconnect!({ connection })).resolves.toBeUndefined(); + expect(calls).toEqual([]); + }); + + it("tolerates METHOD_NOT_IMPLEMENTED from a worker that advertises nothing", async () => { + const { connector, calls } = connectorWith({ + call: async () => { + throw new JsonRpcCallError({ + code: PLUGIN_RPC_ERROR_CODES.METHOD_NOT_IMPLEMENTED, + message: "repositoryProviderDisconnect is not implemented", + }); + }, + }); + + await expect(connector.disconnect!({ connection })).resolves.toBeUndefined(); + expect(calls).toEqual(["repositoryProviderDisconnect"]); + }); + + it("dispatches when the worker advertises the hook", async () => { + const { connector, calls } = connectorWith({ + supportedMethods: [...REQUIRED_REPOSITORY_PROVIDER_METHODS, "repositoryProviderDisconnect"], + call: async () => undefined, + }); + + await connector.disconnect!({ connection }); + expect(calls).toEqual(["repositoryProviderDisconnect"]); + }); + + it("still surfaces a real provider failure", async () => { + const { connector } = connectorWith({ + supportedMethods: [...REQUIRED_REPOSITORY_PROVIDER_METHODS, "repositoryProviderDisconnect"], + call: async () => { + throw new JsonRpcCallError({ code: PLUGIN_RPC_ERROR_CODES.WORKER_UNAVAILABLE, message: "worker crashed" }); + }, + }); + + await expect(connector.disconnect!({ connection })).rejects.toThrow("worker crashed"); + }); +}); diff --git a/server/src/services/repository-providers/plugin-connector.ts b/server/src/services/repository-providers/plugin-connector.ts index e5ea8769b04..35c2455ed61 100644 --- a/server/src/services/repository-providers/plugin-connector.ts +++ b/server/src/services/repository-providers/plugin-connector.ts @@ -2,6 +2,7 @@ import type { PluginRepositoryConnectionSnapshot, PluginRepositoryProviderBaseParams, } from "@paperclipai/plugin-sdk"; +import { JsonRpcCallError, PLUGIN_RPC_ERROR_CODES } from "@paperclipai/plugin-sdk"; import type { RepositoryConnection, PluginRepositoryProviderDeclaration } from "@paperclipai/shared"; import { normalizeRepositoryProviderHost } from "@paperclipai/shared"; import { unprocessable } from "../../errors.js"; @@ -57,6 +58,14 @@ export const REQUIRED_REPOSITORY_PROVIDER_METHODS = [ "repositoryProviderResolveCloneCredential", ] as const; +/** + * Hooks a repository provider may leave unimplemented. Disconnect is advisory — + * it lets a provider revoke an installation on its side — so a provider that has + * nothing to revoke is allowed to omit it, and the host still tears the + * connection down locally. + */ +export const OPTIONAL_REPOSITORY_PROVIDER_METHODS = ["repositoryProviderDisconnect"] as const; + /** * Methods a running worker declared at initialization but does not implement. * Returns an empty list when the worker manager cannot report them, since an @@ -71,6 +80,29 @@ export function missingRepositoryProviderMethods( return REQUIRED_REPOSITORY_PROVIDER_METHODS.filter((method) => !supported.includes(method)); } +/** + * Whether a worker advertises an optional method. + * + * A worker that reports no method list at all predates the advertisement, so it + * is given the benefit of the doubt and the call is attempted — a worker that + * turns out not to implement it answers `METHOD_NOT_IMPLEMENTED`, which the + * caller treats as "nothing to do". + */ +function workerAdvertisesMethod( + workerManager: RepositoryProviderWorkerHost, + pluginId: string, + method: string, +): boolean { + const supported = workerManager.getWorker?.(pluginId)?.supportedMethods; + if (!supported) return true; + return supported.includes(method); +} + +/** True when an RPC failure means "the worker does not implement this method". */ +function isMethodNotImplemented(error: unknown): boolean { + return error instanceof JsonRpcCallError && error.code === PLUGIN_RPC_ERROR_CODES.METHOD_NOT_IMPLEMENTED; +} + export interface PluginRepositoryProviderConnectorOptions { pluginId: string; pluginKey: string; @@ -246,17 +278,30 @@ export function createPluginRepositoryProviderConnector( }; }, + /** + * Disconnect is optional for a provider. When the worker does not advertise + * the hook — or answers `METHOD_NOT_IMPLEMENTED` because it predates the + * advertisement — the host still tears the connection down locally instead + * of failing the disconnect and stranding the connection in `error`. + */ async disconnect(input) { assertOwnedConnection(input.connection); - await options.workerManager.call( - options.pluginId, - "repositoryProviderDisconnect", - { - ...baseParams(input.connection.companyId), - connection: toSnapshot(input.connection), - }, - timeoutMs, - ); + if (!workerAdvertisesMethod(options.workerManager, options.pluginId, "repositoryProviderDisconnect")) { + return; + } + try { + await options.workerManager.call( + options.pluginId, + "repositoryProviderDisconnect", + { + ...baseParams(input.connection.companyId), + connection: toSnapshot(input.connection), + }, + timeoutMs, + ); + } catch (error) { + if (!isMethodNotImplemented(error)) throw error; + } }, async resolveCloneCredential(input: ResolveCloneCredentialInput) { diff --git a/ui/src/api/repositories.ts b/ui/src/api/repositories.ts index ca56585bcde..e5092843616 100644 --- a/ui/src/api/repositories.ts +++ b/ui/src/api/repositories.ts @@ -28,6 +28,23 @@ export interface RepositoryDiscoveryQuery { pageSize?: number; } +/** + * One provider identity a company can connect right now. A provider key can be + * served by more than one host — github.com alongside an enterprise install + * contributed by an extension — so the host is part of the identity. + */ +export interface RepositoryProviderAvailability { + provider: string; + host: string; + displayName: string; + description: string | null; + source: "core" | "plugin"; + pluginKey: string | null; + supportsDiscovery: boolean; + supportsCloneCredentials: boolean; + documentationUrl: string | null; +} + function discoveryQueryString(opts: RepositoryDiscoveryQuery = {}): string { const params = new URLSearchParams(); if (opts.query) params.set("query", opts.query); @@ -37,6 +54,15 @@ function discoveryQueryString(opts: RepositoryDiscoveryQuery = {}): string { return qs ? `?${qs}` : ""; } +/** + * Disambiguates a provider key served by more than one host. Omitting the host + * only resolves server-side when a single host serves the key, so the caller + * sends the host it picked from the available-provider list. + */ +function providerHostQuery(host?: string | null): string { + return host ? `?host=${encodeURIComponent(host)}` : ""; +} + export const repositoriesApi = { list: (companyId: string, opts: { includeArchived?: boolean } = {}) => api.get( @@ -70,18 +96,30 @@ export const repositoriesApi = { ), // ---- Guided provider connection flow (GitHub.com + other providers) ---- - beginConnection: (companyId: string, provider: string, input: { redirectPath?: string | null } = {}) => + listProviders: (companyId: string) => + api + .get<{ providers: RepositoryProviderAvailability[] }>( + `/companies/${encodeURIComponent(companyId)}/repository-providers`, + ) + .then((result) => result.providers), + beginConnection: ( + companyId: string, + provider: string, + input: { redirectPath?: string | null } = {}, + host?: string | null, + ) => api.post( - `/companies/${encodeURIComponent(companyId)}/repository-connections/${encodeURIComponent(provider)}/install`, + `/companies/${encodeURIComponent(companyId)}/repository-connections/${encodeURIComponent(provider)}/install${providerHostQuery(host)}`, input, ), completeConnection: ( companyId: string, provider: string, input: { state: string; installationId: string }, + host?: string | null, ) => api.post( - `/companies/${encodeURIComponent(companyId)}/repository-connections/${encodeURIComponent(provider)}/callback`, + `/companies/${encodeURIComponent(companyId)}/repository-connections/${encodeURIComponent(provider)}/callback${providerHostQuery(host)}`, input, ), discoverConnectionRepositories: (connectionId: string, opts: RepositoryDiscoveryQuery = {}) => diff --git a/ui/src/components/repositories/GitHubConnectionDialog.test.tsx b/ui/src/components/repositories/GitHubConnectionDialog.test.tsx index a086a7c76d9..76569646d60 100644 --- a/ui/src/components/repositories/GitHubConnectionDialog.test.tsx +++ b/ui/src/components/repositories/GitHubConnectionDialog.test.tsx @@ -66,7 +66,7 @@ function discoveryPage(overrides: Partial<{ items: unknown[]; nextCursor: string let container: HTMLDivElement; let root: Root; -function render() { +function render(host: string | null = null) { container = document.createElement("div"); document.body.appendChild(container); root = createRoot(container); @@ -74,7 +74,7 @@ function render() { flushSync(() => { root.render( - {}} /> + {}} /> , ); }); @@ -91,7 +91,7 @@ describe("GitHubConnectionDialog", () => { container.remove(); }); - async function reachSelectStep() { + async function reachSelectStep(host: string | null = null) { repositoriesApiMock.beginConnection.mockResolvedValue({ provider: "github", installUrl: "https://github.com/apps/paperclip/installations/new?state=abc", @@ -104,17 +104,22 @@ describe("GitHubConnectionDialog", () => { repositories: [], }); repositoriesApiMock.discoverConnectionRepositories.mockResolvedValue(discoveryPage()); - render(); + render(host); clickByText("Authorize on GitHub"); await waitFor(() => expect(windowOpen).toHaveBeenCalled()); setInputValue(document.querySelector("#github-installation-id")!, "42"); clickByText("Continue"); await waitFor(() => { - expect(repositoriesApiMock.completeConnection).toHaveBeenCalledWith("company-1", "github", { - state: "signed-state", - installationId: "42", - }); + expect(repositoriesApiMock.completeConnection).toHaveBeenCalledWith( + "company-1", + "github", + { + state: "signed-state", + installationId: "42", + }, + host, + ); expect(document.body.textContent).toContain("acme/alpha"); }); } @@ -177,4 +182,23 @@ describe("GitHubConnectionDialog", () => { clickByText("Authorize on GitHub"); await waitFor(() => expect(windowOpen).toHaveBeenCalled()); }); + + // Without the host, the server cannot tell github.com from an enterprise + // install contributed by an extension, and refuses the ambiguous lookup. + it("sends the provider host on both install calls when one is selected", async () => { + await reachSelectStep("github.acme-corp.example"); + + expect(repositoriesApiMock.beginConnection).toHaveBeenCalledWith( + "company-1", + "github", + {}, + "github.acme-corp.example", + ); + expect(repositoriesApiMock.completeConnection).toHaveBeenCalledWith( + "company-1", + "github", + { state: "signed-state", installationId: "42" }, + "github.acme-corp.example", + ); + }); }); diff --git a/ui/src/components/repositories/GitHubConnectionDialog.tsx b/ui/src/components/repositories/GitHubConnectionDialog.tsx index c3e4c562f73..d5254c216eb 100644 --- a/ui/src/components/repositories/GitHubConnectionDialog.tsx +++ b/ui/src/components/repositories/GitHubConnectionDialog.tsx @@ -29,6 +29,13 @@ interface GitHubConnectionDialogProps { companyId: string; open: boolean; onOpenChange: (open: boolean) => void; + /** + * Host of the GitHub provider identity to connect. The `github` provider key + * can be served by more than one host once an extension registers an + * enterprise install, and the server refuses an ambiguous lookup, so the + * caller passes the host it offered. Omitted when only one host serves the key. + */ + host?: string | null; /** Called after repositories are imported so the catalog can refresh. */ onImported?: (importedCount: number) => void; } @@ -45,6 +52,7 @@ export function GitHubConnectionDialog({ companyId, open, onOpenChange, + host = null, onImported, }: GitHubConnectionDialogProps) { const queryClient = useQueryClient(); @@ -78,7 +86,7 @@ export function GitHubConnectionDialog({ } const beginMutation = useMutation({ - mutationFn: () => repositoriesApi.beginConnection(companyId, "github", {}), + mutationFn: () => repositoriesApi.beginConnection(companyId, "github", {}, host), onSuccess: (result) => { setState(result.state); window.open(result.installUrl, "_blank", "noopener,noreferrer"); @@ -103,10 +111,15 @@ export function GitHubConnectionDialog({ const completeMutation = useMutation({ mutationFn: () => { if (!state) throw new Error("Start the connection before confirming installation"); - return repositoriesApi.completeConnection(companyId, "github", { - state, - installationId: installationId.trim(), - }); + return repositoriesApi.completeConnection( + companyId, + "github", + { + state, + installationId: installationId.trim(), + }, + host, + ); }, onSuccess: async (result) => { setConnection(result.connection); diff --git a/ui/src/lib/queryKeys.ts b/ui/src/lib/queryKeys.ts index 2559df79dd7..fa6dcf277b1 100644 --- a/ui/src/lib/queryKeys.ts +++ b/ui/src/lib/queryKeys.ts @@ -298,6 +298,7 @@ export const queryKeys = { list: (companyId: string, opts: { includeArchived?: boolean } = {}) => ["repositories", companyId, { includeArchived: opts.includeArchived === true }] as const, connections: (companyId: string) => ["repositories", companyId, "connections"] as const, + providers: (companyId: string) => ["repositories", companyId, "providers"] as const, detail: (id: string) => ["repositories", "detail", id] as const, }, cases: { diff --git a/ui/src/pages/Repositories.test.tsx b/ui/src/pages/Repositories.test.tsx index 346b2bd2c3a..40d5ec93037 100644 --- a/ui/src/pages/Repositories.test.tsx +++ b/ui/src/pages/Repositories.test.tsx @@ -12,6 +12,7 @@ const breadcrumbState = vi.hoisted(() => ({ setBreadcrumbs: vi.fn() })); const repositoriesApiMock = vi.hoisted(() => ({ list: vi.fn(), listConnections: vi.fn(), + listProviders: vi.fn(), createManual: vi.fn(), syncConnection: vi.fn(), })); @@ -44,6 +45,21 @@ async function waitFor(assertion: () => void, attempts = 50) { throw lastError; } +function provider(overrides: Partial> = {}) { + return { + provider: "github", + host: "github.com", + displayName: "GitHub", + description: null, + source: "core", + pluginKey: null, + supportsDiscovery: true, + supportsCloneCredentials: true, + documentationUrl: null, + ...overrides, + }; +} + function makeRepo(overrides: Partial> = {}) { return { id: "repo-1", @@ -93,9 +109,11 @@ describe("Repositories catalog", () => { beforeEach(() => { repositoriesApiMock.list.mockReset(); repositoriesApiMock.listConnections.mockReset(); + repositoriesApiMock.listProviders.mockReset(); repositoriesApiMock.createManual.mockReset(); repositoriesApiMock.syncConnection.mockReset(); repositoriesApiMock.listConnections.mockResolvedValue([]); + repositoriesApiMock.listProviders.mockResolvedValue([provider()]); }); afterEach(() => { @@ -173,4 +191,22 @@ describe("Repositories catalog", () => { }); }); }); + + // One `github` key can be served by github.com and by an enterprise install + // an extension registered. The server refuses an ambiguous lookup, so the page + // has to offer each host as its own action. + it("offers one connect action per host when several serve the github provider", async () => { + repositoriesApiMock.list.mockResolvedValue([makeRepo()]); + repositoriesApiMock.listProviders.mockResolvedValue([ + provider(), + provider({ host: "github.acme-corp.example", source: "plugin", pluginKey: "acme.github-enterprise" }), + ]); + render(); + + await waitFor(() => { + const labels = [...document.querySelectorAll("button")].map((button) => button.textContent ?? ""); + expect(labels.some((label) => label.includes("Connect github.com"))).toBe(true); + expect(labels.some((label) => label.includes("Connect github.acme-corp.example"))).toBe(true); + }); + }); }); diff --git a/ui/src/pages/Repositories.tsx b/ui/src/pages/Repositories.tsx index 25c74d4755f..03d6ffbdcf5 100644 --- a/ui/src/pages/Repositories.tsx +++ b/ui/src/pages/Repositories.tsx @@ -58,6 +58,7 @@ export function Repositories() { const queryClient = useQueryClient(); const [search, setSearch] = useState(""); const [githubOpen, setGithubOpen] = useState(false); + const [githubHost, setGithubHost] = useState(null); const [manualOpen, setManualOpen] = useState(false); const [manualUrl, setManualUrl] = useState(""); @@ -75,6 +76,22 @@ export function Repositories() { queryFn: () => repositoriesApi.listConnections(selectedCompanyId!), enabled: !!selectedCompanyId, }); + const providersQuery = useQuery({ + queryKey: queryKeys.repositories.providers(selectedCompanyId!), + queryFn: () => repositoriesApi.listProviders(selectedCompanyId!), + enabled: !!selectedCompanyId, + }); + + // The `github` key can be served by several hosts once an extension registers + // an enterprise install. The server refuses an ambiguous lookup, so the page + // offers each identity separately and sends the host the operator picked. + const githubProviders = (providersQuery.data ?? []).filter((entry) => entry.provider === "github"); + const defaultGithubHost = githubProviders.length === 1 ? githubProviders[0]!.host : null; + + function openGithubDialog(host: string | null) { + setGithubHost(host); + setGithubOpen(true); + } const manualMutation = useMutation({ mutationFn: (cloneUrl: string) => @@ -132,9 +149,17 @@ export function Repositories() { - + {githubProviders.length > 1 ? ( + githubProviders.map((entry) => ( + + )) + ) : ( + + )} ); @@ -197,7 +222,7 @@ export function Repositories() { message="Connect GitHub to import repositories, or add one manually by its clone URL." action="Connect GitHub" hideActionIcon - onAction={() => setGithubOpen(true)} + onAction={() => openGithubDialog(defaultGithubHost)} /> ) : (
@@ -252,6 +277,7 @@ export function Repositories() { refresh()} />