Skip to content

Commit cb0ccdb

Browse files
juliusmarmingeclaude
juliusmarminge
and
claude
committed
fix(source-control): GitCafe listings and line counts degrade instead of failing
- An unreadable actor list (`/filter-options`) lists the repository unnarrowed instead of failing it, as the provider contract asks; a refused token or rate limit still fails, since the service acts on those. - A hunk read that loses a race with a push keeps the changed-file count and zeroes only the line totals. - The redirect test now checks the `redirect` mode the request carries, so it fails without the fix it pins. Co-Authored-By: Claude Opus 5.5 <[email protected]>
1 parent 7f6bf98 commit cb0ccdb

2 files changed

Lines changed: 87 additions & 32 deletions

File tree

‎packages/source-control-gitcafe/src/server/GitCafePullRequestProvider.test.ts‎

Lines changed: 65 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -16,8 +16,10 @@
1616
*/
1717
import { assert, describe, it } from "@effect/vitest";
1818
import * as Effect from "effect/Effect";
19+
import * as Context from "effect/Context";
1920
import * as Layer from "effect/Layer";
2021
import * as Schema from "effect/Schema";
22+
import * as FetchHttpClient from "effect/http/FetchHttpClient";
2123
import * as HttpClient from "effect/http/HttpClient";
2224
import type * as HttpClientRequest from "effect/http/HttpClientRequest";
2325
import * as HttpClientResponse from "effect/http/HttpClientResponse";
@@ -38,6 +40,8 @@ interface SentRequest {
3840
readonly query: URLSearchParams;
3941
readonly host: string;
4042
readonly body: unknown;
43+
/** The `redirect` mode FetchHttpClient would hand to fetch for this request. */
44+
readonly redirect: string | undefined;
4145
}
4246

4347
/**
@@ -49,30 +53,33 @@ function fakeGitCafe(
4953
options: { readonly status?: number; readonly env?: NodeJS.ProcessEnv } = {},
5054
) {
5155
const sent: Array<SentRequest> = [];
52-
const client = HttpClient.make((request: HttpClientRequest.HttpClientRequest) => {
53-
const url = new URL(request.url);
54-
assert.isTrue(url.pathname.startsWith("/api/"), url.pathname);
55-
assert.strictEqual(request.headers.authorization, "Bearer env-token");
56-
const entry: SentRequest = {
57-
method: request.method,
58-
path: url.pathname.slice("/api".length),
59-
query: url.searchParams,
60-
host: url.host,
61-
body:
62-
request.body._tag === "Uint8Array"
63-
? decodeJson(new TextDecoder().decode(request.body.body))
64-
: undefined,
65-
};
66-
sent.push(entry);
67-
const route = routes[`${entry.method} ${entry.path}`];
68-
const response =
69-
route === undefined
70-
? new Response(encodeJson({ type: "https://cafe.sh/errors/not-found" }), { status: 404 })
71-
: new Response(encodeJson(typeof route === "function" ? route(entry) : route), {
72-
status: options.status ?? 200,
73-
});
74-
return Effect.succeed(HttpClientResponse.fromWeb(request, response));
75-
});
56+
const client = HttpClient.make(
57+
(request: HttpClientRequest.HttpClientRequest, _url, _signal, fiber) => {
58+
const url = new URL(request.url);
59+
assert.isTrue(url.pathname.startsWith("/api/"), url.pathname);
60+
assert.strictEqual(request.headers.authorization, "Bearer env-token");
61+
const entry: SentRequest = {
62+
method: request.method,
63+
path: url.pathname.slice("/api".length),
64+
query: url.searchParams,
65+
host: url.host,
66+
body:
67+
request.body._tag === "Uint8Array"
68+
? decodeJson(new TextDecoder().decode(request.body.body))
69+
: undefined,
70+
redirect: Context.getOrUndefined(fiber.context, FetchHttpClient.RequestInit)?.redirect,
71+
};
72+
sent.push(entry);
73+
const route = routes[`${entry.method} ${entry.path}`];
74+
const response =
75+
route === undefined
76+
? new Response(encodeJson({ type: "https://cafe.sh/errors/not-found" }), { status: 404 })
77+
: new Response(encodeJson(typeof route === "function" ? route(entry) : route), {
78+
status: options.status ?? 200,
79+
});
80+
return Effect.succeed(HttpClientResponse.fromWeb(request, response));
81+
},
82+
);
7683
const layer = GitCafeApi.layer.pipe(
7784
Layer.provide(GitCafeCredentials.layer),
7885
Layer.provide(TestSourceControlHost.layer()),
@@ -218,8 +225,6 @@ describe("GitCafePullRequestProvider", () => {
218225
for (const [status, reason] of [
219226
[401, "unauthenticated"],
220227
[429, "rate-limited"],
221-
// A redirect is never followed with the token, so it answers as a failed request.
222-
[302, "failed"],
223228
] as const) {
224229
const server = fakeGitCafe(
225230
{ "GET /repos/owner/repo/pulls/7/stack": { stack: null } },
@@ -237,6 +242,22 @@ describe("GitCafePullRequestProvider", () => {
237242
}),
238243
);
239244

245+
it.effect("never lets fetch follow a redirect with the token", () => {
246+
const server = fakeGitCafe({ "GET /repos/owner/repo/pulls/7/stack": { stack: null } });
247+
return GitCafePullRequestProvider.make.pipe(
248+
Effect.flatMap((provider) => provider.getChangeRequestStack!(target)),
249+
Effect.tap(() =>
250+
Effect.sync(() =>
251+
assert.deepStrictEqual(
252+
server.sent.map((r) => r.redirect),
253+
["manual"],
254+
),
255+
),
256+
),
257+
Effect.provide(server.layer),
258+
);
259+
});
260+
240261
it.effect("reads the stack a pull request belongs to", () => {
241262
const server = fakeGitCafe({
242263
"GET /repos/owner/repo/pulls/7/stack": stack([{ number: 7, headOid }]),
@@ -545,6 +566,24 @@ describe("GitCafePullRequestProvider", () => {
545566
}).pipe(Effect.provide(server.layer));
546567
});
547568

569+
it.effect("lists unnarrowed when GitCafe can't say who the viewer is", () => {
570+
// No `filter-options` route: the fake answers 404, which is an ordinary failure.
571+
const server = fakeGitCafe({ "GET /repos/owner/repo/pulls": { items: [], next: null } });
572+
return Effect.gen(function* () {
573+
const provider = yield* GitCafePullRequestProvider.make;
574+
yield* provider.listChangeRequests({
575+
...target,
576+
state: "open",
577+
involvement: "authored",
578+
viewer: "alice",
579+
limit: 10,
580+
});
581+
const listing = server.sent.find((request) => request.path === "/repos/owner/repo/pulls");
582+
assert.isDefined(listing);
583+
assert.isNull(listing!.query.get("authors"));
584+
}).pipe(Effect.provide(server.layer));
585+
});
586+
548587
it.effect("counts lines from hunk reads and offers only strategies no blocker names", () => {
549588
const snapshot = { version: 4, headOid, comparisonBaseOid: baseOid };
550589
const files = (from: number, count: number) =>

‎packages/source-control-gitcafe/src/server/GitCafePullRequestProvider.ts‎

Lines changed: 22 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,5 @@
11
import * as Effect from "effect/Effect";
2+
import * as Option from "effect/Option";
23
import * as Schema from "effect/Schema";
34
import {
45
NonNegativeInt,
@@ -381,6 +382,8 @@ export const make = Effect.gen(function* () {
381382
after = batch.next;
382383
if (after === null || after === previous || batch.items.length === 0) break;
383384
}
385+
// A hunk read that loses a race with a push leaves the counts at zero rather than dropping
386+
// the file count with them.
384387
const counted =
385388
files.some((file) => file.additions !== undefined) ||
386389
files.length > LINE_STATS_MAX_FILES ||
@@ -390,7 +393,7 @@ export const make = Effect.gen(function* () {
390393
chunk(detailablePaths(files), DIFF_FILES_BATCH),
391394
(paths) => readDiffFiles(input, revision, paths),
392395
{ concurrency: 4 },
393-
)).flat();
396+
).pipe(Effect.orElseSucceed(() => [[]]))).flat();
394397
return {
395398
changedFiles: files.length,
396399
additions: counted.reduce((total, file) => total + (file.additions ?? 0), 0),
@@ -461,17 +464,30 @@ export const make = Effect.gen(function* () {
461464
const involved = input.involvement === "authored" || input.involvement === "reviewing";
462465
let actorId: string | undefined;
463466
if (involved) {
467+
// A host narrows as far as it can: when the actor list can't be read, the listing comes
468+
// back unnarrowed and the page's own involvement filter does the rest. A refused token
469+
// or a rate limit still fails, since the service acts on those for the whole host.
464470
const options = yield* read(
465471
"listChangeRequests",
466472
input,
467473
{ path: `${base}/filter-options` },
468474
FilterOptions,
475+
).pipe(
476+
Effect.map(Option.some),
477+
Effect.catchTags({
478+
PullRequestProviderError: (error) =>
479+
error.reason === "unauthenticated" || error.reason === "rate-limited"
480+
? Effect.fail(error)
481+
: Effect.succeed(Option.none()),
482+
}),
469483
);
470-
actorId = options.actors.find(
471-
(actor) => actor.handle.toLowerCase() === input.viewer.toLowerCase(),
472-
)?.actorId;
473-
// A viewer GitCafe knows no actor for has authored and been asked to review nothing.
474-
if (actorId === undefined) return { items: [], truncated: false, continues: false };
484+
if (Option.isSome(options)) {
485+
actorId = options.value.actors.find(
486+
(actor) => actor.handle.toLowerCase() === input.viewer.toLowerCase(),
487+
)?.actorId;
488+
// A viewer GitCafe knows no actor for has authored and been asked to review nothing.
489+
if (actorId === undefined) return { items: [], truncated: false, continues: false };
490+
}
475491
}
476492
const limit = Math.max(1, input.limit);
477493
const items: Array<Json.GitCafePull> = [];

0 commit comments

Comments
 (0)