From eaf3de4580b04dbda6b97828e57ae970646fe260 Mon Sep 17 00:00:00 2001 From: Julius Marminge Date: Wed, 4 Nov 2026 22:13:36 +0000 Subject: [PATCH 1/7] test(azure-devops): reviewer votes are read back (#31165 repro) These tests reproduce #31165: a reviewer who voted stays in the requested reviewers, no review decision reaches the change request, and a vote's system thread never shows up as a review in PR activity. Co-Authored-By: Claude Opus 5.5 --- .../AzureDevOpsPullRequestProvider.test.ts | 36 +++++++++ .../server/azureDevOpsPullRequestJson.test.ts | 79 +++++++++++++++++++ 2 files changed, 115 insertions(+) diff --git a/packages/source-control-azure-devops/src/server/AzureDevOpsPullRequestProvider.test.ts b/packages/source-control-azure-devops/src/server/AzureDevOpsPullRequestProvider.test.ts index 345d74de84ba1..5eca643149288 100644 --- a/packages/source-control-azure-devops/src/server/AzureDevOpsPullRequestProvider.test.ts +++ b/packages/source-control-azure-devops/src/server/AzureDevOpsPullRequestProvider.test.ts @@ -122,6 +122,42 @@ function patchedPaths(patch: string): ReadonlyArray { ); } +describe("review decision", () => { + it.effect("carries the reviewers' verdict on every change request Azure is asked for", () => + Effect.gen(function* () { + const approved = { ...PULL_REQUEST, reviewDecision: "approved" as const }; + const provider = yield* make.pipe( + Effect.provide( + Layer.mock(AzureDevOpsPullRequestCli.AzureDevOpsPullRequestCli)({ + listPullRequests: () => + Effect.succeed({ items: [approved], truncated: false, cursorAdvance: 1 }), + getPullRequest: () => Effect.succeed(approved), + listIterations: () => Effect.succeed([ITERATION]), + listIterationChanges: () => Effect.succeed({ changes: [], truncated: false }), + }), + ), + ); + const target = { cwd: "/w", repository: "acme/web", host: "dev.azure.com" }; + + const page = yield* provider.listChangeRequests({ + ...target, + state: "open", + involvement: "all", + viewer: "me@example.com", + limit: 10, + }); + const summary = yield* provider.getChangeRequestSummary!({ ...target, number: 7 }); + const detail = yield* provider.getChangeRequest({ ...target, number: 7 }); + + expect([ + page.items[0]?.reviewDecision, + summary.reviewDecision, + detail.reviewDecision, + ]).toEqual(["approved", "approved", "approved"]); + }), + ); +}); + describe("getChangeRequest", () => { it.effect("preserves the newest head when file counting fails", () => Effect.gen(function* () { diff --git a/packages/source-control-azure-devops/src/server/azureDevOpsPullRequestJson.test.ts b/packages/source-control-azure-devops/src/server/azureDevOpsPullRequestJson.test.ts index b9b1e36a94349..1343ee751b05f 100644 --- a/packages/source-control-azure-devops/src/server/azureDevOpsPullRequestJson.test.ts +++ b/packages/source-control-azure-devops/src/server/azureDevOpsPullRequestJson.test.ts @@ -158,6 +158,50 @@ describe("decodePullRequestJson", () => { expect(detail?.location).toBeNull(); }); + it.each([ + ["approved by a required reviewer", [{ vote: 10, isRequired: true }], [], "approved"], + ["rejected", [{ vote: -10 }], [], "changes-requested"], + ["not yet voted on", [{ vote: 0, isRequired: true }], ["me@example.com"], "review-required"], + [ + "approved but waiting on a required reviewer", + [{ vote: 10 }, { uniqueName: "jane@example.com", vote: 0, isRequired: true }], + ["jane@example.com"], + "review-required", + ], + [ + "approved with suggestions, with an optional reviewer still to vote", + [{ vote: 5 }, { uniqueName: "jane@example.com", vote: 0 }], + ["jane@example.com"], + "approved", + ], + [ + "sent back to its author despite an approval", + [{ vote: -5 }, { uniqueName: "jane@example.com", vote: 10, isRequired: true }], + [], + "changes-requested", + ], + ["without reviewers", [], [], null], + ])( + "reads the reviewers' votes on a pull request %s", + (_case, votes, reviewRequestLogins, reviewDecision) => { + const detail = expectSuccess( + decodePullRequestJson( + asJson( + pullRequest({ + reviewers: votes.map((vote) => ({ + displayName: "Reviewer", + uniqueName: "me@example.com", + ...vote, + })), + }), + ), + ), + ); + + expect(detail).toMatchObject({ reviewRequestLogins, reviewDecision }); + }, + ); + it("returns nothing when Azure gave no way to place the pull request at all", () => { const detail = expectSuccess( decodePullRequestJson(asJson(pullRequest({ url: null, repository: null }))), @@ -248,6 +292,41 @@ describe("decodeThreadsJson", () => { expect(comments.map((comment) => comment.id)).toEqual(["4:1", "4:2", "4:3"]); }); + it("reads a reviewer's vote, which Azure records as a system thread, as their review", () => { + const voteThread = (id: number, vote: string, voter: Record) => ({ + id, + properties: { + CodeReviewThreadType: { $type: "System.String", $value: "VoteUpdate" }, + CodeReviewVoteResult: { $type: "System.String", $value: vote }, + }, + comments: [ + { + id: 1, + content: `${voter.displayName} voted ${vote}`, + commentType: "system", + author: voter, + publishedDate: `2026-07-0${id}T00:00:00Z`, + }, + ], + }); + + const comments = expectSuccess( + decodeThreadsJson( + asJson({ + value: [ + voteThread(2, "10", { displayName: "Jane Doe", uniqueName: "jane@acme.dev" }), + voteThread(3, "-10", { displayName: "Bilal", uniqueName: "bilal@acme.dev" }), + ], + }), + ), + ); + + expect(comments).toMatchObject([ + { kind: "review", reviewState: "APPROVED", author: { login: "jane@acme.dev" } }, + { kind: "review", reviewState: "CHANGES_REQUESTED", author: { login: "bilal@acme.dev" } }, + ]); + }); + it("drops deleted threads and threads with nothing to show", () => { const comments = expectSuccess( decodeThreadsJson( From 6c6e705f69d4568692c7c397982c604a9ea4b3f4 Mon Sep 17 00:00:00 2001 From: Julius Marminge Date: Wed, 4 Nov 2026 22:19:13 +0000 Subject: [PATCH 2/7] fix(azure-devops): reviewer votes set review state and clear the review request Azure DevOps reviewer votes were never read, so a reviewer who approved or rejected stayed in the requested reviewers, change requests carried no review decision, and a vote never appeared in PR activity or woke a PR watch. Reviewers now decode their vote and isRequired. Only reviewers who have not voted stay requested, and the votes give a review decision (any rejection or wait-for-author is changes-requested, an unvoted required reviewer is review-required, otherwise any approval is approved) on list rows, the summary and the detail. A vote's system thread (CodeReviewThreadType VoteUpdate) is read as a review entry with GitHub's APPROVED / CHANGES_REQUESTED state; a reset vote and other system comments stay hidden. Fixes pingdotgg/t3code#31165 Co-Authored-By: Claude Opus 5.5 --- .../AzureDevOpsPullRequestProvider.test.ts | 1 + .../server/AzureDevOpsPullRequestProvider.ts | 2 + .../src/server/azureDevOpsPullRequestJson.ts | 65 +++++++++++++++++-- 3 files changed, 61 insertions(+), 7 deletions(-) diff --git a/packages/source-control-azure-devops/src/server/AzureDevOpsPullRequestProvider.test.ts b/packages/source-control-azure-devops/src/server/AzureDevOpsPullRequestProvider.test.ts index 5eca643149288..8e12efca50422 100644 --- a/packages/source-control-azure-devops/src/server/AzureDevOpsPullRequestProvider.test.ts +++ b/packages/source-control-azure-devops/src/server/AzureDevOpsPullRequestProvider.test.ts @@ -35,6 +35,7 @@ const PULL_REQUEST = { body: "", reviewRequestLogins: [], reviewers: [], + reviewDecision: null, location: { project: "acme", repository: "web" }, autoMergeEnabled: false, }; diff --git a/packages/source-control-azure-devops/src/server/AzureDevOpsPullRequestProvider.ts b/packages/source-control-azure-devops/src/server/AzureDevOpsPullRequestProvider.ts index a1d3ddb6b8c96..89e5ceca15f0f 100644 --- a/packages/source-control-azure-devops/src/server/AzureDevOpsPullRequestProvider.ts +++ b/packages/source-control-azure-devops/src/server/AzureDevOpsPullRequestProvider.ts @@ -108,6 +108,7 @@ function toChangeRequest(pullRequest: AzureDevOpsPullRequest): ProviderChangeReq mergedAt: pullRequest.state === "merged" ? pullRequest.closedAt : null, updatedAt: pullRequest.updatedAt, reviewRequestLogins: pullRequest.reviewRequestLogins, + reviewDecision: pullRequest.reviewDecision, // Azure keeps labels on work items rather than on the pull request. labels: [], }; @@ -304,6 +305,7 @@ export const make = Effect.gen(function* () { closedAt: pullRequest.state === "closed" ? pullRequest.closedAt : null, mergedAt: pullRequest.state === "merged" ? pullRequest.closedAt : null, updatedAt: pullRequest.updatedAt, + reviewDecision: pullRequest.reviewDecision, })), ), diff --git a/packages/source-control-azure-devops/src/server/azureDevOpsPullRequestJson.ts b/packages/source-control-azure-devops/src/server/azureDevOpsPullRequestJson.ts index b3632ef1af98f..fd7e8afa352ba 100644 --- a/packages/source-control-azure-devops/src/server/azureDevOpsPullRequestJson.ts +++ b/packages/source-control-azure-devops/src/server/azureDevOpsPullRequestJson.ts @@ -11,6 +11,7 @@ import type { PullRequestComment, PullRequestMergeMethod, PullRequestMergeability, + PullRequestReviewDecision, PullRequestState, } from "@t3tools/contracts"; import { TrimmedNonEmptyString } from "@t3tools/contracts"; @@ -25,6 +26,17 @@ const RawIdentitySchema = Schema.Struct({ imageUrl: Schema.optional(Schema.NullOr(Schema.String)), }); +const RawReviewerSchema = Schema.Struct({ + ...RawIdentitySchema.fields, + /** 10 approved, 5 approved with suggestions, 0 none, -5 waiting for author, -10 rejected. */ + vote: Schema.optional(Schema.NullOr(Schema.Number)), + isRequired: Schema.optional(Schema.NullOr(Schema.Boolean)), +}); + +const RawThreadPropertySchema = Schema.optional( + Schema.NullOr(Schema.Struct({ $value: Schema.optional(Schema.NullOr(Schema.String)) })), +); + const RawPullRequestSchema = Schema.Struct({ pullRequestId: Schema.Int, title: Schema.String, @@ -43,7 +55,7 @@ const RawPullRequestSchema = Schema.Struct({ ), mergeStatus: Schema.optional(Schema.NullOr(Schema.String)), createdBy: Schema.optional(Schema.NullOr(RawIdentitySchema)), - reviewers: Schema.optional(Schema.NullOr(Schema.Array(RawIdentitySchema))), + reviewers: Schema.optional(Schema.NullOr(Schema.Array(RawReviewerSchema))), sourceRefName: TrimmedNonEmptyString, targetRefName: TrimmedNonEmptyString, creationDate: TrimmedNonEmptyString, @@ -74,6 +86,14 @@ const RawPullRequestSchema = Schema.Struct({ const RawThreadSchema = Schema.Struct({ id: Schema.Int, isDeleted: Schema.optional(Schema.NullOr(Schema.Boolean)), + properties: Schema.optional( + Schema.NullOr( + Schema.Struct({ + CodeReviewThreadType: RawThreadPropertySchema, + CodeReviewVoteResult: RawThreadPropertySchema, + }), + ), + ), threadContext: Schema.optional( Schema.NullOr(Schema.Struct({ filePath: Schema.optional(Schema.NullOr(Schema.String)) })), ), @@ -124,6 +144,7 @@ export interface AzureDevOpsPullRequest { readonly body: string; readonly reviewRequestLogins: ReadonlyArray; readonly reviewers: ReadonlyArray; + readonly reviewDecision: PullRequestReviewDecision | null; readonly location: AzureDevOpsRepositoryLocation | null; readonly autoMergeEnabled: boolean; readonly autoMergeMethod?: PullRequestMergeMethod; @@ -187,14 +208,40 @@ function toAutoMergeMethod( } } +function toReviewDecision( + reviewers: ReadonlyArray>, +): PullRequestReviewDecision | null { + if (reviewers.length === 0) return null; + const votes = reviewers.map((reviewer) => reviewer.vote ?? 0); + if (votes.some((vote) => vote < 0)) return "changes-requested"; + if (reviewers.some((reviewer, index) => reviewer.isRequired === true && votes[index] === 0)) { + return "review-required"; + } + return votes.some((vote) => vote > 0) ? "approved" : "review-required"; +} + +/** Azure records a vote as a system thread; GitHub's review states are what consumers read. */ +function toVoteReviewState( + properties: Schema.Schema.Type["properties"], +): "APPROVED" | "CHANGES_REQUESTED" | null { + if (properties?.CodeReviewThreadType?.$value?.trim() !== "VoteUpdate") return null; + const vote = Number(properties.CodeReviewVoteResult?.$value ?? ""); + return vote > 0 ? "APPROVED" : vote < 0 ? "CHANGES_REQUESTED" : null; +} + function toPullRequest( raw: Schema.Schema.Type, ): AzureDevOpsPullRequest | null { const autoMergeMethod = toAutoMergeMethod(raw); - const reviewers = (raw.reviewers ?? []).flatMap((reviewer) => { + const rawReviewers = raw.reviewers ?? []; + const reviewers = rawReviewers.flatMap((reviewer) => { const actor = toActor(reviewer); return actor === null ? [] : [actor]; }); + const reviewRequestLogins = rawReviewers.flatMap((reviewer) => { + const login = (reviewer.vote ?? 0) === 0 ? toActor(reviewer)?.login : undefined; + return login === undefined ? [] : [login]; + }); const closedAt = trimmed(raw.closedDate); const url = trimmed( azureDevOpsPullRequestWebUrl({ @@ -223,8 +270,9 @@ function toPullRequest( updatedAt: closedAt ?? raw.creationDate, closedAt, body: raw.description ?? "", - reviewRequestLogins: reviewers.map((reviewer) => reviewer.login), + reviewRequestLogins, reviewers, + reviewDecision: toReviewDecision(rawReviewers), location: toLocation(raw), autoMergeEnabled: (raw.autoCompleteSetBy ?? null) !== null, ...(autoMergeMethod === undefined ? {} : { autoMergeMethod }), @@ -299,25 +347,28 @@ export function decodeThreadsJson( const thread = decodedThread.value; if (thread.isDeleted === true) continue; const path = toRepositoryPath(thread.threadContext?.filePath); + const voteReviewState = toVoteReviewState(thread.properties); for (const comment of thread.comments ?? []) { const publishedDate = trimmed(comment.publishedDate); + const isSystem = comment.commentType?.trim().toLowerCase() === "system"; if ( comment.isDeleted === true || - comment.commentType?.trim().toLowerCase() === "system" || + (isSystem && voteReviewState === null) || (comment.content ?? "").trim().length === 0 || publishedDate === null ) { continue; } + const reviewState = isSystem ? voteReviewState : null; comments.push({ id: `${thread.id}:${comment.id ?? 0}`, - kind: path === null ? "issue-comment" : "review-comment", + kind: reviewState !== null ? "review" : path === null ? "issue-comment" : "review-comment", author: toActor(comment.author), body: comment.content ?? "", createdAt: publishedDate, url: null, - path, - reviewState: null, + path: reviewState === null ? path : null, + reviewState, }); } } From 1264367cd66f8891271eb1985299b8832dacd440 Mon Sep 17 00:00:00 2001 From: Julius Marminge Date: Wed, 4 Nov 2026 22:40:39 +0000 Subject: [PATCH 3/7] fix(azure-devops): attribute votes to the voter, read resets, and drop voted PRs from Reviewing Azure authors a vote thread's comment as a service account, so every vote collapsed onto one author. Votes now resolve to the reviewer named by the thread's CodeReviewVotedByTfId, falling back to CodeReviewVotedByDisplayName. An explicit vote of 0 reads as DISMISSED so clients clear the earlier verdict, and a numeric $value no longer fails the thread decode. `--reviewer` keeps returning pull requests the viewer has already voted on; the Reviewing list now drops those while still advancing pagination over them. Co-Authored-By: Claude Opus 5.5 --- .../server/AzureDevOpsPullRequestCli.test.ts | 35 +++++ .../src/server/AzureDevOpsPullRequestCli.ts | 44 ++++-- .../AzureDevOpsPullRequestProvider.test.ts | 1 + .../server/AzureDevOpsPullRequestProvider.ts | 1 + .../server/azureDevOpsPullRequestJson.test.ts | 138 +++++++++++++++--- .../src/server/azureDevOpsPullRequestJson.ts | 62 ++++++-- 6 files changed, 243 insertions(+), 38 deletions(-) diff --git a/packages/source-control-azure-devops/src/server/AzureDevOpsPullRequestCli.test.ts b/packages/source-control-azure-devops/src/server/AzureDevOpsPullRequestCli.test.ts index 4b2e46b5a1e93..9677b6c7bb600 100644 --- a/packages/source-control-azure-devops/src/server/AzureDevOpsPullRequestCli.test.ts +++ b/packages/source-control-azure-devops/src/server/AzureDevOpsPullRequestCli.test.ts @@ -257,6 +257,41 @@ layer("AzureDevOpsPullRequestCli.layer", (it) => { }), ); + it.effect("leaves out of Reviewing what the viewer has voted on and still steps over it", () => + Effect.gen(function* () { + const [voted, waiting, groupRequested] = pullRequestRows(3, 1); + const viewer = { displayName: "Bilal Hassan", uniqueName: "Bilal@acme.dev" }; + mockedExecute.mockReturnValueOnce( + Effect.succeed( + output( + JSON.stringify([ + { ...voted, reviewers: [{ ...viewer, vote: 10 }] }, + { ...waiting, reviewers: [{ ...viewer, vote: 0 }] }, + { + ...groupRequested, + reviewers: [{ displayName: "[web]\\Reviewers", uniqueName: "web-reviewers" }], + }, + ]), + ), + ), + ); + const cli = yield* AzureDevOpsPullRequestCli.AzureDevOpsPullRequestCli; + + const batch = yield* cli.listPullRequests({ + cwd: "/w", + repository: "web", + state: "open", + involvement: "reviewing", + viewer: "bilal@acme.dev", + limit: 2, + }); + + expect(batch.items.map((item) => item.number)).toEqual([2, 3]); + assert.isTrue(batch.truncated); + assert.strictEqual(batch.cursorAdvance, 3); + }), + ); + it.effect("reads the signed-in account, which az reports as a bare value", () => Effect.gen(function* () { mockedExecute.mockReturnValueOnce( diff --git a/packages/source-control-azure-devops/src/server/AzureDevOpsPullRequestCli.ts b/packages/source-control-azure-devops/src/server/AzureDevOpsPullRequestCli.ts index 5856d7c0beadf..8d1b9ee9ae943 100644 --- a/packages/source-control-azure-devops/src/server/AzureDevOpsPullRequestCli.ts +++ b/packages/source-control-azure-devops/src/server/AzureDevOpsPullRequestCli.ts @@ -24,6 +24,7 @@ import { type AzureDevOpsItemContent, type AzureDevOpsIteration, type AzureDevOpsPullRequest, + type AzureDevOpsPullRequestBatch, type AzureDevOpsRepositoryLocation, } from "./azureDevOpsPullRequestJson.ts"; import type { ProviderListCursor } from "@t3tools/source-control-core/server/PullRequestProvider"; @@ -161,6 +162,7 @@ export class AzureDevOpsPullRequestCli extends Context.Service< readonly cwd: string; readonly location: AzureDevOpsRepositoryLocation; readonly number: number; + readonly reviewersById: AzureDevOpsPullRequest["reviewersById"]; }) => Effect.Effect, AzureDevOpsPullRequestCliError>; readonly listIterations: (input: { @@ -234,6 +236,26 @@ function involvementArgs(input: { } } +/** `--reviewer` keeps a pull request after the viewer votes on it; only an unvoted one waits on them. */ +function withoutVotedByViewer( + batch: AzureDevOpsPullRequestBatch, + viewer: string, +): AzureDevOpsPullRequestBatch { + const login = viewer.toLowerCase(); + const isViewer = (candidate: string) => candidate.toLowerCase() === login; + const kept = batch.items.flatMap((item, index) => + item.reviewers.some((reviewer) => isViewer(reviewer.login)) && + !item.reviewRequestLogins.some(isViewer) + ? [] + : [{ item, rawIndex: batch.rawIndexes[index] ?? 0 }], + ); + return { + items: kept.map((entry) => entry.item), + rawIndexes: kept.map((entry) => entry.rawIndex), + rawCount: batch.rawCount, + }; +} + function actionArgs( action: PullRequestAction, mergeMethod: PullRequestMergeMethod | undefined, @@ -415,29 +437,33 @@ export const make = Effect.gen(function* () { }), ); } + const batch = + input.involvement === "reviewing" + ? withoutVotedByViewer(decoded.success, input.viewer) + : decoded.success; - const lastItemIndex = decoded.success.rawIndexes[remaining - 1]; + const lastItemIndex = batch.rawIndexes[remaining - 1]; if (lastItemIndex !== undefined) { const consumed = lastItemIndex + 1; return Effect.succeed({ - items: [...input.items, ...decoded.success.items.slice(0, remaining)], - truncated: consumed < decoded.success.rawCount || decoded.success.rawCount === top, + items: [...input.items, ...batch.items.slice(0, remaining)], + truncated: consumed < batch.rawCount || batch.rawCount === top, cursorAdvance: input.cursorAdvance + consumed, }); } - const items = [...input.items, ...decoded.success.items]; - if (decoded.success.rawCount < top) { + const items = [...input.items, ...batch.items]; + if (batch.rawCount < top) { return Effect.succeed({ items, truncated: false, - cursorAdvance: input.cursorAdvance + decoded.success.rawCount, + cursorAdvance: input.cursorAdvance + batch.rawCount, }); } return listPullRequestPage({ ...input, - skip: input.skip + decoded.success.rawCount, - cursorAdvance: input.cursorAdvance + decoded.success.rawCount, + skip: input.skip + batch.rawCount, + cursorAdvance: input.cursorAdvance + batch.rawCount, items, }); }), @@ -552,7 +578,7 @@ export const make = Effect.gen(function* () { resource: "pullRequestThreads", routeParameters: pullRequestRoute(input), maxOutputBytes: REVIEW_HISTORY_MAX_OUTPUT_BYTES, - decode: decodeThreadsJson, + decode: (raw) => decodeThreadsJson(raw, input.reviewersById), }); }), diff --git a/packages/source-control-azure-devops/src/server/AzureDevOpsPullRequestProvider.test.ts b/packages/source-control-azure-devops/src/server/AzureDevOpsPullRequestProvider.test.ts index 8e12efca50422..81613637d21e1 100644 --- a/packages/source-control-azure-devops/src/server/AzureDevOpsPullRequestProvider.test.ts +++ b/packages/source-control-azure-devops/src/server/AzureDevOpsPullRequestProvider.test.ts @@ -35,6 +35,7 @@ const PULL_REQUEST = { body: "", reviewRequestLogins: [], reviewers: [], + reviewersById: new Map(), reviewDecision: null, location: { project: "acme", repository: "web" }, autoMergeEnabled: false, diff --git a/packages/source-control-azure-devops/src/server/AzureDevOpsPullRequestProvider.ts b/packages/source-control-azure-devops/src/server/AzureDevOpsPullRequestProvider.ts index 89e5ceca15f0f..723dcb74991b5 100644 --- a/packages/source-control-azure-devops/src/server/AzureDevOpsPullRequestProvider.ts +++ b/packages/source-control-azure-devops/src/server/AzureDevOpsPullRequestProvider.ts @@ -365,6 +365,7 @@ export const make = Effect.gen(function* () { cwd: input.cwd, location: pullRequest.location, number: input.number, + reviewersById: pullRequest.reviewersById, }) .pipe( Effect.map((comments) => ({ comments, truncated: false })), diff --git a/packages/source-control-azure-devops/src/server/azureDevOpsPullRequestJson.test.ts b/packages/source-control-azure-devops/src/server/azureDevOpsPullRequestJson.test.ts index 1343ee751b05f..1d48db49e6252 100644 --- a/packages/source-control-azure-devops/src/server/azureDevOpsPullRequestJson.test.ts +++ b/packages/source-control-azure-devops/src/server/azureDevOpsPullRequestJson.test.ts @@ -39,6 +39,59 @@ function expectSuccess(result: Result.Result): A { const asJson = (value: unknown) => JSON.stringify(value); +const JANE_ID = "d6245f20-2af8-44f4-9451-8107cb2767db"; +const BILAL_ID = "8c8c7d32-6b1b-47f4-b2e9-30b477b5ab3d"; + +function reviewersById() { + const detail = expectSuccess( + decodePullRequestJson( + asJson( + pullRequest({ + reviewers: [ + { id: JANE_ID, displayName: "Jane Doe", uniqueName: "jane@acme.dev", vote: 10 }, + { id: BILAL_ID, displayName: "Bilal Hassan", uniqueName: "bilal@acme.dev", vote: -10 }, + ], + }), + ), + ), + ); + if (detail === null) throw new Error("expected a pull request"); + return detail.reviewersById; +} + +/** Azure's documented shape: a service account authors the comment; properties name the voter. */ +function voteThread(input: { + readonly id: number; + readonly vote: string | number | null; + readonly voterId: string; + readonly voterName: string; +}) { + return { + id: input.id, + properties: { + CodeReviewThreadType: { $type: "System.String", $value: "VoteUpdate" }, + ...(input.vote === null + ? {} + : { CodeReviewVoteResult: { $type: "System.String", $value: input.vote } }), + CodeReviewVotedByTfId: { $type: "System.String", $value: input.voterId }, + CodeReviewVotedByDisplayName: { $type: "System.String", $value: input.voterName }, + }, + comments: [ + { + id: 1, + content: `${input.voterName} voted ${input.vote}`, + commentType: "system", + author: { + id: "41113706-0000-0000-0000-000000000000", + displayName: "[DefaultCollection]\\Project Collection Service Accounts", + isContainer: true, + }, + publishedDate: `2026-07-0${input.id}T00:00:00Z`, + }, + ], + }; +} + describe("decodePullRequestListJson", () => { it("reads a pull request as a change request", () => { const batch = expectSuccess(decodePullRequestListJson(asJson([pullRequest()]))); @@ -293,31 +346,21 @@ describe("decodeThreadsJson", () => { }); it("reads a reviewer's vote, which Azure records as a system thread, as their review", () => { - const voteThread = (id: number, vote: string, voter: Record) => ({ - id, - properties: { - CodeReviewThreadType: { $type: "System.String", $value: "VoteUpdate" }, - CodeReviewVoteResult: { $type: "System.String", $value: vote }, - }, - comments: [ - { - id: 1, - content: `${voter.displayName} voted ${vote}`, - commentType: "system", - author: voter, - publishedDate: `2026-07-0${id}T00:00:00Z`, - }, - ], - }); - const comments = expectSuccess( decodeThreadsJson( asJson({ value: [ - voteThread(2, "10", { displayName: "Jane Doe", uniqueName: "jane@acme.dev" }), - voteThread(3, "-10", { displayName: "Bilal", uniqueName: "bilal@acme.dev" }), + voteThread({ id: 2, vote: "10", voterId: JANE_ID, voterName: "Jane Doe" }), + // Azure writes the same identity id without dashes and in either case. + voteThread({ + id: 3, + vote: "-10", + voterId: BILAL_ID.replaceAll("-", "").toUpperCase(), + voterName: "Bilal Hassan", + }), ], }), + reviewersById(), ), ); @@ -327,6 +370,63 @@ describe("decodeThreadsJson", () => { ]); }); + it("names a voter who is not among the reviewers by their display name", () => { + const comments = expectSuccess( + decodeThreadsJson( + asJson({ + value: [ + voteThread({ + id: 2, + vote: "10", + voterId: "0f0f0f0f-0000-0000-0000-000000000000", + voterName: "Normal Paulk", + }), + ], + }), + reviewersById(), + ), + ); + + expect(comments).toMatchObject([ + { reviewState: "APPROVED", author: { login: "Normal Paulk", name: "Normal Paulk" } }, + ]); + }); + + it("reads a reset vote as withdrawing the reviewer's earlier verdict", () => { + const comments = expectSuccess( + decodeThreadsJson( + asJson({ + value: [ + voteThread({ id: 2, vote: "10", voterId: JANE_ID, voterName: "Jane Doe" }), + voteThread({ id: 3, vote: "0", voterId: JANE_ID, voterName: "Jane Doe" }), + ], + }), + reviewersById(), + ), + ); + + expect(comments.map((comment) => [comment.author?.login, comment.reviewState])).toEqual([ + ["jane@acme.dev", "APPROVED"], + ["jane@acme.dev", "DISMISSED"], + ]); + }); + + it("reads a vote Azure reports as a number and skips one it does not report", () => { + const comments = expectSuccess( + decodeThreadsJson( + asJson({ + value: [ + voteThread({ id: 2, vote: -10, voterId: JANE_ID, voterName: "Jane Doe" }), + voteThread({ id: 3, vote: null, voterId: JANE_ID, voterName: "Jane Doe" }), + ], + }), + reviewersById(), + ), + ); + + expect(comments.map((comment) => comment.reviewState)).toEqual(["CHANGES_REQUESTED"]); + }); + it("drops deleted threads and threads with nothing to show", () => { const comments = expectSuccess( decodeThreadsJson( diff --git a/packages/source-control-azure-devops/src/server/azureDevOpsPullRequestJson.ts b/packages/source-control-azure-devops/src/server/azureDevOpsPullRequestJson.ts index fd7e8afa352ba..a51ce4047cddd 100644 --- a/packages/source-control-azure-devops/src/server/azureDevOpsPullRequestJson.ts +++ b/packages/source-control-azure-devops/src/server/azureDevOpsPullRequestJson.ts @@ -28,13 +28,18 @@ const RawIdentitySchema = Schema.Struct({ const RawReviewerSchema = Schema.Struct({ ...RawIdentitySchema.fields, + id: Schema.optional(Schema.NullOr(Schema.String)), /** 10 approved, 5 approved with suggestions, 0 none, -5 waiting for author, -10 rejected. */ vote: Schema.optional(Schema.NullOr(Schema.Number)), isRequired: Schema.optional(Schema.NullOr(Schema.Boolean)), }); const RawThreadPropertySchema = Schema.optional( - Schema.NullOr(Schema.Struct({ $value: Schema.optional(Schema.NullOr(Schema.String)) })), + Schema.NullOr( + Schema.Struct({ + $value: Schema.optional(Schema.NullOr(Schema.Union([Schema.String, Schema.Number]))), + }), + ), ); const RawPullRequestSchema = Schema.Struct({ @@ -91,6 +96,8 @@ const RawThreadSchema = Schema.Struct({ Schema.Struct({ CodeReviewThreadType: RawThreadPropertySchema, CodeReviewVoteResult: RawThreadPropertySchema, + CodeReviewVotedByTfId: RawThreadPropertySchema, + CodeReviewVotedByDisplayName: RawThreadPropertySchema, }), ), ), @@ -144,6 +151,8 @@ export interface AzureDevOpsPullRequest { readonly body: string; readonly reviewRequestLogins: ReadonlyArray; readonly reviewers: ReadonlyArray; + /** Keyed by `identityKey`; vote threads name the voter only by identity id. */ + readonly reviewersById: ReadonlyMap; readonly reviewDecision: PullRequestReviewDecision | null; readonly location: AzureDevOpsRepositoryLocation | null; readonly autoMergeEnabled: boolean; @@ -158,6 +167,11 @@ function toActor(raw: Schema.Schema.Type | null | unde : { login, name: trimmed(raw?.displayName), avatarUrl: trimmed(raw?.imageUrl) }; } +/** Azure writes identity ids both with and without dashes. */ +function identityKey(id: string | null | undefined): string | null { + return trimmed(id)?.replaceAll("-", "").toLowerCase() ?? null; +} + function toState(raw: Schema.Schema.Type): PullRequestState { switch (raw.status?.trim().toLowerCase()) { case "completed": @@ -220,13 +234,33 @@ function toReviewDecision( return votes.some((vote) => vote > 0) ? "approved" : "review-required"; } +type RawThreadProperties = NonNullable["properties"]>; + +function propertyText(property: RawThreadProperties[keyof RawThreadProperties]): string | null { + const value = property?.$value; + return trimmed(value === null || value === undefined ? value : String(value)); +} + /** Azure records a vote as a system thread; GitHub's review states are what consumers read. */ function toVoteReviewState( - properties: Schema.Schema.Type["properties"], -): "APPROVED" | "CHANGES_REQUESTED" | null { - if (properties?.CodeReviewThreadType?.$value?.trim() !== "VoteUpdate") return null; - const vote = Number(properties.CodeReviewVoteResult?.$value ?? ""); - return vote > 0 ? "APPROVED" : vote < 0 ? "CHANGES_REQUESTED" : null; + properties: RawThreadProperties | null | undefined, +): "APPROVED" | "CHANGES_REQUESTED" | "DISMISSED" | null { + if (propertyText(properties?.CodeReviewThreadType) !== "VoteUpdate") return null; + const vote = Number(propertyText(properties?.CodeReviewVoteResult) ?? NaN); + if (!Number.isFinite(vote)) return null; + return vote > 0 ? "APPROVED" : vote < 0 ? "CHANGES_REQUESTED" : "DISMISSED"; +} + +/** A vote's comment is authored by a service account; the voter is in the thread properties. */ +function toVoter( + properties: RawThreadProperties | null | undefined, + reviewersById: ReadonlyMap, +): PullRequestActor | null { + const key = identityKey(propertyText(properties?.CodeReviewVotedByTfId)); + const reviewer = key === null ? undefined : reviewersById.get(key); + if (reviewer !== undefined) return reviewer; + const name = propertyText(properties?.CodeReviewVotedByDisplayName); + return name === null ? null : { login: name, name, avatarUrl: null }; } function toPullRequest( @@ -234,10 +268,15 @@ function toPullRequest( ): AzureDevOpsPullRequest | null { const autoMergeMethod = toAutoMergeMethod(raw); const rawReviewers = raw.reviewers ?? []; - const reviewers = rawReviewers.flatMap((reviewer) => { + const reviewers: PullRequestActor[] = []; + const reviewersById = new Map(); + for (const reviewer of rawReviewers) { const actor = toActor(reviewer); - return actor === null ? [] : [actor]; - }); + if (actor === null) continue; + reviewers.push(actor); + const key = identityKey(reviewer.id); + if (key !== null) reviewersById.set(key, actor); + } const reviewRequestLogins = rawReviewers.flatMap((reviewer) => { const login = (reviewer.vote ?? 0) === 0 ? toActor(reviewer)?.login : undefined; return login === undefined ? [] : [login]; @@ -272,6 +311,7 @@ function toPullRequest( body: raw.description ?? "", reviewRequestLogins, reviewers, + reviewersById, reviewDecision: toReviewDecision(rawReviewers), location: toLocation(raw), autoMergeEnabled: (raw.autoCompleteSetBy ?? null) !== null, @@ -335,6 +375,7 @@ export function decodeViewerJson(raw: string): Result.Result = new Map(), ): Result.Result, DecodeFailure> { const decoded = decodeThreadPage(raw); if (!Result.isSuccess(decoded)) { @@ -348,6 +389,7 @@ export function decodeThreadsJson( if (thread.isDeleted === true) continue; const path = toRepositoryPath(thread.threadContext?.filePath); const voteReviewState = toVoteReviewState(thread.properties); + const voter = voteReviewState === null ? null : toVoter(thread.properties, reviewersById); for (const comment of thread.comments ?? []) { const publishedDate = trimmed(comment.publishedDate); const isSystem = comment.commentType?.trim().toLowerCase() === "system"; @@ -363,7 +405,7 @@ export function decodeThreadsJson( comments.push({ id: `${thread.id}:${comment.id ?? 0}`, kind: reviewState !== null ? "review" : path === null ? "issue-comment" : "review-comment", - author: toActor(comment.author), + author: reviewState === null ? toActor(comment.author) : voter, body: comment.content ?? "", createdAt: publishedDate, url: null, From 223ea2c2fe2c1b1904be0d53f4b9aaa972007128 Mon Sep 17 00:00:00 2001 From: Julius Marminge Date: Wed, 4 Nov 2026 22:54:36 +0000 Subject: [PATCH 4/7] fix(azure-devops): bound Reviewing reads, ignore the author's own vote, keep former voters apart Reviewing now drops rows the viewer voted on, so one listing reads at most three az pages and returns a resumable cursor instead of walking every voted pull request. An author's own vote no longer counts towards the review decision, and a voter no longer among the reviewers keeps their Azure id as login, since display names are not unique. Co-Authored-By: Claude Opus 5.5 --- .../server/AzureDevOpsPullRequestCli.test.ts | 32 +++++++++++++++++++ .../src/server/AzureDevOpsPullRequestCli.ts | 9 ++++-- .../server/azureDevOpsPullRequestJson.test.ts | 20 ++++++++++-- .../src/server/azureDevOpsPullRequestJson.ts | 11 +++++-- 4 files changed, 64 insertions(+), 8 deletions(-) diff --git a/packages/source-control-azure-devops/src/server/AzureDevOpsPullRequestCli.test.ts b/packages/source-control-azure-devops/src/server/AzureDevOpsPullRequestCli.test.ts index 9677b6c7bb600..03b3f418244f6 100644 --- a/packages/source-control-azure-devops/src/server/AzureDevOpsPullRequestCli.test.ts +++ b/packages/source-control-azure-devops/src/server/AzureDevOpsPullRequestCli.test.ts @@ -292,6 +292,38 @@ layer("AzureDevOpsPullRequestCli.layer", (it) => { }), ); + it.effect("stops after a few reads of Reviewing pages the viewer has all voted on", () => + Effect.gen(function* () { + const viewer = { displayName: "Bilal Hassan", uniqueName: "bilal@acme.dev", vote: 10 }; + for (let page = 0; page < 5; page += 1) { + mockedExecute.mockReturnValueOnce( + Effect.succeed( + output( + JSON.stringify( + pullRequestRows(3, page * 3 + 1).map((row) => ({ ...row, reviewers: [viewer] })), + ), + ), + ), + ); + } + const cli = yield* AzureDevOpsPullRequestCli.AzureDevOpsPullRequestCli; + + const batch = yield* cli.listPullRequests({ + cwd: "/w", + repository: "web", + state: "all", + involvement: "reviewing", + viewer: "bilal@acme.dev", + limit: 2, + }); + + assert.strictEqual(mockedExecute.mock.calls.length, 3); + expect(batch.items).toEqual([]); + assert.isTrue(batch.truncated); + assert.strictEqual(batch.cursorAdvance, 9); + }), + ); + it.effect("reads the signed-in account, which az reports as a bare value", () => Effect.gen(function* () { mockedExecute.mockReturnValueOnce( diff --git a/packages/source-control-azure-devops/src/server/AzureDevOpsPullRequestCli.ts b/packages/source-control-azure-devops/src/server/AzureDevOpsPullRequestCli.ts index 8d1b9ee9ae943..f61f0a401cb0f 100644 --- a/packages/source-control-azure-devops/src/server/AzureDevOpsPullRequestCli.ts +++ b/packages/source-control-azure-devops/src/server/AzureDevOpsPullRequestCli.ts @@ -105,6 +105,8 @@ export type AzureDevOpsPullRequestCliError = const REST_API_VERSION = "7.1"; const PULL_REQUEST_LIST_MAX_OUTPUT_BYTES = 16 * 1024 * 1024; +/** Reviewing drops rows the viewer voted on; bound the reads one listing may spend skipping them. */ +const PULL_REQUEST_LIST_MAX_READS = 3; /** A 2,000-file page exceeds the default 1 MiB output budget. */ const CHANGE_ENTRIES_MAX_OUTPUT_BYTES = 8 * 1024 * 1024; /** JSON escaping and base64 can exceed the decoded file's size. */ @@ -385,6 +387,7 @@ export const make = Effect.gen(function* () { readonly limit: number; readonly skip: number; readonly cursorAdvance: number; + readonly reads: number; readonly items: ReadonlyArray; }): Effect.Effect< { @@ -453,10 +456,10 @@ export const make = Effect.gen(function* () { } const items = [...input.items, ...batch.items]; - if (batch.rawCount < top) { + if (batch.rawCount < top || input.reads + 1 >= PULL_REQUEST_LIST_MAX_READS) { return Effect.succeed({ items, - truncated: false, + truncated: batch.rawCount === top, cursorAdvance: input.cursorAdvance + batch.rawCount, }); } @@ -464,6 +467,7 @@ export const make = Effect.gen(function* () { ...input, skip: input.skip + batch.rawCount, cursorAdvance: input.cursorAdvance + batch.rawCount, + reads: input.reads + 1, items, }); }), @@ -535,6 +539,7 @@ export const make = Effect.gen(function* () { limit: input.limit, skip: input.cursor?.delivered ?? 0, cursorAdvance: 0, + reads: 0, items: [], }); }), diff --git a/packages/source-control-azure-devops/src/server/azureDevOpsPullRequestJson.test.ts b/packages/source-control-azure-devops/src/server/azureDevOpsPullRequestJson.test.ts index 1d48db49e6252..39d392302ee50 100644 --- a/packages/source-control-azure-devops/src/server/azureDevOpsPullRequestJson.test.ts +++ b/packages/source-control-azure-devops/src/server/azureDevOpsPullRequestJson.test.ts @@ -234,6 +234,7 @@ describe("decodePullRequestJson", () => { "changes-requested", ], ["without reviewers", [], [], null], + ["approved only by its author", [{ uniqueName: "bilal@acme.dev", vote: 10 }], [], null], ])( "reads the reviewers' votes on a pull request %s", (_case, votes, reviewRequestLogins, reviewDecision) => { @@ -370,7 +371,7 @@ describe("decodeThreadsJson", () => { ]); }); - it("names a voter who is not among the reviewers by their display name", () => { + it("keeps voters who are no longer reviewers apart even when they share a display name", () => { const comments = expectSuccess( decodeThreadsJson( asJson({ @@ -379,7 +380,13 @@ describe("decodeThreadsJson", () => { id: 2, vote: "10", voterId: "0f0f0f0f-0000-0000-0000-000000000000", - voterName: "Normal Paulk", + voterName: "Sam Lee", + }), + voteThread({ + id: 3, + vote: "-10", + voterId: "0e0e0e0e-0000-0000-0000-000000000000", + voterName: "Sam Lee", }), ], }), @@ -388,7 +395,14 @@ describe("decodeThreadsJson", () => { ); expect(comments).toMatchObject([ - { reviewState: "APPROVED", author: { login: "Normal Paulk", name: "Normal Paulk" } }, + { + reviewState: "APPROVED", + author: { login: "0f0f0f0f000000000000000000000000", name: "Sam Lee" }, + }, + { + reviewState: "CHANGES_REQUESTED", + author: { login: "0e0e0e0e000000000000000000000000", name: "Sam Lee" }, + }, ]); }); diff --git a/packages/source-control-azure-devops/src/server/azureDevOpsPullRequestJson.ts b/packages/source-control-azure-devops/src/server/azureDevOpsPullRequestJson.ts index a51ce4047cddd..cf700e1148b6d 100644 --- a/packages/source-control-azure-devops/src/server/azureDevOpsPullRequestJson.ts +++ b/packages/source-control-azure-devops/src/server/azureDevOpsPullRequestJson.ts @@ -223,8 +223,11 @@ function toAutoMergeMethod( } function toReviewDecision( - reviewers: ReadonlyArray>, + allReviewers: ReadonlyArray>, + author: PullRequestActor | null, ): PullRequestReviewDecision | null { + // Azure adds an author who votes as a reviewer, but their vote does not review the change. + const reviewers = allReviewers.filter((reviewer) => toActor(reviewer)?.login !== author?.login); if (reviewers.length === 0) return null; const votes = reviewers.map((reviewer) => reviewer.vote ?? 0); if (votes.some((vote) => vote < 0)) return "changes-requested"; @@ -259,8 +262,10 @@ function toVoter( const key = identityKey(propertyText(properties?.CodeReviewVotedByTfId)); const reviewer = key === null ? undefined : reviewersById.get(key); if (reviewer !== undefined) return reviewer; + // Display names are not unique, so a voter no longer among the reviewers keeps their Azure id. const name = propertyText(properties?.CodeReviewVotedByDisplayName); - return name === null ? null : { login: name, name, avatarUrl: null }; + const login = key ?? name; + return login === null ? null : { login, name, avatarUrl: null }; } function toPullRequest( @@ -312,7 +317,7 @@ function toPullRequest( reviewRequestLogins, reviewers, reviewersById, - reviewDecision: toReviewDecision(rawReviewers), + reviewDecision: toReviewDecision(rawReviewers, toActor(raw.createdBy)), location: toLocation(raw), autoMergeEnabled: (raw.autoCompleteSetBy ?? null) !== null, ...(autoMergeMethod === undefined ? {} : { autoMergeMethod }), From 198654c27ffb0cd57aea33116ae5e45d655569e0 Mon Sep 17 00:00:00 2001 From: Julius Marminge Date: Wed, 4 Nov 2026 23:10:01 +0000 Subject: [PATCH 5/7] fix(azure-devops): Reviewing never ends a page empty while voted rows remain The read bound now waits for one row to show: an empty truncated page gave the list service no cursor, so pull requests behind a long run of voted ones could not be reached. A dash-only voter id no longer becomes an empty login. Co-Authored-By: Claude Opus 5.5 --- .../server/AzureDevOpsPullRequestCli.test.ts | 71 +++++++++++++++---- .../src/server/AzureDevOpsPullRequestCli.ts | 10 ++- .../src/server/azureDevOpsPullRequestJson.ts | 2 +- 3 files changed, 68 insertions(+), 15 deletions(-) diff --git a/packages/source-control-azure-devops/src/server/AzureDevOpsPullRequestCli.test.ts b/packages/source-control-azure-devops/src/server/AzureDevOpsPullRequestCli.test.ts index 03b3f418244f6..aa4434a8dc874 100644 --- a/packages/source-control-azure-devops/src/server/AzureDevOpsPullRequestCli.test.ts +++ b/packages/source-control-azure-devops/src/server/AzureDevOpsPullRequestCli.test.ts @@ -292,20 +292,28 @@ layer("AzureDevOpsPullRequestCli.layer", (it) => { }), ); - it.effect("stops after a few reads of Reviewing pages the viewer has all voted on", () => + it.effect("stops reading Reviewing pages of voted pull requests once it has one to show", () => Effect.gen(function* () { - const viewer = { displayName: "Bilal Hassan", uniqueName: "bilal@acme.dev", vote: 10 }; - for (let page = 0; page < 5; page += 1) { - mockedExecute.mockReturnValueOnce( - Effect.succeed( - output( - JSON.stringify( - pullRequestRows(3, page * 3 + 1).map((row) => ({ ...row, reviewers: [viewer] })), - ), + const viewer = { displayName: "Bilal Hassan", uniqueName: "bilal@acme.dev" }; + const page = (count: number, firstNumber: number, pendingNumbers: ReadonlyArray) => + Effect.succeed( + output( + JSON.stringify( + pullRequestRows(count, firstNumber).map((row) => ({ + ...row, + reviewers: [ + { ...viewer, vote: pendingNumbers.includes(Number(row.pullRequestId)) ? 0 : 10 }, + ], + })), ), ), ); - } + // Once one row is kept, `--top` asks for one fewer. + mockedExecute + .mockReturnValueOnce(page(3, 1, [1])) + .mockReturnValueOnce(page(2, 4, [])) + .mockReturnValueOnce(page(2, 6, [])) + .mockReturnValueOnce(page(2, 8, [8])); const cli = yield* AzureDevOpsPullRequestCli.AzureDevOpsPullRequestCli; const batch = yield* cli.listPullRequests({ @@ -318,9 +326,48 @@ layer("AzureDevOpsPullRequestCli.layer", (it) => { }); assert.strictEqual(mockedExecute.mock.calls.length, 3); - expect(batch.items).toEqual([]); + expect(batch.items.map((item) => item.number)).toEqual([1]); + assert.isTrue(batch.truncated); + assert.strictEqual(batch.cursorAdvance, 7); + }), + ); + + it.effect("keeps reading past voted Reviewing pages until it finds one to show", () => + Effect.gen(function* () { + const viewer = { displayName: "Bilal Hassan", uniqueName: "bilal@acme.dev" }; + const page = (count: number, firstNumber: number, pendingNumbers: ReadonlyArray) => + Effect.succeed( + output( + JSON.stringify( + pullRequestRows(count, firstNumber).map((row) => ({ + ...row, + reviewers: [ + { ...viewer, vote: pendingNumbers.includes(Number(row.pullRequestId)) ? 0 : 10 }, + ], + })), + ), + ), + ); + mockedExecute + .mockReturnValueOnce(page(3, 1, [])) + .mockReturnValueOnce(page(3, 4, [])) + .mockReturnValueOnce(page(3, 7, [])) + .mockReturnValueOnce(page(3, 10, [10])); + const cli = yield* AzureDevOpsPullRequestCli.AzureDevOpsPullRequestCli; + + const batch = yield* cli.listPullRequests({ + cwd: "/w", + repository: "web", + state: "all", + involvement: "reviewing", + viewer: "bilal@acme.dev", + limit: 2, + }); + + assert.strictEqual(mockedExecute.mock.calls.length, 4); + expect(batch.items.map((item) => item.number)).toEqual([10]); assert.isTrue(batch.truncated); - assert.strictEqual(batch.cursorAdvance, 9); + assert.strictEqual(batch.cursorAdvance, 12); }), ); diff --git a/packages/source-control-azure-devops/src/server/AzureDevOpsPullRequestCli.ts b/packages/source-control-azure-devops/src/server/AzureDevOpsPullRequestCli.ts index f61f0a401cb0f..144ab43629007 100644 --- a/packages/source-control-azure-devops/src/server/AzureDevOpsPullRequestCli.ts +++ b/packages/source-control-azure-devops/src/server/AzureDevOpsPullRequestCli.ts @@ -105,7 +105,10 @@ export type AzureDevOpsPullRequestCliError = const REST_API_VERSION = "7.1"; const PULL_REQUEST_LIST_MAX_OUTPUT_BYTES = 16 * 1024 * 1024; -/** Reviewing drops rows the viewer voted on; bound the reads one listing may spend skipping them. */ +/** + * Reviewing drops rows the viewer voted on; bound the reads one listing spends skipping them. The + * bound waits for one row, since an empty page gives the list service no cursor to resume from. + */ const PULL_REQUEST_LIST_MAX_READS = 3; /** A 2,000-file page exceeds the default 1 MiB output budget. */ const CHANGE_ENTRIES_MAX_OUTPUT_BYTES = 8 * 1024 * 1024; @@ -456,7 +459,10 @@ export const make = Effect.gen(function* () { } const items = [...input.items, ...batch.items]; - if (batch.rawCount < top || input.reads + 1 >= PULL_REQUEST_LIST_MAX_READS) { + if ( + batch.rawCount < top || + (items.length > 0 && input.reads + 1 >= PULL_REQUEST_LIST_MAX_READS) + ) { return Effect.succeed({ items, truncated: batch.rawCount === top, diff --git a/packages/source-control-azure-devops/src/server/azureDevOpsPullRequestJson.ts b/packages/source-control-azure-devops/src/server/azureDevOpsPullRequestJson.ts index cf700e1148b6d..bfd551141b52b 100644 --- a/packages/source-control-azure-devops/src/server/azureDevOpsPullRequestJson.ts +++ b/packages/source-control-azure-devops/src/server/azureDevOpsPullRequestJson.ts @@ -169,7 +169,7 @@ function toActor(raw: Schema.Schema.Type | null | unde /** Azure writes identity ids both with and without dashes. */ function identityKey(id: string | null | undefined): string | null { - return trimmed(id)?.replaceAll("-", "").toLowerCase() ?? null; + return trimmed(id)?.replaceAll("-", "").toLowerCase() || null; } function toState(raw: Schema.Schema.Type): PullRequestState { From f6a6a7d253768093d577f9d437e7bc8b8d6d620b Mon Sep 17 00:00:00 2001 From: Julius Marminge Date: Tue, 10 Nov 2026 22:38:46 +0000 Subject: [PATCH 6/7] test(azure-devops): fold the vote value-type case into the reset case AI-assisted; verified with the tests and tools listed above. --- .../server/azureDevOpsPullRequestJson.test.ts | 21 +++---------------- 1 file changed, 3 insertions(+), 18 deletions(-) diff --git a/packages/source-control-azure-devops/src/server/azureDevOpsPullRequestJson.test.ts b/packages/source-control-azure-devops/src/server/azureDevOpsPullRequestJson.test.ts index 39d392302ee50..d715c37bb4938 100644 --- a/packages/source-control-azure-devops/src/server/azureDevOpsPullRequestJson.test.ts +++ b/packages/source-control-azure-devops/src/server/azureDevOpsPullRequestJson.test.ts @@ -406,13 +406,14 @@ describe("decodeThreadsJson", () => { ]); }); - it("reads a reset vote as withdrawing the reviewer's earlier verdict", () => { + it("reads a reset vote as withdrawing the verdict, in either value type, and skips a missing one", () => { const comments = expectSuccess( decodeThreadsJson( asJson({ value: [ voteThread({ id: 2, vote: "10", voterId: JANE_ID, voterName: "Jane Doe" }), - voteThread({ id: 3, vote: "0", voterId: JANE_ID, voterName: "Jane Doe" }), + voteThread({ id: 3, vote: 0, voterId: JANE_ID, voterName: "Jane Doe" }), + voteThread({ id: 4, vote: null, voterId: JANE_ID, voterName: "Jane Doe" }), ], }), reviewersById(), @@ -425,22 +426,6 @@ describe("decodeThreadsJson", () => { ]); }); - it("reads a vote Azure reports as a number and skips one it does not report", () => { - const comments = expectSuccess( - decodeThreadsJson( - asJson({ - value: [ - voteThread({ id: 2, vote: -10, voterId: JANE_ID, voterName: "Jane Doe" }), - voteThread({ id: 3, vote: null, voterId: JANE_ID, voterName: "Jane Doe" }), - ], - }), - reviewersById(), - ), - ); - - expect(comments.map((comment) => comment.reviewState)).toEqual(["CHANGES_REQUESTED"]); - }); - it("drops deleted threads and threads with nothing to show", () => { const comments = expectSuccess( decodeThreadsJson( From 8bbdacc68f5f1f25618828d34707b95bfb7c507a Mon Sep 17 00:00:00 2001 From: Julius Marminge Date: Sat, 14 Nov 2026 00:45:33 +0000 Subject: [PATCH 7/7] test(azure-devops): drop the provider wiring test for reviewDecision Co-Authored-By: Claude Opus 5.5 --- .../AzureDevOpsPullRequestProvider.test.ts | 36 ------------------- 1 file changed, 36 deletions(-) diff --git a/packages/source-control-azure-devops/src/server/AzureDevOpsPullRequestProvider.test.ts b/packages/source-control-azure-devops/src/server/AzureDevOpsPullRequestProvider.test.ts index 81613637d21e1..4e609d60fc54b 100644 --- a/packages/source-control-azure-devops/src/server/AzureDevOpsPullRequestProvider.test.ts +++ b/packages/source-control-azure-devops/src/server/AzureDevOpsPullRequestProvider.test.ts @@ -124,42 +124,6 @@ function patchedPaths(patch: string): ReadonlyArray { ); } -describe("review decision", () => { - it.effect("carries the reviewers' verdict on every change request Azure is asked for", () => - Effect.gen(function* () { - const approved = { ...PULL_REQUEST, reviewDecision: "approved" as const }; - const provider = yield* make.pipe( - Effect.provide( - Layer.mock(AzureDevOpsPullRequestCli.AzureDevOpsPullRequestCli)({ - listPullRequests: () => - Effect.succeed({ items: [approved], truncated: false, cursorAdvance: 1 }), - getPullRequest: () => Effect.succeed(approved), - listIterations: () => Effect.succeed([ITERATION]), - listIterationChanges: () => Effect.succeed({ changes: [], truncated: false }), - }), - ), - ); - const target = { cwd: "/w", repository: "acme/web", host: "dev.azure.com" }; - - const page = yield* provider.listChangeRequests({ - ...target, - state: "open", - involvement: "all", - viewer: "me@example.com", - limit: 10, - }); - const summary = yield* provider.getChangeRequestSummary!({ ...target, number: 7 }); - const detail = yield* provider.getChangeRequest({ ...target, number: 7 }); - - expect([ - page.items[0]?.reviewDecision, - summary.reviewDecision, - detail.reviewDecision, - ]).toEqual(["approved", "approved", "approved"]); - }), - ); -}); - describe("getChangeRequest", () => { it.effect("preserves the newest head when file counting fails", () => Effect.gen(function* () {