From db4a0432101f498db44f9b19fe242a14f796bbb2 Mon Sep 17 00:00:00 2001 From: shivam <91240327+shivamhwp@users.noreply.github.com> Date: Wed, 4 Nov 2026 22:27:12 +0000 Subject: [PATCH 1/2] fix(azure-devops): read reviewer votes into review state Azure DevOps reviewers carried no vote, so every reviewer stayed requested, pull requests had no review decision, and votes never reached activity or pull request watches. Reviewers now read their vote. Only reviewers yet to vote stay requested. The review decision is changes-requested when anyone waits or rejects, review-required while a required reviewer has not voted, and approved otherwise once someone approves. Lists, summaries and details carry it. VoteUpdate system threads become typed review entries for the voter. Co-Authored-By: Claude Opus 5.5 --- .../AzureDevOpsPullRequestProvider.test.ts | 34 +++++ .../server/AzureDevOpsPullRequestProvider.ts | 6 + .../server/azureDevOpsPullRequestJson.test.ts | 126 ++++++++++++++++++ .../src/server/azureDevOpsPullRequestJson.ts | 103 +++++++++++++- 4 files changed, 265 insertions(+), 4 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 345d74de84ba1..823e112b5818a 100644 --- a/packages/source-control-azure-devops/src/server/AzureDevOpsPullRequestProvider.test.ts +++ b/packages/source-control-azure-devops/src/server/AzureDevOpsPullRequestProvider.test.ts @@ -189,6 +189,40 @@ describe("getChangeRequest", () => { ); }); +describe("review decision", () => { + it.effect("carries the decision Azure's votes add up to into every read of a pull request", () => + Effect.gen(function* () { + const voted = { ...PULL_REQUEST, reviewDecision: "changes-requested" as const }; + const provider = yield* make.pipe( + Effect.provide( + Layer.mock(AzureDevOpsPullRequestCli.AzureDevOpsPullRequestCli)({ + listPullRequests: () => + Effect.succeed({ items: [voted], truncated: false, cursorAdvance: 1 }), + getPullRequest: () => Effect.succeed(voted), + listIterations: () => Effect.succeed([ITERATION]), + listIterationChanges: () => Effect.succeed({ changes: [], truncated: false }), + }), + ), + ); + const ref = { cwd: "/w", repository: "acme/web", host: "dev.azure.com" }; + + const listed = yield* provider.listChangeRequests({ + ...ref, + state: "open", + involvement: "all", + viewer: "bilal@acme.dev", + limit: 10, + }); + const summary = yield* provider.getChangeRequestSummary!({ ...ref, number: 7 }); + const detail = yield* provider.getChangeRequest({ ...ref, number: 7 }); + + expect(listed.items[0]?.reviewDecision).toBe("changes-requested"); + expect(summary.reviewDecision).toBe("changes-requested"); + expect(detail.reviewDecision).toBe("changes-requested"); + }), + ); +}); + describe("getDiff reads", () => { it.effect("holds every reader together to one request's worth of processes", () => Effect.gen(function* () { diff --git a/packages/source-control-azure-devops/src/server/AzureDevOpsPullRequestProvider.ts b/packages/source-control-azure-devops/src/server/AzureDevOpsPullRequestProvider.ts index a1d3ddb6b8c96..a39988952d007 100644 --- a/packages/source-control-azure-devops/src/server/AzureDevOpsPullRequestProvider.ts +++ b/packages/source-control-azure-devops/src/server/AzureDevOpsPullRequestProvider.ts @@ -108,6 +108,9 @@ function toChangeRequest(pullRequest: AzureDevOpsPullRequest): ProviderChangeReq mergedAt: pullRequest.state === "merged" ? pullRequest.closedAt : null, updatedAt: pullRequest.updatedAt, reviewRequestLogins: pullRequest.reviewRequestLogins, + ...(pullRequest.reviewDecision === undefined + ? {} + : { reviewDecision: pullRequest.reviewDecision }), // Azure keeps labels on work items rather than on the pull request. labels: [], }; @@ -304,6 +307,9 @@ export const make = Effect.gen(function* () { closedAt: pullRequest.state === "closed" ? pullRequest.closedAt : null, mergedAt: pullRequest.state === "merged" ? pullRequest.closedAt : null, updatedAt: pullRequest.updatedAt, + ...(pullRequest.reviewDecision === undefined + ? {} + : { reviewDecision: pullRequest.reviewDecision }), })), ), 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..18bb721b076c2 100644 --- a/packages/source-control-azure-devops/src/server/azureDevOpsPullRequestJson.test.ts +++ b/packages/source-control-azure-devops/src/server/azureDevOpsPullRequestJson.test.ts @@ -94,6 +94,52 @@ describe("decodePullRequestListJson", () => { }); }); + const reviewer = (login: string, fields: Record = {}) => ({ + displayName: login, + uniqueName: `${login}@acme.dev`, + ...fields, + }); + + it.each([ + ["no reviewers", [], null, []], + ["an optional reviewer yet to vote", [reviewer("a", { vote: 0 })], null, ["a"]], + ["an approval", [reviewer("a", { vote: 10 })], "approved", []], + ["an approval with suggestions", [reviewer("a", { vote: 5 })], "approved", []], + ["a wait for the author", [reviewer("a", { vote: -5 })], "changes-requested", []], + [ + "a rejection beside an approval", + [reviewer("a", { vote: 10 }), reviewer("b", { vote: -10 })], + "changes-requested", + [], + ], + [ + "a required reviewer yet to vote beside an optional approval", + [reviewer("a", { vote: 10 }), reviewer("b", { vote: 0, isRequired: true })], + "review-required", + ["b"], + ], + [ + "a required group yet to vote", + [reviewer("leads", { vote: 0, isRequired: true, isContainer: true })], + "review-required", + ["leads"], + ], + [ + "a required vote Azure has no meaning for", + [reviewer("a", { vote: 7, isRequired: true }), reviewer("b", { vote: "soon" })], + "review-required", + ["a", "b"], + ], + ])("reads %s as a review decision", (_case, reviewers, reviewDecision, requested) => { + const batch = expectSuccess(decodePullRequestListJson(asJson([pullRequest({ reviewers })]))); + + expect(batch.items[0]?.reviewDecision).toBe(reviewDecision); + expect(batch.items[0]?.reviewRequestLogins).toEqual( + requested.map((login) => `${login}@acme.dev`), + ); + expect(batch.items[0]?.reviewers).toHaveLength(reviewers.length); + }); + it("skips a malformed row but still counts it, so paging does not stop early", () => { const batch = expectSuccess( decodePullRequestListJson(asJson([{ pullRequestId: "nope" }, pullRequest()])), @@ -248,6 +294,86 @@ describe("decodeThreadsJson", () => { expect(comments.map((comment) => comment.id)).toEqual(["4:1", "4:2", "4:3"]); }); + it("reads a vote Azure records on a system thread as a review by the voter", () => { + const service = { + displayName: "[acme]\\Project Collection Service Accounts", + isContainer: true, + }; + const property = (value: unknown) => ({ $type: "System.String", $value: value }); + const voteThread = ( + id: number, + properties: Record, + extra: Record = {}, + ) => ({ + id, + properties: { CodeReviewThreadType: property("VoteUpdate"), ...properties }, + comments: [ + { + id: 1, + content: "Somebody voted", + commentType: "system", + author: service, + publishedDate: `2026-07-0${id}T00:00:00Z`, + }, + ], + ...extra, + }); + const comments = expectSuccess( + decodeThreadsJson( + asJson({ + value: [ + voteThread( + 1, + { + CodeReviewVoteResult: property("10"), + CodeReviewVotedByIdentity: property("1"), + CodeReviewVotedByDisplayName: property("Julius"), + }, + { identities: { "1": { displayName: "Julius", uniqueName: "julius@acme.dev" } } }, + ), + voteThread(2, { + CodeReviewVoteResult: { $type: "System.Int32", $value: -5 }, + CodeReviewVotedByDisplayName: property("Bilal"), + }), + voteThread(3, { CodeReviewVoteResult: property("0") }), + voteThread(4, { CodeReviewVoteResult: property("7") }), + { + id: 5, + properties: { CodeReviewThreadType: property("MergeAttempt") }, + comments: [ + { + id: 1, + content: "The pull request is mergeable.", + commentType: "system", + publishedDate: "2026-07-05T00:00:00Z", + }, + ], + }, + ], + }), + ), + ); + + expect(comments).toEqual([ + { + id: "1:1", + kind: "review", + author: { login: "julius@acme.dev", name: "Julius", avatarUrl: null }, + body: "", + createdAt: "2026-07-01T00:00:00Z", + url: null, + path: null, + reviewState: "APPROVED", + }, + expect.objectContaining({ + id: "2:1", + author: { login: "Bilal", name: "Bilal", avatarUrl: null }, + reviewState: "CHANGES_REQUESTED", + }), + expect.objectContaining({ id: "3:1", author: null, reviewState: "DISMISSED" }), + ]); + }); + 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 b3632ef1af98f..0008402286305 100644 --- a/packages/source-control-azure-devops/src/server/azureDevOpsPullRequestJson.ts +++ b/packages/source-control-azure-devops/src/server/azureDevOpsPullRequestJson.ts @@ -4,6 +4,7 @@ import { } from "@t3tools/source-control-core/server/pullRequestUtils"; import * as Cause from "effect/Cause"; import * as Exit from "effect/Exit"; +import * as Option from "effect/Option"; import * as Result from "effect/Result"; import * as Schema from "effect/Schema"; import type { @@ -11,6 +12,7 @@ import type { PullRequestComment, PullRequestMergeMethod, PullRequestMergeability, + PullRequestReviewDecision, PullRequestState, } from "@t3tools/contracts"; import { TrimmedNonEmptyString } from "@t3tools/contracts"; @@ -25,6 +27,13 @@ const RawIdentitySchema = Schema.Struct({ imageUrl: Schema.optional(Schema.NullOr(Schema.String)), }); +// Vote and flags stay unknown so an odd value reads as no vote instead of dropping the row. +const RawReviewerSchema = Schema.Struct({ + ...RawIdentitySchema.fields, + vote: Schema.optional(Schema.Unknown), + isRequired: Schema.optional(Schema.Unknown), +}); + const RawPullRequestSchema = Schema.Struct({ pullRequestId: Schema.Int, title: Schema.String, @@ -43,7 +52,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 +83,9 @@ const RawPullRequestSchema = Schema.Struct({ const RawThreadSchema = Schema.Struct({ id: Schema.Int, isDeleted: Schema.optional(Schema.NullOr(Schema.Boolean)), + /** Values are `{ $type, $value }`; system threads name their event in `CodeReviewThreadType`. */ + properties: Schema.optional(Schema.NullOr(Schema.Record(Schema.String, Schema.Unknown))), + identities: Schema.optional(Schema.NullOr(Schema.Record(Schema.String, Schema.Unknown))), threadContext: Schema.optional( Schema.NullOr(Schema.Struct({ filePath: Schema.optional(Schema.NullOr(Schema.String)) })), ), @@ -122,8 +134,11 @@ export interface AzureDevOpsPullRequest { readonly updatedAt: string; readonly closedAt: string | null; readonly body: string; + /** Reviewers yet to vote. */ readonly reviewRequestLogins: ReadonlyArray; readonly reviewers: ReadonlyArray; + /** Absent when Azure listed no reviewers to decide from. */ + readonly reviewDecision?: PullRequestReviewDecision | null; readonly location: AzureDevOpsRepositoryLocation | null; readonly autoMergeEnabled: boolean; readonly autoMergeMethod?: PullRequestMergeMethod; @@ -187,14 +202,36 @@ function toAutoMergeMethod( } } +/** Azure's vote scale: 10 approved, 5 with suggestions, 0 none, -5 waiting for author, -10 rejected. */ +type AzureDevOpsVote = 10 | 5 | 0 | -5 | -10; + +function toVote(value: unknown): AzureDevOpsVote | null { + const vote = typeof value === "string" && value.trim() !== "" ? Number(value) : value; + return vote === 10 || vote === 5 || vote === 0 || vote === -5 || vote === -10 ? vote : null; +} + +/** Any wait or rejection wins; a required reviewer yet to approve outranks optional approvals. */ +function toReviewDecision( + reviewers: ReadonlyArray<{ readonly vote: AzureDevOpsVote; readonly isRequired: boolean }>, +): PullRequestReviewDecision | null { + if (reviewers.some((reviewer) => reviewer.vote < 0)) return "changes-requested"; + if (reviewers.some((reviewer) => reviewer.isRequired && reviewer.vote === 0)) { + return "review-required"; + } + return reviewers.some((reviewer) => reviewer.vote > 0) ? "approved" : null; +} + function toPullRequest( raw: Schema.Schema.Type, ): AzureDevOpsPullRequest | null { const autoMergeMethod = toAutoMergeMethod(raw); - const reviewers = (raw.reviewers ?? []).flatMap((reviewer) => { + const voters = (raw.reviewers ?? []).flatMap((reviewer) => { const actor = toActor(reviewer); - return actor === null ? [] : [actor]; + return actor === null + ? [] + : [{ actor, vote: toVote(reviewer.vote) ?? 0, isRequired: reviewer.isRequired === true }]; }); + const reviewers = voters.map((voter) => voter.actor); const closedAt = trimmed(raw.closedDate); const url = trimmed( azureDevOpsPullRequestWebUrl({ @@ -223,8 +260,11 @@ function toPullRequest( updatedAt: closedAt ?? raw.creationDate, closedAt, body: raw.description ?? "", - reviewRequestLogins: reviewers.map((reviewer) => reviewer.login), + reviewRequestLogins: voters + .filter((voter) => voter.vote === 0) + .map((voter) => voter.actor.login), reviewers, + ...(raw.reviewers == null ? {} : { reviewDecision: toReviewDecision(voters) }), location: toLocation(raw), autoMergeEnabled: (raw.autoCompleteSetBy ?? null) !== null, ...(autoMergeMethod === undefined ? {} : { autoMergeMethod }), @@ -236,6 +276,7 @@ const decodePullRequestEntry = Schema.decodeUnknownExit(RawPullRequestSchema); const decodePullRequest = decodeJsonResult(RawPullRequestSchema); const decodeThreadPage = decodeJsonResult(RawThreadPageSchema); const decodeThreadEntry = Schema.decodeUnknownExit(RawThreadSchema); +const decodeIdentity = Schema.decodeUnknownOption(RawIdentitySchema); const decodeViewer = decodeJsonResult(RawViewerSchema); type DecodeFailure = Cause.Cause; @@ -285,6 +326,55 @@ export function decodeViewerJson(raw: string): Result.Result> | null | undefined, + key: string, +): unknown { + const property = properties?.[key]; + return typeof property === "object" && property !== null && "$value" in property + ? property.$value + : undefined; +} + +const VOTE_REVIEW_STATES: Record = { + 10: "APPROVED", + 5: "APPROVED", + 0: "DISMISSED", + [-5]: "CHANGES_REQUESTED", + [-10]: "CHANGES_REQUESTED", +}; + +/** Azure posts each vote as a system thread, usually authored by a service account. */ +function toVoteReview( + thread: Schema.Schema.Type, +): PullRequestComment | null { + if (propertyValue(thread.properties, "CodeReviewThreadType") !== "VoteUpdate") return null; + const vote = toVote(propertyValue(thread.properties, "CodeReviewVoteResult")); + const comment = thread.comments?.find((entry) => entry.isDeleted !== true); + const createdAt = trimmed(comment?.publishedDate); + if (vote === null || comment === undefined || createdAt === null) return null; + // Newer payloads name the voter by a key into the thread's identities; older ones only by name. + const identityKey = propertyValue(thread.properties, "CodeReviewVotedByIdentity"); + const identity = typeof identityKey === "string" ? thread.identities?.[identityKey] : undefined; + const votedBy = + identity === undefined ? null : toActor(Option.getOrNull(decodeIdentity(identity))); + const votedByName = propertyValue(thread.properties, "CodeReviewVotedByDisplayName"); + const namedVoter = + typeof votedByName === "string" && trimmed(votedByName) !== null + ? { login: votedByName.trim(), name: votedByName.trim(), avatarUrl: null } + : null; + return { + id: `${thread.id}:${comment.id ?? 0}`, + kind: "review", + author: votedBy ?? namedVoter, + body: "", + createdAt, + url: null, + path: null, + reviewState: VOTE_REVIEW_STATES[vote], + }; +} + export function decodeThreadsJson( raw: string, ): Result.Result, DecodeFailure> { @@ -298,6 +388,11 @@ export function decodeThreadsJson( if (Exit.isFailure(decodedThread)) continue; const thread = decodedThread.value; if (thread.isDeleted === true) continue; + const vote = toVoteReview(thread); + if (vote !== null) { + comments.push(vote); + continue; + } const path = toRepositoryPath(thread.threadContext?.filePath); for (const comment of thread.comments ?? []) { const publishedDate = trimmed(comment.publishedDate); From d74d70594f4d8ef3c474e3fe5ea170baa28a11d3 Mon Sep 17 00:00:00 2001 From: shivam <91240327+shivamhwp@users.noreply.github.com> Date: Wed, 4 Nov 2026 22:34:18 +0000 Subject: [PATCH 2/2] fix(azure-devops): exclude voted reviewers from requested pages --- .../server/AzureDevOpsPullRequestCli.test.ts | 30 +++++++++++++++++++ .../src/server/AzureDevOpsPullRequestCli.ts | 20 +++++++++++-- 2 files changed, 47 insertions(+), 3 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..e4c4dd62ab073 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,36 @@ layer("AzureDevOpsPullRequestCli.layer", (it) => { }), ); + it.effect("pages past reviewers who already voted when listing requested reviews", () => + Effect.gen(function* () { + const row = (number: number, vote: number) => ({ + ...pullRequestRow, + pullRequestId: number, + reviewers: [{ uniqueName: "BILAL@acme.dev", vote }], + }); + mockedExecute + .mockReturnValueOnce( + Effect.succeed(output(JSON.stringify([row(1, 10), row(2, 0), row(3, -10)]))), + ) + .mockReturnValueOnce(Effect.succeed(output(JSON.stringify([row(4, 0), row(5, 0)])))); + 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, 4]); + assert.isTrue(batch.truncated); + assert.strictEqual(batch.cursorAdvance, 4); + const nextArgs = argsOfCall(1); + assert.strictEqual(nextArgs[nextArgs.indexOf("--skip") + 1], "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..c2d62d2d8b716 100644 --- a/packages/source-control-azure-devops/src/server/AzureDevOpsPullRequestCli.ts +++ b/packages/source-control-azure-devops/src/server/AzureDevOpsPullRequestCli.ts @@ -416,17 +416,31 @@ export const make = Effect.gen(function* () { ); } - const lastItemIndex = decoded.success.rawIndexes[remaining - 1]; + // Azure's reviewer query includes reviewers who already voted. + const eligibleIndexes = decoded.success.items.flatMap((item, index) => + input.involvement !== "reviewing" || + item.reviewRequestLogins.some( + (login) => login.toLowerCase() === input.viewer.toLowerCase(), + ) + ? [index] + : [], + ); + const eligible = eligibleIndexes.map((index) => decoded.success.items[index]!); + const lastEligibleIndex = eligibleIndexes[remaining - 1]; + const lastItemIndex = + lastEligibleIndex === undefined + ? undefined + : decoded.success.rawIndexes[lastEligibleIndex]; if (lastItemIndex !== undefined) { const consumed = lastItemIndex + 1; return Effect.succeed({ - items: [...input.items, ...decoded.success.items.slice(0, remaining)], + items: [...input.items, ...eligible.slice(0, remaining)], truncated: consumed < decoded.success.rawCount || decoded.success.rawCount === top, cursorAdvance: input.cursorAdvance + consumed, }); } - const items = [...input.items, ...decoded.success.items]; + const items = [...input.items, ...eligible]; if (decoded.success.rawCount < top) { return Effect.succeed({ items,