fix(github): hide approve PR button on own PRs (#1017)

* fix(github): hide approve PR button on own PRs and reject self-approval server-side The frontend's existing self-author check failed open when the current GitHub user hadn't loaded yet — the PR panel didn't call useGitHubStatus, so the green Approve button flashed on every PR (including your own) during the brief window before /api/v1/github/status resolved. Fail closed when the user is unknown, fetch the status from the panel, and add a typed ErrSelfApprove guard in Service.SubmitReview so any path that bypasses the visibility check gets a clean 422 instead of an opaque upstream error. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * test(github): cover controller's self-approve 422 mapping Address PR review feedback: add TestHttpSubmitReview_SelfApproveReturns422 to exercise the controller → service → 422 path end-to-end so a future refactor can't silently reclassify ErrSelfApprove as a generic 500. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> --------- Co-authored-by: Kandev Agent <agent@kandev.dev> Co-authored-by: Claude Opus 4.7 <noreply@anthropic.com>
2026-05-22 17:10:38 +01:00
parent 1fb936fad5
commit b4a696a5e9
6 changed files with 289 additions and 5 deletions
@@ -297,12 +297,16 @@ func (c *Controller) httpSubmitReview(ctx *gin.Context) {
 		ctx.JSON(http.StatusBadRequest, gin.H{"error": "invalid payload"})
 		return
 	}
-	validEvents := map[string]bool{"APPROVE": true, "COMMENT": true, "REQUEST_CHANGES": true}
+	validEvents := map[string]bool{reviewEventApprove: true, "COMMENT": true, "REQUEST_CHANGES": true}
 	if !validEvents[req.Event] {
 		ctx.JSON(http.StatusBadRequest, gin.H{"error": "event must be APPROVE, COMMENT, or REQUEST_CHANGES"})
 		return
 	}
 	if err := c.service.SubmitReview(ctx.Request.Context(), owner, repo, number, req.Event, req.Body); err != nil {
+		if errors.Is(err, ErrSelfApprove) {
+			ctx.JSON(http.StatusUnprocessableEntity, gin.H{"error": err.Error()})
+			return
+		}
 		ctx.JSON(http.StatusInternalServerError, gin.H{"error": err.Error()})
 		return
 	}
@@ -431,6 +431,40 @@ func TestHttpMergePR_NoClient_Returns503(t *testing.T) {
 	}
 }

+// TestHttpSubmitReview_SelfApproveReturns422 exercises the full controller →
+// service guard → HTTP mapping for the ErrSelfApprove path. Guards against a
+// future refactor accidentally reclassifying the typed error as 500, which
+// would mask "you can't approve your own PR" as an opaque upstream failure.
+func TestHttpSubmitReview_SelfApproveReturns422(t *testing.T) {
+	sc := &stubClient{
+		// stubClient.GetAuthenticatedUser returns "test-user"; matching
+		// AuthorLogin triggers the self-approve guard inside SubmitReview.
+		getPRFunc: func(_ context.Context, _, _ string, _ int) (*PR, error) {
+			return &PR{Number: 42, AuthorLogin: "test-user", State: "open"}, nil
+		},
+	}
+	router, _ := setupControllerTest(sc)
+
+	body := bytes.NewBufferString(`{"event":"APPROVE"}`)
+	req := httptest.NewRequest(http.MethodPost, "/api/v1/github/prs/acme/widget/42/reviews", body)
+	req.Header.Set("Content-Type", "application/json")
+	w := httptest.NewRecorder()
+	router.ServeHTTP(w, req)
+
+	if w.Code != http.StatusUnprocessableEntity {
+		t.Fatalf("expected 422, got %d: %s", w.Code, w.Body.String())
+	}
+	var resp struct {
+		Error string `json:"error"`
+	}
+	if err := json.NewDecoder(w.Body).Decode(&resp); err != nil {
+		t.Fatalf("decode body: %v", err)
+	}
+	if resp.Error != ErrSelfApprove.Error() {
+		t.Errorf("expected error %q, got %q", ErrSelfApprove.Error(), resp.Error)
+	}
+}
+
 func TestHttpMergePR_Conflict(t *testing.T) {
 	sc := &stubClient{
 		mergePRFn: func(context.Context, string, string, int, string) error {
@@ -47,6 +47,17 @@ const (
 // the case without string-matching the underlying error message.
 var ErrTaskNotFound = errors.New("github: task not found for cleanup")

+// ErrSelfApprove is returned by SubmitReview when the authenticated user
+// attempts to APPROVE their own PR. GitHub rejects this with a 422; we
+// catch it server-side so the UI sees a clean, typed error rather than a
+// generic upstream failure when the frontend's visibility guard is bypassed.
+var ErrSelfApprove = errors.New("cannot approve your own pull request")
+
+// reviewEventApprove is the GitHub Reviews API event value for a positive
+// review. Extracted because it appears in the controller validator, the
+// service's self-approval guard, and tests.
+const reviewEventApprove = "APPROVE"
+
 // TaskDeleter deletes tasks by ID. Used for cleaning up merged PR tasks.
 // Implementations should return errors wrapping ErrTaskNotFound when the
 // task is already gone.
@@ -425,11 +436,25 @@ func (s *Service) ClearToken(ctx context.Context) error {
 	return nil
 }

-// SubmitReview submits a review on a pull request.
+// SubmitReview submits a review on a pull request. For APPROVE events it
+// first checks that the authenticated user is not the PR author, returning
+// ErrSelfApprove instead of letting the request fail at GitHub with an opaque
+// 422. Lookup failures here are non-fatal — we fall through to GitHub so a
+// transient API hiccup doesn't block legitimate approvals.
 func (s *Service) SubmitReview(ctx context.Context, owner, repo string, number int, event, body string) error {
 	if s.client == nil {
 		return fmt.Errorf("github client not configured")
 	}
+	if event == reviewEventApprove {
+		user, userErr := s.client.GetAuthenticatedUser(ctx)
+		if userErr == nil && user != "" {
+			pr, prErr := s.client.GetPR(ctx, owner, repo, number)
+			if prErr == nil && pr != nil &&
+				strings.EqualFold(strings.TrimSpace(pr.AuthorLogin), strings.TrimSpace(user)) {
+				return ErrSelfApprove
+			}
+		}
+	}
 	return s.client.SubmitReview(ctx, owner, repo, number, event, body)
 }

@@ -0,0 +1,80 @@
+package github
+
+import (
+	"context"
+	"errors"
+	"testing"
+)
+
+// TestService_SubmitReview_RejectsSelfApprove verifies the server-side guard
+// against approving your own PR. GitHub returns a 422 in that case; we catch
+// it server-side so the UI gets a typed error rather than an opaque upstream
+// failure when the frontend's visibility guard is bypassed (e.g. a stale
+// page that didn't see the author update).
+func TestService_SubmitReview_RejectsSelfApprove(t *testing.T) {
+	client := NewMockClient()
+	client.SetUser("octocat")
+	client.AddPR(&PR{
+		Number:      7,
+		RepoOwner:   "octocat",
+		RepoName:    "hello",
+		AuthorLogin: "OCTOCAT", // case-insensitive match
+		State:       "open",
+	})
+	svc := newTestService(client)
+
+	err := svc.SubmitReview(context.Background(), "octocat", "hello", 7, "APPROVE", "")
+	if !errors.Is(err, ErrSelfApprove) {
+		t.Fatalf("expected ErrSelfApprove, got %v", err)
+	}
+	if len(client.SubmittedReviews()) != 0 {
+		t.Fatalf("expected no review submitted, got %d", len(client.SubmittedReviews()))
+	}
+}
+
+// TestService_SubmitReview_AllowsOthersPR verifies the guard only blocks the
+// self-approval case — approving someone else's PR must still flow through.
+func TestService_SubmitReview_AllowsOthersPR(t *testing.T) {
+	client := NewMockClient()
+	client.SetUser("reviewer")
+	client.AddPR(&PR{
+		Number:      8,
+		RepoOwner:   "octocat",
+		RepoName:    "hello",
+		AuthorLogin: "octocat",
+		State:       "open",
+	})
+	svc := newTestService(client)
+
+	if err := svc.SubmitReview(context.Background(), "octocat", "hello", 8, "APPROVE", ""); err != nil {
+		t.Fatalf("SubmitReview: %v", err)
+	}
+	reviews := client.SubmittedReviews()
+	if len(reviews) != 1 || reviews[0].Event != "APPROVE" {
+		t.Fatalf("expected one APPROVE review, got %#v", reviews)
+	}
+}
+
+// TestService_SubmitReview_CommentOnOwnPRAllowed verifies the guard is
+// scoped to APPROVE — leaving a COMMENT or REQUEST_CHANGES review on your
+// own PR is allowed by GitHub and must not be blocked.
+func TestService_SubmitReview_CommentOnOwnPRAllowed(t *testing.T) {
+	client := NewMockClient()
+	client.SetUser("octocat")
+	client.AddPR(&PR{
+		Number:      9,
+		RepoOwner:   "octocat",
+		RepoName:    "hello",
+		AuthorLogin: "octocat",
+		State:       "open",
+	})
+	svc := newTestService(client)
+
+	if err := svc.SubmitReview(context.Background(), "octocat", "hello", 9, "COMMENT", "looks ok"); err != nil {
+		t.Fatalf("SubmitReview: %v", err)
+	}
+	reviews := client.SubmittedReviews()
+	if len(reviews) != 1 || reviews[0].Event != "COMMENT" {
+		t.Fatalf("expected one COMMENT review, got %#v", reviews)
+	}
+}
@@ -0,0 +1,131 @@
+import { describe, it, expect } from "vitest";
+import { shouldHideApproveButton } from "./pr-detail-panel";
+import type { TaskPR, PRFeedback, GitHubPR, PRReview } from "@/lib/types/github";
+
+function makeTaskPR(overrides: Partial<TaskPR> = {}): TaskPR {
+  return {
+    id: "id",
+    task_id: "task",
+    owner: "o",
+    repo: "r",
+    pr_number: 1,
+    pr_url: "",
+    pr_title: "Test PR",
+    head_branch: "feat",
+    base_branch: "main",
+    author_login: "alice",
+    state: "open",
+    review_state: "",
+    checks_state: "",
+    mergeable_state: "",
+    review_count: 0,
+    pending_review_count: 0,
+    comment_count: 0,
+    unresolved_review_threads: 0,
+    checks_total: 0,
+    checks_passing: 0,
+    additions: 0,
+    deletions: 0,
+    created_at: "",
+    merged_at: null,
+    closed_at: null,
+    last_synced_at: null,
+    updated_at: "",
+    ...overrides,
+  };
+}
+
+function makeGitHubPR(overrides: Partial<GitHubPR> = {}): GitHubPR {
+  return {
+    number: 1,
+    title: "Test PR",
+    url: "",
+    html_url: "",
+    state: "open",
+    head_branch: "feat",
+    base_branch: "main",
+    author_login: "alice",
+    repo_owner: "o",
+    repo_name: "r",
+    draft: false,
+    mergeable: true,
+    additions: 0,
+    deletions: 0,
+    ...overrides,
+  } as GitHubPR;
+}
+
+function makeFeedback(
+  overrides: {
+    pr?: Partial<GitHubPR>;
+    reviews?: PRReview[];
+  } = {},
+): PRFeedback {
+  return {
+    pr: makeGitHubPR(overrides.pr),
+    reviews: overrides.reviews ?? [],
+    comments: [],
+    checks: [],
+    has_issues: false,
+  };
+}
+
+describe("shouldHideApproveButton", () => {
+  it("hides when PR is closed", () => {
+    expect(shouldHideApproveButton(makeTaskPR({ state: "closed" }), null, "bob")).toBe(true);
+  });
+
+  it("hides when PR is merged", () => {
+    expect(shouldHideApproveButton(makeTaskPR({ state: "merged" }), null, "bob")).toBe(true);
+  });
+
+  // Regression: pre-fix this returned false (button shown), so the green
+  // Approve button appeared on every PR — including the viewer's own — during
+  // the brief window before /api/v1/github/status resolved client-side.
+  it("hides when current user is unknown (status not loaded yet)", () => {
+    expect(shouldHideApproveButton(makeTaskPR({ author_login: "alice" }), null, null)).toBe(true);
+    expect(shouldHideApproveButton(makeTaskPR({ author_login: "alice" }), null, "")).toBe(true);
+    expect(shouldHideApproveButton(makeTaskPR({ author_login: "alice" }), null, "   ")).toBe(true);
+  });
+
+  it("hides when current user authored the PR (case-insensitive)", () => {
+    expect(shouldHideApproveButton(makeTaskPR({ author_login: "Alice" }), null, "alice")).toBe(
+      true,
+    );
+    expect(shouldHideApproveButton(makeTaskPR({ author_login: "alice" }), null, "ALICE")).toBe(
+      true,
+    );
+  });
+
+  it("hides when current user has already approved", () => {
+    const feedback = makeFeedback({
+      pr: { author_login: "alice" },
+      reviews: [
+        {
+          id: 1,
+          author: "bob",
+          author_avatar: "",
+          state: "APPROVED",
+          body: "",
+          created_at: "",
+        },
+      ],
+    });
+    expect(shouldHideApproveButton(makeTaskPR({ author_login: "alice" }), feedback, "bob")).toBe(
+      true,
+    );
+  });
+
+  it("shows when current user is a different open reviewer", () => {
+    expect(shouldHideApproveButton(makeTaskPR({ author_login: "alice" }), null, "bob")).toBe(false);
+  });
+
+  it("prefers feedback.pr.author_login over taskPR.author_login when both present", () => {
+    // taskPR may be stale; live feedback wins. Here the stored author looks
+    // like a different user but feedback says it's actually us — must hide.
+    const feedback = makeFeedback({ pr: { author_login: "bob" } });
+    expect(shouldHideApproveButton(makeTaskPR({ author_login: "alice" }), feedback, "bob")).toBe(
+      true,
+    );
+  });
+});
@@ -20,6 +20,7 @@ import { useAppStore } from "@/components/state-provider";
 import { useActiveTaskPR, useTaskPR } from "@/hooks/domains/github/use-task-pr";
 import { prPanelLabel } from "@/components/github/pr-utils";
 import { usePRFeedback } from "@/hooks/domains/github/use-pr-feedback";
+import { useGitHubStatus } from "@/hooks/domains/github/use-github-status";
 import { useCommentsStore } from "@/lib/state/slices/comments";
 import type { PRFeedbackComment } from "@/lib/state/slices/comments";
 import { useToast } from "@/components/toast-provider";
@@ -174,7 +175,11 @@ function DescriptionSection({ body }: { body: string }) {
 }

 // GitHub logins are case-insensitive; normalize before comparing.
-function shouldHideApproveButton(
+// Fails closed when the current user is unknown — without that identity we
+// can't tell whether the viewer is the PR author, and GitHub rejects
+// self-approval, so the button would only ever produce a failed request.
+// Exported for unit testing.
+export function shouldHideApproveButton(
  taskPR: TaskPR,
  feedback: PRFeedback | null,
  currentUser: string | null,
@@ -182,7 +187,7 @@ function shouldHideApproveButton(
  const liveState = feedback?.pr.state ?? taskPR.state;
  if (liveState !== "open") return true;
  const normalizedUser = currentUser?.trim().toLowerCase();
-  if (!normalizedUser) return false;
+  if (!normalizedUser) return true;
  const prAuthor = feedback?.pr.author_login ?? taskPR.author_login;
  if (prAuthor?.trim().toLowerCase() === normalizedUser) return true;
  return (
@@ -203,7 +208,12 @@ function ApproveButton({
 }) {
  const { toast } = useToast();
  const [submitting, setSubmitting] = useState(false);
-  const currentUser = useAppStore((s) => s.githubStatus.status?.username ?? null);
+  // Ensures status (and thus the authenticated username) is fetched even when
+  // the PR panel is the first GitHub-aware surface the user opens; without
+  // this, currentUser is null on first render and shouldHideApproveButton has
+  // no identity to compare against the PR author.
+  const { status } = useGitHubStatus();
+  const currentUser = status?.username ?? null;

  if (shouldHideApproveButton(taskPR, feedback, currentUser)) return null;