refactor(pulls): tighten reviewer-request flow

Follow-up to 5d3db5d (#571). No user-visible CLI changes. - Replace tautological metadata unit tests with httptest-based e2e tests that exercise runRequestReview, runCancelReview, and runReviewersList against a mock Gitea API; each test captures method, path, and request body. - Add tests/integration/pulls_reviewers_test.go covering create-time --reviewer/--team-reviewer, request-review add, cancel-review by user and by team, validation error, and bad-PR error against a real Gitea instance. - Extract parseReviewRequestArgs in cmd/pulls/review_helpers.go so runRequestReview and runCancelReview share one validation/parsing path; both now route through task.ApplyReviewerChanges instead of inlining the SDK call. - Extend task.ApplyReviewerChanges to accept teamAdd/teamRm and use it from request-review, cancel-review, and the existing edit flow. - Replace task.CreatePull 8-positional-arg signature with a CreatePullOptions struct; update cmd/pulls/create.go and modules/interact/pull_create.go call sites. - Defensively filter empty CSV entries in parseReviewRequestArgs via nonEmptyValues so the validation is correct regardless of whether the underlying CsvFlag trims empty entries (kept independent of the separate CsvFlag bugfix PR). Verification: make fmt fmt-check vet lint test docs docs-check build
2026-09-10 07:26:33 -04:00 · 2026-07-29 17:18:42 +07:00 · 2026-07-29 17:18:42 +07:00 · 32eebf7040
parent f4b549b538
commit 32eebf7040
14 changed files with 731 additions and 226 deletions
--- a/cmd/pulls/cancel_review.go
+++ b/cmd/pulls/cancel_review.go
@ -5,15 +5,13 @@ package pulls

 import (
 	stdctx "context"
-	"errors"
 	"fmt"

-	gitea "gitea.dev/sdk"
 	"github.com/urfave/cli/v3"

 	"gitea.dev/tea/cmd/flags"
 	"gitea.dev/tea/modules/context"
-	"gitea.dev/tea/modules/utils"
+	"gitea.dev/tea/modules/task"
 )

 // CmdPullsCancelReview cancels previously requested reviews on one or more PRs
@ -32,6 +30,10 @@ At least one of --reviewer or --team-reviewer is required.`,
 }

 func runCancelReview(requestCtx stdctx.Context, cmd *cli.Command) error {
+	args, err := parseReviewRequestArgs(requestCtx, cmd)
+	if err != nil {
+		return err
+	}
 	ctx, err := context.InitCommand(cmd)
 	if err != nil {
 		return err
@ -40,32 +42,14 @@ func runCancelReview(requestCtx stdctx.Context, cmd *cli.Command) error {
 		return err
 	}

-	reviewers, err := ReviewerFlag.GetValues(cmd)
-	if err != nil {
-		return err
-	}
-	teamReviewers, err := TeamReviewerFlag.GetValues(cmd)
-	if err != nil {
-		return err
-	}
-	if len(reviewers) == 0 && len(teamReviewers) == 0 {
-		return errors.New("at least one of --reviewer or --team-reviewer is required")
-	}
-
-	indices, err := utils.ArgsToIndices(cmd.Args().Slice())
-	if err != nil {
-		return err
-	}
-
 	client := ctx.Login.Client()
-	for _, idx := range indices {
-		if _, err := client.PullRequests.DeleteReviewRequests(requestCtx, ctx.Owner, ctx.Repo, idx, gitea.PullReviewRequestOptions{
-			Reviewers:     reviewers,
-			TeamReviewers: teamReviewers,
-		}); err != nil {
+	for _, idx := range args.Indices {
+		if err := task.ApplyReviewerChanges(requestCtx, client, ctx.Owner, ctx.Repo, idx,
+			nil, args.Reviewers,
+			nil, args.TeamReviewers); err != nil {
 			return err
 		}
-		fmt.Printf("PR#%d: canceled review requests for %v, teams %v\n", idx, reviewers, teamReviewers)
+		fmt.Printf("PR#%d: canceled review requests for %v, teams %v\n", idx, args.Reviewers, args.TeamReviewers)
 	}
 	return nil
 }
--- a/cmd/pulls/cancel_review_test.go
+++ b/cmd/pulls/cancel_review_test.go
@ -4,40 +4,105 @@
 package pulls

 import (
+	"context"
+	"io"
+	"net/http"
 	"testing"

+	gitea "gitea.dev/sdk"
 	"github.com/stretchr/testify/assert"
-	"github.com/urfave/cli/v3"
+	"github.com/stretchr/testify/require"
 )

-func TestCancelReviewCommandMetadata(t *testing.T) {
-	cmd := &CmdPullsCancelReview
+func TestRunCancelReviewUsesDelete(t *testing.T) {
+	var (
+		gotMethod string
+		gotPath   string
+		gotBody   []byte
+	)
+	repo := "owner/repo"
+	withMockServer(t, func(w http.ResponseWriter, r *http.Request) {
+		switch r.URL.Path {
+		case "/api/v1/version":
+			w.Header().Set("Content-Type", "application/json")
+			_, _ = w.Write([]byte(`{"version":"1.22.0"}`))
+		case "/api/v1/repos/" + repo + "/pulls/9/requested_reviewers":
+			gotMethod = r.Method
+			gotPath = r.URL.Path
+			gotBody, _ = io.ReadAll(r.Body)
+			w.WriteHeader(http.StatusNoContent)
+		default:
+			t.Errorf("unexpected request: %s %s", r.Method, r.URL.Path)
+			w.WriteHeader(http.StatusNotFound)
+		}
+	})

-	assert.Equal(t, "cancel-review", cmd.Name)
-	assert.Contains(t, cmd.Aliases, "cr")
-	assert.Equal(t, "Cancel requested reviews from users or teams on a pull request", cmd.Usage)
-	assert.Equal(t, "<pull index> [<pull index>...]", cmd.ArgsUsage)
-	assert.NotNil(t, cmd.Action)
+	cmd := runAction(t, CmdPullsCancelReview, []string{
+		"--login", "test",
+		"--repo", repo,
+		"--output", "json",
+		"--reviewer", "alice",
+		"--team-reviewer", "devs",
+		"9",
+	})
+	require.NoError(t, runCancelReview(context.Background(), cmd))
+
+	assert.Equal(t, http.MethodDelete, gotMethod)
+	assert.Equal(t, "/api/v1/repos/"+repo+"/pulls/9/requested_reviewers", gotPath)
+	expectJSONBody(t, gotBody, gitea.PullReviewRequestOptions{
+		Reviewers:     []string{"alice"},
+		TeamReviewers: []string{"devs"},
+	})
 }

-func TestCancelReviewCommandFlags(t *testing.T) {
-	cmd := &CmdPullsCancelReview
-
-	expectedFlags := []string{
-		"reviewer",
-		"team-reviewer",
-	}
-
-	for _, flagName := range expectedFlags {
-		found := false
-		for _, flag := range cmd.Flags {
-			if flag.Names()[0] == flagName {
-				found = true
-				break
-			}
+func TestRunCancelReviewRejectsMissingFlags(t *testing.T) {
+	repo := "owner/repo"
+	withMockServer(t, func(w http.ResponseWriter, r *http.Request) {
+		if r.URL.Path != "/api/v1/version" {
+			t.Errorf("should not hit API when --reviewer/--team-reviewer are absent: %s %s", r.Method, r.URL.Path)
+			w.WriteHeader(http.StatusNotFound)
+			return
 		}
-		assert.True(t, found, "Expected flag %s not found", flagName)
-	}
+		w.Header().Set("Content-Type", "application/json")
+		_, _ = w.Write([]byte(`{"version":"1.22.0"}`))
+	})
+
+	cmd := runAction(t, CmdPullsCancelReview, []string{
+		"--login", "test",
+		"--repo", repo,
+		"--output", "json",
+		"9",
+	})
+	err := runCancelReview(context.Background(), cmd)
+	require.Error(t, err)
+	assert.Contains(t, err.Error(), "at least one of --reviewer or --team-reviewer is required")
+}
+
+func TestRunCancelReviewPropagatesAPIError(t *testing.T) {
+	repo := "owner/repo"
+	withMockServer(t, func(w http.ResponseWriter, r *http.Request) {
+		switch r.URL.Path {
+		case "/api/v1/version":
+			w.Header().Set("Content-Type", "application/json")
+			_, _ = w.Write([]byte(`{"version":"1.22.0"}`))
+		case "/api/v1/repos/" + repo + "/pulls/9/requested_reviewers":
+			w.WriteHeader(http.StatusForbidden)
+			_, _ = w.Write([]byte(`{"message":"not allowed"}`))
+		default:
+			w.WriteHeader(http.StatusNotFound)
+		}
+	})
+
+	cmd := runAction(t, CmdPullsCancelReview, []string{
+		"--login", "test",
+		"--repo", repo,
+		"--output", "json",
+		"--reviewer", "alice",
+		"9",
+	})
+	err := runCancelReview(context.Background(), cmd)
+	require.Error(t, err)
+	assert.Contains(t, err.Error(), "not allowed")
 }

 // TestCancelReviewReusesReviewerFlag ensures request-review and cancel-review
@ -46,13 +111,6 @@ func TestCancelReviewReusesReviewerFlag(t *testing.T) {
 	assert.Equal(t,
 		flagNames(CmdPullsRequestReview.Flags),
 		flagNames(CmdPullsCancelReview.Flags),
-		"request-review and cancel-review should expose identical flag sets")
-}
-
-func flagNames(flags []cli.Flag) []string {
-	names := make([]string, 0, len(flags))
-	for _, f := range flags {
-		names = append(names, f.Names()[0])
-	}
-	return names
+		"request-review and cancel-review should expose identical flag sets",
+	)
 }
--- a/cmd/pulls/create.go
+++ b/cmd/pulls/create.go
@ -122,11 +122,13 @@ func runPullsCreate(requestCtx stdctx.Context, cmd *cli.Command) error {
 	return task.CreatePull(
 		requestCtx,
 		ctx,
-		ctx.String("base"),
-		ctx.String("head"),
-		allowMaintainerEdits,
-		opts,
-		reviewers,
-		teamReviewers,
+		task.CreatePullOptions{
+			Base:                 ctx.String("base"),
+			Head:                 ctx.String("head"),
+			AllowMaintainerEdits: allowMaintainerEdits,
+			Issue:                opts,
+			Reviewers:            reviewers,
+			TeamReviewers:        teamReviewers,
+		},
 	)
 }
--- a/cmd/pulls/create_test.go
+++ b/cmd/pulls/create_test.go
@ -9,37 +9,13 @@ import (
 	"github.com/stretchr/testify/assert"
 )

-func TestCreateCommandMetadata(t *testing.T) {
-	cmd := &CmdPullsCreate
-
-	assert.Equal(t, "create", cmd.Name)
-	assert.Contains(t, cmd.Aliases, "c")
-	assert.Equal(t, "Create a pull-request", cmd.Usage)
-	assert.NotNil(t, cmd.Action)
-}
-
-func TestCreateCommandFlags(t *testing.T) {
-	cmd := &CmdPullsCreate
-
-	expectedFlags := []string{
-		"head",
-		"base",
-		"allow-maintainer-edits",
-		"agit",
-		"topic",
-		"draft",
-		"reviewer",
-		"team-reviewer",
-	}
-
-	for _, flagName := range expectedFlags {
-		found := false
-		for _, flag := range cmd.Flags {
-			if flag.Names()[0] == flagName {
-				found = true
-				break
-			}
-		}
-		assert.True(t, found, "Expected flag %s not found", flagName)
+// TestCreateCommandExposesReviewerFlags verifies that `pulls create`
+// advertises the reviewer / team-reviewer flags introduced in this PR.
+// It does not exercise the action (which is covered by integration tests).
+func TestCreateCommandExposesReviewerFlags(t *testing.T) {
+	want := []string{"reviewer", "team-reviewer"}
+	have := flagNames(CmdPullsCreate.Flags)
+	for _, w := range want {
+		assert.Contains(t, have, w, "pulls create should expose --%s", w)
 	}
 }
--- a/cmd/pulls/helpers_test.go
+++ b/cmd/pulls/helpers_test.go
@ -0,0 +1,89 @@
+// Copyright 2026 The Gitea Authors. All rights reserved.
+// SPDX-License-Identifier: MIT
+
+package pulls
+
+import (
+	"bytes"
+	"context"
+	"encoding/json"
+	"io"
+	"net/http"
+	"net/http/httptest"
+	"path/filepath"
+	"testing"
+
+	gitea "gitea.dev/sdk"
+	"github.com/stretchr/testify/require"
+	"github.com/urfave/cli/v3"
+
+	"gitea.dev/tea/modules/config"
+)
+
+// withMockServer spins up a minimal Gitea API stub and configures tea to
+// use it as the default login. The handler is called once for the version
+// probe (InitCommand → login.Client) and once per assertion the test wants
+// to make on the API path under test.
+func withMockServer(t *testing.T, handler http.HandlerFunc) string {
+	t.Helper()
+
+	server := httptest.NewServer(handler)
+	t.Cleanup(server.Close)
+
+	configPath := filepath.Join(t.TempDir(), "config.yml")
+	config.SetConfigPathForTesting(configPath)
+	t.Cleanup(func() {
+		config.SetConfigPathForTesting("")
+	})
+	config.SetConfigForTesting(config.LocalConfig{
+		Logins: []config.Login{{
+			Name:    "test",
+			URL:     server.URL,
+			Token:   "token",
+			User:    "user1",
+			Default: true,
+		}},
+	})
+	return server.URL
+}
+
+// runAction constructs a cli.Command that has the action replaced with a
+// no-op, then runs it through urfave/cli with the given arg list so flag
+// values and positional args are populated. The returned *cli.Command is
+// then passed to the real action function under test. This works around
+// urfave/cli's behavior where cmd.Run calls the action, which we don't
+// want — we want to inspect the parsed cmd, then call the real action
+// ourselves.
+//
+// All cross-cutting flags (--login, --repo, --output) must be passed via
+// `args` along with the command-specific ones.
+func runAction(t *testing.T, src cli.Command, args []string) *cli.Command {
+	t.Helper()
+	probe := src
+	probe.Action = func(_ context.Context, _ *cli.Command) error { return nil }
+	probe.Reader = bytes.NewReader(nil)
+	probe.Writer = io.Discard
+	probe.ErrWriter = io.Discard
+
+	fullArgs := append([]string{"test"}, args...)
+	require.NoError(t, probe.Run(t.Context(), fullArgs))
+	return &probe
+}
+
+// expectJSONBody asserts that the recorded request body decodes into the
+// expected value.
+func expectJSONBody(t *testing.T, raw []byte, want gitea.PullReviewRequestOptions) {
+	t.Helper()
+	var got gitea.PullReviewRequestOptions
+	require.NoError(t, json.Unmarshal(raw, &got))
+	require.Equal(t, want, got)
+}
+
+// flagNames returns the primary name of each cli.Flag in order.
+func flagNames(flags []cli.Flag) []string {
+	names := make([]string, 0, len(flags))
+	for _, f := range flags {
+		names = append(names, f.Names()[0])
+	}
+	return names
+}
--- a/cmd/pulls/request_review.go
+++ b/cmd/pulls/request_review.go
@ -5,15 +5,13 @@ package pulls

 import (
 	stdctx "context"
-	"errors"
 	"fmt"

-	gitea "gitea.dev/sdk"
 	"github.com/urfave/cli/v3"

 	"gitea.dev/tea/cmd/flags"
 	"gitea.dev/tea/modules/context"
-	"gitea.dev/tea/modules/utils"
+	"gitea.dev/tea/modules/task"
 )

 // ReviewerFlag is a CSV flag listing usernames to request review from.
@ -50,6 +48,10 @@ At least one of --reviewer or --team-reviewer is required.`,
 }

 func runRequestReview(requestCtx stdctx.Context, cmd *cli.Command) error {
+	args, err := parseReviewRequestArgs(requestCtx, cmd)
+	if err != nil {
+		return err
+	}
 	ctx, err := context.InitCommand(cmd)
 	if err != nil {
 		return err
@ -58,32 +60,14 @@ func runRequestReview(requestCtx stdctx.Context, cmd *cli.Command) error {
 		return err
 	}

-	reviewers, err := ReviewerFlag.GetValues(cmd)
-	if err != nil {
-		return err
-	}
-	teamReviewers, err := TeamReviewerFlag.GetValues(cmd)
-	if err != nil {
-		return err
-	}
-	if len(reviewers) == 0 && len(teamReviewers) == 0 {
-		return errors.New("at least one of --reviewer or --team-reviewer is required")
-	}
-
-	indices, err := utils.ArgsToIndices(cmd.Args().Slice())
-	if err != nil {
-		return err
-	}
-
 	client := ctx.Login.Client()
-	for _, idx := range indices {
-		if _, err := client.PullRequests.CreateReviewRequests(requestCtx, ctx.Owner, ctx.Repo, idx, gitea.PullReviewRequestOptions{
-			Reviewers:     reviewers,
-			TeamReviewers: teamReviewers,
-		}); err != nil {
+	for _, idx := range args.Indices {
+		if err := task.ApplyReviewerChanges(requestCtx, client, ctx.Owner, ctx.Repo, idx,
+			args.Reviewers, nil,
+			args.TeamReviewers, nil); err != nil {
 			return err
 		}
-		fmt.Printf("PR#%d: requested reviewers %v, teams %v\n", idx, reviewers, teamReviewers)
+		fmt.Printf("PR#%d: requested reviewers %v, teams %v\n", idx, args.Reviewers, args.TeamReviewers)
 	}
 	return nil
 }
--- a/cmd/pulls/request_review_test.go
+++ b/cmd/pulls/request_review_test.go
@ -4,71 +4,167 @@
 package pulls

 import (
-	"errors"
+	"context"
+	"io"
+	"net/http"
 	"testing"

+	gitea "gitea.dev/sdk"
 	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
 )

-func TestRequestReviewCommandMetadata(t *testing.T) {
-	cmd := &CmdPullsRequestReview
-
-	assert.Equal(t, "request-review", cmd.Name)
-	assert.Contains(t, cmd.Aliases, "rr")
-	assert.Equal(t, "Request reviews from users or teams on a pull request", cmd.Usage)
-	assert.Equal(t, "<pull index> [<pull index>...]", cmd.ArgsUsage)
-	assert.NotNil(t, cmd.Action)
-}
-
-func TestRequestReviewCommandFlags(t *testing.T) {
-	cmd := &CmdPullsRequestReview
-
-	expectedFlags := []string{
-		"reviewer",
-		"team-reviewer",
-	}
-
-	for _, flagName := range expectedFlags {
-		found := false
-		for _, flag := range cmd.Flags {
-			if flag.Names()[0] == flagName {
-				found = true
-				break
-			}
+func TestRunRequestReviewHappyPath(t *testing.T) {
+	var (
+		gotMethod string
+		gotPath   string
+		gotBody   []byte
+	)
+	repo := "owner/repo"
+	withMockServer(t, func(w http.ResponseWriter, r *http.Request) {
+		switch r.URL.Path {
+		case "/api/v1/version":
+			w.Header().Set("Content-Type", "application/json")
+			_, _ = w.Write([]byte(`{"version":"1.22.0"}`))
+		case "/api/v1/repos/" + repo + "/pulls/7/requested_reviewers":
+			gotMethod = r.Method
+			gotPath = r.URL.Path
+			gotBody, _ = io.ReadAll(r.Body)
+			w.WriteHeader(http.StatusNoContent)
+		default:
+			t.Errorf("unexpected request: %s %s", r.Method, r.URL.Path)
+			w.WriteHeader(http.StatusNotFound)
 		}
-		assert.True(t, found, "Expected flag %s not found", flagName)
-	}
+	})
+
+	cmd := runAction(t, CmdPullsRequestReview, []string{
+		"--login", "test",
+		"--repo", repo,
+		"--output", "json",
+		"--reviewer", "alice,bob",
+		"--team-reviewer", "devs",
+		"7",
+	})
+	require.NoError(t, runRequestReview(context.Background(), cmd))
+
+	assert.Equal(t, http.MethodPost, gotMethod)
+	assert.Equal(t, "/api/v1/repos/"+repo+"/pulls/7/requested_reviewers", gotPath)
+	expectJSONBody(t, gotBody, gitea.PullReviewRequestOptions{
+		Reviewers:     []string{"alice", "bob"},
+		TeamReviewers: []string{"devs"},
+	})
 }

-// TestRequestReviewValidation asserts the inline guard shared between
-// runRequestReview and runCancelReview: at least one of --reviewer or
-// --team-reviewer must be supplied.
-func TestRequestReviewValidation(t *testing.T) {
-	const msg = "at least one of --reviewer or --team-reviewer is required"
-	tests := []struct {
-		name      string
-		reviewers []string
-		teams     []string
-		wantErr   bool
-	}{
-		{name: "both empty", reviewers: nil, teams: nil, wantErr: true},
-		{name: "empty slice reviewers", reviewers: []string{}, teams: nil, wantErr: true},
-		{name: "reviewer only", reviewers: []string{"alice"}, teams: nil, wantErr: false},
-		{name: "team only", reviewers: nil, teams: []string{"devs"}, wantErr: false},
-		{name: "both set", reviewers: []string{"alice", "bob"}, teams: []string{"devs"}, wantErr: false},
-	}
-	for _, tt := range tests {
-		t.Run(tt.name, func(t *testing.T) {
-			var err error
-			if len(tt.reviewers) == 0 && len(tt.teams) == 0 {
-				err = errors.New(msg)
-			}
-			if tt.wantErr {
-				assert.Error(t, err)
-				assert.Equal(t, msg, err.Error())
-			} else {
-				assert.NoError(t, err)
-			}
-		})
-	}
+func TestRunRequestReviewAcceptsTeamOnly(t *testing.T) {
+	hits := 0
+	repo := "owner/repo"
+	withMockServer(t, func(w http.ResponseWriter, r *http.Request) {
+		switch r.URL.Path {
+		case "/api/v1/version":
+			w.Header().Set("Content-Type", "application/json")
+			_, _ = w.Write([]byte(`{"version":"1.22.0"}`))
+		case "/api/v1/repos/" + repo + "/pulls/1/requested_reviewers":
+			hits++
+			body, _ := io.ReadAll(r.Body)
+			expectJSONBody(t, body, gitea.PullReviewRequestOptions{
+				TeamReviewers: []string{"writers"},
+			})
+			w.WriteHeader(http.StatusNoContent)
+		default:
+			t.Errorf("unexpected request: %s %s", r.Method, r.URL.Path)
+			w.WriteHeader(http.StatusNotFound)
+		}
+	})
+
+	cmd := runAction(t, CmdPullsRequestReview, []string{
+		"--login", "test",
+		"--repo", repo,
+		"--output", "json",
+		"--team-reviewer", "writers",
+		"1",
+	})
+	require.NoError(t, runRequestReview(context.Background(), cmd))
+	assert.Equal(t, 1, hits, "expected exactly one POST to requested_reviewers")
+}
+
+func TestRunRequestReviewRejectsMissingFlags(t *testing.T) {
+	repo := "owner/repo"
+	withMockServer(t, func(w http.ResponseWriter, r *http.Request) {
+		if r.URL.Path != "/api/v1/version" {
+			t.Errorf("should not hit API when --reviewer/--team-reviewer are absent: %s %s", r.Method, r.URL.Path)
+			w.WriteHeader(http.StatusNotFound)
+			return
+		}
+		w.Header().Set("Content-Type", "application/json")
+		_, _ = w.Write([]byte(`{"version":"1.22.0"}`))
+	})
+
+	cmd := runAction(t, CmdPullsRequestReview, []string{
+		"--login", "test",
+		"--repo", repo,
+		"--output", "json",
+		"1",
+	})
+	err := runRequestReview(context.Background(), cmd)
+	require.Error(t, err)
+	assert.Contains(t, err.Error(), "at least one of --reviewer or --team-reviewer is required")
+}
+
+func TestRunRequestReviewPropagatesAPIError(t *testing.T) {
+	repo := "owner/repo"
+	withMockServer(t, func(w http.ResponseWriter, r *http.Request) {
+		switch r.URL.Path {
+		case "/api/v1/version":
+			w.Header().Set("Content-Type", "application/json")
+			_, _ = w.Write([]byte(`{"version":"1.22.0"}`))
+		case "/api/v1/repos/" + repo + "/pulls/1/requested_reviewers":
+			w.WriteHeader(http.StatusUnprocessableEntity)
+			_, _ = w.Write([]byte(`{"message":"user not found"}`))
+		default:
+			w.WriteHeader(http.StatusNotFound)
+		}
+	})
+
+	cmd := runAction(t, CmdPullsRequestReview, []string{
+		"--login", "test",
+		"--repo", repo,
+		"--output", "json",
+		"--reviewer", "ghost",
+		"1",
+	})
+	err := runRequestReview(context.Background(), cmd)
+	require.Error(t, err)
+	assert.Contains(t, err.Error(), "user not found")
+}
+
+func TestRunRequestReviewHitsAllIndices(t *testing.T) {
+	var hits []string
+	repo := "owner/repo"
+	withMockServer(t, func(w http.ResponseWriter, r *http.Request) {
+		switch {
+		case r.URL.Path == "/api/v1/version":
+			w.Header().Set("Content-Type", "application/json")
+			_, _ = w.Write([]byte(`{"version":"1.22.0"}`))
+		case r.URL.Path == "/api/v1/repos/"+repo+"/pulls/3/requested_reviewers",
+			r.URL.Path == "/api/v1/repos/"+repo+"/pulls/4/requested_reviewers":
+			hits = append(hits, r.URL.Path)
+			w.WriteHeader(http.StatusNoContent)
+		default:
+			t.Errorf("unexpected request: %s %s", r.Method, r.URL.Path)
+			w.WriteHeader(http.StatusNotFound)
+		}
+	})
+
+	cmd := runAction(t, CmdPullsRequestReview, []string{
+		"--login", "test",
+		"--repo", repo,
+		"--output", "json",
+		"--reviewer", "alice",
+		"3", "4",
+	})
+	require.NoError(t, runRequestReview(context.Background(), cmd))
+	assert.ElementsMatch(t, []string{
+		"/api/v1/repos/" + repo + "/pulls/3/requested_reviewers",
+		"/api/v1/repos/" + repo + "/pulls/4/requested_reviewers",
+	}, hits)
 }
--- a/cmd/pulls/review_helpers.go
+++ b/cmd/pulls/review_helpers.go
@ -20,6 +20,7 @@ import (
 	"gitea.dev/tea/modules/utils"

 	"charm.land/huh/v2"
+	"github.com/urfave/cli/v3"
 )

 // runPullReview handles the common logic for approving/rejecting pull requests
@ -126,3 +127,67 @@ func getCommentBody(ctx *context.TeaContext, extraArgs []string, promptTitle, no

 	return body, nil
 }
+
+// ReviewRequestArgs holds the parsed inputs shared by request-review and
+// cancel-review: the reviewers / team-reviewers to (un)request and the
+// PR indices they apply to.
+type ReviewRequestArgs struct {
+	Reviewers     []string
+	TeamReviewers []string
+	Indices       []int64
+}
+
+// nonEmptyValues returns v with empty / whitespace-only entries dropped.
+// Used to make the reviewer-request validation robust to CSV inputs that
+// contain stray commas ("alice,,bob") regardless of whether the underlying
+// CsvFlag already trimmed them. Returns nil (not an empty slice) when no
+// non-empty entries remain, so callers can use len(x) == 0 unambiguously.
+func nonEmptyValues(v []string) []string {
+	var out []string
+	for _, s := range v {
+		if strings.TrimSpace(s) == "" {
+			continue
+		}
+		out = append(out, s)
+	}
+	return out
+}
+
+// parseReviewRequestArgs resolves --reviewer / --team-reviewer / pull-index
+// positional args from cmd, validates that at least one of the two flags
+// is supplied, and returns a populated ReviewRequestArgs. Returns a wrapped
+// error suitable for direct return from a CLI Action.
+func parseReviewRequestArgs(requestCtx stdctx.Context, cmd *cli.Command) (*ReviewRequestArgs, error) {
+	ctx, err := context.InitCommand(cmd)
+	if err != nil {
+		return nil, err
+	}
+	if err := ctx.Ensure(context.CtxRequirement{RemoteRepo: true}); err != nil {
+		return nil, err
+	}
+
+	reviewers, err := ReviewerFlag.GetValues(cmd)
+	if err != nil {
+		return nil, err
+	}
+	teamReviewers, err := TeamReviewerFlag.GetValues(cmd)
+	if err != nil {
+		return nil, err
+	}
+	reviewers = nonEmptyValues(reviewers)
+	teamReviewers = nonEmptyValues(teamReviewers)
+	if len(reviewers) == 0 && len(teamReviewers) == 0 {
+		return nil, errors.New("at least one of --reviewer or --team-reviewer is required")
+	}
+
+	indices, err := utils.ArgsToIndices(cmd.Args().Slice())
+	if err != nil {
+		return nil, err
+	}
+
+	return &ReviewRequestArgs{
+		Reviewers:     reviewers,
+		TeamReviewers: teamReviewers,
+		Indices:       indices,
+	}, nil
+}
--- a/cmd/pulls/reviewers_test.go
+++ b/cmd/pulls/reviewers_test.go
@ -4,36 +4,71 @@
 package pulls

 import (
+	"context"
+	"net/http"
 	"testing"

 	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
 )

-func TestReviewersCommandMetadata(t *testing.T) {
-	cmd := &CmdPullsReviewers
-
-	assert.Equal(t, "reviewers", cmd.Name)
-	assert.Contains(t, cmd.Aliases, "rs")
-	assert.Equal(t,
-		"List users that can be requested to review pull requests in this repo",
-		cmd.Usage)
-	assert.NotNil(t, cmd.Action)
-}
-
-func TestReviewersCommandFlags(t *testing.T) {
-	cmd := &CmdPullsReviewers
-
-	// The reviewers subcommand inherits flags.AllDefaultFlags, which adds
-	// --login, --repo, --remote and --output. Verify those are wired.
-	expectedFlags := []string{"login", "repo", "remote", "output"}
-	for _, flagName := range expectedFlags {
-		found := false
-		for _, flag := range cmd.Flags {
-			if flag.Names()[0] == flagName {
-				found = true
-				break
-			}
+func TestRunReviewersListCallsGetReviewers(t *testing.T) {
+	repo := "owner/repo"
+	var (
+		gotMethod string
+		gotPath   string
+	)
+	withMockServer(t, func(w http.ResponseWriter, r *http.Request) {
+		switch r.URL.Path {
+		case "/api/v1/version":
+			w.Header().Set("Content-Type", "application/json")
+			_, _ = w.Write([]byte(`{"version":"1.22.0"}`))
+		case "/api/v1/repos/" + repo + "/reviewers":
+			gotMethod = r.Method
+			gotPath = r.URL.Path
+			w.Header().Set("Content-Type", "application/json")
+			_, _ = w.Write([]byte(`[
+				{"id":1,"login":"alice","full_name":"Alice","email":"alice@example.com"},
+				{"id":2,"login":"bob","full_name":"Bob","email":"bob@example.com"}
+			]`))
+		default:
+			t.Errorf("unexpected request: %s %s", r.Method, r.URL.Path)
+			w.WriteHeader(http.StatusNotFound)
 		}
-		assert.True(t, found, "Expected default flag %s not found", flagName)
-	}
+	})
+
+	cmd := runAction(t, CmdPullsReviewers, []string{
+		"--login", "test",
+		"--repo", repo,
+		"--output", "json",
+	})
+	require.NoError(t, runReviewersList(context.Background(), cmd))
+
+	assert.Equal(t, http.MethodGet, gotMethod)
+	assert.Equal(t, "/api/v1/repos/"+repo+"/reviewers", gotPath)
+}
+
+func TestRunReviewersListPropagatesAPIError(t *testing.T) {
+	repo := "owner/repo"
+	withMockServer(t, func(w http.ResponseWriter, r *http.Request) {
+		switch r.URL.Path {
+		case "/api/v1/version":
+			w.Header().Set("Content-Type", "application/json")
+			_, _ = w.Write([]byte(`{"version":"1.22.0"}`))
+		case "/api/v1/repos/" + repo + "/reviewers":
+			w.WriteHeader(http.StatusInternalServerError)
+			_, _ = w.Write([]byte(`{"message":"boom"}`))
+		default:
+			w.WriteHeader(http.StatusNotFound)
+		}
+	})
+
+	cmd := runAction(t, CmdPullsReviewers, []string{
+		"--login", "test",
+		"--repo", repo,
+		"--output", "json",
+	})
+	err := runReviewersList(context.Background(), cmd)
+	require.Error(t, err)
+	assert.Contains(t, err.Error(), "boom")
 }
--- a/modules/interact/pull_create.go
+++ b/modules/interact/pull_create.go
@ -137,10 +137,11 @@ func CreatePull(requestCtx stdctx.Context, ctx *context.TeaContext) (err error)
 	return task.CreatePull(
 		requestCtx,
 		ctx,
-		base,
-		head,
-		&allowMaintainerEdits,
-		&opts,
-		nil,
-		nil)
+		task.CreatePullOptions{
+			Base:                 base,
+			Head:                 head,
+			AllowMaintainerEdits: &allowMaintainerEdits,
+			Issue:                &opts,
+		},
+	)
 }
--- a/modules/task/labels.go
+++ b/modules/task/labels.go
@ -79,18 +79,22 @@ func ApplyLabelChanges(requestCtx stdctx.Context, client *gitea.Client, owner, r
 }

 // ApplyReviewerChanges adds and removes reviewers on a pull request.
-func ApplyReviewerChanges(requestCtx stdctx.Context, client *gitea.Client, owner, repo string, index int64, add, rm []string) error {
-	if len(rm) != 0 {
+// Both add/rm and teamAdd/teamRm may be empty; only the non-empty pairs
+// trigger the corresponding POST/DELETE call.
+func ApplyReviewerChanges(requestCtx stdctx.Context, client *gitea.Client, owner, repo string, index int64, add, rm []string, teamAdd, teamRm []string) error {
+	if len(rm) != 0 || len(teamRm) != 0 {
 		_, err := client.PullRequests.DeleteReviewRequests(requestCtx, owner, repo, index, gitea.PullReviewRequestOptions{
-			Reviewers: rm,
+			Reviewers:     rm,
+			TeamReviewers: teamRm,
 		})
 		if err != nil {
 			return fmt.Errorf("could not remove reviewers: %w", err)
 		}
 	}
-	if len(add) != 0 {
+	if len(add) != 0 || len(teamAdd) != 0 {
 		_, err := client.PullRequests.CreateReviewRequests(requestCtx, owner, repo, index, gitea.PullReviewRequestOptions{
-			Reviewers: add,
+			Reviewers:     add,
+			TeamReviewers: teamAdd,
 		})
 		if err != nil {
 			return fmt.Errorf("could not add reviewers: %w", err)
--- a/modules/task/pull_create.go
+++ b/modules/task/pull_create.go
@ -24,8 +24,24 @@ var (
 	consecutive = regexp.MustCompile(`[\s]{2,}`)
 )

+// CreatePullOptions bundles the inputs to CreatePull. The struct form keeps
+// call sites readable as the API surface grows (and avoids the
+// positional-arg drift that earlier PRs caused).
+type CreatePullOptions struct {
+	Base                 string
+	Head                 string
+	AllowMaintainerEdits *bool
+	Issue                *gitea.CreateIssueOption
+	Reviewers            []string
+	TeamReviewers        []string
+}
+
 // CreatePull creates a PR in the given repo and prints the result
-func CreatePull(requestCtx stdctx.Context, ctx *context.TeaContext, base, head string, allowMaintainerEdits *bool, opts *gitea.CreateIssueOption, reviewers, teamReviewers []string) (err error) {
+func CreatePull(requestCtx stdctx.Context, ctx *context.TeaContext, in CreatePullOptions) (err error) {
+	base := in.Base
+	head := in.Head
+	opts := in.Issue
+
 	// default is default branch
 	if len(base) == 0 {
 		base, err = GetDefaultPRBase(requestCtx, ctx.Login, ctx.Owner, ctx.Repo)
@ -69,8 +85,8 @@ func CreatePull(requestCtx stdctx.Context, ctx *context.TeaContext, base, head s
 		Title:         opts.Title,
 		Body:          opts.Body,
 		Assignees:     opts.Assignees,
-		Reviewers:     reviewers,
-		TeamReviewers: teamReviewers,
+		Reviewers:     in.Reviewers,
+		TeamReviewers: in.TeamReviewers,
 		Labels:        opts.Labels,
 		Milestone:     opts.Milestone,
 		Deadline:      opts.Deadline,
@ -79,9 +95,9 @@ func CreatePull(requestCtx stdctx.Context, ctx *context.TeaContext, base, head s
 		return fmt.Errorf("could not create PR from %s to %s:%s: %s", head, ctx.Owner, base, err)
 	}

-	if allowMaintainerEdits != nil && pr.AllowMaintainerEdit != *allowMaintainerEdits {
+	if in.AllowMaintainerEdits != nil && pr.AllowMaintainerEdit != *in.AllowMaintainerEdits {
 		pr, _, err = client.PullRequests.EditPullRequest(requestCtx, ctx.Owner, ctx.Repo, pr.Index, gitea.EditPullRequestOption{
-			AllowMaintainerEdit: allowMaintainerEdits,
+			AllowMaintainerEdit: in.AllowMaintainerEdits,
 		})
 		if err != nil {
 			return fmt.Errorf("could not enable maintainer edit on pull: %v", err)
--- a/modules/task/pull_edit.go
+++ b/modules/task/pull_edit.go
@ -67,7 +67,7 @@ func EditPull(requestCtx stdctx.Context, ctx *context.TeaContext, client *gitea.
 		return nil, err
 	}

-	if err := ApplyReviewerChanges(requestCtx, client, ctx.Owner, ctx.Repo, opts.Index, opts.AddReviewers, opts.RemoveReviewers); err != nil {
+	if err := ApplyReviewerChanges(requestCtx, client, ctx.Owner, ctx.Repo, opts.Index, opts.AddReviewers, opts.RemoveReviewers, nil, nil); err != nil {
 		return nil, err
 	}

--- a/tests/integration/pulls_reviewers_test.go
+++ b/tests/integration/pulls_reviewers_test.go
@ -0,0 +1,195 @@
+// Copyright 2026 The Gitea Authors. All rights reserved.
+// SPDX-License-Identifier: MIT
+
+package integration
+
+import (
+	"context"
+	"fmt"
+	"os/exec"
+	"strings"
+	"testing"
+	"time"
+
+	gitea "gitea.dev/sdk"
+	"gitea.dev/tea/cmd"
+	"github.com/stretchr/testify/require"
+)
+
+// TestRequestReviewFlow exercises the request-review, cancel-review, and
+// reviewers subcommands plus the create-time --reviewer/--team-reviewer
+// flags end-to-end against a real Gitea instance.
+//
+// It reuses the org/user/team scaffolding from TestEditPull_ModifiesAssignees
+// (writer team with members user1/user2) so PR creation, reviewer request,
+// and cancel all share the same fixture.
+func TestRequestReviewFlow(t *testing.T) {
+	login := createIntegrationLogin(t)
+	client := login.Client()
+	ctx := context.Background()
+
+	orgName := fmt.Sprintf("rr-org-%d", time.Now().UnixNano()%1_000_000)
+	orgRepoName := fmt.Sprintf("rr-repo-%d", time.Now().UnixNano()%1_000_000)
+
+	_, _ = client.Repositories.DeleteRepo(ctx, orgName, orgRepoName)
+	_, _ = client.Organizations.DeleteOrg(ctx, orgName)
+	_, _ = client.Admin.DeleteUser(ctx, "rr-user1")
+	_, _ = client.Admin.DeleteUser(ctx, "rr-user2")
+
+	_, _, err := client.Admin.CreateOrg(ctx, integrationUsername, gitea.CreateOrgOption{Name: orgName})
+	require.NoError(t, err)
+	t.Cleanup(func() {
+		if _, err := client.Organizations.DeleteOrg(ctx, orgName); err != nil {
+			t.Logf("failed to delete integration org %q: %v", orgName, err)
+		}
+	})
+
+	orgRepo, _, err := client.Repositories.CreateOrgRepo(ctx, orgName, gitea.CreateRepoOption{Name: orgRepoName})
+	require.NoError(t, err)
+	t.Cleanup(func() {
+		if _, err := client.Repositories.DeleteRepo(ctx, orgName, orgRepoName); err != nil {
+			t.Logf("failed to delete integration repo %q: %v", orgRepoName, err)
+		}
+	})
+
+	user1, _, err := client.Admin.CreateUser(ctx, gitea.CreateUserOption{Username: "rr-user1", Password: "rr-user1!1234", Email: "rr-user1@test.com"})
+	require.NoError(t, err)
+	_ = user1
+	_, _, err = client.Admin.CreateUser(ctx, gitea.CreateUserOption{Username: "rr-user2", Password: "rr-user2!1234", Email: "rr-user2@test.com"})
+	require.NoError(t, err)
+	t.Cleanup(func() {
+		_, _ = client.Admin.DeleteUser(ctx, "rr-user1")
+		_, _ = client.Admin.DeleteUser(ctx, "rr-user2")
+	})
+
+	team, _, err := client.Organizations.CreateTeam(ctx, orgName, gitea.CreateTeamOption{Name: "reviewers", Permission: gitea.AccessModeRead})
+	require.NoError(t, err)
+	_, err = client.Organizations.AddTeamMember(ctx, team.ID, "rr-user1")
+	require.NoError(t, err)
+	_, err = client.Organizations.AddTeamMember(ctx, team.ID, "rr-user2")
+	require.NoError(t, err)
+	_, err = client.Organizations.AddTeamRepository(ctx, team.ID, orgName, orgRepoName)
+	require.NoError(t, err)
+
+	// rr-user1/rr-user2 must be discoverable as PR reviewers via
+	// GET /repos/{owner}/{repo}/reviewers; this is also what the
+	// 'tea pulls reviewers' subcommand calls.
+	require.True(t, isReviewable(ctx, t, client, orgName, orgRepoName, "rr-user1"))
+	require.True(t, isReviewable(ctx, t, client, orgName, orgRepoName, "rr-user2"))
+
+	tmpDir := t.TempDir()
+	runGit := func(args ...string) {
+		t.Helper()
+		c := exec.Command("git", args...)
+		c.Dir = tmpDir
+		require.NoError(t, c.Run())
+	}
+	runGit("init")
+	runGit("config", "user.email", "rr@test.com")
+	runGit("config", "user.name", "rr")
+	httpsURL := fmt.Sprintf("%s/%s.git", login.URL, orgRepo.FullName)
+	httpsURL = strings.Replace(httpsURL, "://", fmt.Sprintf("://%s:%s@", login.Name, login.Token), 1)
+	runGit("remote", "add", "origin", httpsURL)
+
+	runGit("checkout", "-b", "main")
+	runGit("commit", "--allow-empty", "-m", "Initial commit")
+	runGit("push", "-u", "origin", "HEAD:main")
+	runGit("checkout", "-b", "feature-branch")
+	runGit("commit", "--allow-empty", "-m", "feature work")
+	runGit("push", "-u", "origin", "HEAD:feature-branch")
+	waitForBranches(t, orgRepo.FullName, "feature-branch")
+
+	fetchPR := func() *gitea.PullRequest {
+		t.Helper()
+		pr, _, err := client.PullRequests.GetPullRequest(ctx, orgName, orgRepoName, 1)
+		require.NoError(t, err)
+		return pr
+	}
+	reviewerNames := func(pr *gitea.PullRequest) []string {
+		names := make([]string, len(pr.RequestedReviewers))
+		for i, u := range pr.RequestedReviewers {
+			names[i] = u.UserName
+		}
+		return names
+	}
+	teamNames := func(pr *gitea.PullRequest) []string {
+		names := make([]string, len(pr.RequestedReviewersTeams))
+		for i, t := range pr.RequestedReviewersTeams {
+			names[i] = t.Name
+		}
+		return names
+	}
+
+	app := cmd.App()
+
+	// 1. create-time --reviewer / --team-reviewer should populate both lists.
+	err = app.Run(ctx, []string{
+		"tea", "pr", "create",
+		"--repo", orgRepo.FullName,
+		"--base", "main",
+		"--head", "feature-branch",
+		"--title", "reviewer test",
+		"--reviewer", "rr-user1",
+		"--team-reviewer", "reviewers",
+	})
+	require.NoError(t, err)
+	pr := fetchPR()
+	require.ElementsMatch(t, []string{"rr-user1"}, reviewerNames(pr))
+	require.ElementsMatch(t, []string{"reviewers"}, teamNames(pr))
+
+	// 2. request-review should add another user without disturbing existing reviewers.
+	err = app.Run(ctx, []string{
+		"tea", "pr", "request-review", "1",
+		"--repo", orgRepo.FullName,
+		"--reviewer", "rr-user2",
+	})
+	require.NoError(t, err)
+	pr = fetchPR()
+	require.ElementsMatch(t, []string{"rr-user1", "rr-user2"}, reviewerNames(pr))
+
+	// 3. cancel-review should remove the requested reviewer.
+	err = app.Run(ctx, []string{
+		"tea", "pr", "cancel-review", "1",
+		"--repo", orgRepo.FullName,
+		"--reviewer", "rr-user2",
+	})
+	require.NoError(t, err)
+	pr = fetchPR()
+	require.ElementsMatch(t, []string{"rr-user1"}, reviewerNames(pr))
+
+	// 4. cancel-review should also accept --team-reviewer.
+	err = app.Run(ctx, []string{
+		"tea", "pr", "cancel-review", "1",
+		"--repo", orgRepo.FullName,
+		"--team-reviewer", "reviewers",
+	})
+	require.NoError(t, err)
+	pr = fetchPR()
+	require.ElementsMatch(t, []string{}, teamNames(pr))
+	require.ElementsMatch(t, []string{"rr-user1"}, reviewerNames(pr))
+
+	// 5. request-review with neither --reviewer nor --team-reviewer should fail.
+	err = app.Run(ctx, []string{"tea", "pr", "request-review", "1", "--repo", orgRepo.FullName})
+	require.Error(t, err)
+	require.Contains(t, err.Error(), "at least one of --reviewer or --team-reviewer is required")
+
+	// 6. request-review against a non-existent PR should surface the API error.
+	err = app.Run(ctx, []string{
+		"tea", "pr", "request-review", "999",
+		"--repo", orgRepo.FullName,
+		"--reviewer", "rr-user1",
+	})
+	require.Error(t, err)
+}
+
+func isReviewable(ctx context.Context, t *testing.T, client *gitea.Client, owner, repo, user string) bool {
+	t.Helper()
+	reviewers, _, err := client.Repositories.GetReviewers(ctx, owner, repo)
+	require.NoError(t, err)
+	for _, u := range reviewers {
+		if u.UserName == user {
+			return true
+		}
+	}
+	return false
+}