Skip to content
Closed
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
36 changes: 36 additions & 0 deletions apps/server/src/sourceControl/GitHubCli.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -292,6 +292,42 @@ describe("GitHubCli.layer", () => {
}).pipe(Effect.provide(layer)),
);

it.effect("maps a repository resolution failure to GitHubRepositoryNotFoundError", () =>
Effect.gen(function* () {
const exitError = VcsProcessExitError.fromProcessExit(
{
operation: "GitHubCli.execute",
command: "gh",
cwd: "/repo",
argumentCount: 5,
},
{
exitCode: 1,
stderr:
"GraphQL: Could not resolve to a Repository with the name 'octocat/nope'. (repository)",
stderrTruncated: false,
},
"repository-not-found",
);
assert.strictEqual(exitError.detail, "Repository not found.");
mockRun.mockReturnValueOnce(Effect.fail(exitError));

const gh = yield* GitHubCli.GitHubCli;
const error = yield* Effect.flip(
gh.getRepositoryCloneUrls({
cwd: "/repo",
repository: "octocat/nope",
}),
);

assert.strictEqual(error._tag, "GitHubRepositoryNotFoundError");
assert.strictEqual(
error.detail,
"Repository not found. Check the owner/repo path and try again.",
);
}).pipe(Effect.provide(layer)),
);

it.effect("creates repositories and parses clone URLs from create output", () =>
Effect.gen(function* () {
mockRun.mockReturnValueOnce(
Expand Down
17 changes: 17 additions & 0 deletions apps/server/src/sourceControl/GitHubCli.ts
Original file line number Diff line number Diff line change
Expand Up @@ -77,6 +77,19 @@ export class GitHubPullRequestNotFoundError extends Schema.TaggedErrorClass<GitH
}
}

export class GitHubRepositoryNotFoundError extends Schema.TaggedErrorClass<GitHubRepositoryNotFoundError>()(
"GitHubRepositoryNotFoundError",
gitHubCliFailureFields,
) {
get detail(): string {
return "Repository not found. Check the owner/repo path and try again.";
}

override get message(): string {
return `GitHub CLI failed in execute: ${this.detail}`;
}
}

export class GitHubCliCommandError extends Schema.TaggedErrorClass<GitHubCliCommandError>()(
"GitHubCliCommandError",
gitHubCliFailureFields,
Expand Down Expand Up @@ -153,6 +166,7 @@ export const GitHubCliError = Schema.Union([
GitHubCliAuthenticationError,
GitHubCliRateLimitError,
GitHubPullRequestNotFoundError,
GitHubRepositoryNotFoundError,
GitHubCliCommandError,
GitHubPullRequestListDecodeError,
GitHubChangeRequestListDecodeError,
Expand Down Expand Up @@ -190,6 +204,9 @@ export function fromVcsError(
if (error.failureKind === "not-found") {
return new GitHubPullRequestNotFoundError({ ...context, cause: error });
}
if (error.failureKind === "repository-not-found") {
return new GitHubRepositoryNotFoundError({ ...context, cause: error });
}
}

return new GitHubCliCommandError({ ...context, cause: error });
Expand Down
3 changes: 3 additions & 0 deletions apps/server/src/sourceControl/GitHubSourceControlProvider.ts
Original file line number Diff line number Diff line change
Expand Up @@ -253,6 +253,9 @@ export const make = Effect.gen(function* () {
input.repository,
),
detail: error.detail,
// GitHubCliError details are compile-time constants, so they are
// safe to show the user in place of the generic fallback.
userDetail: error.detail,
cause: error,
}),
),
Expand Down
1 change: 1 addition & 0 deletions apps/server/src/sourceControl/GitLabCli.ts
Original file line number Diff line number Diff line change
Expand Up @@ -142,6 +142,7 @@ export class GitLabCliCommandError extends Schema.TaggedErrorClass<GitLabCliComm
case "rate-limited":
return new GitLabCliRateLimitError({ ...context, cause });
case "not-found":
case "repository-not-found":
case "command-failed":
case undefined:
return new GitLabCliCommandError({ ...context, cause });
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -150,6 +150,41 @@ it.effect("preserves provider failures without deriving the repository message f
}).pipe(Effect.provide(makeLayer({ provider })));
});

it.effect("surfaces the provider's curated user detail when one is set", () => {
const providerCause = new SourceControlProviderError({
provider: "github",
operation: "getRepositoryCloneUrls",
cwd: "/workspace",
repository: "octocat/nope",
detail: "gh stderr that stays server-side",
userDetail: "Repository not found. Check the owner/repo path and try again.",
});
const provider = makeProvider({
getRepositoryCloneUrls: () => Effect.fail(providerCause),
});

return Effect.gen(function* () {
const service = yield* SourceControlRepositoryService.SourceControlRepositoryService;
const error = yield* Effect.flip(
service.lookupRepository({
provider: "github",
repository: "octocat/nope",
cwd: "/workspace",
}),
);

assert.strictEqual(
error.detail,
"Repository not found. Check the owner/repo path and try again.",
);
assert.strictEqual(
error.message,
"Source control repository operation lookupRepository failed for github: Repository not found. Check the owner/repo path and try again.",
);
assert.strictEqual(error.cause, providerCause);
}).pipe(Effect.provide(makeLayer({ provider })));
});

it.effect("clones a looked-up repository into the requested destination", () =>
Effect.gen(function* () {
const fs = yield* FileSystem.FileSystem;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import * as Path from "effect/Path";
import * as Schema from "effect/Schema";

import {
SourceControlProviderError,
SourceControlRepositoryError,
type SourceControlCloneRepositoryInput,
type SourceControlCloneRepositoryResult,
Expand All @@ -23,6 +24,7 @@ import { ServerConfig } from "../config.ts";
import * as GitVcsDriver from "../vcs/GitVcsDriver.ts";
import * as SourceControlProviderRegistry from "./SourceControlProviderRegistry.ts";
const isSourceControlRepositoryError = Schema.is(SourceControlRepositoryError);
const isSourceControlProviderError = Schema.is(SourceControlProviderError);

export class SourceControlRepositoryService extends Context.Service<
SourceControlRepositoryService,
Expand All @@ -46,7 +48,12 @@ function mapRepositoryError(operation: string, provider: SourceControlProviderKi
: new SourceControlRepositoryError({
operation,
provider,
detail: "The source control operation could not be completed.",
// Provider `detail` may quote provider output, so only the curated
// `userDetail` opt-in reaches the client; everything else stays
// behind the generic sentence.
detail:
(isSourceControlProviderError(cause) ? cause.userDetail : undefined) ??
"The source control operation could not be completed.",
Comment on lines +51 to +56

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The wrapper's detail — and therefore SourceControlRepositoryError.message — is now taken from the cause's string field rather than from the wrapper's own structural attributes. Suggest classifying the provider failure here (e.g. on its tag/failureKind) into a structured reason or a distinct not-found error class, and deriving the user-facing sentence from that, so the message stays independent of provider-supplied text.

Posted via Macroscope — Effect Service Conventions

cause,
}),
);
Expand Down
23 changes: 23 additions & 0 deletions apps/server/src/vcs/VcsProcess.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -315,3 +315,26 @@ describe("VcsProcess.run", () => {
}).pipe(provideLive),
);
});

describe("classifyNonZeroExit", () => {
it("classifies a gh repository resolution failure as repository-not-found", () => {
expect(
VcsProcess.classifyNonZeroExit(
"gh",
"GraphQL: Could not resolve to a Repository with the name 'octocat/nope'. (repository)",
),
).toBe("repository-not-found");
});

it("keeps gh pull request resolution failures as not-found", () => {
expect(
VcsProcess.classifyNonZeroExit("gh", "GraphQL: Could not resolve to a PullRequest."),
).toBe("not-found");
});

it("does not classify repository resolution failures for other commands", () => {
expect(VcsProcess.classifyNonZeroExit("git", "could not resolve to a repository")).toBe(
"command-failed",
);
});
});
6 changes: 5 additions & 1 deletion apps/server/src/vcs/VcsProcess.ts
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,7 @@ const DEFAULT_TIMEOUT_MS = 30_000;
const DEFAULT_MAX_OUTPUT_BYTES = 1_000_000;
const OUTPUT_TRUNCATED_MARKER = "\n\n[truncated]";

const classifyNonZeroExit = (command: string, stderr: string): VcsProcessExitFailureKind => {
export const classifyNonZeroExit = (command: string, stderr: string): VcsProcessExitFailureKind => {
const normalized = stderr.toLowerCase();

if (
Expand All @@ -79,6 +79,10 @@ const classifyNonZeroExit = (command: string, stderr: string): VcsProcessExitFai
return "rate-limited";
}

if (command === "gh" && normalized.includes("could not resolve to a repository")) {
return "repository-not-found";
}

if (
(command === "gh" &&
(normalized.includes("could not resolve to a pullrequest") ||
Expand Down
7 changes: 7 additions & 0 deletions packages/contracts/src/sourceControl.ts
Original file line number Diff line number Diff line change
Expand Up @@ -160,6 +160,13 @@ export class SourceControlProviderError extends Schema.TaggedErrorClass<SourceCo
repository: Schema.optional(Schema.String),
reference: Schema.optional(Schema.String),
detail: Schema.String,
/**
* Set only to a compile-time constant that is safe to show the user.
* `detail` may quote provider output and stays out of client-facing
* errors; `userDetail` is the adapter's explicit opt-in to surface a
* curated explanation instead of the generic fallback.
*/
userDetail: Schema.optional(Schema.String),
Comment on lines +163 to +169

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

userDetail adds a free-form, prose string field whose only guarantee is the surrounding comment, and it duplicates detail at its single production construction site (GitHubSourceControlProvider.getRepositoryCloneUrls passes userDetail: error.detail). The distinction it encodes ("this failure is a repository lookup miss, and here is the sentence for it") already exists structurally as GitHubRepositoryNotFoundError / VcsProcessExitFailureKind.

Consider modeling it structurally instead of threading text: a dedicated not-found error class (or a reason literal on SourceControlRepositoryError) whose detail/message is derived from the tag/reason, keeping the provider error as cause. That keeps error attributes bounded and the caller-visible sentence owned by the error type rather than copied through two layers.

Posted via Macroscope — Effect Service Conventions

cause: Schema.optional(Schema.Defect()),
},
) {
Expand Down
17 changes: 10 additions & 7 deletions packages/contracts/src/vcs.ts
Original file line number Diff line number Diff line change
Expand Up @@ -76,6 +76,7 @@ export interface VcsProcessTimeoutFailure {
export const VcsProcessExitFailureKind = Schema.Literals([
"authentication",
"not-found",
"repository-not-found",
"rate-limited",
"command-failed",
]);
Expand Down Expand Up @@ -137,13 +138,15 @@ export class VcsProcessExitError extends Schema.TaggedErrorClass<VcsProcessExitE
? "Authentication failed."
: failureKind === "rate-limited"
? "API rate limit exceeded."
: failureKind === "not-found"
? context.command === "glab"
? "Merge request not found."
: context.command === "gh" || context.command === "az"
? "Pull request not found."
: "VCS resource not found."
: "Process exited with a non-zero status.";
: failureKind === "repository-not-found"
? "Repository not found."
: failureKind === "not-found"
? context.command === "glab"
? "Merge request not found."
: context.command === "gh" || context.command === "az"
? "Pull request not found."
: "VCS resource not found."
: "Process exited with a non-zero status.";

return new VcsProcessExitError({
...context,
Expand Down
Loading