mirror of
https://github.com/paperclipai/paperclip.git
synced 2026-10-27 19:35:13 +01:00
fix(repositories): preserve explicit provider imports
Search complete GitHub installations before paginating results, refresh only explicitly imported repositories during sync, and avoid leaking upstream request paths in provider errors. Co-Authored-By: Paperclip <noreply@paperclip.ing>
This commit is contained in:
1 parent
5c648695f0
commit
e22e9aab41
6 files changed
+189
-20
No files matched your search
@@ -0,0 +1,80 @@
|
||||
import { generateKeyPairSync } from "node:crypto";
|
||||
import { describe, expect, it, vi } from "vitest";
|
||||
import { createGitHubAppTransport } from "../services/repository-providers/github-transport.js";
|
||||
import { makeGitHubRepository } from "./helpers/fake-github-transport.js";
|
||||
|
||||
const privateKey = generateKeyPairSync("rsa", { modulusLength: 2048 })
|
||||
.privateKey.export({ type: "pkcs8", format: "pem" })
|
||||
.toString();
|
||||
|
||||
function jsonResponse(body: unknown, status = 200) {
|
||||
return new Response(JSON.stringify(body), {
|
||||
status,
|
||||
headers: { "Content-Type": "application/json" },
|
||||
});
|
||||
}
|
||||
|
||||
describe("GitHub App transport", () => {
|
||||
it("searches the complete installation catalog before paginating matches", async () => {
|
||||
const repositories = Array.from({ length: 205 }, (_, index) => makeGitHubRepository({
|
||||
id: index + 1,
|
||||
owner: index === 100 || index === 101 || index === 200 ? "matching-owner" : "other-owner",
|
||||
name: `repo-${index + 1}`,
|
||||
}));
|
||||
const fetchImpl = vi.fn(async (input: string | URL | Request) => {
|
||||
const url = new URL(typeof input === "string" ? input : input instanceof URL ? input : input.url);
|
||||
if (url.pathname.endsWith("/access_tokens")) {
|
||||
return jsonResponse({ token: "ghs_test", expires_at: "2099-01-01T00:00:00.000Z" });
|
||||
}
|
||||
const page = Number(url.searchParams.get("page"));
|
||||
const perPage = Number(url.searchParams.get("per_page"));
|
||||
const start = (page - 1) * perPage;
|
||||
return jsonResponse({ total_count: repositories.length, repositories: repositories.slice(start, start + perPage) });
|
||||
});
|
||||
const transport = createGitHubAppTransport({ appId: "1", privateKey, fetchImpl });
|
||||
|
||||
await expect(transport.listRepositories({
|
||||
installationId: "installation-1",
|
||||
page: 1,
|
||||
perPage: 2,
|
||||
query: "matching-owner",
|
||||
})).resolves.toMatchObject({
|
||||
totalCount: 3,
|
||||
items: [{ id: 101 }, { id: 102 }],
|
||||
});
|
||||
await expect(transport.listRepositories({
|
||||
installationId: "installation-1",
|
||||
page: 2,
|
||||
perPage: 2,
|
||||
query: "matching-owner",
|
||||
})).resolves.toMatchObject({
|
||||
totalCount: 3,
|
||||
items: [{ id: 201 }],
|
||||
});
|
||||
expect(fetchImpl.mock.calls.some(([input]) => String(input).includes("per_page=100&page=3"))).toBe(true);
|
||||
});
|
||||
|
||||
it("does not expose provider request paths in operator-facing failures", async () => {
|
||||
const fetchImpl = vi.fn(async (input: string | URL | Request) => {
|
||||
const url = new URL(typeof input === "string" ? input : input instanceof URL ? input : input.url);
|
||||
if (url.pathname.endsWith("/access_tokens")) {
|
||||
return jsonResponse({ token: "ghs_test", expires_at: "2099-01-01T00:00:00.000Z" });
|
||||
}
|
||||
return jsonResponse({ message: "unavailable" }, 503);
|
||||
});
|
||||
const transport = createGitHubAppTransport({ appId: "1", privateKey, fetchImpl });
|
||||
|
||||
await expect(transport.listRepositories({
|
||||
installationId: "sensitive-installation",
|
||||
page: 1,
|
||||
perPage: 20,
|
||||
query: null,
|
||||
})).rejects.toThrow("GitHub repository provider request failed with status 503");
|
||||
await expect(transport.listRepositories({
|
||||
installationId: "sensitive-installation",
|
||||
page: 1,
|
||||
perPage: 20,
|
||||
query: null,
|
||||
})).rejects.not.toThrow("sensitive-installation");
|
||||
});
|
||||
});
|
||||
@@ -533,11 +533,18 @@ describeEmbeddedPostgres("repository services and routes", () => {
|
||||
provider: "test_provider",
|
||||
sync: async () => ({
|
||||
cursor: "cursor-1",
|
||||
repositories: [{
|
||||
providerRepositoryId: "provider-repo-1",
|
||||
cloneUrl: "https://git.example.com/acme/provider-repo.git",
|
||||
visibility: "private",
|
||||
}],
|
||||
repositories: [
|
||||
{
|
||||
providerRepositoryId: "provider-repo-1",
|
||||
cloneUrl: "https://git.example.com/acme/provider-repo-renamed.git",
|
||||
visibility: "private",
|
||||
},
|
||||
{
|
||||
providerRepositoryId: "provider-repo-not-imported",
|
||||
cloneUrl: "https://git.example.com/acme/not-imported.git",
|
||||
visibility: "private",
|
||||
},
|
||||
],
|
||||
}),
|
||||
disconnect: async () => {},
|
||||
};
|
||||
@@ -560,9 +567,24 @@ describeEmbeddedPostgres("repository services and routes", () => {
|
||||
expect(created.created).toBe(true);
|
||||
expect(duplicate.created).toBe(false);
|
||||
|
||||
const providerRepository = await repositoryService(db).upsertProviderRepository(
|
||||
seeded.companyA.id,
|
||||
created.connection.id,
|
||||
provider.provider,
|
||||
{
|
||||
providerRepositoryId: "provider-repo-1",
|
||||
cloneUrl: "https://git.example.com/acme/provider-repo.git",
|
||||
visibility: "private",
|
||||
},
|
||||
);
|
||||
const synced = await connections.sync(seeded.companyA.id, created.connection.id);
|
||||
expect(synced?.repositories).toHaveLength(1);
|
||||
const providerRepository = synced!.repositories[0]!;
|
||||
expect(synced?.repositories[0]).toMatchObject({
|
||||
id: providerRepository.id,
|
||||
name: "provider-repo-renamed",
|
||||
});
|
||||
expect((await repositoryService(db).list(seeded.companyA.id)).map((repository) => repository.name))
|
||||
.not.toContain("not-imported");
|
||||
await repositoryAccessService(db).attachProjectRepository({
|
||||
companyId: seeded.companyA.id,
|
||||
projectId: seeded.projectOne.id,
|
||||
|
||||
@@ -378,5 +378,25 @@ export function repositoryService(db: Db) {
|
||||
.returning();
|
||||
return rows.map(toRepository);
|
||||
},
|
||||
|
||||
markProviderRepositoriesUnavailable: async (
|
||||
companyId: string,
|
||||
connectionId: string,
|
||||
providerRepositoryIds: string[],
|
||||
reason: string,
|
||||
) => {
|
||||
if (providerRepositoryIds.length === 0) return [];
|
||||
const rows = await db
|
||||
.update(repositories)
|
||||
.set({ state: "unavailable", unavailableReason: reason, updatedAt: new Date() })
|
||||
.where(and(
|
||||
eq(repositories.companyId, companyId),
|
||||
eq(repositories.connectionId, connectionId),
|
||||
inArray(repositories.providerRepositoryId, providerRepositoryIds),
|
||||
ne(repositories.state, "archived"),
|
||||
))
|
||||
.returning();
|
||||
return rows.map(toRepository);
|
||||
},
|
||||
};
|
||||
}
|
||||
@@ -1,6 +1,6 @@
|
||||
import { and, asc, eq } from "drizzle-orm";
|
||||
import type { Db } from "@paperclipai/db";
|
||||
import { repositoryConnections } from "@paperclipai/db";
|
||||
import { repositoryConnections, repositories } from "@paperclipai/db";
|
||||
import type { CreateRepositoryConnection, RepositoryConnection } from "@paperclipai/shared";
|
||||
import {
|
||||
REPOSITORY_PROVIDER_ANY_HOST,
|
||||
@@ -337,12 +337,36 @@ export function repositoryConnectionService(
|
||||
});
|
||||
return await db.transaction(async (tx) => {
|
||||
const transactionalRepositories = repositoryService(tx as unknown as Db);
|
||||
const importedRows = await tx
|
||||
.select({ providerRepositoryId: repositories.providerRepositoryId })
|
||||
.from(repositories)
|
||||
.where(and(
|
||||
eq(repositories.companyId, companyId),
|
||||
eq(repositories.connectionId, id),
|
||||
));
|
||||
const importedIds = new Set(
|
||||
importedRows
|
||||
.map((repository) => repository.providerRepositoryId)
|
||||
.filter((providerRepositoryId): providerRepositoryId is string => Boolean(providerRepositoryId)),
|
||||
);
|
||||
const snapshotsById = new Map(
|
||||
result.repositories.map((snapshot) => [snapshot.providerRepositoryId, snapshot]),
|
||||
);
|
||||
const syncedRepositories = [];
|
||||
for (const snapshot of result.repositories) {
|
||||
for (const providerRepositoryId of importedIds) {
|
||||
const snapshot = snapshotsById.get(providerRepositoryId);
|
||||
if (!snapshot) continue;
|
||||
syncedRepositories.push(
|
||||
await transactionalRepositories.upsertProviderRepository(companyId, id, row.provider, snapshot),
|
||||
);
|
||||
}
|
||||
const unavailableIds = [...importedIds].filter((providerRepositoryId) => !snapshotsById.has(providerRepositoryId));
|
||||
syncedRepositories.push(...await transactionalRepositories.markProviderRepositoriesUnavailable(
|
||||
companyId,
|
||||
id,
|
||||
unavailableIds,
|
||||
"Repository is no longer available through its provider connection",
|
||||
));
|
||||
const completed = await tx
|
||||
.update(repositoryConnections)
|
||||
.set({
|
||||
|
||||
@@ -228,7 +228,10 @@ export function createGitHubProvider(deps: GitHubProviderDeps): RepositoryProvid
|
||||
query: input.query?.trim() || null,
|
||||
});
|
||||
const items = result.items.map(toDiscovered);
|
||||
const nextCursor = items.length === perPage ? String(page + 1) : null;
|
||||
const hasNextPage = result.totalCount !== null
|
||||
? page * perPage < result.totalCount
|
||||
: items.length === perPage;
|
||||
const nextCursor = hasNextPage ? String(page + 1) : null;
|
||||
return { items, nextCursor, total: result.totalCount };
|
||||
},
|
||||
|
||||
|
||||
@@ -68,7 +68,7 @@ export function createGitHubAppTransport(config: GitHubAppTransportConfig): GitH
|
||||
},
|
||||
});
|
||||
if (!response.ok) {
|
||||
throw new Error(`GitHub app request ${path} failed with ${response.status}`);
|
||||
throw new Error(`GitHub App request failed with status ${response.status}`);
|
||||
}
|
||||
return (await response.json()) as T;
|
||||
}
|
||||
@@ -94,7 +94,7 @@ export function createGitHubAppTransport(config: GitHubAppTransportConfig): GitH
|
||||
},
|
||||
});
|
||||
if (!response.ok) {
|
||||
throw new Error(`GitHub installation request ${path} failed with ${response.status}`);
|
||||
throw new Error(`GitHub repository provider request failed with status ${response.status}`);
|
||||
}
|
||||
return (await response.json()) as T;
|
||||
}
|
||||
@@ -122,20 +122,40 @@ export function createGitHubAppTransport(config: GitHubAppTransportConfig): GitH
|
||||
createInstallationAccessToken,
|
||||
|
||||
async listRepositories(input) {
|
||||
const params = new URLSearchParams({ per_page: String(input.perPage), page: String(input.page) });
|
||||
const body = await installationRequest<{ total_count: number; repositories: GitHubRepository[] }>(
|
||||
input.installationId,
|
||||
`/installation/repositories?${params.toString()}`,
|
||||
);
|
||||
const query = input.query?.trim().toLowerCase();
|
||||
if (!query) {
|
||||
const params = new URLSearchParams({ per_page: String(input.perPage), page: String(input.page) });
|
||||
const body = await installationRequest<{ total_count: number; repositories: GitHubRepository[] }>(
|
||||
input.installationId,
|
||||
`/installation/repositories?${params.toString()}`,
|
||||
);
|
||||
return { totalCount: body.total_count ?? null, items: body.repositories ?? [] };
|
||||
}
|
||||
// The installation-repositories endpoint has no server-side search; filter
|
||||
// the page client-side. total_count no longer reflects the filtered set.
|
||||
|
||||
// GitHub does not search installation repositories server-side. Walk the
|
||||
// complete installation catalog before paginating so a match on a later
|
||||
// upstream page is neither omitted nor given a misleading next cursor.
|
||||
const matches: GitHubRepository[] = [];
|
||||
let upstreamPage = 1;
|
||||
const upstreamPageSize = 100;
|
||||
for (;;) {
|
||||
const params = new URLSearchParams({
|
||||
per_page: String(upstreamPageSize),
|
||||
page: String(upstreamPage),
|
||||
});
|
||||
const body = await installationRequest<{ total_count: number; repositories: GitHubRepository[] }>(
|
||||
input.installationId,
|
||||
`/installation/repositories?${params.toString()}`,
|
||||
);
|
||||
const pageItems = body.repositories ?? [];
|
||||
matches.push(...pageItems.filter((repo) => repo.full_name.toLowerCase().includes(query)));
|
||||
if (pageItems.length < upstreamPageSize || upstreamPage * upstreamPageSize >= body.total_count) break;
|
||||
upstreamPage += 1;
|
||||
}
|
||||
const start = (input.page - 1) * input.perPage;
|
||||
return {
|
||||
totalCount: null,
|
||||
items: (body.repositories ?? []).filter((repo) => repo.full_name.toLowerCase().includes(query)),
|
||||
totalCount: matches.length,
|
||||
items: matches.slice(start, start + input.perPage),
|
||||
};
|
||||
},
|
||||
|
||||
|
||||
Reference in new issue
Block a user