Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions apps/server/src/environment/ServerEnvironment.ts
Original file line number Diff line number Diff line change
Expand Up @@ -217,6 +217,7 @@ export const make = Effect.gen(function* () {
attachmentUploads: true,
fileAttachments: { maxUploadBytes: PROVIDER_SEND_TURN_MAX_FILE_BYTES },
pullRequests: true,
unlinkedGitHubPullRequests: true,
threadSettlement: true,
threadAutoSettlement: true,
threadRestartContinuation: true,
Expand Down
4 changes: 2 additions & 2 deletions apps/server/src/orchestration/ThreadSettlementReactor.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -110,7 +110,7 @@ function makeSnapshot(
}

function makePullRequestSummary(input: {
readonly projectId: ProjectId;
readonly projectId: ProjectId | null;
readonly repository: string;
readonly number: number;
readonly state: "open" | "closed" | "merged";
Expand Down Expand Up @@ -157,7 +157,7 @@ const makeHarness = Effect.fn("makeThreadSettlementHarness")(function* (options:
>([]);
const summaryCalls = yield* Ref.make<
ReadonlyArray<{
readonly projectId: ProjectId;
readonly projectId: ProjectId | null;
readonly repository: string;
readonly number: number;
}>
Expand Down
12 changes: 12 additions & 0 deletions apps/server/src/pullRequest/GitHubPullRequestCli.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2076,6 +2076,18 @@ layer("GitHubPullRequestCli.layer", (it) => {
}),
);

it.effect("uses the explicit GitHub host when reading a viewer outside a checkout", () =>
Effect.gen(function* () {
mockedExecute.mockReturnValueOnce(Effect.succeed(output("reviewer")));
const cli = yield* GitHubPullRequestCli.GitHubPullRequestCli;
assert.strictEqual(
yield* cli.getViewerLogin({ cwd: "/home/test", host: "github.com" }),
"reviewer",
);
expect(callAt(0).args).toEqual(["api", "user", "--jq", ".login", "--hostname", "github.com"]);
}),
);

it.effect("fails when the authenticated account has no login", () =>
Effect.gen(function* () {
mockedExecute.mockReturnValueOnce(Effect.succeed(output(" ")));
Expand Down
30 changes: 22 additions & 8 deletions apps/server/src/pullRequest/GitHubPullRequestCli.ts
Original file line number Diff line number Diff line change
Expand Up @@ -408,6 +408,7 @@ export class GitHubPullRequestCli extends Context.Service<
{
readonly getViewerLogin: (input: {
readonly cwd: string;
readonly host?: string;
}) => Effect.Effect<string, GitHubPullRequestCliError>;

readonly listPullRequests: (input: {
Expand Down Expand Up @@ -1461,14 +1462,27 @@ export const make = Effect.gen(function* () {

return GitHubPullRequestCli.of({
getViewerLogin: (input) =>
github.execute({ cwd: input.cwd, args: ["api", "user", "--jq", ".login"] }).pipe(
Effect.flatMap((result) => {
const login = result.stdout.trim();
return login.length > 0
? Effect.succeed(login)
: Effect.fail(new GitHubViewerLoginUnavailableError({ command: "gh", cwd: input.cwd }));
}),
),
github
.execute({
cwd: input.cwd,
args: [
"api",
"user",
"--jq",
".login",
...(input.host === undefined ? [] : ["--hostname", input.host]),
],
})
.pipe(
Effect.flatMap((result) => {
const login = result.stdout.trim();
return login.length > 0
? Effect.succeed(login)
: Effect.fail(
new GitHubViewerLoginUnavailableError({ command: "gh", cwd: input.cwd }),
);
}),
),

listPullRequests: (input) => {
const fallbackMaxRows = Math.max(input.limit + 1, PULL_REQUEST_FALLBACK_MAX_ROWS);
Expand Down
3 changes: 1 addition & 2 deletions apps/server/src/pullRequest/GitHubPullRequestProvider.ts
Original file line number Diff line number Diff line change
Expand Up @@ -210,8 +210,7 @@ export const make = Effect.gen(function* () {
kind: "github",
capabilities: CAPABILITIES,

getViewer: (input) =>
cli.getViewerLogin({ cwd: input.cwd }).pipe(Effect.mapError(fail("getViewer"))),
getViewer: (input) => cli.getViewerLogin(input).pipe(Effect.mapError(fail("getViewer"))),

listChangeRequests: (input) =>
cli
Expand Down
1 change: 1 addition & 0 deletions apps/server/src/pullRequest/PullRequestProvider.ts
Original file line number Diff line number Diff line change
Expand Up @@ -248,6 +248,7 @@ export interface PullRequestProviderApi {
/** The signed-in account, which is what involvement filtering compares against. */
readonly getViewer: (input: {
readonly cwd: string;
readonly host?: string;
}) => Effect.Effect<string, PullRequestProviderError>;

readonly listChangeRequests: (
Expand Down
103 changes: 103 additions & 0 deletions apps/server/src/pullRequest/PullRequestService.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4137,3 +4137,106 @@ it.effect("names the signed-in account in the detail, and says nothing where the
assert.strictEqual(unnamed.viewer, undefined);
}),
);

it.effect("reads an unlinked GitHub PR and its diff without a workspace", () =>
Effect.gen(function* () {
const calls: Array<{ repository: string; host: string; number: number }> = [];
const service = yield* makeService({
projects: [],
providers: [
fakeProvider("github", {
getChangeRequest: (input) => {
calls.push(input);
return Effect.succeed(hostedChangeRequest("An external contribution"));
},
getDiff: (input) => {
calls.push(input);
return Effect.succeed({ patch: "external diff", truncated: false, nextCursor: null });
},
}),
],
});
const reference = { projectId: null, repository: "someone/other-repo", number: 1 };
const detail = yield* service.detail(reference);
assert.strictEqual(detail.projectId, null);
assert.strictEqual(detail.workspaceRoot, null);
assert.strictEqual(detail.repository, reference.repository);
assert.strictEqual(detail.body, "An external contribution");
assert.strictEqual((yield* service.diff(reference)).patch, "external diff");
assert.deepStrictEqual(
calls.map(({ repository, host, number }) => ({ repository, host, number })),
[
{ repository: reference.repository, host: "github.com", number: 1 },
{ repository: reference.repository, host: "github.com", number: 1 },
],
);
}),
);

it.effect("checks fresh GitHub permissions for unlinked PR actions and comments", () =>
Effect.gen(function* () {
let allowed = true;
const permissions: Array<{ repository: string; host: string; number: number }> = [];
const writes: string[] = [];
const service = yield* makeService({
projects: [],
providers: [
fakeProvider("github", {
getViewerPermissions: (input) => {
permissions.push(input);
return Effect.succeed({
actions: allowed ? ["close"] : [],
comment: allowed,
resolve: false,
verdicts: [],
requestReviewers: false,
});
},
runAction: (input) => {
writes.push(input.action);
return Effect.void;
},
comment: (input) => {
writes.push(input.body);
return Effect.void;
},
}),
],
});
const reference = { projectId: null, repository: "someone/other-repo", number: 1 };
yield* service.runAction({ ...reference, action: "close" });
yield* service.comment({ ...reference, body: "First comment" });

allowed = false;
const actionError = yield* Effect.flip(service.runAction({ ...reference, action: "close" }));
const commentError = yield* Effect.flip(
service.comment({ ...reference, body: "After access was withdrawn" }),
);
assert.strictEqual(actionError._tag, "PullRequestOperationError");
assert.strictEqual(commentError._tag, "PullRequestOperationError");
assert.deepStrictEqual(writes, ["close", "First comment"]);
assert.deepStrictEqual(
permissions.map(({ repository, host, number }) => ({ repository, host, number })),
Array.from({ length: 4 }, () => ({
repository: reference.repository,
host: "github.com",
number: 1,
})),
);
}),
);

it.effect("rejects malformed unlinked repositories before calling GitHub", () =>
Effect.gen(function* () {
const service = yield* makeService({ projects: [], providers: [fakeProvider("github")] });
for (const repository of [
"../repo",
"acme/..",
"https://evil.test/acme/repo",
"acme/repo/extra",
]) {
const error = yield* Effect.flip(service.detail({ projectId: null, repository, number: 1 }));
assert.strictEqual(error._tag, "PullRequestOperationError");
}
}),
);
69 changes: 61 additions & 8 deletions apps/server/src/pullRequest/PullRequestService.ts
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
import * as NodeOS from "node:os";
import * as Cache from "effect/Cache";
import * as Clock from "effect/Clock";
import * as Context from "effect/Context";
Expand Down Expand Up @@ -68,6 +69,12 @@ import {
} from "./PullRequestProvider.ts";
import { PullRequestProviderRegistry } from "./PullRequestProviderRegistry.ts";

const GitHubRepositoryName = Schema.String.check(
Schema.isPattern(/^[A-Za-z0-9][A-Za-z0-9-]*\/(?!\.{1,2}$)[A-Za-z0-9_.-]+$/u),
);

const isGitHubRepositoryName = Schema.is(GitHubRepositoryName);

export interface PullRequestMergeEvent extends PullRequestRef {
readonly mergedAt: string;
}
Expand Down Expand Up @@ -248,6 +255,15 @@ interface SupportedProject {
readonly host: string;
}

/** A PR can be read without a checkout; only workspace listings require a full project. */
type PullRequestRepository = Omit<SupportedProject, "project"> & {
readonly project: {
readonly id: PullRequestRef["projectId"];
readonly title: string;
readonly workspaceRoot: string;
};
};

/**
* What the workspace has, split by whether this build can read it. Hosts with no
* implementation are counted rather than dropped, so their projects are explained in the
Expand Down Expand Up @@ -667,8 +683,33 @@ export const make = Effect.gen(function* () {
}),
);

const requireProject = (ref: PullRequestRef): Effect.Effect<SupportedProject, PullRequestError> =>
listWorkspaceProjects({ projectId: ref.projectId }).pipe(
const requireProject = (
ref: PullRequestRef,
): Effect.Effect<PullRequestRepository, PullRequestError> => {
if (ref.projectId === null) {
Comment thread
shivamhwp marked this conversation as resolved.
// An explicit host keeps the CLI from inferring a repository from its working directory.
// GitHub permissions govern remote review actions; a linked project is only needed
// for operations that use a local checkout. Other hosts still require a linked project.
const api = registry.get("github");
if (api === null) {
return Effect.fail(new PullRequestUnavailableError({ reason: "provider-unsupported" }));
}
if (!isGitHubRepositoryName(ref.repository)) {
return Effect.fail(
new PullRequestOperationError({
operation: "resolveRepository",
detail: "The GitHub repository must be named owner/repository.",
}),
);
}
return Effect.succeed({
project: { id: null, title: ref.repository, workspaceRoot: NodeOS.homedir() },
repository: ref.repository,
host: "github.com",
api: withRateLimitBackoff(api, "github.com", rateLimits),
});
}
return listWorkspaceProjects({ projectId: ref.projectId }).pipe(
Effect.flatMap(({ supported }): Effect.Effect<SupportedProject, PullRequestError> => {
const match = supported[0];
if (!match) {
Expand All @@ -687,6 +728,7 @@ export const make = Effect.gen(function* () {
return Effect.succeed(match);
}),
);
};

/**
* What the signed-in account may do with this change request, asked of the host itself. Every
Expand All @@ -695,7 +737,11 @@ export const make = Effect.gen(function* () {
* handed to a provider on the client's word. Read freshly for that reason, rather than taken
* from whatever the detail said when the page loaded.
*/
const viewerPermissionsOf = (project: SupportedProject, ref: PullRequestRef, operation: string) =>
const viewerPermissionsOf = (
project: PullRequestRepository,
ref: PullRequestRef,
operation: string,
) =>
project.api
.getViewerPermissions({
cwd: project.project.workspaceRoot,
Expand Down Expand Up @@ -791,7 +837,7 @@ export const make = Effect.gen(function* () {
);

const resolveViewers = (
projects: ReadonlyArray<SupportedProject>,
projects: ReadonlyArray<PullRequestRepository>,
viewerRoots: WorkspaceProjects["viewerRoots"],
) =>
Effect.forEach(
Expand Down Expand Up @@ -1228,8 +1274,14 @@ export const make = Effect.gen(function* () {
* ten-minute answer per host — so a page that has already listed anything pays nothing for it,
* and a host that cannot say leaves it null rather than failing the read it decorates.
*/
const viewerOf = (project: SupportedProject): Effect.Effect<string | null> =>
resolveViewers([project], new Map()).pipe(Effect.map(([resolved]) => resolved?.viewer ?? null));
const viewerOf = (project: PullRequestRepository): Effect.Effect<string | null> =>
project.project.id === null
? project.api
.getViewer({ cwd: project.project.workspaceRoot, host: project.host })
.pipe(Effect.orElseSucceed(() => null))
: resolveViewers([project], new Map()).pipe(
Effect.map(([resolved]) => resolved?.viewer ?? null),
);

const summaryUncached: PullRequestService["Service"]["summary"] = (input) =>
requireProject(input).pipe(
Expand Down Expand Up @@ -1287,7 +1339,7 @@ export const make = Effect.gen(function* () {
capabilities: project.api.capabilities,
projectId: project.project.id,
projectTitle: project.project.title,
workspaceRoot: project.project.workspaceRoot,
workspaceRoot: project.project.id === null ? null : project.project.workspaceRoot,
repository: project.repository,
number: changeRequest.number,
title: changeRequest.title,
Expand Down Expand Up @@ -1963,7 +2015,7 @@ export const make = Effect.gen(function* () {
{ readonly project: SupportedProject; readonly number: number }
>();
for (const ref of input.refs) {
const project = byProject.get(ref.projectId);
const project = ref.projectId === null ? undefined : byProject.get(ref.projectId);
// The repository travels through the client, so it is checked against the project's own
// remote rather than being handed to a provider verbatim.
if (
Expand Down Expand Up @@ -2287,6 +2339,7 @@ export const make = Effect.gen(function* () {
return detailUncached({ projectId, repository, number } as PullRequestRef).pipe(
Effect.tap(
Effect.fn("PullRequestService.recordDetailStats")(function* (value: PullRequestDetail) {
if (value.projectId === null) return;
recordStats(
statsKey,
{
Expand Down
Loading
Loading