diff --git a/internal/cli/postcomment.go b/internal/cli/postcomment.go new file mode 100644 index 0000000000..af745e3bf0 --- /dev/null +++ b/internal/cli/postcomment.go @@ -0,0 +1,85 @@ +package cli + +import ( + "fmt" + "os" + "strings" + + "github.com/spf13/cobra" + + gh "github.com/fullsend-ai/fullsend/internal/forge/github" + "github.com/fullsend-ai/fullsend/internal/sticky" + "github.com/fullsend-ai/fullsend/internal/ui" +) + +func newPostCommentCmd() *cobra.Command { + var ( + repo string + number int + marker string + result string + token string + dryRun bool + ) + + cmd := &cobra.Command{ + Use: "post-comment", + Short: "Post or update a sticky comment on an issue or PR", + Long: `Posts a comment with a hidden HTML marker on an issue or pull request. + +On first run, creates a new comment. On re-runs, finds the existing +comment by its marker and edits in-place, collapsing old content into +
blocks. This prevents comment flooding on re-runs. + +The --marker flag identifies this agent's comments. Each agent should +use a unique marker (e.g. ""). + +The --result flag accepts a file path or "-" for stdin.`, + RunE: func(cmd *cobra.Command, args []string) error { + printer := ui.New(os.Stdout) + + if token == "" { + token = os.Getenv("GITHUB_TOKEN") + } + if token == "" { + return fmt.Errorf("--token or GITHUB_TOKEN required") + } + + if number <= 0 { + return fmt.Errorf("--number must be a positive integer, got %d", number) + } + + parts := strings.SplitN(repo, "/", 2) + if len(parts) != 2 || parts[0] == "" || parts[1] == "" { + return fmt.Errorf("--repo must be in owner/repo format, got %q", repo) + } + owner, repoName := parts[0], parts[1] + + body, err := readBody(result) + if err != nil { + return fmt.Errorf("reading comment body: %w", err) + } + + printer.Header("Post Comment") + + client := gh.New(token) + cfg := sticky.Config{ + Marker: marker, + DryRun: dryRun, + } + return sticky.Post(cmd.Context(), client, owner, repoName, number, body, cfg, printer) + }, + } + + cmd.Flags().StringVar(&repo, "repo", "", "repository in owner/repo format (required)") + cmd.Flags().IntVar(&number, "number", 0, "issue or pull request number (required)") + cmd.Flags().StringVar(&marker, "marker", "", "hidden HTML marker to identify this agent's comments (required)") + cmd.Flags().StringVar(&result, "result", "-", "path to comment body file, or '-' for stdin") + cmd.Flags().StringVar(&token, "token", "", "GitHub token (default: $GITHUB_TOKEN)") + cmd.Flags().BoolVar(&dryRun, "dry-run", false, "print what would be posted without making API calls") + _ = cmd.MarkFlagRequired("repo") + _ = cmd.MarkFlagRequired("number") + _ = cmd.MarkFlagRequired("marker") + + return cmd +} diff --git a/internal/cli/postcomment_test.go b/internal/cli/postcomment_test.go new file mode 100644 index 0000000000..fcbd186bc6 --- /dev/null +++ b/internal/cli/postcomment_test.go @@ -0,0 +1,30 @@ +package cli + +import ( + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestNewPostCommentCmd_RequiredFlags(t *testing.T) { + cmd := newPostCommentCmd() + + // Verify required flags are registered. + for _, name := range []string{"repo", "number", "marker"} { + f := cmd.Flags().Lookup(name) + require.NotNil(t, f, "flag %q should exist", name) + } +} + +func TestNewPostCommentCmd_DefaultFlags(t *testing.T) { + cmd := newPostCommentCmd() + + result := cmd.Flags().Lookup("result") + require.NotNil(t, result) + assert.Equal(t, "-", result.DefValue) + + dryRun := cmd.Flags().Lookup("dry-run") + require.NotNil(t, dryRun) + assert.Equal(t, "false", dryRun.DefValue) +} diff --git a/internal/cli/postreview.go b/internal/cli/postreview.go new file mode 100644 index 0000000000..d360d6a862 --- /dev/null +++ b/internal/cli/postreview.go @@ -0,0 +1,314 @@ +package cli + +import ( + "context" + "encoding/json" + "fmt" + "os" + "regexp" + "strings" + + "github.com/spf13/cobra" + + "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" +) + +const reviewMarker = "" + +var hexSHARe = regexp.MustCompile(`^[0-9a-fA-F]{40}$|^[0-9a-fA-F]{64}$`) +var reasonRe = regexp.MustCompile(`^[a-zA-Z0-9_-]*$`) + +func newPostReviewCmd() *cobra.Command { + var ( + repo string + pr int + result string + token string + headSHA string + dryRun bool + ) + + cmd := &cobra.Command{ + Use: "post-review", + Short: "Post or update a sticky review comment on a PR", + Long: `Posts review findings as a sticky issue comment on a pull request, +then submits a formal GitHub PR review with the disposition. + +On first run, creates a new comment with a hidden HTML marker. +On re-runs, finds the existing comment, collapses old content into +a
block, and edits in-place. Stale formal reviews by the +same user are minimized before submitting a new one. + +The --result flag accepts a file path containing a JSON review result +(with action, body, and optionally head_sha fields), or reads from +stdin if set to "-". Plain text input is treated as a comment-only +review. + +When --head-sha is provided (or head_sha is in the JSON), the CLI +verifies that the PR HEAD still matches before posting. If the HEAD +has moved, a stale-head failure is posted instead.`, + RunE: func(cmd *cobra.Command, args []string) error { + printer := ui.New(os.Stdout) + + if token == "" { + token = os.Getenv("GITHUB_TOKEN") + } + if token == "" { + return fmt.Errorf("--token or GITHUB_TOKEN required") + } + + if pr <= 0 { + return fmt.Errorf("--pr must be a positive integer, got %d", pr) + } + + parts := strings.SplitN(repo, "/", 2) + if len(parts) != 2 || parts[0] == "" || parts[1] == "" { + return fmt.Errorf("--repo must be in owner/repo format, got %q", repo) + } + owner, repoName := parts[0], parts[1] + + raw, err := readBody(result) + if err != nil { + return fmt.Errorf("reading review body: %w", err) + } + + parsed, err := parseReviewResult(raw) + if err != nil { + return fmt.Errorf("parsing review result: %w", err) + } + + // CLI flag takes precedence over JSON field. + if headSHA != "" { + parsed.HeadSHA = headSHA + } + if parsed.HeadSHA != "" && !hexSHARe.MatchString(parsed.HeadSHA) { + return fmt.Errorf("head SHA must be a 40 or 64 character hex string, got %q", parsed.HeadSHA) + } + + printer.Header("Post Review") + + client := gh.New(token) + cfg := sticky.Config{ + Marker: reviewMarker, + DryRun: dryRun, + } + + // Stale-head check: refuse to post a review against code + // that has changed since the agent reviewed it. + if parsed.HeadSHA != "" { + stale, currentSHA, err := checkStaleHead(cmd.Context(), client, owner, repoName, pr, parsed.HeadSHA, dryRun, printer) + if err != nil { + return err + } + if stale { + return postStaleHeadNotice(cmd.Context(), client, owner, repoName, pr, parsed.HeadSHA, currentSHA, cfg, printer) + } + } + + // Failure action: post a failure notice as a sticky comment, + // skip formal review. + if strings.ToLower(parsed.Action) == "failure" { + return postFailureNotice(cmd.Context(), client, owner, repoName, pr, parsed, cfg, printer) + } + + if err := sticky.Post(cmd.Context(), client, owner, repoName, pr, parsed.Body, cfg, printer); err != nil { + return err + } + + return submitFormalReview(cmd.Context(), client, owner, repoName, pr, parsed.Action, parsed.HeadSHA, dryRun, printer) + }, + } + + cmd.Flags().StringVar(&repo, "repo", "", "repository in owner/repo format (required)") + cmd.Flags().IntVar(&pr, "pr", 0, "pull request number (required)") + cmd.Flags().StringVar(&result, "result", "-", "path to review result file, or '-' for stdin") + cmd.Flags().StringVar(&token, "token", "", "GitHub token (default: $GITHUB_TOKEN)") + cmd.Flags().StringVar(&headSHA, "head-sha", "", "expected PR HEAD SHA (skips review if HEAD has moved)") + cmd.Flags().BoolVar(&dryRun, "dry-run", false, "print what would be posted without making API calls") + _ = cmd.MarkFlagRequired("repo") + _ = cmd.MarkFlagRequired("pr") + + return cmd +} + +// ReviewResult represents a parsed review result file. +type ReviewResult struct { + Body string `json:"body"` + Action string `json:"action"` // "approve", "request-changes", "comment", "failure" + HeadSHA string `json:"head_sha"` // commit SHA the agent reviewed + Reason string `json:"reason"` // failure reason (when action is "failure") +} + +// reviewActionToEvent maps a ReviewResult action to a GitHub PR review event. +func reviewActionToEvent(action string) (string, bool) { + switch strings.ToLower(action) { + case "approve": + return "APPROVE", true + case "request-changes", "request_changes": + return "REQUEST_CHANGES", true + case "comment": + return "COMMENT", true + default: + return "", false + } +} + +// checkStaleHead compares the reviewed SHA against the current PR HEAD. +// Returns (stale, currentSHA, error). When stale is true, currentSHA +// contains the actual PR HEAD so callers can include it in notices +// without a redundant API call. +func checkStaleHead(ctx context.Context, client forge.Client, owner, repo string, pr int, reviewedSHA string, dryRun bool, printer *ui.Printer) (bool, string, error) { + printer.StepStart("Checking PR HEAD against reviewed SHA") + + if dryRun { + printer.StepInfo("Dry run — would check HEAD SHA") + return false, "", nil + } + + currentSHA, err := client.GetPullRequestHeadSHA(ctx, owner, repo, pr) + if err != nil { + return false, "", fmt.Errorf("fetching PR HEAD: %w", err) + } + + if !strings.EqualFold(currentSHA, reviewedSHA) { + printer.StepInfo(fmt.Sprintf("Stale: reviewed %s but HEAD is now %s", reviewedSHA[:min(len(reviewedSHA), 12)], currentSHA[:min(len(currentSHA), 12)])) + return true, currentSHA, nil + } + + printer.StepDone("HEAD matches reviewed SHA") + return false, currentSHA, nil +} + +// postStaleHeadNotice posts a failure comment when the PR HEAD has moved +// since the review was generated. +func postStaleHeadNotice(ctx context.Context, client forge.Client, owner, repo string, pr int, reviewedSHA, currentSHA string, cfg sticky.Config, printer *ui.Printer) error { + body := fmt.Sprintf(`## Review: automated review + +**Outcome:** failure +**Reason:** stale-head + +The review agent reviewed commit `+"`%s`"+` but the PR HEAD is now `+"`%s`"+`. This review was discarded to avoid approving unreviewed code.`, reviewedSHA, currentSHA) + + if err := sticky.Post(ctx, client, owner, repo, pr, body, cfg, printer); err != nil { + return fmt.Errorf("posting stale-head notice: %w", err) + } + return fmt.Errorf("review stale: reviewed %s but HEAD is now %s", reviewedSHA, currentSHA) +} + +// postFailureNotice posts a failure comment as a sticky comment. +func postFailureNotice(ctx context.Context, client forge.Client, owner, repo string, pr int, parsed ReviewResult, cfg sticky.Config, printer *ui.Printer) error { + printer.StepStart("Review agent reported failure, posting notice") + + reason := parsed.Reason + if reason == "" { + reason = "unknown" + } else if !reasonRe.MatchString(reason) { + reason = "invalid-reason" + } + + var body string + if parsed.Body != "" { + body = parsed.Body + } else { + body = fmt.Sprintf(`## Review: automated review + +**Outcome:** failure +**Reason:** %s + +This PR was NOT reviewed. Do not count this as an approval.`, reason) + } + + if err := sticky.Post(ctx, client, owner, repo, pr, body, cfg, printer); err != nil { + return fmt.Errorf("posting failure notice: %w", err) + } + printer.StepDone("Failure notice posted") + return nil +} + +// submitFormalReview minimizes stale reviews by the same user, then +// submits a new GitHub PR review. When commitSHA is non-empty, the +// review is pinned to that commit via the commit_id field, closing +// the TOCTOU gap between the stale-head check and review submission. +func submitFormalReview(ctx context.Context, client forge.Client, owner, repo string, pr int, action, commitSHA string, dryRun bool, printer *ui.Printer) error { + event, ok := reviewActionToEvent(action) + if !ok { + printer.StepInfo(fmt.Sprintf("Unknown review action %q, skipping formal review", action)) + return nil + } + + if dryRun { + printer.StepInfo(fmt.Sprintf("Dry run — would submit %s review", event)) + return nil + } + + if err := minimizeStaleReviews(ctx, client, owner, repo, pr, printer); err != nil { + return err + } + + printer.StepStart(fmt.Sprintf("Submitting %s review", event)) + reviewBody := "See the review comment above for full details." + if err := client.CreatePullRequestReview(ctx, owner, repo, pr, event, reviewBody, commitSHA); err != nil { + return fmt.Errorf("submitting review: %w", err) + } + printer.StepDone("Review submitted") + return nil +} + +// minimizeStaleReviews lists all reviews on the PR, finds previous reviews +// by the authenticated user, and minimizes them. Called before creating a +// new review, so all existing reviews by this user are stale. +func minimizeStaleReviews(ctx context.Context, client forge.Client, owner, repo string, pr int, printer *ui.Printer) error { + user, err := client.GetAuthenticatedUser(ctx) + if err != nil { + printer.StepInfo("Could not determine authenticated user, skipping stale review cleanup") + return nil + } + + reviews, err := client.ListPullRequestReviews(ctx, owner, repo, pr) + if err != nil { + printer.StepInfo("Could not list reviews, skipping stale review cleanup") + return nil + } + + var stale []forge.PullRequestReview + for _, r := range reviews { + if r.User == user { + stale = append(stale, r) + } + } + + if len(stale) == 0 { + return nil + } + + printer.StepStart(fmt.Sprintf("Minimizing %d stale review(s)", len(stale))) + for _, r := range stale { + if err := client.MinimizeComment(ctx, r.NodeID, "OUTDATED"); err != nil { + printer.StepInfo(fmt.Sprintf("Warning: could not minimize review %s: %v", r.NodeID, err)) + } + } + printer.StepDone("Stale reviews minimized") + + return nil +} + +// parseReviewResult attempts to parse the body as a JSON ReviewResult. +// If parsing fails, treats the entire input as a plain-text body. +// Returns an error if the JSON is valid but the body field is empty +// (unless the action is "failure", which may omit the body). +func parseReviewResult(input string) (ReviewResult, error) { + var result ReviewResult + if err := json.Unmarshal([]byte(input), &result); err != nil { + return ReviewResult{Body: input, Action: "comment"}, nil + } + if result.Body == "" && strings.ToLower(result.Action) != "failure" { + return ReviewResult{}, fmt.Errorf("review result JSON has empty body field") + } + if result.Action == "" { + result.Action = "comment" + } + return result, nil +} diff --git a/internal/cli/postreview_test.go b/internal/cli/postreview_test.go new file mode 100644 index 0000000000..b31e3a0864 --- /dev/null +++ b/internal/cli/postreview_test.go @@ -0,0 +1,346 @@ +package cli + +import ( + "context" + "fmt" + "io" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/fullsend-ai/fullsend/internal/forge" + "github.com/fullsend-ai/fullsend/internal/sticky" + "github.com/fullsend-ai/fullsend/internal/ui" +) + +func TestParseReviewResult_JSON(t *testing.T) { + input := `{"body": "Looks good!", "action": "approve"}` + result, err := parseReviewResult(input) + require.NoError(t, err) + assert.Equal(t, "Looks good!", result.Body) + assert.Equal(t, "approve", result.Action) +} + +func TestParseReviewResult_PlainText(t *testing.T) { + input := "This is plain text review." + result, err := parseReviewResult(input) + require.NoError(t, err) + assert.Equal(t, input, result.Body) + assert.Equal(t, "comment", result.Action) +} + +func TestParseReviewResult_DefaultAction(t *testing.T) { + input := `{"body": "Some review"}` + result, err := parseReviewResult(input) + require.NoError(t, err) + assert.Equal(t, "Some review", result.Body) + assert.Equal(t, "comment", result.Action) +} + +func TestParseReviewResult_EmptyBody(t *testing.T) { + input := `{"action": "approve"}` + _, err := parseReviewResult(input) + assert.Error(t, err) + assert.Contains(t, err.Error(), "empty body") +} + +func TestParseReviewResult_FailureAllowsEmptyBody(t *testing.T) { + input := `{"action": "failure", "reason": "tool-failure"}` + result, err := parseReviewResult(input) + require.NoError(t, err) + assert.Equal(t, "failure", result.Action) + assert.Equal(t, "tool-failure", result.Reason) + assert.Empty(t, result.Body) +} + +func TestParseReviewResult_HeadSHA(t *testing.T) { + input := `{"body": "Review", "action": "approve", "head_sha": "abc1234"}` + result, err := parseReviewResult(input) + require.NoError(t, err) + assert.Equal(t, "abc1234", result.HeadSHA) +} + +func TestReviewActionToEvent(t *testing.T) { + tests := []struct { + action string + wantEvent string + wantOK bool + }{ + {"approve", "APPROVE", true}, + {"Approve", "APPROVE", true}, + {"request-changes", "REQUEST_CHANGES", true}, + {"request_changes", "REQUEST_CHANGES", true}, + {"comment", "COMMENT", true}, + {"unknown", "", false}, + {"", "", false}, + } + for _, tt := range tests { + t.Run(tt.action, func(t *testing.T) { + event, ok := reviewActionToEvent(tt.action) + assert.Equal(t, tt.wantEvent, event) + assert.Equal(t, tt.wantOK, ok) + }) + } +} + +func TestCheckStaleHead_Matches(t *testing.T) { + fc := forge.NewFakeClient() + fc.PullRequestHeadSHA = "abc1234567890" + printer := ui.New(io.Discard) + + stale, currentSHA, err := checkStaleHead(context.Background(), fc, "o", "r", 1, "abc1234567890", false, printer) + require.NoError(t, err) + assert.False(t, stale) + assert.Equal(t, "abc1234567890", currentSHA) +} + +func TestCheckStaleHead_Stale(t *testing.T) { + fc := forge.NewFakeClient() + fc.PullRequestHeadSHA = "new_sha_456" + printer := ui.New(io.Discard) + + stale, currentSHA, err := checkStaleHead(context.Background(), fc, "o", "r", 1, "old_sha_123", false, printer) + require.NoError(t, err) + assert.True(t, stale) + assert.Equal(t, "new_sha_456", currentSHA) +} + +func TestCheckStaleHead_DryRun(t *testing.T) { + fc := forge.NewFakeClient() + printer := ui.New(io.Discard) + + stale, _, err := checkStaleHead(context.Background(), fc, "o", "r", 1, "any_sha", true, printer) + require.NoError(t, err) + assert.False(t, stale, "dry run should not report stale") +} + +func TestPostStaleHeadNotice(t *testing.T) { + fc := forge.NewFakeClient() + fc.AuthenticatedUser = "bot" + fc.PullRequestHeadSHA = "new_sha_456" + printer := ui.New(io.Discard) + + cfg := sticky.Config{Marker: ""} + err := postStaleHeadNotice(context.Background(), fc, "o", "r", 1, "old_sha_123", "new_sha_456", cfg, printer) + require.Error(t, err, "should return an error indicating staleness") + assert.Contains(t, err.Error(), "stale") + + comments := fc.IssueComments["o/r/1"] + require.Len(t, comments, 1) + assert.Contains(t, comments[0].Body, "stale-head") + assert.Contains(t, comments[0].Body, "old_sha_123") +} + +func TestPostFailureNotice_WithBody(t *testing.T) { + fc := forge.NewFakeClient() + fc.AuthenticatedUser = "bot" + printer := ui.New(io.Discard) + + cfg := sticky.Config{Marker: ""} + parsed := ReviewResult{Action: "failure", Body: "Custom failure message", Reason: "tool-failure"} + err := postFailureNotice(context.Background(), fc, "o", "r", 1, parsed, cfg, printer) + require.NoError(t, err) + + comments := fc.IssueComments["o/r/1"] + require.Len(t, comments, 1) + assert.Contains(t, comments[0].Body, "Custom failure message") +} + +func TestPostFailureNotice_WithoutBody(t *testing.T) { + fc := forge.NewFakeClient() + fc.AuthenticatedUser = "bot" + printer := ui.New(io.Discard) + + cfg := sticky.Config{Marker: ""} + parsed := ReviewResult{Action: "failure", Reason: "token-limit"} + err := postFailureNotice(context.Background(), fc, "o", "r", 1, parsed, cfg, printer) + require.NoError(t, err) + + comments := fc.IssueComments["o/r/1"] + require.Len(t, comments, 1) + assert.Contains(t, comments[0].Body, "token-limit") + assert.Contains(t, comments[0].Body, "NOT reviewed") +} + +func TestPostFailureNotice_EmptyReason(t *testing.T) { + fc := forge.NewFakeClient() + fc.AuthenticatedUser = "bot" + printer := ui.New(io.Discard) + + cfg := sticky.Config{Marker: ""} + parsed := ReviewResult{Action: "failure", Reason: ""} + err := postFailureNotice(context.Background(), fc, "o", "r", 1, parsed, cfg, printer) + require.NoError(t, err) + + comments := fc.IssueComments["o/r/1"] + require.Len(t, comments, 1) + assert.Contains(t, comments[0].Body, "unknown") + assert.Contains(t, comments[0].Body, "NOT reviewed") +} + +func TestCheckStaleHead_CaseInsensitive(t *testing.T) { + fc := forge.NewFakeClient() + fc.PullRequestHeadSHA = "abc1234567890abcdef1234567890abcdef123456" + printer := ui.New(io.Discard) + + stale, _, err := checkStaleHead(context.Background(), fc, "o", "r", 1, "ABC1234567890ABCDEF1234567890ABCDEF123456", false, printer) + require.NoError(t, err) + assert.False(t, stale, "case-insensitive SHAs should match") +} + +func TestSubmitFormalReview_CreatesAndMinimizesStale(t *testing.T) { + fc := forge.NewFakeClient() + fc.AuthenticatedUser = "fullsend-bot" + fc.PRReviews = map[string][]forge.PullRequestReview{ + "acme/repo/1": { + {ID: 100, NodeID: "PRR_100", User: "fullsend-bot", State: "COMMENTED", Body: "old review 1"}, + {ID: 200, NodeID: "PRR_200", User: "someone-else", State: "APPROVED", Body: "lgtm"}, + {ID: 300, NodeID: "PRR_300", User: "fullsend-bot", State: "APPROVED", Body: "old review 2"}, + }, + } + + printer := ui.New(io.Discard) + err := submitFormalReview(context.Background(), fc, "acme", "repo", 1, "approve", "abc123def456", false, printer) + require.NoError(t, err) + + require.Len(t, fc.CreatedReviews, 1) + assert.Equal(t, "APPROVE", fc.CreatedReviews[0].Event) + assert.Equal(t, "abc123def456", fc.CreatedReviews[0].CommitSHA) + + require.Len(t, fc.MinimizedComments, 2) + assert.Equal(t, "PRR_100", fc.MinimizedComments[0].NodeID) + assert.Equal(t, "OUTDATED", fc.MinimizedComments[0].Reason) + assert.Equal(t, "PRR_300", fc.MinimizedComments[1].NodeID) + assert.Equal(t, "OUTDATED", fc.MinimizedComments[1].Reason) +} + +func TestSubmitFormalReview_DryRun(t *testing.T) { + fc := forge.NewFakeClient() + printer := ui.New(io.Discard) + + err := submitFormalReview(context.Background(), fc, "acme", "repo", 1, "approve", "", true, printer) + require.NoError(t, err) + assert.Empty(t, fc.CreatedReviews) +} + +func TestSubmitFormalReview_UnknownAction(t *testing.T) { + fc := forge.NewFakeClient() + printer := ui.New(io.Discard) + + err := submitFormalReview(context.Background(), fc, "acme", "repo", 1, "unknown-action", "", false, printer) + require.NoError(t, err) + assert.Empty(t, fc.CreatedReviews) +} + +func TestMinimizeStaleReviews_MinimizesAll(t *testing.T) { + fc := forge.NewFakeClient() + fc.AuthenticatedUser = "fullsend-bot" + fc.PRReviews = map[string][]forge.PullRequestReview{ + "acme/repo/1": { + {ID: 100, NodeID: "PRR_100", User: "fullsend-bot", State: "APPROVED", Body: "only review"}, + }, + } + + printer := ui.New(io.Discard) + err := minimizeStaleReviews(context.Background(), fc, "acme", "repo", 1, printer) + require.NoError(t, err) + require.Len(t, fc.MinimizedComments, 1) + assert.Equal(t, "PRR_100", fc.MinimizedComments[0].NodeID) +} + +func TestMinimizeStaleReviews_ErrorTolerance(t *testing.T) { + fc := forge.NewFakeClient() + fc.AuthenticatedUser = "fullsend-bot" + fc.PRReviews = map[string][]forge.PullRequestReview{ + "acme/repo/1": { + {ID: 100, NodeID: "PRR_100", User: "fullsend-bot", State: "COMMENTED", Body: "review 1"}, + {ID: 200, NodeID: "PRR_200", User: "fullsend-bot", State: "APPROVED", Body: "review 2"}, + }, + } + fc.Errors["MinimizeComment"] = fmt.Errorf("GraphQL error") + + printer := ui.New(io.Discard) + err := minimizeStaleReviews(context.Background(), fc, "acme", "repo", 1, printer) + require.NoError(t, err, "minimizeStaleReviews should not return error when MinimizeComment fails") +} + +func TestMinimizeStaleReviews_NoReviews(t *testing.T) { + fc := forge.NewFakeClient() + fc.AuthenticatedUser = "fullsend-bot" + + printer := ui.New(io.Discard) + err := minimizeStaleReviews(context.Background(), fc, "acme", "repo", 1, printer) + require.NoError(t, err) + assert.Empty(t, fc.MinimizedComments) +} + +func TestHexSHAValidation(t *testing.T) { + tests := []struct { + sha string + valid bool + }{ + {"abc123f", false}, // too short (7 chars) + {"abc123def456", false}, // too short (12 chars) + {"abc123def456abc123def456abc123def456abcd", true}, // 40-char SHA-1 + {"abcdef0123456789abcdef0123456789abcdef0123456789abcdef0123456789", true}, // 64-char SHA-256 + {"abcdef0123456789abcdef0123456789abcdef0123456789abcdef01234567890", false}, // 65 chars (too long) + {"", true}, // empty is valid (means "no SHA provided") + {"not-hex!", false}, // non-hex chars + {"abc 123", false}, // spaces + {"abc123`inject", false}, // backtick injection + {"ABC123DEF456ABC123DEF456ABC123DEF456ABCD", true}, // uppercase 40-char + } + for _, tt := range tests { + t.Run(fmt.Sprintf("sha=%q", tt.sha), func(t *testing.T) { + if tt.sha == "" { + assert.True(t, tt.valid) + return + } + assert.Equal(t, tt.valid, hexSHARe.MatchString(tt.sha)) + }) + } +} + +func TestReasonValidation(t *testing.T) { + tests := []struct { + reason string + valid bool + }{ + {"agent-no-output", true}, + {"tool_failure", true}, + {"token-limit", true}, + {"", true}, + {"reason with spaces", false}, + {"markdown\n**injection**", false}, + {"", false}, + } + for _, tt := range tests { + t.Run(fmt.Sprintf("reason=%q", tt.reason), func(t *testing.T) { + assert.Equal(t, tt.valid, reasonRe.MatchString(tt.reason)) + }) + } +} + +func TestSubmitFormalReview_PassesCommitSHA(t *testing.T) { + fc := forge.NewFakeClient() + fc.AuthenticatedUser = "fullsend-bot" + printer := ui.New(io.Discard) + + err := submitFormalReview(context.Background(), fc, "acme", "repo", 1, "comment", "deadbeef1234", false, printer) + require.NoError(t, err) + require.Len(t, fc.CreatedReviews, 1) + assert.Equal(t, "COMMENT", fc.CreatedReviews[0].Event) + assert.Equal(t, "deadbeef1234", fc.CreatedReviews[0].CommitSHA) +} + +func TestSubmitFormalReview_EmptyCommitSHA(t *testing.T) { + fc := forge.NewFakeClient() + fc.AuthenticatedUser = "fullsend-bot" + printer := ui.New(io.Discard) + + err := submitFormalReview(context.Background(), fc, "acme", "repo", 1, "approve", "", false, printer) + require.NoError(t, err) + require.Len(t, fc.CreatedReviews, 1) + assert.Equal(t, "", fc.CreatedReviews[0].CommitSHA) +} diff --git a/internal/cli/readbody.go b/internal/cli/readbody.go new file mode 100644 index 0000000000..9bf1217edb --- /dev/null +++ b/internal/cli/readbody.go @@ -0,0 +1,43 @@ +package cli + +import ( + "fmt" + "io" + "os" +) + +const maxBodyBytes = 2 << 20 // 2 MB + +// readBody reads content from a file path or stdin (when path is "-"). +// Input is capped at maxBodyBytes to prevent resource exhaustion. +func readBody(path string) (string, error) { + return readBodyFrom(path, os.Stdin) +} + +// readBodyFrom is the testable core of readBody. It accepts an explicit +// stdin reader so callers can inject test data. +func readBodyFrom(path string, stdin io.Reader) (string, error) { + if path == "-" { + data, err := io.ReadAll(io.LimitReader(stdin, maxBodyBytes+1)) + if err != nil { + return "", fmt.Errorf("reading stdin: %w", err) + } + if len(data) > maxBodyBytes { + return "", fmt.Errorf("input exceeds maximum size of %d bytes", maxBodyBytes) + } + return string(data), nil + } + + info, err := os.Stat(path) + if err != nil { + return "", err + } + if info.Size() > maxBodyBytes { + return "", fmt.Errorf("file %s exceeds maximum size of %d bytes", path, maxBodyBytes) + } + data, err := os.ReadFile(path) + if err != nil { + return "", err + } + return string(data), nil +} diff --git a/internal/cli/readbody_test.go b/internal/cli/readbody_test.go new file mode 100644 index 0000000000..4cb43dd822 --- /dev/null +++ b/internal/cli/readbody_test.go @@ -0,0 +1,55 @@ +package cli + +import ( + "os" + "path/filepath" + "strings" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestReadBodyFrom_Stdin(t *testing.T) { + stdin := strings.NewReader("hello from stdin") + body, err := readBodyFrom("-", stdin) + require.NoError(t, err) + assert.Equal(t, "hello from stdin", body) +} + +func TestReadBodyFrom_StdinOversize(t *testing.T) { + big := strings.NewReader(strings.Repeat("x", maxBodyBytes+1)) + _, err := readBodyFrom("-", big) + require.Error(t, err) + assert.Contains(t, err.Error(), "exceeds maximum size") +} + +func TestReadBodyFrom_StdinExactLimit(t *testing.T) { + exact := strings.NewReader(strings.Repeat("x", maxBodyBytes)) + body, err := readBodyFrom("-", exact) + require.NoError(t, err) + assert.Len(t, body, maxBodyBytes) +} + +func TestReadBodyFrom_File(t *testing.T) { + tmp := filepath.Join(t.TempDir(), "body.txt") + require.NoError(t, os.WriteFile(tmp, []byte("file content"), 0o644)) + + body, err := readBodyFrom(tmp, nil) + require.NoError(t, err) + assert.Equal(t, "file content", body) +} + +func TestReadBodyFrom_FileOversize(t *testing.T) { + tmp := filepath.Join(t.TempDir(), "big.txt") + require.NoError(t, os.WriteFile(tmp, make([]byte, maxBodyBytes+1), 0o644)) + + _, err := readBodyFrom(tmp, nil) + require.Error(t, err) + assert.Contains(t, err.Error(), "exceeds maximum size") +} + +func TestReadBodyFrom_FileNotFound(t *testing.T) { + _, err := readBodyFrom("/nonexistent/path", nil) + require.Error(t, err) +} diff --git a/internal/cli/root.go b/internal/cli/root.go index 4874ae196f..5bda415b6d 100644 --- a/internal/cli/root.go +++ b/internal/cli/root.go @@ -18,6 +18,8 @@ func newRootCmd() *cobra.Command { cmd.AddCommand(newAdminCmd()) cmd.AddCommand(newRunCmd()) cmd.AddCommand(newScanCmd()) + cmd.AddCommand(newPostReviewCmd()) + cmd.AddCommand(newPostCommentCmd()) return cmd } diff --git a/internal/forge/fake.go b/internal/forge/fake.go index b0fd68d165..7d6e3ad6d9 100644 --- a/internal/forge/fake.go +++ b/internal/forge/fake.go @@ -42,6 +42,27 @@ type VariableRecord struct { Owner, Repo, Name, Value string } +// UpdatedCommentRecord records an issue comment update call. +type UpdatedCommentRecord struct { + Owner, Repo string + CommentID int + Body string +} + +// MinimizedCommentRecord records a comment minimize call. +type MinimizedCommentRecord struct { + NodeID string + Reason string +} + +// ReviewRecord records a pull request review creation call. +type ReviewRecord struct { + Owner, Repo string + Number int + Event, Body string + CommitSHA string +} + // FakeClient is a thread-safe test double for forge.Client. // Pre-populate its fields to control return values, and inspect // recorder slices after the test to verify which calls were made. @@ -69,6 +90,15 @@ type FakeClient struct { // Error injection: key is method name, value is error to return. Errors map[string]error + // Issue comments for ListIssueComments / UpdateIssueComment. + IssueComments map[string][]IssueComment // key: "owner/repo/number" + + // Pull request head SHA for GetPullRequestHeadSHA. + PullRequestHeadSHA string + + // Pull request reviews for ListPullRequestReviews. + PRReviews map[string][]PullRequestReview // key: "owner/repo/number" + // Call recorders CreatedRepos []Repository CreatedFiles []FileRecord @@ -80,9 +110,13 @@ type FakeClient struct { Variables []VariableRecord DeletedOrgSecrets []string // "org/name" CreatedOrgSecrets []OrgSecretRecord + UpdatedComments []UpdatedCommentRecord + MinimizedComments []MinimizedCommentRecord + CreatedReviews []ReviewRecord - // internal counter for change proposal numbers + // internal counters proposalCounter int + commentCounter int } // err checks for an injected error for the given method name. @@ -521,12 +555,130 @@ func (f *FakeClient) CloseIssue(_ context.Context, _, _ string, _ int) error { return f.err("CloseIssue") } -func (f *FakeClient) ListIssueComments(_ context.Context, _, _ string, _ int) ([]IssueComment, error) { +func (f *FakeClient) ListIssueComments(_ context.Context, owner, repo string, number int) ([]IssueComment, error) { f.mu.Lock() defer f.mu.Unlock() if e := f.err("ListIssueComments"); e != nil { return nil, e } + if f.IssueComments != nil { + key := fmt.Sprintf("%s/%s/%d", owner, repo, number) + if comments, ok := f.IssueComments[key]; ok { + return comments, nil + } + } + return nil, nil +} + +func (f *FakeClient) CreateIssueComment(_ context.Context, owner, repo string, number int, body string) (*IssueComment, error) { + f.mu.Lock() + defer f.mu.Unlock() + if e := f.err("CreateIssueComment"); e != nil { + return nil, e + } + f.commentCounter++ + comment := IssueComment{ + ID: f.commentCounter, + NodeID: fmt.Sprintf("IC_fake_%d", f.commentCounter), + Body: body, + Author: f.AuthenticatedUser, + CreatedAt: "2026-01-01T00:00:00Z", + } + key := fmt.Sprintf("%s/%s/%d", owner, repo, number) + if f.IssueComments == nil { + f.IssueComments = make(map[string][]IssueComment) + } + f.IssueComments[key] = append(f.IssueComments[key], comment) + return &comment, nil +} + +func (f *FakeClient) UpdateIssueComment(_ context.Context, owner, repo string, commentID int, body string) error { + f.mu.Lock() + defer f.mu.Unlock() + if e := f.err("UpdateIssueComment"); e != nil { + return e + } + f.UpdatedComments = append(f.UpdatedComments, UpdatedCommentRecord{ + Owner: owner, + Repo: repo, + CommentID: commentID, + Body: body, + }) + for key, comments := range f.IssueComments { + for i, c := range comments { + if c.ID == commentID { + f.IssueComments[key][i].Body = body + return nil + } + } + } + return nil +} + +func (f *FakeClient) MinimizeComment(_ context.Context, nodeID, reason string) error { + f.mu.Lock() + defer f.mu.Unlock() + if e := f.err("MinimizeComment"); e != nil { + return e + } + f.MinimizedComments = append(f.MinimizedComments, MinimizedCommentRecord{ + NodeID: nodeID, + Reason: reason, + }) + return nil +} + +func (f *FakeClient) GetPullRequestHeadSHA(_ context.Context, _, _ string, _ int) (string, error) { + f.mu.Lock() + defer f.mu.Unlock() + if e := f.err("GetPullRequestHeadSHA"); e != nil { + return "", e + } + return f.PullRequestHeadSHA, nil +} + +func (f *FakeClient) CreatePullRequestReview(_ context.Context, owner, repo string, number int, event, body, commitSHA string) error { + f.mu.Lock() + defer f.mu.Unlock() + if e := f.err("CreatePullRequestReview"); e != nil { + return e + } + f.CreatedReviews = append(f.CreatedReviews, ReviewRecord{ + Owner: owner, + Repo: repo, + Number: number, + Event: event, + Body: body, + CommitSHA: commitSHA, + }) + + review := PullRequestReview{ + ID: len(f.CreatedReviews) + 1000, + NodeID: fmt.Sprintf("PRR_fake_%d", len(f.CreatedReviews)+1000), + User: f.AuthenticatedUser, + State: event, + Body: body, + } + key := fmt.Sprintf("%s/%s/%d", owner, repo, number) + if f.PRReviews == nil { + f.PRReviews = make(map[string][]PullRequestReview) + } + f.PRReviews[key] = append(f.PRReviews[key], review) + return nil +} + +func (f *FakeClient) ListPullRequestReviews(_ context.Context, owner, repo string, number int) ([]PullRequestReview, error) { + f.mu.Lock() + defer f.mu.Unlock() + if e := f.err("ListPullRequestReviews"); e != nil { + return nil, e + } + if f.PRReviews != nil { + key := fmt.Sprintf("%s/%s/%d", owner, repo, number) + if reviews, ok := f.PRReviews[key]; ok { + return reviews, nil + } + } return nil, nil } diff --git a/internal/forge/forge.go b/internal/forge/forge.go index ce2cc9b78a..113b6a3ca4 100644 --- a/internal/forge/forge.go +++ b/internal/forge/forge.go @@ -58,11 +58,22 @@ type Issue struct { // IssueComment represents a comment on an issue. type IssueComment struct { ID int + NodeID string Body string Author string CreatedAt string } +// PullRequestReview represents a formal review on a pull request. +type PullRequestReview struct { + ID int + NodeID string + User string + State string // "APPROVED", "CHANGES_REQUESTED", "COMMENTED", "DISMISSED" + Body string + SubmittedAt string +} + // Installation represents an app installation on an org. type Installation struct { ID int @@ -133,6 +144,18 @@ type Client interface { CreateIssue(ctx context.Context, owner, repo, title, body string) (*Issue, error) CloseIssue(ctx context.Context, owner, repo string, number int) error ListIssueComments(ctx context.Context, owner, repo string, number int) ([]IssueComment, error) + CreateIssueComment(ctx context.Context, owner, repo string, number int, body string) (*IssueComment, error) + UpdateIssueComment(ctx context.Context, owner, repo string, commentID int, body string) error + MinimizeComment(ctx context.Context, nodeID, reason string) error + + // Pull request operations + GetPullRequestHeadSHA(ctx context.Context, owner, repo string, number int) (string, error) + + // Pull request review operations. + // commitSHA, when non-empty, pins the review to a specific commit. + // GitHub rejects the request if the commit is not the PR's current HEAD. + CreatePullRequestReview(ctx context.Context, owner, repo string, number int, event, body, commitSHA string) error + ListPullRequestReviews(ctx context.Context, owner, repo string, number int) ([]PullRequestReview, error) // Change proposal merge MergeChangeProposal(ctx context.Context, owner, repo string, number int) error diff --git a/internal/forge/github/github.go b/internal/forge/github/github.go index 8ad828efa8..d316fb9434 100644 --- a/internal/forge/github/github.go +++ b/internal/forge/github/github.go @@ -956,33 +956,211 @@ func (c *LiveClient) CloseIssue(ctx context.Context, owner, repo string, number return nil } -// ListIssueComments returns up to 100 comments on an issue (single page, no pagination). +// ListIssueComments returns all comments on an issue, paginating automatically. func (c *LiveClient) ListIssueComments(ctx context.Context, owner, repo string, number int) ([]forge.IssueComment, error) { - resp, err := c.get(ctx, fmt.Sprintf("/repos/%s/%s/issues/%d/comments?per_page=100", owner, repo, number)) + var result []forge.IssueComment + + for page := 1; page <= 100; page++ { + resp, err := c.get(ctx, fmt.Sprintf("/repos/%s/%s/issues/%d/comments?per_page=100&page=%d", owner, repo, number, page)) + if err != nil { + return nil, fmt.Errorf("list issue comments page %d: %w", page, err) + } + var raw []struct { + ID int `json:"id"` + NodeID string `json:"node_id"` + Body string `json:"body"` + User struct { + Login string `json:"login"` + } `json:"user"` + CreatedAt string `json:"created_at"` + } + if err := decodeJSON(resp, &raw); err != nil { + return nil, fmt.Errorf("decoding issue comments page %d: %w", page, err) + } + + for _, r := range raw { + result = append(result, forge.IssueComment{ + ID: r.ID, + NodeID: r.NodeID, + Body: r.Body, + Author: r.User.Login, + CreatedAt: r.CreatedAt, + }) + } + + if len(raw) < 100 { + break + } + } + return result, nil +} + +// CreateIssueComment creates a new comment on an issue or pull request. +func (c *LiveClient) CreateIssueComment(ctx context.Context, owner, repo string, number int, body string) (*forge.IssueComment, error) { + payload := map[string]string{"body": body} + resp, err := c.post(ctx, fmt.Sprintf("/repos/%s/%s/issues/%d/comments", owner, repo, number), payload) if err != nil { - return nil, fmt.Errorf("list issue comments: %w", err) + return nil, fmt.Errorf("create issue comment on #%d: %w", number, err) } - var raw []struct { - ID int `json:"id"` - Body string `json:"body"` - User struct { + var result struct { + ID int `json:"id"` + NodeID string `json:"node_id"` + Body string `json:"body"` + User struct { Login string `json:"login"` } `json:"user"` CreatedAt string `json:"created_at"` } - if err := decodeJSON(resp, &raw); err != nil { - return nil, fmt.Errorf("decoding issue comments: %w", err) + if err := decodeJSON(resp, &result); err != nil { + return nil, fmt.Errorf("decode issue comment: %w", err) + } + return &forge.IssueComment{ + ID: result.ID, + NodeID: result.NodeID, + Body: result.Body, + Author: result.User.Login, + CreatedAt: result.CreatedAt, + }, nil +} + +// UpdateIssueComment updates the body of an existing issue comment. +func (c *LiveClient) UpdateIssueComment(ctx context.Context, owner, repo string, commentID int, body string) error { + payload := map[string]string{"body": body} + resp, err := c.patch(ctx, fmt.Sprintf("/repos/%s/%s/issues/comments/%d", owner, repo, commentID), payload) + if err != nil { + return fmt.Errorf("update issue comment %d: %w", commentID, err) + } + resp.Body.Close() + return nil +} + +// MinimizeComment minimizes (hides) an issue or review comment via the +// GitHub GraphQL API. The caller provides the GraphQL node ID directly +// (available in IssueComment.NodeID and PullRequestReview.NodeID). +// The reason must be one of: ABUSE, OFF_TOPIC, OUTDATED, RESOLVED, +// DUPLICATE, SPAM. +func (c *LiveClient) MinimizeComment(ctx context.Context, nodeID, reason string) error { + switch reason { + case "ABUSE", "OFF_TOPIC", "OUTDATED", "RESOLVED", "DUPLICATE", "SPAM": + default: + return fmt.Errorf("minimize comment %s: invalid reason %q", nodeID, reason) } - comments := make([]forge.IssueComment, len(raw)) - for i, r := range raw { - comments[i] = forge.IssueComment{ - ID: r.ID, - Body: r.Body, - Author: r.User.Login, - CreatedAt: r.CreatedAt, + + query := `mutation($id: ID!, $reason: ReportedContentClassifiers!) { + minimizeComment(input: {subjectId: $id, classifier: $reason}) { + minimizedComment { isMinimized } } + }` + gqlPayload := map[string]any{ + "query": query, + "variables": map[string]string{ + "id": nodeID, + "reason": reason, + }, + } + gqlResp, err := c.post(ctx, "/graphql", gqlPayload) + if err != nil { + return fmt.Errorf("minimize comment %s: %w", nodeID, err) + } + var gqlResult struct { + Errors []struct { + Message string `json:"message"` + } `json:"errors"` + } + if err := decodeJSON(gqlResp, &gqlResult); err != nil { + return fmt.Errorf("decode minimize response: %w", err) + } + if len(gqlResult.Errors) > 0 { + return fmt.Errorf("minimize comment %s: %s", nodeID, gqlResult.Errors[0].Message) } - return comments, nil + return nil +} + +// GetPullRequestHeadSHA returns the current HEAD commit SHA of a pull request. +func (c *LiveClient) GetPullRequestHeadSHA(ctx context.Context, owner, repo string, number int) (string, error) { + resp, err := c.get(ctx, fmt.Sprintf("/repos/%s/%s/pulls/%d", owner, repo, number)) + if err != nil { + return "", fmt.Errorf("get pull request #%d: %w", number, err) + } + + var pr struct { + Head struct { + SHA string `json:"sha"` + } `json:"head"` + } + if err := decodeJSON(resp, &pr); err != nil { + return "", fmt.Errorf("decode pull request #%d: %w", number, err) + } + return pr.Head.SHA, nil +} + +// CreatePullRequestReview submits a formal review on a pull request. +// The event must be one of: APPROVE, REQUEST_CHANGES, COMMENT. +// When commitSHA is non-empty it is sent as commit_id, pinning the +// review to that commit. GitHub rejects the request if the commit is +// not the PR's current HEAD, closing the TOCTOU gap between the +// stale-head check and review submission. +func (c *LiveClient) CreatePullRequestReview(ctx context.Context, owner, repo string, number int, event, body, commitSHA string) error { + switch event { + case "APPROVE", "REQUEST_CHANGES", "COMMENT": + default: + return fmt.Errorf("create review on #%d: invalid event %q", number, event) + } + + payload := map[string]string{ + "event": event, + "body": body, + } + if commitSHA != "" { + payload["commit_id"] = commitSHA + } + resp, err := c.post(ctx, fmt.Sprintf("/repos/%s/%s/pulls/%d/reviews", owner, repo, number), payload) + if err != nil { + return fmt.Errorf("create pull request review on #%d: %w", number, err) + } + resp.Body.Close() + return nil +} + +// ListPullRequestReviews returns all reviews on a pull request, paginating automatically. +func (c *LiveClient) ListPullRequestReviews(ctx context.Context, owner, repo string, number int) ([]forge.PullRequestReview, error) { + var result []forge.PullRequestReview + + for page := 1; page <= 100; page++ { + resp, err := c.get(ctx, fmt.Sprintf("/repos/%s/%s/pulls/%d/reviews?per_page=100&page=%d", owner, repo, number, page)) + if err != nil { + return nil, fmt.Errorf("list pull request reviews page %d: %w", page, err) + } + var raw []struct { + ID int `json:"id"` + NodeID string `json:"node_id"` + User struct { + Login string `json:"login"` + } `json:"user"` + State string `json:"state"` + Body string `json:"body"` + SubmittedAt string `json:"submitted_at"` + } + if err := decodeJSON(resp, &raw); err != nil { + return nil, fmt.Errorf("decoding pull request reviews page %d: %w", page, err) + } + + for _, r := range raw { + result = append(result, forge.PullRequestReview{ + ID: r.ID, + NodeID: r.NodeID, + User: r.User.Login, + State: r.State, + Body: r.Body, + SubmittedAt: r.SubmittedAt, + }) + } + + if len(raw) < 100 { + break + } + } + return result, nil } // MergeChangeProposal squash-merges a pull request by number. diff --git a/internal/forge/github/github_comment_test.go b/internal/forge/github/github_comment_test.go new file mode 100644 index 0000000000..d30af855d6 --- /dev/null +++ b/internal/forge/github/github_comment_test.go @@ -0,0 +1,267 @@ +package github + +import ( + "context" + "encoding/json" + "net/http" + "net/http/httptest" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestCreateIssueComment(t *testing.T) { + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + assert.Equal(t, "POST", r.Method) + assert.Equal(t, "/repos/owner/repo/issues/42/comments", r.URL.Path) + + var body map[string]any + json.NewDecoder(r.Body).Decode(&body) + assert.Equal(t, "Great work!", body["body"]) + + w.WriteHeader(http.StatusCreated) + json.NewEncoder(w).Encode(map[string]any{ + "id": 123, + "body": "Great work!", + "user": map[string]any{"login": "bot"}, + "created_at": "2026-01-01T00:00:00Z", + }) + })) + defer srv.Close() + + client := newTestClient(t, srv) + comment, err := client.CreateIssueComment(context.Background(), "owner", "repo", 42, "Great work!") + require.NoError(t, err) + assert.Equal(t, 123, comment.ID) + assert.Equal(t, "Great work!", comment.Body) + assert.Equal(t, "bot", comment.Author) +} + +func TestUpdateIssueComment(t *testing.T) { + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + assert.Equal(t, "PATCH", r.Method) + assert.Equal(t, "/repos/owner/repo/issues/comments/456", r.URL.Path) + + var body map[string]any + json.NewDecoder(r.Body).Decode(&body) + assert.Equal(t, "Updated body", body["body"]) + + w.WriteHeader(http.StatusOK) + json.NewEncoder(w).Encode(map[string]any{ + "id": 456, + "body": "Updated body", + }) + })) + defer srv.Close() + + client := newTestClient(t, srv) + err := client.UpdateIssueComment(context.Background(), "owner", "repo", 456, "Updated body") + require.NoError(t, err) +} + +func TestListIssueComments_SinglePage(t *testing.T) { + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + assert.Equal(t, "GET", r.Method) + assert.Equal(t, "/repos/owner/repo/issues/10/comments", r.URL.Path) + assert.Equal(t, "100", r.URL.Query().Get("per_page")) + assert.Equal(t, "1", r.URL.Query().Get("page")) + + json.NewEncoder(w).Encode([]map[string]any{ + {"id": 1, "body": "first", "user": map[string]any{"login": "alice"}, "created_at": "2026-01-01T00:00:00Z"}, + {"id": 2, "body": "second", "user": map[string]any{"login": "bob"}, "created_at": "2026-01-02T00:00:00Z"}, + }) + })) + defer srv.Close() + + client := newTestClient(t, srv) + comments, err := client.ListIssueComments(context.Background(), "owner", "repo", 10) + require.NoError(t, err) + require.Len(t, comments, 2) + assert.Equal(t, 1, comments[0].ID) + assert.Equal(t, "first", comments[0].Body) + assert.Equal(t, "alice", comments[0].Author) + assert.Equal(t, 2, comments[1].ID) + assert.Equal(t, "second", comments[1].Body) + assert.Equal(t, "bob", comments[1].Author) +} + +func TestListIssueComments_Pagination(t *testing.T) { + page := 0 + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + page++ + assert.Equal(t, "GET", r.Method) + + switch page { + case 1: + assert.Equal(t, "1", r.URL.Query().Get("page")) + // Return exactly 100 comments to trigger pagination. + comments := make([]map[string]any, 100) + for i := range comments { + comments[i] = map[string]any{ + "id": i + 1, + "body": "comment", + "user": map[string]any{"login": "bot"}, + "created_at": "2026-01-01T00:00:00Z", + } + } + json.NewEncoder(w).Encode(comments) + case 2: + assert.Equal(t, "2", r.URL.Query().Get("page")) + // Return fewer than 100 — pagination stops. + json.NewEncoder(w).Encode([]map[string]any{ + {"id": 101, "body": "last", "user": map[string]any{"login": "bot"}, "created_at": "2026-01-02T00:00:00Z"}, + }) + default: + t.Fatalf("unexpected page %d requested", page) + } + })) + defer srv.Close() + + client := newTestClient(t, srv) + comments, err := client.ListIssueComments(context.Background(), "owner", "repo", 5) + require.NoError(t, err) + assert.Len(t, comments, 101) + assert.Equal(t, 2, page, "should have fetched exactly 2 pages") + assert.Equal(t, 101, comments[100].ID) + assert.Equal(t, "last", comments[100].Body) +} + +func TestListIssueComments_APIError(t *testing.T) { + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusNotFound) + json.NewEncoder(w).Encode(map[string]any{"message": "Not Found"}) + })) + defer srv.Close() + + client := newTestClient(t, srv) + comments, err := client.ListIssueComments(context.Background(), "owner", "repo", 999) + assert.Error(t, err) + assert.Nil(t, comments) + assert.Contains(t, err.Error(), "list issue comments page 1") +} + +func TestMinimizeComment(t *testing.T) { + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + assert.Equal(t, "POST", r.Method) + assert.Equal(t, "/graphql", r.URL.Path) + + var body map[string]any + json.NewDecoder(r.Body).Decode(&body) + assert.Contains(t, body["query"], "minimizeComment") + + vars := body["variables"].(map[string]any) + assert.Equal(t, "IC_kwDOTest", vars["id"]) + assert.Equal(t, "OUTDATED", vars["reason"]) + + w.WriteHeader(http.StatusOK) + json.NewEncoder(w).Encode(map[string]any{ + "data": map[string]any{ + "minimizeComment": map[string]any{ + "minimizedComment": map[string]any{ + "isMinimized": true, + }, + }, + }, + }) + })) + defer srv.Close() + + client := newTestClient(t, srv) + err := client.MinimizeComment(context.Background(), "IC_kwDOTest", "OUTDATED") + require.NoError(t, err) +} + +func TestMinimizeComment_GraphQLError(t *testing.T) { + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusOK) + json.NewEncoder(w).Encode(map[string]any{ + "errors": []map[string]any{ + {"message": "Could not resolve to a node with the global id of 'IC_kwDOTest'"}, + }, + }) + })) + defer srv.Close() + + client := newTestClient(t, srv) + err := client.MinimizeComment(context.Background(), "IC_kwDOTest", "OUTDATED") + require.Error(t, err) + assert.Contains(t, err.Error(), "minimize comment IC_kwDOTest") + assert.Contains(t, err.Error(), "Could not resolve to a node") +} + +func TestMinimizeComment_InvalidReason(t *testing.T) { + client := New("test-token") + err := client.MinimizeComment(context.Background(), "IC_kwDOTest", "INVALID") + require.Error(t, err) + assert.Contains(t, err.Error(), "invalid reason") +} + +func TestCreatePullRequestReview(t *testing.T) { + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + assert.Equal(t, "POST", r.Method) + assert.Equal(t, "/repos/owner/repo/pulls/7/reviews", r.URL.Path) + + var body map[string]any + json.NewDecoder(r.Body).Decode(&body) + assert.Equal(t, "APPROVE", body["event"]) + assert.Equal(t, "Looks good!", body["body"]) + assert.Equal(t, "abc123", body["commit_id"]) + + w.WriteHeader(http.StatusOK) + json.NewEncoder(w).Encode(map[string]any{"id": 999}) + })) + defer srv.Close() + + client := newTestClient(t, srv) + err := client.CreatePullRequestReview(context.Background(), "owner", "repo", 7, "APPROVE", "Looks good!", "abc123") + require.NoError(t, err) +} + +func TestCreatePullRequestReview_NoCommitSHA(t *testing.T) { + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + var body map[string]any + json.NewDecoder(r.Body).Decode(&body) + assert.Equal(t, "APPROVE", body["event"]) + _, hasCommitID := body["commit_id"] + assert.False(t, hasCommitID, "commit_id should not be present when empty") + + w.WriteHeader(http.StatusOK) + json.NewEncoder(w).Encode(map[string]any{"id": 999}) + })) + defer srv.Close() + + client := newTestClient(t, srv) + err := client.CreatePullRequestReview(context.Background(), "owner", "repo", 7, "APPROVE", "Looks good!", "") + require.NoError(t, err) +} + +func TestListPullRequestReviews(t *testing.T) { + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + assert.Equal(t, "GET", r.Method) + assert.Equal(t, "/repos/owner/repo/pulls/3/reviews", r.URL.Path) + assert.Equal(t, "100", r.URL.Query().Get("per_page")) + + json.NewEncoder(w).Encode([]map[string]any{ + { + "id": 10, + "node_id": "PRR_abc", + "user": map[string]any{"login": "reviewer"}, + "state": "APPROVED", + "body": "LGTM", + "submitted_at": "2026-01-01T00:00:00Z", + }, + }) + })) + defer srv.Close() + + client := newTestClient(t, srv) + reviews, err := client.ListPullRequestReviews(context.Background(), "owner", "repo", 3) + require.NoError(t, err) + require.Len(t, reviews, 1) + assert.Equal(t, 10, reviews[0].ID) + assert.Equal(t, "PRR_abc", reviews[0].NodeID) + assert.Equal(t, "reviewer", reviews[0].User) + assert.Equal(t, "APPROVED", reviews[0].State) + assert.Equal(t, "LGTM", reviews[0].Body) +} diff --git a/internal/scaffold/fullsend-repo/scripts/post-review.sh b/internal/scaffold/fullsend-repo/scripts/post-review.sh index 5352f44f5f..dcf2d66b74 100755 --- a/internal/scaffold/fullsend-repo/scripts/post-review.sh +++ b/internal/scaffold/fullsend-repo/scripts/post-review.sh @@ -41,18 +41,12 @@ RESULT_FILE=$(find . -maxdepth 4 -path '*/iteration-*/output/agent-result.json' if [ -z "${RESULT_FILE}" ] || [ ! -f "${RESULT_FILE}" ]; then echo "::error::No agent-result.json found — posting failure notice" - gh pr comment "${PR_NUMBER}" --repo "${REPO_FULL_NAME}" --body "$(cat <<'EOF' -## Review: automated review - -**Outcome:** failure -**Reason:** agent-no-output - -The review agent did not produce a result. This PR was NOT reviewed. -Do not count this as an approval. - -Posted by fullsend review agent -EOF -)" + echo '{"action":"failure","reason":"agent-no-output"}' | \ + fullsend post-review \ + --repo "${REPO_FULL_NAME}" \ + --pr "${PR_NUMBER}" \ + --token "${REVIEW_TOKEN}" \ + --result - exit 1 fi @@ -60,29 +54,6 @@ echo "Using result: ${RESULT_FILE}" ACTION=$(jq -r '.action' "${RESULT_FILE}") -# Guard against stale reviews: if the PR head has moved since the agent -# reviewed it (e.g. force-push during the race window after cancel-in-progress), -# refuse to post a review against unreviewed code. -if [ "${ACTION}" != "failure" ]; then - REVIEWED_SHA=$(jq -r '.head_sha // empty' "${RESULT_FILE}") - CURRENT_SHA=$(gh pr view "${PR_NUMBER}" --repo "${REPO_FULL_NAME}" --json headRefOid --jq '.headRefOid') - if [ -n "${REVIEWED_SHA}" ] && [ "${REVIEWED_SHA}" != "${CURRENT_SHA}" ]; then - echo ":⚠:Review stale: reviewed ${REVIEWED_SHA} but HEAD is now ${CURRENT_SHA}" - gh pr comment "${PR_NUMBER}" --repo "${REPO_FULL_NAME}" --body "$(cat <Posted by fullsend review agent -EOF -)" - exit 1 - fi -fi - # --------------------------------------------------------------------------- # Protected-path check: the review agent must not approve PRs that touch # sensitive paths. If the PR modifies any of these, downgrade "approve" to @@ -123,7 +94,6 @@ if [ "${ACTION}" = "approve" ]; then if [ -n "${PROTECTED_MATCHES}" ]; then echo "PR touches protected paths — downgrading approve to comment" echo "${PROTECTED_MATCHES}" | sed '/^$/d' | sed 's/^/ /' - ACTION="comment" PROTECTED_NOTICE=$'\n\n---\n\n' PROTECTED_NOTICE+=$'> **Protected paths detected** — this PR modifies files under one or more\n' @@ -136,51 +106,20 @@ if [ "${ACTION}" = "approve" ]; then PROTECTED_NOTICE+="> - \`${f}\`"$'\n' done <<< "${PROTECTED_MATCHES}" - ORIGINAL_BODY=$(jq -r '.body' "${RESULT_FILE}") - MODIFIED_BODY="${ORIGINAL_BODY}${PROTECTED_NOTICE}" - export MODIFIED_REVIEW_BODY="${MODIFIED_BODY}" + # Rewrite the result file with downgraded action and appended notice. + MODIFIED_RESULT=$(mktemp) + trap 'rm -f "${MODIFIED_RESULT}"' EXIT + jq --arg notice "${PROTECTED_NOTICE}" \ + '.action = "comment" | .body = (.body + $notice)' \ + "${RESULT_FILE}" > "${MODIFIED_RESULT}" + RESULT_FILE="${MODIFIED_RESULT}" fi fi -BODY_FILE=$(mktemp) -trap 'rm -f "${BODY_FILE}"' EXIT - -case "${ACTION}" in - approve) FLAG="--approve" ;; - request-changes) FLAG="--request-changes" ;; - comment) FLAG="--comment" ;; - failure) - REASON=$(jq -r '.reason' "${RESULT_FILE}") - BODY=$(jq -r '.body // empty' "${RESULT_FILE}") - if [ -n "${BODY}" ]; then - printf '%s' "${BODY}" > "${BODY_FILE}" - else - cat > "${BODY_FILE}" <Posted by fullsend review agent -EOF - fi - gh pr comment "${PR_NUMBER}" --repo "${REPO_FULL_NAME}" --body-file "${BODY_FILE}" - echo "Review posted: failure notice (${REASON}) on ${REPO_FULL_NAME}#${PR_NUMBER}" - exit 0 - ;; - *) - echo "::error::Unknown action '${ACTION}'" - exit 1 - ;; -esac - -if [ -n "${MODIFIED_REVIEW_BODY:-}" ]; then - printf '%s' "${MODIFIED_REVIEW_BODY}" > "${BODY_FILE}" -else - jq -r '.body' "${RESULT_FILE}" > "${BODY_FILE}" -fi -gh pr review "${PR_NUMBER}" --repo "${REPO_FULL_NAME}" "${FLAG}" --body-file "${BODY_FILE}" +fullsend post-review \ + --repo "${REPO_FULL_NAME}" \ + --pr "${PR_NUMBER}" \ + --token "${REVIEW_TOKEN}" \ + --result "${RESULT_FILE}" -echo "Review posted: ${ACTION} on ${REPO_FULL_NAME}#${PR_NUMBER}" +echo "Review posted on ${REPO_FULL_NAME}#${PR_NUMBER}" diff --git a/internal/scaffold/fullsend-repo/scripts/post-triage.sh b/internal/scaffold/fullsend-repo/scripts/post-triage.sh index 57fa3ebb6c..308b21f205 100755 --- a/internal/scaffold/fullsend-repo/scripts/post-triage.sh +++ b/internal/scaffold/fullsend-repo/scripts/post-triage.sh @@ -72,7 +72,7 @@ case "${ACTION}" in exit 1 fi echo "Posting clarifying question..." - printf '%s' "${COMMENT}" | gh issue comment "${ISSUE_NUMBER}" --repo "${REPO}" --body-file - + printf '%s' "${COMMENT}" | fullsend post-comment --repo "${REPO}" --number "${ISSUE_NUMBER}" --marker "" --token "${GH_TOKEN}" --result - echo "Applying label..." add_label "needs-info" @@ -89,7 +89,7 @@ case "${ACTION}" in exit 1 fi echo "Posting duplicate notice..." - printf '%s' "${COMMENT}" | gh issue comment "${ISSUE_NUMBER}" --repo "${REPO}" --body-file - + printf '%s' "${COMMENT}" | fullsend post-comment --repo "${REPO}" --number "${ISSUE_NUMBER}" --marker "" --token "${GH_TOKEN}" --result - echo "Applying label and closing..." add_label "duplicate" @@ -111,7 +111,7 @@ case "${ACTION}" in fi echo "Posting triage summary..." - printf '%s' "${COMMENT}" | gh issue comment "${ISSUE_NUMBER}" --repo "${REPO}" --body-file - + printf '%s' "${COMMENT}" | fullsend post-comment --repo "${REPO}" --number "${ISSUE_NUMBER}" --marker "" --token "${GH_TOKEN}" --result - echo "Applying label..." add_label "ready-to-code" diff --git a/internal/sticky/sticky.go b/internal/sticky/sticky.go new file mode 100644 index 0000000000..801183828d --- /dev/null +++ b/internal/sticky/sticky.go @@ -0,0 +1,223 @@ +package sticky + +import ( + "context" + "fmt" + "regexp" + "strconv" + "strings" + "unicode/utf8" + + "github.com/fullsend-ai/fullsend/internal/forge" + "github.com/fullsend-ai/fullsend/internal/ui" +) + +const defaultMaxSize = 65000 + +// Config controls how a sticky comment is identified and managed. +type Config struct { + Marker string // hidden HTML comment, e.g. "" + FooterMarker string // optional footer delimiter, stripped before collapsing history + MaxSize int // max comment body size (default 65000) + DryRun bool +} + +func (c Config) maxSize() int { + if c.MaxSize > 0 { + return c.MaxSize + } + return defaultMaxSize +} + +// Post implements the sticky comment lifecycle: find an existing comment +// bearing the marker, collapse old content into history, and create or +// update in-place. +func Post(ctx context.Context, client forge.Client, owner, repo string, number int, body string, cfg Config, printer *ui.Printer) error { + if strings.TrimSpace(body) == "" { + return fmt.Errorf("comment body is empty") + } + if strings.TrimSpace(cfg.Marker) == "" { + return fmt.Errorf("marker is empty") + } + + botUser, err := client.GetAuthenticatedUser(ctx) + if err != nil { + printer.StepInfo("Could not determine bot user, marker spoofing protection degraded") + } + + comments, err := client.ListIssueComments(ctx, owner, repo, number) + if err != nil { + return fmt.Errorf("listing comments: %w", err) + } + + existing := FindMarkedComment(comments, cfg.Marker, botUser) + markedBody := cfg.Marker + "\n" + body + + if existing != nil { + printer.StepStart("Found existing comment, updating in-place") + + newBody := BuildUpdatedBody(existing.Body, markedBody, cfg) + + if cfg.DryRun { + printer.StepInfo("Dry run — would update comment " + strconv.Itoa(existing.ID)) + printer.StepInfo("Body length: " + strconv.Itoa(len(newBody))) + return nil + } + + if err := client.UpdateIssueComment(ctx, owner, repo, existing.ID, newBody); err != nil { + return fmt.Errorf("updating comment: %w", err) + } + printer.StepDone("Comment updated") + } else { + printer.StepStart("No existing comment found, creating new one") + + if cfg.DryRun { + printer.StepInfo("Dry run — would create new comment") + printer.StepInfo("Body length: " + strconv.Itoa(len(markedBody))) + return nil + } + + if _, err := client.CreateIssueComment(ctx, owner, repo, number, markedBody); err != nil { + return fmt.Errorf("creating comment: %w", err) + } + printer.StepDone("Comment created") + } + + return nil +} + +// FindMarkedComment returns the first comment whose body contains the +// given marker string, or nil if none is found. When botUser is non-empty, +// only comments authored by that user are considered. This prevents +// untrusted users from spoofing the marker in their own comments. +func FindMarkedComment(comments []forge.IssueComment, marker, botUser string) *forge.IssueComment { + for i := range comments { + if botUser != "" && comments[i].Author != botUser { + continue + } + if strings.Contains(comments[i].Body, marker) { + return &comments[i] + } + } + return nil +} + +// History blocks are wrapped with sentinel comments so extraction is safe +// even when review content contains nested
tags. +const ( + historyStart = "" + historyEnd = "" +) + +// detailsRe matches history blocks using sentinel comment delimiters. +var detailsRe = regexp.MustCompile(`(?s)
\s*Previous [^<]*\s*` + regexp.QuoteMeta(historyStart) + `\s*(.*?)\s*` + regexp.QuoteMeta(historyEnd) + `\s*
`) + +// legacyDetailsRe matches old-format history blocks without sentinel comments. +var legacyDetailsRe = regexp.MustCompile(`(?s)
\s*Previous [^<]*\s*(.*?)\s*
`) + +// BuildUpdatedBody collapses the old comment body into a flat list of +//
blocks and prepends the new body. Footer content (delimited +// by FooterMarker) is stripped before collapsing and re-appended after. +func BuildUpdatedBody(oldBody, newBody string, cfg Config) string { + // Strip marker from the old body (prefix-only to avoid matching + // the marker if it appears embedded in review content). + oldContent, _ := strings.CutPrefix(oldBody, cfg.Marker+"\n") + oldContent, _ = strings.CutPrefix(oldContent, cfg.Marker) + + // Strip footer if configured. + var footer string + if cfg.FooterMarker != "" { + if idx := strings.Index(oldContent, cfg.FooterMarker); idx >= 0 { + footer = oldContent[idx:] + oldContent = strings.TrimRight(oldContent[:idx], "\n") + } + } + + // Extract existing
blocks from old content to flatten history. + // Try sentinel-delimited blocks first, fall back to legacy format. + var historyBlocks []string + matches := detailsRe.FindAllStringSubmatch(oldContent, -1) + activeRe := detailsRe + if len(matches) == 0 { + matches = legacyDetailsRe.FindAllStringSubmatch(oldContent, -1) + activeRe = legacyDetailsRe + } + for _, m := range matches { + historyBlocks = append(historyBlocks, m[1]) + } + + // The "current" old content is everything minus the old
blocks. + currentOld := activeRe.ReplaceAllString(oldContent, "") + currentOld = strings.TrimSpace(currentOld) + + // Assemble history entries and drop the oldest ones first if the + // combined body would exceed the size limit. This avoids blind + // byte-level truncation that could sever HTML sentinels and corrupt + // history on subsequent runs. + type histEntry struct { + summary string + content string + } + var entries []histEntry + if currentOld != "" { + entries = append(entries, histEntry{summary: "Previous run", content: currentOld}) + } + for i, block := range historyBlocks { + entries = append(entries, histEntry{ + summary: fmt.Sprintf("Previous run (%d)", i+2), + content: strings.TrimSpace(block), + }) + } + + formatHistory := func(ents []histEntry) string { + var b strings.Builder + for _, e := range ents { + b.WriteString(fmt.Sprintf("\n\n
\n%s\n\n", e.summary)) + b.WriteString(historyStart + "\n") + b.WriteString(e.content) + b.WriteString("\n" + historyEnd) + b.WriteString("\n\n
") + } + return b.String() + } + + footerStr := "" + if footer != "" { + footerStr = "\n\n" + footer + } + + combined := newBody + formatHistory(entries) + footerStr + + for len(combined) > cfg.maxSize() && len(entries) > 0 { + entries = entries[:len(entries)-1] + combined = newBody + formatHistory(entries) + footerStr + } + + if len(combined) > cfg.maxSize() { + combined = TruncateBody(combined, cfg.maxSize()) + } + + return combined +} + +// TruncateBody trims body to fit within maxSize, keeping the current +// content at the top and trimming history from the end. The truncation +// point is aligned to a valid UTF-8 boundary. +func TruncateBody(body string, maxSize int) string { + if len(body) <= maxSize { + return body + } + + truncationMsg := "\n\n---\n*Previous history truncated due to comment size limits.*" + budget := maxSize - len(truncationMsg) + if budget < 0 { + budget = 0 + } + + // Walk backward to a valid UTF-8 boundary. + for budget > 0 && !utf8.RuneStart(body[budget]) { + budget-- + } + + return body[:budget] + truncationMsg +} diff --git a/internal/sticky/sticky_test.go b/internal/sticky/sticky_test.go new file mode 100644 index 0000000000..c2665c37cc --- /dev/null +++ b/internal/sticky/sticky_test.go @@ -0,0 +1,296 @@ +package sticky + +import ( + "context" + "io" + "strings" + "testing" + "unicode/utf8" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/fullsend-ai/fullsend/internal/forge" + "github.com/fullsend-ai/fullsend/internal/ui" +) + +var testCfg = Config{ + Marker: "", + FooterMarker: "", +} + +func TestFindMarkedComment(t *testing.T) { + comments := []forge.IssueComment{ + {ID: 1, Body: "regular comment", Author: "human"}, + {ID: 2, Body: "\nmarked", Author: "bot"}, + {ID: 3, Body: "another comment", Author: "human"}, + } + + found := FindMarkedComment(comments, "", "bot") + require.NotNil(t, found) + assert.Equal(t, 2, found.ID) + + notFound := FindMarkedComment(comments, "", "bot") + assert.Nil(t, notFound) +} + +func TestFindMarkedComment_IgnoresSpoofedMarker(t *testing.T) { + comments := []forge.IssueComment{ + {ID: 1, Body: "\nspoofed by attacker", Author: "attacker"}, + {ID: 2, Body: "\nreal bot comment", Author: "bot"}, + } + + found := FindMarkedComment(comments, "", "bot") + require.NotNil(t, found) + assert.Equal(t, 2, found.ID, "should skip spoofed comment from attacker") +} + +func TestFindMarkedComment_EmptyBotUser(t *testing.T) { + comments := []forge.IssueComment{ + {ID: 1, Body: "\nmarked", Author: "anyone"}, + } + + found := FindMarkedComment(comments, "", "") + require.NotNil(t, found, "empty botUser should match any author") + assert.Equal(t, 1, found.ID) +} + +func TestFindMarkedComment_Empty(t *testing.T) { + found := FindMarkedComment(nil, "", "bot") + assert.Nil(t, found) +} + +func TestBuildUpdatedBody_CollapsesOldContent(t *testing.T) { + oldBody := "\nOld findings." + newBody := "\nNew findings." + + result := BuildUpdatedBody(oldBody, newBody, testCfg) + + assert.Contains(t, result, "New findings.") + assert.Contains(t, result, "
") + assert.Contains(t, result, "Previous run") + assert.Contains(t, result, "Old findings.") +} + +func TestBuildUpdatedBody_FlatHistory(t *testing.T) { + cfg := Config{Marker: ""} + + // Run 1 → Run 2 + body1 := "\nRun 1 content." + body2 := "\nRun 2 content." + result2 := BuildUpdatedBody(body1, body2, cfg) + + // Run 2 → Run 3 + body3 := "\nRun 3 content." + result3 := BuildUpdatedBody(result2, body3, cfg) + + // Run 3 → Run 4 + body4 := "\nRun 4 content." + result4 := BuildUpdatedBody(result3, body4, cfg) + + // Run 4 → Run 5 + body5 := "\nRun 5 content." + result5 := BuildUpdatedBody(result4, body5, cfg) + + // 4 re-runs should produce 4
blocks, all flat (not nested). + assert.Equal(t, 4, strings.Count(result5, "
")) + assert.Equal(t, 4, strings.Count(result5, "
")) + + // All run contents should be present. + assert.Contains(t, result5, "Run 5 content.") + assert.Contains(t, result5, "Run 4 content.") + assert.Contains(t, result5, "Run 3 content.") + assert.Contains(t, result5, "Run 2 content.") + assert.Contains(t, result5, "Run 1 content.") + + // No nested details — each
should not be inside another
. + // A simple proxy: no line should contain both
and
. + for _, line := range strings.Split(result5, "\n") { + if strings.Contains(line, "
") { + assert.NotContains(t, line, "
") + } + } +} + +func TestBuildUpdatedBody_NestedDetailsInContent(t *testing.T) { + cfg := Config{Marker: ""} + + // Run 1 content contains a
block (common in GitHub review output). + body1 := "\nReview findings:\n
\nExpanded diff\nsome diff content\n
\nEnd of review." + body2 := "\nRun 2 content." + result2 := BuildUpdatedBody(body1, body2, cfg) + + // Run 2 → Run 3 to verify content survives multiple collapses. + body3 := "\nRun 3 content." + result3 := BuildUpdatedBody(result2, body3, cfg) + + // All content should be preserved. + assert.Contains(t, result3, "Run 3 content.") + assert.Contains(t, result3, "Run 2 content.") + assert.Contains(t, result3, "some diff content") + assert.Contains(t, result3, "End of review.") +} + +func TestBuildUpdatedBody_FooterStripping(t *testing.T) { + oldBody := "\nOld review.\n\n\n_some footer info_" + newBody := "\nNew review." + + result := BuildUpdatedBody(oldBody, newBody, testCfg) + + assert.Contains(t, result, "New review.") + assert.Contains(t, result, "Old review.") + assert.Contains(t, result, "") + assert.Contains(t, result, "_some footer info_") + + // Footer should not be inside a
block. + footerIdx := strings.Index(result, "") + lastDetailsClose := strings.LastIndex(result, "
") + if lastDetailsClose >= 0 { + assert.Greater(t, footerIdx, lastDetailsClose, "footer should come after all
blocks") + } +} + +func TestBuildUpdatedBody_NoFooterMarker(t *testing.T) { + cfg := Config{Marker: ""} + oldBody := "\nOld content." + newBody := "\nNew content." + + result := BuildUpdatedBody(oldBody, newBody, cfg) + + assert.Contains(t, result, "New content.") + assert.Contains(t, result, "Old content.") +} + +func TestTruncateBody_UnderLimit(t *testing.T) { + body := "short body" + assert.Equal(t, body, TruncateBody(body, defaultMaxSize)) +} + +func TestTruncateBody_OverLimit(t *testing.T) { + body := strings.Repeat("a", defaultMaxSize+1000) + result := TruncateBody(body, defaultMaxSize) + assert.LessOrEqual(t, len(result), defaultMaxSize+100) + assert.Contains(t, result, "truncated") +} + +func TestBuildUpdatedBody_DropsOldestHistoryOnOverflow(t *testing.T) { + cfg := Config{Marker: "", MaxSize: 500} + + body1 := "\n" + strings.Repeat("A", 100) + body2 := "\n" + strings.Repeat("B", 100) + result2 := BuildUpdatedBody(body1, body2, cfg) + + body3 := "\n" + strings.Repeat("C", 100) + result3 := BuildUpdatedBody(result2, body3, cfg) + + body4 := "\n" + strings.Repeat("D", 100) + result4 := BuildUpdatedBody(result3, body4, cfg) + + assert.LessOrEqual(t, len(result4), 500, "combined body should not exceed maxSize") + assert.Contains(t, result4, strings.Repeat("D", 100), "current content must be preserved") + assert.NotContains(t, result4, "truncated", "should drop history blocks, not blind-truncate") +} + +func TestTruncateBody_RuneSafe(t *testing.T) { + // Build a string with multi-byte UTF-8 characters that would be split + // at a naive byte boundary. + prefix := strings.Repeat("a", 100) + multibyte := strings.Repeat("é", 50) // é is 2 bytes in UTF-8 + body := prefix + multibyte + strings.Repeat("b", defaultMaxSize) + + result := TruncateBody(body, 200) + + // The result must be valid UTF-8. + assert.True(t, utf8.ValidString(result), "truncated body should be valid UTF-8") + assert.Contains(t, result, "truncated") +} + +func TestPost_CreateNew(t *testing.T) { + client := forge.NewFakeClient() + client.AuthenticatedUser = "bot" + printer := ui.New(io.Discard) + + cfg := Config{Marker: ""} + err := Post(context.Background(), client, "o", "r", 1, "Body.", cfg, printer) + require.NoError(t, err) + + comments := client.IssueComments["o/r/1"] + require.Len(t, comments, 1) + assert.Contains(t, comments[0].Body, "") + assert.Contains(t, comments[0].Body, "Body.") +} + +func TestPost_UpdateExisting(t *testing.T) { + client := forge.NewFakeClient() + client.AuthenticatedUser = "bot" + client.IssueComments = map[string][]forge.IssueComment{ + "o/r/1": {{ID: 100, Body: "\nOld.", Author: "bot"}}, + } + printer := ui.New(io.Discard) + + cfg := Config{Marker: ""} + err := Post(context.Background(), client, "o", "r", 1, "New.", cfg, printer) + require.NoError(t, err) + + require.Len(t, client.UpdatedComments, 1) + assert.Equal(t, 100, client.UpdatedComments[0].CommentID) + assert.Contains(t, client.UpdatedComments[0].Body, "New.") + assert.Contains(t, client.UpdatedComments[0].Body, "
") + assert.Contains(t, client.UpdatedComments[0].Body, "Old.") +} + +func TestPost_DryRun(t *testing.T) { + client := forge.NewFakeClient() + printer := ui.New(io.Discard) + + cfg := Config{Marker: "", DryRun: true} + err := Post(context.Background(), client, "o", "r", 1, "Body.", cfg, printer) + require.NoError(t, err) + + assert.Empty(t, client.IssueComments) + assert.Empty(t, client.UpdatedComments) +} + +func TestPost_EmptyBody(t *testing.T) { + client := forge.NewFakeClient() + printer := ui.New(io.Discard) + + cfg := Config{Marker: ""} + err := Post(context.Background(), client, "o", "r", 1, "", cfg, printer) + require.Error(t, err) + assert.Contains(t, err.Error(), "empty") +} + +func TestPost_WhitespaceBody(t *testing.T) { + client := forge.NewFakeClient() + printer := ui.New(io.Discard) + + cfg := Config{Marker: ""} + err := Post(context.Background(), client, "o", "r", 1, " \n\t ", cfg, printer) + require.Error(t, err) + assert.Contains(t, err.Error(), "empty") +} + +func TestPost_EmptyMarker(t *testing.T) { + client := forge.NewFakeClient() + printer := ui.New(io.Discard) + + cfg := Config{Marker: ""} + err := Post(context.Background(), client, "o", "r", 1, "Body.", cfg, printer) + require.Error(t, err) + assert.Contains(t, err.Error(), "marker") +} + +func TestPost_DryRunExisting(t *testing.T) { + client := forge.NewFakeClient() + client.IssueComments = map[string][]forge.IssueComment{ + "o/r/1": {{ID: 100, Body: "\nOld.", Author: "bot"}}, + } + printer := ui.New(io.Discard) + + cfg := Config{Marker: "", DryRun: true} + err := Post(context.Background(), client, "o", "r", 1, "New.", cfg, printer) + require.NoError(t, err) + + assert.Empty(t, client.UpdatedComments) +}