Skip to content
Merged
62 changes: 62 additions & 0 deletions apps/server/src/pullRequest/PullRequestService.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,9 @@ import * as PullRequestFilesViewed from "../persistence/PullRequestFilesViewed.t
import * as RepositoryIdentityResolver from "../project/RepositoryIdentityResolver.ts";
import * as SourceControlProviderRegistry from "../sourceControl/SourceControlProviderRegistry.ts";
import * as SourceControlRateLimit from "@t3tools/source-control-core/server/SourceControlRateLimit";
import * as GitLabCli from "@t3tools/source-control-gitlab/server/GitLabCli";
import * as GitLabPullRequestCli from "@t3tools/source-control-gitlab/server/GitLabPullRequestCli";
import * as GitLabPullRequestProvider from "@t3tools/source-control-gitlab/server/GitLabPullRequestProvider";
import * as ForgejoCli from "@t3tools/source-control-forgejo/server/ForgejoCli";
import * as ForgejoPullRequestProvider from "@t3tools/source-control-forgejo/server/ForgejoPullRequestProvider";
import {
Expand Down Expand Up @@ -7654,3 +7657,62 @@ it.effect.each([
assert.deepStrictEqual(calls, [expected]);
}),
);

it.effect.each(["gitlab.com", "gitlab.example.test"])(
"pauses uncached GitLab activity reads after an optional endpoint rate limit on %s",
(host) =>
Effect.gen(function* () {
let commandCalls = 0;
const provider = yield* GitLabPullRequestProvider.make.pipe(
Effect.provide(
GitLabPullRequestCli.layer.pipe(
Layer.provide(
Layer.mock(GitLabCli.GitLabCli)({
execute: () =>
Effect.sync(() => {
commandCalls += 1;
}).pipe(
Effect.andThen(
Effect.fail(
new GitLabCli.GitLabCliRateLimitError({
operation: "execute",
command: "glab",
cwd: "/repo",
cause: null,
}),
),
),
),
}),
),
),
),
);
const service = yield* makeService({
projects: [
project({
id: "p1",
title: "web",
workspaceRoot: "/repo",
repository: "acme/web",
provider: "gitlab",
host,
}),
],
providers: [provider],
});
const reference = { projectId: "p1" as ProjectId, repository: "acme/web", number: 1 };
const first = yield* Effect.flip(service.activity(reference));
assert.instanceOf(first, PullRequestOperationError);
assert.instanceOf(first.cause, PullRequestProviderError);
assert.strictEqual(first.cause.reason, "rate-limited");
const firstCommandCalls = commandCalls;
assert.isAbove(firstCommandCalls, 0);
// A different request bypasses the activity cache and must stop at the host backoff.
const paused = yield* Effect.flip(service.activity({ ...reference, number: 2 }));
assert.instanceOf(paused, PullRequestOperationError);
assert.instanceOf(paused.cause, PullRequestProviderError);
assert.strictEqual(paused.cause.reason, "rate-limited");
assert.strictEqual(commandCalls, firstCommandCalls);
}),
);
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ import { describe, expect, it } from "@effect/vitest";
import * as Effect from "effect/Effect";
import * as Layer from "effect/Layer";

import * as AzureDevOpsCli from "./AzureDevOpsCli.ts";
import * as AzureDevOpsPullRequestCli from "./AzureDevOpsPullRequestCli.ts";
import {
LOCATION_CACHE_CAPACITY,
Expand Down Expand Up @@ -418,3 +419,44 @@ describe("what one diff slice spends", () => {
}),
);
});

describe("optional activity failures", () => {
const context = {
operation: "execute" as const,
command: "az" as const,
cwd: "/w",
argumentCount: 1,
cause: null,
};
const failures = [
[new AzureDevOpsCli.AzureDevOpsCliAuthenticationError(context), "unauthenticated"],
[new AzureDevOpsCli.AzureDevOpsCliRateLimitError(context), "rate-limited"],
[new AzureDevOpsCli.AzureDevOpsCliUnavailableError(context), "missing-tool"],
[new AzureDevOpsCli.AzureDevOpsCommandFailedError(context), null],
] as const;
it.effect.each(failures)(
"preserves critical thread errors and recovers ordinary errors: %s",
([error, reason]) =>
Effect.gen(function* () {
const provider = yield* make.pipe(
Effect.provide(
Layer.mock(AzureDevOpsPullRequestCli.AzureDevOpsPullRequestCli)({
getPullRequest: () => Effect.succeed(PULL_REQUEST),
listThreads: () => Effect.fail(error),
}),
),
);
const read = provider.getChangeRequestActivity({
cwd: "/w",
repository: "web",
host: "dev.azure.com",
number: 7,
});
if (reason === null) {
expect(yield* read).toMatchObject({ comments: [], commentsTruncated: true });
} else {
expect(yield* Effect.flip(read)).toMatchObject({ reason, cause: error });
}
}),
);
});
Original file line number Diff line number Diff line change
Expand Up @@ -386,7 +386,6 @@ export const make = Effect.gen(function* () {

getChangeRequestActivity: (input) =>
cli.getPullRequest({ cwd: input.cwd, number: input.number }).pipe(
Effect.mapError(fail("getChangeRequestActivity")),
Effect.flatMap((pullRequest) =>
(pullRequest.location === null
? Effect.succeed({ comments: [], truncated: true })
Expand All @@ -398,7 +397,14 @@ export const make = Effect.gen(function* () {
})
.pipe(
Effect.map((comments) => ({ comments, truncated: false })),
Effect.orElseSucceed(() => ({ comments: [], truncated: true })),
Effect.catch((error) => {
const { reason } = azureDevOpsProviderFailure(error);
return reason === "rate-limited" ||
reason === "unauthenticated" ||
reason === "missing-tool"
? Effect.fail(error)
: Effect.succeed({ comments: [], truncated: true });
}),
)
).pipe(
Effect.map((conversation): ProviderChangeRequestActivity => ({
Expand All @@ -410,6 +416,7 @@ export const make = Effect.gen(function* () {
})),
),
),
Effect.mapError(fail("getChangeRequestActivity")),
),

// No request at all: Azure has nothing to say about the viewer that a pull request read can
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ for (const operation of [
"listCommits",
] as const) {
it.effect.each(["response", "body read"])(
`preserves rate limits from ${operation} on %s errors while recovering other optional-read failures`,
`preserves authentication and rate limits from ${operation} on %s errors while recovering other optional-read failures`,
(variant) =>
Effect.gen(function* () {
const pullRequest = Result.getOrThrow(
Expand All @@ -33,7 +33,7 @@ for (const operation of [
"links": { "html": { "href": "https://bitbucket.org/acme/web/pull-requests/1" } }
}`),
);
for (const status of [429, 403]) {
for (const status of [429, 401, 403]) {
const provider = yield* make.pipe(
Effect.provide(
Layer.mock(BitbucketPullRequestApi.BitbucketPullRequestApi)({
Expand Down Expand Up @@ -77,6 +77,11 @@ for (const operation of [
_tag: "Failure",
failure: { reason: "rate-limited", retryAt: 120_000 },
});
} else if (status === 401 && variant === "response") {
expect(result).toMatchObject({
_tag: "Failure",
failure: { reason: "unauthenticated" },
});
} else {
expect(result._tag).toBe("Success");
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -137,7 +137,8 @@ export const make = Effect.gen(function* () {
) => {
const recover = () => Effect.succeed(fallback);
return Effect.catchTags(read, {
BitbucketResponseError: (error) => (error.status === 429 ? Effect.fail(error) : recover()),
BitbucketResponseError: (error) =>
error.status === 401 || error.status === 429 ? Effect.fail(error) : recover(),
BitbucketUntrustedUrlError: recover,
BitbucketRepositoryLocatorError: recover,
BitbucketRequestError: recover,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -749,3 +749,46 @@ describe("GitCafePullRequestProvider", () => {
}).pipe(Effect.provide(server.layer));
});
});

it.effect.each(["unauthenticated", "rate-limited", "missing-tool", "failed"] as const)(
"preserves critical optional reaction failures and recovers ordinary failures: %s",
(reason) =>
Effect.gen(function* () {
const provider = yield* GitCafePullRequestProvider.make.pipe(
Effect.provide(
Layer.mock(GitCafeApi.GitCafeApi)({
request: ({ path, operation, host }) => {
if (path.includes("/reactions/"))
return Effect.fail(
new GitCafeApi.GitCafeApiError({
host,
operation,
reason,
status: null,
detail: "Reactions unavailable",
}),
);
const body = path.includes("/comments")
? { items: [root], next: null }
: path.includes("/reviews")
? { items: [], next: null }
: path.includes("/commits")
? { items: [], truncated: false, next: null, headOid }
: pull;
return Effect.succeed(encodeJson(body));
},
}),
),
);
const read = provider.getChangeRequestActivity(target);
if (reason === "failed") {
const activity = yield* read;
assert.strictEqual(activity.comments[0]?.id, root.id);
assert.isUndefined(activity.reactions);
} else {
const error = yield* Effect.flip(read);
assert.strictEqual(error.reason, reason);
assert.strictEqual(error.operation, "getChangeRequestActivity");
}
}),
);
Original file line number Diff line number Diff line change
Expand Up @@ -650,9 +650,16 @@ export const make = Effect.gen(function* () {
pull.headOid === null
? Effect.succeed({ items: [], next: null })
: readPages(operation, input, `${path}/commits`, Json.GitCafeCommits),
// Reactions decorate the conversation; a failed read leaves them out, not it.
// An ordinary reaction failure leaves the conversation intact; account and host
// failures still reach the service's error handling and rate-limit backoff.
readPages(operation, input, `${path}/reactions/`, Json.GitCafeReactions).pipe(
Effect.orElseSucceed(() => undefined),
Effect.catch((error) =>
error.reason === "unauthenticated" ||
error.reason === "rate-limited" ||
error.reason === "missing-tool"
? Effect.fail(error)
: Effect.void,
),
),
],
{ concurrency: 4 },
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import * as Result from "effect/Result";
import type { PullRequestReaction } from "@t3tools/contracts";

import { decodePullRequestDetailJson } from "./gitHubPullRequestJson.ts";
import * as GitHubCredentials from "./GitHubCredentials.ts";
import * as GitHubApi from "./GitHubApi.ts";
import * as SourceControlRateLimit from "@t3tools/source-control-core/server/SourceControlRateLimit";
import * as GitHubPullRequestApi from "./GitHubPullRequestApi.ts";
Expand Down Expand Up @@ -1066,3 +1067,54 @@ describe("loginAvatarUrl", () => {
}
});
});

describe("optional activity failures", () => {
const context = { host: "github.com", operation: "listReviewThreadComments" };
const ordinary = new GitHubApi.GitHubApiRequestError({ ...context, cause: null });
const failures = [
[new GitHubApi.GitHubApiAuthenticationError(context), "unauthenticated"],
[new GitHubApi.GitHubApiRateLimitError({ ...context, retryAt: 120_000 }), "rate-limited"],
[new GitHubCredentials.GitHubCliMissingError({ host: context.host }), "missing-tool"],
[ordinary, null],
] as const;
it.effect.each(failures)(
"preserves critical review-thread errors and recovers ordinary errors: %s",
([error, reason]) =>
Effect.gen(function* () {
const provider = yield* make.pipe(
Effect.provide(
Layer.mock(GitHubPullRequestApi.GitHubPullRequestApi)({
revalidateChecks: (_input, read) => read,
getPullRequestActivity: () =>
Effect.succeed({
author: null,
comments: [],
commits: [
{
oid: "abc",
messageHeadline: "Keep this commit",
committedDate: "2026-07-01T00:00:00Z",
authors: [],
},
],
}),
listReviewThreadComments: () => Effect.fail(error),
}),
),
);
const read = provider.getChangeRequestActivity({
cwd: "/w",
repository: "acme/web",
host: context.host,
number: 7,
});
if (reason === null) {
const activity = yield* read;
expect(activity.commits[0]?.oid).toBe("abc");
expect(activity.commentsTruncated).toBe(true);
} else {
expect(yield* Effect.flip(read)).toMatchObject({ reason, cause: error });
}
}),
);
});
Original file line number Diff line number Diff line change
Expand Up @@ -410,26 +410,36 @@ export const make = Effect.gen(function* () {
// Line comments live on review threads, which `gh pr view --json` cannot reach. A
// GraphQL hiccup degrades to a truncated conversation rather than blanking activity.
cli.listReviewThreadComments(input).pipe(
Effect.orElseSucceed(() => ({
comments: [],
dismissalsByReviewId: new Map<string, string>(),
reactions: [],
reactionsById: new Map<string, ReadonlyArray<PullRequestReaction>>(),
editedAtById: new Map<string, string>(),
reviewThreads: [],
commentCount: 0,
truncated: true,
reviewThreadsTruncated: true,
reviewers: [],
avatarsByLogin: new Map<string, string>(),
botLogins: new Set<string>(),
commitStats: new Map<
string,
{ readonly additions: number; readonly deletions: number }
>(),
commits: [],
viewer: { canUpdate: true, didAuthor: false },
})),
Effect.catch((error) => {
const { reason } = gitHubProviderFailure(error);
if (
reason === "rate-limited" ||
reason === "unauthenticated" ||
reason === "missing-tool"
) {
return Effect.fail(error);
}
return Effect.succeed({
comments: [],
dismissalsByReviewId: new Map<string, string>(),
reactions: [],
reactionsById: new Map<string, ReadonlyArray<PullRequestReaction>>(),
editedAtById: new Map<string, string>(),
reviewThreads: [],
commentCount: 0,
truncated: true,
reviewThreadsTruncated: true,
reviewers: [],
avatarsByLogin: new Map<string, string>(),
botLogins: new Set<string>(),
commitStats: new Map<
string,
{ readonly additions: number; readonly deletions: number }
>(),
commits: [],
viewer: { canUpdate: true, didAuthor: false },
});
}),
),
],
{ concurrency: 2 },
Expand Down
Loading
Loading