Skip to content

Commit fea7cdc

Browse files
fix(server): GitHub provider names a missing repository, and keeps git output out of details
From PR review: - A 404 on a repository read said "Pull request not found", which sent a user to the wrong input when cloning or reading a default branch. Repository reads now say the repository was not found, and creating a repository in an organization says which organization. - A failed checkout or an unreadable PR body copied the git or filesystem error into the detail a client sees, which can carry paths and stderr. Each stage now reports a fixed message and keeps the raw error as the cause. Co-Authored-By: Claude Opus 5.5 (1M context) <[email protected]>
1 parent b459cd8 commit fea7cdc

2 files changed

Lines changed: 85 additions & 28 deletions

File tree

‎apps/server/src/sourceControl/GitHubSourceControlProvider.test.ts‎

Lines changed: 44 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -74,6 +74,8 @@ function harness(input: {
7474
readonly remotes: string;
7575
readonly api: Partial<GitHubApi.GitHubApi["Service"]>;
7676
readonly localBranches?: ReadonlyArray<string>;
77+
/** Fails the local branch listing, the first git read a checkout makes. */
78+
readonly gitFailure?: unknown;
7779
}) {
7880
const git: Array<readonly [string, unknown]> = [];
7981
const record =
@@ -99,7 +101,10 @@ function harness(input: {
99101
fetchRemoteTrackingBranch: (args) => record("fetchRemoteTrackingBranch", undefined)(args),
100102
setBranchUpstream: (args) => record("setBranchUpstream", undefined)(args),
101103
switchRef: (args) => record("switchRef", { refName: args.refName })(args) as never,
102-
listLocalBranchNames: () => Effect.succeed([...(input.localBranches ?? [])]),
104+
listLocalBranchNames: () =>
105+
input.gitFailure === undefined
106+
? Effect.succeed([...(input.localBranches ?? [])])
107+
: Effect.fail(input.gitFailure as never),
103108
resolveCommit: () => Effect.succeed({ commitSha: "abc123" }),
104109
});
105110
const process = Layer.mock(VcsProcess.VcsProcess)({
@@ -182,6 +187,23 @@ describe("GitHubSourceControlProvider repository resolution", () => {
182187
}).pipe(Effect.provide(layer));
183188
});
184189

190+
it.effect("names a missing repository rather than a pull request", () => {
191+
const { layer } = harness({
192+
remotes: remotesOutput(["origin", "[email protected]:acme/gone.git"]),
193+
api: {
194+
rest: (input) =>
195+
Effect.fail(
196+
new GitHubApi.GitHubApiNotFoundError({ host: input.host, operation: input.operation }),
197+
),
198+
},
199+
});
200+
return Effect.gen(function* () {
201+
const gh = yield* GitHubSourceControlProvider.make;
202+
const error = yield* gh.getDefaultBranch({ cwd: "/repo" }).pipe(Effect.flip);
203+
assert.include(error.detail, "Repository not found");
204+
}).pipe(Effect.provide(layer));
205+
});
206+
185207
it.effect("fails clearly when no remote is on GitHub", () => {
186208
const { layer } = harness({
187209
remotes: remotesOutput(["origin", "[email protected]:a/b.git"]),
@@ -581,6 +603,27 @@ describe("GitHubSourceControlProvider.checkoutChangeRequest", () => {
581603
}).pipe(Effect.provide(layer));
582604
});
583605

606+
it.effect("keeps git's own output out of what a failed checkout reports", () => {
607+
const { layer } = harness({
608+
remotes: remotesOutput(["origin", "[email protected]:acme/web.git"]),
609+
gitFailure: new Error("fatal: /home/me/secret-path: permission denied"),
610+
api: {
611+
graphql: () =>
612+
Effect.succeed(
613+
encodeJson({ data: { repository: { pullRequest: node(5, "feature/x") } } }),
614+
),
615+
},
616+
});
617+
return Effect.gen(function* () {
618+
const gh = yield* GitHubSourceControlProvider.make;
619+
const error = yield* gh
620+
.checkoutChangeRequest({ cwd: "/repo", reference: "5" })
621+
.pipe(Effect.flip);
622+
assert.strictEqual(error.detail, "The pull request could not be checked out with git.");
623+
assert.notInclude(error.message, "secret-path");
624+
}).pipe(Effect.provide(layer));
625+
});
626+
584627
it("names the local branch the way gh pr checkout does", () => {
585628
const name = (headRefName: string, isCrossRepository: boolean) =>
586629
GitHubSourceControlProvider.pullRequestCheckoutBranchName({

‎apps/server/src/sourceControl/GitHubSourceControlProvider.ts‎

Lines changed: 41 additions & 27 deletions
Original file line numberDiff line numberDiff line change
@@ -270,7 +270,14 @@ class GitHubFailure extends Data.TaggedError("GitHubFailure")<{
270270

271271
const failure = (detail: string, cause?: unknown) => new GitHubFailure({ detail, cause });
272272

273-
function fromGitHubApiError(error: GitHubApi.GitHubApiError): GitHubFailure {
273+
const PULL_REQUEST_NOT_FOUND = "Pull request not found. Check the PR number or URL and try again.";
274+
const REPOSITORY_NOT_FOUND = "Repository not found. Check the owner and name and try again.";
275+
276+
/** `notFound` names what was missing, which only the operation knows. */
277+
function fromGitHubApiError(
278+
error: GitHubApi.GitHubApiError,
279+
notFound = PULL_REQUEST_NOT_FOUND,
280+
): GitHubFailure {
274281
switch (error._tag) {
275282
case "GitHubCliMissingError":
276283
return failure(
@@ -293,7 +300,7 @@ function fromGitHubApiError(error: GitHubApi.GitHubApiError): GitHubFailure {
293300
error,
294301
);
295302
case "GitHubApiNotFoundError":
296-
return failure("Pull request not found. Check the PR number or URL and try again.", error);
303+
return failure(notFound, error);
297304
case "GitHubCliFailedError":
298305
case "GitHubApiResponseError":
299306
case "GitHubApiRequestError":
@@ -302,8 +309,7 @@ function fromGitHubApiError(error: GitHubApi.GitHubApiError): GitHubFailure {
302309
}
303310
}
304311

305-
const notFound = (cause: string) =>
306-
failure("Pull request not found. Check the PR number or URL and try again.", new Error(cause));
312+
const notFound = (cause: string) => failure(PULL_REQUEST_NOT_FOUND, new Error(cause));
307313

308314
const RawRepositorySchema = Schema.Struct({
309315
full_name: TrimmedNonEmptyString,
@@ -506,8 +512,8 @@ export const make = Effect.gen(function* () {
506512
}),
507513
);
508514

509-
const rest = (input: GitHubApi.GitHubRestInput) =>
510-
api.rest(input).pipe(Effect.mapError(fromGitHubApiError));
515+
const rest = (input: GitHubApi.GitHubRestInput, notFoundDetail?: string) =>
516+
api.rest(input).pipe(Effect.mapError((error) => fromGitHubApiError(error, notFoundDetail)));
511517

512518
const headResolver = RequestResolver.makeGrouped<PullRequestsByHeadRead, string>({
513519
key: ({ request, context }) =>
@@ -654,12 +660,15 @@ export const make = Effect.gen(function* () {
654660
const readRepository = Effect.fn("GitHubSourceControlProvider.readRepository")(function* (
655661
locator: GitHubRepositoryLocator,
656662
) {
657-
const response = yield* rest({
658-
host: locator.host,
659-
operation: "getRepository",
660-
path: `repos/${encodeURIComponent(locator.owner)}/${encodeURIComponent(locator.name)}`,
661-
allowReserve: true,
662-
});
663+
const response = yield* rest(
664+
{
665+
host: locator.host,
666+
operation: "getRepository",
667+
path: `repos/${encodeURIComponent(locator.owner)}/${encodeURIComponent(locator.name)}`,
668+
allowReserve: true,
669+
},
670+
REPOSITORY_NOT_FOUND,
671+
);
663672
const decoded = decodeRawRepository(response.body);
664673
if (Result.isFailure(decoded)) {
665674
return yield* failure("GitHub returned an invalid repository.", decoded.failure);
@@ -678,13 +687,10 @@ export const make = Effect.gen(function* () {
678687
return decoded.success.login;
679688
});
680689

690+
// Git and filesystem errors can carry arguments, paths and stderr, so the detail a client
691+
// sees is fixed and the raw error stays in `cause`.
681692
const gitFailure = (cause: unknown) =>
682-
failure(
683-
cause instanceof Error && cause.message.trim() !== ""
684-
? cause.message.trim()
685-
: "GitHub request failed.",
686-
cause,
687-
);
693+
failure("The pull request could not be checked out with git.", cause);
688694

689695
const runGit = (cwd: string, operation: string, args: ReadonlyArray<string>) =>
690696
git.execute({
@@ -929,7 +935,11 @@ export const make = Effect.gen(function* () {
929935
const locator = yield* resolveRepository({ cwd: input.cwd });
930936
const body = yield* fileSystem
931937
.readFileString(input.bodyFile)
932-
.pipe(Effect.mapError(gitFailure));
938+
.pipe(
939+
Effect.mapError((cause) =>
940+
failure("The pull request description could not be read.", cause),
941+
),
942+
);
933943
yield* rest({
934944
host: locator.host,
935945
operation: "createPullRequest",
@@ -975,14 +985,18 @@ export const make = Effect.gen(function* () {
975985
const name = locator?.name ?? input.repository.trim();
976986
const host = locator?.host ?? (environment.GH_HOST?.trim().toLowerCase() || "github.com");
977987
const isViewer = viewer !== null && owner?.toLowerCase() === viewer.toLowerCase();
978-
const response = yield* rest({
979-
host,
980-
operation: "createRepository",
981-
method: "POST",
982-
path:
983-
isViewer || owner === null ? "user/repos" : `orgs/${encodeURIComponent(owner)}/repos`,
984-
body: { name, private: input.visibility === "private" },
985-
});
988+
const response = yield* rest(
989+
{
990+
host,
991+
operation: "createRepository",
992+
method: "POST",
993+
path:
994+
isViewer || owner === null ? "user/repos" : `orgs/${encodeURIComponent(owner)}/repos`,
995+
body: { name, private: input.visibility === "private" },
996+
},
997+
// GitHub answers 404 for an organization the account cannot create repositories in.
998+
`No organization named ${owner ?? "that"} that this account can create repositories in.`,
999+
);
9861000
const decoded = decodeRawRepository(response.body);
9871001
if (Result.isFailure(decoded)) {
9881002
return yield* failure("GitHub returned an invalid repository.", decoded.failure);

0 commit comments

Comments
 (0)