diff --git a/apps/server/src/pullRequest/AzureDevOpsPullRequestCli.test.ts b/apps/server/src/pullRequest/AzureDevOpsPullRequestCli.test.ts index b52b6d497d69..6c3f2a174b98 100644 --- a/apps/server/src/pullRequest/AzureDevOpsPullRequestCli.test.ts +++ b/apps/server/src/pullRequest/AzureDevOpsPullRequestCli.test.ts @@ -97,6 +97,49 @@ layer("AzureDevOpsPullRequestCli.layer", (it) => { }), ); + it.effect("names the repository the way az does, not the way the remote does", () => + Effect.gen(function* () { + mockedExecute.mockReturnValueOnce(Effect.succeed(output(pullRequests(1, 1)))); + const cli = yield* AzureDevOpsPullRequestCli.AzureDevOpsPullRequestCli; + + yield* cli.listPullRequests({ + cwd: "/w", + // What the service calls a repository below an Azure host: the whole remote path. + repository: "acme/platform/_git/web", + state: "open", + involvement: "all", + viewer: "bilal@acme.dev", + limit: 10, + }); + + // az puts `--repository` straight into the REST route it builds, so the path would address + // a route that does not exist and Azure would answer 404 rather than an empty listing. + const args = argsOfCall(0); + assert.strictEqual(args[args.indexOf("--repository") + 1], "web"); + expect(args).not.toContain("acme/platform/_git/web"); + }), + ); + + it.effect("unescapes the repository name, which the remote path carries escaped", () => + Effect.gen(function* () { + mockedExecute.mockReturnValueOnce(Effect.succeed(output(pullRequests(1, 1)))); + const cli = yield* AzureDevOpsPullRequestCli.AzureDevOpsPullRequestCli; + + yield* cli.listPullRequests({ + cwd: "/w", + // A project and a repository both named with a space, as the remote spells them. + repository: "acme/shared%20platform/_git/web%20client", + state: "open", + involvement: "all", + viewer: "bilal@acme.dev", + limit: 10, + }); + + const args = argsOfCall(0); + assert.strictEqual(args[args.indexOf("--repository") + 1], "web client"); + }), + ); + it.effect("reads the page unnarrowed when asked to search, having nothing to search with", () => Effect.gen(function* () { mockedExecute.mockReturnValueOnce(Effect.succeed(output(pullRequests(3, 1)))); @@ -401,6 +444,13 @@ layer("AzureDevOpsPullRequestCli.layer", (it) => { expect(argsOfCall(0)).toContain( "https://dev.azure.com/acme/platform/_apis/git/r/web/pullRequests/42/threads?api-version=7.1", ); + // Azure DevOps is not the resource az asks for a token for by default, and it answers the + // sign-in page rather than refusing the wrong one, so the call has to name it. + const args = argsOfCall(0); + assert.strictEqual( + args[args.indexOf("--resource") + 1], + "499b84ac-1321-427f-aa17-267ca6975798", + ); }), ); diff --git a/apps/server/src/pullRequest/AzureDevOpsPullRequestCli.ts b/apps/server/src/pullRequest/AzureDevOpsPullRequestCli.ts index 43f929163db1..0677f902f49b 100644 --- a/apps/server/src/pullRequest/AzureDevOpsPullRequestCli.ts +++ b/apps/server/src/pullRequest/AzureDevOpsPullRequestCli.ts @@ -112,6 +112,14 @@ export type AzureDevOpsPullRequestCliError = /** The version every REST call below is pinned to, so a new default cannot reshape a response. */ const REST_API_VERSION = "7.1"; +/** + * The application `az rest` has to ask for a token for. Azure DevOps is not the resource az + * defaults to, and it does not refuse the wrong token: it answers the sign-in page, as HTML, with + * a status the CLI reports as success. So a call without this reads as a decode failure on a + * response nobody asked for rather than as the unauthenticated call it is. + */ +const AZURE_DEVOPS_RESOURCE_ID = "499b84ac-1321-427f-aa17-267ca6975798"; + export class AzureDevOpsPullRequestCli extends Context.Service< AzureDevOpsPullRequestCli, { @@ -173,6 +181,28 @@ export class AzureDevOpsPullRequestCli extends Context.Service< } >()("t3/pullRequest/AzureDevOpsPullRequestCli") {} +/** + * The repository name `az` wants. The service names a repository the way its remote does — below + * an Azure host that is the whole path, `{organization}/{project}/_git/{repository}` — but + * `--repository` takes a name or an id, and az interpolates whatever it is given straight into the + * REST route it builds. A path there does not narrow the listing to the wrong repository; it + * addresses a route that does not exist, and Azure answers 404. + * + * The last segment is the name. It is unescaped because the path carries it as the remote spells + * it: a project or a repository named with a space reaches this as `%20`, which Azure would look + * up literally. A name recorded without a path is already a name and survives both steps. + */ +function repositoryNameOf(repository: string): string { + const name = repository.split("/").findLast((segment) => segment.length > 0); + if (name === undefined) return repository.trim(); + try { + return decodeURIComponent(name); + } catch { + // A stray `%` is not an escape, so the name is whatever the remote spelled. + return name; + } +} + function statusArgs(state: PullRequestListState): ReadonlyArray { switch (state) { case "open": @@ -365,7 +395,8 @@ export const make = Effect.gen(function* () { listPullRequests: (input) => listPullRequestPage({ cwd: input.cwd, - repository: input.repository, + // Translated once, here, so every page of the walk asks for the same repository. + repository: repositoryNameOf(input.repository), state: input.state, involvement: input.involvement, viewer: input.viewer, @@ -419,6 +450,8 @@ export const make = Effect.gen(function* () { "rest", "--method", "get", + "--resource", + AZURE_DEVOPS_RESOURCE_ID, "--url", `${input.threadsUrl}?api-version=${REST_API_VERSION}`, ],