From 51347aca3aa73112bc1e288ae45507ce515f42a2 Mon Sep 17 00:00:00 2001 From: Julius Marminge Date: Fri, 9 Oct 2026 16:22:37 -0700 Subject: [PATCH] refactor(source-control): Forgejo identity and Azure DevOps addressing move into their packages (#17624) Co-authored-by: Claude Opus 5.5 --- .../pullRequest/PullRequestService.test.ts | 5 +- .../src/pullRequest/PullRequestService.ts | 37 ++++++------ apps/server/src/server.ts | 40 +++++-------- .../server/AzureDevOpsPullRequestProvider.ts | 2 + .../src/server/PullRequestProvider.ts | 13 ++++ .../src/server/SourceControlProvider.ts | 13 ++++ .../ForgejoSourceControlProvider.test.ts | 60 +++++++++++++++++++ .../server/ForgejoSourceControlProvider.ts | 38 ++++++++++++ 8 files changed, 162 insertions(+), 46 deletions(-) diff --git a/apps/server/src/pullRequest/PullRequestService.test.ts b/apps/server/src/pullRequest/PullRequestService.test.ts index 04d4c879a6566ee1850b3843fb29d1a5dfe4f74e..c74faa92f06dc5629390f69fad926ed0875e6d7a 100644 --- a/apps/server/src/pullRequest/PullRequestService.test.ts +++ b/apps/server/src/pullRequest/PullRequestService.test.ts @@ -463,8 +463,11 @@ function fakeProvider( setReaction: () => Effect.void, listReviewerCandidates: () => Effect.succeed({ candidates: [], truncated: false }), setReviewerRequest: () => Effect.void, - // GitHub's own merge message rewrite, which the service reads instead of the kind. + // The hosts' own resolvers, which the service reads instead of the kind. ...(kind === "github" ? { mergeMessageRewrite: (message: string) => message } : {}), + ...(kind === "azure-devops" + ? { repositoryKey: ({ canonicalKey }: { readonly canonicalKey: string }) => canonicalKey } + : {}), ...overrides, }; } diff --git a/apps/server/src/pullRequest/PullRequestService.ts b/apps/server/src/pullRequest/PullRequestService.ts index 3644fefe985a413fe83a63fb2212ed9976695781..b44d713c3158896be8d9465b20476fa456aa11cb 100644 --- a/apps/server/src/pullRequest/PullRequestService.ts +++ b/apps/server/src/pullRequest/PullRequestService.ts @@ -369,6 +369,12 @@ export interface SupportedProject { * Unique where `repository` is not: Azure's is a bare name that repeats across an organisation. */ readonly remote: string; + /** + * The host's own key for this checkout's repository (`api.repositoryKey`), normalized, or null + * where `owner/name` on the host identifies it. When set, only a reference whose key matches + * is served by this checkout. + */ + readonly repositoryKey: string | null; } /** @@ -589,6 +595,7 @@ function withRateLimitBackoff( ...(api.mergeMessageRewrite === undefined ? {} : { mergeMessageRewrite: api.mergeMessageRewrite }), + ...(api.repositoryKey === undefined ? {} : { repositoryKey: api.repositoryKey }), // Refused during a pause like any other read, except for the caller that asks for the // bypass: a lookup that failed is not held, so letting every background read through would // spawn this host's CLI on each of them and re-extend the pause it was already in. @@ -852,10 +859,9 @@ export const make = Effect.gen(function* () { if (roots === undefined) viewerRoots.set(host, [project.workspaceRoot]); else if (!roots.includes(project.workspaceRoot)) roots.push(project.workspaceRoot); } - const key = listCursorKey( - host, - kind === "azure-devops" ? identity.canonicalKey : repository, - ); + const repositoryKey = + api?.repositoryKey?.({ canonicalKey: identity.canonicalKey }) ?? null; + const key = listCursorKey(host, repositoryKey ?? repository); if (seen.has(key)) continue; seen.add(key); if (api === null) { @@ -870,10 +876,9 @@ export const make = Effect.gen(function* () { api: withRateLimitBackoff(api, host, rateLimits), repository, host, - remote: - kind === "azure-devops" - ? identity.canonicalKey - : normalizeGitRemoteUrl(`https://${host}/${repository}`), + remote: repositoryKey ?? normalizeGitRemoteUrl(`https://${host}/${repository}`), + repositoryKey: + repositoryKey === null ? null : canonicalRepositoryKey(repositoryKey.toLowerCase()), }); } return { supported, unimplemented, viewerRoots }; @@ -922,25 +927,21 @@ export const make = Effect.gen(function* () { const route = supported.find( (candidate) => - candidate.api.kind === "azure-devops" && - candidate.project.repositoryIdentity != null && - canonicalRepositoryKey( - candidate.project.repositoryIdentity.canonicalKey.toLowerCase(), - ) === repositoryKey, + candidate.repositoryKey !== null && candidate.repositoryKey === repositoryKey, ) ?? onHost.find( (candidate) => - candidate.api.kind !== "azure-devops" && + candidate.repositoryKey === null && candidate.repository.toLowerCase() === repository.toLowerCase(), ) ?? - onHost.find((candidate) => candidate.api.kind !== "azure-devops"); + onHost.find((candidate) => candidate.repositoryKey === null); if (route === undefined) { return Effect.fail( new PullRequestUnavailableError({ reason: "provider-unsupported" }), ); } return Effect.succeed( - route.api.kind === "azure-devops" || + route.repositoryKey !== null || route.repository.toLowerCase() === repository.toLowerCase() ? route : { @@ -2099,9 +2100,7 @@ export const make = Effect.gen(function* () { ), Effect.mapError(toPullRequestError("runAction")), Effect.as( - project.api.kind === "azure-devops" - ? input.repository.trim() - : project.repository, + project.repositoryKey !== null ? input.repository.trim() : project.repository, ), ), ), diff --git a/apps/server/src/server.ts b/apps/server/src/server.ts index 80c5a73ac4c185511c156a7d25d22ce5491ec75b..e24cb335b487014a467f9e6047973278aab96259 100644 --- a/apps/server/src/server.ts +++ b/apps/server/src/server.ts @@ -52,7 +52,6 @@ import * as ProviderHostLive from "./provider/ProviderHostLive.ts"; import * as AcpRegistrySupport from "@t3tools/provider-acp-registry/server/AcpRegistrySupport"; import * as CheckpointDiffQuery from "./checkpointing/CheckpointDiffQuery.ts"; import * as CheckpointStore from "./checkpointing/CheckpointStore.ts"; -import * as ForgejoCli from "@t3tools/source-control-forgejo/server/ForgejoCli"; import * as SourceControlBuiltInDrivers from "./sourceControl/builtInDrivers.ts"; import * as TextGeneration from "./textGeneration/TextGeneration.ts"; import * as ProviderInstanceRegistryHydration from "./provider/ProviderInstanceRegistryHydration.ts"; @@ -315,32 +314,21 @@ const layerRepositoryIdentityResolver = Layer.effect( Effect.gen(function* () { const registry = yield* SourceControlProviderRegistry.SourceControlProviderRegistry; return yield* RepositoryIdentityResolver.make({ + // Each host that can refine an identity gets a turn; the first one that changes it wins. refine: Effect.fn(function* (identity: RepositoryIdentity) { - const remote = ForgejoCli.parseForgejoRemote(identity.locator.remoteUrl); - if ( - !remote || - !identity.rootPath || - (identity.provider !== undefined && - identity.provider !== "unknown" && - identity.provider !== "forgejo") - ) - return identity; - const handle = yield* registry.resolveHandle({ - cwd: identity.rootPath, - context: { - provider: { kind: "unknown", name: "Unknown", baseUrl: "" }, - remoteName: identity.locator.remoteName, - remoteUrl: identity.locator.remoteUrl, - }, - }); - if (handle.context?.provider.kind !== "forgejo") return identity; - const baseUrl = handle.context.provider.baseUrl.replace(/\/+$/, ""); - const basePath = new URL(baseUrl).pathname.replace(/^\/+|\/+$/g, ""); - const path = - !remote.ssh && basePath && remote.path.startsWith(`${basePath}/`) - ? remote.path.slice(basePath.length + 1) - : remote.path; - return { ...identity, provider: "forgejo", webUrl: `${baseUrl}/${path}` }; + for (const kind of SourceControlBuiltInDrivers.BUILT_IN_SOURCE_CONTROL_DRIVERS.map( + (driver) => driver.kind, + )) { + const provider = yield* registry.get(kind); + if (provider.refineRepositoryIdentity === undefined) continue; + const refined = yield* provider.refineRepositoryIdentity({ + identity, + resolveContext: (input) => + registry.resolveHandle(input).pipe(Effect.map((handle) => handle.context)), + }); + if (refined !== identity) return refined; + } + return identity; }), }); }), diff --git a/packages/source-control-azure-devops/src/server/AzureDevOpsPullRequestProvider.ts b/packages/source-control-azure-devops/src/server/AzureDevOpsPullRequestProvider.ts index afd0456369473ec9bdbd53f61673d4f7c672c3b6..b30356bba7b5bdb4ae79c77d8b91d2de939ca04d 100644 --- a/packages/source-control-azure-devops/src/server/AzureDevOpsPullRequestProvider.ts +++ b/packages/source-control-azure-devops/src/server/AzureDevOpsPullRequestProvider.ts @@ -298,6 +298,8 @@ export const make = Effect.gen(function* () { const provider: PullRequestProviderApi = { kind: "azure-devops", capabilities: CAPABILITIES, + // The bare repository name repeats across an organization; the project path does not. + repositoryKey: ({ canonicalKey }) => canonicalKey, getViewer: (input) => cli.getViewer({ cwd: input.cwd }).pipe(Effect.mapError(fail("getViewer"))), diff --git a/packages/source-control-core/src/server/PullRequestProvider.ts b/packages/source-control-core/src/server/PullRequestProvider.ts index 2500c8fae62a5f8499ea567868607dcdfad1b742..85f5d1893ffb29fe0c4d8f59ae58f447f91527d1 100644 --- a/packages/source-control-core/src/server/PullRequestProvider.ts +++ b/packages/source-control-core/src/server/PullRequestProvider.ts @@ -352,6 +352,19 @@ export interface PullRequestProviderApi { * project asks for it. */ readonly mergeMessageRewrite?: (message: string) => string; + /** + * The key a checkout's repository is known by on this host, where `owner/name` is not enough. + * Absent means `owner/name` on the host identifies it, and any checkout on the host can serve a + * change request in another repository there. + * + * A host that resolves it (Azure DevOps: `dev.azure.com/org/project/_git/repo`) derives its + * organization from the checkout, so only a checkout whose key matches the reference serves + * it. The key also names the repository in list cursors and on routing. + */ + readonly repositoryKey?: (input: { + /** The checkout's canonical key, as its remote names it. */ + readonly canonicalKey: string; + }) => string; /** The signed-in account, which is what involvement filtering compares against. */ readonly getViewer: (input: { diff --git a/packages/source-control-core/src/server/SourceControlProvider.ts b/packages/source-control-core/src/server/SourceControlProvider.ts index 74b946651faa9d3301af3d71056ef214c1c140f7..44d4cf4e95c55db0996de4fcbbf804df129ce524 100644 --- a/packages/source-control-core/src/server/SourceControlProvider.ts +++ b/packages/source-control-core/src/server/SourceControlProvider.ts @@ -2,6 +2,7 @@ import * as Context from "effect/Context"; import * as Effect from "effect/Effect"; import type * as Option from "effect/Option"; import type { + RepositoryIdentity, ChangeRequest, ChangeRequestState, SourceControlProviderError, @@ -135,6 +136,18 @@ export class SourceControlProvider extends Context.Service< * more than that (Forgejo's HTTP installation mount). Absent means the whole path. */ readonly repositoryNameFromRemoteUrl?: (url: string) => string | null; + /** + * Fills in what a repository identity read from git cannot know, such as the browser URL of + * a host the remote URL does not name. `resolveContext` asks the registry which host (and + * base URL) serves a remote. Only consulted for identities this host may own. + */ + readonly refineRepositoryIdentity?: (input: { + readonly identity: RepositoryIdentity; + readonly resolveContext: (input: { + readonly cwd: string; + readonly context: SourceControlProviderContext; + }) => Effect.Effect; + }) => Effect.Effect; /** Optional capability for issue and change-request subjects. */ readonly resolveLink?: ResolveSourceControlLink; readonly listChangeRequests: (input: { diff --git a/packages/source-control-forgejo/src/server/ForgejoSourceControlProvider.test.ts b/packages/source-control-forgejo/src/server/ForgejoSourceControlProvider.test.ts index bdddd5c94b406dece81ec7dd6dd89e795475b91c..e3dae3a6c529ade590a782cfe8b0f35817379701 100644 --- a/packages/source-control-forgejo/src/server/ForgejoSourceControlProvider.test.ts +++ b/packages/source-control-forgejo/src/server/ForgejoSourceControlProvider.test.ts @@ -245,6 +245,66 @@ it.effect.each([ }, ); +it.effect("gives Forgejo identities the browser URL of the login that serves them", () => + Effect.gen(function* () { + const provider = yield* ForgejoSourceControlProvider.make; + const refine = provider.refineRepositoryIdentity; + assert.isDefined(refine); + const identity = (remoteUrl: string, provider?: string) => ({ + canonicalKey: "forge.test/team/repo", + locator: { source: "git-remote" as const, remoteName: "origin", remoteUrl }, + rootPath: "/repo", + ...(provider === undefined ? {} : { provider }), + }); + const served = (baseUrl: string) => () => + Effect.succeed({ + provider: { kind: "forgejo" as const, name: "Forgejo / Gitea", baseUrl }, + remoteName: "origin", + remoteUrl: "", + }); + + // An installation mount in the HTTP path is the login's, not the repository's. + assert.deepStrictEqual( + yield* refine!({ + identity: identity("http://forge.test:3000/git/team/repo.git"), + resolveContext: served("http://forge.test:3000/git/"), + }), + { + ...identity("http://forge.test:3000/git/team/repo.git"), + provider: "forgejo", + webUrl: "http://forge.test:3000/git/team/repo", + }, + ); + // An SSH alias keeps its whole path. + assert.strictEqual( + (yield* refine!({ + identity: identity("ssh://git@ssh.forge.test/team/repo.git"), + resolveContext: served("http://forge.test:3000/git"), + })).webUrl, + "http://forge.test:3000/git/team/repo", + ); + // Another host's identity, or a remote no Forgejo login serves, is left alone. + const github = identity("https://github.com/team/repo.git", "github"); + assert.strictEqual( + yield* refine!({ identity: github, resolveContext: () => Effect.die("not asked") }), + github, + ); + const unserved = identity("http://forge.test:3000/team/repo.git"); + assert.strictEqual( + yield* refine!({ identity: unserved, resolveContext: () => Effect.succeed(null) }), + unserved, + ); + }).pipe( + Effect.provide( + Layer.mergeAll( + Layer.succeed(FileSystem.FileSystem, FileSystem.makeNoop({})), + TestSourceControlHost.layer(), + Layer.mock(ForgejoCli.ForgejoCli)({}), + ), + ), + ), +); + it.effect("loads Forgejo pull request references from files and commits views", () => Effect.gen(function* () { const provider = yield* ForgejoSourceControlProvider.make; diff --git a/packages/source-control-forgejo/src/server/ForgejoSourceControlProvider.ts b/packages/source-control-forgejo/src/server/ForgejoSourceControlProvider.ts index 86196975f2a38e1ecf481037b4eebf8cc7c40ce7..fbcc5fbee99da11211a38450e21f4bde98db0012 100644 --- a/packages/source-control-forgejo/src/server/ForgejoSourceControlProvider.ts +++ b/packages/source-control-forgejo/src/server/ForgejoSourceControlProvider.ts @@ -166,6 +166,43 @@ export function repositoryNameFromRemoteUrl(url: string): string | null { : path.split("/").slice(-2).join("/"); } +/** + * A Forgejo remote's URL can't say which server it belongs to (an SSH alias, an installation + * mount), so the identity's browser URL comes from the login that serves it. + */ +const refineRepositoryIdentity: NonNullable< + SourceControlProvider.SourceControlProvider["Service"]["refineRepositoryIdentity"] +> = Effect.fn("ForgejoSourceControlProvider.refineRepositoryIdentity")(function* ({ + identity, + resolveContext, +}) { + const remote = ForgejoCli.parseForgejoRemote(identity.locator.remoteUrl); + if ( + !remote || + !identity.rootPath || + (identity.provider !== undefined && + identity.provider !== "unknown" && + identity.provider !== "forgejo") + ) + return identity; + const context = yield* resolveContext({ + cwd: identity.rootPath, + context: { + provider: { kind: "unknown", name: "Unknown", baseUrl: "" }, + remoteName: identity.locator.remoteName, + remoteUrl: identity.locator.remoteUrl, + }, + }); + if (context?.provider.kind !== "forgejo") return identity; + const baseUrl = context.provider.baseUrl.replace(/\/+$/, ""); + const basePath = new URL(baseUrl).pathname.replace(/^\/+|\/+$/g, ""); + const path = + !remote.ssh && basePath && remote.path.startsWith(`${basePath}/`) + ? remote.path.slice(basePath.length + 1) + : remote.path; + return { ...identity, provider: "forgejo", webUrl: `${baseUrl}/${path}` }; +}); + const RepositorySchema = Schema.Struct({ full_name: Schema.String, clone_url: Schema.String, @@ -237,6 +274,7 @@ export const make = Effect.gen(function* () { return SourceControlProvider.SourceControlProvider.of({ kind: "forgejo", repositoryNameFromRemoteUrl, + refineRepositoryIdentity, listChangeRequests: (input) => Effect.gen(function* () { const repo = yield* cli.resolveRepository(input); -- 2.54.0