From e4785c82a667af46f22a1c70dc5cfe793f0a7591 Mon Sep 17 00:00:00 2001 From: Alexander Kammerer Date: Mon, 17 Aug 2026 13:08:58 +0200 Subject: [PATCH] fix(server): read Azure DevOps pull requests from the app The pull requests page could not list an Azure DevOps repository, and no Azure pull request could show its conversation. The service names a repository the way its remote does, which below an Azure host is the whole path, `{organization}/{project}/_git/{repository}`. That went straight to `az repos pr list --repository`, which takes a name or an id and interpolates whatever it is given into the REST route it builds, so Azure answered 404 and the page reported the repository as unreadable. The name is now taken from the path and unescaped, because a project or repository named with a space reaches this as `%20`. Reading the conversation went through `az rest` without naming the Azure DevOps resource. Azure does not refuse the wrong token: it answers the sign-in page, as HTML, with a status the CLI reports as success, so the response failed to decode and no comment was ever shown. The call now names the resource. Co-Authored-By: Claude Opus 5 (1M context) --- .../AzureDevOpsPullRequestCli.test.ts | 50 +++++++++++++++++++ .../pullRequest/AzureDevOpsPullRequestCli.ts | 35 ++++++++++++- 2 files changed, 84 insertions(+), 1 deletion(-) 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}`, ],