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, 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);