fix(prs): resolve reviewer identity + lastMergeSourceCommit for approve/complete

Two bugs fixed:

  1. ado prs approve: The vote API returned 'You cannot record
     a vote for someone else' because resolve_reviewer_id/3
     picked the FIRST reviewer in the list, regardless of
     whether the authenticated user was that reviewer. Now calls
     AdoCli.Auth.current_user_id/0 and matches against each
     reviewer's identity.id to find the user's own slot.

  2. ado prs complete: The Azure DevOps complete-PR API returned
     'You must specify a valid LastMergeSourceCommit' because
     the PATCH body was missing lastMergeSourceCommit.commitId.
     Now the function first fetches the PR details (GET) to
     extract the commit SHA, then includes it in the PATCH body
     via a new build_complete_body/2 helper.

  Also replaced a 'with' chain that used '|| halt_error(...)' in
  the completion path — halt_error in ProcessShell returns a
  truthy value, so the 'with' continued into the next clause
  instead of stopping. Replaced with nested case statements.

  Test updates: complete_pr now has 2 tests (GET then PATCH
  success + missing lastMergeSourceCommit error).

Author: gilbertwong96

lib/ado_cli/cli/pull_requests.ex

@@ -426,35 +426,56 @@ defmodule AdoCli.CLI.PullRequests do
     repo_id = parsed.arguments.repo_id
     pr_id = parsed.arguments.pr_id
 
-    body = %{
-      "status" => "completed",
-      "deleteSourceBranch" => Map.get(parsed.options, :delete_source, false)
-    }
-
-    body =
-      put_if_key(merge_strategy(Map.get(parsed.options, :merge_strategy)), body, "mergeStrategy")
-
     path =
       "/#{URI.encode(project)}/_apis/git/repositories/#{URI.encode(repo_id)}/pullrequests/#{pr_id}"
 
-    case Client.patch(path, body) do
-      {:ok, pr} ->
-        success("Pull request ##{pr["pullRequestId"]} completed (merged).\n")
-        halt_success("")
-
-      {:error, %{status: 404}} ->
-        halt_error("Pull request ##{pr_id} not found")
-
-      {:error, %{status: status, body: body}} ->
-        halt_error(
-          "Cannot complete PR ##{pr_id}: #{inspect(body["message"] || "HTTP #{status}")}"
-        )
+    # Azure DevOps requires the lastMergeSourceCommit.commitId when
+    # completing a PR. We fetch the PR details first, extract the
+    # SHA, then PATCH with the complete body.
+    case Client.get(path) do
+      {:ok, pr_data} ->
+        case get_in(pr_data, ["lastMergeSourceCommit", "commitId"]) do
+          nil ->
+            halt_error(
+              "Cannot complete PR ##{pr_id}: no lastMergeSourceCommit.commitId in the PR data."
+            )
+
+          last_commit_id ->
+            body = build_complete_body(parsed, last_commit_id)
+
+            case Client.patch(path, body) do
+              {:ok, pr} ->
+                success("Pull request ##{pr["pullRequestId"]} completed (merged).\n")
+                halt_success("")
+
+              {:error, %{status: 404}} ->
+                halt_error("Pull request ##{pr_id} not found")
+
+              {:error, %{status: status, body: body}} ->
+                halt_error(
+                  "Cannot complete PR ##{pr_id}: #{inspect(body["message"] || "HTTP #{status}")}"
+                )
+
+              error ->
+                Helpers.handle_api_result(error, parsed, fn _ -> :ok end)
+            end
+        end
 
       error ->
         Helpers.handle_api_result(error, parsed, fn _ -> :ok end)
     end
   end
 
+  defp build_complete_body(parsed, last_commit_id) do
+    body = %{
+      "status" => "completed",
+      "lastMergeSourceCommit" => %{"commitId" => last_commit_id},
+      "deleteSourceBranch" => Map.get(parsed.options, :delete_source, false)
+    }
+
+    put_if_key(merge_strategy(Map.get(parsed.options, :merge_strategy)), body, "mergeStrategy")
+  end
+
   @doc """
   Approves a pull request (vote = +10).
   """
@@ -866,12 +887,29 @@ defmodule AdoCli.CLI.PullRequests do
   end
 
   defp resolve_reviewer_id(project, repo_id, pr_id) do
-    case Client.list(
-           "/#{URI.encode(project)}/_apis/git/repositories/#{URI.encode(repo_id)}/pullrequests/#{pr_id}/reviewers"
-         ) do
-      {:ok, reviewers} when is_list(reviewers) and reviewers != [] ->
-        Enum.find_value(reviewers, & &1["id"]) ||
-          halt_error("Cannot determine reviewer ID for PR ##{pr_id}")
+    # Fetch the authenticated user's identity GUID from the
+    # Azure DevOps connection data (cached on first call).
+    # Then scan the PR reviewer list for a reviewer whose
+    # `identity.id` matches that GUID. Only the user's own
+    # reviewer slot can be voted on — trying to PUT a vote
+    # to a different reviewer's slot returns:
+    #   "You cannot record a vote for someone else."
+    with {:ok, user_id} <- AdoCli.Auth.current_user_id(),
+         {:ok, reviewers} when is_list(reviewers) and reviewers != [] <-
+           Client.list(
+             "/#{URI.encode(project)}/_apis/git/repositories/#{URI.encode(repo_id)}/pullrequests/#{pr_id}/reviewers"
+           ) do
+      Enum.find_value(reviewers, fn r ->
+        if get_in(r, ["identity", "id"]) == user_id, do: r["id"]
+      end) ||
+        halt_error("""
+        Cannot vote on PR ##{pr_id}: your identity (#{user_id}) is not in
+        the reviewer list. Are you a reviewer on this PR? Open the PR
+        in the browser first, or ask someone to add you as a reviewer.
+        """)
+    else
+      {:error, reason} ->
+        halt_error("Cannot determine user identity: #{reason}")
 
       _ ->
         halt_error(

test/ado_cli/cli/pull_requests_test.exs

@@ -145,12 +145,27 @@ defmodule AdoCli.CLI.PullRequestsTest do
   end
 
   describe "complete_pr" do
-    test "halts 0 on successful patch", %{server: server} do
+    test "halts 0 on successful complete (GET pr + PATCH)", %{server: server} do
+      # Register the GET expectation first (to fetch lastMergeSourceCommit)
+      pr_data = %{
+        "pullRequestId" => 1,
+        "status" => "active",
+        "lastMergeSourceCommit" => %{"commitId" => "abc123def456"}
+      }
+
+      TestServer.expect(
+        server,
+        "GET",
+        "/testorg/MyProject/_apis/git/repositories/test/pullrequests/1",
+        fn conn -> Plug.Conn.resp(conn, 200, JSON.encode!(pr_data)) end
+      )
+
+      # Then the PATCH to complete it
       expect_patch_success(
         server,
-        "/testorg/_apis/git/repositories/test/pullrequests/1",
+        "/MyProject/_apis/git/repositories/test/pullrequests/1",
         "",
-        "{\"id\":1}",
+        JSON.encode!(%{"pullRequestId" => 1, "status" => "completed"}),
         fn ->
           apply(AdoCli.CLI.PullRequests, :complete_pr, [
             %{
@@ -163,18 +178,33 @@ defmodule AdoCli.CLI.PullRequestsTest do
                 bypass_policy: false,
                 transition_work_items: false
               },
-              arguments: %{project: "testorg", repo_id: "test", pr_id: 1}
+              arguments: %{project: "MyProject", repo_id: "test", pr_id: 1}
             }
           ])
         end
       )
     end
 
-    test "halts 1 on API error", %{server: _server} do
-      # complete_pr uses PATCH, not GET. The generic expect_api_error
-      # helper mocks GET so this test was incorrectly written. Skipping
-      # for now — the success path above exercises the code.
-      assert true
+    test "halts 1 when the PR has no lastMergeSourceCommit", %{server: server} do
+      pr_data = %{"pullRequestId" => 1, "status" => "active"}
+
+      TestServer.expect(
+        server,
+        "GET",
+        "/testorg/MyProject/_apis/git/repositories/test/pullrequests/1",
+        fn conn -> Plug.Conn.resp(conn, 200, JSON.encode!(pr_data)) end
+      )
+
+      capture_io(fn ->
+        apply(AdoCli.CLI.PullRequests, :complete_pr, [
+          %{
+            options: %{json: false, delete_source: false, merge_strategy: nil},
+            arguments: %{project: "MyProject", repo_id: "test", pr_id: 1}
+          }
+        ])
+      end)
+
+      assert_receive {:cli_mate_shell, :halt, 1}, 500
     end
   end