diff --git a/apps/backend/internal/github/controller.go b/apps/backend/internal/github/controller.go index e2d550a91a..e9045d5288 100644 --- a/apps/backend/internal/github/controller.go +++ b/apps/backend/internal/github/controller.go @@ -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 } diff --git a/apps/backend/internal/github/controller_test.go b/apps/backend/internal/github/controller_test.go index f5a0b2a159..9fc5fa85f2 100644 --- a/apps/backend/internal/github/controller_test.go +++ b/apps/backend/internal/github/controller_test.go @@ -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 { diff --git a/apps/backend/internal/github/service.go b/apps/backend/internal/github/service.go index b6debd6507..bba3aeffbd 100644 --- a/apps/backend/internal/github/service.go +++ b/apps/backend/internal/github/service.go @@ -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) } diff --git a/apps/backend/internal/github/service_submit_review_test.go b/apps/backend/internal/github/service_submit_review_test.go new file mode 100644 index 0000000000..c6e2122b64 --- /dev/null +++ b/apps/backend/internal/github/service_submit_review_test.go @@ -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) + } +} diff --git a/apps/web/components/github/pr-detail-panel.test.ts b/apps/web/components/github/pr-detail-panel.test.ts new file mode 100644 index 0000000000..bfb8d56a39 --- /dev/null +++ b/apps/web/components/github/pr-detail-panel.test.ts @@ -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 { + 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 { + 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; + 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, + ); + }); +}); diff --git a/apps/web/components/github/pr-detail-panel.tsx b/apps/web/components/github/pr-detail-panel.tsx index 3cd69d8abb..a19b982fb9 100644 --- a/apps/web/components/github/pr-detail-panel.tsx +++ b/apps/web/components/github/pr-detail-panel.tsx @@ -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;