Skip to content

Commit eabcfa2

Browse files
maria-rcksmaria-rcks
authored
fix: canonicalize git.cafe pull request links (#22634)
1 parent 2887855 commit eabcfa2

11 files changed

Lines changed: 174 additions & 28 deletions

File tree

‎apps/server/src/mcp/toolkits/pullRequests/handlers.test.ts‎

Lines changed: 33 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -353,6 +353,39 @@ describe("pull request toolkit handlers", () => {
353353
}),
354354
);
355355

356+
it.effect("canonicalizes GitCafe links independently of the thread's project host", () =>
357+
Effect.gen(function* () {
358+
for (const host of ["git.cafe", "staging.git.cafe"]) {
359+
for (const project of [makeProject(), makeProject(null)]) {
360+
for (const target of [
361+
{ host, repository: "PingDotGG/T3Code", number: 21353 },
362+
{ url: `https://${host}/PingDotGG/T3Code/pull/21353` },
363+
{ url: `https://${host}/PingDotGG/T3Code/pulls/21353` },
364+
]) {
365+
const harness = yield* makeHarness({ project });
366+
const result = yield* harness.call("link_pull_request", target);
367+
const url = `https://${host}/pingdotgg/t3code/pulls/21353`;
368+
expect(result).toMatchObject({
369+
host,
370+
repository: "pingdotgg/t3code",
371+
number: 21353,
372+
url,
373+
});
374+
expect(yield* Ref.get(harness.commands)).toMatchObject([
375+
{
376+
type: "thread.pull-request.link",
377+
host,
378+
repository: "pingdotgg/t3code",
379+
number: 21353,
380+
url,
381+
},
382+
]);
383+
}
384+
}
385+
}
386+
}),
387+
);
388+
356389
it.effect("builds the URL in the project host's own shape", () =>
357390
Effect.gen(function* () {
358391
const harness = yield* makeHarness({

‎apps/server/src/mcp/toolkits/pullRequests/handlers.ts‎

Lines changed: 10 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@ import {
66
type ThreadId,
77
type ThreadPullRequestLink,
88
} from "@t3tools/contracts";
9+
import { canonicalizeGitCafePullRequestUrl } from "@t3tools/source-control-definitions/registry";
910
import * as Cause from "effect/Cause";
1011
import * as Crypto from "effect/Crypto";
1112
import * as Effect from "effect/Effect";
@@ -76,7 +77,10 @@ const resolveTarget = Effect.fn("PullRequestsToolkit.resolveTarget")(function* (
7677
if (parsed === undefined) {
7778
return yield* new PullRequestUrlInvalidError({});
7879
}
79-
return { ...normalizeThreadPullRequestKey(parsed), url: input.url } satisfies ResolvedTarget;
80+
return {
81+
...normalizeThreadPullRequestKey(parsed),
82+
url: canonicalizeGitCafePullRequestUrl(input.url),
83+
} satisfies ResolvedTarget;
8084
}
8185
if (input.repository === undefined || input.number === undefined) {
8286
return yield* new PullRequestTargetIncompleteError({});
@@ -89,7 +93,10 @@ const resolveTarget = Effect.fn("PullRequestsToolkit.resolveTarget")(function* (
8993
const repository = input.repository.toLowerCase();
9094
// Without a URL, a public forge host says what shape its change request URLs take;
9195
// any other host is taken to run the project's own forge.
92-
const kind = sourceControlDefinitions.findByPublicHost(host)?.kind ?? projectHost.kind;
96+
const recognizedKind = sourceControlDefinitions.recognizeRemote(`https://${host}`)?.kind;
97+
const kind =
98+
sourceControlDefinitions.findByPublicHost(host)?.kind ??
99+
(recognizedKind === "gitcafe" ? recognizedKind : projectHost.kind);
93100
const url =
94101
kind === null
95102
? null
@@ -104,7 +111,7 @@ const resolveTarget = Effect.fn("PullRequestsToolkit.resolveTarget")(function* (
104111
}
105112
return {
106113
...normalizeThreadPullRequestKey({ host, repository, number: input.number, url }),
107-
url,
114+
url: canonicalizeGitCafePullRequestUrl(url),
108115
} satisfies ResolvedTarget;
109116
});
110117

‎apps/server/src/orchestration-v2/Orchestrator.ts‎

Lines changed: 6 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,7 @@ import {
1010
runRanAfter,
1111
usageLimitBlockedRun,
1212
} from "@t3tools/shared/orchestrationV2ThreadError";
13+
import { canonicalizeGitCafePullRequestUrl } from "@t3tools/source-control-definitions/registry";
1314
import { threadPullRequestsOf } from "../sourceControl/threadPullRequests.ts";
1415
import {
1516
normalizeThreadPullRequestKey,
@@ -641,7 +642,7 @@ function legacyPullRequestLink(
641642
return withPullRequestWatch(
642643
{
643644
...key,
644-
url: linked.url,
645+
url: canonicalizeGitCafePullRequestUrl(linked.url),
645646
source: "manual",
646647
linkedAt: DateTime.formatIso(now),
647648
snapshot: null,
@@ -3415,14 +3416,14 @@ const makeOrchestrator = Effect.fn("orchestrationV2.Orchestrator.layer")(functio
34153416
? {
34163417
...existing,
34173418
source: command.source,
3418-
url: command.url,
3419+
url: canonicalizeGitCafePullRequestUrl(command.url),
34193420
...(command.repositoryWebUrl !== undefined && {
34203421
repositoryWebUrl: command.repositoryWebUrl,
34213422
}),
34223423
}
34233424
: {
34243425
...key,
3425-
url: command.url,
3426+
url: canonicalizeGitCafePullRequestUrl(command.url),
34263427
...(command.repositoryWebUrl !== undefined && {
34273428
repositoryWebUrl: command.repositoryWebUrl,
34283429
}),
@@ -3443,7 +3444,7 @@ const makeOrchestrator = Effect.fn("orchestrationV2.Orchestrator.layer")(functio
34433444
? [
34443445
{
34453446
...branchKey,
3446-
url: branch.url,
3447+
url: canonicalizeGitCafePullRequestUrl(branch.url),
34473448
source: "manual" as const,
34483449
linkedAt: DateTime.formatIso(now),
34493450
snapshot: null,
@@ -3518,7 +3519,7 @@ const makeOrchestrator = Effect.fn("orchestrationV2.Orchestrator.layer")(functio
35183519
...linked.filter((link) => !threadPullRequestKeysEqual(link, key)),
35193520
{
35203521
...key,
3521-
url: command.link.url,
3522+
url: canonicalizeGitCafePullRequestUrl(command.link.url),
35223523
...((command.link.repositoryWebUrl ?? dismissed?.repositoryWebUrl) !==
35233524
undefined && {
35243525
repositoryWebUrl:

‎apps/server/src/orchestration-v2/runtimeLayer.test.ts‎

Lines changed: 19 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2247,6 +2247,25 @@ it.layer(layerTest)("OrchestrationV2LayerLive lifecycle", (it) => {
22472247
(yield* orchestrator.getThreadShell(threadId))?.pullRequests?.map((link) => link.number),
22482248
[2, 3, 4],
22492249
);
2250+
for (const [index, route] of ["pull", "pulls"].entries()) {
2251+
yield* orchestrator.dispatch({
2252+
type: "thread.pull-request.link",
2253+
commandId: CommandId.make(`gitcafe-pr-link-${index}`),
2254+
threadId,
2255+
host: "git.cafe",
2256+
repository: "pingdotgg/t3code",
2257+
number: 21353 + index,
2258+
url: `https://git.cafe/pingdotgg/t3code/${route}/${21353 + index}`,
2259+
source: "manual",
2260+
});
2261+
}
2262+
assert.isTrue((yield* maintenance.rebuild).valid);
2263+
assert.deepEqual(
2264+
(yield* orchestrator.getThreadShell(threadId))?.pullRequests
2265+
?.filter((link) => link.host === "git.cafe")
2266+
.map((link) => link.url),
2267+
[21353, 21354].map((number) => `https://git.cafe/pingdotgg/t3code/pulls/${number}`),
2268+
);
22502269
}),
22512270
);
22522271

‎packages/client-runtime/src/pullRequestLinkInput.test.ts‎

Lines changed: 27 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,33 @@ const project = {
1515
};
1616

1717
describe("resolveLinkPullRequestInput", () => {
18+
it.each(["git.cafe", "staging.git.cafe"])("canonicalizes both GitCafe routes on %s", (host) => {
19+
for (const [repository, route] of [
20+
["web", "pull"],
21+
["web", "pulls"],
22+
["pull", "pull"],
23+
["pull", "pulls"],
24+
["pulls", "pull"],
25+
["pulls", "pulls"],
26+
]) {
27+
const url = `https://${host}/acme/${repository}/${route}/42`;
28+
expect(
29+
sourceControlClients.get("gitcafe").parseChangeRequestUrl(new URL(url)),
30+
).not.toBeNull();
31+
expect(parsePullRequestReference(url)).toBe(url);
32+
expect(
33+
resolveLinkPullRequestInput({ reference: url, project: null, hasProject: () => true }),
34+
).toEqual({
35+
link: {
36+
host,
37+
repository: `acme/${repository}`,
38+
number: 42,
39+
url: `https://${host}/acme/${repository}/pulls/42`,
40+
},
41+
});
42+
}
43+
});
44+
1845
it.each([
1946
["https://bitbucket.org/acme/web/pull-requests/42", "bitbucket.org"],
2047
["https://github.acme.test/acme/web/pull/42", "github.acme.test"],

‎packages/client-runtime/src/pullRequestLinkInput.ts‎

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,5 @@
11
import type { RepositoryIdentity } from "@t3tools/contracts";
2+
import { canonicalizeGitCafePullRequestUrl } from "@t3tools/source-control-definitions/registry";
23

34
import { sourceControlClients, type ChangeRequestTerminology } from "./sourceControlClients.ts";
45

@@ -86,7 +87,12 @@ export function resolveLinkPullRequestInput(input: {
8687
return { error: `No project in this environment can read ${url.host}/${url.repository}.` };
8788
}
8889
return {
89-
link: { host: url.host, repository: url.repository, number: url.number, url: parsed },
90+
link: {
91+
host: url.host,
92+
repository: url.repository,
93+
number: url.number,
94+
url: canonicalizeGitCafePullRequestUrl(parsed),
95+
},
9096
};
9197
}
9298
const number = Number(parsed);

‎packages/source-control-definitions/src/registry.ts‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,8 @@ import * as GitCafe from "@t3tools/source-control-gitcafe/shared/definition";
66
import * as GitHub from "@t3tools/source-control-github/shared/definition";
77
import * as GitLab from "@t3tools/source-control-gitlab/shared/definition";
88

9+
export { canonicalizePullRequestUrl as canonicalizeGitCafePullRequestUrl } from "@t3tools/source-control-gitcafe/shared/definition";
10+
911
/**
1012
* The server and every client read hosts through this one registry, so they agree on which host
1113
* owns a URL or remote. GitHub comes first because a repository that has not reported its host

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

Lines changed: 8 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -242,12 +242,14 @@ describe("GitCafeSourceControlProvider", () => {
242242
};
243243
return Effect.gen(function* () {
244244
const provider = yield* GitCafeSourceControlProvider.make;
245-
yield* provider.checkoutChangeRequest({
246-
cwd: "/repo",
247-
reference: "https://git.cafe/team/project/pulls/7",
248-
...(context === undefined ? {} : { context }),
249-
});
250-
assert.deepStrictEqual(remotes, [expected]);
245+
for (const route of ["pull", "pulls"]) {
246+
yield* provider.checkoutChangeRequest({
247+
cwd: "/repo",
248+
reference: `https://git.cafe/team/project/${route}/7`,
249+
...(context === undefined ? {} : { context }),
250+
});
251+
}
252+
assert.deepStrictEqual(remotes, [expected, expected]);
251253
}).pipe(
252254
Effect.provide(
253255
Layer.merge(GitCafeApi.layer, NoFiles).pipe(

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

Lines changed: 6 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,7 @@ import {
2020
import { randomUuidV4 } from "@t3tools/provider-core/server/randomUuid";
2121
import * as SourceControlHost from "@t3tools/source-control-core/server/SourceControlHost";
2222
import * as SourceControlProvider from "@t3tools/source-control-core/server/SourceControlProvider";
23+
import { parsePullRequestUrl } from "../shared/definition.ts";
2324

2425
import * as GitCafeApi from "./GitCafeApi.ts";
2526
import * as GitCafeCredentials from "./GitCafeCredentials.ts";
@@ -166,8 +167,6 @@ const cloneUrls = (target: Target) => ({
166167
const isSshRemote = (remoteUrl: string | undefined) =>
167168
remoteUrl !== undefined && (/^ssh:\/\//iu.test(remoteUrl) || /^[^/:]+@[^/:]+:/u.test(remoteUrl));
168169

169-
const pullUrlPattern = /^https:\/\/([^/]+)\/([^/]+\/[^/]+)\/pulls\/(\d+)\/?(?:[?#].*)?$/u;
170-
171170
export const make = Effect.gen(function* () {
172171
const api = yield* GitCafeApi.GitCafeApi;
173172
const host = yield* SourceControlHost.SourceControlHost;
@@ -242,15 +241,15 @@ export const make = Effect.gen(function* () {
242241
readonly context?: SourceControlProvider.SourceControlProviderContext;
243242
readonly reference: string;
244243
}) {
245-
const match = pullUrlPattern.exec(input.reference.trim());
246-
const urlHost = match?.[1] ? GitCafeHosts.gitCafeHost(match[1]) : null;
244+
const match = parsePullRequestUrl(input.reference.trim());
245+
const urlHost = match === null ? null : GitCafeHosts.gitCafeHost(match.host);
247246
const target: Target =
248-
urlHost && match?.[2]
249-
? { host: urlHost, repository: match[2] }
247+
urlHost && match
248+
? { host: urlHost, repository: match.repository }
250249
: yield* resolveTarget("getChangeRequest", input);
251250
const number = match
252251
? urlHost
253-
? match[3]
252+
? String(match.number)
254253
: undefined
255254
: /^#?([1-9]\d*)$/u.exec(input.reference.trim())?.[1];
256255
if (number === undefined)

‎packages/source-control-gitcafe/src/shared/definition.test.ts‎

Lines changed: 35 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,11 +1,45 @@
11
import { recognizeRemoteWith } from "@t3tools/source-control-core/shared/definition";
22
import { describe, expect, it } from "vite-plus/test";
33

4-
import { definition } from "./definition.ts";
4+
import { canonicalizePullRequestUrl, definition, parsePullRequestUrl } from "./definition.ts";
55

66
const parse = (url: string) => definition.parseChangeRequestUrl(new URL(url));
77

88
describe("GitCafe change request URLs", () => {
9+
it.each(["git.cafe", "staging.git.cafe"])("normalizes links for storage on %s", (host) => {
10+
for (const repository of ["web", "pull", "pulls"]) {
11+
for (const route of ["pull", "pulls"]) {
12+
const url = `https://${host}/Acme/${repository}/${route}/42/files?w=1#diff`;
13+
expect(parsePullRequestUrl(url)).toMatchObject({
14+
host,
15+
repository: `acme/${repository}`,
16+
number: 42,
17+
});
18+
expect(definition.isChangeRequestReference(url)).toBe(true);
19+
expect(canonicalizePullRequestUrl(url)).toBe(`https://${host}/acme/${repository}/pulls/42`);
20+
}
21+
}
22+
});
23+
24+
it("preserves other URLs and rejects invalid GitCafe references", () => {
25+
for (const url of [
26+
"https://github.com/team/repo/pull/7",
27+
"https://forge.example/team/repo/pulls/7",
28+
"https://not.git.cafe/team/repo/pull/7",
29+
"https://git.cafe.evil.test/team/repo/pull/7",
30+
"https://git.cafe/tfs/collection/project/_git/repo/pullrequest/7",
31+
"https://git.cafe/team/repo/pull/0",
32+
"https://git.cafe/team/repo/pulls/9007199254740992",
33+
"https://staging.git.cafe/team/repo/pull/7suffix",
34+
"ftp://git.cafe/team/repo/pull/7",
35+
"not a url",
36+
]) {
37+
expect(parsePullRequestUrl(url), url).toBeNull();
38+
expect(definition.isChangeRequestReference(url), url).toBe(false);
39+
expect(canonicalizePullRequestUrl(url), url).toBe(url);
40+
}
41+
});
42+
943
it("reads its canonical /pull/ path and the older /pulls/ path on its own hosts", () => {
1044
for (const path of ["pull", "pulls"]) {
1145
expect(parse(`https://git.cafe/PingDotGG/T3Code/${path}/23897`), path).toEqual({

0 commit comments

Comments
 (0)