diff --git a/internal/cli/postreview.go b/internal/cli/postreview.go index eb9be86eb..3257693fc 100644 --- a/internal/cli/postreview.go +++ b/internal/cli/postreview.go @@ -3,7 +3,9 @@ package cli import ( "context" "encoding/json" + "errors" "fmt" + "net/http" "os" "regexp" "strconv" @@ -61,12 +63,17 @@ has moved, a stale-head failure is posted instead.`, RunE: func(cmd *cobra.Command, args []string) error { printer := ui.New(os.Stdout) + tokenFallback := false if token == "" { token = os.Getenv("GITHUB_TOKEN") + tokenFallback = true } if token == "" { return fmt.Errorf("--token or GITHUB_TOKEN required") } + if tokenFallback { + printer.StepWarn("No --token provided, falling back to GITHUB_TOKEN; self-review 422 errors may occur if this token shares identity with the PR author") + } if pr <= 0 { return fmt.Errorf("--pr must be a positive integer, got %d", pr) @@ -357,12 +364,46 @@ func submitFormalReview(ctx context.Context, client forge.Client, owner, repo st printer.StepInfo(fmt.Sprintf("Attaching %d inline comment(s)", len(inlineComments))) } if err := client.CreatePullRequestReview(ctx, owner, repo, pr, event, reviewBody, commitSHA, inlineComments); err != nil { + if isSelfReviewError(err) { + return fmt.Errorf("submitting review: %w — the token identity matches the PR author; "+ + "set REVIEW_TOKEN (in post-review.sh) or pass --token with a minted token "+ + "that has pull-requests:write permission so the review is submitted as a different identity", err) + } return fmt.Errorf("submitting review: %w", err) } printer.StepDone("Review submitted") return nil } +// isSelfReviewError checks whether an error from CreatePullRequestReview is +// a 422 caused by the authenticated user attempting to review their own PR. +// GitHub rejects this with a validation error. Detecting it lets us surface +// a clear message pointing operators to use a minted REVIEW_TOKEN instead of +// the default GITHUB_TOKEN. +func isSelfReviewError(err error) bool { + var apiErr *gh.APIError + if !errors.As(err, &apiErr) { + return false + } + if apiErr.StatusCode != http.StatusUnprocessableEntity { + return false + } + // GitHub's error message for self-review attempts varies, but the + // combination of 422 + validation message mentioning "review" on a + // review submission call is sufficient. Check both the top-level + // message and detail messages. + combined := strings.ToLower(apiErr.Message) + for _, d := range apiErr.Errors { + combined += " " + strings.ToLower(d.Message) + } + // ponytail: matches GitHub's "Can not approve your own pull request" + // and similar phrasings. Tightened to avoid false positives on + // unrelated 422s (e.g. "cannot request changes on a draft"). + return strings.Contains(combined, "your own") || + strings.Contains(combined, "can not approve") || + strings.Contains(combined, "cannot review") +} + // findingsToReviewComments converts review findings with file and line // locations into inline review comments. Findings without a file path // or line number are omitted — they remain in the sticky comment body. diff --git a/internal/cli/postreview_test.go b/internal/cli/postreview_test.go index 05b7866ca..29e58b4ac 100644 --- a/internal/cli/postreview_test.go +++ b/internal/cli/postreview_test.go @@ -4,12 +4,14 @@ import ( "bytes" "context" "fmt" - "github.com/stretchr/testify/assert" - "github.com/stretchr/testify/require" "io" "testing" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "github.com/fullsend-ai/fullsend/internal/forge" + gh "github.com/fullsend-ai/fullsend/internal/forge/github" "github.com/fullsend-ai/fullsend/internal/sticky" "github.com/fullsend-ai/fullsend/internal/ui" ) @@ -1001,3 +1003,47 @@ func TestPostApprovedFollowUpIssues_DisabledIsNoop(t *testing.T) { err := postApprovedFollowUpIssues(context.Background(), "acme", "repo", 9, parsed, printer) require.NoError(t, err) } + +func TestIsSelfReviewError(t *testing.T) { + tests := []struct { + name string + err error + want bool + }{ + { + name: "GitHub self-review 422", + err: fmt.Errorf("create pull request review on #42: %w", &gh.APIError{StatusCode: 422, Message: "Validation Failed", Errors: []gh.APIErrorDetail{{Message: "Can not approve your own pull request"}}}), + want: true, + }, + { + name: "alternate wording cannot", + err: &gh.APIError{StatusCode: 422, Message: "You cannot review your own pull request"}, + want: true, + }, + { + name: "non-422 error", + err: &gh.APIError{StatusCode: 403, Message: "Resource not accessible by integration"}, + want: false, + }, + { + name: "422 unrelated validation", + err: &gh.APIError{StatusCode: 422, Message: "Validation Failed", Errors: []gh.APIErrorDetail{{Message: "commit_id is not part of the pull request"}}}, + want: false, + }, + { + name: "422 draft PR cannot request changes", + err: &gh.APIError{StatusCode: 422, Message: "You cannot request changes on a draft pull request"}, + want: false, + }, + { + name: "non-API error", + err: fmt.Errorf("network timeout"), + want: false, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + assert.Equal(t, tt.want, isSelfReviewError(tt.err)) + }) + } +} diff --git a/internal/scaffold/fullsend-repo/scripts/post-review.sh b/internal/scaffold/fullsend-repo/scripts/post-review.sh index 955c64de1..c05240e22 100755 --- a/internal/scaffold/fullsend-repo/scripts/post-review.sh +++ b/internal/scaffold/fullsend-repo/scripts/post-review.sh @@ -8,8 +8,8 @@ # if the PR touches sensitive paths, an "approve" action is downgraded # to "comment" so only a human can grant approval. # -# Required environment variables: -# REVIEW_TOKEN — token with pull-requests:write on the target repo +# Environment variables: +# REVIEW_TOKEN — (optional) token with pull-requests:write; falls back to GITHUB_TOKEN # PR_NUMBER — GitHub PR number # REPO_FULL_NAME — owner/repo (e.g. my-org/my-repo) # @@ -18,7 +18,12 @@ # 1 — error (review not posted or fallback comment posted) set -euo pipefail -: "${REVIEW_TOKEN:?REVIEW_TOKEN is required}" +if [[ -z "${REVIEW_TOKEN:-}" ]]; then + # ponytail: fall back to GITHUB_TOKEN with a warning instead of hard-failing. + # The Go CLI detects self-review 422s and gives a clear error message. + echo "::warning::REVIEW_TOKEN is not set; falling back to GITHUB_TOKEN — self-review 422 errors may occur if this token shares identity with the PR author" + REVIEW_TOKEN="${GITHUB_TOKEN:?Neither REVIEW_TOKEN nor GITHUB_TOKEN is set}" +fi : "${PR_NUMBER:?PR_NUMBER is required}" if ! [[ "${PR_NUMBER}" =~ ^[0-9]+$ ]]; then echo "::error::PR_NUMBER must be a positive integer"